ci: cut ds-shots PR wall-clock — build cache + 2-width captures (TASK-22081) - #2907
Conversation
The job is advisory and blocks nothing, but at ~9-12 min it is the longest job on every PR, so the checks list looks busy long after the blocking gates are green. Two cuts, no lost always-on signal: - cache .next/cache (Next docs recipe) — a warm cache roughly halves the ~4 min build - PR runs capture the two extreme widths (320 finds the most overflow, 430 is the widest device); baseline pushes keep all four, and visual-diff.mjs no longer reports widths a partial capture never shot as removed, so the 4-width baseline diffs cleanly Accepted trade-off: a breakpoint bug specific to 375/393 surfaces after merge instead of on the PR. TASK-22081
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Code-analysis diffPainscore total: 7153.05 → 7153.05 (0) |
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![chip-peanut-bot[bot]](https://avatars.githubusercontent.com/in/2915219?s=60&v=4){kind=small}
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean review. The two-width PR capture keeps actionable added, changed, and removed-screen reporting at captured widths, while the Next build cache and capture arguments preserve the workflow's read-only trust boundary.
Checked clean
- Verified detached HEAD, trusted author, dev base ref, and exact merge base against the supplied SHAs.
- Reviewed the Next build cache key and restore behavior, pull-request-only 320/430 project selection, and unchanged contents:read permission boundary.
- Exercised the visual-diff logic adversarially against partial-width captures, genuine fixture removal, additions, pixel changes, empty or failed captures, and the fixture filename contract.
- Exact-head blocking CI completed green, including unit, typecheck, format, eslint, ds-lint, and ci-success; the modified ds-shots cache, build, capture, baseline restore, diff, list, upload, and summary steps also succeeded while downstream regression specs were still running.
- Checked recent history and surrounding Playwright capture and visual-comment contracts for regressions and architecture drift.
Second opinion by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only, so treat its findings as advice.
Exact head: 5f2b4229277b · Context: repo
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean review. The two-width PR capture preserves actionable added, changed, and removed-screen reporting at captured widths, while the Next build cache and capture arguments keep the workflow's read-only trust boundary intact.
Checked clean
- Verified detached HEAD, trusted author, ready PR state, dev base ref, and exact merge base against the supplied SHAs.
- Reviewed the Next build cache key and restore behavior, pull-request-only 320/430 project selection, push behavior, and unchanged contents:read permission boundary.
- Checked partial-width diff behavior for baseline-only widths, genuine fixture removal, pixel changes, additions, and the fixture filename contract; the new CLI-level tests cover those paths.
- All exact-head CI completed green, including unit, typecheck, format, eslint, ds-lint, ci-success, Deploy Preview, and the full ds-shots build, two-width capture, baseline restore, diff, summary, and regression flow.
- Checked recent workflow history and surrounding Playwright capture and visual-comment contracts for regressions, unsafe credential flow, and architecture drift.
Second opinion skipped: daily-budget-reached.
Exact head: 5f2b4229277b · Context: repo
Review finding (validated): the full cache action saved a fresh 100-400 MB entry on nearly every PR push — branch-scoped, so no other PR could read it — churning the shared 10 GB quota and spending 1-2 min of post-step upload that ate part of the speedup. PRs now restore-only and pick up the near-fresh dev-scoped entry via restore-keys; push runs save, right after Build so a capture failure does not lose the cache. TASK-22081
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean review. The PR-only two-width capture remains compatible with four-width baselines, and the push-only Next cache preserves the workflow's read-only PR trust boundary.
Findings
- MINOR · scripts/visual-diff.mjs:104 · [moonshotai/kimi-k3] Empty head capture now diffs all-green instead of reporting every shot removed
The new filter counts a baseline file as removed only if its width exists anywhere in the after set (afterWidths.has(widthOf(f))). If the capture step ever succeeds while producing zero PNGs — e.g. a capture-script or playwright config regression that exits 0 with no output, or a project filter matching a project with no tests —afterWidthsis empty, soremovedis [] and the report shows no removals at all: 120 vanished screenshots look identical to a clean pass. Before this change, an empty after directory reported every baseline file as removed, which is exactly the loud failure signal you want. The job is advisory, so nothing blocks, but the diff-noise fix has converted the tool's most catastrophic input into silence. Fix: guard the degenerate case, e.g.if (before.size > 0 && after.size === 0) fail (exit non-zero or report all before files as removed) before applying the width-aware filter, and/or only apply the filter whenafter.size > 0`.
Checked clean
- Verified the detached worktree head, exact merge base, trusted author, and dev target against the supplied metadata.
- Checked partial-width removal detection against the screenshot filename contract and Playwright project names; deleted screens remain detectable at captured widths.
- Checked Next cache restore/save scoping and keys; PR runs remain restore-only and no write permission or credential boundary was added.
- Checked capture command argument expansion and push-versus-pull-request paths.
- Exact-head CI is green, including unit, ds-shots, typecheck, lint, formatting, and provenance gates. Local dependency-based reruns were unavailable in the detached worktree, so CI supplied the executable validation.
Second opinion by moonshotai/kimi-k3: 1 finding(s), marked with the model name. It reads the diff only, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: f91940338da1 · Context: repo
Chip/kimi finding on the width-aware removal filter: with zero after-files the filter reported an empty capture as all-green — the tool's most catastrophic input became silence. A non-empty baseline with an empty capture now exits 2 before the filter applies, matching the script's contract that non-zero means the tool broke. TASK-22081
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Reviewed the exact head against the pinned dev base. The prior empty-capture finding is fixed, and no actionable correctness, security, adversarial, or maintainability defects remain.
Checked clean
- Confirmed P1 is fixed: a non-empty baseline with an empty head capture exits 2 before width filtering, with a CLI-level regression case.
- Checked partial-width capture selection and width-aware removed-shot reporting; real removals at captured widths remain visible while uncaptured baseline widths are ignored.
- Checked the Next build cache event gating, branch-scoped restore strategy, read-only job permissions, and push-only save behavior.
- Exact-head CI completed successfully, including unit, ds-shots, typecheck, lint, format, and ci-success.
- A focused local Jest rerun was unavailable because the detached worktree has no installed Jest binary; the exact-head unit and ds-shots checks are green.
Second opinion by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 6127f4ac17fa · Context: repo · Took 4m (queued 6m)
2 participants
Summary
Slava reports the ds-shots job adds 4–5 minutes to every PR check. The job is advisory and blocks nothing (it is deliberately not in
ci-success.needs), but at ~9–12 min it is the longest job on every PR, so the checks list looks busy long after the blocking gates went green. Two cuts, keeping the always-on pre-merge signal:.next/cache, Next.js docs recipe). A warm cache roughly halves the ~4 min build. Keyds-shots-next-<os>-<lockfile>-<src-hash>with a per-lockfile restore-keys fallback. Restore-only on PR runs; the save happens on push runs (dev/main) only — a per-PR save would upload a fresh 100–400 MB branch-scoped entry no other PR can read, churning the shared 10 GB Actions cache quota and spending 1–2 min per push on the upload (agentic-review finding, validated). Push runs land several times a day, so every PR restores a near-fresh dev-scoped entry. The save sits right after Build, not in a post step, so a capture failure doesn't lose the cache.--project=320 --project=430onpull_requestevents.Expected: ~10 min → roughly 4–6 min per PR run. No permission or trust-boundary changes — the job keeps
contents: readand nothing else.Task: TASK-22081
The diff-noise fix that makes (2) possible
The 4-width baseline vs a 2-width capture would report ~60 "removed" shots per PR.
scripts/visual-diff.mjsnow counts a file as removed only when its width exists in the after capture — a width the run never shot proves nothing. A screen that is really deleted is still reported at the captured widths. Covered byscripts/__tests__/visual-diff-widths.test.js(real PNGs through the actual CLI): partial-width capture produces zero noise, a genuinely deleted screen still reports, changed/added detection intact.Design notes / accepted trade-offs
ds-shots-next-*cache entries restores cold builds.QA
Screenshots: N/A (no visible app change — CI only).