PT-4370: Settled positions across the public position API - #11
Draft
tjcouch-sil wants to merge 33 commits into
Draft
tjcouch-sil wants to merge 33 commits into
tjcouch-sil wants to merge 33 commits into
Conversation
…ates In editable marker mode a char span renders as `[MarkerNode "\nd", TextNode "<NBSP>LORD", MarkerNode "\nd*"]`: the display separator between the opening glyph and its content is a PREFIX of the span's first plain text child. That byte is presentation the editor->USJ conversion strips, so it occupies no USJ offset - but the shared logical content model counted it, putting every USJ text offset inside a char marker off by one in both directions. A programmatic annotation at settled offset 2 of `LORD` landed between `O` and `R`, and a caret between `O` and `R` reported offset 3. The existing round-trip tests could not see it: they go USJ -> Lexical -> USJ, and a shift applied consistently in both directions cancels out. `$charSeparatorPrefixLength` (markerSeparators.utils.ts, beside the rest of the separator convention) answers how many of a text node's leading characters are the separator, shape-based on glyph adjacency rather than view options, which `libs/shared` cannot see. `LogicalTextSegment` gains `lead`, and the model's coordinate helpers map across it: a local offset inside the lead reports the start of the content after it, and a USJ offset resolves to a live point past it. The new oracle suite pins the model directly against the exporter - the exported USJ string is the oracle, and every offset must select the same tail of it that the live text selects - over 2 Samuel 1-2 in all three marker modes and Standard view, in both directions. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
Four node classes the editor→USJ conversion emits nothing for were still counted as logical content items, so USJ content INDEXES disagreed with getUsj() inside notes, chapters, and around display runs — a settled position addressed the wrong item there: - AttributeRunNode display-run wrappers (\va/\vp, \ca/\cp, milestone runs, a note's \cat run), whose subtree the conversion skips wholesale; - a collapsed note's ImmutableNoteCallerNode caller decorator, recognized through a type-name constant now declared once in libs/shared (the class returns it from getType()), since libs/shared may not import shared-react; - an expanded note's editable caller TextNode, anchored to the caller SLOT the way $buildNoteFragment reads it, so note content spelling the caller is still content; - everything an editable ChapterNode displays: a USJ chapter marker carries no content, so the \c N glyph and its runs occupy no position. The exporter-oracle property test now runs with no exclusions in all four marker modes; its $isSkippedByTheExporter predicate is deleted. Also: $charSeparatorPrefixLength now finds the opening glyph when an annotation started on it (a host annotating from a marker location puts the glyph inside the text's mark, or in a mark of its own), and the oracle reuses markerEdit.test-helpers' serializedState, which takes view options. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
Standard view renders USFM bytes as real nodes — marker glyphs (including nested `\+nd`), attribute display runs, `\va`/`\vp`/`\ca`/`\cp`/`\cat` attribute markers, milestone runs, the optbreak `//` token, a chapter's and a verse's own glyph text, a note's caller, a book's `\id` glyph, and an opaque block's read-only bytes. `$getNodeFromLocation` and `$getLocationFromNode` now map every one of those bytes exactly, in both directions, instead of approximating attribute markers at the end of an element's content. Both functions describe a node's display text as ordered byte spans, each naming the USJ location its bytes carry, so outbound (snap LEFT into the last span that starts at or before the offset) and inbound (the first carrier whose span covers the wanted offset) are derived from one description per node kind. Property paths are emitted in bracket notation, the spelling `UsjReaderWriter` uses and the only one that survives an attribute name such as `x-custom-attribute-1`. A point inside read-only display bytes normalizes to the element boundary beside them, and that boundary now reports what those bytes are: a decorator holds no text, so Lexical rejects a selection point inside one. Over the 2SA corpus this takes the data-driven gap lists from 279 entries to 104: editable mode 124 -> 17, visible 84 -> 49, hidden 70 -> 38, with zero regressions. Every survivor's reason is documented at the gap set. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
…clauses Match paranext-core's platform-bible-utils copy of these types so a settled-document jsonPath produced anywhere up to the deepest USJ shape the editors render (table cell -> char -> nested char -> text) type-checks in both repos. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
…sition API Every position-bearing member of EditorRef/EditorProps now documents that its jsonPath addresses the SETTLED document (getUsj()) rather than the live on-screen tree, that translation between the two is automatic while a marker edit is pending, and that a USFM byte with no USJ representation snaps left to its nearest representable location. No behavior change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
PropertyJsonPath's `$.${string}` catch-all member matches any string
starting with `$.` regardless of depth, so the deep-path assertion on it
never actually guards the eight-clause depth bound — only the
ContentJsonPath assertion gives a genuine RED->GREEN signal for that.
Document this in place, mirroring paranext-core's copy of this test, so the
assertion's limited coverage is visible at the call site rather than
assumed from the ContentJsonPath assertion beside it.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
Split `$selectAtFragmentByteAnchor` into a pure `$resolveFragmentByteAnchor`, which answers "which node and offset holds the byte this anchor names" as a `FragmentPoint`, and the selecting wrapper that places the caret there. The two "after the closing glyph" / "after the sentinel run" fallbacks become element points on the parent, which the wrapper turns back into the same `selectNext` call it made before. Factor `$settledUsj`'s scope discovery into `$collectSettleScopes`, its note pass into `$applySettledNoteScope`, its husk splice into `spliceHusk`, and the transient cut's byte range into `$transientCutRange`, and rename the per-scope rebuilds to `$settledParaScope` / `$settledNoteScope` / `$settledChapterScope`. Export them, along with the fragment/serialization seams a second consumer needs, so the settled-position translation reuses this implementation rather than re-deriving where the settled document differs from the live one. No behavior change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
…document A host's only view of the document is `getUsj()`, which is settled, so every position it hands back to `setSelection`, `setAnnotation` or `insertNote` is settled-derived — and those all resolve against the LIVE tree. While anything is pending the two documents differ: a paragraph that split pushes every later index down, a rejoin pulls them up, and a declared transient run is bytes the settled document does not have at all. The path either fails to resolve or, when the shift is small enough, resolves onto the wrong paragraph. Carry the position across first. Outside a rebuilt scope that is one top-level index restated. Inside one, the settled and live structures genuinely disagree and all they still share is the bytes on screen, so the settled location is resolved against the scope's settled tree — materialized in a scratch editor — reduced to the Tier-2 caret's whitespace-tolerant byte anchor there, and re-resolved against the live fragment. Nodes the settle preserves verbatim cross by their own path instead, and a location naming USFM bytes rather than USJ content is resolved with byte addressing so a closing glyph's own bytes stay reachable. Identity when nothing is pending and nothing is declared, and each scope's plan is memoized on its content so a run of position calls against one pending state settles it once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
…ive locations Review round 1. The paragraph SCOPE fragment join existed in three places — `$rebuildParas`, `$settledParaScope` and the position basis — each a byte-for-byte copy of the others. The claim that a scope's live and settled fragments are comparable holds only while all three stay identical, and nothing enforced it, so the join moves to `$buildParaScopeFragment` in tier2Rebuild.utils.ts and all three call it. The inbound rows resolved through `$livePointFromSettledLocation`, which no production path uses: `setSelection` / `setAnnotation` / `insertNote` go through `$liveSelectionFromSettled`, whose output is a LIVE `UsjDocumentLocation`. The failure-mode rows and every subtype row now assert that location too — its subtype included, which is what byte addressing buys and what caret addressing would silently drop. Also: the failure-mode-2 row for the space the cut collapses (one settled space stands for two live ones, and a position with more than one byte to choose from snaps left); the cache's invalidation and its stillPending sweep, which turned up a real leak — the identity fast path returned before the sweep, so the last plan's scratch editor and live node references outlived their pend; a stale plan's detached sentinel node now refuses rather than invarianting; the plan signature carries the transient declaration's node key; and the API test states why it annotates outside the pending paragraph. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
`getSelection` and `onSelectionChange` reported positions against the live Lexical tree, while `getUsj()` hands a host the settled document — so while anything was pending the host resolved a path against a document it did not come from, either failing outright or, worse, landing on the wrong content. Adds the outbound half of the translation module: a live point is carried into its settle scope's rebuild through the same whitespace-tolerant byte anchor the inbound direction uses, with preserved node runs crossing by their own path, and the result's top-level index restated for how many settled items each preceding pending scope becomes. `OnSelectionChangePlugin` fires inside the uncommitted update that moved the selection, where preparing a scope (which creates nodes) must not run and the committed state is still the pre-move one, so a report that needs translating is deferred past the commit and coalesced to one per tick. A report that needs none keeps the plugin's own timing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
`EditorRefPlugin` assigns its ref in an effect with no cleanup, so `editorRef.current` still names this editor after the tree is gone and the deferred report's guard only ever caught a replacement instance. A discrete event that dispatched a selection change and then unmounted in the same task left the microtask to force-commit a detached editor and call back into a torn-down view. A mount flag an unmount effect actually clears makes the guard mean what it intended. Also: reuse shared-react's `$getJsonPathIndexes` instead of the copy of it the outbound translation carried; log the refusal path both selection readers share; state the ledger-vs-in-flight assumption behind the synchronous report and why `getSelection` flushes where the inbound entry points must not. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
Regenerate packages/platform/dist and packages/utilities/dist via pnpm rebuild-committed-output so the committed api-extractor rollups match the PT-4370 settled-position source changes landed on this branch. etc/*.api.md reports were already current (no diff). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
A preserved node run crosses between the live tree and a scope's settled rebuild by its INDEX in that fragment's run list, and the two lists are built by the same builder over different trees. A pended, emptied optbreak husk is spliced out of the settled output while the live tree still carries it as a sentinel, so the live list is one entry longer and every run past the husk is off by one. Where the run at that index happens to be shape-compatible — two notes in one paragraph is an ordinary document — the child-path walk succeeds and the position lands silently in the wrong note, which is the exact failure this translation exists to prevent. Both directions now refuse a scope whose two run lists cannot correspond. Refusal is a failure mode every caller already handles; recording an explicit live<->settled run correspondence when the plan is built is the complete fix. Also closes the chapter scope kind's test gap — it shipped implemented but unexercised in either direction. One inbound and one outbound row over a first-class \ca span beside its chapter pin the region's contraction (two top-level live items settling to one) and the chapter glyph's own number location. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
ATTRIBUTE_MARKER_KEYS conflated two namespaces: documented as keyed by a run's
marker name, indexed by DisplayRunKind in two places, and using the kind's own
length where the marker name's length was meant. It works only because every
kind registered today spells the same string in both. It is replaced by a
table keyed by kind, exhaustive over the registry, whose entries name the
marker and the USJ attribute explicitly, plus a name-keyed map derived from
it — so a future kind whose marker differs from its kind is a compile error
rather than a wrong key offset.
BYTE_CARRYING_RUN_KINDS was a hand-maintained subset of the registry's eleven
kinds with nothing pinning completeness. It is now derived from an exhaustive
record of which kinds carry bytes, so adding a kind forces the decision.
Four doc corrections alongside: FragmentAccumulator's TSDoc was orphaned above
FragmentPoint and left the exported interface undocumented;
buildSettledPositionContext's read as a prohibition that readSettledSelection
violates by design; $resolveFragmentByteAnchor's carried a backward-facing
phrase about an earlier walk; and PropertyJsonPath claimed a bound that its
own `$.${string}` member makes decorative.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
… code span
TSDoc has no double-backtick code spans, so ``` `` `$.${string}` `` ``` parses
as two empty ones and api-extractor warns twice on every run of the committed
dist rebuild. Name the catch-all in prose instead.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
The committed `dist/index.d.ts` for both published packages held tsc's per-file re-export stub rather than api-extractor's rollup: 47 and 13 lines ending in a source-map trailer, re-exporting from sibling declaration files that `.gitignore` keeps out of the tree and — for the editor — from `shared` and `shared-react`, private workspace packages that can never resolve outside this monorepo. Every consumer typechecking against that dist got unresolved modules, and CI's own rebuild-then-verify could not have matched it. Rebuilt with `pnpm rebuild-committed-output` as the last build action in the worktree, so nothing re-emitted over it: 1865 and 665 lines of `export declare` bodies carrying their `@public` tags, no source-map trailer, no private workspace specifier. `scripts/verify-committed-dist.mjs` is the gate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
37 tasks
A Tier-2 settle re-tokenizes a scope's displayed bytes and splices freshly built nodes in place of its children. A TypedMarkNode is transparent in the fragment — its text bytes belong to its children's spans, the wrapper contributes none of its own — so re-tokenization could never reproduce it and every annotation inside the scope being edited was wiped when it settled. Capture two fragment byte anchors per mark before the splice (the same mechanism that carries the caret), then re-wrap the annotations over whatever the rebuilt nodes spell at those positions. Applies to both paragraph scopes and expanded note content, which setAnnotation can reach by USJ location. The mark node is re-created rather than moved, which is what re-associates AnnotationPlugin's type:id -> node-key map; the host's callbacks ride along per type:id, and no removal callback fires for the old node (a spliced-away subtree is garbage-collected, never remove()d). The fixed-point signature now treats a mark as transparent too, mirroring the fragment builder: an annotated scope would otherwise compare unequal to its own re-tokenization forever and re-splice on every settle. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: 8c6d82a0-1eea-4a50-93e4-5e9bf0cad139
…rying a mark Two holes in the annotation carry. A comment mark's wrap collapses the document selection onto the mark it just built (the COMMENT_MARK_TYPE branch of $wrapSelectionInTypedMarkNode), and the caret restore that follows deliberately does nothing when the caret was parked outside the scope being rebuilt — so settling a paragraph the user had already left dragged the caret back into it. Snapshot the selection around the wrap loop and put it back, null included, leaving the caller's caret policy the only thing that moves it. A preserved node contributes one U+FFFC placeholder, and no byte position can name the boundary in front of it: both the anchor walk and the resolve read a position on the placeholder as being after the node it stands for. A mark that started ON a note therefore came back wrapping everything except the note. Keep the run's node key alongside the anchor and pull the run into the front of the mark after the wrap — by identity, which is exact because $replaceSentinels moves the original preserved nodes into the rebuilt tree. A selection boundary cannot do this: an element point in front of an inline element resolves to that element's first leaf, so wrapping from there marks the note's internals. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: 8c6d82a0-1eea-4a50-93e4-5e9bf0cad139
$insertNoteWithSelect built a note's content (the \fq/\xq quotation) from the caller-supplied selection but inserted the note itself wherever the editor's own caret happened to be: Lexical's insertNodes -> $removeTextAndSplitBlock re-fetches $getSelection() and prefers it over the RangeSelection it was called on, and a caller-supplied selection built by $getRangeFromUsjSelection is a detached object the editor was never told about. Installing it with $setSelection right after glyph normalization makes it the live selection Lexical's own machinery reads, so insertNodes and the caller now agree on where the note lands. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: 8c6d82a0-1eea-4a50-93e4-5e9bf0cad139
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: 8c6d82a0-1eea-4a50-93e4-5e9bf0cad139
A position inside a preserved node run crossed between the live tree and a settle scope's settled tree by the run's raw INDEX in its fragment's run list. The two lists are built by the same builder over different trees, so a construct that needs a preserved run on one side but not the other -- a dead optbreak husk the settle splices out, a char span whose sentinel condition the rebuild resolves -- shifted every run after it by one, and the index then named a different construct. Refusing the whole scope whenever the two lists differed in length made a husk anywhere in a pending paragraph refuse every position in it, and equal lengths was never a sufficient check anyway. Each scope plan now carries `sentinelMap`: where every live preserved-run member sits in the settled fragment's own run list, indexed `[live run][live member]`. It is built from the pairing the rebuild already performs -- `carriedPreservedRuns` (was `serializedRunsOf`) now returns the live members it kept alongside their serialized counterparts -- matched against the settled fragment's runs in document order. Both directions go through it (outbound reads it forwards, inbound backwards), and a position refuses only when its own member has no counterpart. A chapter scope has no preserved runs to pair: `$buildChapterFragment` refuses a region carrying any preserved node at all. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: 8c6d82a0-1eea-4a50-93e4-5e9bf0cad139
Mapping the two sides' preserved runs fixed how a position inside one of them crosses, but left the scope's byte anchors counting a byte the two documents do not share. `$resolveFragmentByteAnchor` counts a run's U+FFFC placeholder as an ordinary document byte -- it is deliberately not whitespace -- so a live fragment still spelling a run the settle dropped put every text position past it one non-whitespace byte away from the settled position that names it: one byte early inbound, one late outbound. A scope plan's live fragment now has the placeholder of every wholly dropped run cut out of it, the same way a declared transient run's bytes already are, so both sides spell the same document bytes and a text position past a dropped husk resolves exactly. The run's own span stays in the list, emptied, so a position inside the dropped node still resolves to that run and is refused there. `sentinelMap` is now `undefined` when the two sides' runs cannot be put in correspondence at all, and that refuses the whole scope -- byte anchors included, since there are then no shared byte coordinates to answer in. A scope with no preserved runs on either side is a vacuous correspondence, not an unknown one, so a chapter scope still answers. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: 8c6d82a0-1eea-4a50-93e4-5e9bf0cad139
The reverse adaptor dropped any note text that happened to equal a note's rendered editable-caller string, wherever in the note it sat, because the check compared BYTES rather than the specific node `$noteEditableCallerNode` anchors as the caller slot. Position-anchor the drop instead: identify the caller-slot child once per note (the serialized twin of `$noteEditableCallerNode`) and exclude only that node by identity, so coincidentally matching content elsewhere in the note - loose body text, a nested char span, a caller wrapped in an annotation mark - round-trips as data. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: 8c6d82a0-1eea-4a50-93e4-5e9bf0cad139
… carry Three review findings on the settle's annotation carry, plus the doc corrections they turned up. A note's `TypedMarkNode` is one of the note's own children, so `$rebuildNoteContent` removes it DIRECTLY - and a direct removal reports "destroyed" to the host even though `$restoreMarkByteRanges` re-wraps the mark immediately after. Suppress that notification the way every other re-creation of a mark node does. The paragraph rebuild needs nothing: marks always sit inside the replaced paragraph, and a detached subtree is garbage-collected without `remove()` running. A mark range whose two ends resolve to the same fragment position covers no bytes, and wrapping a collapsed selection splits a text node at one offset twice, which marks everything in FRONT of it. Refuse instead - a dropped mark beats a mark over the wrong bytes. `handleSelectionChange` took a report ticket only on the deferred path, so a synchronous report could not supersede a deferred one already queued in the same tick and the host heard the same location twice. Take the ticket on both paths. Both "never tells the host its annotation was destroyed" rows now assert positive controls first: `not.toHaveBeenCalled` is equally happy against a settle that never ran. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: ab15f9a8-d773-443c-b080-4e162cb30bb2
The last rebuild predates the seven src commits on this branch plus the review fixes above, so the committed bundle shipped pre-fix code to any consumer staging the editor out of this checkout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: ab15f9a8-d773-443c-b080-4e162cb30bb2
Lexical's own selectionchange listener drops the SELECTION_CHANGE_COMMAND dispatch when a caret move resolves to a point strictly inside a text run at both its anchor and its focus (shouldSkipSelectionChange in Lexical's core selection handling) — the reasoning being that such a move needs no further DOM reconciliation. setSelection places the caret by writing the Lexical model directly and relied entirely on that native, DOM-event-driven path to tell the host where it landed, so a caller placing a collapsed or ranged selection whose endpoints both sit inside a text node's interior never heard back through onSelectionChange at all. setSelection now dispatches SELECTION_CHANGE_COMMAND itself, from inside the same editor.update() that places the selection, but only when both resolved endpoints are interior text points. Lexical runs command listeners for the active editor inline, so this executes synchronously in the same update against the pending selection already set, and OnSelectionChangePlugin's listener sees it exactly as it would from the native path. A boundary or element-type endpoint is left alone, since Lexical's own listener never skips those and dispatching again would report the same placement twice. Reaches every view through the single EditorRef.setSelection implementation; no public API change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: 8c6d82a0-1eea-4a50-93e4-5e9bf0cad139
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: 8c6d82a0-1eea-4a50-93e4-5e9bf0cad139
… mark An expanded editable note renders its caller as a plain text node holding `getEditableCallerText(caller)`. That is display scaffolding — USJ carries the caller as the note's own `caller` attribute — so the exporter has to drop the node. Both halves of the slot finder (`$noteEditableCallerNode` live, `noteCallerSlotNode` serialized) required the child at the caller position to BE a plain text node, and a `TypedMarkNode` is an ElementNode. A comment or any other annotation placed on the caller therefore anchored nothing, the identity-based drop had nothing to drop, and the caller's display bytes were spliced into the note's saved content — coming back as a second, fabricated caller on the next load, and compounding on every save/load cycle after that. Look through annotation marks at the caller position on both sides, and thread the anchored caller slot into the exporter's mark recursion so the drop reaches a node nested inside one. Marks are already transparent to every other USJ-facing view of the tree (the exporter splices them, the logical content model splices them); the caller slot was the one place that was not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: 73a8b035-072d-4680-8d69-4a6118e63c4c
…scopes Four position defects in the settled-position translation, all of them living in refusal paths the suites never exercised. Adds the fixture that reaches them: a note settling inside its own settling paragraph, with an emptied optbreak husk in the paragraph AND in the note, so both documents disagree about the note's index and the note's own content indexes. - A note settling inside a settling paragraph reported at the note's LIVE index. `$settledScopePath` restated only the top-level index, so the note's index WITHIN its paragraph crossed unchanged — but the paragraph's rebuild re-indexes its content around the note. It now asks the enclosing scope where the note actually landed, through the same preserved-run correspondence every other position in that scope crosses by, and refuses when that cannot be determined. - A husk-only paragraph scope carried the PENDING note. `$planForHuskOnlyPara` omitted the notes pass `$planForParas` runs, so a settled path into the note was resolved against a paragraph holding the note's pre-settle children — a confident wrong live node, not a refusal. - `$livePointInPreservedRun` fell through to a child walk when a co-settling note had no byte correspondence to cross by, spelling a SETTLED child path against LIVE children. It refuses instead, which is what the comment above it already said the code did. - An element boundary immediately after a preserved node could never be anchored: the boundary anchors at the end of the span before it, and that span is the preserved node's one-byte sentinel, which `spanFor` refuses. The end of a sentinel is exactly what this boundary means, and `$caretSpanByteAnchor` spells it, so the boundary case anchors from the span it already has. A boundary in FRONT of a sentinel still refuses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: 73a8b035-072d-4680-8d69-4a6118e63c4c
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: 73a8b035-072d-4680-8d69-4a6118e63c4c
…n in settled offsets Standard view shows each space of a typed run of two or more as an NBSP, so the run stays visible, and serialization collapses the run to one space. The logical content model counted every displayed character, so after such a run `LogicalTextItem.length` exceeded the USJ string and every settled position past the run was off by the spaces the run lost — in both directions, with nothing pending, on the identity path that treats the live tree as settled. `setAnnotation`/`setSelection` landed that many characters early, and `getSelection`/`onSelectionChange` reported that many too high. - `spaceRuns.utils.ts` (shared) defines the collapse once, on display text: `collapsedSpaceRunRanges` for the model, `collapseSpaceRuns` for the serializer, which now uses it in place of `normalizeSpaceRuns`. - `LogicalTextSegment` records the run characters it drops (`collapsed`) and its own `length`; a live offset inside a dropped range reports the kept character after it, and a settled offset resolves to the live character it names — the rule the separator `lead` already followed. Each text node is measured on its own, as the serializer collapses each node before joining. - Collapse is view-dependent, so the position layer takes the editor's view options explicitly: `$getLocationFromNode`, `$getNodeFromLocation`, `$getRangeFromUsjSelection`, `$getUsjSelectionFromEditor`, and the model functions that measure text. `OnSelectionChangePlugin` and `AnnotationPlugin` gain a `viewOptions` prop; `PreparedScopes` carries the view it was prepared under. Index-only helpers read the item structure without measuring. - The exporter oracle gains a space-run corpus, checks each live point against the serializer's output for the text before it, and drops the waiver that let a collapsed item go unchecked. One test changes shape: the Tier-2 carry test that annotated a run's last two NBSPs did so through `setAnnotation` in live offsets. Those bytes have no settled address, so it now wraps the live range directly, as CommentPlugin does; the carry refusal it pins is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: 7a82c6b7-bc2c-4048-9099-88868317295b
The ratified screen-vs-file divergence for space runs does not extend to positions: the logical model drops a displayed run's extra spaces from the same shared definition the serializer collapses with, which makes the position layer take the editor's view options. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: 7a82c6b7-bc2c-4048-9099-88868317295b
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: 7a82c6b7-bc2c-4048-9099-88868317295b
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes every public position of the platform editor —
getSelection,onSelectionChange,setSelection,setAnnotation,insertNote— address the SETTLED document (whatgetUsj()returns), and fixes the Standard-view coordinate bugs that mis-placed programmatic annotations with nothing pending at all.Why review this
Part of the Standard-view epic (PT-4186). PT-4370 is its "settled positions" work item. A tester reported that programmatic annotations land in the wrong place "mostly related to text within markers"; the investigation found that bug is STATIC (below) and closed it together with the pending-state contract the work item was opened for.
Companion: paranext-core PR paranext/paranext-core#2823 (widens core's own copy of the jsonPath types, documents the contract, adds the e2e guard). Merge this PR first, then move
platform-yalc; core CI stays red untilplatform-yalccarries these commits.What changed
Static layer — the tester's bug (no pending state involved)
[\nd glyph, "⍽LORD", \nd* glyph]: the NBSP separator is a PREFIX of the first text child. The exporter strips it; the shared logical content model counted it, so every USJ text offset inside a char marker was off by one in both directions.LogicalTextSegmentnow carries aleadand the model matches the exporter (libs/shared).AttributeRunNodewrappers,ImmutableNoteCallerNode, an expanded note's editable caller text, and every child of an editableChapterNode. All skipped now.packages/platform/src/editor/positions/logicalModel.oracle.test.tsx) checks every offset of every text item in both directions against the real editor→USJ exporter overusj2Sa, in editable/visible/hidden modes and Standard view, with zero exclusions.$getNodeFromLocation/$getLocationFromNodemap every Standard-view glyph and display-run node exactly through one display-byte span model (nested+, optbreak//, char attribute runs,\va/\vp/\ca/\cpruns, milestone runs, note callers, chapter/verse own text). Bytes with no USJ representation snap LEFT (spec §4). The 2SA data-driven gap sets shrank 279 → 104 with zero regressions; Standard view has no resolution or round-trip gap left.Dynamic layer — settled ⇄ live translation while a marker edit is pending
packages/platform/src/editor/positions/: each pending settle scope's rebuilt nodes are materialized in a headless scratch Lexical editor and positions are carried across the re-tokenization with the engine's whitespace-tolerant fragment byte anchor, plus a top-level index correction for scopes that split/merge paragraphs. Identity fast path when nothing is pending. Every unresolvable case REFUSES (returnsundefined, logged) rather than mis-anchoring.setSelection,setAnnotation,insertNote) translate in agetEditorState().read()before their update and hand LIVE-coordinate ranges to the untouched resolvers. OutboundgetSelectiontranslates;onSelectionChangedefers past the commit (microtask, coalesced, cancelled on unmount) only when something is pending — synchronous otherwise.ContentJsonPath/PropertyJsonPathwidened to eight clauses (paranext-core's independent declaration widened identically).editor.model.tsdocuments the settled contract on every position-bearing member.Behavior changes hosts will see
$.content[1]['marker'], not$.content[1].marker); inbound accepts both. paranext-core's only jsonPath parser is notation-agnostic and its own writer already emitted brackets.+, second/,|,=,", inter-attribute space) are lossy by design and land on the documented representative.Fixed in this PR (follow-ups, 2026-09-15)
A Tier-2 settle discarded
TypedMarkNodeannotations inside the rebuilt paragraph or note content. Annotations are now carried across the settle by byte anchor (marks re-wrapped over the same content bytes; the fixed-point signature treats marks as transparent; a mark starting on a preserved node keeps it; the caret is not moved by the re-wrap; a note-content mark no longer fires the host's"destroyed"callback). Refuses rather than guesses when a mark's own bytes became a glyph or its anchors do not resolve. Standard view only. —01e61166,d3cd1e97,78636d94insertNote(marker, caller, selection)ignoredselectionfor WHERE the note goes.$insertNoteWithSelectnow makes the passed range the live selection before inserting, so a settled location is honoured for placement as well as content. All views (bug fix; no new behavior). —34a36fe4The Standard-view exporter stripped a leading NBSP from ANY char-span text child, positionally broader than the model's glyph-adjacent rule. It now mirrors
$charSeparatorPrefixLengthexactly (serialized twinprecedesOpeningCharGlyph). Note: the earlier "an authored~after a nested closer is eaten" framing did not reproduce — load and paste both convert an authored NBSP to a display~before any strip check — so this is an exporter↔model invariant alignment, not a data-loss fix. —2f646c68The exporter dropped note content that merely EQUALLED the caller string. It now drops only the caller-slot text node (serialized twin of
$noteEditableCallerNode). Every view that renders an editable caller. —1d640619The preserved-run crossing REFUSED the whole scope when the live and scratch sentinel lists diverged (a husk in a pending paragraph refused every position in it).
SettleScopePlannow carries an explicit live↔scratch run correspondence (sentinelMap), a settled-away run's placeholder byte is cut from the live fragment so byte anchors line up, and refusal is per position. Standard view only. —010e7e68,8c6458edEditorRef.setSelectiondid not report the selection it placed when both endpoints landed inside text nodes: Lexical marks its own DOM-selection update and then skipsSELECTION_CHANGE_COMMANDfor the resultingselectionchangewhen neither endpoint is at a text boundary, soonSelectionChange(and the host's cached selection) stayed stale.setSelectionnow reports explicitly, once. Every view. —d5a6098aCommitted dist rebuilt after the fixes above. —
caa7ff92Fixed in this PR (follow-ups, 2026-09-16)
$noteEditableCallerNodelive,noteCallerSlotNodeserialized) required the child at the caller position to BE a plain text node, and aTypedMarkNodeis an ElementNode — so a marked caller anchored nothing, the identity-based drop had nothing to drop, and the caller's display bytes were spliced into the note's content, returning as a second fabricated caller on the next load and compounding every cycle. Both finders now look through annotation marks, and the exporter threads the anchored slot into its mark recursion so the drop reaches a node nested inside one. Marks were already transparent to every other USJ-facing view of the tree; the caller slot was the one place that was not. Every view that renders an editable caller. —f6e32568settledPositions.nestedScopes.test.tsx): a note settling inside its own settling paragraph, with an emptied optbreak husk in the paragraph AND in the note, so the two documents disagree about the note's index and about the note's own content indexes. —0fd42572, dist3ec08665$settledScopePathrestated only the top-level index, but the paragraph's rebuild re-indexes its content around the note. It now asks the enclosing scope where the note landed, through the same preserved-run correspondence every other position in that scope crosses by, and refuses when that cannot be determined.$planForHuskOnlyParaomitted the notes pass$planForParasruns, so a settled path into the note resolved against a paragraph holding the note's pre-settle children — a confident wrong live node, not a refusal.$livePointInPreservedRunfell through to a child walk when a co-settling note had no byte correspondence, spelling a SETTLED child path against LIVE children. It refuses instead, which is what the comment above it already claimed.spanForrefuses. The end of a sentinel is exactly what this boundary means, so the boundary case now anchors from the span it already holds. A boundary in FRONT of a sentinel still refuses.Fixed in this PR (follow-ups, 2026-09-18)
setAnnotation/setSelectionlanded that many characters early andgetSelection/onSelectionChangereported that many too high. The model now drops a run's extra spaces exactly where serialization does, from one shared definition (libs/shared/src/nodes/usj/spaceRuns.utils.ts; the serializer'snormalizeSpaceRunsis replaced by it). A point inside a run reports the character after the kept space; a settled offset resolves to the live character it names. Collapse is view-dependent, so the position layer now takes the editor's view options explicitly ($getLocationFromNode,$getNodeFromLocation,$getRangeFromUsjSelection,$getUsjSelectionFromEditor, and the model's measuring functions;OnSelectionChangePlugin/AnnotationPlugingain aviewOptionsprop). The exporter oracle gains a space-run corpus in every mode and loses its waiver. Standard view only; the platform-editor's public API report is unchanged. —7793539a, docs25f99782, dist3ef682ecPre-existing bugs found on the way (NOT fixed here; ticket drafts in PT-4370)
FIXED in follow-up 10.LogicalTextItem.lengthis not the USJ string length in Standard view when the text carries a run of two or more spaces: that view's exporter also appliesnormalizeSpaceRuns, which is not length-preserving, so offsets after such a run are shifted. The exporter-oracle'scollapsedItems === []assertion passes only because the 2SA corpus has no space run; it is marked a deliberate waiver that goes red if such a corpus is added. Deliberately not fixed here: the logical content model and the whole$getLocationFromNode/$getNodeFromLocationlayer take noViewOptions, and the collapse cannot be detected from tree shape (an NBSP is a display space in Standard view and data everywhere else) — so a per-segment settled-offset map needs the view threaded through that layer's whole public surface, which is not a change to fold into this PR.return undefinedsites are still only reached by construction, not by a test.settledScopes.utils.tskeys a scope's plan on fragment text only, so asetAnnotationinside a pending scope (which splits text nodes) can hit a stale plan and hand back an out-of-range live point. Pre-existing; fold node keys into$planSignature.TypedMarkNodecallback registries andAnnotationPlugin's key map are never pruned for a mark that is spliced away rather than removed; the annotation carry now abandons one entry set per settle of an annotated scope (bounded by session length).packages/scribe's editor→USJ adaptor still has the byte-comparison caller drop the platform adaptor replaced, so the two adaptors disagree about a data-loss rule.Testing
shared699,shared-react1735 (+1 pre-existing skip),platform1807,scribe2,utilities59 — all green (2026-09-18, after follow-up 10);tier2Rebuild.corpus141 checked / 0 skip-listed; lint 0 errors; typecheck clean.<Editor>(deferred report ordering, coalescing, unmount cancellation, synchronous identity path); a refusal row for diverged sentinel lists; a nested-scope suite (note settling inside its own settling paragraph) covering both translation directions, the husk-only paragraph plan, an element boundary beside a preserved node, and a refusal when the byte correspondence is missing; mark-transparency rows for the note caller slot on both the live and the serialized side.packages/*/distrebuilt withpnpm rebuild-committed-output;node scripts/verify-committed-dist.mjs→ "Committed build output is current. ✓".node scripts/verify-consumer-lockfile-sync.mjs→ in sync.--pool=forks --maxWorkers=4.AI Involvement
AI-assisted — session; follow-ups: local session
8c6d82a0-1eea-4a50-93e4-5e9bf0cad139; review-paratext fixes: sessionab15f9a8-d773-443c-b080-4e162cb30bb2; space-run fix (follow-up 10): session7a82c6b7-bc2c-4048-9099-88868317295b. Plan, implementation, tests and reviews were generated by Claude subagents under a controller session; every task went through a spec/quality review and a whole-branch review with fix rounds; the human developer directed scope and decisions.Risk Level
Medium — the logical-model and
selection.utils.tschanges affect every annotation and selection in every view (covered by the exporter oracle and the 2SA data-driven suites with zero regressions); the translation module only runs while a marker edit is pending and refuses rather than guesses.🤖 Generated with Claude Code
https://claude.ai/code/session_012p1aLgN78tWXKFHgj6AhTu
This change is
Code Review Summary
Branch:
pt-4370-settled-positions(both repos)Base:
origin/main(paranext-core) ·paranext/main(scripture-editors)Date: 2026-09-15
Review model: Claude Opus 5
Files changed: 12 (paranext-core) + 41 (scripture-editors)
Overview
PT-4370 makes every public position of the platform editor —
getSelection,onSelectionChange,setSelection,setAnnotation,insertNote— address the SETTLED document(what
getUsj()returns) rather than the live Lexical tree, and fixes the Standard-view coordinatebugs that mis-placed programmatic annotations with nothing pending at all. The core side keeps its
independent copy of the jsonPath types in lock-step, records the ADR, adds an isolated e2e guard,
and fixes two
convertScriptureRangeToEditorRangeconversion bugs.This was an automated review: nine analysis agents across both repos (API/correctness, style,
compliance, UX, cross-repo contract, plus four scoped passes over the editor's logical model,
settled-positions module, editor wiring, and build/dist hygiene). Every finding below was verified
against the code before being acted on; findings that turned out not to hold are listed as
refuted.
API Changes
lib/platform-bible-utils:ContentJsonPathwidened 4 → 8.content[N]clauses (6 → 10 unionmembers). A real bound — nine clauses are rejected, pinned by a
@ts-expect-errorwhosefalsifiability was verified (removing the ninth clause makes
tscreport an unused directive).lib/platform-bible-utils:PropertyJsonPathwidened 4 → 8 depths (11 → 19 members). Not abound: the pre-existing
`$.${string}`catch-all already admits any depth. Documented assuch.
extensions/.../types/platform-scripture-editor.d.ts:ScriptureRange.endis now optional(
end?:). Non-breaking widening; matches the siblingplatform-scriptureScriptureRange.lib/papi-dts/papi.d.ts: unchanged, correctly — it imports from theplatform-bible-utilsmodule rather than inlining these types.
selectRange/setAnnotationnow accept a range whoseendis absent (collapses ontostart), and accept USJ-addressed ranges outside chapter 1(previously threw
Could not find book ID).scripture-editors: outbound property jsonPaths are bracket notation now(
$.content[1]['marker']). Verified across the whole core repo — core's only parser isnotation-agnostic and its own writer already emitted brackets. No consumer changes.
Findings
Critical — Must address before merge
e8c1b8c4;01e61166…1d640619existed only in the local checkout, so the PR's green CIsaid nothing about them. (fixed during review: branch pushed,
e8c1b8c4..72028744)packages/platform/distdid not reflect HEAD's src.Proved by extracting
sourcesContentfrom the committed sourcemap and diffing againstsrc— six shipped files differed. Core stages the editor straight out of a checkout, so moving
platform-yalcwould have shipped pre-fix bytes. (fixed during review: rebuilt withpnpm rebuild-committed-output;verify-committed-dist.mjs→ "Committed build output iscurrent. ✓"; no tsc-stub regression — 1865/665-line rollups intact)
lib/platform-bible-utils/distwas stale.dist/index.d.tsstill carried the superseded
PropertyJsonPathTSDoc. Documentation-only drift, but theartifact consumers read was the one place not in lock-step. (fixed during review: rebuilt)
John 2:4 plus the new collapsed/start-only
selectRangestep fails against a freshly stagededitor carrying every commit on both branches: the collapsed step polls for settled offset 0
and gets 75 (the previous step's selection), i.e. the editor's reported selection never moves.
A second run also failed the earlier "last two settled offsets" step. Deliberately left
uncommitted — it is in-flight work and committing it would commit a red test. Note CI would
not catch it either way:
.github/workflows/test.ymlruns only the smoke project.Important — Should address before merge
ScriptureRange.enddeclared required while a start-only range is a supported shape. Thenew unit test could only express it with
as ScriptureRange+ an eslint-disable.(fixed during review:
end?:, cast and suppression removed; verified the only reader alreadyhandles absence and no other consumer reads
.end)destroyed. A
TypedMarkNodewrapping note body text is a direct child of the note, so$rebuildNoteContentcallsremove()on it directly andTypedMarkNode.removeunconditionallydispatches
"destroyed"— while$restoreMarkByteRangesre-wraps it moments later. Theparagraph path is quiet only because whole paragraphs are spliced away and garbage-collected.
(fixed during review: suppress the callback before the direct removal, plus a note-case test)
handleSelectionChangetook a report ticket only on the deferred path,so a synchronous report could not supersede a deferred one already queued in the same tick and
the host heard the same location twice. (fixed during review: ticket taken on both paths)
a hardened decision to be folded into the relevant standard. (fixed during review: both rules
— positions are settled coordinates; the jsonPath types must be widened in lock-step — added to
Standard-View-Invariants.md§6)expectTypeOferases at runtime (notest.typecheckin either vitest config) and every assertion was
T extends T, so vitest reported passingtests that asserted nothing;
it('rejects nine content clauses')named an assertion its bodystructurally could not make. (fixed during review: rewritten in both repos with real runtime
assertions; the SE copy gained the missing nine-clause rejection so the lock-step rule is
guarded from both sides; falsifiability verified)
insertCommentAtCurrentSelection's contract-violation guards left no diagnostic trace.(fixed during review:
logger.warnon the unresolvable-path and offset-past-node branches,carrying the jsonPath and offset)
LogicalTextItem.lengthdoes not equal the USJ string length in Standardview when the text carries a run of 2+ spaces. That view's exporter also applies
normalizeSpaceRuns, which is not length-preserving, so every offset after such a run isshifted — the exact invariant this work exists to establish. The exporter-oracle test's
collapsedItems === []assertion passes only because the 2SA corpus contains zero space runs.Partially addressed during review: the doc now states the exception and the oracle's empty
assertion is marked a deliberate waiver that will go red if a corpus with a space run is added.
The real fix is a per-segment settled-offset map and needs a ticket. Fixed 2026-09-18 (follow-up 10).
into saved USJ content. The exporter's caller drop is now identity-based, and both slot
finders require the caller-slot child to be a plain text node — so a caller wrapped in a
TypedMarkNodeanchors nothing and its display bytes are emitted as note content. Reachable:CommentPluginwraps the raw selection with no caller exclusion. Not fixed — the fix is tomake both slot finders mark-transparent, which touches the exporter/model agreement the branch
just established. Needs a ticket.
coverage. ~28
return undefinedrefusal sites; v8 coverage over the wholesrc/editorsuiteshows every one uncovered. The design's central safety property is untested, which is also the
blind spot the four position findings below live in. Not fixed — needs a fixture where a
note scope and its paragraph's scope are pending simultaneously.
same uncovered region — a settling note inside a settling paragraph reports at the note's
live index; the co-settling-note branch falls through to an exact-subtree walk instead of
refusing;
$planForHuskOnlyParaomits the notes pass that$settledUsjruns; and an elementboundary whose nearest preceding span is a preserved node can never be anchored, so an ordinary
caret after a note in a pending paragraph is refused. Not fixed — each needs its own
fixture, and together they are a work item, not a review edit.
Minor — Consider
deepEqual(end, range.start)collapse inconvertScriptureRangeToEditorRange—verified inert once
end = range.end ?? range.startis in place (both ends resolve through thesame deterministic conversion from the same input), and no test could tell it from its absence.
PropertyJsonPathTSDoc claimed the$.catch-all subsumes every member;$['prop']starts$[and is not subsumed. Corrected in both repos, plus two subject-verb fixes.use-character-marker-state.hook.tsandcharacter-marker-coverage.utils.tsstill documentedan unresolvable path as expected editor/USJ drift — the contract now forbids it.
e2e-tests/tests/isolated/README.md's new entry enumerated five of the directory's eightspecs; made non-enumerative so it cannot go stale.
'converts … instead of throwing "Could not find book ID"'renamed.$fragmentTextWithoutTransientwas exported with no consumer outside itsown module;
exportdropped.not.toHaveBeenCalled()with no positive control — equally green against a settle that neverran. Positive controls added.
$charSeparatorPrefixLengthtold a maintainer the Standard-viewexporter "is positionally broader … and should narrow to this rule" — it was narrowed later in
this same branch (
precedesOpeningCharGlyph).cutFragmentandsettledPositions.api.test.tsxdoc comments overstatedwhat passes through a cut untouched / what the rebuild carries across.
packages/platform/test-output/(vitest coverage temp) was neither ignorednor tracked, in a repo that deliberately commits build output. Added to
.gitignore.checks-side-panel.web-view.tsx:580'sselectRange(...)is neither awaited nor.catch-ed,so a rejection is silent. Pre-existing and out of this diff — the file is unchanged. Worth a
follow-up;
find.web-view.tsx:1775shows the pattern.%webView_platformScriptureEditor_error_selectionContainsMarkers%("Selected text must be asimple word or phrase with no markers") for the contract-violation cases, which now tells the
user their selection is at fault for an internal bug. Not fixed — a distinct key needs
translation; the
logger.warnabove makes it diagnosable in the meantime.createCharMarker'snon-Standard separator strip was not narrowed alongside the Standard one;
precedesOpeningCharGlyphapproximates$charGlyphNestedValueand the approximation is notpinned; the oracle corpus contains no
TypedMarkNode;ImpliedParaNodeis counted as onelogical item but spliced by the exporter; two new import cycles inside
libs/shared/src/nodes/usj/;$precedingTextLocationreturns the last text item in the parent rather than the last onebefore the node;
settledToLiveTopIndexfalls back to identity for an out-of-range indexinstead of refusing;
$prepareSettleScopesomits$settledUsj's note-glyph-rename pass; andWHITESPACEinsettledPositions.utils.tsrestates the engine'sFRAGMENT_WS.Refuted / dismissed
adr-editor-edit-side-effects-shared-module<adr-editor-positions-are-settled-coordinates<adr-empty-is-zero-state-primitive. The oneLC_ALL=C sort -cdisorder in the file is the pre-existingadr-{slug}template placeholder..contextis in.prettierignore; not a format failure.c-sharp/assets/WEB/44JHNengWEBUS.SFM, and it is the first\wjspan in chapter 2, which is whatthe spec's search returns.
parser normalizes through
JSONPath.toPathArrayand itscontent[N]regex is blind to theproperty suffix's spelling. One dot-dependent comparison exists
(
character-marker-coverage.utils.ts:155) and is safe because both repos spell content paths$.content[N].platform-yalc" — verified:package.jsonusesfile:dev-packages/staging/...with no version range, neither editor package version changed onthe branch, and the lockfile entries are link pointers.
either diff.
Positive Observations
logicalModel.oracle.test.tsx) is genuinely strong, notdecorative: every early return either records a disagreement or records a skip, both asserted
empty; no try/catch, no filter, no conditional
it; and three counters close the"walk stopped early / corpus was empty" hole this kind of test usually leaves open.
DisplayRunKind,so a new kind is a compile error rather than a silent gap.
crosses into a scratch read and no scratch node crosses out. That is the single easiest way to get
this class of code catastrophically wrong, and it is closed.
and the husk-before-two-compatible-notes fixture is genuinely adversarial — crossed by raw index
the walk succeeds and lands in the wrong note, so the test discriminates.
outside chapter 1 now actually selects the range. A sweep confirmed it was the last unguarded call
site of
usjDocumentLocationToUsfmVerseRefVerseLocationin the repo.detect a symmetric off-by-one and pins each direction against an independent oracle (the browser's
own selection, a click gesture, PDP-sourced USJ).
one instance across ~175 added comment lines in core and none in the SE scoped files.
In-Review Quality Check
scripture-editors — typecheck clean (10 projects); lint 0 errors (45 warnings, all pre-existing
and none in touched files);
shared683 ✓,shared-react1732 + 1 pre-existing skip ✓,platform1793 ✓ (the PR body's 1766 was correct for the un-pushed tip),
utilities59 ✓;verify-committed-dist.mjs→ current ✓;verify-consumer-lockfile-sync.mjs→ in sync ✓.paranext-core —
npm run typecheck(all four legs) ✓;npm run lint0 errors ✓;npm test4590 passed / 15 skipped ✓. One unhandled rejection surfaced insrc/renderer/services/web-view.service-shard.test.tsunder full-suite load; the file passes 88/88in isolation and is untouched by this diff — a pre-existing timing flake.
e2e —
standard-view-annotation-positions.spec.tsrun against a freshly staged editor build:fails on the uncommitted working-tree version (see the open Critical above). The committed
version of the spec is unchanged by this review.
Suggested Review Focus
selectRangeat settled offset 0 does not land — confirmed still brokenend-to-end after every fix on both branches. Is this the editor-side half of deferred bug Remove the scribe package #2,
and does it want a ticket before merge?
The Standard-view space-run offset shift— fixed in follow-up 10.normalizeSpaceRunsis not length-preserving, so thesettled-coordinates guarantee has a hole the oracle corpus cannot see. Ticket?
grows a duplicate caller on reload. Ticket?
fixture (note scope + paragraph scope pending together) covers three of the four.
platform-yalc→ paranext-corePR #2823.