Skip to content

refactor: convert the dates tab to React Query - #1987

Open
brian-smith-tcril wants to merge 1 commit into
bsmith/react-query-tabpage-typescriptfrom
bsmith/react-query-course-home-dates-tab
Open

refactor: convert the dates tab to React Query#1987
brian-smith-tcril wants to merge 1 commit into
bsmith/react-query-tabpage-typescriptfrom
bsmith/react-query-course-home-dates-tab

Conversation

@brian-smith-tcril

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

Copy link
Copy Markdown
Contributor

Summary

Converts the dates tab from Redux thunks to React Query, per OEP-0067 ADR-0010. Part of the Redux → React Query migration (#1946), Phase 3 (course-home), stacked on the TabPage TypeScript conversion (#1986, itself on the CTA-toast conversion #1982). The dates tab becomes self-wrapping — it renders TabPage itself and owns its data loading via query hooks — the shape CoursewareContainer already uses.

Behavior is preserved — timeline, suggested-schedule alerts, access-denied redirects, and the "Shift due dates" refresh — verified with the full test suite and a live manual pass (the banner clears and dates shift via the RQ refetch, plus the toast).

What changed

  • Self-wrapping DatesTab: renders <TabWithTimer> (→ TabPage) and owns its data via useCourseHomeMeta + useDatesTabData (course-home/data/apiHooks.ts); courseId from useParams. Its wrapper line leaves index.jsx; TabContainer is untouched and keeps serving the unconverted tabs.
  • TabPage status derivation: courseStatus becomes a union — StatusValue | { metadataQuery, tabDataQuery }. A converted tab passes its queries and TabPage derives the view (deriveView → loading/error/denied/loaded), reading access straight off the metadata query. Not-yet-converted (Redux) callers still pass a status string; that branch drops when courseware converts. Builds on the TypeScript conversion in refactor: convert TabPage to TypeScript #1986.
  • TabWithTimer: a small wrapper rendering TabPage + OuterExamTimer, so the shared TabPage doesn't import @edx/frontend-lib-special-exams (which the Stage-2 frontend-base port is gated on). TabContainer renders TabWithTimer; CoursewareContainer keeps rendering plain TabPage (no timer, exactly as today).
  • ShiftDatesAlert invalidates the dates query on reset — it owns the resetDeadlines mutation, so it owns invalidating the data that mutation affects. Its fetch prop is now optional (the still-Redux outline tab passes fetchOutlineTab as its transitional refresh; the dates tab passes nothing). The dates subtree (Timeline, Day, ShiftDatesAlert, UpgradeToShiftDatesAlert, UpgradeToCompleteAlert) reads courseId from useParams.
  • Transitional model-store bridge: data/queryKeys.ts + data/modelStoreBridge.ts mirror query results into the existing useModel readers via a QueryCache onSuccess keyed off a meta tag, so the subtree keeps its useModel reads unchanged mid-migration (removed with model-store in Phase 5). createTestQueryClient wires the same bridge when given a store.
  • Redux removed: the fetchDatesTab thunk and its data/index.js re-export.

Testing

Automated: npm run types, npm run lint, and the full npm test suite (106 suites, 888 passing, 3 pre-existing skips) pass. DatesTab.test.jsx (the axios-level parity guard) keeps its assertions — only the TabContainer harness is dropped and it renders <DatesTab /> directly, incl. "handles shift due dates click" (the invalidate-driven refetch clears the banner). OutlineTab.test.jsx gains a /course/:courseId/home route so the now-useParams shared alerts resolve, plus a "shift due dates click" test covering outline's transitional dispatch(fetchOutlineTab) refresh; createTestQueryClient(store) wires the model-store bridge for the subtree's useModel reads; the redux.test.js fetchDatesTab block is removed (its fetchTab coverage lives on via fetchOutlineTab).

Manual (self-paced course with a seeded missed deadline): the timeline and suggested-schedule alerts render, and clicking "Shift due dates" shows the success toast and clears the banner + shifts the dates on its own — the invalidateQueries(datesTab) → refetch → bridge → missedDeadlines: false path, matching the old thunk-dispatch behavior, with no console errors.

Decisions

Full decision log

Decisions — Redux → React Query: the dates tab (#1984)

Working notes for this PR (part of the wider Redux → React Query migration,
#1946). Not checked in — referenced when opening the PR. First slice of
#1975 (course-home tab data), stacked on the CTA-toast conversion (#1982). This
is the pattern-setter for the remaining course-home tabs.

Scope: one tab per PR, each tab self-wrapping

Decision. Convert one tab at a time. The tab component becomes
self-wrapping — it renders <TabPage> itself and owns its data-loading via
React Query hooks. TabContainer is not modified; the only index.jsx
change is removing the converting tab's wrapper line.

Why. Today index.jsx wraps each tab in a generic TabContainer that
(1) dispatches the tab's fetch thunk on mount and (2) reads courseStatus/
courseId from the slice. Under React Query the fetch-on-mount role
evaporates — a component that calls a query hook triggers the fetch itself —
so TabContainer's remaining job is just deriving status and rendering
<TabPage>. CoursewareContainer already renders <TabPage> directly (no
TabContainer), so "the page owns its data and renders TabPage" is the
established courseware shape; the course-home tabs converge onto it.

Alternatives rejected.

  • An upfront "move every wrapper into its tab" setup PR. It'd be a
    behavior-neutral refactor that also plants 5–6 copies of transitional Redux
    fetch-on-mount boilerplate, each rewritten later. The one thing it buys —
    future PRs not touching index.jsx — is a one-line route swap that belongs
    with each tab's own conversion anyway.
  • An injected useTabData seam on TabContainer. Keeping TabContainer and
    parameterizing it by a hook adds coupling and a transitional Redux adapter for
    the courseware route; self-wrapping needs neither. TabContainer simply
    lingers for the unconverted routes and is deleted with course-exit in Phase 4.

Lead tab: dates (after a detour through live)

Decision. Dates is the pattern-setter.

How we got here. We first picked dates ("one tab end-to-end"), then went
looking for a cheaper pattern-setter and explored live: it's the smallest
tab that still exercises a tab-data query (discussion has no tab-data fetch, so
it wouldn't prototype the useXTabData hook the heavier tabs need). But live
turns out to have zero test coverage — no LiveTab.test.jsx, and index.test
only mocks it as a string. Converting it would mean writing its safety net from
scratch, which is a weak pattern-setter and cuts against leaning on existing
tests. Dates, by contrast, has a 365-line DatesTab.test.jsx that mocks at the
axios layer and is already wired with QueryClientProvider + a
/course/:courseId/dates router — so it survives the thunk→RQ conversion with
its assertions intact and becomes the behavior-parity guard. So we came back to
dates; live is deferred to a later "backfill the tests, then convert" PR.

Data bridge: a synchronous QueryCache sync into the model store

Decision. The dates render subtree (DatesTab, Timeline, Day,
ShiftDatesAlert, UpgradeToShiftDatesAlert, and UpgradeToCompleteAlert
reached via BannerDatesUpgradeSlot) keeps its existing useModel(...)
reads unchanged. React Query is the fetch source and mirrors both courseHomeMeta
and the dates payload into model-store via a global QueryCache onSuccess
callback
(in data/modelStoreBridge.ts, wired into the app QueryClient in
index.jsx): any query that tags itself meta: { modelType, courseId } gets a
dispatch(addModel(...)) on success. The query hooks stay pure useQuery (one
meta line, no Redux), the components never touch the bridge, and courseStatus
is derived purely from the query state — final form, no model-store dependency.

Why a QueryCache callback, not a per-hook useEffect. The store must be
populated before the render that flips courseStatus to LOADED, or the shared
readers crash — Timeline does courseDateBlocks.forEach(...) and LoadedTabPage
does tabs.filter(...), both unguarded. A per-hook useEffect bridge writes
after that render → a one-frame gap → crash. The cache callback fires as the
query resolves, before observers re-render, so useModel is populated in time.
(The old thunk had this invariant too — it called addModel before dispatching
LOADED; the cache callback restores it.)

Why not gate courseStatus on the store instead. Deriving LOADED from "is
the data in the store yet" also avoids the crash, but it invents a new
model-store dependency for loaded-state — the exact coupling we're removing.
Keeping courseStatus query-derived + a synchronous bridge fixes the timing
without that regression.

Why meta + a global cache callback (the two reasons this is worth it).

  1. It's fully transitional — once Redux / model-store is gone, the meta
    tags and the entire QueryCache config go with it
    (Phase 5, Dissolve the model-store normalized cache #1977). It adds
    no permanent API surface; it exists only to keep useModel readers alive
    mid-migration.
  2. It keeps the migration clean and localized. The subtree keeps its
    useModel reads (only courseIduseParams), and the shared
    TabPage/LoadedTabPage, the shared alerts, and the outline tab are all left
    untouched. The whole bridge is one file + a one-line QueryClient change, so
    each subsequent tab is a small PR too.

Ecosystem note. meta and QueryCache/MutationCache callbacks are core,
documented React Query (v4+; we're on v5.101), but git grep across the openedx
RQ adopters (learner-dashboard, authn, authoring, course-authoring) finds
no use of meta or global cache callbacks — they use RQ per-hook, and even
its canonical use (global error handling) is absent. So this is the first place to
introduce that class of RQ pattern here; it's chosen for the two reasons above,
not for local precedent. The precedent-matching alternative — prop-threading
courseHomeMeta into TabPage/LoadedTabPage and the banner data into the shared
alerts — was rejected because it balloons this PR into shared-component + outline
changes.

courseId from useParams

Decision. The subtree reads courseId from useParams() rather than
useSelector(state.courseHome).

Why. Once the dates route stops dispatching its thunk, the slice no longer
carries the loaded courseId for this route. useParams is the source the
already-loaded page has (decoded via DecodePageRoute, as CoursewareSearch
relies on today). The existing test already renders through a
/course/:courseId/dates route, so this is covered without new test files.

Reset-deadlines refresh: ShiftDatesAlert invalidates its own queries

Decision. ShiftDatesAlert owns the resetDeadlines mutation, so it also owns
the refresh: on success it invalidates the dates query itself
(queryClient.invalidateQueries(datesTab)). Its fetch prop is kept but made
optional and transitional — the still-Redux outline tab passes
fetch={fetchOutlineTab} so its model-store data is refetched too; the dates tab
passes nothing.

Why. The component doing the write is the right place to invalidate the queries
that write affects — the canonical React Query pattern, not coupling (the alert
already owns the deadline data it's mutating). Shifting deadlines invalidates the
dates data regardless of which tab triggered it, so invalidating the dates query
even from the outline tab is correct (it refetches when dates is next viewed). This
also lets DatesTab pass no refresh prop and leaves OutlineTab's call site
unchanged. (The toast still fires from inside useResetDeadlines, unchanged from
#1982.)

Rejected: a refresh callback the tab supplies (onReset/fetch). It pushes the
"invalidate the query for this mutation" responsibility onto callers that don't own
the mutation, keeps the shared alert ignorant of data it's literally changing, and
read oddly at the call site (onReset for a "Shift due dates" button).

End state. When outline converts to RQ, ShiftDatesAlert invalidates the outline
query too (invalidate all affected queries), and the fetch prop + useDispatch are
deleted — the alert goes fully Redux-free.

OuterExamTimer: a TabWithTimer wrapper, not a TabPage prop

Decision. The proctored-exam OuterExamTimer — which TabContainer renders on
every tab it wraps — moves into a small TabWithTimer component (TabPage + the
timer as its first child). TabContainer and the self-wrapping tabs (DatesTab)
render TabWithTimer; CoursewareContainer keeps rendering plain TabPage.

Why not a withTimer prop on TabPage. OuterExamTimer comes from
@edx/frontend-lib-special-exams, and the Stage-2 frontend-base port (#1905) is gated
on that library. A prop makes the shared TabPage import special-exams directly; a
wrapper keeps that dependency out of the component that has to port cleanly, and reads
as composition ("TabPage plus the timer") rather than a boolean that conditionally
injects one specific external child. Rendering it per-tab was also rejected — it's
cross-cutting, and duplicating the line into every self-wrapping tab scatters it.

Behavior is preserved exactly. Today OuterExamTimer renders only in
TabContainer; CoursewareContainer (the in-unit courseware view) has none. Moving
it into TabWithTimer keeps that split — every TabContainer tab and converted tab
gets it, CoursewareContainer (plain TabPage) still doesn't. Hoisting it into
TabPage unconditionally would have newly rendered it on courseware.

apiHooks.ts stays pure; TabPage owns courseStatus derivation

apiHooks.ts holds only thin useQuery/useMutation wrappers around api.js
(each query hook adds a single meta: { modelType, courseId } tag so the
QueryCache bridge mirrors it — no Redux, no status constants in the data layer).
This follows the Phase-0 recommendations precedent (#1967): the data layer stays a
bare useQuery, and status handling lives with the consumer (there,
CourseRecommendations.jsx branches on isPending/isError; track.js maps
isError ? FAILED : LOADED).

courseStatus is derived in TabPage, via a union-typed prop. TabPage is
the component that renders from the status, so it owns the derivation:

type CourseStatus = StatusValue | {
  metadataQuery: UseQueryResult<{ courseAccess?: { hasAccess: boolean } }>;
  tabDataQuery: UseQueryResult;
};

A converted tab hands TabPage its two queries
(courseStatus={{ metadataQuery, tabDataQuery }}); a not-yet-converted (Redux) caller
(TabContainer, courseware) still passes a plain status string.
deriveView(courseStatus) normalizes either input into three booleans —
{ isLoading, isError, isDenied } — and the render tree branches on those. Access is
read straight off the metadata query (metadataQuery.data?.courseAccess?.hasAccess), so
deriveView doesn't depend on the model-store bridge — the bridge's timing only still
matters for the subtree's useModel reads. The query's data is typed inline to
just the field this file reads: the metadata is untyped JS, and a named one-field
CourseMetadata interface would misrepresent the real shape (and rot as a stub). Named
metadataQuery/tabDataQuery, not meta — which collides with React Query's own meta.

Booleans, not a status constant (don't re-manufacture the Redux vocabulary).
TabPage never passes the status to a child — it only branches on it to decide what to
render. So the query path derives the render booleans directly and never mentions the
LOADING/LOADED/DENIED constants; those appear in exactly one place — the
transitional typeof courseStatus === 'string' branch of deriveView, which maps a
legacy Redux status string onto the same booleans. That branch, the StatusValue union
member, and the constants import are deleted together when courseware (the last string
caller) converts, leaving deriveView purely query-native.

Ordering: access is resolved before tab-data (matches the thunk's short-circuit).
The metadata call is authoritative for access, so deriveView resolves the metadata
phase — metadata error → failed; metadata in-flight → loading; !hasAccess → denied —
before it looks at tabData at all. This mirrors the fetchTab thunk, which denied
"regardless of the tabDataResult" (thunks.js). It matters in two cases a combined
metadataQuery.isError || tabDataQuery.isError ordering gets wrong: (1) no access + a non-auth
tabData error
(e.g. a 500 while metadata says hasAccess: false) — deny, don't
show failed; and (2) no access while tabData is still loading — deny immediately
rather than flashing loading first. (getDatesTabData swallows 401/403 → {}, so the
common no-access path never errors on tab-data anyway; this ordering covers the
residual cases.) Keep the two phases separate — don't recombine the guards.

Render tree: a redirect gate, then a render function per part. isDenied drives
the access-denied redirect (getAccessDeniedRedirectUrl returns no URL for enroll/auth
errors on the outline tab, so that case falls through and renders the page — where
the outline shows its enroll/upgrade CTAs). The page content keys off
shouldRenderContent = !isLoading && !isError (loaded, or denied-without-redirect).
Each part is a render function — renderToast, renderTourButton, renderLoading,
renderLoadedTabPage, renderError — and the JSX gates each call with the relevant
boolean (shouldRenderContent / isLoading / isError), so the return reads as the
render structure top-to-bottom and the primary conditions aren't buried inside the
functions. The functions are called in their original positions, so DOM order is
unchanged; the two with a secondary condition keep it internally (renderTourButton
checks metadataModel, renderLoadedTabPage narrows courseId). Loading / content /
error are mutually exclusive by construction, so at most one renders.

Why TabPage reads courseAccess now (it didn't in the string path). In the
Redux flow this derivation never lived in TabPage: the fetchTab thunk
(data/thunks.js) computed the status — metadata rejected → FAILED;
!courseAccess.hasAccessfetchTabDeniedDENIED; tab-data rejected →
FAILED; else LOADED — and stored the resolved string in the slice, which
TabContainer passed down. So TabPage only ever rendered a finished status and
never inspected courseAccess for it (it read courseAccess solely for the
access-denied redirect URL). The RQ path hands TabPage the raw queries instead of
a pre-resolved string, so deriveView is exactly that thunk branch relocated
into TabPage — which is why the courseAccess.hasAccessDENIED check now
appears here. The value is the same one TabPage already reads from
useModel('courseHomeMeta'), so no new data source is introduced.

Why in TabPage, not elsewhere. Tried and rejected, in order:

  • an inline block in each tab — a four-branch computation wedged into the
    component body reads badly and would be copy-pasted across five tabs;
  • a standalone courseTabStatus.ts helper — a whole file for one derivation
    felt like overkill;
  • a useCourseHomeTab composing hook — drags the status constants (and, in an
    earlier design, the bridge) back into the data layer — the clutter we'd just
    separated out.

Putting it in TabPage writes the logic once for every tab, keeps each tab body to
courseStatus={{ metadataQuery, tabDataQuery }}, and adds no new file.

TabPage is already TypeScript (#1986). The .jsx → .tsx conversion landed in the
lower PR (#1986); this PR only adds the CourseStatus union member and deriveView on
top of it. TabPage is shared and now goes dual-mode during the migration
(string | queries); the string branch is removed once courseware — the last string
caller — converts. getAccessDeniedRedirectUrl and useModel are untyped JS, so adding
the union doesn't ripple into them.

Accepted trade-off: 403 detail on a hard failure

Today fetchTabFailure stashes a 403 body's errorMessage/errorCode into the
slice and TabPage renders it. On the RQ path TabPage falls back to the
generic failure message for the dates route. Access errors normally surface via
DENIED (from metadata), so this only affects rare unexpected failures —
accepted for this PR rather than threading the detail through.

Tests (preserve the teeth)

  • DatesTab.test.jsx is the parity guard. Because it mocks at the axios
    layer
    and already renders through QueryClientProvider + a
    /course/:courseId/dates route, the only harness change is dropping the
    <TabContainer> wrapper (render <DatesTab/> directly) and removing the
    now-unused fetchDatesTab/TabContainer imports. All assertions stay
    verbatim
    — including "handles shift due dates click," where the
    invalidate-driven refetch picks up the swapped missedDeadlines: false mock
    and the banner clears. Adjust await timing only if needed; do not weaken
    assertions.
  • course-home/data/apiHooks.test.tsx — cover useCourseHomeMeta/
    useDatesTabData fetch (axios MockAdapter): the hooks return the mapped data.
    Mirroring into the model store is the QueryCache bridge's job (in
    modelStoreBridge.ts), not the hooks', so it's proven end-to-end by
    DatesTab.test.jsx (the subtree only renders real content if the bridge
    populated useModel); a focused modelStoreBridge test for the
    metaaddModel mapping is optional.
  • TabPage.test — the existing string-courseStatus cases stay valid (the
    union still accepts a string). Add the query/combo path: deriveView producing
    isLoading/isError/isDenied from { meta, tabData } + courseAccess
    this is where the status matrix has its teeth (also exercised end-to-end via
    DatesTab.test).
  • index.test.jsx — the dates route now renders the Dates Tab mock instead
    of the Tab Container mock; update that one assertion (still has teeth: it
    asserts the dates path renders DatesTab directly).
  • OutlineTab.test.jsx — its fetchAndRender now wraps OutlineTab in a
    /course/:courseId/home route so the shared alerts' useParams resolves, and a
    new "shift due dates click" test covers outline's transitional
    dispatch(fetchOutlineTab) refresh — the one line the dates path no longer
    exercises (dates invalidates its query instead).

Verification

nvm use && npm run types && npm run lint && npm test && npm run buildtypes
matters more now that TabPage is TypeScript. Targeted first (DatesTab,
TabPage, apiHooks, OutlineTab, index, and TabContainer still green), then
the full suite + build. git grep "fetchDatesTab" comes back clean (gone from
source), TabPage.jsx is gone (only TabPage.tsx), and the dates subtree no
longer reads useSelector(state.courseHome) for courseId.

Manual testing

The dates tab renders against mocked queries in the suite; for a live sanity pass,
the "Shift due dates" banner needs a self-paced course with a missed
suggested-schedule deadline (seeded via edx-when per the toast PR's notes, since
the authoring MFE can't set relative due dates). Compare master vs. branch:
timeline + suggested-schedule alerts render identically, and "Shift due dates"
refreshes the banner (now via invalidateQueries → refetch → bridge) and shows
the toast. Other tabs are untouched.

Closes #1984

🤖 Generated with Claude Code

@codecov

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.77%. Comparing base (b194e65) to head (86fa73d).

Additional details and impacted files
@@                            Coverage Diff                            @@
##           bsmith/react-query-tabpage-typescript    #1987      +/-   ##
=========================================================================
- Coverage                                  92.86%   92.77%   -0.10%     
=========================================================================
  Files                                        360      363       +3     
  Lines                                       5890     5938      +48     
  Branches                                    1404     1418      +14     
=========================================================================
+ Hits                                        5470     5509      +39     
- Misses                                       402      407       +5     
- Partials                                      18       22       +4     

☔ 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 force-pushed the bsmith/react-query-course-home-dates-tab branch 2 times, most recently from ed7a1e1 to 1544e39 Compare August 9, 2026 04:50
Convert the dates tab off Redux thunks to React Query. The tab becomes
self-wrapping: it renders TabPage itself and owns its data loading via query
hooks.

- DatesTab renders TabWithTimer and owns its data via useCourseHomeMeta +
  useDatesTabData; courseId comes from useParams.
- TabPage: courseStatus becomes a union (StatusValue | { metadataQuery,
  tabDataQuery }); a converted tab passes its queries and TabPage derives the
  view (loading/error/denied/loaded), reading access from the metadata query.
  Builds on the TabPage TypeScript conversion in the layer below (#1986).
- TabWithTimer wraps TabPage with OuterExamTimer, keeping
  @edx/frontend-lib-special-exams out of the shared TabPage; TabContainer uses
  it, CoursewareContainer keeps rendering plain TabPage.
- ShiftDatesAlert invalidates the dates query on reset; its fetch prop is now
  optional (the still-Redux outline tab's transitional refresh). The dates
  subtree reads courseId from useParams.
- Transitional model-store bridge (data/queryKeys, data/modelStoreBridge)
  mirrors query results into the existing useModel readers until model-store is
  removed; createTestQueryClient wires it when given a store.
- Drop the now-unused fetchDatesTab thunk and its re-export.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-course-home-dates-tab branch from 1544e39 to 86fa73d Compare August 9, 2026 04:51
@brian-smith-tcril
brian-smith-tcril marked this pull request as ready for review August 9, 2026 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert the dates tab to React Query

1 participant