mikebridge commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3707155795
########## superset-frontend/src/features/versionHistory/useVersionActions.tsx: ########## @@ -0,0 +1,295 @@ +/** + * 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, useEffect, 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, + ); + + // A pending confirmation names a version of the entity it was opened for. + // If the page's entity changes underneath it (an in-place slice swap), the + // modal must not survive to combine the new uuid with the old version — + // the server would refuse the mismatch, but the user would be shown a + // confusing failure for an action they aimed at something else. + useEffect(() => { + setRestoreTarget(null); + }, [entityType, uuid]); + + 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, + ); + const afterTransactionId = await latestTransactionId(); + if ( + beforeTransactionId !== null && + afterTransactionId !== null && + beforeTransactionId === afterTransactionId + ) { + addInfoToast(t('Already at this version')); + } else { + addSuccessToast(t("Restored to '%s' version", restoreTarget.headline)); + } + // A restore can succeed while dropping chart associations the snapshot + // referenced but that no longer exist, and the endpoint says so in its + // message rather than in a status code. Reporting only the success + // would tell the user their dashboard came back whole when it did not. + if (message && message !== 'OK') { + addWarningToast(message); + } + setRestoreTarget(null); + dispatch(clearVersionPreview()); + dispatch(versionRestored(uuid)); Review Comment: *Generated by Claude (AI) on behalf of @mikebridge.* Confirmed and fixed in `5d9fe736f2` — thanks, this was a real gap. `versionRestored(uuid)` on the very next line carries the uuid for exactly this reason, and the clear didn't. Fixed at the reducer rather than the call site: `clearVersionPreview` now takes a required entity uuid and the reducer no-ops when it doesn't match the preview in flight, so all five dispatchers — and any future one — inherit the guard. The parameter is required rather than optional deliberately: it forces an asynchronous dispatcher to decide whose preview it is. Two new reducer tests fail against the unscoped action. ########## superset-frontend/src/dashboard/types.ts: ########## @@ -123,6 +123,7 @@ export type DashboardState = { isFiltersRefreshing: boolean; hasUnsavedChanges: boolean; dashboardIsSaving: boolean; + lastModifiedTime?: number; Review Comment: *Generated by Claude (AI) on behalf of @mikebridge.* Agreeing with @rusackas that this one's a false positive, and recording the evidence so it isn't re-derived: the `changed_on`/`lastModifiedTime` divergence predates this branch — `hydrate.ts` on master already assigns the timestamp string, and `dashboardState.ts` already declared the numeric field. This PR only adds the type declaration mirroring what was there. The new consumer treats the value strictly as an opaque change-signal string (it's compared for inequality against its own previous value to detect a save, never parsed or arithmetic'd), so nothing here depends on the representation. Normalizing it properly is worth doing, but it's a pre-existing cleanup on master rather than something this PR introduces or should carry. -- 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]
