Repository navigation
fix: one predicate decides "are we on CI" - #310
Merged
Merged
Conversation
Contributor
Reviewer's GuideThe PR makes CI detection consistent by introducing and adopting Flow diagram for unified CI detectionflowchart TD
CI[CI environment variable] --> P[SnapDiff::Config.ci?]
P --> R[Config fail_if_new]
P --> M[ScreenshotMatcher record safeguards]
P --> H[HTML reporter embed_images]
E[CI unset or empty] --> P
N[CI non-empty] --> P
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit details
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/snap_diff/config.rb" line_range="107" />
<code_context>
+ # Are we on CI? Read live, and an empty CI is not CI: `CI: ""` is how a
+ # job opts out (docs/ci-integration.md), so every reader must agree.
+ def self.ci?
+ !ENV["CI"].to_s.empty?
+ end
+
</code_context>
<issue_to_address>
**Sample reports embed opted-out images**
When `CI` is set to an empty string and the sample-report generator runs, `scripts/generate_sample_report.rb` uses `!!ENV["CI"]` instead of the shared predicate, so its HTML reporter embeds every image and produces a data-URI report.
Use `SnapDiff::Config.ci?` when selecting image embedding in the sample-report generator.
Also at `lib/snap_diff/reporters/html.rb:223`.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: lib/snap_diff/config.rb:107
…g it Three readers tested ENV["CI"] and two of them disagreed with the third. The record-mode default and the `:all` refusal treated an empty value as "not CI" (`.to_s.empty?`), which is what the documented update-baselines job relies on when it sets `CI: ""` to opt out. The HTML reporter's auto-registration used `!!ENV["CI"]`, so that same job still got a report with every image inlined as a data URI. `SnapDiff::Config.ci?` is now the single reader -- unset or empty is not CI -- and the three call sites use it. Config is the require-graph leaf the other two already load, so no new dependency edge. Same theme, in the docs: the README and docs/configuration.md snippet for re-recording was `record = ENV["ACCEPT_SCREENSHOTS"] ? :all : :once`. On every ordinary run that sets an explicit `:once`, and an explicit mode outranks the `:none`-under-CI default (Config#record reads `@record || ...`), so a CI that copied the snippet stopped failing on a screenshot with no committed baseline. The snippet now assigns `:all` only when the variable is non-empty -- the same "empty is not set" rule `Config.ci?` applies -- and leaves the default alone otherwise. Not changed: what CI switches on (the `:none` default, image embedding, the `:all` refusal), only that they all read the same answer.
oboxodo
force-pushed
the
fix/one-ci-predicate
branch
from
October 17, 2026 14:20
d678c3e to
7e01feb
Compare
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
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.
What changes
SnapDiff::Config.ci?(!ENV["CI"].to_s.empty?) replaces the three rawENV["CI"]reads inconfig.rb,screenshot_matcher.rbandreporters/html.rb. The HTML reporter used!!ENV["CI"], soCI=""embedded every image as a data URI while the record-mode default treated the same value as "not CI".ACCEPT_SCREENSHOTSsnippet in README,docs/configuration.mdanddocs/ci-integration.mdbecomesSnapDiff.config.record = :all unless ENV["ACCEPT_SCREENSHOTS"].to_s.empty?. The previous form (ENV[...] ? :all : :once) set an explicit:onceon every normal run, and an explicit mode outranks the:none-under-CI default, so a CI run with that config no longer failed on missing baselines.Why merge
One predicate, one meaning of "on CI", in code and in the documented config. Both items are one-liners with a focused test.
Drawbacks
CI=""to get embedded images changes behaviour (relative paths instead). SetCI=1or passembed_images: trueexplicitly.Evidence
bin/rake test:unit: master 812 runs; this branch 813, 0 failures. Full suite in the CI image: 856 runs, 0 failures.CI="",HTML.newdefault →embed_imageswastrue, nowfalse. Default record mode underCI=1with the new snippet and no variable set →:none(was:oncewith the old snippet).Summary by Sourcery
Use a single consistent CI predicate and correct the documented screenshot acceptance configuration.
Bug Fixes:
Enhancements:
Documentation:
Tests:
The PR appears safe to merge; no actionable issues were found.Summary
Centralizes CI detection in
SnapDiff::Config.ci?and fixes the screenshot acceptance examples.No actionable issues found. The previous review commit is not an ancestor of the current head, so this review used the full PR diff. No previous review threads were supplied.
Reviews (2) · Last reviewed commit: "fix: one predicate decides "are we on CI..." · Reviewed by Greptile