From f50971a8d5dbbf1aed076bf8065fb30a6abffa6e Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Tue, 12 May 2026 21:31:16 +0200 Subject: [PATCH] refactor(frontend): wrap UserDraft localStorage payload as { value } MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit localStorage entries now look like {"value": } instead of just . The wrapping is invisible at the API boundary — UserDraft.use, .save, .get, .remove all still operate on the unwrapped draft value — but it leaves room to add metadata (timestamps, originating user, schema version, ...) later without breaking existing entries. Internals: - StoredDraft = { value: V } is what we serialise to localStorage and what useLocalStorageValue's $state holds. - wrap()/unwrap() helpers gate the boundary; the handle returned by use() unwraps on get and wraps on set. - readPersisted() defensively drops entries whose payload isn't a { value: ... } object, so pre-migration drafts written by earlier commits on this branch are simply ignored (has() returns false, get() returns undefined) rather than confusingly surfacing as undefined-shaped drafts. Test data switched from { value: X } (which collides confusingly with the wrapper shape) to plain primitives / objects, plus a regression test for the pre-migration ignore behaviour. 28 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- frontend/src/lib/userDraft.svelte.ts | 77 +++++---- frontend/src/lib/userDraft.test.ts | 224 +++++++++++++-------------- 2 files changed, 155 insertions(+), 146 deletions(-) diff --git a/frontend/src/lib/userDraft.svelte.ts b/frontend/src/lib/userDraft.svelte.ts index 443b3f2fc6..050a8c04c6 100644 --- a/frontend/src/lib/userDraft.svelte.ts +++ b/frontend/src/lib/userDraft.svelte.ts @@ -42,7 +42,14 @@ export type UserDraftUseOptions = UserDraftOptions & { defaultValue?: V } -type DraftState = { val: V | undefined } +/** + * The shape of what we actually persist. Wrapping the value lets us add + * metadata (timestamps, originating user, schema version, ...) later + * without breaking existing entries. + */ +type StoredDraft = { value: V } + +type DraftState = { val: StoredDraft | undefined } type DraftEntry = { count: number @@ -71,13 +78,37 @@ function isLocalOnly(path: string): boolean { return path === '' } -function createInMemoryState(defaultValue: T | undefined): DraftState { - let s = $state(defaultValue) +function wrap(value: V | undefined): StoredDraft | undefined { + return value === undefined ? undefined : { value } +} + +function unwrap(stored: StoredDraft | undefined): V | undefined { + return stored?.value +} + +function readPersisted(key: string): StoredDraft | undefined { + try { + const raw = localStorage.getItem(key) + if (raw == null || raw === 'undefined') return undefined + const parsed = JSON.parse(raw) + // Defensive: drop entries written before the wrapping migration. Their + // raw payload doesn't have a `.value` and would surface as undefined + // anyway — we just don't want to confuse `has()` callers. + if (parsed == null || typeof parsed !== 'object' || !('value' in parsed)) return undefined + return parsed as StoredDraft + } catch (e) { + console.error('UserDraft: localStorage read failed', e) + return undefined + } +} + +function createInMemoryState(defaultValue: StoredDraft | undefined): DraftState { + let s = $state | undefined>(defaultValue) return { - get val(): T | undefined { + get val(): StoredDraft | undefined { return s }, - set val(newVal: T | undefined) { + set val(newVal: StoredDraft | undefined) { s = newVal } } @@ -104,13 +135,13 @@ export const UserDraft = { if (entry) { // Update the shared reactive state so all observers are notified. // For non-empty paths the underlying useLocalStorageValue setter - // persists; for empty paths the in-memory state stays in-memory. - entry.state.val = value + // persists the wrapped value; for empty paths it stays in-memory. + entry.state.val = wrap(value) return } if (isLocalOnly(path)) return try { - localStorage.setItem(localStorageKey(ws, itemKind, path), JSON.stringify(value)) + localStorage.setItem(localStorageKey(ws, itemKind, path), JSON.stringify(wrap(value))) } catch (e) { console.error('UserDraft.save: localStorage write failed', e) } @@ -125,17 +156,10 @@ export const UserDraft = { const mk = mapKey(ws, itemKind, path) const entry = entries.get(mk) if (entry) { - return entry.state.val as V | undefined + return unwrap(entry.state.val as StoredDraft | undefined) } if (isLocalOnly(path)) return undefined - try { - const raw = localStorage.getItem(localStorageKey(ws, itemKind, path)) - if (raw == null || raw === 'undefined') return undefined - return JSON.parse(raw) as V - } catch (e) { - console.error('UserDraft.get: localStorage read failed', e) - return undefined - } + return unwrap(readPersisted(localStorageKey(ws, itemKind, path))) }, /** @@ -150,12 +174,7 @@ export const UserDraft = { const entry = entries.get(mk) if (entry) return entry.state.val !== undefined if (isLocalOnly(path)) return false - try { - const raw = localStorage.getItem(localStorageKey(ws, itemKind, path)) - return raw != null && raw !== 'undefined' - } catch { - return false - } + return readPersisted(localStorageKey(ws, itemKind, path)) !== undefined }, remove(itemKind: UserDraftItemKind, path: string, opts?: UserDraftOptions): void { @@ -174,15 +193,15 @@ export const UserDraft = { ): UserDraftHandle { const ws = resolveWorkspace(opts) const mk = mapKey(ws, itemKind, path) - const defaultValue = opts?.defaultValue + const wrappedDefault = wrap(opts?.defaultValue) let entry = entries.get(mk) if (!entry) { const state: DraftState = isLocalOnly(path) - ? createInMemoryState(defaultValue) - : useLocalStorageValue( + ? createInMemoryState(wrappedDefault) + : useLocalStorageValue | undefined>( localStorageKey(ws, itemKind, path), - defaultValue, + wrappedDefault, undefined, // The first value to flow into the handle (e.g. a backend load // in the editor route) is the baseline — only persist when the @@ -208,12 +227,12 @@ export const UserDraft = { return { get draft(): V | undefined { - return sharedEntry.state.val as V | undefined + return unwrap(sharedEntry.state.val as StoredDraft | undefined) }, set draft(value: V | undefined) { // useLocalStorageValue's setter writes synchronously and // removes the localStorage entry when value is undefined. - sharedEntry.state.val = value + sharedEntry.state.val = wrap(value) } } } diff --git a/frontend/src/lib/userDraft.test.ts b/frontend/src/lib/userDraft.test.ts index fb54995576..80dab74328 100644 --- a/frontend/src/lib/userDraft.test.ts +++ b/frontend/src/lib/userDraft.test.ts @@ -23,6 +23,12 @@ function flushDestroyCallbacks(): void { for (const cb of callbacks) cb() } +// Helper: localStorage payloads are always wrapped as { value: } so +// future metadata fields can be added without breaking existing entries. +function wrapped(value: V): string { + return JSON.stringify({ value }) +} + beforeEach(() => { __resetUserDraftForTesting() onDestroyCallbacks.length = 0 @@ -31,33 +37,39 @@ beforeEach(() => { }) describe('UserDraft.save / get / remove (no observers)', () => { - it('save writes to localStorage under the workspace-scoped key', () => { - UserDraft.save('flow', 'u/me/myflow', { value: { hello: 'world' } }) + it('save writes a wrapped { value } payload under the workspace-scoped key', () => { + UserDraft.save('flow', 'u/me/myflow', { hello: 'world' }) const raw = localStorage.getItem('userdraft/w/test_ws/flow/u/me/myflow') - expect(raw).toBe(JSON.stringify({ value: { hello: 'world' } })) + expect(raw).toBe(wrapped({ hello: 'world' })) }) - it('get reads from localStorage when no observer is registered', () => { - localStorage.setItem( - 'userdraft/w/test_ws/script/u/me/script1', - JSON.stringify({ value: 'code' }) - ) + it('get reads from a wrapped localStorage payload when no observer is registered', () => { + localStorage.setItem('userdraft/w/test_ws/script/u/me/script1', wrapped('code')) - expect(UserDraft.get('script', 'u/me/script1')).toEqual({ value: 'code' }) + expect(UserDraft.get('script', 'u/me/script1')).toBe('code') }) it('get returns undefined when nothing is stored', () => { expect(UserDraft.get('flow', 'u/me/missing')).toBeUndefined() }) - it('get returns undefined when stored value is malformed', () => { + it('get returns undefined when the stored payload is malformed', () => { localStorage.setItem('userdraft/w/test_ws/flow/u/me/bad', 'not-json') expect(UserDraft.get('flow', 'u/me/bad')).toBeUndefined() }) + it('get returns undefined when the stored payload is unwrapped (pre-migration entry)', () => { + // Drafts written before the wrapping was introduced look like the raw + // value rather than { value: ... }. They must be ignored rather than + // surface as undefined-shaped drafts. + localStorage.setItem('userdraft/w/test_ws/flow/u/me/raw', JSON.stringify({ hello: 'world' })) + expect(UserDraft.get('flow', 'u/me/raw')).toBeUndefined() + expect(UserDraft.has('flow', 'u/me/raw')).toBe(false) + }) + it('remove clears the localStorage entry', () => { - UserDraft.save('app', 'u/me/app1', { value: { grid: [] } }) + UserDraft.save('app', 'u/me/app1', { grid: [] }) expect(localStorage.getItem('userdraft/w/test_ws/app/u/me/app1')).not.toBeNull() UserDraft.remove('app', 'u/me/app1') @@ -65,100 +77,91 @@ describe('UserDraft.save / get / remove (no observers)', () => { }) it('uses the workspace from opts when provided', () => { - UserDraft.save('flow', 'u/me/f', { value: 1 }, { workspace: 'other_ws' }) + UserDraft.save('flow', 'u/me/f', 1, { workspace: 'other_ws' }) - expect(localStorage.getItem('userdraft/w/other_ws/flow/u/me/f')).toBe( - JSON.stringify({ value: 1 }) - ) + expect(localStorage.getItem('userdraft/w/other_ws/flow/u/me/f')).toBe(wrapped(1)) // Default workspace key must remain empty. expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/f')).toBeNull() }) it('supports trigger kinds as item kinds', () => { - UserDraft.save('trigger_kafka', 'u/me/topic1', { value: { brokers: ['localhost:9092'] } }) + UserDraft.save('trigger_kafka', 'u/me/topic1', { brokers: ['localhost:9092'] }) const raw = localStorage.getItem('userdraft/w/test_ws/trigger_kafka/u/me/topic1') - expect(raw).toBe(JSON.stringify({ value: { brokers: ['localhost:9092'] } })) + expect(raw).toBe(wrapped({ brokers: ['localhost:9092'] })) }) it('throws when neither opts.workspace nor $workspaceStore is set', () => { workspaceStore.set(undefined) - expect(() => UserDraft.save('flow', 'u/me/x', { value: 1 })).toThrow(/no workspace/) + expect(() => UserDraft.save('flow', 'u/me/x', 1)).toThrow(/no workspace/) }) }) describe('UserDraft.use() — observer sync', () => { it('loads the existing localStorage value on first use', () => { - localStorage.setItem( - 'userdraft/w/test_ws/flow/u/me/loaded', - JSON.stringify({ value: 'preloaded' }) - ) + localStorage.setItem('userdraft/w/test_ws/flow/u/me/loaded', wrapped('preloaded')) - const handle = UserDraft.use<{ value: string }>('flow', 'u/me/loaded') - expect(handle.draft).toEqual({ value: 'preloaded' }) + const handle = UserDraft.use('flow', 'u/me/loaded') + expect(handle.draft).toBe('preloaded') }) it('two handles on the same key share the same underlying state', () => { - const a = UserDraft.use<{ value: number }>('flow', 'u/me/shared') - const b = UserDraft.use<{ value: number }>('flow', 'u/me/shared') + const a = UserDraft.use('flow', 'u/me/shared') + const b = UserDraft.use('flow', 'u/me/shared') - a.draft = { value: 42 } - expect(b.draft).toEqual({ value: 42 }) + a.draft = 42 + expect(b.draft).toBe(42) - b.draft = { value: 99 } - expect(a.draft).toEqual({ value: 99 }) + b.draft = 99 + expect(a.draft).toBe(99) }) it('save() propagates to live use() handles (in-memory)', () => { - const handle = UserDraft.use<{ value: number }>('flow', 'u/me/observed') + const handle = UserDraft.use('flow', 'u/me/observed') expect(handle.draft).toBeUndefined() // First write through a live entry is treated as the "initial value" // (saveInitialValue=false) and is NOT persisted — observers still see it. - UserDraft.save('flow', 'u/me/observed', { value: 7 }) - expect(handle.draft).toEqual({ value: 7 }) + UserDraft.save('flow', 'u/me/observed', 7) + expect(handle.draft).toBe(7) expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/observed')).toBeNull() // Subsequent writes persist. - UserDraft.save('flow', 'u/me/observed', { value: 9 }) - expect(handle.draft).toEqual({ value: 9 }) - expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/observed')).toBe( - JSON.stringify({ value: 9 }) - ) + UserDraft.save('flow', 'u/me/observed', 9) + expect(handle.draft).toBe(9) + expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/observed')).toBe(wrapped(9)) }) it('remove() clears localStorage without touching the in-memory handle', () => { // Seed localStorage so the live handle initialises from it. - localStorage.setItem('userdraft/w/test_ws/flow/u/me/removed', JSON.stringify({ value: 1 })) - const handle = UserDraft.use<{ value: number }>('flow', 'u/me/removed') - expect(handle.draft).toEqual({ value: 1 }) + localStorage.setItem('userdraft/w/test_ws/flow/u/me/removed', wrapped(1)) + const handle = UserDraft.use('flow', 'u/me/removed') + expect(handle.draft).toBe(1) UserDraft.remove('flow', 'u/me/removed') // Live handle keeps its current value — remove() only wipes the // persisted side. This is what lets callers run UserDraft.remove // during navigation without flickering the editor UI. - expect(handle.draft).toEqual({ value: 1 }) + expect(handle.draft).toBe(1) expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/removed')).toBeNull() }) it('the second write through the handle setter persists to localStorage', () => { - const handle = UserDraft.use<{ value: string }>('flow', 'u/me/setter') + const handle = UserDraft.use('flow', 'u/me/setter') // First write is the baseline — not persisted. - handle.draft = { value: 'initial' } + handle.draft = 'initial' expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/setter')).toBeNull() // Second (and onwards) persists. - handle.draft = { value: 'persisted' } - expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/setter')).toBe( - JSON.stringify({ value: 'persisted' }) - ) + handle.draft = 'persisted' + expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/setter')).toBe(wrapped('persisted')) }) it('setting handle.draft = undefined after edits removes the localStorage entry', () => { - const handle = UserDraft.use<{ value: string }>('flow', 'u/me/clear') - handle.draft = { value: 'initial' } // baseline, not persisted - handle.draft = { value: 'edited' } // persisted + const handle = UserDraft.use('flow', 'u/me/clear') + handle.draft = 'initial' // baseline, not persisted + handle.draft = 'edited' // persisted expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/clear')).not.toBeNull() handle.draft = undefined @@ -167,118 +170,109 @@ describe('UserDraft.use() — observer sync', () => { }) it('two handles in different workspaces are isolated', () => { - const a = UserDraft.use<{ value: number }>('flow', 'u/me/iso', { workspace: 'ws_a' }) - const b = UserDraft.use<{ value: number }>('flow', 'u/me/iso', { workspace: 'ws_b' }) + const a = UserDraft.use('flow', 'u/me/iso', { workspace: 'ws_a' }) + const b = UserDraft.use('flow', 'u/me/iso', { workspace: 'ws_b' }) - a.draft = { value: 1 } - b.draft = { value: 2 } + a.draft = 1 + b.draft = 2 - expect(a.draft).toEqual({ value: 1 }) - expect(b.draft).toEqual({ value: 2 }) + expect(a.draft).toBe(1) + expect(b.draft).toBe(2) }) it('save() falls back to localStorage when no handle is registered', () => { - UserDraft.save('flow', 'u/me/noobs', { value: 'fallback' }) + UserDraft.save('flow', 'u/me/noobs', 'fallback') // First use() afterwards loads the persisted value. - const handle = UserDraft.use<{ value: string }>('flow', 'u/me/noobs') - expect(handle.draft).toEqual({ value: 'fallback' }) + const handle = UserDraft.use('flow', 'u/me/noobs') + expect(handle.draft).toBe('fallback') }) }) describe('UserDraft.use() — defaultValue', () => { it('returns defaultValue when localStorage has no entry', () => { - const handle = UserDraft.use<{ value: string }>('flow', 'u/me/withdefault', { - defaultValue: { value: 'fallback' } - }) + const handle = UserDraft.use('flow', 'u/me/withdefault', { defaultValue: 'fallback' }) - expect(handle.draft).toEqual({ value: 'fallback' }) + expect(handle.draft).toBe('fallback') }) it('does not persist the defaultValue on first read', () => { - UserDraft.use<{ value: string }>('flow', 'u/me/lazyDefault', { - defaultValue: { value: 'fallback' } - }) + UserDraft.use('flow', 'u/me/lazyDefault', { defaultValue: 'fallback' }) expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/lazyDefault')).toBeNull() }) it('localStorage value wins over defaultValue', () => { - localStorage.setItem( - 'userdraft/w/test_ws/flow/u/me/overridden', - JSON.stringify({ value: 'persisted' }) - ) + localStorage.setItem('userdraft/w/test_ws/flow/u/me/overridden', wrapped('persisted')) - const handle = UserDraft.use<{ value: string }>('flow', 'u/me/overridden', { - defaultValue: { value: 'fallback' } + const handle = UserDraft.use('flow', 'u/me/overridden', { + defaultValue: 'fallback' }) - expect(handle.draft).toEqual({ value: 'persisted' }) + expect(handle.draft).toBe('persisted') }) it('second write through the setter persists even though defaultValue was set', () => { - const handle = UserDraft.use<{ value: string }>('flow', 'u/me/writeDefault', { - defaultValue: { value: 'fallback' } + const handle = UserDraft.use('flow', 'u/me/writeDefault', { + defaultValue: 'fallback' }) // First write is the initial-value baseline. - handle.draft = { value: 'initial' } + handle.draft = 'initial' expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/writeDefault')).toBeNull() - handle.draft = { value: 'modified' } + handle.draft = 'modified' expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/writeDefault')).toBe( - JSON.stringify({ value: 'modified' }) + wrapped('modified') ) }) }) describe('UserDraft — empty path (new-item, in-memory only)', () => { it('use() with empty path uses defaultValue and does not persist', () => { - const handle = UserDraft.use<{ value: number }>('flow', '', { - defaultValue: { value: 0 } - }) + const handle = UserDraft.use('flow', '', { defaultValue: 0 }) - handle.draft = { value: 99 } + handle.draft = 99 - expect(handle.draft).toEqual({ value: 99 }) + expect(handle.draft).toBe(99) // No localStorage key with empty path should ever be written. expect(localStorage.getItem('userdraft/w/test_ws/flow/')).toBeNull() }) it('two handles with empty path share in-memory state per workspace', () => { - const a = UserDraft.use<{ value: number }>('flow', '') - const b = UserDraft.use<{ value: number }>('flow', '') + const a = UserDraft.use('flow', '') + const b = UserDraft.use('flow', '') - a.draft = { value: 1 } - expect(b.draft).toEqual({ value: 1 }) + a.draft = 1 + expect(b.draft).toBe(1) - b.draft = { value: 2 } - expect(a.draft).toEqual({ value: 2 }) + b.draft = 2 + expect(a.draft).toBe(2) }) it('save() with empty path is a no-op against localStorage but updates live handles', () => { - const handle = UserDraft.use<{ value: number }>('flow', '') + const handle = UserDraft.use('flow', '') - UserDraft.save('flow', '', { value: 5 }) - expect(handle.draft).toEqual({ value: 5 }) + UserDraft.save('flow', '', 5) + expect(handle.draft).toBe(5) expect(localStorage.getItem('userdraft/w/test_ws/flow/')).toBeNull() }) it('get() with empty path returns the in-memory value when a handle is live, else undefined', () => { expect(UserDraft.get('flow', '')).toBeUndefined() - const handle = UserDraft.use<{ value: number }>('flow', '') - handle.draft = { value: 11 } - expect(UserDraft.get('flow', '')).toEqual({ value: 11 }) + const handle = UserDraft.use('flow', '') + handle.draft = 11 + expect(UserDraft.get('flow', '')).toBe(11) }) it('remove() with empty path is a no-op (no localStorage to clear, in-memory untouched)', () => { - const handle = UserDraft.use<{ value: number }>('flow', '') - handle.draft = { value: 1 } + const handle = UserDraft.use('flow', '') + handle.draft = 1 UserDraft.remove('flow', '') // Empty-path entries never touched localStorage and remove() no longer // resets the in-memory state, so the handle keeps its value. - expect(handle.draft).toEqual({ value: 1 }) + expect(handle.draft).toBe(1) expect(localStorage.getItem('userdraft/w/test_ws/flow/')).toBeNull() }) }) @@ -286,12 +280,12 @@ describe('UserDraft — empty path (new-item, in-memory only)', () => { describe('UserDraft.use() — reference counting & cleanup', () => { it('destroys the entry when the last handle is released', () => { // First handle acquires the entry. - const a = UserDraft.use<{ value: number }>('flow', 'u/me/ref') - a.draft = { value: 1 } // baseline write — not persisted + const a = UserDraft.use('flow', 'u/me/ref') + a.draft = 1 // baseline write — not persisted // Second handle increments the count. - const b = UserDraft.use<{ value: number }>('flow', 'u/me/ref') - expect(b.draft).toEqual({ value: 1 }) + const b = UserDraft.use('flow', 'u/me/ref') + expect(b.draft).toBe(1) // onDestroy for both handles got registered. expect(onDestroyCallbacks.length).toBe(2) @@ -300,12 +294,10 @@ describe('UserDraft.use() — reference counting & cleanup', () => { const firstCb = onDestroyCallbacks.shift()! firstCb() - UserDraft.save('flow', 'u/me/ref', { value: 2 }) - expect(a.draft).toEqual({ value: 2 }) + UserDraft.save('flow', 'u/me/ref', 2) + expect(a.draft).toBe(2) // Now persisted (second write after the baseline). - expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/ref')).toBe( - JSON.stringify({ value: 2 }) - ) + expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/ref')).toBe(wrapped(2)) // Releasing the second handle drops the entry; subsequent save() // must go straight to localStorage rather than mutating in-memory @@ -313,21 +305,19 @@ describe('UserDraft.use() — reference counting & cleanup', () => { const secondCb = onDestroyCallbacks.shift()! secondCb() - UserDraft.save('flow', 'u/me/ref', { value: 3 }) - expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/ref')).toBe( - JSON.stringify({ value: 3 }) - ) + UserDraft.save('flow', 'u/me/ref', 3) + expect(localStorage.getItem('userdraft/w/test_ws/flow/u/me/ref')).toBe(wrapped(3)) }) it('a fresh use() after cleanup re-reads the latest persisted value', () => { - const a = UserDraft.use<{ value: string }>('flow', 'u/me/cycle') - a.draft = { value: 'initial' } // baseline — not persisted - a.draft = { value: 'edited' } // persisted + const a = UserDraft.use('flow', 'u/me/cycle') + a.draft = 'initial' // baseline — not persisted + a.draft = 'edited' // persisted flushDestroyCallbacks() // After all handles release, a brand-new use() must pick up the // value persisted to localStorage from the previous round. - const b = UserDraft.use<{ value: string }>('flow', 'u/me/cycle') - expect(b.draft).toEqual({ value: 'edited' }) + const b = UserDraft.use('flow', 'u/me/cycle') + expect(b.draft).toBe('edited') }) })