Wrap Recharts Sankey chart - #6708
Conversation
Greptile SummaryThis PR wraps Recharts' Sankey chart as
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| packages/reflex-components-recharts/src/reflex_components_recharts/charts.py | Adds SankeyChart, SankeyNamespace, typed TypedDicts, and @sankey_node/@sankey_link decorators. Core logic is sound; minor gaps in error handling and missing node-event props. |
| packages/reflex-base/src/reflex_base/vars/special.py | New use_hook_var / use_id utilities. Clean implementation: unique variable name, aliased import to avoid collisions, correct VarData wiring. |
| packages/reflex-components-recharts/src/reflex_components_recharts/general.py | Adds SankeyChart to ResponsiveContainer valid children and exports use_chart_width. Both additions are straightforward and correct. |
| tests/units/components/graphing/test_recharts.py | Thorough new tests: ResponsiveContainer wrapping, alias render, namespace style key, deferred-annotation compatibility, positional-only param rejection, and use_chart_width VarData shape. |
| docs/library/graphing/charts/sankeychart.md | Docs page with simple, stateful, and fully custom examples. F-string Var interpolation is valid via Reflex's format override; gradient linkage pattern matches documented use_id idiom. |
Reviews (15): Last reviewed commit: "Fix Sankey docs renderer type" | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e94c1102db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Merging this PR will not alter performance
Comparing Footnotes
|
masenf
left a comment
There was a problem hiding this comment.
needs a changelog entry.
doesn't build
TypeError: Invalid var passed for prop SankeyChart.data, expected type <class
'reflex_components_recharts.charts.SankeyData'>, got value
reflex___state____state___docgen_exec___docs_library_graphing_charts_sankeychart_md____sankey_state
.data_rx_state_ of type collections.abc.Mapping[str,
collections.abc.Sequence[collections.abc.Mapping[str, int]] |
collections.abc.Sequence[collections.abc.Mapping[str, str]]].
While rendering demo block in docs/library/graphing/charts/sankeychart.md:
class SankeyState(rx.State):
data = {
"nodes": [
{"name": "Marketing"},
{"name": "Trial"},
{"name": "Sales"},
{"name": "Support"},
Happened while evaluating page 'library/graphing/charts/sankeychart'
|
Removed The earlier |
|
hey @masenf @FarhanAliRaza can someone look into this ? |
* Implement use_hook_var, use_chart_width, and use_id * Add memo-based decorator wrappers for `sankey_chart.node` and `sankey_chart.link` that do the needful to convert the decorated function into a custom component capable of being passed to sankey_chart as props. Updated the docs to show a proper custom link+node example; removed broken partial customization example (merged working pieces with the stateful example).
- Add reflex-base news entry for the new use_hook_var/use_id APIs (reflex-dev#6708) - Extend recharts news entry to mention custom node/link renderers and use_chart_width - Add unit tests for reflex_base.vars.special and rx.recharts.use_chart_width - Document hook vars in the var_system API reference and wrapping-react custom-code-and-hooks pages; point to the helpers from the sankey docs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QQXD9PDAbVVDieBfgSHiPX
Addresses #6708 (comment) Verified against the recharts@3.8.1 published types and runtime: the link payload is {...link, source: sourceNode, target: targetNode}, so source and target are resolved node payloads (not indices), thickness is dy (not width), and index exists only on the outer SankeyLinkProps. Also alias the Sankey TypedDicts at module level for the namespace attributes: the generated .pyi rendered 'X = X' class attrs as self-referential type aliases, which broke resolution of sankey_chart.SankeyLinkProps and friends under pyright (7 stub errors, now 0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QQXD9PDAbVVDieBfgSHiPX
There was a problem hiding this comment.
6 issues found across 15 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="docs/library/graphing/charts/sankeychart.md">
<violation number="1" location="docs/library/graphing/charts/sankeychart.md:155">
P2: Custom node labels keep SVG's default black fill and only add a gray outline, so they become hard to read in dark mode. Use `fill` to set the intended text color instead of `stroke`.</violation>
</file>
<file name="packages/reflex-base/src/reflex_base/vars/special.py">
<violation number="1" location="packages/reflex-base/src/reflex_base/vars/special.py:9">
P3: Direct `use_hook_var(..., _var_type=int)` calls lose their static result type because the return annotation is bare `Var`, despite returning a type-specific var at runtime. A generic/overloaded signature that links `_var_type` to `Var[T]` would preserve editor and Pyright type checking for this public helper.</violation>
</file>
<file name="packages/reflex-components-recharts/src/reflex_components_recharts/charts.py">
<violation number="1" location="packages/reflex-components-recharts/src/reflex_components_recharts/charts.py:621">
P3: Minor dead-code/robustness note: `sig.parameters[next(...)] is None` can never be true (a `Parameter` object is never `None`), so this clause is dead. The empty-signature case is already excluded by the preceding `len(sig.parameters) != 1` check. It also uses index-style `next(iter(...))` into the mapping unnecessarily; iterating the values directly is clearer.</violation>
<violation number="2" location="packages/reflex-components-recharts/src/reflex_components_recharts/charts.py:622">
P2: The decorator's runtime validation compares the decorated function's raw parameter annotation with `get_origin(...) is not Var`. When the caller's module uses `from __future__ import annotations` (common in Reflex apps, and where this is enforced on many user modules), `inspect.signature` returns the annotation as an unevaluated string, so `get_origin` returns `None` and the check always fails — the decorator raises `TypeError` at import time for a fully valid `node: rx.Var[SankeyNodeProps]` function, breaking the documented custom-renderer API with a confusing error. Consider resolving annotations (e.g. `typing.get_type_hints(fn, include_extras=True)`) before validating, or skipping the eager check and letting the memo machinery handle typing, so both evaluated and string-lazy annotations are accepted. The same issue exists in `sankey_link`.</violation>
<violation number="3" location="packages/reflex-components-recharts/src/reflex_components_recharts/charts.py:630">
P3: A valid single positional-only renderer parameter passes validation but then fails during memo creation with an unexpected-keyword `TypeError`. Either reject unsupported parameter kinds during validation or invoke positional-only callbacks positionally in both decorators.</violation>
<violation number="4" location="packages/reflex-components-recharts/src/reflex_components_recharts/charts.py:637">
P3: The newly added `sankey_link` decorator duplicates nearly all of `sankey_node`, including validation that already needs identical fixes in two places. A shared renderer-decorator helper parameterized by props type/name would prevent these paths from drifting.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Pushed
|
There was a problem hiding this comment.
5 issues found across 15 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="docs/library/graphing/charts/sankeychart.md">
<violation number="1" location="docs/library/graphing/charts/sankeychart.md:57">
P2: In the "Stateful Example", `SankeyState.data` is declared without a type annotation, so it is not a reactive Var (Reflex builds state vars from annotated fields). The `Randomize flows` button mutates that plain dict in place, which produces no backend state delta, so the chart will not update when clicked. Annotate `data` (e.g. `data: dict[str, Any]` or a `SankeyData`) so the mutation triggers a re-render and the example actually demonstrates the stateful behavior it claims.</violation>
<violation number="2" location="docs/library/graphing/charts/sankeychart.md:165">
P3: Link value labels retain the SVG default black fill, so they can become unreadable on dark backgrounds and render as outlined text elsewhere. This likely should use `fill=rx.color("gray", 12)` like the node labels.</violation>
</file>
<file name="packages/reflex-components-recharts/src/reflex_components_recharts/charts.py">
<violation number="1" location="packages/reflex-components-recharts/src/reflex_components_recharts/charts.py:11">
P3: Importing `_MemoComponentWrapper` makes the Recharts package depend on a private `reflex-base` implementation detail, so an internal memo refactor can break this package at import time. A public callable/protocol return type would avoid exposing that coupling in the Sankey decorator API.</violation>
<violation number="2" location="packages/reflex-components-recharts/src/reflex_components_recharts/charts.py:527">
P3: Custom `name_key`/`data_key` datasets are rejected by the new `SankeyData` type because `name` and `value` remain required default-key fields. Marking those two fields `NotRequired` would let the TypedDict model the alternate-key props exposed by `SankeyChart`.</violation>
<violation number="3" location="packages/reflex-components-recharts/src/reflex_components_recharts/charts.py:739">
P2: Using `rx.recharts.sankey_chart` as a component-style key raises `TypeError: unhashable type: 'SankeyNamespace'`, and it is not normalized by `evaluate_style_namespaces`. Inheriting from `ComponentNamespace` would preserve the standard callable-namespace styling behavior.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 15 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…ankey-6558 # Conflicts: # packages/reflex-components-recharts/src/reflex_components_recharts/__init__.py # packages/reflex-components-recharts/src/reflex_components_recharts/general.py # pyi_hashes.json
|
The failing check is test_call_script[inline], where external4 is not loaded. This PR does not modify the call- script or integration harness code, and the external-script variant passes. Could this be rerun or compared against main to confirm whether it is a baseline/flaky CI failure? |
Closes #6558
Summary
rx.recharts.SankeyChart/rx.recharts.sankey_chartTests
uv run pytest tests/units/components/graphing/test_recharts.py -quv run ruff check ...uv run ruff format --check ...uv run pyright ...