Data-bound chart components 3/7: plans and @reflex_xy.data - #463
Data-bound chart components 3/7: plans and @reflex_xy.data#463FarhanAliRaza wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Greptile SummaryThe PR introduces the foundational data-bound chart layer without yet exposing its component or wire surface.
Confidence Score: 5/5The PR appears safe to merge because no eligible blocking failure or outstanding prior finding remains in the supplied follow-up context. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| python/reflex_xy/data_vars.py | Adds typed data computed vars, runtime column validation, deterministic token publication, async dispatch, and release-on-None behavior. |
| python/reflex_xy/plan.py | Adds canonical chart-plan serialization, zero-row structural validation, digest registration, and fresh-chart binding. |
| python/reflex_xy/registry.py | Adds versioned column storage, dependent-plan indexing and rebuilding, release behavior, TTL sweeping, and an out-of-band error seam. |
| python/reflex_xy/tokens.py | Extends deterministic token parsing and construction for data tokens and composite plan tokens. |
| python/reflex_xy/init.py | Exposes the new data decorator and computed-var types through the package API. |
| spec/design/reflex-integration.md | Documents the data-bound architecture, plan format, token identities, registry lifecycle, and file ownership. |
| tests/reflex_adapter/test_data_var.py | Covers schema typing, dependency tracking, publication, republishing, release behavior, async evaluation, and validation. |
| tests/reflex_adapter/test_plan.py | Covers plan validation, canonical identity, binding behavior, unsupported structures, and registry lookup. |
Reviews (2): Last reviewed commit: "feat(reflex): chart plans and @reflex_xy..." | Re-trigger Greptile
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.
6 issues found across 8 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/reflex_xy/plan.py">
<violation number="1" location="python/reflex_xy/plan.py:204">
P2: Content-addressed plans can drift after registration because `build_plan` keeps references to mutable inputs instead of snapshotting them. If the original mark/props objects are mutated later, future `bind()` calls can produce a chart that no longer matches the plan digest and probe-derived bindings.</violation>
</file>
<file name="python/reflex_xy/data_vars.py">
<violation number="1" location="python/reflex_xy/data_vars.py:40">
P3: This adds a second near-identical copy of the computed-var wrapper plumbing that already exists in `vars.py`, so future fixes can easily land in one path and drift in the other. A small shared internal helper for the common wrapper/deps identity logic would reduce that maintenance risk.</violation>
</file>
<file name="python/reflex_xy/tokens.py">
<violation number="1" location="python/reflex_xy/tokens.py:60">
P2: Data tokens are now accepted by `parse_token`, but the namespace rebuild path still treats any parsed token as a figure rebuild candidate. This makes `xyd1|...` subscribe attempts execute data builders before failing, so it would be safer to gate rebuild attempts to `kind == "figure"` in the rebuild/entry path.</violation>
</file>
<file name="tests/reflex_adapter/test_plan.py">
<violation number="1" location="tests/reflex_adapter/test_plan.py:110">
P3: These tests keep registering plans into the process-global `_PLANS` map and never reset it, so entries accumulate for the whole session and the `is` identity assertions in `test_registry_lookup_and_miss` depend on shared session state. Since content-addressing already makes digests idempotent this is only a leak/order-sensitivity concern, but the dedicated `reset_plans_for_tests()` helper exists precisely for this and could be wired in via an autouse fixture to keep the plan tests hermetic.</violation>
</file>
<file name="python/reflex_xy/registry.py">
<violation number="1" location="python/reflex_xy/registry.py:581">
P2: Plan-index entries can accumulate after charts unmount, increasing memory and republish scan work over time. `bind_plan` only inserts, while normal unsubscribe/disconnect lifecycle does not remove those bindings.</violation>
<violation number="2" location="python/reflex_xy/registry.py:622">
P1: Concurrent republish of one data token can send stale pixels after a newer dataset already published. The rebuild path does not verify column generation before `publish(composite, figure)`, so an older in-flight rebuild can win the race.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| self.release(composite) | ||
| self._schedule_error(composite, str(exc), resync=True) | ||
| return | ||
| self.publish(composite, figure) |
There was a problem hiding this comment.
P1: Concurrent republish of one data token can send stale pixels after a newer dataset already published. The rebuild path does not verify column generation before publish(composite, figure), so an older in-flight rebuild can win the race.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/registry.py, line 622:
<comment>Concurrent republish of one data token can send stale pixels after a newer dataset already published. The rebuild path does not verify column generation before `publish(composite, figure)`, so an older in-flight rebuild can win the race.</comment>
<file context>
@@ -478,6 +535,92 @@ def __len__(self) -> int:
+ self.release(composite)
+ self._schedule_error(composite, str(exc), resync=True)
+ return
+ self.publish(composite, figure)
+
# -- version bump + fan-out ---------------------------------------------
</file context>
| plan = ChartPlan( | ||
| kind=kind, | ||
| children=children, | ||
| chart_props=chart_props, |
There was a problem hiding this comment.
P2: Content-addressed plans can drift after registration because build_plan keeps references to mutable inputs instead of snapshotting them. If the original mark/props objects are mutated later, future bind() calls can produce a chart that no longer matches the plan digest and probe-derived bindings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/plan.py, line 204:
<comment>Content-addressed plans can drift after registration because `build_plan` keeps references to mutable inputs instead of snapshotting them. If the original mark/props objects are mutated later, future `bind()` calls can produce a chart that no longer matches the plan digest and probe-derived bindings.</comment>
<file context>
@@ -0,0 +1,237 @@
+ plan = ChartPlan(
+ kind=kind,
+ children=children,
+ chart_props=chart_props,
+ columns=tuple(probe.seen),
+ tailwind_classes=tailwind_classes,
</file context>
| r"\|(?P<state>[A-Za-z0-9_.]{1,512})" | ||
| r"\|(?P<var>[A-Za-z_][A-Za-z0-9_]{0,255})$" | ||
| ) | ||
| _TOKEN_RE = re.compile(r"^(?P<prefix>xyv1|xyd1)\|" + _STATE_TOKEN_BODY) |
There was a problem hiding this comment.
P2: Data tokens are now accepted by parse_token, but the namespace rebuild path still treats any parsed token as a figure rebuild candidate. This makes xyd1|... subscribe attempts execute data builders before failing, so it would be safer to gate rebuild attempts to kind == "figure" in the rebuild/entry path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/tokens.py, line 60:
<comment>Data tokens are now accepted by `parse_token`, but the namespace rebuild path still treats any parsed token as a figure rebuild candidate. This makes `xyd1|...` subscribe attempts execute data builders before failing, so it would be safer to gate rebuild attempts to `kind == "figure"` in the rebuild/entry path.</comment>
<file context>
@@ -27,47 +36,88 @@
r"\|(?P<state>[A-Za-z0-9_.]{1,512})"
r"\|(?P<var>[A-Za-z_][A-Za-z0-9_]{0,255})$"
)
+_TOKEN_RE = re.compile(r"^(?P<prefix>xyv1|xyd1)\|" + _STATE_TOKEN_BODY)
+# Plan digests are lowercase sha256 hex prefixes (plan.py).
+_PLAN_RE = re.compile(r"^xyp1\|(?P<digest>[0-9a-f]{8,64})\|(?P<data>xyd1\|.+)$")
</file context>
| def bind_plan(self, data_token: str, digest: str) -> None: | ||
| """Record that a mounted plan binds this data token (idempotent).""" | ||
| with self._mutex: | ||
| self._digests_by_data_token.setdefault(data_token, set()).add(digest) |
There was a problem hiding this comment.
P2: Plan-index entries can accumulate after charts unmount, increasing memory and republish scan work over time. bind_plan only inserts, while normal unsubscribe/disconnect lifecycle does not remove those bindings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/registry.py, line 581:
<comment>Plan-index entries can accumulate after charts unmount, increasing memory and republish scan work over time. `bind_plan` only inserts, while normal unsubscribe/disconnect lifecycle does not remove those bindings.</comment>
<file context>
@@ -478,6 +535,92 @@ def __len__(self) -> int:
+ def bind_plan(self, data_token: str, digest: str) -> None:
+ """Record that a mounted plan binds this data token (idempotent)."""
+ with self._mutex:
+ self._digests_by_data_token.setdefault(data_token, set()).add(digest)
+
+ def _rebuild_dependent(self, data_token: str, digest: str, columns: dict[str, Any]) -> None:
</file context>
| __all__ = ["AsyncDataVar", "DataVar", "data", "validate_columns"] | ||
|
|
||
|
|
||
| def _builder_target(var: Any, obj: Any) -> Any: |
There was a problem hiding this comment.
P3: This adds a second near-identical copy of the computed-var wrapper plumbing that already exists in vars.py, so future fixes can easily land in one path and drift in the other. A small shared internal helper for the common wrapper/deps identity logic would reduce that maintenance risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/data_vars.py, line 40:
<comment>This adds a second near-identical copy of the computed-var wrapper plumbing that already exists in `vars.py`, so future fixes can easily land in one path and drift in the other. A small shared internal helper for the common wrapper/deps identity logic would reduce that maintenance risk.</comment>
<file context>
@@ -0,0 +1,233 @@
+__all__ = ["AsyncDataVar", "DataVar", "data", "validate_columns"]
+
+
+def _builder_target(var: Any, obj: Any) -> Any:
+ """Point dependency tracking at the *data method*, not the token wrapper
+ (same reason as vars.py: the wrapper fget reads nothing but the router)."""
</file context>
|
|
||
| def test_registry_lookup_and_miss(): | ||
| plan = scatter_plan() | ||
| assert plan_of(plan.digest) is plan |
There was a problem hiding this comment.
P3: These tests keep registering plans into the process-global _PLANS map and never reset it, so entries accumulate for the whole session and the is identity assertions in test_registry_lookup_and_miss depend on shared session state. Since content-addressing already makes digests idempotent this is only a leak/order-sensitivity concern, but the dedicated reset_plans_for_tests() helper exists precisely for this and could be wired in via an autouse fixture to keep the plan tests hermetic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/reflex_adapter/test_plan.py, line 110:
<comment>These tests keep registering plans into the process-global `_PLANS` map and never reset it, so entries accumulate for the whole session and the `is` identity assertions in `test_registry_lookup_and_miss` depend on shared session state. Since content-addressing already makes digests idempotent this is only a leak/order-sensitivity concern, but the dedicated `reset_plans_for_tests()` helper exists precisely for this and could be wired in via an autouse fixture to keep the plan tests hermetic.</comment>
<file context>
@@ -0,0 +1,113 @@
+
+def test_registry_lookup_and_miss():
+ plan = scatter_plan()
+ assert plan_of(plan.digest) is plan
+ assert plan_of("feedfacefeedfacefeed") is None
+ with pytest.raises(PlanMissError, match="feedfacefeedfacefeed"):
</file context>
The two halves the data-bound component tier is made of, with no wire or
component surface yet.
plan.py — a ChartPlan is a validated, data-free chart structure. Building
one constructs the real xy tree from string channels, binds zero-row
placeholder columns for every referenced channel through the production
resolution path (a recording table, so the column list cannot drift from
what binding will look up), and runs .figure() once: the full mark/config
validation gate, in milliseconds, with no data ingestion. The canonical
JSON (plan_version: 1) is content-addressed into a digest and registered in
a process-local map; binding is columns + plan -> a fresh Chart -> figure,
with mismatch errors that name both sides. PLAN_VERSION and a golden digest
are pinned so accidental format churn fails a test rather than silently
invalidating every mounted client's digest.
data_vars.py — @reflex_xy.data is the exact sibling of @reflex_xy.figure:
same builder-targeted dependency tracking, pre-session short-circuit,
underscore refusal, async dispatch, and None-releases semantics, but its
value is a DataHandle and what it publishes is columns. The method's return
annotation is the schema channel: a TypedDict parametrizes the handle, so
the layer above can read column names without executing user code.
tokens.py grows two families beside xyv1: xyd1 for data tokens (same
grammar and rebuild contract) and the composite xyp1|<digest>|<data token>
that names a bound figure — plan digest for the structure, data token for
the columns, both halves independently recoverable.
registry.py stores column entries as their own kind: immutable generations
with no locks and no pins (a republish replaces the whole entry), swept
under the existing TTL because the data method is the rebuild recipe. The
data-token -> {digest} index and the on_error seam land here; the namespace
wires them in the next change.
Spec: reflex-integration.md §3.6 (data vars, plans, column entries, format
stability), file map.
2f7cb25 to
4b1f770
Compare
1 participant
Stacked on #462. Base is
stack/2-typed-handles.The two halves the data-bound tier is made of. No wire and no component surface yet — those are PRs 4 and 5.
plan.pyA
ChartPlanis a validated, data-free chart structure. Building one constructs the real xy tree from string channels, binds zero-row placeholder columns for every referenced channel through the production resolution path (a recording table, so the column list cannot drift from what binding will look up), and runs.figure()once — the full mark/config validation gate, in milliseconds, with no data ingestion.The canonical JSON (
plan_version: 1) is content-addressed into a digest and registered in a process-local map. Binding is the reverse: columns + plan → a freshChart(never reused) →.figure(), with mismatch errors that name both sides.PLAN_VERSIONand a golden digest are pinned, so accidental format churn fails a test rather than silently invalidating every mounted client's digest.data_vars.py@reflex_xy.datais the exact sibling of@reflex_xy.figure— same builder-targeted dependency tracking, pre-session short-circuit, underscore refusal, async dispatch,None-releases semantics — but its value is aDataHandleand what it publishes is columns.The method's return annotation is the schema channel: a
TypedDictparametrizes the handle, so PR5's factories can read column names without executing user code.tokens.py,registry.pyTwo families join
xyv1:xyd1for data tokens (same grammar and rebuild contract) and the compositexyp1|<digest>|<data token>naming a bound figure — plan digest for structure, data token for columns, both halves independently recoverable.Column entries are their own kind of registry entry: immutable generations with no locks and no pins (a republish replaces the whole entry), swept under the existing TTL because the data method is the rebuild recipe. The
data token → {digest}index and theon_errorseam land here; PR4 wires them to the namespace.Spec
reflex-integration.md§3.6 (data vars, plans, column entries, format stability), file map.Test plan
uv run pytest tests/reflex_adapter tests/test_validation_timing.py— 200 passedpre-commit run --all-files,ruff check,ruff format --check,ty check— clean