Dxclick: nodes-disposing

[dmlvr]

No description provided.

[Copilot]

🟢 Approval recommended

The change corrects the handler passed to unsubscribeNodesDisposing and includes targeted QUnit + Jest regression tests covering the previously broken behavior.

Pull request overview

This PR fixes dxclick’s nodes-disposing cleanup so that it unsubscribes only the handler it registered (instead of accidentally removing all dxremove handlers on the previously clicked node), and adds regression coverage for the scenario.

Changes:

• Fix dxclick cleanup to pass onceCallback to unsubscribeNodesDisposing, ensuring only the intended dxremove handler is removed.
• Add QUnit regression tests to ensure foreign dxremove handlers survive dxclick transitions and unsubscribeNodesDisposing doesn’t over-unsubscribe.
• Add Jest regression tests for the same behavior at the internal events layer.

File summaries

File	Description
packages/devextreme/testing/tests/DevExpress.ui.events/events.utils.nodesDisposing.tests.js	Updates unsubscribe call to use onceCallback and adds a regression test ensuring only the passed handler is removed.
packages/devextreme/testing/tests/DevExpress.ui.events/click.tests.js	Adds a QUnit regression test verifying foreign dxremove handlers remain after clicking a different element.
packages/devextreme/js/__internal/events/click.ts	Fixes dxclick’s nodes-disposing unsubscribe to use onceCallback so it doesn’t remove unrelated dxremove handlers.
packages/devextreme/js/__internal/events/tests/click.test.ts	Adds Jest coverage validating foreign dxremove handlers persist and only the internal handler is removed.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

subscriptions cleanup still leaks Map entries when the clicked node is disposed before the next click, which can accumulate retained DOM references over time.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

[dmlvr]

Good catch — confirmed, and fixed, but not by capturing originalEvent.

Instead I dropped the Map altogether and replaced it with a single module-level variable, lastSubscription, which onNodeRemove now nulls alongside lastFiredEvent.

The event↔subscription pairing is strictly 1:1: subscriptions.set was always paired with lastFiredEvent = originalEvent, and every read was subscriptions.get(lastFiredEvent). There is never more than one live subscription — the previous one is unsubscribed on the next click, and onceCallback removes itself when it fires (including the nested-click path covered by T503035). So the Map was extra structure whose only effect was to create a second place where a delete could be forgotten. With one variable cleared on node removal there is nothing left to forget: the retention is gone by construction rather than handled in one more branch.

This also closes a second case the Map had: on a synthetic trigger, where originalEvent is undefined, the old code stored an entry under the undefined key that the if (lastFiredEvent && …) guard could never delete. The subscription is now created only for a truthy originalEvent, which is behavior-preserving — applyForEach early-returns on a falsy element, so subscribing with undefined was already a no-op.

[Copilot]

🟢 Approval recommended

The fix aligns with the subscribeNodesDisposing/unsubscribeNodesDisposing contract and includes targeted regression tests covering the reported failure mode.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite