[scheduler] Carry custom event data through the edit dialog submit#22874
Conversation
|
@rita-codes Could you please review this PR when you have a chance? |
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b584ed304
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
|
|
||
| const changes: SchedulerEventUpdatedProperties = { | ||
| ...customData, |
There was a problem hiding this comment.
Because these custom fields are captured when Save opens the recurring-scope dialog, they are later replayed from pendingRecurringEventOperation.changes when the user confirms a scope. If a controlled events prop (or another store update) changes the same recurring event's custom fields while the scope dialog is open, choosing a scope such as “All events” now writes this stale snapshot back over the current model; before this change that path omitted custom fields and updateEvents preserved the latest model values. Resolve the custom data at confirm time or avoid adding preserved fields to recurring updates that modify the original event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. The submit-time snapshot could replay stale custom data on the "All events" scope. I have moved the preservation to confirm time: the dialog no longer carries custom data in changes, and selectRecurringEventScope now carries the current model's custom fields onto the events it creates. Non-recurring and "All events" rely on the existing store merge. Added a regression test that fails on the old approach and passes now.
|
Hi @Anexus5919, apologies, this one's on us. The on hold label was only on the umbrella (#22866), not on this sub-issue, so you had no way to know. I've added it to the children now 🙏 This is genuinely great work, though. The issue is part of the edit-dialog customization umbrella, which is planned for after stable and still in the design phase (we're defining the API), so it's on hold for now. Your approach is spot-on, so I'll leave the PR open and we'll come back to it once the design is locked. Thanks a lot for the care you put into this! 🙇♀️ |
rita-codes
added
on hold
|
No worries at all, thanks for clarifying and for adding the label to the children 🙏 Totally understand it's on hold while the API design is being defined, that makes sense. Happy to leave it open and I'll pick it back up whenever the design is locked, including reworking it if the final API shifts. Thanks for the kind words and the thoughtful review! 🙇 |
There was a problem hiding this comment.
First, an update on the hold: although the umbrella (#22866) stays on hold while we design the customization API, this fix doesn't touch any of the surface being designed — it's store-internal and fixes data loss that exists today — so we're unblocking it and moving forward now.
Solid, surgical fix with genuinely guarding tests — I verified locally that the two split-scope tests fail without the store change and pass with it. One change requested before merge: the same mechanism loses custom data on two neighboring paths, and they should be covered here.
Bugs (2)
1. 🔴 duplicateEventOccurrence and pasteEvent (copy) still drop custom data through the same mechanism
Location: packages/x-scheduler-internals/src/internals/utils/SchedulerStore/SchedulerStore.ts:648
The root cause this PR fixes is "event rebuilt from modelInBuiltInFormat", and two more paths rebuild the same way: duplicateEventOccurrence (it even shares extractStandaloneEvent with the only-this scope — its JSDoc promises "the same properties as the original event except" start/end/rrule, which is currently untrue for custom fields) and pasteEvent in copy mode (cut is an update, so it already preserves). All these creation paths already tag the new event with extractedFromId, so there's a natural single place to apply the carry instead of special-casing it per path.
Failure scenario: duplicating an occurrence — or copy-pasting an event — with a custom priority field produces a new event without priority: the same data loss this PR fixes, one method below.
Fix: please extend the custom-data carry to cover duplicateEventOccurrence and pasteEvent (copy) in this PR, with tests mirroring the split-scope ones.
2. ℹ️ getCustomEventProperties classifies mapped model keys as custom when eventModelStructure is used
Location: packages/x-scheduler-internals/src/internals/utils/SchedulerStore/SchedulerStore.utils.ts:85
The helper filters the raw model by built-in property names. When a consumer maps fields via eventModelStructure (e.g. a getter reading startDate for start), the mapped source keys are not in EVENT_PROPERTIES_LOOKUP, so their stale values get carried onto the split event. Today the outcome is still correct because in createOrUpdateEventModelFromBuiltInEventModel setters run after the direct key copies and overwrite the stale values — but that's an implicit ordering dependency, not an expressed invariant. (Getter-without-setter mappings can't produce a correct split event either way, which is pre-existing.)
Failure scenario: none user-visible today; a future reordering in createOrUpdateEventModelFromBuiltInEventModel would silently make split events resurrect stale mapped values.
Fix: worth a test that exercises the carry with an eventModelStructure mapping, so the interaction is pinned rather than incidental.
Tests (1)
1. ℹ️ The confirm-time freshness test only exercises the scope that doesn't use the new code
Location: packages/x-scheduler-premium/src/event-calendar-premium/tests/EventDialog.test.tsx:2186
The "latest custom data" regression test uses scope all, whose preservation comes from the pre-existing store merge. The confirm-time lookup this PR introduces is only hit by the split scopes.
Failure scenario: a future change that snapshots custom data earlier for the split paths would slip past this test.
Fix: a variant of the same test with only-this would pin the confirm-time resolution of the code this PR actually adds.
Simplifications (0)
No findings.
Docs (0)
No findings.
Verdict
Request changes — the fix itself is correct and well-tested, but the same data loss remains on duplicateEventOccurrence and copy-paste and should be covered before this merges.
|
Thanks, all three are fair.
|
rita-codes
left a comment
rita-codes
left a comment
There was a problem hiding this comment.
Thanks for taking care of this, nice fix!!! 👏
2 participants
Part of #22866
Closes #22869
Motivation
The event edit dialog only ever submits a fixed set of built in fields (
title,description,allDay,resource,colorplusstart/end/rrule). Events are generic overTEvent, so a user event can carry its own custom fields. Today those fields are silently dropped on some edit paths, which blocks the broader edit dialog customization work where custom sections will read and write custom data.This PR is the persistence slice of the umbrella. It does not touch the validation or lifecycle contract (that is the separate sub issue #22868).
The actual bug
I traced all four edit paths. Only two of them lose custom data:
updateEventallonly thismodelInBuiltInFormatthis and followingmodelInBuiltInFormatmodelInBuiltInFormatis a strict projection of the 16 built inSchedulerEventkeys, so the two create scopes rebuild a fresh event without any custom keys.What changed
getCustomEventProperties(model)inSchedulerStore.utils.tsthat returns only the keys of an event model that are not part of the built inSchedulerEventshape.selectRecurringEventScopecarries the current model's custom keys onto the newly created events (the only ones rebuilt from the built in shape). Non recurring andallrely on the existing{ ...oldModel }store merge, which already preserves custom data.FormContentor the public store API.Why confirm time, not submit time
For a recurring event the dialog submit only opens the scope dialog. The actual write happens later, when the user confirms a scope. Resolving custom data at confirm time, against the live model, avoids replaying a stale snapshot if the event's custom data changes while the dialog is open. A submit time snapshot would let the
allscope overwrite newer values, so the preservation belongs in the store, not in the form.Testing
getCustomEventProperties.updateEventdoes not strip unknown fields.all,only this,this and following, each asserting the custom field survives viaonEventsChange.EventCalendarPremiumdemo: editing an occurrence with a custompriorityfield and choosing "Only this event" produced a new event missingprioritybefore the fix, and carryingpriorityafter.pnpm eslint,pnpm prettier, and TypeScript across the touched packages all pass.