platform: rebuild ScriptureReferencePlugin as a navigation state machine

[lyonsil]

Problem

Navigation through the editor has been intermittently unreliable: verses reset to 0 (e.g. Psalms 78:9 -> 78:0 on scroll-group reattach, or on any view-option toggle), navigations land somewhere other than their target during rapid check-list movement, and scripture references get stuck or out of sync. #498 fixed one trigger, but a deep review found the causes were structural, spread across the whole plugin:

• plugin state smeared across interacting one-shot boolean refs (hasCursorMovedRef / hasSelectionChangedRef) whose protocols could be wedged, stolen, or double-consumed by event timing; and
• emissions built from sources with different freshness (the book prop paired with the stale document's chapter/verse, and vice versa), producing references that describe no real position.

Every symptom is one of six races among three async signals (the scrRef prop, document content arriving later than the intent it belongs to, and selection events trailing everything) plus the echo loop (our own reports returning as the prop).

Fix

The plugin is rebuilt around an explicit, documented state machine (see the file header, which now carries the full contract, race table R1-R6, invariants I1-I8 each backed by a named test, deliberate trade-offs, and safe/unsafe change lists):

• phase: "idle" | "navigating" - while an external scrRef change (or a document replacement / book correction) is being applied, the plugin emits nothing. The window closes on real user input (pointerdown, keydown, beforeinput) or when one of the plugin's own reports echoes back as the prop. Settles are silenced by state, not by fingerprinting selection shapes - so verse 0 stays an ordinary, addressable reference (P9 semantics), never a sentinel.
• pendingEchoes FIFO - emissions not yet seen back as the prop. A prop change matching one is our own echo: consumed through the match (React batching means only the last of several in-flight reports returns) and never moves the caret. A queue rather than a boolean/slot: two reports can be in flight before the first echo returns.
• One emission gate (report()) with exactly two shapes: position reports (book + chapter + verse from a single document snapshot) and the book-correction contract ({...scrRef, book}) for documents whose book differs from the ref.
• Book-gated cursor placement - prop-driven placement never moves the caret inside a different book's stale document (no more visible jumps in the wrong book while a cross-book navigation loads); pure-created BookNodes while a document is already loaded (cross-editor paste, undo) trigger no placement at all.

Review hardening included: malformed verse ranges ("3-2") no longer throw inside the selection listener; empty chapter numbers resolve as unaddressable instead of emitting NaN; position resolution is gated on phase.

Testing

• Characterization first: pins for the pre-existing intended behavior (mount-time book correction, genuine click reporting, echo-does-not-yank-caret, in-editor book jump, verse ranges, intro-content 1:0) were written against the OLD implementation and pass unmodified on the new one.
• 35 tests in the plugin suite, including regression pins for every race: the reattach verse-0 clobber, rapid re-target hijack, stale-book echo, superseded document arriving late, mid-navigation BookNode mutations, read-only click swallowing, idle same-book reload (view-option toggle), batched-host echo consumption, and verse-0 targets/intro clicks. Several pins were adversarially mutation-tested (break the machine, confirm the test fails with the honest signal).
• jsdom fidelity shims (Range.getBoundingClientRect; selection-preserving focus()) are documented in the test file - both model real-browser behavior jsdom lacks.
• Suite run 15+ times consecutively with zero flakes; whole-monorepo test/lint/typecheck/format green.
• Live-verified in Platform.Bible via yalc: scroll-group reattach holds 78:9; NUM 1 -> previous chapter lands and stays on LEV 27:1; rapid check-list navigation (including A->B->A and verse-0 items) lands correctly; view-option toggles no longer reset the verse; read-only surfaces report the first click after navigation; in-editor verse clicks, book jumps, and typing across verse boundaries all behave.

Notes for review

• The header comment is deliberately long: this file's history is people fixing one race and reintroducing another. It names what each rule defends and which test enforces it, and lists what is unsafe to change without re-running the live repros.
• Documented trade-offs (both pinned by tests): a late echo arriving after a newer external navigation is treated as a real navigation; a user selection equal to a still-pending echo is deduped (emitting instead would poison the echo FIFO and swallow a future real navigation).
• Follow-ups tracked, not in this PR: moving the malformed-range guard into shared-react's $findVerseInNode so all callers are protected; partial-document (chapter N only) intro-click chapter resolution.

AI-assisted (Claude Code)

🤖 Generated with Claude Code

https://claude.ai/code/session_01YGsAqgwChpCeCc8KW4o3HF

---

This change is 

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[katherinejensen00]

This has been a real problem. Thank you for fixing it! Just a couple of nitpicky documentation improvements to consider.

@katherinejensen00 reviewed 2 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on lyonsil).

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 272 at r1 (raw file):

  if (batch.hasCreated && batch.hasDestroyed) {
    // REPLACEMENT (LoadStatePlugin's setEditorState swap, in-editor book jump): the swap's own

Thank you for including such clear yet succinct documentation. The comments make it a lot easier to follow what is happening in this file!

---

packages/platform/src/editor/ScriptureReferencePlugin.test.tsx line 236 at r1 (raw file):

        }
      });
      // Clear hasCursorMovedRef (set by initial "move cursor to verse start" effect) so our dispatch runs BCV logic.

