Repository navigation
fix(desktop): derive terminal agent status from host bindings (single source of truth) - #5449
Merged
Merged
Conversation
…ion liveness listByWorkspace/findActive now read via a SQL join to terminal_sessions (status = 'active' and workspace-owned), so bindings for dead terminals are unrepresentable in reads no matter how the terminal died (kill -9, crash, host downtime) — no exit listeners, startup drains, or read guards. A best-effort deleteDefunct() at startup keeps the table small. Supersedes the prune-layer approach in #5443.
Terminal agent runtime state (working/permission/idle) was tracked in two unreconciled places: the host's TerminalAgentBinding and a localStorage-persisted renderer status map replaying the same hook events. Missed events (app closed, WS disconnected) left phantom "working" dots forever. Statuses are now derived: working/permission/idle come from the binding's lastEventType, and review means the host recorded a Stop newer than a renderer-persisted per-terminal seen timestamp. The renderer store shrinks to manualUnread + terminalSeenAt; the whole source-status system (statusTransitions, setTerminalStatus, chat plumbing with zero callers) is deleted. Chime and native-notification logic stay event-driven and untouched. Interrupt (Esc/Ctrl+C) posts a synthetic Stop to the host since agents fire no Stop hook on interrupt, and the Electron fallback for adopted shells forwards events to the host so the binding converges.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR moves terminal agent runtime status derivation to host binding data, simplifies renderer notification storage to manual unread plus terminal seen timestamps, rewires consumers to derived hooks, and adds host-service liveness-joined reads and cleanup. ChangesTerminal agent status derivation and notification simplification
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Ready to review this PR? Stage has broken it down into 10 individual chapters for you: 
Chapters generated by Stage for commit a98ca31 on Jul 4, 2026 4:40am UTC. |
Contributor
Greptile SummaryTerminal agent status indicators (working / permission / idle / review) are now derived from the host's
Confidence Score: 4/5Safe to merge; the architectural change is structurally sound with good test coverage and no data-loss paths. The refactor correctly eliminates the stale-state bug by removing the second projection entirely. Host-side liveness join is well-tested with the new in-memory SQLite suite. The two remaining concerns are both shallow: a dead
|
| Filename | Overview |
|---|---|
| packages/host-service/src/terminal-agents/persistence.ts | Adds liveness-joined reads (listLiveByWorkspace, findLiveActive) and a best-effort deleteDefunct hygiene sweep; logic is clean and well-tested. |
| packages/host-service/src/terminal-agents/persistence.test.ts | New in-memory SQLite tests cover liveness filtering, store routing, multi-binding findLiveActive ordering, and deleteDefunct — good coverage of the new paths. |
| packages/host-service/src/terminal-agents/store.ts | Routes listByWorkspace/findActive through optional persistence-layer live joins; in-memory map kept as fallback for get() only. |
| packages/host-service/src/app.ts | Adds startup deleteDefunct call wrapped in try/catch so failures don't block launch; ordering (deleteDefunct before TerminalAgentStore construction) is correct. |
| apps/desktop/src/renderer/stores/v2-notifications/store.ts | Notification store shrunk to manualUnread/terminalSeenAt maps with version bump to 2; migration correctly preserves only manual-unread entries from v1 manual: source keys. |
| apps/desktop/src/renderer/hooks/host-service/useTerminalAgentStatuses/deriveTerminalAgentStatus.ts | Pure derivation function mapping lastEventType + seen-at comparison to UI status; straightforward and mutation-tested. |
| apps/desktop/src/renderer/hooks/host-service/useV2NotificationStatus/useV2NotificationStatus.ts | Replaces store-selector functions with hooks that compose useTerminalAgentStatuses + manualUnread; coverage of workspace/pane/sources queries looks complete. |
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.ts | Simplified to seen-marking + chime; dead workspaceId param on handleV2TerminalLifecycleEvent is a leftover (declared in type, not destructured). |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.ts | Now posts a synthetic Stop to the host on Esc/Ctrl+C; markTerminalSeenNow runs before the host confirms the Stop, which can produce a brief "review" flash (self-healing). |
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx | Adds host-forwarding for adopted-shell lifecycle events so the host binding stays in sync; only seen-marking is done locally now. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent as Claude Agent
participant Host as Host Service
participant WS as WebSocket
participant Renderer as Renderer
participant Store as v2-notifications Store
Agent->>Host: "POST /notifications/hook {Start}"
Host->>Host: "upsert binding (lastEventType=Start)"
Host->>WS: broadcast lifecycle event
WS->>Renderer: handleV2AgentLifecycleEvent
Renderer->>Renderer: markSeenIfTargetVisible (if pane visible)
Note over Renderer: useTerminalAgentStatuses derives status from binding.lastEventType
Agent->>Host: "POST /notifications/hook {Stop}"
Host->>Host: "upsert binding (lastEventType=Stop)"
Host->>WS: broadcast lifecycle event
WS->>Renderer: handleV2AgentLifecycleEvent → chime
Renderer->>Store: markTerminalSeen(terminalId, occurredAt) if visible
Note over Store,Renderer: lastSeenAt≥lastEventAt → idle / lastSeenAt<lastEventAt → review
Note over Renderer: Interrupt (Esc/Ctrl+C)
Renderer->>Store: markTerminalSeenNow(terminalId)
Renderer->>Host: "POST /notifications/hook {Stop} synthetic"
Host->>Host: "upsert binding (lastEventType=Stop)"
Host->>WS: broadcast
WS->>Renderer: markSeenIfTargetVisible (clears transient review)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Agent as Claude Agent
participant Host as Host Service
participant WS as WebSocket
participant Renderer as Renderer
participant Store as v2-notifications Store
Agent->>Host: "POST /notifications/hook {Start}"
Host->>Host: "upsert binding (lastEventType=Start)"
Host->>WS: broadcast lifecycle event
WS->>Renderer: handleV2AgentLifecycleEvent
Renderer->>Renderer: markSeenIfTargetVisible (if pane visible)
Note over Renderer: useTerminalAgentStatuses derives status from binding.lastEventType
Agent->>Host: "POST /notifications/hook {Stop}"
Host->>Host: "upsert binding (lastEventType=Stop)"
Host->>WS: broadcast lifecycle event
WS->>Renderer: handleV2AgentLifecycleEvent → chime
Renderer->>Store: markTerminalSeen(terminalId, occurredAt) if visible
Note over Store,Renderer: lastSeenAt≥lastEventAt → idle / lastSeenAt<lastEventAt → review
Note over Renderer: Interrupt (Esc/Ctrl+C)
Renderer->>Store: markTerminalSeenNow(terminalId)
Renderer->>Host: "POST /notifications/hook {Stop} synthetic"
Host->>Host: "upsert binding (lastEventType=Stop)"
Host->>WS: broadcast
WS->>Renderer: markSeenIfTargetVisible (clears transient review)
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.ts:92-100
**Dead `workspaceId` parameter**
`workspaceId` is declared in the parameter object type but never destructured, so it is silently ignored at runtime. TypeScript enforces that callers pass it, creating a false impression that it affects behavior. If `workspaceId` is no longer needed (the only remaining action is `pruneTerminalSeen` keyed on `terminalId`), remove it from the interface; if future callers will need it, destructure it with an `_` prefix to make the intent explicit.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.ts:32-45
**Transient "review" dot on interrupt due to client/host clock ordering**
`markTerminalSeenNow` stamps `lastSeenAt = Date.now()` on the client (T1) *before* the synthetic Stop is sent to the host. The host records `lastEventAt` when it processes the mutation (T2 ≥ T1 due to network round-trip). Until the resulting binding update propagates back over the WebSocket, `deriveTerminalAgentStatus` sees `lastEventAt (T2) > lastSeenAt (T1)` and briefly renders a "review" dot — the opposite of what an interrupt is supposed to do.
The window self-heals: when the WebSocket binding update arrives, `markSeenIfTargetVisible` runs (if the pane is visible) and clears the dot, or `useClearActivePaneAttention` catches the transient "review" state. A simple fix is to pass the client timestamp as `occurredAt` to the hook so the host uses it directly, or to update `lastSeenAt` after the host call resolves.
Reviews (1): Last reviewed commit: "fix(desktop): derive terminal agent stat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
92
to
100
| export function handleV2TerminalLifecycleEvent({ | ||
| workspaceId, | ||
| payload, | ||
| }: { | ||
| workspaceId: string; | ||
| payload: TerminalLifecyclePayload; | ||
| }): void { | ||
| if (payload.eventType !== "exit") return; | ||
| clearSources(workspaceId, [ | ||
| getV2TerminalNotificationSource(payload.terminalId), | ||
| ]); | ||
| } | ||
|
|
||
| function updatePaneStatus( | ||
| workspaceId: string, | ||
| payload: AgentLifecyclePayload, | ||
| target: V2NotificationTarget, | ||
| paneLayout: WorkspaceState<PaneViewerData> | null | undefined, | ||
| ): void { | ||
| const store = useV2NotificationStore.getState(); | ||
| const targetVisible = isV2NotificationTargetVisible({ | ||
| currentWorkspaceId: getCurrentWorkspaceId(), | ||
| paneLayout, | ||
| target, | ||
| }); | ||
| const transition = resolveV2AgentStatusTransition({ | ||
| workspaceId, | ||
| payload, | ||
| statuses: store.sources, | ||
| targetVisible, | ||
| }); | ||
|
|
||
| clearSources(workspaceId, transition.clearSources); | ||
| if (transition.setStatus) { | ||
| store.setSourceStatus( | ||
| transition.setStatus.source, | ||
| workspaceId, | ||
| transition.setStatus.status, | ||
| payload.occurredAt, | ||
| ); | ||
| } | ||
| useV2NotificationStore.getState().pruneTerminalSeen(payload.terminalId); | ||
| } |
Contributor
There was a problem hiding this comment.
workspaceId parameter
workspaceId is declared in the parameter object type but never destructured, so it is silently ignored at runtime. TypeScript enforces that callers pass it, creating a false impression that it affects behavior. If workspaceId is no longer needed (the only remaining action is pruneTerminalSeen keyed on terminalId), remove it from the interface; if future callers will need it, destructure it with an _ prefix to make the intent explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.ts
Line: 92-100
Comment:
**Dead `workspaceId` parameter**
`workspaceId` is declared in the parameter object type but never destructured, so it is silently ignored at runtime. TypeScript enforces that callers pass it, creating a false impression that it affects behavior. If `workspaceId` is no longer needed (the only remaining action is `pruneTerminalSeen` keyed on `terminalId`), remove it from the interface; if future callers will need it, destructure it with an `_` prefix to make the intent explicit.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+32
to
+45
| const binding = useTerminalAgentBinding(workspaceId, terminalId); | ||
|
|
||
| const recordInterrupt = useEffectEvent(() => { | ||
| markTerminalSeenNow(terminalId); | ||
| const agentActive = | ||
| binding?.lastEventType === "Start" || | ||
| binding?.lastEventType === "PermissionRequest"; | ||
| if (!agentActive || !hostUrl) return; | ||
| getHostServiceClientByUrl(hostUrl) | ||
| .notifications.hook.mutate({ terminalId, eventType: "Stop" }) | ||
| .catch((error) => { | ||
| console.warn( | ||
| "[terminal] failed to record synthetic agent stop:", | ||
| error, |
Contributor
There was a problem hiding this comment.
markTerminalSeenNow stamps lastSeenAt = Date.now() on the client (T1) before the synthetic Stop is sent to the host. The host records lastEventAt when it processes the mutation (T2 ≥ T1 due to network round-trip). Until the resulting binding update propagates back over the WebSocket, deriveTerminalAgentStatus sees lastEventAt (T2) > lastSeenAt (T1) and briefly renders a "review" dot — the opposite of what an interrupt is supposed to do.
The window self-heals: when the WebSocket binding update arrives, markSeenIfTargetVisible runs (if the pane is visible) and clears the dot, or useClearActivePaneAttention catches the transient "review" state. A simple fix is to pass the client timestamp as occurredAt to the hook so the host uses it directly, or to update lastSeenAt after the host call resolves.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.ts
Line: 32-45
Comment:
**Transient "review" dot on interrupt due to client/host clock ordering**
`markTerminalSeenNow` stamps `lastSeenAt = Date.now()` on the client (T1) *before* the synthetic Stop is sent to the host. The host records `lastEventAt` when it processes the mutation (T2 ≥ T1 due to network round-trip). Until the resulting binding update propagates back over the WebSocket, `deriveTerminalAgentStatus` sees `lastEventAt (T2) > lastSeenAt (T1)` and briefly renders a "review" dot — the opposite of what an interrupt is supposed to do.
The window self-heals: when the WebSocket binding update arrives, `markSeenIfTargetVisible` runs (if the pane is visible) and clears the dot, or `useClearActivePaneAttention` catches the transient "review" state. A simple fix is to pass the client timestamp as `occurredAt` to the hook so the host uses it directly, or to update `lastSeenAt` after the host call resolves.
How can I resolve this? If you propose a fix, please make it concise.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 26 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.ts:36">
P2: Interrupts can still leave a stale working/permission indicator when they occur before binding data is hydrated. This guard depends on renderer `binding.lastEventType`, so `binding === undefined` returns early and skips the synthetic host `Stop`; consider treating unknown binding as active (or sending Stop whenever `hostUrl` exists) and letting host-side logic no-op when nothing is active.</violation>
</file>
<file name="apps/desktop/src/renderer/hooks/host-service/useTerminalAgentStatuses/useTerminalAgentStatuses.ts">
<violation number="1" location="apps/desktop/src/renderer/hooks/host-service/useTerminalAgentStatuses/useTerminalAgentStatuses.ts:19">
P3: Any terminal seen update now rerenders and recomputes terminal-agent statuses for every mounted workspace/sidebar consumer, not just the workspace or terminal that changed. Consider narrowing the Zustand subscription to the bound terminal IDs (or using an equality-checked derived selector) so unrelated terminalSeenAt entries do not churn all status maps.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Agent as Agent Process
participant HostDB as Host SQLite<br/>(terminal_sessions + bindings)
participant HostPersistence as SqliteTerminalAgentBinding<br/>Persistence
participant HostStore as TerminalAgentStore
participant EventBus as EventBus / WS
participant RendererWS as Renderer WebSocket
participant NotifController as V2NotificationController
participant NotifStore as Zustand Store<br/>(terminalSeenAt only)
participant StatusHook as useTerminalAgentStatuses
participant UI as UI (Sidebar / Pane Tab)
participant UserKeyboard as User (Ctrl+C / Esc)
Note over Agent,HostDB: 1. Normal agent lifecycle event
Agent->>HostStore: POST /notifications.hook (Start / Stop / PermissionRequest)
HostStore->>HostPersistence: recordEvent()
HostPersistence->>HostDB: NEW: upsert (binding row)
HostStore-->>EventBus: emit lifecycle event
EventBus-->>RendererWS: broadcast agent lifecycle event
RendererWS->>NotifController: handleV2AgentLifecycleEvent()
alt CHANGED: target isn't visible & event is Stop
NotifController->>NotifController: play chime (unchanged)
end
opt target pane is visible
NotifController->>NotifStore: markTerminalSeen(terminalId, now)
end
Note over UI,StatusHook: 2. React reads derived status
UI->>StatusHook: useTerminalAgentStatuses(workspaceId)
StatusHook->>HostStore: NEW: listLiveByWorkspace(workspaceId)
HostStore->>HostPersistence: listLiveByWorkspace()
HostPersistence->>HostDB: NEW: SELECT bindings JOIN terminal_sessions<br/>WHERE status='active' AND origin IS NOT NULL
HostDB-->>HostPersistence: live bindings
HostPersistence-->>HostStore: TerminalAgentBinding[]
HostStore-->>StatusHook: bindings
StatusHook->>NotifStore: read terminalSeenAt
NotifStore-->>StatusHook: { terminalId: timestamp }
StatusHook->>StatusHook: NEW: deriveTerminalAgentStatus(lastEventType, lastEventAt, lastSeenAt)
StatusHook-->>UI: Map<terminalId, PaneStatus>
Note over UI: 3. Interrupt (Ctrl+C / Esc)
UserKeyboard->>UI: Ctrl+C in terminal
UI->>UI: useTerminalInterruptClear hook
UI->>NotifStore: NEW: markTerminalSeenNow(terminalId)
UI->>HostStore: NEW: notifications.hook.mutate({ terminalId, eventType: "Stop" })
HostStore->>HostPersistence: recordEvent (synthetic Stop)
HostPersistence->>HostDB: upsert binding with lastEventType='Stop', lastEventAt=now
HostStore-->>EventBus: broadcast Stop event --> renderer sees new binding
Note over Agent,HostDB: 4. Agent killed (kill -9) – no hook
Agent->>Agent: process dies
HostDB->>HostDB: terminal_session status set to 'exited'<br/>(by reaper / dispose route)
Note over HostPersistence,HostDB: Subsequent reads join on 'active' only → binding excluded
Note over HostStore: 5. Host startup – best-effort prune
HostStore->>HostPersistence: NEW: deleteDefunct()
HostPersistence->>HostDB: DELETE bindings WHERE session is missing/exited/disposed/orphan
HostDB-->>HostPersistence: rows removed (non-critical)
Note over Agent,HostDB: 6. Electron fallback for adopted shells
Agent->>UI: POST to Electron express server
UI->>NotifController: handleElectronAgentLifecycle (adopted shell)
NotifController->>HostStore: NEW: GET host URL, forward event to notifications.hook
HostStore-->>NotifController: binding updated (converges)
NotifController->>NotifStore: markV2AgentLifecycleTargetSeen (seen if visible)
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Comment on lines
+36
to
+39
| const agentActive = | ||
| binding?.lastEventType === "Start" || | ||
| binding?.lastEventType === "PermissionRequest"; | ||
| if (!agentActive || !hostUrl) return; |
Contributor
There was a problem hiding this comment.
P2: Interrupts can still leave a stale working/permission indicator when they occur before binding data is hydrated. This guard depends on renderer binding.lastEventType, so binding === undefined returns early and skips the synthetic host Stop; consider treating unknown binding as active (or sending Stop whenever hostUrl exists) and letting host-side logic no-op when nothing is active.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.ts, line 36:
<comment>Interrupts can still leave a stale working/permission indicator when they occur before binding data is hydrated. This guard depends on renderer `binding.lastEventType`, so `binding === undefined` returns early and skips the synthetic host `Stop`; consider treating unknown binding as active (or sending Stop whenever `hostUrl` exists) and letting host-side logic no-op when nothing is active.</comment>
<file context>
@@ -13,17 +16,37 @@ interface UseTerminalInterruptClearOptions {
+
+ const recordInterrupt = useEffectEvent(() => {
+ markTerminalSeenNow(terminalId);
+ const agentActive =
+ binding?.lastEventType === "Start" ||
+ binding?.lastEventType === "PermissionRequest";
</file context>
Suggested change
| const agentActive = | |
| binding?.lastEventType === "Start" || | |
| binding?.lastEventType === "PermissionRequest"; | |
| if (!agentActive || !hostUrl) return; | |
| const agentActive = | |
| binding === undefined || | |
| binding.lastEventType === "Start" || | |
| binding.lastEventType === "PermissionRequest"; | |
| if (!agentActive || !hostUrl) return; |
| ): Map<string, PaneStatus> { | ||
| const bindings = useTerminalAgentBindings(workspaceId, options); | ||
| const terminalSeenAt = useV2NotificationStore( | ||
| (state) => state.terminalSeenAt, |
Contributor
There was a problem hiding this comment.
P3: Any terminal seen update now rerenders and recomputes terminal-agent statuses for every mounted workspace/sidebar consumer, not just the workspace or terminal that changed. Consider narrowing the Zustand subscription to the bound terminal IDs (or using an equality-checked derived selector) so unrelated terminalSeenAt entries do not churn all status maps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/hooks/host-service/useTerminalAgentStatuses/useTerminalAgentStatuses.ts, line 19:
<comment>Any terminal seen update now rerenders and recomputes terminal-agent statuses for every mounted workspace/sidebar consumer, not just the workspace or terminal that changed. Consider narrowing the Zustand subscription to the bound terminal IDs (or using an equality-checked derived selector) so unrelated terminalSeenAt entries do not churn all status maps.</comment>
<file context>
@@ -0,0 +1,36 @@
+): Map<string, PaneStatus> {
+ const bindings = useTerminalAgentBindings(workspaceId, options);
+ const terminalSeenAt = useV2NotificationStore(
+ (state) => state.terminalSeenAt,
+ );
+
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts (1)
4-7: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLogic looks correct — combining
clearManualUnread+markWorkspaceTerminalsSeeninclearWorkspaceAttentionis necessary sinceuseV2WorkspaceIsUnreadtreats manual and derived "review" status as equivalent; without marking terminals seen, toggling "read" would be a no-op whenever unread was review-driven.One scalability note worth verifying:
useV2WorkspaceIsUnreadanduseMarkWorkspaceTerminalsSeenboth resolve throughuseTerminalAgentBindings(workspaceId)(a live-query subscription), and this hook is mounted unconditionally for every renderedDashboardSidebarWorkspaceItemin the sidebar. By contrast, the siblinguseDashboardSidebarWorkspaceRunningAgentsexplicitly threads anenabledflag into the same underlying primitives for lazy loading. Previously,isUnread/attention-clearing read a plain Zustand object (cheap); now every sidebar row establishes a live binding query regardless of visibility, which could scale poorly with many workspaces.Please confirm whether
useTerminalAgentBindings/useTerminalAgentStatusessubscriptions are cheap enough (cache-first, no per-subscription network cost) to mount unconditionally per sidebar row, or whetheruseV2WorkspaceIsUnread/useMarkWorkspaceTerminalsSeenshould also accept anenabledoption for consistency with the running-agents hook.♻️ Possible direction: thread `enabled` through the unread/seen hooks
-export function useV2WorkspaceIsUnread(workspaceId: string): boolean { - const statuses = useTerminalAgentStatuses(workspaceId); +export function useV2WorkspaceIsUnread( + workspaceId: string, + options?: { enabled?: boolean }, +): boolean { + const statuses = useTerminalAgentStatuses(workspaceId, options);Also applies to: 17-17, 41-49, 65-65, 150-162
🤖 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/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts` around lines 4 - 7, The unread/seen hooks used by useDashboardSidebarWorkspaceItemActions are currently always subscribing through useTerminalAgentBindings for every DashboardSidebarWorkspaceItem, which may be too expensive at sidebar scale. Thread an enabled option through useV2WorkspaceIsUnread and useMarkWorkspaceTerminalsSeen (and their underlying useTerminalAgentBindings/useTerminalAgentStatuses calls) so they can be mounted lazily like useDashboardSidebarWorkspaceRunningAgents, and update useDashboardSidebarWorkspaceItemActions to pass that flag where appropriate.
🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.ts:
- Around line 34-48: The interrupt clear flow in useTerminalInterruptClear
currently calls markTerminalSeenNow before recording the synthetic Stop in
recordInterrupt, which can leave terminalSeenAt behind the Stop event and keep
the pane flagged. Move the seen-update to happen only after
getHostServiceClientByUrl(hostUrl).notifications.hook.mutate succeeds, or update
it using the Stop’s occurredAt so the timestamps stay consistent.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts`:
- Around line 4-7: The unread/seen hooks used by
useDashboardSidebarWorkspaceItemActions are currently always subscribing through
useTerminalAgentBindings for every DashboardSidebarWorkspaceItem, which may be
too expensive at sidebar scale. Thread an enabled option through
useV2WorkspaceIsUnread and useMarkWorkspaceTerminalsSeen (and their underlying
useTerminalAgentBindings/useTerminalAgentStatuses calls) so they can be mounted
lazily like useDashboardSidebarWorkspaceRunningAgents, and update
useDashboardSidebarWorkspaceItemActions to pass that flag where appropriate.
🪄 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: a80f2e6a-0d37-49c9-a2ae-d04c67c0565e
📒 Files selected for processing (26)
apps/desktop/plans/done/20260703-1507-derive-terminal-agent-status-from-host-binding.mdapps/desktop/src/renderer/hooks/host-service/useTerminalAgentStatuses/deriveTerminalAgentStatus.test.tsapps/desktop/src/renderer/hooks/host-service/useTerminalAgentStatuses/deriveTerminalAgentStatus.tsapps/desktop/src/renderer/hooks/host-service/useTerminalAgentStatuses/index.tsapps/desktop/src/renderer/hooks/host-service/useTerminalAgentStatuses/useTerminalAgentStatuses.tsapps/desktop/src/renderer/hooks/host-service/useV2NotificationStatus/index.tsapps/desktop/src/renderer/hooks/host-service/useV2NotificationStatus/useV2NotificationStatus.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceDetails/hooks/useDashboardSidebarWorkspaceRunningAgents/useDashboardSidebarWorkspaceRunningAgents.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/V2NotificationStatusIndicator/V2NotificationStatusIndicator.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useClearActivePaneAttention/useClearActivePaneAttention.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.test.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.tsapps/desktop/src/renderer/stores/v2-notifications/index.tsapps/desktop/src/renderer/stores/v2-notifications/store.test.tsapps/desktop/src/renderer/stores/v2-notifications/store.tspackages/host-service/src/app.tspackages/host-service/src/terminal-agents/index.tspackages/host-service/src/terminal-agents/persistence.test.tspackages/host-service/src/terminal-agents/persistence.tspackages/host-service/src/terminal-agents/store.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.test.ts
- apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Member
Author
CDP end-to-end verificationDrove the dev desktop app over CDP ( Verified working end-to-end:
Not coverable over CDP in this environment (the dev workspace has a pane-layout sync bug — panes vanish from the layout seconds after creation, so no mounted
Those three remain on the manual QA checklist. |
#5351 added selectV2AttentionWorkspaceCount to the status-entry store this branch deleted. Reimplemented as useV2AttentionWorkspaceCount in the composite module: distinct workspaces with a derived permission/review terminal status (aggregated from cached bindings queries) or a manual unread mark; working excluded, matching the original semantics.
- Interrupt: re-mark the terminal seen after the synthetic Stop resolves, so the host-stamped Stop can't transiently derive as an unseen review (greptile/cubic/coderabbit). - Seen marks clamp to the binding's host-clock lastEventAt in useClearActivePaneAttention and useMarkWorkspaceTerminalsSeen, so a host clock ahead of the renderer can't leave an unclearable review. - Drop the unused workspaceId param from handleV2TerminalLifecycleEvent. - Cover deleteDefunct's missing-session branch in persistence.test.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Contributor
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/hooks/host-service/useV2NotificationStatus/useV2NotificationStatus.ts (1)
94-109: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the host timestamp directly here.
binding.lastEventAtis already host-clock data.Math.max(Date.now(), binding.lastEventAt)can store a renderer-side timestamp that stays ahead of later hostStopevents on non-local host connections, which can keep a realreviewstate hidden. Usebinding.lastEventAthere; only fall back toDate.now()when no host timestamp is available.🤖 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/hooks/host-service/useV2NotificationStatus/useV2NotificationStatus.ts` around lines 94 - 109, The timestamp handling in useMarkWorkspaceTerminalsSeen is clamping host data with Date.now(), which can hide later host-driven terminal reviews. Update the callback in useMarkWorkspaceTerminalsSeen to pass binding.lastEventAt directly into markTerminalSeen, and only use Date.now() as a fallback when a host timestamp is missing; keep the fix localized around useTerminalAgentBindings and markTerminalSeen.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useClearActivePaneAttention/useClearActivePaneAttention.ts (1)
33-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame clamp pattern duplicated — and same clock-mixing caveat applies.
This repeats the
Math.max(Date.now(), lastEventAt)clamp fromuseMarkWorkspaceTerminalsSeen(see comment inuseV2NotificationStatus.ts). Once that's resolved to use the host-clocklastEventAtdirectly, consider extracting a small shared helper (e.g.clampTerminalSeenAt(lastEventAt?: number)) to avoid drift between the two call sites.🛠️ Suggested fix
if (source.type !== "terminal") continue; - // Clamp to the binding's host-clock lastEventAt so a host clock - // ahead of the renderer can't leave an unclearable review. - const lastEventAt = bindings.get(source.id)?.lastEventAt ?? 0; - markTerminalSeen(source.id, Math.max(Date.now(), lastEventAt)); + const lastEventAt = bindings.get(source.id)?.lastEventAt; + markTerminalSeen(source.id, lastEventAt ?? Date.now());🤖 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/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useClearActivePaneAttention/useClearActivePaneAttention.ts around lines 33 - 44, The `useClearActivePaneAttention` effect is duplicating the same terminal-seen timestamp clamp and still mixes renderer time with the host clock. Update the `markTerminalSeen` call to use the host-provided `lastEventAt` directly for terminal sources, then extract the shared timestamp handling into a small helper like `clampTerminalSeenAt` so this hook and `useMarkWorkspaceTerminalsSeen` stay consistent and don’t drift.
🤖 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/hooks/host-service/useV2NotificationStatus/useV2NotificationStatus.ts`:
- Around line 94-109: The timestamp handling in useMarkWorkspaceTerminalsSeen is
clamping host data with Date.now(), which can hide later host-driven terminal
reviews. Update the callback in useMarkWorkspaceTerminalsSeen to pass
binding.lastEventAt directly into markTerminalSeen, and only use Date.now() as a
fallback when a host timestamp is missing; keep the fix localized around
useTerminalAgentBindings and markTerminalSeen.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useClearActivePaneAttention/useClearActivePaneAttention.ts:
- Around line 33-44: The `useClearActivePaneAttention` effect is duplicating the
same terminal-seen timestamp clamp and still mixes renderer time with the host
clock. Update the `markTerminalSeen` call to use the host-provided `lastEventAt`
directly for terminal sources, then extract the shared timestamp handling into a
small helper like `clampTerminalSeenAt` so this hook and
`useMarkWorkspaceTerminalsSeen` stay consistent and don’t drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a44652fe-a7b4-4b5c-ac00-2f2e3c925fc6
📒 Files selected for processing (8)
apps/desktop/src/renderer/hooks/host-service/useV2NotificationStatus/index.tsapps/desktop/src/renderer/hooks/host-service/useV2NotificationStatus/useV2NotificationStatus.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useClearActivePaneAttention/useClearActivePaneAttention.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.tsapps/desktop/src/renderer/routes/_authenticated/components/DockBadgeController/DockBadgeController.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/components/HostNotificationSubscriber/HostNotificationSubscriber.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.tspackages/host-service/src/terminal-agents/persistence.test.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/hooks/host-service/useV2NotificationStatus/index.ts
- packages/host-service/src/terminal-agents/persistence.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalInterruptClear/useTerminalInterruptClear.ts
Zero consumers, and its deltas fired only on hook-event store changes — not on session-liveness changes — so any future adopter would inherit stale snapshots. If a push channel is wanted later, it should ride the WS event bus with snapshot-on-connect semantics instead.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Replaces the onReconnect bus API + useWorkspaceEventBusReconnect with staleTime: 30s on the bindings query (window-focus refetch re-enabled by default). Events still invalidate for instant updates; staleness from a WS outage now self-heals on the next focus/remount via react-query's built-ins instead of custom protocol surface. workspace-client is back to matching main.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
This was referenced Jul 9, 2026
This was referenced Jul 20, 2026
This was referenced Aug 7, 2026
This branch was previously deployed
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
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.
Links
apps/desktop/plans/done/20260703-1507-derive-terminal-agent-status-from-host-binding.mdSummary
TerminalAgentBindinginstead of being replayed into a localStorage-persisted renderer status map. Stale "working" dots after an app restart become structurally impossible — there is no stored copy left to go stale.listByWorkspace/findActivejointerminal_sessionsonstatus = 'active', so agents in dead terminals (kill -9, crash, host downtime) disappear from reads with zero prune bookkeeping. This is the root fix for the leak fix(host-service): prune terminal-agent bindings on pty exit and at startup #5443 patched with compensating layers.manualUnread,terminalSeenAt).Why / Context
The binding store and the renderer status map were both projections of the same agent-hook stream, coupled only at the source (
notifications.hookbroadcasts the WS event and records the binding in one mutation). The renderer projection only updated while a window was open and connected, and was persisted with no reconciliation — any missedStop/Detachedbecame permanent drift: phantom "working" dots, workspace indicators lit for terminals that no longer exist.How It Works
working⇔binding.lastEventType === "Start",permission⇔"PermissionRequest",idle⇔"Stop"/"Attached"— a pure function (deriveTerminalAgentStatus) over the binding the renderer already fetches.review= the host recorded aStopnewer than a renderer-persisted per-terminallastSeenAt. Seen marks are written when a pane is focused, when an event lands while its target is visible, on interrupt/close, and by the sidebar mark-read action. Because it's computed from the host'slastEventAt, review survives the missed-event case (agent finishes while the app is closed → correct green dot on relaunch).Stopto the existingnotifications.hookendpoint (agents fire no Stop hook on interrupt), gated on the binding actually being working/permission so vim-style Escape spam doesn't spam the host.Manual QA Checklist
claudein a v2 workspace terminal, submit a prompt → amber "working" dot on pane tab, sidebar chip, workspace rowkill -9a bound agent's terminal shell → agent chip disappears from the sidebar without any hook firingv2-notifications-v1showsmanualUnread/terminalSeenAt, no legacysourcesstatusesTesting
cd apps/desktop && bun test— 2045 pass (note: must run with cwdapps/desktopsobunfig.tomlpreloadstest-setup.ts)cd packages/host-service && bun test— 750 pass, incl. newpersistence.test.ts(in-memory sqlite + migrations) covering the liveness join anddeleteDefunctbun run typecheck,bun run lint— cleanDesign Decisions
terminal_sessions.statusis the already-maintained liveness source (pty onExit, dispose routes, reaper healing). Reading through it makes dead bindings unrepresentable; every review finding on fix(host-service): prune terminal-agent bindings on pty exit and at startup #5443 was a failure mode of its patch machinery (listener leaks, startup crash, blocking broadcasts).lastSeenAt, not stored review: a stored review requires witnessing the event; a derived one is correct even when the app was closed when the agent finished.Known Limitations
reviewno longer survives agentDetached(binding is deleted when the agent process exits). Accepted: the chip disappears with the dot, and the failure is benign.Rollback
Revert the two commits; the persist migration is forward-only but harmless on rollback (old code treats the v2 blob as empty state and rebuilds from live events).
Summary by cubic
Derives terminal agent status from the host binding as the single source of truth, removing stale “working” dots and auto-hiding agents from dead terminals via session liveness. Adds a 30s cache window so focus/remount refetch self-heals WS outages; dock badge and workspace indicators now use the derived model.
Bug Fixes
working/permission/idlefromlastEventType,reviewwhen Stop > seen; seen marks use host time only and are written on focus/visible events; exit prunes seen.staleTime: 30s; lifecycle events still invalidate for instant updates.Refactors
useTerminalAgentStatusesanduseV2NotificationStatus; the store now keeps onlymanualUnreadandterminalSeenAtwith a v2 persist migration.terminalAgents.onWorkspaceChangestream; dock badge/workspace indicators aggregate derived statuses from the react‑query cache.listLiveByWorkspace/findLiveActive) anddeleteDefunct; derivation and persistence are covered by new tests.Written for commit a98ca31. Summary will update on new commits.
Summary by CodeRabbit