codeant-ai-for-open-source[bot] commented on code in PR #37012:
URL: https://github.com/apache/superset/pull/37012#discussion_r2676247302
##########
superset-frontend/src/dashboard/reducers/nativeFilters.test.ts:
##########
@@ -160,3 +160,28 @@ test('SET_NATIVE_FILTERS_CONFIG_COMPLETE treats empty
arrays as valid scope prop
expect(result.filters.filter1.chartsInScope).toEqual([]);
expect(result.filters.filter1.tabsInScope).toEqual([]);
});
+
+test('SET_NATIVE_FILTERS_CONFIG_COMPLETE removes filters that are not in the
response', () => {
+ const initialState = {
+ filters: {
+ filter1: createMockFilter('filter1', [1, 2], ['tab1']),
+ filter2: createMockFilter('filter2', [3, 4], ['tab2']),
+ filter3: createMockFilter('filter3', [5, 6], ['tab3']),
+ },
+ };
+
+ // Only filter1 and filter3 are in the response (filter2 was deleted)
+ const action = {
+ type: SET_NATIVE_FILTERS_CONFIG_COMPLETE as typeof
SET_NATIVE_FILTERS_CONFIG_COMPLETE,
+ filterChanges: [
+ createMockFilter('filter1', [1, 2], ['tab1']),
+ createMockFilter('filter3', [5, 6], ['tab3']),
+ ],
+ };
+
+ const result = nativeFilterReducer(initialState, action);
+
+ expect(result.filters.filter1).toBeDefined();
+ expect(result.filters.filter2).toBeUndefined();
Review Comment:
**Suggestion:** The test uses
`expect(result.filters.filter2).toBeUndefined()` to assert that a filter was
removed, but this passes even if the `filter2` property still exists on the
object with the explicit value `undefined` (which would not indicate an actual
deletion). Replace this assertion with a check that the property does not exist
on the `filters` object (e.g.,
`expect(result.filters).not.toHaveProperty('filter2')`) so the test fails if
the reducer left a key present but set to `undefined`. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
expect(result.filters).not.toHaveProperty('filter2');
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current assertion passes whether the key was deleted or left present
with an undefined value.
Using not.toHaveProperty('filter2') makes the test stricter and correctly
verifies the reducer removed the key.
</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/reducers/nativeFilters.test.ts
**Line:** 185:185
**Comment:**
*Logic Error: The test uses
`expect(result.filters.filter2).toBeUndefined()` to assert that a filter was
removed, but this passes even if the `filter2` property still exists on the
object with the explicit value `undefined` (which would not indicate an actual
deletion). Replace this assertion with a check that the property does not exist
on the `filters` object (e.g.,
`expect(result.filters).not.toHaveProperty('filter2')`) so the test fails if
the reducer left a key present but set to `undefined`.
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>
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx:
##########
@@ -254,6 +254,14 @@ const FilterControls: FC<FilterControlsProps> = ({
/>
)}
+ {!showCollapsePanel && filtersOutOfScope.length > 0 && (
Review Comment:
**Suggestion:** Logic/UX inconsistency: when `showCollapsePanel` is false
the new branch renders the out-of-scope collapsible regardless of whether the
filters section is visible/collapsed (`hideHeader` / `sectionsOpen.filters`).
Previously, when `showCollapsePanel` was true the component respected
`(hideHeader || sectionsOpen.filters)`. Restore the same visibility condition
so the out-of-scope panel doesn't appear while the user has collapsed the
filters section. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
{!showCollapsePanel && (hideHeader || sectionsOpen.filters) &&
filtersOutOfScope.length > 0 && (
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Correct and actionable. The earlier branch (when showCollapsePanel is true)
included the visibility guard (hideHeader || sectionsOpen.filters); the newly
added branch omits it, causing the out-of-scope panel to render even when the
filters section is collapsed — a clear UX inconsistency. Adding (hideHeader ||
sectionsOpen.filters) to this condition restores consistent behavior without
further changes.
</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/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
**Line:** 257:257
**Comment:**
*Logic Error: Logic/UX inconsistency: when `showCollapsePanel` is false
the new branch renders the out-of-scope collapsible regardless of whether the
filters section is visible/collapsed (`hideHeader` / `sectionsOpen.filters`).
Previously, when `showCollapsePanel` was true the component respected
`(hideHeader || sectionsOpen.filters)`. Restore the same visibility condition
so the out-of-scope panel doesn't appear while the user has collapsed the
filters section.
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>
##########
superset-frontend/src/dashboard/reducers/nativeFilters.test.ts:
##########
@@ -160,3 +160,28 @@ test('SET_NATIVE_FILTERS_CONFIG_COMPLETE treats empty
arrays as valid scope prop
expect(result.filters.filter1.chartsInScope).toEqual([]);
expect(result.filters.filter1.tabsInScope).toEqual([]);
});
+
+test('SET_NATIVE_FILTERS_CONFIG_COMPLETE removes filters that are not in the
response', () => {
+ const initialState = {
+ filters: {
+ filter1: createMockFilter('filter1', [1, 2], ['tab1']),
+ filter2: createMockFilter('filter2', [3, 4], ['tab2']),
+ filter3: createMockFilter('filter3', [5, 6], ['tab3']),
+ },
+ };
+
+ // Only filter1 and filter3 are in the response (filter2 was deleted)
+ const action = {
+ type: SET_NATIVE_FILTERS_CONFIG_COMPLETE as typeof
SET_NATIVE_FILTERS_CONFIG_COMPLETE,
+ filterChanges: [
+ createMockFilter('filter1', [1, 2], ['tab1']),
+ createMockFilter('filter3', [5, 6], ['tab3']),
+ ],
+ };
+
+ const result = nativeFilterReducer(initialState, action);
+
Review Comment:
**Suggestion:** The test doesn't check that the reducer is not mutating the
original `initialState`; a reducer that mutates its input can still make this
test pass. After calling the reducer, assert that
`initialState.filters.filter2` is still defined to catch accidental in-place
mutation. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
expect(initialState.filters.filter2).toBeDefined();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Verifying initialState.filters.filter2 is still defined after the reducer
call catches accidental in-place mutation of the input state, which reducers
must avoid.
</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/reducers/nativeFilters.test.ts
**Line:** 183:183
**Comment:**
*Possible Bug: The test doesn't check that the reducer is not mutating
the original `initialState`; a reducer that mutates its input can still make
this test pass. After calling the reducer, assert that
`initialState.filters.filter2` is still defined to catch accidental in-place
mutation.
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>
##########
superset-frontend/src/dashboard/components/nativeFilters/state.ts:
##########
@@ -144,20 +144,38 @@ export function useSelectFiltersInScope(filters: (Filter
| Divider)[]) {
let filtersInScope: (Filter | Divider)[] = [];
const filtersOutOfScope: (Filter | Divider)[] = [];
- // we check native filters scopes only on dashboards with tabs
- if (!dashboardHasTabs) {
- filtersInScope = filters;
- } else {
- filters.forEach(filter => {
+ filters.forEach(filter => {
+ // Dividers are always in scope
+ if (isFilterDivider(filter)) {
+ filtersInScope.push(filter);
+ return;
+ }
+
+ // If dashboard has tabs, check scope based on visibility
+ if (dashboardHasTabs) {
const filterInScope = isFilterInScope(filter);
-
if (filterInScope) {
filtersInScope.push(filter);
} else {
filtersOutOfScope.push(filter);
}
- });
- }
+ } else {
+ // If no tabs, check if filter has any charts in scope
+ // Only mark as out of scope if chartsInScope is explicitly defined
AND empty
+ const hasExplicitChartsInScope = Array.isArray(filter.chartsInScope);
+ const hasChartsInScope =
+ hasExplicitChartsInScope && filter.chartsInScope!.length > 0;
Review Comment:
**Suggestion:** Unsafe non-null assertion: the code uses
`filter.chartsInScope!.length` after checking
`Array.isArray(filter.chartsInScope)`, but using a non-null assertion is
unnecessary and brittle; replace with a safe length check that does not rely on
`!` (use nullish coalescing) to avoid runtime errors if `chartsInScope` were
unexpectedly null-ish. [type error]
**Severity Level:** Minor ⚠️
```suggestion
hasExplicitChartsInScope && (filter.chartsInScope ?? []).length >
0;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The non-null assertion (`!`) is unnecessary and slightly brittle; using a
nullish coalescing fallback like `(filter.chartsInScope ?? []).length` is safer
and clearer. This is a small, safe local improvement that doesn't change
behavior but avoids relying on the assertion.
</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/nativeFilters/state.ts
**Line:** 167:167
**Comment:**
*Type Error: Unsafe non-null assertion: the code uses
`filter.chartsInScope!.length` after checking
`Array.isArray(filter.chartsInScope)`, but using a non-null assertion is
unnecessary and brittle; replace with a safe length check that does not rely on
`!` (use nullish coalescing) to avoid runtime errors if `chartsInScope` were
unexpectedly null-ish.
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>
##########
superset-frontend/src/dashboard/reducers/nativeFilters.ts:
##########
@@ -67,12 +67,13 @@ function handleFilterChangesComplete(
state: ExtendedNativeFiltersState,
filters: Filter[],
) {
- const modifiedFilters = { ...state.filters };
+ const modifiedFilters: Record<string, Filter | Divider> = {};
+ // Build fresh filter list from server response to ensure deleted filters
are removed
filters.forEach(filter => {
if (filter.chartsInScope != null && filter.tabsInScope != null) {
modifiedFilters[filter.id] = filter;
} else {
- const existingFilter = modifiedFilters[filter.id];
+ const existingFilter = state.filters[filter.id];
Review Comment:
**Suggestion:** Possible runtime exception when `state.filters` is
undefined: the code accesses `state.filters[filter.id]` directly which will
throw if `state.filters` is null/undefined (for example after a HYDRATE where
`nativeFilters.filters` might be undefined). Use a safe access with a default
empty object so the lookup won't throw. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
const existingFilter = (state.filters ?? {})[filter.id];
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code will throw if `state.filters` is undefined (e.g. after a
HYDRATE action that sets filters to undefined). Using a safe fallback when
reading avoids a potential TypeError at runtime. The proposed change is small,
low-risk, and directly fixes a verifiable issue.
</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/reducers/nativeFilters.ts
**Line:** 76:76
**Comment:**
*Null Pointer: Possible runtime exception when `state.filters` is
undefined: the code accesses `state.filters[filter.id]` directly which will
throw if `state.filters` is null/undefined (for example after a HYDRATE where
`nativeFilters.filters` might be undefined). Use a safe access with a default
empty object so the lookup won't throw.
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>
--
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]
