mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
fix(web): a card the caller may only view cannot be moved from any door: board, list, table and child menus, drag by pointer or keyboard, the roles board, and the lane renumber (BUG-3259) (#1630)
An item grant beats a collection grant, so a guest may edit a collection and still only view one of its cards. Eight doors moved such a card, or renumbered it as a neighbour, gated on the collection, the parent, or "any edit grant", and the server refused each write. - lockableDndzone: dndzone with per-card locking (data-drag-locked). A capture listener disables the zone for one gesture on a locked card (the library's drag-handle mechanism, per zone), and the gesture stops at the zone as the library stops it for an unlocked card, so a nested outer zone (the list's groups) does not take it. - Board, List, ChildItems and the roles board lock !canEditItem cards; the Move menus render only for canEditItem cards. - planLaneOrder plans a lane's sort_order writes so view-only cards keep their value and the stored order sorts to the screen; with every card editable it is the dense renumber as before. No room: nothing written. - persistReorder restores every unconfirmed card on a refusal, under the identity that made the request. - The menu paths hand onReorder the whole lane (laneOrderAfterMove). Claude-Session: https://claude.ai/code/session_01NidgPePY7UvHcEJCMkD2Qz
This commit is contained in:
@@ -306,7 +306,7 @@ async function dragToLane(page: Page, title: string, lane: string): Promise<Drag
|
||||
// A cross-lane drop writes the card's STATUS, and may also write a
|
||||
// neighbour's sort_order. Wait for the status write itself: waiting for
|
||||
// the first write returned on the neighbour's, before the status write
|
||||
// landed (a CI flake on the BUG-3259 pin).
|
||||
// landed (a CI flake on the BUG-3259 leg).
|
||||
await expect
|
||||
.poll(() => 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
|
||||
|
||||
@@ -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<string, string>();
|
||||
|
||||
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<void> {
|
||||
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<unknown> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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([]);
|
||||
});
|
||||
});
|
||||
@@ -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<T extends DndItem>(
|
||||
node: HTMLElement,
|
||||
options: Options<T>
|
||||
): ActionReturn<Options<T>, { onconsider?: (e: CustomEvent<DndEvent<T>>) => void; onfinalize?: (e: CustomEvent<DndEvent<T>>) => 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<T>) {
|
||||
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?.();
|
||||
}
|
||||
};
|
||||
}
|
||||
@@ -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([]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -126,3 +126,22 @@ export function disabledDirections(index: number, length: number): Set<ReorderDi
|
||||
}
|
||||
return disabled;
|
||||
}
|
||||
|
||||
/**
|
||||
* The whole group in its order after moving `itemId` per `dir`, as dense
|
||||
* indices: every row, not only the ones whose index changed. Empty when the
|
||||
* move is a no-op.
|
||||
*
|
||||
* `onReorder` takes the whole lane (BUG-3259): it plans the writes around cards
|
||||
* the caller may only view, which it can only do if it sees every card. The
|
||||
* drag paths already hand over the whole lane; the menu paths use this.
|
||||
*/
|
||||
export function laneOrderAfterMove(
|
||||
ordered: Item[],
|
||||
itemId: string,
|
||||
dir: ReorderDirection
|
||||
): { slug: string; sort_order: number }[] {
|
||||
const next = reorderedList(ordered, itemId, dir);
|
||||
if (next === ordered) return [];
|
||||
return next.map((item, index) => ({ slug: item.id, sort_order: index }));
|
||||
}
|
||||
|
||||
@@ -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<string>) {
|
||||
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 });
|
||||
});
|
||||
});
|
||||
@@ -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<T extends OrderedCard>(
|
||||
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<T extends OrderedCard> {
|
||||
/** 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<T>;
|
||||
/** 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<T extends OrderedCard>(
|
||||
writes: readonly OrderWrite[],
|
||||
deps: PersistDeps<T>
|
||||
): Promise<boolean> {
|
||||
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;
|
||||
}
|
||||
@@ -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<Item>(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 @@
|
||||
<div class="group-label">{formatLabel(status)} ({(groupData[laneKey(status)] ?? []).length})</div>
|
||||
<div
|
||||
class="child-list"
|
||||
use:dndzone={{
|
||||
use:lockableDndzone={{
|
||||
items: groupData[laneKey(status)] ?? [],
|
||||
flipDurationMs,
|
||||
type: 'child-item',
|
||||
@@ -982,7 +975,13 @@
|
||||
{@const isDone = terminal.some((t) => fieldMatches(fields.status, t))}
|
||||
{@const isExpanded = expandedIds.has(child.id)}
|
||||
{@const canExpand = child.has_children}
|
||||
<div class="child-item-wrapper">
|
||||
<!-- data-drag-locked: a child the caller may only view cannot be
|
||||
dragged, though the list allows it (BUG-3259). -->
|
||||
<div
|
||||
class="child-item-wrapper"
|
||||
class:drag-locked={!workspaceStore.canEditItem(child)}
|
||||
data-drag-locked={!workspaceStore.canEditItem(child)}
|
||||
>
|
||||
<div class="child-row-container">
|
||||
{#if canExpand}
|
||||
<button class="expand-toggle" onclick={(e) => { e.preventDefault(); toggleExpand(child); }} title={isExpanded ? 'Collapse' : 'Expand'}>
|
||||
@@ -1002,7 +1001,9 @@
|
||||
</span>
|
||||
{/if}
|
||||
</a>
|
||||
{#if canEdit && !frozen}
|
||||
<!-- Per child as well (BUG-3259): an item grant can make a
|
||||
child view-only under a parent the caller may edit. -->
|
||||
{#if canEdit && !frozen && workspaceStore.canEditItem(child)}
|
||||
<ItemActionsMenu
|
||||
item={child}
|
||||
label={child.title}
|
||||
@@ -1188,6 +1189,12 @@
|
||||
cursor: grabbing;
|
||||
}
|
||||
|
||||
/* A child the caller may only view does not drag (BUG-3259). */
|
||||
.child-item-wrapper.drag-locked .child-row,
|
||||
.child-item-wrapper.drag-locked .child-row:active {
|
||||
cursor: pointer;
|
||||
}
|
||||
|
||||
.child-item-wrapper:last-child .child-row {
|
||||
border-bottom: none;
|
||||
}
|
||||
|
||||
@@ -3,7 +3,7 @@
|
||||
import type { Item, Collection } from '$lib/types';
|
||||
import { getStatusOptions, parseSchema, parseFields } from '$lib/types';
|
||||
import { itemComparator, type SortMode } from '$lib/collections/itemSort';
|
||||
import { reorderGroup, disabledDirections, adjacentColumn, type ReorderDirection } from '$lib/collections/reorder';
|
||||
import { laneOrderAfterMove, disabledDirections, adjacentColumn, type ReorderDirection } from '$lib/collections/reorder';
|
||||
import {
|
||||
bucketByColumn,
|
||||
formatLaneLabel,
|
||||
@@ -22,7 +22,9 @@
|
||||
import { localIndex } from '$lib/stores/localIndex.svelte';
|
||||
import { collectionStore } from '$lib/stores/collections.svelte';
|
||||
import { columnAccentClassFor } from '$lib/utils/fieldColors';
|
||||
import { dndzone, TRIGGERS, SHADOW_ITEM_MARKER_PROPERTY_NAME } from 'svelte-dnd-action';
|
||||
import { TRIGGERS, SHADOW_ITEM_MARKER_PROPERTY_NAME } from 'svelte-dnd-action';
|
||||
import { workspaceStore } from '$lib/stores/workspace.svelte';
|
||||
import { lockableDndzone } from '$lib/collections/lockableDndzone';
|
||||
import type { DndEvent } from 'svelte-dnd-action';
|
||||
import ItemCard from './ItemCard.svelte';
|
||||
import EmptyState from '../common/EmptyState.svelte';
|
||||
@@ -703,6 +705,16 @@
|
||||
}
|
||||
|
||||
|
||||
// A card moves (drag, or the card menu's Move entries) only when the lane
|
||||
// allows reordering AND the caller may edit that card (BUG-3259). The
|
||||
// lane's gate is `canEdit` = canEditCollection, and an item grant beats a
|
||||
// collection grant, so a guest may edit a collection yet only VIEW one of
|
||||
// its cards. Moving that card writes its status or sort_order, which the
|
||||
// server refuses.
|
||||
function cardMovable(columnValue: string, item: Item): boolean {
|
||||
return canReorderLane(columnValue) && workspaceStore.canEditItem(item);
|
||||
}
|
||||
|
||||
// Menu-driven reorder (IDEA-1898), lane-relative — the non-drag
|
||||
// counterpart for touch (board drag is disabled on mobile) and long
|
||||
// lanes. Scope is the item's own lane, matching the drag handler.
|
||||
@@ -711,10 +723,8 @@
|
||||
const grp = (columnData[laneKey(columnValue)] ?? []).filter(
|
||||
(i: any) => !i[SHADOW_ITEM_MARKER_PROPERTY_NAME]
|
||||
);
|
||||
const updates = reorderGroup(grp, item.id, dir);
|
||||
if (updates.length > 0) {
|
||||
onReorder(updates.map((u) => ({ slug: u.item.id, sort_order: u.sort_order })));
|
||||
}
|
||||
const lane = laneOrderAfterMove(grp, item.id, dir);
|
||||
if (lane.length > 0) onReorder(lane);
|
||||
}
|
||||
|
||||
// Menu-driven adjacent-column move (TASK-1908) — the horizontal
|
||||
@@ -976,7 +986,7 @@
|
||||
<!-- svelte-ignore a11y_no_static_element_interactions -->
|
||||
<div
|
||||
class="column-cards"
|
||||
use:dndzone={{
|
||||
use:lockableDndzone={{
|
||||
items: colItems,
|
||||
flipDurationMs,
|
||||
type: 'board-card',
|
||||
@@ -1001,7 +1011,13 @@
|
||||
oncontextmenu={(e) => e.preventDefault()}
|
||||
>
|
||||
{#each colItems as item, i (item.id)}
|
||||
<div class="card-wrapper" class:no-drag={noTouchDrag}>
|
||||
<!-- data-drag-locked: a card the caller may only view cannot be
|
||||
dragged, though the lane allows it (BUG-3259). -->
|
||||
<div
|
||||
class="card-wrapper"
|
||||
class:no-drag={noTouchDrag || !workspaceStore.canEditItem(item)}
|
||||
data-drag-locked={!workspaceStore.canEditItem(item)}
|
||||
>
|
||||
<ItemCard
|
||||
{item}
|
||||
{collection}
|
||||
@@ -1011,10 +1027,10 @@
|
||||
onStatusClick={onStatusChange}
|
||||
progress={itemProgress?.[item.id] ?? null}
|
||||
{progressLabel}
|
||||
onReorderItem={canReorderLane(colValue) ? (it, dir) => reorderItem(colValue, it, dir) : undefined}
|
||||
onMoveItem={canReorderLane(colValue) ? (it, dir) => moveItem(colValue, it, dir) : undefined}
|
||||
horizontal={canReorderLane(colValue)}
|
||||
reorderDisabledDirs={canReorderLane(colValue) ? moveDisabledDirs(colValue, i, colItems.length) : undefined}
|
||||
onReorderItem={cardMovable(colValue, item) ? (it, dir) => reorderItem(colValue, it, dir) : undefined}
|
||||
onMoveItem={cardMovable(colValue, item) ? (it, dir) => moveItem(colValue, it, dir) : undefined}
|
||||
horizontal={cardMovable(colValue, item)}
|
||||
reorderDisabledDirs={cardMovable(colValue, item) ? moveDisabledDirs(colValue, i, colItems.length) : undefined}
|
||||
{onItemOpen}
|
||||
/>
|
||||
</div>
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
import { parseSchema, parseFields } from '$lib/types';
|
||||
import { itemComparator, type SortMode } from '$lib/collections/itemSort';
|
||||
import { formatLaneLabel, isUngrouped, laneKey, laneValue } from '$lib/collections/boardColumns';
|
||||
import { reorderGroup, disabledDirections, type ReorderDirection } from '$lib/collections/reorder';
|
||||
import { laneOrderAfterMove, disabledDirections, type ReorderDirection } from '$lib/collections/reorder';
|
||||
import {
|
||||
narrowRelationRow,
|
||||
relationLaneAcceptsDrop,
|
||||
@@ -17,6 +17,8 @@
|
||||
import { collectionStore } from '$lib/stores/collections.svelte';
|
||||
import { SvelteSet } from 'svelte/reactivity';
|
||||
import { dndzone, TRIGGERS, SHADOW_ITEM_MARKER_PROPERTY_NAME } from 'svelte-dnd-action';
|
||||
import { workspaceStore } from '$lib/stores/workspace.svelte';
|
||||
import { lockableDndzone } from '$lib/collections/lockableDndzone';
|
||||
import type { DndEvent } from 'svelte-dnd-action';
|
||||
import ItemCard from './ItemCard.svelte';
|
||||
import EmptyState from '../common/EmptyState.svelte';
|
||||
@@ -428,16 +430,16 @@
|
||||
// (sort_order is only honored then), and not while search is
|
||||
// preserving relevance order.
|
||||
let canReorderItems = $derived(canEdit && sortMode === 'manual' && !preserveOrder);
|
||||
// Per card, the menu also needs canEditItem (BUG-3259): an item grant beats
|
||||
// a collection grant, so a card in an editable collection can be view-only.
|
||||
|
||||
function reorderItem(groupName: string, item: Item, dir: ReorderDirection) {
|
||||
if (!onReorder) return;
|
||||
const grp = (groupData[laneKey(groupName)] ?? []).filter(
|
||||
(i: any) => !i[SHADOW_ITEM_MARKER_PROPERTY_NAME]
|
||||
);
|
||||
const updates = reorderGroup(grp, item.id, dir);
|
||||
if (updates.length > 0) {
|
||||
onReorder(updates.map((u) => ({ slug: u.item.id, sort_order: u.sort_order })));
|
||||
}
|
||||
const lane = laneOrderAfterMove(grp, item.id, dir);
|
||||
if (lane.length > 0) onReorder(lane);
|
||||
}
|
||||
|
||||
function toggleGroup(groupName: string) {
|
||||
@@ -548,7 +550,7 @@
|
||||
<!-- svelte-ignore a11y_no_static_element_interactions -->
|
||||
<div
|
||||
class="group-items"
|
||||
use:dndzone={{
|
||||
use:lockableDndzone={{
|
||||
items: grpItems,
|
||||
flipDurationMs,
|
||||
type: 'list-item',
|
||||
@@ -571,7 +573,14 @@
|
||||
oncontextmenu={(e) => e.preventDefault()}
|
||||
>
|
||||
{#each grpItems as item, i (item.id)}
|
||||
<div class="list-row" class:kb-focused={focusedItemId === item.id}>
|
||||
<!-- data-drag-locked: a row the caller may only view cannot be
|
||||
dragged, though the group allows it (BUG-3259). -->
|
||||
<div
|
||||
class="list-row"
|
||||
class:kb-focused={focusedItemId === item.id}
|
||||
class:drag-locked={!workspaceStore.canEditItem(item)}
|
||||
data-drag-locked={!workspaceStore.canEditItem(item)}
|
||||
>
|
||||
<ItemCard
|
||||
{item}
|
||||
{collection}
|
||||
@@ -581,8 +590,8 @@
|
||||
onStatusClick={onStatusChange}
|
||||
progress={itemProgress?.[item.id] ?? null}
|
||||
{progressLabel}
|
||||
onReorderItem={canReorderItems ? (it, dir) => reorderItem(groupName, it, dir) : undefined}
|
||||
reorderDisabledDirs={canReorderItems ? disabledDirections(i, grpItems.length) : undefined}
|
||||
onReorderItem={canReorderItems && workspaceStore.canEditItem(item) ? (it, dir) => reorderItem(groupName, it, dir) : undefined}
|
||||
reorderDisabledDirs={canReorderItems && workspaceStore.canEditItem(item) ? disabledDirections(i, grpItems.length) : undefined}
|
||||
{onItemOpen}
|
||||
/>
|
||||
</div>
|
||||
@@ -828,6 +837,12 @@
|
||||
cursor: grabbing;
|
||||
}
|
||||
|
||||
/* A row the caller may only view does not drag (BUG-3259). */
|
||||
.list-row.drag-locked,
|
||||
.list-row.drag-locked:active {
|
||||
cursor: default;
|
||||
}
|
||||
|
||||
.list-row:last-child {
|
||||
border-bottom: none;
|
||||
}
|
||||
|
||||
@@ -8,7 +8,7 @@
|
||||
import { workspaceStore } from '$lib/stores/workspace.svelte';
|
||||
import { parseSchema, parseFields, formatItemRef, itemUrlId } from '$lib/types';
|
||||
import { itemComparator, type SortMode } from '$lib/collections/itemSort';
|
||||
import { reorderGroup, disabledDirections, type ReorderDirection } from '$lib/collections/reorder';
|
||||
import { laneOrderAfterMove, disabledDirections, type ReorderDirection } from '$lib/collections/reorder';
|
||||
import { page } from '$app/state';
|
||||
import { canonicalValueColor, formatFieldLabel as formatLabel } from '$lib/utils/fieldColors';
|
||||
import Chip from '$lib/components/common/Chip.svelte';
|
||||
@@ -167,10 +167,8 @@
|
||||
|
||||
function reorderItem(item: Item, dir: ReorderDirection) {
|
||||
if (!onReorder) return;
|
||||
const updates = reorderGroup(sortedItems, item.id, dir);
|
||||
if (updates.length > 0) {
|
||||
onReorder(updates.map((u) => ({ slug: u.item.id, sort_order: u.sort_order })));
|
||||
}
|
||||
const lane = laneOrderAfterMove(sortedItems, item.id, dir);
|
||||
if (lane.length > 0) onReorder(lane);
|
||||
}
|
||||
|
||||
function relativeTime(dateStr: string): string {
|
||||
@@ -372,12 +370,17 @@
|
||||
<div class="table-cell col-updated" role="cell"><span class="cell-date">{relativeTime(item.updated_at)}</span></div>
|
||||
{#if canReorder}
|
||||
<div class="table-cell col-actions" role="cell">
|
||||
<ItemActionsMenu
|
||||
{item}
|
||||
label={item.title}
|
||||
disabledDirs={disabledDirections(i, sortedItems.length)}
|
||||
onReorder={(dir) => reorderItem(item, dir)}
|
||||
/>
|
||||
<!-- The column is the table's (collection-level); the menu is
|
||||
the row's, since an item grant beats a collection grant and
|
||||
a view-only row cannot be moved (BUG-3259). -->
|
||||
{#if workspaceStore.canEditItem(item)}
|
||||
<ItemActionsMenu
|
||||
{item}
|
||||
label={item.title}
|
||||
disabledDirs={disabledDirections(i, sortedItems.length)}
|
||||
onReorder={(dir) => reorderItem(item, dir)}
|
||||
/>
|
||||
{/if}
|
||||
</div>
|
||||
{/if}
|
||||
</div>
|
||||
|
||||
@@ -23,20 +23,24 @@ const VIEWS = {
|
||||
'../../../routes/[username]/[workspace]/roles/+page.svelte': 1,
|
||||
} as const;
|
||||
|
||||
/** The text of each `use:dndzone={{ … }}` config, brace-matched. */
|
||||
/**
|
||||
* The text of each `use:dndzone={{ … }}` or `use:lockableDndzone={{ … }}`
|
||||
* config, brace-matched. `lockableDndzone` is `dndzone` with per-card locking
|
||||
* (BUG-3259) and takes the same options, so it is the same zone for this guard.
|
||||
*/
|
||||
function dndzoneConfigs(src: string): string[] {
|
||||
const out: string[] = [];
|
||||
let at = src.indexOf('use:dndzone={{');
|
||||
while (at >= 0) {
|
||||
const opener = /use:(?:lockableDndzone|dndzone)=\{\{/g;
|
||||
for (let m = opener.exec(src); m; m = opener.exec(src)) {
|
||||
let depth = 0;
|
||||
let i = at + 'use:dndzone='.length;
|
||||
let i = m.index + m[0].length - 2;
|
||||
const start = i;
|
||||
for (; i < src.length; i++) {
|
||||
if (src[i] === '{') depth++;
|
||||
else if (src[i] === '}' && --depth === 0) break;
|
||||
}
|
||||
out.push(src.slice(start, i + 1));
|
||||
at = src.indexOf('use:dndzone={{', i);
|
||||
opener.lastIndex = i;
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
@@ -251,9 +251,6 @@ const KNOWN_OUTSIDE_FENCE: Record<string, string> = {
|
||||
'lib/components/items/PushToAgentDialog.svelte::handleCopyInstead::handleDismiss': 'local: after a clipboard write, view-fenced on destroyed/presenceGen',
|
||||
'lib/components/common/QuickActionsMenu.svelte::copyAndAnnounce::announce': 'local: copied/failed kind after a clipboard write',
|
||||
// ── local render of content already on screen ──
|
||||
// ── logging ──
|
||||
'lib/components/ChildItems.svelte::handleFinalize::console.error': 'log',
|
||||
'lib/components/ChildItems.svelte::reorderChild::console.error': 'log',
|
||||
// ── a server fact, true under any identity ──
|
||||
'lib/components/items/ItemAttachmentStrip.svelte::performDelete::announceAttachmentDeleted ×2':
|
||||
'global: a 204/404 is a fact about (ws, id), deliberately ahead of the view fence',
|
||||
|
||||
@@ -37,6 +37,7 @@
|
||||
import { uiStore } from '$lib/stores/ui.svelte';
|
||||
import { titleStore } from '$lib/stores/title.svelte';
|
||||
import { workspaceStore } from '$lib/stores/workspace.svelte';
|
||||
import { planLaneOrder, persistReorder } from '$lib/collections/reorderPlan';
|
||||
import { authStore } from '$lib/stores/auth.svelte';
|
||||
import { localIndex } from '$lib/stores/localIndex.svelte';
|
||||
import { enterWorkspaceIndex } from '$lib/stores/workspaceIndexEntry';
|
||||
@@ -2237,52 +2238,59 @@
|
||||
|
||||
async function handleReorder(updates: { slug: string; sort_order: number }[]) {
|
||||
if (!wsSlug) return;
|
||||
const ws = wsSlug;
|
||||
const epochAtEntry = captureIdentity();
|
||||
// Only persist items whose sort_order actually changed
|
||||
const dirty: { id: string; sort_order: number }[] = [];
|
||||
for (const { slug, sort_order } of updates) {
|
||||
const item = items.find((i) => i.slug === slug || i.id === slug);
|
||||
if (item && item.sort_order !== sort_order) {
|
||||
// Optimistic local update: upsert into the local index
|
||||
// with the new sort_order BEFORE awaiting the API. The
|
||||
// caller (e.g. ListView) doesn't await onReorder, so it
|
||||
// resyncs its rendered groups from `items` immediately
|
||||
// after drag-end — without an optimistic write the rows
|
||||
// snap back to the old order until the network PATCH
|
||||
// returns. Clearing `seq` on the optimistic copy
|
||||
// bypasses the per-row seq guard so the real API
|
||||
// response (with a higher seq) wins on arrival
|
||||
// (Codex P2 round 3 of TASK-1357).
|
||||
localIndex.upsert(wsSlug, {
|
||||
...item,
|
||||
sort_order,
|
||||
seq: undefined,
|
||||
});
|
||||
dirty.push({ id: item.id, sort_order });
|
||||
}
|
||||
// The views hand over a whole lane in its new on-screen order, as dense
|
||||
// indices. A card the caller may only VIEW (an item grant beats a
|
||||
// collection grant) cannot be renumbered, so the plan leaves it where it
|
||||
// is and fits the others around it, writing only cards whose value
|
||||
// changes (BUG-3259). With every card editable the plan is exactly the
|
||||
// dense renumber this used to send.
|
||||
const lane = [...updates]
|
||||
.sort((a, b) => a.sort_order - b.sort_order)
|
||||
.map(({ slug }) => items.find((i) => i.slug === slug || i.id === slug))
|
||||
.filter((i): i is Item => !!i);
|
||||
const plan = planLaneOrder(lane, (i) => workspaceStore.canEditItem(i));
|
||||
if (!plan.ok) {
|
||||
// No integer room between view-only cards for the card that moved.
|
||||
// Nothing is written, so the lane re-renders from the stored order.
|
||||
toastStore.show("Couldn't keep that order: a card you can only view is in the way.", 'error');
|
||||
return;
|
||||
}
|
||||
if (dirty.length === 0) return;
|
||||
// Persist to API sequentially (SQLite can't handle concurrent
|
||||
// writes), and upsert each returned row so the local index
|
||||
// settles back to the canonical server seq.
|
||||
try {
|
||||
for (const { id, sort_order } of dirty) {
|
||||
if (plan.writes.length === 0) return;
|
||||
// Optimistic local writes first: the callers (e.g. ListView) don't await
|
||||
// onReorder and resync their rendered groups from `items` at once, so
|
||||
// without them the rows snap back until the PATCH returns. Clearing
|
||||
// `seq` on the optimistic copy bypasses the per-row seq guard so the real
|
||||
// response (higher seq) wins on arrival (Codex P2 round 3 of TASK-1357).
|
||||
// Persisted one at a time (SQLite takes one writer). A refusal restores
|
||||
// every card not yet confirmed to its original row, which carries its
|
||||
// original seq, so a fresher SSE row is not overwritten (BUG-3259).
|
||||
let epoch = localIndex.scopeEpochFor(ws);
|
||||
const landed = await persistReorder<Item>(plan.writes, {
|
||||
original: (id) => lane.find((i) => i.id === id),
|
||||
applyLocal: (row) => localIndex.upsert(ws, { ...row, seq: undefined }),
|
||||
// After a refused await: under the identity that made the request only.
|
||||
restoreLocal: (row) => {
|
||||
if (identityHeld(epochAtEntry)) localIndex.upsert(ws, row, epoch);
|
||||
},
|
||||
send: ({ id, sort_order }) => {
|
||||
// Per-iteration epoch: a resync mid-loop must only reject the
|
||||
// settle-upserts for PATCHes issued before it, not later ones
|
||||
// issued under the new scope (BUG-2098). Capturing outside the
|
||||
// loop would wrongly reject every post-resync iteration. The
|
||||
// optimistic pre-writes above are synchronous (no await gap) so
|
||||
// they need no guard.
|
||||
const epoch = localIndex.scopeEpochFor(wsSlug);
|
||||
const updated = await api.items.update(wsSlug, id, { sort_order });
|
||||
// PER-ITERATION, like the scope epoch beside it: the loop is
|
||||
// sequential and an identity change mid-loop must stop the
|
||||
// remaining settles, not just the current one (BUG-3084).
|
||||
if (!identityHeld(epochAtEntry)) return;
|
||||
localIndex.upsert(wsSlug, updated, epoch);
|
||||
// issued under the new scope (BUG-2098).
|
||||
epoch = localIndex.scopeEpochFor(ws);
|
||||
return api.items.update(ws, id, { sort_order });
|
||||
},
|
||||
settle: (updated) => {
|
||||
// PER-ITERATION, like the scope epoch beside it: an identity
|
||||
// change mid-loop must stop the remaining settles (BUG-3084).
|
||||
if (!identityHeld(epochAtEntry)) return false;
|
||||
localIndex.upsert(ws, updated, epoch);
|
||||
return true;
|
||||
}
|
||||
} catch (e) {
|
||||
console.error('Failed to persist sort order:', e);
|
||||
});
|
||||
if (!landed && identityHeld(epochAtEntry)) {
|
||||
console.error('Failed to persist sort order');
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -28,7 +28,7 @@ identityGateSuite({
|
||||
reviewed: '79b8af40feea',
|
||||
why: `${ENTRY}; the open-children confirm gets identityHeld as its re-check, and the forced re-send checks again; the rethrows stay unfenced so BoardView can undo its optimistic move`,
|
||||
},
|
||||
handleReorder: { reviewed: 'c87eae85bd2d', why: 'optimistic upserts before any await; identityHeld(epochAtEntry) after each update in the loop, before its settle' },
|
||||
handleReorder: { reviewed: '85e3a09f4c5a', why: 'optimistic upserts before any await (persistReorder applies them before its first send); identityHeld(epochAtEntry) after each update, before its settle, and before a refusal restores the original rows (BUG-3259)' },
|
||||
handleGroupReorder: {
|
||||
reviewed: '8a2d335085ec',
|
||||
why: 'collGen, the route, then identityHeld(epochAtEntry) before the collection write; the conflict path checks identity BEFORE its re-read request (BUG-3238) and after it, before the reseed and navigation; both toasts fenced',
|
||||
|
||||
@@ -19,7 +19,9 @@
|
||||
import Button from '$lib/components/common/Button.svelte';
|
||||
import PageHeader from '$lib/components/common/PageHeader.svelte';
|
||||
import EmptyState from '$lib/components/common/EmptyState.svelte';
|
||||
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 } from '$lib/collections/reorderPlan';
|
||||
import type { DndEvent } from 'svelte-dnd-action';
|
||||
|
||||
/**
|
||||
@@ -501,29 +503,52 @@
|
||||
}
|
||||
}
|
||||
|
||||
// Always persist sort order for all items in this lane (covers both
|
||||
// within-lane reorder and cross-lane moves)
|
||||
const reorderUpdates = finalItems.map((item, index) => ({
|
||||
item_id: item.id,
|
||||
role_sort_order: index
|
||||
}));
|
||||
// Persist the lane's order (covers both within-lane reorder and
|
||||
// cross-lane moves). The server refuses the WHOLE batch if it names an
|
||||
// item the caller may only view, so the plan leaves such a card at its
|
||||
// stored order and fits the others around it (BUG-3259). With every
|
||||
// card editable it is the dense renumber this always sent, minus cards
|
||||
// already in place.
|
||||
const plan = planLaneOrder(
|
||||
finalItems.map((item) => ({ id: item.id, sort_order: item.role_sort_order ?? 0 })),
|
||||
(c) => {
|
||||
const item = finalItems.find((i) => i.id === c.id);
|
||||
return !!item && workspaceStore.canEditItem(item);
|
||||
}
|
||||
);
|
||||
const planned = new Map(plan.ok ? plan.writes.map((w) => [w.id, w.sort_order]) : []);
|
||||
const reorderUpdates = [...planned].map(([item_id, role_sort_order]) => ({ item_id, role_sort_order }));
|
||||
|
||||
// Optimistic: update lanes state with new sort orders BEFORE releasing isDragging
|
||||
// See the lost-identity rule at the top of this handler.
|
||||
if (!identityHeld(epochAtEntry)) return;
|
||||
lanes = lanes.map((lane) => {
|
||||
if (laneKey(lane) !== key) return lane;
|
||||
return { ...lane, items: finalItems.map((item, index) => ({ ...item, role_sort_order: index })) };
|
||||
return {
|
||||
...lane,
|
||||
items: finalItems.map((item) => ({ ...item, role_sort_order: planned.get(item.id) ?? item.role_sort_order }))
|
||||
};
|
||||
});
|
||||
if (!plan.ok) {
|
||||
// No integer room between view-only cards: nothing is written.
|
||||
toastStore.show("Couldn't keep that order: a card you can only view is in the way.", 'error');
|
||||
}
|
||||
|
||||
// Now safe to release — lanes has the correct data for the $effect to sync from
|
||||
isDragging = false;
|
||||
|
||||
if (reorderUpdates.length === 0) return;
|
||||
try {
|
||||
await api.agentRoles.reorder(wsSlug, reorderUpdates);
|
||||
if (!identityHeld(epochAtEntry)) return;
|
||||
} catch (err) {
|
||||
console.error('Failed to persist sort order:', err);
|
||||
// A refused batch stored nothing, so the lane shown is an order the
|
||||
// server does not have: reload it, under the identity that made the
|
||||
// request only (BUG-3259 codex round 4; the role write's recovery
|
||||
// above is the same shape).
|
||||
if (!identityHeld(epochAtEntry)) return;
|
||||
await loadData();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1023,7 +1048,7 @@
|
||||
<!-- svelte-ignore a11y_no_static_element_interactions -->
|
||||
<div
|
||||
class="lane-items"
|
||||
use:dndzone={{
|
||||
use:lockableDndzone={{
|
||||
items: laneData[laneKey(lane)] ?? [],
|
||||
flipDurationMs,
|
||||
type: 'role-board-card',
|
||||
@@ -1041,7 +1066,15 @@
|
||||
>
|
||||
{#each (laneData[laneKey(lane)] ?? []) as item (item.id)}
|
||||
{@const coll = collectionForItem(item)}
|
||||
<div class="card-wrapper" class:dimmed={highlightMine && currentUserId && item.assigned_user_id !== currentUserId}>
|
||||
<!-- data-drag-locked: moving a card writes ITS role, so a card
|
||||
the caller may only view cannot be dragged, though the zone
|
||||
allows it for anyone with some edit grant (BUG-3259). -->
|
||||
<div
|
||||
class="card-wrapper"
|
||||
class:dimmed={highlightMine && currentUserId && item.assigned_user_id !== currentUserId}
|
||||
class:drag-locked={!workspaceStore.canEditItem(item)}
|
||||
data-drag-locked={!workspaceStore.canEditItem(item)}
|
||||
>
|
||||
{#if coll}
|
||||
<ItemCard {item} collection={coll} compact={true} showCollection={true} />
|
||||
{:else}
|
||||
@@ -1339,6 +1372,11 @@
|
||||
.card-wrapper:active {
|
||||
cursor: grabbing;
|
||||
}
|
||||
/* A card the caller may only view does not drag (BUG-3259). */
|
||||
.card-wrapper.drag-locked,
|
||||
.card-wrapper.drag-locked:active {
|
||||
cursor: default;
|
||||
}
|
||||
.card-wrapper.dimmed {
|
||||
opacity: 0.35;
|
||||
transition: opacity 0.15s;
|
||||
|
||||
@@ -114,6 +114,7 @@ vi.mock('$lib/stores/workspace.svelte', () => ({
|
||||
get current() { return { id: 'ws1', slug: 'ws', name: 'WS' }; },
|
||||
get currentMembership() { return { role: 'owner' }; },
|
||||
canEditCollection: () => true,
|
||||
canEditItem: () => true,
|
||||
setCurrent: vi.fn(async () => {}),
|
||||
},
|
||||
}));
|
||||
@@ -131,6 +132,7 @@ vi.mock('svelte-dnd-action', () => ({
|
||||
dndzone: () => ({ destroy: () => {} }),
|
||||
TRIGGERS: { DROPPED_INTO_ZONE: 'droppedIntoZone' },
|
||||
SHADOW_ITEM_MARKER_PROPERTY_NAME: '__dndShadow',
|
||||
DRAGGED_ELEMENT_ID: 'dnd-action-dragged-el',
|
||||
}));
|
||||
|
||||
import { api } from '$lib/api/client';
|
||||
@@ -141,6 +143,9 @@ const ROLE = { id: 'r1', name: 'Implementer', slug: 'implementer', icon: '🔨',
|
||||
const ITEM = {
|
||||
id: 'i1', slug: 'i1', title: 'Row', item_number: 1, collection_slug: 'tasks',
|
||||
fields: '{}', tags: '[]', agent_role_id: null, assigned_user_id: null,
|
||||
// The API always sends it. Not 0, so a drop to the head of a lane changes it
|
||||
// and persists (the lane renumber writes only cards that move, BUG-3259).
|
||||
role_sort_order: 3,
|
||||
};
|
||||
|
||||
async function mountPage() {
|
||||
@@ -279,6 +284,22 @@ describe('the roles board stops a commit when the identity moves mid-flight', ()
|
||||
).toBe(reordersBefore);
|
||||
});
|
||||
|
||||
it('a REFUSED lane renumber reloads the board, so the lane does not keep an order the server refused (BUG-3259)', async () => {
|
||||
await mountPage();
|
||||
vi.mocked(api.agentRoles.reorder).mockRejectedValueOnce(new Error('403'));
|
||||
dropIntoRoleLane();
|
||||
const write = await waitFor(() => {
|
||||
if (itemUpdates.length === 0) throw new Error('no write yet');
|
||||
return itemUpdates[0]!;
|
||||
});
|
||||
const boardsBefore = boardCalls.length;
|
||||
write.resolve({});
|
||||
await waitFor(() => {
|
||||
expect(vi.mocked(api.agentRoles.reorder)).toHaveBeenCalled();
|
||||
expect(boardCalls.length, 'the refused batch did not reload the board').toBe(boardsBefore + 1);
|
||||
});
|
||||
});
|
||||
|
||||
it('CONTROL: a drop whose write completes under a held identity DOES persist the sort order', async () => {
|
||||
await mountPage();
|
||||
dropIntoRoleLane();
|
||||
|
||||
@@ -16,7 +16,12 @@ identityGateSuite({
|
||||
submitNewItem: { reviewed: 'd592b0b54568', why: `${PAGE_AND_ENTRY}; the finally clears newItemSaving only under the identity` },
|
||||
handleLaneDrop: { reviewed: 'd268ae879c85', why: `${PAGE_AND_ENTRY}, including before the recovery reload` },
|
||||
handleDndFinalize: {
|
||||
reviewed: '21bb6adcb7c7',
|
||||
// Re-reviewed for BUG-3259: the lane renumber is planned around
|
||||
// view-only cards (planLaneOrder), and its no-room toast and the
|
||||
// empty-batch return both sit after the identity check that guards
|
||||
// the optimistic commit. A refused batch reloads the board, after
|
||||
// an identity check, as the role write's recovery does (codex r4).
|
||||
reviewed: '9c9aadb2ef7c',
|
||||
why: `${PAGE_AND_ENTRY}; the identity check sits BEFORE the role write, because that write carries currentUserId; a lost-identity exit writes no shared interaction state`,
|
||||
},
|
||||
loadData: {
|
||||
|
||||
Reference in New Issue
Block a user