ci: fix the screenshot comment pipeline and post device render captures - #154
Conversation
📝 WalkthroughWalkthroughScreenshot CI now uploads variant-aware summaries, downloads emulator captures, publishes available screenshot artifacts to companion branches, and renders golden, Roborazzi diff, and device galleries in a resilient sticky pull-request comment. ChangesScreenshot CI and comment galleries
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ScreenshotTests as Screenshot tests
participant CommentWorkflow as Comment workflow
participant CompanionBranch as Companion branch
participant StickyComment as Sticky comment
ScreenshotTests->>CommentWorkflow: upload results-summary.json and screenshots
CommentWorkflow->>CommentWorkflow: detect diff, device, and golden states
CommentWorkflow->>CompanionBranch: publish available PNGs
CommentWorkflow->>StickyComment: append summary and available galleries
Possibly related PRs
Suggested reviewers: 🚥 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 |
Snapshot diff report✅ 8 screenshots verified, no changes.
|
![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
🧹 Nitpick comments (1)
.github/workflows/screenshot-comment.yml (1)
157-157: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuezizmor template-injection here is a false positive — but move it to env for a clean scan.
${{ steps.pr.outputs.number }}is regex-guarded to^[0-9]+$at Line 54, so nothing attacker-controllable can reach this expansion. Not exploitable. Optional: reference thePR_NUMBERviaenv:(as the push/gallery steps already do withBRANCH_NAME) so zizmor stops flagging it and the guard survives future edits.🔒 Optional: pass PR number via env
env: BRANCH_NAME: companion_pr-${{ steps.pr.outputs.number }} + PR_NUMBER: ${{ steps.pr.outputs.number }} run: | @@ - git commit -m "Add screenshots for PR #${{ steps.pr.outputs.number }}" + git commit -m "Add screenshots for PR #${PR_NUMBER}"🤖 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/screenshot-comment.yml at line 157, Update the screenshot commit step to pass the validated PR number through an env variable such as PR_NUMBER, then reference that variable in the git commit message instead of interpolating steps.pr.outputs.number directly. Preserve the existing numeric validation and commit-message content.Source: Linters/SAST tools
🤖 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 @.github/workflows/screenshot-comment.yml:
- Around line 106-131: Update the Check for images to publish step’s diff and
device detection predicates to match the gallery filters exactly: only accept
Roborazzi files under roborazzi/ for diff images, and only accept
device-screenshots files matching band_api<SDK>_<case>_{actual,expected}.png for
device images. Keep the existing safe-path validation and GITHUB_OUTPUT values,
but ensure check.diff and check.device cannot become true for files the gallery
later excludes.
---
Nitpick comments:
In @.github/workflows/screenshot-comment.yml:
- Line 157: Update the screenshot commit step to pass the validated PR number
through an env variable such as PR_NUMBER, then reference that variable in the
git commit message instead of interpolating steps.pr.outputs.number directly.
Preserve the existing numeric validation and commit-message content.
🪄 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
Run ID: 67527b17-68d4-4e93-bc4e-f892b8788587
📒 Files selected for processing (2)
.github/workflows/screenshot-comment.yml.github/workflows/screenshot-test.yml
| - id: check | ||
| name: Check for diff images | ||
| name: Check for images to publish | ||
| shell: bash | ||
| run: | | ||
| mapfile -t files < <(find . -type f -name "*_compare.png") | ||
| exist="false" | ||
| for f in "${files[@]}"; do | ||
| # Roborazzi diff images (only exist on a mismatch). | ||
| diff="false" | ||
| while IFS= read -r f; do | ||
| # Reject any path with characters outside the safe set before we touch git / build URLs. | ||
| if [[ "$f" =~ ^[a-zA-Z0-9_./-]+$ ]]; then | ||
| exist="true" | ||
| break | ||
| fi | ||
| done | ||
| echo "exist=$exist" >> "$GITHUB_OUTPUT" | ||
| [[ "$f" =~ ^[a-zA-Z0-9_./-]+$ ]] && { diff="true"; break; } | ||
| done < <(find . -type f -name "*_compare.png") | ||
|
|
||
| # Device render captures (band_api<SDK>_<case>_{actual,expected}.png) under device-screenshots/. | ||
| device="false" | ||
| if [[ -d device-screenshots ]]; then | ||
| while IFS= read -r f; do | ||
| [[ "$f" =~ ^[a-zA-Z0-9_./-]+$ ]] && { device="true"; break; } | ||
| done < <(find device-screenshots -type f -name "*.png") | ||
| fi | ||
|
|
||
| # --- Push diff images to an orphan companion branch (only when there are diffs) --- | ||
| any="false" | ||
| [[ "$diff" == "true" || "$device" == "true" ]] && any="true" | ||
| { | ||
| echo "diff=$diff" | ||
| echo "device=$device" | ||
| echo "any=$any" | ||
| } >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Confirm the diff gate and the diff gallery operate on the same file set.
rg -n -e '_compare\.png' -e 'roborazzi/' -e 'device-screenshots' .github/workflows/screenshot-comment.yml
# 2) Confirm Roborazzi actually nests *_compare.png under a 'roborazzi/' path segment
# (otherwise `grep roborazzi/` in the gallery never matches).
rg -n --iglob '*.gradle*' -e 'roborazzi' -e 'outputDir' -e 'compareOutputDir' -e 'resultsDir'
# 3) Show how device captures are named to validate the regex contract with PR `#152`.
rg -n -e 'band_api' -e 'device-screenshots' -g '!**/build/**'Repository: skydoves/Cloudy
Length of output: 1422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant workflow sections around the gate and render logic.
sed -n '100,320p' .github/workflows/screenshot-comment.yml | cat -n
# Show the exact snippets that decide whether rows are emitted for each gallery.
rg -n -A6 -B6 'grep "roborazzi/"|device_gallery|band_api\([0-9]+\)_\(.+\)_(actual|expected)' .github/workflows/screenshot-comment.ymlRepository: skydoves/Cloudy
Length of output: 15902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Roborazzi configuration and screenshot producers.
git ls-files | rg -n '(^|/)(build\.gradle(\.kts)?|settings\.gradle(\.kts)?|.*(gradle|kts|kt|java|xml|yml|yaml)$)' >/tmp/allfiles.txt || true
rg -n --glob '!**/build/**' -e 'roborazzi' -e 'compareOutputDir' -e 'outputDir' -e 'device-screenshots' -e 'band_api' .
# Narrow to likely producer/test files if present.
fd -H -I '.*(Roborazzi|Screenshot|MirageBand).*' .Repository: skydoves/Cloudy
Length of output: 3166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the Roborazzi setup and screenshot test file names.
sed -n '240,340p' cloudy/build.gradle.kts | cat -n
printf '\n---\n'
sed -n '1,220p' cloudy/src/androidHostTest/kotlin/com/skydoves/cloudy/ScreenshotTestSupport.kt | cat -n
printf '\n---\n'
sed -n '1,220p' cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/LiquidGlassScreenshotTest.kt | cat -n
printf '\n---\n'
sed -n '1,220p' cloudy/src/androidDeviceTest/kotlin/com/skydoves/cloudy/SkyBackdropScreenshotTest.kt | cat -nRepository: skydoves/Cloudy
Length of output: 22772
🌐 Web query:
Roborazzi default compare output directory roborazzi compare png path
💡 Result:
The default output directory for Roborazzi—used for reference images and comparison artifacts—is module/build/outputs/roborazzi [1][2]. You can customize where these files are generated using the following methods: 1. Gradle Configuration: You can set the global output directory in your build.gradle file using the roborazzi.outputDir parameter [3]: roborazzi { outputDir = "src/your/screenshot/folder" } 2. Comparison-Specific Output: For specifically managing the output directory of comparison images (e.g., _compare.png and _actual.png files), you can use the roborazzi.compare.outputDir property (introduced in version 1.36.0) or the outputDirectoryPath option within CompareOptions [4][5]. Note that Roborazzi distinguishes between where reference (golden) images are stored and where comparison artifacts are generated [4]. If you need to separate your workflow (e.g., recording vs. verification), you can dynamically adjust these paths in your Gradle configuration based on the task being executed [5].
Citations:
- 1: https://github.com/takahirom/roborazzi/blob/main/README.md
- 2: https://github.com/takahirom/roborazzi
- 3: https://github.com/takahirom/roborazzi/blob/main/docs/topics/faq.md
- 4: Android unit tests in compose multiplatform project don't capture dialogs takahirom/roborazzi#245
- 5: Having separate directories for recording vs verification takahirom/roborazzi#471
🌐 Web query:
site:github.com takahirom roborazzi compareOutputDir roborazzi png path default
💡 Result:
In Roborazzi, the default directory for both reference images and comparison results is module/build/outputs/roborazzi [1][2]. Regarding your query about the output path, note that Roborazzi distinguishes between how it handles recorded images (golden snapshots) and comparison results (diffs/actual images). 1. Default Path: By default, screenshots captured via captureRoboImage are often saved relative to the current module directory (e.g., module/image.png), though this depends on your configured filePathStrategy [2][3]. The library's internal comparison artifacts are stored in the module/build/outputs/roborazzi directory [1][2]. 2. Customizing Paths: - To customize the global output directory for Roborazzi (which affects where comparison files and potentially recorded images are stored, depending on your strategy), you can configure it in your build.gradle file: roborazzi { outputDir = file("src/your/custom/path") } [4][2] - To control where recorded images are saved, you can set the file path strategy in your gradle.properties file: roborazzi.record.filePathStrategy=relativePathFromRoborazziContextOutputDirectory [2] When this strategy is enabled, images are saved in the directory specified by roborazzi.outputDir instead of the default module root [2]. 3. Comparison Options: While there is an outputDirectoryPath parameter available within CompareOptions, it is specifically intended for managing where comparison-related output files are generated, rather than overriding the primary golden snapshot path for regular capture methods [5].
Citations:
- 1: Having separate directories for recording vs verification takahirom/roborazzi#471
- 2: https://github.com/takahirom/roborazzi?tab=readme-ov-file
- 3: https://github.com/takahirom/roborazzi
- 4: https://github.com/takahirom/roborazzi/blob/main/docs/topics/faq.md
- 5: Android unit tests in compose multiplatform project don't capture dialogs takahirom/roborazzi#245
Align the publish gate with the gallery filters. check.diff and check.device can turn true for files that the gallery steps later drop, so the comment still posts a table header with zero rows. Use the same path/filename predicates in both places (roborazzi/ for diff, band_api<SDK>_<case>_{actual,expected}.png for device).
🤖 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/screenshot-comment.yml around lines 106 - 131, Update the
Check for images to publish step’s diff and device detection predicates to match
the gallery filters exactly: only accept Roborazzi files under roborazzi/ for
diff images, and only accept device-screenshots files matching
band_api<SDK>_<case>_{actual,expected}.png for device images. Keep the existing
safe-path validation and GITHUB_OUTPUT values, but ensure check.diff and
check.device cannot become true for files the gallery later excludes.
Source: Linters/SAST tools
1 participant
Problem
The screenshot comment workflow has failed on every run since it landed (see the run history of
screenshot-comment.yml). Two independent causes:screenshot-test.ymluploads the summary fromcloudy/build/test-results/roborazzi/debug/results-summary.json, but the KMP Android host test writes it under.../roborazzi/androidHostTest/. Withif-no-files-found: warn, thescreenshot-summaryartifact silently never exists.screenshot-comment.ymlhas a fallback body for a missing summary, but the line above it,summary_json="$(find screenshot-summary ... | head -n1)", exits 1 underset -e -o pipefailwhen the directory is absent, killing the step before the fallback can run. That is the message-lessexit code 1in every failed run.Fix
roborazzi/**/results-summary.json) so a task rename cannot silently drop the artifact again.|| trueto thefindso a missing artifact takes the existing "summary unavailable" path instead of killing the step.Also included: always-on golden gallery
The comment now inlines the committed reference screenshots (
cloudy/src/androidHostTest/assets/screenshots/*.png) on every run, not just on a regression.verifyRoborazziAndroidHostTestwrites no images on a clean pass, so previously a clean PR comment had nothing visual to show; the producer now uploads the checked-in goldens as ascreenshot-goldensartifact and the comment renders them as a "Screenshots" table via the companion branch.Also included: device render gallery
The comment workflow gains a "Device rendering" section that posts per-band emulator captures (
band_api<SDK>_<case>_{actual,expected}.png) fromdevice-screenshots-api*artifacts, next to the existing Roborazzi diff gallery. The section skips cleanly when no such artifacts exist, so runs frommainand unrelated PRs are unaffected.The producer side (an emulator matrix job and the instrumented test that writes those captures) lives in #152. Since
workflow_runworkflows always execute the default-branch file, that gallery can only take effect once this lands onmain; #152 then exercises it end to end. This also fixes theSC2044find-in-for-loop actionlint warning in the push step.Summary by CodeRabbit
New Features
Bug Fixes