Data-bound chart components 2/7: typed figure handles - #462
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 SummaryThis PR introduces typed
Confidence Score: 4/5The PR is not yet safe to merge because static charts can still fail page evaluation when a kernel-backed event prop is explicitly disabled with The static-source validation constructs its offender list from keyword presence rather than configured handler values, so Files Needing Attention: python/reflex_xy/component.py
|
| Filename | Overview |
|---|---|
| python/reflex_xy/component.py | Adds the typed live-source path, positional compatibility shim, and static-event validation; the previously reported disabled-handler rejection remains. |
| python/reflex_xy/handles.py | Defines immutable, serializable typed handles and the compatibility token normalizer. |
| python/reflex_xy/vars.py | Changes synchronous and asynchronous figure vars from bare strings to FigureHandle values while preserving the empty-token sentinel. |
| python/reflex_xy/assets/XYChart.jsx | Reduces typed handles and legacy token props to one live subscription token throughout the component lifecycle. |
| python/reflex_xy/app.py | Normalizes handle and string inputs before forwarding public figure operations to the registry. |
| python/reflex_xy/init.py | Exports the handle types and changes registration helpers to return typed handles. |
| spec/design/reflex-integration.md | Documents the handle-valued state contract, typed component prop, compatibility period, and static-event refusal. |
Reviews (2): Last reviewed commit: "feat(reflex): typed figure handles and a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # source would be silent no-ops at runtime — fail the compile | ||
| # with the reason instead. | ||
| if props.get("src") is not None: | ||
| offenders = [name for name in _KERNEL_EVENT_PROPS if name in props] |
There was a problem hiding this comment.
When a static chart receives a kernel-backed event prop explicitly set to None, membership in props alone marks it as active, causing page evaluation to raise ValueError even though no handler is configured.
| offenders = [name for name in _KERNEL_EVENT_PROPS if name in props] | |
| offenders = [ | |
| name for name in _KERNEL_EVENT_PROPS if props.get(name) is not None | |
| ] |
Knowledge Base Used: reflex-xy: the Reflex integration package
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.
7 issues found across 12 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/assets/XYChart.jsx">
<violation number="1" location="python/reflex_xy/assets/XYChart.jsx:285">
P2: When both live spellings are present, an empty `figure.token` currently falls back to the legacy `token`, so a "not ready" handle can incorrectly resubscribe to an old token. Using explicit figure precedence (nullish handling instead of truthiness fallback) preserves the empty-handle sentinel and matches the documented `figure`-wins behavior.</violation>
</file>
<file name="python/reflex_xy/handles.py">
<violation number="1" location="python/reflex_xy/handles.py:78">
P2: Figure-only helper paths can now accept a `DataHandle` and treat its token as a figure token, which hides caller mistakes and can route invalid tokens into registry operations. Restricting `token_of` to `FigureHandle` (plus legacy `str`) keeps the compatibility shim while preserving the intended type guard.</violation>
</file>
<file name="tests/reflex_adapter/test_component.py">
<violation number="1" location="tests/reflex_adapter/test_component.py:52">
P3: The assertion was loosened from the precise `'token:"tok-abc"'` to a bare `'"tok-abc"' in rendered`, which no longer proves the token reaches the `figure` prop (it would pass if the token landed on any prop). Since the entire PR is about routing the token through the typed `figure` prop, consider anchoring the check to the figure prop — e.g. assert `'figure' in rendered` alongside, or match `'{"token": "tok-abc"}'` — so a regression back to the legacy token path is caught.</violation>
</file>
<file name="spec/design/reflex-integration.md">
<violation number="1" location="spec/design/reflex-integration.md:484">
P3: This added line says the typed-`figure` mis-typed source "fails at compile", but §3.1 (and the PR's own description) specifies the same R1 failure as happening at `create()` — page evaluation, not build-time compile. In Reflex, `create()` runs when the component tree is evaluated at render, so the compile claim in this section is inaccurate and contradicts §3.1. Recommend rewording to "fail at `create()` (page evaluation)" to keep the two sections consistent.</violation>
</file>
<file name="python/reflex_xy/component.py">
<violation number="1" location="python/reflex_xy/component.py:128">
P2: Static-source handler refusal currently treats `None` event props as active handlers and can raise on harmless conditional forwarding. Checking for non-`None` values keeps the refusal for real kernel handlers without blocking disabled props.</violation>
<violation number="2" location="python/reflex_xy/component.py:381">
P3: The deprecation warning emitted when a bare `token_string` or legacy `str`-typed var is passed positionally tells the user to "use chart(figure=...)", but `figure=` is the typed `Var[FigureHandle]` prop that now rejects raw strings with a `TypeError` at `create()` (see the new `test_component_rejects_wrong_var_and_raw_string_at_compile`). So the exact users this warning targets — string-token sources — are pointed at an API that refuses their input. Recommend guidance that reflects what actually works for them: wrap the token in a `FigureHandle` (`chart(figure=FigureHandle("..."))`), or explicitly keep the one-release-cycle `token` path, rather than the generic "use figure=` suggestion.</violation>
</file>
<file name="python/reflex_xy/__init__.py">
<violation number="1" location="python/reflex_xy/__init__.py:224">
P2: Invalid `release()` arguments can now fail silently instead of surfacing a type error, which makes leaked registrations harder to debug when cleanup calls are wrong. Aligning `release()` with the other public helper normalization path (`_token`) would keep misuse fail-fast and consistent.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| // One subscription token from the two live spellings. `figure` is the | ||
| // typed handle ({token}); the bare `token` string is the deprecated wire. | ||
| // An empty handle token means "not ready" — no subscription yet. | ||
| const liveToken = (figure && figure.token) || token || null; |
There was a problem hiding this comment.
P2: When both live spellings are present, an empty figure.token currently falls back to the legacy token, so a "not ready" handle can incorrectly resubscribe to an old token. Using explicit figure precedence (nullish handling instead of truthiness fallback) preserves the empty-handle sentinel and matches the documented figure-wins behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/assets/XYChart.jsx, line 285:
<comment>When both live spellings are present, an empty `figure.token` currently falls back to the legacy `token`, so a "not ready" handle can incorrectly resubscribe to an old token. Using explicit figure precedence (nullish handling instead of truthiness fallback) preserves the empty-handle sentinel and matches the documented `figure`-wins behavior.</comment>
<file context>
@@ -276,6 +279,10 @@ export function XYChart(props) {
+ // One subscription token from the two live spellings. `figure` is the
+ // typed handle ({token}); the bare `token` string is the deprecated wire.
+ // An empty handle token means "not ready" — no subscription yet.
+ const liveToken = (figure && figure.token) || token || null;
const elRef = useRef(null); // inner chart mount (wiped on payload swaps)
const outerRef = useRef(null); // stable wrapper: events, tooltip slot
</file context>
| const liveToken = (figure && figure.token) || token || null; | |
| const liveToken = figure != null ? (figure.token ?? null) : (token ?? null); |
| (``append``, ``set_view``, ``release``, ...) accept both a handle and the | ||
| old-style bare token string through this normalizer. | ||
| """ | ||
| if isinstance(source, (FigureHandle, DataHandle)): |
There was a problem hiding this comment.
P2: Figure-only helper paths can now accept a DataHandle and treat its token as a figure token, which hides caller mistakes and can route invalid tokens into registry operations. Restricting token_of to FigureHandle (plus legacy str) keeps the compatibility shim while preserving the intended type guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/handles.py, line 78:
<comment>Figure-only helper paths can now accept a `DataHandle` and treat its token as a figure token, which hides caller mistakes and can route invalid tokens into registry operations. Restricting `token_of` to `FigureHandle` (plus legacy `str`) keeps the compatibility shim while preserving the intended type guard.</comment>
<file context>
@@ -0,0 +1,82 @@
+ (``append``, ``set_view``, ``release``, ...) accept both a handle and the
+ old-style bare token string through this normalizer.
+ """
+ if isinstance(source, (FigureHandle, DataHandle)):
+ return source.token
+ if isinstance(source, str):
</file context>
| # source would be silent no-ops at runtime — fail the compile | ||
| # with the reason instead. | ||
| if props.get("src") is not None: | ||
| offenders = [name for name in _KERNEL_EVENT_PROPS if name in props] |
There was a problem hiding this comment.
P2: Static-source handler refusal currently treats None event props as active handlers and can raise on harmless conditional forwarding. Checking for non-None values keeps the refusal for real kernel handlers without blocking disabled props.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/component.py, line 128:
<comment>Static-source handler refusal currently treats `None` event props as active handlers and can raise on harmless conditional forwarding. Checking for non-`None` values keeps the refusal for real kernel handlers without blocking disabled props.</comment>
<file context>
@@ -102,6 +118,26 @@ class XYChart(rx.Component):
+ # source would be silent no-ops at runtime — fail the compile
+ # with the reason instead.
+ if props.get("src") is not None:
+ offenders = [name for name in _KERNEL_EVENT_PROPS if name in props]
+ if offenders:
+ msg = (
</file context>
| offenders = [name for name in _KERNEL_EVENT_PROPS if name in props] | |
| offenders = [name for name in _KERNEL_EVENT_PROPS if name in props and props[name] is not None] |
| globals()["registry"] = registry | ||
|
|
||
| registry.release(token) | ||
| registry.release(token_of(token) or "") |
There was a problem hiding this comment.
P2: Invalid release() arguments can now fail silently instead of surfacing a type error, which makes leaked registrations harder to debug when cleanup calls are wrong. Aligning release() with the other public helper normalization path (_token) would keep misuse fail-fast and consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/__init__.py, line 224:
<comment>Invalid `release()` arguments can now fail silently instead of surfacing a type error, which makes leaked registrations harder to debug when cleanup calls are wrong. Aligning `release()` with the other public helper normalization path (`_token`) would keep misuse fail-fast and consistent.</comment>
<file context>
@@ -202,16 +211,17 @@ def index():
globals()["registry"] = registry
- registry.release(token)
+ registry.release(token_of(token) or "")
</file context>
| registry.release(token_of(token) or "") | |
| token_str = token_of(token) | |
| if token_str is None: | |
| msg = f"expected a FigureHandle or figure token string, got {type(token).__name__}" | |
| raise TypeError(msg) | |
| registry.release(token_str) |
| assert str(comp.library).startswith("$/public/external/reflex_xy/assets/XYChart") | ||
| rendered = str(comp) | ||
| assert 'token:"tok-abc"' in rendered | ||
| assert '"tok-abc"' in rendered # the handle's token reaches the figure prop |
There was a problem hiding this comment.
P3: The assertion was loosened from the precise 'token:"tok-abc"' to a bare '"tok-abc"' in rendered, which no longer proves the token reaches the figure prop (it would pass if the token landed on any prop). Since the entire PR is about routing the token through the typed figure prop, consider anchoring the check to the figure prop — e.g. assert 'figure' in rendered alongside, or match '{"token": "tok-abc"}' — so a regression back to the legacy token path is caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/reflex_adapter/test_component.py, line 52:
<comment>The assertion was loosened from the precise `'token:"tok-abc"'` to a bare `'"tok-abc"' in rendered`, which no longer proves the token reaches the `figure` prop (it would pass if the token landed on any prop). Since the entire PR is about routing the token through the typed `figure` prop, consider anchoring the check to the figure prop — e.g. assert `'figure' in rendered` alongside, or match `'{"token": "tok-abc"}'` — so a regression back to the legacy token path is caught.</comment>
<file context>
@@ -40,11 +40,16 @@ def app_cwd(tmp_path, monkeypatch):
assert str(comp.library).startswith("$/public/external/reflex_xy/assets/XYChart")
rendered = str(comp)
- assert 'token:"tok-abc"' in rendered
+ assert '"tok-abc"' in rendered # the handle's token reaches the figure prop
assert "onPointHover" in rendered
assert "picked" in rendered # the reflex event dispatch is in the prop
</file context>
| `register()`/`inline()`, landing in the typed `figure` prop | ||
| (`Var[FigureHandle]`) and riding the socket data plane. Because the prop is | ||
| `Var`-typed, `chart(figure=Dash.points)` and `chart(figure="raw string")` | ||
| fail at compile with the framework's `TypeError` (R1). A Chart/Figure |
There was a problem hiding this comment.
P3: This added line says the typed-figure mis-typed source "fails at compile", but §3.1 (and the PR's own description) specifies the same R1 failure as happening at create() — page evaluation, not build-time compile. In Reflex, create() runs when the component tree is evaluated at render, so the compile claim in this section is inaccurate and contradicts §3.1. Recommend rewording to "fail at create() (page evaluation)" to keep the two sections consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/design/reflex-integration.md, line 484:
<comment>This added line says the typed-`figure` mis-typed source "fails at compile", but §3.1 (and the PR's own description) specifies the same R1 failure as happening at `create()` — page evaluation, not build-time compile. In Reflex, `create()` runs when the component tree is evaluated at render, so the compile claim in this section is inaccurate and contradicts §3.1. Recommend rewording to "fail at `create()` (page evaluation)" to keep the two sections consistent.</comment>
<file context>
@@ -469,12 +476,31 @@ reflex_xy.chart(
+`register()`/`inline()`, landing in the typed `figure` prop
+(`Var[FigureHandle]`) and riding the socket data plane. Because the prop is
+`Var`-typed, `chart(figure=Dash.points)` and `chart(figure="raw string")`
+fail at compile with the framework's `TypeError` (R1). A Chart/Figure
+passed positionally compiles to a payload asset and lands in the `src`
+prop, which the wrapper fetches and renders kernel-less — the static tier
</file context>
| _warn_positional( | ||
| "chart(figure=...) — register()/inline() return a FigureHandle, and " | ||
| "@reflex_xy.figure vars are FigureHandle-valued" | ||
| ) |
There was a problem hiding this comment.
P3: The deprecation warning emitted when a bare token_string or legacy str-typed var is passed positionally tells the user to "use chart(figure=...)", but figure= is the typed Var[FigureHandle] prop that now rejects raw strings with a TypeError at create() (see the new test_component_rejects_wrong_var_and_raw_string_at_compile). So the exact users this warning targets — string-token sources — are pointed at an API that refuses their input. Recommend guidance that reflects what actually works for them: wrap the token in a FigureHandle (chart(figure=FigureHandle("..."))), or explicitly keep the one-release-cycle token path, rather than the generic "use figure=` suggestion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/reflex_xy/component.py, line 381:
<comment>The deprecation warning emitted when a bare `token_string` or legacy `str`-typed var is passed positionally tells the user to "use chart(figure=...)", but `figure=` is the typed `Var[FigureHandle]` prop that now rejects raw strings with a `TypeError` at `create()` (see the new `test_component_rejects_wrong_var_and_raw_string_at_compile`). So the exact users this warning targets — string-token sources — are pointed at an API that refuses their input. Recommend guidance that reflects what actually works for them: wrap the token in a `FigureHandle` (`chart(figure=FigureHandle("..."))`), or explicitly keep the one-release-cycle `token` path, rather than the generic "use figure=` suggestion.</comment>
<file context>
@@ -299,7 +357,31 @@ def chart(
+ if tailwind_manifest:
+ props["tailwind_class_tokens"] = _tailwind_scan_literal(tailwind_manifest)
+ elif isinstance(source, (str, rx.Var)):
+ _warn_positional(
+ "chart(figure=...) — register()/inline() return a FigureHandle, and "
+ "@reflex_xy.figure vars are FigureHandle-valued"
</file context>
| _warn_positional( | |
| "chart(figure=...) — register()/inline() return a FigureHandle, and " | |
| "@reflex_xy.figure vars are FigureHandle-valued" | |
| ) | |
| _warn_positional( | |
| "chart(figure=FigureHandle(...)) — register()/inline() now return a " | |
| "FigureHandle; pass it via figure= (a raw string is rejected by the " | |
| "typed figure prop)" | |
| ) |
Chart state vars carried a bare token string, so the component's source was untyped: reflex_xy.chart(Dash.points) — the wrong var — compiled fine and failed at hydrate as a blank mount with an err frame. Wrap the token in a frozen dataclass (handles.py: FigureHandle, and DataHandle[S] for the tier above this one) and give the component a Var[FigureHandle] `figure` prop. The wrong var or a raw string now fails at create(), which is page evaluation — the framework's own TypeError, before a browser is involved. @reflex_xy.figure vars, register(), and inline() all return handles; the empty-token handle keeps the existing "not ready / no chart" sentinel, so the var type stays non-optional. Two compile-time refusals come with it: kernel-backed event props (on_point_hover/on_point_click/on_select_end) on a static src source now raise instead of silently never firing, and the positional live spellings chart(var) / chart(token_string) warn while routing to figure=. The static positional chart(Chart) form is NOT deprecated — it remains the only route for arbitrary Charts such as facet grids. Public helpers that take "a figure" (append, set_view, reset_view, select, clear_selection, release) accept a handle or its bare token for one release cycle. The wrapper reduces `figure` and legacy `token` to one subscription token; nothing below the subscribe path changes. Spec: reflex-integration.md §3.1 (handle-valued vars), §5 (figure= prop, deprecation shim, event refusal), file map.
c42e175 to
d0886d6
Compare
1 participant
Stacked on #461. Base is
stack/1-design-and-pins— the Files-changed tab shows only this layer.Problem
Chart state vars carried a bare token string, so the component's source was untyped.
reflex_xy.chart(Dash.points)— the wrong var — compiled fine and failed at hydrate as a blank mount with anerrframe.Change
Wrap the token in a frozen dataclass (
handles.py:FigureHandle, plusDataHandle[S]for the tier above) and give the component aVar[FigureHandle]figureprop. The wrong var or a raw string now fails atcreate()— page evaluation — with the framework's ownTypeError, before a browser is involved.@reflex_xy.figurevars,register(), andinline()all return handles. The empty-token handle keeps the existing "not ready / no chart" sentinel, so the var type stays non-optional.Two compile-time refusals come with it:
on_point_hover/on_point_click/on_select_end) on a staticsrcsource now raise instead of silently never firing;chart(var)/chart(token_string)warn while routing tofigure=.chart(Chart)— the positional static form — is deliberately NOT deprecated. It's the only route for arbitrary Charts such as facet grids, and it predates no replacement.Public helpers that take "a figure" (
append,set_view,reset_view,select,clear_selection,release) accept a handle or its bare token for one release cycle.The wrapper reduces
figureand legacytokento one subscription token; nothing below the subscribe path changes.Spec
reflex-integration.md§3.1 (handle-valued vars), §5 (figure=prop, deprecation shim, event refusal), file map.Test plan
uv run pytest tests/reflex_adapter tests/test_validation_timing.py— 170 passedpre-commit run --all-files,ruff check,ruff format --check,ty check— clean