[captaincrazybro]

Code Review Summary

Branch: pt-4985-verse-insert-caret

Base: origin/main

Date: 2026-11-03

Review model: Claude Opus 5.5

Files changed: 8

Overview

This fixes PT-4985. In Simple mode, typing \v and Enter inserts the next verse number, but the
caret did not reliably land in the new verse.

• Nothing follows the new marker in its paragraph (for example at the end of a chapter): the
  caret was left before the new number, so typing extended the previous verse. This is the
  ticket's case.
• A footnote follows it (a verse inserted right after a footnoted word): the caret went
  inside the footnote. In Standard view the typed text was lost from the saved USJ. Ira found
  this case in review.

Both came from the plain-insert branch of getUsjMarkerAction, which placed the caret with
nextSibling.selectStart(), or with nodeToInsert.selectStart() when there was no next sibling.
For a verse marker, those calls resolve to the previous verse's text or to the first leaf inside a
following note.

The fix sends every inserted verse marker ($isSomeVerseNode) through
$placeCaretAtVerseContentStart, the rule verse navigation already uses for "the start of this
verse's content". That rule covers four cases:

• Plain text follows: the caret goes to offset 0 of that text.
• Editable marker, nothing that can hold the caret follows: the caret goes to the end of the
  marker's own \v 2 text.
• Immutable marker, a character span follows: the caret goes to the start of the span's text.
• Otherwise, including an empty verse: the caret goes to the boundary after the marker, where
  EmptyVerseCaretGuardPlugin hosts it.

It never enters a note. The helper (with its private $firstCaretHost) moved unchanged from
ScriptureReferencePlugin.tsx into the new packages/platform/src/editor/verseCaret.utils.ts, so
navigation and insertion share one rule. Other inserted nodes, chapter markers included, keep their
current placement. The "duplicate verse number → select the inserted marker" rule
(highlightInserted) is deliberately unchanged.

The author confirmed both cases in Platform.Bible against a locally staged build. The committed
packages/platform/dist/ was rebuilt to match the source. packages/platform/CHANGELOG.md has an
entry under [Unreleased] → ### Fixed.

API Changes

None. The package's public exports, signatures and type declarations are unchanged:

• $placeCaretAtVerseContentStart is now exported from the new internal module verseCaret.utils.ts,
  but not from the package entry point.
• The exported getUsjMarkerAction() changes only in where the caret lands after it inserts a
  verse marker.

Findings

Combined from /review-paratext, a /code-review (high) pass, and Ira's review on this PR.

Critical — Must address before merge

None.

Important — Should address before merge

• Inserting a verse right before a footnote put the caret inside the note; in Standard view
  the typed text was lost from the saved USJ (Ira Adopt this repository into the paranext organization #1, medium) (fixed during review: any
  inserted verse now takes the caret through $placeCaretAtVerseContentStart, ahead of the
  nextNode.selectStart() fallback. The helper moved into verseCaret.utils.ts so navigation
  and insertion share it. New footnote tests in both views failed before the fix.)

Minor — Consider

