refactor: convert progress tab from Redux to React Query - #2001
Draft
brian-smith-tcril wants to merge 1 commit into
Draft
Conversation
Make ProgressTab self-wrapping — it owns useCourseHomeMeta + a new
useProgressTabData query and renders TabWithTimer itself, mirroring the
dates- and outline-tab conversions. The body moves into a ProgressTabContent
component so the reads never run against empty data during the loading phase.
Every progress-subtree reader — the grade/certificate components, the
ProgressTabGradeBreakdownSlot, ProgressHeader, and ProgressTabContent — stops
calling useModel('progress') and reads the tab data from useProgressTabData via
a new useProgressData() hook. Their courseId reads move from useContextId() to
useParams, since the progress route no longer runs a fetch thunk to populate
state.courseHome.courseId.
useProgressTabData keeps a scoped meta: { modelType: 'progress' } so the
model-store bridge still feeds the one shared, tab-generic reader that resolves
to 'progress' — useAccessExpirationMasqueradeBanner, rendered via
InstructorToolbar; that reader migrates with the model-store dissolution (#1977,
tracked in #1999). The fetchProgressTab thunk and the now-dead isProgressTab
branch of TabContainer are deleted. The examsData exam-attempts write stays in
Redux, a separate surface to retire later.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## bsmith/react-query-course-home-outline-tab #2001 +/- ##
==============================================================================
- Coverage 92.96% 92.75% -0.22%
==============================================================================
Files 363 363
Lines 5945 5936 -9
Branches 1417 1414 -3
==============================================================================
- Hits 5527 5506 -21
- Misses 399 410 +11
- Partials 19 20 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
Convert the course-home progress tab from Redux to React Query. Part of the Redux → React Query migration (#1946), stacked on the outline-tab conversion (#1997). Closes #1998.
ProgressTabbecomes self-wrapping — it owns its data (useCourseHomeMeta+ a newuseProgressTabData) and rendersTabWithTimeritself, so theindex.jsxroute drops its<TabContainer tab="progress" fetch={fetchProgressTab} isProgressTab>wrapper for a bare<ProgressTab />andfetchProgressTabis deleted.What changed
apiHooks.ts/queryKeys.ts— adduseProgressTabData(courseId, targetUserId?)and theprogressTab(courseId, targetUserId?)key. It keeps a scopedmeta: { modelType: 'progress', courseId }so the model-store bridge still feeds the one shared holdout reader (below).ProgressTab.jsx— split into a thin data-owningProgressTab(runs the two queries, renders<TabWithTimer courseStatus={{ metadataQuery, tabDataQuery }}><ProgressTabContent/></TabWithTimer>) and aProgressTabContentin the same file. The split keeps the query-data reads out of the loading phase, where the data is still empty.useProgressData()hook (progress-tab/hooks.jsx) — reads the tab data fromuseProgressTabData(courseId, targetUserId); subtree readers call it instead ofuseModel('progress'), so they share one fetch (keyed by the sametargetUserId— critical on the staff/progress/:targetUserId/route).courseId→useParams— the ~20 grade/certificate/related-links components +ProgressTabGradeBreakdownSlotmove offuseContextId(); most dropcourseIdentirely and readuseProgressData(). Required: the route no longer runs a thunk to setstate.courseHome.courseId.ProgressHeader.jsx— readstargetUserId/usernamefromuseParams+useProgressData()instead of the slice +useModel('progress'); thetargetUserId !== userIdint comparison is preserved.TabContainercleanup —fetchProgressTabdeleted; the now-deadisProgressTabprop + itstargetUserIdfetch branch removed fromTabContainer(progress was its only consumer). The sharedfetchTabhelper stays (live/discussion still use it).The one retained bridge
Every progress-specific reader is off the model store.
useProgressTabDatakeeps itsmetabridge for exactly one shared, tab-generic holdout:useAccessExpirationMasqueradeBanner(rendered byInstructorToolbar) does a dynamicuseModel(tab)that resolves to'progress'. Migrating it cleanly needs every tab to be query-backed, so it's deferred to the model-store dissolution (#1977, tracked in #1999) — themetaline retires with the rest of the bridge then. Full rationale in the decision log.Behavior
No user-facing change.
examsData(the exam-attempts Redux write that plugin slots read) stays as-is; only itscourseIdsource moves touseParams.Manual testing surfaced a pre-existing bug (present on
master): masquerading as a specific learner via the masquerade bar shows "Your progress" instead of "Course progress for <username>", because that flow leaves the URL (and:targetUserId) unchanged. Filed as #2000 — out of scope here; this conversion preserves the behavior exactly.Testing
npm run types,npm run lint, and the fullnpm testsuite pass (the oneCourse.test.jsxfailure is a pre-existing parallel-run flake — green in isolation).ProgressTab.test.jsxnow renders the real self-wrapping tab through the bridged query client; newuseProgressTabDatacoverage (success/transform,targetUserIdURL, 401/403/404, error) inapiHooks.test.tsx;redux.test.jsdrops thefetchProgressTabblock (fetchTabstays covered byDiscussionTab.test). Manual browser verification and the test-coverage split are documented in the decision log below.Decisions
Full decision log
Decisions — Redux → React Query: the progress tab (#1946)
Working notes for this PR (part of the wider Redux → React Query migration,
#1946). Not checked in — referenced when opening the PR. Part of #1975
(course-home tab data), stacked on the outline-tab conversion (#1997).
Unlike the dates/outline conversions, this one goes bridge-free for every
progress-specific reader: the whole grade/certificate subtree reads the tab
data straight from the React Query cache, not from the Redux model store. One
shared, tab-generic reader forces a single, narrowly-scoped exception (below).
Scope: self-wrapping conversion
Decision.
ProgressTabbecomes self-wrapping — it owns its data viauseCourseHomeMeta+ a newuseProgressTabDatahook and renders<TabWithTimer>itself. The
index.jsxroute drops<TabContainer tab="progress" fetch={fetchProgressTab} isProgressTab>for a bare<ProgressTab />, andfetchProgressTabis deleted.ProgressTabsplits into a thin wrapper +ProgressTabContentDecision.
ProgressTabruns the two queries and renders<TabWithTimer courseStatus={{ metadataQuery, tabDataQuery }}><ProgressTabContent /></TabWithTimer>;the body moves to
ProgressTabContent. Same rationale asOutlineTabContent:TabPageonly renders children once the data has loaded, so the body reading thequery data (
useProgressData(), andsectionScores.flatMap(...)) never runsbefore the fetch resolves.
Progress-specific readers read the query directly — no bridge for them
Decision. Every progress-subtree reader of the
progressmodel — the ~20grade/certificate/related-links components, the
ProgressTabGradeBreakdownSlotplugin slot,
ProgressHeader, andProgressTabContent— stops callinguseModel('progress', courseId)and reads the tab data fromuseProgressTabData(via the
useProgressDatahook below). No model-store round-trip.Why. The goal of the epic is to remove Redux, not to keep relocating reads
onto a transitional bridge. We're already editing every one of these files to
move
courseIdoffuseContextId()(required — see below), so converting theread in the same pass costs almost nothing and actually retires the Redux
dependency instead of deferring it. React Query dedupes on the query key, so the
wrapper's
useProgressTabDataand every reader share one fetch.useProgressData()reader hookDecision. Add
useProgressData()(inprogress-tab/hooks.jsx):const { courseId, targetUserId } = useParams(); return useProgressTabData(courseId, targetUserId).data;. Subtree readers calluseProgressData()and destructure.Why. React Query keys by every argument, so a reader must pass the same
targetUserIdas the wrapper to hit the same cache entry — otherwise, on thestaff
/progress/:targetUserId/route, it would key onundefined, fire a secondfetch, and show the requesting user's grades instead of the learner's. Threading
targetUserIdthrough ~20 leaves is noise; the model store used to hide it (oneprogressmodel keyed bycourseId, the target user's data baked in). This hookrestores that ergonomic: callers name neither
targetUserIdnor (usually)courseId. The wrapper still uses rawuseProgressTabDatabecause it needs thefull query object (
isLoading/isError) for gating.The one shared reader keeps the
metabridge (its migration is deferred)Decision.
useProgressTabDatakeepsmeta: { modelType: 'progress', courseId },so the model-store bridge still populates
useModel('progress', courseId)— forexactly one reader:
useAccessExpirationMasqueradeBanner(courseId, tab)(renderedby the shared
InstructorToolbar) doesuseModel(tab, courseId)and readsaccessExpiration.masqueradingExpiredCoursefor the "this learner's access expired"masquerade banner. It's the only dynamic
useModel(<var>)reader that resolves to'progress'(the others are dates/outline/courseware); a literaluseModel('progress'grep misses it. Every progress-specific reader is already off the bridge (above) —
this is the lone shared, tab-generic holdout.
Why keep it rather than migrate it here. We tried migrating this banner first,
as a lower stack layer, so progress could be fully bridge-free — it doesn't work
cleanly.
accessExpirationlives in per-tab data, the banner's consumer is theshared, tab-generic
InstructorToolbar, and only some tabs are query-backed, soevery mid-transition shape is bad: thread it down → the hook takes its own data
in (inverting the responsibility of a hook named for that data); have the hook
read the cache → a generic
alerts/hook has to importcourse-homequery keysand special-case progress's
targetUserId. Both still need auseModelfallbackfor the unconverted tabs. It's only clean once every tab is query-backed. So the
banner migration is deferred to the model-store dissolution (#1977) and tracked
in #1999; the
metahere retires with the rest of the bridge at that point.The model store persists until #1977 regardless, so this one
metaline is justone more consumer of a bridge that's staying anyway — not worth holding up progress
or contorting the banner for.
Subtree
courseId:useContextId()→useParams(required)Decision. The 20 progress-only subtree components + the
ProgressTabGradeBreakdownSlotmovecourseIdfromuseContextId()touseParams(most dropcourseIdentirely, readinguseProgressData()instead;the handful that use
courseIdelsewhere keepconst { courseId } = useParams()).Why — required, not cosmetic.
useContextId()readsstate.courseware.courseId ?? state.courseHome.courseId; thecourseHomeone isonly set by the fetch thunk. Once the route stops running it,
useContextId()returns
undefinedand every reader misses. All 20 were verified progress-only.targetUserIdfromuseParams, with the int comparison preservedDecision.
ProgressHeaderreadstargetUserIdfromuseParams(a string)and parses it for its one numeric use — the
targetUserId !== userIdtitle check(
userIdis a number). The old thunkparseInt'dtargetUserIdinto the slice,so a faithful conversion keeps that an int comparison rather than switching to a
string compare.
Masquerade heading is a pre-existing bug, left out of scope (#2000)
Decision.
ProgressHeaderkeeps deriving "viewing another student" from the URL:targetUserIdsegment (the faithful conversion above —parseInt(useParams() .targetUserId, 10)). We do not extend it to also cover the masquerade bar.Why. Manual testing surfaced that masquerading as a specific student via the
masquerade bar shows "Your progress" instead of "Course progress for <username>":
the bar sets a server-side session and leaves the URL (and
:targetUserId) unchanged,so the check is false. Confirmed pre-existing on
master— the conversion preservesthe behavior exactly (old: slice
targetUserIdundefined → falsy; new:parseInt(undefined, 10)→NaN→ falsy). It's an independent bug whose real fix isnon-trivial (distinguishing specific-student masquerade from generic-role masquerade
needs a signal beyond the URL — the masquerade target identity), so bundling it would
break the faithful-conversion principle. Filed standalone as #2000. A
username-basedproxy was considered and rejected as too indirect. (The deep-link
/progress/:targetUserId/path is unaffected and works.)
examsDatastays for now — a separate Redux write to retire laterDecision. Leave the exam-attempts path (
useGetExamsData→fetchExamAttemptsData→state.courseHome.examsData, viagetExamsData) inplace; only its
courseIdsource moves touseParams.Why (and not "it's a plugin contract"). It's a fire-and-forget Redux write
with no in-repo reader — it exists so plugin slots can read
examsDatafrom thestore (PR #1829). The Redux store isn't a supported plugin API, so that's not a
reason to preserve it; it's just another Redux surface to remove. But removing it
means giving plugins a query-based replacement, which is its own task — so it's
out of scope here, not something we protect. (An earlier note calling it a
"contract to keep" was wrong.)
Route +
TabContainercleanupsDecision. Delete
fetchProgressTab(only the route wrapper called it), andremove the now-dead
isProgressTabprop + itstargetUserIdfetch branch fromTabContainer(progress was its only consumer). The sharedfetchTabhelperstays (live and discussion still use it).
Coverage: preserve the shared
fetchTabhelper's coverageDecision. When removing the
fetchProgressTabredux test, keep the sharedfetchTabhelper covered by a remaining tab test (live/discussion still use it),adding one if needed. Give
useProgressTabDataits ownapiHooks.test(success +error). Same indirect-coverage trap the outline PR hit in
getOutlineTabData.Test conversion
ProgressTab.test.jsxrenders the self-wrapping tab through the bridged queryclient. The
fetchAndRenderhelper dropsexecuteThunk(fetchProgressTab)+bare
render(<ProgressTab />)for theOutlineTab.testpattern: anAppProviderMemoryRouter(route/course/:courseId/progress/:targetUserId?) +createTestQueryClient(store)(the model-store bridge, so the one remaininguseModel('progress')reader still resolves) +UserMessagesProvider/ToastProvider,then waits for the loading
statusrole to clear. The masquerade / course-startbanner tests previously hand-rendered
<LoadedTabPage …>...</LoadedTabPage>; theynow render the full
<ProgressTab />and assert on theinstructor-toolbarthetab draws itself — same as the outline access-expiration tests.
Two existing selectors had to disambiguate now that the full page (with tab
navigation) renders. Rendering the whole tab, not just its content, adds the
header +
CourseTabsNavigationSlotlinks to the DOM:sectionviawithin(...), because the tab nav also has a "Dates" link.getAllByRole('link', 'Unlock now')—
'Unlock now'is a positional string (an ignored options arg), so the queryactually matched every link and only passed because the content-only render
had exactly three. They now target the real button,
{ name: 'Upgrade now' }(there is exactly one), keeping the existing
getAllByRole/index/fireEventstyle.
The exam re-fetch test drives the data change through the query. "Re-fetch
exam data when section scores change" used to re-run
fetchProgressTab; it nowcalls
queryClient.invalidateQueries()(the helper returns the client) so the tabquery refetches the updated mock and
useGetExamsDatare-fires.redux.test.jsdrops the wholeTest fetchProgressTabblock (thunk deleted).fetchTabstays covered byDiscussionTab.test(itsTabContainermount dispatchesfetchDiscussionTab→fetchTab);getProgressTabData's success/transform,targetUserIdURL, and 401/403/404/other-error branches move to theuseProgressTabDatablock inapiHooks.test. The 404 branch callsglobal.location.replace, so that one case swapswindow.location(non-configurablereplacecan't be spied) inside atry/finallythat always restores it.TabContainer.test.jsxdrops theisProgressTabtargetUserIdtest — the propand its fetch branch are gone, and that behavior now lives in the progress query's
useParams/useProgressTabDatapath (covered by the/progress/10/route test).Manual testing (in-browser) vs. what we lean on the automated suite for
Tested in a real browser against a live backend (tutor local), scoped to what this
PR changes.
Verified by hand:
useProgressTabDatafetch path; nothing pre-fetched).
useParamscourseId— the highest-riskmechanical rewrite: course-grade header, grade summary, detailed grades + the
per-problem drawer + subsection navigation, related-links + their analytics event,
and the completion graph.
/progress/<numericId>/: heading shows "Course progress for<username>", the body shows that learner's grades/certificate, and the Studio
link shows for admins.
with active access.
/courses/<id>/progress.Left to the automated suite (not re-done by hand):
ProgressTab.testrenders them through the bridged query client, which exercisesthe retained one-reader
metabridge end-to-end in-test.ProgressTab.testCertificate Status suite.ProgressTab.testlocked-feature tests.examsData) —ProgressTab.testexam-integration suite +redux.test'sfetchExamAttemptsData.targetUserId) — the query key includestargetUserId(queryKeys+ theapiHookstargetUserId-URL test); not re-checked in-browser.TabPagedenied handling.The masquerade-bar heading is the known pre-existing bug (#2000 above), not covered here.
🤖 Generated with Claude Code