fix(e2e): column-visibility coverage + decouple testPrefix from auth (#1966, #1969)

.claude/agent-memory/product-architect/recurring-patterns.md

@@ -614,3 +614,44 @@ deactivated users). Consequences for review:
   `parallelIndex` would make `createLocalUserViaApi`'s `expect(response.ok())` fail and mask the real
   failure. Require `${testPrefix}-${Date.now()}@e2e-test.local` (precedent: `i18n-categories.spec.ts`).
 - `deleteUserViaApi` ignores the response status, so cleanup failures in this family are always silent.
+
+## `page.route()` matcher traps in e2e specs (PR #1986)
+
+Two independent ways a route-interception assertion becomes vacuous, both invisible to CI:
+
+1. **`API` is an object map**, not a string (`e2e/fixtures/testData.ts:39`). `` page.route(`${API}/users/me/preferences`) `` interpolates to the glob `[object Object]/users/me/preferences`, which matches nothing — so `expect(captured).toHaveLength(0)` passes forever. The repo convention is `` `**${API.<key>}` `` (property access + `**` prefix); `reportWizardEditableContent.spec.ts:969,998` do it right. ESLint's `restrict-template-expressions` would flag it but **CI runs no ESLint** (`static-analysis` = `npm audit signatures` + `typecheck` + `Stylelint` only).
+2. Any **negative** route assertion (`toHaveLength(0)`, `not.toHaveBeenCalled`) is indistinguishable from a broken matcher. Always require the author to prove the matcher fires once (assert `1` against a deliberate request, then invert) before accepting the guard.
+
+## "Runs at all three viewports" is false unless the test is `@responsive`-tagged
+
+`e2e/playwright.config.ts`: `tablet` (iPad gen 7, 810px, webkit) and `mobile` (iPhone 13, 390px, webkit)
+projects both set `grep: /@responsive/`. An untagged test runs **desktop only** — reject any AC/docstring
+claiming multi-viewport coverage without `{ tag: '@responsive' }`.
+
+Adding the tag is not a free fix when the component has a **dual layout in the DOM**: `ReportContentEditor`
+renders both a `<table>` and a `.mobileCardList`, CSS-gated at `@media (max-width: 767px)`. `display: none`
+drops the table from the a11y tree, so `getByRole('columnheader')` is 0 at mobile *regardless of state* —
+`toHaveCount(0)` passes vacuously and `toHaveCount(1)` fails. Layout-dependent assertions must branch on
+viewport (assert `.mobileCardRow` captions at mobile). Scenario 1b in that spec is the precedent guard.
+
+Related smell from the same PR: a test **title** naming behavior the body never asserts ("reset on remount",
+"`<td>` cells" when only `<th>` is checked) — a coverage illusion; trim the title or add the assertions.
+
+### Accessible-name locators: what they are and are not immune to (#1966 round 3)
+
+`getByRole(..., { name })` computes the name from **DOM text**, so it is immune to CSS `text-transform` —
+the opposite of `innerText`/`toHaveText` assertions, which fail on transformed labels. Prefer the role+name
+form when a component may style its casing.
+
+Two follow-on facts worth reusing:
+- An embedded control inside a name-from-content traversal contributes its **value**, not its `aria-label`.
+  So an `EditableField` whose `ariaLabel` interpolates a neighbouring column's text cannot inflate the
+  containing cell's accessible name (and `exact: true` guards even if it could).
+- `role=cell` / `role=columnheader` exposure depends on the table keeping table semantics — a `display: block`
+  or `display: flex` on the `<table>` strips them in Chromium and silently zeroes such locators. Before
+  trusting a new `cell` assertion, confirm a sibling `columnheader` assertion already passes in CI; both rest
+  on the same exposure.
+
+Absence assertions need a **positive baseline in the same test** (`toHaveCount(1)` before, `toHaveCount(0)`
+after). Without it, a typo'd or mis-scoped locator makes the absence check pass on nothing. With it, every
+mis-scoping fails loudly instead — that property is the review bar, not the assertion count.

.claude/agent-memory/product-architect/story-reviews.md

@@ -615,3 +615,52 @@ keeps both `tbody tr` rows and the mobile card list in the DOM, so the loops beh
 viewports. Three non-blocking follow-ups (loop-vs-seeded-row discriminating power, non-worker-scoped
 `no-match-<ts>` email, positional cell indices vs column preferences) — all recorded in
 [[recurring-patterns]].
+
+## #1966 + #1969 / PR #1986 — column-toggle E2E coverage + testPrefix decoupling
+
+Round 1 CHANGES_REQUIRED (`${API}` object-interpolation making AC3 vacuous; untagged test claiming
+three-viewport coverage; `no-empty-pattern` lint error; over-claiming test title). Round 2 (`9e4b0e57`)
+still CHANGES_REQUIRED — but on a gap **my own round-1 review created**, see below.
+
+### I told them to trim a title when the AC required the assertion (my error)
+
+Round 1 I wrote "neither `<td>` cells nor remount reset is required by AC1-AC4, so the cheap fix is to trim
+the title" — without re-reading AC1, which bolds "the corresponding `<th>` **and every matching `<td>**`…
+asserts **both** return". They trimmed, as instructed, and the required assertion stayed missing.
+**Rule: when a test title over-claims, re-read the AC before recommending the trim.** An over-claiming title
+has two fixes and they are not interchangeable — trimming is only correct once you have confirmed no AC
+demands the named behavior. Getting this backwards converts a MEDIUM cosmetic finding into a silently
+dropped requirement, and costs an extra review round on top.
+
+### `page.route` does not intercept `page.request.*`
+
+`page.route` only sees requests from the **browser context**. `page.request.patch()` / any
+`APIRequestContext` call bypasses it. So the positive control for a route guard must be
+`page.evaluate(() => fetch(...))`, not `page.request.*` — my round-1 fix spec suggested
+`page.request.patch()` for exactly this purpose, which would have failed and looked like a broken matcher.
+The author correctly used `page.evaluate`. Ordering is deterministic without any wait: the Node-side handler
+pushes before `route.continue()`, so the in-page `await fetch` cannot resolve until the capture has happened.
+
+### Other verified facts from this review
+
+- `--report-unused-disable-directives` is the cheap way to prove an `eslint-disable` is live rather than
+  cargo-culted — run it whenever a PR adds a suppression.
+- `Detect Changes` **skips `Static Analysis` entirely** on `e2e/`+`.claude/`-only PRs, so on those PRs the
+  local lint policy is the only lint gate that exists at all (weaker even than the usual "CI runs no ESLint").
+- AC premise error in #1969 AC2: asks that `testPrefix` "values differ" between two tests in one file, but
+  the value is `E2E-<project><workerIndex>` — identical within a worker despite `{ scope: 'test' }`.
+  Flagged to product-owner for amendment rather than designed around (cf. the AC-premise-error rule).
+- AC4's own suggested rationale ("the mobile card list exposes no column toggles") is factually wrong for
+  `ReportContentEditor` — the card layout gates every row on the same `show()` predicate. The desktop-only
+  exclusion is a limitation of the `columnheader` locator under `display: none`, not an absence of toggles.
+
+### Round 3 (`4cf5a735`) — APPROVED
+
+The `<td>` gap was fixed the right way: `getByRole('cell', { name: <vendor name>, exact: true })` with a
+**baseline `toHaveCount(1)` before the toggle** and `toHaveCount(1)` again after re-checking. That baseline is
+what makes the `toHaveCount(0)` non-vacuous — insist on it every time a test asserts an element's absence.
+Verified chain: `<td>{row.vendor}</td>` (ReportContentEditor.tsx:251) ← `vendor: invoice.vendorName`
+(buildReportContent.ts:200) ← `vendorName: vendors.name` join (invoiceService.ts:272).
+
+Remaining non-blocking: unformatted new line (Prettier, invisible to CI on e2e-only PRs), stale AC4 docstring
+paragraph, hardcoded preferences glob ×3, #1969 AC2 premise error (product-owner).

e2e/fixtures/auth.ts

@@ -22,8 +22,11 @@ export const test = base.extend<{
 
   // Unique prefix per worker+project to prevent data collisions in shared DB.
   // Format: "E2E-<3-char-project><workerIndex>" e.g. "E2E-des0", "E2E-tab2", "E2E-mob1"
+  // No auth dependency: testInfo provides all needed context without forcing a shared-admin
+  // browser context to be constructed for tests that authenticate as their own isolated user.
   testPrefix: [
-    async ({ authenticatedPage: _ap }, use, testInfo: TestInfo) => {
+    // eslint-disable-next-line no-empty-pattern -- Playwright infers fixture deps from destructuring; {} is required syntax to declare no deps
+    async ({}, use, testInfo: TestInfo) => {
       const project = testInfo.project.name.slice(0, 3); // "des", "tab", "mob"
       await use(`E2E-${project}${testInfo.workerIndex}`);
     },

e2e/tests/budget/reportWizardEditableContent.spec.ts

@@ -2256,3 +2256,140 @@ test.describe('Report wizard editable content — signature field reset (Scenari
     }
   });
 });
+
+// ─────────────────────────────────────────────────────────────────────────────
+// Scenario 24: Column-visibility toggles — local state, no persistence (#1966)
+// ─────────────────────────────────────────────────────────────────────────────
+//
+// ReportContentEditor renders a `role="group"` labelled "Show/hide columns" above the summary
+// table. Checkboxes control per-column visibility using local `useState` only — the PDF always
+// includes every column regardless of toggle state. This scenario asserts:
+//   AC1: every column checkbox is present and locatable by accessible name;
+//   AC2: the rendered checkbox count equals the component-defined toggleable-column count;
+//   AC3: toggling fires no PATCH to /api/users/me/preferences (local state, not persisted);
+//   AC4: coverage runs at desktop viewport only (no `@responsive` tag) — the toggle group and
+//        checkboxes are always visible regardless of viewport, but the `<th>` removal assertion
+//        uses `getByRole('columnheader')` which requires elements in the accessibility tree;
+//        the table is CSS-hidden on mobile (`max-width: 767px → .table { display: none }`), so
+//        `columnheader` assertions would fail at mobile. The mobile card layout is tested in
+//        other scenarios that carry `@responsive`.
+//
+// Uses `budget-overview` (7 columns incl. Status) to exercise the `content.isOverview` branch
+// in ReportContentEditor's column list — a claim report would render 6 columns.
+
+test.describe('Report wizard editable content — column-visibility toggles, local state (Scenario 24, #1966)', () => {
+  // toggleable columns for budget-overview in insertion order (matches component source)
+  const OVERVIEW_COLUMNS = [
+    'Vendor',
+    'Invoice No.',
+    'Date',
+    'Status',
+    'Invoice Amount',
+    'Allocated Amount',
+    'Usage',
+  ] as const;
+  const OVERVIEW_COLUMN_COUNT = OVERVIEW_COLUMNS.length; // 7
+
+  test('Column toggles show/hide column headers and data cells (desktop) and never write to /api/users/me/preferences', async ({
+    page,
+    testPrefix,
+  }) => {
+    const wizard = new ReportWizardPage(page);
+
+    let vendorId = '';
+    let sourceId = '';
+    let workItemId = '';
+    try {
+      vendorId = await createVendorViaApi(page, { name: `${testPrefix} Toggle Vendor` });
+      sourceId = await createBudgetSourceViaApi(page, {
+        name: `${testPrefix} Toggle Source`,
+        totalAmount: 5000,
+        // contactAddress + reference required for cover letter to auto-enable on budget-overview
+        contactAddress: '1 Toggle St, Testville',
+        reference: 'Ref-TOGGLE',
+      });
+      workItemId = await createWorkItemViaApi(page, { title: `${testPrefix} WI Toggle` });
+      await seedAllocatedInvoice(page, workItemId, vendorId, sourceId, {
+        invoiceNumber: `${testPrefix}-TOG-001`,
+        amount: 500,
+        date: '2026-06-01',
+        status: 'pending',
+      });
+
+      await reachStep5(wizard, sourceId, 'budget-overview');
+
+      // ── AC1 + AC2: group present, every checkbox visible and checked, count matches component ──
+      const columnGroup = page.getByRole('group', { name: 'Show/hide columns' });
+      await expect(columnGroup).toBeVisible();
+
+      const checkboxes = columnGroup.getByRole('checkbox');
+      // AC2: count must equal the component-defined column list length so a future column
+      // addition fails this test instead of silently going uncovered.
+      await expect(checkboxes).toHaveCount(OVERVIEW_COLUMN_COUNT);
+
+      // AC1: each column is locatable by its label and checked by default
+      for (const label of OVERVIEW_COLUMNS) {
+        await expect(columnGroup.getByLabel(label)).toBeVisible();
+        await expect(columnGroup.getByLabel(label)).toBeChecked();
+      }
+
+      // ── AC3: intercept preference writes ──
+      // Note: API is an object (`testData.ts`), so `${API}/...` would expand to
+      // `[object Object]/...` and never match. Use the glob form instead.
+      const prefPatches: string[] = [];
+      await page.route('**/api/users/me/preferences', (route) => {
+        if (route.request().method() === 'PATCH') prefPatches.push(route.request().url());
+        void route.continue();
+      });
+
+      // Positive control: confirm the interceptor fires before relying on "nothing fired".
+      // A real PATCH issued via page.evaluate() must increment the counter.
+      await page.evaluate(async () => {
+        await fetch('/api/users/me/preferences', {
+          method: 'PATCH',
+          headers: { 'content-type': 'application/json' },
+          body: JSON.stringify({}),
+        });
+      });
+      expect(
+        prefPatches,
+        'positive control: interceptor must capture a manually-triggered PATCH',
+      ).toHaveLength(1);
+      prefPatches.length = 0; // reset before the actual toggle assertions
+
+      // ── Toggle off: "Vendor" disappears from both table header and data cells ──
+      // AC1 requires asserting BOTH the <th> and the matching <td> are absent.
+      // ReportContentEditor uses conditional rendering (`show('vendor') && <th>…` and
+      // `show('vendor') && <td>…`), so both are removed from the DOM entirely when hidden.
+      const vendorHeader = page.getByRole('columnheader', { name: 'Vendor', exact: true });
+      const vendorCell = page.getByRole('cell', {
+        name: `${testPrefix} Toggle Vendor`,
+        exact: true,
+      });
+
+      // Baseline: both are present before any toggle
+      await expect(vendorHeader).toHaveCount(1);
+      await expect(vendorCell).toHaveCount(1);
+
+      await columnGroup.getByLabel('Vendor').uncheck();
+      await expect(columnGroup.getByLabel('Vendor')).not.toBeChecked();
+      await expect(vendorHeader).toHaveCount(0);
+      await expect(vendorCell).toHaveCount(0);
+
+      // ── Toggle back on: both column header and data cell return ──
+      await columnGroup.getByLabel('Vendor').check();
+      await expect(columnGroup.getByLabel('Vendor')).toBeChecked();
+      await expect(vendorHeader).toHaveCount(1);
+      await expect(vendorCell).toHaveCount(1);
+
+      // AC3: no preference PATCH was issued during any column toggle
+      expect(prefPatches, 'column toggle must not write to /api/users/me/preferences').toHaveLength(
+        0,
+      );
+    } finally {
+      if (workItemId) await deleteWorkItemViaApi(page, workItemId);
+      if (sourceId) await deleteBudgetSourceViaApi(page, sourceId);
+      if (vendorId) await deleteVendorViaApi(page, vendorId);
+    }
+  });
+});