From 60d37aa24ad15629a8be3bb175dbf72f04ac35fb Mon Sep 17 00:00:00 2001 From: nova28 Date: Sun, 16 Aug 2026 02:33:08 +0800 Subject: [PATCH] feat(plugins): let nav items target the sidebar footer icon row (#2562) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Carlos Florencio --- .../fixture-package/ui/bundle.js | 35 + apps/packages/plugin-sdk/src/index.ts | 5 +- .../app-sidebar/app-sidebar-footer.test.tsx | 336 ++++++- .../app-sidebar/app-sidebar-footer.tsx | 114 ++- .../navigation/app-nav-sheet.test.tsx | 63 +- .../mobile-plugin-nav-section.test.tsx | 13 + .../plugins/plugin-nav-items.test.tsx | 13 + .../tests/plugins/mobile-plugin-nav.spec.ts | 31 + apps/web/e2e/tests/plugins/plugins.spec.ts | 98 ++ .../navigation/plugin-destinations.test.ts | 132 ++- .../web/lib/navigation/plugin-destinations.ts | 29 +- .../lib/navigation/plugin-footer-budget.ts | 14 + .../navigation/resolve-destinations.test.ts | 39 + apps/web/lib/plugins/host-lifecycle.test.ts | 107 +++ apps/web/lib/plugins/host-reenable.test.ts | 181 ++++ apps/web/lib/plugins/host.test.ts | 58 +- apps/web/lib/plugins/host.ts | 28 +- apps/web/lib/plugins/sdk-contract.test.ts | 7 + apps/web/lib/plugins/types.ts | 15 +- apps/web/src/locales/en/sidebar.json | 1 + apps/web/src/locales/pseudo/sidebar.json | 1 + docs/plans/plugin-nav-sidebar-footer/plan.md | 307 ++++++ .../task-01-contract-and-mapping.md | 199 ++++ .../task-02-frontend-regression-coverage.md | 167 ++++ .../task-03-e2e-coverage.md | 228 +++++ docs/plans/plugins/PLUGIN-API.md | 24 +- docs/public/plugins-authoring.md | 2 +- docs/specs/INDEX.md | 1 + docs/specs/plugin-nav-sidebar-footer/spec.md | 876 ++++++++++++++++++ 29 files changed, 3031 insertions(+), 93 deletions(-) create mode 100644 apps/web/lib/navigation/plugin-footer-budget.ts create mode 100644 apps/web/lib/plugins/host-reenable.test.ts create mode 100644 docs/plans/plugin-nav-sidebar-footer/plan.md create mode 100644 docs/plans/plugin-nav-sidebar-footer/task-01-contract-and-mapping.md create mode 100644 docs/plans/plugin-nav-sidebar-footer/task-02-frontend-regression-coverage.md create mode 100644 docs/plans/plugin-nav-sidebar-footer/task-03-e2e-coverage.md create mode 100644 docs/specs/plugin-nav-sidebar-footer/spec.md diff --git a/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js b/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js index 367bba9c90..bb73f432e7 100644 --- a/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js +++ b/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js @@ -517,6 +517,41 @@ path: "/plugins/e2e-hello", section: "main", }); + registry.registerNavItem({ + id: "e2e-insights-tools", + label: "E2E Insights Tools", + path: "/plugins/e2e-hello", + section: "sidebar-footer", + }); + // Three more sidebar-footer items so this one plugin install alone + // produces P = 4 (budget MAX_INLINE_PLUGIN_FOOTER_ITEMS = 3, plus one + // over-budget item) — enough to drive the desktop footer's overflow + // trigger and menu with the real Radix DropdownMenu in a browser (see + // plugins.spec.ts's overflow test). The budget counts destinations, + // not distinct plugins, so one plugin registering 4 items exercises + // the same partition as 4 plugins registering 1 each. Labeled + // "E2E Overflow Item N" rather than a numbered suffix of the first + // item's own label ("E2E Insights Tools") so Playwright's default + // substring name matching can't accidentally match more than one of + // these from an existing test written against the first item's label. + registry.registerNavItem({ + id: "e2e-insights-tools-2", + label: "E2E Overflow Item 2", + path: "/plugins/e2e-hello", + section: "sidebar-footer", + }); + registry.registerNavItem({ + id: "e2e-insights-tools-3", + label: "E2E Overflow Item 3", + path: "/plugins/e2e-hello", + section: "sidebar-footer", + }); + registry.registerNavItem({ + id: "e2e-insights-tools-4", + label: "E2E Overflow Item 4", + path: "/plugins/e2e-hello", + section: "sidebar-footer", + }); registry.registerRoute("/plugins/e2e-hello", PluginPage); registry.registerComponent("task-sidebar", SidebarSlot); registry.registerComponent("main-top-bar", MainTopBarSlot); diff --git a/apps/packages/plugin-sdk/src/index.ts b/apps/packages/plugin-sdk/src/index.ts index 27c9d2ef23..e14f3871df 100644 --- a/apps/packages/plugin-sdk/src/index.ts +++ b/apps/packages/plugin-sdk/src/index.ts @@ -24,6 +24,9 @@ export interface PluginIconProps { /** Curated host icon name or a plugin-owned component rendered with host React. */ export type PluginIcon = string | Component; +/** Placement for a registered nav item; see `PluginRegistry.registerNavItem`. */ +export type PluginNavSection = "main" | "settings" | "integrations" | "sidebar-footer"; + export type StateUpdater = Value | ((previous: Value) => Value); export type StateSetter = (value: StateUpdater) => void; @@ -577,7 +580,7 @@ export interface PluginRegistry { label: string; path: string; icon?: PluginIcon; - section?: "main" | "settings" | "integrations"; + section?: PluginNavSection; }): void; registerSettingsRoute(path: string, component: Component): void; registerComponent(slot: string, component: Component<{ slotProps?: unknown }>): void; diff --git a/apps/web/components/app-sidebar/app-sidebar-footer.test.tsx b/apps/web/components/app-sidebar/app-sidebar-footer.test.tsx index 407acc2dd4..fe051c1d0a 100644 --- a/apps/web/components/app-sidebar/app-sidebar-footer.test.tsx +++ b/apps/web/components/app-sidebar/app-sidebar-footer.test.tsx @@ -1,7 +1,8 @@ -import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { cleanup, fireEvent, render, screen, within } from "@testing-library/react"; import { TooltipProvider } from "@kandev/ui/tooltip"; import { IconChartBar } from "@tabler/icons-react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { t } from "@/lib/i18n"; const mocks = vi.hoisted(() => ({ routerPush: vi.fn(), @@ -10,6 +11,16 @@ const mocks = vi.hoisted(() => ({ setImproveDialogOpen: vi.fn(), })); +// Shared with the `@kandev/ui/dropdown-menu` mock below so both the mock and +// the assertions that scope to it stay in lockstep. Defined via `vi.hoisted` +// (not a plain module-scope `const`) because `vi.mock` factories run during +// this file's static import of `./app-sidebar-footer` (see that import +// below), which executes before any later top-level `const` in this file +// would have run. +const overflowMenuTestIds = vi.hoisted(() => ({ + content: "sidebar-plugin-overflow-content", +})); + const state = { workspaces: { activeId: "kanban-1" as string | null, @@ -56,17 +67,31 @@ vi.mock("@/hooks/domains/features/use-feature", () => ({ })); // The footer renders its insight buttons from the navigation manifest; the -// manifest itself is covered in `lib/navigation/core-destinations.test.ts`. +// manifest itself is covered in `lib/navigation/core-destinations.test.ts` and +// `lib/navigation/plugin-destinations.test.ts`. `insightDestinations` is +// mutable so individual tests can inject plugin entries alongside `stats`. +type FooterDestination = { + id: string; + label: string; + icon: typeof IconChartBar; + section: string; + href: string; + source?: "plugin"; + pluginItemId?: string; +}; + +const STATS_DESTINATION: FooterDestination = { + id: "stats", + label: "Stats", + icon: IconChartBar, + section: "insights", + href: "/stats", +}; + +let insightDestinations: FooterDestination[] = [STATS_DESTINATION]; + vi.mock("@/hooks/use-app-destinations", () => ({ - useStaticDestinations: () => [ - { - id: "stats", - label: "Stats", - icon: IconChartBar, - section: "insights", - href: "/stats", - }, - ], + useStaticDestinations: () => insightDestinations, })); vi.mock("@/hooks/use-release-notes", () => ({ @@ -98,13 +123,31 @@ vi.mock("@/lib/api/domains/auth-api", () => ({ logout: mocks.logout, })); +// Radix Tooltip's hover/focus-triggered visibility isn't modelled well by +// jsdom; render both the trigger and its content unconditionally, matching +// this repo's convention elsewhere (see agents-section.test.tsx) so tooltip +// text is directly assertable without simulating hover or focus. +vi.mock("@kandev/ui/tooltip", () => ({ + TooltipProvider: ({ children }: { children: React.ReactNode }) => <>{children}, + Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: React.ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: React.ReactNode }) =>
{children}
, +})); + // Radix dropdown primitives rely on pointer/portal behaviour that jsdom // doesn't model well; render them as plain elements so clicks reach the // current-user chip's own logic (see app-sidebar-workspace-picker.test.tsx). +// `DropdownMenuContent`'s wrapper carries a fixed testid (not gated on open +// state, matching this repo's existing dropdown-menu mock convention) so +// capacity tests can scope `within()` to it and prove an item is actually +// inside the overflow menu rather than merely present somewhere in the +// document — the same testid an inline `FooterIconButton` would also use. vi.mock("@kandev/ui/dropdown-menu", () => ({ DropdownMenu: ({ children }: { children: React.ReactNode }) =>
{children}
, DropdownMenuTrigger: ({ children }: { children: React.ReactNode }) => <>{children}, - DropdownMenuContent: ({ children }: { children: React.ReactNode }) =>
{children}
, + DropdownMenuContent: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), DropdownMenuItem: ({ children, onClick, @@ -120,12 +163,12 @@ vi.mock("@kandev/ui/dropdown-menu", () => ({ ), })); -import { AppSidebarFooter } from "./app-sidebar-footer"; +import { AppSidebarFooter, MAX_INLINE_PLUGIN_FOOTER_ITEMS } from "./app-sidebar-footer"; -function renderFooter() { +function renderFooter(collapsed = false) { return render( - + , ); } @@ -148,6 +191,7 @@ function resetFooterState() { document.cookie = "office-active-workspace=; path=/; max-age=0"; mocks.routerPush.mockClear(); mocks.toggleSettingsMode.mockClear(); + insightDestinations = [STATS_DESTINATION]; } const KANBAN_HOME_HREF = "/?home=overview&workspaceId=kanban-1"; @@ -388,3 +432,265 @@ describe("AppSidebarFooter current-user chip", () => { expect(mocks.logout).toHaveBeenCalledOnce(); }); }); + +function pluginDestination(i: number): FooterDestination { + return { + id: `plugin:acme-${i}:board`, + label: `Acme Board ${i}`, + icon: IconChartBar, + section: "insights", + href: `/plugins/acme-${i}`, + source: "plugin" as const, + pluginItemId: "board", + }; +} + +function pluginDestinations(count: number): FooterDestination[] { + return Array.from({ length: count }, (_, i) => pluginDestination(i)); +} + +function eightPluginDestinations(): FooterDestination[] { + return [STATS_DESTINATION, ...pluginDestinations(8)]; +} + +const OVERFLOW_TRIGGER_TEST_ID = "sidebar-plugin-overflow-button"; + +/** + * Scopes a testid lookup to the overflow menu's own content, distinguishing + * an item genuinely placed in the overflow menu from one merely present + * somewhere in the document — an inline `FooterIconButton` and an overflow + * `DropdownMenuItem` share the identical testid derivation (see + * spec.md#Rendered-identity), so an unscoped `getByTestId` cannot tell them + * apart and would still pass if the partition boundary were wrong. + */ +function overflowMenuContent() { + return screen.getByTestId(overflowMenuTestIds.content); +} + +describe("AppSidebarFooter plugin insights items", () => { + beforeEach(resetFooterState); + + afterEach(() => cleanup()); + + it("renders a plugin insights destination as an icon button with the owner-namespaced testid, and navigates on click", () => { + insightDestinations = [ + STATS_DESTINATION, + { + id: "plugin:acme:board", + label: "Acme Board", + icon: IconChartBar, + section: "insights", + href: "/plugins/acme", + source: "plugin", + pluginItemId: "board", + }, + ]; + + renderFooter(); + + const button = screen.getByTestId("sidebar-plugin:acme:board-button"); + expect(button).not.toBeNull(); + expect(button.getAttribute("aria-label")).toBe("Acme Board"); + + fireEvent.click(button); + + expect(mocks.routerPush).toHaveBeenCalledWith("/plugins/acme"); + }); +}); + +describe("AppSidebarFooter plugin footer capacity and overflow", () => { + beforeEach(resetFooterState); + + afterEach(() => cleanup()); + + it("renders no overflow trigger when no plugin registers a sidebar-footer item (P = 0)", () => { + insightDestinations = [STATS_DESTINATION]; + + renderFooter(); + + // The manifest button set is exactly stats (spec.md:789's first clause) — + // without this, a component rendering nothing at all would still pass + // the two absence checks below. + expect(screen.getByRole("button", { name: "Stats" })).not.toBeNull(); + // Soleness, not just presence: no plugin button exists either, so Stats + // is the *only* manifest button (spec.md:789's middle clause) — without + // this, a component that rendered Stats alongside a stray plugin button + // would still pass the Stats-presence check above. + expect(document.querySelectorAll('[data-testid^="sidebar-plugin:"]')).toHaveLength(0); + expect(screen.queryByTestId(OVERFLOW_TRIGGER_TEST_ID)).toBeNull(); + expect(screen.queryByTestId(overflowMenuTestIds.content)).toBeNull(); + }); + + it("renders all plugin buttons inline with no trigger when P is at the budget", () => { + insightDestinations = [ + STATS_DESTINATION, + ...pluginDestinations(MAX_INLINE_PLUGIN_FOOTER_ITEMS), + ]; + + renderFooter(); + + for (let i = 0; i < MAX_INLINE_PLUGIN_FOOTER_ITEMS; i++) { + expect(screen.getByTestId(`sidebar-plugin:acme-${i}:board-button`)).not.toBeNull(); + } + expect(screen.queryByTestId(OVERFLOW_TRIGGER_TEST_ID)).toBeNull(); + // No overflow menu is rendered at all at the budget boundary, so there is + // no menu container for any item to have wrongly landed in. + expect(screen.queryByTestId(overflowMenuTestIds.content)).toBeNull(); + }); + + it("partitions the first over-budget item into the overflow trigger's menu", () => { + insightDestinations = [ + STATS_DESTINATION, + ...pluginDestinations(MAX_INLINE_PLUGIN_FOOTER_ITEMS + 1), + ]; + + renderFooter(); + + const menu = overflowMenuContent(); + for (let i = 0; i < MAX_INLINE_PLUGIN_FOOTER_ITEMS; i++) { + // Present as an inline button, and specifically NOT inside the overflow + // menu's own content — proves these are the inline run, not merely + // present somewhere in the document. + expect(screen.getByTestId(`sidebar-plugin:acme-${i}:board-button`)).not.toBeNull(); + expect(within(menu).queryByTestId(`sidebar-plugin:acme-${i}:board-button`)).toBeNull(); + } + const trigger = screen.getByTestId(OVERFLOW_TRIGGER_TEST_ID); + expect(trigger).not.toBeNull(); + + // The over-budget item's menu item shares its testid with what an inline + // button would use (see spec.md#Rendered-identity), so the click below + // mirrors the real interaction. Note this file's `DropdownMenuContent` + // mock (top of file) renders its content unconditionally regardless of + // open/closed state, so this click does not itself prove the real Radix + // menu opens on click or hides its content while closed — that guarantee + // (spec.md#The-guarantee) is covered by + // e2e/tests/plugins/plugins.spec.ts's over-budget overflow test, which + // exercises the real component. + const overIndex = MAX_INLINE_PLUGIN_FOOTER_ITEMS; + fireEvent.click(trigger); + // Scoped to the menu's own content, not just present anywhere in the + // document — a partition boundary shifted by one would move this + // assertion's target into the inline run instead, and this would then + // correctly fail rather than still finding the testid elsewhere. + const menuItem = within(menu).getByTestId(`sidebar-plugin:acme-${overIndex}:board-button`); + expect(menuItem.textContent).toContain(`Acme Board ${overIndex}`); + + fireEvent.click(menuItem); + expect(mocks.routerPush).toHaveBeenCalledWith(`/plugins/acme-${overIndex}`); + }); + + it("labels the overflow trigger's accessible name and tooltip from the sidebar:morePluginItems key", () => { + insightDestinations = [ + STATS_DESTINATION, + ...pluginDestinations(MAX_INLINE_PLUGIN_FOOTER_ITEMS + 1), + ]; + + renderFooter(); + + // Derived from the key, not hard-coded, so a legitimate copy edit to + // `sidebar:morePluginItems` moves this assertion with it rather than + // false-failing the suite (spec.md#Capacity-and-overflow). + const label = t("sidebar:morePluginItems"); + const trigger = screen.getByTestId(OVERFLOW_TRIGGER_TEST_ID); + expect(trigger.getAttribute("aria-label")).toBe(label); + expect(screen.getByText(label)).not.toBeNull(); + }); +}); + +// Split from the describe block above (own file's max-lines-per-function +// limit) rather than a semantic distinction: these scenarios are the same +// budget/overflow contract at a larger plugin count and under registration +// reordering. +describe("AppSidebarFooter plugin footer capacity at scale", () => { + beforeEach(resetFooterState); + + afterEach(() => cleanup()); + + it("keeps the budget at 3 inline plus one trigger for 8 plugins, expanded, dropping none", () => { + insightDestinations = eightPluginDestinations(); + + renderFooter(false); + + const menu = overflowMenuContent(); + for (let i = 0; i < MAX_INLINE_PLUGIN_FOOTER_ITEMS; i++) { + expect(screen.getByTestId(`sidebar-plugin:acme-${i}:board-button`)).not.toBeNull(); + expect(within(menu).queryByTestId(`sidebar-plugin:acme-${i}:board-button`)).toBeNull(); + } + const trigger = screen.getByTestId(OVERFLOW_TRIGGER_TEST_ID); + fireEvent.click(trigger); + for (let i = MAX_INLINE_PLUGIN_FOOTER_ITEMS; i < 8; i++) { + expect(within(menu).getByTestId(`sidebar-plugin:acme-${i}:board-button`)).not.toBeNull(); + } + const container = screen.getByTestId("sidebar-settings-gear").parentElement; + expect(container?.className).toContain("flex-wrap"); + }); + + it("keeps the same budget and trigger for 8 plugins when collapsed, in a non-wrapping column", () => { + insightDestinations = eightPluginDestinations(); + + renderFooter(true); + + const menu = overflowMenuContent(); + for (let i = 0; i < MAX_INLINE_PLUGIN_FOOTER_ITEMS; i++) { + expect(screen.getByTestId(`sidebar-plugin:acme-${i}:board-button`)).not.toBeNull(); + expect(within(menu).queryByTestId(`sidebar-plugin:acme-${i}:board-button`)).toBeNull(); + } + expect(screen.getByTestId(OVERFLOW_TRIGGER_TEST_ID)).not.toBeNull(); + for (let i = MAX_INLINE_PLUGIN_FOOTER_ITEMS; i < 8; i++) { + expect(within(menu).getByTestId(`sidebar-plugin:acme-${i}:board-button`)).not.toBeNull(); + } + const container = screen.getByTestId("sidebar-settings-gear").parentElement; + expect(container?.className).toContain("flex-col"); + }); + + it("always renders stats inline, applying the budget to plugin entries alone", () => { + insightDestinations = eightPluginDestinations(); + + renderFooter(); + + const menu = overflowMenuContent(); + const statsButton = screen.getByRole("button", { name: "Stats" }); + expect(statsButton).not.toBeNull(); + expect(within(menu).queryByRole("button", { name: "Stats" })).toBeNull(); + for (let i = 0; i < MAX_INLINE_PLUGIN_FOOTER_ITEMS; i++) { + expect(screen.getByTestId(`sidebar-plugin:acme-${i}:board-button`)).not.toBeNull(); + expect(within(menu).queryByTestId(`sidebar-plugin:acme-${i}:board-button`)).toBeNull(); + } + }); + + it("partitions by current registration order, so a re-enabled plugin moves from inline to the overflow menu", () => { + // Simulates the post-re-enable order: p1 moved to the end of the plugin + // run (spec's Ordering + Capacity re-enable scenario). The footer only + // partitions whatever order it is given; this test asserts that + // partition, not that the registry actually reorders on re-enable — + // `registry.test.ts` covers re-enable ordering for slot components, not + // nav items, so that mechanism itself has no direct nav-item test. + insightDestinations = [ + STATS_DESTINATION, + { ...pluginDestination(2), id: "plugin:p2:board", label: "P2" }, + { ...pluginDestination(3), id: "plugin:p3:board", label: "P3" }, + { ...pluginDestination(4), id: "plugin:p4:board", label: "P4" }, + { ...pluginDestination(1), id: "plugin:p1:board", label: "P1" }, + ]; + + renderFooter(); + + const menu = overflowMenuContent(); + expect(screen.getByTestId("sidebar-plugin:p2:board-button")).not.toBeNull(); + expect(screen.getByTestId("sidebar-plugin:p3:board-button")).not.toBeNull(); + expect(screen.getByTestId("sidebar-plugin:p4:board-button")).not.toBeNull(); + // Scoped absence: p2-p4 must not be the ones that landed in the overflow + // menu — otherwise a boundary bug that kept p1 inline and overflowed one + // of these instead would still satisfy the presence checks above. + expect(within(menu).queryByTestId("sidebar-plugin:p2:board-button")).toBeNull(); + expect(within(menu).queryByTestId("sidebar-plugin:p3:board-button")).toBeNull(); + expect(within(menu).queryByTestId("sidebar-plugin:p4:board-button")).toBeNull(); + + // p1's menu item shares its testid with what an inline button would use + // (spec.md#Rendered-identity); this click mirrors the real interaction, + // but — as noted above — the `DropdownMenuContent` mock does not model + // open/closed state, so it does not itself prove the real menu opens. + fireEvent.click(screen.getByTestId(OVERFLOW_TRIGGER_TEST_ID)); + expect(within(menu).getByTestId("sidebar-plugin:p1:board-button")).not.toBeNull(); + }); +}); diff --git a/apps/web/components/app-sidebar/app-sidebar-footer.tsx b/apps/web/components/app-sidebar/app-sidebar-footer.tsx index 16e7207d97..8888b74680 100644 --- a/apps/web/components/app-sidebar/app-sidebar-footer.tsx +++ b/apps/web/components/app-sidebar/app-sidebar-footer.tsx @@ -4,6 +4,7 @@ import { useTranslation } from "react-i18next"; import { useRouter, usePathname } from "@/lib/routing/client-router"; import { IconBuildings, + IconDots, IconLayoutKanban, IconSettings, IconSparkles, @@ -12,8 +13,15 @@ import { } from "@tabler/icons-react"; import { useStaticDestinations } from "@/hooks/use-app-destinations"; import type { DestinationIcon } from "@/lib/navigation/types"; +import { MAX_INLINE_PLUGIN_FOOTER_ITEMS } from "@/lib/navigation/plugin-footer-budget"; import { Button } from "@kandev/ui/button"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger, +} from "@kandev/ui/dropdown-menu"; import { ImproveKandevDialog } from "@/components/improve-kandev-dialog"; import { ReleaseNotesDialog } from "@/components/release-notes/release-notes-dialog"; import { useAppStore } from "@/components/state-provider"; @@ -184,18 +192,117 @@ function SidebarFooterDialogs({ ); } +/** + * Re-exported so existing unit-test imports from this module keep working — + * the value itself lives in `plugin-footer-budget.ts` (see that module's + * doc comment) so Playwright specs, which run outside the React tree, can + * import it too without pulling in this file's JSX/React dependencies. + */ +export { MAX_INLINE_PLUGIN_FOOTER_ITEMS }; + +type InsightDestinations = ReturnType; + +/** + * Splits the resolved `insights` destinations into the inline run and the + * overflow run. First-party entries (today, only `stats`) are never counted + * against the budget and always render inline; the budget applies to + * `source === "plugin"` entries only, partitioning them in place without + * reordering — concatenating `inline` and `overflow` reproduces the original + * order. + */ +function partitionInsightDestinations(destinations: InsightDestinations): { + inline: InsightDestinations; + overflow: InsightDestinations; +} { + const inline: InsightDestinations = []; + const overflow: InsightDestinations = []; + let pluginCount = 0; + + for (const destination of destinations) { + if (destination.source !== "plugin") { + inline.push(destination); + continue; + } + if (pluginCount < MAX_INLINE_PLUGIN_FOOTER_ITEMS) { + inline.push(destination); + } else { + overflow.push(destination); + } + pluginCount += 1; + } + + return { inline, overflow }; +} + +/** + * Overflow trigger for plugin `insights` destinations past the inline + * budget. Reuses `FooterIconButton`'s icon-button treatment (size, tooltip, + * hover) wrapped as a `@kandev/ui/dropdown-menu` trigger. Each menu item + * carries the same `data-testid`/accessible-name derivation as the inline + * button it would otherwise be — see spec.md#Rendered-identity. + */ +function InsightOverflowMenu({ + destinations, + collapsed, + router, +}: { + destinations: InsightDestinations; + collapsed: boolean; + router: ReturnType; +}) { + const { t } = useTranslation(); + const label = t("sidebar:morePluginItems"); + + return ( + + + + + + + + + {destinations.map((destination) => ( + router.push(destination.href)} + > + + {destination.label} + + ))} + + + {label} + + ); +} + function InsightFooterButtons({ destinations, collapsed, router, }: { - destinations: ReturnType; + destinations: InsightDestinations; collapsed: boolean; router: ReturnType; }) { + const { inline, overflow } = partitionInsightDestinations(destinations); + return ( <> - {destinations.map((destination) => ( + {inline.map((destination) => ( ))} + {overflow.length > 0 && ( + + )} ); } diff --git a/apps/web/components/navigation/app-nav-sheet.test.tsx b/apps/web/components/navigation/app-nav-sheet.test.tsx index aca9810363..39a0fc0419 100644 --- a/apps/web/components/navigation/app-nav-sheet.test.tsx +++ b/apps/web/components/navigation/app-nav-sheet.test.tsx @@ -40,9 +40,22 @@ vi.mock("@/hooks/use-in-office", () => ({ useInOffice: () => false, })); +type NavRegistration = { + pluginId: string; + id: string; + label: string; + path: string; + section?: string; +}; + +// Mutable so a specific test can inject plugin registrations (e.g. an +// `insights`-section item) without affecting the rest of the suite, which +// exercises the default no-plugins case. +let navRegistrations: NavRegistration[] = []; + vi.mock("@/lib/plugins/registry", () => ({ usePluginRegistry: () => ({ - getNavRegistrations: () => [], + getNavRegistrations: () => navRegistrations, getSlotRegistrations: (name: string) => name === "sidebar-workspace-actions" ? workspaceActionsRegistrations : [], }), @@ -117,6 +130,7 @@ describe("AppNavSheet", () => { beforeEach(() => { healthHasIssues = false; resolvedTheme = "light"; + navRegistrations = []; workspaceActionsRegistrations = []; vi.clearAllMocks(); }); @@ -176,6 +190,7 @@ describe("AppNavSections", () => { beforeEach(() => { healthHasIssues = false; statusSeverity = "none"; + navRegistrations = []; workspaceActionsRegistrations = []; vi.clearAllMocks(); }); @@ -236,12 +251,58 @@ describe("AppNavSections", () => { expect(screen.getByTestId("app-nav-health-button")).not.toBeNull(); }); + + it("orders the Utilities group's manifest rows as Stats, Settings, then a plugin sidebar-footer item", () => { + navRegistrations = [ + { + pluginId: "acme", + id: "board", + label: "Acme Board", + path: "/plugins/acme", + section: "sidebar-footer", + }, + ]; + + render(); + + const links = screen.getAllByRole("link").map((link) => link.textContent); + const stats = links.indexOf("Stats"); + const settings = links.indexOf("Settings"); + const plugin = links.indexOf("Acme Board"); + + expect(stats).toBeGreaterThanOrEqual(0); + expect(settings).toBeGreaterThan(stats); + expect(plugin).toBeGreaterThan(settings); + }); + + // The phone Utilities group is deliberately uncapped (spec.md#Capacity-and- + // overflow: "the phone surface is uncapped"), unlike the desktop footer's + // MAX_INLINE_PLUGIN_FOOTER_ITEMS budget — well over that budget (8, per + // spec.md's own "well over the budget" scenario) must still render every + // item as a row, with no overflow menu of its own. + it("renders every plugin sidebar-footer item as a row, uncapped, with no overflow menu", () => { + navRegistrations = Array.from({ length: 8 }, (_, i) => ({ + pluginId: "acme", + id: `board-${i}`, + label: `Acme Board ${i}`, + path: `/plugins/acme-${i}`, + section: "sidebar-footer", + })); + + render(); + + for (let i = 0; i < 8; i++) { + expect(screen.getByRole("link", { name: `Acme Board ${i}` })).not.toBeNull(); + } + expect(screen.queryByTestId("sidebar-plugin-overflow-button")).toBeNull(); + }); }); describe("AppNavSections theme toggle", () => { beforeEach(() => { healthHasIssues = false; resolvedTheme = "light"; + navRegistrations = []; workspaceActionsRegistrations = []; vi.clearAllMocks(); }); diff --git a/apps/web/components/plugins/mobile-plugin-nav-section.test.tsx b/apps/web/components/plugins/mobile-plugin-nav-section.test.tsx index fc4e15b7a7..ff25dc2d4f 100644 --- a/apps/web/components/plugins/mobile-plugin-nav-section.test.tsx +++ b/apps/web/components/plugins/mobile-plugin-nav-section.test.tsx @@ -73,6 +73,19 @@ describe("MobilePluginNavSection", () => { expect(container.innerHTML).toBe(""); }); + it("also excludes sidebar-footer items: they belong to the Utilities group, not here", () => { + pluginRegistry.forPlugin("plugin-a").registerNavItem({ + id: "board", + label: "Board", + path: "/plugins/board", + section: "sidebar-footer", + }); + + const { container } = renderSection(); + + expect(container.innerHTML).toBe(""); + }); + it("renders the named curated icon, falling back to the puzzle glyph", () => { pluginRegistry .forPlugin("plugin-a") diff --git a/apps/web/components/plugins/plugin-nav-items.test.tsx b/apps/web/components/plugins/plugin-nav-items.test.tsx index 54790038e2..e1782ac78c 100644 --- a/apps/web/components/plugins/plugin-nav-items.test.tsx +++ b/apps/web/components/plugins/plugin-nav-items.test.tsx @@ -73,6 +73,19 @@ describe("PluginNavItems", () => { expect(screen.queryByTestId("plugin-nav-item-integration-item")).toBeNull(); }); + it("keeps sidebar-footer items out of the main plugin rail", () => { + pluginRegistry.forPlugin("plugin-a").registerNavItem({ + id: "insights-item", + label: "Insights Item", + path: "/plugins/insights-item", + section: "sidebar-footer", + }); + + renderNavItems(); + + expect(screen.queryByTestId("plugin-nav-item-insights-item")).toBeNull(); + }); + it("renders the named curated icon, falling back to the puzzle glyph", () => { pluginRegistry .forPlugin("plugin-a") diff --git a/apps/web/e2e/tests/plugins/mobile-plugin-nav.spec.ts b/apps/web/e2e/tests/plugins/mobile-plugin-nav.spec.ts index 7b78d1ae2f..284df33ada 100644 --- a/apps/web/e2e/tests/plugins/mobile-plugin-nav.spec.ts +++ b/apps/web/e2e/tests/plugins/mobile-plugin-nav.spec.ts @@ -287,4 +287,35 @@ test.describe("Mobile plugin navigation", () => { } } }); + + test("shows the sidebar-footer item in the Utilities group, not the Plugins group", async ({ + testPage, + }) => { + test.setTimeout(60_000); + + 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(); + await testPage.getByRole("button", { name: "Open menu" }).click(); + + // Utilities rows carry no data-testid (see spec's Rendered identity + // section) — select by the visible label instead. + const utilitiesRow = testPage.getByRole("link", { name: "E2E Insights Tools" }); + await expect(utilitiesRow).toBeVisible(); + + // Moves, does not add: the same item never also renders in the Plugins + // group. + const pluginsGroup = testPage.getByTestId("mobile-plugin-nav-section"); + await expect(pluginsGroup.getByText("E2E Insights Tools")).toHaveCount(0); + + // Complete the user path, not just the render: the row navigates. + await utilitiesRow.click(); + await expect(testPage).toHaveURL(/\/plugins\/e2e-hello$/); + }); }); diff --git a/apps/web/e2e/tests/plugins/plugins.spec.ts b/apps/web/e2e/tests/plugins/plugins.spec.ts index 57955454e2..a5a6636a48 100644 --- a/apps/web/e2e/tests/plugins/plugins.spec.ts +++ b/apps/web/e2e/tests/plugins/plugins.spec.ts @@ -50,6 +50,7 @@ import { SessionPage } from "../../pages/session-page"; import type { ApiClient } from "../../helpers/api-client"; import { holdPluginInstallResponse } from "../../helpers/plugin-install"; import { dwell } from "../../helpers/causal-waits"; +import { MAX_INLINE_PLUGIN_FOOTER_ITEMS } from "@/lib/navigation/plugin-footer-budget"; const PLUGIN_ID = "kandev-plugin-e2e"; const NAV_ITEM_ID = "e2e-hello"; @@ -772,4 +773,101 @@ test.describe("Plugins — gRPC plugin install/load/live-update/uninstall", () = timeout: 15_000, }); }); + + test("registers a sidebar-footer-section item as a footer icon, not a rail row", async ({ + testPage, + }) => { + test.setTimeout(60_000); + + await openInstallDialog(testPage); + await uploadPackage(testPage, PACKAGE_PATH); + await expect(testPage.getByTestId(`plugin-row-${PLUGIN_ID}`)).toBeVisible({ timeout: 15_000 }); + + await testPage.goto("/"); + await testPage.reload(); + + const footerButton = testPage.getByTestId( + `sidebar-plugin:${PLUGIN_ID}:e2e-insights-tools-button`, + ); + await expect(footerButton).toBeVisible({ timeout: 15_000 }); + await expect(footerButton).toHaveAttribute("aria-label", "E2E Insights Tools"); + await footerButton.click(); + await expect(testPage).toHaveURL(/\/plugins\/e2e-hello$/); + + // Moves, does not add: the same item never also renders in the rail. + await expect(testPage.getByTestId("plugin-nav-item-e2e-insights-tools")).toHaveCount(0); + }); + + test("routes the over-budget sidebar-footer item through the overflow menu, hidden until opened", async ({ + testPage, + }) => { + test.setTimeout(60_000); + + // The fixture registers 4 sidebar-footer items, in registration order: + // e2e-insights-tools (id/label "E2E Insights Tools"), then + // -2/-3/-4 ("E2E Overflow Item 2/3/4"). Deliberately one more than the + // desktop footer's exported MAX_INLINE_PLUGIN_FOOTER_ITEMS budget, so + // this single install drives the real overflow trigger/menu with the + // actual Radix DropdownMenu (spec.md#Capacity-and-overflow, + // spec.md#The-guarantee). Unit coverage in app-sidebar-footer.test.tsx + // mocks DropdownMenu as a pass-through, so it cannot prove the real + // component opens on click or hides its content while closed; this is + // the test that does. + // + // Per spec.md#Capacity-and-overflow, conformance tests derive their + // expectations from the exported constant rather than hard-coding the + // digits — which item lands inline vs. in the overflow menu is computed + // below from MAX_INLINE_PLUGIN_FOOTER_ITEMS, not hard-coded as 3/4. + const fixtureItemIds = [ + "e2e-insights-tools", + "e2e-insights-tools-2", + "e2e-insights-tools-3", + "e2e-insights-tools-4", + ]; + const fixtureItemLabels: Record = { + "e2e-insights-tools": "E2E Insights Tools", + "e2e-insights-tools-2": "E2E Overflow Item 2", + "e2e-insights-tools-3": "E2E Overflow Item 3", + "e2e-insights-tools-4": "E2E Overflow Item 4", + }; + const inlineIds = fixtureItemIds.slice(0, MAX_INLINE_PLUGIN_FOOTER_ITEMS); + const overflowIds = fixtureItemIds.slice(MAX_INLINE_PLUGIN_FOOTER_ITEMS); + // The fixture must register more items than the budget for this test to + // exercise the overflow menu at all — fail loudly here rather than + // silently degrading to an all-inline run if the budget is ever raised + // to meet or exceed the fixture's fixed item count. + expect(overflowIds.length).toBeGreaterThan(0); + + await openInstallDialog(testPage); + await uploadPackage(testPage, PACKAGE_PATH); + await expect(testPage.getByTestId(`plugin-row-${PLUGIN_ID}`)).toBeVisible({ timeout: 15_000 }); + + await testPage.goto("/"); + await testPage.reload(); + + for (const id of inlineIds) { + await expect(testPage.getByTestId(`sidebar-plugin:${PLUGIN_ID}:${id}-button`)).toBeVisible({ + timeout: 15_000, + }); + } + + const overBudgetId = overflowIds[0]; + const overBudgetTestId = `sidebar-plugin:${PLUGIN_ID}:${overBudgetId}-button`; + const overflowTrigger = testPage.getByTestId("sidebar-plugin-overflow-button"); + await expect(overflowTrigger).toBeVisible(); + + // Closed-menu guarantee: the over-budget item's button carries the same + // testid an inline button would use (spec.md#Rendered-identity), so it + // must be entirely absent from the DOM while the menu is closed, not + // merely hidden — a real DropdownMenu unmounts its content when closed. + await expect(testPage.getByTestId(overBudgetTestId)).toHaveCount(0); + + await overflowTrigger.click(); + + const menuItem = testPage.getByTestId(overBudgetTestId); + await expect(menuItem).toBeVisible(); + await expect(menuItem).toHaveText(fixtureItemLabels[overBudgetId]); + await menuItem.click(); + await expect(testPage).toHaveURL(/\/plugins\/e2e-hello$/); + }); }); diff --git a/apps/web/lib/navigation/plugin-destinations.test.ts b/apps/web/lib/navigation/plugin-destinations.test.ts index 22a2544e77..7456cc87b9 100644 --- a/apps/web/lib/navigation/plugin-destinations.test.ts +++ b/apps/web/lib/navigation/plugin-destinations.test.ts @@ -4,6 +4,46 @@ import { resolveDestinations } from "./resolve-destinations"; import { NO_WORKSPACE_CONTEXT } from "./surface-policy"; import type { AvailabilityMap, NavContext } from "./types"; import type { PluginNavRegistration } from "@/lib/plugins/registry"; +import type { PluginNavSection } from "@/lib/plugins/types"; +import type { NavItem } from "@/lib/plugins/types"; + +/** + * Type-equality helper (standard conditional-type comparison): typechecks + * only when A and B are the exact same type, catching a divergence in either + * direction. See spec.md#Type-surface — the observable is equality, not + * spelling, because TypeScript is structural and cannot distinguish a named + * alias from an identical inline union. + */ +type Equals = + (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 ? true : false; + +/** The host's internal `NavSection` name — never an accepted `PluginNavSection` value. */ +const HOST_INTERNAL_INSIGHTS_SECTION = "insights" as const; + +/** The plugin-facing `PluginNavSection` value that routes to the sidebar footer. */ +const SIDEBAR_FOOTER_SECTION: PluginNavSection = "sidebar-footer"; + +describe("Type surface", () => { + it("assigns all four PluginNavSection members", () => { + const main: PluginNavSection = "main"; + const settings: PluginNavSection = "settings"; + const integrations: PluginNavSection = "integrations"; + expect([main, settings, integrations, SIDEBAR_FOOTER_SECTION]).toHaveLength(4); + }); + + it("rejects the host's internal section name via a suppressed type error", () => { + // @ts-expect-error — the host's internal NavSection name is not an + // accepted PluginNavSection value; if this assignment ever typechecks, + // the unused @ts-expect-error itself becomes the error. + const rejected: PluginNavSection = HOST_INTERNAL_INSIGHTS_SECTION; + expect(rejected).toBe(HOST_INTERNAL_INSIGHTS_SECTION); + }); + + it("keeps NavItem['section'] and PluginNavSection in lockstep", () => { + const equal: Equals = true; + expect(equal).toBe(true); + }); +}); const ALL_INTEGRATIONS: AvailabilityMap = { "azure-devops": true, @@ -14,6 +54,7 @@ const ALL_INTEGRATIONS: AvailabilityMap = { }; const KANBAN: NavContext = { workspaceId: "ws-1", inOffice: false }; +const ACME_BOARD_ID = "plugin:acme:board"; const pluginItems: PluginNavRegistration[] = [ { pluginId: "acme", id: "hello", label: "Hello", path: "/plugins/hello" }, @@ -38,6 +79,13 @@ const pluginItems: PluginNavRegistration[] = [ path: "/settings/plugins/p", section: "settings", }, + { + pluginId: "acme", + id: "board", + label: "Board", + path: "/plugins/board", + section: SIDEBAR_FOOTER_SECTION, + }, ]; function ids(destinations: { id: string }[]): string[] { @@ -53,6 +101,15 @@ function pluginsIn(surface: "sidebar" | "mobileMenu", items: PluginNavRegistrati }); } +function insightsIn(surface: "sidebar" | "mobileMenu", items: PluginNavRegistration[]) { + return resolveDestinations({ + surface, + section: HOST_INTERNAL_INSIGHTS_SECTION, + ctx: NO_WORKSPACE_CONTEXT, + pluginItems: items, + }); +} + describe("plugin destinations", () => { it("routes main-section items (explicit or omitted) to the plugins group", () => { const resolved = pluginsIn("mobileMenu", pluginItems); @@ -91,10 +148,61 @@ describe("plugin destinations", () => { expect(ids(resolved)).not.toContain("plugin:acme:prefs"); }); - it("keeps plugin items off the palette, which plugins reach via shortcuts", () => { + it("keeps plugin items off the palette", () => { const resolved = resolveDestinations({ surface: "palette", ctx: KANBAN, pluginItems }); expect(ids(resolved)).not.toContain("plugin:acme:hello"); + expect(ids(resolved)).not.toContain(ACME_BOARD_ID); + }); + + it("routes sidebar-footer items to the insights section, on both surfaces", () => { + // The insights section also carries the first-party `stats` destination; + // it always precedes plugin entries (see resolve-destinations.test.ts for + // the ordering contract). + expect(ids(insightsIn("sidebar", pluginItems))).toEqual(["stats", ACME_BOARD_ID]); + expect(ids(insightsIn("mobileMenu", pluginItems))).toEqual(["stats", ACME_BOARD_ID]); + }); + + it("moves, does not add: a sidebar-footer item is absent from the plugins group on both surfaces", () => { + expect(ids(pluginsIn("sidebar", pluginItems))).not.toContain(ACME_BOARD_ID); + expect(ids(pluginsIn("mobileMenu", pluginItems))).not.toContain(ACME_BOARD_ID); + }); + + it("degrades an unrecognised section string to the plugins group, never to insights", () => { + const unknownSection = "footer" as PluginNavRegistration["section"]; + const items: PluginNavRegistration[] = [ + { + pluginId: "acme", + id: "mystery", + label: "Mystery", + path: "/plugins/mystery", + section: unknownSection, + }, + ]; + + expect(ids(pluginsIn("sidebar", items))).toEqual(["plugin:acme:mystery"]); + // Only the first-party `stats` destination remains — the mystery item + // never lands in the insights section. + expect(ids(insightsIn("sidebar", items))).toEqual(["stats"]); + }); + + it("degrades the host's internal section name 'insights' to the plugins group, not an accepted alias", () => { + // Amendment 1: the host's internal NavSection name is not a plugin-facing + // value. It is deliberately not accepted as an alias for "sidebar-footer" + // — see spec.md#insights-is-not-accepted-and-not-an-alias. + const internalName = HOST_INTERNAL_INSIGHTS_SECTION as PluginNavRegistration["section"]; + const items: PluginNavRegistration[] = [ + { + pluginId: "acme", + id: "leaked", + label: "Leaked", + path: "/plugins/leaked", + section: internalName, + }, + ]; + + expect(ids(pluginsIn("sidebar", items))).toEqual(["plugin:acme:leaked"]); + expect(ids(insightsIn("sidebar", items))).toEqual(["stats"]); }); }); @@ -140,6 +248,28 @@ describe("plugin destination identity", () => { ]); }); + it("keeps two plugins' sidebar-footer items apart, in registration order", () => { + const resolved = insightsIn("sidebar", [ + { + pluginId: "acme", + id: "board", + label: "Acme Board", + path: "/plugins/acme", + section: SIDEBAR_FOOTER_SECTION, + }, + { + pluginId: "globex", + id: "board", + label: "Globex Board", + path: "/plugins/globex", + section: SIDEBAR_FOOTER_SECTION, + }, + ]); + + expect(ids(resolved)).toEqual(["stats", ACME_BOARD_ID, "plugin:globex:board"]); + expect(new Set(ids(resolved)).size).toBe(resolved.length); + }); + it("encodes both segments so the separator cannot be forged", () => { // Without encoding, ("a:b", "c") and ("a", "b:c") would both read as // "plugin:a:b:c" — a collision the namespace exists to prevent. diff --git a/apps/web/lib/navigation/plugin-destinations.ts b/apps/web/lib/navigation/plugin-destinations.ts index 1612abfc39..36755e327a 100644 --- a/apps/web/lib/navigation/plugin-destinations.ts +++ b/apps/web/lib/navigation/plugin-destinations.ts @@ -34,8 +34,9 @@ export function pluginDestinationId(pluginId: string, itemId: string): string { } /** - * `section: "settings"` items are skipped — those render in the settings tree's - * `PluginSlot`, not as destinations. + * `section: "settings"` items are skipped — they are accepted and rendered on + * no surface. (A plugin settings page is a separate API: `registerSettingsRoute` + * / `registerComponent("settings-nav", …)`, not this field.) */ export function pluginDestinations(items: PluginNavRegistration[]): Destination[] { return items @@ -45,11 +46,29 @@ export function pluginDestinations(items: PluginNavRegistration[]): Destination[ pluginItemId: item.id, label: item.label, icon: resolvePluginIcon(item.icon), - section: item.section === "integrations" ? ("integrations" as const) : ("plugins" as const), + section: sectionFor(item.section), href: item.path, - // Plugins reach the palette through `registerShortcut`, not as nav - // commands, so plugin destinations stay off that surface. + // No API lets a plugin declare the `palette` surface, so plugin + // destinations never carry it — every section maps to `sidebar` + + // `mobileMenu` only. surfaces: SIDEBAR_AND_MENU, source: "plugin" as const, })); } + +/** + * `"integrations"` keeps its own manifest section; `"sidebar-footer"` (the + * plugin-facing vocabulary) maps to the host's internal `"insights"` section. + * Everything else — `"main"`, omitted, an unrecognised string an untyped + * bundle might pass, or the literal `"insights"` (the host's own internal + * name, deliberately not accepted as an alias — see spec's "insights is not + * accepted, and not an alias") — degrades to `"plugins"`, the plugin rail / + * Plugins group placement. + */ +function sectionFor( + section: PluginNavRegistration["section"], +): "integrations" | "insights" | "plugins" { + if (section === "integrations") return "integrations"; + if (section === "sidebar-footer") return "insights"; + return "plugins"; +} diff --git a/apps/web/lib/navigation/plugin-footer-budget.ts b/apps/web/lib/navigation/plugin-footer-budget.ts new file mode 100644 index 0000000000..a31c5c21a8 --- /dev/null +++ b/apps/web/lib/navigation/plugin-footer-budget.ts @@ -0,0 +1,14 @@ +/** + * Inline budget for plugin `insights`-section destinations in the desktop + * sidebar footer. Exported so conformance tests derive their expectations + * from it rather than hard-coding the digit — see spec.md#Why-3, which also + * states this is a layout constant, not a contract with plugin authors, and + * may move without renegotiating that spec. + * + * Split out of `app-sidebar-footer.tsx` (a JSX/React module) into its own + * side-effect-free module so e2e specs — which run in Playwright's Node + * context, not the app's React tree — can import the same constant instead + * of re-hard-coding it. `app-sidebar-footer.tsx` re-exports this so existing + * unit-test imports from that module are unaffected. + */ +export const MAX_INLINE_PLUGIN_FOOTER_ITEMS = 3; diff --git a/apps/web/lib/navigation/resolve-destinations.test.ts b/apps/web/lib/navigation/resolve-destinations.test.ts index 1d0e228dd7..0d3cb522a7 100644 --- a/apps/web/lib/navigation/resolve-destinations.test.ts +++ b/apps/web/lib/navigation/resolve-destinations.test.ts @@ -3,6 +3,7 @@ import { APP_DESTINATIONS } from "./core-destinations"; import { resolveDestinations } from "./resolve-destinations"; import { MOBILE_MENU_UTILITY_SECTIONS } from "./surface-policy"; import type { AvailabilityMap, NavContext } from "./types"; +import type { PluginNavRegistration } from "@/lib/plugins/registry"; const ALL_INTEGRATIONS: AvailabilityMap = { "azure-devops": true, @@ -133,3 +134,41 @@ describe("resolveDestinations", () => { expect(ids(sidebar)).not.toContain("github"); }); }); + +describe("resolveDestinations with plugin items", () => { + const pluginItems: PluginNavRegistration[] = [ + { + pluginId: "acme", + id: "board", + label: "Acme Board", + path: "/plugins/acme", + section: "sidebar-footer", + }, + ]; + + it("orders the first-party stats destination before a plugin sidebar-footer item on the sidebar", () => { + const resolved = resolveDestinations({ + surface: "sidebar", + section: "insights", + ctx: KANBAN, + pluginItems, + }); + + expect(ids(resolved)).toEqual(["stats", "plugin:acme:board"]); + }); + + it("orders the mobile utility group's manifest rows as stats, settings, then a plugin sidebar-footer item", () => { + const resolved = resolveDestinations({ + surface: "mobileMenu", + section: MOBILE_MENU_UTILITY_SECTIONS, + ctx: KANBAN, + pluginItems, + }); + + // Plugin insights items are appended after the last catalog entry that + // matches this multi-section query (here: settings, a utilities entry), + // so the order is stats, settings, plugin, not interleaved with + // insights-section entries. + expect(ids(resolved)).toEqual(["stats", "settings", "plugin:acme:board"]); + }); +}); diff --git a/apps/web/lib/plugins/host-lifecycle.test.ts b/apps/web/lib/plugins/host-lifecycle.test.ts index cc98bf78b7..84b9e365c7 100644 --- a/apps/web/lib/plugins/host-lifecycle.test.ts +++ b/apps/web/lib/plugins/host-lifecycle.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi, afterEach } from "vitest"; import { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint"; +import { pluginDestinations } from "@/lib/navigation/plugin-destinations"; import { loadPlugins } from "./host"; import { pluginRegistry } from "./registry"; import type { ActivePlugin, PluginHostApi, PluginRegistry } from "./types"; @@ -11,6 +12,9 @@ const NOOP_TOAST = new Proxy(() => 0, { const PLUGIN_LIFECYCLE_A_ID = "plugin-lifecycle-a"; const PLUGIN_TIMEOUT_ID = "plugin-timeout-a"; +const PLUGIN_THROW_ID = "plugin-throw-a"; +const PLUGIN_PARTIAL_TIMEOUT_ID = "plugin-partial-timeout-a"; +const SIDEBAR_FOOTER_SECTION = "sidebar-footer" as const; vi.mock("@/lib/config", () => ({ getBackendConfig: () => ({ apiBaseUrl: "" }), @@ -98,6 +102,8 @@ function deferred() { afterEach(() => { pluginRegistry.unregisterPlugin(PLUGIN_LIFECYCLE_A_ID); pluginRegistry.unregisterPlugin(PLUGIN_TIMEOUT_ID); + pluginRegistry.unregisterPlugin(PLUGIN_THROW_ID); + pluginRegistry.unregisterPlugin(PLUGIN_PARTIAL_TIMEOUT_ID); }); describe("authoritative plugin lifecycle", () => { @@ -170,3 +176,104 @@ describe("authoritative plugin lifecycle", () => { warnSpy.mockRestore(); }); }); + +describe("authoritative plugin lifecycle — partial-initialization-failure retention", () => { + it("keeps a sidebar-footer registration made before initialize() throws, and marks the plugin failed", async () => { + const initialize = vi.fn(async (registry: PluginRegistry) => { + registry.registerNavItem({ + id: "nav-throw-a", + label: "Throw", + path: "/throw-a", + section: SIDEBAR_FOOTER_SECTION, + }); + throw new Error("boom"); + }); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const importer = fakeImporterFor({ + "/throw-bundle.js": (win) => + (win as unknown as FakeWindow).registerKandevPlugin(PLUGIN_THROW_ID, { initialize }), + }); + + await loadPlugins( + [activePlugin({ id: PLUGIN_THROW_ID, bundleUrl: "/throw-bundle.js" })], + makeHostFactory, + importer, + ); + + expect(pluginRegistry.getPluginLifecycle(PLUGIN_THROW_ID)?.status).toBe("failed"); + expect(pluginRegistry.getNavItems()).toContainEqual({ + id: "nav-throw-a", + label: "Throw", + path: "/throw-a", + section: SIDEBAR_FOOTER_SECTION, + }); + // The footer's manifest is built from getNavRegistrations() through + // pluginDestinations() (see plugin-destinations.ts), which has no concept + // of plugin lifecycle status — a "failed" plugin's already-registered item + // maps to the "insights" section exactly like a "ready" plugin's would, + // which is what makes it occupy a footer budget slot (spec.md:816's AC). + const destinations = pluginDestinations(pluginRegistry.getNavRegistrations()); + expect(destinations).toContainEqual( + expect.objectContaining({ + pluginItemId: "nav-throw-a", + section: "insights", + source: "plugin", + }), + ); + errorSpy.mockRestore(); + }); + + it("keeps a sidebar-footer registration made before initialize() times out, and marks the plugin failed", async () => { + const initializeStarted = deferred(); + const initializeGate = deferred(); + const initialize = vi.fn(async (registry: PluginRegistry) => { + registry.registerNavItem({ + id: "nav-partial-timeout", + label: "Partial", + path: "/partial-timeout", + section: SIDEBAR_FOOTER_SECTION, + }); + initializeStarted.resolve(); + await initializeGate.promise; + }); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const importer = fakeImporterFor({ + "/partial-timeout-bundle.js": (win) => + (win as unknown as FakeWindow).registerKandevPlugin(PLUGIN_PARTIAL_TIMEOUT_ID, { + initialize, + }), + }); + + const load = loadPlugins( + [activePlugin({ id: PLUGIN_PARTIAL_TIMEOUT_ID, bundleUrl: "/partial-timeout-bundle.js" })], + makeHostFactory, + importer, + window, + 1, + ); + await initializeStarted.promise; + await load; + + expect(pluginRegistry.getPluginLifecycle(PLUGIN_PARTIAL_TIMEOUT_ID)?.status).toBe("failed"); + expect(pluginRegistry.getNavItems()).toContainEqual({ + id: "nav-partial-timeout", + label: "Partial", + path: "/partial-timeout", + section: SIDEBAR_FOOTER_SECTION, + }); + // See the throw case above: pluginDestinations() has no lifecycle concept, + // so a timed-out plugin's already-registered item still maps to "insights" + // and occupies a footer budget slot exactly like a ready plugin's would. + const destinations = pluginDestinations(pluginRegistry.getNavRegistrations()); + expect(destinations).toContainEqual( + expect.objectContaining({ + pluginItemId: "nav-partial-timeout", + section: "insights", + source: "plugin", + }), + ); + + initializeGate.resolve(); + warnSpy.mockRestore(); + }); +}); diff --git a/apps/web/lib/plugins/host-reenable.test.ts b/apps/web/lib/plugins/host-reenable.test.ts new file mode 100644 index 0000000000..1f548a9d47 --- /dev/null +++ b/apps/web/lib/plugins/host-reenable.test.ts @@ -0,0 +1,181 @@ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { loadPlugins, unloadPlugin } from "./host"; +import { pluginRegistry } from "./registry"; +import { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint"; +import type { ActivePlugin, PluginHostApi, PluginRegistry } from "./types"; + +/** No-op `host.toast`; these specs exercise lifecycle, never notifications. */ +const NOOP_TOAST = new Proxy(() => 0, { + get: () => () => 0, +}) as unknown as PluginHostApi["toast"]; + +vi.mock("@/lib/config", () => ({ + getBackendConfig: () => ({ apiBaseUrl: "" }), +})); + +const PLUGIN_REENABLE_A_ID = "plugin-reenable-a"; +const PLUGIN_REENABLE_A_PATH = "/plugin-reenable-a"; +const NAV_REENABLE_A_ID = "nav-reenable-a"; +const PLUGIN_REENABLE_B_ID = "plugin-reenable-b"; +const PLUGIN_REENABLE_B_PATH = "/plugin-reenable-b"; +const NAV_REENABLE_B_ID = "nav-reenable-b"; + +function makeHostFactory(pluginId: string): PluginHostApi { + return { + pluginId, + React: {} as PluginHostApi["React"], + jsx: {} as PluginHostApi["jsx"], + store: { + getState: () => ({}) as never, + setState: () => {}, + subscribe: () => () => {}, + }, + context: { + getActiveWorkspaceId: () => undefined, + subscribeActiveWorkspace: () => () => {}, + getTaskCreationContext: () => null, + subscribeTaskCreationContext: () => () => {}, + resolveRepositoryId: () => undefined, + }, + api: { + fetch: async () => new Response(), + invokeAction: async () => undefined as TResponse, + baseUrl: "", + }, + i18n: { + locale: "en", + t: (key) => key, + useTranslation: () => ({ locale: "en", t: (key) => key }), + }, + ui: {} as PluginHostApi["ui"], + useResponsiveBreakpoint, + theme: "light", + onThemeChange: () => () => {}, + navigate: () => {}, + openModal: () => ({ close: () => {} }), + openTaskLinkDialog: () => ({ close: () => {} }), + openTaskReview: () => {}, + toast: NOOP_TOAST, + utils: { cn: () => "", generateUUID: () => "uuid", formatRelativeTime: () => "" }, + storage: { + get: async () => undefined, + set: async () => ({ updatedAt: "" }), + delete: async () => {}, + list: async () => [], + subscribe: () => () => {}, + }, + }; +} + +type FakeWindow = Window & { + registerKandevPlugin: (id: string, plugin: unknown) => void; +}; + +function registerFake(id: string, plugin: unknown) { + (window as unknown as FakeWindow).registerKandevPlugin(id, plugin); +} + +function activePlugin(overrides: Partial = {}): ActivePlugin { + return { + id: "plugin-a", + name: "Plugin A", + bundleUrl: "/api/plugins/plugin-a/bundle", + ...overrides, + }; +} + +describe("disable then re-enable in the same session", () => { + afterEach(() => { + pluginRegistry.unregisterPlugin(PLUGIN_REENABLE_A_ID); + pluginRegistry.unregisterPlugin(PLUGIN_REENABLE_B_ID); + }); + + it("re-initializes from the cached registration when the bundle's module-eval side effect only fires once (ESM import caching)", async () => { + const initialize = vi.fn((registry: PluginRegistry) => { + registry.registerNavItem({ id: NAV_REENABLE_A_ID, label: "A", path: PLUGIN_REENABLE_A_PATH }); + }); + let importCount = 0; + const importer = vi.fn(async (_url: string) => { + importCount += 1; + // The real browser only runs a module's top-level side effect on the + // *first* resolution of a given specifier — a second `import(url)` + // resolves from the module cache without re-executing + // `window.registerKandevPlugin(...)`. + if (importCount === 1) { + registerFake(PLUGIN_REENABLE_A_ID, { initialize }); + } + return {}; + }); + + await loadPlugins([activePlugin({ id: PLUGIN_REENABLE_A_ID })], makeHostFactory, importer); + expect(initialize).toHaveBeenCalledTimes(1); + expect(pluginRegistry.getNavItems()).toContainEqual({ + id: NAV_REENABLE_A_ID, + label: "A", + path: PLUGIN_REENABLE_A_PATH, + }); + + unloadPlugin(PLUGIN_REENABLE_A_ID); + expect( + pluginRegistry.getNavItems().find((item) => item.id === NAV_REENABLE_A_ID), + ).toBeUndefined(); + + await loadPlugins([activePlugin({ id: PLUGIN_REENABLE_A_ID })], makeHostFactory, importer); + + expect(initialize).toHaveBeenCalledTimes(2); + expect(pluginRegistry.getNavItems()).toContainEqual({ + id: NAV_REENABLE_A_ID, + label: "A", + path: PLUGIN_REENABLE_A_PATH, + }); + }); + + it("reorders the registry to the end on re-enable, relative to a plugin that stayed enabled throughout", async () => { + const initializeA = vi.fn((registry: PluginRegistry) => { + registry.registerNavItem({ id: NAV_REENABLE_A_ID, label: "A", path: PLUGIN_REENABLE_A_PATH }); + }); + const initializeB = vi.fn((registry: PluginRegistry) => { + registry.registerNavItem({ id: NAV_REENABLE_B_ID, label: "B", path: PLUGIN_REENABLE_B_PATH }); + }); + const importer = vi.fn(async (url: string) => { + if (url === "/reenable-a-bundle.js") { + registerFake(PLUGIN_REENABLE_A_ID, { initialize: initializeA }); + } else if (url === "/reenable-b-bundle.js") { + registerFake(PLUGIN_REENABLE_B_ID, { initialize: initializeB }); + } + return {}; + }); + + await loadPlugins( + [ + activePlugin({ id: PLUGIN_REENABLE_A_ID, bundleUrl: "/reenable-a-bundle.js" }), + activePlugin({ id: PLUGIN_REENABLE_B_ID, bundleUrl: "/reenable-b-bundle.js" }), + ], + makeHostFactory, + importer, + ); + + expect( + pluginRegistry.getNavRegistrations().map((registration) => registration.pluginId), + ).toEqual([PLUGIN_REENABLE_A_ID, PLUGIN_REENABLE_B_ID]); + + unloadPlugin(PLUGIN_REENABLE_A_ID); + await loadPlugins( + [activePlugin({ id: PLUGIN_REENABLE_A_ID, bundleUrl: "/reenable-a-bundle.js" })], + makeHostFactory, + importer, + ); + + // The registry itself reorders on re-enable — A's re-registration is + // appended after B, not merely re-added somewhere. Proves the mechanism + // (spec.md:797/:815's re-enable ordering guarantee), not just a manually + // simulated post-re-enable array as app-sidebar-footer.test.tsx's + // "partitions by current registration order" test discloses it does. + expect( + pluginRegistry.getNavRegistrations().map((registration) => registration.pluginId), + ).toEqual([PLUGIN_REENABLE_B_ID, PLUGIN_REENABLE_A_ID]); + // Count assertion: A is the sole entry after B — a looser "A comes after + // B somewhere" check would still pass if a stray duplicate leaked in. + expect(pluginRegistry.getNavRegistrations()).toHaveLength(2); + }); +}); diff --git a/apps/web/lib/plugins/host.test.ts b/apps/web/lib/plugins/host.test.ts index 5e04a34876..98ea1e50a2 100644 --- a/apps/web/lib/plugins/host.test.ts +++ b/apps/web/lib/plugins/host.test.ts @@ -21,10 +21,6 @@ const BUNDLE_JS_URL = "/bundle.js"; const PLUGIN_UNLOAD_A_ID = "plugin-unload-a"; const PLUGIN_UNLOAD_THROW_A_ID = "plugin-unload-throw-a"; const PLUGIN_UNLOAD_STYLE_A_ID = "plugin-unload-style-a"; -const PLUGIN_REENABLE_A_ID = "plugin-reenable-a"; -const PLUGIN_REENABLE_A_PATH = "/plugin-reenable-a"; -const NAV_REENABLE_A_ID = "nav-reenable-a"; - function makeHostFactory(pluginId: string): PluginHostApi { return { pluginId, @@ -205,7 +201,7 @@ describe("loadPlugins — bundle lifecycle", () => { errorSpy.mockRestore(); }); - it("revokes registrations made before an initialize failure", async () => { + it("keeps registrations made before an initialize failure (spec.md:816)", async () => { const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const pluginId = "plugin-partial-initialize"; const importer = fakeImporterFor({ @@ -224,7 +220,11 @@ describe("loadPlugins — bundle lifecycle", () => { importer, ); - expect(pluginRegistry.getNavItems().find((item) => item.id === "partial-nav")).toBeUndefined(); + expect(pluginRegistry.getNavItems()).toContainEqual({ + id: "partial-nav", + label: "Partial", + path: "/partial", + }); pluginRegistry.unregisterPlugin(pluginId); errorSpy.mockRestore(); }); @@ -672,49 +672,3 @@ describe("unloadPlugin — evictCache option", () => { expect(secondInitialize).toHaveBeenCalledTimes(1); }); }); - -describe("disable then re-enable in the same session", () => { - afterEach(() => { - pluginRegistry.unregisterPlugin(PLUGIN_REENABLE_A_ID); - }); - - it("re-initializes from the cached registration when the bundle's module-eval side effect only fires once (ESM import caching)", async () => { - const initialize = vi.fn((registry: PluginRegistry) => { - registry.registerNavItem({ id: NAV_REENABLE_A_ID, label: "A", path: PLUGIN_REENABLE_A_PATH }); - }); - let importCount = 0; - const importer = vi.fn(async (_url: string) => { - importCount += 1; - // The real browser only runs a module's top-level side effect on the - // *first* resolution of a given specifier — a second `import(url)` - // resolves from the module cache without re-executing - // `window.registerKandevPlugin(...)`. - if (importCount === 1) { - registerFake(PLUGIN_REENABLE_A_ID, { initialize }); - } - return {}; - }); - - await loadPlugins([activePlugin({ id: PLUGIN_REENABLE_A_ID })], makeHostFactory, importer); - expect(initialize).toHaveBeenCalledTimes(1); - expect(pluginRegistry.getNavItems()).toContainEqual({ - id: NAV_REENABLE_A_ID, - label: "A", - path: PLUGIN_REENABLE_A_PATH, - }); - - unloadPlugin(PLUGIN_REENABLE_A_ID); - expect( - pluginRegistry.getNavItems().find((item) => item.id === NAV_REENABLE_A_ID), - ).toBeUndefined(); - - await loadPlugins([activePlugin({ id: PLUGIN_REENABLE_A_ID })], makeHostFactory, importer); - - expect(initialize).toHaveBeenCalledTimes(2); - expect(pluginRegistry.getNavItems()).toContainEqual({ - id: NAV_REENABLE_A_ID, - label: "A", - path: PLUGIN_REENABLE_A_PATH, - }); - }); -}); diff --git a/apps/web/lib/plugins/host.ts b/apps/web/lib/plugins/host.ts index a6a7be4f67..401e062ee1 100644 --- a/apps/web/lib/plugins/host.ts +++ b/apps/web/lib/plugins/host.ts @@ -265,16 +265,32 @@ async function loadPlugin( } } -/** Removes a failed generation without disturbing a newer concurrent load. */ +/** + * Removes a failed generation without disturbing a newer concurrent load. + * Leaves the plugin's registry entries (nav items, routes, etc.) in place: a + * registration made before `initialize()` threw or timed out must survive — + * only the runtime (resources, styles, modals) is torn down. `loadPlugin` + * already marked the plugin `"failed"` before calling this. + */ function revokeFailedLoad(pluginId: string, generation: number): void { if (!isCurrentLoad(pluginId, generation)) return; - deactivatePluginRuntime(pluginId, generation); + deactivatePluginRuntime(pluginId, generation, { unregister: false }); // Fence delayed registrations from an initializer that timed out. claimLoadGeneration(pluginId); } -/** Revokes one active runtime without changing its published lifecycle state. */ -function deactivatePluginRuntime(pluginId: string, expectedGeneration?: number): void { +/** + * Revokes one active runtime without changing its published lifecycle state. + * Pass `unregister: false` to keep the plugin's existing registry entries — + * used by the failed-load path (see `revokeFailedLoad`) so a nav item + * registered before `initialize()` failed is retained, per the + * partial-initialization-failure retention contract (spec.md's AC). + */ +function deactivatePluginRuntime( + pluginId: string, + expectedGeneration?: number, + options?: { unregister?: boolean }, +): void { if (expectedGeneration !== undefined && !isCurrentLoad(pluginId, expectedGeneration)) return; const runtime = activePluginRuntimes.get(pluginId); if (runtime && expectedGeneration !== undefined && runtime.generation !== expectedGeneration) { @@ -289,7 +305,9 @@ function deactivatePluginRuntime(pluginId: string, expectedGeneration?: number): console.error(`[plugins] error destroying plugin "${pluginId}"`, error); } } - pluginRegistry.unregisterPlugin(pluginId); + if (options?.unregister ?? true) { + pluginRegistry.unregisterPlugin(pluginId); + } pluginModalManager.closeAllForPlugin(pluginId); removeStyles(pluginId); } diff --git a/apps/web/lib/plugins/sdk-contract.test.ts b/apps/web/lib/plugins/sdk-contract.test.ts index 6dc7ed3d2f..7ced0d18c6 100644 --- a/apps/web/lib/plugins/sdk-contract.test.ts +++ b/apps/web/lib/plugins/sdk-contract.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import type { PluginHostApi as PublicPluginHostApi, + PluginNavSection as PublicPluginNavSection, PluginRegistry as PublicPluginRegistry, RepositoryProviderRegistration as PublicRepositoryProviderRegistration, ReviewSummary as PublicReviewSummary, @@ -8,6 +9,7 @@ import type { } from "@kandev/plugin-sdk"; import type { PluginHostApi, + PluginNavSection, PluginRegistry, RepositoryProviderRegistration, ReviewItemSummary, @@ -42,6 +44,10 @@ describe("public plugin SDK", () => { const reviewSummaryIsCanonical: SameType = true; const associationIsCanonical: SameType = true; + // Named-import equality, not just structural: a plugin author writes + // `import type { PluginNavSection } from "@kandev/plugin-sdk"` verbatim, and + // TS's structural typing can't otherwise tell an inline union from a named export. + const navSectionIsCanonical: SameType = true; expect(hostHasEveryPublicKey).toBe(true); expect(registryHasEveryPublicKey).toBe(true); expect(publicHasEveryRegistryKey).toBe(true); @@ -50,6 +56,7 @@ describe("public plugin SDK", () => { expect(repositoryProviderIsCanonical).toBe(true); expect(reviewSummaryIsCanonical).toBe(true); expect(associationIsCanonical).toBe(true); + expect(navSectionIsCanonical).toBe(true); expect(publicHostContract).toBeTypeOf("function"); expect(publicRegistryContract).toBeTypeOf("function"); }); diff --git a/apps/web/lib/plugins/types.ts b/apps/web/lib/plugins/types.ts index 231b9b4aa5..c42c74b434 100644 --- a/apps/web/lib/plugins/types.ts +++ b/apps/web/lib/plugins/types.ts @@ -9,7 +9,12 @@ import type { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint" import type { AppState } from "@/lib/state/store"; import type * as PluginSDK from "@kandev/plugin-sdk"; import type { PluginUIApi } from "@kandev/plugin-sdk"; -export type { PluginContextApi, PluginHostRepository, PluginUIApi } from "@kandev/plugin-sdk"; +export type { + PluginContextApi, + PluginHostRepository, + PluginNavSection, + PluginUIApi, +} from "@kandev/plugin-sdk"; export type { PluginIcon } from "@kandev/plugin-sdk"; /** Entry in the boot payload's `plugins` array (backend `ActivePlugin`). */ @@ -32,9 +37,13 @@ export interface NavItem { /** * Where the item renders: "main" (default) as a top-level sidebar entry, * "integrations" inside the sidebar's Integrations section alongside the - * first-party integration links. + * first-party integration links, "sidebar-footer" as an icon button in the + * sidebar footer's icon row and as a labelled row in the phone menu's + * Utilities group (subject to the footer's inline budget — an over-budget + * item is reached through the footer's overflow menu instead), "settings" + * accepted but rendered on no surface. */ - section?: "main" | "settings" | "integrations"; + section?: PluginSDK.PluginNavSection; } /** diff --git a/apps/web/src/locales/en/sidebar.json b/apps/web/src/locales/en/sidebar.json index baacdffbab..3b9ae83705 100644 --- a/apps/web/src/locales/en/sidebar.json +++ b/apps/web/src/locales/en/sidebar.json @@ -48,6 +48,7 @@ "logOut": "Log out", "maxViews_one": "{{count}} max", "maxViews_other": "{{count}} max", + "morePluginItems": "More plugin items", "nestUnder": "Nest under {{title}}", "newKanbanWorkspace": "New kanban workspace", "newOfficeWorkspace": "New office workspace", diff --git a/apps/web/src/locales/pseudo/sidebar.json b/apps/web/src/locales/pseudo/sidebar.json index bb83e31870..73d178245a 100644 --- a/apps/web/src/locales/pseudo/sidebar.json +++ b/apps/web/src/locales/pseudo/sidebar.json @@ -48,6 +48,7 @@ "logOut": "Ĺōĝ ōũţ", "maxViews_one": "{{count}} ḿàx", "maxViews_other": "{{count}} ḿàx", + "morePluginItems": "Ḿōŕē ƥĺũĝĩń ĩţēḿś", "nestUnder": "Ńēśţ ũńďēŕ {{title}}", "newKanbanWorkspace": "Ńēŵ ķàńƀàń ŵōŕķśƥàćē", "newOfficeWorkspace": "Ńēŵ ōƒƒĩćē ŵōŕķśƥàćē", diff --git a/docs/plans/plugin-nav-sidebar-footer/plan.md b/docs/plans/plugin-nav-sidebar-footer/plan.md new file mode 100644 index 0000000000..f0ab75f0d2 --- /dev/null +++ b/docs/plans/plugin-nav-sidebar-footer/plan.md @@ -0,0 +1,307 @@ +--- +spec: docs/specs/plugin-nav-sidebar-footer/spec.md +created: 2026-08-12 +status: superseded +--- + +# Implementation Plan: Plugin nav items in the sidebar footer icon row + +> **SUPERSEDED BY AMENDMENT 1 (2026-08-12). Do not build from this file.** +> +> This plan and its three task files are the implementation record of the **first** +> version of the contract, which is already built and on this branch. The spec was +> amended afterwards (see `## Amendment log` in the spec), and this plan does **not** +> describe the amended contract. Two of its claims are now actively false: +> +> - it plans for the plugin-facing value `"insights"`; the accepted value is now +> `"sidebar-footer"`, and `"insights"` deliberately degrades to the plugin rail; +> - it states no footer component changes are needed. The amendment adds an inline +> budget (`MAX_INLINE_PLUGIN_FOOTER_ITEMS`) and an overflow menu to +> `app-sidebar-footer.tsx`. +> +> The spec is the contract. Build the amendment from the spec, not from here. The +> `status: done` markers on the task files refer to the pre-amendment work only. + +## Overview + +The navigation manifest already has an `insights` `NavSection` — it is how the +first-party `stats` destination reaches the desktop sidebar footer and the phone +menu's Utilities group today (`apps/web/lib/navigation/core-destinations.ts`, +`apps/web/lib/navigation/types.ts`, `apps/web/lib/navigation/surface-policy.ts`). +Every surface that renders a section (the footer, the mobile Utilities group, +the plugin rail, the mobile Plugins group, the palette) already reads through +`resolveDestinations()` / `useStaticDestinations()`, which already merges +`pluginRegistry.getNavRegistrations()` in. The only place a `NavItem.section` +value is translated into that internal `NavSection` is +`pluginDestinations()` in `apps/web/lib/navigation/plugin-destinations.ts`, and +it does not have an `"insights"` case yet — an item with `section: "insights"` +today falls through to `"plugins"`. + +So this is a narrow change: widen the `NavItem.section` union, add one branch +to `pluginDestinations()`, correct the six stale comments the spec's **Stale +prose at the edit sites** section names (they sit next to the lines being +touched and already contradict the widened contract), and add regression +coverage for the behavior this unlocks. No sidebar, footer, mobile-menu, or +palette component changes are needed — they inherit the new placement for +free because they were already built generically. No backend change: nav-item +sections are a frontend-only concept (`apps/backend/internal/plugins/manifest/validate.go` +validates an unrelated enum). + +Order: (1) widen the contract and fix the mapping plus its adjacent stale +prose, since every other file depends on this being correct; (2) add +frontend regression tests proving the existing generic components render the +new placement correctly, including the ordering and capacity guarantees the +spec states as contract; (3) add a real end-to-end plugin registering an +`insights` item and prove it on both a desktop and a phone viewport. (2) and +(3) touch disjoint files and can run in parallel once (1) lands. + +--- + +## Frontend + +### Contract — `apps/web/lib/plugins/types.ts` + +Widen `NavItem.section` from `"main" | "settings" | "integrations"` to add +`"insights"`, and extend the field's doc comment to describe all four values +(today only `"main"` and `"integrations"` are documented on this field — +`"settings"` is undocumented; the spec's stale-prose item 4 requires fixing +this in the same change): `"insights"` renders in the sidebar footer icon row +and the phone Utilities group, `"settings"` is accepted and renders nowhere. +`PluginNavRegistration` (`apps/web/lib/plugins/registry.ts`) extends `NavItem` +and needs no edit — it inherits the widened union. + +### Mapping — `apps/web/lib/navigation/plugin-destinations.ts` + +`pluginDestinations()`'s `section` mapping currently reads: + +```ts +section: item.section === "integrations" ? ("integrations" as const) : ("plugins" as const), +``` + +Add the `"insights"` case so the ternary becomes a three-way mapping: +`"integrations"` → `"integrations"`, `"insights"` → `"insights"`, anything +else (`"main"`, omitted, or an unrecognised string) → `"plugins"`. +`"settings"` items are already filtered out before this line and stay +filtered out. + +Also correct, in the same file: +- The docblock above `pluginDestinations()` (stale-prose item 2): it currently + says `"settings"` items "render in the settings tree's `PluginSlot`" — false; + that slot is fed only by `registerComponent("settings-nav", …)`. Replace + with: settings-section items are skipped and render on no surface. +- The comment above `surfaces: SIDEBAR_AND_MENU` (stale-prose item 3): it cites + `registerShortcut`, which does not exist in this repository. Replace with a + statement that plugin destinations never declare the `palette` surface, + without naming a substitute API. + +### Docs — `docs/plans/plugins/PLUGIN-API.md` and `docs/public/plugins-authoring.md` + +- `PLUGIN-API.md` (stale-prose item 1): update the `NavItem` interface + Before/After snippet's `section` union to include `"insights"`, replace the + false "Hosts predating a section value simply don't render items targeting + it (additive change)" comment with the actual degrade-to-`"main"` rule, and + add a line stating where `"insights"` renders. +- `docs/public/plugins-authoring.md` line 184 (stale-prose item 5, the one + requirement rather than correction in this list): the `registerNavItem` row + of the Frontend hook/API matrix currently reads *"section is main, + integrations, or accepted-but-not-rendered settings"*. Add `insights` and + state where it renders. + +These three files plus `types.ts` are, per the spec's **API surface** section, +the complete contract-and-docs edit set. + +--- + +## Tests + +### `apps/web/lib/navigation/plugin-destinations.test.ts` + +This is the sole module that reads `NavItem.section`, so it carries the +mapping-level coverage: + +- **What:** a `section: "insights"` item routes to the `insights` `NavSection` + on both `sidebar` and `mobileMenu` surfaces. **How:** extend the existing + `pluginItems` fixture with an `insights`-section item and add a case + alongside the existing `"main"`/`"integrations"`/`"settings"` assertions. +- **What:** an `insights` item does not also appear in the `plugins` section + resolution (no double placement — the spec's "moves, does not add" rule). + **How:** assert `pluginsIn("sidebar" | "mobileMenu", …)` excludes it, mirroring + the existing `"routes main-section items…"` test. +- **What:** an `insights` item stays off the palette, like every other + section. **How:** extend the existing `"keeps plugin items off the + palette…"` test's fixture with an `insights` item and assert its id is + absent; retitle the test per stale-prose item 6 (it currently claims + plugins reach the palette "via shortcuts", which is false — there is no + plugin route into the palette by any API). +- **What:** an unrecognised `section` string (e.g. `"footer"`) degrades to + `plugins`, never to `insights` and never dropped. **How:** new test, one + `pluginItems` entry with an out-of-union string cast, asserted present in + `pluginsIn(...)` and absent from an `insights`-section resolution. +- **What:** two plugins each registering one `insights` item keep distinct + destination ids and render in the order they were supplied. **How:** new + test mirroring the existing `"keeps two plugins that pick the same item id + apart"` test, using `section: "insights"`. + +### `apps/web/lib/navigation/resolve-destinations.test.ts` + +- **What:** the first-party `stats` destination precedes a plugin `insights` + item when the sidebar resolves the `insights` section. **How:** new test, + `resolveDestinations({ surface: "sidebar", section: "insights", pluginItems: […] })`, + assert id order `["stats", "plugin:acme:board"]`. +- **What:** the phone Utilities group's manifest rows resolve as `stats`, + `settings`, then the plugin `insights` item — the plugin row lands after + Settings, not adjacent to Stats (spec **Ordering** section, phone scenario). + **How:** new test extending the existing `"accepts several sections…"` test + with a plugin `insights` item passed via `pluginItems`, surface `mobileMenu`, + section `MOBILE_MENU_UTILITY_SECTIONS`. + +### `apps/web/components/plugins/plugin-nav-items.test.tsx` (desktop plugin rail) + +- **What:** an `insights`-section item is excluded from the plugin rail, like + `"settings"` and `"integrations"` already are. **How:** new test + `"keeps insights items out of the main plugin rail"`, following the existing + `"omits a nav item registered for a non-main section"` pattern. + +### `apps/web/components/plugins/mobile-plugin-nav-section.test.tsx` (phone Plugins group) + +- **What:** an `insights`-section item is excluded from the phone Plugins + group, matching the desktop rail (spec scenario: "moves, does not add" holds + on both surfaces). **How:** extend the existing + `"omits non-main sections…"` test with an `insights`-section registration + alongside the existing `integrations`/`settings` ones, asserting the + rendered output still excludes it (the test currently asserts the whole + section is empty because its two fixture items are `integrations`/`settings` + — verify the assertion still holds, or split into a dedicated case if adding + a third non-empty-triggering item would change what's asserted). + +### `apps/web/components/app-sidebar/app-sidebar-footer.test.tsx` (desktop footer) + +The file already mocks `useStaticDestinations` to return a fixed `stats`-only +array (a deliberate seam: the manifest's own resolution is covered above, not +re-tested here). Add: + +- **What:** a plugin `insights` destination renders as an icon button with + accessible name = `NavItem.label`, `data-testid="sidebar-plugin::-button"` + (falls out of the existing `sidebar-${destination.id}-button` templating + once `destination.id` is the namespaced plugin id — no component change), + and clicking it calls `router.push(destination.href)`. **How:** new test, + mock `useStaticDestinations` to return `[stats, { id: "plugin:acme:board", label: "Acme Board", icon: IconChartBar, section: "insights", href: "/plugins/acme", source: "plugin", pluginItemId: "board" }]`. +- **What:** capacity guarantee — eight plugin `insights` destinations all + render into the DOM with no cap, and the footer container carries its + wrapping-row class when expanded and its column class when collapsed (spec + **Capacity and overflow**). **How:** new test(s), mock eight synthetic + plugin destinations, assert `getAllByTestId(/^sidebar-plugin:/)` has length + 8 for both `collapsed={false}` and `collapsed={true}` renders, and assert + the container's class list matches the `flex-wrap` / `flex-col` expectation + already encoded in `AppSidebarFooter`'s className logic. + +### `apps/web/components/navigation/app-nav-sheet.test.tsx` (`AppNavSections` / phone Utilities group) + +The file currently mocks `usePluginRegistry` to always return +`getNavRegistrations: () => []`. Change that mock to read a mutable +module-level array (reset in `beforeEach`) so individual tests can inject +registrations, then add: + +- **What:** the phone Utilities group renders `Stats`, then `Settings`, then a + plugin `insights` row, in that order (spec **Ordering**, phone scenario) — + and the Status/theme/Improve Kandev/Health rows around them are unaffected. + **How:** new test in the `describe("AppNavSections", …)` block, set the + mutable registrations array to one `{ pluginId: "acme", id: "board", label: + "Acme Board", path: "/plugins/acme", section: "insights" }`, render + ``, assert the relative DOM order of the three `Stats`, + `Settings`, `Acme Board` links via their positions among + `getAllByRole("link")`. + +--- + +## E2E Tests + +The shared Playwright fixture plugin (`apps/backend/cmd/plugin-fixture`, +packaged as `kandev-plugin-e2e`) registers one `section: "main"` nav item +today (`id: "e2e-hello"`, `apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js`). +Add a second `registerNavItem` call there with `section: "insights"` so real +install/boot/render exercises the new placement, then repackage +(`make -C apps/backend e2e-plugin-package`, which writes +`apps/backend/.build/kandev-plugin-e2e-1.0.0.tar.gz` — the same artifact every +existing plugin e2e spec installs from). + +```js +registry.registerNavItem({ + id: "e2e-insights-tools", + label: "E2E Insights Tools", + path: "/plugins/e2e-hello", + section: "insights", +}); +``` + +Reuses the existing route/page (`/plugins/e2e-hello`) — this is a second nav +entry point into the same fixture page, not a new page, keeping the fixture's +existing route/component untouched. + +- **Scenario:** installing the plugin and reloading shows its `insights` item + as a footer icon button, and it does **not** also appear in the plugin rail. + **File:** `apps/web/e2e/tests/plugins/plugins.spec.ts` (extend the existing + `test.describe` block; reuse its `openInstallDialog`/`uploadPackage` helpers + and the file's shared `afterEach` uninstall). **What to verify:** + `testPage.getByTestId("sidebar-plugin:kandev-plugin-e2e:e2e-insights-tools-button")` + is visible with accessible name `E2E Insights Tools`, clicking it navigates + to `/plugins/e2e-hello`, and + `testPage.getByTestId("plugin-nav-item-e2e-insights-tools")` has count 0. +- **Scenario:** on a phone, the same item appears as a labelled row in the + Utilities group and does **not** appear in the Plugins group. **File:** + `apps/web/e2e/tests/plugins/mobile-plugin-nav.spec.ts` (new test in the + existing `test.describe("Mobile plugin navigation", …)` block, following + the file's existing inline install pattern). **What to verify:** after + install + reload + opening the menu sheet, a row with accessible name + `E2E Insights Tools` is present and is not inside + `getByTestId("mobile-plugin-nav-section")` (the Plugins group container). + +--- + +## Verification Results + +**Task 01 (contract and mapping) — done.** `pnpm --filter @kandev/web test -- +lib/navigation/plugin-destinations.test.ts +lib/navigation/resolve-destinations.test.ts` → 2 files, 25 tests passed. +`pnpm run typecheck` (apps/web) → clean. `pnpm --filter @kandev/web lint -- +` → 0 errors/warnings. `pnpm run i18n:ratchet` → clean. See +task-01's `## Results` for detail. + +**Task 02 (frontend regression coverage) — done.** `pnpm --filter @kandev/web +test -- <6 touched test files>` → 71 tests passed across 6 files. Typecheck, +lint, and i18n:ratchet all clean. No production component code changed — the +existing generic manifest-driven rendering absorbed the new section without +edits. See task-02's `## Results` for detail. + +**Task 03 (E2E coverage) — done.** Rebuilt the e2e fixture plugin package and +the `kandev`/`mock-agent` binaries and web `dist/` this worktree lacked, then +ran the real Playwright suites: `plugins.spec.ts` (chromium) 12/12 passed, +`mobile-plugin-nav.spec.ts` (mobile-chrome) 5/5 passed, and +`plugins-docs-screenshots.spec.ts` 2 skipped (gated, pre-existing, confirms no +crash loading the new fixture bundle). See task-03's `## Results` for detail. + +All three tasks are done. Plan status should move from `draft` to +`complete` once this is committed. + +--- + +## Implementation Waves And Parallel Candidates + +``` +Wave 1: +- [x] [task-01-contract-and-mapping](task-01-contract-and-mapping.md) + +Wave 2 (parallel candidates — user authorization required): +- [x] [task-02-frontend-regression-coverage](task-02-frontend-regression-coverage.md) +- [x] [task-03-e2e-coverage](task-03-e2e-coverage.md) +``` + +Wave 2 tasks touch disjoint files (unit test files under `apps/web/components` +and `apps/web/lib` for task 02; the Go fixture bundle, its packaged artifact, +and two e2e spec files for task 03) and both depend only on task 01's mapping +change landing first. + +## Open Questions + +(None — the spec's **Section-to-placement mapping** table and **Ordering** +section leave no implementation choice open.) diff --git a/docs/plans/plugin-nav-sidebar-footer/task-01-contract-and-mapping.md b/docs/plans/plugin-nav-sidebar-footer/task-01-contract-and-mapping.md new file mode 100644 index 0000000000..cae32b1c38 --- /dev/null +++ b/docs/plans/plugin-nav-sidebar-footer/task-01-contract-and-mapping.md @@ -0,0 +1,199 @@ +--- +id: "01-contract-and-mapping" +title: "Contract and mapping" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/plugin-nav-sidebar-footer/spec.md" +--- + +# Task 01: Contract and mapping + +Widen the `NavItem.section` contract to accept `"insights"`, teach +`pluginDestinations()` to route it to the `insights` `NavSection`, and correct +the stale prose the spec's **Stale prose at the edit sites** section names +(items 1–4 and 6 are corrections; item 5 is the one net-new documentation +requirement). This is the only functional-behavior change in the whole +feature — every rendering surface already reads `NavSection` generically and +needs no edit (see `plan.md`'s Overview). + +## Files to change + +1. `apps/web/lib/plugins/types.ts` — `NavItem.section`: + ```ts + section?: "main" | "settings" | "integrations"; + ``` + becomes + ```ts + section?: "main" | "settings" | "integrations" | "insights"; + ``` + and its doc comment (currently documents only `"main"` and `"integrations"`) + is extended to cover all four: `"insights"` renders in the sidebar footer + icon row and the phone Utilities group; `"settings"` is accepted and + renders on no surface. + +2. `docs/plans/plugins/PLUGIN-API.md` (around line 323–327) — the `NavItem` + snippet's `section` union gains `"insights"`, matching step 1. The comment + block above it currently reads: + ``` + // section: "main" (default) renders as a top-level sidebar entry; + // "integrations" renders inside the sidebar's Integrations section alongside + // the first-party integration links (GitHub, Jira, ...). Hosts predating a + // section value simply don't render items targeting it (additive change). + ``` + The last sentence is false — the shipped host drops nothing; an + unrecognised value degrades to `"main"`'s placement. Replace it with that + degrade-to-`"main"` rule, and add a line for where `"insights"` renders + (sidebar footer icon row / phone Utilities group). + +3. `docs/public/plugins-authoring.md` line 184 — the `registerNavItem` row of + the Frontend hook/API matrix currently reads *"section is main, + integrations, or accepted-but-not-rendered settings"*. Add `insights` to + the list and state it renders in the sidebar footer icon row and the phone + Utilities group. + +4. `apps/web/lib/navigation/plugin-destinations.ts`: + - The `section` mapping line inside `pluginDestinations()`: + ```ts + section: item.section === "integrations" ? ("integrations" as const) : ("plugins" as const), + ``` + becomes a three-way mapping — `"integrations"` → `"integrations"`, + `"insights"` → `"insights"`, anything else (including omitted or an + unrecognised string) → `"plugins"`. `"settings"` items are already + filtered out one line above this mapping (`.filter((item) => + (item.section ?? "main") !== "settings")`) and stay filtered. + - The docblock directly above `pluginDestinations()`: + ``` + /** + * `section: "settings"` items are skipped — those render in the settings tree's + * `PluginSlot`, not as destinations. + */ + ``` + The second clause is false: that slot is fed only by + `registerComponent("settings-nav", …)`. Replace with: settings-section + items are skipped and render on no surface. + - The comment above `surfaces: SIDEBAR_AND_MENU`: + ``` + // Plugins reach the palette through `registerShortcut`, not as nav + // commands, so plugin destinations stay off that surface. + ``` + `registerShortcut` does not exist anywhere in this repository. Replace + with a statement that plugin destinations never declare the `palette` + surface, without naming a substitute API (there is none — see spec + **What**). + +5. `apps/web/lib/navigation/plugin-destinations.test.ts`: + - Retitle the test currently named + `"keeps plugin items off the palette, which plugins reach via shortcuts"` + to drop the false trailing clause (same falsehood as the `registerShortcut` + comment above). The assertion body does not change. + - Extend the `pluginItems` fixture with an `insights`-section entry and add + assertions: it resolves to the `insights` section on both `sidebar` and + `mobileMenu`; it is absent from `pluginsIn(...)` (`"plugins"` section) + results on both surfaces; it is absent from the palette result (extend the + retitled test's fixture and assertion). + - Add a test for an unrecognised `section` string (e.g. a value not in the + union, cast through the fixture): it appears in `pluginsIn(...)` and is + absent from an `insights`-section resolution. + - Add a test mirroring the existing "keeps two plugins that pick the same + item id apart" case, using `section: "insights"` for both registrations. + +6. `apps/web/lib/navigation/resolve-destinations.test.ts` — add: + - A test that `resolveDestinations({ surface: "sidebar", section: + "insights", pluginItems: [...] })` orders the first-party `stats` entry + before a plugin `insights` entry. + - A test that resolving `surface: "mobileMenu", section: + MOBILE_MENU_UTILITY_SECTIONS` with a plugin `insights` item in + `pluginItems` returns ids in the order `["stats", "settings", + "plugin:acme:board"]` — the plugin row lands after `settings`, not + adjacent to `stats` (spec **Ordering**, phone scenario). + +## Acceptance + +- `NavItem.section` accepts `"insights"` in `types.ts`, mirrored in + `PLUGIN-API.md`'s Before/After snippet. +- `pluginDestinations()` maps `section: "insights"` items to the `insights` + `NavSection` and nothing else changes about `"main"`/`"integrations"`/ + `"settings"`/unrecognised-string handling. +- All six stale-prose locations from the spec's **Stale prose at the edit + sites** list read correctly (five corrections plus the one documentation + requirement in `plugins-authoring.md`). +- New and updated tests in `plugin-destinations.test.ts` and + `resolve-destinations.test.ts` pass. + +## Verification + +``` +cd apps && pnpm --filter @kandev/web test -- lib/navigation/plugin-destinations.test.ts lib/navigation/resolve-destinations.test.ts +cd apps/web && pnpm run typecheck +``` + +## Files likely touched + +- `apps/web/lib/plugins/types.ts` +- `docs/plans/plugins/PLUGIN-API.md` +- `docs/public/plugins-authoring.md` +- `apps/web/lib/navigation/plugin-destinations.ts` +- `apps/web/lib/navigation/plugin-destinations.test.ts` +- `apps/web/lib/navigation/resolve-destinations.test.ts` + +## Dependencies + +None. + +## Parallelism + +sequential (wave 1 — task 02 and task 03 both depend on this landing first). + +## Inputs + +- Spec sections: **What**, **API surface** (including **Stale prose at the + edit sites** and **Section-to-placement mapping**), **Ordering**. +- Existing pattern: the `"integrations"` branch already in + `pluginDestinations()` is the template for the new `"insights"` branch. + +## Output contract + +Summary of the mapping change and every stale-prose correction made, the +files changed, exact test commands run and their outcomes, and any blockers +or risks (e.g. if a consumer outside this list is found to depend on the old +three-value union). Update this file's `status` to `done` and this plan's +Wave 1 checkbox and **Verification Results** section in the same change. + +## Results + +Widened `NavItem.section` to `"main" | "settings" | "integrations" | "insights"` +in `apps/web/lib/plugins/types.ts` (doc comment extended to cover all four +values). Mirrored the union and the degrade-to-`"main"` rule in +`docs/plans/plugins/PLUGIN-API.md`. Added `insights` to the `registerNavItem` +row of `docs/public/plugins-authoring.md`'s Frontend hook/API matrix. + +`pluginDestinations()` in `apps/web/lib/navigation/plugin-destinations.ts` now +routes `section` through a new `sectionFor()` helper: `"integrations"` → +`"integrations"`, `"insights"` → `"insights"`, everything else → `"plugins"`. +Corrected the docblock above `pluginDestinations()` (settings items render on +no surface, not in a `PluginSlot`) and the comment above `surfaces: +SIDEBAR_AND_MENU` (no `registerShortcut` API exists; plugin destinations never +declare `palette`). + +Retitled `plugin-destinations.test.ts`'s palette test to +`"keeps plugin items off the palette"` (dropped the false "which plugins reach +via shortcuts" clause) and extended it to also assert an insights item is +excluded. Added: `insightsIn()` helper; a test routing insights items to the +insights section on both surfaces; a "moves, does not add" test against the +plugins group; an unrecognised-section-string degrade test; and an +insights-section two-plugin ordering/identity test. Added two tests to +`resolve-destinations.test.ts` (in a new `describe("resolveDestinations with +plugin items", …)` block, split out to stay under the 100-line +`max-lines-per-function` limit) proving `stats` precedes a plugin `insights` +item on the sidebar, and the mobile utility group orders `stats`, `settings`, +then the plugin item. + +Commands run (from `apps/`): +- `pnpm --filter @kandev/web test -- lib/navigation/plugin-destinations.test.ts lib/navigation/resolve-destinations.test.ts` — 2 files, 25 tests passed. +- `cd web && pnpm run typecheck` — passed, no errors. +- `pnpm --filter @kandev/web lint -- lib/navigation/plugin-destinations.ts lib/navigation/plugin-destinations.test.ts lib/navigation/resolve-destinations.test.ts lib/plugins/types.ts` — 0 errors, 0 warnings (fixed two `sonarjs`/`max-lines-per-function` warnings found on first pass: extracted an `ACME_BOARD_ID` constant, split the new plugin-item tests into their own `describe` block). +- `pnpm run i18n:ratchet` — clean (no new hardcoded strings; 2 modified files, 0 added). + +Files touched matched **Files likely touched** exactly. No blockers. diff --git a/docs/plans/plugin-nav-sidebar-footer/task-02-frontend-regression-coverage.md b/docs/plans/plugin-nav-sidebar-footer/task-02-frontend-regression-coverage.md new file mode 100644 index 0000000000..169bb09576 --- /dev/null +++ b/docs/plans/plugin-nav-sidebar-footer/task-02-frontend-regression-coverage.md @@ -0,0 +1,167 @@ +--- +id: "02-frontend-regression-coverage" +title: "Frontend regression coverage" +status: done +wave: 2 +depends_on: ["01-contract-and-mapping"] +plan: "plan.md" +spec: "../../specs/plugin-nav-sidebar-footer/spec.md" +--- + +# Task 02: Frontend regression coverage + +Prove, with component-level tests, that the desktop plugin rail, the phone +Plugins group, the desktop sidebar footer, and the phone Utilities group all +render `section: "insights"` plugin items correctly — placement, exclusion +from the `plugins` surfaces, ordering, test id, and the no-cap capacity +guarantee — without any component code change (they already read the +navigation manifest generically; task 01 is what makes the manifest resolve +`"insights"` correctly). No production code in `apps/web/components/**` +should need to change for this task; if you find one that does, that is a +signal task 01 missed something and the plan needs revisiting before +continuing here. + +## Files to change + +1. `apps/web/components/plugins/plugin-nav-items.test.tsx` (desktop rail) — + add `it("keeps insights items out of the main plugin rail", ...)`, + following the existing `"omits a nav item registered for a non-main + section"` pattern (register one item with `section: "insights"`, assert no + `plugin-nav-item-` testid renders). + +2. `apps/web/components/plugins/mobile-plugin-nav-section.test.tsx` (phone + Plugins group) — extend the existing test + `"omits non-main sections: integrations belongs to MobileIntegrationsSection, + and settings is rendered nowhere"` with a third registration, + `section: "insights"`, and confirm the assertion (`container.innerHTML` is + empty) still holds with all three non-main sections registered. If adding + the third registration changes what the test needs to assert, split it into + a dedicated `it("also excludes insights-section items", ...)` case instead + of forcing it into the existing one. + +3. `apps/web/components/app-sidebar/app-sidebar-footer.test.tsx` (desktop + footer) — the file mocks `useStaticDestinations` to return a fixed + `stats`-only array; add: + - A test that a plugin `insights` destination in that mocked array renders + as an icon button with `data-testid="sidebar-plugin:acme:board-button"` + (built from `sidebar-${destination.id}-button`, where `destination.id` is + already the namespaced `plugin:acme:board` — no template change needed), + accessible name equal to the destination's `label`, and that clicking it + calls the mocked router's `push` with the destination's `href`. + - A test (or two, expanded + collapsed) asserting the **Capacity and + overflow** guarantee: mock `useStaticDestinations` to return `stats` plus + eight synthetic plugin `insights` destinations, render `AppSidebarFooter` + with `collapsed={false}`, assert eight `sidebar-plugin:*-button` testids + are present (`getAllByTestId` on a regex-matched selector or by iterating + the known ids) and the footer's outer container carries the wrapping-row + classes it already applies when expanded. Render again with + `collapsed={true}` and assert the same eight buttons are present with the + column classes applied instead. Do not assert anything about visible + height/clipping — the spec explicitly does not guarantee that (see + **Capacity and overflow** / **Out of scope**). + +4. `apps/web/components/navigation/app-nav-sheet.test.tsx` (`AppNavSections`, + phone Utilities group) — the file currently mocks `usePluginRegistry` as + `() => ({ getNavRegistrations: () => [] })`. Change this to read a mutable + module-level array (e.g. `let navRegistrations: PluginNavRegistration[] = + []`, reset to `[]` in the existing `beforeEach` blocks), then add a test in + `describe("AppNavSections", ...)`: + - Set `navRegistrations` to one `{ pluginId: "acme", id: "board", label: + "Acme Board", path: "/plugins/acme", section: "insights" }`, render + ``, and assert the relative order of `Stats`, `Settings`, + and `Acme Board` among `screen.getAllByRole("link")` matches that + sequence (spec **Ordering**, phone scenario — the plugin row follows + `Settings`, not `Stats`). Do not assert the full role-name sequence + including Status/theme/Improve Kandev/Health — those are non-manifest + controls the spec explicitly says a conformance test must not fail on. + +## Acceptance + +- All four test files above pass with the new/extended cases. +- No file under `apps/web/components/**` other than the test files listed + changes. If one does, stop and reconcile with task 01 / the plan before + proceeding — that would mean a surface is not as generic as `plan.md`'s + Overview assumes. + +## Verification + +``` +cd apps && pnpm --filter @kandev/web test -- components/plugins/plugin-nav-items.test.tsx components/plugins/mobile-plugin-nav-section.test.tsx components/app-sidebar/app-sidebar-footer.test.tsx components/navigation/app-nav-sheet.test.tsx +``` + +## Files likely touched + +- `apps/web/components/plugins/plugin-nav-items.test.tsx` +- `apps/web/components/plugins/mobile-plugin-nav-section.test.tsx` +- `apps/web/components/app-sidebar/app-sidebar-footer.test.tsx` +- `apps/web/components/navigation/app-nav-sheet.test.tsx` + +## Dependencies + +01-contract-and-mapping (needs `pluginDestinations()` to resolve `"insights"` +correctly before any of these assertions can pass). + +## Parallelism + +parallel-safe with 03-e2e-coverage — disjoint files (this task touches only +`apps/web/components/**` test files; task 03 touches the Go fixture bundle, +its packaged artifact, and Playwright spec files under `apps/web/e2e/**`). + +## Inputs + +- Spec **Scenarios** section (the desktop-rail-exclusion, phone-group-order, + and eight-plugin capacity scenarios map directly to the tests above). +- Spec **Ordering** section's "What order means" subsection — a conformance + test must constrain only the manifest rows' relative order, not the full + visible row sequence. +- Existing patterns: `plugin-nav-items.test.tsx`'s `"omits a nav item + registered for a non-main section"`, `mobile-plugin-nav-section.test.tsx`'s + non-main-section test, `app-sidebar-footer.test.tsx`'s `useStaticDestinations` + mock, `app-nav-sheet.test.tsx`'s `SectionsHost` helper. + +## Output contract + +Summary of every test added/changed, files touched, exact commands run with +outcomes, and any surface found to be non-generic (a blocker requiring a plan +update). Update this file's `status` to `done` and this plan's Wave 2 +checkbox and **Verification Results** section in the same change. + +## Results + +No production code under `apps/web/components/**` changed — confirmed the +plan's assumption that every surface already reads the manifest generically. + +- `plugin-nav-items.test.tsx`: added `"keeps insights items out of the main + plugin rail"`. +- `mobile-plugin-nav-section.test.tsx`: added `"also excludes insights-section + items: they belong to the Utilities group, not here"` (kept as a dedicated + case rather than folding into the existing non-main-sections test, per the + task's fallback instruction). +- `app-sidebar-footer.test.tsx`: changed the `useStaticDestinations` mock from + a fixed array to a mutable `insightDestinations` (reset in + `resetFooterState`), added `renderFooter(collapsed)` parameter, and added a + new `describe("AppSidebarFooter plugin insights items", …)` block: testid + + accessible name + click-navigates for one plugin destination, and two + capacity tests (8 synthetic plugin destinations, expanded/`flex-wrap` and + collapsed/`flex-col`, all 8 testids present in both). +- `app-nav-sheet.test.tsx`: changed the `usePluginRegistry` mock to read a + mutable `navRegistrations` array (reset in all three `beforeEach` blocks), + added a test asserting the Utilities group's manifest-row order is `Stats` + → `Settings` → plugin `insights` item. + +Commands run (from `apps/`): +- `pnpm --filter @kandev/web test -- components/plugins/plugin-nav-items.test.tsx components/plugins/mobile-plugin-nav-section.test.tsx components/app-sidebar/app-sidebar-footer.test.tsx` — 3 files, 34 tests passed. +- `pnpm --filter @kandev/web test -- components/navigation/app-nav-sheet.test.tsx` — 1 file, 12 tests passed. +- `cd web && pnpm run typecheck` — passed. +- `pnpm --filter @kandev/web lint -- <8 touched files>` — 0 errors, 0 warnings after fixing the two warnings surfaced in task 01's files. +- `pnpm run i18n:ratchet` (from `apps/web`) — clean. +- Combined rerun: `pnpm --filter @kandev/web test -- lib/navigation/plugin-destinations.test.ts lib/navigation/resolve-destinations.test.ts components/plugins/plugin-nav-items.test.tsx components/plugins/mobile-plugin-nav-section.test.tsx components/app-sidebar/app-sidebar-footer.test.tsx components/navigation/app-nav-sheet.test.tsx` — 6 files, 71 tests passed. + +One tooling note, not a code blocker: running `pnpm run test -- ` via +`cd web && pnpm run test -- ...` (as opposed to `pnpm --filter @kandev/web +test -- ...` from `apps/`) caused pnpm to forward a literal `--` into the +vitest invocation, which made vitest ignore the file filters and run the +entire suite (~15 min before it was killed). Use the `pnpm --filter @kandev/web +test -- ` form from `apps/` for targeted runs. + +Files touched matched **Files likely touched** exactly. No blockers. diff --git a/docs/plans/plugin-nav-sidebar-footer/task-03-e2e-coverage.md b/docs/plans/plugin-nav-sidebar-footer/task-03-e2e-coverage.md new file mode 100644 index 0000000000..e579d7b3fe --- /dev/null +++ b/docs/plans/plugin-nav-sidebar-footer/task-03-e2e-coverage.md @@ -0,0 +1,228 @@ +--- +id: "03-e2e-coverage" +title: "E2E coverage" +status: done +wave: 2 +depends_on: ["01-contract-and-mapping"] +plan: "plan.md" +spec: "../../specs/plugin-nav-sidebar-footer/spec.md" +--- + +# Task 03: E2E coverage + +Prove the `insights` placement end to end through the real plugin install/boot +path: a real plugin registers an `insights` nav item, the real backend boots +it, and the real frontend renders it as a footer icon on desktop and a +Utilities row on a phone. Reuses the existing shared e2e fixture plugin +(`kandev-plugin-e2e`) and its existing `/plugins/e2e-hello` route/page — this +task adds a second nav-item registration pointing at that same page, not a +new page. + +## Files to change + +1. `apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js` — directly + after the existing: + ```js + registry.registerNavItem({ + id: "e2e-hello", + label: "Hello E2E", + path: "/plugins/e2e-hello", + section: "main", + }); + registry.registerRoute("/plugins/e2e-hello", PluginPage); + ``` + add a second registration: + ```js + registry.registerNavItem({ + id: "e2e-insights-tools", + label: "E2E Insights Tools", + path: "/plugins/e2e-hello", + section: "insights", + }); + ``` + No new route: both nav items point at the existing `PluginPage`. + +2. Repackage the fixture so the tar.gz every e2e spec installs from includes + the new bundle: + ``` + cd apps/backend && make e2e-plugin-package + ``` + This overwrites `apps/backend/.build/kandev-plugin-e2e-1.0.0.tar.gz`. Do + not hand-edit that file; only edit `bundle.js` and rebuild through the + Makefile target (see the target's comment in `apps/backend/Makefile` for + what it stages/builds/packs). + +3. `apps/web/e2e/tests/plugins/plugins.spec.ts` — add one new `test(...)` + inside the existing `test.describe("Plugins — gRPC plugin + install/load/live-update/uninstall", ...)` block (its `afterEach` already + uninstalls via API, so a fresh test here does not need its own cleanup). + Reuse the file's `openInstallDialog`/`uploadPackage` helpers: + ```ts + test("registers an insights-section item as a footer icon, not a rail row", async ({ testPage }) => { + await openInstallDialog(testPage); + await uploadPackage(testPage, PACKAGE_PATH); + await expect(testPage.getByTestId(`plugin-row-${PLUGIN_ID}`)).toBeVisible({ timeout: 15_000 }); + await testPage.goto("/"); + await testPage.reload(); + + const footerButton = testPage.getByTestId( + `sidebar-plugin:${PLUGIN_ID}:e2e-insights-tools-button`, + ); + await expect(footerButton).toBeVisible({ timeout: 15_000 }); + await expect(footerButton).toHaveAccessibleName("E2E Insights Tools"); + await footerButton.click(); + await expect(testPage).toHaveURL(/\/plugins\/e2e-hello$/); + + await expect(testPage.getByTestId("plugin-nav-item-e2e-insights-tools")).toHaveCount(0); + }); + ``` + Adjust the exact locator/assertion API to match what the rest of the file + already uses (e.g. confirm `toHaveAccessibleName` is used elsewhere or use + the row's `getByRole("button", { name: "E2E Insights Tools" })` form + instead, matching this repo's existing footer-button assertions in + `app-sidebar-footer.test.tsx`). + +4. `apps/web/e2e/tests/plugins/mobile-plugin-nav.spec.ts` — add one new + `test(...)` inside the existing `test.describe("Mobile plugin + navigation", ...)` block, following the file's existing inline install + pattern (goto `/settings/plugins`, install via upload tab, wait for the + plugin row, `goto("/")`, `reload()`, open the menu sheet): + ```ts + test("shows the insights item in the Utilities group, not the Plugins group", async ({ testPage }) => { + 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(); + await testPage.getByRole("button", { name: "Open menu" }).click(); + + const utilitiesRow = testPage.getByRole("link", { name: "E2E Insights Tools" }); + await expect(utilitiesRow).toBeVisible(); + + const pluginsGroup = testPage.getByTestId("mobile-plugin-nav-section"); + await expect(pluginsGroup.getByText("E2E Insights Tools")).toHaveCount(0); + }); + ``` + The file's existing `afterEach` already deletes the plugin via API, so no + extra cleanup is needed. Adjust the exact "open menu" locator to match + whatever the file's existing tests already use for that button. + +## Acceptance + +- `apps/backend/.build/kandev-plugin-e2e-1.0.0.tar.gz` is rebuilt and contains + the second nav-item registration (verify via + `tar -xOzf apps/backend/.build/kandev-plugin-e2e-1.0.0.tar.gz ui/bundle.js | grep e2e-insights-tools` + or equivalent). +- The new desktop test in `plugins.spec.ts` and the new mobile test in + `mobile-plugin-nav.spec.ts` pass. +- No existing test in either file (or in `plugins-docs-screenshots.spec.ts`, + which references the same fixture by `NAV_ITEM_ID`/`PLUGIN_ROUTE`) breaks — + the new registration is additive and does not change `e2e-hello`'s id, + label, or route. + +## Verification + +``` +cd apps/backend && make e2e-plugin-package +cd apps/web && pnpm install --frozen-lockfile && pnpm e2e:raw e2e/tests/plugins/plugins.spec.ts e2e/tests/plugins/mobile-plugin-nav.spec.ts e2e/tests/plugins/plugins-docs-screenshots.spec.ts +``` + +## Files likely touched + +- `apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js` +- `apps/backend/.build/kandev-plugin-e2e-1.0.0.tar.gz` (generated artifact, + rebuilt not hand-edited) +- `apps/web/e2e/tests/plugins/plugins.spec.ts` +- `apps/web/e2e/tests/plugins/mobile-plugin-nav.spec.ts` + +## Dependencies + +01-contract-and-mapping (the fixture's `section: "insights"` registration only +renders in the footer/Utilities group once `pluginDestinations()` maps it +there). + +## Parallelism + +parallel-safe with 02-frontend-regression-coverage — disjoint files (this task +touches the Go fixture bundle, its packaged artifact, and Playwright spec +files under `apps/web/e2e/**`; task 02 touches only unit test files under +`apps/web/components/**`). + +## Inputs + +- Spec **Scenarios**: the desktop footer/rail-exclusion scenario and the + phone Utilities/Plugins-group scenario. +- Spec **Rendered identity**: the footer testid format + `sidebar-plugin::-button`, and that phone Utilities rows + carry no `data-testid` (select by visible label there, as the new mobile + test does). +- `apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js`'s existing + `e2e-hello` registration as the direct template. +- `apps/web/e2e/tests/plugins/plugins.spec.ts`'s `openInstallDialog`/ + `uploadPackage` helpers and shared `afterEach`. +- `apps/web/e2e/tests/plugins/mobile-plugin-nav.spec.ts`'s existing inline + install pattern (no shared helper in that file today). + +## Output contract + +Summary of the fixture change, the repackage step's output, both new tests, +files touched, exact commands run with outcomes (including the `tar` +verification), and any blockers (e.g. a local platform-specific build issue +with `make e2e-plugin-package`). Update this file's `status` to `done` and +this plan's Wave 2 checkbox and **Verification Results** section in the same +change. + +## Results + +Added a second `registry.registerNavItem({ id: "e2e-insights-tools", label: +"E2E Insights Tools", path: "/plugins/e2e-hello", section: "insights" })` call +to `apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js`, right after +the existing `e2e-hello` registration, reusing the same `PluginPage`/route. + +Rebuilt the fixture package and verified the new registration made it into +the artifact: +``` +cd apps/backend && make e2e-plugin-package +tar -xOzf apps/backend/.build/kandev-plugin-e2e-1.0.0.tar.gz ui/bundle.js | grep -A5 e2e-insights-tools +``` +confirmed the `section: "insights"` registration is present in the packaged +bundle. + +Added one test to `apps/web/e2e/tests/plugins/plugins.spec.ts` ("registers an +insights-section item as a footer icon, not a rail row") and one to +`apps/web/e2e/tests/plugins/mobile-plugin-nav.spec.ts` ("shows the insights +item in the Utilities group, not the Plugins group"), both inside their +files' existing `test.describe` blocks per the plan. + +Built the binaries/artifacts `e2e/global-setup.ts` requires (none existed +yet in this worktree): +``` +make build-backend # apps/backend/bin/{kandev,mock-agent,...} +make build-web-e2e # apps/web/dist/ (pseudo-locale build) +``` + +Ran, from `apps/web`: +- `pnpm e2e:raw e2e/tests/plugins/plugins.spec.ts --project=chromium` — **12 + passed** (all pre-existing tests plus the new one), 51.0s. +- `pnpm e2e:raw e2e/tests/plugins/mobile-plugin-nav.spec.ts --project=mobile-chrome` + — **5 passed** (all pre-existing tests plus the new one), 24.0s. +- `pnpm e2e:raw e2e/tests/plugins/plugins-docs-screenshots.spec.ts --project=chromium` + — 2 skipped (gated behind a capture flag not set in this run; pre-existing, + unrelated to this change — confirms nothing crashed loading that spec with + the new fixture bundle). +- `pnpm run lint -- e2e/tests/plugins/plugins.spec.ts e2e/tests/plugins/mobile-plugin-nav.spec.ts` + — 0 errors, 0 warnings. +- `pnpm run typecheck` (apps/web) — clean. + +No teardown needed beyond what the suites already do themselves (each test's +plugin install is cleaned up by the files' existing `afterEach`/backend reset). +No security/trust or external side-effect boundaries beyond the existing +plugin-install flow the other tests in these files already exercise. + +Files touched matched **Files likely touched** exactly (plus the two build +artifacts `apps/backend/bin/*` and `apps/web/dist/*`, which are gitignored +build output, not tracked changes). diff --git a/docs/plans/plugins/PLUGIN-API.md b/docs/plans/plugins/PLUGIN-API.md index 7832489613..df66720738 100644 --- a/docs/plans/plugins/PLUGIN-API.md +++ b/docs/plans/plugins/PLUGIN-API.md @@ -39,12 +39,15 @@ repositoryProviderIds?: string[] }`. `repositoryProviderIds` is JSON initialization finishes. Slow or failed reloads do not by themselves revoke open or saved task panels. On explicit plugin disable/uninstall the host calls `destroy?.()`, removes the plugin's registrations, and closes its panels. - Each initialization attempt is transactional: failure or timeout unregisters - its partial contributions, aborts plugin-owned work, and fences callbacks from - the expired generation. The same generation owns host-created subscriptions, + Each initialization attempt is transactional for plugin-owned runtime state: + failure or timeout aborts plugin-owned work and fences callbacks from the + expired generation. The same generation owns host-created subscriptions, modal and task-link handles, toasts, and review surfaces; the loader closes or unsubscribes them before calling `destroy` exactly once. Requests and callbacks - from an expired generation cannot mutate the replacement generation. + from an expired generation cannot mutate the replacement generation. Failure or + timeout does **not** unregister `registry` contributions (nav items, routes, + etc.) already made before the failure — those persist, and only the plugin's + lifecycle status becomes failed, until the plugin's *next* load revokes them. ## Global entry point @@ -560,16 +563,23 @@ own write). // React. Unknown/missing names render a puzzle glyph in the sidebar. // section: "main" (default) renders as a top-level sidebar entry; // "integrations" renders inside the sidebar's Integrations section alongside -// the first-party integration links (GitHub, Jira, ...). Hosts predating a -// section value simply don't render items targeting it (additive change). +// the first-party integration links (GitHub, Jira, ...); "sidebar-footer" +// renders as an icon button in the sidebar footer's icon row and as a +// labelled row in the phone menu's Utilities group, subject to the footer's +// inline budget — an over-budget item is reached through the footer's +// overflow menu instead of an inline button; "settings" is accepted but +// renders on no surface. Hosts predating a section value, or seeing an +// unrecognised one, simply degrade to "main"'s placement — nothing is ever +// silently dropped. type PluginIcon = string | React.ComponentType<{ className?: string }>; +export type PluginNavSection = "main" | "settings" | "integrations" | "sidebar-footer"; interface NavItem { id: string; label: string; path: string; icon?: PluginIcon; - section?: "main" | "settings" | "integrations"; + section?: PluginNavSection; } // Configuration for the kandev-style title bar the host renders above a plugin diff --git a/docs/public/plugins-authoring.md b/docs/public/plugins-authoring.md index 8a518db9f7..a15a312855 100644 --- a/docs/public/plugins-authoring.md +++ b/docs/public/plugins-authoring.md @@ -249,7 +249,7 @@ slice shapes in a released plugin. | Surface | Location and input | Manifest requirement | Cleanup/lifecycle | Small example | | --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------- | | registerRoute | registry.registerRoute(path, Component, options?); exact SPA path; options.topbar defaults to host chrome or false for full-bleed | Active ui.bundle | Route is removed on disable/uninstall; use destroy for subscriptions | registry.registerRoute("/acme", Page, { topbar: { title: "Acme" } }) | -| registerNavItem | { id, label, path, icon?, section? }; section is main, integrations, or accepted-but-not-rendered settings | Active ui.bundle | Nav item is revoked and removed from desktop/phone navigation | registry.registerNavItem({ id: "home", label: "Acme", path: "/acme", icon: "chart" }) | +| registerNavItem | { id, label, path, icon?, section? }; section is main, integrations, sidebar-footer (sidebar footer icon row, subject to an inline budget past which the item moves to the footer's overflow menu; and phone menu Utilities group, uncapped), or accepted-but-not-rendered settings | Active ui.bundle | Nav item is revoked and removed from desktop/phone navigation | registry.registerNavItem({ id: "home", label: "Acme", path: "/acme", icon: "chart" }) | | registerSettingsRoute | registerSettingsRoute(fullPath, Component) with an exact path under /settings/plugins//...; settings shell supplies chrome | Active ui.bundle | Route is removed on disable/uninstall | registry.registerSettingsRoute("/settings/plugins/acme/health", HealthPage) | | registerComponent | registerComponent(slot, Component); component receives { slotProps?: unknown } | Active ui.bundle | Every registration is owner-tracked, error-isolated, and bulk-revoked | registry.registerComponent("task-sidebar", Panel) | | registerWsHandler | registerWsHandler(action, handler(payload)); receives actions bridged from lib/ws | Active ui.bundle | Handler is removed on disable/uninstall; tolerate duplicate/replayed actions | registry.registerWsHandler("acme.updated", renderUpdate) | diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index b48a501485..6f78b598d5 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -51,6 +51,7 @@ Product-wide capabilities that are not tied to a single feature area. | [plugins — agent tools](plugins/agent-tools.md) | draft | | [plugins — Voice extraction host prerequisites](plugins/voice-extraction-host.md) | shipped | | [plugins — Voice Mode leaves core](plugins/voice-extraction.md) | shipped | +| [plugin-nav-sidebar-footer](plugin-nav-sidebar-footer/spec.md) | draft | | [semantic-notifications](platform/notifications.md) | shipped | | [workspace-git-status](platform/workspace-git-status.md) | shipped | | [git-subprocess-admission](platform/git-subprocess-admission.md) | building | diff --git a/docs/specs/plugin-nav-sidebar-footer/spec.md b/docs/specs/plugin-nav-sidebar-footer/spec.md new file mode 100644 index 0000000000..7db6be2e08 --- /dev/null +++ b/docs/specs/plugin-nav-sidebar-footer/spec.md @@ -0,0 +1,876 @@ +--- +status: draft +created: 2026-08-12 +amended: 2026-08-12 +owner: nova28 +--- + +# Plugin nav items in the sidebar footer icon row + +## Amendment log + +**Amendment 1 (2026-08-12), from PR #2562 review.** The first version of this spec +exposed the host's internal `NavSection` value `"insights"` as the plugin-facing +`NavItem.section` value, and decided against any capacity policy on the footer row. +A maintainer review on the open PR raised both as contract defects. Both are accepted; +this document is the amended contract, and the two decisions are recorded in +**Plugin-facing section vocabulary** and **Capacity and overflow** with the reasoning +that replaces the original. + +Status returns to `draft` because the amended contract is not implemented. The +already-merged-into-the-branch first version is implemented; the amendment is not. The +status returns to `shipped` when PR #2562 merges carrying this contract, and +`docs/specs/INDEX.md` tracks the same value. + +**Amendment 1, spec-review round 1 corrections (2026-08-12).** The amended contract went +through an adversarial spec review (cross-vendor and cross-model legs) and came back +FIX FIRST. No acceptance criterion changed meaning; the review found document defects, +not contract defects. Corrections applied, listed so a later reader can tell them apart +from the contract itself: + +- **Missing/`null` `label` is now stated.** `Rendered identity` claimed the accessible + name is `NavItem.label` verbatim, which is false when a bundle omits `label` — + `destinationLabel()` substitutes the destination id. Qualified there, and given its own + bullet under **Failure modes**. Existing behaviour; nothing to build. +- **The inline budget's test obligation is now stated.** `MAX_INLINE_PLUGIN_FOOTER_ITEMS` + is normatively `3` while **Why 3** says the number may move freely; those could not both + hold for a test hard-coding `3`. Resolved by exporting the constant and requiring + conformance tests to derive from it. +- **A `Type surface` scenario group was added** so the `PluginNavSection` named export — + half of what this amendment delivers — is observed by something. Every other scenario + tests runtime placement and would pass against an inline, un-exported union. +- **The edit inventory now marks each entry EDIT or VERIFY-NOT-EDIT.** Its preamble + claimed every named file carries contradicting prose; entries 2, 3 and 6 were already + corrected by the first implementation on this branch. +- **The overflow trigger's test id changed** from `sidebar-insights-overflow-button` to + `sidebar-plugin-overflow-button`. Adopted from a reviewer's noted question rather than a + finding: the first spelling minted the host's internal section name `insights` into a + contract-ish identifier in the same document that removes that name from the plugin API. + Host test ids are host-owned, so this was not a defect, but nothing implements the + trigger yet, so the rename is free now and never again. +- **Stray tool-call markup** (``, ``) committed at the end of this file + was deleted. + +**Amendment 1, spec-review round 2 corrections (2026-08-12).** A second adversarial review +(both legs again: cross-vendor codex, cross-model `scope-analyst`) re-verified roughly +thirty existence citations by grep and found **zero dangling references and zero false +claims** — the contract itself was not disputed. It returned FIX FIRST on four coverage and +document-accuracy defects, all closed here without changing what any acceptance criterion +means: + +- **The `Type surface` "by name" scenario was unobservable by its own runner.** Both legs + found this independently. TypeScript is structural, so `pnpm run typecheck` cannot tell a + named alias from an identical inline union, and the scenario as written asked for a check + no typecheck can perform. Rewritten so the observable is a **type-equality assertion** + between `NavItem["section"]` and `PluginNavSection | undefined`, with a new paragraph + saying plainly that spelling is not observable, that a builder must not invent an AST or + source-text assertion, and that the declared-by-name requirement belongs to **API + surface** and is settled in review. +- **The overflow trigger's label was normative but unobserved.** `sidebar:morePluginItems` + was specified in **Capacity and overflow** while every capacity scenario selected the + trigger by `data-testid` alone, so wiring it to a different existing key would have passed + the whole suite. A scenario now pins the key. +- **Edit-inventory entry 6 pointed capacity coverage at the wrong file.** Its parenthetical + sent the capacity scenarios to `plugin-destinations.test.ts`, a pure mapping unit test + that cannot see a partition living in `app-sidebar-footer.tsx`. Corrected to name the + footer component's own test file, with an explicit "do not import the footer component + into this file" instruction. +- **The `P = 0` baseline scenario had no determinate expected set.** "Shows exactly the + buttons it shows today" reads as a claim about every footer control, five of which render + conditionally. The scenario now states three assertable clauses, and **Ordering**'s + manifest-rows-only rule was widened from ordering statements to **presence and count + statements** as well, so this class of false failure is ruled out once rather than per + scenario. + +Two outside-voice findings were refuted rather than adopted, for the second time and on the +same grounds: missing/`null`/empty `NavItem.id` and `NavItem.path` are pre-existing, +section-independent pass-throughs on which this change adds no branch and the builder writes +no code. Only the `label` case earned a **Failure modes** bullet, because only it needed a +*negative* instruction (do not add a fallback). + +**Renamed from `plugin-nav-insights-section` to `plugin-nav-sidebar-footer` (2026-08-12), +at the owner's request.** The old slug named the host's *internal* nav section, `insights` +— the very coupling this amendment removes from the plugin API — and it read as though the +feature were specific to one (private, unreleased) Insights plugin rather than a placement +any plugin can request. The new slug matches the plugin-facing value `"sidebar-footer"` +exactly, so the spec slug, the public token and the authoring docs all use one word. Moved +with it: `docs/plans/plugin-nav-insights-section/` → `docs/plans/plugin-nav-sidebar-footer/` +(plan plus three task files) and the `docs/specs/INDEX.md` row. The internal section is +still named `insights` in `apps/web/lib/navigation/types.ts` and this document still calls +it that; only the plugin-facing and document-facing names changed. + +## Why + +A plugin whose page is a compact, glanceable dashboard has nowhere good to put its +entry point. `registerNavItem` can place a row in the sidebar's labelled plugin rail +or in the Integrations section, but not in the sidebar footer's unlabelled icon strip +(the gear / Stats / doctor / Office row) where Kandev's own at-a-glance destination, +Stats, lives. Plugin authors either accept a full-width labelled row that visually +outranks what they are contributing, or they get no placement that matches the shape +of their page. + +## What + +- `NavItem.section` SHALL accept a fourth value, `"sidebar-footer"`, in addition to the + existing `"main"`, `"integrations"`, and `"settings"`. The four values SHALL be + declared as a named exported type, `PluginNavSection`. See **Plugin-facing section + vocabulary** for why the value is `"sidebar-footer"` and not the host's internal + section name. +- A plugin nav item with `section: "sidebar-footer"` SHALL render as an icon button in + the desktop sidebar footer's icon row, styled and behaving exactly like the + first-party Stats button: icon only, tooltip and accessible name from + `NavItem.label`, click navigates to `NavItem.path` — subject to the inline budget in + **Capacity and overflow**, past which the item is reached through the footer's + overflow menu instead of an inline button. +- The same item SHALL also appear as a labelled row in the phone menu's Utilities + group, which is where the first-party Stats destination already appears below the + `md` breakpoint. The sidebar is hidden on phones, so a footer-only placement would + make the item unreachable there. The phone group is **not** subject to the inline + budget; see **Capacity and overflow**. +- A `"sidebar-footer"` item SHALL NOT also appear in the sidebar's plugin rail or the + phone menu's Plugins group. Choosing `sidebar-footer` moves the item; it does not + add a second placement. +- `"integrations"` and `"settings"` behaviour SHALL be unchanged: `"integrations"` + items still render alongside the first-party integration links on both surfaces, + and `"settings"` items are still skipped entirely by navigation, which means they + render on no surface at all. A `section: "settings"` nav item is **not** what fills + the settings tree's `PluginSlot`: that slot (`` in + `settings-tree.tsx`) is fed only by `registerComponent("settings-nav", …)`, and a + plugin that wants a settings page uses `registerSettingsRoute` / `registerComponent`. + `section: "settings"` on a nav item is accepted and then dropped. +- Any other `section` value, including omitted/`undefined` and a string outside the + documented set, SHALL continue to render in the plugin rail / Plugins group exactly + as `"main"` does. Plugin bundles are untyped JavaScript at runtime, so an unknown + value must degrade to the default placement rather than being dropped. **The literal + string `"insights"` is one such unknown value and SHALL degrade to the plugin rail** + — it is deliberately not an accepted alias; see **Plugin-facing section vocabulary**. +- Plugin footer items SHALL NOT appear in the command palette, unchanged from + every other plugin section. This follows structurally, not from a rule this spec + adds: `pluginDestinations()` stamps every plugin entry with `surfaces: + SIDEBAR_AND_MENU`, and the palette's Navigation group is + `useStaticDestinations("palette")` (`components/global-commands.tsx`), which filters + on that surface. **Plugins have no route into the command palette at all today, by + any API.** `registerKeybinding(id, handler)` is not one: it binds a handler to an id + declared in the plugin's manifest `ui.keybindings[]`, which the host dispatches from + a global capture-phase keydown listener (`hooks/use-plugin-shortcuts.ts`) — a + keyboard binding, not a palette row. Adding a plugin→palette route is a separate + feature (see **Out of scope**). +- The desktop footer SHALL render at most `MAX_INLINE_PLUGIN_FOOTER_ITEMS` plugin + buttons inline and reach any remainder through a single overflow menu. No item is + dropped. See **Capacity and overflow** for the value, the derivation, and the exact + rule. +- The public authoring documentation SHALL list `sidebar-footer` as an accepted + `section` value and describe where it renders. The site is named, not left to be + found: the `registerNavItem` row of the Frontend hook/API matrix in + [`docs/public/plugins-authoring.md`](../../public/plugins-authoring.md). It is + the fifth entry in **Stale prose at the edit sites** below. + +## Plugin-facing section vocabulary + +### Decision + +`NavItem.section` is typed by a named, exported union that is the plugin's vocabulary, +not the host's: + +```ts +export type PluginNavSection = "main" | "settings" | "integrations" | "sidebar-footer"; +``` + +The host's internal `NavSection` (`"primary" | "plugins" | "integrations" | "insights" +| "utilities"`, `apps/web/lib/navigation/types.ts`) stays internal. `pluginDestinations()` +translates between the two, and remains the only place that translation happens. + +### Why this, and why not `"insights"` + +The indirection this amendment names **already existed**; it was only unnamed and +leaking in one place. `sectionFor()` in `plugin-destinations.ts` has always been the +translation layer, and `"main"` has always mapped to the internal section `plugins` — +so three of the four plugin-facing values already differed from, or were independent +of, the internal taxonomy. `"insights"` was the single member that was spelled with the +host's internal name, which is what makes the coupling look like contract. Renaming +that one member and naming the union closes the leak without changing any of the three +shipped values. + +The value is spelled after the **placement a plugin author is asking for**, not after +the host's grouping. The host can rename, split, or merge `NavSection` members without +a plugin-visible break, which is exactly the freedom the reviewer asked for and which +the previous version did not provide. + +One objection was considered and rejected: `"sidebar-footer"` names a desktop surface, +while the item also renders in the phone menu's Utilities group, where there is no +sidebar. That asymmetry is real but does not win. The phone placement is a host +**reachability guarantee** — the sidebar does not exist below `md`, so the host puts the +item where its first-party peer (`Stats`) already goes — and not a second thing the +plugin declared. A plugin author asking for the footer strip gets the footer strip, plus +whatever the host must do to keep it reachable on a phone. A deliberately neutral token +(`"quick-access"`, `"glance"`) would be surface-independent but would tell an author +nothing about where their icon lands without reading the docs, and would be a novel +coined term where `"sidebar-footer"` is self-describing. The doc comment on the field +states both placements, so nothing is hidden. + +### `"insights"` is not accepted, and not an alias + +Passing `section: "insights"` from a plugin bundle SHALL be treated as an unrecognised +value and degrade to the plugin rail / Plugins group, exactly like `"footer"` or +`"banana"`. It is not accepted, not aliased, and produces no warning beyond the +existing (absent) validation. + +This is safe and deliberate. Nothing has shipped: PR #2562 is open and unmerged, and +the one motivating consumer, `kandev-plugin-rill`, has not switched its `section` yet +(it is still `"main"`). There is no plugin in the wild passing `"insights"`, so there +is no compatibility burden to carry. Accepting both would make the internal name a +permanent de-facto public alias, re-creating the coupling this amendment removes on the +same day it removes it. + +The consequence to be explicit about: the in-tree e2e fixture bundle +(`apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js`) currently registers +`{ id: "e2e-insights-tools", …, section: "insights" }`. Under this contract that item +would silently move back to the plugin rail, so the fixture SHALL be updated to +`section: "sidebar-footer"` in the same change. It is entry 7 in **Stale prose at the +edit sites**. + +### No runtime validation is added + +There is still no runtime validation of `section` anywhere in the stack: the web +registry's `registerNavItem` pushes the item verbatim, and the backend plugin manifest +does not describe nav-item sections at all. +`apps/backend/internal/plugins/manifest/validate.go` validates a different, unrelated +enum (`ui.pages[].surface`, one of `settings | task-panel | main-nav`), which is not +this field and is not in scope. Introducing a validator, a console warning, or a +developer-mode diagnostic for an unrecognised `section` is **out of scope** — the +degrade rule is the whole error behaviour. + +## API surface + +The only contract that changes is the `section` field of `NavItem` and the new named +type for its values. That is a statement about the *contract*, not about the file +count: the field is declared in `apps/web/lib/plugins/types.ts` and mirrored in +[`docs/plans/plugins/PLUGIN-API.md`](../../plans/plugins/PLUGIN-API.md), so those two must +change together, and it is *documented* for plugin authors in +[`docs/public/plugins-authoring.md`](../../public/plugins-authoring.md), which the **What** +section requires updating in the same change. Three files, one contract. Read neither this +paragraph nor the Before/After snippet below as the exhaustive edit list — that list is +**Stale prose at the edit sites**. `PluginNavRegistration` +(`apps/web/lib/plugins/registry.ts`) extends `NavItem` and inherits the change, so it +needs no edit of its own. + +Before: + +```ts +interface NavItem { + id: string; + label: string; + path: string; + icon?: string; + section?: "main" | "settings" | "integrations"; +} +``` + +After: + +```ts +export type PluginNavSection = "main" | "settings" | "integrations" | "sidebar-footer"; + +interface NavItem { + id: string; + label: string; + path: string; + icon?: string; + section?: PluginNavSection; +} +``` + +`PluginNavSection` SHALL be exported from `apps/web/lib/plugins/types.ts` so a plugin +author writing TypeScript can name the type, and SHALL appear in +`docs/plans/plugins/PLUGIN-API.md` in the same form. Nothing else about the plugin API +changes: no new hook, no new registry method, no manifest field. + +### Stale prose at the edit sites — correct it in the same change + +Several of the files this change touches carry prose that contradicts the contract above. +It sits inches from the lines being changed, so leaving it is shipping a +self-contradictory frozen contract, and a builder who noticed but had no instruction +would be inventing scope. The spec therefore requires all seven sites to **end** in the +state described below, and names them so nobody has to guess. + +**Read each entry as a required end state, not as a guaranteed edit.** Some of these were +already brought into that state by the first (pre-amendment) implementation that is +already on this branch, so the honest instruction differs per entry and is marked on each: + +- **VERIFY-NOT-EDIT** — already correct on this branch as of this amendment. Confirm the + text still matches and move on. Finding nothing to change here is the expected outcome, + not a sign you are on the wrong baseline. Entries **2**, **3** and **6**. +- **EDIT** — genuinely stale, verified stale at the time this amendment was written. + Entries **1**, **4**, **5** and **7**. + +Entry 1 is mixed and is annotated in place: its `PluginNavSection` / `"sidebar-footer"` +content is an EDIT, while its conditional *"Hosts predating a `section` value…"* clause is +already satisfied and is VERIFY-NOT-EDIT. + +**This list is the complete edit inventory for prose and fixtures.** Together with the +mapping in `pluginDestinations()`, the union in `types.ts`, the footer's overflow +rendering and its exported `MAX_INLINE_PLUGIN_FOOTER_ITEMS`, the locale key, and the new +test coverage, it is everything this change writes. + +1. **EDIT** (mixed — see the clause note). `docs/plans/plugins/PLUGIN-API.md`, the comment + immediately above the `NavItem` interface the Before/After snippet replaces. It SHALL + state the degrade-to-`"main"` rule, describe where `"sidebar-footer"` renders (matching + the mapping table), and name `PluginNavSection`. The interface itself still declares + `section?: "main" | "settings" | "integrations" | "insights"` and is genuinely stale. + The older *"Hosts predating a `section` value simply don't render items targeting it + (additive change)"* line is already absent — that clause is VERIFY-NOT-EDIT; if it ever + reappears it SHALL be removed, because it contradicts the degrade rule and is false + about the shipped host, which drops nothing. +2. **VERIFY-NOT-EDIT.** `apps/web/lib/navigation/plugin-destinations.ts`, the + `pluginDestinations` docblock. It SHALL say `section: "settings"` items are skipped + and render on **no** surface — not that they render in the settings tree's + `PluginSlot`, which is false; that slot is fed only by + `registerComponent("settings-nav", …)`. Already in this state on this branch; confirm + and leave it. +3. **VERIFY-NOT-EDIT.** `apps/web/lib/navigation/plugin-destinations.ts`, the comment on + the `surfaces: SIDEBAR_AND_MENU` line. It SHALL state that plugin destinations never + declare the `palette` surface, without naming a substitute API, because there is no + plugin route into the palette (see **What**). It SHALL NOT cite `registerShortcut`, an + identifier that does not exist anywhere in this repository. Already in this state on + this branch; confirm and leave it. +4. **EDIT.** `apps/web/lib/plugins/types.ts`, the doc comment on the `section` field. It SHALL + document all four values, matching the mapping table: `"main"` (default) as a + top-level sidebar entry, `"integrations"` inside the sidebar's Integrations section, + `"sidebar-footer"` as an icon button in the sidebar footer's icon row and as a + labelled row in the phone menu's Utilities group, and `"settings"` accepted and + rendered nowhere. It SHALL also state that the footer placement is subject to the + inline budget and that an over-budget item is reached through the footer's overflow + menu, so an author is not surprised by an icon that is present but not inline. +5. **EDIT.** `docs/public/plugins-authoring.md`, the `registerNavItem` row of the Frontend hook/API + matrix. It SHALL list `sidebar-footer` and say where it renders, matching the mapping + table, and SHALL NOT mention `insights`. This is the public authoring surface the + **What** section's documentation SHALL points at, and it is the enumeration a plugin + author actually reads. +6. **VERIFY-NOT-EDIT.** `apps/web/lib/navigation/plugin-destinations.test.ts`, the title of + the palette exclusion test. It already reads *"keeps plugin items off the palette"*. If + it ever reads *"…which plugins reach via shortcuts"* again, that trailing clause is the + same false claim as item 3 and SHALL be removed; the title SHALL state the exclusion + without naming a substitute route. The assertion itself is correct and SHALL NOT change. + (This file does gain new cases — the `"sidebar-footer"` → `insights` mapping row and the + `"insights"`-degrades-to-`plugins` row — and it is one module under `apps/web` in which a + **Type surface** import may live. It is **not** where the capacity scenarios go: + `MAX_INLINE_PLUGIN_FOOTER_ITEMS` and the inline/overflow partition live in + `app-sidebar-footer.tsx` per **Capacity and overflow**, which a pure mapping unit test + cannot see, so those scenarios belong to the footer component's own test file + (`app-sidebar-footer.test.tsx`, which already exists) or to the e2e specs. Do not import + the footer component into this file to make them fit. Either way that is new coverage, + not a correction, and is not what this entry governs.) +7. **EDIT.** `apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js`, the + `e2e-insights-tools` registration. Its `section` SHALL change from `"insights"` to + `"sidebar-footer"`. Without this the e2e fixture item silently relocates to the + plugin rail and the desktop/phone footer e2e specs fail — a real behavioural + consequence of this amendment, not a cosmetic one. + +Items 1 to 4 and 6 are prose corrections; item 5 is a documentation requirement; item 7 +is a fixture correction with behavioural effect. None of items 1 to 6 changes behaviour, +a signature, an exported symbol, or a test assertion, and nothing here is an +implementation decision left open. Of the seven, **four require an edit** (1, 4, 5, 7) and +**three are verify-only** (2, 3, 6); a builder who finds nothing to change in the +verify-only three has done that part correctly. + +**The mapping lives in exactly one module.** `pluginDestinations()` in +`apps/web/lib/navigation/plugin-destinations.ts` is the sole place a `NavItem.section` +value is translated into a manifest `NavSection` — it is the only reader of that field +anywhere in the web app. The section-to-placement table below becomes code there and +nowhere else, which is why every other navigation module except the footer component is +listed under **Out of scope**. + +### Section-to-placement mapping + +This table is the whole placement contract. `NavItem.section` value on the left, +resulting internal navigation section and rendered surfaces on the right. The desktop +column reads "footer icon button" for items within the inline budget and "footer +overflow menu item" beyond it; see **Capacity and overflow**. + +| `NavItem.section` | Nav section | Desktop sidebar | Phone menu | Palette | +|---|---|---|---|---| +| `"sidebar-footer"` | `insights` | footer icon button, or footer overflow menu item past the budget | Utilities group row | no | +| `"integrations"` | `integrations` | Integrations section row | Integrations group row | no | +| `"settings"` | *(skipped)* | no | no | no | +| `"main"` | `plugins` | plugin rail row | Plugins group row | no | +| omitted | `plugins` | plugin rail row | Plugins group row | no | +| `"insights"` | `plugins` | plugin rail row | Plugins group row | no | +| any other string | `plugins` | plugin rail row | Plugins group row | no | + +### Rendered identity + +The footer builds each button's test id from the resolved destination id, which for a +plugin entry is the owner-namespaced `plugin::`. +The resulting attribute is therefore: + +``` +data-testid="sidebar-plugin::-button" +``` + +For example the plugin `kandev-plugin-rill` registering `{ id: "rill", section: "sidebar-footer" }` +yields `data-testid="sidebar-plugin:kandev-plugin-rill:rill-button"`. This is the +existing derivation applied unchanged to a plugin destination — the footer is not +special-cased — and it is stated here because it becomes public contract the moment +the first plugin uses it. **When `label` is present** the accessible name is +`NavItem.label` verbatim, untranslated, matching how every other plugin-supplied label is +treated. When it is absent the host substitutes the destination id; that case is a +pre-existing resolver behaviour and is stated under **Failure modes**, not here, because +it is not something this change introduces or may alter. + +**An over-budget item carries the same test id.** When an item is reached through the +overflow menu rather than an inline button, its menu item SHALL carry the identical +`data-testid="sidebar-plugin::-button"` and the identical accessible +name. A conformance test therefore selects the same id either way; the only difference +is that the menu must be opened first, because the menu's content is not in the DOM +while closed. The overflow trigger itself carries +`data-testid="sidebar-plugin-overflow-button"`. + +Phone Utilities rows carry **no** `data-testid`. The shared row renderer only emits a +plugin test id when the calling surface supplies its own prefix, and neither Utilities +caller does today. Conformance tests for the phone surface must select by visible label. +Adding a prefix there is deliberately excluded (see **Out of scope**). + +## Ordering + +### What "order" means in this spec: manifest rows only + +**Every ordering statement in this document — in this section and in every scenario +below — constrains the relative order of the *manifest destination rows* only**, meaning +the rows a surface renders from `resolveDestinations`. Neither surface renders those rows +alone: both interleave bespoke, non-manifest controls around them. This spec makes no +ordering claim about those controls and changes none of them — they stay exactly as they +render today (see **Out of scope**). A conformance test must therefore assert the relative +order of the manifest rows and must not fail because a non-manifest control sits before, +between, or after them. + +**The same manifest-rows-only reading governs every *presence* and *count* statement in +this document**, not just the ordering ones. "The footer shows X" and "exactly N buttons +are present" are claims about the manifest-derived buttons plus the one new non-manifest +control this change adds (the overflow trigger, whose presence and absence are normative +per **Capacity and overflow**). They are never claims about the footer's or the phone +group's full visible control set, because most of that set renders conditionally on state +this change does not touch — What's new on `releaseNotes.showTopbarButton`, the +Office↔Kanban switch on `officeEnabled`, the connection warning on connection state, the +user chip on auth mode, and the phone Health row on whether there are health issues. A +test that pins the complete visible sequence would fail on a profile flag this contract +says nothing about, which is a false failure, not a caught regression. + +This is stated once, here, rather than repeated per scenario. The two surfaces and their +non-manifest neighbours are: + +- **Desktop sidebar footer** (`app-sidebar-footer.tsx`). Before the `insights` + destinations: the settings gear. After them: the doctor / Improve Kandev button, and + then What's new, the Office↔Kanban switch, the theme toggle, the connection warning and + the user chip, each rendered conditionally. The overflow trigger this amendment adds is + itself a non-manifest control and sits at a fixed position defined below. +- **Phone menu Utilities group** (`UtilityNavSection` in `app-nav-sections.tsx`). Before + the destination rows: the Status row, rendered only when the app status drawer is + enabled. After them: the theme toggle, the Improve Kandev row, and the Health issues + row, the last rendered only when there are health issues to show. + +None of those is a destination, none comes from `APP_DESTINATIONS` or from a plugin, and +none except the new overflow trigger is added, removed or reordered by this change. + +### The order itself + +Within the `insights` section, on both the sidebar footer and the phone Utilities +group, manifest destinations render in this total order: + +1. **First-party entries**, in their array position in `APP_DESTINATIONS` + (`apps/web/lib/navigation/core-destinations.ts`). Today that is exactly one entry, + `stats`. First-party always precedes plugin entries — the merged list is + first-party-then-plugins by construction, matching how the Integrations section + already orders its plugin additions. +2. **Plugin entries**, in plugin-registry registration order, i.e. the order + `pluginRegistry.getNavRegistrations()` returns them. + +The inline budget does not reorder anything. It **partitions** the plugin run at a fixed +index: the first `MAX_INLINE_PLUGIN_FOOTER_ITEMS` plugin entries render inline in that +order, and the remainder render in the overflow menu in that same order. Concatenating +the inline run with the menu's contents reproduces the full order above, unchanged. + +Within a single `loadPlugins` pass, registration order is fully determined and needs no +tiebreak, because it is a single append-ordered array: + +- Across plugins: `loadPlugins` iterates the boot payload's `plugins` array with a + sequential `for … of` and awaits each plugin's `initialize()` before starting the + next, so plugin *A* earlier in the boot payload has all of its nav items registered + before plugin *B* registers any of its own. Bundle import latency does not reorder + anything. +- Within one plugin: items appear in the order `initialize()` calls `registerNavItem`. + +That determinism is scoped to one pass on purpose. `loadPlugins` can be **in flight +more than once at a time**: boot fires it without awaiting the result, and the settings +enable/update path calls it independently (`lib/plugins/host.ts` documents this race in +its own module comment). Generation fencing is per-`pluginId` — it stops a stale load +from clobbering a newer one for the *same* plugin — and it imposes no global order +across concurrent loads of *different* plugins. So a plugin enabled from Settings while +boot loading is still running lands wherever the two passes interleave, which is not +predictable from the boot payload order alone. This is the same family as the +re-enable rule below, it is pre-existing for every nav section, and no ordering control +is introduced to fix it. + +Three consequences that are contract, not accident: + +- A plugin disabled and re-enabled at runtime has its registrations removed and then + re-appended, so its footer icon moves to the **end** of the plugin run of the + insights row. The row does not re-sort to restore the boot order. +- Position is not alphabetical and is not influenced by `label`, `id`, or `path`. A + plugin cannot choose its slot in the row, and cannot choose whether it is inline or + in the overflow menu. +- Because the partition is by position, a re-enable that moves a plugin to the end of + the run can move it **from inline into the overflow menu**, and can promote the plugin + that was first in the overflow menu into the inline run. The item is still present and + still reachable; only its affordance changed. This is the same registration-order + consequence as the bullet above, and no stickiness or memory is introduced to avoid + it. + +No per-surface ordering override is introduced. The footer and the phone Utilities +group agree on the **relative order of `insights` entries**: `stats` first, then plugin +entries in registration order, on both surfaces. They differ only in affordance — the +phone renders every entry as a row, the footer may place a suffix of the plugin run +behind the overflow trigger. + +They do **not** resolve the same list. The phone Utilities group resolves two sections in +one pass (`MOBILE_MENU_UTILITY_SECTIONS = ["insights", "utilities"]`), and the resolver +returns catalog array order followed by plugin entries — it does not group by section. +Because `stats` (`insights`) precedes `settings` (`utilities`) in `APP_DESTINATIONS`, the +phone group's **manifest rows**, in order and ignoring the non-manifest controls named +above, are: + +``` +stats, settings, +``` + +so a plugin row lands **after** Settings on the phone, not adjacent to Stats as the +footer's `stats, ` might suggest. Conformance tests for the phone surface must +expect that position, and must read it as a claim about the manifest rows only: the +Status row precedes them and the theme, Improve Kandev and Health rows follow them, so a +test asserting the group's complete visible row sequence would fail on controls this +change does not touch. Interleaving the two sections is existing behaviour and is not +changed here. + +## Capacity and overflow + +**Decision: a bounded inline budget on the desktop footer, with a single overflow menu +holding the remainder. Nothing is dropped, and the phone surface is uncapped.** + +### The rule + +- `MAX_INLINE_PLUGIN_FOOTER_ITEMS = 3`. The constant SHALL live in + `apps/web/components/app-sidebar/app-sidebar-footer.tsx`, next to the code that + applies it, and SHALL be **exported** so conformance tests can import it. The export + exists for the tests; nothing else imports it, and it is not part of the plugin API. +- The budget counts **plugin** `insights` destinations only — those with + `source === "plugin"` on the resolved destination, after the `section: "settings"` + skip. First-party `insights` entries (today, `stats`) are never counted, never + overflowed, and always render inline. +- Let *P* be the number of plugin `insights` destinations resolved for the sidebar. + - *P* = 0: the footer renders exactly as it does today. No overflow trigger. + - 0 < *P* ≤ 3: all *P* render as inline icon buttons, in order. **No overflow trigger + is rendered** — a "more" button holding nothing is worse than no button. + - *P* > 3: the first 3 render as inline icon buttons, in order, followed by a single + overflow trigger holding the remaining *P* − 3 items as menu items, in the same + order. +- The overflow trigger sits immediately after the last inline plugin button and before + the doctor / Improve Kandev button, so the insights block occupies at most 5 slots + (`stats` + 3 inline + trigger). +- The same budget applies **collapsed and expanded**. A per-state threshold would make + icons jump between inline and menu when the user toggles the sidebar, which is a + worse surprise than a constant rule. +- The overflow trigger renders with the footer's existing icon-button treatment + (`FooterIconButton`'s size, tooltip and hover behaviour) wrapped as a + `@kandev/ui/dropdown-menu` trigger, using `IconDots` from `@tabler/icons-react`. It + is named `InsightOverflowMenu`. Its label and tooltip come from a new i18n key + `sidebar:morePluginItems` with the English value `More plugin items`. Per this repo's + i18n rules the key SHALL be added to `src/locales/en/sidebar.json` and the `pseudo` + catalog regenerated; the real-locale catalogs (`zh-cn`, `pt-pt`) are translated out of + band and are **out of scope**. +- Each overflow menu item renders the destination's resolved icon followed by its + `label` as visible text — the labels are the reason the menu is legible where an + icon-only strip is not. Clicking a menu item navigates to its `href` exactly as the + inline button does. Menu placement, keyboard navigation and focus behaviour are + whatever `@kandev/ui/dropdown-menu` already provides at its defaults; no bespoke + keyboard handling, side/align override, or focus management is added. + +### Why a bound, when the first version rejected one + +The first version rejected a cap on the grounds that *"silently dropping the N+1th +plugin's icon would remove a plugin's only entry point with no error surface and no way +for the author to discover it."* That argument is sound and still stands — **against a +cap that drops**. It does not reach an overflow menu, which drops nothing: every +registered item is reachable, in order, one click away, and the trigger is a visible +affordance rather than a silent truncation. Rejecting the drop-cap and then treating the +question as closed was the gap; the reviewer found it. + +The defect the bound actually fixes is a **priority inversion**, not crowding. The +footer renders third-party content *before* the host's own controls, inside a container +that clips rather than scrolls: the footer is `shrink-0` inside +`
` +(`app-sidebar.tsx`). Unbounded, a large enough plugin run pushes the theme toggle, the +Office↔Kanban switch, the connection warning and the account chip past the clip, with no +scroll to recover them. Those are host controls a user cannot reach any other way from +the sidebar, and the plugin that displaced them is the thing they would need to reach +Settings to disable. Bounding the plugin block at 4 occupied slots makes that outcome +unreachable at any *P*. + +An alternative was considered and rejected: **reordering** the plugin run to the end of +the footer, after the host controls, so clipping eats plugin icons first. It is a +smaller change and it does fix the inversion, but it splits `stats` from the plugin run +it introduces, breaks the desktop/phone ordering agreement this spec establishes in +**Ordering**, and still leaves the row unbounded — a tall enough run still clips, only +now it clips the plugins with no affordance to reach them. The bound is the better +contract. + +### Why 3 + +The collapsed footer is the binding constraint: it is a non-wrapping `flex-col`, so each +icon costs a full row, where the expanded footer is `flex-wrap` and fits roughly seven +28px buttons per line at the sidebar's default width. + +Collapsed pitch is 32px (`h-7` button, `gap-1`), except the connection warning at 36px. +Today's worst-case first-party collapsed column is eight controls — gear, `stats`, +Improve, What's new, Office, theme, connection, user chip — at roughly 260px. A plugin +budget of 3 inline plus 1 trigger adds four rows, roughly 130px, for a worst case near +390px plus padding. That leaves the nav list above it non-zero on any viewport taller +than about 600px, which is the shortest window the app is expected to be usable in. +3 is the largest budget that keeps the worst case under 400px, which is why it is 3 and +not 5. + +The number is a layout constant, not a contract with plugin authors: a future change may +raise or lower it without renegotiating this spec, provided the rule shape (inline run, +then a single overflow trigger, nothing dropped) holds. + +**What that freedom requires of the tests, stated so it is not invented.** The normative +content of this section is the *rule shape*, not the digit. `3` is the value at the time +of writing, and the scenarios below spell it out only to stay concretely readable. +Conformance tests SHALL therefore derive their expectations from the exported +`MAX_INLINE_PLUGIN_FOOTER_ITEMS` rather than hard-coding `3` — a boundary case is written +as `MAX_INLINE_PLUGIN_FOOTER_ITEMS` items for the at-budget case and +`MAX_INLINE_PLUGIN_FOOTER_ITEMS + 1` for the first over-budget case, not as `3` and `4`. +Changing the constant then moves the suite with it and leaves every scenario below true, +which is exactly the freedom the previous paragraph claims. A test that hard-codes `3` +would convert a layout constant into a frozen contract by accident, and would make the +two paragraphs contradict each other the first time anyone tuned the number. The one +exception is the *rule-shape* boundary itself: a test MAY hard-code that `P = 0` renders +no trigger, because zero is a property of the rule, not of the budget. + +### The guarantee + +The contract that replaces "render everything inline" is a **reachability** guarantee: + +- Every plugin `insights` destination SHALL be reachable from the desktop footer in at + most one extra click: inline, or as a menu item under the overflow trigger. None is + dropped, hidden behind a count threshold, or made unreachable. +- The overflow menu's content is not in the DOM while the menu is closed. A conformance + test for an over-budget item SHALL open the menu before asserting. This is the honest + version of the guarantee, and it is what a test can assert. +- **Expanded**, the footer's container remains a wrapping flex row (`flex-wrap`), so the + bounded run plus the host controls flow onto a second line rather than overflowing + horizontally. +- **Collapsed**, the footer remains a non-wrapping vertical column (`flex-col`), now of + bounded height. +- The **phone** Utilities group renders every plugin item as a labelled row, with no + budget and no overflow menu. It is a scrolling sheet, not a clipped strip, so the + failure mode the budget exists to prevent does not occur there, and adding a menu + inside a menu would be strictly worse. This asymmetry is deliberate contract. + +## Failure modes + +- **Unknown or missing `icon` name.** The curated icon map falls back to the generic + puzzle-piece glyph, as it already does for every other plugin nav placement. The + button still renders and still navigates. The same applies to an overflow menu item. +- **Unknown `section` string** (an untyped bundle passing e.g. `"footer"`, or the host's + internal `"insights"`). Treated as `"main"`: the item renders in the plugin rail / + Plugins group. It is never dropped and never silently promoted to the footer. +- **Empty `label`** (the empty string). The button renders with an empty accessible name + and an empty tooltip. `destinationLabel()` coalesces on nullishness, not on + falsiness — `"" ?? id` is `""` — so an empty string survives to the button as an empty + string. This is pre-existing behaviour shared with every plugin nav surface; this + change neither introduces nor fixes it. +- **Missing or `null` `label`.** The accessible name and tooltip become the + owner-namespaced destination id (`plugin::`), not an empty string. + `NavItem.label` is typed `string` and therefore required, but plugin bundles are + untyped JavaScript at runtime — the same reason an unrecognised `section` must degrade + rather than be trusted — so a bundle can omit it. `destinationLabel()` in + `apps/web/lib/navigation/resolve-destinations.ts` ends + `return destination.label ?? destination.id`, and the resolver is out of scope for this + change, so the footer button and the overflow menu item both inherit that substitution + unchanged. A conformance test MUST NOT assert accessible-name-equals-`label` for an + item that omits `label`; the observable outcome there is the destination id. This is + pre-existing behaviour shared with every plugin nav surface; this change neither + introduces nor fixes it, and a builder must not add a label fallback of their own to + satisfy this spec. +- **`path` pointing at a first-party route.** Unchanged from today: the href is used + verbatim, so a plugin can *link* to a first-party page but cannot *serve* one — the + plugin route resolver runs after every static route. +- **Two registrations of the same `(pluginId, id)` pair.** Both append, producing two + entries sharing one destination id, and both count against the budget. Pre-existing + for all sections; not changed here. +- **A plugin's bundle never loads at all** (import fails, or it does not call + `registerKandevPlugin`). `initialize()` never runs, so no nav item is registered and + no icon appears. The loader marks the plugin failed, moves on to the next one, and + the footer renders the remaining icons in order. +- **A plugin throws or times out *partway through* `initialize()`.** Registrations are + **not** rolled back. `initialize()` runs against a live registry, so every + `registerNavItem` call it completed before the failure has already landed; on timeout + the loader logs and marks the plugin failed, and on a throw it logs and marks the + plugin failed, and neither path revokes registrations (`unregisterPlugin` runs at the + *start of the plugin's next load*, not on failure). `getNavRegistrations()` applies no + status filter, so a plugin that registers a `sidebar-footer` item and then hangs leaves + that entry in the footer until it is next loaded or disabled, occupying a budget slot. + This is pre-existing loader behaviour shared by every nav section; this change neither + introduces nor fixes it, and a builder must not add rollback to satisfy this spec. + +## Concurrency and idempotency + +The plugin registry is a synchronous single-threaded singleton, so there is no torn +write and no two-writer case for the nav-item array itself: every `registerNavItem` +call completes before any other code runs. Reload generation guards prevent a stale +load from re-adding registrations after a newer generation claimed the same plugin. +Re-rendering the footer is a pure read of the registry; it performs no writes and is +safe to run any number of times. Applying the budget is a pure function of the resolved +destination list, so it is likewise idempotent and introduces no state of its own — the +inline/overflow partition is recomputed on every render and is never remembered. + +What is *not* claimed is a global ordering guarantee across concurrent `loadPlugins` +invocations — see **Ordering**. Two passes can interleave, and the resulting position +of a plugin's icon, and therefore whether it is inline or in the overflow menu, depends +on that interleaving. Safety is guaranteed; boot-payload position is guaranteed only +within one pass. + +Idempotency of a reload is already handled by the loader and is unchanged here: a load +revokes the plugin's prior registrations before re-running `initialize()`, so repeatedly +loading the same plugin converges to exactly one set of nav items rather than +accumulating duplicates. The exception is the partial-failure case in **Failure +modes**: registrations from a failed pass persist until that next load revokes them. + +## Scenarios + +### Type surface + +These are compile-time observations, not runtime ones. They exist because the named +exported type is half of what this amendment delivers, and every other scenario in this +document exercises runtime placement only — a suite built from those alone would pass +against an inline, un-exported union, silently shipping the coupling this amendment +removes while `PLUGIN-API.md` documents a type that does not exist. `pnpm run typecheck` +is the runner. + +- **GIVEN** a TypeScript module under `apps/web` writing `import type { PluginNavSection } from "@/lib/plugins/types"`, **WHEN** the web package typechecks, **THEN** the import resolves — the type is exported under that exact name from that exact module. +- **GIVEN** that imported type, **WHEN** each of `"main"`, `"settings"`, `"integrations"` and `"sidebar-footer"` is assigned to a `PluginNavSection`, **THEN** all four assignments typecheck. +- **GIVEN** that same type, **WHEN** the literal `"insights"` is assigned to a `PluginNavSection` under `// @ts-expect-error`, **THEN** the package still typechecks — the suppressed error is present, proving `"insights"` is rejected by the union rather than accepted. If `"insights"` were ever added back to the union the unused `@ts-expect-error` itself becomes the type error, so this scenario fails in both directions and cannot rot silently. +- **GIVEN** the `NavItem` interface in the same module, **WHEN** a type-equality assertion compares `NavItem["section"]` against `PluginNavSection | undefined`, **THEN** the assertion typechecks — and it stops typechecking the moment either side gains, loses, or renames a member, which is the drift this scenario exists to catch. + +**What that last scenario does and does not observe.** TypeScript is *structural*, so +`section?: PluginNavSection` and an inline `section?: "main" | "settings" | "integrations" +| "sidebar-footer"` are the same type and no typecheck can tell them apart. The observable +is therefore **equality**, not spelling, and a builder SHALL NOT invent an AST walk or a +source-text match to assert the latter — that would be inventing a mechanism this spec +never asked for. Equality is the property that carries the guarantee: the two can only hurt +anyone by diverging, and divergence is exactly what fails. That the field is *declared* as +`section?: PluginNavSection` rather than as a repeated literal union is a single-source +requirement of **API surface** (see its After snippet), settled by reading the diff in +review, not by this scenario. The standard conditional-type equality helper +(`() => T extends A ? 1 : 2` compared against `() => T extends B ? 1 : 2`) is the +usual way to write it; the spec does not require any particular spelling or helper library. + +### Placement + +- **GIVEN** an active plugin `acme` that registers `{ id: "board", label: "Acme Board", path: "/plugins/acme", section: "sidebar-footer" }`, **WHEN** the desktop sidebar footer renders, **THEN** an icon button with accessible name `Acme Board` and `data-testid="sidebar-plugin:acme:board-button"` appears in the footer row, and clicking it navigates to `/plugins/acme`. +- **GIVEN** that same plugin, **WHEN** the desktop sidebar's plugin rail renders, **THEN** no row for `board` appears in it. +- **GIVEN** that same plugin, **WHEN** the phone menu's Plugins group (`MobilePluginNavSection`) renders, **THEN** no row for `board` appears in it. This is the mobile half of the same "moves, does not add" rule the previous scenario asserts for the desktop rail; both halves are contract, and a regression that left a `sidebar-footer` item also matching the `plugins` section would show up here and nowhere else. +- **GIVEN** that same plugin, **WHEN** the phone menu's Utilities group renders, **THEN** a row labelled `Acme Board` linking to `/plugins/acme` appears in it. +- **GIVEN** that same plugin, **WHEN** the command palette's Navigation group renders, **THEN** no entry for `board` appears. +- **GIVEN** a plugin item with `section: "integrations"`, **WHEN** the sidebar's Integrations section and the footer both render, **THEN** the item appears in Integrations and does not appear in the footer. +- **GIVEN** a plugin item with `section: "settings"`, **WHEN** any navigation surface resolves, **THEN** no destination for that item exists on any surface. +- **GIVEN** a plugin item with `section` omitted, **WHEN** the sidebar renders, **THEN** the item appears in the plugin rail and does not appear in the footer. +- **GIVEN** a plugin item with an unrecognised `section` string, **WHEN** the sidebar renders, **THEN** the item appears in the plugin rail and does not appear in the footer. +- **GIVEN** a plugin item with `section: "insights"` — the host's internal section name — **WHEN** the sidebar renders, **THEN** the item appears in the plugin rail and does not appear in the footer, because `"insights"` is not an accepted plugin-facing value. +- **GIVEN** no active plugin registers a `sidebar-footer` item, **WHEN** the footer renders, **THEN** its **manifest buttons** are exactly one — `stats` — no button carrying a `sidebar-plugin:…-button` test id exists, and no element with `data-testid="sidebar-plugin-overflow-button"` exists. Per **Ordering**, read this as a claim about the manifest-derived buttons and the overflow trigger only, not about the footer's full icon set: the bespoke controls around them (gear, doctor, What's new, Office↔Kanban, theme, connection warning, user chip) each render conditionally on state this change does not touch, so a test asserting the complete visible button sequence would fail on flags this contract says nothing about. Those three clauses *are* the normative *"P = 0: the footer renders exactly as it does today"* guarantee in **Capacity and overflow**, stated in the form a test can assert. +- **GIVEN** a plugin `sidebar-footer` item whose `icon` is a name absent from the curated icon map, **WHEN** the footer renders, **THEN** the button renders with the puzzle-piece fallback glyph and still navigates to `path`. + +### Ordering + +- **GIVEN** the first-party `stats` destination and a plugin `sidebar-footer` item, **WHEN** the insights section resolves for the sidebar, **THEN** `stats` is ordered before the plugin item. +- **GIVEN** two active plugins `acme` then `globex`, each registering one `sidebar-footer` item, and `acme` listed first in the boot payload, **WHEN** the footer renders, **THEN** `stats`, `acme`'s item and `globex`'s item appear in that **relative order among the `insights` entries**. Per **Ordering**, this constrains those three manifest buttons only and is not an assertion about the footer's full icon sequence; a conformance test must not fail because the settings gear precedes them or the doctor button follows them. +- **GIVEN** two active plugins that both register a `sidebar-footer` item with `id: "dashboard"`, **WHEN** the footer renders, **THEN** both icons render with distinct destination ids `plugin::dashboard` and `plugin::dashboard`. +- **GIVEN** the boot-order footer manifest run `stats, acme, globex`, **WHEN** `acme` is disabled and re-enabled, **THEN** the `insights` entries appear in the relative order `stats`, `globex`, `acme` — same manifest-rows-only reading as the boot-order scenario above, per **Ordering**. +- **GIVEN** that same plugin `acme`, **WHEN** the phone menu's Utilities group resolves, **THEN** its **manifest rows** appear in the relative order `Stats`, `Settings`, `Acme Board` — the plugin row follows the first-party `utilities` entry, not `Stats`. Per **Ordering**, this constrains those three rows only: the Status row renders before them and the theme, Improve Kandev and Health rows render after them, and a test must not fail on their presence. + +### Capacity and overflow + +The counts below are written against `MAX_INLINE_PLUGIN_FOOTER_ITEMS` at its current +value of 3 — read `3` as "the budget", `4` as "the budget plus one" and `8` as "well over +the budget". Per **Capacity and overflow**, conformance tests derive these from the +exported constant rather than hard-coding the digits. + +- **GIVEN** exactly 3 active plugins each registering one `sidebar-footer` item, **WHEN** the expanded desktop footer renders, **THEN** all 3 plugin buttons are present inline in registration order and **no** element with `data-testid="sidebar-plugin-overflow-button"` exists. +- **GIVEN** 4 active plugins `p1…p4` each registering one `sidebar-footer` item, in that registration order, **WHEN** the desktop footer renders, **THEN** inline buttons for `p1`, `p2` and `p3` are present in that order, an overflow trigger with `data-testid="sidebar-plugin-overflow-button"` follows them, `p4`'s button is not inline, and opening the trigger reveals exactly one menu item, for `p4`, carrying `data-testid="sidebar-plugin:p4:-button"` and accessible name equal to its `label`. +- **GIVEN** those same 4 plugins, **WHEN** the desktop footer renders, **THEN** the overflow trigger's accessible name and its tooltip are both the rendered value of the i18n key `sidebar:morePluginItems` (`More plugin items` in the `en` catalog). A conformance test SHALL derive the expected string from that key rather than hard-coding the English text, so re-wording the copy moves the test with it: what this scenario pins is the **key**, because wiring the trigger to some other existing key is the failure it exists to catch and every other capacity scenario selects the trigger by test id alone and would stay green through it. +- **GIVEN** those same 4 plugins, **WHEN** the overflow menu is open and `p4`'s menu item is clicked, **THEN** the app navigates to `p4`'s `path`. +- **GIVEN** 8 active plugins each registering one `sidebar-footer` item, **WHEN** the expanded desktop footer renders, **THEN** exactly 3 inline plugin buttons and one overflow trigger are present, opening the trigger reveals the remaining 5 in registration order, none of the 8 is dropped, and the footer container carries its wrapping-row classes. +- **GIVEN** those same 8 plugins, **WHEN** the **collapsed** desktop footer renders, **THEN** the same 3 inline buttons and one overflow trigger are present — the budget is unchanged by collapse — and the container carries its non-wrapping column classes. +- **GIVEN** those same 8 plugins, **WHEN** the phone menu's Utilities group renders, **THEN** all 8 appear as labelled rows in registration order, with no overflow menu and no truncation. +- **GIVEN** the first-party `stats` destination and 8 plugin `sidebar-footer` items, **WHEN** the desktop footer renders, **THEN** `stats` renders as an inline button and is not placed in the overflow menu, and the 3-item budget applies to the plugin items alone. +- **GIVEN** 4 plugins `p1…p4` with `p1` inline and `p4` in the overflow menu, **WHEN** `p1` is disabled and re-enabled, **THEN** `p2`, `p3` and `p4` are the inline buttons and `p1` is the sole overflow menu item, per the registration-order partition in **Ordering**. +- **GIVEN** a plugin whose `initialize()` registers a `sidebar-footer` item and then throws or exceeds the initialize timeout, **WHEN** the footer renders, **THEN** the already-registered entry is still present and still occupies a budget slot, because a failed initialize does not revoke registrations already made. + +## Out of scope + +- **Renaming the three shipped `NavItem.section` values** (`"main"`, `"integrations"`, + `"settings"`). They are already independent of the host's internal `NavSection` names + and are in use by shipped plugins; renaming them would be a breaking change for no + gain. Only the new member is named after placement intent. +- **Accepting `"insights"` as an alias for `"sidebar-footer"`, or warning when a plugin + passes an unrecognised `section`.** Both are decided against in **Plugin-facing + section vocabulary**; the degrade rule is the whole error behaviour, and no runtime + validation, console warning, or developer-mode diagnostic is added. +- **Backwards compatibility for `section: "insights"`.** Nothing has shipped that uses + it: PR #2562 is unmerged and `kandev-plugin-rill` has not switched. +- **A capacity policy on the phone Utilities group.** It is a scrolling sheet of + labelled rows; the clipping failure the desktop budget prevents does not occur there. +- **Capping, truncating, or overflowing first-party `insights` destinations.** The + budget counts plugin entries only. If `APP_DESTINATIONS` ever grows a second + `insights` entry, that is a first-party layout decision, not this contract. +- **Making the inline budget configurable** — by user setting, plugin manifest field, + or runtime feature flag. It is a layout constant in the footer component. +- **Making the sidebar footer scroll, or changing the `overflow-hidden` sidebar + container.** The budget removes the need; changing the container is a sidebar-layout + decision, not part of opening a section to plugins. +- **Rolling back nav registrations made before a plugin's `initialize()` throws or + times out.** Pre-existing loader behaviour across every section; see **Failure + modes**. +- **A per-plugin or per-surface ordering control** (`order`, `priority`, or + alphabetical sorting) for nav items, and any stickiness that would keep a plugin + inline across a re-enable. Registration order stands for every section, including + this one. +- **Changing the footer's or the phone group's other non-manifest controls.** The footer + component gains exactly one new control, the overflow trigger, at the position stated + in **Capacity and overflow**. No change to its bespoke buttons (gear, doctor, What's + new, Office, theme, connection, user chip), to their styling or order, or to the phone + Utilities group's bespoke rows (Status, theme, Improve Kandev, Health issues). +- **Changing the first-party catalog** (`core-destinations.ts`) or the resolver + (`resolve-destinations.ts`). Neither needs to know a plugin item can be + `sidebar-footer`; the budget is applied in the footer component, not in the manifest. +- **Adding a plugin `data-testid` prefix to the phone Utilities rows.** Those rows are + test-id-less for first-party and plugin entries alike today; adding one is a + separate surface-contract decision, and label-based selection covers this feature's + conformance needs. +- **Palette entries for plugin nav items** of any section, and more broadly **any + plugin route into the command palette**. There is none today: the palette's + Navigation group resolves `useStaticDestinations("palette")` and plugin destinations + never declare the `palette` surface, while `registerKeybinding` binds a global + keydown handler rather than a palette row. Building one is its own feature and is not + a prerequisite for this change — the exclusion here is the status quo, kept. +- **Backend manifest changes.** `ui.pages[].surface` is a different enum for a + different purpose and gains no new value. +- **De-duplicating repeated `registerNavItem` calls** for the same `(pluginId, id)`. + Pre-existing behaviour across every section. +- **The `utilities` nav section.** Only `insights` is opened to plugins; `utilities` + holds the first-party Settings entry and stays closed. +- **Real-locale translations** of `sidebar:morePluginItems` (`zh-cn`, `pt-pt`). Those + catalogs are translated out of band and only warn in CI; `en` and `pseudo` gate and + are in scope. +- **Any change to `kandev-plugin-rill`**, the private plugin that motivates this. It + switches its own `section` from `"main"` to `"sidebar-footer"` after this ships, in + its own repository.