Port browser tabs into the unified rc tab strip#785
Merged
jeffscottward merged 17 commits intoRunMaestro:rcfrom Apr 10, 2026
Merged
Port browser tabs into the unified rc tab strip#785jeffscottward merged 17 commits intoRunMaestro:rcfrom
jeffscottward merged 17 commits intoRunMaestro:rcfrom
Conversation
📝 WalkthroughWalkthroughAdds first-class "browser tab" support: UI components, persistence utilities, session/tab plumbing, main-process webview hardening, TypeScript types, and extensive unit and E2E tests to validate navigation, security boundaries, persistence, and unified-tab behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Renderer as Renderer Process
participant BrowserTab as BrowserTabView
participant Webview as Electron Webview
participant Main as Main Process
User->>Renderer: Click "New Browser Tab"
Renderer->>BrowserTab: Render BrowserTabView (about:blank)
BrowserTab->>Webview: attach <webview> (partition/src)
Webview->>Main: will-attach-webview
Main->>Main: validate src & partition, harden webPreferences
Main-->>Webview: allow attachment (or preventDefault if invalid)
Webview->>Main: did-attach-webview
BrowserTab->>Webview: set src on navigation submit
Webview->>Main: will-navigate / will-redirect
Main->>Main: enforce allowed protocols (http/https/about:blank)
Main-->>Webview: allow/deny navigation
Webview->>BrowserTab: dom-ready / did-finish-load events
BrowserTab->>Renderer: onUpdateTab(url,title,isLoading,webContentsId)
User->>BrowserTab: in-page window.open(...) or clipboard access
Webview->>Main: setWindowOpenHandler / permission-request
Main->>Main: deny popup and deny clipboard for webview guests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR ports browser tabs into Maestro's unified tab strip (
Confidence Score: 4/5Safe to merge after fixing the missing One P1 defect (stale browser tab metadata in the tab strip due to missing useMemo dependency) requires a one-line fix before merge. The rest of the integration — persistence, session restoration, close/reopen, drag-and-drop, keyboard navigation in non-filter mode — is well-structured and follows existing patterns. The unread-filter keyboard nav gap for browser tabs is a P2 inconvenience, not a data loss or crash risk. src/renderer/hooks/tabs/useTabHandlers.ts (missing dep), src/renderer/utils/tabHelpers.ts (browser tab nav in showUnreadOnly) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TabBar
participant useTabHandlers
participant sessionStore
participant BrowserTabView
participant Webview
User->>TabBar: Click "New Tab" → Browser Tab
TabBar->>useTabHandlers: handleNewBrowserTab()
useTabHandlers->>sessionStore: setSessions (add BrowserTab, set activeBrowserTabId)
sessionStore-->>TabBar: unifiedTabs recomputes → BrowserTabItem renders
User->>TabBar: Click BrowserTabItem
TabBar->>useTabHandlers: handleSelectBrowserTab(tabId)
useTabHandlers->>sessionStore: setSessions (activeBrowserTabId = tabId)
sessionStore-->>BrowserTabView: activeBrowserTab prop updates → webview shown
User->>BrowserTabView: Enter URL / navigate
BrowserTabView->>Webview: webview.src = nextUrl
Webview-->>BrowserTabView: did-navigate / page-title-updated / page-favicon-updated
BrowserTabView->>useTabHandlers: handleUpdateBrowserTab(sessionId, tabId, updates)
useTabHandlers->>sessionStore: setSessions (browserTabs updated)
Note over sessionStore,TabBar: ⚠️ unifiedTabs useMemo missing browserTabs dep → BrowserTabItem shows stale title/favicon
User->>TabBar: Click X on BrowserTabItem
TabBar->>useTabHandlers: handleCloseBrowserTab(tabId)
useTabHandlers->>sessionStore: closeBrowserTab() → fallback tab activated
sessionStore-->>TabBar: unifiedTabs recomputes → BrowserTabItem removed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+387
to
+394
| <webview | ||
| ref={(element) => { | ||
| webviewRef.current = element as unknown as ElectronWebviewElement | null; | ||
| }} | ||
| className="w-full h-full border-0 bg-white" | ||
| partition={tab.partition} | ||
| src={tab.url || DEFAULT_BROWSER_TAB_URL} | ||
| /> |
There was a problem hiding this comment.
bg-white flashes in dark themes
The webview element has a hardcoded bg-white class. While the webview itself renders the page content, the white background is visible while pages are loading and in dark-mode themes it creates a brief white flash on navigation. Consider using a neutral background derived from the theme or bg-transparent once the webview is ready.
Suggested change
| <webview | |
| ref={(element) => { | |
| webviewRef.current = element as unknown as ElectronWebviewElement | null; | |
| }} | |
| className="w-full h-full border-0 bg-white" | |
| partition={tab.partition} | |
| src={tab.url || DEFAULT_BROWSER_TAB_URL} | |
| /> | |
| className="w-full h-full border-0" | |
| style={{ backgroundColor: theme.colors.bgMain }} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/__tests__/renderer/stores/tabStore.test.ts (1)
983-1018:⚠️ Potential issue | 🟡 MinorMissing
browserTabsandactiveBrowserTabIdin terminal tab test helper.The
setupSessionWithTerminalTabshelper creates a session object inline but omits the new required browser tab fields that were added tocreateMockSession. This inconsistency could cause type errors or unexpected behavior.🔧 Proposed fix
function setupSessionWithTerminalTabs(terminalTabs: TerminalTab[]): string { const sessionId = 'test-terminal-session'; const session = { id: sessionId, name: 'Test Session', toolType: 'claude-code', state: 'idle', cwd: '/test', fullPath: '/test', projectRoot: '/test', aiLogs: [], shellLogs: [], workLog: [], contextUsage: 0, inputMode: 'terminal' as const, aiPid: 0, terminalPid: 0, port: 0, isLive: false, changedFiles: [], isGitRepo: false, fileTree: [], fileExplorerExpanded: [], fileExplorerScrollPos: 0, executionQueue: [], activeTimeMs: 0, aiTabs: [], activeTabId: '', closedTabHistory: [], filePreviewTabs: [], activeFileTabId: null, + browserTabs: [], + activeBrowserTabId: null, unifiedTabOrder: terminalTabs.map((t) => ({ type: 'terminal' as const, id: t.id })), unifiedClosedTabHistory: [], terminalTabs, activeTerminalTabId: terminalTabs[0]?.id ?? null, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/stores/tabStore.test.ts` around lines 983 - 1018, The test helper setupSessionWithTerminalTabs constructs a session object but omits the newly required browserTabs and activeBrowserTabId fields (added in createMockSession), causing type mismatches; update setupSessionWithTerminalTabs to include browserTabs: [] and activeBrowserTabId: null (or an appropriate default) in the returned session object so it matches createMockSession's shape and test expectations, referencing the setupSessionWithTerminalTabs function and createMockSession for consistency.src/renderer/utils/tabHelpers.ts (2)
443-456:⚠️ Potential issue | 🟠 MajorNew AI tabs don't deselect the active browser tab.
createTab()clears file/terminal selection but leavesactiveBrowserTabIdintact. If the user creates a chat tab while a browser tab is visible, the browser ref still wins and the new AI tab is not actually activated.Suggested fix
const updatedSession: Session = { ...session, aiTabs: [...(session.aiTabs || []), newTab], activeTabId: newTab.id, activeFileTabId: null, + activeBrowserTabId: null, activeTerminalTabId: null, inputMode: 'ai' as const, unifiedTabOrder: [...(session.unifiedTabOrder || []), newTabRef], };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/utils/tabHelpers.ts` around lines 443 - 456, The new AI tab creation updates session state but forgets to clear the browser selection, so add activeBrowserTabId: null to the updated session object (the same place where activeFileTabId and activeTerminalTabId are cleared) — update the updatedSession construction (referencing newTabRef and updatedSession in tabHelpers.ts / createTab flow) to include activeBrowserTabId: null so the AI tab becomes the effective activeTabId.
1699-1714:⚠️ Potential issue | 🟠 MajorThe AI early-return still treats a browser-selected session as already active.
activeTabIdis intentionally preserved when a browser tab is selected. Without checkingactiveBrowserTabId === nullhere, unified keyboard navigation back to that AI tab can no-op and leave the browser view visible.Suggested fix
if ( session.activeTabId === targetTabRef.id && session.activeFileTabId === null && + session.activeBrowserTabId === null && session.activeTerminalTabId === null && session.inputMode === 'ai' ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/utils/tabHelpers.ts` around lines 1699 - 1714, The early-return in the AI-mode branch incorrectly treats a browser-selected session as already active; update the conditional that checks session.activeTabId === targetTabRef.id && session.activeFileTabId === null && session.activeTerminalTabId === null && session.inputMode === 'ai' to also require session.activeBrowserTabId === null so the return (which yields { type: 'ai', id: targetTabRef.id, session: repairedSession }) only happens when no browser tab is selected, preventing unified keyboard navigation from no-oping and leaving the browser view visible.src/renderer/hooks/tabs/useTabHandlers.ts (1)
196-204:⚠️ Potential issue | 🟠 MajorInclude
activeSession.browserTabsin theunifiedTabsmemo deps.
buildUnifiedTabs(activeSession)now readsbrowserTabs, but this memo never invalidates when only browser-tab metadata changes. That leaves the unified strip stuck with stale browser titles/favicons/loading state after navigation.Suggested fix
}, [ activeSession?.aiTabs, + activeSession?.browserTabs, activeSession?.filePreviewTabs, activeSession?.terminalTabs, activeSession?.unifiedTabOrder, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/tabs/useTabHandlers.ts` around lines 196 - 204, The memo for unifiedTabs (the useMemo that returns buildUnifiedTabs(activeSession)) is missing activeSession.browserTabs in its dependency array, so updates to browser tab metadata don't invalidate the memo; add activeSession.browserTabs to the dependency list of the unifiedTabs useMemo so buildUnifiedTabs(activeSession) reruns when browser tab state (titles/favicons/loading) changes.
🧹 Nitpick comments (3)
src/renderer/components/MainPanel/types.ts (1)
163-166: Narrow the browser-tab update payload.Line 166 exposes
Partial<BrowserTab>, which includes fields likeid,createdAt, andpartition.src/renderer/hooks/tabs/useTabHandlers.tscurrently spreadsupdatesstraight into the stored tab, so any future caller can accidentally rewrite identity or partition state. A dedicated update type for the mutable chrome fields would make this boundary safer.♻️ Suggested shape
+export type BrowserTabUpdate = Pick< + BrowserTab, + 'url' | 'title' | 'canGoBack' | 'canGoForward' | 'isLoading' | 'favicon' | 'webContentsId' +>; + onNewBrowserTab?: () => void; onBrowserTabSelect?: (tabId: string) => void; onBrowserTabClose?: (tabId: string) => void; - onBrowserTabUpdate?: (sessionId: string, tabId: string, updates: Partial<BrowserTab>) => void; + onBrowserTabUpdate?: (sessionId: string, tabId: string, updates: BrowserTabUpdate) => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/MainPanel/types.ts` around lines 163 - 166, The onBrowserTabUpdate signature currently accepts Partial<BrowserTab>, which allows callers to overwrite immutable identity fields; change this to a dedicated update type (e.g., BrowserTabUpdatePayload or MutableBrowserTabFields) containing only the mutable fields you want to allow (title, url, favIcon, active, loading, lastAccessed, etc.), update the type alias in types.ts to use that new payload instead of Partial<BrowserTab>, and then update useTabHandlers (and any callers) to accept and spread only these allowed fields when merging into the stored tab so id/createdAt/partition/identity fields cannot be overwritten.src/renderer/components/TabBar/TabBar.tsx (1)
68-77: Warn on missing browser-tab handlers too.Lines 546-547 fall back to no-op
onSelect/onClose, but the dev-only guard at Lines 68-77 only checks file and terminal handlers. If a future wiring regression drops the browser callbacks, the tabs become inert with no signal. Matching the existing warning pattern here would make that failure obvious during integration.Also applies to: 546-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/TabBar/TabBar.tsx` around lines 68 - 77, In TabBar, when unifiedTabs is true the dev-only checks currently warn if onFileTabSelect/onFileTabClose or onTerminalTabSelect/onTerminalTabClose are missing; add the same console.warn checks for the browser tab handlers (e.g., onBrowserTabSelect and onBrowserTabClose) inside the process.env.NODE_ENV !== 'production' && unifiedTabs guard so missing browser callbacks are logged during development—mirror the existing warning messages and placement used for file/terminal handlers to catch the fallback no-op behavior seen later where missing handlers default to inert no-ops.src/__tests__/renderer/utils/tabHelpers.test.ts (1)
150-160: Type the browser-tab fixture instead of casting it back fromany.Lines 150-160 build an untyped object, and Lines 3489, 3508, and 3528 cast it into
browserTabs. That drops most of the type-safety from the new coverage, so aBrowserTabshape change can leave these tests green while the fixture drifts. ReturningBrowserTabwithPartial<BrowserTab>overrides would keep the test contract aligned with production code.Also applies to: 3489-3489, 3508-3508, 3528-3528
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/utils/tabHelpers.test.ts` around lines 150 - 160, The fixture createMockBrowserTab should return a typed BrowserTab rather than an untyped object and callers should use Partial<BrowserTab> overrides: import the BrowserTab type and change createMockBrowserTab signature to accept overrides: Partial<BrowserTab> and return a BrowserTab, ensure the default properties match BrowserTab fields, and remove any downstream casts from any to BrowserTab in tests (the places that currently cast the fixture to browserTabs should just call createMockBrowserTab and rely on its typed return).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/browser-tab.spec.ts`:
- Around line 195-200: Teardown currently calls server.close() without awaiting
its completion, which can cause EADDRINUSE when the next suite binds to the same
fixed port (7101); change the teardown to await server.close() (handle/ignore
errors with try/catch if necessary) and ensure it completes before closing
firstApp and secondApp (i.e., await server.close() before reusing the port), and
make the same change at the other teardown location that mirrors lines 285-287;
reference the server.close(), firstApp.close(), and secondApp.close() calls when
updating the teardown sequence.
- Around line 150-153: The test currently uses a machine-specific absolute path
passed to openFileTab (the hardcoded
'/Users/jeffscottward/.../ARCHITECTURE.md'); replace that with a portable
fixture by resolving ARCHITECTURE.md from the repository checkout or by
copying/staging the file into the test harness testDataDir and passing that path
to openFileTab. Update the spec to compute the path at runtime (e.g., using repo
root or testDataDir) and call openFileTab(window, resolvedPath) so the test no
longer depends on a local developer's filesystem.
In `@src/main/app-lifecycle/window-manager.ts`:
- Around line 64-66: The current isAllowedBrowserTabPartition uses startsWith
which is too permissive; replace that check with a strict, anchored format
validation (e.g. use a RegExp test) that verifies the partition string exactly
matches the expected pattern prefixed by BROWSER_TAB_PARTITION_PREFIX and
followed only by the allowed identifier characters/delimiter (no extra
characters or partial matches). Update isAllowedBrowserTabPartition to use that
anchored regex test (referencing BROWSER_TAB_PARTITION_PREFIX and
isAllowedBrowserTabPartition) so the main-process gate enforces the same full
partition format as the renderer sanitizer.
- Around line 268-271: The partition variable is being read from webPreferences
but the webview tag's partition attribute comes via params; update the
will-attach-webview handler (mainWindow.webContents.on('will-attach-webview',
...)) to read partition from params.partition (falling back to
webPreferences.partition if needed) so valid HTML-specified partitions are
validated correctly; keep the existing src handling and security checks but use
params.partition as the primary source.
In `@src/renderer/components/MainPanel/BrowserTabView.tsx`:
- Around line 156-178: The did-fail-load handler (handleDidFailLoad) should
ignore aborted navigation failures so temporary/redirect loads don't clear
isLoading or overwrite URL/title: add an early return for aborted errors by
checking the event's errorCode/errorDescription (Electron abort is errorCode ===
-3 / errorDescription 'ERR_ABORTED') before using
validatedURL/getURL/latestTabRef and calling
setAddressValue/setAddressError/updateTabState; keep the existing main-frame
check and only proceed for real failures.
- Around line 231-246: The code sets isLoading true unconditionally before
verifying whether navigation will occur, causing the tab to appear stuck when
the address equals the current webview.src; change the logic in the submit
handler so you first compute whether a navigation will happen (compare nextUrl
to webviewRef.current?.src) and only call onUpdateTab with isLoading: true when
they differ (otherwise preserve or set isLoading:false), and only assign
webview.src when webviewRef.current && webviewRef.current.src !== nextUrl;
adjust the calls to setAddressValue, setAddressError, and onUpdateTab (the
update for tab.id, url, title, isLoading) accordingly so the spinner only
appears when an actual navigation is triggered.
In `@src/renderer/components/TabBar/BrowserTabItem.tsx`:
- Around line 253-260: The BrowserTabItem currently renders tab.favicon directly
into an <img> (see tab.favicon usage and setFaviconFailed in BrowserTabItem),
which allows untrusted schemes to trigger renderer fetches; sanitize or
normalize the favicon before use by either whitelisting allowed URI schemes
(e.g., only http, https, data) and rejecting/ignoring anything else (file:,
about:, chrome:, custom schemes), or convert/validate favicons upstream to
trusted data URLs and only pass safe data URLs into this component; ensure
BrowserTabItem checks the sanitized value and falls back to a default icon
and/or calls setFaviconFailed when the scheme is disallowed or parsing fails.
- Around line 244-251: Browser tabs are missing shortcut badges and bulk-close
actions because BrowserTabItem's props (shortcutHint, onCloseOtherTabs,
onCloseTabsLeft, onCloseTabsRight, totalTabs, tabIndex) are not being forwarded
from the TabBar wiring; update the TabBar rendering logic that instantiates
BrowserTabItem (where TabBar maps/creates tab items) to pass these props through
using the existing tab data and handlers (ensure shortcutHint is derived from
the tab metadata and the three close handlers and totalTabs/tabIndex are
provided from TabBar's state/props), so BrowserTabItem can render the shortcut
badge and close-left/close-right/close-others actions.
- Line 221: In BrowserTabItem remove the native tooltip by deleting the
title={tab.url || label} prop in the JSX so browser tabs no longer get the
default hover tooltip; keep any existing custom overlay/tooltip logic (and if
accessibility needs remain, use aria-label or the custom overlay instead of
title) and ensure no other place in BrowserTabItem reintroduces title for
browser tabs.
In `@src/renderer/components/TabBar/NewTabPopover.tsx`:
- Around line 62-63: The NewTabPopover always renders a "New Terminal" option
even when onNewTerminalTab is undefined; update the component (NewTabPopover) so
the "New Terminal" menu item/option is only rendered when onNewTerminalTab is
truthy, and wire its click handler to call onNewTerminalTab (fall back to
onNewTab only if that is intended); apply the same conditional rendering change
for the other occurrences referenced (the other menu blocks where "New Terminal"
is shown, e.g., the places around the checks for
onNewBrowserTab/onNewTerminalTab) so the terminal entry is hidden when terminal
creation is unavailable.
In `@src/renderer/hooks/session/useSessionRestoration.ts`:
- Around line 386-407: restoredActiveFileTabId is taken directly from
correctedSession.activeFileTabId and can be stale, which then wrongly suppresses
a valid restoredActiveBrowserTabId in the else if (restoredActiveFileTabId)
branch; validate the file tab ID against the same set used for other types
(e.g., validFileTabIds) when assigning restoredActiveFileTabId (similar to how
restoredActiveBrowserTabId and restoredActiveTerminalTabId are computed) and if
the ID is not in validFileTabIds set, set restoredActiveFileTabId = null before
the inputMode/branch logic so a valid browser selection is not accidentally
cleared.
In `@src/renderer/utils/tabHelpers.ts`:
- Around line 1197-1201: The reopen-file-tab branches set activeFileTabId but
forget to clear activeTerminalTabId and reset inputMode, so reopening a file
while a terminal is active leaves the terminal mounted; update both branches
that set activeFileTabId (the one using existingTab.id and the other around
lines 1237-1243) to also set activeTerminalTabId: null and inputMode: 'ai' after
ensuring unifiedTabOrder/unifiedClosedTabHistory (referencing session,
existingTab.id, remainingHistory, ensureInUnifiedTabOrder) so the UI actually
switches from terminal mode back to the file tab.
---
Outside diff comments:
In `@src/__tests__/renderer/stores/tabStore.test.ts`:
- Around line 983-1018: The test helper setupSessionWithTerminalTabs constructs
a session object but omits the newly required browserTabs and activeBrowserTabId
fields (added in createMockSession), causing type mismatches; update
setupSessionWithTerminalTabs to include browserTabs: [] and activeBrowserTabId:
null (or an appropriate default) in the returned session object so it matches
createMockSession's shape and test expectations, referencing the
setupSessionWithTerminalTabs function and createMockSession for consistency.
In `@src/renderer/hooks/tabs/useTabHandlers.ts`:
- Around line 196-204: The memo for unifiedTabs (the useMemo that returns
buildUnifiedTabs(activeSession)) is missing activeSession.browserTabs in its
dependency array, so updates to browser tab metadata don't invalidate the memo;
add activeSession.browserTabs to the dependency list of the unifiedTabs useMemo
so buildUnifiedTabs(activeSession) reruns when browser tab state
(titles/favicons/loading) changes.
In `@src/renderer/utils/tabHelpers.ts`:
- Around line 443-456: The new AI tab creation updates session state but forgets
to clear the browser selection, so add activeBrowserTabId: null to the updated
session object (the same place where activeFileTabId and activeTerminalTabId are
cleared) — update the updatedSession construction (referencing newTabRef and
updatedSession in tabHelpers.ts / createTab flow) to include activeBrowserTabId:
null so the AI tab becomes the effective activeTabId.
- Around line 1699-1714: The early-return in the AI-mode branch incorrectly
treats a browser-selected session as already active; update the conditional that
checks session.activeTabId === targetTabRef.id && session.activeFileTabId ===
null && session.activeTerminalTabId === null && session.inputMode === 'ai' to
also require session.activeBrowserTabId === null so the return (which yields {
type: 'ai', id: targetTabRef.id, session: repairedSession }) only happens when
no browser tab is selected, preventing unified keyboard navigation from no-oping
and leaving the browser view visible.
---
Nitpick comments:
In `@src/__tests__/renderer/utils/tabHelpers.test.ts`:
- Around line 150-160: The fixture createMockBrowserTab should return a typed
BrowserTab rather than an untyped object and callers should use
Partial<BrowserTab> overrides: import the BrowserTab type and change
createMockBrowserTab signature to accept overrides: Partial<BrowserTab> and
return a BrowserTab, ensure the default properties match BrowserTab fields, and
remove any downstream casts from any to BrowserTab in tests (the places that
currently cast the fixture to browserTabs should just call createMockBrowserTab
and rely on its typed return).
In `@src/renderer/components/MainPanel/types.ts`:
- Around line 163-166: The onBrowserTabUpdate signature currently accepts
Partial<BrowserTab>, which allows callers to overwrite immutable identity
fields; change this to a dedicated update type (e.g., BrowserTabUpdatePayload or
MutableBrowserTabFields) containing only the mutable fields you want to allow
(title, url, favIcon, active, loading, lastAccessed, etc.), update the type
alias in types.ts to use that new payload instead of Partial<BrowserTab>, and
then update useTabHandlers (and any callers) to accept and spread only these
allowed fields when merging into the stored tab so
id/createdAt/partition/identity fields cannot be overwritten.
In `@src/renderer/components/TabBar/TabBar.tsx`:
- Around line 68-77: In TabBar, when unifiedTabs is true the dev-only checks
currently warn if onFileTabSelect/onFileTabClose or
onTerminalTabSelect/onTerminalTabClose are missing; add the same console.warn
checks for the browser tab handlers (e.g., onBrowserTabSelect and
onBrowserTabClose) inside the process.env.NODE_ENV !== 'production' &&
unifiedTabs guard so missing browser callbacks are logged during
development—mirror the existing warning messages and placement used for
file/terminal handlers to catch the fallback no-op behavior seen later where
missing handlers default to inert no-ops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0904df7d-e63c-4297-8dce-889d43dfdb82
⛔ Files ignored due to path filters (1)
docs/screenshots/browser-tab-unified-tab-strip.pngis excluded by!**/*.png
📒 Files selected for processing (42)
e2e/browser-tab.spec.tssrc/__tests__/main/app-lifecycle/window-manager.test.tssrc/__tests__/renderer/components/MainPanel/BrowserTabView.test.tsxsrc/__tests__/renderer/components/MainPanel/MainPanelContent.test.tsxsrc/__tests__/renderer/components/TabBar.test.tsxsrc/__tests__/renderer/hooks/useSessionRestoration.test.tssrc/__tests__/renderer/hooks/useTabHandlers.test.tssrc/__tests__/renderer/hooks/utils/useDebouncedPersistence.test.tssrc/__tests__/renderer/stores/tabStore.test.tssrc/__tests__/renderer/utils/browserTabPersistence.test.tssrc/__tests__/renderer/utils/tabHelpers.test.tssrc/main/app-lifecycle/window-manager.tssrc/renderer/App.tsxsrc/renderer/components/CuePipelineEditor/PipelineCanvas.tsxsrc/renderer/components/CuePipelineEditor/panels/CueSettingsPanel.tsxsrc/renderer/components/CuePipelineEditor/utils/pipelineToYaml.tssrc/renderer/components/CuePipelineEditor/utils/yamlToPipeline.tssrc/renderer/components/MainPanel/BrowserTabView.tsxsrc/renderer/components/MainPanel/MainPanel.tsxsrc/renderer/components/MainPanel/MainPanelContent.tsxsrc/renderer/components/MainPanel/types.tssrc/renderer/components/TabBar/BrowserTabItem.tsxsrc/renderer/components/TabBar/NewTabPopover.tsxsrc/renderer/components/TabBar/TabBar.tsxsrc/renderer/components/TabBar/tabBarUtils.tssrc/renderer/components/TabBar/types.tssrc/renderer/global.d.tssrc/renderer/hooks/cue/usePipelineState.tssrc/renderer/hooks/props/useMainPanelProps.tssrc/renderer/hooks/remote/useAppRemoteEventListeners.tssrc/renderer/hooks/session/useSessionCrud.tssrc/renderer/hooks/session/useSessionRestoration.tssrc/renderer/hooks/symphony/useSymphonyContribution.tssrc/renderer/hooks/tabs/useTabHandlers.tssrc/renderer/hooks/utils/useDebouncedPersistence.tssrc/renderer/hooks/wizard/useWizardHandlers.tssrc/renderer/stores/tabStore.tssrc/renderer/types/index.tssrc/renderer/utils/browserTabPersistence.tssrc/renderer/utils/tabHelpers.tssrc/renderer/utils/terminalTabHelpers.tssrc/renderer/utils/worktreeSession.ts
Comment on lines
+150
to
+153
| await openFileTab( | ||
| window, | ||
| '/Users/jeffscottward/Github/tools/Maestro-worktrees/browser-tab/ARCHITECTURE.md' | ||
| ); |
There was a problem hiding this comment.
Replace the machine-specific file fixture path.
Line 152 hardcodes Jeff's local worktree path, so this spec will fail on CI and on any other developer machine. Resolve ARCHITECTURE.md from the checked-out repo root or stage a fixture into testDataDir instead.
Suggested fix
+import path from 'path';
import http from 'http';
...
await openFileTab(
window,
- '/Users/jeffscottward/Github/tools/Maestro-worktrees/browser-tab/ARCHITECTURE.md'
+ path.join(process.cwd(), 'ARCHITECTURE.md')
);📝 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
| await openFileTab( | |
| window, | |
| '/Users/jeffscottward/Github/tools/Maestro-worktrees/browser-tab/ARCHITECTURE.md' | |
| ); | |
| await openFileTab( | |
| window, | |
| path.join(process.cwd(), 'ARCHITECTURE.md') | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/browser-tab.spec.ts` around lines 150 - 153, The test currently uses a
machine-specific absolute path passed to openFileTab (the hardcoded
'/Users/jeffscottward/.../ARCHITECTURE.md'); replace that with a portable
fixture by resolving ARCHITECTURE.md from the repository checkout or by
copying/staging the file into the test harness testDataDir and passing that path
to openFileTab. Update the spec to compute the path at runtime (e.g., using repo
root or testDataDir) and call openFileTab(window, resolvedPath) so the test no
longer depends on a local developer's filesystem.
Comment on lines
+195
to
+200
| server.close(); | ||
| if (firstApp) { | ||
| await firstApp.close().catch(() => {}); | ||
| } | ||
| if (secondApp) { | ||
| await secondApp.close().catch(() => {}); |
There was a problem hiding this comment.
Await server shutdown before reusing port 7101.
server.close() is async. Because both suites bind the same fixed port, the second test can race the first teardown and intermittently hit EADDRINUSE.
Suggested fix
+async function closeLocalTestServer(server: http.Server): Promise<void> {
+ return new Promise((resolve, reject) => {
+ server.close((error) => {
+ if (error) {
+ reject(error);
+ return;
+ }
+ resolve();
+ });
+ });
+}
...
} finally {
- server.close();
+ await closeLocalTestServer(server);
if (firstApp) {
await firstApp.close().catch(() => {});
}
...
} finally {
- server.close();
+ await closeLocalTestServer(server);
if (app) {
await app.close().catch(() => {});
}Also applies to: 285-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/browser-tab.spec.ts` around lines 195 - 200, Teardown currently calls
server.close() without awaiting its completion, which can cause EADDRINUSE when
the next suite binds to the same fixed port (7101); change the teardown to await
server.close() (handle/ignore errors with try/catch if necessary) and ensure it
completes before closing firstApp and secondApp (i.e., await server.close()
before reusing the port), and make the same change at the other teardown
location that mirrors lines 285-287; reference the server.close(),
firstApp.close(), and secondApp.close() calls when updating the teardown
sequence.
Comment on lines
+64
to
+66
| function isAllowedBrowserTabPartition(partition: string): boolean { | ||
| return partition.startsWith(BROWSER_TAB_PARTITION_PREFIX); | ||
| } |
There was a problem hiding this comment.
Tighten the partition allowlist.
startsWith() accepts values like a bare prefix or other unexpected names under the same namespace, so the main-process gate is weaker than the renderer-side partition sanitizer. This hardening layer should enforce the full expected partition format, not just the prefix.
Suggested fix
const BROWSER_TAB_PARTITION_PREFIX = 'persist:maestro-browser-session-';
+const BROWSER_TAB_PARTITION_PATTERN = /^persist:maestro-browser-session-[a-zA-Z0-9_-]+$/;
...
function isAllowedBrowserTabPartition(partition: string): boolean {
- return partition.startsWith(BROWSER_TAB_PARTITION_PREFIX);
+ return BROWSER_TAB_PARTITION_PATTERN.test(partition);
}📝 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
| function isAllowedBrowserTabPartition(partition: string): boolean { | |
| return partition.startsWith(BROWSER_TAB_PARTITION_PREFIX); | |
| } | |
| const BROWSER_TAB_PARTITION_PREFIX = 'persist:maestro-browser-session-'; | |
| const BROWSER_TAB_PARTITION_PATTERN = /^persist:maestro-browser-session-[a-zA-Z0-9_-]+$/; | |
| function isAllowedBrowserTabPartition(partition: string): boolean { | |
| return BROWSER_TAB_PARTITION_PATTERN.test(partition); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/app-lifecycle/window-manager.ts` around lines 64 - 66, The current
isAllowedBrowserTabPartition uses startsWith which is too permissive; replace
that check with a strict, anchored format validation (e.g. use a RegExp test)
that verifies the partition string exactly matches the expected pattern prefixed
by BROWSER_TAB_PARTITION_PREFIX and followed only by the allowed identifier
characters/delimiter (no extra characters or partial matches). Update
isAllowedBrowserTabPartition to use that anchored regex test (referencing
BROWSER_TAB_PARTITION_PREFIX and isAllowedBrowserTabPartition) so the
main-process gate enforces the same full partition format as the renderer
sanitizer.
Comment on lines
+268
to
+271
| mainWindow.webContents.on('will-attach-webview', (event, webPreferences, params) => { | ||
| const src = typeof params.src === 'string' ? params.src : ''; | ||
| const partition = | ||
| typeof webPreferences.partition === 'string' ? webPreferences.partition : ''; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron will-attach-webview event params partition attribute
💡 Result:
The 'will-attach-webview' event is emitted on a WebContents (e.g., win.webContents) when a tag's contents are being attached. It allows configuring the guest WebContents before loading and preventing attachment via event.preventDefault. Event parameters: - event Event - webPreferences WebPreferences - The web preferences for the guest page (modifiable). - params Record<string, any> - Other parameters, including 'partition', 'src', etc. (modifiable). The 'partition' attribute from the tag is included in the 'params' object, which can be modified in the event handler (e.g., event.params.partition = 'new-value';). This provides finer control beyond HTML attributes, such as setting session partitions dynamically. Note: Requires webviewTag: true in BrowserWindow webPreferences. WebContents is created only after this event if not prevented (since recent versions). Example usage: win.webContents.on('will-attach-webview', (event, webPreferences, params) => { // Modify partition params.partition = 'persist:my-session'; // Or enforce/modify other settings webPreferences.nodeIntegration = false; // Prevent if invalid if (!params.src.startsWith('https://allowed.com')) { event.preventDefault; } });
Citations:
- 1: https://www.electronjs.org/docs/api/web-contents
- 2: Add will-attach-webview event electron/electron#8584
- 3: refactor: only create webContents after 'will-attach-webview' electron/electron#35016
- 4: https://electronjs.org/docs/latest/api/webview-tag
🏁 Script executed:
# Get the full context of the code at lines 268-271
sed -n '260,290p' src/main/app-lifecycle/window-manager.tsRepository: RunMaestro/Maestro
Length of output: 1310
🏁 Script executed:
# Search for how partition is used after being read
sed -n '268,310p' src/main/app-lifecycle/window-manager.tsRepository: RunMaestro/Maestro
Length of output: 1762
Read partition from params, not just webPreferences.
Electron's will-attach-webview event passes the <webview> tag's partition attribute via the params object, not webPreferences. The current code reads from webPreferences.partition, which will always be empty when a partition is specified in HTML, causing the security validation to fail for valid webviews.
Suggested fix
mainWindow.webContents.on('will-attach-webview', (event, webPreferences, params) => {
const src = typeof params.src === 'string' ? params.src : '';
- const partition =
- typeof webPreferences.partition === 'string' ? webPreferences.partition : '';
+ const partition =
+ typeof params.partition === 'string'
+ ? params.partition
+ : typeof webPreferences.partition === 'string'
+ ? webPreferences.partition
+ : '';📝 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
| mainWindow.webContents.on('will-attach-webview', (event, webPreferences, params) => { | |
| const src = typeof params.src === 'string' ? params.src : ''; | |
| const partition = | |
| typeof webPreferences.partition === 'string' ? webPreferences.partition : ''; | |
| mainWindow.webContents.on('will-attach-webview', (event, webPreferences, params) => { | |
| const src = typeof params.src === 'string' ? params.src : ''; | |
| const partition = | |
| typeof params.partition === 'string' | |
| ? params.partition | |
| : typeof webPreferences.partition === 'string' | |
| ? webPreferences.partition | |
| : ''; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/app-lifecycle/window-manager.ts` around lines 268 - 271, The
partition variable is being read from webPreferences but the webview tag's
partition attribute comes via params; update the will-attach-webview handler
(mainWindow.webContents.on('will-attach-webview', ...)) to read partition from
params.partition (falling back to webPreferences.partition if needed) so valid
HTML-specified partitions are validated correctly; keep the existing src
handling and security checks but use params.partition as the primary source.
Comment on lines
+156
to
+178
| const handleDidFailLoad = (event: Event) => { | ||
| if ((event as Event & { isMainFrame?: boolean }).isMainFrame === false) return; | ||
| const nextUrl = | ||
| (event as Event & { validatedURL?: string; url?: string }).validatedURL || | ||
| (event as Event & { validatedURL?: string; url?: string }).url || | ||
| webview.getURL?.() || | ||
| latestTabRef.current.url || | ||
| DEFAULT_BROWSER_TAB_URL; | ||
| if (!isAddressFocusedRef.current) { | ||
| setAddressValue(nextUrl); | ||
| } | ||
| setAddressError(null); | ||
| updateTabState({ | ||
| url: nextUrl, | ||
| title: getBrowserTabTitle(nextUrl), | ||
| canGoBack: isDomReadyRef.current ? webview.canGoBack() : latestTabRef.current.canGoBack, | ||
| canGoForward: isDomReadyRef.current | ||
| ? webview.canGoForward() | ||
| : latestTabRef.current.canGoForward, | ||
| isLoading: false, | ||
| webContentsId: webview.getWebContentsId?.(), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Ignore aborted did-fail-load events.
This handler treats every main-frame load failure as terminal. On redirects/reloads Electron also emits aborted failures, so Line 156 can clear isLoading and overwrite the tab URL/title with an intermediate navigation that never actually committed.
Suggested fix
const handleDidFailLoad = (event: Event) => {
if ((event as Event & { isMainFrame?: boolean }).isMainFrame === false) return;
+ if ((event as Event & { errorCode?: number }).errorCode === -3) return;
const nextUrl =
(event as Event & { validatedURL?: string; url?: string }).validatedURL ||
(event as Event & { validatedURL?: string; url?: string }).url ||📝 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 handleDidFailLoad = (event: Event) => { | |
| if ((event as Event & { isMainFrame?: boolean }).isMainFrame === false) return; | |
| const nextUrl = | |
| (event as Event & { validatedURL?: string; url?: string }).validatedURL || | |
| (event as Event & { validatedURL?: string; url?: string }).url || | |
| webview.getURL?.() || | |
| latestTabRef.current.url || | |
| DEFAULT_BROWSER_TAB_URL; | |
| if (!isAddressFocusedRef.current) { | |
| setAddressValue(nextUrl); | |
| } | |
| setAddressError(null); | |
| updateTabState({ | |
| url: nextUrl, | |
| title: getBrowserTabTitle(nextUrl), | |
| canGoBack: isDomReadyRef.current ? webview.canGoBack() : latestTabRef.current.canGoBack, | |
| canGoForward: isDomReadyRef.current | |
| ? webview.canGoForward() | |
| : latestTabRef.current.canGoForward, | |
| isLoading: false, | |
| webContentsId: webview.getWebContentsId?.(), | |
| }); | |
| }; | |
| const handleDidFailLoad = (event: Event) => { | |
| if ((event as Event & { isMainFrame?: boolean }).isMainFrame === false) return; | |
| if ((event as Event & { errorCode?: number }).errorCode === -3) return; | |
| const nextUrl = | |
| (event as Event & { validatedURL?: string; url?: string }).validatedURL || | |
| (event as Event & { validatedURL?: string; url?: string }).url || | |
| webview.getURL?.() || | |
| latestTabRef.current.url || | |
| DEFAULT_BROWSER_TAB_URL; | |
| if (!isAddressFocusedRef.current) { | |
| setAddressValue(nextUrl); | |
| } | |
| setAddressError(null); | |
| updateTabState({ | |
| url: nextUrl, | |
| title: getBrowserTabTitle(nextUrl), | |
| canGoBack: isDomReadyRef.current ? webview.canGoBack() : latestTabRef.current.canGoBack, | |
| canGoForward: isDomReadyRef.current | |
| ? webview.canGoForward() | |
| : latestTabRef.current.canGoForward, | |
| isLoading: false, | |
| webContentsId: webview.getWebContentsId?.(), | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/MainPanel/BrowserTabView.tsx` around lines 156 - 178,
The did-fail-load handler (handleDidFailLoad) should ignore aborted navigation
failures so temporary/redirect loads don't clear isLoading or overwrite
URL/title: add an early return for aborted errors by checking the event's
errorCode/errorDescription (Electron abort is errorCode === -3 /
errorDescription 'ERR_ABORTED') before using validatedURL/getURL/latestTabRef
and calling setAddressValue/setAddressError/updateTabState; keep the existing
main-frame check and only proceed for real failures.
Comment on lines
+244
to
+251
| {shortcutHint !== null && shortcutHint !== undefined && ( | ||
| <span | ||
| className="w-4 h-4 flex items-center justify-center rounded text-[10px] font-medium shrink-0 opacity-50" | ||
| style={{ backgroundColor: theme.colors.border, color: theme.colors.textMain }} | ||
| > | ||
| {shortcutHint} | ||
| </span> | ||
| )} |
There was a problem hiding this comment.
Browser tabs still miss some unified-tab affordances.
These shortcut and bulk-close paths only render when shortcutHint, onCloseOtherTabs, onCloseTabsLeft, onCloseTabsRight, totalTabs, and tabIndex are populated, but the current src/renderer/components/TabBar/TabBar.tsx:535-560 wiring shown in context does not pass them through. In practice that leaves browser tabs without the same close-other/left/right actions and shortcut badges the other unified tab types get.
Also applies to: 380-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/TabBar/BrowserTabItem.tsx` around lines 244 - 251,
Browser tabs are missing shortcut badges and bulk-close actions because
BrowserTabItem's props (shortcutHint, onCloseOtherTabs, onCloseTabsLeft,
onCloseTabsRight, totalTabs, tabIndex) are not being forwarded from the TabBar
wiring; update the TabBar rendering logic that instantiates BrowserTabItem
(where TabBar maps/creates tab items) to pass these props through using the
existing tab data and handlers (ensure shortcutHint is derived from the tab
metadata and the three close handlers and totalTabs/tabIndex are provided from
TabBar's state/props), so BrowserTabItem can render the shortcut badge and
close-left/close-right/close-others actions.
Comment on lines
+253
to
+260
| {tab.favicon && !faviconFailed ? ( | ||
| <img | ||
| src={tab.favicon} | ||
| alt="" | ||
| aria-hidden="true" | ||
| className="w-3.5 h-3.5 shrink-0 rounded-sm object-contain" | ||
| onError={() => setFaviconFailed(true)} | ||
| /> |
There was a problem hiding this comment.
Treat tab.favicon as untrusted input before rendering it.
This value comes from arbitrary page metadata, and feeding it straight into an app-level <img src> lets sites trigger renderer-side fetches for file:, custom schemes, or internal URLs whenever the tab strip renders. Please whitelist safe schemes here at minimum, or better, normalize favicons to trusted data URLs before they reach this component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/TabBar/BrowserTabItem.tsx` around lines 253 - 260,
The BrowserTabItem currently renders tab.favicon directly into an <img> (see
tab.favicon usage and setFaviconFailed in BrowserTabItem), which allows
untrusted schemes to trigger renderer fetches; sanitize or normalize the favicon
before use by either whitelisting allowed URI schemes (e.g., only http, https,
data) and rejecting/ignoring anything else (file:, about:, chrome:, custom
schemes), or convert/validate favicons upstream to trusted data URLs and only
pass safe data URLs into this component; ensure BrowserTabItem checks the
sanitized value and falls back to a default icon and/or calls setFaviconFailed
when the scheme is disallowed or parsing fails.
Comment on lines
+62
to
63
| if (!onNewTerminalTab && !onNewBrowserTab) { | ||
| onNewTab(); |
There was a problem hiding this comment.
Hide terminal option when terminal creation is unavailable.
Line 127–147 always renders New Terminal, but with browser-only mode (onNewBrowserTab present, onNewTerminalTab absent) that action does nothing.
💡 Suggested fix
- title={onNewTerminalTab ? 'New tab…' : `New tab (${formatShortcutKeys(newTabKeys)})`}
+ title={
+ onNewTerminalTab || onNewBrowserTab
+ ? 'New tab…'
+ : `New tab (${formatShortcutKeys(newTabKeys)})`
+ }
@@
- <button
- className="flex items-center gap-2 w-full px-3 py-2 text-sm text-left hover:bg-white/10 transition-colors"
- style={{ color: theme.colors.textMain }}
- onClick={() => closeAndDo(() => onNewTerminalTab?.())}
- >
- <Terminal className="w-3.5 h-3.5" style={{ color: theme.colors.textDim }} />
- New Terminal
- <span className="ml-auto text-xs" style={{ color: theme.colors.textDim }}>
- {formatShortcutKeys(terminalKeys)}
- </span>
- </button>
+ {onNewTerminalTab && (
+ <button
+ className="flex items-center gap-2 w-full px-3 py-2 text-sm text-left hover:bg-white/10 transition-colors"
+ style={{ color: theme.colors.textMain }}
+ onClick={() => closeAndDo(onNewTerminalTab)}
+ >
+ <Terminal className="w-3.5 h-3.5" style={{ color: theme.colors.textDim }} />
+ New Terminal
+ <span className="ml-auto text-xs" style={{ color: theme.colors.textDim }}>
+ {formatShortcutKeys(terminalKeys)}
+ </span>
+ </button>
+ )}Also applies to: 89-90, 127-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/TabBar/NewTabPopover.tsx` around lines 62 - 63, The
NewTabPopover always renders a "New Terminal" option even when onNewTerminalTab
is undefined; update the component (NewTabPopover) so the "New Terminal" menu
item/option is only rendered when onNewTerminalTab is truthy, and wire its click
handler to call onNewTerminalTab (fall back to onNewTab only if that is
intended); apply the same conditional rendering change for the other occurrences
referenced (the other menu blocks where "New Terminal" is shown, e.g., the
places around the checks for onNewBrowserTab/onNewTerminalTab) so the terminal
entry is hidden when terminal creation is unavailable.
Comment on lines
+386
to
+407
| let restoredActiveFileTabId = correctedSession.activeFileTabId ?? null; | ||
| let restoredActiveBrowserTabId = | ||
| correctedSession.activeBrowserTabId && | ||
| validBrowserTabIds.has(correctedSession.activeBrowserTabId) | ||
| ? correctedSession.activeBrowserTabId | ||
| : null; | ||
| const restoredActiveTerminalTabId = | ||
| correctedSession.activeTerminalTabId && | ||
| validTerminalTabIds.has(correctedSession.activeTerminalTabId) | ||
| ? correctedSession.activeTerminalTabId | ||
| : null; | ||
| let restoredInputMode = correctedSession.inputMode; | ||
|
|
||
| if (restoredInputMode === 'terminal') { | ||
| restoredActiveFileTabId = null; | ||
| restoredActiveBrowserTabId = null; | ||
| if (!restoredActiveTerminalTabId) { | ||
| restoredInputMode = 'ai'; | ||
| } | ||
| } else if (restoredActiveFileTabId) { | ||
| restoredActiveBrowserTabId = null; | ||
| } |
There was a problem hiding this comment.
Validate activeFileTabId before it suppresses browser focus.
restoredActiveFileTabId is copied straight from persisted state, unlike the AI/browser/terminal refs. If that file tab was deleted or the ID is stale, restore keeps an orphaned activeFileTabId and clears a valid activeBrowserTabId in the else if (restoredActiveFileTabId) branch, leaving the session with no valid non-AI selection.
Suggested fix
const resetBrowserTabs = (correctedSession.browserTabs || []).map((tab) =>
rehydrateBrowserTab(tab, correctedSession.id)
);
const validAiTabIds = new Set(resetAiTabs.map((tab) => tab.id));
+ const validFileTabIds = new Set(
+ (correctedSession.filePreviewTabs || []).map((tab) => tab.id)
+ );
const validBrowserTabIds = new Set(resetBrowserTabs.map((tab) => tab.id));
const validTerminalTabIds = new Set(resetTerminalTabs.map((tab) => tab.id));
const restoredActiveTabId = validAiTabIds.has(correctedSession.activeTabId)
? correctedSession.activeTabId
: resetAiTabs[0]?.id || correctedSession.activeTabId;
- let restoredActiveFileTabId = correctedSession.activeFileTabId ?? null;
+ let restoredActiveFileTabId =
+ correctedSession.activeFileTabId &&
+ validFileTabIds.has(correctedSession.activeFileTabId)
+ ? correctedSession.activeFileTabId
+ : null;
let restoredActiveBrowserTabId =
correctedSession.activeBrowserTabId &&
validBrowserTabIds.has(correctedSession.activeBrowserTabId)
? correctedSession.activeBrowserTabId
: null;📝 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
| let restoredActiveFileTabId = correctedSession.activeFileTabId ?? null; | |
| let restoredActiveBrowserTabId = | |
| correctedSession.activeBrowserTabId && | |
| validBrowserTabIds.has(correctedSession.activeBrowserTabId) | |
| ? correctedSession.activeBrowserTabId | |
| : null; | |
| const restoredActiveTerminalTabId = | |
| correctedSession.activeTerminalTabId && | |
| validTerminalTabIds.has(correctedSession.activeTerminalTabId) | |
| ? correctedSession.activeTerminalTabId | |
| : null; | |
| let restoredInputMode = correctedSession.inputMode; | |
| if (restoredInputMode === 'terminal') { | |
| restoredActiveFileTabId = null; | |
| restoredActiveBrowserTabId = null; | |
| if (!restoredActiveTerminalTabId) { | |
| restoredInputMode = 'ai'; | |
| } | |
| } else if (restoredActiveFileTabId) { | |
| restoredActiveBrowserTabId = null; | |
| } | |
| const resetBrowserTabs = (correctedSession.browserTabs || []).map((tab) => | |
| rehydrateBrowserTab(tab, correctedSession.id) | |
| ); | |
| const validAiTabIds = new Set(resetAiTabs.map((tab) => tab.id)); | |
| const validFileTabIds = new Set( | |
| (correctedSession.filePreviewTabs || []).map((tab) => tab.id) | |
| ); | |
| const validBrowserTabIds = new Set(resetBrowserTabs.map((tab) => tab.id)); | |
| const validTerminalTabIds = new Set(resetTerminalTabs.map((tab) => tab.id)); | |
| const restoredActiveTabId = validAiTabIds.has(correctedSession.activeTabId) | |
| ? correctedSession.activeTabId | |
| : resetAiTabs[0]?.id || correctedSession.activeTabId; | |
| let restoredActiveFileTabId = | |
| correctedSession.activeFileTabId && | |
| validFileTabIds.has(correctedSession.activeFileTabId) | |
| ? correctedSession.activeFileTabId | |
| : null; | |
| let restoredActiveBrowserTabId = | |
| correctedSession.activeBrowserTabId && | |
| validBrowserTabIds.has(correctedSession.activeBrowserTabId) | |
| ? correctedSession.activeBrowserTabId | |
| : null; | |
| const restoredActiveTerminalTabId = | |
| correctedSession.activeTerminalTabId && | |
| validTerminalTabIds.has(correctedSession.activeTerminalTabId) | |
| ? correctedSession.activeTerminalTabId | |
| : null; | |
| let restoredInputMode = correctedSession.inputMode; | |
| if (restoredInputMode === 'terminal') { | |
| restoredActiveFileTabId = null; | |
| restoredActiveBrowserTabId = null; | |
| if (!restoredActiveTerminalTabId) { | |
| restoredInputMode = 'ai'; | |
| } | |
| } else if (restoredActiveFileTabId) { | |
| restoredActiveBrowserTabId = null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/session/useSessionRestoration.ts` around lines 386 - 407,
restoredActiveFileTabId is taken directly from correctedSession.activeFileTabId
and can be stale, which then wrongly suppresses a valid
restoredActiveBrowserTabId in the else if (restoredActiveFileTabId) branch;
validate the file tab ID against the same set used for other types (e.g.,
validFileTabIds) when assigning restoredActiveFileTabId (similar to how
restoredActiveBrowserTabId and restoredActiveTerminalTabId are computed) and if
the ID is not in validFileTabIds set, set restoredActiveFileTabId = null before
the inputMode/branch logic so a valid browser selection is not accidentally
cleared.
Comment on lines
1197
to
1201
| ...session, | ||
| activeFileTabId: existingTab.id, | ||
| activeBrowserTabId: null, | ||
| unifiedTabOrder: ensureInUnifiedTabOrder(session.unifiedTabOrder, 'file', existingTab.id), | ||
| unifiedClosedTabHistory: remainingHistory, |
There was a problem hiding this comment.
Reopened file tabs won't surface from terminal mode.
Both file-tab reopen branches set activeFileTabId, but neither clears activeTerminalTabId nor forces inputMode back to 'ai'. Reopening a file tab while the terminal is active will leave the terminal mounted instead of the restored file.
Suggested fix
session: {
...session,
activeFileTabId: existingTab.id,
activeBrowserTabId: null,
+ activeTerminalTabId: null,
+ inputMode: 'ai',
unifiedTabOrder: ensureInUnifiedTabOrder(session.unifiedTabOrder, 'file', existingTab.id),
unifiedClosedTabHistory: remainingHistory,
},
...
session: {
...session,
filePreviewTabs: updatedFilePreviewTabs,
activeFileTabId: restoredTab.id,
activeBrowserTabId: null,
+ activeTerminalTabId: null,
+ inputMode: 'ai',
unifiedTabOrder: updatedUnifiedTabOrder,
unifiedClosedTabHistory: remainingHistory,
},Also applies to: 1237-1243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/utils/tabHelpers.ts` around lines 1197 - 1201, The
reopen-file-tab branches set activeFileTabId but forget to clear
activeTerminalTabId and reset inputMode, so reopening a file while a terminal is
active leaves the terminal mounted; update both branches that set
activeFileTabId (the one using existingTab.id and the other around lines
1237-1243) to also set activeTerminalTabId: null and inputMode: 'ai' after
ensuring unifiedTabOrder/unifiedClosedTabHistory (referencing session,
existingTab.id, remainingHistory, ensureInUnifiedTabOrder) so the UI actually
switches from terminal mode back to the file tab.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/dev-port.mjs (1)
16-40: Consider adding a connection timeout to prevent hangs.The
isPortInUsefunction relies on implicit OS-level TCP timeouts. On some systems or network configurations, a connection attempt to a port that isn't explicitly refused could hang for an extended period before the OS times out.💡 Proposed fix to add explicit timeout
function isPortInUse(port) { return new Promise((resolve) => { let resolved = false; let pending = LOCALHOST_HOSTS.length; + const SOCKET_TIMEOUT_MS = 1000; for (const host of LOCALHOST_HOSTS) { const socket = net.createConnection({ host, port }); + socket.setTimeout(SOCKET_TIMEOUT_MS); socket.once('connect', () => { if (resolved) return; resolved = true; socket.end(); resolve(true); }); + socket.once('timeout', () => { + socket.destroy(); + pending -= 1; + if (!resolved && pending === 0) { + resolve(false); + } + }); + socket.once('error', () => { socket.destroy(); pending -= 1; if (!resolved && pending === 0) { resolve(false); } }); } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/dev-port.mjs` around lines 16 - 40, The isPortInUse function can hang waiting on OS TCP timeouts; modify it to set an explicit per-socket connection timeout by using socket.setTimeout(...) after creating the socket (and handle the 'timeout' event similar to 'error' by destroying the socket, decrementing pending, and resolving false when all hosts done). Ensure you clear the timeout/handlers when 'connect' succeeds (socket.end()) and avoid double-resolve by keeping the resolved flag; reference LOCALHOST_HOSTS, net.createConnection, socket, and the pending/resolved logic when implementing the timeout (choose a sensible timeout like a few hundred milliseconds).scripts/start-dev.ps1 (1)
17-19: Hardcoded 5-second sleep is less robust thandev.mjs's port polling.Unlike
dev.mjswhich actively polls the port for up to 20 seconds before launching Electron, this script uses a fixed 5-second sleep. On slower machines or under heavy load, Vite might not be ready in time, causing Electron to fail to connect.Consider adding a PowerShell equivalent of port polling for parity with the Unix flow. If this is acceptable as-is for Windows development, the current approach works for typical scenarios.
💡 Optional: Add port polling to match dev.mjs behavior
Write-Host "Waiting for renderer dev server on port $vitePort..." -ForegroundColor Yellow -Start-Sleep -Seconds 5 +$timeout = 20 +$elapsed = 0 +while ($elapsed -lt $timeout) { + try { + $tcpClient = New-Object System.Net.Sockets.TcpClient + $tcpClient.Connect('127.0.0.1', $vitePort) + $tcpClient.Close() + break + } catch { + Start-Sleep -Seconds 1 + $elapsed++ + } +} +if ($elapsed -ge $timeout) { + Write-Host "Timed out waiting for Vite dev server on port $vitePort" -ForegroundColor Red + exit 1 +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/start-dev.ps1` around lines 17 - 19, Replace the fixed Start-Sleep 5 with a PowerShell port-polling loop that checks $vitePort readiness (e.g., using Test-NetConnection or System.Net.Sockets.TcpClient) and retries at short intervals up to ~20 seconds before continuing; keep the existing Write-Host message, poll until the port is reachable or timeout, and only proceed to launch Electron when the check for $vitePort succeeds to mirror the dev.mjs behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/dev-port.mjs`:
- Around line 16-40: The isPortInUse function can hang waiting on OS TCP
timeouts; modify it to set an explicit per-socket connection timeout by using
socket.setTimeout(...) after creating the socket (and handle the 'timeout' event
similar to 'error' by destroying the socket, decrementing pending, and resolving
false when all hosts done). Ensure you clear the timeout/handlers when 'connect'
succeeds (socket.end()) and avoid double-resolve by keeping the resolved flag;
reference LOCALHOST_HOSTS, net.createConnection, socket, and the
pending/resolved logic when implementing the timeout (choose a sensible timeout
like a few hundred milliseconds).
In `@scripts/start-dev.ps1`:
- Around line 17-19: Replace the fixed Start-Sleep 5 with a PowerShell
port-polling loop that checks $vitePort readiness (e.g., using
Test-NetConnection or System.Net.Sockets.TcpClient) and retries at short
intervals up to ~20 seconds before continuing; keep the existing Write-Host
message, poll until the port is reachable or timeout, and only proceed to launch
Electron when the check for $vitePort succeeds to mirror the dev.mjs behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e15725a7-f2f7-4d89-a249-b88f3a47926d
📒 Files selected for processing (6)
package.jsonscripts/dev-port.mjsscripts/dev.mjsscripts/start-dev.ps1src/main/deep-links.tsvite.config.mts
Contributor
Author
|
8 tasks
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
This ports the
rcbrowser-tab work into Maestro's unified tab strip and removes the older Globe-button entry path so browser content behaves like any other main-panel tab.Screenshot
Validation
npm run test:e2e -- e2e/browser-tab.spec.tsSummary by CodeRabbit
New Features
Security / Behavior
Bug Fixes