Closed
Conversation
Contributor
Reviewer's GuideIntroduces a unified screenshot consistency configuration pipeline (DOM normalization, font-loading waits, CSS/JS injection) and supporting browser helpers, tightens VCS checkout against external git env overrides, updates the GitHub Actions setup to standardize fonts and hinting, and documents OS/CI setup and consistency presets while keeping the README minimal. Sequence diagram for preparing a consistent screenshot capturesequenceDiagram
actor TestSuite
participant Screenshot
participant ConsistencyConfig
participant Screenshoter
participant BrowserHelpers
participant Browser as BrowserDOM
TestSuite->>Screenshot: configure_consistency(preset: default) do |config|
Screenshot->>Screenshot: enable_consistent_screenshots!(normalize_css, wait_for_fonts, disable_animations, hide_caret, blur_active_element)
Screenshot->>ConsistencyConfig: new(css: custom_stylesheets, js: custom_scripts, normalize_css, wait_for_fonts, disable_animations, hide_caret, blur_active_element)
Screenshot-->>ConsistencyConfig: config instance
TestSuite->>ConsistencyConfig: mutate flags and injections
TestSuite->>ConsistencyConfig: apply_to_screenshot!(Screenshot)
ConsistencyConfig->>Screenshot: set normalize_css, wait_for_fonts, disable_animations, hide_caret, blur_active_element
TestSuite->>Screenshoter: prepare_page_for_screenshot(timeout)
Screenshoter->>Screenshoter: wait_images_loaded(timeout)
alt wait_for_fonts? is true
Screenshoter->>Screenshoter: wait_for_fonts(timeout)
loop until fonts loaded or timeout
Screenshoter->>BrowserHelpers: fonts_ready?()
BrowserHelpers->>BrowserDOM: execute FONTS_READY_SCRIPT
BrowserDOM-->>BrowserHelpers: fonts status
BrowserHelpers-->>Screenshoter: Boolean
end
end
alt normalize_css? is true
Screenshoter->>BrowserHelpers: normalize_css(normalize_stylesheet)
BrowserHelpers->>BrowserDOM: inject DEFAULT_NORMALIZE_CSS or custom CSS
end
Screenshoter->>BrowserHelpers: inject_custom_stylesheets(Screenshot.custom_stylesheets)
BrowserHelpers->>BrowserDOM: inject style tags
Screenshoter->>BrowserHelpers: inject_custom_scripts(Screenshot.custom_scripts)
BrowserHelpers->>BrowserDOM: execute custom JS
Screenshoter->>BrowserHelpers: blur_from_focused_element (if blur_active_element)
BrowserHelpers->>BrowserDOM: blur active element
Screenshoter-->>TestSuite: page ready for screenshot
Class diagram for unified screenshot consistency configurationclassDiagram
class Screenshot {
<<module>>
+Boolean enabled
+Boolean hide_caret
+Boolean disable_animations
+Boolean normalize_css
+String normalize_stylesheet
+Boolean wait_for_fonts
+Array custom_stylesheets
+Array custom_scripts
+Numeric stability_time_limit
+Array window_size
+Hash capybara_screenshot_options
+ConsistencyConfig configure_consistency(preset)
+void enable_consistent_screenshots!(normalize_css, wait_for_fonts, disable_animations, hide_caret, blur_active_element)
}
class ConsistencyConfig {
+Array css
+Array js
+Boolean normalize_css
+Boolean wait_for_fonts
+Boolean disable_animations
+Boolean hide_caret
+Boolean blur_active_element
+initialize(css, js, normalize_css, wait_for_fonts, disable_animations, hide_caret, blur_active_element)
+void apply_to_screenshot!(screenshot)
}
class Screenshoter {
-Hash @capture_options
+void prepare_page_for_screenshot(timeout)
+void wait_images_loaded(timeout)
+void wait_for_fonts(timeout)
-Boolean normalize_css?()
-String normalize_stylesheet()
-Boolean wait_for_fonts?()
+void save_and_process_screenshot(screenshot_path)
}
class BrowserHelpers {
<<module>>
+String DEFAULT_NORMALIZE_CSS
+void disable_animations()
+void normalize_css(css)
+void inject_custom_stylesheets(stylesheets)
+void inject_custom_scripts(scripts)
+void inject_stylesheet(css, element_id)
+Boolean fonts_ready?()
+Object session
}
class Vcs {
<<module>>
+Boolean checkout_vcs(root, screenshot_path, checkout_path)
}
Screenshot "1" o-- "1" ConsistencyConfig : uses
Screenshot <.. Screenshoter : config source
Screenshoter ..> BrowserHelpers : calls
Vcs ..> Screenshot : uses Screenshot.use_lfs
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR introduces OS-level screenshot rendering standardization across Ubuntu (glibc) and Alpine (musl) distributions. Changes include enhanced GitHub Actions setup with font packages and Fontconfig configuration, new global configuration options for CSS normalization, font readiness detection, and custom asset injection, plus comprehensive documentation covering OS setup, CI integration, and configuration presets. Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Browser Page
participant SH as Screenshoter
participant BH as BrowserHelpers
participant Fonts as Fonts API
SH->>BH: normalize_css(stylesheet)?
BH->>Page: inject csdNormalizeStyle tag
Page-->>BH: style tag injected
SH->>BH: inject_custom_stylesheets(sheets)
BH->>Page: inject csdCustomStyle tags
Page-->>BH: styles injected
SH->>BH: inject_custom_scripts(scripts)
BH->>Page: execute scripts via session
Page-->>BH: scripts executed
SH->>BH: fonts_ready?
BH->>Fonts: check document.fonts.status
Fonts-->>BH: "loaded" or pending
alt fonts not ready
BH->>BH: poll with timeout
BH->>Fonts: check status again
Fonts-->>BH: status update
end
BH-->>SH: fonts ready or timeout error
SH->>Page: continue (animations, caret, etc.)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 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 detected
|
![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, and left some high level feedback:
- In
Vcs.checkout_vcs, consider extracting thegit_env = {"GIT_DIR" => nil, "GIT_WORK_TREE" => nil, "GIT_INDEX_FILE" => nil}hash into a small helper (e.g.,clean_git_env) so the same environment setup isn’t duplicated across eachOpen3.capture3/systemcall and is easier to extend if more variables need to be scrubbed later. - The
with_envhelper invcs_test.rbcurrently usesvars.transform_values { |_, _| nil }, which is slightly awkward; simplifying this to something likeprevious = ENV.to_h.slice(*vars.keys)would both be clearer and avoid having to manually initialize and populatepreviousin two passes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Vcs.checkout_vcs`, consider extracting the `git_env = {"GIT_DIR" => nil, "GIT_WORK_TREE" => nil, "GIT_INDEX_FILE" => nil}` hash into a small helper (e.g., `clean_git_env`) so the same environment setup isn’t duplicated across each `Open3.capture3`/`system` call and is easier to extend if more variables need to be scrubbed later.
- The `with_env` helper in `vcs_test.rb` currently uses `vars.transform_values { |_, _| nil }`, which is slightly awkward; simplifying this to something like `previous = ENV.to_h.slice(*vars.keys)` would both be clearer and avoid having to manually initialize and populate `previous` in two passes.
## Individual Comments
### Comment 1
<location path="test/unit/screenshoter_test.rb" line_range="51-60" />
<code_context>
+ test "#prepare_page_for_screenshot injects normalization and custom hooks in order" do
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for font-waiting behaviour, including timeout and per-call overrides
Right now, `wait_for_fonts` in `prepare_page_for_screenshot` is only covered indirectly and only for the immediate-return case. Please add targeted tests that:
1. Verify we actually call `BrowserHelpers.fonts_ready?` and exit the loop when it quickly returns `true`.
2. Cover the timeout path where `fonts_ready?` never returns `true` and we raise `CapybaraScreenshotDiff::ExpectationNotMet` with the expected message.
3. Assert that when `wait_for_fonts` is disabled via `@capture_options[:wait_for_fonts]`, we skip the font-waiting logic even if the global default is `true`.
You can stub `BrowserHelpers.fonts_ready?` and time-related calls (`Process.clock_gettime` / `sleep`) to avoid real timing dependencies and flakiness.
Suggested implementation:
```ruby
test "#prepare_page_for_screenshot injects normalization and custom hooks in order" do
Capybara::Screenshot.normalize_css = true
Capybara:: Screenshot.custom_stylesheets = ["body { color: red; }"]
Capybara::Screenshot.custom_scripts = ["window.__csd = true;"]
Capybara::Screenshot.blur_active_element = false
Capybara::Screenshot.hide_caret = false
Capybara::Screenshot.disable_animations = false
screenshoter = Screenshoter.new({wait: nil}, {driver: :chunky_png})
calls = []
```
Because I only see part of the file, the following tests should be **added somewhere in the same test class** that currently contains `#prepare_page_for_screenshot injects normalization and custom hooks in order` (typically near the other `Screenshoter` tests). They are self‑contained and use only standard Minitest and Ruby stubbing patterns:
```ruby
test "#prepare_page_for_screenshot waits for fonts and exits early when they become ready" do
Capybara::Screenshot.wait_for_fonts = true
screenshoter = Screenshoter.new({}, { driver: :chunky_png })
# Simulate fonts not ready once, then ready
ready_sequence = [false, true]
sleep_calls = []
BrowserHelpers.stub(:fonts_ready?, -> { ready_sequence.shift }) do
Kernel.stub(:sleep, ->(t) { sleep_calls << t }) do
# Use a reasonably large timeout; we expect to exit *before* it
assert_nil screenshoter.prepare_page_for_screenshot(timeout: 5.0)
end
end
# We should have slept at least once, but should *not* have hit the timeout
refute_empty sleep_calls
end
test "#prepare_page_for_screenshot times out when fonts never become ready" do
Capybara::Screenshot.wait_for_fonts = true
screenshoter = Screenshoter.new({}, { driver: :chunky_png })
# fonts_ready? always false
BrowserHelpers.stub(:fonts_ready?, false) do
# Simulate monotonic clock advancing until it passes the timeout
times = [0.0, 0.5, 1.0, 1.5, 2.1] # last value exceeds 2.0s timeout
Process.stub(:clock_gettime, ->(*) { times.shift || 2.1 }) do
Kernel.stub(:sleep, ->(_t) { }) do
error = assert_raises(CapybaraScreenshotDiff::ExpectationNotMet) do
screenshoter.prepare_page_for_screenshot(timeout: 2.0)
end
assert_match(/fonts.*not become ready within 2\.0 seconds/i, error.message)
end
end
end
end
test "#prepare_page_for_screenshot skips font waiting when disabled per capture options" do
# Global default is true, but this specific capture opts out
Capybara::Screenshot.wait_for_fonts = true
screenshoter = Screenshoter.new({ wait_for_fonts: false }, { driver: :chunky_png })
# Fail the test if fonts_ready? is ever called
BrowserHelpers.stub(:fonts_ready?, -> { flunk("fonts_ready? should not be called when wait_for_fonts is disabled per capture") }) do
Kernel.stub(:sleep, ->(_t) { flunk("sleep should not be called when wait_for_fonts is disabled per capture") }) do
assert_nil screenshoter.prepare_page_for_screenshot(timeout: 2.0)
end
end
end
```
If the actual API for configuring per-capture options differs (for example, if `wait_for_fonts` is passed as a keyword argument to `prepare_page_for_screenshot` instead of being in the first `Screenshoter.new` hash), adjust the third test so that it sets `@capture_options[:wait_for_fonts]` the same way production code does. The key points are:
1. The per-call override must cause `BrowserHelpers.fonts_ready?` *not* to be invoked.
2. The global `Capybara::Screenshot.wait_for_fonts` being `true` should not override that per-call setting.
3. The method should still return without error when font waiting is disabled.
</issue_to_address>
### Comment 2
<location path="test/unit/vcs_test.rb" line_range="40" />
<code_context>
assert_equal screenshot_path.size, base_screenshot_path.size
end
+
+ test "#checkout_vcs ignores external git env overrides" do
+ screenshot_path = file_fixture("images/a.png")
+ base_screenshot_path = Pathname.new(@base_screenshot.path)
+
+ with_env("GIT_DIR" => "/tmp/does-not-exist", "GIT_WORK_TREE" => "/tmp/does-not-exist") do
+ assert Vcs.checkout_vcs(@tmp_dir, screenshot_path, base_screenshot_path),
+ "checkout_vcs failed with overridden git env: root=#{@tmp_dir}"
+ end
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend git-env test to cover all cleared variables, especially GIT_INDEX_FILE
Since the code also clears `GIT_INDEX_FILE`, please include it in the overridden env for this test (e.g. `"GIT_INDEX_FILE" => "/tmp/does-not-exist"`). That keeps the test consistent with the implementation and ensures we detect regressions if any of these env clears are changed or removed.
```suggestion
with_env("GIT_DIR" => "/tmp/does-not-exist", "GIT_WORK_TREE" => "/tmp/does-not-exist", "GIT_INDEX_FILE" => "/tmp/does-not-exist") do
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+60
| test "#prepare_page_for_screenshot injects normalization and custom hooks in order" do | ||
| Capybara::Screenshot.normalize_css = true | ||
| Capybara::Screenshot.custom_stylesheets = ["body { color: red; }"] | ||
| Capybara::Screenshot.custom_scripts = ["window.__csd = true;"] | ||
| Capybara::Screenshot.blur_active_element = false | ||
| Capybara::Screenshot.hide_caret = false | ||
| Capybara::Screenshot.disable_animations = false | ||
|
|
||
| screenshoter = Screenshoter.new({wait: nil}, {driver: :chunky_png}) | ||
| calls = [] |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for font-waiting behaviour, including timeout and per-call overrides
Right now, wait_for_fonts in prepare_page_for_screenshot is only covered indirectly and only for the immediate-return case. Please add targeted tests that:
- Verify we actually call
BrowserHelpers.fonts_ready?and exit the loop when it quickly returnstrue. - Cover the timeout path where
fonts_ready?never returnstrueand we raiseCapybaraScreenshotDiff::ExpectationNotMetwith the expected message. - Assert that when
wait_for_fontsis disabled via@capture_options[:wait_for_fonts], we skip the font-waiting logic even if the global default istrue.
You can stub BrowserHelpers.fonts_ready? and time-related calls (Process.clock_gettime / sleep) to avoid real timing dependencies and flakiness.
Suggested implementation:
test "#prepare_page_for_screenshot injects normalization and custom hooks in order" do
Capybara::Screenshot.normalize_css = true
Capybara:: Screenshot.custom_stylesheets = ["body { color: red; }"]
Capybara::Screenshot.custom_scripts = ["window.__csd = true;"]
Capybara::Screenshot.blur_active_element = false
Capybara::Screenshot.hide_caret = false
Capybara::Screenshot.disable_animations = false
screenshoter = Screenshoter.new({wait: nil}, {driver: :chunky_png})
calls = []Because I only see part of the file, the following tests should be added somewhere in the same test class that currently contains #prepare_page_for_screenshot injects normalization and custom hooks in order (typically near the other Screenshoter tests). They are self‑contained and use only standard Minitest and Ruby stubbing patterns:
test "#prepare_page_for_screenshot waits for fonts and exits early when they become ready" do
Capybara::Screenshot.wait_for_fonts = true
screenshoter = Screenshoter.new({}, { driver: :chunky_png })
# Simulate fonts not ready once, then ready
ready_sequence = [false, true]
sleep_calls = []
BrowserHelpers.stub(:fonts_ready?, -> { ready_sequence.shift }) do
Kernel.stub(:sleep, ->(t) { sleep_calls << t }) do
# Use a reasonably large timeout; we expect to exit *before* it
assert_nil screenshoter.prepare_page_for_screenshot(timeout: 5.0)
end
end
# We should have slept at least once, but should *not* have hit the timeout
refute_empty sleep_calls
end
test "#prepare_page_for_screenshot times out when fonts never become ready" do
Capybara::Screenshot.wait_for_fonts = true
screenshoter = Screenshoter.new({}, { driver: :chunky_png })
# fonts_ready? always false
BrowserHelpers.stub(:fonts_ready?, false) do
# Simulate monotonic clock advancing until it passes the timeout
times = [0.0, 0.5, 1.0, 1.5, 2.1] # last value exceeds 2.0s timeout
Process.stub(:clock_gettime, ->(*) { times.shift || 2.1 }) do
Kernel.stub(:sleep, ->(_t) { }) do
error = assert_raises(CapybaraScreenshotDiff::ExpectationNotMet) do
screenshoter.prepare_page_for_screenshot(timeout: 2.0)
end
assert_match(/fonts.*not become ready within 2\.0 seconds/i, error.message)
end
end
end
end
test "#prepare_page_for_screenshot skips font waiting when disabled per capture options" do
# Global default is true, but this specific capture opts out
Capybara::Screenshot.wait_for_fonts = true
screenshoter = Screenshoter.new({ wait_for_fonts: false }, { driver: :chunky_png })
# Fail the test if fonts_ready? is ever called
BrowserHelpers.stub(:fonts_ready?, -> { flunk("fonts_ready? should not be called when wait_for_fonts is disabled per capture") }) do
Kernel.stub(:sleep, ->(_t) { flunk("sleep should not be called when wait_for_fonts is disabled per capture") }) do
assert_nil screenshoter.prepare_page_for_screenshot(timeout: 2.0)
end
end
endIf the actual API for configuring per-capture options differs (for example, if wait_for_fonts is passed as a keyword argument to prepare_page_for_screenshot instead of being in the first Screenshoter.new hash), adjust the third test so that it sets @capture_options[:wait_for_fonts] the same way production code does. The key points are:
- The per-call override must cause
BrowserHelpers.fonts_ready?not to be invoked. - The global
Capybara::Screenshot.wait_for_fontsbeingtrueshould not override that per-call setting. - The method should still return without error when font waiting is disabled.
| screenshot_path = file_fixture("images/a.png") | ||
| base_screenshot_path = Pathname.new(@base_screenshot.path) | ||
|
|
||
| with_env("GIT_DIR" => "/tmp/does-not-exist", "GIT_WORK_TREE" => "/tmp/does-not-exist") do |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend git-env test to cover all cleared variables, especially GIT_INDEX_FILE
Since the code also clears GIT_INDEX_FILE, please include it in the overridden env for this test (e.g. "GIT_INDEX_FILE" => "/tmp/does-not-exist"). That keeps the test consistent with the implementation and ensures we detect regressions if any of these env clears are changed or removed.
Suggested change
| with_env("GIT_DIR" => "/tmp/does-not-exist", "GIT_WORK_TREE" => "/tmp/does-not-exist") do | |
| with_env("GIT_DIR" => "/tmp/does-not-exist", "GIT_WORK_TREE" => "/tmp/does-not-exist", "GIT_INDEX_FILE" => "/tmp/does-not-exist") do |
![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
🧹 Nitpick comments (4)
test/unit/screenshoter_test.rb (1)
51-75: Missing test cleanup for global configuration state.The test modifies global
Capybara::Screenshotsettings but doesn't restore them afterward, which could cause test pollution if other tests rely on default values. Consider usingsetup/teardownor anensureblock to reset these flags.♻️ Proposed fix to add cleanup
test "#prepare_page_for_screenshot injects normalization and custom hooks in order" do + original_normalize_css = Capybara::Screenshot.normalize_css + original_custom_stylesheets = Capybara::Screenshot.custom_stylesheets + original_custom_scripts = Capybara::Screenshot.custom_scripts + original_blur = Capybara::Screenshot.blur_active_element + original_hide_caret = Capybara::Screenshot.hide_caret + original_disable_animations = Capybara::Screenshot.disable_animations + Capybara::Screenshot.normalize_css = true Capybara::Screenshot.custom_stylesheets = ["body { color: red; }"] Capybara::Screenshot.custom_scripts = ["window.__csd = true;"] Capybara::Screenshot.blur_active_element = false Capybara::Screenshot.hide_caret = false Capybara::Screenshot.disable_animations = false # ... test body ... assert_equal [ [:normalize_css, nil], [:inject_custom_stylesheets, ["body { color: red; }"]], [:inject_custom_scripts, ["window.__csd = true;"]] ], calls + ensure + Capybara::Screenshot.normalize_css = original_normalize_css + Capybara::Screenshot.custom_stylesheets = original_custom_stylesheets + Capybara::Screenshot.custom_scripts = original_custom_scripts + Capybara::Screenshot.blur_active_element = original_blur + Capybara::Screenshot.hide_caret = original_hide_caret + Capybara::Screenshot.disable_animations = original_disable_animations end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/screenshoter_test.rb` around lines 51 - 75, The test "prepare_page_for_screenshot injects normalization and custom hooks in order" mutates global Capybara::Screenshot settings (normalize_css, custom_stylesheets, custom_scripts, blur_active_element, hide_caret, disable_animations) without restoring them; wrap the changes in a safe teardown (or use setup/teardown) or an ensure block inside the test to capture original values before assignment and restore them after calling Screenshoter#prepare_page_for_screenshot so global state is reset for other tests.lib/capybara/screenshot/diff/browser_helpers.rb (1)
186-195: Font readiness check relies on synchronous status property.The current implementation checks
document.fonts.status === "loaded", which works but could miss edge cases where fonts are still being loaded asynchronously. Thedocument.fonts.readyPromise is generally more reliable for ensuring all fonts have finished loading.That said, the polling approach in
wait_for_fontswill eventually catch the "loaded" status, so this is functional. Consider this a potential future improvement if font-related flakiness is observed.♻️ Alternative using document.fonts.ready
FONTS_READY_SCRIPT = <<~JS (function() { if (!document.fonts) return true; - return document.fonts.status === "loaded"; + // Check both status and pending load count for robustness + return document.fonts.status === "loaded" && document.fonts.size > 0 + ? true + : document.fonts.status === "loaded"; })(); JSOr for more robust async handling, consider using
document.fonts.ready.then()with a callback approach in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/capybara/screenshot/diff/browser_helpers.rb` around lines 186 - 195, The fonts readiness check in FONTS_READY_SCRIPT used by BrowserHelpers.fonts_ready? relies on the synchronous document.fonts.status value which can miss async loads; update the script evaluated by BrowserHelpers.session.evaluate_script (the FONTS_READY_SCRIPT constant) to use the document.fonts.ready Promise (or await document.fonts.ready.then(() => true)) so fonts_ready? returns only after the fonts.ready promise resolves; keep the wait_for_fonts polling logic but replace the status check with the Promise-based readiness check for more reliable detection..github/actions/setup-ruby-and-dependencies/action.yml (1)
29-34: Consider breaking long lines for readability.Static analysis flagged lines 29 and 34 as exceeding 120 characters. While not functionally problematic, multi-line formatting would improve maintainability.
♻️ Proposed fix using YAML multi-line string
with: - packages: libvips libglib2.0-0 libglib2.0-dev libwebp-dev libvips42 libpng-dev fonts-dejavu fonts-liberation fonts-ubuntu fonts-noto-color-emoji + packages: >- + libvips libglib2.0-0 libglib2.0-dev libwebp-dev libvips42 libpng-dev + fonts-dejavu fonts-liberation fonts-ubuntu fonts-noto-color-emoji version: tests-v2 - name: Install vips (fallback) if: ${{ inputs.cache-apt-packages != 'true' }} - run: sudo apt-get -qq update && sudo apt-get -qq install -y libvips fonts-dejavu fonts-liberation fonts-ubuntu fonts-noto-color-emoji + run: | + sudo apt-get -qq update + sudo apt-get -qq install -y libvips \ + fonts-dejavu fonts-liberation fonts-ubuntu fonts-noto-color-emoji shell: bash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup-ruby-and-dependencies/action.yml around lines 29 - 34, The long YAML values exceed 120 characters; split the long "packages" value and the long "run" command in the "Install vips (fallback)" step into multi-line form for readability — for example, change the "packages:" entry to a YAML sequence or use a folded block (>) to wrap the long list, and replace the single-line "run: sudo apt-get -qq update && sudo apt-get -qq install -y libvips ..." with a multi-line folded string or multiple run lines in the "Install vips (fallback)" step so each line stays under 120 chars and the identifiers (packages, Install vips (fallback), run) remain unchanged.test/unit/screenshot_test.rb (1)
28-45: Consider restoring boolean configuration flags after test.While the test correctly resets
custom_stylesheetsandcustom_scriptsat the start, it modifies other global flags (normalize_css,wait_for_fonts,disable_animations,hide_caret,blur_active_element) without restoring them. This could affect subsequent tests that rely on default values.♻️ Proposed fix with ensure block
test "configure_consistency presets defaults and allows custom injections" do + # Store original values + orig_normalize_css = Capybara::Screenshot.normalize_css + orig_wait_for_fonts = Capybara::Screenshot.wait_for_fonts + orig_disable_animations = Capybara::Screenshot.disable_animations + orig_hide_caret = Capybara::Screenshot.hide_caret + orig_blur_active_element = Capybara::Screenshot.blur_active_element + orig_custom_stylesheets = Capybara::Screenshot.custom_stylesheets + orig_custom_scripts = Capybara::Screenshot.custom_scripts + Capybara::Screenshot.custom_stylesheets = [] Capybara::Screenshot.custom_scripts = [] # ... rest of test ... assert_includes Capybara::Screenshot.custom_scripts, "window.__csd_test = true;" + ensure + Capybara::Screenshot.normalize_css = orig_normalize_css + Capybara::Screenshot.wait_for_fonts = orig_wait_for_fonts + Capybara::Screenshot.disable_animations = orig_disable_animations + Capybara::Screenshot.hide_caret = orig_hide_caret + Capybara::Screenshot.blur_active_element = orig_blur_active_element + Capybara::Screenshot.custom_stylesheets = orig_custom_stylesheets + Capybara::Screenshot.custom_scripts = orig_custom_scripts end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/screenshot_test.rb` around lines 28 - 45, The test "configure_consistency presets defaults and allows custom injections" mutates global flags on Capybara::Screenshot and doesn't restore them; before calling Capybara::Screenshot.configure_consistency, capture the original values of Capybara::Screenshot.normalize_css, .wait_for_fonts, .disable_animations, .hide_caret, and .blur_active_element, and ensure they are reset in an ensure/finally block (or use Minitest's teardown) after the assertions so global state is restored; keep the existing resets for custom_stylesheets and custom_scripts and only wrap the flag changes around the configure_consistency call and assertions.
🤖 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.rb`:
- Around line 71-77: apply_to_screenshot! currently copies flags like
normalize_css but never persists any config.css or config.js assignments made in
configure_consistency blocks, so assignment-style overrides are lost; update
apply_to_screenshot! (and the same logic around configure_consistency usage) to
also write any configured css and js back onto the screenshot (e.g., if
config.css or config.js are non-nil/non-empty, set screenshot.css = config.css
and screenshot.js = config.js) so both mutation and full-assignment overrides
are honored; reference apply_to_screenshot!, configure_consistency, config.css
and config.js when making the change.
In `@lib/capybara/screenshot/diff/vcs.rb`:
- Around line 23-24: The call to system(git_env, "git", "-C", root_path, "lfs",
"smudge", in: tmp_path, out: checkout_path.to_s, err: File::NULL) inside
checkout_vcs currently ignores the exit status, allowing checkout_vcs to return
true even when git lfs smudge fails; capture the return value (e.g. result =
system(...)) and if it is false or nil propagate the failure by marking success
as false or returning false immediately (and optionally log the error),
referencing the existing variables and method names (checkout_vcs, git_env,
root_path, tmp_path, checkout_path) so the checkout failure is not silently
ignored.
---
Nitpick comments:
In @.github/actions/setup-ruby-and-dependencies/action.yml:
- Around line 29-34: The long YAML values exceed 120 characters; split the long
"packages" value and the long "run" command in the "Install vips (fallback)"
step into multi-line form for readability — for example, change the "packages:"
entry to a YAML sequence or use a folded block (>) to wrap the long list, and
replace the single-line "run: sudo apt-get -qq update && sudo apt-get -qq
install -y libvips ..." with a multi-line folded string or multiple run lines in
the "Install vips (fallback)" step so each line stays under 120 chars and the
identifiers (packages, Install vips (fallback), run) remain unchanged.
In `@lib/capybara/screenshot/diff/browser_helpers.rb`:
- Around line 186-195: The fonts readiness check in FONTS_READY_SCRIPT used by
BrowserHelpers.fonts_ready? relies on the synchronous document.fonts.status
value which can miss async loads; update the script evaluated by
BrowserHelpers.session.evaluate_script (the FONTS_READY_SCRIPT constant) to use
the document.fonts.ready Promise (or await document.fonts.ready.then(() =>
true)) so fonts_ready? returns only after the fonts.ready promise resolves; keep
the wait_for_fonts polling logic but replace the status check with the
Promise-based readiness check for more reliable detection.
In `@test/unit/screenshot_test.rb`:
- Around line 28-45: The test "configure_consistency presets defaults and allows
custom injections" mutates global flags on Capybara::Screenshot and doesn't
restore them; before calling Capybara::Screenshot.configure_consistency, capture
the original values of Capybara::Screenshot.normalize_css, .wait_for_fonts,
.disable_animations, .hide_caret, and .blur_active_element, and ensure they are
reset in an ensure/finally block (or use Minitest's teardown) after the
assertions so global state is restored; keep the existing resets for
custom_stylesheets and custom_scripts and only wrap the flag changes around the
configure_consistency call and assertions.
In `@test/unit/screenshoter_test.rb`:
- Around line 51-75: The test "prepare_page_for_screenshot injects normalization
and custom hooks in order" mutates global Capybara::Screenshot settings
(normalize_css, custom_stylesheets, custom_scripts, blur_active_element,
hide_caret, disable_animations) without restoring them; wrap the changes in a
safe teardown (or use setup/teardown) or an ensure block inside the test to
capture original values before assignment and restore them after calling
Screenshoter#prepare_page_for_screenshot so global state is reset for other
tests.
🪄 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: 9e40b3e7-e8e3-4f63-a10b-ab3f24609478
📒 Files selected for processing (17)
.github/actions/setup-ruby-and-dependencies/action.ymlREADME.mddocs/adr/0001-screenshot-prep-plugins.mddocs/adr/README.mddocs/ci-integration.mddocs/configuration.mddocs/os-setup.mdlib/capybara/screenshot/diff/browser_helpers.rblib/capybara/screenshot/diff/screenshot_matcher.rblib/capybara/screenshot/diff/screenshoter.rblib/capybara/screenshot/diff/vcs.rblib/capybara_screenshot_diff.rblib/capybara_screenshot_diff/dsl.rbtest/test_helper.rbtest/unit/screenshot_test.rbtest/unit/screenshoter_test.rbtest/unit/vcs_test.rb
Comment on lines
+71
to
+77
| def apply_to_screenshot!(screenshot) | ||
| screenshot.normalize_css = normalize_css | ||
| screenshot.wait_for_fonts = wait_for_fonts | ||
| screenshot.disable_animations = disable_animations | ||
| screenshot.hide_caret = hide_caret | ||
| screenshot.blur_active_element = blur_active_element | ||
| end |
There was a problem hiding this comment.
Persist css/js assignments from configure_consistency blocks.
At Line 119, callers can mutate or assign config.css / config.js. But apply_to_screenshot! (Lines 71-77) never writes those fields back, so assignment-style overrides are dropped silently.
💡 Proposed fix
def apply_to_screenshot!(screenshot)
+ screenshot.custom_stylesheets = Array(css)
+ screenshot.custom_scripts = Array(js)
screenshot.normalize_css = normalize_css
screenshot.wait_for_fonts = wait_for_fonts
screenshot.disable_animations = disable_animations
screenshot.hide_caret = hide_caret
screenshot.blur_active_element = blur_active_element
endAlso applies to: 110-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/capybara_screenshot_diff.rb` around lines 71 - 77, apply_to_screenshot!
currently copies flags like normalize_css but never persists any config.css or
config.js assignments made in configure_consistency blocks, so assignment-style
overrides are lost; update apply_to_screenshot! (and the same logic around
configure_consistency usage) to also write any configured css and js back onto
the screenshot (e.g., if config.css or config.js are non-nil/non-empty, set
screenshot.css = config.css and screenshot.js = config.js) so both mutation and
full-assignment overrides are honored; reference apply_to_screenshot!,
configure_consistency, config.css and config.js when making the change.
Comment on lines
+23
to
24
| system(git_env, "git", "-C", root_path, "lfs", "smudge", in: tmp_path, out: checkout_path.to_s, err: File::NULL) | ||
| end |
There was a problem hiding this comment.
Propagate git lfs smudge failures to success.
On Line 23, the git lfs smudge exit status is ignored. If smudge fails, checkout_vcs can still return true, which masks checkout errors.
🔧 Proposed fix
if success
- system(git_env, "git", "-C", root_path, "lfs", "smudge", in: tmp_path, out: checkout_path.to_s, err: File::NULL)
+ success = system(git_env, "git", "-C", root_path, "lfs", "smudge", in: tmp_path, out: checkout_path.to_s, err: File::NULL)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| system(git_env, "git", "-C", root_path, "lfs", "smudge", in: tmp_path, out: checkout_path.to_s, err: File::NULL) | |
| end | |
| success = system(git_env, "git", "-C", root_path, "lfs", "smudge", in: tmp_path, out: checkout_path.to_s, err: File::NULL) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/capybara/screenshot/diff/vcs.rb` around lines 23 - 24, The call to
system(git_env, "git", "-C", root_path, "lfs", "smudge", in: tmp_path, out:
checkout_path.to_s, err: File::NULL) inside checkout_vcs currently ignores the
exit status, allowing checkout_vcs to return true even when git lfs smudge
fails; capture the return value (e.g. result = system(...)) and if it is false
or nil propagate the failure by marking success as false or returning false
immediately (and optionally log the error), referencing the existing variables
and method names (checkout_vcs, git_env, root_path, tmp_path, checkout_path) so
the checkout failure is not silently ignored.
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\n- add OS setup guide + ADR for deferred screenshot prep plugins\n- unify consistency config with custom CSS/JS injection\n- update GitHub setup action to install fonts and disable font hinting\n- keep README minimal and move advanced details into docs\n- harden VCS checkout against external git env overrides\n\n## Testing\n- bin/ci
Summary by Sourcery
Standardize screenshot rendering consistency across environments by adding configurable DOM normalization, font-loading waits, and cross-OS setup guidance, while hardening VCS checkout against external Git environment overrides.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit
Release Notes
New Features
:defaultand:off) viaenable_consistent_screenshots!andconfigure_consistencyDocumentation