diff --git a/packages/core/api/client.ts b/packages/core/api/client.ts index 46e8dea25..d541d5967 100644 --- a/packages/core/api/client.ts +++ b/packages/core/api/client.ts @@ -116,6 +116,7 @@ import { EMPTY_LIST_ISSUES_RESPONSE, EMPTY_TIMELINE_ENTRIES, ListIssuesResponseSchema, + SquadSchema, SubscribersListSchema, TimelineEntriesSchema, } from "./schemas"; @@ -1456,7 +1457,10 @@ export class ApiClient { } async getSquad(id: string): Promise { - return this.fetch(`/api/squads/${id}`); + const raw = await this.fetch(`/api/squads/${id}`); + return parseWithFallback(raw, SquadSchema, raw as Squad, { + endpoint: `GET /api/squads/${id}`, + }); } async createSquad(data: { name: string; description?: string; leader_id: string; avatar_url?: string }): Promise { diff --git a/packages/core/api/schema.test.ts b/packages/core/api/schema.test.ts index 67c63c0b8..16a64260c 100644 --- a/packages/core/api/schema.test.ts +++ b/packages/core/api/schema.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { z } from "zod"; import { ApiClient } from "./client"; import { parseWithFallback } from "./schema"; +import { SquadSchema } from "./schemas"; // Helper: stub fetch with a single JSON response. Status defaults to 200. function stubFetchJson(body: unknown, status = 200) { @@ -221,6 +222,62 @@ describe("ApiClient schema fallback", () => { }); }); +describe("SquadSchema", () => { + type SquadShape = Record & { issue_count?: number | null }; + const baseSquad: SquadShape = { + id: "x", + workspace_id: "y", + name: "n", + description: "", + instructions: "", + avatar_url: null, + leader_id: "l", + creator_id: "c", + created_at: "t", + updated_at: "t", + archived_at: null, + archived_by: null, + }; + const fallback: SquadShape = { ...baseSquad }; + const opts = { endpoint: "test SquadSchema" }; + + it("accepts a response missing issue_count (older server / list endpoint)", () => { + const result = parseWithFallback(baseSquad, SquadSchema, fallback, opts); + // Field is optional — undefined or null are both "unknown". + expect(result.issue_count ?? null).toBeNull(); + }); + + it("parses a response with issue_count: number", () => { + const result = parseWithFallback( + { ...baseSquad, issue_count: 3 }, + SquadSchema, + fallback, + opts, + ); + expect(result.issue_count).toBe(3); + }); + + it("parses issue_count: null (server-side count error path)", () => { + const result = parseWithFallback( + { ...baseSquad, issue_count: null }, + SquadSchema, + fallback, + opts, + ); + expect(result.issue_count).toBeNull(); + }); + + it("preserves unknown fields via .loose()", () => { + const result = parseWithFallback( + { ...baseSquad, future_field: "x" }, + SquadSchema, + fallback, + opts, + ); + expect(result.future_field).toBe("x"); + }); +}); + // Direct tests for the helper, decoupled from any specific endpoint — // guards against an endpoint refactor masking a regression in the helper. describe("parseWithFallback", () => { diff --git a/packages/core/api/schemas.ts b/packages/core/api/schemas.ts index b6b9ff760..a4bfea6de 100644 --- a/packages/core/api/schemas.ts +++ b/packages/core/api/schemas.ts @@ -306,3 +306,32 @@ export const EMPTY_CREATE_AGENT_FROM_TEMPLATE_RESPONSE: CreateAgentFromTemplateR imported_skill_ids: [], reused_skill_ids: [], }; + +// --------------------------------------------------------------------------- +// Squad detail — `GET /api/squads/{id}` carries an `issue_count` field that +// the archive confirmation dialog uses to render "{leader} will take over N +// issues". Three wire shapes must all be acceptable: +// 1. Older server: field absent entirely. +// 2. Current server, count query errored: field present, value `null`. +// 3. Current server, happy path: field present, value `number`. +// All three collapse to a single "no count → fall back to count-less copy" +// branch on the consumer side, so this schema stays lenient and lets every +// shape through. List/create/update responses don't carry the field by design +// — the converter on the server side is shared and only the GetSquad handler +// populates it, so wrapping those endpoints would lie about the wire shape. +// --------------------------------------------------------------------------- +export const SquadSchema = z.object({ + id: z.string(), + workspace_id: z.string(), + name: z.string().catch(""), + description: z.string().catch(""), + instructions: z.string().catch(""), + avatar_url: z.string().nullable().catch(null), + leader_id: z.string(), + creator_id: z.string(), + created_at: z.string(), + updated_at: z.string(), + archived_at: z.string().nullable().catch(null), + archived_by: z.string().nullable().catch(null), + issue_count: z.number().nullable().optional().catch(null), +}).loose(); diff --git a/packages/core/types/squad.ts b/packages/core/types/squad.ts index 63ed4b43a..a742e0f1f 100644 --- a/packages/core/types/squad.ts +++ b/packages/core/types/squad.ts @@ -15,6 +15,13 @@ export interface Squad { updated_at: string; archived_at: string | null; archived_by: string | null; + /** + * Total issues currently assigned to this squad (all statuses, see backend + * CountIssuesForSquad). Only populated by `GET /api/squads/{id}`; absent + * (`undefined`) on list/create/update responses and on older servers that + * predate the field. Treat `undefined` and `null` identically as "unknown". + */ + issue_count?: number | null; } export interface SquadMember { diff --git a/packages/views/locales/en/squads.json b/packages/views/locales/en/squads.json index 2c215e1dd..92771983c 100644 --- a/packages/views/locales/en/squads.json +++ b/packages/views/locales/en/squads.json @@ -47,5 +47,19 @@ "description": "Squad instructions are injected into the leader agent's prompt whenever it works on an issue assigned to this squad. Use them to give the leader squad-wide guidance, working agreements, or context the leader should follow on every task.", "unsaved_changes": "Unsaved changes", "save_button": "Save" + }, + "archive_dialog": { + "title": "Archive squad \"{{name}}\"?", + "description_with_count_one": "{{leader}} will take over {{count}} issue currently assigned to this squad (including closed ones). The squad will no longer appear in the workspace list, and this cannot be undone.", + "description_with_count_other": "{{leader}} will take over all {{count}} issues currently assigned to this squad (including closed ones). The squad will no longer appear in the workspace list, and this cannot be undone.", + "description_no_count": "{{leader}} will take over all issues currently assigned to this squad (including closed ones). The squad will no longer appear in the workspace list, and this cannot be undone.", + "cancel": "Cancel", + "confirm": "Archive", + "archiving": "Archiving…" + }, + "role_editor": { + "add_role": "+ Add role", + "search_placeholder": "Type or pick a role…", + "no_suggestions": "No existing roles. Press Enter to add." } } diff --git a/packages/views/locales/zh-Hans/squads.json b/packages/views/locales/zh-Hans/squads.json index f98ee0d4c..92172a583 100644 --- a/packages/views/locales/zh-Hans/squads.json +++ b/packages/views/locales/zh-Hans/squads.json @@ -47,5 +47,19 @@ "description": "小队指引会在 Leader 智能体处理分配给该小队的 issue 时注入到它的 prompt 中。可用来给 Leader 提供贯穿全队的指导、协作规范，或每次任务都应遵循的上下文。", "unsaved_changes": "有未保存的修改", "save_button": "保存" + }, + "archive_dialog": { + "title": "归档小队「{{name}}」？", + "description_with_count_one": "{{leader}} 将接管该小队当前 {{count}} 个 issue（含已关闭）。归档后小队不再出现在工作区列表中，该操作不可撤销。", + "description_with_count_other": "{{leader}} 将接管该小队当前全部 {{count}} 个 issue（含已关闭）。归档后小队不再出现在工作区列表中，该操作不可撤销。", + "description_no_count": "{{leader}} 将接管该小队当前的全部 issue（含已关闭）。归档后小队不再出现在工作区列表中，该操作不可撤销。", + "cancel": "取消", + "confirm": "归档", + "archiving": "归档中…" + }, + "role_editor": { + "add_role": "+ 添加角色", + "search_placeholder": "输入或选择角色…", + "no_suggestions": "尚无已用角色，按 Enter 添加" } } diff --git a/packages/views/squads/components/archive-squad-confirm-dialog.test.tsx b/packages/views/squads/components/archive-squad-confirm-dialog.test.tsx new file mode 100644 index 000000000..f3ad5a370 --- /dev/null +++ b/packages/views/squads/components/archive-squad-confirm-dialog.test.tsx @@ -0,0 +1,145 @@ +import type { ReactNode } from "react"; +import { describe, expect, it, vi } from "vitest"; +import { render as rtlRender, screen, type RenderOptions } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { I18nProvider } from "@multica/core/i18n/react"; +import enCommon from "../../locales/en/common.json"; +import enSquads from "../../locales/en/squads.json"; + +// AlertDialog (Base UI) renders into a portal that is awkward to target with +// RTL — strip it down to passthrough wrappers so we can assert on the body +// content directly. The component logic under test (count fallback, pending +// state, button wiring) lives in the dialog body, not in Base UI. +vi.mock("@multica/ui/components/ui/alert-dialog", () => ({ + AlertDialog: ({ children, open }: { children: ReactNode; open: boolean }) => + open ?
{children}
: null, + AlertDialogContent: ({ children }: { children: ReactNode }) =>
{children}
, + AlertDialogHeader: ({ children }: { children: ReactNode }) =>
{children}
, + AlertDialogTitle: ({ children }: { children: ReactNode }) =>

{children}

, + AlertDialogDescription: ({ children }: { children: ReactNode }) =>

{children}

, + AlertDialogFooter: ({ children }: { children: ReactNode }) =>
{children}
, + AlertDialogCancel: ({ + children, + disabled, + }: { + children: ReactNode; + disabled?: boolean; + }) => ( + + ), + AlertDialogAction: ({ + children, + disabled, + onClick, + }: { + children: ReactNode; + disabled?: boolean; + variant?: string; + onClick?: () => void; + }) => ( + + ), +})); + +const TEST_RESOURCES = { + en: { common: enCommon, squads: enSquads }, +}; + +function I18nWrapper({ children }: { children: ReactNode }) { + return ( + + {children} + + ); +} + +function render(ui: React.ReactElement, options?: RenderOptions) { + return rtlRender(ui, { wrapper: I18nWrapper, ...options }); +} + +import { ArchiveSquadConfirmDialog } from "./archive-squad-confirm-dialog"; + +describe("ArchiveSquadConfirmDialog", () => { + it("shows leader name and count when count is provided", () => { + render( + {}} + onConfirm={async () => {}} + pending={false} + />, + ); + expect(screen.getByText(/Squirtle-Leader/)).toBeInTheDocument(); + expect(screen.getByText(/3 issues/)).toBeInTheDocument(); + }); + + it("falls back to no-count copy when issueCount is null", () => { + render( + {}} + onConfirm={async () => {}} + pending={false} + />, + ); + expect(screen.getByText(/all issues currently assigned/i)).toBeInTheDocument(); + }); + + it("disables Cancel and shows the archiving label while pending", () => { + render( + {}} + onConfirm={async () => {}} + pending + />, + ); + expect(screen.getByRole("button", { name: /archiving/i })).toBeDisabled(); + expect(screen.getByRole("button", { name: /cancel/i })).toBeDisabled(); + }); + + it("calls onConfirm when the Archive button is clicked", async () => { + const onConfirm = vi.fn().mockResolvedValue(undefined); + render( + {}} + onConfirm={onConfirm} + pending={false} + />, + ); + await userEvent.click(screen.getByRole("button", { name: /^archive$/i })); + expect(onConfirm).toHaveBeenCalledTimes(1); + }); + + it("uses the singular copy variant when count is 1", () => { + render( + {}} + onConfirm={async () => {}} + pending={false} + />, + ); + expect(screen.getByText(/take over 1 issue\b/)).toBeInTheDocument(); + }); +}); diff --git a/packages/views/squads/components/archive-squad-confirm-dialog.tsx b/packages/views/squads/components/archive-squad-confirm-dialog.tsx new file mode 100644 index 000000000..bb8eebbc7 --- /dev/null +++ b/packages/views/squads/components/archive-squad-confirm-dialog.tsx @@ -0,0 +1,87 @@ +"use client"; + +import { Loader2 } from "lucide-react"; +import { + AlertDialog, + AlertDialogAction, + AlertDialogCancel, + AlertDialogContent, + AlertDialogDescription, + AlertDialogFooter, + AlertDialogHeader, + AlertDialogTitle, +} from "@multica/ui/components/ui/alert-dialog"; +import { useT } from "../../i18n"; + +export interface ArchiveSquadConfirmDialogProps { + open: boolean; + squadName: string; + leaderName: string; + /** + * Total issues currently assigned to the squad. Pass `null` (older server + * or transient count error) to render the "no count" copy variant — the + * action is still safe, the user just sees a less specific number-free + * description. + */ + issueCount: number | null; + pending: boolean; + onCancel: () => void; + onConfirm: () => Promise | void; +} + +export function ArchiveSquadConfirmDialog({ + open, + squadName, + leaderName, + issueCount, + pending, + onCancel, + onConfirm, +}: ArchiveSquadConfirmDialogProps) { + const { t } = useT("squads"); + + const description = + issueCount == null + ? t(($) => $.archive_dialog.description_no_count, { leader: leaderName }) + : t(($) => $.archive_dialog.description_with_count, { + leader: leaderName, + count: issueCount, + }); + + return ( + { + if (!v && !pending) onCancel(); + }} + > + + + + {t(($) => $.archive_dialog.title, { name: squadName })} + + {description} + + + + {t(($) => $.archive_dialog.cancel)} + + void onConfirm()} + > + {pending ? ( + <> + + {t(($) => $.archive_dialog.archiving)} + + ) : ( + t(($) => $.archive_dialog.confirm) + )} + + + + + ); +} diff --git a/packages/views/squads/components/role-editor.test.tsx b/packages/views/squads/components/role-editor.test.tsx new file mode 100644 index 000000000..f69f25787 --- /dev/null +++ b/packages/views/squads/components/role-editor.test.tsx @@ -0,0 +1,207 @@ +import * as React from "react"; +import type { ReactNode } from "react"; +import { Children, cloneElement, isValidElement, useState } from "react"; +import { describe, expect, it, vi } from "vitest"; +import { render as rtlRender, screen, type RenderOptions } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { I18nProvider } from "@multica/core/i18n/react"; +import enCommon from "../../locales/en/common.json"; +import enSquads from "../../locales/en/squads.json"; + +// Popover (Base UI) renders into a portal that is awkward to target with +// RTL. Replace with a simple state-driven open/close shim that surfaces the +// popover content inline whenever `open` is true (or after the trigger is +// clicked). The component logic under test (commit on Enter, commit on +// suggestion click, no commit on blur, Pencil/Loader2 swap) is what we need +// to exercise — Base UI's portal/positioning isn't. +vi.mock("@multica/ui/components/ui/popover", () => { + function Popover({ + open, + onOpenChange, + children, + }: { + open?: boolean; + onOpenChange?: (v: boolean) => void; + children: ReactNode; + }) { + const [innerOpen, setInnerOpen] = useState(false); + const isOpen = open ?? innerOpen; + const setOpen = (v: boolean) => { + setInnerOpen(v); + onOpenChange?.(v); + }; + return ( +
+ {Children.map(children, (child: ReactNode) => + isValidElement(child) + ? cloneElement( + child as React.ReactElement<{ + __popoverOpen?: boolean; + __setPopoverOpen?: (v: boolean) => void; + }>, + { __popoverOpen: isOpen, __setPopoverOpen: setOpen }, + ) + : child, + )} +
+ ); + } + function PopoverTrigger({ + render, + __setPopoverOpen, + }: { + render: React.ReactElement; + __setPopoverOpen?: (v: boolean) => void; + }) { + if (!isValidElement(render)) return null; + return cloneElement(render as React.ReactElement<{ onClick?: (e: unknown) => void }>, { + onClick: () => __setPopoverOpen?.(true), + }); + } + function PopoverContent({ + children, + __popoverOpen, + }: { + children: ReactNode; + __popoverOpen?: boolean; + }) { + if (!__popoverOpen) return null; + return
{children}
; + } + return { Popover, PopoverTrigger, PopoverContent }; +}); + +// Command (cmdk) wraps an input plus a list — simulate it minimally so the +// test can drive Enter/Escape on the input and click the suggestion items. +vi.mock("@multica/ui/components/ui/command", () => { + function Command({ children }: { children: ReactNode }) { + return
{children}
; + } + function CommandInput({ + value, + onValueChange, + placeholder, + onKeyDown, + autoFocus, + }: { + value: string; + onValueChange: (v: string) => void; + placeholder?: string; + onKeyDown?: (e: React.KeyboardEvent) => void; + autoFocus?: boolean; + }) { + return ( + onValueChange?.(e.target.value)} + onKeyDown={onKeyDown} + /> + ); + } + function CommandList({ children }: { children: ReactNode }) { + return
{children}
; + } + function CommandEmpty({ children }: { children: ReactNode }) { + return
{children}
; + } + function CommandGroup({ children }: { children: ReactNode }) { + return
{children}
; + } + function CommandItem({ + children, + onSelect, + value, + }: { + children: ReactNode; + onSelect?: (v: string) => void; + value?: string; + }) { + return ( + + ); + } + return { + Command, + CommandInput, + CommandList, + CommandEmpty, + CommandGroup, + CommandItem, + }; +}); + +const TEST_RESOURCES = { + en: { common: enCommon, squads: enSquads }, +}; + +function I18nWrapper({ children }: { children: ReactNode }) { + return ( + + {children} + + ); +} + +function render(ui: React.ReactElement, options?: RenderOptions) { + return rtlRender(ui, { wrapper: I18nWrapper, ...options }); +} + +import { RoleEditor } from "./squad-detail-page"; + +describe("RoleEditor (combobox)", () => { + it("commits the typed role on Enter", async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + render(); + await userEvent.click(screen.getByRole("button", { name: /add role/i })); + const input = screen.getByPlaceholderText(/type or pick/i); + await userEvent.type(input, "Reviewer{Enter}"); + expect(onSave).toHaveBeenCalledWith("Reviewer"); + }); + + it("commits a suggestion on click", async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + render( + , + ); + await userEvent.click(screen.getByRole("button", { name: /add role/i })); + await userEvent.click(screen.getByRole("button", { name: "Reviewer" })); + expect(onSave).toHaveBeenCalledWith("Reviewer"); + }); + + it("does NOT commit on blur (clicking outside)", async () => { + const onSave = vi.fn(); + render(); + await userEvent.click(screen.getByRole("button", { name: /add role/i })); + await userEvent.type(screen.getByPlaceholderText(/type or pick/i), "Partial"); + await userEvent.click(document.body); + expect(onSave).not.toHaveBeenCalled(); + }); + + it("shows Loader2 when saving", () => { + render( + Promise.resolve()} />, + ); + expect(screen.getByTestId("role-editor-saving")).toBeInTheDocument(); + }); + + it("renders Pencil icon as a persistent affordance when not saving", () => { + render( + Promise.resolve()} />, + ); + expect(screen.getByTestId("role-editor-pencil")).toBeInTheDocument(); + }); + + it("filters out the current value from suggestions", async () => { + const onSave = vi.fn().mockResolvedValue(undefined); + render( + , + ); + await userEvent.click(screen.getByRole("button", { name: /reviewer/i })); + expect(screen.getByRole("button", { name: "Implementer" })).toBeInTheDocument(); + expect(screen.queryAllByRole("button", { name: "Reviewer" })).toHaveLength(1); + }); +}); diff --git a/packages/views/squads/components/squad-detail-page.tsx b/packages/views/squads/components/squad-detail-page.tsx index 48d97a946..77e9a0b37 100644 --- a/packages/views/squads/components/squad-detail-page.tsx +++ b/packages/views/squads/components/squad-detail-page.tsx @@ -11,6 +11,7 @@ import { isImeComposing, timeAgo } from "@multica/core/utils"; import { agentListOptions, memberListOptions, workspaceKeys } from "@multica/core/workspace/queries"; import { runtimeListOptions } from "@multica/core/runtimes"; import { CreateAgentDialog } from "../../agents/components/create-agent-dialog"; +import { ArchiveSquadConfirmDialog } from "./archive-squad-confirm-dialog"; import { useNavigation } from "../../navigation"; import { AppLink } from "../../navigation"; import { PageHeader } from "../../layout/page-header"; @@ -23,6 +24,14 @@ import { PopoverContent, PopoverTrigger, } from "@multica/ui/components/ui/popover"; +import { + Command, + CommandInput, + CommandList, + CommandEmpty, + CommandGroup, + CommandItem, +} from "@multica/ui/components/ui/command"; import { Tooltip, TooltipContent, @@ -102,6 +111,7 @@ export function SquadDetailPage() { const [showAddMember, setShowAddMember] = useState(false); const [showCreateAgent, setShowCreateAgent] = useState(false); + const [archiveOpen, setArchiveOpen] = useState(false); const updateSquadMut = useMutation({ mutationFn: (data: { name?: string; description?: string; instructions?: string; avatar_url?: string; leader_id?: string }) => api.updateSquad(squadId, data), @@ -206,7 +216,7 @@ export function SquadDetailPage() {

{squad.name}

- @@ -268,6 +278,22 @@ export function SquadDetailPage() { onCreate={handleCreateAgent} /> )} + + setArchiveOpen(false)} + onConfirm={async () => { + await deleteMut.mutateAsync(); + setArchiveOpen(false); + }} + /> ); } @@ -642,59 +668,104 @@ function AddMemberDialog({ ); } -// Inline click-to-edit role line. Renders the current role as muted text; -// click (or click the placeholder when empty) to swap in an input that -// commits on blur / Enter and cancels on Escape. Avoids opening a modal -// for what is usually a one-word change. -function RoleEditor({ value, onSave }: { value: string; onSave: (next: string) => Promise }) { +// Click-to-edit role line, rendered as a Combobox (Popover + Command). +// +// The previous inline `` committed on blur, which made it easy to +// accidentally save a half-typed role by clicking elsewhere on the page — +// and there was no edit affordance, so users couldn't tell the muted text +// was clickable. The combobox fixes both: the Pencil icon is always +// visible, commit happens only on Enter or selecting an existing role, +// and clicking outside discards the draft. Suggestions come from the other +// members' roles so repeated patterns ("Reviewer", "Implementer") get +// reused instead of drifting into "reviewer", "Reviewers", etc. +export function RoleEditor({ + value, + suggestions, + saving = false, + onSave, +}: { + value: string; + suggestions: string[]; + saving?: boolean; + onSave: (next: string) => Promise; +}) { const { t } = useT("squads"); - const [editing, setEditing] = useState(false); - const [draft, setDraft] = useState(value); - const [saving, setSaving] = useState(false); + const [open, setOpen] = useState(false); + const [query, setQuery] = useState(""); - useEffect(() => { if (!editing) setDraft(value); }, [value, editing]); + const uniqueSuggestions = useMemo(() => { + const set = new Set(); + for (const s of suggestions) { + const v = s.trim(); + if (v && v !== value) set.add(v); + } + return Array.from(set).sort((a, b) => a.localeCompare(b)); + }, [suggestions, value]); - const commit = async () => { - const next = draft.trim(); - if (next === value.trim()) { setEditing(false); return; } - setSaving(true); + const commit = async (next: string) => { + const trimmed = next.trim(); + if (trimmed === value.trim()) { + setOpen(false); + setQuery(""); + return; + } try { - await onSave(next); - setEditing(false); - } catch { - // toast handled by mutation + await onSave(trimmed); } finally { - setSaving(false); + setOpen(false); + setQuery(""); } }; - if (editing) { - return ( - setDraft(e.target.value)} - onBlur={() => void commit()} - onKeyDown={(e) => { - if (isImeComposing(e)) return; - if (e.key === "Enter") void commit(); - else if (e.key === "Escape") { setDraft(value); setEditing(false); } - }} - disabled={saving} - placeholder="Role (e.g. Reviewer)" - className="h-6 mt-0.5 text-xs px-1.5" - /> - ); - } - return ( - + { if (!saving) setOpen(v); }}> + $.role_editor.add_role)} + > + {value || t(($) => $.role_editor.add_role)} + {saving ? ( + + ) : ( + + )} + + } + /> + + + $.role_editor.search_placeholder)} + onKeyDown={(e) => { + if (isImeComposing(e)) return; + if (e.key === "Enter") { + e.preventDefault(); + void commit(query); + } else if (e.key === "Escape") { + setOpen(false); + setQuery(""); + } + }} + autoFocus + /> + + {t(($) => $.role_editor.no_suggestions)} + + {uniqueSuggestions.map((s) => ( + void commit(s)}> + {s} + + ))} + + + + + ); } @@ -1045,6 +1116,16 @@ function SquadMembersTab({ }) { const { t } = useT("squads"); const p = useWorkspacePaths(); + // Per-member saving state so the Loader2 spinner only renders on the row + // currently being saved — not on every row, which the shared mutation + // `isPending` would otherwise cause. + const [savingMemberId, setSavingMemberId] = useState(null); + // Aggregate role suggestions from the squad's existing members. Drives the + // RoleEditor combobox; recomputed on `members` changes only. + const roleSuggestions = useMemo( + () => members.map((m) => m.role).filter((r): r is string => !!r), + [members], + ); return (
@@ -1092,7 +1173,16 @@ function SquadMembersTab({
{ await onUpdateRole(m, next); }} + suggestions={roleSuggestions} + saving={savingMemberId === m.id} + onSave={async (next) => { + setSavingMemberId(m.id); + try { + await onUpdateRole(m, next); + } finally { + setSavingMemberId(null); + } + }} />
diff --git a/server/internal/handler/squad.go b/server/internal/handler/squad.go index 66fd45dd4..40dcd9c69 100644 --- a/server/internal/handler/squad.go +++ b/server/internal/handler/squad.go @@ -28,6 +28,12 @@ type SquadResponse struct { UpdatedAt string `json:"updated_at"` ArchivedAt *string `json:"archived_at"` ArchivedBy *string `json:"archived_by"` + // IssueCount is only populated by GetSquad — list/create/update leave it + // nil to avoid an N+1 in the list endpoint and because the value is + // semantically irrelevant for the just-written rows. omitempty keeps the + // JSON key absent in those responses; older desktop clients that predate + // this field continue to deserialize the response unchanged. + IssueCount *int64 `json:"issue_count,omitempty"` } type SquadMemberResponse struct { @@ -195,7 +201,20 @@ func (h *Handler) GetSquad(w http.ResponseWriter, r *http.Request) { if !ok { return } - writeJSON(w, http.StatusOK, squadToResponse(squad)) + resp := squadToResponse(squad) + + count, err := h.Queries.CountIssuesForSquad(r.Context(), db.CountIssuesForSquadParams{ + WorkspaceID: squad.WorkspaceID, + AssigneeID: squad.ID, + }) + if err != nil { + // Non-fatal: log and continue with nil. The dialog falls back to + // a "no count" copy variant when the count is missing. + slog.Warn("count squad issues failed", "squad_id", uuidToString(squad.ID), "error", err) + } else { + resp.IssueCount = &count + } + writeJSON(w, http.StatusOK, resp) } func (h *Handler) UpdateSquad(w http.ResponseWriter, r *http.Request) { @@ -289,25 +308,52 @@ func (h *Handler) DeleteSquad(w http.ResponseWriter, r *http.Request) { return } - // Transfer issues assigned to this squad to the leader agent. - if err := h.Queries.TransferSquadAssignees(r.Context(), db.TransferSquadAssigneesParams{ + userID := requestUserID(r) + userUUID, ok := parseUUIDOrBadRequest(w, userID, "user_id") + if !ok { + return + } + + // Transactional: transfer assigned issues to the leader, then archive + // the squad. Either both happen or neither happens. Without this, a + // transfer error followed by a successful archive would leave issues + // pointing at an archived squad (breaking name resolution and the + // "no active issue assigned to an archived squad" invariant); a + // transfer success followed by an archive failure would silently empty + // the squad while reporting failure to the caller. + tx, err := h.TxStarter.Begin(r.Context()) + if err != nil { + writeError(w, http.StatusInternalServerError, "failed to start transaction") + return + } + defer tx.Rollback(r.Context()) + qtx := h.Queries.WithTx(tx) + + if err := qtx.TransferSquadAssignees(r.Context(), db.TransferSquadAssigneesParams{ AssigneeID: squad.ID, AssigneeID_2: squad.LeaderID, }); err != nil { - slog.Warn("transfer squad assignees failed", "squad_id", uuidToString(squad.ID), "error", err) + slog.Error("transfer squad assignees failed", "squad_id", uuidToString(squad.ID), "error", err) + writeError(w, http.StatusInternalServerError, "failed to transfer squad issues") + return } - userID := requestUserID(r) - userUUID, _ := parseUUIDOrBadRequest(w, userID, "user_id") - - if _, err := h.Queries.ArchiveSquad(r.Context(), db.ArchiveSquadParams{ + if _, err := qtx.ArchiveSquad(r.Context(), db.ArchiveSquadParams{ ID: squad.ID, ArchivedBy: userUUID, }); err != nil { + slog.Error("archive squad failed", "squad_id", uuidToString(squad.ID), "error", err) writeError(w, http.StatusInternalServerError, "failed to archive squad") return } + if err := tx.Commit(r.Context()); err != nil { + writeError(w, http.StatusInternalServerError, "failed to commit squad archive") + return + } + + // Publish only after commit — events must never describe state that + // was rolled back. h.publish(protocol.EventSquadDeleted, workspaceID, "member", userID, map[string]any{ "squad_id": uuidToString(squad.ID), "leader_id": uuidToString(squad.LeaderID), diff --git a/server/internal/handler/squad_test.go b/server/internal/handler/squad_test.go new file mode 100644 index 000000000..d2bf5f9e2 --- /dev/null +++ b/server/internal/handler/squad_test.go @@ -0,0 +1,336 @@ +package handler + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" +) + +// withSquadURLParams sets both the squad "id" and the "workspaceId" chi URL +// params on the request in a single route context — withURLParam allocates a +// fresh chi.RouteContext per call, so chaining two calls would drop the first. +func withSquadURLParams(req *http.Request, squadID string) *http.Request { + rctx := chi.NewRouteContext() + rctx.URLParams.Add("id", squadID) + rctx.URLParams.Add("workspaceId", testWorkspaceID) + return req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) +} + +// ── Test helpers ───────────────────────────────────────────────────────────── + +// createSquadForTest creates a squad owned by the workspace's leader agent. +// Returns (squadID, leaderAgentID). +func createSquadForTest(t *testing.T, name string) (string, string) { + t.Helper() + if testHandler == nil { + t.Skip("database not available") + } + + leaderID := createHandlerTestAgent(t, name+"-leader", []byte(`{}`)) + + w := httptest.NewRecorder() + body := map[string]any{ + "name": name, + "leader_id": leaderID, + } + req := newRequest(http.MethodPost, "/api/squads", body) + req = withURLParam(req, "workspaceId", testWorkspaceID) + testHandler.CreateSquad(w, req) + if w.Code != http.StatusCreated { + t.Fatalf("CreateSquad: expected 201, got %d: %s", w.Code, w.Body.String()) + } + + var resp SquadResponse + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode squad: %v", err) + } + + t.Cleanup(func() { + // Force-clean any lingering issue assignees + the squad row, regardless + // of whether the test archived the squad. + testPool.Exec(context.Background(), `DELETE FROM issue WHERE assignee_id = $1 OR creator_id = $2`, resp.ID, testUserID) + testPool.Exec(context.Background(), `DELETE FROM squad_member WHERE squad_id = $1`, resp.ID) + testPool.Exec(context.Background(), `DELETE FROM squad WHERE id = $1`, resp.ID) + }) + + return resp.ID, leaderID +} + +// createIssueAssignedToSquad seeds an issue assigned to the given squad with +// the requested status. Returns the issue UUID string. +func createIssueAssignedToSquad(t *testing.T, squadID, status string) string { + t.Helper() + ctx := context.Background() + + // Find the next number for the workspace to avoid colliding with existing + // test fixtures from other tests in the package. + var number int64 + if err := testPool.QueryRow(ctx, ` + SELECT COALESCE(MAX(number), 0) + 1 FROM issue WHERE workspace_id = $1 + `, testWorkspaceID).Scan(&number); err != nil { + t.Fatalf("compute next issue number: %v", err) + } + + var id string + if err := testPool.QueryRow(ctx, ` + INSERT INTO issue ( + workspace_id, title, description, status, priority, + assignee_type, assignee_id, creator_type, creator_id, + number, position + ) VALUES ($1, $2, '', $3, 'medium', 'squad', $4, 'member', $5, $6, 0) + RETURNING id + `, testWorkspaceID, "squad-test-"+status, status, squadID, testUserID, number).Scan(&id); err != nil { + t.Fatalf("insert squad-assigned issue: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM issue WHERE id = $1`, id) + }) + return id +} + +func getSquadInTest(t *testing.T, squadID string) SquadResponse { + t.Helper() + w := httptest.NewRecorder() + req := newRequest(http.MethodGet, "/api/squads/"+squadID, nil) + req = withSquadURLParams(req, squadID) + testHandler.GetSquad(w, req) + if w.Code != http.StatusOK { + t.Fatalf("GetSquad: expected 200, got %d: %s", w.Code, w.Body.String()) + } + var resp SquadResponse + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode squad: %v", err) + } + return resp +} + +func deleteSquadInTest(t *testing.T, squadID string) (int, string) { + t.Helper() + w := httptest.NewRecorder() + req := newRequest(http.MethodDelete, "/api/squads/"+squadID, nil) + req = withSquadURLParams(req, squadID) + testHandler.DeleteSquad(w, req) + return w.Code, w.Body.String() +} + +// readIssueAssignee reads the (assignee_type, assignee_id) of an issue +// directly from the DB. Used by the post-archive invariant assertions. +func readIssueAssignee(t *testing.T, issueID string) (string, string) { + t.Helper() + var assigneeType, assigneeID *string + if err := testPool.QueryRow(context.Background(), + `SELECT assignee_type::text, assignee_id::text FROM issue WHERE id = $1`, + issueID, + ).Scan(&assigneeType, &assigneeID); err != nil { + t.Fatalf("read issue assignee: %v", err) + } + if assigneeType == nil || assigneeID == nil { + return "", "" + } + return *assigneeType, *assigneeID +} + +// readSquadArchivedAtValid reports whether the squad row's archived_at is set. +func readSquadArchivedAtValid(t *testing.T, squadID string) bool { + t.Helper() + var archivedAt *string + if err := testPool.QueryRow(context.Background(), + `SELECT archived_at::text FROM squad WHERE id = $1`, + squadID, + ).Scan(&archivedAt); err != nil { + t.Fatalf("read squad archived_at: %v", err) + } + return archivedAt != nil +} + +// ── Tests ──────────────────────────────────────────────────────────────────── + +// TestGetSquadIncludesIssueCount locks the v3 invariant that GetSquad reports +// the count of ALL assigned issues (every status), so the archive dialog +// shows a number that matches the eventual transfer. +func TestGetSquadIncludesIssueCount(t *testing.T) { + squadID, _ := createSquadForTest(t, "issuecount") + + // Mix of statuses — count is ALL issues, regardless of status. + createIssueAssignedToSquad(t, squadID, "todo") + createIssueAssignedToSquad(t, squadID, "in_progress") + createIssueAssignedToSquad(t, squadID, "done") + createIssueAssignedToSquad(t, squadID, "cancelled") + + resp := getSquadInTest(t, squadID) + if resp.IssueCount == nil { + t.Fatalf("GetSquad must populate issue_count, got nil") + } + if *resp.IssueCount != 4 { + t.Fatalf("issue_count: expected 4, got %d", *resp.IssueCount) + } +} + +// TestDeleteSquadTransfersAllIssuesIncludingTerminalToLeader locks the v3 +// invariant that on archive every assigned issue (active OR terminal) is +// reassigned to the squad leader. Future "optimization" that reintroduces an +// active-only filter would resurrect the v2-era Unknown Squad / reopen-into- +// archived-squad bugs that the count-all + transfer-all design eliminated. +func TestDeleteSquadTransfersAllIssuesIncludingTerminalToLeader(t *testing.T) { + squadID, leaderID := createSquadForTest(t, "transferall") + + active := createIssueAssignedToSquad(t, squadID, "in_progress") + done := createIssueAssignedToSquad(t, squadID, "done") + cancelled := createIssueAssignedToSquad(t, squadID, "cancelled") + + if code, body := deleteSquadInTest(t, squadID); code != http.StatusNoContent { + t.Fatalf("DeleteSquad: expected 204, got %d: %s", code, body) + } + + for _, c := range []struct { + label string + id string + }{ + {"active", active}, + {"done (terminal)", done}, + {"cancelled (terminal)", cancelled}, + } { + t.Run(c.label, func(t *testing.T) { + at, ai := readIssueAssignee(t, c.id) + if at != "agent" { + t.Fatalf("v3 invariant: %s issue must be reassigned away from squad, got assignee_type=%q", c.label, at) + } + if ai != leaderID { + t.Fatalf("%s issue must be reassigned to squad leader (%s), got %s", c.label, leaderID, ai) + } + }) + } +} + +// ── Transactional rollback tests ───────────────────────────────────────────── +// +// These two tests force one of the two writes (transfer or archive) to fail +// after the transaction is opened, then assert that nothing about either +// the squad or its issues has changed. They lock down the v3 invariant under +// failure: DeleteSquad must be all-or-nothing, never half-archived or +// silently-emptied. + +// faultTxStarter wraps a real txStarter and replaces the returned tx with a +// fault-injecting wrapper. The wrapper fails any Exec/QueryRow whose SQL +// contains a configured substring, surfacing as a real error to the caller. +type faultTxStarter struct { + inner txStarter + failSQL string // first call whose SQL contains this substring fails +} + +func (f *faultTxStarter) Begin(ctx context.Context) (pgx.Tx, error) { + tx, err := f.inner.Begin(ctx) + if err != nil { + return nil, err + } + return &faultTx{Tx: tx, failSQL: f.failSQL}, nil +} + +type faultTx struct { + pgx.Tx + failSQL string + tripped bool +} + +var errFaultInjected = errors.New("fault-injected SQL failure") + +func (f *faultTx) shouldFail(sql string) bool { + if f.tripped || f.failSQL == "" { + return false + } + if strings.Contains(sql, f.failSQL) { + f.tripped = true + return true + } + return false +} + +func (f *faultTx) Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) { + if f.shouldFail(sql) { + return pgconn.CommandTag{}, errFaultInjected + } + return f.Tx.Exec(ctx, sql, args...) +} + +func (f *faultTx) Query(ctx context.Context, sql string, args ...any) (pgx.Rows, error) { + if f.shouldFail(sql) { + return nil, errFaultInjected + } + return f.Tx.Query(ctx, sql, args...) +} + +func (f *faultTx) QueryRow(ctx context.Context, sql string, args ...any) pgx.Row { + if f.shouldFail(sql) { + return faultRow{} + } + return f.Tx.QueryRow(ctx, sql, args...) +} + +type faultRow struct{} + +func (faultRow) Scan(dest ...any) error { return errFaultInjected } + +// withFaultTx temporarily swaps testHandler.TxStarter so DeleteSquad's +// transaction sees forced failures on queries matching failSQL. +func withFaultTx(t *testing.T, failSQL string) { + t.Helper() + original := testHandler.TxStarter + testHandler.TxStarter = &faultTxStarter{inner: original, failSQL: failSQL} + t.Cleanup(func() { testHandler.TxStarter = original }) +} + +// TestDeleteSquadFailedTransferLeavesSquadActive: when TransferSquadAssignees +// errors, the squad must NOT be archived and the issue assignee must NOT have +// changed. This guards against the legacy slog.Warn-and-continue behavior. +func TestDeleteSquadFailedTransferLeavesSquadActive(t *testing.T) { + squadID, _ := createSquadForTest(t, "failtransfer") + issueID := createIssueAssignedToSquad(t, squadID, "in_progress") + + // Match the TransferSquadAssignees SQL by a stable substring of its body. + withFaultTx(t, "UPDATE issue SET assignee_type = 'agent'") + + code, body := deleteSquadInTest(t, squadID) + if code < 500 { + t.Fatalf("transfer failure must surface as 5xx, got %d: %s", code, body) + } + + if readSquadArchivedAtValid(t, squadID) { + t.Fatalf("squad must remain active when transfer fails (archived_at was set)") + } + at, ai := readIssueAssignee(t, issueID) + if at != "squad" || ai != squadID { + t.Fatalf("transfer rollback: expected (squad, %s), got (%s, %s)", squadID, at, ai) + } +} + +// TestDeleteSquadFailedArchiveRollsBackTransfer: when ArchiveSquad errors +// after a successful transfer, the transfer must also roll back. Locks the +// reverse partial-write scenario. +func TestDeleteSquadFailedArchiveRollsBackTransfer(t *testing.T) { + squadID, _ := createSquadForTest(t, "failarchive") + issueID := createIssueAssignedToSquad(t, squadID, "in_progress") + + // Match the ArchiveSquad SQL by a stable substring of its body. + withFaultTx(t, "UPDATE squad SET archived_at = now()") + + code, body := deleteSquadInTest(t, squadID) + if code < 500 { + t.Fatalf("archive failure must surface as 5xx, got %d: %s", code, body) + } + + if readSquadArchivedAtValid(t, squadID) { + t.Fatalf("squad must remain active when archive fails (archived_at was set)") + } + at, ai := readIssueAssignee(t, issueID) + if at != "squad" || ai != squadID { + t.Fatalf("transfer rollback: expected (squad, %s), got (%s, %s)", squadID, at, ai) + } +} diff --git a/server/pkg/db/generated/issue.sql.go b/server/pkg/db/generated/issue.sql.go index 9e77f218b..f06cfdaf6 100644 --- a/server/pkg/db/generated/issue.sql.go +++ b/server/pkg/db/generated/issue.sql.go @@ -129,6 +129,33 @@ func (q *Queries) CountIssues(ctx context.Context, arg CountIssuesParams) (int64 return count, err } +const countIssuesForSquad = `-- name: CountIssuesForSquad :one +SELECT COUNT(*)::bigint AS count +FROM issue +WHERE workspace_id = $1 + AND assignee_type = 'squad' + AND assignee_id = $2 +` + +type CountIssuesForSquadParams struct { + WorkspaceID pgtype.UUID `json:"workspace_id"` + AssigneeID pgtype.UUID `json:"assignee_id"` +} + +// Count all issues currently assigned to a squad. No status filter: +// archive transfers every assigned issue to the leader (see TransferSquadAssignees +// in squad.sql), so count and transfer operate on identical sets. This avoids +// leaving archived-squad pointers in the DB, which would otherwise break +// name resolution (useActorName reads ListSquads which filters archived_at IS NULL) +// and the "no active issue can be assigned to an archived squad" invariant +// enforced by validateAssigneePair. +func (q *Queries) CountIssuesForSquad(ctx context.Context, arg CountIssuesForSquadParams) (int64, error) { + row := q.db.QueryRow(ctx, countIssuesForSquad, arg.WorkspaceID, arg.AssigneeID) + var count int64 + err := row.Scan(&count) + return count, err +} + const createIssue = `-- name: CreateIssue :one INSERT INTO issue ( workspace_id, title, description, status, priority, diff --git a/server/pkg/db/queries/issue.sql b/server/pkg/db/queries/issue.sql index 86b02b171..361668b14 100644 --- a/server/pkg/db/queries/issue.sql +++ b/server/pkg/db/queries/issue.sql @@ -146,3 +146,17 @@ UPDATE issue SET first_executed_at = now() WHERE id = $1 AND first_executed_at IS NULL RETURNING id, workspace_id, creator_type, creator_id, first_executed_at; + +-- name: CountIssuesForSquad :one +-- Count all issues currently assigned to a squad. No status filter: +-- archive transfers every assigned issue to the leader (see TransferSquadAssignees +-- in squad.sql), so count and transfer operate on identical sets. This avoids +-- leaving archived-squad pointers in the DB, which would otherwise break +-- name resolution (useActorName reads ListSquads which filters archived_at IS NULL) +-- and the "no active issue can be assigned to an archived squad" invariant +-- enforced by validateAssigneePair. +SELECT COUNT(*)::bigint AS count +FROM issue +WHERE workspace_id = $1 + AND assignee_type = 'squad' + AND assignee_id = $2;