• No changelog entry for a caret change consumers can see (Ira Remove the scribe package #2) (fixed during review:
  bullet added under [Unreleased] → ### Fixed)
• The verse branch looked up the parent before the editable-marker path, which never used
  it (Ira Consider committing built dist/ like platform-bible-react, so consumers stop building at install time #4) (fixed during review: superseded. The branch is now a single helper call.)
• The caret placement relied on a later normalization in Standard view (selectNext() left
  an element point after an editable VerseNode) (fixed during review: placement is now
  explicit, via the shared helper)
• The comment claimed EmptyVerseCaretGuardPlugin hosts the caret in both views (fixed
  during review: the comment now describes the shared rule)
• The Standard-view test checked only the saved USJ, never the caret position (fixed
  during review: unit and editor-level tests pin the caret at the end of the new \v 2 text,
  with no caret host)
• No test pinned the scripture reference reported after the insert (fixed during review:
  onScrRefChange reports verse 2 in both views)
• if/else chain without braces; test read the paragraph by index; a duplicate
  requireViewOptions helper; tests read selection.anchor instead of focus (fixed
  during review)
• Consumers of getUsjMarkerAction see a behavior change; note it in the consuming
  paranext-core PR (author: this was always the intended behavior, and the signature is
  unchanged)
• Other non-char inline nodes (e.g. a milestone) inserted at the end of a paragraph still get
  the caret before them (deferred: this predates the change and needs a caret-host rule for
  non-verse decorators. It is a candidate for its own ticket.)

Template Propagation

Shared Regions Modified

None.

Extension Config Changes

None.

Positive Observations

• One rule for "the start of a verse's content". Insertion reuses the navigation helper instead
  of copying part of it, so the two can't drift apart. Its documented cases (text host,
  editable-marker end, character span, never entering a note, empty verse) now apply to inserted
  verses too.
• The move is behavior-neutral for navigation. ScriptureReferencePlugin.test.tsx passes
  unchanged.
• Every new test was watched to fail without its fix:
  • Unit tests: an immutable-marker test checks the element point, and an editable-marker test
    checks the end of the marker text.
  • Editor-level tests (verseInsertCaret.test.tsx), each in Simple's hidden-marker view and
    Standard view:
    • a caret host after the new verse with hidden markers, and the end of the marker text with
      editable markers
    • typed text landing in the new verse
    • the reported reference moving to verse 2
    • with a footnote following: the caret staying out of the note, and the typed text landing in
      the new verse
  • The \w span cases already passed and guard against regressions.
• No user-visible strings, components, build config or localization are affected.

Interview Notes

• Purpose (author's words): "To fix a small bug when trying to insert a verse without a
  duplicate verse number marker after it (like at the end of a chapter) putting the cursor before
  the new marker instead of after it."

• Verified in the app:

  • The author reproduced the end-of-chapter bug and Ira's footnote bug in Platform.Bible.
  • With the final fix staged locally (npm run build:editor), both now work as intended.
  • The cursor is visible between the new number and the footnote in Simple mode.

• Choosing the footnote fix: the author weighed three options:

  • A, Ira's suggestion: re-order the copied two-line empty-verse rule.
  • B: share $placeCaretAtVerseContentStart.
  • C: special-case NoteNode.

  The author chose B, because once something can follow the new verse, all of the helper's
  branches apply, and one shared rule can't drift. This supersedes an earlier choice to copy the
  two lines.

• The intermittent report: inserting between two verses that already exist selects the new
  marker. That is the intentional duplicate-number highlight from the PRD's verse-inference rule,
  left unchanged here. It likely explains why the ticket reported the bug as intermittent.

• Behavior change for consumers: the author considers the new caret placement, and the
  reference moving to the new verse, the intended behavior.

• Design record: the design is recorded in paranext-core at
  .context/designs/2026-11-03-verse-insert-caret-design.md, not yet committed. It describes the
  first version (selectNext()) and predates the review changes.

• No unresolved items.

In-Review Quality Check

All checks passed after the review fixes.

• Committed output: rebuilt with the steps in scripts/rebuild-committed-output.mjs, running
  the extract-api targets for platform-editor and then for utilities through npx, because
  pnpm can't find node in this shell. Only packages/platform/dist/index.js and index.js.map
  changed. The utilities output is unchanged.
• Typecheck: tsconfig.lib.json shows no errors other than TS6305 errors, which come from the
  stale local build output.
• Lint and format: clean on the changed files.
• Tests: the platform package passed: 181 test files passed and 2 were skipped; 4253 tests
  passed and 17 were skipped.

Suggested Review Focus

• Check the move of $placeCaretAtVerseContentStart into verseCaret.utils.ts. It moved
  unchanged apart from a doc line that now mentions insertion. Is the shared rule right for a
  freshly inserted verse in every case it covers?
• Check what happens when a character span follows a verse with an immutable marker. The caret
  goes to the start of the span's text, so typing joins the span. That is consistent with
  navigation, but confirm it's wanted for insertion.
• Check the editor-level caret-host test. jsdom fires no selectionchange, so the test
  dispatches SELECTION_CHANGE_COMMAND by hand. In the real app the browser provides it, and
  the author confirmed the behavior there manually.
• Decide whether the deferred case needs its own ticket: milestones and other inline nodes at
  the end of a paragraph still get the caret before them.
• Product question outside this PR: is the duplicate-number "select the inserted marker" rule
  still wanted?

🤖 Generated with Claude Code

---

This change is 

[irahopkinson]

@irahopkinson+AI made 3 comments.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on captaincrazybro).

---

a discussion (no related file):
Reviewed the verse-insert caret fix and its tests; 3 findings follow. Each comment shows the severity I settled on; 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

#2 (low · checked and confirmed) packages/platform/CHANGELOG.md:224 - The platform package's changelog has no entry for this fix, so host developers reading the release notes do not learn that the caret now lands in a verse inserted at the end of a paragraph.

What happens: the PR changes visible caret placement at usj-marker-action.utils.ts:538-546 but leaves packages/platform/CHANGELOG.md untouched; its [Unreleased] → ### Fixed section (:224) already lists comparable caret fixes.

Why it matters: documentation only; the release notes for the next platform-editor version will omit a behavior change consumers can see. Recent fix PRs added entries here, though no repo rule requires one.

Fix: add a bullet under ## [Unreleased] → ### Fixed in packages/platform/CHANGELOG.md stating that a verse inserted at the end of a paragraph now gets the caret after its number, so typing starts the new verse; keep Jira IDs out of it.

How this was checked: The PR changes src, tests and dist only (git diff --stat 06cbc96..HEAD); packages/platform/CHANGELOG.md is untouched, and its [Unreleased] ### Fixed section (CHANGELOG.md:224) holds the recent comparable entries. The caret-placement branch added at usj-marker-action.utils.ts:536-546 is user-visible. Recent fix PRs mostly do add entries: da56685 (caret after the number in an empty verse), 6f87a49, 52c9b53, ef60659 all touched the changelog, while 06cbc96 (context-menu close) did not, so the convention is common but not universal. No repo rule requires an entry, so this stays low and documentation-only.

Not attached to a line: this finding is in a file the pull request does not change, so the review has no file to attach a comment to

(AI-assisted, with my guidance)

---

packages/platform/src/editor/adaptors/usj-marker-action.utils.ts line 536 at r1 (raw file):

          } else {
            const nextNode = nodeToInsert.getNextSibling();
            if (nextNode) {

#1 - medium · checked and confirmed

Inserting a verse right after a footnoted word puts the caret inside the footnote: in Standard view the first typed characters land in the \f marker, not the new verse.

What happens: selection.insertNodes (usj-marker-action.utils.ts:506) places the new verse between the word and its NoteNode. nodeToInsert.getNextSibling() (:535) is that note, so nextNode.selectStart() (:537) descends to the note's first leaf: offset 0 of the \f marker text in Standard view, an element point inside the note in the hidden-marker view. The new $isSomeVerseNode branch (:538) is never reached because a next sibling exists.

Why it matters: in Standard view, text typed right after adding a verse after a footnoted word is lost from the saved USJ (getUsj() shows verse 5 followed directly by the note). In the hidden-marker view the typed text still lands in the new verse, but the caret starts inside the collapsed note. The nextNode.selectStart() line predates this PR, but it is the same wrong-caret-after-verse-insert defect this PR fixes, one branch over, so fixing it here is cheap.

Fix: In the plain-insert branch of getUsjMarkerAction (usj-marker-action.utils.ts:535-550), test $isSomeVerseNode(nodeToInsert) first, before nextNode.selectStart(): call nodeToInsert.selectEnd() for an editable verse ($isTextNode), else $placeCaretAtBoundary(parent, nodeToInsert.getIndexWithinParent() + 1); keep nextNode.selectStart() then nodeToInsert.selectStart() as the fall-backs for non-verse nodes. Verified: with a note after the insertion point the caret ends in the new verse's \v 5 text (Standard) or at a paragraph element point outside the note (hidden), and typed text lands in the new verse. Add one test per view to verseInsertCaret.test.tsx (para with a footnote after the word, insert v, read the selection's focus point, type text, assert getUsj() has the text after the new verse and the caret is never inside a NoteNode), chaining .append(...) if Lexical trees are built, then rebuild packages/platform/dist/ with node scripts/rebuild-committed-output.mjs.

How this was checked: Reproduced through mountViewEditor with para [\v 4, "third", note], caret at the end of "third", insertMarker("v"). In Standard view the caret lands at offset 0 of the note's "\f" marker text (usj-marker-action.utils.ts:535-537, nextNode is the NoteNode); typing "abc" was not in the saved USJ at all (getUsj shows verse 5 followed directly by the note). In the hidden-marker view the caret lands as an element point inside the note (offset 0), as claimed, but typing "abc" still ended up in verse 5 before the note in the saved USJ, so the data damage is Standard-only in jsdom. The if (nextNode) nextNode.selectStart() line is unchanged by this PR (the PR only added the $isSomeVerseNode arm after it), so the defect pre-exists and sits next to the new no-next-sibling arm; the anchor lines are not changed lines.
Correction: lines 536-537 do appear in this pull request's diff, but only because the existing next-sibling branch was reformatted with braces; it still calls selectStart on the next sibling exactly as before, so the behavior described here predates this pull request.

Other findings in this file: #4

---

packages/platform/src/editor/adaptors/usj-marker-action.utils.ts line 544 at r1 (raw file):

              // editable marker's own text, or the boundary after an immutable one, where
              // EmptyVerseCaretGuardPlugin hosts it.
              const parent = nodeToInsert.getParentOrThrow();

#4 - low · checked and confirmed

The new verse branch looks up the verse's parent before the editable-verse path, which never uses it.

What happens: const parent = nodeToInsert.getParentOrThrow() (usj-marker-action.utils.ts:544) runs before the $isTextNode(nodeToInsert) check at :545; only the $placeCaretAtBoundary call at :546 reads parent.

Why it matters: readability only, with no behavior effect: a reader expects parent to matter on both paths.

Fix: replace :544-546 with if ($isTextNode(nodeToInsert)) nodeToInsert.selectEnd(); else $placeCaretAtBoundary(nodeToInsert.getParentOrThrow(), nodeToInsert.getIndexWithinParent() + 1); and rebuild packages/platform/dist/.

How this was checked: In the verse branch, const parent = nodeToInsert.getParentOrThrow() at usj-marker-action.utils.ts:544 runs before the $isTextNode(nodeToInsert) check at :545. Only the $placeCaretAtBoundary(parent, ...) call at :546 reads parent, so the editable-verse path computes a value it never uses. The line is added by this PR (git diff against the merge base shows it as new). The lookup cannot fail on a node that was just inserted, so there is no behavior effect; it is a readability cost only.

Other findings in this file: #1

[captaincrazybro]

Thanks, Ira. All three are addressed in e2cd1d6:

#1: Fixed. An inserted verse now gets its caret from $placeCaretAtVerseContentStart, the same rule verse navigation uses, checked before the nextNode.selectStart() fallback. I moved that helper from ScriptureReferencePlugin.tsx into verseCaret.utils.ts so both paths share it, rather than re-ordering the copied two-line rule. That also covers a character span that opens the verse. verseInsertCaret.test.tsx now has footnote and \w cases in both the hidden-marker view and Standard view. The footnote cases failed before the change, and I confirmed the fix in the app.

#2: Added a bullet under [Unreleased] → ### Fixed in packages/platform/CHANGELOG.md.

#4: Fixed. The parent is now looked up only on the path that uses it, and the code is now a single helper call.

[captaincrazybro]

@captaincrazybro resolved 3 discussions.
Reviewable status: 0 of 8 files reviewed, all discussions resolved (waiting on irahopkinson+AI).

[irahopkinson]

@irahopkinson+AI made 1 comment.
Reviewable status: 0 of 8 files reviewed, all discussions resolved.

---

a discussion (no related file):

Previously, irahopkinson+AI (Agent AI for Ira Hopkinson) wrote…

Reviewed the verse-insert caret fix and its tests; 3 findings follow. Each comment shows the severity I settled on; 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

#2 (low · checked and confirmed) packages/platform/CHANGELOG.md:224 - The platform package's changelog has no entry for this fix, so host developers reading the release notes do not learn that the caret now lands in a verse inserted at the end of a paragraph.

What happens: the PR changes visible caret placement at usj-marker-action.utils.ts:538-546 but leaves packages/platform/CHANGELOG.md untouched; its [Unreleased] → ### Fixed section (:224) already lists comparable caret fixes.

Why it matters: documentation only; the release notes for the next platform-editor version will omit a behavior change consumers can see. Recent fix PRs added entries here, though no repo rule requires one.

Fix: add a bullet under ## [Unreleased] → ### Fixed in packages/platform/CHANGELOG.md stating that a verse inserted at the end of a paragraph now gets the caret after its number, so typing starts the new verse; keep Jira IDs out of it.

How this was checked: The PR changes src, tests and dist only (git diff --stat 06cbc96..HEAD); packages/platform/CHANGELOG.md is untouched, and its [Unreleased] ### Fixed section (CHANGELOG.md:224) holds the recent comparable entries. The caret-placement branch added at usj-marker-action.utils.ts:536-546 is user-visible. Recent fix PRs mostly do add entries: da56685 (caret after the number in an empty verse), 6f87a49, 52c9b53, ef60659 all touched the changelog, while 06cbc96 (context-menu close) did not, so the convention is common but not universal. No repo rule requires an entry, so this stays low and documentation-only.

Not attached to a line: this finding is in a file the pull request does not change, so the review has no file to attach a comment to

(AI-assisted, with my guidance)

Thanks — checked e2cd1d6: #1, #2 and #4 are fixed, and moving the helper into verseCaret.utils.ts also removes the duplicated caret rule. One small thing: now that $placeCaretAtVerseContentStart is exported, the repo CLAUDE.md asks its TSDoc to state the calling context ("Mutating: call inside editor.update()").

[irahopkinson]

I'm approving this so it can merge. Assuming you will either fix the TSDoc in this PR or a follow-up.

@irahopkinson made 1 comment.
Reviewable status: 0 of 8 files reviewed, all discussions resolved (waiting on irahopkinson+AI).