refactor(web): standardize localStorage key naming, simplify session cleanup (LUM-2045)#32621
Conversation
…cleanup (LUM-2045) Rename ~30 localStorage keys to use consistent vellum: prefix for user-scoped data and device: prefix for device-scoped data. Add idempotent one-time migrations that run at boot (after device-settings migration). Simplify session-cleanup.ts from 81 lines with a fragile APP_KEY_PREFIXES list + DEVICE_LEVEL_KEYS set to 41 lines with a single vellum: prefix check. Any new vellum: key is automatically cleaned up on logout without needing manual registration. Key naming convention: - vellum: — user-scoped, cleared on logout - device: — device-scoped, preserved across sessions Closes LUM-2045 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ization Move migrateDeviceSettings() and runStorageMigrations() from boot() to a side-effect import that executes before routes.tsx and its transitive store imports. This fixes a timing bug where onboarding-store and client-feature-flag-store read the new key names at module level before migrations copied values from the old keys. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…rations Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…move internal refs - Fix stale 'Called from boot()' docstring in storage-migration.ts (actually called from run-storage-migrations.ts side-effect import) - Fix stale 'vellum_current_assistant_id__' comment in current-platform-assistant-store.ts (key is now vellum:currentAssistantId:) - DRY: prefs.ts now imports KEY_TOS_ACCEPTED, KEY_AI_DATA_CONSENT, KEY_COMPLETED from onboarding-cleanup.ts instead of duplicating them - Remove Linear ticket references (LUM-2045, LUM-2042) from code comments — this is a public repo; traceability belongs in PR metadata Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
Test Results — localStorage Key Migration & Session CleanupTested end-to-end via Playwright CDP against Vite dev server (localhost:3001) with mocked API endpoints. Test 1: Key Migration — 25/25 passedSet 11 OLD keys with known values, reloaded app, verified all migrated to NEW keys.
Test 2: Migration Timing — 4/4 passed (critical test)Verifies the side-effect import fix for the bug both review bots flagged. Setup: Cleared localStorage, set only OLD onboarding keys (
Why this proves timing is correct: If migrations ran AFTER Test 3: Idempotency — 22/23 passedReloaded page again without clearing localStorage.
One expected deviation: Environment: Vite dev server (port 3001), Chrome CDP (port 29229), Playwright route interception for all API endpoints, no daemon. Devin session. |
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE — reviewed at 5609aef1ba
Value: Replaces a brittle logout allowlist (every new key needed manual registration to be cleaned up) with a two-namespace contract — vellum: user-scoped, device: device-scoped — so any future key auto-clears on logout without touching session-cleanup.ts. Eliminates a whole class of "I added a key and forgot to register it" regressions, including the auth-token survival risk that motivated this work.
What this does. ~30 static keys + 5 dynamic prefix groups renamed to vellum:* / device:* (device migration already in place). session-cleanup.ts rewritten from 81-line allowlist (APP_KEY_PREFIXES + DEVICE_LEVEL_KEYS set) to 41-line prefix sweep with a transitional legacy-prefix safety net. Migrations are idempotent and run synchronously at import time via the new run-storage-migrations.ts side-effect module. 38 new unit tests + Devin's 25/25 Playwright end-to-end run.
Full analysis
The architectural shift is the right one
session-cleanup.ts went from "explicit allowlist of prefixes the cleanup is allowed to touch, plus an exception set for device-scoped keys that share the vellum_ prefix" to "everything starting with vellum: is user-scoped, everything starting with device: is preserved, third-party keys are ignored." That's the same future-proofing move web.dev recommends and it dissolves the registration-debt problem that the old APP_KEY_PREFIXES list created. New keys auto-clear without ceremony. The two-prefix contract is also now self-documenting in every store and helper that touches localStorage.
Two real bugs caught and substantively fixed in the bot review cycle
Codex P1 + Devin BUG_0001 at edd1680ed9 — migration timing race. The first commit ran runStorageMigrations() inside boot() (async), but ES module evaluation order had already pulled routes → onboarding-middleware → prefs → onboarding-store, whose module-level computeInitialFromLS() reads from the new key names (which don't exist yet for existing users). Result: tosAccepted/aiDataConsent/completed would all read false for the entire first page load after upgrade, and resolveOnboardingRedirect would bounce platform users back to onboarding even though they'd completed it. Fixed in 2de4bb3 by hoisting both migrateDeviceSettings() and runStorageMigrations() into a side-effect import (import "@/utils/run-storage-migrations") at the very top of main.tsx, before import { router } from "./routes". The header comment in main.tsx and the docstring on run-storage-migrations.ts both name the constraint explicitly, which is exactly the right thing to do for a load-bearing import order.
Codex P2 at 2de4bb3cea — legacy-prefix sweep gap on logout. With migrations potentially failing on QuotaExceededError, the vellum:-only logout sweep would leave legacy keys (gw:token, voice:ttsApiKey:*, vellum_gemini_key, etc.) behind — gw:token is the auth token, so this would have been a real session-leak. Fixed in 5498f72 with LEGACY_USER_PREFIXES covering all eight legacy prefix families plus a LEGACY_DEVICE_KEYS set that excludes the legacy vellum_theme/vellum_share_*/onboarding.lastUserId keys from the sweep. The precedence in isUserScopedKey() is correct: vellum: first, then the device-key exclusion, then the legacy-prefix match — so vellum_theme (which matches the vellum_ legacy prefix) is correctly preserved by the exclusion check before it ever hits the prefix scan.
Verifications at HEAD
migrateKey()idempotency holds for QuotaExceededError. Read old → write new only if new is absent → re-read new → remove old only if new persisted. So ifsetItemthrows butremoveItemwould have succeeded, the legacy key survives for the next-load retry instead of being silently lost. This is the right shape for partial-failure recovery and Devin's response to the P2 finding names this exact failure mode.migratePrefix()snapshot pattern is correct. Collects[oldKey, newKey]pairs in a first pass before any writes, avoiding the live-iteration footgun whereremoveItemshifts the indiceslocalStorage.key(i)returns. The dynamic prefix renames (voice:ttsApiKey:*,ff:client:*,disk-pressure-warning-dismissed-*,vellum_current_assistant_id__*) all use this path.onboarding.lastUserIddevice migration is intact. Despite being listed in bothLEGACY_DEVICE_KEYS(logout safety net) and thedevice-settings.tsDEVICE_SETTINGSmap (withlegacy: "onboarding.lastUserId"→key: "device:last_user_id"), the precedence is correct:migrateDeviceSettings()runs first (inrun-storage-migrations.ts, line 17 before line 18), moves it todevice:last_user_id, and the legacy entry inLEGACY_DEVICE_KEYSonly fires as a safety net if both that migration and the user-key migration interleave catastrophically.prefs.tsDRY pickup is clean.KEY_TOS_ACCEPTED/KEY_AI_DATA_CONSENT/KEY_COMPLETEDare now imported fromonboarding-cleanup.ts(single source of truth, also referenced by the cleanup function itself). No drift surface.- All 24 caller updates checked.
chat-layout.tsx,chat-route-content.tsx,mic-permission-primer.tsx,skills-tab.tsx,onboarding-store.ts,prefs.ts,ai-page.tsx,integrations-page.tsx,voice-page.tsx,gateway-session.ts,impersonate-version-flag.ts,local-mode.ts,client-feature-flag-store.ts(LS_PREFIX ="vellum:ff:"),current-platform-assistant-store.ts,onboarding-cleanup.ts,ptt-activator.ts,web-search-provider-catalog.gen.ts— every static reference is updated to the canonicalvellum:*name. No stragglers from grep sweep. - Devin's Playwright end-to-end run (21:34 comment). 25/25 migration tests passed against the Vite dev server with real
localStoragereads/writes. This is the kind of integration coverage that the unit tests can't reach (module evaluation order can't be faked in JSDOM).
Small non-blocking observations
onboarding-cleanup.tsis now the de-facto source of truth for the three onboarding key constants. Worth a one-line docstring noting that — bothprefs.tsandstorage-migration.tsreference these keys, and the next reader who edits one place should know to check the others. Not a regression, just a discoverability nudge.- The legacy sweep is correctly framed as transitional. Docstring says "can be removed once we're confident all users have been migrated." Worth filing a follow-up Linear ticket (LUM-2046-ish) to drop
LEGACY_USER_PREFIXES+LEGACY_DEVICE_KEYSafter ~3 release cycles, with the migration telemetry to back the call. Otherwise this debt quietly accretes. onboarding.lastUserIdinLEGACY_DEVICE_KEYSis the only non-vellum_entry. Comment block names the reason (it matchesonboarding.legacy prefix but is device-scoped), but worth a half-line inline// matches "onboarding." prefix, must not be swept on logoutnext to the entry itself. The set is the kind of thing someone will trim 6 months from now without remembering why one entry doesn't fit the pattern.- Allowlist comment.
apps/web/.cross-domain-allowlist.jsonnot in the diff — confirmed there's no cross-domain edge growth from these renames, the dependency direction in the relevant files is unchanged.
Merge gate
- Vex: ✅ this approval at
5609aef1ba. - Devin: "Found 2 potential issues" comment at
edd1680ed9flagged the migration timing race; both issues then explicitly closed ("✅ Resolved" + "Fixed in5498f72") at the subsequent commits. End-to-end Playwright run at 21:34 reports 25/25 migration tests + 9/9 cleanup tests passing. - Codex: P1 (
edd1680) addressed in2de4bb3; P2 (2de4bb3) addressed in5498f72. Boss's@codex reviewping at 21:03 pre-dates the last two fix commits and the audit-finding cleanup — a fresh@codex reviewat5609aef1bawould close the gate cleanly, but the substantive findings are both resolved with tests proving the fix. - CI: 7/7 green at HEAD.
Vellum Constitution — Trust-seeking: replaces an allowlist that silently failed to clean up new keys with a two-namespace contract whose failure modes are visible at the type-system level — and the legacy sweep + idempotent migrations make the upgrade safe to retry rather than catastrophically irreversible.
There was a problem hiding this comment.
✦ APPROVE — reviewed at 5609aef1ba
Value: Replaces a brittle logout allowlist (every new key needed manual registration to be cleaned up) with a two-namespace contract — vellum: user-scoped, device: device-scoped — so any future key auto-clears on logout without touching session-cleanup.ts. Eliminates a whole class of "I added a key and forgot to register it" regressions, including the auth-token survival risk that motivated this work.
What this does. ~30 static keys + 5 dynamic prefix groups renamed to vellum:* / device:* (device migration already in place). session-cleanup.ts rewritten from 81-line allowlist (APP_KEY_PREFIXES + DEVICE_LEVEL_KEYS set) to 41-line prefix sweep with a transitional legacy-prefix safety net. Migrations are idempotent and run synchronously at import time via the new run-storage-migrations.ts side-effect module. 38 new unit tests + Devin's 25/25 Playwright end-to-end run.
Full analysis
The architectural shift is the right one
session-cleanup.ts went from "explicit allowlist of prefixes the cleanup is allowed to touch, plus an exception set for device-scoped keys that share the vellum_ prefix" to "everything starting with vellum: is user-scoped, everything starting with device: is preserved, third-party keys are ignored." That's the same future-proofing move web.dev recommends and it dissolves the registration-debt problem that the old APP_KEY_PREFIXES list created. New keys auto-clear without ceremony. The two-prefix contract is also now self-documenting in every store and helper that touches localStorage.
Two real bugs caught and substantively fixed in the bot review cycle
Codex P1 + Devin BUG_0001 at edd1680ed9 — migration timing race. The first commit ran runStorageMigrations() inside boot() (async), but ES module evaluation order had already pulled routes → onboarding-middleware → prefs → onboarding-store, whose module-level computeInitialFromLS() reads from the new key names (which don't exist yet for existing users). Result: tosAccepted/aiDataConsent/completed would all read false for the entire first page load after upgrade, and resolveOnboardingRedirect would bounce platform users back to onboarding even though they'd completed it. Fixed in 2de4bb3 by hoisting both migrateDeviceSettings() and runStorageMigrations() into a side-effect import (import "@/utils/run-storage-migrations") at the very top of main.tsx, before import { router } from "./routes". The header comment in main.tsx and the docstring on run-storage-migrations.ts both name the constraint explicitly, which is exactly the right thing to do for a load-bearing import order.
Codex P2 at 2de4bb3cea — legacy-prefix sweep gap on logout. With migrations potentially failing on QuotaExceededError, the vellum:-only logout sweep would leave legacy keys (gw:token, voice:ttsApiKey:*, vellum_gemini_key, etc.) behind — gw:token is the auth token, so this would have been a real session-leak. Fixed in 5498f72 with LEGACY_USER_PREFIXES covering all eight legacy prefix families plus a LEGACY_DEVICE_KEYS set that excludes the legacy vellum_theme/vellum_share_*/onboarding.lastUserId keys from the sweep. The precedence in isUserScopedKey() is correct: vellum: first, then the device-key exclusion, then the legacy-prefix match — so vellum_theme (which matches the vellum_ legacy prefix) is correctly preserved by the exclusion check before it ever hits the prefix scan.
Verifications at HEAD
migrateKey()idempotency holds for QuotaExceededError. Read old → write new only if new is absent → re-read new → remove old only if new persisted. So ifsetItemthrows butremoveItemwould have succeeded, the legacy key survives for the next-load retry instead of being silently lost. This is the right shape for partial-failure recovery and Devin's response to the P2 finding names this exact failure mode.migratePrefix()snapshot pattern is correct. Collects[oldKey, newKey]pairs in a first pass before any writes, avoiding the live-iteration footgun whereremoveItemshifts the indiceslocalStorage.key(i)returns. The dynamic prefix renames (voice:ttsApiKey:*,ff:client:*,disk-pressure-warning-dismissed-*,vellum_current_assistant_id__*) all use this path.onboarding.lastUserIddevice migration is intact. Despite being listed in bothLEGACY_DEVICE_KEYS(logout safety net) and thedevice-settings.tsDEVICE_SETTINGSmap (withlegacy: "onboarding.lastUserId"→key: "device:last_user_id"), the precedence is correct:migrateDeviceSettings()runs first (inrun-storage-migrations.ts, line 17 before line 18), moves it todevice:last_user_id, and the legacy entry inLEGACY_DEVICE_KEYSonly fires as a safety net if both that migration and the user-key migration interleave catastrophically.prefs.tsDRY pickup is clean.KEY_TOS_ACCEPTED/KEY_AI_DATA_CONSENT/KEY_COMPLETEDare now imported fromonboarding-cleanup.ts(single source of truth, also referenced by the cleanup function itself). No drift surface.- All 24 caller updates checked.
chat-layout.tsx,chat-route-content.tsx,mic-permission-primer.tsx,skills-tab.tsx,onboarding-store.ts,prefs.ts,ai-page.tsx,integrations-page.tsx,voice-page.tsx,gateway-session.ts,impersonate-version-flag.ts,local-mode.ts,client-feature-flag-store.ts(LS_PREFIX ="vellum:ff:"),current-platform-assistant-store.ts,onboarding-cleanup.ts,ptt-activator.ts,web-search-provider-catalog.gen.ts— every static reference is updated to the canonicalvellum:*name. No stragglers from grep sweep. - Devin's Playwright end-to-end run (21:34 comment). 25/25 migration tests passed against the Vite dev server with real
localStoragereads/writes. This is the kind of integration coverage that the unit tests can't reach (module evaluation order can't be faked in JSDOM).
Small non-blocking observations
onboarding-cleanup.tsis now the de-facto source of truth for the three onboarding key constants. Worth a one-line docstring noting that — bothprefs.tsandstorage-migration.tsreference these keys, and the next reader who edits one place should know to check the others. Not a regression, just a discoverability nudge.- The legacy sweep is correctly framed as transitional. Docstring says "can be removed once we're confident all users have been migrated." Worth filing a follow-up Linear ticket (LUM-2046-ish) to drop
LEGACY_USER_PREFIXES+LEGACY_DEVICE_KEYSafter ~3 release cycles, with the migration telemetry to back the call. Otherwise this debt quietly accretes. onboarding.lastUserIdinLEGACY_DEVICE_KEYSis the only non-vellum_entry. Comment block names the reason (it matchesonboarding.legacy prefix but is device-scoped), but worth a half-line inline// matches "onboarding." prefix, must not be swept on logoutnext to the entry itself. The set is the kind of thing someone will trim 6 months from now without remembering why one entry doesn't fit the pattern.- Allowlist comment.
apps/web/.cross-domain-allowlist.jsonnot in the diff — confirmed there's no cross-domain edge growth from these renames, the dependency direction in the relevant files is unchanged.
Merge gate
- Vex: ✅ this approval at
5609aef1ba. - Devin: "Found 2 potential issues" comment at
edd1680ed9flagged the migration timing race; both issues then explicitly closed ("✅ Resolved" + "Fixed in5498f72") at the subsequent commits. End-to-end Playwright run at 21:34 reports 25/25 migration tests + 9/9 cleanup tests passing. - Codex: P1 (
edd1680) addressed in2de4bb3; P2 (2de4bb3) addressed in5498f72. Boss's@codex reviewping at 21:03 pre-dates the last two fix commits and the audit-finding cleanup — a fresh@codex reviewat5609aef1bawould close the gate cleanly, but the substantive findings are both resolved with tests proving the fix. - CI: 7/7 green at HEAD.
Vellum Constitution — Trust-seeking: replaces an allowlist that silently failed to clean up new keys with a two-namespace contract whose failure modes are visible at the type-system level — and the legacy sweep + idempotent migrations make the upgrade safe to retry rather than catastrophically irreversible.
Add inline comment on onboarding.lastUserId in LEGACY_DEVICE_KEYS explaining why it's in the set (matches onboarding. prefix but is device-scoped). Add docstring on onboarding-cleanup.ts noting it's the source of truth for the three onboarding key constants. Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
devin-ai-integration
Bot
dismissed
vex-assistant-bot[bot]’s stale review
via
6329dd2
|
All three non-blocking observations addressed in
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6329dd2d1e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
If the startup migration fails (e.g. QuotaExceededError on setItem), the old gw:token/gw:expiresAt/gw:tokenSource keys survive but the new vellum:gw:* keys don't exist. Without a fallback read, getGatewayToken() returns null and local-mode users lose their gateway session. Add ?? fallback to legacy key names in getGatewayToken(), ensureGatewayToken(), and clearGatewayToken(). Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
Prompt / plan
Standardize all ~30 app-owned localStorage keys to use consistent prefixes:
vellum:— user-scoped, cleared on logoutdevice:— device-scoped, preserved across sessionsThis replaces the fragmented prefix landscape (
onboarding.,voice:,ff:client:,gw:,local:,integrations.,vellumDebug.,vellum_, and unprefixed keys) with two canonical prefixes, enabling prefix-based session cleanup instead of a brittle allowlist.What changed
vellum:ordevice:namespacestorage-migration.ts) — Read old key → write new key (only if absent) → remove old key. Handles QuotaExceededError gracefully. Runs at import time before any Zustand store reads localStorage.run-storage-migrations.ts) — Side-effect import module at the top ofmain.tsx, before the routes import. ES module evaluation order guarantees migrations complete beforeonboarding-storeandclient-feature-flag-storeread localStorage at module level.session-cleanup.ts) — Rewritten from 81-line allowlist approach to 41-line prefix-based approach. Removes everyvellum:key while preservingdevice:and third-party keys. Legacy prefix sweep as safety net if migration failed.prefs.tsimportsKEY_TOS_ACCEPTED,KEY_AI_DATA_CONSENT,KEY_COMPLETEDfromonboarding-cleanup.tsinstead of duplicating them.Why prefix-based cleanup is better than an allowlist
The old approach maintained an
APP_KEY_PREFIXESlist and aDEVICE_LEVEL_KEYSset insession-cleanup.ts. Any new key with a novel prefix (or no prefix) was silently skipped on logout. The new approach is future-proof: any key starting withvellum:is automatically cleaned up without needing explicit registration.Safety
vellum_theme, etc.) are explicitly excluded from the legacy sweep to prevent accidental deletion ifmigrateDeviceSettings()also failedReferences
Test plan
vellum:removal,device:preservation, third-party preservation, legacy prefix sweep, legacy device key exclusionbunx tsc --noEmit)bun run lint)Link to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka