[tjcouch-sil]

Summary

Fixes the intermittent CI failure of ScriptureReferencePlugin.test.tsx › stops at the next verse marker for an empty verse with an editable marker (seen on macOS in #15 and #19: expected Point{ key: '282', offset: 0 } to deeply equal { key: '280', offset: 5 }).

Cause. Navigating to an empty verse (one followed only by the next verse marker) placed the caret at the boundary after its marker with $placeCaretAtBoundary, which takes the text node that follows as the caret's host. In Standard and Unformatted views the next verse's marker is itself text (\v 6 ), so the caret was put at offset 0 of the NEXT verse's marker. Lexical sometimes normalizes that point back to the end of \v 5 before the selection is next read and sometimes doesn't, which is the race the test lost (1 run in 12 locally on #19's branch, never on main in 54 runs, all measured before main added flushQueuedEvents() calls to the test). Those calls wait for the pending selectionchange and so hide the race in this test; the placement change removes the race itself, so the caret position no longer depends on when Lexical normalizes.

Fix. For an empty verse whose marker is editable text, the caret goes to the end of the verse's own marker — the rule $placeCaretAtVerseContentStart already applies when a verse opens with a note, milestone or char span.

Behavior change

• Standard / Unformatted views only (editable verse markers). After navigating to an empty verse, the caret is always at the end of that verse's marker instead of sometimes at the start of the next verse's marker. Both are the same screen location.
• Formatted view and every other immutable-marker view: no change. The immutable branch still leaves the element point that EmptyVerseCaretGuardPlugin repairs with a caret host.
• The reported verse does not change in either style. A caret at the start of \v 6 (the old race outcome) also reports verse 5, so the race only ever affected where the caret sat, not the verse sent to the host.

Testing

• New: reports the empty verse for a caret the user puts in it — for an editable marker (caret at the end of \v 5 ) and an immutable marker (the element point after it), the plugin reports verse 5 and never 6. These pin the empty-verse verse-reporting behavior Formatted view depends on; they pass before and after the fix, since reporting was unaffected.
• New: rests at the end of the editable verse marker for an empty verse that ends its paragraph. It documents intended behavior but cannot fail without the placement code: with nothing after the marker, the old element point and the new marker-end point converge once Lexical commits the update.
• Measured before rebasing onto main's flushQueuedEvents() mitigation, the flaky test is deterministic with the fix: 30/30 in a loop and 24/24 under six-way parallel load. Full packages/platform suite passes; typecheck and lint clean; rebuild-committed-output then verify-committed-dist reports the committed build output current.

Review follow-up

Addressed in the commits after the first: selectEnd() for the marker-end caret, TSDoc on the editable empty-verse shapes and on the caret-boundary helpers, test rename, and rebuilt committed output (built on Linux).

AI Involvement

AI-assisted. Claude traced the CI failure to the caret placement race, wrote the fix and tests, and ran the checks.

🤖 Generated with Claude Code

---

This change is 

[lyonsil]

I reviewed the empty-verse caret placement change and its tests, and have 6 findings, all at low severity. Each comment shows the severity it settled at and a status: checked and confirmed means the finding was verified against the code, and needs a human call means it could not be settled either way and is yours to judge.

Findings that could not be attached to a line in this diff

#1 (low · checked and confirmed) libs/shared/src/nodes/usj/caretBoundaries.utils.ts:69 - Calling $placeCaretAtBoundary at the boundary after an editable empty verse still puts the caret at offset 0 of the next verse's marker, a position that reports the next verse.

What happens: $caretHostAtBoundary (caretBoundaries.utils.ts:69-72) returns whatever text node follows the boundary. Between \v 5 and \v 6 in Standard view that is verse 6's editable VerseNode, so $placeCaretAtBoundary (:81-85) selects its offset 0. This PR sidesteps that only inside $placeCaretAtVerseContentStart (ScriptureReferencePlugin.tsx:596-600). EmptyVerseCaretGuardPlugin.tsx:33 asks the same helper, gets verse 6's marker back as a host, and leaves the caret alone.

Why it matters: maintenance only: no other current caller parks a caret after an editable empty verse, but the next one that does so through $placeCaretAtBoundary reintroduces the wrong-verse report this PR fixes.

Fix: state in the TSDoc of $caretHostAtBoundary and $placeCaretAtBoundary that a caller placing a caret directly after a verse marker must handle an editable next verse marker itself, since the module's forward-looking convention is deliberate. While there, add each function's calling context per CLAUDE.md ("Mutating: call inside editor.update()" on $placeCaretAtBoundary, the safe read form on $caretHostAtBoundary). The platform bundle's source map embeds this file's source, so run pnpm rebuild-committed-output and commit dist/ in the same PR. The alternative - making $caretHostAtBoundary reject a following SomeVerseNode - changes EmptyVerseCaretGuardPlugin's behavior and needs a unit test in libs/shared.

How this was checked: $caretHostAtBoundary (caretBoundaries.utils.ts:69-72) returns any following TextNode, and an editable VerseNode is one, so $placeCaretAtBoundary (:81-85) selects offset 0 of the next verse marker. The PR sidesteps this only in $placeCaretAtVerseContentStart (ScriptureReferencePlugin.tsx:596-600), whose own comment (:585-590) says the next verse's marker would otherwise be borrowed. I checked every other caller (transientCaretHost.ts:118-132, ParaMarkerPrefixCursorGuardPlugin.tsx:108/141, ArrowNavigationPlugin.tsx:348, NoteShellCaretGuardPlugin.tsx:133/163, markerEditDeletion.utils.ts:75/458, ScriptureReferencePlugin.tsx:547/620): none parks a caret directly after an editable empty verse marker (the guard only repairs the decorator case, where no text host exists). So the hazard is real but has no reachable caller today; the forward-looking convention is deliberate and documented, and the file is unchanged by this PR. Maintenance-only, already low. I did not trace the reported-verse resolution for a caret at offset 0 of verse 6's marker; the PR's own comment treats it as the next verse.

Not attached to a line: not on a changed line; cannot be posted inline

#3 (low · checked and confirmed) packages/platform/src/editor/ScriptureReferencePlugin.test.tsx:385 - The title "stops at the next verse marker for an empty verse with an editable marker" names a different position than the test asserts, the end of verse 5's own marker.

What happens: the title at ScriptureReferencePlugin.test.tsx:385 was kept as is. The updated comment at :396-398 says the caret goes to the end of this verse's own marker, "not to the start of verse 6's marker", and :399 asserts $expectSelectionToBe(emptyVerseMarker, emptyVerseMarker.getTextContentSize()).

Why it matters: test-only, with no behavior effect. This test has failed intermittently in CI, and whoever triages its next failure reads a title that names the position the comment now rules out.

Fix: rename the test at :385 to match its sibling at :367: "rests at the end of the editable verse marker for an empty verse".

How this was checked: The title at ScriptureReferencePlugin.test.tsx:385 ("stops at the next verse marker for an empty verse with an editable marker") is unchanged by this PR, while the comment at :396-398 was rewritten to say the caret goes to the end of verse 5's own marker and not to the start of verse 6's marker, and :399 asserts $expectSelectionToBe(emptyVerseMarker, emptyVerseMarker.getTextContentSize()). "Stops at the next verse marker" can be read loosely as "does not run past it", which the test does guard, but it names the neighbour rather than the asserted position, and the sibling at :367 already uses the "rests at the end of the editable verse marker" wording. Test-only, cosmetic; the intermittent-CI-failure motivation was not checked.

Not attached to a line: not on a changed line; cannot be posted inline

(AI-assisted, with my guidance)

[tjcouch-sil]

Thanks — all six addressed or answered (16fb69e):

• Adopt this repository into the paranext organization #1 Done as a doc-only change: $caretHostAtBoundary / $placeCaretAtBoundary TSDoc now state the editable-next-marker hazard and calling context.
• Remove the scribe package #2 Not changed. I wrote the suggested test (navigate with setScrRef, dispatch SELECTION_CHANGE_COMMAND, assert never verse 6) and it still passed with the $isTextNode(verse) arm disabled: a caret at offset 0 of \v 6 also reports verse 5 (see "The reported verse does not change" in the description), so reporting can't distinguish the two placements. The placement test is the guard, and it is intermittent by nature of the race.
• Flaky interaction tests: ScriptureReferencePlugin and marker-palette each failed once in CI #3 Renamed to "rests at the end of the editable verse marker for an empty verse".
• Consider committing built dist/ like platform-bible-react, so consumers stop building at install time #4 TSDoc now covers both shapes (next marker follows / verse ends its paragraph).
• Separate the published artifact directory from the toolchain's scratch space (both are currently dist/) #5 Added a paragraph-final placement test. It passes with and without the fix (the element point normalizes to the marker's end on commit), so it carries a comment saying it documents intended behavior rather than guards a regression.
• Consume the editor from GitHub release tarballs instead of staged file: copies #6 Both copies now use verse.selectEnd().

dist/ rebuilt on Linux via rebuild-committed-output.

[lyonsil]

#1 Done as a doc-only change: $caretHostAtBoundary / $placeCaretAtBoundary TSDoc now state the editable-next-marker hazard and calling context.

Thanks. One correction, and it's my mistake: the $caretHostAtBoundary TSDoc now ends with "a caret at its offset 0 reports the NEXT verse" (libs/shared/src/nodes/usj/caretBoundaries.utils.ts:68). That came from my #1 finding, and it isn't true. As you found for #2, a caret at offset 0 of \v 6 reports verse 5, because $shouldShowPreviousVerseForBcv treats a caret before the verse number as the previous verse. The actual hazard is only that the caret sits in the next verse's marker node, at the same screen location. Could you drop that clause or reword it along those lines? The $placeCaretAtBoundary note is fine as written.

(AI-assisted, with my guidance)

[lyonsil]

No other issues - your choice on fixing the doc issue mentioned from the earlier review's #1.

[tjcouch-sil]

Thanks, you're right: the clause was wrong. The $caretHostAtBoundary TSDoc now says the host puts the caret in the next verse's marker node, at the same screen location as the end of this verse's marker, with no claim about which verse it reports.

Also rebased onto the latest main (one test conflict, where I kept main's flushQueuedEvents calls) and regenerated dist/ on Linux via rebuild-committed-output.