Skip to content

Read discussion topics from the query, not useModel #2087

Closed
#2122
Closed
Read discussion topics from the query, not useModel#2087
#2122
Assignees
brian-smith-tcril

Description

@brian-smith-tcril
brian-smith-tcril
opened on Sep 21, 2026

Part of #1946 — Redux → React Query migration (Stage 1). Part of the #1977 model-store dissolution (plan) — Layer C. Independent — can slot anywhere in the stack.

Goal: read discussion topics from the query instead of the discussionTopics model, and drop its bridge entry.

Context. #2016 converted the fetch to prefetchDiscussionTopics(queryClient, courseId) but deliberately left the three readers on useModel, since read conversions are this issue's home. The query resolves to a list of topics filtered to those with a usageKey; the bridge writes them into the model store keyed by usageKey (idField: 'usageKey'), which is why the readers look them up by unit id.

Tasks

  • Convert the three useModel('discussionTopics', unitId) readers — widgets/discussions/DiscussionsSidebar.jsx:22, widgets/discussions/DiscussionsTrigger.jsx:21, courseware/course/sidebar/SidebarContextProvider.jsx:35 — to a lookup by usageKey against coursewareQueryKeys.discussionTopics(courseId).
  • Remove meta: { models: [{ modelType: 'discussionTopics', … }] } from prefetchDiscussionTopics.
  • If the bridge's idField pass-through (added in Convert getCourseDiscussionTopics to React Query #2016) has no other user left, note it for layer F rather than removing it here.

Verify: on an openedx-provider course the discussions trigger and sidebar still appear for units with a topic and stay hidden for units without one; legacy-provider courses unaffected; git grep discussionTopics src finds no model-store reference.

Note

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

Findings that shape the task list

  • The readers key by unit id, the query holds a list. The conversion is a find(topic => topic.usageKey === unitId) over the query result, not a direct index. Each topic also carries its own id (the discussion topic id) which the render gates check (topic?.id && topic?.enabledInContext) — keep both fields straight.
  • The data is prefetched, not observed. prefetchDiscussionTopics is fired once from SidebarContextProvider's widget-registry effect, so the readers must subscribe to the cache entry without re-fetching it themselves — the same non-fetching read layer A introduces for the cross-tab outline alerts.
  • Single source, so no merge to untangle — unlike sequences/coursewareMeta, this model has exactly one writer, which is why it is independent of the rest of the stack.

Plan

Note

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

