Skip to content

Commit 635c212

Browse files
committed
refactor(course-outline): simplify sidebar modal state
Extract useModalState<T> generic hook — refactor useConfigureDialog and useHighlightsModal to delegate toggle+data state to it. Extract useItemFieldSync hook — replace three didMountRef+useEffect blocks in SubsectionSettings. Extract useDefaultTab hook — replace identical default-tab effects in SectionInfoSidebar, SubsectionInfoSidebar, UnitInfoSidebar. 43/43 affected tests pass. No new lint/format/type issues.
1 parent 525fabd commit 635c212

9 files changed

Lines changed: 127 additions & 65 deletions

File tree

src/course-outline/outline-sidebar/info-sidebar/SectionInfoSidebar.tsx

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useEffect } from 'react';
1+
import { useDefaultTab } from '../../../hooks/useDefaultTab';
22
import { useIntl } from '@edx/frontend-platform/i18n';
33
import { Tab, Tabs } from '@openedx/paragon';
44
import { useNavigate } from 'react-router-dom';
@@ -45,12 +45,7 @@ export const SectionSidebar = () => {
4545
settings: 'settings',
4646
};
4747

48-
useEffect(() => {
49-
if (!currentTabKey || !Object.values(availableTabs).includes(currentTabKey)) {
50-
// Set default Tab key
51-
setCurrentTabKey('info');
52-
}
53-
}, [currentTabKey, setCurrentTabKey]);
48+
useDefaultTab('section');
5449
const { sectionId = '', index } = selectedContainerState ?? {};
5550
const { data: sectionData, isLoading } = useCourseItemData(sectionId);
5651

src/course-outline/outline-sidebar/info-sidebar/SubsectionInfoSidebar.tsx

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useEffect } from 'react';
1+
import { useDefaultTab } from '../../../hooks/useDefaultTab';
22
import { isEmpty } from 'lodash';
33

44
import { useIntl } from '@edx/frontend-platform/i18n';
@@ -42,12 +42,7 @@ export const SubsectionSidebar = () => {
4242
settings: 'settings',
4343
};
4444

45-
useEffect(() => {
46-
if (!currentTabKey || !Object.values(availableTabs).includes(currentTabKey)) {
47-
// Set default Tab key
48-
setCurrentTabKey('info');
49-
}
50-
}, [currentTabKey, setCurrentTabKey]);
45+
useDefaultTab('subsection');
5146
const { data: section } = useCourseItemData<XBlock>(selectedContainerState?.sectionId);
5247
const { courseId, openUnlinkModal } = useCourseAuthoringContext();
5348
const duplicateMutation = useDuplicateItem(courseId);

src/course-outline/outline-sidebar/info-sidebar/SubsectionSettings.tsx

