codeant-ai-for-open-source[bot] commented on code in PR #37501:
URL: https://github.com/apache/superset/pull/37501#discussion_r2748362168
##########
superset-frontend/src/explore/components/ExploreChartPanel/index.tsx:
##########
@@ -143,11 +165,9 @@ const ExploreChartPanel = ({
can_download: canDownload,
}: ExploreChartPanelProps) => {
const theme = useTheme();
- const gutterMargin = theme.sizeUnit * GUTTER_SIZE_FACTOR;
const gutterHeight = theme.sizeUnit * GUTTER_SIZE_FACTOR;
const {
ref: chartPanelRef,
- observerRef: resizeObserverRef,
width: chartPanelWidth,
height: chartPanelHeight,
Review Comment:
**Suggestion:** The `updateQueryContext` callback captures `force` (and
potentially other values) but its dependency array only includes `[slice]`, so
it will use a stale `force` value when those change; include all referenced
external variables in the dependency array to avoid stale closures. [race
condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Persisted chart query_context may use wrong force flag.
- ⚠️ /api/v1/chart/{slice_id} PUT called with stale payload.
- ⚠️ Chart metadata persistence logic affected in Explore UI.
```
</details>
```suggestion
[slice, force],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Explore chart panel component defined in
superset-frontend/src/explore/components/ExploreChartPanel/index.tsx. The
callback
updateQueryContext is declared at index.tsx (see line indicated above) and
is memoized
with useCallback.
2. Ensure the chart Slice exists and its stored query_context is null
(slice.query_context
=== null). This condition is checked inside updateQueryContext
(index.tsx:150-170).
3. Trigger a prop change that toggles the `force` prop passed into
ExploreChartPanel (for
example, a parent action that sets force to true to force-refresh the
chart). Because
updateQueryContext's dependency array only contains [slice], the callback is
not
recomputed when `force` changes.
4. The useEffect that calls updateQueryContext (index.tsx — the effect
invoking
updateQueryContext is immediately after the callback declaration) will
invoke the stale
updateQueryContext closure which uses the old `force` value when building
the query
context and when calling
SupersetClient.put('/api/v1/chart/${slice.slice_id}'), causing
the persisted query_context to be generated with an incorrect `force` value.
Note: This is not a hypothetical path — the file contains updateQueryContext
(index.tsx)
and a subsequent useEffect that calls it. The missing dependency on `force`
means current
code intentionally captures stale `force`; adding `force` to the dependency
array aligns
the callback lifetime with referenced values.
```
</details>
<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/ExploreChartPanel/index.tsx
**Line:** 172:172
**Comment:**
*Race Condition: The `updateQueryContext` callback captures `force`
(and potentially other values) but its dependency array only includes
`[slice]`, so it will use a stale `force` value when those change; include all
referenced external variables in the dependency array to avoid stale closures.
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]
