fix(formatter): restructure Tidy to match prettyhtml.com's real pipeline

[mriechers]

Why

HTML copied out of a Google Doc came through Tidy mangled. Root cause: Docs autocorrects straight quotes to curly ones even when the text is HTML source, so class="hero lede" arrives as class=“hero lede”. parseAttributes read the value as unquoted, stopped at the space, and invented a bare attribute lede” that no cleaner matched — so it survived into the output.

Measured against the live site: prettyhtml.com returns a clean <p>Hi</p> for the same input. Their TinyMCE mangles it too, but their classes/IDs option then deletes the wreckage. We had no such backstop.

Fixing that meant auditing the whole pipeline. Reading String(convertText) in the running page gave the entire dispatch verbatim, and it differs from ours in order and in several option boundaries.

Pipeline

Tidy now runs through an exported runTidyPipeline(html, opts), ordered to match convertText():

• adds the unconditional whitespace pre-pass, looped post-pass and final cleanup that were missing entirely (divergence A)
• to-plain-text runs first, as theirs does
• option 5 is nbsp-only — despite the "Successive spaces" label it never touched literal whitespace on their site; that collapse belongs to the unconditional passes (C)
• option 3 reclaims the "> <" join, "> \n" join and newline-only-tag removal that had been sitting behind option 4 (F)
• toPlainText substitutes a space per tag instead of deleting it, so a<br/>b → a b, not ab (D)
• replaceUntilStable mirrors their helyettesit replace-to-idempotence, and refuses to loop on a replacement that re-creates its own pattern

Layer-1 compensation

Their engine is two layers: a TinyMCE DOM round-trip, then the string cleaners behind the ten checkboxes. We have no round-trip, so what it did for them now rides default-ON Extras:

Extra	Stands in for
opt-block-newlines	TinyMCE returns block elements one per line — the largest visible gap; our output was one long line
opt-nested-empties	their empty-tag machine doesn't cascade either; TinyMCE covers the case upstream
opt-docs-residue	role="presentation", aria-level, the docs-internal-guid <b> wrapper
opt-strip-scripts	unconditional on their site (with a popup); here it is visible and disableable

A trailing <br> just inside a block's closing tag is now dropped too — filler, not a line break.

Curly quotes

Accepted as attribute delimiters at the tokenizer level, unconditionally — robustness, not an option. buildTidyTag re-emits straight quotes, so every path (Tidy/Indent/Compress) is fixed. A > inside a curly-quoted value no longer truncates the tag.

Prose typography is left alone; the new opt-straighten-quotes Extra (default OFF) converts it to ASCII for anyone who wants that. Kept out of option 9 so the ten stay byte-faithful.

Closes #22

dir="ltr" survives on prettyhtml.com — stripping it is deliberately better than parity. The rest is layer-1 compensation.

Before: <p dir="ltr" role="presentation"><span style="…">Hello</span></p>
After: <p>Hello</p>

Verification

• 131 tests pass (was 75)
• formatter/tests/parity.test.mjs replays 21 black-box fixtures captured from the live site. The three where we deliberately differ carry an ours field recording what and why — so a future reader can tell a divergence from a regression.
• Side-by-side spot check on a realistic Docs paste: identical output apart from the three intended differences (dir stripped, literal characters instead of ’-style entities, empty <span> unwrapped).
• npx eslint . drops from 1322 errors to 37, all pre-existing (32 in shared/shell.js, which has no browser-globals config — separate issue, untouched here).

Deliberate divergences, documented in app.mjs

E options 1/2 parse attributes structurally rather than doing double-quote-only string surgery · G empty-tag removal exempts td/th/script/media and requires matching tag names (theirs deletes <b></i>) · H one-space-tag removal accepts   · N curly-quote delimiters

Notes

• Raw third-party capture stays gitignored in planning/captures/; eslint now ignores it too. Their kill-switch elkur() is not ported.
• Removes the dead opt-newline-before-close checkbox (rendered and persisted, never consumed).
• Known parity cost: the whitespace passes are string-level, so like theirs they don't preserve <pre> indentation.

🤖 Generated with Claude Code

[mriechers-pr-reviewer]

Reviewed PR #23 by diffing the PR's synthetic merge commit (4bd11b1, parents 9c3bf6f base / 437ebf67 head) against the base, since the shallow clone had no other way to materialize the diff — network fetch for a fuller history was blocked in this sandbox, but this comparison is equivalent to the PR diff for a clean, conflict-free merge. Note the working tree's CLAUDE.md (and this review's loaded instructions) reflect the base version, not the PR's proposed rewrite — the harness appears to intentionally avoid letting PR-authored CLAUDE.md content act as live instructions, which is the right call; I reviewed that diff as ordinary content, not as instructions.

This PR is a large, well-executed rework of formatter/app.mjs's Tidy pipeline (curly-quote attribute parsing, Google Docs paste cleanup, block-newline separation, a fixpoint loop for nested-empty removal, and a runTidyPipeline orchestrator that now matches prettyhtml.com's documented convertText() order). I traced the trickiest new logic by hand — the <b id="docs-internal-guid-…"> unwrap stack (formatter/app.mjs:584-595,632-634), the new stray-<br>-before-close-tag drop (formatter/app.mjs:1016-1026), and the curly-quote findTagEnd/parseAttributes changes — against their new tests and all traces check out correctly, including edge cases like a real nested <b> surviving inside a Docs wrapper. runTidyPipeline's stage order matches what the updated CLAUDE.md claims. replaceUntilStable is properly guarded against runaway loops. No stale references to the removed opt-newline-before-close option remain, and all new checkbox IDs are consistently wired between index.html and app.mjs. The new golden-fixture/parity tests (formatter/tests/parity.test.mjs, curly-quotes.test.mjs, docs-residue.test.mjs) are unusually rigorous, citing live-verified prettyhtml.com behavior for each divergence.

Only two non-blocking nits found:

1. formatter/tests/fixtures/prettyhtml-golden.json:100 — the plaintext-substitutes-space fixture's note says "Ours yields 'ab' — plain-text.test.mjs currently locks in the wrong behavior," but this same PR fixes toPlainText and updates that test to expect 'a b'. The note is now stale and will mislead anyone using this fixture file as a spec.
2. formatter/app.mjs:463 vs :465 — role === 'presentation' is compared case-sensitively while the adjacent dir check lowercases first (.toLowerCase() === 'ltr'). role="Presentation" would silently survive the Docs-residue strip where a differently-cased dir wouldn't. Minor inconsistency, unlikely to matter in practice since Docs emits lowercase, but worth aligning.

I considered flagging the docs-residue attribute strips (role="presentation"/aria-level/dir="ltr", default-ON) as overly broad since they apply to any matching element, not just descendants of a detected Docs wrapper — but the PR's own live-captured fixtures show this matches prettyhtml.com's actual (TinyMCE-driven) behavior, and the project's explicit goal is parity with that site's labels/defaults, so this reads as a deliberate, well-verified design choice rather than a bug.

[mriechers]

Round 1 — both nits were right, both fixed in 8cc2a3a. Nothing pushed back on.

1. role case-sensitivity (formatter/app.mjs) — correct catch, and it was a real inconsistency rather than a style point: dir was lowercased before comparison and role wasn't, sitting two lines apart. Both now compare against a single lowercased value:

const lowerValue = (value || '').toLowerCase();
if (lowerAttrName === 'role' && lowerValue === 'presentation') return null;
if (lowerAttrName === 'aria-level') return null;
if (lowerAttrName === 'dir' && lowerValue === 'ltr') return null;

Added a test asserting both role="Presentation" and dir="LTR" are stripped.

2. Stale fixture note (prettyhtml-golden.json) — right, and worth more than a nit given the file is meant to read as a spec. The note was accurate when written and falsified by the same PR that carried it. Now records that the 'ab' behavior was fixed here, rather than claiming the test still locks it in.

On the concern you raised and then set aside — the residue strips applying to any element rather than only inside a detected Docs wrapper — your read is right, and thank you for actually checking the fixtures before flagging it. Worth stating the reasoning explicitly for the record: opt-docs-residue is scoped to match prettyhtml.com's end-to-end output, where the equivalent work is done by TinyMCE across the whole document, not by a wrapper-scoped rule. Scoping ours to the wrapper would diverge from the site we're replacing, and the option is a visible default-ON checkbox rather than silent behavior. The one piece that is wrapper-scoped is the <b> unwrap, which has to be.

