mikebridge commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3691703455
########## superset-frontend/src/features/versionHistory/useDashboardVersionPreview.ts: ########## @@ -0,0 +1,412 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useEffect, useRef } from 'react'; +import { useDispatch, useSelector, useStore } from 'react-redux'; +import { useHistory } from 'react-router-dom'; +import type { DataMaskStateWithId, JsonObject } from '@superset-ui/core'; +import { t } from '@apache-superset/core/translation'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { + hydrateDashboard, + HydrateChartData, + HydrateDashboardData, +} from 'src/dashboard/actions/hydrate'; +import { clearDataMaskState } from 'src/dataMask/actions'; +import type { RootState } from 'src/dashboard/types'; +import { + fetchDashboardHydrationData, + fetchExploreRehydrationData, + fetchVersionSnapshot, + layoutChartId, + swapUnreachableChartSlots, + DashboardHydrationData, +} from './api'; +import { + clearVersionPreview, + selectVersionLastRestoredUuid, + selectVersionPreview, + selectVersionRestoreCount, + versionPreviewApplied, +} from './reducer'; + +export interface SnapshotChartResolution { + charts: HydrateChartData[]; + positionData: JsonObject | null; +} + +// /api/v1/explore/ resolves a single chart per request, so unlike +// fetchReachableChartIds there is nothing to batch. Bound the concurrency +// instead: a snapshot whose layout references many charts the dashboard no +// longer holds would otherwise open one request per chart at once. +const EXPLORE_REHYDRATION_CONCURRENCY = 6; + +/** Runs `worker` over `items`, with at most `limit` in flight at a time. */ +async function forEachWithConcurrency<T>( + items: T[], + limit: number, + worker: (item: T) => Promise<void>, +): Promise<void> { + let cursor = 0; + const runner = async () => { + while (cursor < items.length) { + const item = items[cursor]; + cursor += 1; + await worker(item); + } + }; + await Promise.all( + Array.from({ length: Math.min(limit, items.length) }, runner), + ); +} + +/** + * A version snapshot stores the layout (position_json) but not the charts + * themselves, while the live dashboard payload only includes the charts the + * dashboard references at present. Reconcile the two: keep live charts the + * snapshot layout references (dropping ones added after the snapshot, so the + * hydrate "append new slices" path never fires), fetch metadata for charts + * the dashboard no longer includes, and swap layout slots whose chart cannot + * be fetched (e.g. deleted) for a markdown placeholder. + */ +export async function resolveSnapshotCharts( + liveCharts: HydrateChartData[], + positionData: JsonObject | null, +): Promise<SnapshotChartResolution> { + if (!positionData || Object.keys(positionData).length === 0) { + // The snapshot has no layout; hydrate falls back to an empty layout and + // appends any charts it is given, so pass none. + return { charts: [], positionData }; + } + + const snapshotChartIds = new Set<number>(); + Object.values(positionData).forEach(item => { + const chartId = layoutChartId(item as JsonObject); + if (chartId !== null) { + snapshotChartIds.add(chartId); + } + }); + + const liveById = new Map( + liveCharts.map(chart => [ + (chart.form_data?.slice_id as number | undefined) ?? chart.slice_id, + chart, + ]), + ); + const charts: HydrateChartData[] = []; + const missingIds: number[] = []; + snapshotChartIds.forEach(id => { + const live = liveById.get(id); + if (live) { + charts.push(live); + } else { + missingIds.push(id); + } + }); + + const unreachable = new Set<number>(); + await forEachWithConcurrency( + missingIds, + EXPLORE_REHYDRATION_CONCURRENCY, + async id => { + try { + const { slice, form_data } = await fetchExploreRehydrationData(id); + charts.push({ + slice_id: id, + slice_url: `/explore/?slice_id=${id}`, + slice_name: slice?.slice_name ?? t('Untitled chart'), + form_data: { ...form_data, slice_id: id }, + description: slice?.description ?? '', + description_markeddown: '', + editors: [], + modified: '', + changed_on: new Date().toISOString(), + }); + } catch { + unreachable.add(id); + } + }, + ); + + return { + charts, + positionData: swapUnreachableChartSlots(positionData, unreachable), + }; +} + +/** + * Applies a previewed dashboard version by re-hydrating the page with the + * snapshot's title/css/metadata/layout and the charts that layout references, + * and re-hydrates the live dashboard when the preview is closed. + */ +export function useDashboardVersionPreview(uuid: string | undefined) { + const dispatch = useDispatch(); + const store = useStore<RootState>(); + const history = useHistory(); + const { addDangerToast } = useToasts(); + const preview = useSelector(selectVersionPreview); + const dashboardId = useSelector<RootState, number | undefined>( + state => state.dashboardInfo?.id, + ); + const liveDataRef = useRef<DashboardHydrationData | null>(null); + // The user's filter selections at the moment they entered preview, restored + // when the preview closes. Captured once per live -> preview transition so + // switching between previewed versions keeps the original live state. + const liveDataMaskRef = useRef<DataMaskStateWithId | null>(null); + const appliedVersionRef = useRef<string | null>(null); + const fetchIdRef = useRef(0); + const restoreCount = useSelector(selectVersionRestoreCount); + const lastRestoredUuid = useSelector(selectVersionLastRestoredUuid); + const lastRestoreCountRef = useRef(restoreCount); + // Saves bump one of two redux signals depending on the path: edit-mode + // saves round-trip through ON_SAVE (dashboardState.lastModifiedTime), + // while native-filter and properties saves bump + // dashboardInfo.last_modified_time. + const saveSignal = useSelector<RootState, string>(state => + [ + state.dashboardState?.lastModifiedTime ?? '', + state.dashboardInfo?.last_modified_time ?? '', + ].join('|'), + ); + const lastSaveSignalRef = useRef(saveSignal); + // Hydration writes to the global store, which outlives this hook. The + // fetch-id guard below only invalidates requests superseded by another + // preview; nothing bumps it on unmount, so an apply() still in flight when + // the user navigates away would hydrate a dashboard over whatever page + // mounted next. + const isMountedRef = useRef(true); + useEffect( + () => () => { + isMountedRef.current = false; + }, + [], + ); + + const versionUuid = preview?.versionUuid; + + // A save that lands while no preview is applied makes the cached live + // copy stale — exiting a later preview would rehydrate the pre-save + // state (and a subsequent edit-mode save could overwrite the newer + // server state with it). Drop the cache so the next preview entry + // fetches a fresh live copy. While a preview is applied, saves are + // gated off in the UI and the cache must be kept for exit-preview, so + // only the not-previewing case clears it. + useEffect(() => { + if (saveSignal === lastSaveSignalRef.current) { + return; + } + lastSaveSignalRef.current = saveSignal; + if (appliedVersionRef.current === null) { + liveDataRef.current = null; + liveDataMaskRef.current = null; + } + }, [saveSignal]); + + useEffect(() => { + const hydrateWith = ( + dashboard: HydrateDashboardData, + charts: HydrateChartData[], + dataMask: DataMaskStateWithId, + editMode?: boolean, + ) => { + if (!isMountedRef.current) { + return; + } + // Hydration merges into any existing dataMask entries, which would let + // filter selections from one version leak into another; reset first so + // each hydrate starts from exactly the dataMask passed in. The two + // dispatches are synchronous back-to-back, so React batches them into + // a single render. + dispatch(clearDataMaskState()); + dispatch( + hydrateDashboard({ + history, + dashboard, + charts, + dataMask, + activeTabs: null, + chartStates: null, + editMode, + }), + ); + }; + + if (restoreCount !== lastRestoreCountRef.current) { + lastRestoreCountRef.current = restoreCount; + if (lastRestoredUuid !== uuid) { + // A restore of some other entity, resolving after navigation. This + // dashboard did not change on the server; rehydrating would clear its + // filters and unsaved state for someone else's restore. + return; + } + // The dashboard changed on the server (a version was restored); + // drop the cached live data and rehydrate with a fresh copy. + appliedVersionRef.current = null; + liveDataRef.current = null; + liveDataMaskRef.current = null; + if (!dashboardId) { + return; + } + fetchIdRef.current += 1; + const fetchId = fetchIdRef.current; + fetchDashboardHydrationData(dashboardId) + .then(data => { + if (fetchId !== fetchIdRef.current) { + return; + } + liveDataRef.current = data; + // A restored version behaves like a fresh page load: its own + // filter defaults, no carried-over selections. Explicitly not + // edit mode: a stale `?edit=true` in the URL (it outlives the + // navigation that set it) must not flip the page into edit mode + // as a side effect of the reload. + hydrateWith(data.dashboard, data.charts, {}, false); + }) + .catch(() => { + if (fetchId === fetchIdRef.current) { + addDangerToast(t('Failed to reload the restored version')); + } + }); + return; + } + + if (versionUuid && uuid && dashboardId) { + if (appliedVersionRef.current === versionUuid) { + return; + } + fetchIdRef.current += 1; + const fetchId = fetchIdRef.current; + // A save resolving while this apply is in flight (e.g. a properties + // save confirmed just before the preview opened) moves the save signal + // and outdates the copy being fetched below; capture the generation so + // the cache commit can tell. + const saveSignalAtStart = lastSaveSignalRef.current; + const apply = async () => { + // Work on a local copy and commit it to the cache only after the + // staleness check below — an in-flight fetch resolving after a + // restore (which cleared the cache) must not repopulate it with + // pre-restore content. + let liveData = liveDataRef.current; + if (!liveData) { + liveData = await fetchDashboardHydrationData(dashboardId); + } + const snapshot = await fetchVersionSnapshot( + 'dashboard', + uuid, + versionUuid, + ); + const snapshotLayout: JsonObject | null = snapshot.position_json + ? JSON.parse(snapshot.position_json) + : null; + const { charts, positionData } = await resolveSnapshotCharts( + liveData.charts, + snapshotLayout, + ); + if (fetchId !== fetchIdRef.current) { + return; + } + if (lastSaveSignalRef.current !== saveSignalAtStart) { + // A save landed mid-flight: the copy in hand predates it, and + // caching it would let exit-preview resurrect pre-save state (and + // a later edit-mode save persist it). Fetch a fresh copy instead. + liveData = await fetchDashboardHydrationData(dashboardId); + if (fetchId !== fetchIdRef.current) { + return; + } + } + liveDataRef.current = liveData; + const { dashboard } = liveData; + if (appliedVersionRef.current === null) { + // Entering preview from the live dashboard: remember the user's + // filter selections so closing the preview can bring them back. + liveDataMaskRef.current = store.getState().dataMask; + } + appliedVersionRef.current = versionUuid; + // The snapshot renders with its own filter defaults (from its + // native_filter_configuration), not the live selections. + hydrateWith( + { + ...dashboard, + dashboard_title: snapshot.dashboard_title, + css: snapshot.css ?? '', + metadata: snapshot.json_metadata + ? JSON.parse(snapshot.json_metadata) + : {}, + position_data: positionData, + } as HydrateDashboardData, + charts, + {}, + // Never edit mode: `?edit=true` outlives the navigation that set + // it, so deriving it from the URL would leave a live Save toolbar + // over a historical snapshot. + false, + ); + }; + apply() + .then(() => { + // Only the request that is still current may announce completion; + // a superseded one resolving later must not clear the flag for the + // preview that replaced it. + if (fetchId === fetchIdRef.current) { + dispatch(versionPreviewApplied()); + } + }) Review Comment: Confirmed and fixed in `91706dff80`: the unmount cleanup now invalidates the fetch id, silencing every guarded continuation at once — completion, the failure path's `clearVersionPreview`, and the toast. The unmount regression test additionally pins that the global slice receives no `VERSION_PREVIEW_APPLIED` after unmount. _Triaged and fixed by Claude (AI) on behalf of @mikebridge._ ########## superset-frontend/src/features/versionHistory/useVersionActivity.ts: ########## @@ -0,0 +1,267 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { getClientErrorObject } from '@superset-ui/core'; +import { fetchActivity } from './api'; +import { buildTimeline, mergeActivityPages } from './grouping'; +import type { + ActivityInclude, + ActivityRecord, + SaveGroup, + TimelineEntry, + VersionedEntityType, +} from './types'; + +const PAGE_SIZE = 25; +// Pagination counts raw records but the timeline groups and dedupes +// them, so one fetched page can yield zero new visible rows (e.g. a +// single save fanning out into dozens of records). "Load more" chases +// pages until something new becomes visible, capped per click. +const MAX_CHAINED_PAGES = 8; + +export interface UseVersionActivityResult { + records: ActivityRecord[]; + timeline: TimelineEntry[]; + /** + * The entity's newest self save, fetched by a dedicated one-record probe + * on every reset. The visible timeline cannot be trusted for this: it is + * server-filtered by the search term (its first group is merely the newest + * match), scoped by the include filter, and its first page can be filled + * entirely by newer related records — any of which would mislabel the + * "Current" version or drop it. + */ + newestGroup: SaveGroup | null; + count: number; + isLoading: boolean; + error: string | null; + hasMore: boolean; + /** + * True when the server clipped the history at its fetch ceiling: `count` + * is a floor, and records older than the last page exist but cannot be + * paged to. The panel says so rather than presenting the clipped history + * as complete. + */ + truncated: boolean; + loadMore: () => void; + refresh: () => void; +} + +export function useVersionActivity( + entityType: VersionedEntityType, + uuid: string | undefined, + include: ActivityInclude, + // Free-text search runs server-side over the full history (not just the + // loaded pages). Pass the already-debounced term; an empty/whitespace + // value omits the filter. + q = '', +): UseVersionActivityResult { + const [records, setRecords] = useState<ActivityRecord[]>([]); + const [newestGroup, setNewestGroup] = useState<SaveGroup | null>(null); + const [count, setCount] = useState(0); + const [truncated, setTruncated] = useState(false); + const [page, setPage] = useState(0); + const [isLoading, setIsLoading] = useState(false); + const [error, setError] = useState<string | null>(null); + // Monotonic id so stale responses from a previous uuid/include are dropped. + const fetchIdRef = useRef(0); + // The newest-self probe gets its own staleness counter: loadMore bumps + // fetchIdRef, and a click landing between a reset's page-0 response and + // its probe's response would otherwise discard the probe permanently — + // leaving the newest save untagged. Only a newer reset (or a uuid clear) + // supersedes a probe. + const probeIdRef = useRef(0); + // Mirror of `records` so the chained loadMore loop can see the merged + // result immediately. Kept in lock-step with setRecords by applyRecords — + // never write either one directly. + const recordsRef = useRef<ActivityRecord[]>([]); + const applyRecords = useCallback((next: ActivityRecord[]) => { + recordsRef.current = next; + setRecords(next); + }, []); + + // The cached newest group describes one entity's history; a different + // entity must not inherit it. + useEffect(() => { + setNewestGroup(null); + }, [entityType, uuid]); + + const fetchPage = useCallback( + async (pageToLoad: number, reset: boolean) => { + // Bump before the guard: clearing the uuid must invalidate any + // in-flight response so it cannot land afterwards. + fetchIdRef.current += 1; + const fetchId = fetchIdRef.current; + if (!uuid) { + // The invalidated in-flight request can no longer clear the + // spinner from its own finally block; clear it here. In-flight + // probes are invalidated too — their result describes an entity + // this hook no longer shows. + probeIdRef.current += 1; + setIsLoading(false); + return; + } + setIsLoading(true); + setError(null); + try { + const response = await fetchActivity(entityType, uuid, { + include, + page: pageToLoad, + pageSize: PAGE_SIZE, + q, + }); + if (fetchId !== fetchIdRef.current) { + return; + } + setCount(response.count); + setTruncated(!!response.truncated); + setPage(pageToLoad); + const next = reset + ? response.result + : mergeActivityPages(recordsRef.current, response.result); + applyRecords(next); + // Deliberately not derived from the page above: that one is filtered + // by q/include and mixes self with related records, so the newest + // self save can be absent from it (an active search that excludes it, + // an include='related' filter, or newer related records filling the + // page). A dedicated one-record self probe is authoritative in every + // one of those states, and refreshes after restores made while a + // search is active. + if (reset) { + probeIdRef.current += 1; + const probeId = probeIdRef.current; + fetchActivity(entityType, uuid, { + include: 'self', + page: 0, + pageSize: 1, + }) + .then(probe => { + if (probeId !== probeIdRef.current) { + return; + } + setNewestGroup( + (buildTimeline(probe.result).find( + entry => entry.type === 'group', + ) as SaveGroup | undefined) ?? null, + ); + }) + .catch(() => { + // Keep the previous value: a failed probe should not strip the + // Current tag from a timeline that is otherwise rendering. + }); + } + } catch (response) { + if (fetchId !== fetchIdRef.current) { + return; + } + const { error: clientError, message } = await getClientErrorObject( + response as Parameters<typeof getClientErrorObject>[0], + ); + setError(clientError || message || null); + } finally { + if (fetchId === fetchIdRef.current) { + setIsLoading(false); + } + } + }, + [entityType, uuid, include, q], + ); + + useEffect(() => { + applyRecords([]); + setCount(0); + setTruncated(false); + setPage(0); + fetchPage(0, true); + }, [applyRecords, fetchPage]); Review Comment: Confirmed and fixed in `91706dff80`: the probe id is now bumped when `fetchPage` starts (including the uuid-clear path) rather than when the reset's own probe launches, so an entity switch immediately supersedes the old entity's in-flight probe. Regression test: uuid A's deferred probe resolving after the hook moved to uuid B is discarded, confirmed to fail against the previous placement. _Triaged and fixed by Claude (AI) on behalf of @mikebridge._ ########## superset-frontend/src/features/versionHistory/useVersionActions.tsx: ########## @@ -0,0 +1,278 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { ReactElement, useCallback, useRef, useState } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; +import { t } from '@apache-superset/core/translation'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { getClientErrorObject } from '@superset-ui/core'; +import { + closeOpenedTab, + navigateOpenedTab, + openBlankTab, +} from 'src/utils/navigationUtils'; +import type { VersionedEntityType } from './types'; +import { + createChartFromSnapshot, + createDashboardFromSnapshot, + fetchActivity, + fetchVersionSnapshot, + restoreVersion, +} from './api'; +import { + clearVersionPreview, + selectVersionSessionLog, + versionRestored, + type VersionHistoryRootState, +} from './reducer'; +import { formatVersionMonthDay } from './display'; +import RestoreConfirmModal from './RestoreConfirmModal'; + +/** The version a restore / open-as-new action operates on. */ +export interface VersionActionTarget { + versionUuid: string; + headline: string; + issuedAt: string; +} + +export interface UseVersionActionsResult { + /** Opens the restore confirmation modal for the given version. */ + requestRestore: (target: VersionActionTarget) => void; + /** Forks the given version into a new chart/dashboard in a new tab. */ + openAsNew: (target: VersionActionTarget) => void; + /** True while an openAsNew fork is in flight; disable its triggers. */ + isCreating: boolean; + /** Render this alongside the calling component. */ + restoreModal: ReactElement | null; +} + +/** + * Restore and open-as-new flows shared by the panel kebabs and the + * preview banner. Restore success is broadcast via the redux + * `restoreCount` so page-level hooks can rehydrate and refresh activity. + */ +export function useVersionActions( + entityType: VersionedEntityType, + uuid: string | undefined, +): UseVersionActionsResult { + const dispatch = useDispatch(); + const { addSuccessToast, addInfoToast, addWarningToast, addDangerToast } = + useToasts(); + const [restoreTarget, setRestoreTarget] = + useState<VersionActionTarget | null>(null); + const [isRestoring, setIsRestoring] = useState(false); + const [isCreating, setIsCreating] = useState(false); + // The state flags drive rendering; these refs are the actual locks. Two + // activations in one tick both read the pre-update state value, so a + // guard on state alone lets the second through and forks a duplicate. + const restoringRef = useRef(false); + const creatingRef = useRef(false); + + // A restore rehydrates the page from the server, which silently wipes + // in-progress edits and their undo history — the same hazard the preview + // entry gate guards against. The dirty signal is page-specific: dashboards + // track hasUnsavedChanges; explore's signal is the session log, which lists + // exactly the unsaved control changes the panel shows under "Current + // version". Read here rather than passed in, so no call site (panel kebab, + // preview banner) can forget it. + const hasUnsavedChanges = useSelector< + VersionHistoryRootState & { + dashboardState?: { hasUnsavedChanges?: boolean }; + }, + boolean + >(state => + entityType === 'dashboard' + ? !!state.dashboardState?.hasUnsavedChanges + : selectVersionSessionLog(state).length > 0, + ); + + const requestRestore = useCallback( + (target: VersionActionTarget) => { + if (hasUnsavedChanges) { + addDangerToast( + t('Save or discard your unsaved changes to restore a version.'), + ); + return; + } + setRestoreTarget(target); + }, + [addDangerToast, hasUnsavedChanges], + ); + + const cancelRestore = useCallback(() => { + setRestoreTarget(null); + }, []); + + // The restore endpoint reports success but not whether a new version + // transaction was created (restoring an already-matching state is a + // server-side no-op); probe the newest self transaction to tell the + // two apart in the toast. A save by another user landing between the + // two probes can skew which toast variant shows — accepted, cosmetic. + const latestTransactionId = useCallback(async (): Promise<number | null> => { + if (!uuid) { + return null; + } + try { + const { result } = await fetchActivity(entityType, uuid, { + include: 'self', + page: 0, + pageSize: 1, + }); + return result[0]?.transaction_id ?? null; + } catch { + return null; + } + }, [entityType, uuid]); + + const confirmRestore = useCallback(async () => { + if (!restoreTarget || !uuid || restoringRef.current) { + return; + } + if (hasUnsavedChanges) { + // The request-time gate can be outrun: work turning dirty while the + // confirmation modal sits open (an in-flight edit resolving late) + // would still be wiped by the rehydration. Re-check at the moment of + // mutation. + addDangerToast( + t('Save or discard your unsaved changes to restore a version.'), + ); + setRestoreTarget(null); + return; + } + restoringRef.current = true; + setIsRestoring(true); + try { + const beforeTransactionId = await latestTransactionId(); + const { message } = await restoreVersion( + entityType, + uuid, + restoreTarget.versionUuid, Review Comment: Fixed in `91706dff80` as hardening: a pending confirmation is dropped whenever the page's entity changes. One correction to the stated impact: the combination could not restore the wrong entity/version — version uuids are entity-scoped (UUIDv5 over entity and transaction), so the server refuses the mismatch with a 404. The user-visible harm was a confusing failure toast for an action aimed at something else, and the reachable window is narrow since a save-as navigates with a reload. Cheap to close regardless, with a test. _Triaged and fixed by Claude (AI) on behalf of @mikebridge._ ########## superset-frontend/src/features/versionHistory/ExploreVersionHistory.tsx: ########## @@ -0,0 +1,333 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; +import { useDebounceValue } from 'src/hooks/useDebounceValue'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { getUrlParam } from 'src/utils/urlUtils'; +import { canOverwriteSlice } from 'src/explore/exploreUtils/canOverwriteSlice'; +import { URL_PARAMS } from 'src/constants'; +import { hydrateExplore } from 'src/explore/actions/hydrateExplore'; +import type { Slice } from 'src/types/Chart'; +import type { ExplorePageState } from 'src/explore/types'; +import type { ActivityInclude, ActivityRecord, SaveGroup } from './types'; +import { + clearVersionPreview, + closeVersionHistoryPanel, + openVersionHistoryPanel, + selectIsVersionHistoryPanelOpen, + selectVersionHistoryInclude, + selectVersionPreview, + selectVersionLastRestoredUuid, + selectVersionRestoreCount, + selectVersionSessionLog, + setVersionHistoryInclude, + setVersionPreview, +} from './reducer'; +import { fetchChartUuid, fetchExploreRehydrationData } from './api'; +import { openRelatedEntity } from './openRelated'; +import { useVersionActivity } from './useVersionActivity'; +import { useVersionActions } from './useVersionActions'; +import { groupHeadline } from './display'; +import VersionHistoryPanel from './VersionHistoryPanel'; + +/** + * The explore flex row (datasource rail + control rail + chart) cannot give + * up enough width for the panel on narrow viewports; below the XL breakpoint + * the panel overlays the page (anchored to the relatively-positioned explore + * container) instead of being pushed past the viewport edge. + */ +const PanelHost = styled.div` + ${({ theme }) => ` + height: 100%; + flex-shrink: 0; + @media (max-width: ${theme.screenXL}px) { + position: absolute; + top: 0; + right: 0; + bottom: 0; + height: auto; + z-index: 20; + box-shadow: ${theme.boxShadow}; + } + `} +`; + +export default function ExploreVersionHistory() { + const dispatch = useDispatch(); + const { addDangerToast } = useToasts(); + const slice = useSelector<ExplorePageState, Slice | undefined>( + state => state.explore?.slice ?? undefined, + ); + const user = useSelector<ExplorePageState, ExplorePageState['user']>( + state => state.user, + ); + const canOverwrite = useSelector<ExplorePageState, boolean>( + state => state.explore?.can_overwrite ?? false, + ); + // Same predicate as the menu that opens this panel: can_overwrite alone + // excludes admins and extra editors on charts with no explicit editors. + const canRestore = useMemo( + () => canOverwriteSlice({ slice, user, canOverwrite }), + [slice, user, canOverwrite], + ); + const isPanelOpen = useSelector(selectIsVersionHistoryPanelOpen); + const include = useSelector(selectVersionHistoryInclude); + const preview = useSelector(selectVersionPreview); + const sessionLog = useSelector(selectVersionSessionLog); + const sliceId = slice?.slice_id; + // Key the fetched uuid by slice id so a "save as" (which swaps the slice + // in place) invalidates it instead of keeping the old chart's uuid. + const [fetchedUuid, setFetchedUuid] = useState<{ + sliceId: number; + uuid: string; + } | null>(null); + const uuid = + slice?.uuid ?? + (fetchedUuid && fetchedUuid.sliceId === sliceId + ? fetchedUuid.uuid + : undefined); + + useEffect(() => { + // Match the menu entry's gating: version history is only offered to + // users who could restore (sc-107604) — the URL param must not open + // it for read-only viewers. + if (getUrlParam(URL_PARAMS.versionHistory) && canRestore) { + dispatch(openVersionHistoryPanel('chart')); + } + }, [canRestore, dispatch]); + + // Leaving the page should not carry panel/preview state to other pages. + useEffect( + () => () => { + dispatch(closeVersionHistoryPanel()); + }, + [dispatch], + ); + + useEffect(() => { + if (uuid || !isPanelOpen || !sliceId) { + return undefined; + } + let cancelled = false; + fetchChartUuid(sliceId) + .then(value => { + if (!cancelled) { + setFetchedUuid({ sliceId, uuid: value }); + } + }) + .catch(() => { + if (!cancelled) { + addDangerToast(t('Failed to load version history')); + // Without a uuid the panel would sit on a misleading + // "No history yet" empty state; close it instead. + dispatch(closeVersionHistoryPanel()); + } + }); + return () => { + cancelled = true; + }; + }, [uuid, isPanelOpen, sliceId, addDangerToast, dispatch]); + + // Server-side search over the full history; debounce so each keystroke + // doesn't refetch. + const [searchTerm, setSearchTerm] = useState(''); + const debouncedSearch = useDebounceValue(searchTerm); + const activity = useVersionActivity( + 'chart', + isPanelOpen ? uuid : undefined, + include, + debouncedSearch, + ); + + const { requestRestore, openAsNew, restoreModal } = useVersionActions( + 'chart', + uuid, + ); + + // After a restore the server-side chart changed; reload the explore + // page state in place (same payload the page hydrates from) and + // refresh the activity timeline so the new "Restored version" entry + // shows up. + const restoreCount = useSelector(selectVersionRestoreCount); + const lastRestoredUuid = useSelector(selectVersionLastRestoredUuid); + // An overwrite save re-hydrates explore in place (no remount), which + // replaces the slice with a fresh server copy; watch its changed_on + // so the save surfaces as a new timeline entry while the panel is + // open. A "save as" navigates with PUSH and reloads the page, so it + // needs no signal. + const saveSignal = useSelector<ExplorePageState, string | undefined>( + state => state.explore?.slice?.changed_on, + ); + const lastRestoreCountRef = useRef(restoreCount); + const lastSaveSignalRef = useRef(saveSignal); + const refreshActivity = activity.refresh; + useEffect(() => { + // Cancels the in-flight rehydration when the effect re-runs or the page + // unmounts. hydrateExplore rewrites the whole explore store, so a fetch + // resolving after the user navigated away — or after a save-as swapped + // the slice in place — would overwrite the newly loaded chart's state + // with the old chart's payload. + let cancelled = false; + if (restoreCount !== lastRestoreCountRef.current) { + lastRestoreCountRef.current = restoreCount; + // Guard: a restore of some other entity, resolving after navigation. + // This chart did not change on the server; rehydrating would discard + // its state for someone else's restore. + if (lastRestoredUuid === uuid) { + // The restore refresh covers any save-signal movement caused by + // the same change; sync it so it does not refetch again. + lastSaveSignalRef.current = saveSignal; + refreshActivity(); + if (sliceId) { + fetchExploreRehydrationData(sliceId) + .then(result => { + if (!cancelled) { + dispatch( + hydrateExplore({ ...result, saveAction: 'overwrite' }), + ); + } Review Comment: Confirmed as known and deliberately open — this is the double-refetch item that has been on the tracked-remaining list since the first capstone review ("a restore can still fire two activity refetches"). Your mechanism description is exactly right: the ref is synced to the pre-hydration signal, and the post-hydration value reads as a fresh save. The clean fix needs expected-change tracking (suppress exactly one signal movement per successful rehydration, without a lingering flag that would swallow the *next* genuine save if the signal happens not to move), which is the same design as the session-log false-positive fix — both are queued together for the post-merge batch. Cost meanwhile is one redundant GET after each restore. _Triaged by Claude (AI) on behalf of @mikebridge._ -- 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]
