Gog support cancellable(WIP) #145
Conversation
…when it's not relevant
Simplified GameMangerService so gameManager can be injected through mock
# Conflicts: # app/src/main/java/app/gamenative/PluviaApp.kt # app/src/main/java/app/gamenative/events/AndroidEvent.kt # app/src/main/java/app/gamenative/ui/PluviaMain.kt # app/src/main/java/app/gamenative/utils/ContainerUtils.kt # app/src/main/java/app/gamenative/utils/SteamUtils.kt
WalkthroughAdds GOG platform support end-to-end: Python (Chaquopy) gogdl integration, OAuth activity, foreground GOGService, DB/DAO/models, managers (GOG/Steam) unified via GameManagerService, UI migration to LibraryItem and Accounts screen, container ID migration to String, network/DI setup, and numerous API/type updates across services and UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as PluviaApp
participant GMS as GameManagerService
participant GM as GameManager (Steam/GOG)
participant Svc as SteamService/GOGService
User->>App: Tap Play (LibraryItem)
App->>GMS: launchGameWithSaveSync(libraryItem)
GMS->>GM: delegate by libraryItem.gameSource
GM->>Svc: begin launch (sync saves if needed)
alt Sync OK
Svc-->>GM: PostSyncInfo(Success)
GM-->>GMS: Wine start command / launch
GMS-->>User: Game launching
else Sync Error
Svc-->>GM: PostSyncInfo(Failure)
GM-->>GMS: propagate result
GMS-->>User: Show error dialog
end
sequenceDiagram
autonumber
actor User
participant UI as GOGOAuthActivity
participant GOG as GOGService
participant GLM as GOGLibraryManager
User->>UI: Sign in to GOG
UI-->>UI: Detect redirect with code
UI->>GOG: authenticateWithCode(configPath, code)
alt Success
GOG-->>UI: GOGCredentials
UI->>GLM: startBackgroundSync(clearExisting=true)
GLM-->>GLM: progressive sync to DB
else Failure
GOG-->>UI: Error
UI-->>User: Show auth error
end
sequenceDiagram
autonumber
participant UI as LibraryAppScreen
participant GMS as GameManagerService
participant GM as GOGGameManager
participant GOG as GOGService
UI->>GMS: downloadGameWithResult(libraryItem)
GMS->>GM: downloadGame(...)
GM->>GOG: downloadGame(gameId, installPath, auth)
GOG-->>GM: DownloadInfo (progress via logs)
GM-->>GMS: Result<DownloadInfo?>
GMS-->>UI: Result<DownloadInfo?>
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180+ minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 114
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
app/src/main/java/app/gamenative/ui/component/dialog/state/GameFeedbackDialogState.kt (1)
39-47: Fix restore to handle previously-saved Int appId to avoid ClassCastExceptionReplace the unsafe cast with a tolerant conversion that accepts String/Int/Number and returns a String:
- appId = savedMap["appId"] as String, + appId = when (val v = savedMap["appId"]) { + is String -> v + is Int -> if (v > 0) v.toString() else "" + is Number -> if (v.toInt() > 0) v.toInt().toString() else "" + else -> "" + },Follow-up: decide how to map legacy Ints if IDs are now prefixed (e.g., "steam:123"); ensure upstream code doesn’t assume a prefix.
app/src/main/java/app/gamenative/data/DownloadInfo.kt (1)
45-53: Add bounds checks to prevent AIOOBE for multi-job progress/weights.
s etProgress/setWeighttrustjobIndex; out-of-range will crash.Apply this diff:
fun setProgress(amount: Float, jobIndex: Int = 0) { - progresses[jobIndex] = amount + require(jobIndex in progresses.indices) { "jobIndex out of range: $jobIndex" } + progresses[jobIndex] = amount.coerceIn(0f, 1f) emitProgressChange() } fun setWeight(jobIndex: Int, weightBytes: Long) { // tiny helper - weights[jobIndex] = weightBytes.toFloat() + require(jobIndex in weights.indices) { "jobIndex out of range: $jobIndex" } + weights[jobIndex] = weightBytes.coerceAtLeast(0L).toFloat() weightSum = weights.sum() }app/src/main/java/app/gamenative/di/DatabaseModule.kt (1)
23-25: Avoid destructive migrations in productionfallbackToDestructiveMigration() will wipe user data on DB version bumps — gate it to debug builds and add concrete Room Migration instances for release.
Location: app/src/main/java/app/gamenative/di/DatabaseModule.kt (lines 23–25)
+import app.gamenative.BuildConfig @@ - return Room.databaseBuilder(context, PluviaDatabase::class.java, DATABASE_NAME) - .fallbackToDestructiveMigration() // TODO remove before prod - .build() + val builder = Room.databaseBuilder(context, PluviaDatabase::class.java, DATABASE_NAME) + if (BuildConfig.DEBUG) { + builder.fallbackToDestructiveMigration() + } else { + // TODO: add concrete MIGRATION_X_Y instances here + // builder.addMigrations(MIGRATION_3_4, MIGRATION_4_5, ...) + } + return builder.build()app/src/main/java/app/gamenative/utils/GameFeedbackUtils.kt (1)
56-58: Stop logging full config contents; also avoid double I/O.The raw config may include tokens/paths; logging it risks PII/secret leakage. You’re also reading the file twice.
- 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", "")) + val configStr = FileUtils.readString(container.getConfigFile()) + .replace("\\u0000", "") + .replace("\u0000", "") + val configJson = Json.parseToJsonElement(configStr).jsonObject + Timber.d("Loaded config (redacted): length=${configStr.length}")app/src/main/java/app/gamenative/utils/SteamUtils.kt (1)
644-649: Language lookup should use the same container ID scheme.Make this consistent with Line 315 and centralize the translation from numeric appId to containerId.
- val container = ContainerUtils.getOrCreateContainer(context, GameManagerService.getAppId(appId, GameSource.STEAM)) + val containerId = "STEAM_${appId}" + val container = ContainerUtils.getOrCreateContainer(context, containerId)If
getOrCreateContainerstill takes Int, provide a parallelgetOrCreateContainerById(containerId: String)to avoid overloading confusion.app/src/main/java/app/gamenative/ui/model/LibraryViewModel.kt (2)
45-51: Library won’t update when game metadata changes without size change.You only refresh when list size differs; install status/name changes with same size won’t update UI.
- GameManagerService.getAllGames().collect { games -> - if (allGames.size != games.size) { - allGames = games - onFilterApps(paginationCurrentPage) - } - } + GameManagerService.getAllGames().collect { games -> + allGames = games + onFilterApps(paginationCurrentPage) + }
96-172: Do filtering/sorting on a background dispatcher to avoid jank on large libraries.Heavy sequence + sort + map can block main.
- viewModelScope.launch { + viewModelScope.launch(Dispatchers.Default) {app/src/main/python/gogdl/launch.py (1)
295-314: Parent-window traversal bug: always reading window.parent instead of currentWindow.parent.This prevents correct ancestry walking and can misreport processes. Use currentWindow.parent inside the loop.
Apply:
- var currentWindow: Window = window + var currentWindow: Window = window do { - var parentWindow: Window? = window.parent + var parentWindow: Window? = currentWindow.parent val process = if (parentWindow != null && parentWindow.className.lowercase() != "explorer.exe") { val processId = currentWindow.processId val parentProcessId = parentWindow.processId currentWindow = parentWindowapp/src/main/java/app/gamenative/ui/model/MainViewModel.kt (1)
287-314: Incorrect parent traversal when building AppProcessInfo chain.Uses window.parent instead of currentWindow.parent, so the loop does not advance properly.
- val launchConfig = SteamService.getWindowsLaunchInfos(gameId).firstOrNull { + val launchConfig = SteamService.getWindowsLaunchInfos(gameId).firstOrNull { val gameExe = Paths.get(it.executable.replace('\\', '/')).name.lowercase() val windowExe = window.className.lowercase() gameExe == windowExe } @@ - var currentWindow: Window = window + var currentWindow: Window = window do { - var parentWindow: Window? = window.parent + var parentWindow: Window? = currentWindow.parent val process = if (parentWindow != null && parentWindow.className.lowercase() != "explorer.exe") { val processId = currentWindow.processId val parentProcessId = parentWindow.processId currentWindow = parentWindowapp/src/main/java/app/gamenative/service/SteamService.kt (2)
1183-1196: Token mix-up: passing refreshToken as accessToken breaks OAuth login.LogOnDetails receives accessToken=refreshToken. Use accessToken. This likely causes intermittent login failures.
steamUser.logOn( LogOnDetails( username = SteamUtils.removeSpecialChars(username).trim(), password = password?.let { SteamUtils.removeSpecialChars(it).trim() }, shouldRememberPassword = rememberSession, twoFactorCode = twoFactorAuth, authCode = emailAuth, - accessToken = refreshToken, + accessToken = accessToken, loginID = SteamUtils.getUniqueDeviceId(instance!!), machineName = SteamUtils.getMachineName(instance!!), chatMode = ChatMode.NEW_STEAM_CHAT, ), )
1681-1696: Don’t swallow NullPointerException; null‑check instead.Replace try/catch NPE with safe calls; keep logging real failures.
- try { - steamClient!!.servers.tryMark(steamClient!!.currentEndpoint, PROTOCOL_TYPES, ServerQuality.BAD) - } catch (e: NullPointerException) { - // I don't care - } catch (e: Exception) { - Timber.e(e, "Failed to mark endpoint as bad:") - } + steamClient?.currentEndpoint?.let { endpoint -> + runCatching { steamClient?.servers?.tryMark(endpoint, PROTOCOL_TYPES, ServerQuality.BAD) } + .onFailure { Timber.e(it, "Failed to mark endpoint as bad:") } + } - try { - steamClient!!.disconnect() - } catch (e: NullPointerException) { - // I don't care - } catch (e: Exception) { - Timber.e(e, "There was an issue when disconnecting:") - } + steamClient?.let { + runCatching { it.disconnect() } + .onFailure { Timber.e(it, "There was an issue when disconnecting:") } + }app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (1)
742-813: Gate Steam-only cloud options by source to avoid crashes for GOG titlesForceCloudSync/ForceDownloadRemote/ForceUploadLocal assume Steam context and dereference SteamService.userSteamId!!. These options should be hidden (or no-op) for non‑Steam items.
Apply this diff to wrap Steam-only menu entries:
- AppMenuOption( + if (libraryItem.gameSource == GameSource.STEAM) AppMenuOption( AppOptionMenuType.ForceCloudSync, onClick = { ... }, - ), + ) else null, - AppMenuOption( + if (libraryItem.gameSource == GameSource.STEAM) AppMenuOption( AppOptionMenuType.ForceDownloadRemote, onClick = { ... }, - ), + ) else null, - AppMenuOption( + if (libraryItem.gameSource == GameSource.STEAM) AppMenuOption( AppOptionMenuType.ForceUploadLocal, onClick = { ... }, - ), + ) else null,And when building the array, filter out nulls:
- arrayOf( + arrayOfNotNull( /* entries above */ - ) + )app/src/main/java/app/gamenative/utils/ContainerUtils.kt (4)
457-494: DirectX probe should only run for Steam and use numeric app id
SteamUtils.fetchDirect3DMajor(appId)is passed a source-prefixed id (e.g., "STEAM_12345") and will fail. Gate byGameSourceand pass the numeric Steam id.- SteamUtils.fetchDirect3DMajor(appId) { dxVersion -> + if (extractGameSourceFromContainerId(appId) == GameSource.STEAM) { + val steamId = extractGameIdFromContainerId(appId).toString() + SteamUtils.fetchDirect3DMajor(steamId) { dxVersion -> + deferred.complete(dxVersion) + } + } else { + // Non-Steam: keep existing wrapper + deferred.complete(-1) + } - deferred.complete(dxVersion) - }
457-494: Blocking DX detection + Steam-only logicThis synchronous DX detection blocks the caller and is Steam-specific. At minimum, skip for non‑Steam IDs to avoid errors; ideally, move detection off the UI path.
Minimal guard within this hunk:
- runBlocking { + if (extractGameSourceFromContainerId(appId) == GameSource.STEAM) runBlocking { try { Timber.i("Fetching DirectX version synchronously for app $appId") // ... } catch (e: Exception) { Timber.w(e, "Error determining DirectX version: ${e.message}") // Continue with default wrapper on error } - } + } else { + Timber.i("Skipping DirectX detection for non-Steam appId=%s; using default dxwrapper=%s", appId, containerData.dxwrapper) + }Follow-up (non-blocking alternative): accept default wrapper here and reconcile DX wrapper later in a background task.
220-236: Potential NPE on GPU vendor lookup
getGPUCards(context)[containerData.videoPciDeviceID]!!may be null for custom/unknown IDs. Guard or use a sane default.Suggested change (adjust within your preferred bounds):
- registryEditor.setDwordValue( - "Software\\Wine\\Direct3D", - "VideoPciVendorID", - getGPUCards(context)[containerData.videoPciDeviceID]!!.vendorId, - ) + getGPUCards(context)[containerData.videoPciDeviceID]?.vendorId?.let { vendorId -> + registryEditor.setDwordValue("Software\\Wine\\Direct3D", "VideoPciVendorID", vendorId) + } ?: Timber.w("Unknown VideoPciDeviceID=%d; leaving VideoPciVendorID unchanged", containerData.videoPciDeviceID)
501-553: Blocking calls on UI paths — move container creation off the main threadContainerUtils.createNewContainer uses containerManager.createContainerFuture(...).get() and runBlocking{...await()}, so getOrCreateContainer / getOrCreateContainerWithOverride can block the main thread.
- Problematic UI call sites: app/src/main/java/app/gamenative/ui/PluviaMain.kt:881; app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:205 and 621.
- Safe (already on IO): app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt:713, 752, 789.
Change plan (pick one): make getOrCreateContainer* suspend and run blocking work under withContext(Dispatchers.IO), or ensure all callers dispatch to Dispatchers.IO before calling; replace Future.get/runBlocking with coroutine-friendly awaiting.
| @Volatile | ||
| private var isCancelled = false | ||
|
|
||
| fun cancel() { | ||
| isCancelled = true | ||
| downloadJob?.cancel(CancellationException("Cancelled by user")) | ||
| progressMonitorJob?.cancel(CancellationException("Progress monitoring cancelled by user")) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure cancellation races don’t leak monitor jobs.
If cancel() is called before setProgressMonitorJob, the later setter won’t cancel. Cancel in the setter if already cancelled.
Apply this diff:
@Volatile
private var isCancelled = false
fun cancel() {
isCancelled = true
downloadJob?.cancel(CancellationException("Cancelled by user"))
progressMonitorJob?.cancel(CancellationException("Progress monitoring cancelled by user"))
}
fun isCancelled(): Boolean = isCancelledCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/data/DownloadInfo.kt around lines 17 to 24,
the cancel() call can race with later setters so a job assigned after cancel()
won't be cancelled; update the setter(s) for downloadJob and progressMonitorJob
to assign the incoming Job to the field and immediately cancel it if isCancelled
is already true (i.e., if (isCancelled)
newJob?.cancel(CancellationException(...)) ), or alternatively check isCancelled
before assignment and cancel instead of only setting the field — ensure the same
CancellationException messages are used and that assignment/cancellation is
performed atomically in the setter to avoid leaking monitor jobs.
|
|
||
| fun setProgressMonitorJob(job: Job) { | ||
| progressMonitorJob = job | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cancel newly assigned monitor immediately if already cancelled.
Apply this diff:
fun setProgressMonitorJob(job: Job) {
- progressMonitorJob = job
+ progressMonitorJob = job
+ if (isCancelled) {
+ job.cancel(CancellationException("Progress monitoring cancelled by user"))
+ }
}📝 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 setProgressMonitorJob(job: Job) { | |
| progressMonitorJob = job | |
| } | |
| fun setProgressMonitorJob(job: Job) { | |
| progressMonitorJob = job | |
| if (isCancelled) { | |
| job.cancel(CancellationException("Progress monitoring cancelled by user")) | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/data/DownloadInfo.kt around lines 31-34,
update setProgressMonitorJob so that after assigning progressMonitorJob = job
you immediately check if job.isCancelled and if so call cancel() on it (e.g., if
(job.isCancelled) progressMonitorJob?.cancel()), ensuring a newly assigned job
that is already cancelled is cancelled right away.
| import androidx.room.TypeConverter | ||
| import kotlinx.serialization.json.Json | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing serialization extensions; this won’t compile without imports.
import androidx.room.TypeConverter
import kotlinx.serialization.json.Json
+import kotlinx.serialization.encodeToString
+import kotlinx.serialization.decodeFromString
+import android.util.Log
+import kotlinx.serialization.SerializationException📝 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.
| import androidx.room.TypeConverter | |
| import kotlinx.serialization.json.Json | |
| import androidx.room.TypeConverter | |
| import kotlinx.serialization.json.Json | |
| import kotlinx.serialization.encodeToString | |
| import kotlinx.serialization.decodeFromString | |
| import android.util.Log | |
| import kotlinx.serialization.SerializationException |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/db/converters/GOGConverter.kt around lines
3-5, the file is missing the kotlinx.serialization extension function imports
required for Json.encodeToString/decodeFromString; add the imports "import
kotlinx.serialization.encodeToString" and "import
kotlinx.serialization.decodeFromString" (and any specific serializer imports if
using custom serializers) so the TypeConverter methods compile and can call
Json.encodeToString(...) and Json.decodeFromString(...).
| @TypeConverter | ||
| fun toStringList(value: String): List<String> { | ||
| return try { | ||
| Json.decodeFromString<List<String>>(value) | ||
| } catch (e: Exception) { | ||
| emptyList() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t swallow decode errors; handle null/blank safely.
Return empty for null/blank, and log decode failures with a narrow catch.
- @TypeConverter
- fun toStringList(value: String): List<String> {
- return try {
- Json.decodeFromString<List<String>>(value)
- } catch (e: Exception) {
- emptyList()
- }
- }
+ @TypeConverter
+ fun toStringList(value: String?): List<String> {
+ if (value.isNullOrBlank()) return emptyList()
+ return try {
+ Json.decodeFromString<List<String>>(value)
+ } catch (e: SerializationException) {
+ Log.e("GOGConverter", "Failed to decode List<String> from JSON: $value", e)
+ emptyList()
+ }
+ }📝 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.
| @TypeConverter | |
| fun toStringList(value: String): List<String> { | |
| return try { | |
| Json.decodeFromString<List<String>>(value) | |
| } catch (e: Exception) { | |
| emptyList() | |
| } | |
| } | |
| @TypeConverter | |
| fun toStringList(value: String?): List<String> { | |
| if (value.isNullOrBlank()) return emptyList() | |
| return try { | |
| Json.decodeFromString<List<String>>(value) | |
| } catch (e: SerializationException) { | |
| Log.e("GOGConverter", "Failed to decode List<String> from JSON: $value", e) | |
| emptyList() | |
| } | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 17-17: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/db/converters/GOGConverter.kt around lines
13–20, the TypeConverter currently swallows all decode errors and doesn't handle
null/blank input; update the function to first treat null or blank strings as
returning emptyList(), then attempt Json.decodeFromString inside a try that only
catches JSON/serialization-specific exceptions (e.g.,
kotlinx.serialization.SerializationException) rather than Exception, and on
failure log the error with context (use the project's logging facility such as
Log.e or Timber.e) before returning emptyList().
| if (GOGService.isRunning && !isChangingConfigurations) { | ||
| Timber.i("Stopping GOG Service") | ||
| GOGService.stop() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t stop GOGService during onDestroy if active operations are running
Unconditional stop may terminate downloads/sync. Mirror the onStop guard.
- if (GOGService.isRunning && !isChangingConfigurations) {
+ if (GOGService.isRunning && !isChangingConfigurations && !GOGService.hasActiveOperations()) {
Timber.i("Stopping GOG Service")
GOGService.stop()
}📝 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 (GOGService.isRunning && !isChangingConfigurations) { | |
| Timber.i("Stopping GOG Service") | |
| GOGService.stop() | |
| } | |
| if (GOGService.isRunning && !isChangingConfigurations && !GOGService.hasActiveOperations()) { | |
| Timber.i("Stopping GOG Service") | |
| GOGService.stop() | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/MainActivity.kt around lines 228 to 231, the
onDestroy unconditionally stops GOGService which can abort active
downloads/sync; change the guard to mirror onStop by only calling
GOGService.stop() when the service is running, the activity is not changing
configurations, and there are no active service operations (e.g., add a check
like !GOGService.hasActiveOperations or equivalent API) so that stop() is
skipped while work is in progress.
| def _read_content(self, file_path): | ||
| """Return the contents from a file in /proc""" | ||
| try: | ||
| with open(file_path, encoding='utf-8') as proc_file: | ||
| content = proc_file.read() | ||
| except (ProcessLookupError, FileNotFoundError, PermissionError): | ||
| return "" | ||
| return content |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden file reads against decode errors and permission issues.
/proc entries can contain non-UTF-8 bytes; use errors='replace' and include UnicodeDecodeError in the handled exceptions.
def _read_content(self, file_path):
"""Return the contents from a file in /proc"""
try:
- with open(file_path, encoding='utf-8') as proc_file:
+ with open(file_path, encoding='utf-8', errors='replace') as proc_file:
content = proc_file.read()
- except (ProcessLookupError, FileNotFoundError, PermissionError):
+ except (ProcessLookupError, FileNotFoundError, PermissionError, UnicodeDecodeError):
return ""
return content📝 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.
| def _read_content(self, file_path): | |
| """Return the contents from a file in /proc""" | |
| try: | |
| with open(file_path, encoding='utf-8') as proc_file: | |
| content = proc_file.read() | |
| except (ProcessLookupError, FileNotFoundError, PermissionError): | |
| return "" | |
| return content | |
| def _read_content(self, file_path): | |
| """Return the contents from a file in /proc""" | |
| try: | |
| with open(file_path, encoding='utf-8', errors='replace') as proc_file: | |
| content = proc_file.read() | |
| except (ProcessLookupError, FileNotFoundError, PermissionError, UnicodeDecodeError): | |
| return "" | |
| return content |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/process.py around lines 26 to 33, _read_content
must tolerate non-UTF-8 bytes and decoding errors when reading /proc: open the
file with encoding='utf-8' and errors='replace' (so invalid byte sequences are
replaced) and add UnicodeDecodeError to the exception tuple alongside
ProcessLookupError, FileNotFoundError, and PermissionError so decoding failures
return an empty string instead of crashing.
| def get_stat(self, parsed=True): | ||
| stat_filename = "/proc/{}/stat".format(self.pid) | ||
| try: | ||
| with open(stat_filename, encoding='utf-8') as stat_file: | ||
| _stat = stat_file.readline() | ||
| except (ProcessLookupError, FileNotFoundError): | ||
| return None | ||
| if parsed: | ||
| return _stat[_stat.rfind(")") + 1:].split() | ||
| return _stat | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
get_stat: handle permission/encoding edge cases.
Mirror _read_content robustness for /proc/<pid>/stat.
def get_stat(self, parsed=True):
stat_filename = "/proc/{}/stat".format(self.pid)
try:
- with open(stat_filename, encoding='utf-8') as stat_file:
+ with open(stat_filename, encoding='utf-8', errors='replace') as stat_file:
_stat = stat_file.readline()
- except (ProcessLookupError, FileNotFoundError):
+ except (ProcessLookupError, FileNotFoundError, PermissionError, UnicodeDecodeError):
return None
if parsed:
return _stat[_stat.rfind(")") + 1:].split()
return _stat📝 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.
| def get_stat(self, parsed=True): | |
| stat_filename = "/proc/{}/stat".format(self.pid) | |
| try: | |
| with open(stat_filename, encoding='utf-8') as stat_file: | |
| _stat = stat_file.readline() | |
| except (ProcessLookupError, FileNotFoundError): | |
| return None | |
| if parsed: | |
| return _stat[_stat.rfind(")") + 1:].split() | |
| return _stat | |
| def get_stat(self, parsed=True): | |
| stat_filename = "/proc/{}/stat".format(self.pid) | |
| try: | |
| with open(stat_filename, encoding='utf-8', errors='replace') as stat_file: | |
| _stat = stat_file.readline() | |
| except (ProcessLookupError, FileNotFoundError, PermissionError, UnicodeDecodeError): | |
| return None | |
| if parsed: | |
| return _stat[_stat.rfind(")") + 1:].split() | |
| return _stat |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/process.py around lines 35-45, get_stat currently
opens /proc/<pid>/stat without handling permission or encoding issues; update it
to mirror _read_content by opening the file with encoding='utf-8' and
errors='replace' (or read bytes and decode with errors='replace'), and catch
PermissionError, UnicodeDecodeError and generic OSError/FileNotFoundError (or
ProcessLookupError) so you return None on inaccessible files; preserve the
existing parsed/unparsed return behavior after reading and parsing.
| def cmdline(self): | ||
| """Return command line used to run the process `pid`.""" | ||
| cmdline_path = "/proc/{}/cmdline".format(self.pid) | ||
| _cmdline_content = self._read_content(cmdline_path) | ||
| if _cmdline_content: | ||
| return _cmdline_content.replace("\x00", " ").replace("\\", "/") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutating command-line backslashes; they are valid characters.
Replacing \ with / can corrupt arguments on Linux. Keep only the NUL→space normalization.
_cmdline_content = self._read_content(cmdline_path)
if _cmdline_content:
- return _cmdline_content.replace("\x00", " ").replace("\\", "/")
+ return _cmdline_content.replace("\x00", " ").strip()📝 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.
| def cmdline(self): | |
| """Return command line used to run the process `pid`.""" | |
| cmdline_path = "/proc/{}/cmdline".format(self.pid) | |
| _cmdline_content = self._read_content(cmdline_path) | |
| if _cmdline_content: | |
| return _cmdline_content.replace("\x00", " ").replace("\\", "/") | |
| def cmdline(self): | |
| """Return command line used to run the process `pid`.""" | |
| cmdline_path = "/proc/{}/cmdline".format(self.pid) | |
| _cmdline_content = self._read_content(cmdline_path) | |
| if _cmdline_content: | |
| return _cmdline_content.replace("\x00", " ").strip() |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/process.py around lines 88 to 94, the cmdline()
method currently replaces backslashes with forward slashes which can corrupt
valid arguments; remove the replace("\\", "/") call so the method only
normalizes NUL characters to spaces (i.e., keep _cmdline_content.replace("\x00",
" ") and return that), preserving any original backslashes in the command line.
| @property | ||
| def cwd(self): | ||
| """Return current working dir of process""" | ||
| cwd_path = "/proc/%d/cwd" % int(self.pid) | ||
| return os.readlink(cwd_path) | ||
|
|
There was a problem hiding this comment.
cwd can raise if the process exits or lacks permissions.
Uncaught exceptions here will bubble up and crash callers. Return a safe default.
def cwd(self):
"""Return current working dir of process"""
cwd_path = "/proc/%d/cwd" % int(self.pid)
- return os.readlink(cwd_path)
+ try:
+ return os.readlink(cwd_path)
+ except (FileNotFoundError, PermissionError, ProcessLookupError, OSError):
+ return ""📝 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.
| @property | |
| def cwd(self): | |
| """Return current working dir of process""" | |
| cwd_path = "/proc/%d/cwd" % int(self.pid) | |
| return os.readlink(cwd_path) | |
| @property | |
| def cwd(self): | |
| """Return current working dir of process""" | |
| cwd_path = "/proc/%d/cwd" % int(self.pid) | |
| try: | |
| return os.readlink(cwd_path) | |
| except (FileNotFoundError, PermissionError, ProcessLookupError, OSError): | |
| return "" |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/process.py around lines 95 to 100, the cwd property
calls os.readlink on /proc/<pid>/cwd which can raise (e.g., FileNotFoundError,
PermissionError, OSError) if the process exited or lacks permissions; wrap the
os.readlink call in a try/except that catches these exceptions and return a safe
default (e.g., None or an empty string) instead of letting the exception
propagate, so callers won't crash when the process is gone or inaccessible.
| def wait_for_finish(self): | ||
| """Waits until the process finishes | ||
| This only works if self.pid is a child process of Lutris | ||
| """ | ||
| try: | ||
| pid, ret_status = os.waitpid(int(self.pid) * -1, 0) | ||
| except OSError as ex: | ||
| return -1 | ||
| return ret_status |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
wait_for_finish waits on the wrong target and shadows static analysis.
Using os.waitpid(-pid, 0) waits for any child in the process group, not necessarily self.pid. This can hang or return the wrong exit status. Also ex is unused (Ruff F841).
If the intent is to wait for that specific child process only, switch to os.waitpid(self.pid, 0). If group-wait was intended, please confirm and we can adapt the API to reflect that.
def wait_for_finish(self):
"""Waits until the process finishes
This only works if self.pid is a child process of Lutris
"""
try:
- pid, ret_status = os.waitpid(int(self.pid) * -1, 0)
- except OSError as ex:
- return -1
- return ret_status
+ _, ret_status = os.waitpid(self.pid, 0)
+ except ChildProcessError:
+ return -1
+ except OSError:
+ return -1
+ return ret_statuswait_for_finish waits on the process group — wait on the specific child and remove the unused exception
File: app/src/main/python/gogdl/process.py (lines 130-138)
- os.waitpid(int(self.pid) * -1, 0) waits for any child in the process group (abs(pid)); replace with os.waitpid(int(self.pid), 0) to wait for that specific child (or rename/document if group-wait was intentional).
- Remove the unused exception binding (ex) to fix Ruff F841; catch ChildProcessError for "not a child" and OSError for other failures (or just OSError if preferred).
🧰 Tools
🪛 Ruff (0.12.2)
136-136: Local variable ex is assigned to but never used
Remove assignment to unused variable ex
(F841)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/process.py around lines 130-138, change the waitpid
call to target the specific child by using os.waitpid(int(self.pid), 0) instead
of os.waitpid(int(self.pid) * -1, 0), and remove the unused exception binding;
catch ChildProcessError for the "not a child" case and OSError for other
failures (returning -1 as before) so Ruff F841 is resolved and the function
waits on the intended PID.
| @Transaction | ||
| suspend fun upsertPreservingInstallStatus(games: List<GOGGame>) { | ||
| games.forEach { newGame -> | ||
| val existingGame = getById(newGame.id) | ||
| if (existingGame != null) { | ||
| // Preserve installation status and path from existing game | ||
| val gameToInsert = newGame.copy( | ||
| isInstalled = existingGame.isInstalled, | ||
| installPath = existingGame.installPath, | ||
| ) | ||
| insert(gameToInsert) | ||
| } else { | ||
| // New game, insert as-is | ||
| insert(newGame) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
N+1 database reads in upsert loop; batch-fetch install states
For large libraries this scales poorly. Fetch existing states in one query and upsert using a map.
interface GOGGameDao {
+ // Lightweight projection for state preservation
+ data class InstallState(
+ val id: String,
+ val isInstalled: Boolean,
+ val installPath: String?,
+ )
+
+ @Query("SELECT id, isInstalled, installPath FROM gog_games WHERE id IN (:ids)")
+ suspend fun getInstallStates(ids: List<String>): List<InstallState>
@@
@Transaction
suspend fun upsertPreservingInstallStatus(games: List<GOGGame>) {
- games.forEach { newGame ->
- val existingGame = getById(newGame.id)
- if (existingGame != null) {
- // Preserve installation status and path from existing game
- val gameToInsert = newGame.copy(
- isInstalled = existingGame.isInstalled,
- installPath = existingGame.installPath,
- )
- insert(gameToInsert)
- } else {
- // New game, insert as-is
- insert(newGame)
- }
- }
+ if (games.isEmpty()) return
+ val states = getInstallStates(games.map { it.id }).associateBy { it.id }
+ insertAll(
+ games.map { g ->
+ states[g.id]?.let { s ->
+ g.copy(isInstalled = s.isInstalled, installPath = s.installPath)
+ } ?: g
+ }
+ )
}📝 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.
| @Transaction | |
| suspend fun upsertPreservingInstallStatus(games: List<GOGGame>) { | |
| games.forEach { newGame -> | |
| val existingGame = getById(newGame.id) | |
| if (existingGame != null) { | |
| // Preserve installation status and path from existing game | |
| val gameToInsert = newGame.copy( | |
| isInstalled = existingGame.isInstalled, | |
| installPath = existingGame.installPath, | |
| ) | |
| insert(gameToInsert) | |
| } else { | |
| // New game, insert as-is | |
| insert(newGame) | |
| } | |
| } | |
| } | |
| @Transaction | |
| suspend fun upsertPreservingInstallStatus(games: List<GOGGame>) { | |
| if (games.isEmpty()) return | |
| val states = getInstallStates(games.map { it.id }).associateBy { it.id } | |
| insertAll( | |
| games.map { g -> | |
| states[g.id]?.let { s -> | |
| g.copy(isInstalled = s.isInstalled, installPath = s.installPath) | |
| } ?: g | |
| } | |
| ) | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/db/dao/GOGGameDao.kt around lines 58 to 74,
the current upsertPreservingInstallStatus does an N+1 read by calling getById
inside the loop; replace it with a single batch fetch of existing games by IDs
(e.g., getByIds(ids): List<GOGGame>), build a Map<Id, GOGGame> of existing
states, then iterate the input list and for each newGame look up its existing
entry from the map, copy preserve isInstalled and installPath when present, and
perform the insert/upsert; keep the method @Transaction and ensure you use a
batch insert/upsert if available to avoid per-item DB writes.
| @@ -0,0 +1,27 @@ | |||
| package app.gamenative.data | |||
|
|
|||
| import app.gamenative.enums.AppType | |||
There was a problem hiding this comment.
Wrong import for AppType; will not compile if AppType lives under data.
The rest of the codebase references app.gamenative.data.AppType. Adjust import.
- import app.gamenative.enums.AppType
+ import app.gamenative.data.AppType📝 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.
| import app.gamenative.enums.AppType | |
| import app.gamenative.data.AppType |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GOG/GOGGameWrapper.kt around line 3,
the import currently references app.gamenative.enums.AppType but AppType
actually resides in app.gamenative.data; update the import to
app.gamenative.data.AppType so the file compiles and matches other references
across the codebase.
| // Track if background sync is already running | ||
| private var backgroundSyncInProgress = false | ||
|
|
There was a problem hiding this comment.
Race: backgroundSyncInProgress is not atomic; duplicate syncs can start
Two callers can pass the check before the flag is set inside launch. Use AtomicBoolean or Mutex and reset in finally.
import timber.log.Timber
+import java.util.concurrent.atomic.AtomicBoolean
@@
- // Track if background sync is already running
- private var backgroundSyncInProgress = false
+ // Track if background sync is already running
+ private val backgroundSyncInProgress = AtomicBoolean(false)
@@
- if (backgroundSyncInProgress) {
+ if (!backgroundSyncInProgress.compareAndSet(false, true)) {
Timber.i("Background GOG sync already in progress, skipping")
return Result.failure(Exception("Background sync already in progress"))
}
@@
- scope.launch {
- backgroundSyncInProgress = true
- syncLibraryInBackground(context, clearExisting)
- backgroundSyncInProgress = false
- }
+ scope.launch {
+ try {
+ syncLibraryInBackground(context, clearExisting)
+ } finally {
+ backgroundSyncInProgress.set(false)
+ }
+ }Also applies to: 22-27, 41-46
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GOG/GOGLibraryManager.kt around
lines 15-17 (also apply same fix to lines 22-27 and 41-46): the
backgroundSyncInProgress boolean is not atomic and allows a race where multiple
syncs start; replace it with an AtomicBoolean (or protect with a Mutex) and use
compareAndSet(false, true) to atomically claim the right to start a sync, or
acquire the mutex before checking/starting; ensure the flag is always cleared in
a finally block (or release the mutex) after the sync completes or on error so
duplicate syncs cannot run concurrently.
| val prefixToPath: (String) -> String = { prefix -> | ||
| PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID) | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid NPE: don’t force unwrap SteamService.userSteamId
userSteamId!! will crash if the user isn’t signed in/restored. Guard it and abort gracefully.
- val prefixToPath: (String) -> String = { prefix ->
- PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID)
- }
+ val accountId = SteamService.userSteamId?.accountID
+ ?: run {
+ Timber.w("Steam user session missing; aborting save sync for $gameId")
+ return@withContext PostSyncInfo(app.gamenative.enums.SyncResult.UnknownFail)
+ }
+ 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, gameId, SteamService.userSteamId!!.accountID) | |
| } | |
| val accountId = SteamService.userSteamId?.accountID | |
| ?: run { | |
| Timber.w("Steam user session missing; aborting save sync for $gameId") | |
| return@withContext PostSyncInfo(app.gamenative.enums.SyncResult.UnknownFail) | |
| } | |
| val prefixToPath: (String) -> String = { prefix -> | |
| PathType.from(prefix).toAbsPath(context, gameId, accountId) | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/Steam/SteamGameManager.kt around
lines 118-121, avoid force-unwrapping SteamService.userSteamId to prevent an
NPE: capture the accountID into a local nullable val (val accountId =
SteamService.userSteamId?.accountID) and if it's null log/handle and return or
otherwise abort gracefully before creating the prefixToPath lambda; then use
that non-null accountId when building paths. Ensure you do not use !! anywhere
here and provide a clear early exit or error path when the user is not signed
in/restored.
| if (appLaunchInfo == null) { | ||
| return "\"wfm.exe\"" | ||
| } | ||
|
|
||
| // Check if we should launch through real Steam | ||
| if (container.isLaunchRealSteam()) { | ||
| // Launch Steam with the applaunch parameter to start the game | ||
| return "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " + | ||
| "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $appId" | ||
| } |
There was a problem hiding this comment.
Wrong app id passed to real Steam -applaunch
You’re passing the container appId (e.g., steam-12345) instead of the numeric Steam appId. This will fail to launch.
- if (container.isLaunchRealSteam()) {
- // Launch Steam with the applaunch parameter to start the game
- return "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " +
- "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $appId"
- }
+ if (container.isLaunchRealSteam()) {
+ val steamAppId = ContainerUtils.extractGameIdFromContainerId(appId)
+ return "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " +
+ "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $steamAppId"
+ }🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/Steam/SteamGameManager.kt around
lines 171-180, the code passes the container appId (like "steam-12345") into
Steam's -applaunch which expects the numeric Steam app id; replace the container
app id with the numeric Steam id sourced from appLaunchInfo (e.g.,
appLaunchInfo.steamAppId or a parsed numeric id from appLaunchInfo.id) and use
that numeric value in the applaunch parameter so Steam receives a plain number
(or fallback to extracting digits with a regex if the numeric field isn't
present).
| import subprocess | ||
| import time | ||
| from gogdl.dl.dl_utils import get_case_insensitive_name | ||
| from ctypes import * |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid star import from ctypes; fix cdll usage.
from ctypes import * risks undefined names and lint errors. Import ctypes and use ctypes.CDLL.
-from ctypes import *
+import ctypes
@@
- libc = cdll.LoadLibrary("libc.so.6")
- prctl = libc.prctl
+ libc = ctypes.CDLL("libc.so.6")
+ prctl = libc.prctlAlso applies to: 167-170
🧰 Tools
🪛 Ruff (0.12.2)
7-7: from ctypes import * used; unable to detect undefined names
(F403)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/launch.py around line 7 (and also apply the same
change at lines 167-170), replace the star import "from ctypes import *" with
"import ctypes" and update all usages of cdll/unnamed CDLL references to
explicitly call ctypes.CDLL (or ctypes.cdll.LoadLibrary where appropriate),
ensuring any previously unqualified names from ctypes are prefixed with
"ctypes." to avoid undefined names and lint errors.
| def reap_children(): | ||
| nonlocal status | ||
| while True: | ||
| try: | ||
| child_pid, child_returncode, _resource_usage = os.wait3(os.WNOHANG) | ||
| except ChildProcessError: | ||
| raise NoMoreChildren from None # No processes remain. | ||
| if child_pid == process_pid: | ||
| status = child_returncode | ||
|
|
||
| if child_pid == 0: | ||
| break | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exit code handling: use waitstatus_to_exitcode (or decode) before sys.exit.
status from wait3 is a wait status, not an exit code. Returning it directly can yield bogus codes.
- if child_pid == process_pid:
- status = child_returncode
+ if child_pid == process_pid:
+ status = child_returncode
@@
- sys.exit(status)
+ # Normalize status
+ if isinstance(status, int):
+ try:
+ exit_code = os.waitstatus_to_exitcode(status) # Py3.9+
+ except AttributeError:
+ exit_code = (status >> 8) & 0xFF # best-effort
+ else:
+ exit_code = 0
+ sys.exit(exit_code)Also applies to: 235-245
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/launch.py around lines 210-222, the code assigns
the raw wait status returned by os.wait3 to status and later uses it as an exit
code; you must convert the wait status to a real exit code before using or
passing to sys.exit. Replace the direct assignment with decoding the wait status
(prefer os.waitstatus_to_exitcode(child_returncode) on Python 3.9+, or fall back
to checking os.WIFEXITED/ os.WEXITSTATUS and os.WIFSIGNALED/ os.WTERMSIG to
derive a proper exit code) and set status to that decoded exit code; apply the
same change to the similar block around lines 235-245.
| @dataclass | ||
| class AddressCache: | ||
| s_near = CodeTable.near_modes | ||
| s_same = CodeTable.same_modes | ||
| next_slot = 0 | ||
| near_array = [0 for _ in range(s_near)] | ||
| same_array = [0 for _ in range(s_same * 256)] | ||
|
|
||
| def update(self, addr): | ||
| self.near_array[self.next_slot] = addr | ||
| self.next_slot = (self.next_slot + 1) % self.s_near | ||
|
|
||
| self.same_array[addr % (self.s_same*256)] = addr | ||
|
|
There was a problem hiding this comment.
AddressCache fields should be per-instance, not shared class attributes.
As written, near_array/same_array/next_slot are shared across all instances. Make them instance fields.
-@dataclass
-class AddressCache:
- s_near = CodeTable.near_modes
- s_same = CodeTable.same_modes
- next_slot = 0
- near_array = [0 for _ in range(s_near)]
- same_array = [0 for _ in range(s_same * 256)]
+from dataclasses import field
+
+@dataclass
+class AddressCache:
+ s_near: int = CodeTable.near_modes
+ s_same: int = CodeTable.same_modes
+ next_slot: int = 0
+ near_array: list[int] = field(default_factory=lambda: [0] * CodeTable.near_modes)
+ same_array: list[int] = field(default_factory=lambda: [0] * (CodeTable.same_modes * 256))📝 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.
| @dataclass | |
| class AddressCache: | |
| s_near = CodeTable.near_modes | |
| s_same = CodeTable.same_modes | |
| next_slot = 0 | |
| near_array = [0 for _ in range(s_near)] | |
| same_array = [0 for _ in range(s_same * 256)] | |
| def update(self, addr): | |
| self.near_array[self.next_slot] = addr | |
| self.next_slot = (self.next_slot + 1) % self.s_near | |
| self.same_array[addr % (self.s_same*256)] = addr | |
| from dataclasses import field | |
| @dataclass | |
| class AddressCache: | |
| s_near: int = CodeTable.near_modes | |
| s_same: int = CodeTable.same_modes | |
| next_slot: int = 0 | |
| near_array: list[int] = field(default_factory=lambda: [0] * CodeTable.near_modes) | |
| same_array: list[int] = field(default_factory=lambda: [0] * (CodeTable.same_modes * 256)) | |
| def update(self, addr): | |
| self.near_array[self.next_slot] = addr | |
| self.next_slot = (self.next_slot + 1) % self.s_near | |
| self.same_array[addr % (self.s_same*256)] = addr |
🧰 Tools
🪛 Ruff (0.12.2)
50-50: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
51-51: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/xdelta/objects.py around lines 45-58, the
AddressCache currently defines s_near, s_same, next_slot, near_array and
same_array as class attributes so they are shared across all instances; change
them to per-instance dataclass fields: import and use dataclasses.field and
annotate types, set s_near and s_same to CodeTable.near_modes and
CodeTable.same_modes (or assign them in __post_init__ if needed), make next_slot
an int default 0, make near_array a list with default_factory that creates
[0]*self.s_near (or create in __post_init__), and make same_array a list with
default_factory sized to self.s_same*256 (or initialize in __post_init__);
ensure update uses self.* fields. Ensure no mutable defaults are at class scope.
| print("OVERLAP NOT IMPLEMENTED") | ||
| raise Exception("OVERLAP") | ||
| halfinst.type = objects.XD3_NOOP |
There was a problem hiding this comment.
CPY overlap not implemented – patches can fail
VCDIFF often requires target-overlap copies. Implement copying from the already-decoded target buffer, not just source, to avoid runtime errors.
I can provide an overlap-safe copy implementation based on RFC 3284 semantics.
🧰 Tools
🪛 Ruff (0.12.2)
76-76: Create your own exception
(TRY002)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/xdelta/patcher.py around lines 75-77, the code
currently raises an exception for target-overlap copies; replace that with an
RFC 3284-compliant overlap-safe copy from the already-decoded target buffer.
Detect when the copy source is within the target buffer (not source), compute
source and dest offsets into the target buffer, and perform a memmove-style
copy: if src < dest and regions overlap, copy bytes from the end backwards;
otherwise copy forwards; handle non-overlapping copies normally. After
performing the copy, remove the print/raise and set halfinst.type =
objects.XD3_NOOP as intended.
| src_handle = open(source, 'rb') | ||
| patch_handle = open(patch, 'rb') | ||
| dst_handle = open(out, 'wb') | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure files are closed on exceptions
Use context managers for file handles.
-def patch(source: str, patch: str, out: str, speed_queue: Queue):
- src_handle = open(source, 'rb')
- patch_handle = open(patch, 'rb')
- dst_handle = open(out, 'wb')
+def patch(source: str, patch: str, out: str, speed_queue: Queue):
+ with open(source, 'rb') as src_handle, \
+ open(patch, 'rb') as patch_handle, \
+ open(out, 'wb') as dst_handle:
+ # ... keep existing logic, but remove explicit close/flush at the end
@@
- dst_handle.flush()
- src_handle.close()
- patch_handle.close()
- dst_handle.close()
+ # handles are auto-closed by context managersAlso applies to: 199-203
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/xdelta/patcher.py around lines 81-84 (and similarly
199-203), the file handles are opened without context managers so they won’t be
reliably closed on exceptions; refactor to use with open(source, 'rb') as
src_handle, with open(patch, 'rb') as patch_handle, and with open(out, 'wb') as
dst_handle (nest or combine as needed) and move all processing that uses those
handles inside the with-block(s) so files are automatically closed even if an
exception occurs.
| /** | ||
| * Returns the app info for the given game (steam only, should be refactored) | ||
| */ | ||
| fun getAppInfo(libraryItem: LibraryItem): SteamApp? | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove Steam-specific type from a cross-platform interface
Exposing SteamApp in a source-agnostic GameManager leaks Steam concerns and forces GOG implementations to depend on Steam models. Introduce a source-agnostic PlatformAppInfo (sealed interface with Steam/GOG variants) or move this API behind per-source managers and expose it via GameManagerService when needed.
Example refactor of the signature:
- fun getAppInfo(libraryItem: LibraryItem): SteamApp?
+ fun getAppInfo(libraryItem: LibraryItem): PlatformAppInfo?Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GameManager.kt around lines 69-73,
the method signature leaks the Steam-specific SteamApp type from a
cross-platform GameManager; replace this by either (A) introducing a
source-agnostic sealed interface PlatformAppInfo with concrete Steam and GOG
implementations and change the method to return PlatformAppInfo? (update all
callers and implementations to map SteamApp -> PlatformAppInfo.Steam variant and
GOG -> PlatformAppInfo.Gog variant), or (B) move the Steam-specific API behind a
per-source manager (e.g., SteamGameManager.getAppInfo returning SteamApp) and
expose only source-agnostic methods from GameManagerService; update imports,
implementations, and call sites accordingly so no Steam model is referenced from
GameManager.
🛠️ Refactor suggestion
Steam-specific type leaks into a cross-platform interface
SteamApp? in getAppInfo couples the interface to Steam and forces non-Steam impls to return null. Extract a platform-agnostic AppInfo model or move this method to Steam-only APIs.
I can sketch a minimal AppInfo interface and migration steps if helpful.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GameManager.kt around lines 69-73,
the method return type SteamApp? leaks a Steam-specific type into a
cross-platform interface; replace it with a platform-agnostic AppInfo (new data
class or interface) that captures the minimal fields consumers need, update
GameManager.getAppInfo(libraryItem: LibraryItem): AppInfo?, and then update the
Steam-specific implementation to map SteamApp -> AppInfo (or provide an
adapter). Alternatively, move the Steam-only method into a SteamService
interface and keep GameManager cross-platform; ensure all callers are updated to
use the new AppInfo or SteamService and mark/remove the old SteamApp return
signature.
| /** | ||
| * Returns the install info dialog for the given game | ||
| */ | ||
| fun getInstallInfoDialog(context: Context, libraryItem: LibraryItem): MessageDialogState | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Decouple presentation from domain
Returning MessageDialogState directly from a service interface couples domain/services to Compose UI state. Return a data model (e.g., InstallInfo) and let the UI map it to MessageDialogState.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GameManager.kt around lines 125 to
129, the service method currently returns a Compose UI type MessageDialogState
which couples the domain/service layer to presentation; change the method
signature to return a plain domain/data model (e.g., an InstallInfo data class
containing the dialog title, message/body, button labels and any action
identifiers) instead of MessageDialogState, remove any Compose imports from the
service, implement the InstallInfo data class in a domain-safe package, update
all GameManager implementations to build and return InstallInfo, and update
callers in the UI layer to map InstallInfo -> MessageDialogState (or create the
MessageDialogState) so presentation concerns remain in the UI.
| try { | ||
| // Check if another download is already in progress | ||
| if (GOGService.hasActiveDownload()) { | ||
| return Result.failure(Exception("Another GOG game is already downloading. Please wait for it to finish before starting a new download.")) | ||
| } | ||
|
|
||
| // Check authentication first | ||
| if (!GOGService.hasStoredCredentials(context)) { | ||
| return Result.failure(Exception("GOG authentication required. Please log in to your GOG account first.")) | ||
| } | ||
|
|
||
| // Validate credentials and refresh if needed | ||
| val validationResult = runBlocking { GOGService.validateCredentials(context) } | ||
| if (!validationResult.isSuccess || !validationResult.getOrDefault(false)) { | ||
| return Result.failure(Exception("GOG authentication is invalid. Please re-authenticate.")) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use async/await instead of runBlocking for credential validation
Using runBlocking within a suspend function blocks the current thread unnecessarily. Since downloadGame is already marked as a regular function, consider either making it a suspend function or using coroutine scope properly.
- override fun downloadGame(context: Context, libraryItem: LibraryItem): Result<DownloadInfo?> {
+ override suspend fun downloadGame(context: Context, libraryItem: LibraryItem): Result<DownloadInfo?> {
try {
// Check if another download is already in progress
if (GOGService.hasActiveDownload()) {
return Result.failure(Exception("Another GOG game is already downloading. Please wait for it to finish before starting a new download."))
}
// Check authentication first
if (!GOGService.hasStoredCredentials(context)) {
return Result.failure(Exception("GOG authentication required. Please log in to your GOG account first."))
}
// Validate credentials and refresh if needed
- val validationResult = runBlocking { GOGService.validateCredentials(context) }
+ val validationResult = GOGService.validateCredentials(context)
if (!validationResult.isSuccess || !validationResult.getOrDefault(false)) {
return Result.failure(Exception("GOG authentication is invalid. Please re-authenticate."))
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GOG/GOGGameManager.kt around lines
56 to 72, replace the blocking use of runBlocking for credential validation:
either make downloadGame a suspend function and call
GOGService.validateCredentials(context) directly (awaiting its result in the
coroutine), or keep downloadGame synchronous but call validation via a
CoroutineScope (e.g., withContext(Dispatchers.IO) or a structured scope) to
perform the async validation and suspend only that portion; ensure the chosen
approach avoids blocking the calling thread and properly propagates the
validation result to decide whether to return a failure.
| Timber.i("Starting GOG game installation: ${libraryItem.name} to $installPath") | ||
|
|
||
| // Use the new download method that returns DownloadInfo | ||
| val result = runBlocking { GOGService.downloadGame(libraryItem.appId, installPath, authConfigPath) } |
There was a problem hiding this comment.
Avoid runBlocking in a synchronous function
The use of runBlocking here will block the calling thread. If the function must remain synchronous per the interface, consider spawning a coroutine with proper scope management.
- val result = runBlocking { GOGService.downloadGame(libraryItem.appId, installPath, authConfigPath) }
+ // Consider using a proper coroutine scope or making this function suspend
+ val result = CoroutineScope(Dispatchers.IO).async {
+ GOGService.downloadGame(libraryItem.appId, installPath, authConfigPath)
+ }.await()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GOG/GOGGameManager.kt around line
79, the call uses runBlocking which blocks the calling thread; instead launch
the download in a proper coroutine scope (e.g., a class-level CoroutineScope or
an injected scope) and run GOGService.downloadGame asynchronously, or if the API
must stay synchronous return a Future/Deferred or use blocking-friendly
dispatcher by executing the suspend call via scope.runBlocking in a dedicated IO
thread (not the caller thread); update the method to either launch and manage
the coroutine lifecycle (cancel on cleanup) or adapt the signature to return a
suspend/CompletableFuture/Job so the caller can handle async execution.
| val game = runBlocking { getGameById(gameId) } | ||
| if (game != null) { | ||
| val updatedGame = game.copy( | ||
| isInstalled = false, | ||
| installPath = "", | ||
| ) | ||
| runBlocking { gogGameDao.update(updatedGame) } | ||
| } |
There was a problem hiding this comment.
Multiple runBlocking calls create performance bottlenecks
The deleteGame and isGameInstalled methods contain multiple runBlocking calls that will block the calling thread. This pattern appears throughout the file and should be refactored to use proper coroutine patterns.
For deleteGame, consider making it a suspend function:
- override fun deleteGame(context: Context, libraryItem: LibraryItem): Result<Unit> {
+ override suspend fun deleteGame(context: Context, libraryItem: LibraryItem): Result<Unit> {
try {
val gameId = libraryItem.gameId.toString()
val installPath = getGameInstallPath(context, gameId, libraryItem.name)
val installDir = File(installPath)
// ... existing code ...
- val game = runBlocking { getGameById(gameId) }
+ val game = getGameById(gameId)Similarly for isGameInstalled:
- override fun isGameInstalled(context: Context, libraryItem: LibraryItem): Boolean {
+ override suspend fun isGameInstalled(context: Context, libraryItem: LibraryItem): Boolean {
try {
// ... existing code ...
- val game = runBlocking { getGameById(gameId) }
+ val game = getGameById(gameId)Also applies to: 177-184, 207-215
| else: | ||
| results_queue.put(DownloadTaskResult(False, FailReason.CHECKSUM, task)) | ||
| return |
There was a problem hiding this comment.
Incorrect failure reason after retry exhaustion
After retries are exhausted, the failure reason is set to CHECKSUM, but it should be CONNECTION since the loop exits due to connection failures.
else:
- results_queue.put(DownloadTaskResult(False, FailReason.CHECKSUM, task))
+ results_queue.put(DownloadTaskResult(False, FailReason.CONNECTION, task))
return📝 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.
| else: | |
| results_queue.put(DownloadTaskResult(False, FailReason.CHECKSUM, task)) | |
| return | |
| else: | |
| results_queue.put(DownloadTaskResult(False, FailReason.CONNECTION, task)) | |
| return |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/workers/task_executor.py around lines 160-162,
the code reports a failed download with FailReason.CHECKSUM after retry
exhaustion, but the loop exits due to connection failures; change the failure
result to use FailReason.CONNECTION (i.e., replace FailReason.CHECKSUM with
FailReason.CONNECTION) so the correct failure reason is returned when retries
are exhausted due to connection errors.
| if type(urls) == str: | ||
| url = urls | ||
| else: | ||
| endpoint = deepcopy(urls[0]) | ||
| endpoint["parameters"]["path"] += "/main.bin" | ||
| url = dl_utils.merge_url_with_params( | ||
| endpoint["url_format"], endpoint["parameters"] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use isinstance() for type checking
Consistent with earlier feedback about type checking.
- if type(urls) == str:
+ if isinstance(urls, str):
url = urls📝 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 type(urls) == str: | |
| url = urls | |
| else: | |
| endpoint = deepcopy(urls[0]) | |
| endpoint["parameters"]["path"] += "/main.bin" | |
| url = dl_utils.merge_url_with_params( | |
| endpoint["url_format"], endpoint["parameters"] | |
| ) | |
| if isinstance(urls, str): | |
| url = urls | |
| else: | |
| endpoint = deepcopy(urls[0]) | |
| endpoint["parameters"]["path"] += "/main.bin" | |
| url = dl_utils.merge_url_with_params( | |
| endpoint["url_format"], endpoint["parameters"] | |
| ) |
🧰 Tools
🪛 Ruff (0.12.2)
187-187: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/workers/task_executor.py around lines 187 to
194, the code uses type(urls) == str for string checking; change this to use
isinstance(urls, str) to follow Python best practices and maintain correct
behavior for subclasses of str, leaving the rest of the branch logic unchanged.
| else: | ||
| results_queue.put(DownloadTaskResult(False, FailReason.CHECKSUM, task)) | ||
| return |
There was a problem hiding this comment.
Incorrect failure reason for V1 download
Same issue as V2 - after connection retries are exhausted, the failure should be CONNECTION, not CHECKSUM.
else:
- results_queue.put(DownloadTaskResult(False, FailReason.CHECKSUM, task))
+ results_queue.put(DownloadTaskResult(False, FailReason.CONNECTION, task))
return📝 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.
| else: | |
| results_queue.put(DownloadTaskResult(False, FailReason.CHECKSUM, task)) | |
| return | |
| else: | |
| results_queue.put(DownloadTaskResult(False, FailReason.CONNECTION, task)) | |
| return |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/workers/task_executor.py around lines 230-232,
the code reports FailReason.CHECKSUM when connection retries are exhausted for
V1 downloads; change the failure result to use FailReason.CONNECTION instead
(i.e., replace DownloadTaskResult(False, FailReason.CHECKSUM, task) with
DownloadTaskResult(False, FailReason.CONNECTION, task)) and run tests to ensure
other retry-exhaustion paths consistently report CONNECTION.
|
|
||
| def sync(self, arguments, unknown_args): | ||
| try: | ||
| prefered_action = getattr(arguments, 'prefered_action', None) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable prefered_action
The variable is assigned but never used.
- prefered_action = getattr(arguments, 'prefered_action', None)
self.sync_path = os.path.normpath(arguments.path.strip('"'))📝 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.
| prefered_action = getattr(arguments, 'prefered_action', None) | |
| self.sync_path = os.path.normpath(arguments.path.strip('"')) |
🧰 Tools
🪛 Ruff (0.12.2)
99-99: Local variable prefered_action is assigned to but never used
Remove assignment to unused variable prefered_action
(F841)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/saves.py around line 99 the variable assignment
"prefered_action = getattr(arguments, 'prefered_action', None)" creates an
unused variable; remove this line to eliminate dead code (or if the intent was
to use it, replace with the correct usage and spelling "preferred_action" where
needed), but if no usage is required simply delete the assignment.
| self.client_id, self.client_secret = self.get_auth_ids() | ||
| self.get_auth_token() | ||
| except Exception as e: | ||
| self.logger.error(f"Authentication failed: {e}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use logging.exception for better error tracking
When catching exceptions in error handlers, use logging.exception to preserve the stack trace.
For example:
except Exception as e:
- self.logger.error(f"Authentication failed: {e}")
+ self.logger.exception("Authentication failed")
returnApply similar changes to all the error logging instances.
Also applies to: 139-139, 149-149, 161-161, 178-178, 186-186
🧰 Tools
🪛 Ruff (0.12.2)
131-131: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/saves.py around lines 131, 139, 149, 161, 178, and
186, the exception handlers call self.logger.error(f"...{e}") which drops the
traceback; replace those calls with self.logger.exception("Authentication
failed") (or an appropriate short message per handler) so the logger captures
the full stack trace; keep the existing message text but move dynamic error
details out of the formatted string and rely on logger.exception to record the
exception context for each of the listed lines.
There was a problem hiding this comment.
Actionable comments posted: 97
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
app/build.gradle.kts (1)
80-85: Proguard files in defaultConfig may not apply to release types. Move to buildTypes.To guarantee R8 uses these files for release(-signed/-gold), set proguardFiles on each buildType.
Apply:
- defaultConfig { - ... - proguardFiles( - getDefaultProguardFile("proguard-android.txt"), - "proguard-rules.pro", - ) - } + defaultConfig { ... } buildTypes { release { isMinifyEnabled = true isShrinkResources = true signingConfig = signingConfigs.getByName("debug") + proguardFiles( + getDefaultProguardFile("proguard-android.txt"), + "proguard-rules.pro", + ) } create("release-signed") { isMinifyEnabled = true isShrinkResources = true signingConfig = signingConfigs.getByName("pluvia") + proguardFiles( + getDefaultProguardFile("proguard-android.txt"), + "proguard-rules.pro", + ) } create("release-gold") { isMinifyEnabled = true isShrinkResources = true signingConfig = signingConfigs.getByName("pluvia") applicationIdSuffix = ".gold" buildConfigField("boolean", "GOLD", "true") val iconValue = "@mipmap/ic_launcher_gold" val iconRoundValue = "@mipmap/ic_launcher_gold_round" manifestPlaceholders.putAll(mapOf("icon" to iconValue, "roundIcon" to iconRoundValue)) + proguardFiles( + getDefaultProguardFile("proguard-android.txt"), + "proguard-rules.pro", + ) } }app/src/main/java/app/gamenative/data/DownloadInfo.kt (1)
45-53: Add bounds checks and clamp progress to [0, 1].Prevent crashes and invalid progress with basic validation.
fun setProgress(amount: Float, jobIndex: Int = 0) { - progresses[jobIndex] = amount + require(jobIndex in progresses.indices) { + "jobIndex=$jobIndex out of range [0, ${progresses.size})" + } + val clamped = amount.coerceIn(0f, 1f) + progresses[jobIndex] = clamped emitProgressChange() } fun setWeight(jobIndex: Int, weightBytes: Long) { // tiny helper - weights[jobIndex] = weightBytes.toFloat() + require(jobIndex in weights.indices) { + "jobIndex=$jobIndex out of range [0, ${weights.size})" + } + weights[jobIndex] = weightBytes.toFloat() weightSum = weights.sum() }app/src/main/java/app/gamenative/MainActivity.kt (2)
176-197: External launch is gated on Steam login only — blocks GOG launches when Steam is logged outWith multi-source IDs (String), emitting based solely on
SteamService.isLoggedIncan suppress valid GOG launches. Emit the event unconditionally (let downstream handle source/auth), or gate by the item’s source.Minimal fix:
- if (SteamService.isLoggedIn) { - Timber.d("[MainActivity]: User already logged in, emitting ExternalGameLaunch event immediately") - lifecycleScope.launch { - PluviaApp.events.emit(AndroidEvent.ExternalGameLaunch(launchRequest.appId)) - } - // Apply config override if present - launchRequest.containerConfig?.let { config -> - IntentLaunchManager.applyTemporaryConfigOverride(this, launchRequest.appId, config) - } - } else { - // Store the launch request to be processed after login - setPendingLaunchRequest(launchRequest) - Timber.d("[MainActivity]: User not logged in, stored pending launch request for app ${launchRequest.appId}") - } + // Emit regardless of Steam login; consumers decide how to handle per-source auth/launch. + lifecycleScope.launch { + PluviaApp.events.emit(AndroidEvent.ExternalGameLaunch(launchRequest.appId)) + } + // Apply config override if present + launchRequest.containerConfig?.let { config -> + IntentLaunchManager.applyTemporaryConfigOverride(this, launchRequest.appId, config) + }
176-197: Add source-aware login gating or expose GOG parity APIs
- Verified: GOGService exists (app/src/main/java/app/gamenative/service/GOG/GOGService.kt) but does NOT expose isLoggedIn or isLoginInProgress. It exposes hasStoredCredentials(context), validateCredentials(context), isRunning(), hasActiveOperations(), isSyncInProgress(), etc.
- Issue: MainActivity.handleLaunchIntent currently checks only SteamService.isLoggedIn (app/src/main/java/app/gamenative/MainActivity.kt:176-197), so GOG external launches aren't gated the same way.
- Action: Either add parity getters on GOGService (isLoggedIn / isLoginInProgress) or make MainActivity source-aware: for Steam use SteamService.isLoggedIn; for GOG use hasStoredCredentials + validateCredentials (or isRunning/hasActiveOperations) to decide immediate emit vs store pendingLaunchRequest.
app/src/main/java/app/gamenative/db/PluviaDatabase.kt (1)
33-45: Add a proper Room migration for v3→v4 — do not ship with destructive fallbackPluviaDatabase was bumped to version 4; app/src/main/java/app/gamenative/di/DatabaseModule.kt currently uses fallbackToDestructiveMigration(), which will drop user data on upgrade. Replace that with addMigrations(MIGRATION_3_4) and remove the destructive fallback before production. I will draft MIGRATION_3_4 once you confirm the exact GOGGame table/columns.
Locations: app/src/main/java/app/gamenative/db/PluviaDatabase.kt (lines 33–45), app/src/main/java/app/gamenative/di/DatabaseModule.kt (lines 20–25)
app/src/main/java/app/gamenative/ui/screen/library/LibraryAppScreen.kt (1)
702-813: Gate Steam-only menu items for non‑Steam titles and null Steam login.ForceCloudSync/ForceDownloadRemote/ForceUploadLocal depend on Steam and will crash on GOG (e.g., userSteamId!!). Only show when source = STEAM and Steam logged in.
Minimal approach: wrap these three AppMenuOption entries in a spread block:
- AppMenuOption( - AppOptionMenuType.ForceCloudSync, - onClick = { /* existing */ }, - ), - AppMenuOption( - AppOptionMenuType.ForceDownloadRemote, - onClick = { /* existing */ }, - ), - AppMenuOption( - AppOptionMenuType.ForceUploadLocal, - onClick = { /* existing */ }, - ), + *( + if (libraryItem.gameSource == GameSource.STEAM && SteamService.userSteamId != null) + arrayOf( + AppMenuOption(AppOptionMenuType.ForceCloudSync, onClick = { /* existing */ }), + AppMenuOption(AppOptionMenuType.ForceDownloadRemote, onClick = { /* existing */ }), + AppMenuOption(AppOptionMenuType.ForceUploadLocal, onClick = { /* existing */ }), + ) + else emptyArray() + ),Also replace SteamService.userSteamId!! with a null-safe check where still referenced.
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt (2)
292-299: Protect against null xServerView in lifecycle callbacks.These force-unwrapped calls can crash if events fire before view init.
- exit(xServerView!!.getxServer().winHandler, PluviaApp.xEnvironment, onExit) + xServerView?.let { exit(it.getxServer().winHandler, PluviaApp.xEnvironment, onExit) } ?: onExit() @@ - exit(xServerView!!.getxServer().winHandler, PluviaApp.xEnvironment, onExit) + xServerView?.let { exit(it.getxServer().winHandler, PluviaApp.xEnvironment, onExit) } ?: onExit()Also applies to: 345-353
256-266: Avoid out-of-bounds on profiles[2].Index 2 assumes ≥3 profiles. Use getOrNull fallback.
- } else { - profiles[2] - } + } else { + profiles.getOrNull(2) ?: profiles.first() + }- } else { - profiles[2] - } + } else { + profiles.getOrNull(2) ?: profiles.first() + }Also applies to: 637-644
app/src/main/java/com/winlator/container/ContainerManager.java (2)
196-234: Potential race condition in container duplication.The
generateUniqueContainerIdmethod checks for container existence and then creates the container, but another thread could create a container with the same ID between the check and creation.Consider making the ID generation and directory creation atomic:
-String newId = generateUniqueContainerId(baseId); - -File dstDir = new File(homeDir, ImageFs.USER+"-"+newId); -if (!dstDir.mkdirs()) return; +String newId; +File dstDir; +synchronized(containers) { + newId = generateUniqueContainerId(baseId); + dstDir = new File(homeDir, ImageFs.USER+"-"+newId); + if (!dstDir.mkdirs()) return; + // Reserve the ID immediately + Container placeholder = new Container(newId); + placeholder.setRootDir(dstDir); + containers.add(placeholder); +}Then update the container list at the end:
-containers.add(dstContainer); +synchronized(containers) { + // Remove placeholder and add real container + containers.removeIf(c -> c.id.equals(newId)); + containers.add(dstContainer); +}
166-193: createContainer method inconsistently validates input.The method modifies the input JSONObject by adding an "id" field but doesn't validate if the ID already exists or if the directory creation truly succeeded (mkdirs returns false for existing directories too).
Add validation and make the method more robust:
public Container createContainer(String containerId, JSONObject data) { try { + // Validate container ID doesn't already exist + if (hasContainer(containerId)) { + Log.e("ContainerManager", "Container with ID " + containerId + " already exists"); + return null; + } + data.put("id", containerId); File containerDir = new File(homeDir, ImageFs.USER+"-"+containerId); - if (!containerDir.mkdirs()) return null; + // Check if directory already exists or creation failed + if (containerDir.exists()) { + Log.e("ContainerManager", "Container directory already exists: " + containerDir); + return null; + } + if (!containerDir.mkdirs()) { + Log.e("ContainerManager", "Failed to create container directory: " + containerDir); + return null; + }app/src/main/java/app/gamenative/ui/PluviaMain.kt (1)
847-884: Always hide the loading dialog even on exceptions/cancellation.Wrap preLaunchApp’s coroutine body in try/finally; also ensure ImageFsInstaller and container setup failures surface a message instead of leaving the spinner up.
fun preLaunchApp( @@ - setLoadingDialogVisible(true) + setLoadingDialogVisible(true) @@ - CoroutineScope(Dispatchers.IO).launch { - // set up Ubuntu file system - SplitCompat.install(context) - val imageFsInstallSuccess = - ImageFsInstaller.installIfNeededFuture(context, context.assets) { progress -> - // Log.d("XServerScreen", "$progress") - setLoadingProgress(progress / 100f) - }.get() - setLoadingProgress(-1f) + CoroutineScope(Dispatchers.IO).launch { + try { + // set up Ubuntu file system + SplitCompat.install(context) + ImageFsInstaller.installIfNeededFuture(context, context.assets) { progress -> + setLoadingProgress(progress / 100f) + }.get() + setLoadingProgress(-1f) @@ - setLoadingDialogVisible(false) + } catch (e: Exception) { + Timber.e(e, "preLaunchApp failed for ${libraryItem.appId}") + setMessageDialogState( + MessageDialogState( + visible = true, + type = DialogType.SYNC_FAIL, + title = context.getString(R.string.sync_error_title), + message = e.localizedMessage ?: "Unknown error", + dismissBtnText = context.getString(R.string.ok), + ), + ) + } finally { + setLoadingDialogVisible(false) + }Also applies to: 867-883, 896-898
| alias(libs.plugins.jetbrains.serialization) | ||
| alias(libs.plugins.kotlinter) | ||
| alias(libs.plugins.ksp) | ||
| id("com.chaquo.python") version "15.0.1" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Chaquopy 15.0.1 compatibility with your AGP version.
Ensure the plugin aligns with the Android Gradle Plugin declared in libs.versions.toml to avoid sync/runtime issues.
🏁 Script executed:
#!/bin/bash
# Show AGP version and Chaquopy plugin line.
rg -nP 'com.android.tools.build:gradle|com\.chaquo\.python' -C2
# Optionally print libs.versions.toml AGP entry.
fd libs.versions.toml | xargs -I{} rg -n 'agp|android-gradle' {}Length of output: 1317
Align Chaquopy plugin with the AGP pinned in libs.versions.toml (app/build.gradle.kts:12 — com.chaquo.python 15.0.1 vs agp = "8.8.0")
Chaquopy 15.0.1 only added support for AGP 8.1 and 8.2; upgrade Chaquopy to a release that supports newer AGP (e.g., 16.1+) or pin/downgrade AGP to <=8.2 to avoid sync/build failures. (chaquo.com)
Locations: app/build.gradle.kts:12; libs.versions.toml (agp = "8.8.0").
🤖 Prompt for AI Agents
In app/build.gradle.kts around line 12 (id("com.chaquo.python") version
"15.0.1") and libs.versions.toml where agp = "8.8.0", the Chaquopy plugin
version is incompatible with the pinned AGP; either upgrade the Chaquopy plugin
to a release that supports AGP 8.8+ (for example bump to a 16.x+ Chaquopy
release) or alternatively downgrade/pin the Android Gradle Plugin in
libs.versions.toml to <=8.2; update the version string in app/build.gradle.kts
(or change agp in libs.versions.toml) and run a Gradle sync/build to verify
compatibility.
| ) { | ||
| val clientIconUrl: String | ||
| get() = Constants.Library.ICON_URL + "$appId/$iconHash.ico" | ||
| get() = GameManagerService.getIconImage(this) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify GameManagerService.getIconImage() handles all LibraryItem types correctly
The delegation to GameManagerService.getIconImage(this) should properly handle both Steam and GOG sources.
🏁 Script executed:
#!/bin/bash
# Check GameManagerService.getIconImage implementation
echo "Checking GameManagerService.getIconImage method..."
ast-grep --pattern $'fun getIconImage($param): $_ {
$$$
}'
echo -e "\nChecking how different game sources are handled in icon retrieval..."
rg -n -A5 -B5 "getIconImage" app/src/main/java/app/gamenative/service/GameManagerService.ktLength of output: 667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate GameManagerService.kt files..."
rg -n --hidden -S "GameManagerService.kt" || true
echo
echo "Show getManagerForGame definition & nearby context in service files..."
rg -n -A10 -B10 "fun getManagerForGame" app/src -S || true
echo
echo "Show all implementations of `fun getIconImage(...)` in the repo (with context)..."
rg -n -C3 "fun getIconImage" app/src -S || true
echo
echo "Search for manager classes (Steam/GOG) and any Manager interface/abstract class..."
rg -n -i -S "class .*steam|class .*gog|object .*steam|object .*gog" app/src || true
rg -n -S "interface .*Manager|abstract class .*Manager" app/src || true
echo
echo "List usages of getManagerForGame(...)"
rg -n -S "getManagerForGame" app/src || trueLength of output: 14635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==== LibraryItem.kt ===="
sed -n '1,240p' app/src/main/java/app/gamenative/data/LibraryItem.kt || true
echo
echo "==== GOGGameManager.kt (start..520) ===="
sed -n '1,520p' app/src/main/java/app/gamenative/service/GOG/GOGGameManager.kt || true
echo
echo "==== SteamGameManager.kt (start..320) ===="
sed -n '1,320p' app/src/main/java/app/gamenative/service/Steam/SteamGameManager.kt || true
echo
echo "==== GOGGame.kt ===="
sed -n '1,240p' app/src/main/java/app/gamenative/data/GOGGame.kt || true
echo
echo "==== GOGLibraryManager.kt ===="
sed -n '1,320p' app/src/main/java/app/gamenative/service/GOG/GOGLibraryManager.kt || true
echo
echo "==== Instances of iconHash ===="
rg -n --hidden -S "iconHash" || true
echo
echo "==== Instances of ICON_URL ===="
rg -n --hidden -S "ICON_URL" || true
echo
echo "==== Files containing ICON_URL (preview) ===="
rg -n --hidden -S "ICON_URL" --files | while read -r f; do
echo "---- $f ----"
sed -n '1,160p' "$f"
doneLength of output: 46363
Verify/fix GOG icon population (Steam implementation is fine)
GameManagerService delegates to per-manager getIconImage; Steam returns a full CDN URL but GOG currently returns libraryItem.iconHash while createLibraryItem sets that field to an empty string — this can produce an empty clientIconUrl for GOG items.
- app/src/main/java/app/gamenative/service/GOG/GOGGameManager.kt:418 — createLibraryItem sets
iconHash = ""; populate it (e.g.,iconHash = gogGame.iconUrl) or otherwise attach a usable icon value. - app/src/main/java/app/gamenative/service/GOG/GOGGameManager.kt:493 —
getIconImagereturnslibraryItem.iconHash; alternatively fetch and return the canonical GOGGame.iconUrl here (normalize protocol if it starts with "//"). - app/src/main/java/app/gamenative/service/Steam/SteamGameManager.kt:254 — Steam
getIconImageconstructs a full URL; no change needed.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/data/LibraryItem.kt around line 18 the
getter delegates to GameManagerService.getIconImage but GOG items end up with an
empty clientIconUrl because GOGGameManager.createLibraryItem sets iconHash = ""
instead of a usable value; fix by populating the LibraryItem.iconHash in
app/src/main/java/app/gamenative/service/GOG/GOGGameManager.kt (around line
~418) with the GOG game's canonical icon URL (e.g., iconHash = gogGame.iconUrl
or a normalized full URL), or alternately modify GOGGameManager.getIconImage
(around line ~493) to return the GOGGame.iconUrl directly and normalize
protocol-prefixed URLs (prepend "https:" if they start with "//") so the
consumer receives a full usable CDN URL.
| val gameId: Int | ||
| get() = appId.removePrefix("${gameSource.name}_").toInt() |
There was a problem hiding this comment.
Potential NumberFormatException in gameId getter
The gameId getter assumes the appId always contains a numeric suffix after the prefix removal. This could throw a NumberFormatException for malformed IDs.
Add proper error handling:
val gameId: Int
- get() = appId.removePrefix("${gameSource.name}_").toInt()
+ get() = try {
+ appId.removePrefix("${gameSource.name}_").toInt()
+ } catch (e: NumberFormatException) {
+ throw IllegalStateException("Invalid appId format: $appId for source $gameSource", e)
+ }📝 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: Int | |
| get() = appId.removePrefix("${gameSource.name}_").toInt() | |
| val gameId: Int | |
| get() = try { | |
| appId.removePrefix("${gameSource.name}_").toInt() | |
| } catch (e: NumberFormatException) { | |
| throw IllegalStateException("Invalid appId format: $appId for source $gameSource", e) | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/data/LibraryItem.kt around lines 24-25, the
gameId getter currently calls toInt() on the suffix and can throw
NumberFormatException for malformed appId values; update it to safely parse the
suffix (use toIntOrNull()) and handle failure: verify the expected prefix
exists, extract the suffix, attempt toIntOrNull(), and if null throw a clear
IllegalArgumentException (or return a defined fallback/null if you prefer) that
includes the offending appId and expected format so callers get actionable error
information.
| if (GOGService.isRunning && !isChangingConfigurations) { | ||
| Timber.i("Stopping GOG Service") | ||
| GOGService.stop() | ||
| } |
There was a problem hiding this comment.
Don’t stop GOGService unconditionally in onDestroy — may kill active downloads/sync
Mirror the onStop guard to avoid aborting work when the Activity is destroyed (e.g., swiped away).
Apply:
- if (GOGService.isRunning && !isChangingConfigurations) {
+ if (GOGService.isRunning &&
+ !isChangingConfigurations &&
+ !GOGService.hasActiveOperations()
+ ) {
Timber.i("Stopping GOG Service")
GOGService.stop()
}If available, also include && !GOGService.isLoginInProgress.
📝 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 (GOGService.isRunning && !isChangingConfigurations) { | |
| Timber.i("Stopping GOG Service") | |
| GOGService.stop() | |
| } | |
| if (GOGService.isRunning && | |
| !isChangingConfigurations && | |
| !GOGService.hasActiveOperations() | |
| ) { | |
| Timber.i("Stopping GOG Service") | |
| GOGService.stop() | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/MainActivity.kt around lines 228 to 231, the
onDestroy currently stops GOGService unconditionally when GOGService.isRunning
and not changing configurations, which can abort active downloads/sync; update
the guard to mirror the onStop logic by only calling GOGService.stop() when the
Activity is finishing (not merely destroyed) and when not changing
configurations, and also include a check for && !GOGService.isLoginInProgress so
ongoing login/download work isn't interrupted.
| fun populateDownloadService(context: Context) { | ||
| baseDataDirPath = context.dataDir.path | ||
| baseCacheDirPath = context.cacheDir.path | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing null/error handling in populateDownloadService.
The function directly accesses context.dataDir.path and context.cacheDir.path without checking if the context is valid or if these paths exist.
fun populateDownloadService(context: Context) {
+ if (baseDataDirPath.isNotEmpty() || baseCacheDirPath.isNotEmpty()) {
+ Timber.w("DownloadService already populated, skipping re-initialization")
+ return
+ }
baseDataDirPath = context.dataDir.path
baseCacheDirPath = context.cacheDir.path
+ Timber.i("DownloadService populated with data: $baseDataDirPath, cache: $baseCacheDirPath")
}📝 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 populateDownloadService(context: Context) { | |
| baseDataDirPath = context.dataDir.path | |
| baseCacheDirPath = context.cacheDir.path | |
| } | |
| fun populateDownloadService(context: Context) { | |
| if (baseDataDirPath.isNotEmpty() || baseCacheDirPath.isNotEmpty()) { | |
| Timber.w("DownloadService already populated, skipping re-initialization") | |
| return | |
| } | |
| baseDataDirPath = context.dataDir.path | |
| baseCacheDirPath = context.cacheDir.path | |
| Timber.i("DownloadService populated with data: $baseDataDirPath, cache: $baseCacheDirPath") | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/DownloadService.kt around lines 22
to 25, populateDownloadService currently reads context.dataDir.path and
context.cacheDir.path directly; change it to defensively validate and handle
errors by (1) ensure the passed Context is non-null (use
context.applicationContext if available), (2) wrap access to dataDir/cacheDir in
a try/catch to handle SecurityException/IO errors, (3) check that the returned
File paths are non-null and exist, creating directories if missing, and (4) on
any failure log the error and fall back to safe alternatives such as
context.filesDir.path and context.cacheDir.path or a configured default,
avoiding throwing and crashing the app.
| if self.game_id: | ||
| try: | ||
| import builtins | ||
| flag_name = f'GOGDL_CANCEL_{self.game_id}' | ||
| if hasattr(builtins, flag_name) and getattr(builtins, flag_name, False): | ||
| self.logger.info(f"Progress reporting cancelled for game {self.game_id}") | ||
| self.completed = True | ||
| break | ||
| except: | ||
| pass |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid bare except and silence; log narrowly
Catching all exceptions and pass hides real bugs.
- except:
- pass
+ except (ImportError, AttributeError) as e:
+ self.logger.debug(f"Cancel flag check failed: {e}")📝 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 self.game_id: | |
| try: | |
| import builtins | |
| flag_name = f'GOGDL_CANCEL_{self.game_id}' | |
| if hasattr(builtins, flag_name) and getattr(builtins, flag_name, False): | |
| self.logger.info(f"Progress reporting cancelled for game {self.game_id}") | |
| self.completed = True | |
| break | |
| except: | |
| pass | |
| if self.game_id: | |
| try: | |
| import builtins | |
| flag_name = f'GOGDL_CANCEL_{self.game_id}' | |
| if hasattr(builtins, flag_name) and getattr(builtins, flag_name, False): | |
| self.logger.info(f"Progress reporting cancelled for game {self.game_id}") | |
| self.completed = True | |
| break | |
| except (ImportError, AttributeError) as e: | |
| self.logger.debug(f"Cancel flag check failed: {e}") |
🧰 Tools
🪛 Ruff (0.12.2)
42-42: Do not use bare except
(E722)
42-43: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/progressbar.py around lines 34 to 43, the code
uses a bare except which hides real errors; narrow the exception handling to
only the expected exceptions (e.g., ImportError, AttributeError) and log
failures instead of silently passing — capture the exception and call
self.logger.exception or self.logger.error(..., exc_info=True) so unexpected
issues are visible while preserving the intended cancel-check behavior.
| def print_progressbar(self): | ||
| percentage = (self.written_total / self.total) * 100 | ||
| running_time = time() - self.started_at | ||
| runtime_h = int(running_time // 3600) | ||
| runtime_m = int((running_time % 3600) // 60) | ||
| runtime_s = int((running_time % 3600) % 60) | ||
|
|
||
| print_time_delta = time() - self.last_update |
There was a problem hiding this comment.
Guard division by zero in percentage computation
total can be 0; current code will raise ZeroDivisionError.
def print_progressbar(self):
- percentage = (self.written_total / self.total) * 100
+ percentage = (self.written_total / self.total) * 100 if self.total else 0.0
running_time = time() - self.started_at📝 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.
| def print_progressbar(self): | |
| percentage = (self.written_total / self.total) * 100 | |
| running_time = time() - self.started_at | |
| runtime_h = int(running_time // 3600) | |
| runtime_m = int((running_time % 3600) // 60) | |
| runtime_s = int((running_time % 3600) % 60) | |
| print_time_delta = time() - self.last_update | |
| def print_progressbar(self): | |
| percentage = (self.written_total / self.total) * 100 if self.total else 0.0 | |
| running_time = time() - self.started_at | |
| runtime_h = int(running_time // 3600) | |
| runtime_m = int((running_time % 3600) // 60) | |
| runtime_s = int((running_time % 3600) % 60) | |
| print_time_delta = time() - self.last_update |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/progressbar.py around lines 64 to 71, the
percentage calculation divides by self.total which can be zero and raise
ZeroDivisionError; guard this by checking if self.total and greater than zero
before dividing, otherwise set percentage to 0.0 (or an appropriate default),
then proceed with computing running_time and other values; update the code so
the division only happens when safe.
| def __eq__(self, value: object) -> bool: | ||
| # Compare the class by language code | ||
| if isinstance(value, Language): | ||
| return self.code == value.code | ||
| # If comparing to string, look for the code, name and deprecated code | ||
| if type(value) is str: | ||
| return ( | ||
| value == self.code | ||
| or value.lower() == self.name.lower() | ||
| or value in self.deprecated_codes | ||
| ) | ||
| return NotImplemented |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize string comparisons; include native_name; prefer isinstance.
Current equality misses common inputs like "en-us", "Deutsch", or codes with underscores. Also uses type(value) is str. Normalize and broaden matching.
def __eq__(self, value: object) -> bool:
# Compare the class by language code
if isinstance(value, Language):
return self.code == value.code
# If comparing to string, look for the code, name and deprecated code
- if type(value) is str:
- return (
- value == self.code
- or value.lower() == self.name.lower()
- or value in self.deprecated_codes
- )
+ if isinstance(value, str):
+ v = value.strip()
+ v_norm = v.replace("_", "-").casefold()
+ code_norm = self.code.replace("_", "-").casefold()
+ deprecated_norm = {c.replace("_", "-").casefold() for c in self.deprecated_codes}
+ return (
+ v_norm == code_norm
+ or v.casefold() == self.name.casefold()
+ or v.casefold() == self.native_name.casefold()
+ or v_norm in deprecated_norm
+ )
return NotImplemented📝 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.
| def __eq__(self, value: object) -> bool: | |
| # Compare the class by language code | |
| if isinstance(value, Language): | |
| return self.code == value.code | |
| # If comparing to string, look for the code, name and deprecated code | |
| if type(value) is str: | |
| return ( | |
| value == self.code | |
| or value.lower() == self.name.lower() | |
| or value in self.deprecated_codes | |
| ) | |
| return NotImplemented | |
| def __eq__(self, value: object) -> bool: | |
| # Compare the class by language code | |
| if isinstance(value, Language): | |
| return self.code == value.code | |
| # If comparing to string, look for the code, name and deprecated code | |
| if isinstance(value, str): | |
| v = value.strip() | |
| v_norm = v.replace("_", "-").casefold() | |
| code_norm = self.code.replace("_", "-").casefold() | |
| deprecated_norm = {c.replace("_", "-").casefold() for c in self.deprecated_codes} | |
| return ( | |
| v_norm == code_norm | |
| or v.casefold() == self.name.casefold() | |
| or v.casefold() == self.native_name.casefold() | |
| or v_norm in deprecated_norm | |
| ) | |
| return NotImplemented |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/languages.py around lines 11 to 22, the __eq__ uses
strict type checks and misses common string variants (e.g. "en-us", "Deutsch",
codes with underscores) and doesn't consider native_name; update it to use
isinstance(value, Language) and isinstance(value, str), and normalize string
comparisons by trimming and lowercasing the input, replacing underscores with
hyphens, and doing the same normalization for self.code, self.name,
self.native_name and each deprecated code; then compare against normalized code,
name, native_name, and normalized deprecated codes so variants like "en_us",
"EN-US", or native names match; return NotImplemented if value is neither a
Language nor a str.
| try: | ||
| with open(stat_filename, encoding='utf-8') as stat_file: | ||
| _stat = stat_file.readline() | ||
| except (ProcessLookupError, FileNotFoundError): | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle PermissionError when reading /proc//stat.
On Android/Linux this is common and currently returns None only for NotFound. Add PermissionError to avoid exceptions bubbling elsewhere.
- except (ProcessLookupError, FileNotFoundError):
+ except (ProcessLookupError, FileNotFoundError, PermissionError):
return None📝 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.
| try: | |
| with open(stat_filename, encoding='utf-8') as stat_file: | |
| _stat = stat_file.readline() | |
| except (ProcessLookupError, FileNotFoundError): | |
| return None | |
| try: | |
| with open(stat_filename, encoding='utf-8') as stat_file: | |
| _stat = stat_file.readline() | |
| except (ProcessLookupError, FileNotFoundError, PermissionError): | |
| return None |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/process.py around lines 37 to 41, the file open for
/proc/<pid>/stat currently catches ProcessLookupError and FileNotFoundError but
not PermissionError; update the except clause to also catch PermissionError
(e.g., except (ProcessLookupError, FileNotFoundError, PermissionError):) so
attempts to read stat that are blocked by permissions return None instead of
raising.
| def cwd(self): | ||
| """Return current working dir of process""" | ||
| cwd_path = "/proc/%d/cwd" % int(self.pid) | ||
| return os.readlink(cwd_path) | ||
|
|
There was a problem hiding this comment.
cwd property can raise and crash; guard readlink.
Access to /proc/<pid>/cwd may be denied or disappear. Wrap with try/except and return empty string or None.
def cwd(self):
"""Return current working dir of process"""
cwd_path = "/proc/%d/cwd" % int(self.pid)
- return os.readlink(cwd_path)
+ try:
+ return os.readlink(cwd_path)
+ except (FileNotFoundError, ProcessLookupError, PermissionError):
+ return ""📝 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.
| def cwd(self): | |
| """Return current working dir of process""" | |
| cwd_path = "/proc/%d/cwd" % int(self.pid) | |
| return os.readlink(cwd_path) | |
| def cwd(self): | |
| """Return current working dir of process""" | |
| cwd_path = "/proc/%d/cwd" % int(self.pid) | |
| try: | |
| return os.readlink(cwd_path) | |
| except (FileNotFoundError, ProcessLookupError, PermissionError): | |
| return "" |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/process.py around lines 96 to 100, the cwd method
directly calls os.readlink("/proc/%d/cwd" % int(self.pid)) which can raise if
the /proc entry disappears or is inaccessible; wrap the readlink call in a
try/except catching OSError (or more specific FileNotFoundError/PermissionError)
and return a safe sentinel (e.g., None or empty string) on failure, logging or
suppressing the exception as appropriate so the process won't crash.
| @Transaction | ||
| suspend fun upsertPreservingInstallStatus(games: List<GOGGame>) { | ||
| games.forEach { newGame -> | ||
| val existingGame = getById(newGame.id) | ||
| if (existingGame != null) { | ||
| // Preserve installation status and path from existing game | ||
| val gameToInsert = newGame.copy( | ||
| isInstalled = existingGame.isInstalled, | ||
| installPath = existingGame.installPath, | ||
| ) | ||
| insert(gameToInsert) | ||
| } else { | ||
| // New game, insert as-is | ||
| insert(newGame) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid N+1 queries in upsertPreservingInstallStatus. Batch fetch install status first.
Calling getById per game doesn’t scale with large libraries. Do a single SELECT for all ids, then merge.
interface GOGGameDao {
+ data class InstallState(val id: String, val isInstalled: Boolean, val installPath: String)
+
+ @Query("SELECT id, isInstalled, installPath FROM gog_games WHERE id IN (:ids)")
+ suspend fun getInstallStates(ids: List<String>): List<InstallState>
@@
@Transaction
suspend fun upsertPreservingInstallStatus(games: List<GOGGame>) {
- games.forEach { newGame ->
- val existingGame = getById(newGame.id)
- if (existingGame != null) {
- // Preserve installation status and path from existing game
- val gameToInsert = newGame.copy(
- isInstalled = existingGame.isInstalled,
- installPath = existingGame.installPath,
- )
- insert(gameToInsert)
- } else {
- // New game, insert as-is
- insert(newGame)
- }
- }
+ val map = getInstallStates(games.map { it.id }).associateBy({ it.id }) { it }
+ insertAll(
+ games.map { g ->
+ map[g.id]?.let { s -> g.copy(isInstalled = s.isInstalled, installPath = s.installPath) } ?: g
+ }
+ )
}Committable suggestion skipped: line range outside the PR's diff.
| suspend fun startBackgroundSync(context: Context, clearExisting: Boolean = false): Result<Unit> { | ||
| if (backgroundSyncInProgress) { | ||
| Timber.i("Background GOG sync already in progress, skipping") | ||
| return Result.failure(Exception("Background sync already in progress")) | ||
| } | ||
|
|
||
| // Validate credentials before starting background sync | ||
| return try { | ||
| if (!GOGService.hasStoredCredentials(context)) { | ||
| Timber.w("No GOG credentials found, cannot start background sync") | ||
| return Result.failure(Exception("No GOG credentials found. Please log in first.")) | ||
| } | ||
|
|
||
| val validationResult = GOGService.validateCredentials(context) | ||
| if (validationResult.isFailure || !validationResult.getOrThrow()) { | ||
| Timber.w("GOG credentials validation failed, cannot start background sync") | ||
| return Result.failure(Exception("GOG credentials validation failed. Please log in again.")) | ||
| } | ||
|
|
||
| scope.launch { | ||
| backgroundSyncInProgress = true | ||
| syncLibraryInBackground(context, clearExisting) | ||
| backgroundSyncInProgress = false | ||
| } | ||
|
|
||
| Result.success(Unit) | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Failed to start background sync") | ||
| Result.failure(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix race: backgroundSyncInProgress set too late.
Two callers can pass the check and both launch sync. Set the flag before launching and clear it in finally.
- if (backgroundSyncInProgress) {
+ if (backgroundSyncInProgress) {
Timber.i("Background GOG sync already in progress, skipping")
return Result.failure(Exception("Background sync already in progress"))
}
...
- scope.launch {
- backgroundSyncInProgress = true
- syncLibraryInBackground(context, clearExisting)
- backgroundSyncInProgress = false
- }
+ backgroundSyncInProgress = true
+ scope.launch {
+ try {
+ syncLibraryInBackground(context, clearExisting)
+ } finally {
+ backgroundSyncInProgress = false
+ }
+ }📝 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.
| suspend fun startBackgroundSync(context: Context, clearExisting: Boolean = false): Result<Unit> { | |
| if (backgroundSyncInProgress) { | |
| Timber.i("Background GOG sync already in progress, skipping") | |
| return Result.failure(Exception("Background sync already in progress")) | |
| } | |
| // Validate credentials before starting background sync | |
| return try { | |
| if (!GOGService.hasStoredCredentials(context)) { | |
| Timber.w("No GOG credentials found, cannot start background sync") | |
| return Result.failure(Exception("No GOG credentials found. Please log in first.")) | |
| } | |
| val validationResult = GOGService.validateCredentials(context) | |
| if (validationResult.isFailure || !validationResult.getOrThrow()) { | |
| Timber.w("GOG credentials validation failed, cannot start background sync") | |
| return Result.failure(Exception("GOG credentials validation failed. Please log in again.")) | |
| } | |
| scope.launch { | |
| backgroundSyncInProgress = true | |
| syncLibraryInBackground(context, clearExisting) | |
| backgroundSyncInProgress = false | |
| } | |
| Result.success(Unit) | |
| } catch (e: Exception) { | |
| Timber.e(e, "Failed to start background sync") | |
| Result.failure(e) | |
| } | |
| } | |
| suspend fun startBackgroundSync(context: Context, clearExisting: Boolean = false): Result<Unit> { | |
| if (backgroundSyncInProgress) { | |
| Timber.i("Background GOG sync already in progress, skipping") | |
| return Result.failure(Exception("Background sync already in progress")) | |
| } | |
| // Validate credentials before starting background sync | |
| return try { | |
| if (!GOGService.hasStoredCredentials(context)) { | |
| Timber.w("No GOG credentials found, cannot start background sync") | |
| return Result.failure(Exception("No GOG credentials found. Please log in first.")) | |
| } | |
| val validationResult = GOGService.validateCredentials(context) | |
| if (validationResult.isFailure || !validationResult.getOrThrow()) { | |
| Timber.w("GOG credentials validation failed, cannot start background sync") | |
| return Result.failure(Exception("GOG credentials validation failed. Please log in again.")) | |
| } | |
| backgroundSyncInProgress = true | |
| scope.launch { | |
| try { | |
| syncLibraryInBackground(context, clearExisting) | |
| } finally { | |
| backgroundSyncInProgress = false | |
| } | |
| } | |
| Result.success(Unit) | |
| } catch (e: Exception) { | |
| Timber.e(e, "Failed to start background sync") | |
| Result.failure(e) | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GOG/GOGLibraryManager.kt around
lines 22 to 52, the backgroundSyncInProgress flag is only set inside the
launched coroutine, so two callers can pass the initial check and both start
sync; set backgroundSyncInProgress = true immediately before launching the
coroutine to prevent races, then run the sync call inside a try/finally (inside
the coroutine) that always sets backgroundSyncInProgress = false in the finally
block; also ensure that if scope.launch itself throws, you clear the flag (or
wrap the launch in try/catch to reset the flag) so the flag is always cleared on
both success and failure.
| // Clear existing games if requested | ||
| if (clearExisting) { | ||
| Timber.i("Clearing existing GOG games before sync") | ||
| clearLibrary() | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check and log clearLibrary result.
Currently ignored; log or abort if it fails.
- if (clearExisting) {
- Timber.i("Clearing existing GOG games before sync")
- clearLibrary()
- }
+ if (clearExisting) {
+ Timber.i("Clearing existing GOG games before sync")
+ val cleared = clearLibrary()
+ if (cleared.isFailure) {
+ Timber.w("Failed to clear existing GOG games: ${cleared.exceptionOrNull()?.message}")
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GOG/GOGLibraryManager.kt around
lines 79 to 84, the call to clearLibrary() is ignored; update the call to
capture its result, log whether it succeeded or failed with Timber (include any
error message/exception), and abort the sync flow when it fails (e.g., return or
throw a specific exception) so callers can handle the failure. Ensure the
logging gives clear context like "Failed to clear GOG library" and include
details from the returned error/exception.
| @@ -0,0 +1,111 @@ | |||
| package app.gamenative.service | |||
There was a problem hiding this comment.
Package declaration doesn't match the summary.
The package is declared as app.gamenative.service but the summary mentions it's in app/src/main/java/app/gamenative/ui/internal/. This inconsistency could cause confusion.
Update the package declaration to match the file location:
-package app.gamenative.service
+package app.gamenative.ui.internal📝 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.
| package app.gamenative.service | |
| package app.gamenative.ui.internal |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/ui/internal/FakeGameManager.kt at lines 1-1
the file's package declaration is incorrect (currently app.gamenative.service)
and should match its filesystem path; change the package declaration to
app.gamenative.ui.internal (and verify the file is placed under
src/main/java/app/gamenative/ui/internal) so declarations and location are
consistent.
| items(items = state.appInfoList, key = { it.index }) { item -> | ||
| AppItem( | ||
| modifier = Modifier.animateItem(), | ||
| appInfo = item, | ||
| onClick = { onNavigate(item.appId) } | ||
| onClick = { onNavigate(item) }, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a stable item key (appId) instead of index to avoid recomposition glitches.
Index-based keys break when the list is filtered/paged/reordered and can cause wrong-item state reuse and animation artifacts. Prefer a unique, stable identifier.
- items(items = state.appInfoList, key = { it.index }) { item ->
+ items(items = state.appInfoList, key = { it.appId }) { item ->📝 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.
| items(items = state.appInfoList, key = { it.index }) { item -> | |
| AppItem( | |
| modifier = Modifier.animateItem(), | |
| appInfo = item, | |
| onClick = { onNavigate(item.appId) } | |
| onClick = { onNavigate(item) }, | |
| ) | |
| items(items = state.appInfoList, key = { it.appId }) { item -> | |
| AppItem( | |
| modifier = Modifier.animateItem(), | |
| appInfo = item, | |
| onClick = { onNavigate(item) }, | |
| ) |
🤖 Prompt for AI Agents
In
app/src/main/java/app/gamenative/ui/screen/library/components/LibraryListPane.kt
around lines 192 to 197, the items(...) call currently uses the list index as
the Compose key which can cause wrong-item state reuse when the list is filtered
or reordered; replace the key = { it.index } with a stable unique identifier
such as key = { it.appId } (or item.appId) to ensure stable keys across list
changes, and ensure appId is non-null/unique for every item (or derive a stable
string/UUID) so Compose can correctly preserve item state and animations.
| client_id = "46899977096215655" | ||
| client_secret = "9d85c43b1482497dbbce61f6e4aa173a433796eeae2ca8c5f6129f2dc4de46d9" | ||
| return client_id, client_secret |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded credentials should be moved to configuration.
The client ID and secret are hardcoded in the source code. While these appear to be public GOG client credentials, it would be better to move them to a configuration file or environment variables for better security practices and maintainability.
Would you like me to help refactor this to use a configuration-based approach for storing these credentials?
🧰 Tools
🪛 Ruff (0.12.2)
204-204: Possible hardcoded password assigned to: "client_secret"
(S105)
205-205: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/saves.py around lines 203-205 the client_id and
client_secret are hardcoded; replace them with configuration-backed values by
reading from environment variables or a configuration file (e.g.,
os.environ["GOG_CLIENT_ID"]/["GOG_CLIENT_SECRET"] or a config loader), provide
sensible failure behavior (raise a clear exception or log an error if missing),
remove the literal strings, and update any tests/docs to reference the new
config keys and how to set them.
| buffer = context.data_sec.read(take) | ||
| assert len(buffer) == take | ||
| context.target_buffer.extend(buffer) | ||
| halfinst.type = objects.XD3_NOOP | ||
| else: # XD3_CPY and higher |
There was a problem hiding this comment.
Avoid asserts for runtime checks and prevent potential infinite loop.
Asserts can be stripped with -O. Also, if source.read() returns 0 bytes, left never decreases.
- buffer = context.data_sec.read(take)
- assert len(buffer) == take
+ buffer = context.data_sec.read(take)
+ if len(buffer) != take:
+ raise EOFError("Unexpected end of ADD data")
@@
- while left > 0:
- buffer = context.source.read(min(1024 * 1024, left))
- size = len(buffer)
+ while left > 0:
+ buffer = context.source.read(min(1024 * 1024, left))
+ size = len(buffer)
+ if size == 0:
+ raise EOFError("Unexpected end of source during CPY")Also applies to: 68-73
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/xdelta/patcher.py around lines 59-63 (and similarly
fix lines 68-73), the code uses assert to validate that the number of bytes read
equals the requested amount which is unsafe at runtime and can be stripped with
-O, and it can cause an infinite loop if read() returns zero bytes; replace the
assert with a runtime check that raises a descriptive exception (e.g., EOFError
or IOError) when len(buffer) != take, and additionally detect zero-length reads
in loops and either raise the same exception or break so that left decreases or
the caller can handle the truncated input instead of looping forever.
| else: | ||
| print("OVERLAP NOT IMPLEMENTED") | ||
| raise Exception("OVERLAP") | ||
| halfinst.type = objects.XD3_NOOP |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Raise a specific exception or implement overlap copying.
Generic Exception and unimplemented path make patching brittle.
Replace with raise NotImplementedError("XD3 overlap copy not implemented") and consider implementing overlap logic if needed for GOG patches.
- print("OVERLAP NOT IMPLEMENTED")
- raise Exception("OVERLAP")
+ raise NotImplementedError("XD3 overlap copy not implemented")📝 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.
| else: | |
| print("OVERLAP NOT IMPLEMENTED") | |
| raise Exception("OVERLAP") | |
| halfinst.type = objects.XD3_NOOP | |
| else: | |
| raise NotImplementedError("XD3 overlap copy not implemented") | |
| halfinst.type = objects.XD3_NOOP |
🧰 Tools
🪛 Ruff (0.12.2)
76-76: Create your own exception
(TRY002)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/xdelta/patcher.py around lines 74 to 77, the code
prints a message and raises a generic Exception for the overlap branch which
makes error handling brittle and ambiguous; replace the generic Exception with a
specific NotImplementedError containing a clear message like "XD3 overlap copy
not implemented" (or implement the overlap copy logic if required for GOG
patches), and ensure any callers can catch this specific exception type for
clearer error handling.
| def patch(source: str, patch: str, out: str, speed_queue: Queue): | ||
| src_handle = open(source, 'rb') | ||
| patch_handle = open(patch, 'rb') | ||
| dst_handle = open(out, 'wb') | ||
|
|
||
|
|
||
| # Verify if patch is actually xdelta patch | ||
| headers = patch_handle.read(5) | ||
| try: | ||
| assert headers[0] == 0xD6 | ||
| assert headers[1] == 0xC3 | ||
| assert headers[2] == 0xC4 | ||
| except AssertionError: | ||
| print("Specified patch file is unlikely to be xdelta patch") | ||
| return | ||
|
|
There was a problem hiding this comment.
Close files reliably and avoid early-return leaks.
Use context managers; current early returns (invalid patch, unsupported flags) leak file descriptors.
-def patch(source: str, patch: str, out: str, speed_queue: Queue):
- src_handle = open(source, 'rb')
- patch_handle = open(patch, 'rb')
- dst_handle = open(out, 'wb')
+def patch(source: str, patch: str, out: str, speed_queue: Queue):
+ with open(source, 'rb') as src_handle, \
+ open(patch, 'rb') as patch_handle, \
+ open(out, 'wb') as dst_handle:
@@
- except AssertionError:
- print("Specified patch file is unlikely to be xdelta patch")
- return
+ except AssertionError:
+ print("Specified patch file is unlikely to be xdelta patch")
+ return
@@
- dst_handle.flush()
- src_handle.close()
- patch_handle.close()
- dst_handle.close()
+ dst_handle.flush()Also applies to: 199-203
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/xdelta/patcher.py around lines 80-95 (and also fix
the similar leak at 199-203), the function opens source, patch, and out files
and returns early on invalid headers/flags which leaks file descriptors; change
the implementation to open all files using context managers (with statements) or
ensure a try/finally that closes all handles, and move header validation and
flag checks inside those contexts so any early return still causes files to be
closed automatically; apply the same pattern to the similar block at lines
199-203.
| win_number = 0 | ||
| win_indicator = patch_handle.read(1)[0] | ||
| while win_indicator is not None: | ||
| context.acache = objects.AddressCache() | ||
| source_used = win_indicator & (1 << 0) != 0 | ||
| target_used = win_indicator & (1 << 1) != 0 | ||
| adler32_sum = win_indicator & (1 << 2) != 0 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Defensive reads: guard against EOF when reading single bytes.
read(1)[0] will raise IndexError on truncated input. Handle empty reads.
- win_indicator = patch_handle.read(1)[0]
- while win_indicator is not None:
+ b = patch_handle.read(1)
+ win_indicator = b[0] if b else None
+ while win_indicator is not None:
@@
- indicator = patch_handle.read(1)
- if not len(indicator):
+ indicator = patch_handle.read(1)
+ if not indicator:
win_indicator = None
continue
win_indicator = indicator[0]Also applies to: 191-196
🧰 Tools
🪛 Ruff (0.12.2)
117-117: Local variable target_used is assigned to but never used
Remove assignment to unused variable target_used
(F841)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/xdelta/patcher.py around lines 112 to 119 (and
similarly at 191-196), the code uses patch_handle.read(1)[0] which will raise
IndexError on EOF/truncated input; change to read a single byte into a variable,
check if the returned bytes object is empty (or shorter than expected) and
handle it (either break the loop or raise a clear EOF/truncated-patch exception)
before indexing [0], so reads are defensive and do not crash on truncated input.
| // Init our datastore preferences. | ||
| PrefManager.init(this) | ||
|
|
||
| DownloadService.populateDownloadService(this) |
There was a problem hiding this comment.
Wrap DownloadService.populateDownloadService in try/catch (startup crash risk)
Filesystem/init work here can throw and currently isn’t guarded, unlike the other init blocks.
Apply:
- DownloadService.populateDownloadService(this)
+ try {
+ DownloadService.populateDownloadService(this)
+ Timber.i("[PluviaApp]: DownloadService populated")
+ } catch (e: Exception) {
+ Timber.e(e, "[PluviaApp]: Failed to populate DownloadService")
+ }📝 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.
| DownloadService.populateDownloadService(this) | |
| try { | |
| DownloadService.populateDownloadService(this) | |
| Timber.i("[PluviaApp]: DownloadService populated") | |
| } catch (e: Exception) { | |
| Timber.e(e, "[PluviaApp]: Failed to populate DownloadService") | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/PluviaApp.kt around line 65, the call to
DownloadService.populateDownloadService(this) is unguarded and can throw during
filesystem/init work; wrap that call in a try/catch that catches Exception (or
Throwable if you must be defensive), log the error with context (including the
exception) using the app's logger (or Android Log/TAG), and continue startup
without rethrowing so the app doesn't crash on startup.
| override fun getAppInfo(libraryItem: LibraryItem): SteamApp? { | ||
| val gogGame = runBlocking { getGameById(libraryItem.gameId.toString()) } | ||
| return if (gogGame != null) { | ||
| convertGOGGameToSteamApp(gogGame) | ||
| } else { | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid blocking calls in non-suspend functions.
The getAppInfo method uses runBlocking which blocks the current thread. Consider making this method suspend or caching the results.
This is a recurring pattern in the codebase where runBlocking is used in non-suspend functions. Consider refactoring the GameManager interface to make these methods suspend where appropriate, or implement a caching layer to minimize blocking database calls.
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/GOG/GOGGameManager.kt around lines
463 to 470, replace the blocking runBlocking call inside getAppInfo: either
change getAppInfo to a suspend function in the GameManager interface and
implementations and call getGameById directly (remove runBlocking), or implement
a non-blocking cache that stores recent game lookups and read from the cache
here without blocking; ensure all call sites and interface definitions are
updated to the suspend signature if you choose the suspend route, or
populate/evict the cache appropriately if you choose the caching approach.
| PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against null userSteamId.
The non-null assertion can crash. Fail fast with a clear message or fallback.
- val prefixToPath: (String) -> String = { prefix ->
- PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID)
- }
+ val accountId = SteamService.userSteamId?.accountID
+ ?: return@withContext PostSyncInfo(app.gamenative.enums.SyncResult.UnknownFail).also {
+ Timber.w("Cannot launch: Steam user not available")
+ }
+ 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.
| PathType.from(prefix).toAbsPath(context, gameId, SteamService.userSteamId!!.accountID) | |
| } | |
| val accountId = SteamService.userSteamId?.accountID | |
| ?: return@withContext PostSyncInfo(app.gamenative.enums.SyncResult.UnknownFail).also { | |
| Timber.w("Cannot launch: Steam user not available") | |
| } | |
| val prefixToPath: (String) -> String = { prefix -> | |
| PathType.from(prefix).toAbsPath(context, gameId, accountId) | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/Steam/SteamGameManager.kt around
lines 119 to 121, guard against SteamService.userSteamId being null before
dereferencing with "!!": check if userSteamId is null and either fail fast by
throwing a clear IllegalStateException (including context like gameId and that
the user steam id is missing) or provide a documented fallback value/behavior,
then use the non-null local variable for accountID when calling
PathType.from(...).toAbsPath(...).
| if (container.isLaunchRealSteam()) { | ||
| // Launch Steam with the applaunch parameter to start the game | ||
| return "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " + | ||
| "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $appId" | ||
| } |
There was a problem hiding this comment.
Real Steam launch uses container appId; should use numeric gameId.
-applaunch expects the Steam AppID (integer), not the container ID.
- return "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " +
- "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $appId"
+ val numericId = ContainerUtils.extractGameIdFromContainerId(appId)
+ return "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " +
+ "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $numericId"📝 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 (container.isLaunchRealSteam()) { | |
| // Launch Steam with the applaunch parameter to start the game | |
| return "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " + | |
| "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $appId" | |
| } | |
| if (container.isLaunchRealSteam()) { | |
| // Launch Steam with the applaunch parameter to start the game | |
| val numericId = ContainerUtils.extractGameIdFromContainerId(appId) | |
| return "\"C:\\\\Program Files (x86)\\\\Steam\\\\steam.exe\" -silent -vgui -tcp " + | |
| "-nobigpicture -nofriendsui -nochatui -nointro -applaunch $numericId" | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/app/gamenative/service/Steam/SteamGameManager.kt around
lines 176 to 180, the command string uses the container ID ($appId) for
-applaunch but Steam requires the numeric AppID; replace the container
identifier with the container's numeric Steam AppID (e.g.,
container.getSteamAppId() or container.getGameId()) ensuring it is an integer,
validate or throw/log if missing, and use that numeric value in the applaunch
parameter.
| def get_zlib_encoded(api_handler, url): | ||
| retries = 5 | ||
| while retries > 0: | ||
| try: | ||
| x = api_handler.session.get(url, timeout=TIMEOUT) | ||
| if not x.ok: | ||
| return None, None | ||
| try: | ||
| decompressed = json.loads(zlib.decompress(x.content, 15)) | ||
| except zlib.error: | ||
| return x.json(), x.headers | ||
| return decompressed, x.headers | ||
| except Exception: | ||
| time.sleep(2) | ||
| retries-=1 | ||
| return None, None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Narrow exception handling and avoid silent retries.
Catching bare Exception hides network/JSON errors. Use requests.RequestException and log the last error.
-def get_zlib_encoded(api_handler, url):
+def get_zlib_encoded(api_handler, url):
retries = 5
while retries > 0:
try:
x = api_handler.session.get(url, timeout=TIMEOUT)
if not x.ok:
return None, None
try:
decompressed = json.loads(zlib.decompress(x.content, 15))
except zlib.error:
return x.json(), x.headers
return decompressed, x.headers
- except Exception:
+ except requests.RequestException as e:
+ logging.getLogger("DL_UTILS").warning(f"GET failed ({url}): {e}")
time.sleep(2)
retries-=1
return None, None📝 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.
| def get_zlib_encoded(api_handler, url): | |
| retries = 5 | |
| while retries > 0: | |
| try: | |
| x = api_handler.session.get(url, timeout=TIMEOUT) | |
| if not x.ok: | |
| return None, None | |
| try: | |
| decompressed = json.loads(zlib.decompress(x.content, 15)) | |
| except zlib.error: | |
| return x.json(), x.headers | |
| return decompressed, x.headers | |
| except Exception: | |
| time.sleep(2) | |
| retries-=1 | |
| return None, None | |
| def get_zlib_encoded(api_handler, url): | |
| retries = 5 | |
| while retries > 0: | |
| try: | |
| x = api_handler.session.get(url, timeout=TIMEOUT) | |
| if not x.ok: | |
| return None, None | |
| try: | |
| decompressed = json.loads(zlib.decompress(x.content, 15)) | |
| except zlib.error: | |
| return x.json(), x.headers | |
| return decompressed, x.headers | |
| except requests.RequestException as e: | |
| logging.getLogger("DL_UTILS").warning(f"GET failed ({url}): {e}") | |
| time.sleep(2) | |
| retries-=1 | |
| return None, None |
🧰 Tools
🪛 Ruff (0.12.2)
39-39: Consider moving this statement to an else block
(TRY300)
40-40: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/dl_utils.py around lines 28-44, replace the bare
"except Exception" retry block with handling for requests.RequestException
(e.g., "except requests.RequestException as e:") and capture the last exception
in a local variable; also handle JSON decoding/zlib errors explicitly (catch
zlib.error and json.JSONDecodeError/ValueError where appropriate) so you don't
silently swallow parsing failures; ensure you log or attach the last caught
exception before returning None, None after retries are exhausted and add any
missing import for requests.
| def _write_manifest(self, installed: set): | ||
| repository = self.repository | ||
| repository['HGLInstalled'] = list(installed) | ||
| with open(self.installed_manifest, 'w') as f: | ||
| json.dump(repository, f) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve previously installed IDs to avoid unnecessary re-downloads.
Resetting HGLInstalled to only “this run” drops previously installed redists (e.g., when toggling download_game_deps_only), causing spurious work.
def _write_manifest(self, installed: set):
- repository = self.repository
- repository['HGLInstalled'] = list(installed)
- with open(self.installed_manifest, 'w') as f:
- json.dump(repository, f)
+ repository = dict(self.repository) if self.repository else {}
+ # Merge with previously recorded installations that still exist in repository
+ prev_installed: set[str] = set()
+ try:
+ if os.path.exists(self.installed_manifest):
+ with open(self.installed_manifest, 'r', encoding='utf-8') as f:
+ prev_installed = set((json.load(f) or {}).get('HGLInstalled', []))
+ except (OSError, json.JSONDecodeError):
+ prev_installed = set()
+ repo_ids = {d.get('dependencyId') for d in repository.get('depots', []) if d.get('dependencyId')}
+ merged = (installed | (prev_installed & repo_ids))
+ repository['HGLInstalled'] = sorted(merged)
+ with open(self.installed_manifest, 'w', encoding='utf-8') as f:
+ json.dump(repository, f)📝 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.
| def _write_manifest(self, installed: set): | |
| repository = self.repository | |
| repository['HGLInstalled'] = list(installed) | |
| with open(self.installed_manifest, 'w') as f: | |
| json.dump(repository, f) | |
| def _write_manifest(self, installed: set): | |
| repository = dict(self.repository) if self.repository else {} | |
| # Merge with previously recorded installations that still exist in repository | |
| prev_installed: set[str] = set() | |
| try: | |
| if os.path.exists(self.installed_manifest): | |
| with open(self.installed_manifest, 'r', encoding='utf-8') as f: | |
| prev_installed = set((json.load(f) or {}).get('HGLInstalled', [])) | |
| except (OSError, json.JSONDecodeError): | |
| prev_installed = set() | |
| repo_ids = {d.get('dependencyId') for d in repository.get('depots', []) if d.get('dependencyId')} | |
| merged = (installed | (prev_installed & repo_ids)) | |
| repository['HGLInstalled'] = sorted(merged) | |
| with open(self.installed_manifest, 'w', encoding='utf-8') as f: | |
| json.dump(repository, f) |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/managers/dependencies.py around lines 122-127,
the current implementation overwrites repository['HGLInstalled'] with only the
IDs from this run which drops previously installed IDs and causes unnecessary
re-downloads; change the write logic to read any existing HGLInstalled list from
repository (or an empty list if missing), take the union with the incoming
installed set, and write that combined list back to repository before dumping to
the installed_manifest file (ensuring you handle the case where the manifest
file/key may not exist and convert to a list for JSON serialization).
| if len(new_file.chunks) == 1 and len(old_file.chunks) == 1: | ||
| if new_file.chunks[0]["md5"] != old_file.chunks[0]["md5"]: | ||
| comparison.changed.append(new_file) | ||
| else: | ||
| if (new_file.md5 and old_file.md5 and new_file.md5 != old_file.md5) or (new_file.sha256 and old_file.sha256 != new_file.sha256): | ||
| comparison.changed.append(v2.FileDiff.compare(new_file, old_file)) | ||
| elif len(new_file.chunks) != len(old_file.chunks): | ||
| comparison.changed.append(v2.FileDiff.compare(new_file, old_file)) | ||
| return comparison |
There was a problem hiding this comment.
Fix sha256 comparison bug and consistently emit FileDiff for changes.
The condition mixes truthiness and comparison incorrectly; also prefer FileDiff for uniform downstream handling.
- else:
- if (new_file.md5 and old_file.md5 and new_file.md5 != old_file.md5) or (new_file.sha256 and old_file.sha256 != new_file.sha256):
- comparison.changed.append(v2.FileDiff.compare(new_file, old_file))
- elif len(new_file.chunks) != len(old_file.chunks):
- comparison.changed.append(v2.FileDiff.compare(new_file, old_file))
+ else:
+ if (
+ (new_file.md5 and old_file.md5 and new_file.md5 != old_file.md5) or
+ (new_file.sha256 and old_file.sha256 and new_file.sha256 != old_file.sha256) or
+ (len(new_file.chunks) != len(old_file.chunks))
+ ):
+ comparison.changed.append(v2.FileDiff.compare(new_file, old_file))📝 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 len(new_file.chunks) == 1 and len(old_file.chunks) == 1: | |
| if new_file.chunks[0]["md5"] != old_file.chunks[0]["md5"]: | |
| comparison.changed.append(new_file) | |
| else: | |
| if (new_file.md5 and old_file.md5 and new_file.md5 != old_file.md5) or (new_file.sha256 and old_file.sha256 != new_file.sha256): | |
| comparison.changed.append(v2.FileDiff.compare(new_file, old_file)) | |
| elif len(new_file.chunks) != len(old_file.chunks): | |
| comparison.changed.append(v2.FileDiff.compare(new_file, old_file)) | |
| return comparison | |
| if len(new_file.chunks) == 1 and len(old_file.chunks) == 1: | |
| if new_file.chunks[0]["md5"] != old_file.chunks[0]["md5"]: | |
| comparison.changed.append(new_file) | |
| else: | |
| if ( | |
| (new_file.md5 and old_file.md5 and new_file.md5 != old_file.md5) or | |
| (new_file.sha256 and old_file.sha256 and new_file.sha256 != old_file.sha256) or | |
| (len(new_file.chunks) != len(old_file.chunks)) | |
| ): | |
| comparison.changed.append(v2.FileDiff.compare(new_file, old_file)) | |
| return comparison |
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/managers/dependencies.py around lines 158-166,
the sha256 comparison uses mixed truthiness and comparison and single-chunk MD5
changes append the raw new_file instead of a FileDiff; change the logic so both
single-chunk and multi-chunk differences always append
v2.FileDiff.compare(new_file, old_file), and fix the sha256 check to require
both hashes exist before comparing (i.e., use "new_file.sha256 and
old_file.sha256 and old_file.sha256 != new_file.sha256").
| except Exception as e: | ||
| self.logger.error(f"Unable to resume download, continuing as normal {e}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use logging.exception for better error tracking.
When catching exceptions, use logging.exception to include the full traceback.
except Exception as e:
- self.logger.error(f"Unable to resume download, continuing as normal {e}")
+ self.logger.exception("Unable to resume download, continuing as normal")📝 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.
| except Exception as e: | |
| self.logger.error(f"Unable to resume download, continuing as normal {e}") | |
| except Exception as e: | |
| self.logger.exception("Unable to resume download, continuing as normal") |
🧰 Tools
🪛 Ruff (0.12.2)
176-176: Do not catch blind exception: Exception
(BLE001)
177-177: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/managers/task_executor.py around lines 176 to
178, the except block logs the error with logger.error and an f-string which
omits the traceback; replace the call with logger.exception (or
logger.error(..., exc_info=True)) so the full traceback is recorded, keeping the
descriptive message but using logger.exception to capture stack trace for better
debugging.
| shutil.rmtree(self.temp_dir) | ||
| except: | ||
| self.logger.warning("Failed to clean up temp directory") | ||
|
|
||
| try: | ||
| if os.path.exists(self.resume_file): | ||
| os.remove(self.resume_file) | ||
| except: | ||
| self.logger.error("Failed to remove resume file") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in cleanup.
The bare except clauses could hide important errors. At minimum, log what went wrong.
-try:
- shutil.rmtree(self.temp_dir)
-except:
- self.logger.warning("Failed to clean up temp directory")
+try:
+ shutil.rmtree(self.temp_dir)
+except Exception as e:
+ self.logger.warning(f"Failed to clean up temp directory {self.temp_dir}: {e}")
-try:
- if os.path.exists(self.resume_file):
- os.remove(self.resume_file)
-except:
- self.logger.error("Failed to remove resume file")
+try:
+ if os.path.exists(self.resume_file):
+ os.remove(self.resume_file)
+except Exception as e:
+ self.logger.error(f"Failed to remove resume file {self.resume_file}: {e}")📝 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.
| shutil.rmtree(self.temp_dir) | |
| except: | |
| self.logger.warning("Failed to clean up temp directory") | |
| try: | |
| if os.path.exists(self.resume_file): | |
| os.remove(self.resume_file) | |
| except: | |
| self.logger.error("Failed to remove resume file") | |
| try: | |
| shutil.rmtree(self.temp_dir) | |
| except Exception as e: | |
| self.logger.warning(f"Failed to clean up temp directory {self.temp_dir}: {e}") | |
| try: | |
| if os.path.exists(self.resume_file): | |
| os.remove(self.resume_file) | |
| except Exception as e: | |
| self.logger.error(f"Failed to remove resume file {self.resume_file}: {e}") |
🧰 Tools
🪛 Ruff (0.12.2)
565-565: Do not use bare except
(E722)
571-571: Do not use bare except
(E722)
572-572: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/managers/task_executor.py around lines 564 to
572, the cleanup code uses bare except blocks which can swallow errors; change
them to catch Exception as e and log the exception details — use
self.logger.warning("Failed to clean up temp directory: %s", e) or
self.logger.exception(...) for the temp_dir removal, and similarly
self.logger.error("Failed to remove resume file: %s", e) or
self.logger.exception(...) for resume_file removal so the underlying error and
stack trace are recorded.
| import builtins | ||
| flag_name = f'GOGDL_CANCEL_{game_id}' | ||
| if hasattr(builtins, flag_name) and getattr(builtins, flag_name, False): | ||
| session.close() | ||
| return # Exit worker thread if cancelled | ||
| except: | ||
| pass # Continue if cancellation check fails |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cancellation check robustness.
The bare except clause could mask important exceptions. Consider catching specific exceptions or at least logging when the cancellation check fails.
try:
import builtins
flag_name = f'GOGDL_CANCEL_{game_id}'
if hasattr(builtins, flag_name) and getattr(builtins, flag_name, False):
session.close()
return # Exit worker thread if cancelled
-except:
- pass # Continue if cancellation check fails
+except (ImportError, AttributeError) as e:
+ Timber.w(f"Failed to check cancellation flag: {e}")📝 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.
| import builtins | |
| flag_name = f'GOGDL_CANCEL_{game_id}' | |
| if hasattr(builtins, flag_name) and getattr(builtins, flag_name, False): | |
| session.close() | |
| return # Exit worker thread if cancelled | |
| except: | |
| pass # Continue if cancellation check fails | |
| import builtins | |
| flag_name = f'GOGDL_CANCEL_{game_id}' | |
| if hasattr(builtins, flag_name) and getattr(builtins, flag_name, False): | |
| session.close() | |
| return # Exit worker thread if cancelled | |
| except (ImportError, AttributeError) as e: | |
| Timber.w(f"Failed to check cancellation flag: {e}") |
🧰 Tools
🪛 Ruff (0.12.2)
87-87: Do not use bare except
(E722)
87-88: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
In app/src/main/python/gogdl/dl/workers/task_executor.py around lines 82 to 88,
the current bare except can swallow important errors; change it to catch
Exception as e and log a warning with context (including game_id and the
exception) instead of silently passing, so cancellation check failures are
visible; avoid catching BaseException (so KeyboardInterrupt/SystemExit still
propagate).
| private var python: Python? = null | ||
|
|
||
| // Constants | ||
| private const val GOG_CLIENT_ID = "46899977096215655" |
There was a problem hiding this comment.
What specifically is this client ID?
3 participants
Summary by CodeRabbit
New Features
Enhancements