refactor(web): migrate domain storage files to typed-storage factory (LUM-2046)

[ashleeradka]

Prompt / plan

Part of the localStorage consolidation effort. This is PR C in the chain:

• PR A: #32604 — typed-storage.ts factory utilities (LUM-2044) ✓ merged
• PR B: #32621 — Standardize key naming (LUM-2045) ✓ merged
• PR C (this): Migrate domain storage files to use the factory (LUM-2046)
• PR D: Route inline localStorage calls through shared utilities (LUM-2047)

What changed

Migrates 5 domain storage files from hand-rolled SSR guards + try/catch + JSON parsing to the typed-storage factory created in LUM-2044:

File	Factory	Before → After
app-pin-storage.ts	createStorageAccessor<PinnedAppEntry[]>	73 → 32 lines
last-viewed-conversation-storage.ts	createKeyedStorageAccessor<string|null>	27 → 27 lines (simplified)
context-window-storage.ts	createRecordStorageAccessor<ContextWindowUsage>	97 → 57 lines
dismissed-surfaces-storage.ts	createRecordStorageAccessor<string[]>	113 → 78 lines
sidebar-group-collapse-storage.ts	2× createKeyedStorageAccessor<string[]>	103 → 66 lines

Net: -264 lines (including new shared test helper and storage validators).

Additional changes

• Extract MemoryStorage test helper (memory-storage.test-helper.ts): DRYs up identical MemoryStorage class + window/localStorage mocking duplicated across 3 test files. New installMemoryStorage() function handles beforeAll/afterAll lifecycle.

• Fix pinApp() mutation bug: pinApp() used entries.push() on the array returned by storage.load(), which mutates the cached fallback array inside the accessor. Fixed to use spread ([...entries, newEntry]). This was latent in the old code too (the fallback was [] defined inline), but becomes observable with the factory's snapshot caching.

• Extract shared storage-validators.ts: isStringArray type guard was duplicated across dismissed-surfaces-storage.ts and sidebar-group-collapse-storage.ts. Extracted to domains/chat/utils/storage-validators.ts (both consumers are within the chat domain).

• Colocate misplaced test file: last-viewed-conversation-storage.test.ts was in domains/chat/utils/ but its source is in utils/ (cross-domain shared utility used by both conversations/ and logs/). Moved test to utils/ to colocate per CONVENTIONS.md.

Audit summary (20-point checklist)

All 20 items from the architectural audit checklist pass:

• Location ✓, Abstraction type ✓, Domain boundaries ✓, DRY ✓ (fixed), State management ✓, Patterns ✓, Dead code ✓, Behavioral preservation ✓, Tests ✓ (59 tests), Tech debt ✓ (reduced), Cross-domain imports ✓ (no new allowlist entries), lib/ scoping ✓, No barrel files ✓, Directory structure ✓ (fixed), File size ✓, Security ✓, Documentation ✓

Why this is safe

• Public API unchanged: Every migrated file exports the same functions with the same signatures. No consumer changes needed.
• Behavioral equivalence: The factory applies the same SSR guards, try/catch error swallowing, JSON parsing, and maxEntries trimming that each file implemented manually. The only behavioral difference is snapshot caching in createStorageAccessor (returns stable references for unchanged values), which is strictly better for React rendering.
• All existing tests pass: 59 tests across 4 test files, all green.

Alternatives not taken

• Zustand persist middleware: Only appropriate for store-backed state. These storage files are imperative read/write utilities, not stores. TkDodo confirms: keep Zustand store scope small.
• Third-party typed-storage libraries: Immature (0-68 GitHub stars), don't handle our scoping/cleanup needs. Our factory is ~200 lines built on existing local-settings.ts.

Closes LUM-2046

Test plan

• All 59 unit tests pass across 4 test files (app-pin-storage, last-viewed-conversation-storage, sidebar-group-collapse-storage, typed-storage)
• bun run lint passes
• bunx tsc --noEmit passes
• Pre-commit hooks pass
• CI green (7/7)

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-2046

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 574bf38661

ℹ️ 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]

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

[vex-assistant-bot]

Approve — PR C of the LUM-2042 chain, clean migration of 5 storage files to the typed-storage factory

7/7 CI green. Boss-authored via Devin. Two commits: 574bf38661 (migration) → 9b79810717 (audit fixes).

What I verified

Factory adoption (6/6 accessors, correct factory per shape):

• app-pin-storage.ts → createStorageAccessor<PinnedAppEntry[]> (static key, needs snapshot caching for useValue)
• last-viewed-conversation-storage.ts → createKeyedStorageAccessor<string | null> (per-assistant, imperative-only consumers)
• context-window-storage.ts → createRecordStorageAccessor<ContextWindowUsage> (Map-like, maxEntries: 200)
• dismissed-surfaces-storage.ts → createRecordStorageAccessor<string[]> (Map-like, maxEntries: 200)
• sidebar-group-collapse-storage.ts → 2× createKeyedStorageAccessor<string[]> (separate keys per Radix root — correct, would have been a regression to share)

Behavioral equivalence — line-by-line diff against parent (6f961ed1):

