Skip to content

refactor: convert the courseware metadata fetch to React Query - #2023

Open
brian-smith-tcril wants to merge 2 commits into
masterfrom
bsmith/react-query-courseware-metadata
Open

refactor: convert the courseware metadata fetch to React Query#2023
brian-smith-tcril wants to merge 2 commits into
masterfrom
bsmith/react-query-courseware-metadata

Conversation

@brian-smith-tcril

@brian-smith-tcril brian-smith-tcril commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Convert the courseware metadata fetch (fetchCourse) to React Query, and extend the model-store bridge to mirror collection results. Part of the Redux → React Query migration (#1946), the #1976 courseware decomposition (Target 1); stacked on the CoursewareContainer de-class (#2020) and TypeScript (#2021) layers as the new top of the stack. Two commits — #2009 (bridge → collections) and #2010 (courseware metadata). Closes #2009, #2010.

fetchCourse fetched four things (course metadata, the learning-sequences outline, course-home metadata, sidebar toggles) and derived courseStatus. This moves the three data fetches to query hooks (mirrored into the model store via the bridge so the existing useModel readers keep working), moves the status derivation into transitional bridge hooks, and thins fetchCourse to just the un-converted sidebar-toggles fetch.

What changed

  • courseware/data/apiHooks.ts / queryKeys.ts (new) — useCoursewareMetadata (getCourseMetadata) and useCoursewareOutline (getLearningSequencesOutline), tagged with meta.models so the bridge fans each result into the right model(s). courseId is string | undefined (from useParams) with an enabled guard; key factories stay strict string.
  • course-home/data/modelStoreBridge.ts (Peel: extend the model-store bridge to collection dispatches #2009) — the bridge's meta gains a models: [{ modelType, strategy, source? }] list form, so one query result can fan out to several model targets with the right add-vs-merge semantics (the outline writes three: coursewareMeta/sections/sequences).
  • courseware/data/statusBridge.ts (new) — useCourseStatusBridge (container) and useCourseExitStatusBridge (CourseExit) run the queries and mirror their combined state into state.courseware.courseStatus/courseId, so the still-Redux readers (redirect helpers, selectors, TabPage's string status, the exit-page children) keep working. Transitional — removed when those readers move to React Query.
  • CoursewareContainer.tsx — calls useCourseStatusBridge(routeCourseId); the memoized checkFetchCourse guard is byte-identical to base (it now dispatches the thinned fetchCourse).
  • CourseExit.jsx — self-wrapping on the query hooks via useCourseExitStatusBridge, rendering TabPage itself.
  • courseware/data/thunks.jsfetchCourse thinned to just the sidebar-toggles fetch (its full conversion is Convert the courseware outline sidebar to React Query + context #2013); its model writes move to the bridge and its status derivation to the status bridges.
  • queryClient.ts / data/http-error.ts — the app QueryCache's onError logs query failures; a query overrides the level per HTTP status via meta.logStatusAs: { <status>: <level> } (the outline's expected 403 → logInfo). meta is typed globally via a Register.queryMeta augmentation, so onError and the bridge read it cast-free. Implements the last piece of Restore dropped query error logging via a global QueryCache.onError #2022.
  • useIFrameBehavior.ts — the post-event refetch invalidates the courseware queries instead of dispatching fetchCourse.
  • index.jsx — the courseware route drops the <TabContainer fetch={fetchCourse}> wrapper for a bare <CourseExit />.

Behavior

No user-facing change. Metadata/outline/courseHomeMeta now load via React Query and populate the model store through the bridge; courseStatus/courseId are still written (transitionally, by the status bridges) so the redirects, gating, and TabPage behave as before. The sidebar toggles still load via the thinned fetchCourse. Query error logging is preserved, including the outline's 403 → logInfo (a logged-out learner's expected redirect is logged at info level, not surfaced as an error).

Testing

npm run types, npm run lint, and the full npm test suite pass (109 suites / 912 passing / 3 pre-existing skips at PR-open; the fix below adds courseware/data/apiHooks.test.tsx, +1 suite / +1 test). queryClient.test.ts covers onError (default logError, logStatusAs override) and the model-store bridge; modelStoreBridge.test.ts covers the list-form fan-out; the container / CourseExit / useIFrameBehavior / ProductTours tests render through the bridged query client; setupTest's seedCoursewareModels replaces the executeThunk(fetchCourse) seed.

Manual browser verification is done (tutor local, DemoX), confirming no user-facing change: cold load / hard reload, the full set of URL → redirect rules, prev/next sequence navigation (within and across sequences), unit completion, the course-exit / celebration page, and preview mode all behave as before. The sequence-navigation data path is additionally covered by a new courseware/data/apiHooks.test.tsx, which asserts the coursewareMeta mirror preserves sectionIds regardless of query-resolution order (see the coursewareMeta mirror note in the decision log). Items needing a specific user/backend state (access-denied, the outline's expected 403 telemetry, a forced 5xx, sidebar toggles) are left to the automated suite. Full checklist below.

Manual testing checklist & findings

Run against a live backend (tutor local, DemoX). Since this conversion claims no
user-facing change, the pass confirms equivalence end-to-end with real data, a real unit
iframe, and real redirects — what the jest suite can't fully exercise.

Verified by hand:

  • Cold load + hard reload on a unit URL — the bridge fans the outline into
    coursewareMeta/sections/sequences and the metadata into
    coursewareMeta/courseHomeMeta; page renders fully (title/header, unit content,
    outline sidebar, iframe) with no flash of missing structure.
  • Resume redirect (course root → last-active unit, or first sequence if fresh) —
    checkResumeRedirect.
  • Section-in-URL → the section's first sequence (checkSectionToSequenceRedirect), then
    that sequence's activeUnitIndex unit (saved-position resume, unchanged by refactor: convert the courseware metadata fetch to React Query #2023);
    a never-visited chapter deterministically lands on the sequence's unit 1.
  • Section + unit → drops the section and resolves the unit to its real parent sequence
    (checkSectionUnitToUnitRedirectcheckUnitToSequenceUnitRedirect).
  • Unit-only redirect → fills in the parent sequence (checkUnitToSequenceUnitRedirect).
  • Unit markers /first and /last → first / last unit of the sequence.
  • Sequence + unit navigation (prev/next, within and across sequences) — structure and
    order match the outline.
  • Unit completion after interaction — the unit iframe is not torn down by the
    invalidation. The completion checkmark updates on navigation, not instantly:
    confirmed not a regression — the checkmark reads units[].complete from the
    sequence metadata (fetchSequence), which neither the old dispatch(fetchCourse) nor
    the new invalidation refetches; refactor: convert the courseware metadata fetch to React Query #2023 invalidates exactly the three queries
    fetchCourse used to (courseware metadata, outline, course-home metadata).
  • Course exit / celebration — CourseExit (self-wrapping via useCourseExitStatusBridge)
    renders the correct state.
  • Preview mode — renders; identical to the normal URL, which is expected: refactor: convert the courseware metadata fetch to React Query #2023
    touches no preview-sensitive path (isPreview only affects fetchSequence and the
    CoursewareContainer redirect prefix).

Not exercised by hand (need a specific user/backend state; covered by the suite):

  • Access-denied / unenrolled learner gating (courseHomeMeta.courseAccess).
  • Outline expected 403 → logInfo telemetry (logged-out learner).
  • Genuine failure (5xx) → error UI + logError.
  • Sidebar toggles (the one fetch not converted — thinned fetchCourse).

Covered by the automated suite instead:

  • Bridge fan-out (list-form add-vs-merge) — modelStoreBridge.test.ts.
  • Status-bridge derivation — statusBridge.test.ts.
  • onError default + logStatusAs override — queryClient.test.ts.
  • Redirect-helper branches — CoursewareContainer.test.jsx (70 tests).
  • CourseExit / useIFrameBehavior / ProductTours render paths — their suites via the
    bridged client.
  • Thinned fetchCourse (sidebar toggles) — redux.test.js.

Decisions

Full decision log

Decisions — courseware metadata → React Query (+ bridge collections) (#2009 + #2010)

Working notes for this PR (part of the wider Redux → React Query migration,
#1946). Not checked in — referenced when opening the PR. Part of the #1976
courseware decomposition (Target 1). Stacked on the de-class (#2020) and the TS
conversion (#2021).

Why #2009 and #2010 are one PR (two commits). #2009 (the bridge extension) has
no runtime effect on its own — nothing uses the new meta form until #2010 wires
a query to it — and we expect to tweak the bridge while doing #2010. Landing them
together ships a "real" chunk (courseware metadata actually on React Query) instead of
a dormant infra PR followed by its only consumer. Two commits keep the concerns
legible: commit 1 = bridge, commit 2 = the metadata conversion.


Part 1 — extend the model-store bridge to collections (#2009, commit 1)

Extend the transitional bridge (src/course-home/data/modelStoreBridge.ts) so a query
can mirror collection results (and several model targets) into the models store,
not just a single addModel.

Why: the courseware producers write collections, and one fetch → many models

fetchCourse writes four model types with mixed strategies; the outline endpoint alone
writes three from one response:

source model type action
getCourseMetadata coursewareMeta addModel (data has its own id)
getCourseHomeCourseMetadata courseHomeMeta addModel keyed by courseId
getLearningSequencesOutline.courses coursewareMeta updateModelsMap (merge sectionIds)
getLearningSequencesOutline.sections sections addModelsMap
getLearningSequencesOutline.sequences sequences updateModelsMap (merge)

The bridge runs in the QueryCache onSuccess, which receives the query's raw
result, so a query whose result is { courses, sections, sequences } must fan that one
result out to three mirrors.

Contract: keep the single form, add a models list

  • { modelType, courseId } (unchanged) — mirror the whole result as one model keyed
    by courseId. The course-home tabs use this; byte-compatible.
  • { models: [{ modelType, strategy, source? }] } (new) — one or more mirrors.
    strategy is a model-store action (addModel / updateModel / addModelsMap /
    updateModelsMap / updateModels); source selects a key of the result (omitted =
    the whole result). Lets one query populate several targets with the right add-vs-merge
    semantics; source is what avoids splitting the outline into 3 fetches.

Alternatives considered

  • Single-target + a collection flag. Rejected: the bridge sees the raw result and
    meta is one-target-per-query, so the outline's 3-in-1 shape couldn't be expressed
    without 3 separate fetches (3× network) or a bespoke onSuccess.
  • Bespoke per-query handlers. Same objection — abandons the shared bridge.

North star

The bridge is throwaway scaffolding, deleted with the model store in #1977. After that,
readers stop calling useModel(...) and read from the RQ hooks directly. A couple of
models are assembled from two endpoints (sequences = outline shallow +
getSequenceMetadata full; coursewareMeta = getCourseMetadata + outline
sectionIds), so those readers combine the relevant hooks — a #2011/#2013/#1977
concern, not this PR.

Tests

modelStoreBridge.test.ts drives real queries through createModelStoreQueryCache(store)
and asserts the resulting models state: single form, list-form fan-out via source,
updateModelsMap merging (not clobbering), updateModels over an array, and the no-op
when meta is absent.


Part 2 — convert courseware metadata to React Query (#2010, commit 2)

Convert the courseware metadata fetch (fetchCourse) to React Query, mirroring into the
model store via the Part-1 bridge so the ~14 useModel readers keep working.

Scope: convert the metadata/outline/courseHomeMeta fetches; thin fetchCourse in place

fetchCourse did four fetches (metadata, outline, courseHomeMeta, sidebar toggles) + set
courseStatus. Its consumers: CoursewareContainer (player), the CourseExit route
(<TabContainer fetch={fetchCourse}>), useIFrameBehavior (refetch on an iframe event), and
setupTest's initializeTestStore. Decision: move the three data fetches to RQ hooks and
the status derivation into the container, then thin fetchCourse in place
so it does only
the un-converted remainder (the sidebar toggles) — rather than deleting it and adding a new
fetchCoursewareOutlineSidebarToggles thunk.

  • Why thin, not delete+recreate: Convert the courseware outline sidebar to React Query + context #2013 removes the toggles (its last responsibility) and
    deletes fetchCourse then. Renaming/replacing it now is churn for a transitional step —
    CoursewareContainer's guard (checkFetchCoursedispatch(fetchCourse(id))) stays
    byte-identical to the base, and fetchCourse visibly shrinks (4 fetches → 1) across
    layers until it's gone. Transitional cost: fetchCourse is briefly a misnomer (it only
    fetches toggles now).
  • Converting all consumers (not just the player) avoids the query hooks and
    useIFrameBehavior double-populating models on the player.

The query hooks (new courseware/data/apiHooks.ts + queryKeys.ts)

  • useCoursewareMetadata(courseId)getCourseMetadata,
    meta: { models: [{ modelType: 'coursewareMeta', strategy: 'addModel' }] } (result has its own id).
    • Why the array form here and not the single { modelType, courseId } form (as
      useCourseHomeMeta uses)?
      The single form (bridge line 44) does
      addModel({ model: { id: courseId, ...data } }) — its purpose is to inject courseId
      as the id
      , which courseHomeMeta needs because its payload has no id of its own.
      coursewareMeta's payload has its own id, and fetchCourse stored it with
      addModel({ model: metadata }) (no injection). The array form + strategy: 'addModel'
      (no source) maps exactly to that. The two happen to produce the same stored value here
      (in { id: courseId, ...data } the spread wins, and data.id === courseId on this
      route), so the single form wouldn't break — but the array form (a) expresses the real
      intent ("store by the model's own id") instead of relying on the spread-override
      coincidence, (b) is the byte-analog of fetchCourse's call, and (c) keeps both courseware
      hooks on one contract (the outline hook must use the array form to fan out to 3 targets).
  • useCoursewareOutline(courseId)getLearningSequencesOutline,
    meta: { models: [ courses→coursewareMeta/updateModelsMap, sections→addModelsMap, sequences→updateModelsMap ] }.
  • Reuse useCourseHomeMeta for courseHomeMeta — don't re-fetch it.
    • Faithfulness note: useCourseHomeMeta fetches the 'outline' rootSlug variant;
      fetchCourse used 'courseware'. rootSlug only renames the courseware tab's slug
      in the normalized tabs; courseAccess (what the gating reads) is identical. So
      reuse is faithful for gating; the only difference is that tab's slug.

courseId typing: guarded hooks, strict keys (a migration-wide convention)

useParams() types every route param as string | undefined — React Router can't prove
which route a component renders under — even though :courseId is always present on the
courseware/course-exit routes. This bit only surfaces now because #2010 adds the first
typed (.tsx) caller
of these hooks (CoursewareContainer); the existing course-home
callers are all .jsx, so the argument was never type-checked. Every future .tsx
conversion hits the same thing, so we picked one convention:

  • Hooks take string | undefined + enabled: !!courseId. The hook honestly tolerates
    the useParams type by not firing when the id is absent (the standard React Query
    idiom for "param may not be ready"). Call sites pass useParams straight through — no
    casts or guards proliferating across the migration. No-op for the 6 existing .jsx
    callers, since courseId is never actually undefined there.
  • Key factories take string (never undefined). A query key is a real identity; a
    key on undefined is meaningless. So queryKeys.ts stays strict.
  • The seam is a ! at the key call inside the hookcoursewareQueryKeys.metadata(courseId!).
    This is deliberate, not sloppy: queryKey is evaluated eagerly (React Query computes
    it every render regardless of enabled), so the factory is still called when the query
    is disabled. The ! says "keys are built from real ids"; the adjacent enabled: !!courseId
    is what actually makes the never-happens undefined case safe (no fetch). Runtime-wise the
    ! is purely type-level.

Alternative considered — narrow courseId to string once at the .tsx boundary
(then keys and hooks are string, no enabled, no !). Rejected: it relocates the
useParams undefined into a guard/assertion at every typed boundary — the
cast-in-the-wrong-place friction from #2019 — instead of handling it once, idiomatically,
in the hook. (This also reverts a stringstring | undefined widening of
useCourseHomeMeta/courseHomeQueryKeys.metadata that #2010 briefly introduced before we
settled on this convention.)

Error logging: global QueryCache.onError + the outline's 403 nuance

fetchCourse logged each endpoint independently: logError on failed metadata/courseHomeMeta/
toggles, and for the outline a 403 ? logInfo : logError split (a 403 there is the expected
access-denied case — the learner is redirected — so it's logged via logInfo, not logError,
which would surface it as a noticed error).

React Query v5 removed onError from useQuery (it's only on useMutation and the
QueryCache), so per-hook query logging isn't possible. The home for query error logging is the
global QueryCache.onError — permanent app infra, introduced with the QueryCache in #1987
and tracked by #2022. By default it logErrors.

What #2010 adds — the courseware outline is the one query that surfaces a 403 as an error
(its getter throws it; the course-home tab getters swallow 401/403/404 → {}, so their query
errors are only genuine failures). So:

  • useCoursewareOutline tags meta: { logStatusAs: { 403: 'info' } }.
  • onError reads query.meta.logStatusAs — a status → LogLevel map, defaulting to
    error
    (it says how to log each status, not a "quiet" flag). So an outline 403 →
    logInfo, restoring fetchCourse's behavior; anything else → logError. LogLevel
    ('error' | 'info') derives from a { error: logError, info: logInfo } map — the only two
    loggers platform exposes.
  • The toggles keep their logging via the thinned fetchCourse's catch → logError.

Typed meta, no casts. meta is typed globally via a Register.queryMeta augmentation
(ModelStoreMeta & { logStatusAs?: Record<number, LogLevel> }), so both onError and the
model-store bridge read query.meta without a cast, and meta literals are checked at the
write site. getResponseStatus (data/http-error.ts) reads the error's status. At #1977 the
augmentation drops its ModelStoreMeta half along with the bridge.

Migration-wide context (#2022): other converted queries dropped their thunks' logError too,
but their getters swallow 401/403/404, so the global onError (plain logError) already covers
their genuine failures without per-query meta. The outline is the exception that needs the
logStatusAs tag.

Access-gating + status: a transitional useCourseStatusBridge

The old fetchCourse derived courseStatus (request → success/denied/failure) from
courseAccess.hasAccess + outline success and dispatched fetchCourse{Request,Success, Denied,Failure}. That derivation moves out of the thunk into a transitional bridge hook,
useCourseStatusBridge (courseware/data/statusBridge.ts): it runs the three query hooks and
mirrors their combined state into state.courseware.courseStatus via the same status actions,
so the still-Redux readers (the container's redirect helpers/selectors, TabPage's string
status, useContextId) keep working. CoursewareContainer just calls
useCourseStatusBridge(routeCourseId).

Why a bridge, and why a component hook (not the model-store one): it's the same "keep Redux
populated from RQ transitionally" idea as the model-store bridge, but courseStatus is a
derivation across all three queries — which a per-query QueryCache.onSuccess can't express —
so it's a component-level hook, not part of the centralized bridge. Deleted when those readers
move to RQ.

The status reducers stay. fetchCourse is thinned, not deleted (see the scope section), so
checkFetchCourse stays too — it just dispatches the thinned fetchCourse (toggles only). RQ
auto-fetches on courseId change, so no separate metadata-fetch guard is needed.

CourseExit route → self-wrapping (+ transitional slice write for the exit children)

<TabContainer tab="courseware" fetch={fetchCourse}> becomes CourseExit self-wrapping
on the query hooks (rendering TabPage itself), matching the course-home tab pattern.

Why the transitional slice write: the course-exit children (CourseCelebration,
CourseNonPassing, CourseInProgress, and the recommendation/upgrade helpers) read
courseId from state.courseware — which fetchCourse used to set on this route. Without the
thunk, that slice field would be unset on the CourseExit route, so useModel('courseHomeMeta', undefined) would return {} and tabs.find(...) would throw. Rather than convert all ~7 children off
the slice (that's #1976's job), CourseExit writes courseId/courseStatus to the slice
transitionally via useCourseExitStatusBridge (courseware/data/statusBridge.ts) — the
CourseExit sibling of useCourseStatusBridge (2 queries, no outline). CourseExit owns the
two queries (it also feeds them to its own TabPage gating), so it passes them into the
bridge rather than the bridge returning them. The children keep reading the slice until they
convert.

useIFrameBehavior refetch → query invalidation

The iframe POST_EVENT handler's postEvent.mutate onSuccess did
dispatch(fetchCourse(courseId)); it now queryClient.invalidateQueries the three query keys
(coursewareQueryKeys.metadata, coursewareQueryKeys.outline, courseHomeQueryKeys.metadata)
via useQueryClient, so the refetch goes through RQ.

fetchCourse refetched a fourth thing — the sidebar toggles — but we deliberately don't
invalidate those here, and nothing else is needed for them: enableCompletionTracking is a
static course-level setting, and this trigger is a unit-level learner action (POST_EVENT)
that can't change it, so refetching it on every event was redundant. The three invalidated
queries cover exactly the data such an event can change (completion / gating / access), and
courseStatus re-derives via useCourseStatusBridge once those queries resettle.

Sidebar toggles peeled aside (to #2013)

getCoursewareOutlineSidebarTogglessetCoursewareOutlineSidebarToggles feeds only
the outline sidebar. It's the one fetch left in the thinned fetchCourse (which the
container still dispatches via the unchanged checkFetchCourse guard), so the setting keeps
loading until the sidebar layer (#2013) converts it and deletes fetchCourse. Not folded
into the query hooks here.

Align the toggle + outline-data peels with #1920's data split

Upstream PR #1920 ("Course outline restructure") splits the outline sidebar hook into
useCourseOutlineData() (the data: sections/sequences/units/courseOutlineStatus/
activeSequenceId/sequenceStatus, plus isEnabledCompletionTracking and
isActiveEntranceExam) and useCourseOutlineSidebar() (just open/collapse UI state). Notably
it moves isEnabledCompletionTracking from useCourseOutlineSidebar() to
useCourseOutlineData()
— i.e. the completion-tracking toggle is treated as outline data,
not sidebar chrome.

Implication for our decomposition (not #2010 — the transitional thunk just writes the slice,
which #1920 still reads via useSelector, so there's no conflict): when we convert these to
React Query, group the toggle conversion (#2013) with the outline-data conversion
(sections/sequences/units/status) so both feed useCourseOutlineData. Don't structure the
toggle as its own sidebar-flavored peel. Aligned that way, the eventual rebase over #1920 is
just "point useCourseOutlineData at the query hooks." (#1920 is UI-only — no data//thunk/
slice files — and currently stalled/red, so we align the split now and rebase whenever it moves.)

Test seeding (setupTest.js)

initializeTestStore (used by ~37 test files) seeds via executeThunk(fetchCourse).
Replace that with direct model-store dispatches replicating fetchCourse's writes
(coursewareMeta / courseHomeMeta / sections / sequences) from the same mocked data, so
the 37 callers keep working unchanged. Contained to setupTest.js.


Model-store mirror strategy

The coursewareMeta mirror merges, not replaces

Decision. useCoursewareMetadata mirrors its result into the coursewareMeta model
with strategy: 'updateModel' (merge), not addModel (replace).

Why. coursewareMeta[courseId] is written by two independent queries: the metadata
query (getCourseMetadata, whose payload has no sectionIds) and the outline query
(getLearningSequencesOutline, the only source of sectionIds, mirrored via
updateModelsMap on courses). addModel is a full replace (state[type][id] = model),
so when the metadata query resolved after the outline, it wiped the sectionIds the
outline had merged in. sequenceIdsSelector then returned [], so
useSequenceNavigationMetadata computed sequenceIndex = -1previousSequenceId = null
(couldn't go back a sequence) and isLastUnit true → next went to /course-end.
Intermittent, because it was a network-order race, re-rolled by the unit-completion query
invalidation. The old fetchCourse was immune: after Promise.allSettled it dispatched
addModel(metadata) then updateModelsMap(courses) synchronously, so the sectionIds
merge always ran last (there was even a comment saying so). updateModel restores that
guarantee regardless of resolution order; the payload carries id, and the bridge already
supports the strategy.

Tested. courseware/data/apiHooks.test.tsx renders both hooks through the bridged
query client with the metadata response deferred so its mirror lands last, then asserts
coursewareMeta.sectionIds and sequenceIdsSelector survive — RED with addModel, GREEN
with updateModel.

Audit: no other model-store mirror has the same exposure

Decision. Only the coursewareMeta metadata mirror needed the fix; the other mirrors
are correct as-is.

The rule. A replace-style mirror (addModel / addModelsMap) is only unsafe when
another writer contributes a field that the replacing query's own endpoint does not
return
— a cross-source field. coursewareMeta.sectionIds was the unique case (metadata
query replaces the model; sectionIds only ever comes from the outline endpoint).

Findings.

  • sectionsaddModelsMap (replace-per-section), but the outline is the only runtime
    writer, so nothing else's fields can be clobbered. Safe.
  • sequences — outline updateModelsMap + fetchSequence updateModel, both merge (the
    old code deliberately merged here: "sequence metadata may have come back first"). Safe.
  • coursewareMeta — after the fix, all three runtime writers (metadata mirror, outline
    mirror, a updateModel in thunks.js) merge. Safe.
  • courseHomeMeta (pre-existing, not refactor: convert the courseware metadata fetch to React Query #2023) — same shape (useCourseHomeMeta addModel
    replace vs celebration/streak updateModel), but benign: celebrations is part of the
    course-home metadata response (normalizeCourseHomeCourseMetadata spreads the whole
    payload) and is server-persisted (postCelebrationComplete), so a replace-refetch
    restores it — the field is same-source, unlike sectionIds. refetchOnWindowFocus is
    also false. No action.

@brian-smith-tcril brian-smith-tcril changed the title bsmith/react query courseware metadata refactor: convert the courseware metadata fetch to React Query Aug 21, 2026
@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.53%. Comparing base (020a3ee) to head (1d711f2).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2023   +/-   ##
=======================================
  Coverage   93.53%   93.53%           
=======================================
  Files         363      367    +4     
  Lines        5905     5959   +54     
  Branches     1367     1411   +44     
=======================================
+ Hits         5523     5574   +51     
- Misses        367      368    +1     
- Partials       15       17    +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@brian-smith-tcril brian-smith-tcril linked an issue Aug 21, 2026 that may be closed by this pull request
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch 2 times, most recently from 54b53a9 to b9eb81e Compare August 21, 2026 14:48
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch 2 times, most recently from 27fc4d9 to bd4c718 Compare August 21, 2026 18:24
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from bd4c718 to 11de7ef Compare August 21, 2026 18:32
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from 11de7ef to e145ef9 Compare August 21, 2026 18:34
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from e145ef9 to b72083e Compare August 21, 2026 18:40
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from b72083e to fb407a8 Compare August 21, 2026 18:55
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from fb407a8 to 8d9a956 Compare August 21, 2026 19:05
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from 8d9a956 to e861457 Compare August 21, 2026 19:15
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from e861457 to 29b1274 Compare August 21, 2026 19:23
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch 2 times, most recently from 462f871 to a2eb96d Compare August 22, 2026 07:05
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from a2eb96d to 332f52e Compare August 23, 2026 07:31
@brian-smith-tcril
brian-smith-tcril marked this pull request as ready for review August 23, 2026 07:57

@arbrandes arbrandes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comments are distilled from a bigger list from Claude. They're the ones I figured worth suggesting.

None of them are blockers, so I'm approving and trusting your judgement as to what, if anything, is worth modifying before merging.


const CourseExit = () => {
const { courseId } = useParams();
const metadataQuery = useCoursewareMetadata(courseId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: add useCoursewareOutline(courseId) here and feed it through the exit status bridge / TabPage the way useCourseStatusBridge does. (Or source hasScheduledContent from the outline query directly.)

hasScheduledContent is emitted only by the outline response (utils.js:64), which the old <TabContainer fetch={fetchCourse}> fetched on this route and CourseExit no longer does, so on a cold load of /course/:courseId/course-end the inProgress branch of getCourseExitMode (utils.js:63) is unreachable and the learner gets CourseNonPassing instead - arriving from the player still works, since the player's outline query already mirrored the field into the model store.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in https://github.com/openedx/frontend-app-learning/compare/632bfad9bd850ce29ea2b5fe90155c1f557cfc0b..a5711cc618bcf5e0d1ecf3c5e43f9fcc96b5169a

  • Added a test to cover the outlined scenario
  • Added a call to useCoursewareOutline

Considered the "source hasScheduledContent from the outline query directly" option, but that would increase the scope of this PR. That part will be addressed as part of #1977

Comment thread src/courseware/data/statusBridge.ts Outdated
const dispatch = useDispatch();
const metadataQuery = useCoursewareMetadata(courseId);
const outlineQuery = useCoursewareOutline(courseId);
const courseHomeMetaQuery = useCourseHomeMeta(courseId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: parameterize the hook - useCourseHomeMeta(courseId, rootSlug) with rootSlug in the query key - and pass 'courseware' from here and from CourseExit.

normalizeCourseHomeCourseMetadata renames the courseware tab's slug to whatever rootSlug it's handed (api.js:20), so moving these two routes from the thunk's 'courseware' to the hook's 'outline' leaves no tab matching activeTabSlug="courseware" at LoadedTabPage.tsx:46: the "Course" tab loses its active class and the <title> loses its tab prefix.

@brian-smith-tcril brian-smith-tcril Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in https://github.com/openedx/frontend-app-learning/compare/a5711cc618bcf5e0d1ecf3c5e43f9fcc96b5169a..97c0d3eeb9481626937412abb22c3da0a53bab73


🤖 The summary and decision log below were written by Claude Code, which implemented this change.

Summary: useCourseHomeMeta hardcoded rootSlug: 'outline', so on the CoursewareContainer and CourseExit routes (both render activeTabSlug="courseware") the shared course/outline tab was labeled 'outline' and never matched — the "Course" tab lost its active state and the <title> lost its tab prefix. The fix parameterizes useCourseHomeMeta(courseId, rootSlug) (required, and part of the query key), makes every caller pass its slug explicitly ('courseware' from useCourseStatusBridge + CourseExit; 'outline' from the course-home tabs + CourseAccessErrorPage), and points useIFrameBehavior's invalidation at the 'courseware' variant — matching the pre-conversion dispatch(fetchCourse). The per-context fetch is parity with the old thunks; the single-shared-fetch optimization belongs with #1977.

Full decision log

Comment: parameterize useCourseHomeMeta with a rootSlug (courseware vs outline tab slug)

The regression

useCourseHomeMeta hardcodes getCourseHomeCourseMetadata(courseId, 'outline'). The
course-home metadata API returns a "Course" tab with tab_id: 'courseware', and
normalizeCourseHomeCourseMetadata(data, rootSlug) rewrites that one tab's slug to
whatever rootSlug it's handed (every other tab keeps its own id). That rename exists
because the same API tab means the outline tab on course-home pages and the
courseware tab under CoursewareContainer.

LoadedTabPage reads tabs from useModel('courseHomeMeta') and highlights the tab whose
slug === activeTabSlug. Both CoursewareContainer (:399) and CourseExit (:81) render
with activeTabSlug="courseware". With the hook hardcoded to 'outline', that shared tab gets
slug 'outline' on those routes, so nothing matches → the "Course" tab loses its active state
and the <title> loses its tab prefix. Both CoursewareContainer and CourseExit are affected.

Pre-PR this worked because the courseware route's fetchCourse thunk fetched
getCourseHomeCourseMetadata(courseId, 'courseware'); the conversion unified everyone on
'outline' and broke it.

Fix: explicit rootSlug, in the query key

  • useCourseHomeMeta(courseId, rootSlug)rootSlug required (no default). Every caller
    passes its context slug: 'courseware' from useCourseStatusBridge and CourseExit;
    'outline' from the five course-home tabs (OutlineTab, DatesTab, ProgressTab,
    LiveTab, DiscussionTab) and CourseAccessErrorPage. (useCourseStatusBridge is
    CoursewareContainer's course-home-metadata source — it isn't just writing slice status; it
    calls useCourseHomeMeta, so it carries the 'courseware' rootSlug on that route's behalf,
    which is why the mirrored tab matches CoursewareContainer's activeTabSlug="courseware".)
  • queryFn: () => getCourseHomeCourseMetadata(courseId, rootSlug).
  • rootSlug is part of the query key: courseHomeQueryKeys.metadata(courseId, rootSlug).

Chose explicit everywhere over a = 'outline' default so no call site silently inherits
the wrong slug. CourseAccessErrorPage only reads courseAccess (rootSlug-independent for its
behavior), so its value is purely a cache-sharing choice; it passes 'outline' to preserve
today's behavior and share the course-home entry.

Why rootSlug must be in the query key

rootSlug never reaches the API — the request URL is /api/course_home/course_metadata/{courseId}
regardless; rootSlug only feeds the client-side normalize. But React Query caches the
queryFn's post-normalize return value, and the tab bar reads the mirrored courseHomeMeta
model, which only re-updates when the query actually runs (a cache hit doesn't re-fire the
mirror). So if two contexts shared one key, the first-fetched normalization would be served to
the other context and never re-normalized → the bug persists on navigation. Keying by rootSlug
gives each context its own entry, so each route fetches, re-normalizes, and re-mirrors its own
slug.

The duplicate fetch is parity, not new waste

Keying by rootSlug means the same endpoint is fetched once per context. That matches the
pre-PR Redux behavior exactly: fetchTab fetched it with 'outline' and fetchCourse with
'courseware', both addModel-ing into the same single courseHomeMeta model, and thunks
re-ran on every route entry (no caching). RQ is strictly better within a context (repeat
mounts hit cache). The "fetch once, shared across both contexts, rename per-context at read"
optimization requires getting the tab bar off useModel and reading slugs from the query —
that is #1977 (dissolve the model-store cache), out of scope here.

Invalidation stays explicit 'courseware' (no optional/prefix key)

useIFrameBehavior invalidates course-home metadata after a unit postEvent. Pre-PR that site
did dispatch(fetchCourse(courseId)), which re-fetched with 'courseware' (the unit iframe runs
under CoursewareContainer) and overwrote the single model — it only ever refreshed the
courseware context. Faithful mapping: invalidate courseHomeQueryKeys.metadata(eventCourseId, 'courseware'), an explicit context, not a rootSlug-less prefix that also touches 'outline'.
The stale 'outline' entry (if any) self-heals on the next course-home visit via
refetch-on-mount — same as the thunk.

Consequence: the key builder stays required metadata(courseId, rootSlug) — no optional
param, no falsy check, no empty-string-as-"all-variants" footgun. useIFrameBehavior is just
another explicit 'courseware' caller.

Tests

Hook-level in src/course-home/data/apiHooks.test.tsx (the fix lives in the hook; the
mirror → useModelLoadedTabPage match is unchanged machinery):

  • rootSlug: 'courseware' yields the shared tab with slug: 'courseware' (and not 'outline'),
    and vice-versa — the direct regression.
  • Rendering both variants against one client keeps each slug distinct — guards the query-key
    half of the fix (someone dropping rootSlug from the key would still pass the per-slug tests
    but reintroduce the navigation bug; only this test catches it).

Invalidation is covered by the existing useIFrameBehavior.test.js post-event case, updated to
assert courseHomeQueryKeys.metadata('course-1', 'courseware') (the explicit context, matching
the pre-PR dispatch(fetchCourse)).

TDD baseline (before the fix)

Written test-first. With only the minimal signature in place (rootSlug param + in the key,
queryFn still hardcoded 'outline', invalidation still passing the rootSlug-less key), the
suite is red exactly where the decisions say it should be, and nowhere else:

  • 'courseware' shared-tab slug — queryFn ignores rootSlug.
  • 'outline' shared-tab slug — already the hardcoded value.
  • ✕ query keyed by rootSlug (two variants share one entry).
  • useIFrameBehavior invalidates the 'courseware' metadata query.

The fix (queryFn threads rootSlug; key builder required; 8 callers explicit; invalidation
passes 'courseware') turns all four green.

Verification

npm run types ✓ · npm run lint ✓ · full suite green (111 suites, 934 passed, 3 pre-existing
skips). The four baseline failures are green, and the eight caller changes (5 course-home tabs +
CourseAccessErrorPage + useCourseStatusBridge + CourseExit) broke nothing downstream.

meta: { models: [{ modelType: 'coursewareMeta', strategy: 'updateModel' }] },
});

export const useCoursewareOutline = (courseId: string | undefined) => useQuery({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: add a retry policy - e.g. retry: (count, error) => getResponseStatus(error) !== 403 && count < 3 - here, on useCoursewareMetadata, and on useCourseHomeMeta (or as a QueryClient default in queryClient.ts).

Both 403-throwing getters (getLearningSequencesOutline and getCourseHomeCourseMetadata; only the tab-data getters swallow 4xx) now run under React Query's default retry: 3 with exponential backoff, and a retrying query's status stays pending, so statusBridge.ts:26-28 holds courseStatus at loading for ~7 s before the denied/failed redirect that fetchCourse reached at once. The suite can't see it - setupTest sets retry: false.

@brian-smith-tcril brian-smith-tcril Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in https://github.com/openedx/frontend-app-learning/compare/97c0d3eeb9481626937412abb22c3da0a53bab73..e11cc01fa46764fc180e49f9aa33a282ee92db55

Properly handling that meant fully implementing #2024, so it'd be great if you could take a look at this one before I land it.


🤖 The summary and decision log below were written by Claude Code, which implemented this change.

Summary: Under React Query's default retry: 3, a query that throws a 4xx (e.g. the expected 403 on the outline / course-home-metadata calls) stays pending through ~7s of backoff before the denied/failed redirect — a stall the old single-shot fetchCourse never had, and one already live on master for the merged course-home tabs. Rather than a per-query patch, this completes #2024: a global shouldRetryQuery in queryClient.ts that skips 4xx (fast-fail) and retries only 5xx/network. Errors our own queryFn code throws deterministically (mapSearchResponse's schema validation) are tagged NonRetryableError so they fast-fail without giving up network retry, and useCoursewareSearchEnabled's now-redundant retry: false is removed. Covered by predicate unit tests, a wiring test, and an end-to-end integration pass through the real client.

Full decision log

Comment: add a retry policy so expected 4xx (e.g. 403) fail fast instead of ~7s

Decision

Pull #2024 ("Smart query retry: skip 4xx, retry 5xx/network errors") into this PR: set a
global retry policy on the app query client (src/queryClient.ts) instead of a per-query
patch. This closes/subsumes #2024.

Why (not defer, not a per-query half-measure)

Under RQ's default retry: 3 + exponential backoff, a query that throws a 4xx stays pending
for ~7s before settling. The access getters (getLearningSequencesOutline,
getCourseHomeCourseMetadata) throw on 403, so useCourseStatusBridge pins courseStatus: 'loading' ~7s before the denied/failed redirect that the old single-shot fetchCourse reached
at once.

This stall is systemic and already live on master: useCourseHomeMeta (retry-3, throws
403) already backs the merged course-home tab conversions, so denied learners already wait ~7s
there. #2023 would only extend the stalling set to CoursewareContainer + CourseExit.

Grounding: what the pre-conversion (Redux) code did

The old data layer was Redux thunks — single API calls with no retry:

  • fetchCourse / fetchTab (course-home/data/thunks.js, pre-conversion): Promise.all/
    allSettled of the getters → dispatch → done. No retry loop.
  • searchCourseContent (course-home/data/thunks.js, pre-refactor: convert courseware search from Redux to React Query #1970): one
    searchCourseContentFromAPI + mapSearchResponse in a try/catch that records
    errors = e.message and stops — no retry, for any error (axios or the schema throw).

The per-getter swallow logic (401/403/404 → {}) lives in the shared getters, which both the
old thunks and the new hooks call — so that behavior is unchanged by the conversion.

So the only retry-related delta the conversion introduced is thunk no-retry → RQ default
retry: 3 on everything
(the regression, incl. the 4xx stall). Against that baseline:

  • skip 4xx restores the old fast-fail (getters that swallow those codes never threw them;
    getters that throw now fail fast again).
  • retry 5xx/network is a new enhancement — the old code never retried anything. Smart query retry: skip 4xx, retry 5xx/network errors #2024
    adopts it deliberately as the modern default (per the reference), not as a faithful
    reproduction. Every query's 5xx retry is therefore an intentional deviation from old behavior.

Reference implementation

openedx/frontend-app-learner-dashboard (frontend-base branch,
src/data/hooks/queryHooks.ts):

retry: (failureCount, error) => {
  // Don't retry client errors (4xx) — they won't resolve on retry
  if (error?.response?.status >= 400 && error?.response?.status < 500) return false;
  return failureCount < 3;
},

(There it's applied per-query alongside masquerade-specific retryOnMount/refetchOnMount,
which are hook-specific and not relevant to a global default.) We apply it globally, reading
status via getResponseStatus, and extend the reference with one thing the reference didn't
need: a check for errors our own queryFn code throws deterministically (see the tag decision
below). Both helpers live in src/data/http-error.ts:

export const shouldRetryQuery = (failureCount, error) => {
  if (isNonRetryable(error)) return false;                                      // our deterministic throws
  const status = getResponseStatus(error);
  if (status !== undefined && status >= 400 && status < 500) return false;      // 4xx
  return failureCount < 3;                                                      // 5xx + network
};

Query audit — a global policy is safe for every current query

Every useQuery in the app, and how the "skip 4xx / retry 5xx+network" default treats it:

Hook → getter Getter behavior (evidence) Under the policy
useCoursewareMetadatagetCourseMetadata GET + normalize, no catch → throws all (courseware/data/api.js:32) 4xx fast-fail, 5xx retry — fixes stall
useCoursewareOutlinegetLearningSequencesOutline GET + normalize, no catch → throws all incl. expected 403 (courseware/data/api.js:26) 4xx fast-fail, 5xx retry — fixes stall
useCourseHomeMetagetCourseHomeCourseMetadata GET + normalize, no catch → throws all incl. 403 (course-home/data/api.js:105) 4xx fast-fail, 5xx retry — fixes stall
useDatesTabDatagetDatesTabData catch: 401→{}, 403→{}, else throw (course-home/data/api.js:116) swallowed → no throw (no-op); rest 4xx fast / 5xx retry
useOutlineTabDatagetOutlineTabData catch: 403→{}, else throw (course-home/data/api.js:253) same
useProgressTabDatagetProgressTabData catch: 404→redirect+{}, 401→{}, 403→{}, else throw (course-home/data/api.js:139) same
useLiveTabDatagetLiveTabIframe catch: 404→{}, else throw (course-home/data/api.js:224) same
useTourDatagetTourData catch: 401/403/404→{toursEnabled:false}, else throw (product-tours/data/api.js:4) same
useCourseRecommendationsgetCourseRecommendations [] if no DISCOVERY_API_BASE_URL; else 2 GETs, no catch → throws (course-exit/data/api.js:26) non-critical; 4xx fast / 5xx retry
useCoursewareSearchResultssearchCourseContentFromAPI + mapSearchResponse POST, no catch → throws axios; mapSearchResponse throws a plain Error on Joi schema-validation failure (map-search-response.js:27) axios: 4xx fast / 5xx retry; schema throw → thrown as NonRetryableError → fast-fail (see per-query section)
useCoursewareSearchEnabledgetCoursewareSearchEnabled GET, no catch → throws all (course-home/data/api.js:366) remove its retry: false (no justification) → inherits the policy; see per-query section

Gotchas considered (and why none breaks the policy)

  • Undefined-status errors. The policy retries undefined-status errors so genuine network
    failures retry. The one non-network case is mapSearchResponse (map-search-response.js:27),
    which throws on a schema-invalid 200 — a deterministic our-code failure. We throw it as a
    NonRetryableError so it fast-fails; other undefined-status errors (network) still retry. A
    grep 'throw new' across src/ confirms this is the only explicit non-HTTP throw reachable
    from a queryFn — every getter otherwise re-throws the axios error (which carries
    response.status), and the other throw new Errors are React context-provider misuse guards
    thrown at render, not in queries. Caveat: an implicit runtime throw inside a queryFn (e.g.
    a normalizer TypeError on malformed data) has no status and can't be tagged, so it would retry
    3× — rare, deterministic, and bounded, so accepted.
  • Status extraction. getResponseStatus reads error.response.status only. The
    frontend-platform client sets both response.status and customAttributes.httpErrorStatus
    on HTTP errors, so getters that branch on customAttributes still throw errors that carry
    response.status → classified correctly.
  • Per-query retry: false overrides. None is genuinely justified under the global policy —
    full derivation in the dedicated section below. Net: remove useCoursewareSearchEnabled's and
    add none; deterministic our-code throws are handled by tagging (below), not retry: false.
  • Mutations unaffected. The default targets queries.retry only; RQ mutations default to no
    retry (our mutations don't need it).
  • Tests unaffected. setupTest's createTestQueryClient and the per-test wrappers set
    retry: false, so the global policy is production-facing; existing suites don't change
    (confirmed by the full run — see Verification).

Per-query retry: false: none is justified — tag deterministic throws instead

Derived from first principles. Under the global policy (skip 4xx, retry 5xx/network/undefined),
a per-query retry: false changes behavior in only these cases — 5xx, network, and
undefined-status (plain-Error) throws — turning them from retry into fast-fail. (4xx is already
fast-failed globally, so retry: false adds nothing there.)

So retry: false is justified only if a query must fast-fail on 5xx / network / plain-Error:

  • No query needs 5xx/network fast-fail. Those are transient; retrying either recovers (good)
    or fails after ~3 attempts (same end state, slightly later). Even the redirect-driving queries
    are better off retrying a transient 5xx than fast-failing to an error page.
  • So the only thing any retry: false protects is "don't retry a plain-Error throw" —
    and retry: false is over-broad for that, since it also kills the (fine) 5xx/network retry.

Applied consistently, no per-query retry: false is warranted:

  • useCoursewareSearchEnabled — no plain-Error throw, no fast-fail need → remove its
    retry: false.
  • useCoursewareSearchResults — has a plain-Error throw (mapSearchResponse), but
    retry: false is over-broad → don't add it; handle the throw via the tag below.

The one remaining concern — a plain-Error throw — is handled precisely by tagging. It's our
own code (mapSearchResponse's schema validation), so it's deterministic: re-running
re-fetches the same body and fails identically — retrying is guaranteed-pointless (3 real
round-trips), not a "might recover" transient. Since we own the throw, we mark the error:

  • A NonRetryableError class + isNonRetryable(error) in src/data/http-error.ts (colocated
    with getResponseStatus). shouldRetryQuery calls isNonRetryable first and fast-fails.
  • mapSearchResponse throws a NonRetryableError instead of new Error.

This fast-fails our deterministic throws without touching the 5xx/network retry — network
errors aren't NonRetryableErrors (undefined status, no flag) → still retry, honoring #2024's
"network" — and needs no per-query retry: false anywhere.

Detect by property, not instanceof. NonRetryableError carries a nonRetryable flag and
isNonRetryable reads that property. We use a class for clean construction (new NonRetryableError)
but deliberately don't detect via instanceof — a property read is simpler, matches how we
already inspect errors (getResponseStatus), and sidesteps instanceof's identity assumptions.

Plan (after this doc)

  1. Add a NonRetryableError class + isNonRetryable to src/data/http-error.ts.
  2. Add shouldRetryQuery (checks isNonRetryable, then skip-4xx, then failureCount < 3) as a
    named, exported function and wire it into createQueryClient's defaultOptions.queries.retry.
  3. Throw mapSearchResponse's schema-validation failure as a NonRetryableError.
  4. Remove useCoursewareSearchEnabled's retry: false (verify refactor: convert courseware search from Redux to React Query #1970's search tests don't assert
    the old no-retry).
  5. TDD in src/queryClient.test.ts (shouldRetryQuery as a pure predicate): false for
    400/401/403/404/422 and for a NonRetryableError; true for 500/502/503 and undefined-status
    network errors while failureCount < 3; false once failureCount reaches 3.
  6. Full suite + types + lint.
  7. Smart query retry: skip 4xx, retry 5xx/network errors #2024: close as done-in-refactor: convert the courseware metadata fetch to React Query #2023 (note in the PR / issue).

Verification

npm run types ✓ · npm run lint ✓ · queryClient.test.ts 18 passed, covering three layers:
the shouldRetryQuery predicate (unit), that createQueryClient wires it as the default query
retry, and an integration pass that runs real queries through createQueryClient (with
retryDelay: 0) so React Query invokes the policy with its own (failureCount, error) — proving
params, error shape, and count: 4xx/NonRetryableError → 1 attempt; 5xx/network → 4 (1 + 3
retries). Full suite green except the pre-existing Course.test.jsx waitFor-leak flake
(unrelated; un-awaited waitFor on this branch's base) — it passes in isolation (16/16).

import { getCourseMetadata, getLearningSequencesOutline } from './api';
import { coursewareQueryKeys } from './queryKeys';

export const useCoursewareMetadata = (courseId: string | undefined) => useQuery({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: set refetchOnWindowFocus: false on both hooks, as useLiveTabData does.

fetchCourse only ran on courseId change, so courseStatus never moved on its own; with the default true, a focus refetch re-derives it from fresh results and a transient failure sends the bridge to fetchCourseDenied/fetchCourseFailure (statusBridge.ts:30-45), redirecting the learner off the unit they were reading.

@brian-smith-tcril brian-smith-tcril Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in https://github.com/openedx/frontend-app-learning/compare/e11cc01fa46764fc180e49f9aa33a282ee92db55..1d711f2f7469941f0c26a54dfe1352c2795689a1

Decided to set refetchOnWindowFocus to false globally in createQueryClient instead of per-query. Redux didn't do any window focus refetching so this keeps things in line with previous behavior. I figure if there are places where we want window focus refetching in the future we can look into regression-free ways to do so in follow-up issues/PRs.


🤖 The summary and decision log below were written by Claude Code, which implemented this change.

Summary: RQ defaults refetchOnWindowFocus: true and nothing sets staleTime, so every query refetched on every window focus; on the courseware route that re-ran useCourseStatusBridge, and a failing focus-refetch (e.g. a 4xx session-expiry) redirected the learner off the unit they were reading. Setting it false as a global createQueryClient default (mirrored in the test client) rather than per-hook is the more faithful fix — the Redux app never focus-refetched anywhere (thunks fired on route/mount only), so global restores that uniformly, whereas per-hook would leave focus-refetch on for the tabs/search the app never refetched. Freshness is unaffected: mutable data is invalidation-driven (unit events, date shift, tour mutations) + refetchOnMount. Dropped the now-redundant per-hook flags on useLiveTabData/useTourData, whose focus test now verifies the global default end-to-end.

Full decision log

Comment: set refetchOnWindowFocus: false so a focus refetch can't redirect the learner

Decision

Set refetchOnWindowFocus: false globally on the app query client (src/queryClient.ts,
defaultOptions.queries, beside the retry policy) — and mirror it in the test client
(createTestQueryClient). Drop the now-redundant per-hook flags on useLiveTabData and
useTourData. Off-by-default is the floor: if a specific query later benefits from focus-refetch,
re-enable it per-hook then.

The bug

RQ defaults to refetchOnWindowFocus: true, and nothing sets staleTime, so every query is
immediately stale → it refetches on every window focus. On the courseware route,
useCourseStatusBridge derives courseStatus from useCoursewareMetadata, useCoursewareOutline,
and useCourseHomeMeta; a focus refetch of any of them re-runs the bridge, and a failure
dispatches fetchCourseDenied/fetchCourseFailure (statusBridge.ts:30-45) → the container
redirects the learner off the unit they were reading. (The retry policy from the previous comment
softens transient 5xx/network — those retry and usually recover — but a 4xx like a session-expiry
401/403 still fast-fails → redirect.)

Why global, not per-hook: faithfulness

The strongest argument is parity with the pre-conversion Redux app, which never refetched on
window focus — every thunk (fetchCourse, fetchTab/fetchProgressTab/fetchOutlineTab/
fetchDatesTab, search, tour) fired on route/mount only. RQ's conversion introduced app-wide
focus-refetch as a new default. A global false restores the old app-wide behavior uniformly; a
per-hook fix (just the three bridge hooks) would leave focus-refetch on for
dates/progress/outline-tab/search/recommendations — a behavior the Redux app never had. So global
is the more faithful option; per-hook would leave an inconsistent state the app never shipped.

Future freshness is additive: opt a query back into focus-refetch per-hook if it genuinely benefits.

Safety: restoring old behavior breaks no freshness

Investigation confirms nothing relies on focus-refetch for freshness:

  • No query sets staleTime — focus-refetch fires on every focus, not as targeted freshness.
  • Mutable data is invalidation-driven: useIFrameBehavior invalidates
    metadata/outline/course-home-meta on unit postEvents; ShiftDatesAlert invalidates dates +
    outline on a date shift; tour mutations invalidate tour data. Plus refetchOnMount on navigation.
  • Two queries have no invalidationuseProgressTabData (grades) and the course-home
    useOutlineTabData (completion is written to the Redux model via updateCourseOutlineCompletion,
    not the tab query). They refresh on navigation/mount, and had no focus-refresh under Redux
    either — so global false is parity, not a regression.

Reference

learner-dashboard (frontend-base) sets refetchOnWindowFocus per-hook and keeps it on
for normal users — its dashboard data changes mid-session. We diverge deliberately: our in-course
data is stable and invalidation-driven, so uniform off matches our app (and our Redux history),
not the dashboard's. This is our own call, not a copied pattern.

Mechanism + tests

  • createQueryClient (src/queryClient.ts): add refetchOnWindowFocus: false to
    defaultOptions.queries (alongside retry).
  • createTestQueryClient (src/setupTest.js): add it too, so tests mirror production — and so
    LiveTab.test.jsx's existing "does not refetch on focus" test (focusManager) now verifies the
    global default end-to-end through a real component (it currently passes via useLiveTabData's
    per-hook flag, which we're removing).
  • Remove the redundant per-hook refetchOnWindowFocus: false from useLiveTabData and
    useTourData.
  • Wiring test in queryClient.test.ts: createQueryClient's default query options set
    refetchOnWindowFocus: false.

Plan

  1. Add refetchOnWindowFocus: false to createQueryClient's defaultOptions.queries.
  2. Add it to createTestQueryClient in setupTest.js.
  3. Remove the per-hook flags on useLiveTabData and useTourData.
  4. Wiring test; confirm LiveTab's focus test still passes (now via the global default).
  5. Full suite + types + lint.

Verification

npm run types ✓ · npm run lint ✓ · full suite green (111 suites, 950 passed, 3 pre-existing
skips, 0 failures). The queryClient.test.ts wiring test confirms createQueryClient's default
refetchOnWindowFocus: false, and LiveTab.test.jsx's "does not refetch on focus" now passes via
the global test-client default (per-hook flag removed) — an end-to-end confirmation of the
no-focus-refetch behavior.

dispatch(fetchCourseRequest({ courseId }));
return;
}
if (metadataQuery.isSuccess && courseHomeMetaQuery.data?.courseAccess?.hasAccess) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: give this bridge the same failure branch as useCourseStatusBridge.

There's no error case here, so a failed fetch lands in the else as courseStatus: 'denied' with errorMessage/errorCode still nulled by fetchCourseRequest, and the exit page shows TabPage's generic failure text where fetchCourse surfaced the 403 detail.

errorMessage: is403 ? (error?.response?.data?.detail ?? null) : null,
errorCode: is403 ? (error?.response?.data?.error_code ?? null) : null,
}));
}, [courseId, metadataQuery, outlineQuery, courseHomeMetaQuery, dispatch]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: depend on the derived values (metadataQuery.status, courseHomeMetaQuery.data?.courseAccess?.hasAccess) rather than the query results, here and at line 72.

useQuery returns observer.trackResult(result), a fresh object every render, so these deps never compare equal and the effect re-dispatches on every render of CoursewareContainer - harmless only while the immer reducers no-op on unchanged values.

useCourseExitStatusBridge(courseId, metadataQuery, courseHomeMetaQuery);

return (
<TabPage

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: swap in TabWithTimer.

The old TabContainer wrapper rendered TabWithTimer, which adds <OuterExamTimer courseId={...} /> (TabWithTimer.tsx:6-12), so the exit page silently loses it and becomes the only tab not on that wrapper.

}
dispatch(fetchCourseFailure({ courseId, errorMessage, errorCode }));
});
try {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth a small test for the one fetch left here.

The deleted describe('Test fetchCourse') block was the only place asserting coursewareOutlineSidebarSettings and the toggles' logError, so nothing now covers fetchCourse's surviving responsibility - and the PR description still lists redux.test.js as covering it.

@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from 332f52e to 52b714e Compare August 24, 2026 21:45
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from 52b714e to e6c5f4e Compare August 24, 2026 21:53
Base automatically changed from bsmith/react-query-courseware-container-typescript to master August 24, 2026 23:00
Extend the transitional QueryCache bridge so a query can mirror collection
results (and several model targets) into the models store, not just a single
addModel. Adds a `meta.models` list — each entry runs a model-store action
(addModel/updateModel/addModelsMap/updateModelsMap/updateModels) with an optional
`source` key into the result — while keeping the existing `{ modelType, courseId }`
single form. No runtime effect until a query opts in (wired up in the courseware
metadata conversion that follows in this PR).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch 4 times, most recently from 97c0d3e to e11cc01 Compare August 25, 2026 23:25
@brian-smith-tcril brian-smith-tcril linked an issue Aug 25, 2026 that may be closed by this pull request
Convert fetchCourse's metadata/outline/courseHomeMeta fetches to React Query, mirroring
results into the model store via the bridge so the existing useModel readers keep working.

- Query hooks: useCoursewareMetadata + useCoursewareOutline (courseware/data/apiHooks +
  queryKeys); reuse useCourseHomeMeta (now typed — enabled guard + courseAccess). courseId is
  string|undefined from useParams with an `enabled` guard; key factories stay strict string.
- Status: CoursewareContainer and CourseExit call transitional bridges
  (courseware/data/statusBridge) that mirror the combined query state into
  state.courseware.courseStatus/courseId, so the redirect helpers/selectors, TabPage, and the
  exit-page children keep working until they move to React Query.
- fetchCourse is thinned to just the sidebar-toggles fetch (its full conversion is #2013); its
  model writes move to the model-store bridge and its status derivation to the status bridges.
- Query error logging: the app QueryCache onError logs failures; a query can override the level
  per HTTP status via meta.logStatusAs (the outline's expected 403 -> logInfo). meta is typed
  globally via a Register.queryMeta augmentation, so onError and the bridge read it cast-free.
- useIFrameBehavior's post-event refetch invalidates the courseware queries instead of
  dispatching fetchCourse.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-courseware-metadata branch from e11cc01 to 1d711f2 Compare August 26, 2026 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants