From 1bb80b7fc9ec1b96d7ad80c131b876ef4369c571 Mon Sep 17 00:00:00 2001 From: Henry Date: Tue, 1 Sep 2026 17:54:15 +0000 Subject: [PATCH] fix(kanban): fix preview-on-click and unreachable actions menu in pipeline view (#3254) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 Co-authored-by: Carlos Florencio --- .../kanban/graph2-step-node.test.tsx | 45 ++- .../components/kanban/graph2-step-node.tsx | 8 +- .../kanban/graph2-task-pipeline.test.tsx | 119 ++++++++ .../kanban/graph2-task-pipeline.tsx | 36 ++- apps/web/e2e/pages/kanban-page.ts | 4 + .../e2e/tests/kanban/pipeline-view.spec.ts | 257 ++++++++++++++++++ 6 files changed, 437 insertions(+), 32 deletions(-) create mode 100644 apps/web/components/kanban/graph2-task-pipeline.test.tsx diff --git a/apps/web/components/kanban/graph2-step-node.test.tsx b/apps/web/components/kanban/graph2-step-node.test.tsx index e4d6db52cf..23b6bae5b7 100644 --- a/apps/web/components/kanban/graph2-step-node.test.tsx +++ b/apps/web/components/kanban/graph2-step-node.test.tsx @@ -7,16 +7,12 @@ import type { WorkflowStep } from "@/components/kanban-column"; import type { ForegroundActivity, TaskPendingAction } from "@/lib/types/http"; import { Graph2StepNode } from "./graph2-step-node"; -// The node renders inside the SPA router; stub it so the component mounts. -vi.mock("@/lib/routing/client-router", () => ({ - useRouter: () => ({ push: vi.fn() }), -})); - afterEach(() => { cleanup(); }); -const STEP: WorkflowStep = { id: "step-1", title: "In Progress", color: "#888" }; +const STEP_TITLE = "In Progress"; +const STEP: WorkflowStep = { id: "step-1", title: STEP_TITLE, color: "#888" }; const ICON_CHECK = ".tabler-icon-check"; const ICON_LOADER2 = ".tabler-icon-loader-2"; @@ -40,7 +36,7 @@ function renderCurrentNode(foregroundActivity?: ForegroundActivity | null) { hasPrev={false} hasNext={false} onMoveTask={() => undefined} - onPreviewTask={() => undefined} + onOpenTask={() => undefined} /> , ); @@ -83,7 +79,7 @@ describe("Graph2StepNode — auto-start-failed marker", () => { hasPrev={false} hasNext={false} onMoveTask={() => undefined} - onPreviewTask={() => undefined} + onOpenTask={() => undefined} /> , @@ -135,7 +131,7 @@ describe("Graph2StepNode — waiting-for-input variants", () => { hasPrev={false} hasNext={false} onMoveTask={() => undefined} - onPreviewTask={() => undefined} + onOpenTask={() => undefined} /> , ); @@ -171,12 +167,12 @@ describe("Graph2StepNode — hidden destination disclosure", () => { nextStepTitle="Done" nextStepHidden={nextStepHidden} onMoveTask={() => undefined} - onPreviewTask={() => undefined} + onOpenTask={() => undefined} /> , ); - const currentStep = screen.getByRole("button", { name: "In Progress" }); + const currentStep = screen.getByRole("button", { name: STEP_TITLE }); fireEvent.mouseEnter(currentStep.parentElement!); return screen.getByRole("button", { name: "Move to Done" }); } @@ -206,14 +202,37 @@ describe("Graph2StepNode — hidden destination disclosure", () => { nextStepTitle="Done" nextStepHidden onMoveTask={() => undefined} - onPreviewTask={() => undefined} + onOpenTask={() => undefined} /> , ); - fireEvent.focus(screen.getByRole("button", { name: "In Progress" })); + fireEvent.focus(screen.getByRole("button", { name: STEP_TITLE })); expect(screen.getByRole("button", { name: "Move to Done" })).not.toBeNull(); }); }); + +describe("Graph2StepNode — pill click routes through onOpenTask", () => { + it("calls onOpenTask and does not navigate directly on its own", () => { + const onOpenTask = vi.fn(); + render( + + undefined} + onOpenTask={onOpenTask} + /> + , + ); + + fireEvent.click(screen.getByRole("button", { name: STEP_TITLE })); + + expect(onOpenTask).toHaveBeenCalledWith(makeTask()); + }); +}); diff --git a/apps/web/components/kanban/graph2-step-node.tsx b/apps/web/components/kanban/graph2-step-node.tsx index 6390a90e75..487a01b362 100644 --- a/apps/web/components/kanban/graph2-step-node.tsx +++ b/apps/web/components/kanban/graph2-step-node.tsx @@ -1,7 +1,6 @@ "use client"; import { useState } from "react"; -import { useRouter } from "@/lib/routing/client-router"; import { IconCheck, IconCircleDashed, @@ -10,7 +9,6 @@ import { } from "@tabler/icons-react"; import { cn } from "@kandev/ui/lib/utils"; import { getTaskStateIcon } from "@/lib/ui/state-icons"; -import { linkToTask } from "@/lib/links"; import type { Task } from "@/components/kanban-card"; import type { WorkflowStep } from "@/components/kanban-column"; import { useTaskPendingInput } from "@/hooks/use-task-pending-input"; @@ -30,7 +28,7 @@ export type Graph2StepNodeProps = { hasPrev: boolean; hasNext: boolean; onMoveTask: (task: Task, targetStepId: string) => void; - onPreviewTask: (task: Task) => void; + onOpenTask: (task: Task) => void; prevStepId?: string; nextStepId?: string; prevStepTitle?: string; @@ -113,6 +111,7 @@ export function Graph2StepNode({ hasPrev, hasNext, onMoveTask, + onOpenTask, prevStepId, nextStepId, prevStepTitle, @@ -122,7 +121,6 @@ export function Graph2StepNode({ isMoving, }: Graph2StepNodeProps) { const { t } = useTranslation(); - const router = useRouter(); const [isHovered, setIsHovered] = useState(false); const [isFocused, setIsFocused] = useState(false); const pendingInput = useTaskPendingInput(task.primarySessionId, { @@ -139,7 +137,7 @@ export function Graph2StepNode({ const running = isRunningState(task.state); const handleClick = () => { - router.push(linkToTask(task.id)); + onOpenTask(task); }; const showMoveControls = isHovered || isFocused; diff --git a/apps/web/components/kanban/graph2-task-pipeline.test.tsx b/apps/web/components/kanban/graph2-task-pipeline.test.tsx new file mode 100644 index 0000000000..90e9b703b8 --- /dev/null +++ b/apps/web/components/kanban/graph2-task-pipeline.test.tsx @@ -0,0 +1,119 @@ +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { StateProvider } from "@/components/state-provider"; +import { TooltipProvider } from "@kandev/ui/tooltip"; +import { Graph2TaskPipeline, type Graph2TaskPipelineProps } from "./graph2-task-pipeline"; +import type { Task } from "@/components/kanban-card"; +import type { WorkflowStep } from "@/components/kanban-column"; + +afterEach(cleanup); + +const STEPS: WorkflowStep[] = [ + { id: "todo", title: "Todo", color: "#64748b" }, + { id: "doing", title: "Doing", color: "#3b82f6" }, + { id: "done", title: "Done", color: "#22c55e" }, +]; + +function makeTask(): Task { + return { + id: "task-1", + title: "A task", + workflowStepId: "doing", + } as Task; +} + +function renderPipeline(overrides: Partial = {}) { + const onPreviewTask = vi.fn(); + const onOpenTask = vi.fn(); + const onToggleSelect = vi.fn(); + render( + + + undefined} + onPreviewTask={onPreviewTask} + onOpenTask={onOpenTask} + onDeleteTask={() => undefined} + onArchiveTask={() => undefined} + onToggleSelect={onToggleSelect} + {...overrides} + /> + + , + ); + return { onPreviewTask, onOpenTask, onToggleSelect }; +} + +describe("Graph2TaskPipeline — click routing (the regression that hid defect 1)", () => { + it("routes a title click to onPreviewTask, not onOpenTask", () => { + const { onPreviewTask, onOpenTask } = renderPipeline(); + + fireEvent.click(screen.getByRole("button", { name: "A task" })); + + expect(onPreviewTask).toHaveBeenCalledWith(makeTask()); + expect(onOpenTask).not.toHaveBeenCalled(); + }); + + it("routes a current-step pill click to onOpenTask, not onPreviewTask", () => { + const { onPreviewTask, onOpenTask } = renderPipeline(); + + fireEvent.click(screen.getByRole("button", { name: "Doing" })); + + expect(onOpenTask).toHaveBeenCalledWith(makeTask()); + expect(onPreviewTask).not.toHaveBeenCalled(); + }); + + it("short-circuits a title click to onToggleSelect in multi-select mode", () => { + const { onPreviewTask, onOpenTask, onToggleSelect } = renderPipeline({ + isMultiSelectMode: true, + }); + + fireEvent.click(screen.getByRole("button", { name: "A task" })); + + expect(onToggleSelect).toHaveBeenCalledWith("task-1"); + expect(onPreviewTask).not.toHaveBeenCalled(); + expect(onOpenTask).not.toHaveBeenCalled(); + }); + + it("short-circuits a title click to onToggleSelect when already selected", () => { + const { onPreviewTask, onToggleSelect } = renderPipeline({ isSelected: true }); + + fireEvent.click(screen.getByRole("button", { name: "A task" })); + + expect(onToggleSelect).toHaveBeenCalledWith("task-1"); + expect(onPreviewTask).not.toHaveBeenCalled(); + }); +}); + +describe("Graph2TaskPipeline — actions cluster stays reachable off-screen (defect 2)", () => { + it("wraps the actions cluster in a sticky, right-pinned, opaque, full-height, above-pill-controls container", () => { + renderPipeline(); + + const wrapper = screen.getByTestId("pipeline-task-actions-sticky-task-1"); + expect(wrapper.className).toContain("sticky"); + expect(wrapper.className).toContain("right-0"); + expect(wrapper.className).toContain("bg-background"); + // z-20: must paint above the pill's z-10 move chevrons (F1). + expect(wrapper.className).toContain("z-20"); + // self-stretch: must span the row's full content height, not just its own + // content height, so no pill edge peeks out above/below the patch (F2). + expect(wrapper.className).toContain("self-stretch"); + }); + + it("does not render the sticky actions wrapper in multi-select mode", () => { + renderPipeline({ isMultiSelectMode: true }); + + expect(screen.queryByTestId("pipeline-task-actions-sticky-task-1")).toBeNull(); + }); + + it("keeps the action trigger touch-sized on coarse pointers", () => { + renderPipeline(); + + const trigger = screen.getByTestId("pipeline-task-actions-trigger-task-1"); + expect(trigger.className).toContain("[@media(pointer:coarse)]:h-11"); + expect(trigger.className).toContain("[@media(pointer:coarse)]:w-11"); + }); +}); diff --git a/apps/web/components/kanban/graph2-task-pipeline.tsx b/apps/web/components/kanban/graph2-task-pipeline.tsx index bfeb59e64e..2392e616b6 100644 --- a/apps/web/components/kanban/graph2-task-pipeline.tsx +++ b/apps/web/components/kanban/graph2-task-pipeline.tsx @@ -121,7 +121,7 @@ function PipelineStepNodes({ currentStepIndex, task, onMoveTask, - onPreviewTask, + onOpenTask, isMoving, }: { steps: WorkflowStep[]; @@ -129,7 +129,7 @@ function PipelineStepNodes({ currentStepIndex: number; task: Task; onMoveTask: (task: Task, targetStepId: string) => void; - onPreviewTask: (task: Task) => void; + onOpenTask: (task: Task) => void; isMoving?: boolean; }) { return ( @@ -158,7 +158,7 @@ function PipelineStepNodes({ prevStepHidden={moveTargets.prevStepHidden} nextStepHidden={moveTargets.nextStepHidden} onMoveTask={onMoveTask} - onPreviewTask={onPreviewTask} + onOpenTask={onOpenTask} isMoving={isMoving} /> @@ -192,7 +192,8 @@ function TaskActions({ @@ -334,7 +335,7 @@ export function Graph2TaskPipeline({ onToggleSelect?.(task.id); return; } - onOpenTask(task); + onPreviewTask(task); }; const handleCheckboxClick = (e: React.MouseEvent) => { @@ -345,7 +346,7 @@ export function Graph2TaskPipeline({ return (
{showCheckbox && ( @@ -373,17 +374,24 @@ export function Graph2TaskPipeline({ currentStepIndex={currentStepIndex} task={task} onMoveTask={onMoveTask} - onPreviewTask={onPreviewTask} + onOpenTask={onOpenTask} isMoving={isMoving} /> {!isMultiSelectMode && ( - +
+
+ +
+
)}
diff --git a/apps/web/e2e/pages/kanban-page.ts b/apps/web/e2e/pages/kanban-page.ts index 75c1bb0556..aaaf82b968 100644 --- a/apps/web/e2e/pages/kanban-page.ts +++ b/apps/web/e2e/pages/kanban-page.ts @@ -231,6 +231,10 @@ export class KanbanPage { return this.page.getByTestId(`pipeline-task-repo-${taskId}`); } + pipelineTaskActionsTrigger(taskId: string): Locator { + return this.page.getByTestId(`pipeline-task-actions-trigger-${taskId}`); + } + async switchToPipelineView() { await this.viewTogglePipeline.first().click(); // Wait for a pipeline-specific element — swimlane-container is shared with the kanban view. diff --git a/apps/web/e2e/tests/kanban/pipeline-view.spec.ts b/apps/web/e2e/tests/kanban/pipeline-view.spec.ts index b467595d9e..d604cc459f 100644 --- a/apps/web/e2e/tests/kanban/pipeline-view.spec.ts +++ b/apps/web/e2e/tests/kanban/pipeline-view.spec.ts @@ -1,5 +1,6 @@ import { test, expect } from "../../fixtures/test-base"; import { KanbanPage } from "../../pages/kanban-page"; +import { waitForHttp } from "../../helpers/causal-waits"; test.describe("Pipeline view", () => { test.beforeEach(async ({ testPage }) => { @@ -108,4 +109,260 @@ test.describe("Pipeline view", () => { await expect(kanban.pipelineTask(t2.id)).toHaveCount(0); await expect(kanban.multiSelectToolbar).not.toBeVisible(); }); + + test("title click opens the preview panel when 'Open preview on click' is on", async ({ + testPage, + apiClient, + seedData, + }) => { + await apiClient.saveUserSettings({ enable_preview_on_click: true }); + + const task = await apiClient.createTask(seedData.workspaceId, "Pipeline Preview Task", { + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + }); + const kanban = new KanbanPage(testPage); + await kanban.goto(); + await kanban.switchToPipelineView(); + + await kanban + .pipelineTask(task.id) + .getByRole("button", { name: "Pipeline Preview Task" }) + .click(); + + // Preview panel opens in place (URL carries taskId=), not a full-page navigation to /t/:id. + // This is a synchronous client-side route update, not a backend round trip, + // so the default assertion timeout is enough - no hand-picked budget needed. + await expect(testPage).toHaveURL(/taskId=/); + await expect(testPage.getByTestId("task-preview-panel")).toBeVisible(); + + await apiClient.saveUserSettings({ enable_preview_on_click: false }); + }); + + test("the 3-dots actions trigger stays reachable without scrolling on a long pipeline", async ({ + testPage, + apiClient, + seedData, + }) => { + await testPage.setViewportSize({ width: 1600, height: 900 }); + + // Use a dedicated workflow for the step growth rather than the shared + // worker-scoped seedData.workflowId: e2eReset keeps that workflow between + // tests but never trims its steps, so growing it here would leak a 9-step + // workflow into every later spec in this worker. + const workflow = await apiClient.createWorkflow( + seedData.workspaceId, + "Pipeline Overflow Workflow", + ); + const targetStepCount = 9; + const steps: { id: string; title: string }[] = []; + for (let position = 0; position < targetStepCount; position++) { + const title = `Step ${position}`; + const step = await apiClient.createWorkflowStep(workflow.id, title, position); + steps.push({ id: step.id, title }); + } + await apiClient.saveUserSettings({ + workspace_id: seedData.workspaceId, + workflow_filter_id: workflow.id, + }); + + // Current step is second-to-last so it has both a prev and a next move + // target (needed for the F1 regression below, which hovers its right + // chevron), matching the geometry measured for defect 2 (130px pill + + // 25px connector each). + const currentStepIndex = targetStepCount - 2; + const task = await apiClient.createTask(seedData.workspaceId, "Pipeline Overflow Task", { + workflow_id: workflow.id, + workflow_step_id: steps[currentStepIndex].id, + }); + const kanban = new KanbanPage(testPage); + await kanban.goto(); + await kanban.switchToPipelineView(); + + const scrollport = kanban + .pipelineTask(task.id) + .locator( + 'xpath=ancestor::div[contains(concat(" ", normalize-space(@class), " "), " overflow-x-auto ")]', + ) + .first(); + + // The row must genuinely overflow its scrollport, otherwise the reachability + // assertions below would pass vacuously even if the pipeline stopped overflowing. + await expect + .poll(() => scrollport.evaluate((el) => el.scrollWidth > el.clientWidth)) + .toBe(true); + + const trigger = kanban.pipelineTaskActionsTrigger(task.id); + await expect(trigger).toBeVisible(); + + const box = await trigger.boundingBox(); + const viewportSize = testPage.viewportSize(); + expect(box).not.toBeNull(); + expect(viewportSize).not.toBeNull(); + expect(box!.x + box!.width).toBeLessThanOrEqual(viewportSize!.width); + + // Reachable without scrolling: click opens the dropdown menu directly. + await trigger.click(); + await expect(testPage.getByRole("menuitem", { name: "Delete task" })).toBeVisible(); + await testPage.keyboard.press("Escape"); + + // F1 regression: scroll the current pill directly under the pinned actions + // cluster, hover it so its move chevron renders, then click at the trigger's + // raw coordinates. A missing z-index on the sticky wrapper lets the chevron + // paint (and hit-test) above the trigger, so the menu would not open. + const currentPill = kanban + .pipelineTask(task.id) + .getByRole("button", { name: steps[currentStepIndex].title }); + const [pillBox, stickyBox] = [await currentPill.boundingBox(), await trigger.boundingBox()]; + expect(pillBox).not.toBeNull(); + expect(stickyBox).not.toBeNull(); + const overlapPx = 15; + const scrollDelta = pillBox!.x + pillBox!.width - stickyBox!.x - overlapPx; + await scrollport.evaluate((el, delta) => { + el.scrollLeft += delta; + }, scrollDelta); + + const shiftedPillBox = await currentPill.boundingBox(); + expect(shiftedPillBox).not.toBeNull(); + await testPage.mouse.move( + shiftedPillBox!.x + shiftedPillBox!.width / 2, + shiftedPillBox!.y + shiftedPillBox!.height / 2, + ); + + // Confirm the hover actually rendered the move chevron before clicking past + // it - otherwise this scenario would pass vacuously if the hover never + // registered, including under the exact regression (missing z-index) it + // exists to catch. + const nextStepTitle = steps[currentStepIndex + 1].title; + await expect( + kanban.pipelineTask(task.id).getByRole("button", { name: `Move to ${nextStepTitle}` }), + ).toBeVisible(); + + const shiftedTriggerBox = await trigger.boundingBox(); + expect(shiftedTriggerBox).not.toBeNull(); + await testPage.mouse.click( + shiftedTriggerBox!.x + shiftedTriggerBox!.width / 2, + shiftedTriggerBox!.y + shiftedTriggerBox!.height / 2, + ); + await expect(testPage.getByRole("menuitem", { name: "Delete task" })).toBeVisible(); + }); + + test("keeps the 3-dots actions trigger reachable on a coarse-pointer tablet", async ({ + tabletTestPage, + apiClient, + seedData, + }) => { + const workflow = await apiClient.createWorkflow( + seedData.workspaceId, + "Pipeline Tablet Overflow Workflow", + ); + const targetStepCount = 9; + const steps: { id: string; title: string }[] = []; + for (let position = 0; position < targetStepCount; position++) { + const title = `Tablet Step ${position}`; + const step = await apiClient.createWorkflowStep(workflow.id, title, position); + steps.push({ id: step.id, title }); + } + await apiClient.saveUserSettings({ + workspace_id: seedData.workspaceId, + workflow_filter_id: workflow.id, + }); + + const task = await apiClient.createTask(seedData.workspaceId, "Pipeline Tablet Task", { + workflow_id: workflow.id, + workflow_step_id: steps[targetStepCount - 1].id, + }); + const kanban = new KanbanPage(tabletTestPage); + await kanban.goto(); + await kanban.switchToPipelineView(); + + const row = kanban.pipelineTask(task.id); + const scrollport = row + .locator( + 'xpath=ancestor::div[contains(concat(" ", normalize-space(@class), " "), " overflow-x-auto ")]', + ) + .first(); + await expect + .poll(() => scrollport.evaluate((element) => element.scrollWidth > element.clientWidth)) + .toBe(true); + + const trigger = kanban.pipelineTaskActionsTrigger(task.id); + await expect(trigger).toBeVisible(); + const box = await trigger.boundingBox(); + const viewportSize = tabletTestPage.viewportSize(); + expect(box).not.toBeNull(); + expect(viewportSize).not.toBeNull(); + expect(box!.width).toBeGreaterThanOrEqual(44); + expect(box!.height).toBeGreaterThanOrEqual(44); + expect(box!.x + box!.width).toBeLessThanOrEqual(viewportSize!.width); + + const hitTarget = await tabletTestPage.evaluate( + ({ x, y }) => + document + .elementFromPoint(x, y) + ?.closest('[data-testid^="pipeline-task-actions-trigger-"]')?.dataset + .testid ?? null, + { x: box!.x + box!.width / 2, y: box!.y + box!.height / 2 }, + ); + expect(hitTarget).toBe(`pipeline-task-actions-trigger-${task.id}`); + + await trigger.tap(); + await expect(tabletTestPage.getByRole("menuitem", { name: "Delete task" })).toBeVisible(); + }); + + test("the current pill's right move chevron stays clickable when it is the last visible pill", async ({ + testPage, + apiClient, + seedData, + }) => { + // Auto-hide-empty-steps is the common configuration that makes a task's + // own pill the LAST VISIBLE pill while it still has a next move target: + // the trailing step has no tasks, so it is hidden from `steps` but + // remains in `moveTargetSteps`, which is exactly the hidden-destination- + // disclosure affordance (nextStepHidden). Dedicated workflow, not + // seedData.workflowId, so the auto-hide setting doesn't leak into other + // specs sharing this worker's seed data. + const workflow = await apiClient.createWorkflow( + seedData.workspaceId, + "Pipeline Last Visible Pill Workflow", + ); + const currentStep = await apiClient.createWorkflowStep(workflow.id, "Current", 0); + await apiClient.createWorkflowStep(workflow.id, "Hidden Next", 1); + await apiClient.saveUserSettings({ + workspace_id: seedData.workspaceId, + workflow_filter_id: workflow.id, + workflow_ids_with_auto_hide_empty_steps: [workflow.id], + }); + + const task = await apiClient.createTask(seedData.workspaceId, "Pipeline Last Pill Task", { + workflow_id: workflow.id, + workflow_step_id: currentStep.id, + }); + const kanban = new KanbanPage(testPage); + await kanban.goto(); + await kanban.switchToPipelineView(); + + const row = kanban.pipelineTask(task.id); + const currentPill = row.getByRole("button", { name: "Current" }); + await expect(currentPill).toBeVisible(); + // Hidden Next never renders: it has no tasks and auto-hide is on. + await expect(row.getByRole("button", { name: "Hidden Next" })).toHaveCount(0); + + await currentPill.hover(); + const moveChevron = row.getByRole("button", { name: "Move to Hidden Next" }); + await expect(moveChevron).toBeVisible(); + + // A plain click exercises Playwright's actionability check, which fails + // if another element (the sticky actions wrapper) intercepts the pointer + // event at the chevron's location - the exact F6 regression. + const moved = waitForHttp(testPage, "POST", /\/tasks\/[^/]+\/move$/); + await moveChevron.click(); + await moved; + + // The move actually landed: the task's current pill is now the + // previously-hidden step, which is no longer empty and so no longer + // auto-hidden. The backend round trip is already awaited above, so this + // needs no hand-picked budget beyond the default. + await expect(row.getByRole("button", { name: "Hidden Next" })).toBeVisible(); + }); });