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 <noreply@anthropic.com>
Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
Henry
2026-09-01 17:54:15 +00:00
committed by GitHub
parent 0cf5362cf8
commit 1bb80b7fc9
6 changed files with 437 additions and 32 deletions
@@ -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}
/>
</StateProvider>,
);
@@ -83,7 +79,7 @@ describe("Graph2StepNode — auto-start-failed marker", () => {
hasPrev={false}
hasNext={false}
onMoveTask={() => undefined}
onPreviewTask={() => undefined}
onOpenTask={() => undefined}
/>
</TooltipProvider>
</StateProvider>,
@@ -135,7 +131,7 @@ describe("Graph2StepNode — waiting-for-input variants", () => {
hasPrev={false}
hasNext={false}
onMoveTask={() => undefined}
onPreviewTask={() => undefined}
onOpenTask={() => undefined}
/>
</StateProvider>,
);
@@ -171,12 +167,12 @@ describe("Graph2StepNode — hidden destination disclosure", () => {
nextStepTitle="Done"
nextStepHidden={nextStepHidden}
onMoveTask={() => undefined}
onPreviewTask={() => undefined}
onOpenTask={() => undefined}
/>
</TooltipProvider>
</StateProvider>,
);
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}
/>
</TooltipProvider>
</StateProvider>,
);
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(
<StateProvider>
<Graph2StepNode
step={STEP}
phase="current"
task={makeTask()}
hasPrev={false}
hasNext={false}
onMoveTask={() => undefined}
onOpenTask={onOpenTask}
/>
</StateProvider>,
);
fireEvent.click(screen.getByRole("button", { name: STEP_TITLE }));
expect(onOpenTask).toHaveBeenCalledWith(makeTask());
});
});
@@ -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;
@@ -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<Graph2TaskPipelineProps> = {}) {
const onPreviewTask = vi.fn();
const onOpenTask = vi.fn();
const onToggleSelect = vi.fn();
render(
<StateProvider>
<TooltipProvider delayDuration={0}>
<Graph2TaskPipeline
task={makeTask()}
steps={STEPS}
moveTargetSteps={STEPS}
onMoveTask={() => undefined}
onPreviewTask={onPreviewTask}
onOpenTask={onOpenTask}
onDeleteTask={() => undefined}
onArchiveTask={() => undefined}
onToggleSelect={onToggleSelect}
{...overrides}
/>
</TooltipProvider>
</StateProvider>,
);
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");
});
});
@@ -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({
<button
ref={archiveAnchorRef}
type="button"
className="shrink-0 h-7 w-7 flex items-center justify-center rounded-md text-muted-foreground/40 hover:text-foreground hover:bg-accent/60 transition-colors cursor-pointer"
data-testid={`pipeline-task-actions-trigger-${task.id}`}
className="shrink-0 h-7 w-7 flex items-center justify-center rounded-md text-muted-foreground/40 hover:text-foreground hover:bg-accent/60 transition-colors cursor-pointer [@media(pointer:coarse)]:h-11 [@media(pointer:coarse)]:w-11"
>
<IconDots className="h-3.5 w-3.5" />
</button>
@@ -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 (
<div
data-testid={`pipeline-task-${task.id}`}
className="flex min-w-max items-center justify-start rounded-lg hover:bg-muted/30 transition-colors px-3 py-2"
className="group flex min-w-max items-center justify-start rounded-lg hover:bg-muted/30 transition-colors px-3 py-2"
>
<div className="flex items-center gap-3">
{showCheckbox && (
@@ -373,17 +374,24 @@ export function Graph2TaskPipeline({
currentStepIndex={currentStepIndex}
task={task}
onMoveTask={onMoveTask}
onPreviewTask={onPreviewTask}
onOpenTask={onOpenTask}
isMoving={isMoving}
/>
{!isMultiSelectMode && (
<TaskActions
task={task}
onDeleteTask={onDeleteTask}
onArchiveTask={onArchiveTask}
isDeleting={isDeleting}
isArchiving={isArchiving}
/>
<div
data-testid={`pipeline-task-actions-sticky-${task.id}`}
className="sticky right-0 z-20 shrink-0 self-stretch bg-background"
>
<div className="flex h-full items-center group-hover:bg-muted/30 transition-colors">
<TaskActions
task={task}
onDeleteTask={onDeleteTask}
onArchiveTask={onArchiveTask}
isDeleting={isDeleting}
isArchiving={isArchiving}
/>
</div>
</div>
)}
</div>
</div>
+4
View File
@@ -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.
@@ -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<HTMLElement>('[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();
});
});