codeant-ai-for-open-source[bot] commented on code in PR #41740: URL: https://github.com/apache/superset/pull/41740#discussion_r3518821756
########## superset-frontend/src/dashboard/components/Header/HeadlessAutoRefresh.tsx: ########## @@ -0,0 +1,87 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useMemo } from 'react'; +import { bindActionCreators } from 'redux'; +import { useDispatch, useSelector } from 'react-redux'; +import { RootState } from 'src/dashboard/types'; +import { useChartIds } from 'src/dashboard/util/charts/useChartIds'; +import { onRefresh, setRefreshFrequency } from '../../actions/dashboardState'; +import { logEvent } from '../../../logger/actions'; +import { useHeaderAutoRefresh } from './useHeaderAutoRefresh'; + +/** + * Headless component that drives the dashboard auto-refresh timer when the + * dashboard header is not rendered (e.g. standalone mode or `hideTitle: true` + * in embedded dashboards). The auto-refresh logic lives in the header, so + * hiding the header would otherwise stop the refresh interval from ever + * starting. Rendering this component keeps the timer running independently of + * header visibility. It renders nothing. + */ +const HeadlessAutoRefresh = (): null => { + const dispatch = useDispatch(); + const chartIds = useChartIds(); + const dashboardId = useSelector((state: RootState) => state.dashboardInfo.id); + const refreshFrequency = useSelector( + (state: RootState) => state.dashboardState.refreshFrequency ?? 0, + ); + const timedRefreshImmuneSlices = useSelector( + (state: RootState) => + state.dashboardInfo.metadata?.timed_refresh_immune_slices || [], + ); Review Comment: **Suggestion:** The fallback `[]` here creates a new array reference on every render when `timed_refresh_immune_slices` is missing, which makes downstream hook dependencies look changed each time and repeatedly restarts the periodic timer. Reuse a stable empty array (or memoize like the header implementation) so auto-refresh scheduling is not reset on unrelated rerenders. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Hidden-header dashboards auto-refresh timer unstable and frequently reset. - ⚠️ Charts in standalone or embedded modes may not refresh. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `DashboardBuilder.tsx` (superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx, lines 10-15 and 17-21), note that `hideDashboardHeader` is set true when `uiConfig.hideTitle` is true, or `standaloneMode === DashboardStandaloneMode.HideNavAndTitle`, or in report mode, and that `headerContent` renders <HeadlessAutoRefresh /> instead of <DashboardHeader /> when `hideDashboardHeader` is true. 2. Open a dashboard in one of these header-hidden modes (e.g. `?standalone=2` or embedded with `dashboardUiConfig: { hideTitle: true }`) and configure a non-zero auto-refresh interval; in this state, `HeadlessAutoRefresh` runs and selects `timedRefreshImmuneSlices` via `useSelector` in `HeadlessAutoRefresh.tsx` line 43, `(state: RootState) => state.dashboardInfo.metadata?.timed_refresh_immune_slices || []`. 3. From `Header/index.tsx` (superset-frontend/src/dashboard/components/Header/index.tsx, lines 132-135), observe that `timed_refresh_immune_slices?: number[]` is an optional field on `dashboardInfo.metadata`, so for dashboards without this metadata key the selector in `HeadlessAutoRefresh.tsx` returns a freshly allocated `[]` on every render; React-Redux compares selector results by reference, so any Redux state change causes `timedRefreshImmuneSlices` to be seen as changed even when metadata is unchanged. 4. In `useHeaderAutoRefresh.ts` (superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.ts, lines 248-63, 92-99, and 120-131), `timedRefreshImmuneSlices` participates in the dependency arrays for `startPeriodicRender`, `handlePauseToggle`, and `handleTabVisibilityRefresh`; each time `timedRefreshImmuneSlices`’ reference changes, these callbacks are recreated, the `useEffect` at lines 66-73 re-runs, `stopPeriodicRender()` clears the existing timeout, and `startPeriodicRender(refreshFrequency * 1000)` schedules a new one. With charts and dashboard state updating frequently in real dashboards, the headless path continually stops and restarts the periodic timer when `timed_refresh_immune_slices` is absent, leading to unstable or effectively stalled auto-refresh intervals in header-hidden views, whereas the header path avoids this by memoizing `timedRefreshImmuneSlices` via `useMemo` (Header/index.tsx lines 94-99). ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ef9d2904d0cc4e86bb73c9f1dcf199fd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ef9d2904d0cc4e86bb73c9f1dcf199fd&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/dashboard/components/Header/HeadlessAutoRefresh.tsx **Line:** 43:46 **Comment:** *Performance: The fallback `[]` here creates a new array reference on every render when `timed_refresh_immune_slices` is missing, which makes downstream hook dependencies look changed each time and repeatedly restarts the periodic timer. Reuse a stable empty array (or memoize like the header implementation) so auto-refresh scheduling is not reset on unrelated rerenders. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41740&comment_hash=4c4ca362f47353e67727164c35b864dadb09e7f5f7c2503020df2561ee3ba4d1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41740&comment_hash=4c4ca362f47353e67727164c35b864dadb09e7f5f7c2503020df2561ee3ba4d1&reaction=dislike'>👎</a> -- 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]
