feat: terminal sessions, file watcher, PTY reattach, settings, MCP standalone chat#145
Merged
feat: terminal sessions, file watcher, PTY reattach, settings, MCP standalone chat#145
Conversation
…andalone chat Overhaul terminal session management with persisted work view state, collapsible session groups, session card redesign, and view mode toggle. Refactor file watcher to use ref-counted subscriptions with dual-mode search indexes. Add PTY session reattach and transcript resume. Introduce terminal preferences (font size, line height, scrollback) in settings. Hide coordinator tools from standalone MCP chat sessions. Simplify ChatTerminalDrawer to reuse shared TerminalView. Code cleanup: dead code removal, deduplication, accessibility fixes across services and components. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/main/services/files/fileService.ts (1)
123-128:⚠️ Potential issue | 🟠 MajorEPIPE not fully caught—
stdinerror event is unhandled.The pipeline failure indicates an EPIPE error during
child.stdin.write. Node.js streams emit errors asynchronously via the'error'event rather than throwing synchronously. The current try/catch only handles synchronous throws.🐛 Proposed fix to handle stdin errors
+ child.stdin.on("error", () => { + // Ignore write errors (EPIPE when git exits early) + }); + try { child.stdin.write(`${args.paths.join("\n")}\n`); child.stdin.end(); } catch { finish(new Set<string>()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/files/fileService.ts` around lines 123 - 128, The try/catch around child.stdin.write doesn't catch asynchronous stream errors like EPIPE—attach an 'error' handler to child.stdin (and optionally to child) to call finish(new Set<string>()) when an error occurs, and remove the handler after successful write/close; keep the existing synchronous try/catch for immediate throws but ensure you subscribe to child.stdin.on('error', ...) before writing and clean up the listener after child.stdin.end() or on success to avoid leaks (refer to child.stdin, child, args.paths, and finish).apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx (1)
486-506:⚠️ Potential issue | 🟡 MinorExpose the group’s collapse state to assistive tech.
This button toggles a disclosure, but it never reports whether the section is expanded or collapsed. Add
aria-expanded={!group.collapsed}and, if possible,aria-controlspointing at the session container so screen-reader users can tell whether the group is open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx` around lines 486 - 506, The toggle button for a group (the element that calls toggleTabGroupCollapsed(group.id)) currently doesn't expose the disclosure state; add aria-expanded={!group.collapsed} to that button and set aria-controls to the id of the session container (e.g. ensure the session list element uses a stable id based on group.id like `session-container-${group.id}` or an existing unique id) so assistive tech can read whether the group is open; also add/create that id on the session container component that renders group.sessions to match aria-controls.apps/desktop/src/renderer/components/terminals/useWorkSessions.ts (1)
878-894:⚠️ Potential issue | 🟠 MajorTreat the post-resume refresh as best-effort.
At this point the PTY has already been reattached successfully. If the forced refresh throws, the function exits before selecting/focusing the resumed session, leaving a live session running but not surfaced in the UI.
launchPtySession()already handles refresh failure gracefully; resume should do the same.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.ts` around lines 878 - 894, The forced refresh after reattaching the PTY should be best-effort: wrap the await refresh({ showLoading: false, force: true }) call in a try/catch (or otherwise handle rejection) so any thrown error is swallowed/logged and does not abort the rest of the flow; ensure invalidateSessionListCache() still runs and that selectLane(session.laneId), focusSession(resumed.sessionId) and setActiveItemId(resumed.sessionId) always execute after window.ade.pty.create(...) completes successfully (use the resumed variable) even if refresh fails.apps/desktop/src/renderer/state/appStore.ts (1)
464-517:⚠️ Potential issue | 🟠 MajorMove full-map persistence off the interaction hot path.
Both setters now stringify and write the entire work-view payload on every state update. These setters are used for normal tab/select/close interactions, so this adds synchronous storage I/O to routine clicks in the renderer. Persist this on a debounce/idle boundary, or batch only the final state after a burst of updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/state/appStore.ts` around lines 464 - 517, The setters setWorkViewState and setLaneWorkViewState are calling persistWorkViewState synchronously on every update which causes blocking sync storage I/O on common UI interactions; change them to enqueue the new workViewByProject / laneWorkViewByScope into in-memory store state only and defer calling persistWorkViewState to a debounced/idle flush (e.g. use a module-level debounce timer or requestIdleCallback) that writes the combined payload once after a burst of updates; keep the existing updated object construction logic in setWorkViewState and setLaneWorkViewState but remove the direct persistWorkViewState calls and instead trigger the single flush function that calls persistWorkViewState with { workViewByProject, laneWorkViewByScope } when the timer/idle callback fires.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/lib/sessions.ts (1)
79-90: Consider centralizing tool-type label mappings to prevent drift.
shortToolTypeLabeladds another mapping table alongside existing label formatters. A shared map/helper would reduce inconsistency risk as new tool types are added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/sessions.ts` around lines 79 - 90, shortToolTypeLabel currently implements its own if-chain mapping for tool type labels which can drift from other label formatters; refactor by extracting a shared mapping lookup (e.g., a central TOOL_TYPE_LABELS map and a helper like getToolTypeLabel(toolType: string | null | undefined)) and update shortToolTypeLabel to call that helper (falling back to the existing replace(/-/g, " ") behavior). Replace the hardcoded startsWith/equals logic in shortToolTypeLabel with a single map lookup and ensure other label formatters in the codebase use the same helper to keep mappings centralized and consistent.apps/desktop/src/renderer/components/terminals/SessionCard.tsx (1)
172-183: Improve keyboard discoverability of hover actions.Consider adding
group-focus-within:opacity-100(container) and explicitfocus-visiblering styles on these buttons so keyboard users can reliably discover and track focus on hidden-on-hover controls.♿ Suggested accessibility-focused refinement
- <div className="absolute right-1.5 top-1 flex items-center gap-0.5 opacity-0 transition-opacity group-hover:opacity-100"> + <div className="absolute right-1.5 top-1 flex items-center gap-0.5 opacity-0 transition-opacity group-hover:opacity-100 group-focus-within:opacity-100"> ... - className="inline-flex items-center justify-center h-5 w-5 rounded-full border border-white/[0.08] bg-white/[0.06] text-muted-fg/60 hover:text-fg hover:bg-white/[0.10] transition-colors" + className="inline-flex items-center justify-center h-5 w-5 rounded-full border border-white/[0.08] bg-white/[0.06] text-muted-fg/60 hover:text-fg hover:bg-white/[0.10] focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-accent/70 transition-colors" ... - className="inline-flex items-center gap-0.5 px-1.5 py-0.5 rounded border border-white/[0.08] bg-white/[0.06] text-muted-fg/60 hover:text-fg hover:bg-white/[0.10] transition-colors text-[10px] font-medium" + className="inline-flex items-center gap-0.5 px-1.5 py-0.5 rounded border border-white/[0.08] bg-white/[0.06] text-muted-fg/60 hover:text-fg hover:bg-white/[0.10] focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-accent/70 transition-colors text-[10px] font-medium"As per coding guidelines
apps/desktop/src/**: Electron desktop app — check for IPC security, proper main/renderer process separation, and React best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/terminals/SessionCard.tsx` around lines 172 - 183, The hover-only action buttons in SessionCard need keyboard discoverability: add a container-level utility such as group and group-focus-within:opacity-100 to the wrapper around the action buttons so they become visible when focused via keyboard, and add explicit focus-visible styles (e.g., a visible ring outline) to both the Info button (the element calling onInfoClick) and the Resume button (the button that uses canResume and checks resumingSessionId) so keyboard users can see and track focus; update the className strings for those buttons to include focus-visible ring utilities and ensure the parent container includes the group and group-focus-within classes.apps/desktop/src/main/services/files/fileSearchIndexService.ts (1)
144-148: Potential inconsistency:node_modulesindexed but not watched.When
includeIgnored: true, this function allowsnode_modulesto be indexed (line 146 returnsfalse). However,fileWatcherService.tsalways ignoresnode_modulesviaALWAYS_IGNORED_PATTERNS, meaning changes innode_moduleswon't triggeronFileChanged.This creates a stale index scenario: files in
node_modulesare indexed initially but never updated on changes.Consider aligning the behavior—either always skip
node_modulesin indexing, or add a comment clarifying the intentional divergence:♻️ Option A: Always skip node_modules in indexer
const shouldSkipDirectoryName = (name: string, includeIgnored: boolean): boolean => { if (name === ".git") return true; - if (!includeIgnored && name === "node_modules") return true; + if (name === "node_modules") return true; return false; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/files/fileSearchIndexService.ts` around lines 144 - 148, The indexer currently may include node_modules when includeIgnored is true, but fileWatcherService always ignores node_modules, causing stale entries; update the shouldSkipDirectoryName function so that it always returns true for "node_modules" (remove the includeIgnored conditional for that name) to align indexing with ALWAYS_IGNORED_PATTERNS, and add/update a short comment in fileSearchIndexService.ts near shouldSkipDirectoryName explaining that node_modules is intentionally always skipped to match fileWatcherService behavior; also run/adjust any tests or call sites that assume node_modules can be indexed.
🤖 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/src/main/services/files/fileService.test.ts`:
- Around line 53-92: The test fails with EPIPE because fileService uses git
check-ignore in non-git temp dirs; fix by initializing a git repo in the temp
root before creating the service (e.g., run a git init in the temp root created
with fs.mkdtempSync) or alternatively stub/mock out git calls in the
FileService; update the test around createLaneServiceStub/rootPath and
createFileService so the temp directory is a valid git repo before calling
service.quickOpen and service.searchText.
In `@apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx`:
- Around line 176-190: The close affordance inside ChatTerminalDrawer.tsx should
be a real focusable button sibling to the tab instead of a span with
role="button" and tabIndex={-1}; replace the inner <span> close control with a
proper <button> element (keep using closeTab(tab.id) for the onClick and onKey
handling), move stopPropagation to the button's onClick, add an accessible label
(e.g., aria-label="Close tab"), ensure it is a sibling of the tab content (not
nested) so keyboard users can tab to it, and preserve the existing classes/hover
styles (e.g., "ml-0.5 text-white/30 ...") and any group-hover behavior.
- Around line 90-103: Auto-create is racing with the empty-state close and
causing PTY leaks and retry loops; add a guard ref so the auto-create effect
runs only on the initial open (not after the user closed the last tab) and avoid
re-triggering on failed pty.create calls. Concretely, introduce a ref like
autoCreateDoneRef and in the first useEffect (the one that calls createTab)
require !autoCreateDoneRef.current and set it true after a successful createTab;
keep the existing checks for open, creatingTab and tabs.length, and do not clear
autoCreateDoneRef when hadTabsRef.current flips during user-initiated close so
the effect will not re-run while onToggle/closing occurs. This uses the existing
symbols createTab, creatingTab, open, tabs.length, hadTabsRef, and onToggle to
locate and implement the change.
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts`:
- Around line 12-25: The tests share a mutable fakeAppStoreState initialized
only once via resetFakeAppStoreState(), causing later describe blocks to inherit
state; ensure resetFakeAppStoreState() runs before each test (e.g., add a
top-level beforeEach that calls resetFakeAppStoreState()) or reinitialize
fakeAppStoreState inside each describe to make tests independent; update
references to fakeAppStoreState, resetFakeAppStoreState, and any spies
(focusSessionSpy, selectLaneSpy, setWorkViewStateSpy) so each test gets a fresh
store instance.
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.ts`:
- Around line 584-594: The effect on projectRoot currently clears
refreshQueuedRef.current without resolving its deferred, which can leave waiters
hanging; before setting refreshQueuedRef.current = null in the useEffect inside
useWorkSessions, detect if refreshQueuedRef.current exists and call its resolve
or reject (e.g. reject with a cancellation error or resolve with a sensible
value) to settle the promise, then clear it; ensure you update the logic around
refreshQueuedRef (and any callers awaiting the deferred) so they handle the
settled state correctly and avoid memory leaks or dangling waiters while keeping
the existing invalidateSessionListCache, setSessions, and refresh({ showLoading:
true, force: true }) behavior.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 646-678: The segmented control rendering in WorkViewArea uses
buttons mapped from the array but only conveys state with color; update the
mapped <button> (the one using viewMode and setViewMode) to include
type="button" and aria-pressed={active} so the control exposes the selected
state to keyboard and screen-reader users; keep the existing onClick, className,
title and styles, and use the local variables active, mode, icon, label as
currently used.
In `@apps/mcp-server/src/mcpServer.test.ts`:
- Around line 1092-1103: The assertion using
expect(names).not.toEqual(expect.arrayContaining([...])) is too weak; update the
test to assert each forbidden tool is absent individually by replacing that
arrayContaining check with per-tool assertions like
expect(names).not.toContain("<tool>") for each of the listed tools (use the same
pattern already used earlier in the file for names and not.toContain). Ensure
you reference the same variable names (names) and the exact forbidden tool
strings ("spawn_agent", "delegate_to_subagent", "delegate_parallel",
"report_status", "report_result", "get_worker_output", "read_mission_status",
"list_workers") so each is checked separately.
In `@apps/mcp-server/src/mcpServer.ts`:
- Around line 2517-2527: The predicate isStandaloneChatCaller and
isToolHiddenForStandaloneChat currently rely on caller-controlled fields
(missionId, stepId, attemptId) which can be injected; change the check to use
only server-authorized context (e.g., a server-validated flag or server-side
session fields such as callerCtx.serverVerifiedSessionType or
callerCtx.authorizedMissionId) or alternatively make initialize explicitly
reject missionId/stepId/attemptId for chat-only sessions; update
isStandaloneChatCaller to read that server-validated property instead of
missionId/stepId/attemptId and ensure isToolHiddenForStandaloneChat calls the
updated predicate so coordinator-tool paths remain blocked for true standalone
chat callers.
- Around line 1541-1544: The STANDALONE_CHAT_HIDDEN_TOOL_NAMES set currently
only contains "spawn_agent" plus AGENT_VISIBLE_COORDINATOR_TOOL_NAMES, but you
must hide the entire coordinator surface for standalone chats; replace its
construction to include all coordinator tool names (e.g., union
Object.keys(COORDINATOR_TOOL_SPECS) and "spawn_agent" or simply use
Object.keys(COORDINATOR_TOOL_SPECS)) so every coordinator tool is in
STANDALONE_CHAT_HIDDEN_TOOL_NAMES, and ensure the tools/list and tools/call code
paths that check role === "orchestrator" for standalone chat sessions use this
updated set to filter both listing and execution.
---
Outside diff comments:
In `@apps/desktop/src/main/services/files/fileService.ts`:
- Around line 123-128: The try/catch around child.stdin.write doesn't catch
asynchronous stream errors like EPIPE—attach an 'error' handler to child.stdin
(and optionally to child) to call finish(new Set<string>()) when an error
occurs, and remove the handler after successful write/close; keep the existing
synchronous try/catch for immediate throws but ensure you subscribe to
child.stdin.on('error', ...) before writing and clean up the listener after
child.stdin.end() or on success to avoid leaks (refer to child.stdin, child,
args.paths, and finish).
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.ts`:
- Around line 878-894: The forced refresh after reattaching the PTY should be
best-effort: wrap the await refresh({ showLoading: false, force: true }) call in
a try/catch (or otherwise handle rejection) so any thrown error is
swallowed/logged and does not abort the rest of the flow; ensure
invalidateSessionListCache() still runs and that selectLane(session.laneId),
focusSession(resumed.sessionId) and setActiveItemId(resumed.sessionId) always
execute after window.ade.pty.create(...) completes successfully (use the resumed
variable) even if refresh fails.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 486-506: The toggle button for a group (the element that calls
toggleTabGroupCollapsed(group.id)) currently doesn't expose the disclosure
state; add aria-expanded={!group.collapsed} to that button and set aria-controls
to the id of the session container (e.g. ensure the session list element uses a
stable id based on group.id like `session-container-${group.id}` or an existing
unique id) so assistive tech can read whether the group is open; also add/create
that id on the session container component that renders group.sessions to match
aria-controls.
In `@apps/desktop/src/renderer/state/appStore.ts`:
- Around line 464-517: The setters setWorkViewState and setLaneWorkViewState are
calling persistWorkViewState synchronously on every update which causes blocking
sync storage I/O on common UI interactions; change them to enqueue the new
workViewByProject / laneWorkViewByScope into in-memory store state only and
defer calling persistWorkViewState to a debounced/idle flush (e.g. use a
module-level debounce timer or requestIdleCallback) that writes the combined
payload once after a burst of updates; keep the existing updated object
construction logic in setWorkViewState and setLaneWorkViewState but remove the
direct persistWorkViewState calls and instead trigger the single flush function
that calls persistWorkViewState with { workViewByProject, laneWorkViewByScope }
when the timer/idle callback fires.
---
Nitpick comments:
In `@apps/desktop/src/main/services/files/fileSearchIndexService.ts`:
- Around line 144-148: The indexer currently may include node_modules when
includeIgnored is true, but fileWatcherService always ignores node_modules,
causing stale entries; update the shouldSkipDirectoryName function so that it
always returns true for "node_modules" (remove the includeIgnored conditional
for that name) to align indexing with ALWAYS_IGNORED_PATTERNS, and add/update a
short comment in fileSearchIndexService.ts near shouldSkipDirectoryName
explaining that node_modules is intentionally always skipped to match
fileWatcherService behavior; also run/adjust any tests or call sites that assume
node_modules can be indexed.
In `@apps/desktop/src/renderer/components/terminals/SessionCard.tsx`:
- Around line 172-183: The hover-only action buttons in SessionCard need
keyboard discoverability: add a container-level utility such as group and
group-focus-within:opacity-100 to the wrapper around the action buttons so they
become visible when focused via keyboard, and add explicit focus-visible styles
(e.g., a visible ring outline) to both the Info button (the element calling
onInfoClick) and the Resume button (the button that uses canResume and checks
resumingSessionId) so keyboard users can see and track focus; update the
className strings for those buttons to include focus-visible ring utilities and
ensure the parent container includes the group and group-focus-within classes.
In `@apps/desktop/src/renderer/lib/sessions.ts`:
- Around line 79-90: shortToolTypeLabel currently implements its own if-chain
mapping for tool type labels which can drift from other label formatters;
refactor by extracting a shared mapping lookup (e.g., a central TOOL_TYPE_LABELS
map and a helper like getToolTypeLabel(toolType: string | null | undefined)) and
update shortToolTypeLabel to call that helper (falling back to the existing
replace(/-/g, " ") behavior). Replace the hardcoded startsWith/equals logic in
shortToolTypeLabel with a single map lookup and ensure other label formatters in
the codebase use the same helper to keep mappings centralized and consistent.
🪄 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: 76d82c37-a830-453e-b3b3-1a8fec2828e2
⛔ Files ignored due to path filters (7)
docs/ORCHESTRATOR_OVERHAUL.mdis excluded by!docs/**docs/architecture/AI_INTEGRATION.mdis excluded by!docs/**docs/features/AGENTS.mdis excluded by!docs/**docs/features/CHAT.mdis excluded by!docs/**docs/features/FILES_AND_EDITOR.mdis excluded by!docs/**docs/features/ONBOARDING_AND_SETTINGS.mdis excluded by!docs/**docs/features/TERMINALS_AND_SESSIONS.mdis excluded by!docs/**
📒 Files selected for processing (40)
apps/desktop/src/main/services/files/fileSearchIndexService.tsapps/desktop/src/main/services/files/fileService.test.tsapps/desktop/src/main/services/files/fileService.tsapps/desktop/src/main/services/files/fileWatcherService.test.tsapps/desktop/src/main/services/files/fileWatcherService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsxapps/desktop/src/renderer/components/files/FilesPage.test.tsxapps/desktop/src/renderer/components/files/FilesPage.tsxapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/settings/GeneralSection.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/SessionListPane.tsxapps/desktop/src/renderer/components/terminals/TerminalView.test.tsxapps/desktop/src/renderer/components/terminals/TerminalView.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/components/terminals/cliLaunch.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/lib/format.test.tsapps/desktop/src/renderer/lib/format.tsapps/desktop/src/renderer/lib/sessionListCache.test.tsapps/desktop/src/renderer/lib/sessionListCache.tsapps/desktop/src/renderer/lib/sessions.test.tsapps/desktop/src/renderer/lib/sessions.tsapps/desktop/src/renderer/state/appStore.test.tsapps/desktop/src/renderer/state/appStore.tsapps/desktop/src/shared/types/files.tsapps/desktop/src/shared/types/sessions.tsapps/mcp-server/src/mcpServer.test.tsapps/mcp-server/src/mcpServer.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
Comment on lines
+161
to
+168
| parts.push("--resume"); | ||
| if (targetId.length) parts.push(targetId); | ||
| return parts.join(" "); | ||
| } | ||
|
|
||
| const parts = ["codex", "--no-alt-screen", ...permissionModeToCodexFlags(permissionMode)]; | ||
| if (targetId.length) parts.push("resume", targetId); | ||
| parts.push("resume"); | ||
| if (targetId.length) parts.push(targetId); |
There was a problem hiding this comment.
Teach transcript extraction about the new permission-aware resume commands.
buildTrackedCliResumeCommand() now produces strings like claude --permission-mode default --resume and codex --no-alt-screen --full-auto resume, but extractResumeCommandFromOutput() still uses the regexes at Lines 12-13, which only match when --resume/resume comes immediately after the executable. Any transcript-tail backfill that sees the rebuilt command form will miss it and fall back to the generic picker instead of recovering the concrete resume command.
… test isolation - fileService: handle async EPIPE on child.stdin, git init in test temp dir - fileSearchIndexService: always skip node_modules regardless of includeIgnored - ChatTerminalDrawer: proper <button> for close tab, autoCreateDoneRef guard to prevent PTY leak - useWorkSessions: resolve deferred on projectRoot change, best-effort PTY reattach refresh, test state isolation - WorkViewArea/SessionCard: aria-pressed, aria-expanded, focus-visible keyboard styles - mcpServer: hide all coordinator tools for standalone chat, use server-validated session flag instead of caller-controlled fields - appStore: debounce persistWorkViewState to avoid blocking UI - sessions.ts: extract TOOL_TYPE_LABELS map from if-chain Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/renderer/state/appStore.ts`:
- Around line 169-191: Pending debounced writes can race with immediate persists
and reintroduce stale data; ensure any immediate calls to persistWorkViewState
(from setWorkViewState and setLaneWorkViewState) first cancel the pending
debounced timer by checking and clearing _debouncePersistTimer (and setting it
to null) before performing the immediate persist, or alternatively call
clearTimeout(_debouncePersistTimer) and null it inside persistWorkViewState so
that debouncedPersistWorkViewState cannot later overwrite the freshly written
localStorage entry keyed by WORK_VIEW_STORAGE_KEY.
🪄 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: 9698b1d7-95fc-46b0-9d61-ad62446413ee
📒 Files selected for processing (12)
apps/desktop/src/main/services/files/fileSearchIndexService.tsapps/desktop/src/main/services/files/fileService.test.tsapps/desktop/src/main/services/files/fileService.tsapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/useWorkSessions.test.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/lib/sessions.tsapps/desktop/src/renderer/state/appStore.tsapps/mcp-server/src/mcpServer.test.tsapps/mcp-server/src/mcpServer.ts
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/main/services/files/fileService.test.ts
- apps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/renderer/lib/sessions.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.test.ts
- apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
- apps/desktop/src/main/services/files/fileService.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
- apps/desktop/src/main/services/files/fileSearchIndexService.ts
Prevents a stale debounced write from overwriting a fresh immediate persist (e.g. from refreshLanes or project transitions). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pty.createaccepts existingsessionIdfor session reattach, transcript continuation in append mode, best-effort resume target backfill from transcript tailspawn_agenttools hidden from standalone chat sessionsTerminalViewcomponent instead of managing raw xterm instances<label htmlFor>), consolidated imports, extracted repeated patternsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes
Tests