refactor: extract viewport preparation seam (Capture::Viewport) - #215
Conversation
Guards for 5.5-lite item 6: the window-size guard also fires on the compare-free #capture path, and the check runs exactly once per capture (never per stability retry) - the cadence the seam must preserve. Claude-Session: https://claude.ai/code/session_014BQJX6eWzBj2UTm5zQsjEs
5.5-lite item 6: the raise-only window-size guard moves from ScreenshotMatcher#check_window_size! into the module function SnapDiff::Capture::Viewport.prepare! - the per-capture viewport seam. No new class hierarchy (KISS): a module function, called once per capture, outside the stability retry loop (which is where the check already ran - the extraction is behavior-equivalent by construction). The anchor: nil parameter is accepted and threaded but unused today; it is the v3 seam for scroll-position preservation and element-anchored capture (window resize loses the scroll position of the element under comparison). No scrolling is implemented here. Claude-Session: https://claude.ai/code/session_014BQJX6eWzBj2UTm5zQsjEs
Reviewer's GuideExtracts a dedicated SnapDiff::Capture::Viewport seam for per-capture viewport preparation, moving the window-size guard out of ScreenshotMatcher and pinning its cadence and behavior with focused tests. Sequence diagram for per-capture viewport preparationsequenceDiagram
participant ScreenshotMatcher
participant Viewport as Capture::Viewport
participant BrowserHelpers
participant Screenshoter
ScreenshotMatcher->>Viewport: prepare!(window_size, anchor: nil)
Viewport->>BrowserHelpers: window_size_is_wrong?(expected_window_size)
alt window size mismatch
Viewport-->>ScreenshotMatcher: raise WindowSizeMismatchError
else window size valid
Viewport-->>ScreenshotMatcher: return
ScreenshotMatcher->>Screenshoter: capture screenshot
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 51 seconds 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 |
What
v2 sequence, 5.5-lite item 6 (per the panel amendment): extract the per-capture viewport preparation into a dedicated seam.
SnapDiff::Capture::Viewport.prepare!(expected_window_size, anchor: nil)— a module function (KISS, no new class hierarchy). It carries the raise-only window-size guard that lived inline asScreenshotMatcher#check_window_size!.ScreenshotMatcher#build_screenshot_assertionand#capturenow callCapture::Viewport.prepare!(Capybara::Screenshot.window_size, anchor: nil)at the same point in the flow wherecheck_window_size!ran.anchor:is accepted and threaded but unused today — it is the v3 seam for scroll-position preservation / element-anchored capture (window resize loses the scroll position of the element under comparison). No scrolling is implemented here.Behavior equivalence (the hoist check)
The spec required stopping if hoisting the prepare outside the stability retry loop changed observable call counts. Verified by reading and by a pinning guard: the check already ran once per capture, in
ScreenshotMatcher, before the screenshoter — never insideStableScreenshoter's retry loop. Nothing inScreenshoter/StableScreenshotertouches window size (the adapters' setup-timeresize_window_if_neededis untouched, out of scope). So the extraction is behavior-equivalent by construction: same operation, same order, same cadence, same error message.Gate evidence
Guard commit (d80897d) pins the cadence first:
#captureraisesWindowSizeMismatchErroron wrong window size (was only covered on the assertion path)Mutation check (run twice — against the inline guard and against the final seam shape):
Capture::Viewport.prepare!calls →screenshot_matcher_test.rb: 3 failuresTests:
rake test:unit: 444 runs, 1277 assertions, 0 failures, 0 errors (439 baseline + 2 cadence guards + 3 directViewporttests)rake test: 477 runs, 0 failures, 6 skips (the 2 screenshot-report comparison entries are pre-existing on master99c71a6, verified by running the baseline)standardrb: 138 files, no offensesNot built (deliberately)
Capturestrategy objects, noViewportclass or instances — module function onlyscreenshot_assertion.rb/comparison_result.rb(owned by PR refactor: explicit baseline archiving + #inspect on assertion/result #214)🤖 Generated with Claude Code
Summary by Sourcery
Extract per-capture viewport preparation from ScreenshotMatcher while preserving window-size validation behavior and establishing a seam for future anchored capture support.
New Features:
Bug Fixes:
Enhancements:
Tests: