geido commented on code in PR #43458:
URL: https://github.com/apache/superset/pull/43458#discussion_r3934322840
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -246,6 +244,7 @@ export default function ExplorePage() {
? makeApi<void, { result: Chart }>({
method: 'GET',
endpoint: `api/v1/chart/${chartId}`,
+ signal: controller.signal,
Review Comment:
Confirmed and fixed in 69782fde99.
Worth recording how it reproduces, because it is easy to convince yourself
it does not: the abort rejection does not reach the handler immediately. It
goes through `fetch-retry` with `DEFAULT_FETCH_RETRY_OPTIONS = { retries: 3,
retryDelay: 1000 }`, so it surfaces roughly 3s later. My first attempt at a
regression test flushed only a microtask, saw no dispatch, and passed while
proving nothing. With an adequate flush the pre-fix code does exactly what you
described — `isStale()` is false and `hydrateExplore` dispatches into the
unmounted page.
Fixed by folding the signal into the staleness check rather than adding a
guard per call site:
```ts
const isStale = () =>
generation !== fetchGeneration.current || controller.signal.aborted;
```
That covers the `.then`, the `.catch` you pointed at, and the `.finally`
uniformly, and makes the `&& !controller.signal.aborted` in `.finally`
redundant, so it is gone. `does not hydrate explore when unmount aborts the
chart metadata request` in `Chart.test.tsx` guards it; it carries a 4s wait
tied to those retry constants, which I called out in a comment since it is the
fragile part.
--
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]
