feat(web): add typed-storage factory utilities (LUM-2044)

[ashleeradka]

Summary

Add createStorageAccessor, createKeyedStorageAccessor, and createRecordStorageAccessor factory functions in utils/typed-storage.ts for type-safe localStorage access with useSyncExternalStore integration.

Why

The codebase has ~35 localStorage keys accessed via three inconsistent tiers (see LUM-2042). Four domain storage files (context-window-storage.ts, dismissed-surfaces-storage.ts, sidebar-group-collapse-storage.ts, app-pin-storage.ts) duplicate the exact same SSR-guard → try/catch → JSON.parse → validate → write pattern. This utility provides the shared abstraction they'll all migrate to.

What

Three factory functions, each built on local-settings.ts for consistent SSR guards, error swallowing, and same-tab change notifications:

Factory	Use case	Key feature
createStorageAccessor<T>	Single static key (e.g., vellum:pinnedApps)	useValue() React hook via useSyncExternalStore
createKeyedStorageAccessor<T>	Per-entity keys (e.g., vellum:lastConvo:${assistantId})	Simple load/save/remove per entity
createRecordStorageAccessor<V>	Per-entity record maps (e.g., vellum:ctxwindow:${assistantId} → Record<conversationId, Usage>)	maxEntries trimming, entry-level get/set/delete

All three:

• Declare scope: "user" | "device" for cleanup integration
• Catch errors in both parse and localStorage access (defensive against corrupt data and quota/policy failures)
• Use local-settings.ts for writes → automatic same-tab + cross-tab change events

Why this is safe

• Additive only — no existing code is changed. Zero consumers yet (migration PRs are LUM-2045/2046/2047).
• Built on proven primitives — local-settings.ts wrappers are already used throughout the codebase.
• useSyncExternalStore is React's official recommendation for external stores including localStorage. Already used in 4 existing files in this codebase.

References

• React — useSyncExternalStore
• web.dev — Sign-out best practices (confirms user/device scoping approach)
• Zustand persist middleware — for store-backed state only; NOT appropriate for ad-hoc persistence patterns
• Parent issue: LUM-2042

Prompt / plan

Part A of the 4-PR localStorage consolidation plan (LUM-2042):

• A (this PR): Create typed-storage utility
• B (LUM-2045): Standardize key naming + simplify session cleanup
• C (LUM-2046): Migrate domain storage files to factory
• D (LUM-2047): Route inline localStorage calls through shared utilities

Test plan

24 unit tests covering all three factory functions:

• Static accessor: load/save/remove, fallback on absent/corrupted/invalid data, boolean and number variants
• Keyed accessor: per-entity isolation, remove, keyFn exposure
• Record accessor: entry-level CRUD, maxEntries trimming, invalid entry filtering, corrupted JSON handling, non-object JSON handling, unbounded mode

bun test src/utils/typed-storage.test.ts
# 24 pass, 0 fail

Closes LUM-2044

Link to Devin session: https://app.devin.ai/sessions/4b3b130bbf274e5487da3b4992883093
Requested by: @ashleeradka

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[linear]

LUM-2044

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0ea98f1476

ℹ️ 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".

[ashleeradka]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

ℹ️ 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".

[vex-assistant-bot]

✦ APPROVE — reviewed at 4df75e8a

Value: This is Part A of the 4-PR localStorage consolidation (LUM-2042 → 2044/2045/2046/2047). The right shape for a refactor of this size: land the abstraction first with zero consumers and full test coverage, then migrate in subsequent PRs so each migration is reviewable in isolation. Three factories cover the actual access patterns observed in the codebase (single static key, per-entity keyed, per-entity record-with-trimming) — no fourth factory that nobody needs, no missing factory that a domain file will have to escape from.

Codex's P1 was a real bug: useSyncExternalStore calling getSnapshot → load() → fresh JSON.parse on every render means a new array/object reference even when localStorage didn't change. React then warns about an infinite render loop. Devin's fix in 4df75e8a (raw-string cache → parsed-value cache, identity check via ===) is correct and the new test (first === second after consecutive loads) actually exercises the property useSyncExternalStore relies on. The P2 (maxEntries leftover on the keyed config) was a vestige — removed cleanly.

Architectural observations, drift risks, and small things worth a comment

Asymmetry the migration will encounter

createStorageAccessor has snapshot caching, useValue, and useSyncExternalStore integration. createKeyedStorageAccessor has none of those — it's a purely imperative load(id) / save(id) / remove(id) shape. That's defensible (no current consumer needs a per-entity React hook), but the asymmetry is invisible from the call site. When LUM-2046 migrates a domain file that wants useValue(id), the answer will be "use createRecordStorageAccessor with a single-entry record" or "lift to createStorageAccessor with key=keyFn(id)" — neither obvious.

Two ways to handle this without bloating Part A:

• One-sentence JSDoc on KeyedStorageAccessorConfig explaining the asymmetry: "intentionally no useValue hook; if you need React subscription for per-entity data, prefer createRecordStorageAccessor or compose with useSyncExternalStore at the call site." Prevents a future contributor from "adding it for symmetry" and re-introducing the snapshot-stability bug.
• Or surface this in LUM-2042's tracking issue so PR C (LUM-2046) considers it when migrating.

Non-blocking either way.

createRecordStorageAccessor.set does a full parse+validate on every write

function set(id, entryKey, value) {
  const existing = load(id);    // parseRecord → JSON.parse + iterate + validate every entry
  existing[entryKey] = value;
  ...
  setLocalSetting(keyFn(id), JSON.stringify(existing));   // re-serialize everything
}

For the documented ctxWindow use case (maxEntries: 200, per-conversation usage data), each set is O(200) parse + O(200) validate + O(200) serialize. Fine at 200 entries; would matter at 10k. Worth keeping maxEntries as a hard ceiling (which it already is) and not exposing this factory for unbounded record patterns — which the migration plan looks like it's already doing.

If hot paths ever land on this (e.g., set-on-every-keystroke during a streaming response), the fix is to thread a "skip parse, trust last loaded value" through set. Don't pre-optimize; just be aware.

maxEntries trim relies on insertion-order

const entries = Object.entries(existing);
const trimmed = entries.slice(entries.length - maxEntries);

Object.entries preserves string-key insertion order per ES2015, which is what makes "drop oldest" work. The test (k1 dropped, k2/k3/k4 retained) verifies the behavior end-to-end. Worth one line of comment on maxEntries in the config doc: "Trim is by Object insertion order; numeric-string keys would sort first per the spec and break this." If domain code ever uses entry keys like "1", "2" instead of "conv-abc", oldest-first goes out the window. The vellum: prefix on keyFn outputs makes this unlikely, but the constraint is implicit.

Cache is per-accessor-instance, not per-key

If two createStorageAccessor calls are ever made with the same key (mistake or otherwise), each gets its own cachedRaw / cachedValue. Both subscribe to watchSetting and both re-read on change, so they stay consistent — but referential stability is only guaranteed within a single accessor's load() calls, not across. For the migration this means "one accessor per key, exported as a module singleton" should be the documented pattern in the JSDoc.

The current pinnedApps example already implies this (one const pinnedApps = createStorageAccessor(…)), but worth one line of explicit guidance: "Export one accessor per key from a shared module; don't re-create at the call site."

Small wins worth flagging back

• The decision to NOT update the cache synchronously inside save() is correct. Save writes to localStorage, watchSetting fires the change event, subscribers see it on next render. The cache invalidates lazily via raw === cachedRaw mismatch on next load(). Synchronous cache updates inside save() would create a subtle bug where a same-tab save bypasses the watchSetting notification path and other subscribers don't re-render.

• The parseRecord filter pattern (validate each entry, drop invalid ones, keep good ones) is the right policy for storage that survives schema migrations. Other "drop the whole record if any entry is bad" patterns would lose user data on a single bad entry from a previous version.

• getServerSnapshot returning fallback (not invoking load) is correct for SSR. Calling localStorage server-side would crash; calling load() server-side would return fallback anyway via readRaw's typeof window guard, but returning the constant is faster and clearer.

Small nit

createRecordStorageAccessor.load returns { ...fallback } (defensive copy) on the empty path but returns the freshly-built result object from parseRecord on the populated path. Two different reference-stability semantics in the same function. Tests don't exercise this (each test calls load once or compares values, not references). If a future consumer adds useValue to this accessor, the asymmetry will bite. Either: always return the parsed result by reference and accept that callers shouldn't mutate; or always defensive-copy. The choice between them is consistent caller-mutation policy — pick one and document it on RecordStorageAccessor.load.

CI + bots

• ✅ 7/7 CI green at 4df75e8a (Lint, Type Check, Build, Test, aggregate gate, Socket Security ×2)
• ✅ Codex: re-reviewed clean ("Chef's kiss") after the 4df75e8a follow-up
• ✅ Devin: posted "Fixed in 4df75e8" on both flagged inline comments with specific evidence
• Both bot findings substantively addressed; second-approval criterion met.

Vellum Constitution — Trust-seeking + DRY at the right layer: the four domain storage files duplicating SSR-guard → try/catch → JSON.parse → validate → write were each one regression away from drifting. Landing the abstraction first (zero consumers, full test coverage, both Codex flags fixed in the same PR) means the migration PRs can be reviewed for "did the domain logic survive the lift" instead of re-litigating the abstraction's contract on every PR.