Repository navigation
feat(desktop): auto-resume Claude and Codex cold restores - #5246
kunaldhongade wants to merge 26 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements end-to-end auto-resume of Claude and Codex agent sessions across app or machine restarts. Changes span: notify hook v4 with ChangesAgent Session Auto-Resume
Sequence Diagram(s)sequenceDiagram
participant Terminal as Terminal (renderer)
participant TRPC as TRPC terminal router
participant DaemonManager
participant AgentResume as agent-resume
participant SessionLog as session-location-log
participant HostDb as terminalSessions DB
rect rgba(100, 150, 220, 0.5)
note over Terminal,HostDb: Cold Restore Path
Terminal->>TRPC: createOrAttach (cold restore)
TRPC->>DaemonManager: createOrAttach
DaemonManager->>SessionLog: getSessionLocation(paneId)
SessionLog->>HostDb: SELECT WHERE id=paneId
DaemonManager->>AgentResume: resolveAgentResumeTarget(agentId, cwd, sessionId)
AgentResume-->>DaemonManager: AgentResumeTarget { resumeCommand, sessionId }
DaemonManager->>SessionLog: upsertSessionLocation(paneId, pid=null)
DaemonManager->>SessionLog: updateSessionLocationAgentIdentity(paneId, agentId)
DaemonManager-->>TRPC: SessionResult { resumeCommand }
TRPC-->>Terminal: { resumeCommand }
Terminal->>Terminal: store resumeCommand + set grace window
end
rect rgba(100, 200, 150, 0.5)
note over Terminal,HostDb: Shell Start → Resume Injection
Terminal->>TRPC: startShell (from cold-restored CWD)
Terminal->>Terminal: writeCommandInPane(resumeCommand)
Terminal->>Terminal: preserveCleanExitUntilRef = now + 10s
end
rect rgba(220, 130, 100, 0.5)
note over Terminal,HostDb: Session Exit
DaemonManager->>SessionLog: markSessionLocationExited(paneId, exitReason)
SessionLog->>HostDb: UPDATE SET status=exited, ended_at, exit_reason
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Ready to review this PR? Stage has broken it down into 9 individual chapters for you: 
Chapters generated by Stage for commit adf7f73 on Jun 15, 2026 12:46pm UTC. |
kunaldhongade
changed the title
ef47abf to
e319c6a
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
GitHub Actions CI is currently blocked at Local validation completed on this branch:
Also triggered a manual |
|
In the meantime, your local validation coverage looks solid:
A few things that may help move this forward:
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts (1)
854-863:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
setRestoredResumeCommandto the effect dependency array for consistency.The dependency array includes
setRestoredCwd(line 862) but notsetRestoredResumeCommand. While ReactuseStatesetters are typically stable, the inconsistency suggests either both should be included or both omitted (with an ESLint disable comment if intentional).🔧 Suggested fix
}, [ paneId, workspaceId, maybeApplyInitialState, flushPendingEvents, setConnectionError, resetModes, setIsRestoredMode, setRestoredCwd, + setRestoredResumeCommand, ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts` around lines 854 - 863, The effect dependency array in useTerminalLifecycle (the useEffect that currently lists paneId, workspaceId, maybeApplyInitialState, flushPendingEvents, setConnectionError, resetModes, setIsRestoredMode, setRestoredCwd) is inconsistent because it omits setRestoredResumeCommand; update that dependency array to include setRestoredResumeCommand so both restoration state setters are present (or alternatively remove both setters and add an ESLint disable comment if that was the intent) — locate the useEffect in useTerminalLifecycle.ts and add setRestoredResumeCommand to the dependencies to maintain consistency.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/notifications/server.ts (1)
112-120: 💤 Low valueConsider normalizing empty query parameter strings to
undefined.When
SUPERSET_AGENT_IDis unset in the shell environment, the v1 hook sendsagentId=(empty string). The server receives this as""rather thanundefined. WhileupdateSessionLocationAgentIdentitylikely handles empty strings gracefully, explicitly normalizing empty strings toundefinedwould make the intent clearer and ensure consistent handling across the codebase.♻️ Optional normalization
const locationPaneId = (terminalId as string | undefined) || resolvedPaneId || undefined; if (locationPaneId) { + const normalizedAgentId = agentId && agentId !== "" ? (agentId as string) : undefined; + const normalizedSessionId = sessionId && sessionId !== "" ? (sessionId as string) : undefined; updateSessionLocationAgentIdentity({ paneId: locationPaneId, - agentId: agentId as string | undefined, - agentSessionId: sessionId as string | undefined, + agentId: normalizedAgentId, + agentSessionId: normalizedSessionId, }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/notifications/server.ts` around lines 112 - 120, Normalize empty-string query params to undefined before calling updateSessionLocationAgentIdentity: detect if agentId or sessionId are "" and convert them to undefined (e.g., const normalizedAgentId = agentId === "" ? undefined : agentId), then pass normalizedAgentId and normalizedSessionId into updateSessionLocationAgentIdentity alongside the already-resolved locationPaneId (derived from terminalId/resolvedPaneId). This ensures terminalId, resolvedPaneId, agentId, and sessionId are handled consistently and empty query strings don't get propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`:
- Around line 615-617: The tRPC handler for terminal.createOrAttach is not
returning the SessionResult.resumeCommand, so result.resumeCommand is undefined;
update the terminal.createOrAttach router response to include resumeCommand from
the DaemonTerminalManager's SessionResult (the same value populated by
DaemonTerminalManager) so that cold-restore code (useTerminalLifecycle.ts using
storedColdRestore.resumeCommand) receives it; locate the createOrAttach handler
in the terminal router (terminal.createOrAttach) and add resumeCommand to the
returned object alongside the existing session/other fields.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`:
- Around line 854-863: The effect dependency array in useTerminalLifecycle (the
useEffect that currently lists paneId, workspaceId, maybeApplyInitialState,
flushPendingEvents, setConnectionError, resetModes, setIsRestoredMode,
setRestoredCwd) is inconsistent because it omits setRestoredResumeCommand;
update that dependency array to include setRestoredResumeCommand so both
restoration state setters are present (or alternatively remove both setters and
add an ESLint disable comment if that was the intent) — locate the useEffect in
useTerminalLifecycle.ts and add setRestoredResumeCommand to the dependencies to
maintain consistency.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/notifications/server.ts`:
- Around line 112-120: Normalize empty-string query params to undefined before
calling updateSessionLocationAgentIdentity: detect if agentId or sessionId are
"" and convert them to undefined (e.g., const normalizedAgentId = agentId === ""
? undefined : agentId), then pass normalizedAgentId and normalizedSessionId into
updateSessionLocationAgentIdentity alongside the already-resolved locationPaneId
(derived from terminalId/resolvedPaneId). This ensures terminalId,
resolvedPaneId, agentId, and sessionId are handled consistently and empty query
strings don't get propagated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e4ce44f-11bd-47a3-87aa-983bceeab729
📒 Files selected for processing (18)
apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.tsapps/desktop/src/main/lib/agent-setup/notify-hook.test.tsapps/desktop/src/main/lib/agent-setup/notify-hook.tsapps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.shapps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/main/lib/terminal/agent-resume.test.tsapps/desktop/src/main/lib/terminal/agent-resume.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/daemon/types.tsapps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.tsapps/desktop/src/main/lib/terminal/session-location-log.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
Greptile SummaryThis follow-up commit hardens Claude/Codex cold-restore resumption: scrollback is now consumed once per pane (deduplicated across StrictMode remounts), brand-new agent panes no longer scan transcripts before the hook reports the real session ID, stream activation is deferred until a real daemon-backed session exists, and a 10-second grace window prevents auto-close during resume command injection.
Confidence Score: 5/5This PR is safe to merge. The core cold-restore and resume paths are well-guarded, transcript validation is enforced before any shell-command construction, and the DB migration strategy is correct. The follow-up commit addresses the most significant concerns from prior review rounds: transcript existence is now validated for both Claude and Codex in resolveAgentResumeTarget, scrollback deduplication across StrictMode remounts is handled atomically, live stream activation is correctly deferred to avoid showing a blank terminal during the cold-restore UI phase, and the backfill scan in the live-attach path is guarded behind an established agent identity. The sole remaining observation is the module-level knownTopLevelTranscripts Set in server.ts, which has no eviction and could retain entries for deleted transcripts across a long session, but the consequence is limited to a slower fallback CWD scan rather than incorrect behavior. apps/desktop/src/main/lib/notifications/server.ts — the knownTopLevelTranscripts cache has no size bound or eviction strategy.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/terminal/agent-resume.ts | New file: resolves agent resume targets by validating transcript existence for both Claude and Codex, uses SESSION_ID_PATTERN allowlist before building any shell commands, and performs a bounded CWD-scored transcript scan as fallback. |
| apps/desktop/src/main/lib/notifications/server.ts | Adds agentId/sessionId persistence to the hook handler; correctly guards Claude identity overwrites with transcript verification but uses a module-level unbounded Set cache for known transcript IDs that never evicts stale entries. |
| apps/desktop/src/main/lib/terminal/session-location-log.ts | New file: replaces the JSON-file session-location store with a DB-backed adapter over terminal_sessions; handles legacy import with a one-shot flag that is correctly set before re-throwing parse errors to prevent repeated retries. |
| apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts | Adds cold-restore resume command resolution and session-location upserts; guards transcript backfill in the live-attach path behind existingLocation?.agentId but cold-restore path still infers agent ID from tab title as fallback when no stored identity exists. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.ts | Adds resumeCommand state and writes it via writeCommandInPane after Start Shell; preserves cold-restore state on writeCommandInPane failure (for retry) but correctly clears it on success; sets RESTORED_SESSION_CLEAN_EXIT_GRACE_MS protection. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts | Defers live stream activation until a real daemon session exists; uses consumeColdRestoreScrollback to deduplicate scrollback writes across StrictMode remounts; correctly gates stream activation on shouldActivateCachedTerminalStream. |
| apps/desktop/src/main/lib/host-db.ts | New file: singleton host DB accessor with correct cache-invalidation on org switch; falls back to filesystem scanning for org ID when local-db settings are unavailable; migrations directory lookup tries packaged path first then monorepo. |
| packages/host-service/drizzle/0007_merge_terminal_session_locations_into_terminal_sessions.sql | Migrates terminal_session_locations data into terminal_sessions via COALESCE-based UPDATE; correctly handles NULL originWorkspaceId, status mapping, and all restore-metadata columns without data loss. |
| packages/host-service/drizzle/0008_drop_terminal_sessions_workspace_fk.sql | Drops workspace FK from terminal_sessions via table recreation; uses the correct SQLite pattern of rename + recreate rather than ALTER TABLE DROP CONSTRAINT. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/state.ts | Adds consumeColdRestoreScrollback that marks scrollback as applied on first read, preventing duplicate writes across StrictMode remounts; atomic update pattern is correct. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/terminal-exit-policy.ts | Adds shouldAutoCloseTerminalOnCleanExit with a 10-second grace window to prevent restored sessions from being auto-closed during the resume command injection window. |
Sequence Diagram
sequenceDiagram
participant UI as Terminal (Renderer)
participant DM as DaemonManager (Main)
participant SL as SessionLocationLog
participant AR as AgentResume
participant DB as host.db
Note over UI,DB: Cold Restore Path (app restart / machine restart)
UI->>DM: createOrAttach(paneId)
DM->>SL: getSessionLocation(paneId)
SL->>DB: "SELECT terminal_sessions WHERE id=paneId"
DB-->>SL: agentId, agentSessionId, command, cwd
SL-->>DM: sessionLocation
DM->>AR: resolveAgentResumeTarget(agentId, sessionId, cwd)
AR->>AR: hasClaudeTopLevelTranscript / hasCodexTranscript
AR-->>DM: resumeTarget with resumeCommand
DM->>SL: upsertSessionLocation + updateAgentIdentity
SL->>DB: UPDATE terminal_sessions
DM-->>UI: "isColdRestore=true, scrollback, resumeCommand"
UI->>UI: consumeColdRestoreScrollback (mark applied)
UI->>UI: Show scrollback + Start Shell button
Note over UI,DB: User clicks Start Shell
UI->>DM: "createOrAttach(paneId, cwd=restoredCwd)"
DM-->>UI: "new session (isColdRestore=false)"
UI->>UI: ensureCachedStreamActive
UI->>DM: terminal.write(resumeCommand)
UI->>UI: clearRestoreResumeState
Reviews (14): Last reviewed commit: "fix(desktop): harden agent session autor..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 18 files
Architecture diagram
sequenceDiagram
participant UI as Terminal UI (React)
participant LC as useTerminalColdRestore
participant LL as useTerminalLifecycle
participant DTM as DaemonTerminalManager
participant SL as SessionLocationLog
participant AR as AgentResumeResolver
participant TS as Transcript Store (~/.claude/projects or ~/.codex/sessions)
participant ENV as Terminal Env Builder
Note over UI,ENV: Cold Restore Flow (app restart / crash recovery)
LL->>DTM: createOrAttach(paneId, tabId, workspaceId)
DTM->>SL: getSessionLocation(paneId)
SL-->>DTM: SessionLocationEntry (with agentId, agentSessionId, workspacePath, rootPath)
DTM->>AR: resolveAgentResumeTarget(agentId, sessionId, cwd, workspacePath, rootPath)
alt Known sessionId from session location log
AR-->>AR: Build resume command from known identity
AR-->>DTM: AgentResumeTarget (sourcePath: "session-location-log")
else Unknown identity - scan transcript stores
AR->>TS: Read ~/.claude/projects/*.jsonl (max 200 recent)
AR->>TS: Read ~/.codex/sessions/rollout-*.jsonl
TS-->>AR: Transcript snippets (with sessionId, cwd)
AR-->>AR: Score candidates by cwd match, pick most recent
AR-->>DTM: AgentResumeTarget (best match)
end
alt Resume target found
DTM->>SL: updateSessionLocationAgentIdentity(paneId, agentId, agentSessionId)
DTM-->>LL: SessionResult (isColdRestore=true, resumeCommand="claude --resume <id>")
else No resume target
DTM-->>LL: SessionResult (isColdRestore=true, resumeCommand=undefined)
end
DTM->>SL: upsertSessionLocation(paneId, tabId, workspaceId, cwd, pid)
LL->>LC: Pending initial state with resumeCommand
Note over LC,UI: Shell Start and Auto-Resume
LC->>LC: handleStartShell()
LC->>UI: Start shell session
UI-->>LC: Shell ready
opt resumeCommand is set
LC->>LC: writeCommandInPane(paneId, resumeCommand)
LC->>UI: Write resume command to terminal input
UI-->>LC: Command executed in shell
alt Success
Note over UI: Agent session resumes automatically
else Write failure
UI-->>LC: Error
LC->>UI: setConnectionError("Failed to resume")
end
end
Note over ENV: Terminal Environment Variables
ENV->>ENV: buildTerminalEnv(paneId, tabId, workspaceId)
ENV->>ENV: Set SUPERSET_TERMINAL_ID (=paneId)
ENV->>SL: SESSION_LOCATION_LOG_PATH
ENV->>SL: buildSessionLocationKey(workspaceId, tabId, paneId)
ENV-->>Terminal: SUPERSET_SESSION_LOCATIONS_PATH, SUPERSET_SESSION_LOCATION_KEY
Note over UI,SL: Agent Identity Persistence (via hook server)
participant HS as Hook Server (/hook/complete)
participant AgentScript as Agent Notification Script (v4)
AgentScript->>HS: POST agentId=$SUPERSET_AGENT_ID, sessionId, terminalId
HS->>SL: updateSessionLocationAgentIdentity(paneId, agentId, agentSessionId)
Note over SL: Session lifecycle tracking
DTM-->>DTM: Terminal pane exits
DTM->>SL: markSessionLocationExited(paneId, exitReason)
SL->>SL: Set status: "exited", clear pid, record exit timestamp
Note over UI,SL: Renderer-side cold restore state
LL->>LL: Check coldRestoreState(paneId)
alt State exists (from previous lifecycle)
LL->>LC: setRestoredResumeCommand(state.resumeCommand)
LL->>UI: Write scrollback
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Pushed follow-up commit
Local validation on this head:
GitHub Actions still needs upstream maintainer approval to run on this fork PR. |
|
@nlatuan187 @aronchick local follow-up fixes are pushed on |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal/agent-resume.ts (1)
367-386: 💤 Low valueConsider simplifying the single-element array pattern.
Since
normalizedAgentIdis a single value (not an array of agents to scan), wrapping it inPromise.all([...].map(...))followed by filter/sort is unnecessarily complex. A direct call would be clearer:- const candidates = await Promise.all( - [normalizedAgentId].map((agentId) => - resolveCandidateFromTranscripts({ - agentId, - searchPaths, - }), - ), - ); - - return ( - candidates - .filter( - (candidate): candidate is TranscriptCandidate => candidate !== null, - ) - .sort((a, b) => - b.matchScore !== a.matchScore - ? b.matchScore - a.matchScore - : b.mtimeMs - a.mtimeMs, - )[0] ?? null - ); + return resolveCandidateFromTranscripts({ + agentId: normalizedAgentId, + searchPaths, + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/terminal/agent-resume.ts` around lines 367 - 386, The code currently wraps normalizedAgentId in an array and uses Promise.all(...).map(...) then filter/sort to pick the best TranscriptCandidate; simplify by calling resolveCandidateFromTranscripts directly for the single normalizedAgentId (await resolveCandidateFromTranscripts({ agentId: normalizedAgentId, searchPaths })), assign its result to a variable (e.g., candidate) and return that candidate or null, removing the unnecessary Promise.all, array mapping, filter and sort logic; keep references to resolveCandidateFromTranscripts, normalizedAgentId, candidates (or new candidate variable) and the final return value so the change is easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/src/main/lib/terminal/agent-resume.ts`:
- Around line 367-386: The code currently wraps normalizedAgentId in an array
and uses Promise.all(...).map(...) then filter/sort to pick the best
TranscriptCandidate; simplify by calling resolveCandidateFromTranscripts
directly for the single normalizedAgentId (await
resolveCandidateFromTranscripts({ agentId: normalizedAgentId, searchPaths })),
assign its result to a variable (e.g., candidate) and return that candidate or
null, removing the unnecessary Promise.all, array mapping, filter and sort
logic; keep references to resolveCandidateFromTranscripts, normalizedAgentId,
candidates (or new candidate variable) and the final return value so the change
is easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7310b2fa-e33a-4f60-a276-bea2214da7e4
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/main/lib/terminal/agent-resume.test.tsapps/desktop/src/main/lib/terminal/agent-resume.tsapps/desktop/src/main/lib/terminal/session-location-log.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/lib/notifications/server.ts
- apps/desktop/src/main/lib/terminal/session-location-log.ts
|
@saddlepaddle @Kitenite @AviPeltz current head is locally validated and the remaining blocker is upstream approval of the fork Actions run. When one of you has a moment, could you review the updated head and click "Approve and run" so the required checks can start? |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.ts (1)
258-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the clean-exit grace window on canceled start-shell attempts.
When
createOrAttachis canceled here, the callback returns before resettingpreserveCleanExitUntilRef.current. That leaves the restored-session grace window armed even though no replacement shell was started, so the next clean exit can be preserved incorrectly instead of auto-closing.Suggested fix
onError: (error: { message?: string }) => { if (isTerminalAttachCanceledMessage(error.message)) { + preserveCleanExitUntilRef.current = 0; return; } preserveCleanExitUntilRef.current = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.ts` around lines 258 - 263, In useTerminalColdRestore's onError callback for createOrAttach, ensure the restore grace window is cleared even when the attach is canceled: call preserveCleanExitUntilRef.current = 0 before returning from the isTerminalAttachCanceledMessage(error.message) branch so the canceled start-shell attempt doesn't leave the grace window armed; update the onError handler (referencing onError, isTerminalAttachCanceledMessage, preserveCleanExitUntilRef.current, and createOrAttach) to clear the ref prior to returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.ts`:
- Around line 258-263: In useTerminalColdRestore's onError callback for
createOrAttach, ensure the restore grace window is cleared even when the attach
is canceled: call preserveCleanExitUntilRef.current = 0 before returning from
the isTerminalAttachCanceledMessage(error.message) branch so the canceled
start-shell attempt doesn't leave the grace window armed; update the onError
handler (referencing onError, isTerminalAttachCanceledMessage,
preserveCleanExitUntilRef.current, and createOrAttach) to clear the ref prior to
returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97cfa244-5b87-4daf-899c-489d35c0422d
📒 Files selected for processing (5)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/terminal-exit-policy.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/terminal-exit-policy.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
|
Pushed commit What changed:
Validation:
|
|
@coderabbitai review Please re-run review on the latest commit Local validation after the latest fix:
|
@kunaldhongade I have started the AI code review. It will take a few minutes to complete. |
|
The fallback to Superset's built-in preset command for commandless known Claude/Codex rows is a good hardening step, and the Codex regression coverage for ✅ Action performedReview finished.
|
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/host-db.ts (1)
115-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose stale cached DB when no active organization is resolved.
At Line 117, returning
nullbefore the close/reset block means an existingcachedDbcan stay open indefinitely when org resolution fails. Move cleanup before this early return.Suggested fix
export function getActiveHostDb(): HostDb | null { const organizationId = getActiveOrganizationId(); if (!organizationId) { + if (cachedDb) { + try { + ( + cachedDb as unknown as { $client?: { close: () => void } } + ).$client?.close(); + } catch (error) { + console.warn("[host-db] Failed to close previous host db", error); + } + cachedDb = null; + cachedOrganizationId = null; + cachedDbPath = null; + } return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/host-db.ts` around lines 115 - 141, The getActiveHostDb() function returns null when no active organization is resolved (when !organizationId is true), but this early return happens before the cached database cleanup block. This leaves any previously cached database open indefinitely. Move the cleanup logic that closes cachedDb and resets cachedOrganizationId and cachedDbPath (the block containing the try-catch that calls $client?.close()) to execute before the early return when organizationId is null, ensuring stale databases are properly closed when organization resolution fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts`:
- Around line 264-279: The mutation in the terminal router always returns
success without checking whether the persistence operation actually succeeded.
Modify recordSessionLocationLaunchCommand in session-location-log.ts to return a
boolean (true on successful write/update, false on failure) instead of catching
and swallowing errors internally. Then update the mutation to capture the return
value from recordSessionLocationLaunchCommand and only return { success: true }
if the function actually returned true, allowing callers to know when recording
has failed rather than silently dropping metadata.
---
Outside diff comments:
In `@apps/desktop/src/main/lib/host-db.ts`:
- Around line 115-141: The getActiveHostDb() function returns null when no
active organization is resolved (when !organizationId is true), but this early
return happens before the cached database cleanup block. This leaves any
previously cached database open indefinitely. Move the cleanup logic that closes
cachedDb and resets cachedOrganizationId and cachedDbPath (the block containing
the try-catch that calls $client?.close()) to execute before the early return
when organizationId is null, ensuring stale databases are properly closed when
organization resolution fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b29eb6fa-cf5e-43db-9eaa-201239d12f53
📒 Files selected for processing (18)
apps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/host-db-selection.test.tsapps/desktop/src/main/lib/host-db.tsapps/desktop/src/main/lib/terminal/agent-resume.test.tsapps/desktop/src/main/lib/terminal/agent-resume.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/daemon/types.tsapps/desktop/src/main/lib/terminal/session-location-log.test.tsapps/desktop/src/main/lib/terminal/session-location-log.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.tsapps/desktop/src/renderer/stores/tabs/useTabsWithPresets.tspackages/host-service/drizzle/0008_drop_terminal_sessions_workspace_fk.sqlpackages/host-service/drizzle/meta/0008_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/src/db/schema.test.tspackages/host-service/src/db/schema.ts
✅ Files skipped from review due to trivial changes (3)
- packages/host-service/drizzle/0008_drop_terminal_sessions_workspace_fk.sql
- packages/host-service/drizzle/meta/0008_snapshot.json
- packages/host-service/drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
- apps/desktop/src/main/lib/terminal/types.ts
- apps/desktop/src/main/lib/terminal/daemon/types.ts
- apps/desktop/src/main/lib/terminal/session-location-log.ts
|
Pushed Changes:
Validation on
No unresolved, non-outdated inline review threads remain from the current bot feedback. @coderabbitai review |
|
The changes look well-reasoned:
Results will be posted once the diff analysis completes. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/terminal/agent-resume.ts (1)
665-699:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently switch to a different session when the stored
sessionIdno longer resolves.Once the stored transcript lookup fails, this falls back to cwd-only transcript matching and can return a different
sessionId. Downstream,daemon-managerwrites that new identity back to storage and auto-injects itsresumeCommand, so a missing transcript can reopen the wrong conversation instead of falling back to the manual-retry path described in the PR objective.A safer contract here is: if
params.sessionIdwas supplied, only accept an exact transcript match for that same session, otherwise returnnull/knownCommandand let the user recover manually.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/terminal/agent-resume.ts` around lines 665 - 699, When a stored sessionId is provided in params but its transcript lookup fails (hasStoredTranscript is false), the code should not fall back to searching transcripts by agent ID alone, as this can match a different sessionId and cause the wrong session to be reopened. Modify the logic to return null immediately after the transcript lookup failure when normalizedSessionId was explicitly provided, instead of continuing to call resolveCandidateFromTranscripts. Only allow the fallback transcript scan to occur when no specific sessionId was supplied in params.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/terminal/agent-resume.ts`:
- Around line 341-346: The code appends a resume clause to the command arguments
without checking if one already exists, which causes invalid duplicate resume
invocations when the original command is already something like "claude --resume
<id>" or "codex resume <id>". Before appending the resume clause (either
"--resume" for claude or "resume" for other agents), check whether parsed.args
already contains "resume" or parsed.command already includes a resume
invocation. Only construct the args with the appended resume parameters if no
resume clause is already present; otherwise, return the parsed command as-is
with its original args.
---
Outside diff comments:
In `@apps/desktop/src/main/lib/terminal/agent-resume.ts`:
- Around line 665-699: When a stored sessionId is provided in params but its
transcript lookup fails (hasStoredTranscript is false), the code should not fall
back to searching transcripts by agent ID alone, as this can match a different
sessionId and cause the wrong session to be reopened. Modify the logic to return
null immediately after the transcript lookup failure when normalizedSessionId
was explicitly provided, instead of continuing to call
resolveCandidateFromTranscripts. Only allow the fallback transcript scan to
occur when no specific sessionId was supplied in params.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e87514a-e90a-435b-815a-90948e0ddabc
📒 Files selected for processing (17)
apps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/main/lib/host-db.tsapps/desktop/src/main/lib/terminal/agent-resume.test.tsapps/desktop/src/main/lib/terminal/agent-resume.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/daemon/types.tsapps/desktop/src/main/lib/terminal/session-location-log.test.tsapps/desktop/src/main/lib/terminal/session-location-log.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.tsapps/desktop/src/renderer/stores/tabs/useTabsWithPresets.tspackages/host-service/drizzle/0008_drop_terminal_sessions_workspace_fk.sqlpackages/host-service/drizzle/meta/0008_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/src/db/schema.test.tspackages/host-service/src/db/schema.ts
✅ Files skipped from review due to trivial changes (2)
- packages/host-service/drizzle/0008_drop_terminal_sessions_workspace_fk.sql
- packages/host-service/drizzle/meta/0008_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/desktop/src/main/lib/terminal/types.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
- apps/desktop/src/main/lib/terminal/daemon/types.ts
- apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
- packages/host-service/drizzle/meta/_journal.json
- apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts
- packages/host-service/src/db/schema.test.ts
- packages/host-service/src/db/schema.ts
- apps/desktop/src/main/lib/host-db.ts
- apps/desktop/src/main/lib/terminal/agent-resume.test.ts
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts
- apps/desktop/src/main/lib/terminal/session-location-log.test.ts
- apps/desktop/src/main/lib/terminal/session-location-log.ts
|
@Kitenite thanks for the direction here. I updated the implementation to use the existing host-service DB path instead of a standalone file. What changed from the earlier approach:
I also updated the PR description to reflect the current DB-backed design and resolved the bot review threads. From my side this is ready for maintainer review when you have time. |
|
Small clarification on restore behavior: if the agent session was started from a Superset preset, the restore path uses the persisted Superset launch command/preset command as the base and appends/replaces the recovered resume clause for that agent session. So custom preset options/env/cwd-style launches are preserved instead of falling back to a generic |
|
Pushed follow-up commit
Validation on this branch:
@coderabbitai review |
@kunaldhongade I have started the AI code review. It will take a few minutes to complete. |
|
The fixes in this commit address meaningful edge cases:
Full local validation ( ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.ts`:
- Around line 125-142: The issue is that coldRestoreState is persisted as
restored at the anchor location (lines 125-142 where coldRestoreState.set is
called), but when the writeCommandInPane operation fails in the sibling location
(lines 254-275), the stale coldRestoreState is not cleared. This causes
useTerminalLifecycle to incorrectly read storedColdRestore.isRestored on remount
and re-enter restored-mode handling instead of normal live-session flow. To fix
this: in the failure/error handling path of the writeCommandInPane call (around
lines 254-275), add logic to clear or reset the coldRestoreState entry for the
paneId that was previously set to mark it as restored. This ensures that if
command injection fails, the cold restore state is not left in an inconsistent
state for the next mount cycle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e0e8eea-1126-48c4-9b4d-075169956efb
📒 Files selected for processing (13)
apps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/main/lib/terminal/agent-resume.test.tsapps/desktop/src/main/lib/terminal/agent-resume.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/env.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/state.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/v1-terminal-cache.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
- apps/desktop/src/main/lib/terminal/env.ts
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.test.ts
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts
- apps/desktop/src/main/lib/terminal/agent-resume.ts
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
If session-locations.json is malformed on first boot after upgrade, the previous code set legacyImportEnsured=true before re-throwing, permanently blocking the migration. Removing the inner try/catch lets the outer catch log the error while leaving the flag false so the next boot retries after the corrupt file is repaired or cleared.
…ilure When writeCommandInPane fails after handleStartShell creates the new shell, the coldRestoreState entry still had isRestored:true. On any subsequent remount useTerminalLifecycle would read that flag and re-enter the cold-restore overlay even though a live session was already running. Add clearRestoreResumeState() to the .catch() handler and update the test to assert the state is cleared on failure instead of retained.
|
@Kitenite thanks again for the earlier direction on using the DB here instead of a standalone file. The current head follows that design: restore/session metadata now lives on the existing host-service What changed on the final path:
Current state from my side:
If the scope looks right to you, I'd really appreciate a final review and fork-CI approval. If nothing else stands out on your side, I believe this is ready to merge. |
|
@Kitenite ping |
|
Really nice work, @kunaldhongade — I read through the branch closely and it's well-factored. Coming at this from a different angle: over the last few days we independently built a small userland auto-resume tool for the same problem (a zsh hook that resumes each restored pane from A few notes from reading it:
Two things we verified empirically that support the design here: Since this is waiting on review, we published our version as a works-today stopgap for people hitting this now — it auto-detects the native column and stands down when this merges, so no double-resume during the transition: https://github.com/2solarmax/superset-session-resume . Happy to help however's useful — and glad to see the native fix moving. |
Summary
host.db) by extending the existingterminal_sessionstable, instead of keeping a separatesession-locations.jsonstore~/.claude/projectsand~/.codex/sessionsduring cold restore, including full app relaunch and machine restart flowsterminal_sessions, so users do not lose restore continuity after updatingLinked Issue
Closes #3496
Also addresses #5126
Why
Desktop cold restore already preserved scrollback and cwd, but it stopped short of restoring the active Claude/Codex session. After an app quit, crash, or machine restart, users had to manually rediscover session IDs, and some restore paths could lose the tab or agent identity before the resume command completed.
User Impact
Implementation Notes
terminal_sessionstable inhost.db: tab/workspace identity, cwd/root paths, launch command, pid, agent id, agent session id, status, exit reason, and location key.0006imports the first DB-backed shape,0007folds restore metadata intoterminal_sessions, and0008removes the workspace FK that blocked desktop pane writes.session-location-lognow usesterminal_sessionsthrough the host-service DB adapter and only reads legacysession-locations.jsonfor one-time import/archive.Validation
bun run lintbun run typecheckbun run testbun run --cwd apps/desktop testbun run buildhealth.checkreturned200,host.dbmigrated through0008, and active Claude/Codex rows persisted expected commands/session IDs interminal_sessionsNotes
This includes the durable session-location work tracked in
#5126and extends it with actual Claude/Codex cold-resume behavior plus the follow-up restore hardening discovered during local restart/sleep testing.GitHub Actions required checks may still require maintainer approval because this is a fork PR, but local validation above is green on the current branch work.
Summary by CodeRabbit
New Features
Bug Fixes