EnxDev commented on code in PR #42473:
URL: https://github.com/apache/superset/pull/42473#discussion_r3658322543
##########
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(
Review Comment:
`restoreChartState` re-runs the query via `postChartFormData`, but it
doesn't update `lastQueriedControls`. As a result, after restoring state with
Back, the chart has fresh data but `chartIsStale` still compares against the
controls from before the restore.
This causes the stale-results banner to appear and the Run/Update button to
become primary even though the chart is already up to date.
This isn't a regression from master, but it's visible in the flow introduced
by this PR. Could we update `lastQueriedControls` as part of the
restore/re-query path?
It would also be good to cover the POP restore case with a regression test.
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -227,7 +231,20 @@ const updateHistory = debounce(
force,
false,
);
- history.replace(url, payload);
+ const previousChartState = getChartStateFromHistoryState(
+ history.location.state,
+ );
+ const state = toChartStateHistoryState(payload);
+ if (
+ isReplace ||
+ !previousChartState ||
+ isEqual(previousChartState, payload)
Review Comment:
The replace-vs-push check relies on `isEqual(previousChartState, payload)`,
but `payload` is regenerated from the controls after `setExploreControls`, and
that round-trip isn't always lossless.
For example, `applyMapStateToPropsToControl` replaces `null`/`undefined`
values with `state.default`, so the regenerated payload may differ from the
original state even when the user hasn't actually changed anything.
Could this cause a new history entry to be pushed instead of replacing the
current one?
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -185,6 +221,7 @@ export default function ExplorePage() {
};
}
+ loadedFormData.current = formData as QueryFormData;
Review Comment:
`loadedFormData.current` is only updated on initial load, not after
`restoreChartState`.
This doesn't seem to cause the issue described by CodeAnt, since
`isSameChartState` only compares `slice_id` and `datasource`, which remain
stable across normal undo steps. The actual issue appears when the dataset is
changed in Explore: newer history entries contain the new datasource, while
`loadedFormData.current` still references the old one, causing POP navigation
to fall back to a full reload.
Could we update `loadedFormData.current` inside `restoreChartState` to keep
it in sync?
--
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]
