diff --git a/apps/server/src/agents/manager.ts b/apps/server/src/agents/manager.ts index c0edec8a..15f4c1b5 100644 --- a/apps/server/src/agents/manager.ts +++ b/apps/server/src/agents/manager.ts @@ -38,7 +38,15 @@ import { writeLatestEventIfCurrent, } from "./events.js"; import { runLifecycleHook } from "./lifecycle-hooks.js"; -import { clearBlankPinFields, mergePin } from "./pin-merge.js"; +import { + MAX_PINS, + type PinSpec, + applyPinSpec, + applyPinSpecs, + removePinGroup, + removePinsByIds, + replacePinGroup, +} from "./pin-write.js"; import { validatePinCaption, validatePinShortcutFields, @@ -90,14 +98,6 @@ export type { const CODEX_FULL_ACCESS_ARG = "--dangerously-bypass-approvals-and-sandbox"; const CLAUDE_FULL_ACCESS_ARG = "--dangerously-skip-permissions"; -/** - * Maximum number of pins per agent. Enforced by `upsertPin` when adding - * one at a time and by `normalizeInitialPins` when seeding via - * `createAgent({ initialPins })`. Pins also flow into the startup - * prompt via `buildStartupPrompt`, so the cap also bounds prompt size. - */ -const MAX_PINS = 50; - /** * Validate + de-duplicate the `initialPins` array supplied to * `createAgent`. De-dup is case-insensitive on label with last-write-wins @@ -1135,41 +1135,27 @@ export class AgentManager { } /** - * Update in place when the label already exists, append otherwise. Position + * Update in place when the pin already exists, append otherwise. Position * is deliberately stable: re-pinning to refresh a value must not shuffle the * sidebar out from under the user, and grouped pins would tear apart if an - * update relocated a member. An agent that wants a pin moved deletes it and - * pins it again. + * update relocated a member. + * + * The pin is addressed by `id` when the caller supplies one and by label + * otherwise — see `applyPinSpec`, which both this and the batch path share + * so the two cannot drift apart. */ async upsertPin( id: string, - pin: AgentPin + pin: PinSpec ): Promise<{ agent: AgentRecord; pin: AgentPin; created: boolean }> { - let stored: AgentPin = pin; + // Assigned by the mutation below, which always runs before we read it. + let stored!: AgentPin; let created = true; await this.mutatePins(id, (currentPins) => { - const index = currentPins.findIndex( - (p) => p.label.toLowerCase() === pin.label.toLowerCase() - ); - if (index !== -1) { - const pins = [...currentPins]; - stored = mergePin( - { - ...currentPins[index]!, - id: currentPins[index]!.id ?? randomUUID(), - }, - pin - ); - created = false; - pins[index] = stored; - return pins; - } - if (currentPins.length >= MAX_PINS) { - throw new AgentError(`Maximum of ${MAX_PINS} pins reached.`, 400); - } - stored = clearBlankPinFields({ ...pin, id: pin.id ?? randomUUID() }); - created = true; - return [...currentPins, stored]; + const result = applyPinSpec(currentPins, pin); + stored = result.stored; + created = result.created; + return result.pins; }); return { @@ -1179,14 +1165,62 @@ export class AgentManager { }; } + /** + * Write many pins in one transaction. + * + * The point is atomicity and a single round trip: applying N pins through + * `upsertPin` costs N transactions, N `getAgent` reads and N sidebar + * re-renders, and a failure halfway leaves the set half-applied. + * + * In `replace` mode the named group is rebuilt to contain exactly `specs`, + * in order. There is deliberately no whole-list replace: every destructive + * batch has to name the group it is allowed to clear, so no call can remove + * a pin the agent forgot to restate. + */ + async upsertPins( + id: string, + specs: PinSpec[], + options: { mode?: "merge" | "replace"; group?: string } = {} + ): Promise<{ agent: AgentRecord }> { + const mode = options.mode ?? "merge"; + if (mode === "replace" && !options.group?.trim()) { + throw new AgentError( + "Replace mode requires a group to scope the replacement to.", + 400 + ); + } + + await this.mutatePins(id, (currentPins) => + mode === "replace" + ? replacePinGroup(currentPins, options.group!, specs).pins + : applyPinSpecs(currentPins, specs).pins + ); + + return { agent: (await this.getAgent(id)) as AgentRecord }; + } + async deletePinById(id: string, pinId: string): Promise { - await this.mutatePins(id, (currentPins) => { - const pins = currentPins.filter((p) => p.id !== pinId); - if (pins.length === currentPins.length) { - throw new AgentError("Pin not found.", 404); - } - return pins; - }); + await this.mutatePins(id, (currentPins) => + removePinsByIds(currentPins, [pinId]) + ); + + return (await this.getAgent(id)) as AgentRecord; + } + + /** Delete several pins by id in one transaction; every id must exist. */ + async deletePinsByIds(id: string, pinIds: string[]): Promise { + await this.mutatePins(id, (currentPins) => + removePinsByIds(currentPins, pinIds) + ); + + return (await this.getAgent(id)) as AgentRecord; + } + + /** Clear an entire group in one transaction. */ + async deletePinsByGroup(id: string, group: string): Promise { + await this.mutatePins(id, (currentPins) => + removePinGroup(currentPins, group) + ); return (await this.getAgent(id)) as AgentRecord; } diff --git a/apps/server/src/agents/pin-merge.ts b/apps/server/src/agents/pin-merge.ts index e6013e5d..050b6461 100644 --- a/apps/server/src/agents/pin-merge.ts +++ b/apps/server/src/agents/pin-merge.ts @@ -1,5 +1,13 @@ import type { AgentPin } from "./types.js"; +/** + * A pin whose type has been resolved to a concrete string but not yet checked + * against the allowed set. Merging happens before validation — the effective + * type can come from the stored pin — so these helpers work at this width and + * the caller narrows to `AgentPin` afterwards. + */ +export type DraftPin = Omit & { type: string }; + /** Decorations that an agent clears by sending an empty string. */ const CLEARABLE_FIELDS = ["caption", "group", "icon"] as const; @@ -15,7 +23,7 @@ const SHORTCUT_ONLY_FIELDS = [ * Optional pin decorations are cleared by passing an empty string — there is * no other way to remove one, since an omitted field means "leave as-is". */ -export function clearBlankPinFields(pin: AgentPin): AgentPin { +export function clearBlankPinFields(pin: T): T { const cleared = { ...pin }; for (const field of CLEARABLE_FIELDS) { if (cleared[field] !== undefined && cleared[field]!.trim() === "") { @@ -26,27 +34,35 @@ export function clearBlankPinFields(pin: AgentPin): AgentPin { } /** - * Merge an incoming pin onto the one already stored under the same label. + * Drop decorations the resolved type has no meaning for, and clear the ones + * the agent blanked. + * + * Every write ends here, create and update alike: "the resolved type governs + * which decorations survive" is one rule, and stating it per-branch is how a + * plain pin ends up stored carrying `confirm` that nothing can then remove + * (`variant`/`confirm`/`disabled` aren't clearable by empty string). + */ +export function finalizePin(pin: T): T { + const finalized = clearBlankPinFields(pin); + if (finalized.type !== "shortcut") { + for (const field of SHORTCUT_ONLY_FIELDS) delete finalized[field]; + } + return finalized; +} + +/** + * Merge an incoming pin onto the one already stored. * * Merge rather than replace: an agent re-pinning to change one thing (add a * group, refresh a value) shouldn't have to restate every decoration or * silently lose it. Fields the agent omits keep their stored value; fields it - * sends as an empty string are removed. + * sends as an empty string are removed. Callers run `finalizePin` on the + * result — including to strip shortcut-only fields when a pin is re-typed. */ -export function mergePin(existing: AgentPin, incoming: AgentPin): AgentPin { - const merged = clearBlankPinFields({ +export function mergePin(existing: AgentPin, incoming: DraftPin): DraftPin { + return { ...existing, ...incoming, id: existing.id ?? incoming.id, - }); - - // Omitting a field means "keep it", which would otherwise let a shortcut's - // icon/variant/confirm/disabled ride along when the pin is re-typed as - // something else — stale state an agent could see in dispatch_list_pins - // and have no way to clear. - if (merged.type !== "shortcut") { - for (const field of SHORTCUT_ONLY_FIELDS) delete merged[field]; - } - - return merged; + }; } diff --git a/apps/server/src/agents/pin-write.ts b/apps/server/src/agents/pin-write.ts new file mode 100644 index 00000000..a1b35548 --- /dev/null +++ b/apps/server/src/agents/pin-write.ts @@ -0,0 +1,310 @@ +import { randomUUID } from "node:crypto"; + +import { + isPinType, + validatePinShortcutFields, + validatePinValue, + type PinType, +} from "../pins.js"; +import { AgentError } from "./errors.js"; +import { finalizePin, mergePin, type DraftPin } from "./pin-merge.js"; +import type { AgentPin } from "./types.js"; + +/** + * Maximum number of pins per agent. Enforced by every write path here and by + * `normalizeInitialPins` when seeding via `createAgent({ initialPins })`. Pins + * also flow into the startup prompt via `buildStartupPrompt`, so the cap also + * bounds prompt size. + */ +export const MAX_PINS = 50; + +/** + * A pin as an agent submits it. + * + * `type` and `value` are optional because an update that omits either + * inherits the stored one. Defaulting `type` to "string" would make a pure + * relabel silently demote a shortcut pin and drop its icon; requiring `value` + * would make the same relabel restate the prompt it is not changing. Both are + * required in effect when creating, enforced in `toStorablePin`. + */ +export type PinSpec = Omit & { + type?: PinType; + value?: string; +}; + +/** + * Validate the pin that is actually about to be stored, narrowing it. + * + * This runs after the merge rather than on the incoming spec, because the + * effective type may come from the stored pin — validating the request alone + * would check the value against the wrong type. + */ +function validateStoredPin(pin: DraftPin): AgentPin { + if (!isPinType(pin.type)) { + throw new AgentError(`Invalid pin type: ${pin.type}`, 400); + } + const stored: AgentPin = { ...pin, type: pin.type }; + try { + validatePinValue(stored.type, stored.value); + if (stored.type === "shortcut") validatePinShortcutFields(stored); + } catch (error) { + throw new AgentError( + error instanceof Error ? error.message : String(error), + 400 + ); + } + return stored; +} + +/** + * Resolve a spec against the pin it is updating, inheriting an omitted type or + * value. With nothing to inherit from we are creating, and a pin with no value + * has nothing to display — so that is the one case where value is mandatory. + */ +function toStorablePin(spec: PinSpec, existing?: AgentPin): DraftPin { + const value = spec.value ?? existing?.value; + if (value === undefined) { + throw new AgentError("value is required when creating a pin.", 400); + } + return { ...spec, value, type: spec.type ?? existing?.type ?? "string" }; +} + +/** Case-insensitive label equality — the historical uniqueness rule. */ +function sameLabel(a: string, b: string): boolean { + return a.toLowerCase() === b.toLowerCase(); +} + +function sameGroup(pin: AgentPin, group: string): boolean { + return (pin.group ?? "").trim().toLowerCase() === group.trim().toLowerCase(); +} + +/** + * A blank group name matches every *ungrouped* pin, so accepting one would + * turn "clear this group" into "delete everything without a heading". Group + * targeting is the safety boundary for the destructive paths — it has to name + * something. + */ +function assertNamedGroup(group: string): void { + if (group.trim() === "") { + throw new AgentError("A group name is required.", 400); + } +} + +/** Enforce case-insensitive label uniqueness across a finished pin array. */ +function assertUniqueLabels(pins: AgentPin[]): void { + const seen = new Set(); + for (const pin of pins) { + const key = pin.label.toLowerCase(); + if (seen.has(key)) { + throw new AgentError( + `Two pins would share the label "${pin.label}".`, + 400 + ); + } + seen.add(key); + } +} + +/** + * Locate the pin a spec addresses. + * + * `id` wins when present: it is the only handle that survives a relabel, so + * matching it first is what makes renaming a patch rather than a delete and + * recreate. An `id` that matches nothing is an error rather than a create — + * a typo'd id should not quietly become a second pin alongside the one the + * agent meant to edit. + */ +function findTarget(pins: AgentPin[], spec: PinSpec): number { + if (spec.id !== undefined) { + const index = pins.findIndex((pin) => pin.id === spec.id); + if (index === -1) { + throw new AgentError(`Pin not found: ${spec.id}`, 404); + } + return index; + } + return pins.findIndex((pin) => sameLabel(pin.label, spec.label)); +} + +/** + * Labels are unique case-insensitively, and the sidebar leans on it. A rename + * is the one write that can break the invariant, so it is checked against + * every pin except the one being edited. + */ +function assertLabelFree( + pins: AgentPin[], + label: string, + exceptIndex: number +): void { + const clash = pins.findIndex( + (pin, index) => index !== exceptIndex && sameLabel(pin.label, label) + ); + if (clash !== -1) { + throw new AgentError(`Another pin already uses the label "${label}".`, 400); + } +} + +export type ApplyPinResult = { + pins: AgentPin[]; + stored: AgentPin; + created: boolean; +}; + +/** + * Apply one pin spec to the array, returning a new array. + * + * Position is deliberately stable on update: re-pinning to refresh a value + * must not shuffle the sidebar out from under the user, and grouped pins + * would tear apart if an update relocated a member. + */ +export function applyPinSpec(pins: AgentPin[], spec: PinSpec): ApplyPinResult { + const index = findTarget(pins, spec); + + if (index !== -1) { + assertLabelFree(pins, spec.label, index); + const existing = pins[index]!; + const stored = validateStoredPin( + finalizePin( + mergePin( + { ...existing, id: existing.id ?? randomUUID() }, + toStorablePin(spec, existing) + ) + ) + ); + const next = [...pins]; + next[index] = stored; + return { pins: next, stored, created: false }; + } + + if (pins.length >= MAX_PINS) { + throw new AgentError(`Maximum of ${MAX_PINS} pins reached.`, 400); + } + const stored = validateStoredPin( + finalizePin({ ...toStorablePin(spec), id: spec.id ?? randomUUID() }) + ); + return { pins: [...pins, stored], stored, created: true }; +} + +/** Fold a batch of specs through {@link applyPinSpec}, in order. */ +export function applyPinSpecs( + pins: AgentPin[], + specs: PinSpec[] +): { pins: AgentPin[]; stored: AgentPin[] } { + let next = pins; + const stored: AgentPin[] = []; + for (const spec of specs) { + const result = applyPinSpec(next, spec); + next = result.pins; + stored.push(result.stored); + } + return { pins: next, stored }; +} + +/** + * Make `group` contain exactly `specs`, in the order given, without deleting + * anything outside it. + * + * Specs resolve against the whole array (by id, then by label) so an existing + * pin can be pulled into the group and keep its id and decorations; a pin the + * agent names this way moves rather than being duplicated. Members of the + * group that no spec claimed are dropped — that is the whole point of a + * replace — but a pin outside the group is only ever moved by being named, + * never removed. + * + * The rebuilt block sits where the group already started, so replacing a + * group's contents does not make it jump position in the sidebar. + */ +export function replacePinGroup( + pins: AgentPin[], + group: string, + specs: PinSpec[] +): { pins: AgentPin[]; stored: AgentPin[] } { + assertNamedGroup(group); + const claimed = new Set(); + const stored: AgentPin[] = []; + + for (const rawSpec of specs) { + // Filing members under the group is this function's own job — leaving it + // to the caller means the primitive cannot honour its name, and a member + // stored without `group` renders under no heading yet sits in the block, + // invisible to the next replace of that same group. + const spec: PinSpec = { ...rawSpec, group }; + const index = findTarget(pins, spec); + if (index === -1) { + stored.push( + validateStoredPin( + finalizePin({ + ...toStorablePin(spec), + id: spec.id ?? randomUUID(), + }) + ) + ); + continue; + } + if (claimed.has(index)) { + throw new AgentError( + `Two entries in the batch address the same pin "${pins[index]!.label}".`, + 400 + ); + } + claimed.add(index); + const existing = pins[index]!; + stored.push( + validateStoredPin( + finalizePin( + mergePin( + { ...existing, id: existing.id ?? randomUUID() }, + toStorablePin(spec, existing) + ) + ) + ) + ); + } + + // Anchor the block where the group already lived, so a replace does not + // relocate it relative to ungrouped pins. + const anchor = pins.findIndex((pin) => sameGroup(pin, group)); + const survivors = pins + .map((pin, index) => ({ pin, index })) + .filter(({ pin, index }) => !claimed.has(index) && !sameGroup(pin, group)); + + const next: AgentPin[] = []; + let inserted = false; + for (const { pin, index } of survivors) { + if (!inserted && anchor !== -1 && index > anchor) { + next.push(...stored); + inserted = true; + } + next.push(pin); + } + if (!inserted) next.push(...stored); + + // Each spec resolves against the *original* array, so two creates sharing a + // new label would both look unmatched and slip through — check the finished + // array instead. This also catches a create colliding with a survivor. + assertUniqueLabels(next); + + if (next.length > MAX_PINS) { + throw new AgentError(`Maximum of ${MAX_PINS} pins reached.`, 400); + } + return { pins: next, stored }; +} + +/** Remove pins by id. Every id must exist, so a typo surfaces rather than no-ops. */ +export function removePinsByIds(pins: AgentPin[], ids: string[]): AgentPin[] { + const wanted = new Set(ids); + const missing = ids.filter((id) => !pins.some((pin) => pin.id === id)); + if (missing.length > 0) { + throw new AgentError(`Pin not found: ${missing.join(", ")}`, 404); + } + return pins.filter((pin) => !wanted.has(pin.id ?? "")); +} + +/** Remove every pin in a group. A group with no members is a 404, matching delete-by-id. */ +export function removePinGroup(pins: AgentPin[], group: string): AgentPin[] { + assertNamedGroup(group); + const next = pins.filter((pin) => !sameGroup(pin, group)); + if (next.length === pins.length) { + throw new AgentError(`No pins in group "${group}".`, 404); + } + return next; +} diff --git a/apps/server/src/routes/mcp.ts b/apps/server/src/routes/mcp.ts index f8eb4818..a2761542 100644 --- a/apps/server/src/routes/mcp.ts +++ b/apps/server/src/routes/mcp.ts @@ -69,6 +69,7 @@ type McpRouteDeps = { mcpAddReviewThreadMessage: unknown; mcpListReviewFeedback: unknown; mcpUpsertPin: unknown; + mcpUpsertPins: unknown; mcpDeletePin: unknown; mcpDeletePinByLabel: unknown; mcpGetParentContext: unknown; @@ -214,6 +215,7 @@ export async function registerMcpRoutes( updateWhiteboard: deps.mcpUpdateWhiteboard, clearWhiteboard: deps.mcpClearWhiteboard, upsertPin: deps.mcpUpsertPin, + upsertPins: deps.mcpUpsertPins, deletePin: deps.mcpDeletePin, deletePinByLabel: deps.mcpDeletePinByLabel, listPins: deps.mcpListPins, @@ -328,6 +330,7 @@ export async function registerMcpRoutes( sendMessage: deps.mcpSendMessage, listAgentsForAgent: deps.mcpListAgentsForAgent, upsertPin: deps.mcpUpsertPin, + upsertPins: deps.mcpUpsertPins, deletePin: deps.mcpDeletePin, deletePinByLabel: deps.mcpDeletePinByLabel, listPins: deps.mcpListPins, diff --git a/apps/server/src/server.ts b/apps/server/src/server.ts index 1f094fb2..62b495ea 100644 --- a/apps/server/src/server.ts +++ b/apps/server/src/server.ts @@ -635,6 +635,7 @@ async function registerRoutes() { mcpSendMessage: mcpHandlers.sendMessage, mcpListAgentsForAgent: mcpHandlers.listAgentsForAgent, mcpUpsertPin: mcpHandlers.upsertPin, + mcpUpsertPins: mcpHandlers.upsertPins, mcpDeletePin: mcpHandlers.deletePin, mcpDeletePinByLabel: mcpHandlers.deletePinByLabel, mcpListPins: mcpHandlers.listPins, diff --git a/apps/server/src/server/mcp-handlers.ts b/apps/server/src/server/mcp-handlers.ts index ca1e18ee..b96323f5 100644 --- a/apps/server/src/server/mcp-handlers.ts +++ b/apps/server/src/server/mcp-handlers.ts @@ -5,7 +5,8 @@ import { mkdir, readFile, unlink, writeFile } from "node:fs/promises"; import type { FastifyBaseLogger } from "fastify"; import type { Pool } from "pg"; -import type { AgentManager, AgentPin, AgentRecord } from "../agents/manager.js"; +import type { AgentManager, AgentRecord } from "../agents/manager.js"; +import type { PinSpec } from "../agents/pin-write.js"; import { AgentError } from "../agents/errors.js"; import type { WorktreeCleanupMode } from "../agents/types.js"; import { @@ -33,7 +34,12 @@ import { validatePinValue, type PinShortcutVariant, } from "../pins.js"; -import { toPinListing, type PinListing } from "./pin-listing.js"; +import { + toPinListing, + toPinSummary, + type PinListing, + type PinSummary, +} from "./pin-listing.js"; import { resolveRepoRoot } from "../shared/git/git-context.js"; import { isMediaFile, isTextFile, resolveMediaDir } from "../shared/media.js"; import type { PublishUiEvent, SendAgentPrompt } from "./mcp-handler-types.js"; @@ -181,29 +187,46 @@ async function handleSendNotify( return deps.slackNotifier.sendNotification(agent, input); } -async function handleUpsertPin( - deps: CreateMcpHandlersDeps, - agentId: string, - pin: { - label: string; - value: string; - type: string; - caption?: string; - group?: string; - icon?: string; - variant?: string; - confirm?: boolean; - disabled?: boolean; - } -): Promise<{ pin: PinListing; created: boolean }> { - if (!isPinType(pin.type)) { +type PinInput = { + id?: string; + label: string; + value?: string; + type?: string; + caption?: string; + group?: string; + icon?: string; + variant?: string; + confirm?: boolean; + disabled?: boolean; +}; + +/** + * Narrow one pin spec to a storable shape. + * + * Shared by the single and batch write paths so a pin the batch tool accepts + * is exactly a pin `dispatch_pin` would have accepted — a batch must not + * become a way to smuggle in a shape the single-pin validator rejects. + * + * An omitted `type` stays omitted rather than defaulting: the write layer + * inherits the stored pin's type, so relabelling a shortcut cannot silently + * demote it to a plain string and strip its icon. Validation of the value + * happens there too, once the effective type is known. + */ +function toValidatedPin(pin: PinInput): PinSpec { + if (pin.type !== undefined && !isPinType(pin.type)) { throw new Error(`Invalid pin type: ${pin.type}`); } - validatePinValue(pin.type, pin.value); + // Only a spec carrying both can be checked here; anything relying on an + // inherited type or value is validated in `pin-write` once merged. + if (pin.type !== undefined && pin.value !== undefined) { + validatePinValue(pin.type, pin.value); + } // Captions and grouping are generic; button styling, confirmation, and the // disabled state only mean anything for shortcut pins — silently dropping - // those elsewhere keeps stored pins honest. + // those elsewhere keeps stored pins honest. With no type given we cannot + // tell yet, so they ride along and `mergePin` strips them if the resolved + // type turns out not to be shortcut. if (pin.caption !== undefined) { validatePinCaption(pin.caption); } @@ -211,24 +234,37 @@ async function handleUpsertPin( if (isShortcut) { validatePinShortcutFields(pin); } + const keepShortcutFields = pin.type === undefined || isShortcut; - const result = await deps.agentManager.upsertPin(agentId, { + return { + ...(pin.id !== undefined ? { id: pin.id } : {}), label: pin.label, - value: pin.value, - type: pin.type, + ...(pin.value !== undefined ? { value: pin.value } : {}), + ...(pin.type !== undefined ? { type: pin.type } : {}), ...(pin.caption !== undefined ? { caption: pin.caption } : {}), ...(pin.group !== undefined ? { group: pin.group } : {}), - ...(isShortcut && pin.icon !== undefined ? { icon: pin.icon } : {}), - ...(isShortcut && pin.variant !== undefined + ...(keepShortcutFields && pin.icon !== undefined ? { icon: pin.icon } : {}), + ...(keepShortcutFields && pin.variant !== undefined ? { variant: pin.variant as PinShortcutVariant } : {}), - ...(isShortcut && pin.confirm !== undefined + ...(keepShortcutFields && pin.confirm !== undefined ? { confirm: pin.confirm } : {}), - ...(isShortcut && pin.disabled !== undefined + ...(keepShortcutFields && pin.disabled !== undefined ? { disabled: pin.disabled } : {}), - }); + }; +} + +async function handleUpsertPin( + deps: CreateMcpHandlersDeps, + agentId: string, + pin: PinInput +): Promise<{ pin: PinListing; created: boolean }> { + const result = await deps.agentManager.upsertPin( + agentId, + toValidatedPin(pin) + ); deps.publishUiEvent({ type: "agent.upsert", agent: deps.withStreamFlag(result.agent), @@ -236,12 +272,53 @@ async function handleUpsertPin( return { pin: toPinListing(result.pin), created: result.created }; } +async function handleUpsertPins( + deps: CreateMcpHandlersDeps, + agentId: string, + input: { + pins: PinInput[]; + mode?: "merge" | "replace"; + group?: string; + } +): Promise { + // Validate the whole batch before opening the transaction: a bad entry at + // position 19 should fail the call outright rather than leave the first + // eighteen applied. Replace mode files entries under the scoping group + // itself, so nothing needs stamping here. + const specs = input.pins.map(toValidatedPin); + + const { agent } = await deps.agentManager.upsertPins(agentId, specs, { + ...(input.mode !== undefined ? { mode: input.mode } : {}), + ...(input.group !== undefined ? { group: input.group } : {}), + }); + deps.publishUiEvent({ + type: "agent.upsert", + agent: deps.withStreamFlag(agent), + }); + // A thin projection, not the full listing: the point of the echo is to show + // what the batch produced and in what order, and 50 pins' worth of values + // (2000 chars each) would dwarf that. dispatch_list_pins serves full state. + return (agent.pins ?? []).map(toPinSummary); +} + async function handleDeletePin( deps: CreateMcpHandlersDeps, agentId: string, - pinId: string + input: { id?: string; ids?: string[]; group?: string } ): Promise { - const agent = await deps.agentManager.deletePinById(agentId, pinId); + const targets = [input.id, input.ids, input.group].filter( + (target) => target !== undefined + ); + if (targets.length !== 1) { + throw new Error("Pass exactly one of id, ids, or group."); + } + + const agent = input.group + ? await deps.agentManager.deletePinsByGroup(agentId, input.group) + : await deps.agentManager.deletePinsByIds( + agentId, + input.ids ?? [input.id!] + ); deps.publishUiEvent({ type: "agent.upsert", agent: deps.withStreamFlag(agent), @@ -928,23 +1005,18 @@ export function createMcpHandlers(deps: CreateMcpHandlersDeps) { sendNotify: (agentId: string, input: NotifyInput) => handleSendNotify(deps, agentId, input), - upsertPin: ( + upsertPin: (agentId: string, pin: PinInput) => + handleUpsertPin(deps, agentId, pin), + + upsertPins: ( agentId: string, - pin: { - label: string; - value: string; - type: string; - caption?: string; - group?: string; - icon?: string; - variant?: string; - confirm?: boolean; - disabled?: boolean; - } - ) => handleUpsertPin(deps, agentId, pin), + input: { pins: PinInput[]; mode?: "merge" | "replace"; group?: string } + ) => handleUpsertPins(deps, agentId, input), - deletePin: (agentId: string, pinId: string) => - handleDeletePin(deps, agentId, pinId), + deletePin: ( + agentId: string, + input: { id?: string; ids?: string[]; group?: string } + ) => handleDeletePin(deps, agentId, input), deletePinByLabel: (agentId: string, label: string) => handleDeletePinByLabel(deps, agentId, label), diff --git a/apps/server/src/server/pin-listing.ts b/apps/server/src/server/pin-listing.ts index b4f3d43d..ce1ec326 100644 --- a/apps/server/src/server/pin-listing.ts +++ b/apps/server/src/server/pin-listing.ts @@ -19,6 +19,26 @@ export type PinListing = { disabled?: boolean; }; +/** + * The identity-and-position view of a pin, for echoing back a bulk write. + * Deliberately omits `value` and the decorations: a batch of 50 pins would + * otherwise return tens of KB to say what order things ended up in. + */ +export type PinSummary = { + id: string; + label: string; + group?: string; +}; + +export function toPinSummary(pin: AgentPin): PinSummary { + if (!pin.id) throw new Error("Pin is missing its stable ID."); + return { + id: pin.id, + label: pin.label, + ...(pin.group !== undefined ? { group: pin.group } : {}), + }; +} + export function toPinListing(pin: AgentPin): PinListing { if (!pin.id) throw new Error("Pin is missing its stable ID."); return { diff --git a/apps/server/src/shared/mcp/server.ts b/apps/server/src/shared/mcp/server.ts index 724fa426..cf8f702a 100644 --- a/apps/server/src/shared/mcp/server.ts +++ b/apps/server/src/shared/mcp/server.ts @@ -23,7 +23,7 @@ import type { WhiteboardGetResult, WhiteboardUpdateResult, } from "../whiteboard.js"; -import type { PinListing } from "../../server/pin-listing.js"; +import type { PinListing, PinSummary } from "../../server/pin-listing.js"; import { registerPersonaInteractionTools, type LaunchPersonaAgentType, @@ -33,6 +33,57 @@ import { loadRepoTools, type RepoToolParam } from "./repo-tools.js"; import { VALID_PIN_SHORTCUT_ICONS } from "../../pins.js"; import { toToolError } from "./tool-error.js"; +/** One pin spec as an agent supplies it, shared by the single and batch tools. */ +type McpPinInput = { + id?: string; + label: string; + /** Omitted on an update means "keep the stored value". */ + value?: string; + /** Omitted on an update means "keep the stored type". */ + type?: string; + caption?: string; + group?: string; + icon?: string; + variant?: string; + confirm?: boolean; + disabled?: boolean; +}; + +/** + * The constrained field types every pin write shares. `dispatch_pin` and + * `dispatch_pins` build their schemas from these and override only the + * `.describe()` text — duplicating the *constraints* is how a raised cap ends + * up enforced on one tool and silently not the other. + */ +const pinFields = { + id: z.string().min(1), + label: z.string().max(100), + value: z.string().max(2000), + type: z.enum([ + "string", + "url", + "port", + "code", + "pr", + "filename", + "markdown", + "shortcut", + ]), + caption: z.string().max(160), + /** A pin's own group. Must accept "" — that is how an agent clears it. */ + group: z.string().max(100), + /** + * A group named as the *target* of a bulk operation. Blank is rejected here + * because a missing group compares equal to "", so an empty name would widen + * "clear this group" into "delete every ungrouped pin". + */ + scopingGroup: z.string().trim().min(1).max(100), + icon: z.enum(VALID_PIN_SHORTCUT_ICONS), + variant: z.enum(["default", "primary", "destructive"]), + confirm: z.boolean(), + disabled: z.boolean(), +} as const; + export type McpAgent = { id: string; cwd: string; @@ -61,6 +112,7 @@ const AGENT_TOOLS = new Set([ "dispatch_rename_session", "dispatch_notify", "dispatch_pin", + "dispatch_pins", "dispatch_delete_pin", "dispatch_share", "dispatch_list_media", @@ -123,6 +175,7 @@ const JOB_TOOLS = new Set([ "dispatch_rename_session", "dispatch_notify", "dispatch_pin", + "dispatch_pins", "dispatch_delete_pin", "dispatch_share", "dispatch_list_media", @@ -176,6 +229,7 @@ const JOB_TOOLS = new Set([ const REVIEW_AGENT_TOOLS = new Set([ "dispatch_event", "dispatch_pin", + "dispatch_pins", "dispatch_delete_pin", "dispatch_share", "dispatch_list_media", @@ -420,19 +474,16 @@ export type McpRequestContext = { >; upsertPin?: ( agentId: string, - pin: { - label: string; - value: string; - type: string; - caption?: string; - group?: string; - icon?: string; - variant?: string; - confirm?: boolean; - disabled?: boolean; - } + pin: McpPinInput ) => Promise<{ pin: PinListing; created: boolean }>; - deletePin?: (agentId: string, pinId: string) => Promise; + upsertPins?: ( + agentId: string, + input: { pins: McpPinInput[]; mode?: "merge" | "replace"; group?: string } + ) => Promise; + deletePin?: ( + agentId: string, + input: { id?: string; ids?: string[]; group?: string } + ) => Promise; deletePinByLabel?: (agentId: string, label: string) => Promise; getWhiteboard?: (agentId: string) => Promise; updateWhiteboard?: ( @@ -559,6 +610,7 @@ async function createDispatchMcpServer( }); if (allowed.has("dispatch_pin")) registerPinTool(server, context); + if (allowed.has("dispatch_pins")) registerBatchPinTool(server, context); if (allowed.has("dispatch_delete_pin")) registerDeletePinTool(server, context); if (allowed.has("dispatch_share")) registerShareTool(server, context); @@ -705,71 +757,53 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { "dispatch_pin", { description: - "Pin a key-value pair to the Dispatch UI for this agent. Pins are displayed in the sidebar so users can quickly find important info. To update a pin, set it again with the same label — fields you omit keep their current value, so you can add a group or change a value without restating the rest; pass an empty string to clear caption, group, or icon. To remove a pin, use dispatch_list_pins followed by dispatch_delete_pin. The delete parameter is retained temporarily only for agents that initialized before this tool upgrade. " + + "Pin a key-value pair to the Dispatch UI for this agent. Pins are displayed in the sidebar so users can quickly find important info. To update a pin, set it again with the same label — fields you omit keep their current value, so you can add a group or change a value without restating the rest; pass an empty string to clear caption, group, or icon. To rename a pin, pass its id from dispatch_list_pins along with the new label. To write several pins at once, use dispatch_pins instead of calling this repeatedly. To remove a pin, use dispatch_list_pins followed by dispatch_delete_pin. The delete parameter is retained temporarily only for agents that initialized before this tool upgrade. " + "Good things to pin: dev server URLs (url), PR links (pr), key files changed (filename), test/build result summaries (string), DB migration names (string), relevant doc or issue links (url), architecture decisions or assumptions (string), short structured summaries (markdown), the specific blocking question when in waiting_user state (string). " + "Use type 'shortcut' to give the user a one-click button that sends a prompt back to you — the label is the button text and the value is the prompt you receive when it is clicked. Good for offering the user a concrete next step (launch this work, re-run that check, pick this approach) instead of asking them to type it. When a shortcut pin is how the user answers a question that is blocking you, also emit a waiting_user event so the agent surfaces as needing attention — the pin is the answer mechanism, not the alert. " + "When a shortcut's action becomes temporarily or permanently unavailable but is still worth showing (e.g. its build already started elsewhere), set disabled: true instead of deleting it — the button greys out and stops accepting clicks. Set the caption to explain why (e.g. 'already building — agt_...'); it renders in place of the normal caption. Send disabled: false to re-enable it later.", inputSchema: { - label: z - .string() - .max(100) + id: pinFields.id + .optional() .describe( - "Display label for the pin (e.g. 'API Server', 'Vite Dev', 'DB Port'). For shortcut pins this is the button text." + "Exact pin id from dispatch_list_pins. Pass it to edit that pin specifically — this is the only way to change a pin's label, since without an id the label is what identifies the pin. Omit to match by label." ), - value: z - .string() - .max(2000) + label: pinFields.label.describe( + "Display label for the pin (e.g. 'API Server', 'Vite Dev', 'DB Port'). For shortcut pins this is the button text." + ), + value: pinFields.value .optional() .describe( - "The value to display. For shortcut pins this is the prompt delivered to your session when the button is clicked." + "The value to display. For shortcut pins this is the prompt delivered to your session when the button is clicked. Required on a new pin; omit on an update to keep the stored value." ), - type: z - .enum([ - "string", - "url", - "port", - "code", - "pr", - "filename", - "markdown", - "shortcut", - ]) - .default("string") + type: pinFields.type + .optional() .describe( - "Value type. 'url' renders as a clickable link. 'port' renders as a monospace badge. 'code' renders as a monospace badge. 'pr' renders as a pull request link with a PR icon. 'filename' renders with a file icon in monospace. 'markdown' renders constrained markdown for short summaries. 'shortcut' renders a button that sends `value` to your session when clicked. For list-like types (filename, url, string, port), separate multiple values with commas or newlines." + "Value type, defaulting to 'string' on a new pin. Omit when updating an existing pin and its stored type is kept. 'url' renders as a clickable link. 'port' renders as a monospace badge. 'code' renders as a monospace badge. 'pr' renders as a pull request link with a PR icon. 'filename' renders with a file icon in monospace. 'markdown' renders constrained markdown for short summaries. 'shortcut' renders a button that sends `value` to your session when clicked. For list-like types (filename, url, string, port), separate multiple values with commas or newlines." ), - caption: z - .string() - .max(160) + caption: pinFields.caption .optional() .describe( "A one-line caption rendered under the pin, supporting inline markdown (bold, italic, `code`, strikethrough). Works on any pin type. On shortcut pins it is context for the click, not part of the injected prompt." ), - group: z - .string() - .max(100) + group: pinFields.group .optional() .describe( "Renders this pin under a shared heading with every other pin using the same group name — use it to present a set of related actions, or the question they answer, as one block." ), - icon: z - .enum(VALID_PIN_SHORTCUT_ICONS) + icon: pinFields.icon .optional() .describe("Shortcut pins only: icon shown on the button."), - variant: z - .enum(["default", "primary", "destructive"]) + variant: pinFields.variant .optional() .describe( "Shortcut pins only: button styling. 'primary' for the main suggested action, 'destructive' for dangerous ones, 'default' otherwise." ), - confirm: z - .boolean() + confirm: pinFields.confirm .optional() .describe( "Shortcut pins only: when true, clicking asks the user to confirm and shows them the prompt first. Use for destructive or hard-to-undo actions." ), - disabled: z - .boolean() + disabled: pinFields.disabled .optional() .describe( "Shortcut pins only: when true, the button renders non-interactive instead of being deleted — for an action that's temporarily or permanently unavailable but still worth showing. Pair with a caption explaining why. Send false to re-enable." @@ -793,15 +827,11 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { content: [{ type: "text", text: `Removed pin \"${args.label}\".` }], }; } - if (args.value === undefined) { - return toToolError( - new Error("value is required when creating or updating a pin.") - ); - } const { pin, created } = await upsertPin(agentId, { + ...(args.id !== undefined ? { id: args.id } : {}), label: args.label, - value: args.value, - type: args.type ?? "string", + ...(args.value !== undefined ? { value: args.value } : {}), + ...(args.type !== undefined ? { type: args.type } : {}), ...(args.caption !== undefined ? { caption: args.caption } : {}), ...(args.group !== undefined ? { group: args.group } : {}), ...(args.icon !== undefined ? { icon: args.icon } : {}), @@ -827,6 +857,96 @@ function registerPinTool(server: McpServer, context: McpRequestContext): void { ); } +/** + * The per-entry shape for `dispatch_pins`. Field semantics live on + * `dispatch_pin` — restating them here would double what every agent pays in + * context for the pin toolset, so this stays terse and points there. + */ +const batchPinEntrySchema = z.object({ + id: pinFields.id + .optional() + .describe("Pin id from dispatch_list_pins. Required to change a label."), + label: pinFields.label.describe("Display label, or button text."), + value: pinFields.value + .optional() + .describe( + "Value, or the prompt for a shortcut. Required on a new pin; omit on an update to keep the stored value." + ), + type: pinFields.type + .optional() + .describe( + "Defaults to 'string' on a new pin; omit on an update to keep the stored type. See dispatch_pin." + ), + caption: pinFields.caption.optional().describe("One-line caption."), + group: pinFields.group + .optional() + .describe( + "Shared heading. Ignored in replace mode, which files entries under its own group." + ), + icon: pinFields.icon.optional().describe("Shortcut pins only."), + variant: pinFields.variant.optional().describe("Shortcut pins only."), + confirm: pinFields.confirm.optional().describe("Shortcut pins only."), + disabled: pinFields.disabled.optional().describe("Shortcut pins only."), +}); + +function registerBatchPinTool( + server: McpServer, + context: McpRequestContext +): void { + if (!context.agent || !context.upsertPins) return; + const agentId = context.agent.id; + const upsertPins = context.upsertPins; + + server.registerTool( + "dispatch_pins", + { + description: + "Write several sidebar pins in one atomic call — use this instead of calling dispatch_pin in a loop. Each entry behaves exactly like dispatch_pin: it updates the pin matching its id (or, with no id, its label) and creates one otherwise, keeping any field you omit. Because an id survives a relabel, relabelling a whole set is one call here rather than a delete and recreate per pin. " + + "Default mode 'merge' leaves pins you did not mention alone. Mode 'replace' requires a group and makes that group contain exactly the entries you pass, in the order you pass them — members you omit are deleted, and nothing outside the group is ever removed. Use replace to reorder a group or rewrite it wholesale; use merge for everything else. Returns the full resulting pin list.", + inputSchema: { + pins: z + .array(batchPinEntrySchema) + .min(1) + .max(50) + .describe("Pins to write, applied in order."), + mode: z + .enum(["merge", "replace"]) + .default("merge") + .describe( + "'merge' updates or creates each entry and touches nothing else. 'replace' rebuilds the named group to be exactly these entries." + ), + group: pinFields.scopingGroup + .optional() + .describe( + "Required by mode 'replace': the only group the call may delete from. Entries are filed under it automatically." + ), + }, + }, + async (args) => { + try { + const pins = await upsertPins(agentId, { + pins: args.pins, + ...(args.mode !== undefined ? { mode: args.mode } : {}), + ...(args.group !== undefined ? { group: args.group } : {}), + }); + // Echo the resulting list, not the request: an agent can then see what + // the batch actually produced — order included — rather than assuming + // its input round-tripped. + return { + content: [ + { + type: "text", + text: `Wrote ${args.pins.length} pin(s). Pins are now: ${JSON.stringify(pins)}`, + }, + ], + }; + } catch (error) { + return toToolError(error); + } + } + ); +} + function registerDeletePinTool( server: McpServer, context: McpRequestContext @@ -838,18 +958,36 @@ function registerDeletePinTool( "dispatch_delete_pin", { description: - "Permanently remove one current sidebar pin by its stable ID. Call dispatch_list_pins first and pass the exact returned id.", + "Permanently remove sidebar pins. Pass exactly one of: 'id' for a single pin, 'ids' for several at once, or 'group' to clear an entire group. Call dispatch_list_pins first and pass exact returned ids.", inputSchema: { id: z .string() .min(1) + .optional() .describe("Exact pin id returned by dispatch_list_pins."), + ids: z + .array(z.string().min(1)) + .min(1) + .optional() + .describe( + "Several exact pin ids, removed together. Every id must exist." + ), + group: pinFields.scopingGroup + .optional() + .describe("Remove every pin filed under this group heading."), }, }, async (args) => { try { - await deletePin(agentId, args.id); - return { content: [{ type: "text", text: `Removed pin ${args.id}.` }] }; + await deletePin(agentId, { + ...(args.id !== undefined ? { id: args.id } : {}), + ...(args.ids !== undefined ? { ids: args.ids } : {}), + ...(args.group !== undefined ? { group: args.group } : {}), + }); + const removed = args.group + ? `group "${args.group}"` + : (args.ids ?? [args.id]).join(", "); + return { content: [{ type: "text", text: `Removed ${removed}.` }] }; } catch (error) { return toToolError(error); } diff --git a/apps/server/test/mcp-handlers.test.ts b/apps/server/test/mcp-handlers.test.ts index fbaafe9f..f63eb909 100644 --- a/apps/server/test/mcp-handlers.test.ts +++ b/apps/server/test/mcp-handlers.test.ts @@ -225,11 +225,31 @@ function createMockDeps() { pin: { id: "pin_url", ...pin }, created: true, })), + upsertPins: vi.fn( + async (id: string, specs: Array>) => ({ + agent: { + id, + name: "test-agent", + pins: specs.map((pin, index) => ({ id: `pin_${index}`, ...pin })), + }, + pins: specs.map((pin, index) => ({ id: `pin_${index}`, ...pin })), + }) + ), deletePinById: vi.fn(async (id: string) => ({ id, name: "test-agent", pins: [], })), + deletePinsByIds: vi.fn(async (id: string) => ({ + id, + name: "test-agent", + pins: [], + })), + deletePinsByGroup: vi.fn(async (id: string) => ({ + id, + name: "test-agent", + pins: [], + })), listMedia: vi.fn(async () => []), }, jobService: { @@ -457,15 +477,95 @@ describe("createMcpHandlers", () => { describe("deletePin", () => { it("deletes pin and publishes event", async () => { - await handlers.deletePin("agt_test1", "pin_123"); - expect(deps.agentManager.deletePinById).toHaveBeenCalledWith( + await handlers.deletePin("agt_test1", { id: "pin_123" }); + expect(deps.agentManager.deletePinsByIds).toHaveBeenCalledWith( "agt_test1", - "pin_123" + ["pin_123"] ); expect(deps.publishUiEvent).toHaveBeenCalledWith( expect.objectContaining({ type: "agent.upsert" }) ); }); + + it("deletes several pins in one call", async () => { + await handlers.deletePin("agt_test1", { ids: ["pin_1", "pin_2"] }); + expect(deps.agentManager.deletePinsByIds).toHaveBeenCalledWith( + "agt_test1", + ["pin_1", "pin_2"] + ); + }); + + it("clears a group", async () => { + await handlers.deletePin("agt_test1", { group: "Ready to build" }); + expect(deps.agentManager.deletePinsByGroup).toHaveBeenCalledWith( + "agt_test1", + "Ready to build" + ); + }); + + it("rejects an ambiguous target", async () => { + // Accepting both would leave it unclear which one actually applied. + await expect( + handlers.deletePin("agt_test1", { id: "pin_1", group: "Group" }) + ).rejects.toThrow(/exactly one/i); + await expect(handlers.deletePin("agt_test1", {})).rejects.toThrow( + /exactly one/i + ); + }); + }); + + describe("upsertPins", () => { + it("writes a batch through one manager call", async () => { + await handlers.upsertPins("agt_test1", { + pins: [ + { label: "One", value: "1", type: "string" }, + { label: "Two", value: "2", type: "string" }, + ], + }); + expect(deps.agentManager.upsertPins).toHaveBeenCalledWith( + "agt_test1", + [ + { label: "One", value: "1", type: "string" }, + { label: "Two", value: "2", type: "string" }, + ], + {} + ); + // One event for the whole batch, not one per pin. + expect(deps.publishUiEvent).toHaveBeenCalledTimes(1); + }); + + it("validates every entry before writing any", async () => { + vi.mocked(validatePinValue).mockImplementation((type, value) => { + if (value === "bad") throw new Error("Invalid pin value"); + }); + await expect( + handlers.upsertPins("agt_test1", { + pins: [ + { label: "One", value: "1", type: "string" }, + { label: "Two", value: "bad", type: "string" }, + ], + }) + ).rejects.toThrow(/Invalid pin value/); + expect(deps.agentManager.upsertPins).not.toHaveBeenCalled(); + }); + + it("passes the scoping group through as an option, not per entry", async () => { + // Filing entries under the group is `replacePinGroup`'s own job — the + // handler compensating for it here is what let the primitive drift from + // its own contract. Covered end-to-end in pin-write.test.ts. + await handlers.upsertPins("agt_test1", { + mode: "replace", + group: "Ready to build", + pins: [ + { label: "One", value: "1", type: "string", group: "Elsewhere" }, + ], + }); + expect(deps.agentManager.upsertPins).toHaveBeenCalledWith( + "agt_test1", + [{ label: "One", value: "1", type: "string", group: "Elsewhere" }], + { mode: "replace", group: "Ready to build" } + ); + }); }); describe("listPins", () => { diff --git a/apps/server/test/pin-merge.test.ts b/apps/server/test/pin-merge.test.ts index 1b632f06..9ba97492 100644 --- a/apps/server/test/pin-merge.test.ts +++ b/apps/server/test/pin-merge.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from "vitest"; -import { clearBlankPinFields, mergePin } from "../src/agents/pin-merge.js"; +import { + clearBlankPinFields, + finalizePin, + mergePin, +} from "../src/agents/pin-merge.js"; import type { AgentPin } from "../src/agents/types.js"; const existing: AgentPin = { @@ -52,13 +56,17 @@ describe("mergePin", () => { }); it("clears a decoration when the agent sends an empty string", () => { - const merged = mergePin(existing, { - label: "What day is it?", - value: "What day is it today?", - type: "shortcut", - caption: "", - icon: " ", - }); + // Blank-clearing and the shortcut-field strip moved to `finalizePin`, so + // that every write path gets them — not just updates. + const merged = finalizePin( + mergePin(existing, { + label: "What day is it?", + value: "What day is it today?", + type: "shortcut", + caption: "", + icon: " ", + }) + ); expect(merged).not.toHaveProperty("caption"); expect(merged).not.toHaveProperty("icon"); @@ -68,9 +76,11 @@ describe("mergePin", () => { it("drops shortcut-only fields when the pin is re-typed", () => { // Omission means "keep", which would otherwise strand // icon/variant/confirm/disabled on a pin that can no longer use them. - const merged = mergePin( - { ...existing, confirm: true, disabled: true }, - { label: "What day is it?", value: "https://example.com", type: "url" } + const merged = finalizePin( + mergePin( + { ...existing, confirm: true, disabled: true }, + { label: "What day is it?", value: "https://example.com", type: "url" } + ) ); expect(merged).not.toHaveProperty("icon"); diff --git a/apps/server/test/pin-write.test.ts b/apps/server/test/pin-write.test.ts new file mode 100644 index 00000000..f5518782 --- /dev/null +++ b/apps/server/test/pin-write.test.ts @@ -0,0 +1,400 @@ +import { describe, expect, it } from "vitest"; + +import type { AgentPin } from "../src/agents/types.js"; +import { + applyPinSpec, + applyPinSpecs, + removePinGroup, + removePinsByIds, + replacePinGroup, +} from "../src/agents/pin-write.js"; + +function pin(over: Partial & { label: string }): AgentPin { + return { value: "v", type: "string", ...over }; +} + +// applyPinSpec accepts a spec whose type may be omitted; the helper above +// always sets one, so these tests pass raw objects where inheritance matters. + +const existing: AgentPin[] = [ + pin({ id: "pin_a", label: "Alpha", group: "Build" }), + pin({ id: "pin_b", label: "Bravo", group: "Build" }), + pin({ id: "pin_c", label: "Charlie" }), +]; + +describe("applyPinSpec", () => { + it("updates in place when the label already exists", () => { + const result = applyPinSpec(existing, pin({ label: "alpha", value: "2" })); + expect(result.created).toBe(false); + expect(result.pins.map((p) => p.id)).toEqual(["pin_a", "pin_b", "pin_c"]); + expect(result.pins[0]!.value).toBe("2"); + // The group it already had survives an update that did not mention it. + expect(result.pins[0]!.group).toBe("Build"); + }); + + it("appends an unknown label", () => { + const result = applyPinSpec(existing, pin({ label: "Delta" })); + expect(result.created).toBe(true); + expect(result.pins).toHaveLength(4); + expect(result.pins[3]!.id).toBeTruthy(); + }); + + it("renames when addressed by id, keeping position and decorations", () => { + // The whole point of id matching: the label is free to change because it + // is no longer what identifies the pin. + const result = applyPinSpec( + existing, + pin({ id: "pin_a", label: "Renamed" }) + ); + expect(result.created).toBe(false); + expect(result.pins[0]).toMatchObject({ + id: "pin_a", + label: "Renamed", + group: "Build", + }); + expect(result.pins).toHaveLength(3); + }); + + it("rejects an id that matches nothing rather than creating a stray pin", () => { + expect(() => + applyPinSpec(existing, pin({ id: "pin_zz", label: "Ghost" })) + ).toThrow(/not found/i); + }); + + it("rejects a rename onto another pin's label", () => { + // Case-insensitive label uniqueness is load-bearing for the sidebar. + expect(() => + applyPinSpec(existing, pin({ id: "pin_a", label: "bravo" })) + ).toThrow(/already uses the label/i); + }); + + it("allows a no-op relabel of the pin onto its own label", () => { + expect(() => + applyPinSpec(existing, pin({ id: "pin_a", label: "ALPHA" })) + ).not.toThrow(); + }); + + it("inherits the stored type when an update omits it", () => { + // Defaulting an omitted type to "string" would make a pure relabel demote + // a shortcut to a plain string and strip its icon — the exact operation + // this whole id-matching path exists to make cheap. + const shortcuts: AgentPin[] = [ + { + id: "pin_s", + label: "Run it", + value: "do the thing", + type: "shortcut", + icon: "zap", + variant: "primary", + }, + ]; + const result = applyPinSpec(shortcuts, { + id: "pin_s", + label: "Run it now", + value: "do the thing", + }); + expect(result.stored).toMatchObject({ + type: "shortcut", + icon: "zap", + variant: "primary", + label: "Run it now", + }); + }); + + it("defaults a brand new pin with no type to string", () => { + const result = applyPinSpec([], { label: "Fresh", value: "v" }); + expect(result.stored.type).toBe("string"); + }); + + it("inherits the stored value when an update omits it", () => { + // The point of id matching is a one-field patch; making a relabel restate + // the prompt it is not changing would undercut that. + const shortcuts: AgentPin[] = [ + { + id: "pin_s", + label: "Run it", + value: "do the thing", + type: "shortcut", + icon: "zap", + }, + ]; + const result = applyPinSpec(shortcuts, { + id: "pin_s", + label: "Run it now", + }); + expect(result.stored).toMatchObject({ + label: "Run it now", + value: "do the thing", + type: "shortcut", + icon: "zap", + }); + }); + + it("strips shortcut-only fields from a newly created non-shortcut pin", () => { + // The strip used to live only on the update branch, so a create could + // store `confirm`/`variant` on a plain pin — and neither is clearable by + // empty string, so the agent could never remove them afterwards. + const result = applyPinSpec([], { + label: "Plain", + value: "v", + icon: "zap", + variant: "destructive", + confirm: true, + disabled: true, + }); + expect(result.stored.type).toBe("string"); + expect(result.stored.icon).toBeUndefined(); + expect(result.stored.variant).toBeUndefined(); + expect(result.stored.confirm).toBeUndefined(); + expect(result.stored.disabled).toBeUndefined(); + }); + + it("keeps shortcut-only fields on a newly created shortcut pin", () => { + const result = applyPinSpec([], { + label: "Go", + value: "do it", + type: "shortcut", + icon: "zap", + confirm: true, + }); + expect(result.stored).toMatchObject({ icon: "zap", confirm: true }); + }); + + it("clears a pin's own group when sent an empty string", () => { + // A pin's own `group` is clearable by "" — distinct from the group *named + // as a target* by a bulk op, where blank must be rejected. Sharing one + // constraint between the two made clearing impossible. + const grouped: AgentPin[] = [ + { id: "pin_g", label: "Thing", value: "v", type: "string", group: "Old" }, + ]; + const result = applyPinSpec(grouped, { + id: "pin_g", + label: "Thing", + group: "", + }); + expect(result.stored.group).toBeUndefined(); + }); + + it("still requires a value when there is nothing to inherit from", () => { + expect(() => applyPinSpec([], { label: "Fresh" })).toThrow( + /value is required/i + ); + }); + + it("validates an inherited value against a newly given type", () => { + // Changing only the type has to re-check the value it kept, or a url pin + // could be retyped without its value ever being checked as a url. + const strings: AgentPin[] = [ + { id: "pin_x", label: "Thing", value: "not a url", type: "string" }, + ]; + expect(() => + applyPinSpec(strings, { id: "pin_x", label: "Thing", type: "url" }) + ).toThrow(); + }); + + it("still strips shortcut-only fields when a pin is retyped", () => { + const shortcuts: AgentPin[] = [ + { + id: "pin_s", + label: "Run it", + value: "do the thing", + type: "shortcut", + icon: "zap", + }, + ]; + const result = applyPinSpec(shortcuts, { + id: "pin_s", + label: "Run it", + value: "do the thing", + type: "string", + }); + expect(result.stored.icon).toBeUndefined(); + }); + + it("validates the value against the inherited type, not the request", () => { + const urls: AgentPin[] = [ + { id: "pin_u", label: "Docs", value: "https://x.dev", type: "url" }, + ]; + expect(() => + applyPinSpec(urls, { id: "pin_u", label: "Docs", value: "not a url" }) + ).toThrow(); + }); +}); + +describe("applyPinSpecs", () => { + it("applies a batch in order and reports each stored pin", () => { + const result = applyPinSpecs(existing, [ + pin({ id: "pin_a", label: "A2" }), + pin({ label: "Delta" }), + ]); + expect(result.pins.map((p) => p.label)).toEqual([ + "A2", + "Bravo", + "Charlie", + "Delta", + ]); + expect(result.stored).toHaveLength(2); + }); + + it("leaves pins the batch did not mention alone", () => { + const result = applyPinSpecs(existing, [pin({ label: "Delta" })]); + expect(result.pins.map((p) => p.id)).toContain("pin_c"); + }); +}); + +describe("replacePinGroup", () => { + it("makes the group exactly the given pins, in order", () => { + const result = replacePinGroup(existing, "Build", [ + pin({ id: "pin_b", label: "Bravo", group: "Build" }), + pin({ label: "Echo", group: "Build" }), + ]); + expect(result.pins.map((p) => p.label)).toEqual([ + "Bravo", + "Echo", + "Charlie", + ]); + }); + + it("drops group members the batch omitted", () => { + const result = replacePinGroup(existing, "Build", [ + pin({ id: "pin_b", label: "Bravo", group: "Build" }), + ]); + expect(result.pins.some((p) => p.id === "pin_a")).toBe(false); + }); + + it("never removes a pin outside the group", () => { + // The entire safety argument for replace mode: naming a group bounds the + // blast radius, so an ungrouped pin set hours ago cannot be collateral. + const result = replacePinGroup(existing, "Build", []); + expect(result.pins.map((p) => p.id)).toEqual(["pin_c"]); + }); + + it("anchors the rebuilt group where it already sat", () => { + const pins = [ + pin({ id: "pin_top", label: "Top" }), + pin({ id: "pin_g1", label: "G1", group: "Build" }), + pin({ id: "pin_end", label: "End" }), + ]; + const result = replacePinGroup(pins, "Build", [ + pin({ label: "Fresh", group: "Build" }), + ]); + expect(result.pins.map((p) => p.label)).toEqual(["Top", "Fresh", "End"]); + }); + + it("appends a group that does not exist yet", () => { + const result = replacePinGroup(existing, "New", [ + pin({ label: "Foxtrot", group: "New" }), + ]); + expect(result.pins.map((p) => p.label)).toEqual([ + "Alpha", + "Bravo", + "Charlie", + "Foxtrot", + ]); + }); + + it("moves a named pin into the group instead of duplicating it", () => { + const result = replacePinGroup(existing, "Build", [ + pin({ id: "pin_c", label: "Charlie", group: "Build" }), + ]); + expect(result.pins.map((p) => p.id)).toEqual(["pin_c"]); + expect(result.pins[0]!.group).toBe("Build"); + }); + + it("rejects two new entries that would share a label", () => { + // Each spec resolves against the original array, so two creates sharing a + // label both look unmatched — the uniqueness check has to run on the + // finished array, not per spec. + expect(() => + replacePinGroup(existing, "Build", [ + pin({ label: "Duplicate", group: "Build" }), + pin({ label: "duplicate", group: "Build" }), + ]) + ).toThrow(/share the label/i); + }); + + it("treats a label matching an outside pin as a move, not a duplicate", () => { + // Label matching resolves against the whole array, so naming an existing + // pin pulls it into the group rather than creating a second one with the + // same label — which is also why this can't break label uniqueness. + const result = replacePinGroup(existing, "Build", [ + pin({ label: "charlie", group: "Build" }), + ]); + expect(result.pins.map((p) => p.id)).toEqual(["pin_c"]); + expect(result.pins[0]!.group).toBe("Build"); + }); + + it("files members under the group without the caller pre-setting it", () => { + // The primitive owns group membership. Leaving it to a compensating map in + // the handler made this pass only because the caller remembered — and a + // member stored without `group` renders under no heading yet sits in the + // block, invisible to the next replace of that same group. + const result = replacePinGroup([], "Build", [ + { label: "a", value: "1" }, + { label: "b", value: "2" }, + ]); + expect(result.pins.map((p) => p.group)).toEqual(["Build", "Build"]); + }); + + it("overrides an entry's own group with the scoping group", () => { + const result = replacePinGroup([], "Build", [ + { label: "a", value: "1", group: "Elsewhere" }, + ]); + expect(result.pins[0]!.group).toBe("Build"); + }); + + it("refuses a blank group rather than treating it as 'ungrouped'", () => { + expect(() => replacePinGroup(existing, " ", [])).toThrow( + /group name is required/i + ); + }); + + it("rejects two entries addressing the same pin", () => { + expect(() => + replacePinGroup(existing, "Build", [ + pin({ id: "pin_a", label: "One", group: "Build" }), + pin({ id: "pin_a", label: "Two", group: "Build" }), + ]) + ).toThrow(/same pin/i); + }); +}); + +describe("removePinsByIds", () => { + it("removes every listed id", () => { + expect(removePinsByIds(existing, ["pin_a", "pin_c"])).toHaveLength(1); + }); + + it("rejects an unknown id rather than silently no-opping", () => { + expect(() => removePinsByIds(existing, ["pin_a", "nope"])).toThrow( + /not found/i + ); + }); +}); + +describe("removePinGroup", () => { + it("removes every member of the group", () => { + expect(removePinGroup(existing, "Build").map((p) => p.id)).toEqual([ + "pin_c", + ]); + }); + + it("matches the group name case-insensitively", () => { + expect(removePinGroup(existing, "build")).toHaveLength(1); + }); + + it("rejects an empty group", () => { + expect(() => removePinGroup(existing, "Nothing")).toThrow(/no pins/i); + }); + + it("refuses a blank group name instead of deleting every ungrouped pin", () => { + // sameGroup() treats a missing group as "", so a blank name would match + // every ungrouped pin and quietly turn this into a mass delete. + for (const blank of ["", " "]) { + expect(() => removePinGroup(existing, blank)).toThrow( + /group name is required/i + ); + } + // The ungrouped pin is still there. + expect(existing.some((p) => p.id === "pin_c")).toBe(true); + }); +}); diff --git a/apps/web/src/components/app/agent-history-detail-tabs.tsx b/apps/web/src/components/app/agent-history-detail-tabs.tsx index df4a7514..fe34c17d 100644 --- a/apps/web/src/components/app/agent-history-detail-tabs.tsx +++ b/apps/web/src/components/app/agent-history-detail-tabs.tsx @@ -147,7 +147,11 @@ export function DetailTabs({ {tab === "pins" && pins.length > 0 && (
- +
)} {tab === "pins" && pins.length === 0 && ( diff --git a/apps/web/src/components/app/media-sidebar.tsx b/apps/web/src/components/app/media-sidebar.tsx index 4cbda1b5..a70de314 100644 --- a/apps/web/src/components/app/media-sidebar.tsx +++ b/apps/web/src/components/app/media-sidebar.tsx @@ -239,6 +239,7 @@ export function MediaSidebarContent({ selectedAgentName={selectedAgentName} selectedAgentWorkspaceRoot={selectedAgentWorkspaceRoot} agentIsRunning={selectedAgentIsRunning} + collapseScope={selectedAgentId} // A shortcut fires a real prompt into a live session, so an // in-flight run blocks its own button until it settles — a // double-click would otherwise send the prompt twice. diff --git a/apps/web/src/components/app/pins-panel.test.tsx b/apps/web/src/components/app/pins-panel.test.tsx index 74bc0a88..9b4882a8 100644 --- a/apps/web/src/components/app/pins-panel.test.tsx +++ b/apps/web/src/components/app/pins-panel.test.tsx @@ -22,6 +22,18 @@ const shortcutPin: AgentPin = { caption: "High priority", }; +/** N shortcut pins sharing one group. Group names are unique per test so the + * persisted collapse atoms cannot leak between them. */ +function groupOf(group: string, count: number): AgentPin[] { + return Array.from({ length: count }, (_, index) => ({ + id: `${group}_${index}`, + label: `${group} pin ${index}`, + value: `prompt ${index}`, + type: "shortcut" as const, + group, + })); +} + function renderPanel( pins: AgentPin[], props: Partial[0]> = {} @@ -171,7 +183,8 @@ describe("shortcut pins", () => { const groups = screen.getAllByTestId("pin-group"); expect(groups).toHaveLength(1); expect(groups[0]!.getAttribute("data-pin-group")).toBe("Ready to build"); - expect(within(groups[0]!).getAllByRole("button")).toHaveLength(2); + // Two shortcuts plus the heading's own collapse toggle. + expect(within(groups[0]!).getAllByRole("button")).toHaveLength(3); }); it("marks the shortcut unavailable when no run handler is wired (e.g. agent history)", () => { @@ -220,6 +233,125 @@ describe("shortcut pins", () => { ); }); + it("shows the member count on the group heading", () => { + renderPanel(groupOf("Counted", 3), { onRunShortcut: vi.fn() }); + + expect(screen.getByTestId("pin-group-count").textContent).toBe("3"); + }); + + it("leaves a small group expanded", () => { + renderPanel(groupOf("Small", 3), { onRunShortcut: vi.fn() }); + + const group = screen.getByTestId("pin-group"); + expect(group.getAttribute("data-pin-group-collapsed")).toBe("false"); + expect(within(group).getAllByTestId("pin-item")).toHaveLength(3); + }); + + it("starts a large group collapsed, hiding its members but not its count", () => { + // A long group otherwise pushes every other group off screen. + renderPanel(groupOf("Large", 12), { onRunShortcut: vi.fn() }); + + const group = screen.getByTestId("pin-group"); + expect(group.getAttribute("data-pin-group-collapsed")).toBe("true"); + expect(within(group).queryAllByTestId("pin-item")).toHaveLength(0); + expect(screen.getByTestId("pin-group-count").textContent).toBe("12"); + }); + + it("toggles a group in both directions", () => { + renderPanel(groupOf("Toggled", 2), { onRunShortcut: vi.fn() }); + + const toggle = screen.getByTestId("pin-group-toggle"); + expect(toggle.getAttribute("aria-expanded")).toBe("true"); + + fireEvent.click(toggle); + expect( + screen.getByTestId("pin-group-toggle").getAttribute("aria-expanded") + ).toBe("false"); + expect(screen.queryAllByTestId("pin-item")).toHaveLength(0); + + fireEvent.click(screen.getByTestId("pin-group-toggle")); + expect( + screen.getByTestId("pin-group-toggle").getAttribute("aria-expanded") + ).toBe("true"); + expect(screen.getAllByTestId("pin-item")).toHaveLength(2); + }); + + it("keeps an expanded large group expanded when it gains a pin", () => { + // The size default must not override a choice the user already made, or + // expanding a big group would silently undo itself on the next pin. + const { unmount } = renderPanel(groupOf("Sticky", 12), { + onRunShortcut: vi.fn(), + collapseScope: "agt_sticky", + }); + fireEvent.click(screen.getByTestId("pin-group-toggle")); + expect(screen.getAllByTestId("pin-item")).toHaveLength(12); + unmount(); + + renderPanel(groupOf("Sticky", 13), { + onRunShortcut: vi.fn(), + collapseScope: "agt_sticky", + }); + expect( + screen.getByTestId("pin-group").getAttribute("data-pin-group-collapsed") + ).toBe("false"); + }); + + it("ties the toggle to the member region and names the group by its text", () => { + renderPanel(groupOf("Wired", 2), { onRunShortcut: vi.fn() }); + + const toggle = screen.getByTestId("pin-group-toggle"); + const region = screen.getByTestId("pin-group-members"); + // aria-controls must resolve, so the region stays mounted while collapsed. + expect(toggle.getAttribute("aria-controls")).toBe(region.id); + // The group's accessible name is the heading text, not the whole button + // (which also reads out the count and the collapse action). + const group = screen.getByTestId("pin-group"); + const labelId = group.getAttribute("aria-labelledby")!; + expect(document.getElementById(labelId)!.textContent).toBe("Wired"); + }); + + it("keeps the member region mounted and marked hidden when collapsed", () => { + renderPanel(groupOf("HiddenRegion", 12), { onRunShortcut: vi.fn() }); + + const region = screen.getByTestId("pin-group-members"); + expect(region.hasAttribute("hidden")).toBe(true); + expect(within(region).queryAllByTestId("pin-item")).toHaveLength(0); + }); + + it("keeps collapse ephemeral when no scope is given", () => { + // Without a scope there is nothing to namespace by; persisting under a + // shared fallback key would leak one list's state onto every other. + const { unmount } = renderPanel(groupOf("Unscoped", 2)); + fireEvent.click(screen.getByTestId("pin-group-toggle")); + expect( + screen.getByTestId("pin-group").getAttribute("data-pin-group-collapsed") + ).toBe("true"); + expect( + Object.keys(localStorage).filter( + (k) => k.includes("__unscoped__") || k.includes("unscoped::") + ) + ).toHaveLength(0); + unmount(); + + renderPanel(groupOf("Unscoped", 2)); + expect( + screen.getByTestId("pin-group").getAttribute("data-pin-group-collapsed") + ).toBe("false"); + }); + + it("does not share collapse state between two scopes", () => { + const { unmount } = renderPanel(groupOf("Shared", 2), { + collapseScope: "agt_one", + }); + fireEvent.click(screen.getByTestId("pin-group-toggle")); + unmount(); + + renderPanel(groupOf("Shared", 2), { collapseScope: "agt_two" }); + expect( + screen.getByTestId("pin-group").getAttribute("data-pin-group-collapsed") + ).toBe("false"); + }); + it("blocks a second send while the first is still in flight", () => { const onRunShortcut = vi.fn(); renderPanel([shortcutPin], { onRunShortcut, pendingPinId: "pin_1" }); diff --git a/apps/web/src/components/app/pins-panel.tsx b/apps/web/src/components/app/pins-panel.tsx index b16af97b..38569bfa 100644 --- a/apps/web/src/components/app/pins-panel.tsx +++ b/apps/web/src/components/app/pins-panel.tsx @@ -2,6 +2,7 @@ import { AlertTriangle, Ban, Check, + ChevronRight, CornerDownLeft, Copy, FileText, @@ -9,7 +10,8 @@ import { Loader2, Pin, } from "lucide-react"; -import { useRef, useState } from "react"; +import { useAtom } from "jotai"; +import { useId, useRef, useState } from "react"; import { FrontTruncatedValue } from "@/components/app/agent-meta"; import { type AgentPin } from "@/components/app/types"; @@ -27,6 +29,7 @@ import { Markdown } from "@/components/ui/markdown"; import { useCoarsePointer } from "@/hooks/use-coarse-pointer"; import { useCopyText } from "@/hooks/use-copy"; import { splitPinValues } from "@/lib/pins"; +import { pinGroupCollapsedAtomFamily } from "@/lib/store"; import { rewritePinUrl } from "@/lib/rewrite-pin-url"; import { ScrollArea } from "@/components/ui/scroll-area"; import { @@ -590,6 +593,167 @@ function layoutPins(pins: AgentPin[]): PinRow[] { return rows; } +/** + * Groups past this size start collapsed. A sidebar full of one agent's pins + * pushes every other group off screen, and a long group is exactly the case + * where the heading and count are more useful than the members. + */ +const AUTO_COLLAPSE_THRESHOLD = 8; + +type PinGroupProps = { + name: string; + pins: AgentPin[]; + collapseScope: string | null; + workspaceRoot: string | null; + agentIsRunning?: boolean; + onRunShortcut?: (pin: AgentPin, pointerType?: string) => void; + agentName?: string | null; + pendingPinId?: string | null; + buttonRef?: (pin: AgentPin, element: HTMLButtonElement | null) => void; +}; + +type PinGroupViewProps = Omit & { + collapsed: boolean; + onToggle: () => void; +}; + +function PinGroupView({ + name, + pins, + collapsed, + onToggle, + workspaceRoot, + agentIsRunning, + onRunShortcut, + agentName = null, + pendingPinId = null, + buttonRef, +}: PinGroupViewProps): JSX.Element { + // Ids must be unique per document, not per list: the desktop and mobile + // sidebars are both always mounted, so a name-derived id would appear twice + // and `aria-controls` would resolve to the other instance's region. + const uid = useId(); + const headingId = `pin-group-${uid}`; + const regionId = `pin-group-members-${uid}`; + + return ( +
+ + {/* The region stays in the tree so `aria-controls` always resolves and + `hidden` carries the state; its members unmount while collapsed. */} + +
+ ); +} + +/** An explicit choice always beats the size-based default. */ +function resolveCollapsed(choice: boolean | null, count: number): boolean { + return choice ?? count > AUTO_COLLAPSE_THRESHOLD; +} + +function PersistedPinGroup( + props: PinGroupProps & { collapseScope: string } +): JSX.Element { + const [choice, setChoice] = useAtom( + pinGroupCollapsedAtomFamily( + `${props.collapseScope}::${props.name.toLowerCase()}` + ) + ); + const collapsed = resolveCollapsed(choice, props.pins.length); + return ( + setChoice(!collapsed)} + /> + ); +} + +function EphemeralPinGroup(props: PinGroupProps): JSX.Element { + const [choice, setChoice] = useState(null); + const collapsed = resolveCollapsed(choice, props.pins.length); + return ( + setChoice(!collapsed)} + /> + ); +} + +/** + * Branch above the hooks rather than calling both: with no scope there is + * nothing to namespace by, and persisting to a shared fallback key would leak + * one list's collapse choices onto every other unscoped list. `collapseScope` + * is stable per mount site, so this never swaps a component mid-life. + */ +function PinGroup(props: PinGroupProps): JSX.Element { + return props.collapseScope === null ? ( + + ) : ( + + ); +} + /** * The rendering unit for a set of pins: grouping policy and `PinItem` travel * together, so every consumer gets group headings without re-implementing the @@ -604,6 +768,7 @@ export function PinList({ agentName = null, pendingPinId = null, buttonRef, + collapseScope = null, }: { pins: AgentPin[]; workspaceRoot: string | null; @@ -612,13 +777,19 @@ export function PinList({ agentName?: string | null; pendingPinId?: string | null; buttonRef?: (pin: AgentPin, element: HTMLButtonElement | null) => void; + /** + * Namespaces persisted collapse state — an agent id, so it survives a + * session rename. `null` means don't persist at all: a shared fallback + * bucket would leak one list's collapse choices onto every other list. + */ + collapseScope?: string | null; }): JSX.Element { return ( <> {layoutPins(pins).map((row) => row.kind === "pin" ? ( ) : ( -
-
- {row.name} -
-
- {row.pins.map((pin) => ( - - ))} -
-
+ name={row.name} + pins={row.pins} + collapseScope={collapseScope} + workspaceRoot={workspaceRoot} + agentIsRunning={agentIsRunning} + onRunShortcut={onRunShortcut} + agentName={agentName} + pendingPinId={pendingPinId} + buttonRef={buttonRef} + /> ) )} @@ -673,6 +824,8 @@ type PinsPanelProps = { agentIsRunning?: boolean; onRunShortcut?: (pin: AgentPin, pointerType?: string) => void; pendingPinId?: string | null; + /** Agent id, so persisted group collapse survives a session rename. Omit to keep collapse ephemeral. */ + collapseScope?: string | null; }; /** @@ -738,6 +891,7 @@ export function PinsPanel({ agentIsRunning, onRunShortcut, pendingPinId = null, + collapseScope, }: PinsPanelProps): JSX.Element { const [pendingShortcutPin, setPendingShortcutPin] = useState( null @@ -801,6 +955,7 @@ export function PinsPanel({ agentName={selectedAgentName} pendingPinId={pendingPinId} buttonRef={registerShortcutButton} + collapseScope={collapseScope ?? null} /> lastTrigger.current?.focus()} diff --git a/apps/web/src/lib/store.ts b/apps/web/src/lib/store.ts index 85094eba..c3a65e41 100644 --- a/apps/web/src/lib/store.ts +++ b/apps/web/src/lib/store.ts @@ -116,6 +116,21 @@ export const whiteboardAgentDrewAtomFamily = atomFamily((_agentId: string) => atom(false) ); +/** + * Whether one pin group is collapsed, keyed by `::`. + * + * Stores the user's *choice*, not the rendered state: `null` means they have + * never touched this group, which is distinct from having chosen "expanded". + * The size-based default is applied at render, so a group the user expanded + * stays expanded when it later grows past the auto-collapse threshold. + */ +export const pinGroupCollapsedAtomFamily = atomFamily((key: string) => + atomWithLocalStorage( + `dispatch:pinGroupCollapsed:${key}`, + null + ) +); + export type DiffViewType = "unified" | "split"; export const diffViewTypeAtom = atomWithLocalStorage( diff --git a/e2e/media-sidebar.spec.ts b/e2e/media-sidebar.spec.ts index 43bb2c05..69fabb96 100644 --- a/e2e/media-sidebar.spec.ts +++ b/e2e/media-sidebar.spec.ts @@ -627,4 +627,50 @@ test.describe("Media sidebar", () => { .click(); expect((await runResponse).request().method()).toBe("POST"); }); + + test("remembers a collapsed pin group across a reload", async ({ + page, + request, + }) => { + const agent = await createAgentViaAPI(request, { + name: `e2e-agent-pin-groups-${Date.now()}`, + cwd: process.cwd(), + }); + await setAgentPinsViaDB( + agent.id, + Array.from({ length: 3 }, (_, index) => ({ + id: `pin_group_${index}`, + label: `Grouped pin ${index}`, + type: "shortcut" as const, + value: `prompt ${index}`, + group: "Ready to build", + })) + ); + + await loadApp(page); + await openMediaSidebarForAgent(page, agent); + const group = page.getByTestId("pin-group"); + + // The default and the toggle are unit-tested; only a real reload can show + // that the choice actually round-trips through localStorage. + await expect(group).toHaveAttribute("data-pin-group-collapsed", "false"); + await expect(page.getByTestId("pin-group-count")).toHaveText("3"); + await page.getByTestId("pin-group-toggle").click(); + await expect(group).toHaveAttribute("data-pin-group-collapsed", "true"); + + // Collapsed hides the members but must keep the group findable. + await expect(page.getByTestId("pin-item")).toHaveCount(0); + await expect(page.getByTestId("pin-group-count")).toHaveText("3"); + + // What only a reload can prove is that the choice round-trips through + // localStorage rather than living in component state. Reopening the panel + // afterwards is its own can of worms (two sidebar instances, remembered + // tab), so assert the stored value directly — how a collapsed group + // renders is already covered by the pins-panel unit tests. + const key = `dispatch:pinGroupCollapsed:${agent.id}::ready to build`; + await page.reload({ waitUntil: "domcontentloaded" }); + await expect + .poll(() => page.evaluate((k) => localStorage.getItem(k), key)) + .toBe("true"); + }); });