platform: fix BCV display so verse updates only when cursor is after the verse number

[jolierabideau]

BCV now updates only when the cursor moves past the verse number. It also shows the previous verse when the cursor is before the verse number (e.g. at offset 0 in a verse node or between verses).

Cursor position handling

• BCV shows the previous verse when the cursor is before a verse number (e.g. at offset 0 in a verse node or between verses).
• BCV only updates when the cursor moves past the verse number.

New utilities in shared-react

• $findPreviousVerseInSiblings(parent, fromIndex) – Finds the previous verse when the anchor points at a non-verse child (e.g. TypedMarkNode).
• $getEffectiveVerseForBcv(verseNode, selection) – Returns the effective verse for BCV display based on cursor position, including:
  • Cursor at start of verse node (before number) → previous verse
  • Cursor in verse range text (e.g. "2-3" at offset 1 or 2) → previous verse
  • Cursor in parent at offset 0 (before first verse) → verse 0
  • Cursor in parent between verses → previous verse
  • Cursor past last child → last verse in the paragraph

Selection handling

• getSelectionStartNode can throw when the anchor is on a DecoratorNode (e.g. ImmutableVerseNode).
• Added isSelectionStartNodeExpectedError(err) in shared to detect these Lexical errors.
• Scripture reference logic now:
  1. Checks anchor node type before calling getSelectionStartNode.
  2. Uses anchorNode directly when it would throw.
  3. Catches expected errors and falls back to $getNodeByKey(selection.anchor.key).

Execution context

• $findAndSetChapterAndVerse is wrapped in editor.getEditorState().read() when run from SELECTION_CHANGE_COMMAND, since the handler runs outside a Lexical read context.

Platform-specific

• Added ImmutableVerseNode mutation listener so SELECTION_CHANGE_COMMAND fires when a verse node is destroyed, even if the cursor position didn't change.

Testing

• New tests for $findPreviousVerseInSiblings and $getEffectiveVerseForBcv in node-react-utils.test.ts.
• New tests for isSelectionStartNodeExpectedError in node-utils.test.ts.
• New ScriptureReferencePlugin test: "should report verse 0 when cursor is on verse 1 number (before verse content)".

---

---

---

This change is 

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

• dev:perf-react
• dev:perf-vanilla
• dev:platform

[irahopkinson]

@irahopkinson reviewed 7 files and all commit messages, and made 18 comments.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on jolierabideau).

---

libs/shared/src/nodes/usj/node-utils.test.ts line 340 at r4 (raw file):

  describe("getSelectionStartNode() and isSelectionStartNodeExpectedError()", () => {
    it("isSelectionStartNodeExpectedError identifies Lexical DecoratorNode errors", () => {

NIT the describe and it blocks should read as a sentence as that is how it is output to console. e.g the second it block at the top of the file outputs: Editor Node Utilities isValidNumberedMarker() should identify a valid numbered marker. Here you could remove "getSelectionStartNode() and " from describe and "isSelectionStartNodeExpectedError " from it.

Code quote:

  describe("getSelectionStartNode() and isSelectionStartNodeExpectedError()", () => {
    it("isSelectionStartNodeExpectedError identifies Lexical DecoratorNode errors", () => {

---

libs/shared/src/nodes/usj/node.utils.ts line 570 at r4 (raw file):

    message.includes("$caretFromPoint") ||
    message.includes("does not inherit from ElementNode") ||
    message.includes("does not inherit from TextNode")

NIT the tests suggest the logic could be different, i.e.:

    message.includes("$caretFromPoint") &&
    (message.includes("does not inherit from ElementNode") ||
    message.includes("does not inherit from TextNode"))

Code quote:

    message.includes("$caretFromPoint") ||
    message.includes("does not inherit from ElementNode") ||
    message.includes("does not inherit from TextNode")

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 59 at r4 (raw file):

        const t1 = $createTextNode("text1");
        const t2 = $createTextNode("text2");
        root.append(p1.append(v1, t1, v2, t2));

BTW good clean-up to the more usual pattern which helps you see the node structure more easily. Although typically I inline any const where possible. These are some earlier tests before I settled on the inlined pattern.

Code quote:

root.append(p1.append(v1, t1, v2, t2));

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 236 at r4 (raw file):

      const p = $getRoot().getFirstChild();
      if (!p) throw new Error("paragraph not found");
      const verseNode = $findPreviousVerseInSiblings(p, 2);

BTW this line is the SUT (Software Under Test). It's good to make that obvious in some way. Here it is enough to have a blank line before and after. Check here and elsewhere.

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 499 at r4 (raw file):

        const root = $getRoot();
        const p = $createParaNode();
        const v1 = $createVerseNode("1", " verse one");

FYI we only use VerseNode in editable markerMode (not yet fully supported) so typically there would need to more in the ParaNode before the VerseNode to display the marker for the paragraph. Everywhere else we use ImmutableVerseNode. It likely doesn't matter here.

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 553 at r4 (raw file):

      const t1 = $getNodeByKey(verse1Key)?.getNextSibling();
      if (t1 && $isTextNode(t1)) t1.select(0, 0);
    });

BTW does this really need to be in a different update than the one above? Note some existing tests just do the select in the first update. Check here and below.

Code quote:

    editor.update(() => {
      const t1 = $getNodeByKey(verse1Key)?.getNextSibling();
      if (t1 && $isTextNode(t1)) t1.select(0, 0);
    });

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 419 at r4 (raw file):

  fromIndex: number,
): SomeVerseNode | undefined {
  if (!parent || !$isElementNode(parent) || fromIndex <= 0) return;

NIT you can remove this as the type guard checks for it.

Code quote:

!parent || 

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 423 at r4 (raw file):

  for (let i = fromIndex - 1; i >= 0; i--) {
    const child = children[i];
    if (child && $isSomeVerseNode(child)) return child as SomeVerseNode;

NIT you can remove this as the type guard checks for it.

Code quote:

child && 

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 502 at r4 (raw file):

 */
function getVerseNumberPrefixLength(verseNode: SomeVerseNode): number {
  if (!$isTextNode(verseNode)) return 0;

This would be more understandable if it was:

if (!$isVerseNode(verseNode)) return 0;

Code quote:

if (!$isTextNode(verseNode)) return 0;

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 538 at r4 (raw file):

  selection: RangeSelection,
): boolean {
  const anchorNode = $getNodeByKey(selection.anchor.key);

NIT typically this would be const anchorNode = selection.anchor.getNode().

Code quote:

const anchorNode = $getNodeByKey(selection.anchor.key);

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 580 at r4 (raw file):

  // No selection or not range: use verse node as-is
  if (!selection || !$isRangeSelection(selection)) {

NIT you can remove this as the type guard checks for it.

Code quote:

!selection || 

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 584 at r4 (raw file):

  }

  const selectedVerseNum = parseInt(verseNode.getNumber() ?? "0", 10);

NIT these days it's better to use Number.parsInt as its better and faster than the node one.

Code quote:

parseInt

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 101 at r4 (raw file):

          if (hasCursorMovedRef.current) hasCursorMovedRef.current = false;
          else {
            // Command handler runs outside any Lexical read/update context; read() gives $getSelection() etc. a valid state.

I don't believe this as actually true. What problem where you having without it? Can you point to Lexical docs that validate this? If it was true I don' think what was there before would have worked at all. Even if you do need it why not use the more typical usage in code (not tests) editor.read(...)?

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 165 at r4 (raw file):

}

function $findAndSetChapterAndVerse(

The additions to this function make it a lot harder to understand what is actually supposed to be happening. Can it be refactored to make it clearer and reduce the cognitive load?

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 173 at r4 (raw file):

) {
  const selection = $getSelection();
  let startNode: ReturnType<typeof getSelectionStartNode>;

NIT why not just use the actual type here : LexicalNode | undefined which reads better to me?

Code quote:

ReturnType<typeof getSelectionStartNode>

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 179 at r4 (raw file):

  // ElementNode/TextNode but the node is neither.
  if (selection && $isRangeSelection(selection)) {
    const anchorNode = $getNodeByKey(selection.anchor.key);

NIT typically this would be const anchorNode = selection.anchor.getNode().

Code quote:

const anchorNode = $getNodeByKey(selection.anchor.key);

---

packages/platform/src/editor/ScriptureReferencePlugin.test.tsx line 0 at r4 (raw file):
I tested in the demo app. Note there is an existing issue if you start in v1 - workaround: click elsewhere first. It works well with the left and right arrow keys but not with up and down. For testing I modified test data like this:

export const WEB_PSA_CH1_USX = `<?xml version="1.0" encoding="utf-8"?>
<usx version="3.0">
  <book code="PSA" style="id">World English Bible (WEB)</book>
  <para style="ide">UTF-8</para>
  <para style="h">Psalms</para>
  <para style="toc1">The Psalms</para>
  <para style="toc2">Psalms</para>
  <para style="toc3">Psalm</para>
  <para style="mt1">The Psalms</para>
  <para style="cl">Psalm</para>
  <chapter number="1" style="c" sid="PSA 1" />
  <para style="ms1">BOOK 1</para>
  <para style="q1">
    <verse number="1" style="v" sid="PSA 1:1" /><verse eid="PSA 1:1" /></para>
  <para style="q1">
    <verse number="2" style="v" sid="PSA 1:2" /><verse eid="PSA 1:2" /></para>
  <para style="q1">
    <verse number="3" style="v" sid="PSA 1:3" /><verse eid="PSA 1:3" /></para>
  <para style="q1">
    <verse number="4" style="v" sid="PSA 1:4" /><verse eid="PSA 1:4" /></para>
  <para style="q1">
    <verse number="5" style="v" sid="PSA 1:5" /><verse eid="PSA 1:5" /></para>
  <para style="q1">
    <verse number="6" style="v" sid="PSA 1:6" /><verse eid="PSA 1:6" /></para>
  <chapter eid="PSA 1" />
</usx>
`;

I also tested with verses all in one para and with an implied para (verses not in any para). And I tested in the 3 markerMode states for each of these.

---

packages/scribe/src/editor/plugins/ScriptureReferencePlugin.tsx line 0 at r4 (raw file):
FYI it's nice of you to do this update for Scribe but we don't need to. I only do updates when required by a breaking change.

[jolierabideau]

@jolierabideau made 4 comments and resolved 12 discussions.
Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on irahopkinson and jolierabideau).

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 502 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This would be more understandable if it was:

if (!$isVerseNode(verseNode)) return 0;

Done.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 101 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I don't believe this as actually true. What problem where you having without it? Can you point to Lexical docs that validate this? If it was true I don' think what was there before would have worked at all. Even if you do need it why not use the more typical usage in code (not tests) editor.read(...)?

You are right I did not need this. I think this was one of the first things Cursor recommended that I forgot to remove. I did look at the Lexical docs and it says the command listener is always called from an editor.update.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 165 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

The additions to this function make it a lot harder to understand what is actually supposed to be happening. Can it be refactored to make it clearer and reduce the cognitive load?

Done.

---

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

Previously, irahopkinson (Ira Hopkinson) wrote…

I tested in the demo app. Note there is an existing issue if you start in v1 - workaround: click elsewhere first. It works well with the left and right arrow keys but not with up and down. For testing I modified test data like this:

export const WEB_PSA_CH1_USX = `<?xml version="1.0" encoding="utf-8"?>
<usx version="3.0">
  <book code="PSA" style="id">World English Bible (WEB)</book>
  <para style="ide">UTF-8</para>
  <para style="h">Psalms</para>
  <para style="toc1">The Psalms</para>
  <para style="toc2">Psalms</para>
  <para style="toc3">Psalm</para>
  <para style="mt1">The Psalms</para>
  <para style="cl">Psalm</para>
  <chapter number="1" style="c" sid="PSA 1" />
  <para style="ms1">BOOK 1</para>
  <para style="q1">
    <verse number="1" style="v" sid="PSA 1:1" /><verse eid="PSA 1:1" /></para>
  <para style="q1">
    <verse number="2" style="v" sid="PSA 1:2" /><verse eid="PSA 1:2" /></para>
  <para style="q1">
    <verse number="3" style="v" sid="PSA 1:3" /><verse eid="PSA 1:3" /></para>
  <para style="q1">
    <verse number="4" style="v" sid="PSA 1:4" /><verse eid="PSA 1:4" /></para>
  <para style="q1">
    <verse number="5" style="v" sid="PSA 1:5" /><verse eid="PSA 1:5" /></para>
  <para style="q1">
    <verse number="6" style="v" sid="PSA 1:6" /><verse eid="PSA 1:6" /></para>
  <chapter eid="PSA 1" />
</usx>
`;

I also tested with verses all in one para and with an implied para (verses not in any para). And I tested in the 3 markerMode states for each of these.

Working

[irahopkinson]

@irahopkinson reviewed 7 files and all commit messages, made 11 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on jolierabideau).

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 101 at r4 (raw file):

Previously, jolierabideau wrote…

You are right I did not need this. I think this was one of the first things Cursor recommended that I forgot to remove. I did look at the Lexical docs and it says the command listener is always called from an editor.update.

Gotcha. I've found with AI that if it try's several different ways before it gets a working solution I have to tell to check which of all its changes are actually needed. It does a pretty good job of figuring that out but it won't do it unless I ask it to.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 138 at r5 (raw file):

    () =>
      editor.registerUpdateListener(({ editorState }) => {
        $getBookCode(editorState, onScrRefChange, scrRef);

BTW I didn't copy this solution from Scribe because I was concerned that it was just working around the issue rather than getting to the root of it. However, I haven't actually spent any time investigating so I don't know that this is a good solution or not. Why isn't the right book code already passed in correctly in the scfRef?

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 177 at r5 (raw file):

 * getSelectionStartNode would throw (e.g. cursor on DecoratorNode like ImmutableVerseNode).
 */
function $getSelectionStartNodeSafe(

It seems like this should just replace the function in node.utils.ts. You could just move this function there if doing that makes a breaking change.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 180 at r5 (raw file):

  selection: ReturnType<typeof $getSelection>,
): LexicalNode | undefined {
  if (!selection || !$isRangeSelection(selection)) {

NIT again you can remove this

Code quote:

!selection || 

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 185 at r5 (raw file):

  const anchorNode = selection.anchor.getNode();
  const anchorTypeMismatch =

Use our boolean naming convention, e.g. change to isAnchorTypeMismatch.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 196 at r5 (raw file):

  try {
    const node = getSelectionStartNode(selection);
    return node ?? $getNodeByKey(selection.anchor.key) ?? undefined;

Why does this need to get the selection anchor node again when we already have it. Replace with anchorNode?

Code quote:

$getNodeByKey(selection.anchor.key)

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 209 at r5 (raw file):

 * (e.g. para) rather than inside a verse, looks at the child at offset or walks backward.
 */
function $resolveVerseNode(startNode: LexicalNode, selection: ReturnType<typeof $getSelection>) {

NIT the typical order of functions in TS files in this repo is to define functions below where they are called. This means that when you read a file the important things are first, e.g. exported functions, and then with increasing detail of how it works as you go down the file.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 210 at r5 (raw file):

 */
function $resolveVerseNode(startNode: LexicalNode, selection: ReturnType<typeof $getSelection>) {
  let verseNode = $findThisVerse(startNode);

'verseNode' is never reassigned. Use 'const' instead.
I'm a little concerned with your set up if your IDE is not telling you this.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 252 at r5 (raw file):

    : verseNum === effectiveVerseNum;

  const chapterChanged = chapterNode && selectedChapterNum !== chapterNum;

Use our boolean naming convention, hasChapterChanged

Code quote:

chapterChanged

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 253 at r5 (raw file):

  const chapterChanged = chapterNode && selectedChapterNum !== chapterNum;
  const verseChanged = !isVerseInCurrentRange;

Use our boolean naming convention, hasVerseChanged

Code quote:

verseChanged 

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 275 at r5 (raw file):

) {
  editorState.read(() => {
    const root = $getRoot();

NIT you don't use this anywhere else so you can inline it.

[irahopkinson]

@irahopkinson reviewed 6 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on jolierabideau).

[jolierabideau]

@jolierabideau made 7 comments and resolved 5 discussions.
Reviewable status: 4 of 16 files reviewed, 7 unresolved discussions (waiting on irahopkinson and jolierabideau).

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 138 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I didn't copy this solution from Scribe because I was concerned that it was just working around the issue rather than getting to the root of it. However, I haven't actually spent any time investigating so I don't know that this is a good solution or not. Why isn't the right book code already passed in correctly in the scfRef?

From what I dug into, scrRef and the loaded USJ come in as separate props, so scrRef.book isn't guaranteed to match the \id in the file unless the host always keeps them in sync. I added an Editor change that uses getBookCodeFromUsj to align book with the content we're editing. The plugin still handles the chapter/verse from cursor. So I don't believe the Scribe solution is a workaround but happy to continue investigating or tweaking this solution if you have other insights!

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 177 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

It seems like this should just replace the function in node.utils.ts. You could just move this function there if doing that makes a breaking change.

Done.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 185 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Use our boolean naming convention, e.g. change to isAnchorTypeMismatch.

Done.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 196 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Why does this need to get the selection anchor node again when we already have it. Replace with anchorNode?

Done.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 252 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Use our boolean naming convention, hasChapterChanged

Done.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 253 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Use our boolean naming convention, hasVerseChanged

Done.

---

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

Previously, jolierabideau wrote…

Working

I added some code to ArrowNavigationPlugin to handle up and down arrow verse navigation. For the existing quirk when you start in verse 1 and clicking elsewhere first works around it. What are you seeing in that case?

[irahopkinson]

@irahopkinson reviewed 12 files and all commit messages, made 10 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on jolierabideau).

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 138 at r5 (raw file):

Previously, jolierabideau wrote…

From what I dug into, scrRef and the loaded USJ come in as separate props, so scrRef.book isn't guaranteed to match the \id in the file unless the host always keeps them in sync. I added an Editor change that uses getBookCodeFromUsj to align book with the content we're editing. The plugin still handles the chapter/verse from cursor. So I don't believe the Scribe solution is a workaround but happy to continue investigating or tweaking this solution if you have other insights!

Thank you, that reminds me why I didn't do this. The USJ we get doesn't have a book marker if it isn't the first chapter. A lot of our test data has the book code (for other reasons) so this reality is easy to miss. So we have to rely on the host supplying us with the right scrRef and usj together. When we test this in the editor repo (nx dev platform) the scrRef prop should already be set but but something isn't right on the first load without clicking out of v1 first. It could be that our problem stems for that scrRef is a prop and usj is set by an API call so they inherently aren't changed together! I think this just got too large to fix in the scope of this issue so perhaps just revert this "fix" since it just hides it in ch1 where we do most of our testing.

---

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

Previously, jolierabideau wrote…

I added some code to ArrowNavigationPlugin to handle up and down arrow verse navigation. For the existing quirk when you start in verse 1 and clicking elsewhere first works around it. What are you seeing in that case?

I'm referring to the "fix" you initially pulled from Scribe which just hides the problem in ch1. See the comment in ScriptureReferencePlugin.

---

libs/shared/src/nodes/usj/node.utils.ts line 591 at r10 (raw file):

}

function getSelectionStartNodeInner(selection: BaseSelection | null): LexicalNode | undefined {

NIT function declarations go below where they are called and ideally below any other exported functions (since this one isn't).

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 633 at r10 (raw file):

 * @returns `true` if the selection was moved, `false` otherwise.
 */
export function $selectNextVerse(selection: RangeSelection): boolean {

This function should check that the selection is single-sided and not a range since you modify it to single-sided. I.e.:

  if (!selection.isCollapsed()) return false;

Or maybe it should throw since it would be using it incorrectly?

Also do this in the function below.

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 647 at r10 (raw file):

        const child = children[i];
        if ($isSomeVerseNode(child)) {
          nextVerse = child as SomeVerseNode;

BTW it's odd you need to do this type assert when the type guard $isSomeVerseNode should be doing this for you. Type assertion is a code smell. I wonder why the type guard isn't working?

Code quote:

 as SomeVerseNode

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 243 at r10 (raw file):

  const isCursorOnElement =
    $isElementNode(startNode) &&
    selection &&

NIT again you don't need this as the following type guard checks this.

Code quote:

selection &&

---

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

        firstVerseTextNode = $createTextNode("first verse text ");
        secondVerseTextNode = $createTextNode("second verse text ");
        thirdVerseTextNode = $createTextNode("third verse text ");

NIT this may be irrelevant now but these variables aren't used anywhere so just inline the content. Here and the 3rd test also.

Code quote:

        sectionTextNode = $createTextNode("Section Text");
        firstVerseTextNode = $createTextNode("first verse text ");
        secondVerseTextNode = $createTextNode("second verse text ");
        thirdVerseTextNode = $createTextNode("third verse text ");

---

packages/utilities/etc/scripture-utilities.api.md line 23 at r10 (raw file):

// @public
export function getBookCodeFromUsj(usj: Usj | null | undefined): string | undefined;

BTW here is another reason to move the getBookCodeFromUsj to shared utilities so it doesn't need to be exported through our API to the public.

---

packages/utilities/src/index.ts line 27 at r10 (raw file):

export {
  EMPTY_USJ,
  getBookCodeFromUsj,

BTE here is another reason to move the getBookCodeFromUsj to shared utilities so it doesn't need to be exported through our API to the public.

---

packages/utilities/src/converters/usj/usj.model.ts line 107 at r10 (raw file):

 * @public
 */
export function getBookCodeFromUsj(usj: Usj | null | undefined): string | undefined {

We don't normally put code in a .model.ts file. .model.ts files are useful for stopping circular dependencies - adding code defeats that purpose. This should be moved to a utilities file in shared.

I know there is another function already declared here but it is just a wrapper function.

[jolierabideau]

@jolierabideau made 5 comments and resolved 5 discussions.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on irahopkinson).

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 633 at r10 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This function should check that the selection is single-sided and not a range since you modify it to single-sided. I.e.:

  if (!selection.isCollapsed()) return false;

Or maybe it should throw since it would be using it incorrectly?

Also do this in the function below.

Good point! I decided to return false instead of throw because the function is asking "did we move the caret?" and for a non-collapsed selection we don't. But I am happy to change that if we want it to be a hard rule.

---

libs/shared-react/src/nodes/usj/node-react.utils.ts line 647 at r10 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW it's odd you need to do this type assert when the type guard $isSomeVerseNode should be doing this for you. Type assertion is a code smell. I wonder why the type guard isn't working?

Good catch! I think this was another lingering AI addition, I will be more careful about looking for those. It wasn't necessary I am not getting type errors. I also fixed this in a couple other places.

---

packages/platform/src/editor/ScriptureReferencePlugin.tsx line 138 at r5 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Thank you, that reminds me why I didn't do this. The USJ we get doesn't have a book marker if it isn't the first chapter. A lot of our test data has the book code (for other reasons) so this reality is easy to miss. So we have to rely on the host supplying us with the right scrRef and usj together. When we test this in the editor repo (nx dev platform) the scrRef prop should already be set but but something isn't right on the first load without clicking out of v1 first. It could be that our problem stems for that scrRef is a prop and usj is set by an API call so they inherently aren't changed together! I think this just got too large to fix in the scope of this issue so perhaps just revert this "fix" since it just hides it in ch1 where we do most of our testing.

Okay, that makes sense! Reverted the hiding!

---

packages/utilities/src/index.ts line 27 at r10 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTE here is another reason to move the getBookCodeFromUsj to shared utilities so it doesn't need to be exported through our API to the public.

Done.

---

packages/utilities/src/converters/usj/usj.model.ts line 107 at r10 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

We don't normally put code in a .model.ts file. .model.ts files are useful for stopping circular dependencies - adding code defeats that purpose. This should be moved to a utilities file in shared.

I know there is another function already declared here but it is just a wrapper function.

Done.

[irahopkinson]

Nearly there. Up and down arrow keys and just some unused code to revert.

@irahopkinson partially reviewed 12 files and all commit messages, made 4 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on jolierabideau).

---

libs/shared/src/utils/usj/getBookCodeFromUsj.ts line 7 at r11 (raw file):

 * `type === "book"` and `marker === "id"`.
 */
export function getBookCodeFromUsj(usj: Usj | null | undefined): string | undefined {

BTW Err.. this isn't called from anywhere now. Remove it and the test.

---

libs/shared/src/utils/usj/index.ts line 2 at r11 (raw file):

export * from "./contentToLexicalNode.js";
export * from "./getBookCodeFromUsj.js";

BTW Err.. this isn't called from anywhere now. Remove it.

---

libs/shared-react/src/plugins/usj/ArrowNavigationPlugin.tsx line 67 at r11 (raw file):

      if (!$isRangeSelection(selection) || !selection.isCollapsed()) return false;

      if (event.key === "ArrowUp") {

With this test data the up and down arrow keys are jumpingto every other verse:

export const WEB_PSA_CH1_USX = `<?xml version="1.0" encoding="utf-8"?>
<usx version="3.0">
  <book code="PSA" style="id">World English Bible (WEB)</book>
  <para style="ide">UTF-8</para>
  <para style="h">Psalms</para>
  <para style="toc1">The Psalms</para>
  <para style="toc2">Psalms</para>
  <para style="toc3">Psalm</para>
  <para style="mt1">The Psalms</para>
  <para style="cl">Psalm</para>
  <chapter number="1" style="c" sid="PSA 1" />
  <para style="ms1">BOOK 1</para>
  <para style="q1">
    <verse number="1" style="v" sid="PSA 1:1" /><verse eid="PSA 1:1" /></para>
  <para style="q1">
    <verse number="2" style="v" sid="PSA 1:2" /><verse eid="PSA 1:2" /></para>
  <para style="q1">
    <verse number="3" style="v" sid="PSA 1:3" /><verse eid="PSA 1:3" /></para>
  <para style="q1">
    <verse number="4" style="v" sid="PSA 1:4" /><verse eid="PSA 1:4" /></para>
  <para style="q1">
    <verse number="5" style="v" sid="PSA 1:5" /><verse eid="PSA 1:5" /></para>
  <para style="q1">
    <verse number="6" style="v" sid="PSA 1:6" /><verse eid="PSA 1:6" /></para>
  <chapter eid="PSA 1" />
</usx>
`;

[jolierabideau]

@jolierabideau made 2 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on irahopkinson).

---

libs/shared/src/utils/usj/getBookCodeFromUsj.ts line 7 at r11 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW Err.. this isn't called from anywhere now. Remove it and the test.

Geez, I guess I'm having a Monday 🤦🏻‍♀️ Thank you!

---

libs/shared-react/src/plugins/usj/ArrowNavigationPlugin.tsx line 67 at r11 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

With this test data the up and down arrow keys are jumpingto every other verse:

export const WEB_PSA_CH1_USX = `<?xml version="1.0" encoding="utf-8"?>
<usx version="3.0">
  <book code="PSA" style="id">World English Bible (WEB)</book>
  <para style="ide">UTF-8</para>
  <para style="h">Psalms</para>
  <para style="toc1">The Psalms</para>
  <para style="toc2">Psalms</para>
  <para style="toc3">Psalm</para>
  <para style="mt1">The Psalms</para>
  <para style="cl">Psalm</para>
  <chapter number="1" style="c" sid="PSA 1" />
  <para style="ms1">BOOK 1</para>
  <para style="q1">
    <verse number="1" style="v" sid="PSA 1:1" /><verse eid="PSA 1:1" /></para>
  <para style="q1">
    <verse number="2" style="v" sid="PSA 1:2" /><verse eid="PSA 1:2" /></para>
  <para style="q1">
    <verse number="3" style="v" sid="PSA 1:3" /><verse eid="PSA 1:3" /></para>
  <para style="q1">
    <verse number="4" style="v" sid="PSA 1:4" /><verse eid="PSA 1:4" /></para>
  <para style="q1">
    <verse number="5" style="v" sid="PSA 1:5" /><verse eid="PSA 1:5" /></para>
  <para style="q1">
    <verse number="6" style="v" sid="PSA 1:6" /><verse eid="PSA 1:6" /></para>
  <chapter eid="PSA 1" />
</usx>
`;

Done.

[irahopkinson]

@irahopkinson partially reviewed 6 files and all commit messages, made 6 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on jolierabideau).

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 705 at r12 (raw file):

});

describe("$selectNextVerse() / $selectPreviousVerse() / $resolveVerseNode()", () => {

These tests should be in ArrowNavigationPlugin.test.tsx with the other ones.

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 716 at r12 (raw file):

          $createImmutableVerseNode("1"),
        );
        const p2 = $createParaNode("q1").append(

NIT again it's really hard to see the structure of the document with all these consts. You only need p2 so inline the rest including the .append from p2. Perhaps you need to add something to your AI instructions so it doesn't keep doing this?

Also for the test below.

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 725 at r12 (raw file):

        );
        $getRoot().append($createImmutableChapterNode("1"), pMs, p1, p2, p3);
        para2Key = p2.getKey();

NIT might be better to rename this p2Key for consistency.

Also for the test below.

Code quote:

para2Key

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 737 at r12 (raw file):

      },
      { discrete: true },
    );

Ideally use updateSelection function from test.utils.ts. AI isn't great at reusing existing functions.

Also for the test below.

Code quote:

    editor.update(
      () => {
        const selection = $createRangeSelection();
        selection.anchor = $createPoint(para2Key, 2, "element");
        selection.focus = $createPoint(para2Key, 2, "element");
        $setSelection(selection);
      },
      { discrete: true },
    );

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 745 at r12 (raw file):

      expect($findThisVerse(p2)?.getNumber()).toBe("1");

      const sel = $getSelection();

NIT is this AI using abbreviations for var names? Rename.

Also for the test below.

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 752 at r12 (raw file):

    editor.update(
      () => {
        const sel = $getSelection();

NIT is this AI using abbreviations for var names? Rename.

[jolierabideau]

@jolierabideau made 3 comments and resolved 3 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on irahopkinson).

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 705 at r12 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

These tests should be in ArrowNavigationPlugin.test.tsx with the other ones.

Done.

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 716 at r12 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

NIT again it's really hard to see the structure of the document with all these consts. You only need p2 so inline the rest including the .append from p2. Perhaps you need to add something to your AI instructions so it doesn't keep doing this?

Also for the test below.

That's a good idea! I added some

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 737 at r12 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Ideally use updateSelection function from test.utils.ts. AI isn't great at reusing existing functions.

Also for the test below.

Done.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 18 additional findings in Devin Review.

[devin-ai-integration]

🟡 $selectNextVerse skips a verse when cursor is at element offset before first verse in paragraph

$resolveVerseNode at node-react.utils.ts:717 returns a forward-found verse (via $findNextVerseInNode) when the cursor is at an element offset before all verses in the paragraph (e.g., offset 0 of [linebreak, verse2]). $selectNextVerse then treats this forward-found verse as the "current" verse and searches for the next one after it (node-react.utils.ts:616), causing navigation to skip the verse the user hasn't reached yet.

For example, with paragraphs [..linebreak, v1], [linebreak, v2], [linebreak, v3] and the cursor at element offset 0 of the v2 paragraph, $resolveVerseNode returns v2 via the firstVerseInPara fallback. $selectNextVerse then looks for the verse after v2 and navigates to v3, skipping v2. The correct behavior would be to navigate to v2's content since the cursor is positioned before it.

Trace of the issue at offset 0

1. $resolveVerseNode(para, selection) → isCursorOnElement true → getChildAtIndex(0) = linebreak → $findPreviousVerseInSiblings(para, 0) returns undefined (fromIndex=0) → $findNextVerseInNode(para) finds verse2 → returns verse2
2. $selectNextVerse: currentVerse = verse2 → loops from currentIndex + 1 → no more verses in para → searches next paragraphs → finds verse3 → navigates there, skipping verse2

Prompt for agents

In libs/shared-react/src/nodes/usj/node-react.utils.ts, the $selectNextVerse function (line 603-651) needs to handle the case where $resolveVerseNode returns a verse that is actually AHEAD of the cursor position (found via the firstVerseInPara forward-search fallback in $resolveVerseNode).

When this happens, the resolved verse should be treated as the "next" verse to navigate to, rather than the "current" verse to skip past.

One approach: In $selectNextVerse, after resolving currentVerse, compare the anchor position with the verse's position. If the anchor offset (for element selections) is less than the verse's index in the parent, the verse is ahead of the cursor. In that case, navigate directly to currentVerse.selectNext(0, 0) instead of searching for the next verse after it.

Alternatively, $resolveVerseNode could be modified to not return forward-found verses (or return additional metadata indicating how the verse was found), but this would require changes to how $getEffectiveVerseForBcv uses $resolveVerseNode for BCV display.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[irahopkinson]

@irahopkinson reviewed 4 files and all commit messages, made 3 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on jolierabideau).

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 649 at r13 (raw file):

    const { editor } = createBasicTestEnvironment([ParaNode, ImmutableVerseNode], () => {
      const v1 = $createImmutableVerseNode("1");
      const paraNode = $createParaNode().append(v1, $createImmutableVerseNode("2"));

NIT this .append should go back into the line below to make the structure clearer.

Code quote:

.append(v1, $createImmutableVerseNode("2"))

---

libs/shared-react/src/nodes/usj/node-react-utils.test.ts line 679 at r13 (raw file):

    const { editor } = createBasicTestEnvironment([ParaNode, ImmutableVerseNode], () => {
      const v2 = $createImmutableVerseNode("2");
      const paraNode = $createParaNode().append($createImmutableVerseNode("1"), v2);

NIT this .append should go back into the line below to make the structure clearer.

---

libs/shared-react/src/plugins/usj/ArrowNavigationPlugin.test.tsx line 415 at r13 (raw file):

  });

  describe("$selectNextVerse() / $selectPreviousVerse() / $resolveVerseNode()", () => {

I'm sorry I was clear enough. These tests didn't just need moving as is - they need to test their use through this plugin like the tests above do. Following their patten should greatly simplify these tests so we can see what we are actually testing.