feat: add default tick offset for X-axis configuration #453
Conversation
Reviewer's GuideAdds a configurable default tick offset for the X-axis and wires it through chart configuration, X-axis widgets, and model logic so initial load and "scroll to last tick" use this offset instead of always using the maximum current tick offset. Sequence diagram for applying defaultTickOffset on chart initializationsequenceDiagram
participant ChartConfig as ChartAxisConfig
participant Wrapper as XAxisWrapper
participant Widget as XAxisBase
participant Model as XAxisModel
ChartConfig->>Wrapper: provide maxCurrentTickOffset, defaultTickOffset
Wrapper->>Widget: new XAxisBase(defaultTickOffset, maxCurrentTickOffset)
Widget->>Model: new XAxisModel(maxCurrentTickOffset, defaultTickOffset)
Model->>Model: _defaultTickOffset = clamp(defaultTickOffset, 0, _maxCurrentTickOffset)
Model->>Model: if null use _maxCurrentTickOffset
Model->>Model: _rightBoundEpoch = _shiftEpoch(_maxEpoch, _defaultTickOffset)
Model-->>Widget: initial X axis state with default offset
Sequence diagram for scroll to last tick using defaultTickOffsetsequenceDiagram
actor User
participant UI as ScrollToLastTickButton
participant Model as XAxisModel
User->>UI: tap scroll to last tick
UI->>Model: scrollToLastTick()
Model->>Model: target = _shiftEpoch(lastEntryEpochOrNow, _defaultTickOffset) + duration
Model->>Model: compute distance = target - _rightBoundEpoch
Model-->>UI: animate scroll with distance
UI-->>User: chart shows latest tick with defaultTickOffset from right edge
Class diagram for X-axis defaultTickOffset configurationclassDiagram
class ChartAxisConfig {
+double initialTopBoundQuote
+double initialBottomBoundQuote
+double maxCurrentTickOffset
+double~nullable~ defaultTickOffset
+bool showQuoteGrid
+bool showEpochGrid
+double defaultIntervalWidth
+ChartAxisConfig copyWith(double~nullable~ initialTopBoundQuote, double~nullable~ initialBottomBoundQuote, double~nullable~ maxCurrentTickOffset, double~nullable~ defaultTickOffset)
}
class XAxisWrapper {
+ChartAxisConfig chartAxisConfig
+Widget build(BuildContext context)
}
class XAxisBase {
+int minEpoch
+int maxEpoch
+double? msPerPx
+double? minIntervalWidth
+double? maxIntervalWidth
+EdgeInsets? dataFitPadding
+double~nullable~ defaultTickOffset
+Duration scrollAnimationDuration
}
class XAxisMobile {
}
class XAxisWeb {
}
class XAxisModel {
-double _maxCurrentTickOffset
-double _defaultTickOffset
-int _maxEpoch
-int _rightBoundEpoch
+XAxisModel(int minEpoch, int maxEpoch, double maxCurrentTickOffset, double~nullable~ defaultTickOffset, double~nullable~ msPerPx, double~nullable~ minIntervalWidth, double~nullable~ maxIntervalWidth, EdgeInsets~nullable~ dataFitPadding, void Function(double scale)~nullable~ onScale, void Function(double delta)~nullable~ onScroll)
-int _shiftEpoch(int epoch, double offset)
}
ChartAxisConfig "1" --> "1" XAxisWrapper : used by
XAxisWrapper "1" --> "1" XAxisBase : creates
XAxisBase <|-- XAxisMobile
XAxisBase <|-- XAxisWeb
XAxisBase "1" --> "1" XAxisModel : configures
%% Highlight of defaultTickOffset flow
ChartAxisConfig : +defaultTickOffset
XAxisBase : +defaultTickOffset
XAxisModel : -_defaultTickOffset
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 —— View job PR Review: Default Tick Offset ImplementationSummaryThis PR successfully adds a Issues Found🔴 High Priority1. Incomplete The ChartAxisConfig copyWith({
double? initialTopBoundQuote,
double? initialBottomBoundQuote,
double? maxCurrentTickOffset,
double? defaultTickOffset,
}) =>
ChartAxisConfig(
initialTopBoundQuote: initialTopBoundQuote ?? this.initialTopBoundQuote,
initialBottomBoundQuote:
initialBottomBoundQuote ?? this.initialBottomBoundQuote,
maxCurrentTickOffset: maxCurrentTickOffset ?? this.maxCurrentTickOffset,
defaultTickOffset: defaultTickOffset ?? this.defaultTickOffset,
);Problem: The following constructor parameters are not included in
Impact: If someone calls Fix: Either add all missing fields to 🟡 Medium Priority2. Missing test coverage No tests were added for the new
Location: Should add tests in 3. Potential null-safety issue with clamping In _defaultTickOffset = defaultTickOffset?.clamp(0, _maxCurrentTickOffset) ??
_maxCurrentTickOffset;Concern: If Recommendation: Add validation or a comment explaining the assumption that 🟢 Low Priority / Suggestions4. Documentation could be more explicit While the documentation is good, it could be clearer about:
5. Missing changelog entry Consider adding an entry to Positive Observations✅ Good architectural design: The feature is implemented cleanly through the widget hierarchy (ChartAxisConfig → XAxisWrapper → XAxisBase → XAxisModel) ✅ Consistent documentation: All three places where the parameter is documented (ChartAxisConfig, XAxisBase, XAxisModel) have matching descriptions ✅ Backward compatible: Making ✅ Proper threading: The parameter is correctly passed through all layers (wrapper → mobile/web → base → model) RecommendationsBefore merging:
After merging:
ConclusionThe implementation is solid and follows the codebase patterns well. The main blocker is the incomplete |
![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> `lib/src/deriv_chart/chart/x_axis/x_axis_model.dart:89-90` </location>
<code_context>
_isLive = isLive;
_maxCurrentTickOffset = maxCurrentTickOffset;
- _rightBoundEpoch = _maxRightBoundEpoch;
+ _defaultTickOffset = defaultTickOffset?.clamp(0, _maxCurrentTickOffset) ??
+ _maxCurrentTickOffset;
+ _rightBoundEpoch = _shiftEpoch(_maxEpoch, _defaultTickOffset);
_dataFitMode = startWithDataFitMode;
</code_context>
<issue_to_address>
**issue (bug_risk):** Clamp result is `num`, which may not assign cleanly to `double` without an explicit cast.
Because `clamp` returns `num`, this expression is `num?`, so assigning it to a `double` field can cause analyzer issues in strong mode. Please make the bounds `double` and/or cast the result, e.g. `defaultTickOffset?.clamp(0.0, _maxCurrentTickOffset).toDouble()` or `(defaultTickOffset?.clamp(0.0, _maxCurrentTickOffset) as double?) ?? _maxCurrentTickOffset;`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
The |
Clickup link:
Fixes issue: #
This PR contains the following changes:
Developers Note (Optional)
Pre-launch Checklist (For PR creator)
As a creator of this PR:
Reviewers
Pre-launch Checklist (For Reviewers)
As a reviewer I ensure that:
Pre-launch Checklist (For QA)
Pre-launch Checklist (For Maintainer)
Summary by Sourcery
Add support for configuring a default X-axis tick offset used for initial chart load and scrolling to the latest tick, defaulting to the maximum tick offset when unspecified.
New Features:
Enhancements: