Repository navigation
feat(native-filters): drive cascade dependencies from chart metadata - #43223
Conversation
dosubot
Bot
added
change:frontend
Code Review Agent Run #19c4aeActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| component && | ||
| 'filterType' in component && | ||
| ALLOW_DEPENDENCIES.includes(component.filterType) | ||
| filterSupportsDependencies(component.filterType) |
There was a problem hiding this comment.
Suggestion: Changing a filter to a type whose metadata sets supportsCascadeDependencies to false only prevents it from appearing in newly computed available-parent options; existing child dependencies are still retained by the form and consumed by buildDependencyMap and the dependency preview until the modal is saved. This leaves an unsupported parent actively affecting other filters while the modal remains open, and cancelling the modal can preserve the stale in-memory relationship. Remove or filter existing dependency edges as soon as the parent capability changes, rather than only relying on save-time cleanup. [stale reference]
Severity Level: Minor 🧹
- ⚠️ Filter previews can temporarily use unsupported cascade parents.
- ⚠️ Unmounted child forms retain stale dependency IDs until save.
- ⚠️ Save-time cleanup prevents persisted invalid relationships.
- ⚠️ Cancel resets form state, so cancellation does not preserve them.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.ts
**Line:** 173:173
**Comment:**
*Stale Reference: Changing a filter to a type whose metadata sets `supportsCascadeDependencies` to `false` only prevents it from appearing in newly computed available-parent options; existing child dependencies are still retained by the form and consumed by `buildDependencyMap` and the dependency preview until the modal is saved. This leaves an unsupported parent actively affecting other filters while the modal remains open, and cancelling the modal can preserve the stale in-memory relationship. Remove or filter existing dependency edges as soon as the parent capability changes, rather than only relying on save-time cleanup.
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
|
The reviewer's suggestion is valid. The current implementation only prevents unsupported filters from appearing in the available parent options, but it does not actively remove existing dependency edges if a filter's capability changes while the modal is open. To resolve this, you should update the filter operations to explicitly prune dependencies when a filter's I have analyzed the PR and confirmed that the issue is correct. Would you like me to fetch all other comments on this PR to validate them and implement a minimal fix for all of them? |
| const canDependOnOtherFilters = filterSupportsDependencies( | ||
| formFilter?.filterType, | ||
| ); |
There was a problem hiding this comment.
Suggestion: The dependency gate uses formFilter?.filterType directly, unlike the rest of this form which uses itemTypeField to fall back to filterToEdit?.filterType or the default filter_select while form values are not initialized. On the initial render after opening an existing filter or creating a new one, formFilter?.filterType can be undefined, so the dependency editor is hidden even though the active filter type supports cascade dependencies. Pass itemTypeField to keep this gate consistent with the effective filter type used by the surrounding rendering logic. [incorrect variable usage]
Severity Level: Major ⚠️
- ❌ Cascade dependency editor can disappear initially.
- ⚠️ Existing filters may require a form change before appearing.
- ⚠️ New select filters initially omit dependency configuration.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
**Line:** 475:477
**Comment:**
*Incorrect Variable Usage: The dependency gate uses `formFilter?.filterType` directly, unlike the rest of this form which uses `itemTypeField` to fall back to `filterToEdit?.filterType` or the default `filter_select` while form values are not initialized. On the initial render after opening an existing filter or creating a new one, `formFilter?.filterType` can be undefined, so the dependency editor is hidden even though the active filter type supports cascade dependencies. Pass `itemTypeField` to keep this gate consistent with the effective filter type used by the surrounding rendering logic.
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43223 +/- ##
==========================================
- Coverage 66.63% 66.63% -0.01%
==========================================
Files 2874 2874
Lines 163887 163894 +7
Branches 37816 37819 +3
==========================================
+ Hits 109209 109210 +1
- Misses 52535 52541 +6
Partials 2143 2143
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3a344e6 to
52ddb80
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Replace the hardcoded ALLOW_DEPENDENCIES list with ChartMetadata.supportsCascadeDependencies so plugin filters can opt into dashboard cascade without a core allowlist change. Core select, range, and time filters keep the same parent and child gate; time grain and time column stay opted out. Co-authored-by: ashah65 <137852504+ashah65@users.noreply.github.com>
52ddb80 to
5d42976
Compare
Code Review Agent Run #4f98f6Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…ilter type Follow-up to #43223. Two bot review comments on that PR were never addressed before merge: - FiltersConfigForm.tsx: canDependOnOtherFilters read formFilter?.filterType directly, unlike itemTypeField's own fallback chain (formFilter?.filterType || filterToEdit?.filterType || 'filter_select') used everywhere else in this component. formFilter?.filterType can be undefined on the first render before the antd Form hydrates, which hid the "Values are dependent on other filters" section even for filter types that do support cascading. - useFilterOperations.ts: buildDependencyMap read each filter's dependencies array as-is, without re-checking that a listed parent still supports cascading. If a parent's type changes to one that no longer supports dependencies within the same open-modal editing session, the stale relationship lingered in the live dependency map and preview until save. Now filters each parent id through the existing canBeUsedAsDependency check on every rebuild. Added regression tests for the buildDependencyMap fix (confirmed they fail against the pre-fix code). Did not add a FiltersConfigForm.tsx-level test for the itemTypeField fix - no existing test harness covers this component at that level (matches #43223's own testing approach, which relied on manual QA there and unit tests only for the hooks). Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
SUMMARY
This is the registry-only slice @rusackas asked for on #40905. The original implementation is @ashah65's — I am landing just the part that was already called clean, as a new PR rather than pushing over that branch.
Dashboard cascade used to consult a hardcoded
ALLOW_DEPENDENCIESlist (filter_select/filter_range/filter_time). Third-party native filters could not join cascade without a core change. This replaces that list withChartMetadata.supportsCascadeDependencies:true. Parent picker and child "Values are dependent on other filters" stay as they are today.false. They stay out of both gates — I did not take the feat(native-filters): make filter dependency support extensible via plugin registry #40905 child-side widening that would have shown the dependency section on those types.Behavior.NativeFilterso existing third-party filters keep working.isColumnSelect, the divider change-tracking rewrite, and the ControlLabel work from #40905 are not in this PR. Those can come later if still wanted.I do not think this needs a SIP. It is the same cascade feature, driven from plugin metadata instead of a list, and rusackas already said the core swap is clean.
Related: #40905, discussion #26084.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable. Core filter UI is unchanged. Time column / time grain still do not offer the child dependency control.
TESTING INSTRUCTIONS
On a dashboard:
ADDITIONAL INFORMATION