Repository navigation
Conversation
Contributor
Reviewer's GuideThe PR makes new screenshots first-class reporting results: matcher and reporting state now retain immutable capture metadata across modes and fork workers, the HTML report lists available new captures with shell-safe baseline commands and matching counts, disappeared-baseline races are classified correctly, and extensive tests and documentation cover the behavior. Sequence diagram for reporting a new screenshotsequenceDiagram
participant Matcher as ScreenshotMatcher
participant Reporting as SnapDiff::Reporting
participant HTML as HTMLReporter
participant Browser as HTMLReport
Matcher->>Matcher: capture screenshot
Matcher->>Reporting: record_missing_baseline(name, path: snapshot.path)
Reporting-->>Matcher: first sighting
Matcher->>Reporting: finalize
HTML->>Reporting: uncompared
Reporting-->>HTML: frozen name, path, reason
HTML->>HTML: new_screenshot_entries
HTML->>Browser: write_report with capture and git add command
Flow diagram for new versus disappeared baseline handlingflowchart TD
Capture["Capture screenshot"] --> Baseline{Committed baseline available?}
Baseline -->|Yes| Compare["Compare screenshot"]
Baseline -->|No| CheckedOut{Baseline was checked out earlier?}
CheckedOut -->|Yes| Disappeared["Report disappeared-baseline error"]
CheckedOut -->|No| Record["Record uncompared screenshot\nreason: new"]
Record --> Mode{record mode}
Mode -->|:none| Fail["Fail test with add command"]
Mode -->|:once or other| ReportNew["Count and report new screenshot"]
Compare --> Summary["Finalize counts and report"]
Fail --> Summary
ReportNew --> Summary
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 41 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 11 files. (6 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary change: adding new screenshots to the HTML report. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 11 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/snapdiff.md" line_range="261" />
<code_context>
A reporter that raises is warned about and skipped — the other reporters still run.
+`record` only receives screenshots that were compared. The ones that were not — the summary
+line's "new" and "re-recorded" — are in `SnapDiff::Reporting.uncompared`, complete by the time
+`finalize` runs (fork-parallel workers included):
</code_context>
<issue_to_address>
**Uncompared assertions still reach reporters**
When a custom reporter is written based on the documented guarantee and receives a test assertion with no comparison, a custom reporter following this statement can assume every assertion has a comparison and dereference `assertion.compare`; `Reporting.notify` passes the full assertion list to `record`, including assertions whose comparison is nil, so the reporter raises and its handling of that test is skipped.
Clarify that `record` receives all assertions, including those with a nil comparison, and that `uncompared` supplies the separate captured-path and reason data.
Also at `docs/reporters.md:133`.
</issue_to_address>
### Comment 2
<location path="lib/snap_diff/reporters/html.rb" line_range="185" />
<code_context>
+ image = resolve_image(path)
+ return entry unless image
+
+ entry.merge(new: image, git_add: "git add -- #{Shellwords.escape(display_path(path))}")
+ end
+
</code_context>
<issue_to_address>
**Add command fails in Windows shells**
When a Windows user copies the suggested command for a screenshot path containing spaces into PowerShell or `cmd.exe`, `Shellwords.escape` emits POSIX shell escaping, so a path with spaces is rendered with backslash escapes; PowerShell and `cmd.exe` do not treat those escapes as quoting, and the suggested `git add` command fails for that capture.
Generate a command quoted for the supported shell, or clearly generate and label a POSIX-shell command rather than presenting it as shell-independent.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: docs/snapdiff.md:261, lib/snap_diff/reporters/html.rb:185
A screenshot captured with no committed baseline was only counted by name for the summary line; reporters receive compared assertions only. So the report left it out, a run whose only finding was a new screenshot wrote no report at all, and under record: :none (CI's default) the raise came before the tally, so the summary said `0 new` while the capture sat on disk. Reporting's tallies of new and re-recorded screenshots now keep each one's captured path, and SnapDiff::Reporting.uncompared returns them as frozen Reporting::Uncompared values (name, path, reason :new or :rerecorded): the entries behind the summary line's counts, complete by the time reporters finalize. They travel with the counts in the fork-parallel fragment; the name lists every version writes stay, and a name only an older worker sent is still listed, with no capture. An entry with a reason no version writes is a bug and raises instead of being guessed at. The reporter contract (record/finalize/summary) is unchanged: any reporter, the bundled HTML one included, reads the list in finalize, so the report's `new` is the terminal's by construction, reset_run_totals! clears it with the counts, and no reporter can alter what another reads. The matcher now tallies a new screenshot once, after a successful capture and after the session's disappeared-baseline check (snap-diff#217), and warns from there; only "was a file already there" is still read before the capture. So a listed path is a file that was captured (snap-diff#260), a failed capture is not reported as new, and a baseline git handed over but a same-name test moved away is never new, in any mode: every mode raises the session's "disappeared" error. Before, record: :none told the user to `git add` a screenshot whose baseline was committed. The report builds the image and `git add -- <path>` only for a file on disk at render time; `--` keeps a name starting with a dash from reading as an option. In the page, a new screenshot goes through the existing views: the empty Baseline side explains itself with the existing .img-empty message and the command, which stays selectable (neither a click nor a zoomed-in drag on it toggles or pans). Badges and the `N new` pill use the palette's accent colour, neither pass nor fail. Re-recorded screenshots are counted in a pill when present but not listed; listing them with a before/after against the previous baseline is a separate follow-up. Not changed: the reporter contract and every reporter's state handoff; terminal output under :once and :all except that a failed capture is no longer announced as new; other failure messages; how failure entries render. Also fixes README's legacy `capybara_screenshot_diff/reporters/html` require. ScreenshotAssertion#display_path drops its `rescue ArgumentError`, like Reporters::Default's did: a path Pathname can't relate to the root is a misconfiguration to surface, not hide; the report's copy has none either. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
oboxodo
force-pushed
the
fix/report-new-screenshots
branch
from
October 15, 2026 15:13
0e1bbdd to
f7ab89e
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
1 participant
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.
Problem
Measured on 7bb159c with a real Minitest run, real git baselines, capture stubbed
(test/fixtures/summary_line_case.rb):
record: :once0 verified, 0 changed, 1 newnewlisted; pills0 failed · 0 passed · 0 total · 1 new:once1 verified, 1 changed, 1 newchangedonly;1 failed · 0 passed · 1 totalchanged, thennew;… · 1 newrecord: :none(CI)0 new, after1 new; test fails with the same messagenewlisted with its capture (embedded on CI)Screenshots
Real Minitest runs, the report opened in headless Chrome.
changedis listed; the new capture is missing.newis listed after the failures: the capture, and the command to accept it.CI (
record: :none), only a new screenshot: master writes no report; now the report is written, with the capture embedded:The command is selected with one click, even when zoomed in (here 200%):
Mechanism
SnapDiff::Reporting.uncomparedreturns them as frozen values (
name,path,reason:new/:rerecorded), complete byfinalize. The reporter contract is unchanged: any reporter reads it infinalize; the HTMLreporter does, so its
newis the terminal's. Documented in docs/snapdiff.md anddocs/reporters.md.
disappeared-baseline check (Fiber and thread safety: remaining decisions and hardening after the v2 core work #217); image +
git add -- <root-relative, shell-escaped path>only for a file on disk at render time (The new-screenshot error prints a path that will never exist in CI #260).
uncomparedkey, read with a default; the name lists stay), so the parent's list is the whole run's.
.img-emptymessage with thecommand (one click selects it; clicking it no longer toggles Annotated);
newbadges andpill in
--accent.Not changed
The reporter contract (
record/finalize/summary) and reporters' state handoff; failure messages;failure entries' rendering; view modes and shortcuts. Terminal output under
:once/:all,except: a capture that raised is no longer announced as new, and the same-name race below.
Under
:nonea new screenshot now prints the failure,1 newand the existing "had nocommitted baseline … : new" roll-up line — intended: it is the same line
:onceprints.Adjacent fixes
disappeared in every mode; before, the early window also warned and counted it as new,
and
:nonesaid "No existing screenshot found …git addit".ScreenshotAssertion#display_pathdrops itsrescue ArgumentError(fix: a failed screenshot names the line that took it, once #303), as offered in fix: a size change labels its images like every other failure #305and like
Reporters::Default's: a path that can't relate to the root is a misconfiguration.Known limitations
Keyed by name (as the count already is). Under
record: :alla brand-new screenshot countsas re-recorded. One-process-per-worker runners: last writer wins, now also for a new-only process.
Evidence
keeping entries, fragment without paths, merge ignoring them / guessing an unknown reason /
names overriding them /
re-recorded merged as new, every sighting treated as first, finalize condition, re-recorded
listed, tally before the session check, existence check, template
DATA,shell escape,
--separator,:nonefall-through, no-wwarning.bin/rake test:unit812 runs, 0 failures (PR CI runs MRI 4.0 only).bin/standardrbclean.Follow-up
PR B: list re-recorded (
record: :all) screenshots with a before/after against the previousbaseline from git.
Summary by Sourcery
Include newly captured screenshots in reporting while keeping terminal counts, HTML output, and parallel-run results consistent.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
git addcommand to add new screenshots as baselines.record: :none, new screenshots are counted and fail the test by default.Bug Fixes
Documentation
The PR appears safe to merge; no actionable new defect was found.Summary
The HTML report now lists newly captured screenshots, including runs with no comparisons.
The previous review commit is not an ancestor of this head, so the full PR diff was reviewed. No previous review threads were supplied.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Capture succeeds] --> B{Committed baseline available?} B -->|Yes| C[Compare and record result] B -->|No| D{Baseline checked out earlier?} D -->|Yes| E[Raise disappeared-baseline error] D -->|No| F[Keep name, capture path, and reason] F --> G[Merge fork-worker entries] C --> H[Finalize HTML report] G --> H H --> I[List failures, then new captures]Reviews (2) · Last reviewed commit: "fix: the HTML report lists new screensho..." · Reviewed by Greptile