diff --git a/packages/core/workspace/hooks.test.tsx b/packages/core/workspace/hooks.test.tsx new file mode 100644 index 000000000..e7644c773 --- /dev/null +++ b/packages/core/workspace/hooks.test.tsx @@ -0,0 +1,98 @@ +/** + * @vitest-environment jsdom + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { renderHook } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactNode } from "react"; +import { setApiInstance } from "../api"; +import type { ApiClient } from "../api/client"; +import { workspaceKeys } from "./queries"; +import { useActorName } from "./hooks"; + +// useActorName reads the current workspace from the core WorkspaceId provider; +// the directory-name resolution under test does not depend on the real id. +vi.mock("../hooks", () => ({ + useWorkspaceId: () => "ws-1", +})); + +function createWrapper(qc: QueryClient) { + return function Wrapper({ children }: { children: ReactNode }) { + return {children}; + }; +} + +describe("useActorName", () => { + let qc: QueryClient; + + beforeEach(() => { + qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + }); + + afterEach(() => { + qc.clear(); + vi.restoreAllMocks(); + }); + + // MUL-4985 regression: while the member/agent/squad directory queries are + // still loading, `data` is undefined. A `= []` default allocated a fresh + // array every render, so `getActorName` (memoized on those arrays) changed + // identity on every render. Consumers that list `getActorName` in their own + // memo deps (BoardView's `groups`, SwimLaneView's `laneGroups`) then churned + // a new value each render and spun the column-resync effect without end — + // an infinite re-render that react-virtuoso escalated into "Maximum update + // depth exceeded" on the Issues route. The fix shares one stable empty + // reference for the loading snapshot, so `getActorName` must be stable + // across re-renders while the directories are unresolved. + it("returns a referentially stable getActorName across renders during cold load", () => { + // Directory endpoints never resolve → the queries stay pending, so the + // hook renders repeatedly with undefined directory data (the cold-load + // state that used to loop). + const pending = () => new Promise(() => {}); + setApiInstance({ + listMembers: pending, + listAgents: pending, + listSquads: pending, + } as unknown as ApiClient); + + const { result, rerender } = renderHook(() => useActorName(), { + wrapper: createWrapper(qc), + }); + + const first = result.current.getActorName; + rerender(); + const second = result.current.getActorName; + rerender(); + const third = result.current.getActorName; + + expect(second).toBe(first); + expect(third).toBe(first); + // A stable resolver over an empty directory still resolves gracefully. + expect(first("member", "user-1")).toBe("Unknown"); + }); + + it("resolves names once the directories are loaded", () => { + // Seed the caches directly so the hook reads resolved directories on its + // first render — this guards that stabilizing the loading default did not + // break name resolution when data IS present. + const members = [{ user_id: "user-1", name: "Ada", avatar_url: null }]; + const agents = [{ id: "agent-1", name: "Walt", avatar_url: null }]; + const squads = [{ id: "squad-1", name: "Core", avatar_url: null }]; + setApiInstance({ + listMembers: () => Promise.resolve(members), + listAgents: () => Promise.resolve(agents), + listSquads: () => Promise.resolve(squads), + } as unknown as ApiClient); + qc.setQueryData(workspaceKeys.members("ws-1"), members); + qc.setQueryData(workspaceKeys.agents("ws-1"), agents); + qc.setQueryData(workspaceKeys.squads("ws-1"), squads); + + const { result } = renderHook(() => useActorName(), { + wrapper: createWrapper(qc), + }); + + expect(result.current.getActorName("member", "user-1")).toBe("Ada"); + expect(result.current.getActorName("agent", "agent-1")).toBe("Walt"); + expect(result.current.getActorName("squad", "squad-1")).toBe("Core"); + }); +}); diff --git a/packages/core/workspace/hooks.ts b/packages/core/workspace/hooks.ts index aa3ae5835..34c069ca9 100644 --- a/packages/core/workspace/hooks.ts +++ b/packages/core/workspace/hooks.ts @@ -2,10 +2,25 @@ import { useCallback, useMemo } from "react"; import { useQuery } from "@tanstack/react-query"; +import type { Agent, MemberWithUser, Squad } from "../types"; import { useWorkspaceId } from "../hooks"; import { memberListOptions, agentListOptions, squadListOptions } from "./queries"; import { resolvePublicFileUrl } from "./avatar-url"; +// Stable empties for the still-loading directory queries. A fresh `= []` +// default allocates a new array on every render while `data` is undefined, +// which makes `useMemo(..., [members, agents, squads])` recompute +// `getActorName` on every render during cold load. Consumers that list +// `getActorName` in their own memo deps (BoardView's `groups`, SwimLaneView's +// `laneGroups`) then churn a fresh value each render, and the board/list +// column resync `useEffect(setColumns, [groups])` re-fires without end — an +// infinite re-render that react-virtuoso turns into "Maximum update depth +// exceeded" on the Issues route (MUL-4985). Sharing one reference keeps the +// loading snapshot referentially stable. +const EMPTY_MEMBERS: MemberWithUser[] = []; +const EMPTY_AGENTS: Agent[] = []; +const EMPTY_SQUADS: Squad[] = []; + /** * Pure actor-name resolution over explicit directory snapshots. Async flows * (e.g. CSV export) must resolve names from directories they have awaited @@ -32,9 +47,9 @@ export function buildActorNameResolver(directories: { export function useActorName() { const wsId = useWorkspaceId(); - const { data: members = [] } = useQuery(memberListOptions(wsId)); - const { data: agents = [] } = useQuery(agentListOptions(wsId)); - const { data: squads = [] } = useQuery(squadListOptions(wsId)); + const { data: members = EMPTY_MEMBERS } = useQuery(memberListOptions(wsId)); + const { data: agents = EMPTY_AGENTS } = useQuery(agentListOptions(wsId)); + const { data: squads = EMPTY_SQUADS } = useQuery(squadListOptions(wsId)); const getMemberName = useCallback((userId: string) => { const m = members.find((m) => m.user_id === userId); diff --git a/packages/views/issues/components/issues-cold-load-loop.test.tsx b/packages/views/issues/components/issues-cold-load-loop.test.tsx new file mode 100644 index 000000000..ab5c11ce2 --- /dev/null +++ b/packages/views/issues/components/issues-cold-load-loop.test.tsx @@ -0,0 +1,290 @@ +/** + * @vitest-environment jsdom + * + * MUL-4985 regression — cold-load render loop on the Issues route. + * + * These tests render Board and Swimlane with the REAL react-virtuoso and the + * REAL `useActorName`, while the member/agent/squad directory queries are held + * pending (the cold-load state). Before the fix, `useActorName` returned a + * fresh `getActorName` on every render, which churned BoardView's `groups` / + * SwimLaneView's `laneGroups`, re-fired the column-resync effect without end, + * and react-virtuoso escalated it into "Maximum update depth exceeded". A + * looping render never settles, so each test would hang/throw; the fix lets it + * paint. (Unlike the sibling swimlane-view.test.tsx, this file intentionally + * does NOT mock react-virtuoso or useActorName — those two reals are the whole + * point of the reproduction.) + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, screen, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactNode } from "react"; +import { BoardView } from "./board-view"; +import { SwimLaneView } from "./swimlane-view"; +import { IssueContextMenuProvider } from "../actions"; +import { setApiInstance } from "@multica/core/api"; +import type { ApiClient } from "@multica/core/api/client"; +import type { Issue } from "@multica/core/types"; +import { I18nProvider } from "@multica/core/i18n/react"; +import enCommon from "../../locales/en/common.json"; +import enIssues from "../../locales/en/issues.json"; + +const TEST_RESOURCES = { en: { common: enCommon, issues: enIssues } }; + +vi.mock("@multica/core/hooks", () => ({ + useWorkspaceId: () => "ws-1", +})); + +vi.mock("@multica/core/paths", async () => { + const actual = await vi.importActual( + "@multica/core/paths", + ); + return { + ...actual, + useWorkspaceSlug: () => "acme", + useRequiredWorkspaceSlug: () => "acme", + useWorkspacePaths: () => actual.paths.workspace("acme"), + }; +}); + +const mockAuthUser = { id: "user-1", email: "test@test.com", name: "Test User" }; +vi.mock("@multica/core/auth", () => ({ + useAuthStore: Object.assign( + (selector?: any) => { + const state = { user: mockAuthUser, isAuthenticated: true }; + return selector ? selector(state) : state; + }, + { getState: () => ({ user: mockAuthUser, isAuthenticated: true }) }, + ), + registerAuthStore: vi.fn(), + createAuthStore: vi.fn(), +})); + +vi.mock("../../navigation", () => ({ + AppLink: ({ children, href, ...props }: any) => ( + + {children} + + ), + useNavigation: () => ({ push: vi.fn(), pathname: "/issues" }), + NavigationProvider: ({ children }: { children: React.ReactNode }) => children, +})); + +vi.mock("@multica/core/issues/config", () => ({ + ALL_STATUSES: ["backlog", "todo", "in_progress", "in_review", "done", "blocked", "cancelled"], + STATUS_ORDER: ["backlog", "todo", "in_progress", "in_review", "done", "blocked", "cancelled"], + STATUS_CONFIG: { + backlog: { label: "Backlog", iconColor: "text-muted-foreground", hoverBg: "hover:bg-accent" }, + todo: { label: "Todo", iconColor: "text-muted-foreground", hoverBg: "hover:bg-accent" }, + in_progress: { label: "In Progress", iconColor: "text-warning", hoverBg: "hover:bg-warning/10" }, + in_review: { label: "In Review", iconColor: "text-success", hoverBg: "hover:bg-success/10" }, + done: { label: "Done", iconColor: "text-info", hoverBg: "hover:bg-info/10" }, + blocked: { label: "Blocked", iconColor: "text-destructive", hoverBg: "hover:bg-destructive/10" }, + cancelled: { label: "Cancelled", iconColor: "text-muted-foreground", hoverBg: "hover:bg-accent" }, + }, + PRIORITY_ORDER: ["urgent", "high", "medium", "low", "none"], + PRIORITY_CONFIG: { + urgent: { label: "Urgent", bars: 4, color: "text-destructive" }, + high: { label: "High", bars: 3, color: "text-warning" }, + medium: { label: "Medium", bars: 2, color: "text-warning" }, + low: { label: "Low", bars: 1, color: "text-info" }, + none: { label: "No priority", bars: 0, color: "text-muted-foreground" }, + }, +})); + +const mockLoadMore = vi.fn(); +const loadMoreResult = { + total: 0, + loaded: 0, + hasMore: false, + isLoading: false, + loadMore: mockLoadMore, +}; +vi.mock("@multica/core/issues/mutations", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useLoadMoreByStatus: () => loadMoreResult, + useLoadMoreByAssigneeGroup: () => loadMoreResult, + }; +}); + +vi.mock("@multica/core/properties", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useSetIssueProperty: () => ({ mutate: vi.fn(), mutateAsync: vi.fn() }), + useUnsetIssueProperty: () => ({ mutate: vi.fn(), mutateAsync: vi.fn() }), + }; +}); + +// Board default grouping is "status"; swimlane switches to "assignee" per test. +const mockViewState: Record = { + grouping: "status", + sortBy: "position", + sortDirection: "asc", + cardProperties: { priority: true, assignee: true, dueDate: true, project: true, childProgress: true, labels: true }, + swimlaneGrouping: "assignee", + swimlaneOrders: { parent: [], project: [], assignee: [] }, + collapsedSwimlanes: { parent: [], project: [], assignee: [] }, + setSwimlaneGrouping: vi.fn(), + setSwimlaneOrder: vi.fn(), + toggleSwimlaneCollapsed: vi.fn(), + hideStatus: vi.fn(), + showStatus: vi.fn(), + priorityFilters: [], + assigneeFilters: [], + includeNoAssignee: false, + creatorFilters: [], + projectFilters: [], + includeNoProject: false, + labelFilters: [], + propertyFilters: {}, + cardPropertyIds: [], + agentRunningFilter: false, +}; +vi.mock("@multica/core/issues/stores/view-store-context", () => ({ + ViewStoreProvider: ({ children }: { children: ReactNode }) => children, + useViewStore: (selector?: any) => (selector ? selector(mockViewState) : mockViewState), + useViewStoreApi: () => ({ getState: () => mockViewState, setState: vi.fn(), subscribe: vi.fn() }), +})); + +vi.mock("@multica/core/modals", () => ({ + useModalStore: Object.assign( + () => ({ open: vi.fn() }), + { getState: () => ({ open: vi.fn() }) }, + ), +})); + +vi.mock("@dnd-kit/core", () => ({ + DndContext: ({ children }: any) => children, + DragOverlay: () => null, + PointerSensor: class {}, + useSensor: () => ({}), + useSensors: () => [], + useDroppable: () => ({ setNodeRef: vi.fn(), isOver: false }), + pointerWithin: vi.fn(), + closestCenter: vi.fn(), +})); + +vi.mock("@dnd-kit/sortable", () => ({ + SortableContext: ({ children }: any) => children, + verticalListSortingStrategy: {}, + arrayMove: (arr: T[]): T[] => arr.slice(), + useSortable: () => ({ + attributes: {}, + listeners: {}, + setNodeRef: vi.fn(), + transform: null, + transition: null, + isDragging: false, + }), +})); + +vi.mock("@dnd-kit/utilities", () => ({ + CSS: { Transform: { toString: () => undefined } }, +})); + +// The whole point: directory queries stay pending so useActorName renders in +// the cold-load state. A never-resolving promise keeps `data` undefined. +const pending = () => new Promise(() => {}); + +function makeIssue(overrides: Partial & { id: string }): Issue { + return { + workspace_id: "ws-1", + number: 1, + identifier: `PROJ-${overrides.id}`, + title: `Issue ${overrides.id}`, + description: null, + status: "todo", + priority: "none", + assignee_type: null, + assignee_id: null, + creator_type: "member", + creator_id: "user-1", + parent_issue_id: null, + project_id: null, + position: 100, + stage: null, + start_date: null, + due_date: null, + metadata: {}, + properties: {}, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + ...overrides, + }; +} + +function renderWithProviders(ui: ReactNode) { + const qc = new QueryClient({ + defaultOptions: { queries: { retry: false, gcTime: 0 } }, + }); + return render( + + + {ui} + + , + ); +} + +describe("Issues cold-load render loop (MUL-4985)", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockViewState.grouping = "status"; + mockViewState.swimlaneGrouping = "assignee"; + setApiInstance({ + listMembers: pending, + listAgents: pending, + listSquads: pending, + getAgentTaskSnapshot: () => Promise.resolve([]), + listChildrenByParents: () => Promise.resolve({ issues: [] }), + listProjects: pending, + // ActorAvatar resolves image URLs against the API base. + getBaseUrl: () => "", + } as unknown as ApiClient); + }); + + it("Board with a large column paints during cold load (real Virtuoso mounts, no update-depth loop)", async () => { + // > BOARD_VIRTUALIZE_THRESHOLD (30) issues in one status column so a real + // mounts (VirtuosoSeed is used below the threshold and cannot + // reproduce the store-driven loop). + const issues = Array.from({ length: 40 }, (_, i) => + makeIssue({ id: `b${i}`, title: `Board Card ${i}`, status: "todo", position: 100 + i }), + ); + + renderWithProviders( + , + ); + + // Reaching a stable paint (column header visible) proves the render settled + // instead of looping. + await waitFor(() => { + expect(screen.getByText("Todo")).toBeInTheDocument(); + }); + expect(screen.getByText("Board Card 0")).toBeInTheDocument(); + }); + + it("Swimlane grouped by assignee paints during cold load (real Virtuoso mounts, no update-depth loop)", async () => { + mockViewState.swimlaneGrouping = "assignee"; + const issues = [ + makeIssue({ id: "s1", title: "Swim Card 1", assignee_type: "member", assignee_id: "user-1", status: "todo" }), + makeIssue({ id: "s2", title: "Swim Card 2", assignee_type: "agent", assignee_id: "agent-1", status: "in_progress" }), + makeIssue({ id: "s3", title: "Swim Card 3", assignee_type: null, assignee_id: null, status: "todo" }), + ]; + + renderWithProviders( + , + ); + + await waitFor(() => { + expect(screen.getByText("Swim Card 1")).toBeInTheDocument(); + }); + expect(screen.getByText("Swim Card 3")).toBeInTheDocument(); + }); +}); diff --git a/packages/views/issues/components/use-drag-settle.test.tsx b/packages/views/issues/components/use-drag-settle.test.tsx new file mode 100644 index 000000000..6eb7a1efb --- /dev/null +++ b/packages/views/issues/components/use-drag-settle.test.tsx @@ -0,0 +1,53 @@ +/** + * @vitest-environment jsdom + */ +import { describe, it, expect } from "vitest"; +import { useEffect } from "react"; +import { render, act } from "@testing-library/react"; +import { useDragSettle } from "./use-drag-settle"; + +describe("useDragSettle", () => { + // MUL-4985 defense-in-depth: the board/list/swimlane resync effect calls + // `setColumns(buildColumns(...))` whenever its `groups` input changes + // identity. When `groups` churns a fresh-but-content-equal value every render + // (the cold-load failure mode), an unguarded setter allocated a new column + // object each render and spun into "Maximum update depth exceeded". The + // equality guard returns the previous reference for a content-equal rebuild, + // so React bails and the loop cannot start. + it("does not loop when a resync effect rebuilds a content-equal column map every render", () => { + let renders = 0; + function Harness() { + renders++; + const { columns, setColumns } = useDragSettle(() => ({ todo: ["a", "b"] })); + // A fresh object of identical content on every render — what an unstable + // `groups` fed through buildColumns produces. + useEffect(() => { + setColumns({ todo: ["a", "b"] }); + }); + return
{Object.keys(columns).join(",")}
; + } + + expect(() => render()).not.toThrow(); + // A guarded setter settles in one or two renders; an unguarded loop is in + // the hundreds before React throws. Bound generously to stay robust. + expect(renders).toBeLessThan(10); + }); + + it("still applies a content-changed column update", () => { + function Harness({ next }: { next: Record }) { + const { columns, setColumns } = useDragSettle(() => ({ todo: ["a"] })); + useEffect(() => { + setColumns(next); + }, [next, setColumns]); + return
{(columns.todo ?? []).join(",")}
; + } + + const { getByTestId, rerender } = render(); + expect(getByTestId("cols").textContent).toBe("a"); + + act(() => { + rerender(); + }); + expect(getByTestId("cols").textContent).toBe("a,b"); + }); +}); diff --git a/packages/views/issues/components/use-drag-settle.ts b/packages/views/issues/components/use-drag-settle.ts index 01141b134..beca36d92 100644 --- a/packages/views/issues/components/use-drag-settle.ts +++ b/packages/views/issues/components/use-drag-settle.ts @@ -1,4 +1,36 @@ -import { useCallback, useEffect, useRef, useState } from "react"; +import { + useCallback, + useEffect, + useRef, + useState, + type Dispatch, + type SetStateAction, +} from "react"; + +/** + * Content equality for the column map (`columnId -> ordered issue ids`). Two + * maps are equal when they have the same column keys and each column's id list + * matches element-for-element. Used to skip no-op `setColumns` writes. + */ +function columnsEqual( + a: Record, + b: Record, +): boolean { + if (a === b) return true; + const aKeys = Object.keys(a); + const bKeys = Object.keys(b); + if (aKeys.length !== bKeys.length) return false; + for (const key of aKeys) { + const av = a[key]; + const bv = b[key]; + if (av === bv) continue; + if (!av || !bv || av.length !== bv.length) return false; + for (let i = 0; i < av.length; i++) { + if (av[i] !== bv[i]) return false; + } + } + return true; +} /** * Shared drag/settle state machine for the issue boards (board-view, list-view). @@ -30,12 +62,34 @@ export function useDragSettle( const recentlyMovedRef = useRef(false); const [settleVersion, setSettleVersion] = useState(0); - const [columns, setColumns] = useState>( + const [columns, setColumnsState] = useState>( initialColumns, ); const columnsRef = useRef(columns); columnsRef.current = columns; + // Equality-guarded column setter. When a resync (or a no-op drag) produces a + // column map whose contents match the current one, return the SAME reference + // so React bails out of the re-render. Second line of defense against the + // cold-load update loop (MUL-4985): even if some input to `buildColumns` + // regains a per-render-unstable identity, a content-equal rebuild no longer + // forces a new state object and cannot spin the resync effect. It never + // changes the resulting value — only skips redundant writes — so drag/settle + // semantics are unchanged. + const setColumns = useCallback< + Dispatch>> + >((update) => { + setColumnsState((prev) => { + const next = + typeof update === "function" + ? (update as (p: Record) => Record)( + prev, + ) + : update; + return columnsEqual(prev, next) ? prev : next; + }); + }, []); + useEffect(() => { const id = requestAnimationFrame(() => { recentlyMovedRef.current = false;