codeant-ai-for-open-source[bot] commented on code in PR #35208:
URL: https://github.com/apache/superset/pull/35208#discussion_r3472314326
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:
##########
@@ -705,6 +705,18 @@ const Chart = (props: ChartProps) => {
height={getHeaderHeight()}
exportPivotExcel={exportPivotExcel as unknown as (arg0: string) =>
void}
chartHolderRef={props.chartHolderRef}
+ ownState={createOwnStateWithChartState(
+ (dataMask[props.id]?.ownState as JsonObject) || EMPTY_OBJECT,
+ {
+ state:
+ getChartStateWithFallback(
+ chartState as { state?: JsonObject } | undefined,
+ formData as JsonObject,
+ slice.viz_type,
+ ) ?? undefined,
+ },
+ slice.viz_type,
+ )}
Review Comment:
**Suggestion:** `ownState` is recomputed inline during every render and
passed as a new object reference each time. Since `ViewQueryModal` now
refetches when `ownState` changes, this can trigger repeated backend query
requests while the modal is open even when the effective state did not change.
Memoize the computed `ownState` (for example with `useMemo`) before passing it
down so refetching only happens on real state changes. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard "View query" issues duplicate chart data requests.
- ⚠️ Extra chart-data traffic loads backend unnecessarily during viewing.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. On a dashboard, a chart is rendered by `Chart` in
`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx:170-815`.
In
its JSX return, it passes an `ownState` prop into `SliceHeader` at lines
666-720, computed
inline as a new object on every render via `createOwnStateWithChartState`
(lines 708-719
and helper at 151-168).
2. `SliceHeader` in
`superset-frontend/src/dashboard/components/SliceHeader/index.tsx:145-402`
forwards this
`ownState` prop to `SliceHeaderControls` at line 359-392
(`ownState={ownState}` at
391-392), so every re-render of `Chart` produces a fresh `ownState` object
reference on
`SliceHeaderControls`.
3. `SliceHeaderControls` in
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:166-695`
uses
this prop when constructing the "View query" menu item (lines 479-501). The
`ModalTrigger`
renders `<ViewQueryModal latestQueryFormData={props.formData}
ownState={props.ownState}
/>` at 489-492, so each new `ownState` reference from `Chart` is passed down
to
`ViewQueryModal`.
4. `ViewQueryModal` in
`superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:53-88`
defines
`loadChartData` with `useCallback` depending on `latestQueryFormData` and
`ownState` (line
58 `useCallback` with deps at 84), and a `useEffect` that calls
`loadChartData('query')`
whenever that callback identity changes (lines 86-88). Because `Chart`
recomputes a new
`ownState` object every render (lines 708-719) without memoization, any
re-render of
`Chart` that doesn't actually change the logical `ownState` (for example,
toggling
`isStreamingModalVisible` in `Chart` at lines 266-278 or updating other
local state) still
passes a new `ownState` reference into `ViewQueryModal`, causing
`loadChartData` to change
identity and the effect to fire again. This results in repeated
`getChartDataRequest`
backend calls while the "View query" modal is open, even though the
effective ownState and
query haven't changed.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=219dfb8ac8a34bc785f20188db4e9ae3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=219dfb8ac8a34bc785f20188db4e9ae3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx
**Line:** 708:719
**Comment:**
*Performance: `ownState` is recomputed inline during every render and
passed as a new object reference each time. Since `ViewQueryModal` now
refetches when `ownState` changes, this can trigger repeated backend query
requests while the modal is open even when the effective state did not change.
Memoize the computed `ownState` (for example with `useMemo`) before passing it
down so refetching only happens on real state changes.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35208&comment_hash=a7b1011633955fb24cc79a74a8c1edf8e6bb35ee3515b803c08838e49899835c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35208&comment_hash=a7b1011633955fb24cc79a74a8c1edf8e6bb35ee3515b803c08838e49899835c&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx:
##########
@@ -48,38 +50,42 @@ const ViewQueryModalContainer = styled.div`
gap: ${({ theme }) => theme.sizeUnit * 4}px;
`;
-const ViewQueryModal: FC<Props> = ({ latestQueryFormData }) => {
+const ViewQueryModal: FC<Props> = ({ latestQueryFormData, ownState }) => {
const [result, setResult] = useState<Result[]>([]);
const [isLoading, setIsLoading] = useState(false);
const [error, setError] = useState<string | null>(null);
- const loadChartData = (resultType: string) => {
- setIsLoading(true);
- getChartDataRequest({
- formData: latestQueryFormData,
- resultFormat: 'json',
- resultType,
- })
- .then(({ json }) => {
- setResult(ensureIsArray(json.result) as Result[]);
- setIsLoading(false);
- setError(null);
+ const loadChartData = useCallback(
+ (resultType: string) => {
+ setIsLoading(true);
+ getChartDataRequest({
+ formData: latestQueryFormData,
+ resultFormat: 'json',
+ resultType,
+ ownState: ownState || {},
})
- .catch(response => {
- getClientErrorObject(response).then(({ error, message }) => {
- setError(
- error ||
- message ||
- response.statusText ||
- t('Sorry, An error occurred'),
- );
+ .then(({ json }) => {
+ setResult(ensureIsArray(json.result) as Result[]);
setIsLoading(false);
+ setError(null);
Review Comment:
**Suggestion:** The async request has no cancellation or stale-response
guard, so if multiple requests are fired (for example from rapid prop/state
changes), an older response can resolve last and overwrite newer results in the
modal. Add request sequencing or abort handling in `useEffect` cleanup so only
the latest in-flight request can update component state. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ View query can show SQL from outdated chart state.
- ⚠️ Explore and dashboard users may misinterpret stale queries.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Explore, the additional actions menu hook
`useExploreAdditionalActionsMenu`
(`superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx:194-1085`)
creates the "View query" menu item. At lines 1009-1023 it renders
`<ViewQueryModal
latestQueryFormData={latestQueryFormData as QueryFormData}
ownState={ownState} />` inside
a `ModalTrigger`, so the modal receives the current query form data and
ownState whenever
it is opened or when those props change.
2. `ViewQueryModal`
(`superset-frontend/src/explore/components/controls/ViewQueryModal.tsx`)
defines
`loadChartData` with `useCallback` at lines 58-84. Inside, it calls
`getChartDataRequest`
(imported from `chartAction.ts`) at lines 61-66 with `{ formData:
latestQueryFormData,
resultFormat: 'json', resultType, ownState: ownState || {} }`, and on
success runs
`setResult(...)` and clears loading/error at lines 67-71.
3. A `useEffect` at lines 86-88 calls `loadChartData('query')` whenever
`loadChartData`'s
identity changes. Since `useCallback` depends on `[latestQueryFormData,
ownState]` (line
84), every change to either prop while the modal stays open triggers a new
`getChartDataRequest` call without cancelling any in-flight request. For
example, in
Explore, repeatedly changing controls and re-running the chart while keeping
"View query"
open will send a new request on each change; similarly on dashboards,
`SliceHeaderControls` passes `props.formData` and `props.ownState` into
`ViewQueryModal`
at
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:489-492`,
so
rapid filter or state changes can also trigger multiple overlapping requests.
4. `getChartDataRequest` in
`superset-frontend/src/components/Chart/chartAction.ts:52-95`
simply forwards to `legacyChartDataRequest` or `v1ChartDataRequest`, both of
which use
`SupersetClient.post` with no abort logic or sequencing (see
`v1ChartDataRequest` at
453-50 and `getChartDataRequest` at 52-95). Because there is no cancellation
or "latest
request wins" guard in `ViewQueryModal`, if an older request resolves after
a newer one,
its `.then` handler at lines 67-71 will run last and overwrite `result`
state with stale
data. In practice this manifests as "View query" occasionally showing SQL
from a previous
chart state when users change form controls and trigger multiple backend
queries in quick
succession while the modal remains open.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=806479cc0f1d4734b5ecab667d0cedb9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=806479cc0f1d4734b5ecab667d0cedb9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/controls/ViewQueryModal.tsx
**Line:** 61:70
**Comment:**
*Race Condition: The async request has no cancellation or
stale-response guard, so if multiple requests are fired (for example from rapid
prop/state changes), an older response can resolve last and overwrite newer
results in the modal. Add request sequencing or abort handling in `useEffect`
cleanup so only the latest in-flight request can update component state.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35208&comment_hash=07a84f96bf566466e229d13110f6d7b4fa9b6ad85e5aef02d19839bbaf8247f5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35208&comment_hash=07a84f96bf566466e229d13110f6d7b4fa9b6ad85e5aef02d19839bbaf8247f5&reaction=dislike'>👎</a>
--
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]
