Repository navigation
[scheduler] Introduce the edit dialog form context & lifecycle contract - #23284
Conversation
Groundwork for mui#22868: an ephemeral store holding a generic values bag, a provider/context scoped to the dialog, and the useField hook sections will use to read/write/validate fields. Not wired to the dialog yet. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
Replaces the prop-drilled controlled/errors state with the form store: sections and the premium recurrence tab read/write their fields through useField, title/description move off FormData, the placeholder push is centralized in the provider, and submit merges only the custom fields the user edited so untouched ones keep resolving against the live model. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
…eField Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
Cancel unmounts the provider, so nothing resets the form. Same criterion as leaving useFormIsDirty out: no consumer, no API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
The bare render throw made React log an uncaught error in browser mode, which vitest-fail-on-console turns into a failure on React 18. Follow the ErrorBoundary + toErrorDev pattern used by the charts hooks tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
…ab surface Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
Follows the scheduler convention of keeping selector objects out of the store class file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
…ously validateAll reports all failing fields at once, unlike the previous early-return submit checks — this test keeps that behavior from silently reverting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GcmQD95oLcbbmmLzg6W5pR
There was a problem hiding this comment.
PR review
The refactor is well-shaped — subclassing base-ui Store matches SchedulerStore, the createSelector/useStore pairing matches the house selector idiom, and getDirtyValues correctly preserves the #22874 stale-snapshot fix. Nothing here is merge-blocking. The two things I'd fix before merge are a newly-reachable error-clearing bug that the PR's own new test sets up, and a re-render scope regression from hoisting two subscriptions to the dialog root. The rest is hardening the contract before #22871 makes it public — chiefly the untyped values bag and the unguarded shared key space with the event model.
Verification limit: I could not execute the suites. The worktree has no node_modules and the environment's Node (v22.22.3) is below the repo's engines.node >= 22.23.1, so pnpm install is blocked. Every finding below was checked by reading both revisions of the source.
Bugs (7)
1. 🟠 Editing any date clears every other section's error — including the resource error the new test just made co-visible
Location: packages/x-scheduler/src/internals/components/event-dialog/DateTimeSection.tsx:89
const createHandleChangeDateOrTimeField =
(field: { setValue: (value: string) => void }) =>
(event: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement>) => {
// Editing any date or time field invalidates the range errors as a whole.
formStore.clearErrors(); // set('errors', {}) — the whole map, not the range keys
field.setValue(event.currentTarget.value);
};clearErrors() empties the entire errors record. The base branch did setErrors({}) too, so the line isn't new — but at base handleSubmit early-returned after the first failing check, so only one error could ever exist and clearing all of them was equivalent to clearing the one. This PR removes that early return, and its own new test (EventDialog.test.tsx:606, "should show the range error and the resource error at the same time") pins that two errors now coexist. The blanket clear became observable for the first time.
setValues already clears the written key's error; this call exists only to cross-clear startDate ↔ startTime, which is a two-key problem being solved with a form-wide sledgehammer.
Failure scenario: With shouldEventRequireResource and an event with no resource, invert the dates and press Save — both "Start date must be before end date" and "A resource is required" appear. Now type one character into the end date: the resource error disappears while the resource is still empty. The user reads that as resolved, presses Save, and it pops back.
Fix: Add a key argument — clearErrors(keys?: readonly string[]) — and call formStore.clearErrors(['startDate', 'startTime']). Better, generalize the registry the mechanism is 90% of the way to: useField('startDate', { validate, dependsOn: ['startTime', 'endDate', 'endTime', 'allDay'] }), and have setValues clear errors for any field whose validator depends on a changed key. That deletes createHandleChangeDateOrTimeField entirely and closes the sibling gap where the all-day Switch (line 171) writes without clearing at all, leaving a stale range error behind.
2. 🟠 Two subscriptions hoisted to the dialog root — every title keystroke, and every placeholder push during creation, now re-renders the whole dialog
Location: packages/x-scheduler/src/internals/components/event-dialog/FormContent.tsx:112 and :193
export function FormContent(props: FormContentProps) {
const pushPlaceholder = usePushPlaceholder(); // ← subscribes to placeholder + displayTimezone
...
}
function FormContentInner(props: FormContentProps) {
const title = useField<string>('title'); // ← subscribes to the form storeBoth moved outward from leaves:
- Title. At base it was uncontrolled —
defaultValue={occurrence.title}, read back vianew FormData()at submit — so typing caused zero React renders. It is now controlled fromFormContentInner, which rendersGeneralTab(→DateTimeSection,ResourceAndColorSection,DescriptionSection) andRecurrenceTabRenderer. - Placeholder. At base
usePushPlaceholder()was called inDateTimeSection:64andResourceAndColorSection:152. It now lives in the outerFormContent, so a placeholder write during event creation re-renders the provider and handsFormContentInnera fresh element.
I confirmed there is no React.memo/fastMemo anywhere in either event-dialog folder, so each of these reconciles the full subtree. Note DescriptionSection got this right — useField('description') is scoped to the leaf, so description typing only re-renders its own TextField.
Failure scenario: On a calendar with ~150 resources, typing a 40-character title triggers 40 full-subtree reconciliations that previously cost nothing, each rebuilding ResourceAndColorSection's ~150 <ResourceMenuItem> elements (only resourcesOptions is memoized, not the JSX) plus the entire premium RecurrenceTab. I could not measure whether this crosses into perceptible typing lag, but the work is unambiguously new.
Fix: Extract the title block (FormContent.tsx:296-313) into a leaf EventDialogTitleField that calls useField<string>('title') itself — ~12 lines moved, mirroring DescriptionSection. For the placeholder, move the useStore subscriptions inside the returned pushPlaceholder closure's dependencies via a ref, or keep usePushPlaceholder() in the provider rather than in FormContent's render.
3. 🟠 The values bag shares an unguarded namespace with the event model, and the two submit paths disagree on who wins
Location: packages/x-scheduler/src/internals/components/event-dialog/FormContent.tsx:260 vs :243
// recurring path — metaChanges first, id wins
const changes: SchedulerEventUpdatedProperties = {
...metaChanges,
id: occurrence.id,
start, end,
...(recurrenceModified ? { rrule: rruleToSubmit } : {}),
};
// non-recurring path — id first, metaChanges wins
store.updateEvent({ id: occurrence.id, ...metaChanges, start, end, rrule: rruleToSubmit });metaChanges contains getDirtyValues(BUILT_IN_FORM_KEYS), and BUILT_IN_FORM_KEYS lists only the eleven form keys. The event namespace is a different, larger set that already exists in this codebase as EVENT_PROPERTIES_LOOKUP (SchedulerStore.utils.ts:61): id, readOnly, timezone, className, resizable, draggable, exDates, extractedFromId, …. getCustomEventProperties strips those from the seed, so initialValues.id is undefined and any write makes the key dirty. Nothing warns.
Today no built-in section writes a reserved key, so nothing is broken on next. But useField is exported from @mui/x-scheduler/internals and the premium test already consumes it cross-package — this is the contract #22871 hands to users, and it is the item I'd fix first.
Failure scenario: A custom section calls useField('timezone') (a per-event timezone picker — an entirely natural thing to build) or useField('readOnly') and silently overwrites core event behavior. useField('id') is worse: on the non-recurring path the form value overrides occurrence.id, so updateEvent targets a non-existent event and the user's edit vanishes — while the recurring path, with the opposite spread order, behaves correctly. The milder version is a typo: useField('titel') renders blank, writes fine, and permanently adds a junk titel property to the user's event model that round-trips through onEventsChange into their backend.
Fix: Two things now, one later. (a) Spread metaChanges first on every path so built-ins always win — makes the three paths agree. (b) Throw or warnOnce in dev from useField when the key is in EVENT_PROPERTIES_LOOKUP but not in BUILT_IN_FORM_KEYS, and warn on a write to a key that was neither seeded nor registered. Longer term, give custom fields their own sub-bag (values.custom) — that's the change that gets expensive after #22871 ships.
4. 🟡 A validator returning '' blocks submit forever with nothing rendered
Location: packages/x-scheduler/src/internals/components/event-dialog/form/EventDialogFormStore.ts:100
const result = validator(values[key], values);
if (result != null && !(Array.isArray(result) && result.length === 0)) {
errors[key] = result;
break;
}An empty array is explicitly treated as valid (there's a dedicated test for it), but an empty string is not.
Failure scenario: A section registers validate: (v) => (v ? '' : 'Required') — a natural mistake given the empty-array precedent. When the value is valid, '' is recorded as an error, validateAll() returns false, and Save silently does nothing forever while helperText="" renders nothing.
Fix: if (result != null && result !== '' && !(Array.isArray(result) && result.length === 0)).
5. 🟡 values and initialValues alias the caller's object, so the dirty baseline is externally mutable
Location: packages/x-scheduler/src/internals/components/event-dialog/form/EventDialogFormStore.ts:41
super({ values: initialValues, initialValues, errors: {} }); // same reference, twiceUntil the first write, values and initialValues are the same object — and both are the caller's object, passed straight through from FormContent's useRefWithInit. I checked RecurrenceTab: every handler spreads ({ ...getRruleDraft(), … }), so nothing mutates in place today. This is latent, but getDirtyValues is the sole gate on what reaches onEventsChange.
Failure scenario: A future section does formStore.state.values.rruleDraft.count = 3 — an easy mistake, since getRruleDraft() (RecurrenceTab.tsx:204) hands out the live object. That corrupts initialValues too, so getDirtyValues reports the field clean and the edit is silently dropped from the payload.
Fix: super({ values: { ...initialValues }, initialValues: { ...initialValues }, errors: {} }).
6. 🟡 useField returns undefined for any key absent from the seed, and clearing such a field is a silent no-op
Location: packages/x-scheduler/src/internals/components/event-dialog/form/useField.ts:23
const value = useStore(store, eventDialogFormSelectors.value, key) as T; // undefined when unseededThe cast to T hides it. Both test probes defend against it with value={value ?? ''} (EventDialogForm.test.tsx:21, EventDialog.test.tsx:2274), which is the tell.
Failure scenario: A consumer adds useField<string>('notes'). For events whose model already has notes it works; for one created earlier in the session it doesn't — React logs "A component is changing an uncontrolled input to be controlled" on first keystroke. And because getDirtyValues uses Object.is(undefined, undefined), setting such a field back to undefined reads as clean, so clearing it never reaches the event.
Fix: Type the return as T | undefined, or add a defaultValue option to FieldOptions that seeds the key on first use.
7. 🟡 Custom model properties colliding with a built-in form key are silently unreachable
Location: packages/x-scheduler/src/internals/components/event-dialog/FormContent.tsx:131
return {
...(model ? getCustomEventProperties(model) : {}), // custom first…
title: occurrence.title,
startDate: fmtDate(occurrence.displayTimezone.start), // …then overwrittengetCustomEventProperties only filters the 16 SchedulerEvent keys — not startDate, startTime, endDate, endTime, resourceId, recurrenceSelection, rruleDraft. Those get seeded then clobbered, and getDirtyValues(BUILT_IN_FORM_KEYS) drops them at submit.
Failure scenario: A user model with a domain field startDate: '2025-01-01' ("project kickoff"). A section calling useField('startDate') reads the event's formatted start date instead, and writes go nowhere. No data is lost — the merge in updateEvents preserves the model's value — but the field is permanently unaddressable through the new API with no warning.
Fix: Covered by the dev warning in finding 3, or namespace the built-in form keys.
8. ℹ️ A failed validateAll() on a hidden tab is a silent dead end
FormContent.tsx:198 returns without switching tabs, focusing, or scrolling. GeneralTab stays mounted (EventDialogTabPanel uses hidden + display: none), so its validators run — but the message renders in an invisible panel. With shouldEventRequireResource and no resource, switching to Recurrence and pressing Save does nothing visible. Pre-existing, but the newly centralized validateAll() is the natural place to add "switch to the tab holding the first error and focus it".
Related and also pre-existing: startTime registers a range validator (DateTimeSection.tsx:84) but its TextField renders no error/helperText, so a same-day inverted time blocks Save with no message anywhere. You've already tracked this as #23285 — flagging only because the PR rewrites exactly this code, so the two-line fix is cheap here.
Tests (6)
1. 🟠 Nothing asserts that the placeholder is not pushed on non-placeholder keys
Location: packages/x-scheduler/src/internals/components/event-dialog/usePushPlaceholder.ts:36
The PR inverts the wiring: previously only the two date/resource handlers called pushPlaceholder; now every write in the form reaches onValuesChange, and the only thing stopping a title, description, rruleDraft, or custom-field keystroke from rewriting the creation placeholder is the six-key PLACEHOLDER_KEYS set. All four creation tests exercise keys that are in the set. The negative side is unasserted.
Failure scenario: Drop the guard or add a key to the bag, and every character typed in the title writes a fresh placeholder object into the scheduler store — recomputing surfaceType from allDay and resetting resourceId — churning the ghost occurrence's identity per keystroke. The existing StateWatcher probes cannot catch this: they select scalars and only fire on value change, so a spurious push with identical scalars is invisible. Needs a spy on setOccurrencePlaceholder, or a watcher on placeholder object identity.
2. 🟠 Validation now depends on sections being mounted, and the hidden-tab case is untested
Location: packages/x-scheduler-premium/src/event-calendar-premium/tests/EventDialog.test.tsx:606
Before, handleSubmit ran validateRange and the required-resource check imperatively and unconditionally. Now validateAll() only runs validators that a currently mounted useField registered. That works today solely because EventDialogTabPanel is a styled('div') using the hidden attribute rather than conditional rendering — an implementation detail of another file, asserted nowhere.
Failure scenario: Someone changes EventDialogTabPanel to unmount the inactive panel (a routine perf or a11y refactor — the recurrence panel is already conditionally rendered). useField's cleanup unregisters both range validators and the resource validator, validateAll() returns true, and events save with end < start and no resource despite shouldEventRequireResource. Every existing test submits from the General tab, so the entire suite stays green. The new co-visible-errors test is the natural place to add the sibling case.
3. 🟠 The startTime half of the new range validator has zero coverage
Location: packages/x-scheduler/src/internals/components/event-dialog/DateTimeSection.tsx:84
createRangeValidator is new in this PR and registered twice. validateRange returns { field: 'startTime' } only when start day === end day and end <= start with allDay === false. Every start time interaction in the premium suite uses a valid time; the only range test is a date inversion.
Failure scenario: A regression inverting the ?.field === field comparison — so startDate reports the time error and vice versa — goes completely undetected. Writing this test is also what would have surfaced the missing helperText in finding 8.
4. 🟡 The store lifecycle across two editing sessions is asserted in a comment, never in a test
Location: packages/x-scheduler/src/internals/components/event-dialog/form/EventDialogFormContext.tsx:39
The comment claims "the store lives exactly as long as one editing session". Both the seed and the store use useRefWithInit, i.e. captured once on mount; the guarantee rests entirely on createModal setting state.data → null on close, an unrelated file's implementation detail.
Failure scenario: onOpen fires for occurrence B while the dialog is already open for A (setState({ isOpen: true, data: B }) keeps data non-null, so FormContent does not remount). The store keeps A's seed and A's dirty set, so submitting writes A's edited custom fields onto B. Before this PR custom fields were resolved from the live model at submit time, so this corruption vector is new. A test that opens for A, edits a custom field, closes, reopens for B, and asserts B's seeded values plus the absence of A's edit would pin both the remount assumption and the discard.
5. 🟡 "Only edited custom fields are merged" is pinned only on the recurring path
Location: packages/x-scheduler-premium/src/event-calendar-premium/tests/EventDialog.test.tsx:2268
The two #22874 scope-dialog tests genuinely enforce it for updateRecurringEvent. On the updateEvent path, should preserve custom data when editing a non-recurring event passes identically whether the value came from the live model or from an unfiltered spread, and the new should save a custom field edited through useField only asserts the edited key.
Failure scenario: getDirtyValues is replaced by a plain state.values spread, or BUILT_IN_FORM_KEYS stops being passed. Every seeded custom field is then written back explicitly on every non-recurring save — resurrecting the exact #22874 stale-snapshot bug for updateEvent while both regression tests stay green.
Fix: One assertion in the new test — seed a second, untouched custom key and assert it is absent from the updateEvent changes object, not merely equal to its old value.
6. 🟡 A non-empty string[] error never reaches a render in any test, and one consumer swallows it
Location: packages/x-scheduler/src/internals/components/event-dialog/form/EventDialogFormStore.test.ts:133
The store test covers null and [] but never ['a','b'], and every FieldProbe validator returns a plain string.
Failure scenario: A section returns ['Too long', 'Invalid characters']. ResourceAndColorSection.tsx:161 does typeof error === 'string' ? error : undefined — the array is discarded, nothing renders, and Save stays permanently inert. FormContent:310 and DateTimeSection:117 pass it straight to MUI helperText, rendering Too longInvalid characters with no separator. One end-to-end test pins which of these is intended. (See Simplifications 3 for the fix that makes this unrepresentable.)
7. ℹ️ eventDialogFormSelectors.test.ts asserts a property read, not the contract the selectors exist for
The whole file asserts state.values[key] and state.errors[key] — already implied by the store's own tests. The reason for createSelector + useStore(store, selector, key) is per-key subscription: a component bound to field A must not re-render when B is written. Nothing tests that, so a refactor to useStore(store, s => s.values) would make every field re-render on every keystroke with the suite green. A render-count probe (two FieldProbes, type into one, assert the other's count is unchanged) turns this file into a real test.
Simplifications (7)
1. 🟠 values: Record<string, unknown> discards the EventDialogFormValues interface that already exists, forcing seven casts
Location: packages/x-scheduler/src/internals/components/event-dialog/form/EventDialogFormStore.ts:7
Every read site re-asserts the type that was thrown away: FormContent.tsx:202, DateTimeSection.tsx:76, usePushPlaceholder.ts:40, RecurrenceTab.tsx:205, RecurrenceTab.tsx:288 — plus two unchecked as T inside useField.ts:23,30. This is also a regression from the typed ControlledValue interface the PR removes.
Failure scenario: Rename resourceId → resource in EventDialogFormValues and the compiler stays silent at all five sites; values.resourceId becomes undefined at runtime and the placeholder silently loses its resource. Separately, useField<number>('title') compiles today, and FormContent.tsx:211's values.title.trim() then throws at submit.
Fix: values: EventDialogFormValues in EventDialogFormState. The interface already has extends Record<string, unknown>, so custom keys still type-check, and utils.ts has no import back into form/, so no cycle. That deletes all five casts. Then type the key as keyof EventDialogFormValues | (string & {}) so typos surface. There is precedent for going further — SchedulerStore<TEvent, TResource, State, Parameters> is generic over the user's event shape, and Base UI's Form<FormValues> is generic over values.
2. 🟠 Three hand-maintained key lists describe one vocabulary with no compile-time link
Location: packages/x-scheduler/src/internals/components/event-dialog/utils.ts:36
The 11 strings in BUILT_IN_FORM_KEYS are exactly the 11 members of EventDialogFormValues 18 lines above, and nothing enforces that. PLACEHOLDER_KEYS (usePushPlaceholder.ts:12) is a third untyped Set<string> over the same vocabulary. The codebase already solves this exact problem — SchedulerStore.utils.ts:61 uses const EVENT_PROPERTIES_LOOKUP: { [P in keyof SchedulerEvent]-?: true } followed by Object.keys(...), where the -? mapped type makes a missing key a compile error.
Failure scenario: A follow-up adds location: string to EventDialogFormValues and forgets BUILT_IN_FORM_KEYS. getDirtyValues then classifies location as a custom field and spreads it into metaChanges alongside the built-in handling — a silent double-write on the recurring path, with no test and no type error.
Fix: Split EventDialogBuiltInFormValues (no index signature) from type EventDialogFormValues = EventDialogBuiltInFormValues & Record<string, unknown>, declare BUILT_IN_FORM_KEYS_LOOKUP: { [P in keyof EventDialogBuiltInFormValues]-?: true }, derive the Set from it, and type PLACEHOLDER_KEYS as ReadonlySet<keyof EventDialogBuiltInFormValues>. The current extends Record<string, unknown> is precisely what defeats keyof today.
3. 🟠 The validation contract diverges from Base UI's Field/Form in three ways that each need a breaking change to fix later
Location: packages/x-scheduler/src/internals/components/event-dialog/form/useField.ts:14
Users will pattern-match this against Base UI's Field.Root — reasonably, since this is a Base UI-built library and ResourceAndColorSection imports Toggle/ToggleGroup from it. Three gaps, sharing one root cause:
// this PR
validate?: (value: T, allValues: Record<string, unknown>) => string | string[] | null;
validateAll: () => boolean;
// Base UI Field.Root — same params, same sync return, plus:
validate?: (value, formValues) => string | string[] | null | Promise<string | string[] | null>;- Async. Signature-identical to Base UI but rejects the
Promiseform it advertises. If a user casts around it,EventDialogFormStore.ts:100'sresult != nullis true for a pending Promise, soerrors[key]becomes a thenable,validateAll()returnsfalseforever, andhelperTextgets handed a Promise.validateAll(): booleanis the real lock-in here, notFieldOptionsbeing an object — andhandleSubmitis alreadyasync, so making itPromise<boolean>now costs nothing. - Error shape. Base UI normalizes before exposing:
{ error: string; errors: string[] }, so consumers never branch on a union. This PR leaksstring | string[]to all four render sites, and only one handles it (see Tests 6). The read type is alsostring | string[] | undefinedwhile the write type isstring | string[] | null— two spellings of "absent" in one API. - Localization. Every error the Scheduler produces today comes from
EventDialogLocaleText(requiredResourceError,startDateAfterEndDateError, present in all 41 locale files). A custom section author cannot add keys there, so the contract invites hardcoded English inside a localized dialog. Wideningstring→React.ReactNodelater breaks consumers —ResourceAndColorSection.tsx:161already doestypeof error === 'string'.
Fix: Adopt Base UI's semantics wholesale — | Promise<…> on the validator, async validateAll(), normalized { error, errors } on read, and React.ReactNode as the message type. That also gives you validationMode + submitAttempted re-validation, which deletes Bugs 1's clearErrors() sledgehammer.
4. 🟡 useField is the only unprefixed name in the module, and it collides with a x-date-pickers internals export
Location: packages/x-scheduler/src/internals/components/event-dialog/form/useField.ts:21
Its own siblings are EventDialogFormStore, EventDialogFormContext, eventDialogFormSelectors, and its options type is EventDialogFormFieldOptions — only the hook drops the prefix. Meanwhile @mui/x-date-pickers/internals exports a completely different useField (the engine behind DateField/TimeField, surrounded by ~14 useField*.ts files). In this monorepo "field" already means a segmented date input.
Failure scenario: A dialog section rendering both a DateField and a form field needs import { useField as useFormField }; and MUI X can never promote the Pickers hook to a public useField. Renaming after #22871 is a public breaking change. Also note the hook declares no return type — a consumer writing function useRequiredField(key) { return useField(key, {...}) } cannot annotate it, cannot put it in a props interface, and ends up with ReturnType<typeof useField<string>>.
Fix: useEventDialogFormField, with exported UseEventDialogFormFieldParameters<T> / UseEventDialogFormFieldReturnValue<T> per the repo convention (useButton.ts, useTimeGridEvent.ts).
5. 🟡 Six exports on the @mui/x-scheduler/internals boundary have no consumer
Location: packages/x-scheduler/src/internals/components/event-dialog/index.ts:18-26
I grepped both scheduler packages and docs/: only useEventDialogFormContext and useField are actually imported (by RecurrenceTab.tsx:36-37). Unused: EventDialogFormStore, eventDialogFormSelectors, EventDialogFormState, EventDialogFormValidator, EventDialogFormStoreOptions, EventDialogFormFieldOptions. The tests all import relatively, so they don't justify them either. Exporting the raw EventDialogFormContext alongside the hook also invites a consumer to useContext it and skip the null check the hook exists to provide.
Fix: Keep useEventDialogFormContext and useField; drop the rest until #22871 exposes the surface deliberately, as the PR description says it will.
6. 🟡 getRruleDraft() at 10 call sites exists only because setValues dropped the functional-updater form
Location: packages/x-scheduler-premium/src/internals/components/event-dialog/RecurrenceTab.tsx:203
The removed setControlled(prev => …) gave every handler a guaranteed-fresh prev. setValues(changes) takes a plain object, so the tab hand-rolls getRruleDraft() (read + cast) and calls it at ten sites while also subscribing to the same key via useField('rruleDraft') at line 202 — two access paths to one value in one component, plus an as string cast at line 288 for endDate, which isn't subscribed at all.
Failure scenario: A new handler naturally reaches for the rendered rruleDraft instead of getRruleDraft() and silently drops a write when two setValues land in the same event handler.
Fix: ~4 lines in the store — setValues(changes: Partial<TValues> | ((prev: TValues) => Partial<TValues>)). Deletes getRruleDraft and both casts, and restores the idiom the file was written against.
7. ℹ️ Three smaller shape items
initialValuesin subscribable store state (EventDialogFormStore.ts:11) though it is written once and read in exactly one place. Being inEventDialogFormStatemeans it's re-spread on every keystroke byStore.setStateand scanned byStore.update'sObject.isloop. Make itprivate readonlyand drop it from the state type — ~4 lines, and the state type becomes an honest description of what's reactive.initialValuesmemoized twice —FormContent.tsx:121wraps the seed builder inuseRefWithInit, thenEventDialogFormContext.tsx:39wraps store construction inuseRefWithInitagain and ignores the prop after mount. One is redundant.eventDialogFormSelectors.ts— 7 lines, 2 selectors, one consumer, its own file and its own test file and anindex.tsexport. The separate-selectors-file idiom earns its keep inscheduler-selectorsat ~30 selectors; at 2 it doesn't. Folding them intoEventDialogFormStore.tsremoves a file, an import edge, a test file and an export.
Docs (5)
1. 🟠 The clearErrors comment says "range errors", the code clears all of them
Location: packages/x-scheduler/src/internals/components/event-dialog/DateTimeSection.tsx:92
// Editing any date or time field invalidates the range errors as a whole.
clearErrors() is this.set('errors', {}) — form-wide, including other sections'. A maintainer reading this will not look here when debugging the vanishing resource error from Bugs 1. The comment is doubly imprecise: the all-day Switch (line 171) also affects range validity and does not clear.
Fix: Either fix the code per Bugs 1, or say what it does: // Any date or time edit can flip the range validity, and the store has no per-group clearing, so this wipes every error — including other sections'.
2. 🟡 Error 297 doesn't follow the three-part rule; its direct sibling does
Location: docs/public/static/error-codes.json:297
I verified the string matches the throw site character-for-character, that it's a genuinely new throw site (so the update-in-place rule doesn't apply), and that there's no duplicate. The problem is content — AGENTS.md requires what happened, why it's a problem, and how to fix it. This one does only the first. Entry 207, the directly analogous hook in the same dialog, does all three:
"MUI X Scheduler: useEventDialogStyledContext must be used within an EventDialogStyledContext.Provider. The component requires access to dialog styling and locale information. Ensure the component is rendered inside an EventDialog component."
Failure scenario: A developer rendering a custom section outside the dialog gets Minified MUI X error #297 in production, looks it up, and learns only the hook name.
Fix: Append The section needs access to the draft form values. Ensure the component is rendered inside the event dialog form. Since the argument count and semantics are unchanged, entry 297 can be edited in place per the repo rule.
3. 🟡 BUILT_IN_FORM_KEYS's doc contradicts the comment at its only call site
Location: packages/x-scheduler/src/internals/components/event-dialog/utils.ts:33
* Form keys handled by the built-in submit logic. Every other key in the values
* bag is a custom field and is spread onto the event as-is.
Only the dirty non-built-in keys are spread — FormContent.tsx:207 uses getDirtyValues, and the comment directly above it says the opposite of this one.
Failure scenario: A maintainer adding a custom-field section seeds a default via initialValues and is surprised that an untouched-but-seeded field never reaches onEventsChange.
Fix: "…every other key is a custom field; the ones the user edited are spread onto the event as-is."
4. 🟡 The provider comment asserts a lifecycle guarantee nothing enforces
Location: packages/x-scheduler/src/internals/components/event-dialog/form/EventDialogFormContext.tsx:37
// The seed is captured on mount only — the dialog content unmounts when the
// dialog closes, so the store lives exactly as long as one editing session.
The second clause is an assumption about the dialog not being kept mounted, not an invariant this code guarantees — and EventDialogContent is exported from /internals. Combined with Tests 4's retarget path, the comment tells the next reader that a failure mode which is actually reachable cannot happen.
Fix: State the dependency (relies on the dialog content unmounting on close), or enforce it with key={occurrence.id} on FormContent.
5. ℹ️ Four smaller doc gaps
PLACEHOLDER_KEYS(usePushPlaceholder.ts:12) is a new, silent gate on the whole hook with no comment, and must stay in sync with whatcomputeRangereads plusresourceId. A future placeholder-affecting field will silently stop updating the ghost occurrence with nothing pointing here. One line fixes it.useField'svalidatedoc (useField.ts:11) says "ornullwhen the value is valid" but the store also treats[]as valid, with a dedicated test. No@returnsfor the{ value, setValue, error }tuple, and no mention thaterrorcan bestring[].- Half the store's public API is undocumented —
setValue,setValues,clearErrors,registerValidator,unregisterValidator. The undocumented ones are exactly the non-obvious ones:setValuesclears only the written keys' errors,clearErrorsclears all of them, and the validator registry deliberately lives on the instance rather than in state so registering doesn't notify subscribers. - Stale vocabulary at
RecurrenceTab.tsx:226— "so the controlled form keeps a consistent shape" refers to the removedcontrolled/ControlledValuestate. Should read "the form store" / "therruleDraftvalue".
Also worth two lines in the store's docblock: not building on Base UI's Form/Field is the right call here — every input is MUI Material, and the store holds non-DOM values (rruleDraft, recurrenceSelection) that Field can't represent. Writing that down saves the next reviewer from re-litigating it.
Verdict
Approve after nits — nothing blocks merge; the two I'd fix before merging are the form-wide clearErrors() that erases a still-valid error the PR's own new test just made visible, and the two subscriptions hoisted to the dialog root that undo the uncontrolled-title render savings. The remaining cluster (typed values bag, reserved-key guard, Base UI-aligned validation) is contract hardening that is much cheaper now than after #22871 exposes it.
🤖 Review generated with Claude Code
…ifecycle-contract
Bug fixes:
- Clear only the range errors when editing date/time fields or toggling
all-day, instead of wiping every section's error
- Extract the title field into TitleSection so typing no longer
re-renders the whole dialog content
- Spread metaChanges before id/start/end on every submit path so
built-in properties always win
- Warn in dev when a field key collides with a built-in event property,
and when a custom model property collides with a built-in form key
- Treat an empty error string as valid, like the empty array
- Clone the seed so the dirty baseline is not externally mutable
- Remount FormContent when the dialog is retargeted (key={occurrence.id})
Test coverage:
- Placeholder is not pushed on non-placeholder keys
- Hidden-tab validators still run on submit
- startTime half of the range validator blocks submit
- Untouched custom fields stay out of the updateEvent payload
- Per-key subscription does not re-render other fields
Simplifications:
- Type the values bag (EventDialogBuiltInFormValues + lookup-derived
BUILT_IN_FORM_KEYS), removing the scattered casts
- Rename useField to useEventDialogFormField with exported
parameter/return types
- setValues accepts a functional updater, removing getRruleDraft
- Move initialValues out of the reactive state, fold the selectors into
the store file, drop the unused internals exports and the redundant
seed memoization
Docs: complete error 297 per the error-message guidelines and fix the
imprecise comments flagged in the review.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V1R9sqMQji1Vzj31BaLk3j
|
Applied in d1c3659 and f1e1bff — 24 of the 25 findings. Bugs: One pushback on Bugs 2: the placeholder half isn't a regression. Tests: added the negative placeholder assertion (spy on Simplifications: values bag typed via Validation contract (Simplifications 3 / Tests 6 / Bugs 6): adopted now rather than deferred, since changing the semantics is free while the surface is internal. Validators can be async ( Deferred to #22871, where the contract goes public: whether custom fields get their own sub-bag ( |
…internal
- Validators can be async: validateAll() resolves with the validity and
never stores a pending promise as an error
- Error messages are React.ReactNode and normalized on read: the field
exposes { error, errors } so consumers never branch on a union
- useEventDialogFormField accepts defaultValue, seeding absent keys
without marking them dirty so an untouched default is not submitted,
while clearing a seeded field back to undefined is
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V1R9sqMQji1Vzj31BaLk3j
…ifecycle-contract
…allback - Remount FormContent on `occurrence.key` instead of `occurrence.id`: occurrences of the same recurring event share the id, so retargeting the open dialog between them kept the previous occurrence's draft. Pinned by a retarget test that fails with the id-based key. - Only fall back to `defaultValue` while the key is not seeded yet, so an explicit `undefined` write no longer renders the default while the dirty values submit `undefined`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1R9sqMQji1Vzj31BaLk3j
…ract' of github.com:rita-codes/mui-x into 22868-scheduler-edit-dialog-form-context-lifecycle-contract
…ifecycle-contract
In a real browser, the remount moves the focus to the new title input through a natively dispatched focus event that lands outside act() and trips fail-on-console, poisoning the next test's DOM. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1R9sqMQji1Vzj31BaLk3j
flaviendelangle
left a comment
There was a problem hiding this comment.
PR review
The refactor is sound: I traced the draft-store lifecycle through createModal (data is snapshotted on open, and the creation placeholder's key is the constant 'occurrence-placeholder', so key={occurrence.key} remounts on genuine retargets without wiping the draft while typing during creation), and the full scheduler suite, tsgo, and ESLint all pass locally. Nothing here is merge-blocking. The findings are one real test gap around the close→reopen lifecycle the whole design rests on, plus a few cleanups.
Bugs (1)
1. ℹ️ The new "invalid end time" test pins a silent failure without a pointer to the tracking issue
Location: packages/x-scheduler-premium/src/event-calendar-premium/tests/EventDialog.test.tsx:238
await user.type(screen.getByLabelText(/end time/i), '09:00');
await user.click(screen.getByRole('button', { name: /save/i }));
expect(onEventsChange.called).to.equal(false);createRangeValidator('startTime') writes errors.startTime, but the Start time TextField renders no error/helperText — only Start date does. The submit is blocked with zero user feedback. This is pre-existing (the old setErrors({ startTime: … }) had the same dead end) and the PR body tracks it in #23285, but the new test now asserts the silent block with no in-file marker, so the next reader has no reason to suspect it's a known gap.
Failure scenario: A user sets 10:00–09:00 on the same day, clicks Save, and nothing happens — no error, no close. A future contributor reads this test as "blocking with no message is the intended contract".
Fix: Add // TODO(#23285): the Start time field does not surface this error yet. above the assertion, or assert the (currently missing) message so the test starts failing once #23285 lands.
Tests (2)
1. 🟠 Nothing covers the close→reopen cycle the ephemeral store depends on
Location: packages/x-scheduler/src/internals/components/event-dialog/form/EventDialogFormContext.tsx:40
// The seed is captured on mount only. This relies on the dialog content
// unmounting when the dialog closes and remounting when it is retargeted
// (enforced by the `key` on `FormContent`), so the store lives exactly as
// long as one editing session.The unmount half of that invariant is load-bearing and untested. Every dialog test — including the new retarget one — renders EventDialogContent open directly or drives it with setProps, bypassing createModal.Provider entirely; the retarget test only exercises the key half. The unmount currently holds because createModal gates render() on state.data && anchorRef.current and onClose sets data: null, but no test pins it. Any future change that keeps the dialog mounted for an exit transition (a natural thing to want, since today the dialog vanishes with no close animation) would silently make drafts persist across sessions.
Failure scenario: A user opens event A, types "cancel me" into the title, presses Escape, then reopens event A — the title shows "cancel me", and clicking Save writes an edit the user explicitly abandoned. No test in the suite fails.
Fix: Add one test that mounts a real view (EventCalendar / EventDialogProvider), clicks an event to open the dialog through the trigger, edits the title, closes with Escape, reopens the same event, and asserts the title is back to the model value.
2. 🟡 The only coverage of the new remount key is jsdom-only
Location: packages/x-scheduler-premium/src/event-calendar-premium/tests/EventDialog.test.tsx:117
// In a real browser, the remount moves the focus to the new title input through a
// natively dispatched focus event that lands outside `act()` and trips fail-on-console.
it.skipIf(!isJSDOM)(
'should re-seed the form when the dialog is retargeted to another occurrence of the same event',key={occurrence.key} is the behavioral core of the PR, and the skip reason is itself the interesting part: retargeting now yanks focus into the new title input via titleInputRef. That focus transition — a real, user-visible behavior change, since the previous non-remounting dialog left focus alone — is exactly what browser mode would exercise and is never run there.
Failure scenario: A regression that strands focus on the detached (unmounted) title input, or that focuses nothing at all after a retarget, ships unnoticed: keyboard users lose their place and screen readers announce nothing.
Fix: Keep the jsdom test, and add a browser-only companion (it.skipIf(isJSDOM)) that retargets and asserts document.activeElement is the new title input — wrapping the setProps in act()/waitFor to keep the native focus event inside the act boundary.
Simplifications (2)
1. 🟡 handleChangeWeeklyDays returns {} to mean "no change", which setValues still treats as a write
Location: packages/x-scheduler-premium/src/internals/components/event-dialog/RecurrenceTab.tsx:335
const isRemoving = byDay.includes(dayCode);
if (isRemoving && byDay.length === 1) {
return {};
}The pre-refactor code returned prev, which React's useState bailed out on. setValues({}) has no such bail-out: values = { ...this.state.values } is always a fresh object, so update() sees a changed key, setState runs, every subscriber's selector is re-evaluated, and onValuesChange fires with an empty changedKeys that usePushPlaceholder then has to filter out. No user-visible effect today (the per-key selectors are Object.is-equal so React skips the re-renders), but {} reads as "write nothing" while doing the opposite, and the store has no test for the empty-changes case.
Failure scenario: A maintainer copies this return {} idiom into a hot handler, or adds an onValuesChange consumer that doesn't guard on changedKeys.length, and gets work on every no-op click of the last-selected weekday.
Fix: Early-return from the handler instead of routing a no-op through the store, and/or make setValues bail out when changedKeys.length === 0 — with a store unit test pinning that an empty changes object notifies nobody.
2. 🟡 initialValues is rebuilt on every FormContent render, and the comment justifying it is inaccurate
Location: packages/x-scheduler/src/internals/components/event-dialog/FormContent.tsx:107
// No memoization needed: the provider captures the seed on mount only,
// and rebuilding it on the rare outer re-render is cheap.
const fmtDate = (d: SchedulerProcessedDate) => adapter.formatByString(d.value, 'yyyy-MM-dd');The outer re-render isn't rare during creation. usePushPlaceholder() is now called in FormContent itself and subscribes it to schedulerOccurrencePlaceholderSelectors.value, so each date/time keystroke pushes a placeholder and re-renders FormContent. That rebuilds four adapter.formatByString calls, a getCustomEventProperties(model) walk over the whole event model, and (in dev) the collision-warning loop — all thrown away, since useRefWithInit ignores the prop after mount. Not a perf problem worth blocking on, but the comment tells the next reader the opposite of what happens.
Failure scenario: Someone later adds a genuinely expensive derivation to the seed (a recurrence expansion, a resource lookup) trusting "rare outer re-render", and it runs on every keystroke of an event creation.
Fix: Wrap the seed in useRefWithInit (it's mount-only by contract anyway) and correct the comment to say the provider ignores later values, or move the placeholder subscription out of FormContent so the outer component really is stable.
Docs (1)
1. ℹ️ The PR description describes an earlier iteration of the API
Location: packages/x-scheduler/src/internals/components/event-dialog/form/useEventDialogFormField.ts:64
export function useEventDialogFormField<T = unknown>(
key: EventDialogFormFieldKey,
parameters: UseEventDialogFormFieldParameters<T> = {},The description calls the hook useField(key, { validate }) and names the options type FieldOptions; neither exists — the exports are useEventDialogFormField and UseEventDialogFormFieldParameters. It also lists the store state as values / initialValues / errors, but initialValues is a private instance field, deliberately outside the state so it doesn't notify subscribers.
Failure scenario: #22871 is meant to build the public General-tab slot against "this contract", and its author reads the linked description first — then greps for useField and finds nothing.
Fix: Update the description to the shipped names before merge, since it's the hand-off document for #22871.
Verdict
Approve after nits — no correctness regression survived verification; the one finding worth acting on before merge is the missing close→reopen test for the ephemeral store's unmount invariant.
🤖 Review generated with Claude Code
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1R9sqMQji1Vzj31BaLk3j
Part of #22866. Closes #22868.
Replaces the prop-drilled
controlled/errorsstate in the event edit dialog with an ephemeral draft form store (base-uiStore) plus a provider/context, and auseEventDialogFormField(key, { validate })hook that any section — built-in or custom — uses to read/write a field and register validation. This is the contract the General-tab slot (#22871) will build custom sections against.What changed
EventDialogFormStore(event-dialog/form/): generic values bag (values/errorsin the reactive state; the seed is kept as a private baseline for dirty-checking), validator registry on the instance, asyncvalidateAll()(first error per field, all failing fields at once),getDirtyValues(). Created on dialog open viauseRefWithInit, discarded on close, and re-seeded when the dialog is retargeted to another occurrence while open (key={occurrence.key}remount) — a staging buffer, not a slice ofSchedulerStore.useEventDialogFormField: one hook for read/write + optional validator (sync or async,React.ReactNodemessages, normalizederror/errorson read), registered on mount and cleaned up on unmount, plus adefaultValueoption that seeds keys absent from the event model without marking them dirty.UseEventDialogFormFieldParametersstays an options object so agetValuebridge for form libraries can be added later without a breaking change.FormData, now controlled). The premiumRecurrenceTabreads/writesrecurrenceSelection/rruleDraftthrough the store;RecurrenceTabRendererPropsshrinks to{ occurrence, tabValue }andControlledValueis gone.onValuesChangefeedsusePushPlaceholder, filtered to the keys that affect the placeholder, so future custom sections get it for free and title/description keystrokes don't trigger it.modelLookup+getCustomEventProperties) and only the edited ones are merged into the changes payload (getDirtyValues), so untouched fields keep resolving against the live model on the recurring-split paths — the [scheduler] Carry custom event data through the edit dialog submit #22874 stale-snapshot regression tests pass unchanged.Behavior notes
@mui/x-scheduler/internals; [scheduler] Edit dialog: General-tab slot for composing sections (built-in + custom) #22871 will expose the public surface deliberately.Follow-ups (out of scope)