Conversation
- Canvas - Browser
Add Prettier (^3.5.3) to the dev tooling to enforce consistent code formatting, and add concurrently (^9.2.1) to dependencies to support running multiple processes in scripts. Also adjust trailing comma for JSON validity.
There was a problem hiding this comment.
Pull request overview
This PR introduces a built-in Browser tab (Electron webview-backed, with element selection → context insertion) and a new Canvas workspace mode that lays out tabs as movable/resizable panels with grouping and persisted state.
Changes:
- Add Browser tab kind end-to-end (tab identity, stores, panel registration, UI entry points, external URL handling).
- Add Canvas mode to Workspace screen (toggle, canvas surface, persisted layout/group state).
- Performance/UX adjustments (pane-context stabilization, stream virtualization callback stabilization, terminal padding, message input focus handling).
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/website/src/routeTree.gen.ts | Regenerates router tree entries (adds /docs/skills ordering). |
| packages/desktop/src/main.ts | Enables Electron webviewTag and hooks webview attach/window-open behavior. |
| packages/desktop/src/daemon/daemon-manager.ts | Adds dev-origin-derived CORS origin merging when spawning daemon. |
| packages/app/src/utils/workspace-tab-identity.ts | Extends tab identity helpers to support browser targets. |
| packages/app/src/utils/open-external-url.ts | Opens external URLs in a workspace Browser tab when possible (web/Electron). |
| packages/app/src/stores/workspace-tabs-store.ts | Adds browser tab target support to workspace tabs store. |
| packages/app/src/stores/navigation-active-workspace-store.ts | Exposes a getter for the active workspace selection snapshot. |
| packages/app/src/stores/browser-store.ts | Introduces persisted zustand store for browser sessions/metadata. |
| packages/app/src/stores/browser-element-selection.ts | Adds element-selection → agent-context insertion flow (with canvas-group lookup). |
| packages/app/src/screens/workspace/workspace-tab-menu.ts | Adds browser close-button test id support. |
| packages/app/src/screens/workspace/workspace-screen.tsx | Adds browser tab creation/close behavior + canvas mode toggle + canvas surface rendering. |
| packages/app/src/screens/workspace/workspace-pane-content.tsx | Wraps pane rendering in memo for performance. |
| packages/app/src/screens/workspace/workspace-desktop-tabs-row.tsx | Adds “new browser tab” toolbar action + browser fallback label. |
| packages/app/src/screens/workspace/workspace-actions-menu.tsx | New shared workspace header/canvas actions dropdown. |
| packages/app/src/panels/register-panels.ts | Registers the new browser panel kind. |
| packages/app/src/panels/pane-context.tsx | Stabilizes pane context value via refs/memo to reduce rerenders. |
| packages/app/src/panels/browser-panel.tsx | Adds browser panel registration + descriptor derived from browser store. |
| packages/app/src/panels/agent-panel.tsx | Removes focus-based initialization effect (keeps entry-based init). |
| packages/app/src/components/workspace-canvas-surface.tsx | New canvas surface implementation (drag, resize, zoom/scale, grouping, persistence). |
| packages/app/src/components/terminal-emulator.tsx | Tweaks terminal padding. |
| packages/app/src/components/stream-strategy-web.tsx | Stabilizes virtualizer callbacks via refs/callbacks. |
| packages/app/src/components/split-container.tsx | Extends “new tab” option typing to include browser. |
| packages/app/src/components/message-input.tsx | Uses ref to avoid stale onFocusChange in unmount cleanup. |
| packages/app/src/components/browser-pane.web.tsx | Electron webview-backed browser UI + element selector injection/polling. |
| packages/app/src/components/browser-pane.tsx | Non-web fallback placeholder for browser pane. |
| packages/app/src/components/agent-input-area.tsx | Displays element-context chips and prepends formatted context on send. |
| package.json | Adds Prettier and adds concurrently to dependencies. |
| package-lock.json | Updates lockfile for added dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mainWindow.webContents.on("did-attach-webview", (_event, contents) => { | ||
| contents.setWindowOpenHandler(({ url }) => { | ||
| contents.loadURL(url).catch(() => undefined); | ||
| return { action: "deny" }; | ||
| }); |
There was a problem hiding this comment.
setWindowOpenHandler currently loads any requested url into the webview (contents.loadURL(url)). This allows window.open() to navigate to potentially dangerous protocols (e.g. file:, javascript:, data:) and bypass intended restrictions. Consider validating/whitelisting allowed schemes/hosts before calling loadURL, and returning { action: "deny" } without navigation for anything else.
| const childEnv = { | ||
| ...invocation.env, | ||
| PASEO_DESKTOP_MANAGED: "1", | ||
| PASEO_CORS_ORIGINS: mergeCorsOrigins( | ||
| invocation.env.PASEO_CORS_ORIGINS, | ||
| app.isPackaged ? null : resolveDevServerOrigin(process.env.EXPO_DEV_URL), | ||
| ), |
There was a problem hiding this comment.
childEnv sets PASEO_CORS_ORIGINS to the return value of mergeCorsOrigins(...), which can be undefined. When passed to spawn({ env }), an undefined value can be stringified to 'undefined' and unintentionally set in the child process environment. Build env so PASEO_CORS_ORIGINS is omitted entirely when there are no origins to set.
| const childEnv = { | |
| ...invocation.env, | |
| PASEO_DESKTOP_MANAGED: "1", | |
| PASEO_CORS_ORIGINS: mergeCorsOrigins( | |
| invocation.env.PASEO_CORS_ORIGINS, | |
| app.isPackaged ? null : resolveDevServerOrigin(process.env.EXPO_DEV_URL), | |
| ), | |
| const mergedCorsOrigins = mergeCorsOrigins( | |
| invocation.env.PASEO_CORS_ORIGINS, | |
| app.isPackaged ? null : resolveDevServerOrigin(process.env.EXPO_DEV_URL), | |
| ); | |
| const childEnv = { | |
| ...invocation.env, | |
| PASEO_DESKTOP_MANAGED: "1", | |
| ...(mergedCorsOrigins === undefined | |
| ? {} | |
| : { PASEO_CORS_ORIGINS: mergedCorsOrigins }), |
| </PaneProvider> | ||
| ); | ||
| }, | ||
| (prev, next) => prev.content === next.content || prev.content.key === next.content.key, |
There was a problem hiding this comment.
The memo comparator always returns true for any rerender where content.key matches, but content.key is derived only from tabId (${serverId}:${workspaceId}:${tab.tabId}). This prevents PaneProvider from receiving updates when paneContextValue changes (e.g. isPaneFocused, target, callbacks), leading to stale pane context. The comparator should only skip when the entire content object is referentially equal (or explicitly compare all fields that affect rendering).
| (prev, next) => prev.content === next.content || prev.content.key === next.content.key, | |
| (prev, next) => prev.content === next.content, |
| const oldScale = canvasScale; | ||
| const newScale = clampZoom(canvasScale + direction * ZOOM_STEP); | ||
| // Adjust offset so the point under the cursor stays fixed | ||
| // pointerX = canvasOffset.x + worldX * oldScale => worldX = (pointerX - canvasOffset.x) / oldScale | ||
| // pointerX = newOffset.x + worldX * newScale => newOffset.x = pointerX - worldX * newScale | ||
| const worldX = (pointerX - canvasOffset.x) / oldScale; | ||
| const worldY = (pointerY - canvasOffset.y) / oldScale; | ||
| setCanvasScale(newScale); | ||
| setCanvasOffset({ | ||
| x: pointerX - worldX * newScale, | ||
| y: pointerY - worldY * newScale, | ||
| }); | ||
| return; | ||
| } | ||
| if (ctrlKey) { | ||
| const oldZoom = cameraZoom; | ||
| const newZoom = clampZoom(cameraZoom + direction * ZOOM_STEP); | ||
| const worldX = (pointerX - canvasOffset.x) / (canvasScale * oldZoom); | ||
| const worldY = (pointerY - canvasOffset.y) / (canvasScale * oldZoom); | ||
| setCameraZoom(newZoom); | ||
| setCanvasOffset({ | ||
| x: pointerX - worldX * canvasScale * newZoom, | ||
| y: pointerY - worldY * canvasScale * newZoom, |
There was a problem hiding this comment.
This useEffect registers a wheel handler that uses canvasOffset (and cameraZoom) from the closure, but the dependency array does not include canvasOffset (or ZOOM_STEP if it can change). This can cause zoom-to-cursor to use stale offsets after panning/dragging. Include the missing state in dependencies, or move the mutable values into refs / use functional state updates inside the handler.
| const oldScale = canvasScale; | |
| const newScale = clampZoom(canvasScale + direction * ZOOM_STEP); | |
| // Adjust offset so the point under the cursor stays fixed | |
| // pointerX = canvasOffset.x + worldX * oldScale => worldX = (pointerX - canvasOffset.x) / oldScale | |
| // pointerX = newOffset.x + worldX * newScale => newOffset.x = pointerX - worldX * newScale | |
| const worldX = (pointerX - canvasOffset.x) / oldScale; | |
| const worldY = (pointerY - canvasOffset.y) / oldScale; | |
| setCanvasScale(newScale); | |
| setCanvasOffset({ | |
| x: pointerX - worldX * newScale, | |
| y: pointerY - worldY * newScale, | |
| }); | |
| return; | |
| } | |
| if (ctrlKey) { | |
| const oldZoom = cameraZoom; | |
| const newZoom = clampZoom(cameraZoom + direction * ZOOM_STEP); | |
| const worldX = (pointerX - canvasOffset.x) / (canvasScale * oldZoom); | |
| const worldY = (pointerY - canvasOffset.y) / (canvasScale * oldZoom); | |
| setCameraZoom(newZoom); | |
| setCanvasOffset({ | |
| x: pointerX - worldX * canvasScale * newZoom, | |
| y: pointerY - worldY * canvasScale * newZoom, | |
| // Adjust offset so the point under the cursor stays fixed | |
| // pointerX = canvasOffset.x + worldX * oldScale => worldX = (pointerX - canvasOffset.x) / oldScale | |
| // pointerX = newOffset.x + worldX * newScale => newOffset.x = pointerX - worldX * newScale | |
| setCanvasScale((previousCanvasScale) => { | |
| const newScale = clampZoom(previousCanvasScale + direction * ZOOM_STEP); | |
| setCanvasOffset((previousCanvasOffset) => { | |
| const worldX = (pointerX - previousCanvasOffset.x) / previousCanvasScale; | |
| const worldY = (pointerY - previousCanvasOffset.y) / previousCanvasScale; | |
| return { | |
| x: pointerX - worldX * newScale, | |
| y: pointerY - worldY * newScale, | |
| }; | |
| }); | |
| return newScale; | |
| }); | |
| return; | |
| } | |
| if (ctrlKey) { | |
| setCameraZoom((previousCameraZoom) => { | |
| const newZoom = clampZoom(previousCameraZoom + direction * ZOOM_STEP); | |
| setCanvasOffset((previousCanvasOffset) => { | |
| const worldX = (pointerX - previousCanvasOffset.x) / (canvasScale * previousCameraZoom); | |
| const worldY = (pointerY - previousCanvasOffset.y) / (canvasScale * previousCameraZoom); | |
| return { | |
| x: pointerX - worldX * canvasScale * newZoom, | |
| y: pointerY - worldY * canvasScale * newZoom, | |
| }; | |
| }); | |
| return newZoom; |
| const canvasContentModels = useMemo(() => { | ||
| const prev = canvasContentModelsRef.current; | ||
| const next = new Map<string, WorkspacePaneContentModel>(); | ||
| const tabIds = new Set(tabs.map((tab) => tab.tabId)); |
There was a problem hiding this comment.
tabIds is created but never used. With the app's linting (eslint-config-expo), this can fail CI for unused locals. Remove it or use it (e.g. to prune cached content models for removed tabs).
| const tabIds = new Set(tabs.map((tab) => tab.tabId)); |
| (webview as any).executeJavaScript(js).then(() => { | ||
| // Poll for result | ||
| const poll = setInterval(() => { | ||
| (webview as any).executeJavaScript('JSON.stringify(window.__paseoSelectorResult || null)') | ||
| .then((raw: string) => { | ||
| const result = JSON.parse(raw); | ||
| if (result) { | ||
| clearInterval(poll); | ||
| setSelectorActive(false); | ||
| (webview as any).executeJavaScript('window.__paseoSelectorResult = null;'); | ||
| if (!result.__cancelled) { | ||
| void insertBrowserElementContext(browserIdRef.current, result as BrowserElementSelection); | ||
| } | ||
| } | ||
| }) | ||
| .catch(() => {}); | ||
| }, 200); | ||
| // Auto-cancel after 30s | ||
| setTimeout(() => { | ||
| clearInterval(poll); | ||
| setSelectorActive(false); | ||
| try { | ||
| (webview as any).executeJavaScript('if(window.__paseoSelector) window.__paseoSelector.destroy();'); | ||
| } catch {} | ||
| }, 30000); | ||
| }); |
There was a problem hiding this comment.
The element selector polling sets an interval and a 30s timeout but never clears the timeout when a selection is made, and there is no cleanup on component unmount. This can flip selectorActive back to false later and run destroy() unexpectedly, plus leak timers. Track both timer IDs (e.g. in refs) and clear them when a result is received, when cancelled, and in the effect cleanup.
| "@modelcontextprotocol/sdk": "^1.27.1", | ||
| "concurrently": "^9.2.1" |
There was a problem hiding this comment.
concurrently is listed in both devDependencies and dependencies. This duplicates the package in the lockfile and can cause inconsistent installs. Keep it in only one section (typically devDependencies if it's only used by scripts).
| "@modelcontextprotocol/sdk": "^1.27.1", | |
| "concurrently": "^9.2.1" | |
| "@modelcontextprotocol/sdk": "^1.27.1" |
| function normalizeBrowserUrl(value: string | null | undefined): string { | ||
| const trimmed = trimNonEmpty(value); | ||
| if (!trimmed) { | ||
| return "https://example.com"; | ||
| } | ||
| if (/^[a-zA-Z][a-zA-Z\d+.-]*:/.test(trimmed)) { | ||
| return trimmed; | ||
| } | ||
| if (trimmed.startsWith("//")) { | ||
| return `https:${trimmed}`; | ||
| } | ||
| return `https://${trimmed}`; | ||
| } | ||
|
|
||
| function createBrowserId(): string { | ||
| if (typeof globalThis.crypto?.randomUUID === "function") { | ||
| return globalThis.crypto.randomUUID(); | ||
| } | ||
| return `${Date.now()}-${Math.random().toString(16).slice(2)}`; | ||
| } | ||
|
|
||
| export const useBrowserStore = create<BrowserStoreState>()( | ||
| persist( | ||
| (set) => ({ | ||
| browsersById: {}, | ||
| createBrowser: (input) => { | ||
| const browserId = createBrowserId(); | ||
| const now = Date.now(); | ||
| const initialUrl = normalizeBrowserUrl(input?.initialUrl); | ||
|
|
||
| set((state) => ({ | ||
| browsersById: { | ||
| ...state.browsersById, | ||
| [browserId]: { | ||
| browserId, | ||
| url: initialUrl, |
There was a problem hiding this comment.
This new persisted browser store introduces URL normalization, ID generation, and record patching logic, but there are no accompanying tests. Given the repository already has Vitest coverage for other stores (e.g. workspace-tabs-store.test.ts, workspace-layout-store.test.ts), it would be good to add unit tests for normalizeWorkspaceBrowserUrl/createBrowser/updateBrowser behavior and persistence partialization.
Remove the root devDependency "prettier" from package.json and update package-lock.json accordingly. Update the npm "cli" script to run the TypeScript entry directly via tsx (npx tsx packages/cli/src/index.ts) instead of the .js file, so the CLI runs the TS source.
|
@copilot apply changes based on the comments in this thread |
This PR adds two significant features