Skip to content

refactor: convert progress tab from Redux to React Query - #2001

Draft
brian-smith-tcril wants to merge 1 commit into
bsmith/react-query-course-home-outline-tabfrom
bsmith/react-query-course-home-progress-tab
Draft

refactor: convert progress tab from Redux to React Query#2001
brian-smith-tcril wants to merge 1 commit into
bsmith/react-query-course-home-outline-tabfrom
bsmith/react-query-course-home-progress-tab

Conversation

@brian-smith-tcril

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

Copy link
Copy Markdown
Contributor

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.

ProgressTab becomes self-wrapping — it owns its data (useCourseHomeMeta + a new useProgressTabData) and renders TabWithTimer itself, so the index.jsx route drops its <TabContainer tab="progress" fetch={fetchProgressTab} isProgressTab> wrapper for a bare <ProgressTab /> and fetchProgressTab is deleted.

What changed

  • apiHooks.ts / queryKeys.ts — add useProgressTabData(courseId, targetUserId?) and the progressTab(courseId, targetUserId?) key. It keeps a scoped meta: { modelType: 'progress', courseId } so the model-store bridge still feeds the one shared holdout reader (below).
  • ProgressTab.jsx — split into a thin data-owning ProgressTab (runs the two queries, renders <TabWithTimer courseStatus={{ metadataQuery, tabDataQuery }}><ProgressTabContent/></TabWithTimer>) and a ProgressTabContent in 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 from useProgressTabData(courseId, targetUserId); subtree readers call it instead of useModel('progress'), so they share one fetch (keyed by the same targetUserId — critical on the staff /progress/:targetUserId/ route).
  • Subtree courseIduseParams — the ~20 grade/certificate/related-links components + ProgressTabGradeBreakdownSlot move off useContextId(); most drop courseId entirely and read useProgressData(). Required: the route no longer runs a thunk to set state.courseHome.courseId.
  • ProgressHeader.jsx — reads targetUserId/username from useParams + useProgressData() instead of the slice + useModel('progress'); the targetUserId !== userId int comparison is preserved.
  • Route + TabContainer cleanupfetchProgressTab deleted; the now-dead isProgressTab prop + its targetUserId fetch branch removed from TabContainer (progress was its only consumer). The shared fetchTab helper stays (live/discussion still use it).

The one retained bridge

Every progress-specific reader is off the model store. useProgressTabData keeps its meta bridge for exactly one shared, tab-generic holdout: useAccessExpirationMasqueradeBanner (rendered by InstructorToolbar) does a dynamic useModel(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) — the meta line 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 its courseId source moves to useParams.

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 full npm test suite pass (the one Course.test.jsx failure is a pre-existing parallel-run flake — green in isolation). ProgressTab.test.jsx now renders the real self-wrapping tab through the bridged query client; new useProgressTabData coverage (success/transform, targetUserId URL, 401/403/404, error) in apiHooks.test.tsx; redux.test.js drops the fetchProgressTab block (fetchTab stays covered by DiscussionTab.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. ProgressTab becomes self-wrapping — it owns its data via
useCourseHomeMeta + a new useProgressTabData hook and renders <TabWithTimer>
itself. The index.jsx route drops <TabContainer tab="progress" fetch={fetchProgressTab} isProgressTab> for a bare <ProgressTab />, and
fetchProgressTab is deleted.

ProgressTab splits into a thin wrapper + ProgressTabContent

Decision. ProgressTab runs the two queries and renders <TabWithTimer courseStatus={{ metadataQuery, tabDataQuery }}><ProgressTabContent /></TabWithTimer>;
the body moves to ProgressTabContent. Same rationale as OutlineTabContent:
TabPage only renders children once the data has loaded, so the body reading the
query data (useProgressData(), and sectionScores.flatMap(...)) never runs
before the fetch resolves.

Progress-specific readers read the query directly — no bridge for them

Decision. Every progress-subtree reader of the progress model — the ~20
grade/certificate/related-links components, the ProgressTabGradeBreakdownSlot
plugin slot, ProgressHeader, and ProgressTabContent — stops calling
useModel('progress', courseId) and reads the tab data from useProgressTabData
(via the useProgressData hook 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 courseId off useContextId() (required — see below), so converting the
read 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 useProgressTabData and every reader share one fetch.

useProgressData() reader hook

Decision. Add useProgressData() (in progress-tab/hooks.jsx):
const { courseId, targetUserId } = useParams(); return useProgressTabData(courseId, targetUserId).data;. Subtree readers call useProgressData() and destructure.

Why. React Query keys by every argument, so a reader must pass the same
targetUserId as the wrapper to hit the same cache entry — otherwise, on the
staff /progress/:targetUserId/ route, it would key on undefined, fire a second
fetch, and show the requesting user's grades instead of the learner's. Threading
targetUserId through ~20 leaves is noise; the model store used to hide it (one
progress model keyed by courseId, the target user's data baked in). This hook
restores that ergonomic: callers name neither targetUserId nor (usually)
courseId. The wrapper still uses raw useProgressTabData because it needs the
full query object (isLoading/isError) for gating.

The one shared reader keeps the meta bridge (its migration is deferred)

Decision. useProgressTabData keeps meta: { modelType: 'progress', courseId },
so the model-store bridge still populates useModel('progress', courseId)for
exactly one reader
: useAccessExpirationMasqueradeBanner(courseId, tab) (rendered
by the shared InstructorToolbar) does useModel(tab, courseId) and reads
accessExpiration.masqueradingExpiredCourse for 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 literal useModel('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. accessExpiration lives in per-tab data, the banner's consumer is the
shared, tab-generic InstructorToolbar, and only some tabs are query-backed, so
every 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 import course-home query keys
and special-case progress's targetUserId. Both still need a useModel fallback
for 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 meta here retires with the rest of the bridge at that point.
The model store persists until #1977 regardless, so this one meta line is just
one 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
ProgressTabGradeBreakdownSlot move courseId from useContextId() to
useParams (most drop courseId entirely, reading useProgressData() instead;
the handful that use courseId elsewhere keep const { courseId } = useParams()).

Why — required, not cosmetic. useContextId() reads
state.courseware.courseId ?? state.courseHome.courseId; the courseHome one is
only set by the fetch thunk. Once the route stops running it, useContextId()
returns undefined and every reader misses. All 20 were verified progress-only.

targetUserId from useParams, with the int comparison preserved

Decision. ProgressHeader reads targetUserId from useParams (a string)
and parses it for its one numeric use — the targetUserId !== userId title check
(userId is a number). The old thunk parseInt'd targetUserId into 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. ProgressHeader keeps deriving "viewing another student" from the URL
:targetUserId segment (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 preserves
the behavior exactly (old: slice targetUserId undefined → falsy; new:
parseInt(undefined, 10)NaN → falsy). It's an independent bug whose real fix is
non-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-based
proxy was considered and rejected as too indirect. (The deep-link /progress/:targetUserId/
path is unaffected and works.)

examsData stays for now — a separate Redux write to retire later

Decision. Leave the exam-attempts path (useGetExamsData
fetchExamAttemptsDatastate.courseHome.examsData, via getExamsData) in
place; only its courseId source moves to useParams.

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 examsData from the
store (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 + TabContainer cleanups

Decision. Delete fetchProgressTab (only the route wrapper called it), and
remove the now-dead isProgressTab prop + its targetUserId fetch branch from
TabContainer (progress was its only consumer). The shared fetchTab helper
stays (live and discussion still use it).

Coverage: preserve the shared fetchTab helper's coverage

Decision. When removing the fetchProgressTab redux test, keep the shared
fetchTab helper covered by a remaining tab test (live/discussion still use it),
adding one if needed. Give useProgressTabData its own apiHooks.test (success +
error). Same indirect-coverage trap the outline PR hit in getOutlineTabData.

Test conversion

ProgressTab.test.jsx renders the self-wrapping tab through the bridged query
client.
The fetchAndRender helper drops executeThunk(fetchProgressTab) +
bare render(<ProgressTab />) for the OutlineTab.test pattern: an AppProvider

  • MemoryRouter (route /course/:courseId/progress/:targetUserId?) +
    createTestQueryClient(store) (the model-store bridge, so the one remaining
    useModel('progress') reader still resolves) + UserMessagesProvider/ToastProvider,
    then waits for the loading status role to clear. The masquerade / course-start
    banner tests previously hand-rendered <LoadedTabPage …>...</LoadedTabPage>; they
    now render the full <ProgressTab /> and assert on the instructor-toolbar the
    tab 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 + CourseTabsNavigationSlot links to the DOM:

  • The "Dates" related-link is scoped to the "Related links" section via
    within(...), because the tab nav also has a "Dates" link.
  • The two "locked feature" tests asserted on getAllByRole('link', 'Unlock now')
    'Unlock now' is a positional string (an ignored options arg), so the query
    actually 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/fireEvent
    style.

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 now
calls queryClient.invalidateQueries() (the helper returns the client) so the tab
query refetches the updated mock and useGetExamsData re-fires.

redux.test.js drops the whole Test fetchProgressTab block (thunk deleted).
fetchTab stays covered by DiscussionTab.test (its TabContainer mount dispatches
fetchDiscussionTabfetchTab); getProgressTabData's success/transform,
targetUserId URL, and 401/403/404/other-error branches move to the
useProgressTabData block in apiHooks.test. The 404 branch calls
global.location.replace, so that one case swaps window.location (non-configurable
replace can't be spied) inside a try/finally that always restores it.

TabContainer.test.jsx drops the isProgressTab targetUserId test — the prop
and its fetch branch are gone, and that behavior now lives in the progress query's
useParams/useProgressTabData path (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:

  • Cold load + hard-reload directly on the progress URL (the useProgressTabData
    fetch path; nothing pre-fetched).
  • The whole grade subtree renders off the useParams courseId — the highest-risk
    mechanical rewrite: course-grade header, grade summary, detailed grades + the
    per-problem drawer + subsection navigation, related-links + their analytics event,
    and the completion graph.
  • Staff deep-link /progress/<numericId>/: heading shows "Course progress for
    <username>", the body shows that learner's grades/certificate, and the Studio
    link shows for admins.
  • Access-expiration masquerade banner correctly absent for a masqueraded learner
    with active access.
  • 404 on the progress data → redirect to the legacy /courses/<id>/progress.

Left to the automated suite (not re-done by hand):

  • Both positive masquerade banners (access-expired shows; course-start shows) —
    ProgressTab.test renders them through the bridged query client, which exercises
    the retained one-reader meta bridge end-to-end in-test.
  • Certificate states + "Request certificate" — ProgressTab.test Certificate Status suite.
  • Locked/limited "Upgrade now" preview + its analytics — ProgressTab.test locked-feature tests.
  • Exam-data integration (examsData) — ProgressTab.test exam-integration suite + redux.test's fetchExamAttemptsData.
  • Per-learner query keying (switching targetUserId) — the query key includes
    targetUserId (queryKeys + the apiHooks targetUserId-URL test); not re-checked in-browser.
  • Denied / unenrolled path — shared TabPage denied handling.

The masquerade-bar heading is the known pre-existing bug (#2000 above), not covered here.

🤖 Generated with Claude Code

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

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.75%. Comparing base (8d54974) to head (eb4416c).

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.
📢 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.

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 progress tab to React Query

1 participant