• SSR guards: old typeof window === "undefined" checks → factory's readRaw. ✓
• Empty-string handling on last-viewed-conversation: old raw && raw.length > 0 ? raw : null → new parse: (raw) => (raw.length > 0 ? raw : null), fallback: null. Equivalent (factory short-circuits on raw === null). ✓
• MAX_IDS_PER_CONVERSATION = 500 cap on dismissed-surfaces preserved at call site (factory maxEntries: 200 is record-level, the 500 is entry-level — two-level cap correctly retained). ✓
• loadOpenCategories stale-key filter via OPEN_CATEGORY_KEYS set preserved. ✓
• context-window maxEntries trim — numeric-key caveat in factory docstring N/A here (conversation IDs are UUIDs). ✓

pinApp() mutation fix — real bug caught. Old: entries.push(...) mutated the cached fallback array [] shared across loadPinnedApps() calls. Latent before (each call returned a fresh JSON.parse array), observable now because the static accessor snapshot-caches the parsed value to keep useSyncExternalStore references stable. Spread ([...entries, newEntry]) is the right fix. PR body calls it out correctly.

Cleanup coverage: all 6 keys are vellum:* prefixed → handled by #32621's prefix sweep in session-cleanup.ts. No allowlist additions needed. ✓

Tests: 59 tests, all green. installMemoryStorage helper adopted by all 3 consumer test files (sidebar/app-pin/last-viewed), eliminating 3× duplicated MemoryStorage class + window mocking boilerplate. afterAll correctly restores via captured getOwnPropertyDescriptor.

Bot findings — closed

Codex P2 at 574bf38661 on sanitize-display-messages.ts:DEFAULT_TOOL_EXECUTION_TIMEOUT_SEC (inlined = 120 would drift from canonical API contract). → Devin reverted in 9b79810717: inline const deleted, canonical import { DEFAULT_TOOL_EXECUTION_TIMEOUT_SEC } from "@vellumai/assistant-api" restored (the inline was only present because Devin's local generated package was stale; bun run postinstall resolved it). Closed at HEAD. ✓

Devin Review surface flagged 1 inline issue (same as Codex above — also closed) + "3 additional findings" not visible via REST API. PR body's 20-point audit checklist passes. A 30-second look at https://app.devin.ai/review/vellum-ai/vellum-assistant/pull/32645 before merge would close the gate on those 3 cleanly.

Non-blocking observations

1. createKeyedStorageAccessor has no useValue hook by design (factory docstring: "compose with useSyncExternalStore at the call site"). Both consumers here use imperative load/save only, so fine. Worth bookmarking: the next per-entity consumer wanting reactive subscription will need createRecordStorageAccessor even if not record-shaped, or the factory gains a keyed useValue.
2. saveLastViewedConversationId can't clear — public API doesn't expose storage.remove(assistantId), so once set there's no way to null via the module's public functions. Probably fine (callers overwrite), but worth a follow-up clearLastViewedConversationId export if a callsite needs it.
3. createRecordStorageAccessor.load returns { ...fallback } (fresh object each call), no snapshot caching. Means it's not safe for useSyncExternalStore consumers without external memoization. Currently fine (both record consumers wrap in new Map / new Set imperatively). If a reactive record-shaped consumer arrives, factory will need the same snapshot-cache treatment as createStorageAccessor.

Carry-forward (procedure)

Snapshot caching changes the rules for callers. Once a factory caches parsed values for reference stability, any caller that previously mutated a loadFoo() return value becomes a live bug. The pinApp fix is the model: when migrating to a snapshot-caching accessor, audit every load*().push/splice/sort/assign for in-place mutation and rewrite as spread/non-mutating. Old code's tests still pass; the leak surfaces only at runtime when the cached reference is reused. Filing in pr-review-practices.

Merge gate: 7/7 CI ✓, Codex P2 substantively closed at HEAD ✓, Devin Review surface findings worth a 30s look. Second-approval criterion met when Codex re-reviews 9b79810717 (current bot pass predates the audit-fix commit).

[devin-ai-integration]

E2E Test Results — localStorage Storage Migration

Ran Playwright CDP route interception tests against Vite dev server (localhost:3001) to verify all 5 migrated storage files work correctly at runtime.

9/9 localStorage assertions passed.

Test Results

#	Test	Result
1	App loads without storage-related errors	PASSED
2a	Pinned apps write format (vellum:pinnedApps)	PASSED
2b	Pinned apps survive reload	PASSED
2c	PinnedAppEntry shape validation	PASSED
3	pinApp mutation fix (spread, not push)	PASSED
4	Sidebar collapse persistence (categories + custom groups)	PASSED
5a	Context window persistence (vellum:ctxwindow:*)	PASSED
5b	Dismissed surfaces persistence (vellum:dismissed-surfaces:*)	PASSED
5c	Last viewed conversation persistence	PASSED
Bonus	All migrated keys use vellum: prefix	PASSED

Escalation: Pre-existing JS error (unrelated)

organizationsList export missing from generated sdk.gen.ts — this is a local-only issue from a stale gitignored generated file. CI passes (7/7) because CI regenerates from source. Not related to this PR's storage changes.

localStorage keys observed

vellum:pinnedApps                           ← migrated
vellum:ctxwindow:asst-test-1                ← migrated
vellum:dismissed-surfaces:asst-test-1       ← migrated
vellum:lastViewedConversation:asst-test-1   ← migrated
vellum:sidebar-open-categories:asst-test-1  ← migrated
vellum:sidebar-open-custom-groups:asst-test-1 ← migrated
vellum:onboarding:completed                 (test bypass)
vellum:onboarding:tosAccepted               (test bypass)
vellum:onboarding:aiDataConsent             (test bypass)

Devin session