From e1cd826c0ce7a186888f9d786fc2b0f2be47b839 Mon Sep 17 00:00:00 2001 From: TJ Couch Date: Tue, 7 Jul 2026 14:30:34 -0500 Subject: [PATCH 1/3] fix(platform): stop book-sync listener echoing the stale document's book on chapter/verse-only re-registration ScriptureReferencePlugin's BookNode "book sync" mutation listener (skipInitialization: false) and its "move cursor to new book" sibling (skipInitialization: true) shared a single useEffect keyed on [editor, chapterNum, verseNum]. Only the cursor-move listener actually needs chapterNum/verseNum in its closure; the book-sync listener already reads the latest scrRef via scrRefRef, matching its original intent ("avoid re-registering on every scrRef change", per fc39cc1's commit message) - but sharing the effect meant it still re-registered whenever chapterNum or verseNum changed. Re-registering triggers a fresh skipInitialization: false replay against whatever document is currently mounted. When a host navigates across books, the new scrRef (new book + chapter/verse) can arrive before the new book's document finishes loading async. The replay then compares the OLD (still-mounted) document's BookNode against the NEW scrRef and fires onScrRefChange with the OLD book paired with the NEW chapter/verse, corrupting the navigation (e.g. requesting LEV 27:1 gets echoed back as NUM 27:1). Fix: split the two BookNode mutation listeners into separate effects. The cursor-move listener keeps [editor, chapterNum, verseNum] (safe to re-register - skipInitialization: true never replays). The book-sync listener now depends on [editor] only, so it fires solely on genuine BookNode mutations (initial mount + real document swaps), never on a chapter/verse-only navigation or an in-flight cross-book navigation. Added a regression test pinning that a book-only scrRef change, while the old document is still loaded, does not echo the stale book back to the host. All existing book-sync/cursor/selection tests continue to pass unmodified. Co-authored-by: Claude Fable 5 --- .../editor/ScriptureReferencePlugin.test.tsx | 17 ++++ .../src/editor/ScriptureReferencePlugin.tsx | 98 +++++++++++-------- 2 files changed, 73 insertions(+), 42 deletions(-) diff --git a/packages/platform/src/editor/ScriptureReferencePlugin.test.tsx b/packages/platform/src/editor/ScriptureReferencePlugin.test.tsx index 79a68669..207cd5f8 100644 --- a/packages/platform/src/editor/ScriptureReferencePlugin.test.tsx +++ b/packages/platform/src/editor/ScriptureReferencePlugin.test.tsx @@ -83,6 +83,23 @@ describe("ScriptureReferencePlugin", () => { expect(mockOnScrRefChange).not.toHaveBeenCalled(); }); + + it("should not echo the stale document's book when scrRef navigates to a different book while the old document is still loaded", async () => { + // Content stays on GEN (the old document hasn't been swapped in yet - simulates the + // window between a host navigating across books and the new book's USJ finishing its + // async load). scrRef.book already matches the content at mount, so no sync fires yet. + const { setScrRef } = await testEnvironment(scrRef, mockOnScrRefChange); + + // Host navigates across books (e.g. NUM -> LEV): scrRef now targets a different book (and + // a verse that still exists in the stale document, isolating this test to the book-sync + // listener rather than the separate cursor-placement/BCV path), but the editor still + // contains the OLD book's BookNode. + await setScrRef({ book: "EXO", chapterNum: 1, verseNum: 2 }); + + // The plugin must not echo the stale document's book back to the host combined with the + // new chapter/verse - that would corrupt the navigation (e.g. EXO 1:2 -> GEN 1:2). + expect(mockOnScrRefChange).not.toHaveBeenCalled(); + }); }); describe("Selection Change", () => { diff --git a/packages/platform/src/editor/ScriptureReferencePlugin.tsx b/packages/platform/src/editor/ScriptureReferencePlugin.tsx index 233b8728..645a5019 100644 --- a/packages/platform/src/editor/ScriptureReferencePlugin.tsx +++ b/packages/platform/src/editor/ScriptureReferencePlugin.tsx @@ -60,55 +60,69 @@ export default function ScriptureReferencePlugin({ onScrRefChangeRef.current = onScrRefChange; }, [scrRef, onScrRefChange]); - // Book node: move cursor when a new BookNode appears after load; sync scrRef.book when the - // book code changes (initial tree + updates). Uses a second listener with skipInitialization: - // false so existing BookNodes from initial editor state are included (updateListener ran on - // every keystroke; this only runs when BookNodes are created/updated). + // Book node created: move cursor to the verse start once a new BookNode appears after the + // initial load (e.g. a different book's document has finished mounting). Re-registers on every + // chapterNum/verseNum change so the closure captures the latest target position; safe to + // re-register because skipInitialization: true means re-registering never replays existing + // BookNodes, only genuinely new "created" mutations going forward. useEffect( () => - mergeRegister( - editor.registerMutationListener( - BookNode, - (nodeMutations) => { - editor.update( - () => { - for (const [nodeKey, mutation] of nodeMutations) { - const bookNode = $getNodeByKey(nodeKey); - if (bookNode && $isBookNode(bookNode) && mutation === "created") { - $moveCursorToVerseStart(chapterNum, verseNum, hasCursorMovedRef); - } + editor.registerMutationListener( + BookNode, + (nodeMutations) => { + editor.update( + () => { + for (const [nodeKey, mutation] of nodeMutations) { + const bookNode = $getNodeByKey(nodeKey); + if (bookNode && $isBookNode(bookNode) && mutation === "created") { + $moveCursorToVerseStart(chapterNum, verseNum, hasCursorMovedRef); } - }, - { tag: CURSOR_CHANGE_TAG }, - ); - }, - { skipInitialization: true }, - ), - editor.registerMutationListener( - BookNode, - (nodeMutations) => { - editor.update( - () => { - for (const [nodeKey, mutation] of nodeMutations) { - if (mutation === "destroyed") continue; - const bookNode = $getNodeByKey(nodeKey); - if (!bookNode || !$isBookNode(bookNode)) continue; - const code = bookNode.getCode(); - const current = scrRefRef.current; - if (code && code !== current.book) { - onScrRefChangeRef.current({ ...current, book: code }); - } - } - }, - { tag: CURSOR_CHANGE_TAG }, - ); - }, - { skipInitialization: false }, - ), + } + }, + { tag: CURSOR_CHANGE_TAG }, + ); + }, + { skipInitialization: true }, ), [editor, chapterNum, verseNum], ); + // Book node sync: correct scrRef.book to match the *currently loaded document's* BookNode + // (initial tree + later updates/replacements). Registered once per editor instance ([editor] + // only) rather than re-registering on every chapterNum/verseNum change - scrRefRef/ + // onScrRefChangeRef already track the latest values, so re-registration isn't needed for + // freshness, and re-registering would replay the skipInitialization: false initialization pass + // against whatever document is currently mounted. On a chapter/verse-only navigation (or a + // cross-book navigation still awaiting its new document to load) that document is stale, so a + // replay would incorrectly echo the OLD book paired with the NEW chapter/verse back to the + // host. Registering once means this only fires for genuine BookNode mutations: the initial + // mount (skipInitialization: false's replay) and any later real document swap. + useEffect( + () => + editor.registerMutationListener( + BookNode, + (nodeMutations) => { + editor.update( + () => { + for (const [nodeKey, mutation] of nodeMutations) { + if (mutation === "destroyed") continue; + const bookNode = $getNodeByKey(nodeKey); + if (!bookNode || !$isBookNode(bookNode)) continue; + const code = bookNode.getCode(); + const current = scrRefRef.current; + if (code && code !== current.book) { + onScrRefChangeRef.current({ ...current, book: code }); + } + } + }, + { tag: CURSOR_CHANGE_TAG }, + ); + }, + { skipInitialization: false }, + ), + [editor], + ); + // Scripture Reference changed useEffect(() => { if (hasSelectionChangedRef.current) hasSelectionChangedRef.current = false; From cd8ca70b0287bd8385db76cc9506210de5d1f251 Mon Sep 17 00:00:00 2001 From: TJ Couch Date: Wed, 8 Jul 2026 10:32:44 -0500 Subject: [PATCH 2/3] test(platform): correct cross-book example in stale-book-echo test comment The comment said "NUM -> LEV" but the test navigates GEN -> EXO. Co-authored-by: Claude Fable 5 Session-URL: https://claude.ai/code/session_2f36899d-c94a-41c3-aea1-07a90871f586 --- packages/platform/src/editor/ScriptureReferencePlugin.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/platform/src/editor/ScriptureReferencePlugin.test.tsx b/packages/platform/src/editor/ScriptureReferencePlugin.test.tsx index 207cd5f8..cbbcc6c1 100644 --- a/packages/platform/src/editor/ScriptureReferencePlugin.test.tsx +++ b/packages/platform/src/editor/ScriptureReferencePlugin.test.tsx @@ -90,7 +90,7 @@ describe("ScriptureReferencePlugin", () => { // async load). scrRef.book already matches the content at mount, so no sync fires yet. const { setScrRef } = await testEnvironment(scrRef, mockOnScrRefChange); - // Host navigates across books (e.g. NUM -> LEV): scrRef now targets a different book (and + // Host navigates across books (here GEN -> EXO): scrRef now targets a different book (and // a verse that still exists in the stale document, isolating this test to the book-sync // listener rather than the separate cursor-placement/BCV path), but the editor still // contains the OLD book's BookNode. From a32a481ea70274396eb6c8ac8ddd3b8cd6265a9e Mon Sep 17 00:00:00 2001 From: TJ Couch Date: Wed, 8 Jul 2026 15:45:26 -0500 Subject: [PATCH 3/3] docs(platform): reword book-sync comment per review - guard, not firing set, makes stray mutations harmless Co-Authored-By: Claude Fable 5 --- .../src/editor/ScriptureReferencePlugin.tsx | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/packages/platform/src/editor/ScriptureReferencePlugin.tsx b/packages/platform/src/editor/ScriptureReferencePlugin.tsx index 645a5019..4488f4b9 100644 --- a/packages/platform/src/editor/ScriptureReferencePlugin.tsx +++ b/packages/platform/src/editor/ScriptureReferencePlugin.tsx @@ -87,16 +87,13 @@ export default function ScriptureReferencePlugin({ [editor, chapterNum, verseNum], ); - // Book node sync: correct scrRef.book to match the *currently loaded document's* BookNode - // (initial tree + later updates/replacements). Registered once per editor instance ([editor] - // only) rather than re-registering on every chapterNum/verseNum change - scrRefRef/ - // onScrRefChangeRef already track the latest values, so re-registration isn't needed for - // freshness, and re-registering would replay the skipInitialization: false initialization pass - // against whatever document is currently mounted. On a chapter/verse-only navigation (or a - // cross-book navigation still awaiting its new document to load) that document is stale, so a - // replay would incorrectly echo the OLD book paired with the NEW chapter/verse back to the - // host. Registering once means this only fires for genuine BookNode mutations: the initial - // mount (skipInitialization: false's replay) and any later real document swap. + // Book node sync: correct scrRef.book to match the currently loaded document's BookNode. + // Fires on every non-destroyed BookNode mutation (initial-mount replay + later swaps); the + // `code !== current.book` guard below is what makes stray firings harmless. Registered once + // per editor ([editor] only), NOT re-registered on chapterNum/verseNum change: the refs + // already keep values fresh, and re-registering would replay skipInitialization: false against + // the still-mounted (stale) document on every navigation, echoing the OLD book with the NEW + // chapter/verse back to the host. useEffect( () => editor.registerMutationListener(