Fix workmanager download bugs#316
Open
dishit-wednesday wants to merge 6 commits into
Open
Conversation
…le collision, persist failed state, improve quant matching - Remove pull-to-refresh from Download Manager (redundant, screen syncs via events) - Show alert when starting a 3rd+ download; user can cancel or proceed - Prefix Android temp download filenames with downloadId to prevent concurrent mmproj downloads from clobbering each other's file (root cause of "Downloaded file not found" on devices downloading multiple vision model quantizations) - Add idempotent move: if source missing but target exists, skip re-move - On download failure, keep entry in UI with failed status instead of removing it; only user-initiated cancel clears it - Fix mmproj quant matching: prefer exact model-quant match before falling back to F16
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request removes manual refresh from the Download Manager, implements a two-download concurrency limit with user alerts, and improves Android file naming and error handling. It also refines vision projection file selection and updates download status tracking. A review comment suggests memoizing the active download count calculation for better performance.
Comment on lines
+175
to
183
| const activeDownloadCount = Object.entries(text.downloadProgress).filter(([key, progress]) => { | ||
| const status = progress?.status; | ||
| if (status === 'failed' || status === 'completed') return false; | ||
| if (key.startsWith('image:')) { | ||
| const imageId = key.split('/').slice(0, -1).join('/').replace('image:', ''); | ||
| return !image.downloadedImageModels.some(m => m.id === imageId); | ||
| } | ||
| return true; | ||
| }).length; |
Contributor
There was a problem hiding this comment.
The activeDownloadCount calculation iterates over text.downloadProgress and performs string manipulations on every render. This should be wrapped in useMemo to prevent unnecessary recalculations, especially since it depends on text.downloadProgress and image.downloadedImageModels which are external state dependencies.
Suggested change
| const activeDownloadCount = Object.entries(text.downloadProgress).filter(([key, progress]) => { | |
| const status = progress?.status; | |
| if (status === 'failed' || status === 'completed') return false; | |
| if (key.startsWith('image:')) { | |
| const imageId = key.split('/').slice(0, -1).join('/').replace('image:', ''); | |
| return !image.downloadedImageModels.some(m => m.id === imageId); | |
| } | |
| return true; | |
| }).length; | |
| const activeDownloadCount = useMemo(() => Object.entries(text.downloadProgress).filter(([key, progress]) => { | |
| const status = progress?.status; | |
| if (status === 'failed' || status === 'completed') return false; | |
| if (key.startsWith('image:')) { | |
| const imageId = key.split('/').slice(0, -1).join('/').replace('image:', ''); | |
| return !image.downloadedImageModels.some(m => m.id === imageId); | |
| } | |
| return true; | |
| }).length, [text.downloadProgress, image.downloadedImageModels]); |
…eserve downloadId on failure - activeDownloadCount now filters out failed/completed entries so failed downloads sitting in UI don't wrongly block new downloads or trigger the concurrent limit alert - onError preserves ownerDownloadId from existing progress so retry in Download Manager can still look up the download metadata and When a vision model downloads successfully but the mmproj file could not be moved to its final path, show an alert telling the user to use "Repair Vision" in Download Manager instead of silently showing success
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (52.72%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #316 +/- ##
==========================================
- Coverage 83.83% 83.76% -0.07%
==========================================
Files 223 223
Lines 11460 11482 +22
Branches 3144 3154 +10
==========================================
+ Hits 9607 9618 +11
- Misses 1070 1078 +8
- Partials 783 786 +3
🚀 New features to boost your workflow:
|
…nt before delete Two related mmproj bugs, each silent and user-visible: 1. deleteModel unconditionally unlinked the shared mmproj file. Since sibling quantizations of the same vision family share one mmproj path (by design, to avoid re-downloading ~500MB per quant), deleting any one quant broke vision on every remaining sibling. Now we scan the remaining models and only unlink when no other entry still references the path. 2. When mmproj post-download move failed, the code checked only RNFS.exists on the target and kept using it if present. That target could be a partial or corrupt leftover, leaving the model registered as vision-capable with a broken mmproj. Reuse checkMmProjExists (size check + GGUF magic-byte check, same pattern as llmSafetyChecks.validateModelFile) to validate before trusting. Invalid target is unlinked and the model saves as text-only, surfacing the existing "Repair Vision" recovery path.
|
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.
Summary
Type of Change
Screenshots / Screen Recordings
Android
iOS
Checklist
General
Testing
npm test)React Native Specific
project.pbxproj)SPACING/TYPOGRAPHYconstants from the themeuseThemedStylespattern (not inline or staticStyleSheet.create)FlatList/FlashList(not.map()insideScrollView)Performance & Models
/vs\\)Security
Related Issues
Additional Notes