From f8708da1e0c6261d98229ca2cf1901eee9bfdf2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fiere?= Date: Wed, 23 Sep 2026 12:00:23 +0200 Subject: [PATCH] feat(plugins): render a task menu action as a submenu (#3874) Co-authored-by: Carlos Florencio --- .../fixture-package/ui/bundle.js | 29 + apps/packages/plugin-sdk/src/index.ts | 32 + apps/web/AGENTS.md | 4 +- .../kanban-card-edit-submenu.test.tsx | 52 +- .../components/kanban-card-edit-submenu.tsx | 12 +- .../kanban-card-menu-grouping.test.tsx | 6 +- .../kanban-card-menu-items.test.tsx | 83 ++- .../web/components/kanban-card-menu-items.tsx | 71 +- apps/web/components/kanban-card-menu.tsx | 15 + .../kanban-card-plugin-context.test.tsx | 2 +- .../plugins/task-menu-actions.test.tsx | 621 ++++++++++++++++++ .../components/plugins/task-menu-actions.ts | 370 +++++++++-- .../plugins/task-menu-keys.test.tsx | 165 +++++ .../task-menu-registration-boundary.test.tsx | 66 ++ .../components/task-command-choices.test.tsx | 80 ++- apps/web/components/task-command-choices.tsx | 50 +- apps/web/components/task-command-items.tsx | 5 +- apps/web/components/task-commands.test.tsx | 15 + .../plugins/prompt-history-plugin/bundle.js | 29 + .../plugins/mobile-plugin-task-panel.spec.ts | 42 ++ .../kanban/task-actions-menu-entries.test.ts | 41 +- .../lib/kanban/task-actions-menu-entries.ts | 4 +- apps/web/lib/plugins/icons.test.ts | 84 ++- apps/web/lib/plugins/icons.ts | 50 +- apps/web/lib/plugins/registry.test.ts | 33 + apps/web/lib/plugins/registry.ts | 21 +- apps/web/lib/plugins/sdk-contract.test.ts | 47 ++ apps/web/lib/plugins/types.ts | 8 + docs/plans/plugin-task-menu-submenus/plan.md | 63 ++ .../task-01-host-submenu-contract.md | 97 +++ docs/plans/plugins/PLUGIN-API.md | 70 +- docs/public/plugins-authoring.md | 38 +- .../requirements/plugin-task-menu-submenus.md | 89 +++ .../plugin-task-menu-submenus.md | 192 ++++++ 34 files changed, 2491 insertions(+), 95 deletions(-) create mode 100644 apps/web/components/plugins/task-menu-actions.test.tsx create mode 100644 apps/web/components/plugins/task-menu-keys.test.tsx create mode 100644 apps/web/components/plugins/task-menu-registration-boundary.test.tsx create mode 100644 docs/plans/plugin-task-menu-submenus/plan.md create mode 100644 docs/plans/plugin-task-menu-submenus/task-01-host-submenu-contract.md create mode 100644 docs/specs/plugins/requirements/plugin-task-menu-submenus.md create mode 100644 docs/specs/plugins/system-design/plugin-task-menu-submenus.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 cdcf2a43ea..f463aefe2c 100644 --- a/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js +++ b/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js @@ -1018,6 +1018,35 @@ ); }, }); + registry.registerTaskMenuAction({ + id: "task-shortcuts", + label: "Task shortcuts", + group: "primary", + items: function () { + return [ + { + id: "record-presentation", + label: "Record menu presentation", + run: function (childContext) { + return host.storage.set( + "task", + childContext.taskId, + "primary-submenu-presentation", + childContext.presentation, + ); + }, + }, + ]; + }, + run: function (context) { + return host.storage.set( + "task", + context.taskId, + "primary-submenu-presentation", + context.presentation, + ); + }, + }); registry.registerKeybinding("open-demo", function () { function DemoModalContent() { diff --git a/apps/packages/plugin-sdk/src/index.ts b/apps/packages/plugin-sdk/src/index.ts index 9216e3cf0c..f4d5b8324a 100644 --- a/apps/packages/plugin-sdk/src/index.ts +++ b/apps/packages/plugin-sdk/src/index.ts @@ -422,12 +422,44 @@ export interface PluginTaskMenuContext { presentation: "desktop" | "mobile"; } +/** + * One child of a task menu action that declares `items`. Unlike the parent + * action, an item is never registered on its own: it exists only inside its + * parent's submenu, so it has no `group`, no `visible`, and its own nesting is + * not supported (one level deep). + */ +export interface TaskMenuSubItemRegistration { + /** Unique within the parent action; contributes to the menu entry's React key. */ + id: string; + label: string; + icon?: PluginIcon; + disabled?: boolean; + run(context: PluginTaskMenuContext): void | Promise; +} + export interface TaskMenuActionRegistration { id: string; label: string; icon?: PluginIcon; group: "edit" | "primary"; visible?(context: PluginTaskMenuContext): boolean; + /** + * Declaring this turns the action into a submenu: the host renders `label` + * as an unselectable submenu trigger and calls `items(context)` to get its + * children, in the returned order. It must be synchronous and cheap: the + * host cannot await a menu item, and it builds a card's or row's entries on + * every render — a card's dropdown and context variants are built from one + * evaluation, whether or not a menu is open — so an implementation that + * scans or sorts should memoize on the state it reads. + * + * `run` then serves as the fallback for a host that predates submenus (it + * ignores this field and renders the flat item) and for a build where + * `items` yields nothing usable: an empty list, or a throw (caught, + * logged, and treated as empty). Either way the action stays reachable + * instead of becoming a trigger with no children. + */ + items?(context: PluginTaskMenuContext): readonly TaskMenuSubItemRegistration[]; + /** A rejection is caught and logged; the menu closes either way. */ run(context: PluginTaskMenuContext): void | Promise; } diff --git a/apps/web/AGENTS.md b/apps/web/AGENTS.md index cf271c7f2c..f35a4e7c2a 100644 --- a/apps/web/AGENTS.md +++ b/apps/web/AGENTS.md @@ -280,7 +280,9 @@ and `lib/plugins/types.ts` are its detailed host implementation — all three mu `PluginErrorBoundary`; `mobileEnabled: true` also renders it via the phone bottom nav (`session-mobile-bottom-nav.tsx`) with `presentation: "mobile"`. - **Task contributions:** `registerTaskMenuAction({ group: "edit", ... })` adds card-only actions to - the `Edit` submenu. Group `"primary"` adds flat actions to card and desktop/mobile task-row menus. + the `Edit` submenu. Group `"primary"` adds top-level actions to card and desktop/mobile task-row + menus; declaring `items(context)` renders an action of either group as a submenu of its children + instead, with `run` kept as the flat fallback. Card indicator/tag slots stay card-specific; `task-row-metadata` is generic for sidebar and `/tasks` rows. - **Sidebar workspace actions:** `registerComponent("sidebar-workspace-actions", ...)` renders after Quick Terminal/Quick Chat in the desktop sidebar's New Task row and in the shared phone navigation sheet, forwarding `SidebarWorkspaceActionsSlotProps` with `presentation: "desktop" | "mobile"`; mobile plugin controls own a 44px touch target and accessible name. - **`host.storage`:** authenticated per-user key/value storage (`lib/plugins/host-api.ts`) backed by `/api/plugins/{id}/user-state/...` (`docs/decisions/2026-08-01-per-user-plugin-storage.md`); `subscribe` (`lib/plugins/user-state-sync.ts`) wraps `registerWsHandler` with own-plugin filtering and own-tab echo suppression via a per-tab `writerId`. diff --git a/apps/web/components/kanban-card-edit-submenu.test.tsx b/apps/web/components/kanban-card-edit-submenu.test.tsx index cf4b1b8585..9ea5143a4b 100644 --- a/apps/web/components/kanban-card-edit-submenu.test.tsx +++ b/apps/web/components/kanban-card-edit-submenu.test.tsx @@ -35,6 +35,11 @@ function registerEnhanceAction( overrides: { run?: () => Promise | void; visible?: (context: PluginTaskMenuContext) => boolean; + items?: (context: PluginTaskMenuContext) => readonly { + id: string; + label: string; + run: (context: PluginTaskMenuContext) => Promise | void; + }[]; } = {}, ) { pluginRegistry.forPlugin(PLUGIN_ID).registerTaskMenuAction({ @@ -43,6 +48,7 @@ function registerEnhanceAction( group: "edit", run: overrides.run ?? vi.fn(), ...(overrides.visible ? { visible: overrides.visible } : {}), + ...(overrides.items ? { items: overrides.items } : {}), }); } @@ -51,6 +57,29 @@ afterEach(() => { pluginRegistry.unregisterPlugin(PLUGIN_ID); }); +describe("buildEditMenuEntry — a registration the host cannot render", () => { + // Regression: the submenu-vs-flat decision was made from the number of visible + // registrations, so an action with no usable label wrapped the native Edit item + // in a submenu even though its own entry was dropped -- unlike the primary + // group, which omits such an action entirely. + it("keeps the flat Edit item when the only edit-group action is unusable", () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + pluginRegistry.forPlugin(PLUGIN_ID).registerTaskMenuAction({ + id: "broken", + label: "" as never, + group: "edit", + run: vi.fn(), + }); + + const entry = buildEditMenuEntry({ onEdit: vi.fn(), context: CONTEXT }); + + expect(entry.kind).toBe("item"); + expect(entry.kind === "item" ? entry.key : "").toBe("edit"); + expect(consoleErrorSpy).toHaveBeenCalled(); + consoleErrorSpy.mockRestore(); + }); +}); + describe("buildEditMenuEntry — AC10 (no plugin actions)", () => { it("renders the flat Edit item exactly as before", () => { const entry = renderEntry(vi.fn()); @@ -76,10 +105,31 @@ describe("buildEditMenuEntry — AC9 (plugin action registered)", () => { if (entry.kind === "submenu") { expect(entry.children.map((c) => c.key)).toEqual([ "edit-task", - `plugin-edit-${PLUGIN_ID}-enhance`, + `plugin-edit-${PLUGIN_ID}:enhance`, ]); } }); + + it("renders an edit action's children inside the Edit submenu", () => { + registerEnhanceAction({ + items: () => [{ id: "quick", label: "Quick enhance", run: vi.fn() }], + }); + + const entry = buildEditMenuEntry({ onEdit: vi.fn(), context: CONTEXT }); + expect(entry.kind).toBe("submenu"); + if (entry.kind !== "submenu") return; + + const pluginChild = entry.children.find( + (child) => child.key === `plugin-edit-${PLUGIN_ID}:enhance`, + ); + expect(pluginChild?.kind).toBe("submenu"); + if (pluginChild?.kind !== "submenu") return; + expect(pluginChild.children).toHaveLength(1); + const child = pluginChild.children[0]; + expect(child?.kind).toBe("item"); + if (child?.kind !== "item") return; + expect(child.label).toBe("Quick enhance"); + }); }); describe("buildEditMenuEntry — AC11 (run invoked with context, rejection caught, menu still closes)", () => { diff --git a/apps/web/components/kanban-card-edit-submenu.tsx b/apps/web/components/kanban-card-edit-submenu.tsx index 2d88324dd8..8a58bbad23 100644 --- a/apps/web/components/kanban-card-edit-submenu.tsx +++ b/apps/web/components/kanban-card-edit-submenu.tsx @@ -3,7 +3,7 @@ import { IconPencil } from "@tabler/icons-react"; import { t } from "@/lib/i18n"; import type { PluginTaskMenuContext } from "@/lib/plugins/types"; -import { runnablePluginMenuEntry, visiblePluginMenuActions } from "./plugins/task-menu-actions"; +import { pluginMenuEntry, visiblePluginMenuActions } from "./plugins/task-menu-actions"; import type { KanbanCardMenuEntry } from "./kanban-card-menu-items"; /** @@ -29,10 +29,16 @@ export function buildEditMenuEntry({ forceFlat?: boolean; }): KanbanCardMenuEntry { const pluginActions = forceFlat ? [] : visiblePluginMenuActions("edit", context); + // Built and filtered before choosing between the submenu and the flat item: an + // action the host cannot render contributes no child, so a list yielding none + // must leave the native Edit item as it is rather than wrap it in a submenu. + const pluginEntries = pluginActions + .map((action) => pluginMenuEntry(action, context, disabled)) + .filter((entry): entry is KanbanCardMenuEntry => entry !== null); const icon = ; - if (pluginActions.length === 0) { + if (pluginEntries.length === 0) { return { kind: "item", key: "edit", @@ -59,7 +65,7 @@ export function buildEditMenuEntry({ disabled: disabled || !onEdit, onSelect: onEdit, }, - ...pluginActions.map((action) => runnablePluginMenuEntry(action, context, disabled)), + ...pluginEntries, ], }; } diff --git a/apps/web/components/kanban-card-menu-grouping.test.tsx b/apps/web/components/kanban-card-menu-grouping.test.tsx index b2acdbf300..d652c46968 100644 --- a/apps/web/components/kanban-card-menu-grouping.test.tsx +++ b/apps/web/components/kanban-card-menu-grouping.test.tsx @@ -58,7 +58,7 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => const keys = entryKeys(entries); const sendToIndex = keys.indexOf("send-to-workflow"); - const primaryIndex = keys.indexOf(`plugin-primary-${PLUGIN_ID}-${ACTION_ID}`); + const primaryIndex = keys.indexOf(`plugin-primary-${PLUGIN_ID}:${ACTION_ID}`); const linkIndex = keys.indexOf("link"); expect(sendToIndex).toBeGreaterThanOrEqual(0); @@ -107,7 +107,7 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => "move-to", "send-to-workflow", "plugins-separator", - `plugin-primary-${PLUGIN_ID}-${ACTION_ID}`, + `plugin-primary-${PLUGIN_ID}:${ACTION_ID}`, "remove-separator", "archive", "delete", @@ -119,7 +119,7 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => const entries = buildKanbanCardMenuEntries({ workflows: [], stepsByWorkflowId: {} }); - expect(entryKeys(entries)).not.toContain(`plugin-primary-${PLUGIN_ID}-${ACTION_ID}`); + expect(entryKeys(entries)).not.toContain(`plugin-primary-${PLUGIN_ID}:${ACTION_ID}`); }); it("leaves the 'edit' group submenu unaffected by 'primary' group registrations", () => { diff --git a/apps/web/components/kanban-card-menu-items.test.tsx b/apps/web/components/kanban-card-menu-items.test.tsx index 334b4823d2..1e7d12d644 100644 --- a/apps/web/components/kanban-card-menu-items.test.tsx +++ b/apps/web/components/kanban-card-menu-items.test.tsx @@ -5,9 +5,11 @@ import { IconFlag } from "@tabler/icons-react"; import type { StoreApi } from "zustand"; import { DropdownMenu, DropdownMenuContent, DropdownMenuTrigger } from "@kandev/ui/dropdown-menu"; import { pluginRegistry } from "@/lib/plugins/registry"; +import type { PluginTaskMenuContext, TaskMenuSubItemRegistration } from "@/lib/plugins/types"; import { StateProvider, useAppStoreApi } from "@/components/state-provider"; import type { AppState } from "@/lib/state/store"; import { + buildCardPluginEntries, buildKanbanCardMenuEntries, KanbanCardDropdownMenuItems, useKanbanCardMoveTargets, @@ -22,6 +24,81 @@ const PluginBitbucketIcon = () => null; const WORKFLOW_ONE_NAME = "Workflow 1"; // Regression: React synthetic events bubble through the fiber tree from a Radix portal; without stopPropagation the parent Card's onClick fires instead of the confirm dialog. +/** + * A card builds its dropdown and context variants from one render with the + * same plugin inputs, so the plugin contributions must be built once: each + * plugin's `items()` is a callback into a foreign bundle, and evaluating it + * twice per render for identical children is the API's own documented + * anti-pattern ("memoize anything expensive"). + */ +describe("buildCardPluginEntries", () => { + const PLUGIN_ID = "kandev-plugin-tags"; + const CONTEXT: PluginTaskMenuContext = { + workspaceId: "ws-1", + taskId: "task-1", + taskTitle: "Fix the bug", + workflowStepId: "step-1", + presentation: "desktop", + }; + + afterEach(() => { + pluginRegistry.unregisterPlugin(PLUGIN_ID); + }); + + function registerItemsAction(items: () => readonly TaskMenuSubItemRegistration[]) { + pluginRegistry.forPlugin(PLUGIN_ID).registerTaskMenuAction({ + id: "add-tag", + label: "Add tag", + group: "primary", + items, + run: vi.fn(), + }); + } + + it("evaluates a plugin action's items() once when both variants share the result", () => { + const items = vi.fn(() => [{ id: "more", label: "More tags", run: vi.fn() }]); + registerItemsAction(items); + + const pluginEntries = buildCardPluginEntries({ pluginMenuContext: CONTEXT }); + const dropdown = buildKanbanCardMenuEntries({ + workflows: [], + stepsByWorkflowId: {}, + pluginMenuContext: CONTEXT, + pluginEntries, + }); + const contextMenu = buildKanbanCardMenuEntries({ + workflows: [], + stepsByWorkflowId: {}, + pluginMenuContext: CONTEXT, + pluginEntries, + }); + + expect(items).toHaveBeenCalledTimes(1); + const fromDropdown = dropdown.find( + (entry) => entry.key === `plugin-primary-${PLUGIN_ID}:add-tag`, + ); + const fromContext = contextMenu.find( + (entry) => entry.key === `plugin-primary-${PLUGIN_ID}:add-tag`, + ); + expect(fromDropdown).toBeDefined(); + expect(fromContext).toBe(fromDropdown); + }); + + it("still builds its own contributions when a caller passes none", () => { + const items = vi.fn(() => [{ id: "more", label: "More tags", run: vi.fn() }]); + registerItemsAction(items); + + const entries = buildKanbanCardMenuEntries({ + workflows: [], + stepsByWorkflowId: {}, + pluginMenuContext: CONTEXT, + }); + + expect(items).toHaveBeenCalledTimes(1); + expect(entries.some((entry) => entry.key === `plugin-primary-${PLUGIN_ID}:add-tag`)).toBe(true); + }); +}); + describe("KanbanCardDropdownMenuItems — click propagation", () => { function renderWithParent(entries: KanbanCardMenuEntry[], parentOnClick: () => void) { return render( @@ -205,7 +282,7 @@ describe("buildKanbanCardMenuEntries — !onEdit does not disable plugin edit ac (child) => child.kind === "item" && child.key === "edit-task", ); const pluginAction = editMenu.children.find( - (child) => child.kind === "item" && child.key === `plugin-edit-${PLUGIN_ID}-enhance`, + (child) => child.kind === "item" && child.key === `plugin-edit-${PLUGIN_ID}:enhance`, ); expect(editTask?.kind === "item" && editTask.disabled).toBe(true); @@ -253,7 +330,7 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => const keys = entryKeys(entries); const sendToIndex = keys.indexOf("send-to-workflow"); - const primaryIndex = keys.indexOf(`plugin-primary-${PLUGIN_ID}-quick-tag`); + const primaryIndex = keys.indexOf(`plugin-primary-${PLUGIN_ID}:quick-tag`); const archiveIndex = keys.indexOf("archive"); expect(sendToIndex).toBeGreaterThanOrEqual(0); @@ -281,7 +358,7 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => const entries = buildKanbanCardMenuEntries({ workflows: [], stepsByWorkflowId: {} }); - expect(entryKeys(entries)).not.toContain(`plugin-primary-${PLUGIN_ID}-quick-tag`); + expect(entryKeys(entries)).not.toContain(`plugin-primary-${PLUGIN_ID}:quick-tag`); }); it("leaves the 'edit' group submenu unaffected by 'primary' group registrations", () => { diff --git a/apps/web/components/kanban-card-menu-items.tsx b/apps/web/components/kanban-card-menu-items.tsx index 80ac12e385..151ea34b54 100644 --- a/apps/web/components/kanban-card-menu-items.tsx +++ b/apps/web/components/kanban-card-menu-items.tsx @@ -138,6 +138,14 @@ export type BuildKanbanCardMenuEntriesArgs = { onSendToWorkflow?: (workflowId: string, stepId: string) => void; /** Defaults to an empty-id context (no visible plugin actions match it in practice). */ pluginMenuContext?: PluginTaskMenuContext; + /** + * Plugin contributions already built from this call's `PluginEntryInputs`. + * Callers must build them with the same inputs they pass here: the entries + * carry the disabled state, the edit handler and the menu context, so reusing + * a set across an input mismatch silently renders one call's plugin entries + * with another's behaviour. Building them in place is always correct. + */ + pluginEntries?: CardPluginEntries; /** * Forces the flat Edit item regardless of registered plugin `edit`-group * actions. Group `edit` is a card-only plugin contract; surfaces outside @@ -158,6 +166,43 @@ export function resolvePluginMenuContext(context?: PluginTaskMenuContext): Plugi return context ?? EMPTY_PLUGIN_MENU_CONTEXT; } +/** The two plugin-derived pieces of a card menu: group "primary", and `Edit`. */ +export type CardPluginEntries = { primary: KanbanCardMenuEntry[]; edit: KanbanCardMenuEntry }; + +/** The card-menu inputs that decide those two entries. */ +export type PluginEntryInputs = Pick< + BuildKanbanCardMenuEntriesArgs, + | "disabled" + | "isDeleting" + | "isArchiving" + | "isDetaching" + | "onEdit" + | "forceFlatEdit" + | "pluginMenuContext" +>; + +/** + * Builds both plugin-derived card menu entries. `useKanbanCardMenus` builds one + * card's dropdown and context variants in a single render and they share every + * input here, so it calls this once and passes the result to both -- otherwise + * each plugin action's `items()` runs twice per render for the same children. + */ +export function buildCardPluginEntries(args: PluginEntryInputs): CardPluginEntries { + const isProcessing = Boolean( + args.disabled || args.isDeleting || args.isArchiving || args.isDetaching, + ); + const context = resolvePluginMenuContext(args.pluginMenuContext); + return { + primary: buildPrimaryPluginEntries({ disabled: isProcessing, context }), + edit: buildEditMenuEntry({ + onEdit: args.onEdit, + disabled: isProcessing, + context, + forceFlat: args.forceFlatEdit, + }), + }; +} + function StepBadges({ step, isCurrent }: { step: TaskMoveStep; isCurrent: boolean }) { const { t } = useTranslation(); const hasAutoStart = stepHasAutoStart(step); @@ -371,6 +416,7 @@ export function buildKanbanCardMenuEntries({ onMoveToStep, onSendToWorkflow, pluginMenuContext, + pluginEntries, forceFlatEdit, }: BuildKanbanCardMenuEntriesArgs): KanbanCardMenuEntry[] { const visibleWorkflows = workflows.filter((workflow) => !workflow.hidden); @@ -396,10 +442,16 @@ export function buildKanbanCardMenuEntries({ onSendToWorkflow, }); - const pluginEntries = buildPrimaryPluginEntries({ - disabled: isProcessing, - context: resolvePluginMenuContext(pluginMenuContext), - }); + // `forceFlatEdit` outranks a prebuilt bundle: those surfaces must never show the submenu form. + const pluginContributions = + pluginEntries && !forceFlatEdit + ? pluginEntries + : buildCardPluginEntries({ + disabled: isProcessing, + onEdit, + forceFlatEdit, + pluginMenuContext, + }); const linkEntry = buildLinkSubmenu({ disabled: isProcessing, @@ -418,14 +470,7 @@ export function buildKanbanCardMenuEntries({ { key: "priority", entries: priorityEntry ? [priorityEntry] : [] }, { key: "edit", - entries: [ - buildEditMenuEntry({ - onEdit, - disabled: isProcessing, - context: resolvePluginMenuContext(pluginMenuContext), - forceFlat: forceFlatEdit, - }), - ], + entries: [pluginContributions.edit], }, { key: "relationships", @@ -439,7 +484,7 @@ export function buildKanbanCardMenuEntries({ (entry): entry is KanbanCardMenuEntry => entry !== null, ), }, - { key: "plugins", entries: pluginEntries }, + { key: "plugins", entries: pluginContributions.primary }, { key: "remove", entries: [ diff --git a/apps/web/components/kanban-card-menu.tsx b/apps/web/components/kanban-card-menu.tsx index da7771ee97..94088bb62d 100644 --- a/apps/web/components/kanban-card-menu.tsx +++ b/apps/web/components/kanban-card-menu.tsx @@ -2,6 +2,7 @@ import { useRef } from "react"; import { + buildCardPluginEntries, buildKanbanCardMenuEntries, useKanbanCardMoveTargets, } from "@/components/kanban-card-menu-items"; @@ -220,6 +221,18 @@ export function useKanbanCardMenus({ }; const pluginMenuContext = buildPluginMenuContext(task, workspaceId, presentation); + // Both variants below share every input the plugin entries depend on -- the + // processing flags, the edit handler and the context -- so build them once: + // passing one result to both keeps each plugin action's items() to a single + // evaluation per card per render (see buildCardPluginEntries). + const pluginEntries = buildCardPluginEntries({ + disabled: menuBase.disabled, + isDeleting: menuBase.isDeleting, + isArchiving: menuBase.isArchiving, + isDetaching: menuBase.isDetaching, + onEdit: menuBase.onEdit, + pluginMenuContext, + }); return { ...dialogs, @@ -228,12 +241,14 @@ export function useKanbanCardMenus({ onMoveToStep: moveMenu.moveToStepFromDropdown, onSendToWorkflow: moveMenu.sendTaskToWorkflow, pluginMenuContext, + pluginEntries, }), contextMenuEntries: buildKanbanCardMenuEntries({ ...menuBase, onMoveToStep: moveMenu.moveSelectedToStep, onSendToWorkflow: moveMenu.sendSelectionToWorkflow, pluginMenuContext, + pluginEntries, }), isDetaching, detachAnchorRef, diff --git a/apps/web/components/kanban-card-plugin-context.test.tsx b/apps/web/components/kanban-card-plugin-context.test.tsx index f8214fb3d4..979b8a7489 100644 --- a/apps/web/components/kanban-card-plugin-context.test.tsx +++ b/apps/web/components/kanban-card-plugin-context.test.tsx @@ -27,7 +27,7 @@ async function invokePluginAction(presentation: "desktop" | "mobile") { if (entry.kind !== "submenu") throw new Error("expected Edit submenu"); const action = entry.children.find( (child) => - child.kind === "item" && child.key === `plugin-edit-${PLUGIN_ID}-action-${presentation}`, + child.kind === "item" && child.key === `plugin-edit-${PLUGIN_ID}:action-${presentation}`, ); if (action?.kind !== "item") throw new Error("expected plugin action"); diff --git a/apps/web/components/plugins/task-menu-actions.test.tsx b/apps/web/components/plugins/task-menu-actions.test.tsx new file mode 100644 index 0000000000..2e5458b721 --- /dev/null +++ b/apps/web/components/plugins/task-menu-actions.test.tsx @@ -0,0 +1,621 @@ +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { createElement } from "react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { DropdownMenu, DropdownMenuContent, DropdownMenuTrigger } from "@kandev/ui/dropdown-menu"; +import { pluginRegistry } from "@/lib/plugins/registry"; +import type { + PluginIcon, + PluginTaskMenuContext, + TaskMenuSubItemRegistration, +} from "@/lib/plugins/types"; +import { + buildCardPluginEntries, + KanbanCardDropdownMenuItems, + type KanbanCardMenuEntry, +} from "../kanban-card-menu-items"; +import { buildPrimaryPluginEntries } from "./task-menu-actions"; + +const PLUGIN_ID = "kandev-plugin-tags"; +const ACTION_ID = "add-tag"; +const ACTION_LABEL = "Add tag"; +const ACTION_KEY = `plugin-primary-${PLUGIN_ID}:${ACTION_ID}`; + +const CONTEXT: PluginTaskMenuContext = { + workspaceId: "ws-1", + taskId: "task-1", + taskTitle: "Fix the bug", + workflowStepId: "step-1", + presentation: "desktop", +}; + +function registerAction( + overrides: { + id?: string; + label?: string; + group?: "edit" | "primary"; + icon?: PluginIcon; + run?: (context: PluginTaskMenuContext) => void | Promise; + // Deliberately wider than the contract: this helper also registers results + // the types forbid (a promise, an unknown-returning wrapper), which is what + // a JavaScript bundle can actually send. + items?: (context: PluginTaskMenuContext) => unknown; + } = {}, +) { + const run = overrides.run ?? vi.fn(); + pluginRegistry.forPlugin(PLUGIN_ID).registerTaskMenuAction({ + id: overrides.id ?? ACTION_ID, + label: overrides.label ?? ACTION_LABEL, + group: overrides.group ?? "primary", + run, + ...(overrides.icon ? { icon: overrides.icon } : {}), + ...(overrides.items + ? { + items: overrides.items as ( + context: PluginTaskMenuContext, + ) => readonly TaskMenuSubItemRegistration[], + } + : {}), + }); + return run; +} + +function renderEntries(entries: KanbanCardMenuEntry[]) { + render( + + open + + + + , + ); +} + +/** Radix opens a submenu on a mouse pointer-move over its trigger. */ +async function openSubmenu(label: string, childLabel: string) { + fireEvent.pointerMove(screen.getByRole("menuitem", { name: label }), { pointerType: "mouse" }); + return screen.findByRole("menuitem", { name: childLabel }); +} + +afterEach(() => { + cleanup(); + pluginRegistry.unregisterPlugin(PLUGIN_ID); +}); + +describe("buildPrimaryPluginEntries — action without items", () => { + it("stays a flat item that runs run(context)", async () => { + const run = registerAction(); + renderEntries(buildPrimaryPluginEntries({ context: CONTEXT })); + + fireEvent.click(screen.getByRole("menuitem", { name: ACTION_LABEL })); + + await Promise.resolve(); + expect(run).toHaveBeenCalledWith(CONTEXT); + }); +}); + +describe("buildPrimaryPluginEntries — action declaring items", () => { + it("renders a submenu whose children are the resolved items, in order", () => { + registerAction({ + items: () => [ + { id: "more", label: "More tags", run: vi.fn() }, + { id: "blocked", label: "Blocked", run: vi.fn() }, + ], + }); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + + expect(entries.map((entry) => entry.kind)).toEqual(["submenu"]); + const entry = entries[0]; + if (entry.kind !== "submenu") return; + expect(entry.key).toBe(ACTION_KEY); + expect(entry.label).toBe(ACTION_LABEL); + expect(entry.children.map((child) => child.key)).toEqual([ + `${ACTION_KEY}#more`, + `${ACTION_KEY}#blocked`, + ]); + // Every child is a selectable item, not a separator or a nested submenu. + expect( + entry.children.map((child) => (child.kind === "item" ? child.label : child.kind)), + ).toEqual(["More tags", "Blocked"]); + }); + + it("runs the selected child with the menu context, not the action's fallback", async () => { + const more = vi.fn(); + const blocked = vi.fn(); + const run = registerAction({ + items: () => [ + { id: "more", label: "More tags", run: more }, + { id: "blocked", label: "Blocked", run: blocked }, + ], + }); + renderEntries(buildPrimaryPluginEntries({ context: CONTEXT })); + + const blockedItem = await openSubmenu(ACTION_LABEL, "Blocked"); + fireEvent.click(blockedItem); + + await Promise.resolve(); + expect(blocked).toHaveBeenCalledWith(CONTEXT); + expect(more).not.toHaveBeenCalled(); + expect(run).not.toHaveBeenCalled(); + }); + + it("keeps a disabled child unselectable", async () => { + const disabledRun = vi.fn(); + registerAction({ + items: () => [{ id: "blocked", label: "Blocked", disabled: true, run: disabledRun }], + }); + renderEntries(buildPrimaryPluginEntries({ context: CONTEXT })); + + const child = await openSubmenu(ACTION_LABEL, "Blocked"); + fireEvent.click(child); + + await Promise.resolve(); + expect(disabledRun).not.toHaveBeenCalled(); + }); + + it("disables every child while the entry itself is disabled", async () => { + const childRun = vi.fn(); + registerAction({ items: () => [{ id: "blocked", label: "Blocked", run: childRun }] }); + + const entry = buildPrimaryPluginEntries({ context: CONTEXT, disabled: true })[0]; + expect(entry.kind).toBe("submenu"); + if (entry.kind !== "submenu") return; + // A disabled trigger cannot be opened at all; rendering its children + // directly is what proves the disabled flag reached them. + expect(entry.disabled).toBe(true); + renderEntries(entry.children); + + fireEvent.click(screen.getByRole("menuitem", { name: "Blocked" })); + + await Promise.resolve(); + expect(childRun).not.toHaveBeenCalled(); + }); + + // A plugin's items() runs inside the host's menu build, so an unusable list + // must never produce a trigger with nothing behind it: the action's run() is + // what a host predating `items` would have used anyway. + it("logs and renders the flat fallback when items() throws", () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + registerAction({ + items: () => { + throw new Error("items() blew up"); + }, + }); + + expect(buildPrimaryPluginEntries({ context: CONTEXT }).map((entry) => entry.kind)).toEqual([ + "item", + ]); + expect(consoleErrorSpy).toHaveBeenCalled(); + consoleErrorSpy.mockRestore(); + }); + + it("renders the flat fallback when items() yields nothing", () => { + registerAction({ items: () => [] }); + + expect(buildPrimaryPluginEntries({ context: CONTEXT }).map((entry) => entry.kind)).toEqual([ + "item", + ]); + }); + + it("logs a rejecting child run without throwing", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + registerAction({ + items: () => [ + { id: "blocked", label: "Blocked", run: () => Promise.reject(new Error("boom")) }, + ], + }); + renderEntries(buildPrimaryPluginEntries({ context: CONTEXT })); + + const child = await openSubmenu(ACTION_LABEL, "Blocked"); + expect(() => fireEvent.click(child)).not.toThrow(); + + await vi.waitFor(() => expect(consoleErrorSpy).toHaveBeenCalled()); + consoleErrorSpy.mockRestore(); + }); +}); + +describe("buildPrimaryPluginEntries — malformed plugin results", () => { + // Plugin bundles are plain JavaScript: the registration types are a promise, + // not a guarantee. Every shape a broken plugin can produce must degrade to + // the flat fallback (or drop just the unusable child) instead of crashing the + // card render or handing React children it cannot key. Each case registers a + // distinct action id, since one broken action is reported once -- the host + // rebuilds entries on every render, so a per-build log would spam. + const fallbackCases: [string, () => unknown][] = [ + ["a null child", () => [null]], + ["a child without an id or label", () => [{ run: vi.fn() }]], + ["a child with a blank id", () => [{ id: " ", label: "X", run: vi.fn() }]], + ["a child with a blank label", () => [{ id: "x", label: " ", run: vi.fn() }]], + [ + "a child with a non-boolean disabled", + () => [{ id: "x", label: "X", run: vi.fn(), disabled: 1 }], + ], + ["a child without a run", () => [{ id: "x", label: "X" }]], + ["a child with an unusable icon", () => [{ id: "x", label: "X", icon: {}, run: vi.fn() }]], + ["a non-array result", () => "nope"], + ["a promise", () => Promise.resolve([{ id: "x", label: "X", run: vi.fn() }])], + [ + "an array whose methods were replaced", + () => + new Proxy([{ id: "x", label: "X", run: vi.fn() }], { + get() { + throw new Error("trap"); + }, + }), + ], + [ + "a thenable that throws on lookup", + () => + Object.defineProperty({}, "then", { + get() { + throw new Error("then getter"); + }, + }), + ], + ]; + + for (const [index, [label, items]] of fallbackCases.entries()) { + it(`falls back to the flat item and reports ${label} once`, async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + registerAction({ id: `broken-${index}`, items }); + + expect(buildPrimaryPluginEntries({ context: CONTEXT }).map((entry) => entry.kind)).toEqual([ + "item", + ]); + // A second build must not log again: the defect is permanent for a + // loaded bundle, and this callback runs on every render. + expect(buildPrimaryPluginEntries({ context: CONTEXT }).map((entry) => entry.kind)).toEqual([ + "item", + ]); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + + consoleErrorSpy.mockRestore(); + // A rejected promise must not escape as an unhandled rejection; one + // macrotask is enough for vitest to surface it if it were unobserved. + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + } + + it("observes a rejected promise, so it cannot escape as an unhandled rejection", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + registerAction({ id: "rejecting", items: () => Promise.reject(new Error("boom")) }); + + expect(buildPrimaryPluginEntries({ context: CONTEXT }).map((entry) => entry.kind)).toEqual([ + "item", + ]); + // Vitest fails the file on an unhandled rejection, so reaching the end of + // this test at all proves the rejection was observed -- but the log has to + // say so too, which the deleted-observation mutation otherwise hides. + await vi.waitFor(() => + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("items() rejected"), + expect.any(Error), + ), + ); + consoleErrorSpy.mockRestore(); + }); +}); + +describe("buildPrimaryPluginEntries — unreadable and repeated children", () => { + it("reports a repeated child id and keeps the first entry", () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const first = { id: "dup", label: "First", run: vi.fn() }; + registerAction({ + id: "duplicated", + items: () => [first, { id: "dup", label: "Second", run: vi.fn() }], + }); + + const entry = buildPrimaryPluginEntries({ context: CONTEXT })[0]; + + expect(entry.kind).toBe("submenu"); + if (entry.kind !== "submenu") return; + expect(entry.children.map((child) => child.key)).toEqual([ + `plugin-primary-${PLUGIN_ID}:duplicated#dup`, + ]); + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("repeated a child id"), + "dup", + ); + consoleErrorSpy.mockRestore(); + }); + + it("reports a later defect of a different kind on the same action", () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + let impl: () => unknown = () => [null]; + registerAction({ id: "varies", items: () => impl() }); + + buildPrimaryPluginEntries({ context: CONTEXT }); + impl = () => { + throw new Error("now it throws"); + }; + buildPrimaryPluginEntries({ context: CONTEXT }); + + expect(consoleErrorSpy.mock.calls.map((call) => String(call[0]))).toEqual([ + expect.stringContaining("unusable child"), + expect.stringContaining("could not be read"), + ]); + consoleErrorSpy.mockRestore(); + }); + + it("falls back when a child cannot be read at all", () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + const hostile = { id: "x", label: "X", run: vi.fn() }; + Object.defineProperty(hostile, "icon", { + enumerable: true, + get() { + throw new Error("getter blew up"); + }, + }); + registerAction({ id: "hostile-child", items: () => [hostile] }); + + let entries: KanbanCardMenuEntry[] = []; + expect(() => { + entries = buildPrimaryPluginEntries({ context: CONTEXT }); + }).not.toThrow(); + expect(entries.map((entry) => entry.kind)).toEqual(["item"]); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + consoleErrorSpy.mockRestore(); + }); + + it("keeps the usable children and drops only the broken ones", () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + registerAction({ + id: "partly-broken", + items: () => [ + { id: "gone", label: "Gone", run: vi.fn() }, + { id: "", label: "No id", run: vi.fn() }, + { id: "ok", label: "Ok", run: vi.fn() }, + { id: "gone", label: "Duplicate id", run: vi.fn() }, + ], + }); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + + expect(entries.map((entry) => entry.kind)).toEqual(["submenu"]); + const entry = entries[0]; + if (entry.kind !== "submenu") return; + expect(entry.children.map((child) => child.key)).toEqual([ + `plugin-primary-${PLUGIN_ID}:partly-broken#gone`, + `plugin-primary-${PLUGIN_ID}:partly-broken#ok`, + ]); + consoleErrorSpy.mockRestore(); + }); +}); + +describe("buildPrimaryPluginEntries — unusable registrations and array-likes", () => { + // A registration is plugin-authored data as much as its `items()` result: an + // action whose own fields cannot be read degrades to no entry instead of + // handing React an object child, and a result whose array methods are + // hostile must be copied rather than called. + it("omits an action whose label is not a usable string, and logs once", () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + // Registered around the helper, which defaults a missing label. + const broken = (id: string, label: unknown) => ({ id, label, group: "primary", run: vi.fn() }); + pluginRegistry + .forPlugin(PLUGIN_ID) + .registerTaskMenuAction(broken("blank-label", " ") as never); + pluginRegistry + .forPlugin(PLUGIN_ID) + .registerTaskMenuAction(broken("object-label", { toString: () => "not a label" }) as never); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + + expect(entries).toEqual([]); + // One report per action: the two registrations fail the same way but are + // different action ids. + expect(consoleErrorSpy).toHaveBeenCalledTimes(2); + consoleErrorSpy.mockRestore(); + }); + + it("renders an action whose icon is an object that refuses to be coerced", () => { + // The icon resolver looks a name up in its curated map, so a non-string + // icon is no name at all: the entry keeps rendering with the fallback glyph + // instead of throwing out of the card's render while the map coerces the + // object to a key. + const icon = { + toString() { + throw new Error("key coercion"); + }, + }; + registerAction({ + id: "object-icon", + label: "Object icon", + icon: icon as unknown as PluginIcon, + }); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + + expect(entries.map((entry) => entry.kind)).toEqual(["item"]); + const entry = entries[0]; + if (entry.kind !== "item") return; + expect(entry.label).toBe("Object icon"); + expect(entry.icon).toBeDefined(); + }); + + it("copies a hostile array-like instead of calling its methods", () => { + class HostileArray extends Array { + forEach(): void { + throw new Error("forEach is not yours to call"); + } + map(): never { + throw new Error("map is not yours to call"); + } + } + const items = HostileArray.from([{ id: "child", label: "Child", run: vi.fn() }]); + registerAction({ id: "hostile-array", items: () => items }); + + const entry = buildPrimaryPluginEntries({ context: CONTEXT })[0]; + + expect(entry.kind).toBe("submenu"); + if (entry.kind !== "submenu") return; + expect(entry.children.map((child) => child.key)).toEqual([ + `plugin-primary-${PLUGIN_ID}:hostile-array#child`, + ]); + }); + + it("treats every processing flag as disabling the plugin entries, on both paths", () => { + registerAction({ id: "flagged", items: () => [{ id: "child", label: "Child", run: vi.fn() }] }); + + for (const flag of ["disabled", "isDeleting", "isArchiving", "isDetaching"] as const) { + const inputs = { pluginMenuContext: CONTEXT, [flag]: true }; + const prebuilt = buildCardPluginEntries(inputs).primary.find( + (entry) => entry.key === `plugin-primary-${PLUGIN_ID}:flagged`, + ); + const internal = buildPrimaryPluginEntries({ context: CONTEXT, disabled: true })[0]; + // Both paths must agree, and a submenu's children inherit the state. + const prebuiltSubmenu = prebuilt?.kind === "submenu" ? prebuilt : undefined; + const internalSubmenu = internal.kind === "submenu" ? internal : undefined; + expect(prebuiltSubmenu?.disabled, flag).toBe(true); + expect(internalSubmenu?.disabled, flag).toBe(true); + expect( + prebuiltSubmenu?.children.map((child) => + child.kind === "item" ? child.disabled : undefined, + ), + flag, + ).toEqual([true]); + } + }); +}); + +describe("buildPrimaryPluginEntries — shapes that must survive the boundary", () => { + it("keeps a child whose icon is an exotic component", () => { + // React's memo/forwardRef/lazy return objects, not functions, and an icon + // set -- which host-api tells plugins to bundle -- is built from + // forwardRef, so rejecting objects would drop a contract-valid child. + // memo() returns a non-callable object; a function carrying $$typeof would + // not reproduce the shape that `typeof icon === "function"` accepts. + const ExoticIcon = { $$typeof: Symbol.for("react.memo"), type: () => null }; + registerAction({ + id: "exotic-icon", + items: () => [{ id: "ok", label: "Ok", icon: ExoticIcon as never, run: vi.fn() }], + }); + + const entry = buildPrimaryPluginEntries({ context: CONTEXT })[0]; + expect(entry?.kind).toBe("submenu"); + if (entry?.kind !== "submenu") return; + expect(entry.children.map((child) => child.key)).toEqual([ + `plugin-primary-${PLUGIN_ID}:exotic-icon#ok`, + ]); + }); + + it("keeps a child with an absent or null icon and drops an unusable one", () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + registerAction({ + id: "icon-shapes", + items: () => [ + { id: "null-icon", label: "Null icon", icon: null, run: vi.fn() }, + { id: "no-icon", label: "No icon", run: vi.fn() }, + { id: "array-icon", label: "Array icon", icon: [] as never, run: vi.fn() }, + { id: "object-icon", label: "Object icon", icon: {} as never, run: vi.fn() }, + { id: "null-disabled", label: "Null disabled", disabled: null as never, run: vi.fn() }, + ], + }); + + const entry = buildPrimaryPluginEntries({ context: CONTEXT })[0]; + expect(entry?.kind).toBe("submenu"); + if (entry?.kind !== "submenu") return; + expect(entry.children.map((child) => child.key)).toEqual([ + `plugin-primary-${PLUGIN_ID}:icon-shapes#null-icon`, + `plugin-primary-${PLUGIN_ID}:icon-shapes#no-icon`, + `plugin-primary-${PLUGIN_ID}:icon-shapes#null-disabled`, + ]); + // The two unusable shapes are one defect of the same kind: one report. + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + consoleErrorSpy.mockRestore(); + }); +}); + +describe("buildPrimaryPluginEntries — ids the encoder must survive", () => { + // Regression: `encodeURIComponent` raises `URIError` on a lone surrogate, which + // a truncated astral character (`slice`, `[0]`) produces. The throw left + // `pluginMenuEntry`, so it escaped the builder called during a card's render and + // unmounted the whole route -- and a lone surrogate is a non-blank string, so the + // id guard did not catch it either. + it("builds an entry whose id is a lone surrogate instead of throwing", () => { + const loneSurrogate = "\ud83c"; + registerAction({ + id: loneSurrogate, + items: () => [{ id: loneSurrogate, label: "Child", run: vi.fn() }], + }); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + + expect(entries).toHaveLength(1); + expect(entries[0]?.key).toContain(loneSurrogate); + expect(entries[0]?.kind === "submenu" ? entries[0].children[0]?.key : "").toContain( + loneSurrogate, + ); + }); + + it("keeps an escaped unit from running into the literal after it", () => { + // Regression from review: an encoder that emits `%XX`-style tokens cannot use a + // bare `%` as the part separator, because the separator is then indistinguishable + // from the start of a token. Only `%`, `:` and `#` are escaped now. + registerAction({ id: "%0", label: "Percent" }); + registerAction({ id: "\u0250", label: "Unit" }); + // The same class one width up: a two-digit escape plus one literal hex digit. + registerAction({ id: "\u00e9a", label: "Accented" }); + registerAction({ id: "\u0e9a", label: "Thai" }); + registerAction({ + id: "children", + label: "Children", + items: () => [ + { id: "\u00e9a", label: "Accented child", run: vi.fn() }, + { id: "\u0e9a", label: "Thai child", run: vi.fn() }, + ], + }); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + const keys = entries.flatMap((entry) => + entry + ? [entry.key, ...(entry.kind === "submenu" ? entry.children.map((c) => c.key) : [])] + : [], + ); + + expect(keys).toHaveLength(7); + expect(new Set(keys).size).toBe(7); + }); + + it("keeps a lone-surrogate id distinct from a valid astral one", () => { + registerAction({ id: "\ud83c", label: "Half" }); + registerAction({ id: "\ud83c\udf89", label: "Whole" }); + + const keys = buildPrimaryPluginEntries({ context: CONTEXT }).map((entry) => entry?.key); + + expect(new Set(keys).size).toBe(2); + }); +}); + +describe("buildPrimaryPluginEntries — mixed registrations", () => { + it("renders a submenu action as one entry and keeps the rest flat", () => { + registerAction({ id: "add-tag", items: () => [{ id: "more", label: "More", run: vi.fn() }] }); + registerAction({ id: "other", label: "Other action" }); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + + expect(entries.map((entry) => entry.kind)).toEqual(["submenu", "item"]); + expect(entries.map((entry) => entry.key)).toEqual([ + ACTION_KEY, + `plugin-primary-${PLUGIN_ID}:other`, + ]); + }); +}); + +describe("buildPrimaryPluginEntries — icon resolution", () => { + // Regression: icons registered before the name/component resolution landed + // are ready-made elements (kandev-plugin-tags ships one). Reading one as a + // name looked up `PLUGIN_ICONS["[object Object]"]` and rendered the puzzle + // fallback glyph instead of the plugin's own icon. + it("renders an element-form icon instead of the fallback glyph", () => { + const elementIcon = createElement( + "svg", + { "data-testid": "plugin-menu-icon", viewBox: "0 0 24 24" }, + createElement("path", { d: "M4 4h16" }), + ); + registerAction({ icon: elementIcon as unknown as PluginIcon }); + + renderEntries(buildPrimaryPluginEntries({ context: CONTEXT })); + + expect(screen.getByTestId("plugin-menu-icon")).toBeTruthy(); + }); +}); diff --git a/apps/web/components/plugins/task-menu-actions.ts b/apps/web/components/plugins/task-menu-actions.ts index 23da951d4c..9a926a4816 100644 --- a/apps/web/components/plugins/task-menu-actions.ts +++ b/apps/web/components/plugins/task-menu-actions.ts @@ -1,9 +1,13 @@ -import { createElement } from "react"; +import { createElement, isValidElement, type ReactNode } from "react"; import { pluginRegistry } from "@/lib/plugins/registry"; -import { resolvePluginIcon } from "@/lib/plugins/icons"; -import type { PluginTaskMenuContext } from "@/lib/plugins/types"; +import { isPluginIconComponent, resolvePluginIcon } from "@/lib/plugins/icons"; +import type { PluginTaskMenuActionRegistration } from "@/lib/plugins/registry-registration-types"; +import type { + PluginIcon, + PluginTaskMenuContext, + TaskMenuSubItemRegistration, +} from "@/lib/plugins/types"; import type { KanbanCardMenuEntry } from "../kanban-card-menu-items"; -type PluginTaskMenuAction = ReturnType[number]; /** * Registered actions for `group`, filtered by `visible(context)` (default @@ -11,16 +15,16 @@ type PluginTaskMenuAction = ReturnType * treated as hidden — the same defensive handling as `run`'s rejection. */ export function visiblePluginMenuActions( - group: PluginTaskMenuAction["group"], + group: PluginTaskMenuActionRegistration["group"], context: PluginTaskMenuContext, -): PluginTaskMenuAction[] { +): PluginTaskMenuActionRegistration[] { return pluginRegistry.getTaskMenuActions(group).filter((action) => { if (!action.visible) return true; try { return action.visible(context); } catch (error: unknown) { console.error( - `[plugins] task menu action "${action.pluginId}:${action.id}" visible() threw`, + `[plugins] task menu action "${describeAction(action)}" visible() threw`, error, ); return false; @@ -29,41 +33,328 @@ export function visiblePluginMenuActions( } /** - * Builds a runnable menu entry for a plugin task menu action. A `run` that - * throws or rejects is caught and logged; the menu still closes because - * `DropdownMenuItem`/`ContextMenuItem` already close on select regardless - * of the async result. + * Plugin icons get the same sizing every neighbouring native menu icon uses. + * A ready-made element — the shape registered before icon names/components + * were resolved, and still shipped by kandev-plugin-tags — renders as-is: + * passing it to the resolver would treat it as a name, miss the curated map, + * and silently replace the plugin's own glyph with the fallback puzzle. */ -export function runnablePluginMenuEntry( - action: PluginTaskMenuAction, +function pluginMenuIcon(icon?: PluginIcon): ReactNode { + if (!icon) return undefined; + try { + if (isValidElement(icon)) return icon; + return createElement(resolvePluginIcon(icon), { className: "mr-2 h-4 w-4" }); + } catch { + // `isValidElement` reads `$$typeof` again, so an icon whose accessors answer + // once and then throw must mean "no icon" rather than a failed render. + return undefined; + } +} + +/** + * Plugin bundles are plain JavaScript, so the registration types are a + * promise, not a guarantee. One log per action *and kind* is enough: `items()` + * runs on every menu build, so a permanently malformed registration would + * otherwise log on every render, while a later defect of a different kind on + * the same action still reports. The set is never cleared, so the same kind + * after re-registration stays silent for the life of the page. + * The registry guards its own read of each registration before this module + * receives a copy. The guards below handle malformed values in that copy. + */ +const loggedMenuDefects = new Set(); + +/** + * A plugin value's printable form. Template interpolation would call `ToString`, + * which throws for a value that has no string form -- a `Symbol` id or a + * null-prototype object -- and the omission paths are exactly where such a value + * arrives, so the log must not be able to throw. + */ +function printable(value: unknown): string { + if (typeof value === "string") return value; + try { + return String(value); + } catch { + // i18n-exempt: log placeholder for a value with no string form, never rendered. + return ""; + } +} + +/** The action's identity as a log label, read defensively for the same reason. */ +function describeAction(action: PluginTaskMenuActionRegistration): string { + try { + return `${printable(action.pluginId)}:${printable(action.id)}`; + } catch { + // i18n-exempt: log placeholder for a registration that cannot be read, never rendered. + return ""; + } +} + +function logMenuDefect( + action: PluginTaskMenuActionRegistration, + kind: string, + detail?: unknown, +): void { + const actionLabel = describeAction(action); + const key = `${actionLabel}:${kind}`; + if (loggedMenuDefects.has(key)) return; + loggedMenuDefects.add(key); + console.error(`[plugins] task menu action "${actionLabel}" ${kind}`, detail); +} + +/** + * The action's own label, read once and only when it is a non-blank string. + * A registration is plugin-authored data too, so a missing or hostile accessor + * omits the entry (and reports it) rather than handing React an object child. + */ +/** + * Escapes the three characters a menu key gives meaning to -- `%` first, so an + * escape already inside an id cannot alias one this adds, then `:` and `#` -- and + * leaves every other character verbatim. + * + * That is what makes the key injective for any plugin-authored string: every `%` + * in the result starts one of exactly three two-character escapes, so decoding is + * unambiguous, and neither the part separator (`:`) nor the child separator (`#`) + * survives raw inside a part. A `%XX`-style per-code-unit escape cannot do this, + * because a bare `%` separator is then indistinguishable from the start of an + * escape: `p` + `abcd\uABCD` and `p\uABCD` + `abcd` would share one key. + * + * It also cannot throw, which is why it replaced `encodeURIComponent` here: that + * raises `URIError` on a lone surrogate, and an exception on this path escapes a + * card's render, where no error boundary catches it. + */ +function encodeKeyPart(value: string): string { + return value.replace(/%/g, "%25").replace(/:/g, "%3A").replace(/#/g, "%23"); +} + +/** The action's own id, read once and only when it is a non-blank string. */ +function readActionId(action: PluginTaskMenuActionRegistration): string | null { + try { + const { id } = action; + return typeof id === "string" && id.trim().length > 0 ? id : null; + } catch { + return null; + } +} + +function readActionLabel(action: PluginTaskMenuActionRegistration): string | null { + try { + const { label } = action; + return typeof label === "string" && label.trim().length > 0 ? label : null; + } catch { + return null; + } +} + +/** The action's own icon, read once; an unreadable one simply means no icon. */ +function readActionIcon(action: PluginTaskMenuActionRegistration): ReactNode { + try { + return pluginMenuIcon(action.icon); + } catch { + logMenuDefect(action, "registration icon could not be read"); + return undefined; + } +} + +/** + * An icon the menu can render: a curated name, a plugin-owned component + * (including React's exotic components, which are objects at run time), or the + * menu-only ready-made element. Everything else is a defect, while an absent + * icon is null or undefined in a plain bundle. + */ +function isRenderableIcon(icon: unknown): boolean { + if (icon === undefined || icon === null) return true; + return typeof icon === "string" || isPluginIconComponent(icon) || isValidElement(icon); +} + +/** One child, read once into the plain values the menu actually renders. */ +type SubItemSnapshot = { + id: string; + label: string; + icon?: PluginIcon; + disabled?: boolean; + run: (context: PluginTaskMenuContext) => void | Promise; +}; + +/** + * Reads one plugin-supplied child into a snapshot the render can trust: a + * non-blank id and label, a callable run, and optional fields of the shapes + * the entry builder understands (a curated name, a component, or a ready-made + * element -- never an arbitrary object, which the icon resolver would coerce + * into a key). Everything the render will read is read here, inside the + * catch, so a throwing getter or a Proxy fails this child instead of the + * card's render, and nothing is read twice. + */ +function readSubItem(item: unknown): SubItemSnapshot | null { + if (!item || typeof item !== "object") return null; + try { + const { id, label, icon, disabled, run } = item as Partial; + if (typeof id !== "string" || id.trim().length === 0) return null; + if (typeof label !== "string" || label.trim().length === 0) return null; + if (typeof run !== "function") return null; + if (disabled !== undefined && disabled !== null && typeof disabled !== "boolean") return null; + if (!isRenderableIcon(icon)) return null; + return { id, label, icon: icon ?? undefined, disabled: disabled ?? undefined, run }; + } catch { + return null; + } +} + +/** + * The children of an action that declares `items`, or null when the action + * has no usable submenu — leaving `run` as the entry's behavior. + * + * That fallback is the whole defensive boundary, because `items()` is a + * plugin callback running inside the host's menu build, and every way its + * result can be unreadable is the same answer: a throw, a thenable (the + * contract is synchronous), a non-array, a result whose entries cannot be + * rendered (blank/absent id or label, no callable run, an icon of the wrong + * shape), a child that cannot even be read (a throwing getter, a Proxy), and + * a repeated id all degrade to the flat item — or drop just that child — + * instead of crashing the card's render, producing a trigger nothing can + * open, or handing React children it cannot key. The body is one `try` on + * purpose: a plugin object is free to throw from any property access, + * including the array methods and the `then` lookup this function would + * otherwise perform on it. + */ +function pluginSubItems( + action: PluginTaskMenuActionRegistration, + context: PluginTaskMenuContext, +): readonly SubItemSnapshot[] | null { + try { + if (typeof action.items !== "function") return null; + const result: unknown = action.items(context); + + if (result && typeof (result as { then?: unknown }).then === "function") { + logMenuDefect(action, "items() returned a promise; it must be synchronous"); + // Observe the rejection so an out-of-contract async callback cannot + // surface as an unhandled rejection out of the host's menu build. + void Promise.resolve(result).catch((error: unknown) => { + logMenuDefect(action, "items() rejected", error); + }); + return null; + } + + if (!Array.isArray(result)) { + // null/undefined means "no children"; anything else is out of contract. + if (result !== null && result !== undefined) { + logMenuDefect(action, "items() must return an array"); + } + return null; + } + + // Array.from copies through the array's own length and indices, so no + // array method is looked up on the plugin's object. + const list = Array.from(result as readonly unknown[]); + const seen = new Set(); + const usable: SubItemSnapshot[] = []; + list.forEach((item) => { + const child = readSubItem(item); + if (!child) { + logMenuDefect(action, "items() returned an unusable child", item); + return; + } + if (seen.has(child.id)) { + logMenuDefect(action, "items() repeated a child id", child.id); + return; + } + seen.add(child.id); + usable.push(child); + }); + return usable.length > 0 ? usable : null; + } catch (error: unknown) { + logMenuDefect(action, "items() could not be read", error); + return null; + } +} + +/** + * `Promise.resolve().then(() => run(context))` — not + * `Promise.resolve(run(context))` — so a *synchronous* throw inside run() + * also lands in the `.catch()` below. Calling run directly as the + * Promise.resolve() argument still throws before that expression finishes + * evaluating, escaping past .catch() entirely and straight out of this + * onSelect handler. `subject` names the failing part of a submenu action. + */ +function runPluginCallback( + action: PluginTaskMenuActionRegistration, + run: (context: PluginTaskMenuContext) => void | Promise, + context: PluginTaskMenuContext, + subject = "", +): void { + Promise.resolve() + .then(() => run(context)) + .catch((error: unknown) => { + console.error( + `[plugins] task menu action "${action.pluginId}:${action.id}"${subject} failed`, + error, + ); + }); +} + +/** + * Builds the menu entry for one plugin task menu action: a submenu when the + * action declares a non-empty `items(context)`, otherwise the flat item it + * has always been (and still is on a host that predates submenus, which + * ignores `items` and reads only `label`/`icon`/`run`). + */ +export function pluginMenuEntry( + action: PluginTaskMenuActionRegistration, context: PluginTaskMenuContext, disabled?: boolean, keyPrefix = "plugin-edit", -): KanbanCardMenuEntry { +): KanbanCardMenuEntry | null { + const label = readActionLabel(action); + if (label === null) { + logMenuDefect(action, "registration has no usable label"); + return null; + } + const actionId = readActionId(action); + if (actionId === null) { + logMenuDefect(action, "registration has no usable id"); + return null; + } + const icon = readActionIcon(action); + // Every plugin-controlled part is percent-encoded, so a dash-join cannot spell + // another action's key (`p` + `q-x` vs `p-q` + `x`) and no part can contain the + // separators: the palette flattens every plugin entry into one list where the + // key is both a React key and cmdk's value. + const key = `${keyPrefix}-${encodeKeyPart(action.pluginId)}:${encodeKeyPart(actionId)}`; + const items = pluginSubItems(action, context); + + if (items) { + return { + kind: "submenu", + key, + icon, + label, + disabled, + children: items.map((item) => ({ + kind: "item", + // The id is plugin-controlled and the action key is itself a dash-join + // of plugin-controlled ids, so `-${id}` can spell another action's child + // key exactly; the palette flattens every plugin entry into one list and + // uses this key as both its React key and cmdk's value. The separator + // stays unambiguous because an id's own `#` is percent-encoded. + key: `${key}#${encodeKeyPart(item.id)}`, + icon: pluginMenuIcon(item.icon), + label: item.label, + disabled: disabled || item.disabled, + onSelect: () => runPluginCallback(action, item.run, context, ` item "${item.id}"`), + })), + }; + } + + if (typeof action.run !== "function") { + logMenuDefect(action, "registration has no run"); + return null; + } return { kind: "item", - key: `${keyPrefix}-${action.pluginId}-${action.id}`, - icon: action.icon - ? createElement(resolvePluginIcon(action.icon), { className: "mr-2 h-4 w-4" }) - : undefined, - label: action.label, + key, + icon, + label, disabled, - onSelect: () => { - // Promise.resolve().then(() => action.run(context)) — not - // Promise.resolve(action.run(context)) — so a *synchronous* throw - // inside run() also lands in the .catch() below. Calling action.run - // directly as the Promise.resolve() argument still throws before that - // expression finishes evaluating, escaping past .catch() entirely and - // straight out of this onSelect handler. - Promise.resolve() - .then(() => action.run(context)) - .catch((error: unknown) => { - console.error( - `[plugins] task menu action "${action.pluginId}:${action.id}" failed`, - error, - ); - }); - }, + onSelect: () => runPluginCallback(action, action.run, context), }; } @@ -71,7 +362,8 @@ export function runnablePluginMenuEntry( * Builds flat, top-level menu entries for group "primary" task menu * actions, in registration order. Unlike group "edit" (nested in the Edit * submenu), these render directly in the card menu — after the movement - * group and before the Archive/Delete removal group. + * group and before the Archive/Delete removal group. An action that declares + * `items` is the one exception: it renders as its own submenu. */ export function buildPrimaryPluginEntries({ disabled, @@ -80,7 +372,7 @@ export function buildPrimaryPluginEntries({ disabled?: boolean; context: PluginTaskMenuContext; }): KanbanCardMenuEntry[] { - return visiblePluginMenuActions("primary", context).map((action) => - runnablePluginMenuEntry(action, context, disabled, "plugin-primary"), - ); + return visiblePluginMenuActions("primary", context) + .map((action) => pluginMenuEntry(action, context, disabled, "plugin-primary")) + .filter((entry): entry is KanbanCardMenuEntry => entry !== null); } diff --git a/apps/web/components/plugins/task-menu-keys.test.tsx b/apps/web/components/plugins/task-menu-keys.test.tsx new file mode 100644 index 0000000000..61d1a25583 --- /dev/null +++ b/apps/web/components/plugins/task-menu-keys.test.tsx @@ -0,0 +1,165 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { pluginRegistry } from "@/lib/plugins/registry"; +import type { PluginTaskMenuContext } from "@/lib/plugins/types"; +import { buildPrimaryPluginEntries } from "./task-menu-actions"; + +// Key identity. A plugin entry key is a palette row's React key and cmdk value, +// so it has to be unique for any strings a bundle can author -- including the +// shapes that make a naive escape and a bare separator collide. +const PLUGIN_ID = "kandev-plugin-keys"; + +const CONTEXT: PluginTaskMenuContext = { + workspaceId: "ws-1", + taskId: "task-1", + taskTitle: "Fix the bug", + workflowStepId: "step-1", + presentation: "desktop", +}; + +afterEach(() => { + pluginRegistry.unregisterPlugin(PLUGIN_ID); + pluginRegistry.unregisterPlugin("p"); + pluginRegistry.unregisterPlugin("p-a"); + pluginRegistry.unregisterPlugin("p-q"); + pluginRegistry.unregisterPlugin("p-pick"); + pluginRegistry.unregisterPlugin("p\uABCD"); +}); + +describe("plugin entry keys — escapes and separators", () => { + it("keeps keys distinct when an id ends where another's escape token begins", () => { + // The separator cannot be read as the start of an escape: with a `%XX`-style + // token and a bare `%` separator, plugin `p\uABCD` with action `abcd` and + // plugin `p` with action `abcd\uABCD` both produced `plugin-primary-p%abcd%abcd`. + pluginRegistry.forPlugin("p\uABCD").registerTaskMenuAction({ + id: "abcd", + label: "Shifted", + group: "primary", + run: vi.fn(), + }); + pluginRegistry.forPlugin("p").registerTaskMenuAction({ + id: "abcd\uABCD", + label: "Trailing", + group: "primary", + run: vi.fn(), + }); + pluginRegistry.forPlugin("p").registerTaskMenuAction({ + id: "abcd", + label: "Parent", + group: "primary", + items: () => [ + { id: "\uABCD", label: "Child unit", run: vi.fn() }, + { id: "%3A", label: "Looks like an escape", run: vi.fn() }, + ], + run: vi.fn(), + }); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + const keys = entries.flatMap((entry) => + entry + ? [entry.key, ...(entry.kind === "submenu" ? entry.children.map((c) => c.key) : [])] + : [], + ); + + expect(keys).toHaveLength(5); + expect(new Set(keys).size).toBe(5); + + for (const pluginId of ["p\uABCD", "p"]) pluginRegistry.unregisterPlugin(pluginId); + }); + + it("keeps keys distinct when an id carries the child delimiter or a dash", () => { + // A `#` inside an action id must not forge a sibling child's key, and a `-` + // must not let one (pluginId, actionId) pair spell another's. + pluginRegistry.forPlugin("p").registerTaskMenuAction({ + id: "quick", + label: "Quick", + group: "primary", + items: () => [{ id: "x", label: "Child", run: vi.fn() }], + run: vi.fn(), + }); + pluginRegistry.forPlugin("p").registerTaskMenuAction({ + id: "quick#x", + label: "Hashed", + group: "primary", + run: vi.fn(), + }); + pluginRegistry.forPlugin("p").registerTaskMenuAction({ + id: "a-b", + label: "Dashed", + group: "primary", + run: vi.fn(), + }); + pluginRegistry.forPlugin("p-a").registerTaskMenuAction({ + id: "b", + label: "Shifted", + group: "primary", + run: vi.fn(), + }); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + const keys = entries.flatMap((entry) => + entry + ? [entry.key, ...(entry.kind === "submenu" ? entry.children.map((c) => c.key) : [])] + : [], + ); + + expect(keys).toHaveLength(5); + expect(new Set(keys).size).toBe(5); + // A flat key never contains the raw child delimiter, so it cannot equal a child key. + expect(keys.filter((key) => key.includes("#"))).toHaveLength(1); + + for (const pluginId of ["p", "p-a"]) pluginRegistry.unregisterPlugin(pluginId); + }); +}); + +describe("plugin entry keys — dash joins", () => { + it("keeps child keys distinct when two actions' ids dash-join identically", () => { + // Every key here is a dash-join (before this fix) of plugin-controlled ids, + // and the palette flattens all plugin entries into one list where a key is + // both a React key and cmdk's value. Both pairs below collided under a + // dash-join: a child against a flat action's key, and two actions whose id + // joins agree (`p` + `q-x` versus `p-q` + `x`). + pluginRegistry.forPlugin("p").registerTaskMenuAction({ + id: "pick", + label: "Pick", + group: "primary", + items: () => [{ id: "x", label: "A", run: vi.fn() }], + run: vi.fn(), + }); + pluginRegistry.forPlugin("p-pick").registerTaskMenuAction({ + id: "pick", + label: "Pick x", + group: "primary", + items: () => [{ id: "pick-x", label: "B", run: vi.fn() }], + run: vi.fn(), + }); + pluginRegistry.forPlugin("p").registerTaskMenuAction({ + id: "q-x", + label: "Q x", + group: "primary", + items: () => [{ id: "c", label: "C", run: vi.fn() }], + run: vi.fn(), + }); + pluginRegistry.forPlugin("p-q").registerTaskMenuAction({ + id: "x", + label: "X", + group: "primary", + items: () => [{ id: "c", label: "D", run: vi.fn() }], + run: vi.fn(), + }); + + const entries = buildPrimaryPluginEntries({ context: CONTEXT }); + const keys = entries.map((entry) => entry.key); + const childKeys = entries.flatMap((entry) => + entry && entry.kind === "submenu" ? entry.children.map((child) => child.key) : [], + ); + + expect(keys).toHaveLength(4); + expect(new Set(keys).size).toBe(4); + expect(childKeys).toHaveLength(4); + expect(new Set(childKeys).size).toBe(4); + // A child key can never equal a flat action key either. + expect(childKeys.filter((key) => keys.includes(key))).toEqual([]); + + for (const pluginId of ["p", "p-pick", "p-q"]) pluginRegistry.unregisterPlugin(pluginId); + }); +}); diff --git a/apps/web/components/plugins/task-menu-registration-boundary.test.tsx b/apps/web/components/plugins/task-menu-registration-boundary.test.tsx new file mode 100644 index 0000000000..b6fc602557 --- /dev/null +++ b/apps/web/components/plugins/task-menu-registration-boundary.test.tsx @@ -0,0 +1,66 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { pluginRegistry } from "@/lib/plugins/registry"; +import type { PluginIcon, PluginTaskMenuContext } from "@/lib/plugins/types"; +import { buildPrimaryPluginEntries } from "./task-menu-actions"; + +// The registration boundary: what the menu builder does with a registration whose +// own `id` is missing or has no usable string form. The registry guards its own +// read of these objects (see registry.test.ts); these cases cover the builder's +// half, where a value of the wrong shape must drop the entry rather than key it. +const PLUGIN_ID = "kandev-plugin-boundary"; + +const CONTEXT: PluginTaskMenuContext = { + workspaceId: "ws-1", + taskId: "task-1", + taskTitle: "Fix the bug", + workflowStepId: "step-1", + presentation: "desktop", +}; + +function registerAction( + overrides: { + id?: unknown; + label?: string; + icon?: PluginIcon; + items?: unknown; + } = {}, +) { + pluginRegistry.forPlugin(PLUGIN_ID).registerTaskMenuAction({ + id: overrides.id ?? "action", + label: overrides.label ?? "Action", + group: "primary", + run: vi.fn(), + ...(overrides.icon ? { icon: overrides.icon } : {}), + ...(overrides.items ? { items: overrides.items } : {}), + } as never); +} + +afterEach(() => pluginRegistry.unregisterPlugin(PLUGIN_ID)); + +describe("task menu registration boundary", () => { + it("omits an action whose id is not a usable string", () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + registerAction({ id: {} as never, items: () => [{ id: "ok", label: "Ok", run: vi.fn() }] }); + + expect(buildPrimaryPluginEntries({ context: CONTEXT })).toEqual([]); + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("no usable id"), + undefined, + ); + consoleErrorSpy.mockRestore(); + }); + + it("omits a registration whose id has no string form, without throwing", () => { + // A Symbol id or a null-prototype object has no `ToString`: the log line that + // reports the omission must not be able to throw, or the report itself takes + // the render down while dropping the registration. + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + registerAction({ id: Symbol("add-tag") as never, label: "Symbol id" }); + registerAction({ id: Object.create(null) as never, label: "Null-prototype id" }); + + expect(() => buildPrimaryPluginEntries({ context: CONTEXT })).not.toThrow(); + expect(buildPrimaryPluginEntries({ context: CONTEXT })).toEqual([]); + expect(consoleErrorSpy).toHaveBeenCalled(); + consoleErrorSpy.mockRestore(); + }); +}); diff --git a/apps/web/components/task-command-choices.test.tsx b/apps/web/components/task-command-choices.test.tsx index 92ad51806c..065a11d31b 100644 --- a/apps/web/components/task-command-choices.test.tsx +++ b/apps/web/components/task-command-choices.test.tsx @@ -1,6 +1,6 @@ import { cleanup, renderHook } from "@testing-library/react"; -import { afterEach, expect, it, vi } from "vitest"; -import { useTaskMoveChoices } from "./task-command-choices"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { pluginCommandChoices, useTaskMoveChoices } from "./task-command-choices"; afterEach(cleanup); @@ -29,3 +29,79 @@ it("omits workflows without destinations while preserving both destination actio expect(openMoveOptions).toHaveBeenCalledWith({ ...target, workflow_id: "target" }); expect(moveImmediately).toHaveBeenCalledWith({ ...target, workflow_id: "target" }); }); + +/** + * A plugin submenu carries no action of its own (its label is a trigger), so + * the command palette has to reach its children -- dropping the entry would + * silently remove a plugin's actions from the palette and the sidebar's task + * commands (see task-commands.test.tsx's plugin parity assertions). + */ +describe("pluginCommandChoices", () => { + const item = (label: string, key = label) => ({ + kind: "item" as const, + key, + label, + onSelect: vi.fn(), + }); + + it("turns a flat plugin entry into one command", () => { + const entry = { ...item("Add tag"), disabled: true, icon: "icon" }; + expect(pluginCommandChoices(entry, "Tasks")).toEqual([ + { + id: "Add tag", + label: "Add tag", + group: "Tasks", + action: entry.onSelect, + disabled: true, + icon: "icon", + }, + ]); + }); + + it("flattens a plugin submenu into its item children, keeping their disabled flag", () => { + const first = item("Blocked", "plugin-primary-tags-add-tag-more"); + const second = { ...item("Urgent"), disabled: true }; + const entry = { + kind: "submenu" as const, + key: "plugin-primary-tags-add-tag", + label: "Add tag...", + children: [first, second, { kind: "separator" as const, key: "sep" }], + }; + + expect(pluginCommandChoices(entry, "Tasks").map((command) => command.id)).toEqual([ + "plugin-primary-tags-add-tag-more", + "Urgent", + ]); + const [more, urgent] = pluginCommandChoices(entry, "Tasks"); + more.action?.(); + expect(first.onSelect).toHaveBeenCalledTimes(1); + expect(urgent.disabled).toBe(true); + expect(more.context).toBe("Add tag..."); + }); + + it("reuses the entry key as the command id, which the builder keeps unique", () => { + // Uniqueness is the builder's job: a child's key encodes its id, so the + // palette can use it directly as both the React key and cmdk's value. + const child = item("Pick", "plugin-primary-p-quick#pick-x"); + const submenu = { + kind: "submenu" as const, + key: "plugin-primary-p-quick", + label: "Quick", + children: [child], + }; + const flat = item("Quick pick", "plugin-primary-p-quick-pick"); + + const ids = [ + ...pluginCommandChoices(submenu, "Tasks"), + ...pluginCommandChoices(flat, "Tasks"), + ].map((command) => command.id); + + expect(ids).toEqual(["plugin-primary-p-quick#pick-x", "plugin-primary-p-quick-pick"]); + expect(new Set(ids).size).toBe(ids.length); + }); + + it("ignores a label the palette cannot render", () => { + const entry = { ...item("x"), label: {} as never }; + expect(pluginCommandChoices(entry, "Tasks")).toEqual([]); + }); +}); diff --git a/apps/web/components/task-command-choices.tsx b/apps/web/components/task-command-choices.tsx index 004b61be41..62f423f3f0 100644 --- a/apps/web/components/task-command-choices.tsx +++ b/apps/web/components/task-command-choices.tsx @@ -16,6 +16,7 @@ import { } from "./task/task-move-context-menu"; import { useTaskPluginLinkActions } from "./task/task-session-sidebar-link-actions"; import { useTaskPluginPrimaryMenuEntries } from "./task/task-switcher-plugin-menu-items"; +import type { KanbanCardMenuEntry } from "./kanban-card-menu-items"; import { selectTaskLinkActions, taskLinkMenuOptions, @@ -34,6 +35,43 @@ type ChoiceOptions = { moveImmediately: (step: TaskMoveStep) => void; }; +/** + * One command per *selectable* plugin menu entry. A plugin submenu has no + * activation of its own (its `label` is only a trigger and it carries no + * `onSelect`), so its item children become the commands -- flattening them + * keeps a plugin's actions reachable from the command palette instead of + * dropping the whole contribution, and mirrors what the search surface shows + * for every other card action. + */ +export function pluginCommandChoices( + entry: KanbanCardMenuEntry, + group: string, + parentLabel?: string, +): CommandItem[] { + if (entry.kind === "submenu") { + const label = typeof entry.label === "string" ? entry.label : parentLabel; + return entry.children.flatMap((child) => pluginCommandChoices(child, group, label)); + } + if (entry.kind !== "item" || typeof entry.label !== "string") return []; + return [ + { + // The entry key is unique across every plugin menu entry, including + // flattened children whose key encodes the child id, and it doubles as + // this row's React key and cmdk value. + id: entry.key, + label: entry.label, + group, + // A child's own label ("Blocked") is meaningless on its own in a palette + // listing every action, so the trigger it came from is the entry's + // context -- the same field the sidebar task commands use for the title. + context: parentLabel, + action: entry.onSelect, + disabled: entry.disabled, + icon: entry.icon, + }, + ]; +} + export function useTaskCommandChoices(options: ChoiceOptions) { const { task, linkHandlers } = options; const { t } = useTranslation(); @@ -87,17 +125,7 @@ export function useTaskCommandChoices(options: ChoiceOptions) { icon: createElement(resolvePluginIcon(link.icon), { className: "size-3.5" }), })), ); - const plugins = pluginEntries.flatMap((entry): CommandItem[] => - entry.kind === "item" && typeof entry.label === "string" - ? [ - { - ...item(entry.key, entry.label, entry.onSelect), - disabled: entry.disabled, - icon: entry.icon, - }, - ] - : [], - ); + const plugins = pluginEntries.flatMap((entry) => pluginCommandChoices(entry, group)); return { colors, priorities, diff --git a/apps/web/components/task-command-items.tsx b/apps/web/components/task-command-items.tsx index e59d06d3a9..4e8cb9cd7c 100644 --- a/apps/web/components/task-command-items.tsx +++ b/apps/web/components/task-command-items.tsx @@ -88,6 +88,9 @@ export function buildSidebarTaskCommands(ctx: TaskCommandContext): CommandItem[] ...item, priority: 0, disabled: item.disabled || ctx.disabled, - context: task.title, + // An item that names its own context keeps it (a plugin submenu child + // carries the trigger it came from); the task title is only the default + // second line for everything else. + context: item.context ?? task.title, })); } diff --git a/apps/web/components/task-commands.test.tsx b/apps/web/components/task-commands.test.tsx index 9f513678c9..f5b4c80965 100644 --- a/apps/web/components/task-commands.test.tsx +++ b/apps/web/components/task-commands.test.tsx @@ -74,6 +74,21 @@ describe("sidebar task command parity", () => { expect(ctx.onPin).toHaveBeenCalledTimes(1); expect(ctx.onDelete).toHaveBeenCalledTimes(1); }); + // Regression: this map used to stamp every command with the task title, which + // silently overwrote the trigger label a plugin submenu child carries, so the + // value pluginCommandChoices computed never reached the palette row. + it("keeps a plugin command's own context and defaults the rest to the task title", () => { + const ctx = context({ + plugins: [ + { ...child, id: PLUGIN_ACTION, context: "Add tag..." }, + { ...child, id: "plugin-flat" }, + ], + }); + const commands = buildSidebarTaskCommands(ctx); + expect(commands.find((c) => c.id === PLUGIN_ACTION)?.context).toBe("Add tag..."); + expect(commands.find((c) => c.id === "plugin-flat")?.context).toBe("Task A"); + }); + it("removes vanished plugin actions and unavailable choices on rebuild", () => { const ctx = context({ plugins: [], links: [], steps: [], workflows: [] }); expect(buildSidebarTaskCommands(ctx).map((c) => c.id)).not.toContain(PLUGIN_ACTION); diff --git a/apps/web/e2e/fixtures/plugins/prompt-history-plugin/bundle.js b/apps/web/e2e/fixtures/plugins/prompt-history-plugin/bundle.js index cdcf2a43ea..f463aefe2c 100644 --- a/apps/web/e2e/fixtures/plugins/prompt-history-plugin/bundle.js +++ b/apps/web/e2e/fixtures/plugins/prompt-history-plugin/bundle.js @@ -1018,6 +1018,35 @@ ); }, }); + registry.registerTaskMenuAction({ + id: "task-shortcuts", + label: "Task shortcuts", + group: "primary", + items: function () { + return [ + { + id: "record-presentation", + label: "Record menu presentation", + run: function (childContext) { + return host.storage.set( + "task", + childContext.taskId, + "primary-submenu-presentation", + childContext.presentation, + ); + }, + }, + ]; + }, + run: function (context) { + return host.storage.set( + "task", + context.taskId, + "primary-submenu-presentation", + context.presentation, + ); + }, + }); registry.registerKeybinding("open-demo", function () { function DemoModalContent() { diff --git a/apps/web/e2e/tests/plugins/mobile-plugin-task-panel.spec.ts b/apps/web/e2e/tests/plugins/mobile-plugin-task-panel.spec.ts index f4706fabd0..2507346b05 100644 --- a/apps/web/e2e/tests/plugins/mobile-plugin-task-panel.spec.ts +++ b/apps/web/e2e/tests/plugins/mobile-plugin-task-panel.spec.ts @@ -150,4 +150,46 @@ test.describe("Mobile plugin task panel", () => { ) .toBe("mobile"); }); + + test("opens and selects a primary plugin submenu by touch", async ({ + testPage, + apiClient, + seedData, + }) => { + test.setTimeout(90_000); + + await installFixturePlugin(testPage); + + const task = await apiClient.createTask(seedData.workspaceId, "Mobile plugin submenu task", { + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + }); + const mobile = new MobileKanbanPage(testPage); + await mobile.goto(); + await mobile.taskCard(task.id).getByRole("button", { name: "More options" }).tap(); + + const menu = testPage.locator('[data-slot="dropdown-menu-content"]:visible'); + const submenu = menu.getByRole("menuitem", { name: "Task shortcuts", exact: true }); + await expect(submenu).toBeVisible(); + + await submenu.tap(); + const child = testPage.getByRole("menuitem", { name: "Record menu presentation", exact: true }); + await expect(child).toBeVisible(); + await child.tap(); + + await expect + .poll( + async () => { + const res = await apiClient.rawRequest( + "GET", + `/api/plugins/${PLUGIN_ID}/user-state/task/${task.id}/primary-submenu-presentation`, + ); + if (res.status !== 200) return null; + const body = (await res.json()) as { value: string }; + return body.value; + }, + { timeout: 10_000, intervals: [250, 500, 1000] }, + ) + .toBe("mobile"); + }); }); diff --git a/apps/web/lib/kanban/task-actions-menu-entries.test.ts b/apps/web/lib/kanban/task-actions-menu-entries.test.ts index 2cbe571be7..c991182d22 100644 --- a/apps/web/lib/kanban/task-actions-menu-entries.test.ts +++ b/apps/web/lib/kanban/task-actions-menu-entries.test.ts @@ -1,7 +1,10 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { pluginRegistry } from "@/lib/plugins/registry"; import { buildTaskActionsMenuEntries } from "./task-actions-menu-entries"; -import type { KanbanCardMenuEntry } from "@/components/kanban-card-menu-items"; +import { + buildCardPluginEntries, + type KanbanCardMenuEntry, +} from "@/components/kanban-card-menu-items"; const PLUGIN_ID = "test-task-actions-menu-plugin"; const REMOVE_SEPARATOR = "remove-separator"; @@ -39,6 +42,38 @@ const baseArgs = { onLinkPullRequest: vi.fn(), }; +describe("buildTaskActionsMenuEntries — prebuilt plugin entries", () => { + afterEach(() => pluginRegistry.unregisterPlugin(PLUGIN_ID)); + + // Regression: these tiers force `forceFlatEdit`, so a caller's prebuilt set -- + // built for the card, where an edit-group action nests -- would render the very + // Edit submenu the tier exists to suppress. + it("ignores prebuilt entries, keeping the flat Edit item", () => { + pluginRegistry.forPlugin(PLUGIN_ID).registerTaskMenuAction({ + id: "nested", + label: "Nested", + group: "edit", + items: () => [{ id: "child", label: "Child", run: vi.fn() }], + run: vi.fn(), + }); + const context = { + workspaceId: "ws-1", + taskId: "task-1", + taskTitle: "Task", + workflowStepId: "step-1", + presentation: "desktop" as const, + }; + const cardEntries = buildCardPluginEntries({ onEdit: vi.fn(), pluginMenuContext: context }); + + const entries = buildTaskActionsMenuEntries("normal", { + ...baseArgs, + pluginEntries: cardEntries, + }); + + expect(entries.find((entry) => entry.key === "edit")?.kind).toBe("item"); + }); +}); + describe("buildTaskActionsMenuEntries — normal tier", () => { afterEach(() => pluginRegistry.unregisterPlugin(PLUGIN_ID)); @@ -131,7 +166,7 @@ describe("buildTaskActionsMenuEntries — archived tier", () => { const entries = buildTaskActionsMenuEntries("archived", baseArgs); expect(itemKeys(entries)).toEqual([ - `plugin-primary-${PLUGIN_ID}-primary-action`, + `plugin-primary-${PLUGIN_ID}:primary-action`, REMOVE_SEPARATOR, "delete", ]); @@ -164,7 +199,7 @@ describe("buildTaskActionsMenuEntries — unresolved board row tier", () => { const entries = buildTaskActionsMenuEntries("unresolved-row", baseArgs); expect(itemKeys(entries)).toEqual([ - `plugin-primary-${PLUGIN_ID}-primary-action`, + `plugin-primary-${PLUGIN_ID}:primary-action`, REMOVE_SEPARATOR, "archive", "delete", diff --git a/apps/web/lib/kanban/task-actions-menu-entries.ts b/apps/web/lib/kanban/task-actions-menu-entries.ts index 8bd66ecc1e..ce6de5da4c 100644 --- a/apps/web/lib/kanban/task-actions-menu-entries.ts +++ b/apps/web/lib/kanban/task-actions-menu-entries.ts @@ -24,7 +24,9 @@ export function buildTaskActionsMenuEntries( ): KanbanCardMenuEntry[] { if (tier === "archived") return buildArchivedTaskActionsMenuEntries(args); if (tier === "unresolved-row") return buildUnresolvedRowTaskActionsMenuEntries(args); - return buildKanbanCardMenuEntries({ ...args, forceFlatEdit: true }); + // `forceFlatEdit` is forced here, so any entries built for the card must not be reused: their + // `Edit` entry may be the submenu this tier exists to suppress. + return buildKanbanCardMenuEntries({ ...args, forceFlatEdit: true, pluginEntries: undefined }); } function buildArchivedTaskActionsMenuEntries({ diff --git a/apps/web/lib/plugins/icons.test.ts b/apps/web/lib/plugins/icons.test.ts index c8a7a886dc..425805aca1 100644 --- a/apps/web/lib/plugins/icons.test.ts +++ b/apps/web/lib/plugins/icons.test.ts @@ -1,6 +1,7 @@ +import { createElement, forwardRef, lazy } from "react"; import { describe, expect, it } from "vitest"; import { IconPuzzle, IconTicket } from "@tabler/icons-react"; -import { lookupPluginIcon, resolvePluginIcon } from "./icons"; +import { isPluginIconComponent, lookupPluginIcon, resolvePluginIcon } from "./icons"; describe("plugin icons", () => { it("looks up a known icon name", () => { @@ -25,6 +26,87 @@ describe("plugin icons", () => { expect(lookupPluginIcon(undefined)).toBeUndefined(); }); + // Regression: a JavaScript bundle can pass any value. Indexing PLUGIN_ICONS + // with a non-string coerced it to a key, so an object whose toString threw + // took the rendering surface down, and any other object silently became the + // puzzle glyph by way of a "[object Object]" lookup miss. + it("passes through an exotic component, which memo/forwardRef produce", () => { + // React's memo and forwardRef return objects that are callable per their + // types but not at run time, and @tabler/icons-react -- the set + // PLUGIN_ICONS maps names onto -- builds every icon with forwardRef. + // memo() returns a non-callable object; a function carrying $$typeof would + // not reproduce the shape that `typeof icon === "function"` accepts. + const ExoticIcon = { $$typeof: Symbol.for("react.memo"), type: () => null }; + + expect(lookupPluginIcon(ExoticIcon as never)).toBe(ExoticIcon); + expect(resolvePluginIcon(ExoticIcon as never)).toBe(ExoticIcon); + }); + + it("does not treat an element, a lazy component or a bare tag as a component", () => { + // An element is a rendered node, so handing it to createElement on the twelve + // surfaces that do `createElement(resolvePluginIcon(icon))` would throw; a + // lazy component cannot resolve synchronously in a menu; and an object that + // merely carries `$$typeof` is not a component at all. All three fall back. + const element = createElement("svg", { viewBox: "0 0 24 24" }); + const lazyIcon = lazy(() => Promise.resolve({ default: () => null })); + const bogus = { $$typeof: 1 }; + + for (const icon of [element, lazyIcon, bogus]) { + expect(isPluginIconComponent(icon)).toBe(false); + expect(lookupPluginIcon(icon as never)).toBeUndefined(); + expect(resolvePluginIcon(icon as never)).toBe(IconPuzzle); + } + }); + + it("passes through a forwardRef component, the shape an icon set builds", () => { + const Forwarded = forwardRef(() => null); + + expect(isPluginIconComponent(Forwarded)).toBe(true); + expect(lookupPluginIcon(Forwarded as never)).toBe(Forwarded); + }); + + it("treats an icon whose tag read throws as absent, not as a component", () => { + // Regression: the tag read (and the `in` check before it) runs a + // plugin-supplied trap, and this function backs every icon surface. + const hostile = new Proxy( + {}, + { + get() { + throw new Error("hostile $$typeof getter"); + }, + has() { + throw new Error("hostile in trap"); + }, + }, + ); + + expect(() => isPluginIconComponent(hostile)).not.toThrow(); + expect(isPluginIconComponent(hostile)).toBe(false); + expect(lookupPluginIcon(hostile as never)).toBeUndefined(); + expect(resolvePluginIcon(hostile as never)).toBe(IconPuzzle); + }); + + it("resolves only own keys, so a prototype member is not an icon", () => { + for (const name of ["__proto__", "constructor", "toString", "hasOwnProperty"]) { + expect(lookupPluginIcon(name)).toBeUndefined(); + expect(resolvePluginIcon(name)).toBe(IconPuzzle); + } + expect(lookupPluginIcon("ticket")).toBe(IconTicket); + }); + + it("treats a non-string, non-function icon as absent instead of coercing it to a name", () => { + const hostile = { + toString() { + throw new Error("key coercion"); + }, + }; + + expect(() => lookupPluginIcon(hostile as unknown as string)).not.toThrow(); + expect(lookupPluginIcon(hostile as unknown as string)).toBeUndefined(); + expect(resolvePluginIcon(hostile as unknown as string)).toBe(IconPuzzle); + expect(lookupPluginIcon({} as unknown as string)).toBeUndefined(); + }); + it("falls back to the puzzle glyph from resolvePluginIcon", () => { expect(resolvePluginIcon("not-an-icon")).toBe(IconPuzzle); expect(resolvePluginIcon(undefined)).toBe(IconPuzzle); diff --git a/apps/web/lib/plugins/icons.ts b/apps/web/lib/plugins/icons.ts index b6961bc6fd..6f4c2ca77e 100644 --- a/apps/web/lib/plugins/icons.ts +++ b/apps/web/lib/plugins/icons.ts @@ -54,10 +54,54 @@ export const PLUGIN_ICONS: Record = { users: IconUsers, }; -/** Strict lookup: the named icon, or undefined when the name is unknown/missing. */ +/** + * The React tags a *component* carries at run time. `memo` and `forwardRef` + * return objects (callable per their types, not at run time), and an icon set -- + * which `host-api` explicitly tells plugins to bundle -- is built from + * `forwardRef`, so a plain `typeof` check would reject the host's own icon + * shapes. + * + * Only those two count. A lazy component cannot resolve synchronously in a menu, + * and a *rendered* element is not a component at all: the menu passes an element + * through itself while every other surface maps it to the fallback glyph, so + * admitting it here would hand `createElement` something it refuses and take the + * render down. + */ +const PLUGIN_ICON_COMPONENT_TAGS = new Set([ + Symbol.for("react.memo"), + Symbol.for("react.forward_ref"), +]); + +export function isPluginIconComponent(icon: unknown): icon is ResolvedPluginIcon { + if (typeof icon === "function") return true; + if (typeof icon !== "object" || icon === null) return false; + // The `in` check and the read both run a plugin-supplied trap -- a Proxy `has`, + // or an accessor whose getter throws -- and this function runs on every icon + // surface, so an unreadable tag means "not a component" rather than an escape. + let tag: unknown; + try { + if (!("$$typeof" in icon)) return false; + tag = icon.$$typeof; + } catch { + return false; + } + return typeof tag === "symbol" && PLUGIN_ICON_COMPONENT_TAGS.has(tag); +} + +/** + * Strict lookup: the named icon, or undefined when the name is unknown/missing. + * Only a string names a curated icon: any other value (a stray object or a + * ready-made element from a JavaScript bundle) has no name to look up, and + * indexing the map with it would coerce it to a key -- which a hostile + * `toString` turns into a throw out of whatever is rendering. A component, + * exotic or not, needs no lookup at all. + */ export function lookupPluginIcon(icon?: PluginIcon): ResolvedPluginIcon | undefined { - if (typeof icon === "function") return icon; - return icon ? PLUGIN_ICONS[icon] : undefined; + if (isPluginIconComponent(icon)) return icon; + if (typeof icon !== "string") return undefined; + // Own keys only: `PLUGIN_ICONS["__proto__"]` or `["constructor"]` would + // otherwise resolve through the prototype chain to a value that is not an icon. + return Object.hasOwn(PLUGIN_ICONS, icon) ? PLUGIN_ICONS[icon] : undefined; } /** Sidebar lookup: always renders something — unknown/missing names get the puzzle glyph. */ diff --git a/apps/web/lib/plugins/registry.test.ts b/apps/web/lib/plugins/registry.test.ts index 3c5d42ffa7..dc9bd2c498 100644 --- a/apps/web/lib/plugins/registry.test.ts +++ b/apps/web/lib/plugins/registry.test.ts @@ -359,6 +359,39 @@ describe("pluginRegistry — task panels and task menu actions", () => { expect(pluginRegistry.getTaskMenuActions()).toHaveLength(2); }); + it("skips unreadable task menu registrations without hiding healthy actions", () => { + const scoped = pluginRegistry.forPlugin("plugin-a"); + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const run = vi.fn(); + scoped.registerTaskMenuAction({ + id: "broken-group", + label: "Broken group", + get group(): never { + throw new Error("group failed"); + }, + run, + }); + scoped.registerTaskMenuAction({ + id: "broken-label", + get label(): never { + throw new Error("label failed"); + }, + group: "primary", + run, + }); + scoped.registerTaskMenuAction({ id: "healthy", label: "Healthy", group: "primary", run }); + + try { + expect(pluginRegistry.getTaskMenuActions("primary")).toEqual([ + { pluginId: "plugin-a", id: "healthy", label: "Healthy", group: "primary", run }, + ]); + expect(pluginRegistry.getTaskMenuActions("primary")).toHaveLength(1); + expect(error).toHaveBeenCalledTimes(2); + } finally { + error.mockRestore(); + } + }); + it("bulk-revokes task panels and task menu actions on unregisterPlugin", () => { const scopedA = pluginRegistry.forPlugin("plugin-a"); const scopedB = pluginRegistry.forPlugin("plugin-b"); diff --git a/apps/web/lib/plugins/registry.ts b/apps/web/lib/plugins/registry.ts index 6597b41321..8e6ecdbc58 100644 --- a/apps/web/lib/plugins/registry.ts +++ b/apps/web/lib/plugins/registry.ts @@ -146,6 +146,7 @@ class PluginRegistryStore { private workLifecycle = new PluginWorkLifecycle(); private taskPanels: Owned[] = []; private taskMenuActions: Owned[] = []; + private reportedUnreadableTaskMenuActions = new WeakSet>(); private taskFilters: Owned[] = []; private taskListFacets: Owned[] = []; private nextSlotRegistrationId = 0; @@ -699,9 +700,23 @@ class PluginRegistryStore { getTaskMenuActions( group?: TaskMenuActionRegistration["group"], ): PluginTaskMenuActionRegistration[] { - return this.taskMenuActions - .filter((entry) => !group || entry.value.group === group) - .map((entry) => ({ ...entry.value, pluginId: entry.pluginId })); + const actions: PluginTaskMenuActionRegistration[] = []; + for (const entry of this.taskMenuActions) { + try { + if (!group || entry.value.group === group) { + actions.push({ ...entry.value, pluginId: entry.pluginId }); + } + } catch (error) { + if (!this.reportedUnreadableTaskMenuActions.has(entry)) { + this.reportedUnreadableTaskMenuActions.add(entry); + console.error( + `[plugins] task menu action from "${entry.pluginId}" could not be read`, + error, + ); + } + } + } + return actions; } /** Every registered task filter, in registration order. */ diff --git a/apps/web/lib/plugins/sdk-contract.test.ts b/apps/web/lib/plugins/sdk-contract.test.ts index 9a49654293..8624c3f431 100644 --- a/apps/web/lib/plugins/sdk-contract.test.ts +++ b/apps/web/lib/plugins/sdk-contract.test.ts @@ -20,6 +20,8 @@ import type { RepositoryProviderRegistration as PublicRepositoryProviderRegistration, ReviewSummary as PublicReviewSummary, ReviewTaskAssociation as PublicReviewTaskAssociation, + TaskMenuActionRegistration as PublicTaskMenuActionRegistration, + TaskMenuSubItemRegistration as PublicTaskMenuSubItemRegistration, TaskPanelRegistration as PublicTaskPanelRegistration, } from "@kandev/plugin-sdk"; import type { @@ -41,6 +43,8 @@ import type { RepositoryProviderRegistration, ReviewItemSummary, ReviewTaskAssociation, + TaskMenuActionRegistration, + TaskMenuSubItemRegistration, TaskPanelRegistration, } from "./types"; @@ -53,6 +57,24 @@ type SameType = Left extends Right ? (Right extends Left ? true : f type HasUseLayoutEffect = "useLayoutEffect" extends keyof PublicHostReact ? true : false; type HasPromptMentionText = "PromptMentionText" extends keyof PublicPluginUIApi ? true : false; type HasConversationApi = "conversation" extends keyof PublicPluginHostApi ? true : false; +type HasTaskMenuItems = "items" extends keyof PublicTaskMenuActionRegistration ? true : false; + +// A registration that predates submenus must keep compiling unchanged, and the +// submenu shape must be expressible: that pair is the whole compatibility story +// of the optional `items` field. +const flatOnlyTaskMenuAction: PublicTaskMenuActionRegistration = { + id: "legacy", + label: "Legacy", + group: "primary", + run: () => {}, +}; +const submenuTaskMenuAction: PublicTaskMenuActionRegistration = { + id: "submenu", + label: "Submenu", + group: "primary", + items: () => [{ id: "child", label: "Child", run: () => {} }], + run: () => {}, +}; const legacyTaskPanelRegistration: PublicTaskPanelRegistration = { id: "legacy", @@ -164,3 +186,28 @@ describe("public plugin SDK", () => { expect(publicRegistryContract).toBeTypeOf("function"); }); }); + +// The registrations this feature adds to the public SDK. A plugin author +// imports these names verbatim, and the host's runtime-facing types must stay +// identical to them, including the optional `items` field. +describe("task menu submenu SDK contract", () => { + it("is canonical in both directions", () => { + const taskMenuActionRegistrationIsCanonical: SameType< + TaskMenuActionRegistration, + PublicTaskMenuActionRegistration + > = true; + const taskMenuSubItemRegistrationIsCanonical: SameType< + TaskMenuSubItemRegistration, + PublicTaskMenuSubItemRegistration + > = true; + const publicTaskMenuActionHasItems: HasTaskMenuItems = true; + expect(taskMenuActionRegistrationIsCanonical).toBe(true); + expect(taskMenuSubItemRegistrationIsCanonical).toBe(true); + expect(publicTaskMenuActionHasItems).toBe(true); + // A registration that predates submenus still compiles unchanged, and the + // submenu shape is expressible: that pair is the compatibility story of the + // optional `items` field. + expect(flatOnlyTaskMenuAction.run).toBeTypeOf("function"); + expect(submenuTaskMenuAction.items).toBeTypeOf("function"); + }); +}); diff --git a/apps/web/lib/plugins/types.ts b/apps/web/lib/plugins/types.ts index 30e1d11400..a425204e16 100644 --- a/apps/web/lib/plugins/types.ts +++ b/apps/web/lib/plugins/types.ts @@ -360,9 +360,17 @@ export type PluginTaskMenuContext = PluginSDK.PluginTaskMenuContext; * "edit" nests the item inside the card's `Edit` submenu; group "primary" * renders it as a flat, top-level menu item after the movement group and * before the `Archive`/`Delete` removal group. + * + * An action that declares `items` becomes a submenu: `label` is its + * (unselectable) trigger, the returned children are its entries, and `run` + * remains the fallback for a host that predates the field and for a build + * whose `items` yields nothing usable. */ export type TaskMenuActionRegistration = PluginSDK.TaskMenuActionRegistration; +/** One child of a `TaskMenuActionRegistration` that declares `items`. */ +export type TaskMenuSubItemRegistration = PluginSDK.TaskMenuSubItemRegistration; + /** Read-only context passed to `TaskFilterRegistration.matches`. */ export type PluginTaskFilterContext = Parameters[0]; diff --git a/docs/plans/plugin-task-menu-submenus/plan.md b/docs/plans/plugin-task-menu-submenus/plan.md new file mode 100644 index 0000000000..ed5c5e3a1d --- /dev/null +++ b/docs/plans/plugin-task-menu-submenus/plan.md @@ -0,0 +1,63 @@ +--- +requirements: + - REQ-PLUGINS-TASK-MENU-SUBMENUS-001 + - REQ-PLUGINS-TASK-MENU-SUBMENUS-002 + - REQ-PLUGINS-TASK-MENU-SUBMENUS-003 +system_design: + - ../../specs/plugins/system-design/plugin-task-menu-submenus.md +created: 2026-09-23 +status: draft +--- + +# Implementation Plan: Plugin task menu submenus + +## Overview + +`registerTaskMenuAction` contributes exactly one selectable item per plugin +action, so a plugin whose natural contribution is a short list of frequent +choices (the Tags plugin's recently used tags) has nowhere to put them. This plan +lands the host half of a one-level submenu contract: an optional +`items(context)` beside the required `run`, a single defensive boundary that +reads plugin values, an entry key that is injective for any plugin-authored +string, and flattening that makes `primary` children reachable from the command +palette and the sidebar's task commands. + +The plugin-facing half lives in its own repository and consumes this contract; it +is not part of this plan. Rendering needs no new component, because the menu entry +model already supports submenus and both renderers recurse. + +Implementation is entirely in `apps/web` plus the plugin SDK type and two +documents. There is no backend, state, persistence or protocol change: a host that +predates `items` ignores the field, and `run` keeps its existing behavior, so the +contract is additive for every shipped plugin. + +## Work orders + +| Work order | Scope | Status | +| --- | --- | --- | +| [task-01-host-submenu-contract.md](task-01-host-submenu-contract.md) | The contract, the boundary, keys, icon resolution, palette flattening and their tests | done | + +## Design notes + +- `KanbanCardMenuEntry` already models `kind: "submenu"` for the built-in Move + to / Priority / Link menus, and `ContextEntry`/`DropdownEntry` recurse into + `children`, so cards, the shared task-row menu, the preview/detail menus and the + palette all inherit plugin submenus from one entry builder. +- The contract's fallback rule is what keeps one implementation valid on two host + generations: a host predating `items` reads only `label`/`icon`/`run`, and a + build whose `items()` yields nothing usable renders that same flat item. +- Plugin bundles are plain JavaScript, so the boundary is written for values the + registration types do not describe: a throwing getter, a Proxy, a thenable, a + `Symbol` id, a lone surrogate, an element where a component is expected. +- Keys are identity, not display: the palette uses an entry key as both a React + key and cmdk's value, which is why injectivity is a requirement rather than a + nicety. + +## Verification + +- `cd apps/web && ./node_modules/.bin/tsc --noEmit --incremental false` +- `cd apps/web && pnpm vitest run components/plugins/task-menu-actions.test.tsx components/kanban-card-edit-submenu.test.tsx components/kanban-card-menu-items.test.tsx components/kanban-card-menu-grouping.test.tsx components/kanban-card-plugin-context.test.tsx lib/kanban/task-actions-menu-entries.test.ts lib/plugins/sdk-contract.test.ts lib/plugins/icons.test.ts components/task/task-switcher-context-menu.test.tsx components/task-command-choices.test.tsx components/task-commands.test.tsx` +- `cd apps/web && ./node_modules/.bin/eslint --max-warnings 0 ` and `pnpm i18n:check` +- `python3 scripts/list-docs.py validate` and `python3 scripts/lint-spec-files.py --all` +- The consumer plugin's own suite (`node --test ui/bundle.test.js` in its + repository) exercises the declared `items`/`run` pair against this contract. diff --git a/docs/plans/plugin-task-menu-submenus/task-01-host-submenu-contract.md b/docs/plans/plugin-task-menu-submenus/task-01-host-submenu-contract.md new file mode 100644 index 0000000000..ff095a2a2d --- /dev/null +++ b/docs/plans/plugin-task-menu-submenus/task-01-host-submenu-contract.md @@ -0,0 +1,97 @@ +--- +id: "01-host-submenu-contract" +title: "Host: task menu action submenu contract" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-PLUGINS-TASK-MENU-SUBMENUS-001 + - REQ-PLUGINS-TASK-MENU-SUBMENUS-002 + - REQ-PLUGINS-TASK-MENU-SUBMENUS-003 +acceptance_criteria: + - AC-PLUGINS-TASK-MENU-SUBMENUS-001.1 + - AC-PLUGINS-TASK-MENU-SUBMENUS-001.2 + - AC-PLUGINS-TASK-MENU-SUBMENUS-001.3 + - AC-PLUGINS-TASK-MENU-SUBMENUS-001.4 + - AC-PLUGINS-TASK-MENU-SUBMENUS-001.5 + - AC-PLUGINS-TASK-MENU-SUBMENUS-001.6 + - AC-PLUGINS-TASK-MENU-SUBMENUS-002.1 + - AC-PLUGINS-TASK-MENU-SUBMENUS-002.2 + - AC-PLUGINS-TASK-MENU-SUBMENUS-002.3 + - AC-PLUGINS-TASK-MENU-SUBMENUS-002.4 + - AC-PLUGINS-TASK-MENU-SUBMENUS-002.5 + - AC-PLUGINS-TASK-MENU-SUBMENUS-003.1 + - AC-PLUGINS-TASK-MENU-SUBMENUS-003.2 + - AC-PLUGINS-TASK-MENU-SUBMENUS-003.3 +system_design: + - ../../specs/plugins/system-design/plugin-task-menu-submenus.md +--- + +# Task 01: Host task menu action submenu contract + +- **Acceptance:** + 1. `TaskMenuActionRegistration.items?(context)` is optional and synchronous in + the SDK and the web re-export; `run` stays required; a host that predates the + field renders the flat item (AC-PLUGINS-TASK-MENU-SUBMENUS-001.1). + 2. A usable `items()` renders the action as a one-level submenu: `label` is the + unselectable trigger, children render in order under the same context, the + entry's own `disabled` reaches every child, and a child's own `disabled` + holds (AC-PLUGINS-TASK-MENU-SUBMENUS-001.2). + 3. Every unusable result — absent, `null`, non-array, empty, a throw, a + thenable, or only-unusable children — falls back to the flat `run` item, with + the defect reported once per action and kind + (AC-PLUGINS-TASK-MENU-SUBMENUS-001.3, + AC-PLUGINS-TASK-MENU-SUBMENUS-001.4). + 4. A child is read once inside the boundary's guard into a snapshot: blank or + non-string `id`/`label`, a non-callable `run`, a `disabled` that is neither + boolean nor `null`, and an `icon` that is none of name/component/element/ + `null` all drop that child; a repeated `id` keeps the first occurrence; a + throwing getter or a Proxy fails only that child + (AC-PLUGINS-TASK-MENU-SUBMENUS-001.5). + 5. No malformed registration can fail a render: an unusable action `label` or + `id` contributes no entry, the defect report never throws on a `Symbol`, + null-prototype or throwing-accessor registration, a `primary`-group submenu + whose entries are all unusable leaves the native `Edit` item flat, and the + key encoder neither throws on a lone surrogate nor collides for any two + plugin-authored ids (AC-PLUGINS-TASK-MENU-SUBMENUS-002.1 .. 002.4). + 6. Icon resolution admits `memo`/`forwardRef` component objects, excludes + `lazy` and elements for non-menu surfaces, resolves own keys only, and keeps + the menu-only element tolerance documented + (AC-PLUGINS-TASK-MENU-SUBMENUS-002.5). + 7. Submenu children of a `primary` action reach the command palette and the + sidebar's task commands, one command each, carrying the trigger label as + `context`; `edit` stays card-only; a caller that forces the flat `Edit` form + never reuses a prebuilt entry set + (AC-PLUGINS-TASK-MENU-SUBMENUS-003.1 .. 003.3). +- **Verification:** + - `cd apps/web && ./node_modules/.bin/tsc --noEmit --incremental false` + - `cd apps/web && pnpm vitest run components/plugins/task-menu-actions.test.tsx components/kanban-card-edit-submenu.test.tsx components/kanban-card-menu-items.test.tsx components/kanban-card-menu-grouping.test.tsx components/kanban-card-plugin-context.test.tsx lib/kanban/task-actions-menu-entries.test.ts lib/plugins/sdk-contract.test.ts lib/plugins/icons.test.ts components/task/task-switcher-context-menu.test.tsx components/task-command-choices.test.tsx components/task-commands.test.tsx` + - `cd apps/web && ./node_modules/.bin/eslint --max-warnings 0 ` + - `cd apps/web && pnpm i18n:check` + - `python3 scripts/list-docs.py validate` + - `node .github/scripts/pr-docs.cjs` (CI's documentation-coverage evaluator) +- **Files likely touched:** + - `apps/packages/plugin-sdk/src/index.ts` + - `apps/web/lib/plugins/types.ts` + - `apps/web/lib/plugins/icons.ts` (+ test) + - `apps/web/components/plugins/task-menu-actions.ts` (+ test) + - `apps/web/components/kanban-card-edit-submenu.tsx` (+ test) + - `apps/web/components/kanban-card-menu-items.tsx` (+ test) + - `apps/web/components/kanban-card-menu.tsx` + - `apps/web/components/task-command-choices.tsx` (+ test) + - `apps/web/components/task-command-items.tsx` (+ test) + - `apps/web/lib/kanban/task-actions-menu-entries.ts` (+ test) + - `apps/web/lib/plugins/sdk-contract.test.ts` + - `apps/web/AGENTS.md`, `docs/plans/plugins/PLUGIN-API.md`, `docs/public/plugins-authoring.md` +- **Dependencies:** None. +- **Parallelism:** sequential. +- **Inputs:** + - Requirements: docs/specs/plugins/requirements/plugin-task-menu-submenus.md + - System design: docs/specs/plugins/system-design/plugin-task-menu-submenus.md + - Existing submenu entry model and renderers: `apps/web/components/kanban-card-menu-items.tsx` + - Plugin contract documentation: `docs/plans/plugins/PLUGIN-API.md`, `docs/public/plugins-authoring.md` + - Consumer expectations: the plugin repository's `ui/bundle.js` registration and its tests +- **Output contract:** summary, files changed, exact verification commands with + results, the adversarial review rounds with their dispositions, task status → + `done`, plan checkbox update. diff --git a/docs/plans/plugins/PLUGIN-API.md b/docs/plans/plugins/PLUGIN-API.md index 64bc32c732..b4b4376bda 100644 --- a/docs/plans/plugins/PLUGIN-API.md +++ b/docs/plans/plugins/PLUGIN-API.md @@ -900,6 +900,7 @@ The continuation-renew endpoint from the predecessor transport is not part of the source contract. Load-more and retry use a current source read and preserve the public state until that read succeeds. No browser code imports a persistence store or writes conversation history. + ## `registry: PluginRegistry` ```ts @@ -916,6 +917,11 @@ store or writes conversation history. // 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. +// A curated name (`PLUGIN_ICONS` in the host) or a plugin-owned component. +// Task menu entries additionally render a ready-made React element unchanged, +// for plugins that registered one before icons were resolved this way; that +// tolerance is menu-only (other surfaces map an element to the fallback glyph) +// and is not part of the type. type PluginIcon = string | React.ComponentType<{ className?: string }>; export type PluginNavSection = | "main" @@ -1123,7 +1129,8 @@ interface PluginRegistry { // Contributes an item to the kanban card's Edit submenu (group "edit") or // a flat, top-level card menu item after "Move to"/"Send to workflow" - // and before "Archive"/"Delete" (group "primary"). See "Kanban card contributions" below. + // and before "Archive"/"Delete" (group "primary"), optionally as a + // submenu of plugin-provided children (`items`). See "Kanban card contributions" below. registerTaskMenuAction(registration: TaskMenuActionRegistration): void; // Contributes a client-side filter section to the kanban board's display @@ -1415,15 +1422,39 @@ interface PluginTaskMenuContext { presentation: PluginPresentation; // the actual kanban layout: desktop or mobile } +interface TaskMenuSubItemRegistration { + id: string; // unique within the action; contributes to the entry's React key + label: string; + icon?: PluginIcon; + disabled?: boolean; + run(context: PluginTaskMenuContext): void | Promise; // a rejection is caught and logged +} + interface TaskMenuActionRegistration { id: string; label: string; - icon?: React.ReactNode; + icon?: PluginIcon; // "edit" nests the item in the card's Edit submenu; "primary" renders it // as a flat, top-level item after the "Move to"/"Send to workflow" // submenus and before the "Archive"/"Delete" items. group: "edit" | "primary"; visible?(context: PluginTaskMenuContext): boolean; // default: always visible + // Declaring this renders the action as a submenu instead of a flat item: + // `label` becomes an unselectable trigger and these are its children, in + // order. Must be synchronous, and is evaluated on every menu build (see + // "Kanban card contributions"). Nesting stops at this one level. A child + // needs a non-blank id and label, a callable run, and optional fields of the + // shapes above (a boolean or `null` `disabled`, an `icon` that is a name, + // component or element; `null` means absent for both) -- ids unique within the + // action: children the host cannot read or render are dropped (and reported), + // duplicate ids keep their first occurrence, and a result with nothing usable + // left falls back to `run`. + items?( + context: PluginTaskMenuContext, + ): readonly TaskMenuSubItemRegistration[]; + // Flat behavior, and the fallback whenever `items` is absent, yields no + // entries, or throws (caught and logged) -- so a host that predates + // `items` still renders a working flat item. run(context: PluginTaskMenuContext): void | Promise; // a rejection is caught and logged } @@ -1535,13 +1566,44 @@ action calls `run(context)`; a rejected promise is caught and logged to the console, and the menu still closes either way (Radix's own close-on-select, independent of the async result). -Group `"primary"` renders each visible action as its own flat, top-level menu -item instead of nesting it under `Edit`. It appears after the movement items +Group `"primary"` renders each visible action as its own top-level menu item +instead of nesting it under `Edit`. It appears after the movement items and before the Archive/Delete items on cards and on the shared desktop/mobile task-row menu. Group `"edit"` remains card-only. Visibility filtering, registration order, and `run()`/error handling are identical; the two groups are independent lists (an action only ever belongs to one). +An action from either group that declares `items(context)` renders as a +submenu instead of a flat item: `label` is the trigger (there is nothing to +run on the trigger itself), and the returned items are its children in order, +each invoked with the same `PluginTaskMenuContext` as the action. `items()` is +called synchronously while the host builds that card's or row's menu entries — +on every render, with a card's dropdown and context variants sharing one +evaluation, whether or not a menu is open — so it must read cached state rather +than fetch, and anything expensive behind it should be memoized on the state it +reads. A child's `disabled` (or the +action's own host-level disabled state, e.g. while a row-local move is +running) still renders the entry, unlike `visible()`, which filters. + +`run` is not a second action for a submenu: it stays the flat behavior for a +host that predates `items` — such a host ignores the unknown field and renders +the item it has always rendered — and the fallback whenever `items` yields no +usable children. That boundary is deliberately runtime-defensive, because a +bundle is plain JavaScript and these types are not enforced at run time: an +empty list, a throw, a promise (the contract is synchronous, and its rejection +is observed so it cannot escape as an unhandled rejection), a non-array, and an +array whose entries lack a non-blank `id` or `label`, a callable `run`, a +boolean `disabled` or a recognizable `icon` (a name, component, element or +`null`) all fall back to the flat item instead of crashing the render or +producing a trigger nothing can open. Children are read once, inside the same guard, so a +throwing getter or a Proxy fails that child rather than the card's render; +unusable children are dropped when others remain, duplicate ids keep their +first occurrence, and each defect is logged once per action and kind rather +than on every menu build. An action can therefore ship both: a +quick child list on a host that supports it, and its existing flat behavior +elsewhere. Submenu nesting stops at this one level; a `run` rejection is +caught and logged, and the menu closes either way. + `"task-card-indicators"` (documented above with the other slots) is the matching read-only surface: a small icon/badge rendered beside the PR status icon on every card, receiving `{ taskId, workspaceId, workflowStepId }`. diff --git a/docs/public/plugins-authoring.md b/docs/public/plugins-authoring.md index cc3a2d317b..dc8cf95b5b 100644 --- a/docs/public/plugins-authoring.md +++ b/docs/public/plugins-authoring.md @@ -456,7 +456,7 @@ closing future reads. | registerTaskAction | Child action inside the task menu's native Link section | Active ui.bundle | Action is revoked on unload; host supplies current task/workspace and desktop/mobile presentation | registry.registerTaskAction({ id: "link-pr", placement: "link", ... }) | | registerReviewProvider | Normalized task reviews, workspace associations, unlink, and shared Review panel | ui.bundle and matching `repository_providers[]` id | Snapshots/subscriptions are owner-scoped and revoked on unload; host owns status chrome, indicators, unlink UI, and responsive Review placement | registry.registerReviewProvider({ id: "acme", ...reviews }) | | registerTaskPanel | { id, title, titleKey?, icon?, Component, mobileEnabled?, visible?(context) }; adds a row to the task workspace's "+" (add panel) menu; Component receives { panelId, taskId, sessionId, sessionKind, presentation, conversation: { openMessage(messageId), history } }; `titleKey` is a plugin translation key with literal `title` fallback | Active ui.bundle | Panel renders behind its own error boundary with reactive localized titles; a throwing `visible` hides the item; handles are generation-bound and independently scoped, inert after unmount, identity change, disable, reload, or uninstall; `host.conversation` outside a panel returns stable empty state; desktop preserves layout identity on navigation and mobile uses the full-height Chat surface | registry.registerTaskPanel({ id: "notes", title: "Notes", titleKey: "panels.notes", Component: NotesPanel }) | -| registerTaskMenuAction | { id, label, icon?, group: "edit" \| "primary", visible?(context), run(context) }; "edit" is card-only inside Edit, while "primary" is a flat item on cards and desktop/mobile task-row menus | Active ui.bundle | Action is revoked on disable/uninstall; a throwing/rejecting run is caught and logged | registry.registerTaskMenuAction({ id: "enhance", label: "Enhance", group: "primary", run: doEnhance }) | +| registerTaskMenuAction | { id, label, icon?, group: "edit" \| "primary", visible?(context), items?(context), run(context) }; "edit" is card-only inside Edit, while "primary" is a top-level item on cards and desktop/mobile task-row menus; a synchronous `items(context)` returning TaskMenuSubItemRegistration[] ({ id, label, icon?, disabled?, run(context) }) renders the action as a submenu of those children instead | Active ui.bundle | Action is revoked on disable/uninstall; a throwing/rejecting run is caught and logged; an items() that throws or yields nothing usable falls back to the flat item, and unusable children are dropped | registry.registerTaskMenuAction({ id: "enhance", label: "Enhance", group: "primary", run: doEnhance }) | | registerTaskFilter | { id, label, getOptions(), matches(context, selected) }; adds a client-side, multi-select filter section to the kanban board's display dropdown, alongside Workflow/Repository | Active ui.bundle | Filter is revoked on disable/uninstall; selections are ephemeral (not persisted); matches is only called for a non-empty selection, and a throw is caught, logged, and treated as non-matching | registry.registerTaskFilter({ id: "tags", label: "Tags", getOptions: listTagOptions, matches: taskHasSelectedTag }) | | registerTaskListFacet | { id, label, getValues({ taskId, workspaceId }), subscribe? }; adds page-local Sort and Group choices on `/tasks` | Active ui.bundle | Values apply only to the loaded page, callbacks are isolated, and registrations are revoked on disable/unload | registry.registerTaskListFacet({ id: "tags", label: "Tag", getValues: taskTags }) | | host.React / host.jsx | Shared React instance and React.createElement alias | Active ui.bundle | No cleanup; never bundle a second React/Radix runtime | const h = host.jsx | @@ -2523,7 +2523,7 @@ Host reader because event queues are bounded and delivery is best-effort. `registerTaskMenuAction` adds an item to native task menus. Group `"edit"` is card-only and nests inside the kanban card's `Edit` submenu. Group `"primary"` -renders as a flat top-level item on cards and desktop/mobile task-row menus. +renders as a top-level item on cards and desktop/mobile task-row menus. `task-card-indicators` (see the named slots table) is the matching read-only surface, rendered beside the PR status icon on every card. `task-card-tags` is a sibling read-only surface with the same `slotProps` shape, mounted in its @@ -2558,6 +2558,40 @@ logged, not left to crash the card; the menu closes either way. Group including in desktop and phone task-row menus. Do not patch first-party task components directly. +An action of either group that declares `items(context)` renders as a submenu +instead: `label` becomes an unselectable trigger and the returned +`TaskMenuSubItemRegistration` children are its entries, in order, each called +with the same `context` as the action. Nesting stops at that one level. A +`primary` action's children also reach the command palette and the sidebar's +task commands, one command each; `edit` stays card-only, as it always was. `items()` is synchronous and is called while the host builds that +card's or row's menu entries on every render, whether or not a menu is open; a +card's dropdown and context variants are built from one evaluation, so read +cached state and memoize anything expensive. `run` stays required: it is the +flat item a host that predates `items` renders, and the fallback whenever +`items` yields nothing usable (a non-array, an empty list, a promise, or a +throw, which is caught and logged). A child needs a non-blank, unique `id`, a +non-blank `label` and a callable `run`, with optional fields of the shapes the +entry builder understands: a boolean `disabled` (or `null`, meaning enabled) and +an `icon` that is a curated name, a component, a ready-made element or `null`. A child the host cannot read +or render is dropped, and one whose `id` repeats is reported and skipped. + +```js +registry.registerTaskMenuAction({ + id: "add-tag", + label: "Add tag...", + group: "primary", + // Optional. Turns this action into a submenu of these children. + items: (context) => + recentTags(context).map((tag) => ({ + id: tag.id, + label: tag.name, + run: (ctx) => applyTag(ctx.taskId, tag.id), + })), + // Flat behavior, and the fallback when `items` has nothing usable. + run: (context) => openTagPicker(context.taskId), +}); +``` + `registerTaskFilter` adds a client-side, multi-select filter section to the kanban board's display dropdown, next to the built-in Workflow and Repository sections. The plugin supplies its own options (including any "untagged"-style diff --git a/docs/specs/plugins/requirements/plugin-task-menu-submenus.md b/docs/specs/plugins/requirements/plugin-task-menu-submenus.md new file mode 100644 index 0000000000..27826788de --- /dev/null +++ b/docs/specs/plugins/requirements/plugin-task-menu-submenus.md @@ -0,0 +1,89 @@ +--- +status: draft +system: plugins +created: 2026-09-23 +owners: + - kandev +--- +# Plugin task menu submenus Requirements + +## Overview + +A plugin task-menu action has always been exactly one selectable item. A plugin +whose natural contribution is a short list of frequent choices therefore has +nowhere to put them: the Tags plugin opens a modal picker so the user can add one +of the workspace's recent tags to a card, one menu open plus one modal plus one +click. Registering five sibling actions instead would flood every card's menu +with rows that are mostly noise. + +`registerTaskMenuAction` gains an optional `items(context)` beside its required +`run`. Declaring it renders the action as a one-level submenu whose children the +plugin supplies at menu-build time; the children reach the card menu, the shared +desktop/mobile task-row menu, the command palette and the sidebar's task +commands. `run` stays required and stays the flat behavior, which is the whole +compatibility story: a host that predates the field ignores it, and a build whose +`items()` yields nothing usable falls back to the same flat item, so an action +can ship both a quick child list and its existing behavior. + +Plugin bundles are plain JavaScript, so the registration types are a promise +rather than a guarantee. Everything here is shaped by that: the host reads plugin +values defensively at one boundary, degrades to the flat item or drops a single +child instead of failing a render, and reports what it dropped. + +## Requirements + +### REQ-PLUGINS-TASK-MENU-SUBMENUS-001: A task menu action can declare a one-level submenu + +**Intent:** A plugin with a short list of frequent, per-card choices needs to +offer them in one click each without adding sibling rows to every card's menu. +The host renders the action as a submenu when, and only when, the plugin supplies +children it can actually render. + +#### Acceptance criteria + +- **AC-PLUGINS-TASK-MENU-SUBMENUS-001.1:** `TaskMenuActionRegistration.items` SHALL be optional and synchronous, returning `readonly TaskMenuSubItemRegistration[]`; `run` SHALL remain required. A host that predates `items` SHALL ignore the unknown field and render the flat item it has always rendered, reading only `label`, `icon` and `run`. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-001.2:** When `items()` returns at least one usable child, the action SHALL render as a submenu: `label` becomes an unselectable trigger, each child becomes a selectable entry in the returned order, each child's `run` is invoked with the same `PluginTaskMenuContext` as the action, and a child is unselectable when either the child's own `disabled` or the host's entry-level `disabled` is set. Nesting SHALL stop at one level: a child is never itself a submenu. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-001.3:** A result with nothing usable SHALL fall back to the action's flat `run` item rather than producing a trigger nothing can open. Nothing usable means: `items` absent, `null`/`undefined`, a non-array, an empty array, a thrown exception, a thenable (the contract is synchronous), or an array whose every element is unusable. A thrown `items()` SHALL be caught and reported. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-001.4:** A child the host cannot render SHALL be dropped, and the drop SHALL be reported once per action and defect kind rather than once per menu build. Unusable means: `id` or `label` absent, not a string, or blank after trimming; `run` not callable; `disabled` present and neither boolean nor `null`; `icon` present and neither a curated name, a component (including React's `memo` and `forwardRef`, which are objects at run time), a ready-made element, nor `null`. A child may be read only inside a guard, so a throwing getter or a Proxy fails that child instead of the card's render. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-001.5:** A repeated child `id` SHALL keep its first occurrence and report the repeat, so the host never builds two children with one key. `icon: null` and `disabled: null` SHALL both mean "absent", matching how a JavaScript bundle spells an omitted field. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-001.6:** `items()` SHALL be evaluated while the host builds that card's or row's menu entries, which happens on every render and for both the dropdown and the context variant whether or not a menu is open. The API documentation SHALL say so and SHALL tell authors to read cached state and memoize anything expensive. + +### REQ-PLUGINS-TASK-MENU-SUBMENUS-002: An unrenderable registration never fails a host render + +**Intent:** One plugin's malformed registration must not take down a card, a +board, or a command surface. The host's own contract says unusable input degrades; +these criteria make that true for the values a real bundle can produce, including +values whose own accessors throw. + +#### Acceptance criteria + +- **AC-PLUGINS-TASK-MENU-SUBMENUS-002.1:** A registration whose own `label` or `id` is not a usable string SHALL contribute no entry at all, and SHALL be reported once, instead of rendering an unselectable or unkeyable entry. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-002.2:** The host's defect report SHALL NOT throw while reporting: a `Symbol` id, an object without a string form, or a throwing accessor on the registration SHALL still be reported or skipped without an exception escaping the builder. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-002.3:** A group `edit` registration the host cannot render SHALL leave the card's native `Edit` item flat, exactly as if no plugin action were registered, rather than wrapping it in a submenu with no plugin child under it. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-002.4:** An entry key SHALL be unique for any plugin-authored `pluginId`, action `id` and child `id`, without throwing for any string a bundle can produce, including a lone surrogate (the shape `slice` and `[0]` produce from an astral character). Because the key is also a palette row's React key and cmdk value, two entries SHALL never share one. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-002.5:** A plugin-provided value that is not a usable icon SHALL resolve to the surface's fallback glyph rather than reaching `createElement` as an element type. Context menus SHALL additionally render a ready-made element unchanged, which is the shape plugins registered before icon resolution existed still send; that tolerance SHALL be documented as menu-only. + +### REQ-PLUGINS-TASK-MENU-SUBMENUS-003: Submenu children are reachable from the command surfaces + +**Intent:** A card's plugin contribution should be usable from the same +task-action surfaces that already list card actions, rather than only inside the +card's own menu. + +#### Acceptance criteria + +- **AC-PLUGINS-TASK-MENU-SUBMENUS-003.1:** A `primary`-group submenu SHALL contribute one command per child to the command palette and the sidebar's task commands, each carrying the trigger's label as its `context` so a bare child label stays identifiable; the sidebar SHALL keep the task title as the default `context` for commands that do not set their own. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-003.2:** Group `edit` SHALL remain card-only: its submenu children SHALL NOT appear in the command palette or the sidebar's task commands. +- **AC-PLUGINS-TASK-MENU-SUBMENUS-003.3:** A surface that reuses already-built plugin entries SHALL produce them from the same inputs it renders with, and a caller that forces the flat `Edit` form SHALL NOT receive a prebuilt set that could contain the nested `Edit` submenu. + +## Exclusions + +- No nested plugin submenus: a child is a selectable item, never another submenu. +- No asynchronous `items()`: a menu entry cannot await, so a thenable is treated as a contract violation and falls back. +- The palette lists submenu children as flat commands; it does not render a nested palette submenu. +- Group `edit` gains no palette or sidebar placement. +- The host does not validate or repair a plugin's ids, labels or icons at registration time; malformed input is handled where it is used. + +## System design + +The technical source for these requirements is +[plugin-task-menu-submenus](../system-design/plugin-task-menu-submenus.md). diff --git a/docs/specs/plugins/system-design/plugin-task-menu-submenus.md b/docs/specs/plugins/system-design/plugin-task-menu-submenus.md new file mode 100644 index 0000000000..6952b06cbe --- /dev/null +++ b/docs/specs/plugins/system-design/plugin-task-menu-submenus.md @@ -0,0 +1,192 @@ +--- +status: draft +system: plugins +requirements: + - REQ-PLUGINS-TASK-MENU-SUBMENUS-001 + - REQ-PLUGINS-TASK-MENU-SUBMENUS-002 + - REQ-PLUGINS-TASK-MENU-SUBMENUS-003 +created: 2026-09-23 +owners: + - kandev +--- +# Plugin task menu submenus System Design + +## Purpose and boundaries + +This design covers the host half of plugin task-menu submenus: the registration +contract, the one boundary that reads plugin values, the entry builder every card +and row surface uses, and the flattening that makes submenu children reachable +from the command palette and the sidebar's task commands. + +It does not cover the plugin-facing authoring guide itself, the plugin-side +implementation that consumes the contract, or host-owned menu entries other than +plugin actions. Rendering changed no component: `KanbanCardMenuEntry` already +models `kind: "submenu"` for the built-in Move to / Priority / Link menus and both +renderers recurse, so cards, the shared desktop/mobile task-row menu, the +preview/detail menus and the palette inherit submenus from one entry model. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-PLUGINS-TASK-MENU-SUBMENUS-001` | [Registration contract](#registration-contract), [Entry building](#entry-building), [The reading boundary](#the-reading-boundary) | +| `REQ-PLUGINS-TASK-MENU-SUBMENUS-002` | [The reading boundary](#the-reading-boundary), [Keys and identity](#keys-and-identity), [Icon resolution](#icon-resolution) | +| `REQ-PLUGINS-TASK-MENU-SUBMENUS-003` | [Command surfaces](#command-surfaces), [Shared entry reuse](#shared-entry-reuse) | + +## Registration contract + +`TaskMenuActionRegistration` gains optional `items?(context)`. `run` stays +required, so there is one contract for two host generations: a host predating the +field reads only `label`, `icon` and `run` and renders the flat item, and a build +whose `items()` yields nothing usable does the same. The host never awaits a menu +item, so `items()` is synchronous by contract. + +`TaskMenuSubItemRegistration` is deliberately smaller than the parent: `id`, +`label`, optional `icon` and `disabled`, and `run`. It has no `group` and no +`visible`, because a child exists only inside its parent's submenu, and its own +nesting is not supported. + +## Entry building + +`pluginMenuEntry` produces exactly one `KanbanCardMenuEntry | null` per +registration: a `submenu` when `pluginSubItems` returns usable children, otherwise +a flat `item`, otherwise `null` for a registration the host cannot render. Both +callers — `buildPrimaryPluginEntries` (group `primary`) and `buildEditMenuEntry` +(group `edit`, nested inside the native `Edit` item) — filter that `null`. + +`buildEditMenuEntry` builds and filters the plugin entries **before** choosing +between the submenu and the flat item, so a registration list that yields nothing +still leaves the native `Edit` item exactly as it was rather than wrapping it in a +submenu with no plugin child (AC-PLUGINS-TASK-MENU-SUBMENUS-002.3). + +`buildCardPluginEntries` builds the two plugin-derived pieces of a card menu once +per render from the card's own inputs, and `useKanbanCardMenus` passes that result +to both the dropdown and the context variant, so a plugin's `items()` runs once per +card per render rather than twice. + +## The reading boundary + +`pluginSubItems` is the single place where host code reads a plugin's `items()` +result, and its whole body is one `try`. A plugin object may throw from any +property access — including the array methods a naive implementation would call +and the `then` lookup a naive thenable check would perform — so: + +- the result is copied with `Array.from`, which never calls a method on the + plugin's own object; +- every child is read once, inside the guard, into a snapshot of plain values, so + a throwing getter or a Proxy fails that child and nothing is read twice; +- `id` and `label` must be non-blank strings after trimming, `run` must be + callable, `disabled` must be a boolean or `null`, and `icon` must be a curated + name, a component, a ready-made element or `null`; +- duplicate ids keep their first occurrence and the repeat is reported; +- a throw, a thenable, a non-array, an empty list or an all-unusable list return + `null`, which is AC-PLUGINS-TASK-MENU-SUBMENUS-001.3's flat fallback. + +Defects are reported through one helper keyed by action identity **and kind**, so +a registration that fails every render is reported once while a later, different +defect on the same action still reports. The helper never interpolates a raw +plugin value into a message: template interpolation calls `ToString`, which throws +for a `Symbol` id or a null-prototype object, and the omission paths are exactly +where such a value arrives (AC-PLUGINS-TASK-MENU-SUBMENUS-002.2). The read of the +registration's own `label`, `id` and `icon` is guarded for the same reason, and a +registration missing any of them contributes no entry. + +The registry also reads each action inside a guard when it filters by group and +copies the registration. It skips an action whose getter or Proxy throws, reports +that registration once, and keeps other actions available. It does not validate +the registration when the plugin registers it. + +The set of already-reported defects is module state that is never cleared. A +defect of the *same* kind that recurs on the same action after a plugin generation +swap therefore stays silent for the life of the page; that is a deliberate +tradeoff for a diagnostic line, and it is documented at the helper rather than +implied. + +## Keys and identity + +A plugin entry key is `"-%"`, and a +child's key is `"#"`. Both ids are plugin-authored +strings and the palette flattens every plugin entry into one list where a key is +both a React key and cmdk's value, so the encoding has to be injective for any +string a bundle can produce. + +`encodeKeyPart` percent-encodes per UTF-16 code unit, escaping everything +`encodeURIComponent` would not leave alone. Two properties matter: + +- it cannot throw. `encodeURIComponent` raises `URIError` on a lone surrogate, + which `slice` or `[0]` produce from an astral character, and that exception + leaves the builder a card calls during render with no error boundary above it. +- it is injective. `%` is itself escaped, and every escape is exactly four hex + digits, which makes each token self-delimiting so no escaped unit can run into + the literal characters that follow it — `"%0"` and `"\u0250"` must not share a + key, and neither may two different `(pluginId, actionId)` pairs whose ids + differ only in how a dash join would spell them. + +A child key always contains the `#` separator and a flat key never does, because +encoded ids contain neither `#` nor a bare `%`, so a child key can never equal a +flat action's key. + +## Icon resolution + +`lookupPluginIcon` resolves a curated name for strings only, and passes through a +component — including React's exotic components. `memo` and `forwardRef` return +objects that are callable per their types but not at run time, and +`@tabler/icons-react`, the set the curated map itself is built from, produces +exactly that shape, so a plain `typeof` check would reject the host's own icons. +`lazy` is excluded deliberately: it cannot resolve synchronously in a menu and +would suspend the menu's content. Elements are excluded here too, because this +function's result is passed to `createElement` by every non-menu surface; the +menu's own path returns an element unchanged before reaching it, which is the +documented, menu-only element tolerance (AC-PLUGINS-TASK-MENU-SUBMENUS-002.5). +The lookup resolves own keys only, so `PLUGIN_ICONS["__proto__"]` or +`["constructor"]` cannot resolve through the prototype chain to a non-icon. + +## Command surfaces + +`pluginCommandChoices` walks a built entry and turns a submenu into one command +per selectable child, carrying the trigger's label as the command's `context`; +`buildSidebarTaskCommands` preserves a command's own `context` and keeps the task +title as the default for everything else. The palette feeds it group `primary` +entries only, so `edit` stays card-only +(AC-PLUGINS-TASK-MENU-SUBMENUS-003.1, AC-PLUGINS-TASK-MENU-SUBMENUS-003.2). A +command's id is the entry key, which is what makes the two properties above matter +to the palette as well. + +## Shared entry reuse + +`BuildKanbanCardMenuEntriesArgs` accepts prebuilt plugin entries so one card +render can share them between its variants. Because those entries carry a disabled +state, an edit handler and a menu context, `forceFlatEdit` outranks a prebuilt set +inside `buildKanbanCardMenuEntries`, and the non-card menu tiers additionally drop +any set they are handed: a surface that forces the flat `Edit` form must never +render the nested `Edit` submenu those entries could contain +(AC-PLUGINS-TASK-MENU-SUBMENUS-003.3). The field is documented as valid only for a +call whose inputs match the ones the entries were built from; it is not +runtime-guarded, because no caller passes a mismatch today and the guard would +need the inputs carried alongside for a hazard that does not exist yet. + +## Compatibility and failure behavior + +- A host predating `items` ignores it: the field is additive and `run` is + unchanged, so every existing plugin keeps its exact behavior. +- A registration the host cannot render contributes nothing and is reported, + rather than rendering a broken row. +- A child the host cannot render is dropped, reported once, and the remaining + children still render; if none remain the action falls back to its flat item. +- An exception anywhere in the plugin's `items()`, or in reading a registration's + own fields, degrades at the boundary instead of propagating out of a render. + +## Amendment log + +**Amendment 1 (2026-09-23), from the adversarial review loop on PR #3874.** The +first implementation of this design used `encodeURIComponent` for the key parts +and a variable-width escape for the fallback encoder, and reported defects with a +raw template interpolation of the action's identity. Three defects came out of +review, each with a reproduction: an id containing a lone surrogate threw +`URIError` out of the card's render; `"%0"` and `"\u0250"` collided because a +two-digit escape is not self-delimiting; and a `Symbol` or null-prototype id made +the defect report itself throw. The contract is unchanged; the implementation now +escapes per code unit at a fixed width, reads plugin values defensively on every +path that reports them, admits `memo`/`forwardRef` component objects while +excluding `lazy` and elements, treats `null` as absent for both `icon` and +`disabled`, and keeps the native `Edit` item flat when no plugin entry wraps it.