fix(site-builder): make formatHtml close-tag mismatch recovery self-draining

Round 2's tag-name check on close popping fixed misattribution but could
wedge the stack permanently: a mismatch with a real, still-open ancestor
(e.g. an unclosed <p> before a later </div>, a normal optional-end-tag
slip) never drained, so everything after it inherited the stuck depth and
could print out of source order. Close handling now searches the whole
stack for a matching tag, not just the top; frames above a found match are
popped and implicitly closed (no fabricated close tag, just ending their
indentation) before the match itself closes normally. A close with no
match anywhere is still left in place untouched, since it has nothing to
pair with. Also fixes a related idempotency bug in text-run whitespace
collapsing surfaced while verifying this: boundary whitespace containing a
newline (formatter-introduced structural gap) is now dropped entirely
instead of being collapsed to a preserved space like same-line boundary
spaces are.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

craft/src/utils/format-html.test.ts

@@ -81,3 +81,31 @@ describe('formatHtml - raw content and quoted attributes', () => {
     );
   });
 });
+
+// Regression coverage from round 2 of code review: a close tag whose
+// innermost open frame doesn't match it must not wedge the stack for the
+// rest of the document. An optional end tag skipped by the author (e.g.
+// an unclosed <p>) must self-drain against its real ancestor close tag,
+// while a close tag with no opener anywhere on the stack still has nothing
+// to pair with and is left exactly where it is.
+describe('formatHtml - mismatched close recovery', () => {
+  test('an unclosed <p> before </div> drains against the ancestor close instead of wedging the stack', () => {
+    const src = '<div><p>one<p>two</p></div><p>three</p>';
+    expect(formatHtml(src)).toBe(
+      '<div>\n  <p>\n    one\n    <p>two</p>\n</div>\n<p>three</p>',
+    );
+  });
+
+  test('a close tag with no opener anywhere on the stack is left in place', () => {
+    const src = '<div><p>x</p></footer></div>';
+    expect(formatHtml(src)).toBe('<div>\n  <p>x</p>\n  </footer>\n</div>');
+  });
+
+  test('is idempotent across an unclosed <p> and a stray close with no opener', () => {
+    const unclosedP = formatHtml('<div><p>one<p>two</p></div><p>three</p>');
+    expect(formatHtml(unclosedP)).toBe(unclosedP);
+
+    const strayClose = formatHtml('<div><p>x</p></footer></div>');
+    expect(formatHtml(strayClose)).toBe(strayClose);
+  });
+});

craft/src/utils/format-html.ts

@@ -50,10 +50,34 @@ interface Token {
   kind: 'open' | 'close' | 'void' | 'text' | 'verbatim';
 }
 
