feat: introduce windows controller#162
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRe-architects main-process window, tab, and profile management from a monolithic Browser/WindowManager to controller-based singletons. Adds a windows-controller with typed window classes, a loaded-profiles-controller, and a tabs-controller. Updates IPC, app lifecycle, menus, and utilities to use the new controllers. Tooling changes include bun-based scripts and production minify gating. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Main Entry
participant WinCtrl as windowsController
participant BWIF as browserWindowsController
participant Tabs as tabsController
participant Profiles as loadedProfilesController
User->>App: Launch app
App->>BWIF: create("normal")
BWIF->>WinCtrl: browser.new() (typed)
WinCtrl-->>BWIF: BrowserWindow
App->>Profiles: load(profileId) (on demand)
App->>Tabs: createTab(windowId, profileId, spaceId)
Tabs-->>App: Tab
sequenceDiagram
autonumber
participant OS as OS/Deep Link
participant App as Main
participant URL as handleOpenUrl
participant WinIF as browserWindowsController
participant Tabs as tabsController
OS->>App: Open URL
App->>URL: handleOpenUrl(useNewWindow, url)
URL->>WinIF: getFocusedWindow()/getWindows()
alt No windows or useNewWindow
URL->>WinIF: create("normal")
end
URL->>Tabs: createTab(windowId, profileId?, spaceId?)
Tabs-->>URL: Tab
URL->>WinIF: show window
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Build artifacts for all platforms are ready! 🚀Download the artifacts from: (execution 18883930161 / attempt 1) |
…owserWindow class
…oller and related classes
iamEvanYT
force-pushed
the
evan/windows-controller
branch
from
35bb082 to
d1a86e7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (28)
src/main/controllers/tabs-controller/tab.ts (4)
448-456:URL.parseis not a function on the global URL class.Use
new URL(...)with try/catch. Current code will throw at runtime.- const url = URL.parse(this.url); - - if (url) { - if (WHITELISTED_PROTOCOLS.includes(url.protocol)) { + let parsed: URL | null = null; + try { parsed = new URL(this.url); } catch {} + + if (parsed) { + if (WHITELISTED_PROTOCOLS.includes(parsed.protocol)) { this.view.setBackgroundColor(COLOR_TRANSPARENT); } else { this.view.setBackgroundColor(COLOR_BACKGROUND); } } else {
719-721: SameURL.parsebug in error-page guard.Use
new URL(url).- const parsedURL = URL.parse(url); - if (parsedURL && parsedURL.protocol === "flow:" && parsedURL.hostname === "error") { + let parsedURL: URL | null = null; + try { parsedURL = new URL(url); } catch {} + if (parsedURL && parsedURL.protocol === "flow:" && parsedURL.hostname === "error") { return; }
271-273: Redundant assignment ofthis.window.
setWindow(window)already setsthis.window. Drop the extra line.// Setup window this.setWindow(window); - this.window = window;
664-680: SimplifysetWindowand avoid null‑checks on a non‑nullable param.Tighten logic; fewer branches.
- public setWindow(window: BrowserWindow, index: number = TAB_ZINDEX) { + public setWindow(window: BrowserWindow, index: number = TAB_ZINDEX) { const windowChanged = this.window !== window; if (windowChanged) { // Remove view from old window this.removeViewFromWindow(); } // Add view to new window - if (window) { - this.window = window; - window.viewManager.addOrUpdateView(this.view, index); - } + this.window = window; + window.viewManager.addOrUpdateView(this.view, index); if (windowChanged) { this.emit("window-changed"); } }src/main/saving/tabs.ts (2)
124-150: Restore active/focused tabs per window/spaceOn session restore you create windows and tabs but never restore which tab should be active. Users may land on arbitrary tabs.
- For each window/space, pick the tab with the latest lastActiveAt (or persisted focused/active if available) and call tabsController.setActiveTab(tab).
- Optionally set window.currentSpaceId to the space containing the most recent tab.
136-147: Review comment is valid—confirm fragile positional undefined pattern.The target call in src/main/saving/tabs.ts:139 is the only call site passing explicit
undefinedfor thewebContentsViewOptionsparameter to access thetabCreationOptionsobject. The concern is warranted: this pattern is fragile and could break if the signature changes. The recommendation to refactor using an options object or function overload is a sound improvement.src/main/controllers/app-menu-controller/menu/items/spaces.ts (2)
35-41: Avoid require.resolve in ESM/Bun contextrequire.resolve may not exist under Bun/ESM. Use import.meta.resolve or a package export.
As per coding guidelines
- require.resolve("@phosphor-icons/core"), + new URL(await import.meta.resolve("@phosphor-icons/core")).pathname,
148-162: Consistency: focus sourceYou validate focus via windowsController/browsersWindowsManager, but click handler fetches focus via getFocusedBrowserWindow() (a different path). Prefer a single source to avoid edge discrepancies.
- Pass the validated focused window id into createSpaceMenuItem or resolve focus via windowsController in the click.
src/main/ipc/browser/tabs.ts (2)
59-69: Dedup via Set to avoid O(n²) scanswindowProfiles/windowSpaces grow via includes checks. Use Set for linear traversal.
- const windowProfiles: string[] = []; - const windowSpaces: string[] = []; + const windowProfiles = new Set<string>(); + const windowSpaces = new Set<string>(); ... - if (!windowProfiles.includes(tab.profileId)) { - windowProfiles.push(tab.profileId); - } - if (!windowSpaces.includes(tab.spaceId)) { - windowSpaces.push(tab.spaceId); - } + windowProfiles.add(tab.profileId); + windowSpaces.add(tab.spaceId); ... - for (const spaceId of windowSpaces) { + for (const spaceId of Array.from(windowSpaces)) {
230-235: Position updates may create duplicates/holesMoving a grouped tab sets the same newPosition for all tabs in the group without shifting surrounding tabs. Consider delegating to tabsController for reindexing or normalize positions after updates.
src/main/controllers/windows-controller/utils/browser/portal-component-windows.ts (1)
12-26: Guard feature parsing and deny by defaultdetails.features can be empty; parsing works but consider explicit default‑deny with explicit allowlist to reduce accidental popups without componentId.
- electronWindow.webContents.setWindowOpenHandler((details) => { + electronWindow.webContents.setWindowOpenHandler((details) => { const { features } = details; - const componentId = features + const componentId = (features || "") .split(",") .find((f) => f.startsWith("componentId=")) ?.split("=")[1]; if (!componentId) { ... return { action: "deny" }; }Ensure the caller of initializePortalComponentWindows invokes the returned destroy() on window close to prevent leaked listeners.
src/main/app/urls.ts (1)
5-13: Bug: URL.parse is not a functionGlobal URL has no parse(). Use the WHATWG URL constructor with try/catch.
-export function isValidOpenerUrl(url: string): boolean { - const urlObject = URL.parse(url); - if (!urlObject) return false; - const VALID_PROTOCOLS = ["http:", "https:"]; - if (!VALID_PROTOCOLS.includes(urlObject.protocol)) return false; - return true; -} +export function isValidOpenerUrl(url: string): boolean { + try { + const u = new URL(url); + return u.protocol === "http:" || u.protocol === "https:"; + } catch { + return false; + } +}src/main/browser.ts (1)
13-14: Remove unused Browser singleton export and classThe
browserconstant andBrowserclass are unused and contradict the refactoring objective to move window/tab orchestration to controllers. The class contains only a TODO comment and placeholder code (sendMessageToCoreWebContentsis mostly commented out). All orchestration is already handled bybrowserWindowsControllerand related controllers throughout the codebase.Remove:
src/main/browser.tsexport and instantiationsrc/main/browser/browser.ts(or if it contains other utilities, extract only what's needed)- The dynamic import from
src/main/index.tssrc/main/ipc/browser/navigation.ts (2)
34-48: Tab status handler is sound; consider future-proofing payloadThe handler safely guards navigationHistory and returns minimal state. Optionally include current URL/title later to reduce an extra round-trip.
Ensure navigationHistory.getAllEntries() doesn’t leak large objects across IPC; if heavy, consider mapping to a lean DTO.
50-55: Add bounds check before calling goToIndex to prevent process crashesElectron's navigationHistory.goToIndex throws when index is out of bounds. The current code passes
indexdirectly without validation. Add a guard usingnavigationHistory.length()or wrap in try-catch:ipcMain.on("navigation:go-to-entry", (_event, tabId: number, index: number) => { const tab = tabsController.getTabById(tabId); if (!tab) return; const hist = tab.webContents?.navigationHistory; if (!hist) return; if (index >= 0 && index < hist.length()) { hist.goToIndex(index); } });src/main/controllers/tabs-controller/context-menu.ts (3)
128-147: Await tab creation when opening link in a new windowCreation of the window is awaited, but the subsequent tab creation isn’t—race can leave the new window unfocused/empty briefly.
- click: async () => { - const newWindow = await browserWindowsController.create(); - createNewTab(parameters.linkURL, newWindow); - } + click: async () => { + const newWindow = await browserWindowsController.create(); + await createNewTab(parameters.linkURL, newWindow); + }
186-191: Remove ts-expect-error by typing getContextMenuItemsRelying on // @ts-expect-error hides regressions. Export a proper interface for extensions.getContextMenuItems(...) or add a type guard here.
- // @ts-expect-error: ts error, but still works - const items: Electron.MenuItemConstructorOptions[] = extensions.getContextMenuItems(tab.webContents, parameters); + const items = (extensions.getContextMenuItems as ( + wc: Electron.WebContents, + params: Electron.ContextMenuParams + ) => Electron.MenuItemConstructorOptions[])(tab.webContents, parameters);Or better, type the extensions collection itself.
231-248: Hardcoded search providerSearch engine is hardcoded to Google. Consider sourcing from user settings or a shared default to stay consistent across the app.
src/main/ipc/app/extensions.ts (1)
54-69: Boolean normalization can be simplifiedNit: prefer idiomatic coercion.
- enabled: extensionData.disabled ? false : true, - pinned: extensionData.pinned ? true : false, + enabled: !extensionData.disabled, + pinned: !!extensionData.pinned,src/main/controllers/tabs-controller/tab-groups/index.ts (2)
142-147: Critical: possibleundefinedpassed intosetWindow(number).
tab.getWindow()?.idcan beundefined. The inequality check will be true, andsetWindow(undefined as any)corrupts state.Apply this fix:
- const onTabWindowChanged = () => { - const newWindowId = tab.getWindow()?.id; - if (newWindowId !== this.windowId) { - this.setWindow(newWindowId); - } - }; + const onTabWindowChanged = () => { + const win = tab.getWindow(); + if (!win) return; + const newWindowId = win.id; + if (newWindowId !== this.windowId) { + this.setWindow(newWindowId); + } + };
211-214: Guardpositionwhen the group is empty.If all tabs are removed before destroy,
this.tabs[0]isundefined.Apply this diff:
public get position(): number { this.errorIfDestroyed(); - return this.tabs[0].position; + const first = this.tabs[0]; + if (!first) throw new Error("TabGroup has no tabs"); + return first.position; }src/main/controllers/windows-controller/utils/browser/omnibox.ts (2)
213-218: Cleanup: remove listeners and clear map on destroy to prevent leaks.
destroy()only closes the WebContents; it doesn’t unsubscribe or clearomniboxes.Apply this minimal fix:
destroy() { this.assertNotDestroyed(); this.isDestroyed = true; - this.webContents.close(); + // Unsubscribe listeners if applicable (see note below) + try { this.webContents.close(); } catch {} + omniboxes.delete(this.window); }Recommended follow-up (outside this hunk): store the
blurandresizehandlers as class fields andremoveListenerthem here.
82-120: Round bounds to integers before applying.Electron window/view bounds expect integers; rounding avoids subpixel artifacts.
Apply this diff:
- const newX = clamp(this.bounds.x, 0, windowBounds.width); - const newY = clamp(this.bounds.y, 0, windowBounds.height); - const newWidth = clamp(this.bounds.width, 0, windowBounds.width - newX); - const newHeight = clamp(this.bounds.height, 0, windowBounds.height - newY); + const newX = Math.round(clamp(this.bounds.x, 0, windowBounds.width)); + const newY = Math.round(clamp(this.bounds.y, 0, windowBounds.height)); + const newWidth = Math.round(clamp(this.bounds.width, 0, windowBounds.width - newX)); + const newHeight = Math.round(clamp(this.bounds.height, 0, windowBounds.height - newY));- const omniboxX = windowBounds.width / 2 - omniboxWidth / 2; - const omniboxY = windowBounds.height / 2 - omniboxHeight / 2; + const omniboxX = Math.round(windowBounds.width / 2 - omniboxWidth / 2); + const omniboxY = Math.round(windowBounds.height / 2 - omniboxHeight / 2);src/main/controllers/tabs-controller/index.ts (4)
79-94: Archive interval never cleared; add explicit destroy or tie to app shutdownYou set an interval but only clear it on "destroyed", which is never emitted (destroy() is commented out). This leaks timers in tests/hot‑reloads.
Apply a minimal lifecycle:
@@ - const interval = setInterval(() => { + this.archiveInterval = setInterval(() => { @@ - this.on("destroyed", () => { - clearInterval(interval); - }); + this.on("destroyed", () => { + if (this.archiveInterval) clearInterval(this.archiveInterval); + }); } + + private archiveInterval?: NodeJS.Timeout; + private _isDestroyed = false; + + public destroy() { + if (this._isDestroyed) return; + this._isDestroyed = true; + if (this.archiveInterval) clearInterval(this.archiveInterval); + this.emit("destroyed"); + }
397-401: Guard focus against destroyed WebContentsCalling focus() on a destroyed webContents can throw. Add a safety check.
- this.spaceFocusedTabMap.set(windowSpaceReference, tab); - tab.webContents.focus(); // Ensure the tab's web contents is focused + this.spaceFocusedTabMap.set(windowSpaceReference, tab); + if (!tab.webContents.isDestroyed()) { + tab.webContents.focus(); + }
625-635: Comment/code mismatch for group activation criterionComment says “If any of the initial tabs were active…”, but code only checks the first tab. Fix either the comment or implement “any tab” logic.
- // If any of the initial tabs were active, make the new group active. + // If any of the initial tabs were active, make the new group active. @@ - if (this.getActiveTab(firstTab.getWindow().id, firstTab.spaceId)?.id === firstTab.id) { + const active = this.getActiveTab(firstTab.getWindow().id, firstTab.spaceId); + const anyInitialActive = + (active instanceof Tab && initialTabs.some(t => t.id === active.id)) || + (active instanceof BaseTabGroup && active.tabs.some(t => initialTabs.some(it => it.id === t.id))); + if (anyInitialActive) { this.setActiveTab(tabGroup);
643-651: Sentinel 999 can be wrong; use Infinity with a fallbackIf all positions > 999, returning 999 is incorrect.
- public getSmallestPosition(): number { - let smallestPosition = 999; + public getSmallestPosition(): number { + let smallestPosition = Number.POSITIVE_INFINITY; @@ - return smallestPosition; + return Number.isFinite(smallestPosition) ? smallestPosition : 0; }src/main/browser/browser.ts (1)
35-50: Make destroy() idempotent, not throwingThrowing on repeated destroy can crash shutdown paths. Return early instead.
public destroy(): void { - if (this._isDestroyed) { - throw new Error("Browser already destroyed!"); - } + if (this._isDestroyed) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (74)
.vscode/settings.json(1 hunks)electron.vite.config.ts(2 hunks)package.json(2 hunks)src/main/app/instance.ts(1 hunks)src/main/app/lifecycle.ts(2 hunks)src/main/app/onboarding.ts(1 hunks)src/main/app/platform.ts(3 hunks)src/main/app/urls.ts(2 hunks)src/main/browser.ts(2 hunks)src/main/browser/browser.ts(2 hunks)src/main/browser/events.ts(0 hunks)src/main/browser/profile-manager.ts(0 hunks)src/main/browser/utility/protocols/_protocols/flow-internal.ts(3 hunks)src/main/browser/utility/protocols/_protocols/flow.ts(2 hunks)src/main/browser/window-manager.ts(0 hunks)src/main/browser/window.ts(0 hunks)src/main/controllers/README.md(1 hunks)src/main/controllers/app-menu-controller/index.ts(2 hunks)src/main/controllers/app-menu-controller/menu/helpers.ts(1 hunks)src/main/controllers/app-menu-controller/menu/items/app.ts(1 hunks)src/main/controllers/app-menu-controller/menu/items/archive.ts(2 hunks)src/main/controllers/app-menu-controller/menu/items/edit.ts(2 hunks)src/main/controllers/app-menu-controller/menu/items/file.ts(1 hunks)src/main/controllers/app-menu-controller/menu/items/spaces.ts(4 hunks)src/main/controllers/app-menu-controller/menu/items/view.ts(4 hunks)src/main/controllers/index.ts(1 hunks)src/main/controllers/loaded-profiles-controller/index.ts(1 hunks)src/main/controllers/posthog-controller/posthog-error-capture-sdk/index.ts(1 hunks)src/main/controllers/profiles-controller/index.ts(4 hunks)src/main/controllers/quit-controller/handlers/before-quit.ts(1 hunks)src/main/controllers/tabs-controller/bounds.ts(1 hunks)src/main/controllers/tabs-controller/context-menu.ts(5 hunks)src/main/controllers/tabs-controller/index.ts(10 hunks)src/main/controllers/tabs-controller/tab-groups/glance.ts(1 hunks)src/main/controllers/tabs-controller/tab-groups/index.ts(9 hunks)src/main/controllers/tabs-controller/tab-groups/split.ts(1 hunks)src/main/controllers/tabs-controller/tab.ts(18 hunks)src/main/controllers/windows-controller/index.ts(1 hunks)src/main/controllers/windows-controller/interfaces/browser.ts(1 hunks)src/main/controllers/windows-controller/interfaces/onboarding.ts(1 hunks)src/main/controllers/windows-controller/interfaces/settings.ts(1 hunks)src/main/controllers/windows-controller/type-manager.ts(1 hunks)src/main/controllers/windows-controller/types/base.ts(1 hunks)src/main/controllers/windows-controller/types/browser.ts(1 hunks)src/main/controllers/windows-controller/types/extension-popup.ts(1 hunks)src/main/controllers/windows-controller/types/index.ts(1 hunks)src/main/controllers/windows-controller/types/onboarding.ts(1 hunks)src/main/controllers/windows-controller/types/settings.ts(1 hunks)src/main/controllers/windows-controller/utils/browser/omnibox.ts(4 hunks)src/main/controllers/windows-controller/utils/browser/portal-component-windows.ts(3 hunks)src/main/controllers/windows-controller/utils/close-preventer.ts(2 hunks)src/main/ipc/README.md(1 hunks)src/main/ipc/app/actions.ts(1 hunks)src/main/ipc/app/extensions.ts(7 hunks)src/main/ipc/app/new-tab.ts(1 hunks)src/main/ipc/browser/browser.ts(1 hunks)src/main/ipc/browser/interface.ts(1 hunks)src/main/ipc/browser/navigation.ts(2 hunks)src/main/ipc/browser/page.ts(2 hunks)src/main/ipc/browser/tabs.ts(12 hunks)src/main/ipc/listeners-manager.ts(1 hunks)src/main/ipc/session/profiles.ts(2 hunks)src/main/ipc/session/spaces.ts(4 hunks)src/main/ipc/window/omnibox.ts(1 hunks)src/main/ipc/window/settings.ts(1 hunks)src/main/modules/content-blocker.ts(3 hunks)src/main/modules/icons.ts(3 hunks)src/main/modules/output.ts(1 hunks)src/main/modules/typed-event-emitter.ts(1 hunks)src/main/modules/windows.ts(0 hunks)src/main/onboarding/main.ts(0 hunks)src/main/saving/onboarding.ts(2 hunks)src/main/saving/tabs.ts(5 hunks)src/main/settings/main.ts(0 hunks)
💤 Files with no reviewable changes (7)
- src/main/browser/window-manager.ts
- src/main/modules/windows.ts
- src/main/browser/window.ts
- src/main/browser/events.ts
- src/main/onboarding/main.ts
- src/main/browser/profile-manager.ts
- src/main/settings/main.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
**/*.{js,jsx,ts,tsx}: Usebun <file>instead ofnode <file>orts-node <file>
Bun automatically loads .env, so don't use dotenv.
UseBun.serve()for HTTP servers and WebSockets instead ofexpress.
Usebun:sqlitefor SQLite instead ofbetter-sqlite3.
UseBun.redisfor Redis instead ofioredis.
UseBun.sqlfor Postgres instead ofpgorpostgres.js.
Use built-inWebSocketinstead ofws.
PreferBun.fileovernode:fs's readFile/writeFile.
UseBun.$for shell commands instead of execa.
Files:
src/main/controllers/windows-controller/types/settings.tssrc/main/controllers/tabs-controller/bounds.tssrc/main/controllers/windows-controller/types/index.tselectron.vite.config.tssrc/main/controllers/app-menu-controller/menu/items/app.tssrc/main/app/instance.tssrc/main/controllers/app-menu-controller/menu/items/edit.tssrc/main/ipc/window/settings.tssrc/main/controllers/windows-controller/types/extension-popup.tssrc/main/app/platform.tssrc/main/ipc/app/extensions.tssrc/main/controllers/app-menu-controller/menu/items/file.tssrc/main/saving/tabs.tssrc/main/ipc/app/actions.tssrc/main/ipc/app/new-tab.tssrc/main/app/onboarding.tssrc/main/controllers/windows-controller/types/onboarding.tssrc/main/app/urls.tssrc/main/controllers/tabs-controller/tab-groups/glance.tssrc/main/controllers/app-menu-controller/menu/items/archive.tssrc/main/controllers/windows-controller/index.tssrc/main/controllers/tabs-controller/tab-groups/split.tssrc/main/ipc/browser/browser.tssrc/main/controllers/app-menu-controller/menu/items/spaces.tssrc/main/ipc/listeners-manager.tssrc/main/app/lifecycle.tssrc/main/modules/content-blocker.tssrc/main/controllers/windows-controller/interfaces/browser.tssrc/main/controllers/posthog-controller/posthog-error-capture-sdk/index.tssrc/main/controllers/profiles-controller/index.tssrc/main/browser.tssrc/main/controllers/app-menu-controller/menu/items/view.tssrc/main/ipc/session/profiles.tssrc/main/controllers/index.tssrc/main/browser/utility/protocols/_protocols/flow-internal.tssrc/main/ipc/session/spaces.tssrc/main/ipc/browser/page.tssrc/main/controllers/tabs-controller/context-menu.tssrc/main/ipc/browser/interface.tssrc/main/controllers/windows-controller/types/browser.tssrc/main/controllers/windows-controller/types/base.tssrc/main/controllers/windows-controller/interfaces/settings.tssrc/main/controllers/windows-controller/utils/close-preventer.tssrc/main/controllers/app-menu-controller/menu/helpers.tssrc/main/ipc/browser/tabs.tssrc/main/controllers/windows-controller/interfaces/onboarding.tssrc/main/ipc/browser/navigation.tssrc/main/ipc/window/omnibox.tssrc/main/controllers/windows-controller/utils/browser/omnibox.tssrc/main/modules/icons.tssrc/main/controllers/tabs-controller/tab.tssrc/main/controllers/windows-controller/utils/browser/portal-component-windows.tssrc/main/controllers/windows-controller/type-manager.tssrc/main/controllers/loaded-profiles-controller/index.tssrc/main/saving/onboarding.tssrc/main/browser/utility/protocols/_protocols/flow.tssrc/main/modules/typed-event-emitter.tssrc/main/modules/output.tssrc/main/controllers/tabs-controller/tab-groups/index.tssrc/main/controllers/tabs-controller/index.tssrc/main/browser/browser.tssrc/main/controllers/quit-controller/handlers/before-quit.tssrc/main/controllers/app-menu-controller/index.ts
**/*.{html,ts,css}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuild
Files:
src/main/controllers/windows-controller/types/settings.tssrc/main/controllers/tabs-controller/bounds.tssrc/main/controllers/windows-controller/types/index.tselectron.vite.config.tssrc/main/controllers/app-menu-controller/menu/items/app.tssrc/main/app/instance.tssrc/main/controllers/app-menu-controller/menu/items/edit.tssrc/main/ipc/window/settings.tssrc/main/controllers/windows-controller/types/extension-popup.tssrc/main/app/platform.tssrc/main/ipc/app/extensions.tssrc/main/controllers/app-menu-controller/menu/items/file.tssrc/main/saving/tabs.tssrc/main/ipc/app/actions.tssrc/main/ipc/app/new-tab.tssrc/main/app/onboarding.tssrc/main/controllers/windows-controller/types/onboarding.tssrc/main/app/urls.tssrc/main/controllers/tabs-controller/tab-groups/glance.tssrc/main/controllers/app-menu-controller/menu/items/archive.tssrc/main/controllers/windows-controller/index.tssrc/main/controllers/tabs-controller/tab-groups/split.tssrc/main/ipc/browser/browser.tssrc/main/controllers/app-menu-controller/menu/items/spaces.tssrc/main/ipc/listeners-manager.tssrc/main/app/lifecycle.tssrc/main/modules/content-blocker.tssrc/main/controllers/windows-controller/interfaces/browser.tssrc/main/controllers/posthog-controller/posthog-error-capture-sdk/index.tssrc/main/controllers/profiles-controller/index.tssrc/main/browser.tssrc/main/controllers/app-menu-controller/menu/items/view.tssrc/main/ipc/session/profiles.tssrc/main/controllers/index.tssrc/main/browser/utility/protocols/_protocols/flow-internal.tssrc/main/ipc/session/spaces.tssrc/main/ipc/browser/page.tssrc/main/controllers/tabs-controller/context-menu.tssrc/main/ipc/browser/interface.tssrc/main/controllers/windows-controller/types/browser.tssrc/main/controllers/windows-controller/types/base.tssrc/main/controllers/windows-controller/interfaces/settings.tssrc/main/controllers/windows-controller/utils/close-preventer.tssrc/main/controllers/app-menu-controller/menu/helpers.tssrc/main/ipc/browser/tabs.tssrc/main/controllers/windows-controller/interfaces/onboarding.tssrc/main/ipc/browser/navigation.tssrc/main/ipc/window/omnibox.tssrc/main/controllers/windows-controller/utils/browser/omnibox.tssrc/main/modules/icons.tssrc/main/controllers/tabs-controller/tab.tssrc/main/controllers/windows-controller/utils/browser/portal-component-windows.tssrc/main/controllers/windows-controller/type-manager.tssrc/main/controllers/loaded-profiles-controller/index.tssrc/main/saving/onboarding.tssrc/main/browser/utility/protocols/_protocols/flow.tssrc/main/modules/typed-event-emitter.tssrc/main/modules/output.tssrc/main/controllers/tabs-controller/tab-groups/index.tssrc/main/controllers/tabs-controller/index.tssrc/main/browser/browser.tssrc/main/controllers/quit-controller/handlers/before-quit.tssrc/main/controllers/app-menu-controller/index.ts
package.json
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
package.json: Usebun installinstead ofnpm installoryarn installorpnpm install
Usebun run <script>instead ofnpm run <script>oryarn run <script>orpnpm run <script>
Files:
package.json
🧠 Learnings (2)
📚 Learning: 2025-03-21T23:17:56.301Z
Learnt from: iamEvanYT
PR: MultiboxLabs/flow-browser#2
File: electron/browser/menu.ts:36-47
Timestamp: 2025-03-21T23:17:56.301Z
Learning: The `setOmniboxBounds` function accepts `null` as a valid argument, which means "use the default bounds" rather than requiring explicit bounds.
Applied to files:
src/main/controllers/windows-controller/utils/browser/omnibox.ts
📚 Learning: 2025-07-24T08:49:07.217Z
Learnt from: CR
PR: MultiboxLabs/flow-browser#0
File: .cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc:0-0
Timestamp: 2025-07-24T08:49:07.217Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use `bun <file>` instead of `node <file>` or `ts-node <file>`
Applied to files:
package.json
🪛 markdownlint-cli2 (0.18.1)
src/main/controllers/README.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (69)
.vscode/settings.json (1)
10-11: Good: enforces workspace TypeScript + JSON fix.The trailing comma makes the JSON valid, and setting
typescript.tsdkto the workspace TypeScript prevents editor/runtime version drift. Verification confirms TypeScript ^5.8.3 is declared as a devDependency andnode_modules/typescript/libexists.src/main/controllers/tabs-controller/tab.ts (1)
492-497: Verify BrowserWindow option keys for popup positioning.Electron uses
x/y(notleft/top) inBrowserWindowConstructorOptions. ConfirminstantCreatemapsleft/toptox/y; otherwise the popup may ignore position.If needed:
- ...(parsedFeatures.top ? { top: +parsedFeatures.top } : {}), - ...(parsedFeatures.left ? { left: +parsedFeatures.left } : {}) + ...(parsedFeatures.top ? { y: +parsedFeatures.top } : {}), + ...(parsedFeatures.left ? { x: +parsedFeatures.left } : {})src/main/controllers/posthog-controller/posthog-error-capture-sdk/index.ts (1)
8-8: Import path verification passed—no issues found.The
./stack-parserfile exists at the expected location withcreateStackParserproperly exported. The import will resolve correctly under all bundling scenarios (Electron/Vite).src/main/controllers/profiles-controller/index.ts (2)
162-179: Guard initial-profile side effects and API surface.The review comment is largely valid. I confirmed:
- Line 163: Method is
public async _setupInitialProfile()— a public method with underscore prefix violates naming conventions (underscores conventionally signal private intent).- Line 183:
queueMicrotask(() => profilesController._setupInitialProfile());runs unconditionally at module import time with no environment gating.The concern about unintended initialization during tests or secondary processes is legitimate. However, I could not locate other profile bootstrappers or
_createcalls in the codebase via ripgrep to confirm the full scope of potential race conditions.Recommendations from review remain actionable:
- Rename
_setupInitialProfiletosetupInitialProfile(remove underscore) or make it truly private.- Gate the
queueMicrotaskcall with environment checks (e.g.,process.type === "browser"and NODE_ENV checks).
84-97: Review comment is incorrect. No changes needed.The current boolean return type is appropriate for this API. While the review suggests returning
{success, profileId, profileData}, analysis shows:
Actual caller behavior: The renderer code only logs the boolean result and doesn't access or use a returned
profileId. It doesn't destructure the return value.Event-driven pattern already exists: The
"profile-created"event (emitted at line 89) already communicates bothprofileIdandprofileDatato any interested listeners. The renderer can subscribe to this event if needed.Callers only need success/failure status: The internal
_setupInitialProfile()method only checksif (!profileCreated)to determine success. The IPC handler returns the boolean directly to the preload layer, which is the expected contract.No actual demand for returned data: No code in the codebase currently attempts to access
profileIdfrom thecreate()return value, as it's auto-generated internally and communicated separately via events.Implementing the suggested change would introduce unnecessary breaking changes across the IPC layer, preload API, and potentially renderer code, without addressing any real caller need.
Likely an incorrect or invalid review comment.
src/main/controllers/windows-controller/types/base.ts (4)
41-60: LGTM: WebContents aggregation logic is correct.The method properly collects the main window's
webContentsand iterates through child views to gather additionalwebContentsfromWebContentsViewinstances.
62-75: LGTM: Show/hide methods are well-designed.The
show()method with optional focus parameter provides good control, and the implementations are straightforward wrappers around the underlyingBrowserWindowAPI.
77-81: LGTM: Message broadcasting implementation is correct.The method properly broadcasts messages to all
webContentsassociated with the window by leveraginggetAllWebContents().
83-88: LGTM: Window lifecycle setup is correct.The
closedevent handler correctly triggers cleanup viadestroy(), which safely handles the already-destroyed state by checkingisDestroyed()before attempting any window operations.src/main/ipc/window/settings.ts (1)
4-4: LGTM: Import path updated to use windows controller interface.The import path change aligns with the new controller-based architecture for window management.
src/main/controllers/windows-controller/types/extension-popup.ts (1)
1-9: LGTM: Extension popup window implementation is clean.The class properly extends
BaseWindowand leverages the base class for all lifecycle and messaging functionality. The comment clarifies that thebrowserWindowis pre-created by the extension handler.src/main/controllers/tabs-controller/bounds.ts (1)
1-1: LGTM: Import path updated to relative path.The change from absolute alias to relative import aligns with the controller-based restructuring and improves module locality.
src/main/controllers/tabs-controller/tab-groups/glance.ts (1)
1-1: LGTM: Import path updated to local index.The change to import from
"./index"aligns with the centralized tab-groups architecture and improves module organization.src/main/controllers/app-menu-controller/menu/items/app.ts (1)
2-2: LGTM: Import path updated to windows controller interface.The import path change correctly uses the new controller-based settings interface.
src/main/controllers/tabs-controller/tab-groups/split.ts (1)
1-1: LGTM: Import path updated to local index.The change to import from
"./index"aligns with the centralized tab-groups architecture, consistent with the glance tab group implementation.src/main/ipc/browser/page.ts (1)
2-17: The review comment is based on a misunderstanding of IPC handler timing.The removal of optional chaining on
browserWindowsControlleris safe. Here's why:
browserWindowsController is always available: It's a synchronous object literal exported at module initialization, never null or undefined.
IPC handlers can only be invoked from renderer processes: The "page:set-bounds" message is sent from the preload script (renderer side). Renderer processes only exist within BrowserWindow instances, which are created after
app.whenReady().Practical execution order:
src/main.ts→@/browser→@/ipc/main(registers handlers) → windows created afterapp.whenReady()→ IPC messages can now fire. By the time a renderer can send the IPC message,browserWindowsControllerandbrowserWindowsManagerare fully initialized.The actual null check is on line 18:
if (!window) return;properly handles cases wheregetWindowFromWebContents()returns null or undefined. The optional chaining on the controller itself was unnecessary.Likely an incorrect or invalid review comment.
src/main/saving/onboarding.ts (1)
1-21: LGTM! Clean controller integration.The refactor correctly uses the new controller interfaces. The logic flow remains intact: onboarding is hidden, and a browser window is created if none exist.
src/main/app/onboarding.ts (1)
1-18: LGTM! Import update aligns with controller architecture.The change correctly updates the onboarding import to use the new controller interface while preserving all existing logic.
src/main/browser/utility/protocols/_protocols/flow.ts (1)
1-132: LGTM! Clean migration to loaded-profiles controller.The refactor correctly replaces the browser singleton with the new
loadedProfilesController. The null check on line 108 properly guards against missing profiles, maintaining the same safety guarantees.src/main/controllers/app-menu-controller/menu/items/edit.ts (1)
1-28: LGTM! Simplified window access pattern.The refactor correctly uses the new
getFocusedBrowserWindow()helper, maintaining null safety while simplifying the code. The window is properly validated before passing tofireCopyLinkAction.src/main/modules/icons.ts (1)
1-226: LGTM! Controller-based window management applied correctly.The refactor properly integrates with the windows controller:
getAllWindows()retrieves all windows for icon updates- The
window-addedevent ensures new windows receive the current icon- Window access via
window.browserWindowaligns with the controller's typed window interfacesrc/main/controllers/quit-controller/handlers/before-quit.ts (1)
1-37: LGTM! Session management correctly migrated to controller.The refactor properly sources loaded profile sessions from the new
loadedProfilesController. The session flushing logic remains intact, ensuring data is properly persisted before quit.src/main/modules/content-blocker.ts (1)
1-136: LGTM! Content blocker correctly integrated with profiles controller.The refactor properly:
- Sources loaded profiles from
loadedProfilesController.getAll()- Listens to the
profile-loadedevent from the controller- Maintains the same content blocking logic flow
src/main/modules/typed-event-emitter.ts (1)
4-5: LGTM: Type alias improves consistencyThe
EmitterEventsTypealias provides a standardized event payload type for controller-based event communication introduced in this PR.src/main/app/lifecycle.ts (2)
6-6: LGTM: Function signature updated for controller-based architectureThe removal of the
Browserparameter aligns with the PR's goal to streamline window handling through the new controller system.
23-23: No issues found—browserWindowsController.create()signature is correctThe
createmethod insrc/main/controllers/windows-controller/interfaces/browser.tsis defined as:create: async (type: BrowserWindowType = "normal", options: BrowserWindowCreationOptions = {}) => { ... }Both parameters have default values, so calling it with zero arguments—as done at line 23 of
lifecycle.ts—is fully valid. The code is correct as written.src/main/browser/utility/protocols/_protocols/flow-internal.ts (1)
19-19: LGTM: Successfully migrated to controller-based tab lookupThe replacement of browser-based tab access with
tabsController.getTabById()aligns with the PR's architectural goals while preserving the existing validation logic.Also applies to: 41-41
src/main/app/instance.ts (2)
9-16: LGTM: Clean migration to controller-based URL handlingThe updated function signature and async handler properly delegate to
handleOpenUrlwith the computedshouldCreateflag, aligning with the PR's architectural goals.
18-18: Good addition: Debug logging improves observabilityThe
debugPrintcall enhances debugging capabilities for second instance handling.src/main/ipc/listeners-manager.ts (1)
6-6: Removal ofsendMessageToListenersInWindowverified as completeMultiple comprehensive searches (broad, case-insensitive, across all file types) found zero references to
sendMessageToListenersInWindowthroughout the codebase. The function has been fully removed with no remaining dependencies.src/main/controllers/windows-controller/utils/close-preventer.ts (1)
21-22: Verification Complete:menuCloseTab()has been properly refactoredThe function signature has been correctly updated to accept no parameters. It now obtains the necessary window context internally via
getFocusedWindow(), which delegates towindowsController.getFocused(). This is the intended refactoring pattern: the call site inclose-preventer.tsat line 22 correctly callsmenuCloseTab()without arguments, and the implementation has full access to the browser window context it needs through the controller.src/main/ipc/app/actions.ts (1)
4-6: No issues found—sendMessage method exists and is properly implementedVerification confirms that
BrowserWindowextendsBaseWindow, which implements thesendMessagepublic method atsrc/main/controllers/windows-controller/types/base.ts(lines 78–81). The method is already in use elsewhere in the codebase (e.g.,src/main/ipc/session/spaces.tsline 54). The code is correct as-is.src/main/ipc/window/omnibox.ts (1)
1-42: LGTM! Clean migration to controller-based window management.The refactor properly:
- Uses
windowsController.getWindowFromWebContentsinstead of direct BrowserWindow access- Adds runtime type guards with
browserWindowsManager.isInstanceOfbefore omnibox operations- Migrates from global omnibox functions to instance methods on the window
src/main/controllers/windows-controller/types/index.ts (1)
1-6: LGTM! Standard barrel export pattern.The barrel export consolidates window type controllers into a single entry point, which is a clean pattern for the new controller-based architecture.
src/main/controllers/app-menu-controller/menu/items/file.ts (1)
7-25: LGTM! Clean migration from Browser-centric to controller-based approach.The refactor properly:
- Removes the Browser parameter dependency
- Uses
getFocusedBrowserWindow()to retrieve the active window dynamically- Migrates to
browserWindowsController.create()for window creationsrc/main/ipc/session/profiles.ts (1)
23-33: LGTM! Consistent controller-based refactor.The changes properly migrate to the controller pattern:
- Uses
browserWindowsController.getWindowFromWebContentsinstead of direct window access- Accesses
window.currentSpaceIdproperty instead of method call- Maintains explicit null return for missing window/space
src/main/controllers/app-menu-controller/menu/items/archive.ts (1)
5-37: LGTM! Clean refactor with improved navigation guards.The changes properly:
- Remove Browser parameter dependency
- Use
getTabWcFromFocusedWindow()for focused tab retrieval- Add defensive
canGoBack()andcanGoForward()checks before navigation, preventing potential errorssrc/main/controllers/app-menu-controller/index.ts (1)
35-39: LGTM! Clean migration to parameterless menu creators.The menu creation calls correctly use the new parameterless signatures, aligning with the controller-based architecture where menu items dynamically retrieve focused windows/tabs instead of receiving a Browser instance.
src/main/app/urls.ts (1)
24-30: Both APIs are correctly implemented and used as written.Verification confirms:
browserWindowsController.getFocusedWindow()exists and is properly defined insrc/main/controllers/windows-controller/interfaces/browser.tswindow.show(true)accepts a boolean parameter (defined atsrc/main/controllers/windows-controller/types/base.ts:62) that controls whether the window receives focus after being shownThe code review comment's concerns are unfounded; no changes are needed.
src/main/ipc/app/new-tab.ts (1)
12-18: The null usage is intentional API design, not a code smell.Both
loadInterface()andsetBounds()explicitly acceptnullin their type signatures and have well-defined behavior for null inputs:
loadInterface(null)loads the omnibox interface without query parameterssetBounds(null)triggers calculation of default centered bounds (max 750×350)The implementation encapsulates all null-handling logic, so there are no downstream null checks. This pattern is consistent throughout the codebase and aligns with the design where null serves as a sentinel value for "use defaults."
src/main/ipc/browser/navigation.ts (2)
20-33: Tab stop/reload wiring looks goodLookup via tabsController.getTabById and webContents stop/reload with null-guards is correct.
5-18: Review comment is incorrect—handler properly uses send (one-way) patternThe preload layer exposes
navigationAPI.goTo()at line 278 of src/preload/index.ts usingipcRenderer.send("navigation:go-to", url, tabId), which is intentionally one-way messaging. The handler correctly usesipcMain.on()for this pattern. While the return values in the handler are never used, this is not a functional issue—the caller does not expect a response. Switching toipcMain.handleandipcRenderer.invokewould be incorrect because the API is designed as fire-and-forget.Likely an incorrect or invalid review comment.
src/main/controllers/tabs-controller/context-menu.ts (2)
271-290: Section composition/separators look correctNon-empty sections only; single separator between sections. Good.
52-60: ****The suggested diff to await
loadURLandsetActiveTabis incorrect—both methods are synchronous and cannot be awaited.The code is correct as written:
createNewTabproperly awaitstabsController.createTab(the async operation), then immediately calls the synchronousloadURLandsetActiveTabmethods without await. The fire-and-forget invocation pattern in the click handlers is acceptable for UI event handlers.Likely an incorrect or invalid review comment.
src/main/ipc/session/spaces.ts (3)
27-34: Window resolution + setCurrentSpace looks correctFetching window via controller and updating current space prior to spacesController.setLastUsed is consistent.
36-42: Returning currentSpaceId is fineNull-safe; aligns with controller-based window model.
52-55: No action required—review comment appears to be based on incorrect assumptions.The signature change referenced does not introduce breaking changes. Evidence shows:
TabbedBrowserWindowdoes not exist in the codebase (only mentioned in documentation)- All call sites at lines 135, 501, 230, and 160 already pass
BrowserWindow-typed valuessetWindowSpace()already expectsBrowserWindowand is compatible with all current usagesTab.getWindow()returnsBrowserWindow,browserWindowsController.instantCreate()returns a window object compatible with the function signatureThere is no signature incompatibility or ripple effect to verify.
Likely an incorrect or invalid review comment.
src/main/app/platform.ts (2)
18-42: macOS dock menu: controller-based create is correctRemoving Browser from the API and delegating to browserWindowsController.create() is aligned with the refactor.
44-52: No issues found after verificationAll callers of
setupPlatformIntegration()have been correctly updated to use the arg-less signature. The single call site at src/main/browser.ts:23 invokes the function with no arguments, matching the new definition. The Browser-related import (browserWindowsController) in platform.ts is actively used and not stale.src/main/ipc/app/extensions.ts (3)
73-87: Profile lookup via loadedProfilesController is correctEarly-return empty array on missing profile prevents errors. Good.
90-100: Space/profile resolution from WebContents is cleanNull-guards are thorough; aligns with windowsController and spacesController responsibilities.
140-152: No changes needed; type is correctThe verification confirms that
window.browserWindowis properly typed as the native Electron.BrowserWindow instance. The import on line 3 of browser.ts showsBrowserWindow as ElectronBrowserWindowfrom the "electron" package, and this is correctly instantiated and stored in the wrapper'sbrowserWindowproperty. Passing it todialog.showMessageBox()is safe and matches the API expectations.src/main/controllers/app-menu-controller/menu/items/view.ts (1)
11-13: “Close Tab” closes non-tabbed windows — confirm UX.If the focused window isn’t a managed browser window, this path closes it. That can unintentionally close settings/onboarding. Consider no-op or limiting to popup browser windows only.
Would you prefer ignoring the action for non-browser windows, or only closing
browserWindowType === "popup"?src/main/ipc/browser/interface.ts (1)
90-107: LGTM: window state + event broadcast align with controller abstractions.API boundary between ElectronWindow and BrowserWindow wrapper is clean.
src/main/controllers/windows-controller/utils/browser/omnibox.ts (1)
191-199: LGTM: focus check viabrowserWindowsControllerpreserves UX.Keeps omnibox open when focus is outside the app; matches the intended behavior.
src/main/controllers/windows-controller/type-manager.ts (4)
12-14: Drop explicit any and the eslint-disable; use unknown[].Avoids suppressing lint and improves type safety.
[ suggest_recommended_refactor ]-// eslint-disable-next-line @typescript-eslint/no-explicit-any -export class WindowTypeManager<C extends new (...args: any[]) => BaseWindow> { +export class WindowTypeManager<C extends new (...args: unknown[]) => BaseWindow> {
31-37: Add a runtime type check to catch constructor/type mismatches early.Defensive assert ensures the provided constructor truly yields the declared WindowType.
[ suggest_essential_refactor ]private _new(...args: ConstructorParameters<C>): InstanceType<C> { const WindowConstructor = this.windowConstructor; const window = new WindowConstructor(...args) as InstanceType<C>; + if (window.type !== this.windowType) { + throw new Error( + `WindowType mismatch: expected "${this.windowType}", got "${window.type}"` + ); + } this.windowsController._addWindow(window); return window; }
14-17: Mark immutable fields as readonly.Prevents accidental mutation of controller/type/options after construction.
[ suggest_recommended_refactor ]- private windowsController: WindowsController; - private windowType: WindowType; - private readonly windowConstructor: C; - private options: WindowTypeManagerOptions; + private readonly windowsController: WindowsController; + private readonly windowType: WindowType; + private readonly windowConstructor: C; + private readonly options: WindowTypeManagerOptions;
39-43: Consider renamingnew→createfor clarity.Avoids confusion with the
newoperator and improves readability. Requires updating call sites.
[ suggest_optional_refactor ]- public new(...args: ConstructorParameters<C>): InstanceType<C> { + public create(...args: ConstructorParameters<C>): InstanceType<C> { this._checkNotSingleton(); return this._new(...args); }src/main/controllers/windows-controller/index.ts (3)
45-54: Guard against duplicate window IDs when registering.Avoid silent overwrite/leak if a duplicate id is encountered.
[ raise_major_issue ]public _addWindow(window: BaseWindow) { const id = window.id; - this.windows.set(id, window); + if (this.windows.has(id)) { + debugPrint("WINDOWS", "Duplicate window id detected; replacing existing entry", id); + this._removeWindow(id); + } + this.windows.set(id, window); this.emit("window-added", id, window); - window.browserWindow.on("focus", () => this.emit("window-focused", id, window)); - window.on("destroyed", () => this._removeWindow(id)); + const onFocus = () => this.emit("window-focused", id, window); + window.browserWindow.on("focus", onFocus); + window.on("destroyed", () => { + this._removeWindow(id); + // Tidy up listener on underlying BrowserWindow + window.browserWindow.removeListener("focus", onFocus); + });
67-75: Optional: track lastFocusedId to avoid O(n) scans.Maintain
lastFocusedIdon the "window-focused" event to makegetFocused()O(1).
[ suggest_optional_refactor ]
95-102: SimplifygetIdFromWindowand ensure membership check.Leverage the window’s own id while still verifying it’s tracked.
[ suggest_recommended_refactor ]- public getIdFromWindow(window: BaseWindow) { - for (const [id, w] of this.windows.entries()) { - if (w === window) { - return id; - } - } - return null; - } + public getIdFromWindow(window: BaseWindow) { + return this.windows.has(window.id) ? window.id : null; + }src/main/controllers/app-menu-controller/menu/helpers.ts (2)
29-33: Nullish-checkspaceId; current falsy check rejects valid 0.If
currentSpaceIdcan be 0,if (!spaceId)drops a valid space.
[ raise_major_issue ]- const spaceId = window.currentSpaceId; - if (!spaceId) return null; + const spaceId = window.currentSpaceId; + if (spaceId == null) return null;
4-4: Use a type-only import for WebContents.Avoids unnecessary runtime import.
[ suggest_recommended_refactor ]-import { WebContents } from "electron"; +import { type WebContents } from "electron";src/main/controllers/windows-controller/interfaces/browser.ts (2)
22-28: Guard readiness wait with try/catch; warn once on failure.Prevents unhandled rejection from aborting window creation in environments without Widevine.
[ suggest_essential_refactor ]- const isReady = await waitForElectronComponentsToBeReady(); - if (!isReady) { - warnWidevineLoadFailed(); - } + try { + const isReady = await waitForElectronComponentsToBeReady(); + if (!isReady) { + warnWidevineLoadFailed(); + } + } catch { + warnWidevineLoadFailed(); + }
75-85: Fix JSDoc/return mismatch fordestroyAll.Either return a boolean or update the doc to reflect void. Suggest updating the doc to avoid implying guarantees.
[ suggest_recommended_refactor ]- /** - * Destroys all windows - * @param force Whether to force destroy the windows (Default: false) - * @returns Whether the windows were destroyed - */ + /** + * Destroys all windows. + * @param force Whether to force destroy the windows (Default: false). + */package.json (2)
33-47: Preferbun:sqliteoverbetter-sqlite3.Project guideline mandates Bun’s sqlite. Plan migration to avoid native deps and align with runtime.
[ suggest_essential_refactor ]
As per coding guidelines.
97-102: Confirmpnpmblock is still required.If builds/tests use Bun exclusively, drop the
pnpmstanza to reduce confusion.
[ request_verification ]#!/bin/bash # Find CI configs referencing pnpm; if none, it's safe to remove the pnpm block. fd -a -H -t f -E node_modules -E dist -E build | rg -nP '\bpnpm\b' -C2src/main/controllers/loaded-profiles-controller/index.ts (1)
41-43: Incorrect Electron type: BaseWindow doesn’t exist in public typingsUse BrowserWindow or your app’s BaseWindow type. This likely fails TS.
- parent?: Electron.BaseWindow; + parent?: ElectronBrowserWindow;Likely an incorrect or invalid review comment.
| export function processInitialUrl() { | ||
| const commandLine = process.argv.slice(1); | ||
| const targetUrl = commandLine.pop(); | ||
| if (targetUrl && isValidOpenerUrl(targetUrl)) { | ||
| handleOpenUrl(browser, targetUrl); | ||
| handleOpenUrl(false, targetUrl); | ||
| debugPrint("INITIALIZATION", "initial URL handled"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid fire‑and‑forget initial URL
processInitialUrl() calls handleOpenUrl without awaiting, which can race other startup flows (e.g., onboarding). Consider making processInitialUrl async and returning a handled flag so the caller can sequence.
🤖 Prompt for AI Agents
In src/main/app/urls.ts around lines 32-39, processInitialUrl currently calls
handleOpenUrl fire-and-forget which can race startup flows; change
processInitialUrl to be async, await the call to handleOpenUrl (i.e., await
handleOpenUrl(false, targetUrl)), and return a boolean handled flag (true when
targetUrl exists and isValidOpenerUrl and handleOpenUrl completes successfully,
false otherwise) so callers can sequence startup steps; also update any callers
to await processInitialUrl() and act on the returned flag.
| processInitialUrl(); | ||
|
|
||
| // Setup second instance handler | ||
| setupSecondInstanceHandling(browser); | ||
| setupSecondInstanceHandling(); | ||
|
|
||
| // Setup platform specific features | ||
| setupPlatformIntegration(browser); | ||
| setupPlatformIntegration(); | ||
|
|
||
| // Open onboarding / create initial window | ||
| runOnboardingOrInitialWindow(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Startup race: initial URL vs onboarding can create two windows
processInitialUrl() triggers handleOpenUrl asynchronously, then runOnboardingOrInitialWindow() runs unconditionally. If no windows exist yet, both paths can create one each. Gate onboarding on “any window exists” or await URL handling before deciding.
Apply one of:
- Make processInitialUrl async and return a boolean “handled”; only call runOnboardingOrInitialWindow() if false.
- Or poll windowsController.getWindows().length after URL handling before onboarding.
🤖 Prompt for AI Agents
In src/main/browser.ts around lines 17 to 26, processInitialUrl() is called but
not awaited, so its async handling can race with runOnboardingOrInitialWindow()
and create two windows; change processInitialUrl to be async and return a
boolean "handled" (true if a URL resulted in opening/activating a window),
update its callers to await it, and only call runOnboardingOrInitialWindow() if
the returned handled is false; ensure processInitialUrl awaits handleOpenUrl (or
equivalent) and properly catches/logs errors so the boolean outcome is accurate.
src/main/browser/browser.ts
Outdated
| /** | ||
| * Sends a message to all core WebContents | ||
| * TODO: remove this placeholder function and replace with new one | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| public sendMessageToCoreWebContents(channel: string, ...args: any[]) { | ||
| for (const window of this.getWindows()) { | ||
| window.sendMessageToCoreWebContents(channel, ...args); | ||
| } | ||
| // for (const window of this.getWindows()) { | ||
| // window.sendMessageToCoreWebContents(channel, ...args); | ||
| // } | ||
| settings.sendMessage(channel, ...args); | ||
| onboarding.sendMessage(channel, ...args); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Deprecated shim: route callers to controller surfaces
This placeholder should be removed or callers migrated to controller messaging (settings/onboarding/windows). Add deprecation note and plan removal.
Run to find call sites:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C2 '\bsendMessageToCoreWebContents\s*\('Length of output: 1638
I need to verify which implementation is actually being used by the call sites. Let me check the type definitions and imports.
🏁 Script executed:
#!/bin/bash
# Check type of window in tabs.ts
echo "=== tabs.ts imports and type context ==="
head -30 src/main/ipc/browser/tabs.ts
echo ""
echo "=== interface.ts imports and type context ==="
head -30 src/main/ipc/browser/interface.ts
# Check class hierarchy for BrowserWindow
echo ""
echo "=== Check browser.ts class definition ==="
head -30 src/main/browser/browser.ts
echo ""
echo "=== Check windows-controller browser.ts class definition ==="
head -30 src/main/controllers/windows-controller/types/browser.tsLength of output: 576
🏁 Script executed:
#!/bin/bash
# Check imports and type definitions in call site files
echo "=== tabs.ts imports ==="
head -40 src/main/ipc/browser/tabs.ts | grep -E "^import|^from|BrowserWindow|window"
echo ""
echo "=== interface.ts imports ==="
head -40 src/main/ipc/browser/interface.ts | grep -E "^import|^from|BrowserWindow|win"
# Check class hierarchy for placeholder implementation
echo ""
echo "=== browser.ts class definition ==="
head -40 src/main/browser/browser.ts | grep -E "^export|^class|extends"
# Check class hierarchy for proper implementation
echo ""
echo "=== windows-controller browser.ts class definition ==="
head -40 src/main/controllers/windows-controller/types/browser.ts | grep -E "^export|^class|extends"
# Get more context on what type window/win are in call sites
echo ""
echo "=== Context around tabs.ts call site ==="
sed -n '110,125p' src/main/ipc/browser/tabs.ts
echo ""
echo "=== Context around interface.ts call site ==="
sed -n '15,25p' src/main/ipc/browser/interface.tsLength of output: 3109
🏁 Script executed:
#!/bin/bash
# Check if the placeholder Browser class is actually instantiated/exported/used
echo "=== Search for Browser class usage ==="
rg -nP --type=ts '\bBrowser\b' src/main/browser/browser.ts | head -5
echo ""
echo "=== Search for exports from browser.ts ==="
rg -nP --type=ts '^export' src/main/browser/browser.ts
echo ""
echo "=== Search for who imports Browser from src/main/browser ==="
rg -nP --type=ts 'from.*["\'].*browser\.ts|from.*["\']@/browser' | grep -v node_modules | head -10
echo ""
echo "=== Full class definition of placeholder ==="
sed -n '1,70p' src/main/browser/browser.tsLength of output: 546
🏁 Script executed:
#!/bin/bash
# Search for who imports the Browser class from src/main/browser
echo "=== Search for Browser class imports ==="
rg -nP --type=ts "import.*Browser.*from.*browser" | head -15
echo ""
echo "=== Search for new Browser() instantiation ==="
rg -nP --type=ts "new\s+Browser\s*\(" | head -10
echo ""
echo "=== Check if placeholder sendMessageToCoreWebContents is used ==="
rg -nP --type=ts "\.sendMessageToCoreWebContents" src/main/browser/
echo ""
echo "=== Search in all TS files for references to sendMessageToCoreWebContents ==="
rg -nP --type=ts "\.sendMessageToCoreWebContents" --type=ts | wc -lLength of output: 2332
🏁 Script executed:
#!/bin/bash
# Get all 3 references to sendMessageToCoreWebContents
echo "=== All references to sendMessageToCoreWebContents ==="
rg -nP --type=ts "\.sendMessageToCoreWebContents" -A1 -B1Length of output: 672
Remove the placeholder sendMessageToCoreWebContents method — it has no active callers
All discovered call sites already use the proper BrowserWindow implementation from the windows-controller (src/main/controllers/windows-controller/types/browser.ts). The placeholder in Browser class routes to settings.sendMessage() and onboarding.sendMessage(), but these paths are never executed. Delete this method along with its TODO comment.
🤖 Prompt for AI Agents
In src/main/browser/browser.ts around lines 52 to 63, remove the placeholder
method sendMessageToCoreWebContents and its TODO comment because it has no
active callers; delete the entire method block (including the eslint-disable
comment and the temporary calls to settings.sendMessage and
onboarding.sendMessage) so the Browser class no longer contains this unused
routing method, and run a quick search to confirm no external code relies on it
before committing.
| // This module hasn't loaded yet, so we have to wait | ||
| setImmediate(() => { | ||
| windowsController.on("window-focused", this.render); | ||
| }); |
There was a problem hiding this comment.
Fragile initialization pattern using setImmediate workaround.
The setImmediate with comment "This module hasn't loaded yet" is a code smell indicating improper module initialization order. This creates a race condition where the window-focused event handler may or may not be registered depending on timing.
This relates to the circular dependency issue flagged in src/main/controllers/index.ts. Consider:
- Ensuring proper initialization order by breaking the circular dependency between windows-controller and app-menu-controller
- Using a ready/initialized event from windows-controller instead of timing-based workarounds
- Having windows-controller expose an initialization promise that can be awaited
🤖 Prompt for AI Agents
In src/main/controllers/app-menu-controller/index.ts around lines 24 to 27,
replace the fragile setImmediate registration with a deterministic
initialization: remove the setImmediate block and instead subscribe to a
ready/initialized signal from windows-controller (e.g., await
windowsController.initPromise or listen for a "ready"/"initialized" event)
before calling windowsController.on("window-focused", this.render); to do this,
break the circular dependency by either (a) moving shared types/registration
interfaces into a small neutral module both controllers can import, (b) making
windows-controller expose an initialization Promise or emit a "ready" event once
constructed, or (c) accept windowsController via dependency injection into
app-menu-controller during app startup so you can await windowsController
initialization and then register the "window-focused" handler. Ensure you remove
the timing-based workaround and document the new initialization contract.
| ipcMain.on("browser:load-profile", async (_event, profileId: string) => { | ||
| await browser?.loadProfile(profileId); | ||
| await loadedProfilesController.load(profileId); | ||
| }); | ||
|
|
||
| ipcMain.on("browser:unload-profile", async (_event, profileId: string) => { | ||
| browser?.unloadProfile(profileId); | ||
| loadedProfilesController.unload(profileId); | ||
| }); | ||
|
|
||
| ipcMain.on("browser:create-window", async () => { | ||
| browser?.createWindow(); | ||
| browserWindowsController.create(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify async handling consistency.
Line 6 properly awaits load(), but lines 10 and 14 don't await unload() and create(). If these methods are asynchronous and can fail, consider:
- Whether they should be awaited
- Whether error handling is needed for all three operations
If fire-and-forget is intentional for unload/create, consider adding a comment explaining the rationale.
Run this script to check the signatures of these controller methods:
🏁 Script executed:
#!/bin/bash
# Check signatures of controller methods to verify if they're async
ast-grep --pattern $'class LoadedProfilesController {
$$$
load($$$) {
$$$
}
$$$
}'
ast-grep --pattern $'class LoadedProfilesController {
$$$
unload($$$) {
$$$
}
$$$
}'
ast-grep --pattern $'class $_ {
$$$
create($$$) {
$$$
}
$$$
}' | grep -A 10 -B 2 "browserWindowsController\|WindowsController"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find controller files
echo "=== Searching for LoadedProfilesController ==="
fd -type f "LoadedProfilesController" 2>/dev/null || fd -type f "loaded.*profile" -i 2>/dev/null
echo -e "\n=== Searching for BrowserWindowsController ==="
fd -type f "BrowserWindowsController" 2>/dev/null || fd -type f "browser.*window" -i 2>/dev/null
echo -e "\n=== Searching in src/main/ipc/browser ==="
fd -type f . src/main/ipc/browser 2>/dev/null || echo "Directory not found"
echo -e "\n=== Searching for 'loadedProfilesController' or 'browserWindowsController' definitions ==="
rg -n "loadedProfilesController\s*=|browserWindowsController\s*=" --type ts --type js 2>/dev/null | head -20
echo -e "\n=== Searching for controller class definitions ==="
rg -n "class.*Controller" --type ts --type js 2>/dev/null | grep -i "profile\|window" | head -20Length of output: 102331
🏁 Script executed:
#!/bin/bash
# Find controller files and their definitions
echo "=== Searching for controller file locations ==="
fd -type f . src/main 2>/dev/null | grep -i "controller\|profile\|window" | head -20
echo -e "\n=== Searching for loadedProfilesController/browserWindowsController instantiation ==="
rg -n "loadedProfilesController|browserWindowsController" --type ts --type js -B 2 -A 2 2>/dev/null | head -40
echo -e "\n=== Searching for unload method definitions ==="
rg -n "^\s*(async\s+)?unload\s*\(" --type ts --type js -B 5 2>/dev/null | head -30
echo -e "\n=== Searching for create method definitions ==="
rg -n "^\s*(async\s+)?create\s*\(" --type ts --type js -B 5 2>/dev/null | head -30Length of output: 4440
🏁 Script executed:
#!/bin/bash
# Look at the LoadedProfilesController file directly
echo "=== LoadedProfilesController file content (relevant methods) ==="
cat src/main/controllers/loaded-profiles-controller/index.ts | grep -n "load\|unload" | head -30
echo -e "\n=== Examining load method signature ==="
rg -n "^\s*load\s*\(" src/main/controllers/loaded-profiles-controller/index.ts -A 5
echo -e "\n=== Examining unload method signature ==="
rg -n "^\s*unload\s*\(" src/main/controllers/loaded-profiles-controller/index.ts -A 5
echo -e "\n=== Examining create method in browserWindowsController ==="
cat src/main/controllers/windows-controller/interfaces/browser.ts | grep -n "create\|instantCreate" -A 3Length of output: 2321
🏁 Script executed:
#!/bin/bash
# Get more context around unload
echo "=== Full unload method search in LoadedProfilesController ==="
rg -n "unload" src/main/controllers/loaded-profiles-controller/index.ts -B 2 -A 8
echo -e "\n=== Check if unload is async anywhere ==="
rg -n "async.*unload|unload.*async" src/main/controllers/loaded-profiles-controller/ -r '$0'Length of output: 1807
🏁 Script executed:
#!/bin/bash
# Get full unload method implementation
echo "=== Full context around unload in LoadedProfilesController ==="
rg -n "unload" src/main/controllers/loaded-profiles-controller/index.ts -B 3 -A 10
echo -e "\n=== Check the interface definition ==="
sed -n '20,35p' src/main/controllers/loaded-profiles-controller/index.ts
echo -e "\n=== Search for all method definitions ==="
rg -n "^\s*(public|private).*\(" src/main/controllers/loaded-profiles-controller/index.ts | head -20Length of output: 2813
browserWindowsController.create() should be awaited; loadedProfilesController.unload() does not need to be.
Line 10's unload() call is correct—the method is synchronous (public unload(profileId: string): boolean) and requires no await. However, line 14's create() call is async (async (type: BrowserWindowType...) => Promise<BrowserWindow>) and should be awaited to handle the pending operation properly.
The inconsistency in the review comment stems from incorrectly grouping these two cases. Only create() requires fixing.
🤖 Prompt for AI Agents
In src/main/ipc/browser/browser.ts around lines 5 to 15, the IPC handler for
"browser:create-window" calls browserWindowsController.create() but does not
await it; change the handler to await the async create() call (i.e., make the
arrow function await browserWindowsController.create()) so the Promise is
properly handled and any errors propagate or can be caught; do not modify the
synchronous unload() call.
| const tabbedWindow = browserWindowsController.getWindowFromWebContents(event.sender); | ||
| if (tabbedWindow) { | ||
| tabbedWindow.setWindowButtonVisibility(visible); | ||
| tabbedWindow.setMacOSTrafficLights(visible); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: rename tabbedWindow → win for consistency.
Everything else looks solid; you already constrain by window type and sender.
- const tabbedWindow = browserWindowsController.getWindowFromWebContents(event.sender);
- if (tabbedWindow) {
- tabbedWindow.setMacOSTrafficLights(visible);
+ const win = browserWindowsController.getWindowFromWebContents(event.sender);
+ if (win) {
+ win.setMacOSTrafficLights(visible);
}📝 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.
| const tabbedWindow = browserWindowsController.getWindowFromWebContents(event.sender); | |
| if (tabbedWindow) { | |
| tabbedWindow.setWindowButtonVisibility(visible); | |
| tabbedWindow.setMacOSTrafficLights(visible); | |
| } | |
| const win = browserWindowsController.getWindowFromWebContents(event.sender); | |
| if (win) { | |
| win.setMacOSTrafficLights(visible); | |
| } |
🤖 Prompt for AI Agents
In src/main/ipc/browser/interface.ts around lines 14 to 17, rename the local
variable tabbedWindow to win for naming consistency: change the const
declaration and all uses (e.g., tabbedWindow.setMacOSTrafficLights(visible)) to
use win instead; preserve the existing type checks and logic (only the
identifier changes) and run type-checks to ensure no other references break.
| # IPC Namespaces | ||
|
|
||
| This directory contains all `ipcMain` registrations that the Flow browser exposes to renderer processes. Each module keeps the handlers for a single area of the app (e.g. sessions, windows, browser state) and is imported by `src/main/ipc/main.ts`. Importing a module is enough to register its listeners. | ||
|
|
||
| > **Renderer opt-in:** Renderers that expect push-style updates must emit `listeners:add`/`listeners:remove` (see below) so the main process can deliver broadcasts only to windows that are listening. | ||
|
|
||
| ## Bootstrapping | ||
|
|
||
| - `main.ts` — central entry point that imports every namespace. Add new IPC files here to ensure handlers run. | ||
|
|
||
| ## Listener management | ||
|
|
||
| - `listeners-manager.ts` | ||
| - Tracks which `WebContents` listens to which channel via the `listeners:add` / `listeners:remove` events. Renderer messages should pass a unique listener id so they can unsubscribe cleanly. | ||
| - Exposes helpers: | ||
| - `sendMessageToListeners(channel, ...args)` — broadcast to every subscribed renderer. | ||
| - `sendMessageToListenersWithWebContents(webContents[], channel, ...args)` — scoped broadcast. | ||
|
|
||
| ## Adding a new namespace | ||
|
|
||
| 1. Create a new file (or folder) under `src/main/ipc/` and register handlers with `ipcMain.on` / `ipcMain.handle`. | ||
| 2. Add any broadcast helpers that other main-process modules can call instead of importing `ipcMain` directly. | ||
| 3. If renderers should receive push updates, leverage `listeners-manager` and document the channel name. Remember to clean up listeners when renderer views unmount. | ||
| 4. Import the new module from `src/main/ipc/main.ts`. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add minimal code samples and naming conventions.
Include 1–2 short snippets:
- Renderer opt‑in example (
listeners:add/listeners:removewith a listener id). - Namespace skeleton showing
ipcMain.on/handleand usinglisteners-manager.
Also state a convention for channel names (e.g., namespace:event) to avoid collisions.
I can draft these snippets if helpful.
| MATCH_PATTERN: false, // @/browser/utility/match-pattern.ts | ||
| WINDOWS: true // @/controllers/windows-controller | ||
| } as const; |
There was a problem hiding this comment.
Default-on WINDOWS debug may be noisy in production.
Consider defaulting to false and enabling via flags/env to avoid accidental log noise in packaged builds.
- WINDOWS: true // @/controllers/windows-controller
+ WINDOWS: false // @/controllers/windows-controller📝 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.
| MATCH_PATTERN: false, // @/browser/utility/match-pattern.ts | |
| WINDOWS: true // @/controllers/windows-controller | |
| } as const; | |
| MATCH_PATTERN: false, // @/browser/utility/match-pattern.ts | |
| WINDOWS: false // @/controllers/windows-controller | |
| } as const; |
🤖 Prompt for AI Agents
In src/main/modules/output.ts around lines 23 to 25, the WINDOWS debug flag is
defaulting to true which can produce noisy logs in production; change the
default to false and wire it to be overridable via a runtime flag or environment
variable (e.g., process.env.DEBUG_WINDOWS or a CLI/feature flag) so packaged
builds start quiet, and update any consuming code/tests to read the new env/flag
instead of relying on the hardcoded true; keep the object as const and preserve
existing key shape/type.
| export async function persistTabToStorage(tab: Tab) { | ||
| const window = tab.getWindow(); | ||
| if (window.type !== "normal") return; | ||
| if (window.browserWindowType !== "normal") return; | ||
|
|
||
| // Prevent saving tabs stuck in sleep mode | ||
| // if (tab.url === SLEEP_MODE_URL) return; |
There was a problem hiding this comment.
Guard window type check against undefined
tab.getWindow() should always exist, but during teardown it may be transiently undefined. Add a safe optional check to avoid persisting during destroy.
- const window = tab.getWindow();
- if (window.browserWindowType !== "normal") return;
+ const window = tab.getWindow?.();
+ if (!window || window.browserWindowType !== "normal") return;🤖 Prompt for AI Agents
In src/main/saving/tabs.ts around lines 16 to 21, tab.getWindow() can be
undefined during teardown so the current direct access to
window.browserWindowType may throw; change the guard to first check for the
window's existence and only proceed when it exists and its browserWindowType ===
"normal" (e.g. const window = tab.getWindow(); if (!window ||
window.browserWindowType !== "normal") return;), ensuring you short-circuit out
when window is falsy.
1 participant
Goals
browserobjectSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation
Chores