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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
5d9ece7691
commit
33d700a07e
@@ -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"]);
|
||||
});
|
||||
});
|
||||
|
||||
+24
-5
@@ -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 };
|
||||
|
||||
Reference in New Issue
Block a user