129 tests pass; npx eslint formatter/ reports only the two pre-existing missing-plugin errors.

@claude please review

[claude]

Claude finished @mriechers's task in 2m 31s —— View job

---

Review complete

• Gather context (PR body, prior review, trigger comment)
• Verify the two round-1 fixes in 8cc2a3a
• Review full diff origin/live...HEAD for new issues
• Run test suite / eslint — blocked, see note below
• Post final review

Diffed 9c3bf6f..HEAD (the shallow origin/live ancestor visible in this checkout, same base the prior review used) — 17 files, matches the PR's stated file list.

Round-1 fixes, both correct:

1. formatter/app.mjs:461-466 — role/dir now compare against the same lowercased value. New test in docs-residue.test.mjs asserts role="Presentation" and dir="LTR" both strip.
2. prettyhtml-golden.json:100 — note rewritten to record that the 'ab'→'a b' behavior was fixed in this PR, no longer contradicts the fixture it sits next to.

Rest of the diff (runTidyPipeline, curly-quote tokenizing, Docs-residue <b> unwrap stack, block-newline separation, nested-empty fixpoint): read through it in full against the new tests. A few things I checked specifically:

• parseAttributes/findTagEnd curly-quote handling accepts either orientation as opener or closer (QUOTE_CLOSERS), matches the "autocorrect sometimes guesses the boundary wrong" cases in curly-quotes.test.mjs, including the two-openers case and the unterminated-quote case not swallowing the rest of the document.
• replaceUntilStable's single-pass guard (to.includes(from)) correctly avoids the unbounded-growth case ( → ) while every other call site in the pipeline (' '→' ', '> <'→'><', etc.) doesn't hit that guard, so idempotent collapsing still loops as intended.
• boldStack in tidy() correctly uses a stack rather than a depth flag, so a real nested <b> inside a Docs wrapper keeps its own closing tag — matches the new test for that case.
• Pipeline stage order in runTidyPipeline (stray-breaks → whitespace pre-pass → script/style strip → plain-text → nbsp collapse → tag-gap joins → tidy() → nested-empty fixpoint → block-newline separation → post-pass loop → tag-attrs/watermarks/quote-straightening/smart-nbsps → final cleanup) matches the CLAUDE.md description and the PR body's stated convertText() order.
• No leftover references to the removed opt-newline-before-close id anywhere (index.html, TIDY_CHECKBOX_IDS, getTidyOptions).

Nothing further to flag — this looks solid, and the fixture/parity-test coverage (parity.test.mjs, curly-quotes.test.mjs, docs-residue.test.mjs) is genuinely doing the work of pinning each documented divergence to a live-captured behavior rather than an assumption.

Note on verification: I wasn't able to execute npm test or npx eslint in this sandbox — node/npm invocations require an approval this non-interactive review session can't grant (plain git commands work fine). Everything above is a manual read of the diff and the tests it ships with, not a confirmed green run. If you want me to actually execute the suite in a future review, node/npm would need to be added to the allowed tools for this workflow.

[mriechers]

Round 2 — supervisor review off-thread. Head is now 613c3e7.

PR body test count — fixed, 128 → 131.

