Fix browser inspect element pills with code quality improvements#133
Merged
Conversation
## Changes ### 1. Fix DevTools Expansion Bug (Tauri Native Layer) When clicking the DevTools button, the browser view would unexpectedly expand. Root cause: `_inspector.show()` docks by splitting WKWebView's superview, resizing the webview. After `detach()` moves inspector to floating window, frame isn't restored. **Fix:** Save WKWebView frame before show(), restore after detach(). - Modified: src-tauri/src/commands/webview.rs (lines 570-582) ### 2. Fix Cursor Animation Attribute Mismatch (Issue #1 - HIGH) Cursor visual effects (move, ripple, pin) were silently failing because they searched for `data-cursor-ref` but elements were marked with `data-hive-ref`. **Fix:** Replace all `data-cursor-ref` references with `data-hive-ref` to match the actual attribute used in inject-mode.ts. - Modified: src/features/browser/automation/visual-effects.ts - Line 18: JSDoc comment - Line 25: buildMoveCursorAndRippleJs() - Line 45: buildPinCursorJs() - Line 96: buildHighlightElementJs() ### 3. Add IME Composition Guard to MessageInput (Issue #2 - MEDIUM) CJK (Chinese, Japanese, Korean) users using Input Method Editors (IME) would lose their input when pressing Enter during composition, since the handler didn't check if composition was in progress. **Fix:** Add `!e.nativeEvent.isComposing` guard to Enter key handler to prevent message send during active IME composition. - Modified: src/features/session/ui/MessageInput.tsx (line 286) ### 4. Fix Element Selector Injection Exception Handling (Issue #3 - MEDIUM) When `verify_injection` script threw an exception, the catch block didn't return, causing exception object to fall through to `eval_browser_webview_with_result`, which would parse it as a result string instead of error. **Fix:** Add explicit `return;` in catch block to prevent fall-through. - Modified: src/features/browser/ui/BrowserTab.tsx (line 521) ### 5. Fix Selector Toggle State on Eval Failure (Issue #4 - MEDIUM) When element selector injection failed, `selectorActive` was set in the UI store but outside the try block, so errors during injection were silently swallowed. **Fix:** Move state update inside try block to fail-fast on injection errors. - Modified: src/features/browser/ui/BrowserTab.tsx (line 563) ### 6. Add Polling Guard to Inspect Event Drain (Issue #5 - MEDIUM) Inspect event drain was polling unconditionally every 200ms, consuming ~5 IPC calls/second unnecessarily. With multiple tabs this compounds quickly. **Fix:** Only poll when selector is active. Add `!tab.selectorActive` guard and add to dependency array. - Modified: src/features/browser/ui/BrowserTab.tsx (lines 396-458) ### 7. Replace if/else with ts-pattern (Issue #6 - LOW) Long if/else chain for event type dispatch violates CLAUDE.md requirement to use ts-pattern for discriminated union dispatch. **Fix:** Import ts-pattern and use `.with()` / `.otherwise()` for event type matching. - Modified: src/features/browser/ui/BrowserTab.tsx (lines 26, 427-444) ### 8. Fix Hover Animation Timing (Issue #7 - LOW) InspectedElementCard hover transition was `duration-150` instead of `duration-200`, violating CLAUDE.md animation standard of 200-300ms default duration. **Fix:** Update to `duration-200 ease` to match project animation guidelines. - Modified: src/features/session/ui/InspectedElementCard.tsx (line 48) ## Verification - TypeScript compilation: ✓ (pre-existing Lucide icon issue unrelated to changes) - All fixes verified and applied by parallel code-reviewer agents - Changes preserve existing functionality while fixing correctness issues - Performance improvement: Eliminates ~5 IPC calls/second from unconditional polling ## Impact - **Bug fixes:** 3 (DevTools expansion, cursor animations, IME composition) - **Correctness improvements:** 2 (exception handling, state sync) - **Performance:** Reduced IPC quota consumption from inspect event drain - **Code quality:** Full CLAUDE.md compliance (ts-pattern, animation timing) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a browser automation injection system: prebuilt in-page scripts (browser-utils, inspect-mode, visual-effects) compiled to dist-inject, build tooling and CI/dev wiring to create those bundles, runtime injection and periodic drain flows in BrowserTab, new Tauri devtools commands, UI for inspected elements and message-input integration, plus related types, exports, and small UX/comment refinements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant React as BrowserPanel/Session
participant Tab as BrowserTab
participant Tauri as Tauri Bridge
participant WK as WKWebView
participant Inject as Injected Scripts
User->>React: click element selector
React->>Tab: toggleElementSelector()
Tab->>Tauri: eval INSPECT_MODE_ENABLE
Tauri->>WK: execute JS
WK->>Inject: window.__hiveInspect.enable()
Inject-->>WK: overlay & event buffer installed
User->>WK: select element
Inject->>Inject: buffer element-selected event
Tab->>Tauri: periodic eval (drainEvents)
Tauri->>WK: run drain script
WK->>Inject: window.__hiveInspect.drainEvents()
Inject-->>WK: events JSON
WK-->>Tauri: return string
Tauri-->>Tab: parsed events
Tab->>React: onElementSelected
React->>Session: dispatch insert-to-chat (addInspectedElement)
sequenceDiagram
participant Tab as BrowserTab
participant Tauri as Tauri Bridge
participant WK as WKWebView
participant Visual as Injected Visuals
Tab->>Tauri: eval buildMoveCursorAndRippleJs(ref)
Tauri->>WK: execute JS
WK->>Visual: window.__hiveVisuals.moveCursorToElement(el)
Visual->>Visual: animate cursor SVG
WK->>Visual: window.__hiveVisuals.rippleAt(x,y)
Visual->>Visual: render ripple effect
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Comment |
zvadaadam
commented
Feb 23, 2026
| return; | ||
| } | ||
| for (const log of logs) { | ||
| const level = ( |
Owner
Author
There was a problem hiding this comment.
ts-pattern required for discriminated union dispatch
This nested ternary chain dispatches on log.l (a discriminator field mapping to ConsoleLog["level"]). Per CLAUDE.md:
Use
ts-patternfor switch/case and if/else chains on discriminated unions. When to use: Any switch/case or if/else chain dispatching on.type,.status,.state, or similar discriminator fields.
The match import from ts-pattern is already present in this file (used correctly in the inspect event drain at line 428). This console drain should be consistent:
const level = match(log.l)
.with("warn", () => "warn" as const)
.with("error", () => "error" as const)
.with("debug", () => "debug" as const)
.otherwise(() => "info" as const);
zvadaadam
commented
Feb 23, 2026
| // Uses a serialized queue to avoid collision with console drain. | ||
| // 2. FALLBACK: Pull-based event buffer (window.__HIVE_INSPECT_EVENTS__) | ||
| // drained by eval_browser_webview_with_result from the React side. | ||
| // Catches any events missed by the title channel. |
Owner
Author
There was a problem hiding this comment.
Contradictory architecture comments — stale title-channel description will mislead future developers
This file has three contradictory comments about the communication mechanism:
- Lines 12-18 (top of file): Describes title-channel as "PRIMARY" and buffer as "FALLBACK"
- Lines 38-53 (section header): Repeats that title-channel is primary with detailed queue/timing description
- Lines 58-68 (architecture note): Correctly says "DO NOT use document.title for inspect events"
The actual implementation at line 70-72 (sendToFrontend) only pushes to eventBuffer — no title-channel code exists. The top comments describe a design that was abandoned.
Additionally, BrowserTab.tsx line 393 says "This is the FALLBACK path — the primary path is the title-channel listeners above" but the title-channel listeners were removed from that same file.
And src-tauri/src/commands/webview.rs references a sendViaTitle() function that doesn't exist in the inject script.
Per MEMORY.md: "Never use document.title (title-channel) for inspect events. Use buffer + eval_browser_webview_with_result drain instead."
These stale comments directly contradict the correct architecture note at line 58 and could lead a future developer to reintroduce the title-channel bug.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sidecar/agents/hive-tools/browser.ts (1)
848-859:⚠️ Potential issue | 🔴 CriticalFix data attribute mismatch: sidecar must use
data-hive-refinstead ofdata-cursor-ref.The frontend automation scripts (visual-effects.ts, browser-utils.ts, inspect-mode.ts) now consistently use
data-hive-refto mark elements, but the sidecar still queries fordata-cursor-refon line 849. This will cause the querySelector to return null, breaking ref-based screenshot cropping.Update required in:
- sidecar/agents/hive-tools/browser.ts (line 849 and related code)
- sidecar/protocol.ts (update descriptions to reference
data-hive-refinstead ofdata-cursor-ref)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/agents/hive-tools/browser.ts` around lines 848 - 859, The injected DOM query in sidecar/agents/hive-tools/browser.ts is still looking for data-cursor-ref and should be changed to data-hive-ref: update the selector string in the injected script (the snippet that declares var el = document.querySelector('[data-cursor-ref="${args.ref}"]')) to use '[data-hive-ref="${args.ref}"]' and keep the rest of the bounding-box logic unchanged; also update any related error text if it mentions data-cursor-ref. Additionally, update the protocol descriptions in sidecar/protocol.ts that reference data-cursor-ref so they instead mention data-hive-ref to match the frontend automation scripts (visual-effects.ts, browser-utils.ts, inspect-mode.ts).
🧹 Nitpick comments (8)
sidecar/agents/hive-tools/browser.ts (1)
206-209: Tool descriptions referencedata-cursor-ref— verify consistency with actual implementation.Multiple tool descriptions (BrowserClick, BrowserType, BrowserEvaluate, BrowserHover, BrowserSelectOption) reference
data-cursor-ref. If the injection scripts now usedata-hive-refas indicated in the PR objectives, these descriptions should be updated to avoid confusing the AI agent.Run the verification script from the previous comment to confirm which attribute is used, then update all tool descriptions accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sidecar/agents/hive-tools/browser.ts` around lines 206 - 209, The tool descriptions for BrowserClick, BrowserType, BrowserEvaluate, BrowserHover, and BrowserSelectOption reference the attribute name "data-cursor-ref" but the injected scripts use "data-hive-ref"; run the verification script referenced in the prior PR comment to confirm the actual attribute, then update each tool's .describe(...) text to reference the confirmed attribute ("data-hive-ref" if verified) so the descriptions match the implementation and avoid confusing the agent.src/features/session/ui/InspectedElementCard.tsx (1)
37-41: Consider aligning motion timing with the 200–300ms default.The current 150ms duration is shorter than the standard animation timing guideline for the app; bumping to 0.2s keeps the motion feel consistent.
As per coding guidelines, “Default duration is 200-300ms.”♻️ Suggested timing tweak
- transition={{ duration: 0.15, ease: [0.165, 0.84, 0.44, 1] }} + transition={{ duration: 0.2, ease: [0.165, 0.84, 0.44, 1] }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/session/ui/InspectedElementCard.tsx` around lines 37 - 41, Update the motion transition duration in the InspectedElementCard component: change the transition prop on the animated wrapper (the object with initial/animate/exit/transition) from duration: 0.15 to duration: 0.2 so the animation aligns with the app's 200–300ms default; keep the rest of the transition (ease array) unchanged.src/features/session/ui/blocks/TextBlock.tsx (1)
68-73: Prefer stable keys for inspect segments.Using the array index can cause remounts if segments shift;
InspectElementalready hasrefwhich is a good stable key when the segment is an element.♻️ Suggested tweak for stable keys
- {inspectSegments.map((segment, i) => + {inspectSegments.map((segment, i) => typeof segment === "string" ? ( <span key={i}>{segment}</span> ) : ( - <InspectElementPill key={`inspect-${i}`} element={segment} /> + <InspectElementPill key={segment.ref ?? `inspect-${i}`} element={segment} /> ) )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/session/ui/blocks/TextBlock.tsx` around lines 68 - 73, The inspectSegments mapping uses the array index as a key which can cause remounts; update the JSX in TextBlock (where inspectSegments is mapped and InspectElementPill is rendered) to use a stable key: when segment is an element, use its existing ref (e.g., segment.ref) as the key for <InspectElementPill key=...>, otherwise use a stable string-based key (for example the segment string value or a deterministic id derived from it) instead of the index so both string spans and InspectElementPill components have stable, non-index keys.src/features/browser/automation/build-inject.ts (1)
23-26: Unnecessary tsconfig.json filter.The filter on line 25 checks
f !== "tsconfig.json", but the previous conditionf.endsWith(".ts")already excludes JSON files. This filter is redundant.🧹 Suggested cleanup
const entryPoints = fs .readdirSync(injectDir) - .filter((f) => f.endsWith(".ts") && !f.endsWith(".d.ts") && f !== "tsconfig.json") + .filter((f) => f.endsWith(".ts") && !f.endsWith(".d.ts")) .map((f) => path.join(injectDir, f));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/automation/build-inject.ts` around lines 23 - 26, The filter for entryPoints is redundantly checking f !== "tsconfig.json"; update the entryPoints creation (the readdirSync/filter/map block using injectDir and entryPoints) to remove that unnecessary check and only filter for TypeScript files while excluding declaration files (e.g., keep the f.endsWith(".ts") && !f.endsWith(".d.ts") condition or equivalent), so tsconfig.json is not separately tested.src/features/browser/ui/BrowserPanel.tsx (1)
473-493: Consider user feedback for screenshot failures.The error is logged to console but the user receives no feedback when
screenshot_browser_webviewfails. Consider showing a toast notification.💡 Optional: Add user feedback on screenshot failure
} catch (err) { console.error("Browser screenshot failed:", err); + // TODO: Consider showing a toast notification here + // e.g., toast.error("Screenshot failed") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/ui/BrowserPanel.tsx` around lines 473 - 493, The catch block in handleScreenshot only logs errors to console and gives no user feedback; update the catch to also surface a user-facing toast/notification (e.g., call an existing showToast/toast utility or dispatch a CustomEvent like "show-toast" or "insert-toast") when invoke("screenshot_browser_webview", ...) fails, include a friendly message plus the error details (err.message) and keep the existing console.error for debugging; modify the handleScreenshot catch to trigger that toast/notification so users know the screenshot failed.src-tauri/src/commands/webview.rs (1)
452-463: Debug logging may be noisy in production.The drain result logging at lines 456-462 uses
eprintln!which will output to stderr in production. Consider gating this behind a debug flag or using a proper logging framework with levels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/commands/webview.rs` around lines 452 - 463, The debug drain logging currently prints to stderr via eprintln! inside the eval_with_result block (guarded by is_drain) which is noisy in production; replace those eprintln! calls with proper leveled logging (e.g., log::debug! or trace! for successful drains and log::error! for errors) or gate them behind cfg!(debug_assertions) if you prefer compile-time stripping—update the branch that checks is_drain (and uses result, val, label) to call the chosen logging macros and keep the same truncated-val behavior (val.chars().take(120).collect::<String>()) when formatting the message.src/features/browser/automation/inject/visual-effects.ts (1)
57-75: Comment/behavior mismatch for full‑viewport frame roundingThe comment says the full‑viewport frame is rounded, but the
elsebranch setsborderRadiusto0. Either update the comment or apply a radius for the full‑viewport case to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/automation/inject/visual-effects.ts` around lines 57 - 75, The comment and implementation disagree: the comment above the if/else says full-viewport should be rounded but the else branch sets frameEl.style.borderRadius = '0'; update the else branch to apply a non-zero radius (e.g., '8px' or a named constant) or change the comment to reflect sharp corners; modify the code referencing frameEl and rect (the if (rect) { ... } else { ... } block) so the visual behavior and comment are consistent.src/features/browser/ui/BrowserTab.tsx (1)
427-444: Prefer typed drain events +.exhaustive()
evt.typeis currentlystring, so.otherwise()will silently swallow new event types. If you model the drain payload as a discriminated union, you can use.exhaustive()for compile‑time coverage.As per coding guidelines "Use `ts-pattern` for switch/case and if/else chains on discriminated unions. Prefer `.exhaustive()` to catch missing cases at compile time."♻️ Example typed drain events
+ type InspectDrainEvent = + | { type: "element-event"; data: ElementSelectedEvent } + | { type: "selection-mode"; data: { active: boolean } }; ... - let events: Array<{ type: string; data: Record<string, unknown> }>; + let events: InspectDrainEvent[]; ... - for (const evt of events) { - match(evt.type) - .with("element-event", () => { - const parsed = evt.data as unknown as ElementSelectedEvent; - onElementSelectedRef.current?.(tabId, parsed); - onAddLog( - tabId, - "info", - parsed.type === "area-selected" - ? `Area selected: ${parsed.bounds?.width}\u00d7${parsed.bounds?.height}` - : `Element selected: ${parsed.element?.tagName}${parsed.reactComponent ? ` (${parsed.reactComponent.name})` : ""}` - ); - }) - .with("selection-mode", () => { - const modeData = evt.data as { active: boolean }; - onUpdateTab(tabId, { selectorActive: modeData.active }); - }) - .otherwise(() => {}); - } + for (const evt of events) { + match(evt) + .with({ type: "element-event" }, ({ data }) => { + const parsed = data; + onElementSelectedRef.current?.(tabId, parsed); + onAddLog( + tabId, + "info", + parsed.type === "area-selected" + ? `Area selected: ${parsed.bounds?.width}\u00d7${parsed.bounds?.height}` + : `Element selected: ${parsed.element?.tagName}${parsed.reactComponent ? ` (${parsed.reactComponent.name})` : ""}` + ); + }) + .with({ type: "selection-mode" }, ({ data }) => { + const modeData = data; + onUpdateTab(tabId, { selectorActive: modeData.active }); + }) + .exhaustive(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/ui/BrowserTab.tsx` around lines 427 - 444, The event handling loop in BrowserTab.tsx currently treats evt.type as string and uses .otherwise(), which can silently swallow new event types; define a discriminated union type (e.g., DrainEvent = { type: "element-event"; data: ElementSelectedEvent } | { type: "selection-mode"; data: { active: boolean } } | ...) and ensure the incoming events array is typed as DrainEvent[] (or cast where produced), then update the match(...) call to use .with(...) for each union member and end with .exhaustive() instead of .otherwise(); keep existing handlers (onElementSelectedRef.current, onAddLog, onUpdateTab) but remove the fallback branch so the compiler will force handling of new event variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/browser/automation/inject/inspect-mode.ts`:
- Around line 234-263: getMatchedVarDeclarations currently only scans color
properties so any var(...) used in padding or gap (which are listed in
localProps) never get emitted; update the function to scan the same set as
localProps by (1) renaming colorProps to something like propsToScan and adding
'padding' and 'gap' to that array, (2) ensuring the fallback emission locations
that currently emit 'font-size'/'border-radius' also include 'padding' and 'gap'
(the other similar branch around lines 765-783), and (3) keep the
varDeclCache/VAR_CACHE_TTL logic unchanged; this aligns
getMatchedVarDeclarations, localProps, and the fallback so var(...) tokens for
padding/gap are captured and sent.
In `@src/features/browser/ui/BrowserPanel.tsx`:
- Around line 906-916: The click handler updates devtoolsOpen synchronously
before the async invoke call completes, causing UI state to drift if invoke
fails; change the handler to perform the invoke first and only call
handleUpdateTab(activeTab.id, { devtoolsOpen: !activeTab.devtoolsOpen }) after
the promise resolves successfully (or in a .then), and on rejection call
handleAddLog(activeTab.id, "error", ...) without toggling the state; reference
the existing invoke calls for "open_browser_devtools"/"close_browser_devtools",
activeTab, handleAddLog, and handleUpdateTab when relocating the update into the
success path (or convert the onClick to an async function with try/catch and
update state after await).
In `@src/features/browser/ui/BrowserTab.tsx`:
- Around line 524-531: The current injection success path sets
automationInjectedRef.current and calls onUpdateTab(tabId, { injected: true })
but does not clear a previous error flag; ensure you also reset the stale error
state by calling onUpdateTab(tabId, { injectionFailed: false }) (either
alongside setting injected or immediately before attempting injection) so that
injectionFailed is cleared after a successful inject; reference
automationInjectedRef.current, onUpdateTab(tabId, {...}), and the
injectionFailed flag when making the change.
- Around line 324-388: The current setInterval-based console drain inside the
useEffect (CONSOLE_DRAIN_JS, invoke("eval_browser_webview_with_result"),
CONSOLE_DRAIN_INTERVAL_MS) must be replaced with an event-driven subscription:
have the Rust/Tauri side perform the periodic read/JSONify of
window.__HIVE_LOGS__ (or centralize the polling there) and emit a Tauri event
(e.g. "browser-console-logs" with payload {label, logs}) that this component
listens for; in BrowserTab.tsx subscribe to that event, filter by
webviewLabel/tabId, parse the payload into the same logs shape, call
onAddLog(tabId, level, message) and reset any local failure counters, and remove
the event listener in the cleanup—if centralizing to Rust is impossible,
collapse polling into a single shared service module invoked here and document
why polling remains, but do not keep per-component setInterval polling.
- Around line 247-256: The safeListen function currently calls listen<T>(...)
without a rejection handler, risking unhandled promise rejections and missing
cleanup; update safeListen (reference: safeListen, aborted, unsubs, listen,
unsub) to append a .catch(...) to the promise returned by listen so any error is
handled (log or swallow as appropriate) and ensure that when listen rejects you
do not push an unsub into unsubs and optionally record the failure for
diagnostics; keep the existing aborted check behavior intact.
---
Outside diff comments:
In `@sidecar/agents/hive-tools/browser.ts`:
- Around line 848-859: The injected DOM query in
sidecar/agents/hive-tools/browser.ts is still looking for data-cursor-ref and
should be changed to data-hive-ref: update the selector string in the injected
script (the snippet that declares var el =
document.querySelector('[data-cursor-ref="${args.ref}"]')) to use
'[data-hive-ref="${args.ref}"]' and keep the rest of the bounding-box logic
unchanged; also update any related error text if it mentions data-cursor-ref.
Additionally, update the protocol descriptions in sidecar/protocol.ts that
reference data-cursor-ref so they instead mention data-hive-ref to match the
frontend automation scripts (visual-effects.ts, browser-utils.ts,
inspect-mode.ts).
---
Nitpick comments:
In `@sidecar/agents/hive-tools/browser.ts`:
- Around line 206-209: The tool descriptions for BrowserClick, BrowserType,
BrowserEvaluate, BrowserHover, and BrowserSelectOption reference the attribute
name "data-cursor-ref" but the injected scripts use "data-hive-ref"; run the
verification script referenced in the prior PR comment to confirm the actual
attribute, then update each tool's .describe(...) text to reference the
confirmed attribute ("data-hive-ref" if verified) so the descriptions match the
implementation and avoid confusing the agent.
In `@src-tauri/src/commands/webview.rs`:
- Around line 452-463: The debug drain logging currently prints to stderr via
eprintln! inside the eval_with_result block (guarded by is_drain) which is noisy
in production; replace those eprintln! calls with proper leveled logging (e.g.,
log::debug! or trace! for successful drains and log::error! for errors) or gate
them behind cfg!(debug_assertions) if you prefer compile-time stripping—update
the branch that checks is_drain (and uses result, val, label) to call the chosen
logging macros and keep the same truncated-val behavior
(val.chars().take(120).collect::<String>()) when formatting the message.
In `@src/features/browser/automation/build-inject.ts`:
- Around line 23-26: The filter for entryPoints is redundantly checking f !==
"tsconfig.json"; update the entryPoints creation (the readdirSync/filter/map
block using injectDir and entryPoints) to remove that unnecessary check and only
filter for TypeScript files while excluding declaration files (e.g., keep the
f.endsWith(".ts") && !f.endsWith(".d.ts") condition or equivalent), so
tsconfig.json is not separately tested.
In `@src/features/browser/automation/inject/visual-effects.ts`:
- Around line 57-75: The comment and implementation disagree: the comment above
the if/else says full-viewport should be rounded but the else branch sets
frameEl.style.borderRadius = '0'; update the else branch to apply a non-zero
radius (e.g., '8px' or a named constant) or change the comment to reflect sharp
corners; modify the code referencing frameEl and rect (the if (rect) { ... }
else { ... } block) so the visual behavior and comment are consistent.
In `@src/features/browser/ui/BrowserPanel.tsx`:
- Around line 473-493: The catch block in handleScreenshot only logs errors to
console and gives no user feedback; update the catch to also surface a
user-facing toast/notification (e.g., call an existing showToast/toast utility
or dispatch a CustomEvent like "show-toast" or "insert-toast") when
invoke("screenshot_browser_webview", ...) fails, include a friendly message plus
the error details (err.message) and keep the existing console.error for
debugging; modify the handleScreenshot catch to trigger that toast/notification
so users know the screenshot failed.
In `@src/features/browser/ui/BrowserTab.tsx`:
- Around line 427-444: The event handling loop in BrowserTab.tsx currently
treats evt.type as string and uses .otherwise(), which can silently swallow new
event types; define a discriminated union type (e.g., DrainEvent = { type:
"element-event"; data: ElementSelectedEvent } | { type: "selection-mode"; data:
{ active: boolean } } | ...) and ensure the incoming events array is typed as
DrainEvent[] (or cast where produced), then update the match(...) call to use
.with(...) for each union member and end with .exhaustive() instead of
.otherwise(); keep existing handlers (onElementSelectedRef.current, onAddLog,
onUpdateTab) but remove the fallback branch so the compiler will force handling
of new event variants.
In `@src/features/session/ui/blocks/TextBlock.tsx`:
- Around line 68-73: The inspectSegments mapping uses the array index as a key
which can cause remounts; update the JSX in TextBlock (where inspectSegments is
mapped and InspectElementPill is rendered) to use a stable key: when segment is
an element, use its existing ref (e.g., segment.ref) as the key for
<InspectElementPill key=...>, otherwise use a stable string-based key (for
example the segment string value or a deterministic id derived from it) instead
of the index so both string spans and InspectElementPill components have stable,
non-index keys.
In `@src/features/session/ui/InspectedElementCard.tsx`:
- Around line 37-41: Update the motion transition duration in the
InspectedElementCard component: change the transition prop on the animated
wrapper (the object with initial/animate/exit/transition) from duration: 0.15 to
duration: 0.2 so the animation aligns with the app's 200–300ms default; keep the
rest of the transition (ease array) unchanged.
Comment on lines
+234
to
+263
| function getMatchedVarDeclarations(el: Element): Record<string, string> { | ||
| const cached = varDeclCache.get(el); | ||
| if (cached && Date.now() - cached.ts < VAR_CACHE_TTL) return cached.decls; | ||
|
|
||
| const decls: Record<string, string> = {}; | ||
| const colorProps = ['background-color', 'color', 'border-color']; | ||
| try { | ||
| const sheets = document.styleSheets; | ||
| for (let si = 0; si < sheets.length; si++) { | ||
| let rules: CSSRuleList; | ||
| try { rules = sheets[si].cssRules; } | ||
| catch (_e) { continue; } // Skip cross-origin stylesheets | ||
| if (!rules) continue; | ||
| for (let ri = 0; ri < rules.length; ri++) { | ||
| const rule = rules[ri] as CSSStyleRule; | ||
| if (!rule.selectorText || !rule.style) continue; | ||
| try { if (!el.matches(rule.selectorText)) continue; } | ||
| catch (_e) { continue; } | ||
| for (let ci = 0; ci < colorProps.length; ci++) { | ||
| const val = rule.style.getPropertyValue(colorProps[ci]); | ||
| if (val && val.indexOf('var(') !== -1) { | ||
| decls[colorProps[ci]] = val.trim(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (_e) { /* swallow */ } | ||
| varDeclCache.set(el, { ts: Date.now(), decls }); | ||
| return decls; | ||
| } |
There was a problem hiding this comment.
padding/gap never emitted in local style payload
In the local context branch, localProps includes padding and gap, but getMatchedVarDeclarations only scans color props and the fallback path only emits font-size/border-radius. As a result, padding/gap never get sent even when they use var(...) tokens. Either remove them from localProps or include them in the var() scan.
🛠️ Align var() scanning with localProps
- const colorProps = ['background-color', 'color', 'border-color'];
+ const tokenProps = ['background-color', 'color', 'border-color', 'padding', 'gap'];
...
- for (let ci = 0; ci < colorProps.length; ci++) {
- const val = rule.style.getPropertyValue(colorProps[ci]);
+ for (let ci = 0; ci < tokenProps.length; ci++) {
+ const val = rule.style.getPropertyValue(tokenProps[ci]);
if (val && val.indexOf('var(') !== -1) {
- decls[colorProps[ci]] = val.trim();
+ decls[tokenProps[ci]] = val.trim();
}
}Also applies to: 765-783
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/browser/automation/inject/inspect-mode.ts` around lines 234 -
263, getMatchedVarDeclarations currently only scans color properties so any
var(...) used in padding or gap (which are listed in localProps) never get
emitted; update the function to scan the same set as localProps by (1) renaming
colorProps to something like propsToScan and adding 'padding' and 'gap' to that
array, (2) ensuring the fallback emission locations that currently emit
'font-size'/'border-radius' also include 'padding' and 'gap' (the other similar
branch around lines 765-783), and (3) keep the varDeclCache/VAR_CACHE_TTL logic
unchanged; this aligns getMatchedVarDeclarations, localProps, and the fallback
so var(...) tokens for padding/gap are captured and sent.
…tency
1. Replace nested ternary on log.l with ts-pattern match() in console
drain loop (BrowserTab.tsx:365). Consistent with inspect event drain
at line 428 which already uses match(). CLAUDE.md requires ts-pattern
for all discriminator field dispatch.
2. Fix contradictory architecture comments across 3 files:
- inject/inspect-mode.ts: Remove "DUAL mechanism: PRIMARY title-channel"
language and phantom serialization queue details. Buffer+drain via
eval_browser_webview_with_result is the sole path.
- BrowserTab.tsx: Change "FALLBACK path" to "sole delivery path".
- webview.rs: Remove reference to non-existent sendViaTitle(), mark
title-channel handler as backward-compat only.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/features/browser/ui/BrowserTab.tsx (2)
519-521:⚠️ Potential issue | 🟡 MinorReset
injectionFailedon successful injectionIf an earlier injection attempt failed,
injectionFailedremainstrueeven after a subsequent successful inject. This leaves the tab in a semantically inconsistent state where bothinjected: trueandinjectionFailed: truecould be set.🛠️ Proposed fix
automationInjectedRef.current = true; - onUpdateTab(tabId, { injected: true }); + onUpdateTab(tabId, { injected: true, injectionFailed: false }); onAddLog(tabId, "info", "Automation scripts injected successfully");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/ui/BrowserTab.tsx` around lines 519 - 521, When marking a tab as successfully injected, also clear the previous failure flag so state isn't contradictory: update the success path that sets automationInjectedRef.current and calls onUpdateTab/tab logging (references: automationInjectedRef, onUpdateTab, onAddLog) to set injected: true and injectionFailed: false (and keep the info log via onAddLog) so a prior injectionFailed flag is reset on successful injection.
251-256:⚠️ Potential issue | 🟡 MinorAdd
.catch()to prevent unhandled promise rejectionsThe
safeListenhelper callslisten()without a rejection handler. While rejections are rare, if one occurs the error will be unhandled and the cleanup list incomplete.🛠️ Proposed fix
function safeListen<T>(event: string, handler: (evt: { payload: T }) => void) { listen<T>(event, handler).then((unsub) => { if (aborted) { unsub(); return; } unsubs.push(unsub); - }); + }).catch((err) => { + if (!aborted) { + console.warn(`[BrowserTab] listen(${event}) failed:`, err); + } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/ui/BrowserTab.tsx` around lines 251 - 256, safeListen calls listen(event, handler) without handling promise rejections which can cause unhandled promise rejections and leave the unsubs cleanup incomplete; update safeListen to append a .catch(...) to the listen<T>(...) promise so rejections are handled (e.g., log via processLogger or console.error and skip pushing the unsub), and ensure any unsubscribe returned in a resolved path is still pushed to unsubs only when not aborted (use the existing aborted check inside the .then handler). Reference: safeListen, listen, unsubs, aborted, unsub.src/features/browser/automation/inject/inspect-mode.ts (1)
226-255:padding/gapvar() tokens still not captured.
getMatchedVarDeclarationsscans onlycolorProps(lines 231, 244–248), but the local context branch at lines 760–774 expectspaddingandgapto be available invarDecls. Since they're not scanned, var() tokens for these properties are silently dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/automation/inject/inspect-mode.ts` around lines 226 - 255, getMatchedVarDeclarations currently only checks colorProps and therefore never captures var() values for layout properties used later (padding/gap); update getMatchedVarDeclarations to include padding and gap (and any relevant shorthand variants if needed) in the properties list checked (replace or extend colorProps) so that decls contains entries for 'padding' and 'gap' when rules use var(), keep existing try/catch and caching behavior tied to varDeclCache and ensure the local-context branch can read varDecls['padding'] and varDecls['gap'] as expected.
🧹 Nitpick comments (2)
src/features/browser/ui/BrowserTab.tsx (1)
324-384: Polling is documented but consider centralizationThe console drain polling at 1.5s intervals is now documented with clear rationale (WKWebView's title-change coalescing makes the title-channel unreliable). The gating on
visible && webviewReady && hasLoadedis appropriate.For future consideration: if multiple BrowserTab instances exist, each runs its own interval. A centralized drain service could batch these to further reduce IPC overhead, but this is optional given the current 1.5s interval.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/ui/BrowserTab.tsx` around lines 324 - 384, Multiple BrowserTab instances each create their own console-drain interval which can duplicate IPC calls; centralize the polling into a single shared drain service (instead of per-Tab useEffect) that uses the same CONSOLE_DRAIN_INTERVAL_MS and invoke("eval_browser_webview_with_result") logic and dispatches parsed logs to BrowserTab via tabId/onAddLog, or at minimum add a TODO and a single shared singleton guard to ensure only one interval runs across tabs; update BrowserTab's useEffect to register/unregister with that central service (references: BrowserTab component, CONSOLE_DRAIN_INTERVAL_MS, invoke("eval_browser_webview_with_result"), onAddLog, tabId).src/features/browser/automation/inject/inspect-mode.ts (1)
84-86: Duplicate comment text.Lines 85–86 both describe the same ForwardRef/Memo handling. Similarly, lines 185–186 both mention the Canvas pixel readback method. One of each can be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/automation/inject/inspect-mode.ts` around lines 84 - 86, Remove the duplicated comment lines that repeat the same descriptions: delete the second occurrence of the "Enhanced React fiber detection — handles ForwardRef, Memo, and elementType (which preserves the original unwrapped type)." comment in src/features/browser/automation/inject/inspect-mode.ts (the React fiber detection comment block) and delete the duplicate "Canvas pixel readback" comment in the Canvas readback section so only one instance of each descriptive comment remains; preserve the original single comment for clarity and leave surrounding code and other comments untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/commands/webview.rs`:
- Around line 425-466: The current eprintln! debug statements in
eval_with_result (including the timeout, with_webview error, and drain logs
guarded by is_drain and js_preview) can leak user data in release builds; change
them to be debug-only: replace direct eprintln! calls with either log::debug!
(or conditional compile/runtime checks like cfg!(debug_assertions)) and redact
or truncate any user-supplied JS/console content (use js_preview and limit drain
output length) before logging so release builds do not print sensitive payloads.
Ensure the with_webview error, timeout log, and the drain returned-data/ERROR
branches all follow this debug-only + redaction pattern so production builds do
not emit raw console/JS content.
- Around line 476-632: The code currently logs when wk or _inspector are null
inside the with_webview closures in open_browser_devtools and
close_browser_devtools but still returns Ok(()), falsely signaling success;
change the closures passed to webview.with_webview in both open_browser_devtools
and close_browser_devtools to return Result<(), String> (or an appropriate Err
type) instead of unit so you can return Err("...") when wk is null or inspector
is null, propagate that error out of with_webview (so the outer map_err can
convert it to the function Result), and adjust the surrounding call sites to
handle the Result from with_webview so the command returns an Err when the
inspector is unavailable. Ensure you reference wk and _inspector checks inside
the with_webview closures and return descriptive error strings like "WKWebView
pointer is null" or "_inspector is null — devtools may be disabled".
In `@src/features/browser/automation/inject/inspect-mode.ts`:
- Around line 757-774: The local context loop (localProps / varDecls -> styles)
currently lists "font-weight" in localProps but only falls back to computed
values for "font-size" and "border-radius"; update the fallback logic in the
loop (where cs.getPropertyValue(prop) is checked) to also handle "font-weight"
(e.g., include prop === 'font-weight' in the conditional and check compVal is
present and not a default like 'normal' or '0') so font-weight is added to
styles when no var() token exists; alternatively, if font-weight should not be
captured, remove 'font-weight' from localProps to keep behavior consistent.
---
Duplicate comments:
In `@src/features/browser/automation/inject/inspect-mode.ts`:
- Around line 226-255: getMatchedVarDeclarations currently only checks
colorProps and therefore never captures var() values for layout properties used
later (padding/gap); update getMatchedVarDeclarations to include padding and gap
(and any relevant shorthand variants if needed) in the properties list checked
(replace or extend colorProps) so that decls contains entries for 'padding' and
'gap' when rules use var(), keep existing try/catch and caching behavior tied to
varDeclCache and ensure the local-context branch can read varDecls['padding']
and varDecls['gap'] as expected.
In `@src/features/browser/ui/BrowserTab.tsx`:
- Around line 519-521: When marking a tab as successfully injected, also clear
the previous failure flag so state isn't contradictory: update the success path
that sets automationInjectedRef.current and calls onUpdateTab/tab logging
(references: automationInjectedRef, onUpdateTab, onAddLog) to set injected: true
and injectionFailed: false (and keep the info log via onAddLog) so a prior
injectionFailed flag is reset on successful injection.
- Around line 251-256: safeListen calls listen(event, handler) without handling
promise rejections which can cause unhandled promise rejections and leave the
unsubs cleanup incomplete; update safeListen to append a .catch(...) to the
listen<T>(...) promise so rejections are handled (e.g., log via processLogger or
console.error and skip pushing the unsub), and ensure any unsubscribe returned
in a resolved path is still pushed to unsubs only when not aborted (use the
existing aborted check inside the .then handler). Reference: safeListen, listen,
unsubs, aborted, unsub.
---
Nitpick comments:
In `@src/features/browser/automation/inject/inspect-mode.ts`:
- Around line 84-86: Remove the duplicated comment lines that repeat the same
descriptions: delete the second occurrence of the "Enhanced React fiber
detection — handles ForwardRef, Memo, and elementType (which preserves the
original unwrapped type)." comment in
src/features/browser/automation/inject/inspect-mode.ts (the React fiber
detection comment block) and delete the duplicate "Canvas pixel readback"
comment in the Canvas readback section so only one instance of each descriptive
comment remains; preserve the original single comment for clarity and leave
surrounding code and other comments untouched.
In `@src/features/browser/ui/BrowserTab.tsx`:
- Around line 324-384: Multiple BrowserTab instances each create their own
console-drain interval which can duplicate IPC calls; centralize the polling
into a single shared drain service (instead of per-Tab useEffect) that uses the
same CONSOLE_DRAIN_INTERVAL_MS and invoke("eval_browser_webview_with_result")
logic and dispatches parsed logs to BrowserTab via tabId/onAddLog, or at minimum
add a TODO and a single shared singleton guard to ensure only one interval runs
across tabs; update BrowserTab's useEffect to register/unregister with that
central service (references: BrowserTab component, CONSOLE_DRAIN_INTERVAL_MS,
invoke("eval_browser_webview_with_result"), onAddLog, tabId).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (3)
src-tauri/src/commands/webview.rssrc/features/browser/automation/inject/inspect-mode.tssrc/features/browser/ui/BrowserTab.tsx
Conflicts resolved: - .claude/agent-memory/code-reviewer/MEMORY.md: Keep both sides (our event bus + XML serialization notes, main's CI/distribution notes) - src/features/browser/ui/BrowserPanel.tsx: Keep our branch's changes (inject button → failure indicator, console panel → native DevTools, simplified tab rendering). Added aria-label from main to DevTools button. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/features/browser/ui/BrowserPanel.tsx (1)
913-923: DevTools state update races with async invoke.The
handleUpdateTabcall on line 922 executes synchronously before the asyncinvoke()completes. If the invoke fails, the UI state (devtoolsOpen) will be out of sync with the actual DevTools state. Move the state update into.then()to ensure it only triggers on success.🔧 Suggested fix
onClick={() => { if (!activeTab?.webviewLabel) return; if (activeTab.devtoolsOpen) { invoke("close_browser_devtools", { label: activeTab.webviewLabel }) + .then(() => handleUpdateTab(activeTab.id, { devtoolsOpen: false })) .catch((err) => handleAddLog(activeTab.id, "error", `Close devtools failed: ${err}`)); } else { invoke("open_browser_devtools", { label: activeTab.webviewLabel }) + .then(() => handleUpdateTab(activeTab.id, { devtoolsOpen: true })) .catch((err) => handleAddLog(activeTab.id, "error", `Open devtools failed: ${err}`)); } - handleUpdateTab(activeTab.id, { devtoolsOpen: !activeTab.devtoolsOpen }); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/ui/BrowserPanel.tsx` around lines 913 - 923, The UI toggles devtoolsOpen immediately after calling invoke, causing a race if the async invoke fails; modify the click handler so that for both branches (open_browser_devtools and close_browser_devtools invoked with activeTab.webviewLabel) you call invoke(...).then(() => handleUpdateTab(activeTab.id, { devtoolsOpen: !activeTab.devtoolsOpen })).catch(err => handleAddLog(activeTab.id, "error", `Open/Close devtools failed: ${err}`)); ensure you still guard on activeTab?.webviewLabel before invoking and only update state inside the .then() callback (do not update on error).
🧹 Nitpick comments (1)
src/features/session/ui/MessageInput.tsx (1)
459-465: Consider stabilizing theonRemovecallback for list items.The inline arrow function
() => removeInspectedElement(el.id)creates a new reference on each render, which can cause unnecessary re-renders ofInspectedElementCardchildren. For small lists this is negligible, but if the list could grow, consider memoizing:♻️ Optional: stabilize callback with useCallback
+ const handleRemoveInspectedElement = useCallback((id: string) => { + setInspectedElements((prev) => prev.filter((el) => el.id !== id)); + }, []); - const removeInspectedElement = (id: string) => { - setInspectedElements((prev) => prev.filter((el) => el.id !== id)); - };Then in the render:
<InspectedElementCard key={el.id} element={el} - onRemove={() => removeInspectedElement(el.id)} + onRemove={() => handleRemoveInspectedElement(el.id)} />Note: The callback still creates an arrow function wrapper. For full optimization,
InspectedElementCardwould need to accept anidprop and callonRemove(id)internally, or useReact.memowith a custom comparator. Given the expected small list size, this is optional.As per coding guidelines: "Wrap components rendered inside
.map()loops withReact.memo()when they receive stable props to prevent entire list re-rendering."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/session/ui/MessageInput.tsx` around lines 459 - 465, The inline arrow passed to InspectedElementCard for onRemove creates a new function each render; stabilize it by creating a memoized callback with useCallback (e.g., a handleRemove that calls removeInspectedElement) and pass that to the mapped children, or change InspectedElementCard to accept an id and a stable onRemove(id) so the parent can pass a single memoized function; also wrap InspectedElementCard with React.memo (and add a shallow/custom comparator if needed) to prevent unnecessary re-renders when inspectedElements items haven’t changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/features/browser/ui/BrowserPanel.tsx`:
- Around line 913-923: The UI toggles devtoolsOpen immediately after calling
invoke, causing a race if the async invoke fails; modify the click handler so
that for both branches (open_browser_devtools and close_browser_devtools invoked
with activeTab.webviewLabel) you call invoke(...).then(() =>
handleUpdateTab(activeTab.id, { devtoolsOpen: !activeTab.devtoolsOpen
})).catch(err => handleAddLog(activeTab.id, "error", `Open/Close devtools
failed: ${err}`)); ensure you still guard on activeTab?.webviewLabel before
invoking and only update state inside the .then() callback (do not update on
error).
---
Nitpick comments:
In `@src/features/session/ui/MessageInput.tsx`:
- Around line 459-465: The inline arrow passed to InspectedElementCard for
onRemove creates a new function each render; stabilize it by creating a memoized
callback with useCallback (e.g., a handleRemove that calls
removeInspectedElement) and pass that to the mapped children, or change
InspectedElementCard to accept an id and a stable onRemove(id) so the parent can
pass a single memoized function; also wrap InspectedElementCard with React.memo
(and add a shallow/custom comparator if needed) to prevent unnecessary
re-renders when inspectedElements items haven’t changed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (10)
.claude/agent-memory/code-reviewer/MEMORY.mdpackage.jsonsrc-tauri/src/main.rssrc/app/layouts/MainLayout.tsxsrc/features/browser/ui/BrowserPanel.tsxsrc/features/session/ui/MessageInput.tsxsrc/features/session/ui/SessionPanel.tsxsrc/features/session/ui/theme/chatTheme.tssrc/features/session/ui/tools/components/BaseToolRenderer.tsxsrc/global.css
✅ Files skipped from review due to trivial changes (1)
- src/features/session/ui/tools/components/BaseToolRenderer.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src-tauri/src/main.rs
- src/global.css
- src/app/layouts/MainLayout.tsx
- src/features/session/ui/SessionPanel.tsx
1. **inject-mode.ts: var() scan now covers padding/gap (#3)** Renamed colorProps → varScanProps, added padding/gap so getMatchedVarDeclarations captures var() tokens for spacing props. 2. **inject-mode.ts: font-weight fallback added (#10)** Added font-weight to the fallback conditional alongside font-size and border-radius. Filters out 'normal' (CSS default) as noise. 3. **BrowserPanel.tsx: DevTools state races with async invoke (#4)** Moved handleUpdateTab into .then() so devtoolsOpen only updates after the Rust command succeeds. Prevents UI desync on failure. 4. **BrowserTab.tsx: safeListen missing .catch() (#5)** Added .catch() to the listen() promise chain to prevent unhandled rejections if Tauri listener registration fails. 5. **BrowserTab.tsx: injectionFailed not cleared on success (#7)** Changed onUpdateTab to also set injectionFailed: false after successful injection, clearing stale error indicators. 6. **webview.rs: eprintln! gated for release builds (#8)** All 4 eprintln! calls in eval_browser_webview_with_result wrapped with cfg!(debug_assertions). Prevents leaking JS content, URLs, tokens, or page console data in production. 7. **webview.rs: DevTools error propagation (#9)** open/close_browser_devtools now use Arc<Mutex<Option<String>>> to propagate null-pointer errors out of with_webview closures. Frontend receives Err() instead of silent Ok(()) when DevTools unavailable. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
browser-templates.test.ts imports browser-utils.ts which reads dist-inject/browser-utils.js via ?raw import. The dist-inject/ directory is gitignored and must be built first. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/browser/ui/BrowserTab.tsx (1)
488-530:⚠️ Potential issue | 🟡 MinorMake verification failures update
injectionFailed, and bail on toggle if injection didn’t succeed.Right now, “verify returned bad status” and “verify threw” paths
returnwithout markinginjectionFailed, which can leave the BrowserPanel indicator inaccurate and make failures harder to see.Proposed diff
const injectAutomation = useCallback(async () => { @@ try { + // Clear stale error state while retrying + onUpdateTab(tabId, { injectionFailed: false }); + // Inject browser utils, visual effects, and inspect mode via native eval await invoke("eval_browser_webview", { label: webviewLabel, js: BROWSER_UTILS_SETUP }); await invoke("eval_browser_webview", { label: webviewLabel, js: INSPECT_MODE_SETUP }); await invoke("eval_browser_webview", { label: webviewLabel, js: VISUAL_EFFECTS_SETUP }); @@ const status = JSON.parse(verifyResult); if (!status.hiveInspect || !status.hasDrainEvents) { onAddLog(tabId, "error", `Inspect mode setup incomplete: ${JSON.stringify(status)}` ); + onUpdateTab(tabId, { injectionFailed: true, injected: false }); return; // Don't mark as injected } } catch (verifyErr) { onAddLog(tabId, "warn", `Inspect verification failed: ${verifyErr instanceof Error ? verifyErr.message : String(verifyErr)}` ); + onUpdateTab(tabId, { injectionFailed: true, injected: false }); return; } @@ const toggleElementSelector = useCallback(async () => { @@ if (!automationInjectedRef.current) { await injectAutomation(); } + if (!automationInjectedRef.current) return;Also applies to: 539-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/ui/BrowserTab.tsx` around lines 488 - 530, The verify-failure branches inside injectAutomation currently return without marking the tab as failed; update both the "bad status" branch (after parsing verifyResult) and the verification catch block to call onUpdateTab(tabId, { injectionFailed: true }) and log appropriately via onAddLog before returning, ensure automationInjectedRef.current remains false, and ensure any toggle logic that relies on injected state checks automationInjectedRef/current or the updated tab.injected/injectionFailed so toggling bails when injection didn’t succeed; reference injectAutomation, automationInjectedRef, onUpdateTab, onAddLog, tabId, and the verifyResult/status verification and verifyErr catch paths.
♻️ Duplicate comments (2)
src-tauri/src/commands/webview.rs (2)
425-472: Past review concern addressed.The logging is now properly gated with
cfg!(debug_assertions), preventing sensitive data from leaking in release builds. The truncation limits (80 chars for JS preview, 120 chars for drain output) are reasonable for diagnostics without flooding logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/commands/webview.rs` around lines 425 - 472, No code change required: the previous concern about leaking sensitive data is already addressed in eval_with_result by gating all debug logs with cfg!(debug_assertions), truncating js into js_preview and drain output via is_drain, and returning result; simply leave eval_with_result, eval_js_wkwebview, with_webview, js_preview and is_drain as implemented.
550-603: Past review concern addressed.The error holder pattern (
Arc<Mutex<Option<String>>>) now correctly propagates failures whenwkor_inspectorare null, returningErrinstead of silently succeeding withOk(()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/commands/webview.rs` around lines 550 - 603, The previous silent-failure issue is now handled by using err_holder (Arc<Mutex<Option<String>>>) inside the webview.with_webview closure to store errors when wk or inspector pointers are null; ensure the closure still sets err_inner.lock().unwrap().replace(...) for any early-return error paths (e.g., null wk or _inspector) and that after with_webview returns you check err_holder.lock().unwrap().take() and propagate it as Err from the surrounding function; keep the existing map_err on with_webview to convert webview access errors and ensure no code path bypasses setting the err_holder before returning Ok.
🧹 Nitpick comments (5)
src-tauri/src/commands/webview.rs (1)
558-566: Consider extracting CGRect/CGSize/CGPoint to a shared module.These C-compatible structs are defined identically here and in
screenshot_wkwebview(lines 844-852). Extracting them to a common location would reduce duplication and ensure consistency.♻️ Suggested extraction
Create a shared module or define at module scope:
// At module level or in a shared macos_ffi module #[cfg(target_os = "macos")] mod macos_types { #[repr(C)] #[derive(Clone, Copy)] pub struct CGSize { pub width: f64, pub height: f64 } #[repr(C)] #[derive(Clone, Copy)] pub struct CGPoint { pub x: f64, pub y: f64 } #[repr(C)] #[derive(Clone, Copy)] pub struct CGRect { pub origin: CGPoint, pub size: CGSize } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/commands/webview.rs` around lines 558 - 566, The CGRect/CGSize/CGPoint C-compatible struct definitions are duplicated in webview.rs and in screenshot_wkwebview; extract them into a shared module (e.g., macos_types) and make the structs public so both webview.rs and screenshot_wkwebview reference the single definitions (CGSize, CGPoint, CGRect) to remove duplication and guarantee consistency; update the local uses to import the shared module (use crate::macos_types::{CGSize, CGPoint, CGRect} or equivalent) and remove the duplicate local struct declarations.src/features/browser/ui/BrowserPanel.tsx (1)
473-493: Consider simplifying base64 → Blob conversion (optional) and handling possible data-URL prefixes.The manual
atob+ loop is fine, butfetch(data:)is shorter and less error-prone for large payloads.Proposed diff
const handleScreenshot = useCallback(async () => { if (!activeTab?.webviewLabel || !activeTab.currentUrl) return; try { const base64 = await invoke<string>("screenshot_browser_webview", { label: activeTab.webviewLabel, }); - const binaryStr = atob(base64); - const bytes = new Uint8Array(binaryStr.length); - for (let i = 0; i < binaryStr.length; i++) { - bytes[i] = binaryStr.charCodeAt(i); - } - const blob = new Blob([bytes], { type: "image/jpeg" }); + const raw = base64.startsWith("data:") ? base64 : `data:image/jpeg;base64,${base64}`; + const blob = await fetch(raw).then((r) => r.blob()); const file = new File([blob], `browser-screenshot-${Date.now()}.jpg`, { type: "image/jpeg" }); window.dispatchEvent( new CustomEvent("insert-to-chat", { detail: { files: [file] } }) ); } catch (err) { console.error("Browser screenshot failed:", err); } }, [activeTab?.webviewLabel, activeTab?.currentUrl]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/ui/BrowserPanel.tsx` around lines 473 - 493, handleScreenshot uses atob + manual byte loop to convert the base64 string to a Blob; instead, detect and strip any data URL prefix from the returned string (e.g., "data:image/jpeg;base64,") then create a data: URL and use fetch(dataUrl).then(res => res.blob()) to get the Blob, build the File and dispatch the CustomEvent; update the code around invoke("screenshot_browser_webview") in handleScreenshot to strip prefixes, replace the atob/Uint8Array loop with the fetch-based conversion, and keep using the same File creation and window.dispatchEvent logic.src/features/browser/automation/inject/inspect-mode.ts (3)
226-255: Optional: scanvar(...)for the same properties you might want to preserve as tokens.
localPropsincludesfont-size,font-weight, andborder-radius, butvarScanPropscurrently doesn’t—so if those are tokenized (e.g.var(--radius-sm)), you’ll lose the token name and only ever emit computed fallback values.Proposed diff
function getMatchedVarDeclarations(el: Element): Record<string, string> { @@ - const varScanProps = ['background-color', 'color', 'border-color', 'padding', 'gap']; + const varScanProps = [ + 'background-color', + 'color', + 'border-color', + 'padding', + 'gap', + 'font-size', + 'font-weight', + 'border-radius', + ];Also applies to: 757-774
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/automation/inject/inspect-mode.ts` around lines 226 - 255, getMatchedVarDeclarations currently scans only ['background-color','color','border-color','padding','gap'] (varScanProps) for CSS var(...) usages, so tokenized properties like font-size, font-weight, and border-radius from localProps will be lost; update varScanProps to include 'font-size','font-weight','border-radius' (and any other entries from localProps you need preserved), so getMatchedVarDeclarations (and the other occurrence around lines 757-774) will capture var(...) for those properties before emitting computed values, and keep caching via varDeclCache as-is.
522-547: Makemousemovelistener capture-phase too (more robust against page handlers).Right now
mousedown/mouseup/click/keydownare capture-phase, butmousemoveisn’t. If the page aggressively stops propagation, hover tracking can become flaky.Proposed diff
document.addEventListener('mousedown', handleMouseDown, true); - document.addEventListener('mousemove', handleMouseMove); + document.addEventListener('mousemove', handleMouseMove, true); document.addEventListener('mouseup', handleMouseUp, true); document.addEventListener('click', handleClick, true); document.addEventListener('keydown', handleKeyDown, true); @@ document.removeEventListener('mousedown', handleMouseDown, true); - document.removeEventListener('mousemove', handleMouseMove); + document.removeEventListener('mousemove', handleMouseMove, true); document.removeEventListener('mouseup', handleMouseUp, true); document.removeEventListener('click', handleClick, true); document.removeEventListener('keydown', handleKeyDown, true);Also applies to: 581-588
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/automation/inject/inspect-mode.ts` around lines 522 - 547, The mousemove listener is currently added/removed without capture which can break hover tracking if page handlers stop propagation; update the addEventListener and removeEventListener calls for handleMouseMove to use capture=true (match mousedown/mouseup/click/keydown) in both the enable/disable selection-mode blocks (refer to the functions/blocks where document.addEventListener('mousemove', handleMouseMove) and document.removeEventListener('mousemove', handleMouseMove) are called — also apply the same change to the second occurrence around the 581-588 region) so the mousemove handler runs in the capture phase.
510-519: Avoidtransition: allon overlays that constantly mutateleft/top/width/height.Because you update geometry on every mousemove,
transition:allcan cause jank (and animates layout-affecting properties unintentionally).Proposed diff
overlay = document.createElement('div'); overlay.setAttribute('data-hive-inspect', 'true'); - overlay.style.cssText = 'position:fixed;background:rgba(58,150,221,0.3);border:2px solid `#3a96dd`;pointer-events:none;z-index:2147483647;transition:all 0.1s ease;display:none;'; + overlay.style.cssText = 'position:fixed;background:rgba(58,150,221,0.3);border:2px solid `#3a96dd`;pointer-events:none;z-index:2147483647;display:none;'; document.body.appendChild(overlay); overlayLabel = document.createElement('div'); overlayLabel.setAttribute('data-hive-inspect', 'true'); - overlayLabel.style.cssText = 'position:fixed;background:`#3a96dd`;color:white;padding:2px 6px;font-size:11px;font-family:system-ui,-apple-system,sans-serif;font-weight:500;border-radius:2px;pointer-events:none;z-index:2147483648;transition:all 0.1s ease;white-space:nowrap;display:none;'; + overlayLabel.style.cssText = 'position:fixed;background:`#3a96dd`;color:white;padding:2px 6px;font-size:11px;font-family:system-ui,-apple-system,sans-serif;font-weight:500;border-radius:2px;pointer-events:none;z-index:2147483648;white-space:nowrap;display:none;'; document.body.appendChild(overlayLabel);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/browser/automation/inject/inspect-mode.ts` around lines 510 - 519, The overlay and overlayLabel DOM nodes (variables overlay and overlayLabel) use "transition:all 0.1s ease" which causes jank because you update left/top/width/height on every mousemove; remove or replace that with a non-layout transition (e.g., "transition:opacity 0.1s ease" or no transition) to avoid animating layout properties. Update the overlay.style.cssText and overlayLabel.style.cssText assignments in inspect-mode.ts to remove "transition:all 0.1s ease" (or change it to a specific property like opacity) so geometry updates are instantaneous and no layout properties are animated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/browser/automation/inject/inspect-mode.ts`:
- Around line 368-471: getReactProps and getFilteredAttributes/ATTR_WHITELIST
currently capture potentially sensitive values (props like strings, attributes
such as value/href/src, and any innerHTML) which can leak PII/Secrets; update
getReactProps to skip/redact any keys named "value", "password", "token", or
keys matching /(secret|password|token|csrf)/i and only serialize non-sensitive
props when context === "local" (or a provided safe flag), remove "value" from
ATTR_WHITELIST and ensure getFilteredAttributes redacts href/src/value by
replacing with "[REDACTED]" or omitting them unless context === "local", and
ensure any innerHTML capture is either removed or strictly gated to local-only
capture to prevent accidental leakage (refer to functions getReactProps,
ATTR_WHITELIST, and getFilteredAttributes for where to apply these changes).
- Around line 84-86: Remove the duplicated comment blocks about "Enhanced React
fiber detection — handles ForwardRef, Memo, and elementType" and the duplicate
canvas note in inspect-mode.ts; keep a single clear instance of each comment
(the one that best describes the logic) and delete the accidental paste
duplicates (also remove the repeated lines around the later occurrence
referenced near 184-186) so the inline documentation doesn't repeat or rot.
In `@src/features/browser/ui/BrowserPanel.tsx`:
- Around line 775-781: The visual-only error dot (rendered when
activeTab?.injectionFailed) is not screen-reader accessible and the icon-only
screenshot button lacks an aria-label; update the injection-failure span to be
aria-hidden="true" and add an adjacent visually-hidden status element (e.g., a
span with a screen-reader-only class reading "Automation injection failed") or
alternatively give the wrapper a role="status" + readable text, and update the
icon-only screenshot button element to include a descriptive aria-label (e.g.,
aria-label="Take screenshot" or similar) so both the error state and the button
are exposed to assistive tech; locate these changes in the BrowserPanel
component where the injectionFailed span and the screenshot button are rendered.
In `@src/features/browser/ui/BrowserTab.tsx`:
- Around line 328-388: The interval handler in the useEffect for console drain
can start overlapping async invokes (invoke("eval_browser_webview_with_result"))
when a previous call is still pending; add an in-flight guard (e.g., let
consoleDrainInFlight = false scoped in the effect) and at the top of the
setInterval callback return immediately if consoleDrainInFlight is true, set it
true just before calling invoke and set it false in finally so only one eval
runs at a time; apply the same in-flight guard pattern to the other drain effect
(the one around the other interval at 395-457) and keep existing
consoleDrainFails/CONSOLE_DRAIN_JS logic unchanged.
- Around line 88-91: Update the stale comment above the onElementSelectedRef
declaration: remove the reference to “title-channel listener + drain” and
replace it with a brief, accurate description that matches the current
implementation (e.g., mention that inspect-mode events are delivered via
buffer+drain). Locate the comment near the useRef/onElementSelected lines
(onElementSelectedRef and onElementSelected) in BrowserTab.tsx and make the text
reflect the buffer+drain delivery mechanism only.
---
Outside diff comments:
In `@src/features/browser/ui/BrowserTab.tsx`:
- Around line 488-530: The verify-failure branches inside injectAutomation
currently return without marking the tab as failed; update both the "bad status"
branch (after parsing verifyResult) and the verification catch block to call
onUpdateTab(tabId, { injectionFailed: true }) and log appropriately via onAddLog
before returning, ensure automationInjectedRef.current remains false, and ensure
any toggle logic that relies on injected state checks
automationInjectedRef/current or the updated tab.injected/injectionFailed so
toggling bails when injection didn’t succeed; reference injectAutomation,
automationInjectedRef, onUpdateTab, onAddLog, tabId, and the verifyResult/status
verification and verifyErr catch paths.
---
Duplicate comments:
In `@src-tauri/src/commands/webview.rs`:
- Around line 425-472: No code change required: the previous concern about
leaking sensitive data is already addressed in eval_with_result by gating all
debug logs with cfg!(debug_assertions), truncating js into js_preview and drain
output via is_drain, and returning result; simply leave eval_with_result,
eval_js_wkwebview, with_webview, js_preview and is_drain as implemented.
- Around line 550-603: The previous silent-failure issue is now handled by using
err_holder (Arc<Mutex<Option<String>>>) inside the webview.with_webview closure
to store errors when wk or inspector pointers are null; ensure the closure still
sets err_inner.lock().unwrap().replace(...) for any early-return error paths
(e.g., null wk or _inspector) and that after with_webview returns you check
err_holder.lock().unwrap().take() and propagate it as Err from the surrounding
function; keep the existing map_err on with_webview to convert webview access
errors and ensure no code path bypasses setting the err_holder before returning
Ok.
---
Nitpick comments:
In `@src-tauri/src/commands/webview.rs`:
- Around line 558-566: The CGRect/CGSize/CGPoint C-compatible struct definitions
are duplicated in webview.rs and in screenshot_wkwebview; extract them into a
shared module (e.g., macos_types) and make the structs public so both webview.rs
and screenshot_wkwebview reference the single definitions (CGSize, CGPoint,
CGRect) to remove duplication and guarantee consistency; update the local uses
to import the shared module (use crate::macos_types::{CGSize, CGPoint, CGRect}
or equivalent) and remove the duplicate local struct declarations.
In `@src/features/browser/automation/inject/inspect-mode.ts`:
- Around line 226-255: getMatchedVarDeclarations currently scans only
['background-color','color','border-color','padding','gap'] (varScanProps) for
CSS var(...) usages, so tokenized properties like font-size, font-weight, and
border-radius from localProps will be lost; update varScanProps to include
'font-size','font-weight','border-radius' (and any other entries from localProps
you need preserved), so getMatchedVarDeclarations (and the other occurrence
around lines 757-774) will capture var(...) for those properties before emitting
computed values, and keep caching via varDeclCache as-is.
- Around line 522-547: The mousemove listener is currently added/removed without
capture which can break hover tracking if page handlers stop propagation; update
the addEventListener and removeEventListener calls for handleMouseMove to use
capture=true (match mousedown/mouseup/click/keydown) in both the enable/disable
selection-mode blocks (refer to the functions/blocks where
document.addEventListener('mousemove', handleMouseMove) and
document.removeEventListener('mousemove', handleMouseMove) are called — also
apply the same change to the second occurrence around the 581-588 region) so the
mousemove handler runs in the capture phase.
- Around line 510-519: The overlay and overlayLabel DOM nodes (variables overlay
and overlayLabel) use "transition:all 0.1s ease" which causes jank because you
update left/top/width/height on every mousemove; remove or replace that with a
non-layout transition (e.g., "transition:opacity 0.1s ease" or no transition) to
avoid animating layout properties. Update the overlay.style.cssText and
overlayLabel.style.cssText assignments in inspect-mode.ts to remove
"transition:all 0.1s ease" (or change it to a specific property like opacity) so
geometry updates are instantaneous and no layout properties are animated.
In `@src/features/browser/ui/BrowserPanel.tsx`:
- Around line 473-493: handleScreenshot uses atob + manual byte loop to convert
the base64 string to a Blob; instead, detect and strip any data URL prefix from
the returned string (e.g., "data:image/jpeg;base64,") then create a data: URL
and use fetch(dataUrl).then(res => res.blob()) to get the Blob, build the File
and dispatch the CustomEvent; update the code around
invoke("screenshot_browser_webview") in handleScreenshot to strip prefixes,
replace the atob/Uint8Array loop with the fetch-based conversion, and keep using
the same File creation and window.dispatchEvent logic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (4)
src-tauri/src/commands/webview.rssrc/features/browser/automation/inject/inspect-mode.tssrc/features/browser/ui/BrowserPanel.tsxsrc/features/browser/ui/BrowserTab.tsx
Comment on lines
+84
to
+86
| // Enhanced React fiber detection — handles ForwardRef, Memo, and | ||
| // elementType (which preserves the original unwrapped type). | ||
| // Handles ForwardRef, Memo, and elementType (which preserves the original unwrapped type). |
There was a problem hiding this comment.
Remove duplicated comment lines (they’ll rot quickly).
There are two duplicated comment blocks (React fiber header + canvas note) that look like accidental paste.
Proposed diff
// ========================================================================
// React Fiber Detection
// ========================================================================
// Enhanced React fiber detection — handles ForwardRef, Memo, and
// elementType (which preserves the original unwrapped type).
- // Handles ForwardRef, Memo, and elementType (which preserves the original unwrapped type).
interface ReactComponentInfo {
componentName: string | null;
fileName: string | null;
lineNumber: number | null;
}
@@
// ========================================================================
// Color Conversion via Canvas API
// ========================================================================
// Converts modern color formats (oklch, oklab, etc.) to hex using
// 1x1 Canvas pixel readback — the only reliable cross-browser method.
- // Uses 1x1 Canvas pixel readback — the only reliable cross-browser method.
let colorCanvas: HTMLCanvasElement | null = null;
let colorCtx: CanvasRenderingContext2D | null = null;Based on learnings: “Documentation lives IN the code, not separate markdown files. Add inline comments for complex logic…”
Also applies to: 184-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/browser/automation/inject/inspect-mode.ts` around lines 84 - 86,
Remove the duplicated comment blocks about "Enhanced React fiber detection —
handles ForwardRef, Memo, and elementType" and the duplicate canvas note in
inspect-mode.ts; keep a single clear instance of each comment (the one that best
describes the logic) and delete the accidental paste duplicates (also remove the
repeated lines around the later occurrence referenced near 184-186) so the
inline documentation doesn't repeat or rot.
Comment on lines
+368
to
+471
| function getReactProps(el: Element): Record<string, string> | null { | ||
| try { | ||
| const keys = Object.keys(el); | ||
| let fiberKey: string | null = null; | ||
| for (let i = 0; i < keys.length; i++) { | ||
| if (keys[i].indexOf('__reactFiber') === 0 || keys[i].indexOf('__reactInternalInstance') === 0) { | ||
| fiberKey = keys[i]; | ||
| break; | ||
| } | ||
| } | ||
| if (!fiberKey) return null; | ||
|
|
||
| let fiber = (el as any)[fiberKey]; | ||
| // Walk up to find the first user component fiber (skip host elements) | ||
| while (fiber) { | ||
| const type = fiber.elementType || fiber.type; | ||
| if (type && typeof type !== 'string') { | ||
| // Found a component fiber (function/class component, not a host element) | ||
| const name = typeof type === 'function' | ||
| ? (type.displayName || type.name) | ||
| : (type.render?.displayName || type.render?.name || type.type?.displayName || type.type?.name); | ||
| if (name) break; | ||
| } | ||
| fiber = fiber.return; | ||
| } | ||
| if (!fiber) return null; | ||
|
|
||
| const raw = fiber.memoizedProps || fiber.pendingProps; | ||
| if (!raw || typeof raw !== 'object') return null; | ||
|
|
||
| const result: Record<string, string> = {}; | ||
| let totalLen = 0; | ||
| const MAX_LEN = 500; | ||
|
|
||
| for (const key of Object.keys(raw)) { | ||
| if (totalLen >= MAX_LEN) break; | ||
| if (SKIP_PROP_KEYS.indexOf(key) !== -1) continue; | ||
| if (key.startsWith('on') || key.startsWith('__')) continue; // Skip event handlers and internals | ||
|
|
||
| const val = raw[key]; | ||
| const t = typeof val; | ||
|
|
||
| if (t === 'string') { | ||
| if (val.length > 100) continue; | ||
| result[key] = val; | ||
| totalLen += key.length + val.length + 3; | ||
| } else if (t === 'number' || t === 'boolean') { | ||
| const s = String(val); | ||
| result[key] = s; | ||
| totalLen += key.length + s.length + 3; | ||
| } else if (val === null || val === undefined) { | ||
| // skip nullish | ||
| } else if (t === 'object' && !Array.isArray(val)) { | ||
| try { | ||
| const json = JSON.stringify(val); | ||
| if (json.length <= 80) { | ||
| result[key] = json; | ||
| totalLen += key.length + json.length + 3; | ||
| } | ||
| } catch (_) { /* skip non-serializable */ } | ||
| } | ||
| // Skip functions, arrays, symbols | ||
| } | ||
|
|
||
| return Object.keys(result).length > 0 ? result : null; | ||
| } catch (_e) { return null; } | ||
| } | ||
|
|
||
| // ======================================================================== | ||
| // Filtered HTML Attributes | ||
| // ======================================================================== | ||
| // Whitelist of HTML attributes useful for AI context — identifiers, state, | ||
| // accessibility. Skips class/style (redundant with other fields). | ||
| const ATTR_WHITELIST = [ | ||
| 'id', 'data-testid', 'data-test-id', 'href', 'src', 'alt', | ||
| 'type', 'name', 'placeholder', 'disabled', 'checked', | ||
| 'role', 'aria-label', 'aria-expanded', 'aria-hidden', | ||
| 'value', 'action', 'for', 'target', 'required', 'readonly', | ||
| 'min', 'max', 'pattern', 'method', | ||
| ]; | ||
|
|
||
| function getFilteredAttributes(el: Element): Record<string, string> | null { | ||
| const result: Record<string, string> = {}; | ||
| for (let i = 0; i < ATTR_WHITELIST.length; i++) { | ||
| const attr = ATTR_WHITELIST[i]; | ||
| const val = el.getAttribute(attr); | ||
| if (val !== null && val !== '') { | ||
| result[attr] = val.length > 100 ? val.substring(0, 100) : val; | ||
| } | ||
| } | ||
| // Also grab data-* attrs that look like test IDs or state | ||
| const attrs = el.attributes; | ||
| if (attrs) { | ||
| for (let i = 0; i < attrs.length; i++) { | ||
| const name = attrs[i].name; | ||
| if (name.startsWith('data-') && !result[name] && name !== 'data-hive-ref' && name !== 'data-hive-inspect') { | ||
| if (name.indexOf('test') !== -1 || name.indexOf('state') !== -1 || name.indexOf('variant') !== -1 || name.indexOf('status') !== -1) { | ||
| result[name] = attrs[i].value.substring(0, 100); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return Object.keys(result).length > 0 ? result : null; | ||
| } |
There was a problem hiding this comment.
Sensitive data risk: props/attributes/innerHTML can leak secrets/PII into the app (and then chat/AI context).
Current capture includes:
getReactProps()(can include user data / tokens),ATTR_WHITELISTincludesvalue,href,src, etc.,innerHTMLcan include hidden inputs/CSRF tokens (even when truncated).
At minimum, consider redacting obviously risky fields and/or gating “deep capture” to context === "local" only.
Low-friction mitigation (gate props + innerHTML to local; drop `value` attribute)
const ATTR_WHITELIST = [
'id', 'data-testid', 'data-test-id', 'href', 'src', 'alt',
'type', 'name', 'placeholder', 'disabled', 'checked',
'role', 'aria-label', 'aria-expanded', 'aria-hidden',
- 'value', 'action', 'for', 'target', 'required', 'readonly',
+ 'action', 'for', 'target', 'required', 'readonly',
'min', 'max', 'pattern', 'method',
];
@@
- const reactProps = getReactProps(element);
- const htmlAttrs = getFilteredAttributes(element);
- const innerHTML = getShallowInnerHTML(element);
+ const reactProps = context === 'local' ? getReactProps(element) : null;
+ const htmlAttrs = getFilteredAttributes(element);
+ const innerHTML = context === 'local' ? getShallowInnerHTML(element) : null;Also applies to: 800-834
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/browser/automation/inject/inspect-mode.ts` around lines 368 -
471, getReactProps and getFilteredAttributes/ATTR_WHITELIST currently capture
potentially sensitive values (props like strings, attributes such as
value/href/src, and any innerHTML) which can leak PII/Secrets; update
getReactProps to skip/redact any keys named "value", "password", "token", or
keys matching /(secret|password|token|csrf)/i and only serialize non-sensitive
props when context === "local" (or a provided safe flag), remove "value" from
ATTR_WHITELIST and ensure getFilteredAttributes redacts href/src/value by
replacing with "[REDACTED]" or omitting them unless context === "local", and
ensure any innerHTML capture is either removed or strictly gated to local-only
capture to prevent accidental leakage (refer to functions getReactProps,
ATTR_WHITELIST, and getFilteredAttributes for where to apply these changes).
Comment on lines
+775
to
+781
| {/* Injection failure indicator — red dot, only visible on error */} | ||
| {activeTab?.injectionFailed && ( | ||
| <span | ||
| className="bg-destructive h-2 w-2 shrink-0 rounded-full" | ||
| title="Automation injection failed — check console" | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Accessibility: add aria-label for the icon-only screenshot button; make the error dot screen-reader friendly.
- The screenshot button has a
titlebut noaria-label. - The red dot is purely visual; either mark it
aria-hidden+ add a visually-hidden status text, or give it a role/label.
Proposed diff
{activeTab?.injectionFailed && (
- <span
- className="bg-destructive h-2 w-2 shrink-0 rounded-full"
- title="Automation injection failed — check console"
- />
+ <>
+ <span
+ className="bg-destructive h-2 w-2 shrink-0 rounded-full"
+ title="Automation injection failed — check console"
+ aria-hidden="true"
+ />
+ <span className="sr-only">Automation injection failed — check console</span>
+ </>
)}
@@
<Button
variant="ghost"
size="icon"
className="h-7 w-7"
onClick={handleScreenshot}
disabled={!activeTab?.currentUrl}
title="Screenshot to chat"
+ aria-label="Screenshot to chat"
>
<Camera className="h-4 w-4" />
</Button>As per coding guidelines: “Use semantic HTML first… Proper ARIA semantics reduce CSS you need to write.”
Also applies to: 802-812
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/browser/ui/BrowserPanel.tsx` around lines 775 - 781, The
visual-only error dot (rendered when activeTab?.injectionFailed) is not
screen-reader accessible and the icon-only screenshot button lacks an
aria-label; update the injection-failure span to be aria-hidden="true" and add
an adjacent visually-hidden status element (e.g., a span with a
screen-reader-only class reading "Automation injection failed") or alternatively
give the wrapper a role="status" + readable text, and update the icon-only
screenshot button element to include a descriptive aria-label (e.g.,
aria-label="Take screenshot" or similar) so both the error state and the button
are exposed to assistive tech; locate these changes in the BrowserPanel
component where the injectionFailed span and the screenshot button are rendered.
Comment on lines
88
to
91
| // Stable ref for onElementSelected callback (used by title-channel listener + drain) | ||
| const onElementSelectedRef = useRef(onElementSelected); | ||
| onElementSelectedRef.current = onElementSelected; | ||
|
|
There was a problem hiding this comment.
Fix stale comment: “title-channel listener + drain” no longer matches the implementation.
You’ve documented “inspect mode events are delivered solely via buffer+drain” below, so this should not mention title-channel anymore.
Proposed diff
- // Stable ref for onElementSelected callback (used by title-channel listener + drain)
+ // Stable ref for onElementSelected callback (used by the inspect drain loop)
const onElementSelectedRef = useRef(onElementSelected);
onElementSelectedRef.current = onElementSelected;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/browser/ui/BrowserTab.tsx` around lines 88 - 91, Update the
stale comment above the onElementSelectedRef declaration: remove the reference
to “title-channel listener + drain” and replace it with a brief, accurate
description that matches the current implementation (e.g., mention that
inspect-mode events are delivered via buffer+drain). Locate the comment near the
useRef/onElementSelected lines (onElementSelectedRef and onElementSelected) in
BrowserTab.tsx and make the text reflect the buffer+drain delivery mechanism
only.
Comment on lines
328
to
+388
| // --- Periodic console drain (only when visible and webview ready) --- | ||
| // Uses eval_browser_webview_with_result (native completion handler) instead | ||
| // of the old title-channel approach (drain_browser_console). The title-channel | ||
| // suffers from WKWebView's title-change coalescing — when the 60ms restore | ||
| // window overlaps with another title write (SPA nav, page itself), the | ||
| // \x01CL: message is silently dropped and console logs are lost. | ||
| // | ||
| // The completion-handler path reads the log buffer directly and returns the | ||
| // JSON via WKWebView's evaluateJavaScript callback, which is reliable. | ||
| useEffect(() => { | ||
| if (!visible || !webviewReady || !hasLoaded) return; | ||
|
|
||
| const interval = setInterval(() => { | ||
| invoke("drain_browser_console", { label: webviewLabel }).catch(() => {}); | ||
| const CONSOLE_DRAIN_JS = `(function(){ | ||
| var b = window.__HIVE_LOGS__ || []; | ||
| window.__HIVE_LOGS__ = []; | ||
| return JSON.stringify(b); | ||
| })()`; | ||
|
|
||
| // Track consecutive failures for diagnostic logging | ||
| let consoleDrainFails = 0; | ||
|
|
||
| const interval = setInterval(async () => { | ||
| try { | ||
| const result = await invoke<string>("eval_browser_webview_with_result", { | ||
| label: webviewLabel, | ||
| js: CONSOLE_DRAIN_JS, | ||
| timeoutMs: 2000, | ||
| }); | ||
| // Reset on success | ||
| consoleDrainFails = 0; | ||
|
|
||
| if (!result || result === "[]" || result === "undefined") return; | ||
|
|
||
| let logs: Array<{ l: string; m: string; t: number }>; | ||
| try { | ||
| logs = JSON.parse(result); | ||
| } catch (parseErr) { | ||
| console.error("[BrowserTab] console drain: JSON.parse failed", parseErr, "raw:", result.slice(0, 200)); | ||
| return; | ||
| } | ||
| for (const log of logs) { | ||
| const level = match(log.l) | ||
| .with("warn", () => "warn" as const) | ||
| .with("error", () => "error" as const) | ||
| .with("debug", () => "debug" as const) | ||
| .otherwise(() => "info" as const); | ||
| onAddLog(tabId, level, log.m); | ||
| } | ||
| } catch (err) { | ||
| consoleDrainFails++; | ||
| if (consoleDrainFails <= 3 || consoleDrainFails % 50 === 0) { | ||
| console.warn( | ||
| `[BrowserTab] console drain failed (${consoleDrainFails}x):`, | ||
| err instanceof Error ? err.message : String(err) | ||
| ); | ||
| } | ||
| } | ||
| }, CONSOLE_DRAIN_INTERVAL_MS); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }, [visible, webviewReady, hasLoaded, webviewLabel]); | ||
| }, [visible, webviewReady, hasLoaded, webviewLabel, tabId, onAddLog]); |
There was a problem hiding this comment.
Prevent overlapping async drains (can flood IPC when a call is slow).
Both drain effects use setInterval(async () => ...). If invoke(...) takes longer than the interval (especially 200ms inspect drain with 2s timeout), intervals can overlap and pile up concurrent in-flight evals.
Proposed diff (minimal in-flight guard)
useEffect(() => {
if (!visible || !webviewReady || !hasLoaded) return;
@@
// Track consecutive failures for diagnostic logging
let consoleDrainFails = 0;
+ let inFlight = false;
const interval = setInterval(async () => {
+ if (inFlight) return;
+ inFlight = true;
try {
const result = await invoke<string>("eval_browser_webview_with_result", {
label: webviewLabel,
js: CONSOLE_DRAIN_JS,
timeoutMs: 2000,
});
@@
} catch (err) {
consoleDrainFails++;
@@
}
+ finally {
+ inFlight = false;
+ }
}, CONSOLE_DRAIN_INTERVAL_MS);
@@
useEffect(() => {
if (!visible || !webviewReady || !hasLoaded || !tab.selectorActive) return;
@@
// Track consecutive failures to avoid log spam (log first 3, then every 50th)
let failCount = 0;
+ let inFlight = false;
const interval = setInterval(async () => {
+ if (inFlight) return;
+ inFlight = true;
try {
const result = await invoke<string>("eval_browser_webview_with_result", {
label: webviewLabel,
js: INSPECT_MODE_DRAIN_EVENTS,
timeoutMs: 2000,
});
@@
} catch (err) {
failCount++;
@@
}
+ finally {
+ inFlight = false;
+ }
}, INSPECT_DRAIN_MS);Also applies to: 395-457
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/browser/ui/BrowserTab.tsx` around lines 328 - 388, The interval
handler in the useEffect for console drain can start overlapping async invokes
(invoke("eval_browser_webview_with_result")) when a previous call is still
pending; add an in-flight guard (e.g., let consoleDrainInFlight = false scoped
in the effect) and at the top of the setInterval callback return immediately if
consoleDrainInFlight is true, set it true just before calling invoke and set it
false in finally so only one eval runs at a time; apply the same in-flight guard
pattern to the other drain effect (the one around the other interval at 395-457)
and keep existing consoleDrainFails/CONSOLE_DRAIN_JS logic unchanged.
- Remove dead BROWSER_UTILS import (renamed to BROWSER_UTILS_SETUP)
- Remove incorrect buildPressKeyJs BROWSER_UTILS test (it intentionally
doesn't use browser utils — operates on document.activeElement directly)
- Update assertIsIIFE to accept both classic IIFEs (function(){...})()
and arrow IIFEs (() => {...})() — esbuild format: "iife" emits the
latter for compiled inject scripts like VISUAL_EFFECTS_SETUP
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Gate getReactProps() and getShallowInnerHTML() to local context only; external sites can leak PII/tokens via React fiber props and innerHTML - Remove 'value' from ATTR_WHITELIST (can contain passwords) - Add in-flight guards to console and inspect drain intervals to prevent overlapping async invokes when eval_browser_webview is slow - Add aria-hidden + sr-only text for injection failure indicator - Add aria-label to screenshot button - Remove duplicate comment blocks in inject-mode.ts - Fix stale "title-channel" comment in BrowserTab.tsx Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add two new browser panel toolbar features: 1. Screenshot button (Camera icon): captures WKWebView as JPEG via Rust IPC and attaches to chat input via CustomEvent bridge. 2. Mobile/Desktop viewport toggle (Smartphone/Monitor icon): constrains browser area to 390px (iPhone 14 logical width) centered with mx-auto. The mobile view required replacing the tab stacking strategy — absolute positioning (inset-0) didn't reliably inherit width constraints during parent restructuring, causing getBoundingClientRect() to return stale full-width values. Tabs now stack via CSS Grid ([grid-area:1/1]) which keeps them in normal document flow. A useLayoutEffect with hide → setBounds → show cycle ensures the native WKWebView repositions synchronously on toggle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR completes the
inspect-element-pillsfeature branch with critical bug fixes and code quality improvements. It addresses a DevTools expansion bug and fixes 7 code review issues across the browser automation system.Key Improvements
1. DevTools Expansion Bug Fix
When clicking the DevTools button, the browser view would unexpectedly expand. This was caused by
_inspector.show()docking by splitting the WKWebView's superview and resizing it, but the frame wasn't restored afterdetach()moved the inspector to a floating window.show(), restore afterdetach()src-tauri/src/commands/webview.rs2. Code Quality & Correctness Fixes
data-cursor-refwithdata-hive-refvisual-effects.ts!e.nativeEvent.isComposingguardMessageInput.tsxreturn;in catch blockBrowserTab.tsxBrowserTab.tsx!tab.selectorActiveguard + dependency arrayBrowserTab.tsxBrowserTab.tsxduration-200 easeInspectedElementCard.tsxTechnical Details
Browser Automation Layer:
User Input Handling:
Code Quality:
Impact
Testing Verification
All changes have been:
Files Changed
src-tauri/src/commands/webview.rs- DevTools frame restoresrc/features/browser/automation/visual-effects.ts- Cursor ref attributessrc/features/session/ui/MessageInput.tsx- IME composition guardsrc/features/browser/ui/BrowserTab.tsx- Multiple correctness fixessrc/features/session/ui/InspectedElementCard.tsx- Animation timingReady for Review
This is a high-quality change addressing both user-facing bugs (DevTools expansion, IME input loss) and developer experience issues (cursor animations, error handling, performance). All changes align with project architecture and best practices.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation