refactor: convert outline tab from Redux to React Query - #1997
Draft
brian-smith-tcril wants to merge 1 commit into
Draft
refactor: convert outline tab from Redux to React Query#1997brian-smith-tcril wants to merge 1 commit into
brian-smith-tcril wants to merge 1 commit into
Conversation
Make OutlineTab self-wrapping — it owns useCourseHomeMeta + a new
useOutlineTabData query and renders TabWithTimer itself, mirroring the
dates-tab conversion. The heavy body moves into an OutlineTabContent
component so the useModel('outline') reads never run against the empty
model during the loading phase.
Subtree courseId reads move from the courseHome slice (directly, or via
useContextId in section-outline) to useParams, since the outline route
no longer runs a fetch thunk to populate state.courseHome.courseId.
ShiftDatesAlert now invalidates both datesTab and outlineTab and drops
its fetch/useDispatch. The outline-only POST writers dismissWelcomeMessage
and saveWeeklyLearningGoal become useDismissWelcomeMessage /
useSaveWeeklyLearningGoal mutations, and the fetchOutlineTab thunk is
deleted. proctoringPanelStatus stays in Redux (client UI state).
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-request-cert-mutation #1997 +/- ##
============================================================================
- Coverage 92.96% 92.90% -0.07%
============================================================================
Files 363 363
Lines 5939 5945 +6
Branches 1381 1380 -1
============================================================================
+ Hits 5521 5523 +2
- Misses 399 402 +3
- 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 outline tab from Redux to React Query. Part of the Redux → React Query migration (#1946), and the next course-home tab through the door the dates-tab conversion opened. Closes #1991.
OutlineTabbecomes self-wrapping — it owns its data (useCourseHomeMeta+ a newuseOutlineTabData) and rendersTabWithTimeritself, so theindex.jsxroute drops its<TabContainer tab="outline" fetch={fetchOutlineTab}>wrapper for a bare<OutlineTab />andfetchOutlineTabis deleted.What changed
apiHooks.ts/queryKeys.ts— adduseOutlineTabData(taggedmeta: { modelType: 'outline', courseId }so the model-store bridge still populates the subtree'suseModel('outline')reads) and theoutlineTab(courseId)key.OutlineTab.jsx— split into a thin data-owningOutlineTab(runs the two queries, renders<TabWithTimer courseStatus={{ metadataQuery, tabDataQuery }}><OutlineTabContent/></TabWithTimer>) and anOutlineTabContentin the same file. The split keeps theuseModel('outline')destructure out of the loading phase, where the model is still{}.courseIdfromuseParams;proctoringPanelStatusstays onuseSelector.courseId→useParams— the seven slice-reading widgets (DateSummary,CourseDates,CourseHandouts,CourseTools,StartOrResumeCourseCard,WeeklyLearningGoalCard,ProctoringInfoPanel) and the threesection-outlinecomponents that reached the slice viauseContextId(Section,SequenceDueDate,SequenceTitle). Required: the route no longer runs a thunk to setstate.courseHome.courseId.ShiftDatesAlert.jsx—refreshTabDatanow invalidates bothdatesTabandoutlineTab; drops itsfetch/useDispatch(outline was the lastfetch=caller).dismissWelcomeMessage→useDismissWelcomeMessage(thunk deleted) andsaveWeeklyLearningGoal→useSaveWeeklyLearningGoal(moved out ofthunks.js, where it was a loose non-thunk helper).Behavior
No user-facing change.
proctoringPanelStatusdeliberately stays in Redux (client UI state, out of scope for a data-layer conversion). The mutations addonError: logError, which the thunks lacked — a strict improvement.Testing
npm run types,npm run lint, and the fullnpm testsuite (106 suites, 3 pre-existing skips) pass.OutlineTab.test.jsxandProductTours.test.jsxnow render the real self-wrapping tab through the bridged query client; new mutation coverage (useOutlineTabData,useDismissWelcomeMessage,useSaveWeeklyLearningGoal) inapiHooks.test.tsx. Manual browser verification and the test-coverage split are documented in the decision log below.Decisions
Full decision log
Decisions — Redux → React Query: the outline tab (#1991)
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 dates-tab conversion (#1984), which set
the self-wrapping pattern.
Scope: same self-wrapping conversion the dates tab established
Decision.
OutlineTabbecomes self-wrapping — it renders<TabWithTimer>itself and owns its data via
useCourseHomeMeta+ a newuseOutlineTabDatahook.
courseIdcomes fromuseParams. Theindex.jsxroute drops its<TabContainer tab="outline" fetch={fetchOutlineTab}>wrapper for a bare<OutlineTab />, andfetchOutlineTabis deleted.Why. This is the pattern the dates tab set (#1984), which itself converges
on the shape
CoursewareContaineralready uses. Nothing tab-specific here —the outline tab is just the next tab through the same door.
OutlineTabsplits into a thin wrapper +OutlineTabContentDecision.
OutlineTabis now just the data-owning wrapper: it runs the twoqueries and renders
<TabWithTimer courseStatus={{ metadataQuery, tabDataQuery }}> <OutlineTabContent /></TabWithTimer>. The heavy body — everything that readsuseModel('outline', courseId)— moves into anOutlineTabContentcomponent inthe same file.
Why. Self-wrapping means
OutlineTabrenders before its data resolves (thespinner phase lives inside
TabWithTimer). The body destructures theoutlinemodel (
const { courseBlocks, … } = useModel('outline', courseId)), anduseModelreturns{}for a missing model, so that destructure ran against{}during loading and crashed.
TabPageonly renders its children once the data hasloaded, so moving the body into a child (
OutlineTabContent) means thedestructure never runs empty — no defensive defaults, no guard, and the wrapper
stays a thin data-owner. Same file, so it reads as one unit.
The
outlinemodel stays on the transitional bridge — it isn't outline-onlyDecision. Keep every
useModel('outline'|'courseHomeMeta', …)read in thesubtree exactly as-is, populated by the model-store bridge (the
meta: { modelType, courseId }tag on the query drivesaddModelon success). Donot convert any of these reads to consume
useOutlineTabData's datadirectly in this PR. Dissolving the model store is its own coordinated epic
step (#1977).
Why — the mechanism. We looked into who actually reads these models, to see
if any were outline-tab-only and could be pulled forward:
courseHomeMetais read all over the app (courseware, every tab, dozens ofshared alerts,
TabPage) — plainly shared.outlinelooked outline-scoped — every reader is underoutline-tab/except two:
useEnrollmentAlertanduseLogistrationAlert. Those runinside
LoadedTabPage, which renders on every tab and courseware, andthey read
useModel('outline', courseId)passively and defensively(
outline && outline.courseBlocks && …) — on non-outline pages the model isempty and the alert stays hidden.
The distinction that matters:
useModelis a passive read (returns whateveris cached, fetches nothing);
useOutlineTabData(courseId)is an active query(calling it triggers the outline fetch). Swapping those shared alert hooks to
the query would fire the outline API on the dates tab, the progress tab, and
courseware — a real regression. The passive-read-plus-bridge is exactly what
makes the shared read safe today, which is why untangling it is a coordinated
store dissolution (#1977), not per-tab work.
Alternative rejected. Convert just the outline-tab-only readers to
useOutlineTabData(child calls dedupe againstOutlineTab's query, so noextra fetch), leaving the bridge for the two shared alerts. It can't remove the
bridge or the
metatag (the shared alerts still need the model storepopulated), and it leaves the
outlinemodel read two different ways for #1977to reconcile anyway — churn without retiring the transitional mechanism.
proctoringPanelStatusstays in ReduxDecision.
OutlineTabkeeps readingproctoringPanelStatusfromuseSelector(state.courseHome); onlycourseIdmoves touseParams.ProctoringInfoPanelkeeps itsuseDispatch(fetchProctoringInfoResolved()).Why. It's client UI state (a reducer flips it to
loadedwhen the panelresolves, gating when the goal widget appears), not server data — out of scope
for a data-layer conversion. It's retired when the store is reduced to the
external specialExams reducer (#1978).
Subtree
courseId: slice /useContextIdreads →useParamsDecision. Every subtree component that derived
courseIdfrom the slicemoves to
useParams:useSelector(state.courseHome)readers —DateSummary,CourseDates,CourseHandouts,CourseTools,StartOrResumeCourseCard,WeeklyLearningGoalCard,ProctoringInfoPanel(keeps itsuseDispatch).section-outlinereaders —Section,SequenceDueDate,SequenceTitle— which reached the slice indirectly through
useContextId()(
state.courseware.courseId ?? state.courseHome.courseId). Only these threeoutline-only callers move; the shared
useContextIditself is left alone(~30 progress/course-exit components rely on it).
Why — required, not cosmetic.
state.courseHome.courseIdis only ever setby a tab's fetch thunk (
fetchTabRequest). Once the outline route stops runningone, any descendant still reading it — directly or via
useContextId()— getsundefined, so itsuseModel(…, courseId)misses or itssequences[id]lookupcrashes: the same breakage class as the
UpgradeToCompleteAlertslot child inthe dates PR.
ShiftDatesAlert: invalidate all date-bearing caches, and finish its bridgeDecision.
refreshTabDatainvalidates bothdatesTab(courseId)andoutlineTab(courseId), independent of which tab the alert is on. Themodelprop stays only for the
useModel(model)visibility gate and theresetDeadlines.mutate({ courseId, model })call. Outline is the lastfetch={fetchOutlineTab}caller, so thefetchprop anduseDispatchareremoved entirely.
Why. The alert is about dates; shifting them changes date info wherever
it's cached — the
datesmodel (datesTabquery) and the outline model'sdatesWidget/SequenceDueDate(outlineTabquery). So invalidation ismodel-independent: both keys, always.
Not a fix to the dates PR. The dates PR invalidating only
datesTabwascorrect for its moment — outline was still Redux/thunk-fetched then, so no
outlineTabquery existed to invalidate; its on-mountfetchOutlineTabhandledstaleness. This PR is where the outline cache first exists, so it's where it
joins the alert's invalidation set.
Bucket 3: outline-only POST writers become mutations
Decision. Convert the outline tab's fire-and-forget POST writers to React
Query mutations, removing
useDispatchfrom their components:dismissWelcomeMessage(WelcomeMessage) →useDismissWelcomeMessage; thethunk is deleted.
saveWeeklyLearningGoal(WeeklyLearningGoalCard) →useSaveWeeklyLearningGoal. This one wasn't even a thunk — it was a plainpostWeeklyLearningGoalwrapper sitting inthunks.js. Since we're convertingthe outline writers anyway, it moves out of
thunks.jsand becomes a mutationlike the others rather than a loose helper.
Why. They only POST, dispatch no action, and touch no state — the same kind
of writer the CTA-toast PR (#1982) converted (
resetDeadlines/processEvent→mutations). Converting them here gets the outline tab's genuine remaining Redux
off the store rather than leaving it for a later sweep (#1973).
requestCertwas peeled into a prerequisite PR (#1995), not converted here.It looked like a third outline writer (
CertificateStatusAlert), but it's shared—
CourseCelebration(course-exit) andCertificateStatus(progress) dispatchit too. Converting a shared thunk mid-outline-PR would drag two unrelated
subtrees into this diff, so it became its own lower stack layer (
useRequestCert,all three callers converted); this PR builds on top of it.
Minor behavior note. The mutations use
onError: logErrorto match thesibling mutations in
apiHooks.ts; the original thunks logged nothing (a failedPOST was an unhandled rejection), so this is a small improvement, not a
regression.
Verification: what was checked in a browser vs. left to the tests
The risky part of this PR is mechanical — the query conversion plus the
courseIdrewrites, where a wrongcourseIdmakes auseModelread misssilently or crashes a
sequences[id]lookup. So the manual pass targeted thepaths where that failure mode is only visible at runtime, and leaned on the
automated suite for the rest.
Manually verified in a browser:
OutlineTabContentsplit), including a hard-reload directly on the outline URL.
section-outlineuseContextId→useParamsmove: sections expand/collapse, sequence titles link intocourseware, due dates and completion state render.
useParams-fed sidebar widgets that a normal course surfaces — dateswidget, course handouts, course tools, Start/Resume card.
masquerade-as-learner, and a no-access course.
Left to the automated tests (each renders the real tab through the bridged
query path — the same code path production uses — so they exercise the actual
useParams/query wiring, not a mock of it):OutlineTab.test"handles shift due datesclick" seeds
missed_deadlines: true, swaps the refetch tofalse, clicks,and asserts the button disappears; it only can if
refreshTabDatainvalidatedoutlineTaband refetched. This is the one genuinely new behavior in the PR,and it's directly asserted.
OutlineTab.test"Welcome Message" blockplus
apiHooks.testuseDismissWelcomeMessage(POST + error logging).OutlineTab.test"Weekly Learning Goal" block (with theproctoring endpoint mocked, so the proctoring-gated render is exercised) plus
apiHooks.testuseSaveWeeklyLearningGoal.courseIdis the sameuseParamsvalue provengood by the widgets verified above; the proctoring GET is mocked in the tab
tests.
Why that split is safe. What a browser pass would add beyond the tests is
narrow: whether the live backend persists a dismiss / goal / shift across a
real reload. But those three endpoints (
dismiss_welcome_message,save_course_goal,reset_course_deadlines) are unchanged by this PR — the sameapi.jsfunctions, now called from a mutation instead of a thunk — so that pathis the platform's behavior, not this diff's. Standing up the fixtures they need
(a set welcome message, a self-paced course with a past-due deadline, proctored
exams) buys little signal for this change.
🤖 Generated with Claude Code