Skip to content

test(scripts): normalize line endings on read instead of per assertion - #454

Merged
PathGao merged 3 commits into
sftwrdotdev:masterfrom
PathGao:test/read-source-normalizes-line-endings
Aug 4, 2026
Merged

test(scripts): normalize line endings on read instead of per assertion#454
PathGao merged 3 commits into
sftwrdotdev:masterfrom
PathGao:test/read-source-normalizes-line-endings

Conversation

@PathGao

@PathGao PathGao commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

846b84b2 in #452 fixed this suite on Windows by hand, one assertion at a time,
in the middle of cutting a release. That should not have been your job — the
tests are mine, and the reason fifteen of them broke is that fifty of them each
made the same decision separately. This moves the decision to one place and adds
the guard that keeps it there.

This is test-only. No src/, no src-tauri/, no product behaviour; the only
non-scripts/ file is a new .gitattributes. Merge it before or after v2.7.0 —
whichever suits the release you have in flight. There is no rush and nothing here
that a user can see.

Root cause

Two halves, both the same shape: a decision that belongs on the read, made at
every call site instead.

Line endings. The repo has no .gitattributes, so a Windows clone with the
default core.autocrlf=true checks the entire tree out CRLF. Every test that
reads src/ as text then matches it against a pattern containing a literal \n,
or slices it with an anchor containing one, and none of those match the bytes on
disk. sliceBetween reports it as expected to find "…" for an anchor that is
sitting right there in the file.

Path separators. walkSourceFiles() in sourceTree.ts has always done
.replace(/\\/g, '/'). i18nCoverage.test.ts and monacoStartupGraph.test.ts
each carried their own private copy of the walk, so neither inherited it — which
is precisely what sourceTree.ts's header comment says the module exists to
prevent ("Three copies of walk() used to live in …. One copy each, here.").

What this does

1. readSource(path) in sourceTree.ts.

export function readSource(path: string | URL): string {
	return readFileSync(path, 'utf8').replace(/\r\n/g, '\n');
}

string | URL because both spellings were already in use — a cwd-relative string
and new URL('../src/…', import.meta.url) — so no call site had to change shape
to get normalized. Documented at the length the neighbouring helpers are, naming
#452, so the next reader cannot mistake it for indirection for its own sake.

2. 60 files, 135 call sites routed through it — and the \r? workarounds
removed where they become redundant. That is the point: the assertion goes back
to reading as \n, which is the form the files have in the repository, and the
normalization happens once.

Deliberately left alone: the CRLF fixtures in frontMatter.test.ts,
frontMatterProseBlock.test.ts and pasteUrlContext.test.ts. Those assert on the
parser's handling of a CRLF document, and each spells the document as a literal
in the test — which is the right way round. A fixture whose bytes are the point
must not depend on what Git checked out.

3. Both private walkers folded into walkSourceFiles. Filters checked first:
i18nCoverage's was .svelte || .ts against walkSourceFiles's
.ts|.svelte|.js — the same set today, since src/ has no .js, and the wider
one is the one that should win (a .js file calling t() is a file whose keys
must resolve). monacoStartupGraph's was /\.(svelte|ts|js)$/, identical. Neither
needed walkSourceFiles widened with an argument.

One of your fifteen patches is not subsumed and stays: the
.replace(/\\/g, '/') on relative(process.cwd(), f) in monacoStartupGraph.
That normalizes the output of the import-graph walk, which resolves absolute paths
and never goes through walkSourceFiles — a real separator fix, not a symptom of
the duplication.

4. The guard, as two rows in singleImplementationConvention.test.ts's RULES
rather than a new mechanism — that file already is where a convention like this is
written down. The only thing it could not express was a rule about the suite
itself, because SOURCES was fixed to src; rules now take an optional dir and
the tree walk is memoized per directory. Everything existing is unchanged.

