PT-3835: Don't insert verse spacing after gutter para marker prefixes#497
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders Open Preview |
jolierabideau
left a comment
jolierabideau
left a comment
There was a problem hiding this comment.
Thanks Ira, one small NIT from Claude.
@jolierabideau reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on irahopkinson).
libs/shared-react/src/plugins/usj/TextSpacingPlugin.tsx line 121 at r1 (raw file):
!$isTextNode(previousSibling) && !$isUnknownNode(previousSibling) && // Para marker prefixes carry their own trailing spacing (NBSP baked into the prefix text),
NIT From Claude -
"Para marker prefixes carry their own trailing spacing (NBSP baked into the prefix text)" isn't quite right for either flavor. An editable MarkerNode is just \marker with no trailing spacing (openingMarkerText), and the visible marker's trailing separator is a plain space added by getVisibleOpenMarkerText (the NBSP there sits between the marker and its content, not at the end). The guard itself is correct — the real justification is the next clause: an inserted plain " " becomes exporter-visible USJ content and shifts every downstream content index. Since the guard fires for both marker modes ($isParaMarkerPrefix = $isMarkerNode || $isVisibleMarkerNode), I'd just reword so a future reader doesn't rely on a trailing NBSP that isn't there. Visual separation still holds in visible mode via that trailing plain space.
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
Thanks for continuing this fix, Ira! Just a couple of small things to consider. Approving because this could be merged as is and be good, there just might be a couple small improvements.
@katherinejensen00 reviewed 3 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on irahopkinson).
libs/shared-react/src/plugins/usj/TextSpacingPlugin.tsx line 124 at r1 (raw file):
// and an inserted plain " " would be exporter-visible USJ content that shifts every content // index in the paragraph (see PT-3835). !$isParaMarkerPrefix(previousSibling)
NIT Claude Finding
The fix is right (confirmed MarkerNode extends TextNode, so this correctly targets the gutter/visible ImmutableTypedTextNode prefix). I went down a path worrying the same shift was open for any TypedMarkNode predecessor, but I traced it through $getLogicalContentItems and the common case doesn't reproduce: when an annotation wraps plain text before a verse, the inserted " " coalesces onto the existing text run, so the verse's logical index is unchanged. No off-by-one there.
The one case that does still shift is narrow: an annotation whose trailing content is a non-text element (CharNode/NoteNode) sitting immediately before a verse — e.g. a verse ending in \nd LORD that gets annotated through. There the CharNode breaks the coalesced run, so the inserted space becomes a standalone content item and pushes the verse index by one, the same way the para-marker prefix did. Reachable in principle, but edge-y, so I'd treat it as hardening rather than a live bug.
Two ways to close it if you think it's worth it: add !$isTypedMarkNode(previousSibling) to this same condition (one line, no behavior change for anything else), or — the design angle — note that "which nodes are transparent to USJ content" is now encoded both here and in #496's $shouldIgnoreNodeForContentIndexes, and the two can drift as node types are added (this TypedMarkNode gap is the first instance). Deriving the exclusion from a shared predicate, or inverting to an allowlist of nodes that genuinely need a structural space (the NoteNode/CharNode cases you validated in P9), would make new transparent types default to safe. Not a blocker — the current fix is correct for what shipped.
I have a two-case unit test that pins exactly which scenario shifts — the first passes today, the second fails today (returns 5 instead of 4) and would pass with the !$isTypedMarkNode guard, so it documents both the boundary and the fix. Happy to fold it into TextSpacingPlugin.test.tsx if you'd like the edge case captured:
it("annotation over plain text before a verse does NOT shift the verse's logical index", async () => {
const { editor } = await testEnvironment(() => {
$getRoot().append(
$createParaNode().append(
$createImmutableVerseNode("1"),
$createTextNode("the "),
$createTypedMarkNode({ t: ["1"] }).append($createTextNode("beginning")),
$createImmutableVerseNode("2"),
),
);
});
await act(async () => editor.update(() => undefined));
editor.getEditorState().read(() => {
const para = $getRoot().getFirstChild();
if (!$isParaNode(para)) throw new Error("Expected a ParaNode");
// Space coalesces into "the beginning " → verse 2 stays at logical index 2.
expect($getLogicalContentItems(para)).toHaveLength(3);
});
});
it("annotation ending on a CharNode before a verse DOES shift (the guard's real target)", async () => {
const { editor } = await testEnvironment(() => {
$getRoot().append(
$createParaNode().append(
$createImmutableVerseNode("1"),
$createTextNode("text "),
$createTypedMarkNode({ t: ["1"] }).append(
$createCharNode("nd").append($createTextNode("LORD")),
),
$createImmutableVerseNode("2"),
),
);
});
await act(async () => editor.update(() => undefined));
editor.getEditorState().read(() => {
const para = $getRoot().getFirstChild();
if (!$isParaNode(para)) throw new Error("Expected a ParaNode");
// CharNode breaks the run → inserted space is a standalone item → verse 2 shifts to index 4.
// Fails today (returns 5); passes with `!$isTypedMarkNode` added to $verseNodeTransform.
expect($getLogicalContentItems(para)).toHaveLength(4);
});
});(Needs $getLogicalContentItems imported from shared; the other factories are already imported in the test file.)
packages/platform/src/editor/annotation-pending-rewrap.test.tsx line 8 at r1 (raw file):
* "Failed to find start or end node of the annotation." * * MINIMAL FAILING CONFIGURATION: the failure needs the REAL platform Editor (full plugin
NIT
This is a dense comment with a lot of good information. I wonder if it might be more readable if there is a summary or a TLDR section at the top followed by the more detailed explanation. Here is a potential summary:
* Reproduces ONLY with: real platform Editor + formatted view + hasGutterParaMarkers: true.
* Does NOT reproduce with:
* - a minimal LexicalComposer mounting only AnnotationPlugin (even with identical view options)
* - the real Editor without the gutter option
* - showCharMarkerTitles / hasActiveTextFocusBox alone (see the passing control)
irahopkinson
force-pushed
the
pt-3835-annotation-selection
branch
from
25207dd to
c7f95f3
Compare
- Resolve annotation wrappers to their last content child in $verseNodeTransform: wrappers are presentation-only, so the spacing decision depends on what is inside them. Text inside a wrapper still gets the structural space (coalesces onto the same USJ run; the trailing-space transform can't reach it), a wrapper ending on a char/note still gets it (canonical USJ has the standalone ' ' item - Paratext re-inserts it), and an empty wrapper now inserts nothing. Tests pin all three behaviors. - Reword the para-marker-prefix comment: the old text claimed a trailing NBSP that editable MarkerNodes don't have; the real justification is that prefixes are not USJ content so no structural space belongs after them. - Add a TLDR summary to the pending-rewrap lifecycle test header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
irahopkinson
force-pushed
the
pt-3835-part2
branch
from
0d9de5f to
2a0f7f7
Compare
irahopkinson
force-pushed
the
pt-3835-annotation-selection
branch
from
c7f95f3 to
2953514
Compare
- Resolve annotation wrappers to their last content child in $verseNodeTransform: wrappers are presentation-only, so the spacing decision depends on what is inside them. Text inside a wrapper still gets the structural space (coalesces onto the same USJ run; the trailing-space transform can't reach it), a wrapper ending on a char/note still gets it (canonical USJ has the standalone ' ' item - Paratext re-inserts it), and an empty wrapper now inserts nothing. Tests pin all three behaviors. - Reword the para-marker-prefix comment: the old text claimed a trailing NBSP that editable MarkerNodes don't have; the real justification is that prefixes are not USJ content so no structural space belongs after them. - Add a TLDR summary to the pending-rewrap lifecycle test header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
irahopkinson
force-pushed
the
pt-3835-part2
branch
from
2a0f7f7 to
435f618
Compare
irahopkinson
left a comment
irahopkinson
left a comment
There was a problem hiding this comment.
I've already used more than the time I have for coding in this sprint. However I have addressed the 3 comments. For further work either approve as is and add your own follow-up PR or take over the branch and fix yourself.
@irahopkinson made 4 comments and resolved 3 discussions.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on jolierabideau and katherinejensen00).
libs/shared-react/src/plugins/usj/TextSpacingPlugin.tsx line 121 at r1 (raw file):
Previously, jolierabideau wrote…
NIT From Claude -
"Para marker prefixes carry their own trailing spacing (NBSP baked into the prefix text)" isn't quite right for either flavor. An editable
MarkerNodeis just\markerwith no trailing spacing (openingMarkerText), and the visible marker's trailing separator is a plain space added bygetVisibleOpenMarkerText(the NBSP there sits between the marker and its content, not at the end). The guard itself is correct — the real justification is the next clause: an inserted plain" "becomes exporter-visible USJ content and shifts every downstream content index. Since the guard fires for both marker modes ($isParaMarkerPrefix=$isMarkerNode || $isVisibleMarkerNode), I'd just reword so a future reader doesn't rely on a trailing NBSP that isn't there. Visual separation still holds in visible mode via that trailing plain space.
Done. Good catch — the NBSP claim was wrong as written (editable MarkerNode is bare \marker with its trailing space in a separate node, and the visible marker's trailing separator is a plain space). Reworded: the comment now leads with the real justification — marker prefixes aren't USJ content, so no structural space belongs after them, and an inserted plain " " becomes exporter-visible content that shifts every content index.
libs/shared-react/src/plugins/usj/TextSpacingPlugin.tsx line 124 at r1 (raw file):
Previously, katherinejensen00 wrote…
NIT Claude Finding
The fix is right (confirmed
MarkerNode extends TextNode, so this correctly targets the gutter/visibleImmutableTypedTextNodeprefix). I went down a path worrying the same shift was open for anyTypedMarkNodepredecessor, but I traced it through$getLogicalContentItemsand the common case doesn't reproduce: when an annotation wraps plain text before a verse, the inserted" "coalesces onto the existing text run, so the verse's logical index is unchanged. No off-by-one there.The one case that does still shift is narrow: an annotation whose trailing content is a non-text element (
CharNode/NoteNode) sitting immediately before a verse — e.g. a verse ending in\nd LORDthat gets annotated through. There theCharNodebreaks the coalesced run, so the inserted space becomes a standalone content item and pushes the verse index by one, the same way the para-marker prefix did. Reachable in principle, but edge-y, so I'd treat it as hardening rather than a live bug.Two ways to close it if you think it's worth it: add
!$isTypedMarkNode(previousSibling)to this same condition (one line, no behavior change for anything else), or — the design angle — note that "which nodes are transparent to USJ content" is now encoded both here and in #496's$shouldIgnoreNodeForContentIndexes, and the two can drift as node types are added (thisTypedMarkNodegap is the first instance). Deriving the exclusion from a shared predicate, or inverting to an allowlist of nodes that genuinely need a structural space (theNoteNode/CharNodecases you validated in P9), would make new transparent types default to safe. Not a blocker — the current fix is correct for what shipped.I have a two-case unit test that pins exactly which scenario shifts — the first passes today, the second fails today (returns 5 instead of 4) and would pass with the
!$isTypedMarkNodeguard, so it documents both the boundary and the fix. Happy to fold it intoTextSpacingPlugin.test.tsxif you'd like the edge case captured:it("annotation over plain text before a verse does NOT shift the verse's logical index", async () => { const { editor } = await testEnvironment(() => { $getRoot().append( $createParaNode().append( $createImmutableVerseNode("1"), $createTextNode("the "), $createTypedMarkNode({ t: ["1"] }).append($createTextNode("beginning")), $createImmutableVerseNode("2"), ), ); }); await act(async () => editor.update(() => undefined)); editor.getEditorState().read(() => { const para = $getRoot().getFirstChild(); if (!$isParaNode(para)) throw new Error("Expected a ParaNode"); // Space coalesces into "the beginning " → verse 2 stays at logical index 2. expect($getLogicalContentItems(para)).toHaveLength(3); }); }); it("annotation ending on a CharNode before a verse DOES shift (the guard's real target)", async () => { const { editor } = await testEnvironment(() => { $getRoot().append( $createParaNode().append( $createImmutableVerseNode("1"), $createTextNode("text "), $createTypedMarkNode({ t: ["1"] }).append( $createCharNode("nd").append($createTextNode("LORD")), ), $createImmutableVerseNode("2"), ), ); }); await act(async () => editor.update(() => undefined)); editor.getEditorState().read(() => { const para = $getRoot().getFirstChild(); if (!$isParaNode(para)) throw new Error("Expected a ParaNode"); // CharNode breaks the run → inserted space is a standalone item → verse 2 shifts to index 4. // Fails today (returns 5); passes with `!$isTypedMarkNode` added to $verseNodeTransform. expect($getLogicalContentItems(para)).toHaveLength(4); }); });(Needs
$getLogicalContentItemsimported fromshared; the other factories are already imported in the test file.)
Done. Thanks for the careful trace — the coalescing analysis for the plain-text case is spot on, and your two-case test made the boundary really clear.
I went with a variant of your suggestion: instead of !$isTypedMarkNode(previousSibling), the transform now resolves the wrapper to its last content child and decides from that. Reason: I tested the char/note-before-verse case in Paratext 9 — if you delete the space between them, P9 puts it back. That space is structural (USJ→USFM conversion needs it), so canonical USJ genuinely has the standalone " " item there, and inserting it matches the exported shape rather than shifting it. A blanket !$isTypedMarkNode guard would have made annotated content export differently from the same content unannotated. So:
- wrapper ending in text → insert (coalesces onto the run, no index shift — your case 1, still 3 items)
- wrapper ending in char/note → insert (canonical structural space — your case 2, pinned at 5 items with a comment explaining why)
- empty wrapper → skip (the actual behavior fix; previously it added an exporter-visible space for a presentation-only node)
All three are now pinned in TextSpacingPlugin.test.tsx (your tests, adapted — thank you!). On the design point about "which nodes are USJ-transparent" living in two places: agreed it can drift; I'd like to keep that as a follow-up rather than grow this PR — the look-through keeps this transform aligned with the model's transparency rule for now.
packages/platform/src/editor/annotation-pending-rewrap.test.tsx line 8 at r1 (raw file):
Previously, katherinejensen00 wrote…
NIT
This is a dense comment with a lot of good information. I wonder if it might be more readable if there is a summary or a TLDR section at the top followed by the more detailed explanation. Here is a potential summary:* Reproduces ONLY with: real platform Editor + formatted view + hasGutterParaMarkers: true. * Does NOT reproduce with: * - a minimal LexicalComposer mounting only AnnotationPlugin (even with identical view options) * - the real Editor without the gutter option * - showCharMarkerTitles / hasActiveTextFocusBox alone (see the passing control)
Done. Agreed — added your TLDR block at the top verbatim (reproduces-only-with / does-not-reproduce-with), with the prose detail kept below it. 2a0f7f7.
irahopkinson
dismissed stale reviews from katherinejensen00 and jolierabideau
The base branch was changed.
katherinejensen00
left a comment
katherinejensen00
left a comment
There was a problem hiding this comment.
@katherinejensen00 reviewed 11 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved.
With gutter para markers enabled (formatted view), a paragraph's first
child is a \p marker prefix node that TextSpacingPlugin's
$verseNodeTransform did not recognize, so the first editor update
inserted a spurious single-space TextNode before verse 1. That space is
real content to both the logical content model and the editor->USJ
export, shifting every content index in the paragraph by one. In
paranext-core's Insert Comment flow this made the thread annotation fail
to re-apply after the pending annotation was removed ('Failed to find
start or end node of the annotation.') and left every subsequent
reported selection in the paragraph off by one, tripping the host's
marker-validation guard. The same latent bug existed in plain
visible-marker mode.
Fix: exclude para marker prefixes ($isParaMarkerPrefix) from the
transform's insert-space condition. Marker prefixes already carry their
own trailing spacing, so the inserted space was never needed for
rendering in any mode.
Note: the transform's remaining insertion paths are intentional. A space
between a note/char and a following verse marker is structural rather
than content: USJ->USFM conversion needs it, and Paratext 9 re-inserts
it when removed. Only the para-marker-prefix case was spurious.
Coverage: a unit regression in TextSpacingPlugin.test.tsx (marker prefix
+ verse tree must gain no space), and a lifecycle suite mounting the
real platform Editor (annotation-pending-rewrap.test.tsx) that runs the
exact host flow - pending setAnnotation -> removeAnnotation ->
setAnnotation with the same range - under gutter-only and
titles/gutter/focus-box-all-on view options plus an all-off control, in
separate-act and same-tick sequencing, and asserts post-rewrap
selections still report coalesced-USJ coordinates.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Resolve annotation wrappers to their last content child in $verseNodeTransform: wrappers are presentation-only, so the spacing decision depends on what is inside them. Text inside a wrapper still gets the structural space (coalesces onto the same USJ run; the trailing-space transform can't reach it), a wrapper ending on a char/note still gets it (canonical USJ has the standalone ' ' item - Paratext re-inserts it), and an empty wrapper now inserts nothing. Tests pin all three behaviors. - Reword the para-marker-prefix comment: the old text claimed a trailing NBSP that editable MarkerNodes don't have; the real justification is that prefixes are not USJ content so no structural space belongs after them. - Add a TLDR summary to the pending-rewrap lifecycle test header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
irahopkinson
force-pushed
the
pt-3835-part2
branch
from
435f618 to
2c2b86f
Compare
3 participants
Code Review Summary
Branch: pt-3835-part2
Base: pt-3835-annotation-selection (this branch is stacked on the PT-3835 branch / PR #496; diffing against
origin/mainwould re-include the parent PR's already-reviewed changes)Date: 2026-07-07
Review model: Claude Fable 5
Files changed: 3
Overview
Follow-up fix for PT-3835, found while verifying the annotation-transparency fix in paranext-core. With gutter para markers enabled (formatted view), a paragraph's first child is a
\pmarker prefix node thatTextSpacingPlugin's$verseNodeTransformdid not recognize, so the first editor update inserted a spurious single-space TextNode before verse 1. That space is real content to both the logical content model and the editor→USJ export, shifting every content index in the paragraph by one. In paranext-core's Insert Comment flow this made the thread annotation fail to re-apply after the pending annotation was removed ("Failed to find start or end node of the annotation.") and left every subsequent reported selection off by one, tripping the host's marker-validation guard. The same latent bug existed in plain visible-marker mode.The fix is a one-line guard: exclude para marker prefixes (
$isParaMarkerPrefix) from the transform's insert-space condition — marker prefixes carry their own trailing spacing. The transform's remaining insertion paths are intentional: the author verified in Paratext 9 that a space between a note/char and a following verse marker is structural (USJ→USFM conversion needs it; P9 re-inserts it when removed), so only the marker-prefix case was spurious. Coverage: a unit regression inTextSpacingPlugin.test.tsx, plus a lifecycle suite (annotation-pending-rewrap.test.tsx) mounting the real platform Editor and running the exact host flow under both failing-mode view-option variants plus an all-off passing control, asserting post-rewrap selections still report coalesced-USJ coordinates. The author retested in paranext-core: working in both Genesis 1 and Genesis 2 scenarios.API Changes
None. No public API surfaces changed:
packages/platform(@eten-tech-foundation/platform-editor, published): only a new test file was added — no source or export changes.libs/shared-react(internal lib):TextSpacingPlugin.tsxchanged internally (one added condition inside the non-exported$verseNodeTransform); the exportedTextSpacingPlugincomponent's signature and exports are unchanged.libs/shared: not modified.Findings
Critical — Must address before merge
None.
Important — Should address before merge
None.
Minor — Consider
!$isParaMarkerPrefixexclusion inTextSpacingPlugin.tsx(the "why" lived only in tests and the commit message) (fixed during review: comment added above the condition — marker prefixes carry their own trailing NBSP, and an inserted plain space becomes exporter-visible USJ content that shifts content indices; folded into the amended commit)Test-helper duplication between(author dismissed: same pattern, not verbatim duplication — the new helpers' parameterization (view options, injected logger error-spy, all-marks text assertions) is load-bearing; extract a shared helper when a third editor-lifecycle test file appears)annotation-pending-rewrap.test.tsx(createRealEditor, DOM-mark helpers) andEditor.test.tsx(createEditorRefForTesting,getMarkElement)" "when a verse follows a NoteNode/CharNode) was untracked in code (fixed during review — and reclassified: the author tested in Paratext 9 that this space is structural, not content — USJ→USFM conversion requires it and P9 re-inserts it when removed. No follow-up ticket needed; the commit message was amended to document that the remaining insertion paths are intentional and only the marker-prefix case was spurious)[Author response: author requested fixes for three; the helper-duplication finding was dismissed with the extract-at-third-file rationale. The follow-up finding produced a genuine domain insight (structural vs content spaces) that reshaped the commit message rather than a ticket.]
Template Propagation
Shared Regions Modified
None.
Extension Config Changes
None.
Positive Observations
$isParaMarkerPrefixrather than inlining node-type checks, covers both prefix flavors (editableMarkerNodeand gutter/visibleImmutableTypedTextNode), and matches the transform's existing exclusion-list style exactly.$createImmutableTypedTextNode("marker", openingMarkerText("p") + NBSP)) rather than a stand-in, and is verifiably falsifiable (without the guard, the child count assertion fails).Editorthrough the sameEditorRefAPI paranext-core drives, includes a fixture-sanity test that distinguishes fixture bugs from code bugs, covers both failing-mode variants plus an all-off control, covers same-tick and separate-act sequencing (matching the app's single event handler), and pins the original defect's downstream symptom (post-rewrap selections elsewhere still report correct coalesced-USJ coordinates). The header documents the minimal failing configuration and what does NOT reproduce it.Interview Notes
TextSpacingPlugininserting a spurious, index-shifting space after gutter para marker prefixes (PT-3835 follow-up found during core verification).In-Review Quality Check
pnpm nx run-many -t typecheck: all 10 projects pass.pnpm nx lint shared-react/pnpm nx lint @eten-tech-foundation/platform-editor: 0 errors.pnpm nx format:check: clean.TextSpacingPlugin18/18,annotation-pending-rewrap7/7..claude//CLAUDE.mdpaths now replaced by ai-prompts symlinks on this machine, which broke lint-staged's backup — the author fixed the local state; a repo-level untrack+gitignore may be worth a separate change.)Suggested Review Focus
$verseNodeTransformno longer inserts a space after ANY para marker prefix in any mode — this also silently fixes the same latent bug in plain visible-marker mode. Confirm no visible-mode rendering depended on the old space (analysis found none: prefixes carry their own trailing NBSP).annotation-pending-rewrap.test.tsx) as a pattern: real-Editor-mount regression tests for host-flow bugs — consider whether future editor-level defects should get the same treatment.pt-3835-annotation-selection(PR PT-3835: Make USJ selection locations annotation-transparent #496) — decide merge order / whether to fold into PT-3835: Make USJ selection locations annotation-transparent #496.AI-assisted — session
This change is