platform: stop stale book echo when the host navigates across books#498
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders Open Preview |
lyonsil
left a comment
There was a problem hiding this comment.
lgtm - This appears to fix the bug flagged by TJ without causing new bugs
lyonsil
left a comment
There was a problem hiding this comment.
@lyonsil reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved.
katherinejensen00
left a comment
There was a problem hiding this comment.
This bug fix looks really helpful. Thanks for working on this, TJ! Just one small optional comment clarity change. I will go ahead and approve this.
@katherinejensen00 reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on tjcouch-sil).
packages/platform/src/editor/ScriptureReferencePlugin.tsx line 98 at r2 (raw file):
NIT Claude Finding
Small accuracy fix: this listener actually fires on every non-
destroyedBookNode mutation (including in-editor"updated"ones), not only mount + document swap. What keeps the stray firings harmless is thecode !== current.bookguard, not a limited firing set — worth wording it that way so nobody later relies on a firing-frequency invariant that doesn't hold.
Suggested rewrite of the book-sync comment (replacing lines 90–99), corrected and tightened:
// 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.
katherinejensen00
left a comment
There was a problem hiding this comment.
Thanks for improving that comment, TJ! Looks good.
@katherinejensen00 reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved.
7a3ef99 to
dbc76ed
Compare
…ook 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 <noreply@anthropic.com>
…mment The comment said "NUM -> LEV" but the test navigates GEN -> EXO. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Session-URL: https://claude.ai/code/session_2f36899d-c94a-41c3-aea1-07a90871f586
…ng set, makes stray mutations harmless Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dbc76ed to
a32a481
Compare
Problem
When a host application (Platform.Bible) navigates the scripture reference across a book boundary — e.g. NUM 1 → "previous chapter" → LEV 27:1 — the editor echoes a corrupted reference back through
onScrRefChange: the OLD book with the NEW chapter/verse (NUM 27:1). The host's navigation is overwritten and the user lands on chapter 27 of the wrong book.What broke, and where
#457 added the BookNode book-sync listener (
skipInitialization: false) toScriptureReferencePlugin, so the ref'sbookfollows the loaded document — a legitimate and still-desired behavior. That PR also addedscrRefRef/onScrRefChangeRefrefs specifically so the listener could read current values without re-registering (its comment says it should "only run when BookNodes are created/updated").However, the listener was registered inside the pre-existing
useEffectkeyed on[editor, chapterNum, verseNum]. Every chapter/verse-only navigation therefore tears down and re-registers the listener, and because it usesskipInitialization: false, each re-registration replays initialization against the still-mounted old document (the new book's content loads async in the host). During that windowscrRefRef.currentalready holds the new ref (LEV 27:1) while the stale document's BookNode still readsNUM→ the listener firesonScrRefChange({ ...current, book: 'NUM' })→ NUM 27:1.Fix — preserving #457's intent
Split the two BookNode listeners into separate effects:
[editor]deps), reading current values through the refs platform: fix BCV display so verse updates only when cursor is after the verse number #457 already added for exactly this purpose. Its mount-time initialization still performs the legitimate book correction platform: fix BCV display so verse updates only when cursor is after the verse number #457 introduced, and it still reacts to genuine BookNode created/updated mutations — it just no longer replays against a stale document on every chapter/verse navigation.This completes the design #457's own refs and comment describe, rather than changing what the listener is for.
Testing
onScrRefChangemust NOT fire with the old book. Fails on the pre-fix code with exactly the production echo shape; passes with the fix.AI-assisted (Claude Code)
🤖 Generated with Claude Code
This change is