Skip to content

[scheduler] Introduce the edit dialog form context & lifecycle contract - #23284

Merged
rita-codes merged 20 commits into
mui:masterfrom
rita-codes:22868-scheduler-edit-dialog-form-context-lifecycle-contract
Aug 5, 2026
Merged

rita-codes merged 20 commits into
mui:masterfrom
rita-codes:22868-scheduler-edit-dialog-form-context-lifecycle-contract

Conversation

@rita-codes

@rita-codes rita-codes commented Aug 3, 2026 •

Copy link
Copy Markdown
Member

Part of #22866. Closes #22868.

Replaces the prop-drilled controlled/errors state in the event edit dialog with an ephemeral draft form store (base-ui Store) plus a provider/context, and a useEventDialogFormField(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 / errors in the reactive state; the seed is kept as a private baseline for dirty-checking), validator registry on the instance, async validateAll() (first error per field, all failing fields at once), getDirtyValues(). Created on dialog open via useRefWithInit, 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 of SchedulerStore.
  • useEventDialogFormField: one hook for read/write + optional validator (sync or async, React.ReactNode messages, normalized error/errors on read), registered on mount and cleaned up on unmount, plus a defaultValue option that seeds keys absent from the event model without marking them dirty. UseEventDialogFormFieldParameters stays an options object so a getValue bridge for form libraries can be added later without a breaking change.
  • Sections migrated to the same contract: date/time (range validation now lives here as registered validators), resource & color (required-resource validator), description and title (move off FormData, now controlled). The premium RecurrenceTab reads/writes recurrenceSelection/rruleDraft through the store; RecurrenceTabRendererProps shrinks to { occurrence, tabValue } and ControlledValue is gone.
  • Placeholder push centralized: the provider's onValuesChange feeds usePushPlaceholder, filtered to the keys that affect the placeholder, so future custom sections get it for free and title/description keystrokes don't trigger it.
  • Submit: custom fields are seeded from the raw event model (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

  • The range error and the required-resource error can now show simultaneously (the previous submit logic early-returned after the first failing check). Retargeting the dialog to another occurrence now re-seeds the form and moves focus to the new title input. No other behavior or DOM changes.
  • No public API — everything stays in @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)

rita-codes and others added 4 commits August 3, 2026 12:12
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
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
@rita-codes rita-codes added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. scope: scheduler Changes related to the scheduler. labels Aug 3, 2026
@code-infra-dashboard

code-infra-dashboard Bot commented Aug 3, 2026 •

Copy link
Copy Markdown

Deploy preview

https://deploy-preview-23284--material-ui-x.netlify.app/
QR code for https://deploy-preview-23284--material-ui-x.netlify.app/

Bundle size

Bundle Parsed size Gzip size
@mui/x-data-grid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-pro 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-premium 0B(0.00%) 0B(0.00%)
@mui/x-charts 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 0B(0.00%) 0B(0.00%)
@mui/x-charts-premium 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 0B(0.00%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)
@mui/x-scheduler 🔺+2.81KB(+0.78%) 🔺+980B(+1.02%)
@mui/x-scheduler-premium 🔺+2.73KB(+0.56%) 🔺+893B(+0.66%)
@mui/x-chat 0B(0.00%) 0B(0.00%)
@mui/x-license 0B(0.00%) 0B(0.00%)

Details of bundle changes


Check out the code infra dashboard for more information about this PR.

rita-codes and others added 6 commits August 3, 2026 12:42
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
…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

@flaviendelangle flaviendelangle left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 store

Both moved outward from leaves:

  • Title. At base it was uncontrolled — defaultValue={occurrence.title}, read back via new FormData() at submit — so typing caused zero React renders. It is now controlled from FormContentInner, which renders GeneralTab (→ DateTimeSection, ResourceAndColorSection, DescriptionSection) and RecurrenceTabRenderer.
  • Placeholder. At base usePushPlaceholder() was called in DateTimeSection:64 and ResourceAndColorSection:152. It now lives in the outer FormContent, so a placeholder write during event creation re-renders the provider and hands FormContentInner a 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, twice

Until 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 unseeded

The 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 overwritten

