refactor: de-class CoursewareContainer

[brian-smith-tcril]

Summary

De-class CoursewareContainer: convert it from a class + connect component to a functional component using hooks. Purely structural — no data-layer or behavior change (same thunks, same store reads). First peel of the courseware-player decomposition (#1976 → #2008–#2016), part of the Redux → React Query migration (#1946, Stage 1). Stacked on the live-tab conversion (#2006) as the bottom of the courseware chain. Closes #2008.

Why first: CoursewareContainer is the single orchestrator that calls fetchCourse / fetchSequence; a class can't call the useQuery hooks the later peels introduce, so it has to become a function component before any of them. TypeScript is deliberately not part of this PR — it's the fast-follow #2019 (stacked directly above), so this one reads as a pure structural change.

What changed

• src/courseware/CoursewareContainer.jsx — class → function component.
  • connect → useSelector / useDispatch; route inputs via inlined useParams / useNavigate / useLocation.
  • The reselect memoize fetch/save guards are preserved (held in a useRef, reading a per-render latest ref — the analog of this.props always being current), inside a single no-dependency useEffect that reproduces componentDidMount + componentDidUpdate exactly. The de-dupe mechanism is unchanged (memoize, not dependency arrays).
  • Drop the dead previousSequenceSelector + previousSequence prop (unused; nextSequence stays). The empty previousSequenceHandler is kept — Course → Sequence still requires it.
  • PropTypes / defaultProps removed — the component now takes no props.
• src/courseware/utils.jsx — deleted. withParamsAndNavigation existed only to feed route params into the class; CoursewareContainer was its only consumer.
• src/courseware/course/sequence/Unit/hooks/useIFrameBehavior.ts — guard activeSequence.unitIds?.length > 0 (see Behavior).
• src/courseware/data/slice.js — drop the dead fetchCourseRecommendations{Request,Success,Failure} exports (orphaned from the recommendations RQ conversion in refactor: convert course recommendations to React Query #1967; no readers).

Behavior

No user-facing change — the de-class preserves the exact fetch orchestration, URL-normalization redirects, sequence-position save, and store reads.

One downstream guard is required. Removing connect means useIFrameBehavior's read of the global store id (getSequenceId → state.courseware.sequenceId) can briefly lead the rendered sequenceId prop during a navigation, so the sequence model it looks up can transiently be the course-outline stub (no unitIds). The unconditional activeSequence.unitIds.length then threw Cannot read properties of undefined (reading 'length'). Guarding with ?.length > 0 makes that transient render a no-op (activeUnitId = null) that resolves once the sequence loads.

This is isolated, not the tip of an iceberg: useIFrameBehavior is the only component in the courseware render path that reads the global id and useModel('sequences', …) on it. Every other unguarded .unitIds reader takes sequenceId as a prop/arg — the value that passed Sequence's sequenceStatus === 'loaded' gate — so it's always consistent with the render that mounted it. connect kept the global id and the rendered prop in lockstep, which masked this latent fragility; the full analysis is in the decision log.

Testing

npm run types, npm run lint, and the full suite pass. CoursewareContainer.test.jsx's existing cases are unchanged (they render <CoursewareContainer /> with no props, driven by URL + store + mocks); two tests were added to cover the relocated lines codecov's patch flagged: (1) handleUnitNavigationClick → checkBlockCompletion, via a mock-prefixed opt-in flag that renders the real unit nav only in that test so a Next click exercises the handler (asserted through the get_completion POST); (2) firstSequenceIdSelector's empty-sectionIds branch, via a sectionless course that asserts the resume redirect doesn't fire (no first sequence to pick). The other cases are untouched. The one remaining flagged line is the handleNextSequenceClick celebration branch, which codecov's diff shows unmoved (byte-identical, not counted as patch), pre-existing and equally uncovered on master. In-browser parity sweep verified core load, all URL-normalization redirects, resume, next/prev unit + sequence, and unit completion; plugin/preview/access cases were not exercised in-browser (details in the manual-testing log). Full logs below.

Decisions

Full decision log

Decisions — de-class CoursewareContainer (#2008)

Working notes for this PR (part of the wider Redux → React Query migration,
#1946). Not checked in — referenced when opening the PR. First peel of the
courseware-player decomposition (#1976 → #2008–#2016); stacked on the live-tab
conversion (#2006) as the bottom of the courseware chain. Closes #2008.

This layer is purely structural: CoursewareContainer goes from a
class + connect to a function component using hooks. No data-layer or
behavior change — it dispatches the same thunks and reads the same store. It
exists so the later peels can call useQuery hooks a class can't.

Stays .jsx; TypeScript is the fast-follow #2019

Decision. The de-class is already a churn-y structural diff; layering a
.tsx rename + type annotations on top would make it harder to review. TypeScript
is split into #2019 (stacked directly above this), so the RQ peels happen in
TS-land while this PR reads as a single idea.

Effects: keep the reselect memoize guards (faithful, not dep-arrays)

Decision. The class de-duped its work with reselect memoize (last-args
guards checkFetchCourse / checkFetchSequence / checkSaveSequencePosition),
not React dependency arrays. We preserve that exact mechanism:

• The three guards live in a useRef (stable identity, created once) — recreating
  them per render would reset their last-args memoization.
• They read current values through a per-render latest ref, the analog of
  this.props always being current (so checkSaveSequencePosition reads the live
  sequence/ids, never a stale closure).
• componentDidMount + componentDidUpdate collapse into a single
  useEffect with no dependency array (runs after every render), body identical
  to the old componentDidUpdate.

Why not idiomatic dep-array effects. Swapping the de-dupe engine to React deps
now would mean proving every dependency array exactly right across a redirect block
that reads many inputs — a large correctness argument for no functional gain. We
prove equivalence bit-by-bit as each fetch becomes a useQuery later, when the
guard for that fetch falls away on its own.

Mount-runs-everything is safe. The class ran only the two fetches on mount and
everything on update; the no-dep effect runs the whole body on mount too. Harmless:
saveSequencePosition and all redirects gate on sequenceStatus/courseStatus === 'loaded' (false at mount), and the ids-mismatch bail returns early at mount anyway.

Inline the router hooks; delete utils.jsx

Decision. withParamsAndNavigation (src/courseware/utils.jsx) existed only to
feed route params/navigate into the class. The function component calls
useParams / useNavigate / useLocation directly, so utils.jsx is deleted —
CoursewareContainer was its only consumer (verified repo-wide).

Dead code removed: previousSequence selector + prop

Decision. Remove previousSequenceSelector and the previousSequence prop it
fed. Keep the empty previousSequenceHandler (() => {}) — that's a different
thing, a required prop of Course → Sequence.

We specifically questioned this during review — is previousSequence actually
dead, or a plugin/top-nav extension point we'd be breaking? Verified it is dead:

• The selector is not exported, and repo-wide previousSequence\b has zero
  readers outside this file. Its only consumer was mapStateToProps
  (previousSequence: previousSequenceSelector(state)), feeding a prop that
  render()/handlers never read. Chain: selector → prop → nowhere.
• Contrast nextSequence, which is live — handleNextSequenceClick reads it for
  the next-section celebration. So next stays, previous goes.
• The top-nav plugin does not consume it. The sequence-navigation slots
  (SequenceNavigationSlot, NextUnitTopNavTriggerSlot,
  SequenceBottomNavigationSlot) compute their own prev/next in
  src/courseware/course/sequence/sequence-navigation/hooks.js
  (previousSequenceId = sequenceIndex > 0 ? sequenceIds[sequenceIndex - 1] : null
  → previousLink). Previous-sequence navigation runs through
  previousSequenceHandler (kept) + that hook's own computation, never through the
  container's previousSequence data.

Dead exports removed: fetchCourseRecommendations*

Decision. Drop the fetchCourseRecommendations{Request,Success,Failure}
re-exports from src/courseware/data/slice.js — no readers anywhere outside
slice.js (verified).

Strictly unrelated to the de-class — these are orphaned leftovers from the
course-recommendations React Query conversion (#1967, already merged), which
dropped the reducers but left the dead destructured exports (so they've resolved
to undefined since). Because the PR that should have removed them has already
landed on master, there's no cleaner owner elsewhere in the stack, so this
trivial deletion rides along here rather than manufacturing a separate layer.

Guard useIFrameBehavior's sequence read (crash surfaced by the de-class)

Decision. In useIFrameBehavior.ts, change the unconditional
activeSequence.unitIds.length > 0 to activeSequence.unitIds?.length > 0 — add
the ?. null-guard while preserving the exact > 0 predicate. This is the one
downstream change this PR makes outside CoursewareContainer.

Why — the crash. Manual testing (jumping between units via the course-outline
sidebar) hit Uncaught TypeError: Cannot read properties of undefined (reading 'length') at useIFrameBehavior.ts:42, flashing the ErrorBoundary then
recovering. Instrumentation caught the exact state: useIFrameBehavior was
rendering while sequenceStatus === 'loaded'/mid-transition against a sequence
whose model was still the outline stub fetchCourse seeds (keys=[id, title, sectionId], no unitIds). A 22k-line master log of the same navigation never
crashes; my branch hit it on essentially every jump.

Why it's isolated, not the tip of an iceberg. useIFrameBehavior is the
only component in the courseware render path that reads the global store id
(getSequenceId → state.courseware.sequenceId) and then useModel('sequences', …) on it. Every other unguarded .unitIds reader (Sequence,
SequenceNavigation, UnitNavigationEffortEstimate,
useSequenceNavigationMetadata) takes sequenceId as a prop/arg — the same
value that passed Sequence's sequenceStatus === 'loaded' gate — so their model
is always the loaded one, consistent with the render that mounted them. Only
useIFrameBehavior bypasses the prop, so during a transition its global id can
lead the rendered prop and point at the not-yet-loaded stub.

connect (the pre-de-class wiring) delivered store updates to CoursewareContainer
and its subtree in lockstep, so the global id and the rendered prop never
disagreed and this latent fragility stayed hidden. Removing connect in favor of
independent useSelector subscriptions lets them momentarily diverge, exposing it.
So this is a "should have been guarded all along" fix — reading the global
sequenceId and assuming a fully-loaded model was never safe — not a symptom of
broad breakage. The grep confirming this (single global-id render-path reader) is
the basis for that confidence.

Tests: existing suite unchanged + one added coverage test

Decision. The existing CoursewareContainer.test.jsx cases are left as-is —
they render <CoursewareContainer /> with no props inside <AppProvider store> +
<Routes>, driven by URL + store + axios mocks, and separately unit-test the
exported redirect helpers. Behavior is identical, so they pass without edits.

Two tests added to cover the relocated lines codecov's patch flagged (both
rewritten/moved by the de-class, so codecov sees them as new; both were equally
uncovered on master):

• handleUnitNavigationClick → checkBlockCompletion (was
  this.props.checkBlockCompletion(...) → dispatch(...)). The existing suite
  mocks Unit as a bare div with no navigation, and 7 cases assert
  assertNoSequenceNavigation, so the mock can't render nav globally. A
  mock-prefixed opt-in flag (mockRenderUnitNav, reset in afterEach) makes the
  Unit mock render the real renderUnitNavigation() only in the new test; a
  Next click drives handleNext → unitNavigationHandler → handleUnitNavigationClick,
  asserted via the get_completion POST. Existing cases untouched (flag defaults off).
• firstSequenceIdSelector's empty-sectionIds branch — a course with no sections
  makes firstSequenceId resolve to null, so the resume redirect can't pick a
  first sequence. The test loads a sectionless course at /course/:courseId and
  asserts no redirect happens (URL stays at the course root, no unit rendered) —
  exercising the branch via its observable behavior, not just for the line hit.

The one remaining codecov-flagged line is the handleNextSequenceClick celebration
branch, which codecov's diff shows unmoved (byte-identical, no +/− markers) so it
isn't counted as patch — pre-existing, equally uncovered on master, left as-is.

Manual testing sweep

Manual testing — de-class CoursewareContainer (#2008)

Scoped to what this PR changes: CoursewareContainer goes from a class +
connect to a function component on hooks. It is a purely structural change —
same thunks, same store reads, same behavior. So this list is a parity sweep:
everything the container orchestrated should work exactly as it did on master.

The risky part is that componentDidMount + componentDidUpdate (data fetches,
the URL-normalization redirects, and sequence-position saving) now run in a single
render useEffect, and the memoize de-dupe guards moved into refs. Focus testing
on those flows.

Mark results as you go — [x] pass, [!] problem (add a note), [-] not tested (with reason).

Automated tests already cover (68 passing in CoursewareContainer.test.jsx,
unchanged): initial spinner, successful course/sequence render, the
course_access error path, and all six redirect helpers as units. So this list
targets the real-browser navigation/timing behaviors those can't fully exercise.

1. Core load

• Open a course at a full /course/:courseId/:sequenceId/:unitId URL: spinner →
  course header + sequence + unit render, no flash/crash/double-fetch.
• Hard-reload directly on that URL (cold cache, nothing pre-fetched) — loads clean.

2. URL normalization redirects (the effect's main job)

Each of these should land on the normalized /course/:courseId/:sequenceId/:unitId
URL, same as master:

• Bare /course/:courseId → resume redirect to the last active
  sequence/unit (or the first sequence if none active).
• /course/:courseId/:sequenceId (no unit) → fills in the active-or-first unit.
• /course/:courseId/:sequenceId/first and .../last → resolve to the first /
  last unit of the sequence.
• /course/:courseId/:sectionId (a section id where a sequence is expected) →
  redirects to the section's first sequence.
• /course/:courseId/:unitId (bare unit) → fills in the parent sequence id.
• A URL whose "sequence" is actually a unit (the 422 sequenceMightBeUnit
  path) → redirects to the correct parent sequence, then the unit.
• Confirm none of the above ends up in a redirect loop or a flicker between URLs.

3. Sequence-position save + resume

saveSequencePosition now fires from the effect via the ref'd guard.

• Navigate across a few units in a sequence, then hard-reload the course root
  (/course/:courseId) — resume lands on the last unit you were on.
• [-] Rapidly click through units — position saves once per unit (no duplicate/
  stale saves; the memoize guard should still de-dupe on the unit id).
  (Not specifically exercised; resume itself verified above.)

4. In-sequence and cross-sequence navigation

Exercises the nav slots (top + bottom) and the next/previous handlers passed to
Course:

• Next / Previous unit buttons move within the sequence.
• Next / Previous sequence (stepping off the first/last unit) moves across
  sequences and lands correctly.
• [-] The top-nav "next unit" trigger works (the previously-questioned nav path).
  (Not tested — it's a plugin slot requiring env.config.jsx setup. The nav
  computes prev/next itself in sequence-navigation/hooks.js, independent of
  the container.)

5. Next-section celebration

handleNextSequenceClick still fires handleNextSectionCelebration.

• [-] On a course with first-section celebration enabled, advancing from the last
  unit of the first section into the next section shows the celebration modal
  (same as master). (Not tested — needs celebration setup. Note the handler
  is unchanged from the class version.)

6. Unit completion

handleUnitNavigationClick → checkBlockCompletion.

• Completing/advancing a unit marks it complete (check icon / progress updates),
  same as before.

7. Preview mode

isPreview is now derived from useLocation().pathname instead of the HOC.

• [-] Load a /preview/course/... URL — the same redirects fire but keep the
  /preview prefix on every normalized URL. (Not tested.)

8. Access / error states

• [-] A course you can't access (courseStatus: 'denied') shows the denied/redirect
  behavior, unchanged. (Not tested.)
• [-] A course_access error renders the error state, not a crash. (Not tested
  in-browser; covered by the automated course_access test in CoursewareContainer.test.jsx.)

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.51%. Comparing base (62d92eb) to head (4ca8e3a).

Additional details and impacted files

@@                             Coverage Diff                             @@
##           bsmith/react-query-course-home-live-tab    #2020      +/-   ##
===========================================================================
+ Coverage                                    93.28%   93.51%   +0.22%     
===========================================================================
  Files                                          363      362       -1     
  Lines                                         5901     5889      -12     
  Branches                                      1404     1401       -3     
===========================================================================
+ Hits                                          5505     5507       +2     
+ Misses                                         381      367      -14     
  Partials                                        15       15              

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