feat(webview): native OS notifications from embedded webview apps (#714)#727
Merged
senamakel merged 31 commits intotinyhumansai:mainfrom Apr 22, 2026
Merged
Conversation
tinyhumansai#714) Forward CEF notification intercept payloads to tauri-plugin-notification, prefixing the title with the provider label so the source of each toast is obvious at a glance. Honour `silent` (skip toast, still record route), `icon` (passed through to the native builder), and `tag` (used as the dedup key, with a monotonic timestamp fallback for untagged payloads). Record a NotificationRoute keyed by `{provider}:{account_id}:{tag_or_uuid}` so a future click hook (UNUserNotificationCenter / notify-rust on_response) can route the OS click back to the source account. Entries are cleared on webview_account_close / _purge to bound map growth. Expose webview_notification_permission_state / _request commands mapping tauri::plugin::PermissionState onto the web API triple. Non-cef stubs return "default" so the frontend can call the same invoke names on both runtimes. Wire notification:allow-* capabilities so the plugin can be invoked from the webview. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nsai#714) Round-trip the OS notification permission once per session on first account open via the new invoke pair. Attach a dormant notification:click listener that dispatches setActiveAccount and brings the main window to front when a platform click hook starts emitting the event — contract matches the Rust NotificationRoute shape so the emit side is a one-liner. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Lockfile picked up the pending 0.52.26 version bump from Cargo.toml while building the notification feature. No dependency graph change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 end-to-end native notification support: webview JS shims and CDP injection to surface granted permission and capture events, Tauri command hooks and capability entries, frontend notification UI/state/services and settings, a backend notifications domain with RPCs and SQLite store, and click-to-focus routing. Changes
Sequence DiagramsequenceDiagram
participant Webview as Webview (CEF)
participant Tauri as Tauri Runtime
participant Frontend as Frontend UI
participant Backend as Backend RPC
participant Store as SQLite Store
participant Triage as Triage Pipeline
participant OS as OS Notification
Webview->>Webview: new Notification(...) (ua_spoof.js intercept)
Webview->>Tauri: recipe event (kind='notify', payload)
Tauri->>Frontend: deliver recipe event
Frontend->>Backend: notification_ingest RPC {provider,account,title,body,raw}
Backend->>Backend: check provider settings
alt disabled
Backend-->>Frontend: {skipped:true}
else enabled
Backend->>Store: insert notification (status=unread)
Backend-->>Frontend: {id,skipped:false}
Backend->>Triage: spawn run_triage(...)
Triage->>Store: update_triage (importance, action)
Triage->>Backend: apply_decision (maybe route/escalate)
end
Frontend->>OS: invoke native notification (via tauri)
OS->>Tauri: notification:click {account_id,provider}
Tauri->>Frontend: emit notification:click event
Frontend->>Frontend: setFocusedAccount / setActiveAccount
Frontend->>Tauri: invoke activate_main_window
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
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: 3
🤖 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/webview_accounts/mod.rs`:
- Around line 265-275: The info-level log in log::info! is currently emitting
raw_title (user-facing notification text) which may contain PII; remove
raw_title from the info log call in webview_accounts::notify-cef code path (the
log::info! invocation) and either omit it entirely or replace it with a
non-sensitive placeholder/metadata (e.g., title length or a redacted token) and,
if you still need the full title for debugging, log the raw_title at debug or
trace level instead (move it to log::debug! or log::trace! referencing
raw_title). Ensure the format string and argument list for log::info! are
updated to match the removed/replaced field.
- Around line 256-307: The code currently inserts a notification_routes entry
for every notification (using notification_route_key and
WebviewAccountsState::notification_routes) before checking payload.silent or
whether builder.show() succeeds, causing unbounded growth; change the logic so
entries are only stored for notifications that will be routable later: either
move the insert to after the payload.silent check and after builder.show()
returns Ok(), or if you must insert earlier, ensure you remove the route on
payload.silent and on Err(e) from builder.show(). Use the existing symbols
notification_route_key, NotificationRoute, payload.silent, builder.show(), and
state.notification_routes.lock().unwrap().insert/remove to implement the fix.
In `@app/src/services/webviewAccountService.ts`:
- Around line 123-135: ensureNotificationPermission currently sets
permissionChecked = true before the async invokes, so a transient failure leaves
the session marked checked; move the mutation of permissionChecked into the
success path inside ensureNotificationPermission: only set permissionChecked =
true after the initial invoke('webview_notification_permission_state') completes
successfully (and likewise after the follow-up
invoke('webview_notification_permission_request') if performed), and keep the
try/catch so that on error permissionChecked remains false and future calls will
retry; references: ensureNotificationPermission, permissionChecked,
invoke('webview_notification_permission_state'),
invoke('webview_notification_permission_request').
🪄 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: c9119250-e554-4f60-8ade-e9024ddf561f
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
app/src-tauri/capabilities/default.jsonapp/src-tauri/src/lib.rsapp/src-tauri/src/webview_accounts/mod.rsapp/src/services/webviewAccountService.ts
tinyhumansai#679) - Add NotificationBypassPrefs (global DND, per-account mute, bypass-when-focused) to WebviewAccountsState with thread-safe AtomicBool window focus tracking - Evaluate all three bypass conditions inside forward_native_notification before showing OS toast; each suppression path logs at debug with [notify-bypass] prefix - Add four new Tauri commands: webview_notification_set_dnd, webview_notification_mute_account, webview_notification_get_bypass_prefs, webview_set_focused_account - Wire window focus tracking in setup hook via on_window_event Focused handler - Frontend: add setAccountMuted, setGlobalDnd, getBypassPrefs, setFocusedAccount helpers in webviewAccountService; sync focused account on open + click - Add NotificationsPanel settings page with Global DND toggle - Register NotificationsPanel at /settings/notifications Closes tinyhumansai#679
- Added Notifications page and routing to AppRoutes. - Introduced NotificationRoutingPanel in Settings for managing notification settings. - Updated SettingsHome to include navigation for notification routing. - Integrated notifications reducer into the store for state management. - Enhanced Rust backend to support notification handling from embedded webviews. This commit lays the groundwork for a comprehensive notification system within the application.
- Simplified JSX structure in NotificationCard for better readability. - Consolidated fetchNotifications call in NotificationCenter for cleaner syntax. - Improved formatting in NotificationRoutingPanel and notificationsSlice for consistency. - Enhanced Rust code readability by streamlining function signatures and logic. These changes enhance code maintainability and readability across the notifications feature.
…ction signature - Removed unnecessary line breaks in the webview_notification_set_dnd function for improved readability.
Feat/679 notification bypass
…anagement - Added `getNotificationSettings` and `setNotificationSettings` functions to manage notification settings for providers. - Enhanced `NotificationRoutingPanel` to display and update settings for Gmail, Slack, Discord, and WhatsApp. - Introduced new RPC endpoints for retrieving and updating notification settings. - Updated database schema to store notification settings persistently. This commit establishes a robust system for managing notification preferences, improving user control over notifications.
- Enhanced formatting in NotificationRoutingPanel for better clarity. - Streamlined function signatures in notificationService and Rust backend. - Improved readability of assertions in tests by adjusting line breaks. These changes contribute to a more maintainable and comprehensible codebase for the notifications feature.
Feat/718 notification routing
…nner Updates the tauri-cef submodule to 55db2d6 which adds a navigator.permissions.query shim in the CEF render process. Slack checks this API (not just Notification.permission) to decide whether to show its "needs your permission" banner — the shim returns "granted" for notifications queries so the banner no longer appears. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Switches from context.eval() to a proper PermissionsQueryV8Handler so the navigator.permissions.query fix actually runs in on_context_created. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The V8 set_value_bykey approach in cef-helper's on_context_created does
not stick on CEF platform objects (Chromium's V8 binding layer silently
ignores property writes on native wrappers like Permissions). The init
script path via frame.execute_java_script runs in the fully-initialised
JS context where navigator.permissions IS writable, matching how
ua_spoof.js already overrides navigator.userAgent successfully.
Slack checks navigator.permissions.query({ name: 'notifications' })
before showing its "needs permission" banner — patching it here to
return "granted" removes the banner.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ssions Two-layer fix for the Slack "needs permission to enable notifications" banner: 1. cef-helper (submodule update to 99a2686): context.eval() in on_context_created installs Object.defineProperty(navigator, 'permissions', ...) before any page JS runs. 2. ua_spoof.js: same Object.defineProperty pattern as belt-and-suspenders for frames that reload or trigger permission checks after on_load_end. Simple property assignment on Blink platform objects is silently ignored; Object.defineProperty on the navigator wrapper itself (the same mechanism already used for navigator.userAgent) works correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fication # Conflicts: # Cargo.lock # app/src-tauri/Cargo.lock # app/src-tauri/src/lib.rs # app/src-tauri/src/webview_accounts/mod.rs # app/src-tauri/vendor/tauri-cef # src/core/all.rs # src/openhuman/mod.rs
…ai#727 - Move raw_title out of log::info! (PII risk) — log title_chars at info, raw_title at debug only - Fix permissionChecked set before async invoke in ensureNotificationPermission so transient failures allow retry on next account open
The CEF backend uses a data: URL as the initial webview location so CDP can attach before the real provider URL loads. Tauri's add_child rejects data: URLs unless the webview-data-url feature is enabled.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
app/src/services/webviewAccountService.ts (1)
135-143:⚠️ Potential issue | 🟠 MajorOnly mark the session permission bootstrap complete after the request path succeeds.
permissionCheckedflips immediately after the state probe. Ifwebview_notification_permission_requestthrows once, lateropenWebviewAccount()calls will skip permission bootstrap for the rest of the session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/services/webviewAccountService.ts` around lines 135 - 143, The bug is that permissionChecked is set to true immediately after probing state so a subsequent failing call to invoke('webview_notification_permission_request') will leave the session marked complete; modify ensureNotificationPermission so that permissionChecked is only set to true after the request path succeeds (i.e., set permissionChecked = true after a successful invoke('webview_notification_permission_request') or when state === 'granted'), and ensure any thrown error from invoke('webview_notification_permission_request') does not flip permissionChecked (so leave permissionChecked false and rethrow or handle the error); update references in ensureNotificationPermission and callers like openWebviewAccount if needed to rely on the corrected permissionChecked behavior.
🧹 Nitpick comments (5)
app/src/components/settings/panels/NotificationsPanel.tsx (1)
12-23: Consider adding error handling for resilience.The async operations lack error handling:
getBypassPrefs()rejection will leave the component in loading state forever (line 15 won't execute)- If
setGlobalDnd(next)fails, the UI shows the toggled state but the backend wasn't updated♻️ Suggested improvement
useEffect(() => { - getBypassPrefs().then(prefs => { - if (prefs) setDnd(prefs.global_dnd); - setLoading(false); - }); + getBypassPrefs() + .then(prefs => { + if (prefs) setDnd(prefs.global_dnd); + }) + .catch(err => { + console.warn('[NotificationsPanel] Failed to load preferences:', err); + }) + .finally(() => setLoading(false)); }, []); const handleDndToggle = async () => { const next = !dnd; setDnd(next); - await setGlobalDnd(next); + try { + await setGlobalDnd(next); + } catch (err) { + console.warn('[NotificationsPanel] Failed to update DND:', err); + setDnd(!next); // Revert optimistic update + } };🤖 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 12 - 23, Add robust error handling around the async calls in useEffect and handleDndToggle: wrap the getBypassPrefs() call inside a try/catch/finally in the useEffect so any rejection is caught, ensure setLoading(false) runs in finally, and log or surface the error; in handleDndToggle wrap setGlobalDnd(next) in try/catch and on failure revert the optimistic UI change by calling setDnd(prev) (or re-read state) and surface/log the error so the UI and backend remain consistent. Ensure you update functions referenced (useEffect, getBypassPrefs, setLoading, setDnd, handleDndToggle, setGlobalDnd) only by adding try/catch/finally and revert logic, not by changing their signatures.app/src/services/notificationService.ts (1)
17-22: Prefer named interfaces for the RPC contracts in this file.
opts, the ingest payload, and the settings shapes are all anonymous object types right now. Extracting named interfaces will make the API surface easier to reuse and keep it in line with the repo's TS convention.As per coding guidelines:
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript.Also applies to: 57-80, 92-97
🤖 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 17 - 22, Replace the anonymous object types with named interfaces: define e.g. interface NotificationFetchOptions { provider?: string; limit?: number; offset?: number; min_score?: number } and use it in the fetchNotifications signature (fetchNotifications(opts?: NotificationFetchOptions)), and similarly extract interfaces for the ingest payload (e.g. NotificationIngestPayload) and settings shape (e.g. NotificationSettings) used elsewhere in this file (including the other function signatures referenced around the later blocks). Update function parameters and return types to reference these interfaces so the RPC contract is explicit and reusable, and export the interfaces if they need to be consumed externally.app/src/components/settings/panels/NotificationRoutingPanel.tsx (1)
19-24: Extract a shared settings interface instead of repeating the object shape.The same settings structure is duplicated in the state type and the patch type. A named interface will keep this file aligned with the repo's TS conventions and give you one source of truth.
As per coding guidelines:
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript.Also applies to: 52-56
🤖 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 19 - 24, Extract the repeated object shape into a named interface (e.g., NotificationSetting) and replace the inline type annotations currently used in the useState declaration for settings and the patch type referenced around the patch-handling code (the inline Record<string, { enabled: boolean; importance_threshold: number; route_to_orchestrator: boolean }> and the identical shape used later at lines ~52-56); update the useState generic to Record<string, NotificationSetting> and update the patch type to use NotificationSetting so there is a single source of truth for the settings shape and it follows the project's preference for interfaces.src/openhuman/notifications/store.rs (1)
108-153: Consider adding an upper bound onlimitto prevent excessive memory usage.While
limitis guaranteed to be numeric (parsed fromu64), there's no cap on its value. A client requestinglimit: 999999999could cause memory pressure.♻️ Proposed fix
pub fn list( config: &Config, limit: usize, offset: usize, provider_filter: Option<&str>, min_score: Option<f32>, ) -> Result<Vec<IntegrationNotification>> { + const MAX_LIMIT: usize = 500; + let limit = limit.min(MAX_LIMIT); with_connection(config, |conn| {🤖 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 108 - 153, In list(), clamp the input limit to a reasonable maximum before building the SQL to avoid unbounded allocations: introduce a constant (e.g., MAX_LIMIT), compute let limit = std::cmp::min(limit, MAX_LIMIT); and use that clamped value when formatting the LIMIT/OFFSET fragment; update any type conversions so the formatted limit is a usize/u64 matching MAX_LIMIT and ensure the change is applied inside the with_connection closure (function name: list, variables: limit, MAX_LIMIT, with_connection) so the query uses the safe bounded limit.src/openhuman/notifications/rpc.rs (1)
30-34: Consider adding entry-point debug logging for observability.Per coding guidelines, handlers should have debug/trace logs at entry points. Adding a brief log here would help trace request flow:
tracing::debug!( provider = %req.provider, "[notifications::rpc] handle_ingest called" );Similar entry-point logging could be added to
handle_list,handle_settings_get, andhandle_settings_setfor consistency.🤖 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 30 - 34, Add entry-point debug logging to the notification RPC handlers: in handle_ingest, after deserializing into req, call tracing::debug! to log that handle_ingest was called and include the provider field (e.g., provider = %req.provider) and a short context tag like "[notifications::rpc] handle_ingest called"; also add analogous tracing::debug! calls at the start of handle_list, handle_settings_get, and handle_settings_set so each handler logs its entry and any small identifying fields (e.g., provider or request id) to improve observability.
🤖 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 57-58: The current providers pill list is computed from the
filtered items array, so when selectedProvider is applied via
fetchNotifications({provider}) the providers list collapses; instead compute
providers from an unfiltered source (e.g., originalNotifications, allItems, or a
providers list returned by the API) and stop deriving it from items. Update the
providers calculation (the Array.from(new Set(items.map(n =>
n.provider))).sort() expression) to use the unfiltered state or a dedicated
providers array, and ensure code around selectedProvider and fetchNotifications
uses that unfiltered source so the pill row (and the providers.length > 1 check)
remains correct even when items is filtered.
- Line 5: The component NotificationCenter currently imports markReadAction and
setNotifications but never dispatches loading/error actions, so update the fetch
effect inside NotificationCenter to dispatch setNotificationsLoading(true)
before starting the RPC, then on success dispatch setNotifications(payload) and
setNotificationsLoading(false), and on failure dispatch
setNotificationsError(error) (and setNotificationsLoading(false)); ensure you
reference the existing action creators setNotificationsLoading and
setNotificationsError and the imported setNotifications/markReadAction so the
loading and error branches in the render become reachable.
In `@app/src/components/settings/panels/NotificationRoutingPanel.tsx`:
- Around line 50-66: The optimistic update in updateSetting must be rolled back
if persistence fails: capture the previous provider state (e.g., store a copy of
settings[provider] as prevState before calling setSettings), perform the
optimistic setSettings({ ... }), then wrap await setNotificationSettings({
provider, ...next }) in try/catch; on catch restore the previous state via
setSettings(prev => ({ ...prev, [provider]: prevState })) and rethrow or return
a rejected promise so callers can observe the failure; also ensure updateSetting
preserves and propagates the rejection (don’t swallow it) so callers can await
or handle errors.
In `@app/src/components/settings/SettingsHome.tsx`:
- Around line 130-163: Update the settings navigation types and routing logic to
include the new notifications routes: add 'notifications' and
'notification-routing' to the SettingsRoute union in useSettingsNavigation.ts;
update getCurrentRoute() to detect URL paths for '/settings/notifications' and
'/settings/notification-routing' (returning the corresponding SettingsRoute
values) and ensure any switch/case or path-matching logic covers those two new
routes; and extend getBreadcrumbs() to return appropriate breadcrumb arrays when
route is 'notifications' or 'notification-routing' (matching the pattern used by
other routes) so the panels NotificationsPanel and NotificationRoutingPanel are
discoverable and have breadcrumbs.
In `@app/src/services/notificationService.ts`:
- Around line 57-70: ingestNotification currently types its response as { id:
string } even though callCoreRpc (openhuman.notification_ingest) may return {
skipped: true } for disabled-provider flows; update the return type of
ingestNotification to a discriminated union (e.g. { id: string } | { skipped:
true }) and adjust the call signature to callCoreRpc<{ id: string } | { skipped:
true }>, then ensure any downstream usage of ingestNotification checks for the
skipped branch (inspect result.skipped or result.id) so callers no longer assume
an id exists.
In `@docs/SUMMARY.md`:
- Around line 1-127: This SUMMARY.md should either be removed from the PR or
cleaned up and committed: either drop the file from the commit (exclude it from
the PR/diff) or update SUMMARY.md by (1) removing the local worktree path
reference (/Volumes/Extreme SSD/...) near the top, (2) fixing capitalization
from "macos" to "macOS" in the Architecture Notes, and (3) removing the
self-referential deletion note about deleting SUMMARY.md before opening the PR;
then re-stage the cleaned SUMMARY.md (or remove it) and update the commit so the
PR reflects the chosen option.
In `@scripts/install.sh`:
- Around line 296-299: The dry-run branch currently treats a missing release
asset as success; update the branch that checks DRY_RUN to return a non-zero
exit when no compatible ${OS}/${ARCH} asset is found by replacing the current
exit 0 with a failing exit code (e.g., exit 1) and use a more appropriate error
logging call (swap log_warn for log_error) so installer smoke tests detect the
packaging regression; locate the conditional that references DRY_RUN and
log_warn and change the exit code and log level accordingly.
In `@scripts/setup-dev-codesign.sh`:
- Around line 77-80: The openssl pkcs12 invocation currently includes an
unconditional -legacy flag which overrides the earlier probe and
PKCS12_LEGACY_ARGS handling; remove the literal -legacy token from the openssl
pkcs12 command and rely solely on expanding "${PKCS12_LEGACY_ARGS[@]}" so that
-legacy is only passed when the probe added it to PKCS12_LEGACY_ARGS (ensure the
command still uses the array expansion as shown).
In `@src/openhuman/notifications/schemas.rs`:
- Around line 52-95: Update the "ingest" ControllerSchema outputs so clients can
represent the disabled-provider skip path: change the existing FieldSchema for
"id" in the "ingest" ControllerSchema to required: false (since skipped
responses won’t include an id) and add a new FieldSchema named "skipped" with
ty: TypeSchema::Bool and required: false (with a comment like "true when
provider is disabled and ingest was skipped"). This ensures the schema covers
both the normal { id: string } response and the { skipped: true } response used
by openhuman.notification_ingest.
In `@src/openhuman/notifications/store.rs`:
- Around line 283-284: The code currently silently falls back to Utc::now() when
parsing received_at_str fails; change the parse call for received_at
(DateTime<Utc>) to capture the parse error and emit a warning (e.g., using
log::warn! or tracing::warn!) that includes the offending received_at_str and
the parse error, then return Utc::now() as the fallback; specifically replace
the unwrap_or_else on received_at_str.parse() with a closure or match that logs
the warning with received_at_str and the error before returning Utc::now().
---
Duplicate comments:
In `@app/src/services/webviewAccountService.ts`:
- Around line 135-143: The bug is that permissionChecked is set to true
immediately after probing state so a subsequent failing call to
invoke('webview_notification_permission_request') will leave the session marked
complete; modify ensureNotificationPermission so that permissionChecked is only
set to true after the request path succeeds (i.e., set permissionChecked = true
after a successful invoke('webview_notification_permission_request') or when
state === 'granted'), and ensure any thrown error from
invoke('webview_notification_permission_request') does not flip
permissionChecked (so leave permissionChecked false and rethrow or handle the
error); update references in ensureNotificationPermission and callers like
openWebviewAccount if needed to rely on the corrected permissionChecked
behavior.
---
Nitpick comments:
In `@app/src/components/settings/panels/NotificationRoutingPanel.tsx`:
- Around line 19-24: Extract the repeated object shape into a named interface
(e.g., NotificationSetting) and replace the inline type annotations currently
used in the useState declaration for settings and the patch type referenced
around the patch-handling code (the inline Record<string, { enabled: boolean;
importance_threshold: number; route_to_orchestrator: boolean }> and the
identical shape used later at lines ~52-56); update the useState generic to
Record<string, NotificationSetting> and update the patch type to use
NotificationSetting so there is a single source of truth for the settings shape
and it follows the project's preference for interfaces.
In `@app/src/components/settings/panels/NotificationsPanel.tsx`:
- Around line 12-23: Add robust error handling around the async calls in
useEffect and handleDndToggle: wrap the getBypassPrefs() call inside a
try/catch/finally in the useEffect so any rejection is caught, ensure
setLoading(false) runs in finally, and log or surface the error; in
handleDndToggle wrap setGlobalDnd(next) in try/catch and on failure revert the
optimistic UI change by calling setDnd(prev) (or re-read state) and surface/log
the error so the UI and backend remain consistent. Ensure you update functions
referenced (useEffect, getBypassPrefs, setLoading, setDnd, handleDndToggle,
setGlobalDnd) only by adding try/catch/finally and revert logic, not by changing
their signatures.
In `@app/src/services/notificationService.ts`:
- Around line 17-22: Replace the anonymous object types with named interfaces:
define e.g. interface NotificationFetchOptions { provider?: string; limit?:
number; offset?: number; min_score?: number } and use it in the
fetchNotifications signature (fetchNotifications(opts?:
NotificationFetchOptions)), and similarly extract interfaces for the ingest
payload (e.g. NotificationIngestPayload) and settings shape (e.g.
NotificationSettings) used elsewhere in this file (including the other function
signatures referenced around the later blocks). Update function parameters and
return types to reference these interfaces so the RPC contract is explicit and
reusable, and export the interfaces if they need to be consumed externally.
In `@src/openhuman/notifications/rpc.rs`:
- Around line 30-34: Add entry-point debug logging to the notification RPC
handlers: in handle_ingest, after deserializing into req, call tracing::debug!
to log that handle_ingest was called and include the provider field (e.g.,
provider = %req.provider) and a short context tag like "[notifications::rpc]
handle_ingest called"; also add analogous tracing::debug! calls at the start of
handle_list, handle_settings_get, and handle_settings_set so each handler logs
its entry and any small identifying fields (e.g., provider or request id) to
improve observability.
In `@src/openhuman/notifications/store.rs`:
- Around line 108-153: In list(), clamp the input limit to a reasonable maximum
before building the SQL to avoid unbounded allocations: introduce a constant
(e.g., MAX_LIMIT), compute let limit = std::cmp::min(limit, MAX_LIMIT); and use
that clamped value when formatting the LIMIT/OFFSET fragment; update any type
conversions so the formatted limit is a usize/u64 matching MAX_LIMIT and ensure
the change is applied inside the with_connection closure (function name: list,
variables: limit, MAX_LIMIT, with_connection) so the query uses the safe bounded
limit.
🪄 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: 0d117cbe-ad13-4919-be51-41a8b76abe67
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
app/src-tauri/Cargo.tomlapp/src-tauri/src/webview_accounts/mod.rsapp/src-tauri/src/webview_accounts/ua_spoof.jsapp/src/AppRoutes.tsxapp/src/components/notifications/NotificationCard.tsxapp/src/components/notifications/NotificationCenter.tsxapp/src/components/settings/SettingsHome.tsxapp/src/components/settings/panels/NotificationRoutingPanel.tsxapp/src/components/settings/panels/NotificationsPanel.tsxapp/src/pages/Notifications.tsxapp/src/pages/Settings.tsxapp/src/services/notificationService.tsapp/src/services/webviewAccountService.tsapp/src/store/index.tsapp/src/store/notificationsSlice.tsapp/src/types/notifications.tsdocs/SUMMARY.mdscripts/install.shscripts/setup-dev-codesign.shsrc/openhuman/agent/triage/envelope.rssrc/openhuman/notifications/mod.rssrc/openhuman/notifications/rpc.rssrc/openhuman/notifications/schemas.rssrc/openhuman/notifications/store.rssrc/openhuman/notifications/types.rstests/json_rpc_e2e.rs
✅ Files skipped from review due to trivial changes (4)
- app/src/pages/Notifications.tsx
- app/src-tauri/Cargo.toml
- src/openhuman/notifications/mod.rs
- app/src/types/notifications.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src-tauri/src/webview_accounts/mod.rs
Comment on lines
+52
to
+95
| "ingest" => ControllerSchema { | ||
| namespace: "notification", | ||
| function: "ingest", | ||
| description: "Ingest a new notification from an embedded webview integration. \ | ||
| Immediately persists the record and kicks off background triage scoring.", | ||
| inputs: vec![ | ||
| FieldSchema { | ||
| name: "provider", | ||
| ty: TypeSchema::String, | ||
| comment: "Provider slug, e.g. \"gmail\", \"slack\", \"whatsapp\".", | ||
| required: true, | ||
| }, | ||
| FieldSchema { | ||
| name: "account_id", | ||
| ty: TypeSchema::Option(Box::new(TypeSchema::String)), | ||
| comment: "Webview account identifier (optional).", | ||
| required: false, | ||
| }, | ||
| FieldSchema { | ||
| name: "title", | ||
| ty: TypeSchema::String, | ||
| comment: "Short notification title / subject.", | ||
| required: true, | ||
| }, | ||
| FieldSchema { | ||
| name: "body", | ||
| ty: TypeSchema::String, | ||
| comment: "Notification body or preview text.", | ||
| required: true, | ||
| }, | ||
| FieldSchema { | ||
| name: "raw_payload", | ||
| ty: TypeSchema::Ref("JsonObject"), | ||
| comment: "Full raw event payload from the source for downstream use.", | ||
| required: true, | ||
| }, | ||
| ], | ||
| outputs: vec![FieldSchema { | ||
| name: "id", | ||
| ty: TypeSchema::String, | ||
| comment: "UUID of the newly created notification record.", | ||
| required: true, | ||
| }], | ||
| }, |
Contributor
There was a problem hiding this comment.
Keep the ingest schema aligned with the disabled-provider skip path.
The new e2e coverage exercises openhuman.notification_ingest returning { skipped: true } when a provider is disabled, but this schema still says the only successful output is id: string. Any schema-driven client, docs view, or controller tooling will assume id is always present and miss the skip case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/notifications/schemas.rs` around lines 52 - 95, Update the
"ingest" ControllerSchema outputs so clients can represent the disabled-provider
skip path: change the existing FieldSchema for "id" in the "ingest"
ControllerSchema to required: false (since skipped responses won’t include an
id) and add a new FieldSchema named "skipped" with ty: TypeSchema::Bool and
required: false (with a comment like "true when provider is disabled and ingest
was skipped"). This ensures the schema covers both the normal { id: string }
response and the { skipped: true } response used by
openhuman.notification_ingest.
…ansai#727 - cdp/emulation: bump Chrome UA 124→136 to pass Slack browser check - cdp/session: inject Page.addScriptToEvaluateOnNewDocument to stub Notification.permission as "granted" and silence provider banners - notifications/mod.rs + core/all.rs: wire notifications domain into the controller registry (fixes unknown-method in json_rpc_e2e tests) - notifications/schemas: add skipped bool output to ingest schema - notifications/store: add tracing::warn on datetime parse failure - notificationService: union return type for ingestNotification - webviewAccountService: narrow union before accessing result.id - NotificationCenter: drive loading/error from fetch effect; track allProviders separately so filter pills don't collapse on selection - NotificationRoutingPanel: rollback optimistic update on save failure - useSettingsNavigation: add notifications/notification-routing routes - scripts/install.sh: remove silent dry-run exit 0 on asset failure - scripts/setup-dev-codesign.sh: remove unconditional -legacy flag - docs/SUMMARY.md: remove worktree path, fix macOS capitalisation, remove self-referential deletion note
Auto-apply formatting changes flagged by the pre-push hook: - prettier reformatted NotificationCenter.tsx and notificationService.ts - cargo fmt reformatted all.rs, openhuman/mod.rs, notifications/schemas.rs - NotificationRoutingPanel: move providers array to module scope so useEffect dependency array is satisfied without exhaustive-deps warning
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
app/src/services/webviewAccountService.ts (1)
135-146:⚠️ Potential issue | 🟠 MajorDeduplicate the permission bootstrap and only request on
"default".Two concurrent
openWebviewAccount()calls can both enterensureNotificationPermission()beforepermissionCheckedflips, so this can issue duplicate OS permission requests. It also callswebview_notification_permission_requestfor"denied", andpermissionCheckedis set before that second invoke succeeds, so a transient request failure suppresses retries for the rest of the session.🛠️ Possible fix
-let permissionChecked = false; +let permissionCheckPromise: Promise<void> | null = null; async function ensureNotificationPermission(): Promise<void> { - if (permissionChecked) return; + if (permissionCheckPromise) return permissionCheckPromise; try { - const state = await invoke<string>('webview_notification_permission_state'); - permissionChecked = true; - log('notification permission state=%s', state); - if (state === 'granted') return; - const next = await invoke<string>('webview_notification_permission_request'); - log('notification permission after request=%s', next); + permissionCheckPromise = (async () => { + const state = await invoke<string>('webview_notification_permission_state'); + log('notification permission state=%s', state); + if (state !== 'default') return; + const next = await invoke<string>('webview_notification_permission_request'); + log('notification permission after request=%s', next); + })(); + await permissionCheckPromise; } catch (err) { + permissionCheckPromise = null; errLog('notification permission check failed: %o', err); } }Also applies to: 716-716
🧹 Nitpick comments (1)
src/openhuman/notifications/schemas.rs (1)
278-279: Rename stale test name for clarity.The test name says “three functions” but asserts five; renaming avoids confusion during failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/notifications/schemas.rs` around lines 278 - 279, Rename the stale test function all_controller_schemas_covers_three_functions to reflect the actual assertion count (e.g., all_controller_schemas_covers_five_functions) so the test name matches its expectations; update the function identifier in the test declaration and any references to it to avoid confusion when the test fails.
🤖 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/cdp/session.rs`:
- Around line 146-154: The injected script passed to
cdp.call("Page.addScriptToEvaluateOnNewDocument", ...) currently forces
Notification.permission to "granted" and always resolves
Notification.requestPermission(), which hides the real OS/browser state; change
this so the script consults the real permission state provided by our backend
(do not hardcode "granted") and proxy requestPermission() calls back to our
permission path instead of resolving locally. Locate the injection in session.rs
(the cdp.call invocation and the session_id usage) and replace the hardcoded
shim with one that reads a backend-supplied permission value (or queries it via
an IPC/binding) and routes Notification.requestPermission() through the same
backend permission-check/flow so returned promises reflect the true OS
permission ("default", "denied", or "granted").
In `@app/src/components/notifications/NotificationCenter.tsx`:
- Around line 21-39: The provider pill list (allProviders) can become stale when
a new notification from a previously unseen provider is added in real-time;
update allProviders whenever a notification is added rather than only on
unfiltered fetches. Concretely, in the place that handles incoming notifications
(the same handler that calls setNotifications or addNotification inside
NotificationCenter), check the new notification's provider against allProviders
and, if missing, call setAllProviders(prev => Array.from(new Set([...prev,
newProvider])).sort()); ensure this check runs for single-item additions as well
as batched adds so the pill row is updated immediately.
In `@app/src/components/settings/hooks/useSettingsNavigation.ts`:
- Around line 32-34: getCurrentRoute in useSettingsNavigation doesn't handle the
newly added SettingsRoute entries ('notifications' and 'notification-routing'),
so paths like /settings/notifications fall through to 'home'; update the route
resolution logic inside the getCurrentRoute function to explicitly map
'/settings/notifications' -> 'notifications' and
'/settings/notification-routing' -> 'notification-routing' (use the
SettingsRoute type/enum and any existing pathname parsing used for other
settings routes to keep behavior consistent with the rest of the function).
In `@src/core/all.rs`:
- Around line 148-149: The CLI namespace for "notification" is registered via
controllers.extend(crate::openhuman::notifications::all_notifications_registered_controllers())
but namespace_description() still returns None for "notification", so add a
description entry for "notification" in the namespace_description() function
(match the pattern used for other namespaces) and update/add unit tests to
assert namespace_description("notification") returns the expected string;
reference the controllers.extend(...) call and the namespace_description()
function and ensure tests cover the new mapping.
In `@src/openhuman/notifications/schemas.rs`:
- Around line 89-103: The ingest response schema's outputs vector (the
FieldSchema entries shown) is missing the optional "reason" field returned on
skipped notifications; add a FieldSchema with name: "reason", ty:
TypeSchema::Option(Box::new(TypeSchema::String)), comment: "Optional reason why
the notification was skipped (e.g., provider disabled).", and required: false so
schema-driven clients/docs include the skip reason alongside "skipped" and "id".
- Around line 217-223: The outputs for the settings_set endpoint are missing the
returned normalized settings: update the outputs Vec in the FieldSchema for
settings_set to include a second FieldSchema with name "settings", type matching
the settings schema (e.g., TypeSchema::Ref or the specific settings TypeSchema),
a descriptive comment (e.g., "Normalized settings returned after save."), and
required: true so registry metadata matches the handler return value; modify the
outputs Vec alongside the existing "ok" FieldSchema in the settings_set
definition to reflect both returned fields.
In `@src/openhuman/notifications/store.rs`:
- Around line 47-72: Add debug/trace instrumentation around the notifications
store entry points and DB operations: in with_connection, log entry/exit
(function start and before returning), log the resolved db_path, log before and
after Connection::open, log success/failure of create_dir_all, and log
before/after conn.execute_batch and before calling f(&conn); use the crate's
logging (tracing::debug/trace or log::debug) to record these checkpoints and
include contextual details (db_path.display(), parent.display(), error contexts)
so callers like with_connection, the migration block (conn.execute_batch), and
the invocation f(&conn) are observable; apply the same pattern to the other
store functions mentioned (lines 80-105, 116-152, 163-185, 203-252) at their
entry/exit points, DB calls, branches and error paths.
- Around line 47-72: The with_connection helper opens a new rusqlite Connection
without a busy timeout, causing transient "database is locked" errors under
concurrent access; after creating conn in with_connection (the Connection::open
call) set a busy timeout (e.g., Duration::from_secs(5) or reuse the existing
SQLITE_BUSY_TIMEOUT constant used in memory/tree/store.rs) by calling the
connection's busy_timeout(...) method and propagate any error (with_context
similarly) before running conn.execute_batch(SCHEMA) and invoking f(&conn).
- Around line 293-294: The code silently drops malformed scored_at values by
converting scored_at_str.parse().ok() into None; change the behavior in the
data-loading function that calls row.get(11)? (the code creating scored_at_str
and scored_at) so that parse failures are surfaced instead of silently returning
None — either return an Err with a clear context (including the offending
scored_at_str) or log a warning/error that includes scored_at_str and the parse
error and preserve the original string in the struct (or a separate
"raw_scored_at" field) so on-disk data issues are visible; update the handling
of scored_at (the scored_at_str -> scored_at conversion) accordingly.
- Around line 163-172: The update_triage flow currently ignores the affected-row
count from conn.execute so it returns Ok(()) even when no row matches the given
id; modify the block inside with_connection that calls conn.execute in
update_triage to capture the returned affected row count, and if it is 0 return
a Contextual error (e.g., via anyhow::bail or .with_context) indicating the
notification id was not found; do the same fix for mark_read (the other
conn.execute block) so both functions fail when no rows were updated rather than
silently succeeding.
---
Nitpick comments:
In `@src/openhuman/notifications/schemas.rs`:
- Around line 278-279: Rename the stale test function
all_controller_schemas_covers_three_functions to reflect the actual assertion
count (e.g., all_controller_schemas_covers_five_functions) so the test name
matches its expectations; update the function identifier in the test declaration
and any references to it to avoid confusion when the test fails.
🪄 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: 2a3bc41b-4ea1-4fc7-b302-ad474544e756
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
app/src-tauri/src/cdp/emulation.rsapp/src-tauri/src/cdp/session.rsapp/src-tauri/vendor/tauri-cefapp/src/components/notifications/NotificationCenter.tsxapp/src/components/settings/hooks/useSettingsNavigation.tsapp/src/components/settings/panels/NotificationRoutingPanel.tsxapp/src/services/notificationService.tsapp/src/services/webviewAccountService.tsdocs/SUMMARY.mdscripts/setup-dev-codesign.shsrc/core/all.rssrc/openhuman/mod.rssrc/openhuman/notifications/schemas.rssrc/openhuman/notifications/store.rs
✅ Files skipped from review due to trivial changes (4)
- src/openhuman/mod.rs
- app/src-tauri/vendor/tauri-cef
- scripts/setup-dev-codesign.sh
- app/src-tauri/src/cdp/emulation.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/components/settings/panels/NotificationRoutingPanel.tsx
- docs/SUMMARY.md
- app/src/services/notificationService.ts
Comment on lines
+146
to
+154
| cdp.call( | ||
| "Page.addScriptToEvaluateOnNewDocument", | ||
| json!({ | ||
| "source": "Object.defineProperty(Notification,'permission',{get:()=>'granted'});\ | ||
| Notification.requestPermission=()=>Promise.resolve('granted');" | ||
| }), | ||
| Some(&session_id), | ||
| ) | ||
| .await?; |
Contributor
There was a problem hiding this comment.
Don't force the embedded page's Notification API to "granted".
This makes every provider observe a successful permission state/request even when the OS is still "default" or "denied", so apps like Slack/Gmail can suppress their own permission UI while native toasts are still blocked. Feed the real permission state into this shim, and route requestPermission() through the same backend permission path instead of always resolving "granted".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/cdp/session.rs` around lines 146 - 154, The injected script
passed to cdp.call("Page.addScriptToEvaluateOnNewDocument", ...) currently
forces Notification.permission to "granted" and always resolves
Notification.requestPermission(), which hides the real OS/browser state; change
this so the script consults the real permission state provided by our backend
(do not hardcode "granted") and proxy requestPermission() calls back to our
permission path instead of resolving locally. Locate the injection in session.rs
(the cdp.call invocation and the session_id usage) and replace the hardcoded
shim with one that reads a backend-supplied permission value (or queries it via
an IPC/binding) and routes Notification.requestPermission() through the same
backend permission-check/flow so returned promises reflect the true OS
permission ("default", "denied", or "granted").
Comment on lines
+21
to
+39
| // All providers seen across unfiltered loads — kept separate so the filter | ||
| // pill row doesn't collapse when a provider filter is active. | ||
| const [allProviders, setAllProviders] = useState<string[]>([]); | ||
|
|
||
| // Fetch on mount and when provider filter changes. | ||
| useEffect(() => { | ||
| let cancelled = false; | ||
| const load = async () => { | ||
| dispatch(setNotificationsLoading(true)); | ||
| try { | ||
| const result = await fetchNotifications({ provider: selectedProvider, limit: 100 }); | ||
| if (!cancelled) { | ||
| dispatch(setNotifications(result)); | ||
| // Accumulate providers only from unfiltered loads so the pill row | ||
| // stays stable when a filter is active. | ||
| if (!selectedProvider) { | ||
| const seen = Array.from(new Set(result.items.map(n => n.provider))).sort(); | ||
| setAllProviders(seen); | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the provider pill list synced with live notifications.
allProviders only updates after an unfiltered fetch. If addNotification pushes the first item from a new provider while this screen is open, that notification shows up in items but the new filter pill never appears until a later full reload.
💡 Low-friction fix
- const providers = allProviders;
+ const providers =
+ selectedProvider === undefined
+ ? Array.from(new Set([...allProviders, ...items.map(n => n.provider)])).sort()
+ : allProviders;Also applies to: 78-78
🤖 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 21 -
39, The provider pill list (allProviders) can become stale when a new
notification from a previously unseen provider is added in real-time; update
allProviders whenever a notification is added rather than only on unfiltered
fetches. Concretely, in the place that handles incoming notifications (the same
handler that calls setNotifications or addNotification inside
NotificationCenter), check the new notification's provider against allProviders
and, if missing, call setAllProviders(prev => Array.from(new Set([...prev,
newProvider])).sort()); ensure this check runs for single-item additions as well
as batched adds so the pill row is updated immediately.
Comment on lines
+47
to
+72
| fn with_connection<T>(config: &Config, f: impl FnOnce(&Connection) -> Result<T>) -> Result<T> { | ||
| let db_path = config | ||
| .workspace_dir | ||
| .join("notifications") | ||
| .join("notifications.db"); | ||
|
|
||
| if let Some(parent) = db_path.parent() { | ||
| std::fs::create_dir_all(parent).with_context(|| { | ||
| format!( | ||
| "[notifications::store] failed to create dir {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| } | ||
|
|
||
| let conn = Connection::open(&db_path).with_context(|| { | ||
| format!( | ||
| "[notifications::store] failed to open DB at {}", | ||
| db_path.display() | ||
| ) | ||
| })?; | ||
|
|
||
| conn.execute_batch(SCHEMA) | ||
| .context("[notifications::store] schema migration failed")?; | ||
|
|
||
| f(&conn) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add debug/trace instrumentation around store entry points and DB operations.
This new store opens/migrates the DB and performs inserts, queries, and updates with almost no debug/trace checkpoints. That will make notification-delivery issues much harder to reconstruct in production. As per coding guidelines, "Rust code must use log / tracing at debug or trace level; add substantial development-oriented logs at entry/exit points, branch decisions, external calls, retries, state transitions, and error handling paths".
Also applies to: 80-105, 116-152, 163-185, 203-252
🤖 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 47 - 72, Add debug/trace
instrumentation around the notifications store entry points and DB operations:
in with_connection, log entry/exit (function start and before returning), log
the resolved db_path, log before and after Connection::open, log success/failure
of create_dir_all, and log before/after conn.execute_batch and before calling
f(&conn); use the crate's logging (tracing::debug/trace or log::debug) to record
these checkpoints and include contextual details (db_path.display(),
parent.display(), error contexts) so callers like with_connection, the migration
block (conn.execute_batch), and the invocation f(&conn) are observable; apply
the same pattern to the other store functions mentioned (lines 80-105, 116-152,
163-185, 203-252) at their entry/exit points, DB calls, branches and error
paths.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this store configures any SQLite busy timeout or related lock-handling today.
rg -n -C2 --type=rust 'busy_timeout|PRAGMA\s+busy_timeout|journal_mode' src/openhuman/notifications/store.rsRepository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
# Check for other SQLite Connection opens in the codebase to see how busy_timeout is handled elsewhere
rg -n 'Connection::open|busy_timeout' --type=rust -A2 | head -100Repository: tinyhumansai/openhuman
Length of output: 7731
🏁 Script executed:
# Look at the full notifications/store.rs file to understand the module structure and usage patterns
wc -l src/openhuman/notifications/store.rsRepository: tinyhumansai/openhuman
Length of output: 106
🏁 Script executed:
# Check whether the notifications store module is exposed and used by RPC handlers
rg -n 'notifications.*store|use.*notifications' --type=rust src/core_server/ | head -20Repository: tinyhumansai/openhuman
Length of output: 122
🏁 Script executed:
# Check the notifications module's public API and entry points
fd 'notifications' --type f --path '*/src/openhuman/*' | head -10Repository: tinyhumansai/openhuman
Length of output: 299
🏁 Script executed:
# Check the memory/tree/store.rs pattern with busy_timeout
sed -n '340,355p' src/openhuman/memory/tree/store.rsRepository: tinyhumansai/openhuman
Length of output: 816
🏁 Script executed:
# Find where SQLITE_BUSY_TIMEOUT is defined
rg -n 'SQLITE_BUSY_TIMEOUT\s*=' --type=rustRepository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
# Check how notifications store is actually used (look for imports/calls)
rg -n 'notifications::store' --type=rust | head -20Repository: tinyhumansai/openhuman
Length of output: 1687
🏁 Script executed:
# Search for where notifications store is actually invoked (not just internal error messages)
rg -n 'with_connection|notifications.*insert|notifications.*list|notifications.*update_triage' --type=rust src/ | grep -v 'store.rs' | head -20Repository: tinyhumansai/openhuman
Length of output: 2313
🏁 Script executed:
# Check if SQLITE_BUSY_TIMEOUT is inline or from a dependency
rg -n 'busy_timeout\(' --type=rust -B2 -A2Repository: tinyhumansai/openhuman
Length of output: 555
🏁 Script executed:
# Look for any API handler or module that calls notifications functions
rg -n 'pub.*fn.*notification|notifications::|Notification' --type=rust src/openhuman/ | grep -v test | head -30Repository: tinyhumansai/openhuman
Length of output: 3202
🏁 Script executed:
# Check notifications/rpc.rs to see how it calls store operations
sed -n '1,100p' src/openhuman/notifications/rpc.rsRepository: tinyhumansai/openhuman
Length of output: 3967
🏁 Script executed:
# Check SQLITE_BUSY_TIMEOUT definition
rg -n 'const.*SQLITE_BUSY_TIMEOUT|SQLITE_BUSY_TIMEOUT\s*=' --type=rust -B1 -A1Repository: tinyhumansai/openhuman
Length of output: 280
🏁 Script executed:
# Check if subconscious store also lacks busy_timeout
sed -n '20,35p' src/openhuman/subconscious/store.rsRepository: tinyhumansai/openhuman
Length of output: 607
Configure a SQLite busy timeout on each new connection.
This helper opens a fresh connection for every store operation. The notifications store is accessed concurrently (from RPC handlers and background triage tasks), and without a busy timeout configured, transient SQLite file locking can surface as immediate database is locked failures. This follows the established pattern in src/openhuman/memory/tree/store.rs (which uses SQLITE_BUSY_TIMEOUT = Duration::from_secs(5)).
Suggested change
let conn = Connection::open(&db_path).with_context(|| {
format!(
"[notifications::store] failed to open DB at {}",
db_path.display()
)
})?;
+ conn
+ .busy_timeout(std::time::Duration::from_secs(5))
+ .context("[notifications::store] failed to configure sqlite busy timeout")?;
conn.execute_batch(SCHEMA)
.context("[notifications::store] schema migration failed")?;🤖 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 47 - 72, The
with_connection helper opens a new rusqlite Connection without a busy timeout,
causing transient "database is locked" errors under concurrent access; after
creating conn in with_connection (the Connection::open call) set a busy timeout
(e.g., Duration::from_secs(5) or reuse the existing SQLITE_BUSY_TIMEOUT constant
used in memory/tree/store.rs) by calling the connection's busy_timeout(...)
method and propagate any error (with_context similarly) before running
conn.execute_batch(SCHEMA) and invoking f(&conn).
Comment on lines
+163
to
+172
| with_connection(config, |conn| { | ||
| let now = Utc::now().to_rfc3339(); | ||
| conn.execute( | ||
| "UPDATE integration_notifications | ||
| SET importance_score = ?1, triage_action = ?2, triage_reason = ?3, scored_at = ?4 | ||
| WHERE id = ?5", | ||
| params![score, action, reason, now, id], | ||
| ) | ||
| .context("[notifications::store] update_triage failed")?; | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether update_triage checks the affected-row count from rusqlite::Connection::execute.
rg -n -C3 --type=rust 'fn update_triage|let updated = conn\.execute|conn\.execute\(' src/openhuman/notifications/store.rsRepository: tinyhumansai/openhuman
Length of output: 1922
🏁 Script executed:
#!/bin/bash
# Read the full update_triage function to confirm current state
sed -n '155,175p' src/openhuman/notifications/store.rsRepository: tinyhumansai/openhuman
Length of output: 661
🏁 Script executed:
#!/bin/bash
# Check the test for update_triage to see if it validates error cases
sed -n '369,420p' src/openhuman/notifications/store.rsRepository: tinyhumansai/openhuman
Length of output: 2015
🏁 Script executed:
#!/bin/bash
# Compare mark_read (similar UPDATE operation) to see if it has the same pattern
sed -n '176,195p' src/openhuman/notifications/store.rsRepository: tinyhumansai/openhuman
Length of output: 764
Fail update_triage when the target notification does not exist.
The function ignores the affected-row count from conn.execute(), so a stale id returns Ok(()) even though nothing was updated. The caller would incorrectly believe the triage persisted when the record is already gone. The test only covers the happy path and does not validate this error condition.
Suggested change
let now = Utc::now().to_rfc3339();
- conn.execute(
+ let updated = conn.execute(
"UPDATE integration_notifications
SET importance_score = ?1, triage_action = ?2, triage_reason = ?3, scored_at = ?4
WHERE id = ?5",
params![score, action, reason, now, id],
)
.context("[notifications::store] update_triage failed")?;
+ if updated == 0 {
+ anyhow::bail!("[notifications::store] update_triage missing notification: {id}");
+ }
Ok(())
})
}Note: mark_read has the same issue at lines 177–189.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| with_connection(config, |conn| { | |
| let now = Utc::now().to_rfc3339(); | |
| conn.execute( | |
| "UPDATE integration_notifications | |
| SET importance_score = ?1, triage_action = ?2, triage_reason = ?3, scored_at = ?4 | |
| WHERE id = ?5", | |
| params![score, action, reason, now, id], | |
| ) | |
| .context("[notifications::store] update_triage failed")?; | |
| Ok(()) | |
| with_connection(config, |conn| { | |
| let now = Utc::now().to_rfc3339(); | |
| let updated = conn.execute( | |
| "UPDATE integration_notifications | |
| SET importance_score = ?1, triage_action = ?2, triage_reason = ?3, scored_at = ?4 | |
| WHERE id = ?5", | |
| params![score, action, reason, now, id], | |
| ) | |
| .context("[notifications::store] update_triage failed")?; | |
| if updated == 0 { | |
| anyhow::bail!("[notifications::store] update_triage missing notification: {id}"); | |
| } | |
| Ok(()) | |
| }) | |
| } |
🤖 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 163 - 172, The
update_triage flow currently ignores the affected-row count from conn.execute so
it returns Ok(()) even when no row matches the given id; modify the block inside
with_connection that calls conn.execute in update_triage to capture the returned
affected row count, and if it is 0 return a Contextual error (e.g., via
anyhow::bail or .with_context) indicating the notification id was not found; do
the same fix for mark_read (the other conn.execute block) so both functions fail
when no rows were updated rather than silently succeeding.
- Added new commands for managing notification preferences, including setting global Do Not Disturb (DND), muting specific accounts, and retrieving current bypass preferences. - Implemented a notification permission state handler to ensure consistent behavior across different environments. - Updated the JavaScript shim for notification permissions to handle both Notification and PushManager states, ensuring compatibility with various providers. - Refactored the WebviewAccountsState to include a new structure for managing notification bypass preferences, improving the overall notification handling logic.
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-tauri/src/webview_accounts/mod.rs (1)
515-533:⚠️ Potential issue | 🟠 MajorCEF now publishes two different Chrome versions.
This path prepends
UA_SPOOF_JSon CEF, but the Rust-sideCHROME_UAused byprovider_user_agent()is stillChrome/124.0.0.0whileua_spoof.jshardcodes124.0.6367.118. Providers that compare request headers,navigator.userAgent, anduserAgentDatawill see a split fingerprint and can fall back to unsupported-browser logic. Keep one shared UA source of truth for the CEF path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src-tauri/src/webview_accounts/mod.rs` around lines 515 - 533, The UA spoofing JS currently hardcodes a Chrome version that differs from the Rust CHROME_UA constant, causing inconsistent fingerprints; update the code to derive the spoof string from the shared CHROME_UA constant instead of a literal. Concretely, change UA_SPOOF_JS so it is produced or formatted using CHROME_UA (or expose a function that returns the spoof JS using CHROME_UA) and ensure the code path that builds the final payload (the format! call that references UA_SPOOF_JS, provider_ua_spoof(), provider_recipe_js(), and RUNTIME_JS) uses that unified value so provider_user_agent() and the injected ua_spoof.js always match.
🤖 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/cdp/target.rs`:
- Around line 30-33: The JSON parse error in browser_ws_url currently uses the
`?` on `resp.json().await`, which causes an early return on parse failure and
prevents trying the fallback host; change the logic around
`client.get(&url).send().await`/`resp.json().await` so parse failures are
handled locally (e.g., map_err to log or convert to an Option/continue) instead
of propagating with `?`, allowing the loop to continue to the next host; apply
the same change to the second occurrence around lines handling
`resp.json().await` (the v.get("webSocketDebuggerUrl") block) so both parse
errors fall back instead of exiting.
In `@app/src-tauri/src/webview_accounts/mod.rs`:
- Around line 1119-1136: The functions webview_notification_permission_state and
webview_notification_permission_request must not return compile-time constants;
replace the #[cfg(feature = "cef")] branch that returns "granted" with a real
runtime check and request flow that queries the OS/CEF runtime for the current
notification permission and request result. Concretely, implement or call a
platform/CEF-specific helper (e.g., check_notification_permission_cef or
request_notification_permission_cef) from webview_notification_permission_state
and webview_notification_permission_request respectively, return the actual
states ("granted", "denied", "default") based on the OS/CEF API response, and
ensure webview_notification_permission_request triggers the OS-level prompt and
returns that prompt's outcome instead of delegating to
webview_notification_permission_state's hardcoded value.
- Around line 368-377: The code logs payload.silent but still shows native
toasts and emits the event; update the notification flow in the function that
logs at the log::info call so that when payload.silent is true you skip emitting
"webview-notification:fired" and do not call builder.show() (or any code path
that surfaces an OS toast), while preserving any internal/webview-only
notification handling. Locate the places referencing payload.silent, the emit
call for "webview-notification:fired", and the builder.show() invocation and
gate them behind a clear conditional (e.g., if !payload.silent) so silent web
notifications do not reach the native toast path.
In `@app/src-tauri/src/webview_accounts/ua_spoof.js`:
- Around line 162-176: Before installing the Function.prototype.toString proxy
and Notification wrapper, check a guard flag (window.__OH_NOTIF_SHIM) and bail
out if already set; then set window.__OH_NOTIF_SHIM = true after successful
installation. Specifically, wrap the logic that creates __nativeFnToString,
__wrappedRequest, the proxy for Function.prototype.toString (which inspects
__OHNotification and __wrappedRequest) and the Notification wrapper with an if
(!window.__OH_NOTIF_SHIM) { ... window.__OH_NOTIF_SHIM = true; } guard so
repeated injections do not stack proxies or wrappers.
---
Outside diff comments:
In `@app/src-tauri/src/webview_accounts/mod.rs`:
- Around line 515-533: The UA spoofing JS currently hardcodes a Chrome version
that differs from the Rust CHROME_UA constant, causing inconsistent
fingerprints; update the code to derive the spoof string from the shared
CHROME_UA constant instead of a literal. Concretely, change UA_SPOOF_JS so it is
produced or formatted using CHROME_UA (or expose a function that returns the
spoof JS using CHROME_UA) and ensure the code path that builds the final payload
(the format! call that references UA_SPOOF_JS, provider_ua_spoof(),
provider_recipe_js(), and RUNTIME_JS) uses that unified value so
provider_user_agent() and the injected ua_spoof.js always match.
🪄 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: 75faf977-934a-4ba1-9a42-136eb09269b2
📒 Files selected for processing (6)
app/src-tauri/src/cdp/session.rsapp/src-tauri/src/cdp/target.rsapp/src-tauri/src/lib.rsapp/src-tauri/src/webview_accounts/mod.rsapp/src-tauri/src/webview_accounts/ua_spoof.jsapp/src-tauri/vendor/tauri-cef
✅ Files skipped from review due to trivial changes (1)
- app/src-tauri/vendor/tauri-cef
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src-tauri/src/cdp/session.rs
Comment on lines
+1119
to
+1136
| /// Web-shape notification permission state used by frontend parity code. | ||
| /// CEF path is effectively granted because interception is handled in-app. | ||
| #[tauri::command] | ||
| pub fn webview_notification_permission_state() -> String { | ||
| #[cfg(feature = "cef")] | ||
| { | ||
| "granted".to_string() | ||
| } | ||
| #[cfg(not(feature = "cef"))] | ||
| { | ||
| "default".to_string() | ||
| } | ||
| } | ||
|
|
||
| /// Request notification permission and return web-shape state. | ||
| #[tauri::command] | ||
| pub fn webview_notification_permission_request() -> String { | ||
| webview_notification_permission_state() |
Contributor
There was a problem hiding this comment.
Permission state/request cannot be hardcoded to "granted".
On CEF these commands always report success, and webview_notification_permission_request() never reaches the OS permission flow. If desktop notifications are disabled at the OS level, the frontend will stop prompting while native toasts still fail or get dropped. These commands need to reflect the real permission state/request result instead of a compile-time constant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/webview_accounts/mod.rs` around lines 1119 - 1136, The
functions webview_notification_permission_state and
webview_notification_permission_request must not return compile-time constants;
replace the #[cfg(feature = "cef")] branch that returns "granted" with a real
runtime check and request flow that queries the OS/CEF runtime for the current
notification permission and request result. Concretely, implement or call a
platform/CEF-specific helper (e.g., check_notification_permission_cef or
request_notification_permission_cef) from webview_notification_permission_state
and webview_notification_permission_request respectively, return the actual
states ("granted", "denied", "default") based on the OS/CEF API response, and
ensure webview_notification_permission_request triggers the OS-level prompt and
returns that prompt's outcome instead of delegating to
webview_notification_permission_state's hardcoded value.
Comment on lines
+162
to
+176
| var __nativeFnToString = Function.prototype.toString; | ||
| var __wrappedRequest = function () { | ||
| return Promise.resolve('granted'); | ||
| }; | ||
| Function.prototype.toString = new Proxy(__nativeFnToString, { | ||
| apply: function (target, thisArg, args) { | ||
| if (thisArg === __OHNotification) { | ||
| return 'function Notification() { [native code] }'; | ||
| } | ||
| if (thisArg === __wrappedRequest) { | ||
| return 'function requestPermission() { [native code] }'; | ||
| } | ||
| return Reflect.apply(target, thisArg, args); | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Guard the shim before patching global prototypes.
Line 197 sets window.__OH_NOTIF_SHIM, but nothing checks it before installing the Function.prototype.toString proxy and Notification wrapper. app/src-tauri/src/cdp/session.rs:140-170 already injects an equivalent notification-permission shim on CEF, so repeated evaluations will keep stacking wrappers onto the same page globals.
🛡️ Suggested hardening
(function () {
+ if (window.__OH_NOTIF_SHIM) {
+ return;
+ }
+ window.__OH_NOTIF_SHIM = true;
+
const CHROME_MAJOR = '124';
@@
- window.__OH_NOTIF_SHIM = true;
})();Also applies to: 197-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/webview_accounts/ua_spoof.js` around lines 162 - 176,
Before installing the Function.prototype.toString proxy and Notification
wrapper, check a guard flag (window.__OH_NOTIF_SHIM) and bail out if already
set; then set window.__OH_NOTIF_SHIM = true after successful installation.
Specifically, wrap the logic that creates __nativeFnToString, __wrappedRequest,
the proxy for Function.prototype.toString (which inspects __OHNotification and
__wrappedRequest) and the Notification wrapper with an if
(!window.__OH_NOTIF_SHIM) { ... window.__OH_NOTIF_SHIM = true; } guard so
repeated injections do not stack proxies or wrappers.
…settings routes - app/src/components/settings/hooks/useSettingsNavigation.ts: resolve the new `/settings/notifications` and `/settings/notification-routing` URLs to their SettingsRoute values and feed them into breadcrumbs so the new panels don't silently fall through to `'home'`. Addresses CodeRabbit on useSettingsNavigation.ts:34. - src/openhuman/notifications/schemas.rs: add the optional `reason` output on `notification.ingest` (populated alongside `skipped=true` by the runtime) and the normalized `settings` output on `notification.settings_set` so schema-driven clients see the full response shape. Addresses CodeRabbit on schemas.rs:103 and schemas.rs:217. - src/core/all.rs: add a `notification` namespace_description so CLI help covers the new controllers, plus a test assertion. Addresses CodeRabbit on src/core/all.rs:149.
…n on bad scored_at - Add `tracing::trace!` checkpoints around the `with_connection` DB open and schema migration so notification-delivery issues are reconstructible from logs. - `update_triage` and `mark_read` now inspect `Connection::execute`'s affected-row count: log a `warn!` when the update matched zero rows (row deleted between ingest and scoring / client passed a stale id), `debug!` on the normal path. - `scored_at` parsing no longer silently drops malformed values — log a `warn!` with the raw value and parse error before treating the row as unscored, matching the existing behavior for `received_at`. Addresses CodeRabbit on store.rs (lines 72, 172, 294).
…shim idempotency - webview_accounts/mod.rs: honor the Web Notification `silent` flag. Previously we only logged it and still called `builder.show()`, so pages that marked a notification silent still produced an OS toast. Mirror event still fires so the in-app center updates; only the OS toast is suppressed. Also picks up a prior cargo-fmt rewrap. - cdp/target.rs: `browser_ws_url()` now continues the host loop when `resp.json()` fails instead of early-returning via `?`. A malformed response from the first host (CDP_HOST) no longer prevents the `localhost` fallback from being tried. - webview_accounts/ua_spoof.js: guard the Notification wrapper behind `window.__OH_NOTIF_SHIM` so repeated evaluations of the script (Page.addScriptToEvaluateOnNewDocument + frame-level re-injections) don't stack wrappers onto the same page globals or re-proxy `Function.prototype.toString`. Addresses CodeRabbit on webview_accounts/mod.rs:377, cdp/target.rs:33, and ua_spoof.js:176.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/openhuman/notifications/store.rs (2)
171-196:⚠️ Potential issue | 🟠 MajorDon't return
Ok(())when the target row does not exist.Both
update_triageandmark_readonly warn whenupdated == 0. That makes callers believe the mutation succeeded even when the notification was already gone.Suggested fix
if updated == 0 { - // The row may have been deleted between ingest and scoring. - // Surface it at warn level so orphaned triage runs don't fail - // silently. - tracing::warn!( - id = %id, - action = %action, - "[notifications::store] update_triage matched no rows" - ); + anyhow::bail!("[notifications::store] update_triage missing notification: {id}"); } else { tracing::debug!( id = %id, action = %action, score = score, @@ if updated == 0 { - tracing::warn!( - id = %id, - "[notifications::store] mark_read matched no rows" - ); + anyhow::bail!("[notifications::store] mark_read missing notification: {id}"); } else { tracing::debug!(id = %id, "[notifications::store] mark_read applied"); }#!/bin/bash # Verify the current zero-row behavior and the existing test coverage around these mutations. sed -n '169,218p' src/openhuman/notifications/store.rs printf '\n--- tests ---\n' sed -n '397,423p' src/openhuman/notifications/store.rsExpected result: both functions currently log on
updated == 0and still return success, while the tests only exercise the happy path.Also applies to: 203-218
🤖 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 171 - 196, The current update_triage and mark_read functions log a warning when the UPDATE affected zero rows but still return Ok(()), which hides failures; change both functions (update_triage and mark_read) to return an Err when updated == 0 instead of Ok(()). Specifically, after executing the UPDATE and checking `updated == 0`, construct and return a suitable error (use the same error type/context pattern as other DB errors, e.g., with .context or anyhow::anyhow including the id and action/read indicator) so callers can detect the missing-row case; update existing tests to assert the error path when the target row is absent and keep the happy-path tests unchanged.
67-75:⚠️ Potential issue | 🟠 MajorSet a SQLite busy timeout before running migrations.
Each store call opens a fresh connection. Without
busy_timeout, concurrent writers can fail fast withdatabase is lockedinstead of waiting briefly for the active transaction to finish.Suggested fix
let conn = Connection::open(&db_path).with_context(|| { format!( "[notifications::store] failed to open DB at {}", db_path.display() ) })?; + conn + .busy_timeout(std::time::Duration::from_secs(5)) + .context("[notifications::store] failed to configure sqlite busy timeout")?; conn.execute_batch(SCHEMA) .context("[notifications::store] schema migration failed")?;#!/bin/bash # Verify that `with_connection` still opens a new connection without configuring # a busy timeout, and show existing timeout usage elsewhere in the repo. sed -n '47,76p' src/openhuman/notifications/store.rs printf '\n--- existing busy_timeout usage ---\n' rg -n -C2 --type=rust 'busy_timeout|SQLITE_BUSY_TIMEOUT' src/openhumanExpected result:
with_connectionshows nobusy_timeout(...)call today, while other stores/constants expose the timeout pattern you can reuse.🤖 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 67 - 75, The DB connection opened via Connection::open in the notifications store should set a SQLite busy timeout before running migrations to avoid transient "database is locked" errors; after creating the Connection (the variable named conn) call its busy_timeout(...) with a short Duration (reusing your repo's SQLITE_BUSY_TIMEOUT constant or Duration::from_millis/seconds) before running conn.execute_batch(SCHEMA), ensuring you import std::time::Duration and handle the Result from busy_timeout appropriately.
🧹 Nitpick comments (3)
src/openhuman/notifications/schemas.rs (2)
293-307: Rename test to reflect current behavior (fivefunctions, notthree).The assertion validates 5 functions, so the test name is now misleading.
✏️ Proposed rename
- fn all_controller_schemas_covers_three_functions() { + fn all_controller_schemas_covers_five_functions() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/notifications/schemas.rs` around lines 293 - 307, The test function all_controller_schemas_covers_three_functions is misnamed because it asserts five controller schema function names ("ingest", "list", "mark_read", "settings_get", "settings_set"); rename the test to something accurate (e.g., all_controller_schemas_covers_five_functions or all_controller_schemas_includes_five_functions) and update any references or doc comments to match the new name while leaving the assertion and mapped names unchanged.
264-282: Add trace/debug logging at controller wrapper entry points.These wrapper functions are controller entry points and external-call boundaries into
rpc.rs, but they currently provide no diagnostics for invocation/branch/error tracing.🔧 Proposed change
fn handle_ingest_wrap(params: Map<String, Value>) -> ControllerFuture { + tracing::trace!(target: "openhuman::notifications::schemas", "handle_ingest_wrap called"); Box::pin(async move { super::rpc::handle_ingest(params).await }) } fn handle_list_wrap(params: Map<String, Value>) -> ControllerFuture { + tracing::trace!(target: "openhuman::notifications::schemas", "handle_list_wrap called"); Box::pin(async move { super::rpc::handle_list(params).await }) } fn handle_mark_read_wrap(params: Map<String, Value>) -> ControllerFuture { + tracing::trace!(target: "openhuman::notifications::schemas", "handle_mark_read_wrap called"); Box::pin(async move { super::rpc::handle_mark_read(params).await }) } fn handle_settings_get_wrap(params: Map<String, Value>) -> ControllerFuture { + tracing::trace!(target: "openhuman::notifications::schemas", "handle_settings_get_wrap called"); Box::pin(async move { super::rpc::handle_settings_get(params).await }) } fn handle_settings_set_wrap(params: Map<String, Value>) -> ControllerFuture { + tracing::trace!(target: "openhuman::notifications::schemas", "handle_settings_set_wrap called"); Box::pin(async move { super::rpc::handle_settings_set(params).await }) }As per coding guidelines, "
src/**/*.rs: Rust code must uselog/tracingatdebugortracelevel ... at entry/exit points, branch decisions, external calls, ... and error handling paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/notifications/schemas.rs` around lines 264 - 282, Each controller wrapper (handle_ingest_wrap, handle_list_wrap, handle_mark_read_wrap, handle_settings_get_wrap, handle_settings_set_wrap) needs trace/debug instrumentation at its entry and around the call into super::rpc so invocations and failures can be diagnosed; modify each function to log at the start (e.g., tracing::trace! or log::debug!) with the wrapper name and a lightweight summary of params, then await the rpc call inside the async block capturing the Result and log a trace/debug on success and an error/trace on failure before returning the result; ensure you reference the same function names (handle_*_wrap) and keep the return type ControllerFuture and Box::pin(...) behavior unchanged.src/openhuman/notifications/store.rs (1)
18-35: Index thereceived_atsort path now.
list()always orders byreceived_at DESC, but the schema only indexesproviderandstatus. As this table grows, page fetches will degrade into scan-and-sort work, especially for provider-scoped views.Suggested fix
CREATE INDEX IF NOT EXISTS idx_integration_notifications_provider ON integration_notifications(provider); CREATE INDEX IF NOT EXISTS idx_integration_notifications_status ON integration_notifications(status); +CREATE INDEX IF NOT EXISTS idx_integration_notifications_received_at + ON integration_notifications(received_at DESC); +CREATE INDEX IF NOT EXISTS idx_integration_notifications_provider_received_at + ON integration_notifications(provider, received_at DESC);🤖 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 18 - 35, Add an index to support the ORDER BY received_at DESC used by list(): update the integration_notifications schema to create an index on received_at and, for provider-scoped queries, add a composite index (e.g. ON integration_notifications(provider, received_at DESC) or ON (status, received_at DESC)) so scans avoid file-sort; reference the integration_notifications table and the received_at column and ensure the new index(s) are created with IF NOT EXISTS alongside the existing provider/status indexes.
🤖 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/webview_accounts/mod.rs`:
- Around line 1131-1149: The function webview_notification_permission_request()
currently only calls webview_notification_permission_state() and does not
perform any request; either implement a real request flow or rename it to
indicate it's a state query. Update the doc comment near
webview_notification_permission_state() and
webview_notification_permission_request() to clarify that CEF builds auto-grant
("granted") while non-CEF should reflect actual OS capability; for non-CEF
replace the hardcoded "default" with a capability/permission check (e.g., use
tauri-plugin-notification or the platform-specific API to determine
support/permission) or if you prefer renaming, change
webview_notification_permission_request to
webview_notification_permission_get_state (and update frontend call sites) so
the API name matches behavior. Ensure references to
webview_notification_permission_state and
webview_notification_permission_request are adjusted consistently.
---
Duplicate comments:
In `@src/openhuman/notifications/store.rs`:
- Around line 171-196: The current update_triage and mark_read functions log a
warning when the UPDATE affected zero rows but still return Ok(()), which hides
failures; change both functions (update_triage and mark_read) to return an Err
when updated == 0 instead of Ok(()). Specifically, after executing the UPDATE
and checking `updated == 0`, construct and return a suitable error (use the same
error type/context pattern as other DB errors, e.g., with .context or
anyhow::anyhow including the id and action/read indicator) so callers can detect
the missing-row case; update existing tests to assert the error path when the
target row is absent and keep the happy-path tests unchanged.
- Around line 67-75: The DB connection opened via Connection::open in the
notifications store should set a SQLite busy timeout before running migrations
to avoid transient "database is locked" errors; after creating the Connection
(the variable named conn) call its busy_timeout(...) with a short Duration
(reusing your repo's SQLITE_BUSY_TIMEOUT constant or
Duration::from_millis/seconds) before running conn.execute_batch(SCHEMA),
ensuring you import std::time::Duration and handle the Result from busy_timeout
appropriately.
---
Nitpick comments:
In `@src/openhuman/notifications/schemas.rs`:
- Around line 293-307: The test function
all_controller_schemas_covers_three_functions is misnamed because it asserts
five controller schema function names ("ingest", "list", "mark_read",
"settings_get", "settings_set"); rename the test to something accurate (e.g.,
all_controller_schemas_covers_five_functions or
all_controller_schemas_includes_five_functions) and update any references or doc
comments to match the new name while leaving the assertion and mapped names
unchanged.
- Around line 264-282: Each controller wrapper (handle_ingest_wrap,
handle_list_wrap, handle_mark_read_wrap, handle_settings_get_wrap,
handle_settings_set_wrap) needs trace/debug instrumentation at its entry and
around the call into super::rpc so invocations and failures can be diagnosed;
modify each function to log at the start (e.g., tracing::trace! or log::debug!)
with the wrapper name and a lightweight summary of params, then await the rpc
call inside the async block capturing the Result and log a trace/debug on
success and an error/trace on failure before returning the result; ensure you
reference the same function names (handle_*_wrap) and keep the return type
ControllerFuture and Box::pin(...) behavior unchanged.
In `@src/openhuman/notifications/store.rs`:
- Around line 18-35: Add an index to support the ORDER BY received_at DESC used
by list(): update the integration_notifications schema to create an index on
received_at and, for provider-scoped queries, add a composite index (e.g. ON
integration_notifications(provider, received_at DESC) or ON (status, received_at
DESC)) so scans avoid file-sort; reference the integration_notifications table
and the received_at column and ensure the new index(s) are created with IF NOT
EXISTS alongside the existing provider/status indexes.
🪄 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: f9b00ceb-efd0-408d-abf5-311aaf893edc
📒 Files selected for processing (7)
app/src-tauri/src/cdp/target.rsapp/src-tauri/src/webview_accounts/mod.rsapp/src-tauri/src/webview_accounts/ua_spoof.jsapp/src/components/settings/hooks/useSettingsNavigation.tssrc/core/all.rssrc/openhuman/notifications/schemas.rssrc/openhuman/notifications/store.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/components/settings/hooks/useSettingsNavigation.ts
- src/core/all.rs
Comment on lines
+1131
to
+1149
| /// Web-shape notification permission state used by frontend parity code. | ||
| /// CEF path is effectively granted because interception is handled in-app. | ||
| #[tauri::command] | ||
| pub fn webview_notification_permission_state() -> String { | ||
| #[cfg(feature = "cef")] | ||
| { | ||
| "granted".to_string() | ||
| } | ||
| #[cfg(not(feature = "cef"))] | ||
| { | ||
| "default".to_string() | ||
| } | ||
| } | ||
|
|
||
| /// Request notification permission and return web-shape state. | ||
| #[tauri::command] | ||
| pub fn webview_notification_permission_request() -> String { | ||
| webview_notification_permission_state() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does tauri-plugin-notification provide a permission check API to query OS notification permission state?
💡 Result:
Yes, tauri-plugin-notification provides a permission check API called isPermissionGranted to query the OS notification permission state. It returns a Promise indicating if permission is granted. Usage example from official docs: import { isPermissionGranted } from '@tauri-apps/plugin-notification'; let permissionGranted = await isPermissionGranted;
Citations:
- 1: https://tauri.app/reference/javascript/notification/
- 2: https://v2.tauri.app/plugin/notification/
- 3: https://www.npmjs.com/package/@tauri-apps/plugin-notification
- 4: https://github.com/tauri-apps/tauri-plugin-notification
- 5: https://tauri.app/plugin/notification
🏁 Script executed:
# Search for where these notification permission functions are called
rg "webview_notification_permission_state|webview_notification_permission_request" --type rust --type ts --type jsRepository: tinyhumansai/openhuman
Length of output: 792
🏁 Script executed:
# Check tauri-plugin-notification usage in the codebase
rg "tauri-plugin-notification|isPermissionGranted|permission.*notification" --type toml --type ts --type js --type rustRepository: tinyhumansai/openhuman
Length of output: 632
🏁 Script executed:
# Look for notification-related code in the frontend
fd -e ts -e tsx -e js -e jsx | xargs grep -l "notification" | head -20Repository: tinyhumansai/openhuman
Length of output: 936
🏁 Script executed:
# Search for how permissions are generally handled in webview_accounts
rg "permission" app/src-tauri/src/webview_accounts/ -A 2 -B 2 | head -100Repository: tinyhumansai/openhuman
Length of output: 5103
🏁 Script executed:
# Look at how webviewAccountService uses the permission functions
cat -n app/src/services/webviewAccountService.ts | grep -A 10 -B 10 "webview_notification_permission"Repository: tinyhumansai/openhuman
Length of output: 1187
🏁 Script executed:
# Check NotificationsPanel to understand the frontend UX expectations
cat -n app/src/components/settings/panels/NotificationsPanel.tsx | head -100Repository: tinyhumansai/openhuman
Length of output: 4715
🏁 Script executed:
# Look for any permission request logic in tauri plugin usage
rg "requestPermission|permission.*request" app/src --type ts --type tsx -A 3 -B 3Repository: tinyhumansai/openhuman
Length of output: 93
🏁 Script executed:
# Check git history or comments around webview_notification_permission_request
rg -A 5 -B 5 "webview_notification_permission_request" app/src-tauri/src/webview_accounts/mod.rsRepository: tinyhumansai/openhuman
Length of output: 344
🏁 Script executed:
# Look for any TODO or FIXME comments about notification permissions
rg "TODO|FIXME|todo|fixme" app/src-tauri/src/webview_accounts/mod.rsRepository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
# Check if there's any other permission request logic in the codebase
rg "requestPermission" app/src-tauri --type rustRepository: tinyhumansai/openhuman
Length of output: 181
🏁 Script executed:
# Verify the exact usage context - check webviewAccountService more thoroughly
cat -n app/src/services/webviewAccountService.ts | head -150Repository: tinyhumansai/openhuman
Length of output: 5546
🏁 Script executed:
# Check if there's any permission request logic elsewhere in notification handling
rg "permission" app/src-tauri/src/webview_accounts/mod.rs -A 5 -B 5 | head -80Repository: tinyhumansai/openhuman
Length of output: 831
🏁 Script executed:
# Check the full context around these functions including what comes before
sed -n '1100,1160p' app/src-tauri/src/webview_accounts/mod.rsRepository: tinyhumansai/openhuman
Length of output: 1918
🏁 Script executed:
# Check if there's Tauri notification API for permission request in non-CEF
rg "tauri.*notification|notify" app/src-tauri/Cargo.tomlRepository: tinyhumansai/openhuman
Length of output: 468
🏁 Script executed:
# Look for how notifications are actually sent to understand the permission flow
rg "forward_native_notification|builder.show" app/src-tauri/src/webview_accounts/mod.rs -B 10 -A 3 | head -50Repository: tinyhumansai/openhuman
Length of output: 1642
The webview_notification_permission_request() function is misnamed—it doesn't request, just returns state.
This function (line 1147–1149) delegates directly to webview_notification_permission_state() without actually performing any request operation. The frontend calls it expecting a "request" action (per ensureNotificationPermission in webviewAccountService.ts), but it only returns the current state. Either implement actual permission request logic or rename it to clarify that it's a state query.
Additionally, the non-CEF path returns hardcoded "default" even though Tauri/non-CEF builds support OS notifications via tauri-plugin-notification. Consider reflecting the actual notification capability or permission state instead of a hardcoded string.
For CEF: The hardcoded "granted" is intentional by design (as documented in webviewAccountService.ts: "Desktop plugin auto-grants today"), since the runtime intercepts the Web Notifications API at the browser level. This design choice is acceptable but should be clarified in the doc comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/src/webview_accounts/mod.rs` around lines 1131 - 1149, The
function webview_notification_permission_request() currently only calls
webview_notification_permission_state() and does not perform any request; either
implement a real request flow or rename it to indicate it's a state query.
Update the doc comment near webview_notification_permission_state() and
webview_notification_permission_request() to clarify that CEF builds auto-grant
("granted") while non-CEF should reflect actual OS capability; for non-CEF
replace the hardcoded "default" with a capability/permission check (e.g., use
tauri-plugin-notification or the platform-specific API to determine
support/permission) or if you prefer renaming, change
webview_notification_permission_request to
webview_notification_permission_get_state (and update frontend call sites) so
the API name matches behavior. Ensure references to
webview_notification_permission_state and
webview_notification_permission_request are adjusted consistently.
This was referenced Apr 22, 2026
Merged
6 tasks
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-cefnotification intercept intotauri-plugin-notificationso embedded apps (Slack, Discord, Gmail, WhatsApp) produce real native OS toasts instead of silently dropping them[Slack] New message from Alice) for clear attribution; honoursilent,icon, andtagfrom the Web Notification APINotificationRoutekeyed by{provider}:{account_id}:{tag_or_uuid}to enable click-to-focus routing once platform delegates (UNUserNotificationCenter / notify-rust) are wiredwebview_notification_permission_state/webview_notification_permission_request) so the frontend sees"granted" | "denied" | "default"on both CEF and wry runtimesProblem
Embedded webview apps call
window.Notificationinside CEF but never produce native macOS/Windows toasts. The CEF runtime intercepts the web Notification API but the intercept was dropped on the floor — no bridge totauri-plugin-notification, no click routing back to the originating account, no permission query/request pipeline.Solution
Rust (
app/src-tauri/src/webview_accounts/mod.rs)NotificationRoutestruct:provider,account_id,tag,created_atnotification_routes: Mutex<HashMap<String, NotificationRoute>>onWebviewAccountStateclear_notification_routes(account_id)— purged on close/purgeforward_native_notification(app, provider, account_id, payload)— prefixes title, respectssilent/icon/tag, falls back to monotonic timestamp for untagged payloadswebview_notification_permission_state/webview_notification_permission_request— map 4-varianttauri::plugin::PermissionStateonto 3-value web API shape; non-CEF stubs always return"default"setup:tauri_runtime_cef::notification::registerwith handler callingforward_native_notification;unregisteron account closeFrontend (
app/src/services/webviewAccountService.ts)ensureNotificationPermission(accountId)— invokes permission state, requests if"default", runs once per session; called afteropenWebviewAccounthandleNotificationClick+listen('notification:click', …)— dispatchessetActiveAccountand invokesactivate_main_windowwhen the dormant platform click hook firesCapabilities — added
notification:allow-notify,notification:allow-request-permission,notification:allow-is-permission-grantedSubmission Checklist
.app, open Slack/Discord/Gmail in a webview account, trigger a notification, confirm provider-prefixed OS toast appears.webview_accounts/mod.rsImpact
notification:clickyet — follow-up work to wire UNUserNotificationCenter / notify-rust delegates.Related
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Documentation