fix: expand root fixed width for responsive comparison#213
Merged
Conversation
Add --expand-root flag to visual-compare that replaces the root element's fixed pixel width with width:100% before rendering. This allows the layout to fill the viewport for responsive comparison. Previously, rendering a 375px-fixed HTML at 768px viewport produced a narrow strip inside a wide frame, giving misleading similarity scores (e.g., 41% for mobile-about). - Add expandRootWidth() to visual-compare-helpers (extracted from run-condition.ts removeRootFixedWidth) - Add expandRoot option to VisualCompareOptions and RenderAndCompareOptions - Add --expand-root CLI flag - Update converter.md to use --expand-root for responsive comparisons - Add tests for expandRootWidth Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "visual-compare CLI"
participant Core as "visual-compare core"
participant Helper as "expandRootWidth"
participant Playwright as "Playwright renderer"
User->>CLI: run CLI with --expand-root
CLI->>Core: call visualCompare({ expandRoot: true, ... })
Core->>Core: read input HTML file
Core->>Helper: expandRootWidth(originalHtml)
Helper-->>Core: transformed HTML
Core->>Core: write expanded HTML (code-expanded*.html)
Core->>Playwright: render screenshots from expanded HTML path
Playwright-->>Core: screenshots
Core-->>CLI: comparison results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This was referenced Mar 30, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/commands/visual-compare.ts (1)
8-17: 🛠️ Refactor suggestion | 🟠 MajorDefine the new CLI option in a contract schema, not inline.
This PR extends the external input surface with
expandRoot, but the command is still growing a localz.object(...). Please move the visual-compare options schema intocontracts/and derive the TypeScript type from it so the CLI has one source of truth.As per coding guidelines, "Validate all external inputs with Zod schemas in
contracts/directory" and "Infer TypeScript types from Zod schemas usingz.infer<typeof Schema>".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/visual-compare.ts` around lines 8 - 17, The VisualCompareOptionsSchema currently defined inline should be moved into a contracts module: create a new zod schema (e.g., VisualCompareOptionsSchema) in the contracts directory and export it, then import that schema into src/cli/commands/visual-compare.ts and use z.infer<typeof VisualCompareOptionsSchema> for the CLI option type; ensure the schema includes the expandRoot field and other properties (figmaUrl, figmaScreenshot, token, output, width, height, figmaScale) with the same unions/optionals so the CLI validates external input against the single source-of-truth contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/comparison/visual-compare-helpers.ts`:
- Around line 194-213: expandRootWidth is mistakenly matching "width" substrings
inside other properties and the wrong selector; update it to operate on the
<style> block by first locating the `.root` rule (or the intended root selector)
and then only replace declarations inside that rule (change its `width: Npx` to
`width: 100%` and `min-width: Npx` to `min-width: 0`), rather than using the
global /width:\s*\d+px/ replacement; if finding the `.root` rule via regex
proves brittle, switch to a lightweight CSS parse approach to extract the
`.root` rule and modify its declarations before reinserting into the style block
(keep changes inside the expandRootWidth function).
---
Outside diff comments:
In `@src/cli/commands/visual-compare.ts`:
- Around line 8-17: The VisualCompareOptionsSchema currently defined inline
should be moved into a contracts module: create a new zod schema (e.g.,
VisualCompareOptionsSchema) in the contracts directory and export it, then
import that schema into src/cli/commands/visual-compare.ts and use
z.infer<typeof VisualCompareOptionsSchema> for the CLI option type; ensure the
schema includes the expandRoot field and other properties (figmaUrl,
figmaScreenshot, token, output, width, height, figmaScale) with the same
unions/optionals so the CLI validates external input against the single
source-of-truth contract.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c2b705f-440d-4a2c-9bf2-c0d6a3ff2481
📒 Files selected for processing (6)
.claude/agents/calibration/converter.mdsrc/cli/commands/visual-compare.tssrc/core/comparison/visual-compare-helpers.tssrc/core/comparison/visual-compare.test.tssrc/core/comparison/visual-compare.tssrc/experiments/ablation/run-condition.ts
- Use negative lookbehind (?<![-\w]) to avoid matching width inside min-width/max-width properties - Move VisualCompareCliOptionsSchema to contracts/ for single source of truth - Add edge case tests for min-width and multi-selector scenarios https://claude.ai/code/session_01A2saeyPtMyekth74uEoSmH
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/core/comparison/visual-compare.test.ts (1)
258-264:⚠️ Potential issue | 🟡 MinorAssert the root selector in this regression.
This case currently passes only when
.cardis rewritten and.rootstays at375px, which is the opposite of the feature contract and the test name. Assert selector-specific output so the suite fails untilexpandRootWidth()actually targets the root rule.🧪 Suggested assertion update
it("skips child width when another selector appears first", () => { const html = `<style>.card { width: 200px; } .root { width: 375px; }</style>`; const result = expandRootWidth(html); - expect(result).toContain("width: 100%"); - expect(result).toContain("width: 375px"); - expect(result).not.toContain("width: 200px"); + expect(result).toContain(".card { width: 200px; }"); + expect(result).toContain(".root { width: 100%; }"); + expect(result).not.toContain(".root { width: 375px; }"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/comparison/visual-compare.test.ts` around lines 258 - 264, The test is asserting the wrong selector behavior; update assertions so expandRootWidth actually targets the root rule: call expandRootWidth(html) and assert the output contains a rule for the .root selector with "width: 100%" (and still contains "width: 375px" if you expect the original root value to be preserved somewhere), while asserting the child selector .card remains at "width: 200px" (i.e., do not expect .card to be rewritten to 100%). Locate the test case using expandRootWidth and change the expect(...) checks to specifically look for the .root selector's modified output and the .card selector's unchanged output.src/core/comparison/visual-compare-helpers.ts (1)
200-212:⚠️ Potential issue | 🟠 MajorScope the rewrite to the actual root rule.
The negative lookbehind fixes the
min-width/max-widthsubstring case, but this still rewrites the first standalonewidthin the stylesheet and then zeroes everymin-widthin the whole HTML. With.card { width: 200px; } .root { width: 375px; }, the child still gets mutated while the root stays fixed, and Line 212 will also strip unrelated childmin-widthconstraints or inline styles. Please locate the intended root declaration block first and apply both replacements only inside that block so--expand-rootdoes not keep producing misleading responsive scores.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/comparison/visual-compare-helpers.ts` around lines 200 - 212, Locate the actual root declaration block in the stylesheet string (the block for the intended root selector, e.g., ".root" or ":root") instead of doing global replacements; extract that block from the full style/html content, apply the single standalone width replacement (using the existing replaced/newStyle logic) and the min-width -> 0 replacement only inside that extracted root block, then splice the modified root block back into html/style to produce result, leaving other rules and inline styles untouched (use the existing variables style, newStyle, replaced, and result to implement the scoped edit).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/contracts/visual-compare.ts`:
- Around line 4-13: The VisualCompareCliOptionsSchema currently leaves numeric
coercion and validation to the CLI handler; move coercion and range checks into
the shared Zod contract by updating VisualCompareCliOptionsSchema so width and
height are coerced to numbers (e.g., z.coerce.number()) with the same min/max
constraints used in the CLI, and make figmaScale coerced to a number with its
validation (required range/limits) so the parsed type is numeric
(VisualCompareCliOptions exposes numbers) and the CLI no longer needs manual
Number() conversion or post-parse range checks.
---
Duplicate comments:
In `@src/core/comparison/visual-compare-helpers.ts`:
- Around line 200-212: Locate the actual root declaration block in the
stylesheet string (the block for the intended root selector, e.g., ".root" or
":root") instead of doing global replacements; extract that block from the full
style/html content, apply the single standalone width replacement (using the
existing replaced/newStyle logic) and the min-width -> 0 replacement only inside
that extracted root block, then splice the modified root block back into
html/style to produce result, leaving other rules and inline styles untouched
(use the existing variables style, newStyle, replaced, and result to implement
the scoped edit).
In `@src/core/comparison/visual-compare.test.ts`:
- Around line 258-264: The test is asserting the wrong selector behavior; update
assertions so expandRootWidth actually targets the root rule: call
expandRootWidth(html) and assert the output contains a rule for the .root
selector with "width: 100%" (and still contains "width: 375px" if you expect the
original root value to be preserved somewhere), while asserting the child
selector .card remains at "width: 200px" (i.e., do not expect .card to be
rewritten to 100%). Locate the test case using expandRootWidth and change the
expect(...) checks to specifically look for the .root selector's modified output
and the .card selector's unchanged output.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48d3cbeb-7d81-4324-94a9-bcf08e83bcb5
📒 Files selected for processing (5)
src/cli/commands/visual-compare.tssrc/core/comparison/visual-compare-helpers.tssrc/core/comparison/visual-compare.test.tssrc/core/contracts/index.tssrc/core/contracts/visual-compare.ts
- expandRootWidth now targets only the first CSS rule block instead of replacing the first width match globally across the entire stylesheet - Move numeric coercion/validation into VisualCompareCliOptionsSchema (width, height, figmaScale) — CLI handler no longer needs manual checks - Update test assertions to verify selector-specific behavior https://claude.ai/code/session_01A2saeyPtMyekth74uEoSmH
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
2 participants
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
--expand-rootflag tovisual-compareCLI that replaces root element's fixed pixel width withwidth: 100%before renderingProblem
When comparing a 375px mobile design at 768px viewport, the HTML has
width: 375pxfixed on the root element.visual-compare --width 768sets the browser viewport but the content stays narrow, producing misleading similarity (e.g., mobile-about reported 41% responsive similarity).Before: 375px content in 768px viewport → compared against 768px Figma screenshot → 41%
After:
--expand-rootreplaceswidth: 375pxwithwidth: 100%→ content fills 768px → accurate comparisonChanges
expandRootWidth()added tovisual-compare-helpers.ts(extracted from experimentremoveRootFixedWidth)--expand-rootCLI flag +expandRootoption inVisualCompareOptionsandRenderAndCompareOptions--expand-rootfor all responsive comparisonsexpandRootWidthRelated
Test plan
pnpm test:run— 685 tests passedpnpm lint— cleannpx canicode visual-compare output.html --figma-screenshot screenshot-768.png --width 768 --expand-rooton a mobile fixture🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation