From 4ac31d750ec8e494993c3b119d3144f0719b452d Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Sat, 5 Sep 2026 14:35:33 +0530 Subject: [PATCH] fix(dnd): isolate pending drop and registration lifecycles --- packages/dnd/package.json | 2 +- packages/dnd/src/manager.ts | 39 +++++- packages/dnd/src/types.ts | 2 + packages/dnd/test/manager-lifecycle.test.ts | 139 ++++++++++++++++++++ 4 files changed, 176 insertions(+), 6 deletions(-) create mode 100644 packages/dnd/test/manager-lifecycle.test.ts diff --git a/packages/dnd/package.json b/packages/dnd/package.json index 752db87e..18b6c3b2 100644 --- a/packages/dnd/package.json +++ b/packages/dnd/package.json @@ -1,6 +1,6 @@ { "name": "@wrnexus/dnd", - "version": "0.1.0", + "version": "0.1.1", "type": "module", "description": "Stable accessible drag-and-drop primitives for WRNexus.", "exports": { diff --git a/packages/dnd/src/manager.ts b/packages/dnd/src/manager.ts index 8ed3ae15..db5b49b4 100644 --- a/packages/dnd/src/manager.ts +++ b/packages/dnd/src/manager.ts @@ -20,14 +20,19 @@ export function createDndManager(options: DndManagerOptions): DndManager { >(); const manager = new DragDropManager(); let activeIds: Array = []; + let generation = 0; + let destroyed = false; manager.monitor.addEventListener("dragstart", (event) => { + if (destroyed) return; + generation++; const source = event.operation.source; if (!source) return; activeIds = [source.id]; options.announce?.({ type: "pickedUp", ids: activeIds }); }); manager.monitor.addEventListener("dragover", (event) => { + if (destroyed) return; const source = event.operation.source as (typeof event.operation.source & { group?: string | number; index?: number }) | null; if (!source) return; @@ -38,6 +43,9 @@ export function createDndManager(options: DndManagerOptions): DndManager { } else options.announce?.({ type: "movedOver", ids: activeIds, location: destination }); }); manager.monitor.addEventListener("dragend", async (event) => { + if (destroyed) return; + const endedGeneration = generation; + const ids = [...activeIds]; const source = event.operation.source as | (typeof event.operation.source & { group?: string | number; @@ -47,12 +55,12 @@ export function createDndManager(options: DndManagerOptions): DndManager { }) | null; if (!source || event.operation.canceled) { - options.announce?.({ type: "cancelled", ids: activeIds }); + options.announce?.({ type: "cancelled", ids }); activeIds = []; return; } const intent: DndMoveIntent = { - ids: [...activeIds], + ids, from: [ { group: source.initialGroup ?? source.group ?? "default", @@ -61,17 +69,32 @@ export function createDndManager(options: DndManagerOptions): DndManager { ], to: { group: source.group ?? "default", index: source.index ?? 0 }, }; - const accepted = await options.onMoveIntent(intent); + // A pending commit must not own the next gesture's selection or lifecycle. + activeIds = []; + let accepted: boolean | void; + try { + accepted = + options.accept && !options.accept(ids, intent.to) + ? false + : await options.onMoveIntent(intent); + } catch (error) { + if (!destroyed && generation === endedGeneration) { + options.announce?.({ type: "cancelled", ids, location: intent.to }); + options.onError?.(error, intent); + } + return; + } + if (destroyed || generation !== endedGeneration) return; options.announce?.({ type: accepted === false ? "cancelled" : "dropped", - ids: activeIds, + ids, location: intent.to, }); - activeIds = []; }); return { registerSortable(registration) { + if (destroyed) throw new Error("Cannot register a sortable after manager destruction."); if (entries.has(registration.id)) throw new Error(`Duplicate sortable id ${String(registration.id)}.`); const sortable = new Sortable( @@ -95,11 +118,17 @@ export function createDndManager(options: DndManagerOptions): DndManager { current: registration, }); return () => { + // Cleanup can run after refresh or be called more than once. + if (entries.get(registration.id)?.sortable !== sortable) return; sortable.destroy(); entries.delete(registration.id); }; }, destroy() { + if (destroyed) return; + destroyed = true; + generation++; + activeIds = []; for (const entry of entries.values()) entry.sortable.destroy(); entries.clear(); manager.destroy(); diff --git a/packages/dnd/src/types.ts b/packages/dnd/src/types.ts index a5cf4fe1..420be5b0 100644 --- a/packages/dnd/src/types.ts +++ b/packages/dnd/src/types.ts @@ -35,6 +35,8 @@ export interface DndManagerOptions { announce?: (announcement: DndAnnouncement) => void; accept?: (ids: DndId[], destination: SortableLocation) => boolean; onMoveIntent(intent: DndMoveIntent): boolean | void | Promise; + /** Reports a failed commit without leaking an unhandled event-listener rejection. */ + onError?(error: unknown, intent: DndMoveIntent): void; } export interface DndManager { diff --git a/packages/dnd/test/manager-lifecycle.test.ts b/packages/dnd/test/manager-lifecycle.test.ts new file mode 100644 index 00000000..5594c860 --- /dev/null +++ b/packages/dnd/test/manager-lifecycle.test.ts @@ -0,0 +1,139 @@ +import { expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; +import type { DndAnnouncement, DndManager, DndManagerOptions } from "../src/types.ts"; + +// Run the real wrapper with an isolated backend, without global module mocks. +const source = readFileSync(new URL("../src/manager.ts", import.meta.url), "utf8"); +const compiled = new Bun.Transpiler({ loader: "ts" }) + .transformSync(source) + .replace(/^import .*;\s*$/gm, "") + .replace("export function createDndManager", "function createDndManager"); + +function fixture(overrides: Partial = {}) { + const listeners: Record any> = {}; + const announcements: DndAnnouncement[] = []; + const instances: Array<{ destroyed: number }> = []; + let managerDestructions = 0; + class Backend { + monitor = { + addEventListener: (name: string, callback: any) => { + listeners[name] = callback; + }, + }; + destroy() { + managerDestructions++; + } + } + class Sortable { + destroyed = 0; + constructor() { + instances.push(this); + } + destroy() { + this.destroyed++; + } + } + const create = new Function( + "DragDropManager", + "Sortable", + compiled + ";return createDndManager;", + )(Backend, Sortable); + const manager: DndManager = create({ + onMoveIntent: () => true, + announce: (event: DndAnnouncement) => announcements.push(event), + ...overrides, + }); + const event = (id: string) => ({ + operation: { + source: { id, group: "b", index: 1, initialGroup: "a", initialIndex: 0 }, + canceled: false, + }, + }); + return { + manager, + announcements, + instances, + start: (id: string) => listeners.dragstart!(event(id)), + end: (id: string) => listeners.dragend!(event(id)), + get managerDestructions() { + return managerDestructions; + }, + }; +} + +test("late drop resolution cannot clear or announce a newer gesture", async () => { + let finish!: (value: boolean) => void; + const f = fixture({ + onMoveIntent: (intent) => + intent.ids[0] === "first" + ? new Promise((resolve) => { + finish = resolve; + }) + : true, + }); + f.start("first"); + const pending = f.end("first"); + f.start("second"); + finish(true); + await pending; + await f.end("second"); + expect(f.announcements.filter((event) => event.type === "dropped")).toEqual([ + { type: "dropped", ids: ["second"], location: { group: "b", index: 1 } }, + ]); +}); + +test("drop commit rejection reports cancellation and its original intent", async () => { + const failure = new Error("save failed"); + const errors: unknown[] = []; + const f = fixture({ + onMoveIntent: async () => { + throw failure; + }, + onError: (error, intent) => errors.push([error, intent.ids]), + }); + f.start("first"); + await f.end("first"); + expect(errors).toEqual([[failure, ["first"]]]); + expect(f.announcements.at(-1)?.type).toBe("cancelled"); +}); + +test("drop acceptance is checked again before dispatching a commit", async () => { + let commits = 0; + const f = fixture({ + accept: () => false, + onMoveIntent: () => { + commits++; + }, + }); + f.start("first"); + await f.end("first"); + expect(commits).toBe(0); + expect(f.announcements.at(-1)?.type).toBe("cancelled"); +}); + +test("old cleanup cannot remove replacement registrations and destruction is terminal", async () => { + let finish!: (value: boolean) => void; + const f = fixture({ + onMoveIntent: () => + new Promise((resolve) => { + finish = resolve; + }), + }); + const registration = { id: "same", group: "a", index: 0, element: {} as HTMLElement }; + const cleanup = f.manager.registerSortable(registration); + cleanup(); + f.manager.registerSortable(registration); + cleanup(); + expect(() => f.manager.registerSortable(registration)).toThrow("Duplicate"); + f.start("same"); + const pending = f.end("same"); + f.manager.destroy(); + f.manager.destroy(); + finish(true); + await pending; + cleanup(); + expect(f.instances.map((instance) => instance.destroyed)).toEqual([1, 1]); + expect(f.managerDestructions).toBe(1); + expect(() => f.manager.registerSortable(registration)).toThrow("destruction"); + expect(f.announcements.filter((event) => event.type === "dropped")).toEqual([]); +});