sadpandajoe commented on code in PR #45589: URL: https://github.com/apache/superset/pull/45589#discussion_r4416629341
########## 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: The drain helper looks like it hangs on the out-of-order test. `drainPendingWork` awaits `fetchMock.callHistory.flush(true)`, which waits for every in-flight response, but at the first call (after `pending[1].resolve(...)`) `pending[0]` is still unresolved, so the `pending[0].resolve(...)` that follows is never reached. CI shows that test timing out at 20000 ms and the later tests in the file failing at `renderHome`. Could the drain wait on specific request counts (or only be called once all deferred responses are settled) instead of flushing all fetches? ########## 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 rejection and ordering tests use the same `drainPendingWork` before they reject or resolve the older deferred request (lines 264 and 297), so they would block there too and never reach the stale-guard assertions. Until they run green, the `isStale` guards on the rejection path and the setters are still unexercised. Could you fix the drain so these two tests can complete? -- 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]