Decisions settled in review, refining the task list above:

  • Three layers were peeled out first, and this one builds on them. Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111 (PR refactor!: clean up SidebarContextProvider for React Query and convert it to TypeScript #2113) converted the provider to TypeScript, gave the widget contract named types (SidebarWidget, SidebarWidgetContext, DiscussionTopic), replaced prefetchDiscussionTopics with discussionTopicsQuery(courseId) observed by a DiscussionsProvider, and declared unit: Partial<DiscussionTopic> as the honest type of today's {}-or-topic value. Make useSidebar() the only sidebar context read and test the sidebar suites under the real provider #2112 (PR refactor!: read the sidebar context through useSidebar() and test the sidebar suites under the real provider #2116) merged the provider and context into sidebar/SidebarContext.tsx, made useSidebar() the only read, and put the sidebar suites on the real SidebarProvider. Un-awaited waitFor and act calls in Course.test.jsx and Sequence.test.jsx let tests pass without asserting #2118 (PR test: await every waitFor and act in Course.test and Sequence.test #2120) awaited every waitFor in Course.test and Sequence.test. So the third reader in the task list is now SidebarContext.tsx:87, the "prefetched, not observed" note is now "observed once by the Provider", and the two widget files already read the context through useSidebar().
  • useDiscussionTopic(courseId, unitId), a non-fetching per-unit read beside the query in courseware/data/apiHooks.ts: useQuery({ ...discussionTopicsQuery(courseId), enabled: false, select: topics => topics.find(topic => topic.usageKey === unitId) }). enabled: false is hard-coded, not an option, per the useIsCourseLoaded reasoning in Stop the courseware gate queries refetching from components under the gate #2098: the fetch is the Provider's by design, and a caller with no Provider above it stays pending, which fails loudly. Every reader takes .data by property access.
  • unit?: DiscussionTopic, no sentinel. The hook returns the topic or undefined, and the SidebarWidgetContext declaration says so where a widget author's editor shows it, checked against discussionsIsAvailable's own typed parameter. Every in-repo read is an optional chain. A ?? NO_TOPIC sentinel in select would preserve useModel's exact {} for a JavaScript isAvailable written as unit.id, at the cost of a hand-made value that exists only for that contract. No comment on the declaration, per Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111's decision that the types carry none.
  • Inline select, not useCallback. The docs' concern with an inlined select is compute ("will run on every render"), and this one is a find over one course's unit topics. The reference stability the provider's useCallback dependency array needs comes from find returning an element of the cached array, unchanged while the data is unchanged and kept by structural sharing across a refetch, not from memoizing the selector. As the repo's first select, useCallback here would read as a rule to memoize every select; the rule is the docs' own, memoize when the work matters.
  • The three readers read the render's client, which changes how the suites seed. Today's seed runs the query on a bridged throwaway client so the bridge fills the store; after this layer that client is never rendered under. The two widget suites (DiscussionsTrigger.test, DiscussionsSidebar.test) fill a createTestQueryClient() the same way and nest a QueryClientProvider for it inside setupTest's render, the EnrollmentAlert.test.tsx / LoadedTabPage.test.jsx shape; a queryClient option on render was set aside as a shared-harness change two callers do not justify, and F is where render's options change anyway.
  • Course.test's sidebar cases need the course_topics route on the adapter the render uses, and that route goes into initializeTestStore. Today initializeTestStore registers the discussion config route from options.provider but no topics route, and its onAny fallback answers 200, {}, so the real DiscussionsProvider's query throws on .filter and errors in every one of those renders, tolerated only because the readers read the store the seed filled. initializeTestStore is the only code with a handle to that adapter, so it registers the topics route from its own unitBlocks with a new enabledInContext option (default true), beside the config route; test-utils.jsx's seedDiscussionTopics, its temporary adapter and its restore() go. The route is fixture data for the mocking half of that helper, which survives F's removal of its seeding half; the helper is not renamed or restructured here.
  • Three suites render the provider with no query client and mock useDiscussionTopic. SidebarContext.test.jsx, UpgradeTrigger.test.jsx and UpgradeWidgetContext.test.jsx already mock the provider's other query hook (useCourseHomeMeta) rather than supply a client (Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111's decision 14); the new hook gets the same treatment, jest.fn(() => ({ data: undefined })). Their useModel mocks stay for coursewareMeta until D3 (Read sections and coursewareMeta from the courseware queries, not useModel #2089), which is the moment to give them a client instead, once the provider's last useModel read is gone.
  • Request-count cases in both widget suites plus the hook-level test. The hook case (fetches nothing on its own, the useIsCourseLoaded shape) pins the enabled: false line; a case in each widget suite pins that the component goes through it, per component because each suite is the sole test of its component and the widgets are untyped .jsx. Each flushes before asserting and asserts the whole GET history as an ordered URL list, so a fetching reader's second config request is caught too, and the negative check (enabled: true makes it fail) is part of writing it. A composed count in Course.test needs an adapter handle the harness does not have; worth adding if F's test-infrastructure pass provides one.
  • The bridge entry goes; idField stays for F. discussionTopicsQuery drops its meta; nothing writes the discussionTopics model after this layer, and useModel('discussionTopics', unitId) returns {}. The bridge's idField pass-through then has no user, in production or in setupTest.js, and is noted for F rather than removed here, with its bridge test.
  • refactor!: with a BREAKING CHANGE: footer. useModel('discussionTopics', unitId) returns {}; unit in the isAvailable context is undefined (was {}) for a unit with no topic, so unit.id throws where unit?.id did not; the replacement is useDiscussionTopic(courseId, unitId).data, populated by DiscussionsProvider, and the sidebar README's Accessing Course Data names it in place of its useModel holdover line. No learner-visible change and no request-count change.
Full plan

Plan: #2087 — Read discussion topics from the query, not useModel (C)

The next layer of the running stack #2121, on top of #2116 (#2112,
useSidebar() everywhere). Below it: #2113 (#2111, the provider's React
Query cleanup + TypeScript conversion) and #2120 (#2118, the awaited
waitFors). All three were peeled out of this issue on 2026-09-24 and have
PRs out; this layer builds on that work. Layer C of the #1977 model-store
dissolution; the only discussionTopics layer. C is a wide layer: three
readers swapped lightly on ground the three layers below prepared. Nothing
later depends on it.

