feat: browser focus mode, webview migration, session composer store#255
Merged
Conversation
Browser: migrated to `<webview>` (Codex-pattern) and replaced the legacy `WebContentsView` path; added a Codex-style focus-mode overlay with a floating chat composer. Focus mode auto-enters when the chat panel is collapsed (button or splitter drag) and exits when expanded. Removed the detached-browser feature (DetachedBrowserWindow, useBrowserDetach, BrowserDetachedPlaceholder, ViewportDropdown). Composer: introduced per-session `sessionComposerStore` (Immer + devtools) holding draft, model, thinking, plan-mode, and all staged content — pasted texts, inspected elements, file/skill mentions, image attachments. `SessionComposer` is a thin stateful wrapper over the now-presentational `MessageInput`, which reads/writes the store directly. Cross-surface drift (main chat ↔ modal ↔ focus overlay) is gone by construction. Cross-panel pushers (BrowserPanel element selector, DiffViewer comment, SimulatorPanel screenshot) now dispatch to `sessionComposerActions` directly; the `chatInsertStore` pub/sub layer and MainLayout subscriber are deleted. Visual polish: glass-pill MessageInput (bg-muted/75 + backdrop blur + shadow-lg + ring), concentric-fixed staged cards, scale-on-press on all action buttons, browser-tab-style icon cross-fade applied to chat session tabs. Follow-up: `docs/session-tabs-refactor.md` captures the deferred session-tabs component split for a separate PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Guest DevTools always open detached — docked modes (`bottom`/`right`) silently fail because a `<webview>` guest has no BrowserWindow to dock into. Updated the default + doc comment to match reality. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit full review |
|
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:
📝 WalkthroughWalkthroughReplaces native BrowserView-based browser integration with DOM-managed Electron Changes
Sequence Diagram(s)sequenceDiagram
actor React
participant WM as WebviewManager
participant WV as Webview (DOM)
participant IPC as Renderer IPC
participant Main as Electron Main
React->>WM: useWebview(id, initialUrl)
WM->>WV: create container + <webview> (partition, src)
WV->>WV: loadURL(initialUrl)
React->>React: measure bounds
React->>WM: sync({bounds,isVisible})
WM->>WV: set position/visibility/pointerEvents
WV->>React: did-start-loading / did-stop-loading / ipc-message
React->>WV: executeJavaScript (automation injection) when ready
React->>IPC: invoke browser_webview_emulation_set(params with webContentsId)
IPC->>Main: browser_webview_emulation_set
Main->>Main: attach debugger & CDP Emulation.setDeviceMetricsOverride
Main-->>IPC: { success, error? }
IPC-->>React: response
React->>IPC: browser_webview_devtools_open(webContentsId, mode)
IPC->>Main: browser_webview_devtools_open
Main->>WV: openDevTools(mode)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
apps/web/src/features/browser/ui/BrowserTabBar.tsx (1)
29-31: Consider extracting shared cross-fade transition constant.
ICON_CROSS_FADEis now duplicated verbatim in bothBrowserTabBar.tsxandMainContentTabs.tsx. Consider extracting to a shared utility (e.g.,@/shared/lib/transitions.ts) to ensure consistency and reduce maintenance burden.Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/BrowserTabBar.tsx` around lines 29 - 31, Extract the duplicated ICON_CROSS_FADE constant into a shared module (e.g., create and export from a new transitions utility) and replace the local definitions in BrowserTabBar.tsx and MainContentTabs.tsx with an import of that shared symbol; ensure the new module exports ICON_CROSS_FADE (same string value) and update both components to import and use that exported constant so styling remains identical and maintainable.apps/web/src/features/session/store/sessionComposerStore.ts (2)
74-86: Silent no-op inmutatewhen session is unseeded — consider logging in DEV.The
mutatehelper silently ignores actions ifs.composers[sessionId]doesn't exist. This is defensive, but if a caller forgets to seed before mutating, bugs would be hard to trace. Consider adding aconsole.warnin development mode.🔧 Optional: Add DEV-only warning
function mutate(sessionId: string, recipe: (c: ComposerState) => void, label: string): void { useSessionComposerStore.setState( (s) => { const slice = s.composers[sessionId]; - if (slice) recipe(slice); + if (slice) { + recipe(slice); + } else if (import.meta.env.DEV) { + console.warn(`[sessionComposerStore] mutate called for unseeded session: ${sessionId}`); + } }, false, `composer/${label}` ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/session/store/sessionComposerStore.ts` around lines 74 - 86, The mutate helper silently no-ops when no composer slice exists for sessionId; update mutate (the function that calls useSessionComposerStore.setState) to emit a DEV-only warning when s.composers[sessionId] is missing — e.g., check process.env.NODE_ENV !== 'production' (or your app's isDev flag) and call console.warn including the sessionId and the label parameter so callers can trace forgotten seeding, but keep the runtime behavior the same in production.
160-176: Consider deduplicating pasted texts by content hash.
addPastedTextalways appends a new entry. If a user pastes the same large text block twice, they'll see duplicate cards. This may be intentional UX (showing each paste action), but if not, consider deduplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/session/store/sessionComposerStore.ts` around lines 160 - 176, addPastedText currently always appends a new entry so identical pasted blocks produce duplicate cards; update the addPastedText implementation (the mutate call inside addPastedText that modifies c.pastedTexts) to deduplicate by computing a content key (either compare content string equality or compute a short hash via Web Crypto/SHA-256) and only push a new { id: crypto.randomUUID(), content } when no existing c.pastedTexts entry has the same key/content; ensure any key/hash field is stored on the pastedTexts items so removePastedText and other code that references pastedTexts continue to work.apps/web/src/features/browser/webview-manager.ts (1)
238-249: Global listeners are never removed — acceptable for a singleton but consider cleanup for tests.
ensureGlobalListenersaddsfocusandvisibilitychangehandlers that persist for the app lifetime. This is fine for production (singleton lives forever), but may cause issues in test environments that mount/unmount modules. If tests show flakiness, consider exposing adestroy()method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/webview-manager.ts` around lines 238 - 249, ensureGlobalListeners currently registers window/document handlers and flips globalListenersBound but never removes them; add a public destroy() method on the same class that removes the previously added listeners, resets globalListenersBound to false, and clears any references to instances as needed for tests. To do this, store the listener functions created in ensureGlobalListeners (the resyncAll function and the visibilitychange wrapper) on instance fields (e.g., this._resyncAll, this._onVisibilityChange), use those stored references in removeEventListener inside destroy(), and ensure destroy() also clears or dereferences this.instances if necessary for test cleanup.apps/web/src/features/browser/ui/BrowserPanel.tsx (1)
566-586: Screenshot handling works but is verbose — consider extracting to a utility.The base64-to-Blob conversion (lines 575-580) is correct but duplicates logic that could live in
imageAttachments.ts. This isn't blocking, but extraction would improve reusability if other features need similar conversion.♻️ Optional: Extract dataUrl-to-File helper
// In imageAttachments.ts export function dataUrlToFile(dataUrl: string, filename: string): File { const base64 = dataUrl.replace(/^data:image\/\w+;base64,/, ""); 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/png" }); return new File([blob], filename, { type: "image/png" }); }Then in BrowserPanel:
const file = dataUrlToFile(dataUrl, `browser-screenshot-${Date.now()}.png`); const processed = await processImageFiles([file]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/BrowserPanel.tsx` around lines 566 - 586, The base64-to-Blob conversion inside handleScreenshot is duplicated and should be moved to a reusable helper in imageAttachments.ts; create a function (e.g., export function dataUrlToFile(dataUrl: string, filename: string): File) that performs the base64 -> Uint8Array -> Blob -> File conversion, import that helper into BrowserPanel, replace the conversion block in handleScreenshot with a call to dataUrlToFile(dataUrl, `browser-screenshot-${Date.now()}.png`), and keep the subsequent call to processImageFiles([file]) and sessionComposerActions.addImageAttachments(sid, processed) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/main/browser-emulation.ts`:
- Around line 43-68: Track whether this call attached the debugger and applied
emulation so you can fully roll back on error: set a local flag (e.g.,
attachedHere) when you call wc.debugger.attach("1.3") and only add webContentsId
to emulatedIds after all steps succeed; in the catch block, if attachedHere and
wc.debugger.isAttached() then detach the debugger, call
Emulation.clearDeviceMetricsOverride via wc.debugger.sendCommand, call
Emulation.setTouchEmulationEnabled with enabled: false, and reset the zoom via
wc.setZoomFactor(1); also ensure you remove webContentsId from emulatedIds if
you added it before failure. Use the existing symbols wc.debugger.attach,
wc.debugger.isAttached,
wc.debugger.sendCommand("Emulation.setDeviceMetricsOverride"/"Emulation.setTouchEmulationEnabled"),
wc.setZoomFactor, and emulatedIds to implement this rollback.
In `@apps/desktop/main/index.ts`:
- Around line 95-99: Update the will-attach-webview handler to accept the third
params argument (signature: (_event, webPreferences, params)) and
validate/cleanse guest-supplied webview attributes before attachment: parse
params.src with URL and only allow known safe schemes (e.g., https:, http:,
file:) otherwise replace with 'about:blank' or clear it; strip any disallowed
keys from params (only allow a minimal whitelist such as src and partition) and
override/force secure webPreferences (set preload to join(__dirname,
"../preload/browser-preload.mjs"), contextIsolation = true, nodeIntegration =
false) so renderer-supplied options cannot enable unsafe features (reference the
will-attach-webview handler, webPreferences, and params).
In `@apps/desktop/preload/browser-preload.ts`:
- Around line 27-35: The current preload overrides for window.confirm and
window.prompt are unsafe because they return true and echo defaultValue; change
the handlers in browser-preload.ts (the window.confirm and window.prompt
overrides) to preserve fail-safe defaults: have window.confirm return false and
window.prompt return null when no real dialog is available, while keeping any
logging you want; ensure the signature and optional params remain unchanged so
behavior matches native APIs but defaults to false/null to avoid implicit
approvals or submitted values.
In `@apps/web/src/app/layouts/MainContent.tsx`:
- Around line 389-397: The ResizableHandle onDragging callback flips a
module-level guard via webviewManager.setPointerEventsEnabled but doesn't
guarantee reset if the component unmounts or a drag is cancelled; add a cleanup
effect in the component (useEffect with empty deps) that calls
webviewManager.setPointerEventsEnabled(true) on unmount to ensure pointer-events
are restored for all webviews, and keep the existing onDragging handler using
webviewManager.setPointerEventsEnabled(!isDragging).
In `@apps/web/src/features/browser/hooks/useWebview.ts`:
- Around line 47-50: The render currently calls webviewManager.getOrCreate(id,
initialUrl) via instanceRef in useWebview, causing DOM mutations during render;
move that instantiation into a useEffect that runs on mount/when id or
initialUrl change (e.g., if instanceRef.current === null then set it to
webviewManager.getOrCreate(id, initialUrl) inside useEffect with [id,
initialUrl] deps), keep the existing useLayoutEffect to call
instanceRef.current?.sync({ bounds, isVisible }) on bounds/isVisible changes,
and add a cleanup useEffect that calls instanceRef.current?.detach() on unmount;
remove the render-time getOrCreate call so DOM creation only happens at commit
time.
In `@apps/web/src/features/browser/ui/BrowserTab.tsx`:
- Around line 191-208: The onFailLoad handler currently only sets the error and
doesn't persist the failed navigation target, so "Try Again" reloads the
previous URL; update the onFailLoad function to also extract e.validatedURL on
main-frame failures and call onUpdateTab(tabId, { url: e.validatedURL ||
tab.currentUrl, loading: false, error: desc }) (or set the appropriate
currentUrl/key used by the tab state) before logging so the tab's currentUrl
reflects the failed target for retries.
In `@apps/web/src/features/browser/ui/FocusModeOverlay.tsx`:
- Around line 56-85: When anchorEl changes in FocusModeOverlay's useLayoutEffect
the code must synchronously read and set the new rect to avoid rendering with
the previous rectangle; call setRect(readRect(anchorEl)) immediately after
confirming anchorEl is non-null (before creating the ResizeObserver and
scheduling rAF work), then keep the existing ResizeObserver/raf schedule for
subsequent changes; also update/remove the incorrect comment that claims
ResizeObserver.observe() fires synchronously. Make these edits inside the
useLayoutEffect block (referencing anchorEl, setRect, readRect, schedule, and
the ResizeObserver instance).
In `@apps/web/src/features/session/hooks/useSessionActions.ts`:
- Around line 68-87: Move the model/harness resolution into the guarded send
path so lookup errors are caught and the toast/post-send flow still runs: inside
the try block (before calling sendMessageMutation.mutateAsync) compute
effectiveFull = modelOverride ?? composer.model, then call
getModelId(effectiveFull) and getAgentHarnessForModel(effectiveFull) to produce
effectiveModel and effectiveHarness; remove the pre-try calls to
getModelId/getAgentHarnessForModel so that any thrown errors are handled by the
existing catch surrounding sendMessageMutation.mutateAsync.
In `@apps/web/src/features/session/ui/MessageInput.tsx`:
- Line 67: FALLBACK_MODEL is duplicated across MessageInput (FALLBACK_MODEL),
SessionComposer (FALLBACK_MODEL) and HomeView (DEFAULT_HOME_MODEL); extract a
single exported constant (e.g., EXPORT const DEFAULT_MODEL =
"claude:claude-opus-4-7") into a shared module under apps/web/src/shared
(suggest shared/constants.ts or shared/agents/constants.ts), update
MessageInput.tsx to import that constant instead of its local FALLBACK_MODEL,
update SessionComposer.tsx to import the same constant, and replace
HomeView.tsx's DEFAULT_HOME_MODEL with an import of the shared constant so all
three modules reference the single source of truth.
In `@apps/web/src/features/session/ui/PastedImageCard.tsx`:
- Around line 31-35: The Tailwind class string in PastedImageCard that currently
uses hardcoded colors ("border-black/10 dark:border-white/10") violates the
color-token rule; update the class in the component (the string passed alongside
SIZE_CLASSES[size] inside the PastedImageCard component) to use the project's
CSS color tokens/variables instead of hardcoded black/white—for example replace
those tokens with the appropriate token names (e.g.,
border-[var(--token-border)/10] and dark:border-[var(--token-border-dark)/10] or
the project's equivalent token classes) so both light and dark mode borders use
theme variables.
In `@apps/web/src/features/session/ui/SessionPanel.tsx`:
- Around line 198-210: The drag-drop handler handleDrop currently calls
processImageFiles(files) and discards non-image files; update handleDrop to
separate image and non-image files by inspecting e.dataTransfer.files, pass
image files through processImageFiles and call
sessionComposerActions.addImageAttachments(sessionId, processed) as before, and
for non-image/document files route them into the existing file-mention/staging
flow (e.g., invoke the app's file attachment or file-mention staging action
instead of dropping them) so the panel actually accepts non-image attachments;
ensure sessionId is used for both attachment actions and preserve existing
preventDefault/stopPropagation handling.
In `@docs/session-tabs-refactor.md`:
- Around line 38-51: The fenced tree block in the docs/session-tabs-refactor.md
lacks a language tag so markdownlint flags it; locate the triple-backtick tree
block (the block showing the apps/web/src/features/session/ui/tabs/ tree) and
add the language identifier "text" immediately after the opening ``` (i.e.
change ``` to ```text) so the block is treated as plain text by the linter.
---
Nitpick comments:
In `@apps/web/src/features/browser/ui/BrowserPanel.tsx`:
- Around line 566-586: The base64-to-Blob conversion inside handleScreenshot is
duplicated and should be moved to a reusable helper in imageAttachments.ts;
create a function (e.g., export function dataUrlToFile(dataUrl: string,
filename: string): File) that performs the base64 -> Uint8Array -> Blob -> File
conversion, import that helper into BrowserPanel, replace the conversion block
in handleScreenshot with a call to dataUrlToFile(dataUrl,
`browser-screenshot-${Date.now()}.png`), and keep the subsequent call to
processImageFiles([file]) and sessionComposerActions.addImageAttachments(sid,
processed) unchanged.
In `@apps/web/src/features/browser/ui/BrowserTabBar.tsx`:
- Around line 29-31: Extract the duplicated ICON_CROSS_FADE constant into a
shared module (e.g., create and export from a new transitions utility) and
replace the local definitions in BrowserTabBar.tsx and MainContentTabs.tsx with
an import of that shared symbol; ensure the new module exports ICON_CROSS_FADE
(same string value) and update both components to import and use that exported
constant so styling remains identical and maintainable.
In `@apps/web/src/features/browser/webview-manager.ts`:
- Around line 238-249: ensureGlobalListeners currently registers window/document
handlers and flips globalListenersBound but never removes them; add a public
destroy() method on the same class that removes the previously added listeners,
resets globalListenersBound to false, and clears any references to instances as
needed for tests. To do this, store the listener functions created in
ensureGlobalListeners (the resyncAll function and the visibilitychange wrapper)
on instance fields (e.g., this._resyncAll, this._onVisibilityChange), use those
stored references in removeEventListener inside destroy(), and ensure destroy()
also clears or dereferences this.instances if necessary for test cleanup.
In `@apps/web/src/features/session/store/sessionComposerStore.ts`:
- Around line 74-86: The mutate helper silently no-ops when no composer slice
exists for sessionId; update mutate (the function that calls
useSessionComposerStore.setState) to emit a DEV-only warning when
s.composers[sessionId] is missing — e.g., check process.env.NODE_ENV !==
'production' (or your app's isDev flag) and call console.warn including the
sessionId and the label parameter so callers can trace forgotten seeding, but
keep the runtime behavior the same in production.
- Around line 160-176: addPastedText currently always appends a new entry so
identical pasted blocks produce duplicate cards; update the addPastedText
implementation (the mutate call inside addPastedText that modifies
c.pastedTexts) to deduplicate by computing a content key (either compare content
string equality or compute a short hash via Web Crypto/SHA-256) and only push a
new { id: crypto.randomUUID(), content } when no existing c.pastedTexts entry
has the same key/content; ensure any key/hash field is stored on the pastedTexts
items so removePastedText and other code that references pastedTexts continue to
work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a58d6d28-130f-4c81-a7b2-cc63c8adb0f3
📒 Files selected for processing (49)
apps/backend/test/unit/shared/events.test.tsapps/desktop/main/browser-emulation.tsapps/desktop/main/browser-views.tsapps/desktop/main/index.tsapps/desktop/main/native-handlers.tsapps/desktop/preload/browser-preload.tsapps/desktop/preload/index.tsapps/web/src/app/App.tsxapps/web/src/app/layouts/ContentView.tsxapps/web/src/app/layouts/MainContent.tsxapps/web/src/app/layouts/MainLayout.tsxapps/web/src/app/layouts/MobileLayout.tsxapps/web/src/components/ui/resizable.tsxapps/web/src/features/apps/hooks/useAppsStopped.tsapps/web/src/features/browser/hooks/useBrowserDetach.tsapps/web/src/features/browser/hooks/useWebview.tsapps/web/src/features/browser/store/browserWindowStore.tsapps/web/src/features/browser/store/index.tsapps/web/src/features/browser/types.tsapps/web/src/features/browser/ui/BrowserDetachedPlaceholder.tsxapps/web/src/features/browser/ui/BrowserPanel.tsxapps/web/src/features/browser/ui/BrowserTab.tsxapps/web/src/features/browser/ui/BrowserTabBar.tsxapps/web/src/features/browser/ui/DetachedBrowserWindow.tsxapps/web/src/features/browser/ui/FocusModeOverlay.tsxapps/web/src/features/browser/ui/ViewportDropdown.tsxapps/web/src/features/browser/webview-manager.tsapps/web/src/features/session/hooks/index.tsapps/web/src/features/session/hooks/useImageAttachments.tsapps/web/src/features/session/hooks/useSessionActions.tsapps/web/src/features/session/hooks/useSessionComposer.tsapps/web/src/features/session/lib/imageAttachments.tsapps/web/src/features/session/store/sessionComposerStore.tsapps/web/src/features/session/ui/MessageInput.tsxapps/web/src/features/session/ui/PastedImageCard.tsxapps/web/src/features/session/ui/PastedTextCard.tsxapps/web/src/features/session/ui/PlanModeToggle.tsxapps/web/src/features/session/ui/SessionComposer.tsxapps/web/src/features/session/ui/SessionPanel.tsxapps/web/src/features/session/ui/tools/renderers/RecordingToolRenderers.tsxapps/web/src/features/simulator/ui/SimulatorPanel.tsxapps/web/src/features/workspace/ui/DiffViewer.tsxapps/web/src/features/workspace/ui/MainContentTabs.tsxapps/web/src/platform/capabilities.tsapps/web/src/platform/native/browser-views.tsapps/web/src/shared/stores/chatInsertStore.tsdocs/session-tabs-refactor.mdshared/events.tsskills-lock.json
💤 Files with no reviewable changes (10)
- apps/desktop/main/native-handlers.ts
- apps/web/src/features/browser/store/index.ts
- apps/backend/test/unit/shared/events.test.ts
- apps/web/src/features/session/ui/tools/renderers/RecordingToolRenderers.tsx
- apps/web/src/features/browser/ui/BrowserDetachedPlaceholder.tsx
- apps/web/src/features/browser/ui/ViewportDropdown.tsx
- apps/web/src/features/browser/hooks/useBrowserDetach.ts
- apps/web/src/features/browser/ui/DetachedBrowserWindow.tsx
- apps/desktop/main/browser-views.ts
- apps/web/src/shared/stores/chatInsertStore.ts
Comment on lines
+47
to
+50
| const instanceRef = useRef<WebviewInstance | null>(null); | ||
| if (instanceRef.current === null) { | ||
| instanceRef.current = webviewManager.getOrCreate(id, initialUrl); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "useWebview.ts" | head -20Repository: zvadaadam/deus-machine
Length of output: 117
🏁 Script executed:
find . -type f -name "*webview*" | grep -E "(hook|manager)" | head -20Repository: zvadaadam/deus-machine
Length of output: 116
🏁 Script executed:
cat -n ./apps/web/src/features/browser/hooks/useWebview.tsRepository: zvadaadam/deus-machine
Length of output: 3120
🏁 Script executed:
cat -n ./apps/web/src/features/browser/webview-manager.tsRepository: zvadaadam/deus-machine
Length of output: 10718
Move webview instantiation into an effect to avoid render-time DOM mutations.
Lines 47–50 initialize instanceRef by calling webviewManager.getOrCreate() during render. The WebviewInstance constructor immediately appends a container to document.body (webview-manager.ts line 147), making the render function impure. In React 18, this can cause problems: double-invocation in StrictMode or aborted renders may leave detached DOM elements in the tree, and the cleanup path via useEffect only hides them (via detach()) rather than removing them.
Move this initialization into a useEffect so the DOM mutation happens at commit time:
Suggested refactor
export function useWebview({
id,
initialUrl,
bounds,
isVisible,
}: UseWebviewOptions): UseWebviewResult {
const instanceRef = useRef<WebviewInstance | null>(null);
// Initialize webview instance on mount
useEffect(() => {
if (instanceRef.current === null) {
instanceRef.current = webviewManager.getOrCreate(id, initialUrl);
}
}, [id, initialUrl]);
// Sync on every bounds/visibility change
useLayoutEffect(() => {
instanceRef.current?.sync({ bounds, isVisible });
}, [bounds, isVisible]);
// Detach on unmount
useEffect(() => {
return () => instanceRef.current?.detach();
}, []);
// ... rest of function
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/features/browser/hooks/useWebview.ts` around lines 47 - 50, The
render currently calls webviewManager.getOrCreate(id, initialUrl) via
instanceRef in useWebview, causing DOM mutations during render; move that
instantiation into a useEffect that runs on mount/when id or initialUrl change
(e.g., if instanceRef.current === null then set it to
webviewManager.getOrCreate(id, initialUrl) inside useEffect with [id,
initialUrl] deps), keep the existing useLayoutEffect to call
instanceRef.current?.sync({ bounds, isVisible }) on bounds/isVisible changes,
and add a cleanup useEffect that calls instanceRef.current?.detach() on unmount;
remove the render-time getOrCreate call so DOM creation only happens at commit
time.
Owner
Author
There was a problem hiding this comment.
I looked into this and I'm going to disagree. webviewManager.getOrCreate(id, ...) is idempotent — it looks up the id in an internal Map and returns the existing WebviewInstance if one exists. StrictMode double-mount or an aborted first render don't create duplicate DOM because the second call hits the map cache. The suggested refactor would mean the consumer gets a null instance on first render, which BrowserTab isn't set up for. The current instanceRef pattern is safe given the manager's singleton semantics.
There was a problem hiding this comment.
@zvadaadam, you're right — the idempotency guarantee from the internal Map (returning the cached WebviewInstance on any repeat call) combined with detach() keeping the instance alive in the map across StrictMode unmount/remount cycles makes the render-time initialization safe. And the null-on-first-render issue my refactor would introduce is a real problem for BrowserTab. I'll retract the suggestion.
(∩｀-´)⊃━☆ﾟ.*･｡ﾟ
✏️ Learnings added
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 255
File: apps/web/src/features/browser/hooks/useWebview.ts:47-50
Timestamp: 2026-04-22T16:01:21.381Z
Learning: In `apps/web/src/features/browser/hooks/useWebview.ts`, the render-time call to `webviewManager.getOrCreate(id, initialUrl)` inside the `if (instanceRef.current === null)` guard is intentional and safe. `getOrCreate` is idempotent — it checks an internal `Map` keyed by tab id and returns the cached `WebviewInstance` on any subsequent call, so StrictMode double-invocation or aborted renders never produce duplicate DOM nodes. Moving the call into a `useEffect` would introduce a null instance on first render, which `BrowserTab` is not designed to handle. Do not flag this render-time initialization pattern in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 250
File: apps/web/src/features/browser/store/browserWindowStore.ts:36-43
Timestamp: 2026-04-19T22:14:42.531Z
Learning: In `apps/web/src/features/browser/store/browserWindowStore.ts`, the single-slot `pendingNewTab`/`pendingCloseTab` design is intentional. Two reasons: (1) `apps.service` enforces one running instance per (appId, workspace), so a second `apps:launched` event for the same app is never emitted — the duplicate `launchApp` call returns the existing `runningAppId`; (2) `BrowserPanel`'s `useEffect` consumer fires synchronously on the same render cycle that the producer's Zustand `set()` triggers (same pattern as `chatInsertStore`), so the slot is consumed before any second event could overwrite it. Do not suggest converting to a FIFO queue or requestId-ack pattern in future reviews.
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 250
File: apps/web/src/features/browser/ui/BrowserPanel.tsx:352-354
Timestamp: 2026-04-19T22:14:44.557Z
Learning: In `apps/web/src/features/browser/ui/BrowserPanel.tsx`, the `pendingCloseTab` effect intentionally matches only on `t.openedAt` (not `t.currentUrl`) when filtering tabs to close on `apps:stopped`. Do NOT suggest a `currentUrl` fallback for three reasons: (1) Electron rewrites `currentUrl` to `chrome-error://chromewebdata/` on `ERR_CONNECTION_REFUSED` reload when an app dies, so `startsWith(urlPrefix)` on `currentUrl` would silently miss the tab; (2) `openedAt` and its persistence were introduced in the same PR as AAP tab creation, so there is no migration scenario where AAP-owned tabs lack the field; (3) falling back to `currentUrl` risks accidentally closing user-opened tabs that share the same localhost origin/port prefix.
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 244
File: apps/web/src/features/session/hooks/useSlashCommand.ts:87-154
Timestamp: 2026-04-15T21:01:15.460Z
Learning: In `apps/web/src/features/session/hooks/useSlashCommand.ts`, the `useEffect` that fetches skills/commands on workspace change intentionally omits a stale-response cancellation guard. The trade-off is deliberate: workspace path changes are rare (only on repo switch), the backend reads local filesystem files in sub-100ms, and the race condition window is practically impossible. Do not flag the absence of a cancellation guard in this hook in future reviews.
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 244
File: apps/web/src/features/session/hooks/useSlashCommand.ts:67-154
Timestamp: 2026-04-15T21:01:13.186Z
Learning: In `apps/web/src/features/session/hooks/useSlashCommand.ts`, the one-shot fetch pattern using `useEffect` + `useState` + `fetchedWorkspaceRef` is intentional. Skills/commands are local config reads that happen once per workspace open and do not need TanStack Query (no cache invalidation, stale-while-revalidate, or server state management required). Do not suggest migrating this to TanStack Query; it would be over-engineering for this use case.
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 247
File: apps/web/src/app/layouts/useChatTabs.ts:328-347
Timestamp: 2026-04-17T08:21:41.285Z
Learning: In `apps/web/src/app/layouts/useChatTabs.ts`, `updateChatTabAgentHarness` is intentionally a no-op when `tab.data?.hasStarted` is true. Once a session has messages, the harness is locked server-side in `handleSendMessage`; allowing local UI harness changes on started tabs would cause drift from the persisted session until reload. Do not flag this early-return as missing logic in future reviews.
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 249
File: packages/device-use/src/frontend/stores/sim-store.ts:48-79
Timestamp: 2026-04-18T09:08:53.053Z
Learning: In `packages/device-use/src/frontend/stores/sim-store.ts`, the TanStack Query guideline does NOT apply. `packages/device-use` is a standalone package with its own minimal viewer (Zustand-only), intentionally decoupled from `apps/web/`. The TanStack Query v5 guideline applies to Deus's main UI (`apps/web/`) only. Do not suggest migrating `sim-store.ts` (or any other store in `packages/device-use/src/frontend/stores/`) to TanStack Query in future reviews.
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 240
File: apps/web/src/features/session/hooks/useTextBuffer.ts:49-60
Timestamp: 2026-04-12T23:40:23.156Z
Learning: In `apps/web/src/features/session/hooks/useTextBuffer.ts`, the markdown code-fence check in the rAF streaming buffer is intentionally conservative: it flushes the full text immediately when any unclosed ``` is detected in the current slice, even if it's a false positive (e.g. right after a closing fence). This trade-off is deliberate — premature flush (showing full text) is harmless, while rendering a partial/unclosed code fence causes visual glitches. Do not flag this as a bug in future reviews.
Learnt from: zvadaadam
Repo: zvadaadam/box-ide PR: 177
File: src/app/layouts/hooks/useRepoActions.ts:21-22
Timestamp: 2026-03-12T23:25:58.674Z
Learning: In zvadaadam/box-ide, two TypeScript path aliases coexist: 'shared/*' -> '/shared/' (root-level, cross-package) and '@/shared/*' -> 'src/shared/' (frontend-only). Do not flag mixed usage as inconsistent since they reference different directories and scopes. Ensure imports use the correct alias per intended scope and that tsconfig.json and vite.config.ts are configured accordingly. This applies to all TypeScript files in the repo.
Comment on lines
+31
to
35
| // Pure black/white outline at 10% opacity (the skill's image rule — | ||
| // never a tinted neutral, which picks up the surface color and | ||
| // reads as dirt on the image edge). | ||
| "group bg-muted/50 relative shrink-0 overflow-hidden border border-black/10 dark:border-white/10", | ||
| SIZE_CLASSES[size] |
There was a problem hiding this comment.
Replace hardcoded border colors with theme tokens.
Line 34 introduces border-black/10 dark:border-white/10, which violates the project color-token rule.
🎯 Suggested tokenized update
- "group bg-muted/50 relative shrink-0 overflow-hidden border border-black/10 dark:border-white/10",
+ "group bg-muted/50 relative shrink-0 overflow-hidden border border-[color:var(--color-image-outline)]",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/features/session/ui/PastedImageCard.tsx` around lines 31 - 35,
The Tailwind class string in PastedImageCard that currently uses hardcoded
colors ("border-black/10 dark:border-white/10") violates the color-token rule;
update the class in the component (the string passed alongside
SIZE_CLASSES[size] inside the PastedImageCard component) to use the project's
CSS color tokens/variables instead of hardcoded black/white—for example replace
those tokens with the appropriate token names (e.g.,
border-[var(--token-border)/10] and dark:border-[var(--token-border-dark)/10] or
the project's equivalent token classes) so both light and dark mode borders use
theme variables.
Owner
Author
There was a problem hiding this comment.
I'm going to disagree on this one. The make-interfaces-feel-better skill the user applied explicitly prescribes pure black / pure white at 10% opacity for image outlines specifically: "The color must be pure black in light mode (rgba(0, 0, 0, 0.1)) and pure white in dark mode (rgba(255, 255, 255, 0.1)) — never a near-black like slate, zinc, or any tinted neutral. A tinted outline picks up the surface color underneath it and reads as dirt on the image edge." A theme token is a tinted neutral — using one here would re-introduce exactly the "dirt on edge" problem this rule exists to prevent. Keeping the raw black/white values for this specific image-outline case.
There was a problem hiding this comment.
@zvadaadam, that's a compelling and technically well-grounded rationale. Pure rgba(0,0,0,0.1) / rgba(255,255,255,0.1) is a classic image-border trick precisely because the zero-saturation value can't pick up the hue of whatever surface sits beneath it — a tinted token would. The inline comment already documents the intent clearly. I'll accept this as an intentional exception to the general color-token rule.
✏️ Learnings added
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 255
File: apps/web/src/features/session/ui/PastedImageCard.tsx:31-35
Timestamp: 2026-04-22T16:01:33.688Z
Learning: In `apps/web/src/features/session/ui/PastedImageCard.tsx`, the border classes `border-black/10 dark:border-white/10` on image cards are intentional and should NOT be flagged as a color-token violation. The `make-interfaces-feel-better` skill explicitly prescribes pure black/white at 10% opacity for image outlines — tinted CSS token neutrals pick up the underlying surface hue and "read as dirt on the image edge". This is a deliberate, skill-prescribed exception to the general repo rule requiring CSS variables for all colors.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: zvadaadam/deus-machine PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-04T05:52:13.199Z
Learning: Applies to **/*.{ts,tsx,css,tailwind.*} : All colors must use CSS variables/tokens. Never hardcode colors.
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 240
File: apps/web/src/features/session/hooks/useTextBuffer.ts:49-60
Timestamp: 2026-04-12T23:40:23.156Z
Learning: In `apps/web/src/features/session/hooks/useTextBuffer.ts`, the markdown code-fence check in the rAF streaming buffer is intentionally conservative: it flushes the full text immediately when any unclosed ``` is detected in the current slice, even if it's a false positive (e.g. right after a closing fence). This trade-off is deliberate — premature flush (showing full text) is harmless, while rendering a partial/unclosed code fence causes visual glitches. Do not flag this as a bug in future reviews.
Comment on lines
+198
to
+210
| const handleDrop = useCallback( | ||
| async (e: React.DragEvent) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| setIsDragging(false); | ||
| const files = Array.from(e.dataTransfer.files); | ||
| if (files.length > 0) { | ||
| const processed = await processImageFiles(files); | ||
| if (processed.length) { | ||
| sessionComposerActions.addImageAttachments(sessionId, processed); | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Dropped non-image files are discarded.
This path only runs processImageFiles(files), so source files and documents are ignored even though the panel’s drop affordance tells users they can “add files” to the message. Either narrow the UX copy to images or branch non-image drops into the file-mention/staging flow as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/features/session/ui/SessionPanel.tsx` around lines 198 - 210,
The drag-drop handler handleDrop currently calls processImageFiles(files) and
discards non-image files; update handleDrop to separate image and non-image
files by inspecting e.dataTransfer.files, pass image files through
processImageFiles and call sessionComposerActions.addImageAttachments(sessionId,
processed) as before, and for non-image/document files route them into the
existing file-mention/staging flow (e.g., invoke the app's file attachment or
file-mention staging action instead of dropping them) so the panel actually
accepts non-image attachments; ensure sessionId is used for both attachment
actions and preserve existing preventDefault/stopPropagation handling.
Owner
Author
There was a problem hiding this comment.
Good point — deferring. This is pre-existing behavior (the old chatInsertStore.insertFiles path also routed through processImageFiles and silently dropped non-images); this PR moved the wiring but didn't change the semantics. Handling non-image drops properly needs a design decision (narrow the overlay copy vs. route to file-mention staging vs. surface a toast), so worth a separate UX pass.
There was a problem hiding this comment.
@zvadaadam, understood — that context makes sense. Since the semantics pre-date this PR and fixing it properly requires a UX decision (copy narrowing vs. file-mention staging vs. toast), deferring is the right call.
Would you like me to open a follow-up issue to track this? I can capture the three options you outlined (narrow copy, file-mention staging, toast) as the design choices to resolve.
🧠 Learnings used
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 247
File: apps/web/src/app/layouts/useChatTabs.ts:328-347
Timestamp: 2026-04-17T08:21:41.285Z
Learning: In `apps/web/src/app/layouts/useChatTabs.ts`, `updateChatTabAgentHarness` is intentionally a no-op when `tab.data?.hasStarted` is true. Once a session has messages, the harness is locked server-side in `handleSendMessage`; allowing local UI harness changes on started tabs would cause drift from the persisted session until reload. Do not flag this early-return as missing logic in future reviews.
Learnt from: zvadaadam
Repo: zvadaadam/deus-machine PR: 250
File: apps/web/src/features/browser/store/browserWindowStore.ts:36-43
Timestamp: 2026-04-19T22:14:42.531Z
Learning: In `apps/web/src/features/browser/store/browserWindowStore.ts`, the single-slot `pendingNewTab`/`pendingCloseTab` design is intentional. Two reasons: (1) `apps.service` enforces one running instance per (appId, workspace), so a second `apps:launched` event for the same app is never emitted — the duplicate `launchApp` call returns the existing `runningAppId`; (2) `BrowserPanel`'s `useEffect` consumer fires synchronously on the same render cycle that the producer's Zustand `set()` triggers (same pattern as `chatInsertStore`), so the slot is consumed before any second event could overwrite it. Do not suggest converting to a FIFO queue or requestId-ack pattern in future reviews.
Fixes (9): - browser-emulation: rollback partial CDP state on failure (zoom, metrics, touch, debugger detach) so a failed emulation toggle doesn't corrupt the next call - desktop main: validate `params.src` scheme + strip non-allowlisted webview params in `will-attach-webview` — renderer can no longer smuggle non-http/https URLs or flip sandbox flags at attach time - browser-preload: fail-safe guest dialogs — `confirm()` returns false, `prompt()` returns null (was auto-approving destructive actions / echoing defaultValue) - MainContent: cleanup effect re-enables webview pointer-events on unmount so a mid-drag unmount doesn't leave webviews stuck at pointer-events:none - BrowserTab: persist validatedURL on nav failure so "Try Again" retries the right target (not the previously-loaded page) - FocusModeOverlay: read anchor rect eagerly on anchor change so the overlay doesn't render one frame at the old anchor's bounds - useSessionActions: move model/harness resolution inside the try/catch so a stale stored model or bad override surfaces a toast instead of an unhandled rejection - shared/agents: export `DEFAULT_MODEL`; replace duplicated `FALLBACK_MODEL`/`DEFAULT_HOME_MODEL` constants in MessageInput, SessionComposer, HomeView - docs/session-tabs-refactor: add `text` language tag to fenced tree block (markdownlint) Disagreed (2): - useWebview.ts: `getOrCreate` is idempotent (keyed by id in the manager's Map), so StrictMode double-mount doesn't create orphan DOM. Moving to useEffect would mean a first render with null instance — consumer pattern would need rewiring for no actual benefit. - PastedImageCard border: the `make-interfaces-feel-better` skill explicitly prescribes pure black/white with 10% opacity for image outlines — a theme token (tinted neutral) picks up the surface color and reads as dirt on the image edge, which is exactly what that rule exists to prevent. Deferred (1): - SessionPanel drop handler: non-image files silently dropped is pre-existing behavior (same in the old chatInsertStore path). Narrowing the copy or handling non-image drops is a separate UX pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
Greenlight — Round 1Fixed (9) — in
Disagreed (2):
Deferred (1):
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
apps/web/src/features/session/ui/PastedImageCard.tsx (1)
31-35:⚠️ Potential issue | 🟠 MajorReplace hardcoded border colors with theme tokens.
The
border-black/10 dark:border-white/10classes are still hardcoded and break the color-token rule.Proposed fix
- "group bg-muted/50 relative shrink-0 overflow-hidden border border-black/10 dark:border-white/10", + "group bg-muted/50 relative shrink-0 overflow-hidden border border-border/60",As per coding guidelines,
**/*.{ts,tsx,css,tailwind.*}requires all colors to use CSS variables/tokens and never hardcode colors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/session/ui/PastedImageCard.tsx` around lines 31 - 35, The hardcoded tailwind color classes in PastedImageCard.tsx ("border-black/10 dark:border-white/10") violate the color-token rule; update the class string used in the component (where SIZE_CLASSES[size] is combined with the group classes) to use the project theme tokens/CSS variables instead (for example swap the hardcoded border classes for the appropriate token-based classes or utility that maps to --border-subtle / --border-subtle-dark or your project's border token names) so both light and dark borders come from theme tokens rather than literal colors.docs/session-tabs-refactor.md (1)
38-51:⚠️ Potential issue | 🟡 MinorAdd a language tag to the fenced tree block.
This fence is currently untyped, so markdownlint flags it (MD040). Use
textfor plain text blocks.-``` +```text apps/web/src/features/session/ui/tabs/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/session-tabs-refactor.md` around lines 38 - 51, The fenced code block showing the file tree in docs/session-tabs-refactor.md is missing a language tag (MD040); update the opening fence from ``` to ```text so the tree block is typed as plain text; ensure the rest of the fenced block (the tree lines and closing ```) remain unchanged.apps/web/src/features/browser/hooks/useWebview.ts (1)
47-50:⚠️ Potential issue | 🟠 MajorMove webview instantiation into an effect to avoid render-time DOM mutations.
Lines 47–50 initialize
instanceRefby callingwebviewManager.getOrCreate()during render. TheWebviewInstanceconstructor immediately appends a container todocument.body(webview-manager.ts line 147), making the render function impure. In React 18, double-invocation in StrictMode or aborted renders may leave detached DOM elements.Suggested refactor
export function useWebview({ id, initialUrl, bounds, isVisible, }: UseWebviewOptions): UseWebviewResult { const instanceRef = useRef<WebviewInstance | null>(null); - if (instanceRef.current === null) { - instanceRef.current = webviewManager.getOrCreate(id, initialUrl); - } + + // Initialize webview instance on mount (not during render) + useEffect(() => { + if (instanceRef.current === null) { + instanceRef.current = webviewManager.getOrCreate(id, initialUrl); + } + }, [id, initialUrl]); // Single sync point — writes container style synchronously at commit time. useLayoutEffect(() => { instanceRef.current?.sync({ bounds, isVisible }); }, [bounds, isVisible]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/hooks/useWebview.ts` around lines 47 - 50, The webview is being created during render (instanceRef.current = webviewManager.getOrCreate(id, initialUrl)) which triggers DOM mutations; move that instantiation into an effect: keep const instanceRef = useRef<WebviewInstance | null>(null) in render, remove the direct assignment, and in a useEffect (or useLayoutEffect if you need sync after mount) call webviewManager.getOrCreate(id, initialUrl) and assign the result to instanceRef.current; include id and initialUrl in the effect dependencies and, if WebviewInstance exposes a cleanup (e.g., destroy/unmount) or webviewManager has a release method, call that on effect cleanup to avoid leaked detached DOM nodes.apps/web/src/app/layouts/MainContent.tsx (1)
389-397:⚠️ Potential issue | 🟠 MajorRestore webview pointer events on unmount or cancelled drags.
This callback flips a module-level guard in
webviewManager, so it needs a guaranteed reset path. If the panel unmounts mid-drag,onDragging(false)never runs and every live webview can stay stuck atpointer-events: none.🧯 Suggested follow-up
+ // Ensure pointer-events are restored if component unmounts mid-drag + useEffect(() => { + return () => webviewManager.setPointerEventsEnabled(true); + }, []); + return ( <SidebarInset className="min-w-0">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/layouts/MainContent.tsx` around lines 389 - 397, The ResizableHandle onDragging callback flips a module-level guard via webviewManager.setPointerEventsEnabled but there's no guaranteed reset if the panel unmounts or a drag is aborted; add a cleanup to restore pointer events: in the component that renders <ResizableHandle> add a useEffect that on unmount calls webviewManager.setPointerEventsEnabled(true), and also subscribe to window pointerup/pointercancel (or similar drag-abort events) to call webviewManager.setPointerEventsEnabled(true) so cancelled drags or unmounts always restore pointer events.apps/web/src/features/browser/ui/FocusModeOverlay.tsx (1)
56-85:⚠️ Potential issue | 🟡 MinorImmediately read the new anchor rect on anchor element changes.
ResizeObserver.observe()does not fire synchronously—its callback executes asynchronously after layout. WhenanchorElchanges, the overlay can paint at stale/null rect before the observer callback fires.Also fix the incorrect comment on lines 72-74—ResizeObserver does not fire synchronously on
observe().📐 Proposed fix
useLayoutEffect(() => { if (!anchorEl) { // React to anchor being detached — legitimate dependency-sync case // where the rule's normal "update in callback" advice doesn't apply. // eslint-disable-next-line react-hooks/set-state-in-effect setRect(null); return; } + // Immediately read rect to avoid painting with stale bounds + setRect(readRect(anchorEl)); let rafId: number | null = null; const schedule = () => { if (rafId !== null) return; rafId = requestAnimationFrame(() => { rafId = null; setRect(readRect(anchorEl)); }); }; - // ResizeObserver fires synchronously on observe() with the current - // size — which drives the initial `setRect` through `schedule` → - // rAF. No explicit initial setState needed. + // ResizeObserver fires asynchronously — the initial setRect above + // handles the first paint; the observer handles subsequent resizes. const ro = new ResizeObserver(schedule);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/FocusModeOverlay.tsx` around lines 56 - 85, When anchorEl changes we must synchronously read and set its rect before waiting for the ResizeObserver callback; inside the useLayoutEffect for FocusModeOverlay, if anchorEl is truthy call setRect(readRect(anchorEl)) immediately (then create the ResizeObserver that schedules subsequent updates via schedule) so the overlay doesn't render stale/null rects, and update the comment to remove the incorrect claim that ResizeObserver.observe() fires synchronously; keep existing raf/cancelAnimationFrame, ro.observe(anchorEl), and cleanup logic intact.apps/web/src/features/browser/ui/BrowserTab.tsx (1)
191-207:⚠️ Potential issue | 🟠 MajorPersist the failed navigation target for retries.
On a main-frame failure before
did-navigate,tab.currentUrlstill points at the previous page. Because this handler only storeserror, “Try Again” reloads the wrong target. PersistvalidatedURLhere as the tab’s current URL/title.Patch sketch
- onUpdateTab(tabId, { loading: false, error: desc }); + onUpdateTab(tabId, { + loading: false, + error: desc, + ...(e.validatedURL + ? { + url: e.validatedURL, + currentUrl: e.validatedURL, + title: deriveTitleFromUrl(e.validatedURL), + } + : {}), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/BrowserTab.tsx` around lines 191 - 207, Persist the failed navigation target from the onFailLoad handler by including e.validatedURL in the tab update so retries use the failed URL; modify the onFailLoad flow (function onFailLoad) to pass the validatedURL into onUpdateTab (e.g. set currentUrl and title to e.validatedURL or fallback to existing tab.currentUrl if empty) when calling onUpdateTab(tabId, {...}), so the "Try Again" action reloads the correct target, and keep the existing error/log updates via onAddLog.apps/web/src/features/session/hooks/useSessionActions.ts (1)
68-83:⚠️ Potential issue | 🟠 MajorMove model resolution inside the guarded send path.
getModelId()can throw for stalecomposer.modelvalues, but it still runs before the existingtry/catch. That turns an invalid model into an uncaught rejection instead of the normal toast/error path.Patch sketch
- const effectiveFull = modelOverride ?? composer.model; - const effectiveModel = getModelId(effectiveFull); - const effectiveHarness = getAgentHarnessForModel(effectiveFull); - try { + const effectiveFull = modelOverride ?? composer.model; + const effectiveModel = getModelId(effectiveFull); + const effectiveHarness = getAgentHarnessForModel(effectiveFull); + await sendMessageMutation.mutateAsync({ sessionId, content,Based on learnings,
getModelIdintentionally throws on unknown input because values sent over the wire should fail loudly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/session/hooks/useSessionActions.ts` around lines 68 - 83, The code calls getModelId(...) and getAgentHarnessForModel(...) before entering the try/catch, so a thrown error from stale composer.model bypasses the normal error/toast path; move resolution of effectiveFull, effectiveModel (getModelId) and effectiveHarness (getAgentHarnessForModel) inside the try block that surrounds sendMessageMutation.mutateAsync so any exceptions are caught and handled (same error/toast path) — specifically adjust the logic around composer.model and modelOverride used before calling sendMessageMutation.mutateAsync in useSessionActions.ts so getModelId and getAgentHarnessForModel run within the guarded send path.apps/web/src/features/session/ui/SessionPanel.tsx (1)
198-209:⚠️ Potential issue | 🟡 MinorDropped non-image files are still discarded.
processImageFiles(files)only keeps supported image MIME types, so dragging a source file or document still does nothing even though the overlay says “Add files”. Either route non-image drops into the existing file-staging flow or narrow the drop affordance to images only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/session/ui/SessionPanel.tsx` around lines 198 - 209, handleDrop currently sends all files to processImageFiles and silently drops non-images; change it to split the dropped files into image and non-image sets, call processImageFiles only for images and then sessionComposerActions.addImageAttachments(sessionId, processedImages) for those, and for non-image files route them into the existing file-staging flow by calling the project's file-attachment/staging action (e.g., sessionComposerActions.stageFiles(sessionId, nonImageFiles) or the equivalent function used elsewhere); alternatively, if you prefer to limit the UI, narrow the drop affordance to images only by validating MIME types up-front and showing a message instead of accepting non-image files.apps/desktop/main/browser-emulation.ts (1)
43-67:⚠️ Potential issue | 🟠 MajorRollback emulation state when setup fails midway.
If
attach()or the first CDP command succeeds and a later step throws, this still returns{ success: false }while leaving the guest zoomed and/or debugger-attached. The next emulation toggle then starts from corrupted state. Please clear metrics/touch, reset zoom, and detach only when this call attached the debugger.Patch sketch
- try { - if (!wc.debugger.isAttached()) wc.debugger.attach("1.3"); + let attachedHere = false; + try { + if (!wc.debugger.isAttached()) { + wc.debugger.attach("1.3"); + attachedHere = true; + } @@ - } catch (err) { + } catch (err) { + try { + if (wc.debugger.isAttached()) { + await wc.debugger.sendCommand("Emulation.clearDeviceMetricsOverride", {}); + await wc.debugger.sendCommand("Emulation.setTouchEmulationEnabled", { + enabled: false, + }); + if (attachedHere) wc.debugger.detach(); + } + } catch { + // keep original error + } + wc.setZoomFactor(1); + emulatedIds.delete(webContentsId); return { success: false, error: String(err) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/main/browser-emulation.ts` around lines 43 - 67, If emulation setup fails partway through, we must undo any partial changes only when we actually attached the debugger: track whether wc.debugger.attach("1.3") succeeded (e.g., a local flag), and in the catch block reset state by calling Emulation.setDeviceMetricsOverride (or send a clearing payload), Emulation.setTouchEmulationEnabled (disabled), reset wc.setZoomFactor(1), detach the debugger via wc.debugger.detach() only if attach succeeded, and remove webContentsId from emulatedIds if it was added; adjust the catch to perform these rollbacks before returning { success: false, error: String(err) } so subsequent toggles start from a clean state.
🧹 Nitpick comments (2)
skills-lock.json (1)
7-7: Consider making the hash algorithm explicit.
computedHashis present, but the algorithm is implicit. Adding a field likehashAlgorithm: "sha256"would make future migrations and validation logic less ambiguous.Optional schema tweak
"skills": { "make-interfaces-feel-better": { "source": "jakubkrehel/make-interfaces-feel-better", "sourceType": "github", + "hashAlgorithm": "sha256", "computedHash": "54b6e221bbe4e2d1a8461c3f9ecabe69daf891f964e9ee5e699c323daa018f2e" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills-lock.json` at line 7, Add an explicit hash algorithm field alongside computedHash to remove ambiguity: update the skills-lock.json structure to include a new key like "hashAlgorithm": "sha256" wherever "computedHash" appears (e.g., in the same object that contains computedHash) and ensure any code reading/writing computedHash (search for uses of computedHash) is updated to default to "sha256" when hashAlgorithm is missing and to validate or use the specified algorithm when present.apps/web/src/features/browser/webview-manager.ts (1)
238-249: Global event listeners are never removed — acceptable for singleton, verify HMR behavior.The
focusandvisibilitychangelisteners are added once and persist for the app lifetime. This is correct for a singleton, but during Vite HMR the module may re-execute. TheglobalListenersBoundflag prevents duplicate registration within the same instance, but if the singleton is recreated on HMR, old listeners referencing a staleinstancesMap could fire.Consider whether this is acceptable for dev experience or if a cleanup pattern (e.g.,
window.__WEBVIEW_MANAGER_CLEANUP__?.()) is warranted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/webview-manager.ts` around lines 238 - 249, The ensureGlobalListeners method attaches window/document handlers (resyncAll) but never removes them, which can leak stale listeners across Vite HMR reloads; update ensureGlobalListeners (and related initialization) to register handlers as named functions and expose a cleanup hook (e.g. assign a function to window.__WEBVIEW_MANAGER_CLEANUP__) that calls removeEventListener for the "focus" and "visibilitychange" listeners and clears globalListenersBound, and ensure any HMR teardown code calls that cleanup so stale references to this.instances/resync() are removed on module reload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/features/browser/ui/BrowserTab.tsx`:
- Around line 145-146: The back/forward branch that uses suppressHistoryPushRef
only updates currentUrl but leaves historyIndex stale, so subsequent typed
navigations don't truncate forward entries; in the same handler where
suppressHistoryPushRef is checked (references: suppressHistoryPushRef,
currentUrl, historyIndex and the history array/state), also update historyIndex
to the correct position by locating the new URL in the history array (e.g.,
findIndex on history entries) and calling the existing setter (setHistoryIndex)
with that index; if the URL isn't present, update history by replacing/adding
the entry and setHistoryIndex to the new last index so forward history is
correctly truncated.
In `@apps/web/src/features/session/store/sessionComposerStore.ts`:
- Around line 277-286: The handleTabClose callback in useChatTabs.ts currently
removes the UI tab but does not call sessionComposerActions.discard(sessionId),
leaving stale entries in the composers Record; update handleTabClose to invoke
sessionComposerActions.discard(sid) (or
sessionComposerActions.discard(sessionId) as named in that file) immediately
after the tab is removed so the store’s discard(sid) implementation (from
sessionComposerStore.ts) runs and cleans up the composers entry for that
session; add the appropriate import for sessionComposerActions if not already
present.
---
Duplicate comments:
In `@apps/desktop/main/browser-emulation.ts`:
- Around line 43-67: If emulation setup fails partway through, we must undo any
partial changes only when we actually attached the debugger: track whether
wc.debugger.attach("1.3") succeeded (e.g., a local flag), and in the catch block
reset state by calling Emulation.setDeviceMetricsOverride (or send a clearing
payload), Emulation.setTouchEmulationEnabled (disabled), reset
wc.setZoomFactor(1), detach the debugger via wc.debugger.detach() only if attach
succeeded, and remove webContentsId from emulatedIds if it was added; adjust the
catch to perform these rollbacks before returning { success: false, error:
String(err) } so subsequent toggles start from a clean state.
In `@apps/web/src/app/layouts/MainContent.tsx`:
- Around line 389-397: The ResizableHandle onDragging callback flips a
module-level guard via webviewManager.setPointerEventsEnabled but there's no
guaranteed reset if the panel unmounts or a drag is aborted; add a cleanup to
restore pointer events: in the component that renders <ResizableHandle> add a
useEffect that on unmount calls webviewManager.setPointerEventsEnabled(true),
and also subscribe to window pointerup/pointercancel (or similar drag-abort
events) to call webviewManager.setPointerEventsEnabled(true) so cancelled drags
or unmounts always restore pointer events.
In `@apps/web/src/features/browser/hooks/useWebview.ts`:
- Around line 47-50: The webview is being created during render
(instanceRef.current = webviewManager.getOrCreate(id, initialUrl)) which
triggers DOM mutations; move that instantiation into an effect: keep const
instanceRef = useRef<WebviewInstance | null>(null) in render, remove the direct
assignment, and in a useEffect (or useLayoutEffect if you need sync after mount)
call webviewManager.getOrCreate(id, initialUrl) and assign the result to
instanceRef.current; include id and initialUrl in the effect dependencies and,
if WebviewInstance exposes a cleanup (e.g., destroy/unmount) or webviewManager
has a release method, call that on effect cleanup to avoid leaked detached DOM
nodes.
In `@apps/web/src/features/browser/ui/BrowserTab.tsx`:
- Around line 191-207: Persist the failed navigation target from the onFailLoad
handler by including e.validatedURL in the tab update so retries use the failed
URL; modify the onFailLoad flow (function onFailLoad) to pass the validatedURL
into onUpdateTab (e.g. set currentUrl and title to e.validatedURL or fallback to
existing tab.currentUrl if empty) when calling onUpdateTab(tabId, {...}), so the
"Try Again" action reloads the correct target, and keep the existing error/log
updates via onAddLog.
In `@apps/web/src/features/browser/ui/FocusModeOverlay.tsx`:
- Around line 56-85: When anchorEl changes we must synchronously read and set
its rect before waiting for the ResizeObserver callback; inside the
useLayoutEffect for FocusModeOverlay, if anchorEl is truthy call
setRect(readRect(anchorEl)) immediately (then create the ResizeObserver that
schedules subsequent updates via schedule) so the overlay doesn't render
stale/null rects, and update the comment to remove the incorrect claim that
ResizeObserver.observe() fires synchronously; keep existing
raf/cancelAnimationFrame, ro.observe(anchorEl), and cleanup logic intact.
In `@apps/web/src/features/session/hooks/useSessionActions.ts`:
- Around line 68-83: The code calls getModelId(...) and
getAgentHarnessForModel(...) before entering the try/catch, so a thrown error
from stale composer.model bypasses the normal error/toast path; move resolution
of effectiveFull, effectiveModel (getModelId) and effectiveHarness
(getAgentHarnessForModel) inside the try block that surrounds
sendMessageMutation.mutateAsync so any exceptions are caught and handled (same
error/toast path) — specifically adjust the logic around composer.model and
modelOverride used before calling sendMessageMutation.mutateAsync in
useSessionActions.ts so getModelId and getAgentHarnessForModel run within the
guarded send path.
In `@apps/web/src/features/session/ui/PastedImageCard.tsx`:
- Around line 31-35: The hardcoded tailwind color classes in PastedImageCard.tsx
("border-black/10 dark:border-white/10") violate the color-token rule; update
the class string used in the component (where SIZE_CLASSES[size] is combined
with the group classes) to use the project theme tokens/CSS variables instead
(for example swap the hardcoded border classes for the appropriate token-based
classes or utility that maps to --border-subtle / --border-subtle-dark or your
project's border token names) so both light and dark borders come from theme
tokens rather than literal colors.
In `@apps/web/src/features/session/ui/SessionPanel.tsx`:
- Around line 198-209: handleDrop currently sends all files to processImageFiles
and silently drops non-images; change it to split the dropped files into image
and non-image sets, call processImageFiles only for images and then
sessionComposerActions.addImageAttachments(sessionId, processedImages) for
those, and for non-image files route them into the existing file-staging flow by
calling the project's file-attachment/staging action (e.g.,
sessionComposerActions.stageFiles(sessionId, nonImageFiles) or the equivalent
function used elsewhere); alternatively, if you prefer to limit the UI, narrow
the drop affordance to images only by validating MIME types up-front and showing
a message instead of accepting non-image files.
In `@docs/session-tabs-refactor.md`:
- Around line 38-51: The fenced code block showing the file tree in
docs/session-tabs-refactor.md is missing a language tag (MD040); update the
opening fence from ``` to ```text so the tree block is typed as plain text;
ensure the rest of the fenced block (the tree lines and closing ```) remain
unchanged.
---
Nitpick comments:
In `@apps/web/src/features/browser/webview-manager.ts`:
- Around line 238-249: The ensureGlobalListeners method attaches window/document
handlers (resyncAll) but never removes them, which can leak stale listeners
across Vite HMR reloads; update ensureGlobalListeners (and related
initialization) to register handlers as named functions and expose a cleanup
hook (e.g. assign a function to window.__WEBVIEW_MANAGER_CLEANUP__) that calls
removeEventListener for the "focus" and "visibilitychange" listeners and clears
globalListenersBound, and ensure any HMR teardown code calls that cleanup so
stale references to this.instances/resync() are removed on module reload.
In `@skills-lock.json`:
- Line 7: Add an explicit hash algorithm field alongside computedHash to remove
ambiguity: update the skills-lock.json structure to include a new key like
"hashAlgorithm": "sha256" wherever "computedHash" appears (e.g., in the same
object that contains computedHash) and ensure any code reading/writing
computedHash (search for uses of computedHash) is updated to default to "sha256"
when hashAlgorithm is missing and to validate or use the specified algorithm
when present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e099529-8ec3-407f-89f3-10d3ed7e9132
📒 Files selected for processing (49)
apps/backend/test/unit/shared/events.test.tsapps/desktop/main/browser-emulation.tsapps/desktop/main/browser-views.tsapps/desktop/main/index.tsapps/desktop/main/native-handlers.tsapps/desktop/preload/browser-preload.tsapps/desktop/preload/index.tsapps/web/src/app/App.tsxapps/web/src/app/layouts/ContentView.tsxapps/web/src/app/layouts/MainContent.tsxapps/web/src/app/layouts/MainLayout.tsxapps/web/src/app/layouts/MobileLayout.tsxapps/web/src/components/ui/resizable.tsxapps/web/src/features/apps/hooks/useAppsStopped.tsapps/web/src/features/browser/hooks/useBrowserDetach.tsapps/web/src/features/browser/hooks/useWebview.tsapps/web/src/features/browser/store/browserWindowStore.tsapps/web/src/features/browser/store/index.tsapps/web/src/features/browser/types.tsapps/web/src/features/browser/ui/BrowserDetachedPlaceholder.tsxapps/web/src/features/browser/ui/BrowserPanel.tsxapps/web/src/features/browser/ui/BrowserTab.tsxapps/web/src/features/browser/ui/BrowserTabBar.tsxapps/web/src/features/browser/ui/DetachedBrowserWindow.tsxapps/web/src/features/browser/ui/FocusModeOverlay.tsxapps/web/src/features/browser/ui/ViewportDropdown.tsxapps/web/src/features/browser/webview-manager.tsapps/web/src/features/session/hooks/index.tsapps/web/src/features/session/hooks/useImageAttachments.tsapps/web/src/features/session/hooks/useSessionActions.tsapps/web/src/features/session/hooks/useSessionComposer.tsapps/web/src/features/session/lib/imageAttachments.tsapps/web/src/features/session/store/sessionComposerStore.tsapps/web/src/features/session/ui/MessageInput.tsxapps/web/src/features/session/ui/PastedImageCard.tsxapps/web/src/features/session/ui/PastedTextCard.tsxapps/web/src/features/session/ui/PlanModeToggle.tsxapps/web/src/features/session/ui/SessionComposer.tsxapps/web/src/features/session/ui/SessionPanel.tsxapps/web/src/features/session/ui/tools/renderers/RecordingToolRenderers.tsxapps/web/src/features/simulator/ui/SimulatorPanel.tsxapps/web/src/features/workspace/ui/DiffViewer.tsxapps/web/src/features/workspace/ui/MainContentTabs.tsxapps/web/src/platform/capabilities.tsapps/web/src/platform/native/browser-views.tsapps/web/src/shared/stores/chatInsertStore.tsdocs/session-tabs-refactor.mdshared/events.tsskills-lock.json
💤 Files with no reviewable changes (10)
- apps/web/src/features/session/ui/tools/renderers/RecordingToolRenderers.tsx
- apps/web/src/features/browser/store/index.ts
- apps/backend/test/unit/shared/events.test.ts
- apps/desktop/main/native-handlers.ts
- apps/web/src/features/browser/ui/BrowserDetachedPlaceholder.tsx
- apps/web/src/features/browser/ui/DetachedBrowserWindow.tsx
- apps/web/src/features/browser/ui/ViewportDropdown.tsx
- apps/web/src/features/browser/hooks/useBrowserDetach.ts
- apps/desktop/main/browser-views.ts
- apps/web/src/shared/stores/chatInsertStore.ts
Fixes (2): - BrowserTab: track back/forward direction with `historyNavDeltaRef` and advance `historyIndex` in the suppressed-push branch. Without this, the next typed navigation after a back/forward slices history from a stale index, losing the current page and keeping dead forward entries alive. - useChatTabs: call `sessionComposerActions.discard(sessionId)` in `handleTabClose` so the composer Record doesn't accumulate stale entries across long workspace sessions. Restore re-creates a fresh slice if the user brings the tab back. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
Greenlight — Round 2Fixed (2) — in
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/src/features/browser/ui/BrowserTab.tsx (1)
229-252:⚠️ Potential issue | 🟠 MajorBack/forward navigations still leave
historyIndexstale.When
suppressHistoryPushRefis true (set bygoBack/goForward),handleNavigatedupdatesurl/currentUrl/titlebut does not updatehistoryIndex. After a back navigation,historyIndexstill points to the forward-most entry, so the next typed URL slices history from the wrong position — preserving stale forward entries instead of truncating them.This was flagged in a previous review and appears unaddressed.
🔧 Suggested approach
Track the navigation direction in a ref and apply it in
handleNavigated:const suppressHistoryPushRef = useRef(false); + const historyNavDeltaRef = useRef<-1 | 0 | 1>(0); // In handleNavigated: if (suppressHistoryPushRef.current) { suppressHistoryPushRef.current = false; + const newIndex = Math.max(0, Math.min( + current.historyIndex + historyNavDeltaRef.current, + current.history.length - 1 + )); + historyNavDeltaRef.current = 0; - onUpdateTab(tabId, { url, currentUrl: url, title: deriveTitleFromUrl(url) }); + onUpdateTab(tabId, { + url, + currentUrl: url, + title: deriveTitleFromUrl(url), + historyIndex: newIndex, + }); return; } // In goBack: suppressHistoryPushRef.current = true; + historyNavDeltaRef.current = -1; wv.goBack(); // In goForward: suppressHistoryPushRef.current = true; + historyNavDeltaRef.current = 1; wv.goForward();Also applies to: 366-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/BrowserTab.tsx` around lines 229 - 252, handleNavigated currently ignores historyIndex when suppressHistoryPushRef is set (used by goBack/goForward), so back/forward navigations leave historyIndex stale; fix by tracking the target history index in a ref (e.g., pendingHistoryIndexRef) when initiating goBack/goForward and, inside handleNavigated, when suppressHistoryPushRef.current is true use that pendingHistoryIndexRef value to update historyIndex (and clear both refs), otherwise keep the existing logic (slice history up to current.historyIndex+1 and push new url). Update places that set suppressHistoryPushRef (goBack/goForward) to also set pendingHistoryIndexRef to current.historyIndex - 1 or + 1 respectively so onUpdateTab receives the correct historyIndex.
🧹 Nitpick comments (1)
apps/web/src/features/browser/ui/FocusModeOverlay.tsx (1)
88-94: Consider preventing event propagation on Escape to avoid double-handling.The Escape listener calls
onExit()unconditionally. IfBrowserPanelalso has an Escape listener (as noted in the comment), both may fire. Callinge.stopPropagation()or checkinge.defaultPreventedwould prevent the overlay's handler from firing when a nested element (e.g., a popover insideSessionComposer) already consumed the key.🛡️ Optional defensive fix
useEffect(() => { const onKey = (e: KeyboardEvent) => { + if (e.defaultPrevented) return; if (e.key === "Escape") onExit(); }; window.addEventListener("keydown", onKey); return () => window.removeEventListener("keydown", onKey); }, [onExit]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/FocusModeOverlay.tsx` around lines 88 - 94, The Escape handler in FocusModeOverlay's useEffect (the onKey listener) currently calls onExit() unconditionally and can conflict with other Escape handlers (e.g., BrowserPanel); update the onKey implementation in FocusModeOverlay to first check e.defaultPrevented or call e.stopPropagation()/e.preventDefault() as appropriate and only call onExit() when the event was not already handled, so the overlay won't double-handle Escape. Locate the useEffect and onKey in FocusModeOverlay and add the defensive check/propagation handling around the call to onExit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/session-tabs-refactor.md`:
- Line 100: The sizing rationale incorrectly treats Tailwind classes as pixel
values: update the sentence referencing "h-10 → h-9" and the "7px" vs "9px" math
so it clarifies these are Tailwind scale classes (e.g., h-7, h-9) not literal
pixels, remove the "1px top, 1px bottom" pixel conclusion, and instead state the
intended visual effect (optical centering relative to BrowserTabBar) and advise
verifying actual pixel computed heights in the browser or design tokens before
implementing.
- Around line 25-26: Update the doc wording to avoid stating that Session.title
is "never read or written" or "always null"; instead state that Session.title
exists (Session.title?: string | null) and is persisted/handled by the backend
but is not yet surfaced in the frontend UI, e.g., replace the absolute claims
with "not yet wired into the frontend" or "frontend does not currently use the
persisted title field"; reference the Session.title symbol and the backend
handlers that read/write it (the agent event handler and persistence routines)
as the source of truth.
---
Duplicate comments:
In `@apps/web/src/features/browser/ui/BrowserTab.tsx`:
- Around line 229-252: handleNavigated currently ignores historyIndex when
suppressHistoryPushRef is set (used by goBack/goForward), so back/forward
navigations leave historyIndex stale; fix by tracking the target history index
in a ref (e.g., pendingHistoryIndexRef) when initiating goBack/goForward and,
inside handleNavigated, when suppressHistoryPushRef.current is true use that
pendingHistoryIndexRef value to update historyIndex (and clear both refs),
otherwise keep the existing logic (slice history up to current.historyIndex+1
and push new url). Update places that set suppressHistoryPushRef
(goBack/goForward) to also set pendingHistoryIndexRef to current.historyIndex -
1 or + 1 respectively so onUpdateTab receives the correct historyIndex.
---
Nitpick comments:
In `@apps/web/src/features/browser/ui/FocusModeOverlay.tsx`:
- Around line 88-94: The Escape handler in FocusModeOverlay's useEffect (the
onKey listener) currently calls onExit() unconditionally and can conflict with
other Escape handlers (e.g., BrowserPanel); update the onKey implementation in
FocusModeOverlay to first check e.defaultPrevented or call
e.stopPropagation()/e.preventDefault() as appropriate and only call onExit()
when the event was not already handled, so the overlay won't double-handle
Escape. Locate the useEffect and onKey in FocusModeOverlay and add the defensive
check/propagation handling around the call to onExit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d806c232-ae85-4048-aa69-84f5b6960dad
📒 Files selected for processing (12)
apps/desktop/main/browser-emulation.tsapps/desktop/main/index.tsapps/desktop/preload/browser-preload.tsapps/web/src/app/layouts/MainContent.tsxapps/web/src/features/browser/ui/BrowserTab.tsxapps/web/src/features/browser/ui/FocusModeOverlay.tsxapps/web/src/features/repository/ui/HomeView.tsxapps/web/src/features/session/hooks/useSessionActions.tsapps/web/src/features/session/ui/MessageInput.tsxapps/web/src/features/session/ui/SessionComposer.tsxapps/web/src/shared/agents/index.tsdocs/session-tabs-refactor.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/preload/browser-preload.ts
- apps/web/src/app/layouts/MainContent.tsx
Fixes (2), both in docs/session-tabs-refactor.md: - Softened the `Session.title` claim — backend DOES persist it (event-handler.ts, persistence.ts); the gap is frontend, not the whole codebase. - Corrected the sizing math — `h-7`/`h-9` are Tailwind scale classes (28px / 36px at default root) not literal pixels. Tab pill ends up with ~4px vertical padding each side, not 1px. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
Greenlight — Round 3Fixed (2) — both docs accuracy in
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/app/layouts/useChatTabs.ts (1)
266-270: Guard discard when the same session is still present in another tab.
discardcurrently runs unconditionally for the closing tab’ssessionId. If duplicate session tabs exist (e.g., persisted-state edge case), this can wipe draft/staged state for the still-open tab.♻️ Proposed hardening
- if (closingTab?.data?.sessionId) { + const closingSessionId = closingTab?.data?.sessionId; + if (closingSessionId) { setClosedTabs((prevClosed) => { const entry: ClosedTab = { label: closingTab.label, - sessionId: closingTab.data!.sessionId!, + sessionId: closingSessionId, agentHarness: closingTab.data?.agentHarness, initialModel: closingTab.data?.initialModel, closedAt: Date.now(), }; return [entry, ...prevClosed].slice(0, MAX_CLOSED_TABS); }); - // Drop the session's composer slice so the store doesn't - // accumulate stale entries over long-running workspaces. Restore - // creates a fresh slice from session data if the user brings this - // tab back. - sessionComposerActions.discard(closingTab.data.sessionId); + // Drop composer state only when this was the last open tab + // for that session. + const sessionStillOpen = newTabs.some((t) => t.data?.sessionId === closingSessionId); + if (!sessionStillOpen) { + sessionComposerActions.discard(closingSessionId); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/layouts/useChatTabs.ts` around lines 266 - 270, Currently you call sessionComposerActions.discard(closingTab.data.sessionId) unconditionally; change this to first check whether any other open tab (in the tabs/chatTabs state used by useChatTabs) still has the same sessionId (exclude the closing tab itself) and only call sessionComposerActions.discard(sessionId) when no other tab uses that sessionId. Locate the discard call in useChatTabs.ts and implement a guard like: compute sessionId = closingTab.data.sessionId, then if tabs.some(t => t.id !== closingTab.id && t.data?.sessionId === sessionId) skip discard, otherwise call sessionComposerActions.discard(sessionId).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/features/browser/ui/BrowserTab.tsx`:
- Around line 340-362: The effect must serialize emulation calls so the most
recent toggle wins: introduce a monotonically increasing request token (e.g.,
latestEmulationReqRef) and capture a localRequestId at the top of the async
IIFE; after getting webContentsId and after each await (before calling
clearEmulation or setEmulation and after they resolve) verify localRequestId ===
latestEmulationReqRef.current and bail out if not, and increment
latestEmulationReqRef in the effect cleanup to cancel in-flight work. Apply this
to the logic around wv.getWebContentsId(), tab.isMobileView, setEmulation and
clearEmulation so stale async completions cannot override newer toggles.
---
Nitpick comments:
In `@apps/web/src/app/layouts/useChatTabs.ts`:
- Around line 266-270: Currently you call
sessionComposerActions.discard(closingTab.data.sessionId) unconditionally;
change this to first check whether any other open tab (in the tabs/chatTabs
state used by useChatTabs) still has the same sessionId (exclude the closing tab
itself) and only call sessionComposerActions.discard(sessionId) when no other
tab uses that sessionId. Locate the discard call in useChatTabs.ts and implement
a guard like: compute sessionId = closingTab.data.sessionId, then if tabs.some(t
=> t.id !== closingTab.id && t.data?.sessionId === sessionId) skip discard,
otherwise call sessionComposerActions.discard(sessionId).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfe60d31-37b4-4119-81fa-d0c2ea63bb1b
📒 Files selected for processing (3)
apps/web/src/app/layouts/useChatTabs.tsapps/web/src/features/browser/ui/BrowserTab.tsxdocs/session-tabs-refactor.md
Fix (1): - BrowserTab emulation effect — serialize async toggles via a monotonic `emulationRequestRef` so rapid mobile-view toggles can't leave the webview in a stale mode. The cleanup bumps the token; the in-flight promise checks `isStale()` before and after the native call and bails if a newer toggle has superseded it. The next effect run re-applies with the latest state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
Greenlight — Round 4Fixed (1) — in
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/web/src/features/browser/ui/BrowserTab.tsx (1)
346-375:⚠️ Potential issue | 🟠 MajorStale emulation calls can still leave the webview in the wrong mode.
On Line 357 and Line 362, the native mutation (
clearEmulation/setEmulation) still executes even if this request becomes stale during the await. If an older request resolves after a newer one, final mode can be incorrect. Also, the async IIFE has no catch, so native call failures can surface as unhandled promise rejections.Suggested patch
useEffect(() => { if (!hasLoaded) return; const wv = getWebview(); if (!wv) return; const requestId = ++emulationRequestRef.current; const isStale = () => requestId !== emulationRequestRef.current; (async () => { let webContentsId: number; try { webContentsId = wv.getWebContentsId(); } catch { return; } if (isStale()) return; - if (!tab.isMobileView) { - await clearEmulation(webContentsId); - return; - } - - await setEmulation({ - webContentsId, - width: MOBILE_PREVIEW_WIDTH, - height: MOBILE_PREVIEW_HEIGHT, - deviceScaleFactor: MOBILE_PREVIEW_DPR, - mobile: true, - scale: 1, - }); - // If a newer toggle superseded us while the call was in flight, - // the next effect run will re-apply with the current state — we - // just need to bail here so we don't log / treat this as the - // authoritative result. - if (isStale()) return; - })(); + const apply = async (mobile: boolean) => { + if (!mobile) { + await clearEmulation(webContentsId); + return; + } + await setEmulation({ + webContentsId, + width: MOBILE_PREVIEW_WIDTH, + height: MOBILE_PREVIEW_HEIGHT, + deviceScaleFactor: MOBILE_PREVIEW_DPR, + mobile: true, + scale: 1, + }); + }; + + const requestedMobile = tab.isMobileView; + await apply(requestedMobile); + + // If superseded while awaiting, re-apply latest desired state once. + if (isStale()) { + await apply(tabRef.current.isMobileView); + } + })().catch((err) => { + if (!isStale()) { + onAddLog(tabId, "warn", `Emulation update failed: ${getErrorMessage(err)}`); + } + }); return () => { // Bump the token so any in-flight promise above sees isStale(). emulationRequestRef.current++; }; - }, [tab.isMobileView, hasLoaded, getWebview]); + }, [tab.isMobileView, hasLoaded, getWebview, tabId, onAddLog]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/BrowserTab.tsx` around lines 346 - 375, The async IIFE can perform native mutations (clearEmulation / setEmulation) after it becomes stale and has no catch for promise rejections; update the logic in the block that uses emulationRequestRef and isStale so you: 1) check isStale() immediately after any await that may yield (e.g., after calling wv.getWebContentsId() and after awaiting clearEmulation/setEmulation-related promises) and bail without calling native mutations if stale, and 2) wrap the entire async IIFE body (or at least the native calls) in a try/catch to handle and log errors instead of letting unhandled promise rejections surface; reference emulationRequestRef.current, the local isStale function, clearEmulation, setEmulation, wv.getWebContentsId, and tab.isMobileView when applying these guards and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/features/browser/ui/BrowserTab.tsx`:
- Around line 346-375: The async IIFE can perform native mutations
(clearEmulation / setEmulation) after it becomes stale and has no catch for
promise rejections; update the logic in the block that uses emulationRequestRef
and isStale so you: 1) check isStale() immediately after any await that may
yield (e.g., after calling wv.getWebContentsId() and after awaiting
clearEmulation/setEmulation-related promises) and bail without calling native
mutations if stale, and 2) wrap the entire async IIFE body (or at least the
native calls) in a try/catch to handle and log errors instead of letting
unhandled promise rejections surface; reference emulationRequestRef.current, the
local isStale function, clearEmulation, setEmulation, wv.getWebContentsId, and
tab.isMobileView when applying these guards and error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04738be5-b524-4f21-8fd3-7fb7908c6f9d
📒 Files selected for processing (1)
apps/web/src/features/browser/ui/BrowserTab.tsx
Route the guest DevTools UI into a companion <webview> so the inspector renders inline inside the browser panel (60/40 vertical split) instead of a detached window — docked modes don't work for webview guests on their own because they have no BrowserWindow to attach to. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/features/browser/ui/BrowserPanel.tsx (1)
708-717: DevTools toggle duplicates state update.
handleToggleDevtoolscallsaction()(which is eitheropenDevtoolsorcloseDevtoolsfrom the handle), then updates state viahandleUpdateTab. However,openDevtoolsandcloseDevtoolsinBrowserTab.tsx(Lines 590, 633) already callonUpdateTabinternally to setdevtoolsOpen. This results in the state being set twice.The second
handleUpdateTabcall here will overwrite with the same value, so it's not incorrect, but it's redundant.Optional: Remove redundant state update
Since
BrowserTab.openDevtools/closeDevtoolsalready updatedevtoolsOpenstate internally, this.then()handler is unnecessary:const handleToggleDevtools = useCallback(() => { if (!activeTab) return; const handle = tabRefs.current.get(activeTab.id); if (!handle) return; const action = activeTab.devtoolsOpen ? handle.closeDevtools : handle.openDevtools; if (!action) return; - action() - .then(() => handleUpdateTab(activeTab.id, { devtoolsOpen: !activeTab.devtoolsOpen })) - .catch((err) => handleAddLog(activeTab.id, "error", `DevTools toggle failed: ${err}`)); + action().catch((err) => handleAddLog(activeTab.id, "error", `DevTools toggle failed: ${err}`)); }, [activeTab, handleUpdateTab, handleAddLog]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/browser/ui/BrowserPanel.tsx` around lines 708 - 717, handleToggleDevtools is redundantly calling handleUpdateTab after invoking the handle's openDevtools/closeDevtools, but BrowserTab.openDevtools and closeDevtools already call onUpdateTab internally; remove the post-action state update to avoid duplicate updates by deleting the .then(() => handleUpdateTab(...)) call inside handleToggleDevtools and keep the action() invocation with the existing .catch(err => handleAddLog(...)) so errors are still logged (referencing handleToggleDevtools, openDevtools, closeDevtools, handleUpdateTab, and handleAddLog).apps/desktop/main/browser-emulation.ts (1)
130-168: Consider awaiting the asyncopenDevTools()before returning.
wc.openDevTools()(Line 160 and 163) is asynchronous but the handler returns{ success: true }synchronously. If DevTools fails to attach (e.g., the webContents is destroyed mid-call), the caller won't know. This is likely acceptable for fire-and-forget UX, but worth noting if callers need confirmation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/main/browser-emulation.ts` around lines 130 - 168, Make the ipcMain handler "browser_webview_devtools_open" async and await DevTools actually opening instead of returning immediately: replace the synchronous wc.openDevTools() calls with awaiting a Promise that resolves on wc.once('devtools-opened') and rejects if wc.isDestroyed() or on wc.once('destroyed') (and add a short timeout if desired). Update both branches (the devtoolsWebContentsId branch where you call wc.openDevTools() after wc.setDevToolsWebContents(dt) and the fallback wc.openDevTools({ mode }) branch) to use this await logic, ensure listeners are cleaned up on resolve/reject, and keep the same return shape { success: boolean; error?: string } by returning success only after the promise resolves or returning an error if it rejects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/main/browser-emulation.ts`:
- Around line 130-168: Make the ipcMain handler "browser_webview_devtools_open"
async and await DevTools actually opening instead of returning immediately:
replace the synchronous wc.openDevTools() calls with awaiting a Promise that
resolves on wc.once('devtools-opened') and rejects if wc.isDestroyed() or on
wc.once('destroyed') (and add a short timeout if desired). Update both branches
(the devtoolsWebContentsId branch where you call wc.openDevTools() after
wc.setDevToolsWebContents(dt) and the fallback wc.openDevTools({ mode }) branch)
to use this await logic, ensure listeners are cleaned up on resolve/reject, and
keep the same return shape { success: boolean; error?: string } by returning
success only after the promise resolves or returning an error if it rejects.
In `@apps/web/src/features/browser/ui/BrowserPanel.tsx`:
- Around line 708-717: handleToggleDevtools is redundantly calling
handleUpdateTab after invoking the handle's openDevtools/closeDevtools, but
BrowserTab.openDevtools and closeDevtools already call onUpdateTab internally;
remove the post-action state update to avoid duplicate updates by deleting the
.then(() => handleUpdateTab(...)) call inside handleToggleDevtools and keep the
action() invocation with the existing .catch(err => handleAddLog(...)) so errors
are still logged (referencing handleToggleDevtools, openDevtools, closeDevtools,
handleUpdateTab, and handleAddLog).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47c36fd0-780c-4462-9cb0-83c4adb4f4d1
📒 Files selected for processing (4)
apps/desktop/main/browser-emulation.tsapps/web/src/features/browser/ui/BrowserPanel.tsxapps/web/src/features/browser/ui/BrowserTab.tsxapps/web/src/platform/native/browser-views.ts
This was referenced Apr 23, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<webview>(Codex pattern) and adds a focus-mode overlay with a floating chat composer; focus auto-enters when the chat panel is collapsed via button or splitter drag, and exits when expanded.sessionComposerStore(per-session, Immer-backed) that owns draft, model, thinking level, plan mode, and all staged content (pastes, inspected elements, file/skill mentions, image attachments);MessageInputbecomes presentational,SessionComposerwraps it — cross-surface drift (main chat ↔ modal ↔ focus overlay) is gone.chatInsertStorepub/sub layer — BrowserPanel / DiffViewer / SimulatorPanel now dispatch tosessionComposerActionsdirectly.WebContentsViewpath;docs/session-tabs-refactor.mdcaptures the deferred session-tabs component split for a follow-up PR.Test plan
⌘Tnew chat tab,⌘⇧Trestore closed tab still workbun run test:backendpasses (unit tests; pre-existing DB-path integration failures unrelated)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes
Bug Fixes
Behavior