feat(qa): add visual reference and screenshot checks#464
Conversation
superagent-security
Bot
added
the
contributor:verified
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces a design system reference guide and screenshot QA documentation. A new design system reference page showcases tokens, primitives, and UI components with a collapsible sidebar interface. A separate QA documentation page provides guidance on screenshot testing with Playwright. SEO rules prevent search indexing, and site header navigation links provide access to both new pages. ChangesDesign System Documentation and QA Guidance
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
the
risk-low
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/app/design-system/reference/design-system-reference-client.tsx (1)
68-83: ⚡ Quick winImprove collapsible accessibility semantics.
The toggle in Line 68 should expose expansion state and controls target so screen readers can track section state reliably.
Suggested patch
<button type="button" onClick={() => setIsOpen(!isOpen)} + aria-expanded={isOpen} + aria-controls={`${id}-content`} className="flex w-full items-center justify-between gap-3 rounded-xl border border-border/60 bg-card/60 px-5 py-4 text-left transition hover:border-primary/40 hover:bg-card/80" > @@ - {isOpen && <div className="pl-2">{children}</div>} + <div id={`${id}-content`} className={cn("pl-2", !isOpen && "hidden")}> + {children} + </div> </section> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/design-system/reference/design-system-reference-client.tsx` around lines 68 - 83, The button toggle does not expose ARIA semantics for expansion state or the ID of the controlled content; update the toggle button in design-system-reference-client (the element using onClick={() => setIsOpen(!isOpen)}) to include aria-expanded={isOpen} and aria-controls referencing a stable id for the content panel, and assign that id to the container that renders {children} (e.g., panelId derived from title or a generated uid). Also add an accessible region attribute like role="region" and aria-labelledby pointing back to the button (or ensure the button has an id) so screen readers can associate the header and panel. Ensure the state variable isOpen, the setter setIsOpen, and the children panel use the matching ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/app/design-system/qa/page.tsx`:
- Around line 52-215: This server page (ScreenshotQaPage) must be instrumented
for observability: import withDuration, getServerLogger and normalizeError from
`@heyclaude/web-runtime/logging/server`, obtain or generate a requestId (e.g.,
from incoming headers or a request-id helper) and pass it to the logger, wrap
the component’s execution in withDuration(...) so the duration is tracked,
surround the render logic in a try/catch that logs errors using
normalizeError(error) along with requestId, and emit a final summary log
(including requestId and the recorded duration) before returning the JSX; update
all log calls to include requestId and reference the ScreenshotQaPage symbol so
reviewers can find the change.
In `@apps/web/src/app/design-system/reference/page.tsx`:
- Around line 14-16: Wrap the server page component DesignSystemReferencePage
with the required observability: import server logger from
`@heyclaude/web-runtime/logging/server` and withDuration(), call withDuration()
around the server handler, generate or accept a requestId and pass it to all
logs, and add try/catch that uses normalizeError() to log structured errors
(including requestId) before rethrowing; ensure you log a final summary
(duration, status, requestId) after rendering the DesignSystemReferenceClient so
the page emits start, error (if any) and final summary logs.
---
Nitpick comments:
In `@apps/web/src/app/design-system/reference/design-system-reference-client.tsx`:
- Around line 68-83: The button toggle does not expose ARIA semantics for
expansion state or the ID of the controlled content; update the toggle button in
design-system-reference-client (the element using onClick={() =>
setIsOpen(!isOpen)}) to include aria-expanded={isOpen} and aria-controls
referencing a stable id for the content panel, and assign that id to the
container that renders {children} (e.g., panelId derived from title or a
generated uid). Also add an accessible region attribute like role="region" and
aria-labelledby pointing back to the button (or ensure the button has an id) so
screen readers can associate the header and panel. Ensure the state variable
isOpen, the setter setIsOpen, and the children panel use the matching ids.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: af868266-4812-456c-b965-549d272bfc31
⛔ Files ignored due to path filters (1)
tests/e2e/screenshot-qa.spec.tsis excluded by!**/*.spec.ts,!tests/**and included by none
📒 Files selected for processing (5)
apps/web/src/app/design-system/qa/page.tsxapps/web/src/app/design-system/reference/design-system-reference-client.tsxapps/web/src/app/design-system/reference/page.tsxapps/web/src/app/robots.tsapps/web/src/components/site-header.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (Custom checks)
apps/web/src/**/*.{ts,tsx}: Client components ('use client') must not import server-only utilities (unstable_cache, headers, cookies)
Client components ('use client') must not use Node.js APIs (fs, path, etc.)
Client components ('use client') must use client-safe logging (logClientError, not logger from server)
Server components must not use browser APIs (window, document, localStorage)
Server components must not use React hooks (useState, useEffect, etc.)
Server components must use server logging from@heyclaude/web-runtime/logging/server
Server components and pages must track duration with withDuration()
Server components and pages must include requestId in all logs
Server components and pages must use normalizeError() in catch blocks
Server components and pages must have a final summary log
Client components with async operations must use useLoggedAsync hook
Client components with async operations must never use raw try/catch without logging
Do not use user input in database queries without sanitization
Do not construct URLs without validation
Do not perform file path operations without sanitization
Server actions must have auth checks before executing operations
Do not perform direct database mutations without permission checks
Do not log PII without hashing (userId, email, etc.)
Do not expose sensitive data in error messages
Do not include API keys or secrets in code
Do not use unsanitized user input in HTML rendering
Do not use dangerous eval() or Function() with user input
Do not use unsanitized user input in database queries (SQL/NoSQL injection prevention)
Files:
apps/web/src/app/design-system/reference/page.tsxapps/web/src/components/site-header.tsxapps/web/src/app/design-system/reference/design-system-reference-client.tsxapps/web/src/app/design-system/qa/page.tsxapps/web/src/app/robots.ts
apps/web/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Website/API behavior must live under
apps/web/src/
Files:
apps/web/src/app/design-system/reference/page.tsxapps/web/src/components/site-header.tsxapps/web/src/app/design-system/reference/design-system-reference-client.tsxapps/web/src/app/design-system/qa/page.tsxapps/web/src/app/robots.ts
🔇 Additional comments (2)
apps/web/src/app/robots.ts (1)
11-11: LGTM!Also applies to: 23-23
apps/web/src/components/site-header.tsx (1)
83-96: LGTM!
| export default function ScreenshotQaPage() { | ||
| return ( | ||
| <div className="container-shell space-y-10 py-12"> | ||
| <div className="space-y-4 border-b border-border/80 pb-8"> | ||
| <span className="eyebrow">Internal</span> | ||
| <h1 className="section-title">Screenshot QA Process</h1> | ||
| <P> | ||
| This page documents how we run visual regression checks across | ||
| HeyClaude using Playwright screenshot comparisons. | ||
| </P> | ||
| </div> | ||
|
|
||
| <section className="space-y-4"> | ||
| <H2>Overview</H2> | ||
| <P> | ||
| Screenshot QA prevents unintended visual regressions by capturing | ||
| baseline images of key views and comparing them against subsequent | ||
| builds. Any pixel difference above the configured threshold fails the | ||
| build and blocks deployment until reviewed. | ||
| </P> | ||
| </section> | ||
|
|
||
| <section className="space-y-4"> | ||
| <H2>Target Views</H2> | ||
| <P>Each release must pass screenshot comparison on the following routes:</P> | ||
| <Ul> | ||
| <Li> | ||
| <strong className="text-foreground">Home</strong> —{" "} | ||
| <code className="rounded bg-muted px-1.5 py-0.5 text-sm">/</code>{" "} | ||
| Hero, stats grid, directory preview, and footer. | ||
| </Li> | ||
| <Li> | ||
| <strong className="text-foreground">Browse</strong> —{" "} | ||
| <code className="rounded bg-muted px-1.5 py-0.5 text-sm">/browse</code>{" "} | ||
| Directory listing, search bar, filters, and cards. | ||
| </Li> | ||
| <Li> | ||
| <strong className="text-foreground">Detail</strong> —{" "} | ||
| <code className="rounded bg-muted px-1.5 py-0.5 text-sm">/:category/:slug</code>{" "} | ||
| Entry detail page with markdown prose, related cards, and vote rail. | ||
| </Li> | ||
| <Li> | ||
| <strong className="text-foreground">Submit</strong> —{" "} | ||
| <code className="rounded bg-muted px-1.5 py-0.5 text-sm">/submit</code>{" "} | ||
| Submission form, preview card, and readiness check. | ||
| </Li> | ||
| <Li> | ||
| <strong className="text-foreground">Quality</strong> —{" "} | ||
| <code className="rounded bg-muted px-1.5 py-0.5 text-sm">/quality</code>{" "} | ||
| Quality signals, provenance, and SEO checklist. | ||
| </Li> | ||
| <Li> | ||
| <strong className="text-foreground">Submissions</strong> —{" "} | ||
| <code className="rounded bg-muted px-1.5 py-0.5 text-sm">/submissions</code>{" "} | ||
| Submission queue and editorial status. | ||
| </Li> | ||
| </Ul> | ||
| </section> | ||
|
|
||
| <section className="space-y-4"> | ||
| <H2>Viewport Sizes</H2> | ||
| <P>Two viewport configurations are required:</P> | ||
| <Ul> | ||
| <Li> | ||
| <strong className="text-foreground">Desktop</strong> — 1920×1080 | ||
| (Chromium, full window) | ||
| </Li> | ||
| <Li> | ||
| <strong className="text-foreground">Mobile</strong> — 375×667 | ||
| (iPhone SE / small viewport) | ||
| </Li> | ||
| </Ul> | ||
| </section> | ||
|
|
||
| <section className="space-y-4"> | ||
| <H2>Running Tests</H2> | ||
|
|
||
| <div className="space-y-2"> | ||
| <H3>Update baselines</H3> | ||
| <P>Run after intentional design changes to accept new references:</P> | ||
| <CodeBlock>{`npx playwright test --update-snapshots`}</CodeBlock> | ||
| </div> | ||
|
|
||
| <div className="space-y-2"> | ||
| <H3>Run comparison only</H3> | ||
| <P>Fail on any difference from the stored baseline:</P> | ||
| <CodeBlock>{`npx playwright test tests/e2e/screenshot-qa.spec.ts`}</CodeBlock> | ||
| </div> | ||
|
|
||
| <div className="space-y-2"> | ||
| <H3>Run with UI mode</H3> | ||
| <P>Useful for debugging failures side-by-side:</P> | ||
| <CodeBlock>{`npx playwright test --ui`}</CodeBlock> | ||
| </div> | ||
|
|
||
| <div className="space-y-2"> | ||
| <H3>Run against a deployed preview</H3> | ||
| <P>Point tests to a staging or preview URL:</P> | ||
| <CodeBlock>{`PLAYWRIGHT_BASE_URL=https://preview.heyclau.de npx playwright test tests/e2e/screenshot-qa.spec.ts`}</CodeBlock> | ||
| </div> | ||
| </section> | ||
|
|
||
| <section className="space-y-4"> | ||
| <H2>Test Structure Example</H2> | ||
| <P>A minimal Playwright spec using screenshot comparison:</P> | ||
| <CodeBlock>{`import { test, expect } from "@playwright/test"; | ||
|
|
||
| test("home page matches visual reference", async ({ page }) => { | ||
| await page.goto("/"); | ||
| await expect(page).toHaveScreenshot("home-desktop.png", { | ||
| fullPage: true, | ||
| }); | ||
| }); | ||
|
|
||
| test("browse page matches visual reference", async ({ page }) => { | ||
| await page.goto("/browse"); | ||
| await expect(page).toHaveScreenshot("browse-desktop.png", { | ||
| fullPage: true, | ||
| }); | ||
| });`}</CodeBlock> | ||
| </section> | ||
|
|
||
| <section className="space-y-4"> | ||
| <H2>Thresholds & Tolerance</H2> | ||
| <Ul> | ||
| <Li> | ||
| Default pixel diff ratio: <code className="rounded bg-muted px-1.5 py-0.5 text-sm">0.2</code> | ||
| </Li> | ||
| <Li> | ||
| Animations are disabled during capture via{" "} | ||
| <code className="rounded bg-muted px-1.5 py-0.5 text-sm">caret-color: transparent</code>{" "} | ||
| and reduced-motion emulation. | ||
| </Li> | ||
| <Li> | ||
| Dynamic content (e.g., live GitHub stars) is stubbed or hidden | ||
| with <code className="rounded bg-muted px-1.5 py-0.5 text-sm">data-testid="screenshot-stable"</code>{" "} | ||
| wrappers where possible. | ||
| </Li> | ||
| </Ul> | ||
| </section> | ||
|
|
||
| <section className="space-y-4"> | ||
| <H2>Baseline Storage</H2> | ||
| <P> | ||
| Baseline screenshots are committed to the repository under{" "} | ||
| <code className="rounded bg-muted px-1.5 py-0.5 text-sm">tests/__screenshots__/</code>. | ||
| Each platform and viewport gets its own subdirectory so CI can run | ||
| Linux baselines while local development uses macOS or Windows | ||
| equivalents. | ||
| </P> | ||
| </section> | ||
|
|
||
| <section className="space-y-4"> | ||
| <H2>CI Integration</H2> | ||
| <P> | ||
| The screenshot QA job runs after the build step and before deployment. | ||
| If snapshots are missing for the current platform, the job generates | ||
| them and uploads them as artifacts rather than failing. On subsequent | ||
| runs, the generated artifacts are compared. | ||
| </P> | ||
| </section> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Instrument this server page with required logging/duration flow.
The page is a server component but currently lacks the mandated withDuration() usage plus requestId-based server logs and a final summary log, which leaves it out of compliance with the repo’s server-page observability contract.
As per coding guidelines, “Server components must use server logging from @heyclaude/web-runtime/logging/server”, “Server components and pages must track duration with withDuration()”, “Server components and pages must include requestId in all logs”, “Server components and pages must use normalizeError() in catch blocks”, and “Server components and pages must have a final summary log”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/app/design-system/qa/page.tsx` around lines 52 - 215, This
server page (ScreenshotQaPage) must be instrumented for observability: import
withDuration, getServerLogger and normalizeError from
`@heyclaude/web-runtime/logging/server`, obtain or generate a requestId (e.g.,
from incoming headers or a request-id helper) and pass it to the logger, wrap
the component’s execution in withDuration(...) so the duration is tracked,
surround the render logic in a try/catch that logs errors using
normalizeError(error) along with requestId, and emit a final summary log
(including requestId and the recorded duration) before returning the JSX; update
all log calls to include requestId and reference the ScreenshotQaPage symbol so
reviewers can find the change.
| export default function DesignSystemReferencePage() { | ||
| return <DesignSystemReferenceClient />; | ||
| } |
There was a problem hiding this comment.
Add required server-page observability wrapper and logging.
This server page currently returns immediately without the required duration tracking and structured server logs (including requestId and final summary), so it doesn’t meet the repo’s server-page requirements.
As per coding guidelines, “Server components must use server logging from @heyclaude/web-runtime/logging/server”, “Server components and pages must track duration with withDuration()”, “Server components and pages must include requestId in all logs”, “Server components and pages must use normalizeError() in catch blocks”, and “Server components and pages must have a final summary log”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/app/design-system/reference/page.tsx` around lines 14 - 16, Wrap
the server page component DesignSystemReferencePage with the required
observability: import server logger from `@heyclaude/web-runtime/logging/server`
and withDuration(), call withDuration() around the server handler, generate or
accept a requestId and pass it to all logs, and add try/catch that uses
normalizeError() to log structured errors (including requestId) before
rethrowing; ensure you log a final summary (duration, status, requestId) after
rendering the DesignSystemReferenceClient so the page emits start, error (if
any) and final summary logs.
JSONbored
left a comment
JSONbored
left a comment
There was a problem hiding this comment.
This needs substantial changes before it can be reviewed for merge. Current blockers: validate-ci fails due unformatted files, validate-web fails because the new screenshot tests have no committed baselines, and CodeRabbit still has unresolved observability comments for both new server pages. Also, the issue asked for an unlisted/noindex design reference; adding “Design” and “QA” links to the public site header contradicts that. Please remove public header navigation, fix formatting, resolve CodeRabbit, either commit deterministic screenshot baselines or make the screenshot path documentation/advisory only, and provide desktop/mobile screenshots of the new reference/QA pages.
Kelvinchen03
force-pushed
the
design/qa-visual-reference-screenshot-checks
branch
from
24a547a to
57d1580
Compare
…advisory only - Remove Design/QA links from public header (routes remain unlisted) - Revert Playwright screenshot config to original state - Screenshot QA documentation remains as advisory reference - Routes are still noindex and excluded from robots/sitemap Closes JSONbored#438
2 participants
Pull Request
Summary
Added a visual reference page (
/design-system/reference) and screenshot QA documentation (/design-system/qa) for the HeyClaude design system. This provides contributors with a single location to inspect design tokens, UI primitives, component states, and verify dark/light mode rendering. Also added Playwright screenshot tests for major UI surfaces at desktop and mobile viewports.Submission Source
content/<category>/submittedByandsubmittedByUrlfrontmatter matching the PR author.README.md, generated registry outputs, orapps/web/public/downloads/**unless this is a maintainer/internal automation branch./downloads/...package hosting for community-submitted ZIP/MCPB artifacts.Schema and Quality Checks
pnpm validate:contentpassedpnpm validate:packagespassedpnpm scan:packagespassed when package artifacts changedpnpm audit:contentran and I reviewed findingsviewCount,copyCount,popularityScore)skillType,skillLevel,verificationStatus,verifiedAt,retrievalSources,testedPlatforms)Validation
pnpm build)Notes
robots: { index: false, follow: false }metadata and excluded from robots.ts and sitemap.ts to prevent SEO indexing.