Scope is the issue body. It has no comments. Re-read 2026-09-24 against
bsmith/sidebar-context-hook (the tip of #2116) after the three layers
landed on the stack; what they now own is marked below. The issue body names
courseware/course/sidebar/SidebarContextProvider.jsx:35 as the third
reader; on the stack that read is sidebar/SidebarContext.tsx:87.

Context

How it works after #2111, #2112 and #2118

One query definition, one observer. courseware/data/apiHooks.ts exports
discussionTopicsQuery(courseId), a queryOptions object whose queryFn
fetches the discussion config, stops with [] for a non-openedx provider,
otherwise fetches the course topics and keeps those with a usageKey
(#2111, decision 8). It still carries the bridge entry:

meta: { models: [{ modelType: 'discussionTopics', strategy: 'updateModels', idField: 'usageKey' }] },

widgets/discussions/DiscussionsProvider.tsx observes it with
enabled: !!getConfig().DISCUSSIONS_MFE_BASE_URL && hasDiscussionTab(tabs),
mounted by the framework for every enabled widget inside the sidebar
context (#2111, decisions 4–5). One request per sidebar mount; no refetch
on metadata writes. The old prefetchDiscussionTopics /
discussionsPrefetch and the provider's prefetch effect are gone.

The query resolves to a list of DiscussionTopic ({ id, usageKey, enabledInContext, [key]: unknown }, typed in #2111 beside the query); the
bridge writes them into the store one model per topic keyed by usageKey
(idField), which is why the readers look a topic up by unit id while
each topic also carries its own id, the discussion topic id.

One module, sidebar/SidebarContext.tsx (#2112, decision 3): the
context object (module-private), SidebarProvider, useSidebar(), the
types. SidebarProvider takes widgets: SidebarWidget[] as a prop
(#2112, decision 4); Course.jsx passes getEnabledWidgets(). Every
consumer reads the context through useSidebar() (#2112, decision 1).

The three readers, all still useModel('discussionTopics', unitId):

  • widgets/discussions/DiscussionsSidebar.jsx:21 and
    DiscussionsTrigger.jsx:20 — render gate:
    if (!topic?.id || !topic?.enabledInContext) { return null; }. The
    sidebar takes unitId, courseId and shouldDisplayFullScreen from
    useSidebar(); the trigger takes only unitId.
  • courseware/course/sidebar/SidebarContext.tsx:87 — unit, handed to
    every widget's isAvailable as the unit field of SidebarWidgetContext,
    declared at SidebarContext.tsx:26 as unit: Partial<DiscussionTopic>
    (Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111, decision 7: the honest type of "a topic or {}", and the line
    this layer changes), and sitting in getAvailableWidgets' useCallback
    dependency array. The discussions widget's own check is
    discussionsIsAvailable = ({ unit }: SidebarWidgetContext) => !!(unit?.id && unit?.enabledInContext)
    (widgets/discussions/widgetConfig.ts:6).

useModel returns a referentially stable {} when the model is absent
(generic/model-store/hooks.js:6-11), so a unit with no topic reads {}
and every ?. check falls through to hidden.

How the readers learn the data arrived. The bridge dispatches
updateModels on the query's success; each reader's useSelector
re-renders; in the provider, unit changes, getAvailableWidgets gets a new
identity, useInitialSidebar recomputes and useSidebarSync opens the
discussions panel if the cascade says so.

Tests after the three layers.

What changes structurally

  1. The readers key by unit id; the query holds a list. The conversion is
    a lookup, topics.find(topic => topic.usageKey === unitId), not an index.
  2. The data is observed once, by the Provider. The readers need to
    subscribe to the same cache entry without fetching it themselves —
    otherwise three more observers mounting after the Provider's fetch settles
    would each refetch at staleTime: 0, the Read the dates and outline tab data from their queries, not useModel #2083 problem (decisions-2083.md
    entry 3).
  3. The provider gains a query hook call. Today its only query hook is
    useCourseHomeMeta(courseId, { enabled: false }), which three suites
    mock so they can render it with no client. After this layer it calls a
    second one, so those suites need the same treatment for it (Tests, and
    open question 5).

Simpler than the other layers: single writer, single query, no merge.

The change

1. useDiscussionTopic(courseId, unitId) — a non-fetching, per-unit read

Beside discussionTopicsQuery in courseware/data/apiHooks.ts:

// Reads the topic DiscussionsProvider loaded for the unit; never fetches.
export const useDiscussionTopic = (courseId: string, unitId: string) => useQuery({
  ...discussionTopicsQuery(courseId),
  enabled: false,
  select: (topics) => topics.find(topic => topic.usageKey === unitId),
});

Three choices in that, each with the docs behind it:

  • enabled: false, hard-coded — no { enabled } option. The Stop the courseware gate queries refetching from components under the gate #2098
    reasoning for useIsCourseLoaded (decisions-2098.md entry 4): no caller
    wants it on, because the fetch is the Provider's by design (Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111), and a
    caller with no Provider above it stays pending forever, which fails
    loudly.

    If the query has cached data, then the query will be initialized in the
    status === 'success' or isSuccess state. If the query does not have
    cached data, then the query will start in the status === 'pending' and
    fetchStatus === 'idle' state. The query will not automatically fetch on
    mount. The query will not automatically refetch in the background.
    — Disabling/Pausing Queries › When enabled is false

    Disabled observers still subscribe to the entry, so when the Provider's
    query fills it they re-render — the sync mechanism above, pinned by a test
    (below).

  • select does the unit lookup, so the reader gets one topic or
    undefined.

    The select option lets you extract a specific portion of cached data
    that your component should monitor. … A component using this hook will
    only re-render if the data subset actually changes — not when unrelated
    properties shift.
    — Render Optimizations › select

    find returns an element of the cached array, so the reference is stable
    across renders while the data is unchanged, and undefined is stable by
    nature — what getAvailableWidgets' dependency needs. The same page notes
    an inlined select re-runs every render; it is a find over a short list
    (open question 2).

  • Property access at every reader — useDiscussionTopic(courseId, unitId).data
    — the repo's single-value rule (decisions-2085.md entry 2).

DiscussionTopic already types topics (#2111), so select infers
DiscussionTopic | undefined. Nothing is added to the type.

Alternatives set aside: a list hook with find at each reader (three
copies of the lookup, and the readers learn a list shape they never knew);
queryClient.getQueryData in the readers (non-reactive — "Do not use this
function inside a component, because it won't receive updates",
QueryClient › getQueryData).

2. The three readers

  • DiscussionsSidebar.jsx: const topic = useDiscussionTopic(courseId, unitId).data;
    — courseId is already destructured from useSidebar().
  • DiscussionsTrigger.jsx: the useSidebar() destructure becomes
    { courseId, unitId }, then the same line.
  • SidebarContext.tsx:87: const unit = useDiscussionTopic(courseId, unitId).data;
    — no ?? {}, because unit sits in a useCallback dependency array
    (decisions-2083.md entry 8; the shape the file already uses for
    courseHomeMeta). undefined is stable; {} per render is not.

Each file swaps its useModel import for useDiscussionTopic from
@src/courseware/data/apiHooks (the two .jsx widgets have no other
useModel; SidebarContext.tsx keeps it for coursewareMeta, D3). The
widgets already read the context through useSidebar() (#2112), so the
context lines need nothing beyond the trigger's added courseId.

3. The declared type of unit

SidebarContext.tsx:26 declares unit: Partial<DiscussionTopic> (#2111).
With the hook, the value is the topic or undefined, so the line becomes:

unit?: DiscussionTopic;

No trailing comment: #2111 (decision 13) settled that the declarations
carry no comments, and the ? says what the old {} comment would have.
This is where the old "undefined or a stable {}?" question lives: as a
type an operator's editor shows, checked against discussionsIsAvailable's
own parameter (widgetConfig.ts, typed in #2111), rather than as a README
comment. Inside the repo every read is an optional chain, so
{}.id === undefined?.id and nothing else changes. #2111's decision 7
already states this is the line and the shape; open question 1 records the
sentinel alternative for the record.

4. The bridge entry goes

discussionTopicsQuery drops its meta. After this layer nothing writes
the discussionTopics model; useModel('discussionTopics', unitId) returns
{} everywhere — the plugin-observable change (Behaviour changes).

idField has no user left, in production or in setupTest.js. Per the
issue body it is noted for F, not removed here; the bridge test
updateModels keys models by idField instead of id in
data/modelStoreBridge.test.ts stays for the same reason.

5. README

Most of the README work moved to #2111 and #2112 (the contract sections
link the declarations; the Provider section replaced the prefetch section;
the docs name SidebarProvider and SidebarContext.tsx). What is left for
C:

  • sidebar/README.md:158, Accessing Course Data: the holdover line for
    the per-unit read becomes useDiscussionTopic(courseId, unitId)
    (@src/courseware/data/apiHooks) beside the two metadata hooks, stating
    what it returns and that it reads what DiscussionsProvider loaded (so a
    custom widget using it renders the topic only where that widget is
    enabled). useModel then appears nowhere in the file.
  • widgets/discussions/README.md:30: drop "The query is bridged into the
    discussionTopics model for the widget's useModel readers
    (transitional, Dissolve the model-store normalized cache #1977)."; README.md:32: "reads the discussionTopics
    model" → reads the current unit's topic through useDiscussionTopic.
  • Nothing in SidebarContext.tsx (§3) and nothing in ARCHITECTURE.md
    (its only discussionTopics mention is the Provider code sample, which
    does not change).

Wording is for code review.

6. What does not change

Tests

The seam every affected suite shares: the readers now read the client the
component renders under, not the store the seed's bridged client filled.
Today's seed (createTestQueryClient(store).query(discussionTopicsQuery(courseId)))
fills a throwaway client whose bridge writes the store; setupTest's
render then builds a fresh, empty client. After this layer that render
sees no topics. Two shapes, chosen per suite:

  • Mount the real tree and mock the endpoints. Where DiscussionsProvider
    is in the rendered tree, no seeding is needed: with the two discussion
    routes on the adapter the render uses, the observer fetches into the
    render's client, the production path.
  • Seed the render's client. Where a widget component renders alone
    (<SidebarProvider widgets={[]}> mounts no DiscussionsProvider):
    createTestQueryClient(), await queryClient.query(discussionTopicsQuery(courseId))
    against the suite's mocks, and a nested <QueryClientProvider client={queryClient}>
    inside setupTest's render — the EnrollmentAlert.test.tsx /
    LoadedTabPage.test.jsx shape (open question 3).

A third shape, new since the plan was written, for the suites with no
client: mock the hook, the way they already mock useCourseHomeMeta
(open question 5).

Per suite:

  • widgets/discussions/DiscussionsTrigger.test.jsx and
    DiscussionsSidebar.test.jsx: seed the render's client (the component
    renders alone). The initializeTestStore call stays — it seeds the units
    model whose ids buildTopicsFromUnits turns into topics, and the global
    store the render needs. The seed line drops its store argument
    (createTestQueryClient()), and the client it fills is the one the nested
    provider supplies. The four existing cases (shown for a unit with a topic,
    hidden for 'no-discussion' / 'has-no-discussion') stay as written. One
    new case in each, reads the topics without requesting them: after
    render, axiosMock.history.get holds exactly one request to the
    course_topics URL (the seed's). Negative check: enabled: true in the
    hook makes it read two.
  • courseware/course/test-utils.jsx and Course.test.jsx sidebar
    behavior    : the real tree — Course mounts SidebarProvider, which mounts
    DiscussionsProvider, whose observer fetches into the render's client.
    This is required, not a check: with the topics route missing, the
    onAny 200, {} reply makes the query error (Context), so the four
    cases that expect sidebar-DISCUSSIONS would fail under the converted
    readers. The route has to be on the adapter initializeTestStore creates
    (setupTest.js:212), which nothing outside that function can reach, so
    initializeTestStore registers course_topics itself, from the
    unitBlocks it already builds, honouring a new enabledInContext option
    (default true) through buildTopicsFromUnits — beside the config route
    it already registers from options.provider (open question 6).
    seedDiscussionTopics, its temporary adapter and its restore() are
    deleted; setupDiscussionSidebar passes enabledInContext through to
    initializeTestStore with provider and courseHomeMetadata. The eight
    cases' waitFors already wait for the sidebar state the fetch produces;
    the keeps the sidebar closed case's comment ("Discussions prefetch
    resolves") is reworded to name the Provider's query. The course_topics
    route is harmless for every other caller of initializeTestStore: the
    default provider is 'legacy', for which the queryFn never requests it.
  • courseware/data/apiHooks.test.tsx, the discussionTopicsQuery
    describe: its cases assert on the cache instead of the store —
    queryClient.getQueryData(coursewareQueryKeys.discussionTopics(courseId))
    is the filtered list, [] for the legacy provider (same one-request
    history assertion), undefined after the network error (logError still
    called). The bridged client and initializeStore leave the describe. New
    describe('courseware apiHooks — useDiscussionTopic'), on renderHook
    with the useIsCourseLoaded describe's makeWrapper shape:
    • returns the topic for its unit — seed, render for 'unit-1'.
    • returns undefined for a unit with no topic — same seed, 'unit-2'.
    • fetches nothing on its own — empty client; fetchStatus 'idle',
      data undefined, no requests — the useIsCourseLoaded case's shape.
    • sees the topics when the query resolves after it mounted — render
      first, then await queryClient.query(discussionTopicsQuery(courseId));
      waitFor data to be the topic. The sidebar-sync mechanism, pinned.
  • sidebar/SidebarContext.test.jsx, widgets/upgrade/src/UpgradeTrigger.test.jsx,
    UpgradeWidgetContext.test.jsx: no client, so the provider's new
    useQuery would throw "No QueryClient set". Each adds a mock of
    @src/courseware/data/apiHooks (spread jest.requireActual, as their
    useCourseHomeMeta mocks do) with useDiscussionTopic: jest.fn(() => ({ data: undefined })).
    Their useModel mock stays for coursewareMeta until D3 (Make useSidebar() the only sidebar context read and test the sidebar suites under the real provider #2112,
    decision 5 said both mocks go as the two reads convert; this is the first
    half). In SidebarContext.test.jsx nothing rendered reads unit (its
    stub widgets' isAvailable ignore the argument); the mock exists so the
    provider renders.
  • widgets/discussions/DiscussionsProvider.test.tsx (Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111): expected
    unchanged — it has a client, so the provider's disabled observer joins the
    topics key; does not load the topics when … asserts fetchStatus
    'idle' on that key, which a disabled observer also produces, and the
    request-count assertions do not see it. Verify rather than assume.
  • widgets/upgrade/src/UpgradePanel.test.jsx: renders through
    setupTest's render (a client), so the real hook runs idle with no
    request; its useModel mock stays for coursewareMeta. Expected
    unchanged.
  • data/modelStoreBridge.test.ts: unchanged (§4).
  • Expected unchanged, under a client, no reader of unit:
    Sidebar.test.jsx, SidebarTriggers.test.jsx, SidebarBase.test.jsx,
    LockPaywall.test.jsx, Sequence.test.jsx (its upgrade cases pass
    getEnabledWidgets(), so DiscussionsProvider mounts and requests the
    config — 'legacy' from initializeTestStore — as it does today),
    defaultWidgets.test.js, CourseOutlineTray.test.jsx,
    CourseOutlineTrigger.test.jsx, the sidebar hooks suites.

Behaviour changes

  • None a learner sees, and no request-count change. The trigger and
    panel appear for units with an enabled topic and stay hidden otherwise; the
    topics request is the Provider's alone.
  • Plugins: useModel('discussionTopics', unitId) returns {} — the
    breaking change, the B3 shape. A widget component reads
    useDiscussionTopic(courseId, unitId).data instead; the sidebar README
    says so. SidebarWidgetContext.unit is undefined (was {}) for a unit
    with no topic — declared in the type, so a TypeScript widget sees it at
    compile time; unit?.id, the built-in widget's own check, is unaffected;
    unit.id throws. Named in the footer.

Commit type

refactor!: with a BREAKING CHANGE: footer: what stopped working
(useModel('discussionTopics', unitId) returns {}; unit in the
isAvailable context is undefined for a unit without a topic), what to
call instead (useDiscussionTopic(courseId, unitId).data from
./src/courseware/data/apiHooks, populated by DiscussionsProvider), and
where the README is. One commit: the hook, the readers, the type line and
the bridge entry are one change.

Not in scope

Open questions

Question 1 was settled in the review that peeled out the three layers;
2–4 were still open when it started; 5–6 are new, from re-reading the plan
against the stack.

  1. unit?: DiscussionTopic, or keep Partial<DiscussionTopic> with a
    stable {} sentinel in select?     Settled 2026-09-24, in the review
    that peeled out Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111: unit?: DiscussionTopic     (§3). Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111's issue
    body and decision 7 record it as the line and the shape this layer
    produces; it is what select returns, it is the shape the
    provider uses for courseHomeMeta, every in-repo read is an optional
    chain, and the type now says it where a widget author will see it. The
    alternative, select: topics => topics.find(...) ?? NO_TOPIC, preserves
    useModel's exact value for a JavaScript isAvailable written as
    unit.id, at the cost of a hand-made sentinel that exists only for that
    contract.
  2. Inline select or useCallback? Settled 2026-09-24: inline.
    The docs' concern is compute ("an inlined select function … will run on
    every render"), and this one is a find over one course's unit topics.
    The stability the provider's dependency array needs comes from find
    returning an element of the cached array (the same reference while the
    data is unchanged, and structural sharing keeps unchanged topics at their
    old references across a refetch), not from memoizing the selector.
    useCallback would be the only memoized shape, since the selector closes
    over unitId, and as the repo's first select it would read as a rule
    to memoize every select; the rule is the docs' own — memoize a select
    when it does enough work to matter.
  3. Nested QueryClientProvider or a queryClient option on setupTest's
    render, for the two widget suites?     Settled 2026-09-24: nested
    provider    , the established shape — EnrollmentAlert.test.tsx and
    LoadedTabPage.test.jsx render a component through setupTest's
    render with a pre-filled client nested inside the ui, and React
    resolves the nearest provider. The change stays inside the two suites;
    a render option is a shared-harness change that two callers do not
    justify, and F is where render's options change anyway (it loses
    store), so that is the moment to decide them as a set. Fewer suites
    need a seeded client than before Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111, since the Course suite now
    mounts the real Provider and mocks endpoints instead.
  4. Where the request-count case lives. Settled 2026-09-24: both
    widget suites     plus the hook-level fetches nothing on its own. The
    hook case pins the enabled: false line; the widget cases pin that each
    component goes through it, per component rather than one representative
    because each suite is the sole test of its component, and because the
    widgets are untyped .jsx where a later rewire to a fetching hook would
    pass every other test. The composed homes were weighed and set aside:
    Course.test's adapter is created inside initializeTestStore and not
    returned, so a count there needs a harness handle (F's test-infra pass,
    if wanted; a composed count would be worth adding then), and
    DiscussionsProvider.test.tsx would need the real widget config,
    SidebarTriggers and Sidebar in its tree, changing its subject. The
    provider's own read has no request count; it uses the same hook and is
    typed against the contract. Each widget case flushes before asserting
    and asserts the whole GET history as an ordered URL list (the seed's
    config then topics), so a fetching reader's second config request is
    caught too; the negative check (enabled: true → the case fails) is
    part of writing it, given Make the courseware redirect-rule tests actually assert: un-awaited waitFor and unwired mocks from #1501 #2078, Make the useIsCourseLoaded tests actually assert: replaced mocks and unsettled checks from #2071 #2100 and Make the LoadedTabPage streak test actually assert: an unconditional mock from #354 and a fixture inert since AA-1018 #2107.
  5. The three no-client suites: mock useDiscussionTopic, or give them a
    QueryClientProvider?     Settled 2026-09-24: mock the hook. It is the
    shape those suites already use for the provider's other query hook
    (useCourseHomeMeta), and Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111's decision 14 chose mocking over the
    wrapper for SidebarContext.test.jsx on the grounds that nothing it
    renders should touch a query client. A client instead would be one more
    wrapper in three suites for a hook that returns undefined in all of
    them, and would leave the useCourseHomeMeta mock as the odd one out.
    The client is the right move if these suites are to stop faking the
    provider's data layer altogether, and the time for that is D3, when the
    last useModel read leaves the provider and its mocks have to go anyway;
    the useDiscussionTopic mock added here is one of the lines D3 deletes.
  6. Where the course_topics mock route lives for Course.test.
    Settled 2026-09-24: in initializeTestStore, beside the config route it
    already registers from options.provider, built from its own unitBlocks
    with a new enabledInContext option — the only place with a handle to
    the adapter the render uses. The alternative is a second adapter in
    test-utils.jsx constructed with onNoMatch: 'passthrough', which
    forwards unmatched requests to the adapter beneath it; it works, but it
    keeps the seed helper alive and depends on axios-mock-adapter's stacking
    semantics, which nothing else in the suite relies on. F (Dissolve the model-store normalized cache #1977) later
    deletes initializeTestStore's seeding half and renames what is left;
    the route belongs to the mocking half that survives, beside the config
    route options.provider already registers, so it rides along. This
    layer does not rename or restructure the helper; that is F's.

Verification

  • nvm use && npm run types, npm run lint, full suite (output to a file).
  • Negative checks: enabled: true in useDiscussionTopic → the two
    without requesting them cases and fetches nothing on its own fail; the
    bridge entry restored → nothing fails (the readers no longer look at the
    store), which is expected and is why git grep is part of the check;
    unit removed from the provider's context literal → npm run types fails
    (the Clean up SidebarContextProvider for React Query and convert it to TypeScript #2111 check, still holding).
  • git grep "useModel('discussionTopics'" src empty; git grep discussionTopics src finds only queryKeys.ts, apiHooks.ts (the query
    and the hook), DiscussionsProvider.tsx, the Provider code samples in
    sidebar/README.md and ARCHITECTURE.md, the factory, the new route in
    setupTest.js, and tests.
  • Manual (tutor dev, a course with the "Open edX" discussions provider), the
    Stop the courseware gate queries refetching from components under the gate #2098 protocol — hard reload a unit, wait for the Network tab to go idle:
    • v1/courses/{courseId} then v2/course_topics/{courseId}: 1 each.
    • A unit with an in-context topic: the discussions trigger shows; opening
      it loads {DISCUSSIONS_MFE_BASE_URL}/{courseId}/category/{unitId}?inContextSidebar.
    • A unit with in-context discussions turned off in Authoring: trigger and
      panel hidden; navigating between the two units flips them without a new
      topics request.
    • Stored preference DISCUSSIONS in localStorage: the panel auto-opens
      once the query resolves (the sync path).
    • Legacy-provider course: no course_topics request, no trigger; if no
      local legacy course exists, the query-level case carries it — note in
      the manual-testing doc.
    • A widget in env.config.jsx still reading useModel('discussionTopics')
      gets {} — the breaking change is real and documented.

Stack

One layer on top of bsmith/sidebar-context-hook (#2116, the tip of stack
#2121), added with gh stack add; closes #2087. D1/D2 (#2088) follows.

Activity

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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions