Data-bound chart components 1/7: design record and ground pins - #461
Data-bound chart components 1/7: design record and ground pins#461FarhanAliRaza wants to merge 1 commit into
Conversation
The design space and decision for the reflex_xy component API revision (Option 6: data-bound chart components), its file-level work plan, and the Phase 0 tests everything above this rests on. The pins are the point: test_framework_contracts.py fixes the Reflex behavior the design depends on (parametrized-generic var types survive as _var_type, typed props reject wrong vars at create(), unknown non-event kwargs are silently absorbed into style), and test_validation_timing.py fixes xy's own validation timing (zero-row construction compiles, mark config validates at .figure(), chrome validates eagerly, Chart.figure() memoizes). Each test names the design fact it pins, so a Reflex or grammar upgrade that moves the ground fails here first rather than in the tier built on top of it. No runtime change.
📝 WalkthroughWalkthroughThis PR adds two design documents for the Reflex data-bound chart component API and adds tests that pin Reflex type-checking, component kwarg validation, zero-row chart validation, and figure rebinding behavior. ChangesReflex data-bound chart API
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds the Option 6 design record and Phase 0 contract tests for Reflex typing/kwarg behavior and xy validation timing.
Confidence Score: 4/5The PR appears safe to merge after correcting the design documents so they do not present later stack entries as already shipped. The executable changes are contract tests with no identified behavioral defect, while the authoritative implementation plan currently directs readers to APIs and files that are absent from this revision. Files Needing Attention: spec/design/reflex-component-api-implementation.md, spec/design/reflex-component-api-options.md
|
| Filename | Overview |
|---|---|
| spec/design/reflex-component-api-implementation.md | Adds the phased implementation plan, but inaccurately marks later, currently absent phases as completed. |
| spec/design/reflex-component-api-options.md | Records the design alternatives and Option 6 decision, with its top-level implemented status similarly ahead of this stack entry. |
| tests/reflex_adapter/test_framework_contracts.py | Adds focused Reflex contract pins and correctly relies on the adapter directory's optional-dependency collection guards. |
| tests/test_validation_timing.py | Adds focused tests for zero-row compilation, validation timing, and deliberate Figure memoization behavior. |
Reviews (1): Last reviewed commit: "spec+test: design record and ground pins..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| **Status: implemented — Phases 0–4 landed (2026-08); this document is the | ||
| executed work plan plus its completion record (see "Completion record" | ||
| at the end).** Design authority: |
There was a problem hiding this comment.
The document marks Phases 0–4 as shipped even though this stack entry contains only Phase 0; readers following it will try absent APIs such as reflex_xy.data and reflex_xy.scatter_chart or look for implementation files that are not present at this revision. Please describe the later phases as planned or pending until their corresponding stack entries land.
Knowledge Base Used: The spec/ Directory: Design Dossier and Engineering Source of Truth
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@spec/design/reflex-component-api-options.md`:
- Around line 3-11: The design documents overstate the shipped implementation
status while runtime support remains unimplemented. In
spec/design/reflex-component-api-options.md lines 3-11, change the status to
describe the selected API as planned; in
spec/design/reflex-component-api-implementation.md lines 3-13, state that only
Phase 0 landed in this PR; and in lines 329-376, remove or defer the completion
record until the runtime implementation PRs land.
In `@tests/reflex_adapter/test_framework_contracts.py`:
- Around line 15-16: Update the module-level imports in
test_framework_contracts.py so pytest is imported first, then call
pytest.importorskip("reflex") before importing reflex as rx. Preserve the
existing test behavior while allowing collection to skip cleanly when the reflex
dependency is unavailable.
🪄 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: 48b06c37-05e2-4ea4-ad91-041d6632716b
📒 Files selected for processing (4)
spec/design/reflex-component-api-implementation.mdspec/design/reflex-component-api-options.mdtests/reflex_adapter/test_framework_contracts.pytests/test_validation_timing.py
| **Status: decided — Option 6 adopted (with Options 1 and 2 as its subsumed | ||
| parts), implemented.** See the decision record at the end of this document. | ||
| This document records the design space for that revision of the `reflex_xy` | ||
| public API. The shipped integration is specified in | ||
| [`reflex-integration.md`](reflex-integration.md) (the adopted tier: §3.6), | ||
| and the framework-agnostic composition contract in | ||
| [`reflex-shaped-api.md`](reflex-shaped-api.md). The file-level work plan | ||
| that guided the implementation lives in | ||
| [`reflex-component-api-implementation.md`](reflex-component-api-implementation.md). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not record unimplemented runtime phases as shipped.
This PR adds Phase 0 documents and contract tests only. The supplied runtime code still has FigureVar(return_type=str) in python/reflex_xy/vars.py:139-185 and legacy token/src component props in python/reflex_xy/component.py:58-110. These documents currently publish DataHandle, plans, flat factories, and compile probes as completed behavior.
spec/design/reflex-component-api-options.md#L3-L11: change the status to the selected and planned API.spec/design/reflex-component-api-implementation.md#L3-L13: describe Phase 0 only as landed in this PR.spec/design/reflex-component-api-implementation.md#L329-L376: remove or defer the completion record until the runtime implementation PRs land.
As per coding guidelines, resolve implementation/spec discrepancies.
📍 Affects 2 files
spec/design/reflex-component-api-options.md#L3-L11(this comment)spec/design/reflex-component-api-implementation.md#L3-L13spec/design/reflex-component-api-implementation.md#L329-L376
🤖 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/design/reflex-component-api-options.md` around lines 3 - 11, The design
documents overstate the shipped implementation status while runtime support
remains unimplemented. In spec/design/reflex-component-api-options.md lines
3-11, change the status to describe the selected API as planned; in
spec/design/reflex-component-api-implementation.md lines 3-13, state that only
Phase 0 landed in this PR; and in lines 329-376, remove or defer the completion
record until the runtime implementation PRs land.
Source: Coding guidelines
| import pytest | ||
| import reflex as rx |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'collect_ignore|pytest_ignore_collect|importorskip\(["'\'']reflex|skipif.*reflex|reflex_adapter' \
pyproject.toml pytest.ini setup.cfg tox.ini conftest.py tests 2>/dev/null || trueRepository: reflex-dev/xy
Length of output: 2368
🏁 Script executed:
# Find and examine test_framework_contracts.py to verify current import state
fd -t f 'test_framework_contracts.py' tests/
# Show first 20 lines to see imports
if [ -f tests/reflex_adapter/test_framework_contracts.py ]; then
head -20 tests/reflex_adapter/test_framework_contracts.py
fiRepository: reflex-dev/xy
Length of output: 851
🏁 Script executed:
# Check pytest configuration for any skip handling
cat pyproject.toml | grep -A 20 "tool.pytest"
# Check if there are any pytest plugins or hooks related to skip behavior
rg -n 'pytest_configure|pytest_collection|collect_ignore' tests/reflex_adapter/conftest.py
# Verify the complete conftest.py to see if it has any collection hooks
cat -n tests/reflex_adapter/conftest.pyRepository: reflex-dev/xy
Length of output: 1782
Add pytest.importorskip("reflex") before importing reflex in this module.
Module-level import reflex as rx at line 16 runs during test collection. The conftest.py in this directory has pytest.importorskip("reflex") at line 13, but that guards only conftest.py itself. When pytest parses test_framework_contracts.py, it still attempts the reflex import before reaching any skip logic. If reflex is not installed, collection fails. Add pytest.importorskip("reflex") before line 16 to skip this module's collection when reflex is unavailable.
🤖 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/reflex_adapter/test_framework_contracts.py` around lines 15 - 16,
Update the module-level imports in test_framework_contracts.py so pytest is
imported first, then call pytest.importorskip("reflex") before importing reflex
as rx. Preserve the existing test behavior while allowing collection to skip
cleanly when the reflex dependency is unavailable.
Source: Coding guidelines
Merging this PR will not alter performance
Comparing 
Footnotes
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
1 participant
First of a 7-PR stack implementing the
reflex_xydata-bound component API (Option 6). Review in order — each PR is green on its own, and checking out the last one gives you the whole thing.stack/1-design-and-pinsstack/2-typed-handlesFigureHandle, typedfigurepropstack/3-plans-and-data-varsplan.py,@reflex_xy.data, token grammarstack/4-composite-servingxyp1composites over the data planestack/5-chart-factoriesscatter_chart(data=…)and friendsstack/6-compile-probeprobe=for the escape hatchstack/7-demos-and-galleryThis PR
The design space and decision, its file-level work plan, and the tests everything above rests on. No runtime change.
The pins are the point:
test_framework_contracts.pyfixes the Reflex behavior the design depends on — parametrized-generic var types survive as_var_type(the schema channel), typed props reject wrong vars atcreate(), and unknown non-event kwargs are silently absorbed intostyle(the hazard PR5's kwarg partition has to compensate for).test_validation_timing.pyfixes xy's own validation timing — zero-row construction compiles, mark config validates at.figure()rather than construction, chrome validates eagerly,Chart.figure()memoizes.Each test names the design fact it pins (R1/R7/R8/X1–X3), so a Reflex or grammar upgrade that moves the ground fails here first instead of in the tier built on top.
Note for review
The two design documents are ~1300 of the 1491 lines. If we'd rather not carry them in-tree, say so and I'll drop them — the ~200 lines of pins are the part that has to land.
Test plan
uv run pytest tests/reflex_adapter tests/test_validation_timing.py— 165 passedpre-commit run --all-files,ruff check,ruff format --check,ty check— cleanSummary by CodeRabbit
Documentation
Tests