feat: default blur_active_element and hide_caret to true#161
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideSets default values for screenshot-diff configuration options Class diagram for updated Capybara_Screenshot configuration defaultsclassDiagram
class Capybara_Screenshot {
<<module>>
+add_driver_path
+add_os_path
+blur_active_element = true
+enabled
+hide_caret = true
+root
+stability_time_limit
+window_size
}
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 25 minutes and 8 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 (2)
📝 WalkthroughWalkthroughTwo configuration flags in the Capybara::Screenshot module— Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
blur_active_elementandhide_caretnow default totrueinstead ofnil, double-check any internal conditionals that distinguishnilfromfalse(e.g.,if config.blur_active_element.nil?) to ensure they still behave correctly with the new default.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `blur_active_element` and `hide_caret` now default to `true` instead of `nil`, double-check any internal conditionals that distinguish `nil` from `false` (e.g., `if config.blur_active_element.nil?`) to ensure they still behave correctly with the new default.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/capybara_screenshot_diff.rb (1)
30-32:nilstill disables these flags, which weakens the new default-true behavior.On Line [30] and Line [32], defaults are now
true, but downstream checks are truthy-based, so assigningnilstill disables both features. Since current tests reset these flags tonil, this can silently opt out after a reset.Consider treating only explicit
falseas disabled (e.g.,!= falseinScreenshoter#prepare_page_for_screenshot) or normalizingnilback totrue.🤖 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 30 - 32, The new defaults for mattr_accessor flags blur_active_element and hide_caret are true but downstream checks treat any falsy value (including nil) as disabled; update the logic so only an explicit false disables them by either normalizing the flags to true (e.g., coerce nil to true after reset) or change the checks in Screenshoter#prepare_page_for_screenshot to treat the flags as enabled unless they are exactly false (use a `!= false` style check), ensuring the feature remains enabled after tests reset them to nil; target the mattr_accessor declarations (blur_active_element, hide_caret) and the conditionals inside Screenshoter#prepare_page_for_screenshot when applying the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/capybara_screenshot_diff.rb`:
- Around line 30-32: The new defaults for mattr_accessor flags
blur_active_element and hide_caret are true but downstream checks treat any
falsy value (including nil) as disabled; update the logic so only an explicit
false disables them by either normalizing the flags to true (e.g., coerce nil to
true after reset) or change the checks in
Screenshoter#prepare_page_for_screenshot to treat the flags as enabled unless
they are exactly false (use a `!= false` style check), ensuring the feature
remains enabled after tests reset them to nil; target the mattr_accessor
declarations (blur_active_element, hide_caret) and the conditionals inside
Screenshoter#prepare_page_for_screenshot when applying the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4b47b40-e726-4e44-b82f-6218eb18a2d4
📒 Files selected for processing (1)
lib/capybara_screenshot_diff.rb
Every real-world app enables these to prevent flaky screenshots from blinking cursors and focused input elements. Making them default saves boilerplate and matches universal usage pattern. Users can still set false to opt out. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pftg
force-pushed
the
fix/sensible-defaults
branch
from
1d9bfa6 to
6c5840f
Compare
1 participant
Summary
blur_active_elementtotrue(wasnil)hide_carettotrue(wasnil)Both prevent flaky screenshots from blinking cursors and focused inputs. Every real-world app enables them. Users can still opt out with
= false.Note on
:vipsdriver: The current:autodefault already picks:vipsfirst when available, falling back to:chunky_png. No change needed — it's effectively "vips by default."Test plan
CI=true(185 runs, 0 failures)🤖 Generated with Claude Code
Summary by Sourcery
Default key screenshot configuration options to reduce flakiness from focused elements and carets in captured images.
New Features:
Summary by CodeRabbit