From 4e2a8f59a3c6e624108d2fb14a54bb7ca61fb287 Mon Sep 17 00:00:00 2001 From: collin-thomas Date: Thu, 12 Nov 2026 02:52:51 -0700 Subject: [PATCH] Reconcile whitespace where request-time instances meet With minify.html on, each component instance rendered from a server or stream script is minified on its own. Where two were written with nothing between them, the space each kept at its own edge could differ from the build, which minifies the page once: - in a table, where a removed comment leaves an empty separator, the space landed before the separator instead of after it; - an instance that leaves nothing at its edge (empty, or only comments and whitespace) was treated as a neighbor, so the instance beside it kept or doubled a space the build drops. The finalizer now resolves each instance's neighbors past instances that leave nothing, and merges the whitespace and separators at each seam the way the build treats that stretch. The differences were in bytes, not in what a browser renders. The fidelity test loaded each root's components once and reused them for every variant, so some variants paired files loaded with minify.html off with pages built with it on. No build does that (component files are minified as they load), and it is how a comment at the top of a component file reached this code. Components are now loaded per variant. Co-Authored-By: Claude Fable 5.2 --- ...nent-manifest-fidelity.integration.test.ts | 6 + pkg/src/lib/request-render.test.ts | 124 +++++++++ pkg/src/lib/request-render.ts | 241 +++++++++++++++--- 3 files changed, 334 insertions(+), 37 deletions(-) diff --git a/pkg/src/lib/component-manifest-fidelity.integration.test.ts b/pkg/src/lib/component-manifest-fidelity.integration.test.ts index d3cb4bec..e9abf391 100644 --- a/pkg/src/lib/component-manifest-fidelity.integration.test.ts +++ b/pkg/src/lib/component-manifest-fidelity.integration.test.ts @@ -420,6 +420,12 @@ describe.each(ROOTS)("component root $dir", (root) => { config.base = base; serverSidecarRegistry.clear(); clearScopingTemplates(); + // Components are minified as they are loaded (components.ts), so + // the list is this variant's: one kept from another variant would + // pair files loaded under one `minify.html` with pages built under + // the other, which no build does. + invalidateComponentListCache(); + list = await listComponents(root.components); manifest = await buildComponentManifest(list, BascikConfig); }); diff --git a/pkg/src/lib/request-render.test.ts b/pkg/src/lib/request-render.test.ts index 6c5a729e..99357892 100644 --- a/pkg/src/lib/request-render.test.ts +++ b/pkg/src/lib/request-render.test.ts @@ -20,6 +20,7 @@ * `RequestRenderError`, or a once-per-(page, tag) dev log. */ import { beforeEach, describe, expect, it, vi, type Mock } from "vitest"; +import fc from "fast-check"; import { createHash } from "node:crypto"; vi.mock("./config.js", () => ({ @@ -2405,4 +2406,127 @@ describe("usageFinalizer", () => { ); expect(rendered(requestTime)).toBe(rendered(built)); }); + + describe("whitespace around a comment's separator between adjacent instances", () => { + // In a table the build replaces a removed comment with an empty comment + // (the content model there must not change), and of the whitespace runs + // around the comments between two inline elements it keeps one space, at + // the last run. Each instance is minified on its own at request time, so + // the runs on either side of an instance boundary must be reconciled. + const A = `t`; + const instances: Record = { + "leading comment": `\n${A}\n`, + "trailing comment": `${A}\n\n`, + "trailing comment, no final newline": `${A}\n`, + "leading newline": `\n${A}`, + "comment on both sides": `\n${A}\n\n`, + "comment only": `\n`, + "no comment": `${A}\n`, + "block root with a leading comment": `\n
t
\n`, + "two comments": `\n\n${A}\n`, + "comment touching the root": `${A}\n`, + text: `hello\n`, + "text after a comment": `\nhello`, + "text before a comment": `\nhello\n\n`, + "whitespace only": `\n \n`, + empty: ``, + "a script after the root": `${A}\n\n\n`, + }; + const finalize = usageFinalizer({ + minifyHtml: true, + base: "/", + } as ComponentManifest)!; + const names = Object.keys(instances); + const sequences: string[][] = [ + ...names.flatMap((first) => names.map((second) => [first, second])), + ["leading comment", "leading comment", "leading comment"], + ["no comment", "comment only", "leading comment"], + ["trailing comment, no final newline", "comment only", "leading newline"], + ["comment only", "comment only", "no comment"], + ]; + const wrappers: Array<[string, string, string]> = [ + [ + "a table cell", + "

before

", + "

after

", + ], + [ + "a table cell, between inline elements", + "
x", + "y
", + ], + [ + "a formatting element in a table cell", + "
", + "
", + ], + [ + "a 
 in a table cell",
+        "
",
+        "
",
+      ],
+      ["a paragraph", "
", "
"],
+      ["a formatting element", "", ""],
+    ];
+    it.each(wrappers)("matches the build in %s", (_label, open, close) => {
+      for (const sequence of sequences) {
+        const spans: Array<{ name: string; start: number; end: number }> = [];
+        let page = open;
+        for (const _ of sequence) {
+          spans.push({
+            name: "x-c",
+            start: page.length,
+            end: page.length + 11,
+          });
+          page += "";
+        }
+        const pieces = sequence.map((name) => instances[name]);
+        const finished = finalize(page + close, spans, pieces);
+        expect(
+          `${open}${finished.join("")}${close}`,
+          sequence.join(" + "),
+        ).toBe(minifyHtml(`${open}${pieces.join("")}${close}`));
+      }
+    });
+    it("matches the build for any instances written next to each other", () => {
+      const atom = fc.constantFrom(
+        "",
+        "\n",
+        " ",
+        A,
+        "
d
",
+        "s",
+        "hello",
+        "
",
+      );
+      const instance = fc.array(atom, { maxLength: 6 }).map((a) => a.join(""));
+      fc.assert(
+        fc.property(
+          fc.array(instance, { minLength: 2, maxLength: 5 }),
+          fc.constantFrom(...wrappers),
+          // Author text on either side of the run, which is left as written.
+          fc.constantFrom("", "x", "x", "
x
"),
+          fc.constantFrom("", "y", "y", "
y
"),
+          (pieces, [, open, close], lead, tail) => {
+            const spans: Array<{ name: string; start: number; end: number }> =
+              [];
+            let page = open + lead;
+            for (const _ of pieces) {
+              spans.push({
+                name: "x-c",
+                start: page.length,
+                end: page.length + 11,
+              });
+              page += "";
+            }
+            const finished = finalize(page + tail + close, spans, pieces);
+            expect(`${open}${lead}${finished.join("")}${tail}${close}`).toBe(
+              minifyHtml(`${open}${lead}${pieces.join("")}${tail}${close}`),
+            );
+          },
+        ),
+        { numRuns: 3000 },
+      );
+    });
+  });
 });
diff --git a/pkg/src/lib/request-render.ts b/pkg/src/lib/request-render.ts
index b0f9b062..c35b6859 100644
--- a/pkg/src/lib/request-render.ts
+++ b/pkg/src/lib/request-render.ts
@@ -70,11 +70,11 @@ import {
   DOCUMENT_FRAGMENT_CONTEXT,
   fragmentContexts,
   fragmentEdges,
-  leadingCollapsibleWhitespace,
-  trailingCollapsibleWhitespace,
   minifyHtmlFragment,
   type FragmentContext,
+  type FragmentNeighbor,
 } from "./html-minifier.ts";
+import { SEAM_SEPARATOR } from "./html-scanner.ts";
 import {
   applyScopingResult,
   instantiateTemplate,
@@ -499,6 +499,174 @@ export const expandByUsage = (
   return pieces.join("");
 };
 
+/** One piece of the whitespace and comment separators at an instance's edge. */
+interface SeamToken {
+  text: string;
+  whitespace: boolean;
+  keep: boolean;
+}
+
+/**
+ * Split `html` into the whitespace and separators at its start, what lies
+ * between, and those at its end. A piece that is nothing else is all `lead`.
+ */
+const splitSeams = (
+  html: string,
+): { lead: SeamToken[]; middle: string; trail: SeamToken[] } => {
+  const isSpace = (index: number): boolean => {
+    const code = html.charCodeAt(index);
+    return code === 32 || code === 10 || code === 9 || code === 13;
+  };
+  const lead: SeamToken[] = [];
+  let start = 0;
+  while (start < html.length) {
+    if (isSpace(start)) {
+      let end = start + 1;
+      while (end < html.length && isSpace(end)) end++;
+      lead.push({
+        text: html.slice(start, end),
+        whitespace: true,
+        keep: true,
+      });
+      start = end;
+    } else if (html.startsWith(SEAM_SEPARATOR, start)) {
+      lead.push({ text: SEAM_SEPARATOR, whitespace: false, keep: true });
+      start += SEAM_SEPARATOR.length;
+    } else break;
+  }
+  const trail: SeamToken[] = [];
+  let end = html.length;
+  while (end > start) {
+    if (isSpace(end - 1)) {
+      let from = end - 1;
+      while (from > start && isSpace(from - 1)) from--;
+      trail.push({ text: html.slice(from, end), whitespace: true, keep: true });
+      end = from;
+    } else if (
+      end - SEAM_SEPARATOR.length >= start &&
+      html.startsWith(SEAM_SEPARATOR, end - SEAM_SEPARATOR.length)
+    ) {
+      trail.push({ text: SEAM_SEPARATOR, whitespace: false, keep: true });
+      end -= SEAM_SEPARATOR.length;
+    } else break;
+  }
+  trail.reverse();
+  return { lead, middle: html.slice(start, end), trail };
+};
+
+/** What `mergeSeamWhitespace` needs to know about each finished instance. */
+interface SeamNeighbors {
+  /** What the instance's minified form starts and ends with ("none": nothing). */
+  edges: (index: number) => { first: FragmentNeighbor; last: FragmentNeighbor };
+  /** What renders before and after each instance, past any that leave nothing. */
+  before: readonly FragmentNeighbor[];
+  after: readonly FragmentNeighbor[];
+}
+
+/**
+ * Reconcile the whitespace where instances written with nothing between them
+ * meet. Each was minified on its own, so each kept the space its own edge
+ * called for; in the built page those edges are one stretch of whitespace
+ * and comment separators (a removed comment leaves an empty one in a table)
+ * between two things that render, and of that stretch the build keeps:
+ *
+ * - whitespace that touches text on either side, which is part of that text;
+ * - while a formatting element may be open, one space per whitespace run;
+ * - otherwise one space for every other run together, at the last of them,
+ *   and none when text is on either side of the stretch.
+ *
+ * Two runs that now touch are one run. An instance only ever keeps a space
+ * the build may also keep at that run, so this only removes. Inside a
+ * `
`, and where the page could not be followed, the instances are as
+ * written and stay so. One pass over the pieces' edges.
+ */
+const mergeSeamWhitespace = (
+  finished: string[],
+  spans: readonly UsageSpan[],
+  contexts: readonly FragmentContext[],
+  neighbors: SeamNeighbors,
+): string[] => {
+  if (finished.length < 2) return finished;
+  let changed = false;
+  /** The stretch being read: its tokens, and what it started after. */
+  let stretch: SeamToken[] = [];
+  let afterText = false;
+  let formatting = false;
+  const close = (beforeText: boolean): void => {
+    const runs: SeamToken[] = [];
+    let previous: SeamToken | undefined;
+    for (const token of stretch) {
+      if (!token.whitespace) {
+        previous = undefined;
+        continue;
+      }
+      if (previous) {
+        token.keep = false;
+        changed = true;
+      } else {
+        runs.push(token);
+        previous = token;
+      }
+    }
+    if (!formatting && runs.length > 0) {
+      // Runs that are part of the text on either side stay with it.
+      const first = stretch[0];
+      const last = stretch[stretch.length - 1];
+      let from = afterText && first.whitespace ? 1 : 0;
+      let to = runs.length;
+      if (beforeText && last.whitespace && runs.length > from) to--;
+      // A single run between two texts is both; it is counted once.
+      if (from > to) from = to;
+      // The build keeps that space only between two inline elements. Each
+      // instance checked its own neighbors, but one that follows text was
+      // minified as if it touched that text, which a comment separator
+      // between them undoes.
+      const keepLast = !beforeText && !afterText;
+      for (let index = from; index < to; index++) {
+        if (keepLast && index === to - 1) break;
+        runs[index].keep = false;
+        changed = true;
+      }
+    }
+    stretch = [];
+  };
+  let open = false;
+  const parts = finished.map((piece, index) => {
+    const context = contexts[index];
+    const skipped = context.inPre || context.verbatim;
+    const adjacent = index > 0 && spans[index].start === spans[index - 1].end;
+    if (open && (skipped || !adjacent)) {
+      // The stretch ran to the end of its run of instances.
+      close(neighbors.after[index - 1] === "text");
+      open = false;
+    }
+    if (skipped) return undefined;
+    if (!open) {
+      open = true;
+      afterText = neighbors.before[index] === "text";
+      formatting = context.formatting;
+    }
+    const seams = splitSeams(piece);
+    stretch.push(...seams.lead);
+    if (seams.middle !== "") {
+      const { first, last } = neighbors.edges(index);
+      close(first === "text");
+      afterText = last === "text";
+      formatting = context.formatting;
+      stretch.push(...seams.trail);
+    }
+    return seams;
+  });
+  if (open) close(neighbors.after[finished.length - 1] === "text");
+  if (!changed) return finished;
+  const text = (tokens: SeamToken[]): string =>
+    tokens.map((token) => (token.keep ? token.text : "")).join("");
+  return finished.map((piece, index) => {
+    const seams = parts[index];
+    return seams ? text(seams.lead) + seams.middle + text(seams.trail) : piece;
+  });
+};
+
 /**
  * The request path's finish for each expanded top-level usage. The build
  * rewrites base paths in, then minifies, the whole page once
@@ -520,42 +688,41 @@ export const usageFinalizer = (
         if (!manifest.minifyHtml) return rebased;
         const contexts = fragmentContexts(html, spans, outer);
         // Two usages written next to each other are each other's neighbors
-        // in the page as their instances, not as their usage tags.
-        const edges = new Map>();
-        const edgesOf = (index: number) => {
-          let found = edges.get(index);
-          if (!found) edges.set(index, (found = fragmentEdges(rebased[index])));
-          return found;
-        };
-        const finished = rebased.map((piece, index) => {
-          const context = { ...contexts[index] };
-          if (context.beforeIsSpan) {
-            const last = edgesOf(index - 1).last;
-            if (last !== "none") context.before = last;
-          }
-          if (context.afterIsSpan) {
-            const first = edgesOf(index + 1).first;
-            if (first !== "none") context.after = first;
-          }
-          return minifyHtmlFragment(piece, context);
-        });
-        // Whitespace at the end of one instance and the start of the next,
-        // written with nothing between them, is one text run in the built
-        // page and collapses to one space; outside a 
 it is kept once.
-        for (let index = 1; index < finished.length; index++) {
-          const context = contexts[index];
-          if (
-            context.inPre ||
-            context.verbatim ||
-            spans[index].start !== spans[index - 1].end ||
-            trailingCollapsibleWhitespace(finished[index - 1]) === 0
-          )
-            continue;
-          finished[index] = finished[index].slice(
-            leadingCollapsibleWhitespace(finished[index]),
-          );
+        // in the page as their instances, not as their usage tags. An
+        // instance that leaves nothing at its edge (it is empty, or only
+        // comments and whitespace) is no neighbor: the one past it is, and
+        // past the last of them whatever the output has there.
+        const edges: Array | undefined> = [];
+        const edgesOf = (index: number) =>
+          (edges[index] ??= fragmentEdges(rebased[index]));
+        // Resolved from the far end of each run of usages, so a run of
+        // instances that leave nothing is walked once, not once per member.
+        const before: FragmentNeighbor[] = [];
+        for (let index = 0; index < rebased.length; index++) {
+          before[index] = contexts[index].before;
+          if (!contexts[index].beforeIsSpan) continue;
+          const last = edgesOf(index - 1).last;
+          before[index] = last !== "none" ? last : before[index - 1];
+        }
+        const after: FragmentNeighbor[] = [];
+        for (let index = rebased.length - 1; index >= 0; index--) {
+          after[index] = contexts[index].after;
+          if (!contexts[index].afterIsSpan) continue;
+          const first = edgesOf(index + 1).first;
+          after[index] = first !== "none" ? first : after[index + 1];
         }
-        return finished;
+        const finished = rebased.map((piece, index) =>
+          minifyHtmlFragment(piece, {
+            ...contexts[index],
+            before: before[index],
+            after: after[index],
+          }),
+        );
+        return mergeSeamWhitespace(finished, spans, contexts, {
+          edges: edgesOf,
+          before,
+          after,
+        });
       }
     : undefined;