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.
|
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:
WalkthroughMemory notifications now default to opt-in. Memory controls moved into the main settings page. The dedicated memory settings route was removed. The preference hook now serializes writes with optimistic updates and rollback. ChangesMemory settings consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant UserPreferencesCard
participant useUserPreferences
participant PreferencesAPI
User->>UserPreferencesCard: Change a Memories setting
UserPreferencesCard->>useUserPreferences: Submit partial update
useUserPreferences->>useUserPreferences: Queue and apply optimistic update
useUserPreferences->>PreferencesAPI: Send changed fields
alt API succeeds
PreferencesAPI-->>useUserPreferences: Return updated preferences
useUserPreferences->>useUserPreferences: Process next queued write
else API fails
PreferencesAPI-->>useUserPreferences: Return error
useUserPreferences->>useUserPreferences: Roll back failed update
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
The mutation refetched on success, outside the queue, so a response that predated a newer write could land on top of it and hand the next queued write a stale base. The PUT already returns the merged result, so the write path adopts that instead, and the load effect stands down while writes are pending. Tests drop the never-resolving read they used to need.
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/hooks/useUserPreferences.tsx (1)
57-73: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard stale query data after a write settles.
pendingWrites.currentstops loads only while a write is queued. If an earlierGET/refetch()from mount resolves after thePUTcompletes,pendingWrites.currentis already0, so the effect can apply the stale preferences and revert the fast write. TracklastCommittedAton successful apply/rollback and reject loads received after that timestamp.usePictoQueryspreads the underlyingUseQueryResult, sodataUpdatedAtis available for this comparison.🤖 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 57 - 73, The preferences synchronization effect around pendingWrites and applyPreferences must reject query results older than the most recent successful write or rollback. Track a lastCommittedAt timestamp when a write is successfully applied or rolled back, then only apply preferencesQuery.data when its dataUpdatedAt is newer than that timestamp, while retaining the pendingWrites guard and existing valid-data checks.
🧹 Nitpick comments (1)
frontend/src/hooks/__tests__/useUserPreferences.test.tsx (1)
76-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
anywith explicit types indeepMerge.
base,updates, and the return type are typedany. SincedeepMergemirrors the server's merge overUserPreferencesDataandUpdateUserPreferencesRequest, a generic constrained to a plain-object shape gives type safety without hardcoding the domain type.♻️ Proposed typing for `deepMerge`
-const deepMerge = (base: any, updates: any): any => { +const deepMerge = <T extends Record<string, unknown>>( + base: T, + updates: Partial<T>, +): T => { const merged = { ...base }; for (const [key, value] of Object.entries(updates ?? {})) { const isPlainObject = (v: unknown) => typeof v === 'object' && v !== null && !Array.isArray(v); - merged[key] = + (merged as Record<string, unknown>)[key] = isPlainObject(value) && isPlainObject(merged[key]) - ? deepMerge(merged[key], value) + ? deepMerge(merged[key] as Record<string, unknown>, value as Record<string, unknown>) : value; } return merged; };As per path instructions, "Avoid 'any', use explicit types."
🤖 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 76 - 87, Update deepMerge to remove all any types and use explicit generic types constrained to a plain-object shape, preserving recursive merging for nested plain objects and allowing updates to override values. Type the return value consistently with the generic merge result without hardcoding UserPreferencesData or UpdateUserPreferencesRequest.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/hooks/useUserPreferences.tsx`:
- Around line 57-73: The preferences synchronization effect around pendingWrites
and applyPreferences must reject query results older than the most recent
successful write or rollback. Track a lastCommittedAt timestamp when a write is
successfully applied or rolled back, then only apply preferencesQuery.data when
its dataUpdatedAt is newer than that timestamp, while retaining the
pendingWrites guard and existing valid-data checks.
---
Nitpick comments:
In `@frontend/src/hooks/__tests__/useUserPreferences.test.tsx`:
- Around line 76-87: Update deepMerge to remove all any types and use explicit
generic types constrained to a plain-object shape, preserving recursive merging
for nested plain objects and allowing updates to override values. Type the
return value consistently with the generic merge result without hardcoding
UserPreferencesData or UpdateUserPreferencesRequest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ba14b9b5-0e0c-46ef-978d-4983661d22b9
📒 Files selected for processing (2)
frontend/src/hooks/__tests__/useUserPreferences.test.tsxfrontend/src/hooks/useUserPreferences.tsx
The pending-write guard only covered reads that arrived mid-write. A read issued earlier can resolve after the write settles, and it carries pre-write state, so it reverted the change. Reads now capture a write counter when they start and are dropped if a write has been queued since.
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 gear button on the Memories grid went with them, since the sidebar Settings button is on screen at the same time and does the same thing. The story viewer keeps its gear, now opening/settings, because the sidebar is covered while a memory is playing.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.Preference writes were not concurrency safe
Reviewing the new dropdowns turned up a problem that predates them and affects every control in the panel, so it is fixed here.
useUserPreferencesapplied each change optimistically, snapshotted the whole preferences object, and restored that snapshot on failure. Two overlapping saves meant the second snapshot was taken before the first had applied, so a failure rolled back over an unrelated successful change. The YOLO, GPU and video writes also sent the entire object, so the server merge could clobber a concurrent edit even when nothing failed.truethenfalserather thantruetwice.exclude_unset, so this is what it expects.Behind that sat a read/write race. The mutation refetched on success, outside the queue, and the load effect applied whatever came back, so a response describing the server from before a newer write could land on top of it and hand the next queued write a stale base. Three parts to the fix:
updatePreference, the unused whole-object entry point, is removed.Tests
16 new, 303 total.
Hook tests cover per-key request bodies, a queued write building on the previous one, a failed write rolling back only its own change, the queue surviving a rejection, the server response being adopted, and both stale-read cases. Every one was run against the code before its fix, and each fails there.
Component tests cover both directions of the min and max cross-clamp, a stored value outside the dropdown options, and every control disabled while a save runs.
Verification
GITHUB_ACTIONS=true python -m pytest tests/ -q— 969 passednpx jest --maxWorkers=2— 303 passednpx tsc --noEmit— cleanSummary by CodeRabbit
New Features
Bug Fixes