feat: quick wins + disable_animations helper + driver contract tests#174
Merged
feat: quick wins + disable_animations helper + driver contract tests#174
Conversation
Contributor
Reviewer's GuideImplements a new disable_animations helper following the existing hide_caret pattern, changes the HTML reporter to default its output path under the configured screenshot root, introduces shared driver contract tests, and removes several pieces of dead code and outdated behavior while documenting the Screenshoter constructor contract and adjusting tests/scripts accordingly. Sequence diagram for the new disable_animations helper in screenshot preparationsequenceDiagram
actor TestRunner
participant Screenshoter as Screenshoter
participant ScreenshotConfig as Screenshot
participant BrowserHelpers as BrowserHelpers
TestRunner->>Screenshoter: prepare_page_for_screenshot(timeout)
Screenshoter->>ScreenshotConfig: read blur_active_element
alt blur_active_element is true
Screenshoter->>BrowserHelpers: blur_from_focused_element()
BrowserHelpers-->>Screenshoter: blurred_input
else blur_active_element is false
Screenshoter-->>TestRunner: nil blurred_input
end
Screenshoter->>ScreenshotConfig: read hide_caret
alt hide_caret is true
Screenshoter->>BrowserHelpers: hide_caret()
end
Screenshoter->>ScreenshotConfig: read disable_animations
alt disable_animations is true
Screenshoter->>BrowserHelpers: disable_animations()
end
Screenshoter-->>TestRunner: prepared_page_with_optional_blur
Updated class diagram for Screenshoter, BrowserHelpers, Screenshot config, and HTML reporterclassDiagram
class CapybaraScreenshotDiffScreenshot {
<<module>>
+Boolean blur_active_element
+Boolean enabled
+Boolean hide_caret
+Boolean disable_animations
+Pathname root
+Integer stability_time_limit
+Array window_size
}
class CapybaraScreenshotDiffBrowserHelpers {
<<module>>
+hide_caret()
+disable_animations()
+blur_from_focused_element()
}
class CapybaraScreenshotDiffScreenshotScreenshoter {
+Hash capture_options
+Object driver
+Screenshoter(capture_options, comparison_options)
+prepare_page_for_screenshot(timeout)
}
class CapybaraScreenshotDiffReportersHTML {
+Pathname output_path
+Boolean embed_images
+Array failures
+Integer total
+HTML(output_path, embed_images)
+add_failure(name, compare)
+save()
+self.template_path()
+self.default_output_path()
}
CapybaraScreenshotDiffScreenshotScreenshoter ..> CapybaraScreenshotDiffBrowserHelpers : uses
CapybaraScreenshotDiffScreenshotScreenshoter ..> CapybaraScreenshotDiffScreenshot : reads_flags
CapybaraScreenshotDiffReportersHTML ..> CapybaraScreenshotDiffScreenshot : uses_root_and_save_path
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAdded a configurable animation-disabling hook that injects a guarded global CSS style before screenshots; refactored HTML reporter to lazily compute its default output path; removed unused helpers/test artifacts; added shared driver contract tests; small test harness and CI/script updates. Changes
Sequence Diagram(s)(Skipped — changes are localized and do not introduce multi-component sequential flows that benefit from a diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Screenshot diffs resolvedAll screenshots match their baselines. Previous diffs have been fixed. |
![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 left some high level feedback:
- In
Reporters::HTML.default_output_path, consider returning aPathnameinstead of aStringso callers (including#initialize) don’t need to re-wrap it and the type stays consistent throughout. - The
DISABLE_ANIMATIONS_SCRIPTrelies onstyle.sheet.insertRule, which can benullin some environments right after appending the<style>; usingstyle.textContent = '...css...'(orstyle.appendChild(document.createTextNode(...))) would make the helper more robust across browsers/drivers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Reporters::HTML.default_output_path`, consider returning a `Pathname` instead of a `String` so callers (including `#initialize`) don’t need to re-wrap it and the type stays consistent throughout.
- The `DISABLE_ANIMATIONS_SCRIPT` relies on `style.sheet.insertRule`, which can be `null` in some environments right after appending the `<style>`; using `style.textContent = '...css...'` (or `style.appendChild(document.createTextNode(...))`) would make the helper more robust across browsers/drivers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pftg
force-pushed
the
feat/quick-wins-and-animation-helper
branch
from
55e7992 to
5df6b00
Compare
![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)
test/support/driver_contract_tests.rb (1)
24-30: Make the “annotated images for different images” test self-contained.Add an explicit assertion on
comp.different?in this test to keep failure signals local and easier to diagnose.✅ Optional refinement
test "[contract] different? generates annotated images for different images" do comp = make_comparison(:a, :c) - comp.different? + assert comp.different? assert File.exist?(comp.reporter.annotated_base_image_path) assert File.exist?(comp.reporter.annotated_image_path) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/support/driver_contract_tests.rb` around lines 24 - 30, The test should assert that the comparison reports a difference before checking annotated files: add an explicit assertion such as assert comp.different? (or assert_equal true, comp.different?) immediately after calling comp.different? in the "[contract] different? generates annotated images for different images" test so failure of the comparison itself is reported locally; keep the existing File.exist? assertions for comp.reporter.annotated_base_image_path and comp.reporter.annotated_image_path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/support/test_doubles.rb`:
- Around line 99-101: There is an extra blank line between class definitions in
test/support/test_doubles.rb causing standardrb to fail; remove the extra empty
line so there is only a single blank line separating the previous class and the
TestDifference class (class TestDifference) to satisfy the linter.
---
Nitpick comments:
In `@test/support/driver_contract_tests.rb`:
- Around line 24-30: The test should assert that the comparison reports a
difference before checking annotated files: add an explicit assertion such as
assert comp.different? (or assert_equal true, comp.different?) immediately after
calling comp.different? in the "[contract] different? generates annotated images
for different images" test so failure of the comparison itself is reported
locally; keep the existing File.exist? assertions for
comp.reporter.annotated_base_image_path and comp.reporter.annotated_image_path
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9288cb5-9a35-48ab-91f5-d0e5262a9432
📒 Files selected for processing (14)
lib/capybara/screenshot/diff/browser_helpers.rblib/capybara/screenshot/diff/drivers/vips_driver.rblib/capybara/screenshot/diff/screenshoter.rblib/capybara_screenshot_diff.rblib/capybara_screenshot_diff/reporters/html.rblib/capybara_screenshot_diff/screenshot_namer.rbscripts/generate_sample_report.rbtest/support/driver_contract_tests.rbtest/support/test_doubles.rbtest/test_helper.rbtest/unit/drivers/chunky_png_driver_test.rbtest/unit/drivers/vips_driver_test.rbtest/unit/reporters/html_reporter_test.rbtest/unit/screenshot_namer_test.rb
💤 Files with no reviewable changes (3)
- test/unit/screenshot_namer_test.rb
- lib/capybara/screenshot/diff/drivers/vips_driver.rb
- lib/capybara_screenshot_diff/screenshot_namer.rb
pftg
force-pushed
the
feat/quick-wins-and-animation-helper
branch
4 times, most recently
from
967c6c3 to
d140fda
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/ci (1)
5-13: Make script path resolution independent of the caller’s working directory.
bin/standardrbandbin/rakerely on running from repo root. Resolving the repo root from the script location makes this CI entrypoint more reliable across runners and local usage.Proposed diff
-#!/bin/bash +#!/bin/bash set -e + +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" echo "Running linter..." -if ! bin/standardrb; then +if ! "$ROOT_DIR/bin/standardrb"; then echo "Lint errors found. Auto-fixing..." - bin/standardrb -a + "$ROOT_DIR/bin/standardrb" -a echo "Lint errors were auto-fixed. Please review and re-commit." exit 1 fi echo "Running unit tests..." -bin/rake test:unit +"$ROOT_DIR/bin/rake" test:unit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ci` around lines 5 - 13, The CI script currently invokes bin/standardrb and bin/rake relative to the caller’s cwd; make path resolution independent by resolving the script directory at runtime (use the script’s directory via "$(cd "$(dirname "$0")" && pwd)" or equivalent) and prepend that directory when invoking the tools so calls to bin/standardrb -a and bin/rake test:unit use the repo’s bin directory; update the invocations in the bin/ci script (references: bin/standardrb and bin/rake) to use the resolved script-dir-prefixed paths and keep behavior (auto-fix, messages, exit codes) unchanged.test/unit/drivers/vips_driver_test.rb (1)
19-19: Consider removing now-redundant local assertions.After including
DriverContractTestson Line 19, a few existing VIPS tests in this class overlap with the same contract behaviors. Pruning duplicates would reduce maintenance and test runtime without losing coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/drivers/vips_driver_test.rb` at line 19, The test class now includes the shared suite via include DriverContractTests; remove redundant local assertions and duplicate test methods in the VIPS driver test class that replicate behaviors provided by DriverContractTests (identify methods/assertions referencing the same contracts and delete them), keeping only any VIPS-specific or additional edge-case assertions; ensure any remaining unique tests reference the VIPS driver helper/setup used in this file and run the test suite to confirm coverage is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/ci`:
- Around line 5-13: The CI script currently invokes bin/standardrb and bin/rake
relative to the caller’s cwd; make path resolution independent by resolving the
script directory at runtime (use the script’s directory via "$(cd "$(dirname
"$0")" && pwd)" or equivalent) and prepend that directory when invoking the
tools so calls to bin/standardrb -a and bin/rake test:unit use the repo’s bin
directory; update the invocations in the bin/ci script (references:
bin/standardrb and bin/rake) to use the resolved script-dir-prefixed paths and
keep behavior (auto-fix, messages, exit codes) unchanged.
In `@test/unit/drivers/vips_driver_test.rb`:
- Line 19: The test class now includes the shared suite via include
DriverContractTests; remove redundant local assertions and duplicate test
methods in the VIPS driver test class that replicate behaviors provided by
DriverContractTests (identify methods/assertions referencing the same contracts
and delete them), keeping only any VIPS-specific or additional edge-case
assertions; ensure any remaining unique tests reference the VIPS driver
helper/setup used in this file and run the test suite to confirm coverage is
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3668474d-e6e6-4244-a4a3-148aa413fea5
📒 Files selected for processing (16)
README.mdbin/cilib/capybara/screenshot/diff/browser_helpers.rblib/capybara/screenshot/diff/drivers/vips_driver.rblib/capybara/screenshot/diff/screenshoter.rblib/capybara_screenshot_diff.rblib/capybara_screenshot_diff/reporters/html.rblib/capybara_screenshot_diff/screenshot_namer.rbscripts/generate_sample_report.rbtest/support/driver_contract_tests.rbtest/support/test_doubles.rbtest/test_helper.rbtest/unit/drivers/chunky_png_driver_test.rbtest/unit/drivers/vips_driver_test.rbtest/unit/reporters/html_reporter_test.rbtest/unit/screenshot_namer_test.rb
💤 Files with no reviewable changes (3)
- test/unit/screenshot_namer_test.rb
- lib/capybara/screenshot/diff/drivers/vips_driver.rb
- lib/capybara_screenshot_diff/screenshot_namer.rb
✅ Files skipped from review due to trivial changes (5)
- test/unit/drivers/chunky_png_driver_test.rb
- test/test_helper.rb
- test/unit/reporters/html_reporter_test.rb
- scripts/generate_sample_report.rb
- lib/capybara_screenshot_diff.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/capybara/screenshot/diff/screenshoter.rb
- lib/capybara/screenshot/diff/browser_helpers.rb
- lib/capybara_screenshot_diff/reporters/html.rb
pftg
force-pushed
the
feat/quick-wins-and-animation-helper
branch
3 times, most recently
from
12b70b5 to
d34a625
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/capybara/screenshot/diff/vcs.rb`:
- Around line 12-16: The code resolves screenshot_path with
Pathname.new(screenshot_path).expand_path which uses the process CWD; change it
to expand relative to the repository root (use git_root or root_path as the
base) before calling relative_path_from so resolution is independent of the
process CWD. Locate the vcs_file_path assignment and update the expand_path call
to use the git_root/root_path as the second argument (keeping the subsequent
relative_path_from(Pathname.new(git_root))) so vcs_file_path is computed from
the repo root rather than the current working directory.
- Line 13: Replace all backtick and interpolated string shell invocations that
use root_path, checkout_path, and vcs_file_path (e.g., the git_root assignment
and subsequent system(...) calls) with argv-style execution to avoid shell
interpolation: use Open3.capture3 (or Kernel.system/spawn with the array-form)
and pass the command and each argument as separate parameters (e.g., ['git',
'-C', root_path, 'rev-parse', '--show-toplevel']) so paths are not interpreted
by the shell; update the git_root assignment and every system(...) call that
currently interpolates root_path/checkout_path/vcs_file_path to use this safe
form and handle stdout/stderr/exit status accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 428ab84d-2ed3-45b3-9569-b236652e6b80
📒 Files selected for processing (3)
lib/capybara/screenshot/diff/vcs.rbtest/unit/static_test.rbtest/unit/vcs_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/unit/vcs_test.rb
pftg
force-pushed
the
feat/quick-wins-and-animation-helper
branch
from
d34a625 to
5261dc3
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- VCS uses Open3/array-form system (no Dir.chdir, no shell interpolation) - Global state guard: snapshot/restore all mattr_accessors per test - static_test.rb restores Screenshot.root in teardown - bin/ci: lint + unit tests with auto-fix for pre-push hook Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pftg
force-pushed
the
feat/quick-wins-and-animation-helper
branch
from
5261dc3 to
60222d2
Compare
pftg
added a commit
that referenced
this pull request
Apr 12, 2026
…t.html
Default output path changed from tmp/snap_diff/index.html to
{root}/{save_path}/snap_diff_report.html in PR #174. Updated all
docs and CI action to match.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 12, 2026
2 tasks
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.
Summary
7 tasks from the prioritized backlog:
Quick wins
TestPreprocessor,VipsDriver#region_covers_entire_image?,ScreenshotNamer#clear_current_group_directory(verified with SimpleCov)TestDriver#classcomment — explain WHY, not WHATTestPreprocessorclassFeatures
{root}/{save_path}/snap_diff_report.html) — convention over configuration, no new settingScreenshoterconstructor contract (signatures already unified)Capybara::Screenshot.disable_animations = true— injects CSS to disable all animations/transitions before screenshotTests
DriverContractTestsmodule — 5 contract assertions included in both ChunkyPNG and VIPS driver tests (+30 assertions)Test plan
disable_animationsfollowshide_caretpattern exactlyGenerated with Claude Code
Summary by Sourcery
Introduce animation-disabling support for screenshots, standardize HTML report output location, and add shared driver contract tests while removing unused code and helpers.
New Features:
Enhancements:
Tests:
Chores:
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
Chores
Removals