diff --git a/products/desktop/packages/ui/src/features/browser-tabs/AGENTS.md b/products/desktop/packages/ui/src/features/browser-tabs/AGENTS.md index 0728e728bfa0..c272b2b26bbe 100644 --- a/products/desktop/packages/ui/src/features/browser-tabs/AGENTS.md +++ b/products/desktop/packages/ui/src/features/browser-tabs/AGENTS.md @@ -187,6 +187,7 @@ Each tab carries the nav state its href cannot express, in `viewState`: A rail click navigates the active tab back to its own remembered href rather than to the destination's root. Per tab on purpose: a window-global memory would let one tab's rail click restore an href another tab established. + Record and replay both go through `isRestorableVisitHref` (canvas `railPane.ts`), so a settings or redirect-alias href is never remembered and a stale stored one is never replayed. `BrowserTabStrip`'s navigation effect is the **single writer for settled router navigation**. It runs on every settled navigation, including the ones a rail diff --git a/products/desktop/packages/ui/src/features/browser-tabs/BrowserTabStrip.tsx b/products/desktop/packages/ui/src/features/browser-tabs/BrowserTabStrip.tsx index 5fcd828f7db0..8763bce5cdf1 100644 --- a/products/desktop/packages/ui/src/features/browser-tabs/BrowserTabStrip.tsx +++ b/products/desktop/packages/ui/src/features/browser-tabs/BrowserTabStrip.tsx @@ -25,6 +25,7 @@ import { } from "@posthog/ui/features/canvas/hooks/useDashboards"; import { useProjectTaskFeeds } from "@posthog/ui/features/canvas/hooks/useProjectTaskFeeds"; import { useRailPane } from "@posthog/ui/features/canvas/hooks/useRailSurface"; +import { isRestorableVisitHref } from "@posthog/ui/features/canvas/railPane"; import { activityReportIdFromHref, useActivitySelection, @@ -400,15 +401,9 @@ function BrowserTabStripImpl() { title: routeTitle ?? mirrorActive?.viewState?.title, listOpen, spaceId: stampedSpaceId, - // Settings is a full-window overlay that classifies as the spaces pane, so - // recording its href here would overwrite the tab's real last spaces - // location and a later Spaces rail click would reopen Settings. Keep the - // existing map on the settings route, as the strip did before settings - // stayed mounted. - lastByPane: - routeAppView === "settings" - ? previousLastByPane - : { ...previousLastByPane, [railPane]: visit }, + lastByPane: isRestorableVisitHref(railPane, locationHref) + ? { ...previousLastByPane, [railPane]: visit } + : previousLastByPane, }; const decision = decideTabNavigation({ // The SETTLED tag, not the in-flight one. Pairing the in-flight tag with diff --git a/products/desktop/packages/ui/src/features/canvas/AGENTS.md b/products/desktop/packages/ui/src/features/canvas/AGENTS.md index 7990eed2033e..80a277e8f158 100644 --- a/products/desktop/packages/ui/src/features/canvas/AGENTS.md +++ b/products/desktop/packages/ui/src/features/canvas/AGENTS.md @@ -53,6 +53,7 @@ changing breadcrumbs, canvas naming, or the canvas generation harness. The root Anything a destination does besides navigating must live in its route component, not its `onPick`: the restore path navigates by href and never reaches the navigation bridge. + Both ends share `isRestorableVisitHref` (`railPane.ts`): the writer never records an href a click may not restore (settings, folder settings, redirect aliases) and the restore path re-checks the stored one, so bad persisted state falls through to the destination's root. - **Testing flag-off locally:** dev builds default `project-bluebird` and `code-spaces-layout` on, and that default beats posthog's own override. Force them off with diff --git a/products/desktop/packages/ui/src/features/canvas/components/NavRail.test.tsx b/products/desktop/packages/ui/src/features/canvas/components/NavRail.test.tsx index 7fa10452d254..c1cd3b948e09 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/NavRail.test.tsx +++ b/products/desktop/packages/ui/src/features/canvas/components/NavRail.test.tsx @@ -229,6 +229,17 @@ describe("NavRail", () => { expect(mocks.openBrowserTab).not.toHaveBeenCalled(); }); + it("navigates a Spaces click away from a page that is no destination", async () => { + const user = userEvent.setup(); + mocks.fullPath = "/folders/$folderId"; + mocks.href = "/folders/folder-1"; + render(); + + await user.click(screen.getByLabelText("Spaces")); + + expect(mocks.navigateToSpaces).toHaveBeenCalledOnce(); + }); + it("routes to Activity from a screen that has no column for it", async () => { const user = userEvent.setup(); mocks.fullPath = "/inbox"; @@ -320,6 +331,20 @@ describe("NavRail", () => { expect(useChannelPaneStore.getState().pane).toBe("list"); }); + it("ignores a remembered visit that is not a Spaces page", async () => { + const user = userEvent.setup(); + mocks.fullPath = "/activity"; + rememberVisits({ + spaces: { href: "/settings/general", listOpen: false }, + }); + render(); + + await user.click(screen.getByLabelText("Spaces")); + + expect(mocks.navigate).not.toHaveBeenCalled(); + expect(mocks.navigateToSpaces).toHaveBeenCalledOnce(); + }); + it("returns to the space pane when the list was not open", async () => { const user = userEvent.setup(); useChannelPaneStore.setState({ pane: "list" }); diff --git a/products/desktop/packages/ui/src/features/canvas/components/railDestinations.ts b/products/desktop/packages/ui/src/features/canvas/components/railDestinations.ts index 8b01e6f7c3e8..684dd5402d1a 100644 --- a/products/desktop/packages/ui/src/features/canvas/components/railDestinations.ts +++ b/products/desktop/packages/ui/src/features/canvas/components/railDestinations.ts @@ -12,7 +12,10 @@ import type { RailVisit } from "@posthog/shared"; import type { SidebarNavItem } from "@posthog/shared/analytics-events"; import { readMirror } from "@posthog/ui/features/browser-tabs/tabsSync"; import { SpacesIcon } from "@posthog/ui/features/canvas/components/SpacesIcon"; -import type { NavRailPane } from "@posthog/ui/features/canvas/railPane"; +import { + isRestorableVisitHref, + type NavRailPane, +} from "@posthog/ui/features/canvas/railPane"; import { applyTabViewState, showChannelList, @@ -25,6 +28,7 @@ import { import type { CountBadgeTone } from "@posthog/ui/primitives/CountBadge"; import { LoopIcon } from "@posthog/ui/primitives/LoopIcon"; import { + getCurrentMatches, navigateToActivity, navigateToCanvases, navigateToChannel, @@ -130,7 +134,12 @@ export function pickRailDestination( destination: RailDestination, current: NavRailPane, ): void { - if (destination.pane === current) { + const matches = getCurrentMatches(); + const routePath = matches[matches.length - 1]?.fullPath ?? ""; + const onDestination = + destination.pane === current && + isRestorableVisitHref(destination.pane, routePath); + if (onDestination) { (destination.onReclick ?? destination.onPick)(); return; } @@ -138,7 +147,11 @@ export function pickRailDestination( // A remembered visit that IS where we already are restores nothing, and the // click would look dead. Fall through to the destination's root instead, so // a pick always goes somewhere. - if (visit && visit.href !== currentHref()) restoreVisit(visit); + const restorable = + visit && + visit.href !== currentHref() && + isRestorableVisitHref(destination.pane, visit.href); + if (restorable) restoreVisit(visit); else destination.onPick(); } diff --git a/products/desktop/packages/ui/src/features/canvas/railPane.test.ts b/products/desktop/packages/ui/src/features/canvas/railPane.test.ts index 26a161bf1f80..000c3c0b8764 100644 --- a/products/desktop/packages/ui/src/features/canvas/railPane.test.ts +++ b/products/desktop/packages/ui/src/features/canvas/railPane.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { + isRestorableVisitHref, RAIL_PANE_ROOT, railPaneForPath, railPaneHasSidebar, @@ -46,6 +47,34 @@ describe("railPaneForPath", () => { }); }); +describe("isRestorableVisitHref", () => { + it.each([ + ["spaces", "/spaces/chan-1/tasks/task-1"], + ["spaces", "/tasks/task-1"], + ["spaces", "/new"], + ["activity", "/activity?task=task-1"], + ["inbox", "/inbox/pulls/report-1"], + ["home", "/"], + ] as const)("lets %s replay %s", (pane, href) => { + expect(isRestorableVisitHref(pane, href)).toBe(true); + }); + + it.each([ + ["spaces", "/settings"], + ["spaces", "/settings/general"], + ["spaces", "/settings/general?from=rail"], + ["spaces", "/folders/folder-1"], + ["spaces", "/skills"], + ["spaces", "/mcp-servers"], + ["spaces", "/usage"], + ["inbox", "/inbox/agents"], + ["spaces", "/activity"], + ["activity", "/spaces/chan-1"], + ] as const)("does not let %s replay %s", (pane, href) => { + expect(isRestorableVisitHref(pane, href)).toBe(false); + }); +}); + describe("railPaneHasSidebar", () => { it.each(["home", "inbox", "command-center", "loops"] as const)( "gives %s the whole screen", diff --git a/products/desktop/packages/ui/src/features/canvas/railPane.ts b/products/desktop/packages/ui/src/features/canvas/railPane.ts index 4f93f9371928..151cdc9260ad 100644 --- a/products/desktop/packages/ui/src/features/canvas/railPane.ts +++ b/products/desktop/packages/ui/src/features/canvas/railPane.ts @@ -75,6 +75,27 @@ export function getRailPane(): NavRailPane { return railPaneForMatches(getCurrentMatches()); } +const NON_RESTORABLE_ROOTS = [ + "/settings", + "/folders", + "/skills", + "/mcp-servers", + "/usage", + "/inbox/agents", +]; + +export function isRestorableVisitHref( + pane: NavRailPane, + href: string, +): boolean { + const path = href.replace(/[?#].*$/, ""); + const blocked = NON_RESTORABLE_ROOTS.some( + (root) => path === root || path.startsWith(`${root}/`), + ); + if (blocked) return false; + return railPaneForPath(path) === pane; +} + const PANES_WITH_SIDEBAR = new Set([ "spaces", "activity", diff --git a/products/desktop/packages/ui/src/router/routes/inbox/agents.tsx b/products/desktop/packages/ui/src/router/routes/inbox/agents.tsx index e421cb955b52..2c67b9bc83f2 100644 --- a/products/desktop/packages/ui/src/router/routes/inbox/agents.tsx +++ b/products/desktop/packages/ui/src/router/routes/inbox/agents.tsx @@ -1,9 +1,7 @@ -import { createFileRoute, Navigate } from "@tanstack/react-router"; +import { createFileRoute, redirect } from "@tanstack/react-router"; export const Route = createFileRoute("/inbox/agents")({ - component: InboxAgentsRedirect, + beforeLoad: () => { + throw redirect({ to: "/agents", replace: true }); + }, }); - -function InboxAgentsRedirect() { - return ; -}