mikebridge commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3707404671
########## superset-frontend/src/features/versionHistory/api.ts: ########## @@ -0,0 +1,390 @@ +/** + * 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 { JsonObject, SupersetClient } from '@superset-ui/core'; +import rison from 'rison'; +import { t } from '@apache-superset/core/translation'; +import { DASHBOARD_GET_COLUMNS } from 'src/hooks/apiResources/dashboards'; +import { CHART_TYPE, MARKDOWN_TYPE } from 'src/dashboard/util/componentTypes'; +import type { ExploreResponsePayload } from 'src/explore/types'; +import type { + HydrateChartData, + HydrateDashboardData, +} from 'src/dashboard/actions/hydrate'; +import type { Dashboard } from 'src/types/Dashboard'; +import type { + ActivityEntityKind, + ActivityInclude, + ActivityResponse, + ChartVersionSnapshot, + DashboardVersionSnapshot, + VersionedEntityType, + VersionSnapshot, +} from './types'; + +const API_RESOURCE: Record<VersionedEntityType, string> = { + chart: 'chart', + dashboard: 'dashboard', +}; + +export interface FetchActivityOptions { + include?: ActivityInclude; + page?: number; + pageSize?: number; + /** + * Case-insensitive free-text search over the full history (not just the + * loaded pages) — the server filters before paginating, so `count` + * reflects matches. Debounced upstream. + */ + q?: string; +} + +export async function fetchActivity( + entityType: VersionedEntityType, + uuid: string, + { include = 'all', page = 0, pageSize = 25, q }: FetchActivityOptions = {}, +): Promise<ActivityResponse> { + const params = new URLSearchParams({ + include, + page: String(page), + page_size: String(pageSize), + }); + const trimmedQ = q?.trim(); + if (trimmedQ) { + params.set('q', trimmedQ); + } + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/${API_RESOURCE[entityType]}/${encodeURIComponent(uuid)}/activity/?${params.toString()}`, + }); + return json as ActivityResponse; +} + +export async function fetchVersionSnapshot( + entityType: 'chart', + uuid: string, + versionUuid: string, +): Promise<ChartVersionSnapshot>; +export async function fetchVersionSnapshot( + entityType: 'dashboard', + uuid: string, + versionUuid: string, +): Promise<DashboardVersionSnapshot>; +export async function fetchVersionSnapshot( + entityType: VersionedEntityType, + uuid: string, + versionUuid: string, +): Promise<VersionSnapshot>; +export async function fetchVersionSnapshot( + entityType: VersionedEntityType, + uuid: string, + versionUuid: string, +): Promise<VersionSnapshot> { + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/${API_RESOURCE[entityType]}/${encodeURIComponent(uuid)}/versions/${encodeURIComponent(versionUuid)}/`, + }); + return (json as { result: VersionSnapshot }).result; +} + +export async function restoreVersion( + entityType: VersionedEntityType, + uuid: string, + versionUuid: string, +): Promise<{ message: string }> { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/${API_RESOURCE[entityType]}/${encodeURIComponent(uuid)}/versions/${encodeURIComponent(versionUuid)}/restore`, + }); + return json as { message: string }; +} + +/** Creates a new chart from a version snapshot; returns the new chart id. */ +export async function createChartFromSnapshot( + snapshot: ChartVersionSnapshot, + name: string, +): Promise<number> { + // The chart POST requires all three; the version table allows null for each + // (a delete version carries nulls throughout). Fail here with something the + // caller can turn into a toast rather than sending a payload the API will + // reject with a validation error the user cannot act on. + if ( + snapshot.viz_type == null || + snapshot.datasource_id == null || + snapshot.datasource_type == null + ) { + throw new Error( + 'This version does not record a visualization type and dataset, so a new chart cannot be built from it', + ); + } + const { json } = await SupersetClient.post({ + endpoint: '/api/v1/chart/', + jsonPayload: { + slice_name: name, + viz_type: snapshot.viz_type, + datasource_id: snapshot.datasource_id, + datasource_type: snapshot.datasource_type, + ...(snapshot.params != null && { params: snapshot.params }), + ...(snapshot.query_context != null && { + query_context: snapshot.query_context, + }), + ...(snapshot.description != null && { + description: snapshot.description, + }), + ...(snapshot.cache_timeout != null && { + cache_timeout: snapshot.cache_timeout, + }), + }, + }); + return (json as { id: number }).id; +} + +/** The theme shape `dashboardInfo` holds, keyed by id in the snapshot. */ +export type DashboardTheme = NonNullable<Dashboard['theme']>; + +/** + * Resolves a snapshot's `theme_id` to the theme object hydration expects. + * The version table stores the foreign key, not the theme, so a snapshot + * taken under a different theme than the live dashboard needs one lookup. + */ +export async function fetchDashboardTheme( + themeId: number, +): Promise<DashboardTheme> { + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/theme/${themeId}`, + }); + const { id, result } = json as { + id: number; + result: Omit<DashboardTheme, 'id'>; + }; + return { ...result, id }; +} + +/** + * Activity records identify related entities by uuid only; resolve the + * numeric id (needed for page urls) at click time via the list API. + */ +export async function resolveEntityId( + kind: ActivityEntityKind, + uuid: string, +): Promise<number | null> { + const resource: Record<ActivityEntityKind, string> = { + chart: 'chart', + dashboard: 'dashboard', + dataset: 'dataset', + }; + const q = rison.encode({ + columns: ['id'], + filters: [{ col: 'uuid', opr: 'eq', value: uuid }], + page_size: 1, + }); + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/${resource[kind]}/?q=${q}`, + }); + const { result } = json as { result: Array<{ id: number }> }; + return result.length > 0 ? result[0].id : null; +} + +/** The chart id a layout slot references, or null for non-chart slots. */ +export const layoutChartId = (item: JsonObject): number | null => { + const meta = item?.meta as JsonObject | undefined; + return item?.type === CHART_TYPE && typeof meta?.chartId === 'number' + ? (meta.chartId as number) + : null; +}; + +/** + * Swaps layout slots whose chart is unreachable (deleted, or not visible + * to the current user) for a markdown placeholder, preserving the slot's + * footprint so the rest of the layout is unaffected. + */ +export function swapUnreachableChartSlots( + positionData: JsonObject, + unreachableIds: Set<number>, +): JsonObject { + if (unreachableIds.size === 0) { + return positionData; + } + const layout: JsonObject = { ...positionData }; + Object.entries(layout).forEach(([key, item]) => { + const chartId = layoutChartId(item as JsonObject); + if (chartId !== null && unreachableIds.has(chartId)) { + const meta = (item as JsonObject).meta as JsonObject; + layout[key] = { + ...(item as JsonObject), + type: MARKDOWN_TYPE, + meta: { + width: meta?.width, + height: meta?.height, + code: t('This chart no longer exists.'), + }, + }; + } + }); + return layout; +} + +// FAB list endpoints clamp page_size server-side; batches must stay under +// that cap or reachable charts past it would silently be reported missing. +const REACHABLE_CHART_BATCH_SIZE = 100; + +/** The subset of the given chart ids that the list API can resolve. */ +async function fetchReachableChartIds( + chartIds: number[], +): Promise<Set<number>> { + const batches: number[][] = []; + for (let i = 0; i < chartIds.length; i += REACHABLE_CHART_BATCH_SIZE) { + batches.push(chartIds.slice(i, i + REACHABLE_CHART_BATCH_SIZE)); + } + const results = await Promise.all( + batches.map(async batch => { + const q = rison.encode({ + columns: ['id'], + filters: [{ col: 'id', opr: 'in', value: batch }], + page_size: batch.length, + }); + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/chart/?q=${q}`, + }); + const { result } = json as { result: Array<{ id: number }> }; + return result.map(({ id }) => id); + }), + ); + return new Set(results.flat()); +} + +/** + * Forks a dashboard version into a new dashboard via the copy endpoint; + * returns the new dashboard id. The copy endpoint derives the new + * dashboard's chart associations from the `positions` key of + * `json_metadata`, so the fork references (shares, not duplicates) + * exactly the charts present in the snapshot's layout. Slots whose chart + * no longer resolves are swapped for the same markdown placeholder the + * preview renders — the copy endpoint would silently skip their chart + * associations, leaving dead slots in the forked layout. + */ +export async function createDashboardFromSnapshot( + sourceUuid: string, + snapshot: DashboardVersionSnapshot, + name: string, +): Promise<number> { + const sourceId = await resolveEntityId('dashboard', sourceUuid); + if (sourceId === null) { + throw new Error(`No dashboard found for uuid ${sourceUuid}`); + } + const metadata: JsonObject = snapshot.json_metadata + ? JSON.parse(snapshot.json_metadata) + : {}; + if (snapshot.position_json) { + let positions: JsonObject = JSON.parse(snapshot.position_json); + const chartIds = new Set<number>(); + Object.values(positions).forEach(item => { + const chartId = layoutChartId(item as JsonObject); + if (chartId !== null) { + chartIds.add(chartId); + } + }); + if (chartIds.size > 0) { + const reachable = await fetchReachableChartIds([...chartIds]); + const unreachable = new Set( + [...chartIds].filter(id => !reachable.has(id)), + ); + positions = swapUnreachableChartSlots(positions, unreachable); + } + metadata.positions = positions; + } Review Comment: *Generated by Claude (AI) on behalf of @mikebridge.* Confirmed and fixed in `ae8e6a4fc8` — good catch, this one was real. `metadata.positions` was only set inside the `if (snapshot.position_json)` branch, so a snapshot with no layout sent `json_metadata` without the key at all, and the copy endpoint left the fork carrying the source dashboard's *current* charts — today's dashboard under a historical name. It now always sends `positions`, empty when the snapshot had none. Note the existing test `createDashboardFromSnapshot sends empty layout fields as-is` asserted the buggy shape (`json_metadata` deep-equal `{}`); it's renamed and now pins the fix, and fails against the previous code. ########## superset-frontend/src/features/versionHistory/ExploreVersionHistory.tsx: ########## @@ -0,0 +1,354 @@ +/** + * 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); + + // The URL param is honoured once per mount. It persists for the whole + // visit, and this effect re-runs whenever `canRestore` moves — a late + // false→true flip (slice metadata refetch, an editors change landing) + // would otherwise re-open a panel the user explicitly closed. + const urlParamHandledRef = useRef(false); + 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 ( + !urlParamHandledRef.current && + getUrlParam(URL_PARAMS.versionHistory) && + canRestore + ) { + urlParamHandledRef.current = true; + 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; + // Invalidation token for the in-flight post-restore rehydration. + // hydrateExplore rewrites the whole explore store, so a fetch resolving + // after the page unmounted — or after a save-as swapped the slice in + // place — would overwrite the newly loaded chart's state with the old + // chart's payload. The token belongs to the fetch, not to the effect: + // the effect's identity moves whenever the debounced search term or the + // include filter does (both feed `refreshActivity`), and cancelling on + // those re-runs would drop a live rehydration that nothing re-issues. + const restoreHydrationIdRef = useRef(0); + // Only unmount and a slice/uuid swap supersede a rehydration here; a + // newer restore bumps the token where it starts its own fetch. + useEffect( + () => () => { + restoreHydrationIdRef.current += 1; + }, + [sliceId, uuid], + ); + useEffect(() => { + if (restoreCount !== lastRestoreCountRef.current) { + lastRestoreCountRef.current = restoreCount; + // Guard: 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. + if (lastRestoredUuid === uuid) { + // 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) { + restoreHydrationIdRef.current += 1; + const hydrationId = restoreHydrationIdRef.current; + const isCurrent = () => restoreHydrationIdRef.current === hydrationId; + fetchExploreRehydrationData(sliceId) + .then(result => { + if (isCurrent()) { + dispatch( + hydrateExplore({ ...result, saveAction: 'overwrite' }), + ); + } Review Comment: *Generated by Claude (AI) on behalf of @mikebridge.* Confirmed and fixed in `ae8e6a4fc8`. The rehydration token was bumped by unmount, a slice/uuid swap and a newer restore — but not by a save, so a save committing mid-flight left the payload in hand older than the store, and hydrating it rolled the chart back over the newer save. The dashboard sibling already had exactly this guard (`saveSignalAtStart` in `useDashboardVersionPreview`); the explore side now captures the save signal at fetch start and drops the stale payload at resolve, since the save's own in-place hydration is already correct. New regression test fails against the previous code. -- 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]
