msyavuz commented on code in PR #42473:
URL: https://github.com/apache/superset/pull/42473#discussion_r3665255936
##########
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:
Fixed differently: the ref is gone, both handlers now resolve the chart
through `selectRestoreTarget`.
##########
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:
Good catch, the restore moved into ExploreViewContainer, which now sets
`lastQueriedControls` to the controls it re-queried (regression test added).
##########
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:
Both sides are the stamped state now, and a restore suppresses `addHistory`
outright, so the undo path no longer rests on this comparison.
##########
superset-frontend/src/pages/Chart/index.tsx:
##########
@@ -185,6 +221,7 @@ export default function ExplorePage() {
};
}
+ loadedFormData.current = formData as QueryFormData;
Review Comment:
Right, and the two checks could disagree, so the ref is replaced by a shared
selector over the controls on screen.
--
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]