getCustomEventProperties 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 Promise form it advertises. If a user casts around it, EventDialogFormStore.ts:100's result != null is true for a pending Promise, so errors[key] becomes a thenable, validateAll() returns false forever, and helperText gets handed a Promise. validateAll(): boolean is the real lock-in here, not FieldOptions being an object — and handleSubmit is already async, so making it Promise<boolean> now costs nothing.
  • Error shape. Base UI normalizes before exposing: { error: string; errors: string[] }, so consumers never branch on a union. This PR leaks string | string[] to all four render sites, and only one handles it (see Tests 6). The read type is also string | string[] | undefined while the write type is string | 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. Widening string → React.ReactNode later breaks consumers — ResourceAndColorSection.tsx:161 already does typeof 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

  • initialValues in subscribable store state (EventDialogFormStore.ts:11) though it is written once and read in exactly one place. Being in EventDialogFormState means it's re-spread on every keystroke by Store.setState and scanned by Store.update's Object.is loop. Make it private readonly and drop it from the state type — ~4 lines, and the state type becomes an honest description of what's reactive.
  • initialValues memoized twice — FormContent.tsx:121 wraps the seed builder in useRefWithInit, then EventDialogFormContext.tsx:39 wraps store construction in useRefWithInit again 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 an index.ts export. The separate-selectors-file idiom earns its keep in scheduler-selectors at ~30 selectors; at 2 it doesn't. Folding them into EventDialogFormStore.ts removes 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 what computeRange reads plus resourceId. A future placeholder-affecting field will silently stop updating the ghost occurrence with nothing pointing here. One line fixes it.
  • useField's validate doc (useField.ts:11) says "or null when the value is valid" but the store also treats [] as valid, with a dedicated test. No @returns for the { value, setValue, error } tuple, and no mention that error can be string[].
  • Half the store's public API is undocumented — setValue, setValues, clearErrors, registerValidator, unregisterValidator. The undocumented ones are exactly the non-obvious ones: setValues clears only the written keys' errors, clearErrors clears 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 removed controlled/ControlledValue state. Should read "the form store" / "the rruleDraft value".

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

rita-codes and others added 2 commits August 3, 2026 17:56
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
@rita-codes

rita-codes commented Aug 4, 2026 •

Copy link
Copy Markdown
Member Author

Applied in d1c3659 and f1e1bff — 24 of the 25 findings.

Bugs: clearErrors now takes keys and both the date/time handlers and the all-day switch clear only the range errors; the title moved into its own TitleSection (leaf subscription, same shape as DescriptionSection); updateEvent now spreads metaChanges first so the three submit paths agree; useEventDialogFormField warns in dev on reserved event-model keys, and the seed builder warns on custom properties colliding with built-in form keys; '' is treated as valid; the seed is cloned; and FormContent is keyed on occurrence.id, which removes the retarget corruption vector at the root instead of testing around it.

One pushback on Bugs 2: the placeholder half isn't a regression. FormContentInner subscribes to the raw placeholder for handleSubmit (it did at base too), so a creation push re-rendered the dialog subtree before this PR as well. The new outer subscription only re-renders FormContent, which is cheap now that the seed isn't memoized there. The title half was real and is fixed.

Tests: added the negative placeholder assertion (spy on setOccurrencePlaceholder), the hidden-tab submit case, the startTime half of the range validator (blocks submit only — the missing helperText is #23285), the absent-untouched-custom-field assertion on updateEvent, and a render-count probe pinning the per-key subscription.

Simplifications: values bag typed via EventDialogBuiltInFormValues + a -? lookup deriving BUILT_IN_FORM_KEYS; the store is generic over TValues so the seven casts collapse into one documented boundary cast in the provider; useField → useEventDialogFormField with exported parameter/return types; setValues takes a functional updater (deletes getRruleDraft); initialValues is private readonly; the selectors are folded into the store file; the unused internals exports and the double memoization are gone.

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 (validateAll() resolves with the validity, a pending promise is never stored as an error), messages are React.ReactNode normalized on read ({ error, errors } — the field never leaks the string | string[] union, which also fixes the swallowed-array consumer), and defaultValue seeds absent keys without marking them dirty (an untouched default is not submitted; clearing a seeded field back to undefined is).

Deferred to #22871, where the contract goes public: whether custom fields get their own sub-bag (values.custom). Bugs 8 is tracked in #23285.

rita-codes and others added 6 commits August 4, 2026 18:08
…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
…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
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 flaviendelangle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rita-codes
rita-codes merged commit c37cd3e into mui:master Aug 5, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: scheduler Changes related to the scheduler. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[scheduler] Edit dialog: form context & lifecycle contract

2 participants