ADFA-2206: Fix NPE in asset installation when zip entry is missing

[Daniel-ADFA]

No description provided.

[greptile-apps]

Greptile Overview

Greptile Summary

This PR fixes a NullPointerException in SplitAssetsInstaller.doInstall() that occurred when a requested entry was not found in the assets zip file. The fix adds an elvis operator with a FileNotFoundException throw after the zipFile.getEntry(entryName) call, preventing the NPE that would occur when passing null to zipFile.getInputStream().

• Added null check for zip entry lookup with descriptive error message
• Exception is properly caught by existing error handling in AssetsInstallationHelper.install() via runCatching
• Follows the same pattern used in preInstall() for the zip file existence check

Confidence Score: 5/5

• This PR is safe to merge - it adds defensive null checking that improves error handling without changing existing behavior.
• The change is minimal, defensive, and follows existing patterns in the codebase. It converts a potential NPE into a descriptive FileNotFoundException which is already handled by the caller. No regression risk.
• No files require special attention.

Important Files Changed

File Analysis

Filename	Score	Overview
app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt	5/5	Adds null check after zipFile.getEntry() to throw FileNotFoundException instead of NPE when entry is missing from the zip file.

Sequence Diagram

sequenceDiagram
    participant AH as AssetsInstallationHelper
    participant SAI as SplitAssetsInstaller
    participant ZF as ZipFile

    AH->>SAI: doInstall(context, stagingDir, cpuArch, entryName)
    SAI->>ZF: getEntry(entryName)
    alt Entry exists
        ZF-->>SAI: ZipEntry
        SAI->>ZF: getInputStream(entry)
        ZF-->>SAI: InputStream
        SAI->>SAI: Process entry based on type
        SAI-->>AH: Success
    else Entry missing (NEW FIX)
        ZF-->>SAI: null
        SAI->>SAI: throw FileNotFoundException
        SAI-->>AH: Exception propagates
        AH->>AH: Caught by runCatching
        AH-->>AH: Result.Failure returned
    end

Loading

[greptile-apps]

_{1 file reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[coderabbitai]

📝 Walkthrough

Release Notes - ADFA-2206: Fix NPE in asset installation when zip entry is missing

Changes

• Null-check for ZIP entries: Added proper null-check in asset installation when ZIP entries are missing, throwing a localized FileNotFoundException with message "Entry '%1$s' not found in assets zip file"
• Error message architecture shift: Transitioned from string resource IDs (@stringres Int) to dynamic error message strings throughout error handling
• AssetsInstallationHelper enhancement: Added errorMessage property to Failure data class to provide derived error messages from exceptions
• Progress job lifecycle management: Improved PermissionsFragment with explicit progress job cancellation in finally block to prevent resource leaks
• Simplified error state: Removed flashError call during installation error state for cleaner error handling flow

⚠️ Risks and Best Practices Violations

• BREAKING API CHANGE: InstallationError constructor parameter changed from Int errorMessageResId (@stringres) to String errorMessage. All code instantiating this class must be updated.

• Localization Risk: Shifted from guaranteed translated string resources to dynamic exception messages. Not all exception messages are guaranteed to be properly localized, potentially exposing users to untranslated or cryptic error text.

• Inconsistent error messaging: Reliance on exception.cause?.message for error display assumes causes always provide meaningful, user-friendly text. Fallback behavior and message quality not clearly documented.

• Nullable error message handling: errorMessage derived from cause?.message may be nullable in Failure - usage patterns need verification to ensure null-safety.

✓ Positive Changes

• Proper resource cleanup with progress job cancellation in finally block demonstrates good coroutine lifecycle management
• Fixes potential NullPointerException that could crash asset installation

Walkthrough

Refactor of the installation error handling system to propagate concrete error message strings instead of resource identifiers. Changes span the error state model, installer components, viewmodel error handling, and UI fragment progress management.

Changes

Cohort / File(s)	Summary
Error State Model app/src/main/java/com/itsaky/androidide/viewmodel/InstallationState.kt	Changed InstallationError data class to accept a String errorMessage parameter instead of an Int errorMessageResId. Removed @StringRes annotation and associated import.
Asset Installation Error Handling app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt, app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt, resources/src/main/res/values/strings.xml	Added errorMessage property to Failure data class in AssetsInstallationHelper. Added null-check in SplitAssetsInstaller.doInstall() to raise FileNotFoundException with localized message when ZIP entry is missing. Added new string resource err_asset_entry_not_found for missing entry error messaging.
ViewModel Error Propagation app/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.kt	Updated error handling paths to compute concrete error message strings and propagate via UI events. On AssetsInstallationHelper.Result.Failure, derives error message and emits ShowError with the message. On exceptions, constructs error message from exception or default and updates InstallationError with the string value.
UI Fragment Progress Management app/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.kt	Removed flashError call in InstallationError state handler. Introduced progressJob coroutine on Main dispatcher to collect setup progress updates. Wrapped state-waiting logic in try-finally block to ensure progressJob cancellation on flow exit.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• InstallationViewModel.kt: Error message construction logic across multiple failure paths requires verification that fallback behavior and exception-to-message conversion is sound
• PermissionsFragment.kt: Progress job lifecycle management in try-finally block should be verified for proper cancellation and no resource leaks
• SplitAssetsInstaller.kt: New null-check validity and localized error message propagation chain

Poem

🐰 Errors now speak their minds so clear,
No cryptic IDs to fill you with fear—
Concrete messages flow through the line,
With progress tracked and state divine! 🎯

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to assess relevance to the changeset.	Add a description explaining the changes, such as the NPE fix in zip entry lookup and related refactoring of error handling.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing a null pointer exception (NPE) in asset installation when a zip entry is missing, which aligns with the primary code change in SplitAssetsInstaller.kt.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ADFA-2206

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

app/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.kt (2)

92-99: Consider simplifying the event emission.

The nested viewModelScope.launch inside the withContext(Dispatchers.IO) block is unnecessary. Since emit() is a suspend function and MutableSharedFlow is thread-safe, you can call it directly without launching a new coroutine.

 							is AssetsInstallationHelper.Result.Failure -> {
 								result.cause?.let { Sentry.captureException(it) }
 								val errorMsg = result.errorMessage
 									?: context.getString(R.string.title_installation_failed)
-								viewModelScope.launch {
-									_events.emit(InstallationEvent.ShowError(errorMsg))
-								}
+								_events.emit(InstallationEvent.ShowError(errorMsg))
 								_state.update {
 									InstallationError(errorMsg)
 								}
 							}

---

106-112: Same simplification applies here.

The nested launch in the exception handler can also be removed.

 				} catch (e: Exception) {
 					Sentry.captureException(e)
 					log.error("IDE setup installation failed", e)
 					val errorMsg = e.message ?: context.getString(R.string.unknown_error)
-					viewModelScope.launch {
-						_events.emit(InstallationEvent.ShowError(errorMsg))
-					}
+					_events.emit(InstallationEvent.ShowError(errorMsg))
 					_state.update {
 						InstallationError(errorMsg)
 					}
 				}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 32c5aab and c3be4c1.

📒 Files selected for processing (6)

• app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt (1 hunks)
• app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (2 hunks)
• app/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.kt (2 hunks)
• app/src/main/java/com/itsaky/androidide/viewmodel/InstallationState.kt (1 hunks)
• app/src/main/java/com/itsaky/androidide/viewmodel/InstallationViewModel.kt (1 hunks)
• resources/src/main/res/values/strings.xml (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

app/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.kt (1)

app/src/main/java/com/itsaky/androidide/fragments/debug/Resolvable.kt (1)

• withContext (77-78)

⏰ Context from checks skipped due to timeout of 90000ms. 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/assets/AssetsInstallationHelper.kt (1)

49-52: LGTM! Good enhancement to error propagation.

The addition of errorMessage with a sensible default derived from the cause provides flexibility for callers to either use the automatic message or supply a custom one (as done in SplitAssetsInstaller).

resources/src/main/res/values/strings.xml (1)

47-47: LGTM!

Clear, descriptive error message with proper format placeholder for the missing entry name.

app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (1)

48-49: LGTM! This is the core fix for the NPE.

The null check prevents the NullPointerException that would occur when zipFile.getEntry(entryName) returns null and subsequent code attempts to use it. The localized error message provides clear feedback to users about which entry is missing.

app/src/main/java/com/itsaky/androidide/fragments/onboarding/PermissionsFragment.kt (1)

222-247: LGTM! Good structured concurrency improvement.

The try/finally block ensures the progressJob is properly cancelled regardless of how the flow exits (success, error, or exception). This prevents potential resource leaks from orphaned coroutines.

app/src/main/java/com/itsaky/androidide/viewmodel/InstallationState.kt (1)

13-13: LGTM!

Changing from resource ID to concrete string enables dynamic error messages that can include runtime values (like the missing entry name). This flexibility is essential for the NPE fix's error reporting.