Repository navigation
[scheduler] Refuse the batch when the auto-scheduling cascade clamps an unwritable date - #24002
Conversation
|
Hey @rita-codes, can I get a review on this? |
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
rita-codes
left a comment
There was a problem hiding this comment.
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 itThe 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
|
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! |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Signed-off-by: Mustafa Jawadwala <mustafajw07@gmail.com>
…dulerDependencyCascadeRejection and update EventTimelinePremiumStore instantiation
|
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>
|
Hey @rita-codes, can you review this pr? |
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, 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
|
rita-codes
left a comment
There was a problem hiding this comment.
Round-2 review posted above: the round-1 blocker is resolved and verified; remaining items are nits.
… of "can't be edited" for unwritable events
|
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! |
|
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>
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 ( What I checked on
On the merged branch: ESLint and Prettier pass on the 64 changed files, VerdictApprove — every round-2 item is resolved and verified, and the branch is on current master. 🤖 Review generated with Claude Code |
Closes #23936
When the auto-scheduling cascade had to move an event whose date
eventModelStructurecan't write back, the date was dropped and the rest of the change applied, leaving the dependency broken. Event updates,addDependencyandupdateDependencynow refuse the change and emit nothing. Dependency calls return the newdatesNotWritablerejection reason, which the dependency dialog and drag-to-create show as an error.New
localeTextkeys, for teams that ship a customlocaleText:unwritableEventMovedError,dependencyDatesNotWritableErroranddependencyUpdateDatesNotWritableError.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 theisDateWritableselector tests rewritten forunwritableDates.Changelog
[scheduler] Refuse a change that would break a dependency because an event's dates can't be written back ([scheduler] Refuse the batch when the auto-scheduling cascade clamps an unwritable date #23936)
I have followed (at least) the PR section of the contributing guide.