Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions products/desktop/packages/ui/src/features/canvas/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(<NavRail />);

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";
Expand Down Expand Up @@ -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(<NavRail />);

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" });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -130,15 +134,24 @@ 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;
}
const visit = lastVisitForActiveTab(destination.pane);
// 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();
}

Expand Down
29 changes: 29 additions & 0 deletions products/desktop/packages/ui/src/features/canvas/railPane.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { describe, expect, it } from "vitest";
import {
isRestorableVisitHref,
RAIL_PANE_ROOT,
railPaneForPath,
railPaneHasSidebar,
Expand Down Expand Up @@ -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",
Expand Down
21 changes: 21 additions & 0 deletions products/desktop/packages/ui/src/features/canvas/railPane.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<NavRailPane>([
"spaces",
"activity",
Expand Down
10 changes: 4 additions & 6 deletions products/desktop/packages/ui/src/router/routes/inbox/agents.tsx
Original file line number Diff line number Diff line change
@@ -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 <Navigate to="/agents" replace />;
}
Loading