Align desktop download contract for missing model UI#1694
Align desktop download contract for missing model UI#1694benceruleanlu wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDownloads were rekeyed by a new ChangesDownload Manager lifecycle and API
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant IPC
participant Main as DownloadManager (Main)
participant Electron as BrowserWindow / DownloadItem
participant FS as FileSystem
Renderer->>IPC: START_DOWNLOAD {url, path, filename}
IPC->>Main: startDownload(...)
Main->>FS: validate/create target directory, check existing file
alt file exists
Main-->>IPC: {ok: true, download: COMPLETED DownloadState}
IPC-->>Renderer: StartDownloadResult
else new or resume
Main->>Electron: webContents.downloadURL(url)
Main->>Main: enqueue pending URL→downloadId (PENDING)
Main-->>IPC: {ok: true, download: pending DownloadState}
IPC-->>Renderer: StartDownloadResult (PENDING)
Electron->>Main: will-download (provides DownloadItem)
Main->>Main: bind item to persisted Download
loop during download
Electron->>Main: item.updated (progress/paused)
Main-->>IPC: DOWNLOAD_PROGRESS (DownloadState)
IPC-->>Renderer: DOWNLOAD_PROGRESS
end
Electron->>Main: item.done (completed/cancelled/error)
Main->>FS: rename temp→final on completed / cleanup on cancel/error
Main-->>IPC: DOWNLOAD_PROGRESS (final DownloadState)
IPC-->>Renderer: DOWNLOAD_PROGRESS
end
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
benceruleanlu
changed the title
dosubot
Bot
added
the
size:L
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 610c00ae81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| return true; |
There was a problem hiding this comment.
startDownload now keeps completed entries in downloads, but this branch returns early for any existing status other than paused/error/cancelled. After a model is deleted in the same session (e.g., via deleteModel, which does not clear downloads), calling startDownload for the same URL hits this return true path and never reaches session.defaultSession.downloadURL(url), so the download cannot be restarted until app restart.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models/DownloadManager.ts (1)
98-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't emit
COMPLETEDafter the final move fails.If
renameSyncthrows, this branch still reportsstatus: COMPLETEDwithprogress: 1, so the renderer can hydrate a model that never actually landed atsavePath.Proposed fix
if (state === 'completed') { try { fs.renameSync(download.tempPath, download.savePath); log.info(`Successfully renamed ${download.tempPath} to ${download.savePath}`); + download.item = null; + download.progress = 1; + download.status = DownloadStatus.COMPLETED; + download.message = undefined; } catch (error) { log.error('Failed to rename downloaded file. Deleting temp file.', error); fs.unlinkSync(download.tempPath); + download.item = null; + download.progress = this.calculateProgress(receivedBytes, totalBytes); + download.status = DownloadStatus.ERROR; + download.message = 'Failed to move downloaded file into place'; } - download.item = null; - download.progress = 1; - download.status = DownloadStatus.COMPLETED; - download.message = undefined; this.reportProgress(this.toDownloadState(download));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` around lines 98 - 110, The rename failure branch currently still marks the download as completed and calls reportProgress; modify the DownloadManager code so the post-success updates (setting download.item = null, download.progress = 1, download.status = DownloadStatus.COMPLETED, download.message = undefined and calling this.reportProgress(this.toDownloadState(download))) occur only inside the try after fs.renameSync succeeds; in the catch block do not set COMPLETED or call reportProgress as completed—instead set download.status to a failure state (e.g., DownloadStatus.FAILED), set download.message to the caught error message, ensure the temp file is unlinked, and call reportProgress(this.toDownloadState(download)) with the failure state so the renderer is not told the file landed at savePath (refer to symbols: DownloadManager, download, reportProgress, toDownloadState, DownloadStatus).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/DownloadManager.ts`:
- Around line 170-188: The existingCompletedDownload reuse path only updates
terminal fields and can leak stale metadata; update all location and byte fields
before storing/reporting: set existingCompletedDownload.directoryPath =
normalizedDirectoryPath, existingCompletedDownload.savePath = localSavePath,
existingCompletedDownload.tempPath = this.getTempPath(filename,
normalizedDirectoryPath), existingCompletedDownload.filename = filename, and
refresh receivedBytes and totalBytes (reset or set to current values as
appropriate) and item = null, then this.downloads.set(url,
existingCompletedDownload) and
reportProgress(this.toDownloadState(existingCompletedDownload)); apply these
changes around the existingCompletedDownload handling in DownloadManager
(references: downloads, existingCompletedDownload, getTempPath, toDownloadState,
reportProgress, DownloadStatus.COMPLETED).
---
Outside diff comments:
In `@src/models/DownloadManager.ts`:
- Around line 98-110: The rename failure branch currently still marks the
download as completed and calls reportProgress; modify the DownloadManager code
so the post-success updates (setting download.item = null, download.progress =
1, download.status = DownloadStatus.COMPLETED, download.message = undefined and
calling this.reportProgress(this.toDownloadState(download))) occur only inside
the try after fs.renameSync succeeds; in the catch block do not set COMPLETED or
call reportProgress as completed—instead set download.status to a failure state
(e.g., DownloadStatus.FAILED), set download.message to the caught error message,
ensure the temp file is unlinked, and call
reportProgress(this.toDownloadState(download)) with the failure state so the
renderer is not told the file landed at savePath (refer to symbols:
DownloadManager, download, reportProgress, toDownloadState, DownloadStatus).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5470701d-950e-4182-8ecc-74b38b584fe2
📒 Files selected for processing (3)
src/models/DownloadManager.tssrc/preload.tstests/unit/models/DownloadManager.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models/DownloadManager.ts (1)
107-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename failure is reported as
COMPLETED.If temp→final rename fails, the code still sets
status = COMPLETEDandprogress = 1. That reports a successful terminal state even though persistence failed.Suggested fix
if (state === 'completed') { try { fs.renameSync(download.tempPath, download.savePath); log.info(`Successfully renamed ${download.tempPath} to ${download.savePath}`); } catch (error) { log.error('Failed to rename downloaded file. Deleting temp file.', error); - fs.unlinkSync(download.tempPath); + try { + fs.unlinkSync(download.tempPath); + } catch (unlinkError) { + log.error('Failed to delete temp file after rename failure', unlinkError); + } + download.item = null; + download.progress = this.calculateProgress(receivedBytes, totalBytes); + download.status = DownloadStatus.ERROR; + download.message = 'Failed to finalize downloaded file'; + this.reportProgress(this.toDownloadState(download)); + return; } download.item = null; download.progress = 1; download.status = DownloadStatus.COMPLETED;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` around lines 107 - 118, The rename failure handling currently still marks the download as completed; update the catch block around fs.renameSync(download.tempPath, download.savePath) so that on error you set download.status to a non-completed terminal state (e.g., DownloadStatus.FAILED), set download.progress appropriately (do not set to 1), populate download.message with the error message, ensure the temp file is removed (fs.unlinkSync) only after handling the error, and call this.reportProgress(this.toDownloadState(download)) to report the failure; keep the success path unchanged and only set download.item = null and download.status = DownloadStatus.COMPLETED when renameSync succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/DownloadManager.ts`:
- Around line 219-222: The paused-restart path in startDownload returns the
stale existingDownload snapshot after calling resumeDownload; change
startDownload to await the outcome of resumeDownload (or retrieve the
new/updated download record after resume) and return this fresh state via
toDownloadState so the caller gets the current pending/download entry (do the
same fix for the analogous block that handles paused items around the later
branch). Ensure you use the resumeDownload result or re-fetch the download by
downloadId rather than returning existingDownload directly, and preserve error
handling if resume fails.
In `@tests/integration/post-install/downloadManager.spec.ts`:
- Around line 79-81: The test fails cross-platform because downloadId is
canonicalized (lowercased on Windows); update the assertion to normalize both
sides the same way before comparing: transform expectedFilePath the same way
startResult.download.downloadId is canonicalized (e.g., path.normalize and
lowercasing on Windows) and then assert equality against
startResult.download.downloadId (or compare both lowercased). Reference
startResult.download.downloadId and expectedFilePath when making the change so
the test compares canonicalized values instead of raw strings.
---
Outside diff comments:
In `@src/models/DownloadManager.ts`:
- Around line 107-118: The rename failure handling currently still marks the
download as completed; update the catch block around
fs.renameSync(download.tempPath, download.savePath) so that on error you set
download.status to a non-completed terminal state (e.g., DownloadStatus.FAILED),
set download.progress appropriately (do not set to 1), populate download.message
with the error message, ensure the temp file is removed (fs.unlinkSync) only
after handling the error, and call
this.reportProgress(this.toDownloadState(download)) to report the failure; keep
the success path unchanged and only set download.item = null and download.status
= DownloadStatus.COMPLETED when renameSync succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c5b89d1-50ab-4e73-bbab-e3aa5bdcb543
📒 Files selected for processing (6)
src/infrastructure/ipcChannels.tssrc/main_types.tssrc/models/DownloadManager.tssrc/preload.tstests/integration/post-install/downloadManager.spec.tstests/unit/models/DownloadManager.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/main_types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/preload.ts
- tests/unit/models/DownloadManager.test.ts
| expect(startResult.ok).toBe(true); | ||
| expect(startResult.download.downloadId).toBe(expectedFilePath); | ||
| expect(startResult.download.savePath).toBe(expectedFilePath); |
There was a problem hiding this comment.
Normalize expected downloadId before asserting.
downloadId is canonicalized (lowercased on Windows), so a direct equality with expectedFilePath can fail cross-platform even when behavior is correct.
Suggested fix
+ const expectedDownloadId = process.platform === 'win32' ? expectedFilePath.toLowerCase() : expectedFilePath;
expect(startResult.ok).toBe(true);
- expect(startResult.download.downloadId).toBe(expectedFilePath);
+ expect(startResult.download.downloadId).toBe(expectedDownloadId);
expect(startResult.download.savePath).toBe(expectedFilePath);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(startResult.ok).toBe(true); | |
| expect(startResult.download.downloadId).toBe(expectedFilePath); | |
| expect(startResult.download.savePath).toBe(expectedFilePath); | |
| const expectedDownloadId = process.platform === 'win32' ? expectedFilePath.toLowerCase() : expectedFilePath; | |
| expect(startResult.ok).toBe(true); | |
| expect(startResult.download.downloadId).toBe(expectedDownloadId); | |
| expect(startResult.download.savePath).toBe(expectedFilePath); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/post-install/downloadManager.spec.ts` around lines 79 - 81,
The test fails cross-platform because downloadId is canonicalized (lowercased on
Windows); update the assertion to normalize both sides the same way before
comparing: transform expectedFilePath the same way
startResult.download.downloadId is canonicalized (e.g., path.normalize and
lowercasing on Windows) and then assert equality against
startResult.download.downloadId (or compare both lowercased). Reference
startResult.download.downloadId and expectedFilePath when making the change so
the test compares canonicalized values instead of raw strings.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models/DownloadManager.ts (1)
106-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename failure is reported as COMPLETED
If
renameSyncfails, the code deletes temp data but still setsstatus = COMPLETEDandprogress = 1. That reports success even when finalization failed.Suggested fix
if (state === 'completed') { try { fs.renameSync(download.tempPath, download.savePath); log.info(`Successfully renamed ${download.tempPath} to ${download.savePath}`); } catch (error) { log.error('Failed to rename downloaded file. Deleting temp file.', error); - fs.unlinkSync(download.tempPath); + this.deleteTempFile(download.tempPath); + download.item = null; + download.progress = this.calculateProgress(receivedBytes, totalBytes); + download.status = DownloadStatus.ERROR; + download.message = 'Failed to finalize downloaded file'; + this.reportProgress(this.toDownloadState(download)); + return; } download.item = null; download.progress = 1; download.status = DownloadStatus.COMPLETED;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` around lines 106 - 118, The catch block in DownloadManager (where fs.renameSync(download.tempPath, download.savePath) is attempted) currently deletes the temp file but still sets download.progress = 1 and download.status = DownloadStatus.COMPLETED and reports success; change the catch to mark the download as failed instead: set download.status = DownloadStatus.FAILED, set download.progress to a value < 1 (e.g., 0 or leave current progress), populate download.message with the caught error message, do not set download.item to null/clear successful fields, ensure fs.unlinkSync(download.tempPath) still runs safely, and call this.reportProgress(this.toDownloadState(download)) from inside the catch so the failure is reported correctly (and remove/avoid the post-try success assignments when an exception occurred).
♻️ Duplicate comments (2)
src/models/DownloadManager.ts (2)
192-210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReused completed entries should refresh full metadata before reporting
When an existing object is reused, only terminal fields are reset. Metadata like
url, paths, filename, and byte counters can remain stale in emitted state.Suggested fix
const existingCompletedDownload = this.downloads.get(downloadId) ?? { downloadId, url, directoryPath: normalizedDirectoryPath, savePath: localSavePath, tempPath: this.getTempPath(filename, normalizedDirectoryPath), filename, item: null, progress: 1, status: DownloadStatus.COMPLETED, message: undefined, receivedBytes: 0, totalBytes: 0, }; + existingCompletedDownload.url = url; + existingCompletedDownload.directoryPath = normalizedDirectoryPath; + existingCompletedDownload.savePath = localSavePath; + existingCompletedDownload.tempPath = this.getTempPath(filename, normalizedDirectoryPath); + existingCompletedDownload.filename = filename; existingCompletedDownload.progress = 1; existingCompletedDownload.status = DownloadStatus.COMPLETED; existingCompletedDownload.message = undefined; existingCompletedDownload.item = null; + existingCompletedDownload.receivedBytes = 0; + existingCompletedDownload.totalBytes = 0; this.downloads.set(downloadId, existingCompletedDownload);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` around lines 192 - 210, The reuse of existingCompletedDownload reuses an old object but only resets terminal fields; update the block that constructs/updates existingCompletedDownload (the object created when reading this.downloads.get(downloadId)) to refresh all metadata fields — set url, directoryPath (normalizedDirectoryPath), savePath (localSavePath), tempPath (use this.getTempPath(filename, normalizedDirectoryPath)), filename, receivedBytes and totalBytes — before forcing progress=1, status=DownloadStatus.COMPLETED, message=undefined and item=null, then write it back with this.downloads.set(downloadId, existingCompletedDownload).
219-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPaused re-start can still return a stale row snapshot
This path calls
resumeDownloadand immediately returnsexistingDownload. If resume cannot continue andresumeDownloadrestarts internally, the returned snapshot can point to the pre-restart object.Suggested fix
- if (existingDownload.status === DownloadStatus.PAUSED) { - this.resumeDownload(downloadId); - return { ok: true, download: this.toDownloadState(existingDownload) }; + if (existingDownload.status === DownloadStatus.PAUSED) { + if (existingDownload.item?.canResume()) { + this.resumeDownload(downloadId); + return { ok: true, download: this.toDownloadState(existingDownload) }; + } + this.downloads.delete(downloadId); + return this.startDownload(url, normalizedDirectoryPath, filename); } else if (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` around lines 219 - 221, When handling a PAUSED download in the DownloadManager, calling this.resumeDownload(downloadId) then immediately returning the previously captured existingDownload can return a stale snapshot if resumeDownload mutates or restarts the download; update the code to await the result/state after the resume operation and build the response from the current download object instead of the old existingDownload (e.g., use the value returned by resumeDownload or re-fetch the download by id) so the returned this.toDownloadState(...) reflects the post-resume state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/DownloadManager.ts`:
- Around line 363-373: takePendingDownload currently falls back to any download
with item === null which can match cancelled/completed/error rows; update the
fallback so it only returns truly pending entries. In takePendingDownload, keep
the initial pendingDownloadIdsByUrl lookup, but change the fallback
[...this.downloads.values()].find(...) to also verify the download is in a
pending state (e.g. download.status === 'pending' or download.isPending ===
true) or that its id exists in pendingDownloadIdsByUrl for the URL, instead of
just checking download.item === null; ensure you reference the existing symbols
download.item, this.pendingDownloadIdsByUrl and this.downloads and avoid
returning downloads whose status is cancelled/completed/error.
---
Outside diff comments:
In `@src/models/DownloadManager.ts`:
- Around line 106-118: The catch block in DownloadManager (where
fs.renameSync(download.tempPath, download.savePath) is attempted) currently
deletes the temp file but still sets download.progress = 1 and download.status =
DownloadStatus.COMPLETED and reports success; change the catch to mark the
download as failed instead: set download.status = DownloadStatus.FAILED, set
download.progress to a value < 1 (e.g., 0 or leave current progress), populate
download.message with the caught error message, do not set download.item to
null/clear successful fields, ensure fs.unlinkSync(download.tempPath) still runs
safely, and call this.reportProgress(this.toDownloadState(download)) from inside
the catch so the failure is reported correctly (and remove/avoid the post-try
success assignments when an exception occurred).
---
Duplicate comments:
In `@src/models/DownloadManager.ts`:
- Around line 192-210: The reuse of existingCompletedDownload reuses an old
object but only resets terminal fields; update the block that constructs/updates
existingCompletedDownload (the object created when reading
this.downloads.get(downloadId)) to refresh all metadata fields — set url,
directoryPath (normalizedDirectoryPath), savePath (localSavePath), tempPath (use
this.getTempPath(filename, normalizedDirectoryPath)), filename, receivedBytes
and totalBytes — before forcing progress=1, status=DownloadStatus.COMPLETED,
message=undefined and item=null, then write it back with
this.downloads.set(downloadId, existingCompletedDownload).
- Around line 219-221: When handling a PAUSED download in the DownloadManager,
calling this.resumeDownload(downloadId) then immediately returning the
previously captured existingDownload can return a stale snapshot if
resumeDownload mutates or restarts the download; update the code to await the
result/state after the resume operation and build the response from the current
download object instead of the old existingDownload (e.g., use the value
returned by resumeDownload or re-fetch the download by id) so the returned
this.toDownloadState(...) reflects the post-resume state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55113226-c529-4139-ac5a-37c10eefd0d6
📒 Files selected for processing (1)
src/models/DownloadManager.ts
|
I had amp code do a quick review, found 4 things it thinks should be changed. Seems pretty applicable. Everything below here is not written by me and is amp taking the wheel. What it doesReplaces URL-as-identity with a desktop-issued VerdictSolid direction, but worth tightening before merge. The path-identity refactor is good, but the URL fallback + pending-queue + Windows path casing have correctness cliffs now that multiple rows can share a URL. Must-fix before merge
Should-fix
Nits
Things done right
The core refactor is worth landing — fixing items 1–4 closes the real correctness gaps without enlarging the design. |
For the remaining items, I’m intentionally not changing them in this PR:
|
2 participants
Summary
downloadIdto everyDownloadState, using the final normalized save path as lifecycle identity{ ok, download, error? }fromstartDownload()so the renderer can track the exact row immediatelyDownloadManagerbydownloadId, with FIFO URL correlation for Electronwill-downloadand URL fallback for legacy pause/resume/cancel callersstate,receivedBytes,totalBytes, andisPausedfields for existing consumersContext
Desktop missing-model downloads were starting successfully, but the frontend could not reliably reflect lifecycle state because URL was doing two jobs: source metadata and lifecycle identity. That breaks when multiple missing-model rows share a source URL or when the renderer needs to bind a started download to one specific target file. This makes desktop authoritative for download identity and state while preserving the old fields and URL fallback for bridge compatibility.
Notes
corepack yarn test:unit tests/unit/models/DownloadManager.test.ts,corepack yarn typecheck,corepack yarn typecheck:build,corepack yarn lint:check,corepack yarn format:checkSummary by CodeRabbit
Improvements
Tests