Repository navigation
Conversation
Add an annotation system to the in-app browser that lets users click elements on a web page, add notes, then send annotations to Claude Code or Codex running in a new terminal tab. Key components: - Inject agentation overlay into browser webview via IIFE bundle - Webhook server + fetch interceptor for reliable webview→main IPC (routes through console.log to bypass CSP connect-src restrictions) - tRPC annotation router with subscriptions for real-time events - Auto-send toggle: each annotation immediately spawns an agent tab - Manual Send button: batch annotations with additional context - Claude Code / Codex agent selector in send dialog - Format annotations into heredoc CLI commands with element details, CSS selectors, React component paths, and user feedback
📝 WalkthroughWalkthroughThis PR introduces a comprehensive annotation feature for the desktop application. Users can toggle annotation mode to capture page elements, which are collected via an injected overlay bundle, streamed through a webhook server, and submitted to AI agents (Claude or Codex) with formatted prompts. Integration includes a new TRPC router, browser manager enhancements, and UI components for annotation handling. Changes
Sequence DiagramsequenceDiagram
participant User
participant BrowserPane
participant useAnnotationMode Hook
participant TRPC Router
participant BrowserManager
participant Webhook Server
participant Annotation Overlay
participant AI Agent
User->>BrowserPane: Click annotation toggle
BrowserPane->>useAnnotationMode Hook: toggle()
useAnnotationMode Hook->>TRPC Router: injectOverlay mutation
TRPC Router->>BrowserManager: injectAnnotationOverlay(paneId)
BrowserManager->>BrowserManager: startWebhookServer()
BrowserManager->>Annotation Overlay: Inject bundle with webhook URL
Annotation Overlay->>Annotation Overlay: Mount Agentation component
User->>Annotation Overlay: Select elements & annotate
Annotation Overlay->>Webhook Server: POST annotation event
Webhook Server->>BrowserManager: Parse & emit event
BrowserManager->>TRPC Router: Stream via subscription
TRPC Router->>useAnnotationMode Hook: onAnnotationAdded event
useAnnotationMode Hook->>useAnnotationMode Hook: formatSinglePrompt mutation
useAnnotationMode Hook->>AI Agent: Create tab with formatted command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 Deployment🔗 Preview Links
Preview updates automatically with new commits |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/browser/annotation/build-bundle.ts`:
- Around line 47-52: The generated bundle files (written by build-bundle.ts via
writeFileSync using variables outfile and tsModulePath and exporting
ANNOTATION_BUNDLE/moduleContent) are being checked into git; update the
repository gitignore (root .gitignore or apps/desktop/.gitignore) to ignore
these generated artifacts by adding entries for
apps/desktop/src/main/lib/browser/annotation/annotation-bundle.js and
apps/desktop/src/main/lib/browser/annotation/annotation-bundle-string.ts (or a
single pattern like
apps/desktop/src/main/lib/browser/annotation/annotation-bundle*) so future runs
of the writeFileSync calls won’t produce tracked files.
In `@apps/desktop/src/main/lib/browser/annotation/injection-wrapper.ts`:
- Around line 287-298: The destroy() function currently omits restoring the
original window.fetch and disconnecting the MutationObserver created in
hideWebhookSettings; add module-scope variables (e.g., originalFetch: typeof
window.fetch | null and settingsObserver: MutationObserver | null), assign
originalFetch in installFetchIntercept and settingsObserver in
hideWebhookSettings, and then update destroy() to restore window.fetch from
originalFetch, disconnect and null out settingsObserver, reset originalFetch to
null, and set fetchInterceptInstalled = false so the fetch override and observer
are fully cleaned up.
- Around line 178-196: The MutationObserver created in hideWebhookSettings runs
indefinitely and should be disconnected once it has successfully hidden the
"Manage MCP & Webhooks" UI to avoid continuous callbacks; modify
hideWebhookSettings so that after finding the toolbar and hiding the matching
section (inside the for loop where btn.textContent?.includes("Manage MCP") is
checked) you call observer.disconnect() (or scope/replace the observer to the
toolbar element if you need continued observation), similar to how
injectToolbarButton disconnects its observer, and ensure you still handle the
case where the toolbar isn't yet present by leaving the observer active until
the hide action completes.
- Around line 65-69: The bug is that the ternary for body (init?.body ?? (input
instanceof Request ? null : null)) always returns null, dropping Request bodies;
fix by deriving body from either init.body or, when input is a Request, by
reading its body text (use await input.clone().text() to avoid consuming the
original stream), e.g. set body = init?.body ?? (input instanceof Request ?
await input.clone().text() : null), then keep the existing string check and
console.log(WEBHOOK_MARKER + body) logic; update the surrounding function to be
async if needed so you can await reading the Request body.
In `@apps/desktop/src/main/lib/browser/browser-manager.ts`:
- Around line 230-244: The annotationActive set is stale across navigations
causing injectAnnotationOverlay(paneId) to early-return after a page navigation;
update the logic so annotation state is cleared or the overlay re-injected on
navigation: add a did-navigate (or equivalent webview) listener for the pane
that either removes paneId from annotationActive or calls
injectAnnotationOverlay again (ensuring startWebhookServer(), evaluateJS(paneId,
...) and getAnnotationBundle() are invoked after navigation), and ensure any
cleanup/unsubscribe when panes are destroyed so annotationActive reflects the
actual page state and isAnnotationActive() returns correct results.
- Around line 178-216: The request-body is accumulated into the local variable
`body` in the `req.on("data", ...)` handler with no size cap; modify the HTTP
request handler in browser-manager (the `req.on("data")` / `req.on("end")`
block) to enforce a maximum body size (e.g. a few hundred KB), incrementally
track bytes received and if the limit is exceeded stop reading, respond with a
413 Payload Too Large (or destroy the socket) and return early; only attempt
JSON.parse and the existing logic that extracts `paneId` and emits
`annotations:submitted:${paneId}` or `annotation:added:${paneId}` after
confirming the body size is within limits. Ensure the size check references the
same `body` accumulation path so malformed or huge requests are rejected before
parsing or emitting.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SendAnnotationsDialog/SendAnnotationsDialog.tsx`:
- Around line 45-66: The handleSend flow currently treats formatPrompt
rejections silently because errors fall through to finally; update handleSend to
catch errors from formatPrompt and any subsequent operations (inside the try)
and surface them to the user (e.g., set an inline error state or call the app's
toast notifier). Specifically, wrap the await formatPrompt and following
addTab/setActiveTab/onOpenChange calls in a try/catch in the handleSend
function, set a local error state (or call the global toast API) with the caught
error message, and ensure setIsPending is still cleared in finally; reference
formatPrompt, handleSend, addTab, setActiveTab, onOpenChange when making
changes.
- Around line 105-107: The render uses new URL(annotationData.pageUrl).pathname
which will throw for malformed or relative values and crash the component;
update SendAnnotationsDialog to safely derive the pathname by wrapping the URL
parsing in a try/catch (or add a helper like safePathname(url: string): string
that returns new URL(url).pathname on success and falls back to the original url
or a default like "page" on failure) and replace the direct new URL(...) call
with a call to that safe function or inline try/catch so the render never throws
for bad annotationData.pageUrl.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/useAnnotationMode/useAnnotationMode.ts`:
- Around line 66-73: The promise returned by formatSinglePrompt in the
onAnnotationAdded flow is not handled for rejections; update the
onAnnotationAdded handler (the call to formatSinglePrompt(...) that currently
.then(({ command }) => addTab(workspaceId, { initialCommands: [command] }))) to
handle errors—either convert to async/await with try/catch or append a .catch()
to the promise chain, and in the catch log the error and avoid throwing (or
surface a user-facing error) so that failures in formatSinglePrompt don't become
unhandled promise rejections; keep references to formatSinglePrompt and addTab
when implementing the fix.
- Around line 78-85: The toggle callback currently calls
toggleAnnotation(paneId) regardless of whether injectOverlay or removeOverlay
throws, causing state desync and unhandled rejections; update the useCallback
toggle implementation to await the overlay mutation inside a try/catch, only
call toggleAnnotation(paneId) when the overlay operation succeeds, rethrow or
return the error so the caller can handle the rejection (preserve the async
return), and ensure dependencies remain [isActive, paneId, injectOverlay,
removeOverlay, toggleAnnotation]; reference functions: toggle (the callback),
injectOverlay, removeOverlay, toggleAnnotation, and the isActive/paneId
parameters.
🧹 Nitpick comments (8)
apps/desktop/package.json (1)
179-179: Update esbuild to the latest version for bug fixes and performance improvements.
^0.24.0is now significantly outdated (released late 2024). The latest esbuild is0.27.3. Consider bumping when convenient to pick up improvements from the intervening releases.apps/desktop/src/lib/trpc/routers/annotation/annotation.ts (1)
72-106:formatPromptandformatSinglePromptare pure transformations — consider using.query()instead of.mutation().These procedures have no side effects (no state changes, no I/O). Using
.query()would be more semantically accurate and would allow tRPC client-side caching if needed. Not blocking, but worth considering.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/useAnnotationMode/useAnnotationMode.ts (1)
48-50: Unnecessary type assertion — subscription data should already be typed.The tRPC subscription
onAnnotationsSubmittedis typed asobservable<AnnotationSubmission>, sodatain theonDatacallback should already carry the correct type. Theas AnnotationDatacast bypasses type checking. If the types actually diverge, define a proper type mapping instead of casting.apps/desktop/src/lib/trpc/routers/annotation/utils/formatAnnotationPrompt.ts (1)
61-125: Consider extracting the shared heredoc command wrapping into a helper.Both
formatAnnotationPromptandformatSingleAnnotationPromptduplicate the delimiter generation and agent-specific command wrapping (lines 39-55 vs 108-124). A small helper likewrapInHeredocCommand(prompt, agent)would reduce duplication.apps/desktop/src/main/lib/browser/annotation/injection-wrapper.ts (1)
87-171: Brittle DOM traversal for toolbar injection.The button injection relies on positional child indexing (
container.children[1]), divider detection by element width (offsetWidth < 5), and sibling class copying. Any change to theagentationlibrary's internal DOM structure will silently break this. This is inherently fragile for an external dependency's internals.Consider adding a defensive early-return with a console warning if the expected structure isn't found, and document the agentation version this targets.
apps/desktop/src/main/lib/browser/browser-manager.ts (3)
155-228: Duplicated webhook event parsing and emission logic.The event parsing and
this.emit(...)calls forannotations:submittedandannotation:addedare duplicated almost identically betweenstartWebhookServer(lines 183–209) and the console-message handler (lines 308–329). This is a DRY violation that will lead to divergence if the event schema changes.Consider extracting a shared
private handleAnnotationPayload(paneId: string, data: {...})method that both paths call.Also applies to: 296-334
190-198: No validation that paneId corresponds to a registered pane.The paneId is extracted directly from the URL path with no check against
paneToWebContentsId. Any HTTP client on localhost can emit events for arbitrary pane IDs. While the blast radius is limited (localhost-only), this could cause unexpected behavior in tRPC subscription listeners.A quick
if (!this.paneToWebContentsId.has(paneId))guard before emitting would tighten this up.
31-33: Webhook server is never shut down.
webhookServeris started lazily but never closed — not onunregister, not when all annotation panes are gone. For a long-running Electron app this is a minor leak (port stays open). Consider shutting the server down whenannotationActivebecomes empty, or exposing a teardown method.
| const moduleContent = `// Auto-generated by build-bundle.ts — do not edit manually\nexport const ANNOTATION_BUNDLE = ${JSON.stringify(code)};\n`; | ||
| writeFileSync(outfile, code); | ||
|
|
||
| // Also write as a TS module for direct import | ||
| const tsModulePath = resolve(__dirname, "annotation-bundle-string.ts"); | ||
| writeFileSync(tsModulePath, moduleContent); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check gitignore status for the generated files
# First, try the provided approach
cd apps/desktop/src/main/lib/browser/annotation/ 2>/dev/null && git check-ignore annotation-bundle.js annotation-bundle-string.ts 2>&1 || echo "---"
# Check all .gitignore files in the repository for relevant patterns
echo "=== Searching for gitignore patterns related to annotation-bundle ==="
find . -name ".gitignore" -type f | xargs grep -l "annotation-bundle" 2>/dev/null || echo "No direct matches found"
# Check for broader patterns that might catch these files
echo "=== Checking for .gitignore files at various levels ==="
git ls-files -o --exclude-standard | grep -E "annotation-bundle\.(js|ts)$" || echo "Files are not in untracked list"
# Check the actual .gitignore files
echo "=== All .gitignore files in the repo ==="
find . -name ".gitignore" -type f | head -20Repository: superset-sh/superset
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Use fd to find .gitignore files more reliably
echo "=== Finding all .gitignore files ==="
fd -g ".gitignore" -type f
echo "=== Content of root .gitignore ==="
if [ -f ".gitignore" ]; then cat .gitignore; fi
echo "=== Content of apps/desktop .gitignore if exists ==="
if [ -f "apps/desktop/.gitignore" ]; then cat apps/desktop/.gitignore; fi
echo "=== Checking git status of generated files ==="
git status apps/desktop/src/main/lib/browser/annotation/annotation-bundle.js 2>&1 || true
git status apps/desktop/src/main/lib/browser/annotation/annotation-bundle-string.ts 2>&1 || true
echo "=== Checking if files are tracked ==="
git ls-files | grep -E "annotation-bundle\.(js|ts)$" || echo "NOT in tracked files"
echo "=== Checking untracked and ignored ==="
git status --porcelain apps/desktop/src/main/lib/browser/annotation/ 2>&1 || trueRepository: superset-sh/superset
Length of output: 2153
Add annotation-bundle.js and annotation-bundle-string.ts to .gitignore.
These generated build artifacts are currently tracked in git. Add patterns to the root .gitignore or apps/desktop/.gitignore to exclude them:
apps/desktop/src/main/lib/browser/annotation/annotation-bundle.js
apps/desktop/src/main/lib/browser/annotation/annotation-bundle-string.ts
Alternatively, add a pattern to cover generated files in this directory:
apps/desktop/src/main/lib/browser/annotation/annotation-bundle*
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/browser/annotation/build-bundle.ts` around lines 47
- 52, The generated bundle files (written by build-bundle.ts via writeFileSync
using variables outfile and tsModulePath and exporting
ANNOTATION_BUNDLE/moduleContent) are being checked into git; update the
repository gitignore (root .gitignore or apps/desktop/.gitignore) to ignore
these generated artifacts by adding entries for
apps/desktop/src/main/lib/browser/annotation/annotation-bundle.js and
apps/desktop/src/main/lib/browser/annotation/annotation-bundle-string.ts (or a
single pattern like
apps/desktop/src/main/lib/browser/annotation/annotation-bundle*) so future runs
of the writeFileSync calls won’t produce tracked files.
| if (targetUrl === webhookUrl) { | ||
| const body = init?.body ?? (input instanceof Request ? null : null); | ||
| if (body && typeof body === "string") { | ||
| console.log(WEBHOOK_MARKER + body); | ||
| } |
There was a problem hiding this comment.
Bug: Request body is never extracted — ternary always evaluates to null.
Line 66: init?.body ?? (input instanceof Request ? null : null) — both branches of the ternary return null, so if agentation passes a Request object to fetch (rather than a URL string + init), the body is silently dropped and the webhook message is never forwarded through console.log.
Proposed fix
- const body = init?.body ?? (input instanceof Request ? null : null);
- if (body && typeof body === "string") {
- console.log(WEBHOOK_MARKER + body);
- }
+ const body = init?.body ?? (input instanceof Request ? (input as Request).body : null);
+ if (body && typeof body === "string") {
+ console.log(WEBHOOK_MARKER + body);
+ } else if (input instanceof Request) {
+ // Request.body is a ReadableStream; read it as text
+ input.text().then((text) => {
+ if (text) console.log(WEBHOOK_MARKER + text);
+ }).catch(() => {});
+ }Note: If agentation always calls fetch(url, { body: "..." }) (i.e. never constructs a Request), this is a latent bug. But it's worth fixing defensively.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/browser/annotation/injection-wrapper.ts` around
lines 65 - 69, The bug is that the ternary for body (init?.body ?? (input
instanceof Request ? null : null)) always returns null, dropping Request bodies;
fix by deriving body from either init.body or, when input is a Request, by
reading its body text (use await input.clone().text() to avoid consuming the
original stream), e.g. set body = init?.body ?? (input instanceof Request ?
await input.clone().text() : null), then keep the existing string check and
console.log(WEBHOOK_MARKER + body) logic; update the surrounding function to be
async if needed so you can await reading the Request body.
| function hideWebhookSettings() { | ||
| const observer = new MutationObserver(() => { | ||
| const toolbar = document.querySelector("[data-feedback-toolbar]"); | ||
| if (!toolbar) return; | ||
|
|
||
| // Find and hide the "Manage MCP & Webhooks" nav link | ||
| const buttons = toolbar.querySelectorAll("button"); | ||
| for (const btn of buttons) { | ||
| if (btn.textContent?.includes("Manage MCP")) { | ||
| const section = btn.closest("div"); | ||
| if (section?.parentElement) { | ||
| (section as HTMLElement).style.display = "none"; | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| observer.observe(document.body, { childList: true, subtree: true }); | ||
| } |
There was a problem hiding this comment.
MutationObserver in hideWebhookSettings is never disconnected.
Unlike injectToolbarButton which calls observer.disconnect() after success (line 167), the observer here runs indefinitely on document.body with subtree: true. This fires the callback on every DOM mutation for the lifetime of the page, which can degrade performance — especially on complex pages with frequent DOM updates.
Proposed fix — disconnect after settings are hidden
function hideWebhookSettings() {
const observer = new MutationObserver(() => {
const toolbar = document.querySelector("[data-feedback-toolbar]");
if (!toolbar) return;
const buttons = toolbar.querySelectorAll("button");
+ let hidden = false;
for (const btn of buttons) {
if (btn.textContent?.includes("Manage MCP")) {
const section = btn.closest("div");
if (section?.parentElement) {
(section as HTMLElement).style.display = "none";
+ hidden = true;
}
}
}
+ if (hidden) observer.disconnect();
});
observer.observe(document.body, { childList: true, subtree: true });
}If the settings can reappear (e.g., after a panel re-render), you'd need to keep observing — but in that case, consider throttling or scoping the observer to the toolbar element once found.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/browser/annotation/injection-wrapper.ts` around
lines 178 - 196, The MutationObserver created in hideWebhookSettings runs
indefinitely and should be disconnected once it has successfully hidden the
"Manage MCP & Webhooks" UI to avoid continuous callbacks; modify
hideWebhookSettings so that after finding the toolbar and hiding the matching
section (inside the for loop where btn.textContent?.includes("Manage MCP") is
checked) you call observer.disconnect() (or scope/replace the observer to the
toolbar element if you need continued observation), similar to how
injectToolbarButton disconnects its observer, and ensure you still handle the
case where the toolbar isn't yet present by leaving the observer active until
the hide action completes.
| function destroy() { | ||
| if (reactRoot) { | ||
| reactRoot.unmount(); | ||
| reactRoot = null; | ||
| } | ||
| const container = document.getElementById(CONTAINER_ID); | ||
| if (container) { | ||
| container.remove(); | ||
| } | ||
| currentAnnotations = []; | ||
| autoSendEnabled = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
destroy() does not clean up the fetch override or the hideWebhookSettings observer.
After destroy(), the patched window.fetch remains in place and the hideWebhookSettings MutationObserver continues running. If the overlay is destroyed and the page continues to be used, the fetch intercept silently inspects all fetches matching the webhook URL, and the observer fires on every DOM mutation.
Consider storing references to the original fetch and active observers so destroy() can restore them:
// In module scope:
let originalFetch: typeof window.fetch | null = null;
let settingsObserver: MutationObserver | null = null;
// In installFetchIntercept:
originalFetch = window.fetch;
// In hideWebhookSettings:
settingsObserver = observer;
// In destroy:
if (originalFetch) { window.fetch = originalFetch; originalFetch = null; }
if (settingsObserver) { settingsObserver.disconnect(); settingsObserver = null; }
fetchInterceptInstalled = false;🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/browser/annotation/injection-wrapper.ts` around
lines 287 - 298, The destroy() function currently omits restoring the original
window.fetch and disconnecting the MutationObserver created in
hideWebhookSettings; add module-scope variables (e.g., originalFetch: typeof
window.fetch | null and settingsObserver: MutationObserver | null), assign
originalFetch in installFetchIntercept and settingsObserver in
hideWebhookSettings, and then update destroy() to restore window.fetch from
originalFetch, disconnect and null out settingsObserver, reset originalFetch to
null, and set fetchInterceptInstalled = false so the fetch override and observer
are fully cleaned up.
| let body = ""; | ||
| req.on("data", (chunk: Buffer) => { | ||
| body += chunk.toString(); | ||
| }); | ||
| req.on("end", () => { | ||
| try { | ||
| const data = JSON.parse(body) as { | ||
| event: string; | ||
| annotation?: Record<string, unknown>; | ||
| annotations?: unknown[]; | ||
| output?: string; | ||
| url?: string; | ||
| }; | ||
| // Extract paneId from URL path: /<paneId> | ||
| const paneId = req.url?.replace(/^\//, ""); | ||
| if (paneId && data.event === "submit" && data.output) { | ||
| this.emit(`annotations:submitted:${paneId}`, { | ||
| annotations: data.annotations ?? [], | ||
| output: data.output, | ||
| pageUrl: data.url ?? "", | ||
| } satisfies AnnotationSubmission); | ||
| } | ||
| if ( | ||
| paneId && | ||
| data.event === "annotation.add" && | ||
| data.annotation | ||
| ) { | ||
| this.emit(`annotation:added:${paneId}`, { | ||
| annotation: data.annotation, | ||
| pageUrl: data.url ?? "", | ||
| }); | ||
| } | ||
| res.writeHead(200, { "Content-Type": "application/json" }); | ||
| res.end(JSON.stringify({ ok: true })); | ||
| } catch { | ||
| res.writeHead(400); | ||
| res.end(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Unbounded request body accumulation — add a size limit.
There's no cap on how much data body can accumulate. A malicious or buggy client on localhost could send an arbitrarily large POST, consuming unbounded memory. Even though the server is bound to 127.0.0.1, the webview content is untrusted (arbitrary web pages).
🛡️ Proposed fix
+ const MAX_BODY = 1024 * 256; // 256 KB
let body = "";
req.on("data", (chunk: Buffer) => {
+ if (body.length + chunk.length > MAX_BODY) {
+ res.writeHead(413);
+ res.end();
+ req.destroy();
+ return;
+ }
body += chunk.toString();
});📝 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.
| let body = ""; | |
| req.on("data", (chunk: Buffer) => { | |
| body += chunk.toString(); | |
| }); | |
| req.on("end", () => { | |
| try { | |
| const data = JSON.parse(body) as { | |
| event: string; | |
| annotation?: Record<string, unknown>; | |
| annotations?: unknown[]; | |
| output?: string; | |
| url?: string; | |
| }; | |
| // Extract paneId from URL path: /<paneId> | |
| const paneId = req.url?.replace(/^\//, ""); | |
| if (paneId && data.event === "submit" && data.output) { | |
| this.emit(`annotations:submitted:${paneId}`, { | |
| annotations: data.annotations ?? [], | |
| output: data.output, | |
| pageUrl: data.url ?? "", | |
| } satisfies AnnotationSubmission); | |
| } | |
| if ( | |
| paneId && | |
| data.event === "annotation.add" && | |
| data.annotation | |
| ) { | |
| this.emit(`annotation:added:${paneId}`, { | |
| annotation: data.annotation, | |
| pageUrl: data.url ?? "", | |
| }); | |
| } | |
| res.writeHead(200, { "Content-Type": "application/json" }); | |
| res.end(JSON.stringify({ ok: true })); | |
| } catch { | |
| res.writeHead(400); | |
| res.end(); | |
| } | |
| }); | |
| const MAX_BODY = 1024 * 256; // 256 KB | |
| let body = ""; | |
| req.on("data", (chunk: Buffer) => { | |
| if (body.length + chunk.length > MAX_BODY) { | |
| res.writeHead(413); | |
| res.end(); | |
| req.destroy(); | |
| return; | |
| } | |
| body += chunk.toString(); | |
| }); | |
| req.on("end", () => { | |
| try { | |
| const data = JSON.parse(body) as { | |
| event: string; | |
| annotation?: Record<string, unknown>; | |
| annotations?: unknown[]; | |
| output?: string; | |
| url?: string; | |
| }; | |
| // Extract paneId from URL path: /<paneId> | |
| const paneId = req.url?.replace(/^\//, ""); | |
| if (paneId && data.event === "submit" && data.output) { | |
| this.emit(`annotations:submitted:${paneId}`, { | |
| annotations: data.annotations ?? [], | |
| output: data.output, | |
| pageUrl: data.url ?? "", | |
| } satisfies AnnotationSubmission); | |
| } | |
| if ( | |
| paneId && | |
| data.event === "annotation.add" && | |
| data.annotation | |
| ) { | |
| this.emit(`annotation:added:${paneId}`, { | |
| annotation: data.annotation, | |
| pageUrl: data.url ?? "", | |
| }); | |
| } | |
| res.writeHead(200, { "Content-Type": "application/json" }); | |
| res.end(JSON.stringify({ ok: true })); | |
| } catch { | |
| res.writeHead(400); | |
| res.end(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/browser/browser-manager.ts` around lines 178 - 216,
The request-body is accumulated into the local variable `body` in the
`req.on("data", ...)` handler with no size cap; modify the HTTP request handler
in browser-manager (the `req.on("data")` / `req.on("end")` block) to enforce a
maximum body size (e.g. a few hundred KB), incrementally track bytes received
and if the limit is exceeded stop reading, respond with a 413 Payload Too Large
(or destroy the socket) and return early; only attempt JSON.parse and the
existing logic that extracts `paneId` and emits
`annotations:submitted:${paneId}` or `annotation:added:${paneId}` after
confirming the body size is within limits. Ensure the size check references the
same `body` accumulation path so malformed or huge requests are rejected before
parsing or emitting.
| async injectAnnotationOverlay(paneId: string): Promise<void> { | ||
| if (this.annotationActive.has(paneId)) return; | ||
|
|
||
| // Start the webhook server and set the per-pane webhook URL on the window | ||
| // so the injection bundle can read it. | ||
| const port = await this.startWebhookServer(); | ||
| const webhookUrl = `http://127.0.0.1:${port}/${paneId}`; | ||
| await this.evaluateJS( | ||
| paneId, | ||
| `window.__supersetWebhookUrl = ${JSON.stringify(webhookUrl)}`, | ||
| ); | ||
|
|
||
| const bundle = this.getAnnotationBundle(); | ||
| await this.evaluateJS(paneId, bundle); | ||
| this.annotationActive.add(paneId); |
There was a problem hiding this comment.
Annotation state becomes stale after page navigation.
injectAnnotationOverlay adds the paneId to annotationActive and injects JS into the current page. If the user navigates to a different URL, the injected overlay and window.__supersetWebhookUrl are lost, but annotationActive still contains the paneId. Subsequent calls to injectAnnotationOverlay will early-return (line 231), and isAnnotationActive will return a false positive.
Consider listening for the webview's did-navigate event to clear annotation state, or re-inject the overlay on navigation.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/browser/browser-manager.ts` around lines 230 - 244,
The annotationActive set is stale across navigations causing
injectAnnotationOverlay(paneId) to early-return after a page navigation; update
the logic so annotation state is cleared or the overlay re-injected on
navigation: add a did-navigate (or equivalent webview) listener for the pane
that either removes paneId from annotationActive or calls
injectAnnotationOverlay again (ensuring startWebhookServer(), evaluateJS(paneId,
...) and getAnnotationBundle() are invoked after navigation), and ensure any
cleanup/unsubscribe when panes are destroyed so annotationActive reflects the
actual page state and isAnnotationActive() returns correct results.
| const handleSend = useCallback(async () => { | ||
| if (!annotationData) return; | ||
|
|
||
| setIsPending(true); | ||
| try { | ||
| const { command } = await formatPrompt({ | ||
| output: annotationData.output, | ||
| pageUrl: annotationData.pageUrl, | ||
| additionalContext: additionalContext.trim() || undefined, | ||
| agent, | ||
| }); | ||
|
|
||
| const { tabId: newTabId } = addTab(workspaceId, { | ||
| initialCommands: [command], | ||
| }); | ||
| setActiveTab(workspaceId, newTabId); | ||
|
|
||
| onOpenChange(false); | ||
| setAdditionalContext(""); | ||
| } finally { | ||
| setIsPending(false); | ||
| } |
There was a problem hiding this comment.
Silent failure — no user feedback when formatPrompt rejects.
If the mutation throws, execution skips to finally and the dialog remains open with no error message. The user has no indication of what went wrong.
Consider catching the error and surfacing it (e.g., via a toast or inline error state).
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SendAnnotationsDialog/SendAnnotationsDialog.tsx`
around lines 45 - 66, The handleSend flow currently treats formatPrompt
rejections silently because errors fall through to finally; update handleSend to
catch errors from formatPrompt and any subsequent operations (inside the try)
and surface them to the user (e.g., set an inline error state or call the app's
toast notifier). Specifically, wrap the await formatPrompt and following
addTab/setActiveTab/onOpenChange calls in a try/catch in the handleSend
function, set a local error state (or call the global toast API) with the caught
error message, and ensure setIsPending is still cleared in finally; reference
formatPrompt, handleSend, addTab, setActiveTab, onOpenChange when making
changes.
| {annotationData?.pageUrl | ||
| ? new URL(annotationData.pageUrl).pathname | ||
| : "page"} |
There was a problem hiding this comment.
new URL() in render path will crash on malformed pageUrl.
If annotationData.pageUrl is not a valid URL (e.g., about:blank, a relative path, or an empty string), new URL(...) throws a TypeError which will unmount this component tree.
Proposed fix
- {annotationData?.pageUrl
- ? new URL(annotationData.pageUrl).pathname
- : "page"}
+ {annotationData?.pageUrl
+ ? (() => {
+ try {
+ return new URL(annotationData.pageUrl).pathname;
+ } catch {
+ return annotationData.pageUrl;
+ }
+ })()
+ : "page"}Or extract a helper:
function safePathname(url: string): string {
try { return new URL(url).pathname; } catch { return url; }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {annotationData?.pageUrl | |
| ? new URL(annotationData.pageUrl).pathname | |
| : "page"} | |
| {annotationData?.pageUrl | |
| ? (() => { | |
| try { | |
| return new URL(annotationData.pageUrl).pathname; | |
| } catch { | |
| return annotationData.pageUrl; | |
| } | |
| })() | |
| : "page"} |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/components/SendAnnotationsDialog/SendAnnotationsDialog.tsx`
around lines 105 - 107, The render uses new URL(annotationData.pageUrl).pathname
which will throw for malformed or relative values and crash the component;
update SendAnnotationsDialog to safely derive the pathname by wrapping the URL
parsing in a try/catch (or add a helper like safePathname(url: string): string
that returns new URL(url).pathname on success and falls back to the original url
or a default like "page" on failure) and replace the direct new URL(...) call
with a call to that safe function or inline try/catch so the render never throws
for bad annotationData.pageUrl.
| formatSinglePrompt({ | ||
| annotation, | ||
| pageUrl, | ||
| agent: agentRef.current, | ||
| }).then(({ command }) => { | ||
| // Create a tab in the background — don't switch to it | ||
| addTab(workspaceId, { initialCommands: [command] }); | ||
| }); |
There was a problem hiding this comment.
Unhandled promise rejection in onAnnotationAdded handler.
formatSinglePrompt(...).then(...) has no .catch(). If the mutation fails, the rejection propagates as an unhandled promise rejection, which can crash in strict environments or silently swallow errors.
Proposed fix
formatSinglePrompt({
annotation,
pageUrl,
agent: agentRef.current,
}).then(({ command }) => {
addTab(workspaceId, { initialCommands: [command] });
-});
+}).catch((err) => {
+ console.error("Failed to format single annotation prompt:", err);
+});📝 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.
| formatSinglePrompt({ | |
| annotation, | |
| pageUrl, | |
| agent: agentRef.current, | |
| }).then(({ command }) => { | |
| // Create a tab in the background — don't switch to it | |
| addTab(workspaceId, { initialCommands: [command] }); | |
| }); | |
| formatSinglePrompt({ | |
| annotation, | |
| pageUrl, | |
| agent: agentRef.current, | |
| }).then(({ command }) => { | |
| // Create a tab in the background — don't switch to it | |
| addTab(workspaceId, { initialCommands: [command] }); | |
| }).catch((err) => { | |
| console.error("Failed to format single annotation prompt:", err); | |
| }); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/useAnnotationMode/useAnnotationMode.ts`
around lines 66 - 73, The promise returned by formatSinglePrompt in the
onAnnotationAdded flow is not handled for rejections; update the
onAnnotationAdded handler (the call to formatSinglePrompt(...) that currently
.then(({ command }) => addTab(workspaceId, { initialCommands: [command] }))) to
handle errors—either convert to async/await with try/catch or append a .catch()
to the promise chain, and in the catch log the error and avoid throwing (or
surface a user-facing error) so that failures in formatSinglePrompt don't become
unhandled promise rejections; keep references to formatSinglePrompt and addTab
when implementing the fix.
| const toggle = useCallback(async () => { | ||
| if (isActive) { | ||
| await removeOverlay({ paneId }); | ||
| } else { | ||
| await injectOverlay({ paneId }); | ||
| } | ||
| toggleAnnotation(paneId); | ||
| }, [isActive, paneId, injectOverlay, removeOverlay, toggleAnnotation]); |
There was a problem hiding this comment.
Store state desyncs from overlay if the mutation rejects.
If injectOverlay or removeOverlay throws (e.g., webview is gone, IPC failure), toggleAnnotation(paneId) on line 84 still executes, leaving isActive out of sync with the actual overlay state. The caller in BrowserPane also ignores the returned promise, so the rejection is unhandled.
Proposed fix — catch and guard
const toggle = useCallback(async () => {
- if (isActive) {
- await removeOverlay({ paneId });
- } else {
- await injectOverlay({ paneId });
+ try {
+ if (isActive) {
+ await removeOverlay({ paneId });
+ } else {
+ await injectOverlay({ paneId });
+ }
+ toggleAnnotation(paneId);
+ } catch (err) {
+ console.error("Failed to toggle annotation overlay:", err);
}
- toggleAnnotation(paneId);
}, [isActive, paneId, injectOverlay, removeOverlay, toggleAnnotation]);🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/BrowserPane/hooks/useAnnotationMode/useAnnotationMode.ts`
around lines 78 - 85, The toggle callback currently calls
toggleAnnotation(paneId) regardless of whether injectOverlay or removeOverlay
throws, causing state desync and unhandled rejections; update the useCallback
toggle implementation to await the overlay mutation inside a try/catch, only
call toggleAnnotation(paneId) when the overlay operation succeeds, rethrow or
return the error so the caller can handle the rejection (preserve the async
return), and ensure dependencies remain [isActive, paneId, injectOverlay,
removeOverlay, toggleAnnotation]; reference functions: toggle (the callback),
injectOverlay, removeOverlay, toggleAnnotation, and the isActive/paneId
parameters.
|
Closing: stale experimental browser annotation PR from Feb 16 with no recent activity. |
|
Hey — just a heads up, this was closed as part of an automated stale PR cleanup. If you think this was done in error, feel free to reopen it! |
|
Hey — this was closed by an automated cleanup of PRs with major merge conflicts that are 3+ weeks old. If you think this was done incorrectly, please feel free to reopen it. Sorry for any inconvenience! |
Summary
agentationoverlay into browser webviews via a bundled IIFE (React + agentation component)connect-srcrestrictions that block webview→localhost fetchesNew files
annotation/injection-wrapper.ts— IIFE entry point injected into webviewsannotation/build-bundle.ts— esbuild script to bundle agentation + React into IIFErouters/annotation/annotation.ts— tRPC router (inject, remove, subscriptions, prompt formatting)routers/annotation/utils/formatAnnotationPrompt.ts— Formats annotations into claude/codex CLI commandsSendAnnotationsDialog.tsx— Dialog for batch send with agent selector and context textareauseAnnotationMode.ts— Hook managing overlay lifecycle, subscriptions, and auto-sendTest plan
Summary by CodeRabbit
Release Notes