-/** Collapse a text run's whitespace to single spaces; drop it if it is all whitespace. */
+/**
+ * Turn a raw run of text (between two tags) into a text token, collapsing
+ * interior whitespace to single spaces. A run that is nothing but
+ * whitespace is dropped entirely -- it's structural gap between tags, not
+ * content.
+ *
+ * For a run with real content, its leading/trailing whitespace is boiled
+ * down to at most one space each, and -- critically for idempotency --
+ * that boundary space is kept only if it does NOT contain a newline. A
+ * same-line boundary space (`"hello "` before `<strong>`) is a hand-typed
+ * word separator and must survive. A boundary run that spans a newline
+ * (`"\n    one\n    "`, produced when this formatter puts an element's text
+ * on its own indented line) is formatting whitespace this function itself
+ * introduced; re-parsing formatted output must drop it completely; keeping
+ * even one space there would let re-formatting keep inserting a phantom
+ * space around content that had none in the original source.
+ */
 function pushTextToken(tokens: Token[], raw: string): void {
-  const collapsed = raw.replace(/\s+/g, ' ');
+  const core = raw.trim();
-  if (collapsed.trim()) tokens.push({ text: collapsed, tag: '', kind: 'text' });
+  if (!core) return;
+
+  const leading = /^\s*/.exec(raw)![0];
+  const trailing = /\s*$/.exec(raw)![0];
+  const leadSpace = leading && !leading.includes('\n') ? ' ' : '';
+  const trailSpace = trailing && !trailing.includes('\n') ? ' ' : '';
+  const text = leadSpace + core.replace(/\s+/g, ' ') + trailSpace;
+
+  tokens.push({ text, tag: '', kind: 'text' });
 }
 
 /**
@@ -152,6 +176,24 @@ interface Frame {
   committed: boolean;
 }
 
+/**
+ * Emit whatever a still-open frame needs, without ever fabricating a
+ * closing tag for it: its own open-tag line if it was never committed, and
+ * any inline content it was holding. Used both for elements that are
+ * genuinely never closed anywhere in the document, and for elements
+ * implicitly closed by an ancestor's close tag (e.g. an optional end tag
+ * like `<p>` that the author skipped) -- either way, no synthetic close tag
+ * is written; only indentation for content that really was opened.
+ */
+function implicitlyClose(lines: string[], frame: Frame): void {
+  if (!frame.committed) {
+    lines.push(INDENT.repeat(frame.depth) + frame.text);
+  }
+  if (frame.inline) {
+    lines.push(INDENT.repeat(frame.depth + 1) + frame.inline);
+  }
+}
+
 export function formatHtml(src: string): string {
   if (!src || !src.trim()) return '';
 
@@ -194,21 +236,37 @@ export function formatHtml(src: string): string {
     }
 
     if (token.kind === 'close') {
-      const top = stack[stack.length - 1];
+      // Find the nearest still-open frame with this tag name, anywhere on
+      // the stack -- not just the top. HTML permits skipping optional end
+      // tags (e.g. `<p>`), so the element a close tag pairs with is not
+      // always the innermost open element.
+      let matchIndex = -1;
+      for (let k = stack.length - 1; k >= 0; k -= 1) {
+        if (stack[k].tag === token.tag) {
+          matchIndex = k;
+          break;
+        }
+      }
 
-      if (!top || top.tag !== token.tag) {
+      if (matchIndex === -1) {
-        // Stray close: either nothing is open, or the innermost open frame
+        // Genuinely stray: no frame anywhere was opened with this tag
-        // is a *different* element. Don't fabricate a pairing by popping
+        // name, so there is nothing to pair it with. Don't fabricate a
-        // it anyway -- that would misattribute a real element's close tag
+        // pairing -- emit it in place and leave the stack untouched.
-        // to the wrong element and leave the stack silently desynced for
-        // everything after it. Emit this close in place instead, and leave
-        // whatever is genuinely open on the stack for later resolution.
         if (stack.length === 0) flushRootInline();
         lines.push(INDENT.repeat(stack.length) + token.text);
         continue;
       }
 
-      stack.pop();
+      // Everything above the match was opened but never explicitly closed
+      // in the source (e.g. a skipped `</p>`). Draining them here -- with
+      // no synthetic close tag -- keeps the stack from staying wedged for
+      // the rest of the document, restoring the self-draining property
+      // without fabricating markup.
+      while (stack.length - 1 > matchIndex) {
+        implicitlyClose(lines, stack.pop()!);
+      }
+
+      const top = stack.pop()!;
       if (!top.committed) {
         // Nothing block-level ever interrupted this element: merge the
         // open tag, its inline content, and the close tag onto one line.
@@ -241,13 +299,7 @@ export function formatHtml(src: string): string {
   // Unbalanced opens: nothing ever closed them. Flush what's left rather
   // than silently dropping content.
   while (stack.length) {
-    const top = stack.pop()!;
+    implicitlyClose(lines, stack.pop()!);
-    if (!top.committed) {
-      lines.push(INDENT.repeat(top.depth) + top.text);
-    }
-    if (top.inline) {
-      lines.push(INDENT.repeat(top.depth + 1) + top.inline);
-    }
   }
 
   flushRootInline();