fix(#338): Implement optimistic rollback on Kanban drag-drop errors
- Store previous state before PATCH request - Apply optimistic update immediately on drag - Rollback UI to original position on API error - Show error toast notification on failure - Add comprehensive tests for optimistic updates and rollback Refs #338 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,22 +1,53 @@
|
|||||||
/* eslint-disable @typescript-eslint/no-non-null-assertion */
|
/* eslint-disable @typescript-eslint/no-non-null-assertion */
|
||||||
/* eslint-disable @typescript-eslint/no-empty-function */
|
/* eslint-disable @typescript-eslint/no-empty-function */
|
||||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||||
import { render, screen, within } from "@testing-library/react";
|
import { render, screen, within, waitFor, act } from "@testing-library/react";
|
||||||
import { KanbanBoard } from "./KanbanBoard";
|
import { KanbanBoard } from "./KanbanBoard";
|
||||||
import type { Task } from "@mosaic/shared";
|
import type { Task } from "@mosaic/shared";
|
||||||
import { TaskStatus, TaskPriority } from "@mosaic/shared";
|
import { TaskStatus, TaskPriority } from "@mosaic/shared";
|
||||||
|
import type { ToastContextValue } from "@mosaic/ui";
|
||||||
|
|
||||||
// Mock fetch globally
|
// Mock fetch globally
|
||||||
global.fetch = vi.fn();
|
global.fetch = vi.fn();
|
||||||
|
|
||||||
|
// Mock useToast hook from @mosaic/ui
|
||||||
|
const mockShowToast = vi.fn();
|
||||||
|
vi.mock("@mosaic/ui", () => ({
|
||||||
|
useToast: (): ToastContextValue => ({
|
||||||
|
showToast: mockShowToast,
|
||||||
|
removeToast: vi.fn(),
|
||||||
|
}),
|
||||||
|
}));
|
||||||
|
|
||||||
|
// Mock the api client's apiPatch function
|
||||||
|
const mockApiPatch = vi.fn<(endpoint: string, data: unknown) => Promise<unknown>>();
|
||||||
|
vi.mock("@/lib/api/client", () => ({
|
||||||
|
apiPatch: (endpoint: string, data: unknown): Promise<unknown> => mockApiPatch(endpoint, data),
|
||||||
|
}));
|
||||||
|
|
||||||
|
// Store drag event handlers for testing
|
||||||
|
type DragEventHandler = (event: {
|
||||||
|
active: { id: string };
|
||||||
|
over: { id: string } | null;
|
||||||
|
}) => Promise<void> | void;
|
||||||
|
let capturedOnDragEnd: DragEventHandler | null = null;
|
||||||
|
|
||||||
// Mock @dnd-kit modules
|
// Mock @dnd-kit modules
|
||||||
vi.mock("@dnd-kit/core", async () => {
|
vi.mock("@dnd-kit/core", async () => {
|
||||||
const actual = await vi.importActual("@dnd-kit/core");
|
const actual = await vi.importActual("@dnd-kit/core");
|
||||||
return {
|
return {
|
||||||
...actual,
|
...actual,
|
||||||
DndContext: ({ children }: { children: React.ReactNode }): React.JSX.Element => (
|
DndContext: ({
|
||||||
<div data-testid="dnd-context">{children}</div>
|
children,
|
||||||
),
|
onDragEnd,
|
||||||
|
}: {
|
||||||
|
children: React.ReactNode;
|
||||||
|
onDragEnd?: DragEventHandler;
|
||||||
|
}): React.JSX.Element => {
|
||||||
|
// Capture the event handler for testing
|
||||||
|
capturedOnDragEnd = onDragEnd ?? null;
|
||||||
|
return <div data-testid="dnd-context">{children}</div>;
|
||||||
|
},
|
||||||
};
|
};
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -114,9 +145,14 @@ describe("KanbanBoard", (): void => {
|
|||||||
|
|
||||||
beforeEach((): void => {
|
beforeEach((): void => {
|
||||||
vi.clearAllMocks();
|
vi.clearAllMocks();
|
||||||
|
mockShowToast.mockClear();
|
||||||
|
mockApiPatch.mockClear();
|
||||||
|
// Default: apiPatch succeeds
|
||||||
|
mockApiPatch.mockResolvedValue({});
|
||||||
|
// Also set up fetch mock for other tests that may use it
|
||||||
(global.fetch as ReturnType<typeof vi.fn>).mockResolvedValue({
|
(global.fetch as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||||
ok: true,
|
ok: true,
|
||||||
json: () => ({}),
|
json: (): Promise<object> => Promise.resolve({}),
|
||||||
} as Response);
|
} as Response);
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -273,6 +309,191 @@ describe("KanbanBoard", (): void => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("Optimistic Updates and Rollback", (): void => {
|
||||||
|
it("should apply optimistic update immediately on drag", async (): Promise<void> => {
|
||||||
|
// apiPatch is already mocked to succeed in beforeEach
|
||||||
|
render(<KanbanBoard tasks={mockTasks} onStatusChange={mockOnStatusChange} />);
|
||||||
|
|
||||||
|
// Verify initial state - task-1 is in NOT_STARTED column
|
||||||
|
const todoColumn = screen.getByTestId("column-NOT_STARTED");
|
||||||
|
expect(within(todoColumn).getByText("Design homepage")).toBeInTheDocument();
|
||||||
|
|
||||||
|
// Trigger drag end event to move task-1 to IN_PROGRESS and wait for completion
|
||||||
|
await act(async () => {
|
||||||
|
if (capturedOnDragEnd) {
|
||||||
|
const result = capturedOnDragEnd({
|
||||||
|
active: { id: "task-1" },
|
||||||
|
over: { id: TaskStatus.IN_PROGRESS },
|
||||||
|
});
|
||||||
|
if (result instanceof Promise) {
|
||||||
|
await result;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
// After the drag completes, task should be in the new column (optimistic update persisted)
|
||||||
|
const inProgressColumn = screen.getByTestId("column-IN_PROGRESS");
|
||||||
|
expect(within(inProgressColumn).getByText("Design homepage")).toBeInTheDocument();
|
||||||
|
|
||||||
|
// Verify the task is NOT in the original column anymore
|
||||||
|
const todoColumnAfter = screen.getByTestId("column-NOT_STARTED");
|
||||||
|
expect(within(todoColumnAfter).queryByText("Design homepage")).not.toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should persist update when API call succeeds", async (): Promise<void> => {
|
||||||
|
// apiPatch is already mocked to succeed in beforeEach
|
||||||
|
render(<KanbanBoard tasks={mockTasks} onStatusChange={mockOnStatusChange} />);
|
||||||
|
|
||||||
|
// Trigger drag end event
|
||||||
|
await act(async () => {
|
||||||
|
if (capturedOnDragEnd) {
|
||||||
|
const result = capturedOnDragEnd({
|
||||||
|
active: { id: "task-1" },
|
||||||
|
over: { id: TaskStatus.IN_PROGRESS },
|
||||||
|
});
|
||||||
|
if (result instanceof Promise) {
|
||||||
|
await result;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
// Wait for API call to complete
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(mockApiPatch).toHaveBeenCalledWith("/api/tasks/task-1", {
|
||||||
|
status: TaskStatus.IN_PROGRESS,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
// Verify task is in the new column after API success
|
||||||
|
const inProgressColumn = screen.getByTestId("column-IN_PROGRESS");
|
||||||
|
expect(within(inProgressColumn).getByText("Design homepage")).toBeInTheDocument();
|
||||||
|
|
||||||
|
// Verify callback was called
|
||||||
|
expect(mockOnStatusChange).toHaveBeenCalledWith("task-1", TaskStatus.IN_PROGRESS);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should rollback to original position when API call fails", async (): Promise<void> => {
|
||||||
|
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||||
|
|
||||||
|
// Mock API failure
|
||||||
|
mockApiPatch.mockRejectedValueOnce(new Error("Network error"));
|
||||||
|
|
||||||
|
render(<KanbanBoard tasks={mockTasks} onStatusChange={mockOnStatusChange} />);
|
||||||
|
|
||||||
|
// Verify initial state - task-1 is in NOT_STARTED column
|
||||||
|
const todoColumnBefore = screen.getByTestId("column-NOT_STARTED");
|
||||||
|
expect(within(todoColumnBefore).getByText("Design homepage")).toBeInTheDocument();
|
||||||
|
|
||||||
|
// Trigger drag end event
|
||||||
|
await act(async () => {
|
||||||
|
if (capturedOnDragEnd) {
|
||||||
|
const result = capturedOnDragEnd({
|
||||||
|
active: { id: "task-1" },
|
||||||
|
over: { id: TaskStatus.IN_PROGRESS },
|
||||||
|
});
|
||||||
|
if (result instanceof Promise) {
|
||||||
|
await result;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
// Wait for rollback to occur
|
||||||
|
await waitFor(() => {
|
||||||
|
// After rollback, task should be back in original column
|
||||||
|
const todoColumnAfter = screen.getByTestId("column-NOT_STARTED");
|
||||||
|
expect(within(todoColumnAfter).getByText("Design homepage")).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
// Verify callback was NOT called due to error
|
||||||
|
expect(mockOnStatusChange).not.toHaveBeenCalled();
|
||||||
|
|
||||||
|
consoleErrorSpy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should show error toast notification when API call fails", async (): Promise<void> => {
|
||||||
|
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||||
|
|
||||||
|
// Mock API failure
|
||||||
|
mockApiPatch.mockRejectedValueOnce(new Error("Server error"));
|
||||||
|
|
||||||
|
render(<KanbanBoard tasks={mockTasks} onStatusChange={mockOnStatusChange} />);
|
||||||
|
|
||||||
|
// Trigger drag end event
|
||||||
|
await act(async () => {
|
||||||
|
if (capturedOnDragEnd) {
|
||||||
|
const result = capturedOnDragEnd({
|
||||||
|
active: { id: "task-1" },
|
||||||
|
over: { id: TaskStatus.IN_PROGRESS },
|
||||||
|
});
|
||||||
|
if (result instanceof Promise) {
|
||||||
|
await result;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
// Wait for error handling
|
||||||
|
await waitFor(() => {
|
||||||
|
// Verify showToast was called with error message
|
||||||
|
expect(mockShowToast).toHaveBeenCalledWith(
|
||||||
|
"Unable to update task status. Please try again.",
|
||||||
|
"error"
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
consoleErrorSpy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should not make API call when dropping on same column", async (): Promise<void> => {
|
||||||
|
const fetchMock = global.fetch as ReturnType<typeof vi.fn>;
|
||||||
|
|
||||||
|
render(<KanbanBoard tasks={mockTasks} onStatusChange={mockOnStatusChange} />);
|
||||||
|
|
||||||
|
// Trigger drag end event with same status
|
||||||
|
await act(async () => {
|
||||||
|
if (capturedOnDragEnd) {
|
||||||
|
const result = capturedOnDragEnd({
|
||||||
|
active: { id: "task-1" },
|
||||||
|
over: { id: TaskStatus.NOT_STARTED }, // Same as task's current status
|
||||||
|
});
|
||||||
|
if (result instanceof Promise) {
|
||||||
|
await result;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
// No API call should be made
|
||||||
|
expect(fetchMock).not.toHaveBeenCalled();
|
||||||
|
// No callback should be called
|
||||||
|
expect(mockOnStatusChange).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should handle drag cancel (no drop target)", async (): Promise<void> => {
|
||||||
|
const fetchMock = global.fetch as ReturnType<typeof vi.fn>;
|
||||||
|
|
||||||
|
render(<KanbanBoard tasks={mockTasks} onStatusChange={mockOnStatusChange} />);
|
||||||
|
|
||||||
|
// Trigger drag end event with no drop target
|
||||||
|
await act(async () => {
|
||||||
|
if (capturedOnDragEnd) {
|
||||||
|
const result = capturedOnDragEnd({
|
||||||
|
active: { id: "task-1" },
|
||||||
|
over: null,
|
||||||
|
});
|
||||||
|
if (result instanceof Promise) {
|
||||||
|
await result;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
// Task should remain in original column
|
||||||
|
const todoColumn = screen.getByTestId("column-NOT_STARTED");
|
||||||
|
expect(within(todoColumn).getByText("Design homepage")).toBeInTheDocument();
|
||||||
|
|
||||||
|
// No API call should be made
|
||||||
|
expect(fetchMock).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe("Accessibility", (): void => {
|
describe("Accessibility", (): void => {
|
||||||
it("should have proper heading hierarchy", (): void => {
|
it("should have proper heading hierarchy", (): void => {
|
||||||
render(<KanbanBoard tasks={mockTasks} onStatusChange={mockOnStatusChange} />);
|
render(<KanbanBoard tasks={mockTasks} onStatusChange={mockOnStatusChange} />);
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
/* eslint-disable @typescript-eslint/no-unnecessary-condition */
|
/* eslint-disable @typescript-eslint/no-unnecessary-condition */
|
||||||
"use client";
|
"use client";
|
||||||
|
|
||||||
import React, { useState, useMemo } from "react";
|
import React, { useState, useMemo, useEffect, useCallback } from "react";
|
||||||
import type { Task } from "@mosaic/shared";
|
import type { Task } from "@mosaic/shared";
|
||||||
import { TaskStatus } from "@mosaic/shared";
|
import { TaskStatus } from "@mosaic/shared";
|
||||||
import type { DragEndEvent, DragStartEvent } from "@dnd-kit/core";
|
import type { DragEndEvent, DragStartEvent } from "@dnd-kit/core";
|
||||||
@@ -9,6 +9,7 @@ import { DndContext, DragOverlay, PointerSensor, useSensor, useSensors } from "@
|
|||||||
import { KanbanColumn } from "./KanbanColumn";
|
import { KanbanColumn } from "./KanbanColumn";
|
||||||
import { TaskCard } from "./TaskCard";
|
import { TaskCard } from "./TaskCard";
|
||||||
import { apiPatch } from "@/lib/api/client";
|
import { apiPatch } from "@/lib/api/client";
|
||||||
|
import { useToast } from "@mosaic/ui";
|
||||||
|
|
||||||
interface KanbanBoardProps {
|
interface KanbanBoardProps {
|
||||||
tasks: Task[];
|
tasks: Task[];
|
||||||
@@ -34,9 +35,18 @@ const columns = [
|
|||||||
* - Drag-and-drop using @dnd-kit/core
|
* - Drag-and-drop using @dnd-kit/core
|
||||||
* - Task cards with title, priority badge, assignee avatar
|
* - Task cards with title, priority badge, assignee avatar
|
||||||
* - PATCH /api/tasks/:id on status change
|
* - PATCH /api/tasks/:id on status change
|
||||||
|
* - Optimistic updates with rollback on error
|
||||||
*/
|
*/
|
||||||
export function KanbanBoard({ tasks, onStatusChange }: KanbanBoardProps): React.ReactElement {
|
export function KanbanBoard({ tasks, onStatusChange }: KanbanBoardProps): React.ReactElement {
|
||||||
const [activeTaskId, setActiveTaskId] = useState<string | null>(null);
|
const [activeTaskId, setActiveTaskId] = useState<string | null>(null);
|
||||||
|
// Local task state for optimistic updates
|
||||||
|
const [localTasks, setLocalTasks] = useState<Task[]>(tasks || []);
|
||||||
|
const { showToast } = useToast();
|
||||||
|
|
||||||
|
// Sync local state with props when tasks prop changes
|
||||||
|
useEffect(() => {
|
||||||
|
setLocalTasks(tasks || []);
|
||||||
|
}, [tasks]);
|
||||||
|
|
||||||
const sensors = useSensors(
|
const sensors = useSensors(
|
||||||
useSensor(PointerSensor, {
|
useSensor(PointerSensor, {
|
||||||
@@ -46,7 +56,7 @@ export function KanbanBoard({ tasks, onStatusChange }: KanbanBoardProps): React.
|
|||||||
})
|
})
|
||||||
);
|
);
|
||||||
|
|
||||||
// Group tasks by status
|
// Group tasks by status (using local state for optimistic updates)
|
||||||
const tasksByStatus = useMemo(() => {
|
const tasksByStatus = useMemo(() => {
|
||||||
const grouped: Record<TaskStatus, Task[]> = {
|
const grouped: Record<TaskStatus, Task[]> = {
|
||||||
[TaskStatus.NOT_STARTED]: [],
|
[TaskStatus.NOT_STARTED]: [],
|
||||||
@@ -56,7 +66,7 @@ export function KanbanBoard({ tasks, onStatusChange }: KanbanBoardProps): React.
|
|||||||
[TaskStatus.ARCHIVED]: [],
|
[TaskStatus.ARCHIVED]: [],
|
||||||
};
|
};
|
||||||
|
|
||||||
(tasks || []).forEach((task) => {
|
localTasks.forEach((task) => {
|
||||||
if (grouped[task.status]) {
|
if (grouped[task.status]) {
|
||||||
grouped[task.status].push(task);
|
grouped[task.status].push(task);
|
||||||
}
|
}
|
||||||
@@ -68,17 +78,29 @@ export function KanbanBoard({ tasks, onStatusChange }: KanbanBoardProps): React.
|
|||||||
});
|
});
|
||||||
|
|
||||||
return grouped;
|
return grouped;
|
||||||
}, [tasks]);
|
}, [localTasks]);
|
||||||
|
|
||||||
const activeTask = useMemo(
|
const activeTask = useMemo(
|
||||||
() => (tasks || []).find((task) => task.id === activeTaskId),
|
() => localTasks.find((task) => task.id === activeTaskId),
|
||||||
[tasks, activeTaskId]
|
[localTasks, activeTaskId]
|
||||||
);
|
);
|
||||||
|
|
||||||
function handleDragStart(event: DragStartEvent): void {
|
function handleDragStart(event: DragStartEvent): void {
|
||||||
setActiveTaskId(event.active.id as string);
|
setActiveTaskId(event.active.id as string);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Apply optimistic update to local state
|
||||||
|
const applyOptimisticUpdate = useCallback((taskId: string, newStatus: TaskStatus): void => {
|
||||||
|
setLocalTasks((prevTasks) =>
|
||||||
|
prevTasks.map((task) => (task.id === taskId ? { ...task, status: newStatus } : task))
|
||||||
|
);
|
||||||
|
}, []);
|
||||||
|
|
||||||
|
// Rollback to previous state
|
||||||
|
const rollbackUpdate = useCallback((previousTasks: Task[]): void => {
|
||||||
|
setLocalTasks(previousTasks);
|
||||||
|
}, []);
|
||||||
|
|
||||||
async function handleDragEnd(event: DragEndEvent): Promise<void> {
|
async function handleDragEnd(event: DragEndEvent): Promise<void> {
|
||||||
const { active, over } = event;
|
const { active, over } = event;
|
||||||
|
|
||||||
@@ -91,9 +113,15 @@ export function KanbanBoard({ tasks, onStatusChange }: KanbanBoardProps): React.
|
|||||||
const newStatus = over.id as TaskStatus;
|
const newStatus = over.id as TaskStatus;
|
||||||
|
|
||||||
// Find the task and check if status actually changed
|
// Find the task and check if status actually changed
|
||||||
const task = (tasks || []).find((t) => t.id === taskId);
|
const task = localTasks.find((t) => t.id === taskId);
|
||||||
|
|
||||||
if (task && task.status !== newStatus) {
|
if (task && task.status !== newStatus) {
|
||||||
|
// Store previous state for potential rollback
|
||||||
|
const previousTasks = [...localTasks];
|
||||||
|
|
||||||
|
// Apply optimistic update immediately
|
||||||
|
applyOptimisticUpdate(taskId, newStatus);
|
||||||
|
|
||||||
// Call PATCH /api/tasks/:id to update status (using API client for CSRF protection)
|
// Call PATCH /api/tasks/:id to update status (using API client for CSRF protection)
|
||||||
try {
|
try {
|
||||||
await apiPatch(`/api/tasks/${taskId}`, { status: newStatus });
|
await apiPatch(`/api/tasks/${taskId}`, { status: newStatus });
|
||||||
@@ -103,8 +131,12 @@ export function KanbanBoard({ tasks, onStatusChange }: KanbanBoardProps): React.
|
|||||||
onStatusChange(taskId, newStatus);
|
onStatusChange(taskId, newStatus);
|
||||||
}
|
}
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
// Rollback to previous state on error
|
||||||
|
rollbackUpdate(previousTasks);
|
||||||
|
|
||||||
|
// Show error notification
|
||||||
|
showToast("Unable to update task status. Please try again.", "error");
|
||||||
console.error("Error updating task status:", error);
|
console.error("Error updating task status:", error);
|
||||||
// TODO: Show error toast/notification
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user