fix(web): phone navigation for plugin pages (#2093)
Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -19,6 +19,7 @@ import {
|
||||
} from "@tabler/icons-react";
|
||||
import { AppSidebarWorkspacePicker } from "@/components/app-sidebar/app-sidebar-workspace-picker";
|
||||
import { MobileIntegrationsSection } from "@/components/integrations/integrations-menu";
|
||||
import { MobilePluginNavSection } from "@/components/plugins/mobile-plugin-nav-section";
|
||||
import { TaskSearchInput } from "./task-search-input";
|
||||
import {
|
||||
MobileTasksListOptions,
|
||||
@@ -463,6 +464,7 @@ function MobileMenuContent({
|
||||
showPipeline={showPipeline}
|
||||
/>
|
||||
<MobileDisplayOptions {...displayOptions} />
|
||||
<MobilePluginNavSection onNavigate={() => onOpenChange(false)} />
|
||||
<MobileIntegrationsSection onNavigate={() => onOpenChange(false)} />
|
||||
<MobileUtilityActions
|
||||
showHealthIndicator={showHealthIndicator}
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
import { cleanup, render, screen } from "@testing-library/react";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { PageTopbar } from "./page-topbar";
|
||||
|
||||
vi.mock("@/components/app-status-bar/app-status-surface-provider", () => ({
|
||||
AppStatusDrawerTrigger: () => null,
|
||||
}));
|
||||
|
||||
const PHONE_HOME = "topbar-phone-home";
|
||||
|
||||
describe("PageTopbar phone home crumb", () => {
|
||||
afterEach(cleanup);
|
||||
|
||||
it("renders a phone-only home crumb for a root-level page with no nav of its own", () => {
|
||||
render(<PageTopbar title="Hello E2E" />);
|
||||
|
||||
const home = screen.getByTestId(PHONE_HOME);
|
||||
expect(home).not.toBeNull();
|
||||
// Hidden from md up, where the always-visible AppSidebar owns Home.
|
||||
expect(home.className).toContain("md:hidden");
|
||||
expect(home.querySelector("a")?.getAttribute("href")).toBe("/");
|
||||
});
|
||||
|
||||
it("omits it when the page supplies its own leading nav", () => {
|
||||
render(<PageTopbar title="Plugins" leading={<button type="button">Open menu</button>} />);
|
||||
|
||||
expect(screen.queryByTestId(PHONE_HOME)).toBeNull();
|
||||
});
|
||||
|
||||
it("omits it when the page already shows a real back link", () => {
|
||||
render(<PageTopbar title="Agent" backHref="/settings/agents" backLabel="Agents" />);
|
||||
|
||||
expect(screen.queryByTestId(PHONE_HOME)).toBeNull();
|
||||
expect(screen.getByText("Agents")).not.toBeNull();
|
||||
});
|
||||
|
||||
it("omits it for the root variant, which renders a plain label instead of a breadcrumb", () => {
|
||||
render(<PageTopbar title="Home" variant="root" backLabel="Kandev" />);
|
||||
|
||||
expect(screen.queryByTestId(PHONE_HOME)).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -73,6 +73,7 @@ function TopbarBreadcrumb({
|
||||
title,
|
||||
subtitle,
|
||||
icon,
|
||||
showPhoneHome,
|
||||
}: {
|
||||
backHref: string;
|
||||
backLabel: string;
|
||||
@@ -80,11 +81,16 @@ function TopbarBreadcrumb({
|
||||
title: string;
|
||||
subtitle?: string;
|
||||
icon?: ReactNode;
|
||||
showPhoneHome: boolean;
|
||||
}) {
|
||||
// The Home prefix is redundant now that the AppSidebar always shows a Home
|
||||
// nav item. Only render the back link when a page sets a non-root backHref
|
||||
// (e.g. a true ancestor route within a section).
|
||||
// The Home prefix is redundant on desktop, where the AppSidebar always shows
|
||||
// a Home nav item. Only render the back link when a page sets a non-root
|
||||
// backHref (e.g. a true ancestor route within a section).
|
||||
const showBackLink = backHref !== "/" && !!backLabel;
|
||||
// Below md the sidebar is hidden, so a root-level page that brings no nav of
|
||||
// its own (a plugin route with default chrome) would otherwise strand phone
|
||||
// users with no way back. Keep the home crumb phone-only.
|
||||
const showHomeCrumb = !showBackLink && showPhoneHome;
|
||||
return (
|
||||
<Breadcrumb className="relative z-10 min-w-0">
|
||||
<BreadcrumbList className="flex-nowrap text-sm">
|
||||
@@ -98,6 +104,16 @@ function TopbarBreadcrumb({
|
||||
<BreadcrumbSeparator className="shrink-0" />
|
||||
</>
|
||||
)}
|
||||
{showHomeCrumb && (
|
||||
<>
|
||||
<BreadcrumbItem className="shrink-0 md:hidden" data-testid="topbar-phone-home">
|
||||
<BreadcrumbLink asChild>
|
||||
<BackLink href="/" label="Home" />
|
||||
</BreadcrumbLink>
|
||||
</BreadcrumbItem>
|
||||
<BreadcrumbSeparator className="shrink-0 md:hidden" />
|
||||
</>
|
||||
)}
|
||||
{parents?.flatMap((p) => [
|
||||
<BreadcrumbItem key={`${p.href}-item`} className="shrink-0">
|
||||
<BreadcrumbLink asChild>
|
||||
@@ -138,6 +154,7 @@ type TopbarLeadingProps = {
|
||||
title: string;
|
||||
subtitle?: string;
|
||||
icon?: ReactNode;
|
||||
showPhoneHome: boolean;
|
||||
};
|
||||
|
||||
function TopbarLeading({
|
||||
@@ -148,6 +165,7 @@ function TopbarLeading({
|
||||
title,
|
||||
subtitle,
|
||||
icon,
|
||||
showPhoneHome,
|
||||
}: TopbarLeadingProps) {
|
||||
if (variant === "root") {
|
||||
if (!backLabel) return null;
|
||||
@@ -165,6 +183,7 @@ function TopbarLeading({
|
||||
title={title}
|
||||
subtitle={subtitle}
|
||||
icon={icon}
|
||||
showPhoneHome={showPhoneHome}
|
||||
/>
|
||||
);
|
||||
}
|
||||
@@ -206,6 +225,9 @@ export const PageTopbar = forwardRef<HTMLElement, PageTopbarProps>(function Page
|
||||
title={title}
|
||||
subtitle={subtitle}
|
||||
icon={icon}
|
||||
// A page that supplies its own `leading` nav (Settings' menu sheet)
|
||||
// already gets phone users home; don't add a second affordance.
|
||||
showPhoneHome={!leading}
|
||||
/>
|
||||
{leftActions && (
|
||||
<div className="relative z-10 flex shrink-0 items-center gap-1 [&:empty]:hidden">
|
||||
|
||||
@@ -0,0 +1,106 @@
|
||||
import { cleanup, render, screen } from "@testing-library/react";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { pluginRegistry } from "@/lib/plugins/registry";
|
||||
import { MobilePluginNavSection } from "./mobile-plugin-nav-section";
|
||||
|
||||
const HELLO_PATH = "/plugins/hello";
|
||||
|
||||
vi.mock("@/lib/routing/client-router", () => ({
|
||||
usePathname: () => "/",
|
||||
}));
|
||||
|
||||
function renderSection(onNavigate = () => {}) {
|
||||
return render(<MobilePluginNavSection onNavigate={onNavigate} />);
|
||||
}
|
||||
|
||||
describe("MobilePluginNavSection", () => {
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
["plugin-a", "plugin-b"].forEach((id) => pluginRegistry.unregisterPlugin(id));
|
||||
window.history.pushState({}, "", "/");
|
||||
});
|
||||
|
||||
it("renders nothing when no plugin has registered a nav item", () => {
|
||||
const { container } = renderSection();
|
||||
expect(container.innerHTML).toBe("");
|
||||
});
|
||||
|
||||
it("renders main-section nav items so plugin pages are reachable on a phone", () => {
|
||||
pluginRegistry
|
||||
.forPlugin("plugin-a")
|
||||
.registerNavItem({ id: "hello", label: "Hello", path: HELLO_PATH });
|
||||
|
||||
renderSection();
|
||||
|
||||
expect(screen.getByTestId("mobile-plugin-nav-section")).not.toBeNull();
|
||||
expect(screen.getByTestId("mobile-plugin-nav-item-hello")).not.toBeNull();
|
||||
expect(screen.getByText("Hello")).not.toBeNull();
|
||||
});
|
||||
|
||||
it("treats an omitted section as main, matching the desktop sidebar", () => {
|
||||
pluginRegistry
|
||||
.forPlugin("plugin-a")
|
||||
.registerNavItem({ id: "implicit", label: "Implicit", path: "/plugins/implicit" });
|
||||
pluginRegistry.forPlugin("plugin-b").registerNavItem({
|
||||
id: "explicit",
|
||||
label: "Explicit",
|
||||
path: "/plugins/explicit",
|
||||
section: "main",
|
||||
});
|
||||
|
||||
renderSection();
|
||||
|
||||
expect(screen.getByTestId("mobile-plugin-nav-item-implicit")).not.toBeNull();
|
||||
expect(screen.getByTestId("mobile-plugin-nav-item-explicit")).not.toBeNull();
|
||||
});
|
||||
|
||||
it("omits non-main sections: integrations belongs to MobileIntegrationsSection, and settings is rendered nowhere", () => {
|
||||
pluginRegistry.forPlugin("plugin-a").registerNavItem({
|
||||
id: "tracker",
|
||||
label: "Tracker",
|
||||
path: "/plugins/tracker",
|
||||
section: "integrations",
|
||||
});
|
||||
pluginRegistry.forPlugin("plugin-b").registerNavItem({
|
||||
id: "prefs",
|
||||
label: "Prefs",
|
||||
path: "/settings/plugins/plugin-b",
|
||||
section: "settings",
|
||||
});
|
||||
|
||||
const { container } = renderSection();
|
||||
|
||||
expect(container.innerHTML).toBe("");
|
||||
});
|
||||
|
||||
it("renders the named curated icon, falling back to the puzzle glyph", () => {
|
||||
pluginRegistry
|
||||
.forPlugin("plugin-a")
|
||||
.registerNavItem({ id: "hello", label: "Hello", path: HELLO_PATH, icon: "ticket" });
|
||||
pluginRegistry
|
||||
.forPlugin("plugin-b")
|
||||
.registerNavItem({ id: "other", label: "Other", path: "/plugins/other", icon: "nope" });
|
||||
|
||||
renderSection();
|
||||
|
||||
expect(
|
||||
screen.getByTestId("mobile-plugin-nav-item-hello").querySelector("svg.tabler-icon-ticket"),
|
||||
).not.toBeNull();
|
||||
expect(
|
||||
screen.getByTestId("mobile-plugin-nav-item-other").querySelector("svg.tabler-icon-puzzle"),
|
||||
).not.toBeNull();
|
||||
});
|
||||
|
||||
it("navigates to item.path and closes the menu when tapped", () => {
|
||||
const onNavigate = vi.fn();
|
||||
pluginRegistry
|
||||
.forPlugin("plugin-a")
|
||||
.registerNavItem({ id: "hello", label: "Hello", path: HELLO_PATH });
|
||||
|
||||
renderSection(onNavigate);
|
||||
screen.getByTestId("mobile-plugin-nav-item-hello").click();
|
||||
|
||||
expect(window.location.pathname).toBe(HELLO_PATH);
|
||||
expect(onNavigate).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,52 @@
|
||||
"use client";
|
||||
|
||||
import { Button } from "@kandev/ui/button";
|
||||
import Link from "@/components/routing/app-link";
|
||||
import { resolvePluginIcon } from "@/lib/plugins/icons";
|
||||
import { usePluginRegistry } from "@/lib/plugins/registry";
|
||||
|
||||
type MobilePluginNavSectionProps = {
|
||||
/** Closes the surrounding menu sheet once a plugin page is opened. */
|
||||
onNavigate: () => void;
|
||||
};
|
||||
|
||||
/**
|
||||
* Mobile counterpart to the desktop sidebar's `<PluginNavItems/>`: the
|
||||
* hamburger-sheet surface for plugin-registered "main"-section nav items
|
||||
* (`registerNavItem({ section: "main" })`, the default). The desktop rail is
|
||||
* `hidden md:block`, so without this section a plugin's own page has no phone
|
||||
* entry point at all. "integrations"-section items keep rendering inside
|
||||
* `MobileIntegrationsSection`.
|
||||
*/
|
||||
export function MobilePluginNavSection({ onNavigate }: MobilePluginNavSectionProps) {
|
||||
const registry = usePluginRegistry();
|
||||
const items = registry.getNavItems().filter((item) => (item.section ?? "main") === "main");
|
||||
|
||||
if (items.length === 0) return null;
|
||||
|
||||
return (
|
||||
<div className="space-y-3" data-testid="mobile-plugin-nav-section">
|
||||
<div className="text-sm font-medium">Plugins</div>
|
||||
{items.map((item) => {
|
||||
const Icon = resolvePluginIcon(item.icon);
|
||||
return (
|
||||
<Button
|
||||
key={item.id}
|
||||
asChild
|
||||
variant="outline"
|
||||
className="h-11 w-full cursor-pointer justify-start gap-2"
|
||||
>
|
||||
<Link
|
||||
href={item.path}
|
||||
onClick={onNavigate}
|
||||
data-testid={`mobile-plugin-nav-item-${item.id}`}
|
||||
>
|
||||
<Icon className="h-4 w-4 shrink-0" />
|
||||
<span className="flex-1 truncate text-left">{item.label}</span>
|
||||
</Link>
|
||||
</Button>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -0,0 +1,115 @@
|
||||
/**
|
||||
* E2E: a plugin's own page is reachable from a phone.
|
||||
*
|
||||
* The desktop app sidebar (which renders `<PluginNavItems/>`) is
|
||||
* `hidden md:block`, so before `MobilePluginNavSection` existed a
|
||||
* `registerNavItem({ section: "main" })` entry had no phone entry point at
|
||||
* all — the route worked, but only if you typed the URL. Settings > Plugins
|
||||
* was always reachable through the settings menu sheet; this covers the
|
||||
* plugin's *own* nav item.
|
||||
*/
|
||||
import path from "node:path";
|
||||
import type { Browser, BrowserContext, Page } from "@playwright/test";
|
||||
import { expect, test } from "../../fixtures/test-base";
|
||||
import { PrAssetCapture } from "../../helpers/pr-asset-capture";
|
||||
|
||||
const PLUGIN_ID = "kandev-plugin-e2e";
|
||||
const NAV_ITEM_ID = "e2e-hello";
|
||||
const DESKTOP_VIEWPORT = { width: 1440, height: 900 };
|
||||
const PACKAGE_PATH = path.resolve(
|
||||
__dirname,
|
||||
"../../../../../apps/backend/.build/kandev-plugin-e2e-1.0.0.tar.gz",
|
||||
);
|
||||
|
||||
/** A desktop-sized client against the same backend, for the parity screenshot. */
|
||||
async function openDesktopClient(
|
||||
browser: Browser,
|
||||
frontendUrl: string,
|
||||
backendPort: number,
|
||||
): Promise<{ page: Page; context: BrowserContext }> {
|
||||
const context = await browser.newContext({
|
||||
baseURL: frontendUrl,
|
||||
viewport: DESKTOP_VIEWPORT,
|
||||
});
|
||||
const page = await context.newPage();
|
||||
await page.addInitScript(
|
||||
({ port }: { port: number }) => {
|
||||
localStorage.setItem("kandev.onboarding.completed", "true");
|
||||
window.__KANDEV_API_PORT = String(port);
|
||||
},
|
||||
{ port: backendPort },
|
||||
);
|
||||
return { page, context };
|
||||
}
|
||||
|
||||
test.describe("Mobile plugin navigation", () => {
|
||||
test.afterEach(async ({ apiClient }) => {
|
||||
await apiClient.rawRequest("DELETE", `/api/plugins/${PLUGIN_ID}`).catch(() => undefined);
|
||||
});
|
||||
|
||||
test("opens a plugin page from the phone menu sheet", async ({
|
||||
testPage,
|
||||
backend,
|
||||
browser,
|
||||
}, testInfo) => {
|
||||
test.setTimeout(120_000);
|
||||
const capture = new PrAssetCapture(testPage, testInfo.file);
|
||||
|
||||
// Settings > Plugins is reachable on a phone through the settings menu
|
||||
// sheet — install the fixture through that same real flow.
|
||||
await testPage.goto("/settings/plugins");
|
||||
await testPage.getByTestId("install-plugin-trigger").click();
|
||||
await testPage.getByTestId("install-plugin-tab-upload").click();
|
||||
await testPage.getByTestId("install-plugin-file-input").setInputFiles(PACKAGE_PATH);
|
||||
await testPage.getByTestId("install-plugin-upload-submit").click();
|
||||
await expect(testPage.getByTestId(`plugin-row-${PLUGIN_ID}`)).toBeVisible({ timeout: 30_000 });
|
||||
|
||||
await testPage.goto("/");
|
||||
await testPage.reload();
|
||||
|
||||
// The desktop rail carries the same item but is hidden on a phone.
|
||||
await expect(testPage.getByTestId("app-sidebar")).toBeHidden();
|
||||
|
||||
await testPage.getByRole("button", { name: "Open menu" }).click();
|
||||
const navItem = testPage.getByTestId(`mobile-plugin-nav-item-${NAV_ITEM_ID}`);
|
||||
await expect(navItem).toBeVisible();
|
||||
await expect(navItem).toHaveText(/Hello E2E/);
|
||||
expect((await navItem.boundingBox())?.height).toBeGreaterThanOrEqual(44);
|
||||
|
||||
// The sheet slides up on open and the section sits below the fold, so
|
||||
// settle both before shooting or the asset captures an empty mid-animation
|
||||
// drawer instead of the change under test.
|
||||
await testPage.getByTestId("mobile-plugin-nav-section").scrollIntoViewIfNeeded();
|
||||
await expect(navItem).toBeInViewport();
|
||||
await capture.screenshot("mobile-plugin-nav-section", {
|
||||
caption: "Phone menu sheet: the plugin's page now has an entry point",
|
||||
});
|
||||
|
||||
await navItem.click();
|
||||
await expect(testPage).toHaveURL(/\/plugins\/e2e-hello$/);
|
||||
await expect(testPage.locator("#hello-plugin-page")).toBeVisible();
|
||||
// Tapping the item also dismisses the sheet.
|
||||
await expect(testPage.getByTestId("mobile-plugin-nav-section")).toBeHidden();
|
||||
|
||||
// A plugin route with default chrome brings no nav of its own, and the
|
||||
// sidebar that normally owns Home is hidden below md — so the topbar must
|
||||
// carry a phone escape route or the user is stranded here.
|
||||
const home = testPage.getByTestId("topbar-phone-home");
|
||||
await expect(home).toBeVisible();
|
||||
await home.click();
|
||||
await expect(testPage).toHaveURL(/\/$/);
|
||||
|
||||
// Desktop parity reference: the same item in the always-visible sidebar.
|
||||
const desktop = await openDesktopClient(browser, backend.frontendUrl, backend.port);
|
||||
await desktop.page.goto("/");
|
||||
await desktop.page.waitForLoadState("networkidle");
|
||||
await expect(desktop.page.getByTestId(`plugin-nav-item-${NAV_ITEM_ID}`)).toBeVisible();
|
||||
await capture.screenshot("desktop-plugin-nav-sidebar", {
|
||||
page: desktop.page,
|
||||
caption: "Desktop, unchanged: the same nav item in the app sidebar",
|
||||
});
|
||||
await desktop.context.close();
|
||||
|
||||
capture.flush();
|
||||
});
|
||||
});
|
||||
@@ -129,7 +129,9 @@ interface PluginRegistry {
|
||||
registerRoute(path: string, Component: React.ComponentType, options?: PluginRouteOptions): void;
|
||||
|
||||
// Sidebar/main nav entry. Rendered by <PluginNavItems/> in the app sidebar,
|
||||
// with item.icon resolved against the curated icon map (fallback: puzzle).
|
||||
// and by <MobilePluginNavSection/> in the phone menu sheet (the sidebar is
|
||||
// hidden below md), with item.icon resolved against the curated icon map
|
||||
// (fallback: puzzle).
|
||||
registerNavItem(item: NavItem): void;
|
||||
|
||||
// Route under /settings/plugins/{id}/... rendered inside settings shell.
|
||||
|
||||
@@ -337,6 +337,8 @@ interface NavItem {
|
||||
icon?: string;
|
||||
// "main" (default): top-level sidebar entry. "integrations": renders inside
|
||||
// the sidebar's Integrations section alongside first-party integration links.
|
||||
// Both also render in the phone menu sheet, so plugin pages stay reachable
|
||||
// when the desktop sidebar is hidden.
|
||||
// "settings" is accepted by the type but not rendered by any sidebar section
|
||||
// today — for a settings page use registerSettingsRoute instead.
|
||||
section?: "main" | "settings" | "integrations";
|
||||
|
||||
Reference in New Issue
Block a user