Convert bookmarking to React Query + de-class UnitButton

[brian-smith-tcril]

Part of #1946 — Redux → React Query migration (Stage 1). Part of the #1976 courseware decomposition (plan) — Target 3 (bookmarking). Stacked on the sequence conversion (owns the units model).

Goal: convert bookmarking to React Query and drop UnitButton's connect.

Tasks

• Convert addBookmark / removeBookmark (src/courseware/course/bookmark/data/thunks.js) → useMutation, optimistically patching bookmarked / bookmarkedUpdateState on the units cache.
• De-class src/courseware/course/sequence/sequence-navigation/UnitButton.jsx: replace its connect (state.models.units[unitId] + state.courseware.{courseId,sequenceId}) with useModel / useParams reads.
• UnitTitleSlot continues to read bookmarked from the units model.

Verify: bookmark toggle works (optimistic + rollback on error); UnitButton renders bookmark/complete/title identically; no connect left in UnitButton.

Plan

Note

The findings and plan below were generated by Claude (Claude Code) and reviewed before posting.

Investigation findings that adjust the task list above:

• This issue ships as two stack layers. The two tasks are orthogonal — the mutation writes the units model, which UnitButton reads identically through connect or useModel — so the de-class goes first as its own structural peel PR (the Peel: de-class CoursewareContainer (structural, no data-layer change) #2008/Peel: convert checkBlockCompletion to a React Query mutation #2012 "peel: no data-layer change" convention), and the bookmark conversion follows as a pure data-layer PR that closes this issue. The riskiest part of the issue (preserving connect's merge semantics) gets its own small, focused review.
• The units "cache" is the model store until Dissolve the model-store normalized cache #1977. Every bookmarked reader (Unit → UnitTitleSlot, and UnitButton after the de-class) is a useModel('units', …) read, and the model store stays the merged source of truth for units through the transition. The mutation therefore dispatches updateModel — exactly what useCheckBlockCompletion's onSuccess does for complete — not setQueryData on the sequence query. Durability was verified against the bridge: bridgeToModelStore runs only on a real fetch onSuccess (QueryCache hook), never on cache-hit remounts, and a real refetch carries server truth including the new bookmark state, so an optimistic write can't be reverted by stale cached data. Patching the sequence query cache becomes necessary only when Dissolve the model-store normalized cache #1977 moves the readers onto query data — noted for that issue.
• No useParams swap in UnitButton. The state.courseware.{courseId,sequenceId} read is not part of the connect — it's already a plain useSelector (UnitButton.jsx:24), 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. The peel replaces only the actual connect (the state.models.units[unitId] spread → useModel) and leaves the useSelector line alone.
• One mutation, not two. The two thunks are mirror images differing only in the target boolean and the api call, and BookmarkButton already computes the direction for its conditional dispatch — so a single useSetBookmarked with the direction in its variables replaces both, rather than a thunk-mirroring useAddBookmark/useRemoveBookmark pair. It's named for what it does — set bookmarked to a supplied value — while the toggle lives in the component's named toggleBookmark handler, the thing that knows the current state. Its onMutate/onSuccess/onError map 1:1 onto the thunks' optimistic / confirm / revert-plus-logError dispatches (mutations aren't covered by the global QueryCache.onError, so the explicit onError follows the useCheckBlockCompletion precedent).
• The de-class must preserve connect's merge direction. mapStateToProps spreads the model entry over the passed props, so model values win when the entry exists; the one caller relying on the fallback is the gated branch (SequenceNavigation.jsx: title="", contentType="lock" for a unit with no model entry). The rewrite destructures the model entry with prop fallbacks. The component also converts to TypeScript in the same pass (UnitButton.tsx): propTypes/defaultProps are replaced by a Props interface with destructure defaults — title/contentType become optional (only the gated branch passes them; today connect injects them before propTypes run, so isRequired never fires for the callers that pass neither), and the connect-injected bookmarked/complete leave the prop contract entirely.
• No behavior changes intended in either layer — the peel renders byte-identically for every caller, and the conversion is a fully faithful port: same optimistic flip timing, same revert-on-error, same logError, same model writes, same isProcessing derivation in UnitTitleSlot. One new test the thunk shape couldn't express cleanly: pinning that the optimistic loading write lands before the request resolves (hanging mock).

Full plan

Plan: #2014 — Convert bookmarking to React Query + de-class UnitButton

Context

Part of epic #1946 (Redux → React Query, Stage 1) and the #1976 courseware
decomposition — Target 3 (bookmarking). The issue's two jobs are orthogonal
(neither depends on the other — the mutation writes the units model, which
UnitButton reads identically through connect or useModel), so they ship as
two stack layers on top of #2064, peel first per the #2008/#2012 convention:

1. Layer A — peel: de-class UnitButton.jsx (structural, no data-layer
   change) — the app's last connect() (mapStateToProps spreads
   state.models.units[unitId] over the passed props).
2. Layer B — convert addBookmark / removeBookmark
   (src/courseware/course/bookmark/data/thunks.js) — the last
   model-store-writing thunks outside courseware/data. Each optimistically
   dispatches updateModel({ modelType: 'units', … }) three ways: loading
   (optimistic flip), loaded (confirm), failed (revert + logError). Sole
   dispatcher: BookmarkButton.jsx. This layer closes Convert bookmarking to React Query + de-class UnitButton #2014.

Readers of the written state (all via the units model, none change):

• Unit/index.jsx → useModel('units', id) → UnitTitleSlot (unit.bookmarked,
  isProcessing = unit.bookmarkedUpdateState === 'loading' → BookmarkButton props).
• UnitButton.jsx (bookmarked dot on sequence-nav buttons) — via the connect today,
  via useModel after Layer A.

Corrections to the issue body.

• "optimistically patching bookmarked / bookmarkedUpdateState on the units
  cache" — the units readers are useModel('units', …) reads of the model
  store, which stays the merged source of truth for units until Dissolve the model-store normalized cache #1977. The
  mutation therefore keeps dispatching updateModel (exactly what
  useCheckBlockCompletion's onSuccess does for complete), not
  setQueryData on the sequence query. Durability check (verified against
  src/data/modelStoreBridge.ts / src/queryClient.ts): the bridge re-writes
  units only on a real fetch onSuccess (QueryCache hook), never on cache-hit
  remounts — and a real refetch carries server truth including the new bookmark
  state, so nothing reverts. Patching the sequence query cache becomes necessary
  only when Dissolve the model-store normalized cache #1977 moves the readers onto query data — noted there.
• "replace its connect (state.models.units[unitId] +
  state.courseware.{courseId,sequenceId}) with useModel / useParams reads" —
  the state.courseware read is not part of the connect: it's already a plain
  useSelector (UnitButton.jsx:24), identical to the one in
  SequenceNavigation.jsx:39 that this issue doesn't touch. Both are
  courseware-slice reads owned by the Tear down the courseware Redux slice + replace useContextId #1976 teardown (courseId/sequenceId are
  bridge-written fields). Layer A replaces only the connect (units model →
  useModel) and leaves the useSelector line untouched — no useParams swap here.

---

Layer A — peel: de-class UnitButton (structural, no data-layer change)

Key files

• src/courseware/course/sequence/sequence-navigation/UnitButton.jsx — the change;
  renamed to UnitButton.tsx.
• src/courseware/course/sequence/sequence-navigation/UnitButton.test.jsx — verify.
• Callers (SequenceNavigationTabs.jsx, SequenceNavigationDropdown.jsx,
  SequenceNavigation.jsx) — untouched.

The change

Replace the connect wrapper with a useModel('units', unitId) read:

• Connect semantics to preserve: mergedProps = { ...ownProps, ...state.models.units[unitId] }
  — model values win when the entry exists; passed props only matter when it
  doesn't. The one caller that relies on the fallback is the gated branch
  (SequenceNavigation.jsx:49: title="", contentType="lock" for a unit that has
  no model entry). The other callers (SequenceNavigationTabs,
  SequenceNavigationDropdown via Dropdown.Item as=) pass no model-sourced props
  at all — title/contentType/complete/bookmarked come entirely from the model.
• Implementation: destructure the model entry with prop fallbacks
  (const { title = titleProp, contentType = contentTypeProp, complete = false, bookmarked = false } = useModel('units', unitId) ?? {};
  — exact spelling at implementation time). Model entries always define
  title/contentType (from page_title/type in normalizeSequenceMetadata),
  so default-on-undefined matches the spread for every real payload.
• The file converts to TypeScript (git mv → UnitButton.tsx) and
  propTypes/defaultProps go entirely: a Props interface types the contract
  (onClick, unitId required; title/contentType/isActive/showCompletion/
  showTitle/className optional), and the old defaultProps become destructure
  defaults (isActive = false, showTitle = false, showCompletion = true).
  bookmarked/complete leave the prop contract — they were connect-injected
  only (no caller passes them), so they exist solely as model-entry destructure
  defaults. 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).
• The useSelector stays (see Corrections) and gets the Peel: convert CoursewareContainer to TypeScript (fast-follow to de-class #2008) #2019 typing pattern:
  useSelector((state: RootState) => state.courseware) with
  import type { RootState } from '../../../../store'.
• export default UnitButton (no wrapper); connect and PropTypes imports go.

Tests

• UnitButton.test.jsx: no assertion changes expected — initializeTestStore
  seeds the units model the same way, and useModel reads the same state the
  connect spread did. Verify all seven cases green; watch the no-router renders
  (the component already calls useLocation, so any router-wrapping quirks
  predate this change).
• SequenceNavigation* suites render UnitButton through the store — same reads,
  no edits expected.

Behavior changes

None — renders byte-identically for every caller, gated branch included.

---

Layer B — convert bookmarking to React Query (closes #2014)

Key files

• src/courseware/course/bookmark/data/api.js — createBookmark /
  deleteBookmark. Unchanged.
• src/courseware/course/bookmark/data/thunks.js — deleted (both thunks; only
  importers are BookmarkButton.jsx and data/redux.test.js).
• src/courseware/course/bookmark/data/apiHooks.ts — new: the mutation hook.
• src/courseware/course/bookmark/BookmarkButton.jsx — swap useDispatch +
  thunks for the hook.
• src/courseware/course/bookmark/index.js — unchanged (only exports
  BookmarkButton).
• src/plugin-slots/UnitTitleSlot/index.jsx, src/courseware/course/sequence/Unit/index.jsx
  — untouched (prop flow and the unit model shape are unchanged).
• Tests: bookmark/data/redux.test.js → bookmark/data/apiHooks.test.tsx,
  BookmarkButton.test.jsx.

1. bookmark/data/apiHooks.ts — one mutation, not two

The two thunks are byte-for-byte mirror images differing only in the target
boolean and the api call. One mutation with the direction in its variables
replaces both (the BookmarkButton already computes the direction for its
conditional dispatch):

export const useSetBookmarked = () => {
  const dispatch = useDispatch();
  const setBookmarkState = (unitId: string, bookmarked: boolean, bookmarkedUpdateState: string) => {
    dispatch(updateModel({ modelType: 'units', model: { id: unitId, bookmarked, bookmarkedUpdateState } }));
  };
  const { mutate } = useMutation({
    mutationFn: ({ unitId, bookmarked }: ToggleBookmarkVars) => (
      bookmarked ? createBookmark(unitId) : deleteBookmark(unitId)
    ),
    onMutate: ({ unitId, bookmarked }) => setBookmarkState(unitId, bookmarked, 'loading'),
    onSuccess: (data, { unitId, bookmarked }) => setBookmarkState(unitId, bookmarked, 'loaded'),
    onError: (error, { unitId, bookmarked }) => {
      logError(error);
      setBookmarkState(unitId, !bookmarked, 'failed');
    },
  });
  return useCallback((unitId: string, bookmarked: boolean) => mutate({ unitId, bookmarked }), [mutate]);
};

• onMutate/onSuccess/onError map 1:1 onto the thunks' optimistic /
  confirm / revert dispatches — same values, same order, same logError in the
  failure path (mutations are not covered by the global QueryCache.onError;
  explicit onError per the useCheckBlockCompletion precedent).
• Returns a callback (not the mutation object) per the same precedent; the
  model-store write path means no component needs isPending
  (isProcessing keeps deriving from bookmarkedUpdateState in UnitTitleSlot).
• No mutation key / no query-cache writes (rationale under Corrections).
• Naming: the hook is useSetBookmarked, not useToggleBookmark — it doesn't
  read the current state; the caller supplies the target value, so its semantics
  are "set bookmarked to X". The toggle lives in the component handler, which
  is what knows the current state.
• Alternative shape — two hooks useAddBookmark/useRemoveBookmark mirroring the
  thunk pair — rejected as pure duplication once the direction is a variable;
  recorded in the decision doc.

2. BookmarkButton.jsx

Drop useDispatch + the thunks import; const setBookmarked = useSetBookmarked();
the handler becomes a plain named function, keeping the onClick={toggleBookmark}
JSX line byte-identical to master:

const toggleBookmark = () => setBookmarked(unitId, !isBookmarked);

The useCallback (and its eslint-disable exhaustive-deps) goes — the handler's
only changing input is isBookmarked, so memoization buys nothing
(StatefulButton isn't memoized), and no disable comment survives the rewrite.

3. Cleanup: delete bookmark/data/thunks.js and bookmark/data/redux.test.js

Their content moves to apiHooks.ts / apiHooks.test.tsx. No other importers
(grepped: only BookmarkButton.jsx + the test). setupTest.js seeds no bookmark
state and mocks no bookmark URLs — untouched.

Tests

bookmark/data/apiHooks.test.tsx (new). Port the four redux.test.js cases
onto the courseware/data/apiHooks.test.tsx mutation pattern (renderHook +
AppProvider store × QueryClientProvider wrapper via createTestQueryClient,
axios-mock on the bookmark URLs):

• add: success → units[unitId] has { bookmarked: true, bookmarkedUpdateState: 'loaded' },
  POST body { usage_id }.
• add: network error → logError called, model reverted to
  { bookmarked: false, bookmarkedUpdateState: 'failed' }.
• remove: success → { bookmarked: false, … 'loaded' }, DELETE URL contains
  {username},{unitId}.
• remove: network error → logError, reverted { bookmarked: true, … 'failed' }.
  Plus one new case the thunks version couldn't express cleanly: the optimistic
  loading write lands before the request resolves (hanging mock, assert
  bookmarkedUpdateState === 'loading' mid-flight) — this pins the optimistic
  semantics the whole feature exists for.

BookmarkButton.test.jsx. Stays structurally intact: it renders through
setupTest's render (store + query client both provided), keeps its axios mocks
(the mutation still calls the same api.js fns), and keeps asserting
store.getState().models.units[…] (the bridge is still the source of truth).
Expected diffs: none beyond the import surface — verify green as-is; convert
fireEvent → userEvent only if the file needs touching anyway.

Existing coverage to sweep. Unit/index.test.jsx mocks useModel units with
bookmarkedUpdateState — shape unchanged, no edits expected.

Behavior changes

None intended — a faithful port: same optimistic flip timing, same
revert-on-error, same logError, same model writes (three per toggle), same
isProcessing derivation, fire-and-forget from the button either way.

---

Conventions (both layers): userEvent, no eslint-disable, new files TS,
rationale in the decision doc not inline.

Decision doc

Split across the two PR bodies at submit time (each layer carries its own
decisions):

• Layer A (peel): the connect merge semantics and the gated-branch prop
  fallback; the TypeScript conversion (propTypes/defaultProps replaced by a
  Props interface + destructure defaults; connect-injected bookmarked/
  complete dropped from the contract); the no-useParams correction (the
  state.courseware read is Tear down the courseware Redux slice + replace useContextId #1976's).
• Layer B (bookmarking): the model-store-is-the-units-cache correction (no
  setQueryData; bridge durability); one mutation instead of a thunk-mirroring
  pair; the deferred sequence-query-cache patch (a Dissolve the model-store normalized cache #1977 note); the new
  optimistic-write test.
• Both: the decision to split the issue into two layers (orthogonal changes; peel
  convention from Peel: de-class CoursewareContainer (structural, no data-layer change) #2008/Peel: convert checkBlockCompletion to a React Query mutation #2012).

Stack

Two new layers stacked on #2064 (outline sidebar), in order: the UnitButton peel
(references this issue), then the bookmark conversion (closes this issue).
Submitted once green.

Verification

• Layer A: npm run test -- src/courseware/course/sequence/sequence-navigation.
• Layer B: npm run test -- src/courseware/course/bookmark src/courseware/course/sequence/Unit.
• Both: npm run types and npm run lint.
• Manual smoke (tutor local, DemoX), after Layer A: unit buttons render
  identically (titles in dropdown, completion check, bookmark dot, gated
  sequence's lock button). After Layer B: bookmark toggle on a unit page — icon
  fills optimistically, StatefulButton disables while loading, POST/DELETE in
  the Network tab; bookmarked dot appears on the unit's sequence-nav button (and
  in the narrow-viewport dropdown); revert path via DevTools offline → toggle →
  flag flips back + console error.