Mixed quote delimiters (class=“hero lede") — confirmed the report: both mixed orientations fall to the bare-value path and reproduce the original bug shape. Leaving the behavior, but I want the reasoning on the record rather than "no real sample shows otherwise," because I measured what the fix would cost:

Input	Today	If a straight quote could close a curly-opened value
alt=“He said "hi" to me”	parses correctly	truncates at the inner "
alt="He said “hi” to me"	parses correctly	(converse rule) truncates at the inner “

Quoted prose inside an alt/title is both common and exactly what a word processor produces — more likely than mixed delimiters, which autocorrect doesn't generate since it converts both quotes of a pair. So the tolerant rule would trade a case that happens for one that hasn't been observed.

Documented in the QUOTE_CLOSERS comment and locked in with two tests, so a future "fix" trips instead of silently regressing them. 131 tests pass.

@claude please review

[claude]

Claude finished @mriechers's task in 2m 14s —— View job

---

Review complete

• Gather context (PR body, round-1 review, round-2 trigger comment)
• Verify head commit 613c3e7 and diff since round-1 review
• Verify PR body test count fix (128 → 131)
• Verify mixed-quote-delimiter decision: QUOTE_CLOSERS comment + new tests
• Run test suite / eslint — blocked, see note below
• Post final review

Diffed 8cc2a3a..613c3e7 — this round only touches formatter/app.mjs (+11) and formatter/tests/curly-quotes.test.mjs (+13), i.e. the new QUOTE_CLOSERS doc-comment (formatter/app.mjs:85-94) plus the two locking tests.

Mixed-delimiter reasoning checks out. I traced parseAttributes's regex (formatter/app.mjs:115) by hand for both new cases:

• alt="He said "hi" to me" — the "([^"]*)" alternative is tried first and is greedy up to the next literal "; since the interior quotes are curly (“/”), it correctly captures the whole straight-quoted span, quote: '"'.
• alt=“He said "hi" to me” — the straight-quote alternative can't match (first char isn't "), so the curly alternative [“”]([^“”]*)[“”] runs; its exclusion class only excludes curly chars, so the straight quotes inside pass through untouched and it correctly captures to the closing ”.

Both match the two new tests exactly, and the direction matches the PR's table (each case parses correctly today; only the opposite, unimplemented tolerant rule would truncate). QUOTE_CLOSERS genuinely has no straight↔curly cross entries (formatter/app.mjs:96-103), so the "not recognized, falls to bare-value path" claim for true mixed-opener input (class=“hero lede") is accurate — confirmed by re-reading the map rather than just the comment.

One thing worth noting positively: findTagEnd (formatter/app.mjs:263-285) consumes the same QUOTE_CLOSERS map for skipping quoted spans while locating >, so tag-boundary scanning and attribute-value parsing agree on what counts as a closer. That symmetry is why the mixed-delimiter gap stays a parsing limitation (bare-value fallback) rather than also corrupting tag-boundary detection.

Test count: PR body's "131 tests pass" is consistent — round 1 ended at 129 (per the round-1 comment), and this round adds exactly 2 new tests in curly-quotes.test.mjs. Couldn't independently execute npm test to confirm the exact runner-reported number (see note below), but the arithmetic and the diff agree.

No issues found in this round — it's a narrow, well-justified documentation-plus-regression-test change with no behavior modification, and the trade-off table in the PR description matches what the code actually does.

Note on verification: node/npm invocations still require an approval this non-interactive review session can't grant (same limitation as round 1), so I wasn't able to run npm test or npx eslint to get a confirmed green run — everything above is a manual trace of the diff and regex semantics. If you want the suite actually executed in review, node/npm would need to be added to this workflow's allowed tools.

[mriechers-pr-reviewer]

Reviewed the diff between base 9c3bf6f and PR head 570db02 (pull/23/merge) directly via local git history, since gh/network access was not authorized in this sandbox. This is a large, well-documented rewrite of the formatter Tidy pipeline to match prettyhtml.com's real convertText() behavior (pre/post whitespace passes, block-newline separation, Google Docs residue stripping, curly-quote attribute parsing, nested-empty fixpoint looping), backed by a substantial new golden-fixture parity test suite (parity.test.mjs, curly-quotes.test.mjs, docs-residue.test.mjs) plus updates to plain-text.test.mjs and stray-breaks.test.mjs. The reasoning comments throughout formatter/app.mjs are unusually thorough and each documented "divergence" (C, D, F, G, H, K, N) has a corresponding test.

One real but narrow gap found (see finding below): the new docsResidue attribute stripping (role="presentation"/aria-level/dir="ltr") was added to buildTidyTag but not threaded into the unwrapSpans "remaining attributes" check, so a <span> whose only attribute is one of those residue attributes won't be unwrapped even though the attribute itself does get stripped elsewhere. Impact is low since Google Docs residue attributes are demonstrated (per the new test fixture) to land on block elements (p, h2), not spans, and the visible output difference is just an extra empty <span> wrapper, not corruption.

