Styling compatibility phase 0: preflight contract, staged modes, and the resolved-style IR - #455
Open
Alek99 wants to merge 33 commits into
Open
Styling compatibility phase 0: preflight contract, staged modes, and the resolved-style IR#455Alek99 wants to merge 33 commits into
Alek99 wants to merge 33 commits into
Conversation
…ot matrix chart.style_compatibility_report(target=..., engine=..., custom_css=...) routes every declared slot style for one export target into exactly one of four outcomes — survives, native-subset (naming the kept and lost properties per format family), browser-only, or state-gated — and mirrors the export path's own refusals (custom_css with a pinned native engine, Chromium SVG) instead of re-deciding them. Report-only: computing it never changes an export; the staged compatibility= modes that act on it come separately. Two properties are load-bearing. The report is constant-time when there is nothing to route: no class_names, no per-slot styles, no custom_css means no slot walk, so preflight stays free exactly where exports are hot. And state-gated is not lost: the capability registry now tags every slot with an applicability — present in a clean static export, or gated by hover, selection, crosshair, modebar, or view (reduction badges) — so a styled tooltip is recorded with its gating state rather than counted against a file that never contains a tooltip. Counting it before this change overstated the static parity gap by exactly the chrome a static file cannot contain: of 48 slots, 24 are clean-static and 24 state-gated, and all 10 native-capable slots are in the static set. Routing derives from the capability registry, the honored property subsets from the writers' own constants (xy._svg.SLOT_TEXT_PROPS / SLOT_RASTER_PROPS), and engine selection from export._resolve_image_engine — the preflight restates none of them, so it cannot disagree with them. The legend slot stays at declaration granularity (its box properties route through the merged legend declaration, which has no constant yet) and is qualified rather than guessed either direction (§28). The generated capability matrices gain the applicable-in column and the applicable-slot counts; new registry tests pin the partition (a new modebar_*/tooltip/crosshair/badge slot that forgets its family state fails the suite, and native support on a state-gated slot is rejected until the interaction-snapshot phase adds it deliberately). export.md §9 documents the report as its programmatic form.
…sion table Every image-export API (to_png, to_svg, to_image, write_image, and the write_images batch, at chart, figure, and module level) accepts compatibility=: "legacy" preserves today's behavior exactly, "warn" surfaces every declaration the export would drop as one StyleCompatibilityWarning, and "strict" raises StyleCompatibilityError before emission with the full preflight report attached. "lossless" is reserved and rejected: accepting the name before Engine.auto can actually re-route on preflight evidence would make it a lie. The default path pays one string comparison. The literal-"legacy" short-circuit returns before the preflight machinery is even imported, and export's StyleCompatibilityError/Warning aliases resolve through a module __getattr__, so importing xy.export stays exactly as heavy as it was before the modes existed (the preflight chain reaches the native library through the writers' constants — eagerly aliasing it would have added the dylib load to a module import that never had one). warn/strict pay the routing walk only when the chart carries class or per-slot declarations and the engine is native. Engine selection and compatibility stay orthogonal, and an explicit engine is a hard constraint: strict refuses on the pinned engine rather than re-routing to Chromium, a Chromium pin renders the full cascade and gives every mode nothing to do, and resolution errors (custom_css with a pinned native engine, Chromium SVG) precede and outrank mode logic in every mode. A strict batch fails whole while the plan is resolved, before any file is written. HTML export rejects a non-default mode like the other options it cannot honor, because a document that renders the full cascade has nothing to check. State-gated chrome never trips warn or strict in a clean static export: a file with no tooltip has dropped nothing by not styling one — the applicable-slot contract from the preflight change, now enforced rather than only reported. spec/process/style-compatibility-migration.md names the release each default flips in — warn in 0.0.7, strict at the 0.1.0 minor boundary, legacy removed in 0.2.0 — per the rule that a deprecation window is named when it opens, not "a future release". Facet grids deliberately do not accept the option yet rather than half-honoring it; their per-panel preflight is tracked there too.
… TS mirror python/xy/styling/resolved.py defines the renderer-neutral styling IR the compatibility program converges on: authored styling arrives from five mechanisms and (soon) two resolvers, and renderers should consume exactly one shape regardless of where it came from. Three contract properties, each enforced at construction on both ends of the eventual wire: Concrete values only. No var()/calc()/env()/inherit and no relative units: a value that still depends on a cascade, or on metrics the consumer would have to re-derive, is rejected loudly with the reason (§28). One unresolved value smuggled into the IR would re-create exactly the per-renderer divergence the IR exists to end. Interned declarations. A snapshot stores each distinct declaration once; instances reference it by index and carry only identity qualifiers (e.g. ["y","major","3"]), resolved geometry, and content. Interning is canonicalized, so a builder fed the same styling in any order emits the same snapshot. The dense-axis fixture (400 tick labels + 60 legend rows = 460 instances, 2 declarations) serializes to ~38.6 KB against the spec's 50 KB uncompressed budget, and the test states the headroom so eating it is a visible decision. Closed vocabulary per version. Schema v1's property list (paint, typography, layout, effects) is a generated constant in both languages; growing it is a STYLE_SNAPSHOT_VERSION bump, so a snapshot's vocabulary is always recoverable from its version field. snapshot_from_payload refuses versions it does not know rather than guessing. js/src/14_style_snapshot.ts is the TypeScript mirror, rendered by scripts/gen_style_snapshot_types.py from the Python module — one schema, two languages, and the suite runs --check so the committed mirror cannot drift (the gen_capability_matrix.py contract, applied to types). The client build typechecks it; nothing imports it yet. Nothing rides the wire in this change, so PROTOCOL_VERSION stays at 12. wire-protocol.md gains §8 documenting the payload shape and reserving the style_snapshot_request / style_snapshot message names for the capture change, which bumps the protocol and carries this schema as its reply.
…ounts
Review follow-ups on the preflight change, all in the mirror-the-export
direction:
Browser-resolved targets now validate custom_css through the export path's
own _custom_css_block (type check, </style> and comment-sequence rejection),
in the export's own order — engine resolution first. The report could
previously call an export lossless that would refuse its stylesheet.
Malformed figure styling raises exactly like the export instead of being
skipped: class_names/chrome_styles are assignable, so a report can be
requested before the spec build validates them, and a silently omitted entry
would be a report hiding a declaration (§28). validate_dom_slots runs the
same check the spec build runs; a non-mapping declaration set is refused by
name.
The writer family is now classified from both format sets with a refusal
for anything unclassified, so a future format cannot silently be reported
against the vector subset. The generated summary derives the styles={...}
slot count from the registry instead of hard-coding "nine" (the
axis_style_keys lesson: prose cannot hold a count). Both public entry
points return the typed StyleCompatibilityReport rather than Any.
…cabulary Resolution errors now precede mode logic in to_png for styled charts too: the custom_css/native refusal is checked right after engine resolution, before enforcement can warn or raise, so it stays the documented ValueError in every mode. The precedence test now feeds a lossy chart — the case that previously slipped past it — and asserts the error is not the strict subclass. Compatibility warnings land on the caller's export line. The distance from the warn call to user code varies by entry point (module function, Figure method, Chart method), so the stacklevel is measured: walk outward from enforce to the first frame outside the package. A test pins the attribution for three different entry points. Strict SVG failures no longer recommend engine=Engine.chromium, which that format rejects; the remediation is format-aware. write_images validates the compatibility vocabulary once up front, so a typo fails even an all-HTML batch instead of passing silently — while HTML entries themselves stay exempt from the mode, since a document that renders the full cascade has nothing to check. The changelog entry now names the facet-grid exclusion instead of overstating coverage, and the raster-drops-vector-only test is named for what it exercises.
# Conflicts: # CHANGELOG.md
Review follow-ups on the schema, all four in the same direction: the contract the module claims is now the contract it enforces. Relative units are rejected anywhere in a value, not only as a whole-string suffix. translate(50%, 20%), a "2em 1em" shorthand, and a gradient stop at 50% all carry the same document dependency a bare "1.5em" does; the end-anchored check let exactly those through. Cascade keywords went the other way: inherit/unset (plus initial/revert/revert-layer) reject only as the entire normalized value, so a face named "Inheritance Sans" is no longer refused for containing the letters. Tokens share the full string contract with declarations through one helper — a token "1.5em" or an empty string smuggles what a declaration would — and snapshot_from_payload now enforces the identical rules build() does: environment vocabulary and finiteness, and every token through the shared validator. The payload path is the untrusted end of the wire; a snapshot that could only exist by bypassing the builder must not become renderer-facing IR by arriving serialized. build() now emits canonical bytes: declaration slots assigned by content (instance indices remapped), instances sorted by identity. The docstring claimed order-independence while the payload depended on insertion order, and the old test compared declaration sets, which cannot see index drift — it now asserts byte-equal payloads from builders fed the same styling in different orders. Instance order carries no meaning; identity lives in (slot, qualifiers), which is what makes a snapshot cacheable across producers.
This was referenced Aug 4, 2026
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds a versioned resolved-style snapshot IR, browser and native cascade resolution, style snapshot transport, applicability metadata, compatibility preflight, native styling support, and compatibility-aware export handling. ChangesStyle compatibility and snapshot infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThe PR introduces compatibility preflight and staged export modes, then extends the styling pipeline with resolved snapshots, native cascade support, browser capture, and native chrome rendering.
Confidence Score: 5/5The PR appears safe to merge because the previously reported legend compatibility and geometry defects are fixed and no new eligible blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| python/xy/styling/preflight.py | Routes declared styles by renderer capability; the previously reported legend loss-reporting gaps are fixed at current HEAD. |
| python/xy/_svg.py | Expands native chrome and legend styling while keeping legend measurement consistent with emitted font sizes and supported box properties. |
| python/xy/_raster.py | Adds native raster chrome styling and consumes the shared legend layout without leaving an eligible prior-thread defect. |
| python/xy/styling/resolved.py | Defines and validates the canonical, versioned resolved-style snapshot representation. |
| python/xy/styling/cascade.py | Bridges native cascade resolution into validated style snapshots and compatibility reporting. |
| cascade/src/resolve.rs | Implements the profile-scoped CSS matcher, cascade, variable substitution, inheritance, and concrete-value resolution. |
| python/xy/export.py | Integrates compatibility modes, style sources, snapshots, and up-front batch validation into image export. |
| js/src/16_style_capture.ts | Captures allowlisted computed browser styles into the versioned snapshot contract. |
| Cargo.lock | Records the native cascade dependency graph; no accepted security finding affects the final review. |
Reviews (17): Last reviewed commit: "Record the colorbar shared-layout design..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
tests/test_resolved_style_snapshot.py (1)
69-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a canonical-ordering case that mixes value types for one property.
The current cases use the same value type per property, so the sort in
SnapshotBuilder.buildnever compares a number against a string. That comparison is the failure path described onpython/xy/styling/resolved.pylines 339-344. Add a case that interns{"font-size": 11}and{"font-size": "11px"}in one builder, so the fix stays pinned.♻️ Proposed test
+def test_declarations_sort_when_one_property_mixes_numbers_and_strings() -> None: + # Both values are legal per-declaration, so the canonical sort must order + # them instead of comparing a float against a string. + builder = rs.SnapshotBuilder() + builder.add("title", {"font-size": 11}) + builder.add("axis_title", {"font-size": "11px"}) + snapshot = builder.build(_env()) + assert len(snapshot.declarations) == 2 + assert rs.snapshot_from_payload(snapshot.to_payload()).to_payload() == snapshot.to_payload()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_resolved_style_snapshot.py` around lines 69 - 82, Add a canonical-ordering test case alongside test_builders_fed_the_same_styling_in_any_order_emit_identical_bytes that interns both {"font-size": 11} and {"font-size": "11px"} in a single SnapshotBuilder, then builds the snapshot successfully and verifies its canonical payload or ordering. Ensure the test exercises mixed value types for the same property and remains focused on the comparison path in SnapshotBuilder.build.tests/test_capability_registry.py (1)
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion cannot fail.
capabilities.summary()computeschart_slots_state_gatedaslen(CHART_SLOTS) - len(static), so the sum always equalslen(CHART_SLOTS). The check gives no coverage. Pin the published partition instead, which is the number the documents and the migration schedule quote.♻️ Proposed change
counts = caps.summary() - assert counts["chart_slots_static"] + counts["chart_slots_state_gated"] == len(caps.CHART_SLOTS) + # The 24/24 split the capability matrices and export.md §9 publish. + assert counts["chart_slots"] == 48 + assert counts["chart_slots_static"] == 24 + assert counts["chart_slots_state_gated"] == 24🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_capability_registry.py` around lines 63 - 64, Replace the tautological assertion using caps.summary() with an assertion that pins the published chart-slot partition count quoted by the documentation and migration schedule. Update the assertion in the capability registry test while retaining coverage that validates the expected partition value against the relevant summary field or published capability data.scripts/gen_capability_matrix.py (1)
66-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive or pin the
rootclaim instead of asserting it in prose.The sentence states that
slots_styleable_nativelysplits intoslots_via_stylesplusrootvia the chart-levelstyle=token bag. That relation is hardcoded in the text, not read fromcaps.summary(). Ifrootgains astyles={channel, or another slot gains native support outside that channel, this generated prose overclaims — the same drift theaxis_style_keysnote inpython/xy/styling/capabilities.pywarns about.Either compute the remainder, or pin the relation with a registry test.
♻️ Suggested derivation
f"- **{counts['chart_slots']}** stable chrome slots, CSS- and Tailwind-addressable " "in the browser. " f"**{counts['chart_slots_static']}** of them name chrome a clean static export " f"contains; **{counts['slots_styleable_natively']}** reach the native writers — " - f"**{counts['slots_via_styles']}** through `styles={{slot: ...}}` itself, and " - "`root` through the chart-level `style=` token bag. The other " + f"**{counts['slots_via_styles']}** through `styles={{slot: ...}}` itself, and the " + f"remaining **{int(counts['slots_styleable_natively']) - int(counts['slots_via_styles'])}** " + "(`root`) through the chart-level `style=` token bag. The other "🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/gen_capability_matrix.py` around lines 66 - 74, Update the prose generation around the `counts['slots_styleable_natively']` and `counts['slots_via_styles']` claims to derive the `root` remainder from `caps.summary()` rather than asserting it literally. Ensure the generated text reflects any future native styling channels or slot changes, or add a registry-level test that explicitly validates this relation if the relationship is intentionally fixed.tests/test_style_compatibility_report.py (1)
110-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the intermediate variable.
Line 111 binds
reportto a chart, then line 112 rebinds it to the report. Use a distinct name so the test reads in one pass.♻️ Suggested change
- report = _chart(styles={"legend_swatch": {"border-radius": "2px"}}) - report = report.style_compatibility_report("png") + chart = _chart(styles={"legend_swatch": {"border-radius": "2px"}}) + report = chart.style_compatibility_report("png")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_style_compatibility_report.py` around lines 110 - 116, Rename the chart value assigned by _chart in test_styles_on_a_slot_with_no_native_path_are_named_lost to a distinct intermediate variable, then call style_compatibility_report on that variable and keep report reserved for the resulting compatibility report.python/xy/styling/capabilities.py (1)
299-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard unknown
_STATE_GATED_SLOTSkeys.
test_state_gated_families_cannot_be_misfiledcatches missing family entries. It does not reject extra typo keys because.get(..., "static")remains the fallback. Validate mapping keys againstCHART_DOM_SLOTSat import time and raise a clearAssertionError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/styling/capabilities.py` around lines 299 - 329, Validate _STATE_GATED_SLOTS during module initialization by asserting every mapping key exists in CHART_DOM_SLOTS, and raise a clear AssertionError identifying any unknown keys. Preserve the existing state-gated family validation and static fallback behavior for valid slots.python/xy/export.py (1)
1217-1221: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocument the validation precedence.
_enforce_compatibilityruns before quality, background, dimension, scale, and boolean option validation. Document this order inspec/api/export.md, or move it after the option checks if argument errors must take precedence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/export.py` around lines 1217 - 1221, Document in spec/api/export.md that _enforce_compatibility runs before _validated_quality, _validated_background, dimension, scale, and boolean option validation, preserving the current precedence; alternatively, move _enforce_compatibility in the export flow after all option checks if argument validation must take precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/xy/styling/__init__.py`:
- Around line 19-21: Update python/xy/styling/__init__.py lines 19-21 so
importing the package does not eagerly load preflight; expose it lazily through
__getattr__ while preserving capabilities and resolved access. Update
spec/process/style-compatibility-migration.md line 13 to describe the legacy
guarantee as one string comparison with no slot walk.
In `@python/xy/styling/preflight.py`:
- Around line 1-23: Update the module docstring’s export-behavior paragraph to
reflect that this module defines validate_compatibility and enforce and may warn
or raise during export. Remove the claim that it is report-only or that
compatibility modes are implemented separately, while preserving the surrounding
statements about the report’s purpose and trust boundary.
In `@python/xy/styling/resolved.py`:
- Around line 339-344: Update the declaration ordering key in the
canonicalization logic around the declarations sort so values of different
Python types are always comparable. Build a type-tagged, deterministic
representation for each property name and value, preserving equivalent values’
canonical ordering while distinguishing numbers from strings; keep the existing
declaration sorting behavior for same-typed values and ensure valid mixed-type
declarations no longer raise TypeError.
- Around line 423-448: Update the payload deserialization block in resolved.py
so missing required keys and malformed values consistently raise ValueError
instead of leaking KeyError or TypeError. Safely validate access to environment,
each instance’s d and s fields, and geometry entries before coercion; wrap or
replace float conversion failures with ValueError while preserving the existing
validation messages and successful SlotInstance construction.
- Around line 116-130: The _RELATIVE_UNIT_RE validation must exempt percentage
components inside concrete rgb() and hsl() color functions while continuing to
reject percentage lengths and gradient stop positions. Update the validation
flow around _RELATIVE_UNIT_RE and _validate.css_color so values such as hsl(210
40% 96%) and rgb(100% 0% 0%) remain valid, without weakening relative-unit
rejection elsewhere.
In `@tests/test_style_compatibility_modes.py`:
- Around line 88-97: Update
test_warnings_land_on_the_callers_line_not_export_plumbing to assert exactly
three captured warnings before validating their filenames, ensuring each public
compatibility="warn" route emits one StyleCompatibilityWarning; do not rely on
len(caught) to determine the expected count.
---
Nitpick comments:
In `@python/xy/export.py`:
- Around line 1217-1221: Document in spec/api/export.md that
_enforce_compatibility runs before _validated_quality, _validated_background,
dimension, scale, and boolean option validation, preserving the current
precedence; alternatively, move _enforce_compatibility in the export flow after
all option checks if argument validation must take precedence.
In `@python/xy/styling/capabilities.py`:
- Around line 299-329: Validate _STATE_GATED_SLOTS during module initialization
by asserting every mapping key exists in CHART_DOM_SLOTS, and raise a clear
AssertionError identifying any unknown keys. Preserve the existing state-gated
family validation and static fallback behavior for valid slots.
In `@scripts/gen_capability_matrix.py`:
- Around line 66-74: Update the prose generation around the
`counts['slots_styleable_natively']` and `counts['slots_via_styles']` claims to
derive the `root` remainder from `caps.summary()` rather than asserting it
literally. Ensure the generated text reflects any future native styling channels
or slot changes, or add a registry-level test that explicitly validates this
relation if the relationship is intentionally fixed.
In `@tests/test_capability_registry.py`:
- Around line 63-64: Replace the tautological assertion using caps.summary()
with an assertion that pins the published chart-slot partition count quoted by
the documentation and migration schedule. Update the assertion in the capability
registry test while retaining coverage that validates the expected partition
value against the relevant summary field or published capability data.
In `@tests/test_resolved_style_snapshot.py`:
- Around line 69-82: Add a canonical-ordering test case alongside
test_builders_fed_the_same_styling_in_any_order_emit_identical_bytes that
interns both {"font-size": 11} and {"font-size": "11px"} in a single
SnapshotBuilder, then builds the snapshot successfully and verifies its
canonical payload or ordering. Ensure the test exercises mixed value types for
the same property and remains focused on the comparison path in
SnapshotBuilder.build.
In `@tests/test_style_compatibility_report.py`:
- Around line 110-116: Rename the chart value assigned by _chart in
test_styles_on_a_slot_with_no_native_path_are_named_lost to a distinct
intermediate variable, then call style_compatibility_report on that variable and
keep report reserved for the resulting compatibility report.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 77d5c629-02c7-4174-9d94-d34ddd83a45b
📒 Files selected for processing (21)
CHANGELOG.mddocs/styling/capabilities.mdjs/src/14_style_snapshot.tspython/xy/_figure.pypython/xy/components.pypython/xy/export.pypython/xy/styling/__init__.pypython/xy/styling/capabilities.pypython/xy/styling/preflight.pypython/xy/styling/resolved.pyscripts/gen_capability_matrix.pyscripts/gen_style_snapshot_types.pyspec/api/capability-matrix.mdspec/api/export.mdspec/design/wire-protocol.mdspec/process/style-compatibility-migration.mdtests/test_capability_registry.pytests/test_components.pytests/test_resolved_style_snapshot.pytests/test_style_compatibility_modes.pytests/test_style_compatibility_report.py
The legend qualification was rounding toward silence: a declaration-level
"qualified, lost=()" let a legend letter-spacing or font-family on the
raster path slip past warn and strict entirely. The writer module now owns
its merged-declaration box vocabulary (_svg.LEGEND_BOX_PROPS, next to the
alias table it already keeps), and the preflight routes legend declarations
at property level — text subset per family, plus the box set, and a name in
neither is a provable loss the modes act on.
Percentages inside rgb()/hsl() are color components against a fixed channel
range, not document-relative lengths; the repo's own color contract accepts
them and the compilers emit them, so the concreteness scan masks those
function bodies before rejecting stray percentages (lengths and gradient
stop positions still reject).
The canonical declaration sort is type-tagged: both value kinds are legal
per property, so {"font-size": 11} next to {"font-size": "11px"} must sort,
not raise. snapshot_from_payload raises ValueError for every malformed
shape (missing environment, instances without s/d, non-numeric geometry,
non-mapping declarations) — the deserialization boundary must never leak a
KeyError where callers catch ValueError.
xy.styling resolves its submodules lazily (PEP 562): capabilities reaches
the writers' constants and through them the native library, so importing
the package now costs nothing until a submodule is used — the legacy path's
zero-import guarantee extends to the package itself, with a subprocess test
pinning it. The preflight module docstring now says what the module does:
the reporting core stays report-only; enforce is the one function that acts,
and only for warn and strict.
Member
Author
|
Second review round addressed in 4324206 — all seven findings: Fixed (5):
Skipped (1): the ast-grep non-literal-regex/ReDoS warning — the pattern is built from module constants through CI note: the one red job on the previous run was |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
9 issues found across 21 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="python/xy/styling/capabilities.py">
<violation number="1" location="python/xy/styling/capabilities.py:471">
P2: The `slots_via_styles` count can drift from reality because it parses the human-readable `channel` string instead of a structured source-of-truth. Counting against `STATIC_STYLED_SLOTS` keeps this release-note number stable even if channel descriptions are reworded.</violation>
</file>
<file name="spec/process/style-compatibility-migration.md">
<violation number="1" location="spec/process/style-compatibility-migration.md:5">
P3: There's a word-order slip in the opening sentence: "with the release each step ships in named now" should read "now named" to parse correctly. A reader otherwise stumbles mid-sentence on the doc's first claim.</violation>
</file>
<file name="tests/test_capability_registry.py">
<violation number="1" location="tests/test_capability_registry.py:74">
P2: the *selection* family is only guarded by an exact `slot.id == "selection"` check rather than a prefix, so the guard the test promises ('every member of a chrome family carries its family's state') does not actually hold for the selection family. New `selection_*` chrome would take the silent `"static"` fallback on both sides: a correctly-gated new selection slot fails the suite spuriously, and one that forgets its `_STATE_GATED_SLOTS` entry passes while being wrongly counted as part of the clean-static surface — the exact direction this test exists to prevent. Consider adding a `"selection"` prefix family (and any other state family) so the fallback can never silently report "static", instead of special-casing the exact id.</violation>
</file>
<file name="python/xy/styling/resolved.py">
<violation number="1" location="python/xy/styling/resolved.py:126">
P2: Negative relative lengths bypass the concreteness check. Because `-` is excluded by the lookbehind, values such as `-1.5em`, `-50%`, and `translate(-50%, -20%)` do not match and are accepted into the snapshot even though they still depend on font or layout metrics. Allow the sign while retaining the identifier-boundary check so all relative-unit occurrences are rejected.</violation>
<violation number="2" location="python/xy/styling/resolved.py:186">
P2: `ResolvedStyleSnapshot` can be constructed directly with unresolved or otherwise invalid data, bypassing every validator. For example, `ResolvedStyleSnapshot(SnapshotEnvironment(1, 1), declarations=({"font-size": "var(--x)"},)).to_payload()` succeeds and emits the forbidden value. Since the class is exported in `__all__` and the module/spec promise rejection at construction, validation should live in `__post_init__` (or the raw constructor should be made private) rather than only in `SnapshotBuilder` and `snapshot_from_payload`.</violation>
<violation number="3" location="python/xy/styling/resolved.py:337">
P2: Token insertion order remains observable in the serialized snapshot, so logically identical styling is not byte-canonical. Two builders given the same token mapping in opposite insertion orders produce different `tokens` dict order and therefore different JSON bytes, despite the canonicalization promise in the builder and §8 of the spec. Sort token names while constructing `resolved_tokens`.</violation>
<violation number="4" location="python/xy/styling/resolved.py:431">
P2: Malformed payloads can use a JSON boolean as a declaration index because Python `bool` is a subclass of `int`. With at least two declarations, `d: true` passes this check as index `1`, and the returned IR later serializes the declaration reference back as `true` instead of the required integer. Reject booleans explicitly at this untrusted-payload boundary.</violation>
<violation number="5" location="python/xy/styling/resolved.py:462">
P2: The payload reader accepts a non-interned, non-canonical snapshot instead of enforcing or normalizing the schema's canonical representation. Duplicate declarations and arbitrary declaration/instance order pass through unchanged, so an incoming snapshot can exceed the deduplication budget and serialize differently from an equivalent builder-produced snapshot. The reader should either validate canonical ordering/deduplication or rebuild through the same canonicalization path.</violation>
</file>
<file name="python/xy/styling/preflight.py">
<violation number="1" location="python/xy/styling/preflight.py:311">
P3: For a browser target (e.g. `target="html"` or `engine=Engine.chromium`) the report short-circuits to a bare `StyleCompatibilityReport` with no `findings`, even when the chart declares `class_names`/per-slot styles that `sources` reports as present. A caller inspecting the report sees the declarations but no routing entries, which reads as inconsistent with the module's stated guarantee that every declared style ends in exactly one route. Consider emitting `survives` findings for the declared slots on the browser path (or documenting the early-out) so the report surface is uniform across targets.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| # The `styles={slot: ...}` channel specifically — the writers' own | ||
| # STATIC_STYLED_SLOTS, counted from the registry so generated prose | ||
| # cannot hold a stale number (the axis_style_keys lesson). | ||
| "slots_via_styles": sum(1 for s in CHART_SLOTS if s.channel.startswith("styles={")), |
Contributor
There was a problem hiding this comment.
P2: The slots_via_styles count can drift from reality because it parses the human-readable channel string instead of a structured source-of-truth. Counting against STATIC_STYLED_SLOTS keeps this release-note number stable even if channel descriptions are reworded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/styling/capabilities.py, line 471:
<comment>The `slots_via_styles` count can drift from reality because it parses the human-readable `channel` string instead of a structured source-of-truth. Counting against `STATIC_STYLED_SLOTS` keeps this release-note number stable even if channel descriptions are reworded.</comment>
<file context>
@@ -400,22 +452,32 @@ def axis_style_keys() -> tuple[str, ...]:
+ # The `styles={slot: ...}` channel specifically — the writers' own
+ # STATIC_STYLED_SLOTS, counted from the registry so generated prose
+ # cannot hold a stale number (the axis_style_keys lesson).
+ "slots_via_styles": sum(1 for s in CHART_SLOTS if s.channel.startswith("styles={")),
"extension_points_shipped": sum(1 for e in EXTENSION_POINTS if e.status == "shipped"),
"known_renderer_divergences": len(KNOWN_RENDERER_DIVERGENCES),
</file context>
Suggested change
| "slots_via_styles": sum(1 for s in CHART_SLOTS if s.channel.startswith("styles={")), | |
| "slots_via_styles": sum(1 for s in CHART_SLOTS if s.id in STATIC_STYLED_SLOTS), |
| families = { | ||
| "tooltip": "hover", | ||
| "modebar": "modebar", | ||
| "crosshair_": "crosshair", |
Contributor
There was a problem hiding this comment.
P2: the selection family is only guarded by an exact slot.id == "selection" check rather than a prefix, so the guard the test promises ('every member of a chrome family carries its family's state') does not actually hold for the selection family. New selection_* chrome would take the silent "static" fallback on both sides: a correctly-gated new selection slot fails the suite spuriously, and one that forgets its _STATE_GATED_SLOTS entry passes while being wrongly counted as part of the clean-static surface — the exact direction this test exists to prevent. Consider adding a "selection" prefix family (and any other state family) so the fallback can never silently report "static", instead of special-casing the exact id.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_capability_registry.py, line 74:
<comment>the *selection* family is only guarded by an exact `slot.id == "selection"` check rather than a prefix, so the guard the test promises ('every member of a chrome family carries its family's state') does not actually hold for the selection family. New `selection_*` chrome would take the silent `"static"` fallback on both sides: a correctly-gated new selection slot fails the suite spuriously, and one that forgets its `_STATE_GATED_SLOTS` entry passes while being wrongly counted as part of the clean-static surface — the exact direction this test exists to prevent. Consider adding a `"selection"` prefix family (and any other state family) so the fallback can never silently report "static", instead of special-casing the exact id.</comment>
<file context>
@@ -54,6 +54,49 @@ def test_registry_covers_exactly_the_public_dom_slots() -> None:
+ families = {
+ "tooltip": "hover",
+ "modebar": "modebar",
+ "crosshair_": "crosshair",
+ "badge": "view",
+ }
</file context>
Suggested change
| "crosshair_": "crosshair", | |
| "crosshair_": "crosshair", | |
| "selection": "selection", |
| environment.width, environment.height, environment.dpr, environment.color_scheme | ||
| ) | ||
| resolved_tokens = { | ||
| str(name): assert_resolved_token(name, value) for name, value in (tokens or {}).items() |
Contributor
There was a problem hiding this comment.
P2: Token insertion order remains observable in the serialized snapshot, so logically identical styling is not byte-canonical. Two builders given the same token mapping in opposite insertion orders produce different tokens dict order and therefore different JSON bytes, despite the canonicalization promise in the builder and §8 of the spec. Sort token names while constructing resolved_tokens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/styling/resolved.py, line 337:
<comment>Token insertion order remains observable in the serialized snapshot, so logically identical styling is not byte-canonical. Two builders given the same token mapping in opposite insertion orders produce different `tokens` dict order and therefore different JSON bytes, despite the canonicalization promise in the builder and §8 of the spec. Sort token names while constructing `resolved_tokens`.</comment>
<file context>
@@ -0,0 +1,488 @@
+ environment.width, environment.height, environment.dpr, environment.color_scheme
+ )
+ resolved_tokens = {
+ str(name): assert_resolved_token(name, value) for name, value in (tokens or {}).items()
+ }
+ # Canonicalize: declaration slots by content, instances by identity,
</file context>
Suggested change
| str(name): assert_resolved_token(name, value) for name, value in (tokens or {}).items() | |
| str(name): assert_resolved_token(name, value) for name, value in sorted((tokens or {}).items(), key=lambda item: str(item[0])) |
| return _assert_concrete_text(prop, value) | ||
|
|
||
|
|
||
| @dataclass(frozen=True) |
Contributor
There was a problem hiding this comment.
P2: ResolvedStyleSnapshot can be constructed directly with unresolved or otherwise invalid data, bypassing every validator. For example, ResolvedStyleSnapshot(SnapshotEnvironment(1, 1), declarations=({"font-size": "var(--x)"},)).to_payload() succeeds and emits the forbidden value. Since the class is exported in __all__ and the module/spec promise rejection at construction, validation should live in __post_init__ (or the raw constructor should be made private) rather than only in SnapshotBuilder and snapshot_from_payload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/styling/resolved.py, line 186:
<comment>`ResolvedStyleSnapshot` can be constructed directly with unresolved or otherwise invalid data, bypassing every validator. For example, `ResolvedStyleSnapshot(SnapshotEnvironment(1, 1), declarations=({"font-size": "var(--x)"},)).to_payload()` succeeds and emits the forbidden value. Since the class is exported in `__all__` and the module/spec promise rejection at construction, validation should live in `__post_init__` (or the raw constructor should be made private) rather than only in `SnapshotBuilder` and `snapshot_from_payload`.</comment>
<file context>
@@ -0,0 +1,488 @@
+ return _assert_concrete_text(prop, value)
+
+
+@dataclass(frozen=True)
+class SlotInstance:
+ """One styled occurrence of a slot, referencing an interned declaration.
</file context>
| #: suffix. Mid-string relative units are precisely the ones that slipped a | ||
| #: document dependency past the earlier end-anchored check. | ||
| _RELATIVE_UNIT_RE = re.compile( | ||
| r"(?<![a-z0-9.#_-])\d*\.?\d+(?:" |
Contributor
There was a problem hiding this comment.
P2: Negative relative lengths bypass the concreteness check. Because - is excluded by the lookbehind, values such as -1.5em, -50%, and translate(-50%, -20%) do not match and are accepted into the snapshot even though they still depend on font or layout metrics. Allow the sign while retaining the identifier-boundary check so all relative-unit occurrences are rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/styling/resolved.py, line 126:
<comment>Negative relative lengths bypass the concreteness check. Because `-` is excluded by the lookbehind, values such as `-1.5em`, `-50%`, and `translate(-50%, -20%)` do not match and are accepted into the snapshot even though they still depend on font or layout metrics. Allow the sign while retaining the identifier-boundary check so all relative-unit occurrences are rejected.</comment>
<file context>
@@ -0,0 +1,488 @@
+#: suffix. Mid-string relative units are precisely the ones that slipped a
+#: document dependency past the earlier end-anchored check.
+_RELATIVE_UNIT_RE = re.compile(
+ r"(?<![a-z0-9.#_-])\d*\.?\d+(?:"
+ + "|".join(map(re.escape, _RELATIVE_UNITS))
+ + r")(?![a-z0-9%])",
</file context>
|
|
||
| The staged path from "native exports silently drop `class_names`" to "no | ||
| renderer drops a declaration without saying so" — with the release each step | ||
| ships in named now, so none of them can quietly become permanent. The |
Contributor
There was a problem hiding this comment.
P3: There's a word-order slip in the opening sentence: "with the release each step ships in named now" should read "now named" to parse correctly. A reader otherwise stumbles mid-sentence on the doc's first claim.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/process/style-compatibility-migration.md, line 5:
<comment>There's a word-order slip in the opening sentence: "with the release each step ships in named now" should read "now named" to parse correctly. A reader otherwise stumbles mid-sentence on the doc's first claim.</comment>
<file context>
@@ -0,0 +1,64 @@
+
+The staged path from "native exports silently drop `class_names`" to "no
+renderer drops a declaration without saying so" — with the release each step
+ships in named now, so none of them can quietly become permanent. The
+programmatic foundation is `chart.style_compatibility_report()`
+(`spec/api/export.md` §9) and the `compatibility=` export option.
</file context>
| sources = _sources(figure, custom_css) | ||
| if resolved_engine == "browser": | ||
| # The live client renders the full cascade; nothing can drop. | ||
| return StyleCompatibilityReport(target=fmt, engine=resolved_engine, sources=sources) |
Contributor
There was a problem hiding this comment.
P3: For a browser target (e.g. target="html" or engine=Engine.chromium) the report short-circuits to a bare StyleCompatibilityReport with no findings, even when the chart declares class_names/per-slot styles that sources reports as present. A caller inspecting the report sees the declarations but no routing entries, which reads as inconsistent with the module's stated guarantee that every declared style ends in exactly one route. Consider emitting survives findings for the declared slots on the browser path (or documenting the early-out) so the report surface is uniform across targets.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/styling/preflight.py, line 311:
<comment>For a browser target (e.g. `target="html"` or `engine=Engine.chromium`) the report short-circuits to a bare `StyleCompatibilityReport` with no `findings`, even when the chart declares `class_names`/per-slot styles that `sources` reports as present. A caller inspecting the report sees the declarations but no routing entries, which reads as inconsistent with the module's stated guarantee that every declared style ends in exactly one route. Consider emitting `survives` findings for the declared slots on the browser path (or documenting the early-out) so the report surface is uniform across targets.</comment>
<file context>
@@ -0,0 +1,483 @@
+ sources = _sources(figure, custom_css)
+ if resolved_engine == "browser":
+ # The live client renders the full cascade; nothing can drop.
+ return StyleCompatibilityReport(target=fmt, engine=resolved_engine, sources=sources)
+ if not (figure.class_names or figure.chrome_styles):
+ # The constant-time path: chart-level `style=` and mark/axis `style=`
</file context>
…lently xy.styling.declared.resolve_declared is the Python half of the two-resolver architecture: one pass over a spec's per-slot declarations produces both the byte-exact view the writers read (kebab-case normalization, authored objects preserved — an authored int 600 must stay an int because f-string emission spells 600 and 600.0 apart) and the interned ResolvedStyleSnapshot of the same content for every IR consumer. _svg.slot_styles now derives its result through the resolver, so the writers and the IR cannot disagree about what was declared — the existing export-style-survival and image suites pass unchanged, which is the byte-equivalence gate. The gap between view and snapshot is exactly enumerable and tested, not silent (§28): presentational number formatting (schema numbers are floats by wire contract), plus the legend's em multipliers — padding/row-gap/ font-size em strings are the legend's own geometry domain (_legend_em consumes multipliers, not CSS lengths), so schema v1 rightly refuses them and they ride DeclaredStyling.writer_domain beside the snapshot. The chrome-parity work moves legend geometry to resolved px and retires the residue. Chart tokens ride the snapshot's token bag, minus var()-bearing values the declared resolver has no cascade for — the same values a static file already cannot honor.
Contributor
There was a problem hiding this comment.
6 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="python/xy/styling/resolved.py">
<violation number="1" location="python/xy/styling/resolved.py:141">
P2: Relative-unit filtering can be bypassed by names that merely contain `rgba`/`hsla` (for example `myrgba(10%)`), so `%` outside real color functions may slip through. `_COLOR_FUNCTION_RE` is missing an identifier boundary before the function name; anchoring to standalone function tokens keeps the exception limited to actual rgb()/hsl() components.</violation>
<violation number="2" location="python/xy/styling/resolved.py:141">
P2: The new color-function masking only covers `rgb()/hsl()` (and alpha forms), but the change's purpose — accepting fixed-channel color-component percentages as concrete — extends to every color form the package already treats as a valid, browser-resolved literal via `_validate.css_color`. For example `oklch(60% 0.13 200)` is a legal, concrete XY color whose `60%` lightness is a fixed channel, not a document-relative length, yet `_assert_concrete_text` will reject it because `%` survives the mask and trips `_RELATIVE_UNIT_RE`. Consider widening `_COLOR_FUNCTION_RE` to the other fixed-channel color functions (`oklch`, `lch`, `lab`, `hwb`, `color-mix`, `color`) so the accepted/rectified behavior matches the documented rule, or document why those forms are intentionally rejected.</violation>
<violation number="3" location="python/xy/styling/resolved.py:464">
P2: Malformed payload container fields can still raise `TypeError`, so callers expecting boundary `ValueError` handling may miss these failures. The new loops iterate `declarations`/`instances` without type checks; validating container shape first and raising `ValueError` would make deserialization errors consistent.</violation>
</file>
<file name="tests/test_declared_snapshot.py">
<violation number="1" location="tests/test_declared_snapshot.py:110">
P3: The test name and docstring claim the routed slot_styles leaves export bytes 'unchanged' by the routing, but the assertions only check determinism (a styled export equals its own re-render) and one attribute substring. A change that altered real output bytes while staying deterministic would pass. Recommend asserting against an explicit expected byte baseline (e.g. a golden repr/startswith marker or the legacy normalization result) so the name matches what is verified, or rename it to reflect the determinism/smoke scope.</violation>
</file>
<file name="tests/test_style_compatibility_report.py">
<violation number="1" location="tests/test_style_compatibility_report.py:256">
P2: The spawned interpreter can't resolve `xy` the way the rest of the suite does: `tests/conftest.py` adds `python/` to `sys.path` in-process, which the child doesn't inherit, so `import xy.styling` raises ModuleNotFoundError and `check=True` errors the test in source-tree setups. Make the child find the same package the parent imports, e.g. run it with `cwd` set to the directory containing the `xy` package (or pass a PYTHONPATH env), so the test asserts the lazy-load contract rather than depending on an installed copy.</violation>
</file>
<file name="python/xy/_svg.py">
<violation number="1" location="python/xy/_svg.py:1396">
P3: Every `slot_styles()` call now goes through `resolve_declared()`, which builds the full interned `ResolvedStyleSnapshot` (interning + canonical sorting + validating the entire chart token bag through `_concrete_tokens`/`assert_resolved_token`) and then discards that snapshot — `slot_styles` returns only `slot_view()`, and no production code currently consumes `.snapshot` or `.writer_domain` (only the tests do). Since `slot_styles` is the export hot path used by every static writer, each export pays the whole IR build for output that is thrown away. If the IR isn't wired to a consumer yet, consider deferring the `resolve_declared` construction (e.g. keep the view computation light until the snapshot/routing lane actually reads it), so the byte-equivalent 'routing' doesn't incur cost with no consumer. A comment noting the snapshot is intentionally discarded here would at least make the cost legible.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| if not isinstance(env, Mapping) or "width" not in env or "height" not in env: | ||
| raise ValueError(f"style snapshot environment must map width/height: {env!r}") | ||
| declarations = [] | ||
| for decl in payload.get("declarations", ()): |
Contributor
There was a problem hiding this comment.
P2: Malformed payload container fields can still raise TypeError, so callers expecting boundary ValueError handling may miss these failures. The new loops iterate declarations/instances without type checks; validating container shape first and raising ValueError would make deserialization errors consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/styling/resolved.py, line 464:
<comment>Malformed payload container fields can still raise `TypeError`, so callers expecting boundary `ValueError` handling may miss these failures. The new loops iterate `declarations`/`instances` without type checks; validating container shape first and raising `ValueError` would make deserialization errors consistent.</comment>
<file context>
@@ -420,19 +451,33 @@ def snapshot_from_payload(payload: Mapping[str, Any]) -> ResolvedStyleSnapshot:
+ if not isinstance(env, Mapping) or "width" not in env or "height" not in env:
+ raise ValueError(f"style snapshot environment must map width/height: {env!r}")
+ declarations = []
+ for decl in payload.get("declarations", ()):
+ if not isinstance(decl, Mapping):
+ raise ValueError(f"declaration {decl!r} must be a property mapping")
</file context>
| #: `hsl(210 40% 96%)` and the style compilers emit such text. Those function | ||
| #: bodies are masked out before the relative-unit scan; a percentage | ||
| #: *outside* them (a length, a gradient stop position) still rejects. | ||
| _COLOR_FUNCTION_RE = re.compile(r"(?:rgba?|hsla?)\([^()]*\)", re.IGNORECASE) |
Contributor
There was a problem hiding this comment.
P2: Relative-unit filtering can be bypassed by names that merely contain rgba/hsla (for example myrgba(10%)), so % outside real color functions may slip through. _COLOR_FUNCTION_RE is missing an identifier boundary before the function name; anchoring to standalone function tokens keeps the exception limited to actual rgb()/hsl() components.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/styling/resolved.py, line 141:
<comment>Relative-unit filtering can be bypassed by names that merely contain `rgba`/`hsla` (for example `myrgba(10%)`), so `%` outside real color functions may slip through. `_COLOR_FUNCTION_RE` is missing an identifier boundary before the function name; anchoring to standalone function tokens keeps the exception limited to actual rgb()/hsl() components.</comment>
<file context>
@@ -121,14 +121,25 @@
+#: `hsl(210 40% 96%)` and the style compilers emit such text. Those function
+#: bodies are masked out before the relative-unit scan; a percentage
+#: *outside* them (a length, a gradient stop position) still rejects.
+_COLOR_FUNCTION_RE = re.compile(r"(?:rgba?|hsla?)\([^()]*\)", re.IGNORECASE)
+
_COLOR_SCHEMES = frozenset({"light", "dark"})
</file context>
Suggested change
| _COLOR_FUNCTION_RE = re.compile(r"(?:rgba?|hsla?)\([^()]*\)", re.IGNORECASE) | |
| _COLOR_FUNCTION_RE = re.compile(r"(?<![a-z0-9_-])(?:rgba?|hsla?)\([^()]*\)", re.IGNORECASE) |
| "'xy.styling.resolved', 'xy._svg') if m in sys.modules))" | ||
| ) | ||
| result = subprocess.run( | ||
| [_sys.executable, "-c", probe], capture_output=True, text=True, check=True |
Contributor
There was a problem hiding this comment.
P2: The spawned interpreter can't resolve xy the way the rest of the suite does: tests/conftest.py adds python/ to sys.path in-process, which the child doesn't inherit, so import xy.styling raises ModuleNotFoundError and check=True errors the test in source-tree setups. Make the child find the same package the parent imports, e.g. run it with cwd set to the directory containing the xy package (or pass a PYTHONPATH env), so the test asserts the lazy-load contract rather than depending on an installed copy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_style_compatibility_report.py, line 256:
<comment>The spawned interpreter can't resolve `xy` the way the rest of the suite does: `tests/conftest.py` adds `python/` to `sys.path` in-process, which the child doesn't inherit, so `import xy.styling` raises ModuleNotFoundError and `check=True` errors the test in source-tree setups. Make the child find the same package the parent imports, e.g. run it with `cwd` set to the directory containing the `xy` package (or pass a PYTHONPATH env), so the test asserts the lazy-load contract rather than depending on an installed copy.</comment>
<file context>
@@ -224,6 +239,25 @@ def test_explain_names_every_route() -> None:
+ "'xy.styling.resolved', 'xy._svg') if m in sys.modules))"
+ )
+ result = subprocess.run(
+ [_sys.executable, "-c", probe], capture_output=True, text=True, check=True
+ )
+ assert result.stdout.strip() == "[]"
</file context>
| #: `hsl(210 40% 96%)` and the style compilers emit such text. Those function | ||
| #: bodies are masked out before the relative-unit scan; a percentage | ||
| #: *outside* them (a length, a gradient stop position) still rejects. | ||
| _COLOR_FUNCTION_RE = re.compile(r"(?:rgba?|hsla?)\([^()]*\)", re.IGNORECASE) |
Contributor
There was a problem hiding this comment.
P2: The new color-function masking only covers rgb()/hsl() (and alpha forms), but the change's purpose — accepting fixed-channel color-component percentages as concrete — extends to every color form the package already treats as a valid, browser-resolved literal via _validate.css_color. For example oklch(60% 0.13 200) is a legal, concrete XY color whose 60% lightness is a fixed channel, not a document-relative length, yet _assert_concrete_text will reject it because % survives the mask and trips _RELATIVE_UNIT_RE. Consider widening _COLOR_FUNCTION_RE to the other fixed-channel color functions (oklch, lch, lab, hwb, color-mix, color) so the accepted/rectified behavior matches the documented rule, or document why those forms are intentionally rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/styling/resolved.py, line 141:
<comment>The new color-function masking only covers `rgb()/hsl()` (and alpha forms), but the change's purpose — accepting fixed-channel color-component percentages as concrete — extends to every color form the package already treats as a valid, browser-resolved literal via `_validate.css_color`. For example `oklch(60% 0.13 200)` is a legal, concrete XY color whose `60%` lightness is a fixed channel, not a document-relative length, yet `_assert_concrete_text` will reject it because `%` survives the mask and trips `_RELATIVE_UNIT_RE`. Consider widening `_COLOR_FUNCTION_RE` to the other fixed-channel color functions (`oklch`, `lch`, `lab`, `hwb`, `color-mix`, `color`) so the accepted/rectified behavior matches the documented rule, or document why those forms are intentionally rejected.</comment>
<file context>
@@ -121,14 +121,25 @@
+#: `hsl(210 40% 96%)` and the style compilers emit such text. Those function
+#: bodies are masked out before the relative-unit scan; a percentage
+#: *outside* them (a length, a gradient stop position) still rejects.
+_COLOR_FUNCTION_RE = re.compile(r"(?:rgba?|hsla?)\([^()]*\)", re.IGNORECASE)
+
_COLOR_SCHEMES = frozenset({"light", "dark"})
</file context>
Suggested change
| _COLOR_FUNCTION_RE = re.compile(r"(?:rgba?|hsla?)\([^()]*\)", re.IGNORECASE) | |
| _COLOR_FUNCTION_RE = re.compile( | |
| r"(?:rgba?|hsla?|hwb|lab|lch|oklab|oklch|color-mix|color)\([^()]*\)", re.IGNORECASE | |
| ) |
| assert styling.writer_domain == {} | ||
|
|
||
|
|
||
| def test_export_bytes_are_unchanged_by_the_routing() -> None: |
Contributor
There was a problem hiding this comment.
P3: The test name and docstring claim the routed slot_styles leaves export bytes 'unchanged' by the routing, but the assertions only check determinism (a styled export equals its own re-render) and one attribute substring. A change that altered real output bytes while staying deterministic would pass. Recommend asserting against an explicit expected byte baseline (e.g. a golden repr/startswith marker or the legacy normalization result) so the name matches what is verified, or rename it to reflect the determinism/smoke scope.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_declared_snapshot.py, line 110:
<comment>The test name and docstring claim the routed slot_styles leaves export bytes 'unchanged' by the routing, but the assertions only check determinism (a styled export equals its own re-render) and one attribute substring. A change that altered real output bytes while staying deterministic would pass. Recommend asserting against an explicit expected byte baseline (e.g. a golden repr/startswith marker or the legacy normalization result) so the name matches what is verified, or rename it to reflect the determinism/smoke scope.</comment>
<file context>
@@ -0,0 +1,129 @@
+ assert styling.writer_domain == {}
+
+
+def test_export_bytes_are_unchanged_by_the_routing() -> None:
+ # The real gate: styled exports through the routed slot_styles are
+ # deterministic and carry the styling the writers honored before.
</file context>
| return out | ||
| from .styling.declared import resolve_declared | ||
|
|
||
| return resolve_declared(spec).slot_view() |
Contributor
There was a problem hiding this comment.
P3: Every slot_styles() call now goes through resolve_declared(), which builds the full interned ResolvedStyleSnapshot (interning + canonical sorting + validating the entire chart token bag through _concrete_tokens/assert_resolved_token) and then discards that snapshot — slot_styles returns only slot_view(), and no production code currently consumes .snapshot or .writer_domain (only the tests do). Since slot_styles is the export hot path used by every static writer, each export pays the whole IR build for output that is thrown away. If the IR isn't wired to a consumer yet, consider deferring the resolve_declared construction (e.g. keep the view computation light until the snapshot/routing lane actually reads it), so the byte-equivalent 'routing' doesn't incur cost with no consumer. A comment noting the snapshot is intentionally discarded here would at least make the cost legible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/_svg.py, line 1396:
<comment>Every `slot_styles()` call now goes through `resolve_declared()`, which builds the full interned `ResolvedStyleSnapshot` (interning + canonical sorting + validating the entire chart token bag through `_concrete_tokens`/`assert_resolved_token`) and then discards that snapshot — `slot_styles` returns only `slot_view()`, and no production code currently consumes `.snapshot` or `.writer_domain` (only the tests do). Since `slot_styles` is the export hot path used by every static writer, each export pays the whole IR build for output that is thrown away. If the IR isn't wired to a consumer yet, consider deferring the `resolve_declared` construction (e.g. keep the view computation light until the snapshot/routing lane actually reads it), so the byte-equivalent 'routing' doesn't incur cost with no consumer. A comment noting the snapshot is intentionally discarded here would at least make the cost legible.</comment>
<file context>
@@ -1383,17 +1383,17 @@ def slot_styles(spec: dict[str, Any]) -> dict[str, dict[str, Any]]:
- return out
+ from .styling.declared import resolve_declared
+
+ return resolve_declared(spec).slot_view()
</file context>
Phase 2 of the styling-compatibility program: the browser resolves the cascade once, and the native writers reproduce it ever after. The client half (js/src/16_style_capture.ts) walks every rendered data-xy-slot element after fonts and layout settle (setTimeout ticks, not rAF — headless hosts throttle rAF on unfocused pages) and reads the schema's allowlisted computed properties, interned per distinct declaration. Two capture rules exist because the browser-oracle smoke failed without them, in order: the computed `background` shorthand serializes position/size tokens (`0% 0% / auto`) the concreteness gate rightly refuses, so capture reads the `background-color` longhand under the schema's name; and SVG presentation properties compute on every element, so an HTML tick label reports a phantom `fill: rgb(0, 0, 0)` the writers would prefer over its real `color` — SVG-only paints are captured from SVG elements alone. The transport is the reserved v13 pair: style_snapshot_request out, style_snapshot (or an error, so the kernel future never dangles) back; FigureWidget.capture_style_snapshot is asynchronous by contract and validates every reply through snapshot_from_payload at the boundary. A cached v12 client would silently ignore the request, so the protocol bumps rather than leaving a timeout mystery. Exports consume the capture: style_snapshot= on to_png/to_svg/to_image/ write_image overlays the snapshot's declarations for the render (first instance per slot until chrome parity brings per-instance geometry), restored after. A supplied snapshot is the lossless remedy the modes recommend — strict passes with one where it refuses without one — and the Chromium engine rejects the combination as the contradiction it is. scripts/style_capture_smoke.py is the browser-as-oracle loop with no notebook in it: render standalone HTML in headless Chromium, capture via the public window.xy.captureStyleSnapshot, validate through the Python schema, and assert an exact custom_css-resolved color lands in a native SVG export. 51 instances, 36 declarations, ~18 KB against the 50 KB budget on the smoke chart.
The protocol-pin assertions exist to force exactly this conscious update at every bump (sankey spec pin, the polar client lock, the tick-side lockstep check), and the Chart-to-Figure delegation contract grew the style_snapshot kwarg with the snapshot-fed export path. All four now assert the v13 reality the capture pair shipped in.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/xy/export.py (1)
1350-1364: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject
style_snapshotfor HTML exports.HTML exports render the live cascade and do not consume a native-writer snapshot. The current paths silently ignore this argument.
python/xy/export.py#L1350-L1364: addstyle_snapshotto the rejected HTML-only options when it is notNone.python/xy/components.py#L4382-L4400: forwardstyle_snapshotthrough the HTML branch so the common validation rejects it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/export.py` around lines 1350 - 1364, Reject non-None style_snapshot in the HTML export validation by adding it to the rejected options in python/xy/export.py lines 1350-1364. In the HTML branch of python/xy/components.py lines 4382-4400, forward style_snapshot into the common validation path so the rejection is applied.CHANGELOG.md (1)
12-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve the wire-protocol version conflict.
Line 12 states that this release uses wire protocol v13. Lines 32-34 state that nothing rides the wire and
PROTOCOL_VERSIONis unchanged. State one protocol status and align this release note with the specification.Based on PR objectives: phase 0 keeps wire protocol version 12, and version 13 is later work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 12 - 34, Resolve the conflicting protocol claims in the changelog: update the live style capture entry to state that this phase keeps wire protocol version 12, and remove or revise the v13 wording. Preserve the later-work status by keeping the note that style snapshots do not yet ride the wire and PROTOCOL_VERSION remains unchanged.
♻️ Duplicate comments (2)
python/xy/styling/resolved.py (2)
454-480: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the complete payload shape before coercion.
A payload with a valid header and
"declarations": Nonereachesfor decl in Noneand raisesTypeError.{"d": true}also passesisinstance(index, int)and can select declaration index1.Validate the top-level mapping and each collection before iteration. Reject
boolfor numeric schema fields. Convert all shape and conversion failures toValueError. Extend the malformed-payload test with non-mapping payloads,Nonecollections, and boolean declaration indexes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/styling/resolved.py` around lines 454 - 480, Update the deserialization function containing the environment, declarations, and instances parsing to validate the top-level payload is a Mapping and that declarations and instances are non-string collection values before iterating, converting malformed shapes and access failures to ValueError. Reject bool explicitly wherever numeric schema fields are validated, especially the declaration index raw["d"], while preserving existing range checks and ValueError messages. Extend the malformed-payload tests for non-mapping payloads, None collections, and boolean declaration indexes.
128-142: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClose the relative-unit validation bypasses.
-0.08emdoes not match this expression. The negative lookbehind blocks the match at0, and the expression has no optional sign.The full-body replacement also accepts values such as
rgb(1em 0% 0%)andhsl(1vw 40% 96%). It removes the relative unit before the scan. These values can enter declarations and tokens through the shared concrete-text path.Match an optional sign. Mask only valid percentage color components after color validation. Add regression cases for signed
emand%values, plus non-percentage units insidergb()andhsl().Also applies to: 163-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/styling/resolved.py` around lines 128 - 142, Update the relative-unit validation around _RELATIVE_UNIT_RE and _COLOR_FUNCTION_RE to match optional leading signs, so signed values such as -0.08em are detected. Replace full color-function masking with masking limited to percentage components only after color validation, ensuring non-percentage units inside rgb()/rgba()/hsl()/hsla() remain detectable. Add regression coverage for signed em and percent values and non-percentage units within rgb() and hsl().
🧹 Nitpick comments (2)
scripts/style_capture_smoke.py (2)
44-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the in-page wait budget with the outer deadline.
The in-page loop waits up to 60000 ms for the chart root. The outer
remaining()budget forRuntime.evaluatecan be smaller than that, because launch, navigation, and load already consumed part of the 120 s deadline. In that case the CDP call times out first and the clear"no chart root"message never returns.Pass the remaining budget into the expression so the page reports the specific failure.
♻️ Proposed change
-CAPTURE = """ +CAPTURE_TEMPLATE = """ (async () => { // The standalone client mounts after load (decode + first paint), so the // capture waits for the root the same way a user's eye does. - const until = Date.now() + 60000; + const until = Date.now() + %(budget_ms)d;Then build the expression with a budget derived from
remaining(), and keep the CDP timeout slightly larger:budget_s = max(1.0, remaining() - 5.0) expression = CAPTURE_TEMPLATE % {"budget_ms": int(budget_s * 1000)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/style_capture_smoke.py` around lines 44 - 58, Replace the fixed 60000 ms wait in the CAPTURE expression with a budget placeholder, then construct the evaluated expression using a budget derived from remaining() (subtracting 5 seconds and clamping to at least 1 second). Ensure the Runtime.evaluate timeout remains slightly larger than this in-page budget so the loop can return the explicit "no chart root" error.
91-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose the created tab after the capture.
_page_sessionreturns thetarget_idas the first element, and this call discards it. The script never callsTarget.closeTarget, so the tab stays open until theChromiumSessioncontext exits. The impact is bounded for a short-lived script. Closing the target keeps the failure output clean if the session is reused later.♻️ Proposed change
- _, sid, page_path = session._page_session(document, remaining()) + target_id, sid, page_path = session._page_session(document, remaining())Then close the target in a
try/finallyaround the evaluate call:try: ... # navigate, wait, evaluate finally: with contextlib.suppress(Exception): session._call("Target.closeTarget", {"targetId": target_id}, timeout_s=10.0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/style_capture_smoke.py` at line 91, Retain the target_id returned by session._page_session in the capture flow, then wrap the navigation, wait, and evaluate operations in a try/finally block. In finally, close the created target through session._call("Target.closeTarget", ...) while suppressing close errors, so cleanup runs even when capture fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/src/16_style_capture.ts`:
- Around line 77-94: In the declaration-capture loop, add a guard after
collecting properties and before building the JSON key or interning: when decl
has no properties, skip the current element. Keep non-empty declarations
unchanged, ensuring empty declaration objects are neither interned nor
referenced by emitted instances.
In `@js/src/54_kernel.ts`:
- Around line 912-935: Update _onKernelMsg so style_snapshot_request bypasses
the _glLost drop path and reaches _replyStyleSnapshot, allowing an error
response while communication remains usable. In _replyStyleSnapshot, replace the
destroyed-view early return with an attempted error reply, guarded so sending
cannot throw after destruction; preserve the existing successful capture and
exception response behavior.
In `@python/xy/export.py`:
- Around line 953-956: Update _coerce_style_snapshot() to validate cached
snapshot payload container shapes, including ensuring "instances" is an iterable
collection rather than a scalar, before calling snapshot_from_payload().
Normalize malformed payload TypeError failures to ValueError, and add coverage
through the public export APIs while preserving handling for
ResolvedStyleSnapshot and valid dictionaries.
- Around line 856-863: Make compatibility validation unconditional in the export
paths at python/xy/export.py:856-863, python/xy/export.py:1278-1288, and
python/xy/_figure.py:2258-2262, rejecting reserved "lossless" and preserving
strict behavior unless a snapshot explicitly covers every native-writer loss
source; only suppress warnings or errors after that coverage is proven. Update
spec/api/export.md:264-271 to qualify the lossless-remedy contract or document
this required coverage validation.
- Around line 981-989: Update _snapshot_styles and the export flow that consumes
it to prevent concurrent exports from sharing temporary fig.chrome_styles and
fig.style overlays. Synchronize all accesses that read or replace these figure
fields, or render using isolated per-export state, ensuring each snapshot’s
styles are applied only to its own render and the original figure state is
always restored.
In `@python/xy/styling/resolved.py`:
- Around line 476-480: Replace JSON-based slot geometry handling in the resolved
styling path around SlotInstance.geometry: transport geometry through raw f32
buffers and decode it into the canonical CPU-side f64 representation. Update the
corresponding serializer, TypeScript capture path, schema, and protocol
documentation consistently, removing the JSON “g” numeric representation while
preserving validation of four finite values.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 12-34: Resolve the conflicting protocol claims in the changelog:
update the live style capture entry to state that this phase keeps wire protocol
version 12, and remove or revise the v13 wording. Preserve the later-work status
by keeping the note that style snapshots do not yet ride the wire and
PROTOCOL_VERSION remains unchanged.
In `@python/xy/export.py`:
- Around line 1350-1364: Reject non-None style_snapshot in the HTML export
validation by adding it to the rejected options in python/xy/export.py lines
1350-1364. In the HTML branch of python/xy/components.py lines 4382-4400,
forward style_snapshot into the common validation path so the rejection is
applied.
---
Duplicate comments:
In `@python/xy/styling/resolved.py`:
- Around line 454-480: Update the deserialization function containing the
environment, declarations, and instances parsing to validate the top-level
payload is a Mapping and that declarations and instances are non-string
collection values before iterating, converting malformed shapes and access
failures to ValueError. Reject bool explicitly wherever numeric schema fields
are validated, especially the declaration index raw["d"], while preserving
existing range checks and ValueError messages. Extend the malformed-payload
tests for non-mapping payloads, None collections, and boolean declaration
indexes.
- Around line 128-142: Update the relative-unit validation around
_RELATIVE_UNIT_RE and _COLOR_FUNCTION_RE to match optional leading signs, so
signed values such as -0.08em are detected. Replace full color-function masking
with masking limited to percentage components only after color validation,
ensuring non-percentage units inside rgb()/rgba()/hsl()/hsla() remain
detectable. Add regression coverage for signed em and percent values and
non-percentage units within rgb() and hsl().
---
Nitpick comments:
In `@scripts/style_capture_smoke.py`:
- Around line 44-58: Replace the fixed 60000 ms wait in the CAPTURE expression
with a budget placeholder, then construct the evaluated expression using a
budget derived from remaining() (subtracting 5 seconds and clamping to at least
1 second). Ensure the Runtime.evaluate timeout remains slightly larger than this
in-page budget so the loop can return the explicit "no chart root" error.
- Line 91: Retain the target_id returned by session._page_session in the capture
flow, then wrap the navigation, wait, and evaluate operations in a try/finally
block. In finally, close the created target through
session._call("Target.closeTarget", ...) while suppressing close errors, so
cleanup runs even when capture fails.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 413bd941-8950-4f28-848e-bc45939eee7c
📒 Files selected for processing (23)
CHANGELOG.mdjs/src/00_header.tsjs/src/16_style_capture.tsjs/src/54_kernel.tsjs/src/60_entries.tspython/xy/_figure.pypython/xy/_svg.pypython/xy/channel.pypython/xy/components.pypython/xy/config.pypython/xy/export.pypython/xy/styling/__init__.pypython/xy/styling/declared.pypython/xy/styling/preflight.pypython/xy/styling/resolved.pypython/xy/widget.pyscripts/style_capture_smoke.pyspec/api/export.mdspec/design/wire-protocol.mdtests/test_declared_snapshot.pytests/test_resolved_style_snapshot.pytests/test_style_compatibility_report.pytests/test_style_snapshot_transport.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_resolved_style_snapshot.py
- python/xy/styling/preflight.py
Comment on lines
+77
to
+94
| const decl: Record<string, string | number> = {}; | ||
| for (const prop of STYLE_SNAPSHOT_PROPERTIES) { | ||
| if (!isSvg && SVG_ONLY_PROPERTIES.has(prop)) continue; | ||
| const value = style.getPropertyValue(CAPTURE_SOURCE[prop] ?? prop).trim(); | ||
| if (!value || SKIP_VALUES.has(value)) continue; | ||
| decl[prop] = value; | ||
| } | ||
| const key = JSON.stringify( | ||
| Object.keys(decl) | ||
| .sort() | ||
| .map((k) => [k, decl[k]]), | ||
| ); | ||
| let at = index.get(key); | ||
| if (at === undefined) { | ||
| at = declarations.length; | ||
| index.set(key, at); | ||
| declarations.push(decl); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not emit empty declarations.
If every schema property for an element is absent or in SKIP_VALUES, decl stays {}. Capture then interns that empty object and emits an instance that references it. The Python producer refuses the same content: SnapshotBuilder.intern raises "an empty declaration styles nothing; do not intern it". snapshot_from_payload accepts it, so the two producers disagree, and the payload carries instances that style nothing. Skip the element when decl has no properties.
♻️ Proposed fix
const key = JSON.stringify(
Object.keys(decl)
.sort()
.map((k) => [k, decl[k]]),
);
+ if (Object.keys(decl).length === 0) continue;
let at = index.get(key);Place the guard before the key build for clarity:
- const key = JSON.stringify(
+ if (Object.keys(decl).length === 0) continue;
+ const key = JSON.stringify(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const decl: Record<string, string | number> = {}; | |
| for (const prop of STYLE_SNAPSHOT_PROPERTIES) { | |
| if (!isSvg && SVG_ONLY_PROPERTIES.has(prop)) continue; | |
| const value = style.getPropertyValue(CAPTURE_SOURCE[prop] ?? prop).trim(); | |
| if (!value || SKIP_VALUES.has(value)) continue; | |
| decl[prop] = value; | |
| } | |
| const key = JSON.stringify( | |
| Object.keys(decl) | |
| .sort() | |
| .map((k) => [k, decl[k]]), | |
| ); | |
| let at = index.get(key); | |
| if (at === undefined) { | |
| at = declarations.length; | |
| index.set(key, at); | |
| declarations.push(decl); | |
| } | |
| const decl: Record<string, string | number> = {}; | |
| for (const prop of STYLE_SNAPSHOT_PROPERTIES) { | |
| if (!isSvg && SVG_ONLY_PROPERTIES.has(prop)) continue; | |
| const value = style.getPropertyValue(CAPTURE_SOURCE[prop] ?? prop).trim(); | |
| if (!value || SKIP_VALUES.has(value)) continue; | |
| decl[prop] = value; | |
| } | |
| if (Object.keys(decl).length === 0) continue; | |
| const key = JSON.stringify( | |
| Object.keys(decl) | |
| .sort() | |
| .map((k) => [k, decl[k]]), | |
| ); | |
| let at = index.get(key); | |
| if (at === undefined) { | |
| at = declarations.length; | |
| index.set(key, at); | |
| declarations.push(decl); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/src/16_style_capture.ts` around lines 77 - 94, In the declaration-capture
loop, add a guard after collecting properties and before building the JSON key
or interning: when decl has no properties, skip the current element. Keep
non-empty declarations unchanged, ensuring empty declaration objects are neither
interned nor referenced by emitted instances.
Comment on lines
+912
to
+935
| } else if (msg.type === "style_snapshot_request") { | ||
| this._replyStyleSnapshot(msg); | ||
| } | ||
| }, | ||
|
|
||
| // Capture the live cascade and reply (wire-protocol §8). Explicitly | ||
| // requested only — never on the hover or animation path — and settled | ||
| // first: fonts.ready plus two macrotask ticks, so the values are the | ||
| // document's, not a mid-layout frame's. Errors reply as errors: a request | ||
| // must never dangle a kernel-side future (§28: the outcome is said). | ||
| async _replyStyleSnapshot(msg) { | ||
| try { | ||
| await styleCaptureSettled(this.root.ownerDocument); | ||
| if (this._destroyed) return; | ||
| const snapshot = captureStyleSnapshot(this.root, { | ||
| styleEpoch: typeof msg.style_epoch === "number" ? msg.style_epoch : 0, | ||
| }); | ||
| this.comm.send({ type: "style_snapshot", request_id: msg.request_id, snapshot }); | ||
| } catch (err) { | ||
| this.comm.send({ | ||
| type: "style_snapshot", | ||
| request_id: msg.request_id, | ||
| error: String((err && (err as Error).message) || err), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reply with an error when the request cannot be served.
The comment at Lines 920-921 states that a request must never dangle a kernel-side future. Two paths break that promise:
- The
_glLostguard earlier in_onKernelMsgreturns before the dispatch chain for every type exceptappendandpick_result. Astyle_snapshot_requestthat arrives while the GL context is lost is dropped with no reply, although the comm is still alive. - Line 925 returns after settling when
this._destroyedis set, again with no reply.
In both cases capture_style_snapshot in python/xy/widget.py falls back to its 10 s asyncio.wait_for timeout. The caller then sees TimeoutError instead of the stated cause. Send an error reply on the paths where the comm is still usable.
🛠️ Proposed fix
async _replyStyleSnapshot(msg) {
try {
+ if (this._glLost) throw new Error("style capture unavailable: WebGL context lost");
await styleCaptureSettled(this.root.ownerDocument);
- if (this._destroyed) return;
+ if (this._destroyed) throw new Error("style capture unavailable: view destroyed");
const snapshot = captureStyleSnapshot(this.root, {The _glLost early return in _onKernelMsg must also let this type through:
- if (this._glLost && msg.type !== "append" && msg.type !== "pick_result") return;
+ if (
+ this._glLost
+ && msg.type !== "append"
+ && msg.type !== "pick_result"
+ && msg.type !== "style_snapshot_request"
+ ) return;A destroyed view cannot send, so keep a try around the error send if you adopt that branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/src/54_kernel.ts` around lines 912 - 935, Update _onKernelMsg so
style_snapshot_request bypasses the _glLost drop path and reaches
_replyStyleSnapshot, allowing an error response while communication remains
usable. In _replyStyleSnapshot, replace the destroyed-view early return with an
attempted error reply, guarded so sending cannot throw after destruction;
preserve the existing successful capture and exception response behavior.
Comment on lines
+953
to
+956
| if isinstance(value, ResolvedStyleSnapshot): | ||
| return value | ||
| if isinstance(value, dict): | ||
| return snapshot_from_payload(value) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize malformed snapshot payload errors.
_coerce_style_snapshot() accepts cached payload dictionaries here. A valid base payload with "instances": 42 reaches snapshot_from_payload() and raises TypeError during iteration. The documented boundary contract requires ValueError for malformed shapes. Validate container shapes before iteration and cover the public export APIs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/xy/export.py` around lines 953 - 956, Update _coerce_style_snapshot()
to validate cached snapshot payload container shapes, including ensuring
"instances" is an iterable collection rather than a scalar, before calling
snapshot_from_payload(). Normalize malformed payload TypeError failures to
ValueError, and add coverage through the public export APIs while preserving
handling for ResolvedStyleSnapshot and valid dictionaries.
Comment on lines
+476
to
+480
| if "g" in raw: | ||
| try: | ||
| values = tuple(float(v) for v in raw["g"]) | ||
| except (TypeError, ValueError) as exc: | ||
| raise ValueError(f"instance {raw!r} geometry must be four finite numbers") from exc |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not carry slot geometry as JSON numbers.
g accepts numeric JSON elements and stores them in SlotInstance.geometry. This violates the numeric wire contract.
Move geometry transport to raw f32 buffers. Decode into the canonical CPU-side f64 representation. Update the serializer, TypeScript capture path, schema, and protocol documentation together.
As per coding guidelines, “Never put JSON numbers on the wire; transfer data as raw f32 buffers.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/xy/styling/resolved.py` around lines 476 - 480, Replace JSON-based
slot geometry handling in the resolved styling path around
SlotInstance.geometry: transport geometry through raw f32 buffers and decode it
into the canonical CPU-side f64 representation. Update the corresponding
serializer, TypeScript capture path, schema, and protocol documentation
consistently, removing the JSON “g” numeric representation while preserving
validation of four finite values.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/pyplot/test_tick_side_rendering.py`:
- Line 123: Update spec/design-dossier.md so its documented PROTOCOL_VERSION is
13 instead of 3, matching the established protocol value. The anchor
tests/pyplot/test_tick_side_rendering.py:123-123, sibling
tests/test_polar_phase7_api.py:31-33, and sibling tests/test_sankey.py:230-230
require no direct changes; they confirm the correct value.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7cd04fb6-a6ec-4eeb-86b3-491c0dbbbfe2
📒 Files selected for processing (4)
tests/pyplot/test_tick_side_rendering.pytests/test_components.pytests/test_polar_phase7_api.pytests/test_sankey.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_components.py
|
|
||
| assert spec["x_axis"]["tick_sides"] == ["bottom", "top"] | ||
| assert spec["protocol"] == PROTOCOL_VERSION == 12 | ||
| assert spec["protocol"] == PROTOCOL_VERSION == 13 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -E 'PROTOCOL_VERSION|PROTOCOL =|["'"'"']protocol["'"'"']|wire protocol|protocol v(12|13)' \
python js spec tests CHANGELOG.mdRepository: reflex-dev/xy
Length of output: 292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- matching files ---'
git ls-files | rg '(^|/)(tests/(pyplot/test_tick_side_rendering\.py|test_polar_phase7_api\.py|test_sankey\.py)|00_header\.ts|CHANGELOG\.md|spec/)'
printf '%s\n' '--- protocol references ---'
rg -n -e 'PROTOCOL_VERSION' -e 'PROTOCOL[[:space:]]*=' -e '["'"'"']protocol["'"'"']' -e 'wire protocol' -e 'protocol v(12|13)' \
python js spec tests CHANGELOG.md 2>/dev/null || true
printf '%s\n' '--- relevant test sections ---'
sed -n '105,130p' tests/pyplot/test_tick_side_rendering.py
sed -n '1,45p' tests/test_polar_phase7_api.py
sed -n '215,240p' tests/test_sankey.py
printf '%s\n' '--- JavaScript header ---'
sed -n '1,100p' js/src/00_header.tsRepository: reflex-dev/xy
Length of output: 13791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository state ---'
git status --short
git diff --stat
printf '%s\n' '--- Python protocol definition and payload ---'
sed -n '35,55p' python/xy/config.py
sed -n '1,35p' python/xy/_payload.py
sed -n '260,285p' python/xy/_payload.py
printf '%s\n' '--- client validation ---'
rg -n -C 5 'spec\.protocol|PROTOCOL' js/src/50_chartview.ts js/src/00_header.ts
printf '%s\n' '--- protocol documentation ---'
sed -n '405,435p' spec/design/wire-protocol.md
sed -n '1,45p' CHANGELOG.md
sed -n '400,425p' spec/design-dossier.md
printf '%s\n' '--- static lockstep verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
root = Path(".")
config = (root / "python/xy/config.py").read_text()
payload = (root / "python/xy/_payload.py").read_text()
header = (root / "js/src/00_header.ts").read_text()
client = (root / "js/src/50_chartview.ts").read_text()
tests = [
root / "tests/pyplot/test_tick_side_rendering.py",
root / "tests/test_polar_phase7_api.py",
root / "tests/test_sankey.py",
]
def one(pattern, text, label):
match = re.search(pattern, text)
if not match:
raise SystemExit(f"{label}: no match")
return int(match.group(1))
python_version = one(r"^PROTOCOL_VERSION\s*=\s*(\d+)", config, "Python constant")
js_version = one(r"^export const PROTOCOL\s*=\s*(\d+)", header, "JavaScript constant")
payload_uses_python = '"protocol": PROTOCOL_VERSION' in payload
client_checks_protocol = "spec.protocol !== PROTOCOL" in client
print({
"python_version": python_version,
"javascript_version": js_version,
"payload_uses_python_constant": payload_uses_python,
"client_checks_protocol": client_checks_protocol,
"all_three_tests_pin_13": all("PROTOCOL_VERSION == 13" in p.read_text() for p in tests),
})
PYRepository: reflex-dev/xy
Length of output: 15497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all version claims ---'
rg -n -i -e 'PROTOCOL_VERSION' -e 'export const PROTOCOL' -e 'protocol[[:space:]]+v?12' -e 'protocol[[:space:]]+v?13' \
python js spec tests CHANGELOG.md
printf '%s\n' '--- wire-protocol version section ---'
sed -n '420,455p' spec/design/wire-protocol.md
printf '%s\n' '--- protocol-related implementation comments ---'
sed -n '25,52p' python/xy/config.py
sed -n '30,52p' js/src/00_header.ts
printf '%s\n' '--- corrected static lockstep verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
root = Path(".")
def one(pattern, text, label):
match = re.search(pattern, text, re.MULTILINE)
if not match:
raise SystemExit(f"{label}: no match")
return int(match.group(1))
config = (root / "python/xy/config.py").read_text()
payload = (root / "python/xy/_payload.py").read_text()
header = (root / "js/src/00_header.ts").read_text()
client = (root / "js/src/50_chartview.ts").read_text()
tests = [
root / "tests/pyplot/test_tick_side_rendering.py",
root / "tests/test_polar_phase7_api.py",
root / "tests/test_sankey.py",
]
python_version = one(r"^PROTOCOL_VERSION\s*=\s*(\d+)", config, "Python constant")
js_version = one(r"^export const PROTOCOL\s*=\s*(\d+)", header, "JavaScript constant")
payload_uses_python = '"protocol": PROTOCOL_VERSION' in payload
client_checks_protocol = "spec.protocol !== PROTOCOL" in client
tests_pin_13 = [p.name for p in tests if "PROTOCOL_VERSION == 13" in p.read_text()]
print({
"python_version": python_version,
"javascript_version": js_version,
"payload_uses_python_constant": payload_uses_python,
"client_checks_protocol": client_checks_protocol,
"tests_pinning_13": tests_pin_13,
"lockstep_constants_match": python_version == js_version,
})
PYRepository: reflex-dev/xy
Length of output: 8537
Update the stale protocol value in spec/design-dossier.md.
The producer, payload, JavaScript header, client handshake, and all three tests consistently use protocol 13. The wire-protocol specification also documents 13. spec/design-dossier.md still lists PROTOCOL_VERSION as 3 and must be updated.
📍 Affects 3 files
tests/pyplot/test_tick_side_rendering.py#L123-L123(this comment)tests/test_polar_phase7_api.py#L31-L33tests/test_sankey.py#L230-L230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/pyplot/test_tick_side_rendering.py` at line 123, Update
spec/design-dossier.md so its documented PROTOCOL_VERSION is 13 instead of 3,
matching the established protocol value. The anchor
tests/pyplot/test_tick_side_rendering.py:123-123, sibling
tests/test_polar_phase7_api.py:31-33, and sibling tests/test_sankey.py:230-230
require no direct changes; they confirm the correct value.
Contributor
There was a problem hiding this comment.
7 issues found across 19 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_style_snapshot_transport.py">
<violation number="1" location="tests/test_style_snapshot_transport.py:82">
P3: This test asserts `_pending_style_snapshots == {}` without ever starting a capture, so the assertion is trivially true and does not exercise the ignore-unknown-id path in `_settle_style_snapshot`. To verify the intended behavior, start a real capture first, deliver a reply carrying a different (never-sent) request_id, then assert the pending future for the real request is still unsettled (e.g. the capture still times out and the dict is only empty afterward).</violation>
</file>
<file name="js/src/54_kernel.ts">
<violation number="1" location="js/src/54_kernel.ts:925">
P2: A style snapshot request can now go unanswered when the view is destroyed during the settle wait, which makes kernel callers block until timeout instead of getting an immediate outcome. This comes from returning early on `_destroyed` before sending either `snapshot` or `error`; replying with an explicit error keeps the request/response contract intact.</violation>
</file>
<file name="python/xy/export.py">
<violation number="1" location="python/xy/export.py:862">
P2: With `style_snapshot` provided, invalid `compatibility` values are silently accepted because compatibility validation is skipped along with `_enforce_compatibility`. Validating `compatibility` even for snapshot-fed exports (or rejecting non-default modes there) would keep typo detection consistent.</violation>
</file>
<file name="scripts/style_capture_smoke.py">
<violation number="1" location="scripts/style_capture_smoke.py:30">
P2: This smoke can fail with `no chromium found` on environments that already satisfy other repository smokes, because its browser candidate list is narrower than the established script set. Aligning this list with the shared candidates avoids environment-specific false failures.</violation>
<violation number="2" location="scripts/style_capture_smoke.py:48">
P3: The browser-side wait budget is shorter than the script’s global timeout, so this can fail early on slow runners before the outer deadline is exhausted. Matching the in-page wait window to the shared timeout prevents avoidable time-budget mismatches.</violation>
</file>
<file name="js/src/16_style_capture.ts">
<violation number="1" location="js/src/16_style_capture.ts:121">
P2: Snapshot-fed native export can miss host/theme token values that come from CSS cascade rather than inline root style, causing style drift versus the live chart. Expanding capture beyond `root.style`-enumerated names would preserve stylesheet-driven `--chart-*`/`--xy-*` tokens.</violation>
</file>
<file name="python/xy/widget.py">
<violation number="1" location="python/xy/widget.py:181">
P3: The `style_epoch` sent on every capture request is always `0`. `Figure` exposes no `style_epoch` attribute, so `getattr(self._figure, "style_epoch", 0)` silently falls back to the default every time — a repo-wide search finds no code that ever sets it. That means the field meant to correlate a captured snapshot with the figure's current style state can never actually do so; consumers reading `style_epoch` on a snapshot will always see a stale/constant value. Either expose a real epoch on the figure (e.g. bump it whenever chrome_styles/style are mutated) or drop the field rather than carrying a value that is always 0.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| async _replyStyleSnapshot(msg) { | ||
| try { | ||
| await styleCaptureSettled(this.root.ownerDocument); | ||
| if (this._destroyed) return; |
Contributor
There was a problem hiding this comment.
P2: A style snapshot request can now go unanswered when the view is destroyed during the settle wait, which makes kernel callers block until timeout instead of getting an immediate outcome. This comes from returning early on _destroyed before sending either snapshot or error; replying with an explicit error keeps the request/response contract intact.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At js/src/54_kernel.ts, line 925:
<comment>A style snapshot request can now go unanswered when the view is destroyed during the settle wait, which makes kernel callers block until timeout instead of getting an immediate outcome. This comes from returning early on `_destroyed` before sending either `snapshot` or `error`; replying with an explicit error keeps the request/response contract intact.</comment>
<file context>
@@ -908,6 +909,30 @@ Object.assign(ChartView.prototype, {
+ async _replyStyleSnapshot(msg) {
+ try {
+ await styleCaptureSettled(this.root.ownerDocument);
+ if (this._destroyed) return;
+ const snapshot = captureStyleSnapshot(this.root, {
+ styleEpoch: typeof msg.style_epoch === "number" ? msg.style_epoch : 0,
</file context>
Suggested change
| if (this._destroyed) return; | |
| if (this._destroyed) { | |
| this.comm.send({ | |
| type: "style_snapshot", | |
| request_id: msg.request_id, | |
| error: "chart destroyed before style capture settled", | |
| }); | |
| return; | |
| } |
| @@ -15,7 +15,7 @@ | |||
| import sys | |||
Contributor
There was a problem hiding this comment.
P2: With style_snapshot provided, invalid compatibility values are silently accepted because compatibility validation is skipped along with _enforce_compatibility. Validating compatibility even for snapshot-fed exports (or rejecting non-default modes there) would keep typo detection consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/export.py, line 862:
<comment>With `style_snapshot` provided, invalid `compatibility` values are silently accepted because compatibility validation is skipped along with `_enforce_compatibility`. Validating `compatibility` even for snapshot-fed exports (or rejecting non-default modes there) would keep typo detection consistent.</comment>
<file context>
@@ -852,11 +853,19 @@ def to_png(
+ "style_snapshot feeds the native writers; the Chromium engine renders "
+ "the live cascade itself — drop one of the two"
+ )
+ if snapshot is None:
+ _enforce_compatibility(fig, "png", resolved_engine, custom_css, compatibility)
if resolved_engine == "native":
</file context>
| from xy._chromium import ChromiumSession # noqa: E402 | ||
| from xy.styling.resolved import snapshot_from_payload # noqa: E402 | ||
|
|
||
| CHROMIUM_CANDIDATES = [ |
Contributor
There was a problem hiding this comment.
P2: This smoke can fail with no chromium found on environments that already satisfy other repository smokes, because its browser candidate list is narrower than the established script set. Aligning this list with the shared candidates avoids environment-specific false failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/style_capture_smoke.py, line 30:
<comment>This smoke can fail with `no chromium found` on environments that already satisfy other repository smokes, because its browser candidate list is narrower than the established script set. Aligning this list with the shared candidates avoids environment-specific false failures.</comment>
<file context>
@@ -0,0 +1,141 @@
+from xy._chromium import ChromiumSession # noqa: E402
+from xy.styling.resolved import snapshot_from_payload # noqa: E402
+
+CHROMIUM_CANDIDATES = [
+ "chrome-headless-shell",
+ "chromium",
</file context>
| // Inline root tokens are the authored/theme values the client applied; | ||
| // computed custom properties are not enumerable, so read the ones the | ||
| // chart itself declares on its root style attribute. | ||
| for (const name of Array.from(root.style)) { |
Contributor
There was a problem hiding this comment.
P2: Snapshot-fed native export can miss host/theme token values that come from CSS cascade rather than inline root style, causing style drift versus the live chart. Expanding capture beyond root.style-enumerated names would preserve stylesheet-driven --chart-*/--xy-* tokens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At js/src/16_style_capture.ts, line 121:
<comment>Snapshot-fed native export can miss host/theme token values that come from CSS cascade rather than inline root style, causing style drift versus the live chart. Expanding capture beyond `root.style`-enumerated names would preserve stylesheet-driven `--chart-*`/`--xy-*` tokens.</comment>
<file context>
@@ -0,0 +1,161 @@
+ // Inline root tokens are the authored/theme values the client applied;
+ // computed custom properties are not enumerable, so read the ones the
+ // chart itself declares on its root style attribute.
+ for (const name of Array.from(root.style)) {
+ if (name.startsWith(prefix)) {
+ const value = rootStyle.getPropertyValue(name).trim();
</file context>
Comment on lines
+82
to
+87
| assert widget._pending_style_snapshots == {} | ||
|
|
||
|
|
||
| def test_client_capture_errors_raise_instead_of_dangling() -> None: | ||
| widget, sent = _widget() | ||
|
|
Contributor
There was a problem hiding this comment.
P3: This test asserts _pending_style_snapshots == {} without ever starting a capture, so the assertion is trivially true and does not exercise the ignore-unknown-id path in _settle_style_snapshot. To verify the intended behavior, start a real capture first, deliver a reply carrying a different (never-sent) request_id, then assert the pending future for the real request is still unsettled (e.g. the capture still times out and the dict is only empty afterward).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_style_snapshot_transport.py, line 82:
<comment>This test asserts `_pending_style_snapshots == {}` without ever starting a capture, so the assertion is trivially true and does not exercise the ignore-unknown-id path in `_settle_style_snapshot`. To verify the intended behavior, start a real capture first, deliver a reply carrying a different (never-sent) request_id, then assert the pending future for the real request is still unsettled (e.g. the capture still times out and the dict is only empty afterward).</comment>
<file context>
@@ -0,0 +1,200 @@
+ snapshot = asyncio.run(run())
+ assert isinstance(snapshot, ResolvedStyleSnapshot)
+ assert snapshot.instances[0].slot == "tick_label"
+ assert widget._pending_style_snapshots == {}
+
+
</file context>
Suggested change
| assert widget._pending_style_snapshots == {} | |
| def test_client_capture_errors_raise_instead_of_dangling() -> None: | |
| widget, sent = _widget() | |
| def test_stale_or_unknown_request_ids_are_ignored() -> None: | |
| widget, sent = _widget() | |
| async def run(): | |
| task = asyncio.ensure_future(widget.capture_style_snapshot(timeout=0.1)) | |
| await asyncio.sleep(0) | |
| request = sent[-1][0] | |
| widget._on_custom_msg( | |
| widget, | |
| {"type": "style_snapshot", "request_id": "never-sent", "snapshot": {}}, | |
| None, | |
| ) | |
| await asyncio.sleep(0) | |
| # The unknown id must not settle the real pending future. | |
| assert request["request_id"] in widget._pending_style_snapshots | |
| with pytest.raises(asyncio.TimeoutError): | |
| await task | |
| return widget._pending_style_snapshots | |
| assert asyncio.run(run()) == {} |
| (async () => { | ||
| // The standalone client mounts after load (decode + first paint), so the | ||
| // capture waits for the root the same way a user's eye does. | ||
| const until = Date.now() + 60000; |
Contributor
There was a problem hiding this comment.
P3: The browser-side wait budget is shorter than the script’s global timeout, so this can fail early on slow runners before the outer deadline is exhausted. Matching the in-page wait window to the shared timeout prevents avoidable time-budget mismatches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/style_capture_smoke.py, line 48:
<comment>The browser-side wait budget is shorter than the script’s global timeout, so this can fail early on slow runners before the outer deadline is exhausted. Matching the in-page wait window to the shared timeout prevents avoidable time-budget mismatches.</comment>
<file context>
@@ -0,0 +1,141 @@
+(async () => {
+ // The standalone client mounts after load (decode + first paint), so the
+ // capture waits for the root the same way a user's eye does.
+ const until = Date.now() + 60000;
+ let root = null;
+ while (!(root = document.querySelector('[data-xy-slot="root"]'))) {
</file context>
| { | ||
| "type": "style_snapshot_request", | ||
| "request_id": request_id, | ||
| "style_epoch": int(getattr(self._figure, "style_epoch", 0) or 0), |
Contributor
There was a problem hiding this comment.
P3: The style_epoch sent on every capture request is always 0. Figure exposes no style_epoch attribute, so getattr(self._figure, "style_epoch", 0) silently falls back to the default every time — a repo-wide search finds no code that ever sets it. That means the field meant to correlate a captured snapshot with the figure's current style state can never actually do so; consumers reading style_epoch on a snapshot will always see a stale/constant value. Either expose a real epoch on the figure (e.g. bump it whenever chrome_styles/style are mutated) or drop the field rather than carrying a value that is always 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/widget.py, line 181:
<comment>The `style_epoch` sent on every capture request is always `0`. `Figure` exposes no `style_epoch` attribute, so `getattr(self._figure, "style_epoch", 0)` silently falls back to the default every time — a repo-wide search finds no code that ever sets it. That means the field meant to correlate a captured snapshot with the figure's current style state can never actually do so; consumers reading `style_epoch` on a snapshot will always see a stale/constant value. Either expose a real epoch on the figure (e.g. bump it whenever chrome_styles/style are mutated) or drop the field rather than carrying a value that is always 0.</comment>
<file context>
@@ -150,6 +152,51 @@ def reset_view(self, axes: Any = None) -> None:
+ {
+ "type": "style_snapshot_request",
+ "request_id": request_id,
+ "style_epoch": int(getattr(self._figure, "style_epoch", 0) or 0),
+ }
+ )
</file context>
Phase 3 of the styling-compatibility program: a chart styled with classes and self-contained custom CSS exports natively — no mount, no browser — with the live browser retained as the oracle it should be. The extension (cascade/, its own ~3.8 MB cdylib in the same workspace, lightningcss pinned exact) parses and normalizes stylesheets; the resolver cascades the published profile over the synthetic slot DOM: class / [data-xy-slot=…] / universal / :root selectors with descendant and child combinators, importance-specificity-order cascade, custom properties with fallbacks to a depth-capped fixpoint, em/rem against the font-size cascade (parent-based for font-size itself, own-based for everything else), prefers-color-scheme media, and the inherited text properties flowing top-down after each node's own cascade. Everything outside the profile — pseudo-classes, other at-rules, sibling combinators, percentage lengths — lands in an `unsupported` list with its reason, never in a guessed value (§28). The synthetic tree is browser evidence, not a guess: every parent a rendered chart mounts was probed from a live headless-Chromium DOM, and the remainder follows the client mount sites. python/xy/styling/cascade.py is the lazy ctypes boundary (JSON in/out, one free function, panics caught at the ABI): imported only when a native-cascade export is requested, absent-extension raises with the build command, box shorthands split to the schema's longhands. Exports: style_source="native_cascade" on to_png/to_svg/to_image/write_image at chart, figure, and module level. The cascade IS the CSS engine there, so custom_css is consumed natively instead of forcing Chromium; unsupported constructs surface as one StyleCompatibilityWarning by default and refuse in strict — a brand-new surface has no legacy silence to preserve. The snapshot and the cascade are mutually exclusive sources, and a Chromium pin rejects the combination. Packaging: the workspace builds both cdylibs in one cargo invocation; hatch_build force-includes the cascade beside the core when present and announces a wheel built without it. scripts/cascade_differential_smoke.py is the exit gate in executable form: the same chart and stylesheet resolved by headless Chromium (live capture) and by the mount-free cascade agree on the shared profile, colors compared parsed since Lightning CSS normalizes spellings the browser does not.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/xy/export.py (1)
1417-1443: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject native-style options for HTML exports.
Line 1442 passes neither
style_snapshotnorstyle_sourcetoto_html(). With default compatibility, HTML export silently ignores a supplied snapshot orstyle_source="native_cascade". Reject both non-default options with the other image-only settings.Proposed fix
("compatibility", compatibility, "legacy"), + ("style_snapshot", style_snapshot, None), + ("style_source", style_source, "declared"), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/export.py` around lines 1417 - 1443, Update the HTML rejection logic in the fmt == "html" branch to include non-default style_snapshot and style_source values alongside the existing image-only options, then pass the validated style arguments to to_html() as appropriate. Ensure supplied snapshots or style_source="native_cascade" are rejected under default compatibility instead of being silently ignored.
🧹 Nitpick comments (1)
python/xy/styling/cascade.py (1)
180-206: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNarrow the docstring to
padding. Schema v1 does not definemarginorinset, andborder-widthandborder-radiusare accepted without expansion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/styling/cascade.py` around lines 180 - 206, Narrow the _expand_shorthands docstring to describe only padding expansion and its schema v1 longhand mapping; remove any implication that margin, inset, border-width, or border-radius are expanded, while preserving the existing padding behavior and precedence description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cascade/src/resolve.rs`:
- Around line 422-455: Update matches so descendant-combinator traversal
backtracks instead of permanently selecting the nearest matching ancestor. Add a
recursive ancestor-walk helper around the compounds iteration that, when the
remaining chain fails from one matching ancestor, retries higher ancestors;
preserve direct-child behavior and return true only when the full selector chain
matches.
In `@hatch_build.py`:
- Around line 188-198: Update the cascade artifact lookup in the
_provision_native flow to derive the on-disk filename from native_src.name
rather than lib_name, while retaining lib_name for the wheel-internal
destination path in force_include. Ensure the resolved cascade source path
matches the artifact format produced alongside native_src, including wasm
builds.
In `@python/xy/export.py`:
- Around line 1027-1036: Update _cascade_snapshot() to accept a fmt parameter
and use it when calling route_resolved instead of hardcoding "png"; pass
fmt="png" from to_png, fmt=fmt from to_image, and fmt="svg" from Figure.to_svg
so strict StyleCompatibilityError reports match the requested export format.
In `@python/xy/styling/cascade.py`:
- Around line 169-177: Update the native reply handling around the cascade
resolver to copy the buffer with ctypes.string_at(out, out_len.value) before
lib.xy_cascade_free, replacing the per-byte bytearray loop while preserving
cleanup in the finally block. Handle JSON decoding failures when code is
non-zero by retaining the raw payload as error detail and raising the existing
native cascade ValueError instead of allowing JSONDecodeError to escape.
In `@scripts/cascade_differential_smoke.py`:
- Around line 70-77: The hex-color parsing logic in the `#` branch must support
four-digit RGBA values and explicitly reject unsupported lengths. Update the
parsing near the existing three- and six-digit expansion branches to expand four
digits appropriately before extracting channels, and validate that only
supported lengths proceed; raise the parser’s expected invalid-value error for
all other lengths.
---
Outside diff comments:
In `@python/xy/export.py`:
- Around line 1417-1443: Update the HTML rejection logic in the fmt == "html"
branch to include non-default style_snapshot and style_source values alongside
the existing image-only options, then pass the validated style arguments to
to_html() as appropriate. Ensure supplied snapshots or
style_source="native_cascade" are rejected under default compatibility instead
of being silently ignored.
---
Nitpick comments:
In `@python/xy/styling/cascade.py`:
- Around line 180-206: Narrow the _expand_shorthands docstring to describe only
padding expansion and its schema v1 longhand mapping; remove any implication
that margin, inset, border-width, or border-radius are expanded, while
preserving the existing padding behavior and precedence description.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af658ee5-fb01-41c0-b595-277f215aef25
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
CHANGELOG.mdCargo.tomlcascade/Cargo.tomlcascade/src/lib.rscascade/src/resolve.rshatch_build.pypython/xy/_figure.pypython/xy/components.pypython/xy/export.pypython/xy/styling/__init__.pypython/xy/styling/cascade.pyscripts/cascade_differential_smoke.pyspec/api/export.mdtests/test_components.pytests/test_native_cascade.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_components.py
- CHANGELOG.md
- spec/api/export.md
- python/xy/_figure.py
- python/xy/components.py
Comment on lines
+422
to
+455
| fn matches(profile: &ProfileSelector, doc: &Document, index: usize) -> bool { | ||
| // Subject compound first, then walk ancestors per combinator. | ||
| let mut compounds = profile.compounds.iter(); | ||
| let Some((subject, _)) = compounds.next() else { | ||
| return false; | ||
| }; | ||
| if !compound_matches(subject, doc, index) { | ||
| return false; | ||
| } | ||
| let mut position = index; | ||
| for (compound, child) in compounds { | ||
| if *child { | ||
| match doc.nodes[position].parent { | ||
| Some(p) if compound_matches(compound, doc, p) => position = p, | ||
| _ => return false, | ||
| } | ||
| } else { | ||
| let mut cursor = doc.nodes[position].parent; | ||
| loop { | ||
| match cursor { | ||
| Some(p) => { | ||
| if compound_matches(compound, doc, p) { | ||
| position = p; | ||
| break; | ||
| } | ||
| cursor = doc.nodes[p].parent; | ||
| } | ||
| None => return false, | ||
| } | ||
| } | ||
| } | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Descendant matching is greedy and can miss a valid match.
matches takes the nearest ancestor that satisfies a descendant compound and never backtracks. If a later compound then fails, the selector is rejected even when another ancestor would satisfy the whole chain.
Example chain: root(.p) → a(.q) → b(.q) → node(.r), selector .p > .q .r. The walk selects b for .q, then requires .p on b's parent a and fails. A browser matches through a. The result is a silently dropped declaration, which the profile promises to report instead.
Make the ancestor walk recursive so a failed tail retries the next ancestor.
🐛 Proposed fix: backtracking ancestor walk
-fn matches(profile: &ProfileSelector, doc: &Document, index: usize) -> bool {
- // Subject compound first, then walk ancestors per combinator.
- let mut compounds = profile.compounds.iter();
- let Some((subject, _)) = compounds.next() else {
- return false;
- };
- if !compound_matches(subject, doc, index) {
- return false;
- }
- let mut position = index;
- for (compound, child) in compounds {
- if *child {
- match doc.nodes[position].parent {
- Some(p) if compound_matches(compound, doc, p) => position = p,
- _ => return false,
- }
- } else {
- let mut cursor = doc.nodes[position].parent;
- loop {
- match cursor {
- Some(p) => {
- if compound_matches(compound, doc, p) {
- position = p;
- break;
- }
- cursor = doc.nodes[p].parent;
- }
- None => return false,
- }
- }
- }
- }
- true
-}
+fn matches(profile: &ProfileSelector, doc: &Document, index: usize) -> bool {
+ // Subject compound first, then walk ancestors per combinator with
+ // backtracking: a failed tail retries the next matching ancestor.
+ let Some((subject, _)) = profile.compounds.first() else {
+ return false;
+ };
+ if !compound_matches(subject, doc, index) {
+ return false;
+ }
+ match_tail(&profile.compounds[1..], doc, index)
+}
+
+fn match_tail(rest: &[(Vec<Simple>, bool)], doc: &Document, position: usize) -> bool {
+ let Some(((compound, child), tail)) = rest.split_first().map(|(h, t)| (h, t)) else {
+ return true;
+ };
+ if *child {
+ return match doc.nodes[position].parent {
+ Some(p) => compound_matches(compound, doc, p) && match_tail(tail, doc, p),
+ None => false,
+ };
+ }
+ let mut cursor = doc.nodes[position].parent;
+ while let Some(p) = cursor {
+ if compound_matches(compound, doc, p) && match_tail(tail, doc, p) {
+ return true;
+ }
+ cursor = doc.nodes[p].parent;
+ }
+ false
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn matches(profile: &ProfileSelector, doc: &Document, index: usize) -> bool { | |
| // Subject compound first, then walk ancestors per combinator. | |
| let mut compounds = profile.compounds.iter(); | |
| let Some((subject, _)) = compounds.next() else { | |
| return false; | |
| }; | |
| if !compound_matches(subject, doc, index) { | |
| return false; | |
| } | |
| let mut position = index; | |
| for (compound, child) in compounds { | |
| if *child { | |
| match doc.nodes[position].parent { | |
| Some(p) if compound_matches(compound, doc, p) => position = p, | |
| _ => return false, | |
| } | |
| } else { | |
| let mut cursor = doc.nodes[position].parent; | |
| loop { | |
| match cursor { | |
| Some(p) => { | |
| if compound_matches(compound, doc, p) { | |
| position = p; | |
| break; | |
| } | |
| cursor = doc.nodes[p].parent; | |
| } | |
| None => return false, | |
| } | |
| } | |
| } | |
| } | |
| true | |
| } | |
| fn matches(profile: &ProfileSelector, doc: &Document, index: usize) -> bool { | |
| // Subject compound first, then walk ancestors per combinator with | |
| // backtracking: a failed tail retries the next matching ancestor. | |
| let Some((subject, _)) = profile.compounds.first() else { | |
| return false; | |
| }; | |
| if !compound_matches(subject, doc, index) { | |
| return false; | |
| } | |
| match_tail(&profile.compounds[1..], doc, index) | |
| } | |
| fn match_tail(rest: &[(Vec<Simple>, bool)], doc: &Document, position: usize) -> bool { | |
| let Some(((compound, child), tail)) = rest.split_first().map(|(h, t)| (h, t)) else { | |
| return true; | |
| }; | |
| if *child { | |
| return match doc.nodes[position].parent { | |
| Some(p) => compound_matches(compound, doc, p) && match_tail(tail, doc, p), | |
| None => false, | |
| }; | |
| } | |
| let mut cursor = doc.nodes[position].parent; | |
| while let Some(p) = cursor { | |
| if compound_matches(compound, doc, p) && match_tail(tail, doc, p) { | |
| return true; | |
| } | |
| cursor = doc.nodes[p].parent; | |
| } | |
| false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cascade/src/resolve.rs` around lines 422 - 455, Update matches so
descendant-combinator traversal backtracks instead of permanently selecting the
nearest matching ancestor. Add a recursive ancestor-walk helper around the
compounds iteration that, when the remaining chain fails from one matching
ancestor, retries higher ancestors; preserve direct-child behavior and return
true only when the full selector chain matches.
Comment on lines
+188
to
+198
| cascade_name = lib_name.replace("xy_core", "xy_cascade") | ||
| cascade_src = native_src.parent / cascade_name | ||
| if cascade_src.is_file(): | ||
| build_data["force_include"][str(cascade_src)] = f"xy/_native_lib/{cascade_name}" | ||
| else: | ||
| print( | ||
| "xy: wheel built WITHOUT the optional xy-cascade extension " | ||
| f"({cascade_src} not found); style_source='native_cascade' " | ||
| "will raise with the build instruction.", | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Derive the on-disk cascade filename from the resolved core artifact, not from the wheel-internal name.
lib_name is the wheel-internal destination name, which _lib_filename deliberately differs from the artifact cargo emits. For wasm32-unknown-emscripten, _lib_filename returns libxy_core.so while cargo emits a .wasm file, and _provision_native resolves the real artifact through _resolve_built. Line 189 then searches for libxy_cascade.so next to that artifact, so the lookup fails and the wheel omits the extension even though the extension was built.
Use native_src.name for the disk lookup and keep lib_name for the wheel path.
🔧 Proposed fix
- cascade_name = lib_name.replace("xy_core", "xy_cascade")
- cascade_src = native_src.parent / cascade_name
+ cascade_name = lib_name.replace("xy_core", "xy_cascade")
+ cascade_src = native_src.parent / native_src.name.replace("xy_core", "xy_cascade")
if cascade_src.is_file():
build_data["force_include"][str(cascade_src)] = f"xy/_native_lib/{cascade_name}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cascade_name = lib_name.replace("xy_core", "xy_cascade") | |
| cascade_src = native_src.parent / cascade_name | |
| if cascade_src.is_file(): | |
| build_data["force_include"][str(cascade_src)] = f"xy/_native_lib/{cascade_name}" | |
| else: | |
| print( | |
| "xy: wheel built WITHOUT the optional xy-cascade extension " | |
| f"({cascade_src} not found); style_source='native_cascade' " | |
| "will raise with the build instruction.", | |
| file=sys.stderr, | |
| ) | |
| cascade_name = lib_name.replace("xy_core", "xy_cascade") | |
| cascade_src = native_src.parent / native_src.name.replace("xy_core", "xy_cascade") | |
| if cascade_src.is_file(): | |
| build_data["force_include"][str(cascade_src)] = f"xy/_native_lib/{cascade_name}" | |
| else: | |
| print( | |
| "xy: wheel built WITHOUT the optional xy-cascade extension " | |
| f"({cascade_src} not found); style_source='native_cascade' " | |
| "will raise with the build instruction.", | |
| file=sys.stderr, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hatch_build.py` around lines 188 - 198, Update the cascade artifact lookup in
the _provision_native flow to derive the on-disk filename from native_src.name
rather than lib_name, while retaining lib_name for the wheel-internal
destination path in force_include. Ensure the resolved cascade source path
matches the artifact format produced alongside native_src, including wasm
builds.
Comment on lines
+1027
to
+1036
| snapshot, unsupported = _cascade.resolve_for_figure(fig, custom_css=custom_css or "") | ||
| if unsupported: | ||
| summary = "; ".join(unsupported) | ||
| if compatibility == "strict": | ||
| from .styling.preflight import route_resolved | ||
|
|
||
| raise StyleCompatibilityError( | ||
| f"native cascade could not honor: {summary}", | ||
| route_resolved(fig, fmt="png", resolved_engine="native", custom_css=None), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the requested format in the strict-error report.
Line 1035 always passes fmt="png" to route_resolved. A strict native-cascade error from SVG, PDF, or direct Figure.to_svg() export therefore exposes a StyleCompatibilityError.report for PNG instead of the requested target. Add fmt to _cascade_snapshot() and forward the actual format from every caller.
Proposed fix
def _cascade_snapshot(
fig: "Figure",
custom_css: Optional[str],
compatibility: str,
+ *,
+ fmt: str,
) -> Any:
...
- route_resolved(fig, fmt="png", resolved_engine="native", custom_css=None),
+ route_resolved(fig, fmt=fmt, resolved_engine="native", custom_css=None),Pass fmt="png" from to_png, fmt=fmt from to_image, and fmt="svg" from Figure.to_svg.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/xy/export.py` around lines 1027 - 1036, Update _cascade_snapshot() to
accept a fmt parameter and use it when calling route_resolved instead of
hardcoding "png"; pass fmt="png" from to_png, fmt=fmt from to_image, and
fmt="svg" from Figure.to_svg so strict StyleCompatibilityError reports match the
requested export format.
Comment on lines
+169
to
+177
| try: | ||
| payload = bytes(bytearray(out[i] for i in range(out_len.value))) | ||
| finally: | ||
| lib.xy_cascade_free(out, out_len) | ||
| reply = json.loads(payload.decode("utf-8")) | ||
| if code != 0: | ||
| detail = reply.get("error") or repr(payload[:200]) | ||
| raise ValueError(f"native cascade failed: {detail}") | ||
| return reply |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Copy the reply with ctypes.string_at instead of a per-byte Python loop.
Line 170 indexes the native buffer one byte at a time through ctypes. Each index is a Python-level call, so a reply of tens of kilobytes costs tens of thousands of calls on every export. ctypes.string_at performs one memcpy.
Also handle a non-JSON reply. If json.loads fails on a non-zero code, the resolver error text is lost and the traceback shows a JSONDecodeError instead.
♻️ Proposed fix
try:
- payload = bytes(bytearray(out[i] for i in range(out_len.value)))
+ payload = ctypes.string_at(out, out_len.value)
finally:
lib.xy_cascade_free(out, out_len)
- reply = json.loads(payload.decode("utf-8"))
+ try:
+ reply = json.loads(payload.decode("utf-8"))
+ except (UnicodeDecodeError, json.JSONDecodeError) as exc:
+ raise ValueError(
+ f"native cascade returned an undecodable reply (code {code}): {payload[:200]!r}"
+ ) from exc
if code != 0:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| payload = bytes(bytearray(out[i] for i in range(out_len.value))) | |
| finally: | |
| lib.xy_cascade_free(out, out_len) | |
| reply = json.loads(payload.decode("utf-8")) | |
| if code != 0: | |
| detail = reply.get("error") or repr(payload[:200]) | |
| raise ValueError(f"native cascade failed: {detail}") | |
| return reply | |
| try: | |
| payload = ctypes.string_at(out, out_len.value) | |
| finally: | |
| lib.xy_cascade_free(out, out_len) | |
| try: | |
| reply = json.loads(payload.decode("utf-8")) | |
| except (UnicodeDecodeError, json.JSONDecodeError) as exc: | |
| raise ValueError( | |
| f"native cascade returned an undecodable reply (code {code}): {payload[:200]!r}" | |
| ) from exc | |
| if code != 0: | |
| detail = reply.get("error") or repr(payload[:200]) | |
| raise ValueError(f"native cascade failed: {detail}") | |
| return reply |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/xy/styling/cascade.py` around lines 169 - 177, Update the native reply
handling around the cascade resolver to copy the buffer with
ctypes.string_at(out, out_len.value) before lib.xy_cascade_free, replacing the
per-byte bytearray loop while preserving cleanup in the finally block. Handle
JSON decoding failures when code is non-zero by retaining the raw payload as
error detail and raising the existing native cascade ValueError instead of
allowing JSONDecodeError to escape.
Comment on lines
+70
to
+77
| if v.startswith("#"): | ||
| digits = v[1:] | ||
| if len(digits) == 3: | ||
| digits = "".join(c * 2 for c in digits) | ||
| if len(digits) == 6: | ||
| digits += "ff" | ||
| r, g, b, a = (int(digits[i : i + 2], 16) for i in (0, 2, 4, 6)) | ||
| return r, g, b, round(a / 255.0, 4) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle 4-digit and reject other hex lengths explicitly.
A #rgba value keeps digits at length 4. The code then skips both expansion branches and evaluates int(digits[4:6], 16) on an empty string, which raises ValueError. The caller catches it and records a disagreement, so the smoke reports a false failure instead of comparing the colors.
🐛 Proposed fix
if v.startswith("#"):
digits = v[1:]
- if len(digits) == 3:
+ if len(digits) in (3, 4):
digits = "".join(c * 2 for c in digits)
if len(digits) == 6:
digits += "ff"
+ if len(digits) != 8:
+ raise AssertionError(f"unparseable color {value!r}")
r, g, b, a = (int(digits[i : i + 2], 16) for i in (0, 2, 4, 6))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if v.startswith("#"): | |
| digits = v[1:] | |
| if len(digits) == 3: | |
| digits = "".join(c * 2 for c in digits) | |
| if len(digits) == 6: | |
| digits += "ff" | |
| r, g, b, a = (int(digits[i : i + 2], 16) for i in (0, 2, 4, 6)) | |
| return r, g, b, round(a / 255.0, 4) | |
| if v.startswith("#"): | |
| digits = v[1:] | |
| if len(digits) in (3, 4): | |
| digits = "".join(c * 2 for c in digits) | |
| if len(digits) == 6: | |
| digits += "ff" | |
| if len(digits) != 8: | |
| raise AssertionError(f"unparseable color {value!r}") | |
| r, g, b, a = (int(digits[i : i + 2], 16) for i in (0, 2, 4, 6)) | |
| return r, g, b, round(a / 255.0, 4) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/cascade_differential_smoke.py` around lines 70 - 77, The hex-color
parsing logic in the `#` branch must support four-digit RGBA values and
explicitly reject unsupported lengths. Update the parsing near the existing
three- and six-digit expansion branches to expand four digits appropriately
before extracting channels, and validate that only supported lengths proceed;
raise the parser’s expected invalid-value error for all other lengths.
…utes emphasis
Phase 4 prerequisites, both live-bug fixes found by the parity recon and
probe-verified before a line changed.
PDF text attributes: _pdf's closed subset rejected font-style, font-family,
letter-spacing, and opacity, so to_image("pdf") RAISED today for an italic
title or a letter-spaced label — contradicting the SLOT_TEXT_PROPS contract
that PDF honors the vector subset via the same markup. The whitelist now
speaks all four: font-style selects the oblique base-14 faces (all four
Helvetica faces registered on demand), letter-spacing becomes Tc with the
anchor width counting the len-1 inter-glyph gaps Tc actually renders and an
explicit 0 Tc reset because text state persists past ET, opacity multiplies
into the composed ExtGState alpha beside fill-opacity and the paint's own
alpha, and a declared font-family maps deterministically onto the Helvetica
family — metrically exact for anchoring, recorded in the module contract
note rather than silent, until the font registry lands. The closed-subset
doctrine is unchanged: everything the converter does not speak still fails
loudly (tested).
Raster emphasis routing: SLOT_RASTER_PROPS claims font-weight/font-style and
the glyph atlas carries bold and italic faces, but only title and axis_title
routed through _native_font_emphasis. legend_title, legend_label,
tick_label, colorbar_tick, and colorbar_title now pass emphasis into their
text ops, slot-driven exactly like the SVG side so the two writers read the
same source. Per-slot pixel-change verified for all five; unstyled output
stays byte-identical (the standing parity gate) and the survival suite is
untouched. The capability note that still described a paint-and-size-only
raster glyph primitive now tells the truth the atlas has supported since the
faces were baked.
Contributor
There was a problem hiding this comment.
13 issues found across 16 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="spec/api/export.md">
<violation number="1" location="spec/api/export.md:313">
P3: The added paragraph contradicts itself: it says custom_css can be exported natively via style_source="native_cascade" (which to_svg supports), then immediately states SVG rejects it for *every* engine. The `_resolve_image_engine` table row directly above still reads 'raises | raises' with no native_cascade caveat. Scope the tail sentence so it doesn't contradict the opt-in route described in the same paragraph.</violation>
</file>
<file name="scripts/cascade_differential_smoke.py">
<violation number="1" location="scripts/cascade_differential_smoke.py:131">
P2: Browser-side capture exceptions are not reported clearly here; a JS failure in `CAPTURE` can turn into a Python key/shape error when reading `reply["result"]["value"]`. Adding an `exceptionDetails` check before JSON parsing would keep smoke failures actionable and consistent with the other style-capture smoke script.</violation>
</file>
<file name="cascade/src/resolve.rs">
<violation number="1" location="cascade/src/resolve.rs:85">
P1: Repeated declarations in one CSS rule keep the first value, so later declarations in the same block do not override as CSS source order requires. This happens because `better()` uses a strict `>` comparison on a tuple that is equal for both declarations.</violation>
<violation number="2" location="cascade/src/resolve.rs:259">
P2: Selectors and declarations inside a non-matching (but in-profile) `@media (prefers-color-scheme: …)` are still validated and pushed to the `unsupported` list, because `profile_selector`/`declaration_texts` run before the `!media_active` guard. A class with a `:hover` or an at-rule that only lives inside the *other* color-scheme branch is therefore reported as unsupported in the current scheme even though it can never apply — which can trigger a spurious strict-mode refusal on bytes that never affect the render. Consider skipping the profile-validation/unsupported collection for style rules when `media_active` is false (the `other` arm and the selector parsing too), and only reporting truly-applicable constructs.</violation>
<violation number="3" location="cascade/src/resolve.rs:475">
P2: Non-recursive values with many `var()` usages can be rejected as excessive nesting. The loop currently caps substitutions at 8 total replacements, not 8 nesting levels, because it only processes the first `var()` per iteration.</violation>
</file>
<file name="cascade/src/lib.rs">
<violation number="1" location="cascade/src/lib.rs:50">
P2: Invalid FFI arguments can crash the process instead of returning the documented JSON error reply. `xy_cascade_resolve` dereferences raw pointers without guards, so adding explicit pointer validation (null allowed only when len==0, plus non-null `out`/`out_len`) would make the ABI boundary robust like the rest of the native entrypoints.</violation>
</file>
<file name="python/xy/styling/cascade.py">
<violation number="1" location="python/xy/styling/cascade.py:226">
P2: Native-cascade export can silently ignore or reinterpret invalid `figure.class_names` entries instead of failing like the regular DOM-spec path. This comes from string-coercing keys/values and skipping slot/type validation here; validating and preserving only real `str -> str` entries keeps behavior consistent and avoids hidden style drops.</violation>
</file>
<file name="python/xy/export.py">
<violation number="1" location="python/xy/export.py:861">
P2: `style_source="native_cascade"` now rejects `engine="native"`, even though the export engine resolver still accepts that deprecated alias. The new precheck tuple excludes `"native"`, so callers can get a ValueError on this path before normal engine resolution; allowing the native alias here would keep behavior consistent.</violation>
<violation number="2" location="python/xy/export.py:861">
P3: The allowed-engine set here is not a subset of what the resolver actually accepts. In `to_png` the downstream `_png_engine(engine)` maps any `Engine` instance that is not `Engine.default` to `"browser"` and rejects the strings `"native"`/`"chromium"`/`"browser"`, so the newly allowed `Engine.auto`, `"auto"`, and `None` all fail downstream: `Engine.auto` resolves to `"browser"` and then trips the confusing "style_snapshot feeds the native writers... drop one of the two" error even though the caller never set a snapshot, while `None`/`"auto"` raise the generic "PNG engine must be Engine.default or Engine.chromium" error. The check advertises support it cannot honor. Restrict the allowed set to what `_png_engine` actually resolves (`Engine.default`/`"default"`, and in `to_image` the auto/default/None values `_resolve_image_engine` handles).</violation>
<violation number="3" location="python/xy/export.py:865">
P2: Under `style_source="native_cascade"`, strict/warn no longer cover the figure's declared per-slot styles (`chrome_styles`). The branch builds an internal snapshot from `_cascade_snapshot`, which only inspects the cascade's own `unsupported` list and returns a non-None snapshot, so the downstream `if snapshot is None: _enforce_compatibility(...)` guard is skipped. That means a chart using `styles={...}` on a state-gated slot (which the native writer drops) is exported silently even in `compatibility="strict"`, because strict fires only when the cascade itself has unsupported constructs. The PR's contract is "strict mode causes a failure before any files are emitted" for any drop; this path bypasses it for declared `chrome_styles`. Consider running the declared-styling preflight on the figure (independent of the cascade result) so warn/strict still catch these drops.</violation>
<violation number="4" location="python/xy/export.py:1035">
P3: The strict-mode error report hard-codes `fmt="png"` regardless of the actual export. `_cascade_snapshot` is reached from `to_image` for every format (svg/pdf/webp/jpeg) and from `to_png`, so a strict failure on an SVG or PDF export attaches a report whose `target` is "png", mislabeling the format the user requested. Pass the real format through (e.g. add a `fmt` parameter to `_cascade_snapshot` and forward it to `route_resolved`).</violation>
</file>
<file name="Cargo.toml">
<violation number="1" location="Cargo.toml:7">
P2: Building this workspace now compiles the optional `cascade` crate by default (`default-members = [".", "cascade"]`), and `cascade` depends on `lightningcss`/`parcel_selectors`, which are neither vendored nor mirrored here. Per the repo's documented sandbox constraint (crates.io unreachable, deps must be vendored or the core can't build/test), a plain `cargo build` / `cargo test` will now fail offline even for the core that never links the extension. Consider scoping default-members to just the root package (`.`) and building `xy-cascade` explicitly (or vendoring its deps), with the lockfile staying resolvable for the offline build.</violation>
</file>
<file name="hatch_build.py">
<violation number="1" location="hatch_build.py:189">
P2: The cascade artifact is detected by directly constructing `native_src.parent / cascade_name` from the host/target-derived filename, but the core is located through a resilient scan (`_find_cross_compiled_lib`) precisely because cross-built cdylibs can land under suffixes the host guess never anticipates. The cascade has no such fallback, so for a cross-compile target whose actual artifact suffix differs (e.g. a `.wasm`/`.dll` while the host guesses `.so`), or for the `XY_SKIP_CARGO=1` prebuilt-core path where `native_src` is the `_native_lib` copy instead of `target/<triple>/release/`, the wheel is built without the cascade even when cargo produced it — the core is renamed/shipped but the cascade is silently dropped (only a stderr line). Since `cascade.py` loads the extension from `_native_lib` beside the core, matching the core's discovery approach (scan `native_src.parent` for a `xy_cascade.*` cdylib, and handle the `XY_SKIP_CARGO`/`dest` case by probing the build's `target/` dirs) would keep the two artifacts shipped consistently.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
|
|
||
| fn better(a: &Candidate, b: &Candidate) -> bool { | ||
| (a.important, a.specificity, a.order) > (b.important, b.specificity, b.order) |
Contributor
There was a problem hiding this comment.
P1: Repeated declarations in one CSS rule keep the first value, so later declarations in the same block do not override as CSS source order requires. This happens because better() uses a strict > comparison on a tuple that is equal for both declarations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cascade/src/resolve.rs, line 85:
<comment>Repeated declarations in one CSS rule keep the first value, so later declarations in the same block do not override as CSS source order requires. This happens because `better()` uses a strict `>` comparison on a tuple that is equal for both declarations.</comment>
<file context>
@@ -0,0 +1,729 @@
+}
+
+fn better(a: &Candidate, b: &Candidate) -> bool {
+ (a.important, a.specificity, a.order) > (b.important, b.specificity, b.order)
+}
+
</file context>
Suggested change
| (a.important, a.specificity, a.order) > (b.important, b.specificity, b.order) | |
| (a.important, a.specificity, a.order) >= (b.important, b.specificity, b.order) |
| session_id=sid, | ||
| timeout_s=remaining(), | ||
| ) | ||
| result = json.loads(reply["result"]["value"]) |
Contributor
There was a problem hiding this comment.
P2: Browser-side capture exceptions are not reported clearly here; a JS failure in CAPTURE can turn into a Python key/shape error when reading reply["result"]["value"]. Adding an exceptionDetails check before JSON parsing would keep smoke failures actionable and consistent with the other style-capture smoke script.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/cascade_differential_smoke.py, line 131:
<comment>Browser-side capture exceptions are not reported clearly here; a JS failure in `CAPTURE` can turn into a Python key/shape error when reading `reply["result"]["value"]`. Adding an `exceptionDetails` check before JSON parsing would keep smoke failures actionable and consistent with the other style-capture smoke script.</comment>
<file context>
@@ -0,0 +1,176 @@
+ session_id=sid,
+ timeout_s=remaining(),
+ )
+ result = json.loads(reply["result"]["value"])
+ if result.get("error"):
+ raise SystemExit(f"browser capture failed: {result['error']}")
</file context>
| /// honored, to a fixpoint with a depth cap. | ||
| fn substituted(value: &str, custom: &BTreeMap<String, String>) -> Result<String, String> { | ||
| let mut text = value.to_string(); | ||
| for _ in 0..8 { |
Contributor
There was a problem hiding this comment.
P2: Non-recursive values with many var() usages can be rejected as excessive nesting. The loop currently caps substitutions at 8 total replacements, not 8 nesting levels, because it only processes the first var() per iteration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cascade/src/resolve.rs, line 475:
<comment>Non-recursive values with many `var()` usages can be rejected as excessive nesting. The loop currently caps substitutions at 8 total replacements, not 8 nesting levels, because it only processes the first `var()` per iteration.</comment>
<file context>
@@ -0,0 +1,729 @@
+/// honored, to a fixpoint with a depth cap.
+fn substituted(value: &str, custom: &BTreeMap<String, String>) -> Result<String, String> {
+ let mut text = value.to_string();
+ for _ in 0..8 {
+ let Some(at) = text.find("var(") else {
+ return Ok(text);
</file context>
| out: *mut *mut u8, | ||
| out_len: *mut usize, | ||
| ) -> i32 { | ||
| let css = std::slice::from_raw_parts(css_ptr, css_len); |
Contributor
There was a problem hiding this comment.
P2: Invalid FFI arguments can crash the process instead of returning the documented JSON error reply. xy_cascade_resolve dereferences raw pointers without guards, so adding explicit pointer validation (null allowed only when len==0, plus non-null out/out_len) would make the ABI boundary robust like the rest of the native entrypoints.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cascade/src/lib.rs, line 50:
<comment>Invalid FFI arguments can crash the process instead of returning the documented JSON error reply. `xy_cascade_resolve` dereferences raw pointers without guards, so adding explicit pointer validation (null allowed only when len==0, plus non-null `out`/`out_len`) would make the ABI boundary robust like the rest of the native entrypoints.</comment>
<file context>
@@ -0,0 +1,78 @@
+ out: *mut *mut u8,
+ out_len: *mut usize,
+) -> i32 {
+ let css = std::slice::from_raw_parts(css_ptr, css_len);
+ let doc = std::slice::from_raw_parts(doc_ptr, doc_len);
+ let result = catch_unwind(AssertUnwindSafe(|| resolve::resolve_json(css, doc)));
</file context>
| order: earlier sheets are wider (the Tailwind-core manifest, a project | ||
| bundle), `custom_css` is the narrowest author sheet and comes last. | ||
| """ | ||
| class_names = {str(k): str(v) for k, v in (figure.class_names or {}).items()} |
Contributor
There was a problem hiding this comment.
P2: Native-cascade export can silently ignore or reinterpret invalid figure.class_names entries instead of failing like the regular DOM-spec path. This comes from string-coercing keys/values and skipping slot/type validation here; validating and preserving only real str -> str entries keeps behavior consistent and avoids hidden style drops.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/styling/cascade.py, line 226:
<comment>Native-cascade export can silently ignore or reinterpret invalid `figure.class_names` entries instead of failing like the regular DOM-spec path. This comes from string-coercing keys/values and skipping slot/type validation here; validating and preserving only real `str -> str` entries keeps behavior consistent and avoids hidden style drops.</comment>
<file context>
@@ -0,0 +1,287 @@
+ order: earlier sheets are wider (the Tailwind-core manifest, a project
+ bundle), `custom_css` is the narrowest author sheet and comes last.
+ """
+ class_names = {str(k): str(v) for k, v in (figure.class_names or {}).items()}
+ root_class = ""
+ dom_class = getattr(figure, "class_name", None)
</file context>
| # exports — only style_source="native_cascade" raises, with the | ||
| # build instruction (§28: optional is announced, not silent). | ||
| cascade_name = lib_name.replace("xy_core", "xy_cascade") | ||
| cascade_src = native_src.parent / cascade_name |
Contributor
There was a problem hiding this comment.
P2: The cascade artifact is detected by directly constructing native_src.parent / cascade_name from the host/target-derived filename, but the core is located through a resilient scan (_find_cross_compiled_lib) precisely because cross-built cdylibs can land under suffixes the host guess never anticipates. The cascade has no such fallback, so for a cross-compile target whose actual artifact suffix differs (e.g. a .wasm/.dll while the host guesses .so), or for the XY_SKIP_CARGO=1 prebuilt-core path where native_src is the _native_lib copy instead of target/<triple>/release/, the wheel is built without the cascade even when cargo produced it — the core is renamed/shipped but the cascade is silently dropped (only a stderr line). Since cascade.py loads the extension from _native_lib beside the core, matching the core's discovery approach (scan native_src.parent for a xy_cascade.* cdylib, and handle the XY_SKIP_CARGO/dest case by probing the build's target/ dirs) would keep the two artifacts shipped consistently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At hatch_build.py, line 189:
<comment>The cascade artifact is detected by directly constructing `native_src.parent / cascade_name` from the host/target-derived filename, but the core is located through a resilient scan (`_find_cross_compiled_lib`) precisely because cross-built cdylibs can land under suffixes the host guess never anticipates. The cascade has no such fallback, so for a cross-compile target whose actual artifact suffix differs (e.g. a `.wasm`/`.dll` while the host guesses `.so`), or for the `XY_SKIP_CARGO=1` prebuilt-core path where `native_src` is the `_native_lib` copy instead of `target/<triple>/release/`, the wheel is built without the cascade even when cargo produced it — the core is renamed/shipped but the cascade is silently dropped (only a stderr line). Since `cascade.py` loads the extension from `_native_lib` beside the core, matching the core's discovery approach (scan `native_src.parent` for a `xy_cascade.*` cdylib, and handle the `XY_SKIP_CARGO`/`dest` case by probing the build's `target/` dirs) would keep the two artifacts shipped consistently.</comment>
<file context>
@@ -180,6 +180,22 @@ def initialize(self, version: str, build_data: dict[str, Any]) -> None:
+ # exports — only style_source="native_cascade" raises, with the
+ # build instruction (§28: optional is announced, not silent).
+ cascade_name = lib_name.replace("xy_core", "xy_cascade")
+ cascade_src = native_src.parent / cascade_name
+ if cascade_src.is_file():
+ build_data["force_include"][str(cascade_src)] = f"xy/_native_lib/{cascade_name}"
</file context>
| raise ValueError( | ||
| "style_source='native_cascade' is a native path; drop engine=Engine.chromium" | ||
| ) | ||
| style_snapshot = _cascade_snapshot(fig, custom_css, compatibility) |
Contributor
There was a problem hiding this comment.
P2: Under style_source="native_cascade", strict/warn no longer cover the figure's declared per-slot styles (chrome_styles). The branch builds an internal snapshot from _cascade_snapshot, which only inspects the cascade's own unsupported list and returns a non-None snapshot, so the downstream if snapshot is None: _enforce_compatibility(...) guard is skipped. That means a chart using styles={...} on a state-gated slot (which the native writer drops) is exported silently even in compatibility="strict", because strict fires only when the cascade itself has unsupported constructs. The PR's contract is "strict mode causes a failure before any files are emitted" for any drop; this path bypasses it for declared chrome_styles. Consider running the declared-styling preflight on the figure (independent of the cascade result) so warn/strict still catch these drops.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/export.py, line 865:
<comment>Under `style_source="native_cascade"`, strict/warn no longer cover the figure's declared per-slot styles (`chrome_styles`). The branch builds an internal snapshot from `_cascade_snapshot`, which only inspects the cascade's own `unsupported` list and returns a non-None snapshot, so the downstream `if snapshot is None: _enforce_compatibility(...)` guard is skipped. That means a chart using `styles={...}` on a state-gated slot (which the native writer drops) is exported silently even in `compatibility="strict"`, because strict fires only when the cascade itself has unsupported constructs. The PR's contract is "strict mode causes a failure before any files are emitted" for any drop; this path bypasses it for declared `chrome_styles`. Consider running the declared-styling preflight on the figure (independent of the cascade result) so warn/strict still catch these drops.</comment>
<file context>
@@ -847,6 +848,22 @@ def to_png(
+ raise ValueError(
+ "style_source='native_cascade' is a native path; drop engine=Engine.chromium"
+ )
+ style_snapshot = _cascade_snapshot(fig, custom_css, compatibility)
+ custom_css = None # consumed by the cascade, not by a browser
resolved_engine = _png_engine(engine)
</file context>
| `em`/`rem`, `prefers-color-scheme`) and reports every out-of-profile construct | ||
| through the compatibility modes instead of guessing. The browser remains the | ||
| oracle: `scripts/cascade_differential_smoke.py` asserts the cascade and a live | ||
| Chromium agree on the shared profile. The message says so. SVG rejects it for *every* engine, because a browser |
Contributor
There was a problem hiding this comment.
P3: The added paragraph contradicts itself: it says custom_css can be exported natively via style_source="native_cascade" (which to_svg supports), then immediately states SVG rejects it for every engine. The _resolve_image_engine table row directly above still reads 'raises | raises' with no native_cascade caveat. Scope the tail sentence so it doesn't contradict the opt-in route described in the same paragraph.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/api/export.md, line 313:
<comment>The added paragraph contradicts itself: it says custom_css can be exported natively via style_source="native_cascade" (which to_svg supports), then immediately states SVG rejects it for *every* engine. The `_resolve_image_engine` table row directly above still reads 'raises | raises' with no native_cascade caveat. Scope the tail sentence so it doesn't contradict the opt-in route described in the same paragraph.</comment>
<file context>
@@ -304,7 +304,13 @@ pins the partition (24 static, 24 state-gated today) and
+`em`/`rem`, `prefers-color-scheme`) and reports every out-of-profile construct
+through the compatibility modes instead of guessing. The browser remains the
+oracle: `scripts/cascade_differential_smoke.py` asserts the cascade and a live
+Chromium agree on the shared profile. The message says so. SVG rejects it for *every* engine, because a browser
screenshot cannot produce vector output — that row is a hard "never", not a
default.
</file context>
Suggested change
| Chromium agree on the shared profile. The message says so. SVG rejects it for *every* engine, because a browser | |
| Chromium agree on the shared profile. The message says so. Without `style_source="native_cascade"`, SVG rejects it for every engine, because a browser |
|
|
||
| raise StyleCompatibilityError( | ||
| f"native cascade could not honor: {summary}", | ||
| route_resolved(fig, fmt="png", resolved_engine="native", custom_css=None), |
Contributor
There was a problem hiding this comment.
P3: The strict-mode error report hard-codes fmt="png" regardless of the actual export. _cascade_snapshot is reached from to_image for every format (svg/pdf/webp/jpeg) and from to_png, so a strict failure on an SVG or PDF export attaches a report whose target is "png", mislabeling the format the user requested. Pass the real format through (e.g. add a fmt parameter to _cascade_snapshot and forward it to route_resolved).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/export.py, line 1035:
<comment>The strict-mode error report hard-codes `fmt="png"` regardless of the actual export. `_cascade_snapshot` is reached from `to_image` for every format (svg/pdf/webp/jpeg) and from `to_png`, so a strict failure on an SVG or PDF export attaches a report whose `target` is "png", mislabeling the format the user requested. Pass the real format through (e.g. add a `fmt` parameter to `_cascade_snapshot` and forward it to `route_resolved`).</comment>
<file context>
@@ -989,6 +1006,41 @@ def _snapshot_styles(fig: "Figure", snapshot: Any) -> "Iterator[None]":
+
+ raise StyleCompatibilityError(
+ f"native cascade could not honor: {summary}",
+ route_resolved(fig, fmt="png", resolved_engine="native", custom_css=None),
+ )
+ _warnings.warn(
</file context>
| "style_snapshot and style_source='native_cascade' are two sources " | ||
| "for the same values; pass one" | ||
| ) | ||
| if engine not in (Engine.auto, "auto", None, Engine.default, "default"): |
Contributor
There was a problem hiding this comment.
P3: The allowed-engine set here is not a subset of what the resolver actually accepts. In to_png the downstream _png_engine(engine) maps any Engine instance that is not Engine.default to "browser" and rejects the strings "native"/"chromium"/"browser", so the newly allowed Engine.auto, "auto", and None all fail downstream: Engine.auto resolves to "browser" and then trips the confusing "style_snapshot feeds the native writers... drop one of the two" error even though the caller never set a snapshot, while None/"auto" raise the generic "PNG engine must be Engine.default or Engine.chromium" error. The check advertises support it cannot honor. Restrict the allowed set to what _png_engine actually resolves (Engine.default/"default", and in to_image the auto/default/None values _resolve_image_engine handles).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/export.py, line 861:
<comment>The allowed-engine set here is not a subset of what the resolver actually accepts. In `to_png` the downstream `_png_engine(engine)` maps any `Engine` instance that is not `Engine.default` to `"browser"` and rejects the strings `"native"`/`"chromium"`/`"browser"`, so the newly allowed `Engine.auto`, `"auto"`, and `None` all fail downstream: `Engine.auto` resolves to `"browser"` and then trips the confusing "style_snapshot feeds the native writers... drop one of the two" error even though the caller never set a snapshot, while `None`/`"auto"` raise the generic "PNG engine must be Engine.default or Engine.chromium" error. The check advertises support it cannot honor. Restrict the allowed set to what `_png_engine` actually resolves (`Engine.default`/`"default"`, and in `to_image` the auto/default/None values `_resolve_image_engine` handles).</comment>
<file context>
@@ -847,6 +848,22 @@ def to_png(
+ "style_snapshot and style_source='native_cascade' are two sources "
+ "for the same values; pass one"
+ )
+ if engine not in (Engine.auto, "auto", None, Engine.default, "default"):
+ raise ValueError(
+ "style_source='native_cascade' is a native path; drop engine=Engine.chromium"
</file context>
python/xy/_chromebox.py is the single lowering from a resolved slot declaration to a ChromeBox — background, border (style lowered to the dash construction both writers already use for data strokes), symmetric radius clamped like CSS (rx only; PDF rejects ry), the offset-rect shadow this codebase has always drawn, and opacity. Every request the model cannot draw lands in an `unrepresentable` ledger with its reason — gradients until the effect phase, blur/spread with no blur primitive, asymmetric radii until a path lowering, multiple/inset shadows — so nothing rounds to silence (§28). The shadow parser tokenizes paren-aware, because rgba()'s own commas are not shadow separators. _svg._slot_box_svg and _raster._emit_slot_box are the writer halves: same decisions, two encodings. The SVG emitter guards the duplicate-attribute XML trap (every attribute exactly once per rect, tested by parsing, not substring counting), always writes an explicit fill so a rect inside the labels group cannot inherit the text paint, and stays inside the PDF closed subset (round-tripped in the tests). The raster emitter folds slot opacity into every RGBA (the display list has no group compositing) and strokes borders closed so the final edge is not silently dropped. Both emit nothing for a box that paints nothing — the unstyled-bytes gate, enforced at the primitive itself. The three pre-existing duplicated box drawers (legend frame, annotation text box, background composition) fold onto this pair family by family in the parity phases, per spec/process/static-chrome-parity-plan-2026-08-04.md — the recon plan now committed beside the work it orders.
…endored tailwind_profile="core-v1" prepends one generated stylesheet to the native cascade, so chart.to_png(style_source="native_cascade", tailwind_profile="core-v1") resolves bg-/text-/border- across the full default palette plus the spacing, type, weight, radius, border-width and tracking scales — with no Tailwind engine anywhere. The palette is vendored from tailwindcss@3.4.17 (scripts/tailwind_palette_v3.json carries the provenance) because a hand-typed hex that is slightly wrong is the worst silent failure this program exists to end; scale values are the published rem/em/px spellings, resolved through the cascade's own font-size chain so tracking-wide on a text-sm label is 0.35px of ITS 14px, not of a guess. The manifest is generated (scripts/gen_tailwind_core.py, --check pinned in tests) and emitted as ruff-stable double-quoted literals — a generated file the formatter rewrites is permanently stale to its own drift gate. The cascade normalizes the background-color longhand onto the schema's `background` exactly as the browser capture does, and the resolver's px formatter now rounds the binary-float tail (0.025em × 14px is 0.35px). Project Tailwind builds ride stylesheets=; profile inputs without the cascade source are refused; unknown profiles name what this build ships. Everything outside the manifest stays a reported unknown, never a guess.
…s, and the legacy bbox stroke pair becomes model data The shared primitives get two corrections the annotation fold depends on: _emit_slot_box passed w/h straight through as the far corner that _rect_pts/_round_rect_pts expect, so every raster chrome box was drawn against absolute coordinates its SVG twin never used; and ChromeBox learns the pre-parity pyplot text-bbox serialization (an inert stroke pair on borderless rects, byte-pinned by the corpus) as an explicit field instead of forcing the adapter to bypass the shared emitter.
Contributor
There was a problem hiding this comment.
13 issues found across 21 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/gen_tailwind_core.py">
<violation number="1" location="scripts/gen_tailwind_core.py:118">
P3: The `colors = {"white": "#fff", "black": "#000"}` initialization in `render_css()` duplicates two entries that already exist in the vendored `scripts/tailwind_palette_v3.json` (which carries `black` and `white` as top-level string colors). The palette loop immediately overwrites both keys with identical values, so this line is dead redundancy. Since the whole point of the script is to source colors only from the vendored provenance file, the manual dict can be dropped to keep a single source of truth; if it's kept as a defensive fallback, that intent is undocumented.</violation>
<violation number="2" location="scripts/gen_tailwind_core.py:125">
P2: Conflicting color classes can resolve differently from Tailwind because lexical sorting puts `*-100` before `*-50`, changing later-rule-wins precedence in this manifest. Preserving insertion order (or numeric shade sorting) would keep native-cascade conflict behavior aligned with upstream utilities.</violation>
<violation number="3" location="scripts/gen_tailwind_core.py:129">
P2: `text-transparent` and `border-transparent` currently fall outside the core manifest even though other color utilities are emitted in bg/text/border triplets. Adding transparent variants for text and border would avoid unexpected unsupported reports for common Tailwind color usage.</violation>
</file>
<file name="python/xy/_chromebox.py">
<violation number="1" location="python/xy/_chromebox.py:136">
P2: `_parse_shadow` only handles shadows where the color token comes after the offsets. A valid CSS spelling with the color first (`box-shadow: red 2px 3px`, which the box-shadow grammar permits) makes the `if px is not None and not color` guard fail on the offset tokens once `color` is already set, so the offsets get folded into the color string and the shadow is wrongly reported as unrepresentable with the reason "shadow needs dx and dy". The shadow is actually representable in the offset-rect model, so this is a silent loss the preflight/tests would misreport. Recommend collecting lengths independently of whether a color was already seen (e.g. append numeric tokens to lengths whenever the color is not yet set, and only consider the color established after the first non-numeric token), so color-before-or-after offsets both parse.</violation>
<violation number="2" location="python/xy/_chromebox.py:165">
P2: Unsupported background functions can slip through when authored with mixed casing, which risks missing compatibility warnings and feeding invalid paint text to writers. Lowercasing before the `gradient(`/`url(` check makes this rejection path reliable.</violation>
<violation number="3" location="python/xy/_chromebox.py:192">
P2: Asymmetric `border-radius` values using slash syntax can be dropped without being reported, so preflight can miss a real style loss. The asymmetry guard only checks for spaces; including `/` in that check keeps the unrepresentable ledger accurate.</violation>
</file>
<file name="python/xy/_figure.py">
<violation number="1" location="python/xy/_figure.py:2250">
P2: `Figure.to_svg` now accepts `stylesheets`/`tailwind_profile`, but with `style_source="declared"` those inputs are ignored instead of rejected. That can hide caller misconfiguration and produce an unstyled SVG without any signal; consider mirroring `export.to_png`/`export.to_image` by raising when these args are provided outside `native_cascade`.</violation>
</file>
<file name="docs/styling/capabilities.md">
<violation number="1" location="docs/styling/capabilities.md:118">
P3: The capability note overstates raster font support internals: PNG text uses one baked glyph atlas with synthetic bold/italic transforms, not separate regular/bold/italic faces. Updating this wording will keep the compatibility contract accurate for users evaluating typography fidelity.</violation>
</file>
<file name="tests/test_pdf_text_subset.py">
<violation number="1" location="tests/test_pdf_text_subset.py:66">
P3: The set/reset count works only because the emitter joins each op with a newline, so every "0 Tc" reset is "\n0 Tc". That couples the test to exact PDF byte layout; a formatting change in the emitter quietly unbalances the assertion. Consider counting non-zero sets directly, e.g. re.findall(rb"(?:[1-9][0-9.]*|0\.[0-9]+) Tc", content) == resets, which stays correct regardless of separator whitespace.</violation>
</file>
<file name="python/xy/export.py">
<violation number="1" location="python/xy/export.py:1042">
P2: HTML export can now accept `stylesheets`/`tailwind_profile` but silently drops them, which makes it look like those styling inputs were applied when they were not. It would be safer to reject these options in the `fmt == "html"` branch the same way other non-applicable export options are rejected.</violation>
</file>
<file name="python/xy/styling/cascade.py">
<violation number="1" location="python/xy/styling/cascade.py:194">
P2: When both `background` and `background-color` are present, this mapping can produce the wrong final color because it always lets `background-color` override `background` based on sorted key order, not CSS cascade order. A safer local behavior is to only map `background-color` when no `background` entry already exists.</violation>
</file>
<file name="tests/test_tailwind_manifest.py">
<violation number="1" location="tests/test_tailwind_manifest.py:77">
P3: This test is named and commented around "reported, not guessed," but it only asserts that `backdrop-blur-md` did not produce a `backdrop-filter` declaration. The `_resolve(...)` return is unpacked as `snapshot, _`, discarding the `unsupported` report that is supposed to carry the unmatched class name, so the reporting half of the claim is never actually exercised. Consider asserting that the class (or its reason) is surfaced through `unsupported`/the report surface.</violation>
</file>
<file name="python/xy/_pdf.py">
<violation number="1" location="python/xy/_pdf.py:1107">
P2: The new PDF letter-spacing support only parses bare numbers or an `Npx` literal, so any other valid CSS value that the SVG writer emits verbatim (e.g. `letter-spacing: normal` or a relative unit like `0.5em`) — which the browser/SVG render fine — will crash the PDF export with `_unsupported('letter-spacing ...')`. That's inconsistent with the "PDF honors letter-spacing" claim added in this PR's docs and capability matrix. If only px is in scope, the docs should say so, or the parser should at least map the documented subset (e.g. treat `normal` as 0) rather than aborting a valid export the same SVG markup happily renders.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| rules.append(f".bg-{_class(name)}{{background-color:{value}}}") | ||
| rules.append(f".text-{_class(name)}{{color:{value}}}") | ||
| rules.append(f".border-{_class(name)}{{border-color:{value}}}") | ||
| rules.append(".bg-transparent{background-color:transparent}") |
Contributor
There was a problem hiding this comment.
P2: text-transparent and border-transparent currently fall outside the core manifest even though other color utilities are emitted in bg/text/border triplets. Adding transparent variants for text and border would avoid unexpected unsupported reports for common Tailwind color usage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/gen_tailwind_core.py, line 129:
<comment>`text-transparent` and `border-transparent` currently fall outside the core manifest even though other color utilities are emitted in bg/text/border triplets. Adding transparent variants for text and border would avoid unexpected unsupported reports for common Tailwind color usage.</comment>
<file context>
@@ -0,0 +1,203 @@
+ rules.append(f".bg-{_class(name)}{{background-color:{value}}}")
+ rules.append(f".text-{_class(name)}{{color:{value}}}")
+ rules.append(f".border-{_class(name)}{{border-color:{value}}}")
+ rules.append(".bg-transparent{background-color:transparent}")
+
+ for key in SPACING:
</file context>
| continue | ||
| for shade, value in shades.items(): | ||
| colors[f"{hue}-{shade}"] = value | ||
| for name, value in sorted(colors.items()): |
Contributor
There was a problem hiding this comment.
P2: Conflicting color classes can resolve differently from Tailwind because lexical sorting puts *-100 before *-50, changing later-rule-wins precedence in this manifest. Preserving insertion order (or numeric shade sorting) would keep native-cascade conflict behavior aligned with upstream utilities.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/gen_tailwind_core.py, line 125:
<comment>Conflicting color classes can resolve differently from Tailwind because lexical sorting puts `*-100` before `*-50`, changing later-rule-wins precedence in this manifest. Preserving insertion order (or numeric shade sorting) would keep native-cascade conflict behavior aligned with upstream utilities.</comment>
<file context>
@@ -0,0 +1,203 @@
+ continue
+ for shade, value in shades.items():
+ colors[f"{hue}-{shade}"] = value
+ for name, value in sorted(colors.items()):
+ rules.append(f".bg-{_class(name)}{{background-color:{value}}}")
+ rules.append(f".text-{_class(name)}{{color:{value}}}")
</file context>
| if px is not None and not color: | ||
| lengths.append(px) | ||
| else: | ||
| color = f"{color} {part}".strip() |
Contributor
There was a problem hiding this comment.
P2: _parse_shadow only handles shadows where the color token comes after the offsets. A valid CSS spelling with the color first (box-shadow: red 2px 3px, which the box-shadow grammar permits) makes the if px is not None and not color guard fail on the offset tokens once color is already set, so the offsets get folded into the color string and the shadow is wrongly reported as unrepresentable with the reason "shadow needs dx and dy". The shadow is actually representable in the offset-rect model, so this is a silent loss the preflight/tests would misreport. Recommend collecting lengths independently of whether a color was already seen (e.g. append numeric tokens to lengths whenever the color is not yet set, and only consider the color established after the first non-numeric token), so color-before-or-after offsets both parse.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/_chromebox.py, line 136:
<comment>`_parse_shadow` only handles shadows where the color token comes after the offsets. A valid CSS spelling with the color first (`box-shadow: red 2px 3px`, which the box-shadow grammar permits) makes the `if px is not None and not color` guard fail on the offset tokens once `color` is already set, so the offsets get folded into the color string and the shadow is wrongly reported as unrepresentable with the reason "shadow needs dx and dy". The shadow is actually representable in the offset-rect model, so this is a silent loss the preflight/tests would misreport. Recommend collecting lengths independently of whether a color was already seen (e.g. append numeric tokens to lengths whenever the color is not yet set, and only consider the color established after the first non-numeric token), so color-before-or-after offsets both parse.</comment>
<file context>
@@ -0,0 +1,222 @@
+ if px is not None and not color:
+ lengths.append(px)
+ else:
+ color = f"{color} {part}".strip()
+ if len(lengths) < 2:
+ return None, "shadow needs dx and dy"
</file context>
| compatibility: str = "legacy", | ||
| style_snapshot: Optional[Any] = None, | ||
| style_source: str = "declared", | ||
| stylesheets: tuple[str, ...] = (), |
Contributor
There was a problem hiding this comment.
P2: Figure.to_svg now accepts stylesheets/tailwind_profile, but with style_source="declared" those inputs are ignored instead of rejected. That can hide caller misconfiguration and produce an unstyled SVG without any signal; consider mirroring export.to_png/export.to_image by raising when these args are provided outside native_cascade.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/_figure.py, line 2250:
<comment>`Figure.to_svg` now accepts `stylesheets`/`tailwind_profile`, but with `style_source="declared"` those inputs are ignored instead of rejected. That can hide caller misconfiguration and produce an unstyled SVG without any signal; consider mirroring `export.to_png`/`export.to_image` by raising when these args are provided outside `native_cascade`.</comment>
<file context>
@@ -2247,6 +2247,8 @@ def to_svg(
compatibility: str = "legacy",
style_snapshot: Optional[Any] = None,
style_source: str = "declared",
+ stylesheets: tuple[str, ...] = (),
+ tailwind_profile: Optional[str] = None,
) -> str:
</file context>
|
|
||
| - **`root`** (via `chart style=`) — `styles={'root': ...}` is browser-only, but the chart-level `style=` token bag targets the same element and every renderer reads it (`spec['dom']['style']`). Prefer it for anything that must survive export. | ||
| - **`title`** (via `styles={'title': ...}`) — Vector (SVG, PDF) honors font-size, font-weight, font-style, font-family, letter-spacing, opacity and the text paint (`fill`, or `color`). The raster writer's glyph primitive takes a size and one RGBA paint and nothing else, so it honors font-size and the paint only — font-weight, font-style, font-family, letter-spacing and opacity are vector-only rather than silently approximated. Properties outside the subset stay browser-only. | ||
| - **`title`** (via `styles={'title': ...}`) — Vector (SVG, PDF) honors font-size, font-weight, font-style, font-family, letter-spacing, opacity and the text paint (`fill`, or `color`); PDF maps any declared family onto the base-14 Helvetica faces (regular/bold/oblique/bold-oblique), recorded in `_pdf.py`'s contract note. The raster atlas carries regular, bold and italic faces, so font-size, the paint, font-weight and font-style survive there too — font-family, letter-spacing and opacity remain vector-only rather than silently approximated. Properties outside the subset stay browser-only. |
Contributor
There was a problem hiding this comment.
P3: The capability note overstates raster font support internals: PNG text uses one baked glyph atlas with synthetic bold/italic transforms, not separate regular/bold/italic faces. Updating this wording will keep the compatibility contract accurate for users evaluating typography fidelity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/styling/capabilities.md, line 118:
<comment>The capability note overstates raster font support internals: PNG text uses one baked glyph atlas with synthetic bold/italic transforms, not separate regular/bold/italic faces. Updating this wording will keep the compatibility contract accurate for users evaluating typography fidelity.</comment>
<file context>
@@ -115,15 +115,15 @@ would not survive, before any bytes exist.
- **`root`** (via `chart style=`) — `styles={'root': ...}` is browser-only, but the chart-level `style=` token bag targets the same element and every renderer reads it (`spec['dom']['style']`). Prefer it for anything that must survive export.
-- **`title`** (via `styles={'title': ...}`) — Vector (SVG, PDF) honors font-size, font-weight, font-style, font-family, letter-spacing, opacity and the text paint (`fill`, or `color`). The raster writer's glyph primitive takes a size and one RGBA paint and nothing else, so it honors font-size and the paint only — font-weight, font-style, font-family, letter-spacing and opacity are vector-only rather than silently approximated. Properties outside the subset stay browser-only.
+- **`title`** (via `styles={'title': ...}`) — Vector (SVG, PDF) honors font-size, font-weight, font-style, font-family, letter-spacing, opacity and the text paint (`fill`, or `color`); PDF maps any declared family onto the base-14 Helvetica faces (regular/bold/oblique/bold-oblique), recorded in `_pdf.py`'s contract note. The raster atlas carries regular, bold and italic faces, so font-size, the paint, font-weight and font-style survive there too — font-family, letter-spacing and opacity remain vector-only rather than silently approximated. Properties outside the subset stay browser-only.
- **`legend`** (via `styles={'legend': ...} / xy.legend(style=...) / --chart-legend-bg`) — The frame box. Both spellings and the theme token now converge on one merged declaration block before the writers see it, so what agrees in the browser agrees in a PNG. `background`, `boxShadow`, `borderRadius`, `--xy-legend-frame-alpha`, and `padding`/`rowGap` in `em` are honored; an explicit background paints opaque, as it does in the browser.
-- **`legend_title`** (via `styles={'legend_title': ...}`) — Vector (SVG, PDF) honors font-size, font-weight, font-style, font-family, letter-spacing, opacity and the text paint (`fill`, or `color`). The raster writer's glyph primitive takes a size and one RGBA paint and nothing else, so it honors font-size and the paint only — font-weight, font-style, font-family, letter-spacing and opacity are vector-only rather than silently approximated. Properties outside the subset stay browser-only.
</file context>
| assert b" Tc" in content | ||
| # Text state persists past ET; every spaced run must reset, so the | ||
| # count of sets equals the count of resets. | ||
| sets = len(re.findall(rb"[0-9.]+ Tc", content)) - content.count(b"\n0 Tc") |
Contributor
There was a problem hiding this comment.
P3: The set/reset count works only because the emitter joins each op with a newline, so every "0 Tc" reset is "\n0 Tc". That couples the test to exact PDF byte layout; a formatting change in the emitter quietly unbalances the assertion. Consider counting non-zero sets directly, e.g. re.findall(rb"(?:[1-9][0-9.]*|0.[0-9]+) Tc", content) == resets, which stays correct regardless of separator whitespace.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_pdf_text_subset.py, line 66:
<comment>The set/reset count works only because the emitter joins each op with a newline, so every "0 Tc" reset is "\n0 Tc". That couples the test to exact PDF byte layout; a formatting change in the emitter quietly unbalances the assertion. Consider counting non-zero sets directly, e.g. re.findall(rb"(?:[1-9][0-9.]*|0\.[0-9]+) Tc", content) == resets, which stays correct regardless of separator whitespace.</comment>
<file context>
@@ -0,0 +1,96 @@
+ assert b" Tc" in content
+ # Text state persists past ET; every spaced run must reset, so the
+ # count of sets equals the count of resets.
+ sets = len(re.findall(rb"[0-9.]+ Tc", content)) - content.count(b"\n0 Tc")
+ resets = content.count(b"\n0 Tc")
+ assert sets == resets > 0
</file context>
| def test_unknown_utilities_are_reported_not_guessed() -> None: | ||
| # `backdrop-blur` is outside the manifest: the class simply matches no | ||
| # rule, and the preflight/report boundary carries the class name. | ||
| snapshot, _ = _resolve({"tick_label": "text-sky-800 backdrop-blur-md"}) |
Contributor
There was a problem hiding this comment.
P3: This test is named and commented around "reported, not guessed," but it only asserts that backdrop-blur-md did not produce a backdrop-filter declaration. The _resolve(...) return is unpacked as snapshot, _, discarding the unsupported report that is supposed to carry the unmatched class name, so the reporting half of the claim is never actually exercised. Consider asserting that the class (or its reason) is surfaced through unsupported/the report surface.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_tailwind_manifest.py, line 77:
<comment>This test is named and commented around "reported, not guessed," but it only asserts that `backdrop-blur-md` did not produce a `backdrop-filter` declaration. The `_resolve(...)` return is unpacked as `snapshot, _`, discarding the `unsupported` report that is supposed to carry the unmatched class name, so the reporting half of the claim is never actually exercised. Consider asserting that the class (or its reason) is surfaced through `unsupported`/the report surface.</comment>
<file context>
@@ -0,0 +1,119 @@
+def test_unknown_utilities_are_reported_not_guessed() -> None:
+ # `backdrop-blur` is outside the manifest: the class simply matches no
+ # rule, and the preflight/report boundary carries the class name.
+ snapshot, _ = _resolve({"tick_label": "text-sky-800 backdrop-blur-md"})
+ assert _decls(snapshot, "tick_label")["color"] == "#075985"
+ # Unmatched classes are visible in the report surface: the class list
</file context>
|
|
||
| def render_css(palette: dict) -> str: | ||
| rules: list[str] = [] | ||
| colors: dict[str, str] = {"white": "#fff", "black": "#000"} |
Contributor
There was a problem hiding this comment.
P3: The colors = {"white": "#fff", "black": "#000"} initialization in render_css() duplicates two entries that already exist in the vendored scripts/tailwind_palette_v3.json (which carries black and white as top-level string colors). The palette loop immediately overwrites both keys with identical values, so this line is dead redundancy. Since the whole point of the script is to source colors only from the vendored provenance file, the manual dict can be dropped to keep a single source of truth; if it's kept as a defensive fallback, that intent is undocumented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/gen_tailwind_core.py, line 118:
<comment>The `colors = {"white": "#fff", "black": "#000"}` initialization in `render_css()` duplicates two entries that already exist in the vendored `scripts/tailwind_palette_v3.json` (which carries `black` and `white` as top-level string colors). The palette loop immediately overwrites both keys with identical values, so this line is dead redundancy. Since the whole point of the script is to source colors only from the vendored provenance file, the manual dict can be dropped to keep a single source of truth; if it's kept as a defensive fallback, that intent is undocumented.</comment>
<file context>
@@ -0,0 +1,203 @@
+
+def render_css(palette: dict) -> str:
+ rules: list[str] = []
+ colors: dict[str, str] = {"white": "#fff", "black": "#000"}
+ for hue, shades in palette["colors"].items():
+ if isinstance(shades, str):
</file context>
The sdist manifest is an exact allow/require pair so a distribution cannot quietly gain a build artifact or lose a source file. The cascade extension is a workspace member, so its crate belongs in the same category as src/: allowed at top level, and REQUIRED — an sdist that dropped it would build a wheel whose style_source="native_cascade" raises at a user's first class-styled export instead of failing here.
…ugh the folded chrome box
styles={'annotation_label': ...} used to leave zero trace in a file. The
slot channel now threads into _annotation_svg and _emit_annotations, and
one merge point (annotation_style_with_slot) defines the two-vocabulary
translation exactly once: the slot's kebab CSS folds UNDER the
annotation's own pyplot-derived style= per property group, matching the
browser's _applySlot-then-inline order. The duplicated box drawers
(_svg_text_box / _emit_text_box) become adapters over one shared lowering
(annotation_text_box -> ChromeBox -> the two P0.3 emitters), which fixes
the 4-value CSS padding misread, lowers dashed/dotted border styles to
dash patterns, and adds offset box-shadow, whole-label opacity and
em-in-the-label's-own-unit-domain resolution — while reproducing the
pre-fold output byte-for-byte for every current input (pyplot corpus
plus an 11-case SVG/raster/PDF capture diff). Registry, preflight
(SLOT_BOX_PROPS routing), capability matrix and the export/styling specs
move in the same commit; badge/badge_item stay view-gated with no writer
emission, pinned by test.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/xy/export.py (1)
1018-1023: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the requested format to
_cascade_snapshot().
route_resolved()always receivesfmt="png". A strict native-cascade failure from PDF or SVG export therefore returns a PNG compatibility report. Addfmtto_cascade_snapshot()and pass"png",fmt, or"svg"from each caller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/export.py` around lines 1018 - 1023, Update _cascade_snapshot to accept a fmt parameter, then pass the appropriate format from every caller: "png" for PNG handling, the requested fmt for PDF handling, and "svg" for SVG handling. Ensure route_resolved() uses the caller’s requested format so native-cascade failure reports are not always labeled as PNG.
♻️ Duplicate comments (1)
python/xy/export.py (1)
882-893: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftValidate compatibility independently from snapshot presence.
A non-
Nonesnapshot skips_enforce_compatibility()on every native export path. A valid empty snapshot can therefore letcompatibility="strict"export while native writers still drop the figure's class-based styles. These paths also accept reservedcompatibility="lossless"because they never validate the mode.
python/xy/export.py#L882-L893: validate the mode before snapshot handling, and require evidence that the snapshot covers every loss source before suppressing preflight.python/xy/export.py#L1371-L1381: apply the same validation and coverage rule for every unified image format.python/xy/_figure.py#L2277-L2281: apply the same validation and coverage rule for direct SVG export.Based on PR objectives,
losslessis reserved and compatibility enforcement must occur before rendering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/xy/export.py` around lines 882 - 893, Update python/xy/export.py lines 882-893 and 1371-1381, and python/xy/_figure.py lines 2277-2281, so compatibility mode validation runs before snapshot handling and rejects reserved compatibility="lossless". Only skip _enforce_compatibility when the snapshot explicitly covers every relevant loss source; otherwise enforce compatibility before rendering, including for valid empty snapshots and all native or direct SVG export paths.
🧹 Nitpick comments (1)
scripts/verify_sdist.py (1)
94-99: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the Tailwind runtime module explicitly. The current
include = ["python/xy"]coverspython/xy/styling/_tailwind_core.py, but add it toREQUIRED_FILESto detect future sdist selection changes that could breaktailwind_profile="core-v1".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/verify_sdist.py` around lines 94 - 99, Add python/xy/styling/_tailwind_core.py explicitly to the REQUIRED_FILES collection in scripts/verify_sdist.py, alongside the existing required cascade files, so core-v1 Tailwind runtime coverage is validated independently of the include pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/xy/_chromebox.py`:
- Around line 189-197: Update the border-radius handling around _px and the
unrepresentable list to parse whitespace-separated components, preserve a
symmetric radius when all components are equal (including two- and four-value
forms), and only set radius to 0.0 and record the unsupported message when
corner values differ or the value uses the elliptical “/” form.
In `@python/xy/_figure.py`:
- Around line 2250-2251: Update the style-source validation in the figure export
path to reject any non-empty stylesheets or non-None tailwind_profile when
style_source is "declared", matching export.to_image() behavior; add the
corresponding elif validation without changing the native-cascade path.
In `@python/xy/_pdf.py`:
- Around line 1171-1176: Convert CSS-pixel letter_spacing to PDF text-space
units before passing it to the Tc text-state operator in the text-rendering flow
around _text_width_px and the existing letter-spacing emission. Use the current
font size and PDF’s 1000-unit scaling so rendered gaps match the pixel width
reserved by the anchor calculation, while preserving the existing zero-reset
behavior when letter spacing is absent.
In `@python/xy/_raster.py`:
- Around line 1374-1379: Propagate tick-label emphasis through the polar
rendering path: update _emit_polar_tick_labels to accept tick_italic and
tick_bold, and pass the values computed from tick_label styles at its call site
in the polar branch. Preserve Cartesian behavior and add a polar raster
regression test covering italic and bold tick labels.
- Around line 784-787: Update the pts helper to pass rectangle end coordinates,
using x + box.w and y + box.h, to both _round_rect_pts and _rect_pts instead of
passing box.w and box.h directly.
In `@python/xy/export.py`:
- Around line 1430-1434: Update the HTML export branch around the to_html() call
to add style_snapshot, style_source when it is not "declared", stylesheets when
non-empty, and tailwind_profile when provided to rejected whenever they differ
from their defaults; preserve custom_css handling and ensure these ignored
inputs are reported rather than discarded.
In `@spec/process/static-chrome-parity-plan-2026-08-04.md`:
- Around line 68-82: Complete the specification updates by adding native_cascade
and tailwind_profile="core-v1" to spec/design-dossier.md and the API export
contract, documenting PDF letter-spacing and mathtext behavior, and correcting
spec/api/styling.md to state that raster export honors font-weight and
font-style. Preserve all existing source-distribution requirements in
spec/process/production-readiness.md and spec/design-dossier.md.
---
Outside diff comments:
In `@python/xy/export.py`:
- Around line 1018-1023: Update _cascade_snapshot to accept a fmt parameter,
then pass the appropriate format from every caller: "png" for PNG handling, the
requested fmt for PDF handling, and "svg" for SVG handling. Ensure
route_resolved() uses the caller’s requested format so native-cascade failure
reports are not always labeled as PNG.
---
Duplicate comments:
In `@python/xy/export.py`:
- Around line 882-893: Update python/xy/export.py lines 882-893 and 1371-1381,
and python/xy/_figure.py lines 2277-2281, so compatibility mode validation runs
before snapshot handling and rejects reserved compatibility="lossless". Only
skip _enforce_compatibility when the snapshot explicitly covers every relevant
loss source; otherwise enforce compatibility before rendering, including for
valid empty snapshots and all native or direct SVG export paths.
---
Nitpick comments:
In `@scripts/verify_sdist.py`:
- Around line 94-99: Add python/xy/styling/_tailwind_core.py explicitly to the
REQUIRED_FILES collection in scripts/verify_sdist.py, alongside the existing
required cascade files, so core-v1 Tailwind runtime coverage is validated
independently of the include pattern.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70923d0c-828b-4588-b3ea-e42eb976f83e
📒 Files selected for processing (22)
CHANGELOG.mdcascade/src/resolve.rsdocs/styling/capabilities.mdpython/xy/_chromebox.pypython/xy/_figure.pypython/xy/_pdf.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/components.pypython/xy/export.pypython/xy/styling/_tailwind_core.pypython/xy/styling/capabilities.pypython/xy/styling/cascade.pyscripts/gen_tailwind_core.pyscripts/tailwind_palette_v3.jsonscripts/verify_sdist.pyspec/api/capability-matrix.mdspec/process/static-chrome-parity-plan-2026-08-04.mdtests/test_chromebox.pytests/test_components.pytests/test_pdf_text_subset.pytests/test_tailwind_manifest.py
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/test_components.py
- spec/api/capability-matrix.md
- python/xy/_svg.py
- docs/styling/capabilities.md
- python/xy/components.py
- python/xy/styling/capabilities.py
- cascade/src/resolve.rs
- CHANGELOG.md
- python/xy/styling/cascade.py
Comment on lines
+189
to
+197
| radius = _px(declaration.get("border-radius")) or 0.0 | ||
| if ( | ||
| isinstance(declaration.get("border-radius"), str) | ||
| and " " in str(declaration["border-radius"]).strip() | ||
| ): | ||
| unrepresentable.append( | ||
| "asymmetric border-radius (PDF accepts symmetric rx only; path lowering pending)" | ||
| ) | ||
| radius = 0.0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Lower uniform multi-value radii as a symmetric radius.
Line 190 treats every whitespace-separated border-radius value as asymmetric. Valid values such as "4px 4px" and "4px 4px 4px 4px" describe the same radius at every corner. The current branch removes the radius and reports a false unsupported feature.
Parse the components. Record an unsupported value only when corners differ or the value uses the elliptical / form.
Proposed fix
- radius = _px(declaration.get("border-radius")) or 0.0
- if (
- isinstance(declaration.get("border-radius"), str)
- and " " in str(declaration["border-radius"]).strip()
- ):
+ raw_radius = declaration.get("border-radius")
+ radius = _px(raw_radius) or 0.0
+ if isinstance(raw_radius, str) and " " in raw_radius.strip():
+ parts = raw_radius.strip().split()
+ values = [_px(part) for part in parts]
+ if "/" not in raw_radius and all(value is not None for value in values) and len(set(values)) == 1:
+ radius = float(values[0])
+ else:
+ unrepresentable.append(
+ "asymmetric border-radius (PDF accepts symmetric rx only; path lowering pending)"
+ )
+ radius = 0.0
- unrepresentable.append(
- "asymmetric border-radius (PDF accepts symmetric rx only; path lowering pending)"
- )
- radius = 0.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| radius = _px(declaration.get("border-radius")) or 0.0 | |
| if ( | |
| isinstance(declaration.get("border-radius"), str) | |
| and " " in str(declaration["border-radius"]).strip() | |
| ): | |
| unrepresentable.append( | |
| "asymmetric border-radius (PDF accepts symmetric rx only; path lowering pending)" | |
| ) | |
| radius = 0.0 | |
| raw_radius = declaration.get("border-radius") | |
| radius = _px(raw_radius) or 0.0 | |
| if isinstance(raw_radius, str) and " " in raw_radius.strip(): | |
| parts = raw_radius.strip().split() | |
| values = [_px(part) for part in parts] | |
| if "/" not in raw_radius and all(value is not None for value in values) and len(set(values)) == 1: | |
| radius = float(values[0]) | |
| else: | |
| unrepresentable.append( | |
| "asymmetric border-radius (PDF accepts symmetric rx only; path lowering pending)" | |
| ) | |
| radius = 0.0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/xy/_chromebox.py` around lines 189 - 197, Update the border-radius
handling around _px and the unrepresentable list to parse whitespace-separated
components, preserve a symmetric radius when all components are equal (including
two- and four-value forms), and only set radius to 0.0 and record the
unsupported message when corner values differ or the value uses the elliptical
“/” form.
Comment on lines
+1430
to
+1434
| compatibility: str = "legacy", | ||
| style_snapshot: Optional[Any] = None, | ||
| style_source: str = "declared", | ||
| stylesheets: tuple[str, ...] = (), | ||
| tailwind_profile: Optional[str] = None, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject style inputs that HTML export ignores.
For an HTML path, style_snapshot, style_source="native_cascade", stylesheets, and tailwind_profile are accepted but discarded because this branch calls to_html() with only custom_css. Add these values to rejected when they differ from their defaults.
Proposed fix
("optimize", optimize, False),
+ ("style_snapshot", style_snapshot, None),
+ ("style_source", style_source, "declared"),
+ ("stylesheets", stylesheets, ()),
+ ("tailwind_profile", tailwind_profile, None),
("compatibility", compatibility, "legacy"),Also applies to: 1455-1458
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/xy/export.py` around lines 1430 - 1434, Update the HTML export branch
around the to_html() call to add style_snapshot, style_source when it is not
"declared", stylesheets when non-empty, and tailwind_profile when provided to
rejected whenever they differ from their defaults; preserve custom_css handling
and ensure these ignored inputs are reported rather than discarded.
Comment on lines
+68
to
+82
| ## 2. Phase 0 — cross-cutting prerequisites (blocking) | ||
|
|
||
| **0.1 PDF text-subset extension (live-bug fix).** All four surveys independently probe-verified: `_pdf._ALLOWED_ATTRS['text']` (`_pdf.py:204-206`) rejects `font-style`, `font-family`, `letter-spacing`, `opacity`, so `to_image(format='pdf')` RAISES today for an italic title, a letter-spaced legend_label, an italic tick_label/colorbar_tick, and mathtext annotations (nested tspan, `_pdf.py:1099-1100`). The `SLOT_TEXT_PROPS` docstring claim that PDF honors the vector subset (`_svg.py:1331-1332`) is false; the capability-matrix tick_label PDF note is also wrong. | ||
| Edits: `_ALLOWED_ATTRS['text']` + `_render_text` (`_pdf.py:1062-1141`); Helvetica-Oblique/BoldOblique into `_font()` (`_pdf.py:681-691`, today regular/bold only, bold cutoff weight>=600 at `1065`); letter-spacing via `Tc`; text opacity multiplied into `ca` ExtGState; nested-tspan mathtext either supported or explicitly fenced. `font-family` beyond base-14 is policy-refused, not guessed. | ||
| Acceptance: `styles={'title':{'font_style':'italic'}}`, `{'legend_label':{'letter_spacing':'2px'}}`, `{'tick_label':{'font_style':'italic'}}`, `{'colorbar_tick':{'font_style':'italic'}}` each round-trip `to_svg -> svg_to_pdf` without ValueError; a mathtext annotation exports to PDF (or raises a documented, tested policy error). Closed-subset contract note (`_pdf.py:7-10, 63-64`) updated in the same change. | ||
|
|
||
| **0.2 Raster emphasis-routing gap fix (contract violation).** `SLOT_RASTER_PROPS` (`_svg.py:1350-1356`) claims font-weight/font-style are honored, but only `title` (`_raster.py:1431-1436`) and `axis_title` (`1497-1505`) route through `_native_font_emphasis` (`_raster.py:1645-1655`). Wire it into: legend_title (`_raster.py:3113-3120`), legend_label (`3174-3181`), tick_label (`1388-1397`), colorbar_tick (`3447-3455`, `3486-3494`), colorbar_title (`3456-3464`, `3503-3511`). `cmd.text` already supports italic/bold (`_raster.py:662-700`). | ||
| Acceptance per slot: `font_weight: 700` selects the bold atlas face, `font_style: italic` the italic face, in raster PNG; the pinned vector-minus-raster property set `{font-family, letter-spacing, opacity}` in `tests/test_export_style_survival.py:209-220` and `capabilities.py:267-274` updated in the same commit. | ||
|
|
||
| **0.3 Shared primitives** (§1 above): `ChromeBox` lowering + `_slot_box_svg` + `_emit_slot_box` + shorthand expansion in `resolve_declared` (`declared.py:89-99`). Resolve the legend shadow constants while extracting, don't copy them (flag A/I below). | ||
| Acceptance: unit tests on the lowering (border shorthand, per-side padding, radius clamp, shadow parse); golden SVG/PNG for one synthetic box exercising every field; PDF round-trip of the emitted rect. | ||
|
|
||
| **0.4 Registry/preflight plumbing pattern.** Every slot added to writers must, in the same commit: join `STATIC_STYLED_SLOTS` (`_svg.py:1367-1377`); update `capabilities.py` (`264-296`, `332-345`) and `tests/test_capability_registry.py`; update preflight honored-props (`preflight.py:185-203, 197, 253-262`). Add a shared `SLOT_BOX_PROPS` constant (pattern: `LEGEND_BOX_PROPS`, `_svg.py:1416-1429`) so writers/registry/preflight cannot drift. | ||
|
|
||
| **0.5 Standing gate:** unstyled output stays byte-identical (`tests/test_export_style_survival.py:140-146`). Every phase's emission is strictly conditional on a declaration being present. This gate is the acceptance floor for every edit below. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '^design-dossier\.md$' spec
rg -n -C 2 'native_cascade|tailwind_profile|ChromeBox|letter-spacing|source distribution|sdist' specRepository: reflex-dev/xy
Length of output: 42400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- design dossier matches ---'
rg -n -i -C 4 'native.?cascade|tailwind|pdf|letter-spacing|source distribution|sdist|static chrome|chrome box|export style' spec/design-dossier.md
printf '%s\n' '--- related API and process matches ---'
rg -n -i -C 3 'native.?cascade|tailwind|letter-spacing|pdf text|source distribution|sdist|static chrome|chrome box' \
spec/api spec/process spec/design
printf '%s\n' '--- dossier section map ---'
rg -n '^#{1,4} ' spec/design-dossier.md | rg -i 'export|style|css|release|distribution|tailwind|pdf|render'Repository: reflex-dev/xy
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- design dossier sections around CSS/Tailwind and export ---'
sed -n '1330,1415p' spec/design-dossier.md
sed -n '480,530p' spec/design-dossier.md
printf '%s\n' '--- exact native cascade and PDF contract references ---'
rg -n -i -C 6 'native.?cascade|style_source|tailwind.?profile|letter-spacing|PDF.*(text|font)|text.*PDF' \
spec/api/export.md spec/api/styling.md spec/api/capability-matrix.md spec/design-dossier.md
printf '%s\n' '--- implementation/API references outside spec ---'
rg -n -i -C 3 'native.?cascade|tailwind.?profile|style_source|letter-spacing|_ALLOWED_ATTRS|SLOT_RASTER_PROPS' \
python js reflex_xy pyproject.toml setup.cfg hatch.toml 2>/dev/null || trueRepository: reflex-dev/xy
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- export contract ---'
sed -n '257,340p' spec/api/export.md
printf '%s\n' '--- Tailwind capability taxonomy ---'
sed -n '730,760p' spec/api/styling.md
printf '%s\n' '--- styling export contract ---'
sed -n '1318,1340p' spec/api/styling.md
sed -n '1458,1472p' spec/api/styling.md
printf '%s\n' '--- term coverage by specification file ---'
python3 - <<'PY'
from pathlib import Path
terms = ("native_cascade", "tailwind_profile", "letter-spacing", "source distribution", "sdist")
for path in sorted(Path("spec").rglob("*.md")):
text = path.read_text(encoding="utf-8").lower()
found = [term for term in terms if term.lower() in text]
if found:
print(f"{path}: {', '.join(found)}")
PYRepository: reflex-dev/xy
Length of output: 11310
Complete the specification updates for this plan.
- Add
native_cascadeandtailwind_profile="core-v1"tospec/design-dossier.mdand the API export contract. - Define PDF
letter-spacingand mathtext behavior in the PDF contract. - Update
spec/api/styling.md; it still says raster export excludesfont-weightandfont-style, contrary to this plan. - Keep the existing source-distribution requirements in
spec/process/production-readiness.mdandspec/design-dossier.md.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/process/static-chrome-parity-plan-2026-08-04.md` around lines 68 - 82,
Complete the specification updates by adding native_cascade and
tailwind_profile="core-v1" to spec/design-dossier.md and the API export
contract, documenting PDF letter-spacing and mathtext behavior, and correcting
spec/api/styling.md to state that raster export honors font-weight and
font-style. Preserve all existing source-distribution requirements in
spec/process/production-readiness.md and spec/design-dossier.md.
Source: Coding guidelines
…ipped background The overlay slot reaches the writers: opacity wraps the annotation shape tail (clipped and unclipped alike, never the labels) in a real <g> in SVG/PDF, and folds into every shape RGBA in the raster display list, which has no group compositing — the overlapping-shapes double-blend is recorded in KNOWN_RENDERER_DIVERGENCES and pinned executable. The background is a plot-rect chrome box under the shapes, inside the marks clip: the live overlay canvas is full-bleed, but no writer seam is both above the traces and below the shapes at full bleed, so the plot-clipped geometry is the pinned decision, recorded as the second divergence rather than left to be discovered. Registry, preflight routing, matrix and the export/styling specs move in the same commit; unstyled bytes hold in both writers.
… writers The four slots whose geometry the writers already know become real chrome (spec/process/static-chrome-parity-plan-2026-08-04.md §3): every box goes through the shared _chromebox lowering and the P0.3 emitter pair, and every emission is strictly declaration-gated — tests prove an unstyled chart's SVG and PNG bytes are unchanged by the new paths. title: the anchor math both writers duplicated is hoisted into _svg.title_placement / legacy_title_placement before any box is drawn (the survey's explicit ordering — box extents added to two copies independently would drift). title_box wraps the measured TextBlock — block width plus padding, never the wrap width, because the browser box is the shrink-to-fit div — and the rect rides `chrome` (SVG) / the display list (raster) immediately before its text. _title_room grows by padding plus paintable border WITH the client mirror (_titleBoxExtent, js/src/50_chartview.ts) in the same change, or native and browser disagree on plot.y. Per-entry box styling is native-only (the client's entry-style allowlist) — recorded in KNOWN_RENDERER_DIVERGENCES, not silent. root: the box IS the figure patch — same DOM element, one background property — so its fill replaces theme(background=) instead of stacking, a declared radius rounds it, and apply_export_background is now the single precedence definition (export override > slot declaration > theme token), silencing the root and canvas slot fills alongside the token pair. The raster underlay skip requires a SQUARE fully-opaque patch: a rounded root's corners must show the underlay, never uninitialized native white; a painted root also satisfies the plot-rect white-fallback rule the way a figure background does. box-shadow would fall outside the viewBox — it may never grow the export, so it is a named preflight loss. chrome: background/opacity only (plan §8 flag G), one rect between the backgrounds and the grid in both writers; the browser's own DOM stacking of this slot against titles diverges by design and is recorded in KNOWN_RENDERER_DIVERGENCES rather than papered over. canvas: painted at the ABOVE-grid seam — deliberately not the --chart-bg anchor, the paint-order trap this family exists to avoid — so a declared background hides the grid exactly as the browser's marks canvas does. Radius is a THIRD clipPath wrapped around the marks group (never a mutation of clip_id/marks_clip_id, which polar legends depend on) and opacity rides that wrapper, both PDF-legal on <g>; the raster's rect-only clip and missing group compositing make radius/opacity named raster losses (SLOT_BOX_RASTER_UNSUPPORTED) until the rounded-clip opcode lands. Registry, preflight and matrix move in the same commit (plan 0.4): the three slots join STATIC_STYLED_SLOTS, SLOT_BOX_PROPS(_BY_SLOT) is the writer-owned box vocabulary preflight._honored_props consumes, root loses its browser-only special case, and the capability matrix regenerates. Groundwork this landing completes: the border/padding shorthand expansion promised with P0.3 (_chromebox.expand_box_shorthands, consumed by lower_box and by the declared resolver so shorthands intern as schema longhands while em legend values keep their authored residue spelling); root/chrome SlotInstance geometry from the snapshot environment (spec dims, never host padding — flag J's normalization rule; title/canvas need the layout pass the resolver cannot re-enter and land with the capture producers); and a P0.3 raster-emitter fix — _emit_slot_box fed (w, h) to _rect_pts as the far corner, so any box not anchored at the origin painted the wrong rectangle (now pinned by decoding the emitted quad). Contracts updated with the change: spec/api/export.md §9, spec/api/styling.md (including the P0.2-stale raster claim that weight/style were vector-only), the capability matrix + docs mirror, and the limitations page.
…inherited default
styles={'labels': ...} now does in a file what the container does live.
Color threads as the default under the live var(--chart-text, inherit)
chain — theme token first, container color second, writer default last —
for every text the container holds (tick labels, axis titles, annotation
labels), in both writers, while title/legend/colorbar stay siblings on
default_text. Typography folds UNDER the contained slots exactly where
the live stylesheet leaves the property un-ruled (size/weight cascade
into tick labels only; style/family/letter-spacing into all three), and
opacity rides the SVG label group. The flag-D stacking conflict resolves
to the browser's own order: the full-bleed background paints under the
axis rules and every label text in both writers, pinned by golden; the
residual sibling stacking (live, the container covers the title) is
recorded in KNOWN_RENDERER_DIVERGENCES rather than left to be
discovered. Registry, preflight, matrix and specs move in the same
commit; unstyled bytes hold.
The concat in _annotation_svg widened the declared 3-tuple return type to tuple[list[str], ...]; unpack the layer wrap explicitly so the signature stays checkable (ty back to the repo baseline).
main promoted ty from advisory to a hard CI gate (#451, the PEP 561 typing surface), which this branch predates — so it inherited 79 diagnostics that main had already fixed. Merging clears 69 of them; the remaining ten are byte-identical to main (redundant numpy casts, reflex EventHandler subscripts that resolve differently across installed versions), so this branch's type state now equals the branch it targets. Two were genuinely this program's. The snapshot environment validator converted an `object` straight to float behind a type: ignore; it now narrows first, so a JSON payload carrying a list where a width belongs raises the module's own ValueError instead of relying on a silenced call. The style_snapshot coercion casts its JSON-shaped dict at the boundary it already validates. Neither changes behavior — both replace a silenced or unstated assumption with the check it was standing in for.
…he axis_band ruling axis_line and tick_mark join STATIC_STYLED_SLOTS as pure box slots. Emission is strictly declaration-gated — an unstyled chart's bytes never move — and when a box is declared, the spine/tick strokes are replaced by ChromeBoxes from one shared producer (_svg.axis_chrome_boxes), consumed identically by the SVG writer, the raster writer, and the declared snapshot, which now records per-instance qualifiers and geometry (axis id, major|minor, side, tick index; one interned declaration, N instances). Spines stay centered on the plot edge where the unstyled stroke has always run — the browser insets right/bottom spines by their own width, recorded in KNOWN_RENDERER_DIVERGENCES and pinned by golden rather than silently matched. A box-styled spine keeps its axis_color ink unless the slot declares a background; an explicit transparent erases it (the fill_declared distinction). tick_length stays 0 by default and no length is invented for a styled slot: a zero-area box draws nothing, casts no shadow, records no instance, and the preflight says so. Polar spines/ticks keep stroke semantics in every renderer — the browser shares the limit. The near-duplicate tick_span closures unify on one module-level 3-tuple. tick_label and axis_title additionally draw boxes through the shared text_box metrics (block + padding in anchor space), emitted immediately before each <text> in both writers, cartesian and polar. The room functions learn the box model in the same change: padding/border grows the measured gutters in both orientations (declared tick boxes take the measuring path where unstyled auto ticks keep their historical flat band), the y-title baseline shifts outward past the padded tick boxes, and layout() stashes the extents on the plot record so reservation and placement cannot disagree. The rotated-box PDF lowering is pinned repo-wide per plan flag E's recommendation: pre-rotated <polygon> when radius == 0, <path> with circular arcs when radius > 0, shadow offset applied in element space before rotation — rect takes no transform in the PDF closed subset, and both replacements round-trip it (tested). Per-property completion of the axis text contract: the SVG axis-title branch that dropped the slot's letter-spacing and opacity wholesale whenever the axis authored label_font_family/style is now a per-property merge (axis label_* wins per key, the slot fills the rest; font-size keeps running the other way — slot over label_size — documented in spec/api/styling.md with a precedence-table test), and the raster polar tick-label sink gains the P0.2 emphasis routing it had missed. Letter-spacing stays outside the gutter measurement, qualified in the capability note next to the pre-existing slot-size/axis-size measurement gap rather than half-folded. axis_band is ruled interaction chrome (plan flag F, the badge precedent): the browser creates it only while its axis is navigable and a static file has no gesture for it to serve, so it joins _STATE_GATED_SLOTS under a new "navigation" export state — preflight reports state-gated, no writer draws it, and the capability matrix's stale "clean static" row is regenerated away. Repairs to the P0.3 primitives found en route: _emit_slot_box passed (x, y, w, h) to corner-coordinate helpers, collapsing every raster box off the origin (the P0.3 test only asserted non-empty bytes; a decoded-geometry regression test now pins the rect), and the plan's SLOT_BOX_PROPS constant (P0.4) did not exist yet — created in _svg.py on the LEGEND_BOX_PROPS pattern and read by the writers' gating, the capability registry, and the preflight's honored-props routing so the three cannot drift. Registry, preflight, matrix, spec (§4 status + flags E/F resolutions), docs, and CHANGELOG move in this commit; tests/test_chrome_parity_p2.py is the executable acceptance list.
# Conflicts: # docs/api-reference/limitations-and-alpha-status.md # docs/styling/capabilities.md # python/xy/_chromebox.py # python/xy/_svg.py # python/xy/styling/capabilities.py # python/xy/styling/preflight.py # spec/api/capability-matrix.md # spec/api/export.md # spec/api/styling.md # tests/test_export_style_survival.py
# Conflicts: # CHANGELOG.md # docs/api-reference/limitations-and-alpha-status.md # docs/styling/capabilities.md # python/xy/_chromebox.py # python/xy/_raster.py # python/xy/_svg.py # python/xy/styling/capabilities.py # python/xy/styling/declared.py # python/xy/styling/preflight.py # spec/api/capability-matrix.md # spec/api/export.md # spec/api/styling.md # tests/test_chromebox.py # tests/test_export_style_survival.py
Two parity tests decoded the exported PNG with PIL, which is absent from the 3.11-floor CI environment — so they failed there while passing locally, and the repo's own convention for optional Pillow is pytest.importorskip (test_webp, test_jpeg, test_image_export). Neither applies here: `_raster.to_rgba` is the raster writer's pre-encode surface, so asserting against it checks exactly what the PNG encoder receives, needs no image library at all, and cannot silently skip on a machine that happens to lack the extra.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_chrome_parity_p1.py">
<violation number="1" location="tests/test_chrome_parity_p1.py:47">
P3: These two tests now assert against `_raster.to_rgba` — the internal pre-encode RGBA surface — instead of decoding the bytes produced by the real export path (`export.to_image(fig, "png", ...)` -> `_raster.to_png` -> `_png.encode`). The rendering surface is shared, so the transparent-background override is still exercised through `_export_payload`/`apply_export_background`, but the tests no longer cover the encode step or the public entry point. In particular `test_root_radius_corners_show_the_underlay_not_native_white` is explicitly guarding against "uninitialized native white" — a property of the encoded PNG output — so an encoder/routing regression (e.g. `to_png` reverting to the opaque `fast_png` path, or `_png.encode` mishandling alpha) would no longer be caught by these tests. Consider keeping one decode of the actual `to_image`/`to_png` output to lock in the byte-level contract while the new Pillow-free helper covers the surface.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| a dev-only extra is a test that only runs where someone happens to have | ||
| it. | ||
| """ | ||
| return _raster.to_rgba(fig, background="transparent", scale=1.0, **kw) |
Contributor
There was a problem hiding this comment.
P3: These two tests now assert against _raster.to_rgba — the internal pre-encode RGBA surface — instead of decoding the bytes produced by the real export path (export.to_image(fig, "png", ...) -> _raster.to_png -> _png.encode). The rendering surface is shared, so the transparent-background override is still exercised through _export_payload/apply_export_background, but the tests no longer cover the encode step or the public entry point. In particular test_root_radius_corners_show_the_underlay_not_native_white is explicitly guarding against "uninitialized native white" — a property of the encoded PNG output — so an encoder/routing regression (e.g. to_png reverting to the opaque fast_png path, or _png.encode mishandling alpha) would no longer be caught by these tests. Consider keeping one decode of the actual to_image/to_png output to lock in the byte-level contract while the new Pillow-free helper covers the surface.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_chrome_parity_p1.py, line 47:
<comment>These two tests now assert against `_raster.to_rgba` — the internal pre-encode RGBA surface — instead of decoding the bytes produced by the real export path (`export.to_image(fig, "png", ...)` -> `_raster.to_png` -> `_png.encode`). The rendering surface is shared, so the transparent-background override is still exercised through `_export_payload`/`apply_export_background`, but the tests no longer cover the encode step or the public entry point. In particular `test_root_radius_corners_show_the_underlay_not_native_white` is explicitly guarding against "uninitialized native white" — a property of the encoded PNG output — so an encoder/routing regression (e.g. `to_png` reverting to the opaque `fast_png` path, or `_png.encode` mishandling alpha) would no longer be caught by these tests. Consider keeping one decode of the actual `to_image`/`to_png` output to lock in the byte-level contract while the new Pillow-free helper covers the surface.</comment>
<file context>
@@ -35,10 +34,17 @@ def _raster_pixels(chart: xy.Chart) -> np.ndarray:
+ a dev-only extra is a test that only runs where someone happens to have
+ it.
+ """
+ return _raster.to_rgba(fig, background="transparent", scale=1.0, **kw)
</file context>
The legend frame was drawn twice, and the two copies had drifted: the SVG shadow was an unconditional rx="4" at fill-opacity 0.22 while the raster drew the frame's own radius at 55/255, so a square frame cast a rounded shadow in one writer and a square one in the other. Both copies also pinned the frame radius to 4 for ANY truthy border-radius, honored `borderColor` but silently dropped the `border-color` an author writing CSS would reach for, and ran their geometry in em multipliers only — which is why every legend geometry declaration rode DeclaredStyling.writer_domain instead of the snapshot: there was no px spelling for it to be resolved into. Fold the frame onto the shared chrome-box lowering (plan §6 item 3) and give legend geometry a resolved-px vocabulary beside its em one. Both spellings of every box property now reach the writers, `padding` takes the 1-4 value CSS shorthand and its longhands, and `row-gap`/`gap`/`font-size` take px — so a px declaration interns like any other resolved length and only a genuinely relative value stays writer-domain, for the same reason every other slot's does. The pinned residue enumeration drops to exactly one entry, and `legend` leaves preflight._CONDITIONAL_CHANNEL_SLOTS: it was listed because a "lossless" declaration could still lose a half-honored spelling, which is no longer true. Geometry is the family's real risk, because _legend_layout feeds four consumers — both writers, pyplot's anchored-legend room reservation, and its loc="best" scoring — and a change that moved the drawn frame but not the reserved room would put legends outside their own space. It stays one function; the per-entry geometry it now returns is read by both writers instead of being recomputed from `pad` in each. legend_title/legend_label feed their authored font-size and letter-spacing into that measurement, so an oversized slot font grows the frame rather than escaping it, and legend_item/legend_swatch join as new slots (on a patch entry the swatch IS the patch, so a declared paint or radius wins over the trace's, matching the browser's slot-after-paint precedence). Flags resolved rather than copied: one shadow constant and one shape (A/H), and the frame's alpha keeps dimming its border (B) — the live frame is a single translucent element, so ChromeBox grew border_opacity to carry it. A blurred box-shadow keeps the historical offset-rect and records the blur; honoring it literally would have silently deleted the shadow pyplot authors for legend(shadow=True). Two defects found en route, both outside the letter of the plan and both silent. _svg.SLOT_BOX_PROPS was bound twice, and the binding that _has_box_declaration actually resolved omitted the `border` shorthand, so a slot declaring only `border: 2px solid red` drew no box at all. And `dominant-baseline` and the text stroke pair were outside the PDF closed subset, so any chart carrying a glyph or mathtext marker raised instead of exporting; both are lowered now (baseline shift, text render mode 2). Folding the frame onto the one emitter re-serializes it: the one-off rgba() literal becomes the repo's rgb() plus fill-opacity, and an opacity of 1 is spelled by omission. Raster output is pixel-identical across the unstyled corpus, so the four tests that pinned the old spelling now assert the paint instead; the delta is recorded in the plan rather than left for a reader to find.
…nd it The last parity family has the least shared machinery: bar geometry is duplicated verbatim between the writers, container geometry exists nowhere in Python, and the two writers carry placement divergences nobody had enumerated. This design names the shared record, picks the surviving truth per divergence with browser parity as the tiebreak, and orders the edits. The three adversarial reviews ship WITH it because they are corrections, not commentary: the design as first written breaks six existing tests (two structurally), imports a container height that lands twelve pixels off-canvas in the default horizontal configuration by pairing the browser's box with the writers' measured gap, and gets paint order wrong in five places — one of which breaks PDF. An implementer reading only the design would ship all three.
Contributor
There was a problem hiding this comment.
4 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="spec/process/colorbar-parity-design-2026-08-05.md">
<violation number="1" location="spec/process/colorbar-parity-design-2026-08-05.md:152">
P1: The §1.4 container box is copied from the browser while §3 deliberately keeps the writers' divergent gap (bottom_axis_room or 10 vs browser 24). The doc's own Refutation 2 computes these two decisions combine to paint the horizontal container 12px off-canvas in the default untitled configuration — the plainest spelling. The design body is not reconciled with this BLOCKING finding (step 6's acceptance only guards a vertical titled chart), so the "implementation-ready" contract would ship an out-of-canvas container. Size the container against the writers' reservation rather than the browser's.</violation>
<violation number="2" location="spec/process/colorbar-parity-design-2026-08-05.md:291">
P2: The blast-radius containment claim 'only hand-built specs move' is factually wrong: pyplot writes over_color/under_color only when color_table is an ndarray, so ordinary contourf(extend=...) moves real shim output in both writers. It also hides that 64-band centre resampling alone moves the continuous raster extension default, so splitting changes (c) and (g) reopens a fresh divergence. Correct the containment note and keep the changes bundled or re-measure the blast radius.</violation>
<violation number="3" location="spec/process/colorbar-parity-design-2026-08-05.md:313">
P2: The design body's Step 0 (and §2 case 10) direct the implementer to "delete the second `SLOT_BOX_PROPS`" at `_svg.py:1521-1546` and call it a blocking shadowing bug. At the current checkout there is no second definition — `python/xy/_svg.py` has a single `SLOT_BOX_PROPS` (line 1492) and `SLOT_BOX_PROPS_BY_SLOT` derives from it, so this de-shadowing is already done and Step 0 is a no-op. Your own Refutation D confirms the same. Since the document is being added as the actionable design contract for P5, keeping a "Blocking" step that points at deleted code will mislead anyone acting on the edit list (§6). Please mark Step 0 / case 10 as already-completed (or drop it) so the ordered edit list reflects the merged tree.</violation>
<violation number="4" location="spec/process/colorbar-parity-design-2026-08-05.md:344">
P2: The §10 doc-fix target cites export.md as "17 slots → 21", but the current tree already documents 19 styled slots (including the legend family: `legend_item`, `legend_swatch`, `colorbar_title`, `colorbar_tick`). After this design adds the four colorbar family slots, the correct correction is 19 → 23, not 17 → 21. Your own Refutation D agrees. As written, the edit list would under-report both the baseline and the target slots, so please update the count in Step 10.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| |---|---|---|---| | ||
| | vertical, non-axes | bar x/y | `w + 48` (= 66) | `max(24, h)` | | ||
| | vertical, `placement="axes"` | bar x/y | `w + 44` | `max(24, h)` | | ||
| | horizontal, non-axes | bar x/y | `w` | `h + 32` (= 50) | |
Contributor
There was a problem hiding this comment.
P1: The §1.4 container box is copied from the browser while §3 deliberately keeps the writers' divergent gap (bottom_axis_room or 10 vs browser 24). The doc's own Refutation 2 computes these two decisions combine to paint the horizontal container 12px off-canvas in the default untitled configuration — the plainest spelling. The design body is not reconciled with this BLOCKING finding (step 6's acceptance only guards a vertical titled chart), so the "implementation-ready" contract would ship an out-of-canvas container. Size the container against the writers' reservation rather than the browser's.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/process/colorbar-parity-design-2026-08-05.md, line 152:
<comment>The §1.4 container box is copied from the browser while §3 deliberately keeps the writers' divergent gap (bottom_axis_room or 10 vs browser 24). The doc's own Refutation 2 computes these two decisions combine to paint the horizontal container 12px off-canvas in the default untitled configuration — the plainest spelling. The design body is not reconciled with this BLOCKING finding (step 6's acceptance only guards a vertical titled chart), so the "implementation-ready" contract would ship an out-of-canvas container. Size the container against the writers' reservation rather than the browser's.</comment>
<file context>
@@ -0,0 +1,717 @@
+|---|---|---|---|
+| vertical, non-axes | bar x/y | `w + 48` (= 66) | `max(24, h)` |
+| vertical, `placement="axes"` | bar x/y | `w + 44` | `max(24, h)` |
+| horizontal, non-axes | bar x/y | `w` | `h + 32` (= 50) |
+| horizontal, `placement="axes"` | bar x/y | `w` | `h + 24` |
+
</file context>
| | Hoist bar math into `colorbar_layout` | **none** — 24 of 24 lines byte-identical; only the `domain` hoist and `gradient_attrs` differ, neither geometric | Step 2 must be provably byte-identical: golden-diff SVG **and** PNG bytes across an orientation × placement × extend × levels × line_only matrix before and after. | | ||
| | Tick baseline → +12 | **raster PNG only**, ticks up 1 px | Own commit. P4/P6 unaffected (P6 checks canvas borders; 1 px up increases headroom). | | ||
| | Title baseline → +26 | **SVG only**, title down 4 px | Own commit. P1 survives (node *order* preserved). `tests/test_png_export.py:647-670` asserts only `label_y > plot.y + plot.h` — safe. | | ||
| | Extension default fill | SVG + PNG, discrete + `extend` only | Blast radius small: the pyplot contour path always writes explicit `over_color`/`under_color` (`pyplot/_mplfig.py:1340-1347`), so only hand-built specs move. | |
Contributor
There was a problem hiding this comment.
P2: The blast-radius containment claim 'only hand-built specs move' is factually wrong: pyplot writes over_color/under_color only when color_table is an ndarray, so ordinary contourf(extend=...) moves real shim output in both writers. It also hides that 64-band centre resampling alone moves the continuous raster extension default, so splitting changes (c) and (g) reopens a fresh divergence. Correct the containment note and keep the changes bundled or re-measure the blast radius.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/process/colorbar-parity-design-2026-08-05.md, line 291:
<comment>The blast-radius containment claim 'only hand-built specs move' is factually wrong: pyplot writes over_color/under_color only when color_table is an ndarray, so ordinary contourf(extend=...) moves real shim output in both writers. It also hides that 64-band centre resampling alone moves the continuous raster extension default, so splitting changes (c) and (g) reopens a fresh divergence. Correct the containment note and keep the changes bundled or re-measure the blast radius.</comment>
<file context>
@@ -0,0 +1,717 @@
+| Hoist bar math into `colorbar_layout` | **none** — 24 of 24 lines byte-identical; only the `domain` hoist and `gradient_attrs` differ, neither geometric | Step 2 must be provably byte-identical: golden-diff SVG **and** PNG bytes across an orientation × placement × extend × levels × line_only matrix before and after. |
+| Tick baseline → +12 | **raster PNG only**, ticks up 1 px | Own commit. P4/P6 unaffected (P6 checks canvas borders; 1 px up increases headroom). |
+| Title baseline → +26 | **SVG only**, title down 4 px | Own commit. P1 survives (node *order* preserved). `tests/test_png_export.py:647-670` asserts only `label_y > plot.y + plot.h` — safe. |
+| Extension default fill | SVG + PNG, discrete + `extend` only | Blast radius small: the pyplot contour path always writes explicit `over_color`/`under_color` (`pyplot/_mplfig.py:1340-1347`), so only hand-built specs move. |
+| `_css` on raster line color | PNG, `currentColor`/`var()` lines only | Effectively zero: `lines` is unreachable from the composition API (`xy.colorbar()` accepts only `show/render/title/orientation/ticks/class_name/style`) and the only producer sets explicit colors. |
+| SVG line/extension order swap | SVG **node order**, not coordinates | Safe against P1 (text-node regex) and against every ElementTree-based pin. |
</file context>
| **Step 9 — `colorbar_tick` pills and `colorbar_title` box.** `text_box` consumption; per-tick `SlotInstance` qualifiers + geometry into `python/xy/styling/declared.py`. | ||
| *Accept:* rotated title box geometry byte-identical across SVG / PDF / raster from the single pose (extend the P5 pin to the box); tick pill widths recorded as metrics-divergent (§28, the DejaVu-vs-authored-family misfit already noted in `text_box`'s docstring); tick-overflow note surfaced by preflight. | ||
|
|
||
| **Step 10 — Registry, matrix, docs.** Add `colorbar_bar`, `colorbar_extension`, `colorbar_line`, `colorbar_minor_tick` to `STATIC_STYLED_SLOTS` (`_svg.py:1383-1401`); add their `_SLOT_PAINT_PROPERTY` entries (`tests/test_export_style_survival.py:60-76`) — all four take `background` except `colorbar_line`/`colorbar_minor_tick`, which take `border-color`; extend `styling/preflight.py:_honored_props` (`:191-250`) with the three stroke-only slots; add the ten `RendererDivergence` entries from §3 at `capabilities.py:257`; rerun `scripts/gen_capability_matrix.py`. |
Contributor
There was a problem hiding this comment.
P2: The §10 doc-fix target cites export.md as "17 slots → 21", but the current tree already documents 19 styled slots (including the legend family: legend_item, legend_swatch, colorbar_title, colorbar_tick). After this design adds the four colorbar family slots, the correct correction is 19 → 23, not 17 → 21. Your own Refutation D agrees. As written, the edit list would under-report both the baseline and the target slots, so please update the count in Step 10.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/process/colorbar-parity-design-2026-08-05.md, line 344:
<comment>The §10 doc-fix target cites export.md as "17 slots → 21", but the current tree already documents 19 styled slots (including the legend family: `legend_item`, `legend_swatch`, `colorbar_title`, `colorbar_tick`). After this design adds the four colorbar family slots, the correct correction is 19 → 23, not 17 → 21. Your own Refutation D agrees. As written, the edit list would under-report both the baseline and the target slots, so please update the count in Step 10.</comment>
<file context>
@@ -0,0 +1,717 @@
+**Step 9 — `colorbar_tick` pills and `colorbar_title` box.** `text_box` consumption; per-tick `SlotInstance` qualifiers + geometry into `python/xy/styling/declared.py`.
+*Accept:* rotated title box geometry byte-identical across SVG / PDF / raster from the single pose (extend the P5 pin to the box); tick pill widths recorded as metrics-divergent (§28, the DejaVu-vs-authored-family misfit already noted in `text_box`'s docstring); tick-overflow note surfaced by preflight.
+
+**Step 10 — Registry, matrix, docs.** Add `colorbar_bar`, `colorbar_extension`, `colorbar_line`, `colorbar_minor_tick` to `STATIC_STYLED_SLOTS` (`_svg.py:1383-1401`); add their `_SLOT_PAINT_PROPERTY` entries (`tests/test_export_style_survival.py:60-76`) — all four take `background` except `colorbar_line`/`colorbar_minor_tick`, which take `border-color`; extend `styling/preflight.py:_honored_props` (`:191-250`) with the three stroke-only slots; add the ten `RendererDivergence` entries from §3 at `capabilities.py:257`; rerun `scripts/gen_capability_matrix.py`.
+*Accept:* `tests/test_export_style_survival.py:79-97` green for all four new slots; `tests/test_capability_registry.py:161` and `:91` green; hand-written counts corrected — `spec/api/export.md:251` ("17 slots" → 21), the enumerations at `spec/api/export.md:326` and `spec/api/styling.md:1327`, the false minor-tick claim at `spec/api/styling.md:596-600`, and `spec/api/export.md:255`, which wrongly says `xy.colorbar(style=...)` is dropped by both writers — verified false: it lands in `spec['dom']['styles']['colorbar']`, which is exactly the map `slot_styles` returns.
+
</file context>
|
|
||
| --- | ||
|
|
||
| **Step 0 — De-shadow `SLOT_BOX_PROPS`.** |
Contributor
There was a problem hiding this comment.
P2: The design body's Step 0 (and §2 case 10) direct the implementer to "delete the second SLOT_BOX_PROPS" at _svg.py:1521-1546 and call it a blocking shadowing bug. At the current checkout there is no second definition — python/xy/_svg.py has a single SLOT_BOX_PROPS (line 1492) and SLOT_BOX_PROPS_BY_SLOT derives from it, so this de-shadowing is already done and Step 0 is a no-op. Your own Refutation D confirms the same. Since the document is being added as the actionable design contract for P5, keeping a "Blocking" step that points at deleted code will mislead anyone acting on the edit list (§6). Please mark Step 0 / case 10 as already-completed (or drop it) so the ordered edit list reflects the merged tree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/process/colorbar-parity-design-2026-08-05.md, line 313:
<comment>The design body's Step 0 (and §2 case 10) direct the implementer to "delete the second `SLOT_BOX_PROPS`" at `_svg.py:1521-1546` and call it a blocking shadowing bug. At the current checkout there is no second definition — `python/xy/_svg.py` has a single `SLOT_BOX_PROPS` (line 1492) and `SLOT_BOX_PROPS_BY_SLOT` derives from it, so this de-shadowing is already done and Step 0 is a no-op. Your own Refutation D confirms the same. Since the document is being added as the actionable design contract for P5, keeping a "Blocking" step that points at deleted code will mislead anyone acting on the edit list (§6). Please mark Step 0 / case 10 as already-completed (or drop it) so the ordered edit list reflects the merged tree.</comment>
<file context>
@@ -0,0 +1,717 @@
+
+---
+
+**Step 0 — De-shadow `SLOT_BOX_PROPS`.**
+Delete the second assignment and its comment block, `python/xy/_svg.py:1521-1546`; fold its docstring into the first at `:1455-1481`.
+*Accept:* `'border' in _svg.SLOT_BOX_PROPS`; `_has_box_declaration({'border':'1px solid red'})` is True; `slot_box_declaration({'border':'1px solid red'}, 'colorbar_bar') == {'border': '1px solid red'}`. Unstyled SVG+PNG byte-identical. Regenerate `spec/api/capability-matrix.md` via `scripts/gen_capability_matrix.py` (`tests/test_capability_registry.py:161` forces same-commit regeneration).
</file context>
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.
Consolidates the three stacked changes (#452, #453, #454) into one PR at the request of review flow — same commits, review follow-ups included. Phase 0 of the styling-compatibility program plus the schema every later phase consumes.
1. Report-only preflight and the applicable-slot matrix
chart.style_compatibility_report(target=..., engine=..., custom_css=...)routes every declared slot style into exactly one ofsurvives,native-subset(naming kept/lost properties per format family),browser-only, orstate-gated— and mirrors the export path's refusals by calling its own resolver and validators, so it cannot disagree with what running the export would do. Constant-time early-out when the chart carries no class/per-slot declarations.hover/modebar/crosshair/selection/view(24). A styled tooltip is no longer "dropped" by a PNG that never contains a tooltip; it's recorded with its gating state (§28). Registry tests pin the partition and reject native-support claims on state-gated slots.2. Staged
compatibility=modes and the engine contractEvery chart-, figure-, and module-level image export accepts
compatibility=(facet grids deliberately excluded until their per-panel preflight):"legacy"(default, byte-identical, one string comparison of cost),"warn"(oneStyleCompatibilityWarningnaming each loss),"strict"(StyleCompatibilityErrorbefore emission, report attached)."lossless"is reserved-and-rejected until preflight routing exists. No mode ever re-routes an explicit engine; resolution errors precede and outrank mode logic in every mode; strict batches fail whole before any file is written.spec/process/style-compatibility-migration.mdnames the default flips: warn 0.0.7 → strict 0.1.0 → legacy removed 0.2.0.3. The ResolvedStyleSnapshot (schema v1)
The renderer-neutral IR: interned declarations + identity-carrying instances, concrete values only (no
var()/calc(), no relative units — enforced at both construction ends of the eventual wire), closed per-version vocabulary, canonical byte ordering so identical styling serializes identically. Generated TypeScript mirror (js/src/14_style_snapshot.ts) with a--checkdrift gate. Nothing rides the wire yet:PROTOCOL_VERSIONstays 12; wire-protocol §8 documents the shape and reserves the message names.Review follow-ups already included
From the bot passes on the stacked PRs — fixed: browser-path
custom_cssvalidation mirrored (_custom_css_block); malformed figure styling raises like the export instead of being skipped; summary counts derived from the registry; explicit writer-family classification; typed report returns;to_pngresolution-error precedence for styled charts (test now feeds the lossy case); measured warning stacklevel so warnings land on the caller's line; SVG-aware strict remediation (no recommending an engine SVG rejects); batch vocabulary validated up front with HTML entries exempt; changelog facet wording; relative units rejected anywhere in a value (translate(50%, 20%),2em 1em, gradient stops); cascade keywords exact-match only ("Inheritance Sans" is concrete); tokens share the full string contract;snapshot_from_payloadenforcesbuild()'s validation; canonical declaration/instance ordering with a byte-equality test. Skipped with reason: freezing the report'ssourcesdict (transient value object, never hashed — reviewer tagged it low-value); deriving the family-prefix test from_STATE_GATED_SLOTS(would make the check assert the mapping against itself; independent restatement is the point).Evidence
Suites at each stage: 3973 → 3987 → 4012 passed plus 244 across the program's own suites after the review fixes; engine × mode matrix runs against live Chromium;
node js/build.mjstypechecks the generated mirror; ruff/format/pre-commit clean;xy.exportimport weight verified unchanged (lazy__getattr__— the eager alias would have pulled the native dylib into an import chain that never had it). Dense-axis snapshot fixture: 460 instances, 2 declarations, ~38.6 KB against the 50 KB budget.Next per the plan: the ten native-capable slots routed through the IR behind
_compile_cached, gated byte/pixel-equivalent and within-noise.Summary by CodeRabbit
New Features
legacy,warn, andstrictmodes for exports.Documentation