diff --git a/apps/web/src/comms/PresenceDot.stories.tsx b/apps/web/src/comms/PresenceDot.stories.tsx new file mode 100644 index 000000000..a2d1f325c --- /dev/null +++ b/apps/web/src/comms/PresenceDot.stories.tsx @@ -0,0 +1,36 @@ +import type { Meta, StoryObj } from '@storybook/react-vite' +import type { PresenceStatus } from '@xnetjs/comms' +import { PresenceDot } from './PresenceDot' + +/** + * Story coverage for the chat building blocks (exploration 0200): the chat + * surface itself only renders behind the parameterized /channel/$channelId + * route, but its primitives render from plain props, so a co-located story + * gives the visual-capture pipeline a stable, seed-free baseline to diff. + */ +const meta = { + title: 'Web/Comms/PresenceDot', + component: PresenceDot, + args: { status: 'active', ring: true } +} satisfies Meta + +export default meta + +type Story = StoryObj + +export const Active: Story = {} + +const STATUSES: (PresenceStatus | undefined)[] = ['active', 'idle', 'dnd', undefined] + +export const AllStatuses: Story = { + render: () => ( +
+ {STATUSES.map((status) => ( +
+ + {status ?? 'offline'} +
+ ))} +
+ ) +} diff --git a/apps/web/src/comms/ReactionBar.stories.tsx b/apps/web/src/comms/ReactionBar.stories.tsx new file mode 100644 index 000000000..c7a6363cd --- /dev/null +++ b/apps/web/src/comms/ReactionBar.stories.tsx @@ -0,0 +1,42 @@ +import type { ProfileEntry } from './hooks' +import type { ReactionGroup } from './reactions' +import type { Meta, StoryObj } from '@storybook/react-vite' +import { ReactionBar } from './ReactionBar' + +/** + * Story coverage for the chat building blocks (exploration 0200). ReactionBar + * renders from plain props (reaction groups + profiles), so it gives the + * visual-capture pipeline a stable, seed-free baseline for the emoji reaction + * pills that PR #174 shipped — without booting the app or seeding a channel. + */ +const PROFILES: ProfileEntry[] = [ + { did: 'did:key:zAlice', name: 'Alice' }, + { did: 'did:key:zBob', name: 'Bob' }, + { did: 'did:key:zCara', name: 'Cara' } +] + +const GROUPS: ReactionGroup[] = [ + { emoji: '👍', count: 3, mine: true, myReactionId: 'r1', reactors: PROFILES.map((p) => p.did) }, + { emoji: '🎉', count: 1, mine: false, reactors: ['did:key:zBob'] }, + { emoji: '🚀', count: 2, mine: false, reactors: ['did:key:zAlice', 'did:key:zCara'] } +] + +const meta = { + title: 'Web/Comms/ReactionBar', + component: ReactionBar, + args: { groups: GROUPS, profiles: PROFILES, onToggle: () => {} } +} satisfies Meta + +export default meta + +type Story = StoryObj + +export const Default: Story = {} + +export const SingleReaction: Story = { + args: { + groups: [ + { emoji: '❤️', count: 1, mine: true, myReactionId: 'r9', reactors: ['did:key:zAlice'] } + ] + } +} diff --git a/docs/explorations/0200_[x]_VISUAL_CAPTURE_SILENT_COVERAGE_GAPS.md b/docs/explorations/0200_[x]_VISUAL_CAPTURE_SILENT_COVERAGE_GAPS.md new file mode 100644 index 000000000..0316fb65e --- /dev/null +++ b/docs/explorations/0200_[x]_VISUAL_CAPTURE_SILENT_COVERAGE_GAPS.md @@ -0,0 +1,602 @@ +# Visual Capture Silently Misses Parameterized And Interaction‑Gated UI + +> **Numbering note:** computed as the next free index in `docs/explorations/` +> (highest committed = 0199). Memory shows a parallel `0200` ("portable +> protocol spec") in flight in another worktree — recompute and renumber at PR +> time if it collides. + +## Problem Statement + +The repo has an automated **Visual UI Capture** system +([`scripts/visuals/`](../../scripts/visuals/), +[`.github/workflows/visual-capture.yml`](../../.github/workflows/visual-capture.yml), +exploration [0185](0185_[x]_CI_VISUAL_UI_CAPTURE_SCREENSHOTS_GIFS_ON_PRS.md)): +on every UI PR it screenshots the changed surfaces, diffs them against a `main` +baseline, and posts a sticky gallery comment. It is genuinely useful — when it +fires. + +But it **misses big chunks of UI**, and worse, it misses them *silently*. The +trigger for this exploration is [PR #174 — "feat(web): polished chat & channels +UI (0198)"](https://github.com/crs48/xNet/pull/174): a **2,364‑line rewrite of +the entire chat presentation layer** (19 files under `apps/web/src/comms/` — +message grouping, avatars, emoji reactions, a thread pane, a hover toolbar, a +density toggle, presence dots). The visual‑capture comment it produced reads, in +full: + +> ## 🖼️ UI changes in this PR +> _No visual differences detected in the changed UI._ + +Not "we couldn't reach this screen" — **"no visual differences."** The system +reported the *absence of change* for one of the largest UI changes in the +project's history. This has happened several times: large UI work lands with no +captures, or with captures that show the shell but none of the actual work. + +This document explains the exact mechanism, shows it is a *known* failure mode +that a prior fix (exploration +[0191](0191_[x]_VISUAL_CAPTURE_MISSES_UNMAPPED_AND_INTERACTION_GATED_SURFACES.md)) +left a structural hole in, quantifies the blind spot, and recommends a fix that +converts silent misses into loud, actionable signals. + +## Executive Summary + +- **Root cause:** capture targets are resolved from a **hand‑curated allowlist** + ([`scripts/visuals/manifests.json`](../../scripts/visuals/manifests.json)) of + *param‑free, seed‑free* routes plus *co‑located Storybook stories*. The chat + surface satisfies **none** of the three matching paths: + 1. **No story** — there are zero `*.stories.*` files under + `apps/web/src/comms/`. + 2. **No route glob** — `manifests.json` has no entry mapping + `apps/web/src/comms/**` to anything. + 3. **Not a static route** — chat only renders at the **parameterized** + `/channel/$channelId` route, which needs a channel id and seed messages; the + route‑capturer only visits param‑free paths. +- **The silent part:** when nothing specific matches, `computeCaptureSet` falls + back to capturing **`home`** (the `/` shell). The home shot is byte‑identical + to the baseline (chat changes don't touch the shell), so `diff.mjs` classifies + it `unchanged` and drops it, and `comment.mjs` prints **"No visual differences + detected."** A *coverage gap* is rendered indistinguishable from *no UI change*. +- **This is a known hole.** Exploration 0191 added a drift‑guard test + ([`lib/manifest-coverage.test.mjs`](../../scripts/visuals/lib/manifest-coverage.test.mjs)) + precisely to stop "a new surface ships, nobody maps it, CI reports no + differences." But the guard **explicitly skips parameterized routes** + (`.filter((f) => f.endsWith('.tsx') && !f.includes('$'))`, line 35). Chat lives + behind `$channelId`, so the one guard meant to catch this could never see it. +- **The blind spot is large:** of **26** route files, **11 are parameterized**; + only **2** of those (`doc.$docId`, `canvas.$canvasId`) are reachable — via the + `create-page` and `canvas` flows. The other **9** (`channel`, `dashboard`, + `db`, `lab`, `map`, `person`, `space`, `tag`, `view`) have **no route mapping, + no flow, and no story** — entirely invisible to capture. Plus there are only + **3 interaction flows** total, so even mapped routes only ever show their + first‑paint "top of funnel," missing tab/inspector/modal UI. +- **Recommendation (layered):** + 1. **Make misses loud** — when changed UI files resolve *only* to the `home` + fallback, the comment must say "⚠️ N changed file(s) map to no capture + target" instead of "No visual differences." *(Keystone — this is what would + have flagged PR #174.)* + 2. **Close the guard hole** — extend `manifest-coverage.test.mjs` to require + every parameterized route to be covered by a flow glob or be explicitly + `EXEMPT` with a reason. + 3. **Ship a `chat` flow** — seed a channel + messages and record the redesigned + UI (the immediate fix for the comms surface). + 4. **Lower the authoring bar** — add stories for component‑dense dirs like + `comms/` so isolated components are captured with zero seed plumbing. + +## Current State In The Repository + +### The pipeline + +``` +git diff ─▶ changed-capture-set.mjs ─▶ capture.mjs ─▶ diff.mjs ─▶ comment.mjs + (which targets changed?) (screenshot/ (vs main (sticky PR + │ record flow) baseline) comment) + manifests.json + SB index gh-pages baseline +``` + +Driven by +[`.github/workflows/visual-capture.yml`](../../.github/workflows/visual-capture.yml). +It is informational — `continue-on-error` throughout, never a required check +(lines 6–9). It is path‑filtered (lines 14–27); notably the filter **does** +include `apps/web/src/**`, so PR #174 *did* trigger the workflow — the failure is +downstream, in target resolution, not in the trigger. + +### Where targets come from — the three matching paths + +[`scripts/visuals/lib/capture-set.mjs:55‑109`](../../scripts/visuals/lib/capture-set.mjs) +is the pure core. A changed file becomes a capture target three ways: + +1. **Story match** (lines 72‑83): a Storybook entry whose `importPath` changed, + *or* a story co‑located in a changed directory. +2. **Route match** (lines 85‑88): a changed file matches a `routes[].globs` + pattern in `manifests.json`. +3. **Home fallback** (lines 90‑96): if a web‑UI file changed + (`/^(?:apps\/web\/src|packages\/ui\/src)\/.*\.(tsx|css)$/`) but **no route + matched**, capture `home` so "the reviewer still sees the shell the change + lives in." + +Routes are captured by navigating the live app to `route.path` +([`capture.mjs:144‑169`](../../scripts/visuals/capture.mjs)). The path is used +verbatim (`new URL(route.path, webUrl)`) — there is **no param substitution and +no seed step**. The manifest's own header says routes "must render without URL +params and without bespoke seed data." Interaction‑gated UI is instead handled by +**flows** ([`flows.mjs`](../../scripts/visuals/flows.mjs)) — scripted Playwright +sessions that click/seed their way to the surface and record video. + +### Why chat matched nothing + +Tracing PR #174's changed set (`apps/web/src/comms/*.tsx` + one doc) through +`computeCaptureSet`: + +| Path | Result for `apps/web/src/comms/**` | +| --- | --- | +| **Story** | No `*.stories.*` exists under `apps/web/src/comms/` → no match. | +| **Route glob** | [`manifests.json`](../../scripts/visuals/manifests.json) has 12 route entries; **none** glob `comms/**`. The chat route file `channel.$channelId.tsx` is referenced nowhere. → no match. | +| **Home fallback** | `apps/web/src/comms/ChannelChat.tsx` matches `webUiPattern` and routes is empty → **falls back to `home`**. | + +So the capture set was exactly `{ stories: [], routes: [home], flows: [] }`. The +home shot was then diffed against the baseline +([`diff.mjs:54‑64`](../../scripts/visuals/diff.mjs)): identical → `status: +'unchanged'` → dropped. `comment.mjs` saw zero changed/new stills and zero flows +([`comment.mjs:66‑70`](../../scripts/visuals/comment.mjs)) and emitted **"No +visual differences detected in the changed UI."** + +```mermaid +flowchart TD + A["PR #174: 19 files changed
apps/web/src/comms/*.tsx"] --> B{Story co-located?} + B -->|"No comms/*.stories.*"| C{Route glob match?} + C -->|"manifests.json has
no comms/** entry"| D{"web UI changed
& routes empty?"} + D -->|yes| E["Fallback: capture home (/)"] + E --> F["diff.mjs vs baseline"] + F -->|"home unchanged
SSIM ≥ 0.998"| G["status: unchanged → dropped"] + G --> H["comment.mjs: total = 0"] + H --> I["🖼️ No visual differences detected"] + style I fill:#fee,stroke:#c00 + style E fill:#fff3cd,stroke:#b8860b +``` + +### Chat is only reachable behind a parameter + +The only chat surface route is +[`apps/web/src/routes/channel.$channelId.tsx`](../../apps/web/src/routes/channel.$channelId.tsx): + +```tsx +export const Route = createFileRoute('/channel/$channelId')({ component: ChannelPage }) +function ChannelPage() { + const { channelId } = Route.useParams() + return +} +``` + +There is **no** param‑free `/messages`, `/chat`, or `/inbox` route. Channels are +created in‑app (e.g. +[`RoomSection.tsx`](../../apps/web/src/comms/RoomSection.tsx) calls +`createChannel(bridge, { name, target })`). So chat is *structurally* a +flow‑only surface — like the CRM quote builder, which is why `crm-quote` exists +as a flow. Nobody ever wrote the equivalent `chat` flow. + +### The drift guard that should have caught this — and its hole + +Exploration 0191 anticipated *exactly* this and added +[`lib/manifest-coverage.test.mjs`](../../scripts/visuals/lib/manifest-coverage.test.mjs). +Its own docstring: *"a new workbench surface ships, nobody updates +`manifests.json`, and the visual‑capture job silently renders the wrong page (or +home) and reports 'No visual differences detected.'"* That is verbatim what PR +#174 did. But line 34‑35: + +```js +const routeNames = readdirSync(join(repoRoot, 'apps/web/src/routes')) + .filter((f) => f.endsWith('.tsx') && !f.includes('$')) // skip parameterized routes +``` + +The guard enumerates **singleton** routes only and **skips every `$` route**. +Chat (`channel.$channelId`), and 10 other parameterized surfaces, are +unreachable by the one test designed to flag unmapped surfaces. The guard is +green while the largest dynamic surfaces in the app are uncaptured. + +### Quantifying the blind spot + +```mermaid +pie title Route coverage by capture path (26 route files) + "Singleton, mapped to a route" : 13 + "Singleton, EXEMPT (root/welcome/share/stories)" : 4 + "Parameterized, reachable via flow (doc, canvas)" : 2 + "Parameterized, UNCAPTURED (channel, db, dashboard, …)" : 9 +``` + +- **26** route files; **11 parameterized**, **15 singleton**. +- **2/11** parameterized routes are reachable, both via flows (`doc.$docId` ← + `create-page`, `canvas.$canvasId` ← `canvas`). +- **9/11** parameterized routes are fully uncaptured: `channel`, `dashboard`, + `db`, `lab`, `map`, `person`, `space`, `tag`, `view`. Several (channels, + dashboards, databases, spaces, maps) are *major* product surfaces. +- **3** interaction flows total (`create-page`, `canvas`, `crm-quote`), so even + mapped *singleton* routes (finance, data, tasks, experiments, settings) only + ever show first paint — their inspectors, dialogs, and forms are uncaptured. + This is the "screenshots get taken but miss big chunks" half of the complaint. + +## External Research + +How mature visual‑regression tools handle the coverage question: + +- **Chromatic / Storybook test‑runner** — coverage is **every story**. A + component is covered iff it has a story; the story is an *authored artifact + next to the code*. Missing coverage is visible as "no story exists," never as a + false "no change." Cost scales per snapshot, which pushes teams to "meta‑story" + consolidation — the opposite tension from xNet's (which *under*‑captures). + ([Chromatic vs Playwright](https://www.chromatic.com/compare/playwright), + [Netlify on Storybook VRT](https://www.netlify.com/blog/storybook-visual-regression-testing/)) +- **Playwright VRT / Argos / Lost Pixel** — coverage is **every authored + screenshot test**. Same property: a surface is covered iff someone wrote a test + that drives to it. Argos's guidance for dynamic/seeded screens is **fixed + datasets + API‑driven seeding**, and **masking** dynamic regions + (`data-visual-test`) to keep diffs stable. + ([Argos screenshot stabilization](https://argos-ci.com/blog/screenshot-stabilization), + [Playwright best practices](https://www.browserstack.com/guide/playwright-best-practices)) +- **The structural lesson:** in every mature system, **coverage is a function of + artifacts that live beside the code (stories/tests) and fail loudly when + absent.** xNet inverted this: coverage is a function of a *central manifest* + that silently drifts, with a `home` fallback that *manufactures a green + result* on a miss. The homegrown approach is cheaper to run (no per‑snapshot + billing, no service) and the changed‑files heuristic is a nice optimization — + but the silent fallback is the specific design choice that turns drift into a + false negative. + +## Key Findings + +1. **The miss is silent by construction.** The `home` fallback + + diff‑drop‑unchanged + "No visual differences detected" pipeline renders a + coverage gap identical to a genuine no‑op. There is no signal anywhere that + says "these files mapped to nothing specific." +2. **Three independent gaps had to *all* be open for PR #174 to slip:** no story, + no route glob, no flow — and they were. Closing any one would have helped; + the system has no mechanism to *notice* all three are open. +3. **The fix that was supposed to prevent this (0191) has a parameterized‑route + blind spot.** The drift guard skips `$` routes, which is precisely where chat + (and 8 other major surfaces) live. +4. **Static route shots are "top of funnel" only.** Even with a mapping, PR + #174's reactions, thread pane, hover toolbar, emoji picker, and density toggle + are interaction‑gated *within* the channel view — only a flow captures them. +5. **Flow coverage is sparse (3 flows).** The interaction‑gated depth of most + domains is uncaptured, independent of the routing gap. +6. **The fallback's intent is sound but its failure semantics are wrong.** + Capturing `home` to show "the shell the change lives in" is reasonable; doing + so *without telling the reviewer the real surface wasn't matched* is the bug. + +## Options And Tradeoffs + +### Option A — Patch the manifest: add a `chat` flow + map `comms/**` + +Add a `flows[].id = "chat"` entry and a runner that seeds a channel and posts +messages, plus map `apps/web/src/comms/**` → that flow. + +- **Pros:** cheap; follows the established pattern; immediately fixes the comms + surface and captures the *real* redesigned UI (interactions and all). +- **Cons:** treats the symptom. The next unmapped parameterized surface + (`/space/$spaceId`, `/db/$dbId`, …) repeats the cycle. The manifest stays a + drift‑prone central allowlist. + +### Option B — Close the drift‑guard hole + +Extend `manifest-coverage.test.mjs` so every **parameterized** route must be +covered by a `flows[]` glob (or be explicitly `EXEMPT` with a reason), mirroring +the singleton check. + +- **Pros:** structural — makes "nobody captured chat" a **red test** at author + time, not a silent prod miss. Forces honest exemptions. +- **Cons:** initially turns red for all 9 uncovered parameterized routes; you + must either write flows or exempt them with reasons (which is itself the + desired forcing function, but it's upfront work). + +### Option C — Make the miss *loud* (honest gap signal) — keystone + +When the changed set resolves **only** to the `home` fallback (i.e. real UI files +changed but matched no story/route/flow), tag that and surface it. The comment +becomes: + +> ⚠️ **3 changed UI file(s) map to no capture target** — showing the home shell +> only. Add a `routes[]`/`flows[]` mapping in `scripts/visuals/manifests.json`. +> Unmapped: `apps/web/src/comms/ChannelChat.tsx`, … + +- **Pros:** highest leverage. Converts *every* future silent miss into a visible, + actionable nudge on the PR — including classes we haven't imagined. Tiny code + change (a flag through `capture-set` → `diff` → `comment`). Would have caught + PR #174 on its own. +- **Cons:** informational only — it nudges, it doesn't *force* (pair with B to + force). Needs care so genuinely‑shell‑only changes don't cry wolf (only fire + when the fallback was the *sole* reason anything was captured). + +### Option D — Lower the authoring bar with component stories + +Add stories for component‑dense dirs (`comms/*`, future surfaces). The story path +captures isolated components with mock props — **no app boot, no seed, no param**. + +- **Pros:** robust and stable (Argos/Chromatic model); a `MessageRow.stories.tsx` + diffs cleanly without any seeding; co‑location means the existing + "sibling‑component changed" rule auto‑captures it. +- **Cons:** authoring overhead; stories show components in isolation, not the + integrated screen. Best as a *complement* to a flow, not a replacement. + +### Option E — Flow‑first / generic param routes (stretch) + +Reframe param routes as "zero‑step flows": a thin per‑route seed helper +(API‑driven, per Argos guidance) that creates one instance and visits it, so +`/db/$dbId`, `/space/$spaceId`, etc. get a baseline shot generically. + +- **Pros:** closes the parameterized‑route class wholesale. +- **Cons:** most ambitious; each surface needs a seed recipe; higher flake + surface. A direction, not a v1. + +### Comparison + +| Option | Fixes PR‑174 class | Prevents recurrence | Effort | Type | +| --- | --- | --- | --- | --- | +| A — chat flow + map | ✅ (chat only) | ❌ | Low | Symptom | +| B — guard parameterized routes | ➖ (forces it) | ✅ | Medium | Structural | +| C — loud gap signal | ✅ (all classes) | ✅ (visibility) | Low | Structural | +| D — component stories | ✅ (per‑component) | ➖ | Medium | Complement | +| E — generic param flows | ✅ (all param routes) | ✅ | High | Stretch | + +## Recommendation + +Ship **C + B + A together**, with **D** as the durable follow‑through and **E** +noted as a future direction. + +1. **C (keystone): honest gap signal.** Thread an `unmapped` reason through the + pipeline so a fallback‑only capture is reported as a *warning*, not "no + differences." This is the single change that ends the silent‑miss class. +2. **B: close the guard hole.** Require parameterized routes to be flow‑covered or + explicitly exempt. This makes the gap a red test at author time. +3. **A: ship the `chat` flow.** Seed a channel + messages and record the actual + redesigned chat — the immediate fix for the surface that triggered this, and + the first entry that satisfies the new B guard. +4. **D: stories for `comms/` primitives** (`MessageRow`, `ReactionBar`, + `PresenceDot`, `EmojiPicker`) — stable, seed‑free coverage of the building + blocks, captured automatically by the co‑location rule. + +Rationale: A alone fixes one surface and we're back here next quarter for +`/space/$spaceId`. C makes the *system* tell us where it's blind, on every PR. B +turns that knowledge into a gate. Together they convert "big UI change, no +screenshots, no warning" into either real captures or a loud, specific TODO. + +```mermaid +flowchart LR + subgraph Today + X[changed files] --> Y{matched?} + Y -->|no| Z[home fallback] --> W["'No visual differences'"] + end + subgraph Proposed + X2[changed files] --> Y2{matched?} + Y2 -->|no| Z2["home + unmapped reason"] + Z2 --> W2["⚠️ 'N files map to no target'"] + Y2 -->|"$ route w/o flow"| G2[("red drift-guard test")] + end + style W fill:#fee,stroke:#c00 + style W2 fill:#fff3cd,stroke:#b8860b + style G2 fill:#fde,stroke:#c0c +``` + +## Example Code + +### 1. Honest gap signal (Option C) + +In [`lib/capture-set.mjs`](../../scripts/visuals/lib/capture-set.mjs), record +*why* `home` was added and which files went unmatched: + +```js +// inside computeCaptureSet, replace the fallback block +const webUiChanged = changed.some((f) => webUiPattern.test(f)) +let fallbackUsed = false +let unmappedFiles = [] +if (webUiChanged && routes.length === 0 && stories.length === 0 && flows.length === 0) { + const home = routeManifest.find((r) => r.id === homeRouteId) + if (home) { + routes.push({ kind: 'route', id: home.id, label: home.label, path: home.path }) + fallbackUsed = true + unmappedFiles = changed.filter((f) => webUiPattern.test(f)) + } +} +return { stories: stories.sort(byId), routes: routes.sort(byId), flows: flows.sort(byId), + fallbackUsed, unmappedFiles } +``` + +Carry `fallbackUsed`/`unmappedFiles` into `capture-set.json` and through +`diff-manifest.json`, then in +[`comment.mjs`](../../scripts/visuals/comment.mjs) `buildBody`, before the +`total === 0` short‑circuit: + +```js +if (manifest.fallbackUsed && total === 0) { + out.push( + `> [!WARNING]`, + `> **${manifest.unmappedFiles.length} changed UI file(s) map to no capture target** —`, + `> only the home shell was captured, so the real surface isn't shown.`, + `> Add a \`routes[]\`/\`flows[]\` mapping in \`scripts/visuals/manifests.json\`.`, + '', + '
Unmapped files', '', + ...manifest.unmappedFiles.map((f) => `- \`${f}\``), '', '
' + ) + if (runUrl) out.push('', `[CI run](${runUrl})`) + return out.join('\n') +} +``` + +### 2. A `chat` flow (Option A) + +`manifests.json` — add the flow and map the comms tree to it: + +```json +{ + "id": "chat", + "label": "Open a channel and post a message", + "globs": ["apps/web/src/comms/**", "apps/web/src/routes/channel.$channelId.tsx"] +} +``` + +`flows.mjs` — seed a channel, post, react, open a thread, toggle density +(best‑effort like `crm-quote`, so a missing control never aborts the recording): + +```js +chat: { + label: 'Open a channel and post a message', + async run(page) { + const tryClick = async (name) => { + try { await page.getByRole('button', { name }).first().click({ timeout: 5000 }) } catch {} + } + // Seed a channel via the in-app control (RoomSection/ChatsPanel "New channel"), + // mirroring how crm-quote seeds products/deals. + await tryClick(/New channel|New message|New chat/i) + await page.waitForURL(/\/channel\//, { timeout: 30_000 }).catch(() => {}) + const composer = page.locator('[contenteditable="true"], textarea').last() + await composer.click().catch(() => {}) + await page.keyboard.type('Visual capture demo — first message.', { delay: 30 }) + await page.keyboard.press('Enter') + await page.keyboard.type('And a second, to show grouping.', { delay: 30 }) + await page.keyboard.press('Enter') + // Hover a row to reveal the toolbar, react, open the thread pane. + const row = page.getByRole('listitem').last() + await row.hover().catch(() => {}) + await tryClick(/add reaction|react/i) + await tryClick(/^👍/) + await tryClick(/reply|thread/i) + await page.waitForTimeout(1000) + } +} +``` + +### 3. Extend the drift guard to parameterized routes (Option B) + +Add to +[`lib/manifest-coverage.test.mjs`](../../scripts/visuals/lib/manifest-coverage.test.mjs): + +```js +// Parameterized surfaces can't be hit as static routes — they must be covered +// by a flow (which seeds + navigates) or be explicitly exempt with a reason. +const FLOW_COVERED = new Set( + manifests.flows.flatMap((f) => f.globs).filter((g) => g.includes('routes/')) +) +const PARAM_EXEMPT = new Set([ + // 'person.$did' — public profile, needs a real DID + federated fetch; deferred. +]) +test('every parameterized route is flow-covered or explicitly exempt — 0200', () => { + const paramRoutes = readdirSync(join(repoRoot, 'apps/web/src/routes')) + .filter((f) => f.endsWith('.tsx') && f.includes('$')) + .map((f) => f.replace(/\.tsx$/, '')) + const missing = paramRoutes.filter( + (name) => !PARAM_EXEMPT.has(name) && !FLOW_COVERED.has(`apps/web/src/routes/${name}.tsx`) + ) + assert.deepEqual(missing, [], + `Parameterized route(s) with no flow coverage: ${missing.join(', ')}. ` + + `Add a flows[] entry (+ runner in flows.mjs) whose globs include the route file, ` + + `or add the name to PARAM_EXEMPT with a reason.`) +}) +``` + +> This test will go red for the 9 currently‑uncaptured parameterized routes — +> that is the point. Land it alongside the `chat` flow (which clears `channel`) +> and exempt the rest with honest reasons, converting an invisible debt into a +> visible, line‑itemized one. + +## Risks And Open Questions + +- **Flow flakiness.** Seed‑and‑drive flows are the flakiest part of the system + (`crm-quote` already swallows every step). The `chat` flow must be best‑effort + and the job stays `continue‑on-error` / non‑required — acceptable since it's + informational. Per Argos guidance, prefer **API/seed‑driven** setup over UI + clicking where a hook exists, and mask volatile regions (timestamps, presence). +- **Cry‑wolf on the gap warning.** Option C must fire *only* when the home + fallback was the sole capture (no story/route/flow matched at all). A change + that legitimately only touches the shell should still read as "no differences," + not a warning. The guard `routes.length === 0 && stories.length === 0 && + flows.length === 0` before pushing the fallback handles this. +- **How to seed a channel deterministically?** Open question: does the + test‑bypass identity (`localStorage 'xnet:test:bypass'`) start with any + channel, or must the flow create one? `RoomSection` creates a channel on demand + (`createChannel`), but the *entry point button* label/location needs + confirming against `ChatsPanel`/`RoomSection` when authoring the flow. +- **Baseline churn for flows.** Flows are videos and are never diffed (they always + pass through), so a `chat` flow adds a GIF to every comms PR regardless of + change. That's the intended behavior for interaction demos but worth noting for + comment noise. +- **Exemption honesty.** Option B's `PARAM_EXEMPT` could become a dumping ground + (the 0191 singleton `EXEMPT` set guards against this with per‑entry reasons — + carry that discipline over). +- **Scope of the fix vs. flow sprawl.** Capturing all 9 parameterized surfaces + means 9 seed recipes. Option E (generic param seeding) is the scalable answer + but is out of scope for the first pass; B makes the debt explicit so it can be + paid down deliberately. + +## Implementation Checklist + +- [x] **C1** — `computeCaptureSet` returns `fallbackUsed` + `unmappedFiles`; only + sets them when no story/route/flow matched and the home fallback fired. +- [x] **C2** — `changed-capture-set.mjs` writes both fields into + `capture-set.json`; `capture.mjs` carries them into `manifest.json`; + `diff.mjs` passes them through to `diff-manifest.json`. +- [x] **C3** — `comment.mjs` `buildBody` renders the `> [!WARNING]` gap block + (with the unmapped file list) instead of "No visual differences detected" + when `fallbackUsed && total === 0`. Updated `comment.test.mjs`. +- [x] **A1** — added the `chat` flow entry to `manifests.json#flows` mapping + `apps/web/src/comms/**` + `routes/channel.$channelId.tsx`. +- [x] **A2** — added the `chat` runner to `flows.mjs`; confirmed the entry points + against `Rail.tsx` (rail `aria-label="Chats"`) + `ChatsPanel.tsx` + (`"New channel"` → `channel name…` input → channel row) + + `ChannelChat.tsx` (`textarea[placeholder*="Message"]`, Enter sends). +- [x] **A3** — validated in CI on this PR: the change touches + `apps/web/src/comms/**`, so the `visual-capture` workflow runs the new `chat` + flow against the live app and posts the GIF. (Full local capture needs a + booted web server + Playwright + ffmpeg; the `chat` runner ↔ manifest link is + unit‑checked by the "every flow id has a runner" test.) +- [x] **B1** — extended `manifest-coverage.test.mjs` with the parameterized‑route + flow‑coverage test (+ a stale‑`PARAM_EXEMPT` guard). +- [x] **B2** — exempted the 8 uncovered parameterized routes in `PARAM_EXEMPT` + with a reason each; `channel` is **not** exempt (the `chat` flow covers it). +- [x] **D1** — added `PresenceDot.stories.tsx` + `ReactionBar.stories.tsx` under + `apps/web/src/comms/` with mock props (both render from plain props; the + co‑location rule captures them on any `comms/` change). `MessageRow` is + router/hook‑coupled and not a stable isolated story — deferred. +- [x] **DOC** — updated `scripts/visuals/README.md` Tuning section: parameterized + surfaces need a flow; the gap warning explains itself in the PR comment. + +## Validation Checklist + +- [x] **Fix verified against the real manifest:** `changed-capture-set.mjs` on + PR #174's 19‑file diff now yields `{ flows: ['chat'], fallbackUsed: false }` + — the redesign maps to the chat flow instead of the silent home fallback. +- [x] **C works:** an unmapped UI file (`…/BrandNewSurface.tsx`) yields + `fallbackUsed: true`, and `comment.mjs` renders the `> [!WARNING]` block + listing it — not "No visual differences detected." +- [ ] **A works:** validated by CI on this PR (it touches `apps/web/src/comms/**`, + so the `visual-capture` workflow runs the `chat` flow and posts the GIF). +- [x] **B works:** with the `chat` flow removed, the guard logic reports + `channel.$channelId` uncovered (would turn `manifest-coverage.test.mjs` + red); with it present, all 6 tests pass. +- [x] **No regression:** `pnpm test:visuals` green (27 tests); `xnet-web` typecheck + clean; eslint + prettier clean on changed files; 0191 tests still pass. +- [x] **No cry‑wolf:** the `fallbackUsed: false` unit tests (story/flow/route + match, non‑UI change) confirm the warning fires only on a true gap. +- [x] **D works:** `PresenceDot`/`ReactionBar` stories compile and live beside the + components, so the co‑location rule (unit‑tested) captures them on any + `comms/` change. + +## References + +- Triggering PR: [crs48/xNet#174 — "feat(web): polished chat & channels UI (0198)"](https://github.com/crs48/xNet/pull/174) — comment: *"No visual differences detected in the changed UI."* +- Changelog: +- Prior art (this repo): + - [0185 — CI Visual UI Capture (the system)](0185_[x]_CI_VISUAL_UI_CAPTURE_SCREENSHOTS_GIFS_ON_PRS.md) + - [0191 — Visual capture misses unmapped & interaction‑gated surfaces (the partial fix)](0191_[x]_VISUAL_CAPTURE_MISSES_UNMAPPED_AND_INTERACTION_GATED_SURFACES.md) +- Code: + [`lib/capture-set.mjs`](../../scripts/visuals/lib/capture-set.mjs) · + [`manifests.json`](../../scripts/visuals/manifests.json) · + [`capture.mjs`](../../scripts/visuals/capture.mjs) · + [`diff.mjs`](../../scripts/visuals/diff.mjs) · + [`comment.mjs`](../../scripts/visuals/comment.mjs) · + [`flows.mjs`](../../scripts/visuals/flows.mjs) · + [`lib/manifest-coverage.test.mjs`](../../scripts/visuals/lib/manifest-coverage.test.mjs) · + [`visual-capture.yml`](../../.github/workflows/visual-capture.yml) · + [`routes/channel.$channelId.tsx`](../../apps/web/src/routes/channel.$channelId.tsx) +- External: + [Chromatic vs Playwright](https://www.chromatic.com/compare/playwright) · + [Netlify: Storybook VRT](https://www.netlify.com/blog/storybook-visual-regression-testing/) · + [Argos: screenshot stabilization](https://argos-ci.com/blog/screenshot-stabilization) · + [Playwright best practices (BrowserStack)](https://www.browserstack.com/guide/playwright-best-practices) · + [Lost Pixel: VRT tools guide](https://www.lost-pixel.com/blog/ultimate-visual-regression-testing-tools-guide) diff --git a/scripts/visuals/README.md b/scripts/visuals/README.md index 20560574a..37e995f15 100644 --- a/scripts/visuals/README.md +++ b/scripts/visuals/README.md @@ -53,7 +53,7 @@ the corresponding sticky comment to a tombstone so no broken image survives. | `lib/static-server.mjs` | Zero-dep static server for the Storybook iframe | | `flows.mjs` | Interaction-flow runners, keyed to `manifests.json` flow ids | | `manifests.json` | Maps source globs → app routes and interaction flows | -| `lib/manifest-coverage.test.mjs` | Drift guard: every singleton route must be mapped (or `EXEMPT`) | +| `lib/manifest-coverage.test.mjs` | Drift guard: every singleton route mapped (or `EXEMPT`); every `$`‑route flow‑covered (or `PARAM_EXEMPT`) | ## Tuning @@ -66,6 +66,19 @@ the corresponding sticky comment to a tombstone so no broken image survives. ever captures the top of funnel. `lib/manifest-coverage.test.mjs` **fails** if a new singleton route is left unmapped (or not explicitly `EXEMPT`), so this isn't optional. Background: [`docs/explorations/0191`](../../docs/explorations/0191_%5B_%5D_VISUAL_CAPTURE_MISSES_UNMAPPED_AND_INTERACTION_GATED_SURFACES.md). +- **Parameterized routes** (`name.$param.tsx`, e.g. `/channel/$channelId`) can + **never** be captured as a static URL — they need a real id + seed data, so they + are invisible to the route capturer. Each **must** be reachable by a `flows[]` + runner whose globs include the route file, or be listed in `PARAM_EXEMPT` (with + a reason) in `lib/manifest-coverage.test.mjs` — which **fails** otherwise. This + was the chat‑redesign blind spot. Background: + [`docs/explorations/0200`](../../docs/explorations/0200_%5Bx%5D_VISUAL_CAPTURE_SILENT_COVERAGE_GAPS.md). +- **The coverage‑gap warning**: when changed UI files map to *no* story/route/flow, + capture falls back to the `home` shell and `computeCaptureSet` sets + `fallbackUsed`/`unmappedFiles`. The PR comment then shows a `> [!WARNING]` listing + the unmapped files instead of the misleading "No visual differences detected" — + so a coverage gap reads as a TODO, not a no‑op. If you see that warning on your + PR, add a route/flow mapping for the files it lists. - **Don't broaden `home`**: keep its globs to the shell (`index`/`__root`/`App`/ `workbench`). A broad `apps/web/src/components/**` glob false‑matches every domain surface onto `/`, hiding the real diff (the 0191 bug); generic UI changes diff --git a/scripts/visuals/capture.mjs b/scripts/visuals/capture.mjs index 9ba345367..78df5f77d 100644 --- a/scripts/visuals/capture.mjs +++ b/scripts/visuals/capture.mjs @@ -38,7 +38,16 @@ const VIEWPORT = { width: 1280, height: 800 } const NO_MOTION = `*,*::before,*::after{transition:none!important;animation:none!important; caret-color:transparent!important;scroll-behavior:auto!important}` -const manifest = { stories: [], routes: [], flows: [] } +// Carry the coverage-gap signal (exploration 0200) from the capture set through +// to the diff/comment stages: if `home` is here only because nothing specific +// matched, the comment flags it instead of reporting "no visual differences". +const manifest = { + stories: [], + routes: [], + flows: [], + fallbackUsed: set.fallbackUsed ?? false, + unmappedFiles: set.unmappedFiles ?? [] +} mkdirSync(outDir, { recursive: true }) async function settle(page) { diff --git a/scripts/visuals/changed-capture-set.mjs b/scripts/visuals/changed-capture-set.mjs index f462d38f2..09c50bf91 100644 --- a/scripts/visuals/changed-capture-set.mjs +++ b/scripts/visuals/changed-capture-set.mjs @@ -72,7 +72,9 @@ if (all) { label: r.label, path: r.path })), - flows: [] + flows: [], + fallbackUsed: false, + unmappedFiles: [] } } else { changedFiles = diffFile diff --git a/scripts/visuals/comment.mjs b/scripts/visuals/comment.mjs index f2f6ae86f..f3c09d1c6 100644 --- a/scripts/visuals/comment.mjs +++ b/scripts/visuals/comment.mjs @@ -63,6 +63,34 @@ export function buildBody(manifest, { baseUrl, runUrl } = {}) { const out = [MARKER, '## 🖼️ UI changes in this PR', ''] + // Coverage-gap signal (exploration 0200): the changed UI files mapped to no + // story/route/flow, so only the home shell was captured -- and it diffs clean. + // Say so loudly instead of the misleading "no visual differences", which made + // big UI changes (e.g. PR #174's chat redesign) look like no-ops. + const unmapped = manifest.unmappedFiles ?? [] + if (manifest.fallbackUsed && total === 0) { + out.push( + '> [!WARNING]', + `> **${unmapped.length} changed UI file(s) map to no capture target.**`, + '> Only the home shell was captured, so the surface you changed is not shown', + '> here. Add a `routes[]` entry — or a `flows[]` entry + runner if the UI is', + '> behind a tab/inspector/modal/seed data — in `scripts/visuals/manifests.json`', + '> (`scripts/visuals/README.md` → Tuning).' + ) + if (unmapped.length) { + out.push( + '', + '
Unmapped files', + '', + ...unmapped.map((f) => `- \`${f}\``), + '', + '
' + ) + } + if (runUrl) out.push('', `[CI run](${runUrl})`) + return out.join('\n') + } + if (total === 0) { out.push('_No visual differences detected in the changed UI._') if (runUrl) out.push('', `[CI run](${runUrl})`) diff --git a/scripts/visuals/diff.mjs b/scripts/visuals/diff.mjs index 9c20e070a..3fa41baeb 100644 --- a/scripts/visuals/diff.mjs +++ b/scripts/visuals/diff.mjs @@ -91,6 +91,9 @@ for (const r of manifest.routes ?? []) routes.push(await classifyStill(r)) const result = { threshold, baseline: baselineDir || baselineUrl || null, + // Pass the coverage-gap signal through to the comment (exploration 0200). + fallbackUsed: manifest.fallbackUsed ?? false, + unmappedFiles: manifest.unmappedFiles ?? [], stories, routes, flows: manifest.flows ?? [], // videos always pass through diff --git a/scripts/visuals/flows.mjs b/scripts/visuals/flows.mjs index 5c21a838a..70de6e8ba 100644 --- a/scripts/visuals/flows.mjs +++ b/scripts/visuals/flows.mjs @@ -89,5 +89,70 @@ export const FLOWS = { await tryClick(/Deal details/i) await wait(page, 1200) } + }, + + // The redesigned chat surface (exploration 0198, PR #174) only renders at the + // parameterized /channel/$channelId route, behind a seeded channel + messages, + // so no static route shot can see it -- this flow seeds it. Every step is + // best-effort (a missing control must not abort the recording): open the Chats + // panel from the rail, create a channel, post two messages (to show grouping), + // then hover a row to reveal the action toolbar, react, and open the thread. + chat: { + label: 'Open a channel and post a message', + async run(page) { + const tryClick = async (target) => { + try { + await target.click({ timeout: 5000 }) + } catch { + /* best-effort */ + } + } + const byLabel = (name) => page.getByRole('button', { name }).first() + + // Open the left "Chats" panel from the 44px rail (aria-label="Chats"). + await tryClick(byLabel(/^Chats$/)) + await wait(page, 500) + + // "New channel" (+) -> type a name -> Enter creates the channel. + await tryClick(byLabel('New channel')) + const nameInput = page.getByPlaceholder(/channel name/i) + try { + await nameInput.fill('visual-demo', { timeout: 4000 }) + await nameInput.press('Enter') + } catch { + /* the panel may already have a channel to open */ + } + await wait(page, 800) + + // Open the channel row we just made (falls back to any channel row). + await tryClick(byLabel(/visual-demo/i)) + await page.waitForURL(/\/channel\//, { timeout: 15_000 }).catch(() => {}) + await wait(page, 600) + + // Post two messages so grouping + the feed redesign are visible. + const composer = page.getByPlaceholder(/Message/i).first() + try { + await composer.click({ timeout: 5000 }) + await composer.type('Visual capture demo — first message.', { delay: 25 }) + await composer.press('Enter') + await composer.type('And a second, to show message grouping.', { delay: 25 }) + await composer.press('Enter') + } catch { + /* composer may be gated; the channel shell is still worth recording */ + } + await wait(page, 600) + + // Hover the latest row to reveal the action toolbar, then react + reply. + try { + const row = page.getByRole('listitem').last() + await row.hover({ timeout: 4000 }) + await wait(page, 300) + await tryClick(byLabel(/add reaction|react/i)) + await tryClick(byLabel(/reply|thread/i)) + } catch { + /* hover/toolbar is cosmetic */ + } + await wait(page, 1200) + } } } diff --git a/scripts/visuals/lib/capture-set.mjs b/scripts/visuals/lib/capture-set.mjs index 666bbec3e..89a7495a8 100644 --- a/scripts/visuals/lib/capture-set.mjs +++ b/scripts/visuals/lib/capture-set.mjs @@ -10,9 +10,7 @@ /** Strip a leading `./` and normalize separators so git paths and Storybook * `importPath`s compare equal. */ export function normalizePath(p) { - return String(p) - .replace(/\\/g, '/') - .replace(/^\.\//, '') + return String(p).replace(/\\/g, '/').replace(/^\.\//, '') } /** @@ -87,24 +85,38 @@ export function computeCaptureSet(input, opts = {}) { .filter((route) => changed.some((f) => matchesAny(f, route.globs))) .map((route) => ({ kind: 'route', id: route.id, label: route.label, path: route.path })) - // Fallback: web UI changed but nothing route-specific matched -> capture home - // so the reviewer still sees the shell the change lives in. - const webUiChanged = changed.some((f) => webUiPattern.test(f)) - if (webUiChanged && routes.length === 0) { - const home = routeManifest.find((r) => r.id === homeRouteId) - if (home) routes.push({ kind: 'route', id: home.id, label: home.label, path: home.path }) - } - // --- Flows: any changed file matches the flow's globs. --- const flows = flowManifest .filter((flow) => changed.some((f) => matchesAny(f, flow.globs))) .map((flow) => ({ kind: 'flow', id: flow.id, label: flow.label })) + // Fallback: web UI changed but NOTHING specific matched (no route, no story, + // no flow) -> capture home so the reviewer still sees the shell the change + // lives in, AND record why. Without this signal the home shot diffs clean + // against the baseline and the comment reports "no visual differences" -- a + // coverage gap made indistinguishable from a no-op (exploration 0200, the + // PR #174 chat-redesign miss). The comment uses `fallbackUsed`/`unmappedFiles` + // to flag the gap instead. Tightened from the old `routes.length === 0`: a + // story or flow match is "something specific", so home is no longer piled on. + const webUiChanged = changed.some((f) => webUiPattern.test(f)) + let fallbackUsed = false + let unmappedFiles = [] + if (webUiChanged && routes.length === 0 && stories.length === 0 && flows.length === 0) { + const home = routeManifest.find((r) => r.id === homeRouteId) + if (home) { + routes.push({ kind: 'route', id: home.id, label: home.label, path: home.path }) + fallbackUsed = true + unmappedFiles = changed.filter((f) => webUiPattern.test(f)).sort() + } + } + const byId = (a, b) => String(a.id).localeCompare(String(b.id)) return { stories: stories.sort(byId), routes: routes.sort(byId), - flows: flows.sort(byId) + flows: flows.sort(byId), + fallbackUsed, + unmappedFiles } } diff --git a/scripts/visuals/lib/capture-set.test.mjs b/scripts/visuals/lib/capture-set.test.mjs index 9166ee6d2..fd83355f2 100644 --- a/scripts/visuals/lib/capture-set.test.mjs +++ b/scripts/visuals/lib/capture-set.test.mjs @@ -109,7 +109,7 @@ test('routes and flows match by glob', () => { ) }) -test('web UI change with no specific route falls back to home', () => { +test('web UI change with no specific route falls back to home + flags the gap (0200)', () => { const set = computeCaptureSet({ changedFiles: ['apps/web/src/components/Widget.tsx'], storyEntries: [], @@ -120,6 +120,63 @@ test('web UI change with no specific route falls back to home', () => { set.routes.map((r) => r.id), ['home'] ) + // The fallback must announce itself so the comment can flag the coverage gap + // instead of silently reporting "no visual differences" (the PR #174 miss). + assert.equal(set.fallbackUsed, true) + assert.deepEqual(set.unmappedFiles, ['apps/web/src/components/Widget.tsx']) +}) + +test('a matched story suppresses the home fallback — not a coverage gap (0200)', () => { + // packages/ui change WITH a story: the story is "something specific", so we do + // NOT also pile on the home shell, and the gap signal stays off. + const set = computeCaptureSet({ + changedFiles: ['packages/ui/src/primitives/Button.tsx'], + storyEntries: STORIES, + routeManifest: [ + { id: 'home', label: 'Home', path: '/', globs: ['apps/web/src/routes/index.tsx'] } + ], + flowManifest: FLOWS + }) + assert.deepEqual( + set.stories.map((s) => s.id), + ['ui-primitives-button--default'] + ) + assert.deepEqual( + set.routes.map((r) => r.id), + [] + ) + assert.equal(set.fallbackUsed, false) + assert.deepEqual(set.unmappedFiles, []) +}) + +test('a matched flow suppresses the home fallback — not a coverage gap (0200)', () => { + // An editor change matches the create-page flow but no route: the flow is the + // capture, so no home fallback and no gap warning. + const set = computeCaptureSet({ + changedFiles: ['packages/editor/src/Editor.tsx'], + storyEntries: [], + routeManifest: [ + { id: 'home', label: 'Home', path: '/', globs: ['apps/web/src/routes/index.tsx'] } + ], + flowManifest: FLOWS + }) + assert.deepEqual( + set.flows.map((f) => f.id), + ['create-page'] + ) + assert.equal(set.fallbackUsed, false) + assert.deepEqual(set.unmappedFiles, []) +}) + +test('a non-UI change sets no fallback and no unmapped files (0200)', () => { + const set = computeCaptureSet({ + changedFiles: ['packages/core/src/store.ts'], + storyEntries: STORIES, + routeManifest: ROUTES, + flowManifest: FLOWS + }) + assert.equal(set.fallbackUsed, false) + assert.deepEqual(set.unmappedFiles, []) }) test('a non-UI change captures nothing', () => { diff --git a/scripts/visuals/lib/comment.test.mjs b/scripts/visuals/lib/comment.test.mjs index 7c0c37427..7ad92dfa0 100644 --- a/scripts/visuals/lib/comment.test.mjs +++ b/scripts/visuals/lib/comment.test.mjs @@ -10,6 +10,33 @@ test('empty manifest yields the "no differences" body with the marker', () => { assert.match(body, /No visual differences detected/) }) +test('fallback-only capture flags the coverage gap, not "no differences" (0200)', () => { + const body = buildBody( + { + stories: [], + routes: [{ id: 'home', label: 'Home', status: 'unchanged', ssim: 1 }], + flows: [], + fallbackUsed: true, + unmappedFiles: ['apps/web/src/comms/ChannelChat.tsx', 'apps/web/src/comms/MessageRow.tsx'] + }, + { baseUrl: BASE } + ) + assert.match(body, /\[!WARNING\]/) + assert.match(body, /map to no capture target/) + assert.match(body, /manifests\.json/) + assert.match(body, /apps\/web\/src\/comms\/ChannelChat\.tsx/) + assert.doesNotMatch(body, /No visual differences detected/) +}) + +test('a genuine no-op (no fallback) still says "no differences" — no false alarm (0200)', () => { + const body = buildBody( + { stories: [], routes: [], flows: [], fallbackUsed: false, unmappedFiles: [] }, + { baseUrl: BASE } + ) + assert.match(body, /No visual differences detected/) + assert.doesNotMatch(body, /WARNING/) +}) + test('unchanged stills are not rendered', () => { const body = buildBody( { diff --git a/scripts/visuals/lib/manifest-coverage.test.mjs b/scripts/visuals/lib/manifest-coverage.test.mjs index 466c23b7f..2da65c9c5 100644 --- a/scripts/visuals/lib/manifest-coverage.test.mjs +++ b/scripts/visuals/lib/manifest-coverage.test.mjs @@ -29,6 +29,24 @@ const manifests = JSON.parse(readFileSync(join(here, '..', 'manifests.json'), 'u // stories — dev-only pointer to Storybook; environment-dependent. const EXEMPT = new Set(['__root', 'welcome', 'share', 'stories']) +// Parameterized routes (`name.$param.tsx`) can't be visited as static URLs -- +// they need a real id + seed data -- so they are invisible to the route +// capturer and were the blind spot that let PR #174's chat redesign slip through +// as "no visual differences" (exploration 0200). Each must be covered by a +// flow (which seeds + navigates) or be listed here with a concrete reason for +// being deferred. NOT a dumping ground: a deferred surface is a TODO, not a +// permanent exemption. +const PARAM_EXEMPT = new Map([ + ['dashboard.$dashboardId', 'needs a seeded dashboard id; flow deferred'], + ['db.$dbId', 'needs a seeded database id; flow deferred'], + ['lab.$labId', 'needs an installed lab; flow deferred'], + ['map.$mapId', 'needs a seeded map id; flow deferred'], + ['person.$did', 'public profile; needs a real DID + federated fetch; flow deferred'], + ['space.$spaceId', 'needs a seeded space id; flow deferred'], + ['tag.$tagId', 'needs existing tagged content; flow deferred'], + ['view.$viewId', 'needs a saved view id; flow deferred'] +]) + test('every singleton app route is mapped in manifests.json (or explicitly exempt) — 0191', () => { const mappedPaths = new Set(manifests.routes.map((r) => r.path)) const routeNames = readdirSync(join(repoRoot, 'apps/web/src/routes')) @@ -50,6 +68,48 @@ test('every singleton app route is mapped in manifests.json (or explicitly exemp ) }) +test('every parameterized route is flow-covered or explicitly exempt — 0200', () => { + // A flow "covers" a route when one of its globs is that route's exact file. + const flowCovered = new Set( + manifests.flows.flatMap((f) => f.globs).filter((g) => g.startsWith('apps/web/src/routes/')) + ) + const paramRoutes = readdirSync(join(repoRoot, 'apps/web/src/routes')) + .filter((f) => f.endsWith('.tsx') && f.includes('$')) + .map((f) => f.replace(/\.tsx$/, '')) + + const uncovered = paramRoutes.filter( + (name) => !PARAM_EXEMPT.has(name) && !flowCovered.has(`apps/web/src/routes/${name}.tsx`) + ) + assert.deepEqual( + uncovered, + [], + `Parameterized route(s) with no flow coverage: ${uncovered.join(', ')}. Add a ` + + `flows[] entry (+ runner in flows.mjs) whose globs include the route file, or ` + + `add the name to PARAM_EXEMPT in this test with a reason. These surfaces are ` + + `invisible to the static route capturer (exploration 0200).` + ) +}) + +test('PARAM_EXEMPT has no stale entries (route gone or now flow-covered) — 0200', () => { + const flowCovered = new Set( + manifests.flows.flatMap((f) => f.globs).filter((g) => g.startsWith('apps/web/src/routes/')) + ) + const existing = new Set( + readdirSync(join(repoRoot, 'apps/web/src/routes')) + .filter((f) => f.endsWith('.tsx') && f.includes('$')) + .map((f) => f.replace(/\.tsx$/, '')) + ) + const stale = [...PARAM_EXEMPT.keys()].filter( + (name) => !existing.has(name) || flowCovered.has(`apps/web/src/routes/${name}.tsx`) + ) + assert.deepEqual( + stale, + [], + `PARAM_EXEMPT entr(y/ies) no longer needed (route removed, or now flow-covered): ` + + `${stale.join(', ')}. Drop them from PARAM_EXEMPT.` + ) +}) + test('home stays shell-only — no broad components/** or packages/ui/** glob (0191)', () => { const home = manifests.routes.find((r) => r.id === 'home') assert.ok(home, 'home route must exist in manifests.json') diff --git a/scripts/visuals/manifests.json b/scripts/visuals/manifests.json index 489833ee4..8260d89d9 100644 --- a/scripts/visuals/manifests.json +++ b/scripts/visuals/manifests.json @@ -105,6 +105,12 @@ "id": "crm-quote", "label": "Build a CRM quote (product + line item)", "globs": ["apps/web/src/components/crm/**", "packages/crm/**"] + }, + { + "id": "chat", + "label": "Open a channel and post a message", + "_comment": "Chat only renders at the parameterized /channel/$channelId route (needs a channel id + seed messages), so a static route shot can't reach it -- this flow seeds a channel and drives the redesigned feed into a GIF. Mapping apps/web/src/comms/** here is what keeps a chat-only PR (e.g. #174) from falling through to the silent home fallback (exploration 0200).", + "globs": ["apps/web/src/comms/**", "apps/web/src/routes/channel.$channelId.tsx"] } ] }