Conversation
Contributor
WalkthroughAdds a full download manager: DB schema and migration, main-process controller with resume/progress and macOS integration, IPC surface and preload exposure, renderer UI (popover + manager), route, types, and Drizzle snapshot/journal entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Electron as Electron
participant Controller as DownloadsController
participant DB as Database
participant IPC as IPC Handler
participant Renderer as Renderer
Electron->>Controller: registerSession(session)
Controller->>Controller: Install will-download handler
Electron->>Controller: will-download (new item)
Controller->>DB: upsertDownloadRecord()
DB-->>Controller: OK
Controller->>Controller: track activeDownloads
Controller->>IPC: fireDownloadsChanged()
IPC->>Renderer: "downloads:on-changed"
Electron->>Controller: item.updated (progress)
Controller->>DB: updateDownloadRecord() [throttled]
Controller->>IPC: fireDownloadsChanged()
IPC->>Renderer: "downloads:on-changed"
Electron->>Controller: item.done
Controller->>DB: updateDownloadRecord(state=completed)
Controller->>IPC: fireDownloadsChanged()
IPC->>Renderer: "downloads:on-changed"
sequenceDiagram
participant Renderer as Renderer
participant IPC as IPC Handler
participant Controller as DownloadsController
participant DB as Database
participant Electron as Electron Download
Renderer->>IPC: resumeDownload(downloadId)
IPC->>Controller: resumeDownload()
Controller->>DB: getDownloadRecord(downloadId)
DB-->>Controller: record
Controller->>Controller: resolve session for originProfileId
Controller->>Controller: enqueue PendingResumeRequest (30s)
Controller->>Electron: createInterruptedDownload(urlChain, offsets)
Electron->>Controller: will-download (session matches)
Controller->>Controller: consume PendingResumeRequest
Controller->>Electron: autoResume? item.resume()
Controller->>DB: updateDownloadRecord()
Controller->>IPC: fireDownloadsChanged()
IPC->>Renderer: "downloads:on-changed"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
Contributor
Build artifacts for all platforms are ready! 🚀Download the artifacts for: One-line installer (Unstable):bunx flow-debug-build@1.2.1 --open 24091651794(execution 24091651794 / attempt 1) |
Greptile SummaryThis PR introduces a full-featured download manager for the Flow browser: a persistent SQLite-backed download store (via Drizzle ORM), an Electron Highlights:
Issues found:
Confidence Score: 4/5Safe to merge after fixing the silent shell.openPath error; the formatBytes out-of-bounds fix is minor but also easy to apply. One P1 bug (openFile always reports success even when the OS fails to open the file, giving users false-positive feedback) and one P2 rendering bug (formatBytes produces 'undefined' unit for PB-scale files). All other components — the controller, persistence layer, macOS NSProgress integration, protocol handler, and renderer components — are clean and well-implemented. src/main/ipc/browser/downloads.ts (shell.openPath not awaited) and src/renderer/src/components/downloads/manager/utils.ts (formatBytes array out-of-bounds) Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer
participant IPC as IPC (downloads.ts)
participant DC as DownloadsController
participant E as Electron DownloadItem
participant DB as SQLite
participant MP as macOS NSProgress
R->>IPC: flow.downloads.list()
IPC->>DC: listDownloads()
DC->>DB: SELECT * ORDER BY updated_at DESC
DB-->>R: DownloadRecord[]
E->>DC: session will-download
DC->>DB: UPSERT (state=progressing)
DC->>IPC: fireDownloadsChanged()
IPC-->>R: downloads:on-changed
loop Every updated event (throttled 1 s)
E->>DC: item.updated (progressing)
DC->>MP: updateFileProgress()
DC->>DB: UPDATE received_bytes, state
DC->>IPC: fireDownloadsChanged()
IPC-->>R: downloads:on-changed
end
E->>DC: item.done (completed / cancelled / interrupted)
DC->>MP: completeFileProgress / cancelFileProgress
DC->>DB: UPDATE final state + end_time
DC->>IPC: fireDownloadsChanged()
IPC-->>R: downloads:on-changed
R->>IPC: flow.downloads.resume(id)
IPC->>DC: resumeDownload(id)
DC->>DB: getDownloadRecord(id)
DC->>E: session.createInterruptedDownload()
Note over DC,E: triggers will-download again with pending-resume match
E->>DC: session will-download (restored)
DC->>E: item.resume() via queueMicrotask
Reviews (1): Last reviewed commit: "fix: issues" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+38
to
+43
| ipcMain.handle("downloads:open-file", (_event, downloadId: string) => { | ||
| const record = getDownloadRecord(downloadId); | ||
| if (!record?.savePath || record.state !== "completed") return false; | ||
| shell.openPath(record.savePath); | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
shell.openPath result not awaited — errors silently swallowed
shell.openPath() returns a Promise<string> where an empty string means success and a non-empty string is the OS error message. Because the promise is not awaited, any failure (no associated application, permission denied, file moved/deleted since the record was saved) is silently dropped. The handler always returns true whenever it finds a completed record, so the renderer's if (!ok) toast.error("Could not open file") guard is never triggered — users get no feedback when the open fails.
Suggested change
| ipcMain.handle("downloads:open-file", (_event, downloadId: string) => { | |
| const record = getDownloadRecord(downloadId); | |
| if (!record?.savePath || record.state !== "completed") return false; | |
| shell.openPath(record.savePath); | |
| return true; | |
| }); | |
| ipcMain.handle("downloads:open-file", async (_event, downloadId: string) => { | |
| const record = getDownloadRecord(downloadId); | |
| if (!record?.savePath || record.state !== "completed") return false; | |
| const err = await shell.openPath(record.savePath); | |
| return err === ""; | |
| }); |
Comment on lines
+3
to
+8
| export function formatBytes(bytes: number): string { | ||
| if (bytes === 0) return "0 B"; | ||
| const units = ["B", "KB", "MB", "GB", "TB"]; | ||
| const i = Math.floor(Math.log(bytes) / Math.log(1024)); | ||
| const value = bytes / Math.pow(1024, i); | ||
| return `${value.toFixed(i > 0 ? 1 : 0)} ${units[i]}`; |
There was a problem hiding this comment.
formatBytes out-of-bounds for files ≥ 1 PB
The units array has 5 elements (indices 0–4: B, KB, MB, GB, TB). For any value where bytes >= 1_125_899_906_842_624 (1 PB), Math.floor(Math.log(bytes) / Math.log(1024)) evaluates to 5, which is out of bounds — units[5] is undefined, producing a display string like "1.0 undefined". Large disk images, OS installers, and video files can realistically hit hundreds of GB; petabyte-range values are uncommon but the silent rendering corruption is worth guarding against.
Suggested change
| export function formatBytes(bytes: number): string { | |
| if (bytes === 0) return "0 B"; | |
| const units = ["B", "KB", "MB", "GB", "TB"]; | |
| const i = Math.floor(Math.log(bytes) / Math.log(1024)); | |
| const value = bytes / Math.pow(1024, i); | |
| return `${value.toFixed(i > 0 ? 1 : 0)} ${units[i]}`; | |
| export function formatBytes(bytes: number): string { | |
| if (bytes === 0) return "0 B"; | |
| const units = ["B", "KB", "MB", "GB", "TB", "PB"]; | |
| const i = Math.min(Math.floor(Math.log(bytes) / Math.log(1024)), units.length - 1); | |
| const value = bytes / Math.pow(1024, i); | |
| return `${value.toFixed(i > 0 ? 1 : 0)} ${units[i]}`; | |
| } |
![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: 10
🤖 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/downloads-controller/index.ts`:
- Around line 496-501: getSessionForDownload currently always uses
electronSession.fromPath when originProfileId is set, which can create an
uninitialized profile session; change it to first try returning the fully
initialized session instance used by the app (e.g., the profile/session registry
created during bootstrap) and only fall back to
electronSession.fromPath(path.join(PROFILES_DIR, originProfileId)) if no
initialized session exists; update getSessionForDownload to check the app's
session manager/registry for originProfileId and return that session, otherwise
return electronSession.defaultSession or the fromPath fallback as now.
- Around line 445-460: The updateDownloadRecord call in the resume path is
overwriting persisted metadata with placeholder values from Electron (e.g. 0 or
""), so change the fields startTime, mimeType, eTag, and lastModified to
preserve existing values when Electron reports placeholders: use
getDownloadStartTimeMs(item) only if it yields a truthy/positive timestamp else
keep meta.startTime; use this.emptyToNull(item.getMimeType()),
this.emptyToNull(item.getETag()), and
this.emptyToNull(item.getLastModifiedTime()) only if they return non-null/
non-empty values else keep meta.mimeType, meta.eTag, and meta.lastModified
respectively; leave the rest of the update (including shouldSetEndTime and
received/total bytes) unchanged so buildDownloadInsert/createInterruptedDownload
validators retain original persisted values.
- Around line 336-345: The code updates meta.savePath even when
mp.recreateFileProgressAtPath(...) returns null, which stops future retries
while meta.progressId may point to a removed entry; change the logic in the
block around recreateFileProgressAtPath so that meta.savePath is only assigned
when nextProgressId is truthy (i.e., recreate succeeded), and leave
meta.savePath and meta.progressId unchanged when nextProgressId is null
(optionally add a debug/log line when recreation fails) so the next sync can
retry recreation.
- Around line 133-140: resumeDownload is queuing duplicate interrupted resumes
for the same downloadId; before calling
getSessionForDownload/registerSession/enqueuePendingResume, check for and reject
duplicates by verifying if an in-flight or pending resume already exists for the
downloadId (consult activeDownloadsById and the pending-resume queue inside
enqueuePendingResume or a new pendingResumeById map), and return early if found;
alternatively deduplicate inside enqueuePendingResume by using downloadId as the
unique key so createInterruptedDownload cannot be queued twice for the same
downloadId (refer to functions/members: resumeDownload, getSessionForDownload,
enqueuePendingResume, createInterruptedDownload, activeDownloadsById).
In `@src/main/ipc/browser/downloads.ts`:
- Around line 51-61: The clear-completed handler currently sets changed = true
regardless of whether deleteDownloadRecord(dl.id) actually succeeded; update the
loop in the ipcMain.handle("downloads:clear-completed") callback to check the
boolean result of deleteDownloadRecord for each dl and only set changed = true
when deleteDownloadRecord(...) returns true, optionally collect/log failed
deletions and avoid firing fireDownloadsChanged() if no records were actually
removed; keep references to listDownloads(), deleteDownloadRecord(), and
fireDownloadsChanged() so the change is localized to that handler.
- Around line 38-43: The handler for "downloads:open-file" currently calls
shell.openPath(record.savePath) but doesn’t await its Promise, so it always
returns true; modify the ipcMain.handle callback (the "downloads:open-file"
handler that uses getDownloadRecord) to await shell.openPath(record.savePath)
and check its returned string (empty = success, non-empty = error message) and
return true only on success and false on error (optionally log or process the
error string before returning false).
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/bottom/downloads-popover.tsx`:
- Around line 40-47: handleClick in downloads-popover.tsx currently calls
flow.downloads.openFile(dl.id) without catching errors, so failures are silent;
update handleClick to mirror DownloadCard by awaiting or handling the promise
from flow.downloads.openFile(dl.id) and catching errors, and on catch show a
user toast (or similar UI feedback) before falling back to opening the downloads
tab and calling setOpen(false); reference handleClick, dl.id,
flow.downloads.openFile, flow.tabs.newTab and setOpen when locating the fix.
- Around line 147-159: fetchDownloads can call setDownloads/setFileExistence
after the component unmounts; add a mount guard to avoid setState-after-unmount
warnings. Inside the component create a ref like isMounted and set it true on
mount and false in a cleanup useEffect, then inside the useCallback
fetchDownloads check isMounted.current before calling setDownloads and
setFileExistence (and after awaiting flow.downloads.checkFilesExist) so no state
updates occur if the popover has closed; ensure the cleanup toggles the ref to
false to prevent updates from in-flight promises.
In `@src/renderer/src/components/downloads/manager/main.tsx`:
- Around line 44-47: The clearCompleted handler currently calls
flow.downloads.clearCompleted() then immediately shows toast.success, which will
display success even if clearCompleted() throws; wrap the await call in a
try-catch inside the clearCompleted function, await
flow.downloads.clearCompleted() first, call toast.success only on success, and
on error call toast.error with the caught error message (and optionally rethrow
or log via processLogger) so failures are handled and reported; reference the
clearCompleted function and flow.downloads.clearCompleted and
toast.success/toast.error when making the change.
In `@src/renderer/src/components/downloads/manager/provider.tsx`:
- Around line 22-40: The fetchDownloads function leaves stale entries in
fileExistence when downloads are removed; modify fetchDownloads in provider.tsx
so it clears or trims fileExistence before/when updating: before calling
flow.downloads.checkFilesExist or immediately after getting all, compute the
current ids (all.map(dl => dl.id)) and call setFileExistence with either an
empty object or a filtered object limited to those ids, then merge/apply the
fresh existence results (from checkFilesExist) so only current downloads remain;
update references to setFileExistence and ensure this runs only when
mountedRef.current is true.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 550c0b75-e488-4fef-918b-9e222e5606bf
📒 Files selected for processing (28)
drizzle/0004_add_download_manager.sqldrizzle/meta/0004_snapshot.jsondrizzle/meta/_journal.jsonsrc/main/controllers/downloads-controller/index.tssrc/main/controllers/downloads-controller/macos-progress.tssrc/main/controllers/sessions-controller/default-session/index.tssrc/main/controllers/sessions-controller/handlers/index.tssrc/main/controllers/sessions-controller/protocols/_protocols/flow/file-icon.tssrc/main/controllers/sessions-controller/protocols/_protocols/flow/index.tssrc/main/controllers/sessions-controller/protocols/static-domains/config.tssrc/main/ipc/browser/downloads.tssrc/main/ipc/index.tssrc/main/modules/output.tssrc/main/saving/db/schema.tssrc/main/saving/downloads.tssrc/preload/index.tssrc/renderer/src/components/browser-ui/browser-sidebar/_components/bottom/downloads-popover.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/inner.tsxsrc/renderer/src/components/downloads/manager/download-card.tsxsrc/renderer/src/components/downloads/manager/file-icon.tsxsrc/renderer/src/components/downloads/manager/main.tsxsrc/renderer/src/components/downloads/manager/provider.tsxsrc/renderer/src/components/downloads/manager/utils.tssrc/renderer/src/routes/downloads/config.tsxsrc/renderer/src/routes/downloads/page.tsxsrc/shared/flow/flow.tssrc/shared/flow/interfaces/browser/downloads.tssrc/shared/types/downloads.ts
Comment on lines
+133
to
+140
| const targetSession = this.getSessionForDownload(record.originProfileId); | ||
| this.registerSession(targetSession); | ||
| this.enqueuePendingResume(targetSession, { | ||
| downloadId, | ||
| savePath: record.savePath!, | ||
| lastUrl: record.urlChain[record.urlChain.length - 1] ?? record.url, | ||
| autoResume: true | ||
| }); |
Contributor
There was a problem hiding this comment.
Reject duplicate resume requests for the same download.
Until will-download arrives, this record stays paused/interrupted, so resumeDownload() can be called again and queue a second createInterruptedDownload() for the same downloadId. Once both events arrive, activeDownloadsById gets overwritten and updated/done from the first item can be applied to the second one.
💡 Suggested fix
const targetSession = this.getSessionForDownload(record.originProfileId);
this.registerSession(targetSession);
+ if ((this.pendingResumeRequests.get(targetSession) ?? []).some((r) => r.downloadId === downloadId)) {
+ return false;
+ }
this.enqueuePendingResume(targetSession, {
downloadId,
savePath: record.savePath!,
lastUrl: record.urlChain[record.urlChain.length - 1] ?? record.url,
autoResume: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/controllers/downloads-controller/index.ts` around lines 133 - 140,
resumeDownload is queuing duplicate interrupted resumes for the same downloadId;
before calling getSessionForDownload/registerSession/enqueuePendingResume, check
for and reject duplicates by verifying if an in-flight or pending resume already
exists for the downloadId (consult activeDownloadsById and the pending-resume
queue inside enqueuePendingResume or a new pendingResumeById map), and return
early if found; alternatively deduplicate inside enqueuePendingResume by using
downloadId as the unique key so createInterruptedDownload cannot be queued twice
for the same downloadId (refer to functions/members: resumeDownload,
getSessionForDownload, enqueuePendingResume, createInterruptedDownload,
activeDownloadsById).
Comment on lines
+336
to
+345
| if (meta.savePath && meta.savePath !== savePath) { | ||
| const nextProgressId = mp.recreateFileProgressAtPath(meta.progressId, savePath, () => { | ||
| debugPrint("DOWNLOADS", `Cancel requested from Finder for: ${item.getFilename()}`); | ||
| item.cancel(); | ||
| }); | ||
|
|
||
| if (nextProgressId) { | ||
| meta.progressId = nextProgressId; | ||
| } | ||
| meta.savePath = savePath; |
Contributor
There was a problem hiding this comment.
Keep macOS progress recreation retryable on failure.
recreateFileProgressAtPath() can return null. This branch still advances meta.savePath, so later syncs stop retrying while meta.progressId still points at a removed progress entry.
💡 Suggested fix
const nextProgressId = mp.recreateFileProgressAtPath(meta.progressId, savePath, () => {
debugPrint("DOWNLOADS", `Cancel requested from Finder for: ${item.getFilename()}`);
item.cancel();
});
if (nextProgressId) {
meta.progressId = nextProgressId;
+ meta.savePath = savePath;
+ } else {
+ meta.progressId = null;
}
- meta.savePath = savePath;
}
}📝 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
| if (meta.savePath && meta.savePath !== savePath) { | |
| const nextProgressId = mp.recreateFileProgressAtPath(meta.progressId, savePath, () => { | |
| debugPrint("DOWNLOADS", `Cancel requested from Finder for: ${item.getFilename()}`); | |
| item.cancel(); | |
| }); | |
| if (nextProgressId) { | |
| meta.progressId = nextProgressId; | |
| } | |
| meta.savePath = savePath; | |
| if (meta.savePath && meta.savePath !== savePath) { | |
| const nextProgressId = mp.recreateFileProgressAtPath(meta.progressId, savePath, () => { | |
| debugPrint("DOWNLOADS", `Cancel requested from Finder for: ${item.getFilename()}`); | |
| item.cancel(); | |
| }); | |
| if (nextProgressId) { | |
| meta.progressId = nextProgressId; | |
| meta.savePath = savePath; | |
| } else { | |
| meta.progressId = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/controllers/downloads-controller/index.ts` around lines 336 - 345,
The code updates meta.savePath even when mp.recreateFileProgressAtPath(...)
returns null, which stops future retries while meta.progressId may point to a
removed entry; change the logic in the block around recreateFileProgressAtPath
so that meta.savePath is only assigned when nextProgressId is truthy (i.e.,
recreate succeeded), and leave meta.savePath and meta.progressId unchanged when
nextProgressId is null (optionally add a debug/log line when recreation fails)
so the next sync can retry recreation.
Comment on lines
+445
to
+460
| updateDownloadRecord(meta.downloadId, { | ||
| originProfileId: meta.originProfileId, | ||
| url: item.getURL(), | ||
| urlChain: this.getUrlChain(item), | ||
| suggestedFilename: item.getFilename(), | ||
| savePath: meta.savePath, | ||
| mimeType: this.emptyToNull(item.getMimeType()), | ||
| state, | ||
| receivedBytes: item.getReceivedBytes(), | ||
| totalBytes: item.getTotalBytes(), | ||
| startTime: this.getDownloadStartTimeMs(item), | ||
| endTime: this.shouldSetEndTime(state, item.canResume()) ? this.getDownloadEndTimeMs(item, now) : null, | ||
| eTag: this.emptyToNull(item.getETag()), | ||
| lastModified: this.emptyToNull(item.getLastModifiedTime()), | ||
| canResume: item.canResume(), | ||
| updatedAt: now |
Contributor
There was a problem hiding this comment.
Don't overwrite persisted metadata with placeholder values.
During resume, buildDownloadInsert() intentionally preserves startTime, mimeType, eTag, and lastModified from the existing row. This snapshot path can immediately replace them with Date.now()/null whenever Electron still reports 0 or "", which both breaks day grouping in the renderer and strips the validators later passed back into createInterruptedDownload().
💡 Suggested fix
+ const mimeType = this.emptyToNull(item.getMimeType());
+ const eTag = this.emptyToNull(item.getETag());
+ const lastModified = this.emptyToNull(item.getLastModifiedTime());
+ const startTimeSeconds = item.getStartTime();
+
updateDownloadRecord(meta.downloadId, {
originProfileId: meta.originProfileId,
url: item.getURL(),
urlChain: this.getUrlChain(item),
suggestedFilename: item.getFilename(),
savePath: meta.savePath,
- mimeType: this.emptyToNull(item.getMimeType()),
+ ...(mimeType !== null ? { mimeType } : {}),
state,
receivedBytes: item.getReceivedBytes(),
totalBytes: item.getTotalBytes(),
- startTime: this.getDownloadStartTimeMs(item),
+ ...(startTimeSeconds > 0 ? { startTime: Math.round(startTimeSeconds * 1000) } : {}),
endTime: this.shouldSetEndTime(state, item.canResume()) ? this.getDownloadEndTimeMs(item, now) : null,
- eTag: this.emptyToNull(item.getETag()),
- lastModified: this.emptyToNull(item.getLastModifiedTime()),
+ ...(eTag !== null ? { eTag } : {}),
+ ...(lastModified !== null ? { lastModified } : {}),
canResume: item.canResume(),
updatedAt: now
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/controllers/downloads-controller/index.ts` around lines 445 - 460,
The updateDownloadRecord call in the resume path is overwriting persisted
metadata with placeholder values from Electron (e.g. 0 or ""), so change the
fields startTime, mimeType, eTag, and lastModified to preserve existing values
when Electron reports placeholders: use getDownloadStartTimeMs(item) only if it
yields a truthy/positive timestamp else keep meta.startTime; use
this.emptyToNull(item.getMimeType()), this.emptyToNull(item.getETag()), and
this.emptyToNull(item.getLastModifiedTime()) only if they return non-null/
non-empty values else keep meta.mimeType, meta.eTag, and meta.lastModified
respectively; leave the rest of the update (including shouldSetEndTime and
received/total bytes) unchanged so buildDownloadInsert/createInterruptedDownload
validators retain original persisted values.
Comment on lines
+496
to
+501
| private getSessionForDownload(originProfileId: string | null): Session { | ||
| if (!originProfileId) { | ||
| return electronSession.defaultSession; | ||
| } | ||
|
|
||
| return electronSession.fromPath(path.join(PROFILES_DIR, originProfileId)); |
Contributor
There was a problem hiding this comment.
Resume should reuse the fully initialized profile session.
electronSession.fromPath() can create a profile session that never went through the normal bootstrap path. If the user resumes a download before opening that profile in this process, the resumed request runs without the same protocol, handler, and intercept setup as the original session.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/controllers/downloads-controller/index.ts` around lines 496 - 501,
getSessionForDownload currently always uses electronSession.fromPath when
originProfileId is set, which can create an uninitialized profile session;
change it to first try returning the fully initialized session instance used by
the app (e.g., the profile/session registry created during bootstrap) and only
fall back to electronSession.fromPath(path.join(PROFILES_DIR, originProfileId))
if no initialized session exists; update getSessionForDownload to check the
app's session manager/registry for originProfileId and return that session,
otherwise return electronSession.defaultSession or the fromPath fallback as now.
Comment on lines
+38
to
+43
| ipcMain.handle("downloads:open-file", (_event, downloadId: string) => { | ||
| const record = getDownloadRecord(downloadId); | ||
| if (!record?.savePath || record.state !== "completed") return false; | ||
| shell.openPath(record.savePath); | ||
| return true; | ||
| }); |
Contributor
There was a problem hiding this comment.
shell.openPath result is not awaited, success always returned.
shell.openPath() returns Promise<string> where an empty string indicates success and a non-empty string contains an error message. The current code returns true immediately without checking the result, so failures are silently ignored and the renderer always receives success.
Await and check the result
-ipcMain.handle("downloads:open-file", (_event, downloadId: string) => {
+ipcMain.handle("downloads:open-file", async (_event, downloadId: string) => {
const record = getDownloadRecord(downloadId);
if (!record?.savePath || record.state !== "completed") return false;
- shell.openPath(record.savePath);
- return true;
+ const error = await shell.openPath(record.savePath);
+ return error === "";
});📝 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
| ipcMain.handle("downloads:open-file", (_event, downloadId: string) => { | |
| const record = getDownloadRecord(downloadId); | |
| if (!record?.savePath || record.state !== "completed") return false; | |
| shell.openPath(record.savePath); | |
| return true; | |
| }); | |
| ipcMain.handle("downloads:open-file", async (_event, downloadId: string) => { | |
| const record = getDownloadRecord(downloadId); | |
| if (!record?.savePath || record.state !== "completed") return false; | |
| const error = await shell.openPath(record.savePath); | |
| return error === ""; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/browser/downloads.ts` around lines 38 - 43, The handler for
"downloads:open-file" currently calls shell.openPath(record.savePath) but
doesn’t await its Promise, so it always returns true; modify the ipcMain.handle
callback (the "downloads:open-file" handler that uses getDownloadRecord) to
await shell.openPath(record.savePath) and check its returned string (empty =
success, non-empty = error message) and return true only on success and false on
error (optionally log or process the error string before returning false).
Comment on lines
+51
to
+61
| ipcMain.handle("downloads:clear-completed", () => { | ||
| const downloads = listDownloads(); | ||
| let changed = false; | ||
| for (const dl of downloads) { | ||
| if (dl.state === "completed" || dl.state === "cancelled") { | ||
| deleteDownloadRecord(dl.id); | ||
| changed = true; | ||
| } | ||
| } | ||
| if (changed) fireDownloadsChanged(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider checking deletion result in clear-completed loop.
deleteDownloadRecord returns a boolean, but the loop ignores individual deletion failures. If a deletion fails, changed is still set to true. This could lead to inconsistent state notifications.
Track actual deletions
ipcMain.handle("downloads:clear-completed", () => {
const downloads = listDownloads();
let changed = false;
for (const dl of downloads) {
if (dl.state === "completed" || dl.state === "cancelled") {
- deleteDownloadRecord(dl.id);
- changed = true;
+ const deleted = deleteDownloadRecord(dl.id);
+ if (deleted) changed = true;
}
}
if (changed) fireDownloadsChanged();
});📝 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
| ipcMain.handle("downloads:clear-completed", () => { | |
| const downloads = listDownloads(); | |
| let changed = false; | |
| for (const dl of downloads) { | |
| if (dl.state === "completed" || dl.state === "cancelled") { | |
| deleteDownloadRecord(dl.id); | |
| changed = true; | |
| } | |
| } | |
| if (changed) fireDownloadsChanged(); | |
| }); | |
| ipcMain.handle("downloads:clear-completed", () => { | |
| const downloads = listDownloads(); | |
| let changed = false; | |
| for (const dl of downloads) { | |
| if (dl.state === "completed" || dl.state === "cancelled") { | |
| const deleted = deleteDownloadRecord(dl.id); | |
| if (deleted) changed = true; | |
| } | |
| } | |
| if (changed) fireDownloadsChanged(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/browser/downloads.ts` around lines 51 - 61, The clear-completed
handler currently sets changed = true regardless of whether
deleteDownloadRecord(dl.id) actually succeeded; update the loop in the
ipcMain.handle("downloads:clear-completed") callback to check the boolean result
of deleteDownloadRecord for each dl and only set changed = true when
deleteDownloadRecord(...) returns true, optionally collect/log failed deletions
and avoid firing fireDownloadsChanged() if no records were actually removed;
keep references to listDownloads(), deleteDownloadRecord(), and
fireDownloadsChanged() so the change is localized to that handler.
Comment on lines
+40
to
+47
| const handleClick = () => { | ||
| if (dl.state === "completed" && !fileMissing) { | ||
| void flow.downloads.openFile(dl.id); | ||
| } else { | ||
| flow.tabs.newTab("flow://downloads", true); | ||
| setOpen(false); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Missing error handling for openFile action.
The DownloadCard component shows a toast on openFile failure, but DownloadRow in the popover silently ignores errors. This inconsistency could confuse users when clicking a completed download fails without feedback.
Add error handling consistent with DownloadCard
+import { toast } from "sonner";
+
// In DownloadRow:
const handleClick = () => {
if (dl.state === "completed" && !fileMissing) {
- void flow.downloads.openFile(dl.id);
+ void flow.downloads.openFile(dl.id).then((ok) => {
+ if (!ok) toast.error("Could not open file");
+ });
} else {
flow.tabs.newTab("flow://downloads", true);
setOpen(false);
}
};📝 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
| const handleClick = () => { | |
| if (dl.state === "completed" && !fileMissing) { | |
| void flow.downloads.openFile(dl.id); | |
| } else { | |
| flow.tabs.newTab("flow://downloads", true); | |
| setOpen(false); | |
| } | |
| }; | |
| const handleClick = () => { | |
| if (dl.state === "completed" && !fileMissing) { | |
| void flow.downloads.openFile(dl.id).then((ok) => { | |
| if (!ok) toast.error("Could not open file"); | |
| }); | |
| } else { | |
| flow.tabs.newTab("flow://downloads", true); | |
| setOpen(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/bottom/downloads-popover.tsx`
around lines 40 - 47, handleClick in downloads-popover.tsx currently calls
flow.downloads.openFile(dl.id) without catching errors, so failures are silent;
update handleClick to mirror DownloadCard by awaiting or handling the promise
from flow.downloads.openFile(dl.id) and catching errors, and on catch show a
user toast (or similar UI feedback) before falling back to opening the downloads
tab and calling setOpen(false); reference handleClick, dl.id,
flow.downloads.openFile, flow.tabs.newTab and setOpen when locating the fix.
Comment on lines
+147
to
+159
| const fetchDownloads = useCallback(async () => { | ||
| try { | ||
| const all = await flow.downloads.list(); | ||
| setDownloads(all); | ||
| const idsToCheck = all.filter((d) => !isActive(d.state) && d.savePath).map((d) => d.id); | ||
| if (idsToCheck.length > 0) { | ||
| const existence = await flow.downloads.checkFilesExist(idsToCheck); | ||
| setFileExistence(existence); | ||
| } | ||
| } catch { | ||
| // silently ignore | ||
| } | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Missing mount guard - potential setState after unmount.
Unlike the DownloadsProvider, this component's fetchDownloads doesn't guard against setting state after the component unmounts. If the popover closes while checkFilesExist is in flight, React will log a warning about setting state on an unmounted component.
Add mount guard
+ const mountedRef = useRef(true);
+
+ useEffect(() => {
+ mountedRef.current = true;
+ return () => {
+ mountedRef.current = false;
+ };
+ }, []);
+
const fetchDownloads = useCallback(async () => {
try {
const all = await flow.downloads.list();
+ if (!mountedRef.current) return;
setDownloads(all);
const idsToCheck = all.filter((d) => !isActive(d.state) && d.savePath).map((d) => d.id);
if (idsToCheck.length > 0) {
const existence = await flow.downloads.checkFilesExist(idsToCheck);
- setFileExistence(existence);
+ if (mountedRef.current) setFileExistence(existence);
}
} catch {
// silently ignore
}
}, []);📝 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
| const fetchDownloads = useCallback(async () => { | |
| try { | |
| const all = await flow.downloads.list(); | |
| setDownloads(all); | |
| const idsToCheck = all.filter((d) => !isActive(d.state) && d.savePath).map((d) => d.id); | |
| if (idsToCheck.length > 0) { | |
| const existence = await flow.downloads.checkFilesExist(idsToCheck); | |
| setFileExistence(existence); | |
| } | |
| } catch { | |
| // silently ignore | |
| } | |
| }, []); | |
| const mountedRef = useRef(true); | |
| useEffect(() => { | |
| mountedRef.current = true; | |
| return () => { | |
| mountedRef.current = false; | |
| }; | |
| }, []); | |
| const fetchDownloads = useCallback(async () => { | |
| try { | |
| const all = await flow.downloads.list(); | |
| if (!mountedRef.current) return; | |
| setDownloads(all); | |
| const idsToCheck = all.filter((d) => !isActive(d.state) && d.savePath).map((d) => d.id); | |
| if (idsToCheck.length > 0) { | |
| const existence = await flow.downloads.checkFilesExist(idsToCheck); | |
| if (mountedRef.current) setFileExistence(existence); | |
| } | |
| } catch { | |
| // silently ignore | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/bottom/downloads-popover.tsx`
around lines 147 - 159, fetchDownloads can call setDownloads/setFileExistence
after the component unmounts; add a mount guard to avoid setState-after-unmount
warnings. Inside the component create a ref like isMounted and set it true on
mount and false in a cleanup useEffect, then inside the useCallback
fetchDownloads check isMounted.current before calling setDownloads and
setFileExistence (and after awaiting flow.downloads.checkFilesExist) so no state
updates occur if the popover has closed; ensure the cleanup toggles the ref to
false to prevent updates from in-flight promises.
Comment on lines
+44
to
+47
| const clearCompleted = async () => { | ||
| await flow.downloads.clearCompleted(); | ||
| toast.success("Cleared completed downloads"); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider awaiting the toast or handling potential errors from clearCompleted.
If flow.downloads.clearCompleted() throws, the toast will still show success. Consider wrapping in try-catch or chaining the toast.
🛡️ Suggested improvement
const clearCompleted = async () => {
- await flow.downloads.clearCompleted();
- toast.success("Cleared completed downloads");
+ try {
+ await flow.downloads.clearCompleted();
+ toast.success("Cleared completed downloads");
+ } catch {
+ toast.error("Failed to clear downloads");
+ }
};📝 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
| const clearCompleted = async () => { | |
| await flow.downloads.clearCompleted(); | |
| toast.success("Cleared completed downloads"); | |
| }; | |
| const clearCompleted = async () => { | |
| try { | |
| await flow.downloads.clearCompleted(); | |
| toast.success("Cleared completed downloads"); | |
| } catch { | |
| toast.error("Failed to clear downloads"); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/downloads/manager/main.tsx` around lines 44 - 47,
The clearCompleted handler currently calls flow.downloads.clearCompleted() then
immediately shows toast.success, which will display success even if
clearCompleted() throws; wrap the await call in a try-catch inside the
clearCompleted function, await flow.downloads.clearCompleted() first, call
toast.success only on success, and on error call toast.error with the caught
error message (and optionally rethrow or log via processLogger) so failures are
handled and reported; reference the clearCompleted function and
flow.downloads.clearCompleted and toast.success/toast.error when making the
change.
Comment on lines
+22
to
+40
| const fetchDownloads = useCallback(async () => { | ||
| try { | ||
| const all = await flow.downloads.list(); | ||
| if (!mountedRef.current) return; | ||
| setDownloads(all); | ||
| setIsError(false); | ||
|
|
||
| // Check file existence for non-active downloads | ||
| const idsToCheck = all.filter((dl) => !isActive(dl.state) && dl.savePath).map((dl) => dl.id); | ||
| if (idsToCheck.length > 0) { | ||
| const existence = await flow.downloads.checkFilesExist(idsToCheck); | ||
| if (mountedRef.current) setFileExistence(existence); | ||
| } | ||
| } catch { | ||
| if (mountedRef.current) setIsError(true); | ||
| } finally { | ||
| if (mountedRef.current) setIsLoading(false); | ||
| } | ||
| }, []); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider clearing stale fileExistence entries on refresh.
When fetchDownloads runs, it only updates fileExistence for downloads currently in the list. If a download is removed, its entry remains in fileExistence state. While this doesn't cause visible bugs (the download card won't render), it's a minor memory leak for long-running sessions.
Clear fileExistence before updating
const fetchDownloads = useCallback(async () => {
try {
const all = await flow.downloads.list();
if (!mountedRef.current) return;
setDownloads(all);
setIsError(false);
// Check file existence for non-active downloads
const idsToCheck = all.filter((dl) => !isActive(dl.state) && dl.savePath).map((dl) => dl.id);
if (idsToCheck.length > 0) {
const existence = await flow.downloads.checkFilesExist(idsToCheck);
if (mountedRef.current) setFileExistence(existence);
+ } else {
+ if (mountedRef.current) setFileExistence({});
}
} catch {📝 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
| const fetchDownloads = useCallback(async () => { | |
| try { | |
| const all = await flow.downloads.list(); | |
| if (!mountedRef.current) return; | |
| setDownloads(all); | |
| setIsError(false); | |
| // Check file existence for non-active downloads | |
| const idsToCheck = all.filter((dl) => !isActive(dl.state) && dl.savePath).map((dl) => dl.id); | |
| if (idsToCheck.length > 0) { | |
| const existence = await flow.downloads.checkFilesExist(idsToCheck); | |
| if (mountedRef.current) setFileExistence(existence); | |
| } | |
| } catch { | |
| if (mountedRef.current) setIsError(true); | |
| } finally { | |
| if (mountedRef.current) setIsLoading(false); | |
| } | |
| }, []); | |
| const fetchDownloads = useCallback(async () => { | |
| try { | |
| const all = await flow.downloads.list(); | |
| if (!mountedRef.current) return; | |
| setDownloads(all); | |
| setIsError(false); | |
| // Check file existence for non-active downloads | |
| const idsToCheck = all.filter((dl) => !isActive(dl.state) && dl.savePath).map((dl) => dl.id); | |
| if (idsToCheck.length > 0) { | |
| const existence = await flow.downloads.checkFilesExist(idsToCheck); | |
| if (mountedRef.current) setFileExistence(existence); | |
| } else { | |
| if (mountedRef.current) setFileExistence({}); | |
| } | |
| } catch { | |
| if (mountedRef.current) setIsError(true); | |
| } finally { | |
| if (mountedRef.current) setIsLoading(false); | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/downloads/manager/provider.tsx` around lines 22 -
40, The fetchDownloads function leaves stale entries in fileExistence when
downloads are removed; modify fetchDownloads in provider.tsx so it clears or
trims fileExistence before/when updating: before calling
flow.downloads.checkFilesExist or immediately after getting all, compute the
current ids (all.map(dl => dl.id)) and call setFileExistence with either an
empty object or a filtered object limited to those ids, then merge/apply the
fresh existence results (from checkFilesExist) so only current downloads remain;
update references to setFileExistence and ensure this runs only when
mountedRef.current is true.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/src/routes/pdf-viewer/pdf-viewer/index.tsx (1)
11-26:⚠️ Potential issue | 🟠 MajorThumbsbar pre-load toggle state is not preserved correctly.
At Line 26,
isThumbsbarOpenis derived fromisDocumentLoaded, butToolbartoggles using this derived value (src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/index.tsx:47-56). Before load, it is alwaysfalse, so user clicks do not reliably represent intent, and the sidebar can open on load unexpectedly.Proposed fix
- const [_isThumbsbarOpen, setIsThumbsbarOpen] = useState(true); + const [isThumbsbarOpen, setIsThumbsbarOpen] = useState(true); @@ - const isThumbsbarOpen = isDocumentLoaded && _isThumbsbarOpen; + const isThumbsbarVisible = isDocumentLoaded && isThumbsbarOpen; @@ - <Toolbar {...{ usePDFSlickStore, setIsThumbsbarOpen, isThumbsbarOpen }} /> + <Toolbar {...{ usePDFSlickStore, setIsThumbsbarOpen, isThumbsbarOpen }} /> @@ - <Thumbsbar {...{ thumbsRef, usePDFSlickStore, isThumbsbarOpen }} /> + <Thumbsbar {...{ thumbsRef, usePDFSlickStore, isThumbsbarOpen: isThumbsbarVisible }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/routes/pdf-viewer/pdf-viewer/index.tsx` around lines 11 - 26, The issue is that isThumbsbarOpen is derived from isDocumentLoaded (isThumbsbarOpen = isDocumentLoaded && _isThumbsbarOpen), so Toolbar toggles that derived value and clicks before load are lost; fix it by keeping the user-intent state separate and only derive the visible state for rendering: keep the state variables _isThumbsbarOpen and setIsThumbsbarOpen as the source of truth, update any toggles in Toolbar to call setIsThumbsbarOpen (not modify the derived isThumbsbarOpen), and introduce a separate visible flag (e.g., thumbsbarVisible = isDocumentLoaded && _isThumbsbarOpen) used only for rendering/passing to PDFSlickViewer; update references in index.tsx and Toolbar to use these symbols (_isThumbsbarOpen, setIsThumbsbarOpen, isDocumentLoaded, and the new thumbsbarVisible).src/renderer/src/hooks/use-favicon-color.ts (1)
217-221:⚠️ Potential issue | 🟠 MajorAdd cleanup guard to prevent stale favicon color updates.
The unguarded promise callback creates a race condition: if
faviconUrlchanges before a pendingextractFaviconColorsresolves, the stale callback will overwrite the newer URL's colors.Suggested fix
useEffect(() => { if (!faviconUrl) { return; } + let cancelled = false; + // Check cache first if (cachedColors) { return; } // Extract colors extractFaviconColors(faviconUrl).then((extractedColors) => { + if (cancelled) return; colorCache.set(faviconUrl, extractedColors); setColors(extractedColors); }); + + return () => { + cancelled = true; + }; }, [faviconUrl, cachedColors]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/hooks/use-favicon-color.ts` around lines 217 - 221, The promise callback from extractFaviconColors can overwrite newer state when faviconUrl changes; modify the effect that calls extractFaviconColors so it captures the current faviconUrl (e.g., const current = faviconUrl) or creates a cancelled flag/AbortController and, in the .then handler, verify the captured current === faviconUrl (or !cancelled) before calling colorCache.set(...) and setColors(...); also ensure the effect cleanup sets the cancelled flag (or aborts the controller) to prevent stale updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint.config.mjs`:
- Around line 10-18: The current rules object overwrites
eslintPluginReactHooks.configs.flat.recommended, removing critical rules; update
the config so the recommended rules are preserved by merging them into your
rules key rather than replacing it — e.g., retrieve
eslintPluginReactHooks.configs.flat.recommended.rules and spread them into the
rules object before applying your overrides for "react-hooks/refs",
"react-hooks/set-state-in-effect", "react-hooks/purity", and
"react-hooks/immutability" so that rules-of-hooks and exhaustive-deps remain
active.
In `@package.json`:
- Line 77: The package.json currently depends on "@types/node": "^24.12.2" which
mismatches the project's Node.js 22 runtime; update the dependency entry for
"@types/node" to a 22.x range (e.g., "^22.0.0" or "^22.x.x") so the TypeScript
Node types align with Node 22, then run your package manager install and
type-check to ensure no further type errors.
- Line 113: Upgrade note: after changing the "typescript" devDependency to
6.0.2, audit and update tsconfig.json and source code for TS 6 breaking
changes—ensure tsconfig explicitly sets rootDir and types (because defaults
changed), remove or replace any deprecated/removed options (target: "es5",
--downlevelIteration, moduleResolution: "node", baseUrl, outFile), migrate any
module { } declarations to namespace, update import assertion syntax to the new
"with" attribute form, and ensure any CLI workflows that run tsc foo.ts in a
directory with tsconfig.json are changed to tsc -p or an explicit file list; run
a full tsc build to identify remaining errors and fix affected files
accordingly.
In `@src/main/ipc/webauthn/index.ts`:
- Around line 88-90: Fix the comment above the cast to CreateCredentialResult:
correct the property path from "prf.first" to "prf.results.first", describe the
types error (electron-webauthn declares prf.results.first as string | undefined
but we expect string), and replace the naked "TODO" with an actionable note
linking to a tracking reference (e.g. a GitHub issue or JIRA ID) and expected
remediation (remove the cast when upstream types are fixed) so future readers
can follow up; reference the file-level symbol (the return of result.data as
CreateCredentialResult in src/main/ipc/webauthn/index.ts) and the
electron-webauthn type mismatch in the comment.
In `@src/renderer/src/hooks/use-favicon-color.ts`:
- Around line 203-205: The cache-hit path in use-favicon-color is treating
cached null as a miss and never hydrating state: change the cache check to test
for existence (cachedColors !== undefined) after calling
colorCache.get(faviconUrl), and when that condition is true call
setColors(cachedColors) to hydrate the hook state; also adjust the colors result
logic so the hook returns the cached value immediately (e.g., use cachedColors
if cachedColors !== undefined else fall back to _colors) instead of relying on
stale _colors only.
In `@tsconfig.web.json`:
- Line 12: The tsconfig.web.json currently sets compilerOptions.types to the npm
package name ("@types/chrome") which prevents TypeScript from loading ambient
lib names; change the value in the "types" array to the ambient library name
"chrome" (i.e., replace "@types/chrome" with "chrome") so compilerOptions.types
correctly references the type library.
---
Outside diff comments:
In `@src/renderer/src/hooks/use-favicon-color.ts`:
- Around line 217-221: The promise callback from extractFaviconColors can
overwrite newer state when faviconUrl changes; modify the effect that calls
extractFaviconColors so it captures the current faviconUrl (e.g., const current
= faviconUrl) or creates a cancelled flag/AbortController and, in the .then
handler, verify the captured current === faviconUrl (or !cancelled) before
calling colorCache.set(...) and setColors(...); also ensure the effect cleanup
sets the cancelled flag (or aborts the controller) to prevent stale updates.
In `@src/renderer/src/routes/pdf-viewer/pdf-viewer/index.tsx`:
- Around line 11-26: The issue is that isThumbsbarOpen is derived from
isDocumentLoaded (isThumbsbarOpen = isDocumentLoaded && _isThumbsbarOpen), so
Toolbar toggles that derived value and clicks before load are lost; fix it by
keeping the user-intent state separate and only derive the visible state for
rendering: keep the state variables _isThumbsbarOpen and setIsThumbsbarOpen as
the source of truth, update any toggles in Toolbar to call setIsThumbsbarOpen
(not modify the derived isThumbsbarOpen), and introduce a separate visible flag
(e.g., thumbsbarVisible = isDocumentLoaded && _isThumbsbarOpen) used only for
rendering/passing to PDFSlickViewer; update references in index.tsx and Toolbar
to use these symbols (_isThumbsbarOpen, setIsThumbsbarOpen, isDocumentLoaded,
and the new thumbsbarVisible).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43b36352-f877-4e22-860e-c34dcac27e71
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
eslint.config.mjspackage.jsonsrc/main/ipc/webauthn/index.tssrc/renderer/src/components/browser-ui/browser-content.tsxsrc/renderer/src/components/settings/sections/spaces/section.tsxsrc/renderer/src/components/ui/resizable-sidebar.tsxsrc/renderer/src/components/ui/sidebar.tsxsrc/renderer/src/hooks/use-css-size-to-pixels.tsxsrc/renderer/src/hooks/use-favicon-color.tssrc/renderer/src/routes/error/page.tsxsrc/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/SearchBar.tsxsrc/renderer/src/routes/pdf-viewer/pdf-viewer/index.tsxsrc/shared/types/fido2-types.tstsconfig.node.jsontsconfig.scripts.jsontsconfig.web.json
💤 Files with no reviewable changes (5)
- src/renderer/src/routes/error/page.tsx
- src/renderer/src/routes/pdf-viewer/pdf-viewer/Toolbar/SearchBar.tsx
- src/renderer/src/components/settings/sections/spaces/section.tsx
- src/renderer/src/hooks/use-css-size-to-pixels.tsx
- src/renderer/src/components/browser-ui/browser-content.tsx
Comment on lines
10
to
18
| { | ||
| settings: { | ||
| react: { | ||
| version: "detect" | ||
| } | ||
| files: ["**/*.{ts,tsx}"], | ||
| ...eslintPluginReactHooks.configs.flat.recommended, | ||
| rules: { | ||
| "react-hooks/refs": "off", | ||
| "react-hooks/set-state-in-effect": "off", | ||
| "react-hooks/purity": "off", | ||
| "react-hooks/immutability": "off" | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: rules object overwrites recommended rules from spread.
When an object property is defined after a spread, it completely replaces rather than merges with the spread value. The rules object on lines 13-17 will overwrite the rules from eslintPluginReactHooks.configs.flat.recommended, losing critical rules like rules-of-hooks and exhaustive-deps.
🐛 Proposed fix to preserve recommended rules
{
files: ["**/*.{ts,tsx}"],
...eslintPluginReactHooks.configs.flat.recommended,
rules: {
+ ...eslintPluginReactHooks.configs.flat.recommended.rules,
"react-hooks/refs": "off",
"react-hooks/set-state-in-effect": "off",
"react-hooks/purity": "off",
"react-hooks/immutability": "off"
}
},📝 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
| { | |
| settings: { | |
| react: { | |
| version: "detect" | |
| } | |
| files: ["**/*.{ts,tsx}"], | |
| ...eslintPluginReactHooks.configs.flat.recommended, | |
| rules: { | |
| "react-hooks/refs": "off", | |
| "react-hooks/set-state-in-effect": "off", | |
| "react-hooks/purity": "off", | |
| "react-hooks/immutability": "off" | |
| } | |
| { | |
| files: ["**/*.{ts,tsx}"], | |
| ...eslintPluginReactHooks.configs.flat.recommended, | |
| rules: { | |
| ...eslintPluginReactHooks.configs.flat.recommended.rules, | |
| "react-hooks/refs": "off", | |
| "react-hooks/set-state-in-effect": "off", | |
| "react-hooks/purity": "off", | |
| "react-hooks/immutability": "off" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint.config.mjs` around lines 10 - 18, The current rules object overwrites
eslintPluginReactHooks.configs.flat.recommended, removing critical rules; update
the config so the recommended rules are preserved by merging them into your
rules key rather than replacing it — e.g., retrieve
eslintPluginReactHooks.configs.flat.recommended.rules and spread them into the
rules object before applying your overrides for "react-hooks/refs",
"react-hooks/set-state-in-effect", "react-hooks/purity", and
"react-hooks/immutability" so that rules-of-hooks and exhaustive-deps remain
active.
| "@types/d3-selection": "^3.0.11", | ||
| "@types/jju": "^1.4.5", | ||
| "@types/node": "^22.19.11", | ||
| "@types/node": "^24.12.2", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if `@types/node`@24.12.2 exists on npm
curl -s "https://registry.npmjs.org/@types/node/24.12.2" | jq '.version // "NOT FOUND"'Repository: MultiboxLabs/flow-browser
Length of output: 78
🏁 Script executed:
cat -n .nvmrcRepository: MultiboxLabs/flow-browser
Length of output: 79
🏁 Script executed:
sed -n '77p' package.jsonRepository: MultiboxLabs/flow-browser
Length of output: 99
Update @types/node version to match Node.js 22.
The project uses Node.js 22 (as specified in .nvmrc), but @types/node@^24.12.2 is intended for Node.js 24. This mismatch can cause type inconsistencies and build issues. Change the dependency to @types/node@^22.x.x to align with the Node.js version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 77, The package.json currently depends on
"@types/node": "^24.12.2" which mismatches the project's Node.js 22 runtime;
update the dependency entry for "@types/node" to a 22.x range (e.g., "^22.0.0"
or "^22.x.x") so the TypeScript Node types align with Node 22, then run your
package manager install and type-check to ensure no further type errors.
| "tw-animate-css": "^1.2.9", | ||
| "typescript": "^5.8.3", | ||
| "vite": "^8.0.5" | ||
| "typescript": "^6.0.2", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TypeScript 6.0 release date breaking changes
💡 Result:
TypeScript 6.0 release date (stable): March 23, 2026. [1]
Breaking changes / deprecations called out for TypeScript 6.0
From the official 6.0 release notes (and mirrored in the announcement post): [1][2]
-
New defaults
rootDirnow defaults to.(current directory). [2]typesnow defaults to[](no automatic@types/*inclusion unless brought in via other means). [2]
-
Deprecated / removed compiler options & modes
- Deprecated:
target: "es5". [2] - Deprecated:
--downlevelIteration. [2] - Deprecated:
--moduleResolution node(akanode10). [2] - Deprecated:
modulevaluesamd,umd, andsystemjs. [2] - Deprecated:
--baseUrl. [2] - Deprecated:
--moduleResolution classic. [2] - Deprecated (effectively):
--esModuleInterop falseand--allowSyntheticDefaultImports false— the “safer interop” behavior is always enabled in 6.0. [2] - Deprecated (effectively):
--alwaysStrict false— 6.0 assumes JavaScript strict mode semantics. [2] - Removed:
--outFile. [2]
- Deprecated:
-
Syntax / directive changes
module {}for internal namespaces is now a hard deprecation (usenamespace;declare module "pkg"is still supported). [1][2]- Import assertions syntax is now an error:
import x from "./x.json" asserts { type: "json" }→ use import attributes withwith. [2] /// <reference no-default-lib="true"/>no longer supported (use--noLib/--libReplacementinstead). [2]
-
CLI behavior change
- Running
tsc foo.tsin a directory that has atsconfig.jsonis now an error (use--ignoreConfigif you intended to bypass the config). [2]
- Running
Sources: TypeScript team announcement (Microsoft DevBlogs) [1] and the official TypeScript 6.0 release notes [2].
Review TypeScript 6.0 breaking changes before upgrading.
TypeScript 6.0.2 was released on March 23, 2026 and includes significant breaking changes that may impact this codebase:
- New defaults:
rootDirnow defaults to.andtypesdefaults to[](no automatic@types/*inclusion) - Deprecated/removed:
es5target,--downlevelIteration,--moduleResolution node,--baseUrl,--outFile - Syntax changes:
module {}is now a hard deprecation (usenamespace), import assertions syntax requires migration to import attributes (with) - CLI behavior: Running
tsc foo.tsin a directory withtsconfig.jsonis now an error
Verify that tsconfig.json and the codebase do not rely on removed/deprecated options and update import syntax accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 113, Upgrade note: after changing the "typescript"
devDependency to 6.0.2, audit and update tsconfig.json and source code for TS 6
breaking changes—ensure tsconfig explicitly sets rootDir and types (because
defaults changed), remove or replace any deprecated/removed options (target:
"es5", --downlevelIteration, moduleResolution: "node", baseUrl, outFile),
migrate any module { } declarations to namespace, update import assertion syntax
to the new "with" attribute form, and ensure any CLI workflows that run tsc
foo.ts in a directory with tsconfig.json are changed to tsc -p or an explicit
file list; run a full tsc build to identify remaining errors and fix affected
files accordingly.
Comment on lines
+88
to
+90
| // types error in electron-webauthn (TODO: fix this) | ||
| // result.data.extensions.prf.first should be `string`, but its `string | undefined` in the package types | ||
| return result.data as CreateCredentialResult; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Typo in comment and consider more actionable tracking for the workaround.
The comment references prf.first but the actual path is prf.results.first based on the type definition.
Additionally, the TODO would benefit from linking to a tracking issue or specifying when/how this will be addressed—naked TODO comments tend to be forgotten.
📝 Suggested comment fix
- // types error in electron-webauthn (TODO: fix this)
- // result.data.extensions.prf.first should be `string`, but its `string | undefined` in the package types
+ // WORKAROUND: electron-webauthn types `prf.results.first` as `string | undefined`,
+ // but when `results` is present, `first` is guaranteed per WebAuthn spec.
+ // TODO(electron-webauthn): Upstream fix or add runtime validation if spec changes.
return result.data as CreateCredentialResult;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/webauthn/index.ts` around lines 88 - 90, Fix the comment above
the cast to CreateCredentialResult: correct the property path from "prf.first"
to "prf.results.first", describe the types error (electron-webauthn declares
prf.results.first as string | undefined but we expect string), and replace the
naked "TODO" with an actionable note linking to a tracking reference (e.g. a
GitHub issue or JIRA ID) and expected remediation (remove the cast when upstream
types are fixed) so future readers can follow up; reference the file-level
symbol (the return of result.data as CreateCredentialResult in
src/main/ipc/webauthn/index.ts) and the electron-webauthn type mismatch in the
comment.
Comment on lines
+203
to
205
| const cachedColors = faviconUrl ? colorCache.get(faviconUrl) : null; | ||
| const colors = faviconUrl ? _colors : null; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/src/hooks/use-favicon-color.ts | head -230 | tail -50Repository: MultiboxLabs/flow-browser
Length of output: 1675
Cache-hit path is broken and can return stale/null colors indefinitely.
When a favicon URL exists in the cache, the hook checks cachedColors at line 212 and exits early without calling setColors(). This means the state _colors (returned at line 204) is never hydrated with the cached value, so the hook returns null despite colors being available. Additionally, the truthy check at line 212 fails for cached null values (a valid cached result), causing redundant extraction attempts and inability to distinguish between "no cache entry" and "cached null".
The fix needs to:
- Distinguish "no cache entry" (undefined) from "cached null" using
cachedColors !== undefined - Hydrate state with
setColors(cachedColors)when a cache hit occurs - Return the cached value immediately via ternary logic instead of relying solely on stale state
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/hooks/use-favicon-color.ts` around lines 203 - 205, The
cache-hit path in use-favicon-color is treating cached null as a miss and never
hydrating state: change the cache check to test for existence (cachedColors !==
undefined) after calling colorCache.get(faviconUrl), and when that condition is
true call setColors(cachedColors) to hydrate the hook state; also adjust the
colors result logic so the hook returns the cached value immediately (e.g., use
cachedColors if cachedColors !== undefined else fall back to _colors) instead of
relying on stale _colors only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit