codeant-ai-for-open-source[bot] commented on code in PR #43483: URL: https://github.com/apache/superset/pull/43483#discussion_r3846296445
########## superset/mcp_service/chart/resources/chart_viewer/src/components/DashboardGrid.tsx: ########## @@ -0,0 +1,211 @@ +/** + * 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. + */ + +/** + * A dashboard rendered as a composite visualization: a layout tree whose + * leaves are ordinary charts, drawn with the SAME renderers a single chart + * uses. + * + * Deliberately static. No native filters, no cross-filters, no shared + * selection — composition and the interaction graph are separate capabilities + * and only the first is prototyped here. + * + * Every cell renders something. A leaf with no data becomes a labelled + * placeholder rather than a gap: a partial composite that silently omits cells + * looks like a complete one, which is the failure mode most likely to make + * this read as broken. + */ +import { useMemo, useState, type JSX } from 'react'; +import type { DashboardCell, DashboardRender, ViewType } from '../types'; +import type { ThemeTokens } from '../theme'; +import { + chartDataToEChartsOption, + defaultViewForChartType, + isEChartsView, + isSubstitutedView, +} from '../adapter'; +import { EChart } from './EChart'; +import { BigNumber } from './BigNumber'; +import { DataTable } from './DataTable'; +import { stripUntrustedMarkers } from '../format'; + +function CellBody({ + cell, + theme, +}: { + cell: DashboardCell; + theme: ThemeTokens; +}): JSX.Element { + const data = cell.data ?? null; + + const view: ViewType | null = useMemo( + () => (data ? defaultViewForChartType(data.chart_type, data) : null), + [data], + ); + + if (!data || !view) { + return ( + <div className="sv-cell-placeholder"> + <span className="sv-cell-placeholder-icon" aria-hidden="true"> + {cell.status === 'error' ? '!' : '—'} + </span> + <span> + {stripUntrustedMarkers(cell.message ?? 'No data for this chart.')} + </span> + </div> + ); + } + + // Same dispatch as the single-chart view (App.tsx), so a cell renders + // exactly as it would on its own. + if (view === 'big_number') return <BigNumber data={data} theme={theme} />; + if (view === 'table' || !isEChartsView(view)) { + return <DataTable data={data} />; + } + return ( + <EChart + option={chartDataToEChartsOption(data, view, { theme })} + scheme={theme.scheme} + /> + ); +} + +/** + * Says so when a cell is NOT the chart Superset draws. + * + * Without this the cell carries the real title and the real numbers in a + * different encoding, which reads as a faithful reproduction. A treemap shown + * as a bar chart is a wrong answer that looks right, and nothing else on + * screen contradicts it. + */ +function SubstitutionNote({ cell }: { cell: DashboardCell }): JSX.Element | null { + const data = cell.data; + if (!data) return null; + const view = defaultViewForChartType(data.chart_type, data); + const native = isSubstitutedView(data.chart_type, view); + if (!native) return null; + return ( + <div className="sv-substitution"> + Shown as a {view} chart — Superset renders this as a {native}. + </div> + ); +} + +export function DashboardGrid({ + render, + theme, +}: { + render: DashboardRender; + theme: ThemeTokens; +}): JSX.Element { + // Tabs are presentational here: cells carry their tab id, so switching tabs + // filters what is shown without re-querying anything. + const tabs = render.tabs ?? []; + + // Counts only cells that actually have data. A tab full of `skipped` + // placeholders is empty as far as the user is concerned — opening on one + // shows a screen of "not queried" messages, which is the same symptom as + // opening on a tab with no cells at all. + const countFor = (id: string): number => + render.cells.filter((c) => c.tab_id === id && c.status === 'ok').length; + + /** Cells present at all, drawable or not — for "did we filter this out?" */ + const cellsIn = (id: string): number => + render.cells.filter((c) => c.tab_id === id).length; + + // Opening tab: the one that was explicitly requested, else the first tab + // that actually HAS cells. + // + // Selecting tabs[0] blindly opened a guaranteed-empty tab whenever the + // render was filtered to a different one — the full tab list is still + // returned (correctly, so the user can switch), so tabs[0] can hold nothing. + // "First non-empty" also covers an unfiltered dashboard whose first tab is + // genuinely empty. + const [activeTab, setActiveTab] = useState<string | null>(() => { + if (!tabs.length) return null; + const requested = render.active_tab_id; + if (requested && tabs.some((t) => t.id === requested)) return requested; + return (tabs.find((t) => countFor(t.id) > 0) ?? tabs[0]).id; + }); + + const cells = activeTab + ? render.cells.filter((c) => c.tab_id === activeTab) + : render.cells; Review Comment: **Suggestion:** When a dashboard has tabs, every cell is filtered by `c.tab_id === activeTab`. However, the dashboard contract explicitly allows `tab_id` to be null for charts that are not inside a tab. Such cells are silently omitted from every tab despite the component's guarantee that no leaf is dropped. Include unassigned cells in the appropriate dashboard view or render them separately instead of filtering them out. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Unassigned dashboard charts are silently omitted. - ⚠️ Composite renders do not satisfy their no-dropped-cells contract. - ⚠️ Users may mistake an incomplete dashboard for a complete one. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4bb8cb59465a46c593df0983f468bee1&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=4bb8cb59465a46c593df0983f468bee1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/resources/chart_viewer/src/components/DashboardGrid.tsx **Line:** 147:149 **Comment:** *Incomplete Implementation: When a dashboard has tabs, every cell is filtered by `c.tab_id === activeTab`. However, the dashboard contract explicitly allows `tab_id` to be null for charts that are not inside a tab. Such cells are silently omitted from every tab despite the component's guarantee that no leaf is dropped. Include unassigned cells in the appropriate dashboard view or render them separately instead of filtering them out. 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%2F43483&comment_hash=fec30c9f3389d0e07a77e51a88328b80c11d78e9cbd1d06a3d52ddd8d3f0050e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=fec30c9f3389d0e07a77e51a88328b80c11d78e9cbd1d06a3d52ddd8d3f0050e&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/resources/chart_viewer/src/components/DashboardGrid.tsx: ########## @@ -0,0 +1,211 @@ +/** + * 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. + */ + +/** + * A dashboard rendered as a composite visualization: a layout tree whose + * leaves are ordinary charts, drawn with the SAME renderers a single chart + * uses. + * + * Deliberately static. No native filters, no cross-filters, no shared + * selection — composition and the interaction graph are separate capabilities + * and only the first is prototyped here. + * + * Every cell renders something. A leaf with no data becomes a labelled + * placeholder rather than a gap: a partial composite that silently omits cells + * looks like a complete one, which is the failure mode most likely to make + * this read as broken. + */ +import { useMemo, useState, type JSX } from 'react'; +import type { DashboardCell, DashboardRender, ViewType } from '../types'; +import type { ThemeTokens } from '../theme'; +import { + chartDataToEChartsOption, + defaultViewForChartType, + isEChartsView, + isSubstitutedView, +} from '../adapter'; +import { EChart } from './EChart'; +import { BigNumber } from './BigNumber'; +import { DataTable } from './DataTable'; +import { stripUntrustedMarkers } from '../format'; + +function CellBody({ + cell, + theme, +}: { + cell: DashboardCell; + theme: ThemeTokens; +}): JSX.Element { + const data = cell.data ?? null; + + const view: ViewType | null = useMemo( + () => (data ? defaultViewForChartType(data.chart_type, data) : null), + [data], + ); + + if (!data || !view) { + return ( + <div className="sv-cell-placeholder"> + <span className="sv-cell-placeholder-icon" aria-hidden="true"> + {cell.status === 'error' ? '!' : '—'} + </span> + <span> + {stripUntrustedMarkers(cell.message ?? 'No data for this chart.')} + </span> + </div> + ); + } + + // Same dispatch as the single-chart view (App.tsx), so a cell renders + // exactly as it would on its own. + if (view === 'big_number') return <BigNumber data={data} theme={theme} />; + if (view === 'table' || !isEChartsView(view)) { + return <DataTable data={data} />; + } + return ( + <EChart + option={chartDataToEChartsOption(data, view, { theme })} + scheme={theme.scheme} + /> + ); +} + +/** + * Says so when a cell is NOT the chart Superset draws. + * + * Without this the cell carries the real title and the real numbers in a + * different encoding, which reads as a faithful reproduction. A treemap shown + * as a bar chart is a wrong answer that looks right, and nothing else on + * screen contradicts it. + */ +function SubstitutionNote({ cell }: { cell: DashboardCell }): JSX.Element | null { + const data = cell.data; + if (!data) return null; + const view = defaultViewForChartType(data.chart_type, data); + const native = isSubstitutedView(data.chart_type, view); + if (!native) return null; + return ( + <div className="sv-substitution"> + Shown as a {view} chart — Superset renders this as a {native}. + </div> + ); +} + +export function DashboardGrid({ + render, + theme, +}: { + render: DashboardRender; + theme: ThemeTokens; +}): JSX.Element { + // Tabs are presentational here: cells carry their tab id, so switching tabs + // filters what is shown without re-querying anything. + const tabs = render.tabs ?? []; + + // Counts only cells that actually have data. A tab full of `skipped` + // placeholders is empty as far as the user is concerned — opening on one + // shows a screen of "not queried" messages, which is the same symptom as + // opening on a tab with no cells at all. + const countFor = (id: string): number => + render.cells.filter((c) => c.tab_id === id && c.status === 'ok').length; + + /** Cells present at all, drawable or not — for "did we filter this out?" */ + const cellsIn = (id: string): number => + render.cells.filter((c) => c.tab_id === id).length; + + // Opening tab: the one that was explicitly requested, else the first tab + // that actually HAS cells. + // + // Selecting tabs[0] blindly opened a guaranteed-empty tab whenever the + // render was filtered to a different one — the full tab list is still + // returned (correctly, so the user can switch), so tabs[0] can hold nothing. + // "First non-empty" also covers an unfiltered dashboard whose first tab is + // genuinely empty. + const [activeTab, setActiveTab] = useState<string | null>(() => { + if (!tabs.length) return null; + const requested = render.active_tab_id; + if (requested && tabs.some((t) => t.id === requested)) return requested; + return (tabs.find((t) => countFor(t.id) > 0) ?? tabs[0]).id; + }); Review Comment: **Suggestion:** The selected tab is initialized only once, so when the same widget receives a subsequent dashboard result with a different `active_tab_id`, changed tab contents, or a different tab list, `activeTab` remains stale and can display the wrong tab or the “not included” placeholder. Synchronize the selection when `render` changes, while preserving user tab selections where appropriate. [stale reference] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Requery results can display the wrong dashboard tab. - ⚠️ Removed tabs can leave the widget showing an empty placeholder. - ⚠️ User-visible dashboard state diverges from the latest tool result. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e29cdd932a02439e93b07f69293acff3&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=e29cdd932a02439e93b07f69293acff3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/resources/chart_viewer/src/components/DashboardGrid.tsx **Line:** 140:145 **Comment:** *Stale Reference: The selected tab is initialized only once, so when the same widget receives a subsequent dashboard result with a different `active_tab_id`, changed tab contents, or a different tab list, `activeTab` remains stale and can display the wrong tab or the “not included” placeholder. Synchronize the selection when `render` changes, while preserving user tab selections where appropriate. 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%2F43483&comment_hash=b0088f2a6d730e8f21e9e815198e950130165e438790c4aafb70d4de1b025778&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=b0088f2a6d730e8f21e9e815198e950130165e438790c4aafb70d4de1b025778&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]
