Crash: FileNotFoundException EACCES in Uri.fileContent when reading content URI (#4426)#4435
Conversation
Wrap contentResolver.openInputStream() in try/catch so transient EACCES
or other I/O failures return null instead of crashing the app. Also use
.use {} to ensure the InputStream is closed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds exception handling to ChangesFile URI Exception Handling and ViewModel Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Pushed updated implementation. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
No actionable changes made. The only feedback comment is There are currently no inline review comments and no formal review verdicts to address. I'll wait for CodeRabbit's findings (or human reviewer feedback) before making further changes. If you'd like me to act on something specific in the meantime, please point me at it. |
|
Tip For best results, initiate chat on the files or code changes.
[full_review] |
rkokhatskyi
left a comment
There was a problem hiding this comment.
@Vaulty-bot please cover this case by Unit tests
Add unit tests verifying that Uri.fileContent returns null instead of crashing when the platform throws FileNotFoundException (EACCES), SecurityException, generic IOException during read, or returns a null InputStream — covering the crash scenario from issue #4426. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Uri.fileContent now rethrows CancellationException so a cancelled viewModelScope no longer keeps executing through the caller. - ImportFileViewModel.saveFileToAppDir surfaces the unsupported-file error UiText when fileContent returns null, matching the existing fetchFileName error path for the same EACCES root cause. - Add unit tests for both behaviors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed review feedback in commits b59ad1a and 3e5b842. Formal review (rkokhatskyi, CHANGES_REQUESTED — "please cover this case by Unit tests"):
Inline review comments:
Notes:
|
|
@Vaulty-bot fix the build errors |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
data/src/main/kotlin/com/vultisig/wallet/data/common/FileHelper.kt (1)
153-156: ⚡ Quick winNarrow the fallback catch to expected I/O/permission failures.
Catching
Exceptionhere may mask unrelated runtime defects. Given the PR goal, prefer explicit catches forFileNotFoundException,SecurityException, andIOException(afterCancellationException) and let unexpected exceptions surface.♻️ Suggested change
- } catch (e: Exception) { + } catch (e: java.io.FileNotFoundException) { + Timber.e(e, "error in fileContent") + null + } catch (e: SecurityException) { + Timber.e(e, "error in fileContent") + null + } catch (e: java.io.IOException) { Timber.e(e, "error in fileContent") null }Based on learnings: "Do not use a broad catch (catch (Exception)) to swallow errors in Kotlin. Catch specific, actionable exceptions … Avoid masking real failures."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data/src/main/kotlin/com/vultisig/wallet/data/common/FileHelper.kt` around lines 153 - 156, In the fileContent function replace the broad catch (e: Exception) with specific catches: first handle CancellationException by rethrowing it, then catch FileNotFoundException, SecurityException, and IOException and log them with Timber.e (preserving the "error in fileContent" context) returning null as before; do not catch other Exceptions so unexpected runtime errors propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@data/src/main/kotlin/com/vultisig/wallet/data/common/FileHelper.kt`:
- Around line 153-156: In the fileContent function replace the broad catch (e:
Exception) with specific catches: first handle CancellationException by
rethrowing it, then catch FileNotFoundException, SecurityException, and
IOException and log them with Timber.e (preserving the "error in fileContent"
context) returning null as before; do not catch other Exceptions so unexpected
runtime errors propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a97bc6e-4f6d-4caf-ba2f-89a93bf2a36b
📒 Files selected for processing (4)
app/src/main/java/com/vultisig/wallet/ui/models/ImportFileViewModel.ktapp/src/test/java/com/vultisig/wallet/ui/models/ImportFileViewModelTest.ktdata/src/main/kotlin/com/vultisig/wallet/data/common/FileHelper.ktdata/src/test/kotlin/com/vultisig/wallet/data/common/FileHelperUriFileContentTest.kt
|
Available commands:
Agent runs every ~5 minutes. Only trusted org members and collaborators can trigger commands. |
|
Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it. |
|
Only nitpick feedback remaining on this PR. CodeRabbit review (id 4248235704) flagged one 🧹 Nitpick: narrow the fallback Prior actionable comments (rkokhatskyi, id 3204564773 / id 3204564783) were both addressed in 3e5b842 and have inline replies. CI |
Fixes #4426
Changes
FileHelper.kt: WrapUri.fileContentbody intry/catch, using.use {}for theInputStream. Transient permission failures (FileNotFoundExceptionwithEACCES) and other I/O exceptions now log via Timber and returnnullinstead of propagating and crashing the app.Checklist
./gradlew lintcould not run in this environment due to a 401 fetchingcom.trustwallet:wallet-corefrom GitHub Maven — needs CI run)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests