codeant-ai-for-open-source[bot] commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3704249509
########## 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: **Suggestion:** The preview-clearing action is global and is dispatched after an asynchronous restore without verifying that the page entity is still the one that initiated the restore. If the user navigates away and previews a version on another entity while this request is in flight, the old restore completion will clear the new page's preview state. Scope the clear operation to the originating entity or discard the completion when the entity changes, as is already done for the restore notification. [stale reference] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ New entity's historical preview is cleared by an old restore. - ⚠️ Preview banner disappears during cross-entity navigation. - ⚠️ Historical preview state can be replaced by live page state. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=267793b0dc3f439da338c72cd8461abe&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=267793b0dc3f439da338c72cd8461abe&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/features/versionHistory/useVersionActions.tsx **Line:** 193:194 **Comment:** *Stale Reference: The preview-clearing action is global and is dispatched after an asynchronous restore without verifying that the page entity is still the one that initiated the restore. If the user navigates away and previews a version on another entity while this request is in flight, the old restore completion will clear the new page's preview state. Scope the clear operation to the originating entity or discard the completion when the entity changes, as is already done for the restore notification. 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%2F41551&comment_hash=cb98eb4f4278ba47e72fa2db7defdf09bad237692ca2d845b4ad66aab6b0a6ff&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41551&comment_hash=cb98eb4f4278ba47e72fa2db7defdf09bad237692ca2d845b4ad66aab6b0a6ff&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]
