clicking on notifs should navigate to pane#1344
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds URL-search-based workspace navigation (tabId, paneId) consumed and cleared by the workspace route; updates agent hook to navigate with search params; introduces a new main-process NotificationManager with tests and integrates it into window lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as AgentLifecycleProducer
participant NM as NotificationManager
participant Native as NativeNotification (OS)
participant Win as Window/Renderer
Agent->>NM: handleAgentLifecycle(event)
NM->>NM: shouldSuppressForVisiblePane? (getVisibilityContext)
alt not suppressed
NM->>Native: createNotification(title, body)
Native-->>NM: notificationInstance
NM->>Native: playSound()
Native->>NM: on('click')
Native->>NM: on('close')
Native->>Win: click -> navigate/focus payload (via IPC)
else suppressed
NM-->>Agent: ignored (no notification)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/page.tsx:
- Around line 74-90: The effect that reads useTabsStore.getState() can return
early when state.tabs.find(...) yields undefined and leaves stale search params;
modify the effect in page.tsx so that if no tab is found for searchTabId you
call routeNavigate({ search: {}, replace: true }) before returning (ensuring
search params are cleared even if the tab hasn't loaded), and optionally
consider the alternative approach of subscribing to the tabs slice (instead of
using getState) so the effect re-runs when tabs are populated; target symbols:
useEffect, useTabsStore.getState, state.tabs.find, state.setActiveTab,
state.setFocusedPane, routeNavigate, searchTabId, searchPaneId.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.ts (1)
22-24: Also omit"search"fromNavigateOptionsfor proper type narrowing.
NavigateOptionsfrom TanStack Router has asearchproperty that accepts broader types (true | TToSearch | ((prev: TFromSearch) => TToSearch)). The current intersection overrides this but doesn't prevent type confusion. UseOmit<NavigateOptions, "to" | "params" | "search"> & { search?: WorkspaceSearchParams }to explicitly narrow thesearchtype and prevent callers from accidentally relying on unsupported search patterns.
| useEffect(() => { | ||
| if (!searchTabId) return; | ||
|
|
||
| const state = useTabsStore.getState(); | ||
| const tab = state.tabs.find( | ||
| (t) => t.id === searchTabId && t.workspaceId === workspaceId, | ||
| ); | ||
| if (!tab) return; | ||
|
|
||
| state.setActiveTab(workspaceId, searchTabId); | ||
|
|
||
| if (searchPaneId && state.panes[searchPaneId]) { | ||
| state.setFocusedPane(searchTabId, searchPaneId); | ||
| } | ||
|
|
||
| routeNavigate({ search: {}, replace: true }); | ||
| }, [searchTabId, searchPaneId, workspaceId, routeNavigate]); |
There was a problem hiding this comment.
If the target tab isn't in the store yet, search params silently persist without retry.
When state.tabs.find(...) returns undefined at line 78-81, the effect returns early without clearing the search params. Since the effect's dependency array doesn't include any store state (intentionally — it reads imperatively), it won't re-run when tabs eventually load. This leaves stale ?tabId=...&paneId=... in the URL.
If this is an unlikely edge case (tabs are persisted and always available), this is fine as-is. But if there's any scenario where tabs load asynchronously after mount, consider either:
- Subscribing to the relevant store slice so the effect retries, or
- Clearing the search params even on the early-return path (accepting that the navigation might be a no-op).
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/page.tsx
around lines 74 - 90, The effect that reads useTabsStore.getState() can return
early when state.tabs.find(...) yields undefined and leaves stale search params;
modify the effect in page.tsx so that if no tab is found for searchTabId you
call routeNavigate({ search: {}, replace: true }) before returning (ensuring
search params are cleared even if the tab hasn't loaded), and optionally
consider the alternative approach of subscribing to the tabs slice (instead of
using getState) so the effect re-runs when tabs are populated; target symbols:
useEffect, useTabsStore.getState, state.tabs.find, state.setActiveTab,
state.setFocusedPane, routeNavigate, searchTabId, searchPaneId.
Electron Notification objects get garbage collected on macOS while the native notification is still visible, silently dropping click handlers. Extract notification lifecycle into a testable NotificationManager that retains references, replaces per-pane, and sweeps stale entries on a TTL.
Kitenite
force-pushed
the
kitenite/clicking-on-notifs-should-navigate-to-pane
branch
from
596d670 to
6cdde6d
Compare
Kitenite
deleted the
kitenite/clicking-on-notifs-should-navigate-to-pane
branch
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/notifications/notification-manager.ts`:
- Around line 103-109: The dispose() method currently clears the sweep timer and
the active map but does not close native OS notifications, which can leave click
handlers (e.g., the onNotificationClick closure in main.ts) pointing at a
destroyed BrowserWindow; update NotificationManager.dispose() to iterate over
this.active (or this.active.values()) and call close() on each native
Notification object before clearing the map, then clear the interval and null
the sweepTimer as before so lingering notifications cannot trigger
window.show()/focus() on a destroyed window.
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/notifications/notification-manager.ts (1)
140-147:sweep()removes stale entries without closing the native notification.Same pattern as
dispose()— the notification object loses its strong reference but the OS notification may still be visible. Closing it explicitly is more defensive:Proposed fix
private sweep(): void { const now = Date.now(); for (const [key, entry] of this.active) { if (now - entry.createdAt > NOTIFICATION_TTL_MS) { + entry.notification.close(); this.active.delete(key); } } }apps/desktop/src/main/lib/notifications/notification-manager.test.ts (1)
30-33:clickedIdstyped asAgentLifecycleEvent[]but receivesNotificationIds.The
onNotificationClickcallback receivesNotificationIds(which lackseventType), but the test stores it asAgentLifecycleEventvia anascast on line 51. The test assertion on line 157-159 happens to pass because it only checks the overlapping fields, but this masks a type mismatch. UseNotificationIds[]instead:Proposed fix
+import type { NotificationIds } from "shared/notification-types"; + interface TestDeps extends NotificationManagerDeps { notifications: MockNotification[]; - clickedIds: AgentLifecycleEvent[]; + clickedIds: NotificationIds[]; }And on line 39/51:
- const clickedIds: AgentLifecycleEvent[] = []; + const clickedIds: NotificationIds[] = []; ... - onNotificationClick: (ids) => clickedIds.push(ids as AgentLifecycleEvent), + onNotificationClick: (ids) => clickedIds.push(ids),
| dispose(): void { | ||
| if (this.sweepTimer) { | ||
| clearInterval(this.sweepTimer); | ||
| this.sweepTimer = null; | ||
| } | ||
| this.active.clear(); | ||
| } |
There was a problem hiding this comment.
dispose() clears tracking but does not close the native notifications.
Active OS notifications will remain visible after the window closes, but their click handlers capture the now-destroyed BrowserWindow (via the onNotificationClick closure in main.ts). If a user clicks a lingering notification before GC collects it, calling window.show()/window.focus() on a destroyed window will throw.
Consider closing each notification before clearing the map:
Proposed fix
dispose(): void {
if (this.sweepTimer) {
clearInterval(this.sweepTimer);
this.sweepTimer = null;
}
+ for (const entry of this.active.values()) {
+ entry.notification.close();
+ }
this.active.clear();
}🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/notifications/notification-manager.ts` around lines
103 - 109, The dispose() method currently clears the sweep timer and the active
map but does not close native OS notifications, which can leave click handlers
(e.g., the onNotificationClick closure in main.ts) pointing at a destroyed
BrowserWindow; update NotificationManager.dispose() to iterate over this.active
(or this.active.values()) and call close() on each native Notification object
before clearing the map, then clear the interval and null the sweepTimer as
before so lingering notifications cannot trigger window.show()/focus() on a
destroyed window.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Refactor
Tests