refactor: convert the dates tab to React Query - #1987
Open
brian-smith-tcril wants to merge 1 commit into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
brian-smith-tcril
force-pushed
the
bsmith/react-query-course-home-dates-tab
branch
2 times, most recently
from
August 9, 2026 04:50
ed7a1e1 to
1544e39
Compare
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
force-pushed
the
bsmith/react-query-course-home-dates-tab
branch
from
August 9, 2026 04:51
1544e39 to
86fa73d
Compare
brian-smith-tcril
marked this pull request as ready for review
August 9, 2026 04:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
TabPageitself and owns its data loading via query hooks — the shapeCoursewareContaineralready 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
DatesTab: renders<TabWithTimer>(→TabPage) and owns its data viauseCourseHomeMeta+useDatesTabData(course-home/data/apiHooks.ts);courseIdfromuseParams. Its wrapper line leavesindex.jsx;TabContaineris untouched and keeps serving the unconverted tabs.TabPagestatus derivation:courseStatusbecomes a union —StatusValue | { metadataQuery, tabDataQuery }. A converted tab passes its queries andTabPagederives 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 renderingTabPage+OuterExamTimer, so the sharedTabPagedoesn't import@edx/frontend-lib-special-exams(which the Stage-2 frontend-base port is gated on).TabContainerrendersTabWithTimer;CoursewareContainerkeeps rendering plainTabPage(no timer, exactly as today).ShiftDatesAlertinvalidates the dates query on reset — it owns theresetDeadlinesmutation, so it owns invalidating the data that mutation affects. Itsfetchprop is now optional (the still-Redux outline tab passesfetchOutlineTabas its transitional refresh; the dates tab passes nothing). The dates subtree (Timeline,Day,ShiftDatesAlert,UpgradeToShiftDatesAlert,UpgradeToCompleteAlert) readscourseIdfromuseParams.data/queryKeys.ts+data/modelStoreBridge.tsmirror query results into the existinguseModelreaders via aQueryCacheonSuccesskeyed off ametatag, so the subtree keeps itsuseModelreads unchanged mid-migration (removed with model-store in Phase 5).createTestQueryClientwires the same bridge when given a store.fetchDatesTabthunk and itsdata/index.jsre-export.Testing
Automated:
npm run types,npm run lint, and the fullnpm testsuite (106 suites, 888 passing, 3 pre-existing skips) pass.DatesTab.test.jsx(the axios-level parity guard) keeps its assertions — only theTabContainerharness is dropped and it renders<DatesTab />directly, incl. "handles shift due dates click" (theinvalidate-driven refetch clears the banner).OutlineTab.test.jsxgains a/course/:courseId/homeroute so the now-useParamsshared alerts resolve, plus a "shift due dates click" test covering outline's transitionaldispatch(fetchOutlineTab)refresh;createTestQueryClient(store)wires the model-store bridge for the subtree'suseModelreads; theredux.test.jsfetchDatesTabblock is removed (itsfetchTabcoverage lives on viafetchOutlineTab).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: falsepath, 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 viaReact Query hooks.
TabContaineris not modified; the onlyindex.jsxchange is removing the converting tab's wrapper line.
Why. Today
index.jsxwraps each tab in a genericTabContainerthat(1) dispatches the tab's fetch thunk on mount and (2) reads
courseStatus/courseIdfrom the slice. Under React Query the fetch-on-mount roleevaporates — a component that calls a query hook triggers the fetch itself —
so
TabContainer's remaining job is just deriving status and rendering<TabPage>.CoursewareContaineralready renders<TabPage>directly (noTabContainer), so "the page owns its data and renders TabPage" is theestablished courseware shape; the course-home tabs converge onto it.
Alternatives rejected.
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 belongswith each tab's own conversion anyway.
useTabDataseam onTabContainer. KeepingTabContainerandparameterizing it by a hook adds coupling and a transitional Redux adapter for
the courseware route; self-wrapping needs neither.
TabContainersimplylingers 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
useXTabDatahook the heavier tabs need). But liveturns out to have zero test coverage — no
LiveTab.test.jsx, andindex.testonly 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.jsxthat mocks at theaxios layer and is already wired with
QueryClientProvider+ a/course/:courseId/datesrouter — so it survives the thunk→RQ conversion withits 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
QueryCachesync into the model storeDecision. The dates render subtree (
DatesTab,Timeline,Day,ShiftDatesAlert,UpgradeToShiftDatesAlert, andUpgradeToCompleteAlert—reached via
BannerDatesUpgradeSlot) keeps its existinguseModel(...)reads unchanged. React Query is the fetch source and mirrors both
courseHomeMetaand the
datespayload intomodel-storevia a globalQueryCacheonSuccesscallback (in
data/modelStoreBridge.ts, wired into the appQueryClientinindex.jsx): any query that tags itselfmeta: { modelType, courseId }gets adispatch(addModel(...))on success. The query hooks stay pureuseQuery(onemetaline, no Redux), the components never touch the bridge, andcourseStatusis derived purely from the query state — final form, no model-store dependency.
Why a
QueryCachecallback, not a per-hookuseEffect. The store must bepopulated before the render that flips
courseStatustoLOADED, or the sharedreaders crash —
TimelinedoescourseDateBlocks.forEach(...)andLoadedTabPagedoes
tabs.filter(...), both unguarded. A per-hookuseEffectbridge writesafter that render → a one-frame gap → crash. The cache callback fires as the
query resolves, before observers re-render, so
useModelis populated in time.(The old thunk had this invariant too — it called
addModelbefore dispatchingLOADED; the cache callback restores it.)Why not gate
courseStatuson the store instead. DerivingLOADEDfrom "isthe 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
courseStatusquery-derived + a synchronous bridge fixes the timingwithout that regression.
Why
meta+ a global cache callback (the two reasons this is worth it).model-storeis gone, themetatags and the entire
QueryCacheconfig go with it (Phase 5, Dissolve the model-store normalized cache #1977). It addsno permanent API surface; it exists only to keep
useModelreaders alivemid-migration.
useModelreads (onlycourseId→useParams), and the sharedTabPage/LoadedTabPage, the shared alerts, and the outline tab are all leftuntouched. The whole bridge is one file + a one-line
QueryClientchange, soeach subsequent tab is a small PR too.
Ecosystem note.
metaandQueryCache/MutationCachecallbacks are core,documented React Query (v4+; we're on v5.101), but
git grepacross the openedxRQ adopters (
learner-dashboard,authn,authoring,course-authoring) findsno use of
metaor global cache callbacks — they use RQ per-hook, and evenits 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
courseHomeMetaintoTabPage/LoadedTabPageand the banner data into the sharedalerts — was rejected because it balloons this PR into shared-component + outline
changes.
courseIdfromuseParamsDecision. The subtree reads
courseIdfromuseParams()rather thanuseSelector(state.courseHome).Why. Once the dates route stops dispatching its thunk, the slice no longer
carries the loaded
courseIdfor this route.useParamsis the source thealready-loaded page has (decoded via
DecodePageRoute, asCoursewareSearchrelies on today). The existing test already renders through a
/course/:courseId/datesroute, so this is covered without new test files.Reset-deadlines refresh:
ShiftDatesAlertinvalidates its own queriesDecision.
ShiftDatesAlertowns theresetDeadlinesmutation, so it also ownsthe refresh: on success it invalidates the dates query itself
(
queryClient.invalidateQueries(datesTab)). Itsfetchprop is kept but madeoptional and transitional — the still-Redux outline tab passes
fetch={fetchOutlineTab}so its model-store data is refetched too; the dates tabpasses 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
DatesTabpass no refresh prop and leavesOutlineTab's call siteunchanged. (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 (
onResetfor a "Shift due dates" button).End state. When outline converts to RQ,
ShiftDatesAlertinvalidates the outlinequery too (invalidate all affected queries), and the
fetchprop +useDispatcharedeleted — the alert goes fully Redux-free.
OuterExamTimer: aTabWithTimerwrapper, not aTabPagepropDecision. The proctored-exam
OuterExamTimer— whichTabContainerrenders onevery tab it wraps — moves into a small
TabWithTimercomponent (TabPage+ thetimer as its first child).
TabContainerand the self-wrapping tabs (DatesTab)render
TabWithTimer;CoursewareContainerkeeps rendering plainTabPage.Why not a
withTimerprop onTabPage.OuterExamTimercomes from@edx/frontend-lib-special-exams, and the Stage-2 frontend-base port (#1905) is gatedon that library. A prop makes the shared
TabPageimport special-exams directly; awrapper 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
OuterExamTimerrenders only inTabContainer;CoursewareContainer(the in-unit courseware view) has none. Movingit into
TabWithTimerkeeps that split — everyTabContainertab and converted tabgets it,
CoursewareContainer(plainTabPage) still doesn't. Hoisting it intoTabPageunconditionally would have newly rendered it on courseware.apiHooks.tsstays pure;TabPageownscourseStatusderivationapiHooks.tsholds only thinuseQuery/useMutationwrappers aroundapi.js(each query hook adds a single
meta: { modelType, courseId }tag so theQueryCachebridge 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.jsxbranches onisPending/isError;track.jsmapsisError ? FAILED : LOADED).courseStatusis derived inTabPage, via a union-typed prop.TabPageisthe component that renders from the status, so it owns the derivation:
A converted tab hands
TabPageits 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 isread straight off the metadata query (
metadataQuery.data?.courseAccess?.hasAccess), soderiveViewdoesn't depend on the model-store bridge — the bridge's timing only stillmatters for the subtree's
useModelreads. The query's data is typed inline tojust the field this file reads: the metadata is untyped JS, and a named one-field
CourseMetadatainterface would misrepresent the real shape (and rot as a stub). NamedmetadataQuery/tabDataQuery, notmeta— which collides with React Query's ownmeta.Booleans, not a status constant (don't re-manufacture the Redux vocabulary).
TabPagenever passes the status to a child — it only branches on it to decide what torender. So the query path derives the render booleans directly and never mentions the
LOADING/LOADED/DENIEDconstants; those appear in exactly one place — thetransitional
typeof courseStatus === 'string'branch ofderiveView, which maps alegacy Redux status string onto the same booleans. That branch, the
StatusValueunionmember, and the
constantsimport are deleted together when courseware (the last stringcaller) converts, leaving
deriveViewpurely query-native.Ordering: access is resolved before tab-data (matches the thunk's short-circuit).
The metadata call is authoritative for access, so
deriveViewresolves the metadataphase — metadata error → failed; metadata in-flight → loading;
!hasAccess→ denied —before it looks at
tabDataat all. This mirrors thefetchTabthunk, which denied"regardless of the tabDataResult" (thunks.js). It matters in two cases a combined
metadataQuery.isError || tabDataQuery.isErrorordering gets wrong: (1) no access + a non-authtabDataerror (e.g. a 500 while metadata sayshasAccess: false) — deny, don'tshow failed; and (2) no access while
tabDatais still loading — deny immediatelyrather than flashing loading first. (
getDatesTabDataswallows 401/403 →{}, so thecommon 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.
isDenieddrivesthe access-denied redirect (
getAccessDeniedRedirectUrlreturns no URL for enroll/autherrors 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 relevantboolean (
shouldRenderContent/isLoading/isError), so the return reads as therender 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 (
renderTourButtonchecks
metadataModel,renderLoadedTabPagenarrowscourseId). Loading / content /error are mutually exclusive by construction, so at most one renders.
Why
TabPagereadscourseAccessnow (it didn't in the string path). In theRedux flow this derivation never lived in
TabPage: thefetchTabthunk(
data/thunks.js) computed the status — metadata rejected →FAILED;!courseAccess.hasAccess→fetchTabDenied→DENIED; tab-data rejected →FAILED; elseLOADED— and stored the resolved string in the slice, whichTabContainerpassed down. SoTabPageonly ever rendered a finished status andnever inspected
courseAccessfor it (it readcourseAccesssolely for theaccess-denied redirect URL). The RQ path hands
TabPagethe raw queries instead ofa pre-resolved string, so
deriveViewis exactly that thunk branch relocatedinto
TabPage— which is why thecourseAccess.hasAccess→DENIEDcheck nowappears here. The value is the same one
TabPagealready reads fromuseModel('courseHomeMeta'), so no new data source is introduced.Why in
TabPage, not elsewhere. Tried and rejected, in order:component body reads badly and would be copy-pasted across five tabs;
courseTabStatus.tshelper — a whole file for one derivationfelt like overkill;
useCourseHomeTabcomposing hook — drags the status constants (and, in anearlier design, the bridge) back into the data layer — the clutter we'd just
separated out.
Putting it in
TabPagewrites the logic once for every tab, keeps each tab body tocourseStatus={{ metadataQuery, tabDataQuery }}, and adds no new file.TabPageis already TypeScript (#1986). The.jsx → .tsxconversion landed in thelower PR (#1986); this PR only adds the
CourseStatusunion member andderiveViewontop of it.
TabPageis shared and now goes dual-mode during the migration(string | queries); the string branch is removed once courseware — the last string
caller — converts.
getAccessDeniedRedirectUrlanduseModelare untyped JS, so addingthe union doesn't ripple into them.
Accepted trade-off: 403 detail on a hard failure
Today
fetchTabFailurestashes a 403 body'serrorMessage/errorCodeinto theslice and
TabPagerenders it. On the RQ pathTabPagefalls back to thegeneric 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.jsxis the parity guard. Because it mocks at the axioslayer and already renders through
QueryClientProvider+ a/course/:courseId/datesroute, the only harness change is dropping the<TabContainer>wrapper (render<DatesTab/>directly) and removing thenow-unused
fetchDatesTab/TabContainerimports. All assertions stayverbatim — including "handles shift due dates click," where the
invalidate-driven refetch picks up the swappedmissedDeadlines: falsemockand the banner clears. Adjust
awaittiming only if needed; do not weakenassertions.
course-home/data/apiHooks.test.tsx— coveruseCourseHomeMeta/useDatesTabDatafetch (axios MockAdapter): the hooks return the mapped data.Mirroring into the model store is the
QueryCachebridge's job (inmodelStoreBridge.ts), not the hooks', so it's proven end-to-end byDatesTab.test.jsx(the subtree only renders real content if the bridgepopulated
useModel); a focusedmodelStoreBridgetest for themeta→addModelmapping is optional.TabPage.test— the existing string-courseStatuscases stay valid (theunion still accepts a string). Add the query/combo path:
deriveViewproducingisLoading/isError/isDeniedfrom{ 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 theDates Tabmock insteadof the
Tab Containermock; update that one assertion (still has teeth: itasserts the dates path renders
DatesTabdirectly).OutlineTab.test.jsx— itsfetchAndRendernow wrapsOutlineTabin a/course/:courseId/homeroute so the shared alerts'useParamsresolves, and anew "shift due dates click" test covers outline's transitional
dispatch(fetchOutlineTab)refresh — the one line the dates path no longerexercises (dates invalidates its query instead).
Verification
nvm use && npm run types && npm run lint && npm test && npm run build—typesmatters more now that
TabPageis TypeScript. Targeted first (DatesTab,TabPage,apiHooks,OutlineTab,index, andTabContainerstill green), thenthe full suite + build.
git grep "fetchDatesTab"comes back clean (gone fromsource),
TabPage.jsxis gone (onlyTabPage.tsx), and the dates subtree nolonger reads
useSelector(state.courseHome)forcourseId.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-whenper the toast PR's notes, sincethe 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 showsthe toast. Other tabs are untouched.
Closes #1984
🤖 Generated with Claude Code