AG-17505, AG-17507 Fix duplicate async data calls on lazy zoom - #7896
AG-17505, AG-17507 Fix duplicate async data calls on lazy zoom#7896lsjroberts wants to merge 6 commits into
Conversation
A lazily loaded chart has no axis domain until its first response lands, so `unsafeClamp` read `domainMin`/`domainMax` off `undefined` and threw on the first pointer event. The casts asserted `[Date, Date]`, so the compiler could not see it. Fixed here rather than separately because it blocks the verification of the initial-window behaviour in the following commit.
A `rangeX` memento needs the axis domain to convert to a ratio, which a lazily loaded chart does not have on its first render. The restore fell back to the full range and discarded the memento, so the requested window was ignored outright on every axis whose domain is not known from its options, and a corrective second request followed once data arrived. The requested range is already stated in data space, so it needs no domain to interpret: keep the memento pending until it can be converted, and request the range itself in the meantime. That both honours the request on the first fetch and gives the axis the domain it needs to resolve the memento, so lazy `rangeX` now restores the same zoom as it does from static data. Also stop a domainless scale's `undefined`/invalid-Date inversions reaching `dataSource.getData` as window bounds.
✅ Live-test this PR in PlunkerPaste these two <script src="https://ag-grid.github.io/ag-charts/pr-7896/ag-charts-community.min.js"></script>
<script src="https://ag-grid.github.io/ag-charts/pr-7896/ag-charts-enterprise.min.js"></script>Bundles are removed automatically when the PR is closed. Updated on every push. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| private getPendingWindow(): AgDataSourceCallbackParams | undefined { | ||
| const range = this.ctx.zoomManager?.getPendingRangeX(); | ||
| if (!range) return; | ||
|
|
There was a problem hiding this comment.
ℹ️ [P2] One-sided range windows are discarded
A zoom range may omit either endpoint, but this check requires both to be present. Consequently, a valid start-only or end-only initial range produces an unbounded first request and requires a corrective request after data loads. Preserve whichever valid endpoint is supplied and leave the other callback parameter undefined.
There was a problem hiding this comment.
Fixed in a996726. getPendingWindow now normalises each bound independently and bails only when both are unusable, so a one-sided range is forwarded as a half-open window.
Normalising the bounds alone was not enough to remove the corrective request, though: a windowEnd of undefined never compares equal to the resolved domain-edge value, so shouldRefresh still saw a changed window and fetched again. shouldRefresh now treats a bound the pending request left open as satisfied by the resolved bound that replaces it, since the open bound already asked for everything up to the domain edge.
Also unwraps a grouping value's value, which was discarded for the same reason. bigint still falls through — windowStart does not accept one.
Covered by requests a %s-only range once and requests the range once for a grouping-valued range in dataSource.test.ts.
| private getPendingWindow(): AgDataSourceCallbackParams | undefined { | ||
| const range = this.ctx.zoomManager?.getPendingRangeX(); | ||
| if (!range) return; | ||
|
|
There was a problem hiding this comment.
ℹ️ [P2] Non-finite window bounds pass validation
isNumber accepts values such as NaN and infinities, and isDate may accept an invalid Date object. These values are now forwarded directly to getData, whereas resolving through the scale would not produce a usable window. Validate finite numbers and valid dates before constructing the request.
There was a problem hiding this comment.
Guards tightened in a996726 — isFiniteNumber and isValidDate in place of isNumber and isDate.
For the record, though, these values cannot reach getData by the route described. A memento is decoded through JSON.parse(JSON.stringify(...)), which turns Infinity and NaN into null, and guardMemento then rejects the whole memento with Option rangeX.end cannot be set to null — measured, not assumed. An invalid Date throws earlier still, during encode. So the change is defence-in-depth on a function that no longer has a single caller, not a fix for a reachable defect, and there is no test for it.
|
✅ Codex review complete; 2 issues found (P0: 0 | P1: 0 | P2: 2 | P3: 0) View full reviewAG-17505 Fix double async data call on initialState.rangeX/ratioXPR: #7896 SummaryThis PR defers range-based zoom restoration until axis domains exist and uses pending data-space ranges for the initial data-source request. FindingsP0: 0 | P1: 0 | P2: 2 | P3: 0 2 of 2 finding(s) also posted inline; all findings are listed below. ℹ️ [P2] One-sided range windows are discarded
A zoom range may omit either endpoint, but this check requires both to be present. Consequently, a valid start-only or end-only initial range produces an unbounded first request and requires a corrective request after data loads. Preserve whichever valid endpoint is supplied and leave the other callback parameter undefined. ℹ️ [P2] Non-finite window bounds pass validation
VerdictAssessment: correct The main lazy-domain restoration flow is sound, but pending-window validation mishandles partial and invalid ranges. Required Actions:
|
The pending-window path required both bounds and both to be plain values, so a range stating only one endpoint, or stating it as a grouping value, fell back to an unbounded first request and a correction once the domain existed. Bounds are now normalised individually, and a bound left open by the pending request is treated as satisfied by the resolved bound that replaces it.
A grouping value is the one bound shape carrying its value in a nested field, so keying the unwrap on `value` alone would also strip a serialisable date or bigint down to its raw payload.
d78c1da to
2121810
Compare
…ount A band-positioned time axis addresses its domain by ordinal, so the ratios spanning a value range depend on how many points that range holds. The preserveDomain strategy interpolated those ratios linearly in domain-value space, which restored a different value range whenever a response changed the density inside the window: the window jumped forwards and the load it triggered cost an extra request. Snapshot the range itself for those axes and re-convert it through the scale, the same round-trip a zoom memento restore uses. Note that on such an axis the zoom ratio is the share of bands visible, which is also the navigator mask width, so holding the value range means the mask now moves with the response density. Holding the mask instead is what `zoom.onDataChange.strategy: 'preserveRatios'` selects.
A re-derived zoom differs from the applied one by a few ULPs, which an exact comparison reads as a new window to fetch.
lsjroberts
changed the title
|
on hold while away, will review when back |
1 participant
https://ag-grid.atlassian.net/browse/AG-17505
https://ag-grid.atlassian.net/browse/AG-17507
Fix #AG-17505
Fix #AG-17507