EnxDev commented on PR #41740: URL: https://github.com/apache/superset/pull/41740#issuecomment-4901174506
## EnxDev's Review Agent โ apache/superset#41740 ยท HEAD 0767674 lgtm โ re-review of new commits; supersedes my [prior review](https://github.com/apache/superset/pull/41740#issuecomment-4889877104) (HEAD d1b6a3d). Both findings I raised there are now resolved and verified in the diff. **Prior findings โ status this HEAD:** - ๐ก *No regression test for the `DashboardBuilder` wiring (the actual fix).* **Resolved.** `DashboardBuilder.test.tsx` now covers all three cases: header hidden (`?standalone=2`) mounts `HeadlessAutoRefresh` and not `DashboardHeader`; header visible mounts neither the headless driver nor drops the header; report mode (`?standalone=3`) mounts neither. Reverting the guard at `DashboardBuilder.tsx:519` now fails a test. - ๐ต *Timer would start during report/thumbnail screenshot renders.* **Resolved correctly.** The guard is now `hideDashboardHeader && !isReport`, so report mode (`DashboardStandaloneMode.Report` = `standalone=3`) starts no timer โ preserving master's behavior rather than firing a live refresh mid-capture. **Bot comments also addressed:** `timedRefreshImmuneSlices` is now `useMemo`-wrapped over the raw selector value (matches the header's memoization at `Header/index.tsx:335-337`, so no per-render `[]` churn that would keep resetting the timer), and `HeadlessAutoRefresh.test.tsx` now asserts `onRefresh` / `setRefreshFrequency` / `logEvent` come through as functions. **Behavioral equivalence (re-verified against the header path):** all nine props to `useHeaderAutoRefresh` are computed identically in `HeadlessAutoRefresh.tsx` and `Header/index.tsx` โ `useChartIds()`, `dashboardInfo.id`, `refreshFrequency ?? 0`, the memoized `metadata?.timed_refresh_immune_slices || []`, `common?.conf?.DASHBOARD_AUTO_REFRESH_MODE`, the `start > end` `isLoading` selector (`index.tsx:283-288`), and the `onRefresh` / `setRefreshFrequency` / `logEvent` action creators. Header (`!hideDashboardHeader`) and headless (`hideDashboardHeader && !isReport`) are mutually exclusive, so exactly one timer is ever live and report mode runs none. `AutoRefreshProvider` still wraps `DashboardBuilder`, so the hook's context resolves in the headless path. CI is green (codecov: all modified lines covered). Nothing to change. Clean, minimal fix for #25970. ### ๐ 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. <!-- enxdev-review-agent:0767674 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 0767674._ -- 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]
