diff --git a/e2e/test/scenarios/dashboard/reproductions/12578-prevent-fetching-slow-cards-more-than-one-at-a-time-when-auto-refreshing.cy.spec.js b/e2e/test/scenarios/dashboard/reproductions/12578-prevent-fetching-slow-cards-more-than-one-at-a-time-when-auto-refreshing.cy.spec.js new file mode 100644 index 0000000000000000000000000000000000000000..b3f177f0aaab97192c2954fce8ae896da25bea6a --- /dev/null +++ b/e2e/test/scenarios/dashboard/reproductions/12578-prevent-fetching-slow-cards-more-than-one-at-a-time-when-auto-refreshing.cy.spec.js @@ -0,0 +1,44 @@ +const { SAMPLE_DATABASE } = require("e2e/support/cypress_sample_database"); +const { restore, visitDashboard, popover } = require("e2e/support/helpers"); + +const { ORDERS_ID } = SAMPLE_DATABASE; + +const ORDERS_QUESTION = { + name: "Orders question", + query: { + "source-table": ORDERS_ID, + }, +}; + +describe("issue 12578", () => { + beforeEach(() => { + restore(); + cy.signInAsAdmin(); + }); + + it("should not fetch cards that are still loading when refreshing", () => { + console.log("clock"); + cy.clock(Date.now()); + cy.createQuestionAndDashboard({ questionDetails: ORDERS_QUESTION }).then( + ({ body: { dashboard_id } }) => { + visitDashboard(dashboard_id); + }, + ); + + // Without tick the dashboard header will not load + cy.tick(); + cy.findByLabelText("Auto Refresh").click(); + popover().findByText("1 minute").click(); + + // Mock slow card request + cy.intercept("POST", "/api/dashboard/*/dashcard/*/card/*/query", req => { + req.on("response", res => { + res.setDelay(99999); + }); + }).as("dashcardQuery"); + cy.tick(61 * 1000); + cy.tick(61 * 1000); + + cy.get("@dashcardQuery.all").should("have.length", 1); + }); +}); diff --git a/frontend/src/metabase-types/store/dashboard.ts b/frontend/src/metabase-types/store/dashboard.ts index c560c97231274eb84253d8967f21ae3bb7e7d348..233f5cd4e2c65e1592e62a20510e43c67b4752f8 100644 --- a/frontend/src/metabase-types/store/dashboard.ts +++ b/frontend/src/metabase-types/store/dashboard.ts @@ -56,7 +56,6 @@ export interface DashboardState { parameterValues: Record<ParameterId, ParameterValueOrArray>; loadingDashCards: { - dashcardIds: DashCardId[]; loadingIds: DashCardId[]; loadingStatus: "idle" | "running" | "complete"; startTime: number | null; diff --git a/frontend/src/metabase-types/store/mocks/dashboard.ts b/frontend/src/metabase-types/store/mocks/dashboard.ts index 2bd9096896f9352e01d90e83044f19401aba6f3d..0d632cb6a3c78aae52fe63a21fa8cf6d6eb89a8a 100644 --- a/frontend/src/metabase-types/store/mocks/dashboard.ts +++ b/frontend/src/metabase-types/store/mocks/dashboard.ts @@ -9,7 +9,6 @@ export const createMockDashboardState = ( dashcardData: {}, parameterValues: {}, loadingDashCards: { - dashcardIds: [], loadingIds: [], loadingStatus: "idle", startTime: null, diff --git a/frontend/src/metabase/dashboard/actions/data-fetching.js b/frontend/src/metabase/dashboard/actions/data-fetching.js index c1d1b02c6bb9b55990893743a8e7aa30224f516a..72200e5ef2a162f487a05582011ad8c955ce7d86 100644 --- a/frontend/src/metabase/dashboard/actions/data-fetching.js +++ b/frontend/src/metabase/dashboard/actions/data-fetching.js @@ -98,9 +98,8 @@ function isNewAdditionalSeriesCard(card, dashcard) { const updateLoadingTitle = createThunkAction( SET_DOCUMENT_TITLE, - () => (dispatch, getState) => { + totalCards => (_dispatch, getState) => { const loadingDashCards = getLoadingDashCards(getState()); - const totalCards = loadingDashCards.dashcardIds.length; const loadingComplete = totalCards - loadingDashCards.loadingIds.length; return `${loadingComplete}/${totalCards} loaded`; }, @@ -419,32 +418,69 @@ export const fetchCardData = createThunkAction( }, ); -export const fetchDashboardCardData = createThunkAction( - FETCH_DASHBOARD_CARD_DATA, - options => (dispatch, getState) => { +export const fetchDashboardCardData = + ({ isRefreshing, ...options } = {}) => + (dispatch, getState) => { const dashboard = getDashboardComplete(getState()); const selectedTabId = getSelectedTabId(getState()); - const dashcardIds = []; - const promises = getCurrentTabDashboardCards(dashboard, selectedTabId) - .filter(({ dashcard }) => !isVirtualDashCard(dashcard)) - .map(({ card, dashcard }) => { - dashcardIds.push(dashcard.id); - return dispatch(fetchCardData(card, dashcard, options)).then(() => { - return dispatch(updateLoadingTitle()); - }); + + const loadingIds = getLoadingDashCards(getState()).loadingIds; + const nonVirtualDashcards = getCurrentTabDashboardCards( + dashboard, + selectedTabId, + ).filter(({ dashcard }) => !isVirtualDashCard(dashcard)); + + let nonVirtualDashcardsToFetch = []; + if (isRefreshing) { + nonVirtualDashcardsToFetch = nonVirtualDashcards.filter( + ({ dashcard }) => { + return !loadingIds.includes(dashcard.id); + }, + ); + const newLoadingIds = nonVirtualDashcardsToFetch.map(({ dashcard }) => { + return dashcard.id; }); - dispatch(setDocumentTitle(t`0/${promises.length} loaded`)); + dispatch({ + type: FETCH_DASHBOARD_CARD_DATA, + payload: { + currentTime: performance.now(), + loadingIds: loadingIds.concat(newLoadingIds), + }, + }); + } else { + nonVirtualDashcardsToFetch = nonVirtualDashcards; + const newLoadingIds = nonVirtualDashcardsToFetch.map(({ dashcard }) => { + return dashcard.id; + }); - // XXX: There is a race condition here, when refreshing a dashboard before - // the previous API calls finished. - Promise.all(promises).then(() => { - dispatch(loadingComplete()); + dispatch({ + type: FETCH_DASHBOARD_CARD_DATA, + payload: { + currentTime: performance.now(), + loadingIds: newLoadingIds, + }, + }); + } + + const promises = nonVirtualDashcardsToFetch.map(({ card, dashcard }) => { + return dispatch(fetchCardData(card, dashcard, options)).then(() => { + return dispatch(updateLoadingTitle(nonVirtualDashcardsToFetch.length)); + }); }); - return { currentTime: performance.now(), dashcardIds }; - }, -); + if (nonVirtualDashcardsToFetch.length > 0) { + dispatch( + setDocumentTitle(t`0/${nonVirtualDashcardsToFetch.length} loaded`), + ); + + // TODO: There is a race condition here, when refreshing a dashboard before + // the previous API calls finished. + Promise.all(promises).then(() => { + dispatch(loadingComplete()); + }); + } + }; export const fetchDashboardCardMetadata = createThunkAction( FETCH_DASHBOARD_CARD_METADATA, diff --git a/frontend/src/metabase/dashboard/components/RefreshWidget.jsx b/frontend/src/metabase/dashboard/components/RefreshWidget.jsx index eb1be0c96a18e3a2048b8e2597d78bd60ba202ef..5cbc236ab0707390ca7fdc175d973a2ac08ce1ea 100644 --- a/frontend/src/metabase/dashboard/components/RefreshWidget.jsx +++ b/frontend/src/metabase/dashboard/components/RefreshWidget.jsx @@ -59,7 +59,10 @@ export default class RefreshWidget extends Component { triggerElement={ elapsed == null ? ( <Tooltip tooltip={t`Auto-refresh`}> - <DashboardHeaderButton icon="clock" /> + <DashboardHeaderButton + icon="clock" + aria-label={t`Auto Refresh`} + /> </Tooltip> ) : ( <Tooltip @@ -80,6 +83,7 @@ export default class RefreshWidget extends Component { percent={Math.min(0.95, (period - elapsed) / period)} /> } + aria-label={t`Auto Refresh`} /> </Tooltip> ) @@ -117,7 +121,6 @@ const RefreshOption = ({ name, period, selected, onClick }) => ( onClick={onClick} > <RefreshOptionIcon name="check" /> - <span>{name.split(" ")[0]}</span> - <span>{name.split(" ")[1]}</span> + <span>{name}</span> </RefreshOptionItem> ); diff --git a/frontend/src/metabase/dashboard/constants.ts b/frontend/src/metabase/dashboard/constants.ts index 005d6cfb64f70be8bdf710e68155a262e13ac0e3..d81a9e4c39f8df2d3c461c598d0a1028627a9181 100644 --- a/frontend/src/metabase/dashboard/constants.ts +++ b/frontend/src/metabase/dashboard/constants.ts @@ -19,7 +19,6 @@ export const INITIAL_DASHBOARD_STATE: DashboardState = { dashcardData: {}, parameterValues: {}, loadingDashCards: { - dashcardIds: [], loadingIds: [], loadingStatus: "idle" as const, startTime: null, diff --git a/frontend/src/metabase/dashboard/hoc/DashboardControls.jsx b/frontend/src/metabase/dashboard/hoc/DashboardControls.jsx index de8233fc1d20210bf6876b7dadeeb4e9f97da575..d6ab6bb86f77583a840ccf5728eff6761c6ffdf2 100644 --- a/frontend/src/metabase/dashboard/hoc/DashboardControls.jsx +++ b/frontend/src/metabase/dashboard/hoc/DashboardControls.jsx @@ -174,6 +174,7 @@ export default ComposedComponent => { preserveParameters: true }, ); this.props.fetchDashboardCardData({ + isRefreshing: true, reload: true, clearCache: false, }); diff --git a/frontend/src/metabase/dashboard/reducers.js b/frontend/src/metabase/dashboard/reducers.js index 9836f60827eeee2b7614b7aa98cab02c5250f248..27f3adf83dc788a1db9d26a538554c2843fffdf8 100644 --- a/frontend/src/metabase/dashboard/reducers.js +++ b/frontend/src/metabase/dashboard/reducers.js @@ -43,7 +43,6 @@ import { UNDO_REMOVE_CARD_FROM_DASH, SHOW_AUTO_APPLY_FILTERS_TOAST, tabsReducer, - SET_LOADING_DASHCARDS_COMPLETE, } from "./actions"; import { syncParametersAndEmbeddingParams } from "./utils"; import { INITIAL_DASHBOARD_STATE } from "./constants"; @@ -372,12 +371,9 @@ const loadingDashCards = handleActions( }), }, [FETCH_DASHBOARD_CARD_DATA]: { - next: (state, { payload: { currentTime, dashcardIds } }) => { - const loadingIds = Array.isArray(dashcardIds) ? dashcardIds : []; - + next: (state, { payload: { currentTime, loadingIds } }) => { return { ...state, - dashcardIds: loadingIds, loadingIds, loadingStatus: loadingIds.length > 0 ? "running" : "idle", startTime: loadingIds.length > 0 ? currentTime : null, @@ -406,15 +402,6 @@ const loadingDashCards = handleActions( }; }, }, - [SET_LOADING_DASHCARDS_COMPLETE]: { - next: state => { - return { - ...state, - loadingIds: [], - loadingStatus: "complete", - }; - }, - }, [RESET]: { next: state => ({ ...state, diff --git a/frontend/src/metabase/dashboard/reducers.unit.spec.js b/frontend/src/metabase/dashboard/reducers.unit.spec.js index 39b100b07b11353244dc34f661edc66a72c733f5..e1ebbc35cef2f02688ac1714e2a285c77f77161d 100644 --- a/frontend/src/metabase/dashboard/reducers.unit.spec.js +++ b/frontend/src/metabase/dashboard/reducers.unit.spec.js @@ -27,7 +27,6 @@ describe("dashboard reducers", () => { isNavigatingBackToDashboard: false, isEditing: null, loadingDashCards: { - dashcardIds: [], loadingIds: [], startTime: null, endTime: null, @@ -234,7 +233,6 @@ describe("dashboard reducers", () => { it("should change to running when loading cards", () => { const dashcardIds = [1, 2, 3]; const loadingMatch = { - dashcardIds: dashcardIds, loadingIds: dashcardIds, loadingStatus: "running", startTime: expect.any(Number), @@ -245,13 +243,12 @@ describe("dashboard reducers", () => { { ...initState, loadingDashCards: { - dashcardIds: dashcardIds, loadingIds: dashcardIds, }, }, { type: FETCH_DASHBOARD_CARD_DATA, - payload: { currentTime: 100, dashcardIds }, + payload: { currentTime: 100, loadingIds: dashcardIds }, }, ), ).toMatchObject({ @@ -264,12 +261,11 @@ describe("dashboard reducers", () => { expect( reducer(initState, { type: FETCH_DASHBOARD_CARD_DATA, - payload: {}, + payload: { currentTime: 100, loadingIds: [] }, }), ).toEqual({ ...initState, loadingDashCards: { - dashcardIds: [], loadingIds: [], loadingStatus: "idle", startTime: null, @@ -284,7 +280,6 @@ describe("dashboard reducers", () => { { ...initState, loadingDashCards: { - dashcardIds: [1, 2, 3], loadingIds: [3], loadingStatus: "running", startTime: 100, @@ -303,7 +298,6 @@ describe("dashboard reducers", () => { ).toEqual({ ...initState, loadingDashCards: { - dashcardIds: [1, 2, 3], loadingIds: [], loadingStatus: "complete", startTime: 100,