Followup: Web Interface UX Parity#696
Merged
pedramamini merged 4 commits intoRunMaestro:rcfrom Apr 1, 2026
Merged
Conversation
…wn, tab enhancements - Integrate xterm.js for full terminal emulation in web interface (replaces chat-message rendering). Backend broadcasts raw PTY data via terminal_data messages; frontend renders with full ANSI color, cursor, and resize support. - Add agent completion notifications dropdown — bell icon now shows a list of completed/errored agents with direct switch-to-agent on click. - Fix Auto Run "Configure & Launch" button not working from inline RightPanel (was gated on showAutoRunPanel which is only true for the old bottom sheet). - Replace hamburger menu icon with robot head agent icon in mobile header. - Add plus (+) button menu with "New AI Chat" and "New Terminal" options. - Add close button to terminal tab to switch back to AI mode. - Add left/right resizable panels, mobile viewport detection, and various UI improvements across the web interface. Session: e1e5a5c3-bbca-495d-adab-7d358766737c Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…r tablet - Remove the Auto Run play button from the header (accessible via Right Panel's Auto Run tab and Cmd+K) - Add left-edge swipe gesture to open the agent panel (mirrors existing right-edge swipe for right drawer) - Add slide-in/out animation and swipe-left-to-close to LeftPanel in mobile/full-screen mode - LeftPanel now renders as a 85vw sliding drawer (max 400px) with animated backdrop, matching RightDrawer UX Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add dedicated PTY spawning for web terminal sessions (spawn on mode switch, kill on exit)
- Support web terminal session ID format ({sessionId}-terminal) in PtySpawner
- Add spawnTerminalForWeb/killTerminalForWeb callbacks through WebServer and message handlers
- Expand WebTerminal component with full xterm.js integration, fit addon, and resize handling
- Wire up terminal resize messages through WebSocket
- Include session CWD in detail responses for terminal working directory
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds end-to-end terminal control and tool-event broadcasting: PTY spawn/resize/write/kill callbacks, broadcasting of terminal and tool events to web clients, mobile terminal UI and integrations, resizable mobile panels, file-tree WebSocket handler, and related type/handler expansions across backend, web-server, and mobile UI. Changes
Sequence Diagram(s)sequenceDiagram
participant ProcessManager
participant ForwardingListener
participant WebServer
participant BroadcastService
participant WebClient
ProcessManager->>ForwardingListener: emit tool-execution event
ForwardingListener->>ForwardingListener: derive baseSessionId, tabId, toolLog
ForwardingListener->>WebServer: broadcastToolEvent(baseSessionId, tabId, toolLog)
WebServer->>BroadcastService: broadcastToolEvent(sessionId, tabId, toolLog)
BroadcastService->>WebClient: send 'tool_event' to subscribed clients
WebClient->>MobileApp: deliver ToolEventMessage
sequenceDiagram
participant MobileApp
participant WebSocket
participant WebServer
participant PTYSpawner
participant WebTerminal
MobileApp->>WebSocket: request switch to 'terminal'
WebSocket->>WebServer: switch_mode -> spawnTerminalForWeb
WebServer->>PTYSpawner: spawn/reuse PTY for "{sessionId}-terminal"
PTYSpawner-->>WebServer: return { success, pid }
WebServer->>WebSocket: send 'terminal_ready'
WebSocket->>MobileApp: receive terminal_ready
MobileApp->>WebTerminal: render terminal and send initial resize
PTYSpawner->>WebServer: onData -> WebServer broadcasts 'terminal_data'
WebSocket->>WebTerminal: receive terminal_data -> write to xterm
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR delivers a large set of web interface UX improvements to bring the web client to parity with the native desktop experience. The changes span the full stack: a new PTY-backed xterm.js terminal ( Key changes include:
Confidence Score: 4/5Safe to merge after fixing the drag-listener leak in useResizableWebPanel which can block all mouse input after closing a panel mid-drag. One P1 bug: drag event listeners and the resize overlay div are orphaned when a panel unmounts during an active resize, which blocks all page mouse input. The two P2 findings (uncapped maxDepth, unrestricted dirPath) are low-severity given the security-token-gated WebSocket, and the echo-suppression heuristic is a quality issue rather than a correctness one. All other changes are well-structured, properly cleaned up, and consistent with existing patterns. src/web/hooks/useResizableWebPanel.ts — drag cleanup on unmount; src/main/web-server/handlers/messageHandlers.ts — maxDepth cap and path validation in handleGetFileTree. Important Files Changed
Sequence DiagramsequenceDiagram
participant WC as Web Client (xterm.js)
participant WS as WebSocket Server
participant MH as MessageHandlers
participant PM as ProcessManager (PTY)
participant DL as DataListener
WC->>WS: switch_mode {mode: "terminal"}
WS->>MH: handleSwitchMode
MH->>PM: spawnTerminalForWeb(sessionId-terminal, cwd)
PM-->>MH: {success, pid}
MH-->>WC: terminal_ready {sessionId}
MH-->>WC: mode_switch_result {success}
Note over WC: WebTerminal mounts, 100ms timer fires
WC->>WS: terminal_resize {cols, rows}
WS->>PM: resize(sessionId-terminal, cols, rows)
WC->>WS: terminal_write {data}
WS->>PM: write(sessionId-terminal, data)
PM->>DL: onData(sessionId-terminal, rawData)
DL->>WS: broadcastToSessionClients(sessionId, terminal_data)
WS-->>WC: terminal_data {data}
Note over WC: xterm.js renders ANSI output
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+51
to
+88
| const onResizeStart = useCallback( | ||
| (e: React.MouseEvent) => { | ||
| e.preventDefault(); | ||
| isResizing.current = true; | ||
| startX.current = e.clientX; | ||
| startWidth.current = width; | ||
|
|
||
| // Add a full-screen overlay to capture mouse events during drag | ||
| const overlay = document.createElement('div'); | ||
| overlay.id = 'resize-overlay'; | ||
| overlay.style.cssText = 'position:fixed;inset:0;z-index:9999;cursor:col-resize;'; | ||
| document.body.appendChild(overlay); | ||
|
|
||
| const onMouseMove = (ev: MouseEvent) => { | ||
| if (!isResizing.current || !panelRef.current) return; | ||
| const delta = side === 'left' ? ev.clientX - startX.current : startX.current - ev.clientX; | ||
| const newWidth = Math.max(minWidth, Math.min(maxWidth, startWidth.current + delta)); | ||
| // Direct DOM manipulation for performance (no React re-renders during drag) | ||
| panelRef.current.style.width = `${newWidth}px`; | ||
| }; | ||
|
|
||
| const onMouseUp = (ev: MouseEvent) => { | ||
| isResizing.current = false; | ||
| document.removeEventListener('mousemove', onMouseMove); | ||
| document.removeEventListener('mouseup', onMouseUp); | ||
| const el = document.getElementById('resize-overlay'); | ||
| if (el) el.remove(); | ||
|
|
||
| // Commit final width to React state + localStorage | ||
| const delta = side === 'left' ? ev.clientX - startX.current : startX.current - ev.clientX; | ||
| commitWidth(startWidth.current + delta); | ||
| }; | ||
|
|
||
| document.addEventListener('mousemove', onMouseMove); | ||
| document.addEventListener('mouseup', onMouseUp); | ||
| }, | ||
| [side, width, minWidth, maxWidth, commitWidth] | ||
| ); |
There was a problem hiding this comment.
onMouseMove and onMouseUp are added to document inside onResizeStart, but they are only removed when mouseup fires. If the panel is closed while the user is actively dragging (via keyboard shortcut, mobile swipe, parent re-render, or setShowLeftPanel(false)) the two listeners leak permanently and the resize-overlay div remains in the DOM — blocking all pointer events across the entire page.
A useEffect cleanup that removes the listeners and overlay on unmount is needed:
const activeListenersRef = useRef<{
move: (ev: MouseEvent) => void;
up: (ev: MouseEvent) => void;
} | null>(null);
// in onResizeStart, assign before attaching:
activeListenersRef.current = { move: onMouseMove, up: onMouseUp };
document.addEventListener('mousemove', onMouseMove);
document.addEventListener('mouseup', onMouseUp);Then add a cleanup effect:
useEffect(() => {
return () => {
if (activeListenersRef.current) {
document.removeEventListener('mousemove', activeListenersRef.current.move);
document.removeEventListener('mouseup', activeListenersRef.current.up);
document.getElementById('resize-overlay')?.remove();
activeListenersRef.current = null;
}
};
}, []);
Comment on lines
+989
to
+997
| private handleGetFileTree(client: WebClient, message: WebClientMessage): void { | ||
| const sessionId = message.sessionId as string; | ||
| const dirPath = message.path as string; | ||
| const maxDepth = (message.maxDepth as number) || 3; | ||
|
|
||
| if (!dirPath) { | ||
| this.sendError(client, 'Missing path for get_file_tree'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
dirPath and maxDepth lack server-side validation
Two hardening gaps in handleGetFileTree:
-
Unrestricted path:
dirPathis taken directly from the client message without validating it is within an expected root (e.g., the session'scwd). Any authenticated web client can request a file tree rooted at/,~/.ssh,/etc, etc. -
Uncapped
maxDepth: The client sendsmaxDepth: 3today, but there is no server-side cap. A client sendingmaxDepth: 100triggers deeply recursivebuildFileTreecalls that could exhaust memory or the call stack on large directory trees.
const MAX_ALLOWED_DEPTH = 5;
const maxDepth = Math.min((message.maxDepth as number) || 3, MAX_ALLOWED_DEPTH);
const sessionDetail = this.callbacks.getSessionDetail?.(sessionId);
const allowedRoot = sessionDetail?.cwd;
if (allowedRoot && !dirPath.startsWith(allowedRoot)) {
this.sendError(client, 'Path is outside allowed project directory');
return;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/web/hooks/useMobileSessionManagement.ts (2)
282-315:⚠️ Potential issue | 🟡 MinorDon’t let an aborted fetch clear the current loading state.
When the session/tab changes quickly, the old request is aborted in cleanup but its
finallystill runs and flipsisLoadingLogstofalsewhile the next request is already in flight. That causes the loading spinner to flicker and the empty state can flash briefly.💡 Suggested fix
useEffect(() => { if (!activeSessionId || isOffline) { setSessionLogs({ aiLogs: [], shellLogs: [] }); + setIsLoadingLogs(false); return; } const controller = new AbortController(); + let isCurrent = true; const fetchSessionLogs = async () => { setIsLoadingLogs(true); try { // Pass tabId explicitly to avoid race conditions with activeTabId sync const tabParam = activeTabId ? `?tabId=${activeTabId}` : ''; const apiUrl = buildApiUrl(`/session/${activeSessionId}${tabParam}`); const response = await fetch(apiUrl, { signal: controller.signal }); if (response.ok) { const data = await response.json(); const session = data.session; setSessionLogs({ aiLogs: session?.aiLogs || [], shellLogs: session?.shellLogs || [], }); webLogger.debug('Fetched session logs:', 'Mobile', { aiLogs: session?.aiLogs?.length || 0, shellLogs: session?.shellLogs?.length || 0, requestedTabId: activeTabId, returnedTabId: session?.activeTabId, }); } } catch (err) { if ((err as Error).name === 'AbortError') return; webLogger.error('Failed to fetch session logs', 'Mobile', err); } finally { - setIsLoadingLogs(false); + if (isCurrent) { + setIsLoadingLogs(false); + } } }; fetchSessionLogs(); - return () => controller.abort(); + return () => { + isCurrent = false; + controller.abort(); + }; }, [activeSessionId, activeTabId, isOffline]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/hooks/useMobileSessionManagement.ts` around lines 282 - 315, The effect's fetchSessionLogs finally block always calls setIsLoadingLogs(false), so when an in-flight request is aborted it clears loading even though a new request may have started; update fetchSessionLogs (in useMobileSessionManagement.ts) to skip clearing loading when the request was aborted: in the catch/ finally logic check the AbortController signal (controller.signal.aborted) or the caught error name === 'AbortError' and avoid calling setIsLoadingLogs(false) when aborted, ensuring setIsLoadingLogs is only cleared for non-abort completions.
638-668:⚠️ Potential issue | 🟠 MajorUse
tabIdto mark the emitting tab unread instead of every tab or none.For another session, this marks all tabs unread even when only one AI tab produced output. For a background tab in the current session, the early return drops the chunk without marking that tab unread at all. That makes the new unread badges unreliable as soon as multiple AI tabs are active.
💡 Suggested fix
if (currentActiveId !== sessionId) { webLogger.debug('Marking session as unread - not active session', 'Mobile', { sessionId, activeSessionId: currentActiveId, }); setSessions((prev) => prev.map((s) => { if (s.id !== sessionId) return s; + const unreadTabId = source === 'ai' ? tabId ?? s.activeTabId : s.activeTabId; return { ...s, - aiTabs: s.aiTabs?.map((tab) => ({ - ...tab, - hasUnread: true, - })), + aiTabs: s.aiTabs?.map((tab) => ({ + ...tab, + hasUnread: unreadTabId ? tab.id === unreadTabId || tab.hasUnread : tab.hasUnread, + })), }; }) ); return; } // For AI output with tabId, only update if this is the active tab // This prevents output from newly created tabs appearing in the wrong tab's logs if (source === 'ai' && tabId && currentActiveTabId && tabId !== currentActiveTabId) { webLogger.debug('Skipping output - not active tab', 'Mobile', { sessionId, outputTabId: tabId, activeTabId: currentActiveTabId, }); + setSessions((prev) => + prev.map((s) => + s.id === sessionId + ? { + ...s, + aiTabs: s.aiTabs?.map((tab) => ({ + ...tab, + hasUnread: tab.id === tabId || tab.hasUnread, + })), + } + : s + ) + ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/hooks/useMobileSessionManagement.ts` around lines 638 - 668, The handler incorrectly marks all aiTabs unread or none: when currentActiveId !== sessionId you should mark only the emitting tab (use tabId) instead of setting hasUnread on every tab, and when source === 'ai' and tabId exists but isn't the active tab, instead of returning early and dropping the chunk you should set that specific ai tab's hasUnread = true via setSessions (update s.aiTabs where tab.id === tabId) and then return; update the setSessions call (and the early-return branch) around currentActiveId, sessionId, setSessions, s.aiTabs, tabId and currentActiveTabId to only touch the matching tab's hasUnread flag.src/web/mobile/App.tsx (2)
2405-2425:⚠️ Potential issue | 🟡 MinorReuse
handleAutoRunOpenSetup()for the palette launch path.This is the one path that opens
AutoRunSetupSheetwithout callingloadAutoRunDocuments(activeSessionId), so opening Auto Run from Cmd+K can show stale or empty documents.Also add
handleAutoRunOpenSetupto thecommandPaletteActionsuseMemodependency list when you switch this action over.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/mobile/App.tsx` around lines 2405 - 2425, The palette action with id 'autorun-launch' currently calls setShowAutoRunSetup(true) directly and can open AutoRunSetupSheet without loading documents; change its action to call the existing handleAutoRunOpenSetup() so loadAutoRunDocuments(activeSessionId) runs before showing the sheet, and update the commandPaletteActions useMemo dependency array to include handleAutoRunOpenSetup so the memo updates correctly when that handler changes. Ensure the action still respects available: () => !!activeSessionId && !currentAutoRunState?.isRunning and remove the direct setShowAutoRunSetup usage here.
3078-3084:⚠️ Potential issue | 🟠 MajorTab search should switch back to AI mode before selecting an AI tab.
The direct tab-bar path already does this at Lines 3004-3010, but the search modal still calls
handleSelectTabdirectly. From terminal mode, picking a tab here changes the active AI tab while leaving the UI in terminal mode.💡 Suggested fix
<TabSearchModal tabs={activeSession.aiTabs} activeTabId={activeSession.activeTabId} - onSelectTab={handleSelectTab} + onSelectTab={(tabId) => { + if (currentInputMode !== 'ai') { + handleModeToggle('ai'); + } + handleSelectTab(tabId); + }} onClose={handleCloseTabSearch} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/mobile/App.tsx` around lines 3078 - 3084, When wiring TabSearchModal, ensure selecting a tab forces the UI back into AI mode before activating the tab: change the onSelectTab passed to <TabSearchModal> so it wraps handleSelectTab with the same mode-switch used by the direct tab-bar path (i.e., first set the UI/mode to "ai" using the same routine used earlier, then call handleSelectTab(tabId) and finally call handleCloseTabSearch()); update the onSelectTab prop to this wrapper so selecting from the search modal behaves identical to the direct tab-bar selection.
🧹 Nitpick comments (7)
src/main/web-server/WebServer.ts (1)
842-859: Extract the sharedtoolLogcontract.This inline payload shape is now duplicated here and in
src/main/web-server/services/broadcastService.ts. The next field change can easily update one side but not the other. A sharedToolLog/ToolEventPayloadtype would keep the websocket contract aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/web-server/WebServer.ts` around lines 842 - 859, The inline toolLog payload type used in broadcastToolEvent and the duplicate in broadcastService.broadcastToolEvent should be extracted into a shared type (e.g., ToolLog or ToolEventPayload); create that exported interface/type and use it in the WebServer.broadcastToolEvent method signature and in BroadcastService.broadcastToolEvent so both reference the same type, then update any imports/exports to use the new shared type and remove the inline anonymous shape from the method signatures to keep the websocket contract in sync.src/web/mobile/RightDrawer.tsx (1)
317-327: Potential performance issue with recursive filter matching.The
matchesFilterfunction recursively traverses all children when checking if a folder matches. For large trees, this could cause re-renders to be slow sincefilterLowerchanges triggeruseMemorecalculation.Consider memoizing the filtered tree result instead of checking
matchesFilterduring render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/mobile/RightDrawer.tsx` around lines 317 - 327, The recursive matchesFilter callback (matchesFilter, created with useCallback and depending on filterLower) causes repeated deep traversal of node.children on each render; replace per-node recursion during render by computing and memoizing a filtered tree once when filterLower changes (e.g., use useMemo to produce filteredRoot or getFilteredTree from the original tree) and then render from that memoized filtered tree instead of calling matchesFilter for each node; ensure the memoization logic preserves folder structure and only recurses inside useMemo so matchesFilter (if kept) no longer runs per-render over the whole tree.src/main/web-server/web-server-factory.ts (1)
305-311: Returningpid: 0when PTY already exists may cause confusion.When a PTY already exists, returning
{ success: true, pid: 0 }makes it hard for callers to distinguish between "successfully reused existing PTY" vs "spawned new PTY with pid 0" (which would be unusual). Consider returning the actual PID of the existing process or a distinct response shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/web-server/web-server-factory.ts` around lines 305 - 311, The handler currently returns { success: true, pid: 0 } when processManager.get(terminalSessionId) is truthy, which is ambiguous; instead retrieve the existing PTY's real PID from processManager (or its stored object) and return it (e.g., { success: true, pid: existingPid }) or change the response shape to explicitly indicate reuse (e.g., { success: true, reused: true, pid: existingPid }). Update the block that checks processManager.get(terminalSessionId) in web-server-factory.ts to fetch the PID from the stored entry for terminalSessionId (use the same identifier names processManager and terminalSessionId) and return the clarified response shape, and update any callers to handle the new field if necessary.src/web/hooks/useResizableWebPanel.ts (1)
90-90: ReturningisResizingas a ref may confuse consumers.The hook returns
isResizingwhich is aRefObject<boolean>, but consumers might expect a boolean value. Consider returningisResizing.currentwrapped in state if reactive updates are needed, or document the ref behavior clearly.src/web/mobile/RightPanel.tsx (1)
18-36: Unused prop:onAutoRunOpenDocumentis declared but never used.The
onAutoRunOpenDocumentprop is declared inRightPanelProps(line 26) andsend(line 29), but neither is destructured in the component function (lines 48-63) nor passed to any child component.If these props are intended for future use, consider removing them from the interface to avoid confusion. If they should be wired up, the
AutoRunTabContentmight need them.♻️ Proposed cleanup
export interface RightPanelProps { sessionId: string; activeTab?: RightDrawerTab; autoRunState: AutoRunState | null; gitStatus: UseGitStatusReturn; onClose: () => void; onFileSelect?: (path: string) => void; projectPath?: string; - onAutoRunOpenDocument?: (filename: string) => void; onAutoRunOpenSetup?: () => void; sendRequest: UseWebSocketReturn['sendRequest']; - send: UseWebSocketReturn['send']; onViewDiff?: (filePath: string) => void; panelRef?: React.RefObject<HTMLDivElement>; width?: number; onResizeStart?: (e: React.MouseEvent) => void; isFullScreen?: boolean; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/mobile/RightPanel.tsx` around lines 18 - 36, RightPanelProps declares onAutoRunOpenDocument (and send) but the RightPanel component doesn't destructure or pass them through; either remove these unused props from RightPanelProps to clean up the interface, or wire them up by adding onAutoRunOpenDocument (and send if needed) to the RightPanel component's props destructuring and forwarding onAutoRunOpenDocument to the AutoRunTabContent (or the appropriate child that expects an "open document" callback). Update usages of AutoRunTabContent to accept the forwarded prop name if necessary and remove any dead declarations if you choose the cleanup route.src/main/process-listeners/forwarding-listeners.ts (1)
42-42: Consider handling missing tabId more explicitly.When
REGEX_AI_TAB_IDdoesn't match,tabIdis set to an empty string. Per thebroadcastToolEventsignature inWebServer.ts,tabIdis a required string parameter. An empty string might cause issues with client-side routing.Consider logging a warning when tabId extraction fails or ensuring the regex always matches for valid AI session IDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-listeners/forwarding-listeners.ts` at line 42, The code currently sets tabId to '' when REGEX_AI_TAB_ID fails to match (const tabId = tabIdMatch ? tabIdMatch[1] : ''), but broadcastToolEvent in WebServer.ts requires a valid non-empty tabId; update the logic in forwarding-listeners.ts to handle missing tabId explicitly by either logging a warning/error via the existing logger and skipping/aborting the broadcast or by deriving a safe fallback before calling broadcastToolEvent; reference REGEX_AI_TAB_ID, tabIdMatch/tabId and the broadcastToolEvent call so you can add the warning and early return (or a deterministic fallback) to avoid passing an empty string to broadcastToolEvent.src/main/process-listeners/__tests__/forwarding-listeners.test.ts (1)
23-30: Test dependencies updated correctly, but web broadcast path is untested.The mock dependencies now match the updated function signature. However, since
getWebServerreturnsnull, the web broadcast code path in thetool-executionhandler isn't covered by these tests.Consider adding a separate test case with a mock web server to verify
broadcastToolEventis called correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-listeners/__tests__/forwarding-listeners.test.ts` around lines 23 - 30, Add a new test that supplies mockDeps.getWebServer returning a fake web server with a broadcastToolEvent spy, then emit the "tool-execution" message to exercise the web broadcast path and assert broadcastToolEvent was called with the expected payload; specifically update the test suite to include a case where mockDeps.getWebServer = () => ({ broadcastToolEvent: jest.fn() }) (or equivalent), trigger the handler that processes "tool-execution" and verify the fake server's broadcastToolEvent was invoked, while keeping existing tests that use getWebServer = () => null to cover the non-web path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/web-server/services/broadcastService.ts`:
- Around line 444-467: The broadcastToolEvent currently calls broadcastToSession
which also sends to clients with no subscribedSessionId and thus fans out
tool_event to dashboards; instead restrict delivery to only clients actively
viewing the given session/tab. Update broadcastToolEvent to send only to clients
whose subscribedSessionId === sessionId (and if you track tab subscriptions,
subscribedTabId === tabId) rather than using broadcastToSession, or add/use a
helper (e.g., broadcastToSubscribedClients/broadcastToSessionTabSubscribers)
that filters by subscribedSessionId/subscribedTabId; ensure the toolLog
(including toolLog.metadata.toolState.input) is only sent to those filtered
subscribers.
In `@src/web/hooks/useIsMobile.ts`:
- Around line 28-46: The effect in useIsMobile only updates isMobile on resize
so changing the breakpoint prop doesn't update the hook until a user resizes;
modify the effect in useIsMobile to also synchronize isMobile when breakpoint
changes by running the same check immediately (e.g. call
setIsMobile(window.innerWidth <= breakpoint) or invoke handleResize once) before
registering the resize listener and debounce timer, and keep existing cleanup of
timerRef and event listener in the returned function.
In `@src/web/hooks/useMobileSessionManagement.ts`:
- Around line 562-573: The current onActiveSessionChanged handler suppresses all
server-driven active-session updates for 2s after any local selection; change it
to only suppress echoes for the same session by tracking the last locally
selected session id and comparing it to the incoming sessionId. Add a new ref
(e.g. lastLocalSelectedSessionIdRef) and set it whenever the user locally
selects a session (alongside updating lastLocalSelectionRef), then update
onActiveSessionChanged to only early-return when timeSinceLocalSelect < 2000 AND
sessionId === lastLocalSelectedSessionIdRef.current; keep the existing
webLogger.debug message but include the session id check so legitimate
desktop-driven switches to different sessions are not dropped.
In `@src/web/hooks/useResizableWebPanel.ts`:
- Around line 51-88: The onResizeStart handler installs document-level listeners
and an overlay but never ensures they are removed if the component unmounts or a
new drag starts; update the hook to clean up those listeners and the overlay:
track the onMouseMove/onMouseUp handlers (and overlay id 'resize-overlay') in
refs or in a scoped variable so you can remove them later, add a useEffect
cleanup that checks isResizing/current handlers to call
document.removeEventListener('mousemove', onMouseMove) and
removeEventListener('mouseup', onMouseUp) and remove the overlay element, and
ensure onResizeStart also removes any existing handlers/overlay before attaching
new ones; reference functions/vars to update: onResizeStart, isResizing, startX,
startWidth, panelRef, commitWidth.
In `@src/web/mobile/App.tsx`:
- Around line 1402-1418: The terminal handlers currently act on every PTY event
regardless of which terminal is mounted; fix by tracking the currently mounted
session id in a ref (e.g., mountedSessionIdRef) and gating both onTerminalData
and onTerminalReady on that ref: use the sessionId parameter (rename _sessionId
to sessionId or read it) and only call webTerminalRef.current?.write(...) or
fitAndGetSize()/wsSendRef.current... when sessionId ===
mountedSessionIdRef.current; ensure mountedSessionIdRef is set when the
WebTerminal mounts and cleared on unmount so stray PTY messages are ignored.
In `@src/web/mobile/CommandInputBar.tsx`:
- Around line 40-46: CommandInputBar's terminal-mode branches are unreachable
because the mobile App still passes a hardcoded inputMode={'ai'} and no
mode-toggle callback after replacing the action row; update the mobile App usage
to pass the real inputMode state and a mode-toggle handler (the same props
CommandInputBar expects, e.g., inputMode and onToggleInputMode or equivalent) or
reintroduce a visible mode switch in the action row so the component's inputMode
=== 'terminal' branches can be reached; locate CallSite in
src/web/mobile/App.tsx where CommandInputBar is instantiated and change the prop
wiring to forward the parent state and toggle callback instead of the hardcoded
'ai' value (also fix the other occurrences mentioned: lines ~137-142, 171-173,
760-767).
In `@src/web/mobile/CommandInputButtons.tsx`:
- Around line 561-595: The button is a three-state toggle but only exposes a
generic role; update the JSX for the button in CommandInputButtons (the element
using props isOff and isSticky and handler handleClick) to include an
aria-pressed attribute set to convey mixed/on/off state (use
aria-pressed={isSticky ? 'mixed' : !isOff}) so screen readers announce
sticky/on/off correctly; keep existing aria-label/title and other props
unchanged.
In `@src/web/mobile/LeftPanel.tsx`:
- Around line 583-623: The expander is implemented as a <span role="button">
inside the session row <button>, creating nested interactive controls and
removing it from the tab order; change the expander to a native button
(type="button") with proper aria-expanded and aria-label, ensure it uses the
existing toggleWorktrees(session.id) onClick and reflects isWorktreeExpanded,
include the visual contents (GitBranchIcon, children.length and chevron SVG),
and move or render this button as a sibling (not a descendant) of the session
row button so it can receive keyboard focus and avoid nested button semantics.
In `@src/web/mobile/TabBar.tsx`:
- Around line 793-881: The close "x" inside the Terminal tab is currently a
non-focusable <span> nested within a <button>, which is invalid and not
keyboard-accessible; change this to a separate focusable button element (use
type="button") rendered alongside the main Terminal tab button like the existing
Tab + close-button pattern, wire its onClick to call e.stopPropagation(),
triggerHaptic(HAPTIC_PATTERNS.tap) and onCloseTerminal(), ensure it only renders
when onCloseTerminal && inputMode === 'terminal', add an accessible aria-label
(e.g., "Close terminal"), and copy the visual styles
(width/height/borderRadius/opacity) so it visually matches the previous span
while being keyboard-focusable.
---
Outside diff comments:
In `@src/web/hooks/useMobileSessionManagement.ts`:
- Around line 282-315: The effect's fetchSessionLogs finally block always calls
setIsLoadingLogs(false), so when an in-flight request is aborted it clears
loading even though a new request may have started; update fetchSessionLogs (in
useMobileSessionManagement.ts) to skip clearing loading when the request was
aborted: in the catch/ finally logic check the AbortController signal
(controller.signal.aborted) or the caught error name === 'AbortError' and avoid
calling setIsLoadingLogs(false) when aborted, ensuring setIsLoadingLogs is only
cleared for non-abort completions.
- Around line 638-668: The handler incorrectly marks all aiTabs unread or none:
when currentActiveId !== sessionId you should mark only the emitting tab (use
tabId) instead of setting hasUnread on every tab, and when source === 'ai' and
tabId exists but isn't the active tab, instead of returning early and dropping
the chunk you should set that specific ai tab's hasUnread = true via setSessions
(update s.aiTabs where tab.id === tabId) and then return; update the setSessions
call (and the early-return branch) around currentActiveId, sessionId,
setSessions, s.aiTabs, tabId and currentActiveTabId to only touch the matching
tab's hasUnread flag.
In `@src/web/mobile/App.tsx`:
- Around line 2405-2425: The palette action with id 'autorun-launch' currently
calls setShowAutoRunSetup(true) directly and can open AutoRunSetupSheet without
loading documents; change its action to call the existing
handleAutoRunOpenSetup() so loadAutoRunDocuments(activeSessionId) runs before
showing the sheet, and update the commandPaletteActions useMemo dependency array
to include handleAutoRunOpenSetup so the memo updates correctly when that
handler changes. Ensure the action still respects available: () =>
!!activeSessionId && !currentAutoRunState?.isRunning and remove the direct
setShowAutoRunSetup usage here.
- Around line 3078-3084: When wiring TabSearchModal, ensure selecting a tab
forces the UI back into AI mode before activating the tab: change the
onSelectTab passed to <TabSearchModal> so it wraps handleSelectTab with the same
mode-switch used by the direct tab-bar path (i.e., first set the UI/mode to "ai"
using the same routine used earlier, then call handleSelectTab(tabId) and
finally call handleCloseTabSearch()); update the onSelectTab prop to this
wrapper so selecting from the search modal behaves identical to the direct
tab-bar selection.
---
Nitpick comments:
In `@src/main/process-listeners/__tests__/forwarding-listeners.test.ts`:
- Around line 23-30: Add a new test that supplies mockDeps.getWebServer
returning a fake web server with a broadcastToolEvent spy, then emit the
"tool-execution" message to exercise the web broadcast path and assert
broadcastToolEvent was called with the expected payload; specifically update the
test suite to include a case where mockDeps.getWebServer = () => ({
broadcastToolEvent: jest.fn() }) (or equivalent), trigger the handler that
processes "tool-execution" and verify the fake server's broadcastToolEvent was
invoked, while keeping existing tests that use getWebServer = () => null to
cover the non-web path.
In `@src/main/process-listeners/forwarding-listeners.ts`:
- Line 42: The code currently sets tabId to '' when REGEX_AI_TAB_ID fails to
match (const tabId = tabIdMatch ? tabIdMatch[1] : ''), but broadcastToolEvent in
WebServer.ts requires a valid non-empty tabId; update the logic in
forwarding-listeners.ts to handle missing tabId explicitly by either logging a
warning/error via the existing logger and skipping/aborting the broadcast or by
deriving a safe fallback before calling broadcastToolEvent; reference
REGEX_AI_TAB_ID, tabIdMatch/tabId and the broadcastToolEvent call so you can add
the warning and early return (or a deterministic fallback) to avoid passing an
empty string to broadcastToolEvent.
In `@src/main/web-server/web-server-factory.ts`:
- Around line 305-311: The handler currently returns { success: true, pid: 0 }
when processManager.get(terminalSessionId) is truthy, which is ambiguous;
instead retrieve the existing PTY's real PID from processManager (or its stored
object) and return it (e.g., { success: true, pid: existingPid }) or change the
response shape to explicitly indicate reuse (e.g., { success: true, reused:
true, pid: existingPid }). Update the block that checks
processManager.get(terminalSessionId) in web-server-factory.ts to fetch the PID
from the stored entry for terminalSessionId (use the same identifier names
processManager and terminalSessionId) and return the clarified response shape,
and update any callers to handle the new field if necessary.
In `@src/main/web-server/WebServer.ts`:
- Around line 842-859: The inline toolLog payload type used in
broadcastToolEvent and the duplicate in broadcastService.broadcastToolEvent
should be extracted into a shared type (e.g., ToolLog or ToolEventPayload);
create that exported interface/type and use it in the
WebServer.broadcastToolEvent method signature and in
BroadcastService.broadcastToolEvent so both reference the same type, then update
any imports/exports to use the new shared type and remove the inline anonymous
shape from the method signatures to keep the websocket contract in sync.
In `@src/web/mobile/RightDrawer.tsx`:
- Around line 317-327: The recursive matchesFilter callback (matchesFilter,
created with useCallback and depending on filterLower) causes repeated deep
traversal of node.children on each render; replace per-node recursion during
render by computing and memoizing a filtered tree once when filterLower changes
(e.g., use useMemo to produce filteredRoot or getFilteredTree from the original
tree) and then render from that memoized filtered tree instead of calling
matchesFilter for each node; ensure the memoization logic preserves folder
structure and only recurses inside useMemo so matchesFilter (if kept) no longer
runs per-render over the whole tree.
In `@src/web/mobile/RightPanel.tsx`:
- Around line 18-36: RightPanelProps declares onAutoRunOpenDocument (and send)
but the RightPanel component doesn't destructure or pass them through; either
remove these unused props from RightPanelProps to clean up the interface, or
wire them up by adding onAutoRunOpenDocument (and send if needed) to the
RightPanel component's props destructuring and forwarding onAutoRunOpenDocument
to the AutoRunTabContent (or the appropriate child that expects an "open
document" callback). Update usages of AutoRunTabContent to accept the forwarded
prop name if necessary and remove any dead declarations if you choose the
cleanup route.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 14234a4a-98d7-47c6-a021-1b04bec14f52
📒 Files selected for processing (21)
src/main/process-listeners/__tests__/forwarding-listeners.test.tssrc/main/process-listeners/data-listener.tssrc/main/process-listeners/forwarding-listeners.tssrc/main/process-manager/spawners/PtySpawner.tssrc/main/web-server/WebServer.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/services/broadcastService.tssrc/main/web-server/web-server-factory.tssrc/web/hooks/useIsMobile.tssrc/web/hooks/useMobileSessionManagement.tssrc/web/hooks/useResizableWebPanel.tssrc/web/hooks/useWebSocket.tssrc/web/mobile/App.tsxsrc/web/mobile/CommandInputBar.tsxsrc/web/mobile/CommandInputButtons.tsxsrc/web/mobile/LeftPanel.tsxsrc/web/mobile/MessageHistory.tsxsrc/web/mobile/RightDrawer.tsxsrc/web/mobile/RightPanel.tsxsrc/web/mobile/TabBar.tsxsrc/web/mobile/WebTerminal.tsx
Comment on lines
+793
to
+881
| {/* Terminal tab */} | ||
| {onSelectTerminal && ( | ||
| <button | ||
| onClick={() => { | ||
| triggerHaptic(HAPTIC_PATTERNS.tap); | ||
| onSelectTerminal(); | ||
| }} | ||
| style={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| gap: '6px', | ||
| padding: '6px 10px', | ||
| borderTopLeftRadius: '6px', | ||
| borderTopRightRadius: '6px', | ||
| borderTop: | ||
| inputMode === 'terminal' ? `1px solid ${colors.border}` : '1px solid transparent', | ||
| borderLeft: | ||
| inputMode === 'terminal' ? `1px solid ${colors.border}` : '1px solid transparent', | ||
| borderRight: | ||
| inputMode === 'terminal' ? `1px solid ${colors.border}` : '1px solid transparent', | ||
| borderBottom: | ||
| inputMode === 'terminal' ? `1px solid ${colors.bgMain}` : '1px solid transparent', | ||
| backgroundColor: inputMode === 'terminal' ? colors.bgMain : 'transparent', | ||
| color: inputMode === 'terminal' ? colors.textMain : colors.textDim, | ||
| fontSize: '12px', | ||
| fontWeight: inputMode === 'terminal' ? 600 : 400, | ||
| fontFamily: 'monospace', | ||
| cursor: 'pointer', | ||
| whiteSpace: 'nowrap', | ||
| transition: 'all 0.15s ease', | ||
| marginBottom: inputMode === 'terminal' ? '-1px' : '0', | ||
| zIndex: inputMode === 'terminal' ? 1 : 0, | ||
| touchAction: 'pan-x pan-y', | ||
| WebkitTapHighlightColor: 'transparent', | ||
| userSelect: 'none', | ||
| WebkitUserSelect: 'none', | ||
| flexShrink: 0, | ||
| }} | ||
| > | ||
| {/* Terminal icon */} | ||
| <svg | ||
| width="12" | ||
| height="12" | ||
| viewBox="0 0 24 24" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| strokeWidth="2" | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| > | ||
| <polyline points="4 17 10 11 4 5" /> | ||
| <line x1="12" y1="19" x2="20" y2="19" /> | ||
| </svg> | ||
| Terminal | ||
| {onCloseTerminal && inputMode === 'terminal' && ( | ||
| <span | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| triggerHaptic(HAPTIC_PATTERNS.tap); | ||
| onCloseTerminal(); | ||
| }} | ||
| style={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| width: '16px', | ||
| height: '16px', | ||
| borderRadius: '4px', | ||
| marginLeft: '4px', | ||
| cursor: 'pointer', | ||
| opacity: 0.6, | ||
| }} | ||
| > | ||
| <svg | ||
| width="10" | ||
| height="10" | ||
| viewBox="0 0 24 24" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| strokeWidth="2.5" | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| > | ||
| <line x1="18" y1="6" x2="6" y2="18" /> | ||
| <line x1="6" y1="6" x2="18" y2="18" /> | ||
| </svg> | ||
| </span> | ||
| )} | ||
| </button> |
There was a problem hiding this comment.
Make the terminal close affordance a real button.
The x is a clickable <span> nested inside the terminal tab <button>. It's not keyboard-focusable, and nested interactive content inside a button is invalid HTML, so keyboard users can't close the terminal tab from this UI. Please mirror the existing Tab wrapper + separate close-button pattern here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/mobile/TabBar.tsx` around lines 793 - 881, The close "x" inside the
Terminal tab is currently a non-focusable <span> nested within a <button>, which
is invalid and not keyboard-accessible; change this to a separate focusable
button element (use type="button") rendered alongside the main Terminal tab
button like the existing Tab + close-button pattern, wire its onClick to call
e.stopPropagation(), triggerHaptic(HAPTIC_PATTERNS.tap) and onCloseTerminal(),
ensure it only renders when onCloseTerminal && inputMode === 'terminal', add an
accessible aria-label (e.g., "Close terminal"), and copy the visual styles
(width/height/borderRadius/opacity) so it visually matches the previous span
while being keyboard-focusable.
Greptile: fix resize listener leak on unmount, add file tree path/depth validation. CodeRabbit: scope tool_event broadcasts to subscribed clients, sync useIsMobile on breakpoint change, fix session echo suppression to allow cross-session switches, guard terminal events by session ID, wire real inputMode to CommandInputBar/MessageHistory, add aria-pressed to toggle, fix nested interactive elements in worktree expander and terminal close button. User issues: add swipe-to-close for right panel, transform Auto Run file paths into documents with task counts, persist group collapse state across sidebar open/close. Update all affected tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/__tests__/main/web-server/web-server-factory.test.ts (1)
73-76: Add explicit registration assertions for the new terminal callbacks.These mock additions are correct, but the suite still doesn’t verify that
createWebServerFactoryactually registers all four terminal setters. Please add expectations in thecallback registrationsblock for:
setWriteToTerminalCallback,setResizeTerminalCallback,setSpawnTerminalForWebCallback, andsetKillTerminalForWebCallback, to prevent silent regressions in PTY wiring.Suggested test addition
describe('callback registrations', () => { @@ it('should register file and auto-run callbacks', () => { expect(server.setOpenFileTabCallback).toHaveBeenCalled(); expect(server.setRefreshFileTreeCallback).toHaveBeenCalled(); expect(server.setRefreshAutoRunDocsCallback).toHaveBeenCalled(); expect(server.setConfigureAutoRunCallback).toHaveBeenCalled(); }); + + it('should register terminal callbacks', () => { + expect(server.setWriteToTerminalCallback).toHaveBeenCalled(); + expect(server.setResizeTerminalCallback).toHaveBeenCalled(); + expect(server.setSpawnTerminalForWebCallback).toHaveBeenCalled(); + expect(server.setKillTerminalForWebCallback).toHaveBeenCalled(); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/web-server/web-server-factory.test.ts` around lines 73 - 76, The test currently defines mocks for setWriteToTerminalCallback, setResizeTerminalCallback, setSpawnTerminalForWebCallback, and setKillTerminalForWebCallback but doesn't assert they were registered; update the "callback registrations" test block in the createWebServerFactory spec to add expectations that each mock was invoked (e.g. expect(setWriteToTerminalCallback).toHaveBeenCalled(), expect(setResizeTerminalCallback).toHaveBeenCalled(), expect(setSpawnTerminalForWebCallback).toHaveBeenCalled(), expect(setKillTerminalForWebCallback).toHaveBeenCalled()) after calling createWebServerFactory so the suite verifies the factory actually registers all four terminal setters.src/renderer/App.tsx (1)
2045-2047: Consider adding a debug log for per-file read failures.The error isolation pattern is sound—one file failing shouldn't abort the entire docs fetch. However, the completely silent catch makes debugging harder when files unexpectedly fail to load. A minimal log would preserve the graceful degradation while aiding troubleshooting.
🔧 Suggested improvement
} catch { - // If reading fails, leave counts at 0 + // Expected: file may be deleted, renamed, or inaccessible + console.debug('[Remote] getAutoRunDocs: skipped file (read failed)', filePath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 2045 - 2047, The silent catch in src/renderer/App.tsx (the per-file read try/catch that currently “leaves counts at 0”) should log the failure for debugging: inside that catch block, capture the error and the file identifier (e.g., filename or doc id from the surrounding scope) and call the project's debug logger (or console.debug if no logger available) with a concise message and the error object so per-file read failures are visible without changing behavior. Ensure you reference the same variables used in the try (filename/docId) to provide context.src/__tests__/main/process-listeners/forwarding-listeners.test.ts (1)
77-90: Add one positive test for the web broadcast branch.Line 25 stubs
getWebServertonullfor all cases, so the new web-path behavior fortool-executionisn’t asserted here. A single test with a non-null web server mock would close that gap and protect this new dependency surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/process-listeners/forwarding-listeners.test.ts` around lines 77 - 90, Add a new test that covers the web broadcast branch by stubbing mockDeps.getWebServer to return a non-null mock web server with a jest.fn() broadcast method, then call setupForwardingListeners(mockProcessManager, mockDeps), retrieve the 'tool-execution' handler from eventHandlers and invoke it with a test session id and payload, and finally assert that the mock web server's broadcast was called with the expected channel/payload (and optionally that mockSafeSend was still called if both should run); reference setupForwardingListeners, eventHandlers, mockDeps.getWebServer, and the mock web server's broadcast in the test.src/__tests__/web/mobile/TabBar.test.tsx (1)
106-129: Assert real tab-bar chrome here.
container.firstChildonly proves that React rendered something; an empty wrapper or injected<style>would still pass. Assert one stable piece of chrome instead, such as the “New Tab” button or the tablist, so these cases keep catching regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/web/mobile/TabBar.test.tsx` around lines 106 - 129, Replace the weak assertion that merely checks container.firstChild with a stable DOM assertion that verifies a specific piece of chrome from the TabBar component (e.g., assert the presence of the "New Tab" button or the tablist). In the tests that render <TabBar ... tabs={[]} /> and <TabBar ... tabs={[defaultTab]} />, use testing-library queries such as getByRole('tablist') or getByText('New Tab') against the rendered output (the same render result used now) and assert those elements are present so the tests will fail if the actual chrome is removed or altered.src/__tests__/web/mobile/App.test.tsx (1)
195-220: Consider using stricter typing for the sessions parameter.The mock correctly implements the new LeftPanel interface. However, using
anyfor the session type in the map callback loses type safety.🔧 Suggested improvement for type safety
vi.mock('../../../web/mobile/LeftPanel', () => ({ LeftPanel: ({ sessions, activeSessionId, onSelectSession, onClose, }: { sessions: unknown[]; activeSessionId: string | null; onSelectSession: (id: string) => void; onClose: () => void; collapsedGroups: Set<string>; setCollapsedGroups: React.Dispatch<React.SetStateAction<Set<string>>>; }) => ( <div data-testid="left-panel"> - {sessions.map((s: any) => ( + {(sessions as { id: string; name: string }[]).map((s) => ( <button key={s.id} data-testid={`session-${s.id}`} onClick={() => onSelectSession(s.id)}> {s.name} </button> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/web/mobile/App.test.tsx` around lines 195 - 220, The mock LeftPanel uses sessions.map((s: any) => ...) which loses type safety; replace the any with a concrete session type (e.g., { id: string; name: string } or the existing Session type if available) by updating the mock prop signature for sessions and the map callback parameter in the LeftPanel mock so the compiler knows sessions items have id and name; adjust the sessions parameter type in the mocked LeftPanel declaration (or import/alias the real Session type) and use that type instead of any in the map.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/main/process-listeners/forwarding-listeners.test.ts`:
- Around line 23-30: The test fixture's mockDeps uses patterns.REGEX_AI_SUFFIX
and patterns.REGEX_AI_TAB_ID with [^-]+ which fails to match hyphenated
UUID-style tab IDs; update those two regexes in mockDeps to allow hyphens (e.g.,
use .+ or a character class that includes hyphen/digits/letters such as
[A-Za-z0-9-]+) so the mock mirrors real suffix/tab-id shapes and will catch
parsing regressions in the forwarding listeners tests (refer to mockDeps,
patterns.REGEX_AI_SUFFIX, and patterns.REGEX_AI_TAB_ID).
In `@src/web/hooks/useMobileSessionManagement.ts`:
- Around line 305-310: The spinner is being cleared in the finally block even
when the request was aborted; update the fetch flow in
useMobileSessionManagement (the function that calls setIsLoadingLogs) to capture
a per-request identifier or the AbortSignal for the specific fetch (e.g.,
localFetchId or currentSignal = controller.signal) and in the finally block only
call setIsLoadingLogs(false) if that request was not aborted and is still the
active one (check !currentSignal.aborted or matching fetchId); keep the existing
catch that returns on AbortError but prevent finally from hiding the spinner for
aborted/stale requests.
- Around line 638-655: The current code in the setSessions updater marks every
aiTab.hasUnread = true for the sessionId, which incorrectly flags all tabs;
change the logic in the setSessions mapping (used with currentActiveId,
sessionId, webLogger.debug) to: when a background output includes a tabId only
set hasUnread = true for the aiTab whose id matches that tabId and leave other
tabs unchanged, and if there is no tabId set a session-level unread flag (e.g.,
session.hasUnread or session.unreadCount) on the session object instead of
toggling all aiTabs; update the aiTabs mapping in the function that handles the
output to check tab.id === tabId before setting hasUnread and preserve existing
per-tab flags otherwise.
In `@src/web/mobile/App.tsx`:
- Around line 1484-1490: The Action that opens the Auto Run setup sheet can
bypass the new preload logic because some callers (the "Launch Auto Run"
command) call setShowAutoRunSetup(true) directly; ensure documents are always
loaded when the sheet opens by either funneling all openers to the existing
handleAutoRunOpenSetup helper or moving the load logic into a useEffect that
watches showAutoRunSetup === true and activeSessionId, calling
loadAutoRunDocuments(activeSessionId) when the sheet becomes visible; update
references to setShowAutoRunSetup so they use handleAutoRunOpenSetup or add the
useEffect to guarantee fresh documents regardless of which opener is used.
In `@src/web/mobile/LeftPanel.tsx`:
- Around line 231-237: handleSelect in LeftPanel is causing a duplicate
vibration because it calls triggerHaptic(HAPTIC_PATTERNS.tap) while the caller
useMobileSessionManagement.handleSelectSession already triggers haptics; remove
the redundant haptic call from handleSelect so it only calls
onSelectSession(sessionId) (keep useCallback and [onSelectSession] dependency
intact) and rely on useMobileSessionManagement.handleSelectSession to perform
the vibration.
In `@src/web/mobile/RightPanel.tsx`:
- Around line 66-67: The component initializes currentTab from the activeTab
prop but never updates it when activeTab changes; add a useEffect that watches
activeTab and calls setCurrentTab(activeTab) so the UI stays in sync when a
different entry point requests a new tab (referencing currentTab, setCurrentTab
and activeTab in the RightPanel component).
---
Nitpick comments:
In `@src/__tests__/main/process-listeners/forwarding-listeners.test.ts`:
- Around line 77-90: Add a new test that covers the web broadcast branch by
stubbing mockDeps.getWebServer to return a non-null mock web server with a
jest.fn() broadcast method, then call
setupForwardingListeners(mockProcessManager, mockDeps), retrieve the
'tool-execution' handler from eventHandlers and invoke it with a test session id
and payload, and finally assert that the mock web server's broadcast was called
with the expected channel/payload (and optionally that mockSafeSend was still
called if both should run); reference setupForwardingListeners, eventHandlers,
mockDeps.getWebServer, and the mock web server's broadcast in the test.
In `@src/__tests__/main/web-server/web-server-factory.test.ts`:
- Around line 73-76: The test currently defines mocks for
setWriteToTerminalCallback, setResizeTerminalCallback,
setSpawnTerminalForWebCallback, and setKillTerminalForWebCallback but doesn't
assert they were registered; update the "callback registrations" test block in
the createWebServerFactory spec to add expectations that each mock was invoked
(e.g. expect(setWriteToTerminalCallback).toHaveBeenCalled(),
expect(setResizeTerminalCallback).toHaveBeenCalled(),
expect(setSpawnTerminalForWebCallback).toHaveBeenCalled(),
expect(setKillTerminalForWebCallback).toHaveBeenCalled()) after calling
createWebServerFactory so the suite verifies the factory actually registers all
four terminal setters.
In `@src/__tests__/web/mobile/App.test.tsx`:
- Around line 195-220: The mock LeftPanel uses sessions.map((s: any) => ...)
which loses type safety; replace the any with a concrete session type (e.g., {
id: string; name: string } or the existing Session type if available) by
updating the mock prop signature for sessions and the map callback parameter in
the LeftPanel mock so the compiler knows sessions items have id and name; adjust
the sessions parameter type in the mocked LeftPanel declaration (or import/alias
the real Session type) and use that type instead of any in the map.
In `@src/__tests__/web/mobile/TabBar.test.tsx`:
- Around line 106-129: Replace the weak assertion that merely checks
container.firstChild with a stable DOM assertion that verifies a specific piece
of chrome from the TabBar component (e.g., assert the presence of the "New Tab"
button or the tablist). In the tests that render <TabBar ... tabs={[]} /> and
<TabBar ... tabs={[defaultTab]} />, use testing-library queries such as
getByRole('tablist') or getByText('New Tab') against the rendered output (the
same render result used now) and assert those elements are present so the tests
will fail if the actual chrome is removed or altered.
In `@src/renderer/App.tsx`:
- Around line 2045-2047: The silent catch in src/renderer/App.tsx (the per-file
read try/catch that currently “leaves counts at 0”) should log the failure for
debugging: inside that catch block, capture the error and the file identifier
(e.g., filename or doc id from the surrounding scope) and call the project's
debug logger (or console.debug if no logger available) with a concise message
and the error object so per-file read failures are visible without changing
behavior. Ensure you reference the same variables used in the try
(filename/docId) to provide context.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdd25780-27d1-4141-ac1a-8d06e2eaa88a
📒 Files selected for processing (19)
src/__tests__/main/process-listeners/data-listener.test.tssrc/__tests__/main/process-listeners/forwarding-listeners.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/__tests__/web/mobile/App.test.tsxsrc/__tests__/web/mobile/CommandInputBar.test.tsxsrc/__tests__/web/mobile/TabBar.test.tsxsrc/main/process-listeners/__tests__/data-listener.test.tssrc/main/process-listeners/__tests__/forwarding-listeners.test.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/services/broadcastService.tssrc/renderer/App.tsxsrc/web/hooks/useIsMobile.tssrc/web/hooks/useMobileSessionManagement.tssrc/web/hooks/useResizableWebPanel.tssrc/web/mobile/App.tsxsrc/web/mobile/CommandInputButtons.tsxsrc/web/mobile/LeftPanel.tsxsrc/web/mobile/RightPanel.tsxsrc/web/mobile/TabBar.tsx
✅ Files skipped from review due to trivial changes (1)
- src/web/hooks/useIsMobile.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/process-listeners/tests/forwarding-listeners.test.ts
- src/main/web-server/services/broadcastService.ts
- src/web/mobile/CommandInputButtons.tsx
- src/web/mobile/TabBar.tsx
- src/web/hooks/useResizableWebPanel.ts
- src/main/web-server/handlers/messageHandlers.ts
Comment on lines
+23
to
+30
| mockDeps = { | ||
| safeSend: mockSafeSend, | ||
| getWebServer: () => null, | ||
| patterns: { | ||
| REGEX_AI_SUFFIX: /-ai-[^-]+$/, | ||
| REGEX_AI_TAB_ID: /-ai-([^-]+)$/, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Mocked AI tab regex is too restrictive for UUID-style tab IDs.
Line 27 and Line 28 use [^-]+, which won’t capture full hyphenated UUID tab IDs and can mask parsing regressions in listener behavior. Consider matching the full suffix/tab segment shape used elsewhere.
Proposed test-fixture fix
mockDeps = {
safeSend: mockSafeSend,
getWebServer: () => null,
patterns: {
- REGEX_AI_SUFFIX: /-ai-[^-]+$/,
- REGEX_AI_TAB_ID: /-ai-([^-]+)$/,
+ REGEX_AI_SUFFIX: /-ai-.+$/,
+ REGEX_AI_TAB_ID: /-ai-(.+)$/,
},
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/main/process-listeners/forwarding-listeners.test.ts` around
lines 23 - 30, The test fixture's mockDeps uses patterns.REGEX_AI_SUFFIX and
patterns.REGEX_AI_TAB_ID with [^-]+ which fails to match hyphenated UUID-style
tab IDs; update those two regexes in mockDeps to allow hyphens (e.g., use .+ or
a character class that includes hyphen/digits/letters such as [A-Za-z0-9-]+) so
the mock mirrors real suffix/tab-id shapes and will catch parsing regressions in
the forwarding listeners tests (refer to mockDeps, patterns.REGEX_AI_SUFFIX, and
patterns.REGEX_AI_TAB_ID).
Comment on lines
305
to
310
| } catch (err) { | ||
| if ((err as Error).name === 'AbortError') return; | ||
| webLogger.error('Failed to fetch session logs', 'Mobile', err); | ||
| } finally { | ||
| setIsLoadingLogs(false); | ||
| } |
There was a problem hiding this comment.
Aborted log loads still clear the spinner.
When the active session or tab changes, the old request is aborted, but its finally still runs and calls setIsLoadingLogs(false). A quick switch can therefore hide the loading state while the replacement fetch is still in flight.
💡 Suggested fix
} catch (err) {
if ((err as Error).name === 'AbortError') return;
webLogger.error('Failed to fetch session logs', 'Mobile', err);
} finally {
- setIsLoadingLogs(false);
+ if (!controller.signal.aborted) {
+ setIsLoadingLogs(false);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| if ((err as Error).name === 'AbortError') return; | |
| webLogger.error('Failed to fetch session logs', 'Mobile', err); | |
| } finally { | |
| setIsLoadingLogs(false); | |
| } | |
| } catch (err) { | |
| if ((err as Error).name === 'AbortError') return; | |
| webLogger.error('Failed to fetch session logs', 'Mobile', err); | |
| } finally { | |
| if (!controller.signal.aborted) { | |
| setIsLoadingLogs(false); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/hooks/useMobileSessionManagement.ts` around lines 305 - 310, The
spinner is being cleared in the finally block even when the request was aborted;
update the fetch flow in useMobileSessionManagement (the function that calls
setIsLoadingLogs) to capture a per-request identifier or the AbortSignal for the
specific fetch (e.g., localFetchId or currentSignal = controller.signal) and in
the finally block only call setIsLoadingLogs(false) if that request was not
aborted and is still the active one (check !currentSignal.aborted or matching
fetchId); keep the existing catch that returns on AbortError but prevent finally
from hiding the spinner for aborted/stale requests.
Comment on lines
+638
to
+655
| // Mark as unread if output is for a non-active session | ||
| if (currentActiveId !== sessionId) { | ||
| webLogger.debug('Skipping output - not active session', 'Mobile', { | ||
| webLogger.debug('Marking session as unread - not active session', 'Mobile', { | ||
| sessionId, | ||
| activeSessionId: currentActiveId, | ||
| }); | ||
| setSessions((prev) => | ||
| prev.map((s) => { | ||
| if (s.id !== sessionId) return s; | ||
| return { | ||
| ...s, | ||
| aiTabs: s.aiTabs?.map((tab) => ({ | ||
| ...tab, | ||
| hasUnread: true, | ||
| })), | ||
| }; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Don't fan one unread event out to every AI tab.
This maps every aiTab to hasUnread: true whenever a background session emits output. In a multi-tab session the unread badge stops telling the user which conversation actually changed. Use tabId when it exists, and keep any session-level unread state separate from per-tab flags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/hooks/useMobileSessionManagement.ts` around lines 638 - 655, The
current code in the setSessions updater marks every aiTab.hasUnread = true for
the sessionId, which incorrectly flags all tabs; change the logic in the
setSessions mapping (used with currentActiveId, sessionId, webLogger.debug) to:
when a background output includes a tabId only set hasUnread = true for the
aiTab whose id matches that tabId and leave other tabs unchanged, and if there
is no tabId set a session-level unread flag (e.g., session.hasUnread or
session.unreadCount) on the session object instead of toggling all aiTabs;
update the aiTabs mapping in the function that handles the output to check
tab.id === tabId before setting hasUnread and preserve existing per-tab flags
otherwise.
Comment on lines
1484
to
+1490
| const handleAutoRunOpenSetup = useCallback(() => { | ||
| setShowAutoRunSetup(true); | ||
| }, []); | ||
| // Load documents when setup sheet opens so it has the latest list | ||
| if (activeSessionId) { | ||
| loadAutoRunDocuments(activeSessionId); | ||
| } | ||
| }, [activeSessionId, loadAutoRunDocuments]); |
There was a problem hiding this comment.
This only refreshes Auto Run documents for one open path.
handleAutoRunOpenSetup now preloads documents, but the command-palette “Launch Auto Run” action in this file still opens the sheet with setShowAutoRunSetup(true) directly. That route can still render stale or empty documents. Load on showAutoRunSetup === true, or funnel every opener through the same helper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/mobile/App.tsx` around lines 1484 - 1490, The Action that opens the
Auto Run setup sheet can bypass the new preload logic because some callers (the
"Launch Auto Run" command) call setShowAutoRunSetup(true) directly; ensure
documents are always loaded when the sheet opens by either funneling all openers
to the existing handleAutoRunOpenSetup helper or moving the load logic into a
useEffect that watches showAutoRunSetup === true and activeSessionId, calling
loadAutoRunDocuments(activeSessionId) when the sheet becomes visible; update
references to setShowAutoRunSetup so they use handleAutoRunOpenSetup or add the
useEffect to guarantee fresh documents regardless of which opener is used.
Comment on lines
+231
to
+237
| const handleSelect = useCallback( | ||
| (sessionId: string) => { | ||
| triggerHaptic(HAPTIC_PATTERNS.tap); | ||
| onSelectSession(sessionId); | ||
| }, | ||
| [onSelectSession] | ||
| ); |
There was a problem hiding this comment.
Session selection currently vibrates twice.
This callback triggers HAPTIC_PATTERNS.tap before delegating, and the current caller (useMobileSessionManagement.handleSelectSession) already does the same. Every tap from the left panel will therefore buzz twice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/mobile/LeftPanel.tsx` around lines 231 - 237, handleSelect in
LeftPanel is causing a duplicate vibration because it calls
triggerHaptic(HAPTIC_PATTERNS.tap) while the caller
useMobileSessionManagement.handleSelectSession already triggers haptics; remove
the redundant haptic call from handleSelect so it only calls
onSelectSession(sessionId) (keep useCallback and [onSelectSession] dependency
intact) and rely on useMobileSessionManagement.handleSelectSession to perform
the vibration.
Comment on lines
+66
to
+67
| const [currentTab, setCurrentTab] = useState<RightDrawerTab>(activeTab); | ||
|
|
There was a problem hiding this comment.
Keep currentTab in sync with activeTab.
activeTab is only used for the initial state. If the panel is already open and another entry point asks for a different tab, the prop changes but the UI stays on the old tab.
💡 Suggested fix
const [currentTab, setCurrentTab] = useState<RightDrawerTab>(activeTab);
+
+ useEffect(() => {
+ setCurrentTab(activeTab);
+ }, [activeTab]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [currentTab, setCurrentTab] = useState<RightDrawerTab>(activeTab); | |
| const [currentTab, setCurrentTab] = useState<RightDrawerTab>(activeTab); | |
| useEffect(() => { | |
| setCurrentTab(activeTab); | |
| }, [activeTab]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/mobile/RightPanel.tsx` around lines 66 - 67, The component
initializes currentTab from the activeTab prop but never updates it when
activeTab changes; add a useEffect that watches activeTab and calls
setCurrentTab(activeTab) so the UI stays in sync when a different entry point
requests a new tab (referencing currentTab, setCurrentTab and activeTab in the
RightPanel component).
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
2 participants
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
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit