diff --git a/web/e2e/permission-walk-collections.spec.ts b/web/e2e/permission-walk-collections.spec.ts index 2cf2d9828..4d2af8785 100644 --- a/web/e2e/permission-walk-collections.spec.ts +++ b/web/e2e/permission-walk-collections.spec.ts @@ -306,7 +306,7 @@ async function dragToLane(page: Page, title: string, lane: string): Promise writes.some((w) => w.includes('"status"')), { timeout: 5_000 }) .toBe(true) @@ -350,12 +350,12 @@ test('owner: dragging an editable card to another lane sends its write (control) expect(drag.writes.some((w) => w.startsWith('200 '))).toBe(true); }); -// BUG-3259 PIN. This asserts today's DEFECT, not the goal: the view-only card -// engages, and its status write is refused 403. It is a pin rather than -// test.fail because test.fail would also swallow a failing CONTROL below, -// and a broken gesture must fail loudly. When BUG-3259 is fixed, flip the -// two pinned assertions to: engaged false, and no writes. -test('guestPrecedence: a view-only card is draggable today (BUG-3259 pin)', async ({ page }) => { +// BUG-3259: the view-only card does not drag, and nothing is written. The +// in-test control drags an editable card through the same gesture, and its +// lane renumber no longer writes the view-only neighbour (it used to send a +// refused sort_order write for A). A is not a test.fail: a failing CONTROL +// must fail loudly. The other doors are in permission-walk-move.spec.ts. +test('guestPrecedence: a view-only card cannot be dragged to another lane (BUG-3259)', async ({ page }) => { await open(page, 'guestPrecedence', '/tasks', 'board'); // In-test control: this guest may edit task C through its collection // grant, so the same gesture on C must start and write. @@ -364,13 +364,11 @@ test('guestPrecedence: a view-only card is draggable today (BUG-3259 pin)', asyn expect(control.engaged, 'control drag did not start: the gesture is broken').toBe(true); expect(control.entered, 'control drag never reached the target lane').toBe(true); expect(control.writes.some((w) => w.startsWith('200 ') && w.includes('"status"'))).toBe(true); + expect(control.writes.filter((w) => w.startsWith('403 ')), 'the control drag sent a refused write').toEqual([]); const viewOnly = await dragUntilEngaged(page, walk.grantedTask.title, await otherLane(page, walk.grantedTask.title)); - expect(viewOnly.engaged, 'BUG-3259 fixed? Flip this pin').toBe(true); - expect(viewOnly.entered, 'the view-only card never reached the target lane').toBe(true); - expect(viewOnly.writes, 'the drop sent no status write').toEqual( - expect.arrayContaining([expect.stringMatching(/^403 .*"status"/)]) - ); + expect(viewOnly.engaged, 'the view-only card started a drag').toBe(false); + expect(viewOnly.writes, 'the view-only card sent writes').toEqual([]); }); // Lane bulk actions (archive, move, tag, priority, assign) sit behind each diff --git a/web/e2e/permission-walk-move.spec.ts b/web/e2e/permission-walk-move.spec.ts new file mode 100644 index 000000000..b0532d59d --- /dev/null +++ b/web/e2e/permission-walk-move.spec.ts @@ -0,0 +1,428 @@ +import { test, expect, request, type Locator, type Page } from '@playwright/test'; +import { + actAs, + seedPermissionWalk, + waitForAccessSettled, + type PermissionWalk +} from './lib/permission-walk'; + +/** + * Permission walk, the doors that MOVE a card (BUG-3259). Moving writes the + * card's status, sort_order or role, and each door used to gate on the + * collection (or the parent, or "any edit grant") instead of on the card. + * + * The account is the precedence guest: it may edit Tasks through a collection + * grant, but task A only through an item-VIEW grant, which wins. So A is + * view-only inside a collection the account may edit, and every door must + * refuse to move A while still moving C, which the account may edit. C is the + * in-test control on every leg: an absence on A is evidence only beside the + * same gesture working on C. + * + * Every leg also records the item writes the page sent, and asserts none was + * refused: a 403 is the client sending a write it cannot make. + */ + +test.skip(({ isMobile }) => isMobile, 'the permission walk runs on the desktop project'); + +let walk: PermissionWalk; +// Item ids, to read which card a write names. +const ids: Record<'A' | 'C', string> = { A: '', C: '' }; +const idByTitle = new Map(); + +test.beforeAll(async () => { + walk = await seedPermissionWalk(); + const owner = await request.newContext({ + baseURL: walk.baseURL, + extraHTTPHeaders: { Authorization: `Bearer ${walk.accounts.owner.token}` } + }); + const ws = `/api/v1/workspaces/${walk.workspaceSlug}`; + // Door 7: C is the parent of A (view-only) and B (editable). + for (const child of [walk.grantedTask, walk.tasks[1]]) { + const r = await owner.patch(`${ws}/items/${child.slug}`, { data: { fields_patch: { parent: walk.tasks[2].ref } } }); + expect(r.status(), `parent ${child.ref}`).toBe(200); + } + // Door 8: A and F hold a role, so both sit in a role lane. F is the roles + // legs' own editable card: no other leg moves it, so a board leg cannot + // move it to a status the roles board does not show. + const role = await owner.post(`${ws}/agent-roles`, { data: { name: 'Builder' } }); + expect(role.status()).toBe(201); + const { id: roleId } = (await role.json()) as { id: string }; + const f = await owner.post(`${ws}/collections/tasks/items`, { data: { title: F_TITLE } }); + expect(f.status(), 'create F').toBe(201); + const fItem = (await f.json()) as { id: string; slug: string }; + idByTitle.set(F_TITLE, fItem.id); + for (const slug of [walk.grantedTask.slug, fItem.slug]) { + const r = await owner.patch(`${ws}/items/${slug}`, { data: { agent_role_id: roleId } }); + expect(r.status(), `role ${slug}`).toBe(200); + } + for (const it of [walk.grantedTask, ...walk.tasks]) { + const r = await owner.get(`${ws}/items/${it.slug}`); + idByTitle.set(it.title, ((await r.json()) as { id: string }).id); + } + // Two more editable tasks, so the legs that pick an editable mover always + // have one beside A, with B reserved for the child legs. + for (const title of ['Walk move D', 'Walk move E']) { + const r = await owner.post(`${ws}/collections/tasks/items`, { data: { title } }); + expect(r.status(), `create ${title}`).toBe(201); + idByTitle.set(title, ((await r.json()) as { id: string }).id); + } + ids.A = idByTitle.get(walk.grantedTask.title)!; + ids.C = idByTitle.get(walk.tasks[2].title)!; + await owner.dispose(); +}); + +const A = () => walk.grantedTask.title; +const C = () => walk.tasks[2].title; +// B is C's other child. The child-list legs need it beside A (same status +// group), so no other leg picks it as a card to move. +const B = () => walk.tasks[1].title; +const F_TITLE = 'Walk roles F'; +const F = () => F_TITLE; +const movable = (t: string, allowB = false) => t !== A() && t !== F() && (allowB || t !== B()); + +async function open(page: Page, path: string, view?: 'board' | 'list' | 'table') { + await actAs(page.context(), walk.accounts.guestPrecedence); + if (view) { + await page.context().addInitScript((v) => { + try { + localStorage.setItem('pad-view-tasks', v); + } catch { + // storage unavailable: the collection's default view applies + } + }, view); + } + await page.goto(`${walk.workspacePath}${path}`); + await waitForAccessSettled(page); +} + +// Every write the page sends to the API: item PATCHes, and the lane reorder +// batches (the roles board PUTs role_sort_order for a whole lane, and the +// server refuses the batch if it names a card the caller may only view). +function recordWrites(page: Page): string[] { + const writes: string[] = []; + page.on('response', (r) => { + const method = r.request().method(); + const path = new URL(r.url()).pathname; + if (['PATCH', 'PUT', 'DELETE'].includes(method) && path.startsWith('/api/v1/')) { + writes.push(`${r.status()} ${method} ${path} ${r.request().postData() ?? ''}`); + } + }); + return writes; +} + +const refused = (writes: string[]) => writes.filter((w) => w.startsWith('403 ')); + +/** + * An editable card (any task but A) that sits in a zone with a neighbour, and + * the arrow key that moves it one place: a keyboard move that must COMPLETE, + * so a control proves more than that the drag starts. Legs share one world + * per worker, so the card is chosen from what is on screen now. + */ +async function keyboardControl(page: Page, zoneSel: string, titleSel: string, allowB = false): Promise<{ title: string; step: 'ArrowUp' | 'ArrowDown' }> { + const zones = page.locator(zoneSel); + for (let z = 0; z < (await zones.count()); z++) { + const titles = (await zones.nth(z).locator(titleSel).allTextContents()).map((t) => t.trim()); + if (titles.length < 2) continue; + const i = titles.findIndex((t) => movable(t, allowB)); + if (i >= 0) return { title: titles[i], step: i > 0 ? 'ArrowUp' : 'ArrowDown' }; + } + throw new Error(`no zone matching ${zoneSel} holds an editable card with a neighbour`); +} + +/** The keyboard move completed and persisted: a 200 sort_order write after + * `before`. Not necessarily naming the mover: with a view-only card in the + * zone the plan may keep the mover's value and renumber a neighbour. */ +async function expectMoved(writes: string[], before: number): Promise { + await expect + .poll(() => writes.slice(before).some((w) => w.startsWith('200 ') && w.includes('sort_order')), { timeout: 5_000 }) + .toBe(true); +} + +/** `order` with `mover` swapped one place toward `dir` (-1 up, +1 down). */ +function swapWith(order: string[], mover: string, dir: -1 | 1): string[] { + const i = order.indexOf(mover); + const j = i + dir; + if (i < 0 || j < 0 || j >= order.length) return order; + const next = [...order]; + [next[i], next[j]] = [next[j], next[i]]; + return next; +} + +/** GET a workspace path as the owner, to read stored state the page does not show. */ +async function ownerRead(path: string): Promise { + const owner = await request.newContext({ + baseURL: walk.baseURL, + extraHTTPHeaders: { Authorization: `Bearer ${walk.accounts.owner.token}` } + }); + try { + return await (await owner.get(`/api/v1/workspaces/${walk.workspaceSlug}${path}`)).json(); + } finally { + await owner.dispose(); + } +} + +/** `container` elements whose own `titleSel` element reads exactly `title`. + * A card also shows its PARENT's title in a chip, so hasText would match the + * children of C as C. */ +function titled(page: Page, container: string, titleSel: string, title: string): Locator { + const esc = title.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + return page.locator(container, { has: page.locator(titleSel, { hasText: new RegExp(`^\\s*${esc}\\s*$`) }) }); +} + +/** Whether a pointer drag of `el` starts within three attempts. A gesture can + * fail to start under load (the -collections drag helper retries for the + * same reason), so a control gets three tries and a locked card must refuse + * all three. */ +async function pointerEngages(page: Page, el: Locator): Promise { + for (let attempt = 0; attempt < 3; attempt++) { + if (await pointerEngagesOnce(page, el)) return true; + } + return false; +} + +/** Press on `el`, move far enough to start a drag, and report whether the + * library mounted its dragged element. Releases back where it started. */ +async function pointerEngagesOnce(page: Page, el: Locator): Promise { + await expect(el).toBeVisible(); + // A press needs the card on screen; a long lane pushes it below the fold. + await el.scrollIntoViewIfNeeded(); + const b = (await el.boundingBox())!; + const x = b.x + b.width / 2; + const y = b.y + b.height / 2; + await page.mouse.move(x, y); + await page.mouse.down(); + await page.mouse.move(x + 12, y + 12, { steps: 4 }); + await page.mouse.move(x + 30, y + 30, { steps: 4 }); + const engaged = (await page.locator('#dnd-action-dragged-el').count()) > 0; + // Release OFF the card, in the top bar, outside every drop zone. On a card + // that did not start a drag, a press and release on it is a click, which + // opens the item (the roles board's cards are links). A drag released + // outside every zone returns the card to where it started. + await page.mouse.move(page.viewportSize()!.width / 2, 8, { steps: 4 }); + await page.mouse.up(); + // Let a drop settle before the next gesture. + await expect(page.locator('#dnd-action-dragged-el')).toHaveCount(0); + return engaged; +} + +/** Focus `el` (a zone child) and press Space: whether a keyboard drag began. + * With `step`, the lifted card moves one position before it is dropped. */ +async function keyboardEngages(page: Page, el: Locator, step?: 'ArrowUp' | 'ArrowDown'): Promise { + await el.focus(); + await page.keyboard.press(' '); + const alert = page.locator('#dnd-action-aria-alert'); + const started = await expect(alert) + .toContainText('Started dragging', { timeout: 1_000 }) + .then(() => true) + .catch(() => false); + // Optionally move it one step, then drop it. + if (started && step) await page.keyboard.press(step); + if (started) await page.keyboard.press(' '); + return started; +} + +test.describe('guestPrecedence: a view-only card does not move (BUG-3259)', () => { + test('board: pointer drag', async ({ page }) => { + const writes = recordWrites(page); + await open(page, '/tasks', 'board'); + const card = (t: string) => titled(page, '.column-cards > .card-wrapper', '.card-title', t); + expect(await pointerEngages(page, card(C())), 'control: C drags').toBe(true); + expect(await pointerEngages(page, card(A())), 'A must not drag').toBe(false); + expect(refused(writes)).toEqual([]); + }); + + test('board: keyboard drag (Space on the focused card)', async ({ page }) => { + const writes = recordWrites(page); + await open(page, '/tasks', 'board'); + const card = (t: string) => titled(page, '.column-cards > .card-wrapper', '.card-title', t); + await expect(card(A())).toBeVisible(); + expect(await keyboardEngages(page, card(A())), 'A must not lift by keyboard').toBe(false); + const ctl = await keyboardControl(page, '.column-cards', '.card-title'); + const before = writes.length; + expect(await keyboardEngages(page, card(ctl.title), ctl.step), 'control: an editable card lifts and moves by keyboard').toBe(true); + await expectMoved(writes, before); + expect(refused(writes)).toEqual([]); + }); + + test('list: pointer drag', async ({ page }) => { + const writes = recordWrites(page); + await open(page, '/tasks', 'list'); + const row = (t: string) => titled(page, '.group-items > .list-row', '.card-title', t); + expect(await pointerEngages(page, row(C())), 'control: C drags').toBe(true); + expect(await pointerEngages(page, row(A())), 'A must not drag').toBe(false); + expect(refused(writes)).toEqual([]); + }); + + test('list: keyboard drag (Space on the focused row)', async ({ page }) => { + const writes = recordWrites(page); + await open(page, '/tasks', 'list'); + const row = (t: string) => titled(page, '.group-items > .list-row', '.card-title', t); + await expect(row(A())).toBeVisible(); + expect(await keyboardEngages(page, row(A())), 'A must not lift by keyboard').toBe(false); + const ctl = await keyboardControl(page, '.group-items', '.card-title'); + const before = writes.length; + expect(await keyboardEngages(page, row(ctl.title), ctl.step), 'control: an editable row lifts and moves by keyboard').toBe(true); + await expectMoved(writes, before); + expect(refused(writes)).toEqual([]); + }); + + for (const view of ['board', 'list', 'table'] as const) { + test(`${view}: the card menu's Move entries`, async ({ page }) => { + const writes = recordWrites(page); + await open(page, '/tasks', view); + const holder = (t: string) => + view === 'table' + ? titled(page, '[role=row]', '.title-link', t) + : titled(page, '.item-card', '.card-title', t); + const titleSel = view === 'table' ? '.title-link' : '.card-title'; + await expect(holder(A())).toBeVisible(); + // The control is an editable card in A's own lane or group, so it has + // somewhere to move whatever earlier legs in this world did. Every + // task but A is editable for this account. + const scope = + view === 'board' + ? page.locator('.column-cards', { has: page.locator('.card-title', { hasText: new RegExp(`^\\s*${A()}\\s*$`) }) }) + : view === 'list' + ? page.locator('.group-items', { has: page.locator('.card-title', { hasText: new RegExp(`^\\s*${A()}\\s*$`) }) }) + : page.locator('body'); + const peers = (await scope.locator(titleSel).allTextContents()).map((t) => t.trim()).filter((t) => movable(t)); + expect(peers.length, 'A has an editable neighbour to use as the control').toBeGreaterThan(0); + const control = peers[0]; + await expect(holder(control).locator('.iam-trigger'), 'control: an editable card has the menu').toHaveCount(1); + await expect(holder(A()).locator('.iam-trigger'), 'A has no move menu').toHaveCount(0); + // The control's menu still MOVES it: a named Move entry, and a write + // that names the control lands. Nothing sent is refused. + await holder(control).locator('.iam-trigger').click(); + const move = view === 'board' ? /Move (right|left)/ : /Move (down|up|to top|to bottom)/; + await page.getByRole('menuitem', { name: move }).and(page.locator(':not([disabled])')).first().click(); + const controlId = idByTitle.get(control)!; + await expect + .poll(() => writes.some((w) => w.startsWith('200 ') && w.includes(controlId)), { timeout: 5_000 }) + .toBe(true); + expect(refused(writes)).toEqual([]); + }); + } + + test('board: moving an editable card past a view-only neighbour writes nothing refused', async ({ page }) => { + const writes = recordWrites(page); + await open(page, '/tasks', 'board'); + // Put C in A's lane, directly above it, through the card menu: the lane + // reorder then covers A, which it used to renumber (a 403). + const lanes = page.locator('.column-cards'); + const laneOf = async (t: string) => { + for (let i = 0; i < (await lanes.count()); i++) { + if ((await lanes.nth(i).locator('.card-title', { hasText: new RegExp(`^\\s*${t}\\s*$`) }).count()) > 0) return i; + } + return -1; + }; + const card = (t: string) => titled(page, '.item-card', '.card-title', t); + await expect(card(A())).toBeVisible(); + await expect(card(C())).toBeVisible(); + expect(await laneOf(A()), "A's lane").toBeGreaterThanOrEqual(0); + for (let guard = 0; guard < 4 && (await laneOf(C())) !== (await laneOf(A())); guard++) { + await card(C()).locator('.iam-trigger').click(); + const dir = (await laneOf(C())) < (await laneOf(A())) ? 'Move right' : 'Move left'; + await page.getByRole('menuitem', { name: new RegExp(dir) }).click(); + await expect(page.locator('.iam-trigger[aria-expanded="true"]')).toHaveCount(0); + } + expect(await laneOf(C())).toBe(await laneOf(A())); + // Pick a card-menu move of an editable card in A's lane that gives A a + // new index DIFFERENT from A's stored sort_order: the old renumber + // wrote exactly those neighbours, so this is the move that used to send + // A's refused write. (Earlier legs in this world leave A's stored value + // wherever they left it, so the mover is not always C.) + const lane = lanes.nth(await laneOf(A())); + const order = (await lane.locator('.card-title').allTextContents()).map((t) => t.trim()); + const storedA = await ownerRead(`/items/${walk.grantedTask.slug}`).then((r) => (r as { sort_order: number }).sort_order); + const moves: [RegExp, (o: string[], m: string) => string[]][] = [ + [/Move to top/, (o, m) => [m, ...o.filter((t) => t !== m)]], + [/Move to bottom/, (o, m) => [...o.filter((t) => t !== m), m]], + [/Move up/, (o, m) => swapWith(o, m, -1)], + [/Move down/, (o, m) => swapWith(o, m, 1)] + ]; + const candidates = order + .filter((t) => movable(t)) + .flatMap((mover) => moves.map(([name, f]) => ({ mover, name, next: f(order, mover) }))); + const pick = candidates.find(({ next }) => next.join() !== order.join() && next.indexOf(A()) !== storedA); + expect(pick, `no move changes A's index away from its stored ${storedA} (lane ${order.join(', ')})`).toBeTruthy(); + // Only writes the final move sends count: the placement moves above wrote too. + const before = writes.length; + const moverId = idByTitle.get(pick!.mover)!; + await card(pick!.mover).locator('.iam-trigger').click(); + await page.getByRole('menuitem', { name: pick!.name }).click(); + const moverLanded = () => writes.slice(before).some((w) => w.startsWith('200 ') && w.includes('sort_order') && w.includes(moverId)); + // Settle on whichever comes first: the mover's write, or a refusal (the + // old loop stopped at A's 403, so the mover's write never came). + await expect.poll(() => moverLanded() || refused(writes).length > 0, { timeout: 5_000 }).toBe(true); + expect(refused(writes), 'a write the account cannot make was sent').toEqual([]); + expect(moverLanded(), 'the move persisted').toBe(true); + }); + + test('children (door 7): drag and the Move menu on a view-only child', async ({ page }) => { + const writes = recordWrites(page); + await open(page, `/tasks/${walk.tasks[2].slug}`); + await page.getByRole('tab', { name: 'Relationships' }).click(); + const child = (t: string) => titled(page, '.child-list > .child-item-wrapper', '.child-title', t); + await expect(child(A())).toBeVisible(); + await expect(child(B()).locator('.iam-trigger'), 'control: B has the menu').toHaveCount(1); + await expect(child(A()).locator('.iam-trigger'), 'A has no move menu').toHaveCount(0); + expect(await pointerEngages(page, child(B()).locator('.child-row')), 'control: B drags').toBe(true); + expect(await pointerEngages(page, child(A()).locator('.child-row')), 'A must not drag').toBe(false); + // The control's menu still MOVES it: a write naming B lands. + const before = writes.length; + await child(B()).locator('.iam-trigger').click(); + await page.getByRole('menuitem', { name: /Move (down|up|to top|to bottom)/ }).and(page.locator(':not([disabled])')).first().click(); + const bId = idByTitle.get(B())!; + await expect.poll(() => writes.slice(before).some((w) => w.startsWith('200 ') && w.includes(bId)), { timeout: 5_000 }).toBe(true); + expect(refused(writes)).toEqual([]); + }); + + test('children (door 7): keyboard drag (Space on the focused child)', async ({ page }) => { + const writes = recordWrites(page); + await open(page, `/tasks/${walk.tasks[2].slug}`); + await page.getByRole('tab', { name: 'Relationships' }).click(); + const child = (t: string) => titled(page, '.child-list > .child-item-wrapper', '.child-title', t); + await expect(child(A())).toBeVisible(); + expect(await keyboardEngages(page, child(A())), 'A must not lift by keyboard').toBe(false); + const ctl = await keyboardControl(page, '.child-list', '.child-title', true); + const before = writes.length; + expect(await keyboardEngages(page, child(ctl.title), ctl.step), 'control: an editable child lifts and moves by keyboard').toBe(true); + await expectMoved(writes, before); + expect(refused(writes)).toEqual([]); + }); + + test('roles board: dropping an editable card in a lane with a view-only card writes nothing refused', async ({ page }) => { + const writes = recordWrites(page); + await open(page, '/roles'); + const card = (t: string) => titled(page, '.lane-items > .card-wrapper', '.card-title', t); + // A and F share the Builder lane. Moving F past A renumbers the lane, + // which used to name A in the batch and have the WHOLE batch refused. + // By keyboard, so the move is deterministic: lift F, one step toward A, + // drop. + const lane = page.locator('.lane-items', { has: page.locator('.card-title', { hasText: new RegExp(`^\\s*${A()}\\s*$`) }) }); + // allTextContents does not wait: the board must have rendered both cards. + await expect(card(A())).toBeVisible(); + await expect(card(F())).toBeVisible(); + const titles = (await lane.locator('.card-title').allTextContents()).map((t) => t.trim()); + const [ia, ic] = [titles.indexOf(A()), titles.indexOf(F())]; + expect(ia, 'A is in the lane').toBeGreaterThanOrEqual(0); + expect(ic, 'F shares A\'s lane').toBeGreaterThanOrEqual(0); + expect(await keyboardEngages(page, card(F()), ic > ia ? 'ArrowUp' : 'ArrowDown'), 'control: F lifts').toBe(true); + const reorder = () => writes.find((w) => w.includes(' PUT ') && w.includes('/roles/board/reorder')); + const landed = () => writes.find((w) => w.startsWith('200 PUT ') && w.includes('/roles/board/reorder')); + await expect.poll(reorder, { timeout: 5_000 }).toBeTruthy(); + expect(reorder(), 'the batch names F').toContain(idByTitle.get(F())!); + expect(reorder(), 'the batch must not name view-only A').not.toContain(ids.A); + expect(landed(), 'the reorder persisted (200)').toBeTruthy(); + expect(refused(writes)).toEqual([]); + }); + + test('roles board (door 8): drag a view-only card', async ({ page }) => { + const writes = recordWrites(page); + await open(page, '/roles'); + const card = (t: string) => titled(page, '.lane-items > .card-wrapper', '.card-title', t); + expect(await pointerEngages(page, card(F())), 'control: F drags').toBe(true); + expect(await pointerEngages(page, card(A())), 'A must not drag').toBe(false); + expect(refused(writes)).toEqual([]); + }); +}); diff --git a/web/src/lib/collections/lockableDndzone.ts b/web/src/lib/collections/lockableDndzone.ts new file mode 100644 index 000000000..21624df98 --- /dev/null +++ b/web/src/lib/collections/lockableDndzone.ts @@ -0,0 +1,91 @@ +import { dndzone, DRAGGED_ELEMENT_ID, type DndEvent, type Item as DndItem, type Options } from 'svelte-dnd-action'; +import type { ActionReturn } from 'svelte/action'; + +/** + * `dndzone` with per-card locking (BUG-3259). + * + * svelte-dnd-action disables drag per ZONE only. A zone child (the element + * rendered for one item) carrying `data-drag-locked="true"` cannot start a + * drag here, by pointer or by keyboard, while its unlocked siblings still can. + * The board and list lock a card the caller may only view, which an item grant + * makes possible inside a collection the caller may edit. + * + * Mechanism: the library listens for mousedown / touchstart / keydown on each + * zone child. A capture-phase listener on the zone runs before those, and when + * the gesture starts on a locked child it reconfigures the zone as disabled + * for that one event, synchronously, which removes the library's listeners + * from every child. The event then reaches the child with no drag listener on + * it, and the zone is re-enabled on the next task. This is the library's own + * drag-handle mechanism (`dragHandleZone`), scoped to this zone instead of a + * module-wide store. + * + * The gesture then stops at this zone, in the bubble phase, exactly where the + * library stops it for an unlocked card (its handlers call stopPropagation). + * Without that, a zone nested in another zone hands the gesture to the outer + * one: on the list, pressing a locked row dragged its whole GROUP (measured). + * Listeners above the zone see what they already saw for an unlocked card. + */ +export function lockableDndzone( + node: HTMLElement, + options: Options +): ActionReturn, { onconsider?: (e: CustomEvent>) => void; onfinalize?: (e: CustomEvent>) => void }> { + let current = options; + let suspended = false; + let destroyed = false; + const zone = dndzone(node, current); + + function lockedChild(target: EventTarget | null): boolean { + let el = target instanceof Element ? target : null; + while (el && el.parentElement !== node) el = el.parentElement; + return el instanceof HTMLElement && el.dataset.dragLocked === 'true'; + } + + // A gesture this zone would have started on an unlocked card: a pointer + // press, or a key that starts a keyboard drag (the library's defaults), on + // a locked child, with no pointer drag in progress (reconfiguring mid-drag + // would disturb its shadow item). + function lockedGesture(e: Event): boolean { + if (current.dragDisabled) return false; + if (e instanceof KeyboardEvent && e.key !== 'Enter' && e.key !== ' ') return false; + if (document.getElementById(DRAGGED_ELEMENT_ID)) return false; + return lockedChild(e.target); + } + + function stopAtZone(e: Event) { + if (lockedGesture(e)) e.stopPropagation(); + } + + function gate(e: Event) { + if (suspended || !lockedGesture(e)) return; + suspended = true; + zone.update?.({ ...current, dragDisabled: true }); + setTimeout(() => { + suspended = false; + if (!destroyed) zone.update?.(current); + }, 0); + } + + node.addEventListener('mousedown', gate, true); + node.addEventListener('touchstart', gate, { capture: true, passive: true }); + node.addEventListener('keydown', gate, true); + node.addEventListener('mousedown', stopAtZone); + node.addEventListener('touchstart', stopAtZone, { passive: true }); + node.addEventListener('keydown', stopAtZone); + + return { + update(next: Options) { + current = next; + if (!suspended) zone.update?.(current); + }, + destroy() { + destroyed = true; + node.removeEventListener('mousedown', gate, true); + node.removeEventListener('touchstart', gate, true); + node.removeEventListener('keydown', gate, true); + node.removeEventListener('mousedown', stopAtZone); + node.removeEventListener('touchstart', stopAtZone); + node.removeEventListener('keydown', stopAtZone); + zone.destroy?.(); + } + }; +} diff --git a/web/src/lib/collections/reorder.test.ts b/web/src/lib/collections/reorder.test.ts index 59063f100..0e2e983b4 100644 --- a/web/src/lib/collections/reorder.test.ts +++ b/web/src/lib/collections/reorder.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from 'vitest'; import type { Item } from '$lib/types'; -import { reorderGroup, reorderedList, disabledDirections, adjacentColumn } from './reorder'; +import { reorderGroup, reorderedList, disabledDirections, adjacentColumn, laneOrderAfterMove } from './reorder'; // Minimal Item stand-in — reorder only touches `id` and `sort_order`. function item(id: string, sort_order: number): Item { @@ -133,3 +133,20 @@ describe('adjacentColumn', () => { expect(adjacentColumn(cols, 'archived', 'right')).toBeNull(); }); }); + +describe('laneOrderAfterMove (BUG-3259)', () => { + const mk = (id: string, sort_order: number) => ({ id, sort_order }) as unknown as Item; + it('hands over EVERY row in the new order, unchanged ones included', () => { + const lane = [mk('a', 0), mk('b', 1), mk('c', 2), mk('d', 3)]; + expect(laneOrderAfterMove(lane, 'c', 'up')).toEqual([ + { slug: 'a', sort_order: 0 }, + { slug: 'c', sort_order: 1 }, + { slug: 'b', sort_order: 2 }, + { slug: 'd', sort_order: 3 } + ]); + }); + it('is empty for a no-op move', () => { + const lane = [mk('a', 0), mk('b', 1)]; + expect(laneOrderAfterMove(lane, 'a', 'top')).toEqual([]); + }); +}); diff --git a/web/src/lib/collections/reorder.ts b/web/src/lib/collections/reorder.ts index 5ce4002ca..91da5269f 100644 --- a/web/src/lib/collections/reorder.ts +++ b/web/src/lib/collections/reorder.ts @@ -126,3 +126,22 @@ export function disabledDirections(index: number, length: number): Set ({ slug: item.id, sort_order: index })); +} diff --git a/web/src/lib/collections/reorderPlan.test.ts b/web/src/lib/collections/reorderPlan.test.ts new file mode 100644 index 000000000..5c8dcd615 --- /dev/null +++ b/web/src/lib/collections/reorderPlan.test.ts @@ -0,0 +1,161 @@ +import { describe, expect, it } from 'vitest'; +import { planLaneOrder, persistReorder, type OrderedCard, type OrderWrite } from './reorderPlan'; + +const c = (id: string, sort_order: number): OrderedCard => ({ id, sort_order }); + +// The stored order a plan produces, read the way the lane sorts it: by value, +// frozen ties keeping their on-screen order. +function sortedAfter(lane: OrderedCard[], writes: OrderWrite[]): string[] { + const value = new Map(lane.map((x) => [x.id, x.sort_order])); + for (const w of writes) value.set(w.id, w.sort_order); + return lane + .map((x, i) => ({ id: x.id, v: value.get(x.id)!, i })) + .sort((a, b) => a.v - b.v || a.i - b.i) + .map((x) => x.id); +} + +describe('planLaneOrder (BUG-3259)', () => { + it('renumbers densely when every card is editable, writing only changed cards', () => { + const lane = [c('c', 2), c('a', 0), c('b', 1)]; + const plan = planLaneOrder(lane, () => true); + expect(plan).toEqual({ + ok: true, + writes: [ + { id: 'c', sort_order: 0 }, + { id: 'a', sort_order: 1 }, + { id: 'b', sort_order: 2 } + ] + }); + }); + + it('never writes a view-only card', () => { + // The measured case: C dragged above view-only A. + const lane = [c('c', 2), c('a', 0), c('b', 1)]; + const plan = planLaneOrder(lane, (x) => x.id !== 'a'); + expect(plan.ok).toBe(true); + if (!plan.ok) return; + expect(plan.writes.map((w) => w.id)).not.toContain('a'); + expect(sortedAfter(lane, plan.writes)).toEqual(['c', 'a', 'b']); + }); + + it('a dense renumber that skipped the frozen card would collide; the plan does not', () => { + // C moved to the top, A (frozen) keeps 0. Dense-minus-frozen would write + // C=0, B=2: C and A tie at 0 and the tie-break decides, not the user. + const lane = [c('c', 2), c('a', 0), c('b', 1)]; + const plan = planLaneOrder(lane, (x) => x.id !== 'a'); + if (!plan.ok) throw new Error('expected a plan'); + const v = new Map(lane.map((x) => [x.id, x.sort_order])); + for (const w of plan.writes) v.set(w.id, w.sort_order); + expect(v.get('c')!).toBeLessThan(v.get('a')!); + expect(v.get('a')!).toBeLessThan(v.get('b')!); + }); + + it('keeps an editable card whose value already fits', () => { + const lane = [c('a', 0), c('x', 5), c('b', 9)]; + const plan = planLaneOrder(lane, (x) => x.id === 'x'); + expect(plan).toEqual({ ok: true, writes: [] }); + }); + + it('refuses when there is no integer room between frozen cards', () => { + const lane = [c('a', 0), c('x', 7), c('b', 1)]; + expect(planLaneOrder(lane, (x) => x.id === 'x')).toEqual({ ok: false }); + }); + + it('allows two frozen cards to tie when nothing moved between them', () => { + const lane = [c('x', 3), c('a', 0), c('b', 0)]; + const plan = planLaneOrder(lane, (x) => x.id === 'x'); + expect(plan.ok).toBe(true); + if (!plan.ok) return; + expect(sortedAfter(lane, plan.writes)).toEqual(['x', 'a', 'b']); + }); + + it('refuses a frozen card that would have to sort before what precedes it', () => { + const lane = [c('b', 4), c('a', 1)]; + expect(planLaneOrder(lane, () => false)).toEqual({ ok: false }); + }); + + // Exhaustive over small lanes: every plan that succeeds sorts to the + // on-screen order and never writes a frozen card. + it('every successful plan sorts to the on-screen order (exhaustive, lanes up to 5)', () => { + let checked = 0; + for (let n = 1; n <= 5; n++) { + const perms = permutations([...Array(n).keys()]); + for (const order of perms) { + for (let mask = 0; mask < 1 << n; mask++) { + // Stored values are the ORIGINAL dense order; `order` is the new screen order. + const lane = order.map((orig) => c(`i${orig}`, orig)); + const frozen = (x: OrderedCard) => (mask >> Number(x.id.slice(1))) & 1; + const plan = planLaneOrder(lane, (x) => !frozen(x)); + if (!plan.ok) continue; + checked++; + for (const w of plan.writes) expect(frozen(lane.find((x) => x.id === w.id)!)).toBe(0); + expect(sortedAfter(lane, plan.writes)).toEqual(lane.map((x) => x.id)); + } + } + } + expect(checked).toBeGreaterThan(1000); + }); +}); + +function permutations(xs: number[]): number[][] { + if (xs.length <= 1) return [xs]; + return xs.flatMap((x, i) => permutations([...xs.slice(0, i), ...xs.slice(i + 1)]).map((p) => [x, ...p])); +} + +describe('persistReorder (BUG-3259)', () => { + function harness(refuse: Set) { + const originals = new Map([ + ['a', { id: 'a', sort_order: 0, seq: 10 }], + ['b', { id: 'b', sort_order: 1, seq: 11 }], + ['c', { id: 'c', sort_order: 2, seq: 12 }] + ]); + const local = new Map([...originals].map(([k, v]) => [k, { ...v }])); + const sent: string[] = []; + return { + local, + sent, + deps: { + original: (id: string) => originals.get(id), + applyLocal: (card: { id: string; sort_order: number; seq?: number }) => + local.set(card.id, { ...card, seq: undefined as unknown as number }), + restoreLocal: (card: { id: string; sort_order: number; seq: number }) => local.set(card.id, { ...card }), + send: async (w: OrderWrite) => { + sent.push(w.id); + if (refuse.has(w.id)) throw new Error('403'); + return { ...originals.get(w.id)!, sort_order: w.sort_order, seq: originals.get(w.id)!.seq + 1 }; + }, + settle: (row: { id: string; sort_order: number; seq: number }) => { + local.set(row.id, row); + return true; + } + } + }; + } + const writes = [ + { id: 'c', sort_order: 0 }, + { id: 'a', sort_order: 1 }, + { id: 'b', sort_order: 2 } + ]; + + it('persists every write in order', async () => { + const h = harness(new Set()); + expect(await persistReorder(writes, h.deps)).toBe(true); + expect(h.sent).toEqual(['c', 'a', 'b']); + expect([...h.local.values()].map((r) => [r.id, r.sort_order])).toEqual([ + ['a', 1], + ['b', 2], + ['c', 0] + ]); + }); + + it('a mid-loop refusal restores the refused card and every card after it', async () => { + const h = harness(new Set(['a'])); + expect(await persistReorder(writes, h.deps)).toBe(false); + // Stopped at the refusal: b was never sent. + expect(h.sent).toEqual(['c', 'a']); + // c landed and keeps its server row; a and b are back to their originals. + expect(h.local.get('c')).toEqual({ id: 'c', sort_order: 0, seq: 13 }); + expect(h.local.get('a')).toEqual({ id: 'a', sort_order: 0, seq: 10 }); + expect(h.local.get('b')).toEqual({ id: 'b', sort_order: 1, seq: 11 }); + }); +}); diff --git a/web/src/lib/collections/reorderPlan.ts b/web/src/lib/collections/reorderPlan.ts new file mode 100644 index 000000000..8e6afc64f --- /dev/null +++ b/web/src/lib/collections/reorderPlan.ts @@ -0,0 +1,129 @@ +// Lane reorder planning and persistence (BUG-3259). +// +// A drag or a card-menu move hands over a whole lane in its new on-screen +// order. Every card in it gets a `sort_order` (a dense INTEGER per lane, ties +// broken by created_at), so moving one card renumbers its neighbours. A +// neighbour the caller may only VIEW (an item grant beats a collection grant) +// cannot be written: the server refuses it 403. +// +// `planLaneOrder` decides which writes to send so that the stored order sorts +// to what the user sees, leaving view-only cards where they are. +// `persistReorder` sends them and undoes its optimistic local writes when one +// is refused. + +export interface OrderedCard { + id: string; + sort_order: number; +} + +export interface OrderWrite { + id: string; + sort_order: number; +} + +export type LanePlan = { ok: true; writes: OrderWrite[] } | { ok: false }; + +/** + * Plan the `sort_order` writes for a lane given in its new on-screen order. + * + * With every card editable this is the dense renumber the views have always + * sent (index = sort_order), minus cards already there, so nothing changes for + * an account without a view-only neighbour. + * + * With a frozen (view-only) card present, frozen cards keep their stored value + * and each editable card gets a value strictly between the previous card's and + * the next frozen card's, keeping its own value when that already fits. The + * values are then strictly increasing along the lane, except that two frozen + * cards with nothing moved between them may tie, which is the order the + * tie-break already showed. Returns `{ ok: false }` when there is no integer + * room, for example an editable card dropped between frozen cards at 0 and 1. + */ +export function planLaneOrder( + lane: readonly T[], + canEdit: (card: T) => boolean +): LanePlan { + const editable = lane.map((c) => canEdit(c)); + if (editable.every(Boolean)) { + const writes = lane.flatMap((c, i) => (c.sort_order === i ? [] : [{ id: c.id, sort_order: i }])); + return { ok: true, writes }; + } + + const writes: OrderWrite[] = []; + // The value assigned to the previous card, and whether it was frozen. + let prev = -Infinity; + let prevFrozen = false; + for (let k = 0; k < lane.length; k++) { + const card = lane[k]; + if (!editable[k]) { + const v = card.sort_order; + // A frozen card must not sort before what precedes it. It may tie only + // with a frozen card directly before it (nothing moved between them). + if (v < prev || (v === prev && !prevFrozen)) return { ok: false }; + prev = v; + prevFrozen = true; + continue; + } + // Room: strictly below the next frozen card, leaving one value for each + // editable card between this one and it. + let upper = Infinity; + for (let j = k + 1; j < lane.length; j++) { + if (!editable[j]) { + upper = lane[j].sort_order - (j - k); + break; + } + } + const lower = prev + 1; + if (lower > upper) return { ok: false }; + let v: number; + if (card.sort_order >= lower && card.sort_order <= upper) v = card.sort_order; + else if (lower === -Infinity) v = Math.min(card.sort_order, upper); + else v = lower; + if (v !== card.sort_order) writes.push({ id: card.id, sort_order: v }); + prev = v; + prevFrozen = false; + } + return { ok: true, writes }; +} + +export interface PersistDeps { + /** The card as it stands before the reorder, or undefined if unknown. */ + original: (id: string) => T | undefined; + /** Write the optimistic row locally, before the request. */ + applyLocal: (card: T) => void; + /** Put the original row back locally after a refusal. */ + restoreLocal: (card: T) => void; + /** Persist one write; resolves to the server's row. */ + send: (write: OrderWrite) => Promise; + /** Settle the server's row locally. Returns false to stop (identity changed). */ + settle: (row: T) => boolean; +} + +/** + * Apply `writes` optimistically, then persist them one at a time (SQLite takes + * one writer). A refusal stops the loop and restores every card not yet + * confirmed, the refused one included, to its original row, so the local order + * never shows a move the server did not store. Returns whether all landed. + */ +export async function persistReorder( + writes: readonly OrderWrite[], + deps: PersistDeps +): Promise { + const pending: { write: OrderWrite; original: T }[] = []; + for (const write of writes) { + const original = deps.original(write.id); + if (!original) continue; + deps.applyLocal({ ...original, sort_order: write.sort_order }); + pending.push({ write, original }); + } + for (let i = 0; i < pending.length; i++) { + let row: T; + try { + row = await deps.send(pending[i].write); + } catch { + for (const { original } of pending.slice(i)) deps.restoreLocal(original); + return false; + } + if (!deps.settle(row)) return false; + } + return true; +} diff --git a/web/src/lib/components/ChildItems.svelte b/web/src/lib/components/ChildItems.svelte index f8e246c43..133e1a7f0 100644 --- a/web/src/lib/components/ChildItems.svelte +++ b/web/src/lib/components/ChildItems.svelte @@ -15,10 +15,11 @@ import { collectionsNotStaleFor, categoricalValueFor } from '$lib/collections/categoricalFieldValue'; import { laneKey } from '$lib/collections/boardColumns'; import { fieldMatches, safeText } from '$lib/fields/fieldShape'; - import { dndzone, TRIGGERS, SHADOW_ITEM_MARKER_PROPERTY_NAME } from 'svelte-dnd-action'; + import { TRIGGERS, SHADOW_ITEM_MARKER_PROPERTY_NAME } from 'svelte-dnd-action'; + import { lockableDndzone } from '$lib/collections/lockableDndzone'; + import { planLaneOrder, persistReorder } from '$lib/collections/reorderPlan'; import type { DndEvent } from 'svelte-dnd-action'; import { - reorderGroup, reorderedList, disabledDirections, type ReorderDirection @@ -199,39 +200,48 @@ // stays (the zone gate mirror; a non-editor never reaches finalize anyway). if (!canEdit) return; - const updates = groupData[laneKey(status)] - .filter((i: any) => !i[SHADOW_ITEM_MARKER_PROPERTY_NAME]) - .map((item, index) => ({ id: item.id, sort_order: index })); + await persistChildOrder( + groupData[laneKey(status)].filter((i: any) => !i[SHADOW_ITEM_MARKER_PROPERTY_NAME]) + ); + } - // IDENTITY fence (BUG-3095) + the workspace this reorder is about, - // both captured before the loop. `wsSlug` was read LIVE inside the loop - // before this change, so a workspace switch mid-loop addressed the - // remaining PATCHes at the new workspace. + // Persist a status group's new order (BUG-3259). A child the caller may only + // VIEW (an item grant beats the parent's permission, which is what `canEdit` + // is) cannot be renumbered, so the plan leaves it where it is and fits the + // others around it, writing only children whose value changes. On a refusal, + // or no room to keep the order, the canonical children are reloaded so the + // group never shows an order the server did not store. + async function persistChildOrder(lane: Item[]) { + const plan = planLaneOrder(lane, (c) => workspaceStore.canEditItem(c)); + if (!plan.ok) { + toastStore.show("Couldn't keep that order: an item you can only view is in the way.", 'error'); + void loadChildren(); + return; + } + // IDENTITY fence (BUG-3095) + the workspace this reorder is about, both + // captured before the loop. `wsSlug` was read LIVE inside the loop before + // that change, so a workspace switch mid-loop addressed the remaining + // PATCHes at the new workspace. + // + // HT-2176 Option A (TASK-2172): NO per-PATCH freeze recheck. A reorder + // INITIATED before peeking finishes; breaking mid-loop would persist it + // only partially. IDENTITY is checked per write even though FREEZE is not + // (BUG-3095): a sign-out mid-loop would otherwise send the remaining + // writes on the next user's cookie. const isSameIdentity = authStore.identityFence(); const reqWs = wsSlug; - try { - for (const { id, sort_order } of updates) { - // HT-2176 Option A (TASK-2172): NO per-PATCH freeze recheck. The - // reorder was INITIATED before peeking (the top guard blocks a NEW - // one); breaking mid-loop would persist it only partially, leaving - // inconsistent sort_orders. Let the initiated reorder finish. - // - // IDENTITY is checked per iteration even though FREEZE is not - // (BUG-3095), and the two are not in tension. Option A's argument - // is that a half-applied reorder is worse than a fully-applied one - // the user no longer has permission to make — true while the - // SENDER is the same person throughout. It says nothing about a - // DIFFERENT user: every iteration after the first sends after the - // previous iteration's await, so a sign-out mid-loop would issue - // the remaining writes on the next user's cookie. A partial - // reorder is the lesser harm there, and it is the only outcome - // that keeps the writes attributable to whoever started them. - if (!isSameIdentity()) return; - await api.items.update(reqWs, id, { sort_order }); - } - } catch (e) { - console.error('Failed to persist reorder:', e); - } + const landed = await persistReorder(plan.writes, { + original: (id) => lane.find((c) => c.id === id), + // This component shows the new order from `groupData` already. + applyLocal: () => {}, + restoreLocal: () => {}, + send: async ({ id, sort_order }) => { + if (!isSameIdentity()) throw new Error('identity changed'); + return api.items.update(reqWs, id, { sort_order }); + }, + settle: () => isSameIdentity() + }); + if (!landed && isSameIdentity()) void loadChildren(); } // Menu-driven reorder (IDEA-1898) — the non-drag counterpart, scoped to @@ -250,26 +260,9 @@ const reordered = reorderedList(grp, child.id, dir); if (reordered === grp) return; // no-op (edge of group) - // Optimistic: show the new order immediately with dense sort_order. - groupData[laneKey(status)] = reordered.map((it, idx) => ({ ...it, sort_order: idx })); - - const updates = reorderGroup(grp, child.id, dir); - // IDENTITY fence + captured workspace (BUG-3095) — see handleFinalize - // for why the loop needs a per-iteration check and why that does not - // reopen Option A's partial-reorder argument. - const isSameIdentity = authStore.identityFence(); - const reqWs = wsSlug; - try { - for (const u of updates) { - // Option A (TASK-2172): no per-PATCH freeze recheck — a reorder - // initiated pre-pane finishes fully (see handleFinalize). Identity - // IS checked per iteration even though freeze is not (BUG-3095). - if (!isSameIdentity()) return; - await api.items.update(reqWs, u.item.id, { sort_order: u.sort_order }); - } - } catch (e) { - console.error('Failed to persist reorder:', e); - } + // Optimistic: show the new order immediately. + groupData[laneKey(status)] = reordered; + await persistChildOrder(reordered); } // ── Data loading ───────────────────────────────────────────────────────── @@ -954,7 +947,7 @@
{formatLabel(status)} ({(groupData[laneKey(status)] ?? []).length})
fieldMatches(fields.status, t))} {@const isExpanded = expandedIds.has(child.id)} {@const canExpand = child.has_children} -
+ +
{#if canExpand}