From 56b194383d0267557e98421783f2ce89e21fbda6 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 7 Jun 2026 23:29:37 -0700 Subject: [PATCH] fix: address workflow PR feedback --- packages/dashboard/app/components/Board.tsx | 34 +-- packages/dashboard/app/components/Column.tsx | 3 +- packages/dashboard/app/components/Lane.css | 1 + .../dashboard/app/components/ListView.tsx | 38 +-- .../app/components/__tests__/Board.test.tsx | 20 ++ .../app/components/__tests__/Column.test.tsx | 28 ++- .../components/__tests__/ListView.test.tsx | 223 ++++++++++++++++++ .../__tests__/WorkflowNodeEditor.test.tsx | 1 + .../routes/__tests__/board-workflows.test.ts | 23 +- .../dashboard/src/routes/board-workflows.ts | 6 +- .../src/__tests__/stuck-task-detector.test.ts | 18 +- .../workflow-graph-task-runner.test.ts | 6 +- packages/engine/src/stuck-task-detector.ts | 3 - 13 files changed, 350 insertions(+), 54 deletions(-) diff --git a/packages/dashboard/app/components/Board.tsx b/packages/dashboard/app/components/Board.tsx index d7d09bfa11..6f109c2a29 100644 --- a/packages/dashboard/app/components/Board.tsx +++ b/packages/dashboard/app/components/Board.tsx @@ -452,23 +452,25 @@ export function Board({ tasks, projectId, maxConcurrent, onMoveTask, onPauseTask if (workflowMode && selectedWorkflow) { return (
- {workflowOptions.length > 1 && ( + {(workflowOptions.length > 1 || onCreateWorkflow || onOpenWorkflowEditor) && (
- + {workflowOptions.length > 1 && ( + + )} {onCreateWorkflow && ( +
), })); vi.mock("lucide-react", () => ({ @@ -409,6 +411,28 @@ describe("Column QuickEntryBox", () => { const quickEntry = screen.getByTestId("quick-entry-box"); expect(quickEntry.getAttribute("data-auto-expand")).toBe("false"); }); + + it("preserves selected built-in workflow id when quick-creating in workflow mode", async () => { + const onQuickCreate = vi.fn().mockResolvedValue({}); + render( + , + ); + + fireEvent.click(screen.getByRole("button", { name: "create" })); + + await waitFor(() => expect(onQuickCreate).toHaveBeenCalledWith({ + description: "Quick task", + column: "triage", + workflowId: "builtin:coding", + })); + }); }); describe("Column in-progress/in-review bulk actions", () => { diff --git a/packages/dashboard/app/components/__tests__/ListView.test.tsx b/packages/dashboard/app/components/__tests__/ListView.test.tsx index e92abadee8..8151c49c00 100644 --- a/packages/dashboard/app/components/__tests__/ListView.test.tsx +++ b/packages/dashboard/app/components/__tests__/ListView.test.tsx @@ -1350,6 +1350,52 @@ describe("ListView", () => { }); }); + it("prompts to preserve progress when dropping task with completed steps to a workflow hold column", async () => { + const tasks = [createMockTask({ + id: "FN-001", + column: "doing", + steps: [ + { title: "Step 1", status: "done" }, + { title: "Step 2", status: "pending" }, + ], + })]; + const mockOnMoveTask = vi.fn(() => Promise.resolve(tasks[0])); + mockConfirm.mockResolvedValueOnce(true); + vi.mocked(fetchBoardWorkflows).mockResolvedValue({ + flagEnabled: true, + defaultWorkflowId: "wf-custom", + workflows: [ + { + id: "wf-custom", + name: "Custom", + columns: [ + { id: "queue", name: "Queue", flags: { hold: true } }, + { id: "doing", name: "Doing", flags: { countsTowardWip: true } }, + { id: "shipped", name: "Shipped", flags: { complete: true } }, + ], + }, + ], + taskWorkflowIds: { "FN-001": "wf-custom" }, + }); + + renderListView({ tasks, onMoveTask: mockOnMoveTask }); + await waitFor(() => expect(document.querySelector('[data-column="queue"].list-drop-zone')).toBeTruthy()); + + fireEvent.drop(document.querySelector('[data-column="queue"].list-drop-zone')!, { + preventDefault: vi.fn(), + dataTransfer: { + getData: vi.fn(() => "FN-001"), + }, + }); + + await waitFor(() => { + expect(mockConfirm).toHaveBeenCalledWith(expect.objectContaining({ + title: "Preserve Progress?", + })); + expect(mockOnMoveTask).toHaveBeenCalledWith("FN-001", "queue", { preserveProgress: true }); + }); + }); + it("does not set draggable for paused tasks", () => { const tasks = [createMockTask({ id: "FN-001", paused: true })]; @@ -2334,6 +2380,42 @@ describe("ListView Quick Entry", () => { }); }); + it("preserves selected built-in workflow id when quick-creating in workflow mode", async () => { + const mockOnQuickCreate = vi.fn().mockResolvedValue(undefined); + vi.mocked(fetchBoardWorkflows).mockResolvedValue({ + flagEnabled: true, + defaultWorkflowId: "builtin:default", + workflows: [ + { + id: "builtin:default", + name: "Default", + columns: [{ id: "triage", name: "Triage", flags: { intake: true } }], + }, + { + id: "builtin:coding", + name: "Coding", + columns: [{ id: "triage", name: "Triage", flags: { intake: true } }], + }, + ], + taskWorkflowIds: {}, + }); + renderListView({ onQuickCreate: mockOnQuickCreate }); + + const selector = await screen.findByLabelText("Select workflow") as HTMLSelectElement; + fireEvent.change(selector, { target: { value: "builtin:coding" } }); + const input = screen.getByTestId("quick-entry-input"); + fireEvent.change(input, { target: { value: "Built-in workflow task" } }); + fireEvent.keyDown(input, { key: "Enter" }); + + await waitFor(() => { + expect(mockOnQuickCreate).toHaveBeenCalledWith(expect.objectContaining({ + description: "Built-in workflow task", + column: "triage", + workflowId: "builtin:coding", + })); + }); + }); + it("shows error toast when onQuickCreate fails and keeps input content", async () => { const mockOnQuickCreate = vi.fn().mockRejectedValue(new Error("Create failed")); renderListView({ onQuickCreate: mockOnQuickCreate }); @@ -2728,6 +2810,33 @@ describe("ListView - Bulk Selection", () => { expect(checkbox).toBeDisabled(); }); + it("disables checkbox for workflow archived columns", async () => { + vi.mocked(fetchBoardWorkflows).mockResolvedValue({ + flagEnabled: true, + defaultWorkflowId: "wf-custom", + workflows: [ + { + id: "wf-custom", + name: "Custom", + columns: [ + { id: "active", name: "Active", flags: { countsTowardWip: true } }, + { id: "parked", name: "Parked", flags: { archived: true } }, + ], + }, + ], + taskWorkflowIds: { "FN-001": "wf-custom" }, + }); + const tasks = [ + createMockTask({ id: "FN-001", column: "parked" }), + ]; + + render(); + await waitFor(() => expect(screen.queryAllByText("Parked").length).toBeGreaterThan(0)); + enterBulkEditMode(); + + expect(screen.getByLabelText("Select FN-001")).toBeDisabled(); + }); + it("shows selection count when tasks are selected", () => { const tasks = [ createMockTask({ id: "FN-001" }), @@ -3020,6 +3129,78 @@ describe("ListView - Bulk Selection", () => { expect(mockAddToast).toHaveBeenCalledWith("Archived 1 · 1 skipped · 0 failed", "success"); }); + it("skips workflow archived-column tasks when pausing in bulk", async () => { + const user = userEvent.setup(); + vi.mocked(fetchBoardWorkflows).mockResolvedValue({ + flagEnabled: true, + defaultWorkflowId: "wf-custom", + workflows: [ + { + id: "wf-custom", + name: "Custom", + columns: [ + { id: "active", name: "Active", flags: { countsTowardWip: true } }, + { id: "parked", name: "Parked", flags: { archived: true } }, + ], + }, + ], + taskWorkflowIds: { "FN-001": "wf-custom", "FN-002": "wf-custom" }, + }); + const tasks = [ + createMockTask({ id: "FN-001", column: "active", paused: false }), + createMockTask({ id: "FN-002", column: "parked", paused: false }), + ]; + const onPauseTask = vi.fn(async () => createMockTask()); + localStorage.setItem(scopedStorageKey("kb-dashboard-selected-tasks"), JSON.stringify(["FN-001", "FN-002"])); + + renderListView({ tasks, onPauseTask }); + enterBulkEditMode(); + await waitFor(() => expect(screen.queryAllByText("Parked").length).toBeGreaterThan(0)); + await user.click(screen.getByRole("button", { name: /^pause selected$/i })); + + await waitFor(() => { + expect(onPauseTask).toHaveBeenCalledTimes(1); + expect(onPauseTask).toHaveBeenCalledWith("FN-001"); + }); + expect(mockAddToast).toHaveBeenCalledWith("Paused 1 · 1 skipped · 0 failed", "success"); + }); + + it("skips workflow archived-column tasks when unpausing in bulk", async () => { + const user = userEvent.setup(); + vi.mocked(fetchBoardWorkflows).mockResolvedValue({ + flagEnabled: true, + defaultWorkflowId: "wf-custom", + workflows: [ + { + id: "wf-custom", + name: "Custom", + columns: [ + { id: "active", name: "Active", flags: { countsTowardWip: true } }, + { id: "parked", name: "Parked", flags: { archived: true } }, + ], + }, + ], + taskWorkflowIds: { "FN-001": "wf-custom", "FN-002": "wf-custom" }, + }); + const tasks = [ + createMockTask({ id: "FN-001", column: "active", paused: true }), + createMockTask({ id: "FN-002", column: "parked", paused: true }), + ]; + const onUnpauseTask = vi.fn(async () => createMockTask()); + localStorage.setItem(scopedStorageKey("kb-dashboard-selected-tasks"), JSON.stringify(["FN-001", "FN-002"])); + + renderListView({ tasks, onUnpauseTask }); + enterBulkEditMode(); + await waitFor(() => expect(screen.queryAllByText("Parked").length).toBeGreaterThan(0)); + await user.click(screen.getByRole("button", { name: /^unpause selected$/i })); + + await waitFor(() => { + expect(onUnpauseTask).toHaveBeenCalledTimes(1); + expect(onUnpauseTask).toHaveBeenCalledWith("FN-001"); + }); + expect(mockAddToast).toHaveBeenCalledWith("Unpaused 1 · 1 skipped · 0 failed", "success"); + }); + it("shows error summary when pause has failures", async () => { const user = userEvent.setup(); const tasks = [createMockTask({ id: "FN-001", paused: false })]; @@ -3108,6 +3289,48 @@ describe("ListView - Bulk Selection", () => { expect(screen.getByText("1 selected")).toBeInTheDocument(); }); + it("uses workflow complete and archived flags when bulk delete archives done tasks", async () => { + const user = userEvent.setup(); + vi.mocked(fetchBoardWorkflows).mockResolvedValue({ + flagEnabled: true, + defaultWorkflowId: "wf-custom", + workflows: [ + { + id: "wf-custom", + name: "Custom", + columns: [ + { id: "doing", name: "Doing", flags: { countsTowardWip: true } }, + { id: "shipped", name: "Shipped", flags: { complete: true } }, + { id: "parked", name: "Parked", flags: { archived: true } }, + ], + }, + ], + taskWorkflowIds: { "FN-001": "wf-custom", "FN-002": "wf-custom", "FN-003": "wf-custom" }, + }); + const tasks = [ + createMockTask({ id: "FN-001", column: "shipped" }), + createMockTask({ id: "FN-002", column: "doing" }), + createMockTask({ id: "FN-003", column: "parked" }), + ]; + const onArchiveTask = vi.fn(async () => createMockTask()); + const onDeleteTask = vi.fn(async () => createMockTask()); + mockConfirmWithChoice.mockResolvedValueOnce("tertiary"); + localStorage.setItem(scopedStorageKey("kb-dashboard-selected-tasks"), JSON.stringify(["FN-001", "FN-002", "FN-003"])); + + renderListView({ tasks, onArchiveTask, onDeleteTask }); + enterBulkEditMode(); + await waitFor(() => expect(screen.queryAllByText("Shipped").length).toBeGreaterThan(0)); + await user.click(screen.getByRole("button", { name: /delete selected/i })); + + await waitFor(() => { + expect(onArchiveTask).toHaveBeenCalledTimes(1); + expect(onArchiveTask).toHaveBeenCalledWith("FN-001"); + expect(onDeleteTask).toHaveBeenCalledTimes(1); + expect(onDeleteTask).toHaveBeenCalledWith("FN-002"); + }); + expect(mockAddToast).toHaveBeenCalledWith("Archived 1, deleted 1, failed 0", "success"); + }); + it("does nothing when delete confirm is cancelled", async () => { const user = userEvent.setup(); const tasks = [createMockTask({ id: "FN-001" })]; diff --git a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx index e68ed77ca4..652a754055 100644 --- a/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx +++ b/packages/dashboard/app/components/__tests__/WorkflowNodeEditor.test.tsx @@ -213,6 +213,7 @@ describe("WorkflowNodeEditor", () => { }); afterEach(() => { + localStorage.removeItem("fusion:wf-sidebar-settings-collapsed"); cleanup(); vi.clearAllMocks(); }); diff --git a/packages/dashboard/src/routes/__tests__/board-workflows.test.ts b/packages/dashboard/src/routes/__tests__/board-workflows.test.ts index b1ae4eb8dd..b17a2cc7c1 100644 --- a/packages/dashboard/src/routes/__tests__/board-workflows.test.ts +++ b/packages/dashboard/src/routes/__tests__/board-workflows.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi } from "vitest"; import { buildBoardWorkflowsPayload, DEFAULT_WORKFLOW_LANE_ID } from "../board-workflows.js"; import type { WorkflowDefinition } from "@fusion/core"; import { parseWorkflowIr } from "@fusion/core"; @@ -94,6 +94,27 @@ describe("buildBoardWorkflowsPayload", () => { expect(payload.workflows.map((w) => w.id).sort()).toEqual([DEFAULT_WORKFLOW_LANE_ID, "wf-custom"]); }); + it("logs when workflow definition listing fails and falls back to referenced workflows", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + const store = { + ...makeStore({ flagOn: true, selections: {} }), + async listWorkflowDefinitions() { + throw new Error("db unavailable"); + }, + }; + + try { + const payload = await buildBoardWorkflowsPayload(store as never, ["FN-1"]); + expect(payload.workflows.map((w) => w.id)).toContain(DEFAULT_WORKFLOW_LANE_ID); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("[board-workflows] listWorkflowDefinitions failed"), + expect.any(Error), + ); + } finally { + warnSpy.mockRestore(); + } + }); + it("describes a custom workflow's columns with resolved trait flags", async () => { const store = makeStore({ flagOn: true, diff --git a/packages/dashboard/src/routes/board-workflows.ts b/packages/dashboard/src/routes/board-workflows.ts index b8da205150..45d288515a 100644 --- a/packages/dashboard/src/routes/board-workflows.ts +++ b/packages/dashboard/src/routes/board-workflows.ts @@ -183,9 +183,11 @@ export async function buildBoardWorkflowsPayload( if (definition.kind === "fragment") continue; referenced.add(definition.id); } - } catch { + } catch (err) { // Older/partial test stores may not expose definition listing; the referenced - // workflow set above is still sufficient for task rendering. + // workflow set above is still sufficient for task rendering. Production + // failures are logged so empty workflow definitions do not disappear silently. + console.warn("[board-workflows] listWorkflowDefinitions failed; using referenced workflows only", err); } const workflows: BoardWorkflowDefinition[] = []; diff --git a/packages/engine/src/__tests__/stuck-task-detector.test.ts b/packages/engine/src/__tests__/stuck-task-detector.test.ts index 6301fc0b87..52b497b5b1 100644 --- a/packages/engine/src/__tests__/stuck-task-detector.test.ts +++ b/packages/engine/src/__tests__/stuck-task-detector.test.ts @@ -1156,7 +1156,11 @@ describe("StuckTaskDetector", () => { it("suppresses another loop classification while accepted recovery is pending", async () => { const onLoopDetected = vi.fn().mockResolvedValue(true); - const customDetector = new StuckTaskDetector(store, { onLoopDetected }); + const onStuck = vi.fn(); + const customStore = createMockStore({ + getSettings: vi.fn().mockResolvedValue({ taskStuckTimeoutMs: 60000 }), + }); + const customDetector = new StuckTaskDetector(customStore, { onLoopDetected, onStuck }); const session = createMockSession(); vi.useFakeTimers({ shouldAdvanceTime: true }); @@ -1177,13 +1181,11 @@ describe("StuckTaskDetector", () => { } expect(customDetector.classifyStuckReason("FN-201", 60000)).toBe("no-progress-churn"); - customDetector.recordProgress("FN-201"); - vi.advanceTimersByTime(61000); - for (let i = 0; i < 80; i++) { - customDetector.recordActivity("FN-201"); - } - - expect(customDetector.classifyStuckReason("FN-201", 60000)).toBe("loop"); + await customDetector.checkNow(); + expect(onStuck).toHaveBeenCalledWith(expect.objectContaining({ + taskId: "FN-201", + reason: "no-progress-churn", + })); vi.useRealTimers(); }); diff --git a/packages/engine/src/__tests__/workflow-graph-task-runner.test.ts b/packages/engine/src/__tests__/workflow-graph-task-runner.test.ts index 78506d1b3d..b28177a3e4 100644 --- a/packages/engine/src/__tests__/workflow-graph-task-runner.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-task-runner.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import type { Settings, TaskDetail, WorkflowDefinition, WorkflowIr } from "@fusion/core"; import { WorkflowGraphTaskRunner, type WorkflowGraphRunnerStore } from "../workflow-graph-task-runner.js"; @@ -165,9 +165,10 @@ describe("WorkflowGraphTaskRunner (CU-U2)", () => { it("resolves built-in workflow selections without requiring the store to return a definition", async () => { const calls: string[] = []; + const getWorkflowDefinition = vi.fn(async () => undefined); const store: WorkflowGraphRunnerStore = { getTaskWorkflowSelection: () => ({ workflowId: "builtin:coding", stepIds: [] }), - getWorkflowDefinition: async () => undefined, + getWorkflowDefinition, }; const runner = new WorkflowGraphTaskRunner({ store, @@ -180,6 +181,7 @@ describe("WorkflowGraphTaskRunner (CU-U2)", () => { expect(result.disposition).toBe("completed"); expect(calls).toEqual(["execute", "review", "merge"]); expect(result.reason).toBeUndefined(); + expect(getWorkflowDefinition).not.toHaveBeenCalled(); }); it("falls back (never strands the task) when the interpreter throws", async () => { diff --git a/packages/engine/src/stuck-task-detector.ts b/packages/engine/src/stuck-task-detector.ts index 356284588f..5d0a5a7390 100644 --- a/packages/engine/src/stuck-task-detector.ts +++ b/packages/engine/src/stuck-task-detector.ts @@ -649,9 +649,6 @@ export class StuckTaskDetector { const stuckTasks: string[] = []; for (const [taskId, entry] of this.tracked) { - if (entry.recoveryInProgress) { - continue; - } const reason = this.classifyStuckReason(taskId, timeoutMs); if (reason !== null) { // U8: suppress flagging while the CLI session is waitingOnInput