Conversation
📝 WalkthroughRelease NotesFeatures & Improvements
Code Quality & Maintenance
Risk Considerations & Best Practices Notes
WalkthroughThis change refactors asset installation error handling and extraction flows. AssetsInstallationHelper now derives dynamic error messages from exceptions. BundledAssetsInstaller and SplitAssetsInstaller are refactored to use new channel-based utilities with retry logic. New string resources added for terminal installation errors. InstallerIoUtils introduces helpers for temporary ZIP channel handling, Brotli asset writing, and retry-on-failure patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
editor/src/main/java/com/itsaky/androidide/editor/language/newline/BracketsNewlineHandler.kt (1)
51-51: Unused variable after refactor.The variable
countis declared but never used after the changes to lines 56-57. This appears to be leftover from the previous implementation where it was part of thecoerceIncalculation.🧹 Proposed fix to remove dead code
- val count = 100_000_000 - val advanceBefore: Int = getIndentAdvance(beforeText)
🤖 Fix all issues with AI agents
In @app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt:
- Around line 240-255: The retryOnceOnNoSuchFile function is catching
kotlin.io.NoSuchFileException by default; add an explicit import for
java.nio.file.NoSuchFileException so the catch clauses refer to the java.nio
variant thrown by Files.createDirectories(); update the file-level imports to
include java.nio.file.NoSuchFileException and keep the function signature and
catch clauses as-is so the retry logic will catch the correct exception type.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktapp/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kteditor/src/main/java/com/itsaky/androidide/editor/language/newline/BracketsNewlineHandler.kteditor/src/main/java/com/itsaky/androidide/editor/language/newline/TSBracketsHandler.ktresources/src/main/res/values/strings.xml
🔇 Additional comments (7)
editor/src/main/java/com/itsaky/androidide/editor/language/newline/TSBracketsHandler.kt (1)
42-43: LGTM!The change to dynamically compute indentation by counting leading spaces in the current line is a clear improvement over a hard-coded constant. The
coerceInbounds check ensures safety against excessively large values.resources/src/main/res/values/strings.xml (1)
51-51: LGTM!The new localized string provides a clear, actionable message for users when terminal setup fails due to storage issues. It appropriately suggests the likely causes (low storage or cache cleared) and offers guidance (free up storage).
editor/src/main/java/com/itsaky/androidide/editor/language/newline/BracketsNewlineHandler.kt (1)
56-64: LGTM!The refactored bounds checking directly on
advanceBeforeandadvanceAfteris cleaner. The char literal for newline is a minor but appropriate optimization.app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt (1)
76-80: LGTM!Good improvement to error handling: extracting the actual exception message, logging with the exception for stack traces, and propagating the specific error through both progress and the
Failureresult. This aligns well with the PR objective of better UX and debugging.app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (3)
72-95: LGTM!The retry logic is well-structured: on first
NoSuchFileException, recreate the staging directory (addressing the race condition), and on second failure, throw with the user-friendly localized message. The use ofwithTempZipChannelcleanly abstracts the temp file lifecycle.
209-224: LGTM!Clean helper function with proper resource management. The
finallyblock ensures the temp file is cleaned up regardless of success or failure. The inline modifier is appropriate for this small utility.
226-238: LGTM!Clean helper for Brotli asset extraction with proper nested resource management using
use{}blocks.
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
🔇 Additional comments (4)
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (4)
210-225: Excellent abstraction with proper resource management.The
withTempZipChannelhelper cleanly encapsulates the temp file lifecycle:
- Separates file creation from usage
- Guarantees cleanup via
finallyblock- Properly manages the channel lifetime with
use {}The
inlinemodifier is appropriate here to eliminate lambda allocation overhead.
227-239: Clean Brotli extraction helper.The function correctly handles Brotli decompression with proper resource management. All streams are closed automatically via
use {}blocks.
241-256: Robust single-retry pattern with excellent type safety.The implementation is well-crafted:
- The
Nothingreturn type ononSecondFailureenforces that it must throw an exception, preventing accidental silent failures- Scoped specifically to
NoSuchFileException, allowing other exceptions to propagate immediately- Single retry limit prevents masking persistent storage or permission issues
- Default empty
onFirstFailureprovides flexibilityThis reusable helper elegantly addresses the race condition described in the PR objectives.
77-92: Well-designed retry mechanism for transient file system issues.The retry logic correctly handles the scenario where the staging directory may not exist when the installation process runs. The approach is sound:
- First failure: pre-create the staging directory to eliminate missing parent as a cause
- Second failure: fail fast with a localized, user-friendly message ("Terminal setup failed. Your device may be low on storage or the app cache was cleared during setup. Please free up storage and try again.")
The helper functions are properly implemented:
withTempZipChannelensures correct resource cleanup via try-finally even when exceptions occur,writeBrotliAssetToPathproperly manages nested streams, andretryOnceOnNoSuchFilecorrectly implements the retry logic with exception-specific handling.
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
Outdated
Show resolved
Hide resolved
jatezzz
force-pushed
the
fix/ADFA-2446-terminal-bootstrap-retry
branch
from
ac4cfe4 to
a9fea41
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
editor/src/main/java/com/itsaky/androidide/editor/language/newline/BracketsNewlineHandler.kt (1)
51-57: Remove unusedcountvariable.The
countvariable declared on line 51 is now dead code since it's no longer used in thesafeIndentBeforeandsafeIndentAftercalculations. This should be removed to avoid confusion.🧹 Suggested cleanup
private fun handleNewline( beforeText: String?, afterText: String?, tabSize: Int ): NewlineHandleResult { - val count = 100_000_000 - val advanceBefore: Int = getIndentAdvance(beforeText) val advanceAfter: Int = getIndentAdvance(afterText) val safeIndentBefore = advanceBefore.coerceIn(0, maxIndentColumns) val safeIndentAfter = advanceAfter.coerceIn(0, maxIndentColumns)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktapp/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kteditor/src/main/java/com/itsaky/androidide/editor/language/newline/BracketsNewlineHandler.kteditor/src/main/java/com/itsaky/androidide/editor/language/newline/TSBracketsHandler.ktresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- editor/src/main/java/com/itsaky/androidide/editor/language/newline/TSBracketsHandler.kt
- resources/src/main/res/values/strings.xml
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Universal APK
🔇 Additional comments (7)
editor/src/main/java/com/itsaky/androidide/editor/language/newline/BracketsNewlineHandler.kt (1)
61-61: LGTM!Using a char literal
'\n'instead of a string literal"\n"forStringBuilder.append()is a good micro-optimization.app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt (1)
75-81: LGTM! Improved error propagation.The changes correctly derive the error message with a fallback, log the full exception for debugging, and forward the meaningful message to both the progress consumer and the
Result.Failure. This aligns with the PR objective to improve user-facing error messaging.app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (5)
221-236: LGTM! Proper resource management.The
withTempZipChannelhelper correctly handles the temp file lifecycle: creation, usage via channel withuse, and cleanup infinally. Theinlinemodifier is appropriate for the lambda parameters.
238-250: LGTM!Clean helper with proper stream chaining and resource cleanup via nested
useblocks.
252-267: LGTM! Well-designed retry mechanism.The
Nothingreturn type foronSecondFailureenforces that it must throw, ensuring type safety. The single-retry limit is appropriate per the PR rationale to avoid masking persistent storage issues.
73-108: LGTM! Robust bootstrap installation with clear failure messaging.The implementation correctly addresses the transient
NoSuchFileExceptionscenario described in the PR objectives. The retry recreates the staging directory on first failure, and the second failure surfaces a user-friendly localized message. The result pattern matching properly converts eachInstallResultvariant to an appropriate exception type.
94-107: No action needed. Thewhenexpression is exhaustive and correctly handles all variants of the sealedInstallResultinterface. The compiler enforces exhaustiveness checking for sealed types, so noelsebranch is required.Likely an incorrect or invalid review comment.
editor/src/main/java/com/itsaky/androidide/editor/language/newline/BracketsNewlineHandler.kt
Show resolved
Hide resolved
editor/src/main/java/com/itsaky/androidide/editor/language/newline/TSBracketsHandler.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt:
- Around line 71-92: The onSecondFailure lambda passed to retryOnceOnNoSuchFile
in SplitAssetsInstaller currently does return@withContext and swallows the
error; change it to throw an exception (e.g., throw IOException("Failed to open
temporary bootstrap zip after retry", e2)) so the failure is propagated from
doInstall like BundledAssetsInstaller does; update the onSecondFailure callback
in the retryOnceOnNoSuchFile call that wraps
withTempZipChannel/TerminalInstaller.installIfNeeded to rethrow the
NoSuchFileException (or wrap it) instead of returning, ensuring callers of
doInstall see the error.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/utils/InstallerIoUtils.kt (1)
41-56: Effective single-retry pattern with type-safe failure handling.The
Nothingreturn type ononSecondFailureis a clever design choice that forces callers to either throw an exception or enter an infinite loop (the latter being unlikely). This prevents accidental silent failures.Minor observation: the first
NoSuchFileExceptionis discarded (_), which is acceptable sinceonFirstFailureis meant for recovery actions, not error inspection. If future debugging needs arise, consider logging the first exception path before retry.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/utils/InstallerIoUtils.ktresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/src/main/res/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (1)
app/src/main/java/com/itsaky/androidide/utils/InstallerIoUtils.kt (3)
retryOnceOnNoSuchFile(41-56)withTempZipChannel(10-25)writeBrotliAssetToPath(27-39)
app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (1)
app/src/main/java/com/itsaky/androidide/utils/InstallerIoUtils.kt (2)
retryOnceOnNoSuchFile(41-56)withTempZipChannel(10-25)
🔇 Additional comments (8)
app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (2)
9-10: LGTM!New imports correctly reference the utility functions from
InstallerIoUtils.kt.
94-98: Result handling after retry looks correct.The result check logs an error on non-success, which is appropriate. However, note that if
onSecondFailureis fixed to throw, this block will only execute on success or first-attempt failures that recovered.app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (3)
8-13: LGTM!New imports are properly organized and support the new retry mechanism and error handling.
Also applies to: 26-26
78-93: Well-implemented retry mechanism with proper error propagation.The
onSecondFailurecorrectly throws anIOExceptionwith a localized message, satisfying theNothingreturn type contract. The staging directory creation on first failure is a sensible recovery strategy for transientNoSuchFileExceptionissues.
95-108: Exhaustive result handling with appropriate exception types.The
whenexpression correctly handles allInstallResultvariants:
Success: no-op (correct)Interactive: throwsIOExceptionwith title and messageIsSecondaryUser: throwsIOExceptionwith localized stringNotInstalled: throwsIllegalStateExceptionThis ensures errors propagate to callers for proper handling.
app/src/main/java/com/itsaky/androidide/utils/InstallerIoUtils.kt (3)
1-9: LGTM!Package declaration and imports are appropriate for the utility functions defined.
10-25: Well-designed temp file lifecycle management.The
try-finallyensures cleanup even ifwriteTooruseChannelthrows. Theinlinemodifier is appropriate for a function accepting lambdas to avoid allocation overhead.One consideration: if
Files.createTempFilethrows (e.g.,stagingDirdoesn't exist), the exception propagates directly. This is the intended behavior since the calling code wraps this inretryOnceOnNoSuchFileto handle that case.
27-39: Clean Brotli decompression helper.Proper use of nested
.use {}blocks ensures all streams are closed in the correct order (inner-to-outer). The function is straightforward and does one thing well.
…lure & extracting shared functions to a Util file wip
jatezzz
force-pushed
the
fix/ADFA-2446-terminal-bootstrap-retry
branch
from
33bc6c7 to
b353f64
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktapp/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/utils/InstallerIoUtils.ktresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/src/main/res/values/strings.xml
- app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (1)
app/src/main/java/com/itsaky/androidide/utils/InstallerIoUtils.kt (2)
retryOnceOnNoSuchFile(41-56)withTempZipChannel(10-25)
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (1)
app/src/main/java/com/itsaky/androidide/utils/InstallerIoUtils.kt (3)
retryOnceOnNoSuchFile(41-56)withTempZipChannel(10-25)writeBrotliAssetToPath(27-39)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Universal APK
🔇 Additional comments (5)
app/src/main/java/com/itsaky/androidide/utils/InstallerIoUtils.kt (3)
10-25: Well-designed temp file lifecycle management.The
withTempZipChannelcorrectly handles the temp file cleanup infinally, ensuring deletion even ifwriteTooruseChannelthrows. The.use {}block properly closes the channel.
27-39: LGTM!Clean decompression pipeline with proper resource management through nested
.use {}blocks.
41-56: Solid retry utility with correctNothingreturn type.The
onSecondFailure: (NoSuchFileException) -> Nothingsignature correctly enforces that callers must terminate (throw/exit) on second failure. The intentional suppression of the first exception aligns with the PR rationale of handling transient failures.app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt (2)
78-93: Good retry implementation with proper error propagation.The retry logic correctly creates the staging directory on first failure and throws a user-friendly
IOExceptionwith localized message on second failure. ThewithTempZipChannelusage is clean.
95-108: Comprehensive error handling for all result variants.Exhaustive
whenblock with appropriate exception types:IOExceptionfor user-actionable errors andIllegalStateExceptionfor unexpected states.
4 participants
Description
This PR hardens the terminal bootstrap installation by retrying once when the temporary bootstrap zip disappears during channel open (likely due to cache/staging cleanup or race conditions). If the issue occurs twice, we fail fast with a localized, user-friendly message instead of a generic error, and propagate the exception message through the existing install progress reporting.
Details
NoSuchFileException.AssetsInstallationHelper.install()to forward the underlying failure message to progress output and Result.Failure for better UX and debugging.Ticket
ADFA-2446
Observation
The installer runs multiple asset installs concurrently against a shared staging directory, so transient
NoSuchFileExceptioncan occur if the temp file or parent directory is removed between write and channel open. The retry is intentionally bounded to a single attempt to avoid masking persistent storage issues.