refactor: de-class UnitButton and convert it to TypeScript

[brian-smith-tcril]

Summary

De-class UnitButton — the app's last connect() — and convert the file to TypeScript. This is a structural peel with no data-layer change (the #2008/#2012 convention): the component reads the same units model entry through useModel that mapStateToProps spread before, and renders identically for every caller. It's the first of two layers for Target 3 (bookmarking) of the courseware decomposition (plan) in the Redux → React Query migration (#1946, Stage 1), stacked on #2064; the bookmark-conversion layer above closes the issue. Part of #2014.

What changed

• connect → useModel('units', unitId). Connect's merge direction is preserved: model values win when the entry exists (useModel returns {} for a missing one, so the destructure defaults engage exactly where the spread of a missing entry did). The one caller relying on the prop fallback is the gated branch (SequenceNavigation: title="" / contentType="lock" for a unit with no model entry).
• TypeScript conversion (UnitButton.jsx → .tsx): propTypes/defaultProps are replaced by a Props interface with destructure defaults; the connect-injected bookmarked/complete leave the prop contract (no caller passes them — they're model-sourced); title/contentType become optional props for the gated fallback. The useSelector gets the Peel: convert CoursewareContainer to TypeScript (fast-follow to de-class #2008) #2019 RootState typing pattern.
• What deliberately stays: the state.courseware.{courseId,sequenceId} useSelector — it was never part of the connect (the issue body mis-described it) and is a bridge-written courseware-slice read owned by the Tear down the courseware Redux slice + replace useContextId #1976 teardown, same as the identical read in SequenceNavigation.jsx.

Testing

npm run types (0 errors), npm run lint (clean); all six sequence-navigation suites pass with no edits to any existing assertion (48 tests — two coverage cases added after codecov's patch check, see decision log §7), UnitButton.tsx at 100% statement/branch/function/line coverage, and the full jest suite green at the stack head (112 suites, 1113 passed / 3 pre-existing skips). Manual pass on tutor local (DemoX) in the details block below; the gated-sequence lock button is now also pinned directly by the new no-model-entry unit test.

Decisions

Full decision log

Decisions — Layer A: de-class UnitButton (#2014)

1. This issue ships as two stack layers; this is the structural peel. The two
   jobs in Convert bookmarking to React Query + de-class UnitButton #2014 are orthogonal — the bookmark mutation writes the units model,
   which UnitButton reads identically through connect or useModel — so the
   de-class goes first as its own PR (the Peel: de-class CoursewareContainer (structural, no data-layer change) #2008/Peel: convert checkBlockCompletion to a React Query mutation #2012 "peel: structural, no
   data-layer change" convention) and the bookmark conversion follows as a pure
   data-layer layer that closes the issue. The riskiest part of the issue
   (preserving connect's merge semantics) gets its own small, focused review.

2. useModel, not useParams, and the useSelector line stays. The issue
   body described the connect as covering state.models.units[unitId] +
   state.courseware.{courseId,sequenceId} and suggested useModel /
   useParams reads. The state.courseware read is not part of the connect —
   it was already a plain useSelector in the function body, identical to the
   one in SequenceNavigation.jsx that this issue doesn't touch. Both are
   bridge-written courseware-slice fields owned by the Tear down the courseware Redux slice + replace useContextId #1976 teardown, so this
   layer replaces only the actual connect (the units-model spread) and leaves
   the useSelector line untouched.

3. Merge direction preserved: model values over passed props. Connect's
   default merge is { ...ownProps, ...stateProps }, so the model entry wins
   whenever it exists. The rewrite destructures the model entry with prop
   fallbacks (title = fallbackTitle, contentType = fallbackContentType).
   useModel returns {} for a missing entry, so the fallbacks engage exactly
   where the connect spread of a missing entry did — the one caller relying on
   that is the gated branch (SequenceNavigation.jsx: title="",
   contentType="lock" for a unit with no model entry). Model entries always
   define title/contentType (page_title/type in
   normalizeSequenceMetadata), so default-on-undefined is equivalent to the
   spread for every real payload.

4. The file converts to TypeScript, and propTypes go entirely. With the
   connect wrapper gone the component's contract is worth stating precisely,
   and TS does that better than propTypes: a Props interface (onClick/unitId
   required; title/contentType/isActive/showCompletion/showTitle/
   className optional) replaces propTypes/defaultProps, with the old
   defaults becoming destructure defaults (isActive = false,
   showTitle = false, showCompletion = true). New/reworked files are TS per
   the project direction; the useSelector gets the Peel: convert CoursewareContainer to TypeScript (fast-follow to de-class #2008) #2019 typing pattern
   ((state: RootState) => state.courseware).

5. bookmarked/complete leave the prop contract. They were connect-injected
   only — no caller passes them — so they exist solely as destructure defaults
   off the model entry (bookmarked = false, complete = false, matching the
   old defaultProps). title/contentType stay as optional props for the
   gated-branch fallback (before, connect injected them ahead of the propTypes
   check, so their isRequired never actually fired for the callers that pass
   neither).

6. No behavior changes. Renders byte-identically for every caller, gated
   branch included; all six sequence-navigation suites pass with no edits to any
   existing assertion.

7. Two coverage cases added after codecov's patch check (the rename makes
   the whole file "new patch", and the repo's codecov has zero patch tolerance —
   its 4 "missing lines" were the destructure-default branches, which had zero
   hits suite-wide). Both pin previously-untested pre-existing behavior:

   • No-model-entry fallback (the gated shape: title="",
     contentType="lock", absent units entry → lock icon, nothing
     model-sourced rendered). This was genuinely uncovered everywhere: the
     existing SequenceNavigation "renders locked button for gated content"
     test seeds the units into the store, so the model entry overrides the lock
     props — its own TODO comment admits it asserts fa-tasks instead of a
     lock. The new case is the first to exercise the fallback direction of the
     connect-merge semantics this peel preserves.
   • /preview link prefix (the pathname.startsWith('/preview') ternary's
     true branch, previously never taken): rendered inside a MemoryRouter at a
     /preview/course/… entry, asserting the link href keeps the prefix.
     With these, UnitButton.tsx is at 100% statement/branch/function/line
     coverage.

Manual testing

Manual testing — Layer A: de-class UnitButton (#2014)

In-browser verification for the peel layer, run against a live backend (tutor
local). This layer claims zero user-facing change: UnitButton drops its
connect wrapper and reads the same units model entry through
useModel('units', unitId) instead — same state, same merge direction (model
values over passed props), same rendering for every caller. No network behavior
changes at all (the component fetches nothing).

The one code path with any subtlety is the gated-sequence fallback: the lock
button (SequenceNavigation's isLocked branch) passes title="" /
contentType="lock" for a unit with no model entry, relying on the prop-fallback
direction the de-class must preserve.

Getting real IDs (DemoX on tutor local)

Course id: course-v1:OpenedX+DemoX+DemoCourse; base
http://apps.local.openedx.io:2000/learning. Any unit page shows the sequence
nav. For the completion check and bookmark dot, use a sequence containing at
least one completed unit and one bookmarked unit (bookmark one via the
"Bookmark this page" button first if needed). The dropdown variant needs a
narrow viewport (below the small breakpoint) or a sequence with enough units
to overflow.

Verify by hand

• Desktop sequence nav renders identically — open a unit page: one button
  per unit with the right content-type icon, active unit highlighted,
  completed units show the check, the bookmarked unit shows the bookmark dot
  (top-right of its button).
• Clicking a unit button navigates — URL changes to
  /course/{courseId}/{sequenceId}/{unitId}, unit content loads.
• Overflow dropdown shows titles — the dropdown appears when the unit
  buttons overflow their container (useIndexOfLastVisibleChild), not at a
  viewport breakpoint: use a sequence with enough units and a narrow window
  (~375px device toolbar; an open outline sidebar narrows the container
  too). The "{n} of {m}" dropdown lists unit titles (the showTitle path,
  titles come from the model), completion/bookmark markers intact,
  selection navigates.
• Gated sequence renders the lock button — needs a prereq-gated course
  (not DemoX; rely on the existing SequenceNavigation suite if none is
  handy): the locked sequence shows the single lock-icon button (the
  prop-fallback path — no model entry for the unit).
• Staff preview links stay on /preview — in a unit page under
  /preview/course/…, unit-button links keep the /preview prefix.

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

• All seven UnitButton.test.jsx cases (title hidden/shown, completion
  shown/hidden, bookmark dot, click handler) — unchanged assertions, now
  exercising the useModel read.
• The SequenceNavigation* suites (including the locked-sequence rendering)
  render UnitButton through the seeded store — unchanged.

Results

Env: tutor local, course-v1:OpenedX+DemoX+DemoCourse, run against the local
branch @ 1d801b20 (before any PR).

The four checked items passed as described; nothing surprising observed. The
overflow dropdown needed the real trigger — button overflow of the tabs
container, not a viewport breakpoint — so the checklist item was corrected
mid-pass. The gated-sequence item was not run by hand (no prereq-gated course
locally); it rests on the SequenceNavigation suite's locked-sequence
rendering case, which passes. Setup note: a local env.config.jsx had to be
added for the app to render at all — the file is untracked in this repo (known
local-dev quirk, unrelated to this branch's change).

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.71%. Comparing base (df9f938) to head (a9c2168).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2065      +/-   ##
==========================================
+ Coverage   93.68%   93.71%   +0.02%     
==========================================
  Files         369      369              
  Lines        6005     6017      +12     
  Branches     1423     1429       +6     
==========================================
+ Hits         5626     5639      +13     
+ Misses        363      362       -1     
  Partials       16       16              

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

[arbrandes]

👍🏼