[codex] add context to v2 macOS notifications#4614
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds workspace display fields to host grouping and threads a derived workspaceName through the notification pipeline; introduces getV2NativeNotificationContent to produce native notification title/body and updates the lifecycle handler and tests to use it. ChangesV2 Notification Workspace Metadata and Content
Sequence Diagram(s)sequenceDiagram
participant HostSubscriber as HostNotificationSubscriber
participant Lifecycle as handleV2AgentLifecycleEvent
participant ContentGen as getV2NativeNotificationContent
participant NativeAPI as NativeNotificationShow
HostSubscriber->>Lifecycle: agent:lifecycle + workspaceId + workspaceName + payload
Lifecycle->>ContentGen: getV2NativeNotificationContent({ workspaceName, payload })
ContentGen-->>Lifecycle: { title, body }
Lifecycle->>NativeAPI: show native notification with title/body
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 4 individual chapters for you: 
Chapters generated by Stage for commit d203239 on May 16, 2026 12:53am UTC. |
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts (1)
112-113: 💤 Low valueRedundant
typeofcheck in tab index validation.The
indexOfmethod always returns a number, so thetypeof index === "number"check is redundant. Only theindex >= 0check is needed.♻️ Simplified version
const explicitTitle = cleanLabel(tab.titleOverride); if (explicitTitle) return explicitTitle; const index = paneLayout?.tabs.indexOf(tab); - return typeof index === "number" && index >= 0 ? `Tab ${index + 1}` : null; + return index !== undefined && index >= 0 ? `Tab ${index + 1}` : null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts` around lines 112 - 113, The check for typeof index is redundant because Array#indexOf always returns a number; update the conditional in the expression that computes the tab label (where paneLayout?.tabs.indexOf(tab) is assigned to index and used to return `Tab ${index + 1}`) to only test index >= 0 and drop the typeof check so the return becomes a simple index >= 0 ? `Tab ${index + 1}` : null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts`:
- Around line 112-113: The check for typeof index is redundant because
Array#indexOf always returns a number; update the conditional in the expression
that computes the tab label (where paneLayout?.tabs.indexOf(tab) is assigned to
index and used to return `Tab ${index + 1}`) to only test index >= 0 and drop
the typeof check so the return becomes a simple index >= 0 ? `Tab ${index + 1}`
: null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89a50a8c-1ef4-4973-aff2-bb9e9ef0ae7e
📒 Files selected for processing (5)
apps/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/notificationContent.test.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts
Greptile SummaryThis PR enriches native macOS notifications for v2 agent lifecycle events by adding contextual labels (agent, workspace, pane, tab) instead of the previous generic "Agent Complete / Your agent has finished" copy. The notification pipeline already carried agent identity; this change threads workspace display names and pane layout data through to a new
Confidence Score: 4/5Safe to merge; changes are isolated to the notification formatting path and do not touch agent execution, data persistence, or auth flows. The notification formatter is well-structured with graceful fallbacks at every label level and the unit tests cover the main scenarios. Two minor points worth a quick look: the PANE_KIND_LABELS index type hides an implicit runtime undefined from TypeScript, and shortId can produce 'Terminal terminal' for the rare case where the terminal ID is exactly the stripped prefix. Neither affects correctness for real workspaces. apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts — the PANE_KIND_LABELS typing and shortId fallback logic
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts | New file: formats notification title/body from agent, workspace, pane, and tab labels with clean fallback chains; PANE_KIND_LABELS typed as Record<string, string> makes the runtime undefined case invisible to TypeScript |
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.test.ts | New test file: covers completion, permission-request, and fallback label cases; test assertions match production logic |
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/lifecycleEvents.ts | Threads workspaceName and paneLayout into showNativeNotification; correctly uses paneId as the registry instanceId for a more specific getTitle lookup, then falls back to the primary entry |
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx | Adds name/branch columns to the workspace live-query and computes workspaceName (name → branch → id) before passing to subscriber groups; both columns are notNull in schema so .trim() is safe |
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/components/HostNotificationSubscriber/HostNotificationSubscriber.tsx | Straightforward interface extension: adds workspaceName to HostNotificationWorkspaceState and threads it through to handleV2AgentLifecycleEvent |
Sequence Diagram
sequenceDiagram
participant HS as HostNotificationSubscriber
participant LE as lifecycleEvents
participant RNT as resolveV2NotificationTarget
participant TRR as terminalRuntimeRegistry
participant NC as notificationContent
participant TRPC as electronTrpcClient
HS->>LE: handleV2AgentLifecycleEvent(workspaceId, workspaceName, payload, paneLayout)
LE->>RNT: resolveV2NotificationTarget(workspaceId, payload, paneLayout)
RNT-->>LE: "V2NotificationTarget {tabId, paneId, terminalId}"
LE->>LE: shouldSuppress(target, paneLayout)?
LE->>TRR: getTitle(terminalId, paneId)
TRR-->>LE: terminalTitle or undefined
Note over LE,TRR: Falls back to getTitle(terminalId) if first returns empty
LE->>NC: getV2NativeNotificationContent(workspaceName, payload, target, paneLayout, terminalTitle)
Note over NC: Resolves agentLabel, workspaceLabel, paneLabel, tabLabel
NC-->>LE: "{title, body}"
LE->>TRPC: "notifications.showNative({title, body, clickTarget})"
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/notificationContent.ts:26
The `PANE_KIND_LABELS` map is typed as `Record<string, string>`, so TypeScript considers every index access to always return `string`. At runtime, accessing a key that isn't in the map returns `undefined`, making the `??` fallback on line 100 necessary but invisible to the type checker. Narrowing the type to `Partial<Record<string, string>>` makes the index signature honest and lets TypeScript verify the nullish coalescing is needed.
```suggestion
const PANE_KIND_LABELS: Partial<Record<string, string>> = {
```
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts:132-136
When `terminalId` is a string whose only content is the "terminal" prefix itself (e.g. exactly `"terminal"` or `"terminal-"`), `replace` strips it to an empty string and `candidate` falls back to the original value. With `candidate = "terminal-"` (9 chars), `slice(0, 8)` returns `"terminal"`, so the body becomes `"Pane: Terminal terminal"` — a doubled word. A guard for the empty-suffix case avoids this.
```suggestion
function shortId(value: string): string {
const withoutTerminalPrefix = value.replace(/^terminal[-_:]?/i, "");
const candidate = withoutTerminalPrefix || value;
// If stripping the prefix left nothing meaningful, keep the original.
if (!withoutTerminalPrefix) return value.length > 8 ? value.slice(0, 8) : value;
return candidate.length > 8 ? candidate.slice(0, 8) : candidate;
}
```
Reviews (1): Last reviewed commit: "Adjust v2 notification copy" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pane?: Pane<PaneViewerData>; | ||
| } | ||
|
|
||
| const PANE_KIND_LABELS: Record<string, string> = { |
There was a problem hiding this comment.
PANE_KIND_LABELS map is typed as Record<string, string>, so TypeScript considers every index access to always return string. At runtime, accessing a key that isn't in the map returns undefined, making the ?? fallback on line 100 necessary but invisible to the type checker. Narrowing the type to Partial<Record<string, string>> makes the index signature honest and lets TypeScript verify the nullish coalescing is needed.
| const PANE_KIND_LABELS: Record<string, string> = { | |
| const PANE_KIND_LABELS: Partial<Record<string, string>> = { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts
Line: 26
Comment:
The `PANE_KIND_LABELS` map is typed as `Record<string, string>`, so TypeScript considers every index access to always return `string`. At runtime, accessing a key that isn't in the map returns `undefined`, making the `??` fallback on line 100 necessary but invisible to the type checker. Narrowing the type to `Partial<Record<string, string>>` makes the index signature honest and lets TypeScript verify the nullish coalescing is needed.
```suggestion
const PANE_KIND_LABELS: Partial<Record<string, string>> = {
```
How can I resolve this? If you propose a fix, please make it concise.| .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) | ||
| .join(" "); | ||
| } | ||
|
|
||
| function shortId(value: string): string { |
There was a problem hiding this comment.
terminalId is a string whose only content is the "terminal" prefix itself (e.g. exactly "terminal" or "terminal-"), replace strips it to an empty string and candidate falls back to the original value. With candidate = "terminal-" (9 chars), slice(0, 8) returns "terminal", so the body becomes "Pane: Terminal terminal" — a doubled word. A guard for the empty-suffix case avoids this.
| .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) | |
| .join(" "); | |
| } | |
| function shortId(value: string): string { | |
| function shortId(value: string): string { | |
| const withoutTerminalPrefix = value.replace(/^terminal[-_:]?/i, ""); | |
| const candidate = withoutTerminalPrefix || value; | |
| // If stripping the prefix left nothing meaningful, keep the original. | |
| if (!withoutTerminalPrefix) return value.length > 8 ? value.slice(0, 8) : value; | |
| return candidate.length > 8 ? candidate.slice(0, 8) : candidate; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts
Line: 132-136
Comment:
When `terminalId` is a string whose only content is the "terminal" prefix itself (e.g. exactly `"terminal"` or `"terminal-"`), `replace` strips it to an empty string and `candidate` falls back to the original value. With `candidate = "terminal-"` (9 chars), `slice(0, 8)` returns `"terminal"`, so the body becomes `"Pane: Terminal terminal"` — a doubled word. A guard for the empty-suffix case avoids this.
```suggestion
function shortId(value: string): string {
const withoutTerminalPrefix = value.replace(/^terminal[-_:]?/i, "");
const candidate = withoutTerminalPrefix || value;
// If stripping the prefix left nothing meaningful, keep the original.
if (!withoutTerminalPrefix) return value.length > 8 ? value.slice(0, 8) : value;
return candidate.length > 8 ? candidate.slice(0, 8) : candidate;
}
```
How can I resolve this? If you propose a fix, please make it concise.
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/lib/notificationContent.ts`:
- Around line 36-37: Replace the use of the `in` operator for own-property
checking with an explicit hasOwnProperty call: instead of `if (agentId in
BUILTIN_AGENT_LABELS)`, use
`Object.prototype.hasOwnProperty.call(BUILTIN_AGENT_LABELS, agentId)` and then
return `BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId]` as before; this change
should be applied where `agentId`, `BUILTIN_AGENT_LABELS`, and `BuiltinAgentId`
are referenced in notificationContent.ts to ensure only own properties are
matched.
🪄 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: 1e20c775-3b42-466b-a078-44aae468c601
📒 Files selected for processing (4)
apps/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/notificationContent.test.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts
| if (agentId in BUILTIN_AGENT_LABELS) { | ||
| return BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify BUILTIN_AGENT_LABELS declaration and whether it uses a null-prototype map.
rg -n -C3 "BUILTIN_AGENT_LABELS"
# Verify AgentIdentity/agentId typing or validation constraints upstream.
rg -n -C3 "interface AgentIdentity|type AgentIdentity|agentId"Repository: superset-sh/superset
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find cleanLabel function definition
rg -A 10 "function cleanLabel|const cleanLabel|export.*cleanLabel" apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.ts
# Show the full function context in notificationContent.ts
rg -B 5 -A 15 "function getAgentLabel" apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.tsRepository: superset-sh/superset
Length of output: 973
Use Object.prototype.hasOwnProperty.call() for own-property checks.
On line 36, the in operator checks both own properties and inherited prototype keys. While the practical risk is mitigated by type constraints on agentId (BuiltinAgentId | "droid"), using Object.prototype.hasOwnProperty.call() is the correct approach for own-property checks and avoids relying on type safety for this operation.
Proposed fix
- if (agentId in BUILTIN_AGENT_LABELS) {
+ if (Object.prototype.hasOwnProperty.call(BUILTIN_AGENT_LABELS, agentId)) {
return BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (agentId in BUILTIN_AGENT_LABELS) { | |
| return BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId]; | |
| if (Object.prototype.hasOwnProperty.call(BUILTIN_AGENT_LABELS, agentId)) { | |
| return BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId]; |
🤖 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/components/V2NotificationController/lib/notificationContent.ts`
around lines 36 - 37, Replace the use of the `in` operator for own-property
checking with an explicit hasOwnProperty call: instead of `if (agentId in
BUILTIN_AGENT_LABELS)`, use
`Object.prototype.hasOwnProperty.call(BUILTIN_AGENT_LABELS, agentId)` and then
return `BUILTIN_AGENT_LABELS[agentId as BuiltinAgentId]` as before; this change
should be applied where `agentId`, `BUILTIN_AGENT_LABELS`, and `BuiltinAgentId`
are referenced in notificationContent.ts to ensure only own properties are
matched.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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/components/V2NotificationController/V2NotificationController.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx:126">
P2: Using a constant fallback label (`"Workspace"`) removes unique workspace identification for unnamed/unbranched workspaces, making notifications ambiguous.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| group.push({ | ||
| workspaceId: workspace.workspaceId, | ||
| workspaceName: | ||
| workspace.name.trim() || workspace.branch.trim() || "Workspace", |
There was a problem hiding this comment.
P2: Using a constant fallback label ("Workspace") removes unique workspace identification for unnamed/unbranched workspaces, making notifications ambiguous.
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/components/V2NotificationController/V2NotificationController.tsx, line 126:
<comment>Using a constant fallback label (`"Workspace"`) removes unique workspace identification for unnamed/unbranched workspaces, making notifications ambiguous.</comment>
<file context>
@@ -123,9 +123,7 @@ function groupWorkspacesByHostUrl({
- workspace.name.trim() ||
- workspace.branch.trim() ||
- workspace.workspaceId,
+ workspace.name.trim() || workspace.branch.trim() || "Workspace",
paneLayout: paneLayoutsByWorkspaceId.get(workspace.workspaceId) ?? null,
});
</file context>
| workspace.name.trim() || workspace.branch.trim() || "Workspace", | |
| workspace.name.trim() || workspace.branch.trim() || workspace.workspaceId, |
Tip: Review your code locally with the cubic CLI to iterate faster.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Adds richer native macOS notification content for v2 workspace agent lifecycle events. Instead of generic
Agent Complete/Your agent has finishedcopy, notifications now include the agent label, workspace name, pane label, and tab label when available.Why
The v2 notification pipeline already receives agent identity and can resolve workspace/pane layout context, but the native notification copy only used generic text. That made it hard to tell which agent, pane, or workspace finished when multiple agents were running.
Changes
Validation
bun test apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/lib/notificationContent.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.tsbun run lintbun run --cwd apps/desktop typecheckSummary by cubic
Switches v2 macOS notifications to agent‑first titles and a simpler body. Titles are “{Agent} - Complete/Needs Attention”; the body shows only the workspace name.
name→branch→ “Workspace”).@superset/shared/agent-catalogor humanized ids; threadedworkspaceNamethrough and added unit tests.Written for commit d203239. Summary will update on new commits. Review in cubic
Summary by CodeRabbit