Lesson Plan JS -> TS UI conversion and implementation enhancements - #8559
Open
adi-herwana-nus wants to merge 5 commits into
Open
Lesson Plan JS -> TS UI conversion and implementation enhancements#8559adi-herwana-nus wants to merge 5 commits into
adi-herwana-nus wants to merge 5 commits into
Conversation
- convert LessonPlanEdit and subpages to typescript - add debouncing / request batching logic to prevent duplicate requests / personalized timeline updates
…ms to typescript - migrate onSubmit() hook from redux store to parent components - remove obsolete javascript reducers (information moved to component state)
Contributor
There was a problem hiding this comment.
Pull request overview
Modernizes the lesson-plan frontend with TypeScript, Redux Toolkit, locally controlled dialogs, and serialized debounced saves.
Changes:
- Converts lesson-plan components, reducers, and tests to TypeScript.
- Adds debounced per-row saves and saving indicators.
- Moves form state out of Redux and adopts
createSlice.
Reviewed changes
Copilot reviewed 48 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
client/app/bundles/course/lesson-plan/types.ts |
Adds shared lesson-plan types. |
client/app/bundles/course/lesson-plan/store.ts |
Combines Redux Toolkit reducers/actions. |
client/app/bundles/course/lesson-plan/reducers/utils.ts |
Types grouping and visibility utilities. |
client/app/bundles/course/lesson-plan/reducers/milestoneForm.js |
Removes milestone form reducer. |
client/app/bundles/course/lesson-plan/reducers/lessonPlan.ts |
Adds typed lesson-plan slice. |
client/app/bundles/course/lesson-plan/reducers/lessonPlan.js |
Removes legacy reducer. |
client/app/bundles/course/lesson-plan/reducers/flags.ts |
Adds typed flags slice. |
client/app/bundles/course/lesson-plan/reducers/flags.js |
Removes legacy flags reducer. |
client/app/bundles/course/lesson-plan/reducers/eventForm.js |
Removes event form reducer. |
client/app/bundles/course/lesson-plan/pages/LessonPlanShow/MilestoneAdminTools.tsx |
Converts milestone tools and owns dialog state. |
client/app/bundles/course/lesson-plan/pages/LessonPlanShow/MilestoneAdminTools.jsx |
Removes legacy component. |
client/app/bundles/course/lesson-plan/pages/LessonPlanShow/LessonPlanItem/AdminTools.tsx |
Converts event tools and owns dialog state. |
client/app/bundles/course/lesson-plan/pages/LessonPlanShow/LessonPlanItem/AdminTools.jsx |
Removes legacy component. |
client/app/bundles/course/lesson-plan/pages/LessonPlanShow/LessonPlanItem/__test__/AdminTools.test.tsx |
Updates event tool tests. |
client/app/bundles/course/lesson-plan/pages/LessonPlanShow/__test__/MilestoneAdminTools.test.tsx |
Updates milestone tool tests. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/types.ts |
Defines save context. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/MilestoneRow.tsx |
Adds serialized milestone saving. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/MilestoneRow.jsx |
Removes legacy milestone row. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/ItemRow/PublishedCell.tsx |
Types and disables publication toggle. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/ItemRow/PublishedCell.jsx |
Removes legacy cell. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/ItemRow/index.tsx |
Adds merged, debounced item saves. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/ItemRow/index.jsx |
Removes legacy item row. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/ItemRow/DateCell.tsx |
Types date updates and shifting. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/ItemRow/DateCell.jsx |
Removes legacy date cell. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/index.tsx |
Converts edit page. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/index.jsx |
Removes legacy edit page. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/__test__/MilestoneRow.test.tsx |
Tests debounced milestone updates. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/__test__/MilestoneRow.test.jsx |
Removes legacy tests. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/__test__/ItemRow.test.tsx |
Tests item autosave behavior. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/__test__/ItemRow.test.jsx |
Removes legacy tests. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/__test__/index.test.tsx |
Converts edit-page test. |
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/__test__/index.test.jsx |
Removes legacy test. |
client/app/bundles/course/lesson-plan/operations.ts |
Dispatches slice actions and reports form success. |
client/app/bundles/course/lesson-plan/containers/MilestoneFormDialog/index.tsx |
Makes milestone dialog controlled. |
client/app/bundles/course/lesson-plan/containers/MilestoneFormDialog/index.jsx |
Removes Redux-connected dialog. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/NewMilestoneButton.tsx |
Owns milestone creation dialog. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/NewMilestoneButton.jsx |
Removes legacy button. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/NewEventButton.tsx |
Owns event creation dialog. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/NewEventButton.jsx |
Removes legacy button. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/index.tsx |
Converts layout and removes global dialogs. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/index.jsx |
Removes legacy layout. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/EnterEditModeButton.tsx |
Updates translation handling. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/__test__/NewMilestoneButton.test.tsx |
Updates milestone creation test. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/__test__/NewEventButton.test.tsx |
Updates event creation test. |
client/app/bundles/course/lesson-plan/containers/LessonPlanLayout/__test__/index.test.tsx |
Adds layout fetch coverage. |
client/app/bundles/course/lesson-plan/containers/EventFormDialog/index.tsx |
Makes event dialog controlled. |
client/app/bundles/course/lesson-plan/containers/EventFormDialog/index.jsx |
Removes Redux-connected dialog. |
client/app/bundles/course/lesson-plan/constants.ts |
Retains typed field constants. |
client/app/bundles/course/lesson-plan/constants.js |
Removes legacy action constants. |
Suppressed comments (2)
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/ItemRow/index.tsx:136
- This guard compares only with the last queued value, not with the saved baseline. Before the debounce fires, editing
A → B → AleavesAinpendingRefand sends a no-op PATCH, contrary to the stated requirement that reverting to the saved value sends nothing. Remove that field from the pending payload when it returns to the saved value, while still queuing a revert if a different value for that field is already in flight.
const latest =
key in latestValuesRef.current
? latestValuesRef.current[key]
: (savedValues[key] as ItemValue);
if (sameValue(latest, value)) return acc;
return { ...acc, [key]: value };
client/app/bundles/course/lesson-plan/pages/LessonPlanEdit/MilestoneRow.tsx:116
- The milestone queue has the same saved-value gap: before the debounce fires, changing
A → B → Areplaces the pending update withAand still sends a no-op PATCH. The described dedupe behavior requires cancelling the pending update when the value returns to the saved baseline, unless the differing value has already been sent in flight.
const latest =
latestValueRef.current === undefined ? startAt : latestValueRef.current;
if (sameDate(latest, newDate)) return;
latestValueRef.current = newDate;
pendingRef.current = { startAt: newDate, setError };
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| }), | ||
| hideEventForm: () => ({ type: actionTypes.EVENT_FORM_HIDE }), | ||
| }; | ||
| export const actions = { ...lessonPlanActions, ...flagsActions }; |
Comment on lines
+103
to
+107
| inFlightRef.current = true; | ||
| context | ||
| .dispatch(updateItem(context.id, payload, successMessage, failureMessage)) | ||
| .finally(() => { | ||
| inFlightRef.current = false; |
Comment on lines
+90
to
+95
| .finally(() => { | ||
| inFlightRef.current = false; | ||
| if (pendingRef.current) { | ||
| flush(context); | ||
| } else { | ||
| setSaving(false); |
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.
Key changes
1. Debounced, serialised saves (
ItemRow,MilestoneRow)Modelled on the submission answers autosave loop, with one deliberate difference.
FIELD_LONG_DEBOUNCE_DELAY_MS), so a typed date collapses into one request.than three. This matters more than the debounce alone: the request axis is per item, so merging is
what keeps it to one job per edit.
completion.
no-op check so reverting a field to its saved value sends nothing.
LoadingIndicatorbeside the row title from first keystroke until the save resolves, and thepublished toggle is disabled while saving.
The answers loop uses a
clientVersionlogical clock to let overlapping saves reconcile afterwards.That is right for a text editor and wrong here — we want no overlap. Serialising per row gives the
stronger guarantee with less state, and avoids a latent bug the version approach would have needed to
handle:
ITEM_UPDATE_SUCCESSwrites client-supplied values into the store, so two responses arrivingout of order would leave the grid showing a value the server does not have.
2. Form handlers out of Redux (
EventFormDialog,MilestoneFormDialog)showEventForm/showMilestoneFormstashed anonSubmitfunction in the store — not serialisable,silently tolerated in production, noisy in development, and a blocker for RTK's
serializableCheck.Both dialogs are now controlled by whoever opens them, taking
open,onClose,formTitle,initialValuesandonSubmitas props, with localsubmittingstate. Each opener renders its owndialog; the global mounts in
LessonPlanLayoutare gone. This followsCreateRenameTimelinePromptinthe reference-timelines bundle.
The operations previously closed the dialog themselves via
dispatch(actions.hideMilestoneForm())inside
.then(). Their.catch()swallows errors, so the returned promise resolved either way and thecaller could not tell success from failure. The four create/update operations are now
Operation<boolean>, and the dialog closes itself only on success — so a failed submit still staysopen with its field errors, exactly as before.
eventForm.jsandmilestoneForm.jsare deleted, along with theircombineReducersentries, fouraction creators and four action types. Nothing read them once the handlers moved.
3. TypeScript and
createSlice21 files converted. All the touched class components became function components using
useAppDispatch/useAppSelector/useTranslation, soconnect,injectIntl,PropTypesandFormattedMessageare gone from them — the last of those being the deprecation we are phasing out.Both remaining reducers are now
createSlice, matchingsubmission/reducers/historyandsubmission/reducers/scribing.flagsSlicepicks up the flags that arrive with the lesson plan fetchthrough
extraReducersonlessonPlanActions.loadSucceededrather than sharing a string constant.actionTypesis gone entirely;constants.tskeeps onlyfields.With the reducers typed,
combineReducersinfers the slice and every compatibility cast in thecomponents disappeared —
state.lessonPlan.lessonPlan.groupsand friends now type-check directly.Behaviour changes worth reviewing
These are real changes, not pure refactors:
start_atno longer sends a request. It is required server-side, so an empty field is atransient state on the way to a new date rather than an update the server would accept. Previously it
PATCHed and 4xx'd. Applies to both items and milestones.
createEvent/updateEventnow receivesetError. Their signatures always declared it, but theJS callers passed one argument short and silently dropped it — TypeScript caught this on conversion.
Server-side validation errors now reach the event form's fields, as they already did for milestones.
start_atno longer wipes the end dates. The oldDateCellcomputed the start-shiftunconditionally, so a cleared start produced
moment(null).diff(...)→NaN→nullforend_atand
bonus_end_attoo.AdminToolsguardslesson_plan_item_type?.[0]. MovinginitialValuesfrom dispatch-time intorender meant it evaluated on every render; any event item lacking that field would previously have
been fine until you clicked edit.
LessonPlanLayoutdropped achildrenprop that was declared required inpropTypesbut neverpassed or used — the component renders
<Outlet />.Testing
tsc --noEmit, eslint and prettier are clean. 27 tests pass across 9 suites in the bundle, up from 24.Five tests added, and each of the three regression tests was verified to fail without its fix
rather than pass vacuously:
sends one request when a date is edited several times in quick succession— two rapid changes,asserts exactly one PATCH carrying the final value. This is the incident, in a test.
sends nothing while start date is empty(items) andsends nothing while the date is empty(milestones) — both confirmed failing with the required-field guard removed.
sends the start date once it is valid again, andshows a saving indicator until the update resolves.The four dialog tests got simpler: they used to render the opener and the dialog side by side and let
Redux connect them, which was the coupling itself.
Proving the absence of a request needs the debounce window to actually elapse, so two tests wait it
out via a
settleDebouncehelper. That adds roughly 4s to the suite. Fake timers would avoid it butinteract badly with RTL's
waitForand the MUI pickers.Notes / follow-ups
types.tsimportedAppDispatchfrom the appstore, which imports this bundle's store, which now imports the reducers, which import
types.ts.SaveContextwas the only thing needingAppDispatch, so it moved topages/LessonPlanEdit/types.ts.tscwas happy either way; onlyimport/no-cycleflagged it.updateOrAppendmerges rather than replaces, and the slice preserves that. An item updatedispatches only the changed fields, so a straight replace would blank title and type on every save.
as unknown as Partial<AppState>remain, all in tests. Unrelated to the reducers:Partial<AppState>only permits omitting whole slices, and these tests seed a handful of fields.Removing them means building full slice fixtures.
useDebouncecancels rather than flushes, so navigatingaway within 1500ms drops it. Inherited from the shared hook; the answers loop behaves the same way,
though the longer window makes it more reachable here. Worth a decision, not silently changing shared
hook semantics.
but
DateTimePickerholds its own display state. Pre-existing and orthogonal; the fix is therollback()callback pattern the timelines editor already uses..jsxfiles remain in the bundle — the two form bodies, three containers, and theLessonPlanShowsubtree. None were touched by this work.