codeant-ai-for-open-source[bot] commented on code in PR #40912: URL: https://github.com/apache/superset/pull/40912#discussion_r3382646373
########## superset-frontend/src/features/lineage/LineageView.tsx: ########## @@ -0,0 +1,690 @@ +/** + * 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 { FC, useMemo, useState, useCallback } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { styled, useTheme } from '@apache-superset/core/theme'; +import { Empty, Loading } from '@superset-ui/core/components'; +import { Button } from '@superset-ui/core/components'; +import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; +import type { Resource } from 'src/hooks/apiResources/apiResources'; +import type { + DatasetLineage, + ChartLineage, + DashboardLineage, + ChartEntity, + DashboardEntity, + DatasetEntity, + DatabaseEntity, +} from 'src/hooks/apiResources/lineage'; +import Echart from '../../../plugins/plugin-chart-echarts/src/components/Echart'; +import type { EChartsCoreOption } from 'echarts/core'; + +const LineageContainer = styled.div` + display: flex; + flex-direction: column; + width: 100%; + height: 100%; +`; + +const Legend = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: center; + align-items: center; + gap: ${theme.sizeUnit * 4}px; + padding: ${theme.sizeUnit * 3}px; + background-color: ${theme.colorBgLayout}; + border-bottom: 1px solid ${theme.colorBorder}; + `} +`; + +const LegendItem = styled.div<{ color: string }>` + ${({ theme, color }) => ` + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + font-size: ${theme.fontSizeSM}px; + color: ${theme.colorText}; + + &::before { + content: ''; + width: 12px; + height: 12px; + border-radius: 2px; + background-color: ${color}; + } + `} +`; + +const DetailsPanel = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 4}px; + background-color: ${theme.colorBgLayout}; + border-top: 1px solid ${theme.colorBorder}; + min-height: 120px; + `} +`; + +const DetailsPanelHeader = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 3}px; + `} +`; + +const DetailsPanelActions = styled.div` + ${({ theme }) => ` + display: flex; + gap: ${theme.sizeUnit * 2}px; + `} +`; + +const DetailsPanelTitle = styled.h4` + ${({ theme }) => ` + margin: 0; + font-size: ${theme.fontSizeLG}px; + font-weight: ${theme.fontWeightStrong}; + color: ${theme.colorText}; + `} +`; + +const DetailsPanelContent = styled.div` + ${({ theme }) => ` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit * 2}px; + `} +`; + +const DetailRow = styled.div` + ${({ theme }) => ` + display: flex; + gap: ${theme.sizeUnit * 2}px; + font-size: ${theme.fontSizeSM}px; + color: ${theme.colorText}; + `} +`; + +const DetailLabel = styled.span` + ${({ theme }) => ` + font-weight: ${theme.fontWeightStrong}; + min-width: 100px; + `} +`; + +const DetailValue = styled.span` + ${({ theme }) => ` + color: ${theme.colorTextSecondary}; + `} +`; + +type NodeDetails = { + name: string; + type: 'database' | 'dataset' | 'chart' | 'dashboard'; + id?: number; + additionalInfo?: Record<string, any>; +}; + +type LineageViewProps = { + lineageResource: + | Resource<DatasetLineage> + | Resource<ChartLineage> + | Resource<DashboardLineage>; + entityType: 'dataset' | 'chart' | 'dashboard'; +}; + +const LineageView: FC<LineageViewProps> = ({ lineageResource, entityType }) => { + const theme = useTheme(); + const [selectedNode, setSelectedNode] = useState<NodeDetails | null>(null); + + // Create a mapping of node names to their details + const nodeDetailsMap = useMemo(() => { + if ( + lineageResource.status !== ResourceStatus.Complete || + !lineageResource.result + ) { + return new Map<string, NodeDetails>(); + } + + const data = lineageResource.result; + const map = new Map<string, NodeDetails>(); + + if (entityType === 'dataset' && 'dataset' in data) { + const { dataset, upstream, downstream } = data as DatasetLineage; + + // Add current dataset + map.set(dataset.name, { + name: dataset.name, + type: 'dataset', + id: dataset.id, + additionalInfo: { + schema: dataset.schema, + table_name: dataset.table_name, + database_name: dataset.database_name, + }, + }); + + // Add upstream database + if (upstream?.database) { + map.set(upstream.database.database_name, { + name: upstream.database.database_name, + type: 'database', + id: upstream.database.id, + }); + } + + // Add downstream charts + if (downstream?.charts?.result) { + downstream.charts.result.forEach((chart: ChartEntity) => { + map.set(chart.slice_name, { + name: chart.slice_name, + type: 'chart', + id: chart.id, + additionalInfo: { + viz_type: chart.viz_type, + }, + }); Review Comment: **Suggestion:** The details lookup map is keyed by non-unique labels (e.g., `slice_name`/`title`), so later entries overwrite earlier ones when names collide. Clicking a node can then show/open the wrong entity; key the details map by the same unique node identifier used by the graph. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Lineage node details show wrong chart when names collide. - ❌ "Open Chart" button navigates to incorrect slice for node. - ⚠️ Users may misinterpret lineage impact across dashboards and datasets. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `LineageView`'s `nodeDetailsMap` builder at `superset-frontend/src/features/lineage/LineageView.tsx:170-221`, dataset lineage stores downstream charts with `map.set(chart.slice_name, { name: chart.slice_name, id: chart.id, ... })`, and chart/dataset/dashboard nodes similarly use `dataset.name` or `dashboard.title` as the map key. 2. The entity types in `superset-frontend/src/hooks/apiResources/lineage.ts:41-55` show that `ChartEntity.slice_name` and `DashboardEntity.title` are not required to be unique, so `downstream.charts.result` or `downstream.dashboards.result` can legitimately contain multiple entities sharing the same label. 3. When dataset lineage is opened (e.g., via the dedicated edit page at `superset-frontend/src/features/datasets/AddDataset/EditDataset/index.tsx:66-99` or the classic editor modal through `DatasourceModal` → `DatasourceEditor` at `components/Datasource/DatasourceModal/index.tsx:61-70` and `components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:2091-2138`), and the API returns two charts with identical `slice_name`, the second `map.set(chart.slice_name, { ... id: chart.id })` at `LineageView.tsx:195-205` overwrites the first entry. 4. Later, when the user clicks the chart node in the Sankey, `handleNodeClick` at `LineageView.tsx:341-347` looks up `nodeDetailsMap.get(params.name)`, so the colliding label returns the *last* chart stored under that key; the details panel and "Open Chart" button then display and navigate to the wrong chart ID, mis-associating lineage details whenever labels collide. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=91c10252b7334923a4d9d4ecfd32c492&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=91c10252b7334923a4d9d4ecfd32c492&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/lineage/LineageView.tsx **Line:** 197:204 **Comment:** *Logic Error: The details lookup map is keyed by non-unique labels (e.g., `slice_name`/`title`), so later entries overwrite earlier ones when names collide. Clicking a node can then show/open the wrong entity; key the details map by the same unique node identifier used by the graph. 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%2F40912&comment_hash=58ffcf6554f3e291a44f16f0837fe164a906b03fe3b43b9c83348deddac6ff8e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40912&comment_hash=58ffcf6554f3e291a44f16f0837fe164a906b03fe3b43b9c83348deddac6ff8e&reaction=dislike'>👎</a> ########## superset-frontend/src/features/lineage/LineageView.tsx: ########## @@ -0,0 +1,690 @@ +/** + * 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 { FC, useMemo, useState, useCallback } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { styled, useTheme } from '@apache-superset/core/theme'; +import { Empty, Loading } from '@superset-ui/core/components'; +import { Button } from '@superset-ui/core/components'; +import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; +import type { Resource } from 'src/hooks/apiResources/apiResources'; +import type { + DatasetLineage, + ChartLineage, + DashboardLineage, + ChartEntity, + DashboardEntity, + DatasetEntity, + DatabaseEntity, +} from 'src/hooks/apiResources/lineage'; +import Echart from '../../../plugins/plugin-chart-echarts/src/components/Echart'; +import type { EChartsCoreOption } from 'echarts/core'; + +const LineageContainer = styled.div` + display: flex; + flex-direction: column; + width: 100%; + height: 100%; +`; + +const Legend = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: center; + align-items: center; + gap: ${theme.sizeUnit * 4}px; + padding: ${theme.sizeUnit * 3}px; + background-color: ${theme.colorBgLayout}; + border-bottom: 1px solid ${theme.colorBorder}; + `} +`; + +const LegendItem = styled.div<{ color: string }>` + ${({ theme, color }) => ` + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + font-size: ${theme.fontSizeSM}px; + color: ${theme.colorText}; + + &::before { + content: ''; + width: 12px; + height: 12px; + border-radius: 2px; + background-color: ${color}; + } + `} +`; + +const DetailsPanel = styled.div` + ${({ theme }) => ` + padding: ${theme.sizeUnit * 4}px; + background-color: ${theme.colorBgLayout}; + border-top: 1px solid ${theme.colorBorder}; + min-height: 120px; + `} +`; + +const DetailsPanelHeader = styled.div` + ${({ theme }) => ` + display: flex; + justify-content: space-between; + align-items: center; + margin-bottom: ${theme.sizeUnit * 3}px; + `} +`; + +const DetailsPanelActions = styled.div` + ${({ theme }) => ` + display: flex; + gap: ${theme.sizeUnit * 2}px; + `} +`; + +const DetailsPanelTitle = styled.h4` + ${({ theme }) => ` + margin: 0; + font-size: ${theme.fontSizeLG}px; + font-weight: ${theme.fontWeightStrong}; + color: ${theme.colorText}; + `} +`; + +const DetailsPanelContent = styled.div` + ${({ theme }) => ` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit * 2}px; + `} +`; + +const DetailRow = styled.div` + ${({ theme }) => ` + display: flex; + gap: ${theme.sizeUnit * 2}px; + font-size: ${theme.fontSizeSM}px; + color: ${theme.colorText}; + `} +`; + +const DetailLabel = styled.span` + ${({ theme }) => ` + font-weight: ${theme.fontWeightStrong}; + min-width: 100px; + `} +`; + +const DetailValue = styled.span` + ${({ theme }) => ` + color: ${theme.colorTextSecondary}; + `} +`; + +type NodeDetails = { + name: string; + type: 'database' | 'dataset' | 'chart' | 'dashboard'; + id?: number; + additionalInfo?: Record<string, any>; +}; + +type LineageViewProps = { + lineageResource: + | Resource<DatasetLineage> + | Resource<ChartLineage> + | Resource<DashboardLineage>; + entityType: 'dataset' | 'chart' | 'dashboard'; +}; + +const LineageView: FC<LineageViewProps> = ({ lineageResource, entityType }) => { + const theme = useTheme(); + const [selectedNode, setSelectedNode] = useState<NodeDetails | null>(null); + + // Create a mapping of node names to their details + const nodeDetailsMap = useMemo(() => { + if ( + lineageResource.status !== ResourceStatus.Complete || + !lineageResource.result + ) { + return new Map<string, NodeDetails>(); + } + + const data = lineageResource.result; + const map = new Map<string, NodeDetails>(); + + if (entityType === 'dataset' && 'dataset' in data) { + const { dataset, upstream, downstream } = data as DatasetLineage; + + // Add current dataset + map.set(dataset.name, { + name: dataset.name, + type: 'dataset', + id: dataset.id, + additionalInfo: { + schema: dataset.schema, + table_name: dataset.table_name, + database_name: dataset.database_name, + }, + }); + + // Add upstream database + if (upstream?.database) { + map.set(upstream.database.database_name, { + name: upstream.database.database_name, + type: 'database', + id: upstream.database.id, + }); + } + + // Add downstream charts + if (downstream?.charts?.result) { + downstream.charts.result.forEach((chart: ChartEntity) => { + map.set(chart.slice_name, { + name: chart.slice_name, + type: 'chart', + id: chart.id, + additionalInfo: { + viz_type: chart.viz_type, + }, + }); + }); + } + + // Add downstream dashboards + if (downstream?.dashboards?.result) { + downstream.dashboards.result.forEach((dashboard: DashboardEntity) => { + map.set(dashboard.title, { + name: dashboard.title, + type: 'dashboard', + id: dashboard.id, + additionalInfo: { + slug: dashboard.slug, + }, + }); + }); + } + } else if (entityType === 'chart' && 'chart' in data) { + const { chart, upstream, downstream } = data as ChartLineage; + + // Add current chart + map.set(chart.slice_name, { + name: chart.slice_name, + type: 'chart', + id: chart.id, + additionalInfo: { + viz_type: chart.viz_type, + }, + }); + + // Add upstream dataset + if (upstream?.dataset) { + map.set(upstream.dataset.name, { + name: upstream.dataset.name, + type: 'dataset', + id: upstream.dataset.id, + additionalInfo: { + schema: upstream.dataset.schema, + table_name: upstream.dataset.table_name, + }, + }); + } + + // Add upstream database + if (upstream?.database) { + map.set(upstream.database.database_name, { + name: upstream.database.database_name, + type: 'database', + id: upstream.database.id, + }); + } + + // Add downstream dashboards + if (downstream?.dashboards?.result) { + downstream.dashboards.result.forEach((dashboard: DashboardEntity) => { + map.set(dashboard.title, { + name: dashboard.title, + type: 'dashboard', + id: dashboard.id, + additionalInfo: { + slug: dashboard.slug, + }, + }); + }); + } + } else if (entityType === 'dashboard' && 'dashboard' in data) { + const { dashboard, upstream } = data as DashboardLineage; + + // Add current dashboard + map.set(dashboard.title, { + name: dashboard.title, + type: 'dashboard', + id: dashboard.id, + additionalInfo: { + slug: dashboard.slug, + }, + }); + + // First pass: detect duplicate chart names + const chartNameCounts = new Map<string, number>(); + if (upstream?.charts?.result) { + upstream.charts.result.forEach((chart: ChartEntity) => { + const count = chartNameCounts.get(chart.slice_name) || 0; + chartNameCounts.set(chart.slice_name, count + 1); + }); + } + + // Add upstream charts + if (upstream?.charts?.result) { + upstream.charts.result.forEach((chart: ChartEntity) => { + // Only append ID if there are duplicate names + const hasDuplicate = (chartNameCounts.get(chart.slice_name) || 0) > 1; + const chartNodeName = hasDuplicate + ? `${chart.slice_name} (#${chart.id})` + : chart.slice_name; + map.set(chartNodeName, { + name: chart.slice_name, + type: 'chart', + id: chart.id, + additionalInfo: { + viz_type: chart.viz_type, + }, + }); + }); + } + + // Add upstream datasets + if (upstream?.datasets?.result) { + upstream.datasets.result.forEach((dataset: DatasetEntity) => { + map.set(dataset.name, { + name: dataset.name, + type: 'dataset', + id: dataset.id, + additionalInfo: { + schema: dataset.schema, + table_name: dataset.table_name, + }, + }); + }); + } + + // Add upstream databases + if (upstream?.databases?.result) { + upstream.databases.result.forEach((database: DatabaseEntity) => { + map.set(database.database_name, { + name: database.database_name, + type: 'database', + id: database.id, + }); + }); + } + } + + return map; + }, [lineageResource, entityType]); + + // Handle node click + const handleNodeClick = useCallback( + (params: any) => { + if (params.dataType === 'node') { + const nodeName = params.name; + const nodeDetails = nodeDetailsMap.get(nodeName); + if (nodeDetails) { + setSelectedNode(nodeDetails); + } + } + // Always stop event propagation to prevent tooltip issues + if (params.event) { + params.event.stop(); + } + }, + [nodeDetailsMap], + ); + + const echartOptions: EChartsCoreOption | null = useMemo(() => { + if ( + lineageResource.status !== ResourceStatus.Complete || + !lineageResource.result + ) { + return null; + } + + const data = lineageResource.result; + const nodes: { + name: string; + itemStyle?: { color: string }; + label?: { position?: string }; + }[] = []; + const links: { source: string; target: string; value: number }[] = []; + const nodeSet = new Set<string>(); + + // Helper to add a node with label position + const addNode = ( + name: string, + color: string, + labelPosition: 'left' | 'right' | 'inside', + ) => { + if (!nodeSet.has(name)) { + nodeSet.add(name); + nodes.push({ + name, + itemStyle: { color }, + label: { + position: labelPosition, + }, + }); + } Review Comment: **Suggestion:** Nodes are deduplicated only by display `name`, so distinct entities that share the same title/name collapse into one Sankey node. This corrupts lineage topology (merged links and lost entities); use a stable unique node key (e.g., type + ID) and keep label text separate. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Dataset lineage Sankey merges charts with duplicate slice_name labels. - ❌ Dashboard lineage view collapses datasets sharing identical display names. - ⚠️ Lineage visualization under-represents upstream/downstream relationships for duplicates. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Observe that lineage data for datasets, charts, and dashboards is fetched via `useDatasetLineage`, `useChartLineage`, and `useDashboardLineage` in `superset-frontend/src/features/lineage/LineageModal.tsx:35-47`, and the selected `lineageResource` is passed into `<LineageView lineageResource={lineageResource} entityType={entityType} />`. 2. The lineage response shapes are defined in `superset-frontend/src/hooks/apiResources/lineage.ts:58-73` (`DatasetLineage`), `76-90` (`ChartLineage`), and `94-113` (`DashboardLineage`); `ChartEntity.slice_name`, `DatasetEntity.name`, and `DashboardEntity.title` are plain strings with no uniqueness guarantee across results. 3. In `LineageView` at `superset-frontend/src/features/lineage/LineageView.tsx:366-373`, the Sankey node set `nodeSet` is declared as `new Set<string>()`, and `addNode` at lines `375-390` uses the *display name* string as the node key: it checks `if (!nodeSet.has(name)) { nodeSet.add(name); nodes.push({ name, ... }); }`. 4. When the backend returns lineage where two distinct entities share the same label (for example, two charts with identical `slice_name` in `downstream.charts.result` of `DatasetLineage`, or two dashboards with the same `title` in `downstream.dashboards.result`), later calls to `addNode` are skipped due to `nodeSet.has(name)`, causing the Sankey graph to merge those entities into a single node and drop the extra edges, corrupting the lineage topology in any lineage entry point (dataset edit page at `features/datasets/AddDataset/EditDataset/index.tsx:66-99`, chart list `ChartCard.tsx:105-125`, or dashboard header menu `useHeaderActionsDropdownMenu.tsx:245-260`). ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bc540d733f30472aafba98b5089697f1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bc540d733f30472aafba98b5089697f1&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/lineage/LineageView.tsx **Line:** 381:390 **Comment:** *Logic Error: Nodes are deduplicated only by display `name`, so distinct entities that share the same title/name collapse into one Sankey node. This corrupts lineage topology (merged links and lost entities); use a stable unique node key (e.g., type + ID) and keep label text separate. 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%2F40912&comment_hash=42103e0499af1af7e61cada7db28571a79cffc5c069ecf20416512a5beabe6c6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40912&comment_hash=42103e0499af1af7e61cada7db28571a79cffc5c069ecf20416512a5beabe6c6&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]
