Data-bound chart components 5/8: the chart factories - #465
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 public data-bound chart factory surface and registers compiled chart plans in backend workers.
Confidence Score: 5/5The PR appears safe to merge because no eligible blocking failure or outstanding prior finding was established. No blocking failure remains within the supplied follow-up-review scope.
|
| Filename | Overview |
|---|---|
| python/reflex_xy/factories.py | Adds strict flat and composed data-bound chart factories, plan construction, schema checks, static binding, and component mounting. |
| python/reflex_xy/app.py | Adds fail-closed worker-startup evaluation of page components to populate process-local chart plans. |
| python/reflex_xy/assets/XYChart.jsx | Adds plan/data token composition and bounded resync handling for live plan subscriptions. |
| python/reflex_xy/component.py | Extends the private Reflex component with typed plan and data properties. |
| python/reflex_xy/init.py | Exposes chart factories and a curated set of xy node constructors through the public package surface. |
| docs/app/xy_docs/markdown.py | Restores per-fence execution namespaces when documentation pages are rendered repeatedly. |
| spec/design/reflex-integration.md | Documents the factory API, static symmetry, supported chart kinds, and worker plan-registration lifecycle. |
Reviews (5): Last reviewed commit: "fix(reflex): make the worker-startup ref..." | Re-trigger Greptile
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
1a9158c to
6301617
Compare
6301617 to
5f15a49
Compare
|
Review addressed in 5f15a49:
|
masenf
left a comment
There was a problem hiding this comment.
docs build failing
Traceback (most recent call last):
File "/usr/lib/python3.12/asyncio/events.py", line 88, in _run
self._context.run(self._callback, *self._args)
File "/home/runner/work/xy/xy/docs/app/.venv/lib/python3.12/site-packages/reflex/app_mixins/lifespan.py", line 115, in <lambda>
task_.add_done_callback(lambda t: t.result())
^^^^^^^^^^
File "/home/runner/work/xy/xy/python/reflex_xy/app.py", line 67, in _lifespan
_ensure_page_plans(app)
File "/home/runner/work/xy/xy/python/reflex_xy/app.py", line 113, in _ensure_page_plans
raise RuntimeError(msg)
RuntimeError: reflex_xy: evaluating page component functions for chart-plan registration failed on this worker; serving would leave its plan map incomplete (load-balancer-dependent blank charts), so startup is refused. Failing pages: 'styling/customize': ValueError: area x and y must have equal length, got 8 and 7; 'styling/examples': ValueError: column x and y must have equal length, got 12 and 6
otherwise the implementation seems fine
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The user-facing half of the tier: reflex_xy.scatter_chart(data=Dash.cloud, x="x", y="y", color="mag") and the composed reflex_xy.chart(*nodes, data=...) for multi-mark charts. A factory call at page evaluation builds a plan, so the chart's structure is validated at `reflex run` rather than at hydrate. What that buys, in the order a user hits it: a hallucinated factory name fails at import (the xy node re-exports are an explicit curated map, not getattr passthrough); an unknown kwarg fails with a did-you-mean; a bad colormap, enum, or axis ref fails in the zero-row probe; an unknown column name fails against the data var's TypedDict without executing the data method; and the wrong var or a raw string in data= fails on the typed prop. The kwarg partition — mark options vs chrome vs component props vs event handlers — is derived from inspect.signature at import rather than hand-listed, because a hand-listed partition silently drifts from xy's signatures and Reflex absorbs unknown non-event kwargs into `style` where a typo would vanish rather than raise (the hazard Phase 0 pinned). Collisions get generated aliases, pinned by test. Two mounts from one surface: a Var data source becomes the plan/data props the wrapper composes into a composite subscription, while a concrete mapping binds immediately and routes to the static payload-asset path — same validation, works under `reflex export`. Aggregating kinds (box, violin, hexbin, contour, heatmap, stairs, ecdf) and the data-taking composites (pie, radar, wind_rose, sankey) are excluded from the plan tier and refused by name with the two working routes: their validators need real values, and a synthetic-row probe would validate against made-up data — a silent decimation of the compile guarantee. Page-plan registration lands here too, with the factories that populate the map: backend-only workers import the app without evaluating pages, so the startup lifespan evaluates them once, making "the plan map is populated in every worker" true by construction instead of an assumption about Reflex. Spec: reflex-integration.md §3.6 (factories, static symmetry, kind coverage, page-plan registration), file map.
…er bench
- Flat and composed factories reject every unknown top-level kwarg at
page evaluation (did-you-mean when close) instead of letting far-off
typos silently become CSS; style={...} is the explicit CSS route.
- _ensure_page_plans fails closed: a page that cannot evaluate refuses
worker startup naming every failing page, rather than warning and
serving an incomplete plan map (load-balancer-dependent blank charts).
- scripts/bench_reflex_plans.py: reproducible evidence for the tier's
new cost centers (plan build 0.26ms, 20x4-chart worker startup 15ms,
100k-point republish 1.0ms, ~1.5KB/plan); recorded in
reflex-integration.md §6 with regression contract.
- Quickstart multi-mark example binds a column its schema declares.
The shared Markdown renderer runs every exec fence of a page into one synthetic module and skips re-executing a fence it has already run, so a demo's preview function resolves its data from that module at call time. `_ensure_page_plans` evaluates page bodies a second time in the same process, and on that pass every preview read the end-of-page namespace -- whatever the last fence bound `months`/`x`/`y` to. Pages that reuse names rebuilt from another demo's data: styling/customize and styling/examples failed outright on mismatched lengths and refused worker startup, and where lengths agreed the second pass silently minted plan digests the compiled frontend never references. Snapshot the namespace at the end of each fence's first execution and restore it in place -- the module dict object is what the fence's functions close over -- before that fence renders again, so every render sees the namespace the first one did. Cover it with a repeated-render test over every page whose fences rebind a name, asserting identical content-addressed payload sources, and record the re-evaluability contract that page-plan registration puts on app code.
…m_chart
The errResyncs cap guards against same-connection err{resync} loops, but
it also dead-ended a chart whose backend recovered after five failed
attempts with nothing short of a remount. A fresh connection now resets
the budget alongside the resubscribe it already triggers.
stem_chart was implemented, registered, and importable but missing from
factories.__all__ — the one flat factory absent from the public list.
f10031f to
3b6abdb
Compare
There was a problem hiding this comment.
Greptile has paused reviews on this repository — it used its 100 free open-source review credits for this billing period. Reviews resume automatically on September 3. To continue before then, an organization admin can keep reviews running past the free credits — those bill as normal usage.
`_ensure_page_plans` was fail-closed in name only. Reflex starts a coroutine lifespan task with `asyncio.create_task(task())` and then yields, so a raise inside an `async def` lifespan body landed in a background task on an already-serving worker — the load-balancer-dependent blank charts the check exists to prevent. The registered task is now a plain function that runs the page pass synchronously and returns the sweep coroutine, so the raise happens in the `task()` call Reflex makes inline, before create_task and before the lifespan yields. Alongside it, four narrower corrections: - The signature-derived kwarg partition excluded only `data`, so xy's private adapter knobs (`_artist_alpha`, `_marker_path`, …) were accepted as flat kwargs and offered as did-you-mean candidates. Underscore-prefixed params are now filtered out of both. - Every public `reflex_xy` export is restated under `TYPE_CHECKING` — the twelve chart factories and the curated xy node re-exports, which typed as missing/`Any` behind `__getattr__`. A new test pins `__all__` against the static declarations in both directions. - Demo-fence namespace snapshots key on (page, source, occurrence) instead of (page, source). A page repeating an identical fence would otherwise restore the first occurrence's namespace, discarding what the fences between them defined — not the shared renderer's accumulation semantics. One occurrence counter is threaded through the body and FAQ transformers. - The republish benchmark sweeps data size (10k → 5M, straddling the 200k density threshold) and reports ms per million points. "State deltas independent of data size" is a scaling claim; one datum at one N is consistent with any growth curve. Table in §6 replaced with the sweep. Also: the repeated-render docs guard now detects annotated, augmented, unpacked, and def/class rebinding, not just plain assignment (12 → 13 pages covered), and the fourfold `app_cwd` fixture is hoisted into `tests/reflex_adapter/conftest.py`.
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…blish sweep Three follow-ups from review of af9f99c. Occurrence-keyed fence snapshots made a snapshot's identity positional, but `_FENCE_NAMESPACES` was never invalidated. Editing a page that repeats a fence renumbers every surviving copy, so a fence could restore a namespace captured when it sat elsewhere in the sequence — one predating the bindings now ahead of it. The dev server re-renders in-process on reload, so that is the live path. Snapshots are now versioned by a digest of the page source and a page's entries are dropped as soon as its content changes, which also bounds the cache: superseded page versions are evicted instead of holding a full namespace snapshot alive for the life of the process. The rebound-name detector only walked a fence's top-level statements, so bindings inside `for`/`with`/`if`/`try`/`match` bodies were invisible while the docstring claimed every form. It now recurses through compound statements (stopping at `def`/`class` bodies, which are a separate scope) and handles for/with/except/match targets, imports, and walrus. Imports are the substantive gain: counting them takes the guarded page set from 13 to 41 of 74, for 5.7s. The §6 republish prose claimed the normalized column "stays flat" while its own table showed 5M above the 1M floor. The table was at fault, not the prose: at one trial per size, run-to-run variance exceeds the gap between neighbouring sizes, and the 4.7 ms/1M reading was noise (three trials at that size give 3.40 / 2.74 / 3.02). The sweep now runs `REPUBLISH_TRIALS` trials per size and reports the observed range beside the median, and adds a 2M row at the direct soft ceiling. Re-recorded: 2.95 / 2.86 / 2.77 ms per 1M at 1M/2M/5M with overlapping ranges. The stated regression signal is the top of the sweep rising clear of that band, not any increase between adjacent rows.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Stacked on #464. Base is
stack/4-composite-serving. This is the headline PR — the user-facing surface.What the compile now catches
A factory call at page evaluation builds a plan, so structure is validated at
reflex runrather than at hydrate. In the order a user hits it:getattrpassthrough);data=fails on the typed prop.The kwarg partition
Derived from
inspect.signatureat import, not hand-listed. A hand-listed partition silently drifts from xy's signatures, and Reflex absorbs unknown non-event kwargs intostyle— so a typo would vanish rather than raise (the hazard PR1 pinned). Collisions get generated aliases (mark_<name>, withwidth→stroke_widthwhere the mark hasn't claimed it), pinned by test.Two mounts, one surface
A
Vardata source becomes theplan/dataprops the wrapper composes into a composite subscription. A concrete mapping binds immediately and routes to the static payload-asset path — same validation, works underreflex export.Excluded kinds (recorded decision)
Aggregating marks (box, violin, hexbin, contour, heatmap, stairs, ecdf) and the data-taking composites (pie, radar, wind_rose, sankey) are refused by name, pointing at the two routes that work. Their validators need real values, and a synthetic-row probe would validate against made-up data — a silent decimation of the compile guarantee.
Page-plan registration
_ensure_page_planslands here rather than in PR4, with the factories that populate the map: backend-only workers import the app without evaluating pages, so the startup lifespan evaluates them once, making "the plan map is populated in every worker" true by construction instead of an assumption about Reflex.Spec
reflex-integration.md§3.6 (factories, static symmetry, kind coverage, page-plan registration), file map.Test plan
uv run pytest tests/reflex_adapter tests/test_validation_timing.py— 235 passedpre-commit run --all-files,ruff check,ruff format --check,ty check— clean