Account management screen #153
Conversation
WalkthroughStart destination changed to Welcome; two new screens (Welcome, AccountManagement) and Steam account UI added. Logout callbacks removed from multiple components; library IDs migrated to GameSource-prefixed strings and detail pane now accepts a LibraryItem?. Pagination now clamps bounds; MainState gained isFirstLaunch. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant TopBar as AccountButton
participant Nav as NavController
participant Welcome as WelcomeScreen
participant Accounts as AccountManagementScreen
participant Steam as SteamAccountSection
participant Service as SteamService
User->>Nav: Launch app
Nav->>Welcome: startDestination = "welcome"
User->>Welcome: Tap "Set Up Accounts"
Welcome->>Nav: navigate("accounts")
Nav->>Accounts: show AccountManagementScreen
Accounts->>Steam: render SteamAccountSection
alt User signs in
User->>Steam: Tap "Sign In"
Steam->>Nav: navigate("login_user")
else User signs out
User->>Steam: Tap "Sign Out"
Steam->>Service: logOut()
Service-->>Steam: logged out
end
sequenceDiagram
autonumber
participant User
participant List as LibraryListPane
participant VM as LibraryViewModel
participant Detail as LibraryDetailPane
User->>List: Select item (appId: "STEAM_123")
List->>VM: lookup appId -> LibraryItem
VM-->>List: LibraryItem(...)
List->>Detail: show(libraryItem)
User->>Detail: Play
Detail->>VM: onClickPlay(libraryItem.gameId, ...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
app/src/main/java/app/gamenative/ui/component/dialog/state/GameFeedbackDialogState.kt (1)
38-48: Make Saver.restore resilient to missing keys (prevents crashes after app updates/process death).Hard casts (
as String/Int/...) will throw if older saved state lacks new keys (e.g.,confirmBtnText,dismissBtnText). Use safe casts with defaults and clampratinginto 0..5.Apply this diff:
- restore = { savedMap -> - GameFeedbackDialogState( - visible = savedMap["visible"] as Boolean, - appId = savedMap["appId"] as String, - rating = savedMap["rating"] as Int, - selectedTags = (savedMap["selectedTags"] as List<String>).toSet(), - feedbackText = savedMap["feedbackText"] as String, - confirmBtnText = savedMap["confirmBtnText"] as String, - dismissBtnText = savedMap["dismissBtnText"] as String, - ) - }, + restore = { savedMap -> + val visible = savedMap["visible"] as? Boolean ?: false + val appId = savedMap["appId"] as? String ?: "" + val rating = (savedMap["rating"] as? Int)?.coerceIn(0, 5) ?: 0 + val selectedTags = + (savedMap["selectedTags"] as? List<*>)?.mapNotNull { it as? String }?.toSet() + ?: emptySet() + val feedbackText = savedMap["feedbackText"] as? String ?: "" + val confirmBtnText = savedMap["confirmBtnText"] as? String ?: "Submit" + val dismissBtnText = savedMap["dismissBtnText"] as? String ?: "Close" + GameFeedbackDialogState( + visible = visible, + appId = appId, + rating = rating, + selectedTags = selectedTags, + feedbackText = feedbackText, + confirmBtnText = confirmBtnText, + dismissBtnText = dismissBtnText, + ) + },app/src/main/java/app/gamenative/utils/IntentLaunchManager.kt (1)
240-311: Boolean and default-based merge prevents overriding to “false” or other default values.
Because parse fills defaults, merge treats explicit false as “absent.” Compare against base instead of defaults.Apply this diff:
@@ - private fun mergeConfigurations(base: ContainerData, override: ContainerData): ContainerData { - // Quick return if no actual overrides - if (override == base) return base - - return ContainerData( - name = override.name.ifEmpty { base.name }, - screenSize = if (override.screenSize != Container.DEFAULT_SCREEN_SIZE) { - override.screenSize - } else { - base.screenSize - }, - envVars = if (override.envVars != Container.DEFAULT_ENV_VARS) override.envVars else base.envVars, - graphicsDriver = if (override.graphicsDriver != Container.DEFAULT_GRAPHICS_DRIVER) { - override.graphicsDriver - } else { - base.graphicsDriver - }, - graphicsDriverVersion = override.graphicsDriverVersion.ifEmpty { base.graphicsDriverVersion }, - dxwrapper = if (override.dxwrapper != Container.DEFAULT_DXWRAPPER) override.dxwrapper else base.dxwrapper, - dxwrapperConfig = when { - override.dxwrapperConfig.isNotEmpty() -> override.dxwrapperConfig - base.dxwrapperConfig.isNotEmpty() -> base.dxwrapperConfig - else -> DXVKHelper.DEFAULT_CONFIG - }, - audioDriver = if (override.audioDriver != Container.DEFAULT_AUDIO_DRIVER) override.audioDriver else base.audioDriver, - wincomponents = if (override.wincomponents != Container.DEFAULT_WINCOMPONENTS) { - override.wincomponents - } else { - base.wincomponents - }, - drives = if (override.drives != Container.DEFAULT_DRIVES) override.drives else base.drives, - execArgs = override.execArgs.ifEmpty { base.execArgs }, - executablePath = override.executablePath.ifEmpty { base.executablePath }, - installPath = override.installPath.ifEmpty { base.installPath }, - // Boolean fields: only override if different from parsing defaults - showFPS = if (override.showFPS != false) override.showFPS else base.showFPS, - launchRealSteam = if (override.launchRealSteam != false) override.launchRealSteam else base.launchRealSteam, - cpuList = if (override.cpuList != Container.getFallbackCPUList()) override.cpuList else base.cpuList, - cpuListWoW64 = if (override.cpuListWoW64 != Container.getFallbackCPUListWoW64()) { - override.cpuListWoW64 - } else { - base.cpuListWoW64 - }, - wow64Mode = if (override.wow64Mode != true) override.wow64Mode else base.wow64Mode, - startupSelection = if (override.startupSelection != Container.STARTUP_SELECTION_ESSENTIAL.toInt().toByte()) { - override.startupSelection - } else { - base.startupSelection - }, - box86Version = override.box86Version.ifEmpty { base.box86Version }, - box64Version = override.box64Version.ifEmpty { base.box64Version }, - box86Preset = override.box86Preset.ifEmpty { base.box86Preset }, - box64Preset = override.box64Preset.ifEmpty { base.box64Preset }, - desktopTheme = override.desktopTheme.ifEmpty { base.desktopTheme }, - csmt = if (override.csmt != true) override.csmt else base.csmt, - videoPciDeviceID = if (override.videoPciDeviceID != 1728) override.videoPciDeviceID else base.videoPciDeviceID, - offScreenRenderingMode = if (override.offScreenRenderingMode != "fbo") { - override.offScreenRenderingMode - } else { - base.offScreenRenderingMode - }, - strictShaderMath = if (override.strictShaderMath != true) override.strictShaderMath else base.strictShaderMath, - videoMemorySize = if (override.videoMemorySize != "2048") override.videoMemorySize else base.videoMemorySize, - mouseWarpOverride = if (override.mouseWarpOverride != "disable") override.mouseWarpOverride else base.mouseWarpOverride, - sdlControllerAPI = if (override.sdlControllerAPI != true) override.sdlControllerAPI else base.sdlControllerAPI, - enableXInput = if (override.enableXInput != true) override.enableXInput else base.enableXInput, - enableDInput = if (override.enableDInput != true) override.enableDInput else base.enableDInput, - dinputMapperType = if (override.dinputMapperType != 1.toByte()) override.dinputMapperType else base.dinputMapperType, - disableMouseInput = if (override.disableMouseInput != false) override.disableMouseInput else base.disableMouseInput, - shaderBackend = if (override.shaderBackend != "glsl") override.shaderBackend else base.shaderBackend, - useGLSL = if (override.useGLSL != "enabled") override.useGLSL else base.useGLSL, - ) - } + private fun mergeConfigurations(base: ContainerData, override: ContainerData): ContainerData { + // Prefer values that actually differ from base; this allows overriding to false/empty when intended. + return ContainerData( + name = if (override.name != base.name) override.name else base.name, + screenSize = if (override.screenSize != base.screenSize) override.screenSize else base.screenSize, + envVars = if (override.envVars != base.envVars) override.envVars else base.envVars, + graphicsDriver = if (override.graphicsDriver != base.graphicsDriver) override.graphicsDriver else base.graphicsDriver, + graphicsDriverVersion = if (override.graphicsDriverVersion != base.graphicsDriverVersion) override.graphicsDriverVersion else base.graphicsDriverVersion, + dxwrapper = if (override.dxwrapper != base.dxwrapper) override.dxwrapper else base.dxwrapper, + dxwrapperConfig = when { + override.dxwrapperConfig.isNotEmpty() -> override.dxwrapperConfig + base.dxwrapperConfig.isNotEmpty() -> base.dxwrapperConfig + else -> DXVKHelper.DEFAULT_CONFIG + }, + audioDriver = if (override.audioDriver != base.audioDriver) override.audioDriver else base.audioDriver, + wincomponents = if (override.wincomponents != base.wincomponents) override.wincomponents else base.wincomponents, + drives = if (override.drives != base.drives) override.drives else base.drives, + execArgs = if (override.execArgs != base.execArgs) override.execArgs else base.execArgs, + executablePath = if (override.executablePath != base.executablePath) override.executablePath else base.executablePath, + installPath = if (override.installPath != base.installPath) override.installPath else base.installPath, + showFPS = if (override.showFPS != base.showFPS) override.showFPS else base.showFPS, + launchRealSteam = if (override.launchRealSteam != base.launchRealSteam) override.launchRealSteam else base.launchRealSteam, + cpuList = if (override.cpuList != base.cpuList) override.cpuList else base.cpuList, + cpuListWoW64 = if (override.cpuListWoW64 != base.cpuListWoW64) override.cpuListWoW64 else base.cpuListWoW64, + wow64Mode = if (override.wow64Mode != base.wow64Mode) override.wow64Mode else base.wow64Mode, + startupSelection = if (override.startupSelection != base.startupSelection) override.startupSelection else base.startupSelection, + box86Version = if (override.box86Version != base.box86Version) override.box86Version else base.box86Version, + box64Version = if (override.box64Version != base.box64Version) override.box64Version else base.box64Version, + box86Preset = if (override.box86Preset != base.box86Preset) override.box86Preset else base.box86Preset, + box64Preset = if (override.box64Preset != base.box64Preset) override.box64Preset else base.box64Preset, + desktopTheme = if (override.desktopTheme != base.desktopTheme) override.desktopTheme else base.desktopTheme, + csmt = if (override.csmt != base.csmt) override.csmt else base.csmt, + videoPciDeviceID = if (override.videoPciDeviceID != base.videoPciDeviceID) override.videoPciDeviceID else base.videoPciDeviceID, + offScreenRenderingMode = if (override.offScreenRenderingMode != base.offScreenRenderingMode) override.offScreenRenderingMode else base.offScreenRenderingMode, + strictShaderMath = if (override.strictShaderMath != base.strictShaderMath) override.strictShaderMath else base.strictShaderMath, + videoMemorySize = if (override.videoMemorySize != base.videoMemorySize) override.videoMemorySize else base.videoMemorySize, + mouseWarpOverride = if (override.mouseWarpOverride != base.mouseWarpOverride) override.mouseWarpOverride else base.mouseWarpOverride, + sdlControllerAPI = if (override.sdlControllerAPI != base.sdlControllerAPI) override.sdlControllerAPI else base.sdlControllerAPI, + enableXInput = if (override.enableXInput != base.enableXInput) override.enableXInput else base.enableXInput, + enableDInput = if (override.enableDInput != base.enableDInput) override.enableDInput else base.enableDInput, + dinputMapperType = if (override.dinputMapperType != base.dinputMapperType) override.dinputMapperType else base.dinputMapperType, + disableMouseInput = if (override.disableMouseInput != base.disableMouseInput) override.disableMouseInput else base.disableMouseInput, + shaderBackend = if (override.shaderBackend != base.shaderBackend) override.shaderBackend else base.shaderBackend, + useGLSL = if (override.useGLSL != base.useGLSL) override.useGLSL else base.useGLSL, + ) + }app/src/main/java/app/gamenative/service/SteamService.kt (1)
729-731: Wrong map method; can start duplicate downloads
downloadJobs.contains(appId)checks values (legacy Javacontains) not keys. UsecontainsKey.Fix:
- if (downloadJobs.contains(appId)) return getAppDownloadInfo(appId) + if (downloadJobs.containsKey(appId)) return getAppDownloadInfo(appId)app/src/main/java/app/gamenative/ui/screen/library/components/LibraryList.kt (1)
55-61: Use stable key: appId, not indexUsing index as item key risks incorrect item state reuse when filtering/paginating. Key by appId.
Apply this diff:
- items(items = list, key = { it.index }) { item -> + items(items = list, key = { it.appId }) { item ->app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (1)
1201-1213: Bug: passing String containerId to Steam’s -applaunch. Must be numeric AppID.appId here is a String like "STEAM_12345". Steam expects a numeric AppID. Use the extracted gameId.
Apply this diff:
- val args = if (bootToContainer || appLaunchInfo == null) { + val gameId = ContainerUtils.extractGameIdFromContainerId(appId) + val args = if (bootToContainer || appLaunchInfo == null) { "\"wfm.exe\"" } else { // Check if we should launch through real Steam if (container.isLaunchRealSteam()) { // Launch Steam with the applaunch parameter to start the game - "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " + - "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $appId" + "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " + + "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $gameId" } else { // Original logic for direct game launch - val gameId = ContainerUtils.extractGameIdFromContainerId(appId) val appDirPath = SteamService.getAppDirPath(gameId)app/src/main/java/app/gamenative/utils/SteamUtils.kt (4)
135-185: Wrong value written to steam_appid.txt (breaks loader).ensureSteamSettings writes the appId it receives straight into steam_appid.txt. Calling it with libraryItem.appId ("STEAM_12345") writes a non‑numeric value. Pass the numeric gameId and keep container lookups using the containerId.
Apply this diff to calls:
- ensureSteamSettings(context, it, libraryItem.appId) + ensureSteamSettings(context, it, libraryItem) @@ - ensureSteamSettings(context, it, libraryItem.appId) + ensureSteamSettings(context, it, libraryItem)
634-669: Update ensureSteamSettings to accept LibraryItem, write numeric AppID, read container by containerId.- private fun ensureSteamSettings(context: Context, dllPath: Path, appId: String) { + private fun ensureSteamSettings(context: Context, dllPath: Path, libraryItem: LibraryItem) { + val containerId = libraryItem.appId + val steamAppIdStr = libraryItem.gameId.toString() val appIdFileUpper = dllPath.parent.resolve("steam_appid.txt") if (Files.notExists(appIdFileUpper)) { Files.createFile(appIdFileUpper) - appIdFileUpper.toFile().writeText(appId) + appIdFileUpper.toFile().writeText(steamAppIdStr) } val settingsDir = dllPath.parent.resolve("steam_settings") if (Files.notExists(settingsDir)) { Files.createDirectories(settingsDir) } val appIdFile = settingsDir.resolve("steam_appid.txt") if (Files.notExists(appIdFile)) { Files.createFile(appIdFile) - appIdFile.toFile().writeText(appId) + appIdFile.toFile().writeText(steamAppIdStr) } @@ - val language = runCatching { - val container = ContainerUtils.getOrCreateContainer(context, appId) + val language = runCatching { + val container = ContainerUtils.getOrCreateContainer(context, containerId)
511-591: Mirror the steam_appid.txt fix in restoreSteamApi.After updating ensureSteamSettings signature, update this call as well:
- Timber.i("Starting restoreSteamApi for appId: $libraryItem.appId") + Timber.i("Starting restoreSteamApi for appId: ${libraryItem.appId}") @@ - setupLightweightSteamConfig(imageFs, SteamService.userSteamId!!.accountID.toString()) + setupLightweightSteamConfig(imageFs, SteamService.userSteamId!!.accountID.toString()) @@ - ensureSteamSettings(context, it, libraryItem.appId) + ensureSteamSettings(context, it, libraryItem)
145-148: Use 32‑bit AccountID for Steam userdata folderSteam userdata folders are named with the 32‑bit AccountID — update both call sites to pass the AccountID string.
File: app/src/main/java/app/gamenative/utils/SteamUtils.kt — lines 145–148 and 539–541
- setupLightweightSteamConfig(imageFs, SteamService.userSteamId?.toString()) + setupLightweightSteamConfig(imageFs, SteamService.userSteamId?.accountID?.toString())app/src/main/java/app/gamenative/ui/model/MainViewModel.kt (1)
303-321: Potential infinite loop when walking parent windows (uses window.parent instead of currentWindow.parent).Each iteration re-reads window.parent, not currentWindow.parent, which can loop forever if the first parent isn’t null/explorer.exe.
Apply this diff:
- var currentWindow: Window = window - do { - var parentWindow: Window? = window.parent - val process = if (parentWindow != null && parentWindow.className.lowercase() != "explorer.exe") { - val processId = currentWindow.processId - val parentProcessId = parentWindow.processId - currentWindow = parentWindow - - AppProcessInfo(processId, parentProcessId, false) - } else { - parentWindow = null - - AppProcessInfo(currentWindow.processId, steamProcessId, true) - } - processes.add(process) - } while (parentWindow != null) + var currentWindow: Window = window + while (true) { + val parent = currentWindow.parent + if (parent != null && parent.className.lowercase() != "explorer.exe") { + processes.add(AppProcessInfo(currentWindow.processId, parent.processId, false)) + currentWindow = parent + } else { + processes.add(AppProcessInfo(currentWindow.processId, steamProcessId, true)) + break + } + }app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt (1)
121-131: TextStyle.copy(brush=...) requires a newer Compose than your BOM providesYour libs.versions.toml uses compose-bom 2025.01.01 (the BOM manages androidx.compose.ui:ui = 1.7.7), while TextStyle.copy(brush=...) was added in Compose UI 1.10.0-alpha02 (TextStyle(brush=...) landed in 1.9.0) — style.copy(brush = ...) will not compile under UI 1.7.x. Upgrade the Compose BOM to a version that includes UI >= 1.10 (or adjust the code to apply a TextStyle with brush via TextStyle(brush=...)/merge or use SpanStyle/AnnotatedString). (mvnrepository.com)
Location: app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt (lines 121–131)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (2)
229-234: Possible NPE on GPU vendor lookup.
getGPUCards(context)[deviceId]!! can be null on unknown IDs → crash.Apply:
- registryEditor.setDwordValue( - "Software\\Wine\\Direct3D", - "VideoPciVendorID", - getGPUCards(context)[containerData.videoPciDeviceID]!!.vendorId, - ) + val vendorId = getGPUCards(context)[containerData.videoPciDeviceID]?.vendorId + if (vendorId != null) { + registryEditor.setDwordValue("Software\\Wine\\Direct3D", "VideoPciVendorID", vendorId) + } else { + Timber.w("Unknown VideoPciDeviceID=%d; skipping VideoPciVendorID", containerData.videoPciDeviceID) + }
454-492: ANR risk: runBlocking + Future.get() on UI thread.
createNewContainer can be called from UI; blocking up to 10s is unsafe. Fetch DX version asynchronously after initial apply.Apply:
- // No custom config, so determine the DX wrapper synchronously - runBlocking { - try { - Timber.i("Fetching DirectX version synchronously for app $appId") - - // Create CompletableDeferred to wait for result - val deferred = kotlinx.coroutines.CompletableDeferred<Int>() - - // Start the async fetch but wait for it to complete - SteamUtils.fetchDirect3DMajor(gameId) { dxVersion -> - deferred.complete(dxVersion) - } - - // Wait for the result with a timeout - val dxVersion = try { - withTimeout(10000) { deferred.await() } - } catch (e: Exception) { - Timber.w(e, "Timeout waiting for DirectX version") - -1 // Default on timeout - } - - // Set wrapper based on DirectX version - val newDxWrapper = when { - dxVersion == 12 -> "vkd3d" - dxVersion in 1..8 -> "wined3d" - else -> containerData.dxwrapper // Keep existing for DX10/11 or errors - } - - // Update the wrapper if needed - if (newDxWrapper != containerData.dxwrapper) { - Timber.i("Setting DX wrapper for app $appId to $newDxWrapper (DirectX version: $dxVersion)") - containerData.dxwrapper = newDxWrapper - } - } catch (e: Exception) { - Timber.w(e, "Error determining DirectX version: ${e.message}") - // Continue with default wrapper on error - } - } - - // Apply container data with the determined DX wrapper - applyToContainer(context, container, containerData) + // Apply defaults immediately; detect DX version in background and update if needed + applyToContainer(context, container, containerData) + java.util.concurrent.Executors.newSingleThreadExecutor().execute { + try { + SteamUtils.fetchDirect3DMajor(gameId) { dxVersion -> + val newDxWrapper = when { + dxVersion == 12 -> "vkd3d" + dxVersion in 1..8 -> "wined3d" + else -> containerData.dxwrapper + } + if (newDxWrapper != containerData.dxwrapper) { + Timber.i("Setting DX wrapper for app $appId to $newDxWrapper (DirectX version: $dxVersion)") + containerData.dxwrapper = newDxWrapper + applyToContainer(context, container, containerData, saveToDisk = true) + } + } + } catch (e: Exception) { + Timber.w(e, "Error determining DirectX version") + } + } return containerAlso applies to: 494-496
🧹 Nitpick comments (52)
app/src/main/java/app/gamenative/ui/component/dialog/state/GameFeedbackDialogState.kt (5)
11-12: Localize button labels.Hardcoded English strings will block i18n. Source these via string resources and pass them into state.
8-8: Enforce rating bounds.Add a guard or annotation to keep
ratingwithin 0..5. You already clamp in restore (see above); enforce at construction too.Example:
data class GameFeedbackDialogState( val visible: Boolean, val appId: String = "", - val rating: Int = 0, // 0-5 stars, 0 means no selection + val rating: Int = 0, // 0-5 stars, 0 means no selection ... ) { + init { + require(rating in 0..5) { "rating must be between 0 and 5" } + }
7-7: Avoid empty-string sentinel for IDs when shown.If the dialog is visible, an empty
appIdis likely a bug. Guard it.) { + init { + require(!visible || appId.isNotBlank()) { "appId must be set when dialog is visible" } + }
16-24: Model tags as a type-safe enum or @stringdef.Raw strings are typo-prone and hard to evolve. Consider
enum class GameFeedbackTag(with wire values) or@StringDeffor compile-time checks.
26-36: Extract Saver keys into constants.Reduces risk of key typos across save/restore.
Apply this diff within the block:
- "visible" to state.visible, - "appId" to state.appId, - "rating" to state.rating, - "selectedTags" to state.selectedTags.toList(), - "feedbackText" to state.feedbackText, - "confirmBtnText" to state.confirmBtnText, - "dismissBtnText" to state.dismissBtnText, + KEY_VISIBLE to state.visible, + KEY_APP_ID to state.appId, + KEY_RATING to state.rating, + KEY_SELECTED_TAGS to state.selectedTags.toList(), + KEY_FEEDBACK_TEXT to state.feedbackText, + KEY_CONFIRM_BTN_TEXT to state.confirmBtnText, + KEY_DISMISS_BTN_TEXT to state.dismissBtnText,And add these constants (outside the selected range):
private const val KEY_VISIBLE = "visible" private const val KEY_APP_ID = "appId" private const val KEY_RATING = "rating" private const val KEY_SELECTED_TAGS = "selectedTags" private const val KEY_FEEDBACK_TEXT = "feedbackText" private const val KEY_CONFIRM_BTN_TEXT = "confirmBtnText" private const val KEY_DISMISS_BTN_TEXT = "dismissBtnText"app/src/main/java/app/gamenative/utils/IntentLaunchManager.kt (7)
61-87: Override application flow is reasonable; watch original backup timing.
If the container is created after this call, no original backup is taken. Consider backing up on first “apply to container” as part of the launch path.
111-114: Clearing override also drops original backup—surprising side effect.
Consider adding a restoreAndClear(appId) helper or documenting the order-of-operations.
121-132: Optionally clear stored state after successful restore.
Prevents stale originals accumulating.Apply this diff if desired:
@@ ContainerUtils.applyToContainer(context, container, originalConfig, saveToDisk = false) Timber.i("[IntentLaunchManager]: Restored original configuration for app $appId") + TemporaryConfigStore.clearOverride(appId) // also clears original snapshot
150-170: Tighten validation regexes; current drives pattern accepts empty/concatenated entries.
Small hardening; keep messages the same.Apply this diff:
@@ - if (!config.screenSize.matches(Regex("\\d+x\\d+"))) { + if (!config.screenSize.matches(Regex("^[1-9]\\d{1,4}x[1-9]\\d{1,4}$"))) { @@ - if (config.cpuList.isNotEmpty() && !config.cpuList.matches(Regex("\\d+(,\\d+)*"))) { + if (config.cpuList.isNotEmpty() && !config.cpuList.matches(Regex("^\\d+(,\\d+)*$"))) { @@ - if (!config.videoMemorySize.matches(Regex("\\d+"))) { + if (!config.videoMemorySize.matches(Regex("^\\d{1,6}$"))) { @@ - if (config.drives.isNotEmpty() && !config.drives.matches(Regex("([A-Z]:([^:]+))*"))) { + if (config.drives.isNotEmpty() && !config.drives.matches(Regex("^[A-Z]:[^,;:]+([,;][A-Z]:[^,;:]+)*$"))) {If Container expects a different delimiter for drives, align the regex accordingly.
172-239: JSON parsing is robust; consider opt helpers to reduce exceptions and document size as “chars.”*
Non-blocking. Using JSONObject.opt* would avoid try/catch churn; comment that MAX_CONFIG_JSON_SIZE is characters, not bytes.
315-349: String-keyed stores with synchronization look good; consider bounding memory.
If many appIds churn, consider LRU or time-based eviction.
27-59: Intent source trust: ensure the receiver enforcing this parser is permission-gated.
If ACTION_LAUNCH_GAME is exported, require a signature-level permission or validate caller; otherwise any app can push overrides.Please confirm the Intent receiver/activity is either android:exported="false" or protected by a custom permission.
app/src/main/java/app/gamenative/service/SteamService.kt (2)
429-445: Guard against empty/unsafe install dir namesIf
installDirandnameare blank or contain slashes, this can resolve to the root "common" dir. Sanitize and provide a fallback.Apply:
- fun getAppDirPath(gameId: Int): String { - - val appName = getAppDirName(getAppInfoOf(gameId)) + fun getAppDirPath(gameId: Int): String { + val appInfo = getAppInfoOf(gameId) + val rawName = getAppDirName(appInfo).ifBlank { "app_$gameId" } + // strip any path separators just in case + val appName = rawName.replace(Regex("""[\\/]+"""), "_")
738-743: Timeout comment vs value mismatchComment says “5 s” but code uses 1,000 ms. Align both.
Choose one:
- withTimeout(1_000) { // 5 s is enough for a normal reply + withTimeout(5_000) { // 5 s is enough for a normal replyor
- withTimeout(1_000) { // 5 s is enough for a normal reply + withTimeout(1_000) { // 1 s is enough for a normal replyapp/src/main/java/app/gamenative/ui/component/dialog/ProfileDialog.kt (2)
11-11: Remove unused import
Logouticon is no longer used.-import androidx.compose.material.icons.automirrored.filled.Logout
116-117: Externalize hardcoded strings for i18n"Manage Accounts", "Settings", "Help & Support", "Go Online", "Close" should use
stringResource(...).Example:
-Text(text = "Manage Accounts") +Text(text = stringResource(R.string.manage_accounts))Replicate for the others and add keys to
strings.xml.Also applies to: 122-123, 128-129, 135-136, 142-143
app/src/main/java/app/gamenative/utils/ContainerMigrator.kt (2)
129-133: Symlink target check is brittle
canonicalPath.endsWith(legacyId)can false‑match. Prefer checking for/xuser-$legacyIdsuffix (or reading the link target if available).- if (activeSymlink.exists() && activeSymlink.canonicalPath.endsWith(legacyId)) { + if (activeSymlink.exists() && activeSymlink.canonicalPath.endsWith("/${ImageFs.USER}-$legacyId")) {
85-94: DRY: extract legacy-container discoveryThe filter logic is duplicated; extract a private helper
findLegacyContainers(homeDir: File): List<File>.Also applies to: 172-181
app/src/main/java/app/gamenative/events/AndroidEvent.kt (1)
16-18: Document expected appId format or use a dedicated value classappId is now a String in AndroidEvent/MainUiEvent and is passed as a string at all call sites I found (AndroidEvent.kt; MainViewModel.kt — onExternalGameLaunch/_uiEvent sends; MainActivity.kt — emits ExternalGameLaunch; PluviaMain.kt — listeners for PromptSaveContainerConfig/ShowGameFeedback). No usages treating appId as an Int (no toInt/toLong conversions or arithmetic) were found.
Recommendation (optional): either document the canonical appId format (e.g., "STEAM_12345") or introduce an inline value class (e.g., @JvmInline value class AppId(val value: String)) and use it for the event/data classes to prevent mixing arbitrary strings.
app/src/main/java/app/gamenative/ui/data/MainState.kt (1)
18-18: Replace empty-string sentinel for launchedAppId with a typed ID or nullableChange val launchedAppId: String = "" to either a value class/typealias (e.g., AppId) or String? = null to avoid a magic empty-string leaking into persistence/UI; update callers to handle null/typed ID.
Locations to update:
- app/src/main/java/app/gamenative/ui/data/MainState.kt (launchedAppId)
- app/src/main/java/app/gamenative/ui/model/MainViewModel.kt (initialization -> launchedAppId = "" and setLaunchedAppId)
- app/src/main/java/app/gamenative/ui/PluviaMain.kt (createLibraryItemFromAppId(state.launchedAppId), XServerScreen(appId = state.launchedAppId), onWindowMapped/onExit/onConfirmClick sites)
Minimal migration: introduce AppId (typealias or value class), make launchedAppId: AppId? = null, change setLaunchedAppId to accept AppId/nullable, and null-guard or adapt createLibraryItemFromAppId/XServerScreen.
app/src/main/java/app/gamenative/ui/screen/library/components/LibrarySearchBar.kt (1)
140-140: Centralize container/appId constructionAvoid manual string interpolation for IDs to prevent drift. Prefer a single factory/helper (e.g., ContainerUtils.buildContainerId(GameSource.STEAM, item.id)) reused across the app, even in previews.
app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java (1)
230-232: Guard against non‑Steam containers before deriving gameIdIf a container id isn’t STEAM_* (or is malformed), extractGameIdFromContainerId may throw or map to a wrong path. Check the source first and skip non‑Steam containers.
Apply this diff:
@@ - try { - int gameId = ContainerUtils.INSTANCE.extractGameIdFromContainerId(container.id); - String mappedPath = SteamService.Companion.getAppDirPath(gameId); + try { + // Only applicable to Steam-backed containers + if (app.gamenative.data.GameSource.STEAM != ContainerUtils.INSTANCE.extractGameSourceFromContainerId(container.id)) { + continue; + } + int gameId = ContainerUtils.INSTANCE.extractGameIdFromContainerId(container.id); + String mappedPath = SteamService.Companion.getAppDirPath(gameId);Also add the missing import:
@@ -import app.gamenative.utils.ContainerUtils; +import app.gamenative.utils.ContainerUtils; +import app.gamenative.data.GameSource;app/src/main/java/app/gamenative/utils/GameFeedbackUtils.kt (1)
47-51: Parameter name suggests containerId, not appIdThe function expects a platform‑prefixed String and derives gameId. Consider renaming to containerId for clarity (non‑blocking).
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt (2)
55-60: Key remember on gameId; align dependencies with data sourceremember() keys should reflect the inputs used. Use gameId instead of appId for both downloadInfo and installed state.
Apply this diff:
- val downloadInfo = remember(appInfo.appId) { SteamService.getAppDownloadInfo(appInfo.gameId) } + val downloadInfo = remember(appInfo.gameId) { SteamService.getAppDownloadInfo(appInfo.gameId) } @@ - val isInstalled = remember(appInfo.appId) { - SteamService.isAppInstalled(appInfo.gameId) - } + val isInstalled = remember(appInfo.gameId) { SteamService.isAppInstalled(appInfo.gameId) }
216-217: Preview: centralize ID constructionSame note as elsewhere: prefer a helper for building STEAM_* IDs to avoid string duplication.
app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt (4)
158-160: Define a clear secondary sort for stabilitysortedWith(compareByDescending(installed)) relies on upstream alphabetical order. Add an explicit thenBy { it.name } to make intent clear and stable regardless of DAO ordering.
Example:
- .sortedWith( - // Comes from DAO in alphabetical order - compareByDescending<SteamApp> { downloadDirectoryApps.contains(SteamService.getAppDirName(it)) }, - ) + .sortedWith( + compareByDescending<SteamApp> { downloadDirectoryApps.contains(SteamService.getAppDirName(it)) } + .thenBy { it.name }, + )
163-175: Avoid double traversal of the filtered sequencecount() and then take() re-run the full pipeline twice. Materialize once, then reuse.
Apply this diff:
- // Total count for the current filter - val totalFound = filteredList.count() + // Materialize once to avoid re-running the pipeline + val filtered = filteredList.toList() + val totalFound = filtered.size @@ - val pagedSequence = filteredList.take(endIndex) + val pagedSequence = filtered.asSequence().take(endIndex)
178-182: Centralize STEAM_ appId construction*Use a single helper to build container/app IDs to avoid scattering the format across the codebase.
Example:
- appId = "${GameSource.STEAM.name}_${item.id}", + appId = ContainerUtils.buildContainerId(GameSource.STEAM, item.id),If such a helper doesn’t exist yet, consider adding it to ContainerUtils.
186-194: Minor: log includes totalFound; consider also logging page infoNot required, but logging current/last pages can help diagnose pagination.
app/src/main/java/app/gamenative/ui/screen/accounts/SteamAccountSection.kt (2)
14-28: Login state can go stale; logOut likely should not run on the UI thread.
- The state is initialized once from SteamService.isLoggedIn and never updated after a successful login elsewhere, so the tile may not reflect current auth state.
- Also, if SteamService.logOut() performs I/O, call it off the main thread.
Apply this diff to make logout non-blocking and prep for observing a stream later:
@Composable fun SteamAccountSection( onNavigateRoute: (String) -> Unit, modifier: Modifier = Modifier, ) { - val isSteamLoggedIn = remember { mutableStateOf(SteamService.isLoggedIn)} + val isSteamLoggedIn = remember { mutableStateOf(SteamService.isLoggedIn) } + val scope = rememberCoroutineScope() @@ onLogin = { onNavigateRoute(PluviaScreen.LoginUser.route) }, onLogout = { - SteamService.logOut() - isSteamLoggedIn.value = false // Trigger a redraw + scope.launch { + // If logOut() is blocking, this keeps the UI responsive. + SteamService.logOut() + isSteamLoggedIn.value = false + } },Consider exposing a Flow/State from SteamService (e.g., loginStateFlow) and collecting it here to keep isLoggedIn in sync.
7-7: Unused import.kotlinx.coroutines.launch is unused prior to the suggested change; remove it if you don’t adopt the coroutine change above.
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryDetailPane.kt (1)
75-80: Preview data: safe but consider smaller sentinel.Using Int.MAX_VALUE for a preview appId works, but a small constant (e.g., 999999) makes logs easier to skim.
app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (3)
223-231: Redundant state write in download progress handler.You set isInstalled twice (once from SteamService, then true). Drop the second assignment.
- if (it >= 1f) { - isInstalled = SteamService.isAppInstalled(gameId) - downloadInfo = null - isInstalled = true - MarkerUtils.addMarker(getAppDirPath(gameId), Marker.DOWNLOAD_COMPLETE_MARKER) - } + if (it >= 1f) { + isInstalled = SteamService.isAppInstalled(gameId) + downloadInfo = null + MarkerUtils.addMarker(getAppDirPath(gameId), Marker.DOWNLOAD_COMPLETE_MARKER) + }
533-538: Avoid potential main‑thread I/O when starting downloads.Mixed usage: some paths dispatch to IO, others call SteamService.downloadApp(gameId) directly. Standardize to IO to prevent UI jank.
- } else { - downloadInfo = SteamService.downloadApp(gameId) - } + } else { + CoroutineScope(Dispatchers.IO).launch { + downloadInfo = SteamService.downloadApp(gameId) + } + } @@ - onUpdateClick = { CoroutineScope(Dispatchers.IO).launch { downloadInfo = SteamService.downloadApp(gameId) } }, + onUpdateClick = { + CoroutineScope(Dispatchers.IO).launch { + downloadInfo = SteamService.downloadApp(gameId) + } + }, @@ - CoroutineScope(Dispatchers.IO).launch { - downloadInfo = SteamService.downloadApp(gameId) - } + CoroutineScope(Dispatchers.IO).launch { + downloadInfo = SteamService.downloadApp(gameId) + }Also applies to: 550-550, 623-632
122-125: Duplicate imports.androidx.compose.runtime getValue/setValue/mutableStateOf are imported twice. Remove the duplicates to reduce noise.
app/src/main/java/app/gamenative/utils/SteamUtils.kt (1)
302-341: Future-proofing: avoid hardcoding "STEAM_" when resolving container.restoreUnpackedExecutable constructs "STEAM_$appId". Prefer passing a containerId (or LibraryItem) to support non‑Steam GameSource in the future.
app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt (3)
114-133: Add loading/placeholder for network icon to avoid layout jank.CoilImage can show a lightweight placeholder and a loading indicator; right now it pops in abruptly.
Apply this diff:
CoilImage( imageModel = { icon }, imageOptions = ImageOptions( contentScale = androidx.compose.ui.layout.ContentScale.Fit, alignment = androidx.compose.ui.Alignment.Center, ), - modifier = Modifier.size(32.dp), + modifier = Modifier.size(32.dp), + loading = { + CircularProgressIndicator( + modifier = Modifier.size(16.dp), + strokeWidth = 2.dp, + ) + }, failure = { Icon( imageVector = Icons.Default.AccountCircle, contentDescription = null, modifier = Modifier.size(32.dp), tint = if (isLoggedIn) { MaterialTheme.colorScheme.onPrimaryContainer } else { MaterialTheme.colorScheme.onSurface }, ) }, )
139-158: Prefer surface/onSurface colors for readability on this card.Using onPrimaryContainer against a surfaceVariant card can hurt contrast in some themes. Recommend onSurface/onSurfaceVariant consistently.
Apply this diff:
- color = if (isLoggedIn) { - MaterialTheme.colorScheme.onPrimaryContainer - } else { - MaterialTheme.colorScheme.onSurface - }, + color = MaterialTheme.colorScheme.onSurface,- color = if (isLoggedIn) { - MaterialTheme.colorScheme.onPrimaryContainer.copy(alpha = 0.8f) - } else { - MaterialTheme.colorScheme.onSurfaceVariant - }, + color = MaterialTheme.colorScheme.onSurfaceVariant,
161-171: Expose a11y semantics for status icon.Add a semantic role and keep the text in sync for screen readers.
Apply this diff:
Icon( imageVector = if (isLoggedIn) Icons.Default.CheckCircle else Icons.Default.Circle, - contentDescription = if (isLoggedIn) "Connected" else "Not connected", + contentDescription = if (isLoggedIn) "Connected" else "Not connected", tint = if (isLoggedIn) { MaterialTheme.colorScheme.primary } else { MaterialTheme.colorScheme.onSurfaceVariant }, modifier = Modifier.size(20.dp), )And wrap it with:
+ androidx.compose.ui.semantics.semantics { + this.role = androidx.compose.ui.semantics.Role.Image + }app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt (1)
112-125: Avoid capturing possibly-stale selected item inside onClick lambda.Minor: derive selectedLibraryItem once and pass a stable onClick that uses the passed Boolean, or recompute libraryItem inside the lambda using the latest selectedAppId.
Apply this diff:
- LibraryDetailPane( - libraryItem = selectedLibraryItem, - onBack = { selectedAppId = null }, - onClickPlay = { - selectedLibraryItem?.let { libraryItem -> - onClickPlay(libraryItem.gameId, it) - } - }, - ) + LibraryDetailPane( + libraryItem = selectedLibraryItem, + onBack = { selectedAppId = null }, + onClickPlay = { asContainer -> + selectedAppId?.let { id -> + state.appInfoList.find { it.appId == id }?.let { li -> + onClickPlay(li.gameId, asContainer) + } + } + }, + )Please confirm this doesn’t regress recomposition behavior when paging updates state.appInfoList.
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt (2)
58-61: Duplicate import of distinctUntilChanged.Remove the duplicate to appease lint.
Apply this diff:
-import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.distinctUntilChanged
197-202: Use stable keys to prevent recomposition glitches when paging.Index-based keys can reorder; prefer appId.
Apply this diff:
- items(items = state.appInfoList, key = { it.index }) { item -> + items(items = state.appInfoList, key = { it.appId }) { item ->app/src/main/java/app/gamenative/ui/PluviaMain.kt (2)
887-897: Prefer a function type over KFunction2 for onSuccess.Cleaner type, fewer reflection edges.
Apply this diff:
- onSuccess: KFunction2<Context, LibraryItem, Unit>, + onSuccess: (Context, LibraryItem) -> Unit,And update call sites if needed (no behavioral change).
1029-1067: Avoid re-parsing appId; use the local gameId variable.Minor efficiency/readability improvement.
Apply this diff:
- message = "You played ${SteamService.getAppInfoOf(createLibraryItemFromAppId(appId).gameId)?.name} " + + message = "You played ${SteamService.getAppInfoOf(gameId)?.name} " +- "${SteamService.getAppInfoOf(createLibraryItemFromAppId(appId).gameId)?.name} " + + "${SteamService.getAppInfoOf(gameId)?.name} " +- "already playing ${SteamService.getAppInfoOf(createLibraryItemFromAppId(appId).gameId)?.name} " + + "already playing ${SteamService.getAppInfoOf(gameId)?.name} " +app/src/main/java/app/gamenative/utils/ContainerUtils.kt (6)
179-184: Avoid swallowing exceptions; use runCatching or log.
These try/catch blocks mask issues and trigger detekt warnings. Replace with runCatching and keep behavior.Apply:
- language = try { - container.language - } catch (e: Exception) { - container.getExtra("language", "english") - }, + language = runCatching { container.language } + .getOrElse { container.getExtra("language", "english") }, @@ - emulateKeyboardMouse = try { - container.isEmulateKeyboardMouse() - } catch (e: Exception) { - false - }, + emulateKeyboardMouse = runCatching { container.isEmulateKeyboardMouse() }.getOrDefault(false), @@ - controllerEmulationBindings = try { - container.getControllerEmulationBindings()?.toString() ?: "" - } catch (e: Exception) { - "" - }, + controllerEmulationBindings = runCatching { container.getControllerEmulationBindings()?.toString().orEmpty() } + .getOrDefault(""),Also applies to: 189-199
280-283: Don't swallow; add context to logs when falling back to extras.
Keeps current behavior but preserves diagnostics.Apply:
- } catch (e: Exception) { - container.putExtra("language", containerData.language) - } + } catch (e: Exception) { + Timber.w(e, "Container.language not available; storing in extras") + container.putExtra("language", containerData.language) + }
570-575: Prefer runCatching + verbose log over empty catch here.
Retains null fallback while providing traceability.Apply:
- val emuJson = try { - container.controllerEmulationBindings - } catch (_: Exception) { - null - } + val emuJson = runCatching { container.controllerEmulationBindings } + .onFailure { Timber.v(it, "No controllerEmulationBindings for %s", container.id) } + .getOrNull()
362-374: Prefer specific exception type for absent containers.
Throwing Exception is noisy; IllegalStateException reads better in Kotlin callers.Apply:
- throw Exception("Container does not exist for game $appId") + throw IllegalStateException("Container does not exist for game $appId")
508-550: Temporary override path: confirm restore flow exists.
You back up original config but this method doesn’t restore it. Ensure caller clears override and reapplies original config on exit.
733-738: Defaulting unknown sources to STEAM may hide issues.
Consider logging and/or introducing GameSource.UNKNOWN to make mismatches visible.app/src/main/java/com/winlator/container/ContainerManager.java (2)
196-201: Duplication flow works; minor ID quality nit.
Current scheme can produce "foo(1)(1)". Consider normalizing base before suffixing.Also applies to: 208-209
236-251: Optional: normalize baseId before suffixing.
Prevents chains of parentheses when duplicating a duplicate.Apply:
- private String generateUniqueContainerId(String baseId) { - // If the base ID doesn't exist, use it as-is - if (!hasContainer(baseId)) { - return baseId; - } - // Try baseId(1), baseId(2), etc. until we find a unique one - int counter = 1; - String candidateId; - do { - candidateId = baseId + "(" + counter + ")"; - counter++; - } while (hasContainer(candidateId)); - return candidateId; - } + private String generateUniqueContainerId(String baseId) { + String normalized = baseId.replaceFirst("\\(\\d+\\)$", ""); + if (!hasContainer(normalized)) return normalized; + int counter = 1; + String candidateId; + do { + candidateId = normalized + "(" + counter + ")"; + counter++; + } while (hasContainer(candidateId)); + return candidateId; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
app/src/main/java/app/gamenative/data/LibraryItem.kt(1 hunks)app/src/main/java/app/gamenative/events/AndroidEvent.kt(1 hunks)app/src/main/java/app/gamenative/service/SteamService.kt(1 hunks)app/src/main/java/app/gamenative/ui/PluviaMain.kt(28 hunks)app/src/main/java/app/gamenative/ui/component/dialog/ProfileDialog.kt(2 hunks)app/src/main/java/app/gamenative/ui/component/dialog/state/GameFeedbackDialogState.kt(4 hunks)app/src/main/java/app/gamenative/ui/component/topbar/AccountButton.kt(0 hunks)app/src/main/java/app/gamenative/ui/data/MainState.kt(1 hunks)app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt(5 hunks)app/src/main/java/app/gamenative/ui/model/MainViewModel.kt(7 hunks)app/src/main/java/app/gamenative/ui/screen/HomeScreen.kt(0 hunks)app/src/main/java/app/gamenative/ui/screen/PluviaScreen.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/accounts/SteamAccountSection.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt(15 hunks)app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt(4 hunks)app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt(3 hunks)app/src/main/java/app/gamenative/ui/screen/library/components/LibraryDetailPane.kt(4 hunks)app/src/main/java/app/gamenative/ui/screen/library/components/LibraryList.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt(11 hunks)app/src/main/java/app/gamenative/ui/screen/library/components/LibrarySearchBar.kt(2 hunks)app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt(7 hunks)app/src/main/java/app/gamenative/utils/ContainerMigrator.kt(1 hunks)app/src/main/java/app/gamenative/utils/ContainerUtils.kt(11 hunks)app/src/main/java/app/gamenative/utils/GameFeedbackUtils.kt(1 hunks)app/src/main/java/app/gamenative/utils/IntentLaunchManager.kt(9 hunks)app/src/main/java/app/gamenative/utils/SteamUtils.kt(10 hunks)app/src/main/java/com/winlator/container/Container.java(2 hunks)app/src/main/java/com/winlator/container/ContainerManager.java(6 hunks)app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java(2 hunks)
💤 Files with no reviewable changes (2)
- app/src/main/java/app/gamenative/ui/component/topbar/AccountButton.kt
- app/src/main/java/app/gamenative/ui/screen/HomeScreen.kt
🧰 Additional context used
🧬 Code graph analysis (11)
app/src/main/java/com/winlator/container/Container.java (1)
app/src/main/java/app/gamenative/utils/GameFeedbackUtils.kt (1)
id(119-119)
app/src/main/java/com/winlator/xenvironment/ImageFsInstaller.java (2)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (1)
extractGameIdFromContainerId(719-727)app/src/main/java/app/gamenative/service/SteamService.kt (1)
getAppDirPath(429-445)
app/src/main/java/app/gamenative/ui/screen/accounts/SteamAccountSection.kt (1)
app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt (1)
AccountSection(70-230)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryDetailPane.kt (1)
app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (1)
AppScreen(171-807)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (4)
app/src/main/java/app/gamenative/ui/component/ConnectingServersScreen.kt (1)
ConnectingServersScreen(13-49)app/src/main/java/app/gamenative/ui/screen/login/UserLoginScreen.kt (1)
UserLoginScreen(101-129)app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt (1)
AccountManagementScreen(26-57)app/src/main/java/app/gamenative/ui/screen/HomeScreen.kt (1)
HomeScreen(18-42)
app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt (1)
app/src/main/java/app/gamenative/ui/screen/accounts/SteamAccountSection.kt (1)
SteamAccountSection(9-29)
app/src/main/java/com/winlator/container/ContainerManager.java (3)
app/src/main/java/com/winlator/xenvironment/ImageFs.java (1)
ImageFs(13-113)app/src/main/java/com/winlator/core/FileUtils.java (1)
FileUtils(38-442)app/src/main/java/app/gamenative/utils/ContainerUtils.kt (1)
hasContainer(362-365)
app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (1)
app/src/main/java/app/gamenative/service/SteamService.kt (1)
getAppDirPath(429-445)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt (1)
app/src/main/java/com/winlator/container/ContainerManager.java (1)
ContainerManager(33-323)
app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt (1)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryDetailPane.kt (1)
LibraryDetailPane(22-62)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt (3)
app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt (1)
onPageChange(101-106)app/src/main/java/app/gamenative/ui/component/topbar/AccountButton.kt (1)
AccountButton(29-91)app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt (1)
AppItem(48-196)
🪛 detekt (1.23.8)
app/src/main/java/app/gamenative/utils/ContainerUtils.kt
[warning] 181-181: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 191-191: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 196-196: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 281-281: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
| val clientIconUrl: String | ||
| get() = Constants.Library.ICON_URL + "$appId/$iconHash.ico" | ||
| get() = Constants.Library.ICON_URL + "${gameId}/$iconHash.ico" | ||
|
|
||
| /** | ||
| * Helper property to get the game ID as an integer | ||
| * Extracts the numeric part by removing the gameSource prefix | ||
| */ | ||
| val gameId: Int | ||
| get() = appId.removePrefix("${gameSource.name}_").toInt() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Crash risk: gameId derivation can throw NumberFormatException.
appId is user/data‑derived and may not always match "${gameSource.name}_". Guard the invariant or fail fast with a clear message.
Apply this diff:
data class LibraryItem(
@@
val gameSource: GameSource = GameSource.STEAM,
) {
val clientIconUrl: String
- get() = Constants.Library.ICON_URL + "${gameId}/$iconHash.ico"
+ get() = Constants.Library.ICON_URL + "${gameId}/$iconHash.ico"
- /**
- * Helper property to get the game ID as an integer
- * Extracts the numeric part by removing the gameSource prefix
- */
- val gameId: Int
- get() = appId.removePrefix("${gameSource.name}_").toInt()
+ init {
+ require(appId.isEmpty() || appId.startsWith("${gameSource.name}_")) {
+ "LibraryItem.appId must start with '${gameSource.name}_'. Got '$appId'"
+ }
+ }
+ /**
+ * Helper property to get the numeric game ID (STEAM AppID, etc.)
+ */
+ val gameId: Int
+ get() = appId
+ .substringAfter("${gameSource.name}_", missingDelimiterValue = "")
+ .toIntOrNull()
+ ?: error("Invalid appId '$appId': expected '${gameSource.name}_<number>'")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val clientIconUrl: String | |
| get() = Constants.Library.ICON_URL + "$appId/$iconHash.ico" | |
| get() = Constants.Library.ICON_URL + "${gameId}/$iconHash.ico" | |
| /** | |
| * Helper property to get the game ID as an integer | |
| * Extracts the numeric part by removing the gameSource prefix | |
| */ | |
| val gameId: Int | |
| get() = appId.removePrefix("${gameSource.name}_").toInt() | |
| } | |
| val clientIconUrl: String | |
| get() = Constants.Library.ICON_URL + "${gameId}/$iconHash.ico" | |
| init { | |
| require(appId.isEmpty() || appId.startsWith("${gameSource.name}_")) { | |
| "LibraryItem.appId must start with '${gameSource.name}_'. Got '$appId'" | |
| } | |
| } | |
| /** | |
| * Helper property to get the numeric game ID (STEAM AppID, etc.) | |
| */ | |
| val gameId: Int | |
| get() = appId | |
| .substringAfter("${gameSource.name}_", missingDelimiterValue = "") | |
| .toIntOrNull() | |
| ?: error("Invalid appId '$appId': expected '${gameSource.name}_<number>'") | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/data/LibraryItem.kt around lines 21 to 30,
the computed gameId uses toInt() on a substring of user-derived appId which can
throw NumberFormatException; replace this with a guarded parse: compute the
expected prefix = "${gameSource.name}_", verify appId startsWith that prefix,
extract the suffix, parse it with toIntOrNull(), and if null (or appId doesn't
start with the prefix) throw an IllegalStateException with a clear message
including the invalid appId and the expected format so failures are fail-fast
and informative.
| SteamService.closeApp(gameId, isOffline.value) { prefix -> | ||
| PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID) | ||
| }.await() |
There was a problem hiding this comment.
Guard against null userSteamId during exit path.
Double-bang (!!) can crash if user logged out or offline.
Apply this diff:
- SteamService.closeApp(gameId, isOffline.value) { prefix ->
- PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID)
- }.await()
+ val accountId = SteamService.userSteamId?.accountID
+ if (accountId == null) {
+ Timber.w("exitSteamApp: userSteamId is null; skipping closeApp")
+ return@launch
+ }
+ SteamService.closeApp(gameId, isOffline.value) { prefix ->
+ PathType.from(prefix).toAbsPath(context, gameId, accountId)
+ }.await()🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/ui/model/MainViewModel.kt around lines 251
to 253, the code uses SteamService.userSteamId!! which will throw if userSteamId
is null during exit; change this to safely handle null by capturing userSteamId
into a local val (e.g. val userId = SteamService.userSteamId?.accountID) and if
null either skip the closeApp/await call or provide a sensible fallback (log and
return early), then call PathType.from(prefix).toAbsPath(context, gameId,
userId) only when userId is non-null to avoid a crash.
| val prefixToPath: (String) -> String = { prefix -> | ||
| PathType.from(prefix).toAbsPath(context, appId, SteamService.userSteamId!!.accountID) | ||
| PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID) | ||
| } | ||
| val postSyncInfo = SteamService.beginLaunchApp( | ||
| appId = appId, | ||
| appId = gameId, | ||
| prefixToPath = prefixToPath, | ||
| ignorePendingOperations = ignorePendingOperations, | ||
| preferredSave = preferredSave, |
There was a problem hiding this comment.
Offline launch can crash: userSteamId forced unwrap (!!).
When continuing offline without a Steam session, userSteamId is null and this will NPE.
Apply this diff:
- val prefixToPath: (String) -> String = { prefix ->
- PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID)
- }
+ val accountId = SteamService.userSteamId?.accountID
+ if (accountId == null) {
+ // fail fast with a user-facing message
+ setLoadingDialogVisible(false)
+ setMessageDialogState(
+ MessageDialogState(
+ visible = true,
+ type = DialogType.SYNC_FAIL,
+ title = context.getString(R.string.sync_error_title),
+ message = "You must be logged in to launch games (even offline) because we need your Steam account ID for save paths.",
+ dismissBtnText = context.getString(R.string.ok),
+ ),
+ )
+ return
+ }
+ val prefixToPath: (String) -> String = { prefix ->
+ PathType.from(prefix).toAbsPath(context, gameId, accountId)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val prefixToPath: (String) -> String = { prefix -> | |
| PathType.from(prefix).toAbsPath(context, appId, SteamService.userSteamId!!.accountID) | |
| PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID) | |
| } | |
| val postSyncInfo = SteamService.beginLaunchApp( | |
| appId = appId, | |
| appId = gameId, | |
| prefixToPath = prefixToPath, | |
| ignorePendingOperations = ignorePendingOperations, | |
| preferredSave = preferredSave, | |
| val accountId = SteamService.userSteamId?.accountID | |
| if (accountId == null) { | |
| // fail fast with a user-facing message | |
| setLoadingDialogVisible(false) | |
| setMessageDialogState( | |
| MessageDialogState( | |
| visible = true, | |
| type = DialogType.SYNC_FAIL, | |
| title = context.getString(R.string.sync_error_title), | |
| message = "You must be logged in to launch games (even offline) because we need your Steam account ID for save paths.", | |
| dismissBtnText = context.getString(R.string.ok), | |
| ), | |
| ) | |
| return | |
| } | |
| val prefixToPath: (String) -> String = { prefix -> | |
| PathType.from(prefix).toAbsPath(context, gameId, accountId) | |
| } | |
| val postSyncInfo = SteamService.beginLaunchApp( | |
| appId = gameId, | |
| prefixToPath = prefixToPath, | |
| ignorePendingOperations = ignorePendingOperations, | |
| preferredSave = preferredSave, |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/ui/PluviaMain.kt around lines 929 to 936,
the code force-unwraps SteamService.userSteamId with !! which will NPE when
launching offline; change this to handle a null Steam session by retrieving
accountID safely (e.g. SteamService.userSteamId?.accountID) and supply a
sensible fallback or nullable value to PathType.from/toAbsPath and
beginLaunchApp (or guard and call a different offline path). Ensure the
prefixToPath lambda and the beginLaunchApp call accept the nullable accountID
(or early-return/branch when no session) so no forced unwrap remains.
| LaunchedEffect(Unit) { | ||
| if (isInstalled) { | ||
| appSizeOnDisk = "..." | ||
| DownloadService.getSizeOnDiskDisplay(appInfo.appId) { appSizeOnDisk = it } | ||
| DownloadService.getSizeOnDiskDisplay(appInfo.gameId) { appSizeOnDisk = it } | ||
| } |
There was a problem hiding this comment.
Re-run size query when gameId or install state changes
LaunchedEffect(Unit) won’t re-trigger across different items. Key it to gameId and isInstalled to avoid stale UI.
Apply this diff:
- LaunchedEffect(Unit) {
+ LaunchedEffect(appInfo.gameId, isInstalled) {
if (isInstalled) {
appSizeOnDisk = "..."
- DownloadService.getSizeOnDiskDisplay(appInfo.gameId) { appSizeOnDisk = it }
+ val currentId = appInfo.gameId
+ DownloadService.getSizeOnDiskDisplay(currentId) { size ->
+ // defensive: update only if still the same item
+ if (currentId == appInfo.gameId) appSizeOnDisk = size
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LaunchedEffect(Unit) { | |
| if (isInstalled) { | |
| appSizeOnDisk = "..." | |
| DownloadService.getSizeOnDiskDisplay(appInfo.appId) { appSizeOnDisk = it } | |
| DownloadService.getSizeOnDiskDisplay(appInfo.gameId) { appSizeOnDisk = it } | |
| } | |
| LaunchedEffect(appInfo.gameId, isInstalled) { | |
| if (isInstalled) { | |
| appSizeOnDisk = "..." | |
| val currentId = appInfo.gameId | |
| DownloadService.getSizeOnDiskDisplay(currentId) { size -> | |
| // defensive: update only if still the same item | |
| if (currentId == appInfo.gameId) appSizeOnDisk = size | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt
around lines 64 to 68, the LaunchedEffect currently uses Unit so it never
re-runs when different items render; change it to key the LaunchedEffect on
appInfo.gameId and isInstalled so the size query re-runs whenever the gameId or
install state changes, and inside the effect set appSizeOnDisk to "..." before
invoking DownloadService.getSizeOnDiskDisplay and update/clear the value
appropriately based on isInstalled.
| while (finalNewDir.exists()) { | ||
| finalContainerId = "STEAM_$legacyId($counter)" | ||
| finalNewDir = File(homeDir, "${ImageFs.USER}-$finalContainerId") // WITH xuser- prefix | ||
| counter++ | ||
| } |
There was a problem hiding this comment.
Conflict suffix breaks ID schema
Fallback "STEAM_$legacyId($counter)" violates the numeric-only expectation and will likely break parsers (e.g., extractors expecting STEAM_\d+). Use an underscore suffix.
Apply:
- while (finalNewDir.exists()) {
- finalContainerId = "STEAM_$legacyId($counter)"
- finalNewDir = File(homeDir, "${ImageFs.USER}-$finalContainerId") // WITH xuser- prefix
- counter++
- }
+ while (finalNewDir.exists()) {
+ finalContainerId = "STEAM_${legacyId}_$counter"
+ finalNewDir = File(homeDir, "${ImageFs.USER}-$finalContainerId")
+ counter++
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while (finalNewDir.exists()) { | |
| finalContainerId = "STEAM_$legacyId($counter)" | |
| finalNewDir = File(homeDir, "${ImageFs.USER}-$finalContainerId") // WITH xuser- prefix | |
| counter++ | |
| } | |
| while (finalNewDir.exists()) { | |
| finalContainerId = "STEAM_${legacyId}_$counter" | |
| finalNewDir = File(homeDir, "${ImageFs.USER}-$finalContainerId") | |
| counter++ | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/utils/ContainerMigrator.kt around lines 116
to 120, the fallback ID uses parentheses ("STEAM_$legacyId($counter)") which
breaks numeric-only parsers; change the suffix format to an underscore-separated
numeric suffix (e.g., "STEAM_${legacyId}_$counter") and update the subsequent
File construction to use that ID so the final container name remains
"xuser-STEAM_<digits>_<counter>" without parentheses.
| if (previousLanguage.lowercase() != containerData.language.lowercase()) { | ||
| val appDirPath = SteamService.getAppDirPath(container.id) | ||
| val steamAppId = extractGameIdFromContainerId(container.id) | ||
| val appDirPath = SteamService.getAppDirPath(steamAppId) | ||
| MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED) | ||
| Timber.i("Language changed from '$previousLanguage' to '${containerData.language}'. Cleared STEAM_DLL_REPLACED marker for container ${container.id}.") | ||
| } |
There was a problem hiding this comment.
STEAM-specific marker removal should be gated by source.
For non‑Steam IDs (future EPIC_/GOG_), SteamService path is wrong.
Apply:
- if (previousLanguage.lowercase() != containerData.language.lowercase()) {
- val steamAppId = extractGameIdFromContainerId(container.id)
- val appDirPath = SteamService.getAppDirPath(steamAppId)
- MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED)
- Timber.i("Language changed from '$previousLanguage' to '${containerData.language}'. Cleared STEAM_DLL_REPLACED marker for container ${container.id}.")
- }
+ if (previousLanguage.lowercase() != containerData.language.lowercase()) {
+ when (extractGameSourceFromContainerId(container.id)) {
+ GameSource.STEAM -> {
+ val steamAppId = extractGameIdFromContainerId(container.id)
+ val appDirPath = SteamService.getAppDirPath(steamAppId)
+ MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED)
+ }
+ else -> Timber.d("Language changed for non‑Steam container %s; skipping STEAM_DLL_REPLACED removal.", container.id)
+ }
+ Timber.i("Language changed from '$previousLanguage' to '${containerData.language}'. Cleared/handled language-change markers for container ${container.id}.")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (previousLanguage.lowercase() != containerData.language.lowercase()) { | |
| val appDirPath = SteamService.getAppDirPath(container.id) | |
| val steamAppId = extractGameIdFromContainerId(container.id) | |
| val appDirPath = SteamService.getAppDirPath(steamAppId) | |
| MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED) | |
| Timber.i("Language changed from '$previousLanguage' to '${containerData.language}'. Cleared STEAM_DLL_REPLACED marker for container ${container.id}.") | |
| } | |
| if (previousLanguage.lowercase() != containerData.language.lowercase()) { | |
| when (extractGameSourceFromContainerId(container.id)) { | |
| GameSource.STEAM -> { | |
| val steamAppId = extractGameIdFromContainerId(container.id) | |
| val appDirPath = SteamService.getAppDirPath(steamAppId) | |
| MarkerUtils.removeMarker(appDirPath, Marker.STEAM_DLL_REPLACED) | |
| } | |
| else -> Timber.d("Language changed for non‑Steam container %s; skipping STEAM_DLL_REPLACED removal.", container.id) | |
| } | |
| Timber.i("Language changed from '$previousLanguage' to '${containerData.language}'. Cleared/handled language-change markers for container ${container.id}.") | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/utils/ContainerUtils.kt around lines 288 to
293, the code always uses SteamService to compute an app directory and remove
the STEAM_DLL_REPLACED marker even for non‑Steam containers; guard this logic so
it only runs for Steam containers (e.g., check container.id startsWith("STEAM_")
or containerData.source == "STEAM"), and only then call
extractGameIdFromContainerId, SteamService.getAppDirPath and
MarkerUtils.removeMarker; otherwise skip the Steam-specific marker removal.
| val gameId = extractGameIdFromContainerId(appId) | ||
| val appDirPath = SteamService.getAppDirPath(gameId) | ||
| val drive: Char = Container.getNextAvailableDriveLetter(defaultDrives) | ||
| val drives = "$defaultDrives$drive:$appDirPath" | ||
| Timber.d("Prepared container drives: $drives") |
There was a problem hiding this comment.
Container drive path assumes Steam; branch by source.
Non‑Steam IDs will resolve to wrong dir.
Apply:
- val gameId = extractGameIdFromContainerId(appId)
- val appDirPath = SteamService.getAppDirPath(gameId)
+ val gameId = extractGameIdFromContainerId(appId)
+ val appDirPath = when (extractGameSourceFromContainerId(appId)) {
+ GameSource.STEAM -> SteamService.getAppDirPath(gameId)
+ // TODO: add other platforms' resolvers
+ else -> SteamService.getAppDirPath(gameId) // safe fallback for now
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val gameId = extractGameIdFromContainerId(appId) | |
| val appDirPath = SteamService.getAppDirPath(gameId) | |
| val drive: Char = Container.getNextAvailableDriveLetter(defaultDrives) | |
| val drives = "$defaultDrives$drive:$appDirPath" | |
| Timber.d("Prepared container drives: $drives") | |
| val gameId = extractGameIdFromContainerId(appId) | |
| val appDirPath = when (extractGameSourceFromContainerId(appId)) { | |
| GameSource.STEAM -> SteamService.getAppDirPath(gameId) | |
| // TODO: add other platforms' resolvers | |
| else -> SteamService.getAppDirPath(gameId) // safe fallback for now | |
| } | |
| val drive: Char = Container.getNextAvailableDriveLetter(defaultDrives) | |
| val drives = "$defaultDrives$drive:$appDirPath" | |
| Timber.d("Prepared container drives: $drives") |
| fun extractGameIdFromContainerId(containerId: String): Int { | ||
| val afterUnderscore = containerId.split("_", limit = 2)[1] | ||
| // Remove duplicate suffix like (1), (2) if present | ||
| return if (afterUnderscore.contains("(")) { | ||
| afterUnderscore.substringBefore("(").toInt() | ||
| } else { | ||
| afterUnderscore.toInt() | ||
| } | ||
| } |
There was a problem hiding this comment.
Fragile parsing of containerId; add validation.
Accessing [1] can throw, and toInt() may fail; provide clear error.
Apply:
- fun extractGameIdFromContainerId(containerId: String): Int {
- val afterUnderscore = containerId.split("_", limit = 2)[1]
- // Remove duplicate suffix like (1), (2) if present
- return if (afterUnderscore.contains("(")) {
- afterUnderscore.substringBefore("(").toInt()
- } else {
- afterUnderscore.toInt()
- }
- }
+ fun extractGameIdFromContainerId(containerId: String): Int {
+ val parts = containerId.split("_", limit = 2)
+ require(parts.size == 2 && parts[1].isNotEmpty()) { "Invalid containerId format: $containerId" }
+ val numeric = parts[1].substringBefore("(")
+ return numeric.toIntOrNull() ?: throw IllegalArgumentException("Invalid numeric gameId in containerId: $containerId")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun extractGameIdFromContainerId(containerId: String): Int { | |
| val afterUnderscore = containerId.split("_", limit = 2)[1] | |
| // Remove duplicate suffix like (1), (2) if present | |
| return if (afterUnderscore.contains("(")) { | |
| afterUnderscore.substringBefore("(").toInt() | |
| } else { | |
| afterUnderscore.toInt() | |
| } | |
| } | |
| fun extractGameIdFromContainerId(containerId: String): Int { | |
| val parts = containerId.split("_", limit = 2) | |
| require(parts.size == 2 && parts[1].isNotEmpty()) { "Invalid containerId format: $containerId" } | |
| val numeric = parts[1].substringBefore("(") | |
| return numeric.toIntOrNull() ?: throw IllegalArgumentException("Invalid numeric gameId in containerId: $containerId") | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/utils/ContainerUtils.kt around lines
719-727, the current parsing accesses split("_", limit=2)[1] and calls toInt()
directly which can throw IndexOutOfBoundsException or NumberFormatException;
update the method to validate the input: check that containerId contains an
underscore and that split with limit=2 returns at least 2 parts, trim the
extracted suffix, remove any parenthetical suffix via substringBefore("("), then
use toIntOrNull() to safely parse and if null throw an IllegalArgumentException
(or a custom exception) with a clear message including the original containerId
and why it failed (missing underscore, empty id part, or non-numeric id); ensure
no unchecked array accesses remain and provide descriptive error text for
callers.
| val gameId = ContainerUtils.extractGameIdFromContainerId(appId) | ||
| val container = ContainerUtils.getContainer(context, appId) | ||
| val configJson = Json.parseToJsonElement(FileUtils.readString(container.getConfigFile()).replace("\\u0000", "").replace("\u0000", "")).jsonObject | ||
| Timber.d("config string is: " + FileUtils.readString(container.getConfigFile()).replace("\\u0000", "").replace("\u0000", "")) | ||
| Timber.d("configJson: $configJson") | ||
| // Get the game name from container or use a fallback | ||
| val appInfo = SteamService.getAppInfoOf(appId)!! | ||
| val appInfo = SteamService.getAppInfoOf(gameId)!! | ||
| val gameName = appInfo.name |
There was a problem hiding this comment.
Avoid NPE and duplicate I/O; don’t log full config content
- getAppInfoOf(gameId)!! can crash; handle null.
- The config file is read twice; read once and reuse.
- Logging the full config JSON/string may leak PII/secrets. Remove or redact.
Apply this diff:
- val gameId = ContainerUtils.extractGameIdFromContainerId(appId)
- val container = ContainerUtils.getContainer(context, appId)
- val configJson = Json.parseToJsonElement(FileUtils.readString(container.getConfigFile()).replace("\\u0000", "").replace("\u0000", "")).jsonObject
- Timber.d("config string is: " + FileUtils.readString(container.getConfigFile()).replace("\\u0000", "").replace("\u0000", ""))
- Timber.d("configJson: $configJson")
+ val gameId = ContainerUtils.extractGameIdFromContainerId(appId)
+ val container = ContainerUtils.getContainer(context, appId)
+ val configStr = FileUtils.readString(container.getConfigFile())
+ .replace("\\u0000", "")
+ .replace("\u0000", "")
+ val configJson = Json.parseToJsonElement(configStr).jsonObject
+ Timber.d("Loaded container config JSON for feedback (redacted)")
@@
- val appInfo = SteamService.getAppInfoOf(gameId)!!
- val gameName = appInfo.name
+ val appInfo = SteamService.getAppInfoOf(gameId)
+ val gameName = appInfo?.name ?: "Unknown ($gameId)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val gameId = ContainerUtils.extractGameIdFromContainerId(appId) | |
| val container = ContainerUtils.getContainer(context, appId) | |
| val configJson = Json.parseToJsonElement(FileUtils.readString(container.getConfigFile()).replace("\\u0000", "").replace("\u0000", "")).jsonObject | |
| Timber.d("config string is: " + FileUtils.readString(container.getConfigFile()).replace("\\u0000", "").replace("\u0000", "")) | |
| Timber.d("configJson: $configJson") | |
| // Get the game name from container or use a fallback | |
| val appInfo = SteamService.getAppInfoOf(appId)!! | |
| val appInfo = SteamService.getAppInfoOf(gameId)!! | |
| val gameName = appInfo.name | |
| val gameId = ContainerUtils.extractGameIdFromContainerId(appId) | |
| val container = ContainerUtils.getContainer(context, appId) | |
| val configStr = FileUtils.readString(container.getConfigFile()) | |
| .replace("\\u0000", "") | |
| .replace("\u0000", "") | |
| val configJson = Json.parseToJsonElement(configStr).jsonObject | |
| Timber.d("Loaded container config JSON for feedback (redacted)") | |
| // Get the game name from container or use a fallback | |
| val appInfo = SteamService.getAppInfoOf(gameId) | |
| val gameName = appInfo?.name ?: "Unknown ($gameId)" |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/utils/GameFeedbackUtils.kt around lines 54
to 61, avoid the forced non-null (!!) on SteamService.getAppInfoOf(gameId) by
handling a null return (e.g., check for null and use a fallback name or return
early), read the container config file once into a local string variable (apply
the replace("\\u0000", "") or .replace("\u0000", "") a single time) and parse
that single value to JSON rather than calling FileUtils.readString twice, and
remove or redact the detailed config logging (replace full config/Timber.d calls
with a minimal safe message or sanitized summary).
| val gameId = intent.getIntExtra(EXTRA_APP_ID, -1) | ||
| Timber.d("[IntentLaunchManager]: Extracted app_id: $gameId from intent extras") | ||
|
|
||
| if (appId <= 0) { | ||
| Timber.w("[IntentLaunchManager]: Invalid or missing app_id in launch intent: $appId") | ||
| if (gameId <= 0) { | ||
| Timber.w("[IntentLaunchManager]: Invalid or missing app_id in launch intent: $gameId") | ||
| return null | ||
| } | ||
|
|
||
| val appId = "${GameSource.STEAM.name}_$gameId" | ||
| Timber.d("[IntentLaunchManager]: Converted to appId: $appId") | ||
|
|
There was a problem hiding this comment.
Parsing locks appId to STEAM and ignores string-based appId—backward/forward compat bug.
Support string appId (e.g., "STEAM_1234"), legacy int, and optional game_source.
Apply this diff:
@@
- val gameId = intent.getIntExtra(EXTRA_APP_ID, -1)
- Timber.d("[IntentLaunchManager]: Extracted app_id: $gameId from intent extras")
-
- if (gameId <= 0) {
- Timber.w("[IntentLaunchManager]: Invalid or missing app_id in launch intent: $gameId")
- return null
- }
-
- val appId = "${GameSource.STEAM.name}_$gameId"
- Timber.d("[IntentLaunchManager]: Converted to appId: $appId")
+ val explicitAppId = intent.getStringExtra(EXTRA_APP_ID)
+ val sourceName = intent.getStringExtra(EXTRA_GAME_SOURCE)?.uppercase()
+ val legacySteamId = if (explicitAppId.isNullOrBlank()) intent.getIntExtra(EXTRA_STEAM_APP_ID, -1) else -1
+ val numericId = if (explicitAppId.isNullOrBlank()) intent.getIntExtra(EXTRA_APP_ID, -1) else -1
+
+ val appId = when {
+ !explicitAppId.isNullOrBlank() -> explicitAppId
+ legacySteamId > 0 -> "${GameSource.STEAM.name}_$legacySteamId"
+ numericId > 0 -> {
+ val source = sourceName?.let { runCatching { GameSource.valueOf(it) }.getOrNull() } ?: GameSource.STEAM
+ "${source.name}_$numericId"
+ }
+ else -> {
+ Timber.w("[IntentLaunchManager]: Invalid or missing app_id in launch intent (explicit:'$explicitAppId', numeric:$numericId, legacySteamId:$legacySteamId)")
+ return null
+ }
+ }
+ Timber.d("[IntentLaunchManager]: Resolved appId: $appId")And define these extras:
@@
private const val ACTION_LAUNCH_GAME = "app.gamenative.LAUNCH_GAME"
private const val MAX_CONFIG_JSON_SIZE = 50000 // 50KB limit to prevent memory exhaustion
+ private const val EXTRA_GAME_SOURCE = "game_source" // optional, e.g., STEAM/EPIC
+ private const val EXTRA_STEAM_APP_ID = "steam_app_id" // legacy support📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val gameId = intent.getIntExtra(EXTRA_APP_ID, -1) | |
| Timber.d("[IntentLaunchManager]: Extracted app_id: $gameId from intent extras") | |
| if (appId <= 0) { | |
| Timber.w("[IntentLaunchManager]: Invalid or missing app_id in launch intent: $appId") | |
| if (gameId <= 0) { | |
| Timber.w("[IntentLaunchManager]: Invalid or missing app_id in launch intent: $gameId") | |
| return null | |
| } | |
| val appId = "${GameSource.STEAM.name}_$gameId" | |
| Timber.d("[IntentLaunchManager]: Converted to appId: $appId") | |
| private const val ACTION_LAUNCH_GAME = "app.gamenative.LAUNCH_GAME" | |
| private const val MAX_CONFIG_JSON_SIZE = 50000 // 50KB limit to prevent memory exhaustion | |
| private const val EXTRA_GAME_SOURCE = "game_source" // optional, e.g., STEAM/EPIC | |
| private const val EXTRA_STEAM_APP_ID = "steam_app_id" // legacy support | |
| val explicitAppId = intent.getStringExtra(EXTRA_APP_ID) | |
| val sourceName = intent.getStringExtra(EXTRA_GAME_SOURCE)?.uppercase() | |
| val legacySteamId = if (explicitAppId.isNullOrBlank()) intent.getIntExtra(EXTRA_STEAM_APP_ID, -1) else -1 | |
| val numericId = if (explicitAppId.isNullOrBlank()) intent.getIntExtra(EXTRA_APP_ID, -1) else -1 | |
| val appId = when { | |
| !explicitAppId.isNullOrBlank() -> explicitAppId | |
| legacySteamId > 0 -> "${GameSource.STEAM.name}_$legacySteamId" | |
| numericId > 0 -> { | |
| val source = sourceName?.let { runCatching { GameSource.valueOf(it) }.getOrNull() } ?: GameSource.STEAM | |
| "${source.name}_$numericId" | |
| } | |
| else -> { | |
| Timber.w("[IntentLaunchManager]: Invalid or missing app_id in launch intent (explicit:'$explicitAppId', numeric:$numericId, legacySteamId:$legacySteamId)") | |
| return null | |
| } | |
| } | |
| Timber.d("[IntentLaunchManager]: Resolved appId: $appId") |
unbelievableflavour
force-pushed
the
account-management-screen
branch
from
b35fb55 to
4ff7e17
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
865-867: Critical: Force-unwrap of userSteamId can cause NPE.This is the same critical issue flagged in the past review. The forced unwrap of
SteamService.userSteamId!!.accountIDwill crash when launching games offline.Apply this diff to handle null Steam sessions safely:
- val prefixToPath: (String) -> String = { prefix -> - PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID) - } + val accountId = SteamService.userSteamId?.accountID + if (accountId == null) { + // fail fast with a user-facing message + setLoadingDialogVisible(false) + setMessageDialogState( + MessageDialogState( + visible = true, + type = DialogType.SYNC_FAIL, + title = context.getString(R.string.sync_error_title), + message = "You must be logged in to launch games (even offline) because we need your Steam account ID for save paths.", + dismissBtnText = context.getString(R.string.ok), + ), + ) + return + } + val prefixToPath: (String) -> String = { prefix -> + PathType.from(prefix).toAbsPath(context, gameId, accountId) + }
🧹 Nitpick comments (4)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
61-61: Duplicate import detected.The
AccountManagementScreenis imported twice on lines 61 and 83.Apply this diff to remove the duplicate import:
import app.gamenative.ui.screen.PluviaScreen import app.gamenative.ui.screen.accounts.AccountManagementScreen import app.gamenative.ui.screen.chat.ChatScreen -import app.gamenative.ui.screen.accounts.AccountManagementScreenAlso applies to: 83-83
app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt (1)
82-90: Minor: Modifier chain formatting could be improved.The
safePaddingModifierassignment and.then()usage could be more readable.Apply this diff to improve readability:
- val safePaddingModifier = - if (LocalConfiguration.current.orientation == Configuration.ORIENTATION_PORTRAIT) { - Modifier.displayCutoutPadding() - } else { - Modifier - } + val safePaddingModifier = if (LocalConfiguration.current.orientation == Configuration.ORIENTATION_PORTRAIT) { + Modifier.displayCutoutPadding() + } else { + Modifier + } - Box( - Modifier.background(MaterialTheme.colorScheme.background) - .then(safePaddingModifier), - ) { + Box( + modifier = Modifier + .background(MaterialTheme.colorScheme.background) + .then(safePaddingModifier), + ) {app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt (2)
69-82: Consider removing the backward compatibility comment.Since this is a new file, the comment about "backward compatibility" on line 69 may be misleading. The
AccountSectionappears to be a reusable component rather than something maintained for compatibility.-// Keep the existing AccountSection for backward compatibility +// Reusable account section component
64-64: Make SteamAccountSection lifecycle‑aware / observe central login state
- SteamAccountSection uses
val isSteamLoggedIn = remember { mutableStateOf(SteamService.isLoggedIn) }(app/src/main/java/app/gamenative/ui/screen/accounts/SteamAccountSection.kt:14). This localrememberwon't survive configuration changes and won't react to external changes toSteamService.isLoggedIn.- Fix options (pick one): use
rememberSaveable(or a Saver) to persist across config changes; or expose/login state from a ViewModel or a lifecycle-aware observable (e.g. StateFlow) andcollectAsStateWithLifecycle()in the composable so the UI updates automatically; or add aLaunchedEffect(SteamService.isLoggedIn)to update the local state when the service changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/src/main/java/app/gamenative/ui/PluviaMain.kt(5 hunks)app/src/main/java/app/gamenative/ui/component/dialog/ProfileDialog.kt(2 hunks)app/src/main/java/app/gamenative/ui/component/topbar/AccountButton.kt(0 hunks)app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt(4 hunks)app/src/main/java/app/gamenative/ui/screen/HomeScreen.kt(0 hunks)app/src/main/java/app/gamenative/ui/screen/PluviaScreen.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/accounts/SteamAccountSection.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt(2 hunks)app/src/main/java/app/gamenative/ui/screen/library/components/LibraryDetailPane.kt(2 hunks)app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt(9 hunks)
💤 Files with no reviewable changes (2)
- app/src/main/java/app/gamenative/ui/screen/HomeScreen.kt
- app/src/main/java/app/gamenative/ui/component/topbar/AccountButton.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/java/app/gamenative/ui/screen/accounts/SteamAccountSection.kt
- app/src/main/java/app/gamenative/ui/screen/PluviaScreen.kt
- app/src/main/java/app/gamenative/ui/component/dialog/ProfileDialog.kt
- app/src/main/java/app/gamenative/ui/screen/library/components/LibraryDetailPane.kt
- app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (3)
app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt (1)
app/src/main/java/app/gamenative/ui/screen/accounts/SteamAccountSection.kt (1)
SteamAccountSection(9-29)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt (1)
AccountManagementScreen(26-57)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt (3)
app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt (1)
onPageChange(101-106)app/src/main/java/app/gamenative/ui/component/topbar/AccountButton.kt (1)
AccountButton(29-91)app/src/main/java/app/gamenative/ui/screen/library/components/LibraryAppItem.kt (1)
AppItem(48-196)
🔇 Additional comments (19)
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt (4)
86-98: LGTM! Proper infinite scroll implementation.The infinite scroll logic using
snapshotFlowanddistinctUntilChangedis correctly implemented. The boundary conditions check both the last visible index and total available items to trigger pagination appropriately.
201-201: Navigation parameter type changed from Int to String.The navigation now uses
item.appId(String) instead of an integer ID. This aligns with the string-based library ID changes mentioned in the summary.
207-218: LGTM! Loading indicator for pagination.The loading indicator properly shows when more items are available (
state.appInfoList.size < state.totalAppsInFilter) and provides good user feedback during pagination.
160-165: AccountButton API verified — accepts onGoOnline; no onLogout required.AccountButton is defined in app/src/main/java/app/gamenative/ui/component/topbar/AccountButton.kt (params: onNavigateRoute, onGoOnline, isOffline) and all call sites (e.g., LibraryListPane.kt, PluviaMain.kt, HomeScreen.kt, LibraryScreen.kt) pass onGoOnline; no AccountButton usage expects onLogout. No change required.
app/src/main/java/app/gamenative/ui/PluviaMain.kt (2)
706-716: LGTM! Account management route properly configured.The new AccountManagementScreen route is correctly integrated with proper navigation callbacks. The
onBackusesnavigateUp()which maintains proper navigation stack behavior.
694-694: Start destination changed to Home — verify auth/UXNavHost startDestination is set to PluviaScreen.Home.route (app/src/main/java/app/gamenative/ui/PluviaMain.kt:694). HomeScreen (app/src/main/java/app/gamenative/ui/screen/HomeScreen.kt) renders the library unconditionally and does not enforce SteamService.isLoggedIn, so users can land on Home while not logged in. Either revert startDestination to PluviaScreen.LoginUser or add a redirect/guard (or explicit offline/login UI in Home) to avoid unexpected unauthenticated UX.
app/src/main/java/app/gamenative/ui/screen/library/LibraryScreen.kt (3)
40-46: LGTM! onLogout parameter removed from public API.The signature change aligns with the broader refactoring to remove logout handling from individual screens and centralize it in the account management flow.
113-125: LGTM! String-based navigation with proper item lookup.The navigation correctly uses string-based
appIdand properly looks up the correspondingLibraryItemfor the detail pane. The null safety handling is appropriate.
117-125: LibraryDetailPane API updated to accept LibraryItem.The component now receives the full
LibraryItemobject instead of just an integer ID, providing richer context for the detail view.app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt (10)
1-25: LGTM! Clean imports and package structure.The imports are well-organized with standard Android Compose imports, Material3 components, and app-specific dependencies. The package structure follows Android conventions.
26-57: Well-structured main screen composable.The
AccountManagementScreenfollows good Compose practices with proper state management, scaffold layout, and navigation callbacks. The use ofdisplayCutoutPadding()ensures proper display on devices with notches or cutouts.
59-67: Good extensibility design with clear placeholder for future accounts.The
AccountsGroupis well-designed for extensibility, with a clear comment indicating where other account sections (GOG, Epic Games, etc.) will be added. The use ofSettingsGroupprovides consistent UI patterns.
83-106: Excellent visual design with gradient accent.The card styling with gradient border, proper theming, and alpha transparency creates a polished look. The horizontal gradient using primary and tertiary colors provides good visual hierarchy.
114-133: Good fallback handling for account icons.The
CoilImageimplementation with proper fallback toAccountCircleicon ensures the UI remains functional even when external icons fail to load. The conditional tinting based on login state provides good visual feedback.
147-158: Smart conditional text display logic.The logic to show "Logged in as {username}" when authenticated versus the description when not logged in provides clear user feedback. The color and styling changes based on state enhance the UX.
162-172: Clear visual status indicators.The use of
CheckCirclevsCircleicons with appropriate color theming provides immediate visual feedback about connection status. The size and positioning work well with the overall layout.
174-189: Proper error handling UI.The error message card with error container theming follows Material Design guidelines and provides clear visual distinction for error states.
191-228: Excellent loading state and action button implementation.The conditional rendering of login/logout buttons with proper loading states, disabled states, and progress indicators provides a smooth user experience. The use of
AutoMirroredicons ensures proper RTL support.
232-242: Comprehensive preview setup.The preview includes both dark mode and a specific device configuration (Odin2 Mini), which helps ensure the UI works across different contexts and screen sizes.
| MainViewModel.MainUiEvent.OnLoggedOut -> { | ||
| // Pop stack and go back to login. | ||
| navController.popBackStack( | ||
| route = PluviaScreen.LoginUser.route, | ||
| inclusive = false, | ||
| saveState = false, | ||
| ) | ||
| // Do nothing - let users stay on current page after logout | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Logout behavior change - users now stay on current page.
The OnLoggedOut event handler now does nothing instead of navigating back to login. This could create inconsistent UX if users expect to be redirected after logout.
🏁 Script executed:
#!/bin/bash
# Description: Verify how logout is handled across the app and if users get proper feedback
echo "=== Searching for logout handling patterns ==="
rg -nP --type=kt -A5 -B5 'OnLoggedOut|logOut|logout' -g '!**/test/**'
echo -e "\n=== Checking if there are logout confirmation dialogs or feedback ==="
rg -nP --type=kt 'logout.*[Dd]ialog|[Ll]ogout.*feedback' -g '!**/test/**'Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== rg version ==="
rg --version || true
echo -e "\n=== Searching for OnLoggedOut occurrences ==="
rg -n -C3 -S 'OnLoggedOut\s*->|OnLoggedOut' -g '!**/test/**' || true
echo -e "\n=== Searching for MainUiEvent usages ==="
rg -n -C3 -S 'MainUiEvent' -g '!**/test/**' || true
echo -e "\n=== Searching for logout / sign-out / navigation to login patterns ==="
rg -n -C3 -i -S 'logout|log out|signout|sign out|signOut|navigate.*login|navigate.*auth|navigateToLogin|navigateToAuth|LoginActivity|AuthActivity|popBackStack' -g '!**/test/**' || true
echo -e "\n=== Searching for NavController usage and popBackStack ==="
rg -n -C3 -S 'NavController|navController|NavHostController|popBackStack' -g '!**/test/**' || true
echo -e "\n=== Locating PluviaMain.kt and printing surrounding lines (if present) ==="
pluvia_files=$(find . -type f -iname 'PluviaMain.kt' -print || true)
if [ -n "$pluvia_files" ]; then
echo "$pluvia_files" | while IFS= read -r f; do
echo "---- $f (lines 1-260) ----"
sed -n '1,260p' "$f"
done
else
echo "PluviaMain.kt not found"
fi
echo -e "\n=== Searching for ViewModel / event handlers mentioning LoggedOut ==="
rg -n -C3 -S 'OnLoggedOut|onLoggedOut|LoggedOut|logoutEvent|logOut' -g '!**/test/**' || trueLength of output: 13744
Restore post-logout navigation or explicit feedback — OnLoggedOut is a no-op (app/src/main/java/app/gamenative/ui/PluviaMain.kt: lines 196–198).
OnLoggedOut currently does nothing, leaving users on the current page after logout; this can produce inconsistent UX and may expose screens expecting an authenticated user. Restore a redirect to the login route (clearing backstack) or show explicit logout feedback and disable/guard protected UI.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/ui/PluviaMain.kt around lines 196–198, the
OnLoggedOut branch is a no-op which leaves users on protected screens; update
this handler to perform post-logout navigation and UI changes: trigger
navigation to the login route and clear the navigation backstack (so users
cannot navigate back to authenticated screens), and optionally emit a brief
logout confirmation (Toast/Snackbar) and reset any protected ViewModel/UI state
or guards to disable protected controls. Ensure the navigation/stack-clear is
done on the UI thread and any auth tokens/state are cleared before navigating.
| .toList() | ||
|
|
||
| Timber.tag("LibraryViewModel").d("Filtered list size: ${totalFound}") | ||
| Timber.tag("LibraryViewModel").d("Filtered list size: $totalFound") |
There was a problem hiding this comment.
please keep linting and formatting out of the PRs. This file is fine because there are no actual changes here, but it makes it very difficult to see actual changes. In the other PR SteamService had been linted and I couldn't tell what was a formatting change and what was an actual change.
if you want, make a pure linting PR later on
There was a problem hiding this comment.
Are there actual changes in this file or is it just linting? again, please keep linting out of the PRs
| NavHost( | ||
| navController = navController, | ||
| startDestination = PluviaScreen.LoginUser.route, | ||
| startDestination = PluviaScreen.Home.route, |
There was a problem hiding this comment.
what happens the first time they open the app and they don't have any games? it just shows them a random blank screen? if so that's not ok
There was a problem hiding this comment.
this is what it looks like when you open the app and are not signed in, it's not really clear for the user what they should do and is confusing, as this is what they'll see the first time they open the app.
I suggest that if they are logged out of all accounts, we show a new screen that says something like:
"Welcome Back
Sign in to access your games" (Exactly as we have right now)
And then below that have 3 buttons as options (similar to how account management currently looks):
- Sign in to Steam - takes you to current Steam sign in flow
- Sign in to GOG - your new flow (not implemented yet)
- Continue offline - takes you to home screen
Mockup attached
There was a problem hiding this comment.
Oh and please do check that the offline flows still work. Try
- opening the app without an internet connection when signed in
- opening the app without an internet connection when signed out
- turning off the internet when you have the app open and in the menu
- turning off the internet when you have the app open in-game
|
|
||
| FilledTonalButton(modifier = Modifier.fillMaxWidth(), onClick = { onNavigateRoute(PluviaScreen.AccountManagement.route) }) { | ||
| Icon(imageVector = Icons.Default.AccountCircle, contentDescription = null) | ||
| Spacer(modifier = Modifier.size(ButtonDefaults.IconSize)) | ||
| Text(text = "Manage Accounts") | ||
| } | ||
|
|
There was a problem hiding this comment.
fine to have this here, but can we have a "Log out of all accounts" at the bottom?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/src/main/java/app/gamenative/ui/model/MainViewModel.kt (1)
249-253: NPE risk:SteamService.userSteamId!!in exit path.This will crash if user is logged out/offline. Guard and early‑return.
Apply:
- SteamService.closeApp(gameId, isOffline.value) { prefix -> - PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID) - }.await() + val accountId = SteamService.userSteamId?.accountID + if (accountId == null) { + Timber.w("exitSteamApp: userSteamId is null; skipping closeApp for $gameId") + return@launch + } + SteamService.closeApp(gameId, isOffline.value) { prefix -> + PathType.from(prefix).toAbsPath(context, gameId, accountId) + }.await()app/src/main/java/app/gamenative/ui/PluviaMain.kt (2)
197-199: Post‑logout is a no‑op; redirect or give explicit feedback.Users remain on potentially protected screens after logout. Navigate to Welcome/Login and clear back stack or show a confirmation and disable protected UI.
879-882: Offline launch crash:userSteamId!!in preLaunchApp.Same NPE hazard when signed out/offline. Guard and surface a user‑facing error.
Apply:
- val prefixToPath: (String) -> String = { prefix -> - PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID) - } + val accountId = SteamService.userSteamId?.accountID + if (accountId == null) { + setLoadingDialogVisible(false) + setMessageDialogState( + MessageDialogState( + visible = true, + type = DialogType.SYNC_FAIL, + title = context.getString(R.string.sync_error_title), + message = "You must be logged in to launch games (even offline) because your Steam account ID is required for save paths.", + dismissBtnText = context.getString(R.string.ok), + ), + ) + return@launch + } + val prefixToPath: (String) -> String = { prefix -> + PathType.from(prefix).toAbsPath(context, gameId, accountId) + }Run to catch any remaining force‑unwraps:
#!/bin/bash rg -nP --type=kt -C2 'userSteamId!!'
🧹 Nitpick comments (5)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (3)
61-62: Duplicate import of AccountManagementScreen.Remove one of these imports.
Apply:
-import app.gamenative.ui.screen.accounts.AccountManagementScreen ... -import app.gamenative.ui.screen.accounts.AccountManagementScreen(keep a single import)
Also applies to: 83-85
314-317: Auto‑nav condition is effectively unreachable.Start destination is Welcome and success flow goes to AccountManagement; checking for
currentScreen == LoginUser+!isFirstLaunchwon’t trigger. FlipisFirstLaunchto false when leaving Welcome to make this useful.Apply in Welcome route below to set
isFirstLaunch = falseon first action.
701-710: Mark first‑launch complete when user takes an action.Set
isFirstLaunch=falsebefore navigating so future sessions can auto‑nav.Apply:
- WelcomeScreen( - onSetupAccounts = { - navController.navigate(PluviaScreen.AccountManagement.route) - }, - onSkipSignIn = { - navController.navigate(PluviaScreen.Home.route + "?offline=true") - }, - ) + WelcomeScreen( + onSetupAccounts = { + viewModel.setIsFirstLaunch(false) + navController.navigate(PluviaScreen.AccountManagement.route) + }, + onSkipSignIn = { + viewModel.setIsFirstLaunch(false) + navController.navigate(PluviaScreen.Home.route + "?offline=true") + }, + )app/src/main/java/app/gamenative/ui/screen/welcome/WelcomeScreen.kt (2)
135-137: Button label: align with spec (“Continue offline”).Spec and Home route use offline mode; rename for clarity.
Apply:
- text = "Skip Sign In", + text = "Continue Offline",
73-88: Hard‑coded strings: move to resources for i18n.Extract texts to
strings.xml(title, body, buttons).If helpful, I can submit the resource entries.
Also applies to: 97-114, 116-139
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/ui/PluviaMain.kt(6 hunks)app/src/main/java/app/gamenative/ui/data/MainState.kt(1 hunks)app/src/main/java/app/gamenative/ui/model/MainViewModel.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/PluviaScreen.kt(1 hunks)app/src/main/java/app/gamenative/ui/screen/welcome/WelcomeScreen.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/app/gamenative/ui/PluviaMain.kt (2)
app/src/main/java/app/gamenative/ui/screen/welcome/WelcomeScreen.kt (1)
WelcomeScreen(25-142)app/src/main/java/app/gamenative/ui/screen/accounts/AccountManagementScreen.kt (1)
AccountManagementScreen(26-57)
🔇 Additional comments (2)
app/src/main/java/app/gamenative/ui/screen/PluviaScreen.kt (1)
7-13: Routes addition looks good.Welcome and AccountManagement routes extend the graph cleanly; no conflicts detected.
app/src/main/java/app/gamenative/ui/data/MainState.kt (1)
21-21: Change approved — no positional/destructuring usages found.
MainState is only constructed via MainState() (MainViewModel.kt); all .copy(...) calls use named arguments and there are no destructuring (val (...) = ...) or componentN() usages referencing MainState.
| _state.update { | ||
| it.copy( | ||
| isFirstLaunch = true, // Temporarily always true for debugging | ||
| isSteamConnected = SteamService.isConnected, | ||
| hasCrashedLastStart = PrefManager.recentlyCrashed, | ||
| launchedAppId = "", | ||
| ) | ||
| } |
There was a problem hiding this comment.
Debug default keeps users in “first launch” forever; gate with real state.
isFirstLaunch = true will block your auto‑nav path. Remove this or set it based on persisted onboarding state; flip to false after first action.
Apply:
_state.update {
it.copy(
- isFirstLaunch = true, // Temporarily always true for debugging
isSteamConnected = SteamService.isConnected,
hasCrashedLastStart = PrefManager.recentlyCrashed,
launchedAppId = "",
)
}Add a setter (outside this hunk):
fun setIsFirstLaunch(value: Boolean) {
_state.update { it.copy(isFirstLaunch = value) }
}🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/ui/model/MainViewModel.kt around lines
135-142 the state is being force-set with isFirstLaunch = true for debugging
which will permanently block the auto-navigation path; remove the hardcoded true
and initialize isFirstLaunch from persisted onboarding state (e.g., PrefManager
or equivalent) and provide a public setter to update it when onboarding
completes; add a setIsFirstLaunch(value: Boolean) method that updates _state to
copy(isFirstLaunch = value) and ensure callers flip it to false after first user
action or when persisted flag indicates onboarding is finished.
3 participants
Summary by CodeRabbit
New Features
Improvements
Behavior Changes
Bug Fixes