Final download refactor#322
Merged
dishit-wednesday merged 37 commits intotestv/0.0.90from Apr 27, 2026
Merged
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
…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
- Eye/repair button only shows for actual vision models (was showing for any model missing mmProjPath, including plain text models). - Repair button now gives a useful message when the upstream repo doesn't publish a separate mmproj file (e.g. mradermacher i1 quants), instead of a generic "Could not find vision projection file" error. - Text-model completion now removes the store entry after the model is registered. Prevents the UI staying at 100% with both progress bar and "downloaded" state until manual refresh. - Hydration filters out completed/failed rows so terminal states from prior sessions don't resurrect into the active store. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…air signals - Add lastProgressAt to DownloadEntry; stamp on every progress event. - Surface "stalled" state when no progress for >30s on a running/pending download. Card label changes to "Looks stuck — try restart" in warning color. Addresses the GH #320 "stops at 90%" failure mode where a worker is alive but no progress is moving. - Always show a Restart button on active text downloads (not only on failed ones). Reuses existing executeRetryDownload plumbing. - Vision repair: use a name heuristic in addition to isVisionModel, since storage.ts collapses "is vision" with "has mmproj on disk". Eye icon now stays visible on broken vision models like SmolVLM/VL. - Chat "Vision Not Supported" alert points to Download Manager (where the eye icon actually lives) and routes there directly instead of relying on a repairModelId lookup against RECOMMENDED_MODELS. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rwriting Foreground-resume hydration used to call setAll() which replaced every store entry, blowing away lastProgressAt and any progress that JS listeners had advanced past the native snapshot in SQLite. Add a hydrate() action that merges: if a local entry's bytesDownloaded is at-or-ahead of the native row, keep the local entry (just refresh total bytes and metadataJson). Otherwise take the native snapshot. Eliminates a real foreground-resume race where an active download would briefly look "stuck" right after resume because lastProgressAt got reset to Date.now() at hydration. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… mmproj-tolerant flow Completes the unified download-store migration so the product rules hold end-to-end across restart and rapid double-tap. Persist transient states across restart - DownloadStatus enum gains RETRYING and WAITING_FOR_NETWORK; the worker writes those to Room when it returns Result.retry() (network exception or 5xx/429), and the WorkInfo observer writes WAITING_FOR_NETWORK when WorkManager parks the worker on the NetworkType.CONNECTED constraint. - toUiState maps the new enum values back to the matching UI strings. - hydrateDownloadStore restores 'retrying' and 'waiting_for_network' exactly instead of collapsing them to plain 'pending'. Strict one-entry-per-modelKey - downloadStore.add() refuses if any entry exists for the modelKey, active or otherwise. Prevents a fresh start path from silently replacing a visible failed entry. - modelManager.startBgDownload checks for an existing entry and calls retryEntry(modelKey, newDownloadId) when present, or add() only when truly new. - useTextModels.handleDownload no longer adds to the store directly — modelManager owns store population. - ModelDownloadScreen migrated off appStore.downloadProgress / local downloadIds refs onto the store. mmproj-tolerant completion - setStatus on a sidecar id only mutates mmProjStatus; the parent's status is left alone. - modelManager error listener for the mmproj clears mmProjLocalPath, marks the sidecar complete from the finalizer's perspective, and surfaces the failure to the store so the eye/repair affordance shows. Hydration preserves in-flight progress - Add hydrate() action that merges instead of replacing. - Filter rule loosened: only cancelled and completed are dropped. Failed/retrying/pending/waiting_for_network all survive restart. HTTP 429 explicitly retryable - Native: classified with 5xx as transient. http_429 reason code added on both Kotlin and TS sides. Vision-icon fixes - needsVisionRepair uses a name heuristic in addition to isVisionModel so the eye stays visible on broken vision models. - Repair "no mmproj in repo" case shows a clearer message. - Chat "Vision Not Supported" alert routes to Download Manager. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…code deleted Phase 1: cleanup - Delete dead buildDownloadItems and DownloadItemsData from DownloadManagerScreen/items.tsx. The new useDownloadManager builds items directly from useDownloadStore + downloadedModels; the legacy helper had no caller. Phase 2: UI single-source-of-truth for image models - ImageModelCardItem subscribes to useDownloadStore via the stable image:<id> modelKey. Drilled imageModelDownloading and imageModelProgress props are gone from the tab, the scroll content, and the props pick on ModelsScreenViewModel. - useModelsScreen no longer threads image.imageModelDownloading / image.imageModelProgress through; ModelsScreen/index.tsx no longer passes them as props. - StorageSettingsScreen stale-downloads section now derives from useDownloadStore entries (filtered for missing fields) and uses store.remove(modelKey) for clearing, replacing the appStore.activeBackgroundDownloads tuple model. The write side of the image pipeline (imageDownloadActions, useImageModels, restore-on-resume) still writes to appStore - that is phase 3. After this commit, screens that display download state are all on the store; only writes remain to consolidate.
Phase 3: Image download write path on the unified store
- imageDownloadActions rewritten: every flow (zip, HF multifile, CoreML
multifile) now writes to useDownloadStore via the stable image:<id>
modelKey. metadataJson on the entry carries imageDownloadType, sizes,
backend, and tokenizer hints so the entry is self-describing.
- Multi-file flows use a synthetic image-multi:<id> downloadId because
they do not go through WorkManager. Progress updates land on the
store via updateProgress; cancel = store.remove.
- Zip flow goes through native WorkManager with modelKey + modelType +
metadataJson set so hydration can rebuild the entry on restart.
- ImageDownloadDeps shrunk to the four things actually needed
(addDownloadedImageModel, activeImageModelId, setActiveImageModelId,
setAlertState, triedImageGen). All the appStore writers are gone.
- registerAndNotify is the single completion path for all three flows.
Phase 3: useImageModels simplified
- Drops imageModelDownloading[], imageModelProgress{},
imageModelDownloadIds{}, setBackgroundDownload, setDownloadProgress,
the 3-branch restoreActiveImageDownloads, the syncSharedProgress
bridge, and the makeDeps factory.
- handleCancelImageDownload reads the store entry and removes it
there; native cancel is a no-op for synthetic multifile ids.
- Mount-only restoration is gone - hydrateDownloadStore at app root
rebuilds image entries from native rows.
Phase 4: Image rows hydrate properly
- Native zip downloads carry modelType: image, modelKey: image:<id>,
and metadataJson, so a kill-and-restart during a zip download brings
back the entry visible in UI.
- Multi-file downloads do not survive kill (no WorkManager backing) -
documented gap; user retries from the model card. Phase 4 close
enough; full multifile-survives-kill is a separate native lift.
Phase 5: Old appStore download fields deleted
- downloadProgress / setDownloadProgress
- activeBackgroundDownloads / setBackgroundDownload /
clearBackgroundDownloads
- imageModelDownloading[] / addImageModelDownloading /
removeImageModelDownloading / clearImageModelDownloading
- imageModelDownloadIds{} / setImageModelDownloadId
- partialize no longer persists any of the above; migrate function
drops them on rehydrate so existing users do not crash.
Phase 5: App.tsx cleaned up
- Drops setBackgroundDownloadMetadataCallback wiring,
syncBackgroundDownloads recovery, syncCompletedImageDownloads
recovery, and clearImageModelDownloading. hydrateDownloadStore
(cold start + foreground resume) now owns the in-flight recovery
story, and refreshModelLists scans disk for completed-while-killed.
Phase 5: Cleanup in DownloadManagerScreen
- useDownloadManager no longer pulls removeImageModelDownloading.
- entryToActiveItem reads metadataJson for image entries so the active
card shows imageModelName instead of '<id>.zip', and the right
backend label (Core ML / NPU / GPU).
BackgroundDownloadMetadataCallback type retained as a deprecated
stub so legacy modelManager methods (still called from a few tests
and the legacy restore path) keep compiling without runtime effect.
…hitecture Remove tests that relied on dead appStore patterns (downloadProgress, activeBackgroundDownloads, setBackgroundDownload) and rewrite/add tests for the new SQLite → useDownloadStore → UI flow. - Delete restoreImageDownloads.test.ts (tested removed function) - Strip dead appStore download state from appStore, ModelsScreen, StorageSettingsScreen, backgroundDownloadService, and modelManager tests - Rewrite DownloadManagerScreen tests to populate mockDownloadStoreDownloads instead of old appStore fields; remove tests for event callbacks and cross-reference logic that moved to App.tsx - Add useDownloadStore mock to StorageSettingsScreen tests for stale-entry detection - Add unit tests for downloadStore (add, hydrate, updateProgress, setStatus, retryEntry, remove, mmproj paths) and downloadHydration (isMmProjFileName, hydrateDownloadStore branches) to bring branch coverage above 80%
![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 implements a major refactor of the download system, introducing a unified useDownloadStore backed by a native SQLite database on Android. Key changes include the migration of download IDs from timestamps to UUIDs, the introduction of stable ModelKey identifiers to prevent UI flicker during retries, and the consolidation of download event handling into a global useDownloads hook. Feedback highlights critical issues in the startup recovery flow, specifically the need to re-attach watchDownload listeners and include completed rows during hydration to ensure downloads finished while the app was closed are correctly finalized. Additionally, improvements were suggested for consistent status checking using the isActiveStatus helper and aligning the DownloadEntry interface with the architecture documentation by adding the missing lastProgressAt field for stuck detection.
| await ensureAppStoreHydrated(); | ||
|
|
||
| // Hydrate download store from SQLite before any screen mounts. | ||
| hydrateDownloadStore().catch(() => {}); |
Contributor
There was a problem hiding this comment.
The refactor removes the logic that re-attaches watchDownload listeners and finalizes completed downloads on app startup. While hydrateDownloadStore repopulates the UI state in the Zustand store, it does not trigger the necessary file moves or model registrations for downloads that were in-progress or finished while the app was closed.
Without calling modelManager.restoreInProgressDownloads (which is still implemented in restore.ts but no longer invoked), text model downloads will remain stuck in the store at 100% and will never be moved from the temporary downloads directory to the final models directory.
Collaborator
Author
There was a problem hiding this comment.
Addressed. reattachTextDownloadRecovery() (App.tsx line 50–68) calls modelManager.restoreInProgressDownloads() followed by modelManager.watchDownload() for each restored ID. This re-attaches the complete/error listeners and triggers finalization (file move + model registration) for any download that was in-progress or finished while the app was dead. It runs on both cold start (line 101) and every foreground resume (line 80).
| !mmProjIds.has(r.downloadId) && | ||
| !isMmProjFileName(r.fileName) && | ||
| r.status !== 'cancelled' && | ||
| r.status !== 'completed', |
Contributor
There was a problem hiding this comment.
Filtering out completed rows here prevents the recovery of downloads that finished while the app was in the background or closed. These rows represent files that are fully downloaded but haven't been moved to their final destination or registered in the app store yet. They should be included in hydration so that the finalization logic (once re-attached) can process them.
Suggested change
| r.status !== 'completed', | |
| r.status !== 'cancelled', |
Collaborator
Author
There was a problem hiding this comment.
Intentional and correct. Completed rows are excluded from hydration deliberately — they should not flash in the download list UI. Instead, they are finalized via the restore path: restoreInProgressDownloads includes 'completed' in isRestorable (restore.ts line 27), sets mainCompleted: true in the background context, and then watchBackgroundDownload calls tryFinalize() immediately — which moves the file to its final destination and registers the model. The end result is identical to the Gemini suggestion, just without the UI flicker of a completed entry appearing and then disappearing.
Comment on lines
+176
to
+179
| const activeDownloadCount = useDownloadStore(state => | ||
| Object.values(state.downloads).filter( | ||
| d => d.status === 'pending' || d.status === 'running', | ||
| ).length, |
Contributor
There was a problem hiding this comment.
The activeDownloadCount calculation should use the isActiveStatus helper from downloadStore.ts. This ensures it correctly accounts for all non-terminal states (like retrying, waiting_for_network, or processing) when determining the badge count and enforcing the concurrency guard.
Suggested change
| const activeDownloadCount = useDownloadStore(state => | |
| Object.values(state.downloads).filter( | |
| d => d.status === 'pending' || d.status === 'running', | |
| ).length, | |
| const activeDownloadCount = useDownloadStore(state => | |
| Object.values(state.downloads).filter( | |
| d => isActiveStatus(d.status), | |
| ).length, | |
| ); |
Collaborator
Author
There was a problem hiding this comment.
Already addressed. activeDownloadCount at line 176–179 already uses isActiveStatus(d.status) from downloadStore.ts, which covers all non-terminal states including retrying, waiting_for_network, and processing.
Comment on lines
+16
to
+34
| export interface DownloadEntry { | ||
| modelKey: ModelKey | ||
| downloadId: string | ||
| modelId: string | ||
| fileName: string | ||
| quantization: string | ||
| modelType: ModelType | ||
| status: DownloadStatus | ||
| bytesDownloaded: number | ||
| totalBytes: number | ||
| combinedTotalBytes: number | ||
| progress: number | ||
| mmProjDownloadId?: string | ||
| mmProjBytesDownloaded?: number | ||
| mmProjStatus?: DownloadStatus | ||
| errorMessage?: string | ||
| createdAt: number | ||
| metadataJson?: string | ||
| } |
Contributor
There was a problem hiding this comment.
The DownloadEntry interface is missing the lastProgressAt field mentioned in the architecture documentation (DOWNLOAD_ARCHITECTURE_V3.md). This field is required for the "stuck detection" logic to function correctly. It should be initialized in add and updated in updateProgress and updateMmProjProgress.
Collaborator
Author
There was a problem hiding this comment.
lastProgressAt was part of a stuck-detection commit that was reverted during this PR's development. It is referenced in docs/DOWNLOAD_ARCHITECTURE_V3.md (lines 99 and 271) but is not present in the current implementation. The architecture doc is stale on this point. We will update the doc to remove the lastProgressAt reference and the stuck-detection section so it reflects the actual current design.
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (53.90%) 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 @@
## testv/0.0.90 #322 +/- ##
================================================
- Coverage 83.83% 82.53% -1.30%
================================================
Files 223 229 +6
Lines 11460 11457 -3
Branches 3144 3160 +16
================================================
- Hits 9607 9456 -151
- Misses 1070 1187 +117
- Partials 783 814 +31
🚀 New features to boost your workflow:
|
- restore.ts: extract collectMmProjIds helper, reduce cognitive complexity - downloadStore.ts: convert ACTIVE_STATUSES array to Set; extract nested ternary - downloadHydration.ts: remove unnecessary type assertions - ImageModelsTab.tsx: flip negated condition to positive form - DownloadManagerModule.kt: use check() idiom; document empty catch block - appStore.test.ts: remove redundant as-any on already-any variables - imageDownloadActions.test.ts: remove unused imports
dishit-wednesday
force-pushed
the
final-download-refactor
branch
from
f8490d9 to
c3e49d1
Compare
- imageDownloadActions.ts: refactor downloadCoreMLMultiFile to reuse downloadSequentialFiles, ensureDirectory, cleanupImageModelDir; reduces cognitive complexity from 21 to under 15 - downloadHydration.ts: remove redundant as-ModelType assertion - DownloadManagerModule.kt: handle boolean return value of File.delete() - modelManager/index.ts: extract mmProjFile local to remove non-null assertion - imageDownloadActions.test.ts: spread undefined instead of empty object fallback - appStore.test.ts: NOSONAR on two assertions that tsc requires
- imageDownloadActions.ts: use === undefined instead of !promise to avoid Promise-in-boolean-conditional reliability issue (blocks quality gate) - downloadHydration.ts: replace filter+map+! with flatMap to eliminate unnecessary non-null assertion on mmProjDownloadId
…re doc The stuck-detection feature (lastProgressAt field, stallTick, isStalled) was reverted during development. Remove references from the architecture doc to keep it in sync with the actual implementation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on restart
- serialize download-store hydration before text download reattachment on startup/foreground
- replace silent hydration catches with logged errors
- add per-row isolation in download hydration so one bad native row does not break the whole Download
Manager
- add per-entry isolation in restore so one malformed in-progress download does not abort all recovery
- guard downloaded model list JSON parsing against corrupt AsyncStorage state
- keep raw mmproj/projector/clip sidecar downloads hidden during hydration, including after app kill/
restart
- add repairing state UX for vision repair on Models and Download Manager screens
- show spinner/disabled repair action and Repairing... badge while mmproj repair is in flight
- fail Android downloads with significant final size mismatch when no expected SHA is available, instead
of marking them completed
…ge and mmproj names
… update interval to 1500ms
…lure Network errors, 5xx, and 429 responses now fail immediately with a visible error state and a retry button instead of silently looping through WorkManager's exponential backoff. Tapping retry re-enqueues the same native download row, preserving the partial file so OkHttp's Range header resumes from the last byte. Key changes: - WorkerDownload: handleDownloadException and 5xx/429 path now call failDownload() instead of Result.retry(). OS-killed worker still retries silently via handleStoppedState (unchanged). - DownloadManagerModule: new retryDownload() React method; enqueue runs before observeForever to avoid replaying the stale FAILED WorkInfo on the new observer; DB rolled back to FAILED in catch so hydration after restart does not produce a zombie pending row. - startProgressPolling: containsKey guard prevents re-registering already-watched downloads, eliminating mid-session replay bursts. - RUNNING observer: fileName/modelId cached at registration time, removing a Room read on every progress event. - Old RETRYING DB rows are remapped to failed with a retriable reason code so existing stuck downloads surface the retry button on upgrade. - download.ts: cancels the old native worker before retryEntry swaps the downloadId, preventing an orphaned worker with no store listener. - UI: retry button shown on Android for retriable reason codes; isRetryable() helper classifies network/server errors as retriable vs permanent failures.
…d fix lifecycle gaps - Call hydrateDownloadStore on app mount (AppNavigator) so active downloads survive app restart instead of vanishing from UI - Keep COMPLETED image rows during hydration and map them to 'processing' so interrupted finalization (unzip/register) is detected after restart - Add resumeImageDownload to re-run move+unzip+register for processing image entries found on mount, covering all three recovery cases (already unzipped, zip moved, zip still in WorkManager temp) - Wire restoreProcessingImageDownloads in useImageModels mount effect so recovery runs automatically when the models screen loads - Add disk-existence guard in proceedWithDownload so already-downloaded image models register directly instead of re-downloading - Fix wireZipListeners error handler to set failed status directly instead of relying on unmounted useDownloads hook - Fix canCancel in TextModelsTab to use isActiveStatus covering retrying, waiting_for_network and processing states
- Call hydrateDownloadStore on app mount so in-progress downloads survive app kills and reappear correctly on restart - Keep completed image rows during hydration (previously filtered out), surfacing them as 'processing' so JS finalization can re-run - Add resumeImageDownload to re-finalize processing entries on restart, handling all three recovery states: already unzipped, zip in app dir, zip still in WorkManager temp location - Add disk-existence guard in proceedWithDownload to skip re-download when model files already exist on disk - Fix wireZipListeners error handler to set failed status in store so the entry stays visible for user action - Fix canCancel in TextModelsTab to use isActiveStatus instead of only checking pending/running, covering retrying/waiting_for_network states - Remove debug panel from DownloadManagerScreen - Extract resumeImageDownload into imageDownloadResume.ts to stay under max-lines limit; refactor resolveMmProj helper to reduce complexity - Add timing logs around moveCompletedDownload and unzip for diagnosis
|
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.
This branch completes the download-state refactor and consolidates model download UI around the
unified Zustand download store.
The main outcome is that text and image downloads now follow the same store-backed lifecycle
for active UI state, hydration, cancel/remove, failure visibility, and completed/downloaded
transitions. The old fragmented progress state has been reduced significantly, and the Download
Manager plus model screens now read from the same core state model.
Major Changes
Behavioral Result
Validation
Focused tests were updated and passed for the touched areas: