diff --git a/desktop/src/app/navigation/navigationHistory.test.mjs b/desktop/src/app/navigation/navigationHistory.test.mjs index 8886a95d4b..b6a04eab2c 100644 --- a/desktop/src/app/navigation/navigationHistory.test.mjs +++ b/desktop/src/app/navigation/navigationHistory.test.mjs @@ -2,15 +2,21 @@ import assert from "node:assert/strict"; import test from "node:test"; import { + createNavigationHistoryState, describeHistoryLocation, getBackHistoryEntries, getForwardHistoryEntries, + recordHistoryVisit, } from "./navigationHistory.ts"; function entry(index) { return { index, key: `key-${index}`, label: `Entry ${index}` }; } +function visit(state, index, key, label) { + return recordHistoryVisit(state, { index, key, label }); +} + test("back history returns the nearest ten entries in reverse order", () => { const entriesByIndex = new Map( Array.from({ length: 13 }, (_, index) => [index, entry(index)]), @@ -73,6 +79,80 @@ test("history labels identify channel and thread destinations", () => { ); }); +test("replacing an entry mid-history keeps the forward entries", () => { + // Inbox → #general → #random, then back to #general. + let state = createNavigationHistoryState({ + index: 0, + key: "key-inbox", + label: "Inbox", + }); + state = visit(state, 1, "key-general", "#general"); + state = visit(state, 2, "key-random", "#random"); + state = visit(state, 1, "key-general", "#general"); + + // A `replace: true` navigation mints a fresh key at the same index while the + // browser keeps entry 2 reachable with forward. + state = visit(state, 1, "key-general-replaced", "#general thread"); + + assert.equal(state.maxIndex, 2); + assert.deepEqual( + getForwardHistoryEntries(state.entriesByIndex, 1, state.maxIndex).map( + ({ label }) => label, + ), + ["#random"], + ); + assert.equal(state.entriesByIndex.get(1).label, "#general thread"); +}); + +test("pushing from mid-history drops the stale forward entries", () => { + let state = createNavigationHistoryState({ + index: 0, + key: "key-inbox", + label: "Inbox", + }); + state = visit(state, 1, "key-general", "#general"); + state = visit(state, 2, "key-random", "#random"); + state = visit(state, 1, "key-general", "#general"); + state = visit(state, 0, "key-inbox", "Inbox"); + + // Navigating anew from the inbox invalidates both entries ahead of it. + state = visit(state, 1, "key-design", "#design"); + + assert.equal(state.maxIndex, 1); + assert.deepEqual([...state.entriesByIndex.keys()], [0, 1]); + assert.deepEqual( + getForwardHistoryEntries(state.entriesByIndex, 1, state.maxIndex), + [], + ); +}); + +test("traversing back and forward leaves the tracked entries untouched", () => { + let state = createNavigationHistoryState({ + index: 0, + key: "key-inbox", + label: "Inbox", + }); + state = visit(state, 1, "key-general", "#general"); + state = visit(state, 2, "key-random", "#random"); + state = visit(state, 0, "key-inbox", "Inbox"); + + assert.equal(state.maxIndex, 2); + assert.deepEqual( + getForwardHistoryEntries(state.entriesByIndex, 0, state.maxIndex).map( + ({ label }) => label, + ), + ["#general", "#random"], + ); + + state = visit(state, 2, "key-random", "#random"); + + assert.equal(state.maxIndex, 2); + assert.deepEqual( + getBackHistoryEntries(state.entriesByIndex, 2).map(({ label }) => label), + ["#general", "Inbox"], + ); +}); + test("history labels cover static and detail routes", () => { assert.equal( describeHistoryLocation({ pathname: "/", search: {} }, []), @@ -90,3 +170,10 @@ test("history labels cover static and detail routes", () => { "Project details", ); }); + +test("an unlabelled route falls back to its pathname, not the inbox", () => { + assert.equal( + describeHistoryLocation({ pathname: "/not-labelled-yet", search: {} }, []), + "/not-labelled-yet", + ); +}); diff --git a/desktop/src/app/navigation/navigationHistory.ts b/desktop/src/app/navigation/navigationHistory.ts index 6ba95efb73..8dc7844cdf 100644 --- a/desktop/src/app/navigation/navigationHistory.ts +++ b/desktop/src/app/navigation/navigationHistory.ts @@ -1,4 +1,5 @@ import type { Channel } from "@/shared/api/types"; +import { trimMapToSize } from "@/shared/lib/trimMapToSize"; export type NavigationHistoryEntry = { index: number; @@ -6,7 +7,66 @@ export type NavigationHistoryEntry = { label: string; }; +export type NavigationHistoryState = { + entriesByIndex: Map; + /** Index of the entry the previous visit landed on. */ + lastIndex: number; + /** Highest index still reachable with forward navigation. */ + maxIndex: number; +}; + const MAX_HISTORY_MENU_ENTRIES = 10; +const MAX_TRACKED_HISTORY_ENTRIES = 200; + +export function createNavigationHistoryState( + entry: NavigationHistoryEntry, +): NavigationHistoryState { + return { + entriesByIndex: new Map([[entry.index, entry]]), + lastIndex: entry.index, + maxIndex: entry.index, + }; +} + +/** + * Folds the location the router just landed on into the tracked history. + * + * TanStack's history mints a fresh `__TSR_key` for pushes *and* replaces, so + * the key alone cannot tell them apart. Only a push moves the index forward, + * and only a push from mid-history drops the browser's forward entries. A + * replace performed mid-history — `goHome({ replace: true })`, the huddle + * redirects, settings section switches — keeps them, so it must not clear + * what we track or forward navigation would go dark while `history.go(1)` + * still works. + */ +export function recordHistoryVisit( + state: NavigationHistoryState, + entry: NavigationHistoryEntry, +): NavigationHistoryState { + const entriesByIndex = new Map(state.entriesByIndex); + const pushedOverForwardEntries = + entry.index > state.lastIndex && + entriesByIndex.get(entry.index)?.key !== entry.key; + + if (pushedOverForwardEntries) { + for (const storedIndex of entriesByIndex.keys()) { + if (storedIndex >= entry.index) { + entriesByIndex.delete(storedIndex); + } + } + } + + entriesByIndex.set(entry.index, entry); + trimMapToSize(entriesByIndex, MAX_TRACKED_HISTORY_ENTRIES); + + return { + entriesByIndex, + lastIndex: entry.index, + maxIndex: pushedOverForwardEntries + ? entry.index + : Math.max(state.maxIndex, entry.index), + }; +} export function getBackHistoryEntries( entriesByIndex: ReadonlyMap, @@ -63,6 +123,25 @@ function searchHasValue(search: unknown, key: string): boolean { return typeof value === "string" && value.length > 0; } +// Mirrors the route table in `routes.ts`. A route missing from here falls back +// to its raw pathname rather than a plausible-looking label, so the omission is +// visible instead of masquerading as some other destination. +const ROUTE_LABELS: Record = { + "/": "Inbox", + "/agents": "Agents", + "/messages/new": "New message", + "/projects": "Projects", + "/pulse": "Pulse", + "/reminders": "Reminders", + "/settings": "Settings", + "/workflows": "Workflows", +}; + +const ROUTE_PREFIX_LABELS: readonly (readonly [string, string])[] = [ + ["/projects/", "Project details"], + ["/workflows/", "Workflow details"], +]; + export function describeHistoryLocation( location: HistoryLocation, channels: readonly Channel[], @@ -92,15 +171,12 @@ export function describeHistoryLocation( return channelLabel; } - if (pathname === "/messages/new") return "New message"; - if (pathname === "/agents") return "Agents"; - if (pathname === "/workflows") return "Workflows"; - if (pathname.startsWith("/workflows/")) return "Workflow details"; - if (pathname === "/projects") return "Projects"; - if (pathname.startsWith("/projects/")) return "Project details"; - if (pathname === "/pulse") return "Pulse"; - if (pathname === "/reminders") return "Reminders"; - if (pathname === "/settings") return "Settings"; + const routeLabel = ROUTE_LABELS[pathname]; + if (routeLabel) return routeLabel; - return "Inbox"; + for (const [prefix, label] of ROUTE_PREFIX_LABELS) { + if (pathname.startsWith(prefix)) return label; + } + + return pathname || "Inbox"; } diff --git a/desktop/src/app/navigation/useBackForwardControls.ts b/desktop/src/app/navigation/useBackForwardControls.ts index f9c87575d1..91e9708082 100644 --- a/desktop/src/app/navigation/useBackForwardControls.ts +++ b/desktop/src/app/navigation/useBackForwardControls.ts @@ -9,16 +9,16 @@ import { listen } from "@tauri-apps/api/event"; import { matchBackForwardChord } from "@/app/navigation/backForwardChords"; import { - type NavigationHistoryEntry, + createNavigationHistoryState, getBackHistoryEntries, getForwardHistoryEntries, + recordHistoryVisit, } from "@/app/navigation/navigationHistory"; import { traverseHistory, traverseHistoryBy, } from "@/app/navigation/navigationGuard"; import { isMacPlatform } from "@/shared/lib/platform"; -import { trimMapToSize } from "@/shared/lib/trimMapToSize"; type RouterHistoryState = { __TSR_index?: number; @@ -26,8 +26,6 @@ type RouterHistoryState = { key?: string; }; -const MAX_TRACKED_HISTORY_ENTRIES = 200; - export function useBackForwardControls(currentLabel: string) { const router = useRouter(); const canGoBack = useCanGoBack(); @@ -37,45 +35,22 @@ export function useBackForwardControls(currentLabel: string) { const locationIndex = locationState.__TSR_index ?? 0; const locationKey = locationState.__TSR_key ?? locationState.key ?? String(locationIndex); - const [historyState, setHistoryState] = React.useState(() => ({ - entriesByIndex: new Map([ - [ - locationIndex, - { index: locationIndex, key: locationKey, label: currentLabel }, - ], - ]), - maxIndex: locationIndex, - })); + const [historyState, setHistoryState] = React.useState(() => + createNavigationHistoryState({ + index: locationIndex, + key: locationKey, + label: currentLabel, + }), + ); React.useEffect(() => { - setHistoryState((current) => { - const entriesByIndex = new Map(current.entriesByIndex); - const currentEntry = entriesByIndex.get(locationIndex); - const replacedForwardEntry = - currentEntry !== undefined && currentEntry.key !== locationKey; - - if (replacedForwardEntry) { - for (const storedIndex of entriesByIndex.keys()) { - if (storedIndex >= locationIndex) { - entriesByIndex.delete(storedIndex); - } - } - } - - entriesByIndex.set(locationIndex, { + setHistoryState((current) => + recordHistoryVisit(current, { index: locationIndex, key: locationKey, label: currentLabel, - }); - trimMapToSize(entriesByIndex, MAX_TRACKED_HISTORY_ENTRIES); - - return { - entriesByIndex, - maxIndex: replacedForwardEntry - ? locationIndex - : Math.max(current.maxIndex, locationIndex), - }; - }); + }), + ); }, [currentLabel, locationIndex, locationKey]); const canGoForward = locationIndex < historyState.maxIndex; diff --git a/desktop/tests/e2e/navigation.spec.ts b/desktop/tests/e2e/navigation.spec.ts index f93bac4da7..aa9244491d 100644 --- a/desktop/tests/e2e/navigation.spec.ts +++ b/desktop/tests/e2e/navigation.spec.ts @@ -58,50 +58,37 @@ async function openHistoryMenuWithLongPress( await expect(menu).toBeVisible(); } -async function captureHistoryControlAppearance( +/** + * Describes an open history menu by where it sits relative to its button, how + * big it is, and what it lists. Comparing this instead of raw pixels keeps the + * "both gestures open the same menu" assertion legible: a mismatch names the + * field that drifted rather than handing back an opaque Buffer diff. + */ +async function readHistoryMenuLayout( page: import("@playwright/test").Page, button: import("@playwright/test").Locator, menu: import("@playwright/test").Locator, + itemTestId: string, ) { await waitForAnimations(page); - const [buttonBounds, menuBounds] = await Promise.all([ + const [buttonBounds, menuBounds, items] = await Promise.all([ button.boundingBox(), menu.boundingBox(), + menu.getByTestId(itemTestId).allTextContents(), ]); if (!buttonBounds || !menuBounds) { throw new Error("History control is not visible"); } - const padding = 4; - const x = Math.max( - 0, - Math.floor(Math.min(buttonBounds.x, menuBounds.x)) - padding, - ); - const y = Math.max( - 0, - Math.floor(Math.min(buttonBounds.y, menuBounds.y)) - padding, - ); - const right = Math.ceil( - Math.max( - buttonBounds.x + buttonBounds.width, - menuBounds.x + menuBounds.width, - ), - ); - const bottom = Math.ceil( - Math.max( - buttonBounds.y + buttonBounds.height, - menuBounds.y + menuBounds.height, - ), - ); - - return page.screenshot({ - clip: { - height: bottom - y + padding, - width: right - x + padding, - x, - y, - }, - }); + return { + height: Math.round(menuBounds.height), + items, + // Radix anchors a context menu at the pointer, and both gestures point at + // the button's centre, so the offset must come out identical. + offsetX: Math.round(menuBounds.x - buttonBounds.x), + offsetY: Math.round(menuBounds.y - buttonBounds.y), + width: Math.round(menuBounds.width), + }; } async function createWorkflow( @@ -180,10 +167,11 @@ test("back and forward history menus match across right click and long press", a "#general", "Inbox", ]); - const rightClickAppearance = await captureHistoryControlAppearance( + const rightClickLayout = await readHistoryMenuLayout( page, backButton, historyMenu, + "global-back-history-item", ); await page.keyboard.press("Escape"); await expect(historyMenu).not.toBeVisible(); @@ -195,8 +183,13 @@ test("back and forward history menus match across right click and long press", a "Inbox", ]); expect( - await captureHistoryControlAppearance(page, backButton, historyMenu), - ).toEqual(rightClickAppearance); + await readHistoryMenuLayout( + page, + backButton, + historyMenu, + "global-back-history-item", + ), + ).toEqual(rightClickLayout); await historyMenu .getByRole("menuitem", { name: "Go back to #general" }) .click(); @@ -211,10 +204,11 @@ test("back and forward history menus match across right click and long press", a await expect( forwardHistoryMenu.getByTestId("global-forward-history-item"), ).toHaveText(["#random", "#engineering"]); - const forwardRightClickAppearance = await captureHistoryControlAppearance( + const forwardRightClickLayout = await readHistoryMenuLayout( page, forwardButton, forwardHistoryMenu, + "global-forward-history-item", ); await page.keyboard.press("Escape"); await expect(forwardHistoryMenu).not.toBeVisible(); @@ -224,18 +218,55 @@ test("back and forward history menus match across right click and long press", a forwardHistoryMenu.getByTestId("global-forward-history-item"), ).toHaveText(["#random", "#engineering"]); expect( - await captureHistoryControlAppearance( + await readHistoryMenuLayout( page, forwardButton, forwardHistoryMenu, + "global-forward-history-item", ), - ).toEqual(forwardRightClickAppearance); + ).toEqual(forwardRightClickLayout); await forwardHistoryMenu .getByRole("menuitem", { name: "Go forward to #engineering" }) .click(); await expect(page.getByTestId("chat-title")).toHaveText("engineering"); }); +test("left clicking a history button only dismisses its open menu", async ({ + page, +}) => { + await page.goto("/"); + + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); + await page.getByTestId("channel-random").click(); + await expect(page.getByTestId("chat-title")).toHaveText("random"); + + const backButton = page.getByTestId("global-back"); + const historyMenu = page.getByTestId("global-back-history-menu"); + await backButton.click({ button: "right" }); + await expect(historyMenu).toBeVisible(); + await waitForAnimations(page); + + // The open menu is modal, so its dismiss layer — not the button — takes the + // click. Drive the mouse directly: Playwright's actionability checks refuse + // to click a button that is covered. + const bounds = await backButton.boundingBox(); + expect(bounds).not.toBeNull(); + await page.mouse.click( + (bounds?.x ?? 0) + (bounds?.width ?? 0) / 2, + (bounds?.y ?? 0) + (bounds?.height ?? 0) / 2, + ); + + // One click closes the menu and does nothing else, like the native control. + await expect(historyMenu).not.toBeVisible(); + await expect(page.getByTestId("chat-title")).toHaveText("random"); + + // The next click is an ordinary back — proof the dismissal neither consumed + // it nor left the button inert. + await backButton.click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); +}); + test("back/forward keyboard chords work while the composer has focus", async ({ page, }) => {