Skip to content

fix(native-filter): infinite filter loading by deps #39175

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
michael-s-molina merged 3 commits into from
Apr 14, 2026
Merged

fix(native-filter): infinite filter loading by deps #39175

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b545bd
fix(native-filter): infinite filter loading by deps
justinpark Apr 8, 2026
9f7f5ca
setHasDepsFilterValue
justinpark Apr 8, 2026
e03f0a2
address issue
justinpark Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions ...-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ import { useDispatch, useSelector } from 'react-redux';
import { isEqual, isEqualWith } from 'lodash';
import { getChartDataRequest } from 'src/components/Chart/chartAction';
import { ErrorAlert, ErrorMessageWithStackTrace } from 'src/components';
import { Loading, Constants } from '@superset-ui/core/components';
import { Loading, Constants, Flex } from '@superset-ui/core/components';
import { waitForAsyncData } from 'src/middleware/asyncEvent';
import { FilterBarOrientation, RootState } from 'src/dashboard/types';
import {
Expand Down Expand Up @@ -128,6 +128,8 @@ const FilterValue: FC<FilterValueProps> = ({
[isCustomization],
);
const [state, setState] = useState<ChartDataResponseResult[]>([]);
const hasDeps = Boolean(filter.cascadeParentIds?.length);
const [hasDepsFilterValue, setHasDepsFilterValue] = useState(hasDeps);
const dashboardId = useSelector<RootState, number>(
state => state.dashboardInfo.id,
);
Expand Down Expand Up @@ -160,6 +162,10 @@ const FilterValue: FC<FilterValueProps> = ({
}
}, [dispatch, shouldRefresh]);

useEffect(() => {
setHasDepsFilterValue(hasDeps);
}, [hasDeps]);

useEffect(() => {
if (!inViewFirstTime && inView) {
setInViewFirstTime(true);
Expand All @@ -185,28 +191,31 @@ const FilterValue: FC<FilterValueProps> = ({
// Prevent unnecessary backend requests by validating parent filter selections first

let selectedParentFilterValueCounts = 0;

let isTimeRangeSelected = false;
(filter.cascadeParentIds ?? []).forEach(pId => {
const extraFormData = dataMaskSelected?.[pId]?.extraFormData;
if (extraFormData?.filters?.length) {
selectedParentFilterValueCounts += extraFormData.filters.length;
} else if (extraFormData?.time_range) {
selectedParentFilterValueCounts += 1;
}
if (extraFormData?.time_range) {
isTimeRangeSelected = true;
}
Comment on lines 197 to 202

@bito-code-review bito-code-review Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic Bug: Incomplete Parent Selection Check

The 'else if' condition here prevents checking for time_range when filters are present on a parent filter. If a parent has both filters and a time_range selected, isTimeRangeSelected won't be set, potentially causing incorrect dependency satisfaction logic.

Code suggestion
Check the AI-generated fix before applying
Suggested change
if (extraFormData?.filters?.length) {
selectedParentFilterValueCounts += extraFormData.filters.length;
} else if (extraFormData?.time_range) {
selectedParentFilterValueCounts += 1;
isTimeRangeSelected = true;
}
if (extraFormData?.filters?.length) {
selectedParentFilterValueCounts += extraFormData.filters.length;
}
if (extraFormData?.time_range) {
isTimeRangeSelected = true;
}

Code Review Run #58bc36

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

});

// check if all parent filters with defaults have a value selected

let depsCount = dependencies.filters?.length ?? 0;
const depsCount = dependencies.filters?.length ?? 0;
const hasTimeRangeDeps = Boolean(dependencies?.time_range);

if (dependencies?.time_range) {
depsCount += 1;
}
if (selectedParentFilterValueCounts !== depsCount) {
if (
selectedParentFilterValueCounts !== depsCount ||
(hasTimeRangeDeps && !isTimeRangeSelected)
) {
// child filter should not request backend until it
// has all the required information from parent filters
return;

@codeant-ai-for-open-source codeant-ai-for-open-source Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The dependency-status flag is only set to false when dependencies are satisfied and never set back to true when dependencies become invalid again. This causes stale UI state and can show "Loading filter values" while the filter is actually blocked waiting on parent selections. Set the flag to true before returning from the unmet-dependencies branch. [logic error]

Severity Level: Minor 🧹 
- ⚠️ Native filter bar shows misleading loading text for cascades.
- ⚠️ Dashboard child filters appear loading instead of awaiting parents.
Suggested change
return;
setHasDepsFilterValue(true);
Steps of Reproduction ✅ 
1. Open any dashboard that uses native filters with cascade parents, so that each filter
is rendered through `FilterControl` which mounts `FilterValue` (see
`superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx:13-27`
where `<FilterValue>` is instantiated).

2. Configure a parent time range filter and a child filter with `cascadeParentIds` so that
the child's `FilterValue` effect executes the dependency validation and data-fetch logic
(see `FilterValue`'s `useEffect` around `getFormData` and `getChartDataRequest` in
`FilterValue.tsx:170-238`).

3. With the PR's `hasDepsFilterValue` state in place (initialized from
`filter.cascadeParentIds` and set to `false` only after the dependency check, as in the
provided `existing_code` block at diff lines 206–214), select a valid value in the parent
so the child starts a backend request via `getChartDataRequest`, leaving `isLoading` true
while `hasDepsFilterValue` is set to `false`.

4. Before the backend response returns, clear or otherwise invalidate the parent selection
so the child's dependency check (`if (selectedParentFilterValueCounts !== depsCount ||
(hasTimeRangeDeps && !isTimeRangeSelected))` in `FilterValue.tsx`, see around
`grep`-reported line 205) returns early without resetting `hasDepsFilterValue` to `true`;
while `isLoading` is still true, the render branch at `FilterValue.tsx:401-407` shows
`<Loading>` with the message `t('Loading filter values')` instead of `t('Awaiting filter
selection')`, even though the child is effectively blocked by unmet parent filter
dependencies.
Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
**Line:** 212:212
**Comment:**
	*Logic Error: The dependency-status flag is only set to `false` when dependencies are satisfied and never set back to `true` when dependencies become invalid again. This causes stale UI state and can show "Loading filter values" while the filter is actually blocked waiting on parent selections. Set the flag to `true` before returning from the unmet-dependencies branch.

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.
👍 | 👎

Comment on lines 214 to 216

@bito-code-review bito-code-review Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

State Bug: Missing State Reset

The hasDepsFilterValue state is only set to false when dependencies are satisfied, but not reset to true when they become unsatisfied. This could cause the UI to show 'Loading filter values' instead of 'Awaiting filter selection' after parent selections change.

Code suggestion
Check the AI-generated fix before applying
Suggested change
// child filter should not request backend until it
// has all the required information from parent filters
return;
// child filter should not request backend until it
// has all the required information from parent filters
setHasDepsFilterValue(true);
return;

Code Review Run #58bc36

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

}
setHasDepsFilterValue(false);
}

// TODO: We should try to improve our useEffect hooks to depend more on
Expand Down Expand Up @@ -281,6 +290,7 @@ const FilterValue: FC<FilterValueProps> = ({
isRefreshing,
shouldRefresh,
dataMaskSelected,
setHasDepsFilterValue,
]);

useEffect(() => {
Expand Down Expand Up @@ -397,7 +407,12 @@ const FilterValue: FC<FilterValueProps> = ({
overflow={overflow}
>
{isLoading ? (
<Loading position="inline-centered" size="s" muted />
<Flex align="center">
<Loading position="inline" size="s" muted />
{hasDepsFilterValue
? t('Awaiting filter selection')
: t('Loading filter values')}
</Flex>
) : (
<SuperChart
height={HEIGHT}
Expand Down
Loading