fix: quote bound animation for disjoint price range#467
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a disjoint price-range check to the chart’s quote-bound update logic so that when switching between markets with non-overlapping price ranges, the Y-axis bounds and current tick position snap immediately instead of animating across a large vertical gap, while preserving smooth animations for normal in-range updates. Sequence diagram for disjoint price range update on market switchsequenceDiagram
participant MarketDataStream
participant BasicChartState
participant BottomBoundController as BottomBoundAnimationController
participant TopBoundController as TopBoundAnimationController
MarketDataStream->>BasicChartState: onNewQuoteRange(minQuote, maxQuote)
activate BasicChartState
BasicChartState->>BasicChartState: _updateQuoteBoundTargets(minQuote, maxQuote)
alt minQuote or maxQuote is NaN
BasicChartState-->>MarketDataStream: return (no update)
else rangeDisjoint
BasicChartState->>BottomBoundController: set value = minQuote
BasicChartState->>TopBoundController: set value = maxQuote
BasicChartState->>BasicChartState: completeCurrentTickAnimation()
BasicChartState-->>MarketDataStream: return (snap, no animation)
else overlapping ranges
alt minQuote changed
BasicChartState->>BottomBoundController: animateTo(minQuote, easeOut)
end
alt maxQuote changed
BasicChartState->>TopBoundController: animateTo(maxQuote, easeOut)
end
BasicChartState-->>MarketDataStream: return (smooth animation)
end
deactivate BasicChartState
Class diagram for updated BasicChartState quote bound handlingclassDiagram
class BasicChartState {
+double bottomBoundQuoteTarget
+double topBoundQuoteTarget
+AnimationController bottomBoundQuoteAnimationController
+AnimationController topBoundQuoteAnimationController
+double currentTickPercent
+void _updateQuoteBoundTargets(double minQuote, double maxQuote)
+void completeCurrentTickAnimation()
}
class AnimationController {
+double value
+void animateTo(double target, Curve curve)
}
class Curve
BasicChartState --> AnimationController : uses
BasicChartState --> Curve : uses
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 |
![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 left some high level feedback:
- Consider extracting the disjoint-range logic (
rangeDisjoint) into a small helper with a brief doc comment so that the intention (market switch vs. normal update) is clearer and reusable if this check is needed elsewhere. - When
minQuote.isNaN || maxQuote.isNaNyou now return early and keep the previous bounds and tick state; double-check whether you should instead reset or clamp the controllers to a safe state to avoid the chart getting stuck with stale bounds if NaNs persist.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the disjoint-range logic (`rangeDisjoint`) into a small helper with a brief doc comment so that the intention (market switch vs. normal update) is clearer and reusable if this check is needed elsewhere.
- When `minQuote.isNaN || maxQuote.isNaN` you now return early and keep the previous bounds and tick state; double-check whether you should instead reset or clamp the controllers to a safe state to avoid the chart getting stuck with stale bounds if NaNs persist.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Claude finished @behnam-deriv's task —— View job Code Review CompleteI've reviewed the changes in this PR. The fix is well-implemented and addresses the core issue effectively. Here's my detailed analysis: High Priority✅ Core Logic is Sound ✅ NaN Guard is Good Medium Priority1. Consider Extracting Disjoint Logic (Sourcery-ai suggestion) The inline disjoint check is clear enough for this use case, but if this logic might be reused elsewhere, consider extracting it: /// Returns true if the new quote range [minQuote, maxQuote] has no overlap
/// with the current target range, indicating a market switch.
bool _isDisjointRange(double minQuote, double maxQuote) {
return maxQuote < bottomBoundQuoteTarget || minQuote > topBoundQuoteTarget;
}However, given that this logic appears specific to 2. Edge Case: Touching Boundaries The disjoint check uses strict inequality (
These ranges touch at Recommendation: The current behavior seems correct for most cases, but document this decision if the touching-range behavior is intentional. 3. Initial Bounds Edge Case When the chart first loads, Low Priority1. Code Removed NaN Checks From Individual Conditions The old code checked 2. Testing Coverage I couldn't find unit tests for
3. Documentation The inline comment on line 329 is clear and explains the "why" well. Good work! ✅ Potential RegressionsNone identified. The changes are defensive:
Security & PerformanceNo concerns. This is purely visualization logic with no security implications. Performance impact is negligible (one additional boolean check per update). SummaryApprove with minor suggestions. The fix is correct and well-implemented. The sourcery-ai suggestions are valid but optional - the current code is clear and maintainable as-is. The most valuable follow-up would be adding tests to prevent regression of this visual bug. Concrete Actions (Optional):
|
behnam-deriv
deleted the
adjust-quote-bound-animation-for-disjoint-price-ranges
branch
1 participant
Problem
When switching markets (e.g. from EURUSD ~1.08 to XAUUSD ~2000), the chart animates the Y-axis quote bounds and the current spot indicator from the old market's price to the new market's price. Because the price ranges can differ, this creates a jarring long vertical line on screen during the transition.
Two things caused the artifact:
_updateQuoteBoundTargetsalways calledanimateToon the bound controllers, even when the new price range had no overlap with the current one.previousLastEntryand the new last tick on every update. On a market switch the two entries are from completely different price levels, so it slides across a huge vertical gap.Solution
Added a disjoint-range check in
_updateQuoteBoundTargets. Before deciding whether to animate, the incoming[minQuote, maxQuote]range is compared with the current[bottomBoundQuoteTarget, topBoundQuoteTarget]. If the ranges have no overlap at all the data is clearly from a different market, so both transitions are skipped:.value = …) instead of callinganimateTo, snapping the Y-axis immediately.completeCurrentTickAnimation()is called, jumpingcurrentTickPercentto1.0so the spot indicator renders at the new price without interpolating from the old one.Normal same-market tick updates always stay within or near the current range, so they are unaffected and continue to use the smooth
animateToanimation.Summary by Sourcery
Prevent jarring Y-axis and spot animations when switching between markets with disjoint price ranges.
Bug Fixes: