From 5bd5995ef0f619e8575cf8e46d325b3d1c8cd810 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 02:43:53 +0300 Subject: [PATCH] fix(prosemirror-markdown): coalesce list seams on insert (#535) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit insertNode with a markdown/JSON list now appends/prepends its items into an adjacent same-type sibling list instead of creating a separate sibling list that the serializer splits with ``. Adds a single shared merge rule (isCoalescibleList/listsMergeable) used by both the markdown path (insertNodesRelative) and the raw-node path (insertNodeRelative). Coalescing is strictly local to the two seams of the active insertion (each merged at most once — no greedy loop, no global normalization). Survivor is chosen POSITIONALLY (the pre-existing neighbour outside the inserted [i,j) range), so it keeps its block id and list-level attrs (e.g. orderedList start); the inserted wrapper's re-minted id is discarded. Handles the three-way case (list inserted between two same-type lists → left survives) and refuses to coalesce an empty inserted list. before/after now resolve via findAnchorChain and splice into the anchor's immediate parent, so coalescing runs against the actual parent array (incl. lists nested in callouts / table cells). Serializer / LIST_MARKER_SEPARATOR untouched — two genuinely-separate lists still emit ``. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../prosemirror-markdown/src/lib/node-ops.ts | 207 +++++++---- .../test/node-ops-list-coalesce.test.ts | 344 ++++++++++++++++++ 2 files changed, 483 insertions(+), 68 deletions(-) create mode 100644 packages/prosemirror-markdown/test/node-ops-list-coalesce.test.ts diff --git a/packages/prosemirror-markdown/src/lib/node-ops.ts b/packages/prosemirror-markdown/src/lib/node-ops.ts index 6d88e3a3..55aa13b2 100644 --- a/packages/prosemirror-markdown/src/lib/node-ops.ts +++ b/packages/prosemirror-markdown/src/lib/node-ops.ts @@ -633,6 +633,112 @@ function findAnchorChain( return null; } +// =========================================================================== +// List seam coalescing (#535) +// +// When a markdown/JSON insert places a list block directly next to an existing +// sibling list of the SAME node type, the two must become ONE list (items +// appended/prepended) rather than two adjacent lists — otherwise the serializer +// correctly emits a `` separator between them (that separator is right +// for two genuinely-separate lists; the bug is that the extra sibling was ever +// created). Coalescing is STRICTLY LOCAL to the two seams of the active +// insertion — never a global "collapse all adjacent lists" normalization, which +// would destroy intentionally-separate lists elsewhere. +// =========================================================================== + +/** + * The three list container types we structurally coalesce at an insertion seam. + * Deliberately an explicit set, NOT a `type.endsWith("List")` test — that also + * matches `footnotesList`, which must NEVER be structurally merged. + */ +function isCoalescibleList(n: any): boolean { + return ( + isObject(n) && + (n.type === "bulletList" || + n.type === "orderedList" || + n.type === "taskList") + ); +} + +/** + * True when two adjacent list nodes may have their ITEMS merged into one list. + * Requires identical node type and, for orderedList, a compatible numbering + * style: a missing/null `attrs.type` counts as the default, and the merge is + * blocked ONLY when both lists carry an explicit, differing `attrs.type`. + */ +function listsMergeable(a: any, b: any): boolean { + if (!isCoalescibleList(a) || !isCoalescibleList(b)) return false; + if (a.type !== b.type) return false; + if (!Array.isArray(a.content) || !Array.isArray(b.content)) return false; + if (a.type === "orderedList") { + const ta = a.attrs?.type ?? null; + const tb = b.attrs?.type ?? null; + if (ta != null && tb != null && ta !== tb) return false; + } + return true; +} + +/** + * Coalesce list seams around a freshly inserted run occupying indices `[i, j)` + * in `parent`. At MOST the left seam (`parent[i-1]` ↔ `parent[i]`) and the right + * seam (`parent[j-1]` ↔ `parent[j]`) are merged, each AT MOST ONCE — never a + * `while (neighbours same type) merge` loop, which would swallow a further-out + * intentionally-separate list. Mutates `parent` in place. + * + * Survivor choice is POSITIONAL, never by id: the neighbour OUTSIDE the `[i, j)` + * range is pre-existing (the survivor, keeping its block id and list-level + * attrs); the boundary block INSIDE the range is the freshly inserted list, + * whose items move into the survivor and whose wrapper is then deleted + * (appended when the survivor is on the left, prepended when on the right). + * + * Empty inserted list: if the inserted boundary list has zero items, its seam is + * NOT coalesced — the block is left exactly as inserted. + */ +function coalesceSeams(parent: any[], i: number, j: number): void { + if (!Array.isArray(parent)) return; + const n = parent.length; + + const left = parent[i - 1]; + const boundaryLeft = parent[i]; + const boundaryRight = parent[j - 1]; + const right = parent[j]; + + // A seam fires only when the pre-existing neighbour and the inserted boundary + // list are mergeable AND the inserted boundary list is non-empty. + const leftMergeable = + i - 1 >= 0 && + listsMergeable(left, boundaryLeft) && + boundaryLeft.content.length > 0; + const rightMergeable = + j < n && + listsMergeable(boundaryRight, right) && + boundaryRight.content.length > 0; + + // Three-way collision: a SINGLE inserted list (i === j-1) landed exactly + // between two pre-existing same-type lists. The LEFT pre-existing list wins: + // the inserted items then the right list's items fold into it, and both the + // inserted wrapper and the right pre-existing list are deleted (the right + // block id is NOT preserved — rare, documented). + if (i === j - 1 && leftMergeable && rightMergeable) { + left.content.push(...boundaryLeft.content, ...right.content); + parent.splice(i, 2); + return; + } + + // Otherwise the two seams are independent. Process the RIGHT seam FIRST so its + // deletion at the higher indices cannot shift the left seam's [i-1, i]. + if (rightMergeable) { + // Survivor is the pre-existing right neighbour; PREPEND the inserted items. + right.content.unshift(...boundaryRight.content); + parent.splice(j - 1, 1); + } + if (leftMergeable) { + // Survivor is the pre-existing left neighbour; APPEND the inserted items. + left.content.push(...boundaryLeft.content); + parent.splice(i, 1); + } +} + /** Options controlling where `insertNodeRelative` places the new node. */ export interface InsertOptions { position: "before" | "after" | "append"; @@ -685,7 +791,10 @@ export function insertNodeRelative( } if (isObject(out)) { if (!Array.isArray(out.content)) out.content = []; + const at = out.content.length; out.content.push(fresh); + // Coalesce the left seam with the prior tail block (#535). + coalesceSeams(out.content, at, out.content.length); return { doc: out, inserted: true }; } return { doc: out, inserted: false }; @@ -737,40 +846,19 @@ export function insertNodeRelative( return { doc: out, inserted: true }; } - // Resolve by id anywhere in the tree: splice into the parent content array. - if (opts.anchorNodeId != null) { - let inserted = false; - const walkContent = (content: any[]): void => { - for (let i = 0; i < content.length; i++) { - const child = content[i]; - if (matchesId(child, opts.anchorNodeId as string)) { - content.splice(i + offset, 0, fresh); - inserted = true; - return; - } - if (isObject(child) && Array.isArray(child.content)) { - walkContent(child.content); - if (inserted) return; - } - } - }; - if (isObject(out) && Array.isArray(out.content)) { - walkContent(out.content); - } - return { doc: out, inserted }; - } - - // Resolve by text: only top-level doc.content blocks are scanned. Exact - // match wins; a markdown-stripped fallback is tried only on a miss. - if (opts.anchorText != null && isObject(out) && Array.isArray(out.content)) { - const i = findAnchorTextIndex(out.content, opts.anchorText); - if (i !== -1) { - out.content.splice(i + offset, 0, fresh); - return { doc: out, inserted: true }; - } - } - - return { doc: out, inserted: false }; + // before/after (non-structural): resolve the anchor's ancestor chain and + // splice into the anchor's IMMEDIATE parent, so seam coalescing runs against + // the ACTUAL parent array (also correctly handling a list nested in a callout + // / table cell). The first-match / top-level-only semantics of anchorNodeId / + // anchorText are preserved by findAnchorChain (identical to the old walk). + const chain = findAnchorChain(out, opts); + if (chain == null || chain.length < 2) return { doc: out, inserted: false }; + const parent = chain[chain.length - 2].node.content; + if (!Array.isArray(parent)) return { doc: out, inserted: false }; + const at = chain[chain.length - 1].index + offset; + parent.splice(at, 0, fresh); + coalesceSeams(parent, at, at + 1); + return { doc: out, inserted: true }; } /** @@ -811,7 +899,11 @@ export function insertNodesRelative( if (opts.position === "append") { if (isObject(out)) { if (!Array.isArray(out.content)) out.content = []; + const at = out.content.length; out.content.push(...fresh); + // Coalesce the left seam with the prior tail block; the run's right side + // has no neighbour at the top level (#535). + coalesceSeams(out.content, at, out.content.length); return { doc: out, inserted: true }; } return { doc: out, inserted: false }; @@ -819,40 +911,19 @@ export function insertNodesRelative( const offset = opts.position === "after" ? 1 : 0; - // Resolve by id anywhere in the tree: splice the whole array into the parent. - if (opts.anchorNodeId != null) { - let inserted = false; - const walkContent = (content: any[]): void => { - for (let i = 0; i < content.length; i++) { - const child = content[i]; - if (matchesId(child, opts.anchorNodeId as string)) { - content.splice(i + offset, 0, ...fresh); - inserted = true; - return; - } - if (isObject(child) && Array.isArray(child.content)) { - walkContent(child.content); - if (inserted) return; - } - } - }; - if (isObject(out) && Array.isArray(out.content)) { - walkContent(out.content); - } - return { doc: out, inserted }; - } - - // Resolve by text: only top-level doc.content blocks are scanned. Exact match - // wins; a markdown-stripped fallback is tried only on a miss. - if (opts.anchorText != null && isObject(out) && Array.isArray(out.content)) { - const i = findAnchorTextIndex(out.content, opts.anchorText); - if (i !== -1) { - out.content.splice(i + offset, 0, ...fresh); - return { doc: out, inserted: true }; - } - } - - return { doc: out, inserted: false }; + // before/after: resolve the anchor's ancestor chain and splice the whole run + // into the anchor's IMMEDIATE parent, then coalesce ONLY the run's two + // boundary seams (inner blocks are untouched). Converting to findAnchorChain + // makes coalescing run against the ACTUAL parent array (incl. lists nested in + // callouts / table cells); first-match / top-level-only semantics preserved. + const chain = findAnchorChain(out, opts); + if (chain == null || chain.length < 2) return { doc: out, inserted: false }; + const parent = chain[chain.length - 2].node.content; + if (!Array.isArray(parent)) return { doc: out, inserted: false }; + const at = chain[chain.length - 1].index + offset; + parent.splice(at, 0, ...fresh); + coalesceSeams(parent, at, at + fresh.length); + return { doc: out, inserted: true }; } // =========================================================================== diff --git a/packages/prosemirror-markdown/test/node-ops-list-coalesce.test.ts b/packages/prosemirror-markdown/test/node-ops-list-coalesce.test.ts new file mode 100644 index 00000000..17ea1b3c --- /dev/null +++ b/packages/prosemirror-markdown/test/node-ops-list-coalesce.test.ts @@ -0,0 +1,344 @@ +import { describe, expect, it } from "vitest"; +import { + insertNodesRelative, + insertNodeRelative, +} from "../src/lib/node-ops.js"; +import { markdownToProseMirrorSync } from "../src/lib/markdown-to-prosemirror.js"; +import { convertProseMirrorToMarkdown } from "../src/lib/markdown-converter.js"; + +// #535: inserting a markdown/JSON list next to an existing same-type sibling +// list must APPEND/PREPEND its items into that list (one list), not leave two +// adjacent lists that the serializer separates with ``. Coalescing is +// strictly local to the two seams of the active insertion. + +// ---- Minimal builders (structure is what these tests assert) --------------- + +function item(label: string | number): any { + return { + type: "listItem", + content: [ + { type: "paragraph", content: [{ type: "text", text: String(label) }] }, + ], + }; +} +function taskItem(label: string | number, checked = false): any { + return { + type: "taskItem", + attrs: { checked }, + content: [ + { type: "paragraph", content: [{ type: "text", text: String(label) }] }, + ], + }; +} +function bulletList(id: string, ...labels: (string | number)[]): any { + return { type: "bulletList", attrs: { id }, content: labels.map(item) }; +} +function orderedList( + id: string, + start: number, + attrsType: string | null, + ...labels: (string | number)[] +): any { + return { + type: "orderedList", + attrs: { id, start, type: attrsType }, + content: labels.map(item), + }; +} +function taskList(id: string, ...labels: (string | number)[]): any { + return { type: "taskList", attrs: { id }, content: labels.map((l) => taskItem(l)) }; +} + +/** Read a list block's item labels (the text of each item's first paragraph). */ +function labels(list: any): string[] { + return (list.content ?? []).map( + (it: any) => it.content?.[0]?.content?.[0]?.text, + ); +} + +describe("#535 list seam coalescing — markdown path (insertNodesRelative)", () => { + it("crit1: append a bulletList onto an adjacent bulletList -> ONE list", () => { + const doc = { type: "doc", content: [bulletList("A", 1, 2, 3)] }; + const { doc: out, inserted } = insertNodesRelative( + doc, + [bulletList("B", 4)], + { position: "append" }, + ); + expect(inserted).toBe(true); + // Exactly one block, and it is the pre-existing survivor (id "A"). + expect(out.content.length).toBe(1); + expect(out.content[0].type).toBe("bulletList"); + expect(out.content[0].attrs.id).toBe("A"); + expect(labels(out.content[0])).toEqual(["1", "2", "3", "4"]); + }); + + it("crit2: append an orderedList -> ONE list, original start kept, 1..4 continuous, no start:1 sibling", () => { + const doc = { type: "doc", content: [orderedList("A", 1, null, 1, 2, 3)] }; + const { doc: out } = insertNodesRelative( + doc, + [orderedList("B", 1, null, 4)], + { position: "append" }, + ); + expect(out.content.length).toBe(1); + expect(out.content[0].type).toBe("orderedList"); + expect(out.content[0].attrs.start).toBe(1); + expect(labels(out.content[0])).toEqual(["1", "2", "3", "4"]); + }); + + it("crit3: after/before by anchorNodeId on a same-type list -> one merged list", () => { + const docA = { type: "doc", content: [bulletList("A", 1, 2)] }; + const afterRes = insertNodesRelative(docA, [bulletList("B", 3)], { + position: "after", + anchorNodeId: "A", + }); + expect(afterRes.doc.content.length).toBe(1); + expect(labels(afterRes.doc.content[0])).toEqual(["1", "2", "3"]); + + const docB = { type: "doc", content: [bulletList("A", 2, 3)] }; + const beforeRes = insertNodesRelative(docB, [bulletList("B", 1)], { + position: "before", + anchorNodeId: "A", + }); + expect(beforeRes.doc.content.length).toBe(1); + // Survivor is the pre-existing "A"; inserted items PREPEND. + expect(beforeRes.doc.content[0].attrs.id).toBe("A"); + expect(labels(beforeRes.doc.content[0])).toEqual(["1", "2", "3"]); + }); + + it("crit3: anchorText resolving to a list block also merges", () => { + const doc = { type: "doc", content: [bulletList("A", "alpha", "beta")] }; + const { doc: out } = insertNodesRelative(doc, [bulletList("B", "gamma")], { + position: "after", + anchorText: "alpha", + }); + expect(out.content.length).toBe(1); + expect(labels(out.content[0])).toEqual(["alpha", "beta", "gamma"]); + }); + + it("crit4: both seams (three-way) — inserting a list between two same-type lists -> ONE list in order", () => { + const doc = { + type: "doc", + content: [bulletList("A", 1, 2), bulletList("C", 4, 5)], + }; + const { doc: out } = insertNodesRelative(doc, [bulletList("B", 3)], { + position: "after", + anchorNodeId: "A", + }); + expect(out.content.length).toBe(1); + // LEFT pre-existing "A" survives; order A-items, inserted, C-items. + expect(out.content[0].attrs.id).toBe("A"); + expect(labels(out.content[0])).toEqual(["1", "2", "3", "4", "5"]); + }); + + it("crit4: multi-block run coalesces BOTH boundary seams, inner block untouched", () => { + // parent = [preA, Bi, para, Bj, preB]; run [Bi, para, Bj] inserted after preA. + const doc = { + type: "doc", + content: [bulletList("A", 1), bulletList("E", 9)], + }; + const run = [ + bulletList("Bi", 2), + { type: "paragraph", content: [{ type: "text", text: "mid" }] }, + bulletList("Bj", 8), + ]; + const { doc: out } = insertNodesRelative(doc, run, { + position: "after", + anchorNodeId: "A", + }); + // Expect: [A(1,2), para(mid), E(8,9)] — preA absorbed Bi, preE absorbed Bj. + expect(out.content.map((n: any) => n.type)).toEqual([ + "bulletList", + "paragraph", + "bulletList", + ]); + expect(labels(out.content[0])).toEqual(["1", "2"]); + expect(out.content[1].content[0].text).toBe("mid"); + expect(labels(out.content[2])).toEqual(["8", "9"]); + expect(out.content[0].attrs.id).toBe("A"); + expect(out.content[2].attrs.id).toBe("E"); + }); + + it("crit6: before for orderedList{start:5} keeps start:5 (positional survivor = pre-existing)", () => { + const doc = { type: "doc", content: [orderedList("A", 5, null, "a", "b")] }; + const { doc: out } = insertNodesRelative( + doc, + [orderedList("B", 1, null, "c")], + { position: "before", anchorNodeId: "A" }, + ); + expect(out.content.length).toBe(1); + expect(out.content[0].attrs.id).toBe("A"); + expect(out.content[0].attrs.start).toBe(5); + expect(labels(out.content[0])).toEqual(["c", "a", "b"]); + }); + + it("crit7: different list types do NOT coalesce", () => { + // bulletList next to orderedList + const d1 = { type: "doc", content: [bulletList("A", 1)] }; + const r1 = insertNodesRelative(d1, [orderedList("B", 1, null, 2)], { + position: "append", + }); + expect(r1.doc.content.length).toBe(2); + expect(r1.doc.content.map((n: any) => n.type)).toEqual([ + "bulletList", + "orderedList", + ]); + + // bulletList next to taskList + const d2 = { type: "doc", content: [bulletList("A", 1)] }; + const r2 = insertNodesRelative(d2, [taskList("B", 2)], { + position: "append", + }); + expect(r2.doc.content.length).toBe(2); + expect(r2.doc.content.map((n: any) => n.type)).toEqual([ + "bulletList", + "taskList", + ]); + }); + + it("crit7b: orderedLists with explicit DIFFERING attrs.type do NOT coalesce", () => { + const doc = { type: "doc", content: [orderedList("A", 1, "a", 1)] }; + const { doc: out } = insertNodesRelative( + doc, + [orderedList("B", 1, "i", 2)], + { position: "append" }, + ); + expect(out.content.length).toBe(2); + }); + + it("taskList next to taskList coalesces, item checked attrs move with items", () => { + const doc = { + type: "doc", + content: [ + { + type: "taskList", + attrs: { id: "A" }, + content: [taskItem("x", true)], + }, + ], + }; + const { doc: out } = insertNodesRelative( + doc, + [{ type: "taskList", attrs: { id: "B" }, content: [taskItem("y", false)] }], + { position: "append" }, + ); + expect(out.content.length).toBe(1); + expect(out.content[0].content.map((it: any) => it.attrs.checked)).toEqual([ + true, + false, + ]); + }); +}); + +describe("#535 list seam coalescing — node path (insertNodeRelative)", () => { + it("append a single bulletList node onto an adjacent bulletList -> ONE list", () => { + const doc = { type: "doc", content: [bulletList("A", 1, 2)] }; + const { doc: out } = insertNodeRelative(doc, bulletList("B", 3), { + position: "append", + }); + expect(out.content.length).toBe(1); + expect(labels(out.content[0])).toEqual(["1", "2", "3"]); + }); + + it("crit8: EMPTY inserted list is NOT coalesced (stays as inserted)", () => { + const doc = { type: "doc", content: [bulletList("A", 1, 2)] }; + const empty = { type: "bulletList", attrs: { id: "B" }, content: [] }; + const { doc: out, inserted } = insertNodeRelative(doc, empty, { + position: "append", + }); + expect(inserted).toBe(true); + expect(out.content.length).toBe(2); + expect(out.content[1].attrs.id).toBe("B"); + expect(out.content[1].content.length).toBe(0); + }); + + it("crit5b: nested parent — merging a list next to a list INSIDE a callout, top level untouched", () => { + const doc = { + type: "doc", + content: [ + { + type: "callout", + attrs: { id: "co" }, + content: [bulletList("A", 1, 2)], + }, + ], + }; + const { doc: out } = insertNodeRelative(doc, bulletList("B", 3), { + position: "after", + anchorNodeId: "A", + }); + // Top level unchanged: still one callout. + expect(out.content.length).toBe(1); + expect(out.content[0].type).toBe("callout"); + // The callout's content array holds ONE merged bulletList. + expect(out.content[0].content.length).toBe(1); + expect(out.content[0].content[0].type).toBe("bulletList"); + expect(labels(out.content[0].content[0])).toEqual(["1", "2", "3"]); + }); +}); + +describe("#535 locality (regression guard) + round-trip", () => { + it("crit5a: appending next to ONE of two intentionally-separate lists merges only that seam; the further-out separate list survives and still emits ", () => { + // Reimport a doc with two deliberately-separate bullet lists: [(x,y),(z)]. + const doc = markdownToProseMirrorSync("- x\n- y\n\n\n\n- z"); + expect(doc.content.filter((n: any) => n.type === "bulletList").length).toBe( + 2, + ); + + // Append a list. It lands at the end, adjacent to the SECOND list (z) and + // must merge ONLY into it — the FIRST list (x,y) is the intentionally- + // separate one two hops away and must NOT be swallowed (a greedy `while` + // merge would reach and collapse it, which this guards against). + const { doc: out, inserted } = insertNodesRelative( + doc, + [bulletList("NEW", "q")], + { position: "append" }, + ); + expect(inserted).toBe(true); + + // Still TWO lists: (x,y) stayed separate; (z) absorbed the appended item. + expect(out.content.filter((n: any) => n.type === "bulletList").length).toBe( + 2, + ); + expect(labels(out.content[0])).toEqual(["x", "y"]); + expect(labels(out.content[1])).toEqual(["z", "q"]); + + // Serialize: the separator between the two separate lists remains. + const md = convertProseMirrorToMarkdown(out); + expect(md).toContain(""); + }); + + it("idempotency: re-running the transform on a fresh doc yields the same result", () => { + const doc = { type: "doc", content: [bulletList("A", 1, 2, 3)] }; + const r1 = insertNodesRelative(doc, [bulletList("B", 4)], { + position: "append", + }); + const r2 = insertNodesRelative(doc, [bulletList("B", 4)], { + position: "append", + }); + expect(r2.doc).toEqual(r1.doc); + // Input never mutated. + expect(doc.content.length).toBe(1); + expect(labels(doc.content[0])).toEqual(["1", "2", "3"]); + }); + + it("#351 P1/P2: a merged list serializes to one list and is byte-fixpoint on the 2nd pass", () => { + // Build a merged bulletList via a canonical import so the round-trip is real. + const base = markdownToProseMirrorSync("- one\n- two"); + const add = markdownToProseMirrorSync("- three"); + const { doc: merged } = insertNodesRelative(base, add.content, { + position: "append", + }); + expect(merged.content.length).toBe(1); + + const md1 = convertProseMirrorToMarkdown(merged); + // A single list — no separator inside. + expect(md1).not.toContain(""); + expect(md1).toBe("- one\n- two\n- three"); + + // Reimport -> still ONE bulletList; re-serialize -> byte-identical (fixpoint). + const reimported = markdownToProseMirrorSync(md1); + expect(reimported.content.filter((n: any) => n.type === "bulletList").length).toBe(1); + const md2 = convertProseMirrorToMarkdown(reimported); + expect(md2).toBe(md1); + }); +});