Announce a new memory with a native desktop notification - #1440
Announce a new memory with a native desktop notification#1440rohan-pandeyy wants to merge 13 commits into
Conversation
Collapsible group beside Video Tagging, holding the generate and desktop notification toggles plus dropdowns for seconds per photo and the photo count bounds. The min and max pair still cross-clamps, since a value saved by the old sliders can fall outside these options.
Its controls now live in the settings page, so the page, the memories/settings route and ROUTES.MEMORIES_SETTINGS all go. The gear buttons on the grid and in the story viewer point at /settings instead.
Alerts are opt-in now; the memory is waiting on the page either way. Flipped in MemoriesPreferences, the MemoryStatusData echo and the frontend defaults, with openapi.json regenerated to match.
The switches already did this, the three dropdown triggers did not. Rapid selections could overlap, and each mutation rolls back to its own snapshot, so an out-of-order failure could leave the min/max pair inconsistent.
Every write snapshotted and restored the whole preferences object, so two overlapping saves would have the later one roll back over the earlier one. Writes now queue, each builds from the state that actually landed before it, and each sends only the keys it changed so the server merge cannot clobber a concurrent edit either. Drops the unused updatePreference, which was the whole-object path.
Both directions of the min and max cross-clamp, a stored value that is not one of the dropdown options, and every control disabled while a save runs.
reqwest gains the json feature and tokio gains time, both needed by the curation task. notify-rust is pinned to 4.17.0 because 4.18 needs rustc 1.89. Nothing goes under plugins in tauri.conf.json: the plugin takes no config and even an empty map panics at startup.
The backend only curates when asked, so nothing surfaced on a day with no import. This waits for the backend and for indexing to settle, requests a run, then raises a native notification for the memory it produced. The generate response decides how to wait: run_started_at only identifies our own run, so a run that was already queued or complete would otherwise burn the full timeout on every launch. Focus re-runs the check at most every 6 hours, which covers the app being left open across midnight.
Rust emits memory:pending whether or not it notified, because desktop notification clicks are not dependable across platforms, so the in-app card is the path that always works. Clicking it goes through the open_memory command, which focuses the window and emits memory:open. Lives inside the router so a memory opens from any page.
…d on macOS and Linux gate notifications behind an OS prompt, and the switch is the only moment a user expects one. The preference saves either way; the desktop task re-checks the permission before every notification.
Card, dismissal, both events, and the fallback when the command is missing.
WalkthroughThe PR adds automatic daily memory curation with native and in-app notifications. It moves memory controls into general settings, serializes preference updates with rollback isolation, removes the dedicated memory settings route, and changes notification defaults to opt-in. ChangesSmart Memories desktop flow
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/components/Memories/MemoryStoryViewer.tsx (1)
292-299: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
aria-labelstill says "Memory settings" after the route consolidation.The button now navigates to
ROUTES.SETTINGS(the general Settings page), butaria-label="Memory settings"at Line 295 still describes it as memory-specific settings. This finding shares one root cause with the same mismatch inMemories.tsx; see the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/Memories/MemoryStoryViewer.tsx` around lines 292 - 299, Update the settings navigation button in MemoryStoryViewer so its aria-label describes the general ROUTES.SETTINGS destination rather than memory-specific settings. Change only the accessible label while preserving the existing onClick navigation and styling.frontend/src/pages/Memories/Memories.tsx (1)
108-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
aria-labelstill says "Memory settings" after the route consolidation.Same issue as
MemoryStoryViewer.tsxLine 295: the button now navigates toROUTES.SETTINGS, but the label still reads "Memory settings". See the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/Memories/Memories.tsx` around lines 108 - 116, Update the settings navigation Button in Memories.tsx to use an aria-label that reflects the consolidated general settings destination instead of “Memory settings”; keep its existing ROUTES.SETTINGS navigation and styling unchanged.frontend/src/hooks/useUserPreferences.tsx (1)
51-66: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize external
refetch()into the preference write model.
refetchreachesUserPreferencesCarddirectly via Model Manager close and window focus. Its success applies its fetched data throughapplyPreferences, bypassingwriteQueue. If a queued write is in flight when the refetch stores newer values, a later write failure restores the write queue’s pre-write snapshot and discards the refetch result without a diagnostic. Either funnel refetched state throughwriteQueueor make rollback restore only the failed write’s own keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useUserPreferences.tsx` around lines 51 - 66, The preferencesQuery refetch path currently applies fetched preferences directly through applyPreferences, bypassing writeQueue and allowing rollback to overwrite newer refetched state. Update the useUserPreferences flow so external refetch results are serialized through writeQueue, or constrain rollback to restore only the failed write’s keys; preserve queued writes while retaining newer refetched values and emit diagnostics for failures.
🧹 Nitpick comments (2)
frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx (1)
84-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for the notification permission request.
toggleMemoryNotificationschecksisPermissionGranted()and callsrequestPermission()when the user enables desktop notifications. This is a core requirement from the linked issue. The companion test file only verifies that the switch is disabled when memory generation is off; no test clicks the switch totrueand assertsisPermissionGranted/requestPermissionare called, or that a warning is logged when the permission call fails.Do you want me to draft a test that mocks
@tauri-apps/plugin-notificationand asserts this flow?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx` around lines 84 - 100, Add tests for UserPreferencesCard’s toggleMemoryNotifications flow: mock `@tauri-apps/plugin-notification`, enable the switch, and assert isPermissionGranted is checked and requestPermission is called only when permission is absent; also cover the rejected permission request and verify console.warn is emitted.frontend/src-tauri/src/memories.rs (1)
123-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd Rust test coverage for the curation state machine.
This file introduces a non-trivial async orchestration flow: backend readiness polling, indexing wait, run-completion polling with dedup logic, permission gating, and notification dedup. None of this is covered by unit tests in the diff. Consider extracting the pure decision logic (e.g.,
is_ours/settledcomputation inwait_for_run, the guard insurface) into small, directly-testable functions, and add tests around the HTTP layer using a mockable client trait or a local test server.As per path instructions,
**/*: "Ensure that test code is automated, comprehensive, and follows testing best practices" and "Verify that all critical functionality is covered by tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src-tauri/src/memories.rs` around lines 123 - 378, Add automated Rust tests covering the curation state machine in run, wait_while_indexing, wait_for_run, surface, and notification deduplication. Extract pure decisions such as wait_for_run’s is_ours/settled logic and surface’s notification guard into testable helpers, and exercise HTTP polling/error paths through a mockable client or local test server while preserving existing behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src-tauri/src/memories.rs`:
- Around line 178-206: Update surface so the notifications_enabled and
memory.notified_at guard runs before emitting memory:pending, preventing
repeated cards and honoring the preference. After the card is emitted or the OS
notification is shown, persist notified_at via mark_notified regardless of
whether notify succeeds, ensuring both delivery paths are deduplicated.
- Around line 208-246: Move the synchronous has_permission check in notify into
tauri::async_runtime::spawn_blocking, await its result before proceeding to
build and show the notification, and return false when permission is unavailable
or the blocking task fails. Keep the notification construction and show flow
unchanged, while ensuring permission_state and request_permission no longer run
directly on the async runtime worker.
---
Outside diff comments:
In `@frontend/src/components/Memories/MemoryStoryViewer.tsx`:
- Around line 292-299: Update the settings navigation button in
MemoryStoryViewer so its aria-label describes the general ROUTES.SETTINGS
destination rather than memory-specific settings. Change only the accessible
label while preserving the existing onClick navigation and styling.
In `@frontend/src/hooks/useUserPreferences.tsx`:
- Around line 51-66: The preferencesQuery refetch path currently applies fetched
preferences directly through applyPreferences, bypassing writeQueue and allowing
rollback to overwrite newer refetched state. Update the useUserPreferences flow
so external refetch results are serialized through writeQueue, or constrain
rollback to restore only the failed write’s keys; preserve queued writes while
retaining newer refetched values and emit diagnostics for failures.
In `@frontend/src/pages/Memories/Memories.tsx`:
- Around line 108-116: Update the settings navigation Button in Memories.tsx to
use an aria-label that reflects the consolidated general settings destination
instead of “Memory settings”; keep its existing ROUTES.SETTINGS navigation and
styling unchanged.
---
Nitpick comments:
In `@frontend/src-tauri/src/memories.rs`:
- Around line 123-378: Add automated Rust tests covering the curation state
machine in run, wait_while_indexing, wait_for_run, surface, and notification
deduplication. Extract pure decisions such as wait_for_run’s is_ours/settled
logic and surface’s notification guard into testable helpers, and exercise HTTP
polling/error paths through a mockable client or local test server while
preserving existing behavior.
In `@frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx`:
- Around line 84-100: Add tests for UserPreferencesCard’s
toggleMemoryNotifications flow: mock `@tauri-apps/plugin-notification`, enable the
switch, and assert isPermissionGranted is checked and requestPermission is
called only when permission is absent; also cover the rejected permission
request and verify console.warn is emitted.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e6f2249-ae37-4051-be36-85e2bfa98ec8
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
backend/app/schemas/memories.pybackend/app/schemas/user_preferences.pybackend/tests/test_user_preferences.pydocs/backend/backend_python/openapi.jsonfrontend/jest.setup.tsfrontend/package.jsonfrontend/src-tauri/Cargo.tomlfrontend/src-tauri/capabilities/migrated.jsonfrontend/src-tauri/src/main.rsfrontend/src-tauri/src/memories.rsfrontend/src/App.tsxfrontend/src/components/Memories/MemoryNotificationListener.tsxfrontend/src/components/Memories/MemoryStoryViewer.tsxfrontend/src/components/Memories/__tests__/MemoryNotificationListener.test.tsxfrontend/src/constants/routes.tsfrontend/src/hooks/__tests__/useUserPreferences.test.tsxfrontend/src/hooks/useUserPreferences.tsxfrontend/src/pages/Memories/Memories.tsxfrontend/src/pages/Memories/MemorySettings.tsxfrontend/src/pages/SettingsPage/components/UserPreferencesCard.tsxfrontend/src/pages/SettingsPage/components/__tests__/UserPreferencesCard.test.tsxfrontend/src/routes/AppRoutes.tsx
💤 Files with no reviewable changes (3)
- frontend/src/constants/routes.ts
- frontend/src/routes/AppRoutes.tsx
- frontend/src/pages/Memories/MemorySettings.tsx
memory:pending was emitted before the notified_at guard, and /today keeps returning a memory until it is viewed or dismissed, so the in-app card came back on every launch and every resume. The guard now covers both paths and notified_at is recorded whenever the card goes out. Drops the Rust permission check with it: the plugin's desktop implementation returns Granted unconditionally, so it never did anything.
github-actions
Bot
added
GSoC 2026
backend
enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src-tauri/src/memories.rs (1)
197-213: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTrack memory-event delivery before setting
notified.
MemoryNotificationListenersubscribes only afterAppmounts, whilespawn_memory_task()runs immediately after window setup and can callmark_notified(). If curation emitsmemory:pendingbefore that subscription is established, the in-app card is missed but later launches seenotified: trueand skip it again. Use a listener-ready acknowledgement protocol, or defer emitting this event until frontend readiness is known, before persisting the notified state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src-tauri/src/memories.rs` around lines 197 - 213, Update the memory delivery flow around the event emission and mark_notified call so the memory is marked notified only after the frontend has acknowledged that its MemoryNotificationListener is ready and has received the memory:pending event. Add or reuse a listener-ready handshake, or defer emission until readiness is confirmed; preserve notification behavior while preventing missed in-app cards from being persisted as notified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/src/pages/SettingsPage/components/__tests__/UserPreferencesCard.test.tsx`:
- Around line 158-174: Extend the UserPreferencesCard permission-failure tests
to cover a false isPermissionGranted result followed by a rejected
requestPermission call. Mock that sequence, click the Desktop Notifications
switch, and assert notifications_enabled remains true and console.warn is
called, while preserving the existing rejected permission-check coverage.
---
Outside diff comments:
In `@frontend/src-tauri/src/memories.rs`:
- Around line 197-213: Update the memory delivery flow around the event emission
and mark_notified call so the memory is marked notified only after the frontend
has acknowledged that its MemoryNotificationListener is ready and has received
the memory:pending event. Add or reuse a listener-ready handshake, or defer
emission until readiness is confirmed; preserve notification behavior while
preventing missed in-app cards from being persisted as notified.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e67fcf6-38c0-4ef3-b617-e048051995f0
📒 Files selected for processing (2)
frontend/src-tauri/src/memories.rsfrontend/src/pages/SettingsPage/components/__tests__/UserPreferencesCard.test.tsx
| it('keeps the preference when the permission call fails', async () => { | ||
| const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| mockIsPermissionGranted.mockRejectedValue(new Error('unavailable')); | ||
| const user = userEvent.setup(); | ||
| render(<UserPreferencesCard />); | ||
| await openPanel(user); | ||
|
|
||
| await user.click( | ||
| screen.getByRole('switch', { name: /Desktop Notifications/i }), | ||
| ); | ||
|
|
||
| expect(mockUpdateMemoriesPreferences).toHaveBeenCalledWith({ | ||
| notifications_enabled: true, | ||
| }); | ||
| expect(warn).toHaveBeenCalled(); | ||
| warn.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test a rejected requestPermission() call.
Line 160 rejects isPermissionGranted(). This does not test the path where the permission check returns false and requestPermission() rejects. Add that case and assert that the preference remains enabled and console.warn is called.
As per path instructions, “Ensure that test code is automated, comprehensive, and follows testing best practices.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/src/pages/SettingsPage/components/__tests__/UserPreferencesCard.test.tsx`
around lines 158 - 174, Extend the UserPreferencesCard permission-failure tests
to cover a false isPermissionGranted result followed by a rejected
requestPermission call. Mock that sequence, click the Desktop Notifications
switch, and assert notifications_enabled remains true and console.warn is
called, while preserving the existing rejected permission-check coverage.
Source: Path instructions
1 participant
Closes #1439.
What this does
Memories only got curated when an import or a sync happened to trigger it, and nothing ever told you a new one was ready. This adds a background task in the desktop shell that asks for a run on launch, then announces the result through the operating system's own notification centre.
Acting on the notification takes you to the memory in the story viewer.
How it works
frontend/src-tauri/src/memories.rsholds the whole task:/health, since the backend is spawned alongside the windowGET /memories/status, and stop here if the user has memories turned offindexing_busy, up to 30 minutesPOST /memories/generateGET /memories/today, then emit, notify, andPATCH {notified: true}A few decisions that are not obvious from the code:
The generate response decides how to wait, not just
run_started_at. Aqueued: falseresponse means today's run is either already complete or already someone else's. Only our own run can be told apart by its start time, so waiting on that value to change in either of those cases would burn the full 30 minute timeout on every launch. Not queued and running gets waited out without the check; not queued and complete skips straight to reading status.Never curate mid-index. A run that starts while indexing is in progress scores a library whose labels are only half written, so the task waits for it to settle rather than racing it.
notified_atis what stops a memory being announced twice across launches, andnotifications_enabledis re-read after the run settles so toggling it mid-run is honoured.Focus re-runs the check, throttled to once every 6 hours and guarded by an
AtomicBool. That covers the app being left open across midnight, which matters because anniversary memories are written withsurface_date = todayand are never generated retroactively.Routing the user to the memory
Desktop notification click callbacks are not dependable in Tauri v2, so the OS notification is announcement only. Rust emits
memory:pendingwhether or not it notified, andMemoryNotificationListenerrenders an in-app card from it. The card mounts inside the router inApp.tsx, so a memory opens from any page.Clicking the card goes through the
open_memorycommand, which unminimizes and focuses the window before emittingmemory:open. That keeps a single entry point for routing, and the card falls back to routing locally if the command is unavailable, as it is outside the desktop shell.Turning on Desktop Notifications in settings now also calls
requestPermission(), since macOS and Linux gate on an OS prompt and the switch is the only moment a user expects one. The preference saves either way and Rust re-checks the permission before every notification.Notes for reviewers
notify-rustis pinned to 4.17.0 inCargo.lock. 4.18 raised its MSRV to rustc 1.89, so on 1.88 the resolver refuses to build. A barecargo updatewill undo the pin.pluginsintauri.conf.json. The notification plugin's config type is(), so even"notification": {}panics at startup withinvalid type: map, expected unit. The"notification:default"entry incapabilities/migrated.jsonis the whole of the wiring.src-taurihas no#[cfg(test)]anywhere, so no Rust test harness was invented for this. The task itself was verified by hand.Verification
cargo clippyreports nothing in the new file. The two warnings it does emit are pre-existing inmain.rsand were left alone.Summary by CodeRabbit
New Features
Improvements