rlei-odes opened a new pull request, #43228:
URL: https://github.com/apache/superset/pull/43228
### SUMMARY
I hit this while adding a chart to a dashboard: the page died outright with a
React error screen instead of degrading, and it happened again later while I
was
just browsing other dashboards.
`chartReducer` routes every action through a shared handler map:
```ts
if (action.type in actionHandlers) {
return {
...charts,
[action.key]: actionHandlers[action.type](charts[action.key]),
};
}
```
Every handler reads the chart's previous state, so if `action.key` is not in
`charts` the handler receives `undefined`. Most handlers then throw, which
takes
down the whole page rather than degrading.
In my case it surfaced as:
```
TypeError: Cannot read properties of undefined (reading 'queryController')
at Object.CHART_UPDATE_STOPPED (src/components/Chart/chartReducer.ts)
at chartReducer (src/components/Chart/chartReducer.ts)
```
`CHART_UPDATE_STOPPED` is the handler that surfaces it, because it
dereferences
`state.queryController`. It only throws when the action carries a controller
—
otherwise the `action.queryController &&` short-circuit hides the problem —
and
the dispatch site that always passes one is the abort branch in
`chartAction.ts`:
```ts
// Abort is expected: filters changed, chart unmounted, etc.
return dispatch(chartUpdateStopped(key as string | number, controller));
```
Its own comment lists an unmounted chart as an expected reason for the
abort, so
an action arriving after the chart has left the store is a supported case
that
the reducer cannot currently survive. The block immediately below it already
guards for this (`getState().charts?.[key]?.queryController`), which
suggests the
intent was there but the abort path was missed.
This adds the guard in the reducer rather than at the dispatch site, because
all
of the handlers share the assumption, not just this one. **Happy to move it
into
`chartAction.ts` instead if maintainers prefer the fix closer to the cause**
—
that would fix the crash I saw but leave the other handlers exposed to the
same
shape of bug.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — the before state is a full-page React error screen with the
stack trace above; the after state is that the action is ignored and the app
carries on.
### TESTING INSTRUCTIONS
The behaviour is covered by a unit test added in this PR. Without the fix it
fails with the `TypeError` above; with it, it passes:
```bash
cd superset-frontend
npx jest src/components/Chart/chartReducers.test.ts
```
The test dispatches an action for a chart key that is not in state, in the
shape
the abort path produces (with an `AbortController`):
```ts
test('ignores an action for a chart that is no longer in state', () => {
const action = actions.chartUpdateStopped(999, new AbortController());
expect(() => chartReducer(charts, action)).not.toThrow();
expect(chartReducer(charts, action)).toEqual(charts);
});
```
To confirm nothing regressed for the normal path, the existing reducer suites
still pass:
```bash
npx jest src/components/Chart src/dashboard/reducers src/explore/reducers
```
I have not been able to derive reliable click-by-click UI steps that trigger
the
crash, which is why the unit test carries the argument rather than a
recording.
### ADDITIONAL INFORMATION
- [x] Has associated issue: No — found while building a custom chart plugin
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in SIP-59)
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
