test(notifications): add Phase 4 RPC and dismiss reducer coverage#878
Merged
Conversation
…ith dedup Wire CDP-migrated providers (Slack, WhatsApp, Discord, Telegram) into the core triage pipeline by calling ingestNotification in handleFired after the existing Redux dispatches. Add a 60-second content-hash deduplication check (exists_recent) in the Rust store, invoke it at the top of handle_ingest to drop duplicate fires, and standardise observability prefixes to [notification_intel] throughout rpc.rs.
…bus integration - Add NotificationStats struct to types.rs for aggregate pipeline metrics - Add mark_dismissed, mark_acted, stats functions to store.rs with unit tests - Add NotificationIngested and NotificationTriaged variants to DomainEvent with domain() routing and test coverage - Publish NotificationIngested after ingest and NotificationTriaged after triage scoring in rpc.rs - Add handle_dismiss, handle_mark_acted, handle_stats RPC handlers to rpc.rs - Register 3 new controllers (dismiss, mark_acted, stats) in schemas.rs; update counts to 8 - Add NotificationTriaged match arm in bus.rs to surface escalate/react events as Agents notifications; add 2 bus tests
…date Notifications page Merge notificationsSlice.ts into notificationSlice.ts (integration fields under integrationItems/integrationUnreadCount/etc.), remove the separate slice and store key, update all import sites. Add dismiss button to NotificationCard, dismissNotification/markNotificationActed/fetchNotificationStats to notificationService, NotificationStats type, pipeline stats card in NotificationRoutingPanel, and render NotificationCenter above system events on the /notifications page.
Add JSON-RPC E2E coverage for ingest/list roundtrip, dismiss/mark_acted flows, stats assertions, and async triage progress, plus focused dismiss reducer tests for integration notifications. Made-with: Cursor
Apply rustfmt-driven line wraps in the new notification JSON-RPC E2E coverage so pre-push format checks pass consistently. Made-with: Cursor
Contributor
📝 WalkthroughWalkthroughThe PR updates the notification system across multiple layers: the frontend NotificationCenter component now derives notification data from integration-specific Redux state and dispatches corresponding actions; a new test suite validates the dismiss functionality; the backend store refactors transaction handling with explicit BEGIN IMMEDIATE and COMMIT/ROLLBACK semantics; and e2e tests are extended with conditional message formatting for triage-formatted content. Changes
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 |
Make ingest dedup atomic in a single transaction, compare recent-window timestamps by epoch, and redact account identifiers from debug logs while adding regression coverage for stale duplicate detection. Made-with: Cursor
![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/components/notifications/NotificationCard.tsx (1)
60-117:⚠️ Potential issue | 🟠 MajorFix nested
<button>elements that violate HTML structure and break accessibility.The outer card container (line 60) is a
<button>, and the dismiss control (line 101) is a nested<button>within it. This violates the HTML content model —<button>elements cannot contain interactive descendants — and causes unpredictable keyboard and screen-reader behavior.🐛 Suggested fix (separate card interaction from dismiss button)
- <button + <div + role="button" + tabIndex={0} onClick={() => { if (isUnread) onMarkRead(n.id); }} + onKeyDown={e => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + if (isUnread) onMarkRead(n.id); + } + }} className={`w-full text-left p-3 border-b border-stone-100 hover:bg-stone-50 transition-colors duration-150 focus:outline-none focus-visible:ring-2 focus-visible:ring-primary-500 focus-visible:ring-offset-1 ${ isUnread ? 'bg-primary-50/30' : 'bg-white' }`}> @@ - <button + <button + type="button" onClick={e => { e.stopPropagation(); + e.preventDefault(); onDismiss(n.id); }} className="ml-1 flex-shrink-0 p-0.5 rounded hover:bg-stone-200 text-stone-400 hover:text-stone-600 transition-colors" aria-label="Dismiss notification"> @@ - </button> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/notifications/NotificationCard.tsx` around lines 60 - 117, The outer card is a <button> that contains another interactive <button> for dismiss, which is invalid HTML; change the outer interactive container (the top-level JSX button that calls onMarkRead) into a non-nested interactive element (e.g., replace it with a <div> or <li> with role="button" and tabIndex={0}) and wire the same onClick handler to it (call onMarkRead(n.id) when activated) plus keyboard handlers (onKeyDown to handle Enter/Space) and the existing focus/visible styling so it remains accessible; keep the inner dismiss control as a real <button> that stops propagation (the onDismiss call and e.stopPropagation() should remain unchanged) so the dismiss action works independently of the card activation.
🧹 Nitpick comments (4)
app/src/components/settings/panels/NotificationRoutingPanel.tsx (1)
33-37: Use namespaceddebuglogging instead ofconsole.warnfor stats fetch failures.Line 36 should follow the panel/frontend logging convention so logs are filterable and environment-safe.
♻️ Suggested change
+import debug from 'debug'; import { useEffect, useState } from 'react'; @@ +const log = debug('settings:notification-routing'); + @@ useEffect(() => { void fetchNotificationStats() .then(s => setStats(s)) - .catch(err => console.warn('[settings][notification-routing] stats load failed', err)); + .catch(err => log('stats load failed %o', err)); }, []);As per coding guidelines, "Frontend debug logging should use namespaced
debugcalls with dev-only detail."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/NotificationRoutingPanel.tsx` around lines 33 - 37, The catch in the useEffect inside NotificationRoutingPanel uses console.warn; replace it with a namespaced debug logger (e.g., create a debug instance like debug('settings:notification-routing') or use the app's debug helper) and call that logger with the descriptive message and the error (so the call mirrors the current args but uses debug instead of console.warn); update the import/initialization accordingly near the top of the file and keep the message "[settings][notification-routing] stats load failed" with the error passed through.app/src/services/notificationService.ts (1)
138-145: Add the missing error-path logging forfetchNotificationStats.This is the only new notification RPC wrapper that skips the
try/catch+notifications:errorpath, so stats failures won't show up in the existing debug namespace.Suggested fix
export async function fetchNotificationStats(): Promise<NotificationStats> { log('fetchNotificationStats'); - const result = await callCoreRpc<NotificationStats>({ - method: 'openhuman.notification_stats', - params: {}, - }); - log('fetchNotificationStats result: total=%d unread=%d', result.total, result.unread); - return result; + try { + const result = await callCoreRpc<NotificationStats>({ + method: 'openhuman.notification_stats', + params: {}, + }); + log('fetchNotificationStats result: total=%d unread=%d', result.total, result.unread); + return result; + } catch (err) { + errLog('fetchNotificationStats failed: %o', err); + throw err; + } }As per coding guidelines: "Frontend debug logging should use namespaced
debugcalls with dev-only detail. Log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/notificationService.ts` around lines 138 - 145, fetchNotificationStats currently lacks the error-path logging and bypasses the standard notifications:error debug namespace; wrap the callCoreRpc invocation in a try/catch inside fetchNotificationStats, on catch emit a namespaced debug/error log (notifications:error) including the caught error and context (e.g., method name and params), then rethrow the error so callers still observe failures; ensure you reference fetchNotificationStats and callCoreRpc when making the change so the error path mirrors other notification RPC wrappers.src/openhuman/notifications/store.rs (2)
304-386: Same log prefix inconsistency instatsfunction.The
statsfunction also uses[notification_intel]while the rest of the store module uses[notifications::store]. Consider aligning for consistency.Otherwise, the aggregation logic is correct — total/unread/unscored counts and per-provider/per-action groupings are properly computed with scoped statement blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/notifications/store.rs` around lines 304 - 386, The log prefix in the stats function is inconsistent: within fn stats the tracing and error context strings use "[notification_intel]" but the rest of the module uses "[notifications::store]"; update all occurrences of the "[notification_intel]" literal in stats (including context messages on query_row/prepare/query_map and the tracing::debug call) to use "[notifications::store]" so logging/context is consistent with the store module and types::NotificationStats.
268-284: Inconsistent log prefix within store module.The new
mark_dismissedandmark_actedfunctions use[notification_intel]prefix, while existing functions in this file (insert,list,update_triage,mark_read, etc.) use[notifications::store]. This makes grep-based log filtering inconsistent within the same module. As per coding guidelines, logging must use stable grep-friendly prefixes.Proposed fix: align with existing prefix
) - .context("[notification_intel] mark_dismissed failed")?; + .context("[notifications::store] mark_dismissed failed")?; if updated == 0 { - tracing::warn!(id = %id, "[notification_intel] mark_dismissed matched no rows"); + tracing::warn!(id = %id, "[notifications::store] mark_dismissed matched no rows"); } else { - tracing::debug!(id = %id, "[notification_intel] mark_dismissed applied"); + tracing::debug!(id = %id, "[notifications::store] mark_dismissed applied"); }Apply the same pattern to
mark_acted(lines 294, 296, 298).Also applies to: 286-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/notifications/store.rs` around lines 268 - 284, The logging prefix in mark_dismissed and mark_acted is inconsistent with the rest of this module; update all tracing messages in the mark_dismissed and mark_acted functions to use the existing grep-friendly prefix "[notifications::store]" instead of "[notification_intel]" (i.e., change the tracing::warn!, tracing::debug!, and any other tracing calls inside the mark_dismissed and mark_acted functions to use "[notifications::store]" so they match insert, list, update_triage, mark_read, etc.).
🤖 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/components/notifications/NotificationCenter.tsx`:
- Around line 24-28: NotificationCenter is rendering integrationItems directly
(from useAppSelector s => s.notifications) which mixes provider-scoped fetch
results with global live inserts and doesn't remove dismissed rows; reapply
provider and status filtering inside the NotificationCenter component before
mapping/rendering (and immediately after handling addIntegrationNotification or
dismiss actions) so the UI only shows items matching the current provider filter
and excludes items with status dismissed. Concretely: derive a filteredItems
array from integrationItems by applying the current provider filter and
excluding items where status === 'dismissed' (or the reducer's dismissed enum),
use filteredItems in all render paths (the mappings referenced in
NotificationCenter render blocks and handlers that currently use
integrationItems), and ensure any local handlers that update items (e.g.,
addIntegrationNotification, the dismiss handler) update the selector-derived
list or recompute filteredItems so the UI updates correctly.
In `@src/openhuman/notifications/bus.rs`:
- Around line 146-154: The triage notification body in CoreNotificationEvent
(constructed in bus.rs: title/body when action == "escalate" || "react")
unconditionally states "Routed to orchestrator" though routing is conditional in
the routing logic in src/openhuman/notifications/rpc.rs; change the notification
to either reflect the actual routing decision or use a neutral phrasing (e.g.,
"May be routed to orchestrator") and/or pass a routing flag from the routing
logic into the CoreNotificationEvent so the body is generated based on that flag
(refer to symbols: CoreNotificationEvent, CoreNotificationCategory::Agents,
title, body, action, importance_score, provider and the routing decision in
rpc.rs).
In `@src/openhuman/notifications/schemas.rs`:
- Around line 289-300: The ControllerSchema for "stats" currently advertises
only a "total" output but the runtime RPC (handle_stats returning
NotificationStats) includes additional fields; update the "stats"
ControllerSchema (namespace: "notification", function: "stats") outputs to
mirror NotificationStats by adding fields for "unread" and "unscored" (I64,
required true) and the aggregated maps "by_provider" and "by_action" with
appropriate map/object TypeSchema entries matching how NotificationStats
represents them; ensure the output field names and types exactly match the
NotificationStats struct so consumers of controller metadata see the same
payload as handle_stats.
In `@tests/json_rpc_e2e.rs`:
- Around line 2499-2540: The test currently allows triage to be false so it only
verifies ingest visibility; change it to require triage completion by asserting
triaged is true after the poll loop. Locate the polling block around the
variables triaged, observed_pending_or_triaged and the notification_list call
(post_json_rpc + assert_no_jsonrpc_error using ingested_id), remove the
conditional "if triaged { assert!(triaged, ...); }" and replace it with a direct
assertion assert!(triaged, "triage completion should populate score/action")
(optionally increase the retry loop count or timeout if flakiness is a concern).
---
Outside diff comments:
In `@app/src/components/notifications/NotificationCard.tsx`:
- Around line 60-117: The outer card is a <button> that contains another
interactive <button> for dismiss, which is invalid HTML; change the outer
interactive container (the top-level JSX button that calls onMarkRead) into a
non-nested interactive element (e.g., replace it with a <div> or <li> with
role="button" and tabIndex={0}) and wire the same onClick handler to it (call
onMarkRead(n.id) when activated) plus keyboard handlers (onKeyDown to handle
Enter/Space) and the existing focus/visible styling so it remains accessible;
keep the inner dismiss control as a real <button> that stops propagation (the
onDismiss call and e.stopPropagation() should remain unchanged) so the dismiss
action works independently of the card activation.
---
Nitpick comments:
In `@app/src/components/settings/panels/NotificationRoutingPanel.tsx`:
- Around line 33-37: The catch in the useEffect inside NotificationRoutingPanel
uses console.warn; replace it with a namespaced debug logger (e.g., create a
debug instance like debug('settings:notification-routing') or use the app's
debug helper) and call that logger with the descriptive message and the error
(so the call mirrors the current args but uses debug instead of console.warn);
update the import/initialization accordingly near the top of the file and keep
the message "[settings][notification-routing] stats load failed" with the error
passed through.
In `@app/src/services/notificationService.ts`:
- Around line 138-145: fetchNotificationStats currently lacks the error-path
logging and bypasses the standard notifications:error debug namespace; wrap the
callCoreRpc invocation in a try/catch inside fetchNotificationStats, on catch
emit a namespaced debug/error log (notifications:error) including the caught
error and context (e.g., method name and params), then rethrow the error so
callers still observe failures; ensure you reference fetchNotificationStats and
callCoreRpc when making the change so the error path mirrors other notification
RPC wrappers.
In `@src/openhuman/notifications/store.rs`:
- Around line 304-386: The log prefix in the stats function is inconsistent:
within fn stats the tracing and error context strings use "[notification_intel]"
but the rest of the module uses "[notifications::store]"; update all occurrences
of the "[notification_intel]" literal in stats (including context messages on
query_row/prepare/query_map and the tracing::debug call) to use
"[notifications::store]" so logging/context is consistent with the store module
and types::NotificationStats.
- Around line 268-284: The logging prefix in mark_dismissed and mark_acted is
inconsistent with the rest of this module; update all tracing messages in the
mark_dismissed and mark_acted functions to use the existing grep-friendly prefix
"[notifications::store]" instead of "[notification_intel]" (i.e., change the
tracing::warn!, tracing::debug!, and any other tracing calls inside the
mark_dismissed and mark_acted functions to use "[notifications::store]" so they
match insert, list, update_triage, mark_read, etc.).
🪄 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: b5194b97-2c19-4fc9-8ecc-2c25a0b647e1
📒 Files selected for processing (20)
app/src/components/notifications/NotificationCard.tsxapp/src/components/notifications/NotificationCenter.tsxapp/src/components/settings/panels/NotificationRoutingPanel.tsxapp/src/lib/webviewNotifications/service.tsapp/src/pages/Notifications.tsxapp/src/services/notificationService.tsapp/src/services/webviewAccountService.tsapp/src/store/__tests__/notificationsSlice.dismissActions.test.tsapp/src/store/__tests__/notificationsSlice.test.tsapp/src/store/index.tsapp/src/store/notificationSlice.tsapp/src/store/notificationsSlice.tsapp/src/types/notifications.tssrc/core/event_bus/events.rssrc/openhuman/notifications/bus.rssrc/openhuman/notifications/rpc.rssrc/openhuman/notifications/schemas.rssrc/openhuman/notifications/store.rssrc/openhuman/notifications/types.rstests/json_rpc_e2e.rs
💤 Files with no reviewable changes (2)
- app/src/store/index.ts
- app/src/store/notificationsSlice.ts
Add frontend coverage for ingest success/skip/error paths, expand notification_stats schema outputs to match RPC payload, and stabilize webview bridge integration coverage by running request/error assertions in one runtime. Made-with: Cursor
Make ingest dedup atomic in a single transaction, compare recent-window timestamps by epoch, and redact account identifiers from debug logs while adding regression coverage for stale duplicate detection. Made-with: Cursor
Add frontend coverage for ingest success/skip/error paths, expand notification_stats schema outputs to match RPC payload, and stabilize webview bridge integration coverage by running request/error assertions in one runtime. Made-with: Cursor
Avoid double-rendering webview notifications by removing the legacy generic dispatch, add dismiss and ingest-outcome frontend coverage, and make react triage bridge messaging action-accurate. Made-with: Cursor
Make ingest dedup atomic in a single transaction, compare recent-window timestamps by epoch, and redact account identifiers from debug logs while adding regression coverage for stale duplicate detection. Made-with: Cursor
Add frontend coverage for ingest success/skip/error paths, expand notification_stats schema outputs to match RPC payload, and stabilize webview bridge integration coverage by running request/error assertions in one runtime. Made-with: Cursor
Avoid double-rendering webview notifications by removing the legacy generic dispatch, add dismiss and ingest-outcome frontend coverage, and make react triage bridge messaging action-accurate. Made-with: Cursor
Render NotificationCenter from a visibleItems projection so dismissed rows and non-selected providers are excluded consistently for list, unread count, and mark-all behavior. 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 (2)
app/src/components/notifications/NotificationCenter.tsx (1)
67-97:⚠️ Potential issue | 🟠 MajorReconcile optimistic state when read/dismiss RPCs fail.
These handlers mutate Redux first and then swallow the RPC error. If the backend call fails, the item stays locally read/dismissed until the next refetch, then reappears or flips back. Please either roll the state back or trigger a refetch in the
catchpath, and log the failure so it is diagnosable.As per coding guidelines, "Frontend debug logging should use namespaced
debugcalls with dev-only detail. Log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors."🤖 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 67 - 97, The optimistic-update handlers (handleMarkRead, handleDismiss, handleMarkAllRead) currently swallow RPC errors leaving UI state inconsistent; update each catch block to either roll back the optimistic Redux mutation (dispatch the inverse action: e.g., undoMarkIntegrationRead/restoreIntegrationNotification) or dispatch a refetch action to reconcile state, and ensure the catch also logs the error using the namespaced debug logger (e.g., debug('notifications:handleMarkRead') / debug('notifications:handleDismiss') / debug('notifications:handleMarkAllRead') with the error object and context). Keep existing optimistic flow but add rollback/refetch + debug logging in every catch so failures are diagnosable and UI state is reconciled.src/openhuman/notifications/rpc.rs (1)
129-173:⚠️ Potential issue | 🟠 MajorStop after
update_triagefails.This path still publishes
NotificationTriagedand may runapply_decisioneven when the DB update failed. That leaves subscribers and downstream side effects observing a "triaged" notification thatlist/statsstill persist as unscored. Return early on the persistence error.Suggested guard
if let Err(e) = store::update_triage( &config_for_triage, &id_for_triage, score, &action, &reason, ) { tracing::warn!( id = %id_for_triage, error = %e, "[notification_intel] failed to persist triage result" ); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/notifications/rpc.rs` around lines 129 - 173, The DB update failure handler around store::update_triage must stop further processing: after logging the error when update_triage returns Err(e), return early instead of continuing to compute latency, publish DomainEvent::NotificationTriaged via publish_global, or call apply_decision; modify the code in the block that currently logs the error to perform an early return (or propagate the error) so that NotificationTriaged is not emitted and apply_decision is not invoked for failed persistence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openhuman/notifications/store.rs`:
- Around line 117-175: The current insert_if_not_recent uses
unchecked_transaction() (DEFERRED) so two concurrent calls can both see
COUNT(*)==0 and both insert; change the transaction to acquire a write lock
up-front (use an IMMEDIATE transaction via
transaction_with_behavior(TransactionBehavior::Immediate) or execute a "BEGIN
IMMEDIATE" before the COUNT) so tx.query_row / tx.execute are inside an
IMMEDIATE tx, or alternatively replace the COUNT + INSERT pair with a single
atomic SQL (INSERT ... SELECT ... WHERE NOT EXISTS (...) or UPSERT) to ensure
one-step deduplication; update the tx.commit() usage accordingly and add a
concurrent-insertion regression test that calls insert_if_not_recent
concurrently and asserts only one row is inserted.
---
Outside diff comments:
In `@app/src/components/notifications/NotificationCenter.tsx`:
- Around line 67-97: The optimistic-update handlers (handleMarkRead,
handleDismiss, handleMarkAllRead) currently swallow RPC errors leaving UI state
inconsistent; update each catch block to either roll back the optimistic Redux
mutation (dispatch the inverse action: e.g.,
undoMarkIntegrationRead/restoreIntegrationNotification) or dispatch a refetch
action to reconcile state, and ensure the catch also logs the error using the
namespaced debug logger (e.g., debug('notifications:handleMarkRead') /
debug('notifications:handleDismiss') / debug('notifications:handleMarkAllRead')
with the error object and context). Keep existing optimistic flow but add
rollback/refetch + debug logging in every catch so failures are diagnosable and
UI state is reconciled.
In `@src/openhuman/notifications/rpc.rs`:
- Around line 129-173: The DB update failure handler around store::update_triage
must stop further processing: after logging the error when update_triage returns
Err(e), return early instead of continuing to compute latency, publish
DomainEvent::NotificationTriaged via publish_global, or call apply_decision;
modify the code in the block that currently logs the error to perform an early
return (or propagate the error) so that NotificationTriaged is not emitted and
apply_decision is not invoked for failed persistence.
🪄 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: a0c45ede-cf22-4c3e-b877-a474f40db236
📒 Files selected for processing (9)
app/src/components/notifications/NotificationCenter.tsxapp/src/lib/webviewNotifications/service.test.tsapp/src/lib/webviewNotifications/service.tsapp/src/store/__tests__/notificationsSlice.test.tssrc/openhuman/notifications/bus.rssrc/openhuman/notifications/rpc.rssrc/openhuman/notifications/schemas.rssrc/openhuman/notifications/store.rstests/webview_apis_bridge.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/openhuman/notifications/bus.rs
- app/src/store/tests/notificationsSlice.test.ts
Fix payload fidelity and account-id redaction in webview notification ingest, align notification RPC latency/event semantics, add missing notification service wrappers and about-app capability entries, and expand schema/test coverage plus bridge diagnostics. Made-with: Cursor
Make dismiss/acted RPCs return accurate success for missing IDs, refresh provider settings before orchestration routing, harden notification service error-path logging, make triage notification IDs event-deterministic, and add store coverage for exists_recent/status transitions/stats. Made-with: Cursor
Publish NotificationTriaged only after routing decisions are evaluated, include an explicit routed flag in the domain event contract, and ensure notification bus consumers only surface routed triage actions. Made-with: Cursor
…feat/718-phase3-frontend
…o remove eslint disable comments. This improves code readability and maintains logging functionality without unnecessary comments.
…kiness Address unresolved PR feedback by removing nested interactive controls in notification cards, stabilizing async webview notification tests, and correcting triage messaging coverage. Also remove duplicated Rust test definitions that were breaking CI compilation. Made-with: Cursor
Resolve TypeScript workflow failures by removing an unused payload field and a duplicate NotificationStats type alias, and apply Prettier-required formatting updates in E2E files so format checks pass locally. Made-with: Cursor
Made-with: Cursor
Increase Linux WebDriver session retry timeout/retry count and retry the agent-review E2E run once in workflow to reduce intermittent tauri-driver startup timeout failures. Made-with: Cursor
Apply formatting changes required by pre-push checks so the E2E stability update can be pushed. Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Use pnpm setup/caching and pnpm script invocations in Type Check and Frontend Unit Tests workflows to match the repository packageManager and avoid setup-node yarn cache failures. Made-with: Cursor
Made-with: Cursor
…eat/718-phase4-tests
…nction and add concurrency test - Refactored the insert_if_not_recent function to use a single connection for the transaction, enhancing clarity and error handling. - Added a new test to ensure that insert_if_not_recent behaves atomically under concurrent calls, preventing duplicate notifications from being inserted.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/openhuman/notifications/store.rs (2)
1-835: File exceeds the ~500 line guideline.At 835 lines, this module is approaching the point where splitting would improve maintainability. The public API, row conversion helpers, and tests could potentially be organized into submodules. This is optional given the current PR scope.
As per coding guidelines: "
src/**/*.rs: Source files should be ≤ ~500 lines; split modules when growing to improve maintainability."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/notifications/store.rs` around lines 1 - 835, The module is too large; split it into smaller submodules to stay under ~500 lines by extracting the row conversion helpers (rows_to_notifications, row_to_notification) into a private submodule (e.g., store::rows or store::mapper), moving the large test suite (the #[cfg(test)] mod tests) into a dedicated test file (e.g., tests/store_tests.rs or a store/tests.rs submodule) and keep the public API functions (with_connection, insert, insert_if_not_recent, list, update_triage, mark_read, unread_count, exists_recent, mark_dismissed, mark_acted, stats, upsert_settings, get_settings) in the main store.rs; update visibility and imports accordingly and ensure unit tests import the functions and helper types from the new module locations.
696-722: Addbusy_timeoutto notifications database connection to prevent SQLITE_BUSY during concurrent transactions.The
with_connectionfunction (lines 49-81) doesn't setbusy_timeouton the SQLite connection. Wheninsert_if_not_recentusesBEGIN IMMEDIATE(line 121) with concurrent callers, both threads could attempt to acquire the write lock simultaneously. Withoutbusy_timeout, one thread may receiveSQLITE_BUSYand propagate an error, causing the.unwrap()calls at lines 714-715 to panic.The pattern is already used in
src/openhuman/memory/tree/store.rswith a 5-second timeout. Apply the same pattern here to ensure robust concurrent access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/notifications/store.rs` around lines 696 - 722, The tests can panic from SQLITE_BUSY because with_connection does not set a busy_timeout on the SQLite connection; update with_connection to call set_busy_timeout (or equivalent) on the returned rusqlite::Connection with a 5-second timeout (same pattern used in src/openhuman/memory/tree/store.rs) so concurrent callers of insert_if_not_recent (which uses BEGIN IMMEDIATE) will wait instead of erroring; ensure the change applies where with_connection constructs/returns the Connection so list(), insert_if_not_recent() and other callers benefit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/openhuman/notifications/store.rs`:
- Around line 1-835: The module is too large; split it into smaller submodules
to stay under ~500 lines by extracting the row conversion helpers
(rows_to_notifications, row_to_notification) into a private submodule (e.g.,
store::rows or store::mapper), moving the large test suite (the #[cfg(test)] mod
tests) into a dedicated test file (e.g., tests/store_tests.rs or a
store/tests.rs submodule) and keep the public API functions (with_connection,
insert, insert_if_not_recent, list, update_triage, mark_read, unread_count,
exists_recent, mark_dismissed, mark_acted, stats, upsert_settings, get_settings)
in the main store.rs; update visibility and imports accordingly and ensure unit
tests import the functions and helper types from the new module locations.
- Around line 696-722: The tests can panic from SQLITE_BUSY because
with_connection does not set a busy_timeout on the SQLite connection; update
with_connection to call set_busy_timeout (or equivalent) on the returned
rusqlite::Connection with a 5-second timeout (same pattern used in
src/openhuman/memory/tree/store.rs) so concurrent callers of
insert_if_not_recent (which uses BEGIN IMMEDIATE) will wait instead of erroring;
ensure the change applies where with_connection constructs/returns the
Connection so list(), insert_if_not_recent() and other callers benefit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7f5d7da-6f5f-41f5-9923-b5e54fcf2be7
📒 Files selected for processing (2)
src/openhuman/notifications/store.rstests/json_rpc_e2e.rs
✅ Files skipped from review due to trivial changes (1)
- tests/json_rpc_e2e.rs
senamakel
approved these changes
Apr 24, 2026
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
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
2 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
tests/json_rpc_e2e.rsdismissIntegrationNotificationinapp/src/store/__tests__/notificationsSlice.dismissActions.test.tsTest plan
cargo test --test json_rpc_e2e notification_ingest_list_dismiss_mark_acted_and_stats_roundtripcargo test --test json_rpc_e2e notification_ingest_async_triage_progresses_without_blocking_ingestyarn workspace openhuman-app test:unit src/store/__tests__/notificationsSlice.dismissActions.test.tsMade with Cursor
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Closes #718