.convoys/.metrics.jsonl

@@ -72,3 +72,5 @@
 {"ts": "2026-06-13T04:14:48Z", "role": "role-implementer", "convoy": "rotate-default-admin", "repo": "tcg-vault", "skip_flags": [], "classification": "security", "duration_s": 900, "outcome": "pr-open"}
 {"ts": "2026-06-13T06:16:56Z", "role": "role-architect", "convoy": "enable-no-undef-eslint-rule", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 180, "outcome": "architecture-only"}
 {"ts": "2026-06-13T06:16:56Z", "role": "role-implementer", "convoy": "enable-no-undef-eslint-rule", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 900, "outcome": "pr-open"}
+{"ts": "2026-06-13T06:20:00Z", "role": "role-architect", "convoy": "scanner-disambiguation-render-test", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 180, "outcome": "architecture-only"}
+{"ts": "2026-06-13T06:20:00Z", "role": "role-implementer", "convoy": "scanner-disambiguation-render-test", "repo": "tcg-vault", "skip_flags": [], "classification": "ci", "duration_s": 600, "outcome": "pr-open"}

.convoys/scanner-disambiguation-render-test.md

@@ -0,0 +1,70 @@
+---
+slug: scanner-disambiguation-render-test
+status: shipping
+opened: 2026-06-13
+owner: rstillw
+related:
+  - PR #144 (`31da384`) — the runtime `useFocusTrap is not defined` ReferenceError this test locks in against
+  - `enable-no-undef-eslint-rule` convoy — sibling that closes the same bug class at lint time
+---
+
+# scanner-disambiguation-render-test
+
+## Problem
+
+PR #144 shipped a runtime `ReferenceError: useFocusTrap is not defined` to production because `ScanDisambiguationDialog` called a hook it never imported. The sibling `enable-no-undef-eslint-rule` convoy closes that bug class at lint time. This convoy locks the regression at **render time** as well, so even if someone disables the lint rule (or a future rule-set change drops it), the same bug would still fail CI.
+
+## Why vitest + jsdom instead of Playwright smoke
+
+The task was originally queued as "scanner-disambiguation-smoke-test." Re-scoped because:
+
+| Path | Catches PR #144 | Setup cost | Run time |
+|---|---|---|---|
+| Playwright smoke | ✓ if the disambiguation modal mounts during the smoke run | High — need auth bypass, a real way to enter the disambiguation state (multiple-candidate scan), and a stable fixture image | ~10s + browser overhead |
+| Vitest + @testing-library/react | ✓ directly — the render-throw is caught by the test | Low — fixture is plain JS, props are explicit | <100ms |
+
+A vitest render test catches the **exact** same bug class (`ReferenceError` during component render) at 1/100th the cost, and matches the existing `test/components/*.test.js` pattern (`Modal.test.js`, `ScannedCardItem.test.js`, etc.). A Playwright smoke test of the disambiguation flow could be added later as an integration-coverage layer but is **not** the right tool for regression-locking THIS specific bug class.
+
+The deferred Playwright disambiguation smoke test is queued separately (see § Follow-ups).
+
+## What ships
+
+`test/components/ScanDisambiguationDialog.test.js` — 8 tests:
+
+1. **`renders without crashing when given a disambiguation (PR #144 regression-lock)`** — the cheapest, most direct regression-lock. If `useFocusTrap` (or any other imported identifier) is missing, `render()` throws and this assertion fails. Comment in the test calls this out by name.
+2. `returns null when disambiguation prop is falsy` — the early-return branch
+3. `renders dialog with the correct ARIA shape` — `role="dialog"`, `aria-modal`, `aria-labelledby`
+4. `renders one button per candidate with accessible labels`
+5. `calls onPick with the selected candidate`
+6. `renders the vision hint when one is provided` — exercises the optional `disambiguation.visionHint` branch
+7. `disables the "send for review" button while submitting and shows in-flight copy`
+8. `calls onCancel when the Cancel button is clicked`
+
+## Verification
+
+Mutation test executed locally: commented out the `useFocusTrap` import → all 8 tests fail with the same `ReferenceError` shape that hit prod in PR #144. Restored the import → all 8 pass. Full suite: 26 files / 131 tests pass (up from 25 / 123 pre-PR).
+
+## Decisions
+
+- **D1.** Cover the early-return branch explicitly even though it's a one-liner. Cost is negligible (~2 lines) and it locks the `if (!disambiguation) return null;` semantics — a future refactor that returns a placeholder instead would intentionally break this test and force a review of the contract change.
+- **D2.** Use `fireEvent` for click handlers, not `userEvent`. `userEvent` is more realistic but adds a dependency (`@testing-library/user-event`) for marginal gain on these simple button-click assertions. Matches existing test pattern in `Modal.test.js`.
+- **D3.** Do NOT mock `useFocusTrap`. The hook is real and runs against jsdom. Reasoning: the original PR #144 bug was that the hook was *missing*; mocking would mask exactly the kind of failure this test is designed to catch.
+
+## Follow-ups (queued, not in this PR)
+
+- `add-component-render-smoke-pattern` — sweep the other components that conditionally mount (`UploadImageModal`, `CollectionEditModal`, `CollectionDeleteModal`, `ShareModal`, `CollaboratorFacepile`'s expanded view, etc.) and add a minimal "renders without crashing with realistic props" test to each. Same regression-lock value, one PR per ~5 components.
+- `scanner-disambiguation-playwright-smoke` — add a Playwright smoke test that exercises the full scan → ambiguous-match → pick-candidate flow against a deployed preview. Higher value as integration-layer coverage, but blocked on (a) a stable test fixture image that consistently produces multiple-candidate OCR matches and (b) auth bypass for the scanner route. Deferred.
+
+## Acceptance
+
+- [x] `test/components/ScanDisambiguationDialog.test.js` exists with the 8 listed assertions
+- [x] Mutation test confirmed all 8 tests fail when the `useFocusTrap` import is removed
+- [x] Full vitest suite passes (26 files / 131 tests)
+- [ ] CI on the PR green
+
+## Non-goals
+
+- Adding `@testing-library/user-event` (use `fireEvent` to match existing pattern)
+- Adding a Playwright smoke for this flow (queued separately)
+- Adding render tests for other modal components (queued separately)
+- Refactoring the component itself (it's already well-scoped post-PR #144 fix)

test/components/ScanDisambiguationDialog.test.js

@@ -0,0 +1,169 @@
+// @vitest-environment jsdom
+//
+// Regression lock for PR #144 (`31da384`, 2026-06-13) — `useFocusTrap`
+// was called by this component but never imported, shipping a runtime
+// `ReferenceError: useFocusTrap is not defined` to production. The
+// `enable-no-undef-eslint-rule` convoy closes that bug class at lint
+// time; this file additionally exercises the component at render time
+// so the regression is locked in BOTH static-analysis AND
+// dynamic-execution paths. A future identical bug (call a hook without
+// importing it) would fail this test even if someone disabled the
+// lint rule.
+//
+// The "renders without crashing" test is intentionally the FIRST
+// assertion — it's the cheapest failure mode and most directly tied
+// to the original PR #144 regression.
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import { render, screen, fireEvent, cleanup } from '@testing-library/react';
+import ScanDisambiguationDialog from '../../components/ScanDisambiguationDialog';
+
+const baseFixture = {
+  message: 'Multiple matches found. Pick one.',
+  candidates: [
+    {
+      id: 'card-1',
+      name: 'Lightning Bolt',
+      set_name: 'Alpha',
+      set_code: 'LEA',
+      card_number: '161',
+      image_url: 'https://cards.scryfall.io/normal/front/1/1.jpg',
+    },
+    {
+      id: 'card-2',
+      name: 'Lightning Bolt',
+      set_name: 'Beta',
+      set_code: 'LEB',
+      card_number: '161',
+      // intentionally no image — exercises the placeholder branch
+    },
+  ],
+};
+
+const noop = () => {};
+
+describe('ScanDisambiguationDialog', () => {
+  afterEach(() => cleanup());
+
+  // ── Regression-lock for PR #144 ─────────────────────────────────────
+  // If `useFocusTrap` (or any other imported identifier) is missing,
+  // React's render throws `ReferenceError` from inside the function
+  // component, and this assertion fails. Do NOT relax this test to
+  // accept thrown errors — that's the entire point.
+  it('renders without crashing when given a disambiguation (PR #144 regression-lock)', () => {
+    expect(() => {
+      render(
+        <ScanDisambiguationDialog
+          disambiguation={baseFixture}
+          submittingReview={false}
+          onPick={noop}
+          onNotInCatalog={noop}
+          onCancel={noop}
+        />
+      );
+    }).not.toThrow();
+  });
+
+  it('returns null when disambiguation prop is falsy', () => {
+    const { container } = render(
+      <ScanDisambiguationDialog
+        disambiguation={null}
+        submittingReview={false}
+        onPick={noop}
+        onNotInCatalog={noop}
+        onCancel={noop}
+      />
+    );
+    expect(container.querySelector('[role="dialog"]')).toBeNull();
+  });
+
+  it('renders dialog with the correct ARIA shape', () => {
+    render(
+      <ScanDisambiguationDialog
+        disambiguation={baseFixture}
+        submittingReview={false}
+        onPick={noop}
+        onNotInCatalog={noop}
+        onCancel={noop}
+      />
+    );
+    const dialog = screen.getByRole('dialog');
+    expect(dialog.getAttribute('aria-modal')).toBe('true');
+    expect(dialog.getAttribute('aria-labelledby')).toBe('disambiguation-title');
+    expect(screen.getByText('Which card is this?')).toBeTruthy();
+    expect(screen.getByText(baseFixture.message)).toBeTruthy();
+  });
+
+  it('renders one button per candidate with accessible labels', () => {
+    render(
+      <ScanDisambiguationDialog
+        disambiguation={baseFixture}
+        submittingReview={false}
+        onPick={noop}
+        onNotInCatalog={noop}
+        onCancel={noop}
+      />
+    );
+    const buttons = screen.getAllByRole('button', { name: /Select Lightning Bolt/i });
+    expect(buttons).toHaveLength(2);
+    expect(buttons[0].getAttribute('aria-label')).toContain('Alpha');
+    expect(buttons[1].getAttribute('aria-label')).toContain('Beta');
+  });
+
+  it('calls onPick with the selected candidate', () => {
+    const onPick = vi.fn();
+    render(
+      <ScanDisambiguationDialog
+        disambiguation={baseFixture}
+        submittingReview={false}
+        onPick={onPick}
+        onNotInCatalog={noop}
+        onCancel={noop}
+      />
+    );
+    fireEvent.click(screen.getByRole('button', { name: /Select Lightning Bolt, Alpha/i }));
+    expect(onPick).toHaveBeenCalledTimes(1);
+    expect(onPick).toHaveBeenCalledWith(baseFixture.candidates[0]);
+  });
+
+  it('renders the vision hint when one is provided', () => {
+    render(
+      <ScanDisambiguationDialog
+        disambiguation={{ ...baseFixture, visionHint: 'LEA' }}
+        submittingReview={false}
+        onPick={noop}
+        onNotInCatalog={noop}
+        onCancel={noop}
+      />
+    );
+    expect(screen.getByText(/Vision detected set: LEA/i)).toBeTruthy();
+  });
+
+  it('disables the "send for review" button while submitting and shows in-flight copy', () => {
+    render(
+      <ScanDisambiguationDialog
+        disambiguation={baseFixture}
+        submittingReview={true}
+        onPick={noop}
+        onNotInCatalog={noop}
+        onCancel={noop}
+      />
+    );
+    const reviewBtn = screen.getByRole('button', { name: /Submitting/i });
+    expect(reviewBtn.hasAttribute('disabled')).toBe(true);
+  });
+
+  it('calls onCancel when the Cancel button is clicked', () => {
+    const onCancel = vi.fn();
+    render(
+      <ScanDisambiguationDialog
+        disambiguation={baseFixture}
+        submittingReview={false}
+        onPick={noop}
+        onNotInCatalog={noop}
+        onCancel={onCancel}
+      />
+    );
+    fireEvent.click(screen.getByRole('button', { name: /^Cancel$/i }));
+    expect(onCancel).toHaveBeenCalledTimes(1);
+  });
+});