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

[EugeniyKiyashko]

Looks correct, I would merge after a rebase. Main has since renamed the m_ files and typed the util, and the new Jest test does not survive that - details inline. I rebased it locally, the adaptation is two lines in the test plus one conflict in click.ts that needs a careful resolve.

Two things with no line to hang them on:

• worth a case for clicking the same element twice. On current main the foreign dxremove handler there gets 0 calls, with the fix 1.

• unsubscribeNodesDisposing on main declares callback: EventHandler | undefined. That | undefined is what still permits the blanket removal. Dropping it type checks clean.

[EugeniyKiyashko]

This path is gone on main. The m_ prefix was dropped after this branch was cut, so it is @ts/events/core/events_engine now. As it stands the suite cannot even load after a rebase.

The rebase also conflicts in click.ts. Worth resolving by hand: git merges most of the file on its own but leaves unsubscribeNodesDisposing(lastFiredEvent, callback, nodes) outside the conflict markers, so just taking your side of the marked block leaves a reference to a callback that no longer exists.

One thing you get for free from the rebase: main's click.ts already imports NodesDisposingSubscription from the util, so the local interface this PR adds at the top of click.ts disappears.

[EugeniyKiyashko]

On main elementDataMap is now optional (elementDataMap?: WeakMap<...>), so this line becomes a type error after a rebase. And because the internal tsconfig sets noEmitOnError: true, ts-jest then refuses to emit and the whole suite fails to run with a confusing Unable to process ... outDir message instead of a type error. Took me a minute to trace, so flagging it.

The fix makes the helper smaller rather than bigger. Main types the map as WeakMap<EngineTarget, ElementEventData>, so both the cast and the hand-written ElementEventData above become unnecessary:

const getRemoveHandlersCount = (element: Element): number => {
  const elementData = eventsEngine.elementDataMap?.get(element);

  return elementData?.[removeEvent]?.handleObjects.length ?? 0;
};

I rebased the branch locally with just this and the import above changed, and both tests pass and the project type checks clean.

[EugeniyKiyashko]

Small one, the code is correct as it stands.

lastFiredEvent && is not doing any work here. The two variables are always set and cleared together, so lastSubscription on its own is enough. The first argument to unsubscribeNodesDisposing is dead too, since nodes is always a real array and the util only falls back to the event when nodes is missing.

if (originalEvent) also appears twice in the same block. Putting the pairing in one place makes the "these two always move together" rule visible instead of implied:

if (lastSubscription) {
  unsubscribeNodesDisposing(null, lastSubscription.onceCallback, lastSubscription.nodes);
}

lastFiredEvent = originalEvent;
lastSubscription = originalEvent ? subscribeNodesDisposing(originalEvent, onNodeRemove) : null;

[EugeniyKiyashko]

The test never checks that the two clicks actually reached the dxclick handler. If they ever stop firing, foreignHandlerCallCount stays at 1 and the test keeps passing while testing nothing. The Jest version is protected because it asserts the handler count is 1 right after the click, but this one has no such guard. Counting dxclick calls instead of passing noop would cover it.