fix(02-review): WR-04 require explicit persist on resources.ts entry()/optimistic()
entry(key, persist = true) and optimistic(key, update) silently defaulted
to persist:true after the first call for a key, and optimistic() didn't
accept a persist argument at all. Every current call site happened to be
safe, but the invariant was unenforced: a future caller invoking
store.optimistic() before any useCachedResource({persist:false}) has run
for that key in the same tick would silently start writing to
sessionStorage with no indication anything is wrong (T-02-01).
persist is now a required argument on both functions (no default), and the
per-key decision is recorded and asserted (dev-only warning) against any
later call that disagrees. useCachedResource's optimistic() wrapper now
threads its own already-resolved persist value through automatically, so
no existing composable caller changes behavior. The two call sites that
use the resources store directly (Cloud.vue/PeerFiles.vue's per-peer
browse cache) now pass persist:true explicitly, matching their existing
behavior exactly.
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -126,6 +126,9 @@ export function useCachedResource<T>(opts: CachedResourceOptions<T>): CachedReso
|
|||||||
ageMs: computed(() => (entry.fetchedAt === null ? null : Date.now() - entry.fetchedAt)),
|
ageMs: computed(() => (entry.fetchedAt === null ? null : Date.now() - entry.fetchedAt)),
|
||||||
refresh,
|
refresh,
|
||||||
invalidate: () => store.invalidate(opts.key),
|
invalidate: () => store.invalidate(opts.key),
|
||||||
optimistic: (update) => store.optimistic<T>(opts.key, update),
|
// Pass this resource's own already-decided `persist` through explicitly
|
||||||
|
// (WR-04) — store.optimistic() requires it rather than defaulting, so
|
||||||
|
// the entry's persist decision can never silently diverge by omission.
|
||||||
|
optimistic: (update) => store.optimistic<T>(opts.key, update, persist),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -16,7 +16,7 @@ describe('resources store — stale-while-revalidate semantics', () => {
|
|||||||
|
|
||||||
it('first fetch goes idle → loading → ready with data', async () => {
|
it('first fetch goes idle → loading → ready with data', async () => {
|
||||||
const store = useResourcesStore()
|
const store = useResourcesStore()
|
||||||
const e = store.entry<string>('k1')
|
const e = store.entry<string>('k1', true)
|
||||||
expect(e.loadState).toBe('idle')
|
expect(e.loadState).toBe('idle')
|
||||||
const p = store.refresh('k1', async () => 'hello')
|
const p = store.refresh('k1', async () => 'hello')
|
||||||
expect(e.loadState).toBe('loading')
|
expect(e.loadState).toBe('loading')
|
||||||
@@ -29,7 +29,7 @@ describe('resources store — stale-while-revalidate semantics', () => {
|
|||||||
it('sticky-ready: refresh never regresses ready → loading', async () => {
|
it('sticky-ready: refresh never regresses ready → loading', async () => {
|
||||||
const store = useResourcesStore()
|
const store = useResourcesStore()
|
||||||
await store.refresh('k2', async () => 1)
|
await store.refresh('k2', async () => 1)
|
||||||
const e = store.entry<number>('k2')
|
const e = store.entry<number>('k2', true)
|
||||||
const p = store.refresh('k2', async () => 2)
|
const p = store.refresh('k2', async () => 2)
|
||||||
expect(e.loadState).toBe('refreshing')
|
expect(e.loadState).toBe('refreshing')
|
||||||
await p
|
await p
|
||||||
@@ -40,7 +40,7 @@ describe('resources store — stale-while-revalidate semantics', () => {
|
|||||||
it('keeps last-known data on refresh error (ready + error set)', async () => {
|
it('keeps last-known data on refresh error (ready + error set)', async () => {
|
||||||
const store = useResourcesStore()
|
const store = useResourcesStore()
|
||||||
await store.refresh('k3', async () => 'good')
|
await store.refresh('k3', async () => 'good')
|
||||||
const e = store.entry<string>('k3')
|
const e = store.entry<string>('k3', true)
|
||||||
await store.refresh('k3', async () => {
|
await store.refresh('k3', async () => {
|
||||||
throw new Error('boom')
|
throw new Error('boom')
|
||||||
})
|
})
|
||||||
@@ -54,7 +54,7 @@ describe('resources store — stale-while-revalidate semantics', () => {
|
|||||||
await store.refresh('k4', async () => {
|
await store.refresh('k4', async () => {
|
||||||
throw new Error('down')
|
throw new Error('down')
|
||||||
})
|
})
|
||||||
const e = store.entry('k4')
|
const e = store.entry('k4', true)
|
||||||
expect(e.loadState).toBe('error')
|
expect(e.loadState).toBe('error')
|
||||||
expect(e.data).toBeNull()
|
expect(e.data).toBeNull()
|
||||||
})
|
})
|
||||||
@@ -74,7 +74,7 @@ describe('resources store — stale-while-revalidate semantics', () => {
|
|||||||
// Fresh pinia = fresh memory cache, same sessionStorage.
|
// Fresh pinia = fresh memory cache, same sessionStorage.
|
||||||
setActivePinia(createPinia())
|
setActivePinia(createPinia())
|
||||||
const store2 = useResourcesStore()
|
const store2 = useResourcesStore()
|
||||||
const e = store2.entry<{ n: number }>('k6')
|
const e = store2.entry<{ n: number }>('k6', true)
|
||||||
expect(e.loadState).toBe('ready')
|
expect(e.loadState).toBe('ready')
|
||||||
expect(e.data).toEqual({ n: 42 })
|
expect(e.data).toEqual({ n: 42 })
|
||||||
})
|
})
|
||||||
@@ -82,8 +82,8 @@ describe('resources store — stale-while-revalidate semantics', () => {
|
|||||||
it('optimistic update applies immediately and rollback restores', async () => {
|
it('optimistic update applies immediately and rollback restores', async () => {
|
||||||
const store = useResourcesStore()
|
const store = useResourcesStore()
|
||||||
await store.refresh('k7', async () => ['a'])
|
await store.refresh('k7', async () => ['a'])
|
||||||
const e = store.entry<string[]>('k7')
|
const e = store.entry<string[]>('k7', true)
|
||||||
const rollback = store.optimistic<string[]>('k7', (cur) => [...(cur ?? []), 'b'])
|
const rollback = store.optimistic<string[]>('k7', (cur) => [...(cur ?? []), 'b'], true)
|
||||||
expect(e.data).toEqual(['a', 'b'])
|
expect(e.data).toEqual(['a', 'b'])
|
||||||
rollback()
|
rollback()
|
||||||
expect(e.data).toEqual(['a'])
|
expect(e.data).toEqual(['a'])
|
||||||
@@ -95,7 +95,7 @@ describe('resources store — stale-while-revalidate semantics', () => {
|
|||||||
const revalidate = vi.fn()
|
const revalidate = vi.fn()
|
||||||
store.subscribe('k8', revalidate)
|
store.subscribe('k8', revalidate)
|
||||||
store.invalidate('k8')
|
store.invalidate('k8')
|
||||||
expect(store.entry('k8').fetchedAt).toBeNull()
|
expect(store.entry('k8', true).fetchedAt).toBeNull()
|
||||||
expect(revalidate).not.toHaveBeenCalled()
|
expect(revalidate).not.toHaveBeenCalled()
|
||||||
vi.advanceTimersByTime(900)
|
vi.advanceTimersByTime(900)
|
||||||
expect(revalidate).toHaveBeenCalledTimes(1)
|
expect(revalidate).toHaveBeenCalledTimes(1)
|
||||||
|
|||||||
@@ -73,7 +73,7 @@ describe('resources store — clearAll (T-02-02 logout purge)', () => {
|
|||||||
// The old fetch resolved after clearAll's generation bump — its result
|
// The old fetch resolved after clearAll's generation bump — its result
|
||||||
// must be dropped, not written into a fresh entry or sessionStorage.
|
// must be dropped, not written into a fresh entry or sessionStorage.
|
||||||
expect(store.entries.has('k2')).toBe(false)
|
expect(store.entries.has('k2')).toBe(false)
|
||||||
const e = store.entry<string>('k2')
|
const e = store.entry<string>('k2', true)
|
||||||
expect(e.data).toBeNull()
|
expect(e.data).toBeNull()
|
||||||
expect(sessionStorage.getItem('resource:k2')).toBeNull()
|
expect(sessionStorage.getItem('resource:k2')).toBeNull()
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -56,6 +56,11 @@ export const useResourcesStore = defineStore('resources', () => {
|
|||||||
const inflight = new Map<string, Promise<void>>()
|
const inflight = new Map<string, Promise<void>>()
|
||||||
const revalidators = new Map<string, Set<() => void>>()
|
const revalidators = new Map<string, Set<() => void>>()
|
||||||
const invalidateTimers = new Map<string, ReturnType<typeof setTimeout>>()
|
const invalidateTimers = new Map<string, ReturnType<typeof setTimeout>>()
|
||||||
|
// Records each key's persist decision at creation time (WR-04) — reused to
|
||||||
|
// detect (and warn on, in dev) a later call for the same key disagreeing
|
||||||
|
// about persist, since that almost always means two call sites disagree
|
||||||
|
// about whether a cache key is safe to write to sessionStorage (T-02-01).
|
||||||
|
const entryPersist = new Map<string, boolean>()
|
||||||
// Bumped by clearAll() so an in-flight fetch from a just-ended session that
|
// Bumped by clearAll() so an in-flight fetch from a just-ended session that
|
||||||
// resolves afterward can detect it and skip writing its result — without
|
// resolves afterward can detect it and skip writing its result — without
|
||||||
// this guard the resolving promise would still call writeSnapshot() and
|
// this guard the resolving promise would still call writeSnapshot() and
|
||||||
@@ -64,8 +69,12 @@ export const useResourcesStore = defineStore('resources', () => {
|
|||||||
|
|
||||||
/** Get (or create) the reactive entry for a key, hydrating from the
|
/** Get (or create) the reactive entry for a key, hydrating from the
|
||||||
* sessionStorage snapshot on first sight so revisits after a reload paint
|
* sessionStorage snapshot on first sight so revisits after a reload paint
|
||||||
* before any RPC completes. Pass `persist: false` to skip snapshots. */
|
* before any RPC completes. `persist` is REQUIRED (no default) so a call
|
||||||
function entry<T>(key: string, persist = true): ResourceEntry<T> {
|
* site can never silently opt a cache key into sessionStorage by omission
|
||||||
|
* (WR-04/T-02-01) — it only takes effect on the very first call for a
|
||||||
|
* given key; every later call for the same key must keep passing the SAME
|
||||||
|
* decision, asserted (dev-only warning) rather than silently reused. */
|
||||||
|
function entry<T>(key: string, persist: boolean): ResourceEntry<T> {
|
||||||
let e = entries.get(key)
|
let e = entries.get(key)
|
||||||
if (!e) {
|
if (!e) {
|
||||||
const snap = persist ? readSnapshot<T>(key) : null
|
const snap = persist ? readSnapshot<T>(key) : null
|
||||||
@@ -76,6 +85,12 @@ export const useResourcesStore = defineStore('resources', () => {
|
|||||||
error: null,
|
error: null,
|
||||||
})
|
})
|
||||||
entries.set(key, e)
|
entries.set(key, e)
|
||||||
|
entryPersist.set(key, persist)
|
||||||
|
} else if (import.meta.env.DEV && entryPersist.get(key) !== persist) {
|
||||||
|
console.warn(
|
||||||
|
`[resources] entry("${key}") called with persist=${persist}, but this key was already created with persist=${entryPersist.get(key)}. `
|
||||||
|
+ 'The original decision wins for the lifetime of this entry — persist is decided once per key, never per call (T-02-01).',
|
||||||
|
)
|
||||||
}
|
}
|
||||||
return e as ResourceEntry<T>
|
return e as ResourceEntry<T>
|
||||||
}
|
}
|
||||||
@@ -149,9 +164,14 @@ export const useResourcesStore = defineStore('resources', () => {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/** Optimistically apply `update` to the cached value; returns a rollback.
|
/** Optimistically apply `update` to the cached value; returns a rollback.
|
||||||
* Pattern: rollback on RPC failure (generalized TransportPrefsCard). */
|
* Pattern: rollback on RPC failure (generalized TransportPrefsCard).
|
||||||
function optimistic<T>(key: string, update: (current: T | null) => T): () => void {
|
* `persist` is REQUIRED (no default) for the same reason as entry() —
|
||||||
const e = entry<T>(key)
|
* silently falling back to persist:true here was the exact footgun WR-04
|
||||||
|
* flagged: a caller that runs before any useCachedResource({persist:false})
|
||||||
|
* has created the entry would otherwise start writing to sessionStorage
|
||||||
|
* with no indication anything is wrong. */
|
||||||
|
function optimistic<T>(key: string, update: (current: T | null) => T, persist: boolean): () => void {
|
||||||
|
const e = entry<T>(key, persist)
|
||||||
const before = e.data
|
const before = e.data
|
||||||
const beforeState = e.loadState
|
const beforeState = e.loadState
|
||||||
e.data = update(before)
|
e.data = update(before)
|
||||||
|
|||||||
@@ -827,7 +827,10 @@ async function runBrowsePeerPool(targets: PeerNode[]): Promise<void> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
function peerBrowseEntry(onion: string) {
|
function peerBrowseEntry(onion: string) {
|
||||||
return resources.entry<PeerBrowse>(peerBrowseKey(onion))
|
// persist:true matches this key's existing resources.refresh() calls
|
||||||
|
// above, which never pass `{ persist: false }` either — WR-04 just makes
|
||||||
|
// that pre-existing decision explicit instead of relying on a default.
|
||||||
|
return resources.entry<PeerBrowse>(peerBrowseKey(onion), true)
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Transport badge data for a peer (null until its first browse resolves). */
|
/** Transport badge data for a peer (null until its first browse resolves). */
|
||||||
|
|||||||
@@ -643,7 +643,11 @@ interface PeerBrowse {
|
|||||||
}
|
}
|
||||||
const peerOnion = computed(() => props.peerId || currentPeer.value?.onion || '')
|
const peerOnion = computed(() => props.peerId || currentPeer.value?.onion || '')
|
||||||
function browseEntry() {
|
function browseEntry() {
|
||||||
return resources.entry<PeerBrowse>(`cloud.peer-browse:${peerOnion.value}`)
|
// persist:true matches this key's existing resources.refresh() call in
|
||||||
|
// loadCatalog() below, which never passes `{ persist: false }` either —
|
||||||
|
// WR-04 just makes that pre-existing decision explicit instead of relying
|
||||||
|
// on a default.
|
||||||
|
return resources.entry<PeerBrowse>(`cloud.peer-browse:${peerOnion.value}`, true)
|
||||||
}
|
}
|
||||||
const catalogItems = computed(() => browseEntry().data?.items ?? [])
|
const catalogItems = computed(() => browseEntry().data?.items ?? [])
|
||||||
const catalogError = computed(() => browseEntry().error ?? '')
|
const catalogError = computed(() => browseEntry().error ?? '')
|
||||||
|
|||||||
Reference in New Issue
Block a user