From a9e78fd14bc3c9fde7a389d2e6ac9d5dd6ea0527 Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Fri, 17 Jan 2025 15:47:18 -0500 Subject: [PATCH 1/9] fix: call createAppRouter within App render --- src/components/app/App.jsx | 6 ++++-- src/components/app/routes/data/utils.js | 18 ------------------ 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/components/app/App.jsx b/src/components/app/App.jsx index 2ff141af85..c3616283bd 100644 --- a/src/components/app/App.jsx +++ b/src/components/app/App.jsx @@ -63,14 +63,16 @@ const queryClient = new QueryClient({ }, }); -const router = createAppRouter(queryClient); - const App = () => { const [showReactQueryDevtools, setShowReactQueryDevtools] = useState(false); useEffect(() => { window.toggleReactQueryDevtools = () => setShowReactQueryDevtools((prevState) => !prevState); }); + // Create the app router during render vs. at the top-level of the module to ensure + // the logging and auth modules are initialized before the router is created. + const router = createAppRouter(queryClient); + return ( diff --git a/src/components/app/routes/data/utils.js b/src/components/app/routes/data/utils.js index 5ec90b2887..5086e6a291 100644 --- a/src/components/app/routes/data/utils.js +++ b/src/components/app/routes/data/utils.js @@ -1,16 +1,9 @@ import { generatePath, matchPath, redirect } from 'react-router-dom'; import { getConfig } from '@edx/frontend-platform'; import { - AxiosJwtAuthService, - configure as configureAuth, fetchAuthenticatedUser, getLoginRedirectUrl, } from '@edx/frontend-platform/auth'; -import { - configure as configureLogging, - getLoggingService, - NewRelicLoggingService, -} from '@edx/frontend-platform/logging'; import { getProxyLoginUrl } from '@edx/frontend-enterprise-logistration'; import Cookies from 'universal-cookie'; @@ -208,17 +201,6 @@ export function redirectToRemoveTrailingSlash(requestUrl) { throw redirect(requestUrl.pathname.slice(0, -1)); } -// Configure the logging and authentication services, only for non-test environments. -if (process.env.NODE_ENV !== 'test') { - configureLogging(NewRelicLoggingService, { - config: getConfig(), - }); - configureAuth(AxiosJwtAuthService, { - loggingService: getLoggingService(), - config: getConfig(), - }); -} - /** * Ensures that the user is authenticated. If not, redirects to the login page. * @param {URL} requestUrl - The current request URL to redirect back to if the From 208fb17ec9d33bb26dc4538ba387314dd17dc38f Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Fri, 17 Jan 2025 16:24:30 -0500 Subject: [PATCH 2/9] chore: tests --- src/components/app/App.test.jsx | 93 +++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 src/components/app/App.test.jsx diff --git a/src/components/app/App.test.jsx b/src/components/app/App.test.jsx new file mode 100644 index 0000000000..d59b8f0cb3 --- /dev/null +++ b/src/components/app/App.test.jsx @@ -0,0 +1,93 @@ +import { render, screen, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom/extend-expect'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { createMemoryRouter } from 'react-router-dom'; +import { act } from 'react-dom/test-utils'; +import App from './App'; +import { createAppRouter } from './routes'; + +jest.mock('./routes', () => ({ + ...jest.requireActual('./routes'), + createAppRouter: jest.fn(), +})); + +jest.mock('@tanstack/react-query-devtools', () => ({ + ReactQueryDevtools: () =>
ReactQueryDevtools
, +})); + +jest.mock('@tanstack/react-query-devtools/production', () => ({ + ReactQueryDevtools: () =>
ReactQueryDevtoolsProduction
, +})); + +jest.mock('@edx/frontend-platform/react', () => ({ + AppProvider: ({ children }) =>
{children}
, +})); + +describe('App', () => { + let queryClient; + + beforeEach(() => { + queryClient = new QueryClient(); + const mockRouter = createMemoryRouter( + [{ path: '/', element:
Mock Route
}], + { initialEntries: ['/'] }, + ); + createAppRouter.mockReturnValue(mockRouter); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + test('renders App component without errors', () => { + render( + + + , + ); + + expect(screen.getByTestId('app-provider')).toBeInTheDocument(); + }); + + test('toggles ReactQueryDevtoolsProduction visibility', async () => { + render(); + + expect(screen.getByTestId('react-query-devtools')).toBeInTheDocument(); + + // Toggle visibility ON + act(() => { + window.toggleReactQueryDevtools(); + }); + await waitFor(() => { + expect(screen.getByTestId('react-query-devtools-production')).toBeInTheDocument(); + }); + + // Toggle visibility OFF + act(() => { + window.toggleReactQueryDevtools(); + }); + await waitFor(() => { + expect(screen.queryByTestId('react-query-devtools-production')).not.toBeInTheDocument(); + }); + }); + + test('uses the custom router created by createAppRouter', () => { + render(); + + expect(createAppRouter).toHaveBeenCalledWith( + expect.objectContaining({ + defaultOptions: { + queries: expect.objectContaining({ + cacheTime: 1000 * 60 * 30, + keepPreviousData: true, + retry: expect.any(Function), + staleTime: 1000 * 20, + suspense: true, + }), + }, + }), + ); + + expect(screen.getByTestId('mock-route')).toBeInTheDocument(); + }); +}); From 3114e4d9863a6b2442d99ca5ea5628493cb9c658 Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Tue, 21 Jan 2025 17:23:04 -0500 Subject: [PATCH 3/9] chore: updates to use shouldRevalidate --- src/components/app/App.jsx | 16 ++++++------- .../app/data/hooks/useNProgressLoader.js | 4 +++- src/components/app/data/hooks/useNotices.js | 4 +++- src/components/app/data/utils.js | 19 +++++++++++++++ .../app/routes/AppSuspenseFallback.jsx | 23 ------------------- src/components/app/routes/data/utils.js | 14 +++++++++++ src/components/app/routes/index.js | 1 - src/components/search/SearchPage.jsx | 3 +-- src/routes.tsx | 13 +++++++++++ 9 files changed, 60 insertions(+), 37 deletions(-) delete mode 100644 src/components/app/routes/AppSuspenseFallback.jsx diff --git a/src/components/app/App.jsx b/src/components/app/App.jsx index c3616283bd..fca99c60cd 100644 --- a/src/components/app/App.jsx +++ b/src/components/app/App.jsx @@ -1,5 +1,5 @@ import { - Suspense, lazy, useEffect, useState, + Suspense, lazy, useEffect, useMemo, useState, } from 'react'; import { RouterProvider } from 'react-router-dom'; import { AppProvider } from '@edx/frontend-platform/react'; @@ -15,7 +15,7 @@ import { defaultQueryClientRetryHandler, } from '../../utils/common'; -import { AppSuspenseFallback, RouterFallback, createAppRouter } from './routes'; +import { RouterFallback, createAppRouter } from './routes'; // eslint-disable-next-line import/no-unresolved const ReactQueryDevtoolsProduction = lazy(() => import('@tanstack/react-query-devtools/production').then((d) => ({ @@ -71,7 +71,7 @@ const App = () => { // Create the app router during render vs. at the top-level of the module to ensure // the logging and auth modules are initialized before the router is created. - const router = createAppRouter(queryClient); + const router = useMemo(() => createAppRouter(queryClient), []); return ( @@ -82,12 +82,10 @@ const App = () => { )} - }> - } - /> - + } + /> ); diff --git a/src/components/app/data/hooks/useNProgressLoader.js b/src/components/app/data/hooks/useNProgressLoader.js index efaa061be0..50a8bddc27 100644 --- a/src/components/app/data/hooks/useNProgressLoader.js +++ b/src/components/app/data/hooks/useNProgressLoader.js @@ -18,7 +18,9 @@ function useNProgressLoader() { const { data: noticeRedirectUrl, isLoading: isLoadingNotices, - } = useNotices(); + } = useNotices({ + suspense: false, + }); const hasNoticeRedirectUrl = !isLoadingNotices && !!noticeRedirectUrl; const isAppDataHydrated = isAuthenticatedUserHydrated && !hasNoticeRedirectUrl; diff --git a/src/components/app/data/hooks/useNotices.js b/src/components/app/data/hooks/useNotices.js index 821e8bd84d..d61dd2984f 100644 --- a/src/components/app/data/hooks/useNotices.js +++ b/src/components/app/data/hooks/useNotices.js @@ -1,5 +1,6 @@ import { useEffect } from 'react'; import { useQuery } from '@tanstack/react-query'; +import { getConfig } from '@edx/frontend-platform/config'; import { queryNotices } from '../queries'; @@ -10,7 +11,8 @@ import { queryNotices } from '../queries'; function useNotices(queryOptions = {}) { const queryResults = useQuery({ ...queryNotices(), - queryOptions, + enabled: !!getConfig().ENABLE_NOTICES, + ...queryOptions, }); useEffect(() => { diff --git a/src/components/app/data/utils.js b/src/components/app/data/utils.js index 68ddb7f8a4..82669c7524 100644 --- a/src/components/app/data/utils.js +++ b/src/components/app/data/utils.js @@ -503,6 +503,25 @@ export function retrieveErrorMessage(error) { if (error.customAttributes) { return error.customAttributes.httpErrorResponseData; } + // Identify development suspense error + const isDeveloperSuspenseError = ( + process.env.NODE_ENV === 'development' + && ['suspended while rendering', 'no fallback UI was specified'].every( + (str) => error.message.includes(str), + ) + ); + if (isDeveloperSuspenseError) { + const errorMessage = error.message; + return ( + `A component or hook triggered suspense, possibly due to + missing pre-fetched data. Since \`suspense: true\` is configured + by default for all React Query queries, ensure that any necessary + API data accessed via \`useQuery\` is pre-fetched in a route loader + to avoid triggering suspense (loading states) unexpectedly. + Error: + ${errorMessage.split('\n')[0]}` + ); + } return error.message; } diff --git a/src/components/app/routes/AppSuspenseFallback.jsx b/src/components/app/routes/AppSuspenseFallback.jsx deleted file mode 100644 index 1dfee5114a..0000000000 --- a/src/components/app/routes/AppSuspenseFallback.jsx +++ /dev/null @@ -1,23 +0,0 @@ -import { useEffect } from 'react'; -import nprogress from 'accessible-nprogress'; - -import { NPROGRESS_DELAY_MS } from '../data/hooks/useNProgressLoader'; - -export function useNProgressLoaderWithoutRouter() { - useEffect(() => { - const timeoutId = setTimeout(() => { - nprogress.start(); - }, NPROGRESS_DELAY_MS); - return () => { - clearTimeout(timeoutId); - nprogress.done(); - }; - }, []); -} - -const AppSuspenseFallback = () => { - useNProgressLoaderWithoutRouter(); - return null; -}; - -export default AppSuspenseFallback; diff --git a/src/components/app/routes/data/utils.js b/src/components/app/routes/data/utils.js index 5086e6a291..20ea81b26e 100644 --- a/src/components/app/routes/data/utils.js +++ b/src/components/app/routes/data/utils.js @@ -10,6 +10,7 @@ import Cookies from 'universal-cookie'; import { activateOrAutoApplySubscriptionLicense, addLicenseToSubscriptionLicensesByStatus, + queryAcademiesList, queryBrowseAndRequestConfiguration, queryContentHighlightsConfiguration, queryCouponCodeRequests, @@ -109,24 +110,30 @@ export async function ensureEnterpriseAppData({ ); } enterpriseAppDataQueries.push(...[ + // Redeemable Learner Credit Policies queryClient.ensureQueryData( queryRedeemablePolicies({ enterpriseUuid: enterpriseCustomer.uuid, lmsUserId: userId, }), ), + // Enterprise Coupon Codes queryClient.ensureQueryData( queryCouponCodes(enterpriseCustomer.uuid), ), + // Enterprise Learner Offers queryClient.ensureQueryData( queryEnterpriseLearnerOffers(enterpriseCustomer.uuid), ), + // Browse and Request Configuration queryClient.ensureQueryData( queryBrowseAndRequestConfiguration(enterpriseCustomer.uuid), ), + // License Requests queryClient.ensureQueryData( queryLicenseRequests(enterpriseCustomer.uuid, userEmail), ), + // Coupon Code Requests queryClient.ensureQueryData( queryCouponCodeRequests(enterpriseCustomer.uuid, userEmail), ), @@ -134,13 +141,20 @@ export async function ensureEnterpriseAppData({ queryClient.ensureQueryData( queryContentHighlightsConfiguration(enterpriseCustomer.uuid), ), + // Academies List + queryClient.ensureQueryData( + queryAcademiesList(enterpriseCustomer.uuid), + ), ]); if (getConfig().ENABLE_NOTICES) { enterpriseAppDataQueries.push( + // Notices queryClient.ensureQueryData(queryNotices()), ); } + + // Ensure all enterprise app data queries are resolved. await Promise.all(enterpriseAppDataQueries); } diff --git a/src/components/app/routes/index.js b/src/components/app/routes/index.js index 052f479540..cf25703dcb 100644 --- a/src/components/app/routes/index.js +++ b/src/components/app/routes/index.js @@ -1,5 +1,4 @@ export { default as RouterFallback } from './RouterFallback'; -export { default as AppSuspenseFallback } from './AppSuspenseFallback'; export { default as RouteErrorBoundary } from './RouteErrorBoundary'; export { default as createAppRouter } from './createAppRouter'; diff --git a/src/components/search/SearchPage.jsx b/src/components/search/SearchPage.jsx index 1b88a680e3..8121f1e642 100644 --- a/src/components/search/SearchPage.jsx +++ b/src/components/search/SearchPage.jsx @@ -8,9 +8,8 @@ import { features } from '../../config'; import { useHasValidLicenseOrSubscriptionRequestsEnabled } from '../app/data'; const SearchPage = () => { - const hasValidLicenseOrSubRequest = useHasValidLicenseOrSubscriptionRequestsEnabled(); - const intl = useIntl(); + const hasValidLicenseOrSubRequest = useHasValidLicenseOrSubscriptionRequestsEnabled(); const enableVideos = ( features.FEATURE_ENABLE_VIDEO_CATALOG && hasValidLicenseOrSubRequest diff --git a/src/routes.tsx b/src/routes.tsx index f9314201f9..fd849324c1 100644 --- a/src/routes.tsx +++ b/src/routes.tsx @@ -191,6 +191,19 @@ function getEnterpriseSlugRoutes(queryClient?: Types.QueryClient) { loader: getRouteLoader(makeRootLoader, queryClient), element: , children: enterpriseSlugChildRoutes, + shouldRevalidate: ({ currentUrl, nextUrl }) => { + const currentPathname = currentUrl.pathname; + const nextPathname = nextUrl.pathname; + if (currentPathname === nextPathname) { + // If the pathname hasn't changed, we don't need to revalidate. + return false; + } + + // Check if the next pathname is a sub-route of the enterprise slug route. + // If it is, we should revalidate the route; otherwise, it shouldn't revalidate. + const matchedSubRoute = matchPath({ path: ':enterpriseSlug?/*' }, nextPathname); + return !!matchedSubRoute; + }, }, ]; return enterpriseSlugRoutes; From 01d60f446d5130244106b4035ded5176e51215fa Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 22 Jan 2025 07:53:51 -0500 Subject: [PATCH 4/9] fix: disable suspense for useNotices --- .../app/data/hooks/useNProgressLoader.js | 6 ++---- .../app/data/hooks/useNotices.test.jsx | 21 +++++++++++++++++++ src/components/app/routes/RouterFallback.jsx | 7 ++++++- .../routes/loaders/tests/rootLoader.test.jsx | 14 +++++++++++-- 4 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/components/app/data/hooks/useNProgressLoader.js b/src/components/app/data/hooks/useNProgressLoader.js index 50a8bddc27..a3139462aa 100644 --- a/src/components/app/data/hooks/useNProgressLoader.js +++ b/src/components/app/data/hooks/useNProgressLoader.js @@ -10,7 +10,7 @@ import useNotices from './useNotices'; // for quick route transitions. export const NPROGRESS_DELAY_MS = 300; -function useNProgressLoader() { +function useNProgressLoader(queryOptions = {}) { const { authenticatedUser } = useContext(AppContext); const isAuthenticatedUserHydrated = !!authenticatedUser?.extendedProfile; const navigation = useNavigation(); @@ -18,9 +18,7 @@ function useNProgressLoader() { const { data: noticeRedirectUrl, isLoading: isLoadingNotices, - } = useNotices({ - suspense: false, - }); + } = useNotices(queryOptions.useNotices); const hasNoticeRedirectUrl = !isLoadingNotices && !!noticeRedirectUrl; const isAppDataHydrated = isAuthenticatedUserHydrated && !hasNoticeRedirectUrl; diff --git a/src/components/app/data/hooks/useNotices.test.jsx b/src/components/app/data/hooks/useNotices.test.jsx index c4f9ca01ce..b31bacfaa2 100644 --- a/src/components/app/data/hooks/useNotices.test.jsx +++ b/src/components/app/data/hooks/useNotices.test.jsx @@ -1,5 +1,6 @@ import { QueryClientProvider } from '@tanstack/react-query'; import { renderHook } from '@testing-library/react-hooks'; +import { getConfig } from '@edx/frontend-platform/config'; import { queryClient } from '../../../../utils/tests'; import useNotices from './useNotices'; @@ -10,6 +11,11 @@ jest.mock('../services', () => ({ fetchNotices: jest.fn().mockResolvedValue(null), })); +jest.mock('@edx/frontend-platform/config', () => ({ + ...jest.requireActual('@edx/frontend-platform/config'), + getConfig: jest.fn(), +})); + const wrapper = ({ children }) => ( {children} @@ -31,6 +37,21 @@ describe('useNotices', () => { beforeEach(() => { jest.clearAllMocks(); + getConfig.mockReturnValue({ ENABLE_NOTICES: true }); + }); + + it('should do nothing with notices disabled', () => { + getConfig.mockReturnValue({ ENABLE_NOTICES: false }); + + const { result } = renderHook(() => useNotices(), { wrapper }); + + expect(result.current).toEqual( + expect.objectContaining({ + data: undefined, + isLoading: true, + isFetching: false, + }), + ); }); it('should return null and NOT redirect with no notices', async () => { diff --git a/src/components/app/routes/RouterFallback.jsx b/src/components/app/routes/RouterFallback.jsx index ee7c4cb50a..63b06aee70 100644 --- a/src/components/app/routes/RouterFallback.jsx +++ b/src/components/app/routes/RouterFallback.jsx @@ -1,7 +1,12 @@ import { useNProgressLoader } from '../data'; const RouterFallback = () => { - useNProgressLoader(); + const queryOptions = { + useNotices: { + suspense: false, + }, + }; + useNProgressLoader(queryOptions); return null; }; diff --git a/src/components/app/routes/loaders/tests/rootLoader.test.jsx b/src/components/app/routes/loaders/tests/rootLoader.test.jsx index 444e9ca2e8..4671112586 100644 --- a/src/components/app/routes/loaders/tests/rootLoader.test.jsx +++ b/src/components/app/routes/loaders/tests/rootLoader.test.jsx @@ -9,6 +9,7 @@ import { activateOrAutoApplySubscriptionLicense, extractEnterpriseCustomer, getBaseSubscriptionsData, + queryAcademiesList, queryBrowseAndRequestConfiguration, queryContentHighlightsConfiguration, queryCouponCodeRequests, @@ -299,13 +300,13 @@ describe('rootLoader', () => { await waitFor(() => { // Assert that the expected number of queries were made. - let expectedQueryCount = 9; + let expectedQueryCount = 10; if (enterpriseSlug !== activeEnterpriseCustomer.slug) { if (!(isLinked || isStaffUser)) { expectedQueryCount = 2; } } else if (hasResolvedBFFQuery) { - expectedQueryCount = 8; + expectedQueryCount = 9; } expect(mockQueryClient.ensureQueryData).toHaveBeenCalledTimes(expectedQueryCount); }); @@ -430,5 +431,14 @@ describe('rootLoader', () => { queryFn: expect.any(Function), }), ); + + // Academies list query + const academiesListQuery = queryAcademiesList(enterpriseCustomer.uuid); + expect(mockQueryClient.ensureQueryData).toHaveBeenCalledWith( + expect.objectContaining({ + queryKey: academiesListQuery.queryKey, + queryFn: expect.any(Function), + }), + ); }); }); From 0bec2a8f112a831311c8f0708e4ca1118f0cf6c0 Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 22 Jan 2025 09:09:41 -0500 Subject: [PATCH 5/9] docs: add ADR about shouldRevalidate on rootLoader --- .../decisions/0014-rootLoader-revalidation.md | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 docs/decisions/0014-rootLoader-revalidation.md diff --git a/docs/decisions/0014-rootLoader-revalidation.md b/docs/decisions/0014-rootLoader-revalidation.md new file mode 100644 index 0000000000..139505c253 --- /dev/null +++ b/docs/decisions/0014-rootLoader-revalidation.md @@ -0,0 +1,54 @@ +# 0014. Triggering Revalidation on the `rootLoader` Route Loader on Sub-Route Navigation + +## Status + +Accepted (January 2025) + +## Context + +In this route-based application, the `rootLoader` is responsible for pre-fetching common queries needed across multiple routes. However, when navigating between sub-routes (e.g., from the Dashboard to the Search page), the `rootLoader` doesn’t re-execute. This becomes a problem when a component in the target route accesses query data with `useQuery` that hasn’t already been pre-fetched by a loader. + +By default, this issue is masked due to the query client configuration where queries have `suspense: true`. In these cases, if a query hasn’t been pre-fetched in a route loader, the `useQuery` hook causes the component to suspend, triggering the `AppSuspenseFallback` component while the query resolves asynchronously. This fallback behavior hides the underlying problem, making it harder to identify when query data is being accessed without pre-fetching. + +## Decision + +To ensure data consistency and explicitly address the issue of missing pre-fetched queries, two key changes will be implemented: + +### 1. Remove the Parent `Suspense` and `AppSuspenseFallback` Component + +The parent `Suspense` boundary and its fallback (`AppSuspenseFallback`) will be removed from the `App` component. By default, the query client configuration uses `suspense: true`, which masks the issue of missing pre-fetched query data by suspending components and displaying the fallback while queries resolve asynchronously. Removing this fallback ensures that missing query data triggers explicit errors instead of silently deferring to the fallback. This change improves the navigation experience and makes it easier to debug and fix data-loading issues during development. + +### 2. Implement `shouldRevalidate` on the `rootLoader` + +React Router by default only revalidates loaders after specific actions, such as form submissions or mutations. It does not automatically revalidate loaders during navigation between sibling routes under the same parent. To address this, the `shouldRevalidate` function will be implemented on the `rootLoader` route, with logic explicitly tailored to handle sub-route navigation. + +The `shouldRevalidate` function will determine whether the loader should re-execute by checking the following: + +* If the pathname has not changed between the current and next URLs, no revalidation will occur. +* If the pathname indicates navigation to a sub-route within the parent route (e.g., an enterprise-specific route or its sub-routes), the `rootLoader` will revalidate to ensure all required query data is pre-fetched and available. + +This logic ensures that revalidation is scoped specifically to relevant sub-route transitions, avoiding unnecessary data fetching while ensuring consistent query cache availability. + +### Benefits of this Approach + +* Queries required by UI components in the target route are always pre-fetched and cached before rendering. +* Developers can easily identify and address missing pre-fetches through explicit errors instead of relying on silent fallback behavior. +* Data consistency is maintained across sibling route transitions, particularly during the incremental migration to a Backend-for-Frontend (BFF) or API Gateway architecture. + +## Consequences + +### Positive Outcomes + +* **Reliable Query Data Availability:** Ensures that all required query data is pre-fetched and cached during sub-route transitions, preventing issues where `useQuery` tries to access missing or stale data. +* **Explicit Error Handling:** Removing the `AppSuspenseFallback` ensures that missing pre-fetched data triggers explicit errors instead of being masked by the fallback. This simplifies debugging and leads to better long-term reliability. +* **Scoped and Efficient Revalidation:** The `shouldRevalidate` function selectively targets relevant sub-route transitions. Combined with `ensureQueryData`, it prevents redundant API requests for data that remains fresh, minimizing performance impact. +* **Improved Developer Workflow:** By exposing missing pre-fetches, the approach facilitates early identification and resolution of query-related issues, reducing risks of data inconsistencies. + +### Negative Outcomes + +* **Initial Debugging Effort:** Removing the fallback may reveal existing cases where query data was not pre-fetched. This change could introduce additional debugging during development as missing pre-fetches surface as explicit errors. However, addressing these issues early ensures long-term consistency in data-loading strategies by enforcing a robust pattern of pre-fetching query data in route loaders. +* **Small Overhead for Non-Fresh Data:** Revalidating the `rootLoader` during sub-route navigation may result in occasional additional backend calls if data is not fresh. However, these calls are scoped and optimized, ensuring minimal impact on performance. + +## Alternatives Considered + +* Keeping the parent `Suspense` and its fallback component (`AppSuspenseFallback`) in the `App` component would allow the application to handle missing pre-fetches silently, suspending components and showing a secondary loading state during navigation. While this approach mitigates user-facing errors, it obscures data-loading inconsistencies and makes debugging more difficult. Additionally, it risks subtle issues in route navigation where query data dependencies are unclear. By adopting the chosen approach, explicit error handling ensures better enforcement of consistent and predictable data-loading practices. From 59cb844088b27eb9ac301f5f36ed9671b7cf633d Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 22 Jan 2025 09:19:15 -0500 Subject: [PATCH 6/9] fix: fallback to defaultShouldRevalidate --- src/routes.tsx | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/routes.tsx b/src/routes.tsx index fd849324c1..af5ba819f0 100644 --- a/src/routes.tsx +++ b/src/routes.tsx @@ -191,18 +191,19 @@ function getEnterpriseSlugRoutes(queryClient?: Types.QueryClient) { loader: getRouteLoader(makeRootLoader, queryClient), element: , children: enterpriseSlugChildRoutes, - shouldRevalidate: ({ currentUrl, nextUrl }) => { + shouldRevalidate: ({ currentUrl, nextUrl, defaultShouldRevalidate }) => { const currentPathname = currentUrl.pathname; const nextPathname = nextUrl.pathname; + + // If the pathname hasn't changed, defer to defaultShouldRevalidate if (currentPathname === nextPathname) { - // If the pathname hasn't changed, we don't need to revalidate. - return false; + return defaultShouldRevalidate; } // Check if the next pathname is a sub-route of the enterprise slug route. - // If it is, we should revalidate the route; otherwise, it shouldn't revalidate. - const matchedSubRoute = matchPath({ path: ':enterpriseSlug?/*' }, nextPathname); - return !!matchedSubRoute; + // If it is, we should revalidate; otherwise, fallback to defaultShouldRevalidate. + const hasMatchedSubRoute = !!matchPath({ path: ':enterpriseSlug?/*' }, nextPathname); + return hasMatchedSubRoute || defaultShouldRevalidate; }, }, ]; From b84f52d30ca552d2480b743633d0916755d140dd Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 22 Jan 2025 11:29:03 -0500 Subject: [PATCH 7/9] fix: updates --- .../app/routes/createAppRouter.test.jsx | 50 +++++++++++++++---- src/components/app/routes/createAppRouter.ts | 2 +- src/routes.tsx | 15 ++---- src/types.d.ts | 1 + 4 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/components/app/routes/createAppRouter.test.jsx b/src/components/app/routes/createAppRouter.test.jsx index 80f810af44..47602f7a14 100644 --- a/src/components/app/routes/createAppRouter.test.jsx +++ b/src/components/app/routes/createAppRouter.test.jsx @@ -1,5 +1,6 @@ import { - act, render, screen, waitFor, + act, + render, screen, waitFor, } from '@testing-library/react'; import { Outlet, RouterProvider } from 'react-router-dom'; import { IntlProvider } from '@edx/frontend-platform/i18n'; @@ -248,6 +249,10 @@ describe('createAppRouter', () => { expectedRouteTestId, expectedRouteLoaders, }) => { + // Update the current route path + window.history.pushState({}, '', currentRoutePath); + + // Render the app router const router = createAppRouter(mockQueryClient); render( @@ -259,15 +264,11 @@ describe('createAppRouter', () => { expect(makeRootLoader).toHaveBeenCalledTimes(1); expect(makeRootLoader).toHaveBeenCalledWith(mockQueryClient); expect(screen.getByTestId('root')).toBeInTheDocument(); - expect(screen.getByTestId('layout')).toBeInTheDocument(); - }); - - act(() => { - router.navigate(currentRoutePath); - }); - - await waitFor(() => { expect(screen.getByTestId(expectedRouteTestId)).toBeInTheDocument(); + if (expectedRouteTestId !== 'invite') { + // The invite routes are not associated with the `rootLoader` and `Layout` route + expect(screen.getByTestId('layout')).toBeInTheDocument(); + } }); if (expectedRouteLoaders.length > 0) { @@ -279,6 +280,37 @@ describe('createAppRouter', () => { }); } }); + + it('renders and revalidates rootLoader appropriately when navigating sub-routes (%s)', async () => { + // Create custom mocks + const rootLoaderFn = jest.fn().mockReturnValue(null); + makeRootLoader.mockReturnValue(rootLoaderFn); + + // Render the app router + const router = createAppRouter(mockQueryClient); + render( + + + , + ); + + // Assert initial load + await waitFor(() => { + expect(makeRootLoader).toHaveBeenCalledTimes(1); + expect(makeRootLoader).toHaveBeenCalledWith(mockQueryClient); + expect(rootLoaderFn).toHaveBeenCalledTimes(1); + }); + + // Trigger navigation to the next route + act(() => { + router.navigate('/test-enterprise/search'); + }); + + // Assert revalidation behavior + await waitFor(() => { + expect(rootLoaderFn).toHaveBeenCalledTimes(2); // Called again + }); + }); }); describe('getRouteLoader', () => { diff --git a/src/components/app/routes/createAppRouter.ts b/src/components/app/routes/createAppRouter.ts index b71c9d8b90..2845e07684 100644 --- a/src/components/app/routes/createAppRouter.ts +++ b/src/components/app/routes/createAppRouter.ts @@ -8,7 +8,7 @@ import { getRoutes } from '../../../routes'; * @param {Object} queryClient React Query query client. * @returns {Object} React Router browser router. */ -export default function createAppRouter(queryClient: Types.QueryClient) { +export default function createAppRouter(queryClient: Types.QueryClient): Types.Router { const { routes } = getRoutes(queryClient); const router = createBrowserRouter(routes); return router; diff --git a/src/routes.tsx b/src/routes.tsx index af5ba819f0..5a5877e602 100644 --- a/src/routes.tsx +++ b/src/routes.tsx @@ -192,18 +192,13 @@ function getEnterpriseSlugRoutes(queryClient?: Types.QueryClient) { element: , children: enterpriseSlugChildRoutes, shouldRevalidate: ({ currentUrl, nextUrl, defaultShouldRevalidate }) => { - const currentPathname = currentUrl.pathname; - const nextPathname = nextUrl.pathname; - - // If the pathname hasn't changed, defer to defaultShouldRevalidate - if (currentPathname === nextPathname) { - return defaultShouldRevalidate; + // If the pathname changed, we should revalidate + if (currentUrl.pathname !== nextUrl.pathname) { + return true; } - // Check if the next pathname is a sub-route of the enterprise slug route. - // If it is, we should revalidate; otherwise, fallback to defaultShouldRevalidate. - const hasMatchedSubRoute = !!matchPath({ path: ':enterpriseSlug?/*' }, nextPathname); - return hasMatchedSubRoute || defaultShouldRevalidate; + // If the pathname didn't change, fallback to the default behavior + return defaultShouldRevalidate; }, }, ]; diff --git a/src/types.d.ts b/src/types.d.ts index 9c0c4f9e71..aa086e7516 100644 --- a/src/types.d.ts +++ b/src/types.d.ts @@ -20,6 +20,7 @@ export type RouteLoaderFunctionArgs = import('react-router-dom').LoaderFunctionA export type MakeRouteLoaderFunction = (queryClient?: QueryClient) => RouteLoaderFunction; export type MakeRouteLoaderFunctionWithQueryClient = (queryClient: QueryClient) => RouteLoaderFunction; export type RouteObject = import('react-router-dom').RouteObject; +export type Router = import('@remix-run/router').Router; // Application Data (general) export interface AuthenticatedUser { From 6c567b5b565790b06f55b783bd0daa1d2548d834 Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 22 Jan 2025 11:55:18 -0500 Subject: [PATCH 8/9] chore: test coverage --- src/components/app/data/utils.js | 12 ++++---- .../app/routes/RouteErrorBoundary.test.jsx | 29 ++++++++++++++++++- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/components/app/data/utils.js b/src/components/app/data/utils.js index 82669c7524..dac50c04d3 100644 --- a/src/components/app/data/utils.js +++ b/src/components/app/data/utils.js @@ -500,9 +500,7 @@ export function retrieveErrorMessage(error) { if (!error) { return null; } - if (error.customAttributes) { - return error.customAttributes.httpErrorResponseData; - } + let errorMessage = error.message; // Identify development suspense error const isDeveloperSuspenseError = ( process.env.NODE_ENV === 'development' @@ -511,8 +509,7 @@ export function retrieveErrorMessage(error) { ) ); if (isDeveloperSuspenseError) { - const errorMessage = error.message; - return ( + errorMessage = ( `A component or hook triggered suspense, possibly due to missing pre-fetched data. Since \`suspense: true\` is configured by default for all React Query queries, ensure that any necessary @@ -522,7 +519,10 @@ export function retrieveErrorMessage(error) { ${errorMessage.split('\n')[0]}` ); } - return error.message; + if (error.customAttributes) { + errorMessage += `\nCustom attributes: ${error.customAttributes.httpErrorResponseData}`; + } + return errorMessage; } /** diff --git a/src/components/app/routes/RouteErrorBoundary.test.jsx b/src/components/app/routes/RouteErrorBoundary.test.jsx index f8614b7502..a1a8f5eb3f 100644 --- a/src/components/app/routes/RouteErrorBoundary.test.jsx +++ b/src/components/app/routes/RouteErrorBoundary.test.jsx @@ -18,9 +18,10 @@ const RouteErrorBoundaryWrapper = () => ( ); - +const originalNodeEnv = process.env.NODE_ENV; describe('RouteErrorBoundary', () => { beforeEach(() => { + process.env.NODE_ENV = originalNodeEnv; useRouteError.mockReturnValue(null); useAsyncError.mockReturnValue(null); @@ -43,6 +44,32 @@ describe('RouteErrorBoundary', () => { expect(screen.getByText('We apologize for the inconvenience. Please try again later.')).toBeInTheDocument(); }); + it('overrides default error message for development suspense errors', () => { + useRouteError.mockReturnValue(new Error('A React component suspended while rendering, but no fallback UI was specified')); + process.env.NODE_ENV = 'development'; + renderWithRouterProvider(); + expect(screen.getByText('An error occurred while processing your request')).toBeInTheDocument(); + expect(screen.getByText('We apologize for the inconvenience. Please try again later.')).toBeInTheDocument(); + expect(screen.getByText( + 'A component or hook triggered suspense, possibly due to missing pre-fetched data.', + { exact: false }, + )).toBeInTheDocument(); + }); + + it('uses customAttributes.httpErrorResponseData for axios errors', () => { + const error = new Error('RouteErrorWithCustomAttributes'); + error.customAttributes = { + httpErrorResponseData: { + status: 404, + }, + }; + useRouteError.mockReturnValue(error); + renderWithRouterProvider(); + expect(screen.getByText('An error occurred while processing your request')).toBeInTheDocument(); + expect(screen.getByText('We apologize for the inconvenience. Please try again later.')).toBeInTheDocument(); + expect(screen.getByText('Custom attributes:', { exact: false })).toBeInTheDocument(); + }); + it('displays the update available modal correctly when there is a ChunkLoadError route error', async () => { const chunkLoadError = new Error('ChunkLoadError'); chunkLoadError.name = 'ChunkLoadError'; From e45969f7db65bbb5f62b4aecc3af48de156dc6fe Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 22 Jan 2025 12:07:42 -0500 Subject: [PATCH 9/9] chore: test coverage --- .../app/data/hooks/useNProgressLoader.test.jsx | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/components/app/data/hooks/useNProgressLoader.test.jsx b/src/components/app/data/hooks/useNProgressLoader.test.jsx index db87eaf228..69b0d8fda5 100644 --- a/src/components/app/data/hooks/useNProgressLoader.test.jsx +++ b/src/components/app/data/hooks/useNProgressLoader.test.jsx @@ -122,7 +122,19 @@ describe('useNProgressLoader', () => { expect(result.current).toBe(true); }); - it('should call nprogress done with hydrated user and no notices', async () => { + it.each([ + { hasNoticesEnabled: true }, + { hasNoticesEnabled: false }, + ])('should call nprogress done with hydrated user and no notices (%s)', async ({ hasNoticesEnabled }) => { + const noticesQueryResult = { + data: undefined, + isLoading: true, + }; + if (hasNoticesEnabled) { + noticesQueryResult.data = null; + noticesQueryResult.isLoading = false; + } + useNotices.mockReturnValue(noticesQueryResult); const Wrapper = ({ children }) => ( {children}