Skip to content

[scheduler] Refuse the batch when the auto-scheduling cascade clamps an unwritable date - #24002

Merged
rita-codes merged 12 commits into
mui:masterfrom
mustafajw07:fix/23936-scheduler-refuse-unwritable-batch
Oct 30, 2026
Merged

rita-codes merged 12 commits into
mui:masterfrom
mustafajw07:fix/23936-scheduler-refuse-unwritable-batch

Conversation

@mustafajw07

@mustafajw07 mustafajw07 commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Closes #23936

When the auto-scheduling cascade had to move an event whose date eventModelStructure can't write back, the date was dropped and the rest of the change applied, leaving the dependency broken. Event updates, addDependency and updateDependency now refuse the change and emit nothing. Dependency calls return the new datesNotWritable rejection reason, which the dependency dialog and drag-to-create show as an error.

New localeText keys, for teams that ship a custom localeText: unwritableEventMovedError, dependencyDatesNotWritableError and dependencyUpdateDatesNotWritableError.

Tests: 7 new store tests (8 cases) and 2 updated ones, 2 dependency dialog tests, 1 drag-to-create test, a unit test for getUserFacingErrorMessage, and the isDateWritable selector tests rewritten for unwritableDates.

Changelog

@mustafajw07

Copy link
Copy Markdown
Contributor Author

Hey @rita-codes, can I get a review on this?

@code-infra-dashboard

code-infra-dashboard Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Deploy preview

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 🔺+371B(+0.09%) 🔺+32B(+0.03%)
@mui/x-scheduler-premium 🔺+371B(+0.06%) 🔺+35B(+0.02%)
@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 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 Oct 27, 2026

@rita-codes rita-codes 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 fix is correct and well tested: the new store check runs before the plugin's commit(), before the strip loop and before any emission, the plugin's upfront check runs before updateDependencies, and I traced the engine to confirm the only single-bound contributions it produces are for the caller's own event (the allDay/timezone flips the issue describes), so nothing that used to apply legitimately is refused now. The four new tests cover the three paths of #23936 and fail against master's code (I ran them). The full scheduler jsdom suites pass (3,942 tests), with tsc, ESLint and Prettier clean. One change I'd like before merging: the new check belongs in the scheduling plugin, not in the core store, which with #24011 making dependencies opt-in would otherwise ship it to every consumer. That move also resolves the two message issues below. The rest are nits.

Bugs (3)

1. 🔴 Design: move the writability check into the plugin

Location: packages/x-scheduler-internals/src/internals/utils/SchedulerStore/SchedulerStore.ts:952

