Added container config export for all platforms#649
Added container config export for all platforms#649unbelievableflavour wants to merge 1 commit intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughRefactors SteamAppScreen to delegate config actions to BaseAppScreen, adds a ContainerConfigTransfer utility for exporting container JSON, and implements companion-level export request tracking plus UI wiring (document picker launch) in BaseAppScreen. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as BaseAppScreen UI
participant Picker as DocumentPicker (CreateDocument)
participant Transfer as ContainerConfigTransfer
participant Storage as SystemStorage
U->>UI: Select "Export Config" menu option
UI->>UI: requestExportConfig(appId) (companion-state)
UI->>UI: snapshotFlow detects request -> launch Picker with filename
Picker-->>UI: returns Uri (or cancels)
alt Uri returned
UI->>Transfer: exportConfig(context, appId, uri)
Transfer->>Storage: open OutputStream at Uri, write JSON
Transfer-->>UI: return success/failure (Boolean)
UI->>UI: clearExportConfigRequest(appId)
UI-->>U: show toast via Transfer result
else Cancelled
UI->>UI: clearExportConfigRequest(appId)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
app/src/main/java/app/gamenative/ui/screen/library/appscreen/EpicAppScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt:27">
P2: `openOutputStream` can return null; the safe-call skips writing and still reports success, causing false-positive export completion.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt:669">
P2: exportConfigRequested remains true until the ActivityResult returns, so a configuration change can re-run LaunchedEffect and launch a second file picker. Clear the request before launching to prevent duplicate pickers on activity recreation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt`:
- Around line 26-31: The write path currently ignores a null return from
context.contentResolver.openOutputStream(uri) and still reports success; update
the code in ContainerConfigTransfer (the suspend function using
withContext(Dispatchers.IO)) to treat openOutputStream(uri) == null as a
failure: check the result of openOutputStream before calling use (replace the
safe-call chain openOutputStream(uri)?.use { ... } with an explicit val out =
context.contentResolver.openOutputStream(uri) and if (out == null) return false
(or throw a handled exception), otherwise use out.use {
it.write(jsonText.toByteArray(Charsets.UTF_8)); it.flush() } so the function
only returns true when the stream was actually opened and written.
- Around line 39-59: The current catch ordering makes the IOException handler
unreachable and silently swallows coroutine cancellations; in the try/catch
around the export logic in ContainerConfigTransfer (the catch (e: Exception) and
catch (e: IOException) blocks), reorder and refine the handlers: first catch
CancellationException and rethrow it, then catch IOException to show the
IO-specific Toast, and finally catch Exception for other errors; ensure each
Toast uses the same string resource but with an appropriate fallback message
(e.g., "IO error" for IOException, "Unknown error" for Exception).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/CustomGameAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/EpicAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/GOGAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.ktapp/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt
app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt
Outdated
Show resolved
Hide resolved
f408c7f to
5047203
Compare
phobos665
left a comment
There was a problem hiding this comment.
LGTM.
Have you tested for all of the various ones?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt (2)
39-59:⚠️ Potential issue | 🟠 MajorReorder catches and rethrow coroutine cancellation.
Line 39 catches
ExceptionbeforeIOException(Line 49), so the IO-specific branch is dead. This also swallows coroutine cancellation unlessCancellationExceptionis rethrown.🛠️ Proposed fix
import java.io.IOException +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext @@ - } catch (e: Exception) { + } catch (e: CancellationException) { + throw e + } catch (e: IOException) { Toast.makeText( context, context.getString( R.string.base_app_export_failed, - e.message ?: "Unknown error", + e.message ?: "IO error", ), Toast.LENGTH_SHORT, ).show() false - } catch (e: IOException) { + } catch (e: Exception) { Toast.makeText( context, context.getString( R.string.base_app_export_failed, - e.message ?: "IO error", + e.message ?: "Unknown error", ), Toast.LENGTH_SHORT, ).show() false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt` around lines 39 - 59, In ContainerConfigTransfer.kt fix the catch ordering and preserve coroutine cancellation: move the catch (e: IOException) block so it comes before the broader catch (e: Exception), and add an explicit catch for kotlinx.coroutines.CancellationException that immediately rethrows (throw e) before other catches; ensure any generic Exception catch does not swallow CancellationException and that toast/error handling remains only in the more specific Exception/IOException handlers.
26-31:⚠️ Potential issue | 🟠 MajorHandle null
openOutputStreamas an export failure.At Line 27, a null stream path currently falls through to success (
true) even though no bytes were written.🛠️ Proposed fix
withContext(Dispatchers.IO) { - context.contentResolver.openOutputStream(uri)?.use { outputStream -> - outputStream.write(jsonText.toByteArray(Charsets.UTF_8)) - outputStream.flush() - } + val outputStream = context.contentResolver.openOutputStream(uri) + ?: throw IOException("Unable to open output stream for URI: $uri") + outputStream.use { + it.write(jsonText.toByteArray(Charsets.UTF_8)) + it.flush() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt` around lines 26 - 31, The export currently assumes context.contentResolver.openOutputStream(uri) returns non-null; if it returns null the function still yields success. Update the withContext(Dispatchers.IO) block in ContainerConfigTransfer (the code using context.contentResolver.openOutputStream(uri), jsonText and withContext(Dispatchers.IO)) to check the result of openOutputStream: if it is null treat the export as failed (return false / propagate a failure) rather than proceeding, and only write/flush when a non-null OutputStream is returned; make sure to keep the existing use { ... } resource handling and return the correct boolean result when the stream is null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt`:
- Around line 704-714: Replace the custom SupervisorJob usage in the export
config launcher so the coroutine inherits the existing job hierarchy: change the
uiScope.launch(SupervisorJob() + Dispatchers.Main) invocation to use
uiScope.launch { } or uiScope.launch(Dispatchers.Main) { } instead, keeping the
call to ContainerConfigTransfer.exportConfig(context, appId, uri) and the
finally block that calls clearExportConfigRequest(appId) unchanged; this ensures
the coroutine launched for exporting config follows the surrounding composition
scope rather than creating a new SupervisorJob.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt`:
- Around line 39-59: In ContainerConfigTransfer.kt fix the catch ordering and
preserve coroutine cancellation: move the catch (e: IOException) block so it
comes before the broader catch (e: Exception), and add an explicit catch for
kotlinx.coroutines.CancellationException that immediately rethrows (throw e)
before other catches; ensure any generic Exception catch does not swallow
CancellationException and that toast/error handling remains only in the more
specific Exception/IOException handlers.
- Around line 26-31: The export currently assumes
context.contentResolver.openOutputStream(uri) returns non-null; if it returns
null the function still yields success. Update the withContext(Dispatchers.IO)
block in ContainerConfigTransfer (the code using
context.contentResolver.openOutputStream(uri), jsonText and
withContext(Dispatchers.IO)) to check the result of openOutputStream: if it is
null treat the export as failed (return false / propagate a failure) rather than
proceeding, and only write/flush when a non-null OutputStream is returned; make
sure to keep the existing use { ... } resource handling and return the correct
boolean result when the stream is null.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.ktapp/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt
app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.kt
Outdated
Show resolved
Hide resolved
Have tested for Steam, Epic, GOG. Pretty sure it just works for all though. Doesn't do anything platform specific. Just translated the container config to json and writes it to file. Importing is more complex, but that's not in this PR so it's fine. |
5047203 to
71bf12b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt (1)
27-32:⚠️ Potential issue | 🟠 Major
openOutputStreamreturningnullis still silently treated as successThe
?.use { ... }safe-call at line 28 skips the write whenopenOutputStreamreturnsnull, but execution continues to the successToastandreturn true. The fix from the previous review cycle was not applied.🛠️ Proposed fix
withContext(Dispatchers.IO) { - context.contentResolver.openOutputStream(uri)?.use { outputStream -> - outputStream.write(jsonText.toByteArray(Charsets.UTF_8)) - outputStream.flush() - } + context.contentResolver.openOutputStream(uri)?.use { outputStream -> + outputStream.write(jsonText.toByteArray(Charsets.UTF_8)) + outputStream.flush() + } ?: throw IOException("Unable to open output stream for URI: $uri") }
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/screen/library/appscreen/BaseAppScreen.ktapp/src/main/java/app/gamenative/ui/screen/library/appscreen/SteamAppScreen.ktapp/src/main/java/app/gamenative/ui/util/ContainerConfigTransfer.kt
No config import yet, export is already a useful feature to have even without import.
Summary by CodeRabbit
New Features
Refactor