Conversation
…with terminals + ensure wrapper works
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces a local notifications subsystem: an Express webhook server, a tRPC notifications router with subscription streaming, agent CLI hook setup and wrappers, terminal environment propagation, renderer listeners to mark tabs needing attention, and UI badges for attention state. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as External Agent
participant NotifServer as Notifications Server (Express)
participant Main as Main Process (Emitter)
participant TRPC as tRPC Notifications
participant Renderer as Renderer (useAgentHookListener)
participant Store as Tabs Store
Agent->>NotifServer: GET /hook/complete
NotifServer->>Main: Emit "agent-complete" via notificationsEmitter
Main->>TRPC: Emit/bridge event for subscribers
TRPC->>Renderer: Notification subscription delivers "agent-complete"
alt tab inactive
Renderer->>Store: setNeedsAttention(tabId, true)
Store->>Renderer: UI shows attention badge
end
Main->>Main: On notification click -> emit "focus-tab"
TRPC->>Renderer: "focus-tab" delivered
Renderer->>Renderer: setActiveWorkspace -> setActiveTab(tabId)
Renderer->>Store: Clear needsAttention(tabId)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
6-17: AddtabTitleto the useEffect dependency array to prevent stale closure.The
restartTerminalfunction (line 95) capturestabTitleand uses it when restarting after process exit (line 103). If a tab is renamed while the terminal is running, the effect won't re-run sincetabTitleis missing from the dependency array, causing the old title to be used on restart.Update line 166:
- }, [tabId, workspaceId, setActiveTab]); + }, [tabId, workspaceId, tabTitle, setActiveTab]);
🧹 Nitpick comments (5)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
38-43: Optimize needsAttention computation with useMemo.The
needsAttentioncomputation runs on every render, filtering all tabs each time. With many tabs, this could impact performance. Consider memoizing this derived value to avoid unnecessary recalculations.Apply this diff to optimize the computation:
+import { useMemo } from "react"; + const tabs = useTabs(); // Derive if workspace needs attention from any of its tabs - const needsAttention = tabs - .filter((t) => t.workspaceId === id) - .some((t) => t.needsAttention); + const needsAttention = useMemo( + () => tabs + .filter((t) => t.workspaceId === id) + .some((t) => t.needsAttention), + [tabs, id] + );apps/desktop/src/main/windows/main.ts (1)
79-82: Improve cleanup robustness.The server cleanup could fail silently, and
removeAllListeners()is overly broad.Apply this diff to improve cleanup:
window.on("close", () => { - server.close(); - notificationsEmitter.removeAllListeners(); + server.close((err) => { + if (err) { + console.error("[notifications] Error closing server:", err); + } + }); + notificationsEmitter.removeAllListeners("agent-complete"); + notificationsEmitter.removeAllListeners("focus-tab"); });apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
7-21: Tab/workspace context propagation into terminals looks consistent.The new
tabTitleinput and derivedworkspaceNameare correctly threaded intoterminalManager.createOrAttach, and the DB lookup for cwd/workspaceName has sane fallbacks. The behavior ofcreateOrAttachshould remain backward-compatible while enriching the terminal env.One small suggestion: consider moving or referencing the “IMPORTANT: env vars” doc from here to where the env is actually assembled (in
TerminalManager) to avoid drift between comments and implementation over time.Also applies to: 27-41, 51-59
apps/desktop/src/main/lib/notifications/server.ts (1)
16-24: Tighten HTTP semantics and exposure for the hooks endpoint.Functionally this works, but two things are worth tightening:
/hook/completeperforms a side-effect (emittingagent-complete) viaGET. UsingPOSTfor this hook would better match HTTP semantics and leaves room for a JSON body if you ever extend the payload.Since this is a generic, unauthenticated CORS-
*endpoint, please ensure that wherevernotificationsApp.listenis called, it’s bound to127.0.0.1(or otherwise firewalled) so the port isn’t exposed to the local network unnecessarily.Also applies to: 27-44
apps/desktop/src/main/lib/agent-setup.ts (1)
54-96: Consider adding idempotency checks for wrapper creation.The wrappers are recreated on every app startup even if they haven't changed. Consider checking if the wrapper already exists and has the correct content before rewriting it.
This would improve startup performance and reduce unnecessary disk I/O:
function createClaudeWrapper(): void { const wrapperPath = path.join(BIN_DIR, "claude"); const realClaude = findRealBinary("claude"); const notifyPath = path.join(HOOKS_DIR, "notify.sh"); if (!realClaude) { console.log("[agent-setup] Claude not found, skipping wrapper"); return; } const script = `#!/bin/bash # Superset wrapper for Claude Code # Injects notification hook settings SUPERSET_CLAUDE_SETTINGS='{"hooks":{"Stop":[{"matcher":"","hooks":[{"type":"command","command":"${notifyPath}"}]}]}}' exec "${realClaude}" --settings "$SUPERSET_CLAUDE_SETTINGS" "$@" `; // Only write if content has changed if (fs.existsSync(wrapperPath)) { const existing = fs.readFileSync(wrapperPath, "utf-8"); if (existing === script) { console.log(`[agent-setup] Claude wrapper unchanged, skipping`); return; } } fs.writeFileSync(wrapperPath, script, { mode: 0o755 }); console.log(`[agent-setup] Created Claude wrapper -> ${realClaude}`); }Apply similar logic to
createCodexWrapperandcreateNotifyScript.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
apps/desktop/CLAUDE.md(1 hunks)apps/desktop/package.json(2 hunks)apps/desktop/src/lib/trpc/routers/index.ts(2 hunks)apps/desktop/src/lib/trpc/routers/notifications.ts(1 hunks)apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(3 hunks)apps/desktop/src/main/index.ts(2 hunks)apps/desktop/src/main/lib/agent-setup.ts(1 hunks)apps/desktop/src/main/lib/notifications/server.ts(1 hunks)apps/desktop/src/main/lib/terminal-manager.ts(3 hunks)apps/desktop/src/main/windows/main.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/index.tsx(2 hunks)apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts(2 hunks)apps/desktop/src/renderer/stores/tabs/hooks.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/index.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(2 hunks)apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts(1 hunks)apps/desktop/src/shared/constants.ts(1 hunks)package.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
apps/desktop/src/renderer/screens/main/index.tsx (1)
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
useAgentHookListener(9-41)
apps/desktop/src/main/index.ts (1)
apps/desktop/src/main/lib/agent-setup.ts (1)
setupAgentHooks(102-115)
apps/desktop/src/shared/constants.ts (1)
apps/desktop/src/main/lib/notifications/server.ts (1)
NOTIFICATIONS_PORT(57-57)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(35-149)
apps/desktop/src/renderer/stores/tabs/hooks.ts (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(35-149)
apps/desktop/src/main/lib/terminal-manager.ts (3)
apps/desktop/src/main/lib/agent-setup.ts (1)
getSupsersetPath(120-122)apps/desktop/src/main/lib/notifications/server.ts (1)
NOTIFICATIONS_PORT(57-57)apps/desktop/src/shared/constants.ts (1)
NOTIFICATIONS_PORT(11-11)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
apps/desktop/src/renderer/stores/tabs/hooks.ts (1)
useTabs(3-3)
apps/desktop/src/lib/trpc/routers/notifications.ts (2)
apps/desktop/src/main/lib/notifications/server.ts (2)
AgentCompleteEvent(5-10)notificationsEmitter(12-12)apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)
apps/desktop/src/main/windows/main.ts (2)
apps/desktop/src/main/lib/notifications/server.ts (4)
notificationsApp(56-56)NOTIFICATIONS_PORT(57-57)notificationsEmitter(12-12)AgentCompleteEvent(5-10)apps/desktop/src/shared/constants.ts (1)
NOTIFICATIONS_PORT(11-11)
apps/desktop/src/lib/trpc/routers/index.ts (1)
apps/desktop/src/lib/trpc/routers/notifications.ts (1)
createNotificationsRouter(12-37)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
apps/desktop/src/renderer/stores/tabs/hooks.ts (1)
useTabs(3-3)
🔇 Additional comments (18)
apps/desktop/CLAUDE.md (1)
96-96: LGTM!The code block formatting has been corrected. This is a documentation-only change with no functional impact.
package.json (1)
4-46: LGTM!These fields are standard monorepo configuration. The repository metadata, devDependencies, packageManager specification, type module declaration, and workspace definitions are appropriate for this setup.
apps/desktop/src/renderer/stores/tabs/index.ts (1)
5-5: LGTM!The new export follows the existing pattern and properly exposes the useAgentHookListener hook.
apps/desktop/src/renderer/stores/tabs/hooks.ts (1)
23-24: LGTM!The hook follows the established pattern and properly exposes the setNeedsAttention action from the store.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
99-104: LGTM!The attention indicator implementation is clean and follows the same pattern used in TabItem. The visual feedback (animated ping + solid indicator) effectively draws attention to workspaces that need it.
apps/desktop/package.json (1)
39-50: I'll search the codebase to verify if the notification server implementation uses any Express patterns that might break with v5.Based on my verification of Express 5.1.0:
Express 5.1.0 is stable, secure, and recommended for production use.
Express 5 is tagged as "latest" on npm, and the latest version 5.1.0 is confirmed as non-vulnerable by Snyk. CVE-2024-45590 has been patched in body-parser version 1.20.3, which is included in Express 5.1.0.
Regarding breaking changes: Express 5 maintains the same basic API as Express 4, though breaking changes exist that could affect applications built with Express 4. The primary breaking changes involve deprecated method signatures removed since v4.11.0, route matching updates via path-to-regexp 8.x (which improves security and mitigates ReDoS vulnerabilities), and automatic promise rejection handling in async middleware.
The app should run your test suite after the upgrade to catch any incompatibilities with the notification server implementation, as the review suggests. These are standard migration checks rather than red flags.
apps/desktop/src/renderer/screens/main/index.tsx (1)
6-6: LGTM! Clean integration of the notification listener.The hook is properly placed and will establish the subscription when the main screen mounts.
Also applies to: 22-23
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
152-160: LGTM! Attention indicator is well-implemented.The pinging animation provides clear visual feedback, and the title attribute aids accessibility.
apps/desktop/src/renderer/stores/tabs/store.ts (1)
133-140: LGTM! The implementation correctly updates the tab state.The action maps over the tabs array to update the matching tab while preserving immutability.
apps/desktop/src/renderer/stores/tabs/types.ts (2)
14-14: LGTM! Type definition is appropriate.The optional
needsAttentionflag is well-suited for tracking attention state.
65-65: LGTM! Method signature is clear and consistent.The
setNeedsAttentionmethod signature aligns with the store implementation.apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts (1)
19-24: LGTM! Clearing attention on activation is the correct behavior.When a tab becomes active, its attention indicator should be dismissed. The implementation correctly updates the tabs array and includes it in the returned state.
Also applies to: 35-35
apps/desktop/src/lib/trpc/routers/index.ts (1)
3-3: LGTM! Router integration follows the established pattern.The notifications router is properly wired into the main AppRouter.
Also applies to: 19-19
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
27-37: LGTM! Focus-tab handling uses proper async workflow.The workspace switch is performed first with a mutation, and tab activation occurs in the success callback. This ensures proper sequencing.
apps/desktop/src/lib/trpc/routers/notifications.ts (1)
1-36: Notifications subscription wiring looks solid.The
NotificationEventunion, dual listeners onnotificationsEmitter, and cleanup viaoffon unsubscribe are all consistent and should behave well with TRPC subscriptions. I don’t see issues in this router as written.apps/desktop/src/main/lib/agent-setup.ts (3)
1-8: LGTM: Imports and directory structure are well-defined.The Node.js built-in imports are appropriate, and the directory structure under
~/.supersetis cleanly organized.
78-96: LGTM: Codex wrapper correctly uses resolved paths.Unlike the Claude wrapper, this correctly uses
path.join()to construct the notify script path and references it via a variable in the generated script.
127-129: LGTM: Simple getter function.The function correctly returns the bin directory path.
| function createNotifyScript(): void { | ||
| const notifyPath = path.join(HOOKS_DIR, "notify.sh"); | ||
| const script = `#!/bin/bash | ||
| # Superset agent notification hook | ||
| # Called by CLI agents (Claude Code, Codex, etc.) when they complete | ||
|
|
||
| # Only run if inside a Superset terminal | ||
| [ -z "$SUPERSET_TAB_ID" ] && exit 0 | ||
|
|
||
| curl -sG "http://127.0.0.1:\${SUPERSET_PORT:-31415}/hook/complete" \\ | ||
| --data-urlencode "tabId=$SUPERSET_TAB_ID" \\ | ||
| --data-urlencode "tabTitle=$SUPERSET_TAB_TITLE" \\ | ||
| --data-urlencode "workspaceName=$SUPERSET_WORKSPACE_NAME" \\ | ||
| --data-urlencode "workspaceId=$SUPERSET_WORKSPACE_ID" \\ | ||
| > /dev/null 2>&1 | ||
| `; | ||
| fs.writeFileSync(notifyPath, script, { mode: 0o755 }); | ||
| } |
There was a problem hiding this comment.
Use NOTIFICATIONS_PORT constant for consistency.
The port is hardcoded as 31415 in the notify script, but the AI summary indicates that NOTIFICATIONS_PORT is defined in apps/desktop/src/shared/constants.ts. This creates a potential mismatch if the port constant differs from the hardcoded value.
Consider passing the NOTIFICATIONS_PORT value into the script generation:
-function createNotifyScript(): void {
+function createNotifyScript(port: number): void {
const notifyPath = path.join(HOOKS_DIR, "notify.sh");
const script = `#!/bin/bash
# Superset agent notification hook
# Called by CLI agents (Claude Code, Codex, etc.) when they complete
# Only run if inside a Superset terminal
[ -z "$SUPERSET_TAB_ID" ] && exit 0
-curl -sG "http://127.0.0.1:\${SUPERSET_PORT:-31415}/hook/complete" \\
+curl -sG "http://127.0.0.1:\${SUPERSET_PORT:-${port}}/hook/complete" \\
--data-urlencode "tabId=$SUPERSET_TAB_ID" \\
--data-urlencode "tabTitle=$SUPERSET_TAB_TITLE" \\
--data-urlencode "workspaceName=$SUPERSET_WORKSPACE_NAME" \\
--data-urlencode "workspaceId=$SUPERSET_WORKSPACE_ID" \\
> /dev/null 2>&1
`;
fs.writeFileSync(notifyPath, script, { mode: 0o755 });
}Then import and pass NOTIFICATIONS_PORT when calling this function.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/agent-setup.ts around lines 32 to 49 the notify
script hardcodes port 31415; update to use the NOTIFICATIONS_PORT constant
instead by importing NOTIFICATIONS_PORT from
apps/desktop/src/shared/constants.ts, change createNotifyScript to accept a port
parameter (or read the imported constant inside the function), interpolate that
port into the curl URL (replacing 31415), and update all call sites to pass the
constant so the script and constant remain consistent.
| function createClaudeWrapper(): void { | ||
| const wrapperPath = path.join(BIN_DIR, "claude"); | ||
| const realClaude = findRealBinary("claude"); | ||
|
|
||
| if (!realClaude) { | ||
| console.log("[agent-setup] Claude not found, skipping wrapper"); | ||
| return; | ||
| } | ||
|
|
||
| const script = `#!/bin/bash | ||
| # Superset wrapper for Claude Code | ||
| # Injects notification hook settings | ||
|
|
||
| SUPERSET_CLAUDE_SETTINGS='{"hooks":{"Stop":[{"matcher":"","hooks":[{"type":"command","command":"~/.superset/hooks/notify.sh"}]}]}}' | ||
|
|
||
| exec "${realClaude}" --settings "$SUPERSET_CLAUDE_SETTINGS" "$@" | ||
| `; | ||
| fs.writeFileSync(wrapperPath, script, { mode: 0o755 }); | ||
| console.log(`[agent-setup] Created Claude wrapper -> ${realClaude}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use proper path construction instead of hardcoding ~/.superset.
The hook path is hardcoded as ~/.superset/hooks/notify.sh in the generated script, which may not work if the home directory path contains special characters or in edge cases where ~ doesn't expand properly.
Apply this diff to use the actual resolved path:
function createClaudeWrapper(): void {
const wrapperPath = path.join(BIN_DIR, "claude");
const realClaude = findRealBinary("claude");
+ const notifyPath = path.join(HOOKS_DIR, "notify.sh");
if (!realClaude) {
console.log("[agent-setup] Claude not found, skipping wrapper");
return;
}
const script = `#!/bin/bash
# Superset wrapper for Claude Code
# Injects notification hook settings
-SUPERSET_CLAUDE_SETTINGS='{"hooks":{"Stop":[{"matcher":"","hooks":[{"type":"command","command":"~/.superset/hooks/notify.sh"}]}]}}'
+SUPERSET_CLAUDE_SETTINGS='{"hooks":{"Stop":[{"matcher":"","hooks":[{"type":"command","command":"${notifyPath}"}]}]}}'
exec "${realClaude}" --settings "$SUPERSET_CLAUDE_SETTINGS" "$@"
`;
fs.writeFileSync(wrapperPath, script, { mode: 0o755 });
console.log(`[agent-setup] Created Claude wrapper -> ${realClaude}`);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/agent-setup.ts around lines 54 to 73, the generated
Claude wrapper hardcodes the hook as "~/.superset/hooks/notify.sh" which can
fail because "~" may not expand; resolve the actual user home and build the full
hook path instead (e.g. use Node's os.homedir() or process.env.HOME and
path.join to construct the superset hooks path), inject that resolved, properly
escaped/quoted absolute path into the script string, and write the wrapper with
the computed path so the wrapper calls the real notify.sh reliably across
environments.
| export function setupAgentHooks(): void { | ||
| console.log("[agent-setup] Initializing agent hooks..."); | ||
|
|
||
| // Create directories | ||
| fs.mkdirSync(BIN_DIR, { recursive: true }); | ||
| fs.mkdirSync(HOOKS_DIR, { recursive: true }); | ||
|
|
||
| // Create scripts | ||
| createNotifyScript(); | ||
| createClaudeWrapper(); | ||
| createCodexWrapper(); | ||
|
|
||
| console.log("[agent-setup] Agent hooks initialized"); | ||
| } |
There was a problem hiding this comment.
Add error handling for filesystem operations.
The function performs multiple filesystem operations without error handling. If directory creation or script writing fails (e.g., due to permissions), the app will crash on startup.
Apply this diff to add basic error handling:
export function setupAgentHooks(): void {
console.log("[agent-setup] Initializing agent hooks...");
- // Create directories
- fs.mkdirSync(BIN_DIR, { recursive: true });
- fs.mkdirSync(HOOKS_DIR, { recursive: true });
-
- // Create scripts
- createNotifyScript();
- createClaudeWrapper();
- createCodexWrapper();
-
- console.log("[agent-setup] Agent hooks initialized");
+ try {
+ // Create directories
+ fs.mkdirSync(BIN_DIR, { recursive: true });
+ fs.mkdirSync(HOOKS_DIR, { recursive: true });
+
+ // Create scripts
+ createNotifyScript();
+ createClaudeWrapper();
+ createCodexWrapper();
+
+ console.log("[agent-setup] Agent hooks initialized");
+ } catch (error) {
+ console.error("[agent-setup] Failed to initialize agent hooks:", error);
+ // Optionally: decide if this should be a fatal error or just log and continue
+ }
}🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/agent-setup.ts around lines 102 to 115, the
filesystem operations (fs.mkdirSync and the create* script calls) lack error
handling and can crash the app on permission or I/O failures; wrap the directory
creation and script-creation calls in a try/catch (either per-operation or a
single block) and on error log a clear message with the caught error
(console.error or a logger) and terminate gracefully (process.exit(1)) or
rethrow to surface the failure; ensure any synchronous operations remain
synchronous inside the try/catch and preserve the existing success log only when
no error occurred.
| // Start notifications HTTP server | ||
| const server = notificationsApp.listen(NOTIFICATIONS_PORT, "127.0.0.1", () => { | ||
| console.log(`[notifications] Listening on http://127.0.0.1:${NOTIFICATIONS_PORT}`); | ||
| }); |
There was a problem hiding this comment.
Add error handling for the notifications server.
The server starts without error handling, which could cause unhandled exceptions if the port is already in use or if there are other network issues.
Apply this diff to add error handling:
// Start notifications HTTP server
- const server = notificationsApp.listen(NOTIFICATIONS_PORT, "127.0.0.1", () => {
- console.log(`[notifications] Listening on http://127.0.0.1:${NOTIFICATIONS_PORT}`);
- });
+ const server = notificationsApp.listen(NOTIFICATIONS_PORT, "127.0.0.1", () => {
+ console.log(`[notifications] Listening on http://127.0.0.1:${NOTIFICATIONS_PORT}`);
+ });
+
+ server.on("error", (error) => {
+ console.error("[notifications] Server error:", error);
+ });📝 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.
| // Start notifications HTTP server | |
| const server = notificationsApp.listen(NOTIFICATIONS_PORT, "127.0.0.1", () => { | |
| console.log(`[notifications] Listening on http://127.0.0.1:${NOTIFICATIONS_PORT}`); | |
| }); | |
| // Start notifications HTTP server | |
| const server = notificationsApp.listen(NOTIFICATIONS_PORT, "127.0.0.1", () => { | |
| console.log(`[notifications] Listening on http://127.0.0.1:${NOTIFICATIONS_PORT}`); | |
| }); | |
| server.on("error", (error) => { | |
| console.error("[notifications] Server error:", error); | |
| }); |
🤖 Prompt for AI Agents
In apps/desktop/src/main/windows/main.ts around lines 47 to 50, the
notifications HTTP server is started without any error handling; add a server
'error' event listener after calling notificationsApp.listen to catch startup
errors (e.g., EADDRINUSE) and log them with context, then exit or retry as
appropriate; specifically, attach server.on('error', (err) => { handle known
codes like EADDRINUSE with a clear console.error message and process.exit(1) } )
and ensure the existing listen callback remains for successful start logging.
| notificationsEmitter.on("agent-complete", (event: AgentCompleteEvent) => { | ||
| if (Notification.isSupported()) { | ||
| const notification = new Notification({ | ||
| title: `Agent Complete — ${event.workspaceName}`, | ||
| body: `"${event.tabTitle}" has finished its task`, | ||
| silent: false, | ||
| }); | ||
|
|
||
| notification.on("click", () => { | ||
| window.show(); | ||
| window.focus(); | ||
| // Request focus on the specific tab | ||
| notificationsEmitter.emit("focus-tab", { | ||
| tabId: event.tabId, | ||
| workspaceId: event.workspaceId, | ||
| }); | ||
| }); | ||
|
|
||
| notification.show(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Verify window validity before interaction and add error handling.
The notification click handler interacts with the window without verifying it's still valid. Additionally, notification creation could throw exceptions.
Apply this diff to add safety checks:
// Handle agent completion notifications
notificationsEmitter.on("agent-complete", (event: AgentCompleteEvent) => {
+ if (window.isDestroyed()) return;
+
if (Notification.isSupported()) {
+ try {
const notification = new Notification({
title: `Agent Complete — ${event.workspaceName}`,
body: `"${event.tabTitle}" has finished its task`,
silent: false,
});
notification.on("click", () => {
+ if (window.isDestroyed()) return;
window.show();
window.focus();
// Request focus on the specific tab
notificationsEmitter.emit("focus-tab", {
tabId: event.tabId,
workspaceId: event.workspaceId,
});
});
notification.show();
+ } catch (error) {
+ console.error("[notifications] Failed to show notification:", error);
+ }
}
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/main/windows/main.ts around lines 53 to 73, the notification
click handler currently interacts with the BrowserWindow without checking that
it still exists or is not destroyed and the Notification constructor can throw;
wrap the Notification creation and show in a try/catch to handle/ log creation
errors, and inside the "click" handler first verify the window reference is not
null/undefined and !window.isDestroyed() (optionally check !window.isDestroyed()
&& window.webContents) before calling show(), focus(), or emitting "focus-tab";
if the window is invalid, skip those calls and log or noop to avoid exceptions.
| export function useAgentHookListener() { | ||
| const setActiveWorkspace = useSetActiveWorkspace(); | ||
| const { data: activeWorkspace } = trpc.workspaces.getActive.useQuery(); | ||
|
|
||
| trpc.notifications.subscribe.useSubscription(undefined, { | ||
| onData: (event) => { | ||
| if (event.type === "agent-complete") { | ||
| const { tabId, workspaceId } = event.data; | ||
| const state = useTabsStore.getState(); | ||
|
|
||
| // Only show red dot if not already viewing this tab | ||
| const isAlreadyActive = | ||
| activeWorkspace?.id === workspaceId && | ||
| state.activeTabIds[workspaceId] === tabId; | ||
|
|
||
| if (!isAlreadyActive) { | ||
| state.setNeedsAttention(tabId, true); | ||
| } |
There was a problem hiding this comment.
Potential race condition with workspace state.
The activeWorkspace data comes from a React Query result, which may be stale when the subscription event fires. This could cause the attention indicator to show or hide incorrectly.
Consider reading the active workspace directly from a store or ensuring the query data is fresh:
export function useAgentHookListener() {
const setActiveWorkspace = useSetActiveWorkspace();
- const { data: activeWorkspace } = trpc.workspaces.getActive.useQuery();
trpc.notifications.subscribe.useSubscription(undefined, {
onData: (event) => {
if (event.type === "agent-complete") {
const { tabId, workspaceId } = event.data;
const state = useTabsStore.getState();
- // Only show red dot if not already viewing this tab
- const isAlreadyActive =
- activeWorkspace?.id === workspaceId &&
- state.activeTabIds[workspaceId] === tabId;
+ // Get fresh workspace state at event time
+ // TODO: If activeWorkspace is stored elsewhere, retrieve it here
+ // For now, check only tab activeness within the workspace
+ const isAlreadyActive = state.activeTabIds[workspaceId] === tabId;
if (!isAlreadyActive) {
state.setNeedsAttention(tabId, true);
}
}Note: This assumes you want to show attention for inactive tabs regardless of workspace. If workspace check is needed, consider storing active workspace ID in a Zustand store for synchronous access.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts around lines
9-26, the subscription handler uses React Query's activeWorkspace (which can be
stale) to decide whether to mark a tab as needing attention; replace that
reactive query access with a synchronous read of the workspace ID from a local
store (e.g. call the zustand store getter like
useWorkspacesStore.getState().activeWorkspaceId) so the comparison with
workspaceId is always current; if you intend to ignore workspace context
entirely, remove the workspace check and always mark non-active tabs, otherwise
ensure you import/use the correct workspace store getter and use it
synchronously in the onData handler before calling state.setNeedsAttention.
| IS_LINUX: process.platform === "linux", | ||
| }; | ||
|
|
||
| export const NOTIFICATIONS_PORT = 31415; |
There was a problem hiding this comment.
🧩 Analysis chain
Consider port collision handling.
Port 31415 is in the registered port range. While unlikely to conflict, the notification server should gracefully handle the case where this port is already in use by another service on the user's machine. This is particularly important for a desktop application that users might run alongside other development tools.
Verify that the notification server implementation (in apps/desktop/src/main/lib/notifications/server.ts) includes error handling for EADDRINUSE errors when binding to this port.
🏁 Script executed:
#!/bin/bash
# Description: Check if notification server handles port conflicts
# Look for error handling in the notification server
ast-grep --pattern $'app.listen($$$, () => {
$$$
})'
# Also check for error event handlers
rg -A 5 "EADDRINUSE|address already in use" apps/desktop/src/main/lib/notifications/Length of output: 0
I need to broaden the verification approach, as the initial script may have had pattern matching issues. Let me search more comprehensively for the notification server and its error handling.
🏁 Script executed:
#!/bin/bash
# Step 1: Verify the notification server file exists
echo "=== Checking if notification server file exists ==="
fd -e ts notification || echo "File not found with fd"
find apps/desktop -name "*notification*" -o -name "*server*" 2>/dev/null | head -20
echo ""
echo "=== Searching for port binding patterns ==="
# Search for listen, bind, or server creation with broader patterns
rg -n "listen|bind|createServer" apps/desktop/src/main --type=ts -C 3
echo ""
echo "=== Searching for error handling in notifications directory ==="
rg -n "error|catch|Error" apps/desktop/src/main/lib/notifications --type=ts -C 2Length of output: 3228
🏁 Script executed:
#!/bin/bash
# Step 1: Verify the notification server file exists and explore structure
echo "=== Finding notification-related files ==="
fd "notification" apps/desktop/src/main --type f
echo ""
echo "=== Check if server.ts exists at expected path ==="
ls -la apps/desktop/src/main/lib/notifications/server.ts 2>/dev/null || echo "File not found at expected path"
echo ""
echo "=== Broader search for port 31415 or NOTIFICATIONS_PORT usage ==="
rg "31415|NOTIFICATIONS_PORT" apps/desktop/src --type=ts -B 2 -A 5
echo ""
echo "=== Search for error handlers on server/listener objects ==="
rg -n "\.on\s*\(\s*['\"]error['\"]|\.on\s*\(\s*['\"]EADDRINUSE" apps/desktop/src/main --type=ts -C 3Length of output: 4450
I need to examine the exact code where the notification server is started to confirm the absence of error handling.
🏁 Script executed:
#!/bin/bash
# Get exact code context around the server.listen call in main.ts
echo "=== Exact code in main.ts around notification server binding ==="
sed -n '45,60p' apps/desktop/src/main/windows/main.ts
echo ""
echo "=== Check if there's error handling AFTER the listen call ==="
sed -n '48,80p' apps/desktop/src/main/windows/main.tsLength of output: 1579
Add error handling for port binding in notification server initialization.
The notification server at apps/desktop/src/main/windows/main.ts (line 48) does not handle EADDRINUSE errors when binding to port 31415. If this port is already in use, the application startup will fail without graceful error handling. Add an error event listener to the server object, similar to the pattern used in apps/desktop/src/main/lib/port-manager.ts:
const server = notificationsApp.listen(NOTIFICATIONS_PORT, "127.0.0.1", () => {
console.log(`[notifications] Listening on http://127.0.0.1:${NOTIFICATIONS_PORT}`);
});
server.on("error", (err: NodeJS.ErrnoException) => {
if (err.code === "EADDRINUSE") {
console.error(`[notifications] Port ${NOTIFICATIONS_PORT} is already in use`);
// Handle gracefully: retry with different port, notify user, etc.
}
});🤖 Prompt for AI Agents
In apps/desktop/src/shared/constants.ts around line 11, the notification port
constant is declared but the notification server initialization in
apps/desktop/src/main/windows/main.ts (line ~48) lacks EADDRINUSE handling;
update the server creation there to capture the returned server from
notificationsApp.listen and attach an "error" listener that checks err.code ===
"EADDRINUSE", logs a clear error mentioning NOTIFICATIONS_PORT, and implements a
graceful fallback (e.g., pick a different free port or surface a user-friendly
error/notification), following the same pattern used in
apps/desktop/src/main/lib/port-manager.ts.
2 participants
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.