Repository navigation
ci: post story screenshots at ai-loop handoff (#284) - #352
Conversation
Give the human reviewer eyes at handoff: when an ai-loop PR flips to needs-human-review, a new story-screenshots.yml workflow captures the Storybook stories affected by the PR's changed files with Playwright — once light, once dark — and posts them to the PR as an upserted marker comment (inline images) plus a 30-day artifact. The loop's reviewers are text-only; CSS bugs invisible to jsdom survived four review rounds on PR #257, and a picture at handoff is the cheapest catch. - bulma-ui/scripts/capture-stories.mjs maps changed files to story IDs via storybook-static/index.json (importPath for changed stories files, componentPath for changed components, basename heuristic for scss), serves the built storybook, and screenshots each story per theme on a fresh load. Dark needs emulated prefers-color-scheme, not just the data-theme attribute: bulma-prefixed.min.css loads after bulma.min.css and its media-scoped light :root block outranks the unprefixed [data-theme=dark] block in the cascade. - story-screenshots.yml splits privileges: `capture` runs PR code with a read-only token (reusing the CI run's storybook artifact when present, building as fallback); `publish` holds the write scopes, runs no PR code, treats capture output as untrusted (filename whitelist, escaped titles), commits images to the disposable orphan branch story-screenshots for raw.githubusercontent embedding, and upserts the <!-- story-screenshots --> comment. - claude-pr-loop.yml dispatches the workflow at both handoff sites — label edits via GITHUB_TOKEN fire no `labeled` runs (recursion guard), workflow_dispatch does. The labeled trigger covers humans applying the label by hand. Closes #284
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughAdds automated Storybook screenshot capture for affected stories during AI-loop handoff, including light and dark themes, artifact publication, pull request comments, failure reporting, and documentation for manual runs. ChangesStory screenshot handoff
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AI Loop
participant Story Screenshots
participant Storybook
participant Playwright
participant GitHub PR
AI Loop->>Story Screenshots: dispatch with PR number
Story Screenshots->>Storybook: build or reuse static output
Story Screenshots->>Playwright: capture affected stories in light and dark themes
Playwright-->>Story Screenshots: PNGs and manifest
Story Screenshots->>GitHub PR: publish artifact and upsert screenshot comment
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Preview DeploymentPreview URL: https://07acc85f.bestax.pages.dev |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@claude deep review |
|
Claude finished @allxsmith's task in 6m 26s —— View job Deep review
This is an unusually well-documented PR — the security split (read-only One real gap: No signal if the Everything else — |
The deep review on #352 caught a silent-failure gap: `publish` hangs off a plain `needs: capture`, so Actions auto-skips it whenever capture fails or hits its 20-minute timeout. A skipped publish is silence — which is exactly what a healthy "nothing to screenshot" run looks like from the PR. A Playwright install that 500s would read to the reviewer as "no CSS to look at here", defeating the point of putting eyes on the handoff. Add a report-failure job, gated on `needs.capture.result == 'failure'`, that upserts the same marker comment with the failure and a link to the run. Reusing the comment means the notice also supersedes images from an earlier run, rather than leaving stale ones to pass as current. `failure` only: `cancelled` is the concurrency group retiring a superseded run and `skipped` is the label guard, neither of which is news. When capture dies before resolving a PR number — the deliberate refusals, closed or cross-repo — the job stays silent, since declining to run is not breakage. The marker moves to a workflow-level env var so the two jobs that upsert that comment cannot drift apart and start stacking two of them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014se1ctqQjBCBaqjQRfHdzi
Preview DeploymentPreview URL: https://5c5f97e4.bestax.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/story-screenshots.yml (3)
47-51: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftReject protected-configuration PRs before capture.
The eligibility check only looks for
bulma-ui/src/; an AI-loop PR that also changes.github/**, Jest, commitlint, release, or pnpm-workspace configuration is still checked out, built, captured, and published. Add a read-only preflight that marks such PRs ineligible and gate all downstream jobs on it.As per coding guidelines, “Do not allow the AI development loop to process pull requests that modify
.github/**or jest, commitlint, release, or pnpm-workspace configuration.”Also applies to: 96-113
🤖 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 @.github/workflows/story-screenshots.yml around lines 47 - 51, Add a read-only preflight job that detects pull requests modifying .github/** or Jest, commitlint, release, or pnpm-workspace configuration, marks those PRs ineligible, and expose its result as an output. Update the workflow_dispatch/pull-request eligibility condition and all downstream jobs to require the preflight approval, while preserving manual dispatch behavior.Source: Coding guidelines
248-283: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle already-exists bootstrap races for
story-screenshots.Concurrency is per PR, so two first-time PR runs can both observe a missing
story-screenshotsref. Both may run the bootstrap path and both then awaitcreateRef; the first can succeed while the later one fails before it reaches the append retry loop, so the PR gets no handoff comment. Catch the ref-already-exists response, fetchheads/${BRANCH}, and fall through to the existing append retry path.🤖 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 @.github/workflows/story-screenshots.yml around lines 248 - 283, Update the bootstrap logic around createRef in the story-screenshots workflow to handle an already-existing ref caused by concurrent runs. Catch the specific ref-already-exists response from github.rest.git.createRef, fetch the current heads/${BRANCH} ref to refresh tip, and continue into the existing append retry path; rethrow unrelated errors and preserve the current bootstrap behavior when creation succeeds.
64-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPin the changed-file list to the resolved base/head SHAs.
gh pr diff "$PR"reads the PR’s current diff at runtime, whilehead_shais pinned from the resolved PR metadata. Ifpush/the PR branch advances afterResolve PR, the checkout can use the old head while changed-file detection uses the new one. FetchbaseRefOidand run a pinned compare likegh pr diff "$PR" ... --base "$BASE_SHA" --head "$HEAD_SHA"or use the compare API against the stored bases/heads.🤖 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 @.github/workflows/story-screenshots.yml around lines 64 - 82, Update the Resolve PR step to also retrieve and expose the resolved base SHA, alongside head_sha. In the changed-file detection flow, replace the unpinned gh pr diff invocation with a comparison explicitly using the stored base and head SHAs, ensuring file detection matches the checkout selected by Resolve PR.
🤖 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 `@docs/docs/guides/getting-started/ai-development.md`:
- Line 97: Update the manual story-screenshots workflow command in the
getting-started documentation to use a shell-safe concrete pull request value
such as 123, and note that readers should replace it with the relevant pull
request number. Keep the existing command and surrounding instructions
unchanged.
---
Outside diff comments:
In @.github/workflows/story-screenshots.yml:
- Around line 47-51: Add a read-only preflight job that detects pull requests
modifying .github/** or Jest, commitlint, release, or pnpm-workspace
configuration, marks those PRs ineligible, and expose its result as an output.
Update the workflow_dispatch/pull-request eligibility condition and all
downstream jobs to require the preflight approval, while preserving manual
dispatch behavior.
- Around line 248-283: Update the bootstrap logic around createRef in the
story-screenshots workflow to handle an already-existing ref caused by
concurrent runs. Catch the specific ref-already-exists response from
github.rest.git.createRef, fetch the current heads/${BRANCH} ref to refresh tip,
and continue into the existing append retry path; rethrow unrelated errors and
preserve the current bootstrap behavior when creation succeeds.
- Around line 64-82: Update the Resolve PR step to also retrieve and expose the
resolved base SHA, alongside head_sha. In the changed-file detection flow,
replace the unpinned gh pr diff invocation with a comparison explicitly using
the stored base and head SHAs, ensuring file detection matches the checkout
selected by Resolve PR.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 799fe662-c448-4c2a-8fdd-ed2314e01fb5
📒 Files selected for processing (2)
.github/workflows/story-screenshots.ymldocs/docs/guides/getting-started/ai-development.md
`-f pr=<number>` looks like a placeholder but a shell reads `<number>` as input redirection, so copying the line verbatim fails on a missing file rather than running the workflow. Use a concrete number and say to swap it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014se1ctqQjBCBaqjQRfHdzi
Preview DeploymentPreview URL: https://3aa57f20.bestax.pages.dev |
Three findings from the CodeRabbit review, posted outside the diff range. Pin the changed-file list to the resolved SHAs. "Resolve PR" pins head_sha and the checkout uses it, but the file list came from `gh pr diff <n>`, which re-reads the PR's current diff — so a push landing in between paired the new head's file list with the old head's checkout. The step comment already claimed the diff was pinned; now it is, via a three-dot compare of the recorded base and head. Survive losing the orphan-branch bootstrap race. Concurrency is per-PR, so two first-ever runs can both see the 404 and both bootstrap; the loser's createRef threw 422 and killed the job before the append retry loop, costing that PR its handoff comment. Adopt the winner's branch and fall through. Verified against a stubbed API: pre-fix the race throws and posts nothing, post-fix it commits and comments. Extend the failure notice to `publish`. It was watching `capture` only, but publish is the job that dies on a blob upload or a lost branch race, and its silence reads to the reviewer exactly the same. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014se1ctqQjBCBaqjQRfHdzi
|
Addressing the three outside-diff-range findings from the last CodeRabbit review. Two fixed in a48bc14, one refuted. ✅ ✅ While here I also widened the failure notice to watch ❌ The rule protects the code-writing loop from editing its own gates — There's also no privilege here to escalate into. Adding the guard would also break the case it's meant to serve: a PR touching both Happy to add the preflight if you'd rather have belt-and-braces here, but I don't think it's buying safety at the moment. Generated by Claude Code |
Preview DeploymentPreview URL: https://0f4d7ae6.bestax.pages.dev |
|
@coderabbitai full review Re-requesting because Generated by Claude Code |
|
✅ Action performedFull review finished. |
|
🎉 This PR is included in version 3.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.8.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary
Gives the human reviewer eyes at handoff (#284): when an
ai-loopPR flips toneeds-human-review, a newstory-screenshots.ymlworkflow captures the Storybook stories affected by the PR's changed files with Playwright — once light, once dark — and posts them to the PR as an upserted<!-- story-screenshots -->comment with inline images, plus a 30-day workflow artifact. The loop's reviewers are text-only; CSS bugs invisible to jsdom survived four review rounds on #257.How it works
bulma-ui/scripts/capture-stories.mjsmaps changed files → story IDs viastorybook-static/index.json:importPathfor a changed*.stories.tsx,componentPath(index v5) for a changed component, basename heuristic for scss (_carousel.scss→Carousel.stories.tsx). Caps at 6 stories/file, 24 total (skipped IDs listed in the comment). Each story is screenshotted on a fresh page load per theme — an in-place toggle leaves mid-transition colors from the other theme in the picture.prefers-color-scheme, not justdata-theme: the preview loadsbulma.min.cssthenbulma-prefixed.min.css, and the prefixed file's media-scoped light:rootblock outranks the unprefixed[data-theme=dark]block in the cascade (equal specificity, later wins). The attribute alone changes nothing — verified empirically. ⚠ This means the existing smoke test'sSTORYBOOK_THEME=darkpass isn't actually rendering dark either; that's a separate fix outside this PR's scope.story-screenshots.ymlsplits privileges likevisual-regression.yml: thecapturejob runs PR code with a read-only token (reuses the CI run'sstorybookartifact for the pinned head SHA when present — at loop handoff CI is green by definition — and builds as fallback); thepublishjob holdscontents: write+pull-requests: write, runs no PR code, and treats everything from capture as untrusted (filename charset whitelist, HTML-escaped titles, env-var injection only).story-screenshotsunderpr-<n>/<run-id>/— run-id-unique paths so GitHub's Camo proxy never serves a stale cache. Deleting the branch only breaks images in old handoff comments; the next run re-bootstraps it.GITHUB_TOKENfire nolabeledruns (GitHub's recursion guard), butworkflow_dispatchvia API does. Thepull_request: [labeled]trigger covers a human applying the label by hand (same-repo guard;pull_request_targetdeliberately avoided since this builds PR code).Verified locally
Block.tsx+_carousel.scss) → 28 stories matched (componentPath + scss paths), capped to 12, all captured; light/dark PNGs eyeballed correct (including the extras' Carousel SCSS going dark).--max 2→ skipped list populated.Post-merge verification plan
needs-human-reviewto a trivial test PR (labeled path, artifact reuse) → comment renders images.gh workflow run story-screenshots.yml -f pr=<n>on the same PR → comment is edited in place, new run-id dir on the branch.story-screenshotsbranch, re-dispatch → bootstrap works.Closes #284
Summary by CodeRabbit