Skip to content

fix(recharts): support function vars for axis tick formatters - #7366

Open
harsh21234i wants to merge 8 commits into
reflex-dev:mainfrom
harsh21234i:fix/7257-recharts-function-tick-formatter
Open

harsh21234i wants to merge 8 commits into
reflex-dev:mainfrom
harsh21234i:fix/7257-recharts-function-tick-formatter

Conversation

@harsh21234i

@harsh21234i harsh21234i commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Allow Reflex function vars as Recharts axis tick formatters.
  • Reject dynamic string vars with guidance to use FunctionStringVar.
  • Add regression tests and a changelog entry.

Testing

  • Recharts cartesian unit tests: 18 passed.
  • scripts/make_pyi.py --check passed.
  • Ruff focused checks and git diff --check passed.

Fixes #7257

@harsh21234i
harsh21234i requested a review from a team as a code owner September 30, 2026 14:57

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files

You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/reflex-components-recharts/src/reflex_components_recharts/cartesian.py Outdated
@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Adds function variable support to chart axis tick formatters.

The PR appears safe to merge; the previously reported regression-test mismatch is fixed and no new issue was identified.

Summary

This PR allows Recharts axis tick formatters to accept Reflex function vars while rejecting dynamic string vars with guidance. It also adds regression tests and a changelog fragment.

Reviews (7) · Last reviewed commit: "update test assertion for new exception ..."

Comment thread packages/reflex-components-recharts/src/reflex_components_recharts/cartesian.py Outdated
Comment thread tests/units/components/recharts/test_cartesian.py Outdated
Comment thread packages/reflex-components-recharts/news/7366.bugfix.md
@codspeed

codspeed Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 147 untouched benchmarks
⏩ 17 skipped benchmarks1


Comparing harsh21234i:fix/7257-recharts-function-tick-formatter (38cff9c) with main (7b90fd3)

Open in CodSpeed

Footnotes

  1. 17 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Comment thread packages/reflex-components-recharts/src/reflex_components_recharts/cartesian.py Outdated
@masenf masenf added the on deck PRs lined up to review / merge next label Oct 1, 2026
masenf
masenf previously approved these changes Oct 1, 2026

@masenf masenf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment on lines +163 to +164
elif tick_formatter is not None:
raise TypeError(_TICK_FORMATTER_TYPE_ERROR)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this branch. It rejects a plain rx.Var with type Any, and that input works on main.

XAxis.create(tick_formatter=rx.Var("((v) => v)")) renders tickFormatter:((v) => v) on main. On this branch it raises TypeError: tick_formatter must be a FunctionVar. A user can use rx.Var("myFormatter") to point at a JS function from custom code, so this breaks working apps. Please also add a test for this case.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk about this suggestion. a Var could be anything, we don't want to take anything for this prop. The point of strong type hinting is that it makes it harder to represent a bad state. So while a subset of plain Var might work, there's a lot of Var objects that won't work and will give weird frontend errors as runtime.

I think it makes sense to enforce that this field is a plain literal string, or explicitly a FunctionVar

Comment on lines +157 to +162
elif isinstance(tick_formatter, FunctionVar):
props["tick_formatter"] = tick_formatter
elif isinstance(tick_formatter, Var) and tick_formatter._var_type is str:
raise TypeError(_TICK_FORMATTER_DYNAMIC_VAR_ERROR)
elif callable(tick_formatter) and not isinstance(tick_formatter, Var):
raise TypeError(_TICK_FORMATTER_CALLABLE_ERROR)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the FunctionVar branch and the callable branch. The framework already does both of these checks.

  • The FunctionVar branch puts back the value that is already in props. The new annotation accepts FunctionVar, so super().create() lets it through.
  • super().create() already raises TypeError for a Python lambda on main.

With only the str-Var check kept, FunctionStringVar, .partial(), ArgsFunctionOperation, a literal string and an Any Var all render correctly. A State str var is still rejected. _TICK_FORMATTER_CALLABLE_ERROR and _TICK_FORMATTER_TYPE_ERROR are then unused and can go too.

Comment on lines +159 to +160
elif isinstance(tick_formatter, Var) and tick_formatter._var_type is str:
raise TypeError(_TICK_FORMATTER_DYNAMIC_VAR_ERROR)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create() now raises TypeError here. Please add a Raises: section to the Axis.create docstring. The summary line talks only about string formatters, so please update it to describe function vars too.

@@ -0,0 +1,53 @@
import pytest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move these tests into tests/units/components/recharts/test_cartesian.py. That file already has the tick_formatter tests, including the tests that reject non-callables and Python callables.

@FarhanAliRaza FarhanAliRaza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this branch in a small Reflex app with four line charts. Each chart uses a different tick_formatter: a literal string, a FunctionStringVar, a FunctionStringVar.partial(State.suffix), and an ArgsFunctionOperation.

All four charts render the formatted ticks. A click that changes State.suffix updates the partial formatter's ticks. The browser console shows no errors.

I also called XAxis.create() with the same inputs on main and on this branch. On main, a FunctionStringVar raises the TypeError from #7257, and this branch fixes that. A State str var is now rejected, as the issue asks. A plain rx.Var with type Any works on main but raises on this branch.

The requested changes are in the inline comments.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on deck PRs lined up to review / merge next

Projects

None yet

3 participants