codeant-ai-for-open-source[bot] commented on code in PR #42502:
URL: https://github.com/apache/superset/pull/42502#discussion_r3662926584
##########
superset-frontend/src/dashboard/util/getChartIdsInFilterScope.ts:
##########
@@ -24,65 +24,86 @@ interface ExtendedNativeFilterScope extends
NativeFilterScope {
selectedLayers?: string[];
}
+export type ChartLayoutItemMap = ReadonlyMap<number, LayoutItem>;
+export type ChartLayoutItems = readonly LayoutItem[] | ChartLayoutItemMap;
+
+const isChartLayoutItemArray = (
+ chartLayoutItems: ChartLayoutItems,
+): chartLayoutItems is readonly LayoutItem[] =>
Array.isArray(chartLayoutItems);
+
+export function createChartLayoutItemMap(
+ layoutItems: readonly (LayoutItem | null | undefined)[],
+): ChartLayoutItemMap {
+ const chartLayoutItemMap = new Map<number, LayoutItem>();
+
+ layoutItems.forEach(layoutItem => {
+ const chartId = layoutItem?.meta?.chartId;
+ if (
+ layoutItem?.type === CHART_TYPE &&
+ typeof chartId === 'number' &&
+ !chartLayoutItemMap.has(chartId)
+ ) {
+ chartLayoutItemMap.set(chartId, layoutItem);
Review Comment:
**Suggestion:** The index discards every layout holder after the first one
for a chart ID. Dashboards can contain duplicate chart holders with the same
`meta.chartId`; when those holders are located under different tabs or scope
roots, scope calculation and tab discovery consult only the first holder and
omit valid locations. Preserve all layout items per chart ID, or aggregate
their parent paths when building the index. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Tab-scoped native filters can omit duplicated chart placements.
- ❌ Cross-filter scope calculations can exclude valid chart locations.
- ⚠️ Chart customization tab highlighting can miss affected tabs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load a dashboard whose position data contains two CHART layout holders
with different
IDs but the same meta.chartId, placing the holders under different tabs or
scope roots;
the dashboard layout is supplied as Object.values(dashboardLayout) at
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx:199.
2. DashboardContainer passes the resulting map into calculateScopes at
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx:203-210,
and calculateScopes reuses it for every scope at
superset-frontend/src/dashboard/util/calculateScopes.ts:44-70.
3. During map creation, createChartLayoutItemMap at
superset-frontend/src/dashboard/util/getChartIdsInFilterScope.ts:39-46
inserts only the
first holder for the chart ID because of !chartLayoutItemMap.has(chartId);
the second
holder's parents are discarded.
4. For a scope rooted at the second holder's tab, getChartIdsInFilterScope at
superset-frontend/src/dashboard/util/getChartIdsInFilterScope.ts:70-74
checks only the
first holder's parents and omits the chart. If the chart is otherwise
included,
findTabsWithChartsInScope at
superset-frontend/src/dashboard/components/nativeFilters/utils.ts:183-187
also reports
only tabs from the retained holder, so native-filter, chart-customization,
and
cross-filter tab scope calculations miss the valid placement.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7a0ba3f340034056ac3dc3a97a69bd9a&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=7a0ba3f340034056ac3dc3a97a69bd9a&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/dashboard/util/getChartIdsInFilterScope.ts
**Line:** 41:46
**Comment:**
*Logic Error: The index discards every layout holder after the first
one for a chart ID. Dashboards can contain duplicate chart holders with the
same `meta.chartId`; when those holders are located under different tabs or
scope roots, scope calculation and tab discovery consult only the first holder
and omit valid locations. Preserve all layout items per chart ID, or aggregate
their parent paths when building the index.
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%2F42502&comment_hash=5dace13f3baf674283887c859496abd56ab61916dfd7e20e173bafca0060363b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42502&comment_hash=5dace13f3baf674283887c859496abd56ab61916dfd7e20e173bafca0060363b&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]
