mikebridge commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3691103979
########## 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: Confirmed and fixed in `308f5cb859`. The concrete danger is the in-place slice swap (save-as), which the container's own uuid-caching already treats as real — the rehydration fetch just hadn't gotten the same care. The effect now cancels its in-flight fetch on cleanup; the regression test drives a restore, unmounts before the fetch resolves, and asserts no hydration is dispatched — confirmed to fail without the cancellation. _Triaged and fixed by Claude (AI) on behalf of @mikebridge._ ########## superset-frontend/src/features/versionHistory/sessionLogMiddleware.ts: ########## @@ -0,0 +1,86 @@ +/** + * 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 type { Middleware } from 'redux'; +import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core'; +import { t } from '@apache-superset/core/translation'; +import { appendVersionSessionLog, clearVersionSessionLog } from './reducer'; + +// Action types are inlined (rather than imported from the explore +// module) so this middleware does not pull explore code into every +// page's bundle; the store applies it globally. Exported so the test +// suite can assert they match the real explore constants — a rename +// there would otherwise silently kill the session log. +export const SET_FIELD_VALUE = 'SET_FIELD_VALUE'; +export const HYDRATE_EXPLORE = 'HYDRATE_EXPLORE'; + +interface SessionLogState { + user?: { firstName?: string; lastName?: string }; + explore?: { + controls?: Record<string, { label?: unknown } | undefined>; + }; +} + +function controlLabel(state: SessionLogState, controlName: string): string { + const label = state.explore?.controls?.[controlName]?.label; + return typeof label === 'string' && label + ? label + : controlName.replace(/_/g, ' '); +} + +function userName(state: SessionLogState): string | null { + const name = [state.user?.firstName, state.user?.lastName] + .filter(Boolean) + .join(' '); + return name || null; +} + +/** + * Records unsaved explore control changes in the version history + * session log ("Current version" section) and resets the log whenever + * the explore page (re)hydrates — initial load, save, or restore. + */ +export const versionSessionLogMiddleware: Middleware = + store => next => action => { + const result = next(action); + if (!isFeatureEnabled(FeatureFlag.VersionHistory)) { + return result; + } + if (action.type === HYDRATE_EXPLORE) { + store.dispatch(clearVersionSessionLog()); + } else if ( + action.type === SET_FIELD_VALUE && + typeof action.controlName === 'string' && + // Effects rewrite controls with no user gesture (transferred-control + // cleanup after load, derived values set alongside another control); + // logging those would tell the user they have unsaved edits they never + // made. Only user-initiated changes belong in the session log. + !action.programmatic + ) { Review Comment: Partially confirmed. All fifteen `setControlValue` call sites were audited when the mark was introduced; the six effect-driven rewrites (transferred-control cleanup, tooltip_template, four axis margins) were marked then. Re-auditing against this finding surfaced one more that belongs in the derived class: the `granularity_sqla` rewrite after a datasource swap — the user's gesture is the swap, not a time-column edit. Marked in `d704f6ae88`. The remaining unmarked sites all follow a real user gesture: the dialog-confirmed adhoc-filter add, the clear-form reset, the temporal-filter deletion reset, annotation-layer edits, and drag edits of axis titles from the chart. Logging those is the intent — they are unsaved changes the user made. If a future rewrite path appears, the fail-safe is the mark, and the middleware test pins the key against the real action creator so a rename on either side fails the suite. _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]
