Add page-scope style comparison (matcher + artifact) - #32
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds page-scope ChangesPage-scope style comparison
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change adds page-scope style diagnostics without altering pixel comparison or merge-gate behavior; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Test as Playwright test
participant Matcher as toMatchFigma
participant Converter as expectStyleToSnapshot
participant Comparator as compareStyles
participant TopIssues as topIssues
Test->>Matcher: provide page-scope styleChecks
Matcher->>Converter: convert checkpoint expectStyle
Matcher->>Comparator: compare selected element style
Comparator->>TopIssues: return selector-tagged style issues
Matcher->>Test: preserve passing pixel comparison
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR adds page-scope style checkpoint comparisons using baked expectations and tags resulting diagnostics with their originating selectors. It also addresses the previously reported region-routing and matcher-budget issues.
|
| Filename | Overview |
|---|---|
| packages/playwright/src/matchers/to-match-figma.ts | Adds bounded page-checkpoint style comparison and correctly separates page and region behavior without leaving a blocking prior finding. |
| packages/verify/src/figma-node-style.ts | Adds a focused conversion from baked contract styles to the existing comparison snapshot representation. |
| packages/contracts/src/score.ts | Extends persisted top issues with an optional selector while preserving compatibility for existing issues. |
| packages/verify/src/types.ts | Mirrors the optional checkpoint selector in the runtime TopIssue contract. |
| packages/playwright/tests/to-match-figma.test.ts | Covers page checkpoints, missing expectations, empty checks, and the corrected region fallback behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[toMatchFigma] --> B[Resolve baseline and capture page]
B --> C[Run pixel comparison]
C --> D{Region selector present?}
D -->|Yes| E[Compare region style when Figma style exists]
D -->|No| F[Compare baked page checkpoints]
E --> G[Apply remaining timeout budget]
F --> G
G --> H[Merge non-blocking style issues]
H --> I[Attach images and score artifact]
Reviews (5): Last reviewed commit: "Enhance style comparison with page-scope..." | Re-trigger Greptile
… scope Greptile review on #32 flagged two real bugs in the page-scope style-check wiring: - captureCheckPointStyleIssues (and region's own captureStyleIssues) ran outside the matcher's withTimeout boundary, so a stale/missing selector's page.$eval could hang past the matcher's own configured timeout instead of degrading to no style issues like every other style-capture failure. - the styleIssues conditional fell through to the page check-point branch whenever a region call's figmaStyle was unavailable for any reason -- including the retryable-cached-baseline fallback in baseline.ts, whose ResolvedBaseline omits figmaStyle entirely rather than an empty object. A region call could then pick up unrelated check-point diagnostics. Fixes: gate the check-point branch strictly on `!options.selector`, and wrap both style-check paths in a shared timeout that degrades to no issues on expiry, consistent with captureStyleIssues' own best-effort semantics. Added a regression test that reproduces the cached-baseline fallback via a real two-call retryable-500 sequence and asserts no check-point diagnostic leaks into the region result.
0100a13 to
793ff37
Compare
…gging This commit introduces the ability to perform style comparisons at page-scope using defined check-points, allowing for multiple style checks to be executed per page. Each style issue is now tagged with its corresponding selector, improving the clarity of style mismatch reporting. The `expectStyleToSnapshot` function has been added to facilitate the conversion of expected styles into a format suitable for comparison. Additionally, tests have been implemented to validate the new functionality, ensuring that style issues are accurately captured and reported based on the defined check-points.
2c03fce to
c878ca1
Compare
Summary
toMatchFigmacalls withstyleChecksnow run onecompareStylesper declared check-point, each against its own bakedexpectStyle(no live per-checkpoint Figma re-fetch at verify time).expectStyleToSnapshot()(packages/verify/src/figma-node-style.ts) to bridge the contract's bakedExpectStyle(0-255 RGB + separate alpha,fontSizePx) into theStyleSnapshotshapecompareStylesconsumes.selector(new optionalTopIssue.selector/topIssueSchema.selectorfield) so multiple check-points stay distinguishable once merged intotopIssues.topIssuesthe same way region-scope's existing bake-in is, anddone-gateis unaffected since it only blocks onkind === "residual".expectStylenever baked at authoring time (failed Figma fetch) is skipped rather than compared against nothing.styleChecksbehave exactly as before (pixel-only); region-scope's existing single-selector style comparison is untouched.Closes #27
Test plan
expectStyleToSnapshot(empty input, each field, color+colorProperty pairing, colorProperty omitted, combined fields) —packages/verify/tests/figma-node-style.test.tspackages/playwright/tests/to-match-figma.test.ts:expectStyleproduces no issuestyleChecksarray produces no style issuespnpm -r typecheckandoxlintcleanSummary by CodeRabbit
New Features
Documentation
Tests