Skip to content

feat(native-filters): make filter dependency support extensible via plugin registry#40905

Open
ashah65 wants to merge 14 commits into
apache:masterfrom
ashah65:feat/extensible-filter-dependency-registry
Open

feat(native-filters): make filter dependency support extensible via plugin registry#40905
ashah65 wants to merge 14 commits into
apache:masterfrom
ashah65:feat/extensible-filter-dependency-registry

Conversation

@ashah65

@ashah65 ashah65 commented Jun 9, 2026

Copy link
Copy Markdown

Replace hardcoded ALLOW_DEPENDENCIES list with a registry-based check so any filter plugin registering with Behavior.NativeFilter automatically supports the cascade dependency feature — no core changes needed for new plugins.

Also adds isColumnSelect support in getControlItemsMap to allow plugins to declare dataset column picker controls.

SUMMARY

Currently, the mechanism that determines which native filters support cascading dependencies relies on a hardcoded list (ALLOW_DEPENDENCIES) within the core Superset codebase. This creates a tight coupling that blocks external, third-party, or custom filter plugins from leveraging cascading features without directly mutating core files.

This PR refactors the filter dependency system to be entirely registry-driven and enhances control mapping flexibility.

Related Discussion: #26084

Design Decisions & Changes:

Registry-Driven Dependencies: Replaced the hardcoded check with a dynamic lookup. Any filter plugin that registers with Behavior.NativeFilter now automatically inherits support for the cascade dependency feature out-of-the-box. This eliminates the need for future core repository changes when new filters are introduced.

Dynamic Column Selection Controls: Added explicit support for isColumnSelect inside getControlItemsMap. This allows external plugins to declaratively request dataset column picker controls directly from the native filter configuration modal.

TESTING INSTRUCTIONS

Since this is an architectural enablement patch, testing should focus on verifying that existing core filters suffer no regressions, and that the underlying hooks resolve correctly.

Navigate to a dashboard and open the Manage native filters modal.

Create a standard core filter (e.g., a Select or Numerical Range filter).

Ensure that the "Filter is dependent on..." (cascading) configuration option is visible, selectable, and functional.

Verify that dataset column configuration selectors still populate and save accurately for out-of-the-box filters.

Register a custom dummy plugin leveraging Behavior.NativeFilter to verify it successfully reveals dependency options without updating core mapping arrays.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

…lugin registry

Replace hardcoded ALLOW_DEPENDENCIES list with a registry-based check so
any filter plugin registering with Behavior.NativeFilter automatically
supports the cascade dependency feature — no core changes needed for
new plugins.

Also adds isColumnSelect support in getControlItemsMap to allow plugins
to declare dataset column picker controls.
@dosubot dosubot Bot added the dashboard:native-filters Related to the native filters of the Dashboard label Jun 9, 2026
@bito-code-review

Copy link
Copy Markdown
Contributor

The flagged issue is correct. When divider edits are made, the handleValuesChange function in FiltersConfigModal does not trigger the necessary state updates to mark the divider as modified, which prevents the canSave flag from enabling the Save button.

To resolve this, you should ensure that divider-specific changes are tracked. You can update handleValuesChange to inspect the changed values and call handleModifyItem when the changed ID corresponds to a divider. Alternatively, passing a modification callback into DividerConfigForm would allow it to explicitly add the divider ID to changes.modified.

Would you like me to fetch all other comments on this PR to validate them and implement minimal fixes for those as well?

superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx

const handleValuesChange = (changedValues: any, allValues: any) => {
  // ... existing logic
  if (changedValues.filters) {
    const changedId = Object.keys(changedValues.filters)[0];
    if (isDivider(changedId)) {
      handleModifyItem(changedId);
    }
  }
  // ...
};

@bito-code-review bito-code-review Bot left a comment

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.

Code Review Agent Run #d2347d

Actionable Suggestions - 3
  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx - 2
  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx - 1
Additional Suggestions - 2
  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx - 1
    • Dead code from incomplete removal · Line 650-691
      Since the operatorType auto-reset logic depends on columns.type_generic, you should either add 'columns.type_generic' back to the API query's columns list or remove the related operatorType auto-reset code (lines 650–691) entirely.
  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx - 1
    • Missing test coverage for isColumnSelect · Line 322-350
      The new `isColumnSelect` feature block (lines 322-350) lacks test coverage. Existing tests mock `ColumnSelect` but not `DatasetColumnSelect`. Add unit tests following the pattern in lines 212-251 to validate the new feature's behavior, including `notifyChange` propagation and initial value resolution.
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx - 1
    • Missing unit tests for DatasetColumnSelect · Line 105-149
  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx - 1
Review Details
  • Files reviewed - 5 · Commit Range: 3596459..3596459
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/index.ts
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.ts
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Copilot AI left a comment

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.

Pull request overview

This PR refactors native filter cascading-dependency support to be registry-driven (based on plugin metadata behaviors) and extends the native filter config modal’s control mapping to support plugin-declared dataset column picker controls (isColumnSelect).

Changes:

  • Replaced the hardcoded native-filter dependency allowlist with a registry-based Behavior.NativeFilter check.
  • Exported the new dependency-support helper through the hooks index and config modal module.
  • Added isColumnSelect handling in getControlItemsMap and introduced a dataset-column fetching select control for plugins.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.ts Switches dependency eligibility to a registry/behavior-based check.
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/index.ts Re-exports the new helper instead of the removed hardcoded allowlist.
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx Wires the new helper export; modifies form onValuesChange handling.
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx Adds isColumnSelect rendering and a dataset column select control.
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx Adopts the new dependency helper; includes additional unrelated UI logic changes.

- Mark divider edits as modified so canSave is enabled for divider changes
- Persist sortMetric=undefined when Select is cleared via allowClear
- Restore datasetLabel() for semantic-layers feature flag consistency
- Restore time_grains allowlist UI for filter_timegrain config
- Restore operatorType (Match type) UI for filter_select config
- Fix ControlLabel to resolve function-typed label/description props
- Fix DatasetColumnSelect: add cancellation, error handling, allowClear

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@netlify

netlify Bot commented Jun 9, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit d56f758
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a4bc4f94aa2f80008f11bf8
😎 Deploy Preview https://deploy-preview-40905--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@bito-code-review

bito-code-review Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #0adbb6

Actionable Suggestions - 0
Additional Suggestions - 2
  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx - 1
    • Null reference on undefined object · Line 660-660
      The `onOperatorTypeChanged` function accesses `.controlValues` on a potentially undefined object. If `form.getFieldValue('filters')?.[filterId]` is undefined, this will throw a TypeError at runtime. Add optional chaining (`?.controlValues`) and a fallback empty object.
      Code suggestion
      --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
      +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
       @@ -657,7 +657,7 @@ const FiltersConfigForm = (
          }, [formFilter?.column, datasetDetails?.columns]);
       
          const onOperatorTypeChanged = (value: SelectFilterOperatorType) => {
      -    const previous = form.getFieldValue('filters')?.[filterId].controlValues;
      +    const previous = form.getFieldValue('filters')?.[filterId]?.controlValues ?? {};
            setNativeFilterFieldValues(form, filterId, {
              controlValues: {
                ...previous,
  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx - 1
    • Tooltip text conversion complexity · Line 98-103
      The `tooltipText` extraction has redundant String conversion logic (lines 98-103). When `resolvedDescription` is a React element (e.g., `text`), `String(resolvedDescription)` produces `'[object Object]'` instead of preserving the rich content. Consider directly using `resolvedDescription` when it's ReactNode, or document this behavior.
Review Details
  • Files reviewed - 3 · Commit Range: 3596459..212fbbc
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

ashah65 and others added 2 commits June 10, 2026 10:37
…g in FiltersConfigForm

Add optional chaining on controlValues access to prevent TypeError when
filter field value is undefined, and pass ReactNode descriptions directly
to InfoTooltip to avoid String() coercion producing [object Object].

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bito-code-review

bito-code-review Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #28597f

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 212fbbc..627d19e
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

⚠️ Translation Regression Detected

A source change in this PR renamed or reworded strings, invalidating existing translations (they are now #, fuzzy) in fi, th. Please resolve the affected .po files before merging.

Note: intentionally deleting a translatable string is not a regression and is not flagged here — only translations invalidated by a renamed/reworded source string are.

Language Fuzzy before Fuzzy after New
fi 4808 4809 +1
th 4808 4809 +1

How to fix

1. Install dependencies (if not already set up):

pip install -r superset/translations/requirements.txt
sudo apt-get install gettext   # or: brew install gettext

2. Re-extract strings and sync .po files:

./scripts/translations/babel_update.sh

This rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.

3. Resolve the fuzzy entries in the affected language files (fi, th):

grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.po

For each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.

4. Commit your changes to the .po files.

…ilter-dependency-registry

# Conflicts:
#	superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
…bility, not generic behavior

Replace the Behavior.NativeFilter check in filterSupportsDependencies with a
dedicated supportsCascadeDependencies flag on ChartMetadata. Every native
filter declares Behavior.NativeFilter, so the previous check silently let
filter_timegrain/filter_timecolumn act as cascade parents even though they
emit dataset/column-bound extraFormData (time_grain_sqla, granularity_sqla)
that isn't safe to merge into a child filter on a different dataset. Restores
the original filter_select/filter_range/filter_time allowlist behavior while
keeping the capability extensible for other plugins to opt into.

Also reset defaultDataMask when the dataset column changes in the plugin
column-picker path, mirroring the existing groupby column handler, so a
stale default isn't persisted against the newly selected column.
@bito-code-review

bito-code-review Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #040c3a

Actionable Suggestions - 0
Additional Suggestions - 1
  • superset-frontend/packages/superset-ui-core/src/chart/models/ChartMetadata.ts - 1
    • Optional vs required property alignment · Line 112-112
      Class property `supportsCascadeDependencies: boolean` (line 112) is non-optional while the config interface has `supportsCascadeDependencies?: boolean` (line 64). This is correct for the pattern where the instance always has a boolean value (defaulting to false), but verify this is the intended design for consistency with other properties.
Review Details
  • Files reviewed - 7 · Commit Range: 627d19e..34de64d
    • superset-frontend/packages/superset-ui-core/src/chart/models/ChartMetadata.ts
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/NativeFiltersModal.test.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.ts
    • superset-frontend/src/filters/components/Range/index.ts
    • superset-frontend/src/filters/components/Select/index.ts
    • superset-frontend/src/filters/components/Time/index.ts
  • Files skipped - 0
  • Tools
    • Eslint (Linter) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

…cade fallback

Add validation rules to the isColumnSelect plugin control loop in
getControlItemsMap.tsx, mirroring the existing groupby column handler, so a
plugin control declaring config.required is actually validated before save.

Make ChartMetadata.supportsCascadeDependencies distinguish "never declared"
(undefined) from an explicit opt-out (false), and have
filterSupportsDependencies fall back to the plugin's Behavior.NativeFilter
declaration only when the flag is unset. This preserves cascade-dependency
support for existing/third-party native filter plugins that predate the new
flag, while filter_timegrain/filter_timecolumn keep their explicit false
opt-out instead of relying on an implicit default.

Update NativeFiltersModal.test.tsx's getChartMetadataRegistry mock to include
supportsCascadeDependencies for filter_select, matching production metadata.
@bito-code-review

bito-code-review Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #3f71f9

Actionable Suggestions - 0
Additional Suggestions - 1
  • superset-frontend/packages/superset-ui-core/src/chart/models/ChartMetadata.ts - 1
    • Missing unit test coverage · Line 112-114
      The unit test in `ChartMetadata.test.ts` does not cover `supportsCascadeDependencies`. Without an explicit test, future refactors could inadvertently re-add a default value or change the property semantics, breaking the intended behavior change. Rule 6262 requires tests to verify actual business logic, not just component rendering.
Review Details
  • Files reviewed - 6 · Commit Range: 34de64d..853a936
    • superset-frontend/packages/superset-ui-core/src/chart/models/ChartMetadata.ts
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/NativeFiltersModal.test.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.ts
    • superset-frontend/src/filters/components/TimeColumn/index.ts
    • superset-frontend/src/filters/components/TimeGrain/index.ts
  • Files skipped - 0
  • Tools
    • Eslint (Linter) - ✔︎ Successful
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.80220% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.80%. Comparing base (045674a) to head (374626d).
⚠️ Report is 642 commits behind head on master.

Files with missing lines Patch % Lines
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx 80.00% 1 Missing ⚠️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #40905      +/-   ##
==========================================
+ Coverage   63.76%   63.80%   +0.03%     
==========================================
  Files        2652     2652              
  Lines      144846   144901      +55     
  Branches    33421    33436      +15     
==========================================
+ Hits        92366    92455      +89     
+ Misses      50808    50774      -34     
  Partials     1672     1672              
Flag Coverage Δ
javascript 68.62% <97.80%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…kbox, and filter control handlers

Closes Codecov patch-coverage gaps from the extensible filter dependency
registry changes by exercising the new isColumnSelect plugin control,
the requiredFirst checkbox branch, and the sort/single-value/match-type
handlers that previously had no test invoking them.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bito-code-review

bito-code-review Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #bbac83

Actionable Suggestions - 0
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx - 2
Review Details
  • Files reviewed - 2 · Commit Range: 853a936..c506e5d
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@EnxDev EnxDev left a comment

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.

EnxDev's Review Agent — #40905 · HEAD c506e5d

comment — Core registry refactor is sound and most earlier bot findings are already addressed at this HEAD; one CI blocker (prettier) plus a couple of small guards.

Note: several automated reviews above (Copilot/codeant/bito) were filed against an earlier, larger force-pushed revision. Their findings — broken handleValuesChange divider tracking, missing defaultDataMask reset, missing required validation, missing fetch error handling, the stale-response race, removed operatorType/time_grains/datasetLabel UI — are not present at HEAD c506e5d; they're fixed or never existed here. Reviewed against the current diff only.

🔴 Functional

  • CI pre-commit is redprettier-frontend reformats getControlItemsMap.test.tsx (the rest are (unchanged)). lint-frontend passes, so it's pure formatting. Run pre-commit run prettier --files .../getControlItemsMap.test.tsx (or npm run prettier) and push — this blocks merge.

🟡 Should-fix

  • getControlItemsMap.tsx:273 — the renderTrigger checkbox loop doesn't exclude isColumnSelect controls. A plugin declaring both renderTrigger: true and isColumnSelect: true renders twice (checkbox and column picker). Add && !item.config?.isColumnSelect to the filter as a cheap guard? (test: a control config with both flags renders one element)
  • useFilterOperations.ts:26 — the behavior fallback makes every already-installed third-party Behavior.NativeFilter plugin cascade-capable by default, where before only filter_range/select/time were. That's the intended extensibility, but it's a silent default change for existing deployments — worth stating explicitly in the PR body/docs so it's a deliberate opt-out (supportsCascadeDependencies: false), not a surprise.

🔵 Nits

  • FiltersConfigModal.tsx:474useCallback(fn, deps)useMemo(() => fn, deps) is churn; they're identical, keep useCallback. Also now reads only Object.keys(changedValues.filters)[0], where the original iterated all changed ids — if two dividers change in one event the rest are skipped (edge case).
  • getControlItemsMap.tsx:191notifyChange swaps the checkbox path's order (formChanged(); forceUpdate()forceUpdate(); formChanged()). Harmless, but an unflagged behavior reorder inside a "refactor."
  • FiltersConfigForm.tsx:950 — dropping optionRender/getOptionDataTest removes data-test hooks from the filter-type/customization-type options, and the new sort/single-value/match-type modal tests are unrelated to this PR's stated goal. Confirmed nothing else references those data-test strings (safe), but the scope creep makes review harder — consider splitting.

🙌 Praise

  • ChartMetadata.ts:56 / TimeColumn+TimeGrain index.tssupportsCascadeDependencies undefined → fall back to NativeFilter behavior, with time-grain/time-column explicitly false, cleanly preserves third-party extensibility while blocking the unsafe cross-dataset extraFormData merge. Correct call.
  • getControlItemsMap.tsx:135DatasetColumnSelect guards both the stale-response race (cancelled flag on datasetId change) and fetch errors. Good async hygiene.

Reviewed by EnxDev's Review Agent — @EnxDev.

…ocks

Replace the any-typed filterValues column param and the as-any-cast
cachedSupersetGet mock payloads with concrete types, per code review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…description callbacks

ControlLabel now only invokes zero-argument label/description callbacks;
state-dependent ones (state, controlState, chartState) are a real
BaseControlConfig pattern but this control list has no such state to
supply, so invoking them would throw. Also flattens the new
isColumnSelect test block into standalone test() cases instead of
introducing a second describe() wrapper.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bito-code-review

bito-code-review Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #1ff897

Actionable Suggestions - 0
Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx - 2
Review Details
  • Files reviewed - 2 · Commit Range: c506e5d..b9bcea0
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

…n picker

DatasetColumnSelect fetched columns for the new dataset but never checked
whether the previously selected value still existed, so switching datasets
could leave an invalid column reference in form state and saved with the
filter. Mirror ColumnSelect's existing reset behavior by clearing the value
via onChange(null) when it's absent from the fetched columns, the fetch
fails, or the dataset is cleared.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment on lines +167 to +173
.catch(() => {
if (cancelled) return;
setFetchState({ loadedForId: datasetId, fetchedColumns: [] });
if (value) {
onChange?.(null);
}
});

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 error path clears the current column value whenever the dataset-column fetch fails, so a transient API/network failure will silently wipe an already-saved plugin column selection and trigger downstream default-mask reset logic even though the user did not change anything. Keep the existing value on fetch failure and only clear when you positively know the selected column is invalid from a successful response. [logic error]

Severity Level: Major ⚠️
❌ Native filter plugin column-picker loses selection on fetch failure.
⚠️ Default data mask resets unexpectedly on transient dataset API errors.
Steps of Reproduction ✅
1. Open a dashboard and launch the Manage native filters modal, which renders
`FiltersConfigForm` at
`/workspace/superset/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:410-449`;
this calls `getControlItemsMap` with `datasetId` and `filterToEdit` (line 10-24 of that
snippet).

2. Register or use a filter plugin whose control panel item has `config.isColumnSelect ===
true` so that `getControlItemsMap` renders the plugin column picker block at
`/workspace/superset/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx:111-154`,
wrapping `DatasetColumnSelect` in a `StyledFormItem` with `name={['filters', filterId,
'controlValues', controlItem.name]}` and `initialValue` bound to the previously saved
column (lines 115-125).

3. Ensure the filter already has a saved column selection in
`controlValues[controlItem.name]` so that AntD Form injects this as the `value` prop into
`DatasetColumnSelect` when the modal opens; the component’s effect (lines 143-178) calls
`cachedSupersetGet` to fetch dataset columns from `/api/v1/dataset/${datasetId}` (lines
152-156).

4. Trigger a transient failure for that dataset API call (e.g., by forcing the endpoint to
return 500 or simulating a network error) so `cachedSupersetGet` rejects; the `.catch`
block at lines 167-173 executes, and because `value` still contains the previously saved
column, `if (value) { onChange?.(null); }` clears the form field, wiping the user’s column
selection and (via the Form-controlled `onChange` plus the plugin’s `onChange` callback at
lines 138-146) also resets `defaultDataMask` even though the user did not change the
column.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
**Line:** 167:173
**Comment:**
	*Logic Error: The error path clears the current column value whenever the dataset-column fetch fails, so a transient API/network failure will silently wipe an already-saved plugin column selection and trigger downstream default-mask reset logic even though the user did not change anything. Keep the existing value on fetch failure and only clear when you positively know the selected column is invalid from a successful response.

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

@bito-code-review bito-code-review Bot left a comment

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.

Code Review Agent Run #3c0345

Actionable Suggestions - 1
  • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx - 1
Review Details
  • Files reviewed - 2 · Commit Range: b9bcea0..d56f758
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Comment on lines +143 to +178
useEffect(() => {
if (!datasetId) {
// dataset cleared — drop any stale selection immediately
if (value) {
onChange?.(null);
}
return undefined;
}
let cancelled = false;
cachedSupersetGet({
endpoint: `/api/v1/dataset/${datasetId}?q=${rison.encode({
columns: ['columns.column_name'],
})}`,
})
.then(({ json: { result } }) => {
if (cancelled) return;
const columnNames: string[] = result.columns
.map((col: { column_name: string }) => col.column_name)
.filter(Boolean);
setFetchState({ loadedForId: datasetId, fetchedColumns: columnNames });
if (value && !columnNames.includes(value)) {
onChange?.(null);
}
})
.catch(() => {
if (cancelled) return;
setFetchState({ loadedForId: datasetId, fetchedColumns: [] });
if (value) {
onChange?.(null);
}
});
return () => {
cancelled = true;
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [datasetId]);

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.

Missing test coverage for new behavior

New behavior introduced by this diff (clearing stale selections on dataset clear, column mismatch, and errors) lacks test coverage. Per testing guidelines, verify actual business logic in tests, not just component rendering.

Code Review Run #3c0345


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

  • Yes, avoid them

@rusackas

rusackas commented Jul 7, 2026

Copy link
Copy Markdown
Member

Thanks @ashah65. The core change, swapping the hardcoded ALLOW_DEPENDENCIES list for the registry-driven supportsCascadeDependencies flag, is clean, and the core filters map to the exact same behavior as before, so no regression to the cascade gate.

Two things: this PR is also carrying an isColumnSelect control, a divider change-tracking refactor, and a ControlLabel change that aren't really part of the titled feature. Any chance of splitting those out? The dependency change stands on its own, and the riders widen the review surface a fair bit.

There's also a small bug in the new isColumnSelect path the bot flagged... left a suggestion on it.

…rsConfigModal/FiltersConfigForm/getControlItemsMap.tsx

Co-authored-by: Evan Rusackas <evan@preset.io>
@EnxDev

EnxDev commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

EnxDev's Review Agent — #40905 · HEAD 8e50b80

comment — Core registry refactor is sound; the prettier CI blocker from my last pass is now green, and the remaining items are scope + minor guards, not blockers.

Supersedes my earlier review at c506e5d. Since then, new commits landed and CI is now fully greenpre-commit (current) passes, so the prettier failure I flagged before is resolved. No new regressions in the delta.

Note on the automated reviews above: several Copilot/codeant/bito findings target an earlier, larger force-pushed revision. Verified against the diff at HEAD 8e50b80, the following are not present here and should not gate merge — broken handleValuesChange divider tracking (it does call handleModifyItem via isDivider), String(description)[object Object] (now passes the ReactNode through), the stale-response race (guarded by the cancelled flag), missing saved-value restore (injected via Form.Item initialValue), and the removed operatorType/time_grains/datasetLabel UI (none of those are in this diff).

🟡 Should-fix

  • getControlItemsMap.tsx:294 — the renderTrigger checkbox loop still doesn't exclude isColumnSelect controls. A plugin declaring both renderTrigger: true and isColumnSelect: true renders twice — once as a checkbox (controlItems) and once as the column picker (mainControlItems). Add && !item.config?.isColumnSelect to the filter as a cheap guard. regression test: a control config with both flags should yield exactly one element.
  • Scope — reinforcing @rusackas: the titled feature is the registry-driven dependency gate (ChartMetadata flag + filterSupportsDependencies + the 5 plugin flags). The isColumnSelect/DatasetColumnSelect column-picker API, the ControlLabel/resolveInitialValue refactors, and the handleValuesChange divider refactor are unrelated riders that widen the surface — and isColumnSelect ships a plugin-facing control API with zero in-tree consumers (exercised only via mocks). Worth splitting into its own PR.
  • useFilterOperations.ts:21 — the behavior fallback makes every already-installed third-party Behavior.NativeFilter plugin cascade-capable by default, where before only filter_range/select/time qualified. That's the intended extensibility, but it's a silent default change for existing deployments — state it explicitly in the PR body/docs so the opt-out (supportsCascadeDependencies: false) is deliberate, not a surprise.

🔵 Nits

  • getControlItemsMap.tsx:159DatasetColumnSelect's .catch sets empty options with no user feedback; a failed dataset fetch leaves the picker silently empty. Keeping the saved value on a transient failure is right, but a addDangerToast (as the groupby ColumnSelect path does) would surface the failure.
  • FiltersConfigModal.tsx:474useCallback(fn, deps)useMemo(() => fn, deps) is churn; identical result, keep useCallback. It also now reads only Object.keys(changedValues.filters)[0] where master iterated all changed ids — safe because antd reports one changed leaf per event, but the single-key assumption is implicit; a one-line comment would help.
  • getControlItemsMap.tsx:210notifyChange reorders the checkbox path (formChanged(); forceUpdate()forceUpdate(); formChanged()). Harmless, but an unflagged behavior reorder inside a "refactor."
  • FiltersConfigForm.tsx:950 — dropping optionRender/getOptionDataTest removes data-test hooks from the filter-type/customization-type options. Confirmed nothing else references those strings (safe), but it's unrelated scope.

🙌 Praise

  • TimeColumn/TimeGrain index.ts — explicitly setting supportsCascadeDependencies: false preserves the prior exclusion while the undefined→NativeFilter fallback keeps third-party plugins extensible. That's the subtle part that makes the refactor behavior-preserving — correct call.
  • getControlItemsMap.tsx:143DatasetColumnSelect guards both the stale-response race (cancelled flag keyed on datasetId) and fetch errors, and the new tests cover stale-value reset, fetch-reject fallback, and dataset-cleared reset. Good async hygiene and coverage.

Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 8e50b80.

…column-picker race condition

Split filterSupportsDependencies into filterCanBeDependencyParent and
filterCanHaveDependencies so a filter that opts out of being a cascade
parent (e.g. filter_timegrain) can still be configured to depend on
other filters, which the single predicate incorrectly prevented.

Add a fallback label to the checkbox and isColumnSelect control loops
in getControlItemsMap so controls with non-zero-arity label functions
no longer render blank.

Fix DatasetColumnSelect to read the latest value via a ref inside its
async fetch handlers instead of a stale closure, and stop clearing the
selected value when the dataset-column fetch fails outright, since a
failed request says nothing about the value's validity.
@bito-code-review

bito-code-review Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #3e30ca

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: d56f758..8e50b80
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

…y-registry' into feat/extensible-filter-dependency-registry

# Conflicts:
#	superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
@bito-code-review

bito-code-review Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #658896

Actionable Suggestions - 0
Review Details
  • Files reviewed - 7 · Commit Range: 8e50b80..374626d
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.test.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/index.ts
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.test.ts
    • superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.ts
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dashboard:native-filters Related to the native filters of the Dashboard packages size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants