fix: prevent stale prevLastEntry animation on market switch#468
Merged
behnam-deriv merged 1 commit intomasterfrom Mar 12, 2026
Merged
fix: prevent stale prevLastEntry animation on market switch#468behnam-deriv merged 1 commit intomasterfrom
behnam-deriv merged 1 commit intomasterfrom
Conversation
Contributor
Reviewer's GuideAdds a guard in DataSeries.didUpdate to only carry over prevLastEntry when the new series is a continuation of the previous data, using a new _isSameSeries helper that compares first entries, and ensures updated remains true even when no animation state is transferred to avoid stale OHLC wick animations on market switches. Sequence diagram for updated DataSeries.didUpdate behavior on market switchsequenceDiagram
actor User
participant MarketSwitcher
participant ChartWidget
participant DataSeries
participant OhlcPainter
User->>MarketSwitcher: selectNewMarket()
MarketSwitcher->>ChartWidget: setState(newTicks)
ChartWidget->>DataSeries: create new DataSeries with input
ChartWidget->>DataSeries: didUpdate(oldSeries)
alt oldSeries.entries not null and same last entry
DataSeries->>DataSeries: prevLastEntry = oldSeries.prevLastEntry
else oldSeries.entries not null and _isSameSeries(oldSeries) is true
DataSeries->>DataSeries: prevLastEntry = IndexedEntry(oldSeries.entries.last)
DataSeries->>ChartWidget: didUpdate returns updated = true
else different series (e.g. market switch)
DataSeries->>DataSeries: prevLastEntry remains null
DataSeries->>ChartWidget: didUpdate returns updated = true
end
ChartWidget->>OhlcPainter: paint with lastCandle and prevLastEntry
alt prevLastEntry is not null
OhlcPainter->>OhlcPainter: animate wick using prevLastEntry and lastCandle
else prevLastEntry is null (market switch)
OhlcPainter->>OhlcPainter: render lastCandle without cross-series animation
end
Class diagram for DataSeries with new _isSameSeries guardclassDiagram
class Tick {
double quote
int epoch
}
class IndexedEntry_T_ {
T entry
int index
}
class Series {
}
class DataSeries_T_ {
T extends Tick
List~T~? entries
List~T~ input
IndexedEntry_T_? prevLastEntry
+didUpdate(DataSeries_T_ oldSeries) bool
+isOldDataAvailable(DataSeries_Tick_ oldSeries) bool
-_isSameSeries(DataSeries_Tick_ oldSeries) bool
+shouldRepaint(ChartData oldDelegate) bool
}
class DataSeries_Tick_ {
}
class ChartData {
}
DataSeries_T_ ..|> Series
DataSeries_T_ --> Tick : uses
DataSeries_T_ --> IndexedEntry_T_ : wraps
DataSeries_T_ --> DataSeries_Tick_ : parameter in _isSameSeries and isOldDataAvailable
DataSeries_T_ --> ChartData : parameter in shouldRepaint
%% Method detail for _isSameSeries
class DataSeries_T_ {
-_isSameSeries(oldSeries DataSeries_Tick_) bool
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Claude finished @behnam-deriv's task —— View job Code Review Complete ✅I've reviewed this PR and the fix looks solid. The implementation correctly addresses the root cause of the stale SummaryVerdict: APPROVED ✅ The fix is well-designed and implements a clean guard condition to prevent animation artifacts when switching between different market datasets. The solution is minimal, focused, and follows good engineering practices. High Priority ItemsNone identified. The implementation is correct and safe. Medium Priority Items1. Missing Test CoverageSeverity: Medium There are no tests covering the
Example test structure: test('_isSameSeries returns true for incremental update', () {
final oldSeries = CandleSeries([
Candle(epoch: 1000, quote: 100, ...),
Candle(epoch: 2000, quote: 101, ...),
]);
final newSeries = CandleSeries([
Candle(epoch: 1000, quote: 100, ...), // Same first entry
Candle(epoch: 2000, quote: 101, ...),
Candle(epoch: 3000, quote: 102, ...), // New tick
]);
expect(newSeries._isSameSeries(oldSeries), isTrue);
});
test('_isSameSeries returns false for market switch', () {
final oldSeries = CandleSeries([
Candle(epoch: 1000, quote: 996, ...), // Old market price
Candle(epoch: 2000, quote: 997, ...),
]);
final newSeries = CandleSeries([
Candle(epoch: 1000, quote: 1209, ...), // New market price
Candle(epoch: 2000, quote: 1210, ...),
]);
expect(newSeries._isSameSeries(oldSeries), isFalse);
});2. Documentation Could Mention the Painter ContextSeverity: Low The documentation is excellent but could briefly mention that this guard prevents visual artifacts in Optional addition: /// Returns true when [oldSeries] and this series share the same data source,
/// i.e. this series is an incremental update of [oldSeries] rather than an
/// entirely new dataset (e.g. a market/symbol switch).
///
/// This guard prevents visual artifacts in painters (e.g. OhlcPainter) that
/// use [prevLastEntry] for animation: when markets switch, the old market's
/// prices would be rendered with the new market's Y-axis scale, causing
/// off-screen wick lines.
///
/// Compares the first entries by epoch and quote: for an incremental update
/// (new tick, new candle) the beginning of the data is unchanged, so the
/// first entries are identical. For a market switch both datasets cover the
/// same time window, but the prices differ, so the first entries will not
/// match and [prevLastEntry] must not carry over.Low Priority ItemsNone identified. Detailed AnalysisCorrectness ✅The
Edge Cases ✅The implementation handles edge cases well:
Performance ✅
Potential Regressions ✅No regressions identified. The change is conservative:
Alternative Approaches ConsideredThe PR description mentions this approach is superior to:
The chosen solution is the most elegant - prevent the problem at the source rather than patching symptoms. ConclusionThis is a high-quality fix that addresses a real visual bug with a minimal, well-documented change. The implementation is correct, handles edge cases properly, and follows the codebase's architecture patterns. Recommendations:
The PR is ready to merge. Great work on the detailed problem analysis and clean solution! 🎯 |
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_isSameSeries, consider aligning the parameter type with the generic onDataSeries(e.g.DataSeries<T>orDataSeries<T extends Tick>) instead ofDataSeries<Tick>to avoid unnecessary type narrowing and potential subtype issues. _isSameSeriescurrently comparesinput.firstagainstoldSeries.entries!.first; ifentriesis the canonical, transformed representation used for painting, you may want to compareentrieson both sides for consistency or document whyinputvsentriesis intentional.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_isSameSeries`, consider aligning the parameter type with the generic on `DataSeries` (e.g. `DataSeries<T>` or `DataSeries<T extends Tick>`) instead of `DataSeries<Tick>` to avoid unnecessary type narrowing and potential subtype issues.
- `_isSameSeries` currently compares `input.first` against `oldSeries.entries!.first`; if `entries` is the canonical, transformed representation used for painting, you may want to compare `entries` on both sides for consistency or document why `input` vs `entries` is intentional.
## Individual Comments
### Comment 1
<location path="lib/src/deriv_chart/chart/data_visualization/chart_series/data_series.dart" line_range="301-308" />
<code_context>
+ if (input.isEmpty || (oldSeries.entries?.isEmpty ?? true)) {
+ return false;
+ }
+ final T newFirst = input.first;
+ final Tick oldFirst = oldSeries.entries!.first;
+ return newFirst.epoch == oldFirst.epoch && newFirst.quote == oldFirst.quote;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using direct equality on quote for floating-point-like data may be fragile.
This comparison hinges on `newFirst.quote == oldFirst.quote`. If `quote` is a floating-point value or can differ slightly between sources, strict equality may cause logically identical updates to be treated as different series. If that’s possible here, compare within a tolerance or use a more stable key (e.g., instrument ID plus epoch) instead.
```suggestion
bool _isSameSeries(DataSeries<Tick> oldSeries) {
if (input.isEmpty || (oldSeries.entries?.isEmpty ?? true)) {
return false;
}
final T newFirst = input.first;
final Tick oldFirst = oldSeries.entries!.first;
// Epoch must match exactly; quote may differ slightly, so compare with a tolerance.
if (newFirst.epoch != oldFirst.epoch) {
return false;
}
const double quoteTolerance = 1e-8;
final double quoteDiff = (newFirst.quote - oldFirst.quote).abs();
return quoteDiff <= quoteTolerance;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+301
to
+308
| bool _isSameSeries(DataSeries<Tick> oldSeries) { | ||
| if (input.isEmpty || (oldSeries.entries?.isEmpty ?? true)) { | ||
| return false; | ||
| } | ||
| final T newFirst = input.first; | ||
| final Tick oldFirst = oldSeries.entries!.first; | ||
| return newFirst.epoch == oldFirst.epoch && newFirst.quote == oldFirst.quote; | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Using direct equality on quote for floating-point-like data may be fragile.
This comparison hinges on newFirst.quote == oldFirst.quote. If quote is a floating-point value or can differ slightly between sources, strict equality may cause logically identical updates to be treated as different series. If that’s possible here, compare within a tolerance or use a more stable key (e.g., instrument ID plus epoch) instead.
Suggested change
| bool _isSameSeries(DataSeries<Tick> oldSeries) { | |
| if (input.isEmpty || (oldSeries.entries?.isEmpty ?? true)) { | |
| return false; | |
| } | |
| final T newFirst = input.first; | |
| final Tick oldFirst = oldSeries.entries!.first; | |
| return newFirst.epoch == oldFirst.epoch && newFirst.quote == oldFirst.quote; | |
| } | |
| bool _isSameSeries(DataSeries<Tick> oldSeries) { | |
| if (input.isEmpty || (oldSeries.entries?.isEmpty ?? true)) { | |
| return false; | |
| } | |
| final T newFirst = input.first; | |
| final Tick oldFirst = oldSeries.entries!.first; | |
| // Epoch must match exactly; quote may differ slightly, so compare with a tolerance. | |
| if (newFirst.epoch != oldFirst.epoch) { | |
| return false; | |
| } | |
| const double quoteTolerance = 1e-8; | |
| final double quoteDiff = (newFirst.quote - oldFirst.quote).abs(); | |
| return quoteDiff <= quoteTolerance; | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: prevent stale
prevLastEntryanimation on market switch for OHLC chartsProblem
When switching markets on a candlestick (or any OHLC) chart, a one-frame glitch
appears on the last candle: a long vertical line shoots from the new market's
candle body down to the old market's price level.
Root cause chain:
setState(() { ticks = fetchedTicks; }), whichtriggers a rebuild and creates a new
CandleSerieswith the new market's data.DataSeries.didUpdate(oldSeries)is called on this new instance with the oldmarket's series as
oldData.entries!.last != oldSeries.entries!.last),the
else ifbranch fires unconditionally and sets:prevLastEntrynow holds the old market's last candle (e.g. price ~996).OhlcPainter.onPaintData, the animation logic usesprevLastEntrytocompute
yHigh:quoteToY(996)maps far off thebottom of the canvas. The wick line is drawn from that off-screen coordinate up
to
yLow, producing the visible artifact.The same tick animation that was fixed for area/line charts in
fix: quote bound animation for disjoint price range does not cover this
case: even after
completeCurrentTickAnimation()snapscurrentTickPercentto1.0, theyHigh/yLowclamping logic inOhlcPainterstill reads the staleprevLastEntryin that frame.Solution
Add a
_isSameSeriesguard inDataSeries.didUpdateso thatprevLastEntryisonly set when the incoming data is a continuation of the old series — not a
wholesale replacement.
_isSameSeriescompares the first entries of both series by epoch and quote:For an incremental update (new tick, updated candle) the start of the dataset is
unchanged, so the first entries match → animation runs as before.
For a market switch both datasets span the same time window but at completely
different prices → first quotes differ →
prevLastEntryis never set → no glitch.Because
prevLastEntryis a nullable field initialised tonullon every newDataSeriesinstance, not setting it is sufficient — no explicitresetLastEntryAnimation()call is needed.The
else { updated = true; }branch is preserved so that the method stillcorrectly returns
trueon a market switch (new data did arrive), which keepscompound indicator sub-series recalculation and
_playNewTickAnimationworkingcorrectly.
Summary by Sourcery
Guard last-entry animations so that OHLC chart series only animate as continuations of the same data source, preventing visual glitches when switching markets.
Bug Fixes:
Enhancements: