Clean up ScriptureReferencePlugin after PR #499#500
Conversation
- moved helper functions below the main plugin export function following the existing code ordering pattern. - minor doc changes to improve clarity. - renamed a couple of helper functions for clarity. - changed to a named plugin export (using a `default` export is the old style). - use `editor.read()` instead of `editor.getEditorState().read()` - it flushes pending updates before reading and provides editor context, and is the recommended form.
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders Open Preview |
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Thanks for making these fixes, Ira! The code looks good so I will go ahead and approve. There are two documentation notes that maybe you can fold into another PR later, but no worries if you don't.
@katherinejensen00 reviewed 3 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on irahopkinson).
packages/platform/src/editor/ScriptureReferencePlugin.tsx line 340 at r1 (raw file):
// synthetic selection settle fires before the deferred placement and must be silenced - // regardless of book match, since a same-book reload's transient chapter-top settle is the // R2 clobber this branch exists to prevent. The correction below (d), if any, runs after.
NIT Claude Finding
The (a)–(d) cross-references in onDocumentChanged are used in prose but never anchored to their code branches.
The REPLACEMENT branch says "The correction below (d), if any, runs after." The book correction block says "Runs after (a)–(c); under (a) phase is already 'navigating' here." But no comment in the code is actually labeled // (a), // (b), etc. — a reader has to manually count branches to decode the letter scheme. If (a) is REPLACEMENT, (b) is MOUNT, (c) is the paste/undo no-op, and (d) is the book correction block, that can be inferred, but it takes real effort. This predates your PR, so it doesn't need to block merge — but a one-liner label at the start of each branch (// (a) REPLACEMENT: ..., etc.) would make the cross-references self-evident.
packages/platform/src/editor/ScriptureReferencePlugin.tsx line 375 at r1 (raw file):
} function $moveCaretToVerseStart(chapterNum: number, verseNum: number) {
NIT Claude Finding
$moveCaretToVerseStart is the only named handler in the file without a JSDoc, and it's the most complex one.
Every other handler in the file — onPropChanged, consumePendingEcho, $resolvePosition, onDocumentChanged, schedulePlacingCaretAtVerseStart, onSelectionSettled, positionMatchesScrRef, verseInRangeSafe, positionToScrRef, report, refsEqual, onUserInteracted — has a function-level JSDoc comment. $moveCaretToVerseStart doesn't, and it's the function with the most state assumptions: it must be called inside a Lexical editor-state update, it has an early-return when the caret is already in a verse range, and it handles the "no verse node, fall back to para" case. The file-top HOW-THIS-PLUGIN-WORKS block explains the overall algorithm but doesn't substitute for a function signature–adjacent description that callers read in context. Since this PR renamed the function and moved it, this is a good moment to close that gap with even two or three sentences.
lyonsil
left a comment
lyonsil
left a comment
There was a problem hiding this comment.
Review: the reorg is clean, but two editor.read() swaps are not cleanup
I verified the mechanical half of this PR and it is solid: every moved function body is byte-identical apart from the two swaps below, no other file imports the old default export, no barrel/mock/lazy-import references it, and nx extract-api is not needed (the plugin is not on the package's public surface). No CLAUDE.md violations, no over-length lines, no em dashes.
The concern is the two editor.getEditorState().read(cb) -> editor.read(cb) swaps (lines 199 and 270). In Lexical 0.43 those are not synonyms (Lexical.dev.mjs:10935):
read(callbackFn) {
$commitPendingUpdates(this); // <-- synchronously FLUSHES pending updates
return this.getEditorState().read(callbackFn, { editor: this });
}There is no _updating guard - the very guard Lexical applies to itself in setEditorState ("otherwise this will cause a second commit with an already read-only state and selection") and warns about in triggerCommandListeners ("...will potentially corrupt the nodeMap by doing GC too early").
And the flush window is real in production: LoadStatePlugin calls editor.setEditorState() inside editor.update(), so _updating === true, setEditorState's own commit is skipped, and _pendingEditorState survives - holding the new document - until a later microtask.
Recommendation: revert both swaps to editor.getEditorState().read(...). At line 199 the flush is provably incapable of changing the value read (Lexical nulls _pendingEditorState at :8504, before firing mutation listeners at :8605), so it is pure risk for zero benefit. At line 270 it is actively wrong: it commits a pending document and then reads it, which is exactly what the gate on the next line exists to prevent. The other two mutation listeners in this editor (AnnotationPlugin.tsx:85, CommentPlugin.tsx:862) both still use getEditorState().read(...), so reverting restores consistency rather than breaking it.
If the terseness is the goal, extract a named helper instead - see the comment on line 199.
Comments below: 1-6 are introduced by this PR; 14-25 are cleanup notes held to the PR's own "cleanup" billing. (I also found 7 pre-existing defects from #499 in the functions this PR moved - empty-book-code placement, sawDocument vs book-less documents, a stale verse range riding into book corrections, and others. Those are not this PR's doing and I have left them off this review; happy to file them separately.)
Verified against Lexical 0.43.0 and React 18.3.1 (note: repo CLAUDE.md still says React 19.1.0 - it resolves 18.3.1).
| machine.phase = "navigating"; | ||
| machine.pendingEchoes.length = 0; | ||
| // Prop-driven placement gate: never move the caret inside a different book's (stale) document. | ||
| const bookCode = editor.read(() => $getFirstBookNode()?.getCode() || undefined); |
There was a problem hiding this comment.
[1/18 - CONFIRMED] The flush makes useDecorators call flushSync inside React's commit phase.
onPropChanged runs from a React passive effect. When this editor.read() flush is non-inert, $commitPendingUpdates fires Lexical's decorator listener, and @lexical/react's useDecorators (LexicalRichTextPlugin.dev.mjs:72-77) calls flushSync. That now runs inside flushPassiveEffects, so React logs "flushSync was called from inside a lifecycle method" and silently defers the decorator render instead of flushing it.
Reproduced end-to-end against real React 18.3.1 + Lexical 0.43.0 with a LexicalComposer + RichTextPlugin + a DecoratorNode. This is not a corner case: ImmutableVerseNode, ImmutableChapterNode, ImmutableNoteCallerNode, MilestoneNode all extend DecoratorNode, so any document-load FULL_RECONCILE populates _pendingDecorators.
Worth noting LoadStatePlugin already guards this exact hazard on purpose: "Use queueMicrotask to defer the editor update outside of React's lifecycle, preventing flushSync warnings when this is triggered by a parent component update." This line reintroduces precisely what that queueMicrotask exists to avoid.
This is the one consequence of the swap that is live and confirmed rather than latent.
Fix: editor.getEditorState().read(...).
There was a problem hiding this comment.
Fixed in 36bceac - both sites reverted to editor.getEditorState().read() via a getCommittedBookCode helper that documents the committed-snapshot requirement.
| machine.phase = "navigating"; | ||
| machine.pendingEchoes.length = 0; | ||
| // Prop-driven placement gate: never move the caret inside a different book's (stale) document. | ||
| const bookCode = editor.read(() => $getFirstBookNode()?.getCode() || undefined); |
There was a problem hiding this comment.
[2/18 - PLAUSIBLE, most severe] editor.read() has no _updating guard, so it can commit a half-built editor state.
report() calls the host's onScrRefChange from inside onSelectionSettled, which Lexical runs inside an open $beginUpdate (_updating === true, _pendingEditorState a live mutable clone) - the file's own comment on onSelectionSettled says as much. If the host responds with a synchronous re-render (flushSync, a sync external store), React flushes the pending passive effect re-entrantly, onPropChanged runs, and this editor.read() commits that half-built state: $commitPendingUpdates sets _readOnly = true and freezes the selection / swaps the node map for throwing stubs while $beginUpdate is still between updateFn() and $applyAllTransforms/$garbageCollectDetachedNodes.
Reproduced under a flushSync host:
MODE=pr (editor.read) -> errors: ['Cannot call set() on a frozen Lexical node map']
committed text: "GEN" <-- enclosing update's work LOST
MODE=main (getEditorState().read) -> errors: []
committed text: "GEN\n\nWORK-AFTER-THE-READ" <-- survives
The enclosing update's tail-scheduled commit then no-ops, because _pendingEditorState is already null - so the edit is silently dropped. And the freeze guard is dev-only (grep -c "frozen Lexical node map" Lexical.prod.mjs -> 0), so production corrupts silently where dev throws.
This is exactly the hazard Lexical guards in setEditorState with if (!this._updating) { $commitPendingUpdates(this); }. editor.read() has no such guard.
Marked PLAUSIBLE only because the trigger depends on whether the Platform.Bible host commits scrRef synchronously - which I cannot see from this repo. Could you confirm whether the host wraps the scrRef update in flushSync or a sync store? If it does, this is a crash-and-lose-the-edit bug.
There was a problem hiding this comment.
Update: I answered my own question - the trigger is NOT present. Downgrading this to latent.
Rather than leave this hanging for you, I traced the host chain in paranext-core:
platform-scripture-editor.web-view.tsx:1692 passes onScrRefChange={setScrRefNoScroll} -> setScrRefNoScroll (:1127-1140) -> setScrRefWithScroll from useWebViewScrollGroupScrRef() -> useScrollGroupScrRef (use-scroll-group-scr-ref.hook.ts:99-114) -> setScrRefLocalIfDifferent -> setScrRefLocal, a plain useState setter (:67).
flushSync, useSyncExternalStore and unstable_batchedUpdates appear zero times in the host's first-party source. The only flushSync in the dependency tree is Lexical's own useDecorators, which fires at the end of a commit rather than re-entrantly inside onScrRefChange. (setScrRefSync in scroll-group.service-host.ts is a red herring - "Sync" there means "not Promise-returning", nothing to do with React.) The feedback loop back into React is useEvent, a plain useEffect subscription, not useSyncExternalStore. The other three consumers (resource-text-panel, model-text-panel, enhanced-resource) go through the same hook.
This finding requires a re-entrant passive-effect flush inside an open Lexical update (_updating === true), and nothing in the current host produces one. So it is latent, not live - please don't spend time on it. It remains an argument for not using editor.read() here (the API gives you no guard if a host ever introduces a synchronous commit), but it is not a bug you can hit today.
This does not affect findings 1 and 3 on this line. Those need only _pendingEditorState !== null with _updating === false - i.e. after the stack has unwound - which the plain-setState host does produce: onScrRefChange fires from a selectionchange-driven Lexical command listener, React treats that as discrete/sync-lane and schedules its flush as a microtask, and that microtask can run before Lexical's own deferred commit microtask. Both still stand.
Sorry for putting an open question in front of you that was mine to answer.
There was a problem hiding this comment.
Resolved by the revert in 36bceac - editor.read() is gone from the file, and the header's UNSAFE list now bans it. Agreed it's latent given the host's plain useState setter chain; the revert removes the exposure either way.
| machine.phase = "navigating"; | ||
| machine.pendingEchoes.length = 0; | ||
| // Prop-driven placement gate: never move the caret inside a different book's (stale) document. | ||
| const bookCode = editor.read(() => $getFirstBookNode()?.getCode() || undefined); |
There was a problem hiding this comment.
[3/18 - PLAUSIBLE] Gate inversion: the flush commits the pending document, then reads it - defeating invariant I6.
The comment right above this line states the purpose: "never move the caret inside a different book's (stale) document", and I6 says "prop-driven placement never moves the caret in a different book's document". That question can only be answered by the currently loaded document - which is what getEditorState().read() returned.
editor.read() commits the pending swap first, then reads it. Demonstrated against real Lexical 0.43.0, replicating LoadStatePlugin's queueMicrotask(() => editor.update(() => editor.setEditorState(...))):
_pendingEditorState !== null : true _updating: false
OLD editor.getEditorState().read() -> "GEN"
NEW editor.read() -> "EXO"
GATE INPUT CHANGED BY THE PR : true
So the gate on line 271 passes where it previously (correctly) failed, and a prop-driven placement runs inside a document the user cannot see yet. The gate also stops being a pure predicate: evaluating it installs the document it then approves.
The read is also demonstrably reachable with a live pending state and a plain host (no flushSync): Lexical dispatches SELECTION_CHANGE_COMMAND synchronously inside the DOM selectionchange event, which React 18 treats as DiscreteEventPriority -> SyncLane, and a SyncLane commit flushes its passive effects synchronously (react-dom.development.js:26973). I measured 3 hits of this line with _pendingEditorState !== null. What I did not reproduce end-to-end is that window coinciding with a cross-book swap - hence PLAUSIBLE rather than CONFIRMED.
No test pins this: all 37 stay green either way.
There was a problem hiding this comment.
Fixed in 36bceac - the gate reads the committed snapshot again (getCommittedBookCode), so evaluating it can no longer install the document it then approves.
| // Prop-driven placement gate: never move the caret inside a different book's (stale) document. | ||
| const bookCode = editor.read(() => $getFirstBookNode()?.getCode() || undefined); | ||
| if (!bookCode || bookCode === newRef.book) { | ||
| editor.update(() => $moveCaretToVerseStart(newRef.chapterNum, newRef.verseNum), { |
There was a problem hiding this comment.
[5/18 - CONFIRMED] Side effect of the line-270 flush: it splits one commit into two.
Without the flush, this editor.update() merges into the still-pending document state ($beginUpdate sees pending non-null and not _readOnly, so editorStateWasCloned = false) and the document and the caret move commit together.
With the flush, the document commits first - firing every update/mutation/decorator listener with the caret not yet placed - and the caret move then commits separately. So DeltaOnChangePlugin, HistoryPlugin and the annotation plugins all observe an intermediate state that previously never existed.
This resolves itself if line 270 goes back to getEditorState().read(...).
There was a problem hiding this comment.
Fixed in 36bceac - with the flush gone, the caret move merges into the still-pending document state again, so the document and caret commit together.
| const code = editor | ||
| .getEditorState() | ||
| .read(() => $getFirstBookNode()?.getCode() || undefined); | ||
| const code = editor.read(() => $getFirstBookNode()?.getCode() || undefined); |
There was a problem hiding this comment.
[4/18 - CONFIRMED] At this site the swap provably buys nothing, and adds a re-entrancy.
On the normal path this listener runs from inside $commitPendingUpdates, which nulls _pendingEditorState at Lexical.dev.mjs:8504 and installs the new state at :8505 - 100 lines before it fires mutation listeners at :8605. So editor.read()'s $commitPendingUpdates hits the early return at :8486-8488, and getEditorState() already holds exactly the state it would have returned. The value read cannot differ. Zero benefit.
The risk is not zero, though. This listener explicitly opts into { skipInitialization: false }, and Lexical's registerMutationListener inserts the listener into _listeners.mutation (:10648) before calling initializeMutationListener (:10653), which invokes it synchronously from inside the useEffect - not inside a commit, so _pendingEditorState is not nulled there. If a pending update exists at that moment, this editor.read() runs a full commit whose triggerMutationListeners iterates Array.from(editor._listeners.mutation) (:8646) - which already contains this listener - and re-enters it. onDocumentChanged then runs twice for one document arrival, mutating machine.phase / sawDocument / pendingEchoes under itself.
The pre-PR getEditorState().read() could not do this. Note the file's own header calls the skipInitialization replay out as the source of the NUM 27:1 bug.
Fix: revert to editor.getEditorState().read(...) - which is also what AnnotationPlugin.tsx:85 and CommentPlugin.tsx:862 still use.
There was a problem hiding this comment.
Fixed in 36bceac - reverted via the shared getCommittedBookCode helper, which documents why editor.read() is off-limits here (including the skipInitialization replay re-entry).
| * @returns null, i.e. no DOM elements. | ||
| */ | ||
| export default function ScriptureReferencePlugin({ | ||
| export function ScriptureReferencePlugin({ |
There was a problem hiding this comment.
[21/18 - cleanup] The export default -> named export migration is half-applied, and it creates a name collision.
Half-applied: TreeViewPlugin is still export default and is imported on the line directly below this one's import in Editor.tsx (import TreeViewPlugin from "./TreeViewPlugin";, line 9). CommentPlugin and Editor itself are also still default exports. A convention applied to one of four plugins in a directory is a diff, not a convention - either finish it or leave it.
Collision: libs/shared-react/src/plugins/ScriptureReferencePlugin.tsx exports a different plugin under the same name (props {book?, onChangeReference?}, both optional), re-exported from the shared-react barrel that Editor.tsx already imports ~25 names from. Two identically-named exports now sit in Editor.tsx's import scope; IDE auto-import will offer both, and because both of the other plugin's props are optional, <ScriptureReferencePlugin /> from the wrong module type-checks silently and yields a no-op plugin. The default export was the only thing preventing this.
Not a blocker - just worth knowing the migration bought a footgun that didn't exist before.
There was a problem hiding this comment.
Leaving as is for this PR. Scribe's ScriptureReferencePlugin is already a named export under the same name, so named exports are the established direction here. Finishing the migration (TreeViewPlugin, CommentPlugin, Editor) and dealing with the legacy shared-react plugin (whose only consumer is the perf-react demo) belongs in a follow-up.
|
|
||
| /** isVerseInRange throws on malformed ranges (reversed, 3-part) from imported USFM; a malformed | ||
| * range simply doesn't contain the verse. */ | ||
| function verseInRangeSafe(verseNum: number, verseRange: string): boolean { |
There was a problem hiding this comment.
[22/18 - cleanup/reuse] One root cause, guarded twice in this file.
isVerseInRange (libs/shared/src/nodes/usj/node.utils.ts) throws on malformed ranges. This file catches that single throw in two different ways: this verseInRangeSafe wrapper (used at lines 378 and 427), and a separate ad-hoc try/catch around $findVerseOrPara at lines 391-396 - which only throws because $findVerseInNode (libs/shared-react/src/nodes/usj/node-react.utils.ts) calls the same unguarded isVerseInRange. The comment above that try/catch admits as much.
So the guard is at the wrong altitude: two mechanisms here for one root cause, and $findVerseOrPara / $findVerseInNode stay throw-prone for every other consumer of the shared-react barrel.
Better: export an isVerseInRangeSafe sibling next to isVerseInRange in shared, have $findVerseInNode use it, then delete both this wrapper and the try/catch at 391-396. (Don't change isVerseInRange itself - its throw is pinned by node-utils.test.ts.)
Out of scope for a cleanup PR if you'd rather not - but this PR is the cleanup PR.
There was a problem hiding this comment.
Agreed on the direction, but moving the guard into shared/shared-react changes malformed-range behavior for scribe and the demos too - that deserves its own PR and tests rather than riding on this one. Leaving both local guards for now.
| } | ||
|
|
||
| /** First BookNode of the root (documents put it first; strays after content are ignored). */ | ||
| function $getFirstBookNode(): BookNode | undefined { |
There was a problem hiding this comment.
[23/18 - cleanup/reuse] This hand-rolled sibling walk is $getRoot().getChildren().find($isBookNode).
Six lines of getFirstChild() / getNextSibling() for what the codebase already expresses as a one-line find with a type guard - which is the established idiom here: $getFirstPara (node-react.utils.ts), $findChapter and $findNextChapter (node.utils.ts) are all written that way, and this plugin's own test file already writes exactly $getRoot().getChildren().find($isBookNode) (ScriptureReferencePlugin.test.tsx:471).
$moveCaretToVerseStart in this same file already calls $getRoot().getChildren() (line 382), so the file now carries two different root-scan shapes for no behavioral gain.
|
|
||
| // The unaddressable check needs the NODE's existence - a BookNode with an empty code still | ||
| // makes a document addressable. | ||
| const bookNode = $getFirstBookNode(); |
There was a problem hiding this comment.
[24/18 - cleanup/efficiency] $resolvePosition re-walks the root children on every selection change.
This runs on every idle selection change: every caret move, every typed character, every click, continuously during a mouse drag, plus the synthetic dispatch this plugin's own verse listener fires on every verse created/destroyed batch. Each call re-derives the first BookNode by walking root children.
In a well-formed document the BookNode is the first child, so it's cheap. But for the book-less document this file explicitly supports (the I1 prop-book fallback), the loop visits every root child before returning undefined - one per para, ~40 for a typical chapter, 190+ for a Psalm-119-shaped one - on every caret move.
The value is invariant between BookNode mutations, and registerMutationListener(BookNode, ...) on line 196 is already the perfect invalidation hook - it computes exactly this value and throws it away. Caching it on Machine would delete the walk from the hot path entirely. (Would need invalidating on the destroy-only early return at line 198 too.)
Low priority - flagging it because the mechanism to fix it already exists 100 lines up.
There was a problem hiding this comment.
Declining, with context the repo doesn't show: the host splits USJ into chapters and loads one at a time, and only chapter 1's document carries a BookNode - so the full-walk miss is actually the common case here, not the book-less edge case. It stays bounded by paras-per-chapter (~40, ~190 for Psalm 119), i.e. microseconds per selection event. And the proposed Machine cache can't invalidate on chapter N -> N+1 swaps, which create/destroy no BookNode and fire no mutation at all - a stale-state hazard in exactly the file that exists to contain them.
| }); | ||
| } | ||
|
|
||
| function $moveCaretToVerseStart(chapterNum: number, verseNum: number) { |
There was a problem hiding this comment.
[25/18 - cleanup] The only helper in the file with no doc comment - and its first branch contradicts its name.
Every other helper here carries a /** */ (report, consumePendingEcho, $resolvePosition, positionMatchesScrRef, verseInRangeSafe, ...). This one has none, and lines 376-380 deliberately do not move the caret when it already sits inside a verse range containing verseNum (pinned by "should not move the cursor if already in range") - i.e. the function's first act is the opposite of what $moveCaretToVerseStart promises, with no explanation.
The other three no-op exits (no chapter, $findVerseOrPara throws, no target found) are unsurprising, so the name is fine - the gap is just the one-liner. Since this PR renamed this exact signature line, it seems like the moment to add it:
/** Moves the caret to the start of `verseNum` in `chapterNum`. No-op when the caret is already
* inside a verse range containing `verseNum` (a range is one location), or the target is absent. */There was a problem hiding this comment.
Added in c8459c7, essentially verbatim.
|
The previous comments were directly posted after a
I'm double checking there aren't new, meaningful issues it says were latent. So far, I haven't found any. It seems like Claude is most unhappy with the changes to remove |
Four pre-existing latent issues in
|
Code review demonstrated the two are not synonyms: editor.read() runs $commitPendingUpdates first - a full synchronous commit with no _updating guard. At the onPropChanged site that flush installs a pending document swap and then approves it, inverting the I6 stale-book gate, splits one navigation into two commits, and triggers React's "flushSync was called from inside a lifecycle method" warning via useDecorators. At the mutation-listener site the flush is provably value-neutral on the normal path but re-enters the listener from the skipInitialization replay. Both sites now go through a getCommittedBookCode helper that documents the committed-snapshot requirement, and the header's UNSAFE list gains an entry for editor.read() since no test pins the hazard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
L1: the |
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
@irahopkinson made 3 comments and resolved 2 discussions.
Reviewable status: 2 of 3 files reviewed, 18 unresolved discussions (waiting on katherinejensen00 and lyonsil).
packages/platform/src/editor/ScriptureReferencePlugin.tsx line 340 at r1 (raw file):
Previously, katherinejensen00 wrote…
NIT Claude Finding
The
(a)–(d)cross-references inonDocumentChangedare used in prose but never anchored to their code branches.The REPLACEMENT branch says "The correction below (d), if any, runs after." The book correction block says "Runs after (a)–(c); under (a) phase is already 'navigating' here." But no comment in the code is actually labeled
// (a),// (b), etc. — a reader has to manually count branches to decode the letter scheme. If (a) is REPLACEMENT, (b) is MOUNT, (c) is the paste/undo no-op, and (d) is the book correction block, that can be inferred, but it takes real effort. This predates your PR, so it doesn't need to block merge — but a one-liner label at the start of each branch (// (a) REPLACEMENT: ..., etc.) would make the cross-references self-evident.
Done.
packages/platform/src/editor/ScriptureReferencePlugin.tsx line 375 at r1 (raw file):
Previously, katherinejensen00 wrote…
NIT Claude Finding
$moveCaretToVerseStartis the only named handler in the file without a JSDoc, and it's the most complex one.Every other handler in the file —
onPropChanged,consumePendingEcho,$resolvePosition,onDocumentChanged,schedulePlacingCaretAtVerseStart,onSelectionSettled,positionMatchesScrRef,verseInRangeSafe,positionToScrRef,report,refsEqual,onUserInteracted— has a function-level JSDoc comment.$moveCaretToVerseStartdoesn't, and it's the function with the most state assumptions: it must be called inside a Lexical editor-state update, it has an early-return when the caret is already in a verse range, and it handles the "no verse node, fall back to para" case. The file-top HOW-THIS-PLUGIN-WORKS block explains the overall algorithm but doesn't substitute for a function signature–adjacent description that callers read in context. Since this PR renamed the function and moved it, this is a good moment to close that gap with even two or three sentences.
Done.
| } | ||
|
|
||
| /** First BookNode of the root (documents put it first; strays after content are ignored). */ | ||
| function $getFirstBookNode(): BookNode | undefined { |
- rename onDocumentChanged's bookCode param and the listener local to match getCommittedBookCode (finishes the contentBook -> bookCode rename) - warn at the SELECTION_CHANGE_COMMAND call site that $resolvePosition must stay inline, now that onSelectionSettled's doc lives far below - move MAX_PENDING_ECHOES next to report(), its only consumer, and give it a doc comment - label onDocumentChanged's branches (a)-(d) so the existing cross-references resolve - capture isFirstDocument before the single sawDocument write so the read-before-write ordering is explicit instead of emergent - replace the hand-rolled sibling walk in $getFirstBookNode with the getChildren().find() idiom used elsewhere - document $moveCaretToVerseStart's already-in-range no-op - alphabetize imports Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lyonsil
left a comment
lyonsil
left a comment
There was a problem hiding this comment.
I included L3 and L4 out of completeness, but I don't think either is strictly needed. They both require the plug-in and editor to do things that neither does today. Fixing L4 is also potentially part of a larger refactor to slightly simplify the state machine further that I previously chose to defer given time constraints and complexity.
@lyonsil reviewed 3 files and all commit messages, made 1 comment, and resolved 18 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved.
3 participants
Code Review Summary
Branch: cleanup
Base: origin/main
Date: 2026-07-14
Review model: Claude Sonnet 5
Files changed: 3
Overview
This branch is a cleanup pass following PR #499 ("rebuild ScriptureReferencePlugin as a navigation state machine"), touching only
packages/platform/src/editor/Editor.tsx,ScriptureReferencePlugin.tsx, andScriptureReferencePlugin.test.tsx. It reorganizes helper functions to sit below the main plugin export (matching the convention used elsewhere in the package), renames a couple of helpers for clarity ($moveCursorToVerseStart→$moveCaretToVerseStart,schedulePlacement→schedulePlacingCaretAtVerseStart), switches the plugin from adefaultexport to a named export, and replaceseditor.getEditorState().read()witheditor.read()at the two sites where that's the correct pattern. It also restores a chunk of pre-PR-#499 logic in$moveCaretToVerseStart, described in the commit message as restoring "affordance for more than one chapter in USJ."That last item turned out to be the one substantive discussion point: three independent analysis passes, followed by a detailed manual trace (including reading
$findNextChapter's actual implementation and diffing all three historical versions of the function — pre-#499, PR #499, and this cleanup), confirmed the "restored" guard was behaviorally identical to the PR #499 code it replaced — it didn't deliver the stated affordance, just reordered computation. The author agreed and had this hunk reverted to the PR #499 form during the review (see Findings). A genuine but unrelated pre-existing edge-case bug in$findNextChapterwas surfaced along the way (see Suggested Review Focus) but is not part of this diff and was intentionally left alone.API Changes
None.
ScriptureReferencePlugin.tsxis an internal editor-implementation file, not re-exported throughpackages/platform/src/index.ts, and is not tracked by the package'sapi-extractorreport (packages/platform/etc/platform-editor.api.md), which has no diff versus the merge base.Editor.tsxonly changed one import line to match the new named export. No changes touchlibs/shared/,libs/shared-react/,packages/utilities/, or any package's public surface.Findings
Critical — Must address before merge
None.
Important — Should address before merge
$moveCaretToVerseStart(lines 375–389) didn't deliver its stated purpose — it was logically/behaviorally equivalent to the simpler PR platform: rebuild ScriptureReferencePlugin as a navigation state machine #499 form it replaced, confirmed via truth-table analysis and a three-way diff of pre-platform: rebuild ScriptureReferencePlugin as a navigation state machine #499/PR-platform: rebuild ScriptureReferencePlugin as a navigation state machine #499/cleanup versions of the function, including tracing$findNextChapter'sisCurrentChapterAtFirstNodeargument through both branches. (fixed during review: reverted to the PR platform: rebuild ScriptureReferencePlugin as a navigation state machine #499 form — earlyif (!chapterNode) return;followed by a hardcodedtrueargument to$findNextChapter, removing the redundant combined condition and the wasted pre-guard computation.)$moveCursorToVerseStartin the top-of-file "HOW THIS PLUGIN WORKS" doc comment (ScriptureReferencePlugin.tsx, under "SAFE (additive) changes") and in a comment inScriptureReferencePlugin.test.tsx, left over after the rename to$moveCaretToVerseStartelsewhere in this same diff. (fixed during review: both updated to$moveCaretToVerseStart.)Author confirmed the guard restoration was "probably more restore to match old code" than an intentional behavior fix, and asked a substantive follow-up question about
$findNextChapter's second argument before agreeing to revert it — see Interview Notes.Minor — Consider
None.
Template Propagation
Shared Regions Modified
None. No
#region shared with <url>markers in any changed file.Extension Config Changes
Not applicable — this repository has no
extensions/-directory/template-propagation convention.Positive Observations
ArrowNavigationPlugin.tsx,NoteNodePlugin.tsx,ParaMarkerPrefixGuardPlugin.tsx,ActiveTextPlugin.tsx,TreeViewPlugin.tsx).default export→ namedexport function ScriptureReferencePluginmatches how other plugins in this codebase are exported/consumed; confirmed no other file imports the old default export.editor.read(...)replacingeditor.getEditorState().read(...)was applied at exactly the two correct sites, and the one site that must not use this pattern ($resolvePosition, inline in theSELECTION_CHANGE_COMMANDhandler) was correctly left alone, with its explanatory comment about why intact.schedulePlacement→schedulePlacingCaretAtVerseStart,$moveCursorToVerseStart→$moveCaretToVerseStart, "window" → "navigation window" in prose) were applied consistently everywhere except the two stale spots noted above (now fixed).ScriptureReferencePlugin.test.tsxpass unchanged;nx lint/nx typecheckclean for the package — confirmed both before and after the in-review fixes.Interview Notes
Author's stated purpose (from the branch's commit message): "cleanup after PR #499" — reorg, doc clarity, renames, named export,
editor.read()adoption, and restoring the multi-chapter guard.The one substantive discussion was around the restored guard in
$moveCaretToVerseStart. When asked whether it fixed a real observed bug or was a "restore to match old code" move, the author said it was probably the latter. When told the guard reorder appeared to be behaviorally inert, the author pushed back with a sharp, specific technical question — whether moving the return check to after computingnextChapterNodecould matter given what$findNextChapter's second argument does. That was a good challenge and led to tracing the actual implementations ofremoveNodesBeforeNodeand$findNextChapter, and diffing all three historical versions of the function (pre-#499 commit6bb03956, PR #499 commitb315cac9, and this cleanup). That trace confirmed no behavioral difference in either branch of the guard, but surfaced a real, pre-existing edge case: when the current chapter has zero verses (immediately followed by another chapter node),$findNextChapter's hardcodedisCurrentChapterAtFirstNode=trueskips index 0 of the sliced node array and misses that next chapter node, causing verse lookup to leak into the following chapter's content. This bug exists identically in both PR #499 and pre-#499 code — it predates this diff and is unrelated to it, so it was intentionally not fixed here (see Suggested Review Focus for a follow-up).Based on this, the author agreed to revert the guard hunk to the PR #499 form, and separately asked for the two stale
$moveCursorToVerseStartreferences to be fixed. Both were done during the review (see Findings).No unresolved items — the author engaged directly and substantively with the one non-trivial technical question raised, rather than deferring or expressing uncertainty. No areas where the author did not understand their own changes.
Separately: the branch's commit was amended and pushed to
origin/cleanupmid-review by the author directly (not by any action taken during this review) — confirmed by the author and by the subagent that ran quality checks, which reported it took nogit commit/git commit --amend/git pushactions of its own.In-Review Quality Check
After the two in-review fixes (guard revert, stale-name fixes), a subagent ran the package's checks against
@eten-tech-foundation/platform-editor(the Nx project forpackages/platform):nx typecheck) — passed, no errors.nx lint) — passed clean, no auto-fix needed.prettier --writeon the two edited files) — both already correctly formatted, no changes.nx test) — 148 passed, 3 skipped (unrelated data-generation tests), 0 failed. All 37 tests inScriptureReferencePlugin.test.tsxpassed.No unfixable failures. No issues found that need the author's attention.
Suggested Review Focus
$findNextChapter(libs/shared/src/nodes/usj/node.utils.ts), a current chapter with zero verses immediately followed by another chapter node causes the hardcodedisCurrentChapterAtFirstNode=trueskip-index-0 behavior to miss that next chapter node, letting verse lookup leak into the following chapter's content. Exists identically in both PR platform: rebuild ScriptureReferencePlugin as a navigation state machine #499 and pre-platform: rebuild ScriptureReferencePlugin as a navigation state machine #499 code. Worth a follow-up ticket/test if empty chapters are a real scenario in production USJ documents.AI-assisted — session
This change is