refactor: convert the discussion tab from Redux to React Query - #2005
Open
brian-smith-tcril wants to merge 1 commit into
Open
Conversation
Self-wrap the discussion tab (metadata-only: no tab-data fetch, so TabPage's tabDataQuery becomes optional). Also convert CourseAccessErrorPage — the other fetchDiscussionTab caller — to useCourseHomeMeta, letting fetchDiscussionTab be deleted. Ordered below the live tab so the shared fetchTab helper retires cleanly there. Part of #1975; closes #2003. 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-progress-tab #2005 +/- ##
===============================================================================
- Coverage 93.10% 93.10% -0.01%
===============================================================================
Files 363 363
Lines 5936 5934 -2
Branches 1377 1414 +37
===============================================================================
- Hits 5527 5525 -2
Misses 391 391
Partials 18 18 ☔ 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 discussion tab from Redux to React Query. Part of the Redux → React Query migration (#1946), stacked on the progress-tab conversion (#2001), below the live-tab conversion (#2002). Closes #2003.
The discussion tab is metadata-only (no tab-data endpoint), so it self-wraps
<TabWithTimer>with justuseCourseHomeMetaand needs no model-store bridge. The otherfetchDiscussionTabcaller (CourseAccessErrorPage) is converted too, so the thunk is deleted outright.What changed
tab-page/TabPage.tsx—tabDataQueryis now optional inCourseStatus;deriveViewguards its twotabDataQuerychecks. A metadata-only tab passescourseStatus={{ metadataQuery }}; the data-bearing tabs still pass both.course-home/discussion-tab/DiscussionTab.jsx— self-wrapping: a thin data-owningDiscussionTab(runsuseCourseHomeMeta, renders<TabWithTimer>) + aDiscussionTabContentchild holding the iframe and theuseIFrameHeight/useIFramePluginEvents('discussions.navigate')wiring, so it mounts post-load.courseId/pathfromuseParams.generic/CourseAccessErrorPage.jsx— the otherfetchDiscussionTabcaller (a metadata-only access probe) moves touseCourseHomeMeta; its four-statecourseStatusbranch maps 1:1 to the query states.index.jsx— discussion route → bare<DiscussionTab />;fetchDiscussionTabimport/wiring dropped.course-home/data/thunks.js—fetchDiscussionTabdeleted (both callers converted). The sharedfetchTabhelper andfetchLiveTabstay — the live tab consumes them until Convert the live tab to React Query #2002, wherefetchTabretires.Behavior
No user-facing change. No
discussionmodel bridge is added:fetchDiscussionTabnever wrote adiscussionmodel, souseModel('discussion')was already empty and the access-expiration masquerade banner never rendered on this tab — preserved exactly.TabWithTimeris retained, so the outer exam timer still mounts as it did viaTabContainer.Testing
npm run types,npm run lint, and the affected suites pass.DiscussionTab.test.jsxrenders<DiscussionTab />through the bridged query client;CourseAccessErrorPage.test.jsxmocksuseCourseHomeMeta(loading / has-access redirect / denied);redux.test.jsis untouched (the sharedfetchTabhelper retires in the live layer, #2002). Manual browser verification and the manual-vs-automated split are documented in the decision log below.Decisions
Full decision log
Decisions — Redux → React Query: the discussion 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 progress-tab conversion (#2001), below
the live-tab conversion (#2002). Closes #2003.
The discussion tab is the simplest course-home conversion: it's metadata-only
(no tab-data endpoint) and needs no model-store bridge. The only real work
beyond the self-wrap is a small shared-type change (a metadata-only tab is now a
first-class shape) and converting the other
fetchDiscussionTabcaller so thethunk can be deleted outright.
Scope: self-wrapping conversion
Decision.
DiscussionTabbecomes self-wrapping — it owns its access gatingvia
useCourseHomeMetaand renders<TabWithTimer>itself. Theindex.jsxroute drops
<TabContainer tab="discussion" fetch={fetchDiscussionTab} slice="courseHome">for a bare<DiscussionTab />.The discussion tab is metadata-only —
tabDataQuerybecomes optionalDecision.
fetchDiscussionTabwasfetchTab(courseId, 'discussion')withno
getTabData— it only fetched course metadata (for access gating); the tabitself builds the discussions-MFE iframe URL from
courseId+ the routepath,reading no tab-data model. So the converted tab has a
metadataQueryand notab-data query, and passes
courseStatus={{ metadataQuery }}.To make that a first-class shape,
TabPage'sCourseStatusmarkstabDataQueryoptional and
deriveViewguards its two checks (tabDataQuery?.isError/
?.isLoading). The data-bearing tabs (dates/outline/progress/live) still passboth — nothing changes for them.
Rejected: passing
metadataQueryas a dummytabDataQueryto avoid touchingTabPage. That double-reads the same query and reads confusingly at the callsite; "a tab may have only metadata" is the honest model.
DiscussionTabsplits into a thin wrapper +DiscussionTabContentDecision.
DiscussionTabruns the metadata query and renders<TabWithTimer courseStatus={{ metadataQuery }}><DiscussionTabContent /></TabWithTimer>;the iframe + its
useIFrameHeight/useIFramePluginEvents('discussions.navigate')wiring move to
DiscussionTabContent. Same rationale as the outline/progresssplits:
TabPageonly renders children once loaded, so the plugin-eventregistration and iframe mount happen post-load — matching today's timing, where
DiscussionTabisTabContainer's already-loaded child.TabWithTimeris kept (the discussion tab always had the exam timer)Decision. Keep
TabWithTimer, not bareTabPage. The old Redux path wrappeddiscussion in
TabContainer, which rendersTabWithTimer(→OuterExamTimer).So the discussion tab has always rendered the outer exam timer; keeping
TabWithTimerpreserves that exactly.No transitional bridge
Decision.
useCourseHomeMetakeeps its existingcourseHomeMetabridge (ason every tab, for the shared
TabPage/LoadedTabPagereads), but the discussiontab adds no
discussionbridge.fetchDiscussionTabnever wrote adiscussionmodel (nogetTabData), sostate.models.discussionwas alwaysempty — the masquerade banner's
useModel('discussion')already resolves emptyand never rendered on this tab. Preserved exactly, no bridge needed.
courseIdfromuseParamsDecision. Both
DiscussionTabandDiscussionTabContentreadcourseId(and
path) fromuseParams, replacinguseSelector(state.courseHome.courseId).Required: with no fetch thunk,
state.courseHome.courseIdis never set for thisroute.
CourseAccessErrorPage converted too (the other
fetchDiscussionTabcaller)Decision.
generic/CourseAccessErrorPage.jsxdispatchedfetchDiscussionTabpurely as a cheap access probe — metadata-only — then read
state.courseHome.courseStatusto decide: redirect home vs. show the deniedpage. It's moved to
useCourseHomeMeta(courseId). It never needed anythingdiscussion-specific; converting it lets
fetchDiscussionTabbe deleted entirely.The four-state
courseStatusbranch maps cleanly onto the query:courseStatus)metadataQuery)LOADING(initial + fetching) → loadingisLoading→ loadingLOADED(set only after ahasAccesssuccess) → redirectdata.courseAccess.hasAccess→ redirectDENIED/FAILED→ denied pageuseCourseHomeMetais always enabled, so it'sisLoadingon first mount (nopremature "denied" flash before data arrives).
fetchDiscussionTabdeleted;fetchTabkept for the live layerDecision. Delete
fetchDiscussionTab(both callers converted). Keep theshared
fetchTabhelper andfetchLiveTab— the live tab still consumes themuntil #2002.
Ordering: below live, so
fetchTabretires cleanly thereDecision. Sequence discussion below live. After this layer,
fetchLiveTabisfetchTab's sole remaining consumer, so the live layer (#2002)deletes
fetchLiveTab, the now-orphanedfetchTab, and theredux.test.jsTest fetchTabblock in one clean removal. The alternative order (live first)would force retargeting that test block from
fetchLiveTab→fetchDiscussionTaband then deleting it a layer later — churn on
redux.test.jsfor no benefit.Consequently
redux.test.jsis untouched in this layer.Tests
DiscussionTab.test.jsxrenders<DiscussionTab />directly through thebridged query client (
createTestQueryClient(store)), dropping theTabContainer+fetchDiscussionTabwrapper. The existing iframe-resizeassertion is unchanged (it exercises
DiscussionTabContent).CourseAccessErrorPage.test.jsxmocksuseCourseHomeMetainstead of theRedux
useSelector/useDispatch; the three cases (loading / has-accessredirect / denied) map 1:1 to the query states above.
redux.test.js— untouched (see Ordering).Manual testing (in-browser)
Verified:
CourseAccessErrorPage(the real behavioral change) — all three states,confirmed on both this branch and the untouched Redux branch, so the
useCourseHomeMetaswap is behavior-preserving: non-enrolled → renders theaccess-denied page (
access-denied-mainpresent,has_access:falsein themetadata response, URL stays); enrolled → redirects to
/redirect/home/<courseId>;brief loading spinner before metadata resolves.
iframe on a cold, direct-URL load. The doubled header is expected for an embedded
MFE and is unchanged from master.
Not manually exercised (and why it's fine):
in this env the nav Discussion tab links to the external discussions MFE
(per the metadata
tabs), so the embedded route isn't entered organically. Thediscussions.navigate→navigatewiring is unchanged from the pre-conversioncomponent.
structurally (
TabContaineralways wrapped discussion inTabWithTimer, and theconverted tab keeps it).
/homeviagetAccessDeniedRedirectUrl('discussion')(inline denied is outline-only);matches master.
🤖 Generated with Claude Code