codeant-ai-for-open-source[bot] commented on code in PR #38762:
URL: https://github.com/apache/superset/pull/38762#discussion_r2966071716
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -628,6 +628,7 @@ const PropertiesModal = ({
}}
title={t('Dashboard properties')}
isEditMode
+ resizable
Review Comment:
**Suggestion:** Enabling the modal `resizable` mode here unintentionally
disables the modal mask in the shared `Modal` component (`mask` is turned off
when `resizable` is true), which allows click-through interactions with the
dashboard while the properties dialog is open. That can cause users to mutate
dashboard state behind an active edit form and create inconsistent/accidental
edits. Keep this modal non-resizable (or only enable resizing after adding a
masked-resizable behavior in the base modal). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard tabs can switch behind properties modal.
- ⚠️ Background dashboard state mutates during property editing.
- ❌ Modal isolation breaks on a common edit flow.
```
</details>
```suggestion
resizable={false}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a dashboard page and trigger "Edit properties" from the header
actions menu;
`useHeaderActionsDropdownMenu.tsx:24-26` calls `showPropertiesModal()`, and
`Header/index.tsx:512-514` sets `showingPropertiesModal` true.
2. The modal renders `PropertiesModal` (`Header/index.tsx` JSX block in the
820-929
section), and this component passes `resizable` to `StandardModal` at
`dashboard/components/PropertiesModal/index.tsx:631`.
3. `StandardModal` forwards `resizable` into core `Modal`
(`src/components/Modal/StandardModal.tsx:135-136`), where `shouldShowMask =
!(resizable ||
draggable)` (`packages/superset-ui-core/src/components/Modal/Modal.tsx:280`)
and
`mask={shouldShowMask}` is applied (`Modal.tsx:360`), disabling backdrop
mask when
resizable is true.
4. While properties dialog is open, click a dashboard tab behind it; tab
clicks execute
`handleClickTab` in
`dashboard/components/gridComponents/Tabs/Tabs.tsx:230-249`
(`setSelectedTabIndex` / `setActiveKey`), proving background dashboard state
can mutate
behind an active edit modal.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
**Line:** 631:631
**Comment:**
*Logic Error: Enabling the modal `resizable` mode here unintentionally
disables the modal mask in the shared `Modal` component (`mask` is turned off
when `resizable` is true), which allows click-through interactions with the
dashboard while the properties dialog is open. That can cause users to mutate
dashboard state behind an active edit form and create inconsistent/accidental
edits. Keep this modal non-resizable (or only enable resizing after adding a
masked-resizable behavior in the base modal).
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38762&comment_hash=745c905b3ef5f6a9b632960f68f0743278e19d48ec3d0520d446304768c07b9e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38762&comment_hash=745c905b3ef5f6a9b632960f68f0743278e19d48ec3d0520d446304768c07b9e&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/util/getRelatedCharts.ts:
##########
@@ -55,31 +55,10 @@ function getRelatedChartsForCrossFilter(
slices: Record<string, Slice>,
scope: number[],
): number[] {
- const sourceSlice = slices[filterKey];
-
- if (!sourceSlice) return [];
-
- const fullScope = [
- ...scope.filter(s => String(s) !== filterKey),
- Number(filterKey),
- ];
- const scopeSet = new Set(scope);
-
- return Object.values(slices).reduce((result: number[], slice) => {
- if (slice.slice_id === Number(filterKey)) {
- return result;
- }
- // Check if it's in the global scope
- if (isGlobalScope(fullScope, slices)) {
- result.push(slice.slice_id);
- return result;
- }
- // Check if it's hand-picked in scope
- if (scopeSet.has(slice.slice_id)) {
- result.push(slice.slice_id);
- }
- return result;
- }, []);
+ // Always return all charts except source chart
+ return Object.values(slices)
+ .map(slice => slice.slice_id)
+ .filter(id => id !== Number(filterKey));
Review Comment:
**Suggestion:** The new cross-filter logic ignores the `scope` argument and
always returns every chart except the source chart. This breaks existing scoped
cross-filter behavior (including chart-level custom scoping) and will trigger
refreshes for charts that should remain out of scope. Filter the result by
`scope` first, then exclude the source chart. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard.applyFilters refreshes out-of-scope charts unnecessarily.
- ⚠️ Cross-filter scoping modal settings are functionally ignored.
- ⚠️ Focus highlighting can include unrelated charts.
```
</details>
```suggestion
const chartsInScopeSet = new Set(scope);
return Object.values(slices)
.map(slice => slice.slice_id)
.filter(id => chartsInScopeSet.has(id) && id !== Number(filterKey));
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open cross-filter scoping UI and set a custom scope for one chart;
`ScopingModal.handleScopeUpdate()` computes scoped `chartsInScope` via
`getChartIdsInFilterScope` in
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/ScopingModal/ScopingModal.tsx:151-166`.
2. Save scoping; it is persisted through `saveChartConfiguration()` in
`superset-frontend/src/dashboard/actions/dashboardInfo.ts:57-94`, then
consumed as
`chart_configuration` in Redux metadata.
3. Trigger a cross-filter from that chart; active filters are built in
`getAllActiveFilters()` using
`chartConfiguration[filterId].crossFilters.chartsInScope`
(`superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts:115-123`)
and passed
into `Dashboard.applyFilters()`
(`superset-frontend/src/dashboard/components/Dashboard.tsx:288-303`).
4. `getRelatedChartsForCrossFilter()` ignores `scope` and returns all charts
except source
in `superset-frontend/src/dashboard/util/getRelatedCharts.ts:53-61`, so
`triggerQuery()`
refreshes out-of-scope charts in `Dashboard.refreshCharts()`
(`Dashboard.tsx:100-103`).
```
</details>
<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/getRelatedCharts.ts
**Line:** 58:61
**Comment:**
*Logic Error: The new cross-filter logic ignores the `scope` argument
and always returns every chart except the source chart. This breaks existing
scoped cross-filter behavior (including chart-level custom scoping) and will
trigger refreshes for charts that should remain out of scope. Filter the result
by `scope` first, then exclude the source chart.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38762&comment_hash=57eec24d8a51f19d42a8c311ad7e1802cb2fd70f2a1672ebcc7f0bf8e7031877&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38762&comment_hash=57eec24d8a51f19d42a8c311ad7e1802cb2fd70f2a1672ebcc7f0bf8e7031877&reaction=dislike'>👎</a>
##########
superset-frontend/src/dashboard/util/crossFilters.ts:
##########
@@ -98,17 +98,10 @@ export const getCrossFiltersConfiguration = (
},
};
}
- chartConfiguration[chartId].crossFilters.chartsInScope =
-
isCrossFilterScopeGlobal(chartConfiguration[chartId].crossFilters.scope)
- ? globalChartConfiguration.chartsInScope.filter(
- id => id !== Number(chartId),
- )
- : getChartIdsInFilterScope(
- chartConfiguration[chartId].crossFilters
- .scope as NativeFilterScope,
- Object.values(charts).map(chart => chart.id),
- chartLayoutItems,
- );
+ chartConfiguration[chartId].crossFilters.chartsInScope =
+ Object.values(charts)
+ .map(chart => chart.id)
+ .filter(id => id !== Number(chartId));
Review Comment:
**Suggestion:** The new assignment ignores each chart's configured scope and
always targets every chart except itself. This breaks saved custom scoping
(including exclusions/layer selections) and also ignores global scope
exclusions for globally-scoped charts. Recompute `chartsInScope` from the
effective scope (`global` pointer resolved to global scope, otherwise per-chart
scope) via `getChartIdsInFilterScope`, then remove the source chart id. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Saved custom scope exclusions ignored after dashboard reload.
- ❌ Excluded charts still receive cross-filter query constraints.
- ⚠️ Global-scope exclusions for global charts are bypassed.
```
</details>
```suggestion
const scope = chartConfiguration[chartId].crossFilters.scope;
const effectiveScope: NativeFilterScope =
isCrossFilterScopeGlobal(scope)
? globalChartConfiguration.scope
: scope;
chartConfiguration[chartId].crossFilters.chartsInScope =
getChartIdsInFilterScope(
effectiveScope,
Object.values(charts).map(chart => chart.id),
chartLayoutItems,
).filter(id => id !== Number(chartId));
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a custom cross-filter scope in the UI and save it:
`ScopingModal.tsx:151-167`
computes scoped `chartsInScope` via `getChartIdsInFilterScope`, and
`ScopingModal.tsx:143-146` persists it using `saveChartConfiguration`.
2. Reload the dashboard: hydration calls `getCrossFiltersConfiguration` from
`hydrate.ts:314-320`; then `crossFilters.ts:101-104` overwrites every
interactive chart's
`crossFilters.chartsInScope` to all charts except itself, ignoring saved
per-chart scope
and global exclusions.
3. Apply a cross-filter from the source chart: active filters are rebuilt in
`DashboardPage.tsx:95-110`, and `activeAllDashboardFilters.ts:119-121` reads
the
overwritten `chart_configuration[filterId].crossFilters.chartsInScope`.
4. Observe excluded charts still filtered/requeried:
`getFormDataWithExtraFilters.ts:53-56` applies filters to any chart id in
that scope, so
charts meant to be excluded now receive cross-filter `extra_form_data`.
```
</details>
<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/crossFilters.ts
**Line:** 101:104
**Comment:**
*Logic Error: The new assignment ignores each chart's configured scope
and always targets every chart except itself. This breaks saved custom scoping
(including exclusions/layer selections) and also ignores global scope
exclusions for globally-scoped charts. Recompute `chartsInScope` from the
effective scope (`global` pointer resolved to global scope, otherwise per-chart
scope) via `getChartIdsInFilterScope`, then remove the source chart id.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38762&comment_hash=3d11275afdacf74609af66c7866ac3cfb548a7b73a36b78d942049f39793fcb3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38762&comment_hash=3d11275afdacf74609af66c7866ac3cfb548a7b73a36b78d942049f39793fcb3&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]
