Skip to content

feat(ci): add visual PR scoring to analysis report (dry run)#3748

Open
jiunshinn wants to merge 1 commit into
facebook:mainfrom
jiunshinn:ci/visual-diff-classifier
Open

feat(ci): add visual PR scoring to analysis report (dry run)#3748
jiunshinn wants to merge 1 commit into
facebook:mainfrom
jiunshinn:ci/visual-diff-classifier

Conversation

@jiunshinn

@jiunshinn jiunshinn commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Part of #3667phase 1 (dry run). Statically classifies each PR's diff as visual / likely-visual / maybe-visual / non-visual and surfaces the result in the PR Analysis Report comment. No labels, no reviewer requests — the point is to calibrate against real PR traffic first (open question 1 in #3667).

Summary

  • .github/scripts/lib/classify-visual.js — pure function over the unified diff; no rendering, no install/build.
  • .github/scripts/classify-visual-pr.js — CI entry point. Fails loudly if there is no merge base instead of classifying an empty diff as non-visual.
  • ci.yml — new lightweight classify-visual job with fetch-depth: 0, runs on every PR including docsite-only (docsite CSS is a visual surface, e.g. fix/docsite: flashing of desktop UI on docs/components #3665). Both pr-comment and pr-comment-update download the artifact, so the section survives comment updates.
  • generate-pr-comment.js reads the classification from its own artifact; analyze-pr.js is untouched.
  • vitest.config.ts — include .github/scripts/**/*.test.mjs so the 23 classifier tests run under pnpm test.

Scoring

Per #3667: CSS-property edits (×3) + design-token references (×2) + style-surface files (×3 per file) + JSX structural changes (×1, capped at 10); buckets ≥12 / ≥5 / ≥2.

Deviations from the issue spec: generic token regex (*Vars. and *Vars[...] — covers shadowVars, borderVars, bracket access); *.markers.stylex.ts are not style surfaces (scoping markers move no pixels); TS type members (width: number;) don't count; multi-line block comments are tracked per diff side.

Scope decisions and known limitations

  • Only product surfaces (packages/, apps/) are scanned. Tooling (.github/, scripts/, root configs) can mention CSS-ish content — mocks, fixtures, CSS generators — without moving pixels. (This PR classifies itself non-visual, score 0.)
  • Test/story/doc/snapshot/__tests__ files are skipped; only .ts/.tsx/.js/.jsx/.mjs/.css lines are scanned.
  • Known miss: code that generates CSS (e.g. build-theme.mjs in Fix built theme.css pinning color-scheme so Theme mode can force ligh… #3690) has no static signal. The rendered-pixel tier from Auto-flag visual PRs for non-blocking design review #3667 would catch it.
  • A hunk can begin mid-block-comment the diff never shows; the tracker resets at hunk boundaries rather than guessing.

Validation

Calibrated against 22 real PRs (recent merged + open): 21/22 matched manual expectations; the a11y wave classifies non-visual while focus-ring fixes (#3722/#3723) correctly flag. #3649 flags visual because it bundles a Text font-size change alongside the pagination fix — the "bundled style edit" case #3667 calls arguably correct.

Calibration table
PR Change Expected Classified Score
#3713 TabList hover underline visual visual 114
#3712 ClickableCard hover tint visual visual 165
#3730 prefers-reduced-motion streaming text visual likely-visual 10
#3690 theme.css color-scheme pinning visual non-visual ⚠️ known miss 0
#3682 CollapsibleGroup hasDividers visual visual 16
#3633 ContextMenu display:contents visual visual 88
#3665 docsite flash of desktop UI visual visual 21
#3677 WCAG palette contrast visual visual 259
#3722 restore Collapsible focus ring visual likely-visual 8
#3723 CodeBlock focus ring visual likely-visual 8
#3727 announce Lightbox changes (SR) boundary non-visual 0
#3724 announce Calendar month (SR) boundary non-visual 0
#3708 aria-current on Calendar today non-visual non-visual 0
#3742 ProgressBar zero-max guard non-visual non-visual 0
#3738 forward rest props to input non-visual non-visual 0
#3737 node: protocol refactor non-visual non-visual 0
#3700 pnpm 11 bump non-visual non-visual 0
#3746 test fixture swap non-visual non-visual 0
#3649 Table page index sanitize (+ bundled Text font-size change) non-visual visual ⚠️ correct 58
#3598 SegmentedControl pure focus move non-visual non-visual 0
#3705 docs comment on memo comparator non-visual non-visual 0
#3735 useEffectEvent refactor non-visual non-visual 0

Tests: 23 vitest cases (signals, file filtering, block comments, buckets, section rendering); full pnpm test and check:repo green. No changeset (no published package changed).

Found while validating (pre-existing, not fixed here): the build job fetches the base branch with --depth=1 on a depth-1 checkout, so analyze-pr.js's three-dot diff has no merge base and component detection comes up empty on PRs. Happy to fix in a follow-up.

Out of scope (phase 2 — needs maintainer decisions)

Applying design/tokens/theming labels and requesting designer reviews (which designers? collaborator access?). Fork PRs: pr-comment still skips forks (read-only token); the classification artifact is uploaded regardless, but surfacing it on fork PRs would need wiring into #3659's comment flow as well.

@vercel

vercel Bot commented Jul 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview, Comment Jul 10, 2026 10:59am

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 10, 2026
@jiunshinn jiunshinn marked this pull request as ready for review July 10, 2026 09:57
@jiunshinn jiunshinn force-pushed the ci/visual-diff-classifier branch from 0964ffb to a28f421 Compare July 10, 2026 10:17
@jiunshinn jiunshinn changed the title feat(ci): statically classify visual PRs in the PR analysis comment (dry run) feat(ci): add visual PR scoring to analysis report (dry run) Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant