refactor: derived config logic moves to SnapDiff::Config (ADR-008 step 7b) - #230
Merged
Conversation
…p 7b)
config_legacy.rb was the last file in the v1 trees holding real logic.
Step 1 moved config STORAGE to SnapDiff::Config but left the DERIVED
values behind, so deleting lib/capybara/ at 3.0 would have lost
behaviour rather than being a `git rm`.
Moved into SnapDiff::Config as instance methods:
- #active? (was Capybara::Screenshot.active?)
- #screenshot_area (was Capybara::Screenshot.screenshot_area)
- #screenshot_area_abs
- #default_options (was Diff.default_options, incl. the vips
tolerance 0.001 literal)
Inverted so the canonical names hold the bodies:
- SnapDiff.compare now builds the Comparison; Diff.compare forwards.
- SnapDiff.start now does the two-arg yield; Diff.configure forwards.
The Config::MAPPING accessor generator moved to snap_diff/config.rb
alongside the storage it delegates to -- same reason legacy_shims.rb
generates the legacy constants from the snap_diff side: the generator
is code, and the v1 trees must stay alias-only.
Net effect: the alias-only gate's ALLOWED_WITH_CODE allowlist is now
EMPTY, and config_legacy.rb is checked by the general rule like every
other legacy file. The pinned method inventory that narrowed its
exemption goes with the exemption.
default_options[:wait] stays a call-time read of
Capybara.default_max_wait_time -- freezing it into Config#initialize
reds all four #223 timing guards.
Found while moving: the active? precedence rule had no test. Replacing
the whole expression with a bare `enabled` kept all 529 unit tests
green. Its truth table is now pinned through both the canonical method
and the legacy forwarder.
AVAILABLE_DRIVERS stays in config_legacy.rb (it is a bare constant
assignment, so alias-shaped and never a gate blocker). Moving the
storage to SnapDiff::Drivers was tried and reverted: eagerly it needs
Utils, which requires Drivers back; lazily it breaks
image_compare_test's published stubbing point, because Utils reads
Drivers.available and would no longer see a stubbed legacy constant.
Legacy public surface verified byte-identical: singleton_methods +
parameters for Capybara::Screenshot and Capybara::Screenshot::Diff are
unchanged. On the SnapDiff side, .compare gains an explicit signature
(was a `(...)` pass-through) and .start loses its &block capture (now a
bare yield) -- both a consequence of the bodies landing there, and both
call-compatible.
rake test:unit 530 runs / 0 failures; rake test 563 runs / 0 failures.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
Reviewer's GuideDerived configuration logic and comparison/configuration entry points are moved from legacy Capybara::Screenshot::Diff modules into SnapDiff::Config and SnapDiff, leaving the v1 tree alias-only and enforcing an empty allowlist, while preserving public APIs and adding tests around previously unguarded behaviour. Sequence diagram for canonical comparison and configuration flowssequenceDiagram
participant Caller
participant Diff as Legacy Diff
participant SnapDiff
participant Config as SnapDiff::Config
participant Comparison
Caller->>Diff: compare(baseline_path, current_path, **options)
Diff->>SnapDiff: compare(baseline_path, current_path, **options)
SnapDiff->>Config: default_options()
Config-->>SnapDiff: defaults
SnapDiff->>Comparison: new(current_path, baseline_path, merged_options)
Comparison-->>Caller: comparison
Caller->>Diff: configure()
Diff->>SnapDiff: start()
SnapDiff-->>Diff: yield Capybara::Screenshot, Diff
Diff-->>Caller: configuration block
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change moves derived configuration and comparison behavior into ChangesConfiguration forwarding
Estimated code review effort: 3 (Moderate) | ~20 minutes ✨ 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 |
pftg
added a commit
that referenced
this pull request
Aug 23, 2026
Twenty-nine reads across twelve core files went through Capybara::Screenshot / Capybara::Screenshot::Diff -- the legacy VIEW of a storage that has lived in SnapDiff::Config since #230. Same storage, canonical name; behaviour is unchanged and the legacy delegators stay for users. Two follow-ons: - browser_helpers dropped its respond_to?(:window_size) guard: that existed because the mattr_accessor might not be installed yet, and Config always has the attribute. - the fail_if_new error message now tells users 'SnapDiff.config.fail_if_new = false' -- it named an accessor that 3.0 deletes. Tests that STUBBED the legacy accessors now stub SnapDiff.config: a delegator write still reaches the storage, but a stubbed delegator method does not, so those stubs were silently no-ops against the new reads. The legacy accessors keep their own coverage in snap_diff_config_test.rb (every MAPPING writer) and config_default_timing_test.rb.
pftg
added a commit
that referenced
this pull request
Aug 23, 2026
…ess) (#235) * test: reverse gate -- no core file may depend on the v1 trees legacy_tree_is_alias_only_test.rb proves the v1 trees hold no logic. Nothing proved the mirror image, and it was false: 64 core->legacy edges (6 backward requires, AVAILABLE_DRIVERS, ~55 config reads through the legacy view) meant `git rm lib/capybara*` would break the gem. This gate reports every one as file:line and starts with them all allowlisted, so it lands green and the following commits can only shrink it. Red on master with an empty allowlist; mutation-checked by adding a Capybara::Screenshot::Diff reference to region.rb (gate named it). Comments are ignored (history, not dependency); strings are not. * refactor: driver detection lives on SnapDiff::Drivers SnapDiff::Drivers.available is documented canonical API, but the list it read was defined only in config_legacy.rb, so ruby -Ilib -e 'require "snap_diff/drivers"; SnapDiff::Drivers.available' raised NameError: uninitialized constant Capybara::Screenshot::Diff. It now answers. Detection moved to Drivers.detect_available and runs at drivers.rb load (same load moment in every entry-point path); SnapDiff::Utils .detect_available_drivers one-lines into it, so the documented Utils name and its tests are unchanged. config_legacy keeps AVAILABLE_DRIVERS as an eager same-object alias -- test_helper still reads it at boot, and namespace_forwarding_test still pins assert_same. The stubbing point moves with the value: image_compare_test now stubs SnapDiff::Drivers::AVAILABLE_DRIVERS. Stubbing the legacy alias would only rebind the alias, which is the whole point of cutting the edge. utils.rb requires drivers and drivers now needs Utils at call time, so drivers.rb requires utils at the bottom -- either file can be required first. * refactor: core config reads go to SnapDiff.config, not the legacy view Twenty-nine reads across twelve core files went through Capybara::Screenshot / Capybara::Screenshot::Diff -- the legacy VIEW of a storage that has lived in SnapDiff::Config since #230. Same storage, canonical name; behaviour is unchanged and the legacy delegators stay for users. Two follow-ons: - browser_helpers dropped its respond_to?(:window_size) guard: that existed because the mattr_accessor might not be installed yet, and Config always has the attribute. - the fail_if_new error message now tells users 'SnapDiff.config.fail_if_new = false' -- it named an accessor that 3.0 deletes. Tests that STUBBED the legacy accessors now stub SnapDiff.config: a delegator write still reaches the storage, but a stubbed delegator method does not, so those stubs were silently no-ops against the new reads. The legacy accessors keep their own coverage in snap_diff_config_test.rb (every MAPPING writer) and config_default_timing_test.rb. * refactor: core requires point at snap_diff/*, not the v1 forwarders dsl.rb, reporters/html.rb and screenshot_matcher.rb pulled their dependencies through capybara/screenshot/diff/* and capybara_screenshot_diff/* forwarders -- files whose only content is a require of the snap_diff/* unit the core actually wanted. Point at the units directly. snap_diff.rb's own two backward requires are the last ones left and go with the v1-surface consolidation. * refactor: one file holds the v1 surface, and the core names none of it Three legacy edges were left in the canonical core: - lib/snap_diff/config.rb DEFINED the Capybara::Screenshot::Diff module skeleton so Config::MAPPING could name it, and generated the legacy mattr_accessors. After a 3.0 `git rm` that would have survived: the core would still define a phantom v1 namespace, so adopters' `defined?(Capybara::Screenshot::Diff)` checks would keep passing. - lib/snap_diff.rb required config_legacy.rb and image_compare.rb -- the only reason being that those forwarders happened to install the v1 surface for bare `require "snap_diff"` processes. - SnapDiff.start yields the two legacy holders, so it cannot outlive them. All three move to lib/snap_diff/legacy_shims.rb, which now holds the whole v1 surface as code: const_missing forwarders, CONFIG_MAPPING and its generator, the derived forwarders that were in config_legacy.rb (Screenshot.active?, Diff.configure/.compare/.default_options), SnapDiff.start, and the eager AVAILABLE_DRIVERS / Comparison aliases. config_legacy.rb and image_compare.rb are now requires only. Config keeps the setting list as Config::SETTINGS and names nothing legacy; a new test pins CONFIG_MAPPING.keys == SETTINGS so the split cannot drift into storage with no accessor (or the reverse). Behaviour is unchanged, including the awkward part: bare `require "snap_diff"` still answers Capybara::Screenshot.active?, Diff.default_options, Diff::AVAILABLE_DRIVERS and Diff::Comparison, exactly as it did when it reached through the v1 forwarders (verified before/after in a fresh process). The reverse gate's allowlist is now EMPTY. * test: alias-only gate demands a real single-expression forwarder The rule was `body.include?("SnapDiff")` plus a following `end`, which accepted anything that merely mentioned the canonical namespace: def x SnapDiff.config.a ? b : c # a conditional -- real behaviour end def x; SnapDiff.config.a; File.write(...); end # two statements Both are now rejected: a semicolon is never alias-shaped (it is how several statements, or a whole def, hide inside one "line"), and a forwarder body must match the whole of FORWARDER_BODY -- one method-call chain rooted at SnapDiff, at most one argument list and one block. Mutation-checked with exactly those two shapes plus a genuine forwarder, which still passes. * build: gemspec resolves the version from snap_diff/version The gemspec required capybara/screenshot/diff/version and read Capybara::Screenshot::Diff::VERSION -- a build-time dependency on a file 3.0 deletes, so `gem build` would have failed the moment it did. Same value (the legacy constant is an alias of SnapDiff::VERSION), one fewer 3.0 blocker. The legacy constant stays for adopters who read it. Found by the 3.0 dry run; fixed here because it is one line and in scope. * fix: legacy VERSION vanished from six entry points Capybara::Screenshot::Diff::VERSION raised NameError -- and defined? returned nil -- under "snap_diff", snap_diff/dsl, both integrations, snap_diff/static and, worst, the LEGACY capybara_screenshot_diff/dsl. Cause: the constant was assigned by capybara/screenshot/diff/version.rb, and the 3.0-readiness pass stopped the core requiring that forwarder. legacy_shims deliberately omits VERSION from its const_missing map -- it is one of the documented eager exceptions -- so nothing filled the gap and adopters' `defined?` feature detection silently went false. Assign it in legacy_shims next to the other eager aliases: that file is required by every entry point, canonical and legacy, so it is the only place the eager exceptions can actually be eager. version.rb drops the assignment (a second one is a duplicate-constant warning, not a safety net) and becomes a require. The suite could not have caught it: EAGER_USER_FACING was only probed for the four legacy entry points, and VERSION was not in the list at all. New EAGER_EVERYWHERE probe covers all 15 entry points including capybara_screenshot_diff/dsl, which is in none of the existing lists -- exactly why it was the legacy entry that lost VERSION unnoticed. Red without the alias, naming all six. Also corrects two now-false claims in the legacy_shims header: it said AVAILABLE_DRIVERS was aliased in config_legacy.rb (it is aliased in this file) and that VERSION/Comparison were defined by their own forwarder files (nothing requires those anymore). * test: reverse gate catches require_relative escapes LEGACY_REQUIRE anchored straight on the opening quote, so require_relative "../capybara/screenshot/diff/version" passed the gate while genuinely loading the v1 file (confirmed via $LOADED_FEATURES). Every core file sits one directory below lib/, so that is a one-line escape, not a hypothetical. Allow an optional (\.{1,2}/)* -- mutation-checked with exactly that line. Two smaller fixes while here: the header claimed comments are ignored when only WHOLE-LINE ones are (trailing notes on a live code line are scanned, which is the behaviour worth keeping -- the header now says so), and the stale-allowlist check now reports a deleted allowlisted file instead of raising Errno::ENOENT on it. * test: forwarder rule takes simple pass-through args only The tightened rule rejected ternaries and semicolons but left `(.*)` and `{ .* }` unbounded, so both of these still passed as "forwarders": SnapDiff.config.x(File.exist?("/etc/passwd") ? raise("boom") : ENV.fetch("HOME")) SnapDiff.config.tap { |c| File.write("/tmp/pwned", c.inspect); exit 1 } The block case also dodged the semicolon check, because the walk steps over a def's body line without re-examining it -- that scan now runs over every line up front. An argument list is now names, commas, splats and keyword colons, or Ruby's `...` forwarding. No parens, so no nested call; no `.`, so no bare receiver call; no `?`/quote/`=`, so no conditional, literal or assignment. The block form is gone entirely: nothing in these trees has a def left, and an unbounded block is exactly the hole above. Mutation-checked with both shapes above (rejected, the block twice) plus three genuine forwarders that must not false-positive: a bare chain, an argument pass-through, and CapybaraScreenshotDiff.serve(...) -- which the first draft of this rule did reject. * docs: flag the two beta moves that fail silently Both are read-identical and only break on write, so nothing warns: - stubbing Capybara::Screenshot::Diff::AVAILABLE_DRIVERS now only rebinds an alias -- the gem reads SnapDiff::Drivers::AVAILABLE_DRIVERS, so a downstream test stubbing it to [] stops exercising the no-drivers path and passes for the wrong reason; - SnapDiff::Config::MAPPING is gone mid-beta, split into Config::SETTINGS and the @api-private LegacyShims::CONFIG_MAPPING.
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.
ADR-008 step 7b.
config_legacy.rbwas the last file in the v1 trees holding real logic. Step 1 moved config storage toSnapDiff::Configbut left the derived values behind, so deletinglib/capybara/at 3.0 would have lost behaviour instead of being agit rm. This finishes the move.What moved where
Into
SnapDiff::Configas instance methods, next to the storage they read:Capybara::Screenshot.active?SnapDiff::Config#active?Capybara::Screenshot.screenshot_areaSnapDiff::Config#screenshot_areaCapybara::Screenshot.screenshot_area_absSnapDiff::Config#screenshot_area_absDiff.default_options(incl. the vips0.001tolerance literal)SnapDiff::Config#default_optionsInverted so the canonical names hold the bodies rather than forwarding into the legacy tree:
SnapDiff.comparenow builds theComparison;Diff.compareforwards to it.SnapDiff.startnow does the two-arg yield;Diff.configureforwards to it.The
Config::MAPPINGaccessor generator moved fromconfig_legacy.rbtosnap_diff/config.rb, alongside the storage it delegates to — same side of the fence aslegacy_shims.rb, which already generates the legacy constants from thesnap_diffside, for the same reason: the generator is code, and the v1 trees must stay alias-only.Everything left in
config_legacy.rbis six one-line forwarders plus theAVAILABLE_DRIVERSconstant.Win condition: the gate allowlist is now EMPTY
config_legacy.rbwas the only entry. It is now checked by the general rule like every other legacy file, and the pinned method inventory that narrowed its exemption goes with the exemption (replaced by an assertion that the allowlist stays empty).Proof the empty allowlist actually guards — a
def self.sneaky_logicwith a real body added toconfig_legacy.rb:Mutation evidence
(i) break
active?precedence — replaced the whole expression with a bareenabled:So a guard was added (
snap_diff_config_test.rb) pinning the full truth table through both the canonical method and the legacy forwarder. Re-running the same mutation:The other half of the rule (the nil fall-through) was already well covered — mutating to a bare
screenshot_enabledreds ~14 tests across the suite, since screenshots go inactive everywhere.(ii) freeze
default_options[:wait]intoConfig#initialize— the #223 timing guards red on all four entry points:config_default_timing_test.rbis untouched by this PR.(iii) break
screenshot_areapath assembly — dropped the os/driver segments:AVAILABLE_DRIVERS: stays put, and it was never a gate blocker
It is a bare constant assignment, which the gate's
ALIAS_SHAPESalready accepts as a constant alias — so it never stood between us and the empty allowlist.Moving the storage to
SnapDiff::Driverswas tried anyway, and reverted on two independent blockers:AVAILABLE = Utils.detect_available_drivers.freeze) it needsUtils, andutils.rbrequiresdrivers.rbback →uninitialized constant SnapDiff::Drivers::Utils (NameError)at load.@available ||= ...) it loads fine but silently breaks the published stubbing point, becauseUtilsreadsDrivers.availableand would no longer see a stubbed legacy constant:#227's reasoning holds. Documented in the allowlist comment so the next reader does not retry it.
Public surface
singleton_methods(false)+parametersdiffed before/after (before generated from a pristinegit archive HEADtree). Both legacy modules are byte-identical — zero lines of the diff touchCapybara::ScreenshotorCapybara::Screenshot::Diff, includingcompare[[:req, :baseline_path], [:req, :current_path], [:keyrest, :options]]andconfigure[].The only changes are on the canonical side, as a direct consequence of the bodies landing there — both call-compatible:
Diff.configurekeeps a bareyieldrather than an explicit&blockspecifically to keep its published arity at[].Tests
rake test:unitrake teststandardrbNet test delta is exactly +1: the
active?truth-table guard added, the pinned-inventory test replaced one-for-one by the empty-allowlist assertion. No existing test was modified.🤖 Generated with Claude Code
Summary by Sourcery
Complete the configuration migration by moving derived behavior and canonical entry points into SnapDiff while leaving the legacy APIs as alias-only compatibility shims.
Enhancements:
Tests:
Summary by CodeRabbit
Refactor
Tests