fix(dashboard): initialize label colors before mounting charts - #44557
Conversation
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Code Review Agent Run #c475feActionable Suggestions - 0Additional Suggestions - 2
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44557 +/- ##
==========================================
- Coverage 66.22% 66.18% -0.04%
==========================================
Files 2949 2949
Lines 177381 177383 +2
Branches 41107 41108 +1
==========================================
- Hits 117469 117407 -62
- Misses 57409 57467 +58
- Partials 2503 2509 +6
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:
|
Upstream apache#44574 landed the messages.pot regeneration on master; the babel-extract failure on this PR was base drift, not contributor code. Fresh CI will run against the new merge commit with the fixed base pot.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
Addressed both additional suggestions in Bito review #c475fe. The initialization sentinel is now null, so an absent dashboard ID cannot satisfy the render gate before hydration. The comment now explains that the grid waits for hydration and color initialization. The new regression fails on the previous head because the grid mounts before hydration, then passes after the fix and verifies mounting after the dashboard ID arrives. All 20 DashboardContainer tests and the full plugin build pass. |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hi @deepujain, thanks for chasing this one down. It is a fiddly corner of the dashboard and the change is small and well scoped, so this is an approve from me.
What I ran locally against 13bf6706af and its parent 61fffbe0c0: all five new cases in DashboardContainer.test.tsx fail on the parent and pass on this head, so they are real regression tests and not invariants that would have held either way. The DashboardContainer, DashboardBuilder, DashboardWrapper, DashboardPage, dashboardState and colorScheme suites are green on this head, 129 tests. I also drove an in-place dashboardInfo.id change through a mounted DashboardContainer on both branches and compared the dispatches, which is what the one inline note is about. On the sentinel change you just pushed, I checked every dashboardInfoChanged caller and they all run after hydration, and DashboardPage only mounts DashboardBuilder once dashboardInfo is non-empty, so null cannot leave the grid withheld in the app.
CI is fully green on this head, 69 passing and 14 skipped, including babel-extract, all eight jest shards, cypress and both playwright matrices, so the message drift you flagged in the description looks resolved.
One non-blocking comment inline, nothing that needs to hold the merge.
| dispatch(applyDashboardLabelsColorOnLoad(dashboardInfo.metadata)); | ||
| // apply labels color as dictated by stored metadata (if any) | ||
| setDashboardLabelsColorInitiated(true); | ||
| setColorInitializedDashboardId(dashboardInfo.id); |
There was a problem hiding this comment.
Not a blocker, just a note for the description: I think this line is the load-bearing half of the fix, and the summary currently reads as if the first-render race were the main story.
I ran both branches side by side. With DashboardContainer mounted and dashboardInfo.id changing in place, this branch dispatches applyDashboardLabelsColorOnLoad a second time with the new label_colors; on 61fffbe0c0 it is never dispatched again, because dashboardLabelsColorInitiated is already true. The effect cleanup has meanwhile run onBeforeUnload, which calls resetColors() on the namespace, so on master the second dashboard keeps palette colors for the rest of the session until a full reload. That matches the reporter's After refreshing or clicking legend, the colors become correct in #40708, and src/pages/Dashboard/index.tsx:25 renders DashboardPage with no key, so in-app navigation really does swap dashboardInfo.id under a mounted container.
For the first-render half I could not build a cold-load repro: hydrate.ts:174 seeds every chart from the initial chart state with queriesResponse: null, and DashboardPage.tsx:423 only mounts the container once dashboardInfo is populated, so no chart owns a query response at that first commit on a fresh page load. Am I missing a path there? Either way, calling the navigation case out in the description would make this easier to justify.
|
The observation is correct. The core of the fix is indeed the transition from a simple boolean superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx |
Code Review Agent Run #a1451eActionable Suggestions - 0Review 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 |
3 participants
SUMMARY
A chart with cached data can create its color scale during the dashboard's first render, before the dashboard effect installs
label_colors. Its initial output then keeps palette colors even though the metadata specifies custom colors.Keep the grid unmounted until that dashboard's color initialization has run. Track the initialized dashboard ID so navigating to another dashboard also waits for its colors. The existing color-selection and synchronization algorithms stay unchanged.
Fixes #40708.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
The screenshots use the production
DashboardContainer, ECharts Timeseries transform, and ECharts renderer in an isolated Storybook fixture with an already-cached query. They demonstrate initial rendering, not the reporter's authenticated deployment. The metadata requests dark red for60_Crashed, red for50_Error, and#008000for20_Passed.Before: the first render uses palette colors.
After: the same fixture uses the metadata colors on its first render.
Fixture and recorded browser series colors.
TESTING INSTRUCTIONS
Run the component and dashboard action suites:
59 tests pass. Four regression cases fail on unmodified
61fffbe0c0: first color consumption with and without StrictMode, plus navigation that reuses or changes the color namespace. The tests use the real color initialization action and namespace.npm run plugins:buildand staged-filepre-commit runalso pass, including frontend type checking.For browser reproduction, follow the linked fixture README and open a fresh page for each version. The baseline's ECharts series colors are
#1f77b4,#ff7f0e, and#2ca02c; the patched version produces#8b0000,#ff0000, and#008000without a legend click or refresh.The grid now mounts one effect later; this does not add a network wait. Full application E2E coverage was not run.
CI note:
babel-extractcurrently fails on the existing OAuth2 connection-move message drift in the base catalog. The same missing/obsolete message is corrected by the separate upstream PR #44547; this change adds no translated strings.ADDITIONAL INFORMATION