feat(desktop): port browser pane to v2 workspaces with global persistence#3346
Merged
saddlepaddle merged 4 commits intomainfrom Apr 11, 2026
Merged
feat(desktop): port browser pane to v2 workspaces with global persistence#3346saddlepaddle merged 4 commits intomainfrom
saddlepaddle merged 4 commits intomainfrom
Conversation
…ence - Port v1 browser pane (webview, URL toolbar, history, error overlay, new-window handler) into v2 workspace tree as a native implementation. Uses the v2 Workspace component's renderToolbar slot for a Chrome-style URL bar in the pane header. - Float-over webview: each <webview> lives at document.body under a fixed #browser-runtime-root, positioned via ResizeObserver on a per-pane placeholder div. Elements never reparent, so Electron's guest WebContents survives tab switches, pane drags, and workspace route remounts — no page reloads. - Keep-alive tab rendering in @superset/panes Workspace.tsx: tabs that have been visited stay mounted behind visibility:hidden so their React state (and persistent DOM like webviews) survives tab switches. Also benefits terminal scrollback, chat subscriptions, editor state. - BrowserRuntimeRegistry: module-level singleton keyed by paneId, modeled on terminalRuntimeRegistry. Owns webview lifecycle, state subscriptions via useSyncExternalStore, drag-passthrough listeners, equality-guarded setState to deduplicate webview event spam. - Single-pane-browser tabs show the page favicon + live page title in the tab bar via a new optional renderTabLabel prop on the panes library. - BrowserPane subscribes to the workspace store for activeTabId to drive webview visibility, so inactive-tab webviews don't paint on top of the active tab's content. - Destroy path: diff the set of browser pane ids across all tabs each render, destroying any that disappeared. Correctly handles tab closes, intra-tab split closes, and pane drags between tabs (moved panes stay in the set). Known limitation: dropping panes onto another browser pane's body doesn't register — the webview compositor layer intercepts drag events even with pointer-events:none/visibility:hidden. Drops on pane headers work. Deferred.
Contributor
📝 WalkthroughWalkthroughAdds a full Browser pane feature: a browser runtime registry managing Electron webviews, new BrowserPane UI (toolbar, overflow menu, error overlay, URL autocomplete), URL sanitization/defaults, pane lifecycle hooks, and workspace/tab API extensions for custom tab titles/icons and cleanup. Changes
Sequence DiagramsequenceDiagram
participant React as BrowserPane Component
participant Registry as BrowserRuntimeRegistry
participant Webview as Electron.WebviewTag
participant TRPC as TRPC Client
participant DOM as Placeholder/Window
React->>Registry: attach(paneId, placeholderEl, initialUrl)
Registry->>DOM: create root container & placeholder observer
Registry->>Webview: create webview, set src, bind event listeners
Webview->>Registry: dom-ready / did-start-loading
Registry->>Registry: set isLoading, update state
Webview->>Registry: did-navigate / page-title-updated / page-favicon-updated
Registry->>TRPC: upsert history / register webContentsId
Webview->>Registry: did-stop-loading / did-fail-load
Registry->>React: notify onStateChange (url, title, favicon, error, nav state)
React->>React: render toolbar, overlays, and url/autocomplete based on state
React->>Registry: navigate/goBack/goForward/reload (user actions)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR ports the v1 browser pane into v2 workspaces using a float-over webview architecture — every Key design decisions are sound and well-reasoned:
Issues found:
Confidence Score: 4/5Safe to merge; the architecture is solid and the known issues are non-blocking bugs with limited practical impact. The float-over webview design is the correct solution to Electron's reparenting constraint, and the implementation is well-structured. The
Important Files Changed
Sequence DiagramsequenceDiagram
participant React as BrowserPane
participant Hook as usePersistentWebview
participant Registry as browserRuntimeRegistry
participant DOM as browser-runtime-root
participant WV as webview element
participant Main as Main Process tRPC
React->>Hook: mount with paneId and placeholder
Hook->>Registry: attach(paneId, placeholder, initialUrl)
Registry->>DOM: appendChild(webview) — first time only
Registry->>Registry: ResizeObserver on placeholder
Registry-->>WV: position via fixed CSS top/left/width/height
Note over WV,Main: User navigates to URL
WV->>Registry: did-start-loading — isLoading true
WV->>Registry: did-navigate — setState currentUrl and pageTitle
WV->>Registry: did-stop-loading — pushHistory and onDidStopLoading
Registry->>Main: browserHistory.upsert
Registry->>Hook: onDidStopLoading callback
Hook->>React: ctx.actions.updateData
Note over React,WV: Tab switch away — keep-alive
React->>Registry: setVisibility(paneId, false)
Registry-->>WV: visibility hidden — stays in DOM, no reload
Note over React,WV: Tab switch back
React->>Registry: setVisibility(paneId, true)
Registry->>Registry: updateLayout entry
Registry-->>WV: visibility visible
Note over Hook,WV: Workspace unmount
Hook->>Registry: detach(paneId)
Registry-->>WV: hidden and observer stopped
Note over React,Main: Tab closed via diff effect
React->>Registry: destroy(paneId)
Registry->>DOM: webview.remove()
Registry->>Main: browser.unregister
Reviews (1): Last reviewed commit: "feat(desktop): port browser pane to v2 w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+214
to
+241
| const handleDidStopLoading = () => { | ||
| if (entry.isHistoryNavigation) { | ||
| entry.isHistoryNavigation = false; | ||
| this.setState(paneId, { isLoading: false }); | ||
| return; | ||
| } | ||
| const url = webview.getURL() ?? ""; | ||
| const title = webview.getTitle() ?? ""; | ||
| this.setState(paneId, { | ||
| isLoading: false, | ||
| currentUrl: url, | ||
| pageTitle: title, | ||
| }); | ||
| if (url && url !== "about:blank") { | ||
| this.pushHistory(paneId, url, title); | ||
| electronTrpcClient.browserHistory.upsert | ||
| .mutate({ url, title, faviconUrl: entry.state.faviconUrl }) | ||
| .catch((err) => { | ||
| console.error("[browserRuntimeRegistry] upsert history:", err); | ||
| }); | ||
| } | ||
| entry.onDidStopLoading?.({ url, title }); | ||
| }; | ||
|
|
||
| const handleDidNavigate = (e: Electron.DidNavigateEvent) => { | ||
| if (entry.isHistoryNavigation) { | ||
| entry.isHistoryNavigation = false; | ||
| return; |
There was a problem hiding this comment.
isHistoryNavigation guard is dead code in handleDidStopLoading
In Electron, did-navigate always fires before did-stop-loading. That means handleDidNavigate (lines 238–241) resets entry.isHistoryNavigation = false and returns early — so by the time handleDidStopLoading reaches its own guard on line 215, the flag is already false.
As a result, for every back/forward navigation:
- The guard
if (entry.isHistoryNavigation)at line 215 is never true, making lines 216–219 dead code. pushHistoryis still called (though it correctly deduplicates because the target URL is already at the currenthistoryIndex, so no extra entry is added).entry.onDidStopLoading?.()is called, triggeringctx.actions.updateData(benign but unintended).electronTrpcClient.browserHistory.upsert.mutate()fires for every back/forward navigation (extra IPC call).
The fix is to only reset the flag inside handleDidStopLoading, not in handleDidNavigate:
// handleDidNavigate — don't reset the flag; just skip state update
const handleDidNavigate = (e: Electron.DidNavigateEvent) => {
if (entry.isHistoryNavigation) {
// flag stays true — handleDidStopLoading will clear it
return;
}
// ...
};
// handleDidStopLoading — single place that resets the flag
const handleDidStopLoading = () => {
if (entry.isHistoryNavigation) {
entry.isHistoryNavigation = false;
this.setState(paneId, { isLoading: false });
return;
}
// ...
};The same issue applies to handleDidNavigateInPage (line 254) which also resets the flag early, though in-page navigations typically don't trigger did-stop-loading so it's less impactful in practice.
Comment on lines
+210
to
+212
| const handleDidStartLoading = () => { | ||
| this.setState(paneId, { isLoading: true, error: null }); | ||
| }; |
There was a problem hiding this comment.
handleDidStartLoading clears isLoading and error but doesn't reset faviconUrl. If a user navigates from a page that has a favicon to one that doesn't (or whose favicon loads slowly), the old favicon will persist in both the registry state and the BrowserTabLabel until page-favicon-updated fires for the new page — or indefinitely if it never fires.
Consider clearing faviconUrl when loading begins:
Suggested change
| const handleDidStartLoading = () => { | |
| this.setState(paneId, { isLoading: true, error: null }); | |
| }; | |
| const handleDidStartLoading = () => { | |
| this.setState(paneId, { isLoading: true, error: null, faviconUrl: null }); | |
| }; |
Comment on lines
+177
to
+191
| const getTabTitle = useCallback( | ||
| (tab: Tab<PaneViewerData>): string => { | ||
| const browserPaneId = getSingleBrowserPaneId(tab); | ||
| if (!browserPaneId) return tab.titleOverride ?? tab.id; | ||
| const state = browserRuntimeRegistry.getState(browserPaneId); | ||
| if (state.pageTitle) return state.pageTitle; | ||
| if (state.currentUrl && state.currentUrl !== "about:blank") { | ||
| try { | ||
| return new URL(state.currentUrl).hostname; | ||
| } catch {} | ||
| } | ||
| return tab.titleOverride ?? "Browser"; | ||
| }, | ||
| [getSingleBrowserPaneId], | ||
| ); |
There was a problem hiding this comment.
getTabTitle is not reactive — tooltip shows stale browser tab titles
getTabTitle reads browserRuntimeRegistry.getState(browserPaneId) at render time but is not subscribed to registry state changes. TabItem uses this value for its <TooltipContent> (the title shown on hover), and won't re-render when browser state changes — only when the tab array itself changes (reorder, add, remove).
The visible label is handled correctly by BrowserTabLabel (via useSyncExternalStore), so the tab content is reactive. However, hovering the tab will show a stale title until the next unrelated re-render.
If the tooltip title needs to stay live, one approach is to have BrowserTabLabel own the full rendered label and include the title for the tooltip slot too. Alternatively, noting this as a known limitation is acceptable given the complexity involved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/hooks/useUrlAutocomplete/useUrlAutocomplete.ts (1)
21-52: Remove unusedsuggestionsRefto simplify hook state.
suggestionsRefis written but never consumed, so it can be safely removed to reduce noise.♻️ Suggested cleanup
- const suggestionsRef = useRef<HistorySuggestion[]>([]); @@ - suggestionsRef.current = suggestions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/hooks/useUrlAutocomplete/useUrlAutocomplete.ts around lines 21 - 52, Remove the unused suggestionsRef to simplify state: delete the const suggestionsRef = useRef<HistorySuggestion[]>([]); declaration and the trailing assignment suggestionsRef.current = suggestions; in the useUrlAutocomplete hook, and also remove any now-unused useRef import; keep all existing logic that computes suggestions (allHistory, suggestions, useEffect) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserErrorOverlay/BrowserErrorOverlay.tsx:
- Around line 94-100: The copy button inside BrowserErrorOverlay is an icon-only
control with no accessible name; update the button (the element with
onClick={copyDetails}) to provide an accessible label by adding either an
aria-label (e.g., aria-label="Copy error details") or aria-labelledby/visually
hidden descriptive text, ensuring screen readers announce its purpose; keep the
existing onClick and classes intact and prefer a localized/descriptive string if
available.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserOverflowMenu/BrowserOverflowMenu.tsx:
- Around line 67-72: The overflow trigger in BrowserOverflowMenu is an icon-only
button (the button wrapping TbDots) with no accessible name; add a clear
accessible label by giving that button an aria-label (e.g., "More options" or
"Open menu") or include visually hidden text inside the BrowserOverflowMenu
component so assistive tech can announce it, and optionally add
aria-haspopup="menu" to indicate it opens a menu.
- Around line 32-61: The handlers handleScreenshot, handleHardReload,
handleCopyUrl, handleClearCookies, handleClearHistory and handleClearAllData
currently swallow errors via `.catch(() => {})`; change each to surface failures
by catching the error and reporting it (e.g., call a user-facing
notification/toast API and log the error) so users see when destructive/browser
actions fail and can retry; implement either async/await with try/catch or
replace `.catch(() => {})` with `.catch(err => { logger.error(err, "action
failed"); notifyUser("Action failed: " + err.message) })` (use your project's
notify/log utilities) and ensure handleCopyUrl also reports if copyToClipboard
fails.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserTabLabel/BrowserTabLabel.tsx:
- Around line 46-52: BrowserTabLabel.tsx currently binds raw remote faviconUrl
(from page-favicon-updated in browserRuntimeRegistry.ts) directly to the <img
src>, causing the renderer to make external requests outside the webview
session; change this to use a session-backed source by either (a) proxying the
favicon through the main process (implement an IPC or main-endpoint that fetches
the favicon using the webview's session and returns a proxied URL) and update
BrowserTabLabel to request that proxied URL instead of faviconUrl, or (b) have
the main process fetch/convert the remote favicon to a data URL (via IPC) and
set the img src to that data URL; update usages of faviconUrl and
setBrokenFaviconUrl in BrowserTabLabel to accept the proxied/data URL and ensure
the browserRuntimeRegistry.ts emission triggers the main-process
fetch/conversion rather than exposing raw remote URLs to the renderer.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/components/UrlSuggestions/UrlSuggestions.tsx:
- Around line 33-39: The button in UrlSuggestions.tsx currently only handles
selection via onMouseDown which breaks keyboard activation; keep the existing
onMouseDown (for blur prevention) and add an onClick handler on the same button
that calls onSelect(item.url) so keyboard/enter/space activation works (retain
key={item.url} and type="button" and do not remove e.preventDefault() from
onMouseDown).
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/sanitizeUrl.ts:
- Around line 1-11: sanitizeUrl currently treats any input with a dot as a
hostname; first trim the input and then treat inputs containing whitespace as
search queries; only auto-prefix `http(s)://` for inputs that either already
have a scheme (`^https?://`, `about:`) or match a host/path-like pattern (e.g.,
host labels separated by dots, optional port and path) or the explicit
`localhost`/`127.0.0.1` cases; otherwise return a google search URL with
encodeURIComponent of the trimmed input. Update the sanitizeUrl function to
perform trim(), check for spaces before the `url.includes(".")` branch, and
replace the broad dot-check with a stricter host/path regex to decide when to
prefix `https://`.
In `@packages/panes/src/react/components/Workspace/Workspace.tsx`:
- Around line 29-37: The active tab can render blank because activeTabId is
added to mountedTabIds only in useEffect after render; update the logic so the
active tab is considered mounted synchronously by including activeTabId in any
filters and in the setMountedTabIds updater (i.e., inside the useEffect that
calls setMountedTabIds, ensure you treat prev.has(activeTabId) as including
activeTabId immediately or merge activeTabId into the returned Set), and apply
the same change to the similar filter logic around the other occurrence (the
block referenced at lines ~100-102) so the active tab is always visible on
initial render.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/hooks/useUrlAutocomplete/useUrlAutocomplete.ts:
- Around line 21-52: Remove the unused suggestionsRef to simplify state: delete
the const suggestionsRef = useRef<HistorySuggestion[]>([]); declaration and the
trailing assignment suggestionsRef.current = suggestions; in the
useUrlAutocomplete hook, and also remove any now-unused useRef import; keep all
existing logic that computes suggestions (allHistory, suggestions, useEffect)
intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c07f4b1-1f6f-4226-8c1b-442265188673
📒 Files selected for processing (28)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDefaultContextMenuActions/useDefaultContextMenuActions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserErrorOverlay/BrowserErrorOverlay.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserErrorOverlay/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserOverflowMenu/BrowserOverflowMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserOverflowMenu/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserTabLabel/BrowserTabLabel.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserTabLabel/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/components/UrlSuggestions/UrlSuggestions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/components/UrlSuggestions/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/hooks/useUrlAutocomplete/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/hooks/useUrlAutocomplete/useUrlAutocomplete.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/constants.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/sanitizeUrl.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.tspackages/panes/src/react/components/Workspace/Workspace.tsxpackages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsxpackages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsxpackages/panes/src/react/types.ts
Comment on lines
+94
to
+100
| <button | ||
| type="button" | ||
| onClick={copyDetails} | ||
| className="shrink-0 text-muted-foreground/40 hover:text-muted-foreground/70 transition-colors" | ||
| > | ||
| <TbCopy className="size-4" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add an accessible name to the copy button.
The icon-only control here has no text or aria-label, so screen readers will announce it as an unlabeled button.
Suggested fix
<button
type="button"
onClick={copyDetails}
+ aria-label="Copy error details"
className="shrink-0 text-muted-foreground/40 hover:text-muted-foreground/70 transition-colors"
>📝 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
| <button | |
| type="button" | |
| onClick={copyDetails} | |
| className="shrink-0 text-muted-foreground/40 hover:text-muted-foreground/70 transition-colors" | |
| > | |
| <TbCopy className="size-4" /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={copyDetails} | |
| aria-label="Copy error details" | |
| className="shrink-0 text-muted-foreground/40 hover:text-muted-foreground/70 transition-colors" | |
| > | |
| <TbCopy className="size-4" /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserErrorOverlay/BrowserErrorOverlay.tsx
around lines 94 - 100, The copy button inside BrowserErrorOverlay is an
icon-only control with no accessible name; update the button (the element with
onClick={copyDetails}) to provide an accessible label by adding either an
aria-label (e.g., aria-label="Copy error details") or aria-labelledby/visually
hidden descriptive text, ensuring screen readers announce its purpose; keep the
existing onClick and classes intact and prefer a localized/descriptive string if
available.
Comment on lines
+32
to
+61
| const handleScreenshot = () => { | ||
| electronTrpcClient.browser.screenshot.mutate({ paneId }).catch(() => {}); | ||
| }; | ||
|
|
||
| const handleHardReload = () => { | ||
| electronTrpcClient.browser.reload | ||
| .mutate({ paneId, hard: true }) | ||
| .catch(() => {}); | ||
| }; | ||
|
|
||
| const handleCopyUrl = () => { | ||
| if (currentUrl) { | ||
| copyToClipboard(currentUrl); | ||
| } | ||
| }; | ||
|
|
||
| const handleClearCookies = () => { | ||
| electronTrpcClient.browser.clearBrowsingData | ||
| .mutate({ type: "cookies" }) | ||
| .catch(() => {}); | ||
| }; | ||
|
|
||
| const handleClearHistory = () => { | ||
| electronTrpcClient.browserHistory.clear.mutate().catch(() => {}); | ||
| }; | ||
|
|
||
| const handleClearAllData = () => { | ||
| electronTrpcClient.browser.clearBrowsingData | ||
| .mutate({ type: "all" }) | ||
| .catch(() => {}); |
Contributor
There was a problem hiding this comment.
Don't swallow browser action failures.
These mutations include destructive/global actions, but every failure is dropped with .catch(() => {}). If screenshot, reload, or data-clearing fails, the UI still looks successful and the user gets no recovery path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserOverflowMenu/BrowserOverflowMenu.tsx
around lines 32 - 61, The handlers handleScreenshot, handleHardReload,
handleCopyUrl, handleClearCookies, handleClearHistory and handleClearAllData
currently swallow errors via `.catch(() => {})`; change each to surface failures
by catching the error and reporting it (e.g., call a user-facing
notification/toast API and log the error) so users see when destructive/browser
actions fail and can retry; implement either async/await with try/catch or
replace `.catch(() => {})` with `.catch(err => { logger.error(err, "action
failed"); notifyUser("Action failed: " + err.message) })` (use your project's
notify/log utilities) and ensure handleCopyUrl also reports if copyToClipboard
fails.
Comment on lines
+67
to
+72
| <button | ||
| type="button" | ||
| className="rounded p-0.5 text-muted-foreground/60 transition-colors hover:text-muted-foreground" | ||
| > | ||
| <TbDots className="size-3.5" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Label the overflow trigger for assistive tech.
This is an icon-only button with no accessible name.
Suggested fix
<button
type="button"
+ aria-label="Browser actions"
className="rounded p-0.5 text-muted-foreground/60 transition-colors hover:text-muted-foreground"
>📝 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
| <button | |
| type="button" | |
| className="rounded p-0.5 text-muted-foreground/60 transition-colors hover:text-muted-foreground" | |
| > | |
| <TbDots className="size-3.5" /> | |
| </button> | |
| <button | |
| type="button" | |
| aria-label="Browser actions" | |
| className="rounded p-0.5 text-muted-foreground/60 transition-colors hover:text-muted-foreground" | |
| > | |
| <TbDots className="size-3.5" /> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserOverflowMenu/BrowserOverflowMenu.tsx
around lines 67 - 72, The overflow trigger in BrowserOverflowMenu is an
icon-only button (the button wrapping TbDots) with no accessible name; add a
clear accessible label by giving that button an aria-label (e.g., "More options"
or "Open menu") or include visually hidden text inside the BrowserOverflowMenu
component so assistive tech can announce it, and optionally add
aria-haspopup="menu" to indicate it opens a menu.
Comment on lines
+33
to
+39
| <button | ||
| key={item.url} | ||
| type="button" | ||
| onMouseDown={(e) => { | ||
| e.preventDefault(); | ||
| onSelect(item.url); | ||
| }} |
Contributor
There was a problem hiding this comment.
Add onClick alongside onMouseDown for keyboard activation.
Current selection is mouse-driven only. Adding onClick preserves keyboard/button activation semantics while keeping onMouseDown for blur prevention.
♿ Suggested fix
onMouseDown={(e) => {
e.preventDefault();
onSelect(item.url);
}}
+ onClick={() => {
+ onSelect(item.url);
+ }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/components/UrlSuggestions/UrlSuggestions.tsx
around lines 33 - 39, The button in UrlSuggestions.tsx currently only handles
selection via onMouseDown which breaks keyboard activation; keep the existing
onMouseDown (for blur prevention) and add an onClick handler on the same button
that calls onSelect(item.url) so keyboard/enter/space activation works (retain
key={item.url} and type="button" and do not remove e.preventDefault() from
onMouseDown).
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
11 issues found across 28 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsx:14">
P2: Avoid stripping trailing slashes from URLs, because submitting an unchanged address bar can navigate to a different URL.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/sanitizeUrl.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/sanitizeUrl.ts:8">
P2: `url.includes(".")` is too broad for URL detection and misclassifies dotted search queries as URLs.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/hooks/useUrlAutocomplete/useUrlAutocomplete.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/hooks/useUrlAutocomplete/useUrlAutocomplete.ts:31">
P2: Do not silently swallow autocomplete history fetch errors; add observable handling (at least a warning log) so failures can be diagnosed.
(Based on your team's feedback about handling async errors explicitly and avoiding empty catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts:210">
P2: `handleDidStartLoading` clears `error` but not `faviconUrl`. When navigating from a page with a favicon to one without, the old favicon persists indefinitely (since `page-favicon-updated` never fires for the new page). Clear `faviconUrl` when loading begins.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts:215">
P1: `isHistoryNavigation` flag is consumed by `handleDidNavigate` before `handleDidStopLoading` gets to check it. In Electron, `did-navigate` fires before `did-stop-loading`, so the flag is always `false` by the time `handleDidStopLoading` runs. This causes back/forward navigations to fall through to the normal path — calling `pushHistory` (which can truncate forward history if the URL differs even slightly) and making a redundant `browserHistory.upsert` call.
Remove the flag reset here; let `handleDidStopLoading` be the sole consumer.</violation>
<violation number="3" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts:407">
P2: Silent `.catch(() => {})` swallows errors with no logging. At minimum, log a warning so failures in `unregister` are observable during debugging.
(Based on your team's feedback about handling async errors explicitly and avoiding silent catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserErrorOverlay/BrowserErrorOverlay.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserErrorOverlay/BrowserErrorOverlay.tsx:63">
P2: Handle clipboard copy failures explicitly to avoid unhandled promise rejections from the async IPC call.</violation>
</file>
<file name="packages/panes/src/react/components/Workspace/Workspace.tsx">
<violation number="1" location="packages/panes/src/react/components/Workspace/Workspace.tsx:101">
P2: Include the active tab in the render filter immediately; relying on the post-render effect causes a blank frame when first switching to an unmounted tab.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx:78">
P2: Do not silently swallow `openDevTools` failures; log or otherwise surface the error so this failure path is observable.
(Based on your team's feedback about handling async errors and avoiding empty catch blocks.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:181">
P3: `getTabTitle` reads `browserRuntimeRegistry.getState()` at render time but is not subscribed to registry state changes. The tooltip will show a stale title until an unrelated re-render happens. The visible tab label is correctly reactive via `BrowserTabLabel` using `useSyncExternalStore`, but the tooltip title computed here will drift.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserOverflowMenu/BrowserOverflowMenu.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserOverflowMenu/BrowserOverflowMenu.tsx:33">
P2: Every browser action failure is silently swallowed with `.catch(() => {})`. These include destructive/global operations like clearing cookies, clearing all browsing data, and taking screenshots. If any of these fail, the user gets no feedback and no recovery path. At minimum, log the error or show a toast on failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| function displayUrl(url: string): string { | ||
| if (url === "about:blank") return ""; | ||
| return url.endsWith("/") ? url.slice(0, -1) : url; |
Contributor
There was a problem hiding this comment.
P2: Avoid stripping trailing slashes from URLs, because submitting an unchanged address bar can navigate to a different URL.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsx, line 14:
<comment>Avoid stripping trailing slashes from URLs, because submitting an unchanged address bar can navigate to a different URL.</comment>
<file context>
@@ -0,0 +1,215 @@
+
+function displayUrl(url: string): string {
+ if (url === "about:blank") return "";
+ return url.endsWith("/") ? url.slice(0, -1) : url;
+}
+
</file context>
| .then((items) => { | ||
| if (!cancelled) setAllHistory(items as HistorySuggestion[]); | ||
| }) | ||
| .catch(() => {}); |
Contributor
There was a problem hiding this comment.
P2: Do not silently swallow autocomplete history fetch errors; add observable handling (at least a warning log) so failures can be diagnosed.
(Based on your team's feedback about handling async errors explicitly and avoiding empty catch blocks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserToolbar/hooks/useUrlAutocomplete/useUrlAutocomplete.ts, line 31:
<comment>Do not silently swallow autocomplete history fetch errors; add observable handling (at least a warning log) so failures can be diagnosed.
(Based on your team's feedback about handling async errors explicitly and avoiding empty catch blocks.) </comment>
<file context>
@@ -0,0 +1,129 @@
+ .then((items) => {
+ if (!cancelled) setAllHistory(items as HistorySuggestion[]);
+ })
+ .catch(() => {});
+ return () => {
+ cancelled = true;
</file context>
| entry.webview.remove(); | ||
| this.entries.delete(paneId); | ||
| this.listenersByPaneId.delete(paneId); | ||
| electronTrpcClient.browser.unregister.mutate({ paneId }).catch(() => {}); |
Contributor
There was a problem hiding this comment.
P2: Silent .catch(() => {}) swallows errors with no logging. At minimum, log a warning so failures in unregister are observable during debugging.
(Based on your team's feedback about handling async errors explicitly and avoiding silent catch blocks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts, line 407:
<comment>Silent `.catch(() => {})` swallows errors with no logging. At minimum, log a warning so failures in `unregister` are observable during debugging.
(Based on your team's feedback about handling async errors explicitly and avoiding silent catch blocks.) </comment>
<file context>
@@ -0,0 +1,483 @@
+ entry.webview.remove();
+ this.entries.delete(paneId);
+ this.listenersByPaneId.delete(paneId);
+ electronTrpcClient.browser.unregister.mutate({ paneId }).catch(() => {});
+ }
+
</file context>
| const state = useBrowserState(paneId); | ||
|
|
||
| const handleOpenDevTools = useCallback(() => { | ||
| electronTrpcClient.browser.openDevTools.mutate({ paneId }).catch(() => {}); |
Contributor
There was a problem hiding this comment.
P2: Do not silently swallow openDevTools failures; log or otherwise surface the error so this failure path is observable.
(Based on your team's feedback about handling async errors and avoiding empty catch blocks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx, line 78:
<comment>Do not silently swallow `openDevTools` failures; log or otherwise surface the error so this failure path is observable.
(Based on your team's feedback about handling async errors and avoiding empty catch blocks.) </comment>
<file context>
@@ -0,0 +1,142 @@
+ const state = useBrowserState(paneId);
+
+ const handleOpenDevTools = useCallback(() => {
+ electronTrpcClient.browser.openDevTools.mutate({ paneId }).catch(() => {});
+ }, [paneId]);
+
</file context>
| const { copyToClipboard } = useCopyToClipboard(); | ||
|
|
||
| const handleScreenshot = () => { | ||
| electronTrpcClient.browser.screenshot.mutate({ paneId }).catch(() => {}); |
Contributor
There was a problem hiding this comment.
P2: Every browser action failure is silently swallowed with .catch(() => {}). These include destructive/global operations like clearing cookies, clearing all browsing data, and taking screenshots. If any of these fail, the user gets no feedback and no recovery path. At minimum, log the error or show a toast on failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/components/BrowserOverflowMenu/BrowserOverflowMenu.tsx, line 33:
<comment>Every browser action failure is silently swallowed with `.catch(() => {})`. These include destructive/global operations like clearing cookies, clearing all browsing data, and taking screenshots. If any of these fail, the user gets no feedback and no recovery path. At minimum, log the error or show a toast on failure.</comment>
<file context>
@@ -0,0 +1,115 @@
+ const { copyToClipboard } = useCopyToClipboard();
+
+ const handleScreenshot = () => {
+ electronTrpcClient.browser.screenshot.mutate({ paneId }).catch(() => {});
+ };
+
</file context>
- Float-over webviews already preserve state across tab switches via the registry's attach/detach pair, so keep-alive tab rendering was redundant. Revert packages/panes Workspace.tsx to rendering only the active tab. - Move all browser-specific wiring from page.tsx into a new useBrowserPaneIntegration hook co-located with BrowserPane. page.tsx now consumes a single hook call instead of owning tab title derivation, tab label rendering, and the destroy diff effect. - Registry attach takes over the visibility flip directly; setVisibility method is no longer needed. Drop the rAF deferral — React useEffect fires after layout commit so getBoundingClientRect reads correct values synchronously. - BrowserPane no longer subscribes to the workspace store for isTabActive.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts:
- Around line 214-236: Replace any pre-mutation + webview.loadURL() usage in
goBack()/goForward() (and similar navigation helpers) with calls to the native
webview.goBack()/goForward() methods; remove the immediate this.setState(paneId,
{ isLoading: true }) and avoid calling webview.loadURL(savedUrl) so you don't
reload or desynchronize on failure. Instead, let the webview emit navigation
events (e.g., handleDidStopLoading, did-navigate, did-navigate-in-page,
did-start-loading) and derive/update UI state (this.setState and
this.pushHistory) from those events (use existing handleDidStopLoading,
entry.onDidStopLoading, pushHistory, setState, and webview.getURL()/getTitle()).
Also add error/fallback handling if goBack()/goForward() report failure so
registry state stays consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66057b41-2d5f-41d2-b311-72f5751e91e8
📒 Files selected for processing (6)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/useBrowserPaneIntegration.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxpackages/panes/src/react/components/Workspace/Workspace.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/index.ts
- packages/panes/src/react/components/Workspace/Workspace.tsx
Contributor
There was a problem hiding this comment.
3 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/useBrowserPaneIntegration.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/useBrowserPaneIntegration.tsx:45">
P2: Avoid an empty `catch` when parsing the URL; handle parse failures explicitly instead of silently swallowing them.</violation>
</file>
<file name="packages/panes/src/react/components/Workspace/Workspace.tsx">
<violation number="1">
P1: Rendering only `activeTab` unmounts inactive tabs on every switch, regressing keep-alive behavior and state persistence across tabs.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx:4">
P1: Active tab visibility is no longer synchronized for browser panes. Since the webviews are floated at the document root and only hidden on detach, removing the tab-activity effect leaves inactive tab webviews visible, so they can paint over the active tab after a tab switch.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -9,6 +9,8 @@ export function Workspace<TData>({ | |||
| registry, | |||
Contributor
There was a problem hiding this comment.
P1: Rendering only activeTab unmounts inactive tabs on every switch, regressing keep-alive behavior and state persistence across tabs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/panes/src/react/components/Workspace/Workspace.tsx, line 69:
<comment>Rendering only `activeTab` unmounts inactive tabs on every switch, regressing keep-alive behavior and state persistence across tabs.</comment>
<file context>
@@ -96,36 +66,19 @@ export function Workspace<TData>({
- </div>
- )}
- </div>
+ {activeTab ? (
+ <Tab
+ store={store}
</file context>
| import type { RendererContext } from "@superset/panes"; | ||
| import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip"; | ||
| import { GlobeIcon } from "lucide-react"; | ||
| import { useCallback, useSyncExternalStore } from "react"; |
Contributor
There was a problem hiding this comment.
P1: Active tab visibility is no longer synchronized for browser panes. Since the webviews are floated at the document root and only hidden on detach, removing the tab-activity effect leaves inactive tab webviews visible, so they can paint over the active tab after a tab switch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx, line 4:
<comment>Active tab visibility is no longer synchronized for browser panes. Since the webviews are floated at the document root and only hidden on detach, removing the tab-activity effect leaves inactive tab webviews visible, so they can paint over the active tab after a tab switch.</comment>
<file context>
@@ -1,10 +1,9 @@
import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip";
import { GlobeIcon } from "lucide-react";
-import { useCallback, useEffect, useSyncExternalStore } from "react";
+import { useCallback, useSyncExternalStore } from "react";
import { TbDeviceDesktop } from "react-icons/tb";
import { electronTrpcClient } from "renderer/lib/trpc-client";
</file context>
- Add PaneDefinition.onRemoved(pane) lifecycle hook to @superset/panes.
Workspace.tsx tracks a flat snapshot of panes across all tabs and fires
onRemoved with the pre-removal Pane object when an id disappears. Pane
moves between tabs keep the same id in the current set so moves correctly
don't fire onRemoved.
- Wire browser cleanup declaratively via the new hook in usePaneRegistry.tsx:
`onRemoved: (pane) => browserRuntimeRegistry.destroy(pane.id)`. Removes
the need for useBrowserPaneDestroy in userland.
- Persist faviconUrl on BrowserPaneData alongside pageTitle. The registry's
persist callback fires from handleDidStopLoading AND handlePageFaviconUpdated
via a shared firePersist helper. Favicons now survive app restart.
- Replace renderTabLabel with renderTabIcon in @superset/panes. TabItem renders
{icon}{title}{accessory} — icon leading, accessory trailing, native gap-2
from the parent button. Drop redundant `| null` from the ReactNode types.
- Fix titleOverride precedence in Workspace.tsx: `tab.titleOverride ??
getTabTitle(tab) ?? tab.id`. Renaming a browser tab now actually sticks.
- Empty tab rename clears titleOverride instead of being swallowed; thread
`string | undefined` through TabItem → TabBar → Workspace so the derived
title takes back over on clear.
- Collapse useBrowserPaneIntegration.tsx and BrowserTabLabel component into
BrowserPane.tsx as pure functions (getBrowserTabTitle, renderBrowserTabIcon).
Favicon is now a plain <img> reading from persisted pane data — no more
subscribing component just for one field.
- page.tsx loses browser-specific imports, refs, effects, and callbacks. Just
two pure helpers passed as props.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts (2)
88-107: Global event listeners are never cleaned up.The
dragstart,dragend,drop, andresizelisteners are added once but never removed. While HMR preservation mitigates this during development, if the registry were ever destroyed and recreated (or in edge cases with multiple windows), these would accumulate.Consider storing references and providing a cleanup path, or document that this singleton is expected to live for the app lifetime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts around lines 88 - 107, The installGlobalListeners method attaches window listeners (dragstart, dragend, drop, resize) but never removes them; update installGlobalListeners to save the listener callbacks (e.g., references to the setPassthrough wrapper and the resize handler) on the instance (use properties like this._globalListenerRefs) and add a public cleanup method (e.g., uninstallGlobalListeners or dispose) that removes each listener via window.removeEventListener and clears this.globalListenersInstalled; ensure uninstallGlobalListeners also undoes any pointer-events changes if needed and call it from any lifecycle teardown where the registry might be destroyed or recreated so entries/updateLayout behavior remains correct without leaking listeners.
223-245: History upsert may fire with stale favicon.In
handleDidStopLoading,upsertis called withentry.state.faviconUrl, but the favicon event (page-favicon-updated) may arrive afterdid-stop-loading. This could persist anullfavicon even when one is available shortly after.The subsequent
handlePageFaviconUpdateddoes call upsert again with the correct favicon, so this is self-correcting but causes an extra mutation. Consider deferring the history upsert slightly or relying solely on the favicon handler for favicon persistence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts around lines 223 - 245, handleDidStopLoading currently calls electronTrpcClient.browserHistory.upsert with entry.state.faviconUrl which can be stale because the page-favicon-updated handler (handlePageFaviconUpdated) may arrive after did-stop-loading; change handleDidStopLoading to only call upsert immediately when entry.state.faviconUrl is non-null (so you avoid persisting null), and if favicon is null, skip the upsert here (still call this.pushHistory/persist URL/title) and let handlePageFaviconUpdated perform the upsert when the favicon becomes available; alternatively, if you prefer a short delay, debounce the upsert in handleDidStopLoading (e.g., schedule a small timeout and re-check entry.state.faviconUrl before calling electronTrpcClient.browserHistory.upsert) so handlePageFaviconUpdated can supply the real favicon and avoid an extra mutation.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx (1)
15-23: Guard against undefined pane access.
tab.panes[paneIds[0]]could be undefined if the panes object is inconsistent with its keys. While unlikely, a defensive check would prevent a runtime error.🛡️ Suggested defensive check
function getSingleBrowserPane( tab: Tab<PaneViewerData>, ): { id: string; data: BrowserPaneData } | null { const paneIds = Object.keys(tab.panes); if (paneIds.length !== 1) return null; const pane = tab.panes[paneIds[0]]; + if (!pane) return null; if (pane.kind !== "browser") return null; return { id: pane.id, data: pane.data as BrowserPaneData }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx around lines 15 - 23, getSingleBrowserPane currently assumes tab.panes[paneIds[0]] exists and accesses its properties directly; add a defensive null/undefined check after computing pane (e.g. if (!pane) return null) and additionally validate expected properties (pane.kind, pane.id, pane.data) before returning to avoid runtime errors when tab.panes is inconsistent. Update the function getSingleBrowserPane to perform these guards and return null on any missing/invalid pane.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsx:
- Around line 15-23: getSingleBrowserPane currently assumes
tab.panes[paneIds[0]] exists and accesses its properties directly; add a
defensive null/undefined check after computing pane (e.g. if (!pane) return
null) and additionally validate expected properties (pane.kind, pane.id,
pane.data) before returning to avoid runtime errors when tab.panes is
inconsistent. Update the function getSingleBrowserPane to perform these guards
and return null on any missing/invalid pane.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts:
- Around line 88-107: The installGlobalListeners method attaches window
listeners (dragstart, dragend, drop, resize) but never removes them; update
installGlobalListeners to save the listener callbacks (e.g., references to the
setPassthrough wrapper and the resize handler) on the instance (use properties
like this._globalListenerRefs) and add a public cleanup method (e.g.,
uninstallGlobalListeners or dispose) that removes each listener via
window.removeEventListener and clears this.globalListenersInstalled; ensure
uninstallGlobalListeners also undoes any pointer-events changes if needed and
call it from any lifecycle teardown where the registry might be destroyed or
recreated so entries/updateLayout behavior remains correct without leaking
listeners.
- Around line 223-245: handleDidStopLoading currently calls
electronTrpcClient.browserHistory.upsert with entry.state.faviconUrl which can
be stale because the page-favicon-updated handler (handlePageFaviconUpdated) may
arrive after did-stop-loading; change handleDidStopLoading to only call upsert
immediately when entry.state.faviconUrl is non-null (so you avoid persisting
null), and if favicon is null, skip the upsert here (still call
this.pushHistory/persist URL/title) and let handlePageFaviconUpdated perform the
upsert when the favicon becomes available; alternatively, if you prefer a short
delay, debounce the upsert in handleDidStopLoading (e.g., schedule a small
timeout and re-check entry.state.faviconUrl before calling
electronTrpcClient.browserHistory.upsert) so handlePageFaviconUpdated can supply
the real favicon and avoid an extra mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d718533d-ab59-4c6d-8ea8-cfa00e1ec3e9
📒 Files selected for processing (11)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/BrowserPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.tspackages/panes/src/react/components/Workspace/Workspace.tsxpackages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsxpackages/panes/src/react/components/Workspace/components/TabBar/components/TabItem/TabItem.tsxpackages/panes/src/react/types.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/index.ts
- packages/panes/src/react/types.ts
- packages/panes/src/react/components/Workspace/components/TabBar/TabBar.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/hooks/usePersistentWebview/usePersistentWebview.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.ts
- Drop the custom history array and isHistoryNavigation flag. goBack/goForward now call webview.goBack()/goForward() directly, preserving scroll position, form state, and BFCache on back/forward navigation. - Add refreshNavState helper that reads webview.canGoBack()/canGoForward() and writes them into runtime state. Called from did-navigate, did-navigate-in-page, did-stop-loading, and on re-attach of existing entries. - The isHistoryNavigation guard was dead code: did-navigate fires before did-stop-loading, so the flag was always false by the time the guard ran, causing a redundant browserHistory.upsert IPC call on every back/forward nav. - Clear faviconUrl in handleDidStartLoading so stale favicons don't persist when navigating to a page with no favicon. - Tighten sanitizeUrl: reject dotted strings containing whitespace (e.g. "release v2.1") as URLs, trim input, tighten the localhost/127.0.0.1 regex to accept optional port and path. Dotted search queries now correctly fall through to Google search.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts (1)
141-145: Consider logging suppressed errors for debugging.The empty catch block silently swallows errors from
canGoBack()/canGoForward(). While this may be intentional if the webview can be in an invalid state, adding minimal logging would aid debugging.♻️ Suggested improvement
try { canGoBack = entry.webview.canGoBack(); canGoForward = entry.webview.canGoForward(); -} catch {} +} catch (err) { + console.debug("[browserRuntimeRegistry] refreshNavState:", err); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts around lines 141 - 145, The empty catch around calling entry.webview.canGoBack()/canGoForward() swallows errors; update the try/catch in the block that computes canGoBack/canGoForward so the catch logs the caught error (e.g., console.warn or the module's logger) along with the paneId and a brief context string; keep the existing behavior of setting state via this.setState(paneId, { canGoBack, canGoForward }) but ensure any exception from entry.webview is surfaced in the log to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts:
- Around line 141-145: The empty catch around calling
entry.webview.canGoBack()/canGoForward() swallows errors; update the try/catch
in the block that computes canGoBack/canGoForward so the catch logs the caught
error (e.g., console.warn or the module's logger) along with the paneId and a
brief context string; keep the existing behavior of setting state via
this.setState(paneId, { canGoBack, canGoForward }) but ensure any exception from
entry.webview is surfaced in the log to aid debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15402c31-b218-4a88-8f5f-03478e62b895
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/sanitizeUrl.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/sanitizeUrl.ts
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts:144">
P2: Avoid swallowing errors in `refreshNavState`; the empty catch hides webview failures and makes navigation-state bugs hard to diagnose. Log the failure (or otherwise surface it) with context.
(Based on your team's feedback about handling async/runtime failures explicitly and avoiding silent catch blocks.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| try { | ||
| canGoBack = entry.webview.canGoBack(); | ||
| canGoForward = entry.webview.canGoForward(); | ||
| } catch {} |
Contributor
There was a problem hiding this comment.
P2: Avoid swallowing errors in refreshNavState; the empty catch hides webview failures and makes navigation-state bugs hard to diagnose. Log the failure (or otherwise surface it) with context.
(Based on your team's feedback about handling async/runtime failures explicitly and avoiding silent catch blocks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/BrowserPane/browserRuntimeRegistry.ts, line 144:
<comment>Avoid swallowing errors in `refreshNavState`; the empty catch hides webview failures and makes navigation-state bugs hard to diagnose. Log the failure (or otherwise surface it) with context.
(Based on your team's feedback about handling async/runtime failures explicitly and avoiding silent catch blocks.) </comment>
<file context>
@@ -138,34 +133,16 @@ class BrowserRuntimeRegistryImpl {
+ try {
+ canGoBack = entry.webview.canGoBack();
+ canGoForward = entry.webview.canGoForward();
+ } catch {}
+ this.setState(paneId, { canGoBack, canGoForward });
}
</file context>
Suggested change
| } catch {} | |
| } catch (err) { | |
| console.warn("[browserRuntimeRegistry] refreshNavState failed:", err); | |
| } |
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 11, 2026
…ence (superset-sh#3346) * feat(desktop): port browser pane to v2 workspaces with global persistence - Port v1 browser pane (webview, URL toolbar, history, error overlay, new-window handler) into v2 workspace tree as a native implementation. Uses the v2 Workspace component's renderToolbar slot for a Chrome-style URL bar in the pane header. - Float-over webview: each <webview> lives at document.body under a fixed #browser-runtime-root, positioned via ResizeObserver on a per-pane placeholder div. Elements never reparent, so Electron's guest WebContents survives tab switches, pane drags, and workspace route remounts — no page reloads. - Keep-alive tab rendering in @superset/panes Workspace.tsx: tabs that have been visited stay mounted behind visibility:hidden so their React state (and persistent DOM like webviews) survives tab switches. Also benefits terminal scrollback, chat subscriptions, editor state. - BrowserRuntimeRegistry: module-level singleton keyed by paneId, modeled on terminalRuntimeRegistry. Owns webview lifecycle, state subscriptions via useSyncExternalStore, drag-passthrough listeners, equality-guarded setState to deduplicate webview event spam. - Single-pane-browser tabs show the page favicon + live page title in the tab bar via a new optional renderTabLabel prop on the panes library. - BrowserPane subscribes to the workspace store for activeTabId to drive webview visibility, so inactive-tab webviews don't paint on top of the active tab's content. - Destroy path: diff the set of browser pane ids across all tabs each render, destroying any that disappeared. Correctly handles tab closes, intra-tab split closes, and pane drags between tabs (moved panes stay in the set). Known limitation: dropping panes onto another browser pane's body doesn't register — the webview compositor layer intercepts drag events even with pointer-events:none/visibility:hidden. Drops on pane headers work. Deferred. * refactor(desktop): co-locate browser pane integration, remove keep-alive - Float-over webviews already preserve state across tab switches via the registry's attach/detach pair, so keep-alive tab rendering was redundant. Revert packages/panes Workspace.tsx to rendering only the active tab. - Move all browser-specific wiring from page.tsx into a new useBrowserPaneIntegration hook co-located with BrowserPane. page.tsx now consumes a single hook call instead of owning tab title derivation, tab label rendering, and the destroy diff effect. - Registry attach takes over the visibility flip directly; setVisibility method is no longer needed. Drop the rAF deferral — React useEffect fires after layout commit so getBoundingClientRect reads correct values synchronously. - BrowserPane no longer subscribes to the workspace store for isTabActive. * refactor(desktop): simplify browser pane integration, add onRemoved hook - Add PaneDefinition.onRemoved(pane) lifecycle hook to @superset/panes. Workspace.tsx tracks a flat snapshot of panes across all tabs and fires onRemoved with the pre-removal Pane object when an id disappears. Pane moves between tabs keep the same id in the current set so moves correctly don't fire onRemoved. - Wire browser cleanup declaratively via the new hook in usePaneRegistry.tsx: `onRemoved: (pane) => browserRuntimeRegistry.destroy(pane.id)`. Removes the need for useBrowserPaneDestroy in userland. - Persist faviconUrl on BrowserPaneData alongside pageTitle. The registry's persist callback fires from handleDidStopLoading AND handlePageFaviconUpdated via a shared firePersist helper. Favicons now survive app restart. - Replace renderTabLabel with renderTabIcon in @superset/panes. TabItem renders {icon}{title}{accessory} — icon leading, accessory trailing, native gap-2 from the parent button. Drop redundant `| null` from the ReactNode types. - Fix titleOverride precedence in Workspace.tsx: `tab.titleOverride ?? getTabTitle(tab) ?? tab.id`. Renaming a browser tab now actually sticks. - Empty tab rename clears titleOverride instead of being swallowed; thread `string | undefined` through TabItem → TabBar → Workspace so the derived title takes back over on clear. - Collapse useBrowserPaneIntegration.tsx and BrowserTabLabel component into BrowserPane.tsx as pure functions (getBrowserTabTitle, renderBrowserTabIcon). Favicon is now a plain <img> reading from persisted pane data — no more subscribing component just for one field. - page.tsx loses browser-specific imports, refs, effects, and callbacks. Just two pure helpers passed as props. * refactor(desktop): use native webview back/forward + tighten sanitizeUrl - Drop the custom history array and isHistoryNavigation flag. goBack/goForward now call webview.goBack()/goForward() directly, preserving scroll position, form state, and BFCache on back/forward navigation. - Add refreshNavState helper that reads webview.canGoBack()/canGoForward() and writes them into runtime state. Called from did-navigate, did-navigate-in-page, did-stop-loading, and on re-attach of existing entries. - The isHistoryNavigation guard was dead code: did-navigate fires before did-stop-loading, so the flag was always false by the time the guard ran, causing a redundant browserHistory.upsert IPC call on every back/forward nav. - Clear faviconUrl in handleDidStartLoading so stale favicons don't persist when navigating to a page with no favicon. - Tighten sanitizeUrl: reject dotted strings containing whitespace (e.g. "release v2.1") as URLs, trim input, tighten the localhost/127.0.0.1 regex to accept optional port and path. Dotted search queries now correctly fall through to Google search.
6 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
@superset/panesidioms (runtime registry +ctx.actions.updateData+renderToolbarslot). v1 is untouched.<webview>lives atdocument.body > #browser-runtime-rootand is positioned via aResizeObserveron a per-pane placeholder div. The element never reparents, so Electron's guestWebContentssurvives tab switches, workspace route remounts, and pane drags — no page reloads, no lost state, ever.@superset/panes: visited tabs stay mounted behindvisibility: hiddenso their React state survives tab switches. Benefits every stateful pane kind (terminal scrollback, chat subscriptions, editor state).renderTabLabelprop.Why / Context
The v2 workspace previously registered a
browserpane kind with a placeholder<iframe>stub. Porting v1's real implementation surfaced a fundamental Electron issue: reparenting a<webview>element viaappendChilddestroys its guestWebContentsand reloads the page. v1 works around this with a "park in hidden container on unmount / reclaim on remount" pattern — but every reparent triggers the reload (v1 even has a// e.g. after DOM reparentingcomment in itsdom-readyhandler re-registering the newwebContentsId).For v2, where tab switches, pane drags, and workspace switches all cause React subtrees to unmount and remount, the only correct answer is don't reparent at all. Float-over is that guarantee.
How It Works
browserRuntimeRegistry (module-level singleton, modeled on
terminalRuntimeRegistry):pointer-events: none#browser-runtime-rootdiv todocument.body.<webview>element (child of root container), per-pane runtime state, and aResizeObserver.attach(paneId, placeholder, initialUrl, onDidStopLoading): creates the webview on first call (appended once, forever), stores the placeholder ref, starts aResizeObserveron it, and rAF-schedules an initialupdateLayout.updateLayout(entry): readsplaceholder.getBoundingClientRect()and writes the result to the webview's fixed-positiontop/left/width/height.detach: stops the observer and hides the webview. Does NOT reparent, does NOT remove.setVisibility(paneId, visible): driven byBrowserPane's subscription toactiveTabId === ctx.tab.id, toggles the webview's CSS visibility so inactive-tab webviews don't paint on top of the current tab.destroy: the only path that removes the webview from the DOM. Called from a single diff effect inpage.tsxthat walks all tabs' browser pane ids each render — anything that disappeared since the previous render gets destroyed. Correctly handles tab closes, intra-tab split closes, and pane drags between tabs (moved panes stay in the current set under their new tab, so they're not destroyed).Keep-alive in
packages/panes:Workspace.tsxtracksmountedTabIdsand renders every visited tab into an absolute-stacked container. The active tab is fully interactive; inactive visited tabs useinvisible pointer-events-none.BrowserPanedoesn't unmount on tab switch, sousePersistentWebview's cleanup never runs, so the webview never even gets a chance to move.Registry subscription pattern: to avoid the
@trpc/react-querysingle-shared-React-context gotcha whereworkspaceTrpc.ProviderhijackselectronTrpcReact hooks inside v2 routes, every Electron-router call goes through the imperativeelectronTrpcClientproxy (matching the v2TerminalPanepattern). All registry mutations/queries/subscriptions use.mutate(...)/.query(...)/.subscribe(...)directly — nouseMutation/useQuery/useSubscriptionforelectronTrpc.browser.*orelectronTrpc.browserHistory.*.Manual QA Checklist
Browser pane basics
Global persistence (the main win)
Tab bar
Split panes
Other pane kinds (benefit from keep-alive)
Destroy paths
#browser-runtime-rootKnown Limitations
<webview>compositor layer intercepts drag events at its painted position regardless ofpointer-events: none,visibility: hidden, or moving the element off-screen. Drops on pane headers still work. Deferred for a follow-up that likely involves patching@superset/panesto portal the drop target overlay above the webview, or moving the pane's drop target to a dedicated element.http://localhost:3000resolves to the renderer machine, not the host. No port-proxy route exists on the host-service or relay yet. Out of scope; browser panes work correctly for local workspaces. A future/proxy/:port/*route + a last-mile URL rewrite inusePersistentWebviewis the planned path.Testing
bun run typecheck(25 packages) ✓bun run lint✓bunx sherif✓bun test— 2389 pass / 3 pre-existing failures unrelated to this work (teardown.test.ts,setup.test.ts— git worktree fixture issues;useOrderedSections.test.tsx— pre-existingelectronTrpcClientinit-order bug in test env; none touch browser pane or panes library files)bun devDesign Decisions
<webview>reparenting is fundamentally broken for state preservation. Float-over guarantees no reparenting and is the only reliable solution.BrowserPaneneeds to stay mounted on tab switch so its subscription toactiveTabIdkeeps the webview visible at the right moments. Keep-alive also benefits every other stateful pane for free.electronTrpcClientimperative calls inside v2.@trpc/react-querystores its context as a module singleton, soworkspaceTrpc.Providerinside v2 routes hijacks everyelectronTrpc.*React hook. Using the imperative proxy sidesteps the issue. Canonical precedent: v2TerminalPanedoes the same.onTabRemovedcallback that received the removed tab's old snapshot — which broke pane drags between tabs, because the moved pane was still in the old snapshot's panes record and got destroyed right as the target tab tried to attach it. The current pane-id diff correctly handles moves.faviconUrlfallback icon. Tabs without a favicon show just the title, no Globe icon.Follow-ups
/proxy/:port/*+ renderer URL rewrite for dev server previews on remote workspaces.devtoolspane kind (currently still a stub).Summary by cubic
Ports the browser pane to v2 workspaces with a float‑over Electron webview and global persistence. Removes keep‑alive tabs; persistence now comes from a registry that keeps each webview alive across tab switches, drags, and workspace remounts.
New Features
#browser-runtime-rootand is positioned viaResizeObserver(no reparenting, no reloads).getTabTitle+renderTabIcon; favicons persist across restarts.about:blank.Migration
@superset/panesWorkspace adds optionalrenderTabIcon,getTabTitle, and a newonRemoved(pane)hook. Workspace renders only the active tab; browser cleanup usesonRemovedto call the runtime registry’sdestroy.BrowserPaneData: removemode; add optionalpageTitleandfaviconUrl; defaulturlisabout:blank.Written for commit 84a8f63. Summary will update on new commits.
Summary by CodeRabbit
New Features
Changes