Stop funnel labels overlapping on short vertical charts - #481
Conversation
Value labels sit at the stage centers and drop-off labels at the boundaries between them, so the two ladders interleave at HALF the stage pitch. Both were budgeted against a whole one, so every pitch between one and two line heights admitted labels that then piled onto each other — an eight-stage funnel at 260px drew all fifteen on top of one another while each ladder believed it had room. A drop-off label now keeps its row only where neither neighbouring value label reaches into the span it starts at. The value label wins that tie: it is the datum, the drop-off is arithmetic derived from it, and every number stays in the tooltip and the event payload either way. Horizontal funnels are exempt — their value labels ride above their own segment rather than in the boundary lane — and a roomy chart is unchanged. The ladder's plot-size estimate was optimistic in the same direction. It scaled the chart height by 0.85, but the title band, tick labels and margins cost a roughly fixed ~90px rather than a fixed fraction, so at height 180 it claimed 153px of a real 92px plot and placed labels that could not fit. It now subtracts the band, which is pessimistic on a tall chart — the safe direction, since over-stating the room prints text over text while under-stating it only hides a label the tooltip still carries. Verified by parsing the rendered SVGs and intersecting every label box across a 60-case matrix (8/6/5 stages, both orientations, both geometries, heights 180-520): 11 cases carried real overlaps before, none do now, and label counts are untouched wherever there was already room.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe funnel chart now estimates usable plot space, interleaves value and drop-off labels, and hides colliding drop-off labels when required. The contract, implementation, tests, documentation, and changelog describe the updated behavior. ChangesFunnel label layout
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Chart
participant marks
participant FunnelLayout
participant TooltipEvents
Chart->>marks: pass chart dimensions
marks->>FunnelLayout: estimate plot size and place labels
FunnelLayout->>FunnelLayout: interleave label rows and suppress collisions
FunnelLayout-->>Chart: render visible labels
FunnelLayout-->>TooltipEvents: retain numeric label data
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 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 `@python/xy/marks.py`:
- Line 1021: Update the dimension handling in the surrounding _svg.layout() flow
so configured float width and height values are accepted as concrete chart
dimensions, not replaced by the default 640×400 values. Ensure the
_funnel.estimate_plot_px(plot_w, plot_h) call receives the same effective
dimensions later used by the renderer, while preserving existing behavior for
unsupported or absent dimensions.
🪄 Autofix
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: bc432bf0-b911-477f-8db1-e29fb6f42918
📒 Files selected for processing (6)
CHANGELOG.mddocs/charts/funnel-chart.mdpython/xy/_funnel.pypython/xy/marks.pyspec/api/chart-kind-contract.mdtests/test_funnel.py
![reflex-reviewer[bot]](https://avatars.githubusercontent.com/in/4540092?s=60&v=4){kind=small}
There was a problem hiding this comment.
💬 Comments
Reviewed 8f850cede8
This PR revises funnel label budgeting so vertical value/drop-off ladders account for half-stage spacing, adds a conservative fixed chrome estimate, and documents/tests the decimation rule. The targeted funnel suite passes. I also rendered the same deterministic 8-stage, 260px titled funnel at merge base and head: the base has 15 labels with a 10.49px minimum outside-lane gap, while the head keeps 9 labels with a 20.99px minimum gap. The matched screenshots show the intended short-chart improvement. One narrow boundary remains where the estimate can still admit overlapping labels.
Visual evidence
What was executed
uv run pytest tests/test_funnel.py -q✅ — 112 passed.uv run python <SVG outside-lane gap probe for 8 stages, height=260, merge base>❌ exit 1 — Reproduced old behavior: 15 labels, minimum outside-lane baseline gap 10.49px vs 16.80px required.uv run python <SVG outside-lane gap probe for 8 stages, height=260, PR head>✅ — Verified claimed short-chart fix: 9 labels, minimum outside-lane baseline gap 20.99px.uv run python <SVG outside-lane gap probe for 8 stages, height=359, PR head>❌ exit 1 — Found remaining threshold overlap: 15 labels, minimum outside-lane baseline gap 16.33px vs 16.80px required.
Outside the diff
- 🟡 Keep the plot-height estimate conservative at the admission threshold —
python/xy/_funnel.py:65
The fixed 90px subtraction is still slightly optimistic for the titled SVG layout at some heights. For the documented 8-stage/default-12px scenario at chart height 359,estimate_plot_pxgives 269px, so line 485 admits all drop-offs (269 / 8 / 2 = 16.8125px >= 16.8px). The actual SVG plot is only 277px high after the funnel’s own stage padding, yielding adjacent outside-label baselines just 16.33px apart. The targeted SVG probe therefore fails (outside_min_gap=16.33px required=16.80px, pair-69%/9,100 1%). Because the estimate is explicitly required to be pessimistic, increase the chrome allowance (or derive a stricter effective plot budget including renderer padding) so this threshold case hides the drop-offs rather than retaining overlap. A regression test should exercise the composed/rendered chart near this transition, not onlydecide_labelswith its estimatedplot_px.
/review re-run · /review skip mute this PR · label review-deep for a deeper model
|
Reflex Reviewer — ✅ Looks good at |
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}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…re them The label-ladder note listed show_dropoff=False alongside more height and a smaller label_size as ways to get the drop-off labels back, but it is the opt-out, not a way to recover the room.
There was a problem hiding this comment.
✅ Looks good
Reviewed f0f2757547
This PR makes vertical funnel label placement account for the half-stage spacing between value and drop-off rows, and replaces the optimistic proportional plot-height estimate with a conservative fixed chrome allowance. It also documents the decimation rule and adds focused regression coverage. I ran the five new targeted tests successfully and reproduced the dense 8-stage, 260px case on merge base 659888c versus head f0f2757: the base retained all drop-off rows and visibly crowded the value ladder, while the head hides those derived labels and leaves the values separated. The matched native PNG exports are attached.
Visual evidence
Before — merge base 659888c — dense 8-stage vertical funnel at 520×260; value and drop-off rows crowd together.
After — PR head f0f2757 — identical funnel; derived drop-off rows are hidden and value labels remain separated.
What was executed
.venv/bin/pytest tests/test_funnel.py -q -k 'dropoff_labels_share or roomy_funnel or wide_inside or horizontal_dropoff or plot_box_estimate'❌ exit 127 — Initial targeted-test attempt; the checkout had no pre-existing virtual environment.uv run pytest tests/test_funnel.py -q -k 'dropoff_labels_share or roomy_funnel or wide_inside or horizontal_dropoff or plot_box_estimate'✅ — All five focused regression tests passed (107 unrelated tests deselected).uv run python -c '<render dense 8-stage 520x260 funnel to PNG>'✅ — Run in separate merge-base worktree 659888c; generated the before screenshot.uv run python -c '<render dense 8-stage 520x260 funnel to PNG>'✅ — Run at PR head f0f2757 with identical data and dimensions; generated the after screenshot.uv run python -c '<assert dense-case drop-off placement states>'✅ — On merge base, confirmed at least one (in fact all seven) drop-off labels remained outside; on head, confirmed all seven are hidden for this cramped case.
/review re-run · /review skip mute this PR · label review-deep for a deeper model
1 participant
The bug
Funnel value labels and drop-off labels overlap on short vertical charts. An
8-stage funnel at 260px draws all fifteen labels on top of one another; a
5-stage funnel in a 260px dashboard tile collides its last value label with the
drop-off above it.
Two causes, both in the build-time label ladder.
The ladders interleave at half the pitch. Value labels sit at the stage
centers and drop-off labels at the boundaries between them, so consecutive
rows in the outside lane are half a stage pitch apart. Both were gated on
_fits(line_px, pitch_px)— a whole pitch — so every pitch between one and twoline heights admitted labels that then collided, while each ladder believed it
had room. With the 12px default that is a stage pitch of 17–34px.
The plot-size estimate was optimistic. The ladder cannot see the real plot
rectangle (only the renderers lay it out) so it estimates from the configured
chart size, and it scaled height by
0.85. But the title band, tick labels andmargins cost a roughly fixed ~90px rather than a fixed fraction: at height 180
the estimate claimed 153px of a real 92px plot, 65% too much, and placed labels
that could not fit. Measured against the SVG exporter,
height - 88tracks thereal plot box from 180px through 640px.
The fix
A drop-off label keeps its row only where neither neighbouring value label
reaches into the span it starts at. The value label wins that tie — it is the
datum, the drop-off is arithmetic derived from it, and every number stays in the
tooltip and the event payload either way. So a cramped funnel now drops the
derived percentage instead of printing it over the stage value.
Horizontal funnels are exempt and untouched: their value labels ride above
their own segment rather than in the boundary lane, which is why only the
vertical ladder could be contested.
The estimate now subtracts the chrome band instead of scaling. That is
pessimistic on a tall chart, which is the safe direction — over-stating the room
prints text over text, while under-stating it only hides a label the tooltip
still carries. The reasoning is written down next to the constant, and the rule
is now normative in
spec/api/chart-kind-contract.md(§28 requires decimationdecisions be recorded).
Verification
Ground truth rather than the ladder's own model: parse the rendered SVGs and
intersect every label's text box. Across a 60-case matrix — 8/6/5 stages, both
orientations, both geometries, heights 180 through 520 — 11 cases carried real
overlaps before and none do now, with label counts unchanged wherever there
was already room (the docs' 440px examples place identically).
The 11 previously-broken cases, with the worst measured overlap in each:
dense8_v180…dense8_v260(5 heights)wide6_v180…wide6_v220(3 heights)basic5_v180,basic5_v200,basic5_bar180Three new regression tests, each confirmed to fail on
mainand pass here:test_dropoff_labels_share_the_outside_lane_at_half_the_stage_pitch— sweepsplot heights and asserts no two labels sharing the lane are closer than a line
test_a_wide_inside_label_hides_the_dropoff_it_would_reach_intotest_plot_box_estimate_subtracts_a_fixed_chrome_bandplus
test_a_roomy_funnel_still_draws_every_dropoff_labelandtest_horizontal_dropoff_labels_keep_their_own_rowguarding againstover-correction.
Gates:
tests/test_funnel.py112 passed; full suite 4114 passed / 5 skippedwith one pre-existing unrelated failure
(
tests/pyplot/test_pdsh_gap_features.py::test_pandas_datetime_series_plot_completes,a matplotlib
ticker.pyNotImplementedErrorthat fails identically onunmodified
main);ruff check,ruff format --checkand the pre-commit ruffhooks pass. Also confirmed in a live browser that the client honours the same
decisions as the static exporters.
Summary by CodeRabbit
Bug Fixes
Documentation