From 7b70a3048b7b46b7c8f8e67f578fe015e6934780 Mon Sep 17 00:00:00 2001 From: dormouse-bot <287024035+dormouse-bot@users.noreply.github.com> Date: Tue, 20 Oct 2026 23:31:26 +0000 Subject: [PATCH 1/4] fix: let source edits to an unparseable Markdown file be saved MDXEditor clears its initial-normalization flag only in its rich-text export, which a parse error suppresses, so every source edit reached the viewer as normalization and rebased the session clean: Save stayed disabled. Once the editor reports a parse error, its changes count as edits until the next load. Refs #1254 --- dor-tools-builtin/viewer/markdown.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dor-tools-builtin/viewer/markdown.tsx b/dor-tools-builtin/viewer/markdown.tsx index 1dc4e3812..c737bf9fd 100644 --- a/dor-tools-builtin/viewer/markdown.tsx +++ b/dor-tools-builtin/viewer/markdown.tsx @@ -90,6 +90,7 @@ const session = documentSession({ async apply(text) { // A reload mounts a fresh editor: its own normalization is then not an edit, and undo starts over. uploadedWidths.clear(); + unparsed = false; flushSync(() => root.render()); await new Promise(resolve => setTimeout(resolve)); }, @@ -255,9 +256,14 @@ function markdownStyle(markdown: string) { return { bullet, rule: count(/^\*{3,}[ \t]*$/gm) > count(/^-{3,}[ \t]*$/gm) ? '*' as const : '-' as const }; } +/** Whether this editor failed to parse its document. MDXEditor clears its + * normalize flag only in a rich-text export, which a parse error suppresses, + * so every later source edit would otherwise arrive as normalization. */ +let unparsed = false; // The editor's own reformatting of loaded text is not an edit. -const onEditorChange: MDXEditorProps['onChange'] = (_, normalize) => { if (normalize) session.rebase(); else session.changed(); }; +const onEditorChange: MDXEditorProps['onChange'] = (_, normalize) => { if (normalize && !unparsed) session.rebase(); else session.changed(); }; const onEditorError: MDXEditorProps['onError'] = ({ error: reason }) => { + unparsed = true; realm?.pub(viewMode$, 'source'); session.fail(`Showing source: the rich editor cannot parse this file (${reason}).`); }; From 8f9e8f8bd393050364fefd3982cd15e15218552e Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 20 Oct 2026 16:49:58 -0700 Subject: [PATCH 2/4] fix: preserve Markdown labels, entities, and heading spacing --- docs/specs/dor-tools-builtin.md | 4 +- docs/specs/dor-tools-builtin.rationale.md | 2 + .../test/markdown-format.test.mjs | 69 +++++++++++++++++++ dor-tools-builtin/viewer/markdown-format.ts | 57 +++++++++++++++ dor-tools-builtin/viewer/markdown.tsx | 12 +--- scripts/spec-word-budgets.json | 2 +- 6 files changed, 133 insertions(+), 13 deletions(-) create mode 100644 dor-tools-builtin/test/markdown-format.test.mjs create mode 100644 dor-tools-builtin/viewer/markdown-format.ts diff --git a/docs/specs/dor-tools-builtin.md b/docs/specs/dor-tools-builtin.md index 39d4b8b4e..346f3ea81 100644 --- a/docs/specs/dor-tools-builtin.md +++ b/docs/specs/dor-tools-builtin.md @@ -44,8 +44,8 @@ Source of truth: `saveText` / `StorePath` in `dor-tools-builtin/src/editor-route `.md` and `.markdown` open in the bundled MDXEditor page; `.mdx` and other text stay in Monaco. Saving, reloading, and dirty reports follow [Editing files](#editing-files). - **Must resolve the viewer's direct `lexical` and `@lexical/link` imports to the same installed modules MDXEditor uses** (rationale). -- **Must support GFM tables, task lists, frontmatter, code blocks, fenced `mermaid` diagrams, and a whole-document source mode**, which opens with the error when the rich editor cannot parse a file. Mermaid stays on 11 (rationale). -- **Must keep HTML comments verbatim and save with one final newline, the file's majority bullet and thematic-break marker, and unpadded tables**; the rest follows MDXEditor's serialization. Its normalization of loaded text is not an edit (rationale). +- **Must support GFM tables, task lists, frontmatter, code blocks, fenced `mermaid` diagrams, and a whole-document source mode**, which opens with the error when the rich editor cannot parse a file. **Must let source edits remain dirty and saveable after a parse error, and allow repaired source back into rich text.** Mermaid stays on 11 (rationale). +- **Must keep HTML comments verbatim and save with one final newline, the file's majority bullet and thematic-break marker, and unpadded tables**. **Must retain simple standalone bracket labels, encode literal angle brackets in text as entities, and preserve recognized headings' existing blank-line choice**, with new or renamed headings following the majority style; ambiguous Markdown keeps the serializer's escapes. The rest follows MDXEditor's serialization. Its normalization of loaded text is not an edit (rationale). - **Never let document HTML reach the live DOM outside an allowlist** of tags and attributes, `href` limited to `http(s):`, `mailto:`, fragments, and scheme-less paths; saving keeps every original attribute, escaped, and HTML images serialize in an inert document. Mermaid diagrams are the one other path, under mermaid's own sanitizer (`docs/specs/security-local.md` → "Local-file viewer"; rationale). - **Must serve images on request from regular image-format files at or under the document's canonical directory**, by realpath, under a `sandbox` CSP. Nothing else loads, external URLs included. - **Never show an image at a size the document does not state**: unsized, it shows pixel for pixel, within the column. A pasted or dropped PNG or JPEG with a recorded scale goes in at its pixel width over that scale (rationale). diff --git a/docs/specs/dor-tools-builtin.rationale.md b/docs/specs/dor-tools-builtin.rationale.md index 00eb8b878..a5ee2a723 100644 --- a/docs/specs/dor-tools-builtin.rationale.md +++ b/docs/specs/dor-tools-builtin.rationale.md @@ -22,6 +22,8 @@ MDXEditor 4.3.3 depends on Lexical `^0.48.0`; independent direct bumps to 0.52 i MDXEditor 4.3 parses `` and discards it, and trims and re-serializes the whole document on export; without the comment node and style detection, saving a typical README after a one-word edit deleted its comments and rewrote every `-` bullet as `*` (measured 2026-10). +Issue #1254 (2026-10) exposed unconditional bracket escaping, literal angle brackets changing spelling across rich/source imports, and blank lines inserted after every heading. Serializer options keep simple labels and per-heading spacing without retaining a parallel source tree, while entity-encoding text brackets survives MDX JSX parsing. Code and HTML still use their own serializers. MDXEditor also leaves its initial-normalization flag set after a failed import; treating source changes as edits after that failure permits repair and save. + Image sizes live in the document because GitHub shows an `` without a `width` pixel for pixel: a Retina screenshot shown here at its recorded scale but left unsized in the file came out twice as large on GitHub. Writing the width on paste keeps the two renderings alike (2026-10). Image URLs carry a per-load generation because the browser keeps decoded images by URL: a reload that only remounted the editor still showed an image's old pixels after the file changed on disk (observed live, 2026-10). diff --git a/dor-tools-builtin/test/markdown-format.test.mjs b/dor-tools-builtin/test/markdown-format.test.mjs new file mode 100644 index 000000000..a208fd317 --- /dev/null +++ b/dor-tools-builtin/test/markdown-format.test.mjs @@ -0,0 +1,69 @@ +import assert from 'node:assert/strict'; +import { createRequire } from 'node:module'; +import { test } from 'node:test'; +import { load } from './bundle.mjs'; + +// Exercise the exact parser/serializer versions used by the installed editor. +const require = createRequire(import.meta.url); +const editor = createRequire(require.resolve('@mdxeditor/editor')); +const dependency = name => import(editor.resolve(name)); +const { fromMarkdown } = await dependency('mdast-util-from-markdown'); +const { toMarkdown } = await dependency('mdast-util-to-markdown'); +const { mdxJsx } = await dependency('micromark-extension-mdx-jsx'); +const { mdxJsxFromMarkdown, mdxJsxToMarkdown } = await dependency('mdast-util-mdx-jsx'); +const { markdownFormat } = await load('../viewer/markdown-format.ts'); +const parse = text => fromMarkdown(text, { extensions: [mdxJsx()], mdastExtensions: [mdxJsxFromMarkdown()] }); +const serialize = (tree, original) => toMarkdown(tree, { extensions: [mdxJsxToMarkdown()], ...markdownFormat(original) }); +const roundtrip = text => serialize(parse(text), text); +const semantic = tree => JSON.parse(JSON.stringify(tree, (key, value) => key === 'position' ? undefined : value)); + +for (const source of [ + '## [Unreleased]\n\n## [1.3.0] - 2026-10-20\n', + 'Use <your relay> here.\n', + '## Tight\nbody\n\n## Loose\n\nbody\n', + '## First\n## Second\nbody\n', +]) { + test(`preserves issue 1254 formatting: ${JSON.stringify(source)}`, () => { + assert.equal(roundtrip(source), source); + assert.equal(roundtrip(roundtrip(source)), source); + }); +} + +test('a real edit keeps surrounding labels, entities, and heading spacing', () => { + const source = '## [Unreleased]\nold <your relay>\n\n## [1.3.0] - 2026-10-20\n\nprevious\n'; + const tree = parse(source); + tree.children[1].children[0].value = 'new '; + const saved = serialize(tree, source); + assert.equal(saved, source.replace('old', 'new')); + assert.deepEqual(semantic(parse(saved)), semantic(tree)); +}); + +for (const source of [ + '\\[literal](url)\n', + '\\[literal][ref]\n\n[ref]: /target\n', + '\\[literal]\n\n[literal]: /target\n', + '\\[literal]: /target\n', + '[link \\[label\\]](/target)\n', + '- \\[x] plain list text\n', + '\\[outer \\[inner] end]\n', + '\\\\[path]\n', + '![image](image.png)\n', + '` [literal]`\n', + '```md\n## Example\nbody [literal] \n```\n', + 'safe\n', + 'Use \\ and <https://example.com>.\n', + '\\\n', + 'A \\[label] **bold** and \\[label]\\(url).\n', +]) { + test(`retains semantics: ${JSON.stringify(source)}`, () => { + const before = parse(source); + const saved = serialize(before, source); + assert.deepEqual(semantic(parse(saved)), semantic(before)); + assert.equal(roundtrip(saved), saved); + }); +} + +test('new headings follow the majority spacing, ignoring fenced examples', () => { + const source = '## One\ntext\n\n```md\n## Example\n\ntext\n```\n'; + assert.equal(serialize(parse('## New\n\ntext\n'), source), '## New\ntext\n'); +}); diff --git a/dor-tools-builtin/viewer/markdown-format.ts b/dor-tools-builtin/viewer/markdown-format.ts new file mode 100644 index 000000000..6cfca93f1 --- /dev/null +++ b/dor-tools-builtin/viewer/markdown-format.ts @@ -0,0 +1,57 @@ +import type { MDXEditorProps } from '@mdxeditor/editor'; + +type Options = NonNullable; + +/** Keep the document's inexpensive style choices without retaining a second + * editable syntax tree. Ambiguous Markdown still uses the stock serializer. */ +export function markdownFormat(markdown: string): Options { + const prose = markdown.replace(/^(`{3,}|~{3,})[^]*?^\1[^\n]*$/gm, ''); + const count = (pattern: RegExp) => prose.match(pattern)?.length ?? 0; + const bullets = (['-', '*', '+'] as const).map(b => [b, count(new RegExp(`^[ \\t]*\\${b}[ \\t]+\\S`, 'gm'))] as const); + const bullet = bullets.reduce((best, next) => next[1] > best[1] ? next : best)[0]; + // Per-heading choices preserve mixed files too. Newly added/renamed headings + // use the majority; ties retain the serializer's blank line. + const headings = new Map(); + let tight = 0, loose = 0; + for (const match of prose.matchAll(/^ {0,3}(#{1,6})[ \t]+([^\n]+)\n(?=([^\n]*))/gm)) { + const gap = match[3].trim() ? 0 : 1; + headings.set(`${match[1].length}:${match[2].trim()}`, gap); + if (gap) loose++; else tight++; + } + let references = false; + return { + bullet, + rule: count(/^\*{3,}[ \t]*$/gm) > count(/^-{3,}[ \t]*$/gm) ? '*' : '-', + // A literal angle bracket must not turn into JSX when source mode imports + // the saved text again. Only text nodes use these entity escapes. + unsafe: [{ character: '<', inConstruct: 'phrasing' }, { character: '>', inConstruct: 'phrasing' }], + handlers: { + root(node, _parent, state, info) { + references = hasDefinition(node); + return state.containerFlow(node, info); + }, + text(node, _parent, state, info) { + const safe = state.safe(node.value, { ...info, encode: ['<', '>'] }) + .replace(/</gi, '<').replace(/>/gi, '>'); + // Restore only simple, complete labels. Keep escapes inside link labels, + // list items (task syntax), and documents with reference definitions. + if (references || state.stack.some(kind => ['label', 'reference', 'listItem'].includes(kind))) return safe; + return safe.replace(/\\(?:\\|\[([\w .-]+)\])/g, (escaped, label: string | undefined, offset: number) => { + if (!label) return escaped; // A literal backslash is not an escape to remove. + const after = safe.slice(offset + escaped.length) || info.after; + if (/^\s*[\[(:\]]/.test(after) || safe[offset - 1] === '!') return escaped; + return `[${label}]`; + }); + }, + }, + join: [(left) => { + if (left.type !== 'heading') return; + const text = left.children.map(child => 'value' in child ? child.value : '').join(''); + return headings.get(`${left.depth}:${text}`) ?? (tight > loose ? 0 : 1); + }], + }; +} + +function hasDefinition(node: { type: string; children?: { type: string }[] }): boolean { + return node.type === 'definition' || !!node.children?.some(hasDefinition); +} diff --git a/dor-tools-builtin/viewer/markdown.tsx b/dor-tools-builtin/viewer/markdown.tsx index c737bf9fd..7fbf31bb3 100644 --- a/dor-tools-builtin/viewer/markdown.tsx +++ b/dor-tools-builtin/viewer/markdown.tsx @@ -16,6 +16,7 @@ import { checked, documentSession, loadingState, request, stateLabel, textSource import { editableImage, handOffImageEdits, imageSources, imageUrl, ImagesPanel, localImagePath, pathRoute } from './images-panel'; import { ImageResizeHandles } from './image-resize'; import { commentsPlugin } from './markdown-comments'; +import { markdownFormat } from './markdown-format'; import { markdownSafetyPlugin } from './markdown-safety'; import { pageTheme, subscribeTheme } from './page-theme'; import { mermaidDescriptor } from './mermaid-block'; @@ -247,15 +248,6 @@ const plugins = [ toolbarPlugin({ toolbarContents: Toolbar }), ]; -/** The list bullet and thematic break the file uses most, so a save keeps its style. */ -function markdownStyle(markdown: string) { - const prose = markdown.replace(/^(```|~~~)[^]*?^\1/gm, ''); - const count = (pattern: RegExp) => prose.match(pattern)?.length ?? 0; - const bullets = (['-', '*', '+'] as const).map(b => [b, count(new RegExp(`^[ \\t]*\\${b}[ \\t]+\\S`, 'gm'))] as const); - const bullet = bullets.reduce((best, next) => next[1] > best[1] ? next : best)[0]; - return { bullet, rule: count(/^\*{3,}[ \t]*$/gm) > count(/^-{3,}[ \t]*$/gm) ? '*' as const : '-' as const }; -} - /** Whether this editor failed to parse its document. MDXEditor clears its * normalize flag only in a rich-text export, which a parse error suppresses, * so every later source edit would otherwise arrive as normalization. */ @@ -271,7 +263,7 @@ const onEditorError: MDXEditorProps['onError'] = ({ error: reason }) => { /** The rich editor; it follows only the theme, so page state changes leave it be. */ function Editor({ markdown }: { markdown: string }) { const theme = useSyncExternalStore(subscribeTheme, pageTheme); - const style = useMemo(() => markdownStyle(markdown), [markdown]); + const style = useMemo(() => markdownFormat(markdown), [markdown]); return ( Date: Tue, 20 Oct 2026 23:54:33 +0000 Subject: [PATCH 3/4] fix: keep literal < / > text when encoding angle brackets --- dor-tools-builtin/test/markdown-format.test.mjs | 1 + dor-tools-builtin/viewer/markdown-format.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/dor-tools-builtin/test/markdown-format.test.mjs b/dor-tools-builtin/test/markdown-format.test.mjs index a208fd317..106401ec8 100644 --- a/dor-tools-builtin/test/markdown-format.test.mjs +++ b/dor-tools-builtin/test/markdown-format.test.mjs @@ -53,6 +53,7 @@ for (const source of [ 'safe\n', 'Use \\ and <https://example.com>.\n', '\\\n', + 'Write \\< and \\> literally, but \\\\<.\n', 'A \\[label] **bold** and \\[label]\\(url).\n', ]) { test(`retains semantics: ${JSON.stringify(source)}`, () => { diff --git a/dor-tools-builtin/viewer/markdown-format.ts b/dor-tools-builtin/viewer/markdown-format.ts index 6cfca93f1..cfb8039bd 100644 --- a/dor-tools-builtin/viewer/markdown-format.ts +++ b/dor-tools-builtin/viewer/markdown-format.ts @@ -32,7 +32,7 @@ export function markdownFormat(markdown: string): Options { }, text(node, _parent, state, info) { const safe = state.safe(node.value, { ...info, encode: ['<', '>'] }) - .replace(/</gi, '<').replace(/>/gi, '>'); + .replace(/\\[^]|([CE]);/g, (match, hex: string | undefined) => hex ? (hex === 'C' ? '<' : '>') : match); // Restore only simple, complete labels. Keep escapes inside link labels, // list items (task syntax), and documents with reference definitions. if (references || state.stack.some(kind => ['label', 'reference', 'listItem'].includes(kind))) return safe; From e813f4847a6ab5876d0b85037fc2c21fe93c5e68 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Tue, 20 Oct 2026 16:56:30 -0700 Subject: [PATCH 4/4] refactor: clear MDXEditor's normalize flag on a parse error instead of shadowing it Publish initialMarkdownNormalize$ false from onError rather than keeping a module-level unparsed flag that apply() had to reset. Resolve the format test's MDXEditor dependencies from the viewer, as markdown-dependencies does, and drop an assertion the preceding one already implied. Co-Authored-By: Claude Opus 5.5 --- dor-tools-builtin/test/markdown-format.test.mjs | 5 ++--- dor-tools-builtin/viewer/markdown-format.ts | 2 +- dor-tools-builtin/viewer/markdown.tsx | 13 +++++-------- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/dor-tools-builtin/test/markdown-format.test.mjs b/dor-tools-builtin/test/markdown-format.test.mjs index a208fd317..25e860549 100644 --- a/dor-tools-builtin/test/markdown-format.test.mjs +++ b/dor-tools-builtin/test/markdown-format.test.mjs @@ -4,8 +4,8 @@ import { test } from 'node:test'; import { load } from './bundle.mjs'; // Exercise the exact parser/serializer versions used by the installed editor. -const require = createRequire(import.meta.url); -const editor = createRequire(require.resolve('@mdxeditor/editor')); +const viewer = createRequire(new URL('../viewer/markdown.tsx', import.meta.url)); +const editor = createRequire(viewer.resolve('@mdxeditor/editor')); const dependency = name => import(editor.resolve(name)); const { fromMarkdown } = await dependency('mdast-util-from-markdown'); const { toMarkdown } = await dependency('mdast-util-to-markdown'); @@ -25,7 +25,6 @@ for (const source of [ ]) { test(`preserves issue 1254 formatting: ${JSON.stringify(source)}`, () => { assert.equal(roundtrip(source), source); - assert.equal(roundtrip(roundtrip(source)), source); }); } diff --git a/dor-tools-builtin/viewer/markdown-format.ts b/dor-tools-builtin/viewer/markdown-format.ts index 6cfca93f1..b43bfec70 100644 --- a/dor-tools-builtin/viewer/markdown-format.ts +++ b/dor-tools-builtin/viewer/markdown-format.ts @@ -44,7 +44,7 @@ export function markdownFormat(markdown: string): Options { }); }, }, - join: [(left) => { + join: [left => { if (left.type !== 'heading') return; const text = left.children.map(child => 'value' in child ? child.value : '').join(''); return headings.get(`${left.depth}:${text}`) ?? (tight > loose ? 0 : 1); diff --git a/dor-tools-builtin/viewer/markdown.tsx b/dor-tools-builtin/viewer/markdown.tsx index 7fbf31bb3..23cc6a4f1 100644 --- a/dor-tools-builtin/viewer/markdown.tsx +++ b/dor-tools-builtin/viewer/markdown.tsx @@ -7,7 +7,7 @@ import { classHighlighter } from '@lezer/highlight'; import { $isImageNode, createActiveEditorSubscription$, editImageToolbarComponent$, iconComponentFor$, ImageNode, INSERT_IMAGE_COMMAND, useCellValue, BlockTypeSelect, BoldItalicUnderlineToggles, codeBlockPlugin, codeMirrorPlugin, CodeToggle, CreateLink, diffSourcePlugin, - DiffSourceToggleWrapper, frontmatterPlugin, headingsPlugin, imagePlugin, InsertCodeBlock, InsertTable, InsertThematicBreak, + DiffSourceToggleWrapper, frontmatterPlugin, headingsPlugin, imagePlugin, initialMarkdownNormalize$, InsertCodeBlock, InsertTable, InsertThematicBreak, linkDialogPlugin, linkDialogState$, openLinkEditDialog$, linkPlugin, listsPlugin, ListsToggle, markdownShortcutPlugin, MDXEditor, quotePlugin, realmPlugin, rootEditor$, Separator, viewMode$, tablePlugin, thematicBreakPlugin, toolbarPlugin, UndoRedo, type MDXEditorMethods, type MDXEditorProps, type Realm, } from '@mdxeditor/editor'; @@ -91,7 +91,6 @@ const session = documentSession({ async apply(text) { // A reload mounts a fresh editor: its own normalization is then not an edit, and undo starts over. uploadedWidths.clear(); - unparsed = false; flushSync(() => root.render()); await new Promise(resolve => setTimeout(resolve)); }, @@ -248,14 +247,12 @@ const plugins = [ toolbarPlugin({ toolbarContents: Toolbar }), ]; -/** Whether this editor failed to parse its document. MDXEditor clears its - * normalize flag only in a rich-text export, which a parse error suppresses, - * so every later source edit would otherwise arrive as normalization. */ -let unparsed = false; // The editor's own reformatting of loaded text is not an edit. -const onEditorChange: MDXEditorProps['onChange'] = (_, normalize) => { if (normalize && !unparsed) session.rebase(); else session.changed(); }; +const onEditorChange: MDXEditorProps['onChange'] = (_, normalize) => { if (normalize) session.rebase(); else session.changed(); }; const onEditorError: MDXEditorProps['onError'] = ({ error: reason }) => { - unparsed = true; + // MDXEditor clears its normalize flag only in a rich-text export, which a + // parse error suppresses, so every later source edit would arrive as normalization. + realm?.pub(initialMarkdownNormalize$, false); realm?.pub(viewMode$, 'source'); session.fail(`Showing source: the rich editor cannot parse this file (${reason}).`); };