Implement in-app browser#1519
Conversation
… cleanup - Replace broken setDevToolsWebContents approach (Electron #15874) with CDP remote debugging URL loaded directly in the devtools webview - Discover CDP port dynamically via app.commandLine.getSwitchValue - Match webview targets by type "webview" (not "page") in /json endpoint - Add PaneToolbarActions to DevToolsPane for close/split buttons - Add "devtools" to pane type persistence schema to fix validation error - Cascade removePane: closing a browser pane removes its devtools panes
…wser pane Add Chrome-like URL autocomplete to the browser pane URL bar, backed by persistent SQLite history. Navigating to a page records the URL, title, and favicon in the local database. Clicking the URL bar shows a dropdown of recent history filtered as you type, with keyboard navigation and favicon display. Also wires detected ports to open in the in-app browser pane instead of the system browser. Key changes: - Add `browser_history` table to local-db with indexed URL and timestamp - Add `browserHistory` tRPC router (getAll, search, upsert, clear) - Capture favicons via `page-favicon-updated` webview event - Create `useUrlAutocomplete` hook with client-side filtering - Create `UrlSuggestions` dropdown component - Add "Clear Browsing History" to overflow menu - Widen CSP `img-src` to `https:` for arbitrary favicon domains - Open port links in browser pane via `addBrowserTab` - Strip trailing slash from URL bar display
📝 WalkthroughWalkthroughAdds an in-app browser feature: browser and browserHistory TRPC routers, browser_history DB table, BrowserPane/DevTools UI and hooks, browser pane state in tabs store/types, Electron main process extension loading, and related renderer, extension, and config updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (BrowserPane)
participant TRPC as TRPC Router (browser/history)
participant Main as Electron Main (BrowserManager)
participant DB as Local DB (drizzle)
Renderer->>TRPC: register / navigate / screenshot / getDevToolsUrl
TRPC->>Main: invoke browser action / query webContents
Main->>Main: manage webContents, take screenshot, eval JS
TRPC->>DB: upsert history (upsert)
DB-->>TRPC: ack
Main-->>TRPC: event callbacks (console, new-window)
TRPC-->>Renderer: stream events / responses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In @.superset/lib/setup/steps.sh:
- Line 387: Add DESKTOP_AUTOMATION_PORT to the ports.json generation block so
the port written by write_env_var "DESKTOP_AUTOMATION_PORT" is documented;
locate the ports.json creation code (the block that currently writes
CODE_INSPECTOR_PORT and other ports) and insert an entry for
DESKTOP_AUTOMATION_PORT alongside the existing port entries so it appears in the
generated ports.json output.
In `@apps/desktop/src/main/index.ts`:
- Around line 281-292: The dev extension path for loading the browser extension
is incorrect and the resource isn't being copied into dist; update the dev
branch for extensionPath used in the app.isPackaged check so it points to the
correct source location (e.g., use ../../src/resources/browser-extension if you
want to load from source) OR add "browser-extension" to the RESOURCES_TO_COPY
array in vite/helpers.ts so the extension is copied into dist and the existing
path works; adjust the code around extensionPath and the
session.fromPartition("persist:superset").loadExtension call accordingly and
ensure electron-builder.ts continues to include src/resources/ for packaged
builds.
In `@apps/desktop/src/main/lib/browser/browser-manager.ts`:
- Around line 80-96: The current target lookup uses URL matching which can
return the wrong DevTools endpoint when multiple webviews/pages share the same
URL; replace the URL-based match with the CDP target ID obtained from the
webContents debugger: call wc.debugger.sendCommand('Target.getTargetInfo') to
read targetInfo.targetId and then find the correct entry in the fetched targets
list by comparing t.id === targetId (or use webContents.fromDevToolsTargetId()
if you need reverse lookup), updating the logic around wc, cdpPort, targets and
the find(...) call to use targetId instead of targetUrl.
- Around line 51-55: The evaluateJS method (evaluateJS/paneId/getWebContents)
allows arbitrary JS execution and is exposed via a publicProcedure in the trpc
router without auth; fix by adding an authorization check before executing code:
either attach a tRPC middleware that verifies the requesting user/context owns
the target paneId or add an explicit ownership/permission check inside
evaluateJS (validate paneId, confirm caller identity/session and pane ownership)
and throw if unauthorized; also ensure BrowserWindow creation uses
webPreferences.contextIsolation = true as an extra safety measure.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsx`:
- Around line 127-153: The BrowserPane component (via webviewRefCallback and
useBrowserNavigation) currently lacks handling for webview load failures; add
listeners on the webview element for events such as "did-fail-load", "crashed",
and "unresponsive" (and optionally "did-finish-load" to clear errors), update
component state (e.g., an error/state variable in BrowserPane or exposed by
useBrowserNavigation) when these events fire, and render a concise error UI in
place of the blank overlay that shows the error message and a retry button that
reloads the webview (using the webview ref reload() or loadURL). Ensure you
remove listeners on cleanup to avoid leaks and surface error details
(code/url/message) from the event in the UI.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/components/BrowserOverflowMenu/BrowserOverflowMenu.tsx`:
- Around line 59-61: The clear-all action in BrowserOverflowMenu is destructive
and currently executed immediately by handleClearAllData calling
clearBrowsingDataMutation.mutate({ type: "all" });—add a user confirmation step:
replace the direct mutate call with a confirmation dialog or modal (e.g.,
openConfirmDialog or useConfirm hook) from your UI toolkit inside
BrowserOverflowMenu, present a clear warning and confirm/cancel buttons, and
only call clearBrowsingDataMutation.mutate({ type: "all" }) when the user
confirms; ensure keyboard/escape cancels and consider disabling the menu item
while the mutation is pending to prevent double submissions.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/DevToolsPane/DevToolsPane.tsx`:
- Around line 31-36: The DevTools URL query
(electronTrpc.browser.getDevToolsUrl.useQuery with { browserPaneId: targetPaneId
}) lacks error handling causing the UI to stuck on "Connecting to DevTools...";
update the hook call to capture the error (and loading) from useQuery, derive
devToolsUrl from data?.url, and render three states: success -> render the
<webview>, error -> render a centered error UI (or trigger pane close) showing a
failure message, loading -> keep the connecting fallback. Ensure you reference
the existing targetPaneId/devToolsUrl flow and add the error variable from
useQuery to decide the UI path.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/useBrowserNavigation/useBrowserNavigation.ts`:
- Around line 254-268: goBack and goForward set isHistoryNavigation.current =
true before attempting webviewRef.current?.loadURL(url), which can leave the
flag true if webviewRef is null; change both functions to first check
webviewRef.current exists (or store it to a local const) and only set
isHistoryNavigation.current = true immediately before calling loadURL, and
ensure no-ops (when webviewRef.current is null) do not flip the flag;
references: goBack, goForward, isHistoryNavigation, webviewRef,
navigateBrowserHistory, and the consumers handleDidNavigate /
handleDidStopLoading.
- Around line 274-286: The navigateTo function currently treats inputs without a
dot as searches, which makes localhost:PORT and host:port addresses open a
Google search; update navigateTo (and uses of webviewRef.current?.loadURL) to
first normalize and detect explicit schemes (http:, https:, file:, chrome:,
about:, etc.) and to explicitly handle 'localhost' and host:port patterns (e.g.,
strings matching /^localhost(:\d+)?$/i or /^[^\/:]+:\d+$/) by prepending the
appropriate scheme (default to http:// for localhost/host:port), otherwise keep
your existing dot-based heuristic and search fallback; ensure any encoded search
queries still use encodeURIComponent before constructing the Google search URL.
- Around line 46-52: The callback dependencies use the unstable mutation objects
causing handlers to be recreated; update handleDomReady to depend on
registerMutation.mutate instead of the whole registerMutation object and call
registerMutation.mutate(...) inside it, and apply the same pattern to
handleDidStopLoading and handlePageFaviconUpdated by using
upsertHistoryMutation.mutate and faviconMutation?.mutate (or the corresponding
.mutate stable references) in their implementations and deps so listeners are
stable and not torn down/readded every render.
In `@packages/local-db/drizzle/0026_add_browser_history.sql`:
- Around line 10-11: Remove the redundant non-unique index on url: delete the
CREATE INDEX `browser_history_url_idx` ON `browser_history` (`url`); line from
the migration (leave the unique index `browser_history_url_unique` intact) and
remove the corresponding index declaration named browser_history_url_idx from
the Drizzle schema definition for the browser_history table in schema.ts so only
the unique index browser_history_url_unique remains; update any migration
generation or schema sync steps as needed to keep DB/schema in sync.
🧹 Nitpick comments (13)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/components/BrowserOverflowMenu/BrowserOverflowMenu.tsx (1)
42-49:handleCopyUrlhas no error handling and no user feedback.
navigator.clipboard.writeTextcan reject (e.g., if the document isn't focused or permissions are denied). Additionally, there's no visual indication to the user that the copy succeeded.Suggested improvement
const handleCopyUrl = () => { const webview = document.querySelector( `webview[data-pane-id="${paneId}"]`, ) as Electron.WebviewTag | null; if (webview) { - navigator.clipboard.writeText(webview.getURL()); + navigator.clipboard.writeText(webview.getURL()).catch((err) => { + console.error("[BrowserOverflowMenu] Failed to copy URL:", err); + }); } };apps/desktop/src/shared/tabs-types.ts (1)
11-16: Naming could be confusing:PaneTypehas no"browser"variant, butPanehas abrowser?field.The
Pane.browserstate field is populated for panes of type"webview". This implicit coupling betweentype: "webview"and thebrowserfield may confuse future contributors. Consider either adding a code comment on thebrowserfield clarifying it applies to"webview"panes, or renaming the pane type to"browser"if that better reflects its purpose.Also applies to: 141-142
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/components/UrlSuggestions/UrlSuggestions.tsx (1)
46-62: Favicon fallback via DOM manipulation is fragile.The
onErrorhandler usese.currentTarget.style.display = "none"andnextElementSibling?.classList.remove("hidden")to swap the<img>for the globe icon. This couples rendering to sibling DOM order — any layout change will silently break the fallback.A more React-idiomatic approach would track broken favicons in local state:
Sketch using state-based fallback
+import { useState, useEffect, useRef } from "react"; + export function UrlSuggestions({ suggestions, highlightedIndex, onSelect }: UrlSuggestionsProps) { + const [brokenFavicons, setBrokenFavicons] = useState<Set<string>>(new Set()); const listRef = useRef<HTMLDivElement>(null); ... {item.faviconUrl ? ( - <img - src={item.faviconUrl} - alt="" - className="size-4 shrink-0 rounded-sm" - onError={(e) => { - e.currentTarget.style.display = "none"; - e.currentTarget.nextElementSibling?.classList.remove("hidden"); - }} - /> - ) : null} - {!item.faviconUrl ? ( - <TbGlobe className="size-4 shrink-0 text-muted-foreground/50" /> - ) : ( - <TbGlobe className="hidden size-4 shrink-0 text-muted-foreground/50" /> - )} + item.faviconUrl && !brokenFavicons.has(item.faviconUrl) ? ( + <img + src={item.faviconUrl} + alt="" + className="size-4 shrink-0 rounded-sm" + onError={() => setBrokenFavicons(prev => new Set(prev).add(item.faviconUrl!))} + /> + ) : ( + <TbGlobe className="size-4 shrink-0 text-muted-foreground/50" /> + ) + )}apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/index.tsx (1)
67-83: Consider extracting the inline type to align with the shared pane types.The
devtoolsshape{ targetPaneId: string }is defined inline here. If this structure is already defined inrenderer/stores/tabs/types.ts(which the AI summary suggests), consider importing and reusing it to keep the type in sync.apps/desktop/src/lib/trpc/routers/browser-history/index.ts (1)
18-34: LIKE wildcard characters in user input are not escaped.If
input.querycontains%or_, they act as LIKE wildcards, producing unexpected matches. This is a minor UX issue for a local autocomplete but worth noting.🛡️ Suggested fix — escape LIKE meta-characters
search: publicProcedure .input(z.object({ query: z.string() })) .query(({ input }) => { - const pattern = `%${input.query}%`; + const escaped = input.query.replace(/[%_]/g, "\\$&"); + const pattern = `%${escaped}%`; return localDb .select() .from(browserHistory) .where( or( - like(browserHistory.url, pattern), - like(browserHistory.title, pattern), + sql`${browserHistory.url} LIKE ${pattern} ESCAPE '\\'`, + sql`${browserHistory.title} LIKE ${pattern} ESCAPE '\\'`, ), ) .orderBy(sql`${browserHistory.lastVisitedAt} desc`) .limit(10) .all(); }),packages/local-db/drizzle/meta/0026_snapshot.json (1)
55-69: Remove redundant index onurlcolumn in browser_history table.Line 321 defines
url: text("url").notNull().unique(), which creates a unique constraint and implicit index for lookups. Line 330's explicit non-uniqueindex("browser_history_url_idx").on(table.url)is redundant — SQLite will use the unique index for all url-based queries. This adds unnecessary write I/O overhead on every insert/update without performance benefit.Fix in
packages/local-db/src/schema/schema.tsby removing line 330:index("browser_history_url_idx").on(table.url),apps/desktop/src/renderer/stores/tabs/utils.ts (1)
241-241: Remove duplicateDEFAULT_BROWSER_URLconstant and import from the shared location.This constant is already exported from
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/constants.ts. Instead of defining it locally on line 241, import it from the existing constants file to maintain a single source of truth and prevent accidental divergence.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/BrowserToolbar.tsx (1)
63-68:autocompleteobject reference as dependency defeatsuseCallbackmemoization.
useUrlAutocompletereturns a plain object on each render, soautocompleteis a new reference every time. Using it as a dependency inenterEditMode,exitEditMode,handleSubmit,handleInputChange, andhandleKeyDownmeans none of these callbacks are ever memoized.Destructure the stable individual functions instead:
Destructure hook return for stable deps
- const autocomplete = useUrlAutocomplete({ + const { + isOpen: autocompleteIsOpen, + suggestions: autocompleteSuggestions, + highlightedIndex: autocompleteHighlightedIndex, + open: autocompleteOpen, + close: autocompleteClose, + updateQuery: autocompleteUpdateQuery, + selectSuggestion: autocompleteSelectSuggestion, + handleKeyDown: autocompleteHandleKeyDown, + } = useUrlAutocomplete({ onSelect: (selectedUrl) => { onNavigate(selectedUrl); setIsEditing(false); }, });Then use these individual references in the dependency arrays.
packages/local-db/src/schema/schema.ts (1)
329-332: Redundant index onurl— SQLite's UNIQUE constraint already creates one.The
urlcolumn has.unique()(Line 321), which implicitly creates an index in SQLite. The explicitbrowser_history_url_idxon Line 330 is redundant and wastes storage.Remove the redundant index
(table) => [ - index("browser_history_url_idx").on(table.url), index("browser_history_last_visited_at_idx").on(table.lastVisitedAt), ],apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserToolbar/hooks/useUrlAutocomplete/useUrlAutocomplete.ts (1)
20-20:suggestionsRefis assigned but never read — dead code.
suggestionsRefis created on Line 20 and kept in sync on Line 44, buthandleKeyDown(and all other code) reads from thesuggestionsstate value directly, not fromsuggestionsRef.current. The ref serves no purpose.Remove dead ref
- const suggestionsRef = useRef<HistorySuggestion[]>([]); ... - // Keep ref in sync for keyboard handler - suggestionsRef.current = suggestions;Also applies to: 43-44
apps/desktop/src/lib/trpc/routers/browser/browser.ts (2)
23-30: Consider validating the URL protocol before callingloadURL.
wc.loadURL(input.url)accepts any string from the renderer process. While this is a desktop app, a malicious or buggy extension/component could navigate the webview tofile:///etc/passwdor other sensitive local resources. Consider restricting tohttp:andhttps:protocols.Add protocol validation
navigate: publicProcedure .input(z.object({ paneId: z.string(), url: z.string() })) .mutation(({ input }) => { const wc = browserManager.getWebContents(input.paneId); if (!wc) throw new Error(`No webContents for pane ${input.paneId}`); + const parsed = new URL(input.url); + if (!['http:', 'https:'].includes(parsed.protocol)) { + throw new Error(`Unsupported protocol: ${parsed.protocol}`); + } wc.loadURL(input.url); return { success: true }; }),
148-174: Extract the session partition name to a shared constant.The partition name
"persist:superset"is hardcoded in multiple places across the codebase (BrowserPane.tsx, main/index.ts, main/windows/main.ts, main/lib/sentry.ts, and this file). Define it as a constant inapps/desktop/src/shared/constants.tsand import it here for consistency and maintainability.apps/desktop/src/renderer/stores/tabs/store.ts (1)
1135-1172:addBrowserTabduplicates the history-stack management pattern fromaddTabandaddChatTab.This is the third instance of the same "push current active tab to history, create tab, set active" pattern. Not a blocker since it matches the existing convention in this file, but worth noting as a future DRY candidate.
Consider extracting a shared helper like
addTabToWorkspace(state, workspaceId, tab, pane)that handles the common history-stack and active-tab bookkeeping.
| write_env_var "ELECTRIC_PORT" "$ELECTRIC_PORT" | ||
| write_env_var "CADDY_ELECTRIC_PORT" "$CADDY_ELECTRIC_PORT" | ||
| write_env_var "CODE_INSPECTOR_PORT" "$CODE_INSPECTOR_PORT" | ||
| write_env_var "DESKTOP_AUTOMATION_PORT" "$DESKTOP_AUTOMATION_PORT" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "DESKTOP_AUTOMATION_PORT" --type-all -C 3Repository: superset-sh/superset
Length of output: 502
🏁 Script executed:
# Check the ports.json generation section
fd "steps.sh" -type f | head -5Repository: superset-sh/superset
Length of output: 234
🏁 Script executed:
rg "DESKTOP_AUTOMATION_PORT" -C 3Repository: superset-sh/superset
Length of output: 2639
🏁 Script executed:
fd steps.shRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Find and read the steps.sh file to check lines mentioned
fd "steps.sh" -type f | xargs wc -lRepository: superset-sh/superset
Length of output: 253
🏁 Script executed:
fd "steps.sh" -o fd "ports.json"Repository: superset-sh/superset
Length of output: 187
🏁 Script executed:
# Search more broadly for the file
find . -name "steps.sh" -o -name "ports.json" 2>/dev/null | head -20Repository: superset-sh/superset
Length of output: 128
🏁 Script executed:
# Check if .superset directory exists
ls -la .superset 2>/dev/null || echo ".superset not found"Repository: superset-sh/superset
Length of output: 408
🏁 Script executed:
wc -l ./.superset/lib/setup/steps.shRepository: superset-sh/superset
Length of output: 98
🏁 Script executed:
# Read the steps.sh file to check the relevant sections
cat -n ./.superset/lib/setup/steps.sh | sed -n '375,395p'Repository: superset-sh/superset
Length of output: 1445
🏁 Script executed:
# Check the ports.json generation section
cat -n ./.superset/lib/setup/steps.sh | sed -n '425,450p'Repository: superset-sh/superset
Length of output: 1227
Add DESKTOP_AUTOMATION_PORT to ports.json.
The DESKTOP_AUTOMATION_PORT is written to the .env file at line 387, but it's missing from the ports.json file generated at lines 428-445. All other ports (including CODE_INSPECTOR_PORT) are included in that file. Since this port is actively used for CDP/automation in the application, it should be documented in ports.json for consistency.
Add the following entry:
Suggested diff
{ "port": $CODE_INSPECTOR_PORT, "label": "Code Inspector" }
+ { "port": $DESKTOP_AUTOMATION_PORT, "label": "Desktop Automation" }
]🤖 Prompt for AI Agents
In @.superset/lib/setup/steps.sh at line 387, Add DESKTOP_AUTOMATION_PORT to the
ports.json generation block so the port written by write_env_var
"DESKTOP_AUTOMATION_PORT" is documented; locate the ports.json creation code
(the block that currently writes CODE_INSPECTOR_PORT and other ports) and insert
an entry for DESKTOP_AUTOMATION_PORT alongside the existing port entries so it
appears in the generated ports.json output.
| // Load browser extension for webview panes | ||
| try { | ||
| const extensionPath = app.isPackaged | ||
| ? path.join(process.resourcesPath, "browser-extension") | ||
| : path.join(__dirname, "../src/resources/browser-extension"); | ||
| await session | ||
| .fromPartition("persist:superset") | ||
| .loadExtension(extensionPath); | ||
| console.log("[main] Browser extension loaded"); | ||
| } catch (error) { | ||
| console.error("[main] Failed to load browser extension:", error); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find electron-builder and electron.vite config files
fd -e "json" -e "config" | grep -E "(electron|build)" | head -20Repository: superset-sh/superset
Length of output: 96
🏁 Script executed:
# Also check the actual desktop app structure
find apps/desktop -type f -name "*.json" | head -20Repository: superset-sh/superset
Length of output: 280
🏁 Script executed:
# Look for the browser-extension directory
find . -type d -name "browser-extension" 2>/dev/nullRepository: superset-sh/superset
Length of output: 110
🏁 Script executed:
# Check for electron.vite config in apps/desktop
find apps/desktop -name "electron.vite*" -o -name "vite.config*" | head -10Repository: superset-sh/superset
Length of output: 100
🏁 Script executed:
# Read the package.json to see build configuration and electron-builder setup
cat apps/desktop/package.json | head -100Repository: superset-sh/superset
Length of output: 4148
🏁 Script executed:
# Read the electron.vite.config.ts to understand build output structure
cat apps/desktop/electron.vite.config.tsRepository: superset-sh/superset
Length of output: 7208
🏁 Script executed:
# Read the electron-builder.ts to check for extraResources configuration
cat apps/desktop/electron-builder.tsRepository: superset-sh/superset
Length of output: 5025
🏁 Script executed:
# Find and read the vite/helpers file
find apps/desktop/vite -type f -name "helpers*" -exec cat {} \;Repository: superset-sh/superset
Length of output: 2307
Fix the dev extension path: browser-extension is not copied to the dist/ directory and the relative path is incorrect.
The dev path path.join(__dirname, "../src/resources/browser-extension") resolves to dist/src/resources/browser-extension (one level too shallow), and browser-extension is not listed in RESOURCES_TO_COPY in vite/helpers.ts, so it won't be copied to dist/ at all during build. Either add browser-extension to the copy list (alongside sounds and tray) or change the path to ../../src/resources/browser-extension if relying on the source directory directly. The packaged path using process.resourcesPath works correctly since electron-builder.ts includes all of src/resources/ via the files configuration.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/index.ts` around lines 281 - 292, The dev extension
path for loading the browser extension is incorrect and the resource isn't being
copied into dist; update the dev branch for extensionPath used in the
app.isPackaged check so it points to the correct source location (e.g., use
../../src/resources/browser-extension if you want to load from source) OR add
"browser-extension" to the RESOURCES_TO_COPY array in vite/helpers.ts so the
extension is copied into dist and the existing path works; adjust the code
around extensionPath and the
session.fromPartition("persist:superset").loadExtension call accordingly and
ensure electron-builder.ts continues to include src/resources/ for packaged
builds.
| async evaluateJS(paneId: string, code: string): Promise<unknown> { | ||
| const wc = this.getWebContents(paneId); | ||
| if (!wc) throw new Error(`No webContents for pane ${paneId}`); | ||
| return wc.executeJavaScript(code); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and find the tRPC routers
find apps/desktop -type f -name "*.ts" -path "*/trpc/*" | head -20Repository: superset-sh/superset
Length of output: 1578
🏁 Script executed:
# Check how evaluateJS is exposed in the tRPC router
rg -n "evaluateJS" --type=ts -C5 -g '*/trpc/routers/**'Repository: superset-sh/superset
Length of output: 212
🏁 Script executed:
# Also search more broadly for evaluateJS references
rg -n "evaluateJS" --type=ts apps/desktop/srcRepository: superset-sh/superset
Length of output: 381
🏁 Script executed:
# Read the browser router implementation around the evaluateJS procedure
cat -n apps/desktop/src/lib/trpc/routers/browser/browser.ts | sed -n '60,85p'Repository: superset-sh/superset
Length of output: 953
🏁 Script executed:
# Check the full browser router file to understand the context better
wc -l apps/desktop/src/lib/trpc/routers/browser/browser.tsRepository: superset-sh/superset
Length of output: 120
🏁 Script executed:
# Get broader context around the evaluateJS procedure
cat -n apps/desktop/src/lib/trpc/routers/browser/browser.ts | head -80Repository: superset-sh/superset
Length of output: 3055
🏁 Script executed:
# Check the tRPC router definition and understand publicProcedure
cat -n apps/desktop/src/lib/trpc/index.tsRepository: superset-sh/superset
Length of output: 1642
🏁 Script executed:
# Check if publicProcedure is defined with any context/middleware
rg -n "publicProcedure" apps/desktop/src/lib/trpc -B5 -A5 | head -60Repository: superset-sh/superset
Length of output: 4576
🏁 Script executed:
# Search for how tRPC is exposed and whether webview can call it
rg -n "createTRPCClient\|useClient\|trpc\." apps/desktop/src/renderer -l | head -10Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for where tRPC is set up as a client in the renderer
rg -n "createTRPCReact\|useContext\|trpc\." apps/desktop/src/renderer -A3 | head -40Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check how the tRPC router is exposed/registered with Electron IPC
rg -n "ipcHandle\|ipcMain\|ipcRenderer\|handle\(" apps/desktop/src/main -l | head -10Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for electron IPC setup or preload bridge
find apps/desktop/src -name "preload*" -o -name "*ipc*" -o -name "*bridge*"Repository: superset-sh/superset
Length of output: 146
🏁 Script executed:
# Check main.ts or app initialization for tRPC server setup
rg -n "createTRPCProxyClient\|nodeHTTPBatchStreamLink\|createContext" apps/desktop/src/main -B3 -A3 | head -60Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check preload directory contents
ls -la apps/desktop/src/preload/Repository: superset-sh/superset
Length of output: 232
🏁 Script executed:
# Look at preload files
cat -n apps/desktop/src/preload/*.ts 2>/dev/null | head -100Repository: superset-sh/superset
Length of output: 2951
🏁 Script executed:
# Search for trpc-electron or similar in the codebase
rg -n "trpc-electron\|createTRPCClient\|ipcLink" apps/desktop/src --type=ts | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the main app entry point
find apps/desktop/src/main -name "*.ts" -maxdepth 1 -type fRepository: superset-sh/superset
Length of output: 128
🏁 Script executed:
# Check main/index.ts for tRPC setup
cat -n apps/desktop/src/main/index.ts | head -150Repository: superset-sh/superset
Length of output: 5198
🏁 Script executed:
# Search for context isolation and sandbox settings in Electron config
rg -n "contextIsolation\|sandbox\|nodeIntegration" apps/desktop/src/main --type=ts -B2 -A2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's any authentication context passed to tRPC
rg -n "createContext\|context:" apps/desktop/src/lib/trpc -B3 -A5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for how browser manager relates to security and pane context
cat -n apps/desktop/src/main/lib/browser/browser-manager.ts | head -80Repository: superset-sh/superset
Length of output: 3083
🏁 Script executed:
# Check if there's any paneId validation or authorization in browser router
cat -n apps/desktop/src/lib/trpc/routers/browser/browser.ts | sed -n '1,100p'Repository: superset-sh/superset
Length of output: 3704
🏁 Script executed:
# Search for how panes are managed and if there's ownership/scope checking
rg -n "paneId\|pane.*owner\|pane.*auth\|register.*pane" apps/desktop/src/main -B2 -A2 | head -60Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's context passed to tRPC procedures in main setup
rg -n "createProxySSGHelpers\|createInnerTRPCContext\|router\(" apps/desktop/src/main/lib -A5 | head -40Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for BrowserWindow creation and context isolation settings
rg -n "new BrowserWindow\|contextIsolation\|preload" apps/desktop/src/main -B3 -A5 | head -80Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the MainWindow class to see security settings
find apps/desktop/src/main -name "*window*" -o -name "*main*" | grep -i windowRepository: superset-sh/superset
Length of output: 285
🏁 Script executed:
# Look for how webContents/panes are created and if there's scope checking
rg -n "webContents\|createWindow\|new.*Window" apps/desktop/src/main/lib -lRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the main window implementation
cat -n apps/desktop/src/main/windows/main.tsRepository: superset-sh/superset
Length of output: 10573
🏁 Script executed:
# Search for context isolation and webPreferences
rg -n "webPreferences\|contextIsolation\|sandbox" apps/desktop/src/main/windows -B2 -A3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's any window.open/webview handling that might create untrusted contexts
cat -n apps/desktop/src/main/lib/browser/browser-manager.ts | sed -n '60,130p'Repository: superset-sh/superset
Length of output: 2481
🏁 Script executed:
# Check how webContents are registered - is there origin/URL validation?
rg -n "register\|webContentsId" apps/desktop/src/lib/trpc/routers/browser -B3 -A3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for any authorization/access control in tRPC context or middleware
rg -n "createInnerTRPCContext\|ctx\.\|context:" apps/desktop/src/lib/trpc/routers --type=ts -B2 -A4 | head -40Repository: superset-sh/superset
Length of output: 46
Arbitrary JS execution via evaluateJS — tRPC route lacks authorization checks.
The evaluateJS procedure is exposed via publicProcedure (line 68 of apps/desktop/src/lib/trpc/routers/browser/browser.ts), which applies only Sentry error middleware with no authentication or authorization checks. Any code with access to the tRPC client can invoke evaluateJS on any registered paneId, regardless of ownership or scope. The paneId parameter receives no validation beyond being a string, allowing arbitrary enumeration or hijacking of browser panes.
While this may be acceptable if all renderer content is trusted, consider:
- Adding an authorization middleware to verify pane ownership before executing JS
- Implementing per-pane access control tied to the requesting context
- Explicitly enabling context isolation in BrowserWindow webPreferences if not already enforced
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/browser/browser-manager.ts` around lines 51 - 55,
The evaluateJS method (evaluateJS/paneId/getWebContents) allows arbitrary JS
execution and is exposed via a publicProcedure in the trpc router without auth;
fix by adding an authorization check before executing code: either attach a tRPC
middleware that verifies the requesting user/context owns the target paneId or
add an explicit ownership/permission check inside evaluateJS (validate paneId,
confirm caller identity/session and pane ownership) and throw if unauthorized;
also ensure BrowserWindow creation uses webPreferences.contextIsolation = true
as an extra safety measure.
| const targetUrl = wc.getURL(); | ||
|
|
||
| try { | ||
| const res = await fetch(`http://127.0.0.1:${cdpPort}/json`); | ||
| const targets = (await res.json()) as Array<{ | ||
| id: string; | ||
| url: string; | ||
| type: string; | ||
| webSocketDebuggerUrl?: string; | ||
| }>; | ||
|
|
||
| // Webview guests have type "webview", not "page" | ||
| const target = targets.find( | ||
| (t) => | ||
| (t.type === "webview" || t.type === "page") && t.url === targetUrl, | ||
| ); | ||
| if (!target) return null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/main/lib/browser/browser-manager.tsRepository: superset-sh/superset
Length of output: 5496
🏁 Script executed:
# Check webContents registration and tracking patterns in the file
rg -n "register\|webContentsId\|getProcessId\|debugger" apps/desktop/src/main/lib/browser/browser-manager.ts -B2 -A2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for any existing CDP target matching patterns in the codebase
rg -n "devToolsUrl\|cdpPort\|\/json" apps/desktop/src --type=ts -B2 -A5Repository: superset-sh/superset
Length of output: 180
🏁 Script executed:
# Check how webContents IDs are used in the codebase
rg -n "wc\.id\|webContents\.id\|processId" apps/desktop/src --type=ts | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for other uses of CDP or debugger APIs in the desktop app
rg -n "remote-debugging-port|cdpPort|devToolsUrl|/json" apps/desktop/src --type=ts -B3 -A3Repository: superset-sh/superset
Length of output: 6976
🏁 Script executed:
# Check if there are patterns for handling multiple webContents with same URL
rg -n "browserPaneId|paneId.*same.*url|multiple.*pane" apps/desktop/src --type=ts -i -B2 -A2Repository: superset-sh/superset
Length of output: 8880
🏁 Script executed:
# Look for how panes are created and what identifiers are available
rg -n "new.*BrowserPane|createPane|register.*pane" apps/desktop/src --type=ts -B2 -A5Repository: superset-sh/superset
Length of output: 34079
🏁 Script executed:
# Check webContents property usage to see if id or processId is available
rg -n "webContents\.(id|processId|getProcessId|debugger)" apps/desktop/src --type=ts -B2 -A2Repository: superset-sh/superset
Length of output: 46
🌐 Web query:
Electron webContents to CDP target matching identifier debugger
💡 Result:
To match an Electron webContents with a Chrome DevTools Protocol (CDP) TargetID (the identifier you see in CDP / “targets”), use Electron’s built-in mapping:
- CDP TargetID →
WebContents:webContents.fromDevToolsTargetId(targetId)(electronjs.org) WebContents→ CDP TargetID: attach the debugger and callTarget.getTargetInfo, then readtargetInfo.targetId(electronjs.org)
Example (main process)
const { BrowserWindow, webContents } = require('electron');
const win = new BrowserWindow();
(async () => {
const wc = win.webContents;
wc.debugger.attach('1.3');
const { targetInfo } = await wc.debugger.sendCommand('Target.getTargetInfo');
const targetId = targetInfo.targetId; // CDP TargetID
const sameWc = webContents.fromDevToolsTargetId(targetId); // -> WebContents
console.log({ wcId: wc.id, targetId, same: sameWc?.id === wc.id });
wc.debugger.detach();
})();Target.getTargetInfo / targetInfo.targetId are defined by the CDP Target domain. (chromedevtools.github.io)
Citations:
- 1: https://www.electronjs.org/docs/latest/api/web-contents?utm_source=openai
- 2: https://www.electronjs.org/docs/latest/api/debugger?utm_source=openai
- 3: https://chromedevtools.github.io/devtools-protocol/tot/Target/?utm_source=openai
🌐 Web query:
Chromium DevTools Protocol target id webview page type distinguish multiple
💡 Result:
In CDP, you distinguish “page” vs “webview” (and multiple instances of either) via Target.TargetInfo returned by Target.getTargets (or via Target.targetCreated/targetInfoChanged events after Target.setDiscoverTargets(true)):
-
Use
targetInfo.typeto tell kind of targetTargetInfo.typeis the canonical field; Chromium emits types including"page"and"webview"(among many others). (chromedevtools.github.io)
-
Use
targetInfo.targetIdas the unique identifier- Every discovered target has a unique
targetId; that’s what you attach to (viaTarget.attachToTarget/ auto-attach) and what you use to route messages. (chromedevtools.github.io)
- Every discovered target has a unique
-
To distinguish multiple webviews/pages, don’t rely on
typealone—also key on:targetInfo.urlandtargetInfo.title(often the simplest discriminator once navigated). (chromedevtools.github.io)targetInfo.openerIdto associate a popup/child target back to the opener target (often helps group which page spawned which webview/popup). (chromedevtools.github.io)targetInfo.browserContextIdif you’re using multiple browser contexts/profiles. (chromedevtools.github.io)- If URLs are identical/
about:blank, listen fortargetInfoChangedand re-readurl/titleafter navigation (common with newly created webviews). (chromedevtools.github.io)
-
If you’re using Puppeteer, the same concepts map directly
- Puppeteer exposes target types including
WEBVIEWandPAGE, andTarget.page()works for"page","webview","background_page"targets. (pptr.dev)
- Puppeteer exposes target types including
If you share a sample Target.getTargets output (a few TargetInfo objects), I can tell you exactly which fields will reliably separate your multiple webviews (some environments reuse titles/URLs, so the best key can vary).
Citations:
- 1: https://chromedevtools.github.io/devtools-protocol/tot/Target/?utm_source=openai
- 2: https://chromedevtools.github.io/devtools-protocol/tot/Target/?utm_source=openai
- 3: https://chromedevtools.github.io/devtools-protocol/tot/Target/?utm_source=openai
- 4: https://chromedevtools.github.io/devtools-protocol/tot/Target/?utm_source=openai
- 5: https://chromedevtools.github.io/devtools-protocol/tot/Target/?utm_source=openai
- 6: https://chromedevtools.github.io/devtools-protocol/tot/Target/?utm_source=openai
- 7: https://pptr.dev/api/puppeteer.targettype?utm_source=openai
Use CDP targetId instead of URL matching to reliably identify DevTools targets.
When multiple browser panes navigate to the same URL, targets.find(t => … && t.url === targetUrl) will match the first one, potentially returning the wrong DevTools endpoint. Use Electron's debugger API to get the CDP target ID instead:
const { targetInfo } = await wc.debugger.sendCommand('Target.getTargetInfo');
const targetId = targetInfo.targetId;
// Then find: targets.find(t => t.id === targetId)Alternatively, use webContents.fromDevToolsTargetId() for the reverse lookup if needed.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/browser/browser-manager.ts` around lines 80 - 96,
The current target lookup uses URL matching which can return the wrong DevTools
endpoint when multiple webviews/pages share the same URL; replace the URL-based
match with the CDP target ID obtained from the webContents debugger: call
wc.debugger.sendCommand('Target.getTargetInfo') to read targetInfo.targetId and
then find the correct entry in the fetched targets list by comparing t.id ===
targetId (or use webContents.fromDevToolsTargetId() if you need reverse lookup),
updating the logic around wc, cdpPort, targets and the find(...) call to use
targetId instead of targetUrl.
| // Query the CDP debug server for the DevTools frontend URL | ||
| const { data } = electronTrpc.browser.getDevToolsUrl.useQuery( | ||
| { browserPaneId: targetPaneId }, | ||
| { refetchOnWindowFocus: false }, | ||
| ); | ||
| const devToolsUrl = data?.url; |
There was a problem hiding this comment.
No error handling if the DevTools URL query fails.
If the target browser pane is closed or the CDP server isn't reachable, useQuery will error out but the component will perpetually show "Connecting to DevTools..." with no feedback. Consider handling the error state (e.g., showing an error message or auto-closing the pane).
- const { data } = electronTrpc.browser.getDevToolsUrl.useQuery(
+ const { data, error } = electronTrpc.browser.getDevToolsUrl.useQuery(
{ browserPaneId: targetPaneId },
{ refetchOnWindowFocus: false },
);
const devToolsUrl = data?.url;Then render an error state alongside the loading fallback:
{devToolsUrl ? (
<webview ... />
) : error ? (
<div className="flex h-full w-full items-center justify-center text-destructive text-xs">
Failed to connect to DevTools
</div>
) : (
<div className="...">Connecting to DevTools...</div>
)}🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/DevToolsPane/DevToolsPane.tsx`
around lines 31 - 36, The DevTools URL query
(electronTrpc.browser.getDevToolsUrl.useQuery with { browserPaneId: targetPaneId
}) lacks error handling causing the UI to stuck on "Connecting to DevTools...";
update the hook call to capture the error (and loading) from useQuery, derive
devToolsUrl from data?.url, and render three states: success -> render the
<webview>, error -> render a centered error UI (or trigger pane close) showing a
failure message, loading -> keep the connecting fallback. Ensure you reference
the existing targetPaneId/devToolsUrl flow and add the error variable from
useQuery to decide the UI path.
| const navigateTo = useCallback((url: string) => { | ||
| let finalUrl = url; | ||
| if (!/^https?:\/\//i.test(url) && !url.startsWith("about:")) { | ||
| // If it looks like a domain (has a dot), add https | ||
| if (url.includes(".")) { | ||
| finalUrl = `https://${url}`; | ||
| } else { | ||
| // Otherwise treat as search | ||
| finalUrl = `https://www.google.com/search?q=${encodeURIComponent(url)}`; | ||
| } | ||
| } | ||
| webviewRef.current?.loadURL(finalUrl); | ||
| }, []); |
There was a problem hiding this comment.
localhost:PORT URLs are broken — they'll trigger a Google search instead of navigation.
navigateTo checks for a dot to distinguish domain-like input from search queries. localhost:3000 has no dot, so it falls through to the Google search branch. For a developer-facing browser pane this is a critical gap — local dev servers are the primary use case.
Consider also handling other common schemes (file:, chrome:, etc.) and localhost explicitly.
🐛 Proposed fix
const navigateTo = useCallback((url: string) => {
let finalUrl = url;
if (!/^https?:\/\//i.test(url) && !url.startsWith("about:")) {
- // If it looks like a domain (has a dot), add https
- if (url.includes(".")) {
+ // If it looks like a domain (has a dot) or is localhost, add https
+ if (url.includes(".") || /^localhost(:\d+)?(\/|$)/i.test(url)) {
finalUrl = `https://${url}`;
} else {
// Otherwise treat as search
finalUrl = `https://www.google.com/search?q=${encodeURIComponent(url)}`;
}
}
webviewRef.current?.loadURL(finalUrl);
}, []);Also note: detected-port links from the sidebar are likely http:// (not https://). You may want to default to http:// for localhost targets specifically.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const navigateTo = useCallback((url: string) => { | |
| let finalUrl = url; | |
| if (!/^https?:\/\//i.test(url) && !url.startsWith("about:")) { | |
| // If it looks like a domain (has a dot), add https | |
| if (url.includes(".")) { | |
| finalUrl = `https://${url}`; | |
| } else { | |
| // Otherwise treat as search | |
| finalUrl = `https://www.google.com/search?q=${encodeURIComponent(url)}`; | |
| } | |
| } | |
| webviewRef.current?.loadURL(finalUrl); | |
| }, []); | |
| const navigateTo = useCallback((url: string) => { | |
| let finalUrl = url; | |
| if (!/^https?:\/\//i.test(url) && !url.startsWith("about:")) { | |
| // If it looks like a domain (has a dot) or is localhost, add https | |
| if (url.includes(".") || /^localhost(:\d+)?(\/|$)/i.test(url)) { | |
| finalUrl = `https://${url}`; | |
| } else { | |
| // Otherwise treat as search | |
| finalUrl = `https://www.google.com/search?q=${encodeURIComponent(url)}`; | |
| } | |
| } | |
| webviewRef.current?.loadURL(finalUrl); | |
| }, []); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/useBrowserNavigation/useBrowserNavigation.ts`
around lines 274 - 286, The navigateTo function currently treats inputs without
a dot as searches, which makes localhost:PORT and host:port addresses open a
Google search; update navigateTo (and uses of webviewRef.current?.loadURL) to
first normalize and detect explicit schemes (http:, https:, file:, chrome:,
about:, etc.) and to explicitly handle 'localhost' and host:port patterns (e.g.,
strings matching /^localhost(:\d+)?$/i or /^[^\/:]+:\d+$/) by prepending the
appropriate scheme (default to http:// for localhost/host:port), otherwise keep
your existing dot-based heuristic and search fallback; ensure any encoded search
queries still use encodeURIComponent before constructing the Google search URL.
| CREATE UNIQUE INDEX `browser_history_url_unique` ON `browser_history` (`url`);--> statement-breakpoint | ||
| CREATE INDEX `browser_history_url_idx` ON `browser_history` (`url`);--> statement-breakpoint |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the SQL migration file content
cat -n packages/local-db/drizzle/0026_add_browser_history.sqlRepository: superset-sh/superset
Length of output: 687
🏁 Script executed:
# Search for the browser_history schema definition in the Drizzle schema file
find . -name "schema.ts" -path "*packages/local-db*" | head -5Repository: superset-sh/superset
Length of output: 104
🏁 Script executed:
# Look for browser_history table definition in schema files
rg -n "browser_history" --type=ts -C 10 -g '*schema*'Repository: superset-sh/superset
Length of output: 2284
Redundant index: Remove browser_history_url_idx as it duplicates the unique index on the same column.
The browser_history_url_unique unique index on url already provides B-tree index lookups. The additional browser_history_url_idx regular index on url duplicates this functionality and wastes storage and write overhead. Remove it from both the migration file and the Drizzle schema definition.
Required changes
Migration file (packages/local-db/drizzle/0026_add_browser_history.sql):
CREATE UNIQUE INDEX `browser_history_url_unique` ON `browser_history` (`url`);--> statement-breakpoint
-CREATE INDEX `browser_history_url_idx` ON `browser_history` (`url`);--> statement-breakpoint
CREATE INDEX `browser_history_last_visited_at_idx` ON `browser_history` (`last_visited_at`);Schema definition (packages/local-db/src/schema/schema.ts lines 329-332):
},
(table) => [
- index("browser_history_url_idx").on(table.url),
index("browser_history_last_visited_at_idx").on(table.lastVisitedAt),
],📝 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.
| CREATE UNIQUE INDEX `browser_history_url_unique` ON `browser_history` (`url`);--> statement-breakpoint | |
| CREATE INDEX `browser_history_url_idx` ON `browser_history` (`url`);--> statement-breakpoint | |
| CREATE UNIQUE INDEX `browser_history_url_unique` ON `browser_history` (`url`);--> statement-breakpoint | |
| CREATE INDEX `browser_history_last_visited_at_idx` ON `browser_history` (`last_visited_at`); |
🤖 Prompt for AI Agents
In `@packages/local-db/drizzle/0026_add_browser_history.sql` around lines 10 - 11,
Remove the redundant non-unique index on url: delete the CREATE INDEX
`browser_history_url_idx` ON `browser_history` (`url`); line from the migration
(leave the unique index `browser_history_url_unique` intact) and remove the
corresponding index declaration named browser_history_url_idx from the Drizzle
schema definition for the browser_history table in schema.ts so only the unique
index browser_history_url_unique remains; update any migration generation or
schema sync steps as needed to keep DB/schema in sync.
…location DevToolsPane is a sibling pane type dispatched by TabView's renderPane(), not a child of BrowserPane. Move it from BrowserPane/components/ to TabView/ to match the co-location rules.
saddlepaddle
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/DevToolsPane/DevToolsPane.tsx (1)
56-56: All pane types intentionally share theCLOSE_TERMINALhotkey — but the naming is misleadingThe hotkey is reused across DevToolsPane, ChatPane, BrowserPane, and the actual TabPane (Terminal), with the workspace handler closing whichever pane is focused. This is intentional, but the hotkey's label ("Close Terminal") and category don't reflect that it's a generic pane-closing mechanism. Since
PaneToolbarActionsalready defaults to"CLOSE_PANE", consider either:
- Renaming
CLOSE_TERMINALtoCLOSE_PANEfor clarity, or- Adding a brief comment explaining the shared hotkey design
…history flag - Handle localhost/127.0.0.1 URLs with http:// instead of Google search - Destructure stable .mutate refs from mutations to avoid listener churn - Only set isHistoryNavigation flag when webview ref exists
Shows a friendly error page with human-readable label, error description, show/hide details toggle, and retry button when webview navigation fails (connection refused, DNS errors, timeouts, etc.).
Left-align content, add friendly message, dot separator before Show/Hide Details toggle, bordered details box with copy button, and "Restart Browser" label.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
778-809:⚠️ Potential issue | 🟠 Major
adjacentPaneIdmay point to a devtools pane that is also being removed, leavingfocusedPaneIdsstale.When removing a browser pane that has an associated devtools pane,
getAdjacentPaneIdis computed from the original layout using only the primarypaneId. If the devtools pane happens to be the adjacent pane in visual order,adjacentPaneIdwill reference a pane that is also inpaneIdsToRemove. The??fallback on Line 807 won't trigger becauseadjacentPaneIdis a non-null string.Example: layout
[browser, devtools, terminal]→ closebrowser→paneIdsToRemove = [browser, devtools]→adjacentPaneId = devtools→ focused pane set to the deleteddevtoolsID.🐛 Proposed fix
// Must get adjacent pane BEFORE removing from layout const adjacentPaneId = getAdjacentPaneId(tab.layout, paneId); +// If adjacent is also being removed, fall through to getFirstPaneId +const safeAdjacentPaneId = adjacentPaneId && !paneIdsToRemove.includes(adjacentPaneId) + ? adjacentPaneId + : null; // ... let newFocusedPaneIds = state.focusedPaneIds; if (paneIdsToRemove.includes(state.focusedPaneIds[tab.id])) { newFocusedPaneIds = { ...state.focusedPaneIds, - [tab.id]: adjacentPaneId ?? getFirstPaneId(newLayout), + [tab.id]: safeAdjacentPaneId ?? getFirstPaneId(newLayout), }; }
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsx`:
- Line 104: The BrowserPane component currently passes
closeHotkeyId="CLOSE_TERMINAL" which causes HotkeyTooltipContent to show "Close
Terminal"; change the prop to use the generic CLOSE_PANE identifier instead
(i.e., set closeHotkeyId="CLOSE_PANE") so PaneToolbarActions and tooltips
display the correct "Close Pane" label; update the JSX in BrowserPane where
closeHotkeyId is set and ensure no other references rely on CLOSE_TERMINAL for
this pane type.
🧹 Nitpick comments (5)
apps/desktop/src/shared/tabs-types.ts (1)
181-188: Nit:errorandviewportuse both?and| null.
error?: BrowserLoadError | nullmeans the field can beundefined,null, or aBrowserLoadError. If this is intentional (e.g.,undefined= "not yet set",null= "explicitly cleared"), this is fine. Otherwise, consider using just?(optional, defaulting toundefined) or just| null(always present) to reduce ambiguity for consumers.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/BrowserErrorOverlay/BrowserErrorOverlay.tsx (1)
60-62:navigator.clipboard.writeTextcan reject — no feedback on success or failure.The clipboard API can throw (e.g., document not focused, permission denied in some contexts). Consider adding a
.catch()and optionally brief user feedback (e.g., a transient "Copied!" state).♻️ Suggested improvement
+const [copied, setCopied] = useState(false); + const copyDetails = useCallback(() => { - navigator.clipboard.writeText(detailsText); + navigator.clipboard.writeText(detailsText).then(() => { + setCopied(true); + setTimeout(() => setCopied(false), 1500); + }).catch(() => { + // Silently fail — clipboard may not be available + }); }, [detailsText]);apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/useBrowserNavigation/useBrowserNavigation.ts (2)
300-312:localhostdetection is loose —startsWithcan match unintended strings.
url.startsWith("localhost")would match inputs like"localhostevil.com". A word-boundary check would be more precise. The past review comment about this area is still relevant.♻️ Proposed fix
const navigateTo = useCallback((url: string) => { let finalUrl = url; if (!/^https?:\/\//i.test(url) && !url.startsWith("about:")) { - if (url.startsWith("localhost") || url.startsWith("127.0.0.1")) { + if (/^(localhost|127\.0\.0\.1)(:\d+)?(\/|$)/i.test(url)) { finalUrl = `http://${url}`; } else if (url.includes(".")) { finalUrl = `https://${url}`;
57-76: History navigation triggers URL update fromhandleDidStopLoadingafterhandleDidNavigateresets the flag.During history navigations,
handleDidNavigatefires first and resetsisHistoryNavigationtofalse. ThenhandleDidStopLoadingfires, sees the flag isfalse, and proceeds to callupdateBrowserUrlandupsertHistoryagain. This isn't a correctness bug sinceupdateBrowserUrldetects same-URL updates andupsertHistoryis idempotent, but it results in unnecessary work (extra store update + tRPC mutation).Consider resetting the flag only in
handleDidStopLoading(the last event in the sequence), or using a counter instead of a boolean.Also applies to: 104-125
apps/desktop/src/renderer/stores/tabs/store.ts (1)
1332-1380:openDevToolsPaneshould check for an existing devtools pane targeting the same browser pane.Clicking the DevTools button multiple times creates duplicate devtools panes for the same browser pane. Add a guard to check if a devtools pane already exists for
browserPaneIdand focus it instead.♻️ Suggested guard
openDevToolsPane: (tabId, browserPaneId, path) => { const state = get(); const tab = state.tabs.find((t) => t.id === tabId); if (!tab) return null; const sourcePane = state.panes[browserPaneId]; if (!sourcePane || sourcePane.tabId !== tabId) return null; + // Check if devtools pane already exists for this browser pane + const existingDevTools = Object.values(state.panes).find( + (p) => p.type === "devtools" && p.devtools?.targetPaneId === browserPaneId, + ); + if (existingDevTools) { + set({ + focusedPaneIds: { + ...state.focusedPaneIds, + [tabId]: existingDevTools.id, + }, + }); + return existingDevTools.id; + } + const newPane = createDevToolsPane(tabId, browserPaneId);
| splitOrientation={handlers.splitOrientation} | ||
| onSplitPane={handlers.onSplitPane} | ||
| onClosePane={handlers.onClosePane} | ||
| closeHotkeyId="CLOSE_TERMINAL" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for CLOSE_TERMINAL definition and usage
rg -n "CLOSE_TERMINAL" --type ts --type tsx -B2 -A2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for other closeHotkeyId usages to see what values are used
rg -n "closeHotkeyId=" --type ts --type tsx -B2 -A2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for hotkey ID definitions/constants
rg -n "CLOSE_" --type ts --type tsx | grep -i "const\|export\|enum" | head -20Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for CLOSE_TERMINAL definition and usage (without file type restriction)
rg -n "CLOSE_TERMINAL" -C2Repository: superset-sh/superset
Length of output: 3945
🏁 Script executed:
#!/bin/bash
# Search for closeHotkeyId usages
rg -n "closeHotkeyId=" -C2Repository: superset-sh/superset
Length of output: 3093
🏁 Script executed:
#!/bin/bash
# Find all hotkey ID constants/enums
rg -n "CLOSE_|TERMINAL" --type ts | head -30Repository: superset-sh/superset
Length of output: 2847
🏁 Script executed:
#!/bin/bash
# Get the full definition of CLOSE_PANE and CLOSE_TERMINAL hotkeys
rg -n "CLOSE_PANE|CLOSE_TERMINAL" -A3 apps/desktop/src/shared/hotkeys.tsRepository: superset-sh/superset
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Check how closeHotkeyId is used - is it displayed to users?
rg -n "closeHotkeyId" -B5 -A5 | head -100Repository: superset-sh/superset
Length of output: 11976
Use CLOSE_PANE instead of CLOSE_TERMINAL for the closeHotkeyId prop.
The closeHotkeyId value is displayed to users in tooltips via HotkeyTooltipContent. CLOSE_TERMINAL produces a "Close Terminal" label in the UI, which is semantically incorrect for a browser pane. The PaneToolbarActions component defaults to CLOSE_PANE (with label "Close Pane"), which is the appropriate hotkey for all pane types.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/BrowserPane.tsx`
at line 104, The BrowserPane component currently passes
closeHotkeyId="CLOSE_TERMINAL" which causes HotkeyTooltipContent to show "Close
Terminal"; change the prop to use the generic CLOSE_PANE identifier instead
(i.e., set closeHotkeyId="CLOSE_PANE") so PaneToolbarActions and tooltips
display the correct "Close Pane" label; update the JSX in BrowserPane where
closeHotkeyId is set and ensure no other references rely on CLOSE_TERMINAL for
this pane type.
1 participant
Summary
page-favicon-updatedwebview event, persisted across sessionshttps://github.com/→https://github.com)img-srctohttps:to allow favicons from arbitrary domainsTest plan
Summary by CodeRabbit
New Features
Infrastructure