fix(useDoc): don't self-evict the doc ref on first uncached load#800
Merged
Conversation
|
Concerns — fix stops the crash, but leaves a side-effect worth a look.
|
…cStore The first uncached load of any doc via useDoc crashed and rendered blank: Failed to set doc in IDB: TypeError: Cannot read properties of undefined (reading 'value') (useDoc.ts → storeRef.value) Two distinct defects, both rooted in getDoc returning the in-memory ref synchronously while its caller (useDoc's `doc` computed) dereferences it immediately: 1. setDoc set lastFetched AFTER assigning docRef.value. Assigning the ref synchronously re-runs the computed, which re-enters getDoc; the entry still looked stale (lastFetched unset), so getDoc kicked off a reload that, via cleanup(), deleted the very ref it was about to return — getDoc then returned undefined and the computed crashed on `.value`. This fired on every first uncached load. Fix: mark lastFetched fresh before assigning the ref, so the synchronous re-run sees a fresh entry and never reloads — no crash, and no eviction of the just-written IDB copy (which the reload's idbStore.delete would otherwise wipe). 2. Independently, the stale-reload branch in loadDoc called cleanup(), whose first synchronous line deletes the map entry. Any doc accessed after it goes stale (a long-lived useDoc re-evaluated past the cache timeout) would make getDoc return undefined and crash. Fix: the stale path now evicts the IDB copy and clears the timestamp but keeps the in-memory ref. cleanup() stays for the explicit invalidateDoc/removeDoc paths, which intend full removal. Adds docStore.test.ts covering both: a synchronously-tracked reader through a first uncached load, and a stale access. Both fail on the prior code and pass here. Claude-Session: https://claude.ai/code/session_01CG9r6WdYSLUhcTKbNXcoki
29c1e14 to
f5be824
Compare
Contributor
Author
|
Updated from review feedback:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The first uncached load of any document via
useDoccrashes and renders blank:Root cause — two defects, both from
getDocreturning the in-memory ref synchronouslygetDocreturns the ref synchronously and its caller (useDoc'sdoccomputed) dereferences it immediately. Two separate paths violate that contract:1.
setDocordering → first-uncached-load crash + IDB churnsetDocsetlastFetchedafterdocRef.value = doc. Assigning the ref synchronously re-runs thedoccomputed, which re-entersgetDoc; the entry still looked stale (lastFetchedunset), sogetDockicked off a reload that — viacleanup()— deleted the very ref it was about to return.getDocthen returnedundefinedand the computed crashed on.value.loadDoc(firstLoad=true)never setslastFetchedwhen IDB is empty, so this fired on every first uncached load.2.
loadDocstale path → stale-access crash (independent of #1)The stale-reload branch called
cleanup(), whose first synchronous line isthis.docs.delete(key). So any doc accessed after it goes stale (a long-liveduseDocre-evaluated past the cache timeout) makesgetDoc'sreturn this.docs.get(key)!yieldundefinedand crash — the first-uncached-load was just the most common trigger.Fix
setDocmarkslastFetchedbefore assigning the ref, so the synchronous re-run sees a fresh entry and never reloads — no crash, and the just-written IDB copy isn't evicted by a needless reload.loadDocstale path evicts the IDB copy and clears the timestamp but keeps the in-memory ref.cleanup()is untouched and still used byinvalidateDoc/removeDoc, which intend full removal.Tests
Adds
src/data-fetching/docStore.test.ts(none existed):watchSyncEffect) throughsetDoc; asserts no crash and that the IDB copy survives.getDocnever returnsundefined.Both fail on the prior code and pass with this change. Full
src/data-fetching/suite (29 tests) passes.Reproduced and verified in a downstream app (Gameplan): every first discussion open rendered blank with the error above; with this change the doc loads on first visit, the console is clean, and no unhandled rejection fires.
https://claude.ai/code/session_01CG9r6WdYSLUhcTKbNXcoki
Coverage: 57.93% (±0.00% vs
main)