shelkesays commented on code in PR #40512: URL: https://github.com/apache/superset/pull/40512#discussion_r3486121866
########## superset-frontend/src/dashboard/components/Header/useHeaderAutoRefresh.test.tsx: ########## @@ -0,0 +1,154 @@ +/** + * 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 { renderHook, act } from '@testing-library/react'; +import { Provider } from 'react-redux'; +import { createStore } from 'redux'; +import { ReactNode } from 'react'; +import { LOG_ACTIONS_FORCE_REFRESH_DASHBOARD } from 'src/logger/LogUtils'; +import { useHeaderAutoRefresh } from './useHeaderAutoRefresh'; + +jest.mock('src/dashboard/contexts/AutoRefreshContext', () => ({ + useAutoRefreshContext: () => ({ + startAutoRefresh: jest.fn(), + endAutoRefresh: jest.fn(), + setRefreshInFlight: jest.fn(), + }), +})); + +jest.mock('src/dashboard/hooks/useRealTimeDashboard', () => ({ + useRealTimeDashboard: () => ({ + isPaused: false, + setStatus: jest.fn(), + setPaused: jest.fn(), + setPausedByTab: jest.fn(), + recordSuccess: jest.fn(), + recordError: jest.fn(), + setFetchStartTime: jest.fn(), + autoRefreshPauseOnInactiveTab: false, + setPauseOnInactiveTab: jest.fn(), + }), +})); + +jest.mock('src/dashboard/hooks/useAutoRefreshTabPause', () => ({ + useAutoRefreshTabPause: jest.fn(), +})); + +const createWrapper = (conf: Record<string, unknown> = {}) => { + const store = createStore(() => ({ + charts: { + 1: { latestQueryFormData: { datasource: '1__table' } }, + 2: { latestQueryFormData: { datasource: '2__table' } }, + }, + dashboardInfo: { + common: { conf }, + }, + })); + return ({ children }: { children: ReactNode }) => ( + <Provider store={store}>{children}</Provider> + ); +}; + +const renderHeaderAutoRefresh = ( + conf: Record<string, unknown> = {}, + overrides = {}, +) => { + const props = { + chartIds: [1, 2], + dashboardId: 100, + refreshFrequency: 0, + timedRefreshImmuneSlices: [], + isLoading: false, + onRefresh: jest.fn().mockResolvedValue(undefined), + setRefreshFrequency: jest.fn(), + logEvent: jest.fn(), + ...overrides, + }; + const { result } = renderHook(() => useHeaderAutoRefresh(props), { + wrapper: createWrapper(conf), + }); + return { result, props }; +}; + +test('forceRefresh passes the default stagger interval (5000ms) when no config is provided', async () => { Review Comment: Good catch, you'\''re right that mocking at the hook boundary meant the stagger loop itself was never exercised. I added a test that drives the real `fetchCharts` dispatch with fake timers and asserts the timing: with `interval=4000` and 3 charts the first fires at `t=0`, the second at `t=2000`, and the last at `t=4000`, with nothing in between. It lives next to the other `fetchCharts` tests in `dashboardState.test.ts` since that is where the real dispatch runs. Deleting the stagger loop now fails it. Done in 1bad76a3. ########## docs/docs/faq.mdx: ########## @@ -181,6 +181,20 @@ value in milliseconds in the JSON Metadata field: Here, the entire dashboard will refresh at once if periodic refresh is on. The stagger time of 2.5 seconds is ignored. +The manual **Refresh dashboard** button can also stagger its chart requests, controlled by the +`SUPERSET_DASHBOARD_MANUAL_REFRESH_STAGGER_MS` server config in `superset_config.py`. This defaults +to `0`, which preserves the original behavior where every chart request fires at the same time when +the button is clicked. To opt in to staggering, set a positive number of milliseconds; the window Review Comment: Agreed, that interaction was not documented. I added a line noting that a per-dashboard `stagger_refresh: false` disables staggering on the manual-refresh path too, so those charts fire all at once even when the server config is positive. Done in 1bad76a3. ########## superset/config.py: ########## @@ -215,6 +215,17 @@ def _try_json_readsha(filepath: str, length: int) -> str | None: SUPERSET_DASHBOARD_PERIODICAL_REFRESH_LIMIT = 0 SUPERSET_DASHBOARD_PERIODICAL_REFRESH_WARNING_MESSAGE = None +# Manual dashboard refresh can stagger chart data requests across this many +# milliseconds so they do not all hit the backend at the same instant. This +# defaults to 0, which preserves the original behavior where every chart +# request fires at the same time when the user clicks the Refresh dashboard +# button. Set a positive value to opt in to staggering; the frontend then +# uses the larger of this value and the dashboard's stagger_time metadata. Review Comment: Fair point, the `stagger_time` default was implicit. I noted in both the config comment and the FAQ that `stagger_time` defaults to 5000 ms when not set, so a small config value can still produce a 5 second window. Done in 1bad76a3. -- 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]
