fix(desktop): fix notification click not opening workspace/tab#354
fix(desktop): fix notification click not opening workspace/tab#354
Conversation
- Extract NotificationIds interface for shared type across notification system - Add resolveNotificationTarget utility to resolve missing IDs from state - Refactor useAgentHookListener to use shared resolution logic - Handle cases where paneId or tabId is missing by looking up from state - Add unit tests for notification target resolution 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. WalkthroughIntroduce a shared NotificationIds type and propagate it through server types, TRPC router events, renderer notification handling, and add a resolver utility with tests to normalize pane/tab/workspace IDs before acting on notifications. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/notifications/server.ts (1)
29-41: Consider validating query parameters.The query parameters are cast directly to
string | undefinedwithout validation. If a client sends an array (e.g.,?paneId=a&paneId=b),req.query.paneIdwould be an array, and the cast would be incorrect at runtime.app.get("/hook/complete", (req, res) => { - const { paneId, tabId, workspaceId, eventType } = req.query; + const paneId = typeof req.query.paneId === "string" ? req.query.paneId : undefined; + const tabId = typeof req.query.tabId === "string" ? req.query.tabId : undefined; + const workspaceId = typeof req.query.workspaceId === "string" ? req.query.workspaceId : undefined; + const eventType = typeof req.query.eventType === "string" ? req.query.eventType : undefined; const event: AgentCompleteEvent = { - paneId: paneId as string | undefined, - tabId: tabId as string | undefined, - workspaceId: workspaceId as string | undefined, + paneId, + tabId, + workspaceId, eventType: eventType === "PermissionRequest" ? "PermissionRequest" : "Stop", };apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.ts (1)
13-16: JSDoc priority description is inconsistent with actual tabId resolution.The JSDoc states "Priority: event data > pane's tab" but for
tabId, line 27 usespane?.tabId ?? tabId, meaning pane's tabId takes precedence over event tabId. This is likely intentional (trusting pane's relationship), but the documentation is misleading./** * Resolves notification target IDs by looking up missing values from state. - * Priority: event data > pane's tab > tab's workspace + * Priority: pane's tabId > event tabId; event workspaceId > tab's workspaceId */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/notifications.ts(1 hunks)apps/desktop/src/main/lib/notifications/server.ts(1 hunks)apps/desktop/src/main/windows/main.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts(3 hunks)apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/main/windows/main.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.tsapps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.tsapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/lib/trpc/routers/notifications.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.tsapps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.tsapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/lib/trpc/routers/notifications.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.tsapps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.tsapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/lib/trpc/routers/notifications.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.tsapps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.tsapps/desktop/src/renderer/stores/tabs/useAgentHookListener.tsapps/desktop/src/lib/trpc/routers/notifications.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.tsapps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.tsapps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
apps/desktop/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.test.{ts,tsx,js,jsx}: Tests should have one assert per test
Tests should be readable
Tests should be fast
Tests should be independent
Tests should be repeatable
Files:
apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.ts
🧠 Learnings (3)
📚 Learning: 2025-12-12T05:45:09.673Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.673Z
Learning: Applies to apps/desktop/src/main/lib/*.ts : Implement IPC handlers accepting object parameters (not positional parameters) in apps/desktop/src/main/lib/*.ts files
Applied to files:
apps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/lib/trpc/routers/notifications.ts
📚 Learning: 2025-12-12T05:45:09.673Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T05:45:09.673Z
Learning: Applies to apps/desktop/src/shared/ipc-channels.ts : Define IPC channel types in apps/desktop/src/shared/ipc-channels.ts before implementing handlers
Applied to files:
apps/desktop/src/main/lib/notifications/server.ts
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.test.{ts,tsx,js,jsx} : Tests should be independent
Applied to files:
apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.ts
🧬 Code graph analysis (3)
apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.ts (2)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
TabsState(19-21)apps/desktop/src/main/lib/notifications/server.ts (1)
NotificationIds(4-8)
apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.ts (2)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createPane(46-62)apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.ts (1)
resolveNotificationTarget(17-44)
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (2)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(72-615)apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.ts (1)
resolveNotificationTarget(17-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy Web
- GitHub Check: Deploy API
- GitHub Check: Deploy Admin
- GitHub Check: Build
🔇 Additional comments (7)
apps/desktop/src/main/lib/notifications/server.ts (1)
4-11: Clean interface extraction for shared notification types.The
NotificationIdsinterface and theextendspattern forAgentCompleteEventprovide good type reusability across the notification system. This aligns well with the PR objective of centralizing notification-related types.apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.test.ts (1)
1-169: Well-structured and comprehensive test suite.The tests cover all key resolution scenarios with clear, readable structure. Each test follows the one-assert-per-test guideline and tests are independent. The helper functions are appropriately minimal for the test context.
apps/desktop/src/renderer/stores/tabs/utils/resolve-notification-target.ts (1)
17-43: Clean resolution logic with appropriate null safety.The function handles missing IDs gracefully and returns
nullwhen a valid target cannot be determined. The use of optional chaining and nullish coalescing is idiomatic TypeScript.apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (2)
15-19: Good refactor to centralized resolution logic.The early resolution and null-check pattern cleanly handles cases where notification data is incomplete. This simplifies the downstream logic for both event types.
46-63: Re-resolving from fresh state after workspace switch is correct.The pattern of re-fetching state after the async
setActiveWorkspace.mutatecompletes ensures the code operates on current data, avoiding potential stale state issues. The additional validation checks (tab existence, workspace ownership, pane existence) provide good defensive coding.apps/desktop/src/lib/trpc/routers/notifications.ts (2)
9-11: Type union updated to use shared NotificationIds.The
NotificationEventtype now consistently uses the shared interfaces from the notifications server module, improving type alignment across the notification system.
17-23: Handler signatures simplified with consistent naming.Renaming the parameter to
dataand passing it directly to the emit improves clarity and reduces redundancy. The cleanup function properly removes both listeners.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
42-72: Consider adding error handling for workspace switch failures.The re-resolution logic after workspace switch is well-designed and handles state changes during async operations correctly. The defensive checks for tab/pane existence prevent errors gracefully.
However, there's no
onErrorhandler for thesetActiveWorkspace.mutatecall. If workspace switching fails, the notification click silently does nothing. Consider adding anonErrorcallback for better observability, even if it only logs the error:setActiveWorkspace.mutate( { id: workspaceId }, { + onError: (error) => { + console.error("Failed to switch workspace from notification:", error); + }, onSuccess: () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for code formatting and linting, running at root level for speed
Files:
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid
anytype and prioritize type safety in TypeScript code
Files:
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Call IPC methods from renderer process using window.ipcRenderer.invoke with type-safe object parameters
Files:
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Deploy Web
- GitHub Check: Deploy Admin
- GitHub Check: Deploy Marketing
- GitHub Check: Deploy Docs
- GitHub Check: Deploy API
- GitHub Check: Build
🔇 Additional comments (3)
apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (3)
5-5: LGTM!The import follows the project's path conventions and the utility is correctly used throughout the file.
23-34: LGTM!The agent-complete handler correctly guards against missing
paneIdand only sets the attention indicator when the user isn't already viewing the target pane. The logic is clear and defensive.
17-21: No changes required. The return type ofresolveNotificationTargetis already properly typed asResolvedTarget | null, whereResolvedTargetextendsNotificationIdswith a requiredworkspaceIdfield. The null check on line 19 correctly guards against undefined targets, and the destructuring on line 21 is fully type-safe.
1 participant
Summary
NotificationIdsinterface for shared type across notification systemresolveNotificationTargetutility to resolve missing IDs from stateuseAgentHookListenerto use shared resolution logicTest plan
resolveNotificationTarget(10 tests)🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.