private checkContributedDatesWritable(
  contributed: SchedulerEventUpdatedProperties[] | undefined,
  deleted: ReadonlySet<SchedulerEventId>,
): Error | null {

handleEventsUpdate already returns { rejected: true, error } when the cascade would move a read-only event, and the store refuses the whole batch on that branch and skips commit(). Add the same check for an unwritable start/end right after the blocked one, with the event title in the error like cascadeBlocked does ("This change would move the read-only event "b"…"), and the warnOnce + datesNotWritableReason there too (export datesNotWritableReason from the store, or move it next to isDateWritable in the selectors). That removes checkContributedDatesWritable and its ?? call from SchedulerStore, which every consumer ships, including the community EventCalendar where no plugin ever contributes. With #24011 making dependencies opt-in, the rule belongs in the feature's code, not in the core store, and it also keeps the two PRs from both editing SchedulerStore.ts. The commitDependencyChange check stays (that path goes through applyCascade with skipScheduling), sharing the predicate and the warning with the new one. The current comment above checkContributedDatesWritable argues for the store doing it because it knows which entries came from the plugin; the plugin knows that better, since it produced them.

Failure scenario: A timeline without the dependencies feature, and every Event Calendar, carries 40 lines of dead code for a check that can never fire there.

Fix: As above. The two store tests then assert the plugin's warning text instead; the handleEventsUpdate JSDoc sentence "for a reason this hook can't predict (e.g. a date eventModelStructure can't write back)" goes, since the hook now predicts it.

2. 🟡 The dependency-path refusal has no dev warning, unlike every other datesNotWritable refusal

Location: packages/x-scheduler-internals-premium/src/internals/plugins/SchedulerSchedulingPlugin.ts:344

if (unwritable) {
  return { status: 'rejected', reason: 'datesNotWritable', eventId: unwritable.id };
}

checkCreatedDatesWritable, checkUpdatedDatesWritable and the new store check all pair the user error with a warnOnce(... datesNotWritableReason(state)) naming the getter-only property. cascadeBlocked has no warning because a read-only event is intentional configuration; a missing setter is the structural mistake the store surfaces in dev everywhere else.

Failure scenario: A developer wires eventModelStructure: { start: { getter } } and draws a dependency in the Timeline: they see the toast and nothing in the console pointing at eventModelStructure.start.

Fix: Resolved by finding 1 (one warning helper in the plugin, used by both checks).

3. 🟡 The store path blames "this date" for an edit that touched no date

Location: packages/x-scheduler-internals/src/internals/utils/SchedulerStore/SchedulerStore.ts:977

return createDatesNotWritableError();
// → "This date can't be changed, so the change was not applied."

The scenario this check exists for is updateEvent({ id: 'b', allDay: true }): the caller changed no date, the cascade did. The file's own precedent is createEventNotCreatableError ("unlike a move, nothing the user did necessarily touched a date… so the message doesn't blame 'this date' for it").

Fix: Resolved by finding 1: the plugin's error can name the event, e.g. "This change would move the event "b", whose dates can't be edited, so it was not applied."

Tests (2)

1. 🟡 The two new UI messages have no component test, while cascadeBlocked has both

Location: packages/x-scheduler-premium/src/event-timeline-premium/content/timeline-dependencies/dependency-dialog/DependencyDialogFormContent.tsx:159

const isLagRejected =
  (result.reason === 'cascadeBlocked' || result.reason === 'datesNotWritable') &&
  isLagChanged;

dependencyEditor.EventTimelinePremium.test.tsx covers cascadeBlocked on type, on lagAmount and "both changed → lag"; dependencyTerminals.EventTimelinePremium.test.tsx:1760 asserts the creation monitor's cascadeBlocked message in store.state.errors. grep datesNotWritable *.test.tsx finds nothing, so the new branch above and the two new strings are guarded only by the Record<Reason, string> types.

Failure scenario: Dropping || result.reason === 'datesNotWritable' puts the error on type instead of lagAmount for a lag change, with every test green.

Fix: createDependencyTimelineRenderer spreads its parameters into the component, so eventModelStructure: { start: { getter } } passes straight through. Copy the renderReadOnlyCascade trio with an overlapping writable target and a getter-only start: type → error on type; lag → error on lagAmount; drag-to-create → store.state.errors[0].error.message.

2. ℹ️ The store tests don't assert that nothing changed

Location: packages/x-scheduler-internals-premium/src/use-event-timeline-premium/tests/autoScheduling.EventTimelinePremiumStore.test.ts:682

Both store tests end with result.applied === false and onEventsChange not called, matching the sibling at line ~499. One line, expect(store.state.processedEventLookup.get('b')!.allDay).to.equal(false), would also pin that the refusal happened before any state write. Optional.

Simplifications (2)

1. 🟡 The "entry carries an unwritable bound" predicate is written twice

Location: packages/x-scheduler-internals-premium/src/internals/plugins/SchedulerSchedulingPlugin.ts:344

(['start', 'end'] as const).some(
  (property) =>
    property in entry && !schedulerEventSelectors.isDateWritable(this.store.state, property),
)

The same closure is in the store at SchedulerStore.ts:964. After finding 1 both copies live in the plugin (handleEventsUpdate and commitDependencyChange); a hasUnwritableDate(state, changes) next to isDateWritable in schedulerEventSelectors serves both, and removeUnwritableDates/datesNotWritableReason run the same test in loop form.

2. ℹ️ Two rejection types that differ by one literal

Location: packages/x-scheduler-internals-premium/src/models/dependency.ts:151

SchedulerDependencyDatesNotWritableRejection is SchedulerDependencyCascadeBlockedRejection with another literal; one { status: 'rejected'; reason: 'cascadeBlocked' | 'datesNotWritable'; eventId } keeps the reason narrowing the dialog relies on and collapses the three union edits. Optional.

Docs (2)

1. 🟡 Three JSDoc comments now say the opposite of the code

Location: packages/x-scheduler-internals/src/internals/utils/SchedulerStore/SchedulerStore.ts:916

 * back. Checked after the scheduling plugin's contributions are merged into `updated`, so a
 * cascaded successor (which always carries both bounds) is covered too: partially applying it

The engine reports only the bound it moves (auto-scheduling.ts pushes ...(startChanged ? { start } : {})), which is the whole premise of this PR. Two more: updateEvents' JSDoc still says "updatedEntries are … the dates the plugin clamped or cascaded, with any unwritable resize-style date already dropped" (line 573), now true only of the caller's own entries; and the store-facing EventTimelinePremiumStore.addDependency JSDoc (line 366) still enumerates the old rejection list ("…and dependencies needing a read-only event to move — see the returned SchedulerAddDependencyResult") while the plugin's was updated.

Fix: "…so a cascaded successor is covered too when it carries both bounds (a single clamped bound is refused by the plugin)"; "…with any unwritable resize-style date of the caller's own entries already dropped (a contributed one refuses the batch instead)"; add "or a date eventModelStructure can't write back to change" to the store's addDependency.

2. ℹ️ PR description, label and two long comment lines

The PR body is "Closes #23936" plus the checklist: no description of the three fixed paths, no test summary, no ## Changelog section although the fix is user-visible (a new datesNotWritable rejection reason and two new UI messages). The "Check PR type labels" check fails only because no type: label is set (I'll add type: bug and scope: scheduler). In SchedulerStore.ts, line 678 runs to 104 columns in a comment block wrapped at 100, and the edited updateEvents JSDoc leaves a 54-character line ("result then carries the rejection for the caller to") mid-paragraph; Prettier does not reflow comments.

Verdict

Request changes — the fix is right and pinned by tests that fail on master, but the check should live in the scheduling plugin, where #24011 is moving everything dependency-related, rather than in the core store every consumer ships.


🤖 Review generated with Claude Code

@rita-codes rita-codes added type: bug It doesn't behave as expected. and removed type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Oct 27, 2026
@mustafajw07

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review! I’ve addressed the feedback, moved the writability check into the scheduling plugin, added the missing tests, and cleaned up the duplicated logic and outdated JSDoc. I’ve pushed the changes, including the PR description and label updates. Ready for another review!

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: Mustafa Jawadwala <mustafajw07@gmail.com>
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 28, 2026
@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Move the new datesNotWritable messages to localeText, following the
user-facing error codes introduced on master.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 30, 2026
@mustafajw07

Copy link
Copy Markdown
Contributor Author

Hey @rita-codes, can you review this pr?

@rita-codes

Copy link
Copy Markdown
Member

PR review (round 2)

Nothing merge-blocking anymore. The round-1 blocker is done the way it was asked: the writability check lives in the scheduling plugin, SchedulerStore no longer carries it, and I traced the three paths of #23936 (an allDay flip, addDependency, updateDependency) to the veto that returns before commit(), updateDependencies() and any emission. The dev warning, the event-naming message, the merged rejection type, the component tests and the JSDoc fixes from round 1 are all in. The branch is on current master; TypeScript, ESLint and Prettier pass; the Scheduler suite passes (212 files, 4,063 tests, TZ=UTC); the 40 locale stubs and localization/data.json are exactly what pnpm l10n regenerates.

What's left: the new veto also fires for the caller's own clamped move and changes which message that case gets, the rewritten store test lost the scenario it used to cover, the writability rule is now spelled in four places, and the public docs don't list the new rejection reason.

Bugs (1)

1. 🟡 The veto also catches the caller's own clamped move, so the same updateEvent gets a different rejection depending on where it lands

Location: packages/x-scheduler-internals-premium/src/internals/plugins/SchedulerSchedulingPlugin.ts:192

      const unwritable = this.findUnwritableCascadedEntry(result.updated);

The engine seeds the cascade with the caller's own entries and reports any event whose dates changed, seeds included (auto-scheduling.ts:339-340, :415-421): a seed placed before its predecessor ends comes back clamped. So result.updated is not only successors, and the new check runs before the store's own checkUpdatedDatesWritable. Verified with a scratch test on this branch, eventModelStructure: { start: { getter } }, FinishToStart a→b:

  • updateEvent({ id: 'b', start: 09:00, end: 10:00 }) (violates the dependency) → { applied: false, rejection: { code: 'unwritableEventMovedError', params: { title: 'b' } } }, with the plugin's "keeping a dependency would move an event" warning.
  • updateEvent({ id: 'b', start: 12:00, end: 13:00 }) (no violation) → { applied: false, rejection: { code: 'datesNotWritableError' } }, with the store's "An event was not moved" warning.
  • On master both returned datesNotWritableError.

Second effect of the same ordering: when the caller's own entry is the unwritable move and a successor gets clamped (a 11:00→12:00 pushing b, the input the old test at :506 used), the rejection carries b's title, so the toast blames the successor for the user's own drag.

Nothing is applied in any of these cases, so no regression in effect. The drag path can't reach it (isDraggable is false whenever a date is unwritable, schedulerEventSelectors.ts:82-83, :225), but the event form can (FormContent.tsx:505-510 submits both bounds and pins the rejection on the range field), and programmatic updateEvent can.

Failure scenario: a developer with a getter-only start opens the form on b, types a start that overlaps a, and reads "This change would move the event "b", whose dates can't be edited" plus a console warning about a dependency; moving it the other way gives "This date can't be changed". Same edit, same cause, two explanations.

Fix: leave the caller's own two-bound entries to the store, which already refuses them with the right code: in handleEventsUpdate, const callerMoves = new Set(updated.filter((e) => 'start' in e && 'end' in e).map((e) => e.id)); and run findUnwritableCascadedEntry(result.updated.filter((e) => !callerMoves.has(e.id))). A two-bound caller entry with an unwritable bound is never applied anyway, so nothing slips through. One store test for the violating position asserting datesNotWritableError pins it.

Tests (3)

1. 🟡 The rewritten store test no longer covers the scenario it used to, and nothing else does

Location: packages/x-scheduler-internals-premium/src/use-event-timeline-premium/tests/autoScheduling.EventTimelinePremiumStore.test.ts:507

            { id: 'a', start: date('2025-07-03T08:00:00Z'), end: date('2025-07-03T09:00:00Z') },

Before, a moved later past b (getter-only start) with d deleted in the same batch: the cascade pushed b, the batch was refused, and the test pinned that the dependency cleanup for d did not go out. With this PR the plugin's veto wins for that input (I ran the old dates: plugin warning, unwritableEventMovedError naming b, both callbacks at 0), and the author moved the input to a non-cascading earlier move instead of asserting the new behaviour. The read-only veto twin at :451 still exists; the unwritable-veto-plus-deletion combination, which is #23936 folded into a batch with a deletion, is now asserted nowhere (grep "deleted: \['d'\]" finds :469 read-only, :505 this test, :889 the positive path). A reorder in handleEventsUpdate that built commit before the check would pass the suite.

Fix: keep the rewritten test (it does pin that the store's own refusal beats the cleanup) and add the old input as a sibling asserting toWarnDev([cascadeNotWritableWarning('start')]), rejection deep-equal { code: 'unwritableEventMovedError', params: { title: eventB.title } }, and onEventsChange/onDependenciesChange at 0; hoist cascadeNotWritableWarning (:523) to module scope so both describes can use it. The comment above the rewritten test (:479-481, "before the store's own date-writability check runs on the merged batch") still describes the old scenario.

2. ℹ️ Three small assertion gaps

Location: autoScheduling.EventTimelinePremiumStore.test.ts:548, :820-825

Round 1's "assert nothing changed" landed in the two single-bound tests but not in the first one (:527-549 ends at onEventsChange not called; one line on a's end staying at 10:00 completes it). "The warning above is only logged once" (:820) is proved only by vitest-fail-on-console, not by the test; expect(() => …).not.toWarnDev() says it. And the relaxing paths are not pinned positively: I checked that updateDependency('dep-1', { type: 'StartToStart' }) and deleteDependency('dep-1') with a getter-only start elsewhere return { status: 'updated' } / true with one onDependenciesChange call and no warning, which is right and cheap to assert against over-refusal.

3. ℹ️ The new user-facing string is never rendered by a test

Location: packages/x-scheduler/src/internals/utils/user-facing-error.ts:11-12

  if (error.code === 'readOnlyEventMovedError' || error.code === 'unwritableEventMovedError') {
    return localeText[error.code](error.params.title);

unwritableEventMovedError appears in one test only, as a descriptor (autoScheduling…test.ts:546); no test resolves it through getUserFacingErrorMessage, and the terminals test asserts errors[0].code rather than the text because TestTimeline never mounts ErrorContainer (same as its read-only sibling at :1780). A six-line unit test of getUserFacingErrorMessage with enUS covering the two function-typed codes and one plain code closes both. Also two test comments went stale with the move: event.SchedulerStore.test.ts:23 and editing.SchedulerStore.test.ts:26 say datesNotWritableReason lives "in SchedulerStore.ts (not exported)"; it is in SchedulerStore.utils.ts and exported now.

Simplifications (2)

1. 🟡 "Which bound has no setter" is now spelled four ways across two packages

Location: SchedulerSchedulingPlugin.ts:379-381, SchedulerStore.utils.ts:30-31, SchedulerStore.ts:858-859, schedulerEventSelectors.ts:71-72

      (['start', 'end'] as const).some(
        (property) => property in entry && !schedulerEventSelectors.isDateWritable(state, property),

Master had two copies, both in the store (datesNotWritableReason's filter, removeUnwritableDates' for); the PR adds a third in the premium plugin, and canWriteEventDates is a fourth phrasing of the same rule. Round 1 asked for one predicate shared by the two plugin paths, which is done; the remaining duplication is across the internals boundary.

Fix: one selector next to isDateWritable, unwritableDates(state): ('start' | 'end')[] (built on the existing module-private isPropertyMissingSetter). Then datesNotWritableReason formats it, removeUnwritableDates iterates it, and the plugin does cascaded.find((entry) => unwritable.some((p) => p in entry)), which also replaces the canWriteEventDates fast path at :376 (an empty list finds nothing). No behaviour change.

2. ℹ️ Two shapes that could be smaller

Location: SchedulerSchedulingPlugin.ts:372-374, dependencyTestUtils.tsx:87-90

findUnwritableCascadedEntry returns the entry while both callers use only .id (:194, :357) and the sibling blocked path hands back ids (:352 result.blocked[0]); returning the id (findUnwritableCascadedEventId) makes the two veto branches symmetric. In the test utils, the four-line EventModelStructure alias indexes the store parameters when SchedulerEventModelStructure<SchedulerEvent> is exported from @mui/x-scheduler-internals/models, which the file already imports from (the adjacent PresetConfig alias uses the same pattern, so this is a consistency call either way).

Docs (4)

1. 🟡 The public "Rejected dependencies" list doesn't include the new reason

Location: docs/data/scheduler/event-timeline/dependencies/dependencies.md:141-145

- would need to move a read-only event

The list enumerates every refusal (cycle, duplicate, recurring event, read-only event); a developer with a getter-only start whose terminal drop is refused with "This dependency would move an event whose dates can't be changed" finds no bullet that matches. The box at :102-104 ("Read-only events never move…") and the getter-only contract at events.md:196 ("only the other edge can be resized") don't say that a change whose cascade would write such a date is refused as a whole either, which is exactly the PR's own allDay test. #24039 only appends to this page after the list and doesn't touch events.md, so the bullet belongs here and the two branches merge cleanly.

Fix: a fifth bullet, "would need to change a date your eventModelStructure can't write back (a getter without a setter)", one sentence in the auto-scheduling box ("The same applies to an event whose start or end has no setter"), and one in events.md for the Timeline ("With dependencies, a change whose cascade would need to write such a date is refused as a whole").

2. 🟡 One of the three new strings says "edited" where its siblings say "changed"

Location: packages/x-scheduler/src/locales/enUS.ts:96-97

  unwritableEventMovedError: (title) =>
    `This change would move the event "${title}", whose dates can't be edited, so it was not applied.`,

datesNotWritableError (:93), dependencyDatesNotWritableError (:100-101) and dependencyUpdateDatesNotWritableError (:212-213) all say "can't be changed" for the same condition; translators get the one condition in two wordings. "whose dates can't be changed", then pnpm l10n to refresh the 40 stubs.

3. ℹ️ Four comment nits

Location: SchedulerStore.ts:910-912, SchedulerSchedulingPlugin.ts:152-154, :225-226, dependencyEditor.EventTimelinePremium.test.tsx:522-523

The round-1 JSDoc fixes hold. Remaining: the checkUpdatedDatesWritable JSDoc sentence "Checked after the scheduling plugin's contributions are merged into updated, but a cascaded entry … never gets here" repeats the inline comment at :671-672 and explains an ordering that no longer matters (drop it or keep one). "needing a date eventModelStructure can't write back to change" (plugin :225-226, EventTimelinePremiumStore.ts:402-403) reads twice; "or changing a date eventModelStructure can't write back". "updateEvents can still refuse the merged batch afterward for a reason of its own" lost its example, which was still correct: the store still refuses the caller's own two-bound unwritable move (:676-684) before commit(). And the test comment "would push the start of the writable event-o, which eventModelStructure can't write back" contradicts itself; "not read-only" is what it means.

4. ℹ️ PR body

The ## Changelog is accurate. Two additions would help: the three new localeText keys (unwritableEventMovedError, dependencyDatesNotWritableError, dependencyUpdateDatesNotWritableError), since teams shipping a custom localeText need to know, and a line on the tests (four new and one modified store tests, two dialog tests, one terminals test), which round 1 asked for.


Heads-up on the base: #24039 ("Make the Event Timeline dependencies public") is about to land and touches dependencyTestUtils.tsx on the same lines as this PR's new eventModelStructure prop (it renames the features type). One small conflict to resolve after it merges; dependency.ts and EventTimelinePremiumStore.ts merge on their own, and #24039 doesn't make the rejection types public, so nothing above changes.

Verdict

Approve after nits — the round-1 design change is in and verified on all three paths; what remains is the precedence of the new veto over the store's own check for the caller's move, a lost test scenario, and the docs list.


🤖 Review generated with Claude Code

@rita-codes rita-codes 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.

Round-2 review posted above: the round-1 blocker is resolved and verified; remaining items are nits.

@mustafajw07

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review and approval, @rita-codes! I’ve addressed the remaining feedback, including the caller-move handling, additional test coverage, shared writability logic, and documentation updates. I’ve pushed the changes and resolved the outstanding items. Ready for another review!

@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Resolves the conflict with mui#24039 in dependencyTestUtils.tsx: keeps the
PR's `eventModelStructure` prop next to the now-public `features` type.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 30, 2026
@rita-codes

Copy link
Copy Markdown
Member

PR review (round 3)

Thank you for the contribution and for the care you put into all three rounds. Moving the writability check into the plugin, rebuilding the tests around the real scenarios and keeping the strings and docs in sync is a lot of follow-through, and it makes #23936 a clean fix.

No findings. Everything from round 2 is in, and I merged master into the branch to resolve the conflict with #24039 (dependencyTestUtils.tsx: your eventModelStructure prop now sits next to the public EventTimelinePremiumFeatures type), so the PR is mergeable again.

What I checked on bd2128998a:

  • Veto precedence: leaving a two-bound caller move to the store is exactly the store's own condition (isDatesMove is "both bounds present", and checkUpdatedDatesWritable runs before commit()), so nothing slips through and the same edit now gets the same rejection wherever it lands. The two it.each cases pin both positions.
  • Tests: the old input (a to 11:00 with d deleted) is back and asserts datesNotWritableError, and the resize sibling asserts the plugin's veto with the cleanup still unsent. The "nothing changed" assertion, not.toWarnDev() on the second updateDependency, the relax/delete positive case and the getUserFacingErrorMessage unit test are all there; the two "not exported" comments are fixed.
  • One predicate: unwritableDates replaces isDateWritable in the store, datesNotWritableReason and the plugin, with no leftover reference; the helper returns the id and the test utils use the exported SchedulerEventModelStructure.
  • Docs and strings: the new bullet, the auto-scheduling box, the sentence in events.md, "can't be changed" in enUS with the 40 stubs regenerated (pnpm l10n produces no diff), the JSDoc nits and the PR body.

On the merged branch: ESLint and Prettier pass on the 64 changed files, tsc passes in the four Scheduler packages, and the Scheduler suite passes (214 files, 4,092 tests, TZ=UTC).

Verdict

Approve — every round-2 item is resolved and verified, and the branch is on current master.


🤖 Review generated with Claude Code

@rita-codes
rita-codes merged commit a68a517 into mui:master Oct 30, 2026
21 of 22 checks passed
@mustafajw07
mustafajw07 deleted the fix/23936-scheduler-refuse-unwritable-batch branch October 30, 2026 18:55
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: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[scheduler] Refuse the batch when the auto-scheduling cascade clamps an unwritable date

2 participants