feat(ui-prefs): persist Lume palette/appearance server-side per user …#392
Conversation
ConnysCode
left a comment
There was a problem hiding this comment.
Review: Request changes
One MAJOR before merge: a 401 from this endpoint bounces a validly-authenticated user to /login. The router design, the storage/escaping, the purge protection and the tests are otherwise sound (verified below).
Must change before merge
🟠 MAJOR — a 401 from ui-prefs redirects a logged-in user to /login. requireSessionUserId returns 401 whenever the session has no omadia_user_id (uiPrefs.ts:86). That claim is optional on a valid session — signSession only sets it when resolveChannelIdentity succeeds, and that resolver returns undefined in the documented eventual-consistency window of a brand-new user's first OIDC login (index.ts:2249-2254: "Returns undefined if the users-row was just created in the same request"), and undefined again on any KG hiccup (the catch at auth.ts:608 swallows it). The client treats this 401 as a dead session: the mount getUiPrefs() runs through getJson → maybeNavigateToLogin(401) and the palette-change putUiPrefs() does the same — so such a user is redirected to /login on page load (and again on every palette change), even though their session is live. Re-login doesn't help while the KG is unresolved → a loop. requireAuth already guarantees req.session.sub (a required claim). Please key the store on req.session.omadia_user_id ?? req.session.sub so a live session never 401s here; the router's 401 then only fires for a genuinely session-less request (its own unit-test path).
Should fix
🟡 MINOR — the prefs cookie is not cleared on logout. POST /logout clears omadia_session but not omadia-ui-prefs (auth.ts:296; the cookie has a 1-year max-age). On a shared browser the next user's first paint shows the previous user's palette until the mount getUiPrefs() corrects it. Non-secret, but a visible cross-user flash. Please clear omadia-ui-prefs in the logout handler too.
PR description — factcheck
| Claim | Reality |
|---|---|
| "uiPrefs router 9/9" | ✅ Confirmed — 9 it() blocks in uiPrefsRoute.test.ts, exercising round-trip, partial merge, injective-id isolation (a b vs a_b), .. containment, corrupt-file recovery, and the 401 guard. |
"Scratch purge protects ui-prefs; full reseed still clears it" |
✅ Confirmed — topLevelName('/memories/ui-prefs/…') → ui-prefs, which is in PROTECTED_SEED_ENTRIES; skipped under axis:'all' unless reseed. |
| "per-user server store … the choice follows the user across devices" (durable) | memoryStore is the RAM-backed InMemoryMemoryStore (harness-memory/plugin.ts:51 — "DB-less, RAM-backed … default provider when no Postgres-backed store is installed"); without @omadia/memory-postgres the prefs are lost on every middleware restart. Worth stating in the PR/issue so the durability expectation matches the deployment. |
| "No schema/migration … no new env var" | ✅ Accurate. |
Minimum to merge
- Fall back to
subfor the storage key (or otherwise stop a missing-claim 401 from hijacking navigation) so first-login / KG-degraded users aren't bounced to/login. - Clear the
omadia-ui-prefscookie on logout.
Nits (non-blocking)
- The palette/appearance enums are duplicated in
middleware/src/routes/uiPrefs.tsandweb-ui/app/_lib/uiPrefs.ts(unavoidable across the two packages). Consider a cross-reference comment in each so a spec addition (a 4th palette) isn't silently applied to only one side — the server would 400 the new value the client offers. readPrefsswallows a corrupt stored value as{}(good, and tested), but a genuine parse error is indistinguishable from "unset" in logs. Optional:log()the corrupt-discard so a recurring bad write is visible.
(Reviewed against head 3292d26.)
3292d26 to
20f207d
Compare
ConnysCode
left a comment
There was a problem hiding this comment.
Review: Approve
Both items from my previous review are addressed and verified against the code at head 20f207d. No blockers remain — the two NITs below are optional.
Already changed since the last review
- MAJOR (401 bounce) fixed.
requireSessionUserIdnow keys onreq.session?.omadia_user_id ?? req.session?.sub(uiPrefs.ts:93).subis a required claim (SessionClaims.subis non-optional andverifySessionthrows when it is empty —sessionJwt.ts:96), andrequireAuthruns before the router, so a live session can no longer 401 here. The router's 401 now only fires for a genuinely session-less request — exactly the unit-test path. - MINOR (prefs cookie on logout) fixed.
POST /logoutnowres.clearCookie('omadia-ui-prefs', { path: '/' })alongside the session cookie (auth.ts:298); name +path=/match the cookieThemeControlswrites, so the delete takes effect and the cross-user first-paint flash is gone. - Coverage grew with the fix. A dedicated
sub-fallback test was added and the session-less test now drives a null session (uiPrefsRoute.test.ts:225,:238) — 10it()blocks total.
PR description — factcheck
| Claim | Reality |
|---|---|
| "uiPrefs router 9/9" | Now 10 it() blocks — the fix added the sub-fallback case. Cosmetic; please bump the number if you touch the description. |
"Scratch purge protects ui-prefs; full reseed still clears it" |
✅ Still accurate — 'ui-prefs' is in PROTECTED_SEED_ENTRIES (memoryPurge.ts:43). |
| "No schema/migration … no new env var" | ✅ Accurate. |
Note (non-blocking, verification not executed)
I did not re-run the suite this pass — a full monorepo install is heavy and the delta is a self-contained new test plus a null-session tweak. My previous pass confirmed the suite green; the changed paths are verified by reading. If CI is green on this head, treat the test claim as confirmed.
Nits (non-blocking)
middleware/src/routes/uiPrefs.ts:93— thesubfallback introduces a one-time key change: prefs a user sets during the first-login / KG-unresolved window are stored undersub, then read underomadia_user_idonce the KG resolves and a new session is minted — so that early choice is silently orphaned and the user sees defaults again. Rare and self-healing (they just re-pick), and strictly better than the old 401 loop. Optional: a short comment noting the key can migrate, or readingsubas a fallback on GET too.- The palette/appearance enums are still duplicated across
middleware/src/routes/uiPrefs.ts:29-30andweb-ui/app/_lib/uiPrefs.ts:6-9(unavoidable across the two packages). Optional: a cross-reference comment so a 4th palette isn't added to only one side (the server would then 400 a value the client offers).
(Reviewed against head 20f207d.)
What
Move Lume palette/appearance from per-browser localStorage to a per-user server store (Closes #287). New
/api/v1/ui-prefsGET/PUT backed by MemoryStore; RSC layout reads a mirrored cookie for no-FOUC pre-paint.
Why
localStorage was per-device — no multi-device sync. §2.5.4 wants the choice per user.
Test plan
npm run testmiddleware — uiPrefs router 9/9npm run testweb-ui — ThemeControlsRisk / blast radius
ui-prefs; fullreseedstill clears it.