feat: make the accumulators barriers draggable - #500
behnam-deriv wants to merge 10 commits into
Conversation
Adds an opt-in way for consumer apps to let users adjust the growth rate by dragging either accumulators barrier, instead of only through a separate trade-params control. A consumer creates an `AccumulatorBarrierDragController`, feeds it the ladder of selectable growth rates, and passes it to `AccumulatorIndicator`. The chart snaps the band to the nearest rung as the user drags and reports the rung back; committing the value and pushing the real barriers down stays the consumer's job. Without a controller the barriers render exactly as before. Notable details: - The controller is owned by the consumer because it has to outlive the annotation, which is rebuilt on every tick. It also carries the grip style: `ChartTheme` is a pure interface, so adding a getter there would break any app that implements it. - The painter publishes the geometry it resolved for the frame it drew, so hit-testing matches the pixels even during the post-tick lerp. - A drag preview latches past drag end until the model moves, otherwise the band rubber-bands back to its pre-drag width while the consumer's commit is in flight. A timeout drops the latch if that commit never lands. - `recalculateMinMax` follows the preview, so a wider band cannot paint outside the visible quote range. - The gesture recognizer claims the pointer only over a grip or its barrier line, which beats the chart's pan/scale recognizer, and passes the hit transform so drag positions stay local. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewer's GuideAdds an opt-in, consumer-controlled draggable Accumulators barrier experience: the painter supplies frame-accurate geometry, an overlay eagerly captures only valid barrier targets, movement snaps to configured growth-rate steps, and a commit latch keeps previews stable until the model catches up or times out, with comprehensive controller and widget coverage. Sequence diagram for dragging and committing Accumulators barrierssequenceDiagram
participant User
participant Overlay as AccumulatorBarrierDragOverlay
participant Recognizer as AccumulatorBarrierGestureRecognizer
participant Controller as AccumulatorBarrierDragController
participant Chart as MainChart
participant Consumer
User->>Recognizer: Pointer down on grip or barrier line
Recognizer->>Controller: geometry.hitTest()
Recognizer->>Recognizer: startTrackingPointer(event.pointer, event.transform)
Recognizer->>Controller: beginDrag(side)
Controller-->>Consumer: onDragStart()
User->>Recognizer: Pointer move
Recognizer->>Overlay: onBarrierDragUpdate(local)
Overlay->>Controller: nearestStep(distance, hysteresis)
Overlay->>Controller: updateDrag(step)
Controller-->>Chart: notifyListeners()
Chart->>Chart: recalculateMinMax() using preview
User->>Recognizer: Pointer up
Recognizer->>Overlay: onBarrierDragEnd()
Overlay->>Controller: endDrag(commit: true)
Controller-->>Consumer: onDragEnd(step)
Controller->>Controller: latch preview and start commitTimeout
Consumer->>Chart: Push committed barriers
Chart->>Controller: releaseLatchIfModelMoved(highBarrier, lowBarrier)
Controller->>Controller: clearPreview()
State diagram for the Accumulators barrier drag controllerstateDiagram-v2
[*] --> Idle
Idle --> Hovered: setHovered(side)
Hovered --> Idle: setHovered(null)
Idle --> Dragging: beginDrag(side)
Hovered --> Dragging: beginDrag(side)
Dragging --> Dragging: updateDrag(step)
Dragging --> PreviewLatched: endDrag(commit: true)
Dragging --> Idle: endDrag(commit: false)
PreviewLatched --> Idle: releaseLatchIfModelMoved()
PreviewLatched --> Idle: clearPreview()
PreviewLatched --> Idle: commitTimeout
Idle --> Disabled: enabled = false
Hovered --> Disabled: enabled = false
Dragging --> Disabled: enabled = false
PreviewLatched --> Disabled: enabled = false
Disabled --> Idle: enabled = true
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
|
Claude finished @behnam-deriv's task in 4m 49s —— View job I'll analyze this and get back to you. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/src/deriv_chart/chart/main_chart.dart" line_range="192-194" />
<code_context>
+ /// The accumulators annotation the user is allowed to drag, if any.
+ AccumulatorIndicator? get _draggableAccumulator =>
+ widget.annotations?.whereType<AccumulatorIndicator>().firstWhereOrNull(
+ (AccumulatorIndicator indicator) =>
+ (indicator.dragController?.enabled ?? false) &&
+ indicator.activeContract == null,
+ );
+
</code_context>
<issue_to_address>
**issue (bug_risk):** An enabled controller with an empty `steps` ladder still mounts the overlay and claims pointer events over the barrier lines or grips, but `nearestStep` returns null, so the drag produces no preview or callback while preventing the chart's pan/scale gesture.
**Triggers:** When a consumer constructs the controller before populating `steps`, or intentionally supplies an empty ladder.
**Suggested fix:** Only mount and hit-test the overlay when `controller.steps` is non-empty, or reject the gesture when the ladder is empty.
</issue_to_address>Four refinements from testing the drag end to end. Scale the drag to the ladder. Real barrier offsets are nearly flat across growth rates — the ladder spans ~26% from its tightest rung to its widest — so mapping the pointer straight onto the barrier put every growth rate within a few pixels of travel. The gesture is now scaled so the whole ladder takes `ladderTravel` pixels, leaving the band on true barrier positions at the cost of the grip no longer tracking the finger exactly. A ladder already wider than that target is left at 1:1, since reducing sensitivity would only hurt. Hold ladder updates back for the duration of a drag. Barrier distances move with the spot, so a ladder refreshing on every tick shifted the snap targets under the user's finger — and the rungs sit close enough together that a rung could change with the pointer completely still. Stop hover from rebuilding the chart. Every controller notification drove a setState on MainChart, so crossing a barrier re-ran updateVisibleData twice, recomputed min/max across every series and annotation, and rebuilt the whole stack — to change a cursor and a stroke width, which showed up as a CPU spike on a profiler. Hover now repaints only the annotation layer and rebuilds only the overlay; the expensive path is reserved for a moved band, which is the one case that genuinely needs the quote bounds re-measured. Glide between rungs instead of stepping. The band now animates on each snap, and on the commit, matching what already happens when the growth rate is changed from the trade params. It cannot share that animation controller — playNewTickAnimation is a no-op while an animation is in flight and rungs are crossed far faster than it runs — so it reuses the duration and curve through its own. The transition starts from the distance last painted, so changing rung mid-glide carries on from where the band actually is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @behnam-deriv's task in 1m 39s —— View job I'll analyze this and get back to you. |
… Y axis The grips were small and sat in the middle of the band, so a finger dragging one covered the barrier value it was changing — the one number the gesture exists to adjust. They are now 56x18 rather than 31x12, which with the existing 44px minimum touch target makes the tap area 56x44, and they are pinned to the right of the plotting area instead of centred. The Y-axis label strip is excluded from that edge, so the grips sit beside the axis rather than under its labels; the painter had no access to the plotting width, so the overlay publishes it to the controller the same way the painter publishes geometry back. Right-aligning is clamped so a band narrower than the grip still holds it inside. The grip fill now defaults to the chart's own background instead of a hardcoded near-white, so it stops being a white block on a dark chart. In the default themes that resolves to exactly the colour the designs use in dark mode. Placement moved into `AccumulatorBarrierGeometry.gripCenterX` so it can be tested without a golden — buried in `onPaint` it could only have been verified by eye. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @behnam-deriv's task in 1m 38s —— View job I'll analyze this and get back to you. |
The consumer needs to know which barrier the user has hold of: which grip is in hand decides which way they must drag to leave an end of the ladder — at the tightest band the top grip goes up and the bottom one goes down. Carry the side through onDragStart/onDragUpdate/onDragEnd rather than making the consumer infer it. Also fold the "at least two rungs" rule into `enabled`. A one-rung ladder has nowhere to drag to, and grips the user cannot move are worse than no grips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @behnam-deriv's task in 1m 27s —— View job I'll analyze this and get back to you. |
`verticalPaddingFraction` was read once in initState, so a consumer could choose the scale the chart opened at but never change it afterwards. It now re-applies when the value changes, which is what lets the scale follow something like a trade-type switch without the chart being recreated. Only on a change: the value the user drags to lives in the same field, so re-applying on every rebuild would fight their gesture. Also names the clamp bounds the quote-label drag already used, and clamps the incoming value to them, so a consumer cannot ask for a scale the user would be unable to drag back to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @behnam-deriv's task in 1m 52s —— View job I'll analyze this and get back to you. |
Tapping a grip and letting go without moving left the consumer's readout on screen for good. `endDrag` returned early when nothing had been previewed, so `onDragEnd` never fired — but `onDragStart` had already fired on touch-down, and that is what puts the readout up. It now reports on every release: the settled step when there is one, and otherwise the rung the band is already on, which is where the user let go. That also covers a cancelled gesture, which had the same bug in a worse form — the previewed rate stayed on the readout with nothing to clear it. Also stops a release from re-highlighting the barrier it just ended on. Letting go does not move the pointer, so the first hover afterwards lands back on the same grip — on touch as well, via the synthetic mouse event a browser fires after a tap. Hover is ignored at that spot until the pointer genuinely moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @behnam-deriv's task in 2m 31s —— View job I'll analyze this and get back to you. |
Coming back from the background, the Accumulators band slid across the chart to catch up with the spot instead of simply being there. An annotation eases from `previousObject`, and frames stop while the app is away — so the position it was left in is as old as the absence, and the whole backlog lands in one rebuild. Easing from a position that has since scrolled out of view is a sweep across the chart, not a step. Dropped in `onUpdate`, which already asks the same question of the annotation itself one line above. Annotations whose object leaves an epoch bound null — a plain horizontal barrier spans the chart rather than sitting at a point in time — are unaffected, since they can never scroll out of view. Follows the same shape as the disjoint-range and stale-prevLastEntry fixes: decide from the data whether the new position continues the old one, rather than from an out-of-band signal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @behnam-deriv's task in 3m 29s —— View job I'll analyze this and get back to you. |
The barriers are no longer dragged directly. The whole band is a tap target instead, and the host puts its own control on screen when the tap is reported, moving the band from there with previewGrowthRate. Dragging stays implemented and tested, but is off unless a consumer opts in, so by default there are no grips and a press that turns into a pan still pans. The press is reported as well as the tap, because a host that decides what a gesture means from a document listener in the capture phase has already acted by the time the tap is known. While a rate is being chosen the two barrier values are the thing the user is reading, so they grow and thicken, then settle back. They grow away from their own barrier rather than outward from their middle: anchored on the centre, half of every pixel of growth walked the text onto the line it labels. The + label pins its baseline and the - label its top edge, so whichever edge faces the barrier is the one that stays put, and both grow right into the empty chart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @behnam-deriv's task in 9m 13s —— View job I'll analyze this and get back to you. |
A hand on a disc inside the band's top-left corner, pulsing until the band is tapped. Off unless the consumer asks for it: the chart draws and animates the hint, and remembering whether this user has already been shown it is the consumer's. Two halo rings travel out from the disc half a cycle apart, easing out so they read as emitted rather than as circles that grow, and the disc swells as each one leaves so the ping looks like it comes off the badge. One ping a second. The loop only runs while a hint is actually up — otherwise it would repaint the annotation layer every frame, all session, for nothing. Anchored to the band's top-left rather than centred, so it stays put as the band widens and narrows with the growth rate, and centred instead when a band is too short to hold it without crossing the lower barrier. The tap that answers the hint retires it in the chart there and then, and latches it. A consumer goes on sending the old flag for the frame or two before its own state catches up, and the band republishes every tick, so without the latch the hint would flick back on over the control the tap just opened. Sending false releases it, which is also how a consumer asks for the hint back. The hand is transcribed from the design's SVG into a Path. One icon does not justify a dependency on every host app bundling a font, and a path scales with the swell for free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @behnam-deriv's task in 5m 1s —— View job I'll analyze this and get back to you. |
A previewed band was drawn around the incoming tick's centre outright, with no ease from the previous one, while barrierX was interpolated right beside it. So on every tick the band arrived at the new price on the first frame and then slid horizontally into place: a jump, and only vertically. The preview owns the band's width, not its position. The centre belongs to the spot, which keeps moving for as long as the picker is open, so it now eases whether or not a rung is being previewed and only the two barrier quotes stay the preview's. The comment that drove this conflated the two. The geometry still publishes the committed centre: that is the quote-space reference the drag measures its pointer distance against, so it has to stay still even as the rendered band moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @behnam-deriv's task in 3m 39s —— View job I'll analyze this and get back to you. |
🤖 Claude PR Review CompleteModel: Summary0 of 3 issues from the previous review have been resolved — but this push isn't trying to fix any of them. Commit I traced this change end-to-end against the drag math it could have broken: None of this push touches Recommendation: APPROVE (this push's own change is a correct, well-tested bug fix; the three carried-over findings are refinements, not blockers — same as prior review) 🟡 Medium Priority Issues (carried over, unresolved)🟡 1. Pinch-zoom isn't blocked while a barrier is being dragged —
|
| Severity | File | Lines |
|---|---|---|
| MEDIUM | lib/src/deriv_chart/chart/main_chart.dart |
734-743 |
❌ Problematic Code:
onDragBegin: () {
crosshairController.onExit(const PointerExitEvent());
_xScrollBlockedBeforeBarrierDrag = xAxis.isScrollBlocked;
// A second finger opens a new gesture arena the barrier recognizer
// does not join, so the chart's scale recognizer could still pan.
xAxis.isScrollBlocked = true;
completeCurrentTickAnimation();
},
onDragFinish: () =>
xAxis.isScrollBlocked = _xScrollBlockedBeforeBarrierDrag,📋 Issue: isScrollBlocked only guards XAxisModel.onPanUpdate and the momentum path — it does not guard onScaleUpdate, which is what a second finger's pinch actually drives. While one finger drags a barrier grip, a second finger can still zoom the time axis. Still unaddressed; isScaleBlocked does not exist anywhere in lib/.
✅ Fix:
// x_axis_model.dart
bool _isScaleBlocked = false;
bool get isScaleBlocked => _isScaleBlocked;
set isScaleBlocked(bool value) => _isScaleBlocked = value;
void onScaleUpdate(ScaleUpdateDetails details) {
if (_isScaleBlocked) {
return;
}
if (_currentViewingMode == ViewingMode.followCurrentTick) {
_scaleWithNowFixed(details);
} else {
_scaleWithFocalPointFixed(details);
}
}// main_chart.dart
onDragBegin: () {
crosshairController.onExit(const PointerExitEvent());
_xScrollBlockedBeforeBarrierDrag = xAxis.isScrollBlocked;
xAxis.isScrollBlocked = true;
xAxis.isScaleBlocked = true;
completeCurrentTickAnimation();
},
onDragFinish: () {
xAxis.isScrollBlocked = _xScrollBlockedBeforeBarrierDrag;
xAxis.isScaleBlocked = false;
},💡 Explanation: Mirrors the existing isScrollBlocked toggle for scale, closing the gap the code's own comment already identifies for pan.
🟡 2. New MainChart wiring has no test coverage — lib/src/deriv_chart/chart/main_chart.dart
Details
| Severity | File | Lines |
|---|---|---|
| MEDIUM | lib/src/deriv_chart/chart/main_chart.dart |
_draggableAccumulator getter, didUpdateChartData release/glide block, animation-controller lifecycle |
❌ Problematic Code:
AccumulatorIndicator? get _draggableAccumulator =>
widget.annotations?.whereType<AccumulatorIndicator>().firstWhereOrNull(
(AccumulatorIndicator indicator) =>
(indicator.dragController?.enabled ?? false) &&
indicator.activeContract == null,
);📋 Issue: Same gap as the previous two reviews. This push's new test (accumulator_barrier_tick_glide_test.dart) exercises AccumulatorIndicatorPainter and AccumulatorBarrierDragController directly, which is exactly the right level for the animation bug it fixes — but, like the prior pushes' new tests, it still doesn't reach anything inside MainChart. _draggableAccumulator selection, releaseLatchIfModelMoved inside didUpdateChartData, the previousObject glide-source reconstruction, and the animation controllers' forward/reverse/repeat decisions remain untested at the integration level.
MainChart.
✅ Fix:
Add a MainChart widget test covering:
testWidgets('drag overlay is not mounted once a contract becomes active', ...);
testWidgets('releaseLatchIfModelMoved glides from the rendered preview once the model barriers move', ...);💡 Explanation: Closes the gap where the previously-tested controller/overlay/painter logic actually gets exercised end-to-end. vertical_padding_fraction_test.dart shows the team already has the harness needed to write this as a MainChart widget test.
🟢 Low Priority Issues (carried over, unresolved)
🟢 3. Gesture recognizer can leave the controller stuck mid-drag if pointer tracking stops abnormally — lib/src/deriv_chart/chart/data_visualization/annotations/barriers/accumulators_barriers/accumulator_barrier_gesture_recognizer.dart:182-186
Details
| Severity | File | Lines |
|---|---|---|
| LOW | lib/src/deriv_chart/chart/data_visualization/annotations/barriers/accumulators_barriers/accumulator_barrier_gesture_recognizer.dart |
182-186 |
❌ Problematic Code:
@override
void didStopTrackingLastPointer(int pointer) {
_isBarrierHit = false;
_downPosition = null;
}📋 Issue: Unchanged — this file isn't touched by this push either. If tracking ever stops through a route other than the explicit up/cancel handling in handleEvent, controller.isDragging can stay true with no further pointer events to clear it, since this override never calls onBarrierDragCancel.
dispose() already force-ends the drag on unmount, but if triggered, barriers appear stuck in the dragged/highlighted state until another drag starts and finishes.
✅ Fix:
@override
void didStopTrackingLastPointer(int pointer) {
if (_isBarrierHit && !tapOnly) {
onBarrierDragCancel();
}
_isBarrierHit = false;
_downPosition = null;
}💡 Explanation: Makes the recognizer self-defensive so controller.isDragging can never outlive the pointer it was tracking, in both the drag and tap modes.
Summary Table
| Priority | Count | Categories |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 High | 0 | — |
| 🟡 Medium | 2 | Multi-touch interaction gap, missing integration tests |
| 🟢 Low | 1 | Defensive gesture-recognizer cleanup |
Recommendations
- Add an
isScaleBlockedguard (or equivalent) alongsideisScrollBlockedso a second-finger pinch can't rescale the chart mid-drag. - Add a
MainChart-level widget test for_draggableAccumulatorselection,releaseLatchIfModelMoved, and the animation controllers' start/stop/forward/reverse transitions. - Optionally harden
AccumulatorBarrierGestureRecognizer.didStopTrackingLastPointerto fireonBarrierDragCancelwhen a real drag (not a tap) is interrupted.
None of these block the PR. This push's own change — correctly keeping the drag's quote-space anchor (bandCenterQuote, still committedCenterQuote) stable while fixing only the rendered band centre to glide with the tick animation — is a clean, well-targeted fix with a new test that reproduces the exact regression it closes.
Auto Fix Claude Reviews
| Action | Open Dashboard |
|---|
1 participant
What
Lets consumer apps offer growth-rate adjustment by dragging either accumulators barrier, rather than only through a separate trade-params control. Opt-in: without a controller the barriers render exactly as they do today.
Design: whole flow · barrier component
How it's used
The chart snaps the band to the nearest rung as the user drags and reports it back. Committing the value and pushing the real barriers back down stays the consumer's job.
Design notes
ChartThemeis a pure interface, so adding a getter there would break any app that implements it.recalculateMinMaxfollows the preview, so a wider preview cannot paint outside the visible quote range.startTrackingPointer, without which every drag position is global rather than local.useDrawingToolsV2.Public API
New exports:
AccumulatorBarrierDragController,AccumulatorGrowthRateStep,AccumulatorBarrierGripStyle,AccumulatorBarrierSide, plus thedragControllerparameter onAccumulatorIndicator. All additive — nothing existing changes shape.Testing
flutter analyze --no-fatal-infos— clean (8 pre-existing infos in unrelated files)flutter test— 211 passed, 26 of them new: snapping and hysteresis, hit-testing, the commit latch,recalculateMinMaxunder a preview, and widget tests covering the drag winning the gesture arena over a competing pan.🤖 Generated with Claude Code
Summary by Sourcery
Enable consumers to make Accumulators barriers draggable while preserving existing rendering and behavior when the feature is not configured.
New Features:
Bug Fixes:
Enhancements:
Tests: