[codex] fix sidebar notifications after host restart#4703
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:
📝 WalkthroughWalkthroughThe PR threads terminalId through agent hook templates, server, NotificationManager, main window routing, and renderer V2 subscription; separately, it adds deterministic preferred-port selection and remembering to host-service startup/spawn flows. ChangesTerminal ID propagation and host service improvements
Sequence DiagramsequenceDiagram
participant Agent as Agent Hook Script
participant Server as Notification Server
participant MainProc as Main Process NotificationManager
participant MainWin as Main Window Event Emitter
participant Renderer as Renderer V2Controller
Agent->>Server: curl /hook/complete with terminalId
Server->>MainProc: emit AgentLifecycleEvent (includes terminalId)
MainProc->>MainProc: onNotificationClick includes terminalId
MainWin->>MainWin: if workspaceId && terminalId -> emit FOCUS_V2_NOTIFICATION_SOURCE
MainWin->>Renderer: otherwise emit FOCUS_TAB
Server->>Renderer: subscribers receive AGENT_LIFECYCLE via RPC
Renderer->>Renderer: validate ids, lookup workspace, handleV2AgentLifecycleStatusEvent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Ready to review this PR? Stage has broken it down into 5 individual chapters for you: 
Chapters generated by Stage for commit c90c924 on May 19, 2026 5:38pm UTC. |
Greptile SummaryThis PR fixes v2 sidebar notification status when host-service restarts and adopted terminal PTYs carry a stale hook URL, by threading
Confidence Score: 3/5The stale-URL fix and port-preference logic are directionally correct, but the relaxed hook guard introduces duplicate notifications and sounds in every healthy v2 session, and the new subscription callback has a stale-closure that will silently miss status updates for workspaces added after mount. The hook guard change causes the Electron fallback to fire for all v2 terminal events, meaning users in healthy sessions will see two native notifications and hear two sounds for every agent completion or permission request — a visible regression that affects the core notification path. Separately, the new tRPC subscription in V2NotificationController captures workspaceStatesById without useEffectEvent, so any workspace added after the component mounts will silently fail to get sidebar status updates from the fallback path. The hook template files (notify-hook, copilot-hook, cursor-hook, gemini-hook) and V2NotificationController.tsx need the most attention before merging.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh | Relaxed exit guard now lets the Electron fallback fire for all v2 terminal events, causing double native notifications and double sound in healthy (non-stale-URL) sessions. |
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx | New tRPC subscription callback closes over workspaceStatesById without useEffectEvent; the closure goes stale after the first render where workspaces change, silently dropping status updates for new workspaces. |
| apps/desktop/src/main/lib/host-service-coordinator.ts | Port preference logic (FNV-1a hash, lastKnownPorts map, manifest port extraction) is well-structured; startWithPreferredPorts duplicates start() body but is otherwise correct. |
| apps/desktop/src/main/lib/host-service-utils.ts | canBindPort probe-then-release approach introduces an inherent TOCTOU window on preferred ports, but this is an accepted trade-off of preferred-port selection. |
| apps/desktop/src/main/lib/notifications/server.ts | terminalId is cleanly extracted from the query and threaded through both the short AgentLifecycleEvent and the extended emitted object; no issues. |
| apps/desktop/src/main/windows/main.ts | Click routing correctly dispatches FOCUS_V2_NOTIFICATION_SOURCE when both workspaceId and terminalId are present, falling back to FOCUS_TAB otherwise. |
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.ts | handleV2AgentLifecycleStatusEvent correctly calls only updatePaneStatus with no ringtone or native notification, satisfying the no-duplication intent. |
| apps/desktop/src/shared/notification-types.ts | Adds optional terminalId to NotificationIds; backward-compatible change with no issues. |
Sequence Diagram
sequenceDiagram
participant Shell as Adopted Shell
participant V2Hook as v2 Host-Service Hook URL
participant ElectronHook as Electron /hook/complete
participant NM as NotificationManager (main)
participant V2Sub as V2NotificationController (renderer)
participant HSub as HostNotificationSubscriber (renderer)
Note over Shell,HSub: Stale URL scenario (host-service restarted)
Shell->>V2Hook: curl SUPERSET_HOST_AGENT_HOOK_URL (FAILS)
Shell->>ElectronHook: "curl localhost/hook/complete?terminalId=..."
ElectronHook->>NM: AGENT_LIFECYCLE event
NM-->>Shell: native notification
ElectronHook->>V2Sub: tRPC subscription onData
V2Sub->>V2Sub: handleV2AgentLifecycleStatusEvent (status update only)
Note over Shell,HSub: Healthy scenario (v2 URL valid)
Shell->>V2Hook: curl SUPERSET_HOST_AGENT_HOOK_URL (succeeds)
V2Hook->>HSub: SSE AgentLifecyclePayload
HSub->>HSub: handleV2AgentLifecycleEvent (notification + ringtone)
Shell->>ElectronHook: "curl localhost/hook/complete?terminalId=... (NEW)"
ElectronHook->>NM: AGENT_LIFECYCLE event
NM-->>Shell: second native notification + ringtone (regression)
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh:96
**Double native notification + sound in healthy v2 scenarios**
The relaxed guard now allows the Electron fallback to fire for _every_ v2 terminal event, not just when the v2 hook URL is stale. In a healthy session where the host-service SSE connection is alive, `HostNotificationSubscriber` already calls `handleV2AgentLifecycleEvent` (which calls `showNativeNotification` + `playRingtone`). The Electron fallback now also fires, causing `NotificationManager.handleAgentLifecycle` to show a second native notification and play the sound a second time. The same pattern applies to the copilot/cursor/gemini templates.
### Issue 2 of 3
apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx:86-109
**Stale `workspaceStatesById` closure in subscription callback**
`electronTrpc` is `@trpc/react-query`'s `createTRPCReact`. Its `useSubscription` hook ties the subscription lifetime to the (stable) `undefined` input; it does NOT re-subscribe when the `onData` function reference changes. The `onData` here closes over `workspaceStatesById`, which is a `useMemo`-derived value. After the first render, if workspaces are added or removed, `workspaceStatesById` is recomputed but the running subscription still holds the stale closure. Any subsequent event for a newly-added workspace will silently miss the lookup at `workspaceStatesById.get(data.workspaceId)` and skip the status update entirely.
`HostNotificationSubscriber` handles the same problem with `useEffectEvent`; the same fix applies here.
### Issue 3 of 3
apps/desktop/src/main/lib/host-service-coordinator.ts:144-173
**`startWithPreferredPorts` duplicates the body of `start`**
The two methods are identical in structure — early-return if running, deduplicate via `pendingStarts`, run the adopt-or-spawn IIFE — differing only in whether `preferredPorts` is computed inline or taken as a parameter. This duplication means any future logic added to one method (e.g. error handling, status emission) needs to be mirrored in the other. Consider making `start` a thin wrapper that computes `preferredPorts` and delegates to `startWithPreferredPorts`.
Reviews (1): Last reviewed commit: "fix sidebar notifications after host res..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| # v1 fallback: Electron localhost hook server. Kept while v1 terminals exist. | ||
| [ -z "$SUPERSET_TAB_ID" ] && [ -z "$SESSION_ID" ] && exit 0 | ||
| [ -z "$SUPERSET_TAB_ID" ] && [ -z "$SESSION_ID" ] && [ -z "$SUPERSET_TERMINAL_ID" ] && exit 0 |
There was a problem hiding this comment.
The relaxed guard now allows the Electron fallback to fire for every v2 terminal event, not just when the v2 hook URL is stale. In a healthy session where the host-service SSE connection is alive, HostNotificationSubscriber already calls handleV2AgentLifecycleEvent (which calls showNativeNotification + playRingtone). The Electron fallback now also fires, causing NotificationManager.handleAgentLifecycle to show a second native notification and play the sound a second time. The same pattern applies to the copilot/cursor/gemini templates.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh
Line: 96
Comment:
**Double native notification + sound in healthy v2 scenarios**
The relaxed guard now allows the Electron fallback to fire for _every_ v2 terminal event, not just when the v2 hook URL is stale. In a healthy session where the host-service SSE connection is alive, `HostNotificationSubscriber` already calls `handleV2AgentLifecycleEvent` (which calls `showNativeNotification` + `playRingtone`). The Electron fallback now also fires, causing `NotificationManager.handleAgentLifecycle` to show a second native notification and play the sound a second time. The same pattern applies to the copilot/cursor/gemini templates.
How can I resolve this? If you propose a fix, please make it concise.| electronTrpc.notifications.subscribe.useSubscription(undefined, { | ||
| onData: (event) => { | ||
| if (event.type !== NOTIFICATION_EVENTS.AGENT_LIFECYCLE) return; | ||
| const data = event.data; | ||
| if (!data?.workspaceId || !data.terminalId) return; | ||
| const workspace = workspaceStatesById.get(data.workspaceId); | ||
| if (!workspace) return; | ||
|
|
||
| // Adopted shells keep their launch-time host-service hook URL. When | ||
| // that URL is stale, the Electron fallback still has terminal context. | ||
| handleV2AgentLifecycleStatusEvent({ | ||
| workspaceId: data.workspaceId, | ||
| payload: { | ||
| eventType: | ||
| data.eventType === "PendingQuestion" | ||
| ? "PermissionRequest" | ||
| : data.eventType, | ||
| terminalId: data.terminalId, | ||
| occurredAt: Date.now(), | ||
| }, | ||
| paneLayout: workspace.paneLayout, | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
workspaceStatesById closure in subscription callback
electronTrpc is @trpc/react-query's createTRPCReact. Its useSubscription hook ties the subscription lifetime to the (stable) undefined input; it does NOT re-subscribe when the onData function reference changes. The onData here closes over workspaceStatesById, which is a useMemo-derived value. After the first render, if workspaces are added or removed, workspaceStatesById is recomputed but the running subscription still holds the stale closure. Any subsequent event for a newly-added workspace will silently miss the lookup at workspaceStatesById.get(data.workspaceId) and skip the status update entirely.
HostNotificationSubscriber handles the same problem with useEffectEvent; the same fix applies here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx
Line: 86-109
Comment:
**Stale `workspaceStatesById` closure in subscription callback**
`electronTrpc` is `@trpc/react-query`'s `createTRPCReact`. Its `useSubscription` hook ties the subscription lifetime to the (stable) `undefined` input; it does NOT re-subscribe when the `onData` function reference changes. The `onData` here closes over `workspaceStatesById`, which is a `useMemo`-derived value. After the first render, if workspaces are added or removed, `workspaceStatesById` is recomputed but the running subscription still holds the stale closure. Any subsequent event for a newly-added workspace will silently miss the lookup at `workspaceStatesById.get(data.workspaceId)` and skip the status update entirely.
`HostNotificationSubscriber` handles the same problem with `useEffectEvent`; the same fix applies here.
How can I resolve this? If you propose a fix, please make it concise.| @@ -110,6 +172,45 @@ export class HostServiceCoordinator extends EventEmitter { | |||
| } | |||
| } | |||
There was a problem hiding this comment.
startWithPreferredPorts duplicates the body of start
The two methods are identical in structure — early-return if running, deduplicate via pendingStarts, run the adopt-or-spawn IIFE — differing only in whether preferredPorts is computed inline or taken as a parameter. This duplication means any future logic added to one method (e.g. error handling, status emission) needs to be mirrored in the other. Consider making start a thin wrapper that computes preferredPorts and delegates to startWithPreferredPorts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/host-service-coordinator.ts
Line: 144-173
Comment:
**`startWithPreferredPorts` duplicates the body of `start`**
The two methods are identical in structure — early-return if running, deduplicate via `pendingStarts`, run the adopt-or-spawn IIFE — differing only in whether `preferredPorts` is computed inline or taken as a parameter. This duplication means any future logic added to one method (e.g. error handling, status emission) needs to be mirrored in the other. Consider making `start` a thin wrapper that computes `preferredPorts` and delegates to `startWithPreferredPorts`.
How can I resolve this? If you propose a fix, please make it concise.
Kitenite
force-pushed
the
debug-sidebar-notificatio
branch
from
4cd257b to
0e8e376
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Kitenite
force-pushed
the
debug-sidebar-notificatio
branch
3 times, most recently
from
e08f100 to
4c81317
Compare
![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
🤖 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/components/V2NotificationController/V2NotificationController.tsx`:
- Around line 201-206: mergeWorkspaceHostRows currently iterates inFlightRows
then persistedRows so persistedRows overwrite fresher in-flight entries; reverse
the merge order or guard sets so in-flight wins: iterate persistedRows first
(populate rowsById from persistedRows) and then iterate inFlightRows calling
rowsById.set(row.workspaceId, row) so inFlightRows overwrite persisted, or
alternatively keep the current order but only set for persistedRows when
rowsById doesn't already have row.workspaceId; update the loops referencing
inFlightRows, persistedRows, rowsById, and row.workspaceId in
mergeWorkspaceHostRows accordingly.
🪄 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: cc7ff5b6-7a52-42e4-8177-8000f00fbb25
📒 Files selected for processing (22)
apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-cursor.tsapps/desktop/src/main/lib/agent-setup/agent-wrappers-gemini.tsapps/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/copilot-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/gemini-hook.template.shapps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.shapps/desktop/src/main/lib/host-service-coordinator.test.tsapps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/main/lib/host-service-utils.tsapps/desktop/src/main/lib/notifications/notification-manager.test.tsapps/desktop/src/main/lib/notifications/notification-manager.tsapps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/components/HostNotificationSubscriber/HostNotificationSubscriber.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/resolveV2NotificationTarget.test.tsapps/desktop/src/shared/notification-types.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/main/lib/notifications/notification-manager.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- apps/desktop/src/main/lib/notifications/server.ts
- apps/desktop/src/main/lib/agent-setup/agent-wrappers-copilot.ts
- apps/desktop/src/main/lib/notifications/notification-manager.test.ts
- apps/desktop/src/main/windows/main.ts
- apps/desktop/src/shared/notification-types.ts
- apps/desktop/src/main/lib/agent-setup/notify-hook.ts
- apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh
- apps/desktop/src/main/lib/agent-setup/agent-wrappers-cursor.ts
- apps/desktop/src/main/lib/agent-setup/templates/cursor-hook.template.sh
- apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts
- apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts
- apps/desktop/src/main/lib/agent-setup/templates/copilot-hook.template.sh
- apps/desktop/src/main/lib/host-service-utils.ts
- apps/desktop/src/main/lib/host-service-coordinator.ts
- apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.ts
Kitenite
force-pushed
the
debug-sidebar-notificatio
branch
from
4c81317 to
c90c924
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Fixes v2 sidebar notification status when host-service restarts while existing terminal PTYs are adopted instead of recreated.
Root Cause
Terminal adoption does restore the host-service terminal session row and
originWorkspaceId, so the terminal-to-workspace mapping itself is not the missing piece. The fragile part is the adopted shell process environment: an already-running shell keeps theSUPERSET_HOST_AGENT_HOOK_URLit launched with. If host-service is respawned on a different port, that URL can become stale and the v2 host-service hook post can fail.V2-only Scope
This is scoped to v2 terminal notifications:
HostServiceCoordinator, which is the v2 host-service path.SUPERSET_PORT/DESKTOP_NOTIFICATIONS_PORTfor the Electron notification server and is not moved to stable host-service ports./hook/completeendpoint remains the fallback transport, but the new v2 sidebar bridge only handles events that include bothworkspaceIdandterminalId. v1 terminals do not setSUPERSET_TERMINAL_ID, so their existing pane/tab notification behavior remains unchanged.Changes
terminalIdthrough the Electron/hook/completefallback and notification event types.terminalIdand allow terminal-only v2 context when tab IDs are absent.notify.shv3, Cursor/Gemini hooks v3, Copilot hook v2).workspaceId + terminalIdinto the v2 notification status store, without duplicating native notifications or ringtone handling.terminalIdis available.48000-48999range, then fall back to an ephemeral port if needed.Prior Behavior
Before this change, host-service spawn called
findFreePort()with no preferences. That implementation bound a temporary server to port0, so the OS selected any available ephemeral port on each spawn. That is why a respawn could produce a newSUPERSET_HOST_AGENT_HOOK_URLwhile adopted shells still held the old one.Validation
bun test apps/desktop/src/main/lib/host-service-coordinator.test.ts apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts apps/desktop/src/main/lib/notifications/notification-manager.test.ts apps/desktop/src/main/lib/notifications/server.test.ts apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.test.ts apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/resolveV2NotificationTarget.test.tsbun test apps/desktop/src/main/lib/host-service-coordinator.test.tsbun test apps/desktop/src/main/lib/agent-setup/notify-hook.test.ts apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.tsbun run lintbun run typecheckSummary by CodeRabbit
New Features
Bug Fixes
Tests