fix: prevent downloads sheet crash during active downloads - #817
Conversation
|
Thanks, it's obviously a lot of AI noise but still appreciated. |
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of correctness/maintainability issues to address (notably validating non-positive download IDs and updating misleading log messages) before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses a crash when opening the Downloads bottom sheet while Android DownloadManager contains active (running/pending/paused) downloads by introducing a dedicated DownloadData model that safely decodes cursor rows with transient/null/unknown metadata.
Changes:
- Add a package-level
DownloadDatamodel with safe optional column decoding and explicit required-field validation. - Refactor
DownloadsFragmentto decode/update UI fromDownloadDatainstead of directly reading cursor columns. - Add Robolectric unit tests covering decoding of active downloads with null/unknown fields and missing columns.
File summaries
| File | Description |
|---|---|
| app/src/main/java/fulguris/settings/fragment/DownloadData.kt | New cursor-decoding model for downloads with required/optional field handling. |
| app/src/main/java/fulguris/settings/fragment/DownloadsFragment.kt | Switch downloads UI population/updates to use DownloadData decoding to prevent cursor/nullable crashes. |
| app/src/test/java/fulguris/settings/fragment/DownloadDataTest.kt | Add unit tests for decoding active/completed downloads and missing/invalid cursor data. |
Review details
Suppressed comments (1)
app/src/main/java/fulguris/settings/fragment/DownloadsFragment.kt:495
- These debug log messages still reference the old method name
updateDownloadFromCursor, which makes logs misleading now that the implementation isupdateDownload(downloadData: DownloadData).
setDownloadIcon(downloadPref, downloadData.status, downloadData.mimeType, downloadData.localUri)
Timber.d("updateDownloadFromCursor: Updated existing preference for download $id")
} else {
// Create new preference
downloadPref = createDownloadPreference(downloadData)
downloadsListCategory.addPreference(downloadPref)
Timber.d("updateDownloadFromCursor: Created new preference for download $id")
}
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| fun fromCursor(cursor: Cursor): DownloadData? { | ||
| val id = cursor.requiredLong(DownloadManager.COLUMN_ID)?.takeIf { it >= 0L } | ||
| ?: return null | ||
| val status = cursor.requiredInt(DownloadManager.COLUMN_STATUS) ?: return null |
| @Test | ||
| fun `row missing status is rejected`() { | ||
| val cursor = MatrixCursor(arrayOf(DownloadManager.COLUMN_ID)).apply { | ||
| addRow(arrayOf(5L)) | ||
| } | ||
|
|
||
| val data = cursor.use { | ||
| assertThat(it.moveToFirst()).isTrue() | ||
| DownloadData.fromCursor(it) | ||
| } | ||
|
|
||
| assertThat(data).isNull() | ||
| } |
|
Fair point, and thanks for taking the time on it anyway. The description was doing too much - here it is in two lines: opening the Downloads sheet crashes when DownloadManager holds an active download, because the cursor's local URI, MIME type and total size can all be null or unknown mid-transfer. DownloadData now treats those as optional and keeps only id and status required. If the diff is the noisier part rather than the prose, I'm happy to split it: keep the null-safety fix plus its test here, and move the DownloadData extraction to a follow-up. Say the word and I'll reshape it. |
|
Do you have a way for me to reproduce that crash? On which phone and Android version were you seeing it? |
|
No device of my own, so let me be straight about what I have and do not have. I did not reproduce this on hardware. The environment is @Wandering5oul's from #802: Moto g(8) play, Android 10, build QMDS30.47-33-5, WebView 148.0.7778.120, crash on opening Downloads while a large file is mid-transfer. You already tried in May and could not hit it, so the interesting question is why it is device dependent. My reading of the old code path, which is a hypothesis and not a stack trace I have seen: If that is right, the confirming evidence is one line of the crash: One thing I should flag about my own PR rather than let you find it: the tests I added pin The offer to split still stands: null-safety fix plus that regression test here, |
Add a package-level
DownloadDataproduction model that owns cursor decoding for the fields consumed by the downloads UI, treating metadata that can be null, unknown, or temporarily unavailable during running, pending, and paused states as optional while keeping identity and status validation explicit. Opening the Downloads bottom sheet while Android'sDownloadManagercontains an active download crashes the app, while the same entry renders after the transfer finishes.Decode and render a running download whose local URI and MIME type are null and whose total size is still unknown; opening/populating the list remains successful and shows the existing generic downloading state; Decode running, pending, and paused rows with zero or partial byte counts and confirm each produces a stable model without arithmetic or null failures.
Fixes #802