feat(notifications): native OS banners for agent/chat/socket events (Phase 1c of #395)#780
Conversation
…nsai#395) Adds persistent notification center that captures events from the existing webview notification surface shipped in tinyhumansai#741. - notificationSlice (Redux + redux-persist) with categories + preferences - /notifications route + Notification Center page (read/unread, click-to-navigate, mark all, clear) - Bell tab in BottomTabBar with unread badge - NotificationsPanel in Settings → Features for per-category toggles (messages/agents/skills/system) - Webview notification handler dispatches into the center in addition to bumping account unread - Tests: 7 reducer cases covering prepend, preference gating, read flips, cap, unread count Next: PR B wires core-side native banners for agent/skill/channel events (tauri-plugin-notification).
…se 1c of tinyhumansai#395) Stacked on tinyhumansai#769. Adds OS-level notifications for events the frontend already sees over the existing socket.io + Tauri event surface, so the notification center earns its keep even when the app is backgrounded. - Tauri command `show_native_notification(title, body, tag?)` using the already-installed `tauri-plugin-notification` - Frontend `nativeNotifications` service subscribes to socket events: - `chat_done` → agents category - `chat_error` → system category - `disconnect` → system category - Only fires the OS banner when the window is NOT focused — in-app center covers the focused case - Respects per-category preferences from the settings panel - Vitest: 5 cases (dispatch, truncation, preference gating, focus gate, unfocused banner fire) Skipped the core→shell DomainEvent bridge for this PR: the Rust core runs in a separate process so piping DomainEvents into the Tauri shell would need a new event-relay transport. Existing socket events already carry agent completions, so this PR banner-izes them without new Rust plumbing. Bridge lands in a follow-up if we need cron/webhook/skill-sync banners too.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 45 minutes and 22 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request introduces a complete native notification system spanning backend and frontend. It adds a Tauri command for displaying OS-level notifications, a Redux-persisted notification store with preference management by category, a notifications page with unread count tracking, and services that integrate socket events and webview notifications into the centralized notification management layer. Changes
Sequence Diagram(s)sequenceDiagram
participant Socket as Socket Event
participant Service as Native Notifications<br/>Service
participant Redux as Redux Store<br/>(notificationSlice)
participant UI as Notifications Page<br/>& Badge
participant Native as Native OS<br/>Banner
Socket->>Service: chat_done / chat_error / webview:fired
Service->>Service: Normalize payload, construct item
Service->>Redux: Check preferences for category
alt Category enabled
Service->>Redux: Dispatch notificationReceived(item)
Redux->>UI: Update items & unread count
UI->>UI: Re-render notifications page & badge
alt Window not focused
Service->>Native: showNativeNotification(title, body)
Native-->>Service: Display OS banner
end
else Category disabled
Service->>Service: Drop notification (no dispatch)
end
sequenceDiagram
participant User as User
participant UI as Notifications UI<br/>(Page & Bottom Tab)
participant Redux as Redux Store
participant Storage as Persistent<br/>Storage
User->>UI: View notifications page
UI->>Redux: Select items & unread count
Redux->>Storage: Load from persisted notifications branch
Storage-->>Redux: Return items & preferences
Redux-->>UI: items[], unreadCount
UI->>UI: Render list with unread badge
User->>UI: Click notification item
UI->>Redux: Dispatch markRead(id)
Redux->>Storage: Persist updated read status
User->>UI: Access Settings > Notifications
UI->>Redux: Select preferences
User->>UI: Toggle category switch
UI->>Redux: Dispatch setPreference({category, enabled})
Redux->>Storage: Persist preference change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR introduces a multi-layered notification system spanning Tauri backend, Redux state, services, UI components, and integrations with existing webview notifications. While individual changes follow consistent patterns, reviewers must verify correct event flow, preference filtering logic, Redux state mutations, native banner invocation conditions (window focus, category enabled), item capping at 200, persistence configuration, deep link routing, and test coverage across heterogeneous file types. Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 5
🧹 Nitpick comments (9)
app/src/pages/Notifications.tsx (2)
32-41:useMemowrappingselectUnreadCountprovides no real benefit.
itemscomes straight from Redux; every reducer touching items returns a new array reference via Immer, so the memo invalidates on every notification-related state change and just adds overhead. The O(n≤200) reduce can be inlined without memoization, orselectUnreadCountcan be memoized upstream viareselectif it's ever needed elsewhere.- const unread = useMemo(() => selectUnreadCount(items), [items]); + const unread = selectUnreadCount(items);Can drop the
useMemoimport afterwards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Notifications.tsx` around lines 32 - 41, The useMemo wrapping selectUnreadCount in the Notifications component is unnecessary; change unread to compute directly from items (e.g., use selectUnreadCount(items) or inline the reduce) and remove the useMemo import; update the Notifications function where unread is defined (reference: Notifications, items, unread, selectUnreadCount, useMemo) and delete the now-unused useMemo import from the top of the file.
38-41:navigate(item.deepLink)trusts a string supplied by event payloads.
deepLinkvalues today are hard-coded (/chat,/accounts/...) by the notification producers innativeNotifications/service.tsandwebviewNotifications/service.ts, so this is fine in practice. Flagging defensively: if a future producer ever sourcesdeepLinkfrom a socket payload directly, a value likehttps://evil.exampleorjavascript:...would be handed toreact-router'snavigate, which will attempt to match it as a path. Consider gating onitem.deepLink.startsWith('/')before navigating.- if (item.deepLink) navigate(item.deepLink); + if (item.deepLink?.startsWith('/')) navigate(item.deepLink);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Notifications.tsx` around lines 38 - 41, handleClick trusts item.deepLink from NotificationItem and calls navigate(item.deepLink) directly; guard against unsafe or external URLs by validating item.deepLink is a string that starts with '/' before calling navigate (and still dispatch markRead via markRead({ id: item.id }) regardless). Update the handleClick function to check typeof item.deepLink === 'string' && item.deepLink.startsWith('/') and only call navigate when that condition is true; otherwise skip navigation (or log/handle invalid deepLink) to prevent external or protocol-based values from being passed to react-router's navigate.app/src/lib/nativeNotifications/service.ts (2)
68-76: Duplicated payload-handling logic betweenstartNativeNotificationsServiceand__handleChatDoneForTests.Both the real
chat_donelistener (lines 68-76) and the test helper (lines 103-110) construct the exact same notification item. If the id format, title, or body truncation changes in one place it's easy to miss the other, which defeats the point of the test. Extract a small helper and have both paths call it:+function buildChatDoneItem(p: ChatDonePayload) { + return { + id: `chat_done:${p.thread_id ?? 'unknown'}:${p.request_id ?? Date.now()}`, + title: 'Agent reply ready', + body: truncate(p.full_response?.trim() || 'Agent finished processing.', 160), + deepLink: '/chat', + } as const; +}Then both
socketService.on('chat_done', ...)and__handleChatDoneForTestscalldispatchAndMaybeBanner('agents', buildChatDoneItem(p)). Same applies to thechat_errorpath if you want a future test helper for it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/nativeNotifications/service.ts` around lines 68 - 76, The chat_done notification construction is duplicated between the socket listener and __handleChatDoneForTests; extract a helper (e.g., buildChatDoneItem or buildChatNotification) that accepts a ChatDonePayload and returns the notification object (id, title, body, deepLink) using the same id format and truncation logic, then replace the inline constructions in both socketService.on('chat_done', ...) and __handleChatDoneForTests to call dispatchAndMaybeBanner('agents', buildChatDoneItem(p)); apply the same pattern for chat_error later if desired.
53-56:truncateedge case:max <= 1drops the ellipsis or produces wrong length.With
max = 1,input.slice(0, 0) + '…'='…'(length 1, OK). Withmax = 0,input.slice(0, -1) + '…'mangles the string. Call sites use 160 and 80 so this is theoretical, but addingif (max <= 0) return ''would make it robust against future reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/nativeNotifications/service.ts` around lines 53 - 56, The truncate function can misbehave for non-positive max values (e.g., max === 0) because it uses input.slice(0, max - 1) unconditionally; update truncate to guard for max <= 0 by returning an empty string, and only compute the ellipsis branch when max > 0 (keep existing behavior for max === 1 returning '…' and for larger max returning input.slice(0, max - 1) + '…'). This change should be applied in the truncate function to make it robust for future reuse.app/src/lib/nativeNotifications/__tests__/service.test.ts (2)
3-19: Tests use the live productionstoresingleton.Importing the real
storefrom../../../storemeans every test in this file shares (and mutates) the same persisted Redux state, including any redux-persist rehydration side effects. ThebeforeEachmitigation (manualclearAll+setPreference) works, but it's brittle: any new slice reducer or initial-state change elsewhere can silently affect these tests, and parallel file execution could see cross-contamination.Consider constructing an isolated store per test via
configureStore({ reducer: { notifications: notificationReducer } })and injecting it (e.g. via a tinysetStoreForTestshook inservice.ts, matching the__resetForTestspattern already in place).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/nativeNotifications/__tests__/service.test.ts` around lines 3 - 19, Tests import the real shared store causing state leakage; instead add a test-only setter in the native notifications service (e.g., setStoreForTests(storeInstance) alongside __resetForTests) and in tests create an isolated store per test using configureStore({ reducer: { notifications: notificationReducer } }), pass that instance into setStoreForTests before each test, and use that store to dispatch setPreference and clear notifications so tests no longer rely on the production singleton store.
13-19: Usevi.restoreAllMocks()sodocument.hasFocusspies don't leak between tests.
vi.clearAllMocks()only resets.mock.calls/.mock.results; it does not undovi.spyOn(document, 'hasFocus'). After the focused/unfocused tests (lines 45-58) run, the spy stays installed globally for whatever happens next in this file (and, more importantly, for other test files sharing this jsdom environment if Vitest reuses it). Switching torestoreAllMocks()also removes the global store leakage risk if this file grows.beforeEach(() => { __resetForTests(); - vi.clearAllMocks(); + vi.restoreAllMocks(); // Clean slate for each test — clear any notifications persisted by prior ones. store.dispatch({ type: 'notifications/clearAll' }); store.dispatch(setPreference({ category: 'agents', enabled: true })); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/nativeNotifications/__tests__/service.test.ts` around lines 13 - 19, Replace the vi.clearAllMocks() call inside the beforeEach block with vi.restoreAllMocks() so any spies (e.g., vi.spyOn(document, 'hasFocus')) are fully removed between tests; update the beforeEach that currently calls __resetForTests(); vi.clearAllMocks(); to call __resetForTests(); vi.restoreAllMocks(); to ensure document.hasFocus and other spies do not leak across tests.app/src/components/settings/panels/NotificationsPanel.tsx (1)
57-70: Addtype="button"on the toggle for defensive correctness.The other two buttons in this PR (
Notifications.tsx— "Mark all read", "Clear") correctly settype="button". Matching that here prevents accidental form submission if this panel is ever embedded inside a<form>in the future.<button + type="button" onClick={() => handleToggle(cat.id)} className={...} role="switch"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/NotificationsPanel.tsx` around lines 57 - 70, The toggle button in NotificationsPanel.tsx (the button that calls handleToggle(cat.id) and uses aria-label `Toggle ${cat.title} notifications`) lacks an explicit type, which can cause accidental form submission if rendered inside a form; add type="button" to that button element so it behaves as a non-submit control (matching the other buttons like the "Mark all read"/"Clear" buttons).app/src-tauri/src/lib.rs (1)
460-465: Nit: log label saystitle_chars/body_charsbut reports byte length.
String::len()returns the UTF-8 byte count, not the character count. For diagnostic logs this is harmless, but the label is misleading for multi-byte titles (e.g., CJK / emoji). Either rename totitle_bytes/body_bytesor call.chars().count().Proposed tweak
- log::debug!( - "[notify] show_native_notification title_chars={} body_chars={} tag={:?}", - title.len(), - body.len(), - tag - ); + log::debug!( + "[notify] show_native_notification title_bytes={} body_bytes={} tag={:?}", + title.len(), + body.len(), + tag + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src-tauri/src/lib.rs` around lines 460 - 465, The log labels `title_chars`/`body_chars` are misleading because title.len()/body.len() return UTF-8 byte counts; update the debug call in the notify code (the log::debug! invocation inside the native notification helper, e.g., the show_native_notification function) to either use `.chars().count()` for `title` and `body` if you want true character counts or rename the labels to `title_bytes`/`body_bytes`; specifically replace `title.len()` and `body.len()` with `title.chars().count()` and `body.chars().count()` to keep the existing label names, and leave `tag` as-is.app/src/store/__tests__/notificationSlice.test.ts (1)
1-72: LGTM — covers the reducer surface well.Prepend ordering, preference gating,
markRead/markAllRead,clearAll, selector, and the 200-item cap are all exercised deterministically with no timing or I/O dependencies.Minor optional nit: if
MAX_ITEMSis ever tuned, the literal200/'209'in the cap test will silently go stale — consider importingMAX_ITEMSfrom the slice (if exported) or computing the expected head id from a single constant at the top of the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/store/__tests__/notificationSlice.test.ts` around lines 1 - 72, The cap test hardcodes 200 and '209' which will break if MAX_ITEMS changes; update the test to reference the slice's MAX_ITEMS constant (export it from notificationSlice) or compute expected length/index from a single test constant (e.g., const MAX = <imported or locally set>), then replace the literal 200 and '209' assertions to use MAX and computed head id; locate the cap test using symbols reducer and notificationReceived in notificationSlice.test.ts and change the expectations to use MAX_ITEMS (or the test constant) so the test stays correct when the cap is tuned.
🤖 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-tauri/src/lib.rs`:
- Around line 466-473: The notification command currently logs the
Option<String> tag but never forwards it to the NotificationBuilder; update the
builder chain returned from app.notification().builder() to call .group(tag)
when the tag is Some(...) (i.e., convert the Option<String> tag into a string
and call builder = builder.group(tag_val) before building), using the
NotificationBuilder API to enable deduplication, and then keep the existing body
handling and .show() error mapping; additionally register this command in the
Tauri command list and add a short entry documenting the new tag behavior in
docs/src-tauri/02-commands.md so the frontend usage is recorded.
In `@app/src/lib/nativeNotifications/service.ts`:
- Around line 98-100: stopNativeNotificationsService currently only flips the
started flag but leaves socketService listeners registered; modify
stopNativeNotificationsService to remove the handlers it added in start (e.g.
call socketService.off('chat_done', chatDoneHandler),
socketService.off('chat_error', chatErrorHandler),
socketService.off('disconnect', disconnectHandler)) — if those handlers are
created inline in startNativeNotificationsService, hoist them to module-level
named functions or store their references in module-scoped variables so
stopNativeNotificationsService can call socketService.off(...) to unregister the
same function references and prevent listener buildup on repeated start/stop
cycles.
- Around line 88-95: The disconnect handler on socketService currently emits a
new "Connection lost" banner per transport blip (socketService.on('disconnect'))
using a unique id with Date.now(), causing spam; change it to debounce/gate
notifications by starting a timeout (e.g., N seconds) in the disconnect handler
and only call dispatchAndMaybeBanner if no 'connect'/'reconnect' arrives before
the timeout, clear that timeout in socketService.on('connect' or 'reconnect'),
reuse a stable dedupe id like "socket_disconnect" instead of Date.now(), and
filter out benign reasons (e.g., "io client disconnect", "transport close")
before scheduling the notification; keep using truncate for the reason text when
dispatching.
In `@app/src/lib/webviewNotifications/service.ts`:
- Around line 72-84: The dispatched notification uses a hardcoded deepLink
(`/accounts/${accountId}`) and always pushes to the center without checking
message notification preferences; update the logic around
store.dispatch(notificationReceived(...)) to (1) consult the user's message
notification preference (use the existing notification preference selector or
service) and only dispatch if messages are enabled, and (2) construct the
deepLink using the app's chat route helper (e.g., AppRoutes or the existing
getChatRoute/getConversationRoute helper) including accountId and tag so the
click lands in the current chat, falling back to a safe route if the helper is
unavailable; keep the same payload shape and id generation but replace the
deepLink and gate the dispatch on the preference check.
In `@app/src/store/notificationSlice.ts`:
- Around line 40-47: notificationReceived can insert duplicate notifications
with the same id; add O(1) deduplication by tracking ids in a Set on the slice
state (e.g., state.itemIds: Set<string>). In notificationReceived, return early
if state.itemIds.has(item.id); otherwise unshift item,
state.itemIds.add(item.id), and when trimming to MAX_ITEMS remove the
popped/removed items' ids from state.itemIds; also update other reducers that
modify state.items (e.g., markRead, any remove/clear handlers) to keep
state.itemIds in sync.
---
Nitpick comments:
In `@app/src-tauri/src/lib.rs`:
- Around line 460-465: The log labels `title_chars`/`body_chars` are misleading
because title.len()/body.len() return UTF-8 byte counts; update the debug call
in the notify code (the log::debug! invocation inside the native notification
helper, e.g., the show_native_notification function) to either use
`.chars().count()` for `title` and `body` if you want true character counts or
rename the labels to `title_bytes`/`body_bytes`; specifically replace
`title.len()` and `body.len()` with `title.chars().count()` and
`body.chars().count()` to keep the existing label names, and leave `tag` as-is.
In `@app/src/components/settings/panels/NotificationsPanel.tsx`:
- Around line 57-70: The toggle button in NotificationsPanel.tsx (the button
that calls handleToggle(cat.id) and uses aria-label `Toggle ${cat.title}
notifications`) lacks an explicit type, which can cause accidental form
submission if rendered inside a form; add type="button" to that button element
so it behaves as a non-submit control (matching the other buttons like the "Mark
all read"/"Clear" buttons).
In `@app/src/lib/nativeNotifications/__tests__/service.test.ts`:
- Around line 3-19: Tests import the real shared store causing state leakage;
instead add a test-only setter in the native notifications service (e.g.,
setStoreForTests(storeInstance) alongside __resetForTests) and in tests create
an isolated store per test using configureStore({ reducer: { notifications:
notificationReducer } }), pass that instance into setStoreForTests before each
test, and use that store to dispatch setPreference and clear notifications so
tests no longer rely on the production singleton store.
- Around line 13-19: Replace the vi.clearAllMocks() call inside the beforeEach
block with vi.restoreAllMocks() so any spies (e.g., vi.spyOn(document,
'hasFocus')) are fully removed between tests; update the beforeEach that
currently calls __resetForTests(); vi.clearAllMocks(); to call
__resetForTests(); vi.restoreAllMocks(); to ensure document.hasFocus and other
spies do not leak across tests.
In `@app/src/lib/nativeNotifications/service.ts`:
- Around line 68-76: The chat_done notification construction is duplicated
between the socket listener and __handleChatDoneForTests; extract a helper
(e.g., buildChatDoneItem or buildChatNotification) that accepts a
ChatDonePayload and returns the notification object (id, title, body, deepLink)
using the same id format and truncation logic, then replace the inline
constructions in both socketService.on('chat_done', ...) and
__handleChatDoneForTests to call dispatchAndMaybeBanner('agents',
buildChatDoneItem(p)); apply the same pattern for chat_error later if desired.
- Around line 53-56: The truncate function can misbehave for non-positive max
values (e.g., max === 0) because it uses input.slice(0, max - 1)
unconditionally; update truncate to guard for max <= 0 by returning an empty
string, and only compute the ellipsis branch when max > 0 (keep existing
behavior for max === 1 returning '…' and for larger max returning input.slice(0,
max - 1) + '…'). This change should be applied in the truncate function to make
it robust for future reuse.
In `@app/src/pages/Notifications.tsx`:
- Around line 32-41: The useMemo wrapping selectUnreadCount in the Notifications
component is unnecessary; change unread to compute directly from items (e.g.,
use selectUnreadCount(items) or inline the reduce) and remove the useMemo
import; update the Notifications function where unread is defined (reference:
Notifications, items, unread, selectUnreadCount, useMemo) and delete the
now-unused useMemo import from the top of the file.
- Around line 38-41: handleClick trusts item.deepLink from NotificationItem and
calls navigate(item.deepLink) directly; guard against unsafe or external URLs by
validating item.deepLink is a string that starts with '/' before calling
navigate (and still dispatch markRead via markRead({ id: item.id }) regardless).
Update the handleClick function to check typeof item.deepLink === 'string' &&
item.deepLink.startsWith('/') and only call navigate when that condition is
true; otherwise skip navigation (or log/handle invalid deepLink) to prevent
external or protocol-based values from being passed to react-router's navigate.
In `@app/src/store/__tests__/notificationSlice.test.ts`:
- Around line 1-72: The cap test hardcodes 200 and '209' which will break if
MAX_ITEMS changes; update the test to reference the slice's MAX_ITEMS constant
(export it from notificationSlice) or compute expected length/index from a
single test constant (e.g., const MAX = <imported or locally set>), then replace
the literal 200 and '209' assertions to use MAX and computed head id; locate the
cap test using symbols reducer and notificationReceived in
notificationSlice.test.ts and change the expectations to use MAX_ITEMS (or the
test constant) so the test stays correct when the cap is tuned.
🪄 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: 2ab39c66-6dd7-49f1-b1df-f8f3382225c5
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
app/src-tauri/src/lib.rsapp/src-tauri/src/webview_accounts/mod.rsapp/src/App.tsxapp/src/AppRoutes.tsxapp/src/components/BottomTabBar.tsxapp/src/components/settings/panels/NotificationsPanel.tsxapp/src/lib/nativeNotifications/__tests__/service.test.tsapp/src/lib/nativeNotifications/index.tsapp/src/lib/nativeNotifications/service.tsapp/src/lib/nativeNotifications/tauriBridge.tsapp/src/lib/webviewNotifications/service.tsapp/src/pages/Notifications.tsxapp/src/pages/Settings.tsxapp/src/store/__tests__/notificationSlice.test.tsapp/src/store/index.tsapp/src/store/notificationSlice.ts
| let mut builder = app.notification().builder().title(&title); | ||
| if !body.is_empty() { | ||
| builder = builder.body(&body); | ||
| } | ||
| builder | ||
| .show() | ||
| .map_err(|e| format!("notification show failed: {e}")) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
tauri-plugin-notification NotificationBuilder API set tag or identifier to dedupe
💡 Result:
In the official tauri-plugin-notification (tauri-apps), the NotificationBuilder API in Rust uses .id(i32) to set the notification identifier, which serves as the unique identifier for deduplication—using the same ID replaces or groups existing notifications depending on the platform's native behavior. In JavaScript/guest bindings, sendNotification accepts an optional numeric 'id' for the same purpose. Note there are two plugins: the official tauri-plugin-notification (with NotificationBuilder.id) and a third-party tauri-plugin-notifications (with NotificationsBuilder.id and optional 'tag?: string' in removeActive for more flexible identification).
Citations:
- 1: https://docs.rs/tauri-plugin-notification/latest/tauri_plugin_notification/struct.NotificationBuilder.html
- 2: https://docs.rs/tauri-plugin-notifications/latest/tauri_plugin_notifications/struct.NotificationsBuilder.html
- 3: https://tauri.app/reference/javascript/notification/
- 4: https://github.com/Choochmeque/tauri-plugin-notifications
- 5: https://v2.tauri.app/reference/javascript/notification
🏁 Script executed:
# First, find and read the relevant function in lib.rs
cd app/src-tauri/src
wc -l lib.rsRepository: tinyhumansai/openhuman
Length of output: 76
🏁 Script executed:
# Read the function that contains the notification code (around lines 460-480)
sed -n '450,490p' app/src-tauri/src/lib.rsRepository: tinyhumansai/openhuman
Length of output: 1390
🏁 Script executed:
# Check the function signature by looking further back
sed -n '430,475p' app/src-tauri/src/lib.rsRepository: tinyhumansai/openhuman
Length of output: 1538
🏁 Script executed:
# Check Cargo.toml for notification plugin dependency
cat app/src-tauri/Cargo.toml | grep -A 5 notificationRepository: tinyhumansai/openhuman
Length of output: 1537
🏁 Script executed:
# Also check if there's a lock file that might show the exact version
cd app/src-tauri
if [ -f Cargo.lock ]; then grep -A 3 "name = \"tauri-plugin-notification" Cargo.lock; fiRepository: tinyhumansai/openhuman
Length of output: 284
🏁 Script executed:
# Check the NotificationBuilder API for version 2.3.3
# Look at the tauri-plugin-notification source or docs
curl -s "https://docs.rs/tauri-plugin-notification/2.3.3/tauri_plugin_notification/struct.NotificationBuilder.html" | grep -i "fn " | head -20Repository: tinyhumansai/openhuman
Length of output: 14484
🏁 Script executed:
# Check if the command is documented per coding guidelines
if [ -f docs/src-tauri/02-commands.md ]; then
echo "=== File exists ==="
grep -n "show_native_notification\|notification" docs/src-tauri/02-commands.md | head -20
else
echo "File not found"
fiRepository: tinyhumansai/openhuman
Length of output: 85
🏁 Script executed:
# Search in the repo for how NotificationBuilder is used elsewhere
rg "notification.*builder" -A 3 -B 1 app/src-tauri/src/Repository: tinyhumansai/openhuman
Length of output: 691
🏁 Script executed:
# Check if there are any other uses of the notification builder to see what methods are available
rg "\.id\(|\.tag\(|\.group\(" app/src-tauri/src/Repository: tinyhumansai/openhuman
Length of output: 234
Forward the tag parameter to the notification builder for proper deduplication.
The command accepts an Option<String> tag (used by the frontend to dedupe/replace notifications) but logs it without passing it to the builder. The NotificationBuilder API (v2.3.3) provides .group(String) for this purpose—using the same group identifier will collapse repeat notifications instead of stacking them across platforms.
Additionally, this command should be registered and documented in docs/src-tauri/02-commands.md per the Tauri guidelines.
Proposed change
let mut builder = app.notification().builder().title(&title);
if !body.is_empty() {
builder = builder.body(&body);
}
+ if let Some(tag) = tag.as_deref().filter(|t| !t.is_empty()) {
+ builder = builder.group(tag);
+ }
builder
.show()
.map_err(|e| format!("notification show failed: {e}"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/lib.rs` around lines 466 - 473, The notification command
currently logs the Option<String> tag but never forwards it to the
NotificationBuilder; update the builder chain returned from
app.notification().builder() to call .group(tag) when the tag is Some(...)
(i.e., convert the Option<String> tag into a string and call builder =
builder.group(tag_val) before building), using the NotificationBuilder API to
enable deduplication, and then keep the existing body handling and .show() error
mapping; additionally register this command in the Tauri command list and add a
short entry documenting the new tag behavior in docs/src-tauri/02-commands.md so
the frontend usage is recorded.
| socketService.on('disconnect', (...args: unknown[]) => { | ||
| const reason = typeof args[0] === 'string' ? args[0] : 'unknown'; | ||
| dispatchAndMaybeBanner('system', { | ||
| id: `socket_disconnect:${Date.now()}`, | ||
| title: 'Connection lost', | ||
| body: `OpenHuman lost its connection to the core service (${truncate(reason, 80)}).`, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
disconnect notifications will spam on flaky networks.
socket.io-client emits disconnect on every transport blip (plus transparent reconnects), so a user on an unstable Wi-Fi link can get a chain of "Connection lost" banners / notification-center entries — one per blip. Also, every id uses Date.now(), so the dedupe key is unique per emission, and the category is system which is enabled by default.
Consider (a) debouncing / gating so we only notify after the socket has been disconnected for N seconds without a reconnect, and/or (b) reusing a stable id (e.g. socket_disconnect) so repeated disconnects collapse via the dedupe the slice should be doing. Also consider filtering benign reasons like "io client disconnect" / "transport close" during normal sign-out/app-close flows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/nativeNotifications/service.ts` around lines 88 - 95, The
disconnect handler on socketService currently emits a new "Connection lost"
banner per transport blip (socketService.on('disconnect')) using a unique id
with Date.now(), causing spam; change it to debounce/gate notifications by
starting a timeout (e.g., N seconds) in the disconnect handler and only call
dispatchAndMaybeBanner if no 'connect'/'reconnect' arrives before the timeout,
clear that timeout in socketService.on('connect' or 'reconnect'), reuse a stable
dedupe id like "socket_disconnect" instead of Date.now(), and filter out benign
reasons (e.g., "io client disconnect", "transport close") before scheduling the
notification; keep using truncate for the reason text when dispatching.
| store.dispatch( | ||
| notificationReceived({ | ||
| id: `${accountId}:${tag ?? ''}:${Date.now()}`, | ||
| category: 'messages', | ||
| title, | ||
| body, | ||
| timestamp: Date.now(), | ||
| read: false, | ||
| accountId, | ||
| provider, | ||
| deepLink: `/accounts/${accountId}`, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Use the current chat route and honor message notification preferences.
/accounts/${accountId} is not registered in AppRoutes; webview notification clicks will miss the intended destination. This dispatch also bypasses the messages category preference before adding items to the notification center.
Proposed fix
+ const preferences = store.getState().notifications.preferences;
+ if (!preferences.messages) {
+ log('messages category disabled, skipping notification center item');
+ return;
+ }
+ const now = Date.now();
store.dispatch(
notificationReceived({
- id: `${accountId}:${tag ?? ''}:${Date.now()}`,
+ id: `${accountId}:${tag ?? ''}:${now}`,
category: 'messages',
title,
body,
- timestamp: Date.now(),
+ timestamp: now,
read: false,
accountId,
provider,
- deepLink: `/accounts/${accountId}`,
+ deepLink: '/chat',
})
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/webviewNotifications/service.ts` around lines 72 - 84, The
dispatched notification uses a hardcoded deepLink (`/accounts/${accountId}`) and
always pushes to the center without checking message notification preferences;
update the logic around store.dispatch(notificationReceived(...)) to (1) consult
the user's message notification preference (use the existing notification
preference selector or service) and only dispatch if messages are enabled, and
(2) construct the deepLink using the app's chat route helper (e.g., AppRoutes or
the existing getChatRoute/getConversationRoute helper) including accountId and
tag so the click lands in the current chat, falling back to a safe route if the
helper is unavailable; keep the same payload shape and id generation but replace
the deepLink and gate the dispatch on the preference check.
| notificationReceived(state, action: PayloadAction<NotificationItem>) { | ||
| const item = action.payload; | ||
| if (!state.preferences[item.category]) return; | ||
| state.items.unshift(item); | ||
| if (state.items.length > MAX_ITEMS) { | ||
| state.items.length = MAX_ITEMS; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Consider deduplicating by id in notificationReceived.
service.ts uses deterministic ids such as chat_done:${thread_id}:${request_id} (with Date.now() fallback when either is missing). If the socket ever re-delivers the same chat_done payload (reconnection replay, server retry), or if two events share the same fallback timestamp within a ms, the slice will store multiple entries with the same id, making the UI show duplicates and markRead({id}) only flip the first. A simple O(1) dedupe check prevents this:
notificationReceived(state, action: PayloadAction<NotificationItem>) {
const item = action.payload;
if (!state.preferences[item.category]) return;
+ // Dedupe: if we already have this id, leave the existing (likely
+ // already-read) entry in place rather than unshifting a duplicate.
+ if (state.items.some(i => i.id === item.id)) return;
state.items.unshift(item);
if (state.items.length > MAX_ITEMS) {
state.items.length = MAX_ITEMS;
}
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/store/notificationSlice.ts` around lines 40 - 47,
notificationReceived can insert duplicate notifications with the same id; add
O(1) deduplication by tracking ids in a Set on the slice state (e.g.,
state.itemIds: Set<string>). In notificationReceived, return early if
state.itemIds.has(item.id); otherwise unshift item, state.itemIds.add(item.id),
and when trimming to MAX_ITEMS remove the popped/removed items' ids from
state.itemIds; also update other reducers that modify state.items (e.g.,
markRead, any remove/clear handlers) to keep state.itemIds in sync.
…mansai#780) - Resolve merge conflict in NotificationsPanel.tsx: take main-branch superset that includes DnD toggle + categories sections - Resolve merge conflict in store/index.ts: preserve both notification slices (notifications + integrationNotifications) - Resolve merge conflict in Settings.tsx: add NotificationRoutingPanel import and route from main - Fix stopNativeNotificationsService to capture listener refs and call socketService.off() on teardown — previously leaked all three listeners - Fix duplicate Date.now() in webviewNotifications/service.ts: use a single `now` variable so id and timestamp are guaranteed consistent - Remove dead `const providers = allProviders` alias in NotificationCenter - Add `off: vi.fn()` to socketService mock in service.test.ts to cover the new stop path - Replace invisible bg-transparent placeholder span in NotificationCard with conditional render; keep w-2 container for alignment
3 participants
Summary
Stacked on #769. Adds OS-level notification banners for events the frontend already hears over socket.io, so the notification center from PR A earns its keep even when the app is backgrounded.
show_native_notification(title, body, tag?)wrapping the already-installedtauri-plugin-notificationnativeNotificationsservice subscribes at boot and routes:chat_done→ agents category + banner when unfocusedchat_error→ system category + bannerdisconnect→ system category + bannerOut of scope
Skipped the core→shell DomainEvent bridge. The Rust core runs in a separate process, so piping DomainEvents (cron completions, webhook requests, skill sync) into the Tauri shell needs a new event relay. Socket events already cover agent completions, which is the big one — bridge can ship as Phase 1d if we want cron/webhook banners too.
Merge order
This branch is stacked on #769. After #769 merges, GitHub will auto-rebase this onto main.
Test plan
tsc --noEmitcleaneslintcleancargo checkclean (warnings only, all in vendored tauri-cef)Summary by CodeRabbit