Audit e2e/ and playwright.config.ts comments (#225 Phase 2)

e2e/index.spec.ts

@@ -16,8 +16,7 @@ test.describe("index.html", () => {
 
   test("language select change is persisted in localStorage", async ({ page }) => {
     await page.goto("/index.html");
-    // The label-lang select is populated asynchronously by LabelHandler; wait
-    // for at least one extra option to appear beyond the default.
+    // WHY: LabelHandler populates the select asynchronously, so wait for options before interacting.
     await expect
       .poll(async () => await page.locator("#label-lang option").count())
       .toBeGreaterThan(1);
@@ -32,8 +31,6 @@ test.describe("index.html", () => {
 
   test("min-tier cannot exceed max-tier — they correct each other", async ({ page }) => {
     await page.goto("/index.html");
-    // Set min above current max; min-max-inputs.ts should drag max up so
-    // max >= min.
     await page.locator("#max-tier").evaluate((el: HTMLInputElement) => {
       el.value = "2";
       el.dispatchEvent(new Event("input", { bubbles: true }));
@@ -52,18 +49,15 @@ test.describe("index.html", () => {
     test.setTimeout(60_000);
     await page.goto("/index.html");
 
-    // Wait for the bundled-map select to be populated.
+    // WHY: The bundled-map select is populated asynchronously.
     await expect
       .poll(async () =>
         await page.locator("#bundled-map-select option").count()
       )
       .toBeGreaterThan(1);
 
-    // Install an observer that latches when the load/processing dialog opens
-    // and when it subsequently closes. The dialog is opened via showModal()
-    // and closed once processing finishes; using the close transition as the
-    // "processing done" signal avoids polling canvas dimensions before the
-    // worker has resized the canvas.
+    /* WHY: Latch dialog open/close via MutationObserver so we can detect
+       when processing finishes without polling canvas dimensions prematurely. */
     await page.evaluate(() => {
       const w = globalThis as unknown as {
         __dialogWasOpen?: boolean;
@@ -80,7 +74,6 @@ test.describe("index.html", () => {
 
     await page.selectOption("#bundled-map-select", "Navezgane");
 
-    // Loading dialog should have been open at some point during processing.
     await expect
       .poll(async () =>
         await page.evaluate(() =>
@@ -90,9 +83,7 @@ test.describe("index.html", () => {
       )
       .toBe(true);
 
-    // Wait for the dialog to close, signalling processing finished. Cold
-    // workers under parallel test load can take tens of seconds, so allow a
-    // generous window before checking canvas dimensions.
+    // WHY: Cold workers under parallel test load can take tens of seconds, so allow a generous timeout before asserting canvas dimensions.
     await expect
       .poll(
         async () =>
@@ -104,8 +95,7 @@ test.describe("index.html", () => {
       )
       .toBe(true);
 
-    // Canvas dimensions: Navezgane HeightMapSize is 6144x6144 and the default
-    // render scale is 0.12 -> 6144 * 0.12 = 737.28 -> 737.
+    // WHY: Navezgane HeightMapSize is 6144x6144 and the default render scale is 0.12, so the expected canvas size is 6144 * 0.12 = 737.28 -> 737.
     const slow = { timeout: 20_000 };
     await expect.poll(
       async () =>
@@ -118,19 +108,16 @@ test.describe("index.html", () => {
       slow,
     ).toBe(737);
 
-    // Prefab list is populated.
     await expect.poll(
       async () => await page.locator("#prefabs-list li").count(),
       slow,
     ).toBeGreaterThan(0);
 
-    // Map name input reflects the selection.
     await expect.poll(
       async () => await page.locator("#map-name").inputValue(),
       slow,
     ).toBe("Navezgane");
 
-    // Apply the "trader" prefab-filter preset.
     await page.click(
       'button[data-input-prefab-filter="trader"]:text-is("All Traders")',
     );
@@ -141,15 +128,13 @@ test.describe("index.html", () => {
       await page.locator("#prefabs-list li").count()
     ).toBeGreaterThan(0);
 
-    // Clear the prefab filter via its X button (data-input-prefab-filter="").
     await page.click(
       'button[data-input-prefab-filter=""]',
     );
     await expect.poll(async () =>
       await page.locator("#prefab-filter").inputValue()
     ).toBe("");
 
-    // Apply the "Super Corn" block-filter preset.
     await page.click(
       'button[data-input-block-filter="(Grace|Super)Corn"]:text-is("Super Corn")',
     );
@@ -160,8 +145,6 @@ test.describe("index.html", () => {
       await page.locator("#prefabs-list li").count()
     ).toBeGreaterThan(0);
 
-    // Narrow the tier range to [0, 0]; with Super Corn still in the block
-    // filter, no prefab should match.
     await page.locator("#max-tier").evaluate((el: HTMLInputElement) => {
       el.value = "0";
       el.dispatchEvent(new Event("input", { bubbles: true }));
@@ -180,8 +163,7 @@ test.describe("index.html", () => {
   test("block filter input stays responsive (freeze regression)", async ({ page }) => {
     test.setTimeout(60_000);
 
-    // Collect long tasks with their start times so we can look only at the
-    // window around typing, ignoring long tasks from the initial map load.
+    // WHY: Collect long-task timings so we can isolate typing-window latency from initial load overhead.
     await page.addInitScript(() => {
       const w = globalThis as unknown as {
         __longtasks?: { start: number; duration: number }[];
@@ -248,8 +230,7 @@ test.describe("index.html", () => {
     }, typingStart);
     const maxSingle = durations.length ? Math.max(...durations) : 0;
     const total = durations.reduce((a, b) => a + b, 0);
-    // Thresholds are loose vs the pre-fix baseline (single 1,864ms / total
-    // 5,100ms) to tolerate CI speed variance; prefer retries over loosening.
+    // WHY: Thresholds are loose versus the pre-fix baseline (single 1,864ms / total 5,100ms) so CI speed variance does not cause flakes.
     expect(maxSingle).toBeLessThan(500);
     expect(total).toBeLessThan(1500);
 
@@ -272,10 +253,8 @@ test.describe("index.html", () => {
       "terrain-viewer-title",
     );
 
-    // Show button is disabled until a DTM is loaded.
     await expect(page.locator("#terrain-viewer-show")).toBeDisabled();
 
-    // Close button carries an accessible name for screen readers.
     await expect(page.locator("#terrain-viewer-close"))
       .toHaveAttribute("aria-label", "Close terrain viewer");
   });
@@ -300,8 +279,7 @@ test.describe("index.html", () => {
     );
     await expect(button).toBeVisible();
 
-    // Replace clipboard.writeText with a spy stored on globalThis so the test
-    // can assert it was invoked by a keyboard activation.
+    // WHY: Replace clipboard.writeText with a spy on globalThis so the test can assert keyboard activation invoked it.
     await page.evaluate(() => {
       const w = globalThis as unknown as {
         __copyCalls: string[];

e2e/prefabs.spec.ts

@@ -6,7 +6,7 @@ test.describe("prefabs.html", () => {
     await page.goto("/prefabs.html");
     await expect(page.locator("#prefab-filter")).toBeVisible();
     await expect(page.locator("#prefabs-list")).toBeAttached();
-    // The status updates when the worker finishes the first filter pass.
+    // WHY: Poll until the worker finishes the first filter pass so the status text is non-empty.
     await expect.poll(async () =>
       (await page.locator("#prefabs-status").textContent())?.trim().length ?? 0
     ).toBeGreaterThan(0);
@@ -15,7 +15,7 @@ test.describe("prefabs.html", () => {
   test("filter input updates the URL querystring", async ({ page }) => {
     await page.goto("/prefabs.html");
     await page.fill("#prefab-filter", "trader");
-    // URL update is debounced behind UrlState; poll until it shows up.
+    // WHY: UrlState debounces updates, so poll until the querystring reflects the input.
     await expect.poll(() =>
       new URL(page.url()).searchParams.get("prefab-filter")
     )
@@ -36,21 +36,17 @@ test.describe("prefabs.html", () => {
   });
 
   test("highlighted prefab names do not execute injected scripts (XSS regression)", async ({ page }) => {
-    // The XSS surface was the highlighted name being injected via innerHTML.
-    // We can't easily plant a malicious-named prefab without controlling the
-    // backing JSON, so we verify (a) the page does not surface any
-    // accidental dialog (alert/confirm/prompt) within a short window, and
-    // (b) <mark> elements are produced when filtering — which proves the
-    // highlighter path runs and uses the escape-then-mark pipeline added in
-    // commit 92abcb8c.
+    /* WHY: We can't easily plant a malicious-named prefab without controlling
+       the backing JSON. We therefore verify (a) no accidental dialog fires
+       and (b) <mark> elements are produced, which proves the escape-then-mark
+       pipeline added in commit 92abcb8c is active. */
     let dialogTriggered = false;
     page.on("dialog", async (d) => {
       dialogTriggered = true;
       await d.dismiss();
     });
     await page.goto("/prefabs.html");
     await page.fill("#prefab-filter", "house");
-    // Wait for at least one filtered result.
     await expect
       .poll(async () => await page.locator("#prefabs-list li").count())
       .toBeGreaterThan(0);

playwright.config.ts

@@ -10,9 +10,9 @@ export default defineConfig({
   fullyParallel: true,
   forbidOnly: !!CI,
   retries: CI ? 2 : 0,
-  // The bundled-map load test fetches ~5MB of assets through the dev server.
-  // Cap workers to keep server contention bounded even though serve:static is
-  // sturdier than esbuild's serve under load.
+  /** The bundled-map load test fetches ~5MB of assets through the dev server.
+   *  Cap workers to keep server contention bounded even though serve:static is
+   *  sturdier than esbuild's serve under load. */
   workers: CI ? 1 : 2,
   reporter: CI ? [["html", { open: "never" }], ["list"]] : "list",
   timeout: 30_000,

tools/lint-plugins/require-comment-rationale.ts

@@ -17,18 +17,9 @@ const PRESERVE_PATTERNS: readonly string[] = [
   "FIXME",
 ];
 
-/* WHY: gradual rollout ratchet for #225. Entries skip the rule so existing code
-   is not audited in one big sweep; shrink to empty, then delete this mechanism.
-   Match is POSIX-style substring on the absolute filename. */
-const EXCLUDED_PATHS: readonly string[] = [
-  "/e2e/index.spec.ts",
-  "/e2e/prefabs.spec.ts",
-  "/playwright.config.ts",
-];
-
 const TRIPLE_SLASH_RE = /^\/\s*<(reference|amd-)/;
 
-export function shouldPreserve(
+function shouldPreserve(
   comment: { type: string; value: string },
 ): boolean {
   const raw = comment.value;
@@ -39,20 +30,11 @@ export function shouldPreserve(
   return PRESERVE_PATTERNS.some((p) => raw.includes(p));
 }
 
-export function isExcludedFile(
-  filename: string,
-  excludedPaths: readonly string[] = EXCLUDED_PATHS,
-): boolean {
-  const normalized = filename.replaceAll("\\", "/");
-  return excludedPaths.some((p) => normalized.includes(p));
-}
-
 const plugin: Deno.lint.Plugin = {
   name: "local",
   rules: {
     "require-comment-rationale": {
       create(context) {
-        if (isExcludedFile(context.filename)) return {};
         return {
           Program() {
             for (const comment of context.sourceCode.getAllComments()) {

tools/test/require-comment-rationale.test.ts

@@ -1,9 +1,7 @@
 /// <reference lib="deno.unstable" />
 import { expect } from "@std/expect";
 import { describe, it } from "@std/testing/bdd";
-import plugin, {
-  isExcludedFile,
-} from "../lint-plugins/require-comment-rationale.ts";
+import plugin from "../lint-plugins/require-comment-rationale.ts";
 
 function run(source: string, filename = "main.ts") {
   return Deno.lint.runPlugin(plugin, filename, source);
@@ -87,18 +85,6 @@ describe("require-comment-rationale", () => {
     expect(d.length).toBe(1);
   });
 
-  it("skips files matching excluded path substrings", () => {
-    expect(isExcludedFile("/repo/src/index/foo.ts", ["/src/index/"])).toBe(
-      true,
-    );
-    expect(isExcludedFile("/repo/src/lib/bar.ts", ["/src/index/"])).toBe(false);
-    expect(isExcludedFile("C:\\repo\\src\\index\\foo.ts", ["/src/index/"]))
-      .toBe(true);
-    expect(isExcludedFile("/repo/src/lib/foo.ts", ["/src/lib/foo.ts"])).toBe(
-      true,
-    );
-  });
-
   it("flags multiple bare comments in one file", () => {
     const d = run(
       "// first\n// second\n// WHY: keep me\n// fourth\nconst x = 1;\n",