mikebridge commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3690778639
########## superset-frontend/src/features/versionHistory/ExploreVersionHistory.tsx: ########## @@ -0,0 +1,312 @@ +/** + * 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, + 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); + // 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; + // 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) { Review Comment: Confirmed and fixed in `3b7a1fb944`, together with the sibling finding on `VersionHistoryPanel.tsx` — they share a root cause. `newestGroup` was derived from the first visible page, which is the wrong source three ways: filtered by the search term (this finding), scoped by the include filter, and susceptible to newer related records filling the page (the sibling). It now comes from a dedicated one-record `include=self` probe on every reset, so a restore made while a search is active moves the Current marker and restore notice correctly. Regression tests cover both scenarios. _Triaged and fixed by Claude (AI) on behalf of @mikebridge._ ########## superset-frontend/src/features/versionHistory/VersionHistoryPanel.tsx: ########## @@ -0,0 +1,300 @@ +/** + * 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 { useMemo } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import { Alert } from '@apache-superset/core/components'; +import { + Button, + EmptyState, + Icons, + Input, + Select, + Skeleton, +} from '@superset-ui/core/components'; +import type { + ActivityInclude, + ActivityRecord, + SaveGroup, + SessionLogEntry, + VersionedEntityType, +} from './types'; +import type { UseVersionActivityResult } from './useVersionActivity'; +import { relatedEntryKey } from './grouping'; +import { formatVersionDateTime } from './display'; +import SaveGroupItem from './SaveGroupItem'; +import RelatedUpdateRow from './RelatedUpdateRow'; +import CurrentVersionSection from './CurrentVersionSection'; + +const VERSION_HISTORY_PANEL_WIDTH = 320; + +const Panel = styled.aside` + ${({ theme }) => ` + width: ${VERSION_HISTORY_PANEL_WIDTH}px; + min-width: ${VERSION_HISTORY_PANEL_WIDTH}px; + height: 100%; + display: flex; + flex-direction: column; + border-left: 1px solid ${theme.colorSplit}; + background-color: ${theme.colorBgContainer}; + `} +`; + +const PanelHeader = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + justify-content: space-between; + padding: ${theme.sizeUnit * 4}px ${theme.sizeUnit * 6}px; + border-bottom: 1px solid ${theme.colorSplit}; + `} +`; + +const PanelTitle = styled.span` + ${({ theme }) => ` + font-size: ${theme.fontSize}px; + line-height: ${theme.lineHeight}; + color: ${theme.colorText}; + `} +`; + +const Controls = styled.div` + ${({ theme }) => ` + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + padding: ${theme.sizeUnit * 4}px ${theme.sizeUnit * 6}px; + > * { + flex: 1 1 0; + min-width: 0; + } + `} +`; + +const Body = styled.div` + ${({ theme }) => ` + flex: 1; + overflow-y: auto; + padding: 0 ${theme.sizeUnit * 4}px; + `} +`; + +// Icon-only trigger: neutral icon color instead of the link-button blue. +const CloseButton = styled(Button)` + ${({ theme }) => ` + && { + color: ${theme.colorText}; + } + &&:hover, + &&:focus { + color: ${theme.colorTextSecondary}; + } + `} +`; + +const Footer = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 2}px; + text-align: center; + `} +`; + +const PaddedContent = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 4}px; + `} +`; + +export interface VersionHistoryPanelProps { + entityType: VersionedEntityType; + activity: UseVersionActivityResult; + include: ActivityInclude; + canRestore?: boolean; + onIncludeChange: (include: ActivityInclude) => void; + /** + * Free-text search is controlled by the container, which debounces it + * and feeds it to the activity hook so the server filters the full + * history (not just the loaded pages). The panel renders the matches + * verbatim. + */ + searchTerm: string; + onSearchChange: (value: string) => void; + previewedTransactionId: number | null; + onClose: () => void; + onPreview: (group: SaveGroup) => void; + /** Leave an active historical preview (back to the live version). */ + onExitPreview?: () => void; + onRestore: (group: SaveGroup) => void; + onOpenAsNew: (group: SaveGroup) => void; + onOpenRelated?: (record: ActivityRecord) => void; + sessionEntries?: SessionLogEntry[]; +} + +export default function VersionHistoryPanel({ + entityType, + activity, + include, + canRestore = true, + onIncludeChange, + searchTerm, + onSearchChange, + previewedTransactionId, + onClose, + onPreview, + onExitPreview, + onRestore, + onOpenAsNew, + onOpenRelated, + sessionEntries = [], +}: VersionHistoryPanelProps) { + const { timeline, newestGroup, isLoading, error, hasMore, loadMore } = + activity; + + const includeOptions = useMemo( + () => [ + { value: 'all', label: t('All changes') }, + { + value: 'self', + label: + entityType === 'chart' + ? t('This chart only') + : t('This dashboard only'), + }, + { value: 'related', label: t('Related items only') }, + ], + [entityType], + ); + + const hasSearch = searchTerm.trim().length > 0; + + // The newest self save IS the live state: it gets a "Current" tag, + // no preview affordances, and no restore action. The newest save + // being a restore additionally means the live entity matches an + // older version; surface that in the "Current version" section. + // newestGroup comes from the hook's last unfiltered fetch — the visible + // timeline is search-filtered, so its first group may be an older save. + const currentTransactionId = newestGroup?.transactionId ?? null; Review Comment: Confirmed and fixed in `3b7a1fb944` — same root cause as the `ExploreVersionHistory.tsx` finding, one fix for both. One correction to the stated consequence: because records are ordered newest-first, an *older* save can never be mis-tagged as Current — the failure mode is the marker dropping to null when newer related records fill page 0 (which also made restore appear on the true live version, a harmless server-side no-op). The dedicated `include=self` probe restores a single authoritative source; a regression test pins the related-records-fill-the-page case. _Triaged and fixed 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]
