jcoffey-dev
2e33b4c467
Two complaints in #71, one cause. Deleting from the keyboard left `focusId` pointing at a row that was no longer in the list, and nothing moved it. The confirmation appearing "every other message": `targetIds()` falls back to the focused id, so the second `#` re-targeted the message the first one had just deleted. The optimistic update had already moved that message into Deleted Items, so it read as a permanent delete -- and a permanent delete always confirms, whatever "Confirm before deleting" is set to. The dialog was correct about the message it was asked about; it was asked about the wrong one. `k` jumping to the top: `moveFocus` reads `ids.indexOf(focusId)`, which was -1 for the departed row, and -1 is treated as "before the first row". Adding -1 to that clamps to 0. Both explain why the mouse was fine: clicking sets focus to a row that exists. Focus now moves to whatever slid into the deleted row's place, honouring "After archiving or deleting" -- the row below by default, the one above when set to newer -- and clears when the folder empties. `moveFocus` also no longer reads a missing row as index 0; it falls back to where the list thinks it is. Verified in the browser against the mock, since arithmetic tests cannot prove the wiring: with confirmation off, two deletes in a row both go through silently, focus stepping e4 to e5 to e6 as rows close up; then `k` moves up exactly one instead of to the top of the list.
76 lines
3.0 KiB
TypeScript
76 lines
3.0 KiB
TypeScript
import { describe, expect, it } from "vitest";
|
|
|
|
/**
|
|
* Issue #71, both halves of it, reduced to the arithmetic they turn on.
|
|
*
|
|
* After deleting a row from the keyboard, `focusId` used to keep pointing at
|
|
* the row that had gone. Two things fell out of that:
|
|
*
|
|
* - `targetIds()` falls back to the focused id, so the next `#` re-targeted
|
|
* the deleted message. The optimistic update had already moved it into
|
|
* Deleted Items, so it looked like a permanent delete and raised a
|
|
* confirmation the user had switched off.
|
|
* - `moveFocus` read `ids.indexOf(focusId)` as -1 and treated that as
|
|
* "before the first row", so `k` clamped to the top of the list.
|
|
*
|
|
* Clicking was unaffected: it sets focus to a row that exists. That is why it
|
|
* only ever happened from the keyboard.
|
|
*/
|
|
|
|
/** Where focus lands after the row at `wasAt` is removed. */
|
|
function focusAfterRemove(freshIds: string[], wasAt: number, autoAdvance: "newer" | "older" | "list"): string | null {
|
|
if (!freshIds.length) return null;
|
|
if (wasAt < 0) return undefined as unknown as string;
|
|
const want = autoAdvance === "newer" ? wasAt - 1 : wasAt;
|
|
return freshIds[Math.max(0, Math.min(want, freshIds.length - 1))] ?? null;
|
|
}
|
|
|
|
/** What moveFocus resolves to, given a focus id that may no longer exist. */
|
|
function nextIndex(ids: string[], focus: string | null, listIndex: number, delta: number): number {
|
|
const fromFocus = focus ? ids.indexOf(focus) : -1;
|
|
const cur = fromFocus >= 0 ? fromFocus : listIndex;
|
|
return Math.max(0, Math.min(ids.length - 1, (cur < 0 ? (delta > 0 ? -1 : 0) : cur) + delta));
|
|
}
|
|
|
|
describe("focus after deleting a row", () => {
|
|
const after = ["b", "c", "d"]; // "a" was at 0 and has gone
|
|
|
|
it("lands on the row that slid into the gap", () => {
|
|
expect(focusAfterRemove(after, 0, "older")).toBe("b");
|
|
});
|
|
|
|
it("lands on the row above when auto-advance is set to newer", () => {
|
|
// deleted "c" at index 2; newer means the one before it
|
|
expect(focusAfterRemove(["a", "b", "d"], 2, "newer")).toBe("b");
|
|
});
|
|
|
|
it("does not run off the end when the last row was deleted", () => {
|
|
expect(focusAfterRemove(["a", "b"], 2, "older")).toBe("b");
|
|
});
|
|
|
|
it("clears focus when the list is now empty", () => {
|
|
expect(focusAfterRemove([], 0, "older")).toBeNull();
|
|
});
|
|
});
|
|
|
|
describe("moving focus when the focused row has gone", () => {
|
|
const ids = ["b", "c", "d"];
|
|
|
|
it("no longer sends k to the top of the list", () => {
|
|
// The regression: focus is on the deleted "a", the list says we were at 1.
|
|
expect(nextIndex(ids, "a", 1, -1)).toBe(0);
|
|
// …and with focus repaired to a real row, k moves by one as it should.
|
|
expect(ids[nextIndex(ids, "c", 1, -1)]).toBe("b");
|
|
});
|
|
|
|
it("moves by one from a row that exists, in both directions", () => {
|
|
expect(ids[nextIndex(ids, "c", 1, 1)]).toBe("d");
|
|
expect(ids[nextIndex(ids, "b", 0, 1)]).toBe("c");
|
|
});
|
|
|
|
it("stops at the ends rather than wrapping", () => {
|
|
expect(ids[nextIndex(ids, "b", 0, -1)]).toBe("b");
|
|
expect(ids[nextIndex(ids, "d", 2, 1)]).toBe("d");
|
|
});
|
|
});
|