refactor: Moved Proton downloading to launch deps (again, but mirroring original behaviour + tests)#1052
Open
unbelievableflavour wants to merge 4 commits intoutkarshdalal:masterfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changes
Sequence DiagramsequenceDiagram
participant App as App (preLaunchApp)
participant Split as SplitCompat
participant LaunchDeps as LaunchDependencies
participant Bionic as BionicDefaultProtonDependency
participant Steam as SteamService
participant Tar as TarCompressorUtils
participant FS as ImageFs
App->>Split: install(context)
App->>LaunchDeps: ensureLaunchDependencies(context,...)
LaunchDeps->>Bionic: appliesTo(container, gameSource, gameId)?
alt applies
LaunchDeps->>Bionic: isSatisfied(context,...)
alt not satisfied
Bionic->>LaunchDeps: getLoadingMessage(...)
LaunchDeps->>Bionic: install(context, callbacks,...)
Bionic->>Steam: isFileInstallable(archive)
alt not installable
Bionic->>Steam: downloadFile(archive, progressCallback)
Steam->>FS: write archive
progressCallback-->>Bionic: progress updates
end
Bionic->>Tar: extract(archive, opt/<version>)
Tar->>FS: create opt/<version>/bin
Tar-->>Bionic: extraction result
end
end
LaunchDeps-->>App: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt (1)
69-69: Minor: Reuse theimageFsvariable from line 50.
ImageFs.find(context)is called again at line 69 when theimageFsvariable from line 50 could be reused for consistency and minor efficiency.♻️ Suggested fix
- val outFile = File(ImageFs.find(context).getRootDir(), "opt/$protonVersion") + val outFile = File(imageFs.getRootDir(), "opt/$protonVersion")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt` at line 69, Replace the redundant ImageFs.find(context) call with the already-obtained imageFs variable when building outFile: use imageFs.getRootDir() (or the equivalent accessor used earlier) instead of calling ImageFs.find(context) again so outFile is created with File(imageFs.getRootDir(), "opt/$protonVersion"), referencing the existing imageFs variable and protonVersion in BionicDefaultProtonDependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt`:
- Line 69: Replace the redundant ImageFs.find(context) call with the
already-obtained imageFs variable when building outFile: use
imageFs.getRootDir() (or the equivalent accessor used earlier) instead of
calling ImageFs.find(context) again so outFile is created with
File(imageFs.getRootDir(), "opt/$protonVersion"), referencing the existing
imageFs variable and protonVersion in BionicDefaultProtonDependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1707fd8f-995c-447e-9790-96b3b7dd1b09
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/utils/LaunchDependencies.ktapp/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt
unbelievableflavour
force-pushed
the
proton-mirror-previous
branch
from
ec934cc to
ccf9a03
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt (1)
69-71: ReuseimageFsvariable instead of callingImageFs.find(context)again.Line 50 already fetches
imageFs, but line 69 callsImageFs.find(context)a second time. Reuse the existing variable for consistency and to avoid redundant work.♻️ Suggested fix
- val outFile = File(ImageFs.find(context).getRootDir(), "opt/$protonVersion") + val outFile = File(imageFs.getRootDir(), "opt/$protonVersion")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt` around lines 69 - 71, The code calls ImageFs.find(context) again when constructing outFile; reuse the previously obtained imageFs variable instead. Update the outFile and subsequent paths to use imageFs.getRootDir() (and any other places using ImageFs.find(context)) so that the existing imageFs value is used consistently in BionicDefaultProtonDependency (variables: imageFs, outFile, binDir, needsExtract).app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt (1)
99-169: Tests cover the download path well; consider adding extraction coverage.The
installtests correctly verify:
- Unsupported Proton versions don't trigger downloads
- Download is invoked with correct archive name when file isn't installable
- Progress callback is properly forwarded
Note:
install_downloadsArchive_andForwardsProgresscreatesbinDir(line 130), so the extraction path is intentionally skipped. Consider adding a separate test for the extraction logic (e.g., verifyingTarCompressorUtils.extractis called whenbinDirdoesn't exist) for more complete coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt` around lines 99 - 169, Add a new test alongside install_downloadsArchive_andForwardsProgress that exercises the extraction path of BionicDefaultProtonDependency.install: do not create the binDir (use ImageFs.find(context).rootDir to compute the expected "opt/proton-9.0-arm64ec/bin" path and ensure it does not exist), mock SteamService.downloadFile to return a Deferred whose await completes, mockkStatic(TarCompressorUtils::class) and stub/isolate TarCompressorUtils.extract, call BionicDefaultProtonDependency.install with the same parameters, then verify TarCompressorUtils.extract(...) was invoked with the downloaded archive and the expected destination and that SteamService.downloadFile was called; reference functions/classes: BionicDefaultProtonDependency.install, SteamService.downloadFile, TarCompressorUtils.extract, ImageFs.find.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt`:
- Around line 69-71: The code calls ImageFs.find(context) again when
constructing outFile; reuse the previously obtained imageFs variable instead.
Update the outFile and subsequent paths to use imageFs.getRootDir() (and any
other places using ImageFs.find(context)) so that the existing imageFs value is
used consistently in BionicDefaultProtonDependency (variables: imageFs, outFile,
binDir, needsExtract).
In
`@app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt`:
- Around line 99-169: Add a new test alongside
install_downloadsArchive_andForwardsProgress that exercises the extraction path
of BionicDefaultProtonDependency.install: do not create the binDir (use
ImageFs.find(context).rootDir to compute the expected
"opt/proton-9.0-arm64ec/bin" path and ensure it does not exist), mock
SteamService.downloadFile to return a Deferred whose await completes,
mockkStatic(TarCompressorUtils::class) and stub/isolate
TarCompressorUtils.extract, call BionicDefaultProtonDependency.install with the
same parameters, then verify TarCompressorUtils.extract(...) was invoked with
the downloaded archive and the expected destination and that
SteamService.downloadFile was called; reference functions/classes:
BionicDefaultProtonDependency.install, SteamService.downloadFile,
TarCompressorUtils.extract, ImageFs.find.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a96c929a-66ea-4b37-89dd-546a610a1268
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/utils/LaunchDependencies.ktapp/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.ktapp/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/utils/LaunchDependencies.kt
unbelievableflavour
changed the title
unbelievableflavour
changed the title
unbelievableflavour
force-pushed
the
proton-mirror-previous
branch
from
ccf9a03 to
7f9c0d2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt (1)
69-69: Reuse the existingimageFsinstance.Line 69 does another
ImageFs.find(context)despite already resolvingimageFson Line 50. Reusing the existing value improves clarity and avoids redundant lookup.♻️ Proposed cleanup
- val outFile = File(ImageFs.find(context).getRootDir(), "opt/$protonVersion") + val outFile = File(imageFs.getRootDir(), "opt/$protonVersion")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt` at line 69, The code calls ImageFs.find(context) again when creating outFile even though imageFs was already resolved earlier; change the outFile creation to reuse the existing imageFs instance (use imageFs.getRootDir() when constructing outFile) so you avoid the redundant lookup—update the expression that sets outFile in BionicDefaultProtonDependency.kt to reference the imageFs variable instead of calling ImageFs.find(context) again.
🤖 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/utils/launchdependencies/BionicDefaultProtonDependency.kt`:
- Around line 16-19: The KDoc in BionicDefaultProtonDependency.kt is out of
sync: it claims extraction to imagefs_shared/proton but the implementation
writes to opt/<wineVersion> (see usage of wineVersion and the destination path
construction); update the KDoc to describe the real behavior (that Proton is
downloaded/extracted into opt/<wineVersion> for the BIONIC container variant and
only for proton-9.0-arm64ec or proton-9.0-x86_64), or alternatively change the
extraction target in the code to match the comment—choose and make one source of
truth so the KDoc and the extraction path (where opt/<wineVersion> is
constructed) are consistent.
- Around line 60-65: The call to SteamService.downloadFile uses named arguments
followed by a positional argument (archiveName), which is invalid Kotlin; update
the call site in BionicDefaultProtonDependency (inside the coroutineScope block
where callbacks.setLoadingProgress is used) to pass the file name as a named
parameter (fileName = archiveName) so all parameters are named consistently and
the code compiles.
---
Nitpick comments:
In
`@app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt`:
- Line 69: The code calls ImageFs.find(context) again when creating outFile even
though imageFs was already resolved earlier; change the outFile creation to
reuse the existing imageFs instance (use imageFs.getRootDir() when constructing
outFile) so you avoid the redundant lookup—update the expression that sets
outFile in BionicDefaultProtonDependency.kt to reference the imageFs variable
instead of calling ImageFs.find(context) again.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93b5c082-0f78-473d-8457-46770e174357
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/utils/LaunchDependencies.ktapp/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.ktapp/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt
- app/src/main/java/app/gamenative/ui/PluviaMain.kt
app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt
Show resolved
Hide resolved
app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt
Show resolved
Hide resolved
unbelievableflavour
force-pushed
the
proton-mirror-previous
branch
2 times, most recently
from
2243517 to
548e845
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt (1)
74-77: Isolate filesystem side effects between tests.These tests create real
opt/proton-9.0-arm64ecpaths buttearDown()doesn’t remove them, which can introduce order-dependent behavior over time.♻️ Proposed cleanup patch
class BionicDefaultProtonDependencyTest { private lateinit var context: Context private lateinit var container: Container + private val createdPaths = mutableSetOf<File>() @@ `@After` fun tearDown() { + createdPaths.forEach { it.deleteRecursively() } unmockkAll() } @@ val imageFsRoot = ImageFs.find(context).rootDir val binDir = File(imageFsRoot, "opt/proton-9.0-arm64ec/bin") binDir.mkdirs() + createdPaths += File(imageFsRoot, "opt/proton-9.0-arm64ec") @@ val imageFsRoot = ImageFs.find(context).rootDir val binDir = File(imageFsRoot, "opt/proton-9.0-arm64ec/bin") binDir.mkdirs() + createdPaths += File(imageFsRoot, "opt/proton-9.0-arm64ec")Also applies to: 128-130, 41-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt` around lines 74 - 77, Tests are creating real filesystem paths via ImageFs.find(context).rootDir and binDir.mkdirs() (in BionicDefaultProtonDependencyTest) without cleanup; modify the test to isolate side effects by creating a temporary ImageFs root (or temporary directory) and point ImageFs to it for the test, or ensure tearDown() deletes the created "opt/proton-9.0-arm64ec" subtree (remove binDir and parent dirs) after each test; update the code paths that call ImageFs.find(context).rootDir and binDir.mkdirs() (and the other occurrences at the indicated lines) to use the temp root or add removal logic in tearDown() so tests don't leave persistent files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt`:
- Around line 74-77: Tests are creating real filesystem paths via
ImageFs.find(context).rootDir and binDir.mkdirs() (in
BionicDefaultProtonDependencyTest) without cleanup; modify the test to isolate
side effects by creating a temporary ImageFs root (or temporary directory) and
point ImageFs to it for the test, or ensure tearDown() deletes the created
"opt/proton-9.0-arm64ec" subtree (remove binDir and parent dirs) after each
test; update the code paths that call ImageFs.find(context).rootDir and
binDir.mkdirs() (and the other occurrences at the indicated lines) to use the
temp root or add removal logic in tearDown() so tests don't leave persistent
files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81c479eb-bcc3-459e-b8eb-ba781b566687
📒 Files selected for processing (2)
app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.ktapp/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt
…dalal#1050) This reverts commit 1cd84b4.
unbelievableflavour
force-pushed
the
proton-mirror-previous
branch
from
548e845 to
2c604d1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt (1)
74-77: Isolate filesystem side effects to keep tests order-independent.These tests create real directories under
ImageFsroot but never clean them up, which can leak state into other tests.♻️ Suggested cleanup pattern
class BionicDefaultProtonDependencyTest { private lateinit var context: Context private lateinit var container: Container + private val createdPaths = mutableListOf<File>() @@ `@After` fun tearDown() { + createdPaths + .distinct() + .sortedByDescending { it.absolutePath.length } + .forEach { it.deleteRecursively() } unmockkAll() } @@ val binDir = File(imageFsRoot, "opt/proton-9.0-arm64ec/bin") binDir.mkdirs() + createdPaths += File(imageFsRoot, "opt/proton-9.0-arm64ec") @@ val binDir = File(imageFsRoot, "opt/proton-9.0-arm64ec/bin") binDir.mkdirs() + createdPaths += File(imageFsRoot, "opt/proton-9.0-arm64ec")Also applies to: 128-130, 41-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt` around lines 74 - 77, The test mutates the real ImageFs by creating directories (using ImageFs.find(context).rootDir and binDir.mkdirs()) and never cleans them up; modify BionicDefaultProtonDependencyTest to isolate filesystem side effects by either (a) creating a temporary directory and redirecting ImageFs.find(context).rootDir to that temp before the test and restoring it after, or (b) explicitly deleting the created directories (the binDir and any parent directories you create) in an `@After/`@AfterEach cleanup method; update all similar uses (lines around 41-44 and 128-130) so tests are order-independent and leave no state behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt`:
- Around line 74-77: The test mutates the real ImageFs by creating directories
(using ImageFs.find(context).rootDir and binDir.mkdirs()) and never cleans them
up; modify BionicDefaultProtonDependencyTest to isolate filesystem side effects
by either (a) creating a temporary directory and redirecting
ImageFs.find(context).rootDir to that temp before the test and restoring it
after, or (b) explicitly deleting the created directories (the binDir and any
parent directories you create) in an `@After/`@AfterEach cleanup method; update
all similar uses (lines around 41-44 and 128-130) so tests are order-independent
and leave no state behind.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07676269-0ed1-4c75-9ebb-01ebfa80ed88
📒 Files selected for processing (4)
app/src/main/java/app/gamenative/ui/PluviaMain.ktapp/src/main/java/app/gamenative/utils/LaunchDependencies.ktapp/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.ktapp/src/test/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependencyTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/utils/launchdependencies/BionicDefaultProtonDependency.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Unreverted the proton refactor and moved launch deps to mirror origina behaviour + removed delete which wasnt previously there.
Summary by cubic
Restores original Proton handling on Bionic by moving launch-dependency checks earlier and encapsulating Proton 9.0 downloads/extraction in
BionicDefaultProtonDependency. Reapplies the refactor, removes the deletion step, shows a sync-fail dialog on errors, and adds tests for progress and timing.BionicDefaultProtonDependencyto download/extractproton-9.0-arm64ecandproton-9.0-x86_64intoopt/<version>on Bionic when missing; forwards progress and throws on extraction failure.ensureLaunchDependencies(...)earlier inPluviaMain; removed the manual Proton block and the later duplicate call; failures stop launch with a sync-fail dialog.Written for commit 2c604d1. Summary will update on new commits.
Summary by CodeRabbit
Refactor
New Features
Tests