fix: keep data anchored to the left edge in data-fit mode - #492
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Reviewer's GuideAdds a persistent left-anchor behavior for charts initialized in data-fit mode, updates scroll bounds to respect that anchor, and clamps zoom-out to the data-fit scale so bounded-data charts keep their oldest entry fixed at the left edge and never zoom out past the point where all data already fits on screen. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Claude encountered an error —— 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 left some high level feedback:
- In
_scaleOutMsPerPxLimit, using_entries!.length * granularityassumes no gaps and consistent granularity; consider basing the duration on_maxEpoch - _minEpochso the zoom limit reflects the actual data span. - The
_anchorDataToLeftbehavior is currently tied only tostartWithDataFitMode; if future use cases require toggling this at runtime, consider exposing a controlled way to change it rather than keeping it strictlylateand immutable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_scaleOutMsPerPxLimit`, using `_entries!.length * granularity` assumes no gaps and consistent granularity; consider basing the duration on `_maxEpoch - _minEpoch` so the zoom limit reflects the actual data span.
- The `_anchorDataToLeft` behavior is currently tied only to `startWithDataFitMode`; if future use cases require toggling this at runtime, consider exposing a controlled way to change it rather than keeping it strictly `late` and immutable.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @behnam-deriv's task in 6m 27s —— View job I'll analyze this and get back to you. |
🤖 Claude PR Review CompleteModel: SummaryThis PR makes a focused, well-reasoned change to Recommendation: APPROVE 🟡 Medium Priority Issues🟡 1. No test coverage for the new left-anchor / zoom-out-limit behavior —
|
| Severity | File | Lines |
|---|---|---|
| MEDIUM | lib/src/deriv_chart/chart/x_axis/x_axis_model.dart |
209-264, 619-629 |
❌ Problematic Code:
The three new pieces of logic (_minRightBoundEpoch left-anchor branch, _scaleOutMsPerPxLimit, and the _clampRightBoundEpoch left-anchor fallback) ship with no accompanying test. This repo already has a pattern for constructing XAxisModel directly in tests (test/deriv_chart/chart/interactive_layer/interactive_layer_sync_test.dart:47-57), so this is straightforward to add.
📋 Issue: This is bound/scroll-limit math with several interacting edge cases (sparse data, width == null, _minRightBoundEpoch > _maxRightBoundEpoch). Without a test, a future refactor of _fitData, _maxCurrentTickOffset, or _dataFitPadding handling could silently reintroduce the "gap on the left" or "zoom out past data-fit" bugs this PR fixes.
✅ Fix:
test('anchors oldest entry to the left edge and blocks zoom past data-fit scale', () {
final XAxisModel model = XAxisModel(
entries: ticks, // e.g. 20 ticks, granularity 1000
granularity: 1000,
animationController: animationController,
isLive: false,
snapMarkersToIntervals: false,
maxCurrentTickOffset: 150,
startWithDataFitMode: true,
)
..width = 800
..graphAreaWidth = 740;
model.fitAvailableData();
final double fitMsPerPx = model.msPerPx;
// Panning right should not push the oldest entry past its data-fit
// position.
model.scrollBy(100000);
expect(model.xFromEpoch(ticks.first.epoch),
closeTo(defaultDataFitPadding.left, 1));
// Zooming out should not exceed the data-fit scale.
model.onScaleAndPanStart(ScaleStartDetails());
model.scale(0.01); // large zoom-out factor
expect(model.msPerPx, lessThanOrEqualTo(fitMsPerPx));
});💡 Explanation: Asserting on xFromEpoch/msPerPx (public API) rather than the private fields keeps the test resilient to internal refactors while still pinning down the externally-visible behavior this PR guarantees.
🟢 Low Priority Issues
🟢 2. Data-fit scale expression duplicated between _fitData() and _scaleOutMsPerPxLimit — lib/src/deriv_chart/chart/x_axis/x_axis_model.dart:252-264,411-425
Details
| Severity | File | Lines |
|---|---|---|
| LOW | lib/src/deriv_chart/chart/x_axis/x_axis_model.dart |
252-264, 411-425 |
❌ Problematic Code:
// _scaleOutMsPerPxLimit (new)
final double pxTargetDataWidth = width! - _dataFitPadding.horizontal;
if (pxTargetDataWidth > 0) {
final int msDataDuration = _entries!.length * granularity;
return (msDataDuration / pxTargetDataWidth).clamp(_minMsPerPx, _maxMsPerPx);
}
// _fitData (existing)
final int msDataDuration = _entries!.length * granularity;
final double pxTargetDataWidth = width! - _dataFitPadding.horizontal;
_msPerPx = (msDataDuration / pxTargetDataWidth).clamp(_minMsPerPx, _maxMsPerPx);📋 Issue: The doc comment on _scaleOutMsPerPxLimit ("Same expression as _fitData, so the limit lands exactly on the data-fit scale") calls out the coupling explicitly, but the expression itself is now copy-pasted in two places. Sourcery's review also flagged that _entries!.length * granularity ignores gaps in the data (weekends/off-hours) rather than using the actual _maxEpoch - _minEpoch span — a fair point, but since it mirrors _fitData's existing approximation, fixing only one side would make them diverge, which is worse than the status quo.
✅ Fix:
/// Ms-per-px scale that fits all of [_entries] within
/// `width - _dataFitPadding.horizontal` px, clamped to the zoom range.
double? _dataFitMsPerPx() {
if (width == null || !(_entries?.isNotEmpty ?? false)) {
return null;
}
final double pxTargetDataWidth = width! - _dataFitPadding.horizontal;
if (pxTargetDataWidth <= 0) {
return null;
}
final int msDataDuration = _entries!.length * granularity;
return (msDataDuration / pxTargetDataWidth).clamp(_minMsPerPx, _maxMsPerPx);
}
double get _scaleOutMsPerPxLimit =>
_anchorDataToLeft ? (_dataFitMsPerPx() ?? _maxMsPerPx) : _maxMsPerPx;
// in _fitData():
final double? fitMsPerPx = _dataFitMsPerPx();
if (fitMsPerPx != null) {
_msPerPx = fitMsPerPx;
_scrollTo(_shiftEpoch(lastEntryEpoch, _dataFitPadding.right));
}💡 Explanation: Extracting the shared calculation into one helper guarantees _scaleOutMsPerPxLimit and _fitData() can never disagree on what "the data-fit scale" is, and gives a single place to later switch to a gap-aware duration if that's ever needed.
Summary Table
| Priority | Count | Categories |
|---|---|---|
| 🔴 Critical | 0 | — |
| 🟠 High | 0 | — |
| 🟡 Medium | 1 | Missing test coverage |
| 🟢 Low | 1 | Duplicated data-fit scale calculation |
Recommendations
- Add a unit test exercising
startWithDataFitMode: truethrough the public API (fitAvailableData,scrollBy,scale) to lock in the left-anchor and zoom-out-limit behavior described in the PR body. - Consider extracting the shared "data-fit ms-per-px" expression into one helper used by both
_fitData()and_scaleOutMsPerPxLimitto prevent future drift between the two. - Confirmed no impact on the default (
startWithDataFitMode: false) live-chart path, and no impact on the manually-toggled "fit data" button on live charts (main_chart.dart:355) —_anchorDataToLeftstaysfalsefor those, which matches the PR's stated scope.
Auto Fix Claude Reviews
| Action | Open Dashboard |
|---|
1 participant
What
For charts started in data-fit mode, the oldest entry now stays anchored to the left edge, and zooming out stops at the data-fit scale.
Three changes in
XAxisModel:_anchorDataToLeftflag, set fromstartWithDataFitMode. Unlike_dataFitMode— which any pan or scale gesture turns off — this stays on for the lifetime of the model._minRightBoundEpochnow returns therightBoundEpochthat puts the oldest entry exactly where data-fit mode would (_dataFitPadding.leftpx from the left edge), instead of a bound derived frommaxCurrentTickOffset.scale()clamps against_scaleOutMsPerPxLimit— the data-fit scale — rather than_maxMsPerPx.When the data no longer spans the viewport, the left anchor wins over the
maxCurrentTickOffsetupper bound.Why
These charts show a bounded data window (e.g. contract details), not a live stream, so the old behaviour was wrong in two ways:
Scope
Only affects charts constructed with
startWithDataFitMode: true. Charts that start in follow-current-tick mode keep the existing bounds and zoom limits.🤖 Generated with Claude Code
Summary by Sourcery
Ensure charts initialized in data-fit mode keep their data anchored to the left edge and prevent zooming out beyond the data-fit scale.
Bug Fixes:
Enhancements: