fix(timeline): align scrub crosshair to the plotted line + reserve readout slot#144
Conversation
…adout slot The merged-vitals timeline chart draws each line from its 15-minute bucket averages (_Vital.avg), but the scrub crosshair read the raw nearest sample (_Vital.valueAt over pts). At a touched x the dot therefore sat off the line and jittered at a different granularity from the smooth static render. Resolve the crosshair against the SAME bucketed series and x-scale the line is stroked from: plottedLineValueAt() linearly interpolates avg between adjacent bucket centres — the exact y the painter's lineTo segments pass through — so the marker lands on the line at matching granularity. The value readout now reports the same on-line value. x→time mapping is extracted as scrubTimeAt(); both are pure and unit-tested. Also reserve the crosshair readout's vertical space at all times (Visibility with maintainSize) so it appearing on scrub no longer grows the tile and reflows the peak/low + events below it. Idle it lays out the same rows as dashes and is hidden. Static rendering is unchanged. Closes OpenStrap#141 Reported in OpenStrap#102.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe timeline scrubber now maps pointer positions to chart timestamps using shared chart geometry, reports interpolated values from the plotted averaged line, and keeps the crosshair readout area reserved while idle. Unit tests cover clamping, interpolation, degenerate inputs, and the composed scrub path. ChangesTimeline scrub alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 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 `@lib/ui/timeline/timeline_screen.dart`:
- Around line 64-66: Update _scrubT in
lib/ui/timeline/timeline_screen.dart#L64-L66 to return null when t is before
avg.first.t or after avg.last.t, while preserving endpoint values at exact
bucket-centre times. Update test/timeline_scrub_test.dart#L82-L85 to expect null
outside the plotted range and retain endpoint assertions only at exact centres.
🪄 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: 9e4f5063-e184-4d52-ad14-6306ae766796
📒 Files selected for processing (2)
lib/ui/timeline/timeline_screen.darttest/timeline_scrub_test.dart
…n range plottedLineValueAt clamped the scrub value to the nearest bucket centre when the scrub time fell outside a vital's buckets. But the scrub band spans the whole timeline while each vital's line is only drawn from its first to its last bucket centre, so scrubbing before a vital starts (or after it ends) reported a flat/extrapolated value at a time where that vital's line isn't drawn. Return null when t is strictly outside [first centre, last centre]; the boundary centres still return their value and interpolation within the range is unchanged. The crosshair dot already skips a null value (no dot) and the readout already renders it as "—", so an out-of-coverage vital is now omitted exactly where its line is absent, matching the plot. Addresses CodeRabbit review on OpenStrap#144.
2 participants
What & why
Fixes the two scrubbing bugs on the merged-vitals timeline graph (issue #141, originally reported in #102).
Bug 1 — scrub markers don't align with the plotted line
The chart draws each vital's line from its 15-minute bucket averages (
_Vital.avg, via_ChartPainter._drawLine), but the scrub crosshair resolved the value from the raw nearest sample (_Vital.valueAtoverpts). So at any touched x the dot sat off the drawn line and jittered at a visibly different granularity from the clean static render.Root cause:
lib/ui/timeline/timeline_screen.dart— the paint usesv.avg(bucketed, interpolated by segment) while the hit-test usedv.pts(raw, nearest-neighbour). Same x-scale, different array + different resolution.Fix: the crosshair now resolves against the same bucketed series and x-scale the line is stroked from.
plottedLineValueAt(avg, t)linearly interpolatesavgbetween the two adjacent bucket centres — the exactythe painter'slineTosegments pass through — so the marker lands on the line at the touched x, at matching (bucketed) granularity. The value readout reports the same on-line value, so dot and numbers agree.Bug 2 — legend/readout appearing on scrub reflows the layout
The per-moment vital readout was only inserted into the tile's
Columnwhile scrubbing, so starting a scrub grew the tile and shoved the peak/low + events below it — a jarring reflow.Fix: the readout's slot is reserved at all times (
VisibilitywithmaintainSize/maintainState). Idle, it lays out the same rows as dashes and is simply hidden, so height is stable whether or not you're scrubbing.Static rendering is unchanged — only the scrub path and the readout's layout slot are touched.
Tests
Extracted the scrub mapping as pure functions and unit-tested them in
test/timeline_scrub_test.dart:scrubTimeAt(x-pixel → time): edges, midpoint, left-pad/right-edge clamping, degenerate width.plottedLineValueAt(time → value on the drawn line): vertices, segment interpolation, endpoint clamping, empty/single-bucket, coincident timestamps — including an explicit assertion that the interpolated point is the segment midpoint, not either raw vertex.scrub → valuegroup asserting a touched x lands on the line at matching granularity.Needs on-device confirmation
No Flutter SDK/device in this environment, so the visual/gesture parts are inspection-verified. The actual scrub feel — marker sitting exactly on the line under a moving finger, and zero vertical shift when the readout shows/hides — should be confirmed on a real device.
Closes #141
Reported in #102.
Summary by CodeRabbit
Bug Fixes
Tests