refactor!: clean up SidebarContextProvider for React Query and convert it to TypeScript - #2113
Open
brian-smith-tcril wants to merge 1 commit into
Open
brian-smith-tcril wants to merge 1 commit into
brian-smith-tcril wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## bsmith/await-test-assertions #2113 +/- ##
================================================================
+ Coverage 94.79% 94.86% +0.07%
================================================================
Files 368 370 +2
Lines 5952 5995 +43
Branches 1457 1465 +8
================================================================
+ Hits 5642 5687 +45
+ Misses 297 295 -2
Partials 13 13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
brian-smith-tcril
force-pushed
the
bsmith/sidebar-provider-react-query
branch
2 times, most recently
from
September 24, 2026 12:24
171f7f8 to
193b3fd
Compare
brian-smith-tcril
force-pushed
the
bsmith/sidebar-provider-react-query
branch
from
September 24, 2026 13:47
193b3fd to
637db28
Compare
brian-smith-tcril
added this pull request to stack #2117
September 24, 2026 13:47
7 tasks
…t it to TypeScript The courseware sidebar provider becomes TypeScript, and the two patterns in it that predate React Query go with the conversion instead of being typed around. The context was created with a default object, so a consumer rendered outside `SidebarProvider` got empty values instead of an error; it is now `createContext<SidebarContextValue | null>(null)` with a `useSidebarContext()` hook that throws outside the provider. The provider ran every widget's `prefetch` from an effect that read the merged course metadata through a ref, PR #1897's fix for the effect firing twice as the two metadata models settled in separate batches, approved then as a workaround pending React Query. The one `prefetch` user, the discussions widget, now loads its topics as a query observer in a `Provider` component, mounted by the framework for every enabled widget inside `SidebarContext`, with `enabled` set only when the discussions MFE is configured and the course has a discussion tab. An observer fetches on mount and on key change, not when the metadata it reads re-renders, so one sidebar mount is one topics request and a metadata write is none. The query definition is a `queryOptions` object, `discussionTopicsQuery`, shared with the tests and with #2087's reader hook; its bridge `meta` stays until that layer converts the three `useModel` readers. The effect, `courseMetaRef`, the duplicated merge and the `prefetch` field of the widget contract are gone. The widget contract gets named types in `SidebarContext.ts`: `SidebarWidget`, `SidebarWidgetContext` (`course: CourseHomeMeta & CoursewareMeta`, `unit: Partial<DiscussionTopic>`, the line #2087 changes) and `SidebarContextValue`. `CoursewareMeta` and `DiscussionTopic` are declared beside their queries. Both built-in widget configs are TypeScript and declared against `SidebarWidget`; the upgrade widget's context module converts with them. The sidebar README's contract sections point at the declarations instead of restating them, and the prefetch section describes the `Provider` observer. Tests: `DiscussionsProvider.test.tsx` measures the observer's gates as requests and pins that a course-metadata write does not refetch the topics; `SidebarContext.test.tsx` covers the hook's throw; `LockPaywall`, `SequenceNavigation` and `SequenceNavigationTabs` render their context consumer under a provider, which the default value had let them skip. The provider's first render now reads `useWindowSize().width ?? window.innerWidth` instead of an undefined width. That fixes a bug present since #1885: on a narrow viewport the first render seeded the sidebar closed, and the mobile branches of the sidebar hooks never corrected it, so a panel a learner had opened did not restore after a refresh. Desktop reaches the same state as before, without a transient outline render and two storage writes per load. BREAKING CHANGE: the `prefetch` field of a `SIDEBAR_WIDGETS` entry is no longer called. A widget that loaded data through it loads it in its `Provider` component with `useQuery` and `enabled`; see "Loading widget data" in src/courseware/course/sidebar/README.md. `useContext(SidebarContext)` returns `null` outside `SidebarProvider`; read the context with `useSidebarContext()` from src/courseware/course/sidebar/SidebarContext.ts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
brian-smith-tcril
force-pushed
the
bsmith/sidebar-provider-react-query
branch
from
September 24, 2026 20:50
637db28 to
7b8db4f
Compare
brian-smith-tcril
added a commit
that referenced
this pull request
Sep 24, 2026
… sidebar suites under the real provider Every consumer of the courseware sidebar context reads it through the `useSidebar()` hook #2113 introduced, and the context object is private to one module, `courseware/course/sidebar/SidebarContext.tsx`, which holds the types, the `createContext` call, `SidebarProvider` and the hook, the shape of the repo's four other typed contexts. Ten files swap `useContext(SidebarContext)` for the hook and change nothing else. Merging the provider and the hook put the provider's import of the built-in widget list on an import cycle (`SidebarContext.tsx -> defaultWidgets.js -> widgetConfig.ts -> DiscussionsProvider.tsx -> SidebarContext.tsx`). The provider stops importing the built-ins: `SidebarProvider` takes `widgets` as a required prop and `Course.jsx`, the composition root, passes `getEnabledWidgets()`, memoised once per mount. `buildSidebarsRegistry` and `getSidebarOrder` move into the module with their tests, since they operate on the provider's input. Two guards go with the tests that existed to hit them: `!SIDEBARS` in `Sidebar.jsx` and `!SIDEBAR_ORDER` in `SidebarTriggers.jsx` checked for a value the provider cannot produce and only a faked context could supply. The real conditions each keep one test under the provider. Tests: the eighteen sidebar suites that rendered a raw `SidebarContext.Provider` with a hand-rolled value render the real `SidebarProvider` with stub widgets, or the real registry where the registry is the subject, and set viewport width, the stored preference and the closed-by-user flag instead of a literal `currentSidebar`. A probe component, `src/tests/SidebarState.tsx`, renders the current sidebar as text for the suites whose tree does not show it. `Sequence.test`'s wrapper and its upgrade-panel cases, which #2118 put on the course route and the real provider, pass the provider its `widgets`. `test-utils.jsx` drops a context wrapper around `Course`, which renders the provider itself. Docs name `SidebarProvider` and `SidebarContext.tsx`; the use case verification's code-location links point at the hooks that hold each fix instead of line numbers in a file gone since the hooks extraction. BREAKING CHANGE: `SidebarContext` is no longer exported from `courseware/course/sidebar/SidebarContext` or `courseware/course/sidebar`; read the context with `useSidebar()` from the same module. `SidebarProvider` requires a `widgets` prop (`getEnabledWidgets()`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
brian-smith-tcril
removed this pull request from stack #2117
September 24, 2026 20:50
brian-smith-tcril
changed the base branch from
master
to
bsmith/await-test-assertions
September 24, 2026 20:51
brian-smith-tcril
added this pull request to stack #2121
September 24, 2026 20:51
brian-smith-tcril
added a commit
that referenced
this pull request
Sep 25, 2026
… sidebar suites under the real provider Every consumer of the courseware sidebar context reads it through the `useSidebar()` hook #2113 introduced, and the context object is private to one module, `courseware/course/sidebar/SidebarContext.tsx`, which holds the types, the `createContext` call, `SidebarProvider` and the hook, the shape of the repo's four other typed contexts. Ten files swap `useContext(SidebarContext)` for the hook and change nothing else. Merging the provider and the hook put the provider's import of the built-in widget list on an import cycle (`SidebarContext.tsx -> defaultWidgets.js -> widgetConfig.ts -> DiscussionsProvider.tsx -> SidebarContext.tsx`). The provider stops importing the built-ins: `SidebarProvider` takes `widgets` as a required prop and `Course.jsx`, the composition root, passes `getEnabledWidgets()`, memoised once per mount. `buildSidebarsRegistry` and `getSidebarOrder` move into the module with their tests, since they operate on the provider's input. Two guards go with the tests that existed to hit them: `!SIDEBARS` in `Sidebar.jsx` and `!SIDEBAR_ORDER` in `SidebarTriggers.jsx` checked for a value the provider cannot produce and only a faked context could supply. The real conditions each keep one test under the provider. Tests: the eighteen sidebar suites that rendered a raw `SidebarContext.Provider` with a hand-rolled value render the real `SidebarProvider` with stub widgets, or the real registry where the registry is the subject, and set viewport width, the stored preference and the closed-by-user flag instead of a literal `currentSidebar`. A probe component, `src/tests/SidebarState.tsx`, renders the current sidebar as text for the suites whose tree does not show it. `Sequence.test`'s wrapper and its upgrade-panel cases, which #2118 put on the course route and the real provider, pass the provider its `widgets`. `test-utils.jsx` drops a context wrapper around `Course`, which renders the provider itself. Docs name `SidebarProvider` and `SidebarContext.tsx`; the use case verification's code-location links point at the hooks that hold each fix instead of line numbers in a file gone since the hooks extraction. BREAKING CHANGE: `SidebarContext` is no longer exported from `courseware/course/sidebar/SidebarContext` or `courseware/course/sidebar`; read the context with `useSidebar()` from the same module. `SidebarProvider` requires a `widgets` prop (`getEnabledWidgets()`). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
brian-smith-tcril
marked this pull request as ready for review
September 25, 2026 02:55
This was referenced Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The courseware sidebar's
SidebarContextProviderbecomes TypeScript, and the two patterns in it that predate React Query go with the conversion rather than being typed around: the context's default object, which let a consumer outside the provider render empty instead of failing, and theprefetcheffect that read the merged course metadata through a ref (courseMetaRef) to avoid duplicate requests — PR #1897's fix, approved then as "a bit hacky. The real solution is going to be moving to React Query, later on." The discussions widget now loads its topics as a React Query observer in aProvider, gated withenabled, and theprefetchfield leaves the widget contract. The contract gets named types —SidebarWidget,SidebarWidgetContext,SidebarContextValueinSidebarContext.ts— that the README points at instead of restating, and both built-in widget configs are declared against them. One user-facing fix and no request-count change: a sidebar panel opened on a narrow viewport now restores after a refresh, which it never has onmaster(the provider's first render ran before Paragon had measured the window and seeded the closed state; decision 10). Otherwise one topics request per sidebar mount, as before, and none on a metadata write. Breaking for operators: theprefetchwidget-config field is removed anduseContext(SidebarContext)outside the sidebar returnsnull— see Operators — breaking below. Part of the Redux → React Query migration (#1946, Stage 1); peeled out of #2087 so that layer changes the declared type of one field. Closes #2111.What changed
widgets/discussions/DiscussionsProvider.tsxcallsuseQueryondiscussionTopicsQuery(courseId)withenabled: !!DISCUSSIONS_MFE_BASE_URL && hasDiscussionTab(tabs)— the gatediscussionsPrefetchapplied — mounted through the contract's existingProviderfield, which the framework wraps around the sidebar children for every enabled widget whether or not it is available. An observer fetches on mount and on key change, not when the metadata it reads re-renders, so the case the ref protected against (a re-fire after acourseHomeMetawrite) is handled by the model.prefetchDiscussionTopicsanddiscussionsPrefetchare deleted; the query definition is aqueryOptionsobject exported fromcourseware/data/apiHooks.ts, and its bridgemetastays until Read discussion topics from the query, not useModel #2087 converts the threeuseModelreaders.courseMetaRef, the second merge anduseQueryClient.courseis built once, ingetAvailableWidgets, as in feat: decouple notifications panel using widget registry mechanism #1885.SidebarContext.ts.createContext<SidebarContextValue | null>(null)plus a throwinguseSidebar(), theToastContextshape; exportsSidebarWidget,SidebarWidgetContext(course: CourseHomeMeta & CoursewareMeta,unit: Partial<DiscussionTopic>) andSidebarContextValue.CoursewareMetaandDiscussionTopicare added beside their queries incourseware/data/apiHooks.ts, the way refactor: read courseHomeMeta from the query in the tab page, alerts and course-home tabs #2109 addedCourseHomeMeta.SidebarContextProvider.tsx. APropsinterface, typed refs,enabledWidgetstypedSidebarWidget[]so the compiler checks theisAvailablecontext literal, Paragon's breakpoint constant asserted, and the first render readsuseWindowSize().width ?? window.innerWidth— which fixes amasterbug where a sidebar panel opened on a narrow viewport did not restore after a refresh (decision 10). The four sidebar hooks convert with it, their JSDoc@paramblocks (which had typed the refs asFunction/Object) becomingParamsinterfaces;priorityis required onSidebarWidget, as the contract always documented.SidebarWidget, withdiscussionsIsAvailableandupgradeIsAvailabletakingSidebarWidgetContext;UpgradeWidgetContext.tsxconverts with them (itspropTypes-inferredchildrendid not fit theProvidertype).sidebar/README.md's Widget Structure and Context Object become import lines plus one annotated example, Theprefetchfield is gone and TheProviderfield covers its subject, showingDiscussionsProvideras the data-loading use; consumer examples useuseSidebar().ARCHITECTURE.md,USE_CASE_VERIFICATION.md, the discussions README and the upgrade README follow.DiscussionsProvider.test.tsxreplaceswidgetConfig.test.ts: the same gates measured as requests, plus does not load the topics again when the course metadata changes — the feat: move discussion topic prefetch from trigger to widget config lifecycle #1897 case, pinned.SidebarContext.test.tsxcovers the hook's throw. Three suites that rendered a context consumer without a provider (LockPaywall,SequenceNavigation,SequenceNavigationTabs) get one; they were found by running the suite with the default removed.Operators — breaking
The
prefetchfield of aSIDEBAR_WIDGETSentry is no longer called. A widget that loaded data through it loads it in aProvidercomponent instead, as a React Query observer gated withenabled; the framework mounts every enabled widget'sProviderinsideSidebarContextwhether or not the widget is available, so the data loads before the widget's trigger can render, asprefetchdid. The discussions widget is the built-in example:useContext(SidebarContext)returnsnulloutsideSidebarProvider. Under the sidebar — where every widget component renders — nothing changes;useSidebar()from the same module is the typed read and throws a readable error if a component is rendered outside it. A widget'sisAvailablestill receives{ courseId, unitId, course, unit }with the same values as before; the shape is now the exportedSidebarWidgetContexttype, and the sidebar README's Widget Structure and Context Object sections point at the declarations.Testing
npm run typesandnpm run lintclean; full suite 117 suites, 1173 passed, 0 skipped (on #2118, which un-skips three);SidebarContextProvider.tsxat 100% lines and branches under its own suite. Negative check: removingunitfrom the provider'sisAvailablecontext literal failsnpm run types("not assignable to parameter of type 'SidebarWidgetContext'"). The suites that needed a provider were identified by running the suite with the context default removed before adding any: exactlyLockPaywall,SequenceNavigationandSequenceNavigationTabsfailed. Manual testing per the checklist below, on tutor dev: 8 of 11 checks run, all passing — onev1/coursesand onev2/course_topicsrequest per hard reload and none added by unit or sequence navigation, the trigger and panel present on a unit with a topic and absent on one without, the stored-preference auto-open, a console clean ofuseSidebarand null-destructure errors, and anenv.config.jsxprobe widget whoseprefetchnever fires while itsProvidermounts. Not run: the celebration-write case (needs a fresh enrolment with celebrations on; pinned by theDiscussionsProvidertest), the upgrade trigger (needs a paid track; the widget's runtime is unchanged), and a legacy-provider course (none local; covered by the query's legacy case).Decisions
Full decision log
Decisions — clean up
SidebarContextProviderfor React Query and convert it to TypeScript (#2111)Peeled out of #2087 (layer C of the #1977 model-store dissolution); the
second layer of the stack that follows the fully landed #2080, above the
test-only #2118 layer inserted in review on 2026-09-25. Entries 1–9 were
settled in the plan review on 2026-09-24 and posted to #2111; the rest landed
with the code.
A deep layer, peeled out of a wide one. Read discussion topics from the query, not useModel #2087 swaps three
useModel('discussionTopics')readers lightly — a wide layer. One of themis the provider's
unit, which it hands to every widget'sisAvailableunder a README contract that types it only as
unit: object, so the"
undefinedor{}for a unit with no topic" question had no honest homein a README comment. This layer converts the provider and gives the widget
contract named types, so Read discussion topics from the query, not useModel #2087 changes the declared type of one field.
Few files, every change in them the right one.
Not a plain
.jsx → .tsxpass. The first draft was "only what thecompiler forces"; rejected in review. Typing the file as it stood meant
typing around two patterns that exist only because it predates React
Query — a context created with a default object, and an effect that reads
the merged metadata through a ref to avoid re-firing — and the reason not
to do a plain conversion is exactly that we would be hacking around
patterns that no longer make sense just to make the types work. Both go
here; the types describe the file as it should be.
The context has no default value;
useSidebar()throws outsidethe provider.
createContexthad a default object, souseContext(SidebarContext)outsideSidebarProviderreturned{ currentSidebar: null, …, availableSidebarIds: [] }and the consumerrendered empty with no error. Every consumer in the app is under the
provider (
Course.jsx), so the default was read only by tests thatrendered a consumer bare — and it hid exactly the mistake a context should
report. Now
createContext<SidebarContextValue | null>(null)plus a hookwith a readable error, the
ToastContextshape. The hook was firstnamed
useSidebarContext; renamed in review touseSidebarto match therepo's four typed contexts (
useToast,usePluginOverrides,useCoursewareSearch,useTourState), which name what the hook givesyou, not the mechanism.
SidebarContextstaysexported: the built-in widgets,
LockPaywall,SidebarBaseand the twohooks files still call
useContext(SidebarContext)— anullon thedestructure is the loud failure wanted, and those files convert to the
hook as their own layers touch them;
SidebarContext.Provideris howtests supply a value. First adopters of the hook:
DiscussionsProviderand
UpgradeWidgetContext. The suites that rendered a consumer without aprovider were found by running the suite with the default removed before
any provider was added:
LockPaywall.test.jsx,SequenceNavigation.test.jsxandSequenceNavigationTabs.test.jsx(bothreach
useIsSidebarOpeninsequence-navigation/hooks.js). The plan'sstatic count had
UnitNavigation.test.jsxand five bare renders inSequence.test.jsxinstead of the tabs suite; neither reaches a consumer,and the tabs suite does. Each got a
SidebarContext.Providerwith the twofields its consumer reads, the partial-
valueshape the widget suites use.The prefetch effect and
courseMetaRefare replaced by a queryobserver in the discussions widget's
Provider. How it worked: theprovider ran every widget's
prefetchfrom an effect keyed oncourseIdand the widget list, reading the merged course metadata through a ref
assigned every render. Why: PR feat: move discussion topic prefetch from trigger to widget config lifecycle #1897's first version had the effect depend
on the two metadata models;
arbrandesrequested changes because themodels settled in separate React batches on initial load, so the effect
fired twice and the thunk fetched twice ("two requests to each endpoint
returning identical data"), and suggested a ref; his approval read "the
refworkaround solves it, but it's a bit hacky. The real solution isgoing to be moving to React Query, later on." What React Query changed:
under B3 the provider renders after the gate, so both metadata sources are
present at first render (no two-batch double fire), and
queryClient.querydedupes an in-flight request. What it did not change: at
staleTime: 0afire after settlement refetches, and the merge does change after first
render — the celebration writers B3 moved onto
setQueryDatagivecourseHomeMetaa new reference — so simply depending on the merge againwould trade duplicate requests on load for a new topics request on every
metadata write. The fix is the model's own: an observer with
enabledfetches on mount and on key change, not when the metadata it reads
re-renders.
DiscussionsProviderobservesdiscussionTopicsQuery(courseId)with
enabled: !!DISCUSSIONS_MFE_BASE_URL && hasDiscussionTab(tabs), thesame gate
discussionsPrefetchapplied, mounted through the contract'sexisting
Providerfield — which the framework wraps around the sidebarchildren for every enabled widget, inside
SidebarContext, whether or notthe widget is available. Same post-mount timing as the effect, one
request per sidebar mount, none on a metadata write (pinned by a test).
The bridge
metastays on the query definition until Read discussion topics from the query, not useModel #2087 converts thethree readers.
The
prefetchfield leaves the widget contract — breaking. Why itexisted: before feat: move discussion topic prefetch from trigger to widget config lifecycle #1897
DiscussionsTriggerfetched its own topics, but theframework mounts a trigger only when its widget is available and the
discussions widget's availability depends on the topics, so the fetch
moved to framework level, run for every enabled widget regardless of
availability. In a Redux world "load data" was "dispatch a thunk", a plain
function call, so it became a callback given
dispatch(laterqueryClient, Convert getCourseDiscussionTopics to React Query #2016) andcourse;Provider, added days earlier infeat: decouple notifications panel using widget registry mechanism #1885, was framed as panel↔trigger shared state, not data. With React
Query, loading is a hook and needs a component; the framework already
mounts one per enabled widget regardless of availability —
Provider.Same idea, React Query's shape, no ref. Keeping
prefetchfor ahypothetical external widget would keep the effect and the ref for no
in-repo caller, two ways to do one thing with the older one needing the
workaround. Removed, with
courseMetaRef, the second merge anduseQueryClientin the provider;refactor!:with a footer naming thefield and its replacement. The field had already changed shape once with
no external consumer found (Convert getCourseDiscussionTopics to React Query #2016,
dispatch → queryClient).course: CourseHomeMeta & CoursewareMeta, inline. The merge{ ...coursewareMeta, ...courseHomeMeta }is two endpoints with a modestoverlap (
id,title,start,celebrations,isEnrolled,userTimezone); thecourseware/course/{id}payload has ~30 fields thecourse_metadatapayload does not (enrollmentMode,accessExpiration,userNeedsIntegritySignature,entranceExamData, …) and vice versa(
tabs,org,isStaff,verifiedMode,courseAccess, …). The firstdraft declared
CourseHomeMetaalone on the strength of its indexsignature; review asked whether the course-home payload is a superset —
it is not — so that was half a type under the wrong endpoint's name. A
sidebar-local alias (
SidebarCourse) to keep "course home" out of acourseware contract was offered and rejected as a second name for the
same thing.
CoursewareMetais added tocourseware/data/apiHooks.tsthe way B2 added its counterpart: an index-signature interface and the
type parameter on
useCoursewareMetadata; it starts as the indexsignature alone because no TypeScript reader of that payload exists yet
(D3 names fields as its readers convert),
getCourseMetadatareturnsanyso the annotation is accepted, and no caller reads.datatoday.Not
Partial: the provider renders under the gate (LoadedTabPagethrows unless the metadata query is in success), so
courseHomeMetaisdefined at every render in the app. Spreading the
anycoursewareMetamakes the literal
any, so the compiler accepts the declaration ratherthan deriving it; the declaration is the statement.
unit: Partial<DiscussionTopic>— the honest type of today's value, andthe line Read discussion topics from the query, not useModel #2087 changes.
useModel('discussionTopics', unitId)returnsthe topic or
{}; both arePartial<DiscussionTopic>, andunit?.id && unit?.enabledInContexttype-checks against it.DiscussionTopic(id,usageKey,enabledInContext, index signature)is added beside the query that produces it in
courseware/data/apiHooks.ts,replacing the inline
{ usageKey: string | null }[]annotation; Read discussion topics from the query, not useModel #2087'sselectreuses it. When Read discussion topics from the query, not useModel #2087's hook returnsundefinedfor a unit withno topic, the declaration becomes
unit?: DiscussionTopic— a change awidget author's editor shows, checked against the built-in widgets' own
typed
isAvailableparameters.Where the two new types live, and why
courseware/data/apiHooks.tswasnot split.
DiscussionTopicandCoursewareMetasit beside the queriesthat produce them, the repo's convention (
CourseHomeMetabesideuseCourseHomeMeta,CourseOutlineDataincourseOutline.ts,TabMetadataincourse-tabs/utils.ts); puttingDiscussionTopicunderwidgets/discussions/would have the data layer import from a widget,which nothing else does. Review noted that
apiHooks.tshas become a grabbag — three course-level queries, the sequence query, the sidebar outline
query and its toggles, the topics query, four mutations, the types and
useSequenceIds— and chose not to split it in this layer: no bettershape is obvious yet, splitting a data module is not this layer's subject,
and after Dissolve the model-store normalized cache #1977 removes the bridge entries and the D layers move the
mutation writes onto the queries, the file's contents change enough that
seams chosen now may not be the right ones. A per-concern split
(
courseOutline.tsis the existing example of the shape) is its own smalllayer when the shape is clear.
One query definition,
discussionTopicsQuery(courseId), throughqueryOptions. It is what the Provider observes, what Read discussion topics from the query, not useModel #2087's readerhook will spread, and what the tests seed with. The
queryFnbody movedverbatim from
prefetchDiscussionTopics(comment included); the bridgemetamoved with it.prefetchDiscussionTopicsanddiscussionsPrefetchare deleted. One test-visible difference: the prefetch swallowed
rejections with
.catch(noop);queryClient.query(discussionTopicsQuery(…))rejects, so the config-failure case in
apiHooks.test.tsxawaitsrejects.toThrow()and then asserts what it did before (logErrorcalled through the QueryCache
onError, nothing written). The threeseeding suites swap one line each.
Both built-in widget configs convert, and with them the upgrade widget's
context module.
widgets/discussions/widgetConfig.tschanges anyway(
prefetchout,Providerin) and is declaredSidebarWidget, withdiscussionsIsAvailabletakingSidebarWidgetContext— that is whatmakes the contract check a built-in's own function, not only the argument
the provider hands it.
widgets/upgrade/src/widgetConfig.tsandutils.tsconvert the same way ("related enough").
UpgradeWidgetContext.jsxhadto follow: its
propTypes(children: PropTypes.node.isRequired) makeTypeScript infer a
children: NonNullable<ReactNodeLike>prop that is notassignable to
Provider?: ComponentType<{ children: ReactNode }>. Thealternatives were loosening the contract type to fit a JavaScript
component's inferred props, or converting the component; it converted
(
UpgradeWidgetContext.tsx, theToastContextshape,useSidebar()for
courseId). Its JSDoc typedef became the real interface; its test isunchanged.
The first render reads the real window width, and a stored sidebar
preference now restores on a narrow viewport.
useWindowSizereturnswidth: number | undefined, and it really is undefined on the firstrender: Paragon's hook initialises state undefined and measures in a
layout effect (its comment: "so server and client renders match",
Paragon #3125).
breakpoints.extraLarge.minWidthis typedminWidth?: numberbecause oneBreakpointRangeinterface covers allsix ranges and
extraSmallhas nominWidth; forextraLargeit is theconstant
1200(Paragon #3524). The oldwidth < breakpoints.extraLarge.minWidthwas therefore two type errors. Now:
The
!on the breakpoint states a fact about Paragon's constants — theshape this repo used for the same problem in February 2026 (
8d2347b7,"always defined in practice"). The
?? window.innerWidthis the valueParagon's layout effect is about to store, so the first render computes
the same viewport branch the second will.
This one is a behaviour fix, found by review of the next layer. The
first version of this layer kept the JavaScript behaviour exactly —
width !== undefined && width < …,falseon the first render asundefined < 1200was — and recorded the fallback as "left for its ownevidence": it had lived only in the
new-sidebarTypeScript variant thatfeat: decouple notifications panel using widget registry mechanism #1885 deleted (Braden's Bump paragon to v22.13.0, fix minor TypeScript warning #1572, a type fix for Paragon's new
undefined,whose fallback happened to be the right runtime value), never in the
JavaScript provider this file descends from, so adopting it was new
behaviour for this file, and the desktop checks on hand passed. The
evidence came from Make useSidebar() the only sidebar context read and test the sidebar suites under the real provider #2112's suites, the first to render the real
provider on a narrow viewport with a stored preference: with the
falsefirst render,useInitialSidebartakes the desktop branch,sees
isInitiallySidebarOpenfalse and returnsnull, whichuseState(initialSidebar)seedscurrentSidebarwith; after the layouteffect measures,
initialSidebarbecomes the stored value, but on anarrow viewport
useSidebarSyncanduseUnitShiftBehaviorboth returnearly by design ("MOBILE: persist state, no auto-switching"), so nothing
corrects the seed. A learner who opens a panel on a phone and refreshes
finds it closed, on
mastersince feat: decouple notifications panel using widget registry mechanism #1885 — reproduced by hand on tutordev 2026-09-24. On desktop the seed was being corrected in two hops
(
useResponsiveBehavioropens the outline and writes it to storage,then
useSidebarSyncswitches to the stored panel and writes that),which is why fix: don't auto-open right sidebar by default #1923's desktop checks and refactor!: clean up SidebarContextProvider for React Query and convert it to TypeScript #2113's manual test passed.
With the fallback the seed is right on both viewports; desktop reaches
the same end state in every case (nothing stored, a stored right panel,
a stored outline, closed by user) without the transient outline render
after first paint and without the two storage writes per load.
useInitialSidebartreats "storedCOURSE_OUTLINE" and "nothing stored"identically, so the missing write changes nothing later. Decided in
review 2026-09-24 to land here rather than in Make useSidebar() the only sidebar context read and test the sidebar suites under the real provider #2112, since this layer
owns the provider. Two drafts were rejected earlier: an
isWidthKnownalias narrowing both operands (a runtime check of a constant that can
never be undefined) and a named
extraLargeMinWidthvariable with acomment. A
width = 0default would have flipped the first render theother way (
0 < 1200).History for the record: before feat: decouple notifications panel using widget registry mechanism #1885 there were two providers, chosen
at runtime by the
isNewDiscussionSidebarViewEnabledcourse flag — theJavaScript
sidebar/SidebarContextProvider.jsx(this file's lineage,bare comparison) and the TypeScript
new-sidebar/SidebarContextProvider.tsxwith the fallback and, later, the February
!s. feat: decouple notifications panel using widget registry mechanism #1885 removed the flagand the
new-sidebartree.The sidebar hooks' JSDoc was wrong, TypeScript said so, and the hooks
converted. The project type-checks JavaScript through JSDoc, so
@param {Function} params.hasUserToggledRef(useSidebarSync,useResponsiveBehavior) and@param {Object} params.previousUnitIdRef(
useUnitShiftBehavior) rejected the typed refs the provider now passes.The first fix corrected the JSDoc in place (
{import('react').MutableRefObject<boolean>}/
<string|null>) and left the hooks JavaScript; review asked what aconversion would touch — nothing outside each hook:
hooks/index.jsre-exports by extension-less path, the provider already passes typed
arguments, the tests are JavaScript and pass
{ current: false }refs —and chose to convert all four (
useInitialSidebar,useUnitShiftBehavior,useSidebarSync,useResponsiveBehavior, plushooks/index.ts) so thedirectory is one thing. Each
@paramblock became aParamsinterface;the descriptive JSDoc prose stays; bodies are byte-identical.
getAvailableWidgetsis typed() => SidebarWidget[], which made the twoprioritycomparisons (firstAvailableWidget.priority < storedWidget.priority)fail while
prioritywas optional. It is now required onSidebarWidget:the contract always documented it as required (
priority: numberin theold README block), and
getEnabledWidgets'|| WIDGET_PRIORITIES.DEFAULTstays as the runtime fallback for untyped configs. In the same vein, the old
SidebarContextValuetypedef inSidebarContext.jslisted ten fieldsand omitted
availableSidebarIds, which the default object below itand the provider's
contextValuehad carried since feat: decouple notifications panel using widget registry mechanism #1885 — a JSDoctypedef on a
createContextcall is never checked against the value.The interface names it (
string[]), and the provider'suseMemo<SidebarContextValue>now fails to compile without it. Twotypes were also narrowed on purpose rather than corrected:
TriggerisComponentType<{ onClick: () => void }>(the one waySidebarTriggersrenders it; the README already said
onClickis injected by theframework) and
SidebarisComponentTypewith no props (Sidebar.jsxrenders it bare), where the typedef had
React.ComponentTypefor both.A widget whose Trigger or Sidebar needs another prop would render with
it missing, which is what the narrower type catches — for TypeScript
configs; the built-in components are still JavaScript, so their
propTypesare not compared until they convert.renderWithWidgetProvidersis onereduceRightwith a conditional.The
.filter(w => w.Provider).reduceRight(…)shape leftProviderpossibly undefined inside the reduce for TypeScript; a type-guard filter
or an assertion would have satisfied it.
(acc, { Provider }) => (Provider ? <Provider>{acc}</Provider> : acc)is the same behaviour withneither.
The README links to the declarations instead of mirroring them.
Widget Structure and Context Object each become one sentence naming
the type and linking
SidebarContext.ts. The declarations carry nocomments either: a first pass put one on each field the old README block
had annotated, and review found them either derivable from code an author
reads anyway (
getEnabledWidgetsshows theprioritydefault and theenabled !== falserule), covered by the README (Provider), ordescribing a transitional value Read discussion topics from the query, not useModel #2087 removes (
unit's{}); readingthem alongside the types was more confusing than the types alone. The first version of each section also carried an
import typeline and a short annotated example config; review droppedthem — the old sections were hand-written type definitions, and an
example with a lone
prioritycomment that disagreed with the field'sown comment was drift starting on day one. Say it is the type, link it,
stop. The
prefetchfield is gone and TheProviderfield absorbedits subject: the section now explains what a Provider is (always mounted
while the sidebar is, inside
SidebarContext), names its two uses withthe built-in that does each — shared state (
UpgradeWidgetProvider) andloading the data
isAvailabledepends on (DiscussionsProvider, withits code) — and keeps the config example. A first version put the
discussions example in a separate Loading widget data section written
in React Query terms ("an observer gated with
enabled… fetches when itmounts and when its key changes, not when the metadata it reads
re-renders"); review found it argued against a bug the reader had never
heard of (feat: move discussion topic prefetch from trigger to widget config lifecycle #1897's double fire) and separated the example from the field
it demonstrates. The rebuttal lives in entry 4; the README says what to
write and what the framework then does. The consumer examples use
useSidebar().ARCHITECTURE.md's prefetch lifecycle,USE_CASE_VERIFICATION.md's"effect #0", the discussions README's Data Prefetch and the upgrade
README's config shape follow. Mirroring was rejected for the reason
CourseHomeMeta's own comment gives — a copy drifts — and this contracthad drifted once already (Convert getCourseDiscussionTopics to React Query #2016 edited three docs by hand).
One
ARCHITECTURE.mdchange reads in the diff as a replacement and isnot. The provider's responsibilities list loses "Prefetch widget data …
via
widget.prefetch" and gains "Mount each widget'sProvideraroundthe sidebar children". Those are two independent edits that happen to
touch the same list: the prefetch bullet goes because the provider no
longer does that; the
Providerbullet is added because the providerhas done that since feat: decouple notifications panel using widget registry mechanism #1885 (
renderWithWidgetProviders, used by theupgrade widget's
UpgradeWidgetContextall along) and the doc neversaid so — it should have been there before. This layer gives the
mechanism a second user, which is what surfaced the omission. The doc
describes the behaviour, not its history, so it does not say "since
feat: decouple notifications panel using widget registry mechanism #1885"; that is recorded here.
Tests.
DiscussionsProvider.test.tsxreplaceswidgetConfig.test.tswith the same four gates measured as requests on a
MockAdapter(discussion tab → config + topics once; no tab / metadata not loaded / no
MFE URL →
fetchStatus'idle'and no request), plus does not load thetopics again when the course metadata changes — a
setQueryDataon thecourse-home key after the topics resolved leaves the request count at
two — the feat: move discussion topic prefetch from trigger to widget config lifecycle #1897 problem, pinned.
SidebarContext.test.tsxcovers thehook's throw.
SidebarContextProvider.test.jsxdrops theQueryClientProviderwrapper that Convert getCourseDiscussionTopics to React Query #2016 added for the prefetch effect'suseQueryClient(), now gone. The provider still calls one query hook,useCourseHomeMeta(courseId, { enabled: false }), but that suite hasmocked the module since Read courseHomeMeta from the query: tab-page, alerts, and course-home tabs #2085, its mocked widgets define no
Provider,and the one
Providerit does supply is a plain function — so nothingrendered touches a query client. Un-mocking
useCourseHomeMetatherewould bring the wrapper back. The
three provider-less suites are entry 3's. Negative type check: removing
unitfrom the provider's context literal failsnpm run typeswith"not assignable to parameter of type 'SidebarWidgetContext'".
Codecov after submit. The patch check flagged one line: the
return truefor a widget with noisAvailable— pre-existing anduncovered on
mastertoo, but the.jsx → .tsxrename makes the wholefile patch lines (the refactor: de-class CoursewareContainer #2020 / refactor: retire the courseHome Redux slice #2081 effect). Covered with treats a
widget without
isAvailableas always available inSidebarContextProvider.test.jsx, which tests the branch's behaviourrather than just hitting the line. The same run showed one partial
branch, the
children = nulldefault parameter that no caller everexercises (
Coursealways passes children); the default was dropped —children?: ReactNodealready makes the prop optional, and an omittedchild renders nothing whether
undefinedornull— so the file is100% lines and branches under its own suite.
The three suites supply
SidebarContext.Providerwith test data forvalue, not the realSidebarProvider— for now. Settled in reviewafter the code landed. The repo's convention for its other typed contexts
is to mount the real provider component in tests:
ToastProvider,PluginOverridesProvider,CoursewareSearchProviderandTourProviderare never faked, and
setupTest'srenderwraps every test in the twoapp-root ones.
SidebarContextis the odd one out — some twenty suitesrender the raw
SidebarContext.Providerwith a hand-rolled partialvalue (
mockData,defaultContextValue,buildContext(overrides)),a convention from feat: decouple notifications panel using widget registry mechanism #1885/feat: make widget registry to backward compatible #1899 that predates this layer. It exists for a
structural reason:
SidebarProvideris not an app-root provider;Course.jsxmounts it withcourseIdandunitId, and it derives itsvalue from the widget registry, the model store, the course-home query,
localStorage and the viewport, and wraps its children in every enabled
widget's
Provider(UpgradeWidgetProvider, and from this layerDiscussionsProvider, which observes the topics query when the metadatahas a discussion tab). A test of
LockPaywallwants "the sidebar isclosed", not all of that.
Three shapes were weighed for the three suites this layer made
provider-dependent. A shared fixture (
src/tests/sidebarContextValue.ts,one complete
SidebarContextValue, spread with overrides) was rejectedas a third pattern that institutionalises the fake. The real
SidebarProvideris the repo's pattern and is feasible here — the twonavigation suites already render under a router with the course hooks
mounted,
LockPaywallwould gainwrapWithRouter: trueand aunitId,and "nothing open" becomes the provider's own answer for a jsdom viewport
with empty storage — at the cost of
DiscussionsProviderrequesting thetwo discussion endpoints against an adapter that 404s them wherever the
metadata fixture has a discussion tab (as
Course.test.jsxalreadydoes), and of leaving the other suites on the fake unless they move too.
Hand-rolled value, matching the siblings (chosen, for now): each suite
passes the two fields its consumer reads. The move to the real provider
is a sweep across the sidebar suites, not this layer's. Where the
component under test is one the provider itself mounts
(
DiscussionsProvider.test.tsx,UpgradeWidgetContext.test.jsx) the rawcontext with a value stays legitimate regardless — wrapping those in the
provider would be circular.
Follow-up layer — filed as Make useSidebar() the only sidebar context read and test the sidebar suites under the real provider #2112 (2026-09-24): a wide layer that
(a) moves every remaining
useContext(SidebarContext)consumer —LockPaywall,SidebarBase,Sidebar,SidebarTriggers,sequence-navigation/hooks.js,course-outline/hooks.js, the discussionsand upgrade widget components — onto
useSidebar(), and (b)stops exporting
SidebarContextitself, so the hook is the only read.The two are coupled with the test convention: once the context is not
exported, the twenty-odd suites that render
SidebarContext.Providerwith a hand-rolled
valuehave to render the realSidebarProviderinstead — which is the repo's pattern for its other contexts and the
sweep this entry deferred. The two suites that test components the
provider itself mounts (
DiscussionsProvider,UpgradeWidgetContext)need a way to supply the context without the provider; a test-only
export or a small test helper is that layer's call.
refactor!:with aBREAKING CHANGE:footer. UnderSidebarProvidernothing a learner or an operator's widget observes changes: one topics
request per sidebar mount, trigger and panel where they were. Outside it,
useContext(SidebarContext)returnsnullanduseSidebar()throws — unreachable in the app. The
prefetchfield of aSIDEBAR_WIDGETSentry is removed; a widget that used it loads its datain a
ProviderwithuseQueryandenabled, as the sidebar README nowshows.
Manual testing
Checklist
Manual testing — clean up
SidebarContextProviderfor React Query and convert it to TypeScript (#2111)In-browser verification against a live backend (tutor dev).
What changed: the courseware sidebar's discussions widget loads its topics
through a
Providerthat observes the topics query withenabled, instead ofthe provider's
prefetcheffect; the effect, thecourseMetaRefworkaroundand the
prefetchwidget-config field are gone.SidebarContexthas nodefault value. The provider, the context, both built-in widget configs and the
upgrade widget's context module are TypeScript. The topics still land in the
model store for the three
useModelreaders (#2087 converts them).The three bugs this layer could introduce. (1) The topics request fires
more than once per sidebar mount, or fires again on a metadata write — the
observer's
enabledgate or its dependency on nothing but the key is wrong.(2) The topics never load — the gate never passes (wrong
tabsread, wrongconfig key), so the trigger never appears on any unit. (3) A consumer renders
outside the provider — now an error instead of an empty render; in the app
every consumer is under
Course, so this would only show as a crash on thecourseware page.
Setup
A course with the Open edX discussions provider (Authoring → Pages &
Resources → Discussions) and a verified mode,
DISCUSSIONS_MFE_BASE_URLsetin the tutor config (it is by default). At least one unit with in-context
discussions on and one with it off (Authoring, the unit sidebar's discussion
toggle). Browser devtools open on the Network tab, filtered to
discussion.Checks
Hard reload each page and wait for the Network tab to go idle before counting.
Request counts (bug 1)
Learning makes exactly two discussion requests, both from
DiscussionsProvider:GET …/api/discussion/v1/courses/{courseId}andGET …/api/discussion/v2/course_topics/{courseId}.The Network tab also shows the discussions MFE's own requests once the panel is open, because the
iframe is a frame of the same tab —
…/api/discussion/v1/threads/…,…/api/discussion/v2/courses/{courseId}/,and more. Count with the panel closed (the trigger renders without the iframe), or use the
Initiator column: learning's requests come from the learning bundle, the iframe's from the
discussions MFE.
v1/courses/{courseId}1,v2/course_topics/{courseId}1, nothing else under/api/discussion/.Course; one new pair of discussion requests at most, none if the sequence did not remount the provider. Record what was observed.courseHomeMetawithsetQueryData). No new discussion requests. Skip if no such course is set up; theDiscussionsProvidertest pins this case.The topics load and gate the widget (bug 2)
{DISCUSSIONS_MFE_BASE_URL}/{courseId}/category/{unitId}?inContextSidebar.useUnitShiftBehavior/useSidebarSync, unchanged here).sidebar.{courseId}="DISCUSSIONS"in localStorage (open discussions once, reload), the discussions panel auto-opens once the topics resolve.v1/coursesrequest; no trigger on any unit. If no legacy course exists locally, thediscussionTopicsQuerylegacy case carries it — note that here.A stored panel restores on a narrow viewport (the
masterbug this layer fixes)masterit is closed (reproduced 2026-09-24).Nothing renders outside the provider (bug 3)
useSidebaror a null destructure, on a unit with a topic and one without, with the paywall (a gated unit as an audit learner) and the sequence navigation visible.The breaking change is real
env.config.jsxwidget still declaringprefetch: register a minimal widget withprefetch: () => console.log('prefetch'); nothing logs, the widget'sSidebar/Triggerbehave per itsisAvailable. Optional; documents that the field is inert.Results
Run 2026-09-24 on tutor dev, course
course-v1:OpenedX+DemoX+DemoCourse(Open edX discussions provider). 10 of 13 checks run, all passing; 3 not run. The two narrow-viewport checks were run 2026-09-25, after the fix was added.Run
v1/courses/{courseId}once andv2/course_topics/{courseId}once; with the panel open the iframe added its ownv1/threads/?…andv2/courses/{courseId}/.v2/course_topicsrequest (v1/coursesnot checked); panel open — only the iframe's requests. So the provider did not remount across sequences.master2026-09-24 (panel closed after a narrow-viewport reload).useSidebarorCannot destructure. The remaining console noise (defaultPropsdeprecations; a ParagonDropdownidprop-type warning from@edx/frontend-component-header'sAuthenticatedUserDropdown) is unrelated to this layer and present onmaster.env.config.jsxwidget declaringprefetch(the breaking change): a probe widget with bothprefetchandProvider(each logging): the "Probe" trigger rendered and its panel opened;[probe] Provider mountedlogged (twice, ReactStrictMode's development double-invoke — one real mount);[probe] prefetch callednever logged. The field is inert, as documented.Not run
DiscussionsProvider.test.tsxdoes not load the topics again when the course metadata changes.verifiedMode). The upgrade widget's runtime is unchanged in this layer (its config,utils.tsandUpgradeWidgetContext.tsxconverted with bodies intact); its four suites pass.discussionTopicsQuerylegacy case inapiHooks.test.tsxand the no discussion tab case inDiscussionsProvider.test.tsx.🤖 Generated with Claude Code