EnxDev commented on PR #41740: URL: https://github.com/apache/superset/pull/41740#issuecomment-4889877104
## EnxDev's Review Agent โ apache/superset#41740 ยท HEAD d1b6a3d comment โ clean, well-scoped fix; behavioral equivalence with the header path verified, but the actual `DashboardBuilder` wiring that constitutes the fix has no regression test. I audited the headless path against how `DashboardHeader` drives the same hook. All nine props passed to `useHeaderAutoRefresh` are computed identically in both places โ `useChartIds()`, `dashboardInfo.id`, `dashboardState.refreshFrequency ?? 0`, the memoized `metadata?.timed_refresh_immune_slices || []`, `common?.conf?.DASHBOARD_AUTO_REFRESH_MODE`, the `start > end` `isLoading` selector, and the `onRefresh`/`setRefreshFrequency`/`logEvent` action creators (same imports from `actions/dashboardState` and `logger/actions`). Only one of the two components ever mounts (ternary), and those are the only two callers of the hook, so no double-firing. `AutoRefreshProvider` wraps `DashboardBuilder`, so `useAutoRefreshContext()` resolves in the headless path. The two earlier bot comments (unstable `[]` reference; incomplete test assertions) are both already addressed at this HEAD. ### ๐ก Should-fix - **`HeadlessAutoRefresh.test.tsx` / `DashboardBuilder.tsx:519`** โ The only new test unit-tests `HeadlessAutoRefresh` in isolation with `useHeaderAutoRefresh` fully mocked, so it verifies prop wiring but not the fix itself. Nothing asserts that `DashboardBuilder` renders `<HeadlessAutoRefresh />` when `hideDashboardHeader` is true โ reverting the ternary at `DashboardBuilder.tsx:519` would keep every test green while reintroducing #25970. **regression test:** render `DashboardBuilder` with the header hidden (`standalone=2` / `uiConfig.hideTitle: true`) and assert `HeadlessAutoRefresh` mounts and `DashboardHeader` does not, plus the inverse for the header-visible case. ### ๐ต Nits - `DashboardBuilder.tsx:427` ยท _Low_ โ `hideDashboardHeader` folds in `isReport`, so the refresh timer now also starts during report/thumbnail screenshot renders, a path that previously ran no timer. Screenshot capture is a one-shot snapshot taken after load, so the first interval typically never fires before capture โ likely benign โ but worth confirming a live timer is wanted in the report path (a dashboard with a short interval + slow charts could trigger an extra fetch mid-capture). ### ๐ Praise - `HeadlessAutoRefresh.tsx` โ Reuses `useHeaderAutoRefresh` verbatim and mirrors the header's prop computation exactly, so pause / tab-visibility / immune-slice behavior can't drift between the visible and hidden paths. Right call over duplicating the refresh logic. <!-- enxdev-review-agent:d1b6a3d --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD d1b6a3d._ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
