Conversation
…fication-hooks-on-the-client-side-instead-of-electron-side-because-its-ove
v2 terminals don't carry SUPERSET_PANE_ID, only SUPERSET_TERMINAL_ID, and agent payloads send empty strings for missing fields rather than omitting them. The sidebar status writer now threads terminalId through the whole pipeline (shell hook -> host-service -> event bus -> renderer) and falls back to terminalId/sessionId/hookSessionId as the store key when paneId is blank. Empty strings are now treated as missing (was using ??, now uses a firstNonBlank helper). Adds a workspace-level v2 pane-status store so the dashboard sidebar can render the same working/permission/review indicator the v1 sidebar did, and clears attention statuses when the user views the workspace. Also logs each hop of the pipeline so future debugging doesn't require guessing where the chain breaks.
Mount `V2AgentHookListeners` at the authenticated layout level so every open v2 workspace subscribes for agent-lifecycle events. Backgrounded workspaces now light up the sidebar dot and play the finish sound, not just the currently-viewed one. Multiple listeners per host reuse one WebSocket connection, so this is O(1 socket per host). Strips the per-hop console.logs added while debugging the initial pipeline — they served their purpose (empty-string paneId fallback bug in the coalesce chain, missing terminalId forwarding).
|
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:
📝 WalkthroughWalkthroughAdds v2 agent lifecycle notifications: agents POST to a host-service hook; host-service normalizes and broadcasts Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent / Terminal
participant HostSvc as Host-Service
participant EventBus as Event Bus / WebSocket
participant Client as Renderer
participant UI as Sidebar / Notifications
Agent->>HostSvc: POST /hook/complete (v2 JSON)
HostSvc->>HostSvc: Validate & map eventType
HostSvc->>EventBus: broadcastAgentLifecycle({workspaceId, eventType, ids, occurredAt})
EventBus->>Client: "agent:lifecycle" websocket message
Client->>Client: useV2AgentHookListener receives payload
Client->>Client: Update v2-pane-status store (paneId -> status)
Client->>UI: Derive workspaceStatus from store
UI->>UI: Render sidebar status indicator
Client->>Client: Evaluate suppression (pane visible & focused?)
alt suppressed
Client->>Client: Skip ringtone & notification
else
Client->>Client: playRingtone(selected, volume)
Client->>Client: Show native Notification (if permission)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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 |
Greptile SummaryThis PR migrates v2 agent notification hooks from Electron-main (
Confidence Score: 3/5Two concrete bugs need addressing before merge: suppression never fires (regression vs v1 behaviour) and the audio-primer retry path is silently broken. The architecture and host-service/event-bus plumbing are correct. However two real behavioural bugs affect the core UX this PR ships: (1) shouldSuppress always returns false for v2 because paneId is never populated in v2 hook payloads, breaking the stated v1 parity requirement; (2) primeRingtoneAudioOnFirstGesture removes the retry listener immediately after adding it in the catch block. Both are simple fixes. useV2AgentHookListener.ts (suppression logic) and ringtones/play.ts (audio primer retry)
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2AgentHookListener/useV2AgentHookListener.ts | Core hook bridging WebSocket events to sound/status; sound suppression (v1 parity) is dead code for v2 because paneId is always empty in v2 payloads |
| apps/desktop/src/renderer/lib/ringtones/play.ts | New audio playback module; autoplay-unlock retry logic is broken — listeners removed immediately after re-adding in error path |
| packages/host-service/src/trpc/router/notifications/notifications.ts | New tRPC hook endpoint; cleanly validates, maps event type, and fans out via event bus behind protectedProcedure |
| apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh | Adds v2 hook path that POSTs to host-service then exits; json_escape helper missing newline/CR/tab handling |
| packages/host-service/src/events/event-bus.ts | Adds broadcastAgentLifecycle delegating to existing broadcast; matches git:changed pattern |
| apps/desktop/src/renderer/stores/v2-pane-status/store.ts | New Zustand store for per-pane agent status; clearWorkspaceAttention correctly leaves working/permission untouched |
| packages/host-service/src/terminal/terminal.ts | Injects hostAgentHookUrl (localhost only) and HOST_SERVICE_SECRET token into terminal env |
| packages/workspace-client/src/lib/eventBus.ts | Extends client-side event bus to handle agent:lifecycle messages; dispatching and cleanup look correct |
| apps/desktop/src/renderer/routes/_authenticated/components/V2AgentHookListeners/V2AgentHookListeners.tsx | Mounts one listener per v2 workspace at layout level so backgrounded workspaces update sidebar; correctly keyed |
| packages/host-service/src/events/map-event-type.ts | Normalizes diverse agent event strings into Start/Stop/PermissionRequest; comprehensive mapping, returns null for unknowns |
Sequence Diagram
sequenceDiagram
participant Agent as Agent Shell Hook
participant HS as host-service tRPC
participant EB as EventBus WebSocket
participant R as Renderer V2AgentHookListeners
participant Store as v2PaneStatusStore
participant Audio as HTMLAudioElement
Agent->>HS: POST /trpc/notifications.hook with Bearer PSK
Note over Agent,HS: v2 path when SUPERSET_HOST_AGENT_HOOK_URL is set
HS->>HS: mapEventType and validate workspaceId
HS->>EB: broadcastAgentLifecycle payload
EB-->>R: agent lifecycle WS message to all clients
R->>Store: updatePaneStatus working or permission or review or idle
R->>R: shouldSuppress check (always false for v2 - paneId empty)
R->>Audio: playRingtone ringtoneId volume muted
R->>R: showNativeNotification
Note over Store: useClearPaneAttentionOnView clears review on workspace mount
Comments Outside Diff (2)
-
apps/desktop/src/renderer/lib/ringtones/play.ts, line 118-135 (link)Retry listener removed immediately in error path
In the catch block the code re-adds a
pointerdownlistener to retry on the next gesture, but then the tworemoveEventListenercalls at lines 130–131 run unconditionally (they're not inside the catch block). At that point the newly-added listener is immediately removed, so no retry ever happens, contrary to the comment "listener is re-added below."The
removeEventListenerpair is meant to cancel whichever of the two (pointerdown / keydown) wasn't the one that triggeredprime. They should only run on the success path:export function primeRingtoneAudioOnFirstGesture(): void { if (audioPrimed || typeof window === "undefined") return; const prime = () => { audioPrimed = true; // Cancel the other listener first (success path cleanup). window.removeEventListener("pointerdown", prime); window.removeEventListener("keydown", prime); const silent = new Audio(); silent.muted = true; silent.play().catch(() => { audioPrimed = false; window.addEventListener("pointerdown", prime, { once: true }); window.addEventListener("keydown", prime, { once: true }); }); }; window.addEventListener("pointerdown", prime, { once: true }); window.addEventListener("keydown", prime, { once: true }); }
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/lib/ringtones/play.ts Line: 118-135 Comment: **Retry listener removed immediately in error path** In the catch block the code re-adds a `pointerdown` listener to retry on the next gesture, but then the two `removeEventListener` calls at lines 130–131 run unconditionally (they're not inside the catch block). At that point the newly-added listener is immediately removed, so no retry ever happens, contrary to the comment "listener is re-added below." The `removeEventListener` pair is meant to cancel whichever of the two (pointerdown / keydown) wasn't the one that triggered `prime`. They should only run on the success path: ```typescript export function primeRingtoneAudioOnFirstGesture(): void { if (audioPrimed || typeof window === "undefined") return; const prime = () => { audioPrimed = true; // Cancel the other listener first (success path cleanup). window.removeEventListener("pointerdown", prime); window.removeEventListener("keydown", prime); const silent = new Audio(); silent.muted = true; silent.play().catch(() => { audioPrimed = false; window.addEventListener("pointerdown", prime, { once: true }); window.addEventListener("keydown", prime, { once: true }); }); }; window.addEventListener("pointerdown", prime, { once: true }); window.addEventListener("keydown", prime, { once: true }); } ``` How can I resolve this? If you propose a fix, please make it concise.
-
apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh, line 10-12 (link)json_escapedoesn't sanitize control charactersThe helper escapes
\and", which covers the most common injection vectors, but doesn't handle newlines (\n), carriage returns (\r), or tabs (\t). If any variable (particularlySESSION_IDorHOOK_SESSION_ID, which may come from external agent runtimes) contains a literal newline, the resulting JSON string will be malformed and thecurlPOST will be rejected by the server, silently dropping the notification.Consider adding control-character stripping:
json_escape() { printf '%s' "$1" | sed -e 's/\\/\\\\/g' -e 's/"/\\"/g' \ -e 's/\n/\\n/g' -e 's/\r/\\r/g' -e 's/\t/\\t/g' }
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: 10-12 Comment: **`json_escape` doesn't sanitize control characters** The helper escapes `\` and `"`, which covers the most common injection vectors, but doesn't handle newlines (`\n`), carriage returns (`\r`), or tabs (`\t`). If any variable (particularly `SESSION_ID` or `HOOK_SESSION_ID`, which may come from external agent runtimes) contains a literal newline, the resulting JSON string will be malformed and the `curl` POST will be rejected by the server, silently dropping the notification. Consider adding control-character stripping: ```sh json_escape() { printf '%s' "$1" | sed -e 's/\\/\\\\/g' -e 's/"/\\"/g' \ -e 's/\n/\\n/g' -e 's/\r/\\r/g' -e 's/\t/\\t/g' } ``` How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2AgentHookListener/useV2AgentHookListener.ts
Line: 120-141
Comment:
**Sound suppression never fires for v2 events**
`shouldSuppress` guards on `payload.paneId` being truthy, but the code in `updatePaneStatus` explicitly acknowledges that v2 terminals don't populate `paneId` (only `terminalId` / `sessionId` are reliable). Because `paneId` arrives as an empty string (the hook payload comment: "agents frequently send empty strings for missing fields"), the first guard on line 124 short-circuits to `false` on every v2 event. That means the v1-parity suppression rule — "don't play if the pane is visible and the window is focused" — is dead code for v2.
Result: the ringtone plays on every `Stop`/`PermissionRequest` event even when the user is actively looking at the terminal, unlike v1 behavior.
A straightforward fix is to fall back to a workspace-level visibility check when `paneId` is blank, mirroring the `isCurrentWorkspace` helper already used in `updatePaneStatus`:
```typescript
function shouldSuppress(
workspaceId: string,
payload: AgentLifecyclePayload,
): boolean {
if (typeof document !== "undefined" && document.hidden) return false;
if (typeof window !== "undefined" && !document.hasFocus()) return false;
// v2 payloads don't carry paneId; fall back to workspace-level visibility.
if (!payload.paneId || !payload.tabId) {
return isCurrentWorkspace(workspaceId);
}
const tabsState = useTabsStore.getState();
return isPaneVisible({
currentWorkspaceId: workspaceId,
tabsState: {
activeTabIds: tabsState.activeTabIds,
focusedPaneIds: tabsState.focusedPaneIds,
},
pane: {
workspaceId,
tabId: payload.tabId,
paneId: payload.paneId,
},
});
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/ringtones/play.ts
Line: 118-135
Comment:
**Retry listener removed immediately in error path**
In the catch block the code re-adds a `pointerdown` listener to retry on the next gesture, but then the two `removeEventListener` calls at lines 130–131 run unconditionally (they're not inside the catch block). At that point the newly-added listener is immediately removed, so no retry ever happens, contrary to the comment "listener is re-added below."
The `removeEventListener` pair is meant to cancel whichever of the two (pointerdown / keydown) wasn't the one that triggered `prime`. They should only run on the success path:
```typescript
export function primeRingtoneAudioOnFirstGesture(): void {
if (audioPrimed || typeof window === "undefined") return;
const prime = () => {
audioPrimed = true;
// Cancel the other listener first (success path cleanup).
window.removeEventListener("pointerdown", prime);
window.removeEventListener("keydown", prime);
const silent = new Audio();
silent.muted = true;
silent.play().catch(() => {
audioPrimed = false;
window.addEventListener("pointerdown", prime, { once: true });
window.addEventListener("keydown", prime, { once: true });
});
};
window.addEventListener("pointerdown", prime, { once: true });
window.addEventListener("keydown", prime, { once: true });
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh
Line: 10-12
Comment:
**`json_escape` doesn't sanitize control characters**
The helper escapes `\` and `"`, which covers the most common injection vectors, but doesn't handle newlines (`\n`), carriage returns (`\r`), or tabs (`\t`). If any variable (particularly `SESSION_ID` or `HOOK_SESSION_ID`, which may come from external agent runtimes) contains a literal newline, the resulting JSON string will be malformed and the `curl` POST will be rejected by the server, silently dropping the notification.
Consider adding control-character stripping:
```sh
json_escape() {
printf '%s' "$1" | sed -e 's/\\/\\\\/g' -e 's/"/\\"/g' \
-e 's/\n/\\n/g' -e 's/\r/\\r/g' -e 's/\t/\\t/g'
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore(desktop): hoist v2 agent-hook list..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/components/V2AgentHookListeners/V2AgentHookListeners.tsx (1)
35-38: MoveWorkspaceListenerinto its own component file.This TSX file now contains both
V2AgentHookListenersandWorkspaceListener. Please split the helper component into its own folder/file or otherwise restructure to satisfy the repository component rule. As per coding guidelines,**/*.{tsx,ts}: “Do not create multi-component files; use one component per file.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/V2AgentHookListeners/V2AgentHookListeners.tsx` around lines 35 - 38, The file currently contains two components; extract the helper component WorkspaceListener into its own component file (e.g., a new WorkspaceListener.tsx) and export it; update the original V2AgentHookListeners component to import and use the new WorkspaceListener; ensure the new file imports useV2AgentHookListener and types it as ({ workspaceId }: { workspaceId: string }) => null, default-export the component, and update any relative imports/exports so the project still builds and adheres to the one-component-per-file rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh`:
- Around line 82-101: The script currently unconditionally exits after
attempting the host-service POST, which drops the v1 fallback on curl failure or
non-2xx responses; change the logic around
SUPERSET_HOST_AGENT_HOOK_URL/SUPERSET_HOST_AGENT_HOOK_TOKEN so both
DEBUG_HOOKS_ENABLED and non-debug branches capture the HTTP status into
STATUS_CODE (use curl -o /dev/null -w "%{http_code}" or equivalent), log when
DEBUG_HOOKS_ENABLED, and only run exit 0 if STATUS_CODE is a 2xx (e.g. check
STATUS_CODE starts with "2" or 200<=STATUS_CODE<300); otherwise do not exit so
the existing v1 fallback can run. Ensure you update both branches that currently
call curl and remove the unconditional exit 0.
In `@apps/desktop/src/renderer/lib/ringtones/play.ts`:
- Around line 15-39: The current primeRingtoneAudioOnFirstGesture logic can
stack event listeners across repeated mounts and drops keyboard-based retries;
add a new module-level flag (e.g., audioPrimingListenersAdded) to avoid
re-adding listeners if they've already been registered, and change the prime
handler so it only sets audioPrimed = true after silent.play() resolves
successfully; on play().catch(...) re-add both pointerdown and keydown listeners
(with { once: true }) to preserve keyboard retry, and on successful play remove
both listeners; reference the existing audioPrimed variable and the
primeRingtoneAudioOnFirstGesture / prime handler when making these changes.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2AgentHookListener/useV2AgentHookListener.ts:
- Around line 120-140: The suppression currently returns false if payload.paneId
or payload.tabId is missing, which breaks v2 terminals that send terminalId
instead; update shouldSuppress to handle a v2 fallback: when paneId/tabId are
absent but payload.terminalId exists, look up tabsState
(useTabsStore.getState()) to find a pane/tab in the same workspace that has that
terminalId (or otherwise check workspace-level visibility) and use that pane/tab
info with isPaneVisible; only fall back to playing audio if no matching pane/tab
or workspace is visible. Ensure you reference shouldSuppress,
AgentLifecyclePayload, payload.terminalId, useTabsStore.getState(), and
isPaneVisible in your change.
- Around line 111-114: The isCurrentWorkspace function's regex only matches
"/workspace/:id" so it returns false for the v2 route; update the regex in
isCurrentWorkspace (in useV2AgentHookListener.ts) to match the v2 route (for
example allow "v2-workspace" or both variants) — e.g. change the pattern to
capture the workspaceId from either "/v2-workspace/:id" or "/workspace/:id"
(e.g. use a non-capturing optional prefix like /\/(?:v2-)?workspace\/([^/?#]+)/)
so the hook correctly detects the current v2 workspace and will not mark viewed
completions as review.
- Around line 156-160: The notification tag uses only payload.paneId or
payload.sessionId and falls back to "_" causing collisions; update the tag
construction in useV2AgentHookListener where new Notification(...) is created to
reuse the existing firstNonBlank helper to pick the first non-empty identifier
among payload.paneId, payload.sessionId, payload.terminalId,
payload.hookSessionId, payload.resourceId and then fallback to "_" so the tag
becomes `${workspaceId}:${firstNonBlank(...)}` ensuring
terminalId/hookSessionId/resourceId participate; locate the Notification call in
useV2AgentHookListener and replace the ad-hoc tag expression with the
firstNonBlank-based identifier.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx:
- Around line 109-115: The effect in useClearPaneAttentionOnView only runs on
mount/workspaceId changes, so new "review" attention added while the workspace
is open is not cleared; update useClearPaneAttentionOnView to also subscribe to
the workspace's attention value and re-run clearWorkspaceAttention whenever that
attention state changes (e.g. select the workspace's attention/status via
useV2PaneStatusStore for workspaceId and include it in the effect deps) so
clearWorkspaceAttention(workspaceId) is invoked when a new "review" status
arrives while viewing the workspace.
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 280-281: The code currently injects the broad HOST_SERVICE_SECRET
into PTY env via hostAgentHookToken (hostAgentHookUrl/getHostAgentHookUrl()),
which exposes a general credential to shell processes; remove passing
process.env.HOST_SERVICE_SECRET into the PTY and instead provision a hook-scoped
secret or ephemeral nonce specifically for the notifications.hook (e.g., create
and use HOOK_SCOPED_TOKEN or generate a per-session nonce from the host-service
before spawning the PTY), pass that limited token into hostAgentHookToken, and
update/verify the notifications.hook handler to accept and validate only this
hook-scoped secret/nonce (not the general host-service credential) so the
terminal cannot misuse broader host-service permissions.
In `@packages/host-service/src/trpc/router/notifications/notifications.ts`:
- Around line 31-43: Add an explicit workspace authorization check in the hook
mutation before calling ctx.eventBus.broadcastAgentLifecycle: after computing
eventType and confirming input.workspaceId, verify the caller is authorized for
input.workspaceId (e.g., using the request/session/token info on ctx such as
ctx.session.user or a helper like ctx.ensureWorkspaceAccess or
ctx.checkWorkspaceAccess(workspaceId)); if the check fails, return { success:
true, ignored: true } or throw an unauthorized error instead of broadcasting.
Ensure this check sits in the hook handler (the
protectedProcedure.input(hookInput).mutation) immediately before invoking
ctx.eventBus.broadcastAgentLifecycle so only authorized tokens can trigger
lifecycle broadcasts for that workspace.
In `@plans/20260422-v2-notification-hooks-client-side.md`:
- Around line 22-49: The diagram and plan reference the old POST /hook/complete
path; update the docs to reflect the implemented tRPC route
(/trpc/notifications.hook), add a language tag to the code fence (use ```text),
and adjust the explanatory lines to mention the host-service tRPC mutation and
its behavior; specifically edit the block showing the flow to use "agent ──POST
/trpc/notifications.hook──▶ host-service" and change the bullet about adding
POST /hook/complete to instead describe adding the host-service tRPC hook
mutation (and reference porting validation from map-event-type.ts), and update
the exit criteria line to state that posting to /trpc/notifications.hook
produces a WebSocket broadcast on agent:lifecycle (the WebSocket event-bus
broadcast scoped by workspaceId).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V2AgentHookListeners/V2AgentHookListeners.tsx`:
- Around line 35-38: The file currently contains two components; extract the
helper component WorkspaceListener into its own component file (e.g., a new
WorkspaceListener.tsx) and export it; update the original V2AgentHookListeners
component to import and use the new WorkspaceListener; ensure the new file
imports useV2AgentHookListener and types it as ({ workspaceId }: { workspaceId:
string }) => null, default-export the component, and update any relative
imports/exports so the project still builds and adheres to the
one-component-per-file rule.
🪄 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: fd342cf0-23e8-42e8-bd6f-e4655a853890
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.shapps/desktop/src/renderer/hooks/host-service/useWorkspaceEvent/useWorkspaceEvent.tsapps/desktop/src/renderer/lib/ringtones/play.tsapps/desktop/src/renderer/lib/ringtones/urls.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2AgentHookListener/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2AgentHookListener/isPaneVisible.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2AgentHookListener/useV2AgentHookListener.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2AgentHookListeners/V2AgentHookListeners.tsxapps/desktop/src/renderer/routes/_authenticated/components/V2AgentHookListeners/index.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/stores/v2-pane-status/index.tsapps/desktop/src/renderer/stores/v2-pane-status/store.tspackages/host-service/src/app.tspackages/host-service/src/events/event-bus.tspackages/host-service/src/events/index.tspackages/host-service/src/events/map-event-type.tspackages/host-service/src/events/types.tspackages/host-service/src/terminal/env.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/notifications/index.tspackages/host-service/src/trpc/router/notifications/notifications.tspackages/host-service/src/trpc/router/router.tspackages/host-service/src/types.tspackages/workspace-client/src/index.tspackages/workspace-client/src/lib/eventBus.tsplans/20260422-v2-notification-hooks-client-side.md
There was a problem hiding this comment.
9 issues found across 30 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/useV2AgentHookListener/useV2AgentHookListener.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2AgentHookListener/useV2AgentHookListener.ts:113">
P2: `isCurrentWorkspace` uses a v1 route regex (`/workspace/...`) so v2 workspace URLs are not recognized, which can leave Stop events in `review` instead of clearing to `idle` while the workspace is open.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2AgentHookListener/useV2AgentHookListener.ts:124">
P2: `shouldSuppress` returns `false` (no suppression) when `paneId` or `tabId` is missing, but v2 terminals only expose `terminalId` — not `paneId`/`tabId`. Normal v2 completions will never be suppressed, so the ringtone plays even when the user is focused on the workspace where the agent finished.</violation>
<violation number="3" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2AgentHookListener/useV2AgentHookListener.ts:130">
P1: `shouldSuppress` passes the event workspace as `currentWorkspaceId`, so visibility checks can treat background workspaces as visible and suppress notifications incorrectly.</violation>
<violation number="4" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2AgentHookListener/useV2AgentHookListener.ts:159">
P2: For v2 events where `paneId` and `sessionId` are both absent, every notification in the workspace gets the same tag (`<workspaceId>:_`), causing each new notification to replace the previous one. Reuse the `firstNonBlank` helper (already defined above) to include `terminalId`/`hookSessionId`/`resourceId` as fallbacks.</violation>
</file>
<file name="plans/20260422-v2-notification-hooks-client-side.md">
<violation number="1" location="plans/20260422-v2-notification-hooks-client-side.md:64">
P2: Use a cross-tab channel for deduplication; `sessionStorage` is isolated per tab and won't prevent duplicate playback across tabs.</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh:85">
P1: The unconditional `exit 0` after the v2 POST prevents the documented v1 fallback when host-service is unreachable.</violation>
</file>
<file name="apps/desktop/src/renderer/lib/ringtones/play.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/ringtones/play.ts:32">
P2: The retry path re-registers only `pointerdown` and drops `keydown`, so failed priming can stop working for keyboard-only interactions.</violation>
</file>
<file name="packages/host-service/src/terminal/terminal.ts">
<violation number="1" location="packages/host-service/src/terminal/terminal.ts:281">
P1: Avoid exposing the global `HOST_SERVICE_SECRET` to terminal child processes; it grants broad host-service API access if exfiltrated from PTY env.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx:115">
P2: This effect only runs on mount/workspace change. If an agent completes while the user is already viewing the workspace, the listener can add a new `"review"` status after this effect ran, leaving a stale sidebar attention indicator on the active page. Subscribe to the review-status presence so the effect re-fires.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- isCurrentWorkspace now matches v2 routes (`/v2-workspace/...`), not just v1. Without this, Stop events always marked `review` even while the user was viewing the workspace. - shouldSuppress falls back to isCurrentWorkspace + focus when the payload lacks paneId/tabId — v2 terminals only expose terminalId, so the previous early-return made v1-parity suppression dead code. - Native notification `tag` reuses firstNonBlank so v2 events don't collide on `workspaceId:_` and stomp each other. - useClearPaneAttentionOnView now re-runs when a new review status arrives for the viewed workspace (not just on mount), so Stop events arriving while on-page also clear the sidebar dot. - Split WorkspaceListener into its own file per AGENTS.md one-component rule. - Plan doc now references the actual endpoint `/trpc/notifications.hook`. Not addressed in this commit (held for discussion): - HOST_SERVICE_SECRET exposure via SUPERSET_HOST_AGENT_HOOK_TOKEN — needs a hook-scoped token design. - v1 fallback when the v2 POST returns non-2xx — design choice between silent fallback and surfacing v2 failures. - Autoplay priming listener stacking + keyboard-retry path.
notifications.hook only broadcasts a chime + sidebar dot — no state change, no data access, no code execution. Previously it reused the global HOST_SERVICE_SECRET as a bearer, which was both (a) exposed to every agent shell's env as SUPERSET_HOST_AGENT_HOOK_TOKEN and (b) redundant, since that same secret is already stored in the user- readable manifest.json alongside HOST_SERVICE_SECRET. - Route is now publicProcedure with a note explaining the rationale. - Removes SUPERSET_HOST_AGENT_HOOK_TOKEN from v2 PTY env entirely; the agent script only needs the URL. - Shell hook drops the Authorization header. If the endpoint ever grows capabilities beyond "fan out a chime", re-introduce auth with a hook-scoped secret — not the global PSK.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh (1)
83-101:⚠️ Potential issue | 🟠 Majorv1 fallback still unreachable when host-service POST fails.
The current code always runs
exit 0at line 100 wheneverSUPERSET_HOST_AGENT_HOOK_URLis set, regardless of whethercurlactually succeeded:
- Debug branch captures
STATUS_CODEonly for logging, never checks it.- Non-debug branch discards the result entirely.
If host-service is restarting, crashed, or otherwise unreachable, the script will silently drop the notification instead of falling through to the v1 electron
/hook/completefallback that the surrounding comments advertise. Given this is purely a user-facing chime/indicator path, silent drops during host-service blips degrade UX in exactly the scenarios the fallback was designed for.🛠️ Proposed fix — only exit on 2xx, else fall through to v1
- if [ "$DEBUG_HOOKS_ENABLED" = "1" ]; then - STATUS_CODE=$(curl -sX POST "$SUPERSET_HOST_AGENT_HOOK_URL" \ - --connect-timeout 1 --max-time 2 \ - -H "Content-Type: application/json" \ - -d "$PAYLOAD" \ - -o /dev/null -w "%{http_code}" 2>/dev/null) - echo "[notify-hook] host-service dispatched status=$STATUS_CODE" >&2 - else - curl -sX POST "$SUPERSET_HOST_AGENT_HOOK_URL" \ - --connect-timeout 1 --max-time 2 \ - -H "Content-Type: application/json" \ - -d "$PAYLOAD" \ - > /dev/null 2>&1 - fi - exit 0 + STATUS_CODE=$(curl -sX POST "$SUPERSET_HOST_AGENT_HOOK_URL" \ + --connect-timeout 1 --max-time 2 \ + -H "Content-Type: application/json" \ + -d "$PAYLOAD" \ + -o /dev/null -w "%{http_code}" 2>/dev/null) + if [ "$DEBUG_HOOKS_ENABLED" = "1" ]; then + echo "[notify-hook] host-service dispatched status=$STATUS_CODE" >&2 + fi + case "$STATUS_CODE" in + 2*) exit 0 ;; + esac + # Non-2xx / network failure → fall through to v1 electron fallback below. fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh` around lines 83 - 101, The host-hook branch currently always exits after attempting the POST to SUPERSET_HOST_AGENT_HOOK_URL (using PAYLOAD) even if the request failed; update the logic so that both the DEBUG_HOOKS_ENABLED and non-debug paths capture the HTTP status/exit result (e.g., via STATUS_CODE or curl exit) and only run exit 0 when the POST returned a successful 2xx response; if the status is non-2xx or curl failed, do not exit so execution falls through to the v1 electron /hook/complete fallback. Ensure you reference the existing variables SUPERSET_HOST_AGENT_HOOK_URL, DEBUG_HOOKS_ENABLED, PAYLOAD and STATUS_CODE and preserve the current logging behavior when DEBUG_HOOKS_ENABLED is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh`:
- Around line 83-101: The host-hook branch currently always exits after
attempting the POST to SUPERSET_HOST_AGENT_HOOK_URL (using PAYLOAD) even if the
request failed; update the logic so that both the DEBUG_HOOKS_ENABLED and
non-debug paths capture the HTTP status/exit result (e.g., via STATUS_CODE or
curl exit) and only run exit 0 when the POST returned a successful 2xx response;
if the status is non-2xx or curl failed, do not exit so execution falls through
to the v1 electron /hook/complete fallback. Ensure you reference the existing
variables SUPERSET_HOST_AGENT_HOOK_URL, DEBUG_HOOKS_ENABLED, PAYLOAD and
STATUS_CODE and preserve the current logging behavior when DEBUG_HOOKS_ENABLED
is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2528f281-791d-453c-90d9-7e51b754a732
📒 Files selected for processing (4)
apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.shpackages/host-service/src/terminal/env.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/router/notifications/notifications.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/host-service/src/trpc/router/notifications/notifications.ts
- Shell hook now captures the status code from the v2 POST (regardless of debug mode) and only exits if it was 2xx. Otherwise falls through to the electron v1 endpoint — covers host-service restarts, crashes, transient 5xxs without silently dropping the notification. - primeRingtoneAudioOnFirstGesture is now idempotent: if called twice before any gesture, listeners are only installed once. Audio is marked primed only after silent.play() resolves. On rejection the listeners are re-armed so the next gesture retries — with both pointerdown and keydown preserved (the retry path previously dropped keydown, breaking keyboard-only users).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/lib/ringtones/play.ts (2)
62-78: Optional: collapse the two-level fallback.The custom-id branch and the unknown-id branch both end in "resolve the DEFAULT_RINGTONE_ID URL", and the
&& builtInRingtoneUrls[ringtone.filename]guard is redundant with the?? nullon the following line. A single fallback keeps the resolver easier to reason about.♻️ Proposed simplification
function resolveRingtoneUrl(ringtoneId: string): string | null { - if (ringtoneId === CUSTOM_RINGTONE_ID) { - // Custom uploads aren't wired into renderer playback yet — fall back - // to the default so muted is the only way to get silence in v2. - return ( - builtInRingtoneUrls[ - getRingtoneById(DEFAULT_RINGTONE_ID)?.filename ?? "" - ] ?? null - ); - } - const ringtone = getRingtoneById(ringtoneId); - if (ringtone && builtInRingtoneUrls[ringtone.filename]) { - return builtInRingtoneUrls[ringtone.filename] ?? null; - } - const fallback = getRingtoneById(DEFAULT_RINGTONE_ID); - return fallback ? (builtInRingtoneUrls[fallback.filename] ?? null) : null; + // Custom uploads aren't wired into renderer playback yet — fall back to + // the default so muted is the only way to get silence in v2. + const ringtone = + ringtoneId === CUSTOM_RINGTONE_ID ? null : getRingtoneById(ringtoneId); + const resolved = ringtone ? builtInRingtoneUrls[ringtone.filename] : undefined; + if (resolved) return resolved; + const fallback = getRingtoneById(DEFAULT_RINGTONE_ID); + return fallback ? (builtInRingtoneUrls[fallback.filename] ?? null) : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/ringtones/play.ts` around lines 62 - 78, The resolveRingtoneUrl function duplicates fallback logic: both the CUSTOM_RINGTONE_ID branch and the unknown-id branch end up returning the DEFAULT_RINGTONE_ID URL, and the guard `&& builtInRingtoneUrls[ringtone.filename]` is redundant with the subsequent `?? null`; simplify by collapsing to a single resolution flow: check if ringtoneId === CUSTOM_RINGTONE_ID then treat as DEFAULT_RINGTONE_ID, otherwise fetch ringtone = getRingtoneById(ringtoneId); attempt to return builtInRingtoneUrls[ringtone?.filename] ?? builtInRingtoneUrls[getRingtoneById(DEFAULT_RINGTONE_ID)?.filename] ?? null; update resolveRingtoneUrl to use CUSTOM_RINGTONE_ID, DEFAULT_RINGTONE_ID, getRingtoneById, and builtInRingtoneUrls accordingly.
34-51: Minor: remove the partner listener eagerly and drop the no-op toggle.Two small cleanups in
prime:
- When one gesture fires (say
pointerdown),keydownremains armed because{ once: true }only removes the one that fired. If akeydownhappens whilesilent.play()is still pending,primeruns a second time concurrently. It's not user-visible here (success is idempotent), but it's easy to avoid by removing both listeners at the top ofprime.- The
audioPrimingListenersInstalled = false; … = true;pair on lines 46/49 is a synchronous no-op — the flag is never observed in between — and obscures intent.♻️ Proposed cleanup
const prime = () => { + removeListeners(); const silent = new Audio(); silent.muted = true; silent .play() .then(() => { audioPrimed = true; - removeListeners(); }) .catch(() => { // Browser refused even with a gesture — wait for the next one. - // Listeners stay active (once:true triggered, so re-attach). - audioPrimingListenersInstalled = false; window.addEventListener("pointerdown", prime, { once: true }); window.addEventListener("keydown", prime, { once: true }); - audioPrimingListenersInstalled = true; }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/ringtones/play.ts` around lines 34 - 51, The prime() function can run twice concurrently because only the fired listener is removed by { once:true }, so remove both priming listeners at the start of prime by calling removeListeners() (or explicitly remove pointerdown and keydown) before creating/playing the silent Audio to prevent re-entrancy; also drop the synchronous no-op toggling of audioPrimingListenersInstalled in the catch block (remove the audioPrimingListenersInstalled = false; … = true; pair) and only manage that flag where listeners are actually installed/removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.sh`:
- Around line 85-101: The v2 POST to SUPERSET_HOST_AGENT_HOOK_URL can time out
too quickly (curl --max-time 2) causing STATUS_CODE "000" and falling back to
the v1 path, producing duplicate notifications; update the curl invocation in
notify-hook.template.sh (the PAYLOAD/STATUS_CODE block that uses curl with
--connect-timeout and --max-time) to increase --max-time (e.g., from 2 to 5) and
optionally bump --connect-timeout to 2 so the host-service has more time to
respond; alternatively (or in a follow-up) add dedupe logic in the v1 handler
keyed by (paneId, sessionId, eventType) with a short TTL to avoid double-chimes.
---
Nitpick comments:
In `@apps/desktop/src/renderer/lib/ringtones/play.ts`:
- Around line 62-78: The resolveRingtoneUrl function duplicates fallback logic:
both the CUSTOM_RINGTONE_ID branch and the unknown-id branch end up returning
the DEFAULT_RINGTONE_ID URL, and the guard `&&
builtInRingtoneUrls[ringtone.filename]` is redundant with the subsequent `??
null`; simplify by collapsing to a single resolution flow: check if ringtoneId
=== CUSTOM_RINGTONE_ID then treat as DEFAULT_RINGTONE_ID, otherwise fetch
ringtone = getRingtoneById(ringtoneId); attempt to return
builtInRingtoneUrls[ringtone?.filename] ??
builtInRingtoneUrls[getRingtoneById(DEFAULT_RINGTONE_ID)?.filename] ?? null;
update resolveRingtoneUrl to use CUSTOM_RINGTONE_ID, DEFAULT_RINGTONE_ID,
getRingtoneById, and builtInRingtoneUrls accordingly.
- Around line 34-51: The prime() function can run twice concurrently because
only the fired listener is removed by { once:true }, so remove both priming
listeners at the start of prime by calling removeListeners() (or explicitly
remove pointerdown and keydown) before creating/playing the silent Audio to
prevent re-entrancy; also drop the synchronous no-op toggling of
audioPrimingListenersInstalled in the catch block (remove the
audioPrimingListenersInstalled = false; … = true; pair) and only manage that
flag where listeners are actually installed/removed.
🪄 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: 49050176-1791-4deb-bf95-bd09936a1e7d
📒 Files selected for processing (2)
apps/desktop/src/main/lib/agent-setup/templates/notify-hook.template.shapps/desktop/src/renderer/lib/ringtones/play.ts
828ca8c to
3206979
Compare
Intermediate commit capturing in-progress merge work before resolving remaining conflicts with main.
…-notification-hooks-on-the-client-side-instead-of-electron-side-because-its-ove # Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx # bun.lock # packages/host-service/src/trpc/router/workspace-creation/workspace-creation.ts
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
|
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 |
upstream の 10 commits は #426 と #427 で fork 固有の差分を保ちながら個別に cherry-pick 取り込み済み。本 merge は ours strategy で **記録だけ** マージ済みに することで behind=0 を達成し、git 履歴上の追跡を正しくする。 Cherry-pick 取り込み済 (PR #426): - 5aab22a fix closed picker filters (superset-sh#3702) → cdb52f9 - 99db5be [codex] simplify workspace controls (superset-sh#3714) → f079606 - 186078a fix(chat): prevent ask_user question from shadowing sandbox access prompt (superset-sh#3662) → 09d6b57 - 47893c2 fix desktop workspace creation title clamp (superset-sh#3718) → 6a8c4ae - 09323ff Add diff pane file viewer action (superset-sh#3715) → 817ed8d - a5891c6 remove pending launch log (superset-sh#3721) → 0764d03 - c83de0c Add Tiptap table support (superset-sh#3719) → e67a885 - 486b621 [codex] Fix v2 terminal lifecycle after sleep (superset-sh#3711) → b71fbbb (+ #426 内 review fixups) Cherry-pick 取り込み済 (PR #427): - e07aef6 feat(desktop): play v2 notification hooks client-side (superset-sh#3675) → 27ac18a - eae6008 [codex] Port v2 terminal hotkeys to v1 (superset-sh#3724) → 05a77b8 (+ #427 内 Windows .ps1 v2 化) Fork 固有領域は変更ゼロで保持: 19 tRPC procedures (workspaces.githubExtended)、 AudioScheduler / Aivis TTS / notification-manager、terminal suggestion handler (新 terminalKeyboardHandler.ts に移植)、TERMINAL_OPTIONS、SUPERSET_WORKSPACE_NAME、 MainWindowEffects、INCEPTION_AUTH_PROVIDER_ID、v1MigrationState、TiptapPromptEditor、 electron-builder.ts (dmg.size="4g", fileAssociations)、Service Status Dashboard、 Linux daemon systemd、Worktree auto-sync、Windows support、DnD scratch route 他。
Summary
terminalIdandeventType; host-service derivesworkspaceIdfrom its terminal session table before broadcasting.v2-notificationsstore keyed by typed notification source (terminal:<id>,chat:<id>). Workspace, tab, pane, terminal, and chat UI can resolve status through selector hooks instead of prop drilling or duplicating status state.terminal:<id>/chat:<id>) and include a per-click focus nonce so repeated notifications for the same source can refocus the pane.V2NotificationControllerwith per-hostHostNotificationSubscriberchildren so the naming matches the fanout role./hook/completefallback unchanged for legacy pane/tab/session payloads.Plan
See
plans/20260422-v2-notification-hooks-client-side.mdfor architecture, phases, and non-goals.Out of scope
Test plan
bun test apps/desktop/src/renderer/stores/v2-notifications/store.test.ts packages/host-service/src/trpc/router/notifications/notifications.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/renderer/routes/_authenticated/components/V2NotificationController/lib/resolveV2NotificationTarget.test.ts apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/statusTransitions.test.ts apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/hooks/useConsumeAutomationRunLink/useConsumeAutomationRunLink.test.tsbun run typecheckbun run lintgit diff --check