refactor: drop the dead anchor: kwarg + gate the legacy trees as alias-only (ADR-008 step 7) - #229
Conversation
…s-only (ADR-008 step 7) Part A -- kill `anchor:`. Viewport.prepare! accepted an `anchor:` kwarg no caller ever passed non-nil (both ScreenshotMatcher call sites hardcoded `anchor: nil`), plus a test defending its acceptance. Adding an optional kwarg later is 100% non-breaking, so reserving it now bought nothing and shipped a test guarding dead code. v3's scroll-preservation work designs the real contract. Part B -- the CI gate. New unit test asserts every .rb file under lib/capybara/ and lib/capybara_screenshot_diff/ is nothing but requires, namespace reopening, constant aliases and one-line forwarders into SnapDiff. That is what keeps 3.0 a `git rm` instead of a refactor. One allowlisted file (config_legacy.rb, with a written reason), narrowed by pinning its method inventory so new logic there still reds.
Reviewer's GuideRemoves an unused Sequence diagram for viewport preparation without the dead anchor keywordsequenceDiagram
participant Matcher as SnapDiff::ScreenshotMatcher
participant Viewport as SnapDiff::Capture::Viewport
participant Browser as BrowserHelpers
Matcher->>Viewport: prepare!(expected_window_size)
Viewport->>Browser: window_size_is_wrong?(expected_window_size)
Browser-->>Viewport: size mismatch or valid
Viewport-->>Matcher: return or raise WindowSizeMismatchError
Flow diagram for the legacy tree alias-only CI gateflowchart TD
Start["rake test:unit"] --> Discover["Discover legacy .rb files and root entry files"]
Discover --> Parse["Strip comments/blanks and fold continuations"]
Parse --> Validate["Validate allowed declarations and SnapDiff forwarders"]
Validate --> Inventory["Check config_legacy.rb method inventory"]
Inventory --> Result{Pass?}
Result -->|Yes| Green["Unit suite passes"]
Result -->|No| Failure["Report file and offending line"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 3 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
Part A — kill the dead
anchor:kwargSnapDiff::Capture::Viewport.prepare!(expected_window_size, anchor: nil)accepted a kwarg no caller ever passed non-nil — bothscreenshot_matcher.rbcall sites hardcodedanchor: nil— plus a unit test pinning its acceptance and a doc comment promising it to v3.Adding an optional kwarg later is 100% non-breaking, so reserving it now bought nothing and shipped a test defending dead code. v3's scroll-preservation work will design the real contract; the panel found this seam's shape wrong for that anyway (no session access, no resize responsibility).
Deleted: the kwarg, both call-site args, the
@param anchordoc, and the test.Part B — the mechanical CI gate
test/unit/legacy_tree_is_alias_only_test.rb— a plain unit test, so it runs inrake test:unitand in CI with no new workflow wiring.What it asserts. Every
.rbfile underlib/capybara/andlib/capybara_screenshot_diff/(plus both root entry files) contains nothing but:require/require_relative/autoloadmodule/class/class << self/end/private/extend/includeFoo = SnapDiff::Foo)def_delegator(s)defs whose entire body is a single line delegating intoSnapDiffThat last rule is the one that matters: a one-line forwarder is not lost when the file is deleted, real logic is. This is what keeps 3.0 a
git rminstead of a refactor.Mechanism: strip comments/blanks, fold trailing-comma continuations into one logical line, then walk. Failures name the file and the offending line.
The allowlist — the honest state of the tree
One file.
lib/capybara/screenshot/diff/config_legacy.rbSnapDiff::Config::MAPPINGviadefine_method(ADR-008 step 1 — it is delegation, but containsdef/define_method). It also still holds derived config logic that never moved:.active?precedence,.screenshot_areapath assembly, and.default_options— which carries one literal default, the vipstolerance0.001.AVAILABLE_DRIVERSalso lives here on purpose (#227:test_helperreads it at boot,image_compare_teststubs it) — the gate does not push it out.Narrowed with a second cheap assertion:
config_legacy.rb's method inventory is pinned to exactlyactive?,screenshot_area,screenshot_area_abs,self.compare,self.configure,self.default_options. Any new method there reds the gate.Findings (files that did NOT need allowlisting, contrary to the brief)
lib/capybara_screenshot_diff.rbneeds no exception. Its error-class eager aliases are plain constant assignments and it hosts nodefs at all — the session forwarders actually live inlib/capybara_screenshot_diff/screenshot_assertion.rb.screenshot_assertion.rbandstatic.rbneed no exception either. Every method in them is a one-lineSnapDiff.*forwarder, so the shape rule accepts them without a per-file carve-out.config_legacy.rb. ADR-008 step 1 moved config storage toSnapDiff::Configbut left derived config logic behind. ADR-008's "alias-only" claim holds for the whole tree except this file. Worth amending the ADR to say so.Gate-check evidence
Three mutation probes, each reverted with a targeted edit (no
git checkout):1. One-line
defin a non-allowlisted file (lib/capybara_screenshot_diff/static.rb):2. Realistic multi-line method (
lib/capybara/screenshot/diff/region.rb):3. New method in the ALLOWLISTED file (
config_legacy.rb) — proves the allowlist is not a blanket pass:After all three reverts (
git diff --statclean of probes):2 runs, 5 assertions, 0 failureson the untouched tree. The gate alsorefute_emptys the file glob, so it cannot pass vacuously.Test numbers
rake test:unitrake teststandardrb lib test529 = 528 − 1 (deleted anchor test) + 2 (gate tests).
No CHANGELOG or version.rb changes.
🤖 Generated with Claude Code
Summary by Sourcery
Remove the dead viewport anchor seam and enforce that legacy compatibility code remains removable alias-only plumbing.
Enhancements:
anchor:parameter and its associated test and call-site arguments.Tests: