Polar zoom defaults to off; opt-in per chart with interaction_config - #390
Open
masenf wants to merge 6 commits into
Open
Polar zoom defaults to off; opt-in per chart with interaction_config#390masenf wants to merge 6 commits into
masenf wants to merge 6 commits into
Conversation
The centre of a disc is a fixed point of the polar transform and radial zoom pins the minimum, so zooming a composition whose radius is *layout* rather than data — a pie or donut (the value is the angle, the radius a constant rim), a radial bar, a gauge, a radar on a fixed frame — could only crop the rim while the geometry stayed welded to the middle. That reads as a broken chart, not as navigation. `coords="polar"` now resolves `zoom` to `False` and ships the flag explicitly. `wind_rose` is the exception the default is written around and ships `zoom=True`: its radius is a frequency count, so pulling the outer ring in magnifies the short sectors of a rose dominated by one prevailing direction. Either side is overridable with `xy.interaction_config(zoom=...)` or a `zoom=` chart prop, and the gesture itself is unchanged once enabled. The decision is made in Python rather than inferred by the renderers because `Chart.kind` never reaches the wire: a pie and a wind rose are the same figure to the client and want opposite answers. That makes this the one documented exception to pan-and-zoom-configuration.md §5.2's "unspecified keys stay absent" rule, and §28 wants the choice on the wire anyway. Two consequences are the point of the change rather than side effects. The wheel handler returns before `preventDefault()`, so a page scrolls normally over a pie or gauge instead of the chart swallowing the gesture. And the modebar grows no zoom menu: `canRecordHistory` now gates view history on the durable state actually being mutable, since an enabled `history` flag alone was otherwise enough to build a "100%" trigger over two permanently dead Back/Next items. Specs, the polar overview's Hover and Zoom section (including a live opt-in example), and every sibling polar guide that claimed radial zoom was available are updated to match.
Contributor
|
We've triggered an ultrareview automatically — This PR changes the default zoom behavior for all polar charts, affecting both the Python API and client-side interaction model across 20 files, which could break existing charts or cause unexpected behavior if a subtle bug is missed.. I'll post findings when complete. An ultrareview is cubic's deepest review, catching hard-to-find bugs in the most critical PRs. It runs a longer, multi-pass analysis using cubic's most capable review models, and typically takes around 30 minutes. It consumes your team's reviewed-lines allowance at 3× the standard rate. Automated ultrareviews are disabled by default. We triggered this run as part of your trial. Want cubic to do this for every high-risk PR? Enable auto-ultrareview in your settings. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPolar charts now disable zoom by default, while wind rose charts retain enabled zoom. Explicit chart or interaction settings override these defaults. Frontend controls, validation, documentation, and tests reflect the resolved behavior. ChangesPolar zoom behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ChartFactory
participant PythonInteraction
participant PolarPayload
participant BrowserInteraction
participant Modebar
ChartFactory->>PythonInteraction: resolve polar zoom setting
PythonInteraction->>PolarPayload: serialize zoom capability
PolarPayload->>BrowserInteraction: provide interaction payload
BrowserInteraction->>Modebar: create controls for enabled capabilities
BrowserInteraction-->>ChartFactory: apply radial zoom or preserve page scrolling
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/charts/polar-chart.md`:
- Around line 251-256: Update the polar-chart documentation to reflect the
resolved reset policy: with zoom disabled, polar charts must not describe reset
controls or double-click axis restoration as available; retain the documented
zoom-enabled behavior only if it matches the implementation, including wind-rose
exceptions.
In `@docs/charts/wind-rose.md`:
- Around line 119-125: Update the wind rose documentation to remove the claim
that other polar chart radii are layout-only. Keep the statement that other
polar chart types default to zoom off, and direct authors to use
xy.interaction_config(zoom=True) when their radial values are meaningful and
worth magnifying.
In `@js/src/53_interaction.ts`:
- Around line 1277-1279: Update the history eligibility logic around
canRecordHistory and canHistory so selection history remains available when
navigation is disabled; do not require canNavigate for selection-only
configurations, while preserving navigation and zoom gating behavior. Add a
regression test covering navigation=False with selection enabled, then run the
relevant core, JavaScript build, ABI, rendering, smoke, test, lint, formatting,
and type-check commands, excluding native-core fallback tests.
In `@python/xy/_figure.py`:
- Around line 1724-1740: Update the polar interaction validation around
_validate_interaction() so default_drag_action="zoom" is checked against the
resolved zoom value after polar defaults are applied, rejecting the incompatible
false combination while preserving wind_rose’s explicit zoom=True behavior. Add
a regression test that confirms the invalid configuration raises during
construction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ee50ab5b-4582-4afb-b15d-2ecb17e7c9ff
📒 Files selected for processing (20)
CHANGELOG.mddocs/api-reference/limitations-and-alpha-status.mddocs/app/tests/test_docs_site.pydocs/charts/pie-chart.mddocs/charts/polar-chart.mddocs/charts/radar-chart.mddocs/charts/radial-bar-chart.mddocs/charts/wind-rose.mddocs/components/modebars-and-interaction-controls.mddocs/core-concepts/interactions.mddocs/guides/display-and-export.mdjs/src/53_interaction.tspython/xy/_figure.pypython/xy/components.pyspec/api/chart-kind-contract.mdspec/api/interaction.mdspec/design/pan-and-zoom-configuration.mdspec/design/polar-axes.mdtests/test_polar_charts.pytests/test_polar_client_regressions.py
Comment on lines
+1277
to
+1279
| const canRecordHistory = canPan || canSelect | ||
| || (canZoom && this._axisPolicy("zoom_axes").length > 0); | ||
| const canHistory = canNavigate && this._historyEnabled() && canRecordHistory; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate file outline ---'
ast-grep outline js/src/53_interaction.ts --match 'class $_' --view compact 2>/dev/null || true
printf '%s\n' '--- target implementation ---'
sed -n '1220,1310p' js/src/53_interaction.ts
printf '%s\n' '--- navigation/history references ---'
rg -n -C 3 'canNavigate|canRecordHistory|canHistory|_historyEnabled|history|navigation' js/src/53_interaction.ts js/src js/test tests 2>/dev/null | head -n 500
printf '%s\n' '--- relevant files ---'
git ls-files | rg '(^|/)(53_interaction|interaction|history|navigation|select|brush|render|test)' | head -n 300Repository: reflex-dev/xy
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target implementation ---'
sed -n '1220,1310p' js/src/53_interaction.ts
printf '%s\n' '--- relevant symbols and tests ---'
rg -n -C 4 'canNavigate|canRecordHistory|canHistory|_historyEnabled|history|navigation' js/src js/test tests 2>/dev/null | head -n 500
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(53_interaction|interaction|history|navigation|select|brush|render|test)' | head -n 300Repository: reflex-dev/xy
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- interaction contract and history documentation ---'
rg -n -C 5 'navigation|history|selection.*history|history.*selection|brush.*history' \
spec docs python tests --glob '!tests/pyplot/**' --glob '!tests/**/corpus/**' | head -n 350
printf '%s\n' '--- focused test filenames ---'
git ls-files tests js | rg -i '(toolbar|interaction|view.?state|history|select|brush|modebar)'
printf '%s\n' '--- focused test references ---'
rg -n -C 5 'history|navigation|modebar|selection' tests/test_toolbar_controls.py tests/test_interaction_probe_retries.py tests/test_modebar_select_drill.py tests/test_boxzoom_tool_keeps_wheel_and_reset.py tests/reflex_adapter/test_view_state_push.py 2>/dev/null | head -n 400
printf '%s\n' '--- interaction flag definitions/defaults ---'
rg -n -C 6 '_interactionFlag|INTERACTION|navigation.*true|history.*true' js/src python/xy spec/api/interaction.md | head -n 400Repository: reflex-dev/xy
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- interaction contract and history documentation ---'
rg -n -C 5 'navigation|history|selection.*history|history.*selection|brush.*history' \
spec docs python tests --glob '!tests/pyplot/**' --glob '!tests/**/corpus/**' | head -n 350
printf '%s\n' '--- focused test filenames ---'
git ls-files tests js | rg -i '(toolbar|interaction|view.?state|history|select|brush|modebar)'
printf '%s\n' '--- focused test references ---'
rg -n -C 5 'history|navigation|modebar|selection' \
tests/test_toolbar_controls.py tests/test_interaction_probe_retries.py \
tests/test_modebar_select_drill.py tests/test_boxzoom_tool_keeps_wheel_and_reset.py \
tests/reflex_adapter/test_view_state_push.py 2>/dev/null | head -n 400
printf '%s\n' '--- interaction flag definitions/defaults ---'
rg -n -C 6 '_interactionFlag|INTERACTION|navigation.*true|history.*true' \
js/src python/xy spec/api/interaction.md | head -n 400Repository: reflex-dev/xy
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- focused contract excerpts ---'
sed -n '45,65p' spec/api/interaction.md
sed -n '286,302p' spec/api/interaction.md
sed -n '344,354p' spec/api/interaction.md
printf '%s\n' '--- focused view-state contract ---'
rg -n -C 8 'history|selection|navigation' spec/design/view-state.md | head -n 220
printf '%s\n' '--- standalone invariant verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
src = Path("js/src/53_interaction.ts").read_text()
viewstate = Path("js/src/57_viewstate.ts").read_text()
contract = Path("spec/api/interaction.md").read_text()
assert re.search(
r'const canSelect = this\._interactionFlag\("brush", true\)\s*'
r'&& this\._interactionFlag\("select", true\);',
src,
)
assert re.search(
r'const canRecordHistory = canPan \|\| canSelect\s*'
r'\|\| \(canZoom && this\._axisPolicy\("zoom_axes"\)\.length > 0\);',
src,
)
history_line = re.search(r'const canHistory = ([^;]+);', src).group(1)
assert "this._historyEnabled()" in history_line
assert "canRecordHistory" in history_line
assert "canNavigate" in history_line
# Evaluate the relevant capability matrix using the implementation's boolean
# structure, without importing or executing repository code.
def current_can_history(navigation, select, history, pan=False, zoom=False, zoom_axes=False):
can_pan = navigation and pan
can_zoom = navigation and zoom
can_record = can_pan or select or (can_zoom and zoom_axes)
return navigation and history and can_record
def intended_can_history(navigation, select, history, pan=False, zoom=False, zoom_axes=False):
can_pan = navigation and pan
can_zoom = navigation and zoom
can_record = can_pan or select or (can_zoom and zoom_axes)
return history and can_record
case = dict(navigation=False, select=True, history=True)
print("case:", case)
print("current:", current_can_history(**case))
print("selection-independent:", intended_can_history(**case))
assert current_can_history(**case) is False
assert intended_can_history(**case) is True
# Selection itself is explicitly independent of navigation in the documented
# gesture contract, while history is documented as a durable-state feature.
assert "`navigation` | on | Master gate" in contract
assert "`brush`, `select`, `_pickable`" in contract
assert "history" in contract
assert "this._historyEnabled()" in viewstate
assert "if (!this._historyPast || !this._historyEnabled()" in viewstate
print("invariant: navigation suppresses the history UI despite selectable durable state")
PYRepository: reflex-dev/xy
Length of output: 19038
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
src = Path("js/src/53_interaction.ts").read_text()
viewstate = Path("js/src/57_viewstate.ts").read_text()
contract = Path("spec/api/interaction.md").read_text()
assert re.search(
r'const canSelect = this\._interactionFlag\("brush", true\)\s*'
r'&& this\._interactionFlag\("select", true\);',
src,
)
assert re.search(
r'const canRecordHistory = canPan \|\| canSelect\s*'
r'\|\| \(canZoom && this\._axisPolicy\("zoom_axes"\)\.length > 0\);',
src,
)
history_line = re.search(r'const canHistory = ([^;]+);', src).group(1)
assert "this._historyEnabled()" in history_line
assert "canRecordHistory" in history_line
assert "canNavigate" in history_line
def current(navigation, select, history, pan=False, zoom=False, zoom_axes=False):
can_pan = navigation and pan
can_zoom = navigation and zoom
can_record = can_pan or select or (can_zoom and zoom_axes)
return navigation and history and can_record
def intended(navigation, select, history, pan=False, zoom=False, zoom_axes=False):
can_pan = navigation and pan
can_zoom = navigation and zoom
can_record = can_pan or select or (can_zoom and zoom_axes)
return history and can_record
case = dict(navigation=False, select=True, history=True)
print("case:", case)
print("current:", current(**case))
print("selection-independent:", intended(**case))
assert current(**case) is False
assert intended(**case) is True
assert "this._historyEnabled()" in viewstate
assert "if (!this._historyPast || !this._historyEnabled()" in viewstate
assert "`brush`, `select`, `_pickable`" in contract
print("invariant: navigation suppresses the history UI despite selectable durable state")
PYRepository: reflex-dev/xy
Length of output: 336
Do not gate selection history on navigation.
navigation=False still permits selection, so this gate hides Back/Next for selection-only history configurations.
Fix
- const canHistory = canNavigate && this._historyEnabled() && canRecordHistory;
+ const canHistory = this._historyEnabled() && canRecordHistory;Add a regression test and run the relevant core, JavaScript build, ABI, rendering, smoke, test, lint, formatting, and type-check commands without native-core fallback tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const canRecordHistory = canPan || canSelect | |
| || (canZoom && this._axisPolicy("zoom_axes").length > 0); | |
| const canHistory = canNavigate && this._historyEnabled() && canRecordHistory; | |
| const canRecordHistory = canPan || canSelect | |
| || (canZoom && this._axisPolicy("zoom_axes").length > 0); | |
| const canHistory = this._historyEnabled() && canRecordHistory; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@js/src/53_interaction.ts` around lines 1277 - 1279, Update the history
eligibility logic around canRecordHistory and canHistory so selection history
remains available when navigation is disabled; do not require canNavigate for
selection-only configurations, while preserving navigation and zoom gating
behavior. Add a regression test covering navigation=False with selection
enabled, then run the relevant core, JavaScript build, ABI, rendering, smoke,
test, lint, formatting, and type-check commands, excluding native-core fallback
tests.
Source: Coding guidelines
Contributor
Author
There was a problem hiding this comment.
Skipping — the canNavigate term is pre-existing and unchanged by this PR. Before: const canHistory = canNavigate && this._historyEnabled();. My edit only appended && canRecordHistory, which narrows further; it did not introduce the navigation gate.
The underlying observation is fair (§5.3 does say navigation=False leaves selection working, so selection-only history could in principle record), but dropping canNavigate here has a consequence beyond this PR's scope: history lives inside the zoom menu, whose trigger renders a "100%" zoom-percentage label. Removing the gate would put a zoom indicator on a chart that cannot zoom — the same dead-affordance problem canRecordHistory exists to remove, just inverted. Fixing it properly means deciding whether view history belongs in the zoom menu at all for a navigation-disabled chart, which is a separate change to non-polar behavior.
Filing it as a follow-up rather than folding it in here.
Generated by Claude Code
There was a problem hiding this comment.
@masenf Understood. The canNavigate gate predates this PR. This PR only adds canRecordHistory to prevent empty history controls for polar charts with zoom disabled.
Removing canNavigate would require a separate design for selection-only history controls. The current zoom-menu trigger would otherwise show a misleading zoom percentage. I will not pursue this finding in this PR.
🐇
✏️ Learnings added
Learnt from: masenf
Repo: reflex-dev/xy PR: 390
File: js/src/53_interaction.ts:1277-1279
Timestamp: 2026-07-31T17:25:42.743Z
Learning: In `js/src/53_interaction.ts`, `canHistory` is intentionally gated by `canNavigate` because Back and Next controls are currently hosted in the zoom menu, whose trigger displays a zoom-percentage label. Supporting selection-only history when `navigation=False` requires a separate UI design for history controls and is outside polar zoom changes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
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}
Contributor
There was a problem hiding this comment.
Ultrareview completed in 10m 37s
1 issue found across 20 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/xy/components.py">
<violation number="1" location="python/xy/components.py:6865">
P2: Default wind roses expose Box Zoom and resolve plain drag to box zoom because this enables the generic `zoom` capability while `box_zoom` still defaults to true. Box rectangles are explicitly unsupported for polar charts, so the wind-rose exception should retain radial wheel/button zoom without restoring that unsupported drag mode.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| # sectors of a rose dominated by one prevailing direction). `setdefault`, so | ||
| # an author's own `zoom=` — or an `xy.interaction_config(zoom=False)` child, | ||
| # which is applied after chart props — still wins. | ||
| props.setdefault("zoom", True) |
Contributor
There was a problem hiding this comment.
P2: Default wind roses expose Box Zoom and resolve plain drag to box zoom because this enables the generic zoom capability while box_zoom still defaults to true. Box rectangles are explicitly unsupported for polar charts, so the wind-rose exception should retain radial wheel/button zoom without restoring that unsupported drag mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/xy/components.py, line 6865:
<comment>Default wind roses expose Box Zoom and resolve plain drag to box zoom because this enables the generic `zoom` capability while `box_zoom` still defaults to true. Box rectangles are explicitly unsupported for polar charts, so the wind-rose exception should retain radial wheel/button zoom without restoring that unsupported drag mode.</comment>
<file context>
@@ -6842,6 +6853,16 @@ def wind_rose(
+ # sectors of a rose dominated by one prevailing direction). `setdefault`, so
+ # an author's own `zoom=` — or an `xy.interaction_config(zoom=False)` child,
+ # which is applied after chart props — still wins.
+ props.setdefault("zoom", True)
return Chart("wind_rose", children, **props)
</file context>
Suggested change
| props.setdefault("zoom", True) | |
| props.setdefault("zoom", True) | |
| props.setdefault("box_zoom", False) |
Contributor
Author
There was a problem hiding this comment.
Not reproducible — the premise doesn't hold. _interactionFlag in 50_chartview.ts forces box_zoom, select, brush, and crosshair to false under coords="polar" regardless of the flag values, so enabling the generic zoom capability cannot restore box zoom. I probed a default wind rose in headless Chromium:
boxZoomFlag: false, boxZoomItem: false, panButton: false, dragMode: "none",
zoomFlag: true, zoomAxes: ["y"], zoomIn: true, resetItem: true
No Box Zoom item, no Pan button, and plain drag resolves to none — dragMode === "zoom" also can't engage because that branch requires canBoxZoom. Wind roses already had zoom effectively on before this PR, with the same result. props.setdefault("box_zoom", False) would be dead config.
Locked in as test_wind_rose_keeps_radial_zoom_without_restoring_box_zoom in tests/test_polar_client_regressions.py (b844f51), which asserts exactly this split: radial zoom + reset granted, every rectangle gesture withheld.
Generated by Claude Code
Merged
Validation and the payload disagreed about the polar zoom default:
`_validate_interaction` read an absent `zoom` as enabled while
`_interaction_spec` resolved it to False, so
`polar_chart(..., default_drag_action="zoom")` passed construction and
then shipped the self-contradicting
`{"zoom": false, "default_drag_action": "zoom"}`. Both now go through one
`Figure._zoom_enabled` predicate, so that combination raises at
construction. Two neighbouring holes are older than this rule and are left
alone, recorded in polar-axes.md §8: with zoom explicitly True, polar
still accepts `default_drag_action="zoom"`, and it accepts `"pan"` in any
case, though the client forces `box_zoom` off and `pan_axes` to `[]` for
polar.
`wind_rose(..., zoom=None)` disabled zoom instead of keeping the rose's
default. `None` means "unset" everywhere else in the interaction API, so
a wrapper forwarding an `Optional[bool]` turned zoom off by passing the
value that means "no opinion". Only a real bool now overrides.
Docs: reset follows the zoom capability, since the resolved reset-axis
policy is pan axes union zoom axes and both are empty under polar with
zoom off — Fit Data / Reset View do not render and double-click restores
nothing. Every polar guide that promised reset as part of the default
contract is corrected. The wind rose page no longer says other polar
radii are "layout rather than data", which contradicted the overview's own
opt-in example (a measured `polar_chart` radius is the expected opt-in
case); the rationale is the fixed centre, not the data. The
limitations page names the `zoom=` chart prop alongside
`interaction_config`, and the export guide names the wind-rose exception.
Tests: the resolved-zoom refusal, `zoom=None` on a rose, and a browser
probe asserting the wind-rose exception grants radial zoom and reset
WITHOUT restoring box zoom, the Pan button, selection, or a `zoom` drag
mode — the client forces every rectangle gesture off under polar
regardless of the flag, so the generic `zoom` capability cannot leak them
back. Docs contract tests pin the corrected claims and guard the removed
ones.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/charts/polar-chart.md`:
- Around line 251-257: Qualify the default-only reset and zoom claims in
docs/charts/polar-chart.md (251-257), docs/charts/pie-chart.md (676-683),
docs/charts/radar-chart.md (138-147), and
docs/components/modebars-and-interaction-controls.md (26-36) to state that an
explicit reset_axes policy still enables Reset View and double-click reset when
zoom is off. Update spec/design/polar-axes.md (577-581) to state that the
resolved reset policy is empty only when reset_axes is not supplied.
In `@spec/design/polar-axes.md`:
- Around line 530-538: Update polar configuration validation to reject both
default_drag_action values "pan" and "zoom" whenever coords is "polar",
including when zoom is explicitly true. Keep validation for other coordinate
systems unchanged and align the accepted actions with the inert polar pan_axes
and box_zoom behavior described near Figure._zoom_enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6fdf314-86bd-47c1-bb46-6da31fc5d8f6
📒 Files selected for processing (16)
CHANGELOG.mddocs/api-reference/limitations-and-alpha-status.mddocs/app/tests/test_docs_site.pydocs/charts/pie-chart.mddocs/charts/polar-chart.mddocs/charts/radar-chart.mddocs/charts/radial-bar-chart.mddocs/charts/wind-rose.mddocs/components/modebars-and-interaction-controls.mddocs/core-concepts/interactions.mddocs/guides/display-and-export.mdpython/xy/_figure.pypython/xy/components.pyspec/design/polar-axes.mdtests/test_polar_charts.pytests/test_polar_client_regressions.py
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/guides/display-and-export.md
- tests/test_polar_client_regressions.py
- CHANGELOG.md
- docs/api-reference/limitations-and-alpha-status.md
- docs/charts/radial-bar-chart.md
- tests/test_polar_charts.py
- docs/app/tests/test_docs_site.py
- python/xy/components.py
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 16 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
`default_drag_action` now accepts only "auto"/"none" under coords="polar". A disc has no drag tools at all — the client resolves `pan_axes` to `[]` and forces `box_zoom`/`select`/`brush` off whatever the flags say — so "pan", "zoom", and the four `select*` values were each accepted and then resolved to no usable tool, including with `zoom=True`, since radial zoom is a wheel/button gesture rather than a drag. This closes the family the previous commit narrowed to `zoom` alone and left recorded as a known hole. Reset is not unconditionally absent, which four pages and the spec claimed. `_resetAxisPolicy` returns an authored `reset_axes` verbatim instead of deriving pan-axes union zoom-axes, so Fit Data / Reset View render and double-click restores those axes even with zoom off — verified in Chromium. The hatch is not vestigial: a polar view can still move through linked axes or application-driven updates, which `navigation` does not gate, so an authored `reset_axes` is the right way to offer reset on a gesture-free chart. Every default-only claim is qualified. Two smaller doc corrections. The interactions page said a radial bar's or radar's radius is not the value — it is; the reason zoom is a bad default there is that the extent is read against an authored rim or shared domain that zoom clips away, so the rationale is reframed around the fixed frame rather than around the radius not being data. And the pie page told readers `zoom=True` restores the reset controls, when those examples also pass `xy.modebar(show=False)`; it now separates restoring double-click from unhiding the modebar.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/charts/pie-chart.md`:
- Around line 680-683: Clarify the interaction guidance in
docs/charts/pie-chart.md lines 680-683 and docs/charts/radial-bar-chart.md lines
685-689: state that double-click reset is inert only when reset_axes is absent,
and document that explicitly authoring reset_axes restores reset behavior in
both guides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba96b030-8042-4ab9-b05a-22e0552b1130
📒 Files selected for processing (12)
CHANGELOG.mddocs/api-reference/limitations-and-alpha-status.mddocs/app/tests/test_docs_site.pydocs/charts/pie-chart.mddocs/charts/polar-chart.mddocs/charts/radar-chart.mddocs/charts/radial-bar-chart.mddocs/components/modebars-and-interaction-controls.mddocs/core-concepts/interactions.mdpython/xy/_figure.pyspec/design/polar-axes.mdtests/test_polar_charts.py
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/api-reference/limitations-and-alpha-status.md
- CHANGELOG.md
- docs/charts/radar-chart.md
- docs/app/tests/test_docs_site.py
- docs/charts/polar-chart.md
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…tion list Four small corrections from review. The polar drag-action diagnostic prescribed `interaction_config(zoom=True)` as the remedy, which is inapplicable on the two paths where zoom is already enabled (`wind_rose`, or an explicit `zoom=True`) — and enabling zoom never makes a drag action valid anyway, since radial zoom is a wheel/button gesture. It now describes zoom as a separate capability instead of prescribing an action. `test_polar_refuses_every_inert_drag_action` hand-listed the refused actions and had already drifted: it omitted `select-y`. The list is now derived from the public `DefaultDragAction` type minus `auto`/`none`, with a guard test pinning the derivation, so a newly added drag action is covered the moment it exists. Three guides stated that double-click reset is inert on a zoom-off polar chart without scoping it to the case the claim actually holds for. An authored `reset_axes` grants reset — controls and double-click alike — whatever the zoom switch says, and the interactions page contradicted itself by noting that exception two sentences later. Every statement is now scoped to the derived/default policy.
`test_client_source_is_the_installed_bundle` looked for `class ChartView`,
`function decodeFrame(` and `function renderStandalone(` in
`python/xy/static/index.js`. `node js/build.mjs` minifies that bundle (§33),
so those identifiers are mangled and the markers could only ever match an
unminified build — the test failed on any correctly built checkout.
It now asserts the bundle exports every name `XYChart.jsx` imports from
`./xy_client.js`, with the import list read out of the wrapper rather than
duplicated, so the adapter and the client cannot drift apart silently. That
is also what this test is actually for: export aliases are the one part of
the bundle minification preserves, and they are precisely the surface the
wrapper binds to.
The export names are parsed from the `export{...}` clause instead of
substring-matched. `"as render" in text` is satisfied by
`as renderStandalone`, so dropping the anywidget `render` entry point would
have gone unnoticed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/reflex_adapter/test_assets.py (1)
55-61: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winParse aliased imports before comparing export names.
Although the current import is unaliased, valid ESM such as
renderStandalone as renderwould be stored as"renderStandalone as render". That value cannot match the exported source name. Parse each import entry and compare the identifier beforeas.Proposed fix
- imported = {name.strip() for name in clause.group(1).split(",") if name.strip()} + imported = { + entry.strip().rsplit(" as ", 1)[0].strip() + for entry in clause.group(1).split(",") + if entry.strip() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/reflex_adapter/test_assets.py` around lines 55 - 61, Update the import parsing in the XYChart.jsx validation around the imported set so aliased ESM entries are normalized to their source identifier before comparison. For each comma-separated import, remove the optional “as” alias and retain the name before it, while preserving existing handling for unaliased imports and the empty-import assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/reflex_adapter/test_assets.py`:
- Around line 58-62: Update the required export set in the test around imported
and exported names to explicitly include the anywidget render entry point, while
preserving the existing JSX import validation. Ensure the missing-export
assertion fails when render is absent from the client bundle.
---
Nitpick comments:
In `@tests/reflex_adapter/test_assets.py`:
- Around line 55-61: Update the import parsing in the XYChart.jsx validation
around the imported set so aliased ESM entries are normalized to their source
identifier before comparison. For each comma-separated import, remove the
optional “as” alias and retain the name before it, while preserving existing
handling for unaliased imports and the empty-import assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ca0a702-2d1c-45a8-87c1-61dd7132b11b
📒 Files selected for processing (1)
tests/reflex_adapter/test_assets.py
Comment on lines
+58
to
+62
| imported = {name.strip() for name in clause.group(1).split(",") if name.strip()} | ||
| assert imported, "XYChart.jsx imports no names from the client bundle" | ||
|
|
||
| missing = sorted(imported - exported) | ||
| assert not missing, f"client bundle does not export {missing}, which XYChart.jsx imports" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the required anywidget render export.
The check derives required names only from the imports in python/reflex-xy/reflex_xy/assets/XYChart.jsx. Those imports contain ChartView, decodeFrame, and renderStandalone, but not render. A bundle can therefore lose the required anywidget entry point while this test still passes. Add render to the required export set.
Proposed fix
- missing = sorted(imported - exported)
+ required = imported | {"render"}
+ missing = sorted(required - exported)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| imported = {name.strip() for name in clause.group(1).split(",") if name.strip()} | |
| assert imported, "XYChart.jsx imports no names from the client bundle" | |
| missing = sorted(imported - exported) | |
| assert not missing, f"client bundle does not export {missing}, which XYChart.jsx imports" | |
| imported = {name.strip() for name in clause.group(1).split(",") if name.strip()} | |
| assert imported, "XYChart.jsx imports no names from the client bundle" | |
| required = imported | {"render"} | |
| missing = sorted(required - exported) | |
| assert not missing, f"client bundle does not export {missing}, which XYChart.jsx imports" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/reflex_adapter/test_assets.py` around lines 58 - 62, Update the
required export set in the test around imported and exported names to explicitly
include the anywidget render entry point, while preserving the existing JSX
import validation. Ensure the missing-export assertion fails when render is
absent from the client bundle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Polar charts now ship with zoom disabled by default, with
wind_roseas the sole exception. This addresses the fundamental mismatch between zoom semantics and polar compositions: the center is a fixed point, so zooming a pie, radial bar, gauge, or radar crops the rim rather than navigating it. Leaving zoom off also releases the wheel to the page, so scrolling works normally over embedded charts.Key Changes
Python API and defaults:
Figure._interaction_spec()resolveszoomtoFalseundercoords="polar"when unspecified, and ships the flag explicitly on the wire (breaking the usual "unspecified keys stay absent" rule because the client cannot re-derive it —Chart.kindnever reaches the payload).wind_rose()opts back in by shippingzoom=Truein its interaction spec.interaction_config(zoom=True)and thezoom=chart prop both override the default, allowing authors to enable zoom when the radius is a measured quantity.interaction_config(),polar_chart(), and related factories to document the new default and escape hatch.Client-side (TypeScript):
canZoom(not justcanHistory), so a polar chart with zoom off shows no zoom controls, zoom percentage, or Back/Next history — because nothing can move the view.preventDefault()when zoom is off, leaving the event uncancelled so the page scrolls normally.Tests and documentation:
test_polar_zoom_is_off_by_default_and_releases_page_scroll()verifies the three user-visible consequences: radial range unchanged, wheel event uncancelled, modebar zoom controls absent.test_polar_wheel_zoom_is_alive_and_keeps_r_lo_fixed()to explicitly opt into zoom withxy.interaction_config(zoom=True).test_polar_ships_zoom_disabled_by_default()covers all polar chart factories.wind_rosekeeping zoom on and forinteraction_configoverriding the default.Implementation Details
The decision to resolve
zoomin Python (rather than infer it in the renderers) is structural: a pie and a wind rose are identical figures to the client, so the choice must be on the wire. This is the one documented exception to the "unspecified keys stay absent" rule inpan-and-zoom-configuration.md§5.2.Radial zoom always keeps
r_minfixed, so an ordinary zoom never turns a disc into an annulus. Values above the zoomedr_maxare culled in the shader, so zooming in on a chart whose interesting structure sits at the rim hides it; authors can reach forr_axis(type_="log")when a wide radial range needs to be readable at every scale.https://claude.ai/code/session_01KkjqqbCGF1AEdsMiuRqRGM
Summary by CodeRabbit
New Features
Bug Fixes
Documentation