From 33d700a07ee8a02dbb9e71c0c4aa8f240cb010ef Mon Sep 17 00:00:00 2001 From: Gerald Lonlas Date: Sat, 27 Jun 2026 10:43:58 +0800 Subject: [PATCH] fix(drag): correct off-by-one in profile drag-and-drop reorder When the dragged profile came from an earlier index than the drop target, `splice(sourceIndex, 1)` shifted every subsequent element left by one, making the raw `targetIndex` point one slot past the original drop target. The moved item landed AFTER the target card instead of AT it. Fix: extract `reorderProfiles()` pure helper that uses `targetIndex - 1` when `sourceIndex < targetIndex`, and delegate the drop handler to it. Adds 10 regression tests covering forward drags (the bug case), backward drags (unchanged), adjacent-element edge cases, and immutability of the input array. Co-Authored-By: Claude Opus 4.8 --- web/__tests__/image_profile_pure.test.mjs | 89 ++++++++++++++++++++++- web/image_profile.js | 29 ++++++-- 2 files changed, 112 insertions(+), 6 deletions(-) diff --git a/web/__tests__/image_profile_pure.test.mjs b/web/__tests__/image_profile_pure.test.mjs index 1785a2b..b0c2192 100644 --- a/web/__tests__/image_profile_pure.test.mjs +++ b/web/__tests__/image_profile_pure.test.mjs @@ -46,7 +46,7 @@ registerHooks({ // "../../scripts/app.js" import to the in-repo stub so registerExtension() // is a no-op instead of throwing. // --------------------------------------------------------------------------- -let clamp, sanitizeDimension, sanitizeSteps, parseResolution; +let clamp, sanitizeDimension, sanitizeSteps, parseResolution, reorderProfiles; before(async () => { const mod = await import("../image_profile.js"); @@ -54,6 +54,7 @@ before(async () => { sanitizeDimension = mod.sanitizeDimension; sanitizeSteps = mod.sanitizeSteps; parseResolution = mod.parseResolution; + reorderProfiles = mod.reorderProfiles; }); // =========================================================================== @@ -463,3 +464,89 @@ describe("parseResolution", () => { assert.equal(parseResolution("-1x-1"), null); }); }); + +// =========================================================================== +// reorderProfiles +// =========================================================================== +// Regression test for the drag-and-drop off-by-one bug: +// When sourceIndex < targetIndex, removing the source shifts every subsequent +// element left by one. The insertion index must be targetIndex - 1, not +// targetIndex, otherwise the moved item lands AFTER the drop target instead +// of AT it. +describe("reorderProfiles", () => { + // Convenience: build a profile array from single-letter ids. + const makeProfiles = (...ids) => ids.map((id) => ({ id, name: id })); + const ids = (profiles) => profiles.map((p) => p.id); + + // ---- forward drag (source before target) — the bug case ---- + + it("forward drag: [A,B,C,D] drag A(0) onto D(3) → [B,C,D,A] is WRONG; correct is [B,C,A,D]", () => { + // This test documents the fix: the moved item should land at D's original + // position (index 3), not after it. + const profiles = makeProfiles("A", "B", "C", "D"); + const result = reorderProfiles(profiles, 0, 3); + assert.deepEqual(ids(result), ["B", "C", "A", "D"]); + }); + + it("forward drag: [A,B,C,D] drag A(0) onto C(2) → [B,A,C,D]", () => { + const profiles = makeProfiles("A", "B", "C", "D"); + const result = reorderProfiles(profiles, 0, 2); + assert.deepEqual(ids(result), ["B", "A", "C", "D"]); + }); + + it("forward drag: [A,B,C,D] drag B(1) onto D(3) → [A,C,B,D]", () => { + const profiles = makeProfiles("A", "B", "C", "D"); + const result = reorderProfiles(profiles, 1, 3); + assert.deepEqual(ids(result), ["A", "C", "B", "D"]); + }); + + it("forward drag: [A,B,C,D] drag A(0) onto B(1) → [A,B,C,D] (adjacent, no change)", () => { + // Dropping on the immediately-next element inserts in the same position. + const profiles = makeProfiles("A", "B", "C", "D"); + const result = reorderProfiles(profiles, 0, 1); + assert.deepEqual(ids(result), ["A", "B", "C", "D"]); + }); + + // ---- backward drag (source after target) — already worked before the fix ---- + + it("backward drag: [A,B,C,D] drag D(3) onto A(0) → [D,A,B,C]", () => { + const profiles = makeProfiles("A", "B", "C", "D"); + const result = reorderProfiles(profiles, 3, 0); + assert.deepEqual(ids(result), ["D", "A", "B", "C"]); + }); + + it("backward drag: [A,B,C,D] drag C(2) onto A(0) → [C,A,B,D]", () => { + const profiles = makeProfiles("A", "B", "C", "D"); + const result = reorderProfiles(profiles, 2, 0); + assert.deepEqual(ids(result), ["C", "A", "B", "D"]); + }); + + it("backward drag: [A,B,C,D] drag D(3) onto B(1) → [A,D,B,C]", () => { + const profiles = makeProfiles("A", "B", "C", "D"); + const result = reorderProfiles(profiles, 3, 1); + assert.deepEqual(ids(result), ["A", "D", "B", "C"]); + }); + + // ---- input array is not mutated ---- + + it("does not mutate the original array", () => { + const profiles = makeProfiles("A", "B", "C"); + const original = [...profiles]; + reorderProfiles(profiles, 0, 2); + assert.deepEqual(profiles, original); + }); + + // ---- two-element edge case ---- + + it("[A,B] drag A(0) onto B(1) → [A,B] (adjacent forward, no change)", () => { + const profiles = makeProfiles("A", "B"); + const result = reorderProfiles(profiles, 0, 1); + assert.deepEqual(ids(result), ["A", "B"]); + }); + + it("[A,B] drag B(1) onto A(0) → [B,A]", () => { + const profiles = makeProfiles("A", "B"); + const result = reorderProfiles(profiles, 1, 0); + assert.deepEqual(ids(result), ["B", "A"]); + }); +}); diff --git a/web/image_profile.js b/web/image_profile.js index b619c92..f4f9a79 100644 --- a/web/image_profile.js +++ b/web/image_profile.js @@ -432,6 +432,28 @@ function clamp(value, minimum, maximum) { return Math.max(minimum, Math.min(maximum, value)); } +/** + * Return a new array with the element at sourceIndex moved to targetIndex's + * position (i.e. the moved element ends up where targetIndex pointed before + * the removal). + * + * When the source lies before the target, removing the source shifts every + * subsequent element left by one, so the effective insertion index is + * targetIndex - 1 (not targetIndex). + * + * @param {Array} profiles + * @param {number} sourceIndex + * @param {number} targetIndex + * @returns {Array} + */ +function reorderProfiles(profiles, sourceIndex, targetIndex) { + const reordered = [...profiles]; + const [moved] = reordered.splice(sourceIndex, 1); + const insertIndex = sourceIndex < targetIndex ? targetIndex - 1 : targetIndex; + reordered.splice(insertIndex, 0, moved); + return reordered; +} + function sanitizeDimension(value) { let integer = Number.parseInt(value, 10); if (Number.isNaN(integer)) { @@ -796,10 +818,7 @@ function renderProfiles(node) { return; } - const reordered = [...state.profiles]; - const [moved] = reordered.splice(sourceIndex, 1); - reordered.splice(targetIndex, 0, moved); - state.profiles = reordered; + state.profiles = reorderProfiles(state.profiles, sourceIndex, targetIndex); persistState(node); renderProfiles(node); @@ -1073,4 +1092,4 @@ app.registerExtension({ // Named exports for unit-testing pure helpers. // ComfyUI ignores extra named exports — it only executes the registerExtension side-effect above. -export { clamp, sanitizeDimension, sanitizeSteps, parseResolution }; +export { clamp, sanitizeDimension, sanitizeSteps, parseResolution, reorderProfiles };