marker allowed in
readFileSync( scripts/sourceTree.ts
readdirSync( scripts/sourceTree.ts

Both markers are the call, not the import — import { readFileSync } on its own
reads nothing. Both messages name the replacement and how to call it. The existing
every rule keeps at least one live implementation test covers the new rows, so a
marker that stops matching sourceTree.ts is reported rather than silently
guarding nothing.

5. .gitattributes* text=auto eol=lf, with .nsi/.nsh/.ps1/.bat/.cmd
pinned to CRLF. Those are read by NSIS and Chocolatey on Windows and are pinned so
the bytes those toolchains see stay identical to what a core.autocrlf=true runner
hands them today; this is not the PR to also change the installer build. Every
tracked file in the repo is LF right now (verified — git add --renormalize .
stages nothing outside this branch's own edits), so no stored bytes change and no
Linux or macOS working tree changes. What changes is the next Windows clone.

Why both. .gitattributes is the belt and it is the weaker half on purpose: it
does not touch a tree that is already checked out — that needs
git add --renormalize . — and it cannot stop an editor from writing CRLF into a
file it saves. readSource is the braces, and it is the half that actually holds.

Falsification

The guard catches existing drift. Run before either cleanup, against master:

rule files flagged on first run
readFileSync( outside sourceTree.ts 59
readdirSync( outside sourceTree.ts 2 (i18nCoverage, monacoStartupGraph)

The fix fixes the Windows case. Measured on macOS by converting every tracked
text file to CRLF end to end — src/, src-tauri/, scripts/, the workflows,
snapcraft.yaml, Cargo.toml, the JSON — and running the suite, then restoring.

tree CRLF result
master as it stands 596/596 pass — your patch works
master with 846b84b2 reverse-applied 21 tests fail across 8 files
this branch 598/598 pass, with the workarounds deleted rather than kept
this branch, readSource reduced to a plain read 21 tests fail across 8 files — the same set

The last two rows are the pair that matters: one line of normalization does
character-for-character the work that 33 hand-edited characters across 15 files
were doing, and rolling back only that line reproduces the failure exactly.

The 8 files that genuinely reddened are foldStatePerDocument,
issue281MinimalMacosMenu, liveModeWatchedPath, macosPdfExport,
menuModalGuards, windowOrganization, windowStateRestore,
youtubeExternalFallback. Of the other seven you patched, two were the
path-separator half — which a macOS run cannot reproduce at all, since it is
backslashes and not \r — and five were precautionary: their patterns already
tolerated the extra \r because a neighbouring \s* absorbed it. Those five are
reverted here as no-ops, not as behaviour changes.

Verification

npm test              598/598 pass, 0 fail          (596 before; +2 are the new rules)
npm run check         638 files, 0 errors, 0 warnings
cargo test            150/150 pass

.github/workflows/test.yml runs npm ci, npm audit, npm run check, npm test
and cargo test; all of them are above except npm audit, which this branch cannot
affect — no dependency changed and package-lock.json is untouched.

Each of the three commits was also checked out and run on its own — 596, 596, 598,
zero failures — so the 62-file diff is reviewable commit by commit rather than only
at the tip.

Not touched, on purpose: src-tauri/ in any form, including
src-tauri/src/tab_transfer.rs.

PathGao and others added 3 commits August 5, 2026 07:26
Fifteen test files were red on a Windows checkout while v2.7.0 was being cut,
and were hand-patched assertion by assertion in #452. The cause is two decisions
made in the wrong place.

Git on Windows defaults to `core.autocrlf=true`, so the whole working tree is
checked out CRLF. Every test here that reads `src/` as text then matches it
against a pattern containing a literal `\n`, or slices it with an anchor
containing one — and none of those match the bytes on disk. `sliceBetween` turns
that into `expected to find "…"` for an anchor that is right there in the file.

The patch that landed fixed the assertions: `[^\r\n]` in one place,
`source.includes('\r\n') ? '\r\n\t…' : '\n\t…'` in another. Correct, and 33
characters spread over 15 files that nothing requires the next test to repeat.
The decision belongs on the read, not on each of the 135 places that consume it.

  export function readSource(path: string | URL): string {
      return readFileSync(path, 'utf8').replace(/\r\n/g, '\n');
  }

`string | URL` because both spellings were already in use — a cwd-relative
string and `new URL('../src/…', import.meta.url)` — so no call site had to
change shape to get normalized.

So this replaces the ad-hoc `readFileSync(…, 'utf8')` in 57 files and *removes*
the `\r?` workarounds that become redundant: the assertions go back to reading
as `\n`, which is the form the files have in the repository.

What is deliberately left reading raw bytes: the CRLF fixtures in
frontMatter.test.ts, frontMatterProseBlock.test.ts and pasteUrlContext.test.ts.
Those assert on the parser's handling of a CRLF *document* and each spells the
document as a literal in the test, which is the right way round — a fixture
whose bytes are the point must not depend on what Git checked out.

Measured, on a working tree converted to CRLF end to end: with `readSource`
reduced to a plain read, 21 tests across 8 files fail — character for character
the same set that fails on master with #452's patch reverse-applied. With the
normalization in place, 596/596 pass on that same CRLF tree, with the
workarounds deleted rather than kept.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…eFiles

sourceTree.ts exists because three copies of `walk()` used to live in
singleImplementationConvention, renderPipelineConvention and previewSanitize.
Two more had survived that consolidation, and both paid for it in #452.

i18nCoverage.test.ts built its paths with `join(dir, entry)`, which on Windows
spells them with backslashes, and then compared them against forward-slash
literals — so it needed the same `.replace(/\\/g, '/')` that `walkSourceFiles`
has had all along. monacoStartupGraph.test.ts's copy escaped only because it
happened to build paths with a template literal instead of `join`; nothing in
it made that a decision rather than an accident.

Both filters were already what `walkSourceFiles` matches. i18nCoverage's was
`.svelte` or `.ts` against `walkSourceFiles`'s `.ts|.svelte|.js`, which is the
same set today (`src/` contains no `.js`) and the wider one is the one that
should win: a `.js` file calling `t()` is a file whose keys must resolve.
monacoStartupGraph's was `/\.(svelte|ts|js)$/` — identical. Neither needed
`walkSourceFiles` widened with an argument.

monacoStartupGraph's local reader was itself named `readSource` and held its own
`readFileSync`, which is how the file came to own a private copy of the
line-ending decision. It is a *filter* on top of a read — only `<script>` blocks
can carry an import — so it is now `importableSource` and delegates.

Not folded: the `.replace(/\\/g, '/')` #452 added to `relative(process.cwd(), f)`
in the same file. That normalizes the output of the *import-graph* walk, which
resolves absolute paths and never goes through `walkSourceFiles`, so it is a
real separator fix rather than a symptom of the duplication. It stays.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…to LF

A cleanup without a guard comes back. The two commits before this one removed
every private read and every private walk; nothing yet stops the next test from
adding one, and the loss is silent until someone runs the suite on Windows.

singleImplementationConvention.test.ts already is the place a convention like
this is written down: a row in RULES naming a marker and the complete set of
files allowed to contain it. The only thing it could not express was a rule
about the suite itself, because SOURCES was fixed to `src`. Rules now carry an
optional `dir` and the tree walk is memoized per directory, so the two new rows
scan `scripts` and everything else is unchanged.

  readFileSync(  — allowed in scripts/sourceTree.ts
  readdirSync(   — allowed in scripts/sourceTree.ts

Both markers are the *call*, not the import: `import { readFileSync }` on its
own reads nothing, and a file may legitimately keep the import for another
member. Both failure messages name the replacement and how to call it rather
than only reporting that something is wrong.

First run, before either cleanup: 59 files flagged by the read rule and 2 by the
walk rule. `every rule keeps at least one live implementation` covers the new
rows too, so a marker that stops matching sourceTree.ts is reported instead of
silently guarding nothing.

.gitattributes is the belt to `readSource`'s braces, and it is the weaker half
of the pair on purpose: it does not touch a tree that is already checked out —
that needs `git add --renormalize .` — and it cannot stop an editor from writing
CRLF into a file it saves. Read-side normalization is the half that actually
holds; this just stops the tree arriving wrong in the first place.

`* text=auto eol=lf`, with `.nsi/.nsh/.ps1/.bat/.cmd` pinned to CRLF. Those are
consumed by NSIS and Chocolatey on Windows, and pinning them keeps the bytes
those toolchains see identical to what a `core.autocrlf=true` runner hands them
today — this is not the commit to also change the installer build. Every tracked
file in the repo is LF right now, verified, so `git add --renormalize .` stages
nothing outside this branch's own edits: no stored bytes change and no Linux or
macOS working tree changes. What changes is the next Windows clone.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao merged commit 32ac3be into sftwrdotdev:master Aug 4, 2026
4 checks passed
@PathGao
PathGao deleted the test/read-source-normalizes-line-endings branch August 4, 2026 23:48
PathGao added a commit that referenced this pull request Aug 5, 2026
…cannot delete them (#455)

Every diagram whose labels Mermaid put in a `<foreignObject>` rendered as empty
shapes — in the live preview and in the exported HTML alike, because both call
`renderRichContent` → `sanitizeDiagramSvg`.

## Mechanism

`sanitizeDiagramSvg` allowed the `foreignObject` *element*:

    DOMPurify.sanitize(svg, { ADD_TAGS: ['foreignObject'], ADD_ATTR: [...] })

The label is not the element. It is the HTML inside it — `<div
class="labelBkg">…<span class="nodeLabel">Alpha</span></div>` — and DOMPurify
deletes HTML children of an SVG element unless the parent is an HTML integration
point:

    // dompurify 3.4.12, dist/purify.es.mjs
    const HTML_INTEGRATION_POINTS = freeze(['annotation-xml']);
    _checkHtmlNamespace = function (tagName, parent, parentTagName) {
      if (parent.namespaceURI === SVG_NAMESPACE && !HTML_INTEGRATION_POINTS[parentTagName])
        return false;

`foreignobject` is not on that list, so no `ADD_TAGS` entry could save its
children. `DOMPurify.removed` grew one entry per label and what survived was
literally `<foreignObject width="37.98" height="24"></foreignObject>`.

## Scope, measured

Real mermaid 11.16.0 driven by real dompurify 3.4.12 in a browser, over every
diagram type 11.16.0 ships a renderer for — not only the five sampled in the
report:

  labels entirely deleted (10)  flowchart, flowchart-v2, classDiagram,
                                stateDiagram, stateDiagram-v2, erDiagram,
                                requirementDiagram, mindmap, block, kanban
  labels hidden (1)             journey — see the `<switch>` note below
  unaffected                    sequence, gantt, pie, quadrantChart, gitGraph,
                                C4Context, timeline, sankey, xychart,
                                architecture, info, ishikawa, wardley, treemap,
                                packet, radar, treeView — all SVG `<text>`

## The fix

`mermaid.initialize` now passes `htmlLabels: false`. Mermaid then emits SVG
`<text>`, which no sanitizer objects to; every label above survives. One
root-level key is enough — the per-diagram `flowchart.htmlLabels` /
`class.htmlLabels` / … settings are deprecated in 11.x and the root one takes
precedence over them — so the diagram-specific keys are deliberately not set.

Because nothing then depends on HTML inside the SVG, `foreignObject` also leaves
`sanitizeDiagramSvg`'s `ADD_TAGS`. Nothing needs it:

  - Mermaid still emits `foreignObject` unconditionally in three places — venn
    `text` nodes, eventmodeling boxes, architecture `iconText` — but their HTML
    children are deleted by the rule above whether or not the tag is allowed, so
    the allowance could only ever produce an empty box. Measured: venn's "Bravo"
    label is absent with the tag allowed and absent without it.
  - The `<switch>`-based renderers (journey by default, and anything configured
    `textPlacement: 'fo'`) pair the `foreignObject` with an SVG `<text>`
    fallback. A browser renders the first child it supports, so an
    emptied-but-present `foreignObject` *suppressed* a label that had come
    through the filter intact. Measured: the journey's task labels are 0×0 with
    the tag on the allowlist and 37×20 with it removed. Dropping it fixes a
    diagram `htmlLabels: false` alone cannot.

So the filter gets smaller, not larger.

## The rejected alternative

DOMPurify 3.4.12 accepts `HTML_INTEGRATION_POINTS` as a config option, so
allowing `foreignObject` to be an integration point is a candidate fix that
would have preserved rendering fidelity exactly. Two findings:

  - It only works as an object map. `HTML_INTEGRATION_POINTS: ['annotation-xml',
    'foreignobject']` is a silent no-op — the value is `clone()`d, and cloning an
    array yields index keys, which also drops `annotation-xml`. Passing
    `{ 'annotation-xml': true, foreignobject: true }` does restore every label.
  - It is the wrong trade. It re-permits HTML inside SVG, which DOMPurify keeps
    out on purpose because serialise-then-reparse is the mutation-XSS primitive —
    and `container.innerHTML = sanitizeDiagramSvg(svg)` is exactly that reparse.
    Mermaid source is document content, so the SVG is attacker-influenced; #384
    exists in this same cycle because a document's `<style>` reached the app's own
    DOM. Measured difference: with the override, `<svg><foreignObject><img
    src=x></foreignObject></svg>` survives sanitisation and materialises as a
    live `<img>` on the reparse, where today (and with this fix) it does not.

## Trade-offs of `htmlLabels: false`

  - Markup inside a node label renders literally: `A["<b>bold</b> text"]` comes
    out as the six characters `<b>` followed by `bold`. No sample, test or doc in
    this repo puts HTML in a Mermaid label (checked: `samples/` has one diagram,
    `graph TD` with plain labels).
  - KaTeX inside a label goes the same way — Mermaid's math path runs only under
    `useHtmlLabels` — so `A["$$x^2$$"]` renders as its source. It rendered as
    nothing at all before this change, so this is not a loss.
  - Wrapping is measured by the SVG text engine instead of the browser's layout,
    so long labels break at slightly different points.

## Not a v2.7.0 regression

The allowance predates this cycle: it was introduced in eb9a1c7 ("Fix Mermaid
diagram rendering with SVG foreignObject support", 2026-02-04) and #411 only
moved it into `richContent.ts`. v2.6.13 ships the byte-identical config, and the
dompurify it shipped with (3.3.1) has the same `addToSet({}, ['annotation-xml'])`
and the same SVG-namespace check. So the release does not have to hold for this.

## Tests

`scripts/mermaidDiagramLabels.test.ts` runs the real `renderRichContent` over a
real code block with Mermaid answered out of `scripts/mermaidDiagramCorpus.json`
— bytes real mermaid 11.16.0 emitted, captured once with the old config and once
with the new one, and keyed by the config the pipeline actually sends, so a
config nobody has measured is an error rather than a pass. It parses what the
pipeline produced and asserts there is no HTML in the SVG for the namespace rule
to reach and that every label is in an SVG `<text>`.

What it cannot execute is DOMPurify: without a DOM the library returns a bare
factory with no `sanitize` at all, so it is stood in for by the identity function
as `exportRichContent.test.ts` already does, and a third test asserts that state
so the middle one is not misread as "the filter kept the labels". Making that
half real needs a DOM faithful enough to reproduce HTML5 foreign-content parsing,
which is the rule under test — a shim written here would be marking its own
homework, and no test-only DOM dependency was added.

Falsified: with only `htmlLabels: false` reverted, the corpus stand-in returns
the old bytes and the middle test fails with `flowchart: the rendered diagram
still carries its labels in foreignObject, whose HTML children DOMPurify removes
regardless of ADD_TAGS`, 3 !== 0.

`previewSanitize.test.ts` asserted the source text `ADD_TAGS: ['foreignObject']`.
That assertion confirmed a config string existed while every label was being
stripped, so it is replaced rather than re-anchored: the split between the two
sanitizer configs is now pinned on the reason that survives — the diagram filter
must permit the `<style>` the document policy forbids.

The rationale comment in `scripts/sourceTree.ts` still says the diagram config
"needs `foreignObject`"; it is left for #454's rewrite of that tree rather than
conflicting with it.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant