Move memory settings into the settings page - #1438
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.
WalkthroughMemory notifications now default to opt-in. Memory controls moved into the main settings page. The dedicated memory settings route and page wiring were removed. The preference update hook now serializes writes through a queue to prevent concurrent-edit races, with optimistic updates and targeted rollback on failure. ChangesMemory settings consolidation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant MemoriesPanel
participant useUserPreferences
participant PreferencesAPI
User->>MemoriesPanel: Change a Memories setting
MemoriesPanel->>useUserPreferences: patchMemories(partial update)
useUserPreferences->>useUserPreferences: Queue write from latest state
useUserPreferences-->>MemoriesPanel: Apply optimistic update
useUserPreferences->>PreferencesAPI: POST {changed_fields_only}
alt API succeeds
PreferencesAPI-->>useUserPreferences: Return updated preferences
useUserPreferences->>useUserPreferences: Apply next queued write
else API fails
PreferencesAPI-->>useUserPreferences: Error response
useUserPreferences-->>useUserPreferences: Rollback failed write
useUserPreferences->>useUserPreferences: Continue with next write
end
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/UserPreferencesCard.tsx`:
- Around line 484-508: Disable the dropdown trigger Buttons for the memories
duration and related preference dropdowns by adding disabled={isUpdating},
matching the existing switch behavior. Update the buttons around the memories
duration control and the corresponding controls near the second and third
dropdown sections so they cannot start overlapping patchMemories mutations while
an update is active.
🪄 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: a058ddde-e3e8-423d-9ab4-ba673f5e960f
📒 Files selected for processing (11)
backend/app/schemas/memories.pybackend/app/schemas/user_preferences.pybackend/tests/test_user_preferences.pydocs/backend/backend_python/openapi.jsonfrontend/src/components/Memories/MemoryStoryViewer.tsxfrontend/src/constants/routes.tsfrontend/src/hooks/useUserPreferences.tsxfrontend/src/pages/Memories/Memories.tsxfrontend/src/pages/Memories/MemorySettings.tsxfrontend/src/pages/SettingsPage/components/UserPreferencesCard.tsxfrontend/src/routes/AppRoutes.tsx
💤 Files with no reviewable changes (3)
- frontend/src/constants/routes.ts
- frontend/src/pages/Memories/MemorySettings.tsx
- frontend/src/routes/AppRoutes.tsx
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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx (1)
73-78: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize all preference mutations or disable every dependent control.
isUpdatingdisables only the Memories controls, while YOLO, GPU, and video controls can still start mutations through the sameupdatePreferencesMutation. Because both update paths snapshot and restore the entire preferences object, a concurrent non-Memory update can be lost from local state when the Memory request fails or completes out of order. Serialize these writes inuseUserPreferences, or disable all controls sharing this mutation until it settles.🤖 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 73 - 78, Serialize all preference mutations in useUserPreferences, including the patchMemories path and the YOLO, GPU, and video update paths, so only one updatePreferencesMutation can run at a time. Ensure dependent controls cannot trigger concurrent writes, preserving the existing snapshot/restore behavior without allowing out-of-order updates to overwrite local preferences.
🧹 Nitpick comments (1)
frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx (1)
381-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd automated coverage for the Memories panel.
Please test min/max cross-clamping, legacy values outside the dropdown options, and that all controls remain disabled while a save is pending. These are the core behaviors introduced by this panel.
🤖 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 381 - 611, Add automated tests for the Memories controls in UserPreferencesCard, covering min/max cross-clamping in both update directions, rendering and preserving legacy values not present in the dropdown options, and disabling every Memories control while a save is pending. Reuse the existing component test patterns and exercise the relevant patchMemories interactions.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.
Outside diff comments:
In `@frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx`:
- Around line 73-78: Serialize all preference mutations in useUserPreferences,
including the patchMemories path and the YOLO, GPU, and video update paths, so
only one updatePreferencesMutation can run at a time. Ensure dependent controls
cannot trigger concurrent writes, preserving the existing snapshot/restore
behavior without allowing out-of-order updates to overwrite local preferences.
---
Nitpick comments:
In `@frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx`:
- Around line 381-611: Add automated tests for the Memories controls in
UserPreferencesCard, covering min/max cross-clamping in both update directions,
rendering and preserving legacy values not present in the dropdown options, and
disabling every Memories control while a save is pending. Reuse the existing
component test patterns and exercise the relevant patchMemories interactions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dc60ba56-192d-4fa1-9ccb-079370180dd4
📒 Files selected for processing (1)
frontend/src/pages/SettingsPage/components/UserPreferencesCard.tsx
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/src/pages/SettingsPage/components/__tests__/UserPreferencesCard.test.tsx (2)
85-87: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert one update call for each interaction.
toHaveBeenCalledWithaccepts any matching call. It does not fail when the handler emits an additional stale or duplicate patch. AddtoHaveBeenCalledTimes(1)before each payload assertion, or assert the completemock.callsarray.This protects the queued-save contract and verifies that each user action sends only the intended patch.
Suggested assertion
+ expect(mockUpdateMemoriesPreferences).toHaveBeenCalledTimes(1); expect(mockUpdateMemoriesPreferences).toHaveBeenCalledWith({As per path instructions, ensure test code is automated and comprehensive.
Also applies to: 112-115, 126-129, 139-142, 167-169
🤖 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 85 - 87, Strengthen the interaction assertions in UserPreferencesCard tests by verifying mockUpdateMemoriesPreferences is called exactly once before each existing payload assertion at all referenced cases. Preserve the current expected patch payloads while ensuring each user action produces only one update call.Source: Path instructions
90-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for enabling desktop notifications.
This test only checks the disabled state. It does not verify that the control is enabled when memory generation is active or that clicking it sends
{ notifications_enabled: true }. A permanently disabled or miswired switch could pass.Add a test for the default fixture that checks the enabled state, the default unchecked state, and the update payload.
Suggested test
+ it('enables desktop notifications', async () => { + const user = userEvent.setup(); + render(<UserPreferencesCard />); + await openPanel(user); + + const notifications = screen.getByRole('switch', { + name: /Desktop Notifications/i, + }); + expect(notifications).not.toBeDisabled(); + expect(notifications).not.toBeChecked(); + + await user.click(notifications); + + expect(mockUpdateMemoriesPreferences).toHaveBeenCalledWith({ + notifications_enabled: true, + }); + });As per path instructions, ensure test code is automated, comprehensive, and covers critical functionality.
🤖 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 90 - 100, Add coverage alongside the existing `locks notifications behind the generate toggle` test for the default memory fixture: render and open `UserPreferencesCard`, verify the Desktop Notifications switch is enabled and initially unchecked, click it, and assert the update handler receives `{ notifications_enabled: true }`. Ensure the test exercises the real user interaction and preserves the existing disabled-state coverage.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/hooks/__tests__/useUserPreferences.test.tsx`:
- Around line 75-92: Update updatePreferencesMutation’s onSuccess/refetch
coordination in useUserPreferences so a preferences fetch resolving while writes
remain queued cannot overwrite an optimistically applied or pending write. Add a
regression test in the useUserPreferences test suite that resolves
getUserPreferences mid-queue and verifies the concurrent write remains applied,
while preserving existing optimistic apply and rollback assertions.
In `@frontend/src/hooks/useUserPreferences.tsx`:
- Around line 63-65: The write-success refetch in updatePreferencesMutation can
overwrite newer queued optimistic updates through the preferencesQuery data
effect. Update updatePreferencesMutation and the surrounding writeQueue/refetch
flow so stale refetch responses cannot reach applyPreferences while newer writes
are pending, applying reconciliation only after the queue drains or using the
mutation response; preserve queued writes’ use of preferencesRef.current as
their base state.
In `@frontend/src/pages/Memories/Memories.tsx`:
- Around line 2-6: Restore the Memories page settings entry by retaining the
Settings import, navigation hook, and page-level settings button in the Memories
component. Configure the button to navigate to ROUTES.SETTINGS, or preserve an
equivalent always-visible control that provides the same access.
---
Nitpick comments:
In
`@frontend/src/pages/SettingsPage/components/__tests__/UserPreferencesCard.test.tsx`:
- Around line 85-87: Strengthen the interaction assertions in
UserPreferencesCard tests by verifying mockUpdateMemoriesPreferences is called
exactly once before each existing payload assertion at all referenced cases.
Preserve the current expected patch payloads while ensuring each user action
produces only one update call.
- Around line 90-100: Add coverage alongside the existing `locks notifications
behind the generate toggle` test for the default memory fixture: render and open
`UserPreferencesCard`, verify the Desktop Notifications switch is enabled and
initially unchecked, click it, and assert the update handler receives `{
notifications_enabled: true }`. Ensure the test exercises the real user
interaction and preserves the existing disabled-state coverage.
🪄 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: c8fcfcd1-c2c8-42de-8244-f46712949bf8
📒 Files selected for processing (4)
frontend/src/hooks/__tests__/useUserPreferences.test.tsxfrontend/src/hooks/useUserPreferences.tsxfrontend/src/pages/Memories/Memories.tsxfrontend/src/pages/SettingsPage/components/__tests__/UserPreferencesCard.test.tsx
| beforeEach(() => { | ||
| mockGetUserPreferences | ||
| .mockReset() | ||
| // The refetch after a successful write would otherwise overwrite local | ||
| // state from the server and mask what optimistic apply and rollback did. | ||
| .mockResolvedValueOnce({ | ||
| success: true, | ||
| message: 'ok', | ||
| user_preferences: baseline, | ||
| }) | ||
| .mockImplementation(() => new Promise(() => {})); | ||
|
|
||
| mockUpdateUserPreferences.mockReset().mockResolvedValue({ | ||
| success: true, | ||
| message: 'ok', | ||
| user_preferences: baseline, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Test setup avoids the refetch/queue race instead of covering it.
This beforeEach explains that mockGetUserPreferences must hang forever after the first call, or "the refetch after a successful write would otherwise overwrite local state from the server and mask what optimistic apply and rollback did." This confirms updatePreferencesMutation's unconditional onSuccess -> refetch() (in useUserPreferences.tsx) can race with the write queue in production. No test here exercises that interaction; once the hook coordinates refetch with the write queue, add a regression test where getUserPreferences resolves mid-queue to confirm a concurrently applied write is not reverted.
🤖 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/__tests__/useUserPreferences.test.tsx` around lines 75 -
92, Update updatePreferencesMutation’s onSuccess/refetch coordination in
useUserPreferences so a preferences fetch resolving while writes remain queued
cannot overwrite an optimistically applied or pending write. Add a regression
test in the useUserPreferences test suite that resolves getUserPreferences
mid-queue and verifies the concurrent write remains applied, while preserving
existing optimistic apply and rollback assertions.
| applyPreferences(preferencesQuery.data.user_preferences); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Refetch after a successful write can overwrite a concurrently queued optimistic update.
This effect applies preferencesQuery.data.user_preferences to local state whenever the query updates. preferencesQuery.refetch() is called unconditionally in updatePreferencesMutation's onSuccess (Lines 68-75), independent of writeQueue. If write A succeeds and its refetch is still in flight when write B is optimistically applied, the refetch response can arrive and call applyPreferences with data that predates write B. Because every queued write reads its base state from preferencesRef.current, this can revert B's optimistic value in the UI and also feed a stale current into whichever write runs next in the queue.
The test suite's beforeEach (in useUserPreferences.test.tsx, Lines 75-85) documents this exact hazard and works around it by making every getUserPreferences call after the first hang forever, rather than exercising the real interaction. In production, no such workaround exists.
Consider one of:
- Skip
refetch()whenwriteQueuehas additional writes pending (only refetch once the queue drains). - Use the mutation response as the reconciled state instead of triggering a full refetch.
- Gate the effect's
applyPreferencescall on some notion of "no newer local write in flight" (e.g., a monotonically increasing write counter compared before applying).
🤖 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 63 - 65, The
write-success refetch in updatePreferencesMutation can overwrite newer queued
optimistic updates through the preferencesQuery data effect. Update
updatePreferencesMutation and the surrounding writeQueue/refetch flow so stale
refetch responses cannot reach applyPreferences while newer writes are pending,
applying reconciliation only after the queue drains or using the mutation
response; preserve queued writes’ use of preferencesRef.current as their base
state.
| import { RefreshCw } from 'lucide-react'; | ||
|
|
||
| import { Button } from '@/components/ui/button'; | ||
| import { MemoryCard } from '@/components/Memories/MemoryCard'; | ||
| import { MemoryStoryViewer } from '@/components/Memories/MemoryStoryViewer'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the Memories settings entry.
This change removes the Settings import, navigation hook, and page-level settings button. The PR objective requires the former memory-settings entry to open /settings, not to disappear. Keep the button and navigate to ROUTES.SETTINGS, or verify that an always-visible replacement provides the same access before merging.
As per path instructions, verify that the code meets the project's requirements and objectives.
Also applies to: 33-34
🤖 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 2 - 6, Restore the
Memories page settings entry by retaining the Settings import, navigation hook,
and page-level settings button in the Memories component. Configure the button
to navigate to ROUTES.SETTINGS, or preserve an equivalent always-visible control
that provides the same access.
Source: Path instructions
1 participant
Follow-up to #1429.
Memory preferences had their own page at
memories/settings, reachable only from a gear icon on the Memories grid. Every other preference in the app lives in/settings, so this moves them there and deletes the separate page.What changed
A collapsible Memories group now sits in User Preferences, beside Video Tagging and behaving the same way:
The three sliders became dropdowns to match
Keyframe Intervalin the video group. Discrete options also keep the min and max pair valid by construction, though the cross-clamp is kept because a value saved by the old sliders can sit outside these sets.MemorySettings.tsx, thememories/settingsroute andROUTES.MEMORIES_SETTINGSare gone. The two gear buttons that pointed at it, on the grid page and in the story viewer, now open/settings.Notable decisions
Background music was dropped from the UI.
frontend/public/memory-theme.mp3is not bundled yet, so the toggle controlled nothing.story_music_enabledstays in the schema and the story viewer, it just has no control until the theme ships.Desktop notifications now default to off. Flipped in
MemoriesPreferences, in theMemoryStatusDataecho and in the frontend defaults. Alerts are opt-in; the memory is waiting on the page either way.openapi.jsonis regenerated to match, and the one assertion covering the default was updated.Verification
GITHUB_ACTIONS=true python -m pytest tests/ -q— 969 passednpx jest --maxWorkers=2— 287 passednpx tsc --noEmit— cleanSummary by CodeRabbit
New Features
Bug Fixes