sadpandajoe commented on code in PR #45589: URL: https://github.com/apache/superset/pull/45589#discussion_r4410650519
########## superset-frontend/src/pages/Home/Home.refresh.test.tsx: ########## @@ -0,0 +1,252 @@ +/** + * 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. + */ +// Imported first so the antd mock is defined before anything requires 'antd'. +import { mockAntdWithDesktopBreakpoint } from 'spec/helpers/mobileTestUtils'; +import fetchMock from 'fetch-mock'; +import { SupersetClient } from '@superset-ui/core'; +import { + render, + screen, + userEvent, + waitFor, +} from 'spec/helpers/testing-library'; +import Welcome from 'src/pages/Home'; +import { LocalStorageKeys, setItem } from 'src/utils/localStorageHelpers'; + +const chartsEndpoint = 'glob:*/api/v1/chart/?*'; +const dashboardsEndpoint = 'glob:*/api/v1/dashboard/?*'; +const savedQueryEndpoint = 'glob:*/api/v1/saved_query/?*'; +const recentActivityEndpoint = 'glob:*/api/v1/log/recent_activity/*'; + +jest.mock('antd', () => mockAntdWithDesktopBreakpoint()); + +// The delete flow lives in the tables; Home only receives `onActivityRefresh`. +// Stub the tables with a button that simulates a completed delete. +jest.mock('src/features/home/DashboardTable', () => ({ + __esModule: true, + default: ({ onActivityRefresh }: { onActivityRefresh?: () => void }) => ( + <button type="button" onClick={onActivityRefresh}> + simulate delete + </button> + ), +})); +jest.mock('src/features/home/ChartTable', () => ({ + __esModule: true, + default: () => <div />, +})); + +const user = { + username: 'alpha', + firstName: 'alpha', + lastName: 'alpha', + createdOn: '2016-11-11T12:34:17', + userId: 5, + email: '[email protected]', + isActive: true, + isAnonymous: false, + permissions: {}, + roles: { sql_lab: [['can_read', 'SavedQuery']] }, +}; + +const recentItem = (title: string, id: number) => ({ + action: 'dashboard', + item_title: title, + item_type: 'dashboard', + item_url: `/superset/dashboard/${id}/`, + time: 1741644942130 + id, + time_delta_humanized: 'a day ago', +}); + +const editedDashboard = (title: string, id: number) => ({ + id, + dashboard_title: title, + url: `/superset/dashboard/${id}/`, + changed_on_delta_humanized: 'a day ago', +}); + +let recentResponder: () => unknown; +let editedDashboards: ReturnType<typeof editedDashboard>[] = []; + +beforeAll(() => { + SupersetClient.configure().init(); +}); + +beforeEach(() => { + localStorage.clear(); + // Start with the Recents and Dashboards panels expanded. + setItem(LocalStorageKeys.HomepageCollapseState, ['recents', 'dashboards']); + fetchMock.get(chartsEndpoint, { result: [] }); + editedDashboards = []; + fetchMock.get(dashboardsEndpoint, ({ url }) => + url.includes('changed_by') ? { result: editedDashboards } : { result: [] }, + ); + fetchMock.get(savedQueryEndpoint, { result: [] }); + fetchMock.get(recentActivityEndpoint, () => recentResponder()); +}); + +afterEach(() => { + fetchMock.removeRoutes(); + fetchMock.clearHistory(); +}); + +const renderHome = async () => { + render(<Welcome user={user} />, { useRedux: true, useRouter: true }); + return screen.findByRole('button', { name: 'simulate delete' }); +}; + +test('delete refetches recents without resetting the selected tab', async () => { + recentResponder = () => ({ result: [recentItem('Viewed dash', 1)] }); + const deleteButton = await renderHome(); + expect(await screen.findByText('Viewed dash')).toBeInTheDocument(); + expect(fetchMock.callHistory.calls(recentActivityEndpoint)).toHaveLength(1); + const dashboardCallsBefore = + fetchMock.callHistory.calls(dashboardsEndpoint).length; + + // Switch away from the default (Viewed) tab; a refresh must not undo this. + await userEvent.click(screen.getByText('Created')); + await waitFor(() => + expect(screen.queryByText('Viewed dash')).not.toBeInTheDocument(), + ); + + // Drop the persisted tab so only the in-memory selection survives; a + // refresh that re-ran the first-load tab restore would flip back to Viewed. + localStorage.removeItem(LocalStorageKeys.HomepageActivityFilter); + + recentResponder = () => ({ result: [recentItem('Fresh dash', 2)] }); + await userEvent.click(deleteButton); + + await waitFor(() => + expect(fetchMock.callHistory.calls(recentActivityEndpoint)).toHaveLength(2), + ); + await waitFor(() => + expect( + fetchMock.callHistory.calls(dashboardsEndpoint).length, + ).toBeGreaterThan(dashboardCallsBefore), + ); + // Still on the Created tab: neither the old nor the refreshed Viewed items + // are shown, and the collapse panels were not reset. + expect(screen.queryByText('Viewed dash')).not.toBeInTheDocument(); + expect(screen.queryByText('Fresh dash')).not.toBeInTheDocument(); + expect( + screen.getByRole('button', { name: 'simulate delete' }), + ).toBeInTheDocument(); + + await userEvent.click(screen.getByText('Viewed')); + expect(await screen.findByText('Fresh dash')).toBeInTheDocument(); +}); + +test('an older in-flight refresh cannot repopulate a deleted item', async () => { + recentResponder = () => ({ + result: [recentItem('Doomed dash', 1), recentItem('Kept dash', 2)], + }); + const deleteButton = await renderHome(); + expect(await screen.findByText('Doomed dash')).toBeInTheDocument(); + + // Defer the next two recents responses so they can resolve out of order. + const resolvers: Array<(value: unknown) => void> = []; + recentResponder = () => + new Promise(resolve => { + resolvers.push(resolve); + }); + + await userEvent.click(deleteButton); + await waitFor(() => expect(resolvers).toHaveLength(1)); + await userEvent.click(deleteButton); + await waitFor(() => expect(resolvers).toHaveLength(2)); + + // The newer refresh (post-delete data) resolves first... + resolvers[1]({ result: [recentItem('Kept dash', 2)] }); + await waitFor(() => + expect(screen.queryByText('Doomed dash')).not.toBeInTheDocument(), + ); + expect(screen.getByText('Kept dash')).toBeInTheDocument(); + + // ...then the older, stale response arrives and must be discarded. + resolvers[0]({ + result: [recentItem('Doomed dash', 1), recentItem('Kept dash', 2)], + }); + await new Promise(resolve => { + setTimeout(resolve, 50); Review Comment: Resolving the older recents response kicks off further chart/dashboard requests before Home gets its result, and a fixed 50 ms sleep doesn't prove that chain and the React updates have finished. If the `isStale` guard regressed, this negative assertion could still pass before "Doomed dash" is repopulated. Could this drain the pending requests inside an async `act` (or wait on the request counts) before asserting the item stays absent? ########## superset-frontend/src/pages/Home/index.tsx: ########## @@ -214,28 +219,42 @@ function Welcome({ user, addDangerToast }: WelcomeProps) { useEffect(() => { if (!otherTabFilters || WelcomeMainExtension) { - return; + return undefined; } + // Guards against out-of-order responses: if `refreshKey` changes again + // (e.g. a rapid follow-up delete) before this run's requests resolve, + // its results are discarded instead of overwriting newer state. + let isStale = false; const activeTab = getItem(LocalStorageKeys.HomepageActivityFilter, null); - setActiveState(collapseState.length > 0 ? collapseState : DEFAULT_TAB_ARR); + // Collapse state and the initial tab are only restored on first load, so + // later refreshes update the data without overriding user choices. + const isInitialLoad = refreshKey === 0; + if (isInitialLoad) { + setActiveState( + collapseState.length > 0 ? collapseState : DEFAULT_TAB_ARR, + ); + } getRecentActivityObjs(user.userId!, recent, addDangerToast, otherTabFilters) .then(res => { + if (isStale) return; const data: ActivityData | null = {}; data[TableTab.Other] = res.other; if (res.viewed) { const filtered = reject(res.viewed, ['item_url', null]).map(r => r); data[TableTab.Viewed] = filtered; - if (!activeTab && data[TableTab.Viewed]) { - setActiveChild(TableTab.Viewed); - } else if (!activeTab && !data[TableTab.Viewed]) { - setActiveChild(TableTab.Created); - } else setActiveChild(activeTab || TableTab.Created); - } else if (!activeTab) setActiveChild(TableTab.Created); - else setActiveChild(activeTab); + } + if (isInitialLoad) { + if (res.viewed) { + if (!activeTab) setActiveChild(TableTab.Viewed); + else setActiveChild(activeTab); + } else if (!activeTab) setActiveChild(TableTab.Created); + else setActiveChild(activeTab); + } setActivityData(activityData => ({ ...activityData, ...data })); }) .catch( createErrorHandler((errMsg: unknown) => { + if (isStale) return; Review Comment: The new stale guards on the rejection path and on the editable-object setters (`setDashboardData`, `setChartData`, `setQueryData`) aren't covered by the added tests: the out-of-order test only varies the recents response and never rejects a request. If refresh A is still pending, refresh B renders current items, and A then rejects, dropping this guard would blank B's Viewed items and fire an obsolete error toast, and every test here would still pass. Could a Home-level test defer A, let B succeed, then reject A and assert the current items remain and no danger toast fires (and resolve the editable-object responses out of order the same way)? -- 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]
