codeant-ai-for-open-source[bot] commented on code in PR #36639:
URL: https://github.com/apache/superset/pull/36639#discussion_r2619212184
##########
superset-frontend/src/components/Chart/chartAction.js:
##########
@@ -476,7 +476,17 @@ export function exploreJSON(
return dispatch(chartUpdateSucceeded(queriesResponse, key));
})
.catch(response => {
+ // Ignore abort errors - they're expected when filters change quickly
+ if (
+ response?.name === 'AbortError' ||
+ response?.message === 'Request aborted'
+ ) {
Review Comment:
**Suggestion:** Fragile/incorrect abort detection: checking
`response?.message === 'Request aborted'` is unreliable across environments and
implementations; normalize abort detection to check well-known abort indicators
(`name === 'AbortError'` or status/statusText === 'abort' or a case-insensitive
message contains "abort") to avoid misclassifying aborts as regular errors.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
const isAbort =
response?.name === 'AbortError' ||
response?.statusText === 'abort' ||
response?.status === 'abort' ||
(typeof response?.message === 'string' &&
response.message.toLowerCase().includes('abort'));
if (isAbort) {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing check (name === 'AbortError' or message === 'Request aborted')
is somewhat brittle across runtimes and fetch wrappers. Expanding the
detection to also check statusText/status and a case-insensitive message
match makes the abort-detection more robust and reduces false-positive
error handling. The suggested improvement is sensible and addresses a
real class of misclassified aborts.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/chartAction.js
**Line:** 480:483
**Comment:**
*Possible Bug: Fragile/incorrect abort detection: checking
`response?.message === 'Request aborted'` is unreliable across environments and
implementations; normalize abort detection to check well-known abort indicators
(`name === 'AbortError'` or status/statusText === 'abort' or a case-insensitive
message contains "abort") to avoid misclassifying aborts as regular errors.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/components/Chart/chartAction.js:
##########
@@ -476,7 +476,17 @@ export function exploreJSON(
return dispatch(chartUpdateSucceeded(queriesResponse, key));
})
.catch(response => {
+ // Ignore abort errors - they're expected when filters change quickly
+ if (
+ response?.name === 'AbortError' ||
+ response?.message === 'Request aborted'
+ ) {
+ // Abort is expected: filters changed, chart unmounted, etc.
+ return dispatch(chartUpdateStopped(key));
+ }
+
if (isFeatureEnabled(FeatureFlag.GlobalAsyncQueries)) {
+ // In async mode we just pass the raw error response through
return dispatch(chartUpdateFailed([response], key));
Review Comment:
**Suggestion:** In GlobalAsyncQueries mode the code dispatches
`chartUpdateFailed` with the raw `response` (which can be a DOMException or
other non-normalized error). This can break downstream reducers/UI that expect
a normalized error shape; normalize the error via
`getClientErrorObject(response)` before dispatching `chartUpdateFailed`. [logic
error]
**Severity Level:** Minor ⚠️
```suggestion
// In async mode normalize the error before dispatching so
reducers/UI get a consistent shape
return getClientErrorObject(response).then(parsed =>
dispatch(chartUpdateFailed([parsed], key)),
);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Dispatching the raw response can produce inconsistent shapes for reducers/UI.
Normalizing via getClientErrorObject(response) before dispatch keeps error
payloads consistent with the non-async path and avoids surprises downstream.
This is a functional improvement, not just style.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/chartAction.js
**Line:** 489:490
**Comment:**
*Logic Error: In GlobalAsyncQueries mode the code dispatches
`chartUpdateFailed` with the raw `response` (which can be a DOMException or
other non-normalized error). This can break downstream reducers/UI that expect
a normalized error shape; normalize the error via
`getClientErrorObject(response)` before dispatching `chartUpdateFailed`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/components/Chart/chartAction.js:
##########
@@ -494,10 +504,7 @@ export function exploreJSON(
}),
);
};
- if (response.name === 'AbortError') {
- appendErrorLog('abort');
- return dispatch(chartUpdateStopped(key));
- }
+
return getClientErrorObject(response).then(parsedResponse => {
if (response.statusText === 'timeout') {
Review Comment:
**Suggestion:** The code calls
`getClientErrorObject(response).then(parsedResponse => { ... })` but still
checks `response.statusText === 'timeout'` inside the then; this is incorrect —
the parsed error should be used for statusText/error fields. Use
`parsedResponse.statusText` in the timeout check and subsequent logic.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
if (parsedResponse.statusText === 'timeout') {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
After awaiting getClientErrorObject the code should consult the normalized
parsedResponse for fields like statusText. Checking the original response
is incorrect and can mis-handle timeouts; swapping to
parsedResponse.statusText
fixes a real bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/chartAction.js
**Line:** 509:509
**Comment:**
*Possible Bug: The code calls
`getClientErrorObject(response).then(parsedResponse => { ... })` but still
checks `response.statusText === 'timeout'` inside the then; this is incorrect —
the parsed error should be used for statusText/error fields. Use
`parsedResponse.statusText` in the timeout check and subsequent logic.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
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]
