Repository navigation
feat(design-sync): read Figma auto-layout sizing fields (CE-6) - #474
Conversation
CE-16 column widths and the CE-19 DSL spike need Figma's FIXED/HUG/FILL intent, and the parser dropped it. Parse the eight auto-layout child sizing fields onto DesignNode (sizing on every node type, wrap only on auto-layout frames, unknown values to None), carry them through both node serializers, and record sizing_horizontal on ColumnGroup and EmailSection. The DB-cache serializer also gains scale_mode, rotation and effects_summary, which live conversions were losing, so the live path now matches the corpus harness; a sentinel completeness test fails if a future DesignNode field misses either serializer. Re-sync all seven data/debug structure.json files so CI sees the fields; this closes ce-3-fixture-schema-lag-6-10 and empties _KNOWN_LAGGING_KEYS. Converter output is byte-identical (case 8 equal to the base-structure output), A3 identical, fidelity gate unchanged. .secrets.baseline gains the 79 restored image_ref hashes. Closes #424 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
linardsb
left a comment
There was a problem hiding this comment.
Verdict: Approve, with F1 recommended before merge. It is posted as a comment because GitHub does not let an author approve their own PR.
PR #474 review: feat(design-sync): read Figma auto-layout sizing fields (CE-6)
Head 0657538c · Base main @ 56598f36178d545b198313a4c10cf5887eeb6fc2 (live origin/main = the same sha, so the branch is not behind) · Round 1 · Reviewer: fresh code-reviewer agent plus a separate validation agent. The authoring session did not take part.
Summary
The PR reads eight Figma auto-layout sizing fields onto DesignNode. Both node serializers carry them, and the DB-cache serializer also keeps scale_mode, rotation and effects_summary. That last change alters the live path on purpose, and the user decided it. All 7 structure.json fixtures are re-synced. Converter output does not move.
There are no Critical or High findings. Validation is green. Every figure in the PR body was re-derived independently and matches.
Issues
Critical: none
High: none
Medium
F1. sizing_horizontal is lost at the document bridge, and no ledger entry records it. Files: app/design_sync/figma/layout_analyzer.py:179, :284, app/design_sync/email_design_document.py:936-964, :1254.
- What happens: production conversion runs through
document.to_layout_description()(converter_service.py:352,436). On that pathDocumentColumn.from_column_groupandto_column_grouprebuild everyColumnGroupwithoutsizing_horizontal.compute_column_width_fractionsthen reruns on the rebuilt groups (email_design_document.py:1254). - Effect on CE-16: CE-16 plans to feed FIXED/FILL/HUG into
compute_column_width_fractions. On the real path it would always readNoneand fall back to geometry without any error. A RED test written against_build_column_groupswould still pass. This is the same trap as RC-A/RC-B (the inert serializer bridge). - What is already documented: keeping the field off
DocumentSectionis in the plan (:43,:252GOTCHA,:374user decision), and it is correct for this ticket. - What is missing: the consequence. The plan says nothing about it, and neither does the ledger or the "read by CE-16" comments.
.claude/rules/deferred-items.mdcounts this as a code-shape concession, which needs an entry. - Fix: do not add the field to
DocumentSection; the plan's GOTCHA forbids it. Instead:- Add a deferred entry.
code_refs:email_design_document.py:936-964,1254.closes_when: CE-16/CE-26 carriessizing_horizontalacrossfrom_legacy→to_layout_description, with a test on that path. - Reword both comments to say the field is dropped at the document bridge.
- The change is limited to docs and the ledger.
- Add a deferred entry.
Low
F2. A comment in app/design_sync/figma/raw_types.py:120 says "Figma omits each field at its default". The fixtures show otherwise.
- Case 5 carries
layoutGrow 0×91,layoutAlign INHERIT×79,layoutWrap NO_WRAP×86 andFIXED×22. - Only
layoutPositioning(AUTO) and nullminWidth/maxWidthare actually absent. - Risk: a later reader could treat a missing value as FIXED when it really means "not captured".
- Fix: reword the comment.
F3. test_live_cache_path_matches_corpus_load (app/design_sync/tests/test_converter_data_regression.py:602-610) claims more than it checks.
- Its docstring says the path "loses no field". Five fields (
min_width,max_width,layout_positioning,effects_summary,rotation) areNoneon every corpus node, so for them the test only comparesNonewithNone. - The sentinel tests cover
serialize_node→cached_dict_to_nodeand_dataclass_to_dict→_node_from_dict. Neither covers the production pair,serialize_node→ JSON →_node_from_dict(conversion_service.py:59-76). - Fix: run
make_full_design_node()through that pair, or narrow the docstring.
F4. .agents/deferred-items.json:1359 has closed_commit: "pending". This is expected. Stamp the squash SHA after merge with the deferred-items skill (precedent: #355).
F5. The live-path note in the report and PR body leaves out two effects. Both list only _crop_export_id and the warnings. On live data effects_summary also changes:
- the child-vs-frame export choice in
is_small_decoration(layout_analyzer.py:1689-1697), which changes the emitted width/height and export id; - where
_icon_leafstops on a styled wrapper (layout_analyzer.py:1893-1897).
These touch the open ledger entries phase-53f-decorative-image-flag and ce-10-large-styled-icon-wrapper-exports-bare-glyph. The corpus is unaffected, because effects_summary is null on every node (observed).
Validation (observed, PR worktree at 0657538c, 2026-10-05)
| Gate | Result |
|---|---|
| CI, 14 checks | all pass; Ready for review flipped |
Code scanning (Semgrep, CodeQL), refs/pull/474/merge |
0 open alerts |
ruff format + ruff check --fix |
clean, no source rewrites |
| mypy | no issues, 1398 files |
| pyright | 0 errors |
pytest (unit set from make check-full) |
9181 passed, 123 skipped, 7 xfailed, 0 failed |
fidelity-gate |
1 passed, no re-stamp |
| security-check, migration-lint, overlays, numeric lint, golden-conformance (26 passed), flag-audit (0 errors), env drift | all pass |
check-fe |
not run locally: the fresh worktree has no cms/node_modules. The PR has no frontend change, and CI Frontend is green. |
| Targeted design-sync tests (5 files) | 161 passed, 48 skipped, 4 xfailed |
The run re-stamped date: in app/ai/agents/{dark_mode,scaffolder}/skill-versions.yaml to today, and the reviewer reverted both. This PR does not touch those files. The same two files are modified in the main checkout too, so this is a pre-existing side effect of the suite and is outside this ticket.
Numbers pass: every PR-body figure re-derived (observed, independent script)
| Claim | Observed |
|---|---|
data/debug/ +10,749/−995, per-file numstat |
match. Files are in alphabetical order: 10, 5, 6, 7, 8, 9, reframe. |
| PR 22 files, +12,141/−1,072 | match |
| case 7 is 525,405 B | match |
ids and order unchanged, no keys removed, only change is image_ref null→hash on 0/11/23/10/13/17/16 nodes |
match; 0 other value changes |
scale_mode all FILL, 12/11/23/10/13/17/16; effects_summary and rotation 0 |
match |
.secrets.baseline +79 (15+11+16+10+13+14), case 5 keeps 7 |
match |
resync-case-structure.py 7 reproduces the file byte for byte |
match; empty diff |
625/625 frame-like raw nodes carry layoutMode |
match (86/45/145/76/71/109/93) |
d555ece2→0657538c differs only in the plan |
match (+8/−7 in the plan only) |
The make check-full figure in the PR body (9181 passed / 123 skipped) was reproduced at the same head.
Not re-run by this review: byte-identity against expected.html and the A3 before/after scores. Indirect evidence: fidelity-gate passes, and expected.html plus the ladder snapshot are unchanged in the diff.
Constraint pass
- Plan
:252GOTCHA: "do not add toDocumentSection.from_email_section… or the section cache canonical form". This binds the fix for F1, which is why the fix is a ledger entry and not a code change. - No other constraints match.
- Deviations D1–D7 are documented in the plan's AMENDMENTS, so they are not findings.
Deferred items touching this diff
- Closed here:
ce-3-fixture-schema-lag-6-10. Stamp it after merge. - Carried forward:
phase-53g-band-item-spacing-defaults-vs-wrapper-paddingandce-9-peel-row-fixed-width-wrap, as the PR body states. - Now fed live
effects_summary(see F5):phase-53f-decorative-image-flagandce-10-large-styled-icon-wrapper-exports-bare-glyph. - New entry needed: F1.
What's good
- Sizing is read on every node type, outside the frame gate. That is correct, because sizing is a property of the child.
- Unknown values are allow-listed to
None. - The shared
make_full_design_nodesentinel andtest_sentinel_sets_every_fieldmake future serializer drift fail loudly in both pairs. - The fixture re-sync is audited by byte identity and a scripted key classification, and it empties
_KNOWN_LAGGING_KEYS. - Every figure in the PR body carries a provenance tag, and all of them survived independent re-derivation.
Recommendation
Approve. Before merge, fix F1 (ledger entry plus comment rewording, docs only). F2, F3 and F5 are cheap and can go in the same pass. F4 is done after merge.
F1: sizing_horizontal is rebuilt away at the EmailDesignDocument bridge (to_layout_description), so CE-16 would read None on the production path. Kept off DocumentSection per the plan; logged as ledger entry ce-6-sizing-horizontal-dropped-at-document-bridge and named in both layout_analyzer comments. F2: raw_types comment no longer claims Figma omits sizing fields at their defaults; the fixtures carry explicit FIXED/0/INHERIT/NO_WRAP. F3: new test runs the all-fields sentinel through the production cache read (serialize_node -> JSON -> report._node_from_dict). The corpus parity test stays green when min_width, max_width, layout_positioning, effects_summary or rotation is dropped; the new test fails on each. F5: ledger notes on phase-53f-decorative-image-flag and ce-10-large-styled-icon-wrapper-exports-bare-glyph record the two live-only effects_summary effects. make check-full: exit 0, 9182 passed, 123 skipped (observed). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Names the document-bridge drop under the Task 8 GOTCHA and dates the review fixes under AMENDMENTS. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Summary
CE-16 (column widths) and the CE-19 DSL spike need the designer's sizing intent: whether each Figma node is FIXED, FILL or HUG. The parser dropped it, and the committed
structure.jsonfixtures did not carry it either, so CI could not see it. This PR reads and records the sizing fields. It does not change converter output.What changed
feat(design-sync): read Figma auto-layout sizing fields (CE-6)(0657538c)fix(design-sync): address PR #474 review findings (CE-6)(a9bd6391): review F1–F3, F5; see.claude/reports/pr-474-review-fixes.md.docs(design-sync): record PR #474 review round in CE-6 plan(77863d1e).figma/raw_types.py,figma/service.py,protocol.py): eight newDesignNodefields:layout_sizing_horizontal/vertical,layout_grow,layout_align,layout_positioning,layout_wrap,min_width,max_width.layout_wrapis read only on auto-layout frames.None.diagnose/report.py:_node_from_dictand the DB-cache pairservices/_serialization.pycarry all eight. The cache pair also gainsscale_mode,rotationandeffects_summary(see the behaviour note below).ColumnGroupandEmailSectionget asizing_horizontalfield (figma/layout_analyzer.py). Nothing reads it yet; CE-16 will. It is dropped at the document bridge (to_layout_descriptionrebuilds both types without it), so CE-16 must carry it across first: ledgerce-6-sizing-horizontal-dropped-at-document-bridge(review F1). It is deliberately left out ofDocumentSectionand the section-cache key. Both build explicit field lists, so it changes neither the document JSON nor any cache key.data/debug/*/structure.jsonfiles are re-synced from the localraw_figma.jsonsnapshots withscripts/resync-case-structure.py._KNOWN_LAGGING_KEYSis now empty.ce-3-fixture-schema-lag-6-10is closed (closed_commit: "pending")..secrets.baselinegains the restoredimage_refhashes.test_parse_props.py).make_full_design_node). A completeness test fails if a futureDesignNodefield is missing from either serializer.TestSizingCaptureinvariants over every committed case: every node has sizing; wrap appears iff the frame is auto-layout; the root frame is FIXED; root width equalscontainer_width; the live cache read keeps the corpus values.test_every_field_survives_the_production_cache_read: the all-fields sentinel throughserialize_node→ JSON →report._node_from_dict, the pair live conversion reads with (review F3).Large fixture diff: 7
structure.jsonfiles, +10,749/−995 lines (observed,git diff --shortstat origin/main...0657538c -- data/debug/). Per file: 2074+1107+898+2689+1467+1301+1213 = 10,749 added and 177+123+81+229+123+113+149 = 995 removed (derived,--numstat). The whole PR is 22 files, +12,141/−1,072 (observed,--shortstat). Case 7 is now 525,405 B (observed,git show 0657538c:data/debug/7/structure.json | wc -c), above the 500 KBcheck-added-large-filescap. That hook checks only newly added files, and these are modified; it passed on the commit.Validation
observed—make check-fullviarecord-gate.shat77863d1e, clean tree before and after, exit 0, 2026-10-05T08:06:00Z → 08:11:34Z (.claude/last-gate.json,short_gate: false):9182 = 9181 at
0657538c+ 1 new review-F3 test (derived).Converter evidence was produced at
d555ece2. That tree differs from0657538conly in.agents/plans/ce-6-sizing-fields.md(observed,git diff --stat d555ece2 0657538c), so it holds for0657538c.a9bd6391and77863d1echange only comments, tests, the ledger and the plan (observed,git diff --stat 0657538c 77863d1e), andmake check-fullincludesfidelity-gate.observed,DESIGN_SYNC__SECTION_CACHE_ENABLED=false scripts/snapshot-capture.py+cmp):expected.html.origin/mainstructure. Its only gap fromexpected.htmlis trailing whitespace that already existed, and it is equal after_normalize_html.origin/main,observed):image_refnull → hash on 0/11/23/10/13/17/16 nodes (5/6/7/8/9/10/reframe)..secrets.baseline: 15+11+16+10+13+14 = 79 entries added for cases 10/6/7/8/9/reframe (observed, per-file countorigin/mainvs0657538c). Case 5 keeps its 7 entries, renumbered.scripts/score-fidelity-cases.py,observed):scores.jsonis identical between base56598f36and the branch on all 7 cases.make fidelity-gatepasses without a re-stamp.expected.htmlare unchanged againstorigin/main.resync-case-structure.py 7reproduces the committed file byte for byte, and the root frame readsFIXED(observed).Notes for the reviewer
Live-path behaviour change (intended, user decision 2026-10-05).
scale_mode,rotationandeffects_summary, so live conversions (read from the cache) lost them. A synced design with a cropped image fill (scale_mode→_crop_export_idexport path) or effects (effects_summary→ conversion warnings) converted differently from the same design in the corpus harness. Both paths now match:test_live_cache_path_matches_corpus_loadpins the corpus values on every case, andtest_every_field_survives_the_production_cache_readcovers every field.effects_summaryalso changesis_small_decoration's child-vs-frame export choice (layout_analyzer.py:1693-1701: emitted width/height and export id) and where_icon_leafstops on a styled wrapper (:1897-1901). Noted on ledger entriesphase-53f-decorative-image-flagandce-10-large-styled-icon-wrapper-exports-bare-glyph(review F5).rotationis still used only behindframe_export_fallback_enabled, which is off by default.scale_modeon head isFILL(12/11/23/10/13/17/16 nodes for 5/6/7/8/9/10/reframe), andeffects_summaryandrotationare null everywhere (observed, per-case count over the committed fixtures).Deviations from the plan (all intentional, recorded in the plan's AMENDMENTS):
AttributeError, notTypeError.layout_mode not in (None, "NONE"). The parser stores"NONE"on frames that are not auto-layout.cmp(byte equality) rather thandiff --ignore-all-space, plus a scripted key classification. Case 8 was compared to base-structure output.56598f36, not via a stash.0657538cran all of them.expected.html.Not covered by data:
layoutPositioning,minWidth,maxWidthandWRAPnever occur in the corpus, so only the parser unit test covers them.Wrong-if: "a real client file has no
layoutMode" stays unchecked until CE-5 (#423). In the corpus, 625 of 625 frame-like nodes carry it (observed, walk of the 7raw_figma.jsonfiles).Carried forward:
phase-53g-band-item-spacing-defaults-vs-wrapper-paddingandce-9-peel-row-fixed-width-wrap(both CE-16/17).Linked
Closes #424 · Epic #439 · Plan
.agents/plans/ce-6-sizing-fields.md· Ledger: closesce-3-fixture-schema-lag-6-10, addsce-6-sizing-horizontal-dropped-at-document-bridgeOpened as a draft; CI's
readyjob flips it when every ci.yml job (backend,frontend,sdk-check,trivy,migrations,integration,migration-lint,commit-lint,e2e-smoke) andcodeqlare green on this head. A red job leaves it here with the failing check on the PR.🤖 Generated with Claude Code