fix: call createAppRouter within App component; implement shouldRevalidate for rootLoader sub-route navigation

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.

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) => ({
@@ -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 = useMemo(() => createAppRouter(queryClient), []);
+
   return (
     <QueryClientProvider client={queryClient}>
       <ReactQueryDevtools initialIsOpen={false} />
@@ -80,12 +82,10 @@ const App = () => {
         </Suspense>
       )}
       <AppProvider wrapWithRouter={false}>
-        <Suspense fallback={<AppSuspenseFallback />}>
-          <RouterProvider
-            router={router}
-            fallbackElement={<RouterFallback />}
-          />
-        </Suspense>
+        <RouterProvider
+          router={router}
+          fallbackElement={<RouterFallback />}
+        />
       </AppProvider>
     </QueryClientProvider>
   );

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: () => <div data-testid="react-query-devtools">ReactQueryDevtools</div>,
+}));
+
+jest.mock('@tanstack/react-query-devtools/production', () => ({
+  ReactQueryDevtools: () => <div data-testid="react-query-devtools-production">ReactQueryDevtoolsProduction</div>,
+}));
+
+jest.mock('@edx/frontend-platform/react', () => ({
+  AppProvider: ({ children }) => <div data-testid="app-provider">{children}</div>,
+}));
+
+describe('App', () => {
+  let queryClient;
+
+  beforeEach(() => {
+    queryClient = new QueryClient();
+    const mockRouter = createMemoryRouter(
+      [{ path: '/', element: <div data-testid="mock-route">Mock Route</div> }],
+      { initialEntries: ['/'] },
+    );
+    createAppRouter.mockReturnValue(mockRouter);
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  test('renders App component without errors', () => {
+    render(
+      <QueryClientProvider client={queryClient}>
+        <App />
+      </QueryClientProvider>,
+    );
+
+    expect(screen.getByTestId('app-provider')).toBeInTheDocument();
+  });
+
+  test('toggles ReactQueryDevtoolsProduction visibility', async () => {
+    render(<App />);
+
+    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(<App />);
+
+    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();
+  });
+});

src/components/app/data/hooks/useNProgressLoader.js

@@ -10,15 +10,15 @@ 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();
   const fetchers = useFetchers();
   const {
     data: noticeRedirectUrl,
     isLoading: isLoadingNotices,
-  } = useNotices();
+  } = useNotices(queryOptions.useNotices);
 
   const hasNoticeRedirectUrl = !isLoadingNotices && !!noticeRedirectUrl;
   const isAppDataHydrated = isAuthenticatedUserHydrated && !hasNoticeRedirectUrl;

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 }) => (
       <AppContext.Provider value={appContextValueWithHydratedUser}>
         {children}

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(() => {

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 }) => (
   <QueryClientProvider client={queryClient()}>
     {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 () => {

src/components/app/data/utils.js

@@ -500,10 +500,29 @@ export function retrieveErrorMessage(error) {
   if (!error) {
     return null;
   }
+  let errorMessage = error.message;
+  // 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) {
+    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
+      API data accessed via \`useQuery\` is pre-fetched in a route loader
+      to avoid triggering suspense (loading states) unexpectedly.
+      Error:
+      ${errorMessage.split('\n')[0]}`
+    );
+  }
   if (error.customAttributes) {
-    return error.customAttributes.httpErrorResponseData;
+    errorMessage += `\nCustom attributes: ${error.customAttributes.httpErrorResponseData}`;
   }
-  return error.message;
+  return errorMessage;
 }
 
 /**

src/components/app/routes/RouteErrorBoundary.test.jsx

@@ -18,9 +18,10 @@ const RouteErrorBoundaryWrapper = () => (
     <RouteErrorBoundary />
   </IntlProvider>
 );
-
+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(<RouteErrorBoundaryWrapper />);
+    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(<RouteErrorBoundaryWrapper />);
+    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';