fix(tooltip): guard against removed series in cached axis tooltip params (Fixes #21732) - #21737
fix(tooltip): guard against removed series in cached axis tooltip params (Fixes #21732)#21737waterWang wants to merge 1 commit into
Conversation
Fixes apache#21732 _showAxisTooltip iterates `axisItem.seriesDataIndices` whose seriesIndex values come from the cached `_lastDataByCoordSys` pointer state. When a merged setOption removes series (e.g. `replaceMerge: ['series']`), those indices become stale and `getSeriesByIndex` returns undefined, causing: Uncaught TypeError: Cannot read properties of undefined (reading getDataParams) Also moved the `!axisModel` guard before the `axisModel.axis` access so a removed axis handles the same way instead of crashing earlier.
|
Thanks for your contribution! Please DO NOT commit the files in dist, i18n, and ssr/client/dist folders in a non-release pull request. These folders are for release use only. To reviewers: If this PR is going to be described in the changelog in the future release, please make sure this PR has one of the following labels: This message is shown because the PR description doesn't contain the document related template. |
There was a problem hiding this comment.
🟡 Changes recommended
Cached tooltip content can remain stale, and regression coverage is missing.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens cached axis-tooltip refreshes after merged updates remove axes or series.
Changes:
- Guards missing axis models before dereferencing.
- Skips stale series references during tooltip generation.
- Adds defensive handling, but cached tooltip content may remain stale.
File summaries
| File | Review summary |
|---|---|
src/component/tooltip/TooltipView.ts |
Adds axis and series guards. Two moderate findings remain about stale cached content, plus one nit requesting regression coverage. |
Review details
Suppressed comments (2)
src/component/tooltip/TooltipView.ts:558
- This short-circuit avoids the null dereference, but it leaves
_lastDataByCoordSyspointing at the same cached data._updateContentNotChangedOnAxiscan therefore compare the cache against itself, returntrue, and only move the existing tooltip instead of rebuilding it; the tooltip can continue displaying the removed axis's old content. Mark the cached content invalid when this guard fires so the tooltip is rebuilt or hidden.
if (!axisModel || axisValue == null) {
src/component/tooltip/TooltipView.ts:583
- Please add a regression case for the cached axis-tooltip path: keep the pointer over a chart with several series, then call merged
setOptionwithreplaceMerge: ['series', 'xAxis', 'yAxis']so the asynchronous_keepShowcallback processes stale indices. The repository has browser tooltip/setOption cases, but this change adds no coverage for the uncaught stale-series/removed-axis scenario, so this guard could regress unnoticed.
if (!series) {
return;
}
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| each(axisItem.seriesDataIndices, function (idxItem) { | ||
| const series = ecModel.getSeriesByIndex(idxItem.seriesIndex); | ||
| if (!series) { |
Fixes #21732
Problem
TooltipView._showAxisTooltipiteratesaxisItem.seriesDataIndiceswhoseseriesIndexvalues come from the cached pointer state (_lastDataByCoordSys). When a mergedsetOptionremoves series (e.g.replaceMerge: ['series', ...]) while the tooltip is still showing, those cached indices become stale andecModel.getSeriesByIndex(idxItem.seriesIndex)returnsundefined, causing:Root cause
TooltipViewcaches the last hovered pointer state in_lastX/_lastY/_lastDataByCoordSys.setOptionrunsTooltipView.render()→_keepShow(), which re-shows the tooltip with the cacheddataByCoordSysafter the update.getSeriesByIndexreturnsundefined→series.getDataParams(...)throws.Fix
Two defensive guards in
_showAxisTooltip:seriesDataIndicesentries whose series no longer exists:if (!series) { return; }!axisModelguard before theaxisModel.axisaccess (it was previously checked after being dereferenced), so a removed axis also short-circuits instead of crashing.Test
With
tooltip: {trigger: 'axis'}, hover to show the tooltip, thensetOptionwith fewer series in merge mode — the tooltip no longer throws and safely skips removed series.