Skip to content

Conversation

@geido
Copy link
Member

@geido geido commented Jan 6, 2026

SUMMARY

Fixes an issue where dashboard filters with default values were not automatically applied on dashboard load when extraFormData was set to empty. Previously, filters would show default values selected but require users to manually click the Apply button. The fix automatically applies filters when they have default values but empty extraFormData, ensuring charts load with filters applied from the start.

AFTER

531449025-2a09ba78-5d55-44e9-992e-69914e15dd43.mp4

TESTING INSTRUCTIONS

  1. Create a dashboard.
  2. Add filters to it. Set default values for the filters.
  3. Manually edit the dashboard config to set the extraFormData value to empty.
  4. Reopen the dashboard.
  5. The dashboard loads with the default filter values properly applied.

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

@codeant-ai-for-open-source
Copy link
Contributor

CodeAnt AI is reviewing your PR.


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@netlify
Copy link

netlify bot commented Jan 6, 2026

Deploy Preview for superset-docs-preview canceled.

Name Link
🔨 Latest commit d14e068
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/695d422f0b6bdb00086e13df

@bito-code-review
Copy link
Contributor

bito-code-review bot commented Jan 6, 2026

Code Review Agent Run #e29eb6

Actionable Suggestions - 0
Additional Suggestions - 1
  • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx - 1
    • Test assertion missing · Line 355-410
      The test 'auto-applies filter when extraFormData is empty in applied state' sets up a spy on updateDataMask but doesn't assert it's called. The auto-apply logic requires appliedDataMask to have value with empty extraFormData and incoming dataMask to have non-empty extraFormData, but here both have empty extraFormData, so updateDataMask won't be called. This means the test doesn't verify the auto-apply behavior as named.
Review Details
  • Files reviewed - 3 · Commit Range: d14e068..d14e068
    • superset-frontend/src/dashboard/components/FiltersBadge/index.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.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 [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

@dosubot dosubot bot added the dashboard:native-filters Related to the native filters of the Dashboard label Jan 6, 2026
@codeant-ai-for-open-source
Copy link
Contributor

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review

  • Changed status-condition semantics
    The new condition makes shouldRecalculate true whenever prevChartStatus !== 'success', independent of whether any meaningful chart/filter state changed. This is different from the previous control flow (where recalculation only happened when prevChartStatus !== 'success' AND one of the comparisons changed). Verify this is intentional — it can cause extra work or incorrect indicator updates.

  • Redundant dispatch risk
    The new auto-apply logic dispatches updateDataMask when needsAutoApply is true. If appliedDataMask and the incoming dataMask are deeply equal this dispatch is redundant and may trigger unnecessary re-renders, url updates, or network activity. Confirm a deep-equality check is performed before dispatching to avoid extra work or update loops.

  • Reference vs deep equality
    The new effect uses reference equality (e.g., dataMask !== prevDataMask, nativeFilters !== prevNativeFilters, chartLayoutItems !== prevChartLayoutItems) to decide when to recalculate indicators. Complex objects are often recreated even when semantically equal which can cause unnecessary recalculations, or conversely, deep changes masked by stable references could be missed. Consider using a deep-equality check for these objects or a more targeted change detector.

  • Missing assertion
    The new test "auto-applies filter when extraFormData is empty in applied state" creates a spy on updateDataMask but never asserts it was called. The test advances timers and only checks UI presence, so it can pass without actually verifying the intended auto-apply behavior. Add explicit assertions to ensure the auto-apply side effect happened.

  • Potential side effects from spying original action
    The test uses jest.spyOn(dataMaskActions, 'updateDataMask') without mocking the implementation. If updateDataMask triggers real dispatches or other side effects, the test may become flaky or impact global state. Consider mocking the implementation to a no-op action object or safe stub to isolate the unit test.

Comment on lines 209 to 211
const isFirstTimeInitialization =
!initializedFilters.has(filter.id) &&
dataMaskSelectedRef.current[filter.id]?.filterState?.value ===
undefined;
existingDataMask?.filterState?.value === undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: The isFirstTimeInitialization flag currently also depends on existingDataMask?.filterState?.value === undefined, which means that if a filter ever gets a value in dataMaskSelected before it has been marked as initialized (for example via a default/permalink value without extraFormData), it will permanently stop being treated as "first time" and required-first auto-apply logic may never fire even though initializedFilters was never updated; using only the explicit initializedFilters tracking avoids this inconsistent state. [logic error]

Severity Level: Minor ⚠️

Suggested change
const isFirstTimeInitialization =
!initializedFilters.has(filter.id) &&
dataMaskSelectedRef.current[filter.id]?.filterState?.value ===
undefined;
existingDataMask?.filterState?.value === undefined;
const isFirstTimeInitialization = !initializedFilters.has(filter.id);
Why it matters? ⭐

The current check couples "first-time" to the presence of an existing value in the selected mask. That can create a surprising state where a filter receives a value (e.g. from permalink/default) before initializedFilters is ever updated, which makes isFirstTimeInitialization false even though we never actually marked this filter as initialized. For requiredFirst handling we only care whether the filter was recorded in initializedFilters; using the explicit flag (!initializedFilters.has(filter.id)) is a clearer and more robust signal and will restore expected required-first auto-apply behavior. This is a functional fix, not just a cosmetic change.

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

**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
**Line:** 209:211
**Comment:**
	*Logic Error: The `isFirstTimeInitialization` flag currently also depends on `existingDataMask?.filterState?.value === undefined`, which means that if a filter ever gets a value in `dataMaskSelected` before it has been marked as initialized (for example via a default/permalink value without `extraFormData`), it will permanently stop being treated as "first time" and required-first auto-apply logic may never fire even though `initializedFilters` was never updated; using only the explicit `initializedFilters` tracking avoids this inconsistent state.

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.

Copy link
Member

Choose a reason for hiding this comment

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

This seems relevant?

@codeant-ai-for-open-source
Copy link
Contributor

CodeAnt AI finished reviewing your PR.

Copy link
Member

@msyavuz msyavuz left a comment

Choose a reason for hiding this comment

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

There's the bot comment about permalinks that might need checking. Otherwise LGTM

@geido geido force-pushed the geido/fix/fitlers-auto-apply branch from d14e068 to 23c5ad6 Compare January 7, 2026 11:14
@codeant-ai-for-open-source
Copy link
Contributor

CodeAnt AI is running Incremental review


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@geido geido added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Jan 7, 2026
@github-actions github-actions bot added 🎪 23c5ad6 🚦 building Environment 23c5ad6 status: building 🎪 23c5ad6 📅 2026-01-07T11-15 Environment 23c5ad6 created at 2026-01-07T11-15 🎪 23c5ad6 🤡 geido Environment 23c5ad6 requested by geido 🎪 ⌛ 48h Environment expires after 48 hours (default) and removed 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR labels Jan 7, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2026

🎪 Showtime is building environment on GHA for 23c5ad6

@github-actions github-actions bot added 🎪 23c5ad6 🚦 deploying Environment 23c5ad6 status: deploying 🎪 23c5ad6 🚦 running Environment 23c5ad6 status: running 🎪 🎯 23c5ad6 Active environment pointer - 23c5ad6 is receiving traffic 🎪 23c5ad6 🌐 54.244.145.45:8080 Environment 23c5ad6 URL: http://54.244.145.45:8080 (click to visit) and removed 🎪 23c5ad6 🚦 building Environment 23c5ad6 status: building 🎪 23c5ad6 🚦 deploying Environment 23c5ad6 status: deploying 🎪 23c5ad6 🚦 running Environment 23c5ad6 status: running 🎪 🎯 23c5ad6 Active environment pointer - 23c5ad6 is receiving traffic labels Jan 7, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2026

🎪 Showtime deployed environment on GHA for 23c5ad6

Environment: http://54.244.145.45:8080 (admin/admin)
Lifetime: 48h auto-cleanup
Updates: New commits create fresh environments automatically

Copy link
Contributor

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

Choose a reason for hiding this comment

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

Code Review Agent Run #a0652c

Actionable Suggestions - 1
  • superset-frontend/src/dashboard/components/FiltersBadge/index.tsx - 1
Additional Suggestions - 1
  • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx - 1
    • Missing test assertion · Line 355-410
      The test 'auto-applies filter when extraFormData is empty in applied state' sets up a spy on `updateDataMask` but lacks an assertion to confirm the auto-apply behavior. Without checking if the spy was invoked, the test doesn't validate the claimed functionality and could pass incorrectly. Consider adding an expect statement to assert the call, or clarify if the test intent is only rendering verification.
Review Details
  • Files reviewed - 3 · Commit Range: 23c5ad6..23c5ad6
    • superset-frontend/src/dashboard/components/FiltersBadge/index.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.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 [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

Comment on lines 204 to 235
const shouldRecalculate =
prevChartStatus !== 'success' ||
dataMask !== prevDataMask ||
chart?.queriesResponse?.[0]?.rejected_filters !==
prevChart?.queriesResponse?.[0]?.rejected_filters ||
chart?.queriesResponse?.[0]?.applied_filters !==
prevChart?.queriesResponse?.[0]?.applied_filters ||
nativeFilters !== prevNativeFilters ||
chartLayoutItems !== prevChartLayoutItems ||
prevChartConfig !== chartConfiguration;

if (shouldRecalculate) {
const newIndicators = selectNativeIndicatorsForChart(
nativeFilters,
dataMask,
chartId,
chart,
chartLayoutItems,
chartConfiguration,
);
setNativeIndicators(newIndicators);
} else if (!showIndicators && nativeIndicators.length > 0) {
setNativeIndicators(indicatorsInitialState);
} else if (prevChartStatus !== 'success') {
if (
chart?.queriesResponse?.[0]?.rejected_filters !==
prevChart?.queriesResponse?.[0]?.rejected_filters ||
chart?.queriesResponse?.[0]?.applied_filters !==
prevChart?.queriesResponse?.[0]?.applied_filters ||
nativeFilters !== prevNativeFilters ||
chartLayoutItems !== prevChartLayoutItems ||
dataMask !== prevDataMask ||
prevChartConfig !== chartConfiguration
) {
setNativeIndicators(
selectNativeIndicatorsForChart(
nativeFilters,
dataMask,
chartId,
chart,
chartLayoutItems,
chartConfiguration,
),
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic Error in useEffect

The reset logic for nativeIndicators when showIndicators is false should take precedence over recalculating. Currently, if shouldRecalculate evaluates to true but showIndicators is false, the code will set new indicators instead of resetting them, leading to incorrect state where indicators are shown when they shouldn't be.

Code Review Run #a0652c


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

  • Yes, avoid them

@github-actions github-actions bot removed the 🎪 23c5ad6 🌐 54.244.145.45:8080 Environment 23c5ad6 URL: http://54.244.145.45:8080 (click to visit) label Jan 9, 2026
@github-actions
Copy link
Contributor

🎪 Showtime deployed environment on GHA for 23c5ad6

Environment: http://34.221.74.245:8080 (admin/admin)
Lifetime: 48h auto-cleanup
Updates: New commits create fresh environments automatically

@github-actions github-actions bot removed 🎪 23c5ad6 🚦 running Environment 23c5ad6 status: running 🎪 23c5ad6 🌐 34.221.74.245:8080 Environment 23c5ad6 URL: http://34.221.74.245:8080 (click to visit) 🎪 23c5ad6 📅 2026-01-12T18-14 Environment 23c5ad6 created at 2026-01-12T18-14 🎪 23c5ad6 🤡 sadpandajoe Environment 23c5ad6 requested by sadpandajoe labels Jan 15, 2026
@geido geido force-pushed the geido/fix/fitlers-auto-apply branch from 23c5ad6 to 991cf73 Compare January 15, 2026 16:30
@codeant-ai-for-open-source
Copy link
Contributor

CodeAnt AI is running Incremental review


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@codeant-ai-for-open-source
Copy link
Contributor

CodeAnt AI Incremental review completed.

@bito-code-review
Copy link
Contributor

bito-code-review bot commented Jan 15, 2026

Code Review Agent Run #75376a

Actionable Suggestions - 0
Additional Suggestions - 1
  • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx - 1
    • Misleading test name and unused spy · Line 355-355
      The test name implies it verifies auto-apply behavior, but the state setup (with empty extraFormData in both applied and default dataMask) does not trigger the auto-apply logic from FilterBar's handleFilterSelectionChange. This could confuse developers expecting auto-apply testing. The spy on updateDataMask is also unused, as no assertion checks if it was called.
      Code suggestion
       @@ -355,1 +355,1 @@
      -  test('auto-applies filter when extraFormData is empty in applied state', async () => {
      +  test('renders filter icon when extraFormData is empty in applied state', async () => {
Review Details
  • Files reviewed - 3 · Commit Range: 991cf73..991cf73
    • superset-frontend/src/dashboard/components/FiltersBadge/index.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx
    • superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.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 [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

@geido geido added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Jan 15, 2026
@github-actions github-actions bot added 🎪 991cf73 🚦 building Environment 991cf73 status: building 🎪 991cf73 📅 2026-01-15T17-52 Environment 991cf73 created at 2026-01-15T17-52 🎪 991cf73 🤡 geido Environment 991cf73 requested by geido and removed 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR labels Jan 15, 2026
@github-actions
Copy link
Contributor

🎪 Showtime is building environment on GHA for 991cf73

@github-actions github-actions bot added 🎪 991cf73 🚦 deploying Environment 991cf73 status: deploying 🎪 991cf73 🚦 running Environment 991cf73 status: running 🎪 🎯 991cf73 Active environment pointer - 991cf73 is receiving traffic 🎪 991cf73 🌐 44.247.95.175:8080 Environment 991cf73 URL: http://44.247.95.175:8080 (click to visit) and removed 🎪 991cf73 🚦 building Environment 991cf73 status: building 🎪 991cf73 🚦 deploying Environment 991cf73 status: deploying 🎪 991cf73 🚦 running Environment 991cf73 status: running 🎪 🎯 991cf73 Active environment pointer - 991cf73 is receiving traffic labels Jan 15, 2026
@github-actions
Copy link
Contributor

🎪 Showtime deployed environment on GHA for 991cf73

Environment: http://44.247.95.175:8080 (admin/admin)
Lifetime: 48h auto-cleanup
Updates: New commits create fresh environments automatically

@github-actions github-actions bot removed 🎪 991cf73 🤡 geido Environment 991cf73 requested by geido 🎪 991cf73 🚦 running Environment 991cf73 status: running 🎪 991cf73 🌐 44.247.95.175:8080 Environment 991cf73 URL: http://44.247.95.175:8080 (click to visit) 🎪 991cf73 📅 2026-01-15T17-52 Environment 991cf73 created at 2026-01-15T17-52 labels Jan 17, 2026
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 size/L 🎪 ⌛ 48h Environment expires after 48 hours (default)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants