feat: Add adaptive interval feature #396
Draft
Conversation
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
Reviewer's GuideThis PR implements an adaptive interval (auto-interval) feature that dynamically adjusts chart time granularity based on zoom level. It introduces new configuration and observer interfaces, wraps core chart widgets in an AutoIntervalWrapper, integrates zoom-level callbacks into the XAxisModel and chart lifecycle, adds animated transitions for granularity changes, updates the example app with toggles and zoom controls, and provides documentation and public API exports for the new feature. Sequence diagram for adaptive interval granularity suggestion flowsequenceDiagram
participant User as actor User
participant Chart
participant AutoIntervalWrapper
participant XAxisModel
participant ParentApp as Parent App
User->>Chart: Zooms in/out (via UI)
Chart->>XAxisModel: scale(zoomStep)
XAxisModel->>AutoIntervalWrapper: onZoomLevelChanged(msPerPx, granularity)
AutoIntervalWrapper->>ParentApp: onGranularityChangeRequested(suggestedGranularity)
ParentApp->>Chart: Update granularity
Chart->>XAxisModel: Update granularity
Note over Chart,AutoIntervalWrapper: Chart re-renders with new granularity
Class diagram for adaptive interval feature integrationclassDiagram
class ChartAxisConfig {
+bool autoIntervalEnabled
+List<AutoIntervalZoomRange> autoIntervalZoomRanges
+Duration autoIntervalTransitionDuration
+copyWith(...)
}
class AutoIntervalZoomRange {
+int granularity
+double minPixelsPerInterval
+double maxPixelsPerInterval
+double optimalPixelsPerInterval
}
class AutoIntervalWrapper {
+bool enabled
+int granularity
+List<AutoIntervalZoomRange> zoomRanges
+void Function(int)? onGranularityChangeRequested
}
class ZoomLevelObserver {
+void onZoomLevelChanged(double msPerPx, int currentGranularity)
}
class XAxisModel {
+bool autoIntervalEnabled
+ZoomLevelObserver? zoomLevelObserver
+void scale(double scale)
}
class Chart {
+OnGranularityChangeRequestedCallback? onGranularityChangeRequested
}
class DerivChart {
+void Function(int)? onGranularityChangeRequested
}
ChartAxisConfig --> AutoIntervalZoomRange : uses
AutoIntervalWrapper ..|> ZoomLevelObserver : implements
XAxisModel --> ZoomLevelObserver : uses
Chart --> AutoIntervalWrapper : wraps
DerivChart --> Chart : uses
Chart --> ChartAxisConfig : uses
Chart --> XAxisModel : uses
AutoIntervalWrapper --> AutoIntervalZoomRange : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @behnam-deriv - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/src/deriv_chart/chart/auto_interval/auto_interval_wrapper.dart:59` </location>
<code_context>
+ implements ZoomLevelObserver {
+ late int _currentGranularity;
+ double? _currentMsPerPx;
+ int? _lastSuggestedGranularity;
+
+ @override
</code_context>
<issue_to_address>
Consider thread-safety and race conditions for _lastSuggestedGranularity.
Ensure the logic for _lastSuggestedGranularity handles rapid zoom changes and multiple post-frame callbacks, as race conditions may occur if the widget is rebuilt or disposed between scheduling and execution.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| implements ZoomLevelObserver { | ||
| late int _currentGranularity; | ||
| double? _currentMsPerPx; | ||
| int? _lastSuggestedGranularity; |
There was a problem hiding this comment.
issue (bug_risk): Consider thread-safety and race conditions for _lastSuggestedGranularity.
Ensure the logic for _lastSuggestedGranularity handles rapid zoom changes and multiple post-frame callbacks, as race conditions may occur if the widget is rebuilt or disposed between scheduling and execution.
There was a problem hiding this comment.
Hey @behnam-deriv - I've reviewed your changes - here's some feedback:
- Consider removing the
kIsWebcheck in XAxisModel.scale so that zoom-level notifications fire on all platforms rather than only on web. - It may be helpful to debounce or throttle the
onGranularityChangeRequestedcallback to avoid rapid successive granularity-change requests during fast or continuous zoom actions. - In AutoIntervalWrapper, add a fallback for zoom levels that fall outside the configured ranges so you can default to the nearest valid granularity instead of doing nothing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider removing the `kIsWeb` check in XAxisModel.scale so that zoom-level notifications fire on all platforms rather than only on web.
- It may be helpful to debounce or throttle the `onGranularityChangeRequested` callback to avoid rapid successive granularity-change requests during fast or continuous zoom actions.
- In AutoIntervalWrapper, add a fallback for zoom levels that fall outside the configured ranges so you can default to the nearest valid granularity instead of doing nothing.
## Individual Comments
### Comment 1
<location> `lib/src/deriv_chart/chart/x_axis/x_axis_model.dart:499` </location>
<code_context>
void scale(double scale) {
_msPerPx = (_prevMsPerPx! / scale).clamp(_minMsPerPx, _maxMsPerPx);
onScale?.call();
+ if (kIsWeb) {
+ zoomLevelObserver?.onZoomLevelChanged(_msPerPx, _granularity);
+ }
notifyListeners();
</code_context>
<issue_to_address>
Zoom level observer is only notified on web during scale, but not on other platforms.
Currently, zoomLevelObserver is only updated on web. If cross-platform consistency is required, review whether restricting this to kIsWeb is appropriate.
</issue_to_address>
### Comment 2
<location> `example/lib/main.dart:290` </location>
<code_context>
- WidgetsBinding.instance.addPostFrameCallback(
- (Duration timeStamp) => _controller.scrollToLastTick(),
- );
+ if (!_adaptiveInterval) {
+ WidgetsBinding.instance.addPostFrameCallback(
+ (Duration timeStamp) => _controller.scrollToLastTick(),
+ );
</code_context>
<issue_to_address>
scrollToLastTick is skipped when adaptive interval is enabled, which may affect user experience.
If auto-scrolling to the latest tick is still desired with adaptive interval enabled, consider decoupling these behaviors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| _triggerScrollMomentum(details.velocity); | ||
| } | ||
|
|
||
| zoomLevelObserver?.onZoomLevelChanged(_msPerPx, _granularity); |
There was a problem hiding this comment.
issue (bug_risk): Zoom level observer is only notified on web during scale, but not on other platforms.
Currently, zoomLevelObserver is only updated on web. If cross-platform consistency is required, review whether restricting this to kIsWeb is appropriate.
|
|
||
| _resetCandlesTo(historyCandles); | ||
| } | ||
|
|
There was a problem hiding this comment.
question: scrollToLastTick is skipped when adaptive interval is enabled, which may affect user experience.
If auto-scrolling to the latest tick is still desired with adaptive interval enabled, consider decoupling these behaviors.
There was a problem hiding this comment.
Hey @behnam-deriv - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/src/deriv_chart/chart/auto_interval/auto_interval_wrapper.dart:59` </location>
<code_context>
+ implements ZoomLevelObserver {
+ late int _currentGranularity;
+ double? _currentMsPerPx;
+ int? _lastSuggestedGranularity;
+
+ @override
</code_context>
<issue_to_address>
Consider debouncing granularity change requests to avoid rapid toggling.
Rapid zoom level changes near thresholds can cause repeated granularity updates. Adding debounce or hysteresis would reduce redundant API calls and enhance user experience.
Suggested implementation:
```
late int _currentGranularity;
double? _currentMsPerPx;
int? _lastSuggestedGranularity;
Timer? _debounceTimer;
```
```
int? _lastSuggestedGranularity;
void _requestGranularityChange(int newGranularity) {
if (_debounceTimer?.isActive ?? false) {
_debounceTimer?.cancel();
}
_debounceTimer = Timer(const Duration(milliseconds: 200), () {
if (_currentGranularity != newGranularity) {
setState(() {
_currentGranularity = newGranularity;
});
// Place any additional logic for granularity change here, e.g., API calls.
}
});
}
@override
void dispose() {
_debounceTimer?.cancel();
super.dispose();
}
```
You will need to replace any direct updates to `_currentGranularity` (or wherever granularity is changed in response to zoom/thresholds) with calls to `_requestGranularityChange(newGranularity)`. This ensures all changes are debounced.
</issue_to_address>
### Comment 2
<location> `lib/src/deriv_chart/chart/x_axis/x_axis_model.dart:337` </location>
<code_context>
_granularity = newGranularity;
- _msPerPx = _defaultMsPerPx;
- _scrollTo(_maxRightBoundEpoch);
+ if (!_autoIntervalEnabled) {
+ _msPerPx = _defaultMsPerPx;
+ _scrollTo(_maxRightBoundEpoch);
</code_context>
<issue_to_address>
Clearing ticks only when auto-interval is disabled may cause stale data if granularity changes while enabled.
If granularity changes with auto-interval enabled, ticks aren't cleared, which may cause mismatched data. Please confirm if this is intended or if further handling is required.
</issue_to_address>
### Comment 3
<location> `lib/src/models/chart_axis_config.dart:15` </location>
<code_context>
const double defaultMaxCurrentTickOffset = 150;
+/// {@template auto_interval_zoom_range}
+/// Configuration for auto-interval zoom ranges.
+/// Maps granularity (in milliseconds) to the optimal pixel range for that interval.
+/// {@endtemplate}
</code_context>
<issue_to_address>
All defaultAutoIntervalRanges use the same min/max pixel values.
Consider adjusting min/max pixel values per granularity to ensure the auto-interval feature differentiates between zoom levels as intended.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| implements ZoomLevelObserver { | ||
| late int _currentGranularity; | ||
| double? _currentMsPerPx; | ||
| int? _lastSuggestedGranularity; |
There was a problem hiding this comment.
suggestion (performance): Consider debouncing granularity change requests to avoid rapid toggling.
Rapid zoom level changes near thresholds can cause repeated granularity updates. Adding debounce or hysteresis would reduce redundant API calls and enhance user experience.
Suggested implementation:
late int _currentGranularity;
double? _currentMsPerPx;
int? _lastSuggestedGranularity;
Timer? _debounceTimer;
int? _lastSuggestedGranularity;
void _requestGranularityChange(int newGranularity) {
if (_debounceTimer?.isActive ?? false) {
_debounceTimer?.cancel();
}
_debounceTimer = Timer(const Duration(milliseconds: 200), () {
if (_currentGranularity != newGranularity) {
setState(() {
_currentGranularity = newGranularity;
});
// Place any additional logic for granularity change here, e.g., API calls.
}
});
}
@override
void dispose() {
_debounceTimer?.cancel();
super.dispose();
}
You will need to replace any direct updates to _currentGranularity (or wherever granularity is changed in response to zoom/thresholds) with calls to _requestGranularityChange(newGranularity). This ensures all changes are debounced.
| _granularity = newGranularity; | ||
| _msPerPx = _defaultMsPerPx; | ||
| _scrollTo(_maxRightBoundEpoch); | ||
| if (!_autoIntervalEnabled) { |
There was a problem hiding this comment.
question (bug_risk): Clearing ticks only when auto-interval is disabled may cause stale data if granularity changes while enabled.
If granularity changes with auto-interval enabled, ticks aren't cleared, which may cause mismatched data. Please confirm if this is intended or if further handling is required.
| const double defaultMaxCurrentTickOffset = 150; | ||
|
|
||
| /// {@template auto_interval_zoom_range} | ||
| /// Configuration for auto-interval zoom ranges. |
There was a problem hiding this comment.
suggestion: All defaultAutoIntervalRanges use the same min/max pixel values.
Consider adjusting min/max pixel values per granularity to ensure the auto-interval feature differentiates between zoom levels as intended.
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.
Clickup link: https://app.clickup.com/t/20696747/GRWT-6233
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
Enable automatic and manual granularity adjustments by introducing an adaptive interval system, wrapping chart components in AutoIntervalWrapper, and integrating new configuration, callbacks, and UI controls to manage zoom-driven interval changes.
New Features:
Enhancements:
Documentation: