| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #2068 +/- ##
==========================================
+ Coverage 93.72% 93.74% +0.01%
==========================================
Files 369 368 -1
Lines 6028 6029 +1
Branches 1428 1391 -37
==========================================
+ Hits 5650 5652 +2
Misses 361 361
+ Partials 17 16 -1 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Pre-approved with one minor suggestion.
Sorry, something went wrong.
| export const prefetchDiscussionTopics = (queryClient: QueryClient, courseId: string) => ( | ||
| queryClient.prefetchQuery({ | ||
| queryKey: coursewareQueryKeys.discussionTopics(courseId), | ||
| queryFn: async () => { | ||
| const config: { provider: string } = await getCourseDiscussionConfig(courseId); | ||
| // Only load topics for the openedx provider, the legacy provider uses | ||
| // the xblock | ||
| if (config.provider !== 'openedx') { | ||
| return []; | ||
| } | ||
| const topics: { usageKey: string | null }[] = await getCourseTopics(courseId); | ||
| return topics.filter(topic => topic.usageKey); | ||
| }, | ||
| meta: { models: [{ modelType: 'discussionTopics', strategy: 'updateModels', idField: 'usageKey' }] }, | ||
| }) |
There was a problem hiding this comment.
It turns out prefetchQuery is now @deprecated in the version of query-core we're installing. The deprecation message suggests using queryClient.query({ ... }).catch(() => {}).
Sorry, something went wrong.
There was a problem hiding this comment.
used an imported noop as the examples in https://tanstack.com/query/latest/docs/framework/react/guides/prefetching do, but went with () => {} for the README example to avoid recommending extra imports.
Sorry, something went wrong.
The last courseware thunk becomes prefetchDiscussionTopics in courseware/data/apiHooks.ts — a plain prefetchQuery wrapper, not a hook, since its only consumer is the imperative widget-registry prefetch effect in SidebarContextProvider. The provider gate (topics only for the openedx provider) moves inside queryFn, the model-store bridge learns idField so the discussionTopics model stays keyed by usageKey, and useDispatch leaves SidebarContextProvider. The useModel readers are untouched (#1977), and courseware/data/thunks.js is deleted. BREAKING CHANGE: the sidebar widget prefetch contract no longer receives dispatch — the context object is now { courseId, course, queryClient }. External SIDEBAR_WIDGETS whose prefetch dispatched a Redux thunk must fetch via the provided React Query client instead. Closes #2016 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| Back | FazBrowse Home | New Git URL |
Summary
Convert the last courseware thunk to React Query: getCourseDiscussionTopics becomes prefetchDiscussionTopics, an imperative queryClient.query(...).catch(noop) wrapper driven by the sidebar widget-registry prefetch, and courseware/data/thunks.js is deleted. This completes Target 5 (leaf) of the courseware decomposition (plan) in the Redux → React Query migration (#1946, Stage 1), stacked on the remaining-writers conversion #2067. Closes #2016.
Breaking change
The sidebar widget prefetch contract no longer receives dispatch — the context object is now { courseId, course, queryClient }. External widgets registered via the SIDEBAR_WIDGETS config key whose prefetch dispatched a Redux thunk must fetch through the provided React Query client instead. The commit carries a refactor!: subject and a BREAKING CHANGE: footer (semantic-release major). Why break now rather than at the Redux teardown: the registry mechanism is ~5 months old (#1885/#1897), its only in-repo prefetch user was this thunk, a code search finds no external consumer, and #1976 makes the break inevitable — one contract change instead of two.
What changed
Testing
npm run types (0 errors), npm run lint (clean), full jest suite green at head (111 suites, 1120 passed / 3 pre-existing skips). Manual pass on tutor local in the details block below; the legacy-provider skip rests on its hook-level case (mapped in the manual-testing results).
Decisions
Full decision logDecisions — getCourseDiscussionTopics → React Query (#2016)
-
-
-
-
-
-
-
-
-
-
Manual testingA prefetch function, not a hook. No reader converts in this issue
(useModel → query-result read conversions are Dissolve the model-store normalized cache #1977's home), and the
thunk's only dispatcher is imperative — the widget-registry prefetch effect
in SidebarContextProvider — so courseware/data/apiHooks.ts gains
prefetchDiscussionTopics(queryClient, courseId) wrapping
queryClient.query(...).catch(noop) rather than a useQuery hook. The three
useModel('discussionTopics', unitId) readers (SidebarContextProvider,
DiscussionsSidebar, DiscussionsTrigger) are untouched and keep working
through the bridge.
The provider gate lives inside queryFn. Same two-call sequence as
the thunk: fetch the discussion config, and only for
config.provider === 'openedx' fetch topics (original comment and
usageKey filter preserved). Non-openedx resolves to [] — the bridge's
updateModels over an empty array is a no-op, the same end state as the
thunk's dispatch-nothing path (models.discussionTopics stays unset either
way; returning null instead would crash the bridge's forEach).
The .catch(noop) means the call never rejects; failures still log
through the app QueryCache onError → logError, the thunk's catch behavior.
The bridge gained an idField pass-through — the keying behavior itself
is pre-existing Redux code, not something this layer added. The
model-store slice has always keyed models by model[idField ?? 'id'] (the
add/update helpers in generic/model-store/slice.js, with idField
accepted on every add/update action payload), and the old thunk already
dispatched updateModels({ …, idField: 'usageKey' }) — the
discussionTopics model is keyed by usageKey (the unit id; each topic
keeps its own id, the discussion topic id, which readers also check).
What was missing was only the bridge link: ModelMirror couldn't express a
non-id key, so this layer forwards idField from the query meta onto
the dispatched action payloads — uniformly across all five strategies
rather than special-casing updateModels. The slice is untouched.
The widget prefetch contract is broken deliberately: dispatch →
queryClient. prefetch({ courseId, course, dispatch }) is a documented
plugin contract (sidebar/README.md, ARCHITECTURE.md) for external
widgets registered via the SIDEBAR_WIDGETS config key, so this is a
breaking change and the commit advertises it twice: a refactor!: subject
and a BREAKING CHANGE: footer (semantic-release major). It breaks now rather than at the Redux teardown
(Tear down the courseware Redux slice + replace useContextId #1976) because: the registry mechanism is ~5 months old (PRs feat: decouple notifications panel using widget registry mechanism #1885/feat: move discussion topic prefetch from trigger to widget config lifecycle #1897);
its only in-repo prefetch user was this thunk; a GitHub code search finds
no external SIDEBAR_WIDGETS consumer (caveat: operator env.config.jsx
files are untracked and unsearchable); and the teardown makes the break
inevitable regardless — deferring would mean touching the same contract
line and the same three docs twice for one break's worth of change. The
context object is now { courseId, course, queryClient } (queryClient
from useQueryClient(), a stable reference in the effect deps), and
useDispatch + the react-redux import leave SidebarContextProvider
entirely. The two sidebar docs and the discussions widget README were
updated in the same layer.
courseware/data/thunks.js deleted — getCourseDiscussionTopics was
the last thunk left after Convert saveIntegritySignature + saveSequencePosition to React Query mutations #2015, and courseware/data/index.js already had
no ./thunks exports, so only the file itself goes.
Light structural types instead of any. The discussion api functions
(api.js) are untyped, so the queryFn annotates locally:
config: { provider: string } and topics: { usageKey: string | null }[]
(course-wide topics carry a null usage key — that's what the filter drops).
No as any.
Tests seed through the real converted path. The three files that
seeded via executeThunk(getCourseDiscussionTopics(...))
(DiscussionsSidebar.test.jsx, DiscussionsTrigger.test.jsx,
courseware/course/test-utils.jsx) now run
prefetchDiscussionTopics(createTestQueryClient(store), courseId) — the
bridge-wired client populates the model store exactly as production does,
and the existing endpoint mocks keep exercising the provider gate. New
apiHooks.test.tsx cases: openedx success with the usage-key filter
(previously untested), the legacy-provider skip (no topics request), and
the config-failure path (logError, nothing written); plus a bridge
idField case in modelStoreBridge.test.ts.
SidebarContextProvider.test.jsx gained a QueryClientProvider
wrapper and lost its react-redux mock. That suite renders the provider
with raw RTL render in a fully mocked environment (the model store is
jest-mocked), so the new useQueryClient() call needed a plain
new QueryClient() wrapper — consistent with the file's isolated-unit
style; no store/bridge wiring needed since the mocked widgets define no
prefetch. The react-redux jest mock existed only to feed the provider's
useDispatch, which is gone.
Behavior deltas are the standard query-conversion posture. The thunk
fetched once per effect fire; the query gets the app default
shouldRetryQuery (up to 3 retries on 5xx/network) and queryClient.query
dedupes an in-flight fetch. With the default staleTime: 0, effect
re-fires still refetch — effectively the thunk's fire-every-effect
behavior.
queryClient.query(...).catch(noop), not prefetchQuery (review,
arbrandes). The installed @tanstack/query-core (5.102.8, via
^5.90.19) marks prefetchQuery @deprecated: "Use queryClient.query(options)
instead. You can swallow errors with .catch(noop). This method will be
removed in the next major version." The two are the same call —
prefetchQuery(options) is literally fetchQuery(options).then(noop).catch(noop)
and query(options) is fetchQuery(options) — so the replacement unwraps
the deprecated helper without changing the cache build, staleTime check,
retry defaults, or the meta bridge. noop is the library's own export
(import { noop } from '@tanstack/react-query'), the idiom the TanStack
prefetching guide shows; it reads as "deliberately discarded" where an
empty arrow reads as unhandled. The function now resolves with the topics
instead of undefined; the one production caller (discussionsPrefetch)
ignores the result and the five test call sites only await it. The
sidebar README's widget prefetch example switched to
queryClient.query(...) too, with a self-contained .catch(() => {})
rather than an extra import for a six-line snippet.
Manual testing — getCourseDiscussionTopics → React Query (#2016)
In-browser verification for the discussion-topics layer, run against a live
backend (tutor local). This layer claims zero user-facing change: the
thunk becomes prefetchDiscussionTopics (same config → provider gate →
topics sequence, bridged into the same discussionTopics model keyed by
usage key), and every reader keeps reading the model. The things to watch are
the old semantics: the prefetch firing from SidebarContextProvider's
post-mount effect, the trigger/sidebar appearing only for units with an
in-context topic, and legacy-provider courses staying untouched.
Getting real IDs (DemoX on tutor local)
Course id: course-v1:OpenedX+DemoX+DemoCourse; base
http://apps.local.openedx.io:2000/learning.
and forum plugin) — confirm via the config GET below reporting
"provider": "openedx".
followed (openedx provider only) by a GET to
/api/discussion/v2/course_topics/{courseId}, fired once per course on
courseware mount.
Verify by hand
On a unit with discussions enabled in context (openedx provider):
the v1/courses config GET, then the v2/course_topics GET; no console
errors.
renders in the right rail; clicking it opens the sidebar iframe at
{DISCUSSIONS_MFE_BASE_URL}/{courseId}/category/{unitId}?inContextSidebar.
discussion topic (or discussions disabled in context), the trigger does
not render.
URL to the new unit without refetching topics (the query is cached per
course; a remount refetches).
Legacy-provider course (env permitting; if no local legacy-provider course
exists, the automated coverage below carries this half):
provider and no course_topics request follows; no trigger renders and
no console errors.
Left to the automated suite (not re-done by hand)
(prefetchDiscussionTopics): openedx success keyed by usage key with the
course-wide-topic filter, the legacy-provider skip (no topics request), and
the config-failure path (logError, nothing written).
(render with a topic, nothing without), now seeded through the real
prefetch + bridged test client.
Course.test.jsx (via setupDiscussionSidebar), CoursewareContainer.test.jsx.
Results
Env: tutor local, run against the local branch @ 1ed4aa20 (before any PR).
The four openedx-provider items passed as described on DemoX (prefetch pair
on mount, trigger + iframe category URL, no trigger without an in-context
topic, no refetch on unit navigation).
The legacy-provider skip was not run by hand — no local course uses the
legacy provider. A course with discussions disabled entirely (no discussion
tab) was checked instead: no discussion requests fire at all, no trigger, no
console errors — that exercises discussionsPrefetch's tab gate in
widgetConfig.js, not the provider gate inside queryFn. The provider gate
rests on the automated legacy-provider case in apiHooks.test.tsx (config
GET only, no course_topics request, nothing written).
🤖 Generated with Claude Code