Scheduler: T1308596 — "Uncaught TypeError: Cannot read properties of undefined (reading 'getBoundingClientRect')" is thrown when dragging an appointment while an onAppointmentUpdating promise is pending#31572
Conversation
…the appointment is dragged to" This reverts commit f46d03b.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the Scheduler component where attempting to drag an appointment while it's already being dragged causes a runtime error. The fix includes both code and styling changes to prevent the error.
- Added null check for
getFocusedCell()to prevent accessing properties on undefined - Added CSS rule to disable pointer events on dragged appointments
- Added regression test to verify drag-during-drag scenario doesn't throw errors
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/devextreme/js/__internal/scheduler/workspaces/m_work_space.ts | Added null check before accessing cellData property to prevent undefined access error |
| packages/devextreme-scss/scss/widgets/base/scheduler/appointment/regular/_index.scss | Added pointer-events: none to drag source appointments to prevent interaction during drag |
| e2e/testcafe-devextreme/tests/scheduler/common/dragAndDrop/T1308596.ts | Added regression test for drag-during-drag scenario with async promise delay |
There was a problem hiding this comment.
I noticed that we have e2e tests named after ticket numbers. I’m not fully convinced this naming convention is ideal — it makes it hard to understand what the test actually does without referring to an external ticket.
Regarding this specific test: it feels a bit overengineered to me. It’s testing a negative scenario — something that ideally shouldn’t even happen. In e2e tests we usually want to focus on the positive, expected path, not exceptions.
In my opinion, this test belongs in the code workspace tests instead. I’d suggest moving it there.
There was a problem hiding this comment.
I moved test to a different file, e2e/testcafe-devextreme/tests/scheduler/common/dragAndDrop/dragEvents.ts
I think it is related to drag & drop, because the error occurs only during dragging the appointment.
There was a problem hiding this comment.
Also I optimized the test, hope it looks a bit better now
sjbur
force-pushed
the
issue-2112_25_2
branch
from
a57522f to
a5ffd6e
Compare
| e.cancel = true; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing check for _isAppointmentBeingUpdated() in createDragEndHandler. If an appointment update starts during a drag operation (between dragMove and dragEnd), the dragEnd handler will proceed without checking if the appointment is being updated. This could lead to inconsistent state or race conditions. Add the same _isAppointmentBeingUpdated() check after the appointmentInfo null check for consistency with dragStart and dragMove handlers.
| if (this.scheduler._isAppointmentBeingUpdated(this.appointmentInfo.appointment)) { | |
| e.cancel = true; | |
| return; | |
| } |
| } catch (err) { | ||
| performFailAction(err); | ||
| this._updatingAppointments.delete(target); | ||
| deferred.resolve(); | ||
| } |
There was a problem hiding this comment.
If the .fail() handler at line 1779 executes, performFailAction() is called but this._updatingAppointments.delete(target) is not. This means the appointment will remain in the tracking Set indefinitely, permanently blocking future drag operations for that appointment. The .always() handler at lines 1775-1778 only runs for successful promise creation, not for promise rejection. Consider moving the delete() call to ensure it executes in all failure scenarios.
sjbur
changed the title
No description provided.