Skip to content

refactor: convert the discussion tab from Redux to React Query - #2005

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

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

Conversation

@brian-smith-tcril

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

Copy link
Copy Markdown
Contributor

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 just useCourseHomeMeta and needs no model-store bridge. The other fetchDiscussionTab caller (CourseAccessErrorPage) is converted too, so the thunk is deleted outright.

What changed

  • tab-page/TabPage.tsxtabDataQuery is now optional in CourseStatus; deriveView guards its two tabDataQuery checks. A metadata-only tab passes courseStatus={{ metadataQuery }}; the data-bearing tabs still pass both.
  • course-home/discussion-tab/DiscussionTab.jsx — self-wrapping: a thin data-owning DiscussionTab (runs useCourseHomeMeta, renders <TabWithTimer>) + a DiscussionTabContent child holding the iframe and the useIFrameHeight/useIFramePluginEvents('discussions.navigate') wiring, so it mounts post-load. courseId/path from useParams.
  • generic/CourseAccessErrorPage.jsx — the other fetchDiscussionTab caller (a metadata-only access probe) moves to useCourseHomeMeta; its four-state courseStatus branch maps 1:1 to the query states.
  • index.jsx — discussion route → bare <DiscussionTab />; fetchDiscussionTab import/wiring dropped.
  • course-home/data/thunks.jsfetchDiscussionTab deleted (both callers converted). The shared fetchTab helper and fetchLiveTab stay — the live tab consumes them until Convert the live tab to React Query #2002, where fetchTab retires.

Behavior

No user-facing change. No discussion model bridge is added: fetchDiscussionTab never wrote a discussion model, so useModel('discussion') was already empty and the access-expiration masquerade banner never rendered on this tab — preserved exactly. TabWithTimer is retained, so the outer exam timer still mounts as it did via TabContainer.

Testing

npm run types, npm run lint, and the affected suites pass. DiscussionTab.test.jsx renders <DiscussionTab /> through the bridged query client; CourseAccessErrorPage.test.jsx mocks useCourseHomeMeta (loading / has-access redirect / denied); redux.test.js is untouched (the shared fetchTab helper 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 fetchDiscussionTab caller so the
thunk can be deleted outright.

Scope: self-wrapping conversion

Decision. DiscussionTab becomes self-wrapping — it owns its access gating
via useCourseHomeMeta and renders <TabWithTimer> itself. The index.jsx
route drops <TabContainer tab="discussion" fetch={fetchDiscussionTab} slice="courseHome"> for a bare <DiscussionTab />.

The discussion tab is metadata-only — tabDataQuery becomes optional

Decision. fetchDiscussionTab was fetchTab(courseId, 'discussion') with
no getTabData — it only fetched course metadata (for access gating); the tab
itself builds the discussions-MFE iframe URL from courseId + the route path,
reading no tab-data model. So the converted tab has a metadataQuery and no
tab-data query, and passes courseStatus={{ metadataQuery }}.

To make that a first-class shape, TabPage's CourseStatus marks tabDataQuery
optional and deriveView guards its two checks (tabDataQuery?.isError
/ ?.isLoading). The data-bearing tabs (dates/outline/progress/live) still pass
both — nothing changes for them.

Rejected: passing metadataQuery as a dummy tabDataQuery to avoid touching
TabPage. That double-reads the same query and reads confusingly at the call
site; "a tab may have only metadata" is the honest model.

DiscussionTab splits into a thin wrapper + DiscussionTabContent

Decision. DiscussionTab runs 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/progress
splits: TabPage only renders children once loaded, so the plugin-event
registration and iframe mount happen post-load — matching today's timing, where
DiscussionTab is TabContainer's already-loaded child.

TabWithTimer is kept (the discussion tab always had the exam timer)

Decision. Keep TabWithTimer, not bare TabPage. The old Redux path wrapped
discussion in TabContainer, which renders TabWithTimer (→ OuterExamTimer).
So the discussion tab has always rendered the outer exam timer; keeping
TabWithTimer preserves that exactly.

No transitional bridge

Decision. useCourseHomeMeta keeps its existing courseHomeMeta bridge (as
on every tab, for the shared TabPage/LoadedTabPage reads), but the discussion
tab adds no discussion bridge. fetchDiscussionTab never wrote a
discussion model (no getTabData), so state.models.discussion was always
empty — the masquerade banner's useModel('discussion') already resolves empty
and never rendered on this tab. Preserved exactly, no bridge needed.

courseId from useParams

Decision. Both DiscussionTab and DiscussionTabContent read courseId
(and path) from useParams, replacing useSelector(state.courseHome.courseId).
Required: with no fetch thunk, state.courseHome.courseId is never set for this
route.

CourseAccessErrorPage converted too (the other fetchDiscussionTab caller)

Decision. generic/CourseAccessErrorPage.jsx dispatched fetchDiscussionTab
purely as a cheap access probe — metadata-only — then read
state.courseHome.courseStatus to decide: redirect home vs. show the denied
page. It's moved to useCourseHomeMeta(courseId). It never needed anything
discussion-specific; converting it lets fetchDiscussionTab be deleted entirely.

The four-state courseStatus branch maps cleanly onto the query:

Old (courseStatus) New (metadataQuery)
LOADING (initial + fetching) → loading isLoading → loading
LOADED (set only after a hasAccess success) → redirect data.courseAccess.hasAccess → redirect
DENIED / FAILED → denied page else (success-without-access or error) → denied page

useCourseHomeMeta is always enabled, so it's isLoading on first mount (no
premature "denied" flash before data arrives).

