From 1e440bc6a8874fc5b1e28fdf5bc795cf2450371d Mon Sep 17 00:00:00 2001 From: Hako Date: Mon, 12 Oct 2026 05:53:52 +0900 Subject: [PATCH 1/2] fix(mobile): close a menu sheet on Back instead of switching panes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since deep links made each pane a history entry, the system Back on a phone with a ⋯ sheet open went to the previous pane and left the sheet up. A sheet now holds a history entry of its own: Back closes it, and a sheet closed any other way steps off its entry once the closing click's effects have run, so no Back press is wasted. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 ++ scripts/deep-link-demo-regression.ts | 37 ++++++++++++ src/components/RowMenu.tsx | 4 ++ src/lib/sheetHistory.ts | 87 ++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+) create mode 100644 src/lib/sheetHistory.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index e6762424cc..0dc59e1e8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ between releases do not reach them. Remote-PC runtime bundles are versioned sepa ## [Unreleased] +### Fixed +- On a phone, the system Back button closes an open ⋯ menu sheet instead of switching to the pane + opened before, which it did since 0.4.7 made each pane a history entry. A sheet closed by Cancel, + a swipe or one of its items leaves no extra Back step behind. + ## [0.4.7] - 2026-10-12 ### Added diff --git a/scripts/deep-link-demo-regression.ts b/scripts/deep-link-demo-regression.ts index 9b005aabe0..d011c0358d 100644 --- a/scripts/deep-link-demo-regression.ts +++ b/scripts/deep-link-demo-regression.ts @@ -472,6 +472,43 @@ try { } }); console.log("PASS a pane Back selects under an open modal does not take the keyboard out of it"); + + // a phone's ⋯ sheet is an entry of its own: Back closes it and stays on the pane; a sheet + // closed by Cancel or by an item leaves no entry behind, so the next Back goes to the pane before + const phone = await browser.newContext({ viewport: { width: 390, height: 844 }, isMobile: true, hasTouch: true, locale: "en-US" }); + try { + await phone.addInitScript(() => localStorage.setItem("herdr-web-ui:settings", JSON.stringify({ language: "en" }))); + const page = await phone.newPage(); + const errors: string[] = []; + page.on("pageerror", (error) => errors.push(error.message)); + await page.goto(`${url}?ws=${wsOf(INFRA)}&pane=${encodeURIComponent(INFRA)}`); + await page.locator(".conn-live").waitFor({ state: "attached" }); + await waitSelected(page, INFRA); + // the sidebar is a closed drawer on a phone: a click on its row in the page is enough here + await page.locator(`.agents-sidebar .agent-item[data-pane="${DOCS}"] .agent-select`).evaluate((node) => (node as HTMLElement).click()); + await waitQuery(page, "pane", DOCS); + const length = await page.evaluate(() => history.length); + const more = page.locator(".header-more-button"); + const sheet = page.getByRole("dialog", { name: "More", exact: true }); + await more.tap(); + await sheet.waitFor(); + assert.equal(await page.evaluate(() => history.length), length + 1, "an open sheet is an entry"); + await page.goBack(); + await sheet.waitFor({ state: "detached" }); + assert.equal(await selected(page), DOCS, "Back closed the sheet and left the pane alone"); + assert.equal((await query(page))["pane"], DOCS); + await more.tap(); + await sheet.waitFor(); + await sheet.getByRole("button", { name: "Cancel", exact: true }).tap(); + await sheet.waitFor({ state: "detached" }); + await page.waitForFunction(() => !Object.keys(history.state ?? {}).includes("herdr-web-ui:sheet")); + await page.goBack(); + await waitSelected(page, INFRA); + assert.deepEqual(errors, []); + } finally { + await phone.close(); + } + console.log("PASS a phone's sheet closes on Back without leaving its pane, and a sheet closed by Cancel leaves no Back step behind"); } finally { await browser.close(); } diff --git a/src/components/RowMenu.tsx b/src/components/RowMenu.tsx index 536649f7d5..482bdcd49b 100644 --- a/src/components/RowMenu.tsx +++ b/src/components/RowMenu.tsx @@ -2,6 +2,7 @@ * The ⋯ menu of a sidebar row. On a desktop it is a popover under its button, drawn through a * portal at fixed coordinates: the roster scrolls, and a menu inside it would be cut off at the * list's edge. At phone width it is the modal primitive, which is already a bottom sheet there. + * The system Back closes the sheet (lib/sheetHistory.ts). * Escape, a press outside, focus leaving it (the palette opening over it, a Tab out) and, on a * desktop, a scroll of what holds the button or a resize close it. Focus goes back to the button when the menu goes, * unless what an item mounted (a rename field, a confirm) takes it first: the button is focused @@ -16,6 +17,7 @@ import "./RowMenu.css"; import { useT } from "../lib/i18n.ts"; import { useMediaQuery } from "../lib/useMediaQuery.ts"; import { SHEET_QUERY, useSheetSwipe } from "../lib/sheets.ts"; +import { useSheetHistory } from "../lib/sheetHistory.ts"; export interface RowMenuItem { id: string; @@ -64,6 +66,8 @@ export function RowMenu({ anchor, point, restoreFocusTo, title, subtitle, header const sheet = useMediaQuery(SHEET_QUERY); const surface = useRef(null); useSheetSwipe(surface, sheet ? onClose : null); + // Back closes the sheet, not the pane under it + useSheetHistory(sheet, onClose); const [place, setPlace] = useState<{ top: number; left: number; maxHeight: number } | null>(null); // where the button was when the menu was placed: a scroll that leaves it there is not a reason to close const placedAt = useRef<{ top: number; left: number } | null>(null); diff --git a/src/lib/sheetHistory.ts b/src/lib/sheetHistory.ts new file mode 100644 index 0000000000..bd7e31f215 --- /dev/null +++ b/src/lib/sheetHistory.ts @@ -0,0 +1,87 @@ +/** + * A phone's bottom sheet is a history entry, so the system Back closes it instead of going to the + * pane before (deep links make each pane its own entry). The entry keeps the address it opened on. + * A sheet closed any other way steps back off its entry once whatever it ran has written its own + * history; an entry left under a newer one (an item that opened another pane) is stepped over + * when Back lands on it, since the entry below it names the same place. + */ +import { useEffect, useRef } from "react"; + +const KEY = "herdr-web-ui:sheet"; + +const open = new Map void>(); +const leaving = new Map>(); +let pushing = false; + +function sheetOf(state: unknown): string | null { + if (state === null || typeof state !== "object") return null; + const id = (state as Record)[KEY]; + return typeof id === "string" ? id : null; +} + +function without(state: unknown): unknown { + if (state === null || typeof state !== "object") return state; + const { [KEY]: _, ...rest } = state as Record; + return rest; +} + +function enter(onBack: () => void, reuse: string | null): string { + // StrictMode's second mount, or a sheet reopened before its entry went: the same entry + const timer = reuse === null ? undefined : leaving.get(reuse); + if (reuse !== null && timer !== undefined && sheetOf(window.history.state) === reuse) { + clearTimeout(timer); + leaving.delete(reuse); + open.set(reuse, onBack); + return reuse; + } + const id = `${Date.now().toString(36)}${Math.random().toString(36).slice(2, 8)}`; + const base = window.history.state; + pushing = true; + try { window.history.pushState({ ...(base !== null && typeof base === "object" ? base : {}), [KEY]: id }, ""); } + finally { pushing = false; } + open.set(id, onBack); + return id; +} + +function leave(id: string): void { + if (!open.delete(id)) return; + // after the closing click's commit and effects, which may push an entry of their own + leaving.set(id, setTimeout(() => { + leaving.delete(id); + if (sheetOf(window.history.state) === id) window.history.back(); + }, 0)); +} + +if (typeof window !== "undefined") { + // an entry pushed over a sheet's copies its state (the file viewer's does): only the sheet's own carries its mark + const history = window.history; + const push = history.pushState.bind(history); + history.pushState = ((state: unknown, ...rest: [string, (string | URL | null)?]) => push(pushing ? state : without(state), ...rest)) as History["pushState"]; + window.addEventListener("popstate", (event) => { + const landed = sheetOf(event.state); + for (const [id, onBack] of [...open]) { + if (id === landed) continue; + open.delete(id); + onBack(); + } + if (landed !== null && !open.has(landed) && !leaving.has(landed)) { + window.history.replaceState(without(event.state), ""); + window.history.back(); + } + }); + // a reload keeps the history and not the sheet: its entry is an ordinary one now + if (sheetOf(window.history.state) !== null) window.history.replaceState(without(window.history.state), ""); +} + +/** While `active`, the sheet holds a history entry; Back calls `onBack`. */ +export function useSheetHistory(active: boolean, onBack: () => void): void { + const latest = useRef(onBack); + latest.current = onBack; + const id = useRef(null); + useEffect(() => { + if (!active) return; + const entry = enter(() => latest.current(), id.current); + id.current = entry; + return () => leave(entry); + }, [active]); +} From 77f914bc819adb56ed5b787b6511ac23e25d6acd Mon Sep 17 00:00:00 2001 From: Hako Date: Mon, 12 Oct 2026 06:28:50 +0900 Subject: [PATCH 2/2] fix(mobile): let a sheet's item take over the sheet's history entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the sheet entry (Codex, pre-merge): - More → New tab closed the dialog by itself: the app replaced the sheet entry's address with the dialog's, and the sheet's cleanup then stepped back off it. An entry whose address changed while the sheet left is kept. - A navigation an item pushed (another pane, Settings, a file) left the sheet's entry between, so Forward landed on a duplicate. The push now replaces the sheet's entry; a sheet opened from a sheet reuses it. - Forward onto the entry of a sheet Back closed went back again; it goes on. Unit tests drive the module through a fake history; the phone deep-link check covers New tab and Move pane to… from the More sheet. Co-Authored-By: Claude Fable 5.1 --- scripts/deep-link-demo-regression.ts | 30 +++++++- src/lib/sheetHistory.test.ts | 111 +++++++++++++++++++++++++++ src/lib/sheetHistory.ts | 58 ++++++++++---- 3 files changed, 184 insertions(+), 15 deletions(-) create mode 100644 src/lib/sheetHistory.test.ts diff --git a/scripts/deep-link-demo-regression.ts b/scripts/deep-link-demo-regression.ts index d011c0358d..b1d2ea8ecb 100644 --- a/scripts/deep-link-demo-regression.ts +++ b/scripts/deep-link-demo-regression.ts @@ -504,11 +504,39 @@ try { await page.waitForFunction(() => !Object.keys(history.state ?? {}).includes("herdr-web-ui:sheet")); await page.goBack(); await waitSelected(page, INFRA); + await page.goForward(); + await waitSelected(page, DOCS); + // an item that opens a dialog on the sheet's address keeps the sheet's entry: the New tab + // dialog stays up once the sheet's closing has settled, and Back closes the dialog alone + await more.tap(); + await sheet.waitFor(); + await sheet.getByRole("button", { name: "New tab", exact: true }).tap(); + const creation = page.locator(".new-session-modal"); + await creation.waitFor(); + await page.waitForTimeout(150); + assert.equal(await creation.isVisible(), true, "the New tab dialog stays up after the sheet's entry settles"); + assert.equal(await page.evaluate(() => history.length), length + 1, "the dialog took the sheet's entry"); + await page.goBack(); + await creation.waitFor({ state: "detached" }); + assert.equal(await selected(page), DOCS, "Back closed the dialog and left the pane alone"); + // a sheet opened from a sheet (More → Move pane to…) reuses the entry: one Back closes it + await more.tap(); + await sheet.waitFor(); + await sheet.getByRole("button", { name: "Move pane to…", exact: true }).tap(); + const move = page.getByRole("dialog", { name: /^Move / }); + await move.waitFor(); + await page.waitForTimeout(150); + assert.equal(await page.evaluate(() => history.length), length + 1, "the second sheet took the first's entry"); + await page.goBack(); + await move.waitFor({ state: "detached" }); + assert.equal(await selected(page), DOCS, "Back closed the second sheet and left the pane alone"); + await page.goBack(); + await waitSelected(page, INFRA); assert.deepEqual(errors, []); } finally { await phone.close(); } - console.log("PASS a phone's sheet closes on Back without leaving its pane, and a sheet closed by Cancel leaves no Back step behind"); + console.log("PASS a phone's sheet closes on Back without leaving its pane; Cancel, New tab and a second sheet leave no Back step behind"); } finally { await browser.close(); } diff --git a/src/lib/sheetHistory.test.ts b/src/lib/sheetHistory.test.ts new file mode 100644 index 0000000000..fb32048663 --- /dev/null +++ b/src/lib/sheetHistory.test.ts @@ -0,0 +1,111 @@ +import { afterAll, describe, expect, it } from "bun:test"; + +const originalWindow = Object.getOwnPropertyDescriptor(globalThis, "window"); + +/** A browser's history as the module uses it: entries with a state and an address, a cursor, popstate after a traversal. */ +function fakeWindow() { + const entries: Array<{ state: unknown; href: string }> = [{ state: null, href: "/?pane=a" }]; + let index = 0; + const listeners: Array<(event: { state: unknown }) => void> = []; + const traverse = (by: number): void => { + const next = Math.max(0, Math.min(entries.length - 1, index + by)); + if (next === index) return; + index = next; + queueMicrotask(() => { for (const listener of listeners) listener({ state: entries[index]!.state }); }); + }; + const history = { + get state() { return entries[index]!.state; }, + pushState(state: unknown, _title: string, url?: string | null) { entries.splice(index + 1); entries.push({ state, href: url ?? entries[index]!.href }); index += 1; }, + replaceState(state: unknown, _title: string, url?: string | null) { entries[index] = { state, href: url ?? entries[index]!.href }; }, + back() { traverse(-1); }, + forward() { traverse(1); }, + go(by: number) { traverse(by); }, + }; + const window = { + history, + get location() { return { href: entries[index]!.href }; }, + addEventListener(type: string, listener: (event: { state: unknown }) => void) { if (type === "popstate") listeners.push(listener); }, + }; + return { window, entries, at: () => index, marks: () => entries.map((entry) => typeof (entry.state as Record | null)?.["herdr-web-ui:sheet"] === "string") }; +} +const fake = fakeWindow(); +(globalThis as unknown as { window: unknown }).window = fake.window; +// after the window: the module wraps and listens to its history as it loads +const settled = async () => { await new Promise((resolve) => setTimeout(resolve, 5)); for (let i = 0; i < 4; i++) await new Promise((resolve) => queueMicrotask(() => resolve(undefined))); }; + +const h = fake.window.history; +/** RowMenu's effect, without React: the hook's enter on mount and leave on unmount (sheetHistory exports them for this) */ +const { enterSheet, leaveSheet } = await import("./sheetHistory.ts"); +const hooks = { enter: (onBack: () => void): (() => void) => { const id = enterSheet(onBack, null); return () => leaveSheet(id); } }; + +afterAll(() => { + if (originalWindow) Object.defineProperty(globalThis, "window", originalWindow); else delete (globalThis as { window?: unknown }).window; +}); + +describe("a phone's sheet in the history", () => { + it("is an entry Back closes, and leaves no entry behind when closed another way", async () => { + let closed = 0; + const leave = hooks.enter(() => { closed += 1; }); + expect(fake.entries.length).toBe(2); expect(fake.marks()).toEqual([false, true]); + h.back(); await settled(); + expect(closed).toBe(1); expect(fake.at()).toBe(0); + leave(); + // Cancel: the sheet's entry is stepped off after the click's effects + const leave2 = hooks.enter(() => { closed += 1; }); + expect(fake.at()).toBe(1); + leave2(); await settled(); + expect(fake.at()).toBe(0); expect(closed).toBe(1); + }); + + it("gives its entry to a navigation an item makes, so Back goes to the pane before and Forward returns", async () => { + // on pane a (entry 0); the retired entry of the first test lies at 1 + const leave = hooks.enter(() => {}); + const length = fake.entries.length; + leave(); + // the item's navigation, in the same click: another pane is an entry of its own in the app + h.pushState({ nav: true }, "", "/?pane=b"); + await settled(); + expect(fake.entries.length).toBe(length); + expect(fake.marks()).toEqual([false, false]); + expect(fake.window.location.href).toBe("/?pane=b"); + h.back(); await settled(); + expect(fake.window.location.href).toBe("/?pane=a"); + h.forward(); await settled(); + expect(fake.window.location.href).toBe("/?pane=b"); + }); + + it("keeps its entry for an item that changed the address on it, instead of stepping back", async () => { + // the New tab dialog: the app replaces the entry's address while the sheet leaves + const leave = hooks.enter(() => {}); + leave(); + h.replaceState(h.state, "", "/?pane=b&new=tab"); + await settled(); + expect(fake.window.location.href).toBe("/?pane=b&new=tab"); + expect(fake.marks().at(-1)).toBe(false); + h.back(); await settled(); + expect(fake.window.location.href).toBe("/?pane=b"); + }); + + it("reuses the entry for a sheet opened from a sheet", async () => { + const start = fake.at(); + const leaveFirst = hooks.enter(() => {}); + leaveFirst(); + // More → Move pane to…: the second sheet mounts in the first's closing click + const leaveSecond = hooks.enter(() => {}); + await settled(); + expect(fake.at()).toBe(start + 1); expect(fake.marks()[start + 1]).toBe(true); + leaveSecond(); await settled(); + expect(fake.at()).toBe(start); + }); + + it("goes on past the entry of a sheet that Back closed when Forward lands on it", async () => { + const start = fake.at(); + let closed = 0; + hooks.enter(() => { closed += 1; }); + h.back(); await settled(); + expect(closed).toBe(1); expect(fake.at()).toBe(start); + // beyond the retired entry: nothing, so Forward stays put on it as an ordinary entry + h.forward(); await settled(); + expect(fake.at()).toBe(start + 1); expect(fake.marks()[start + 1]).toBe(false); + }); +}); diff --git a/src/lib/sheetHistory.ts b/src/lib/sheetHistory.ts index bd7e31f215..f2a906b386 100644 --- a/src/lib/sheetHistory.ts +++ b/src/lib/sheetHistory.ts @@ -1,9 +1,11 @@ /** * A phone's bottom sheet is a history entry, so the system Back closes it instead of going to the * pane before (deep links make each pane its own entry). The entry keeps the address it opened on. - * A sheet closed any other way steps back off its entry once whatever it ran has written its own - * history; an entry left under a newer one (an item that opened another pane) is stepped over - * when Back lands on it, since the entry below it names the same place. + * What an item of the sheet does with the history takes the sheet's entry over: a navigation it + * pushes (another pane, Settings, a file) replaces the entry, a sheet it opens reuses it, and an + * address it changes on the entry (the New tab dialog) keeps it. A sheet closed any other way steps + * back off its entry once whatever it ran has written its own history. A sheet closed by Back + * leaves its entry ahead of the app; Forward onto it goes on to the next. */ import { useEffect, useRef } from "react"; @@ -11,6 +13,8 @@ const KEY = "herdr-web-ui:sheet"; const open = new Map void>(); const leaving = new Map>(); +/** the address each sheet's entry was made on */ +const addresses = new Map(); let pushing = false; function sheetOf(state: unknown): string | null { @@ -25,7 +29,14 @@ function without(state: unknown): unknown { return rest; } -function enter(onBack: () => void, reuse: string | null): string { +/** whether the entry the state names belongs to a sheet that is up, or just closed and not yet stepped off */ +function owned(state: unknown): boolean { + const id = sheetOf(state); + return id !== null && (open.has(id) || leaving.has(id)); +} + +/** Makes the sheet's entry (RowMenu's effect on mount; exported for its test). */ +export function enterSheet(onBack: () => void, reuse: string | null): string { // StrictMode's second mount, or a sheet reopened before its entry went: the same entry const timer = reuse === null ? undefined : leaving.get(reuse); if (reuse !== null && timer !== undefined && sheetOf(window.history.state) === reuse) { @@ -36,27 +47,44 @@ function enter(onBack: () => void, reuse: string | null): string { } const id = `${Date.now().toString(36)}${Math.random().toString(36).slice(2, 8)}`; const base = window.history.state; + const state = { ...(base !== null && typeof base === "object" ? base : {}), [KEY]: id }; pushing = true; - try { window.history.pushState({ ...(base !== null && typeof base === "object" ? base : {}), [KEY]: id }, ""); } - finally { pushing = false; } + try { + // a sheet opened from another sheet (More → Move pane to…) takes that sheet's entry: one Back closes it + if (owned(base)) window.history.replaceState(state, ""); + else window.history.pushState(state, ""); + } finally { pushing = false; } open.set(id, onBack); + addresses.set(id, window.location.href); return id; } -function leave(id: string): void { +/** Steps off the sheet's entry once the closing click's effects are through (RowMenu's effect on unmount). */ +export function leaveSheet(id: string): void { if (!open.delete(id)) return; - // after the closing click's commit and effects, which may push an entry of their own + // after the closing click's commit and effects, which may write history of their own leaving.set(id, setTimeout(() => { leaving.delete(id); - if (sheetOf(window.history.state) === id) window.history.back(); + const address = addresses.get(id); + addresses.delete(id); + if (sheetOf(window.history.state) !== id) return; + // an item that changed the address on the sheet's entry (the New tab dialog) keeps the entry as its own + if (window.location.href !== address) window.history.replaceState(without(window.history.state), ""); + else window.history.back(); }, 0)); } if (typeof window !== "undefined") { - // an entry pushed over a sheet's copies its state (the file viewer's does): only the sheet's own carries its mark const history = window.history; const push = history.pushState.bind(history); - history.pushState = ((state: unknown, ...rest: [string, (string | URL | null)?]) => push(pushing ? state : without(state), ...rest)) as History["pushState"]; + history.pushState = ((state: unknown, ...rest: [string, (string | URL | null)?]) => { + if (pushing) return push(state, ...rest); + // a navigation made from a sheet (an item that opened another pane, Settings, a file) takes the + // sheet's entry: Back from there goes to the pane before the sheet, and no entry is left between + if (owned(history.state)) return history.replaceState(without(state), ...rest); + // an entry pushed over a sheet's copies its state (the file viewer's does): only the sheet's own carries its mark + return push(without(state), ...rest); + }) as History["pushState"]; window.addEventListener("popstate", (event) => { const landed = sheetOf(event.state); for (const [id, onBack] of [...open]) { @@ -64,9 +92,11 @@ if (typeof window !== "undefined") { open.delete(id); onBack(); } + // the entry of a sheet that Back closed lies ahead of the app: Forward lands on it and goes on, + // and the entry is an ordinary one from here if (landed !== null && !open.has(landed) && !leaving.has(landed)) { window.history.replaceState(without(event.state), ""); - window.history.back(); + window.history.forward(); } }); // a reload keeps the history and not the sheet: its entry is an ordinary one now @@ -80,8 +110,8 @@ export function useSheetHistory(active: boolean, onBack: () => void): void { const id = useRef(null); useEffect(() => { if (!active) return; - const entry = enter(() => latest.current(), id.current); + const entry = enterSheet(() => latest.current(), id.current); id.current = entry; - return () => leave(entry); + return () => leaveSheet(entry); }, [active]); }