fix(notifications): request OS permission at startup and fix socketService listener leak#998
Merged
senamakel merged 11 commits intotinyhumansai:mainfrom Apr 29, 2026
Conversation
…rvice listener leak Two independent bugs prevented desktop notifications from firing: 1. OS notification permission was never proactively requested. The Tauri plugin's .show() call silently fails on macOS/Windows when the user hasn't granted permission. Added ensureNotificationPermission() to tauriBridge.ts (checks isPermissionGranted, requests if denied) and calls it at startNativeNotificationsService() time. 2. socketService.on() wrapped every callback in a new closure but off() passed the original reference to socket.off(), which uses reference equality — so listeners were never actually removed. Fixed by tracking original→wrapped in a listenerMap; off() now looks up the wrapped reference, deletes from the map, and filters the pendingListeners queue. Also adds diagnostic logging to the Rust show_native_notification command (permission state now visible in logs), data-testid attributes to Notifications.tsx, and a new WDIO E2E spec that tests the full integration notification pipeline via core RPC (notification_ingest, notification_list, notification_mark_read, notification_stats) plus DOM assertions for the Notifications page (Linux CI only). Closes tinyhumansai#932
Contributor
|
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 OS notification permission queries/requests to the Tauri bridge and frontend; ensures permissions at service startup and before test sends; switches native notify to plugin API (macOS default sound); tightens socket listener lifecycle; deduplicates notifications in reducer; adds E2E tests and test selectors for Notifications UI. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant Service as Notifications Service
participant Bridge as Frontend Bridge
participant Tauri as Tauri Runtime
participant OS as OS / User
participant Socket as Socket Service
App->>Service: startNativeNotificationsService()
Service->>Bridge: ensureNotificationPermission()
Bridge->>Tauri: invoke `notification_permission_state`
Tauri->>OS: Query UNUserNotificationCenter / OS permission
OS-->>Tauri: Permission state
Tauri-->>Bridge: state (granted/denied/err)
alt Not granted on supported platform
Bridge->>Tauri: invoke `notification_permission_request`
Tauri->>OS: Prompt user
OS-->>Tauri: User response
Tauri-->>Bridge: request result
end
Bridge-->>Service: boolean granted
Service->>Socket: register `core_notification` listener (wrapped callback)
Socket->>Service: `core_notification` event (id,title,...)
Service->>Bridge: showNativeNotification(options with sound="default" on macOS)
Bridge->>Tauri: invoke `plugin:notification|notify`
Tauri->>OS: Display native toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/services/socketService.ts (1)
327-355:⚠️ Potential issue | 🟠 MajorListener map needs event scoping; current shape can still leak listeners.
The new
listenerMapis keyed only by callback. If the same callback is subscribed to multiple events, lateron()calls overwrite earlier mappings, andoff(event, callback)may detach the wrong wrapped function. Also,off(event)does not clear queuedpendingListenersfor that event.🔧 Suggested refactor
- private listenerMap = new Map<(...args: unknown[]) => void, (...args: unknown[]) => void>(); + private listenerMap = new Map< + string, + Map<(...args: unknown[]) => void, (...args: unknown[]) => void> + >(); on(event: string, callback: (...args: unknown[]) => void): void { const wrappedCallback = (...args: unknown[]) => { socketLog('Received event', { event, argsCount: args.length, hasData: args.length > 0 }); callback(...args); }; - this.listenerMap.set(callback, wrappedCallback); + const byEvent = this.listenerMap.get(event) ?? new Map(); + byEvent.set(callback, wrappedCallback); + this.listenerMap.set(event, byEvent); if (this.socket) { this.socket.on(event, wrappedCallback); } else { socketLog('Socket not ready, queuing listener', { event }); this.pendingListeners.push({ event, callback: wrappedCallback }); } } off(event: string, callback?: (...args: unknown[]) => void): void { if (callback) { - const wrapped = this.listenerMap.get(callback) ?? callback; - const hadWrapped = this.listenerMap.has(callback); - this.listenerMap.delete(callback); + const byEvent = this.listenerMap.get(event); + const wrapped = byEvent?.get(callback) ?? callback; + const hadWrapped = byEvent?.has(callback) ?? false; + byEvent?.delete(callback); + if (byEvent && byEvent.size === 0) this.listenerMap.delete(event); socketLog('Removing listener', { event, hadWrappedVersion: hadWrapped }); if (this.socket) this.socket.off(event, wrapped); this.pendingListeners = this.pendingListeners.filter( p => !(p.event === event && p.callback === wrapped) ); } else { socketLog('Removing all listeners for event', { event }); this.socket?.off(event); + this.pendingListeners = this.pendingListeners.filter(p => p.event !== event); + this.listenerMap.delete(event); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/socketService.ts` around lines 327 - 355, The listenerMap must be scoped by event so callbacks subscribed to multiple events don't collide: change listenerMap from Map<Function, Wrapped> to a two-level structure keyed by event (e.g., Map<string, Map<callback, wrappedCallback>>) or use a composite key (event+callback) when storing in on(), and update off(event, callback) to look up and remove the wrappedCallback only within that event's scope (use the inner map or composite key to delete and decide hadWrapped). Also ensure off(event) removes any pendingListeners for that event (filter pendingListeners by p.event === event) in addition to calling this.socket?.off(event). Keep references to wrappedCallback when queuing and clearing so queued listeners are removed correctly.
🧹 Nitpick comments (1)
app/test/e2e/specs/notifications.spec.ts (1)
116-117: Replace the fixed 2s sleeps with state-based waits.
browser.pause(2_000)is a common CI flake source here. Wait for the route-specific section or notification text instead so the test advances when the page is actually ready, not after an arbitrary delay.Also applies to: 142-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/notifications.spec.ts` around lines 116 - 117, Replace the fixed browser.pause(2_000) after navigateViaHash('/notifications') with a state-based wait: instead of sleeping, wait for a route-specific element to exist/ be displayed (e.g., the notifications container or a notification text) using the test framework's waitForExist/waitForDisplayed or browser.waitUntil so the test proceeds when the page is ready; update the same pattern at the other occurrence(s) around the 142-143 region. Target symbols: navigateViaHash and browser.pause and the notifications container/notification text selector used in this spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/test/e2e/helpers/shared-flows.ts`:
- Around line 250-252: The helper navigateToNotifications calls
navigateViaHash('/notifications') but the Mac2 sidebar fallback mapping lacks a
'/notifications' entry causing failures; add a '/notifications' key to the Mac2
sidebar fallback mapping object (the same mapping structure used for other Mac2
routes) and point it to the corresponding internal route used by Mac2 so
navigateViaHash('/notifications') resolves correctly (update the Mac2 sidebar
fallback mapping where sidebar route mappings are defined so
navigateToNotifications and navigateViaHash no longer throw on non-execute
sessions).
In `@app/test/e2e/specs/notifications.spec.ts`:
- Around line 92-108: Before calling openhuman.notification_mark_read, call
callOpenhumanRpc('openhuman.notification_stats', {}) to capture the initial
unread_count from result.result?.result (e.g. store as initialUnread), then call
callOpenhumanRpc('openhuman.notification_mark_read', { id: 'e2e-notif-001' })
and finally re-fetch stats and assert the unread_count decreased (or if
initialUnread is 0, that it did not increase). Update the test that currently
uses variables result/stats to perform these three RPCs and add an assertion
comparing the two unread_count values to ensure the mutation actually changed
state.
- Around line 64-74: The test currently only asserts RPC-level success; update
the 'notification_ingest creates a new notification via core RPC' test to also
assert the inner payload indicates a real creation (not a skipped outcome) by
checking the returned payload (result.result) for an outcome like 'created' and
that it is not 'skipped' or 'provider_disabled', and then verify persistence by
calling the read RPC (e.g., callOpenhumanRpc('openhuman.notification_get' or the
appropriate fetch/list RPC) to fetch the notification with id 'e2e-notif-001'
and assert the fetched record exists and has the expected id/title; use the
symbols callOpenhumanRpc, result, result.result, and the test id 'e2e-notif-001'
to locate and change the assertions.
---
Outside diff comments:
In `@app/src/services/socketService.ts`:
- Around line 327-355: The listenerMap must be scoped by event so callbacks
subscribed to multiple events don't collide: change listenerMap from
Map<Function, Wrapped> to a two-level structure keyed by event (e.g.,
Map<string, Map<callback, wrappedCallback>>) or use a composite key
(event+callback) when storing in on(), and update off(event, callback) to look
up and remove the wrappedCallback only within that event's scope (use the inner
map or composite key to delete and decide hadWrapped). Also ensure off(event)
removes any pendingListeners for that event (filter pendingListeners by p.event
=== event) in addition to calling this.socket?.off(event). Keep references to
wrappedCallback when queuing and clearing so queued listeners are removed
correctly.
---
Nitpick comments:
In `@app/test/e2e/specs/notifications.spec.ts`:
- Around line 116-117: Replace the fixed browser.pause(2_000) after
navigateViaHash('/notifications') with a state-based wait: instead of sleeping,
wait for a route-specific element to exist/ be displayed (e.g., the
notifications container or a notification text) using the test framework's
waitForExist/waitForDisplayed or browser.waitUntil so the test proceeds when the
page is ready; update the same pattern at the other occurrence(s) around the
142-143 region. Target symbols: navigateViaHash and browser.pause and the
notifications container/notification text selector used in this spec.
🪄 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: f398e4e2-de7e-4b03-b736-4d3346668db8
📒 Files selected for processing (9)
app/src-tauri/src/lib.rsapp/src/lib/nativeNotifications/index.tsapp/src/lib/nativeNotifications/service.tsapp/src/lib/nativeNotifications/tauriBridge.tsapp/src/pages/Notifications.tsxapp/src/services/socketService.tsapp/test/e2e/helpers/shared-flows.tsapp/test/e2e/specs/notifications.spec.tsdocs/E2E-TESTING.md
Unify notification permission/send behavior on the plugin command path, add a delayed test send for macOS banner verification, and dedupe repeated notification IDs to prevent duplicate System Events/Alerts entries. Made-with: Cursor
Capture formatter changes triggered by repository hooks so notification-related updates pass pre-push quality checks. Made-with: Cursor
…ermission-and-listener-leak
Scope socket listener wrappers per event to prevent callback collisions, add Mac2 notifications route mapping, and strengthen notification E2E assertions/waits to validate ingest and read-state transitions. Made-with: Cursor
Apply Prettier formatting adjustments required by pre-push checks. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/components/OpenhumanLinkModal.tsx (1)
191-241:⚠️ Potential issue | 🟠 MajorCancel the queued notification when the modal closes.
The 3.5s timer survives unmount/close, so the test notification can still fire after the user dismisses the flow, and the final
setStatus('sent')will hit an unmounted component. Track the timeout in a ref and clear it on close/unmount.♻️ Suggested fix
const NotificationsBody = ({ close }: { close: () => void }) => { const [status, setStatus] = useState<'idle' | 'sending' | 'queued' | 'sent' | 'error'>('idle'); const [error, setError] = useState<string | null>(null); + const timeoutRef = useRef<number | null>(null); + + useEffect( + () => () => { + if (timeoutRef.current !== null) { + window.clearTimeout(timeoutRef.current); + } + }, + [] + ); @@ - await new Promise(resolve => setTimeout(resolve, TEST_NOTIFICATION_DELAY_MS)); + await new Promise<void>(resolve => { + timeoutRef.current = window.setTimeout(resolve, TEST_NOTIFICATION_DELAY_MS); + }); + timeoutRef.current = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/OpenhumanLinkModal.tsx` around lines 191 - 241, The queued 3.5s timer in handleAllow can outlive the modal and call state on an unmounted component; modify handleAllow to capture the timeout id (from setTimeout or Promise wrapper) in a ref (e.g., notificationTimeoutRef) and clear it when the modal closes or the component unmounts, and also guard the post-timeout work (the showNativeNotification and final setStatus('sent')) by checking the ref/state to avoid running after clear; implement a cleanup in the modal close handler or a useEffect return that calls clearTimeout(notificationTimeoutRef.current) and resets the ref so the pending promise/timeout is cancelled if the user dismisses the modal before TEST_NOTIFICATION_DELAY_MS elapses.app/src/services/socketService.ts (1)
276-285:⚠️ Potential issue | 🟠 MajorClear queued listeners even when there is no active socket.
As written, the cleanup is skipped when
this.socketis null, so callbacks queued during an in-flight connect survive logout and can be replayed on the next session. Move the listener-state reset outside theif (this.socket)guard, and clearpendingListenersas part of the disconnect path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/socketService.ts` around lines 276 - 285, The disconnect() method only performs cleanup when this.socket exists, leaving queued callbacks in pendingListeners and listenerMap when there is no active socket; to fix it, compute uid (via getSocketUserId()) then always clear listener state and auth/transport fields (listenerMap.clear(), pendingListeners.clear(), this.token = null, this.mcpTransport = null) and dispatch resetForUser({ userId: uid }) outside the if-block, while keeping the socket-specific actions (socketLog, this.socket.disconnect(), this.socket = null) inside the existing if (this.socket) guard so we both properly disconnect an active socket and ensure queued listeners are cleared on logout even if no socket was present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/services/socketService.ts`:
- Around line 120-125: listenerMap currently maps event -> Map<originalFn,
singleWrappedFn> so registering the same original function twice overwrites the
stored wrapper and leaves the first wrapper attached; change listenerMap to map
event -> Map<originalFn, Set<wrappedFn>> (or array) in the on(event, fn) path:
when creating a new wrappedCallback push/add it into the Set instead of
replacing, and in off(event, fn) iterate the Set removing each stored wrapped
callback from socket.off and then clear/delete the entry; also ensure
once(event, fn) creates a tracked wrapped callback (and is removed from the Set
after it fires) so bookkeeping remains consistent with on/off.
---
Outside diff comments:
In `@app/src/components/OpenhumanLinkModal.tsx`:
- Around line 191-241: The queued 3.5s timer in handleAllow can outlive the
modal and call state on an unmounted component; modify handleAllow to capture
the timeout id (from setTimeout or Promise wrapper) in a ref (e.g.,
notificationTimeoutRef) and clear it when the modal closes or the component
unmounts, and also guard the post-timeout work (the showNativeNotification and
final setStatus('sent')) by checking the ref/state to avoid running after clear;
implement a cleanup in the modal close handler or a useEffect return that calls
clearTimeout(notificationTimeoutRef.current) and resets the ref so the pending
promise/timeout is cancelled if the user dismisses the modal before
TEST_NOTIFICATION_DELAY_MS elapses.
In `@app/src/services/socketService.ts`:
- Around line 276-285: The disconnect() method only performs cleanup when
this.socket exists, leaving queued callbacks in pendingListeners and listenerMap
when there is no active socket; to fix it, compute uid (via getSocketUserId())
then always clear listener state and auth/transport fields (listenerMap.clear(),
pendingListeners.clear(), this.token = null, this.mcpTransport = null) and
dispatch resetForUser({ userId: uid }) outside the if-block, while keeping the
socket-specific actions (socketLog, this.socket.disconnect(), this.socket =
null) inside the existing if (this.socket) guard so we both properly disconnect
an active socket and ensure queued listeners are cleared on logout even if no
socket was present.
🪄 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: 7d92eb24-61bb-407c-9d81-deacfbde0c36
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
app/src-tauri/Cargo.tomlapp/src-tauri/src/lib.rsapp/src-tauri/vendor/tauri-plugin-notificationapp/src/components/OpenhumanLinkModal.tsxapp/src/lib/nativeNotifications/service.tsapp/src/lib/nativeNotifications/tauriBridge.tsapp/src/services/socketService.tsapp/src/store/__tests__/notificationSlice.test.tsapp/src/store/notificationSlice.tsapp/test/e2e/helpers/shared-flows.tsapp/test/e2e/specs/notifications.spec.ts
✅ Files skipped from review due to trivial changes (2)
- app/src-tauri/Cargo.toml
- app/src-tauri/vendor/tauri-plugin-notification
🚧 Files skipped from review as they are similar to previous changes (4)
- app/test/e2e/helpers/shared-flows.ts
- app/test/e2e/specs/notifications.spec.ts
- app/src/lib/nativeNotifications/service.ts
- app/src-tauri/src/lib.rs
Cancel queued notification timers on modal unmount and harden socket listener cleanup by tracking multiple wrapped callbacks per event+handler and clearing queued listeners during disconnect. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/services/socketService.ts (1)
179-185:⚠️ Potential issue | 🟠 MajorPreserve
once()semantics when the listener is queued before connect.If
once()runs before a socket exists, it queues the wrapped handler, but the connect path replays every pending entry withsocket.on(...). That turns a one-shot listener into a permanent one, and after the first fire thefinallyblock removes itslistenerMapentry, so lateroff()calls can no longer detach the still-registered wrapper.💡 Suggested fix
- private pendingListeners: Array<{ event: string; callback: (...args: unknown[]) => void }> = []; + private pendingListeners: Array<{ + event: string; + callback: (...args: unknown[]) => void; + once: boolean; + }> = [];- for (const { event, callback } of this.pendingListeners) { - this.socket.on(event, callback); + for (const { event, callback, once } of this.pendingListeners) { + if (once) { + this.socket.once(event, callback); + } else { + this.socket.on(event, callback); + } }- this.pendingListeners.push({ event, callback: wrappedCallback }); + this.pendingListeners.push({ event, callback: wrappedCallback, once: false });- this.pendingListeners.push({ event, callback: wrappedCallback }); + this.pendingListeners.push({ event, callback: wrappedCallback, once: true });Also applies to: 381-412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/socketService.ts` around lines 179 - 185, Pending one-time listeners queued in this.pendingListeners get registered with socket.on during the flush, turning them into permanent listeners and breaking off() removal; change the flush logic (both the block that iterates pendingListeners and the similar code at lines ~381-412) to detect queued entries that were created via once() and call socket.once(...) for them instead of socket.on(...), and ensure the wrapper function stored in listenerMap (and removed in the finally block) remains the same wrapper reference so off() can locate and remove the underlying socket listener correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/src/services/socketService.ts`:
- Around line 179-185: Pending one-time listeners queued in
this.pendingListeners get registered with socket.on during the flush, turning
them into permanent listeners and breaking off() removal; change the flush logic
(both the block that iterates pendingListeners and the similar code at lines
~381-412) to detect queued entries that were created via once() and call
socket.once(...) for them instead of socket.on(...), and ensure the wrapper
function stored in listenerMap (and removed in the finally block) remains the
same wrapper reference so off() can locate and remove the underlying socket
listener correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 630a6052-28b9-40e2-af18-b84ebe77f0a3
📒 Files selected for processing (2)
app/src/components/OpenhumanLinkModal.tsxapp/src/services/socketService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/components/OpenhumanLinkModal.tsx
graycyrus
reviewed
Apr 28, 2026
Contributor
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Reviewed the two root-cause fixes (permission request + listener leak) and the surrounding changes. The core ideas are sound — ensureNotificationPermission and listenerMap are the right primitives. However there are a few correctness issues worth addressing before merge, and two metadata points the author should clarify.
#984 is not closed by this PR. The PR body says "Closes #984" but there is zero routing logic anywhere in the diff. handleClick in Notifications.tsx is unchanged, and the deep_link field that the service already populates on NotificationItem is never consumed on click. Please remove #984 from the closing references and open a separate PR/issue for that work.
@ts-nocheck in the new spec. app/test/e2e/specs/notifications.spec.ts opens with // @ts-nocheck, which is not used in any other spec in the project. This silences the type errors from the raw window.__TAURI_INTERNALS__ access (see inline comment below) but also suppresses every other type check in the file. Please remove it and fix the underlying types instead.
Address remaining PR feedback by scoping socket reset dispatch to active connections, removing always-on notification console logs, handling non-Tauri notification checks in the modal flow, and tightening notifications E2E polling/assertions. Made-with: Cursor
Include formatter changes from pre-push checks so branch is clean for push. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/services/socketService.ts (1)
407-412:⚠️ Potential issue | 🟠 Major
once()queues wrapped callback but flush usessocket.on()instead ofsocket.once().When the socket isn't ready,
once()pushes topendingListeners(line 411). However,connectAsync()flushes all pending listeners viathis.socket.on(event, callback)(line 183), notsocket.once(). This means aonce()listener registered before connection will fire on every subsequent event, not just once.🐛 Proposed fix — track listener type in pendingListeners
- private pendingListeners: Array<{ event: string; callback: (...args: unknown[]) => void }> = []; + private pendingListeners: Array<{ event: string; callback: (...args: unknown[]) => void; once?: boolean }> = []; // In connectAsync flush: for (const { event, callback } of this.pendingListeners) { - this.socket.on(event, callback); + const { event, callback, once } = pending; + if (once) { + this.socket.once(event, callback); + } else { + this.socket.on(event, callback); + } } // In once(): - this.pendingListeners.push({ event, callback: wrappedCallback }); + this.pendingListeners.push({ event, callback: wrappedCallback, once: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/socketService.ts` around lines 407 - 412, The pending once-listeners are being queued by once() but connectAsync() flushes everything with this.socket.on, causing once listeners to become persistent; modify the shape pushed into pendingListeners (from { event, callback: wrappedCallback } to include a type or once boolean, e.g. { event, callback: wrappedCallback, once: true }) in the once() implementation, and update the flush logic in connectAsync() (where it currently calls this.socket.on(event, callback)) to call this.socket.once(event, callback) for entries with once: true and this.socket.on for the rest so queued once-listeners fire only once.
🧹 Nitpick comments (2)
app/src/components/OpenhumanLinkModal.tsx (1)
219-227: Promise-based delay works but could be cleaner with AbortController.The current pattern using
cancelledRefis functional. For future consideration, anAbortControllerpattern would be more idiomatic for cancellable async operations, but this works correctly as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/OpenhumanLinkModal.tsx` around lines 219 - 227, Replace the manual cancelledRef + timeout Promise pattern with an AbortController-driven cancellable delay: create an AbortController (e.g., local or stored in a ref) whose signal is passed to the delay logic that currently uses notificationTimeoutRef and TEST_NOTIFICATION_DELAY_MS; set the timeout as before but also add signal.addEventListener('abort') to clear the timeout and reject/resolve the Promise early, and call controller.abort() from the modal cleanup/close path instead of setting cancelledRef. Remove usages of cancelledRef and ensure notificationTimeoutRef is cleared when aborting.app/test/e2e/specs/notifications.spec.ts (1)
1-1: Consider removing@ts-nocheckand fixing type issues.
@ts-nocheckdisables all TypeScript checking for the file. While this may be expedient for E2E specs with complex browser context types, it masks potential issues. Consider using targeted//@ts-expect-error`` comments or proper type assertions where needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/notifications.spec.ts` at line 1, Remove the top-level "// `@ts-nocheck`" and instead address the specific TypeScript errors in the file: run the type checker to locate failing symbols (e.g., references to Playwright's page, browser, locator, or test helpers inside notifications.spec.ts), replace broad suppression with targeted "// `@ts-expect-error`" only where unavoidable, add proper type assertions/casts (e.g., as Page, Locator) or import correct types, and ensure any custom helpers have accurate type definitions so the file can be type-checked without disabling TypeScript globally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/test/e2e/specs/notifications.spec.ts`:
- Around line 252-257: The test calls the notification plugin via
invoker('plugin:notification|notify', { options: { title, body } }) but omits
the sound field used in production; update the options passed in the test (the
invoker call in notifications.spec.ts) to include sound: 'default' so it mirrors
the production showNativeNotification behavior and exercises the same code path.
---
Outside diff comments:
In `@app/src/services/socketService.ts`:
- Around line 407-412: The pending once-listeners are being queued by once() but
connectAsync() flushes everything with this.socket.on, causing once listeners to
become persistent; modify the shape pushed into pendingListeners (from { event,
callback: wrappedCallback } to include a type or once boolean, e.g. { event,
callback: wrappedCallback, once: true }) in the once() implementation, and
update the flush logic in connectAsync() (where it currently calls
this.socket.on(event, callback)) to call this.socket.once(event, callback) for
entries with once: true and this.socket.on for the rest so queued once-listeners
fire only once.
---
Nitpick comments:
In `@app/src/components/OpenhumanLinkModal.tsx`:
- Around line 219-227: Replace the manual cancelledRef + timeout Promise pattern
with an AbortController-driven cancellable delay: create an AbortController
(e.g., local or stored in a ref) whose signal is passed to the delay logic that
currently uses notificationTimeoutRef and TEST_NOTIFICATION_DELAY_MS; set the
timeout as before but also add signal.addEventListener('abort') to clear the
timeout and reject/resolve the Promise early, and call controller.abort() from
the modal cleanup/close path instead of setting cancelledRef. Remove usages of
cancelledRef and ensure notificationTimeoutRef is cleared when aborting.
In `@app/test/e2e/specs/notifications.spec.ts`:
- Line 1: Remove the top-level "// `@ts-nocheck`" and instead address the specific
TypeScript errors in the file: run the type checker to locate failing symbols
(e.g., references to Playwright's page, browser, locator, or test helpers inside
notifications.spec.ts), replace broad suppression with targeted "//
`@ts-expect-error`" only where unavoidable, add proper type assertions/casts
(e.g., as Page, Locator) or import correct types, and ensure any custom helpers
have accurate type definitions so the file can be type-checked without disabling
TypeScript globally.
🪄 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: f8fe2576-a320-4e12-8d37-e2b1dcf26e7d
📒 Files selected for processing (5)
app/src/components/OpenhumanLinkModal.tsxapp/src/lib/nativeNotifications/service.tsapp/src/lib/nativeNotifications/tauriBridge.tsapp/src/services/socketService.tsapp/test/e2e/specs/notifications.spec.ts
Add shared notification route resolution for integration and system alerts so card clicks navigate to the best destination with deterministic fallbacks, mark integration notifications as acted after click, and cover routing + acted-state behavior with new unit tests. Closes tinyhumansai#984 Made-with: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/lib/notificationRouter.test.ts (1)
67-93: Consider adding test for unknown category fallback.The
resolveSystemRoutefunction has adefaultcase that returns/notificationsfor unrecognized categories, but this path isn't tested. Adding coverage ensures the fallback behavior is verified.💚 Suggested test addition
it('prefers deepLink over category default', () => { const item = makeSystem({ category: 'messages', deepLink: '/notifications' }); expect(resolveSystemRoute(item)).toBe('/notifications'); }); + + it('falls back to /notifications for unknown category', () => { + // Cast to bypass TypeScript for edge-case testing + const item = makeSystem({ category: 'unknown-category' as NotificationItem['category'] }); + expect(resolveSystemRoute(item)).toBe('/notifications'); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/notificationRouter.test.ts` around lines 67 - 93, Add a unit test verifying the default fallback path from resolveSystemRoute for unrecognized categories: create a system item via makeSystem with an unknown category (e.g., 'unknown' or 'bogus') and assert resolveSystemRoute(...) returns '/notifications'; this ensures the function's default case is covered alongside the existing category-specific tests.app/src/lib/notificationRouter.ts (1)
46-47: Consider removing redundant prefix from log messages.The
debugnamespace already identifies the source asnotifications:router. The[notification-router]prefix in each log call is redundant.♻️ Suggested simplification
- log('[notification-router] integration id=%s explicit deep_link=%s', n.id, n.deep_link); + log('integration id=%s explicit deep_link=%s', n.id, n.deep_link);Apply similar changes to other log calls in both functions.
Also applies to: 51-52, 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/notificationRouter.ts` around lines 46 - 47, Remove the redundant "[notification-router]" prefix from the debug log messages and keep the existing debug namespace (notifications:router) consistent; update the format strings passed to log(...) in notificationRouter.ts (the calls around the return of n.deep_link and the other log calls in the same functions) to drop the prefix and only include the concise message and variables (e.g., change log('[notification-router] integration id=%s explicit deep_link=%s', n.id, n.deep_link) to log('integration id=%s explicit deep_link=%s', n.id, n.deep_link)), and apply the same simplification to the other log calls referenced in these functions so messages remain clear but not duplicated.app/src/components/notifications/NotificationCenter.tsx (1)
81-93: Optimistic update without rollback could cause state drift.If
markNotificationActed(id)fails (per context snippet showing it throws), the UI will displayactedstatus while the server retainsunread. Unlike transient read-marks, an "acted" status implies the user took meaningful action — persisting this mismatch could cause confusion (e.g., notification reappears as unread on refresh).Consider logging the error for diagnostics or implementing a rollback:
🛡️ Option 1: Log the error for observability
try { await markNotificationActed(id); } catch { - // Optimistic update already applied; failure is non-critical. + // Optimistic update already applied; log for diagnostics. + console.warn('[NotificationCenter] markNotificationActed failed for id=%s', id); }🛡️ Option 2: Rollback on failure (more robust)
+import { markIntegrationRead } from '../../store/notificationSlice'; + const handleNavigate = async (id: string) => { const n = items.find(i => i.id === id); if (!n) return; + const previousStatus = n.status; const route = resolveIntegrationRoute(n); dispatch(markIntegrationActed(id)); navigate(route); try { await markNotificationActed(id); } catch { - // Optimistic update already applied; failure is non-critical. + // Rollback to previous status on failure + if (previousStatus === 'unread') { + dispatch(markIntegrationRead(id)); // or a dedicated rollback action + } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/notifications/NotificationCenter.tsx` around lines 81 - 93, handleNavigate currently applies an optimistic "acted" update (dispatch(markIntegrationActed(id))) then calls markNotificationActed(id) but does not undo the optimistic change on failure or log the error; update handleNavigate to catch errors from markNotificationActed(id), log the error (using the same logger/context used elsewhere) and perform a rollback by dispatching an undo action (e.g., dispatch(unmarkIntegrationActed(id)) or a dedicated revert action) so UI state stays consistent with server state; keep the optimistic flow (resolveIntegrationRoute, navigate) but ensure you reference the existing symbols handleNavigate, markNotificationActed, markIntegrationActed, unmarkIntegrationActed (or create it if missing), items, resolveIntegrationRoute, navigate and add the error logging and rollback inside the catch block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/components/notifications/NotificationCenter.tsx`:
- Around line 81-93: handleNavigate currently applies an optimistic "acted"
update (dispatch(markIntegrationActed(id))) then calls markNotificationActed(id)
but does not undo the optimistic change on failure or log the error; update
handleNavigate to catch errors from markNotificationActed(id), log the error
(using the same logger/context used elsewhere) and perform a rollback by
dispatching an undo action (e.g., dispatch(unmarkIntegrationActed(id)) or a
dedicated revert action) so UI state stays consistent with server state; keep
the optimistic flow (resolveIntegrationRoute, navigate) but ensure you reference
the existing symbols handleNavigate, markNotificationActed,
markIntegrationActed, unmarkIntegrationActed (or create it if missing), items,
resolveIntegrationRoute, navigate and add the error logging and rollback inside
the catch block.
In `@app/src/lib/notificationRouter.test.ts`:
- Around line 67-93: Add a unit test verifying the default fallback path from
resolveSystemRoute for unrecognized categories: create a system item via
makeSystem with an unknown category (e.g., 'unknown' or 'bogus') and assert
resolveSystemRoute(...) returns '/notifications'; this ensures the function's
default case is covered alongside the existing category-specific tests.
In `@app/src/lib/notificationRouter.ts`:
- Around line 46-47: Remove the redundant "[notification-router]" prefix from
the debug log messages and keep the existing debug namespace
(notifications:router) consistent; update the format strings passed to log(...)
in notificationRouter.ts (the calls around the return of n.deep_link and the
other log calls in the same functions) to drop the prefix and only include the
concise message and variables (e.g., change log('[notification-router]
integration id=%s explicit deep_link=%s', n.id, n.deep_link) to log('integration
id=%s explicit deep_link=%s', n.id, n.deep_link)), and apply the same
simplification to the other log calls referenced in these functions so messages
remain clear but not duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4767fb65-2b2c-47a2-9165-372984f7b946
📒 Files selected for processing (8)
app/src/components/notifications/NotificationCard.tsxapp/src/components/notifications/NotificationCenter.tsxapp/src/lib/notificationRouter.test.tsapp/src/lib/notificationRouter.tsapp/src/pages/Notifications.tsxapp/src/store/__tests__/notificationsSlice.test.tsapp/src/store/notificationSlice.tsapp/src/types/notifications.ts
✅ Files skipped from review due to trivial changes (1)
- app/src/types/notifications.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/pages/Notifications.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tauri-plugin-notification's.show()silently fails on macOS/Windows when permission has not been granted. AddedensureNotificationPermission()totauriBridge.tsthat checksisPermissionGrantedand callsrequestPermissionif needed. Called atstartNativeNotificationsService()time.socketService.on()wrapped every callback in a new closure butoff()passed the original reference tosocket.off()— which uses reference equality and therefore never matched. Listeners were never actually removed. Fixed by adding alistenerMap: Map<original, wrapped>to track the mapping;off()now looks up the wrapped reference, deletes from the map, and filters the pending queue.log::debug!to the Rustshow_native_notificationcommand to surface permission state and call metadata in logs.Test plan
pnpm typecheck— zero errorspnpm lint— zero errors (pre-existing warnings only)cargo check --manifest-path app/src-tauri/Cargo.toml— passesapp/test/e2e/specs/notifications.spec.tscovers:notification_ingestvia core RPC — creates a test notificationnotification_list— asserts it's returnednotification_mark_read— asserts status transitionnotification_stats— asserts aggregate statisticsbash app/scripts/e2e-run-spec.sh test/e2e/specs/notifications.spec.ts notificationsdocs/E2E-TESTING.md)Closes #932 #984
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation