Add drag-to-select date range on balance charts#2733
Conversation
Dragging across the dashboard net worth chart or an account's balance chart now selects the dragged date range, reusing the same query-param mechanism as the period picker (start_date/end_date via Periodable). Built on d3-brush (already vendored) and Turbo.visit, so both charts share one code path in time_series_chart_controller.js. Also fixes UI::PeriodPicker, which silently fell back to "30D" instead of showing "Custom" for a custom-range Period.
Drive the chart tooltip from d3-brush events and snap drag selections to the nearest plotted data points so tooltips and range navigation stay accurate. Opt the chart out of the parent card's HTML5 drag-and-drop by adding draggable="false" to the chart container and remove the turbo-frame visit option. Update assertions in PagesControllerTest and add a system test (DashboardChartDragSelectTest) that verifies dragging the net worth chart navigates to the selected date range instead of reordering dashboard cards.
Disable touch handling on d3-brush so native scrolling works on mobile devices. Drag-select is a mouse-only affordance. Also extract custom period parsing into its own method with targeted error handling, so date range validation errors aren't confused with unrelated failures elsewhere in #set_period.
|
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:
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 (6)
📝 WalkthroughWalkthroughCharts now support drag-selecting date ranges and navigating with ChangesSelectable chart flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Chart
participant Turbo
participant Periodable
User->>Chart: Drag across selectable chart
Chart->>Chart: Snap endpoints to data points
Chart->>Turbo: Visit URL with start_date and end_date
Turbo->>Periodable: Request selected date range
Periodable-->>User: Render custom period selection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
The mouse-drag gesture used fixed pixel offsets from the overlay's center, assuming a specific rendered chart width. CI's headless Chrome (Linux) renders the layout slightly differently than local runs, shifting the overlay's actual width enough that the drag start point landed outside it, so the brush never fired and no navigation happened. Compute the offset and drag distance from the overlay's own measured width instead, keeping a fixed inset from each edge so the gesture always starts and ends within the overlay regardless of environment.
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Reviewed for correctness only (no changes made).
Nicely scoped feature with good coverage — unit tests for Periodable/UI::PeriodPicker plus system tests for both chart drag paths. A few things I specifically checked and didn't find issues with:
Periodable#custom_period_from_paramscorrectly falls back to last-30-days both for unparsable date strings and for a reversed range, viaPeriod's ownmust_be_valid_date_rangevalidation raisingActiveModel::ValidationErroronvalidate!._nearestDataPointForPixelintime_series_chart_controller.jsnow clamps both bounds (Math.max(x0 - 1, 0)/Math.min(x0, length - 1)), fixing a latent out-of-bounds risk that existed in the old hover-only bisector code near the edges of the chart.- The
draggable="false"on the net-worth chart container correctly overrides the ancestor<section draggable="true">used for dashboard card reordering, and there's a system test asserting the drag-select gesture doesn't reorder cards.
No blocking concerns from my read.
Generated by Claude Code
|
This looks solid — CI is fully green, the feature is well-tested (new Generated by Claude Code |
Dragging across the dashboard net worth chart or an account's balance chart now selects the dragged date range, reusing the same query-param mechanism as the period picker (start_date/end_date via Periodable). Built on d3-brush (already vendored) and Turbo.visit, so both charts share one code path in time_series_chart_controller.js. Also fixes UI::PeriodPicker, which silently fell back to "30D" instead of showing "Custom" for a custom-range Period.
Drive the chart tooltip from d3-brush events and snap drag selections to the nearest plotted data points so tooltips and range navigation stay accurate. Opt the chart out of the parent card's HTML5 drag-and-drop by adding draggable="false" to the chart container and remove the turbo-frame visit option. Update assertions in PagesControllerTest and add a system test (DashboardChartDragSelectTest) that verifies dragging the net worth chart navigates to the selected date range instead of reordering dashboard cards.
Disable touch handling on d3-brush so native scrolling works on mobile devices. Drag-select is a mouse-only affordance. Also extract custom period parsing into its own method with targeted error handling, so date range validation errors aren't confused with unrelated failures elsewhere in #set_period.
The mouse-drag gesture used fixed pixel offsets from the overlay's center, assuming a specific rendered chart width. CI's headless Chrome (Linux) renders the layout slightly differently than local runs, shifting the overlay's actual width enough that the drag start point landed outside it, so the brush never fired and no navigation happened. Compute the offset and drag distance from the overlay's own measured width instead, keeping a fixed inset from each edge so the gesture always starts and ends within the overlay regardless of environment.
1954c50 to
53aab2b
Compare
gariasf
left a comment
gariasf
left a comment
There was a problem hiding this comment.
Nice feature, and I like that both charts end up on one code path. Two things I'd want fixed before merge, both inline. I also verified a few claims that are easy to doubt, so noting them here so nobody re-checks:
d3-brush@3.0.0really is available: pinned inconfig/importmap.rbandvendor/javascript/d3-brush.jsexists, andvendor/javascript/d3.jsre-exports it.- The
.touchable(false)reasoning holds up in the vendored source —style("touch-action", "none")sits after.filter(touchable)in the same chain, so it genuinely isn't applied. Native scroll and touch card-reorder both stay intact. - A plain click is safe: d3 sets
state.selection = nullbeforeemit.end(), soif (!event.selection) returncatches it. .move(null)from inside theendhandler doesn't loop — the programmatic move has nosourceEvent(move handler bails) and re-emitsendwith a null selection (end handler bails).- d3 removes its window-level
mousemove/mouseuplisteners beforeemit.end(), so theTurbo.visitteardown can't leak them. ReportsControlleralsoinclude Periodableand ownsstart_date/end_datewith different semantics, but every action reassigns@periodafter thebefore_action, so nothing regresses there.pagypassesrequest.query_parametersthrough and the activity list isn't period-filtered, so paging keeps the selected range. Correct as-is.period.custom.label_shortexists in every locale file, so the new "Custom" label isn't hardcoded English.
Two smaller things not worth inline comments:
_renderTooltipAtderef'sthis._d3Tooltipunguarded, soselectable: true+use_tooltip: falsewouldTypeErroron every mousedown. No caller hits it today (sparkline and onboarding areuse_tooltip=falseand non-selectable), so latent only — but_hideTooltipalready uses?., worth matching with an earlyif (!this._d3Tooltip) return;.- The two new system tests duplicate 14 lines of locale-agnostic sign-in plus an identical overlay-measuring drag block. The rationale for not using the shared
sign_inis fair, but it belongs inapplication_system_test_case.rbnext to it rather than copy-pasted twice.
Also: test/controllers/accounts_controller_test.rb "show accepts a custom start_date/end_date range" only asserts :success, so it passes even if the params are ignored entirely — an assert_select "button[aria-label='Time period: Custom']" would give it teeth. And start_date present with end_date absent is an untested branch.
| if (x1 - x0 < this._dragSelectMinPx) { | ||
| this._d3DragSelectGroup.call(this._d3DragSelectBrush.move, null); | ||
| return; | ||
| } | ||
|
|
||
| // Snap to the actual plotted data points (same as the tooltip shown | ||
| // during the drag) rather than the raw continuous pixel-to-time value — | ||
| // a few pixels of mouse imprecision can otherwise land on a date that's | ||
| // days away from the one the user was looking at on a wide chart. | ||
| const startDate = this._nearestDataPointForPixel(x0).date; | ||
| const endDate = this._nearestDataPointForPixel(x1).date; | ||
|
|
||
| this._navigateToDateRange(startDate, endDate); |
There was a problem hiding this comment.
The guard is in pixels, but what actually decides the outcome is the snapped dates three lines below it.
On a 30D chart at ~800px that's roughly 27px per day, so any drag between 4px and ~13px clears this threshold and then snaps both ends to the same datum, giving start_date == end_date. Period.custom accepts that — must_be_valid_date_range only rejects start > end — the series builder returns a single point, and _draw() takes the _normalDataPoints.length < 2 branch into _drawEmpty(). So a slightly-too-small drag blanks the chart and removes its brush and tooltip along with it; the only way back out is the period picker.
Guarding the real invariant instead makes the pixel threshold redundant, so _dragSelectMinPx on line 27 can go too:
| if (x1 - x0 < this._dragSelectMinPx) { | |
| this._d3DragSelectGroup.call(this._d3DragSelectBrush.move, null); | |
| return; | |
| } | |
| // Snap to the actual plotted data points (same as the tooltip shown | |
| // during the drag) rather than the raw continuous pixel-to-time value — | |
| // a few pixels of mouse imprecision can otherwise land on a date that's | |
| // days away from the one the user was looking at on a wide chart. | |
| const startDate = this._nearestDataPointForPixel(x0).date; | |
| const endDate = this._nearestDataPointForPixel(x1).date; | |
| this._navigateToDateRange(startDate, endDate); | |
| // Snap to the actual plotted data points (same as the tooltip shown | |
| // during the drag) rather than the raw continuous pixel-to-time value — | |
| // a few pixels of mouse imprecision can otherwise land on a date that's | |
| // days away from the one the user was looking at on a wide chart. | |
| const startDatum = this._nearestDataPointForPixel(x0); | |
| const endDatum = this._nearestDataPointForPixel(x1); | |
| // Reject *after* snapping: a short drag can resolve to a single data | |
| // point on a wide chart, and a zero-day range renders as the empty-state | |
| // chart — which has no brush to drag back out of. | |
| if (startDatum === endDatum) { | |
| this._d3DragSelectGroup.call(this._d3DragSelectBrush.move, null); | |
| return; | |
| } | |
| this._navigateToDateRange(startDatum.date, endDatum.date); |
| _navigateToDateRange(startDate, endDate) { | ||
| const formatDate = d3.timeFormat("%Y-%m-%d"); | ||
| const url = new URL(window.location.href); | ||
|
|
||
| url.searchParams.delete("period"); | ||
| url.searchParams.set("start_date", formatDate(startDate)); | ||
| url.searchParams.set("end_date", formatDate(endDate)); | ||
|
|
||
| Turbo.visit(url.toString()); |
There was a problem hiding this comment.
This makes the URL authoritative for the selected range, but on the dashboard the period picker can't clear it again.
dashboard.html.erb renders the picker with frame: "dashboard_sections", which becomes data-turbo-frame on each link. Turbo's getVisitAction finds no data-turbo-action on either the link or the frame, so the frame navigation never touches the address bar. Sequence:
- Drag-select → this
Turbo.visitputs?start_date=A&end_date=Bin the URL. - Click "30D" → the frame re-renders as 30D, address bar still
A/B. - Refresh, share, or bookmark → back to the custom range, because
set_periodgivesstart_date/end_dateprecedence overperiod.
So the user sees 30D, reloads, and silently gets the custom range back. Pre-PR this was invisible because the dashboard URL never carried period state at all.
Cleanest fix is in the picker, scoped to frame-mode callers so the account chart (which renders its picker outside the frame and already does a full visit) is unaffected — DS::MenuItem#merged_opts already merges caller data: with the frame attribute, so this composes:
<%# app/components/UI/period_picker.html.erb %>
<% menu.with_item(
variant: :link,
text: period.label_short,
href: href_for(period.key),
frame: frame,
selected: selected?(period.key),
# Frame navigations don't touch the address bar, so a stale
# ?start_date/&end_date from a chart drag-select would outlive a preset
# pick and come back on refresh. Advance so the URL always matches
# what's rendered.
data: frame.present? ? { turbo_action: "advance" } : {}
) %>The alternative, if you'd rather keep ?period= out of the dashboard URL, is to give this controller a frame value and navigate the frame on the dashboard — consistent either way, but then a drag-selected range is no longer shareable or refresh-safe, which is most of the point of putting it in the URL.
Follow-up: the picker can't represent the range it just selectedTesting the drag-select on a local build, the interaction itself works well. Where it falls down is the control that's supposed to reflect it. Three separate things, all in
Before — trigger says "Custom", and there isn't a check mark anywhere in the list:
After — trigger carries the range, and a trailing checked
The
|
….com/Holdner/sure into feature/chart-drag-date-range-select
Enhance the period picker to better display and handle custom date ranges (e.g., from chart drag-select): - Show actual dates in picker trigger instead of generic "Custom" - Add trailing menu row for custom ranges so list always has one checked item - Add `label_range()` method to Period for compact date display - Use `turbo_action="advance"` to update URL bar, preventing stale date params from persisting after preset selection - Fix time series chart drag-select: improve small-drag detection, snap to nearest data point before validating range, handle missing tooltip gracefully - Refactor system test helpers: make `sign_in()` i18n-safe by using field types, extract `drag_across()` helper for consistent chart testing - Update all related tests and test data
|
Thanks @gariasf, this is an unusually thorough review, and every point landed. I re-derived each claim against the source before touching anything; all six hold. Two of them hold harder than stated, which changed the fix in ways worth flagging.
|
![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 `@app/models/period.rb`:
- Around line 222-224: Update both cross-year label_range and custom-period
comparison_label to use I18n date formatting/range keys instead of
strftime(`@date_format`), so month names follow the active locale while preserving
the displayed range format. Add a regression test using a non-English locale
covering both user-facing ranges.
🪄 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: 8a60b3af-332c-4719-99ee-5dd46c2e1138
📒 Files selected for processing (11)
app/components/UI/period_picker.html.erbapp/components/UI/period_picker.rbapp/javascript/controllers/time_series_chart_controller.jsapp/models/period.rbtest/application_system_test_case.rbtest/components/UI/period_picker_test.rbtest/controllers/accounts_controller_test.rbtest/controllers/concerns/periodable_test.rbtest/models/period_test.rbtest/system/account_chart_drag_select_test.rbtest/system/dashboard_chart_drag_select_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- test/controllers/accounts_controller_test.rb
- test/controllers/concerns/periodable_test.rb
- app/javascript/controllers/time_series_chart_controller.js
Remove hardcoded `date_format:` parameter and replace `strftime` calls with `I18n.l` to ensure date formatting respects the active locale's conventions (e.g., day-month-year vs month-day-year order, locale-specific month names, and translatable separators). Add new i18n keys for period ranges and custom comparison labels, plus `short_with_year` date format to English and French locales.
4 participants
Summary
start_date/end_date), reusing the samePeriodablequery-param mechanism as the period picker — built on d3-brush (already vendored) andTurbo.visit, so both charts share one code path intime_series_chart_controller.js.UI::PeriodPicker, which silently fell back to "30D" instead of showing "Custom" for a custom-rangePeriod.draggable="false") so dragging the chart selects a range instead of reordering dashboard cards.touchable(false)on the brush) so native scrolling/panning is untouched on mobile.Test plan
bin/rails testDISABLE_PARALLELIZATION=true bin/rails test:system(new:AccountChartDragSelectTest,DashboardChartDragSelectTest)bin/rubocop -f github -abundle exec erb_lint ./app/**/*.erb -anpm run lintbin/brakeman --no-pager🤖 Generated with Claude Code
Summary by CodeRabbit
start_dateandend_dateare provided, the app prioritizes them for the selected period (invalid/reversed inputs fall back to the default range).