diff --git a/libs/shared-react/src/plugins/usj/TextSpacingPlugin.test.tsx b/libs/shared-react/src/plugins/usj/TextSpacingPlugin.test.tsx index ba9a0b3d..11510949 100644 --- a/libs/shared-react/src/plugins/usj/TextSpacingPlugin.test.tsx +++ b/libs/shared-react/src/plugins/usj/TextSpacingPlugin.test.tsx @@ -16,14 +16,19 @@ import { $createTextNode, $getRoot, $isTextNode, TextNode, $setSelection } from import { $createCharNode, $createImmutableChapterNode, + $createImmutableTypedTextNode, $createNoteNode, $createParaNode, $createTypedMarkNode, $createUnknownNode, + $getLogicalContentItems, $isCharNode, $isParaNode, $isTypedMarkNode, $isUnknownNode, + $isVisibleMarkerNode, + NBSP, + openingMarkerText, ParaNode, UnknownNode, } from "shared"; @@ -364,6 +369,119 @@ describe("TextSpacingPlugin", () => { }); }); + it("should not insert a space before a verse if preceded by a gutter paragraph marker prefix", async () => { + // Regression test for PT-3835 Gen 2: gutter mode (`hasGutterParaMarkers: true`) renders the + // paragraph's `\p` marker as a visible-marker ImmutableTypedTextNode (textType "marker") that is + // verse 1's previous sibling. That node is a DecoratorNode, not a TextNode/UnknownNode, so + // $verseNodeTransform used to treat it like arbitrary unrecognized content and insert a spurious + // leading space, shifting every logical content index in the paragraph. + const { editor } = await testEnvironment(() => { + $getRoot().append( + $createParaNode().append( + $createImmutableTypedTextNode("marker", openingMarkerText("p") + NBSP), + $createImmutableVerseNode("1"), + ), + ); + }); + + // Trigger an update (no change expected). + await act(async () => editor.update(() => undefined)); + + editor.getEditorState().read(() => { + const para = $getRoot().getFirstChild(); + if (!$isParaNode(para)) throw new Error("Expected a ParaNode"); + // Should remain [marker prefix, VerseNode] -- no spurious space inserted. + expect(para.getChildren()).toHaveLength(2); + expect($isVisibleMarkerNode(para.getChildAtIndex(0))).toBe(true); + expect($isSomeVerseNode(para.getChildAtIndex(1))).toBe(true); + }); + }); + + it("should space an annotation over plain text before a verse without shifting its logical index", async () => { + // The annotation wrapper is transparent to USJ content: the inserted space coalesces onto + // the wrapped text's run (the trailing-space transform can't reach text inside a + // TypedMarkNode), so the verse's logical content index is unchanged. + const { editor } = await testEnvironment(() => { + $getRoot().append( + $createParaNode().append( + $createImmutableVerseNode("1"), + $createTextNode("the "), + $createTypedMarkNode({ t: ["1"] }).append($createTextNode("beginning")), + $createImmutableVerseNode("2"), + ), + ); + }); + + // Trigger an update (transforms run). + await act(async () => editor.update(() => undefined)); + + editor.getEditorState().read(() => { + const para = $getRoot().getFirstChild(); + if (!$isParaNode(para)) throw new Error("Expected a ParaNode"); + // [verse 1, "the beginning ", verse 2] — space coalesced into the run, no index shift. + expect($getLogicalContentItems(para)).toHaveLength(3); + expect(para.getTextContent()).toBe("the beginning "); + }); + }); + + it("should insert the structural space when an annotation ending on a CharNode precedes a verse", async () => { + // A space between a char and a following verse marker is structural, not content: USJ→USFM + // conversion needs it and Paratext 9 re-inserts it when removed. Canonical USJ therefore has + // a standalone " " item here, so inserting it matches the exported shape — the annotation + // wrapper must not suppress it. + const { editor } = await testEnvironment(() => { + $getRoot().append( + $createParaNode().append( + $createImmutableVerseNode("1"), + $createTextNode("text "), + $createTypedMarkNode({ t: ["1"] }).append( + $createCharNode("nd").append($createTextNode("LORD")), + ), + $createImmutableVerseNode("2"), + ), + ); + }); + + // Trigger an update (transforms run). + await act(async () => editor.update(() => undefined)); + + editor.getEditorState().read(() => { + const para = $getRoot().getFirstChild(); + if (!$isParaNode(para)) throw new Error("Expected a ParaNode"); + // [verse 1, "text ", char, " ", verse 2] — the structural space is its own content item, + // exactly as canonical USJ from Paratext has it. + const items = $getLogicalContentItems(para); + expect(items).toHaveLength(5); + expect(items[3].type).toBe("text"); + }); + }); + + it("should not insert a space before a verse for an empty annotation wrapper", async () => { + // An empty TypedMarkNode resolves to no content, so no structural space belongs after it — + // inserting one would add exporter-visible USJ content because of a presentation-only node. + const { editor } = await testEnvironment(() => { + $getRoot().append( + $createParaNode().append( + $createImmutableVerseNode("1"), + $createTextNode("a "), + $createTypedMarkNode({ t: ["1"] }), + $createImmutableVerseNode("2"), + ), + ); + }); + + // Trigger an update (transforms run). + await act(async () => editor.update(() => undefined)); + + editor.getEditorState().read(() => { + const para = $getRoot().getFirstChild(); + if (!$isParaNode(para)) throw new Error("Expected a ParaNode"); + // [verse 1, "a ", verse 2] — no double space, no extra content item. + expect($getLogicalContentItems(para)).toHaveLength(3); + expect(para.getTextContent()).toBe("a "); + }); + }); + it("should not remove a space left after deletion if it precedes a verse", async () => { let textNodeToDelete: TextNode; const { editor } = await testEnvironment(() => { diff --git a/libs/shared-react/src/plugins/usj/TextSpacingPlugin.tsx b/libs/shared-react/src/plugins/usj/TextSpacingPlugin.tsx index cbf06f06..37d010cf 100644 --- a/libs/shared-react/src/plugins/usj/TextSpacingPlugin.tsx +++ b/libs/shared-react/src/plugins/usj/TextSpacingPlugin.tsx @@ -7,12 +7,13 @@ import { } from "../../nodes/usj/node-react.utils"; import { useLexicalComposerContext } from "@lexical/react/LexicalComposerContext"; import { mergeRegister } from "@lexical/utils"; -import { $createTextNode, $isTextNode, LexicalEditor, TextNode } from "lexical"; +import { $createTextNode, $isTextNode, LexicalEditor, LexicalNode, TextNode } from "lexical"; import { useEffect } from "react"; import { $isCharNode, $isNoteNode, $isParaLikeNode, + $isParaMarkerPrefix, $isTypedMarkNode, $isUnknownNode, CharNode, @@ -111,12 +112,26 @@ function $textNodeInUnknownTransform(node: TextNode, editor: LexicalEditor): voi function $verseNodeTransform(node: SomeVerseNode): void { if (!node.isAttached()) return; - const previousSibling = node.getPreviousSibling(); + // Annotation wrappers (TypedMarkNode) are presentation-only, so the spacing decision depends + // on the content INSIDE them: resolve to the wrapper's last content child (nested wrappers + // are transient but resolved for safety). An empty wrapper resolves to null and inserts + // nothing. + let previousSibling: LexicalNode | null = node.getPreviousSibling(); + while ($isTypedMarkNode(previousSibling)) previousSibling = previousSibling.getLastChild(); + if ( previousSibling && !$isSomeVerseNode(previousSibling) && - !$isTextNode(previousSibling) && - !$isUnknownNode(previousSibling) + !$isUnknownNode(previousSibling) && + // Para marker prefixes are presentation scaffolding, not USJ content, so no structural + // space belongs after them — and an inserted plain " " would be exporter-visible USJ + // content that shifts every content index in the paragraph (see PT-3835). Their visual + // separation comes from the prefix nodes' own text. + !$isParaMarkerPrefix(previousSibling) && + // Bare text before a verse gets its structural space from $textNodeTrailingSpaceTransform; + // text inside an annotation wrapper can't (that transform skips TypedMarkNode parents), so + // the space is inserted here instead and coalesces onto the same USJ text run. + (!$isTextNode(previousSibling) || $isTypedMarkNode(previousSibling.getParent())) ) node.insertBefore($createTextNode(" ")); } diff --git a/packages/platform/src/editor/annotation-pending-rewrap.test.tsx b/packages/platform/src/editor/annotation-pending-rewrap.test.tsx new file mode 100644 index 00000000..3714e10d --- /dev/null +++ b/packages/platform/src/editor/annotation-pending-rewrap.test.tsx @@ -0,0 +1,236 @@ +/** + * PT-3835 follow-up (Gen 2 repro): re-applying a comment annotation to the SAME range after a + * pending-annotation wrap/unwrap cycle must resolve. Mirrors paranext-core's Insert Comment + * save flow exactly: setAnnotation(pending) → save → removeAnnotation(pending) → + * setAnnotation(threadId, same range). Confirmed failing in the app at the last step with + * "Failed to find start or end node of the annotation." + * + * 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) + * + * Detail: gutter mode renders paragraph markers as extra immutable typed-text nodes in the + * tree, and the failure needs the full plugin stack driven through the Editor's ref API like + * paranext does. Same-tick and separate-act sequencing fail identically at the re-apply step; + * the pending annotation and its removal both work. + * + * The suite covers both failing-mode variants (gutter-only minimal config, and + * titles/gutter/focus-box all on — the original app configuration) plus the all-off passing + * control. + */ +import Editor from "./Editor"; +import { EditorRef } from "./editor.model"; +import { Usj, USJ_TYPE, USJ_VERSION } from "@eten-tech-foundation/scripture-utilities"; +import { act, render } from "@testing-library/react"; +import { createRef } from "react"; +import { AnnotationRange, getViewOptions, ViewOptions } from "shared-react"; +import { vi } from "vitest"; + +// Genesis 2:1-3 (WEB). In verse 1: "earth" = 17..22, "all" = 28..31. +const v1Text = "The heavens, the earth, and all their vast array were finished."; + +const usjGen2: Usj = { + type: USJ_TYPE, + version: USJ_VERSION, + content: [ + { type: "chapter", marker: "c", number: "2" }, + { + type: "para", + marker: "p", + content: [ + { type: "verse", marker: "v", number: "1" }, + v1Text, + { type: "verse", marker: "v", number: "2" }, + "On the seventh day God finished his work which he had done; and he rested on the seventh" + + " day from all his work which he had done.", + { type: "verse", marker: "v", number: "3" }, + "God blessed the seventh day, and made it holy, because he rested in it from all his work" + + " of creation which he had done.", + ], + }, + ], +}; + +// USJ content: [0]=chapter, [1]=para. Para content items: [0]=verse 1 marker, [1]=v1 text, ... +const jsonPath = "$.content[1].content[1]"; +const earthStart = v1Text.indexOf("earth"); +const earthRange: AnnotationRange = { + start: { jsonPath, offset: earthStart }, + end: { jsonPath, offset: earthStart + "earth".length }, +}; + +// Used to pin the original defect's step 5: after the rewrap, every reported selection in that +// paragraph (not just the annotated range itself) must still resolve to correct USJ coordinates. +const allStart = v1Text.indexOf("all"); +const allRange: AnnotationRange = { + start: { jsonPath, offset: allStart }, + end: { jsonPath, offset: allStart + "all".length }, +}; + +const formattedViewOptions = getViewOptions("formatted"); +if (!formattedViewOptions) throw new Error("Expected formatted view options to exist"); + +/** + * Both failing-mode variants: the minimal failing configuration (gutter only) and the original + * app configuration (`showCharMarkerTitles`/`hasGutterParaMarkers`/`hasActiveTextFocusBox` all + * on). The all-off configuration is the passing control below. + */ +const failingModeVariants: { name: string; viewOptions: ViewOptions }[] = [ + { + name: "gutter only (minimal failing config)", + viewOptions: { ...formattedViewOptions, hasGutterParaMarkers: true }, + }, + { + name: "titles/gutter/focus-box all on", + viewOptions: { + ...formattedViewOptions, + showCharMarkerTitles: true, + hasGutterParaMarkers: true, + hasActiveTextFocusBox: true, + }, + }, +]; + +describe.each(failingModeVariants)( + "pending-comment rewrap in the real Editor, $name", + ({ viewOptions }) => { + it("fixture sanity: 'earth' USJ coordinates round-trip through the Editor selection API", async () => { + const editor = await createRealEditor(viewOptions, createLoggerMock()); + + await act(async () => { + editor.setSelection({ start: earthRange.start, end: earthRange.end }); + }); + const reported = editor.getSelection(); + + // If THIS fails the fixture/jsonPath assumptions are wrong — fix the test, not the code. + expect(reported?.start).toEqual(earthRange.start); + expect(reported?.end).toEqual(earthRange.end); + }); + + it("re-applies the same range after a pending wrap/unwrap cycle", async () => { + const logError = createLoggerMock(); + const editor = await createRealEditor(viewOptions, logError); + + // 1. Pending highlight (works in the app too). + await act(async () => { + editor.setAnnotation(earthRange, "translator-comment", "pending-comment"); + }); + expect(logError).not.toHaveBeenCalled(); + expectDomMarkOver("earth"); + + // 2. Save removes the pending annotation (works in the app too). + await act(async () => { + editor.removeAnnotation("translator-comment", "pending-comment"); + }); + expectNoDomMarks(); + + // 3. Thread annotation with the SAME captured range. Did FAIL HERE, exactly like the app: + // logger.error("Failed to find start or end node of the annotation.") and no mark is + // created. + await act(async () => { + editor.setAnnotation(earthRange, "translator-comment", "thread-1"); + }); + expect(logError).not.toHaveBeenCalled(); + expectDomMarkOver("earth"); + + // 4. Post-rewrap: selection reporting elsewhere in the same paragraph must still resolve to + // correct, coalesced USJ coordinates. This pins step 5 of the original defect: after the + // rewrap, every reported selection in that paragraph was wrong. + await act(async () => { + editor.setSelection({ start: allRange.start, end: allRange.end }); + }); + const reportedAfterRewrap = editor.getSelection(); + expect(reportedAfterRewrap?.start).toEqual(allRange.start); + expect(reportedAfterRewrap?.end).toEqual(allRange.end); + }); + + it("re-applies when remove and set happen back-to-back in one update cycle", async () => { + // The app calls removeAnnotation + setAnnotation in ONE event handler, so cover the + // same-tick sequencing as its own case. Fails identically to the separate-act variant. + const logError = createLoggerMock(); + const editor = await createRealEditor(viewOptions, logError); + + await act(async () => { + editor.setAnnotation(earthRange, "translator-comment", "pending-comment"); + }); + await act(async () => { + editor.removeAnnotation("translator-comment", "pending-comment"); + editor.setAnnotation(earthRange, "translator-comment", "thread-1"); + }); + + expect(logError).not.toHaveBeenCalled(); + expectDomMarkOver("earth"); + }); + }, +); + +describe("control: same lifecycle in the real Editor WITHOUT gutter para markers", () => { + it("re-applies the same range after a pending wrap/unwrap cycle (plain formatted view)", async () => { + // Passing control documenting the boundary: identical lifecycle, identical Editor, only + // `hasGutterParaMarkers` differs. (`showCharMarkerTitles: true` or + // `hasActiveTextFocusBox: true` alone also pass — verified during narrowing.) + const logError = createLoggerMock(); + const editor = await createRealEditor(formattedViewOptions, logError); + + await act(async () => { + editor.setAnnotation(earthRange, "translator-comment", "pending-comment"); + }); + await act(async () => { + editor.removeAnnotation("translator-comment", "pending-comment"); + }); + await act(async () => { + editor.setAnnotation(earthRange, "translator-comment", "thread-1"); + }); + + expect(logError).not.toHaveBeenCalled(); + expectDomMarkOver("earth"); + }); +}); + +// --------------------------------------------------------------------------- +// Helpers (after the tests; function declarations hoist) +// --------------------------------------------------------------------------- + +/** A `vi.fn()` typed to satisfy `LoggerBasic`'s `(...params: unknown[]) => void` methods. */ +type LoggerMock = ReturnType void>>; + +function createLoggerMock(): LoggerMock { + return vi.fn<(...params: unknown[]) => void>(); +} + +/** + * Mount the REAL platform Editor (full plugin stack) with the given view options and return its + * ref API — the same surface paranext-core drives. + */ +async function createRealEditor( + viewOptions: ViewOptions, + logError: LoggerMock, +): Promise { + const logger = { + error: logError, + warn: createLoggerMock(), + info: createLoggerMock(), + debug: createLoggerMock(), + }; + const ref = createRef(); + await act(async () => { + render( + , + ); + }); + if (!ref.current) throw new Error("EditorRef did not mount"); + return ref.current; +} + +/** Asserts exactly one rendered element exists and it wraps the expected text. */ +function expectDomMarkOver(expectedText: string) { + const markTexts = [...document.querySelectorAll("mark")].map((mark) => mark.textContent); + expect(markTexts).toEqual([expectedText]); +} + +/** Asserts no rendered elements remain (annotation fully removed). */ +function expectNoDomMarks() { + expect(document.querySelectorAll("mark")).toHaveLength(0); +}