fetchDiscussionTab deleted; fetchTab kept for the live layer

Decision. Delete fetchDiscussionTab (both callers converted). Keep the
shared fetchTab helper and fetchLiveTab — the live tab still consumes them
until #2002.

Ordering: below live, so fetchTab retires cleanly there

Decision. Sequence discussion below live. After this layer,
fetchLiveTab is fetchTab's sole remaining consumer, so the live layer (#2002)
deletes fetchLiveTab, the now-orphaned fetchTab, and the redux.test.js
Test fetchTab block in one clean removal. The alternative order (live first)
would force retargeting that test block from fetchLiveTabfetchDiscussionTab
and then deleting it a layer later — churn on redux.test.js for no benefit.
Consequently redux.test.js is untouched in this layer.

Tests

  • DiscussionTab.test.jsx renders <DiscussionTab /> directly through the
    bridged query client (createTestQueryClient(store)), dropping the
    TabContainer + fetchDiscussionTab wrapper. The existing iframe-resize
    assertion is unchanged (it exercises DiscussionTabContent).
  • CourseAccessErrorPage.test.jsx mocks useCourseHomeMeta instead of the
    Redux useSelector/useDispatch; the three cases (loading / has-access
    redirect / 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
    useCourseHomeMeta swap is behavior-preserving: non-enrolled → renders the
    access-denied page (access-denied-main present, has_access:false in the
    metadata response, URL stays); enrolled → redirects to /redirect/home/<courseId>;
    brief loading spinner before metadata resolves.
  • Discussion embedded load — the discussions MFE renders embedded in the tab's
    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):

  • Navigate into the tab from another tab / in-iframe topic-click navigation
    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. The
    discussions.navigatenavigate wiring is unchanged from the pre-conversion
    component.
  • Exam timer on discussion — no active timed exam handy; preservation is assured
    structurally (TabContainer always wrapped discussion in TabWithTimer, and the
    converted tab keeps it).
  • Discussion route for a no-access course — redirects to /home via
    getAccessDeniedRedirectUrl('discussion') (inline denied is outline-only);
    matches master.

🤖 Generated with Claude Code

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

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.10%. Comparing base (8c3ef36) to head (47fcd97).

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.
📢 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 discussion tab to React Query

1 participant