chore(backstop): add uniq report ids, add all theme images - #8364
Merged
Conversation
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (299)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR makes backstop's exported configuration ID dynamic based on active theme CLI flags ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Collaborator
|
Preview: https://pf-pr-8364.surge.sh A11y report: https://pf-pr-8364-a11y.surge.sh |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backstop.js (1)
11-13: 💤 Low value
idPrefixis a redundant single-use variable.
idPrefixis only ever used once as the first array element. Inline it for less noise.♻️ Proposed simplification
-const idPrefix = 'pf-core'; const backstopId = [ - idPrefix, + 'pf-core', isFelt ? 'felt' : 'default',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backstop.js` around lines 11 - 13, The idPrefix variable is redundant: remove the const idPrefix = 'pf-core' declaration and inline the string directly in the backstopId array (replace idPrefix with 'pf-core' as the first element), then delete the now-unused idPrefix identifier to reduce noise and keep backstopId as the sole declaration referencing that value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backstop.js`:
- Around line 11-18: The change to backstopId (constructed from idPrefix and
flags in the backstopId array using idPrefix, isFelt, isDark, isGlass,
isHighContrast) alters the default variant id from "pf-core" to
"pf-core-default", orphaning existing reference screenshots; either revert the
id change (remove the default branch so backstopId remains idPrefix for the
default case) or, if the new id is intentional, add a clear note to the
PR/README and run/regenerate Backstop references (backstop reference or backstop
approve) so the reference images are recreated to match the new backstopId
naming.
In `@package.json`:
- Line 22: The "backstop:test:all" npm script currently chains many
backstop:test variants with semicolons which hides intermediate failures; update
the script so failures propagate by either replacing the semicolon chain with a
failure-aware chain (use && between commands) or, preferably, use npm-run-all
(e.g., npm-run-all -s backstop:test backstop:test:dark ... ) to run them
sequentially and fail fast; ensure you update the "backstop:test:all" script
entry to reference the exact names of the constituent scripts (backstop:test,
backstop:test:dark, backstop:test:glass, backstop:test:hc,
backstop:test:dark-hc, backstop:test:dark-glass, backstop:test:felt,
backstop:test:felt-dark, backstop:test:felt-glass, backstop:test:felt-hc,
backstop:test:felt-dark-hc, backstop:test:felt-dark-glass) and add npm-run-all
as a devDependency if choosing that approach.
---
Nitpick comments:
In `@backstop.js`:
- Around line 11-13: The idPrefix variable is redundant: remove the const
idPrefix = 'pf-core' declaration and inline the string directly in the
backstopId array (replace idPrefix with 'pf-core' as the first element), then
delete the now-unused idPrefix identifier to reduce noise and keep backstopId as
the sole declaration referencing that value.
🪄 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: d4660023-2390-4f27-8d3a-b18fbaceb7df
📒 Files selected for processing (2)
backstop.jspackage.json
Comment on lines
+11
to
+18
| const idPrefix = 'pf-core'; | ||
| const backstopId = [ | ||
| idPrefix, | ||
| isFelt ? 'felt' : 'default', | ||
| ...(isDark ? ['dark'] : []), | ||
| ...(isGlass ? ['glass'] : []), | ||
| ...(isHighContrast ? ['high-contrast'] : []) | ||
| ].join('-'); |
Contributor
There was a problem hiding this comment.
id change for the default variant orphans existing reference screenshots.
BackstopJS's id field is used for screenshot naming — the id is embedded as a prefix in each screenshot filename within the bitmaps_reference and bitmaps_test directories. Changing the default-run id from pf-core to pf-core-default means any previously stored reference images (named pf-core_…) will no longer match new test screenshots (named pf-core-default_…), so the default variant's reference set must be regenerated via backstop reference or backstop approve after this change lands. If that's part of the intended workflow for this PR, no code change is needed — just worth calling out explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backstop.js` around lines 11 - 18, The change to backstopId (constructed from
idPrefix and flags in the backstopId array using idPrefix, isFelt, isDark,
isGlass, isHighContrast) alters the default variant id from "pf-core" to
"pf-core-default", orphaning existing reference screenshots; either revert the
id change (remove the default branch so backstopId remains idPrefix for the
default case) or, if the new id is intentional, add a clear note to the
PR/README and run/regenerate Backstop references (backstop reference or backstop
approve) so the reference images are recreated to match the new backstopId
naming.
| "backstop:test:felt-hc": "backstop test --config='backstop.js' --felt --high-contrast", | ||
| "backstop:test:felt-dark-hc": "backstop test --config='backstop.js' --felt --dark --high-contrast", | ||
| "backstop:test:felt-dark-glass": "backstop test --config='backstop.js' --felt --dark --glass", | ||
| "backstop:test:all": "npm run backstop:test; npm run backstop:test:dark; npm run backstop:test:glass; npm run backstop:test:hc; npm run backstop:test:dark-hc; npm run backstop:test:dark-glass; npm run backstop:test:felt; npm run backstop:test:felt-dark; npm run backstop:test:felt-glass; npm run backstop:test:felt-hc; npm run backstop:test:felt-dark-hc; npm run backstop:test:felt-dark-glass", |
Contributor
There was a problem hiding this comment.
; chaining silently swallows intermediate failures — overall exit code reflects only the last run.
With ;, if backstop:test (the first variant) fails but backstop:test:felt-dark-glass (the last) passes, npm run backstop:test:all exits 0, masking the failure. This is harmless for purely local/manual use where terminal output is inspected, but would produce false-green results in CI.
If CI integration is planned, consider npm-run-all (already a common addition in projects like this) to sequence commands and propagate any failure:
♻️ Alternative using npm-run-all (if added as a dev dep)
-"backstop:test:all": "npm run backstop:test; npm run backstop:test:dark; ...",
+"backstop:test:all": "npm-run-all backstop:test backstop:test:dark backstop:test:glass backstop:test:hc backstop:test:dark-hc backstop:test:dark-glass backstop:test:felt backstop:test:felt-dark backstop:test:felt-glass backstop:test:felt-hc backstop:test:felt-dark-hc backstop:test:felt-dark-glass",-"backstop:approve:all": "npm run backstop:approve; npm run backstop:approve:dark; ...",
+"backstop:approve:all": "npm-run-all backstop:approve backstop:approve:dark backstop:approve:glass backstop:approve:hc backstop:approve:dark-hc backstop:approve:dark-glass backstop:approve:felt backstop:approve:felt-dark backstop:approve:felt-glass backstop:approve:felt-hc backstop:approve:felt-dark-hc backstop:approve:felt-dark-glass",Also applies to: 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 22, The "backstop:test:all" npm script currently chains
many backstop:test variants with semicolons which hides intermediate failures;
update the script so failures propagate by either replacing the semicolon chain
with a failure-aware chain (use && between commands) or, preferably, use
npm-run-all (e.g., npm-run-all -s backstop:test backstop:test:dark ... ) to run
them sequentially and fail fast; ensure you update the "backstop:test:all"
script entry to reference the exact names of the constituent scripts
(backstop:test, backstop:test:dark, backstop:test:glass, backstop:test:hc,
backstop:test:dark-hc, backstop:test:dark-glass, backstop:test:felt,
backstop:test:felt-dark, backstop:test:felt-glass, backstop:test:felt-hc,
backstop:test:felt-dark-hc, backstop:test:felt-dark-glass) and add npm-run-all
as a devDependency if choosing that approach.
Collaborator
|
🎉 This PR is included in version 6.5.0-prerelease.82 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #8363
"files changed" tab is unusable because of the number of image/file changes.
The first two commits are code changes, the rest are file changes I've reviewed with @jcmill
Commit 1 - 4728132
backstop:test:allandbackstop:approve:allscripts in package.json to kick off and approve all tests with a single command.caffeinatebefore the command if you intend on stepping away at any point so that your computer doesn't go to sleep, which will cause the active test to failCommit 2 - 24046ed
&&with;between the tests)Summary by CodeRabbit
New Features
backstop:test:allandbackstop:approve:all) to run all theme test variants sequentially