Center resolved Y-axis titles in the browser [codex bughunt] - #356
Conversation
📝 WalkthroughWalkthroughY-axis title rendering now shares placement logic across primary and named axes, using measured tick geometry and fallback spacing. New SVG and browser regression tests validate centering, rotation, inward spacing, and tickless-axis behavior. ChangesY-axis title layout
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Cohere visual acceptanceMatplotlib reference:
XY static renderer target (already centered; the browser renderer now uses the same resolved Y-title anchor):
This regression exists only in the interactive browser fallback, so I am not presenting the static image as a browser before/after. The focused browser geometry tests assert the actual fix for default primary placement, named secondary axes, both sides, start/center/end, explicit rotation and label padding, tickless axes, and SVG longitudinal consistency. @coderabbitai review |
|
The Comment image 1 (Matplotlib reference) shows two vertically stacked charts: a time-series plot from 0.00–2.00 seconds with the vertical Y-axis title “s1 and s2”, and a coherence plot over frequency with the vertical Y-axis title “Coherence” centered alongside the lower plot. The Comment image 2 (XY static renderer target) shows the corresponding two charts with matching data, layout, and centered vertical Y-axis labels—especially “Coherence” on the lower panel—consistent with the stated static-renderer target.
✅ Action performedReview finished.
|
8091a4b to
a705bf1
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
js/src/50_chartview.ts (1)
5952-5995: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffBatch tick/title measurement reads before writes across all y-axes.
Each
renderYTitle→attachYTitleToTickscall performs its owngetBoundingClientRect()reads immediately followed by atitle.style.leftwrite, once per axis (primary + every named y-axis). Interleaving reads and writes like this across theextraYAxesloop forces the browser to re-flow between each axis's title placement instead of measuring all axes first and writing once. This runs on every_drawChrome()pass, including non-animated hover redraws (updateLabelsistruewhenever there's no active_viewAnim), so charts with several outside y-axis titles pay repeated layout thrashing on frequent interaction frames.Consider splitting
attachYTitleToTicksinto a measure phase (collect{title, targetLeft}for every axis) and an apply phase (write allstyle.leftvalues after all measurements complete) to keep reads and writes batched.🤖 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/50_chartview.ts` around lines 5952 - 5995, The y-axis title placement currently interleaves getBoundingClientRect reads and style.left writes across axes, causing repeated layout recalculation. Refactor attachYTitleToTicks and the renderYTitle flow so each eligible title first collects its measured targetLeft, including primary and extra y-axes, then applies all title.style.left writes after measurement is complete; preserve structured placement behavior where placement.style is non-null.
🤖 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.
Nitpick comments:
In `@js/src/50_chartview.ts`:
- Around line 5952-5995: The y-axis title placement currently interleaves
getBoundingClientRect reads and style.left writes across axes, causing repeated
layout recalculation. Refactor attachYTitleToTicks and the renderYTitle flow so
each eligible title first collects its measured targetLeft, including primary
and extra y-axes, then applies all title.style.left writes after measurement is
complete; preserve structured placement behavior where placement.style is
non-null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c24b6ff-f902-4c61-b1ea-c73cd1174533
📒 Files selected for processing (2)
js/src/50_chartview.tstests/test_ui_issue_regressions.py
a705bf1 to
7609729
Compare
The merge-base changed after approval.
7609729 to
93aad1c
Compare
|
Addressed the two remaining issues on the rebased head:
The existing failing assertion was intentionally kept unchanged. TypeScript, the non-browser client build, focused static/layout tests, full pre-commit, Ruff, formatting, and @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_ui_issue_regressions.py (1)
44-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate chart fixture between the two SVG/DOM parity tests.
The
xy.chart(...)construction (4 lines, 4 y-axes, marks, sizing) is byte-for-byte identical betweentest_svg_y_axis_title_locations_center_primary_and_named_axesandtest_y_axis_titles_center_and_match_svg_longitudinally_for_named_axes. Since these two tests exist specifically to cross-check that browser-rendered geometry matches the static SVG for the same configuration, keeping them in sync via a shared helper avoids silent drift if one copy is edited without the other.♻️ Suggested extraction
+_NAMED_AXES_LABELS = {"primary default", "right start", "left center", "right end"} + + +def _named_axes_chart() -> xy.Chart: + return xy.chart( + xy.line([0, 1], [0, 1]), + xy.line([0, 1], [1, 2], y_axis="y2"), + xy.line([0, 1], [2, 3], y_axis="y3"), + xy.line([0, 1], [3, 4], y_axis="y4"), + xy.x_axis(), + xy.y_axis(label="primary default", side="left"), + xy.y_axis(id="y2", label="right start", side="right", label_position="start"), + xy.y_axis(id="y3", label="left center", side="left", label_position="center"), + xy.y_axis(id="y4", label="right end", side="right", label_position="end"), + width=720, + height=440, + padding=(48, 120, 48, 120), + )Then both tests replace their inline construction with
chart = _named_axes_chart()andlabels = _NAMED_AXES_LABELS.Also applies to: 665-678
🤖 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/test_ui_issue_regressions.py` around lines 44 - 57, Extract the identical xy.chart construction from test_svg_y_axis_title_locations_center_primary_and_named_axes and test_y_axis_titles_center_and_match_svg_longitudinally_for_named_axes into a shared _named_axes_chart() helper, preserving all marks, axes, dimensions, and padding. Define the shared _NAMED_AXES_LABELS value for the expected labels, then update both tests to use chart = _named_axes_chart() and labels = _NAMED_AXES_LABELS.
🤖 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 `@js/src/50_chartview.ts`:
- Around line 5975-5990: Update the y-axis room calculation in _yAxisLeftRoom to
use the same computed title font-size source as the axis-title gap calculation,
rather than label_size for the 0.4 multiplier. Reuse the title element’s
getComputedStyle(...).fontSize value and preserve the existing fallback behavior
so reserved space and title positioning stay consistent when axis_title is
styled by CSS.
---
Nitpick comments:
In `@tests/test_ui_issue_regressions.py`:
- Around line 44-57: Extract the identical xy.chart construction from
test_svg_y_axis_title_locations_center_primary_and_named_axes and
test_y_axis_titles_center_and_match_svg_longitudinally_for_named_axes into a
shared _named_axes_chart() helper, preserving all marks, axes, dimensions, and
padding. Define the shared _NAMED_AXES_LABELS value for the expected labels,
then update both tests to use chart = _named_axes_chart() and labels =
_NAMED_AXES_LABELS.
🪄 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: 3f3b1104-a948-4158-ac5c-9e9c36b5a275
📒 Files selected for processing (2)
js/src/50_chartview.tstests/test_ui_issue_regressions.py
| const fontSize = parseFloat(getComputedStyle(title).fontSize) || 12; | ||
| const labelOffset = Number(axis.label_offset || 0); | ||
| const targetEdge = onRight | ||
| ? Math.max(...tickRects.map((rect) => rect.right)) + 0.4 * fontSize + labelOffset | ||
| : Math.min(...tickRects.map((rect) => rect.left)) - 0.4 * fontSize - labelOffset; | ||
| const rawOffset = axis.label_offset; | ||
| const gap = rawOffset !== undefined | ||
| && rawOffset !== null | ||
| && Number.isFinite(Number(rawOffset)) | ||
| ? Number(rawOffset) | ||
| : 0.4 * fontSize; | ||
| // Matplotlib centers the title along the axis, then offsets it | ||
| // perpendicular to the union of the tick-label bounds and the | ||
| // corresponding spine. Including the spine keeps inward/negative-pad | ||
| // labels from pulling an outside title back into the plot. | ||
| const spineEdge = root.left + (onRight ? p.x + p.w : p.x); | ||
| const tickEdge = onRight | ||
| ? Math.max(spineEdge, ...tickRects.map((rect) => rect.right)) | ||
| : Math.min(spineEdge, ...tickRects.map((rect) => rect.left)); | ||
| const targetEdge = onRight ? tickEdge + gap : tickEdge - gap; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the axis_title slot defaults and any font-size declarations that could
# diverge from the spec's label_size.
fd -e ts -e css . js/src | xargs rg -n -C3 'axis_title'
rg -n -C3 'label_size' js/srcRepository: reflex-dev/xy
Length of output: 6460
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the y-axis room reservation and title rendering paths together.
sed -n '690,790p' js/src/50_chartview.ts
printf '\n---\n'
sed -n '5960,6065p' js/src/50_chartview.ts
printf '\n---\n'
sed -n '175,190p' js/src/20_theme.tsRepository: reflex-dev/xy
Length of output: 12346
Keep the reserved y-axis room on the same font-size source as the title gap. The attachment pass uses getComputedStyle(title).fontSize, but _yAxisLeftRoom still reserves 0.4 * label_size. Since axis_title can be overridden by CSS, those two can diverge and leave the title clamped tighter than authored.
🤖 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/50_chartview.ts` around lines 5975 - 5990, Update the y-axis room
calculation in _yAxisLeftRoom to use the same computed title font-size source as
the axis-title gap calculation, rather than label_size for the 0.4 multiplier.
Reuse the title element’s getComputedStyle(...).fontSize value and preserve the
existing fallback behavior so reserved space and title positioning stay
consistent when axis_title is styled by CSS.
2 participants
Summary
translate(-50%, -50%) rotate(...)and a centered transform originlabel_positionCSS authoritativeThis is a general renderer fix for a consolidation regression, not a Cohere special case. It ports the relevant placement model from
8cd0c9cwithout cherry-picking the old commit wholesale. The static SVG renderer already resolves the longitudinal axis-label anchor correctly; the browser renderer now follows the same semantics.Stack: this PR is layer 3 and targets
agent/bughunt-axes-helper-parity(#355).Focused coverage
plot.y + plot.h / 2label_position=start|center|endValidation
uv run --with pre-commit pre-commit run --all-filesuv run ruff check .uv run ruff format --check .git diff --checkThe browser geometry tests run in CI, including the exact rotated-title labelpad case that previously failed. Local browser capture was intentionally avoided; no Chrome workaround was used.
Visual acceptance: Matplotlib Cohere
The Matplotlib Cohere gallery example is the acceptance target. Static XY already centers
Coherence; this PR makes the interactive browser title use that same resolved position. Evidence images will be linked in a follow-up comment so they remain outside the product branch.Summary by CodeRabbit