From 82a58f1d03eed03780876b8f94220f11455c692b Mon Sep 17 00:00:00 2001 From: "Adolfo R. Brandes" Date: Mon, 28 Sep 2026 12:10:28 -0300 Subject: [PATCH] fix: stop replacing the shell's query client site-wide App providers wrap the whole site, so gradebook's QueryClientProvider imposed its settings on every other app sharing the site. Its query options now live on gradebook's own hooks. Fixes #634 Co-Authored-By: Claude --- README.rst | 6 +- src/app.ts | 2 - .../BulkManagementHistoryView/data/apiHook.ts | 5 +- .../GradebookFilters/data/apiHook.ts | 6 +- src/components/GradesView/data/apiHook.ts | 11 ++-- src/components/GradesView/data/hooks.js | 6 +- src/components/GradesView/data/hooks.test.ts | 6 ++ src/components/GradesView/data/queryKeys.ts | 5 ++ src/data/apiHook.test.ts | 3 +- src/data/apiHook.ts | 7 +-- src/data/query.test.ts | 60 +++++++++++++++++++ src/data/query.ts | 38 ++++++++++++ src/data/queryClient.test.ts | 37 ------------ src/data/queryClient.ts | 44 -------------- src/data/queryKeys.ts | 3 + src/providers.test.tsx | 18 ------ src/providers.tsx | 18 ------ src/testUtils.tsx | 6 ++ 18 files changed, 140 insertions(+), 141 deletions(-) create mode 100644 src/data/query.test.ts create mode 100644 src/data/query.ts delete mode 100644 src/data/queryClient.test.ts delete mode 100644 src/data/queryClient.ts delete mode 100644 src/providers.test.tsx delete mode 100644 src/providers.tsx diff --git a/README.rst b/README.rst index 2bb0e4ff..1c90c4ca 100644 --- a/README.rst +++ b/README.rst @@ -176,16 +176,12 @@ Directory Structure * ``app.ts`` - * The ``App`` object consumed by ``site.config.*.tsx`` — wires ``appId``, ``routes``, and ``providers`` for frontend-base to register. + * The ``App`` object consumed by ``site.config.*.tsx`` - wires ``appId``, ``routes``, and ``slots`` for frontend-base to register. * ``routes.tsx`` * React Router route definitions for the app. - * ``providers.tsx`` - - * App-scoped context providers registered with frontend-base. - * ``slots`` * Slots this app exposes for host sites to plug into. diff --git a/src/app.ts b/src/app.ts index f3531e79..98de502f 100644 --- a/src/app.ts +++ b/src/app.ts @@ -1,13 +1,11 @@ import { App } from '@openedx/frontend-base'; import { appId } from './constants'; import routes from './routes'; -import providers from './providers'; import slots from './slots'; const app: App = { appId, routes, - providers, slots, }; diff --git a/src/components/BulkManagementHistoryView/data/apiHook.ts b/src/components/BulkManagementHistoryView/data/apiHook.ts index e0725088..77556121 100644 --- a/src/components/BulkManagementHistoryView/data/apiHook.ts +++ b/src/components/BulkManagementHistoryView/data/apiHook.ts @@ -1,6 +1,5 @@ -import { useQuery } from '@tanstack/react-query'; - import { useAssignmentTypes, useCourseIdWithGate } from '@src/data/apiHook'; +import { useGradebookQuery } from '@src/data/query'; import { getBulkOperationHistory } from './api'; import { bulkOperationHistoryQueryKeys } from './queryKeys'; @@ -18,7 +17,7 @@ const EMPTY_ARRAY: never[] = []; export const useBulkOperationHistory = ( courseId: string, { enabled = true }: { enabled?: boolean } = {}, -) => useQuery({ +) => useGradebookQuery({ queryKey: bulkOperationHistoryQueryKeys.byCourse(courseId), queryFn: () => getBulkOperationHistory(courseId), enabled: !!courseId && enabled, diff --git a/src/components/GradebookFilters/data/apiHook.ts b/src/components/GradebookFilters/data/apiHook.ts index 8fb35489..e7d142bd 100644 --- a/src/components/GradebookFilters/data/apiHook.ts +++ b/src/components/GradebookFilters/data/apiHook.ts @@ -1,4 +1,4 @@ -import { useQuery } from '@tanstack/react-query'; +import { useGradebookQuery } from '@src/data/query'; import { getCohorts, getTracks } from './api'; import { cohortsQueryKeys, tracksQueryKeys } from './queryKeys'; @@ -11,7 +11,7 @@ import { cohortsQueryKeys, tracksQueryKeys } from './queryKeys'; export const useCohorts = ( courseId: string, { enabled = true }: { enabled?: boolean } = {}, -) => useQuery({ +) => useGradebookQuery({ queryKey: cohortsQueryKeys.byCourse(courseId), queryFn: () => getCohorts(courseId), enabled: !!courseId && enabled, @@ -25,7 +25,7 @@ export const useCohorts = ( export const useTracks = ( courseId: string, { enabled = true }: { enabled?: boolean } = {}, -) => useQuery({ +) => useGradebookQuery({ queryKey: tracksQueryKeys.byCourse(courseId), queryFn: () => getTracks(courseId), enabled: !!courseId && enabled, diff --git a/src/components/GradesView/data/apiHook.ts b/src/components/GradesView/data/apiHook.ts index 8975b034..517e6bd4 100644 --- a/src/components/GradesView/data/apiHook.ts +++ b/src/components/GradesView/data/apiHook.ts @@ -1,5 +1,5 @@ import { useMemo } from 'react'; -import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; +import { useMutation, useQueryClient } from '@tanstack/react-query'; import lms from '@src/data/services/lms'; import { sortAlphaAsc } from '@src/data/formatUtils'; @@ -7,6 +7,7 @@ import { filtersSnapshot } from '@src/data/filtersSnapshot'; import { useGradebookUi } from '@src/data/gradebookUiContext'; import { useCourseIdWithGate } from '@src/data/apiHook'; import { useCourseId } from '@src/data/courseIdContext'; +import { useGradebookQuery } from '@src/data/query'; import { trackGradesDisplayed, trackGradeOverrideSucceeded, @@ -19,7 +20,7 @@ import { import { bulkOperationHistoryQueryKeys } from '@src/components/BulkManagementHistoryView/data/queryKeys'; import { getGradeOverrideHistory, getGrades, getGradesPage } from './api'; -import { gradeOverrideHistoryQueryKeys, gradesQueryKeys } from './queryKeys'; +import { gradeMutationKeys, gradeOverrideHistoryQueryKeys, gradesQueryKeys } from './queryKeys'; import { buildGradesFetchParams } from './utils'; /** @@ -45,7 +46,7 @@ export const useGrades = ( courseId: string, gradesPageEndpoint: string | null, { enabled = true }: { enabled?: boolean } = {}, -) => useQuery({ +) => useGradebookQuery({ queryKey: [...gradesQueryKeys.byCourse(courseId), gradesPageEndpoint ?? 'base'], queryFn: async () => { const data = gradesPageEndpoint @@ -143,7 +144,7 @@ export const useGradesData = (): GradesData => { * is not worth retrying. */ export const useGradeOverrideHistory = (subsectionId?: string, userId?: string | number) => ( - useQuery({ + useGradebookQuery({ queryKey: gradeOverrideHistoryQueryKeys.byCell(subsectionId ?? null, userId ?? null), queryFn: () => getGradeOverrideHistory(subsectionId as string, userId as string | number), enabled: !!subsectionId && userId !== undefined && userId !== null, @@ -187,6 +188,7 @@ export const useUpdateGrades = () => { const courseId = useCourseId(); const { setShowSuccess, setGradesPageEndpoint, modalState } = useGradebookUi(); const mutation = useMutation({ + mutationKey: gradeMutationKeys.updateGrades, mutationFn: (updateData: GradeOverrideUpdate[]) => lms.api.updateGradebookData(courseId, updateData), onSuccess: ({ data }, updateData) => { trackGradeOverrideSucceeded(courseId, data); @@ -238,6 +240,7 @@ export const useSubmitImportGradesButtonData = () => { setCsvUploadErrors, } = useGradebookUi(); const mutation = useMutation({ + mutationKey: gradeMutationKeys.uploadGradeCsv, mutationFn: (formData) => lms.api.uploadGradeCsv(courseId, formData), onMutate: () => { resetCsvUpload(); diff --git a/src/components/GradesView/data/hooks.js b/src/components/GradesView/data/hooks.js index 0e370f80..b27d73b4 100644 --- a/src/components/GradesView/data/hooks.js +++ b/src/components/GradesView/data/hooks.js @@ -7,6 +7,7 @@ import { useFilters } from '@src/data/filtersContext'; import { useGradebookUi } from '@src/data/gradebookUiContext'; import { useCanViewGradebook } from '@src/data/apiHook'; import { useCourseId } from '@src/data/courseIdContext'; +import { BASE_KEY } from '@src/data/queryKeys'; import { useSelectedCohortEntry, useSelectedTrackEntry, @@ -87,12 +88,13 @@ export const useSelectedAssignmentLabel = () => useSelectedAssignmentData()?.lab /** * useShouldShowSpinner() * The busy indicator: the roles gate combined with the grades query fetching or - * any in-flight mutation (grade override save / CSV upload). + * one of the gradebook's own mutations in flight (grade override save / CSV + * upload). */ export const useShouldShowSpinner = () => { const canViewGradebook = useCanViewGradebook(); const { isFetching } = useGradesData(); - const mutatingCount = useIsMutating(); + const mutatingCount = useIsMutating({ mutationKey: BASE_KEY }); return canViewGradebook && (isFetching || mutatingCount > 0); }; diff --git a/src/components/GradesView/data/hooks.test.ts b/src/components/GradesView/data/hooks.test.ts index c80d610e..80550eea 100644 --- a/src/components/GradesView/data/hooks.test.ts +++ b/src/components/GradesView/data/hooks.test.ts @@ -6,6 +6,7 @@ import { useFilters } from '@src/data/filtersContext'; import { useGradebookUi } from '@src/data/gradebookUiContext'; import { useCanViewGradebook } from '@src/data/apiHook'; import { useCourseId } from '@src/data/courseIdContext'; +import { BASE_KEY } from '@src/data/queryKeys'; import { useSelectedCohortEntry, useSelectedTrackEntry, } from '@src/components/GradebookFilters/data/hooks'; @@ -217,6 +218,11 @@ describe('GradesView/data hooks', () => { const { result } = renderHook(useShouldShowSpinner); expect(result.current).toBe(true); }); + + it('only counts the gradebook\'s own mutations', () => { + renderHook(useShouldShowSpinner); + expect(useIsMutatingMock).toHaveBeenCalledWith({ mutationKey: BASE_KEY }); + }); }); describe('read-model hooks', () => { diff --git a/src/components/GradesView/data/queryKeys.ts b/src/components/GradesView/data/queryKeys.ts index 1e10be8a..ace4f395 100644 --- a/src/components/GradesView/data/queryKeys.ts +++ b/src/components/GradesView/data/queryKeys.ts @@ -9,6 +9,11 @@ export const gradesQueryKeys = { byCourse: (courseId: string) => [...BASE_KEY, courseId, 'grades'] as const, }; +export const gradeMutationKeys = { + updateGrades: [...BASE_KEY, 'updateGrades'] as const, + uploadGradeCsv: [...BASE_KEY, 'uploadGradeCsv'] as const, +}; + /** Grade-override history for one learner on one subsection (edit modal). */ export const gradeOverrideHistoryQueryKeys = { all: [...BASE_KEY, 'gradeOverrideHistory'] as const, diff --git a/src/data/apiHook.test.ts b/src/data/apiHook.test.ts index b5a8b69d..bc5b629d 100644 --- a/src/data/apiHook.test.ts +++ b/src/data/apiHook.test.ts @@ -77,7 +77,8 @@ describe('root apiHook', () => { }); it('returns false when the roles query errors out', async () => { - canViewMock.mockRejectedValue(new Error('boom')); + // 4xx-shaped so the shared retry rule (`query.ts`) doesn't retry it. + canViewMock.mockRejectedValue({ response: { status: 403 } }); const { result } = renderQueryHook(useCanViewGradebook); await waitFor(() => expect(result.current).toBe(false)); }); diff --git a/src/data/apiHook.ts b/src/data/apiHook.ts index baf5fbfa..851c0ff5 100644 --- a/src/data/apiHook.ts +++ b/src/data/apiHook.ts @@ -1,7 +1,6 @@ -import { useQuery } from '@tanstack/react-query'; - import { getAssignmentTypes, getCanUserViewGradebook } from './api'; import { useCourseId } from './courseIdContext'; +import { useGradebookQuery } from './query'; import { assignmentTypesQueryKeys, rolesQueryKeys } from './queryKeys'; /** @@ -12,7 +11,7 @@ import { assignmentTypesQueryKeys, rolesQueryKeys } from './queryKeys'; */ export const useCanUserViewGradebook = () => { const courseId = useCourseId(); - return useQuery({ + return useGradebookQuery({ queryKey: rolesQueryKeys.byCourse(courseId), queryFn: () => getCanUserViewGradebook(courseId), enabled: !!courseId, @@ -28,7 +27,7 @@ export const useCanUserViewGradebook = () => { export const useAssignmentTypes = ( courseId: string, { enabled = true }: { enabled?: boolean } = {}, -) => useQuery({ +) => useGradebookQuery({ queryKey: assignmentTypesQueryKeys.byCourse(courseId), queryFn: () => getAssignmentTypes(courseId), enabled: !!courseId && enabled, diff --git a/src/data/query.test.ts b/src/data/query.test.ts new file mode 100644 index 00000000..e87d9271 --- /dev/null +++ b/src/data/query.test.ts @@ -0,0 +1,60 @@ +import { useQuery } from '@tanstack/react-query'; +import { renderHook } from '@testing-library/react'; + +import { gradebookQueryOptions, retryUnlessClientError, useGradebookQuery } from './query'; + +jest.mock('@tanstack/react-query', () => ({ + ...jest.requireActual('@tanstack/react-query'), + useQuery: jest.fn(), +})); + +const useQueryMock = useQuery as jest.Mock; + +describe('gradebookQueryOptions', () => { + it('uses a 5-minute staleTime and disables refetchOnWindowFocus', () => { + expect(gradebookQueryOptions.staleTime).toBe(5 * 60 * 1000); + expect(gradebookQueryOptions.refetchOnWindowFocus).toBe(false); + expect(gradebookQueryOptions.retry).toBe(retryUnlessClientError); + }); + + describe('retryUnlessClientError', () => { + it('does not retry 4xx client errors', () => { + expect(retryUnlessClientError(0, { response: { status: 400 } })).toBe(false); + expect(retryUnlessClientError(0, { response: { status: 404 } })).toBe(false); + expect(retryUnlessClientError(0, { response: { status: 499 } })).toBe(false); + }); + + it('retries 5xx server errors up to 3 attempts', () => { + expect(retryUnlessClientError(0, { response: { status: 500 } })).toBe(true); + expect(retryUnlessClientError(2, { response: { status: 502 } })).toBe(true); + expect(retryUnlessClientError(3, { response: { status: 500 } })).toBe(false); + }); + + it('retries network errors (no response) up to 3 attempts', () => { + expect(retryUnlessClientError(0, new Error('network'))).toBe(true); + expect(retryUnlessClientError(3, new Error('network'))).toBe(false); + }); + }); +}); + +describe('useGradebookQuery', () => { + const queryFn = jest.fn(); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('applies the gradebook options to the query', () => { + renderHook(() => useGradebookQuery({ queryKey: ['test'], queryFn })); + expect(useQueryMock).toHaveBeenCalledWith({ + ...gradebookQueryOptions, + queryKey: ['test'], + queryFn, + }); + }); + + it('lets the caller override them', () => { + renderHook(() => useGradebookQuery({ queryKey: ['test'], queryFn, retry: false })); + expect(useQueryMock).toHaveBeenCalledWith(expect.objectContaining({ retry: false })); + }); +}); diff --git a/src/data/query.ts b/src/data/query.ts new file mode 100644 index 00000000..8814058a --- /dev/null +++ b/src/data/query.ts @@ -0,0 +1,38 @@ +import { + DefaultError, useQuery, UseQueryOptions, UseQueryResult, +} from '@tanstack/react-query'; + +/** Subset of an Axios-style error we inspect, to avoid depending on axios types. */ +interface HttpErrorLike { + response?: { status?: number }; +} + +const isClientError = (error: unknown): boolean => { + const status = (error as HttpErrorLike)?.response?.status; + return typeof status === 'number' && status >= 400 && status < 500; +}; + +export const retryUnlessClientError = (failureCount: number, error: unknown): boolean => { + if (isClientError(error)) { + return false; + } + return failureCount < 3; +}; + +export const gradebookQueryOptions = { + staleTime: 5 * 60 * 1000, // 5 minutes + refetchOnWindowFocus: false, + retry: retryUnlessClientError, +} as const; + +/** + * These can't be defaults on a QueryClient: app providers wrap the whole site, + * so an app-owned client would impose them on every other app sharing it. The + * shell's client sets no defaults, so a query that skips this wrapper silently + * falls back to stock React Query behavior. + */ +export function useGradebookQuery( + options: UseQueryOptions, +): UseQueryResult { + return useQuery({ ...gradebookQueryOptions, ...options }); +} diff --git a/src/data/queryClient.test.ts b/src/data/queryClient.test.ts deleted file mode 100644 index 9dc09000..00000000 --- a/src/data/queryClient.test.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { QueryClient } from '@tanstack/react-query'; -import { makeQueryClient } from './queryClient'; - -describe('makeQueryClient', () => { - const client = makeQueryClient(); - const { queries } = client.getDefaultOptions(); - - it('returns a QueryClient instance', () => { - expect(client).toBeInstanceOf(QueryClient); - }); - - it('uses a 5-minute staleTime and disables refetchOnWindowFocus', () => { - expect(queries?.staleTime).toBe(5 * 60 * 1000); - expect(queries?.refetchOnWindowFocus).toBe(false); - }); - - describe('retry rule', () => { - const retry = queries?.retry as (n: number, e: unknown) => boolean; - - it('does not retry 4xx client errors', () => { - expect(retry(0, { response: { status: 400 } })).toBe(false); - expect(retry(0, { response: { status: 404 } })).toBe(false); - expect(retry(0, { response: { status: 499 } })).toBe(false); - }); - - it('retries 5xx server errors up to 3 attempts', () => { - expect(retry(0, { response: { status: 500 } })).toBe(true); - expect(retry(2, { response: { status: 502 } })).toBe(true); - expect(retry(3, { response: { status: 500 } })).toBe(false); - }); - - it('retries network errors (no response) up to 3 attempts', () => { - expect(retry(0, new Error('network'))).toBe(true); - expect(retry(3, new Error('network'))).toBe(false); - }); - }); -}); diff --git a/src/data/queryClient.ts b/src/data/queryClient.ts deleted file mode 100644 index 6b810bd1..00000000 --- a/src/data/queryClient.ts +++ /dev/null @@ -1,44 +0,0 @@ -import { QueryClient } from '@tanstack/react-query'; - -/** - * Shape of the subset of an Axios-style error we inspect to decide retries. - * We only need the HTTP status, so we avoid a hard dependency on axios types. - */ -interface HttpErrorLike { - response?: { status?: number }; -} - -/** - * isClientError(error) - * True for 4xx responses, which are not worth retrying (bad request, auth, - * not found, etc.). Server (5xx) and network errors remain retryable. - */ -const isClientError = (error: unknown): boolean => { - const status = (error as HttpErrorLike)?.response?.status; - return typeof status === 'number' && status >= 400 && status < 500; -}; - -/** - * Application-wide React Query client. - * - * Defaults are promoted here (rather than repeated per hook) to keep individual - * query hooks lean: cache server data for 5 minutes and skip retries on 4xx. - */ -export const makeQueryClient = (): QueryClient => new QueryClient({ - defaultOptions: { - queries: { - staleTime: 5 * 60 * 1000, // 5 minutes - refetchOnWindowFocus: false, - retry: (failureCount: number, error: unknown) => { - if (isClientError(error)) { - return false; - } - return failureCount < 3; - }, - }, - }, -}); - -const queryClient = makeQueryClient(); - -export default queryClient; diff --git a/src/data/queryKeys.ts b/src/data/queryKeys.ts index 2bf6d3c6..e876690b 100644 --- a/src/data/queryKeys.ts +++ b/src/data/queryKeys.ts @@ -7,6 +7,9 @@ // caches independently per course and mutations can invalidate a whole course // subtree via a broad prefix. The array shapes match the legacy // `gradebookQueryKeys` so cache identity / invalidation is preserved. +// +// Mutations are keyed under `BASE_KEY` too: the client is shared site-wide, so +// `useShouldShowSpinner` counts by this prefix and a keyless mutation is missed. export const BASE_KEY = ['gradebook'] as const; /** Roles-based "can view the gradebook" permission gate (the fetch-cascade root). */ diff --git a/src/providers.test.tsx b/src/providers.test.tsx deleted file mode 100644 index 140607da..00000000 --- a/src/providers.test.tsx +++ /dev/null @@ -1,18 +0,0 @@ -import { render, screen } from '@testing-library/react'; -import { useQueryClient } from '@tanstack/react-query'; - -import providers from './providers'; -import queryClient from './data/queryClient'; - -const Probe = () => { - const client = useQueryClient(); - return
{client === queryClient ? 'configured' : 'other'}
; -}; - -describe('app providers', () => { - it('provide the configured query client to the app subtree', () => { - const [QueryProvider] = providers; - render(); - expect(screen.getByTestId('probe')).toHaveTextContent('configured'); - }); -}); diff --git a/src/providers.tsx b/src/providers.tsx deleted file mode 100644 index e3e0c37c..00000000 --- a/src/providers.tsx +++ /dev/null @@ -1,18 +0,0 @@ -import { QueryClientProvider } from '@tanstack/react-query'; -import { AppProvider } from '@openedx/frontend-base'; - -import queryClient from '@src/data/queryClient'; - -/** - * Wraps the app subtree in the configured query client (5-minute staleTime, no - * refetch on focus, no retries on 4xx — see `src/data/queryClient.ts`), - * shadowing the shell's default client. - */ -// eslint-disable-next-line react/prop-types -const QueryProvider: AppProvider = ({ children }) => ( - {children} -); - -const providers: AppProvider[] = [QueryProvider]; - -export default providers; diff --git a/src/testUtils.tsx b/src/testUtils.tsx index 7992bab4..549c0de1 100644 --- a/src/testUtils.tsx +++ b/src/testUtils.tsx @@ -67,6 +67,11 @@ interface WrapperProps { /** * Builds a `renderHook` wrapper that provides a React Query client. Pass an * existing client when the test needs to inspect it (e.g. spy on invalidation). + * + * `retry: false` here loses to the per-query retry in `src/data/query.ts`, and + * the app's hooks take `enabled` and nothing else, so a test that rejects with + * a 5xx or a bare `Error` runs the real backoff and times out. Reject with a + * 4xx-shaped error instead. */ export function createQueryClientWrapper( client?: QueryClient, @@ -96,6 +101,7 @@ export const renderWithAllProviders = ( ui: ReactElement, { courseId = testCourseId, ...options }: { courseId?: string } = {}, ) => { + // Same `retry: false` caveat as `createQueryClientWrapper`. const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false, gcTime: 0 },