Skip to content

fix(dashboard): prevent table chart infinite reload loop#36686

Merged
rusackas merged 1 commit into
masterfrom
fix-table-chart-reload
Dec 19, 2025
Merged

fix(dashboard): prevent table chart infinite reload loop#36686
rusackas merged 1 commit into
masterfrom
fix-table-chart-reload

Conversation

@sadpandajoe
Copy link
Copy Markdown
Member

SUMMARY

Fixes an infinite reload loop in Dashboard context where table charts would continuously re-query, causing hundreds/thousands of API calls per user.

Root Cause: The recent "Export table data with Search box" feature (PR #36281) added clientView to ownState on every filtered-row change. In Dashboard context, getRelevantDataMask passed this through unchanged, causing Dashboard.jsx to see continuous state changes and trigger re-queries in an infinite loop.

Fix: Strip clientView from ownState in getRelevantDataMask (the selector that extracts chart state for Dashboard). This matches the existing pattern in ExploreViewContainer which already strips clientView before comparing state changes.

  // Before: passed ownState through unchanged
  .map(item => [item.id, item[prop]])

  // After: strips clientView when prop is 'ownState'
  .map(item => [
    item.id,
    prop === 'ownState' ? omit(item[prop], ['clientView']) : item[prop],
  ])

Why this location: clientView is explicitly for export functionality, not for triggering re-queries. Stripping at the selector level ensures Dashboard never sees these changes as state updates.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

  1. Open a dashboard containing a table chart with server-side pagination disabled
  2. Apply a filter or interact with the table search box
  3. Open browser DevTools → Network tab
  4. Verify there are NO repeated /api/v1/chart/data calls
  5. Verify the "Export" functionality still works correctly (exports filtered data)

ADDITIONAL INFORMATION

  • Has associated issue: Fixes Table charts reloading non-stop #36595
  • 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

@sadpandajoe sadpandajoe requested a review from Copilot December 17, 2025 01:09
@sadpandajoe sadpandajoe added the 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR label Dec 17, 2025
@github-actions github-actions Bot added 🎪 0770786 🚦 building 🎪 ⌛ 48h Environment expires after 48 hours (default) and removed 🎪 ⚡ showtime-trigger-start Create new ephemeral environment for this PR labels Dec 17, 2025
@github-actions
Copy link
Copy Markdown
Contributor

🎪 Showtime is building environment on GHA for 0770786

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 fixes a critical infinite reload loop in dashboard table charts caused by the clientView property being continuously written to ownState. The fix strips clientView from ownState in the getRelevantDataMask selector, preventing Dashboard from seeing these changes as state updates that trigger re-queries.

Key changes:

  • Modified getRelevantDataMask to strip clientView from ownState using lodash omit
  • Added comprehensive unit tests in activeAllDashboardFilters.test.ts covering the fix and edge cases
  • Added integration tests in Dashboard.test.jsx verifying Dashboard behavior with ownDataCharts changes

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts Added logic to strip clientView from ownState in the getRelevantDataMask selector, preventing infinite reload loops
superset-frontend/src/dashboard/util/activeAllDashboardFilters.test.ts Added 4 comprehensive test cases covering clientView stripping behavior, equality checks, and verifying other properties remain unchanged
superset-frontend/src/dashboard/components/Dashboard.test.jsx Added 2 integration tests verifying Dashboard correctly handles ownDataCharts updates when clientView changes are filtered out

The implementation correctly mirrors the existing pattern in ExploreViewContainer (lines 606 and 946) where clientView is already stripped. The fix is minimal, well-tested, and addresses the root cause at the selector level where Dashboard receives state updates.

@github-actions
Copy link
Copy Markdown
Contributor

🎪 Showtime deployed environment on GHA for 0770786

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

@github-actions
Copy link
Copy Markdown
Contributor

🎪 Showtime is building environment on GHA for b6e572c

@github-actions
Copy link
Copy Markdown
Contributor

🎪 Showtime deployed environment on GHA for 9e6d1c7

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

@sadpandajoe sadpandajoe marked this pull request as ready for review December 17, 2025 21:29
@dosubot dosubot Bot added the dashboard:performance Related to Dashboard performance label Dec 17, 2025
.filter(item => item[prop])
.map(item => [item.id, item[prop]]),
.map(item => {
const value = item[prop];
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this be handled by the chart component instead of getRelevantDataMask? I'm wondering if clientView might be useful for another caller of getRelevantDataMask and should just be discarded by the chart when re-rendering.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think modifying the table chart plugin (separate package) to conditionally write clientView based on context could potentially be more complex and could introduce regressions. Also this mirrors how Explore does it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we have two options:

  1. Given the "relevant" word on getRelevantDataMask, we might consider that clientView is not relevant and your solution would be correct. This means that clientView is never relevant for any caller of this function which might be true given that there's only one caller currently (DashboardPage.tsx).
  2. If clientView might be useful for other callers, we could change DashboardPage.tsx and omit the property there:
const selectRelevantDatamask = createSelector(
  (state: RootState) => state.dataMask,
  dataMask => omit(getRelevantDataMask(dataMask, 'ownState'), ['clientView']),
);

I'm good with either solution, so I'm approving the PR.

@sadpandajoe sadpandajoe removed the 🎪 ⌛ 48h Environment expires after 48 hours (default) label Dec 19, 2025
@rusackas
Copy link
Copy Markdown
Member

OMG THANK YOU. @SBIN2010 and I were just talking about this yesterday...

@rusackas rusackas merged commit c026ae2 into master Dec 19, 2025
177 of 181 checks passed
@rusackas rusackas deleted the fix-table-chart-reload branch December 19, 2025 21:45
sadpandajoe added a commit that referenced this pull request Dec 29, 2025
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
(cherry picked from commit c026ae2)
@SkinnyPigeon
Copy link
Copy Markdown
Contributor

Hey @rusackas, @sadpandajoe 👋. Do we have an estimate for when this will be released? This has been a real issue for us for a month or so and we'd love to get this out to our teams

@rusackas
Copy link
Copy Markdown
Member

Looks like it's on Preset, but for Superset, it'll likely be part of 6.0.1 or 6.1.0, whichever comes first (both are being worked on, but it's a volunteer thing to build/fix/test these releases, so we do our best)

@rusackas
Copy link
Copy Markdown
Member

@sadpandajoe might have a more detailed answer.

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

Labels

dashboard:performance Related to Dashboard performance size/L v6.0 Label added by the release manager to track PRs to be included in the 6.0 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Table charts reloading non-stop

5 participants