Open
Conversation
Contributor
Build artifacts for all platforms are ready! 🚀Download the artifacts for: One-line installer (Unstable):bunx flow-debug-build@1.2.1 --open 23387308049(execution 23387308049 / attempt 1) |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Chrome-like download pipeline: new download handler creating Changes
Sequence DiagramsequenceDiagram
participant User
participant Electron
participant Handler as DownloadHandler
participant Dialog as SaveDialog
participant FS as FileSystem
participant macOS as macOSProgress
User->>Electron: start download
Electron->>Handler: emit "will-download"
Handler->>Handler: set `.crdownload`, pause/resume, track metadata
Handler->>Dialog: open save dialog (async)
Dialog-->>Handler: final path or cancel
Handler->>Electron: resume download
loop downloading
Electron->>Handler: "updated" (progressing)
Handler->>FS: mirror temp -> visible path (rename/link/symlink/fallback)
Handler->>macOS: create/update progress (bytes, throughput, ETA)
end
Electron->>Handler: "done"
alt completed
Handler->>macOS: complete progress
Handler->>FS: move temp to final, clean mirrors
else cancelled
Handler->>macOS: cancel progress
Handler->>FS: remove mirrors, delete partial
else interrupted
Handler->>macOS: cancel progress
Handler->>FS: remove visible mirror only
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR significantly improves the download experience by introducing Chrome-style The previous review concerns around cross-device rename failures, early completions, and the async module-load race have all been addressed in this revision. One new race condition remains:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant E as Electron (will-download)
participant H as handleDownload
participant D as Save Dialog
participant FS as Filesystem
participant MP as NSProgress (macOS)
E->>H: will-download event
H->>H: setSavePath(crdownloadPath)
H->>H: create DownloadMetadata (saveConfirmed=false)
H-->>E: return (sync)
par Async dialog + progress setup
H->>MP: createFileProgress(crdownloadPath)
MP-->>H: progressId
H->>D: showSaveDialog(defaultPath)
and Download bytes flowing
E->>H: updated(progressing)
H->>MP: updateFileProgress(bytes)
end
alt User confirms save location
D-->>H: { filePath, canceled:false }
H->>H: saveConfirmed=true, finalPath=chosenPath
H->>FS: moveCrdownloadToFinalDir() [fire-and-forget ⚠️]
Note over H,FS: race: done may fire before move completes
E->>H: done(completed)
H->>FS: cleanupSecondaryPath (mirrorKind may be undefined ⚠️)
H->>FS: moveTempToFinal(crdownloadPath → finalPath)
H->>MP: completeFileProgress(progressId)
else User cancels dialog
D-->>H: { canceled:true }
H->>H: saveRejected=true
H->>E: item.cancel()
E->>H: done(cancelled)
H->>FS: deleteFile(crdownloadPath)
H->>MP: cancelFileProgress(progressId)
end
Last reviewed commit: "fix: codex discovere..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/main/controllers/tabs-controller/tab.ts`:
- Around line 504-518: The code unsafely casts constructorOptions to
Electron.WebContentsViewConstructorOptions (via viewOptions) though createWindow
receives BrowserWindowConstructorOptions; instead of the imprecise cast, extract
only the properties you use (webContents and webPreferences) from
constructorOptions and build a small object (e.g., { webContents, webPreferences
}) used where viewOptions is referenced so needsManualLoad =
!extracted.webContents and the this.emit("new-tab-requested",
handlerDetails.url, handlerDetails.disposition, extracted, handlerDetails, {
noLoadURL: !needsManualLoad }) call receives a correctly-shaped, type-safe
object.
In `@src/main/modules/downloads-handler.ts`:
- Around line 90-168: The promise returned by dialog.showSaveDialog(...) (the
chain starting at dialog.showSaveDialog(...).then(...)) is missing a .catch
handler, so if the dialog rejects the download stays paused and tempPath leaks;
add a .catch on that promise that logs the error via debugError, cancels the
download item (item.cancel()), removes any tempPath file if exists
(fs.unlinkSync/existsSync) and ensures activeDownloads does not retain an entry
for item (activeDownloads.delete(item)), and if macosProgress was created remove
its progress using macosProgress.cancel/remove with the stored progressId; place
this cleanup logic next to the existing cancel/cleanup code path so both
user-cancel and dialog rejection behave the same.
- Around line 123-138: The catch block that wraps the temp/crdownload setup must
abort the download and clean up to avoid resuming with invalid metadata: inside
the existing catch (referencing tempPath, crdownloadPath, debugError/debugPrint)
call the downloadItem.cancel() (or the appropriate DownloadItem.cancel method
used in this module), remove any partial files/symlinks created (crdownloadPath
and tempPath), log the failure, and return/throw to stop further execution so
the code that resumes the download (later in this function) does not run;
additionally, add a .catch() handler to the Promise chain started earlier (the
Promise referenced at line ~90 in this file) to log the error and cancel the
downloadItem there as well so a failed dialog/promise does not leave the
download paused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a63f64cb-0ce4-4f46-8db4-d3892c7f2885
📒 Files selected for processing (6)
src/main/controllers/sessions-controller/default-session/index.tssrc/main/controllers/sessions-controller/handlers/index.tssrc/main/controllers/tabs-controller/tab.tssrc/main/modules/downloads-handler.tssrc/main/modules/macos-progress.tssrc/main/modules/output.ts
Comment on lines
+123
to
+138
| try { | ||
| if (fs.existsSync(tempPath)) { | ||
| fs.renameSync(tempPath, crdownloadPath); | ||
| debugPrint("DOWNLOADS", `Moved temp file to: ${crdownloadPath}`); | ||
| } | ||
| // If temp doesn't exist yet, Electron will create it at tempPath | ||
| // but we'll handle that case with a symlink as fallback | ||
| else { | ||
| // Create empty file at crdownload location and symlink | ||
| fs.writeFileSync(crdownloadPath, ""); | ||
| fs.symlinkSync(crdownloadPath, tempPath); | ||
| debugPrint("DOWNLOADS", `Created symlink fallback: ${tempPath} -> ${crdownloadPath}`); | ||
| } | ||
| } catch (err) { | ||
| debugError("DOWNLOADS", `Failed to move/setup file:`, err); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "downloads-handler.ts" --exec cat -n {} \;Repository: MultiboxLabs/flow-browser
Length of output: 12608
Cancel download if .crdownload setup fails.
If the file move/setup fails in the try block (lines 123-138), the code continues execution without returning, resuming the download at lines 166 with invalid metadata. This orphans the download in an inconsistent state where the crdownloadPath may not exist but is still tracked for completion handling.
Additionally, the Promise chain at line 90 lacks a .catch() handler, leaving the download paused indefinitely if the dialog fails.
Suggested fixes
For lines 123-138, add cancellation and cleanup on setup failure:
} catch (err) {
debugError("DOWNLOADS", `Failed to move/setup file:`, err);
+ item.cancel();
+ try {
+ if (fs.existsSync(crdownloadPath)) fs.unlinkSync(crdownloadPath);
+ if (fs.existsSync(tempPath)) fs.unlinkSync(tempPath);
+ } catch {
+ // Ignore cleanup errors
+ }
+ return;
}For line 90, add a .catch() handler to the Promise chain:
.then(({ filePath: chosenPath, canceled }) => {
// ... existing code
})
+ .catch((err) => {
+ debugError("DOWNLOADS", `Save dialog error:`, err);
+ item.cancel();
+ try {
+ if (fs.existsSync(tempPath)) fs.unlinkSync(tempPath);
+ } catch {
+ // Ignore cleanup errors
+ }
+ });📝 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.
Suggested change
| try { | |
| if (fs.existsSync(tempPath)) { | |
| fs.renameSync(tempPath, crdownloadPath); | |
| debugPrint("DOWNLOADS", `Moved temp file to: ${crdownloadPath}`); | |
| } | |
| // If temp doesn't exist yet, Electron will create it at tempPath | |
| // but we'll handle that case with a symlink as fallback | |
| else { | |
| // Create empty file at crdownload location and symlink | |
| fs.writeFileSync(crdownloadPath, ""); | |
| fs.symlinkSync(crdownloadPath, tempPath); | |
| debugPrint("DOWNLOADS", `Created symlink fallback: ${tempPath} -> ${crdownloadPath}`); | |
| } | |
| } catch (err) { | |
| debugError("DOWNLOADS", `Failed to move/setup file:`, err); | |
| } | |
| try { | |
| if (fs.existsSync(tempPath)) { | |
| fs.renameSync(tempPath, crdownloadPath); | |
| debugPrint("DOWNLOADS", `Moved temp file to: ${crdownloadPath}`); | |
| } | |
| // If temp doesn't exist yet, Electron will create it at tempPath | |
| // but we'll handle that case with a symlink as fallback | |
| else { | |
| // Create empty file at crdownload location and symlink | |
| fs.writeFileSync(crdownloadPath, ""); | |
| fs.symlinkSync(crdownloadPath, tempPath); | |
| debugPrint("DOWNLOADS", `Created symlink fallback: ${tempPath} -> ${crdownloadPath}`); | |
| } | |
| } catch (err) { | |
| debugError("DOWNLOADS", `Failed to move/setup file:`, err); | |
| item.cancel(); | |
| try { | |
| if (fs.existsSync(crdownloadPath)) fs.unlinkSync(crdownloadPath); | |
| if (fs.existsSync(tempPath)) fs.unlinkSync(tempPath); | |
| } catch { | |
| // Ignore cleanup errors | |
| } | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/modules/downloads-handler.ts` around lines 123 - 138, The catch
block that wraps the temp/crdownload setup must abort the download and clean up
to avoid resuming with invalid metadata: inside the existing catch (referencing
tempPath, crdownloadPath, debugError/debugPrint) call the downloadItem.cancel()
(or the appropriate DownloadItem.cancel method used in this module), remove any
partial files/symlinks created (crdownloadPath and tempPath), log the failure,
and return/throw to stop further execution so the code that resumes the download
(later in this function) does not run; additionally, add a .catch() handler to
the Promise chain started earlier (the Promise referenced at line ~90 in this
file) to log the error and cancel the downloadItem there as well so a failed
dialog/promise does not leave the download paused.
iamEvanYT
changed the title
iamEvanYT
force-pushed
the
evan/improved-download-experience
branch
from
243f29a to
c85f50e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/main/modules/downloads/handler.ts`:
- Around line 26-36: Remove the top-level conditional dynamic import block that
attempts to import "@/modules/downloads/macos-progress" (the if
(process.platform === "darwin") { import(...).then(...).catch(...) } block) and
rely solely on the existing lazy-loader function ensureMacosProgressModule() to
import and set the macosProgress variable; keep the macosProgress declaration
(let macosProgress: typeof import("@/modules/downloads/macos-progress") | null =
null;) and any debugError handling only inside ensureMacosProgressModule(), so
there is a single, awaited import path and no redundant asynchronous import at
module top-level.
- Around line 64-66: The helper function generateCrdownloadNumber currently uses
Math.random(), which is not cryptographically secure; replace its implementation
to use Node's crypto.randomInt to produce a 6-digit number (e.g., range
100000–999999) and update imports to include crypto (or import { randomInt }
from 'crypto') so collision resistance improves while keeping the same string
return and the function name generateCrdownloadNumber unchanged.
- Around line 97-103: The current collision handling deletes an existing file at
visiblePath (using fs.unlinkSync) which is unsafe; instead detect the collision
and regenerate the random suffix (recompute the filename that produces
visiblePath) until a non-existent path is found, then proceed without removing
existing files; update the code around the visiblePath generation logic so it
loops (or retries) to produce a new Unconfirmed {random}.crdownload name on
collision, and only use fs.unlinkSync (or write) when you have a guaranteed
unique visiblePath.
In `@src/main/modules/downloads/macos-progress.ts`:
- Around line 14-18: activeProgressMap and cancelCallbackMap can leak if
downloads never reach completeFileProgress or cancelFileProgress; add an
automatic cleanup to remove stale entries. Implement a cleanup mechanism that
tracks creation timestamps (when entries are added to
activeProgressMap/cancelCallbackMap) and periodically (e.g., via a single
setInterval) purges entries older than a safe TTL, or tie cleanup to app
lifecycle events (onShutdown/onCrash) to call a new function like
purgeStaleFileProgress or disposeAllFileProgress; ensure purgeStaleFileProgress
unregisters NSProgress and invokes any cancel callbacks before deleting keys so
completeFileProgress/cancelFileProgress invariants remain respected.
- Around line 125-157: The two functions updateFileProgressThroughput and
updateFileProgressEstimatedTime currently return silently when
activeProgressMap.get(progressId) is falsy; change them to mirror the other
helpers by calling debugError with a clear message including the progressId
(e.g., debugError("DOWNLOADS", "macOS: updateFileProgressThroughput missing
progress for id:", progressId) and similarly for
updateFileProgressEstimatedTime) before returning so missing progress IDs are
consistently logged; keep the existing try/catch and error handling intact.
In `@src/main/modules/output.ts`:
- Line 26: Update the inline comment next to the DOWNLOADS flag in output.ts to
point to the correct module path; replace the incorrect
"@/modules/downloads-handler.ts" comment with "@/modules/downloads/handler.ts"
so the comment matches the actual module location (refer to the DOWNLOADS
constant in src/main/modules/output.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70e3b763-b797-458d-a259-29890d25d89f
📒 Files selected for processing (5)
src/main/controllers/sessions-controller/default-session/index.tssrc/main/controllers/sessions-controller/handlers/index.tssrc/main/modules/downloads/handler.tssrc/main/modules/downloads/macos-progress.tssrc/main/modules/output.ts
Previously, download events were ignored if they occurred before the save dialog completed, leading to leaked temp files and missed completions when downloads finished quickly or users delayed the dialog. Changes: - Create metadata immediately before save dialog instead of after - Track saveConfirmed state to distinguish pre/post-dialog events - Defer completion handling if dialog hasn't finished (earlyCompletion) - Process deferred completions once user confirms save location - Only create visible mirror after save location is confirmed - Extract handleDownloadCompletion() to centralize cleanup logic Fixes issues where fast downloads or cancelled downloads before dialog confirmation would leave orphaned .crdownload files in Downloads folder.
Remove hidden/visible file duality which was causing 0kb temp files to
persist on Windows. Now uses one visible temp file from the start.
Changes:
- Start downloads as visible 'Unconfirmed {id}.crdownload' in Downloads
- Remove dot prefix (no more .Unconfirmed files)
- Only move file if final location is in a different directory
- Add 'same-dir' mirror kind for when no move is needed
- Remove visibleCrdownloadPath from metadata
- Replace mirrorCrdownloadToVisible() with moveCrdownloadToFinalDir()
- Simplify cleanup logic to use removeSecondaryPath()
Fixes issue where hidden .crdownload files would leak on Windows.
For fast downloads, the file may complete before the user responds to the save dialog. If the user then clicks Cancel, item.cancel() does nothing since the download is already done, leaving orphaned .crdownload files. Now we check if earlyCompletion state is 'completed' when user cancels, and manually delete the temp file instead of calling item.cancel().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/modules/downloads/handler.ts (1)
65-67: 🧹 Nitpick | 🔵 TrivialConsider using
crypto.randomInt()for collision resistance.
Math.random()is not cryptographically secure. While acceptable for temp filenames,crypto.randomInt()provides better collision resistance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/modules/downloads/handler.ts` around lines 65 - 67, The generateCrdownloadNumber function uses Math.random which is not cryptographically secure; replace it with crypto.randomInt to produce a 6-digit integer (e.g., crypto.randomInt(100000, 1000000)) and convert to string, and add the import for crypto (import { randomInt } from 'crypto' or import * as crypto) at the top of the module; ensure the function name generateCrdownloadNumber remains unchanged and that any callers continue to receive a string.
🤖 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/main/modules/downloads/handler.ts`:
- Around line 99-121: The three empty catch blocks swallowing errors for
fs.renameSync, fs.linkSync and fs.symlinkSync make debugging impossible; update
each catch to log the failure including the strategy name (rename/link/symlink),
the currentPath and targetPath, and the caught error object (use the module's
existing logger if available, otherwise console.debug/console.error), then
continue with the fallback logic as before so you still try the next strategy.
In `@src/main/modules/downloads/macos-progress.ts`:
- Around line 28-31: The progressId generation in createFileProgress currently
uses `${filePath}-${Date.now()}` which can collide; update the progressId
creation to append a random suffix (e.g., use crypto.randomUUID() or a short
Math.random-based token) so the identifier is
`${filePath}-${Date.now()}-${randomSuffix}`; modify the symbol progressId in
createFileProgress accordingly and ensure any code that references progressId
continues to use the new value.
- Around line 194-199: The cancelFileProgress function lacks a log message when
the progress ID is not found in activeProgressMap, unlike completeFileProgress
which logs this event. Add a processLogger.warn or similar log entry inside the
if (!progress) block in cancelFileProgress to indicate the progress ID was not
found, ensuring consistent logging behavior between these functions.
---
Duplicate comments:
In `@src/main/modules/downloads/handler.ts`:
- Around line 65-67: The generateCrdownloadNumber function uses Math.random
which is not cryptographically secure; replace it with crypto.randomInt to
produce a 6-digit integer (e.g., crypto.randomInt(100000, 1000000)) and convert
to string, and add the import for crypto (import { randomInt } from 'crypto' or
import * as crypto) at the top of the module; ensure the function name
generateCrdownloadNumber remains unchanged and that any callers continue to
receive a string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee133b49-d1d9-47b8-ba81-eef632f3c341
📒 Files selected for processing (3)
src/main/modules/downloads/handler.tssrc/main/modules/downloads/macos-progress.tssrc/main/modules/output.ts
Comment on lines
+99
to
+121
| // Same volume: one inode moves to target; open FD from Chromium keeps working. | ||
| try { | ||
| fs.renameSync(currentPath, targetPath); | ||
| return { kind: "moved", newPath: targetPath }; | ||
| } catch { | ||
| /* continue */ | ||
| } | ||
|
|
||
| // Same volume, second name for the same inode. | ||
| try { | ||
| fs.linkSync(currentPath, targetPath); | ||
| return { kind: "hardlink", newPath: targetPath }; | ||
| } catch { | ||
| /* continue */ | ||
| } | ||
|
|
||
| // Cross-volume: symlink to absolute path. | ||
| try { | ||
| fs.symlinkSync(currentPath, targetPath); | ||
| return { kind: "symlink", newPath: targetPath }; | ||
| } catch { | ||
| /* continue */ | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Empty catch blocks hinder debugging.
The fallback logic is well-designed, but silently swallowing errors makes troubleshooting difficult. Consider adding debug logging to understand which strategy failed and why.
♻️ Add debug logging for fallback attempts
// Same volume: one inode moves to target; open FD from Chromium keeps working.
try {
fs.renameSync(currentPath, targetPath);
return { kind: "moved", newPath: targetPath };
- } catch {
- /* continue */
+ } catch (err) {
+ debugPrint("DOWNLOADS", `rename failed (will try hardlink):`, err);
}
// Same volume, second name for the same inode.
try {
fs.linkSync(currentPath, targetPath);
return { kind: "hardlink", newPath: targetPath };
- } catch {
- /* continue */
+ } catch (err) {
+ debugPrint("DOWNLOADS", `hardlink failed (will try symlink):`, err);
}
// Cross-volume: symlink to absolute path.
try {
fs.symlinkSync(currentPath, targetPath);
return { kind: "symlink", newPath: targetPath };
- } catch {
- /* continue */
+ } catch (err) {
+ debugPrint("DOWNLOADS", `symlink failed (will try placeholder):`, err);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/modules/downloads/handler.ts` around lines 99 - 121, The three empty
catch blocks swallowing errors for fs.renameSync, fs.linkSync and fs.symlinkSync
make debugging impossible; update each catch to log the failure including the
strategy name (rename/link/symlink), the currentPath and targetPath, and the
caught error object (use the module's existing logger if available, otherwise
console.debug/console.error), then continue with the fallback logic as before so
you still try the next strategy.
Comment on lines
+28
to
+31
| export function createFileProgress(filePath: string, totalBytes: number, onCancel?: () => void): string | null { | ||
| try { | ||
| // Generate a unique ID first (needed for the closure) | ||
| const progressId = `${filePath}-${Date.now()}`; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor collision risk in progress ID generation.
The progressId uses ${filePath}-${Date.now()} which could collide if multiple downloads for the same file start within the same millisecond. Consider adding a random suffix for robustness.
♻️ Add random suffix to progress ID
- const progressId = `${filePath}-${Date.now()}`;
+ const progressId = `${filePath}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`;📝 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.
Suggested change
| export function createFileProgress(filePath: string, totalBytes: number, onCancel?: () => void): string | null { | |
| try { | |
| // Generate a unique ID first (needed for the closure) | |
| const progressId = `${filePath}-${Date.now()}`; | |
| export function createFileProgress(filePath: string, totalBytes: number, onCancel?: () => void): string | null { | |
| try { | |
| // Generate a unique ID first (needed for the closure) | |
| const progressId = `${filePath}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/modules/downloads/macos-progress.ts` around lines 28 - 31, The
progressId generation in createFileProgress currently uses
`${filePath}-${Date.now()}` which can collide; update the progressId creation to
append a random suffix (e.g., use crypto.randomUUID() or a short
Math.random-based token) so the identifier is
`${filePath}-${Date.now()}-${randomSuffix}`; modify the symbol progressId in
createFileProgress accordingly and ensure any code that references progressId
continues to use the new value.
Comment on lines
+194
to
+199
| export function cancelFileProgress(progressId: string): void { | ||
| try { | ||
| const progress = activeProgressMap.get(progressId); | ||
| if (!progress) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing error log when progress not found in cancelFileProgress.
For consistency with completeFileProgress (line 167), cancelFileProgress should also log when the progress ID is not found.
♻️ Add consistent logging
export function cancelFileProgress(progressId: string): void {
try {
const progress = activeProgressMap.get(progressId);
if (!progress) {
+ debugError("DOWNLOADS", `macOS: no progress found for ID ${progressId}`);
return;
}📝 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.
Suggested change
| export function cancelFileProgress(progressId: string): void { | |
| try { | |
| const progress = activeProgressMap.get(progressId); | |
| if (!progress) { | |
| return; | |
| } | |
| export function cancelFileProgress(progressId: string): void { | |
| try { | |
| const progress = activeProgressMap.get(progressId); | |
| if (!progress) { | |
| debugError("DOWNLOADS", `macOS: no progress found for ID ${progressId}`); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/modules/downloads/macos-progress.ts` around lines 194 - 199, The
cancelFileProgress function lacks a log message when the progress ID is not
found in activeProgressMap, unlike completeFileProgress which logs this event.
Add a processLogger.warn or similar log entry inside the if (!progress) block in
cancelFileProgress to indicate the progress ID was not found, ensuring
consistent logging behavior between these functions.
Reduce code duplication and improve maintainability by extracting reusable helper functions for common operations. New helper functions: - needsSecondaryCleanup(): Check if mirror kind needs cleanup - cleanupSecondaryPath(): Remove hardlink/symlink/placeholder - finalizeMacProgress(): Complete or cancel macOS progress indicator - deleteFile(): Safe file deletion with error logging - moveTempToFinal(): Move .crdownload to final path (rename or copy) - updateMacProgress(): Update macOS progress stats (throttled) Benefits: - handleDownloadCompletion() reduced from 85 lines to 29 lines - Progress update logic centralized and simplified - File cleanup logic reused across completion states - Easier to test and maintain individual operations - More consistent error handling and logging
When the user saves to Downloads (same directory as temp file), we were calling updateFileProgressPath() even though the path didn't change, causing macOS to drop the progress indicator. Now we only update crdownloadPath and call updateFileProgressPath() when the file actually moved to a different directory (kind !== 'same-dir').
Add better logging and verification when updating macOS progress path: - Log the path change from original to new location - Verify file exists at new location before updating progress - Add error logging if file doesn't exist after move - Keep tracking original file for hardlink/symlink cases This should help diagnose why progress bar disappears when moving to Documents folder.
…tory updateFileProgressPath() doesn't work reliably - Finder loses track of the progress indicator when the file URL is updated after initial registration. Solution: - Add recreateFileProgressAtPath() to macos-progress module - Cancel old progress indicator and create new one at new location - Preserve progress state (completed bytes, throughput, ETA) - Update handler to use recreate instead of update This ensures the progress bar continues to show in Finder when downloading to a different folder (e.g., Documents instead of Downloads).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main/modules/downloads/handler.ts`:
- Around line 334-347: The cancel branch leaves temp files orphaned for terminal
earlyCompletion states and when cancelling in-progress downloads; update the
logic in the handler so that if metadata.earlyCompletion exists and its state is
any terminal value ("completed", "cancelled", "interrupted") you perform the
same cleanup as for "completed" (call deleteFile(metadata.crdownloadPath, ...),
activeDownloads.delete(item), and finalizeMacProgress(mp, progressId, false) as
needed) instead of falling through to item.cancel(); for the case where there is
no earlyCompletion (download in-progress) set a boolean flag on metadata (e.g.,
metadata.shouldCleanupOnDone) before calling item.cancel() so the existing done
handler (the function that currently sets earlyCompletion and returns when
saveConfirmed is false) will detect metadata.shouldCleanupOnDone and perform
deleteFile(...) and activeDownloads.delete(item) when the download reaches a
terminal state; update DownloadMetadata to include this flag and modify the done
handler to check metadata.shouldCleanupOnDone and run the cleanup path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 506a3ae2-a887-4026-b2ca-173673733c7d
📒 Files selected for processing (2)
src/main/modules/downloads/handler.tssrc/main/modules/downloads/macos-progress.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/modules/downloads/handler.ts (1)
343-355:⚠️ Potential issue | 🟠 MajorDialog cancellation still misses non-completed terminal states.
When the dialog closes after the item has already reached
cancelledorinterrupted, this branch still falls through toitem.cancel()even thoughdonehas already fired, so no later cleanup runs. The in-flight case has the same leak: thedonehandler just storesearlyCompletionwhilesaveConfirmedis false, leavingactiveDownloadsand the temp file behind. Carry a cleanup-on-done flag and handle every terminalearlyCompletion.statehere, not just"completed".Also applies to: 440-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/modules/downloads/handler.ts` around lines 343 - 355, The cancellation branch currently only cleans up when metadata.earlyCompletion.state === "completed", causing leaks when the item is already "cancelled" or "interrupted" or when the download finishes before saveConfirmed is set; update the handler around canceled/chosenPath to check metadata.earlyCompletion?.state for all terminal states ("completed", "cancelled", "interrupted") and perform the same cleanup actions (await deleteFile(metadata.crdownloadPath, ...); activeDownloads.delete(item); finalizeMacProgress(mp, progressId, false); return) for any terminal state. Additionally, introduce and set a "cleanupOnDone" flag (used by the existing done handler that currently only sets earlyCompletion when saveConfirmed is false) so that if saveConfirmed is false but the download finished earlier, the done handler will run the same cleanup and activeDownloads.delete when it sees cleanupOnDone; update references to item.cancel() to only be called when there is no terminal earlyCompletion state and clear/return immediately after cleanup.
🤖 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/main/modules/downloads/handler.ts`:
- Around line 194-218: The current moveTempToFinal unlinks finalPath before
staging the new file, which risks losing the original if rename/copy later
fails; update moveTempToFinal to (1) short-circuit and return true if
crdownloadPath === finalPath, (2) do NOT call fs.unlink(finalPath) up front, and
instead stage the new file to a temporary sibling (e.g., derive tempFinalPath
from finalPath), by renaming/copying crdownloadPath -> tempFinalPath (use
fs.rename or fs.copyFile+unlink fallback), then atomically replace the original
by renaming tempFinalPath -> finalPath (this ensures the existing finalPath
remains until the replacement is fully written); keep references to finalPath,
crdownloadPath, fs.rename, fs.copyFile, fs.unlink and pathExists when
implementing this swap.
- Around line 325-371: Wrap the detached async IIFE body in a top-level
try/catch and on any caught error run the same cleanup/unwind used for
user-cancelled dialog: call finalizeMacProgress(mp, progressId, false), then if
metadata.earlyCompletion?.state === "completed" await
deleteFile(metadata.crdownloadPath, "completed download after error") and
activeDownloads.delete(item) else call item.cancel(); ensure
metadata.saveConfirmed remains false (or unset) and remove the orphaned
activeDownloads entry when appropriate; include a debugPrint with the error for
context. Use the existing symbols (the async IIFE, ensureMacosProgressModule,
mp.createFileProgress, dialog.showSaveDialog, finalizeMacProgress, deleteFile,
handleDownloadCompletion, activeDownloads, metadata, item) to locate and
implement the try/catch.
- Around line 132-135: Replace the Node fs writeFile call in the placeholder
fallback with the repo's Bun write path: remove await fs.writeFile(targetPath,
"") and use Bun's write API (e.g., Bun.write or Bun.writeSync) to create an
empty file at targetPath, keeping the rest of the placeholder branch intact and
returning { kind: "placeholder", newPath: currentPath }; also remove the node:fs
import if it becomes unused.
- Around line 266-272: The Finder progress is being completed before the temp
file is moved and the result of moveTempToFinal is ignored; change the ordering
so you first run cleanupSecondaryPath(meta, crdownloadBasename) then await
moveTempToFinal(meta.crdownloadPath, meta.finalPath) and check its boolean
result, and only on success call recreateFileProgressAtPath(mp, meta.progressId,
meta.finalPath) (to rebind the progress entry) followed by
completeFileProgress(mp, meta.progressId) / finalizeMacProgress as appropriate;
if moveTempToFinal returns false, do not complete the progress and instead
log/handle the failure so the .crdownload remains visible.
---
Duplicate comments:
In `@src/main/modules/downloads/handler.ts`:
- Around line 343-355: The cancellation branch currently only cleans up when
metadata.earlyCompletion.state === "completed", causing leaks when the item is
already "cancelled" or "interrupted" or when the download finishes before
saveConfirmed is set; update the handler around canceled/chosenPath to check
metadata.earlyCompletion?.state for all terminal states ("completed",
"cancelled", "interrupted") and perform the same cleanup actions (await
deleteFile(metadata.crdownloadPath, ...); activeDownloads.delete(item);
finalizeMacProgress(mp, progressId, false); return) for any terminal state.
Additionally, introduce and set a "cleanupOnDone" flag (used by the existing
done handler that currently only sets earlyCompletion when saveConfirmed is
false) so that if saveConfirmed is false but the download finished earlier, the
done handler will run the same cleanup and activeDownloads.delete when it sees
cleanupOnDone; update references to item.cancel() to only be called when there
is no terminal earlyCompletion state and clear/return immediately after cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3482770e-ef7b-4cac-bb90-c2cafcb4f031
📒 Files selected for processing (1)
src/main/modules/downloads/handler.ts
Comment on lines
+132
to
+135
| // Last resort: empty decoy at target path. | ||
| try { | ||
| await fs.writeFile(targetPath, ""); | ||
| return { kind: "placeholder", newPath: currentPath }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use the repo's Bun write path for the placeholder fallback.
This branch is the only direct writeFile() usage in the module, and it breaks the TS guideline to avoid Node's read/write helpers. If this Electron main-process entrypoint is expected to follow the repo's Bun runtime conventions, switch the empty-file creation to the Bun equivalent here.
As per coding guidelines, "Prefer Bun.file over node:fs's readFile/writeFile`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/modules/downloads/handler.ts` around lines 132 - 135, Replace the
Node fs writeFile call in the placeholder fallback with the repo's Bun write
path: remove await fs.writeFile(targetPath, "") and use Bun's write API (e.g.,
Bun.write or Bun.writeSync) to create an empty file at targetPath, keeping the
rest of the placeholder branch intact and returning { kind: "placeholder",
newPath: currentPath }; also remove the node:fs import if it becomes unused.
Comment on lines
+194
to
+218
| // Remove existing final file if present | ||
| if (await pathExists(finalPath)) { | ||
| try { | ||
| await fs.unlink(finalPath); | ||
| } catch { | ||
| /* ignore */ | ||
| } | ||
| } | ||
|
|
||
| // Try rename first (fastest) | ||
| try { | ||
| await fs.rename(crdownloadPath, finalPath); | ||
| debugPrint("DOWNLOADS", `Moved to final path: ${finalPath}`); | ||
| return true; | ||
| } catch { | ||
| // Fall back to copy+delete for cross-device moves | ||
| try { | ||
| await fs.copyFile(crdownloadPath, finalPath); | ||
| await fs.unlink(crdownloadPath); | ||
| debugPrint("DOWNLOADS", `Copied to final path: ${finalPath}`); | ||
| return true; | ||
| } catch (copyErr) { | ||
| debugError("DOWNLOADS", `Failed to move download:`, copyErr); | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't delete the existing destination before the replacement is ready.
moveTempToFinal() unlinks finalPath up front. If the later rename/copy fails (ENOSPC, permissions, device removal), the previous file is gone and the finished download is still stranded at crdownloadPath. Keep the old file until the new bytes have been staged successfully, then swap. At minimum, short-circuit crdownloadPath === finalPath.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/modules/downloads/handler.ts` around lines 194 - 218, The current
moveTempToFinal unlinks finalPath before staging the new file, which risks
losing the original if rename/copy later fails; update moveTempToFinal to (1)
short-circuit and return true if crdownloadPath === finalPath, (2) do NOT call
fs.unlink(finalPath) up front, and instead stage the new file to a temporary
sibling (e.g., derive tempFinalPath from finalPath), by renaming/copying
crdownloadPath -> tempFinalPath (use fs.rename or fs.copyFile+unlink fallback),
then atomically replace the original by renaming tempFinalPath -> finalPath
(this ensures the existing finalPath remains until the replacement is fully
written); keep references to finalPath, crdownloadPath, fs.rename, fs.copyFile,
fs.unlink and pathExists when implementing this swap.
Comment on lines
+266
to
+272
| if (state === "completed") { | ||
| finalizeMacProgress(mp, meta.progressId, true); | ||
|
|
||
| // Only move to final path if user confirmed save dialog | ||
| if (meta.saveConfirmed && meta.finalPath) { | ||
| await cleanupSecondaryPath(meta, crdownloadBasename); | ||
| await moveTempToFinal(meta.crdownloadPath, meta.finalPath); |
Contributor
There was a problem hiding this comment.
Complete Finder progress only after the final path exists.
completeFileProgress() runs before moveTempToFinal(), and the boolean result of moveTempToFinal() is ignored. If that move fails, Finder shows a finished download while the file is still left behind as .crdownload. Even on success, the progress entry never gets rebound from the temp name to meta.finalPath, although src/main/modules/downloads/macos-progress.ts:251-296 already provides recreateFileProgressAtPath() for path changes.
♻️ Suggested completion ordering
if (state === "completed") {
- finalizeMacProgress(mp, meta.progressId, true);
-
// Only move to final path if user confirmed save dialog
if (meta.saveConfirmed && meta.finalPath) {
await cleanupSecondaryPath(meta, crdownloadBasename);
- await moveTempToFinal(meta.crdownloadPath, meta.finalPath);
+ const moved = await moveTempToFinal(meta.crdownloadPath, meta.finalPath);
+ if (!moved) {
+ finalizeMacProgress(mp, meta.progressId, false);
+ return;
+ }
+ if (mp && meta.progressId) {
+ const newProgressId = mp.recreateFileProgressAtPath(meta.progressId, meta.finalPath);
+ if (newProgressId) {
+ meta.progressId = newProgressId;
+ }
+ }
+ finalizeMacProgress(mp, meta.progressId, true);
} else {
// Download completed before user chose save location; leave temp file
debugPrint("DOWNLOADS", `No save location chosen yet`);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/modules/downloads/handler.ts` around lines 266 - 272, The Finder
progress is being completed before the temp file is moved and the result of
moveTempToFinal is ignored; change the ordering so you first run
cleanupSecondaryPath(meta, crdownloadBasename) then await
moveTempToFinal(meta.crdownloadPath, meta.finalPath) and check its boolean
result, and only on success call recreateFileProgressAtPath(mp, meta.progressId,
meta.finalPath) (to rebind the progress entry) followed by
completeFileProgress(mp, meta.progressId) / finalizeMacProgress as appropriate;
if moveTempToFinal returns false, do not complete the progress and instead
log/handle the failure so the .crdownload remains visible.
Comment on lines
+325
to
+371
| void (async () => { | ||
| const mp = await ensureMacosProgressModule(); | ||
|
|
||
| let progressId: string | null = null; | ||
| if (mp) { | ||
| progressId = mp.createFileProgress(crdownloadPath, totalBytes > 0 ? totalBytes : 0, () => { | ||
| debugPrint("DOWNLOADS", `Cancel requested from Finder for: ${suggestedFilename}`); | ||
| item.cancel(); | ||
| }); | ||
| debugPrint("DOWNLOADS", `macOS progress created: ${progressId}`); | ||
| metadata.progressId = progressId; | ||
| } | ||
|
|
||
| const { filePath: chosenPath, canceled } = await dialog.showSaveDialog(window.browserWindow, { | ||
| defaultPath, | ||
| properties: ["createDirectory", "showOverwriteConfirmation"] | ||
| }); | ||
|
|
||
| if (canceled || !chosenPath) { | ||
| debugPrint("DOWNLOADS", `Download cancelled by user: ${suggestedFilename}`); | ||
| finalizeMacProgress(mp, progressId, false); | ||
|
|
||
| // If download already completed before user cancelled, manually clean up the file | ||
| if (metadata.earlyCompletion?.state === "completed") { | ||
| await deleteFile(metadata.crdownloadPath, "completed download after user cancelled"); | ||
| activeDownloads.delete(item); | ||
| } else { | ||
| // Download still in progress, cancel it normally | ||
| item.cancel(); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const finalPath = chosenPath; | ||
| debugPrint("DOWNLOADS", `User chose final path: ${finalPath}`); | ||
|
|
||
| // Update metadata with final path and mark as confirmed | ||
| metadata.finalPath = finalPath; | ||
| metadata.saveConfirmed = true; | ||
|
|
||
| // If download already completed/cancelled before dialog finished, handle it now | ||
| if (metadata.earlyCompletion) { | ||
| debugPrint("DOWNLOADS", `Handling early completion (${metadata.earlyCompletion.state})`); | ||
| await handleDownloadCompletion(item, metadata, metadata.earlyCompletion.state, mp, crdownloadBasename); | ||
| activeDownloads.delete(item); | ||
| } | ||
| })(); |
Contributor
There was a problem hiding this comment.
Catch failures in the detached dialog task and unwind the download.
This fire-and-forget async block has no top-level try/catch. If any awaited step throws, the item keeps downloading with saveConfirmed === false, and the done handler only records earlyCompletion, so the map entry and .crdownload can be orphaned. Route errors through the same cancellation/cleanup path as a user-cancelled dialog.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/modules/downloads/handler.ts` around lines 325 - 371, Wrap the
detached async IIFE body in a top-level try/catch and on any caught error run
the same cleanup/unwind used for user-cancelled dialog: call
finalizeMacProgress(mp, progressId, false), then if
metadata.earlyCompletion?.state === "completed" await
deleteFile(metadata.crdownloadPath, "completed download after error") and
activeDownloads.delete(item) else call item.cancel(); ensure
metadata.saveConfirmed remains false (or unset) and remove the orphaned
activeDownloads entry when appropriate; include a debugPrint with the error for
context. Use the existing symbols (the async IIFE, ensureMacosProgressModule,
mp.createFileProgress, dialog.showSaveDialog, finalizeMacProgress, deleteFile,
handleDownloadCompletion, activeDownloads, metadata, item) to locate and
implement the try/catch.
Member
Author
|
@greptile review |
Comment on lines
+426
to
+465
| // Only move file if user has confirmed save location and file exists | ||
| if (state === "progressing" && !meta.mirrorSetup && meta.saveConfirmed && meta.finalPath) { | ||
| meta.mirrorSetup = true; | ||
| void (async () => { | ||
| if (!(await pathExists(meta.crdownloadPath))) { | ||
| meta.mirrorSetup = false; | ||
| return; | ||
| } | ||
| const finalDir = path.dirname(meta.finalPath!); | ||
| const originalPath = meta.crdownloadPath; | ||
| const result = await moveCrdownloadToFinalDir(meta.crdownloadPath, finalDir, crdownloadBasename); | ||
| meta.mirrorKind = result.kind; | ||
|
|
||
| debugPrint("DOWNLOADS", `In-progress .crdownload (${result.kind}): ${result.newPath}`); | ||
|
|
||
| // Only update macOS progress path if we successfully MOVED the file (not hardlink/symlink/placeholder) | ||
| if (result.kind === "moved") { | ||
| meta.crdownloadPath = result.newPath; | ||
|
|
||
| // Verify file exists at new location before updating progress | ||
| if (await pathExists(result.newPath)) { | ||
| if (macosProgress && meta.progressId) { | ||
| debugPrint("DOWNLOADS", `Recreating macOS progress from ${originalPath} to ${result.newPath}`); | ||
| // Recreate progress at new location (more reliable than updating path) | ||
| const newProgressId = macosProgress.recreateFileProgressAtPath(meta.progressId, result.newPath, () => { | ||
| debugPrint("DOWNLOADS", `Cancel requested from Finder for: ${item.getFilename()}`); | ||
| item.cancel(); | ||
| }); | ||
| if (newProgressId) { | ||
| meta.progressId = newProgressId; | ||
| } | ||
| } | ||
| } else { | ||
| debugError("DOWNLOADS", `File doesn't exist at new path after move: ${result.newPath}`); | ||
| } | ||
| } else if (result.kind === "hardlink" || result.kind === "symlink") { | ||
| // Keep tracking original file, but user sees progress on the link in final directory | ||
| debugPrint("DOWNLOADS", `Keeping macOS progress on original file: ${meta.crdownloadPath}`); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
moveCrdownloadToFinalDir is launched fire-and-forget (void (async () => {...})()) and meta.mirrorKind is only assigned after that async chain resolves. If the download's done event fires and handleDownloadCompletion runs while moveCrdownloadToFinalDir is still in-flight (which can happen at the await ensureMacosProgressModule() yield on line 505), the following sequence leaves junk in the user's directory:
moveCrdownloadToFinalDirstarts:renamefails (cross-device),linkfails — now blocked waiting onfs.symlink(orfs.writeFilefor the placeholder).donefires →await ensureMacosProgressModule()yields.cleanupSecondaryPathis called —meta.mirrorKindis stillundefined, soneedsSecondaryCleanupreturnsfalseand no cleanup happens.moveTempToFinalcopies the originalcrdownloadPathto the final location and then deletescrdownloadPath.moveCrdownloadToFinalDirresumes and callsfs.symlink(crdownloadPath, secondaryPath)— creates a dangling symlink (or, for theplaceholderpath, an empty ghost file) atfinalDir/Unconfirmed-xxx.crdownload, which the user now sees in their chosen save directory.
The fix is to track the in-flight move promise on the metadata so cleanupSecondaryPath (or handleDownloadCompletion) can await it:
// In DownloadMetadata
mirrorSetupPromise?: Promise<void>;
// In the updated handler, store instead of void-ing:
meta.mirrorSetupPromise = (async () => {
// ... existing body ...
})();
// In handleDownloadCompletion, before cleanupSecondaryPath:
if (meta.mirrorSetupPromise) {
await meta.mirrorSetupPromise;
}
Comment on lines
+246
to
+257
| } catch (copyErr) { | ||
| debugError("DOWNLOADS", `Failed to move download:`, copyErr); | ||
| return { success: false, finalPath: targetPath }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async function cleanupRejectedDownload( | ||
| meta: DownloadMetadata, | ||
| mp: MacOSProgress | null, | ||
| crdownloadBasename: string, | ||
| state: "completed" | "cancelled" | "interrupted" |
There was a problem hiding this comment.
getAvailableFinalPath has no iteration upper bound
The while (await pathExists(candidatePath)) loop has no cap on index. In a pathological directory (or a test environment with hundreds of identically-named files), this becomes an unbounded sequence of async I/O calls and can hang the download completion indefinitely. Adding a reasonable ceiling (e.g., 1000) and falling back to a timestamp suffix makes the behavior deterministic:
Suggested change
| } catch (copyErr) { | |
| debugError("DOWNLOADS", `Failed to move download:`, copyErr); | |
| return { success: false, finalPath: targetPath }; | |
| } | |
| } | |
| } | |
| async function cleanupRejectedDownload( | |
| meta: DownloadMetadata, | |
| mp: MacOSProgress | null, | |
| crdownloadBasename: string, | |
| state: "completed" | "cancelled" | "interrupted" | |
| async function getAvailableFinalPath(filePath: string): Promise<string> { | |
| const { dir, ext, base } = getPathParts(filePath); | |
| const MAX_ATTEMPTS = 1000; | |
| let index = 1; | |
| let candidatePath = path.join(dir, `${base} (${index})${ext}`); | |
| while (index <= MAX_ATTEMPTS && (await pathExists(candidatePath))) { | |
| index += 1; | |
| candidatePath = path.join(dir, `${base} (${index})${ext}`); | |
| } | |
| if (index > MAX_ATTEMPTS) { | |
| candidatePath = path.join(dir, `${base} (${Date.now()})${ext}`); | |
| } | |
| return candidatePath; | |
| } |
iamEvanYT
force-pushed
the
main
branch
2 times, most recently
from
ce3f35d to
b2cbe93
Compare
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.
Improve File Downloading Experience by:
.crdownloadwhen download is in-progress to indicate itNSProgressto setup a native download bar in Finder on macOSSummary by CodeRabbit