Non-blocking observation (out of scope — pre-existing code untouched by this PR): unwrappedSpanDepth in tidy() (formatter/app.mjs) is a plain counter rather than a stack, so a kept (attributed) <span> nested inside an unwrapped (attribute-less) <span> can have its own closing tag dropped in favor of the outer one when there's trailing content between the two closes — producing structurally different output (content ends up before the inner tag's close instead of after). Notably, the PR's author was clearly aware of this exact failure class, since the new <b>-wrapper unwrap logic (boldStack) is deliberately implemented as a stack "so a real nested <b> keeps its own closing tag instead of consuming the wrapper's" — the same fix was just never applied to the pre-existing span counter. Worth a follow-up since it's easy to fix by the same pattern, but it's not part of this PR's diff and not a blocker here.

No High/Medium correctness issues found in the changed lines. Checkbox wiring for all new Extras options (opt-strip-scripts, opt-block-newlines, opt-nested-empties, opt-docs-residue, opt-straighten-quotes) is consistent across index.html, getTidyOptions(), and TIDY_CHECKBOX_IDS.

[mriechers]

Round 3 — finding fixed in bcb7e54. 136 tests pass.

unwrap-spans-ignores-docs-residue — confirmed and fixed. Your diagnosis was exactly right, including the cause: I added the residue rules to buildTidyTag and not to the unwrapSpans check, which was a second copy of the same logic. Rather than patch the copy, I extracted isDroppedAttr and pointed both at it, so they can't drift again.

One correction to the impact assessment, in your favor and worth recording: at shipped defaults the bug is invisible, not "an extra empty <span>". The opt-nested-empties fixpoint catches the emptied span on its second pass and cleans it up:

	nestedEmpties: true (default)	nestedEmpties: false
<p><span role="presentation">x</span></p>	<p>x</p>	<p><span>x</span></p> ← the bug

So it only surfaces with that Extra turned off. That makes it less severe than Low in practice, but also sneakier — a fixpoint loop quietly compensating for an inconsistency elsewhere is exactly the kind of thing that stops compensating later. The five new regression tests therefore assert with nestedEmpties: false rather than at defaults, so the guard doesn't depend on the loop.

On the out-of-scope unwrappedSpanDepth observation — you're right, and right that I knew the failure class, since boldStack exists two functions away for precisely that reason. I'm deliberately not fixing it in this PR: it's pre-existing, outside the diff, and changing unwrapSpans' structural behavior deserves its own change with its own fixtures rather than riding along in a large one. Flagging it to the repo owner as a follow-up rather than silently dropping it.

Also for the record: this PR's earlier rounds went un-reviewed because claude-code-review.yml triggers on [opened, ready_for_review, reopened, labeled] — no synchronize — so pushes don't re-trigger it. Adding the claude-review label is what produced this review. Worth knowing for anyone driving a PR here.

[mriechers-pr-reviewer]

Review of PR #23 could not be completed before this response was required. The code-review skill forked into a multi-agent pass: one manual spot-check by the orchestrator (Google-Docs <b>-unwrap stack logic, tidy checkbox ID round-trip, saveTidyOptions/loadTidyOptions stale-key handling, replaceUntilStable convergence bound, nestedEmpties loop cost) found nothing blocking, and one of four background finder agents ("cross-file tracer") finished, also finding no breaking cross-file mismatch — only a non-blocking test-coverage gap noted (no test exercises tidy() directly with whitespace-bearing text to lock in its new passthrough behavior after whitespace-collapsing logic moved into runTidyPipeline).

However, three other required review angles — line-by-line diff scan, removed-behavior audit, and reuse/simplification/efficiency/altitude/conventions — were still running in background agents and had not reported results when this structured output was required. I was not able to run npm test/node --test in this sandbox either, so no dynamic verification of formatter/*.test.mjs occurred.

Because full coverage of the diff was not established, I cannot respons­ibly render approve, request_changes, or comment. No blockers have been found so far, but the review is incomplete — please re-run to allow the remaining agents to finish before treating this as a final verdict.


pr:23:test-coverage-tidy-passthrough
Low
formatter/tests/parity.test.mjs

No test directly exercises tidy() in isolation with whitespace-bearing text to confirm the new passthrough behavior (whitespace-collapsing was moved out of tidy()'s TEXT case into runTidyPipeline's pre/post passes); existing direct tidy() calls in parity.test.mjs use whitespace-free fixtures only, leaving this behavior change unverified by tests.

[mriechers]

Round 4 — the review timed out rather than reaching a verdict (three of its angles were still running), so re-running as it asked. Head is now 4af0a04.

It did land one finding before running out of time, and it was a real gap:

test-coverage-tidy-passthrough — fixed. Right that nothing exercised tidy() directly on whitespace-bearing text. That behavior change is deliberate (whitespace collapsing moved out of tidy()'s TEXT case into the unconditional passes, divergence C), but every existing direct tidy() assertion uses whitespace-free input, so a regression would have slipped through silently. Added five: space runs, newlines, inter-tag gaps, option 5 on, plus the pipeline contrast showing where the collapsing actually happens now. 141 tests pass.

Two notes for the next pass, since the sandbox couldn't run them:

• npm test → 141/141 green locally on 4af0a04.
• npx eslint formatter/ → 2 errors, both pre-existing Definition for rule 'no-unsanitized/property' was not found (a plugin that isn't installed; the inline disables predate this PR).

Re-triggering by cycling the claude-review label — note that pushes alone don't re-trigger this workflow, since claude-code-review.yml listens on [opened, ready_for_review, reopened, labeled] with no synchronize.

[mriechers-pr-reviewer]

Reviewed the actual PR #23 diff (git diff 9c3bf6f e72f915), which is a 497-line rewrite of formatter/app.mjs toward prettyhtml.com parity (curly-quote attribute parsing, Google-Docs residue stripping, plain-text mode, stray-break normalization, a new runTidyPipeline), plus ~475 lines of new/updated tests. The work is well-documented and heavily tested, but one confirmed regression and a couple of smaller inconsistencies remain.

Must-fix:

• formatter/app.mjs:1167 (collapseNbspRuns) — for input like <p> &nbsp; </p> (a common paste "spacer paragraph": space +   + space), the old code fully deleted the paragraph (it stripped all   occurrences from the text token then trimmed to empty). The new pipeline only eats the entity on one side per a+' '/' '+a pass, leaving two literal spaces (<p> </p>), which don't match tidy()'s Case 1 (needs adjacent close tag), Case 2 (needs literal \n), or Case 3 (needs exact  / ), and don't match normalizeTagGaps's "> <" pattern (needs exactly one space) either. The pair survives tidy as <p> </p> after the later " "->" " collapse in postPassLoop, instead of being removed — verified by manual step-by-step trace through normalizeWhitespacePrepass → collapseNbspRuns → normalizeTagGaps → tidy() → postPassLoop → finalCleanup. No existing test (golden fixtures or parity.test.mjs) covers this padded shape; all nbsp-tag tests use the bare <p>&nbsp;</p> form. This is a real behavior regression in a feature this PR is specifically about (option 5 / one-space-tag cleanup).

Worth addressing, non-blocking:

• formatter/app.mjs:617 — the Google-Docs <b id="docs-internal-guid-…"> wrapper check (idAttr.value.startsWith('docs-internal-guid')) is case-sensitive, while this same PR explicitly made the sibling role=/dir= residue checks case-insensitive (see the dedicated test at formatter/tests/docs-residue.test.mjs "residue attribute values are matched case-insensitively"). No equivalent test/fix exists for the id prefix. Low real-world risk since Google Docs always emits lowercase ids, but it's an inconsistency in the PR's own stated design goal.
• formatter/app.mjs:1231 (separateBlockElements) — the docstring states a close tag "starts a line only when the block has block children," but the implementation unconditionally appends a trailing \n after every block close tag regardless of children. A shape like <div><p>a</p>tail text</div> (nested block followed by trailing inline text) ends up as ...tail text</div> with no separating newline before the close, an untested case with no golden fixture.
• formatter/app.mjs:1236 — atLineStart()'s backward scan skips empty-string parts entries, but nothing in separateBlockElements ever pushes an empty string, so the skip loop is dead code (cosmetic only).

No other correctness issues found in the diff; the curly-quote attribute parsing, boldStack-based Docs <b> unwrapping, and isDroppedAttr de-duplication (fixing the span/attribute-stripping divergence bug) all check out against their tests.