diff --git a/.changeset/kanban-rejected-move-rollback-4138.md b/.changeset/kanban-rejected-move-rollback-4138.md new file mode 100644 index 0000000000..08e39a2547 --- /dev/null +++ b/.changeset/kanban-rejected-move-rollback-4138.md @@ -0,0 +1,13 @@ +--- +'@object-ui/plugin-kanban': patch +--- + +A rejected Kanban drag rolls the card back on both data ownerships, not just when the board owns its own records + +Dragging a card into a column the server refuses (`PATCH` 400 `invalid_transition`) left the card sitting in the target column until a manual reload, whenever the board was hosted by a parent that supplies records through the `data` prop — the ListView/console path, which is the one real users meet. The toast fired and the server value was untouched, so the board was showing a move that had not happened. + +`handleCardMove` performed its failure revert only inside `if (!hasExternalData)`. The reasoning recorded next to it was that the parent handles the refresh, and for an accepted move it does — the parent's mutation subscription refetches and the new value propagates. A *rejected* move changes nothing server-side, so no refetch is ever triggered and nothing un-said the optimistic move. + +The revert is now unconditional, which is also what makes it a single code path rather than two. The card's on-screen position does not live in `ObjectKanban` at all: the board component moves the card inside its own column state before reporting the move upward, and re-syncs that state from its `columns` prop whenever the prop's identity changes — which any re-render of `ObjectKanban` produces, since the renderer re-buckets the records into fresh column arrays. On the internal path the revert corrects the record and re-renders; on the external path it re-renders against the parent's records, which the server never changed, and the re-bucket puts the card back where it started. + +The optimistic write on the way *in* stays gated on internal data deliberately, and the asymmetry is now pinned by tests: writing it on the external path would re-render against the unchanged parent records and snap an accepted move back before the server had answered. Accepted moves on both paths, and the existing rejection toast, are covered by controls alongside the regression test. diff --git a/packages/plugin-kanban/src/ObjectKanban.rejectedMoveRollback.test.tsx b/packages/plugin-kanban/src/ObjectKanban.rejectedMoveRollback.test.tsx new file mode 100644 index 0000000000..ae1a852569 --- /dev/null +++ b/packages/plugin-kanban/src/ObjectKanban.rejectedMoveRollback.test.tsx @@ -0,0 +1,252 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * A REJECTED cross-column drag rolls the card back to the column it came from — + * on BOTH data ownerships, including the ListView-hosted (external `data` prop) + * one. objectui#4138. + * + * ── Where the optimistic move actually lives ────────────────────────────── + * Not in `ObjectKanban`. `KanbanImpl`'s `handleDragEnd` moves the card inside + * its OWN `boardColumns` state and only then calls `onCardMove`; that state is + * reset from the `columns` prop by an effect that fires whenever `ObjectKanban` + * re-renders (the schema handed to `KanbanRenderer` is a fresh object literal + * each render, so `bucketCardsIntoColumns` re-runs and the columns identity + * changes). So the card moves on screen on both paths, and what un-says the + * move is a re-render of `ObjectKanban` that re-buckets the source-of-truth + * records. + * + * That is why the pre-fix bug was invisible to `ObjectKanban`'s own state: on + * the external path `handleCardMove`'s failure branch took NO state action at + * all (`if (!hasExternalData)` gated the revert), so nothing re-rendered and + * `boardColumns` kept the card in the target column until a manual reload — + * exactly the QA signature in #4138. + * + * ── Direction of these pins ─────────────────────────────────────────────── + * - external + rejected → RED before the fix (card stuck in "In Progress"), + * GREEN after. This is the issue. + * - internal + rejected → GREEN before and after. The control that proves + * the suite can see a rollback at all, so the red + * above is the gate and not the harness. + * - both + accepted → GREEN before and after. Guards the fix against + * over-reverting a move the server accepted. + * + * The board is driven through `DndContext`'s real `onDragEnd`, captured by the + * module mock below: dnd-kit's pointer sensors need layout/pointer-capture that + * jsdom does not provide, so a synthesized drop on the production handler is + * the closest honest reproduction. Everything downstream of that call — + * `KanbanImpl`'s local move, `ObjectKanban`'s persist, the failure branch, the + * re-bucket and the reset effect — is the real code path. + */ + +import React from 'react'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, within, act, cleanup, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { registerAllFields } from '@object-ui/fields'; +import { toast } from '@object-ui/components'; +import type { DataSource } from '@object-ui/types'; +import { ObjectKanban } from './ObjectKanban'; + +// Pay the board's lazy chunk at import time rather than racing it against a +// `findBy` budget (AGENTS.md §测试纪律); specifier byte-identical to `./index`'s +// so the component's own `React.lazy` factory resolves from the ESM cache. +import './KanbanImpl'; + +// `vi.hoisted` so the mock factory — hoisted above every import — can reach this +// box. A plain `const` would still be in its TDZ when `@dnd-kit/core` is first +// requested by `KanbanImpl`. +const dnd = vi.hoisted(() => ({ + onDragEnd: undefined as undefined | ((event: unknown) => void), +})); + +// Capture the board's real `onDragEnd` while still rendering the real provider, +// so `@dnd-kit/sortable`'s hooks keep reading the same context instance. +vi.mock('@dnd-kit/core', async (importOriginal) => { + const actual = await importOriginal(); + const ReactMod = await import('react'); + const CapturingDndContext = (props: Record) => { + dnd.onDragEnd = props.onDragEnd as (event: unknown) => void; + return ReactMod.createElement(actual.DndContext, props as never); + }; + return { ...actual, DndContext: CapturingDndContext }; +}); + +registerAllFields(); + +const objectDef = { + name: 'task', + fields: { + title: { type: 'text', label: 'Title' }, + status: { + type: 'picklist', + label: 'Status', + options: [ + { value: 'backlog', label: 'Backlog' }, + { value: 'in_progress', label: 'In Progress' }, + ], + }, + }, +}; + +const CARD = 'Fix the widget'; + +const schema = { + type: 'object-kanban', + objectName: 'task', + groupBy: 'status', + cardTitle: 'title', + columns: [ + { id: 'backlog', title: 'Backlog' }, + { id: 'in_progress', title: 'In Progress' }, + ], +} as never; + +/** Server truth: the card is in `backlog` and the server never moves it. */ +const serverRecords = () => [{ id: 't1', title: CARD, status: 'backlog' }]; + +/** + * The 400 the QA run measured: an illegal Backlog → In Progress transition. + * Deliberately NOT a 403 — `isPermissionError` must not claim it, so the + * failure branch takes its `extractWriteErrorMessage` arm. + */ +function invalidTransition(): Error { + return Object.assign(new Error('Invalid status transition'), { + status: 400, + code: 'invalid_transition', + }); +} + +function makeDataSource(update: DataSource['update']): DataSource { + return { + getObjectSchema: vi.fn(async () => objectDef), + find: vi.fn(async () => ({ value: serverRecords() })), + update, + } as unknown as DataSource; +} + +/** The cards currently rendered inside a column, by the column's visible title. */ +function cardsIn(columnTitle: string): string[] { + const list = screen.getByRole('list', { name: `${columnTitle} cards` }); + return within(list) + .queryAllByRole('listitem') + .map((el) => el.getAttribute('aria-label') ?? ''); +} + +/** + * Mount the board on one of the two data ownerships and wait until it has + * settled — the object-schema fetch and (internal path) the record fetch both + * land as async state updates, and a stray one arriving AFTER the drag would + * re-render the board and mask the very bug under test. + */ +async function mountBoard(mode: 'external' | 'internal', dataSource: DataSource) { + render( + , + ); + expect(await screen.findByText(CARD)).toBeInTheDocument(); + // Flush the object-def / record fetches so nothing re-renders mid-assertion. + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + await waitFor(() => expect(cardsIn('Backlog')).toEqual([CARD])); +} + +/** Drop the card onto the "In Progress" column, exactly as the board would. */ +async function dropOnInProgress() { + expect(dnd.onDragEnd).toBeTypeOf('function'); + await act(async () => { + dnd.onDragEnd!({ active: { id: 't1' }, over: { id: 'in_progress' } }); + }); +} + +beforeEach(() => { + dnd.onDragEnd = undefined; + vi.spyOn(toast, 'error').mockImplementation(() => 'toast-id' as never); +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe('ObjectKanban — rejected drag rolls the card back (#4138)', () => { + it('reverts on the EXTERNAL-data path (ListView-hosted): card returns to Backlog', async () => { + const update = vi.fn(async () => { + throw invalidTransition(); + }); + await mountBoard('external', makeDataSource(update as never)); + + await dropOnInProgress(); + + expect(update).toHaveBeenCalledTimes(1); + expect(update).toHaveBeenCalledWith('task', 't1', { status: 'in_progress' }); + // The gate: pre-fix this reads ['Fix the widget'] for "In Progress" and [] + // for "Backlog" — the stuck card of #4138. + expect(cardsIn('Backlog')).toEqual([CARD]); + expect(cardsIn('In Progress')).toEqual([]); + // The rollback moves the card, it does not clone it. + expect(screen.getAllByRole('listitem', { name: CARD })).toHaveLength(1); + }); + + it('still surfaces the rejection as a toast on the EXTERNAL-data path', async () => { + const update = vi.fn(async () => { + throw invalidTransition(); + }); + await mountBoard('external', makeDataSource(update as never)); + + await dropOnInProgress(); + + expect(toast.error).toHaveBeenCalledTimes(1); + expect(toast.error).toHaveBeenCalledWith('Invalid status transition'); + }); + + it('reverts on the INTERNAL-data path (control: green before and after)', async () => { + const update = vi.fn(async () => { + throw invalidTransition(); + }); + await mountBoard('internal', makeDataSource(update as never)); + + await dropOnInProgress(); + + expect(update).toHaveBeenCalledTimes(1); + expect(cardsIn('Backlog')).toEqual([CARD]); + expect(cardsIn('In Progress')).toEqual([]); + expect(screen.getAllByRole('listitem', { name: CARD })).toHaveLength(1); + }); + + it('keeps an ACCEPTED move committed on the EXTERNAL-data path', async () => { + const update = vi.fn(async () => ({ id: 't1', status: 'in_progress' })); + await mountBoard('external', makeDataSource(update as never)); + + await dropOnInProgress(); + + expect(update).toHaveBeenCalledTimes(1); + expect(toast.error).not.toHaveBeenCalled(); + // The parent owns the data and has not re-fetched yet; the board must keep + // showing the move it just made rather than snapping back. + expect(cardsIn('In Progress')).toEqual([CARD]); + expect(cardsIn('Backlog')).toEqual([]); + }); + + it('keeps an ACCEPTED move committed on the INTERNAL-data path', async () => { + const update = vi.fn(async () => ({ id: 't1', status: 'in_progress' })); + await mountBoard('internal', makeDataSource(update as never)); + + await dropOnInProgress(); + + expect(update).toHaveBeenCalledTimes(1); + expect(toast.error).not.toHaveBeenCalled(); + expect(cardsIn('In Progress')).toEqual([CARD]); + expect(cardsIn('Backlog')).toEqual([]); + }); +}); diff --git a/packages/plugin-kanban/src/ObjectKanban.tsx b/packages/plugin-kanban/src/ObjectKanban.tsx index 166b5b4939..8177d36532 100644 --- a/packages/plugin-kanban/src/ObjectKanban.tsx +++ b/packages/plugin-kanban/src/ObjectKanban.tsx @@ -584,8 +584,12 @@ export const ObjectKanban: React.FC = ({ if (toColumnId === KANBAN_UNCOLUMNED_ID) return; // Optimistic local update so the card visibly stays in the new column. - // Skipped when data is owned by a parent (ListView) — the parent's - // mutation subscription will refetch and propagate the change. + // Skipped when data is owned by a parent (ListView): `fetchedData` is not + // what renders on that path (`rawData` prefers `externalData`, :219), and + // writing it anyway would re-render us and re-bucket from the unchanged + // `externalData` — snapping the card back before the server has answered. + // The board's own `boardColumns` already shows the move there. The + // failure revert below is deliberately NOT gated — see the note on it. if (!hasExternalData) { setFetchedData((prev) => prev.map((r) => @@ -611,16 +615,37 @@ export const ObjectKanban: React.FC = ({ ? tt('errors.unauthorized', 'You are not authorized to perform this action.') : extractWriteErrorMessage(err) ?? tt('table.saveFailed', 'Save failed'), ); - if (!hasExternalData) { - // Revert optimistic update on failure - setFetchedData((prev) => - prev.map((r) => - String(r.id ?? r._id) === String(cardId) - ? { ...r, [groupBy]: fromColumnId } - : r, - ), - ); - } + // Roll the optimistic move back, on BOTH data ownerships (#4138). + // + // The optimistic move is the kanban's own local display state, so + // un-saying it on rejection is the kanban's job regardless of who owns + // the records. This used to be gated on `!hasExternalData` in the + // belief that the parent handles the refresh (:147); a parent does + // re-render on its own refetch, but a REJECTED move changes nothing + // server-side, so nothing ever triggers that refetch and the card sat + // in the target column until a manual reload. + // + // ONE unconditional call covers both paths, because the card's + // on-screen position lives in `KanbanImpl`'s `boardColumns` rather than + // here: its `handleDragEnd` moves the card there before calling us, and + // an effect re-syncs `boardColumns` from the `columns` prop whenever + // that prop's identity changes — which every re-render of this + // component causes (`KanbanRenderer` re-buckets into a fresh array). + // - internal data: `fetchedData` is the source of truth, so the map + // below both corrects the record and re-renders. + // - external data: `fetchedData` is unread and normally empty, but + // `Array#map` always allocates, so the fresh identity re-renders us + // and the board re-buckets from `externalData` — which the server + // never changed. That IS the revert: the card returns to + // `fromColumnId`, and an accepted move is left alone because this + // runs only on the failure branch. + setFetchedData((prev) => + prev.map((r) => + String(r.id ?? r._id) === String(cardId) + ? { ...r, [groupBy]: fromColumnId } + : r, + ), + ); } }, [schema.groupBy, schema.objectName, dataSource, hasExternalData, tt],