codeant-ai-for-open-source[bot] commented on code in PR #42473:
URL: https://github.com/apache/superset/pull/42473#discussion_r3664354777
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -227,7 +235,20 @@ const updateHistory = debounce(
force,
false,
);
- history.replace(url, payload);
+ const previousChartState = getChartStateFromHistoryState(
+ history.location.state,
+ );
+ const state = toChartStateHistoryState(payload, sliceId);
+ if (
+ isReplace ||
+ !previousChartState ||
+ isEqual(previousChartState, getChartStateFromHistoryState(state))
+ ) {
+ history.replace(url, state);
+ } else {
+ // one entry per chart state is what makes the Back button undo it
+ history.push(url, state);
+ }
Review Comment:
**Suggestion:** The debounced callback reads `history.location` only when it
finally executes, so a query initiated on chart A can run after navigation to
chart B and then push or replace chart A's form data into B's current Explore
route. The pathname check does not establish that the route still represents
the same chart. Capture the originating location/chart identity and discard the
delayed update when it no longer matches. [race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Chart B history can receive chart A state.
- ❌ Explore URLs may point to the wrong chart.
- ⚠️ POP navigation can mix states across charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Edit chart A and trigger `addHistory()` through `reRenderChart()` at
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:618-631`;
`updateHistory` is debounced for 1000ms at
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:172-258`.
2. Navigate to chart B in Explore before the debounced callback executes, or
while its
`postFormData`/`putFormData` request is awaiting completion at
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:225-237`.
3. The callback still receives chart A's captured `formData`, but reads the
shared
`history.location` only after the await at
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:238-241`.
Its
only route guard, `pathname.startsWith('/explore')` at lines 227-229,
remains true for
chart B.
4. The callback then pushes or replaces chart A's URL and chart-state
payload into the
current chart B history entry at
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:242-251`.
Subsequent POP handling can reload or restore the wrong chart state.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=46e46a656d8d4653bcb741ca7ce44e59&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=46e46a656d8d4653bcb741ca7ce44e59&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/ExploreViewContainer/index.tsx
**Line:** 238:251
**Comment:**
*Race Condition: The debounced callback reads `history.location` only
when it finally executes, so a query initiated on chart A can run after
navigation to chart B and then push or replace chart A's form data into B's
current Explore route. The pathname check does not establish that the route
still represents the same chart. Capture the originating location/chart
identity and discard the delayed update when it no longer matches.
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%2F42473&comment_hash=13d8cf0f9f0d3e86838105e4648067019f8f5bccebe696b52fffe955cbd6d479&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42473&comment_hash=13d8cf0f9f0d3e86838105e4648067019f8f5bccebe696b52fffe955cbd6d479&reaction=dislike'>👎</a>
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -611,6 +639,32 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
],
);
+ // Explore's own entries carry the chart state they were pushed with, so a
POP
+ // into one is an undo/redo: apply it and re-query in place, which is also
why
+ // ExplorePage skips its reload for them.
+ const sliceId = props.form_data.slice_id ?? props.slice?.slice_id;
+ const { datasource: datasourceUid } = props.form_data;
+ useEffect(
+ () =>
+ history.listen((loc: Location, action: Action) => {
+ const chartState = getChartStateFromHistoryState(loc.state);
+ if (
+ action !== 'POP' ||
+ !chartState ||
+ !isSameChartState(chartState, {
+ slice_id: sliceId,
+ datasource: datasourceUid,
+ })
+ ) {
+ return;
+ }
+ restoringFromHistory.current = true;
+ props.actions.setExploreControls(chartState);
+ props.actions.triggerQuery(true, props.chart.id);
Review Comment:
**Suggestion:** `setExploreControls` only updates the explore controls; it
does not update `form_data`, while the chart query path invoked by
`triggerQuery` posts the existing `formData` prop. Therefore a POP restore
immediately queries using the prior chart state rather than the popped state,
and the stale `form_data` also remains available to subsequent history updates.
Restore the complete form-data state or explicitly build and post the query
from `chartState` after the controls have been applied. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Explore Back restores controls but queries stale form data.
- ❌ Chart results can disagree with displayed control values.
- ⚠️ Subsequent history entries retain stale form-data state.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a saved chart in Explore and create multiple history states through
the query path
at
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:618-631`,
which stores the changed form data in each history entry.
2. Press Back to a same-chart entry; the POP listener at
`superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:649-664`
calls
`setExploreControls(chartState)` and then `triggerQuery(true,
props.chart.id)`.
3. `setExploreControls()` only dispatches `SET_EXPLORE_CONTROLS` at
`superset-frontend/src/explore/actions/exploreActions.ts:105-108`; the
reducer updates
only `controls` at
`superset-frontend/src/explore/reducers/exploreReducer.ts:537-545` and
leaves `form_data` unchanged.
4. `triggerQuery()` only toggles the chart trigger flag at
`superset-frontend/src/components/Chart/chartAction.ts:661-665`. The
resulting `Chart`
query calls `postChartFormData(formData, ...)` using its existing `formData`
prop at
`superset-frontend/src/components/Chart/Chart.tsx:229-245`, so the request
posts the
pre-POP form data instead of `chartState`. The visible controls and queried
data can
therefore diverge, and later history updates continue using stale
`props.form_data`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=baaebc34cba546c69542712219477f2a&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=baaebc34cba546c69542712219477f2a&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/ExploreViewContainer/index.tsx
**Line:** 661:663
**Comment:**
*Incomplete Implementation: `setExploreControls` only updates the
explore controls; it does not update `form_data`, while the chart query path
invoked by `triggerQuery` posts the existing `formData` prop. Therefore a POP
restore immediately queries using the prior chart state rather than the popped
state, and the stale `form_data` also remains available to subsequent history
updates. Restore the complete form-data state or explicitly build and post the
query from `chartState` after the controls have been applied.
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%2F42473&comment_hash=9947231ed3837632ef8589d0523ebcb6ace86bf9614645e60bc6256f85df0f67&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42473&comment_hash=9947231ed3837632ef8589d0523ebcb6ace86bf9614645e60bc6256f85df0f67&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]
