Grey, persistent box/lasso/x-range/y-range selections#238
Conversation
Greptile SummaryThis PR updates chart selection styling and makes rectangular selections persist. The main changes are:
Confidence Score: 5/5Safe to merge with minimal risk. The changed code preserves public styling override precedence, validates the new range mode, clears mutually exclusive overlays consistently, and includes focused browser probes for the changed behavior. No files require special attention.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Reviews (3): Last reviewed commit: "Hydrate persisted selection overlay on l..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Merging this PR will not alter performance
Comparing 
Footnotes
|
Alek99
changed the title
|
The two red checks ( #236 ("Decouple CI tests from README", MERGEABLE) fixes exactly these. I merged #236 into this branch locally (no conflicts) and ran the previously-failing suites plus this PR's new selection test — 117 passed, including all 20 that CI reports red. So once #236 lands and this branch picks it up, CI goes green with no change needed here. |
|
Both Greptile findings addressed in be421f6: P1 — Restore linked overlays. Fixed. The link-group handler in P2 — Document selection overrides. Fixed in Added |
Box, lasso, x-range, and y-range selection overlays defaulted to a blue that matched nothing else in the chrome, while box-zoom used its own flat grey. Route all of them through new scheme-aware internal tokens (--xy-selection / --xy-selection-fill) set to the modebar's text greys: #5c6573 in light mode, #adb4bf under a .dark root class. The public --chart-selection* and --chart-zoom-selection* tokens still override both schemes.
Box, x-range, and y-range brushes vanished the instant the drag ended, leaving only the highlighted points; only the lasso stayed drawn. Make all three persist like the lasso: the data-space rectangle re-projects onto the selection overlay every draw (through pan/zoom) until the selection is cleared. The overlay is shaped by a mode discriminator (box | x | y). The range brushes span the full plot on their free axis and drop the cross-axis border pair — x-range keeps only its left/right edges, y-range only its top/bottom — so each reads as the bounds it constrains. The mode round-trips through durable state as an optional selection.range.mode (a plain box omits it) and never affects the selected point set. Also soften the band border to .6 alpha. Box and lasso overlays stay mutually exclusive, double-click still clears, and a browser probe covers persistence, edge borders, mode round-trip, and rejection of a bad mode value.
Addresses Greptile review on the box-persistence change: - P1 (correctness): a range selection received from a link-group peer updated _stateSelection and the local masks but never set _boxSelection, so _drawNow had nothing to re-project and the persistent band was absent on linked charts. The linked handler now hydrates the same overlay state a local gesture would — box/x/y range sets _boxSelection (clearing any lasso), a polygon sets _lassoPolygon (clearing any box), and clear wipes both — so peers show and re-project the overlay. The range broadcast now carries the x/y band mode so peers render the correct edge-only shape. - P2 (docs): spec/api/styling.md now lists --chart-selection / --chart-selection-fill and --chart-zoom-selection / --chart-zoom-selection-fill in the public override sentence, matching the CSS which resolves each public token ahead of the --xy-selection* fallback. Adds test_linked_selection_hydrates_persisted_overlay, driving the real BroadcastChannel handler with the exact message shapes the sender emits (x-range, polygon, plain box, clear) and asserting overlay + durable state on the receiving peer. Verified end-to-end in-browser with two link-grouped charts.
be421f6 to
6637b52
Compare
📝 WalkthroughWalkthroughRectangular selections now persist across redraws and view-state updates, support box/x/y modes, synchronize through linked selections, and remain mutually exclusive with lasso overlays. Selection styling uses scheme-aware tokens, with documentation and browser coverage updated accordingly. ChangesSelection overlays
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChartView
participant ViewState
participant LinkedChart
User->>ChartView: complete box/x/y range gesture
ChartView->>ViewState: persist range and optional mode
ChartView->>LinkedChart: broadcast selection.range
LinkedChart->>ChartView: hydrate range overlay
ChartView->>ChartView: re-render overlay after redraw
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/src/53_interaction.ts (1)
197-218: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBox-zoom click/cancel can hide a persisted selection without restoring it.
previousLasso/previousBoxare only snapshotted whenmode.startsWith("select"), butend()(lines 279-280) andpointercancel(lines 338-339) unconditionally hideselRect/selLassofor any band, including"zoom"(box-zoom). Since a box-zoom gesture never capturespreviousBox/previousLasso, a stray click or a cancelled box-zoom drag while a box/x-range/y-range (or lasso) selection is active will hide that overlay with nothing to restore it (the "not moved"/else if (band.previousBox)branches at lines 315-321, and the equivalent inpointercancelat 343-346, never trigger for zoom mode). The overlay only comes back on the next unrelated_drawNow()call, which isn't guaranteed to happen soon — this undercuts the "persists until cleared" contract this PR adds for box/x/y-range selections specifically.
_boxSelection/_lassoPolygonare never mutated by a drag that stays under the 3px threshold (only the DOM preview is touched), so it's safe to snapshot them regardless of gesture mode.🐛 Proposed fix: capture snapshots regardless of gesture mode
- const previousLasso = mode.startsWith("select") && this._lassoPolygon - ? this._lassoPolygon.map((point) => [...point]) - : null; - // A prior box/x/y selection is restored the same way as a prior lasso - // when the new gesture turns out to be a no-op click or is cancelled. - const previousBox = mode.startsWith("select") && this._boxSelection - ? { ...this._boxSelection } - : null; + const previousLasso = this._lassoPolygon + ? this._lassoPolygon.map((point) => [...point]) + : null; + // A prior box/x/y selection is restored the same way as a prior lasso + // when the new gesture turns out to be a no-op click or is cancelled — + // including a box-zoom drag, which shares selRect/selLasso with the + // persisted overlay and would otherwise hide it with nothing to + // restore it. + const previousBox = this._boxSelection + ? { ...this._boxSelection } + : null;🤖 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 197 - 218, Update the gesture initialization block around previousLasso and previousBox to snapshot the active _lassoPolygon and _boxSelection regardless of mode, rather than guarding them with mode.startsWith("select"). This ensures the existing restoration branches in end() and pointercancel restore persisted selections after no-op or cancelled box-zoom gestures while preserving current selection snapshots.
🤖 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.
Outside diff comments:
In `@js/src/53_interaction.ts`:
- Around line 197-218: Update the gesture initialization block around
previousLasso and previousBox to snapshot the active _lassoPolygon and
_boxSelection regardless of mode, rather than guarding them with
mode.startsWith("select"). This ensures the existing restoration branches in
end() and pointercancel restore persisted selections after no-op or cancelled
box-zoom gestures while preserving current selection snapshots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e844a1be-404a-4160-8a85-978e60171427
📒 Files selected for processing (9)
docs/styling/chrome-slots.mddocs/styling/themes-and-tokens.mdjs/src/20_theme.tsjs/src/50_chartview.tsjs/src/53_interaction.tsjs/src/57_viewstate.tsspec/api/styling.mdspec/design/view-state.mdtests/test_view_state_client.py
1 participant
What
Two changes to how selections look and behave.
1. Grey defaults matching the toolbar. The box, lasso, x-range, and y-range overlays defaulted to a blue (
rgba(90,140,240,…)) that matched nothing else in the chrome, and box-zoom used a separate flat grey. All of them now default to the modebar's own text greys via scheme-aware internal tokens--xy-selection/--xy-selection-fill:rgba(92,101,115,.6)stroke /…,.12)fill (#5c6573, the modebar text grey).darkon the chart root or any ancestor):rgba(173,180,191,.6)/…,.12)(#adb4bf)The public
--chart-selection*/--chart-zoom-selection*tokens are unchanged and still override both schemes; only the built-in fallbacks moved.2. Persistent box / x-range / y-range selections. Previously only the lasso stayed drawn after the gesture — box and range brushes vanished the instant the drag ended, leaving just the highlighted points. Now all three persist like the lasso: the data-space rectangle re-projects onto the selection overlay every draw (through pan and zoom) until the selection is cleared.
modediscriminator (box | x | y). Range brushes span the full plot on their free axis and drop the cross-axis border pair: x-range keeps only its left/right edges, y-range only its top/bottom — so each reads as the bounds it constrains.moderound-trips through durable state as an optionalselection.range.mode(a plain box omits it, keeping the existing{x0,x1,y0,y1}shape) and never affects the selected point set.Spec and docs (
spec/api/styling.md,spec/design/view-state.md,docs/styling/*) updated to match.Verification
node js/build.mjs(tsc typecheck + bundle) passestest_rectangular_selection_persists_with_range_borderscovers persistence through the real draw path, the range-edge borders,moderound-trip viaapplyState, rejection of a badmode, and box/lasso mutual exclusiontest_view_state_client,test_static_client_security,test_components,test_ui_issue_regressions,test_modebar_select_drill,test_selection_rows,test_view_state.darkroot classSummary by CodeRabbit
New Features
Bug Fixes
Style