Skip to content

Read units and sequences from the courseware queries, not useModel #2088

Description

@brian-smith-tcril

Part of #1946 — Redux → React Query migration (Stage 1). Part of the #1977 model-store dissolution (plan) — Layers D1 + D2. One or two PRs — the two model types come from the same query (useSequenceMetadata) and share readers, so whether they land together is a call for implementation time.

Goal: read the units and sequences models from the courseware queries that already produce them, and move their three optimistic writes onto those queries.

Context. units comes only from useSequenceMetadata (plus two mutation writes). sequences is a merge of two queries: the outline query writes { id, title, sectionId } for every sequence, and useSequenceMetadata writes the full normalizeSequenceMetadata shape (unitIds, activeUnitIndex, gatedContent, isTimeLimited, …) for the active one — so useModel('sequences', id) returns a different shape depending on which sequence it is, and each reader converts to whichever query owns the fields it reads.

Tasks — D1, units (6 sites)

  • courseware/course/sequence/Sequence.jsx:48, SequenceContent.jsx:48, Unit/index.jsx:33, Unit/UnitSuspense.jsx:21, Unit/hooks/useShouldDisplayHonorCode.js:12, sequence-navigation/UnitButton.tsx:37 → the units array of useSequenceMetadata(sequenceId).
  • Move the bookmarked write (courseware/course/bookmark/data/apiHooks.ts:17) and the complete write (useCheckBlockCompletion, courseware/data/apiHooks.ts:144) onto that query with setQueryData, including the mutation's store.getState() "already complete, don't re-check" guard (apiHooks.ts:163-166).

Tasks — D2, sequences (11 sites)

  • Active-sequence readers → useSequenceMetadata(sequenceId): Sequence.jsx:46, SequenceContent.jsx:22, sequence-navigation/SequenceNavigation.jsx:27, sequence-navigation/hooks.js:12, Unit/hooks/useIFrameBehavior.ts:40, alerts/sequence-alerts/hooks.js:8,23, Course.jsx:35.
  • Title-only readers → the outline query: breadcrumbs/CourseBreadcrumbs.jsx:29, sequence-navigation/UnitNavigationEffortEstimate.jsx:35,36.
  • Move useSaveSequencePosition's optimistic activeUnitIndex write and its store.getState() rollback read (courseware/data/apiHooks.ts:180-203) onto the sequence query.
  • Restructure CourseBreadcrumbs' useModels('sequences', …) call, which currently runs inside .map() (see below).

Verify: unit navigation, bookmarking, completion ticks, the honor-code gate, sequence gating/timed-exam banners and breadcrumbs all behave as before; saving a sequence position still rolls back on failure; git grep "useModel('units'\|useModel('sequences'\|modelKeys.units" src is empty.

Note

This issue was authored by Claude (Claude Code) and reviewed before posting.

Findings that shape the task list

  • CourseBreadcrumbs calls hooks in a loop. useModels('sections', course.sectionIds)?.map(section => ({ …, sequences: useModels('sequences', section.sequenceIds) })) (CourseBreadcrumbs.jsx:24-29) calls useModels once per section inside the map — it only survives because the array identity is stable per render. The conversion has to read the outline query once and index into its sections/sequences maps. Fixed here because the conversion forces it, not as a general cleanup.
  • UnitNavigationEffortEstimate's effort branch is unreachable, and stays that way. It bails unless nextSequence.effortActivities || nextSequence.effortTime, and neither writer of the sequences model produces those fields (they come from course-home/data/api.js, the outline tab's blocks). That is intentional: the file's header comment records that effort estimation was lost when the LMS blocks API stopped being called, with AA-930 tracking its revival. Convert it verbatim, including the Object.keys(sequence).length === 0 guards, which depend on useModel returning {}.
  • The three writers hold the last useDispatch calls in courseware/data besides useSaveIntegritySignature (layer D3/D4), and two of them also read Redux state directly via useStore() — the completion guard and the position rollback both need a cache read to replace store.getState().

Note

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

Plan summary. Two stacked layers on top of #2123: A converts the six units readers onto a useUnit(sequenceId, unitId) hook (a disabled select observer of the sequence query, the #2087 shape) and moves the bookmark and completion writes onto setQueryData at the exact sequence key; B converts the eleven sequences readers (active-sequence readers to the sequence query, title-only readers to the outline query), restructures CourseBreadcrumbs' hook-in-a-loop, moves the position write, and pulls the three sequences store selectors forward from D4 because the model-store bridge only runs on a fetch, so a setQueryData write never reaches the store copy redirects.ts reads activeUnitIndex from. Review outcomes: SequenceContent's !unit guard wakes (it was #98's handling of a deleted unit, dead since #808 made useModel return {} for a missing id); writers address the exact key through one shared helper, with the sidebar's completion call corrected to the active sequence; the under-gate refetching was peeled as #2123.

Full plan

Plan: #2088 — read units and sequences from the courseware queries

Layers D1 + D2 of the #1977 model-store dissolution, as two stacked layers
on top of #2123 (PR #2124)
, which was peeled out of this plan:

Reviewed 2026-09-25; the four open questions and their outcomes are at the
end. Entry numbers become the decisions-2088A.md / decisions-2088B.md
numbering as each layer lands.

How it works today

  1. One query produces both models. useSequenceMetadata(sequenceId)
    (courseware/data/apiHooks.ts:74-97) is keyed
    coursewareQueryKeys.sequence(sequenceId, isPreview), where isPreview is
    useLocation().pathname.startsWith('/preview') read inside the hook, and
    resolves to { sequence, units } from normalizeSequenceMetadata
    (courseware/data/utils.js:108-148). Its meta.models mirrors sequence
    into sequences[id] (updateModel, a merge) and units into units[id]
    (updateModels). The outline query also writes sequences[id] with
    { id, title, sectionId } for every released sequence (utils.js:20-55),
    so the sequences model is a merge: outline fields for all, the full
    sequence shape for the fetched one.

  2. Readers. units: Sequence.jsx:49, SequenceContent.jsx:48,
    UnitButton.tsx:37 (literal), Unit/index.jsx:33, UnitSuspense.jsx:21,
    useShouldDisplayHonorCode.js:12 (via modelKeys.units). sequences:
    Sequence.jsx:47, SequenceContent.jsx:22, SequenceNavigation.jsx:27,
    sequence-navigation/hooks.js:11, useIFrameBehavior.ts:40,
    sequence-alerts/hooks.js:8,23, Course.jsx:37,
    CourseBreadcrumbs.jsx:29 (a useModels inside .map()),
    UnitNavigationEffortEstimate.jsx:35,36. useModel returns {} for a
    missing entry (model-store/hooks.js:6-11); readers destructure off it or
    read .title directly, and SequenceContent's !unitId || !unit guard
    (:49) is dead on the !unit side because {} is truthy.

  3. Writers. Three, all dispatching updateModel and two reading the store
    directly: useSetBookmarked (bookmark/data/apiHooks.ts:17, keyed by unit
    id, no sequence id in its signature), useCheckBlockCompletion
    (courseware/data/apiHooks.ts:173 write; :192 guard reads
    store.getState().models.units[unitId].complete), useSaveSequencePosition
    (:210 write; :217 rollback pre-read of activeUnitIndex). These are the
    last useDispatch / useStore calls in courseware/data besides
    useSaveIntegritySignature (D3).

  4. The bridge cannot see a cache write. bridgeToModelStore is the app
    QueryCache's onSuccess, and in the installed query-core (5.102.8) that
    callback runs only from Query.fetch's success path
    (node_modules/@tanstack/query-core/build/modern/query.js:252);
    setQueryData goes through query.setData and never reaches it. So the
    moment a writer moves onto the cache, the store's copy of that field stops
    tracking it. That matters for sequences.activeUnitIndex: three D4
    selectors still read sequences[id] from the store —
    CoursewareContainer.tsx:43,53 (saveUnitPosition, unitIds, sectionId,
    id) and redirects.ts:257 — and sequenceToSequenceUnitRedirect
    (redirects.ts:180) uses sequence.activeUnitIndex to fill in the unit
    for a /course/:courseId/:sequenceId URL. Navigating units within a
    sequence keeps every observer mounted (same key, no refetch), so after the
    position write moves to the cache that redirect would read the position
    fetched at load, not the one just saved. No store selector reads units.

  5. The sidebar's completion call is not keyed by the unit's own sequence.
    SidebarSequence.jsx:75-78 renders each SidebarUnit with sequenceId
    = that sidebar sequence's id and activeUnitId = the unit being left;
    UnitLinkWrapper.tsx:39 forwards both and useCourseOutlineSidebar
    calls checkBlockCompletion(courseId, sequenceId, activeUnitId)
    (course-outline/hooks.js:80). Clicking a unit in another sequence
    therefore checks the active unit against the target sequence's handler.
    The store write is keyed by unit id alone, so it lands regardless; a cache
    write keyed by the sequence id the mutation receives would miss.

  6. Fetch ownership of the sequence query. CoursewareContainer.tsx:34
    and useCoursewareRedirects (redirects.ts:251) observe it at mount; five
    more fetching observers sit under the gate — Sequence.jsx:50,
    SequenceNavigation.jsx:37, CourseBreadcrumbs.jsx:21,
    sequence-alerts/hooks.js:9,24, sequence-navigation/hooks.js:14.
    useSequenceMetadata takes no { enabled } option and sets no
    staleTime, so any observer that mounts after the data settled refetches
    (refetchOnMount + stale), the pattern Stop the courseware gate queries refetching from components under the gate #2098 removed for the three course
    queries. No layer has recorded the per-load count of
    /api/courseware/sequence/{id} yet; this one has to, because every new
    reader that observes the query fetching would add to it.

  7. Where a reader can find its sequence. Every units reader renders
    under Sequence for the route's :sequenceId: Sequence →
    SequenceContent (has sequenceId) → Unit → UnitSuspense /
    useShouldDisplayHonorCode (have only id, courseId); UnitButton
    already reads useParams().sequenceId (UnitButton.tsx:38) for its link.
    Units also carry sequenceId in the normalized shape. useIFrameBehavior
    and sequence-navigation/hooks.js read the route the same way.

  8. Tests. initializeTestStore seeds units / sequences into the store
    (setupTest.js:184-189) and render builds a fresh query client per call
    with no way to pass one in, so today's reader suites see model data without
    a query resolving. Suites that render a reader with no owner above it:
    UnitButton.test.jsx, SequenceContent.test.jsx, Unit/index.test.jsx,
    BookmarkButton.test.jsx (asserts on store.getState().models.units);
    UnitSuspense.test.jsx and useShouldDisplayHonorCode.test.js mock
    useModel by key; useIFrameBehavior.test.js mocks useModel to return
    unitIds. Sequence.test.jsx, SequenceNavigation.test.jsx,
    UnitNavigation.test.jsx, CourseBreadcrumbs.test.jsx mount
    MountCourseQueryHooks (course queries only; the sequence fetch there is
    the component's own observer). The writer suites in apiHooks.test.tsx
    and bookmark/data/apiHooks.test.tsx seed with seedSequenceModels and
    assert on the store.

Peel — #2123, stop the sequence query refetching from components under the gate

Filed 2026-09-25 as #2123, a sub-issue of #1946. Its plan and proposed
decisions are in decisions-2123.md; the P1–P5 sketch that stood here moved
there. A and B stack on it and inherit the { enabled } option on
useSequenceMetadata and the sequenceId prop on MountCourseQueryHooks.

Layer A — units (Part of #2088)

A1. Reader shape: a useUnit(sequenceId, unitId) hook on the sequence
query, enabled: false, select to the unit.
Follows C's
useDiscussionTopic(courseId, unitId) (#2087, entries 1–2): an inline
select: ({ units }) => units.find(unit => unit.id === unitId), no
useCallback, undefined for a missing unit. The plan's D1 note ("no
per-model hooks") was written for the one-line .data ?? {} reads; six
copies of a find over the units array is what that note was avoiding in the
other direction, and C already set the shape for a list-to-item read.
Alternative: useSequenceMetadata(sequenceId, { enabled: false }).data?.units .find(...) at each site.

A2. Where the three id-only readers get sequenceId: useParams(), not
prop threading.
Unit/index.jsx, UnitSuspense.jsx and
useShouldDisplayHonorCode.js each add const { sequenceId } = useParams();,
the way UnitButton.tsx:38, useIFrameBehavior.ts:39 and
sequence-navigation/hooks.js:9 already locate the route's sequence.
Threading it as a prop touches SequenceContent → Unit → UnitSuspense
and the hook's argument object, plus propTypes and three suites, for the same
value. Sequence.jsx and SequenceContent.jsx pass their own sequenceId.
Alternative: the hook reads the route itself (useUnit(unitId)), which hides
the coupling and leaves the two explicit ids unused.

A3. undefined where {} was; SequenceContent's !unit guard wakes
up. Settled (a).
With C's precedent every reader converts to an optional
read: unit?.title, { graded } = useUnit(...).data ?? {} where a
destructure stays, (unit || {}).id unchanged. SequenceContent's
!unitId || !unit branch becomes reachable again: a URL naming a unit that
is not in the sequence renders "There is no content here." instead of an
iframe the LMS then fails to load. Not a new behaviour — the history:

So (a) restores #98's handling of a deleted unit. The UnitButton fallback
(title = fallbackTitle for a unit with no entry) is preserved; the
TypeScript wrinkle there is that ?? {} against a typed SequenceUnit needs
a typed empty or per-field ??. Rejected (b): ?? {} at that one site,
which keeps a sentinel alive purely to preserve #808's accident.

A4. Types. SequenceUnit (and, for B, SequenceMetadata) declared in
courseware/data/apiHooks.ts beside CoursewareMeta / DiscussionTopic,
typed at the query. Unlike DiscussionTopic these need no index signature:
normalizeSequenceMetadata is ours and picks exactly these fields.
bookmarkedUpdateState?: 'loading' | 'loaded' | 'failed' is on the unit type
as the one field the server never sends and the bookmark writer adds.

A5. Writers address the exact cache entry. Settled. Each writer builds
the full key coursewareQueryKeys.sequence(sequenceId, isPreview) and calls
setQueryData on it, patching the matching element of units; the
completion guard reads getQueryData on the same key and looks the unit up.
What that entails:

  • Key construction in one place. useSequenceMetadata already derives
    isPreview from the route (apiHooks.ts:75); that derivation moves into
    one helper (a useSequenceQueryKey(sequenceId) or similar in
    courseware/data/apiHooks.ts) shared by the query and all three writers.
    No writer reads the route on its own.
  • useSetBookmarked gains a sequence id. BookmarkButton gets it from
    unit.sequenceId, which every unit carries from the normalizer
    (utils.js:139).
  • The sidebar call changes. checkBlockCompletion(courseId, sequenceId, activeUnitId) in course-outline/hooks.js:80 passes the target
    sequence with the active unit (mechanism 5); it has been that way since
    the sidebar was created in [FC-0056] Course outline sidebar #1375 (2024-05, FC-0056), where sequenceId was
    the prop in scope, mirroring CoursewareContainer's route-based call. It
    becomes checkBlockCompletion(courseId, activeSequenceId, activeUnitId)
    (the hook already has activeSequenceId from useParams). That also
    changes the request on a cross-sequence click: the get_completion
    handler is addressed on the sequence xblock in the URL, so it now goes to
    the sequence that contains the unit. A same-sequence click is unchanged.
    Recorded as a change in the decisions entry; the manual test covers a
    cross-sequence sidebar click explicitly.

Why the flat store made this invisible: #32 (2020-03, ADR 0004) normalized
units by id following the Redux FAQ, and because two APIs (course blocks and
sequence metadata) then wrote the same unit; a unit-keyed write landed
regardless of sequence. The blocks API call is gone and the sequence query is
the only writer, so every unit has exactly one entry.

Rejected: address by unit id across every cached sequence entry
(setQueriesData over a 'sequence' prefix, updater returning undefined
for entries without the unit). It reproduces the store's lookup and needs
none of the three edits above, but it writes by scanning entries the cache
keys by request, and its only real justification was keeping the sidebar's
mis-keyed call out of the diff.

A6. useUnit is non-fetching (enabled: false), on the { enabled }
option the peel adds. Owner: CoursewareContainer / useCoursewareRedirects.
A adds no fetching observer; the request count is the peel's.

A7. Bridge: the units mirror leaves useSequenceMetadata's meta in A;
modelKeys.units goes from Unit/constants.ts
(coursewareMeta stays
for D3). seedSequenceModels keeps seeding units into the store until F,
harmlessly.

A8. Tests. useUnit describe in apiHooks.test.tsx: fetches nothing on
its own
, returns the unit the owner loaded, returns undefined for a unit
not in the sequence
(the #2087 entry-4 shape). Writer suites assert on the
cache instead of the store, seeded through the real query
(queryClient.fetchQuery(<sequence query options>) against the existing
axios route — needs a queryOptions export like discussionTopicsQuery)
rather than a hand-written shape (#2098, entry 3). UnitButton,
SequenceContent, Unit/index and BookmarkButton suites render the
peel's sequence owner (P2) beside the component, under a route that carries
:sequenceId. UnitSuspense and
useShouldDisplayHonorCode suites mock useUnit (jest.fn(() => ({ data })))
and keep their useModel mock for coursewareMeta until D3. The
sidebar hook's suite pins that checkBlockCompletion is called with the
active sequence id (A5).

A9. Commit: refactor!: with a BREAKING CHANGE: footer. For plugins:
useModel('units', unitId) returns {} after this layer; the replacement is
useUnit(sequenceId, unitId).data, undefined for a unit not in the
sequence. UnitTitleSlot's unit prop keeps its shape.

A10. Manual test. Bookmark toggle (button state through
loading/loaded/failed, the nav icon), completion ticks on next-unit
navigation and from the sidebar including a cross-sequence click, the
honor-code gate, the content-type-gating message; sequence request
count unchanged from the peel's after-count.

Layer B — sequences (Closes #2088)

B1. Active-sequence readers → the sequence query; title-only readers → the
outline query.
Per mechanism 1. Sequence.jsx, SequenceContent.jsx,
SequenceNavigation.jsx, sequence-navigation/hooks.js,
sequence-alerts/hooks.js (both), useIFrameBehavior.ts, Course.jsx read
useSequenceMetadata(sequenceId, { enabled: false }).data?.sequence — in the
five files that already hold a sequenceQuery for isSuccess, that is the
same variable. CourseBreadcrumbs.jsx and UnitNavigationEffortEstimate.jsx
read useCoursewareOutline(courseId, { enabled: false }).data?.sequences[id].
undefined for a missing entry, optional chains at the sites; the visible
difference is Course.jsx's Helmet title while loading (empty segments today
from {}.title, none after).

B2. CourseBreadcrumbs: read the outline once, index into it. Replaces
the useModels('sequences', …) inside .map() with
section.sequenceIds.map(id => sequences[id]) over the outline's sequences
map. The coursewareMeta and useModels('sections', …) reads stay for D3;
the sections read is at top level, so no hook-in-loop remains.

B3. UnitNavigationEffortEstimate: verbatim, minus the Object.keys
guards.
The effort branch stays unreachable (AA-930; #1977 decision D4).
With a undefined-or-object read the !sequence || !nextSequence half of
the guard is the whole guard; the Object.keys(x).length === 0 halves
existed for useModel's {} and are dead — drop them. They are #808's own
workaround for the normalization that killed A3's guard (its author: "This
code was counting on a bug in useModel that returned undefined if the model
existed, but the ID didn't"; the reviewer who wrote the original guard
suggested restructuring them away and withdrew it as not worth it), so once
{} stops being the missing-value sentinel there is nothing left for them
to guard. The issue body's "including the Object.keys guards" was written
before that history was looked up.

B4. Pull the three sequences store selectors forward from D4. Because
of mechanism 4: CoursewareContainer.tsx:43 → sequenceQuery.data?.sequence ?? null; :53 (nextSequence, outline fields only) → the outline query's
sequences[nextSequenceId], which adds
useCoursewareOutline(courseId, { enabled: false }) to CoursewareContainer
(useCoursewareRedirects owns that fetch); redirects.ts:257 →
sequenceQuery.data?.sequence ?? null. D4 keeps the coursewareMeta ×2 and
sections ×2 selectors and deletes modelReader.ts. With no store reader of
sequences left, B drops the sequences mirror from both queries'
meta.models. getTestStoreIds reads models.sequences seeded directly by
seedCoursewareModels, unaffected.

B5. Peeled. The five under-gate observers are the peel's; B converts
their sequences reads on the sequenceQuery variable each already holds.

B6. useSaveSequencePosition writes the exact key. Settled. Same as
A5: setQueryData on useSequenceQueryKey(sequenceId) patching
sequence.activeUnitIndex, the rollback pre-read from getQueryData on the
same key. Drops the hook's useStore / useDispatch.

B7. Tests. Writer suite asserts on the cache (A8's seeding). Course and
useIFrameBehavior's consumers mount the peel's sequence owner where they
do not already; useIFrameBehavior.test.js mocks
useSequenceMetadata ({ data: { sequence: { unitIds, activeUnitIndex } } })
in place of its useModel mock; CoursewareContainer.test.jsx's
activeUnitIndex redirect cases (:249, :359) are the B4 regression
tests and should pass unchanged — plus one new case: a saved position is what
the /course/:courseId/:sequenceId redirect uses.

B8. Commit: refactor!: with a BREAKING CHANGE: footer. For plugins:
useModel('sequences', id) returns {}; the replacement is
useSequenceMetadata(sequenceId, { enabled: false }).data?.sequence for the
active sequence's full shape, or the outline query's sequences[id] for
title / section membership.

B9. Manual test. Prerequisite gating (ContentLock), hidden-after-due,
a timed/proctored sequence banner, the banner-text and entrance-exam alerts,
breadcrumbs and jump nav, previous/next across a sequence boundary, then the
B4 case: navigate to a later unit, open /course/{id}/{sequenceId} and land
on that unit; block the goto_position POST in devtools and see the position
roll back; sequence request count unchanged from the peel's after-count.

Review outcomes (2026-09-25)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions