From c192f2a2e11b4e88aac9cb5d9ec19ee846af6325 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 4 Jul 2026 21:17:17 +0300 Subject: [PATCH] =?UTF-8?q?test(prosemirror-markdown):=20pin=20the=20third?= =?UTF-8?q?=20state=20=E2=80=94=20explicit=20""=20converges=20once,=20then?= =?UTF-8?q?=20idempotent?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer addition to the round-trip stability matrix: besides "attr absent" and "attr has a real value", a string attr in the empty-string class has a third, degenerate state — a LITERAL "" (a user types alt/title/name in the editor then deletes it, and Tiptap persists `attr: ""`, distinct from never-set). The fix's `getAttribute(...) || null` coercion normalizes such a stored "" to the default on the FIRST round-trip (a one-time "" -> null diff) and is byte-stable from the SECOND round-trip on. Adds a convergence contract to the reusable matrix helper (emptyStringClass flag + runConvergenceCase): pass 1 must converge the attr to its schema default (NOT asserted byte-stable vs the "" input — that is the intended one-time normalization); pass 2 must deep-equal pass 1 (idempotent thereafter). Driven for every empty-string-class attr across image + the media family (image/drawio alt+title, video alt via aria-label, pdf/attachment name, attachment mime). Documents the one-time normalization so a future sync/QA diff does not flag the single "" -> null change as converter corruption. Gate: package suite 33 files / 682 tests passed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../test/roundtrip-stability.helper.ts | 138 ++++++++++++++++++ .../test/roundtrip-stability.test.ts | 56 +++++-- 2 files changed, 184 insertions(+), 10 deletions(-) diff --git a/packages/prosemirror-markdown/test/roundtrip-stability.helper.ts b/packages/prosemirror-markdown/test/roundtrip-stability.helper.ts index 3e63a7aa..f996efdf 100644 --- a/packages/prosemirror-markdown/test/roundtrip-stability.helper.ts +++ b/packages/prosemirror-markdown/test/roundtrip-stability.helper.ts @@ -50,6 +50,16 @@ export interface AttrMatrixEntry { default: unknown; /** A representative NON-default value to exercise (must survive verbatim). */ nonDefault: unknown; + /** + * Marks the attr as a member of the EMPTY-STRING class the fix targets: a + * string attr whose schema default is `null`/absent and whose parseHTML + * coerces `"" -> default` (image/drawio `alt`+`title`, video `alt` via + * aria-label, pdf/attachment `name`, attachment `mime`). Set true to also + * drive the THIRD-STATE convergence case (see runConvergenceCase) for this + * attr. Attrs whose default is NOT null (e.g. embed `provider`, default "") + * or that are not `""`-coerced (control attrs) are left unset. + */ + emptyStringClass?: boolean; } /** A node type + the attribute matrix to sweep for it. */ @@ -280,6 +290,134 @@ export function unstableCombos(report: MatrixReport): ComboResult[] { ); } +// --------------------------------------------------------------------------- +// THIRD STATE: an EXPLICITLY-STORED empty string on a string attr. +// +// The matrix above sweeps TWO states per string attr: absent/default and a +// non-default value — and asserts FIRST-pass byte-stability for both. There is +// a third, degenerate state the matrix does NOT cover: the attr stored as a +// LITERAL `""`. This is DISTINCT from "the node never had the attr": a user +// types an alt in the editor, then deletes it, and Tiptap's +// `updateAttributes({ alt: "" })` persists a literal `alt: ""` in the stored +// JSON. There is no absent-vs-"" distinction in the DOM once serialized, so the +// fix's `getAttribute("alt") || null` coercion canonicalizes BOTH to the +// default (`null`). +// +// Consequence — and this is CORRECT, not a bug: a doc carrying an explicit `""` +// converges to the default on the FIRST round-trip (a ONE-TIME diff: `"" -> +// null`), then is byte-stable from the SECOND round-trip on (idempotent). So +// this state must be pinned with a DIFFERENT contract than the matrix's: +// - do NOT assert first-pass byte-stability (the first pass legitimately +// changes `""` -> default), and +// - DO assert the first pass converges to the default AND the second pass is +// idempotent (rt2 deep-equals rt1). +// +// A future sync/QA pass diffing stored pages will see this one-time `"" -> null` +// normalization exactly once per affected node; it is the converter canon, not +// corruption, and must not be flagged as data loss. +// --------------------------------------------------------------------------- + +/** Result of the third-state ("explicit empty string") convergence probe. */ +export interface ConvergenceResult { + type: string; + attr: string; + /** The schema default the attr must converge to on pass 1 (null / absent). */ + expectedDefault: unknown; + /** rt1's materialized value for the attr — must equal `expectedDefault`. */ + firstPassValue: unknown; + /** True when the node round-tripped AND rt1 converged the attr to default. */ + convergedToDefault: boolean; + /** rt1-vs-rt2 divergence; MUST be null (idempotent from pass 2 on). */ + secondPassDivergence: { path: string; a: unknown; b: unknown } | null; + /** True when the node type failed to round-trip at all (structural loss). */ + missing: boolean; +} + +/** Round-trip a full PM doc through the real converter once. */ +async function roundtripDoc(doc: any): Promise { + return markdownToProseMirror(convertProseMirrorToMarkdown(doc)); +} + +/** + * Third-state convergence probe for one string attr of the empty-string class. + * + * (a) builds a doc with the attr EXPLICITLY set to `""` (baseAttrs + `""`), + * (b) rt1 = roundtrip(doc); asserts rt1's attr equals the schema default — the + * documented ONE-TIME `"" -> default` normalization (NOT byte-stable vs the + * `""` input, so first-pass stability is deliberately NOT asserted here), + * (c) rt2 = roundtrip(rt1); asserts rt2 deep-equals rt1 — idempotent from the + * second round-trip on. + * + * Returns a structured result (does NOT throw) so the caller can assert and + * print. Reusable across the whole node family: drive it for every attr flagged + * `emptyStringClass` on every spec (see convergenceCasesFor / the test driver). + */ +export async function runConvergenceCase( + spec: NodeStabilitySpec, + attr: string, +): Promise { + const expectedDefault = schemaDefaults(spec.type)[attr]; + + // (a) The degenerate third state: attr persisted as a LITERAL "". + const authored = { ...(spec.baseAttrs ?? {}), [attr]: "" }; + const doc = { type: "doc", content: [{ type: spec.type, attrs: authored }] }; + + // (b) First round-trip: "" must normalize to the default (a one-time diff). + const rt1 = await roundtripDoc(doc); + const node1 = findFirst(rt1, spec.type); + const firstPassValue = node1?.attrs?.[attr]; + const convergedToDefault = + node1 != null && firstDivergence(firstPassValue, expectedDefault) === null; + + // (c) Second round-trip: must be byte-stable (rt2 deep-equals rt1). We compare + // the WHOLE docs — both are converter OUTPUTS already in the same materialized + // form (numeric attrs are strings on both sides), so no numeric normalization + // is needed here, unlike the raw/canonical contours above. + const rt2 = node1 != null ? await roundtripDoc(rt1) : rt1; + const secondPassDivergence = + node1 != null ? firstDivergence(rt1, rt2) : null; + + return { + type: spec.type, + attr, + expectedDefault, + firstPassValue, + convergedToDefault, + secondPassDivergence, + missing: node1 == null, + }; +} + +/** The attrs of a spec flagged as members of the empty-string class. */ +export function convergenceCasesFor(spec: NodeStabilitySpec): string[] { + return spec.attrMatrix + .filter((e) => e.emptyStringClass) + .map((e) => e.attr); +} + +/** True when a convergence result honours the "converges once, then stable" contract. */ +export function convergenceOk(r: ConvergenceResult): boolean { + return !r.missing && r.convergedToDefault && r.secondPassDivergence === null; +} + +/** Render a convergence result as a legible one-liner for a failed assertion. */ +export function formatConvergence(r: ConvergenceResult): string { + if (r.missing) return `${r.type}.${r.attr}: DID-NOT-ROUND-TRIP`; + const parts: string[] = []; + if (!r.convergedToDefault) { + parts.push( + `pass1 did NOT converge: got ${JSON.stringify(r.firstPassValue)} (expected default ${JSON.stringify(r.expectedDefault)})`, + ); + } + if (r.secondPassDivergence) { + parts.push( + `pass2 NOT idempotent @ ${r.secondPassDivergence.path}: ${JSON.stringify(r.secondPassDivergence.a)} vs ${JSON.stringify(r.secondPassDivergence.b)}`, + ); + } + const status = parts.length === 0 ? "converges-once-then-stable" : parts.join("; "); + return `${r.type}.${r.attr}: ${status}`; +} + /** Render a report as a legible multi-line string for a failed assertion. */ export function formatReport(report: MatrixReport): string { const lines: string[] = [`node "${report.type}":`]; diff --git a/packages/prosemirror-markdown/test/roundtrip-stability.test.ts b/packages/prosemirror-markdown/test/roundtrip-stability.test.ts index bd3464bc..57b91b23 100644 --- a/packages/prosemirror-markdown/test/roundtrip-stability.test.ts +++ b/packages/prosemirror-markdown/test/roundtrip-stability.test.ts @@ -3,6 +3,10 @@ import { runStabilityMatrix, unstableCombos, formatReport, + runConvergenceCase, + convergenceCasesFor, + convergenceOk, + formatConvergence, type NodeStabilitySpec, } from "./roundtrip-stability.helper.js"; @@ -31,8 +35,8 @@ const SPECS: NodeStabilitySpec[] = [ type: "image", baseAttrs: { src: "/i.png" }, attrMatrix: [ - { attr: "alt", default: undefined, nonDefault: "a real alt text" }, - { attr: "title", default: undefined, nonDefault: "a real title" }, + { attr: "alt", default: undefined, nonDefault: "a real alt text", emptyStringClass: true }, + { attr: "title", default: undefined, nonDefault: "a real title", emptyStringClass: true }, { attr: "caption", default: undefined, nonDefault: "a real caption" }, { attr: "attachmentId", default: undefined, nonDefault: "att-42" }, ], @@ -42,7 +46,7 @@ const SPECS: NodeStabilitySpec[] = [ type: "video", baseAttrs: { src: "/v.mp4" }, attrMatrix: [ - { attr: "alt", default: undefined, nonDefault: "a clip" }, + { attr: "alt", default: undefined, nonDefault: "a clip", emptyStringClass: true }, { attr: "attachmentId", default: undefined, nonDefault: "att-1" }, ], }, @@ -59,7 +63,7 @@ const SPECS: NodeStabilitySpec[] = [ type: "pdf", baseAttrs: { src: "/d.pdf" }, attrMatrix: [ - { attr: "name", default: undefined, nonDefault: "report.pdf" }, + { attr: "name", default: undefined, nonDefault: "report.pdf", emptyStringClass: true }, { attr: "attachmentId", default: undefined, nonDefault: "att-3" }, ], }, @@ -68,8 +72,8 @@ const SPECS: NodeStabilitySpec[] = [ type: "attachment", baseAttrs: { url: "/f.zip" }, attrMatrix: [ - { attr: "name", default: undefined, nonDefault: "bundle.zip" }, - { attr: "mime", default: undefined, nonDefault: "application/zip" }, + { attr: "name", default: undefined, nonDefault: "bundle.zip", emptyStringClass: true }, + { attr: "mime", default: undefined, nonDefault: "application/zip", emptyStringClass: true }, { attr: "attachmentId", default: undefined, nonDefault: "att-4" }, ], }, @@ -98,8 +102,8 @@ const SPECS: NodeStabilitySpec[] = [ type: "drawio", baseAttrs: { src: "blob:drawio" }, attrMatrix: [ - { attr: "title", default: undefined, nonDefault: "flow chart" }, - { attr: "alt", default: undefined, nonDefault: "an alt" }, + { attr: "title", default: undefined, nonDefault: "flow chart", emptyStringClass: true }, + { attr: "alt", default: undefined, nonDefault: "an alt", emptyStringClass: true }, { attr: "attachmentId", default: undefined, nonDefault: "att-5" }, ], }, @@ -108,8 +112,8 @@ const SPECS: NodeStabilitySpec[] = [ type: "excalidraw", baseAttrs: { src: "blob:excalidraw" }, attrMatrix: [ - { attr: "title", default: undefined, nonDefault: "sketch" }, - { attr: "alt", default: undefined, nonDefault: "an alt" }, + { attr: "title", default: undefined, nonDefault: "sketch", emptyStringClass: true }, + { attr: "alt", default: undefined, nonDefault: "an alt", emptyStringClass: true }, { attr: "attachmentId", default: undefined, nonDefault: "att-6" }, ], }, @@ -126,3 +130,35 @@ describe("round-trip stability matrix (image + media family)", () => { }); } }); + +// --------------------------------------------------------------------------- +// THIRD STATE: an attr EXPLICITLY stored as a literal "" (GS-EDIT-REVERT: a user +// typed alt/title/name/... then deleted it, so Tiptap persisted `attr: ""` — a +// value DISTINCT from "attr was never set"). Unlike the absent case above, this +// state is NOT first-pass byte-stable: the fix's `"" -> default` coercion is a +// deliberate ONE-TIME normalization on the FIRST sync round-trip, stable +// thereafter. We therefore assert a DIFFERENT contract — "converges to default +// on pass 1, then idempotent from pass 2 on" — for every empty-string-class attr +// across the whole node family (image/video/pdf/attachment/drawio/excalidraw). +// +// IMPORTANT for a future sync/QA pass: the pass-1 `"" -> null` diff is the +// converter canon, not corruption. It appears at most once per affected node and +// must NOT be flagged as "the converter is losing/corrupting page data". +// --------------------------------------------------------------------------- +describe("round-trip third state: explicit empty string converges once, then idempotent", () => { + for (const spec of SPECS) { + for (const attr of convergenceCasesFor(spec)) { + it(`${spec.type}.${attr}: "" normalizes to default on pass 1, byte-stable from pass 2`, async () => { + const r = await runConvergenceCase(spec, attr); + // Pass 1 must converge "" -> the schema default (the one-time diff) and + // pass 2 (roundtrip of pass-1 output) must be byte-stable. formatConvergence + // prints exactly which half failed. + expect(convergenceOk(r), `\n${formatConvergence(r)}\n`).toBe(true); + // Spell the contract out explicitly so the intent is legible in the test: + expect(r.convergedToDefault, `\n${formatConvergence(r)}\n`).toBe(true); + expect(r.firstPassValue).toEqual(r.expectedDefault); + expect(r.secondPassDivergence, `\n${formatConvergence(r)}\n`).toBeNull(); + }); + } + } +});