codeant-ai-for-open-source[bot] commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3690962792
########## superset-frontend/src/features/versionHistory/ExploreVersionHistory.tsx: ########## @@ -0,0 +1,322 @@ +/** + * 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(() => { + if (restoreCount !== lastRestoreCountRef.current) { + lastRestoreCountRef.current = restoreCount; + if (lastRestoredUuid !== uuid) { + // 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. + return; + } + // 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) { + return; + } + fetchExploreRehydrationData(sliceId) + .then(result => { + dispatch(hydrateExplore({ ...result, saveAction: 'overwrite' })); + }) Review Comment: **Suggestion:** The rehydration request is not cancelled or validated after it starts. If the user navigates to another chart before this restore request completes, the unmounted component can still dispatch `hydrateExplore` with the old chart's response and overwrite the newly opened chart's state. Track cancellation in the effect cleanup or verify that the slice UUID/ID is still current before dispatching. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Navigation can display the previously restored chart's state. - ⚠️ The new chart's Explore hydration can be overwritten transiently. - ⚠️ Restore and navigation requests race without shared generation checks. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8a6c5cbe0a674078ba2df8ed2ff32341&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=8a6c5cbe0a674078ba2df8ed2ff32341&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/ExploreVersionHistory.tsx **Line:** 199:202 **Comment:** *Race Condition: The rehydration request is not cancelled or validated after it starts. If the user navigates to another chart before this restore request completes, the unmounted component can still dispatch `hydrateExplore` with the old chart's response and overwrite the newly opened chart's state. Track cancellation in the effect cleanup or verify that the slice UUID/ID is still current before dispatching. 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=3d874357d564e068e759743163d60df9f5f241d7c7dbd076530470eb70c26972&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41551&comment_hash=3d874357d564e068e759743163d60df9f5f241d7c7dbd076530470eb70c26972&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]
