From 92c305bab9281c6ce08b03d10d8adddf0131d244 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 00:17:09 +0200 Subject: [PATCH] fix(nav): a navigation closes every overlay before it pushes The overlay history stack let a navigation land ABOVE open overlays and "buried" their entries with a counter. Measured in Chrome: Shift+W over About navigated under the still-open dialog, the next Back both undid the navigation and closed About, and the Back after that did nothing; Shift+W on Home with the player open did nothing at all. The counter could also be spent by a different overlay closing first, and callers that could not report whether they pushed were assumed to have. setPage and openChannel now close every open overlay and push only once the history is back in sync (one entry per live overlay, no programmatic back() in flight, no reconcile queued), so nothing is ever buried and the whole mechanism goes: navigateOverOverlays, closeThenNavigate, buryUnder, setPage's boolean, and the three test files that pinned them. The palette closes only itself before running a row, so a theme picked over the player keeps the player. Shift+W on Home with the player open now closes it (owner decision 2026-09-09). The wait is on state, not on "a popstate": the old 250 ms fallback could run with back()s still in flight. An overlay that refuses to close within 1.5 s drops the command instead of being navigated out from under. --- frontend/src/App.keymap.contract.test.ts | 33 --- frontend/src/App.tsx | 16 +- frontend/src/components/ChannelNameLink.tsx | 2 +- .../NavigationProvider.dom.test.tsx | 153 ++++++---- .../src/components/NavigationProvider.tsx | 101 ++++--- frontend/src/components/PlayerModal.tsx | 14 +- .../components/ui/CommandPalette.dom.test.tsx | 2 +- .../ui/CommandPalette.ordering.dom.test.tsx | 16 +- frontend/src/components/ui/CommandPalette.tsx | 10 +- frontend/src/lib/history.burial.dom.test.ts | 43 --- frontend/src/lib/history.dom.test.ts | 267 +++++++++++------- frontend/src/lib/history.ts | 198 ++++++------- frontend/src/lib/nav.dom.test.ts | 40 --- frontend/src/lib/nav.ts | 19 +- 14 files changed, 449 insertions(+), 465 deletions(-) delete mode 100644 frontend/src/App.keymap.contract.test.ts delete mode 100644 frontend/src/lib/history.burial.dom.test.ts delete mode 100644 frontend/src/lib/nav.dom.test.ts diff --git a/frontend/src/App.keymap.contract.test.ts b/frontend/src/App.keymap.contract.test.ts deleted file mode 100644 index ee26db20..00000000 --- a/frontend/src/App.keymap.contract.test.ts +++ /dev/null @@ -1,33 +0,0 @@ -// The global Shift+W wiring, pinned at its call site. -// -// What this is: a source contract, the same idiom as uiBoundary.contract.test.ts — a structural rule -// asserted directly rather than left to prose. What it is NOT: a behaviour test. The behaviour lives -// in history.dom.test.ts and history.burial.dom.test.ts, which drive `navigateOverOverlays` against -// real overlay entries; the end-to-end gesture (Shift+W over an open player, then Back) is a browser -// check, because jsdom reproduces history ordering only as bookkeeping. -// -// It exists because the composition line itself was unpinned: App.tsx is the one module no test -// renders (main.tsx lazy-imports it and nothing else does), globalKeymap.dom.test.tsx drives the -// hook with its OWN handler, and history.dom.test.ts drives the helper with its own navigate — so -// dropping `navigateOverOverlays` from App left all 818 tests green while the user-visible bug came -// straight back (round 2, verified by running that revert). -import { readFileSync } from "node:fs"; -import { fileURLToPath } from "node:url"; - -import { describe, expect, it } from "vitest"; - -const APP = readFileSync(fileURLToPath(new URL("./App.tsx", import.meta.url)), "utf8"); - -describe("App · global keymap wiring", () => { - it("routes the global home shortcut through navigateOverOverlays", () => { - const call = /useGlobalKeymap\(([\s\S]*?)\);/.exec(APP)?.[1]; - // A rename or a move is a legitimate change — but then this test is the thing that has to be - // updated deliberately, which is the whole point of pinning it. - expect( - call, - "App.tsx no longer calls useGlobalKeymap in a shape this test can read", - ).toBeTypeOf("string"); - expect(call).toContain("navigateOverOverlays"); - expect(call).toContain('setPage("home")'); - }); -}); diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 707e07c6..869d7533 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -37,7 +37,6 @@ import Toaster from "./components/ui/Toaster"; import KeyboardHelp from "./components/ui/KeyboardHelp"; import AppCommandPalette from "./components/AppCommandPalette"; import { useGlobalKeymap } from "./lib/keymap"; -import { navigateOverOverlays } from "./lib/history"; import { StateMessage } from "./components/ui/QueryState"; import ErrorDialog from "./components/ErrorDialog"; import VersionBanner from "./components/VersionBanner"; @@ -142,13 +141,10 @@ export default function App() { // Home ("mit nézzek?"). Bound only once signed in, because that is where the overlays are mounted // (bottom of this tree) and where the modules exist at all — see useGlobalKeymap on why binding it // above the signed-out branch is worse than not binding it. - // Through `navigateOverOverlays`, because a global key can fire while an overlay is open and does - // not know it: the page change unmounts the player (it lives inside the module), whose reconcile - // then reclaimed the entry this push had just made — Shift+W over a playing video closed the - // player and snapped straight back to the feed. `setPage`'s boolean is load-bearing here, not - // decoration: pressed while ALREADY on Home (the default landing page) it navigates nowhere, and - // burying the open player's entry under a push that never happened orphaned it — see `buryUnder`. - useGlobalKeymap(!!meQuery.data, () => navigateOverOverlays(() => setPage("home"))); + // A global key can fire while an overlay is open; `setPage` closes every overlay before it pushes, + // so Shift+W over the player lands on Home with the player closed — also when Home is already the + // page underneath (owner decision 2026-09-09: the key always ends with the answer on screen). + useGlobalKeymap(!!meQuery.data, () => setPage("home")); // Pin this tab to whatever account it first loaded as. Without this, a tab that never picked // an account (it was riding the session's default) would silently swap identity when ANOTHER @@ -279,8 +275,8 @@ export default function App() { label: t("common.accessRequestsReview"), onClick: () => { focusAccessRequestsTab(); - // Over whatever overlay the toast was clicked in — as navigateTo does (see lib/nav). - navigateOverOverlays(() => setPage("users")); + // Clicked inside whatever overlay hosts the toast; setPage closes it before pushing. + setPage("users"); }, }, }); diff --git a/frontend/src/components/ChannelNameLink.tsx b/frontend/src/components/ChannelNameLink.tsx index f567573c..2be89068 100644 --- a/frontend/src/components/ChannelNameLink.tsx +++ b/frontend/src/components/ChannelNameLink.tsx @@ -11,7 +11,7 @@ import { urlWithParams } from "../lib/urlState"; // The href is the same URL `openChannel` pushes — `?chan=` on the current page — so a new tab // lands where the in-app navigation would have. A plain left click is still handled in-app, because // the SPA path does more than navigate (the player has to close first and reclaim its own -// Back-to-close history entry; see closeThenNavigate). +// Back-to-close history entry; see lib/history.closeOverlaysThenNavigate). type Props = Omit, "href" | "onClick" | "title"> & { channelId: string; title: string | null | undefined; diff --git a/frontend/src/components/NavigationProvider.dom.test.tsx b/frontend/src/components/NavigationProvider.dom.test.tsx index dc6e38a8..34606a34 100644 --- a/frontend/src/components/NavigationProvider.dom.test.tsx +++ b/frontend/src/components/NavigationProvider.dom.test.tsx @@ -1,16 +1,14 @@ // @vitest-environment jsdom // -// `setPage`'s no-op contract — the PRODUCER side of the history-burial fix, which nothing rendered -// before this. `setPage` early-returns when the requested page is already current with no channel -// page over it, and reports that by returning `false`; `lib/history.buryUnder` reads the boolean to -// decide whether the overlay entries beneath a navigation are still reclaimable. Get it wrong and -// the round-2 bug is back: Shift+W over the player on Home buries an entry nothing pushed past, and -// the user's next browser Back does nothing. +// The navigation actions against the overlay history stack: `setPage` and `openChannel` never push +// under an open overlay — they close it and push once its Back-to-close entry is reclaimed — and +// `setPage`'s same-page guard, which decides whether anything is pushed at all. // -// The type does NOT fence this. `NavigationActions` pins the SHAPE (`=> boolean`), so deleting one -// `return` fails tsc — but round 3 measured that swapping the two values (early return `true`, tail -// `false`) passes typecheck and all 822 tests. Hence assertions on the value AND on the effects the -// early return skips, which is what the boolean actually stands for. +// The overlay half is what used to live at each call site (Shift+W in App, a toast's "View", the +// palette, the player's channel link) and could be forgotten at any of them; it now lives in the two +// actions, so this file is where it is pinned. The end-to-end gesture — Shift+W over the player, +// then browser Back — is still checked in a real browser: jsdom reproduces history ordering only as +// bookkeeping. import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; import { renderHook, act, cleanup } from "@testing-library/react"; import type { ReactNode } from "react"; @@ -27,6 +25,7 @@ vi.mock("react-i18next", async (importOriginal) => { }); import { NavigationProvider, useNavigation, useNavigationActions } from "./NavigationProvider"; +import { useBackToClose } from "../lib/history"; const wrapper = ({ children }: { children: ReactNode }) => ( {children} @@ -34,6 +33,25 @@ const wrapper = ({ children }: { children: ReactNode }) => ( const render = () => renderHook(() => ({ ...useNavigation(), ...useNavigationActions() }), { wrapper }); +const settle = () => + act(async () => { + await new Promise((r) => setTimeout(r, 0)); + }); + +/** An open overlay (the player) whose close really unmounts it; its reclaiming back() is stubbed to + * land its popstate at once. */ +async function openOverlay() { + const back = vi.spyOn(window.history, "back").mockImplementation(() => { + window.dispatchEvent(new PopStateEvent("popstate", { state: window.history.state })); + }); + const holder: { unmount?: () => void } = {}; + const onClose = vi.fn(() => holder.unmount?.()); + const layer = renderHook(() => useBackToClose(onClose)); + holder.unmount = layer.unmount; + await settle(); + return { back, onClose }; +} + beforeEach(() => { localStorage.clear(); sessionStorage.setItem("siftlode.activeAccount", "1"); // account-scoped page memory @@ -41,46 +59,29 @@ beforeEach(() => { }); // Not automatic: vitest runs without `globals`, so RTL never registers its own afterEach — leaving a // provider (and its popstate listener) mounted for the next case. Same reason Home.dom.test.tsx does. -afterEach(cleanup); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); -describe("NavigationProvider · setPage reports whether it navigated", () => { - it("returns true and pushes an entry for a real page change", () => { +describe("NavigationProvider · setPage's same-page guard", () => { + it("pushes an entry for a real page change", () => { const { result } = render(); const push = vi.spyOn(window.history, "pushState"); - try { - let reported: boolean | undefined; - act(() => { - reported = result.current.setPage("plex"); - }); - expect(reported).toBe(true); - expect(push).toHaveBeenCalledTimes(1); - expect(result.current.page).toBe("plex"); - } finally { - push.mockRestore(); - } + act(() => result.current.setPage("plex")); + expect(push).toHaveBeenCalledTimes(1); + expect(result.current.page).toBe("plex"); }); - it("returns false and pushes nothing when it is already that page", () => { + it("pushes nothing when it is already that page", () => { const { result } = render(); - act(() => { - result.current.setPage("plex"); - }); + act(() => result.current.setPage("plex")); const push = vi.spyOn(window.history, "pushState"); - try { - let reported: boolean | undefined; - act(() => { - reported = result.current.setPage("plex"); - }); - // Both halves matter: `false` is what `buryUnder` reads, and "pushed nothing" is what makes - // that answer true. A value-only assertion would pass against an inverted pair. - expect(reported).toBe(false); - expect(push).not.toHaveBeenCalled(); - } finally { - push.mockRestore(); - } + act(() => result.current.setPage("plex")); + expect(push).not.toHaveBeenCalled(); }); - it("returns true even for the current page when a channel page is over it", () => { + it("still navigates to the current page when a channel page is over it", () => { // The nav rail's own module while a channel page is open over it is a real navigation: it closes // the channel page. The `&& !channelViewRef.current` half of the guard is what makes it one. const { result } = render(); @@ -90,22 +91,64 @@ describe("NavigationProvider · setPage reports whether it navigated", () => { // asking for "feed" was just an ordinary page change and deleting the clause left it green. const current = result.current.page; expect(current).toBe("home"); - act(() => { - result.current.openChannel("UC1", "A channel"); - }); + act(() => result.current.openChannel("UC1", "A channel")); expect(result.current.channelView).not.toBeNull(); const push = vi.spyOn(window.history, "pushState"); - try { - let reported: boolean | undefined; - act(() => { - reported = result.current.setPage(current); - }); - expect(reported).toBe(true); - expect(push).toHaveBeenCalledTimes(1); - expect(result.current.channelView).toBeNull(); - } finally { - push.mockRestore(); - } + act(() => result.current.setPage(current)); + expect(push).toHaveBeenCalledTimes(1); + expect(result.current.channelView).toBeNull(); + }); +}); + +describe("NavigationProvider · never under an open overlay", () => { + it("setPage closes the overlay first and pushes only once its entry is reclaimed", async () => { + const { result } = render(); + const { back, onClose } = await openOverlay(); + const push = vi.spyOn(window.history, "pushState"); + + act(() => result.current.setPage("plex")); + expect(onClose).toHaveBeenCalledTimes(1); + expect(push).not.toHaveBeenCalled(); // not while the player's entry is still current + + await settle(); + await settle(); + expect(back).toHaveBeenCalledTimes(1); + expect(push).toHaveBeenCalledTimes(1); + expect(result.current.page).toBe("plex"); + expect(window.history.state).toMatchObject({ sfPage: "plex" }); + }); + + // Shift+W on Home with the player open (owner decision 2026-09-09): nothing to push, but the player + // closes, and its entry is reclaimed rather than orphaned under a push that never happened. + it("setPage to the current page closes the overlay and pushes nothing", async () => { + const { result } = render(); + const { back, onClose } = await openOverlay(); + const push = vi.spyOn(window.history, "pushState"); + + act(() => result.current.setPage(result.current.page)); + await settle(); + await settle(); + expect(onClose).toHaveBeenCalledTimes(1); + expect(back).toHaveBeenCalledTimes(1); + expect(push).not.toHaveBeenCalled(); + }); + + // The player's channel link and the palette's channel rows. + it("openChannel closes the overlay first and pushes the channel once its entry is reclaimed", async () => { + const { result } = render(); + const { back, onClose } = await openOverlay(); + const push = vi.spyOn(window.history, "pushState"); + + act(() => result.current.openChannel("UC1", "A channel")); + expect(onClose).toHaveBeenCalledTimes(1); + expect(push).not.toHaveBeenCalled(); + + await settle(); + await settle(); + expect(back).toHaveBeenCalledTimes(1); + expect(push).toHaveBeenCalledTimes(1); + expect(result.current.channelView).toEqual({ id: "UC1", name: "A channel" }); + expect(window.history.state).toMatchObject({ _chan: "UC1" }); }); }); diff --git a/frontend/src/components/NavigationProvider.tsx b/frontend/src/components/NavigationProvider.tsx index 2186076f..ec2168e5 100644 --- a/frontend/src/components/NavigationProvider.tsx +++ b/frontend/src/components/NavigationProvider.tsx @@ -14,7 +14,7 @@ import { pageTitleKey } from "../lib/pageMeta"; import { setAccountRaw } from "../lib/storage"; import { useAccountRehydrate } from "../lib/useAccountRehydrate"; import { setNavigator } from "../lib/nav"; -import { nextNavId, notifyNav } from "../lib/history"; +import { closeOverlaysThenNavigate, nextNavId, notifyNav } from "../lib/history"; import { channelViewFrom, loadInitialPage, PAGE_KEY, type ChannelView } from "./navigationInit"; // Owns the app's navigation + browser-history state, lifted out of the App god component so pages @@ -37,10 +37,11 @@ export interface NavigationState { } export interface NavigationActions { - /** Switch modules. Returns whether it actually navigated — it is a no-op when the requested page - * is already current with no channel page over it, and a caller that buries overlay history - * entries under the push has to know that (see `lib/history.navigateOverOverlays`). */ - setPage: (next: Page) => boolean; + /** Switch modules. Closes every open overlay first and pushes only once their history entries are + * back (`lib/history.closeOverlaysThenNavigate`), so over an open player the push lands a moment + * after the call; with nothing open it is synchronous. The requested page being already current + * (with no channel page over it) pushes nothing — but the overlays still close. */ + setPage: (next: Page) => void; openChannel: (id: string, name?: string) => void; closeChannel: () => void; enterYtSearch: (q: string) => void; @@ -53,7 +54,7 @@ const StateContext = createContext({ ytSearch: null, }); const ActionsContext = createContext({ - setPage: () => false, + setPage: () => {}, openChannel: () => {}, closeChannel: () => {}, enterYtSearch: () => {}, @@ -118,53 +119,61 @@ export function NavigationProvider({ children }: { children: ReactNode }) { }); const setPage = useCallback( - (next: Page) => { - // A channel page overlays the content column, so a nav click must close it even when the - // underlying page is unchanged (e.g. "Feed" while a channel opened from the feed). - if (next === pageRef.current && !channelViewRef.current) return false; - markDirty(); - setChannelView(null); // leave any open channel page when navigating via the rail - setPageState(next); - setAccountRaw(PAGE_KEY, next); - // Push an in-app history entry so browser/mouse Back steps through pages instead of leaving the - // app. A fresh { sfPage } (no carried-over _yt/_chan markers) so each page starts at its root, and - // the address bar reflects the page (feed is implicit) while clearing the view-specific params - // (chan/v/pl and the plex content keys pscope/psort/pshow/pf) — a page switch resets to that - // module's root. Feed filters (owned by FeedFiltersProvider) are left intact; the plex/channels - // keys re-mirror from PlexProvider / ChannelsProvider on return to their page. - window.history.pushState( - { sfPage: next, sfScroll: nextNavId() }, - "", - urlWithParams({ - page: next === "feed" ? null : next, - chan: null, - v: null, - pl: null, - pscope: null, - psort: null, - pshow: null, - pf: null, - ctab: null, - csort: null, - cf: null, - }), - ); - notifyNav(); - return true; - }, + (next: Page) => + // Never under an open overlay — see the interface doc. The refs are read when the push + // actually happens, which over an open player is a moment after this call. + closeOverlaysThenNavigate(() => { + // A channel page overlays the content column, so a nav click must close it even when the + // underlying page is unchanged (e.g. "Feed" while a channel opened from the feed). + if (next === pageRef.current && !channelViewRef.current) return; + markDirty(); + setChannelView(null); // leave any open channel page when navigating via the rail + setPageState(next); + setAccountRaw(PAGE_KEY, next); + // Push an in-app history entry so browser/mouse Back steps through pages instead of leaving + // the app. A fresh { sfPage } (no carried-over _yt/_chan markers) so each page starts at its + // root, and the address bar reflects the page (feed is implicit) while clearing the + // view-specific params (chan/v/pl and the plex content keys pscope/psort/pshow/pf) — a page + // switch resets to that module's root. Feed filters (owned by FeedFiltersProvider) are left + // intact; the plex/channels keys re-mirror from PlexProvider / ChannelsProvider on return to + // their page. + window.history.pushState( + { sfPage: next, sfScroll: nextNavId() }, + "", + urlWithParams({ + page: next === "feed" ? null : next, + chan: null, + v: null, + pl: null, + pscope: null, + psort: null, + pshow: null, + pf: null, + ctab: null, + csort: null, + cf: null, + }), + ); + notifyNav(); + }), [markDirty], ); // A sub-view (channel / YT search) is an OVERLAY, not a page change, so it does NOT markDirty: a // pre-pin account re-hydrate should still restore the stored page underneath — the re-hydrate's // replaceState spreads the _chan/_yt markers, so the overlay survives on top of the restored page. + // + // Like setPage it never lands under an open overlay: the player's channel link and the palette's + // channel rows call it from inside one. const openChannel = useCallback((id: string, name?: string) => { - setChannelView({ id, name }); - const st = window.history.state || {}; - const url = urlWithParams({ chan: id }); - if (st._chan) - window.history.replaceState({ ...st, _chan: id, _chanName: name ?? null }, "", url); - else window.history.pushState({ ...st, _chan: id, _chanName: name ?? null }, "", url); + closeOverlaysThenNavigate(() => { + setChannelView({ id, name }); + const st = window.history.state || {}; + const url = urlWithParams({ chan: id }); + if (st._chan) + window.history.replaceState({ ...st, _chan: id, _chanName: name ?? null }, "", url); + else window.history.pushState({ ...st, _chan: id, _chanName: name ?? null }, "", url); + }); }, []); const closeChannel = useCallback(() => { diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 6b71e37f..15ccd35e 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -46,7 +46,7 @@ import { import { renderDescription } from "../lib/descriptionLinks"; import { fullscreenElement } from "../lib/fullscreen"; import { dispatch, isActivationTarget, useKeyScope } from "../lib/keymap"; -import { closeThenNavigate, useBackToClose } from "../lib/history"; +import { useBackToClose } from "../lib/history"; import ChannelNameLink from "./ChannelNameLink"; import Toaster, { useOverlayOrder } from "./ui/Toaster"; import { afterRemoval } from "../lib/playerQueue"; @@ -822,13 +822,11 @@ export default function PlayerModal({ channelId={active.channel_id} title={active.channel_title} onOpenInApp={() => { - const cid = active.channel_id; - const cname = active.channel_title ?? undefined; - // Close first, open the channel only once this player's own Back-to-close - // entry has been reclaimed — otherwise that reclaim undoes the push and - // clicking the name merely closes the player. The rule lives in history.ts - // now; the command palette needs exactly the same one. - closeThenNavigate(onClose, () => openChannel(cid, cname)); + // openChannel closes this player first and pushes only once its + // Back-to-close entry is reclaimed — otherwise that reclaim undoes the push + // and clicking the name merely closes the player + // (lib/history.closeOverlaysThenNavigate). + openChannel(active.channel_id, active.channel_title ?? undefined); }} className="font-medium hover:text-accent truncate text-left" /> diff --git a/frontend/src/components/ui/CommandPalette.dom.test.tsx b/frontend/src/components/ui/CommandPalette.dom.test.tsx index c73afbcb..70bd113b 100644 --- a/frontend/src/components/ui/CommandPalette.dom.test.tsx +++ b/frontend/src/components/ui/CommandPalette.dom.test.tsx @@ -21,7 +21,7 @@ beforeAll(() => { afterEach(cleanup); // `choose` closes and waits for the dialog's own Back-to-close history entry to be reclaimed before -// running the command (see closeThenNavigate), so the command lands a tick later — hence the waits. +// running the command (see closeTopOverlayThenRun), so the command lands a tick later — hence the waits. const ran: string[] = []; const groups = (): CommandGroup[] => [ { diff --git a/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx b/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx index 81cf801e..3191b6d1 100644 --- a/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx +++ b/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx @@ -1,10 +1,9 @@ // @vitest-environment jsdom // // ONE test, ONE file, on purpose. This is the close→reclaim→run ordering contract of the palette -// (see closeThenNavigate), and it ASSERTS on a real useBackToClose entry driven through a real -// programmatic history.back() — which is why it cannot share module state with anything. (It is not -// the only place in the suite where an unstubbed back() fires; history.burial.dom.test.ts is the -// other file that asserts on one, and any test rendering a real Modal can let one go.) +// (see closeTopOverlayThenRun), and it ASSERTS on a real useBackToClose entry driven through a real +// programmatic history.back() — which is why it cannot share module state with anything. (Any test +// rendering a real Modal can let an unstubbed back() go, too.) // // It cannot live in CommandPalette.dom.test.tsx. jsdom gives a test FILE a single session history, // and history.ts's overlay bookkeeping is module-global, so the sibling tests there leave an @@ -50,7 +49,7 @@ function Harness() { ); } -describe("CommandPalette · closeThenNavigate ordering", () => { +describe("CommandPalette · close-then-run ordering", () => { it("closes, and runs the command only once the close has landed", async () => { render(); // Load-bearing: useBackToClose pushes its entry from a coalesced microtask, so a click in the @@ -62,8 +61,9 @@ describe("CommandPalette · closeThenNavigate ordering", () => { fireEvent.click(screen.getByText("TheVR Podcast")); expect(ran).toEqual(["closed"]); - // Inside closeThenNavigate's 250ms no-popstate fallback, so a pass means the RECLAIM ran the - // command — not the safety net that exists for overlays which never got an entry at all. - await waitFor(() => expect(ran).toEqual(["closed", "ch:1"]), { timeout: 200 }); + // Well inside closeTopOverlayThenRun's 1500 ms give-up timer, so a pass means the RECLAIM ran the + // command — not the late path for a back() that never reports. (The margin used to be 200 ms + // under a 250 ms fallback, which made this load-sensitive by construction.) + await waitFor(() => expect(ran).toEqual(["closed", "ch:1"]), { timeout: 1000 }); }); }); diff --git a/frontend/src/components/ui/CommandPalette.tsx b/frontend/src/components/ui/CommandPalette.tsx index d0f00fca..aff1833a 100644 --- a/frontend/src/components/ui/CommandPalette.tsx +++ b/frontend/src/components/ui/CommandPalette.tsx @@ -2,7 +2,7 @@ import { useEffect, useId, useMemo, useRef, useState, type ReactNode } from "rea import { useTranslation } from "react-i18next"; import { Search } from "lucide-react"; import Modal from "./Modal"; -import { closeThenNavigate } from "../../lib/history"; +import { closeTopOverlayThenRun } from "../../lib/history"; // The app's jump-to bar: one text field over every place you can go and every app-wide thing you can // do, opened with Ctrl/Cmd+K. It is the "one-layer operation" half of E10 — reaching a channel three @@ -92,9 +92,11 @@ export default function CommandPalette({ if (!open) return null; - // Close, wait for this dialog's own Back-to-close entry to be reclaimed, THEN run. Most commands - // navigate, and a pushState issued before that reclaim is undone by it — see closeThenNavigate. - const choose = (item: CommandItem) => closeThenNavigate(onClose, item.run); + // Close, wait for this dialog's own Back-to-close entry to be reclaimed, THEN run: most commands + // navigate, and a pushState issued before that reclaim is undone by it. Only THIS dialog closes — + // a command that navigates closes whatever is underneath itself, one that does not (a theme) must + // leave it standing. See closeTopOverlayThenRun. + const choose = (item: CommandItem) => closeTopOverlayThenRun(onClose, item.run); // These live on the input rather than in the shared keymap: they are the field's own editing keys // (like a