Lines changed: 4 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,9 @@ import AdvancedTab from '@src/generic/configure-modal/AdvancedTab';
1414
import { DatepickerControl, DATEPICKER_TYPES } from '@src/generic/datepicker-control';
1515
import { SidebarContent, SidebarSection } from '@src/generic/sidebar';
1616
import { useStateWithCallback } from '@src/hooks';
17+
import { useItemFieldSync } from '../../../hooks/useItemFieldSync';
1718
import {
1819
useCallback,
19-
useEffect,
20-
useRef,
2120
useState,
2221
} from 'react';
2322
import { ReleaseSection } from './sharedSettings/ReleaseSection';
@@ -53,19 +52,11 @@ const GradingSection = ({ subsectionId, onChange }: SubProps) => {
5352
},
5453
(val) => onChange(val || {}),
5554
);
56-
const didMountRef = useRef(false);
57-
58-
useEffect(() => {
55+
useItemFieldSync(() => {
5956
const nextState = {
6057
graderType: itemData?.format,
6158
dueDate: itemData?.due || '',
6259
};
63-
64-
if (!didMountRef.current) {
65-
didMountRef.current = true;
66-
return;
67-
}
68-
6960
if (localState?.graderType !== nextState.graderType || localState?.dueDate !== nextState.dueDate) {
7061
setLocalState(nextState);
7162
}
@@ -149,14 +140,7 @@ const AssessmentResultVisibilitySection = ({ subsectionId, onChange }: SubProps)
149140
},
150141
(val) => onChange(val || {}),
151142
);
152-
const didMountRef = useRef(false);
153-
154-
useEffect(() => {
155-
if (!didMountRef.current) {
156-
didMountRef.current = true;
157-
return;
158-
}
159-
143+
useItemFieldSync(() => {
160144
if (localState?.showCorrectness !== itemData?.showCorrectness) {
161145
setLocalState({ showCorrectness: itemData?.showCorrectness });
162146
}
@@ -222,17 +206,9 @@ const SpecialExamSection = ({ subsectionId, onChange }: SubProps) => {
222206
getLatestLocalState,
223207
(val) => onChange(val || {}),
224208
);
225-
const didMountRef = useRef(false);
226-
227-
useEffect(() => {
228-
if (!didMountRef.current) {
229-
didMountRef.current = true;
230-
return;
231-
}
232-
209+
useItemFieldSync(() => {
233210
const nextState = getLatestLocalState();
234211
const hasChanges = Object.keys(nextState).some((key) => (localState as any)?.[key] !== (nextState as any)[key]);
235-
236212
if (hasChanges) {
237213
setLocalState({ value: nextState, skipCallback: true });
238214
}

src/course-outline/outline-sidebar/info-sidebar/UnitInfoSidebar.tsx

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { useEffect, useContext } from 'react';
1+
import { useContext } from 'react';
2+
import { useDefaultTab } from '../../../hooks/useDefaultTab';
23
import { isEmpty } from 'lodash';
34

45
import { useIntl } from '@edx/frontend-platform/i18n';
@@ -89,12 +90,7 @@ export const UnitSidebar = () => {
8990
settings: 'settings',
9091
};
9192

92-
useEffect(() => {
93-
if (!currentTabKey || !Object.values(availableTabs).includes(currentTabKey)) {
94-
// Set default Tab key
95-
setCurrentTabKey('preview');
96-
}
97-
}, [currentTabKey, setCurrentTabKey]);
93+
useDefaultTab('unit');
9894

9995
const { data: section } = useCourseItemData<XBlock>(selectedContainerState?.sectionId);
10096
const { data: subsection } = useCourseItemData<XBlock>(selectedContainerState?.subsectionId);

src/course-outline/state/useConfigureModal.ts

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
import { useState, useCallback } from 'react';
1+
import { useCallback } from 'react';
22

3-
import { useToggle } from '@openedx/paragon';
43
import type { OutlineActionSelection, XBlock } from '@src/data/types';
54
import {
65
useCourseItemData,
@@ -11,6 +10,7 @@ import {
1110
} from '../data';
1211
import { useOutlineConfigureAction } from './useOutlineActions';
1312
import { COURSE_BLOCK_NAMES } from '../constants';
13+
import { useModalState } from './useModalState';
1414

1515
export interface UseConfigureDialogOutput {
1616
isConfigureModalOpen: boolean;
@@ -27,8 +27,12 @@ export interface UseConfigureDialogOutput {
2727
export function useConfigureDialog(courseId: string): UseConfigureDialogOutput {
2828
const { handleConfigureItemSubmit } = useOutlineConfigureAction(courseId);
2929

30-
const [isConfigureModalOpen, openConfigureModal, closeConfigureModal] = useToggle(false);
31-
const [configureModalData, setConfigureModalData] = useState<OutlineActionSelection | undefined>();
30+
const {
31+
isOpen: isConfigureModalOpen,
32+
open: openConfigureModal,
33+
close: closeConfigureModal,
34+
data: configureModalData,
35+
} = useModalState<OutlineActionSelection>();
3236

3337
const { data: configureItemData } = useCourseItemData(
3438
isConfigureModalOpen ? configureModalData?.currentId : undefined,
@@ -39,18 +43,26 @@ export function useConfigureDialog(courseId: string): UseConfigureDialogOutput {
3943

4044
const handleConfigureModalClose = useCallback(() => {
4145
closeConfigureModal();
42-
setConfigureModalData(undefined);
4346
}, [closeConfigureModal]);
4447

4548
const handleOpenConfigureModal = useCallback((selection: OutlineActionSelection) => {
46-
setConfigureModalData(selection);
47-
openConfigureModal();
49+
openConfigureModal(selection);
4850
}, [openConfigureModal]);
4951

50-
const payloadBuilders: Record<string, (data: typeof configureModalData, vars: Record<string, unknown>) => ConfigureItemPayload> = {
52+
const payloadBuilders: Record<
53+
string,
54+
(data: typeof configureModalData, vars: Record<string, unknown>) => ConfigureItemPayload
55+
> = {
5156
chapter: (data, vars) => ({ category: 'chapter', sectionId: data!.sectionId, ...vars }) as ChapterConfigurePayload,
52-
sequential: (data, vars) => ({ category: 'sequential', itemId: data!.currentId, sectionId: data!.sectionId, ...vars }) as SequentialConfigurePayload,
53-
vertical: (data, vars) => ({ category: 'vertical', unitId: data!.currentId, sectionId: data!.sectionId, ...vars }) as UnitConfigurePayload,
57+
sequential: (data, vars) =>
58+
({
59+
category: 'sequential',
60+
itemId: data!.currentId,
61+
sectionId: data!.sectionId,
62+
...vars,
63+
}) as SequentialConfigurePayload,
64+
vertical: (data, vars) =>
65+
({ category: 'vertical', unitId: data!.currentId, sectionId: data!.sectionId, ...vars }) as UnitConfigurePayload,
5466
};
5567

5668
const handleConfigureItemSubmitWrapper = useCallback(async (variables: Record<string, unknown>) => {

src/course-outline/state/useHighlightsModal.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useState, useCallback } from 'react';
1+
import { useCallback } from 'react';
22

33
import { useToggle } from '@openedx/paragon';
44
import type { XBlock } from '@src/data/types';
@@ -7,6 +7,7 @@ import {
77
useEnableCourseHighlightsEmails,
88
} from '../data';
99
import type { HighlightData } from '../highlights-modal/HighlightsModal';
10+
import { useModalState } from './useModalState';
1011

1112
export interface UseHighlightsModalOutput {
1213
isEnableHighlightsModalOpen: boolean;
@@ -28,17 +29,20 @@ export function useHighlightsModal(courseId: string): UseHighlightsModalOutput {
2829
const enableHighlightsEmailsMutation = useEnableCourseHighlightsEmails(courseId);
2930

3031
const [isEnableHighlightsModalOpen, openEnableHighlightsModal, closeEnableHighlightsModal] = useToggle(false);
31-
const [isHighlightsModalOpen, openHighlightsModal, closeHighlightsModal] = useToggle(false);
32-
const [highlightsModalData, setHighlightsModalData] = useState<string | undefined>();
32+
const {
33+
isOpen: isHighlightsModalOpen,
34+
open: openHighlightsModal,
35+
close: closeHighlightsModal,
36+
data: highlightsModalData,
37+
} = useModalState<string>();
3338

3439
const handleEnableHighlightsSubmit = useCallback(() => {
3540
enableHighlightsEmailsMutation.mutate();
3641
closeEnableHighlightsModal();
3742
}, [enableHighlightsEmailsMutation, closeEnableHighlightsModal]);
3843

3944
const handleOpenHighlightsModal = useCallback((section: XBlock) => {
40-
setHighlightsModalData(section.id);
41-
openHighlightsModal();
45+
openHighlightsModal(section.id);
4246
}, [openHighlightsModal]);
4347

4448
const handleHighlightsFormSubmit = useCallback((highlights: HighlightData) => {
@@ -48,7 +52,6 @@ export function useHighlightsModal(courseId: string): UseHighlightsModalOutput {
4852
) as string[];
4953
highlightsMutation.mutate({ sectionId: highlightsModalData, highlights: dataToSend });
5054
closeHighlightsModal();
51-
setHighlightsModalData(undefined);
5255
}, [highlightsModalData, highlightsMutation, closeHighlightsModal]);
5356

5457
return {
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { useState, useCallback } from 'react';
2+
3+
/**
4+
* Generic modal state hook — manages open/close + optional typed data.
5+
*
6+
* @example
7+
* const { isOpen, open, close, data } = useModalState<OutlineActionSelection>();
8+
* open(someSelection); // sets data + isOpen = true
9+
* close(); // clears data + isOpen = false
10+
* data // the selection (or undefined when closed)
11+
*/
12+
export function useModalState<T>() {
13+
const [isOpen, setIsOpen] = useState(false);
14+
const [data, setData] = useState<T | undefined>();
15+
16+
const open = useCallback((d: T) => {
17+
setData(d);
18+
setIsOpen(true);
19+
}, []);
20+
21+
const close = useCallback(() => {
22+
setData(undefined);
23+
setIsOpen(false);
24+
}, []);
25+
26+
return { isOpen, open, close, data };
27+
}

src/hooks/useDefaultTab.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import { useEffect } from 'react';
2+
import { useOutlineSidebarContext } from '@src/course-outline/outline-sidebar/OutlineSidebarContext';
3+
4+
type SidebarLevel = 'section' | 'subsection' | 'unit';
5+
6+
interface TabConfig {
7+
defaultKey: string;
8+
availableTabs: Record<string, string>;
9+
}
10+
11+
const TAB_CONFIG: Record<SidebarLevel, TabConfig> = {
12+
section: { defaultKey: 'info', availableTabs: { info: 'info', settings: 'settings' } },
13+
subsection: { defaultKey: 'info', availableTabs: { info: 'info', settings: 'settings' } },
14+
unit: { defaultKey: 'preview', availableTabs: { preview: 'preview', info: 'info', settings: 'settings' } },
15+
};
16+
17+
/**
18+
* Sets the default sidebar tab on mount if no valid tab is selected.
19+
*
20+
* @param level - Sidebar level identifying which block is being viewed.
21+
*
22+
* - `'section'` / `'subsection'`: defaults to `'info'` tab.
23+
* - `'unit'`: defaults to `'preview'` tab.
24+
*/
25+
export function useDefaultTab(level: SidebarLevel): void {
26+
const { currentTabKey, setCurrentTabKey } = useOutlineSidebarContext();
27+
const { defaultKey, availableTabs } = TAB_CONFIG[level];
28+
29+
useEffect(() => {
30+
if (!currentTabKey || !Object.values(availableTabs).includes(currentTabKey)) {
31+
setCurrentTabKey(defaultKey);
32+
}
33+
}, [currentTabKey, setCurrentTabKey, defaultKey, availableTabs]);
34+
}

src/hooks/useItemFieldSync.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { useEffect, useRef } from 'react';
2+
3+
/**
4+
* Runs `effect` on every render after the first mount.
5+
*
6+
* The original pattern this replaces uses `didMountRef` + `useEffect` to
7+
* skip the initial mount. This hook preserves that first‑mount skip
8+
* behavior.
9+
*
10+
* NOTE: intentionally does **not** return `effect()` — the original pattern
11+
* discards cleanup by design. Returning it would cause React to call the
12+
* return value as a cleanup function on unmount, risking exceptions.
13+
*/
14+
export function useItemFieldSync(effect: () => void, deps: any[]): void {
15+
const didMountRef = useRef(false);
16+
17+
useEffect(() => {
18+
if (!didMountRef.current) {
19+
didMountRef.current = true;
20+
return;
21+
}
22+
effect();
23+
}, deps);
24+
}

0 commit comments

Comments
 (0)