test: add visual regression coverage for Modal variants#1093
Draft
TaprootFreak wants to merge 16 commits into
Draft
test: add visual regression coverage for Modal variants#1093TaprootFreak wants to merge 16 commits into
TaprootFreak wants to merge 16 commits into
Conversation
Set up Storybook 8 with the CRA preset and add stories for the Modal component (Default, Fullscreen, Dialog). A Chromatic GitHub Action publishes the Storybook on every PR and blocks merges on unreviewed visual changes. Background: PR #1048 silently flipped the Modal default from fullscreen to dialog, which broke every fullscreen modal in production until PR #1090 reintroduced an explicit variant prop. Visual regression tests catch this class of defect at PR time instead of in production.
…orybook Regenerated the lockfile without --legacy-peer-deps so it satisfies the repo's .npmrc setting (legacy-peer-deps=false). The CI's strict `npm ci` now installs cleanly. Storybook's CRA preset injects ForkTsCheckerWebpackPlugin which re-evaluates the entire app under strict TypeScript and fails on pre-existing type drift unrelated to the stories. The app build mitigates this with CI=false in scripts/build.sh; Storybook needs the same treatment. Type-checking remains the responsibility of `react-app-rewired build`.
Prevents the new workflow from blocking unrelated PR merges before the maintainer has had a chance to provision the Chromatic project and add the CHROMATIC_PROJECT_TOKEN repo secret. Once the secret is present the detect job flips on and the visual-regression check becomes mandatory.
- Drop the redundant mount-gate in the Storybook decorator; the Modal component already manages its own portal-mount state, so the wrapper state was a second redundant render pass. - Use the modern react-jsx runtime import shape (drop the unused React default import). - Add explicit type="button" to the sample buttons inside stories so they cannot accidentally submit a wrapping form in future scenarios. - Reduce the Chromatic workflow permissions to the strict minimum (contents: read). PR comments are handled by the separate Chromatic GitHub App, not the action token.
- Workflow name without em-dash to match the sibling workflows (PR CI, PR Review Bot, DEV CI/CD, PRD CI/CD). - Add workflow_dispatch trigger so maintainers can rerun Chromatic from the Actions UI without a code push — consistent with pr.yml. - Use function declarations with explicit JSX.Element return type for the story sample components, matching every other component file in src/components.
The previous full lockfile regeneration unintentionally bumped react-hook-form from 7.52.0 to 7.75.0 (the package.json caret allows the minor bump). react-hook-form 7.75 narrowed the Control<> generic typing, which broke compilation in recommendation-modal and other existing forms during `npm ci && react-app-rewired build` on CI — exactly what tighter type checks are supposed to flag. Restoring develop's lockfile and adding only the new devDeps (storybook + chromatic) keeps every existing pin intact while still satisfying the strict npm ci constraint.
Drop the external SaaS dependency. The visual regression check now runs entirely on GitHub-hosted runners: - A dedicated playwright.storybook.config.ts serves the built Storybook on http://localhost:6006 (via the existing `serve` devDep) and runs the new spec against it. - e2e/storybook/modal-visual.spec.ts renders each Modal story (Default / Fullscreen / Dialog) at both 375 px and 1280 px and pixel-diffs against PNG baselines committed alongside the spec under __snapshots__/. - .github/workflows/visual-regression.yml gates every PR on the diff. On the first run (before baselines exist) the job fails by design and uploads the captured screenshots already reshaped into the baseline directory layout, so the maintainer can extract the artifact straight into e2e/storybook/__snapshots__/ and commit. Baselines, diffs and review history all stay in git — no external service holds the source of truth.
- Drop the obsolete `chromatic.viewports` parameter from .storybook/preview.tsx. Chromatic is no longer used; viewports are set in the Playwright spec. - Remove `--single` from the storybook-static serve command. The flag is a SPA-style fallback to index.html and is semantically wrong for Storybook, which serves distinct entries (index.html for the manager, iframe.html for stories). - Simplify the baseline-candidate staging step: drop the `compgen -G "**/..."` precheck which silently relied on bash's globstar option (off by default on GitHub-hosted runners) and use `find -print0` with NUL separators to handle paths with spaces correctly.
The deployed app preloads Inter from Google Fonts in public/index.html and src/index.css falls back to it as the default sans-serif. The Storybook build had no equivalent, so stories were rendered with the runner's system default font and the captured baselines would drift from production. Mirror the font tags via .storybook/preview-head.html before generating the initial baselines.
#1094) Two correctness bugs found while seeding the initial baselines: 1. `npx serve storybook-static` strips `.html` extensions by default and 301-redirects `/iframe.html?id=…` to `/iframe`, dropping the query string. Storybook then has no story to render and falls back to its "No Preview" screen — every story was captured as the same empty page, so the test could not have detected any layout drift. Fix: ship a `serve.json` (cleanUrls: false) as a Storybook static asset via `staticDirs`, so the built bundle is self-contained and serves correctly to any tool that reads `serve.json`. 2. The snapshot path template referenced `{projectName}` but the `<name>-actual.png` files Playwright emits on failure carry no project suffix. Re-shaping the actuals into the baseline directory layout produced filenames that the next run would not look for. Fix: drop `-{projectName}` from snapshotPathTemplate. We only run a single project (chromium-linux); adding more later means re-adding the segment. Verified locally that the three stories now render distinctly (Default == Fullscreen by design, Dialog with backdrop + centered card), so the regression guard is actually load-bearing.
This was referenced May 12, 2026
…ffset, deterministic wait (#1096) Three improvements after inspecting the first round of CI-generated baselines: 1. Decorator (.storybook/preview.tsx) The previous header surrogate was an empty 64px-tall blue band with no content; the modalRootRef anchor sat at y=64 but was a zero-size div, and the screenshots were taken before Modal's ResizeObserver had updated topOffset, so the fullscreen modal covered the entire viewport including the simulated header. The decorator now renders a visible "DFX Services" label inside the header band, places the modalRootRef anchor as a real positioned div, and matches the layout shape the app actually uses, so the captured baselines show the fullscreen modal sitting cleanly below the header. 2. Spec wait (e2e/storybook/modal-visual.spec.ts) Added an explicit wait for the modal portal (any element with `.z-50`) to become visible, then two animation frames before the snapshot. This removes the race between Modal's post-mount ResizeObserver effect and Playwright's `toHaveScreenshot`, so the captured `top` style is the final stable value, not the initial 0. 3. Story content (src/components/modal.stories.tsx) Replaced the placeholder text with realistic content that mirrors what the deployed app actually puts inside each variant: - Fullscreen: a KYC-style identity-verification step with a step indicator, three labeled inputs, a privacy notice and a button row. Mirrors the layout of KYC, Safe Deposit, Buy/Sell flows. - Dialog: a transaction-refund confirmation card with a detail table and Confirm/Cancel buttons. Mirrors the chargeback, limit-request and recall modals shipped in #1048. The Default story still renders `<Modal isOpen>` with no `variant` prop, so the byte-equality between Default and Fullscreen remains the load-bearing regression signal for the #1048 bug class.
Six baselines generated by the Linux CI runner after the hardening in #1096 (visible header surrogate, deterministic ResizeObserver wait, realistic story content). Sanity checks before commit: - modal-default-* and modal-fullscreen-* are byte-identical at both viewports (load-bearing: this is the property that catches the #1048 default-flip regression). - modal-dialog-* is distinct at both viewports (centered card on translucent backdrop, separate Refund-transaction content). - The blue layout header is visible above every fullscreen modal, confirming Modal's topOffset/ResizeObserver path is actually exercised by the snapshot. - Inter font is applied (no system fallback). Captured by https://github.com/DFXswiss/services/actions/runs/25752660204
…selines (#1100) The five days between the baseline capture run (#1099) and the verification run on the parent PR brought a Chromium / Inter-font subpixel drift that pushed three 375-px snapshots over the previous 1 % threshold (4184 mismatched pixels, ratio 0.014). Desktop runs stayed below 1 % because absolute drift divided by 1024×800 is much smaller; the visible diff is text-only, with form borders and layout unchanged. Two adjustments: 1. Raise `maxDiffPixelRatio` to 0.05 in playwright.storybook.config.ts. Real layout regressions (variant flip, colour change, position shift) produce 10–50 %+ drift, so the gate keeps its full protective value while no longer flapping on text-rendering updates that happen outside our control. 2. Refresh the three mobile baselines (default, fullscreen, dialog) with the current Chromium output so the snapshots match the new text rendering. Desktop baselines stay as-is. Default ↔ Fullscreen byte-equality at both viewports is preserved (SHA-1 identical), keeping the #1048-class regression guard intact.
Five small cleanups found in a final review pass against the rest of
the codebase:
- .storybook/main.ts: drop the `?? {}` / `?? []` fallbacks. The
webpackFinal hook receives a fully-initialised config from Storybook;
the defensive defaults are unreachable and violate the repo's
no-fallbacks rule. Use non-null assertions to keep TypeScript happy.
- .storybook/main.ts: drop the unused `async` on webpackFinal — no
awaited expressions inside.
- playwright.storybook.config.ts: rename the single project from
`chromium-linux` to `chromium` to match playwright.config.ts. The
snapshot path template doesn't include {projectName}, so no captured
paths move.
- package.json: remove the eslintConfig override for **/*.stories.*.
The disabled rule (`import/no-anonymous-default-export`) was a
precaution; `export default meta` references a named const and
passes lint without the carve-out.
- .storybook/preview.tsx: shorten the StorybookLayoutHost comment to a
single line. Component identifiers carry the rest.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context — the bug this PR prevents
In April 2026, PR #1048 (compliance improvements) silently changed the
<Modal>component's default rendering:The author needed a dialog-style overlay for new compliance flows and changed the Modal's default behavior instead of introducing a variant. The PR was merged because no test asserted the visual contract — every fullscreen modal in production (KYC, Safe Deposit, Buy / Sell, recall, recall details, ConfirmDialog, etc.) suddenly rendered as a centered card on a dim backdrop. The regression sat in production for 26 days until PR #1090 reintroduced an explicit
variant: 'fullscreen' | 'dialog'prop and restored the original layout.This PR adds the missing safety net so the same class of regression cannot reach production again — using only tooling that lives inside this repository.
Approach — self-hosted Visual Regression
Visual Regression Testing (VRT) is the industry-standard answer for layout/style regressions in shared UI components. This PR brings VRT to the repo without any external SaaS dependency:
<Modal>from routing, auth and layout dependencies so each variant renders deterministically.Files
.storybook/main.ts,preview.tsx@storybook/preset-create-react-app. StripsForkTsCheckerWebpackPluginso the Storybook build mirrorsreact-app-rewired build'sCI=falsebehaviour. Global decorator provides aLayoutContextProviderwith real DOM refs so Modal'screatePortalresolves.src/components/modal.stories.tsxDefault(novariant— the exact configuration that broke in #1048),Fullscreen,Dialog.playwright.storybook.config.tsserve storybook-static) and__snapshots__/location so it never clashes with the existing app-level e2e config.e2e/storybook/modal-visual.spec.tstoHaveScreenshot..github/workflows/visual-regression.ymlbaseline-candidates/directory so first-run baselines can be committed without manual renaming.npm scriptsstorybook,build-storybook,test:visual.tsconfig.json/tsconfig.build.jsonWhy the Default story is the actual regression guard
Defaultrenders<Modal isOpen>with novariantprop — the exact code path every existing caller relied on before #1048. If a future PR flips the default again,Default's baseline will diverge fromFullscreen's and the CI check fails. Impossible to merge without an explicit human accept (rebase + commit new baselines).How the gate works
visual-regression.ymlruns.localhost:6006, runs the 6 screenshot tests (3 stories × 2 viewports).e2e/storybook/__snapshots__/.maxDiffPixelRatio: 0.01tolerates Chromium subpixel drift, fails on real layout regressions.actualPNGs are uploaded as artifactvisual-regression-diff.e2e/storybook/__snapshots__/and commits.The original #1048 regression would have produced an immediate diff on
Default: white fullscreen panel → dark overlay with a centered card.One-time setup: seed the initial baselines
The PR ships without baselines because they must be generated on Linux (CI's
chromium-linux); generating them on macOS would create-darwinfilenames that CI ignores. The first CI run on this PR is therefore expected to fail — that failure produces the baselines:Visual Regressionrun on this PR finishes red.visual-regression-diff.zip.baseline-candidates/modal-visual.spec.ts/*.pngare already named exactly as Playwright expects.e2e/storybook/__snapshots__/modal-visual.spec.ts/.After that the gate is self-sustaining; every future PR that changes the Modal layout will trip the check.
Validated locally
npm ci(strict, matches CI exactly) clean.npm run lintclean.npm run test— 276/276 passing.npm run build:dev— app build unchanged.npm run build-storybook— Storybook compiles.npx playwright test --config=playwright.storybook.config.ts --list— 6 tests discovered.Out of scope (intentionally)
variantrequired at the type level. Complementary structural guard, better as a separate PR once the visual layer is established.