From ccb7fad0b31de07bf72b9383439994a8d424b3fa Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Thu, 24 Sep 2026 03:58:29 -0400 Subject: [PATCH] refactor!: clean up SidebarContextProvider for React Query and convert 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(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`, 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 --- .../lock-paywall/LockPaywall.test.jsx | 6 +- .../SequenceNavigation.test.jsx | 7 +- .../SequenceNavigationTabs.test.jsx | 14 +- src/courseware/course/sidebar/ARCHITECTURE.md | 39 +++--- src/courseware/course/sidebar/README.md | 73 +++------- .../course/sidebar/SidebarContext.js | 35 ----- .../course/sidebar/SidebarContext.test.tsx | 10 ++ .../course/sidebar/SidebarContext.ts | 55 ++++++++ .../sidebar/SidebarContextProvider.test.jsx | 29 ++-- ...rovider.jsx => SidebarContextProvider.tsx} | 60 +++------ .../course/sidebar/USE_CASE_VERIFICATION.md | 6 +- .../sidebar/hooks/{index.js => index.ts} | 0 ...InitialSidebar.js => useInitialSidebar.ts} | 23 ++-- ...veBehavior.js => useResponsiveBehavior.ts} | 19 +-- .../{useSidebarSync.js => useSidebarSync.ts} | 25 ++-- ...iftBehavior.js => useUnitShiftBehavior.ts} | 34 ++--- src/courseware/course/test-utils.jsx | 6 +- src/courseware/data/apiHooks.test.tsx | 10 +- src/courseware/data/apiHooks.ts | 53 +++++--- .../discussions/DiscussionsProvider.test.tsx | 125 ++++++++++++++++++ .../discussions/DiscussionsProvider.tsx | 20 +++ .../discussions/DiscussionsSidebar.test.jsx | 4 +- .../discussions/DiscussionsTrigger.test.jsx | 5 +- src/widgets/discussions/README.md | 15 +-- src/widgets/discussions/widgetConfig.js | 23 ---- src/widgets/discussions/widgetConfig.test.ts | 49 ------- src/widgets/discussions/widgetConfig.ts | 16 +++ src/widgets/upgrade/README.md | 15 ++- ...etContext.jsx => UpgradeWidgetContext.tsx} | 43 +++--- src/widgets/upgrade/src/utils.js | 8 -- src/widgets/upgrade/src/utils.ts | 3 + .../src/{widgetConfig.js => widgetConfig.ts} | 3 +- 32 files changed, 466 insertions(+), 367 deletions(-) delete mode 100644 src/courseware/course/sidebar/SidebarContext.js create mode 100644 src/courseware/course/sidebar/SidebarContext.test.tsx create mode 100644 src/courseware/course/sidebar/SidebarContext.ts rename src/courseware/course/sidebar/{SidebarContextProvider.jsx => SidebarContextProvider.tsx} (76%) rename src/courseware/course/sidebar/hooks/{index.js => index.ts} (100%) rename src/courseware/course/sidebar/hooks/{useInitialSidebar.js => useInitialSidebar.ts} (78%) rename src/courseware/course/sidebar/hooks/{useResponsiveBehavior.js => useResponsiveBehavior.ts} (71%) rename src/courseware/course/sidebar/hooks/{useSidebarSync.js => useSidebarSync.ts} (82%) rename src/courseware/course/sidebar/hooks/{useUnitShiftBehavior.js => useUnitShiftBehavior.ts} (84%) create mode 100644 src/widgets/discussions/DiscussionsProvider.test.tsx create mode 100644 src/widgets/discussions/DiscussionsProvider.tsx delete mode 100644 src/widgets/discussions/widgetConfig.js delete mode 100644 src/widgets/discussions/widgetConfig.test.ts create mode 100644 src/widgets/discussions/widgetConfig.ts rename src/widgets/upgrade/src/{UpgradeWidgetContext.jsx => UpgradeWidgetContext.tsx} (57%) delete mode 100644 src/widgets/upgrade/src/utils.js create mode 100644 src/widgets/upgrade/src/utils.ts rename src/widgets/upgrade/src/{widgetConfig.js => widgetConfig.ts} (73%) diff --git a/src/courseware/course/sequence/lock-paywall/LockPaywall.test.jsx b/src/courseware/course/sequence/lock-paywall/LockPaywall.test.jsx index 18c725b8b4..c8dee0cd11 100644 --- a/src/courseware/course/sequence/lock-paywall/LockPaywall.test.jsx +++ b/src/courseware/course/sequence/lock-paywall/LockPaywall.test.jsx @@ -6,6 +6,7 @@ import { fireEvent, getTestStoreIds, initializeTestStore, render, screen, } from '../../../../setupTest'; import MountCourseQueryHooks from '../../../../tests/MountCourseQueryHooks'; +import SidebarContext from '../../sidebar/SidebarContext'; import LockPaywall from './LockPaywall'; jest.mock('@edx/frontend-platform/analytics'); @@ -13,6 +14,7 @@ jest.mock('@edx/frontend-platform/analytics'); describe('Lock Paywall', () => { let store; const mockData = { currentSidebar: null }; + const sidebarContextValue = { currentSidebar: null, availableSidebarIds: [] }; beforeAll(async () => { store = await initializeTestStore(); @@ -22,10 +24,10 @@ describe('Lock Paywall', () => { }); const renderPaywall = (props, options) => render( - <> + - , + , options, ); diff --git a/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.test.jsx b/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.test.jsx index 4017e3b5e9..92e1662e6e 100644 --- a/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.test.jsx +++ b/src/courseware/course/sequence/sequence-navigation/SequenceNavigation.test.jsx @@ -5,6 +5,7 @@ import { render, screen, fireEvent, getByText, getTestStoreIds, initializeTestStore, } from '../../../../setupTest'; import MountCourseQueryHooks from '../../../../tests/MountCourseQueryHooks'; +import SidebarContext from '../../sidebar/SidebarContext'; import SequenceNavigation from './SequenceNavigation'; import useIndexOfLastVisibleChild from '../../../../generic/tabs/useIndexOfLastVisibleChild'; @@ -32,6 +33,8 @@ describe('Sequence Navigation', () => { }; }); + const sidebarContextValue = { currentSidebar: null, availableSidebarIds: [] }; + const renderNav = (props = {}, { store } = {}) => { const sequenceId = props.sequenceId ?? mockData.sequenceId; return render( @@ -40,10 +43,10 @@ describe('Sequence Navigation', () => { + - + )} /> diff --git a/src/courseware/course/sequence/sequence-navigation/SequenceNavigationTabs.test.jsx b/src/courseware/course/sequence/sequence-navigation/SequenceNavigationTabs.test.jsx index 22631d9040..77ae0dc916 100644 --- a/src/courseware/course/sequence/sequence-navigation/SequenceNavigationTabs.test.jsx +++ b/src/courseware/course/sequence/sequence-navigation/SequenceNavigationTabs.test.jsx @@ -4,6 +4,7 @@ import { getAllByRole } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { initializeTestStore, render, screen } from '../../../../setupTest'; +import SidebarContext from '../../sidebar/SidebarContext'; import SequenceNavigationTabs from './SequenceNavigationTabs'; import useIndexOfLastVisibleChild from '../../../../generic/tabs/useIndexOfLastVisibleChild'; @@ -40,9 +41,18 @@ describe('Sequence Navigation Tabs', () => { }; }); + const sidebarContextValue = { currentSidebar: null, availableSidebarIds: [] }; + + const renderTabs = () => render( + + + , + { wrapWithRouter: true }, + ); + it('renders unit buttons', () => { useIndexOfLastVisibleChild.mockReturnValue([0, null, null]); - render(, { wrapWithRouter: true }); + renderTabs(); expect(screen.getAllByRole('link')).toHaveLength(unitBlocks.length); }); @@ -51,7 +61,7 @@ describe('Sequence Navigation Tabs', () => { let container = null; useIndexOfLastVisibleChild.mockReturnValue([-1, null, null]); - const booyah = render(, { wrapWithRouter: true }); + const booyah = renderTabs(); // wait for links to appear so we aren't testing an empty div await screen.findAllByRole('link'); diff --git a/src/courseware/course/sidebar/ARCHITECTURE.md b/src/courseware/course/sidebar/ARCHITECTURE.md index 7e98380e79..12b38b6ba7 100644 --- a/src/courseware/course/sidebar/ARCHITECTURE.md +++ b/src/courseware/course/sidebar/ARCHITECTURE.md @@ -159,32 +159,35 @@ _Example with built-in widgets:_ - Manage `currentSidebar` state (shared by both sidebars) - Handle unit shift logic for RIGHT sidebar panels - Provide context to both left and right sidebar components -- **Prefetch widget data** after mount via `widget.prefetch` (sync logic re-evaluates availability once data arrives) +- **Mount each widget's `Provider`** around the sidebar children, whether or not the widget is available (sync logic re-evaluates availability once its data arrives) -### Widget Prefetch Lifecycle +### Widget Data Lifecycle -Widgets can define a `prefetch` function in their config to pre-load data that their `isAvailable` or render logic depends on. The framework calls `prefetch` for every enabled widget on mount (and when `courseId` or the widget list changes): +A widget whose `isAvailable` depends on fetched data loads it in its `Provider` as a React Query observer gated with `enabled`. The framework mounts every enabled widget's `Provider` inside `SidebarContext`, so the observer runs before the widget is available and independently of whether its trigger or panel render: ```javascript -// In SidebarContextProvider.jsx -const courseMetaRef = useRef(null); -courseMetaRef.current = { ...coursewareMeta, ...courseHomeMeta }; - -useEffect(() => { - enabledWidgets.forEach(widget => { - if (widget.prefetch) { - widget.prefetch({ courseId, course: courseMetaRef.current, queryClient }); - } +// In SidebarContextProvider.tsx +const renderWithWidgetProviders = useCallback((content) => enabledWidgets + .reduceRight((acc, { Provider }) => (Provider ? {acc} : acc), content), [enabledWidgets]); + +// In widgets/discussions/DiscussionsProvider.tsx +const DiscussionsProvider = ({ children }) => { + const { courseId } = useSidebar(); + const tabs = useCourseHomeMeta(courseId, { enabled: false }).data?.tabs; + useQuery({ + ...discussionTopicsQuery(courseId), + enabled: !!getConfig().DISCUSSIONS_MFE_BASE_URL && hasDiscussionTab(tabs), }); -}, [enabledWidgets, courseId, queryClient]); + return <>{children}; +}; ``` -`courseMetaRef` is updated on every render so the effect always reads the latest `coursewareMeta` + `courseHomeMeta` values without `coursewareMeta`/`courseHomeMeta` being reactive dependencies. This means the effect fires once per `courseId` change rather than on every model reference update. +An observer fetches when it mounts and when its query key changes, not when the metadata it reads re-renders, so one sidebar mount is one request even as `courseHomeMeta` updates (celebration writes, refetches). -**Why prefetch lives in the provider, not in individual components:** -- Starts data loading post-mount so the sync logic can re-evaluate availability once the data arrives -- Individual Trigger/Sidebar components can remain pure render components -- Centralises fetch orchestration in one place +**Why data loading lives in the widget's `Provider`, not in its Trigger/Sidebar components:** +- The trigger is mounted only when the widget is available, and availability depends on the data — a component cannot load the data its own mounting waits for +- Starts loading post-mount so the sync logic can re-evaluate availability once the data arrives +- Individual Trigger/Sidebar components remain pure render components **Key Logic:** ```javascript diff --git a/src/courseware/course/sidebar/README.md b/src/courseware/course/sidebar/README.md index 2841ee360e..9c7ab63729 100644 --- a/src/courseware/course/sidebar/README.md +++ b/src/courseware/course/sidebar/README.md @@ -13,28 +13,27 @@ Widget implementations: ### Widget Structure -Each widget must provide: - -```javascript -{ - id: string, // Unique identifier (e.g., 'DISCUSSIONS', 'CUSTOM_TOOL') - priority: number, // Display order (lower = first, default: 50) - Sidebar: ReactComponent, // Main panel component - Trigger: ReactComponent, // Trigger button component - isAvailable: (context) => boolean, // Optional: check if widget should be shown - prefetch: ({ courseId, course, queryClient }) => void, // Optional: pre-load data (runs post-mount; sync logic re-evaluates availability) - enabled: boolean, // Whether widget is enabled - Provider?: ReactComponent, // Optional: React Provider for Panel↔Trigger shared state -} -``` +Each widget is a `SidebarWidget`, declared in [`SidebarContext.ts`](SidebarContext.ts). ### The `Provider` field -An optional hook point for widgets that need to share React state between their `Sidebar` and `Trigger` components. The widget owns the full Provider implementation. The framework simply mounts it. +An optional component the widget supplies, taking `children`. `SidebarContextProvider` wraps all children in each registered widget's `Provider` (in reverse-priority order), whether or not the widget is currently available, and the Provider can read `courseId` etc. from `SidebarContext` since it mounts inside it. That gives the widget one component that is mounted for as long as the sidebar is, where it can run hooks and hold state that its `Sidebar` and `Trigger` both read. The two built-in widgets use it for the two things it is for: -`SidebarContextProvider` wraps all children in each registered widget's `Provider` (in reverse-priority order), so both components have access to the same widget-level context. The Provider itself can safely read `courseId` etc. from `SidebarContext` since it mounts inside it. +- **Shared state.** The upgrade widget's `UpgradeWidgetProvider` keeps the seen/unseen status and the upgrade stage in a context of its own, which `UpgradeTrigger` and `UpgradePanel` read. +- **Loading the data `isAvailable` depends on.** A widget's trigger is mounted only once the widget is available, so a fetch the availability check needs cannot live in the trigger. The discussions widget's `DiscussionsProvider` runs one `useQuery`, with the conditions for fetching in `enabled`, and renders its children unchanged. The request goes out once when the sidebar mounts; the first availability check runs before the data arrives, and when the query resolves the framework re-evaluates availability and the trigger appears. -No built-in widgets use this field — it exists as a generic extension point for custom widgets that need cross-component coordination without polluting `SidebarContext`. +```javascript +// widgets/discussions/DiscussionsProvider.tsx +const DiscussionsProvider = ({ children }) => { + const { courseId } = useSidebar(); + const tabs = useCourseHomeMeta(courseId, { enabled: false }).data?.tabs; + useQuery({ + ...discussionTopicsQuery(courseId), + enabled: !!getConfig().DISCUSSIONS_MFE_BASE_URL && hasDiscussionTab(tabs), + }); + return <>{children}; +}; +``` ```javascript // In your widget's widgetConfig.js @@ -42,46 +41,15 @@ export const myWidgetConfig = { id: 'MY_WIDGET', Sidebar: MyWidgetPanel, Trigger: MyWidgetTrigger, - Provider: MyWidgetProvider, // optional — omit if Sidebar/Trigger don't share state + Provider: MyWidgetProvider, // optional — omit if the widget has no shared state and no data to load isAvailable: ({ course }) => !!course?.someField, enabled: true, }; ``` -### The `prefetch` field - -An optional function called by `SidebarContextProvider` after mount (and when `courseId` or the widget list changes). Use it to prefetch a React Query query (via the `queryClient` argument) or otherwise fetch data that `isAvailable`, `Trigger`, or `Sidebar` depend on. The `course` argument always reflects the latest `coursewareMeta` + `courseHomeMeta` values at the time the effect fires. Because this runs post-mount, it does not guarantee the data is present for the initial render-time availability check; widgets that depend on prefetched data may become available after the store updates and the framework sync logic re-evaluates availability. - -```javascript -export const myWidgetPrefetch = ({ courseId, course, queryClient }) => { - if (course?.someCondition) { - queryClient.query(myWidgetDataQuery(courseId)).catch(() => {}); - } -}; - -export const myWidgetConfig = { - id: 'MY_WIDGET', - // ... - prefetch: myWidgetPrefetch, -}; -``` - -The `course` object is a merged view of the courseware metadata (`coursewareMeta`) and the course-home metadata. - ### Context Object -The `isAvailable` function receives a context object with: - -```javascript -{ - courseId: string, - unitId: string, - course: object, // Merged coursewareMeta + courseHomeMeta (verifiedMode, enrollmentMode, courseModes, …) - unit: object, // discussionTopics model for the current unit (id, enabledInContext, …) -} -``` - -Widgets pick whatever they need from `course` or `unit` — the sidebar makes no assumptions about which fields any given widget requires. +The `isAvailable` function receives a `SidebarWidgetContext`, declared in [`SidebarContext.ts`](SidebarContext.ts). Widgets pick whatever they need from its `course` or `unit` — the sidebar makes no assumptions about which fields any given widget requires. ## Adding Widgets @@ -123,16 +91,15 @@ export default { The main panel component that renders when the widget is active. Wrap your content in `SidebarBase` to get the standard close button, fullscreen handling, and show/hide behaviour: ```javascript -import { useContext } from 'react'; import { useIntl } from '@edx/frontend-platform/i18n'; import SidebarBase from '@src/courseware/course/sidebar/common/SidebarBase'; -import SidebarContext from '@src/courseware/course/sidebar/SidebarContext'; +import { useSidebar } from '@src/courseware/course/sidebar/SidebarContext'; export const ID = 'MY_WIDGET'; const MySidebar = () => { const intl = useIntl(); - const { courseId } = useContext(SidebarContext); + const { courseId } = useSidebar(); return ( void} toggleSidebar - Function to toggle sidebar - * @property {boolean} shouldDisplaySidebarOpen - Whether sidebar should be open (desktop) - * @property {boolean} shouldDisplayFullScreen - Whether in mobile/fullscreen view - * @property {string} courseId - Current course ID - * @property {string} unitId - Current unit ID - * @property {Object.} SIDEBARS - * - Registry of available sidebar widgets - * @property {Array} SIDEBAR_ORDER - Ordered list of widget IDs by priority - */ - -/** - * Sidebar Context - * @type {React.Context} - */ -const SidebarContext = React.createContext({ - currentSidebar: null, - initialSidebar: null, - toggleSidebar: () => {}, - shouldDisplaySidebarOpen: false, - shouldDisplayFullScreen: false, - courseId: '', - unitId: '', - SIDEBARS: {}, - SIDEBAR_ORDER: [], - availableSidebarIds: [], -}); - -export default SidebarContext; diff --git a/src/courseware/course/sidebar/SidebarContext.test.tsx b/src/courseware/course/sidebar/SidebarContext.test.tsx new file mode 100644 index 0000000000..e641ccdb93 --- /dev/null +++ b/src/courseware/course/sidebar/SidebarContext.test.tsx @@ -0,0 +1,10 @@ +import { renderHook } from '@testing-library/react'; + +import { useSidebar } from './SidebarContext'; + +describe('useSidebar', () => { + it('throws outside a SidebarProvider', () => { + expect(() => renderHook(() => useSidebar())) + .toThrow('useSidebar must be used within a SidebarProvider'); + }); +}); diff --git a/src/courseware/course/sidebar/SidebarContext.ts b/src/courseware/course/sidebar/SidebarContext.ts new file mode 100644 index 0000000000..9237702bd4 --- /dev/null +++ b/src/courseware/course/sidebar/SidebarContext.ts @@ -0,0 +1,55 @@ +import { + createContext, useContext, type ComponentType, type ReactNode, +} from 'react'; + +import type { CourseHomeMeta } from '@src/course-home/data/apiHooks'; +import type { CoursewareMeta, DiscussionTopic } from '@src/courseware/data/apiHooks'; + +export interface SidebarWidgetContext { + courseId: string; + unitId: string; + course: CourseHomeMeta & CoursewareMeta; + unit: Partial; +} + +export interface SidebarWidget { + id: string; + priority: number; + Sidebar: ComponentType; + Trigger: ComponentType<{ onClick: () => void }>; + Provider?: ComponentType<{ children: ReactNode }>; + isAvailable?: (context: SidebarWidgetContext) => boolean; + enabled?: boolean; +} + +export interface SidebarRegistryEntry { + ID: string; + Sidebar: ComponentType; + Trigger: ComponentType<{ onClick: () => void }>; + isAvailable?: (context: SidebarWidgetContext) => boolean; +} + +export interface SidebarContextValue { + currentSidebar: string | null; + initialSidebar: string | null; + toggleSidebar: (sidebarId: string) => void; + shouldDisplaySidebarOpen: boolean; + shouldDisplayFullScreen: boolean; + courseId: string; + unitId: string; + SIDEBARS: Record; + SIDEBAR_ORDER: string[]; + availableSidebarIds: string[]; +} + +const SidebarContext = createContext(null); + +export const useSidebar = (): SidebarContextValue => { + const context = useContext(SidebarContext); + if (!context) { + throw new Error('useSidebar must be used within a SidebarProvider'); + } + return context; +}; + +export default SidebarContext; diff --git a/src/courseware/course/sidebar/SidebarContextProvider.test.jsx b/src/courseware/course/sidebar/SidebarContextProvider.test.jsx index e95b0df76b..c7a9814763 100644 --- a/src/courseware/course/sidebar/SidebarContextProvider.test.jsx +++ b/src/courseware/course/sidebar/SidebarContextProvider.test.jsx @@ -1,6 +1,5 @@ import React, { useContext } from 'react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; -import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { render, screen, fireEvent, act, } from '@testing-library/react'; @@ -85,13 +84,11 @@ const ContextConsumer = () => { function renderProvider(props = {}) { return render( - - - - - - - + + + + + , ); } @@ -145,6 +142,22 @@ describe('SidebarContextProvider', () => { expect(availableIds).toContain('DISCUSSIONS'); expect(availableIds).not.toContain('UNAVAILABLE_WIDGET'); }); + + it('treats a widget without isAvailable as always available', () => { + const { getEnabledWidgets } = jest.requireMock('./defaultWidgets'); + getEnabledWidgets.mockReturnValueOnce([ + { + id: 'ALWAYS_ON', + priority: 10, + Sidebar: () => null, + Trigger: () => null, + enabled: true, + }, + ]); + renderProvider(); + + expect(screen.getByTestId('available-ids').textContent).toBe('ALWAYS_ON'); + }); }); describe('Use Case 7: Manual toggle interactions', () => { diff --git a/src/courseware/course/sidebar/SidebarContextProvider.jsx b/src/courseware/course/sidebar/SidebarContextProvider.tsx similarity index 76% rename from src/courseware/course/sidebar/SidebarContextProvider.jsx rename to src/courseware/course/sidebar/SidebarContextProvider.tsx index 5a9ca0d984..d3705bad96 100644 --- a/src/courseware/course/sidebar/SidebarContextProvider.jsx +++ b/src/courseware/course/sidebar/SidebarContextProvider.tsx @@ -1,15 +1,13 @@ import { breakpoints, useWindowSize } from '@openedx/paragon'; -import PropTypes from 'prop-types'; -import { useQueryClient } from '@tanstack/react-query'; import { - useState, useMemo, useCallback, useRef, useEffect, + useState, useMemo, useCallback, useRef, type ReactNode, } from 'react'; import { useSearchParams } from 'react-router-dom'; import { useCourseHomeMeta } from '@src/course-home/data/apiHooks'; import { useModel } from '@src/generic/model-store'; -import SidebarContext from './SidebarContext'; +import SidebarContext, { type SidebarContextValue, type SidebarWidget } from './SidebarContext'; import { getEnabledWidgets, buildSidebarsRegistry, @@ -26,23 +24,28 @@ import { useResponsiveBehavior, } from './hooks'; +interface Props { + courseId: string; + unitId: string; + children?: ReactNode; +} + const SidebarProvider = ({ courseId, unitId, children, -}) => { +}: Props) => { const courseHomeMeta = useCourseHomeMeta(courseId, { enabled: false }).data; const coursewareMeta = useModel('coursewareMeta', courseId); const unit = useModel('discussionTopics', unitId); - const queryClient = useQueryClient(); - const { width } = useWindowSize(); - const shouldDisplayFullScreen = width < breakpoints.extraLarge.minWidth; - const shouldDisplaySidebarOpen = width > breakpoints.extraLarge.minWidth; + const width = useWindowSize().width ?? window.innerWidth; + const shouldDisplayFullScreen = width < breakpoints.extraLarge.minWidth!; + const shouldDisplaySidebarOpen = width > breakpoints.extraLarge.minWidth!; const [searchParams] = useSearchParams(); const isInitiallySidebarOpen = shouldDisplaySidebarOpen || searchParams.get('sidebar') === 'true'; // Build registry of enabled widgets - const enabledWidgets = useMemo(() => getEnabledWidgets(), []); + const enabledWidgets = useMemo(() => getEnabledWidgets(), []); const SIDEBARS = useMemo(() => buildSidebarsRegistry(enabledWidgets), [enabledWidgets]); const SIDEBAR_ORDER = useMemo(() => getSidebarOrder(enabledWidgets), [enabledWidgets]); @@ -82,22 +85,11 @@ const SidebarProvider = ({ // Track if user has manually toggled sidebar within current unit const hasUserToggledRef = useRef(false); - const previousUnitIdRef = useRef(null); // Start null so first render triggers unit shift logic + const previousUnitIdRef = useRef(null); // Start null so first render triggers unit shift logic // Track which unit set COURSE_OUTLINE (to prevent immediate switching) - const courseOutlineSetByUnitRef = useRef(null); + const courseOutlineSetByUnitRef = useRef(null); // Track if this is initial page load (to allow data loading switches) const isInitialLoadRef = useRef(true); - const courseMetaRef = useRef(null); - courseMetaRef.current = { ...coursewareMeta, ...courseHomeMeta }; - - // Prefetch widget data - useEffect(() => { - enabledWidgets.forEach(widget => { - if (widget.prefetch) { - widget.prefetch({ courseId, course: courseMetaRef.current, queryClient }); - } - }); - }, [enabledWidgets, courseId, queryClient]); // Apply unit navigation behavior useUnitShiftBehavior({ @@ -141,7 +133,7 @@ const SidebarProvider = ({ [getAvailableWidgets], ); - const toggleSidebar = useCallback((sidebarId) => { + const toggleSidebar = useCallback((sidebarId: string) => { // Mark that user has manually interacted with sidebar hasUserToggledRef.current = true; @@ -152,7 +144,7 @@ const SidebarProvider = ({ setSidebarClosedByUser(newSidebar === null); }, [currentSidebar, courseId]); - const contextValue = useMemo(() => ({ + const contextValue = useMemo(() => ({ initialSidebar, toggleSidebar, currentSidebar, @@ -175,12 +167,8 @@ const SidebarProvider = ({ SIDEBAR_ORDER, availableSidebarIds, ]); - const renderWithWidgetProviders = useCallback((content) => enabledWidgets - .filter(w => w.Provider) - .reduceRight((acc, widget) => { - const { Provider } = widget; - return {acc}; - }, content), [enabledWidgets]); + const renderWithWidgetProviders = useCallback((content: ReactNode) => enabledWidgets + .reduceRight((acc, { Provider }) => (Provider ? {acc} : acc), content), [enabledWidgets]); return ( @@ -189,14 +177,4 @@ const SidebarProvider = ({ ); }; -SidebarProvider.propTypes = { - courseId: PropTypes.string.isRequired, - unitId: PropTypes.string.isRequired, - children: PropTypes.node, -}; - -SidebarProvider.defaultProps = { - children: null, -}; - export default SidebarProvider; diff --git a/src/courseware/course/sidebar/USE_CASE_VERIFICATION.md b/src/courseware/course/sidebar/USE_CASE_VERIFICATION.md index d50378bbdd..55f6d61bf4 100644 --- a/src/courseware/course/sidebar/USE_CASE_VERIFICATION.md +++ b/src/courseware/course/sidebar/USE_CASE_VERIFICATION.md @@ -149,9 +149,9 @@ The system always follows priority cascade logic: │ │ │ ┌───────────────────────────────────────────────┐ │ │ │ Effects: │ │ -│ │ 0. Prefetch Effect │ │ -│ │ - Calls widget.prefetch() for each widget │ │ -│ │ - Runs post-mount to warm widget data │ │ +│ │ 0. Widget Providers │ │ +│ │ - Each widget's Provider loads its data │ │ +│ │ as a React Query observer │ │ │ │ - Influences subsequent availability/sync │ │ │ │ behavior │ │ │ │ │ │ diff --git a/src/courseware/course/sidebar/hooks/index.js b/src/courseware/course/sidebar/hooks/index.ts similarity index 100% rename from src/courseware/course/sidebar/hooks/index.js rename to src/courseware/course/sidebar/hooks/index.ts diff --git a/src/courseware/course/sidebar/hooks/useInitialSidebar.js b/src/courseware/course/sidebar/hooks/useInitialSidebar.ts similarity index 78% rename from src/courseware/course/sidebar/hooks/useInitialSidebar.js rename to src/courseware/course/sidebar/hooks/useInitialSidebar.ts index 42d5bcb1d0..ec66de3eec 100644 --- a/src/courseware/course/sidebar/hooks/useInitialSidebar.js +++ b/src/courseware/course/sidebar/hooks/useInitialSidebar.ts @@ -4,6 +4,15 @@ import { getSidebarId, isSidebarClosedByUser, } from '../utils/storage'; +import type { SidebarWidget } from '../SidebarContext'; + +interface Params { + courseId: string; + shouldDisplayFullScreen: boolean; + isInitiallySidebarOpen: boolean; + getFirstAvailablePanel: () => string | null; + getAvailableWidgets: () => SidebarWidget[]; +} /** * Calculate initial sidebar based on screen size and available widgets @@ -11,16 +20,6 @@ import { * Manages ALL panels: DISCUSSIONS, UPGRADE, and COURSE_OUTLINE * DESKTOP (>1200px): Auto-opens panels with priority cascade * MOBILE (<1200px): Respects localStorage, no auto-open - * - * @param {Object} params - * @param {string} params.courseId - Current course ID - * @param {boolean} params.shouldDisplayFullScreen - Whether in mobile view - * @param {boolean} params.isInitiallySidebarOpen - Whether the viewport / URL - * permits auto-opening the sidebar on initial render. False on viewports - * below the extra-large breakpoint unless the URL has `?sidebar=true`. - * @param {Function} params.getFirstAvailablePanel - Get first available widget - * @param {Function} params.getAvailableWidgets - Get all available widgets - * @returns {string|null} Initial sidebar ID */ export function useInitialSidebar({ courseId, @@ -28,8 +27,8 @@ export function useInitialSidebar({ isInitiallySidebarOpen, getFirstAvailablePanel, getAvailableWidgets, -}) { - return useMemo(() => { +}: Params) { + return useMemo(() => { // MOBILE: Use stored value or null (no auto-open) if (shouldDisplayFullScreen) { return isSidebarClosedByUser() ? null : getSidebarId(courseId); diff --git a/src/courseware/course/sidebar/hooks/useResponsiveBehavior.js b/src/courseware/course/sidebar/hooks/useResponsiveBehavior.ts similarity index 71% rename from src/courseware/course/sidebar/hooks/useResponsiveBehavior.js rename to src/courseware/course/sidebar/hooks/useResponsiveBehavior.ts index dca2699366..107b086465 100644 --- a/src/courseware/course/sidebar/hooks/useResponsiveBehavior.js +++ b/src/courseware/course/sidebar/hooks/useResponsiveBehavior.ts @@ -1,23 +1,24 @@ -import { useEffect } from 'react'; +import { useEffect, type MutableRefObject } from 'react'; import { WIDGETS } from '@src/constants'; import { setSidebarId, isSidebarClosedByUser, } from '../utils/storage'; +interface Params { + shouldDisplaySidebarOpen: boolean; + currentSidebar: string | null; + setCurrentSidebar: (sidebarId: string | null) => void; + courseId: string; + hasUserToggledRef: MutableRefObject; +} + /** * Handle sidebar behavior when window resizes between mobile/desktop * * When resizing to desktop and no sidebar open, recover to COURSE_OUTLINE (the default). * * Respects user actions: Only applies auto-behavior if user hasn't manually toggled. - * - * @param {Object} params - * @param {boolean} params.shouldDisplaySidebarOpen - Whether sidebar can be open - * @param {string|null} params.currentSidebar - Currently active sidebar - * @param {Function} params.setCurrentSidebar - Update current sidebar state - * @param {string} params.courseId - Current course ID - * @param {Function} params.hasUserToggledRef - Ref tracking user manual toggles */ export function useResponsiveBehavior({ shouldDisplaySidebarOpen, @@ -25,7 +26,7 @@ export function useResponsiveBehavior({ setCurrentSidebar, courseId, hasUserToggledRef, -}) { +}: Params) { useEffect(() => { // Skip if user has manually toggled within current unit (respect user action) if (hasUserToggledRef.current) { diff --git a/src/courseware/course/sidebar/hooks/useSidebarSync.js b/src/courseware/course/sidebar/hooks/useSidebarSync.ts similarity index 82% rename from src/courseware/course/sidebar/hooks/useSidebarSync.js rename to src/courseware/course/sidebar/hooks/useSidebarSync.ts index 7571776b7a..b1fb1e8bba 100644 --- a/src/courseware/course/sidebar/hooks/useSidebarSync.js +++ b/src/courseware/course/sidebar/hooks/useSidebarSync.ts @@ -1,26 +1,27 @@ -import { useEffect } from 'react'; +import { useEffect, type MutableRefObject } from 'react'; import { WIDGETS } from '@src/constants'; import { setSidebarId, isSidebarClosedByUser, } from '../utils/storage'; +interface Params { + initialSidebar: string | null; + currentSidebar: string | null; + setCurrentSidebar: (sidebarId: string | null) => void; + courseId: string; + unitId: string; + shouldDisplayFullScreen: boolean; + hasUserToggledRef: MutableRefObject; + courseOutlineSetByUnitRef: MutableRefObject; +} + /** * Sync currentSidebar with initialSidebar when async data loads * * sync currentSidebar with the updated initialSidebar (priority cascade). * * Respects user actions: Only syncs if user hasn't manually toggled in current unit. - * - * @param {Object} params - * @param {string|null} params.initialSidebar - Calculated initial sidebar - * @param {string|null} params.currentSidebar - Currently active sidebar - * @param {Function} params.setCurrentSidebar - Update current sidebar state - * @param {string} params.courseId - Current course ID - * @param {string} params.unitId - Current unit ID - * @param {boolean} params.shouldDisplayFullScreen - Whether in mobile view - * @param {Function} params.hasUserToggledRef - Ref tracking user manual toggles - * @param {Function} params.courseOutlineSetByUnitRef - Ref tracking COURSE_OUTLINE auto-set */ export function useSidebarSync({ initialSidebar, @@ -31,7 +32,7 @@ export function useSidebarSync({ shouldDisplayFullScreen, hasUserToggledRef, courseOutlineSetByUnitRef, -}) { +}: Params) { useEffect(() => { // Skip if on mobile if (shouldDisplayFullScreen) { diff --git a/src/courseware/course/sidebar/hooks/useUnitShiftBehavior.js b/src/courseware/course/sidebar/hooks/useUnitShiftBehavior.ts similarity index 84% rename from src/courseware/course/sidebar/hooks/useUnitShiftBehavior.js rename to src/courseware/course/sidebar/hooks/useUnitShiftBehavior.ts index b703fc7631..2a1fefc301 100644 --- a/src/courseware/course/sidebar/hooks/useUnitShiftBehavior.js +++ b/src/courseware/course/sidebar/hooks/useUnitShiftBehavior.ts @@ -1,9 +1,25 @@ -import { useEffect } from 'react'; +import { useEffect, type MutableRefObject } from 'react'; import { WIDGETS } from '@src/constants'; import { setSidebarId, isSidebarClosedByUser, } from '../utils/storage'; +import type { SidebarWidget } from '../SidebarContext'; + +interface Params { + unitId: string; + currentSidebar: string | null; + setCurrentSidebar: (sidebarId: string | null) => void; + getFirstAvailablePanel: () => string | null; + getAvailableWidgets: () => SidebarWidget[]; + courseId: string; + shouldDisplayFullScreen: boolean; + shouldDisplaySidebarOpen: boolean; + hasUserToggledRef: MutableRefObject; + previousUnitIdRef: MutableRefObject; + courseOutlineSetByUnitRef: MutableRefObject; + isInitialLoadRef: MutableRefObject; +} /** * Handle sidebar behavior when navigating between units @@ -15,20 +31,6 @@ import { * 3. RIGHT panels available → Apply priority cascade / fallback when current is stale; * on a wide viewport with nothing open, recover to COURSE_OUTLINE (default); * on a narrow viewport with nothing open, preserve null - * - * @param {Object} params - * @param {string} params.unitId - Current unit ID - * @param {string|null} params.currentSidebar - Currently active sidebar - * @param {Function} params.setCurrentSidebar - Update current sidebar state - * @param {Function} params.getFirstAvailablePanel - Get first available widget - * @param {Function} params.getAvailableWidgets - Get all available widgets - * @param {string} params.courseId - Current course ID - * @param {boolean} params.shouldDisplayFullScreen - Whether in mobile view - * @param {boolean} params.shouldDisplaySidebarOpen - Whether sidebar can be open - * @param {Object} params.hasUserToggledRef - Ref tracking user manual toggles - * @param {Object} params.previousUnitIdRef - Ref tracking previous unit ID - * @param {Object} params.courseOutlineSetByUnitRef - Ref tracking COURSE_OUTLINE auto-set - * @param {Object} params.isInitialLoadRef - Ref tracking if this is initial load */ export function useUnitShiftBehavior({ unitId, @@ -43,7 +45,7 @@ export function useUnitShiftBehavior({ previousUnitIdRef, courseOutlineSetByUnitRef, isInitialLoadRef, -}) { +}: Params) { useEffect(() => { // Detect unit change if (previousUnitIdRef.current !== unitId) { diff --git a/src/courseware/course/test-utils.jsx b/src/courseware/course/test-utils.jsx index 0c478508d7..6d4d759545 100644 --- a/src/courseware/course/test-utils.jsx +++ b/src/courseware/course/test-utils.jsx @@ -10,7 +10,7 @@ import { import SidebarContext from '@src/courseware/course/sidebar/SidebarContext'; import MountCourseQueryHooks from '@src/tests/MountCourseQueryHooks'; import { buildTopicsFromUnits } from '../data/__factories__/discussionTopics.factory'; -import { prefetchDiscussionTopics } from '../data/apiHooks'; +import { discussionTopicsQuery } from '../data/apiHooks'; import Course from './Course'; const mockData = { @@ -19,7 +19,7 @@ const mockData = { unitNavigationHandler: () => {}, }; -// Seed the discussionTopics model through the real prefetch path, against +// Seed the discussionTopics model through the real query, against // temporary mocks of the two discussion endpoints. const seedDiscussionTopics = async (testStore, courseId, enabledInContext) => { const axiosMock = new MockAdapter(getAuthenticatedHttpClient()); @@ -28,7 +28,7 @@ const seedDiscussionTopics = async (testStore, courseId, enabledInContext) => { axiosMock.onGet(`${getConfig().LMS_BASE_URL}/api/discussion/v2/course_topics/${courseId}`) .reply(200, topicsResponse); - await prefetchDiscussionTopics(createTestQueryClient(testStore), courseId); + await createTestQueryClient(testStore).query(discussionTopicsQuery(courseId)); axiosMock.restore(); // put the previous adapter back }; diff --git a/src/courseware/data/apiHooks.test.tsx b/src/courseware/data/apiHooks.test.tsx index 7e2762e957..53e584549d 100644 --- a/src/courseware/data/apiHooks.test.tsx +++ b/src/courseware/data/apiHooks.test.tsx @@ -21,7 +21,7 @@ import { courseHomeQueryKeys } from '../../course-home/data/queryKeys'; import type { CourseOutlineData } from './courseOutline'; import { useCourseHomeMeta } from '../../course-home/data/apiHooks'; import { - prefetchDiscussionTopics, sequenceMightBeUnit, useCheckBlockCompletion, useCourseOutlineStructure, + discussionTopicsQuery, sequenceMightBeUnit, useCheckBlockCompletion, useCourseOutlineStructure, useCoursewareMetadata, useCoursewareOutline, useCoursewareOutlineSidebarToggles, useIsCourseLoaded, useSaveIntegritySignature, useSaveSequencePosition, useSequenceIds, useSequenceMetadata, } from './apiHooks'; @@ -428,7 +428,7 @@ describe('courseware apiHooks — useCoursewareOutlineSidebarToggles', () => { }); }); -describe('courseware apiHooks — prefetchDiscussionTopics', () => { +describe('courseware apiHooks — discussionTopicsQuery', () => { const courseMetadata = Factory.build('courseMetadata'); const courseId = courseMetadata.id; const configUrl = `${getConfig().LMS_BASE_URL}/api/discussion/v1/courses/${courseId}`; @@ -454,7 +454,7 @@ describe('courseware apiHooks — prefetchDiscussionTopics', () => { { id: 'course-wide-topic', usage_key: null, enabled_in_context: true }, ]); - await prefetchDiscussionTopics(createTestQueryClient(store), courseId); + await createTestQueryClient(store).query(discussionTopicsQuery(courseId)); expect(discussionTopicModels()).toEqual({ 'unit-1': { id: 'topic-1', usageKey: 'unit-1', enabledInContext: true }, @@ -465,7 +465,7 @@ describe('courseware apiHooks — prefetchDiscussionTopics', () => { it('skips the topics request entirely for a legacy provider', async () => { axiosMock.onGet(configUrl).reply(200, { provider: 'legacy' }); - await prefetchDiscussionTopics(createTestQueryClient(store), courseId); + await createTestQueryClient(store).query(discussionTopicsQuery(courseId)); expect(axiosMock.history.get.map(request => request.url)).toEqual([configUrl]); expect(discussionTopicModels()).toBeUndefined(); @@ -474,7 +474,7 @@ describe('courseware apiHooks — prefetchDiscussionTopics', () => { it('logs the error and writes nothing when the config request fails', async () => { axiosMock.onGet(configUrl).networkError(); - await prefetchDiscussionTopics(createTestQueryClient(store), courseId); + await expect(createTestQueryClient(store).query(discussionTopicsQuery(courseId))).rejects.toThrow(); expect(loggingService.logError).toHaveBeenCalled(); expect(discussionTopicModels()).toBeUndefined(); diff --git a/src/courseware/data/apiHooks.ts b/src/courseware/data/apiHooks.ts index 4ee02de087..5d8224cf9d 100644 --- a/src/courseware/data/apiHooks.ts +++ b/src/courseware/data/apiHooks.ts @@ -2,7 +2,7 @@ import { useCallback, useMemo } from 'react'; import { useLocation } from 'react-router-dom'; import { logError } from '@edx/frontend-platform/logging'; import { - noop, useMutation, useQuery, useQueryClient, type QueryClient, + queryOptions, useMutation, useQuery, useQueryClient, } from '@tanstack/react-query'; import { useDispatch, useStore } from 'react-redux'; @@ -21,10 +21,15 @@ interface QueryOptions { enabled?: boolean; } +// No TypeScript reader names a field yet; #2089 adds them as its readers convert. +export interface CoursewareMeta { + [key: string]: unknown; +} + export const useCoursewareMetadata = ( courseId: string | undefined, { enabled = true }: QueryOptions = {}, -) => useQuery({ +) => useQuery({ queryKey: coursewareQueryKeys.metadata(courseId!), queryFn: () => getCourseMetadata(courseId), enabled: enabled && !!courseId, @@ -117,24 +122,32 @@ export const useCoursewareOutlineSidebarToggles = (courseId: string | undefined) staleTime: Infinity, }); -// Not a hook: the sole consumer is the widget-registry prefetch effect in -// SidebarContextProvider, which passes its own queryClient. -export const prefetchDiscussionTopics = (queryClient: QueryClient, courseId: string) => ( - queryClient.query({ - 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' }] }, - }).catch(noop) -); +// Names only the fields this repo's TypeScript readers need; the endpoint returns many more, +// left reachable as `unknown` so plugins importing this type are not limited to our list. +// The full shape is openedx-platform's to describe — a copy of it here would drift — so this +// stays partial until the platform ships types we can import. +export interface DiscussionTopic { + id: string; + usageKey: string | null; + enabledInContext: boolean; + [key: string]: unknown; +} + +// Observed by DiscussionsProvider, which owns the fetch. +export const discussionTopicsQuery = (courseId: string) => queryOptions({ + 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: DiscussionTopic[] = await getCourseTopics(courseId); + return topics.filter(topic => topic.usageKey); + }, + meta: { models: [{ modelType: 'discussionTopics', strategy: 'updateModels', idField: 'usageKey' }] }, +}); interface CheckBlockCompletionVars { courseId: string | undefined; diff --git a/src/widgets/discussions/DiscussionsProvider.test.tsx b/src/widgets/discussions/DiscussionsProvider.test.tsx new file mode 100644 index 0000000000..e135cc9073 --- /dev/null +++ b/src/widgets/discussions/DiscussionsProvider.test.tsx @@ -0,0 +1,125 @@ +import { act, render } from '@testing-library/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import MockAdapter from 'axios-mock-adapter'; +import { getConfig, mergeConfig } from '@edx/frontend-platform'; +import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; + +import { courseHomeQueryKeys } from '@src/course-home/data/queryKeys'; +import SidebarContext, { type SidebarContextValue } from '@src/courseware/course/sidebar/SidebarContext'; +import { coursewareQueryKeys } from '@src/courseware/data/queryKeys'; +import { createTestQueryClient, initializeMockApp, seedQueryData } from '@src/setupTest'; + +import DiscussionsProvider from './DiscussionsProvider'; + +initializeMockApp(); + +const courseId = 'course-v1:edX+DemoX+Demo_Course'; +const configUrl = `${getConfig().LMS_BASE_URL}/api/discussion/v1/courses/${courseId}`; +const topicsUrl = `${getConfig().LMS_BASE_URL}/api/discussion/v2/course_topics/${courseId}`; +const datesTab = { tabId: 'dates', title: 'Dates', url: 'http://localhost/dates' }; +const discussionTab = { tabId: 'discussion', title: 'Discussion', url: 'http://localhost/discussion' }; +const topicsQueryKey = coursewareQueryKeys.discussionTopics(courseId); + +const sidebarContextValue: SidebarContextValue = { + currentSidebar: null, + initialSidebar: null, + toggleSidebar: () => {}, + shouldDisplaySidebarOpen: false, + shouldDisplayFullScreen: false, + courseId, + unitId: 'unit-1', + SIDEBARS: {}, + SIDEBAR_ORDER: [], + availableSidebarIds: [], +}; + +describe('DiscussionsProvider', () => { + let axiosMock: MockAdapter; + let queryClient: QueryClient; + + const requestedUrls = () => axiosMock.history.get.map(request => request.url); + + const renderProvider = (tabs?: typeof datesTab[]) => { + if (tabs) { + seedQueryData(queryClient, courseHomeQueryKeys.metadata(courseId), { tabs }); + } + return render( + + + +
child
+ + + , + ); + }; + + beforeEach(() => { + axiosMock = new MockAdapter(getAuthenticatedHttpClient()); + queryClient = createTestQueryClient(); + mergeConfig({ DISCUSSIONS_MFE_BASE_URL: 'http://localhost:2002' }); + axiosMock.onGet(configUrl).reply(200, { provider: 'openedx' }); + axiosMock.onGet(topicsUrl).reply(200, [{ id: 'topic-1', usage_key: 'unit-1', enabled_in_context: true }]); + }); + + it('renders its children', () => { + const { getByText } = renderProvider([discussionTab]); + + expect(getByText('child')).toBeInTheDocument(); + }); + + it('loads the topics once when the course has a discussion tab', async () => { + renderProvider([datesTab, discussionTab]); + + await act(async () => { + await queryClient.getQueryCache().find({ queryKey: topicsQueryKey })!.promise; + }); + + expect(requestedUrls()).toEqual([configUrl, topicsUrl]); + expect(queryClient.getQueryData(topicsQueryKey)).toEqual([ + { id: 'topic-1', usageKey: 'unit-1', enabledInContext: true }, + ]); + }); + + it('does not load the topics when the course has no discussion tab', async () => { + renderProvider([datesTab]); + await act(async () => {}); + + expect(queryClient.getQueryState(topicsQueryKey)?.fetchStatus).toBe('idle'); + expect(requestedUrls()).toEqual([]); + }); + + it('does not load the topics before the course metadata has loaded', async () => { + renderProvider(); + await act(async () => {}); + + expect(queryClient.getQueryState(topicsQueryKey)?.fetchStatus).toBe('idle'); + expect(requestedUrls()).toEqual([]); + }); + + it('does not load the topics without a discussions MFE configured', async () => { + mergeConfig({ DISCUSSIONS_MFE_BASE_URL: '' }); + renderProvider([discussionTab]); + await act(async () => {}); + + expect(queryClient.getQueryState(topicsQueryKey)?.fetchStatus).toBe('idle'); + expect(requestedUrls()).toEqual([]); + }); + + it('does not load the topics again when the course metadata changes', async () => { + renderProvider([discussionTab]); + await act(async () => { + await queryClient.getQueryCache().find({ queryKey: topicsQueryKey })!.promise; + }); + + act(() => { + queryClient.setQueryData( + courseHomeQueryKeys.metadata(courseId), + (meta: { tabs: typeof datesTab[] }) => ({ ...meta, celebrations: { firstSection: false } }), + ); + }); + await act(async () => {}); + + expect(requestedUrls()).toEqual([configUrl, topicsUrl]); + }); +}); diff --git a/src/widgets/discussions/DiscussionsProvider.tsx b/src/widgets/discussions/DiscussionsProvider.tsx new file mode 100644 index 0000000000..5babec444a --- /dev/null +++ b/src/widgets/discussions/DiscussionsProvider.tsx @@ -0,0 +1,20 @@ +import type { ReactNode } from 'react'; +import { getConfig } from '@edx/frontend-platform'; +import { useQuery } from '@tanstack/react-query'; + +import { useCourseHomeMeta } from '@src/course-home/data/apiHooks'; +import { hasDiscussionTab } from '@src/course-tabs/utils'; +import { useSidebar } from '@src/courseware/course/sidebar/SidebarContext'; +import { discussionTopicsQuery } from '@src/courseware/data/apiHooks'; + +const DiscussionsProvider = ({ children }: { children: ReactNode }) => { + const { courseId } = useSidebar(); + const tabs = useCourseHomeMeta(courseId, { enabled: false }).data?.tabs; + useQuery({ + ...discussionTopicsQuery(courseId), + enabled: !!getConfig().DISCUSSIONS_MFE_BASE_URL && hasDiscussionTab(tabs), + }); + return <>{children}; +}; + +export default DiscussionsProvider; diff --git a/src/widgets/discussions/DiscussionsSidebar.test.jsx b/src/widgets/discussions/DiscussionsSidebar.test.jsx index 6c94c36ee0..aca01a721e 100644 --- a/src/widgets/discussions/DiscussionsSidebar.test.jsx +++ b/src/widgets/discussions/DiscussionsSidebar.test.jsx @@ -7,7 +7,7 @@ import { createTestQueryClient, initializeMockApp, getTestStoreIds, initializeTestStore, render, screen, } from '@src/setupTest'; import { buildTopicsFromUnits } from '@src/courseware/data/__factories__/discussionTopics.factory'; -import { prefetchDiscussionTopics } from '@src/courseware/data/apiHooks'; +import { discussionTopicsQuery } from '@src/courseware/data/apiHooks'; import SidebarContext from '@src/courseware/course/sidebar/SidebarContext'; import DiscussionsSidebar from './DiscussionsSidebar'; @@ -43,7 +43,7 @@ describe('Discussions Trigger', () => { ); axiosMock.onGet(`${getConfig().LMS_BASE_URL}/api/discussion/v2/course_topics/${courseId}`) .reply(200, buildTopicsFromUnits(state.models.units)); - await prefetchDiscussionTopics(createTestQueryClient(store), courseId); + await createTestQueryClient(store).query(discussionTopicsQuery(courseId)); }); function renderWithProvider(testData = {}) { diff --git a/src/widgets/discussions/DiscussionsTrigger.test.jsx b/src/widgets/discussions/DiscussionsTrigger.test.jsx index d5969a08de..027226456e 100644 --- a/src/widgets/discussions/DiscussionsTrigger.test.jsx +++ b/src/widgets/discussions/DiscussionsTrigger.test.jsx @@ -7,7 +7,7 @@ import { createTestQueryClient, fireEvent, initializeMockApp, getTestStoreIds, initializeTestStore, render, screen, } from '@src/setupTest'; import { buildTopicsFromUnits } from '@src/courseware/data/__factories__/discussionTopics.factory'; -import { prefetchDiscussionTopics } from '@src/courseware/data/apiHooks'; +import { discussionTopicsQuery } from '@src/courseware/data/apiHooks'; import SidebarContext from '@src/courseware/course/sidebar/SidebarContext'; import DiscussionsTrigger from './DiscussionsTrigger'; @@ -43,8 +43,7 @@ describe('Discussions Trigger', () => { axiosMock.onGet(`${getConfig().LMS_BASE_URL}/api/discussion/v2/course_topics/${courseId}`) .reply(200, buildTopicsFromUnits(state.models.units)); - // Pre-fetch discussion topics since prefetch is now in widgetConfig, not in the Trigger - await prefetchDiscussionTopics(createTestQueryClient(store), courseId); + await createTestQueryClient(store).query(discussionTopicsQuery(courseId)); }); const SidebarWrapper = ({ contextValue, onClick }) => ( diff --git a/src/widgets/discussions/README.md b/src/widgets/discussions/README.md index 1ca6513f00..7ebdc1a0ca 100644 --- a/src/widgets/discussions/README.md +++ b/src/widgets/discussions/README.md @@ -9,6 +9,7 @@ Built-in right-sidebar widget that embeds the Discussions MFE in an iframe for t | `id` | `DISCUSSIONS` | | `priority` | `10` (highest built-in priority) | | `isAvailable` | `({ unit }) => !!(unit?.id && unit?.enabledInContext)` | +| `Provider` | `DiscussionsProvider` — loads the topics (see *Data Loading*) | ## Availability @@ -22,19 +23,13 @@ Only shown when the current unit has a discussion topic enabled in context. Both |--------|-------------| | `discussionsWidgetConfig` | Ready-to-use widget config object | | `discussionsIsAvailable` | Availability function, usable standalone for custom configs | -| `discussionsPrefetch` | Prefetch function that loads discussion topics into the React Query cache | +| `DiscussionsProvider` | The widget's `Provider`; loads the course's discussion topics into the React Query cache | -## Data Prefetch +## Data Loading -The widget defines a `prefetch` function in its config. The sidebar framework calls this from `SidebarContextProvider` after mount and when course metadata changes to populate or refresh the discussion-topics query (bridged into the `discussionTopics` model for its `useModel` readers — transitional, #1977). Because this runs from a `useEffect`, initial render-time checks such as `isAvailable` (and initial sidebar computation) may still occur before the prefetch has completed; the framework's sync logic re-evaluates availability once the store updates: +The widget's `Provider`, `DiscussionsProvider`, observes the course's discussion-topics query (`discussionTopicsQuery` in `courseware/data/apiHooks.ts`), enabled only when `DISCUSSIONS_MFE_BASE_URL` is configured and the course has a `discussion` tab. The sidebar framework mounts it around the sidebar children whether or not the widget is available, so the topics load once per sidebar mount and not again when course metadata changes. The query is bridged into the `discussionTopics` model for the widget's `useModel` readers (transitional, #1977). Because the fetch starts after mount, the initial `isAvailable` check (and initial sidebar computation) runs before the topics arrive; the framework's sync logic re-evaluates availability once the query resolves. -```javascript -// Conditions checked before fetching: -// 1. DISCUSSIONS_MFE_BASE_URL is configured -// 2. Course has a 'discussion' tab (edxProvider) -``` - -The `DiscussionsTrigger` component itself is a pure render component — it reads the `discussionTopics` model but does not dispatch any fetches. +The `DiscussionsTrigger` component itself is a pure render component — it reads the `discussionTopics` model and fetches nothing. ## Customising Availability diff --git a/src/widgets/discussions/widgetConfig.js b/src/widgets/discussions/widgetConfig.js deleted file mode 100644 index 2009fe1392..0000000000 --- a/src/widgets/discussions/widgetConfig.js +++ /dev/null @@ -1,23 +0,0 @@ -import { getConfig } from '@edx/frontend-platform'; -import { hasDiscussionTab } from '@src/course-tabs/utils'; -import { prefetchDiscussionTopics } from '@src/courseware/data/apiHooks'; -import DiscussionsSidebar from './DiscussionsSidebar'; -import DiscussionsTrigger, { ID } from './DiscussionsTrigger'; - -export const discussionsIsAvailable = ({ unit }) => !!(unit?.id && unit?.enabledInContext); - -export const discussionsPrefetch = ({ courseId, course, queryClient }) => { - if (getConfig().DISCUSSIONS_MFE_BASE_URL && hasDiscussionTab(course?.tabs)) { - prefetchDiscussionTopics(queryClient, courseId); - } -}; - -export const discussionsWidgetConfig = { - id: ID, - priority: 10, - Sidebar: DiscussionsSidebar, - Trigger: DiscussionsTrigger, - isAvailable: discussionsIsAvailable, - prefetch: discussionsPrefetch, - enabled: true, -}; diff --git a/src/widgets/discussions/widgetConfig.test.ts b/src/widgets/discussions/widgetConfig.test.ts deleted file mode 100644 index 6b0df20633..0000000000 --- a/src/widgets/discussions/widgetConfig.test.ts +++ /dev/null @@ -1,49 +0,0 @@ -import { mergeConfig } from '@edx/frontend-platform'; -import { prefetchDiscussionTopics } from '@src/courseware/data/apiHooks'; -import { initializeMockApp } from '@src/setupTest'; - -import { discussionsPrefetch } from './widgetConfig'; - -jest.mock('@src/courseware/data/apiHooks', () => ({ - prefetchDiscussionTopics: jest.fn(), -})); - -initializeMockApp(); - -const courseId = 'course-v1:edX+DemoX+Demo_Course'; -const queryClient = { id: 'queryClient' }; -const datesTab = { tabId: 'dates', title: 'Dates', url: 'http://localhost/dates' }; -const discussionTab = { tabId: 'discussion', title: 'Discussion', url: 'http://localhost/discussion' }; - -describe('discussionsPrefetch', () => { - beforeEach(() => { - jest.clearAllMocks(); - mergeConfig({ DISCUSSIONS_MFE_BASE_URL: 'http://localhost:2002' }); - }); - - it('prefetches topics when the course has a discussion tab', () => { - discussionsPrefetch({ courseId, course: { tabs: [datesTab, discussionTab] }, queryClient }); - - expect(prefetchDiscussionTopics).toHaveBeenCalledWith(queryClient, courseId); - }); - - it('does not prefetch when the course has no discussion tab', () => { - discussionsPrefetch({ courseId, course: { tabs: [datesTab] }, queryClient }); - - expect(prefetchDiscussionTopics).not.toHaveBeenCalled(); - }); - - it('does not prefetch before the course metadata has loaded', () => { - discussionsPrefetch({ courseId, course: undefined, queryClient }); - - expect(prefetchDiscussionTopics).not.toHaveBeenCalled(); - }); - - it('does not prefetch without a discussions MFE configured', () => { - mergeConfig({ DISCUSSIONS_MFE_BASE_URL: '' }); - - discussionsPrefetch({ courseId, course: { tabs: [discussionTab] }, queryClient }); - - expect(prefetchDiscussionTopics).not.toHaveBeenCalled(); - }); -}); diff --git a/src/widgets/discussions/widgetConfig.ts b/src/widgets/discussions/widgetConfig.ts new file mode 100644 index 0000000000..9d17250ad0 --- /dev/null +++ b/src/widgets/discussions/widgetConfig.ts @@ -0,0 +1,16 @@ +import type { SidebarWidget, SidebarWidgetContext } from '@src/courseware/course/sidebar/SidebarContext'; +import DiscussionsProvider from './DiscussionsProvider'; +import DiscussionsSidebar from './DiscussionsSidebar'; +import DiscussionsTrigger, { ID } from './DiscussionsTrigger'; + +export const discussionsIsAvailable = ({ unit }: SidebarWidgetContext) => !!(unit?.id && unit?.enabledInContext); + +export const discussionsWidgetConfig: SidebarWidget = { + id: ID, + priority: 10, + Sidebar: DiscussionsSidebar, + Trigger: DiscussionsTrigger, + Provider: DiscussionsProvider, + isAvailable: discussionsIsAvailable, + enabled: true, +}; diff --git a/src/widgets/upgrade/README.md b/src/widgets/upgrade/README.md index f1cdc1748c..6957600eae 100644 --- a/src/widgets/upgrade/README.md +++ b/src/widgets/upgrade/README.md @@ -79,14 +79,17 @@ export default config; ### Widget config shape +`upgradeWidgetConfig` is a `SidebarWidget` (`src/courseware/course/sidebar/SidebarContext.ts`): + ```javascript { - id: 'UPGRADE', // string - priority: 20, // number (lower = shown first; discussions = 10) - Sidebar: UpgradePanel, // React component - Trigger: UpgradeTrigger, // React component - isAvailable: Function, // ({ course }) => boolean — receives merged coursewareMeta + courseHomeMeta - enabled: true, // boolean + id: 'UPGRADE', + priority: 20, // lower = shown first; discussions = 10 + Sidebar: UpgradePanel, + Trigger: UpgradeTrigger, + Provider: UpgradeWidgetProvider, + isAvailable: upgradeIsAvailable, + enabled: true, } ``` diff --git a/src/widgets/upgrade/src/UpgradeWidgetContext.jsx b/src/widgets/upgrade/src/UpgradeWidgetContext.tsx similarity index 57% rename from src/widgets/upgrade/src/UpgradeWidgetContext.jsx rename to src/widgets/upgrade/src/UpgradeWidgetContext.tsx index 73f7fbb97a..0db0bc2534 100644 --- a/src/widgets/upgrade/src/UpgradeWidgetContext.jsx +++ b/src/widgets/upgrade/src/UpgradeWidgetContext.tsx @@ -1,37 +1,36 @@ import { - createContext, useCallback, useContext, useMemo, useState, + createContext, useCallback, useContext, useMemo, useState, type ReactNode, } from 'react'; -import PropTypes from 'prop-types'; -import SidebarContext from '@src/courseware/course/sidebar/SidebarContext'; + +import { useSidebar } from '@src/courseware/course/sidebar/SidebarContext'; import { getLocalStorage, setLocalStorage } from '@src/data/localStorage'; -/** - * @typedef {Object} UpgradeWidgetContextValue - * @property {string|null} upgradeWidgetStatus - 'active' | 'inactive' | null - * @property {(status: string) => void} setUpgradeWidgetStatus - * @property {string|null} upgradeCurrentState - Current upgrade stage - * @property {(state: string) => void} setUpgradeCurrentState - * @property {() => void} onUpgradeWidgetSeen - Mark widget as seen (hides red dot) - */ +interface UpgradeWidgetContextValue { + upgradeWidgetStatus: string | null; // 'active' | 'inactive' | null + setUpgradeWidgetStatus: (status: string) => void; + upgradeCurrentState: string | null; // Current upgrade stage + setUpgradeCurrentState: (state: string) => void; + onUpgradeWidgetSeen: () => void; // Mark widget as seen (hides red dot) +} -const UpgradeWidgetContext = createContext(null); +const UpgradeWidgetContext = createContext(null); -export const UpgradeWidgetProvider = ({ children }) => { - const { courseId } = useContext(SidebarContext); +export const UpgradeWidgetProvider = ({ children }: { children: ReactNode }) => { + const { courseId } = useSidebar(); - const [upgradeWidgetStatus, setUpgradeWidgetStatusState] = useState( + const [upgradeWidgetStatus, setUpgradeWidgetStatusState] = useState( () => getLocalStorage(`upgradeWidget.${courseId}`) || 'active', ); - const [upgradeCurrentState, setUpgradeCurrentStateRaw] = useState( + const [upgradeCurrentState, setUpgradeCurrentStateRaw] = useState( () => getLocalStorage(`upgradeWidgetState.${courseId}`) || null, ); - const setUpgradeWidgetStatus = useCallback((status) => { + const setUpgradeWidgetStatus = useCallback((status: string) => { setUpgradeWidgetStatusState(status); setLocalStorage(`upgradeWidget.${courseId}`, status); }, [courseId]); - const setUpgradeCurrentState = useCallback((state) => { + const setUpgradeCurrentState = useCallback((state: string) => { setUpgradeCurrentStateRaw(state); setLocalStorage(`upgradeWidgetState.${courseId}`, state); }, [courseId]); @@ -40,7 +39,7 @@ export const UpgradeWidgetProvider = ({ children }) => { setUpgradeWidgetStatus('inactive'); }, [setUpgradeWidgetStatus]); - const value = useMemo(() => ({ + const value = useMemo(() => ({ upgradeWidgetStatus, setUpgradeWidgetStatus, upgradeCurrentState, @@ -61,11 +60,7 @@ export const UpgradeWidgetProvider = ({ children }) => { ); }; -UpgradeWidgetProvider.propTypes = { - children: PropTypes.node.isRequired, -}; - -export function useUpgradeWidgetContext() { +export function useUpgradeWidgetContext(): UpgradeWidgetContextValue { const ctx = useContext(UpgradeWidgetContext); if (!ctx) { throw new Error('useUpgradeWidgetContext must be used inside UpgradeWidgetProvider'); diff --git a/src/widgets/upgrade/src/utils.js b/src/widgets/upgrade/src/utils.js deleted file mode 100644 index adcdff8db0..0000000000 --- a/src/widgets/upgrade/src/utils.js +++ /dev/null @@ -1,8 +0,0 @@ -/** - * Check if the upgrade widget should be shown for the current course/unit context. - * - * @param {Object} context - * @param {Object} context.course - Merged coursewareMeta + courseHomeMeta for the current course - * @returns {boolean} - */ -export const upgradeIsAvailable = ({ course }) => !!course?.verifiedMode; diff --git a/src/widgets/upgrade/src/utils.ts b/src/widgets/upgrade/src/utils.ts new file mode 100644 index 0000000000..af6805942e --- /dev/null +++ b/src/widgets/upgrade/src/utils.ts @@ -0,0 +1,3 @@ +import type { SidebarWidgetContext } from '@src/courseware/course/sidebar/SidebarContext'; + +export const upgradeIsAvailable = ({ course }: SidebarWidgetContext) => !!course?.verifiedMode; diff --git a/src/widgets/upgrade/src/widgetConfig.js b/src/widgets/upgrade/src/widgetConfig.ts similarity index 73% rename from src/widgets/upgrade/src/widgetConfig.js rename to src/widgets/upgrade/src/widgetConfig.ts index c64a34bd78..cc7750603b 100644 --- a/src/widgets/upgrade/src/widgetConfig.js +++ b/src/widgets/upgrade/src/widgetConfig.ts @@ -1,9 +1,10 @@ +import type { SidebarWidget } from '@src/courseware/course/sidebar/SidebarContext'; import UpgradePanel from './UpgradePanel'; import UpgradeTrigger, { ID } from './UpgradeTrigger'; import { UpgradeWidgetProvider } from './UpgradeWidgetContext'; import { upgradeIsAvailable } from './utils'; -export const upgradeWidgetConfig = { +export const upgradeWidgetConfig: SidebarWidget = { id: ID, priority: 20, Sidebar: UpgradePanel,