mirror of
https://github.com/block/buzz.git
synced 2026-11-04 08:21:48 +00:00
fix(desktop): keep forward history across replace navigations
Resolves the review comments on the back/forward history menus.
A `replace: true` navigation mints a fresh __TSR_key at the *same*
history index, which the old heuristic read as "the forward stack was
truncated". Mid-history replaces — the huddle redirects, settings
section switches, `goHome({ replace: true })` — therefore wiped the
tracked forward entries and disabled the forward control even though
`history.go(1)` still worked. Only a push moves the index forward, so
truncation now keys off an index that advanced past a stale entry.
The fold is extracted as a pure `recordHistoryVisit` reducer so the
push/replace/traverse cases are unit-testable without a renderer.
Also:
- Route labels move to a table that falls back to the raw pathname. The
old chain ended in `return "Inbox"`, so any future route would have
shown up in the menus as the inbox with nothing to flag the omission.
- The right-click vs long-press E2E now compares menu geometry and item
text instead of raw screenshot bytes; a mismatch names the field that
drifted rather than handing back an opaque Buffer diff.
The reported left-click-while-open double action does not reproduce:
Radix's modal dismiss layer sets `pointer-events: none` on the body, so
the click lands on the layer and never reaches the button — one click
dismisses and nothing else, matching the native control. Locked in with
an E2E case that also proves the next click still navigates.
Verified: pnpm typecheck, pnpm check, pnpm test (5517 pass),
check:file-sizes, and the navigation smoke spec (21 pass, 1 pre-existing
fixme).
Signed-off-by: Matt Toohey <contact@matttoohey.com>
This commit is contained in:
@@ -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",
|
||||
);
|
||||
});
|
||||
|
||||
@@ -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<number, NavigationHistoryEntry>;
|
||||
/** 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<number, NavigationHistoryEntry>,
|
||||
@@ -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<string, string> = {
|
||||
"/": "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";
|
||||
}
|
||||
|
||||
@@ -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<number, NavigationHistoryEntry>([
|
||||
[
|
||||
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;
|
||||
|
||||
@@ -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,
|
||||
}) => {
|
||||
|
||||
Reference in New Issue
Block a user