fix(models): wire the Models app download to the real backend (was a frontend mock) (#1548)#1571
Conversation
The Models app download flow was a pure frontend mock: DownloadProgress
animated a fake percentage with setInterval + Math.random and called
onDone without ever hitting the network, handleDownload just added the
model id to a Set, and handleDownloadDone fabricated a downloaded entry
that vanished the next time /api/models was refetched.
Wire it up to the real endpoints instead:
- carry a default variantId (smallest variant by size_mb, falling back
to the first) into AvailableModel from the /api/models catalog
- handleDownload now POSTs /api/models/download with {app_id,
variant_id} and records the returned download_id
- DownloadProgress now polls GET /api/models/downloads/{id} every
second and drives the bar from the real percent instead of a timer
- on status "complete" the model list is refetched from /api/models
instead of fabricating a downloaded entry, so the Downloaded Models
list reflects backend truth and survives reopening the app
- on status "error" (or a failed POST) the backend error is shown on
the download card with a Retry button
- a model with no variant (e.g. the offline MOCK_AVAILABLE fallback)
surfaces a clear "no downloadable variant" error instead of faking
success
- isDownloaded now also honours the backend's own has_downloaded_variant
verdict, since the union of controller/worker/cloud downloads is keyed
by filename/host rather than by catalog model id
Adds ModelsApp.download.test.tsx pinning #1548: mocks fetch and asserts Download issues POST /api/models/download with the correct {app_id, variant_id}, the progress bar reflects polled percent, a polled status "error" surfaces the error with a Retry button, and a polled status "complete" triggers a real /api/models refetch instead of a fabricated entry. Also covers the no-variant fallback case.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughModelsApp now uses backend download state for model downloads instead of a client-side fake timer. It adds variant/install tracking, polls download progress and errors from the API, updates the download UI accordingly, and covers the flow with new tests. ChangesBackend-driven download flow and tests
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ModelsApp
participant DownloadProgress
participant Backend
User->>ModelsApp: click Download
ModelsApp->>Backend: POST /api/models/download {app_id, variant_id}
Backend-->>ModelsApp: download_id
ModelsApp->>DownloadProgress: render with state.downloadId
loop poll interval
DownloadProgress->>Backend: GET /api/models/downloads/:downloadId
Backend-->>DownloadProgress: percent, status
DownloadProgress->>ModelsApp: onUpdate(state)
end
alt status complete
DownloadProgress->>ModelsApp: onComplete()
ModelsApp->>Backend: GET /api/models
Backend-->>ModelsApp: updated catalog
else status error
DownloadProgress->>ModelsApp: onRetry available
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| downloadId, | ||
| percent: state.percent, | ||
| status: "error", | ||
| error: "Lost contact with the download; retry to continue", |
There was a problem hiding this comment.
WARNING: A single failed poll flips the download to error and prompts a Retry, but the backend's download is almost certainly still running. The Retry click calls handleDownload(model) which POST /api/models/downloads again — if the backend doesn't deduplicate by (app_id, variant_id), this silently starts a second concurrent download and the original is left to complete (or fail) without the UI ever knowing. Consider distinguishing poll transport errors from backend errors: on a network failure, leave the in-flight downloadId in place and just retry the next tick, only escalating to error once the backend itself returns status: "error" or a definitive non-retryable HTTP status.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| const poll = async () => { | ||
| try { | ||
| const res = await fetch(`/api/models/downloads/${downloadId}`, { |
There was a problem hiding this comment.
WARNING: The poll fetch has no AbortController and the effect has no cleanup for an in-flight request. If the component unmounts (e.g. the user navigates away, or onComplete removes the entry from downloading which unmounts <DownloadProgress>), a still-pending poll() will resolve and call onUpdate/onComplete on an unmounted component, potentially triggering React's "set state on unmounted component" warning and — worse — a stale onComplete() that triggers an extra fetchModels(). Use an AbortController ref, abort in the effect cleanup, and guard the onUpdate/onComplete calls with an aborted flag.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| }; | ||
|
|
||
| const progress = Math.min(100, Math.round(pct)); | ||
| timer.current = setInterval(poll, 1000); |
There was a problem hiding this comment.
WARNING: setInterval does not await previous iterations, so two polls can be in flight at once if the network is slow. The slower poll's onUpdate will then overwrite the fresher state with stale percent/status. Also, timer.current = setInterval(poll, 1000) fires poll immediately at t=0 and at t=1000ms — the immediate poll can race with the parent state that just set downloadId. Prefer a self-rescheduling setTimeout chain that awaits poll() before scheduling the next tick.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const isDownloading = downloading.has(model.id); | ||
| const isDownloaded = | ||
| model.installed || downloaded.some((d) => d.id === model.id); | ||
| const isDownloading = model.id in downloading; |
There was a problem hiding this comment.
SUGGESTION: model.id in downloading is true for any entry — including status: "error". The Available Models card then renders the disabled Downloading... button (line 794), hiding the failure from the most prominent surface and forcing the user to scroll up to the "Downloading" section to find Retry. Differentiate the states (e.g. isDownloading && state.status !== "error", or add an isError flag) and surface the error inline on the Available card so the user can retry without hunting.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summary (commit ed379d1)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ed379d1)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Reviewed by minimax-m3 · Input: 47.9K · Output: 9.3K · Cached: 887.4K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
desktop/src/apps/ModelsApp.download.test.tsx (2)
45-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mockFetch's prefix matching is order-dependent.Routes are matched with
url.startsWith(prefix)iterated viaObject.entries(routes), and "/api/models" is a prefix of "/api/models/download", which is itself a prefix of "/api/models/downloads/". This only resolves correctly today because every test happens to declare the routes from most-specific to least-specific. Reordering keys (or adding a new endpoint under/api/models/...) would silently route requests to the wrong handler with no test failure hinting why.Consider matching on exact segments or sorting by prefix length/specificity internally so correctness doesn't depend on call-site ordering:
♻️ Suggested more robust matching
- for (const [prefix, make] of Object.entries(routes)) { + const sorted = Object.entries(routes).sort((a, b) => b[0].length - a[0].length); + for (const [prefix, make] of sorted) { if (url.startsWith(prefix)) return Promise.resolve(make()); }Also applies to: 66-71, 88-96, 112-118, 130-138
🤖 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 `@desktop/src/apps/ModelsApp.download.test.tsx` around lines 45 - 58, mockFetch currently resolves handlers by the insertion order of Object.entries(routes), so prefix matches can pick the wrong endpoint when one route is a prefix of another. Update mockFetch to choose the most specific match internally in ModelsApp.download.test.tsx, ideally by exact path/segment matching or by sorting prefixes by length before checking startsWith, so calls to /api/models, /api/models/download, and /api/models/downloads/ do not depend on the call-site order in the affected tests.
65-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert the HTTP method for the download POST.
The test verifies the request body but not
init?.method. AssertingPOSTwould catch a regression where the app accidentally issues a GET (or another verb) with a body that gets silently dropped by somefetchimplementations.✅ Suggested addition
const body = JSON.parse(call!.init!.body as string); expect(body).toEqual({ app_id: "test-model", variant_id: "q4" }); + expect(call!.init!.method).toBe("POST");🤖 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 `@desktop/src/apps/ModelsApp.download.test.tsx` around lines 65 - 84, The ModelsApp.download test currently checks the request body for /api/models/download but does not verify the HTTP verb. Update the test around the mocked fetch call in ModelsApp.download.test.tsx to assert that the recorded request for /api/models/download has init.method set to POST, alongside the existing body assertion, so regressions in the download request method are caught.
🤖 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 `@desktop/src/apps/ModelsApp.download.test.tsx`:
- Around line 45-58: mockFetch currently resolves handlers by the insertion
order of Object.entries(routes), so prefix matches can pick the wrong endpoint
when one route is a prefix of another. Update mockFetch to choose the most
specific match internally in ModelsApp.download.test.tsx, ideally by exact
path/segment matching or by sorting prefixes by length before checking
startsWith, so calls to /api/models, /api/models/download, and
/api/models/downloads/ do not depend on the call-site order in the affected
tests.
- Around line 65-84: The ModelsApp.download test currently checks the request
body for /api/models/download but does not verify the HTTP verb. Update the test
around the mocked fetch call in ModelsApp.download.test.tsx to assert that the
recorded request for /api/models/download has init.method set to POST, alongside
the existing body assertion, so regressions in the download request method are
caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 991fd0f5-832c-4f8c-92dc-fbbd44eebdd4
📒 Files selected for processing (2)
desktop/src/apps/ModelsApp.download.test.tsxdesktop/src/apps/ModelsApp.tsx
… races, error card) Fold review findings on the download wiring: - a single failed poll no longer flips a still-running backend download to error and prompts a duplicate re-download; only a real backend status of error, or 3 consecutive poll misses, ends the download - the poll fetch now uses an AbortController and a cancelled flag so an in-flight poll after unmount cannot call setState / refetch on a gone view - replaced setInterval with a self-scheduling awaited poll so a slow response can never overlap or clobber a fresher one - the Available Models card now shows an actionable Retry (not a stuck disabled Downloading...) when a download has errored Docs-Reviewed: frontend-only change plus a new test file inside the existing Models app; no desktop app or API route added or removed, so README needs no change.
1 participant
Root cause
The Models app's download flow in
desktop/src/apps/ModelsApp.tsxnever called the backend at all:DownloadProgressanimated a fake percentage withsetInterval+Math.random()and calledonDoneon a timer, with no network call.handleDownloadjust added the model id to a localdownloadingSet.handleDownloadDonepushed a fabricated entry (filename${model.id}-q4_k_m.gguf) into localdownloadedstate. Nothing was persisted, so on the next/api/modelsrefetch (e.g. reopening the app) the entry vanished, no files were ever written, and the "complete" state was purely cosmetic.The real backend endpoints already existed and worked (
POST /api/models/download,GET /api/models/downloads/{id},GET /api/modelswithvariants[].size_mb/has_downloaded_variant) — the frontend simply never called them.Fix
AvailableModelnow carries avariantId, chosen in the/api/modelsmapping as the smallest variant bysize_mb(falls back to the first variant;undefinedif a model has no variants, e.g. the offlineMOCK_AVAILABLElist).handleDownloadnow doesPOST /api/models/downloadwith{ app_id, variant_id }(credentials: "include"). A missingvariantIdor a failed/erroring response surfaces a clear message instead of pretending to succeed.DownloadProgresspollsGET /api/models/downloads/{download_id}every second and drives the bar from the realpercent.status: "complete"stops polling and refetches/api/models(no fabricated entry — the Downloaded Models list now reflects backend truth and survives reopening the app).status: "error"stops polling and shows the backend's error with a Retry button that re-invokeshandleDownload.downloadingstate changed from aSet<string>to aRecord<string, DownloadState>so each in-flight download tracks its owndownload_id,percent,status, anderror.isDownloadednow also honours the backend's ownhas_downloaded_variantverdict, since the existing union of controller/worker/cloud downloads is keyed by filename/host, not catalog model id, and would otherwise never mark a freshly-completed local download as installed.Tests
Added
desktop/src/apps/ModelsApp.download.test.tsx, mockingfetchto assert:POST /api/models/downloadwith the correct{app_id, variant_id}percentstatus: "error"surfaces the error text with a Retry buttonstatus: "complete"triggers a real/api/modelsrefetch (asserted via a second catalog fetch flippinghas_downloaded_variant), not a fabricated entryTest plan
cd desktop && npm install && npm run buildsucceedscd desktop && npx vitest run— 278 files / 2320 tests passModelsApp.download.test.tsxsuite passes (5/5)Summary by CodeRabbit