Persist user preferences (theme and locale) in database - #246
Open
slax57 wants to merge 14 commits into
Open
Conversation
ThieryMichel
requested changes
Apr 2, 2026
ThieryMichel
requested changes
Apr 3, 2026
# Conflicts: # src/components/admin/theme-mode-toggle.tsx
…ate grant Reworks the review feedback on #246. Centralise the write mechanism, not the trigger. usePersistPreference holds the single copy of the request, the query cache reconciliation and the error notification; each control still calls it explicitly right after setTheme or setLocale. Observing the store instead would have to infer intent from a derived value, which is wrong whenever the value moves for another reason: the i18n remount, the cross-tab mirror, or the load itself. Fixes the language round trip. ra-core's I18nContextProvider remounts its whole subtree on every locale change, so anything holding per-mount state loses it while the query cache survives. The cache is now written optimistically at call time and rolled back on failure, so a remount re-applies the fresh value instead of an unreconciled one. Covered by a regression test. Security: the self-update policy on `sales` allowed updating every column of one's own row, and `06_grants.sql` grants all to `authenticated`, so a user could set `administrator = true` on themselves. Restrict the write to the `preferences` column. Admin edits go through the `users` edge function with the service_role and are unaffected. Also: - re-wire theme-mode-toggle, which the merge with main silently dropped - namespace the query key per user, so a shared device cannot leak preferences through the mobile persisted cache - keep the server-side merge and send only the changed key, so a stale snapshot cannot wipe another device's value - detect zero-row updates, which PostgREST reports as success - tolerate a missing sales row on read - add `preferences` to the `Sale` type - fix the missing `useMemo` dependency in CRM.tsx - regenerate the migration, its timestamp predated already applied ones
…trigger Second pass on the review feedback for #246. Validate the jsonb on read. The column is free-form jsonb an authenticated user can PATCH directly through PostgREST, and the theme reaches classList.add, which throws InvalidCharacterError on a value containing whitespace. Since ThemeProvider sits above the layout error boundary, that blanked the app on every load with no in-app recovery. Values are now parsed field by field, so one bad key cannot discard a valid sibling. Guard the column by trigger, not only by grant ordering. The revoke sat three lines under the `grant all` it undoes, so re-adding or regenerating that line would silently restore table-wide UPDATE and with it self-promotion to administrator. enforce_sales_self_update_scope compares every column except preferences and rejects the write, which holds regardless of grant order and protects columns added later by construction. auth.uid() is null for the service_role, so the users edge function is unaffected. Verified against a simulated grant regression. Also: - wrap Postgrest failures in HttpError with the response status, so ra-core's checkError sees a 401 and logs the user out instead of showing raw backend text - route the fakerest twin through the wrapped provider so the sales lifecycle callback fires, and fix its identity guard, which could never trigger because fakerest getIdentity falls back to id 0, the administrator row - run the two login prefetches concurrently instead of serially
Third pass on the review feedback for #246. The previous tests called the hooks from a bespoke probe inside an overridden layout, so deleting the call in Layout.tsx or the persist call in a control left them green: they covered the hook, not the feature. They now render the real Layout and drive the real header toggle. Verified by mutation: removing usePreferencesLoader() from Layout fails 3 of 4, removing persist() from theme-mode-toggle fails 2 of 4. Assertions are on user-visible state and on a fake server's contents rather than on mock call ledgers, which .claude/rules/testing.md lists as blocking. Add e2e/preferences.spec.ts: set the theme and the language, reload, check both held. That round trip is the only layer that catches a control wired to setTheme or setLocale without its persist call, which is the failure mode of the explicit approach, and the only one that exercises the real RLS policy and column grant. Per the project e2e conventions it is left for CI to run. StoryWrapper takes optional i18nProvider and layout props so a test can use two locales and the real layout. Both default to the previous behaviour.
Three of the four points from the latest review round. The fourth, replacing the read-modify-write with an atomic jsonb RPC, is left out pending a call on the project convention against stored procedures. Demo mode was broken by the previous commit. The dead `if (!identity)` guard was replaced by `if (!identity?.id)`, but the demo administrator sale has id 0, so the falsy check bailed out and preferences were never read or written under fakerest. The guard now keys off whether a user is actually stored, which distinguishes "logged out" from "sale id 0". A test drives the real fakerest provider instead of substituting both methods, and fails if the id-0 guard comes back. The trigger guard was auth.uid() IS NOT NULL, which exempts anon since anon has no JWT subject while keeping `grant all` on the table. It now keys off current_user, which does not depend on claims and cannot be spoofed by a forwarded token. Verified: with a synthetic anon select and update policy, the trigger rejects a write to administrator and lets a preferences-only write through. Note the exemption was not reachable today, anon has no select policy on sales so it cannot see a row to update. The e2e assertion could not fail. theme and locale live in localStorage and page.reload() keeps the browsing context, so both assertions after the reload passed even with the server write entirely broken. The spec now drops the CRM store keys, keeping the auth session, so the values can only come back from the server. That is the only coverage of the RLS policy, the column grant and the trigger.
Keeps the merge in the data provider, per the project convention against stored procedures, and closes the race it reintroduces without any SQL. updatePreferences reads then writes over two requests, so two changes fired back to back both read the same snapshot and the second write dropped the first key. Because every control now goes through the one helper, chaining the writes there serialises all of them for the tab: the second read happens after the first write has landed. The queue lives at module scope on purpose, a per-hook ref would give each control its own chain and serialise nothing. Covered by a test with a fake server that reads, waits, then writes, like the real provider. Without the chain it fails with theme still 'dark', the exact lost update. Remaining and accepted: two devices writing within the same round trip can still lose a key. Only an atomic jsonb write server-side would close that, which is the rejected RPC. It is strictly narrower than the whole-column write it replaced, where a snapshot minutes old clobbered another device every time. The fakerest test now reuses createCrmDb instead of a duplicated fixture.
…pruning
The three points flagged as blocking before merge, plus a fresh migration
timestamp. No RPC: the cross-context race stays, see below.
The e2e cleanup matched nothing. ra-core builds its localStorage prefix as
`${RA_STORE}${appKey}` with no separator, so the real keys are RaStoreCRM.theme,
not CRM.theme. The filter returned an empty array, the reload rehydrated from
localStorage, and the spec passed even with both persist calls removed. It now
matches RaStore and throws when nothing matches, so a future prefix change fails
loudly instead of silently making the spec vacuous.
The rollback did nothing on a cold cache. setQueryData returns early when the
value is undefined, so a failed write before the query had populated the cache
left the optimistic entry in place. It now removes the query instead.
The write merged onto the validated value, so any key this version does not model
was pruned on the first change. The merge now uses the raw stored object and
validation stays on the read path, where it protects setTheme from a jsonb value
that would throw in classList.add.
Fresh migration timestamp: the file changed content three times under the same
name, and the last change altered security behaviour, so a database that applied
an earlier copy would silently keep the trigger that exempted anon. Statements are
idempotent, verified by replaying the migration over an already-migrated database.
Accepted and not fixed: two tabs or two devices writing within the same round trip
can still lose a key, since PostgREST cannot express a jsonb merge and the
alternative was ruled out. pendingWrites covers the common single-tab case. Also
unfixed, and narrow: a write queued at module scope crosses an identity change,
and the locale is validated on shape rather than against the configured locales.
…logout The e2e cleanup still bypassed the database on mobile. MobileAdmin wraps the tree in PersistQueryClientProvider, whose persister stores the react-query snapshot under REACT_QUERY_OFFLINE_CACHE, a key that does not start with RaStore. The spec runs on both the chromium and Mobile Chrome projects, so the mobile run was decided by that restored snapshot rather than by a request. The filter is now inverted: everything goes except the Supabase session, which supabase-js stores under sb-<ref>-auth-token. The guard on a missing RaStore key stays, so the cleanup still fails loudly if it ever stops matching. Reset pendingWrites on logout, so a write queued by one user cannot land in the next user's row when a login follows within the same round trip.
resetPendingPreferenceWrites only reassigned the module variable, but a continuation already chained holds its own reference to the previous promise, so a write queued before the logout still ran once the in-flight one settled and resolved the session at that moment. Reproduced with a standalone script: the queued write executed after the reset. A generation counter now guards the callback, so anything enqueued before the reset resolves to a no-op while later writes still go through. Both verified with the same script. A stored locale is applied only when the i18nProvider offers it, so a value like "de" no longer reaches changeLocale and raises ra.notification.i18n_error. Falls back to applying it when the provider declares no locales, since useLocales returns an empty array in that case and rejecting would break persistence for a bare provider. The two-locale test provider is now built per test. changeLocale mutates the provider's current locale, so the shared instance was leaking the previous test's language into the next one, which is what surfaced while adding the locale test.
Both projects failed on the step right after a direct page.goto to a sub route,
desktop on /profile and mobile on /settings. Every spec that passes reaches its
page by clicking from the root, so the spec now does the same and never loads a sub
route directly, including for the final check, which navigates to / and clicks
again rather than reloading in place.
Reaching /profile on desktop needs the header user menu, whose trigger is an
icon-only button with no accessible name: the only name it exposed was the avatar
fallback initial. It now carries aria-label={translate("ra.auth.user_menu")},
which is an accessibility fix in its own right and gives the spec a stable locator.
A unit test renders the real Layout and asserts the menu opens through that exact
locator, so the e2e no longer relies on an unverified guess.
Menu labels are matched with language-tolerant patterns, since the second half of
the test runs after switching to French.
Contributor
|
Rebased on main. |
…session Seven findings from a fresh review, all confirmed. An optimistic write did not cancel the read already in flight, so a GET issued before the toggle but resolving after it put the stale value back in the cache and the loader re-applied the old theme. cancelQueries now runs before the optimistic write, which is the documented react-query recipe. Covered by a test whose fake read snapshots the server before its latency, like a real request: without the cancel it fails with the UI back on the previous theme. The login prefetch wrote the locale straight to the store without the check the loader applies, so a locale the app does not offer was applied at login and the loader could not repair it. Both paths now share isOfferedLocale. The cache key embeds identity.id, which is undefined until useGetIdentity resolves, so a toggle in that window wrote to an orphan key. The helper now skips the cache when the identity is unknown and invalidates the preferences queries once the write lands, so the loader refetches the truth instead of serving a stale entry. The e2e cleared caches right after asserting the optimistic UI, without waiting for the fire-and-forget PATCH, which page.goto then aborted. It was sound but flaky. It now waits for each successful PATCH before moving on. Also: - the fakerest twin returned the raw merged object while the read path and the Supabase provider return a validated one, so a corrupt stored theme could reach classList.add and throw above every error boundary - both providers threw nothing when there was no session or logged in user, so the helper marked the cache as persisted while nothing was written - the locale pattern rejected valid BCP-47 tags such as zh-Hans or three-letter codes, silently discarding the choice for such a deployment
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.
Problem
User preferences (theme and locale) are stored locally and lost when switching devices or browsers, or after logging out.
Solution
Persist user preferences in a
preferencesJSONB column on thesalestable, synced on change and pre-fetched at login.How To Test
Additional Checks
Also, please make sure to read the contributing guidelines.