[#2863] Added opt-in for visual regression on non-PR deployments. - #2864
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughDiffy visual regression gating now supports opted-in non-PR branches, glob-based matching, safer PR metadata handling, and updated documentation for execution paths and result reporting. ChangesVisual regression flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Deployment
participant DiffyWorkflow
participant GitHub
Deployment->>DiffyWorkflow: Dispatch visual regression workflow
DiffyWorkflow->>GitHub: Resolve PR metadata when available
GitHub-->>DiffyWorkflow: Return PR data or no PR
DiffyWorkflow->>DiffyWorkflow: Apply PR label, auto-branch, or non-PR branch gating
DiffyWorkflow->>GitHub: Publish PR comment or workflow summary
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/test-vr.yml:
- Around line 307-319: Gate the GITHUB_STEP_SUMMARY report block on an empty
resolved PR number so it runs only when no PR comment will be posted. Preserve
the existing summary contents and non-PR reporting behavior, using the
workflow’s resolved PR number variable.
In @.vortex/docs/content/development/visual-regression.mdx:
- Around line 145-157: Update the visual regression flow diagram so the PR
deployment path also leads from vr-report to run summary, while preserving the
existing PR comment step for that path. Ensure both PR-resolved and
no-PR-resolved paths show the unconditional run-summary output described in the
Jobs section.
🪄 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
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 24e9b714-5f8b-442c-9cda-0e5beaaab944
⛔ Files ignored due to path filters (1)
.vortex/installer/tests/Fixtures/handler_process/visual_regression_enabled/.github/workflows/test-vr.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (2)
.github/workflows/test-vr.yml.vortex/docs/content/development/visual-regression.mdx
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2864 +/- ##
==========================================
- Coverage 86.81% 86.38% -0.43%
==========================================
Files 100 93 -7
Lines 4846 4687 -159
Branches 47 3 -44
==========================================
- Hits 4207 4049 -158
+ Misses 639 638 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
📖 Documentation preview for this pull request has been deployed to Netlify: https://6a69d54e598381806734e2a9--vortex-docs.netlify.app This preview is rebuilt on every commit and is not the production documentation site. |
Added the 'VR_DIFFY_NON_PR_BRANCHES' repository variable so deployments without a pull request environment - release branches, integration branches, permanent environments - can run a Diffy comparison when the deployed branch matches. Empty by default, preserving the current skip. Extracted the glob matching into a 'ref_matches' helper shared by the PR head-branch bypass and the new branch opt-in, reported the result to the workflow run summary for runs that have no pull request to comment on, and skipped instead of failing when a resolved PR number cannot be read.
A pattern list written as 'release/*, develop' left the extra spaces in place, so the pattern never matched.
github-actions
Bot
added
the
CONFLICT
…ult. The flow diagrams showed every run ending in a PR comment, and the manual entry point showed a 'vr-report' job that never runs without a resolved PR.
…low diagram. The entry box implied the branch opt-in was evaluated before the deployment, when the workflow evaluates it after the dispatch.
…elpers. Recording the decision and ending the step was written out at all seven exit points, so a change to the step's output contract had to be repeated seven times.
2e68f70 to
7c08efd
Compare
github-actions
Bot
removed
the
CONFLICT
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test-vr.yml (1)
333-337: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a random delimiter for the
$GITHUB_OUTPUTheredoc.The multi-line output uses a static
VR_SUMMARY_EOFdelimiter while the body interpolatesTARGET_URLandSHARED_URL(Diffy's shared report URL). If either value ever contains a line equal toVR_SUMMARY_EOF, subsequent lines get parsed as arbitrary$GITHUB_OUTPUTkey=value pairs — including overridingpr_number, whichvr-reportlater uses to pick the PR to comment on. Low likelihood today, but the fix is trivial.♻️ Proposed fix
+ delimiter="vr_summary_$(date +%s%N)_${RANDOM}" { - echo "summary<<VR_SUMMARY_EOF" + echo "summary<<${delimiter}" printf '%s\n' "${summary}" - echo "VR_SUMMARY_EOF" + echo "${delimiter}" } >> "$GITHUB_OUTPUT"🤖 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/test-vr.yml around lines 333 - 337, Update the multi-line GITHUB_OUTPUT block around the summary output to generate and use a per-run random heredoc delimiter instead of the static VR_SUMMARY_EOF value. Ensure the same delimiter wraps the printf output and cannot collide with interpolated TARGET_URL or SHARED_URL content, while preserving the existing summary output name and formatting.
🤖 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.
Outside diff comments:
In @.github/workflows/test-vr.yml:
- Around line 333-337: Update the multi-line GITHUB_OUTPUT block around the
summary output to generate and use a per-run random heredoc delimiter instead of
the static VR_SUMMARY_EOF value. Ensure the same delimiter wraps the printf
output and cannot collide with interpolated TARGET_URL or SHARED_URL content,
while preserving the existing summary output name and formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1b7a8447-5276-411e-8cf7-488664c3d267
⛔ Files ignored due to path filters (1)
.vortex/installer/tests/Fixtures/handler_process/visual_regression_enabled/.github/workflows/test-vr.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (2)
.github/workflows/test-vr.yml.vortex/docs/content/development/visual-regression.mdx
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AlexSkrypnyk
added
the
Needs review
Named the new variable 'VR_DIFFY_BRANCHES' rather than describing it by what it is not, and renamed 'VR_DIFFY_AUTO_BRANCHES' to 'VR_DIFFY_PR_SKIP_BRANCHES' so each name states which gate it controls. Collapsed the gate to roughly half its height: single-line helpers, guard clauses as AND lists, splitting the pattern list on whitespace as well as commas instead of trimming each pattern by hand, and a case-insensitive fixed-string 'grep' in place of lowercasing both sides of the label comparison. 'VR_DIFFY_AUTO_BRANCHES' is an existing repository variable, so a consumer that has set it falls back to the 'deps/*' default until it is renamed.
|
Code coverage (threshold: 90%) Per-class coverage |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
|
Code coverage (threshold: 90%) Per-class coverage |
1 participant
Closes #2863
Summary
The gate step in
.github/workflows/test-vr.ymlexited as soon as the dispatchedtarget_urlhad nopr-<number>segment, so a release branch or any other permanent-environment deployment could never trigger a Diffy comparison automatically - only a manualworkflow_dispatchwith a hand-pasted URL could. This adds an opt-in repository variable,VR_DIFFY_BRANCHES, holding a comma-separated glob list of deployed branch names. When a dispatch resolves no PR, the gate matches the deployed branch against that list before deciding to skip. The branch is already carried in theclient_payload.branchfield thatvortex-notify-diffysends, so the check costs no extra API call. The variable is empty by default, which preserves today's behaviour exactly.Breaking change
VR_DIFFY_AUTO_BRANCHESis renamed toVR_DIFFY_PR_SKIP_BRANCHES, so each variable now states which gate it controls:VR_DIFFY_PR_SKIP_BRANCHESlists PR head branches that skip the label check, andVR_DIFFY_BRANCHESlists deployed branches that run without a PR. A consumer that has set the old repository variable falls back to thedeps/*default until it is renamed - same value in the common case, but it needs calling out in the release notes.Changes
Gate
DIFFY_BRANCHES(fromvars.VR_DIFFY_BRANCHES, empty by default) andBRANCH(fromclient_payload.branch) to the workflow-levelenvblock, so neither value is interpolated into arun:script.::notice::as before.ref_matcheshelper, reused by both the new opt-in check and theVR_DIFFY_PR_SKIP_BRANCHESPR head-branch check, which previously carried the only copy of that loop inline. The list is split on whitespace as well as commas, so padding around a pattern is absorbed rather than trimmed by hand.proceedandskiphelpers for the "record the decision and end the step" pair that was written out at all seven exit points, and collapsed the guards into AND lists. The step is roughly half its previous height, and each decision is now a single line.grep, which also removes the risk of a label containing regex metacharacters being treated as a pattern.gh pr viewlookup with an exit-status check. Apr-<number>segment that does not resolve to a real pull request now skips the run with a::notice::instead of failing thevr-comparecheck underset -euo pipefail- a comparison that should not run must not present as a red check.Gate run on PR label or opted-in branch, and downgraded the "no PR pattern found" line from::notice::to a plainechosince it is no longer a terminal outcome.Documentation
VR_DIFFY_BRANCHESin the variables table, renamedVR_DIFFY_AUTO_BRANCHESthroughout, and added aRelease and other non-PR deploymentssection with a run/no-run decision table.release/26.7.5to a host containingrelease-26-7-5, and the gate compares against the original branch name.vr-reportruns only when a PR was resolved.How the PR is resolvedandCosts and quotas, the latter warning that*compares on every deployment to every permanent environment.Fixtures
visual_regression_enabledinstaller fixture viaahoy update-snapshots.Verification
The gate is inline shell inside YAML with no test harness in this repository, so the decision table was exercised locally by running the step verbatim against a mocked
gh: 19 cases covering manual dispatch, the four PR paths (label, lowercase label, no label,deps/*skip), a failed PR lookup, a mixed-case configured label, and nine no-PR cases including the empty default, the*wildcard, multi-pattern lists, space- and tab-padded lists, an all-empty list, and the sanitized-hostname trap.actionlint(the pinned CI image and ignore list) andzizmorboth pass.Before / After
Summary by CodeRabbit
New Features
Bug Fixes
Documentation