From 2e33b4c46732475973f83699e5a3c1aec38b95d9 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Wed, 26 Aug 2026 15:15:48 -0700 Subject: [PATCH] Move focus off a row that has been deleted 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. --- .../lib/__tests__/focusAfterRemove.test.ts | 75 +++++++++++++++++++ web/src/views/mail/MailView.tsx | 39 +++++++++- 2 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 web/src/lib/__tests__/focusAfterRemove.test.ts diff --git a/web/src/lib/__tests__/focusAfterRemove.test.ts b/web/src/lib/__tests__/focusAfterRemove.test.ts new file mode 100644 index 0000000..0c7915b --- /dev/null +++ b/web/src/lib/__tests__/focusAfterRemove.test.ts @@ -0,0 +1,75 @@ +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"); + }); +}); diff --git a/web/src/views/mail/MailView.tsx b/web/src/views/mail/MailView.tsx index e854d8b..581513e 100644 --- a/web/src/views/mail/MailView.tsx +++ b/web/src/views/mail/MailView.tsx @@ -115,6 +115,37 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; (removed: boolean) => { useMail.getState().clearSelection(); if (!removed) return; + + /* + * Move the focused row off the message that just went away. + * + * Nothing did this before, so `focusId` kept pointing at a row that was + * no longer in the list, and two separate complaints in #71 fell out of + * it. `targetIds()` falls back to the focused id, so the next `#` + * re-targeted the deleted message -- which the optimistic update had + * already marked as being in Deleted Items, making it look like a + * permanent delete and raising a confirmation the setting had turned + * off. And `moveFocus` reads `ids.indexOf(focusId)`, which was -1, which + * it treats as "before the start" -- so `k` clamped to the top of the + * list. + * + * Clicking a row was unaffected, because that sets focus to a row that + * exists, which is why it only ever happened from the keyboard. + * + * `currentRowIndex` here is the value from the render that started this + * action, so it is the index the message had *before* it was removed. + * The row that slid into that slot is the one to focus. + */ + const wasAt = currentRowIndex; + const freshIds = useMail.getState().list?.ids ?? []; + if (!freshIds.length) { + setFocusId(null); + } else if (wasAt >= 0) { + const want = settings.autoAdvance === "newer" ? wasAt - 1 : wasAt; + const next = freshIds[Math.max(0, Math.min(want, freshIds.length - 1))]; + if (next) setFocusId(next); + } + // auto-advance if (threadId) { const idx = currentRowIndex; @@ -128,7 +159,7 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; } } }, - [threadId, currentRowIndex, settings.autoAdvance, ids, rowThreadId, openThread], + [threadId, currentRowIndex, settings.autoAdvance, ids, rowThreadId, openThread, setFocusId], ); const actions = useMemo( @@ -191,7 +222,11 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; focusRef.current = focusId; useEffect(() => { const moveFocus = (delta: number) => { - const cur = focusRef.current ? ids.indexOf(focusRef.current) : currentRowIndex; + // A focused id that is no longer in the list gives -1, which must not be + // read as "just before the first row" -- that is what sent `k` to the + // top. Fall back to where the list thinks we are instead. + const fromFocus = focusRef.current ? ids.indexOf(focusRef.current) : -1; + const cur = fromFocus >= 0 ? fromFocus : currentRowIndex; const next = Math.max(0, Math.min(ids.length - 1, (cur < 0 ? (delta > 0 ? -1 : 0) : cur) + delta)); const id = ids[next]; if (!id) return;