Repository navigation
fix: correct the onLoadDocument callback type and skip the document self-apply - #1155
Conversation
…elf-apply The callback that consumes onLoadDocument return values annotates its parameter as Uint8ArrayConstructor rather than Uint8Array. It only typechecks because the instanceof check narrows it at runtime. 667e145, which added that branch, describes the hook as accepting "a yjs update (Uint8Array or Buffer) or a Y.Doc", so the annotation contradicts what the branch is for. The same callback also re-applies a document to itself. When a hook returns the document it was given, loadDocument runs applyUpdate(document, encodeStateAsUpdate(document)). Encoding a document's own state and applying it back can never add content, but it costs a full encode and apply on the first connection to every document. The bundled Database extension used to return the document and had that return removed in c4c6094 (ueberdosis#849, closing ueberdosis#848). The core was never guarded, so any hook that still returns its document keeps paying the round trip. Skip it when the instance is the same; returning a different Y.Doc or a Uint8Array is unchanged, and the new test for that path covers the merge branch, which no existing test exercised.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary
WalkthroughChangesThe document loading callback now uses the correct Document loading
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Document loading now avoids redundant self-application while preserving application of separately returned documents, with coverage for both paths. No merge-blocking risk remains. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
The callback that handles
onLoadDocumentreturn values has two problems.The parameter is annotated
Uint8ArrayConstructorinstead ofUint8Array. It onlytypechecks because the
instanceofcheck narrows it at runtime. 667e145 added thatbranch and describes the hook as accepting "a yjs update (Uint8Array or Buffer) or a
Y.Doc", so the annotation says the opposite of what the branch is for.
The same callback also applies a document to itself. When a hook returns the document
it was given,
loadDocumentrunsapplyUpdate(document, encodeStateAsUpdate(document)).Encoding a document's own state and applying it back can never add content, but it costs
a full encode and apply on the first connection to every document.
The bundled Database extension used to return the document, and c4c6094 removed that
return for this reason (#849, closing #848). The core was never guarded, so any hook that
still returns its document keeps paying the round trip.
This skips the apply when the returned instance is the same one. Returning a different
Y.Docor aUint8Arrayis unchanged.Two tests:
does not apply the document to itself when onLoadDocument returns it— recordstransaction origins after the hook returns and asserts none is null. The self-apply
calls
applyUpdatewith no origin, while the client sync carries aConnectionTransactionOrigin, so a null origin can only be the self-apply. Fails onmain with 1.
applies a document returned by the onLoadDocument callback— no existing test returneda different
Y.Doc, so the merge branch had no coverage once the skip is in place.This pins it.