codeant-ai-for-open-source[bot] commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3707200099
########## 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: **Suggestion:** When `position_json` is null or empty, this branch never adds `metadata.positions` to the copy payload. The copy endpoint then receives no snapshot layout, so `set_dash_metadata` does not rebuild the new dashboard's layout or chart associations and the fork can retain the source dashboard's current charts instead of representing the empty historical version. Always include an explicit empty positions object for snapshots without a layout. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Open-as-new produces an incorrect dashboard for empty-layout snapshots. - ⚠️ Current chart associations can leak into historical dashboard forks. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3b86602d7c5c43eea4156c67e9524100&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=3b86602d7c5c43eea4156c67e9524100&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/api.ts **Line:** 290:307 **Comment:** *Api Mismatch: When `position_json` is null or empty, this branch never adds `metadata.positions` to the copy payload. The copy endpoint then receives no snapshot layout, so `set_dash_metadata` does not rebuild the new dashboard's layout or chart associations and the fork can retain the source dashboard's current charts instead of representing the empty historical version. Always include an explicit empty positions object for snapshots without a layout. 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=3d6b9eff1d5d209ea0a43e8c98b54e06fb3386766ac207d1bcaf41a08298ac5a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41551&comment_hash=3d6b9eff1d5d209ea0a43e8c98b54e06fb3386766ac207d1bcaf41a08298ac5a&reaction=dislike'>👎</a> ########## superset-frontend/src/features/versionHistory/useVersionActions.tsx: ########## @@ -0,0 +1,323 @@ +/** + * 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, 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; +} + +// The in-flight locks live at module scope because the invariant they +// protect is entity-wide, not instance-wide: the preview banner and the +// history panel each mount their own useVersionActions for the same +// entity, so a ref-scoped lock would let the banner's activation slip +// past the panel's and fork a duplicate (or start a second restore). +// State guards alone are no lock at all — two activations in one tick +// both read the pre-update state value. Keys are removed in `finally`, +// so the set is self-cleaning; the per-instance isRestoring/isCreating +// state remains only to drive that instance's spinner. +const inFlightActions = new Set<string>(); + +const restoreLockKey = (entityType: string, uuid: string): string => + `restore:${entityType}:${uuid}`; +const forkLockKey = ( + entityType: string, + uuid: string, + versionUuid: string, +): string => `fork:${entityType}:${uuid}:${versionUuid}`; + +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); + + // 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); + }, []); + + // TODO(version-history): backend workaround — remove when the restore + // endpoint reports whether it created a version (e.g. `created: boolean` + // in its response). The 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) { + return; + } + // Entity-wide, not per-version: two concurrent restores to different + // versions of the same entity would race each other's rehydration. + const lockKey = restoreLockKey(entityType, uuid); + if (inFlightActions.has(lockKey)) { + 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; + } + inFlightActions.add(lockKey); + 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(uuid)); + dispatch(versionRestored(uuid)); Review Comment: **Suggestion:** The restore completion unconditionally dispatches `clearVersionPreview(uuid)`. If the user exits the original preview and selects another version of the same entity while the restore request is in flight, the reducer sees the same entity UUID and clears the newly selected preview as well. Scope the completion action to the version that initiated the restore, or only clear the preview when it still matches the original target. [stale reference] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ A restore completion unexpectedly exits a newer preview. - ⚠️ Users lose the historical version they selected afterward. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=eef2f5a258194f28a40ecd102191f85a&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=eef2f5a258194f28a40ecd102191f85a&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:** 215:216 **Comment:** *Stale Reference: The restore completion unconditionally dispatches `clearVersionPreview(uuid)`. If the user exits the original preview and selects another version of the same entity while the restore request is in flight, the reducer sees the same entity UUID and clears the newly selected preview as well. Scope the completion action to the version that initiated the restore, or only clear the preview when it still matches the original target. 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=bf6ff5eb05c6ee4502180ac85c09eae7ab3c8d938cfb24e4b1e21744cf72c52f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41551&comment_hash=bf6ff5eb05c6ee4502180ac85c09eae7ab3c8d938cfb24e4b1e21744cf72c52f&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]
