refactor: convert bookmarking to React Query

[brian-smith-tcril]

Summary

Convert bookmarking off Redux to React Query: the addBookmark/removeBookmark thunks — the last model-store-writing thunks outside courseware/data — become a single useSetBookmarked mutation with the same optimistic semantics. This completes Target 3 (bookmarking) of the courseware decomposition (plan) in the Redux → React Query migration (#1946, Stage 1), stacked on the UnitButton peel #2065. Closes #2014.

What changed

• New bookmark/data/apiHooks.ts: useSetBookmarked. One mutation replaces the two mirror-image thunks — the caller supplies the target value, so the hook is named for what it does (set bookmarked to X) while the toggle lives in the component handler that knows the current state. onMutate/onSuccess/onError map 1:1 onto the thunks' optimistic loading flip / loaded confirm / failed revert + logError — all still updateModel dispatches, because the model store remains the merged source of truth for units until Dissolve the model-store normalized cache #1977 dissolves it (the readers are useModel reads; a query-cache write would update state nothing reads).
• BookmarkButton drops useDispatch, the thunks import, and its useCallback + eslint-disable exhaustive-deps; the handler is a plain named toggleBookmark = () => setBookmarked(unitId, !isBookmarked), keeping the onClick={toggleBookmark} JSX line byte-identical to master.
• Deletions: bookmark/data/thunks.js and bookmark/data/redux.test.js (no orphaned coverage — the only code the test exercised was the thunks file, deleted with it).
• Tests: new bookmark/data/apiHooks.test.tsx ports the four redux.test.js cases assertion-for-assertion (the case-by-case mapping table is in the decision log §7), plus one net-new case pinning that the optimistic loading write lands before the request resolves. BookmarkButton.test.jsx needed no changes.

Testing

npm run types (0 errors), npm run lint (clean), full jest suite green at head (112 suites, 1111 passed / 3 pre-existing skips). Manual pass on tutor local (DemoX) in the details block below; the revert-on-error and rapid-toggle-guard items rest on their unit tests (mapped in the manual-testing results).

Decisions

Full decision log

Decisions — Layer B: bookmarking → React Query (#2014)

1. The mutation writes the model store, not a query cache. The issue said
   "optimistically patching bookmarked/bookmarkedUpdateState on the units
   cache" — but every reader (Unit → UnitTitleSlot, and UnitButton via
   useModel since the peel below) reads the model store, which stays the
   merged source of truth for units until Dissolve the model-store normalized cache #1977. So the mutation dispatches
   updateModel, exactly what useCheckBlockCompletion's onSuccess does for
   complete — no setQueryData on the sequence query. Durability: the model
   bridge (bridgeToModelStore) runs only on a real fetch onSuccess, 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.

2. One mutation, not two. addBookmark/removeBookmark were byte-for-byte
   mirror images differing only in the target boolean and the api call, and
   BookmarkButton already computed the direction for its conditional dispatch.
   useSetBookmarked puts the direction in the mutation variables; a
   thunk-mirroring useAddBookmark/useRemoveBookmark pair was rejected as
   pure duplication.

3. Naming: useSetBookmarked, and the toggle lives in the component. The
   hook doesn't read the current state — the caller supplies the target value —
   so "toggle" would promise semantics its signature doesn't deliver; it sets
   bookmarked to X. The component's named handler toggleBookmark
   (() => setBookmarked(unitId, !isBookmarked)) is where toggle semantics
   genuinely exist, and it keeps the onClick={toggleBookmark} JSX line
   byte-identical to master. A first-pass useToggleBookmark name (and an
   inline-arrow handler, then a mutateBookmark binding) were rejected in
   review in favor of this split.

4. onMutate/onSuccess/onError map 1:1 onto the thunks' dispatches —
   optimistic loading flip, loaded confirm, failed revert + logError.
   Same values, same order. The explicit onError: logError follows the
   useCheckBlockCompletion precedent: the global QueryCache.onError (Restore dropped query error logging via a global QueryCache.onError #2022)
   covers queries only, not mutations.

5. The hook returns a callback, not the mutation object (the
   useCheckBlockCompletion shape). No component needs isPending:
   isProcessing keeps deriving from bookmarkedUpdateState === 'loading' in
   UnitTitleSlot, preserving the prop contract of the
   org.openedx.frontend.learning.unit_title.v1 plugin slot.

6. BookmarkButton's useCallback (and its eslint-disable react-hooks/exhaustive-deps) is gone. The handler's only changing input is
   isBookmarked, so memoization buys nothing (StatefulButton isn't
   memoized); toggleBookmark is a plain named function and no disable comment
   survives the rewrite (per the no-eslint-disable convention).

7. Tests. The four redux.test.js cases port assertion-for-assertion onto
   the courseware/data/apiHooks.test.tsx mutation pattern (same URLs, same
   objectContaining model checks, same logError expectations), plus one new
   case the thunk shape couldn't express cleanly: the optimistic loading
   write lands before the request resolves (hanging mock). BookmarkButton.test.jsx
   needed no changes — it renders through the real provider stack and keeps its
   axios mocks (the mutation calls the same api.js functions).

   Where each deleted redux.test.js case went (all in
   bookmark/data/apiHooks.test.tsx; the driver changed from
   executeThunk(thunks.…) to renderHook + act + waitFor, the predicates
   did not):

   Deleted case	What it pinned	Now covered by
   addBookmark › "Should create bookmark and update model state"	units model → { bookmarked: true, bookmarkedUpdateState: 'loaded' }	"creates the bookmark and updates the model state" — same model check, plus the POST url and { usage_id } body (body was previously pinned only in BookmarkButton.test.jsx, where it still is)
   addBookmark › "Should fail to create bookmark in case of error"	logError called; POST url; model reverted → { bookmarked: false, … 'failed' }	"logs the error and reverts the flag when creating the bookmark fails" — same three assertions
   removeBookmark › "Should delete bookmark and update model state"	units model → { bookmarked: false, … 'loaded' }	"deletes the bookmark and updates the model state" — same model check, plus the DELETE url ({username},{unitId})
   removeBookmark › "Should fail to remove bookmark in case of error"	logError called; DELETE url; model reverted → { bookmarked: true, … 'failed' }	"logs the error and reverts the flag when deleting the bookmark fails" — same three assertions
   — (no old counterpart)	—	"flips the flag optimistically before the request resolves" — new: { bookmarked: true, … 'loading' } lands mid-flight, then 'loaded' after the hanging mock resolves

8. No behavior changes intended — same optimistic flip timing, same
   revert-on-error, same logError, same three model writes per toggle,
   fire-and-forget from the button either way.

Manual testing

Manual testing — Layer B: bookmarking → React Query (#2014)

In-browser verification for the bookmark-conversion layer, run against a live
backend (tutor local). This layer claims zero user-facing change: the
addBookmark/removeBookmark thunks become one useToggleBookmark mutation
with the same optimistic model writes (loading flip → loaded confirm, or
failed revert + logError), and every reader keeps reading the units model.
The things to watch are exactly the old semantics: the optimistic flip, the
disabled-while-processing button, and the revert on error.

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 has the
"Bookmark this page" button under the unit title. The requests show in
DevTools → Network filtered on bookmarks (POST to
/api/bookmarks/v1/bookmarks/ on add; DELETE to
…/bookmarks/{username},{unitId}/ on remove).

Verify by hand

• Add bookmark — click "Bookmark this page": the button flips to
  "Bookmarked" (filled icon) immediately (optimistic, before the response),
  is disabled while the POST is in flight, and the POST body carries
  { "usage_id": "<unitId>" }.
• Bookmark dot reaches the sequence nav — after adding, the unit's
  sequence-nav button shows the bookmark dot (and in the narrow-viewport
  dropdown) without any refetch.
• Remove bookmark — click "Bookmarked": flips back to "Bookmark this
  page" optimistically; the DELETE URL contains {username},{unitId}.
• State survives a reload — hard-reload the unit page: the bookmark
  state matches the server (comes from the sequence-metadata fetch, not the
  mutation).
• Revert on error — DevTools → Network → Offline, then toggle: the flag
  flips optimistically, the request fails, the flag flips back
  (revert), the button re-enables, and a page-action logError appears in
  the console. Back online: toggling works again.
• Rapid-toggle guard unchanged — while a toggle is in flight the button
  is disabled (isProcessing from bookmarkedUpdateState === 'loading'),
  so no second request fires.

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

• The four ported thunk cases (add/remove × success/network-error: model writes,
  request shapes, logError + revert on failure) — bookmark/data/apiHooks.test.tsx.
• The new optimistic-semantics case: bookmarkedUpdateState === 'loading' lands
  before the request resolves (hanging mock) — apiHooks.test.tsx.
• BookmarkButton.test.jsx — button states and store writes through the real
  render path, unchanged assertions.

Results

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

The four checked items passed as described; nothing surprising observed. The
revert-on-error and rapid-toggle-guard items were not run by hand; both rest on
the automated suite — the two network-error cases in apiHooks.test.tsx pin the
logError + revert writes, its hanging-mock case pins the mid-flight loading
state that drives the disabled button, and BookmarkButton.test.jsx's
"does not handle adding bookmark when processing" case pins that a disabled
button fires no request.

🤖 Generated with Claude Code

[codecov]

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2066      +/-   ##
==========================================
- Coverage   93.71%   93.71%   -0.01%     
==========================================
  Files         369      369              
  Lines        6017     6016       -1     
  Branches     1429     1428       -1     
==========================================
- Hits         5639     5638       -1     
  Misses        361      361              
  Partials       17       17              

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

👍🏼