refactor(web): route inline localStorage calls through shared utilities (LUM-2047)#32652
Conversation
…es (LUM-2047) Replace raw localStorage.getItem/setItem/removeItem with shared local-settings.ts helpers (getLocalBool, setLocalBool, getLocalNumber, setLocalNumber, getLocalSetting, setLocalSetting, removeLocalSetting) in 7 production files. Eliminates duplicated SSR guards, try/catch boilerplate, and typeof-window checks. Additional fixes: - Deduplicate OAuth popup utilities in integration-detail-modal.tsx (was copy-pasted from lib/auth/oauth-popup.ts) - Move nudge legacy key cleanup from nudge-store.ts to storage-migration.ts (correct infrastructure location) - Add app. prefix to session-cleanup.ts legacy user prefixes - Add removeKey() helper to storage-migration.ts 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:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb40a8e8e
ℹ️ 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".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…ailure
- Add migrateValue() helper for same-key value format conversions
- Migrate skills tip from '1' to 'true' so getLocalBool recognizes it
- Fix PTT readActivator: return { kind: 'off' } when storage is
unavailable (was defaulting to Ctrl via parseActivator(null))
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6086c396dc
ℹ️ 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".
Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe5c5d8e81
ℹ️ 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".
| "integrations.", | ||
| "vellumDebug.", | ||
| "vellum_", | ||
| "app.", |
There was a problem hiding this comment.
When a user logs out after dismissing or clicking either app-download nudge, this broad app. fallback now removes still-active localStorage keys such as app.iosNudge.downloaded, app.iosNudge.bannerDismissed, app.macOsNudge.downloaded, and app.macOsNudge.bannerDismissed (verified with rg 'app\\.(iosNudge|macOsNudge)' apps/web/src). Those hooks still read/write these keys, so the next login on the same browser forgets prior dismissals/downloads and can resurface the banners or reset turn counts. Please limit cleanup to the actual legacy GitHub/Discord keys (or migrate the active nudge keys first) rather than sweeping all app. keys.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real bug — good catch. use-ios-app-nudge.ts and use-macos-app-nudge.ts actively read/write app.iosNudge.* and app.macOsNudge.* keys. Sweeping all "app." on logout clears those dismissals, resurfacing banners on next login.
Fixed in a00dc62: removed "app." from LEGACY_USER_PREFIXES. The dead github/discord nudge keys (app.githubNudge.*, app.discordNudge.*, app.nudgeLegacy.cleaned) are already explicitly removed at startup by removeKey() calls in storage-migration.ts, so the broad prefix sweep was unnecessary.
Updated the test from "clears legacy app. prefixed nudge keys" → "preserves active app. nudge keys on logout" — verifies iOS/macOS nudge keys survive clearUserScopedStorage().
…dge keys Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed HEAD a00dc62ae5 (4 commits, +177/-182, 13 files). CI green: Lint, Type Check & Build + Type Check + Build + Test + Lint + Socket Security ×2 all success. The right cut for Part D — net deletion PR (-5 lines) via boilerplate removal + DRY win + dead-code removal, with all bot findings closing by construction across the 3 fix commits.
Adoption verification — 7 routed files
skills-tab.tsx✓getLocalBool(TIP_STORAGE_KEY, false)(was raw=== "1"with SSR guard)mic-permission-primer.tsx✓getLocalBool(MIC_PRIMER_STORAGE_KEY, false)(was try/catch +!== "true")chat-route-content.tsx✓getLocalBool/setLocalBool/removeLocalSettingchat-layout.tsx✓ 4-helper mix:getLocalBool/setLocalBool(sidebar collapsed) +getLocalNumber/setLocalNumber(sidebar width). Was raw + try/catch + manualNumber()parsing.impersonate-version-flag.ts✓getLocalSetting/setLocalSetting/removeLocalSetting+ verify-after-writeuse-draft-input.ts✓getLocalSetting/setLocalSettinguse-push-to-talk.ts✓getLocalSettingwith explicit off-default on empty
Files correctly kept inline (sanity-checked via grep against full apps/web/src/**/*.ts*):
current-platform-assistant-store.ts— implements ZustandpersistStateStorageadapter (literally needs raw access)client-feature-flag-store.ts— iterates dynamicLS_PREFIX + keyset; the null-vs-absent distinction mattersgateway-session.ts— auth infrastructure withLS_TOKEN_KEY ?? LEGACY_TOKEN_KEYmulti-key coordinationdevice-settings.ts— typed registry with its ownentry.key ?? entry.legacymigration fallbacksoauth-popup-complete-page.tsx/oauth-complete-page.tsx/google-connect-screen.tsx/integration-detail-modal.tsx(3 remaining) — cross-tab IPC viaStorageEvent, not traditional storagelocal-settings.ts/typed-storage.ts— base layerstorage-migration.ts/session-cleanup.ts/sandbox-bridge.ts— migration/cleanup infrastructure that needs raw access- All grep'd; PR body's justification table covers each.
Bonus wins beyond stated scope
- OAuth utility DRY (-60 lines):
integration-detail-modal.tsxhad a full copy ofOAuthCompletePayload,oauthCompletionStorageKey,getOAuthCompleteMessagePayload,getOAuthCompleteStoragePayloadfromlib/auth/oauth-popup.ts. Now imported from canonical source (verified at L43-47). The 5 call sites (L271/287/339/514/518) all use the imported names. - Nudge legacy cleanup relocation (-26 lines from
nudge-store.ts): Moved fromnudge-store.ts(where it ran inside the store module on first import) tostorage-migration.tsvia 6removeKey()calls + theapp.nudgeLegacy.cleanedflag itself. Two architectural wins from the move: (a) theapp.nudgeLegacy.cleanedflag becomes unnecessary becauseremoveKey()is idempotent (removeItem on absent key is a no-op); (b) cleanup is now co-located with all other one-time storage migrations, single import-time execution point.
Bot findings — all 5 closed at HEAD
| # | Bot | Finding | Closed in |
|---|---|---|---|
| 1 | Codex P2 | Mic primer shown on storage failure (was hidden) | Documented as intentional in PR body — consent-transparency > silent suppression, Devin defended in thread |
| 2 | Devin BUG_0001 | Skills tip dismissed-flag lost ("1" vs "true" format mismatch) | 6086c39 — new migrateValue(key, oldValue, newValue) helper + migrateValue("vellum:skills:tipDismissed", "1", "true") in runStorageMigrations(). Idempotent (only writes when current matches oldValue). Migration order verified correct: migrateKey("vellum:skillsTabTipDismissed", "vellum:skills:tipDismissed") runs first (line 129), then migrateValue("vellum:skills:tipDismissed", "1", "true") (line 176) — handles both renamed-but-old-format AND already-renamed-new-format users. |
| 3 | Devin BUG_0002 | PTT silently activates with Ctrl when storage fails | 6086c39 — raw ? parseActivator(raw) : { kind: "off" } at L122-124. See observation #2 below — this also changes first-time-user default behavior; worth a thought. |
| 4 | Codex P2 | Impersonate flag reloads even when storage write fails | fe5c5d8 — getImpersonatedAssistantVersion() re-read after setLocalSetting/removeLocalSetting. If mismatch, log warn + return early without reload. Both branches (set / clear) handled symmetrically. |
| 5 | Codex P2 | "app." prefix in LEGACY_USER_PREFIXES sweeps active iOS/macOS nudge keys |
a00dc62 — removed "app." entirely from LEGACY_USER_PREFIXES (8 entries remain: onboarding., voice:, gw:, ff:client:, local:, integrations., vellumDebug., vellum_). Dead app.githubNudge.* + app.discordNudge.* + app.nudgeLegacy.cleaned keys removed at startup via explicit removeKey() calls in storage-migration.ts:181-187, not at logout. New test "preserves active app. nudge keys on logout" asserts app.iosNudge.downloaded, app.macOsNudge.bannerDismissed, AND app.githubNudge.starred all survive clearUserScopedStorage(). |
Codex's last pass at fe5c5d8e81 predates a00dc62ae5 — the app. finding is closed at HEAD; one @codex review ping would formalize closure. Devin Review surface mentions 5 additional findings — 30-second look at app.devin.ai/review/.../pull/32652 before merge.
Migration correctness audit (cross-checked against logout-auth-review.md checklist)
- ✓ Navigation: unchanged (logout path is upstream)
- ✓ User-scope sweep:
vellum:prefix + 8 legacy prefixes correct;LEGACY_DEVICE_KEYSSet (9 entries) correctly excludes device-scoped legacy keys (vellum_theme,vellum_share_analytics, etc., plusonboarding.lastUserIdwhich matches"onboarding."prefix but is device-scoped) - ✓ sessionStorage: fully cleared at top of
clearUserScopedStorage() - ✓ BYOK credentials:
vellum:ai:geminiKey/perplexityKey/braveKey/tavilyKeycorrectly clear viavellum:prefix sweep - ✓ Device keys: preserved via Set exclusion in
isUserScopedKey() - ✓ Active nudge keys (
app.iosNudge.*,app.macOsNudge.*): preserved (no longer swept) - ✓ Dead nudge keys (
app.githubNudge.*,app.discordNudge.*,app.nudgeLegacy.cleaned): removed at startup, idempotent (next launch is no-op) - ✓ Verify-after-write on impersonate: matches old catch-block early-return semantics
- ✓ Tests: 47 pass (37 migration + 10 cleanup). New tests:
migrateValue(3 unit) + skills tip integration (1) + nudge cleanup (1) + app. preservation (1)
Non-blocking observations
-
PR body framing of mic-primer change is the right call. Showing the consent flow when storage is broken is correct UX — a permission dialog requires user understanding, and silently suppressing the explanation while still triggering the OS prompt is worse. The intentional-change framing is honest and the Devin defense in-thread is convincing. Good behavioral-change disclosure pattern; this is the shape future PRs that make storage-failure semantics changes should follow.
-
PTT first-time-user default also changed, not just the storage-failure case. The fix
raw ? parseActivator(raw) : { kind: "off" }treats emptyrawas"off". ButgetLocalSetting(key, "")returns""in both scenarios: (a) storage failure, (b) key not present (first-time user). The old code calledparseActivator(localStorage.getItem(key))which fornullreturns{ kind: "modifierOnly", modifiers: ["control"] }— so the OLD first-time-user default was PTT-active-on-Ctrl, and the NEW first-time-user default is PTT-off. Devin's review (and the fix) frames PTT-Ctrl-on-storage-failure as a bug, which I agree with — surprise voice recording on Ctrl is bad UX. But the same change also removes Ctrl-PTT as the first-time-user default. If that default was intentional (Ctrl is a common PTT activator across apps), this is a minor regression for new users. If it was just a side effect ofparseActivator(null)semantics, the change is correct. Worth a one-line PR-body note like the mic-primer one to document the intent. Not blocking. -
Test description
"preserves active app. nudge keys on logout"includesapp.githubNudge.starred(dead key). The assertion is correct from logout's perspective (logout doesn't touch anyapp.*keys after the prefix removal), but the description groups dead and active keys together. A comment in the test body distinguishes them, but the test name could read as "github nudge is active" to a future reader. Cosmetic. -
No test for the impersonate verify-after-write pattern. The fix is correct (verified by tracing) but
setImpersonatedAssistantVersionis a debug surface — it's reasonable to skip. A unit test withMemoryStoragethat throws onsetItemwould lock in the no-reload-on-failure invariant. Minor.
LUM-2042 arc completeness
- PR A (#32604, LUM-2044, typed-storage factory) ✓ merged
- PR B (#32621, LUM-2045, key naming standardization) ✓ merged
- PR C (#32645, LUM-2046, domain storage migration) ✓ merged
- PR D (this one, LUM-2047, route inline calls) — this approval closes the arc
Four-PR sequencing was the right shape: factory → keys → high-traffic domain stores → leaf consumers. Each PR independently reviewable, no Big Bang merge. Per the carry-forward from #32645, every callsite migration to the snapshot-caching accessor was audited for load*().push/splice/sort/assign-style mutation; this PR's adoption is all imperative get/set helpers (not snapshot accessors), so the mutation-leak class doesn't apply here.
Merge gate: CI green at HEAD. All 5 bot findings substantively closed. Codex's last pass (fe5c5d8e81) predates the app. removal in a00dc62. Fresh @codex review + @devin-ai review this PR at HEAD would formalize closure.
|
Addressing observations from vex-assistant-bot review: #2 (PTT first-time-user default): Good catch. The old #3 (Test description): Agreed cosmetic — the in-test comment already distinguishes active vs dead keys. The assertion is correct: logout should not touch any #4 (No impersonate verify-after-write test): Agreed this is minor for a debug surface. Could add one later if the pattern gets reused elsewhere. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a00dc62ae5
ℹ️ 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".
…absent) Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
devin-ai-integration
Bot
dismissed
vex-assistant-bot[bot]’s stale review
via
8c925e2
Test Results — localStorage Routing (LUM-2047)Tested via Playwright CDP against Vite dev server (port 3001). All assertions programmatic via Results: 17/17 passed
Test methodology
|
There was a problem hiding this comment.
Re-APPROVE at HEAD 8c925e26de after one new commit. CI 7/7 green.
What changed since a00dc62ae5:
8c925e26de—voice-page.tsx:PushToTalkCardinitial state aligned with runtime. Same shape asuse-push-to-talk.ts:124:raw ? parseActivator(raw) : { kind: "off" }. Directly addresses my non-blocking observation #2 from prior review. Symmetric grep confirms these are the only twoparseActivator(getLocalSetting(...))call sites — both now agree the empty-string case →{ kind: "off" }, so first-time-user default + storage-failure default + settings-page initial-render all converge on "PTT off" instead of the accidental Ctrl-PTT-default that fell out ofparseActivator(null)semantics.
Bot consensus at HEAD:
- Codex re-review at
a00dc62ae5posted with zero findings (the silent-pass shape). No new inline comments. - Devin's
00:59:20Zcomment substantively addressed my 3 observations: agreed on #2 (Ctrl-PTT-default was unintentional fallout ofparseActivator(null)semantics, not a deliberate design — and updated the PR body), agreed on #3 (test description cosmetic), agreed on #4 (impersonate verify-after-write test not worth the noise for a debug surface). - Devin's
01:06:47ZPlaywright report: 17/17 passed, including the exact scenarios I called out as needing manual coverage: 1× skills tip"1"→"true"migration, 2× sidebar round-trips, 1× PTT off-by-default, 7× dead-key removal (app.githubNudge.*,app.discordNudge.*,app.nudgeLegacy.cleaned), 3× active-key preservation (app.iosNudge.*,app.macOsNudge.bannerDismissed), draft-input + disk-pressure + impersonate round-trips. CDP-driven assertions viapage.evaluate()— no flaky GUI interaction.
Merge gate: ✓ Vex APPROVE (this), ✓ Codex no-findings re-review, ✓ Devin behavioral validation 17/17, ✓ CI 7/7. The 4-PR LUM-2042 arc closes. Per the nighttime merge rule, this is mergeable now.
vex-assistant-bot
Bot
deleted the
devin/1780100823-lum-2047-route-inline-localstorage
branch
1 participant
Prompt / plan
Final PR in the localStorage consolidation effort (LUM-2042). Routes all remaining inline
localStorage.getItem/setItem/removeItemcalls through the sharedlocal-settings.tshelpers established in LUM-2044.Closes LUM-2047
Why needed
After PRs A–C (LUM-2044 typed-storage factory, LUM-2045 key naming standardization, LUM-2046 domain storage migration), 10 production files still used raw
localStoragecalls with duplicated SSR guards, try/catch wrappers, andtypeof windowchecks. This final PR eliminates that boilerplate so all localStorage access flows through a single code path with consistent error handling, SSR safety, and same-tab change notifications.Changes
Route inline calls through
local-settings.ts(7 files, net −69 lines)skills-tab.tsxwindow.localStorage.getItem(KEY) === "1"+ SSR guardgetLocalBool(KEY, false)mic-permission-primer.tsxlocalStorage.getItem(KEY) !== "true"+ try/catchgetLocalBool(KEY, false)chat-route-content.tsxgetLocalBool/setLocalBool/removeLocalSettingchat-layout.tsxwindow.localStorage.getItem/setItem+ try/catch + Number parsinggetLocalBool/setLocalBool/getLocalNumber/setLocalNumberimpersonate-version-flag.tswindow.localStorage.getItem/setItem/removeItem+ SSR + try/catchgetLocalSetting/setLocalSetting/removeLocalSetting+ verify-after-writeuse-draft-input.tswindow.localStorage.getItem/setItem+ SSR + try/catchgetLocalSetting/setLocalSettinguse-push-to-talk.tswindow.localStorage.getItem+ try/catchgetLocalSettingwith explicit off-defaultFix DRY: deduplicate OAuth utilities (−60 lines)
integration-detail-modal.tsxhad a full copy of 5 functions/types fromlib/auth/oauth-popup.ts(OAuthCompletePayload,oauthCompletionStorageKey,getOAuthCompleteMessagePayload,getOAuthCompleteStoragePayload). Replaced with imports from the canonical source.Fix: move nudge legacy cleanup to storage-migration.ts
The one-time nudge key cleanup (removing 6 dead
app.githubNudge.*/app.discordNudge.*keys + theapp.nudgeLegacy.cleanedflag) was running insidenudge-store.tswith its ownapp.nudgeLegacy.cleanedflag. This is migration infrastructure, not store logic. Moved tostorage-migration.tsusing the newremoveKey()helper, which makes the cleanup idempotent without needing a flag.Note: the
"app."prefix was intentionally NOT added toLEGACY_USER_PREFIXESinsession-cleanup.tsbecause active iOS/macOS app-download nudge hooks (use-ios-app-nudge.ts,use-macos-app-nudge.ts) still read/writeapp.iosNudge.*andapp.macOsNudge.*keys. Sweeping allapp.*keys on logout would reset those dismissal states. The dead GitHub/Discord keys are removed at startup by explicitremoveKey()calls instead.Fix: skills tip value format migration
The old code stored the tip-dismissed flag as
"1", butgetLocalBoolonly recognizes"true"/"false". AddedmigrateValue(key, oldValue, newValue)helper tostorage-migration.tsfor same-key value format conversions, and a migration that converts the old"1"to"true"at startup.Fix: PTT activator off-by-default on storage failure
When
localStorageis unavailable,getLocalSettingreturns"", andparseActivator(null)returns{ kind: "modifierOnly", modifiers: ["control"] }— PTT would silently activate with Ctrl. Fixed:raw ? parseActivator(raw) : { kind: "off" }preserves the old catch-block behavior.Fix: impersonate-version-flag reload on failed storage write
When
setLocalSetting/removeLocalSettingswallow a storage exception, execution previously fell through towindow.location.reload()even though the write didn't persist — causing a reload back into the old state. Fixed: verify the write took effect by re-reading the value immediately after. If the value doesn't match, log a warning and return early without reloading — matching the old catch-block behavior.Calls intentionally kept inline
current-platform-assistant-store.tspersistStateStorageadapterclient-feature-flag-store.tsgateway-session.tslocal-settings.tsdevice-settings.tsStorageEvent, not traditional storageIntentional behavioral changes
mic-permission-primer: When
localStorageis unavailable,shouldShowMicPrimer()now returnstrue(show primer) instead offalse(hide). The primer is a consent flow — silently hiding it when storage is broken is worse than showing it again.PTT off-by-default: When
localStorageis unavailable, PTT activator now defaults to{ kind: "off" }instead of{ kind: "modifierOnly", modifiers: ["control"] }. PTT silently activating via Ctrl is a safety risk.Architectural audit
Full 20-item checklist
Test plan
"1"→"true"migration verified via page.evaluate()Safety
Link to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka