You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sadpandajoe (via GitHub)" <gi...@apache.org> on 2026/09/14 23:54:22 UTC
[PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
sadpandajoe opened a new pull request, #44272:
URL: https://github.com/apache/superset/pull/44272
### Summary
A customer-reported issue: when switching between dashboard tabs, a chart that starts loading for
the first time (or is otherwise not part of an in-flight background auto-refresh batch) never
showed its loading spinner, leaving the tile blank until the data arrived.
`AutoRefreshContext.isAutoRefreshing` was a single dashboard-wide boolean, set while *any* silent
auto-refresh cycle was in flight. Every chart on the dashboard read this same global flag to decide
whether to suppress its own loading spinner — including charts that had nothing to do with that
refresh cycle, such as one mounting for the first time when a user switches into a tab.
### Fix
Scope the spinner suppression to the specific chart IDs actually included in the in-flight refresh
batch, instead of the whole dashboard:
- `AutoRefreshContext` now also tracks `autoRefreshingChartIds` alongside the existing
`isAutoRefreshing` boolean (left untouched, since other consumers — the tab-visibility catch-up
refresh and the filters-bar indicator — legitimately need the dashboard-wide signal for their own
purposes).
- Added `useIsChartAutoRefreshing(chartId)`, which checks membership in that batch.
- The dashboard chart tile now calls `useIsChartAutoRefreshing(props.id)` instead of the global
`useIsAutoRefreshing()` when deciding whether to suppress its spinner.
- `useHeaderAutoRefresh` passes the actual filtered chart-id batch into `startAutoRefresh`.
### Testing
- Added a failing test first, reproducing the bug: a chart outside the refresh batch had its
spinner suppressed. Confirmed it failed against the unmodified code before implementing the fix.
- Added regression coverage in `AutoRefreshContext.test.tsx`, `useHeaderAutoRefresh.test.tsx`, and
`Chart.test.tsx` covering batch scoping, the empty-batch default, and clearing on refresh end.
- Full suite for the touched area (`AutoRefreshContext`, `Chart` (dashboard grid), `Tab`,
`FiltersBadge`, `useHeaderAutoRefresh`) passes; lint and typecheck are clean on all touched files.
## Before
## After
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44272:
URL: https://github.com/apache/superset/pull/44272#discussion_r4074866757
##########
superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.test.tsx:
##########
@@ -152,3 +158,23 @@ test('forceRefresh normalizes a negative config value to 0 (unstaggered)', async
expect.objectContaining({ interval: 0 }),
);
});
+
+test('a silent refresh reports only the affected chart ids to startAutoRefresh, not the whole dashboard', async () => {
+ mockStartAutoRefresh.mockClear();
+ mockUseAutoRefreshTabPause.mockClear();
+ const { props } = renderHeaderAutoRefresh(
+ {},
+ { chartIds: [1, 2], timedRefreshImmuneSlices: [2] },
+ );
+
+ const { onRefresh: handleTabVisibilityRefresh } =
+ mockUseAutoRefreshTabPause.mock.calls[0][0];
+
+ await act(async () => {
+ await handleTabVisibilityRefresh();
+ });
+
+ expect(props.onRefresh).toHaveBeenCalledTimes(1);
+ expect(mockStartAutoRefresh).toHaveBeenCalledWith([1]);
+ expect(mockStartAutoRefresh).not.toHaveBeenCalledWith([1, 2]);
+});
Review Comment:
<!-- Bito Reply -->
The test environment configuration in this project, specifically the use of `jsdom` with `pretendToBeVisual`, provides a native `requestAnimationFrame` implementation. Since the test suite inherits this environment, the additional mock is not required for the test to resolve correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44272:
URL: https://github.com/apache/superset/pull/44272#issuecomment-5672504120
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #44272:
URL: https://github.com/apache/superset/pull/44272#discussion_r4087136281
##########
superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.test.tsx:
##########
@@ -152,3 +158,23 @@ test('forceRefresh normalizes a negative config value to 0 (unstaggered)', async
expect.objectContaining({ interval: 0 }),
);
});
+
+test('a silent refresh reports only the affected chart ids to startAutoRefresh, not the whole dashboard', async () => {
+ mockStartAutoRefresh.mockClear();
+ mockUseAutoRefreshTabPause.mockClear();
+ const { props } = renderHeaderAutoRefresh(
+ {},
+ { chartIds: [1, 2], timedRefreshImmuneSlices: [2] },
Review Comment:
Good idea, added. There is a third chart in the test store with an empty `latestQueryFormData` (the unvisited-tab case), and a new test asserting both `startAutoRefresh` and `onRefresh` only see `[1]` when `2` is immune and `3` has never been queried. Thanks for the nudge!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #44272:
URL: https://github.com/apache/superset/pull/44272
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44272:
URL: https://github.com/apache/superset/pull/44272#issuecomment-5672601140
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44272?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.18%. Comparing base ([`eeafd65`](https://app.codecov.io/gh/apache/superset/commit/eeafd65b746c8cff1c424906d385fe911175661b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2ab4222`](https://app.codecov.io/gh/apache/superset/commit/2ab4222ee42d736f3466d2da746f31c0eb55aeff?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 17 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44272 +/- ##
=======================================
Coverage 80.18% 80.18%
=======================================
Files 2925 2925
Lines 172692 172698 +6
Branches 40090 40091 +1
=======================================
+ Hits 138474 138485 +11
+ Misses 31623 31618 -5
Partials 2595 2595
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44272/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/44272/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.68% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44272?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #44272:
URL: https://github.com/apache/superset/pull/44272#discussion_r4074864552
##########
superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.test.tsx:
##########
@@ -152,3 +158,23 @@ test('forceRefresh normalizes a negative config value to 0 (unstaggered)', async
expect.objectContaining({ interval: 0 }),
);
});
+
+test('a silent refresh reports only the affected chart ids to startAutoRefresh, not the whole dashboard', async () => {
+ mockStartAutoRefresh.mockClear();
+ mockUseAutoRefreshTabPause.mockClear();
+ const { props } = renderHeaderAutoRefresh(
+ {},
+ { chartIds: [1, 2], timedRefreshImmuneSlices: [2] },
Review Comment:
Could we also cover a chart that's in `chartIds` but has an empty `latestQueryFormData`, as an unvisited tab would? This case checks the immune-slice filter, while the first-load fix also relies on the later filter in `executeRefresh`. A third chart with no previous query data, asserting that both `startAutoRefresh` and `onRefresh` receive only the eligible IDs, would protect that part of the reported scenario. Non-blocking; the implementation already handles it.
##########
superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.test.tsx:
##########
@@ -152,3 +158,23 @@ test('forceRefresh normalizes a negative config value to 0 (unstaggered)', async
expect.objectContaining({ interval: 0 }),
);
});
+
+test('a silent refresh reports only the affected chart ids to startAutoRefresh, not the whole dashboard', async () => {
+ mockStartAutoRefresh.mockClear();
+ mockUseAutoRefreshTabPause.mockClear();
+ const { props } = renderHeaderAutoRefresh(
+ {},
+ { chartIds: [1, 2], timedRefreshImmuneSlices: [2] },
+ );
+
+ const { onRefresh: handleTabVisibilityRefresh } =
+ mockUseAutoRefreshTabPause.mock.calls[0][0];
+
+ await act(async () => {
+ await handleTabVisibilityRefresh();
+ });
+
+ expect(props.onRefresh).toHaveBeenCalledTimes(1);
+ expect(mockStartAutoRefresh).toHaveBeenCalledWith([1]);
+ expect(mockStartAutoRefresh).not.toHaveBeenCalledWith([1, 2]);
+});
Review Comment:
I checked this at 2ab4222 with Node 24: the new test completes, and all five tests in `useHeaderAutoRefresh.test.tsx` pass without a RAF mock. The Jest jsdom environment enables `pretendToBeVisual`, which supplies `requestAnimationFrame`; the custom environment inherits that setup. I don't think we need an additional mock for this test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44272:
URL: https://github.com/apache/superset/pull/44272#issuecomment-5769835661
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44272:
URL: https://github.com/apache/superset/pull/44272#issuecomment-5672518337
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 2ab4222ee42d736f3466d2da746f31c0eb55aeff |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa8892ee5faba000806b874 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44272--superset-docs-preview.netlify.app](https://deploy-preview-44272--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctBCoQwDADAv-RsoMiq1R_sMxJbbUDb0hR0Wfy7vc1l_pBLOnOFBaADLhTX0LzJjY40cKLisBKjXlLXgKeoStxRs8ToSzvO5yP9vq6tkcjaufd-2IjJGGPNyHb6wPMC) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44272--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44272:
URL: https://github.com/apache/superset/pull/44272#discussion_r4067765110
##########
superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.test.tsx:
##########
@@ -152,3 +158,23 @@ test('forceRefresh normalizes a negative config value to 0 (unstaggered)', async
expect.objectContaining({ interval: 0 }),
);
});
+
+test('a silent refresh reports only the affected chart ids to startAutoRefresh, not the whole dashboard', async () => {
+ mockStartAutoRefresh.mockClear();
+ mockUseAutoRefreshTabPause.mockClear();
+ const { props } = renderHeaderAutoRefresh(
+ {},
+ { chartIds: [1, 2], timedRefreshImmuneSlices: [2] },
+ );
+
+ const { onRefresh: handleTabVisibilityRefresh } =
+ mockUseAutoRefreshTabPause.mock.calls[0][0];
+
+ await act(async () => {
+ await handleTabVisibilityRefresh();
+ });
+
+ expect(props.onRefresh).toHaveBeenCalledTimes(1);
+ expect(mockStartAutoRefresh).toHaveBeenCalledWith([1]);
+ expect(mockStartAutoRefresh).not.toHaveBeenCalledWith([1, 2]);
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test hangs on missing RAF</b></div>
<div id="fix">
This test drives the `suppressSpinners=true` path, which calls `requestAnimationFrame` (useHeaderAutoRefresh.ts:207) to settle the refresh promise. jsdom provides no RAF and the repo's test setup has no polyfill, so `await handleTabVisibilityRefresh()` never resolves and the test times out. The sibling `Header.test.tsx` (742-746) mocks `window.requestAnimationFrame` for exactly this path; this test needs the same mock.
</div>
</div>
<small><i>Code Review Run #fe6cc3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): show loading spinner for charts outside an in-flight auto-refresh batch [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44272:
URL: https://github.com/apache/superset/pull/44272#issuecomment-5804847723
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org