Skip to content

Commit 89f911f

Browse files
steipetesteipete
authored
refactor(ui): consolidate sidebar navigation contract (#100612)
* fix(ui): restore sidebar navigation contract * refactor(ui): remove duplicate sidebar navigation model
1 parent aca316e commit 89f911f

4 files changed

Lines changed: 84 additions & 90 deletions

File tree

‎ui/src/app-navigation-groups.test.ts‎

Lines changed: 37 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1,47 +1,55 @@
1-
// Control UI tests cover navigation groups behavior.
1+
// Control UI tests cover sidebar pinned-route customization behavior.
22
import { describe, expect, it } from "vitest";
33
import {
4+
DEFAULT_SIDEBAR_PINNED_ROUTES,
45
SETTINGS_NAVIGATION_ROUTES,
5-
SIDEBAR_SECTIONS,
6+
SIDEBAR_NAV_ROUTES,
67
isSettingsNavigationRoute,
7-
isRouteInSidebarSection,
8+
normalizeSidebarPinnedRoutes,
9+
sidebarMoreRoutes,
810
} from "./app-navigation.ts";
911
import { routeIdFromPath } from "./app-routes.ts";
1012

11-
describe("SIDEBAR_SECTIONS", () => {
12-
it("collapses detailed settings slices into one sidebar entry", () => {
13-
const settings = SIDEBAR_SECTIONS.find((group) => group.label === "settings");
14-
expect(settings?.routes).toEqual(["config"]);
13+
describe("sidebar pinned routes", () => {
14+
it("defaults to a small pinned set drawn from the customizable routes", () => {
15+
expect(DEFAULT_SIDEBAR_PINNED_ROUTES.length).toBeLessThan(SIDEBAR_NAV_ROUTES.length);
16+
for (const routeId of DEFAULT_SIDEBAR_PINNED_ROUTES) {
17+
expect(SIDEBAR_NAV_ROUTES).toContain(routeId);
18+
}
19+
});
20+
21+
it("keeps managed worktrees in the customizable sidebar", () => {
22+
expect(SIDEBAR_NAV_ROUTES).toContain("worktrees");
23+
});
24+
25+
it("keeps channel management and settings slices out of the customizable sidebar", () => {
26+
expect(SIDEBAR_NAV_ROUTES).not.toContain("channels");
27+
expect(SIDEBAR_NAV_ROUTES).not.toContain("config");
28+
expect(SETTINGS_NAVIGATION_ROUTES).toContain("channels");
1529
expect(SETTINGS_NAVIGATION_ROUTES.every((routeId) => isSettingsNavigationRoute(routeId))).toBe(
1630
true,
1731
);
1832
});
1933

20-
it("keeps channel management out of the primary control sidebar", () => {
21-
const control = SIDEBAR_SECTIONS.find((group) => group.label === "control");
22-
expect(control?.routes).toEqual([
23-
"overview",
24-
"activity",
25-
"workboard",
26-
"worktrees",
27-
"instances",
28-
"sessions",
29-
"usage",
30-
"cron",
31-
]);
32-
expect(SETTINGS_NAVIGATION_ROUTES).toContain("channels");
34+
it("normalizes persisted pinned routes, dropping unknown and duplicate entries", () => {
35+
expect(
36+
normalizeSidebarPinnedRoutes(["worktrees", "overview", "worktrees", "bogus", 7]),
37+
).toEqual(["worktrees", "overview"]);
38+
expect(normalizeSidebarPinnedRoutes([])).toEqual([]);
3339
});
3440

35-
it("keeps the settings group active for nested settings routes", () => {
36-
const settings = SIDEBAR_SECTIONS.find((group) => group.label === "settings");
37-
if (!settings) {
38-
throw new Error("Expected settings group");
39-
}
41+
it("falls back to null for non-list values so callers use defaults", () => {
42+
expect(normalizeSidebarPinnedRoutes(undefined)).toBeNull();
43+
expect(normalizeSidebarPinnedRoutes({ overview: true })).toBeNull();
44+
expect(normalizeSidebarPinnedRoutes("overview")).toBeNull();
45+
});
4046

41-
expect(isRouteInSidebarSection(settings, "appearance")).toBe(true);
42-
expect(isRouteInSidebarSection(settings, "channels")).toBe(true);
43-
expect(isRouteInSidebarSection(settings, "debug")).toBe(true);
44-
expect(isRouteInSidebarSection(settings, "chat")).toBe(false);
47+
it("puts every unpinned nav route into the More section", () => {
48+
const pinned = ["overview", "worktrees"] as const;
49+
const more = sidebarMoreRoutes(pinned);
50+
expect(more).not.toContain("overview");
51+
expect(more).not.toContain("worktrees");
52+
expect(new Set([...pinned, ...more])).toEqual(new Set(SIDEBAR_NAV_ROUTES));
4553
});
4654

4755
it("routes every published settings slice", () => {

‎ui/src/app-navigation.test.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ describe("pathForRoute", () => {
174174
it("returns correct path without base", () => {
175175
expect(pathForRoute("chat")).toBe("/chat");
176176
expect(pathForRoute("overview")).toBe("/overview");
177+
expect(pathForRoute("worktrees")).toBe("/worktrees");
177178
});
178179

179180
it("prepends base path", () => {
@@ -187,6 +188,7 @@ describe("routeIdFromPath", () => {
187188
expect(routeIdFromPath("/chat")).toBe("chat");
188189
expect(routeIdFromPath("/overview")).toBe("overview");
189190
expect(routeIdFromPath("/activity")).toBe("activity");
191+
expect(routeIdFromPath("/worktrees")).toBe("worktrees");
190192
expect(routeIdFromPath("/sessions")).toBe("sessions");
191193
expect(routeIdFromPath("/dreaming")).toBe("dreams");
192194
expect(routeIdFromPath("/dreams")).toBe("dreams");

‎ui/src/app-navigation.ts‎

Lines changed: 39 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -5,71 +5,61 @@ import { t } from "./i18n/index.ts";
55

66
export type NavigationRouteId = RouteId;
77

8-
type SidebarSection = {
9-
label: string;
10-
routes: readonly NavigationRouteId[];
11-
};
12-
138
type NavigationItem = {
149
[TRouteId in NavigationRouteId]: IconName;
1510
};
1611

17-
export const SIDEBAR_SECTIONS = [
18-
{ label: "chat", routes: ["chat"] },
19-
{
20-
label: "control",
21-
routes: [
22-
"overview",
23-
"activity",
24-
"workboard",
25-
"worktrees",
26-
"instances",
27-
"sessions",
28-
"usage",
29-
"cron",
30-
],
31-
},
32-
{ label: "agent", routes: ["agents", "skills", "skill-workshop", "nodes", "dreams"] },
33-
{ label: "settings", routes: ["config"] },
34-
] as const satisfies readonly SidebarSection[];
35-
36-
type SidebarSectionRouteId = (typeof SIDEBAR_SECTIONS)[number]["routes"][number];
37-
38-
export type SidebarNavRoute = Exclude<SidebarSectionRouteId, "chat" | "config">;
12+
// The sidebar shows a small user-customizable pinned set; every other nav route
13+
// lives in the collapsed "More" section. Chat is reachable through the session
14+
// list and Settings/Docs live in the sidebar footer, so neither is listed here.
15+
export const SIDEBAR_NAV_ROUTES = [
16+
"overview",
17+
"activity",
18+
"workboard",
19+
"worktrees",
20+
"instances",
21+
"sessions",
22+
"usage",
23+
"cron",
24+
"agents",
25+
"skills",
26+
"skill-workshop",
27+
"nodes",
28+
"dreams",
29+
] as const satisfies readonly NavigationRouteId[];
3930

40-
export const SIDEBAR_NAV_ROUTES = SIDEBAR_SECTIONS.flatMap((section) =>
41-
section.label === "control" || section.label === "agent" ? section.routes : [],
42-
) as readonly SidebarNavRoute[];
31+
export type SidebarNavRoute = (typeof SIDEBAR_NAV_ROUTES)[number];
4332

33+
// Sessions are the sidebar's core content; Overview is the only page pinned by
34+
// default. Users pin more via the customize menu.
4435
export const DEFAULT_SIDEBAR_PINNED_ROUTES = [
4536
"overview",
4637
] as const satisfies readonly SidebarNavRoute[];
4738

48-
const SIDEBAR_NAV_ROUTE_SET = new Set<NavigationRouteId>(SIDEBAR_NAV_ROUTES);
49-
50-
function isSidebarNavRoute(value: unknown): value is SidebarNavRoute {
51-
return typeof value === "string" && SIDEBAR_NAV_ROUTE_SET.has(value as NavigationRouteId);
52-
}
53-
54-
export function normalizeSidebarPinnedRoutes(value: unknown): SidebarNavRoute[] | undefined {
39+
/**
40+
* Normalize a persisted pinned-route list. Returns null when the value is not a
41+
* list (caller falls back to defaults); unknown or duplicate entries are dropped
42+
* so prefs survive route renames/removals without a migration.
43+
*/
44+
export function normalizeSidebarPinnedRoutes(value: unknown): SidebarNavRoute[] | null {
5545
if (!Array.isArray(value)) {
56-
return undefined;
46+
return null;
5747
}
58-
const seen = new Set<SidebarNavRoute>();
59-
const routes: SidebarNavRoute[] = [];
60-
for (const routeId of value) {
61-
if (!isSidebarNavRoute(routeId) || seen.has(routeId)) {
62-
continue;
48+
const pinned: SidebarNavRoute[] = [];
49+
for (const entry of value) {
50+
if (
51+
typeof entry === "string" &&
52+
(SIDEBAR_NAV_ROUTES as readonly string[]).includes(entry) &&
53+
!pinned.includes(entry as SidebarNavRoute)
54+
) {
55+
pinned.push(entry as SidebarNavRoute);
6356
}
64-
seen.add(routeId);
65-
routes.push(routeId);
6657
}
67-
return routes;
58+
return pinned;
6859
}
6960

70-
export function sidebarMoreRoutes(pinnedRoutes: readonly SidebarNavRoute[]): SidebarNavRoute[] {
71-
const pinned = new Set(pinnedRoutes);
72-
return SIDEBAR_NAV_ROUTES.filter((routeId) => !pinned.has(routeId));
61+
export function sidebarMoreRoutes(pinned: readonly SidebarNavRoute[]): SidebarNavRoute[] {
62+
return SIDEBAR_NAV_ROUTES.filter((routeId) => !pinned.includes(routeId));
7363
}
7464

7565
export const SETTINGS_NAVIGATION_ROUTES = [
@@ -117,16 +107,6 @@ export function isSettingsNavigationRoute(routeId: NavigationRouteId): boolean {
117107
return (SETTINGS_NAVIGATION_ROUTES as readonly NavigationRouteId[]).includes(routeId);
118108
}
119109

120-
export function isRouteInSidebarSection(
121-
section: SidebarSection,
122-
routeId: NavigationRouteId,
123-
): boolean {
124-
if (section.label === "settings") {
125-
return isSettingsNavigationRoute(routeId);
126-
}
127-
return section.routes.includes(routeId);
128-
}
129-
130110
export function navigationIconForRoute(routeId: NavigationRouteId): IconName {
131111
return NAVIGATION_ICONS[routeId] ?? "folder";
132112
}

‎ui/src/e2e/sidebar-customization.e2e.test.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,15 +98,19 @@ describeControlUiE2e("Control UI sidebar customization mocked Gateway E2E", () =
9898
const menu = sidebar.getByRole("menu", { name: "Customize sidebar" });
9999
const overviewItem = menu.getByRole("menuitemcheckbox", { name: "Overview" });
100100
await expect.poll(() => overviewItem.getAttribute("aria-checked")).toBe("true");
101+
const worktreesItem = menu.getByRole("menuitemcheckbox", { name: "Worktrees" });
102+
await expect.poll(() => worktreesItem.getAttribute("aria-checked")).toBe("false");
101103
await expect
102104
.poll(() => overviewItem.evaluate((element) => element === document.activeElement))
103105
.toBe(true);
104106
await captureUiProof(page, "02-customize-menu.png");
105107

108+
await worktreesItem.click();
109+
await expect.poll(() => trimmedTextContents(pinnedItems)).toEqual(["Overview", "Worktrees"]);
106110
await overviewItem.click();
107-
await expect.poll(() => trimmedTextContents(pinnedItems)).toEqual([]);
111+
await expect.poll(() => trimmedTextContents(pinnedItems)).toEqual(["Worktrees"]);
108112
await page.reload();
109-
await expect.poll(() => trimmedTextContents(pinnedItems)).toEqual([]);
113+
await expect.poll(() => trimmedTextContents(pinnedItems)).toEqual(["Worktrees"]);
110114
await expect.poll(() => moreButton.getAttribute("aria-expanded")).toBe("true");
111115
await expect
112116
.poll(() =>

0 commit comments

Comments
 (0)