codeant-ai-for-open-source[bot] commented on code in PR #42473:
URL: https://github.com/apache/superset/pull/42473#discussion_r3656410779
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -136,8 +148,32 @@ export default function ExplorePage() {
const [isLoaded, setIsLoaded] = useState(false);
const fetchGeneration = useRef(0);
const abortControllerRef = useRef<AbortController | null>(null);
+ const loadedFormData = useRef<QueryFormData>();
const dispatch = useDispatch();
const history = useHistory();
+ const force = useSelector<ExplorePageState, boolean>(
+ state => !!state.explore?.force,
+ );
+ const timeout = useSelector<ExplorePageState, number | undefined>(
+ state => state.common?.conf?.SUPERSET_WEBSERVER_TIMEOUT,
+ );
+
+ // Restores a chart state stored in a history entry by updateHistory, so that
+ // Back/Forward step through chart states without reloading the page.
+ const restoreChartState = useCallback(
+ (formData: QueryFormData) => {
+ dispatch(setExploreControls(formData));
+ dispatch(
+ postChartFormData(
+ formData,
+ force,
+ timeout,
+ formData.slice_id ?? UNSAVED_CHART_ID,
+ ),
+ );
+ },
+ [dispatch, force, timeout],
+ );
Review Comment:
**Suggestion:** After restoring a chart state in place,
`loadedFormData.current` is never updated. Consequently, a subsequent Back or
Forward navigation is compared against the original form data rather than the
currently displayed state, so navigating across multiple same-chart history
entries incorrectly falls through to `loadExploreData` and performs a full
reload. Update the ref whenever `restoreChartState` applies a state. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Explore Back/Forward can trigger full reloads instead of instant
restoration.
- ⚠️ Consecutive chart undo/redo loses the intended in-place behavior.
- ⚠️ Reloads can show loading state during history navigation.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a saved chart through the Explore page entry point implemented by
`ExplorePage` at
`superset-frontend/src/pages/Chart/index.tsx:147`; the initial
`loadExploreData()` call
stores the loaded form data in `loadedFormData.current` at lines 224-230.
2. Change a chart control and run the chart twice, causing `updateHistory`
to create
multiple same-chart history entries as described by the PR.
3. Press Back. `history.listen()` at lines 347-369 extracts the popped chart
state at line
350, then compares it with the stale `loadedFormData.current` at lines
355-358.
`restoreChartState()` at lines 163-176 applies the popped form data but
never updates
`loadedFormData.current`.
4. Press Back again, or press Forward after the first undo. The next state
is compared
with the original form data rather than the state currently displayed by
`restoreChartState()`. When `isSameChartState()` returns false, execution
falls through to
lines 363-368, sets `isLoaded` to false, and calls `loadExploreData()`,
causing a full
reload instead of in-place undo/redo. In the common case where the first
popped state
already differs from the originally loaded data, even the first Back falls
through this
path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c23cb64c61a6420dbf82d411bed15641&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=c23cb64c61a6420dbf82d411bed15641&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/pages/Chart/index.tsx
**Line:** 163:176
**Comment:**
*Logic Error: After restoring a chart state in place,
`loadedFormData.current` is never updated. Consequently, a subsequent Back or
Forward navigation is compared against the original form data rather than the
currently displayed state, so navigating across multiple same-chart history
entries incorrectly falls through to `loadExploreData` and performs a full
reload. Update the ref whenever `restoreChartState` applies a 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%2F42473&comment_hash=1ce81df9f62c0eee6e6bcdc6ec299a26009456848baba565077e16e5e77f8f97&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42473&comment_hash=1ce81df9f62c0eee6e6bcdc6ec299a26009456848baba565077e16e5e77f8f97&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]