NIT Documentation Cleanup

Summary: This comment explains the setup in terms of hasCursorMovedRef, a ref this PR deletes.

hasCursorMovedRef existed only in the old implementation. The line "Clear hasCursorMovedRef (set by initial 'move cursor to verse start' effect) so our dispatch runs BCV logic" describes a mechanism that no longer exists — and the characterization test 30 lines below (line 263) already states the correct new-code reason: "a no-op report (position == scrRef) in the new code." Update line 236 to match, so the two comments stop contradicting each other.

---

packages/platform/src/editor/ScriptureReferencePlugin.test.tsx line 527 at r1 (raw file):

        const selection = $getSelection();
        const startNode = getSelectionStartNodeForTest(selection);
        expect(startNode?.getTextContent()).not.toBe("second verse text ");

NIT

Summary: expect(startNode?.getTextContent()).not.toBe("second verse text ") is nearly always true and pins little.

In "applies a navigation to verse 0," the caret started on secondVerseTextNode and the assertion only checks it left that text — almost any landing (including a wrong one) passes. Prefer asserting the caret is in the chapter-top region you actually expect. Note: the visually-identical assertion at line 577 (placement-gate test) is fine as-is — the swapped-in EXO fixture also contains "second verse text ", so there it precisely targets the known wrong-placement node, as the line-576 comment explains. Only line 527 is weak.

[katherinejensen00]

Looks great. Thanks, Matt!

@katherinejensen00 reviewed 2 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved.

[lyonsil]

@lyonsil made 3 comments.
Reviewable status: complete! all files reviewed, all discussions resolved.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 272 at r1 (raw file):

Previously, katherinejensen00 wrote…

Thank you for including such clear yet succinct documentation. The comments make it a lot easier to follow what is happening in this file!

I'm glad you find them helpful! I thought being overly verbose for this file in particular might be helpful in preventing future regressions.

---

packages/platform/src/editor/ScriptureReferencePlugin.test.tsx line 236 at r1 (raw file):

Previously, katherinejensen00 wrote…

NIT Documentation Cleanup

Summary: This comment explains the setup in terms of hasCursorMovedRef, a ref this PR deletes.

hasCursorMovedRef existed only in the old implementation. The line "Clear hasCursorMovedRef (set by initial 'move cursor to verse start' effect) so our dispatch runs BCV logic" describes a mechanism that no longer exists — and the characterization test 30 lines below (line 263) already states the correct new-code reason: "a no-op report (position == scrRef) in the new code." Update line 236 to match, so the two comments stop contradicting each other.

Good point - fixed this comment.

---

packages/platform/src/editor/ScriptureReferencePlugin.test.tsx line 527 at r1 (raw file):

Previously, katherinejensen00 wrote…

NIT

Summary: expect(startNode?.getTextContent()).not.toBe("second verse text ") is nearly always true and pins little.

In "applies a navigation to verse 0," the caret started on secondVerseTextNode and the assertion only checks it left that text — almost any landing (including a wrong one) passes. Prefer asserting the caret is in the chapter-top region you actually expect. Note: the visually-identical assertion at line 577 (placement-gate test) is fine as-is — the swapped-in EXO fixture also contains "second verse text ", so there it precisely targets the known wrong-placement node, as the line-576 comment explains. Only line 527 is weak.

Good point. Reworked the assertion.