diff --git a/web/e2e/task-2998-relation-filter-group.spec.ts b/web/e2e/task-2998-relation-filter-group.spec.ts new file mode 100644 index 000000000..201f11033 --- /dev/null +++ b/web/e2e/task-2998-relation-filter-group.spec.ts @@ -0,0 +1,211 @@ +import { test, expect } from './fixtures'; +import { browserLogin } from './lib/collab-helpers'; +import type { APIRequestContext } from '@playwright/test'; +import type { SuiteFixture } from './fixtures'; + +/** + * TASK-2998 / PLAN-2857 U7 — filtering and grouping a collection view by a + * RELATION field, driven through a real browser against a binary built from + * this tree (CONVE-34 / CONVE-2729). + * + * The unit tests cover the rules and the wiring; what only this leg can show is + * that the whole path holds together — a schema with a relation field, rows + * carrying target ids, the local index resolving them, the board deriving lanes + * from them, and the filter narrowing the list to one target. + * + * The design table's proving row is the grouping assertion: every car lands in + * exactly one lane, and the one pointing at nothing lands in an honest lane + * rather than under a uuid. + */ + +function authJson(fixture: SuiteFixture) { + return { + Authorization: `Bearer ${fixture.apiToken}`, + 'Content-Type': 'application/json', + }; +} + +async function createCollection( + fixture: SuiteFixture, + request: APIRequestContext, + name: string, + schema?: unknown, + settings?: unknown, +): Promise<{ slug: string; prefix: string }> { + const resp = await request.post(`/api/v1/workspaces/${fixture.workspaceSlug}/collections`, { + headers: authJson(fixture), + data: { name }, + }); + if (!resp.ok()) throw new Error(`collection create failed (${resp.status()}): ${await resp.text()}`); + const coll = (await resp.json()) as { slug: string; prefix: string }; + if (schema || settings) { + const patch: Record = {}; + if (schema) patch.schema = JSON.stringify(schema); + if (settings) patch.settings = JSON.stringify(settings); + const up = await request.patch( + `/api/v1/workspaces/${fixture.workspaceSlug}/collections/${coll.slug}`, + { headers: authJson(fixture), data: patch }, + ); + if (!up.ok()) throw new Error(`collection patch failed (${up.status()}): ${await up.text()}`); + } + return coll; +} + +async function createItem( + fixture: SuiteFixture, + request: APIRequestContext, + collSlug: string, + title: string, + fields: Record = {}, +): Promise<{ id: string; slug: string; item_number: number }> { + const resp = await request.post( + `/api/v1/workspaces/${fixture.workspaceSlug}/collections/${collSlug}/items`, + { headers: authJson(fixture), data: { title, fields: JSON.stringify(fields), content: '' } }, + ); + if (!resp.ok()) throw new Error(`item create failed (${resp.status()}): ${await resp.text()}`); + return (await resp.json()) as { id: string; slug: string; item_number: number }; +} + +/** + * Best-effort, and it SWALLOWS everything — including the request throwing + * because the context is already closed after a timeout. The repo's own + * `deleteCollection` helper carries this warning and I reimplemented it without + * the protection, so the first failing run reported + * `apiRequestContext.delete: Target page, context or browser has been closed` + * and hid the assertion that actually failed. A leaked scratch collection is + * visible in the warning and harmless; a swallowed assertion failure is not. + */ +async function deleteCollection(fixture: SuiteFixture, request: APIRequestContext, slug: string) { + try { + const resp = await request.delete( + `/api/v1/workspaces/${fixture.workspaceSlug}/collections/${slug}`, + { headers: authJson(fixture) }, + ); + if (!resp.ok()) console.warn(`cleanup: collection ${slug} not deleted (${resp.status()})`); + } catch (err) { + console.warn(`cleanup: collection ${slug} not deleted: ${String(err)}`); + } +} + +/** + * NOTE ON WHAT THIS LEG CANNOT COVER, found by trying it. A value that resolves + * to NOTHING cannot be created through the API any more: U1's validator + * (TASK-2878) refuses it with + * `"…" does not name an item in collection "colors-…"`. That lane only exists + * for LEGACY rows written before U1 landed, so it is covered by the unit tests + * — where the fixture can hold one — and named here rather than quietly + * dropped, because "the e2e does not test it" and "the e2e cannot test it" are + * different facts. + * + * The DELETED branch IS reachable, and is the one this leg drives: soft-delete + * a colour and the local index still holds the row, so the lane can name it + * honestly instead of losing it. + */ + +test.describe('relation filter and group (PLAN-2857 U7)', () => { + test.setTimeout(90_000); + test.beforeEach(({}, testInfo) => { + test.skip( + testInfo.project.name !== 'desktop-chromium', + 'one project is enough for a data-shape concern', + ); + }); + + test('board lanes come from the relation targets, and the filter narrows to one', async ({ + page, + fixture, + request, + }) => { + const stamp = Date.now(); + const colors = await createCollection(fixture, request, `Colors ${stamp}`); + const cars = await createCollection( + fixture, + request, + `Cars ${stamp}`, + { + fields: [ + { key: 'car_color', label: 'Colour', type: 'relation', collection: colors.slug }, + ], + }, + { board_group_by: 'car_color' }, + ); + + try { + const red = await createItem(fixture, request, colors.slug, `Red ${stamp}`); + const blue = await createItem(fixture, request, colors.slug, `Blue ${stamp}`); + await createItem(fixture, request, cars.slug, `Ferrari ${stamp}`, { car_color: red.id }); + await createItem(fixture, request, cars.slug, `Bugatti ${stamp}`, { car_color: blue.id }); + await createItem(fixture, request, cars.slug, `Unpainted ${stamp}`); + // Points at a colour that is then DELETED — the lane must keep + // naming it rather than folding it in with "no colour". + const doomed = await createItem(fixture, request, colors.slug, `Doomed ${stamp}`); + await createItem(fixture, request, cars.slug, `Ghost ${stamp}`, { car_color: doomed.id }); + const del = await request.delete( + `/api/v1/workspaces/${fixture.workspaceSlug}/items/${doomed.slug}`, + { headers: authJson(fixture) }, + ); + if (!del.ok()) throw new Error(`colour delete failed (${del.status()})`); + + await browserLogin(page); + await page.goto( + `/${fixture.adminUsername}/${fixture.workspaceSlug}/${cars.slug}?view=board`, + ); + + const laneNames = page.locator('.kanban-column .column-name'); + await expect(laneNames.filter({ hasText: `Red ${stamp}` })).toBeVisible(); + await expect(laneNames.filter({ hasText: `Blue ${stamp}` })).toBeVisible(); + // The deleted colour keeps its own lane, and says what it is. + const doomedLane = laneNames.filter({ hasText: `Doomed ${stamp}` }); + await expect(doomedLane).toBeVisible(); + await expect(doomedLane).toContainText('(deleted)'); + // And no lane anywhere is labelled with a raw id. + await expect(page.locator('.board-view')).not.toContainText(red.id); + + // The ref is on the lane, so the label is the chip rather than a + // bare title — the thing that distinguishes this from grouping by + // any old text field. + await expect( + page.locator('.kanban-column .column-name .lane-ref').first(), + ).toBeVisible(); + + // Every car is placed exactly once: four cars, four cards. + await expect(page.locator('.kanban-column .item-card')).toHaveCount(4); + + // AND IN THE RIGHT LANES (codex round 4). A total count survives a + // mutant that puts every card in one lane while the headings still + // render, which is exactly what dropping the bucketing callback + // does. + const laneWith = (name: string) => + page.locator('.kanban-column').filter({ has: page.locator('.column-name', { hasText: name }) }); + await expect(laneWith(`Red ${stamp}`).locator('.item-card')).toHaveCount(1); + await expect(laneWith(`Blue ${stamp}`).locator('.item-card')).toHaveCount(1); + await expect(laneWith(`Doomed ${stamp}`).locator('.item-card')).toHaveCount(1); + await expect(laneWith('Uncategorized').locator('.item-card')).toHaveCount(1); + + // FILTER by Red: the picker is scoped to the colours collection. + // The bar lives behind the toolbar's filter toggle — found by the + // first run of this leg timing out on a trigger that was never + // rendered, not by reading the page. + await page.getByRole('button', { name: 'Toggle filters' }).click(); + await page.locator('.relation-filter-trigger').click(); + const picker = page.locator('.relation-filter-picker'); + await expect(picker).toBeVisible(); + // role=combobox, not textbox — ItemPicker's input is an ARIA + // combobox, which is not a textbox to Playwright's role engine. + await picker.getByRole('combobox').fill(`Red ${stamp}`); + await picker.getByRole('option', { name: new RegExp(`Red ${stamp}`) }).first().click(); + + await expect(page.locator('.relation-filter-trigger')).toContainText(`Red ${stamp}`); + await expect(page.locator('.kanban-column .item-card')).toHaveCount(1); + await expect(page.locator('.kanban-column .item-card')).toContainText(`Ferrari ${stamp}`); + + // And clearing restores every card — a filter that could not be + // cleared would pass every assertion above. + await page.locator('.relation-filter-clear').click(); + await expect(page.locator('.kanban-column .item-card')).toHaveCount(4); + } finally { + await deleteCollection(fixture, request, cars.slug); + await deleteCollection(fixture, request, colors.slug); + } + }); +}); diff --git a/web/src/lib/collections/boardColumns.ts b/web/src/lib/collections/boardColumns.ts index 93631b968..ca0035053 100644 --- a/web/src/lib/collections/boardColumns.ts +++ b/web/src/lib/collections/boardColumns.ts @@ -40,7 +40,15 @@ function laneValue(raw: unknown): string { export function bucketByColumn( items: Item[], groupField: string, - columns: string[] + columns: string[], + /** + * How to read an item's lane value, when the field's own value is not it. + * A RELATION field needs this (TASK-2998): values that resolve to nothing + * are folded onto a sentinel first, so "no value", "target deleted" and + * "points at nothing" do not all collapse into UNCATEGORIZED. Defaults to + * the field's own value, so every existing caller is unchanged. + */ + valueFor?: (item: Item) => string ): Record { const known = new Set(columns); const result: Record = { [UNCATEGORIZED]: [] }; @@ -48,7 +56,7 @@ export function bucketByColumn( result[col] = []; } for (const item of items) { - const value = laneValue(parseFields(item)[groupField]); + const value = valueFor ? valueFor(item) : laneValue(parseFields(item)[groupField]); if (value && known.has(value)) { result[value].push(item); } else { diff --git a/web/src/lib/collections/relationGroups.test.ts b/web/src/lib/collections/relationGroups.test.ts new file mode 100644 index 000000000..0c809992a --- /dev/null +++ b/web/src/lib/collections/relationGroups.test.ts @@ -0,0 +1,328 @@ +import { describe, expect, it } from 'vitest'; +import type { Item, ItemIndexRow } from '$lib/types'; +import { UNCATEGORIZED, bucketByColumn } from './boardColumns'; +import { + UNRESOLVED_LANE, + narrowRelationRow, + relationChipFor, + relationFilterMatches, + relationLaneAcceptsDrop, + relationLaneAriaName, + relationLaneValueFor, + relationLanes, +} from './relationGroups'; + +/** + * TASK-2998 / PLAN-2857 U7 — grouping a view by a relation field. + * + * The proving row on the design table: every item with a value lands in exactly + * ONE group, and an item with a dangling value lands in an honest group rather + * than under a bare UUID. Both are asserted here against a fixture this file + * builds; when U5's reverse index lands the same assertions get a second + * source to cross-check against, which is the point of that row. + */ + +// `Item.fields` is a JSON STRING on the wire (`parseFields` JSON.parses it), so +// the fixture builds it the same way the API does. An object here would make +// every lane empty and the whole file pass vacuously in the other direction. +function item(id: string, value?: string): Item { + return { + id, + slug: id, + title: id, + fields: JSON.stringify(value === undefined ? {} : { car: value }), + } as unknown as Item; +} + +function row(id: string, title: string, opts: { number?: number; deleted?: boolean } = {}): ItemIndexRow { + return { + id, + title, + item_number: opts.number ?? 1, + collection_prefix: 'COLOR', + // Set, so the positive narrowing case exercises the collection check + // rather than skipping it on an undefined slug. + collection_slug: 'colors', + deleted_at: opts.deleted ? '2026-01-01T00:00:00Z' : null, + } as unknown as ItemIndexRow; +} + +const ROWS: Record = { + red: row('red', 'Red', { number: 1 }), + blue: row('blue', 'Blue', { number: 2 }), + gone: row('gone', 'Gone', { number: 3, deleted: true }), +}; +const resolve = (id: string) => ROWS[id] ?? null; + +describe('relationLanes', () => { + it('derives lanes from the ITEMS, because a relation has no options', () => { + const lanes = relationLanes([item('a', 'red'), item('b', 'blue'), item('c', 'red')], 'car', resolve); + expect(lanes.map((l) => l.value)).toEqual(['blue', 'red']); + }); + + it('orders alphabetically rather than by first appearance', () => { + // Items arrive asynchronously through the local index; a + // first-appearance order would reshuffle the board as they land. + const lanes = relationLanes([item('a', 'red'), item('b', 'blue')], 'car', resolve); + expect(lanes.map((l) => l.title)).toEqual(['Blue', 'Red']); + }); + + it('gives a DELETED target its own lane, with its ref and title', () => { + // It still has both, so it can be labelled honestly — and merging it + // with the unresolved lane would combine rows pointing at different + // things. + const lanes = relationLanes([item('a', 'gone')], 'car', resolve); + expect(lanes).toHaveLength(1); + expect(lanes[0]).toMatchObject({ value: 'gone', ref: 'COLOR-3', title: 'Gone', state: 'deleted' }); + }); + + it('folds values that resolve to NOTHING into one honest lane, never a bare id', () => { + const lanes = relationLanes( + [item('a', 'f47ac10b-58cc-4372-a567-0e02b2c3d479'), item('b', 'purple')], + 'car', + resolve, + ); + expect(lanes).toHaveLength(1); + expect(lanes[0].value).toBe(UNRESOLVED_LANE); + expect(lanes[0].label).toBe('Unresolved reference'); + // The assertion that matters: nothing in the lane repeats the stored + // string, which is usually free text an older unvalidated write left. + expect(JSON.stringify(lanes[0])).not.toContain('f47ac10b'); + expect(JSON.stringify(lanes[0])).not.toContain('purple'); + }); + + it('has no lanes when nothing carries a value', () => { + expect(relationLanes([item('a'), item('b', '')], 'car', resolve)).toEqual([]); + }); +}); + +describe('relationLaneValueFor + bucketByColumn', () => { + it('puts every item in EXACTLY ONE lane — the proving row', () => { + const items = [ + item('live-1', 'red'), + item('live-2', 'red'), + item('live-3', 'blue'), + item('deleted', 'gone'), + item('dangling', 'purple'), + item('empty'), + ]; + const lanes = relationLanes(items, 'car', resolve); + const rewritten = items.map((i) => ({ + ...i, + fields: JSON.stringify({ car: relationLaneValueFor(i, 'car', resolve) }), + })) as Item[]; + + const buckets = bucketByColumn(rewritten, 'car', lanes.map((l) => l.value)); + + const placements = Object.entries(buckets).flatMap(([lane, list]) => + list.map((i) => ({ id: i.id, lane })), + ); + // Exactly one placement per item, and no item lost. + expect(placements).toHaveLength(items.length); + expect(new Set(placements.map((p) => p.id)).size).toBe(items.length); + + const laneOf = (id: string) => placements.find((p) => p.id === id)?.lane; + expect(laneOf('live-1')).toBe('red'); + expect(laneOf('live-2')).toBe('red'); + expect(laneOf('live-3')).toBe('blue'); + expect(laneOf('deleted')).toBe('gone'); + expect(laneOf('dangling')).toBe(UNRESOLVED_LANE); + expect(laneOf('empty')).toBe(UNCATEGORIZED); + }); + + it('keeps "no value", "deleted target" and "points at nothing" APART', () => { + // Without the rewrite, `bucketByColumn` routes every value it does not + // recognise to UNCATEGORIZED — collapsing three different facts about an + // item into the lane that means "this field is empty". + expect(relationLaneValueFor(item('a'), 'car', resolve)).toBe(UNCATEGORIZED); + expect(relationLaneValueFor(item('b', 'gone'), 'car', resolve)).toBe('gone'); + expect(relationLaneValueFor(item('c', 'nope'), 'car', resolve)).toBe(UNRESOLVED_LANE); + }); + + it('treats a non-string value as no value rather than crashing', () => { + const weird = { + id: 'w', + slug: 'w', + title: 'w', + fields: JSON.stringify({ car: { id: 'red' } }), + } as unknown as Item; + expect(relationLaneValueFor(weird, 'car', resolve)).toBe(UNCATEGORIZED); + }); +}); + +describe('relationLaneAcceptsDrop', () => { + it('accepts a live lane and refuses the other three', () => { + // A live lane is the natural board gesture: dropping a card sets the + // relation. The others would either clear the field or write a + // reference TASK-2878's validator refuses, so the card would move on + // screen and the write would fail behind it. + expect(relationLaneAcceptsDrop({ state: 'live' })).toBe(true); + expect(relationLaneAcceptsDrop({ state: 'deleted' })).toBe(false); + expect(relationLaneAcceptsDrop({ state: 'unresolved' })).toBe(false); + expect(relationLaneAcceptsDrop({ state: 'empty' })).toBe(false); + }); +}); + +describe('narrowRelationRow', () => { + const live = row('red', 'Red'); + const known = new Set(['colors', 'tasks']); + + it('refuses a SLUG match, because the field stores an id', () => { + // The pre-TASK-2878 fallback wrote free text, so a value like "red" + // resolves through `findByIdOrSlug` to whatever item is slugged "red" + // and would render as a working reference to something nobody chose. + const bySlug = { ...live, id: 'a-real-uuid' } as ItemIndexRow; + expect(narrowRelationRow(bySlug, 'red', 'colors', known)).toBeNull(); + expect(narrowRelationRow(live, 'red', 'colors', known)).toBe(live); + }); + + it('refuses a row from the WRONG collection', () => { + const stray = { ...live, collection_slug: 'tasks' } as ItemIndexRow; + expect(narrowRelationRow(stray, 'red', 'colors', known)).toBeNull(); + }); + + it('does NOT judge the collection when either slug is unknown to the list', () => { + // A renamed collection, or a rename the index has applied and the + // collection list has not, is not the value's fault — and judging it + // would flash every lane to unresolved during that window. + const stray = { ...live, collection_slug: 'tasks' } as ItemIndexRow; + expect(narrowRelationRow(stray, 'red', 'colors', new Set(['colors']))).toBe(stray); + expect(narrowRelationRow(stray, 'red', 'colors', undefined)).toBe(stray); + }); + + it('passes a missing row straight through as null', () => { + expect(narrowRelationRow(null, 'red', 'colors', known)).toBeNull(); + expect(narrowRelationRow(undefined, 'red', 'colors', known)).toBeNull(); + }); +}); + +describe('relationChipFor', () => { + it('is the SAME vocabulary a lane header uses', () => { + // The filter chip and the lane header describe the same value, so they + // are built from one function. Two would drift, and the drift would be + // invisible until a user saw a board lane and a filter chip disagree + // about what the same id is called. + // + // `relationLanes` CALLS `relationChipFor`, so this leg alone is partly + // tautological — a change to the shared helper moves both sides (codex + // round 4). The independent anchor is below: the expected shape is + // written out by hand, so the helper cannot redefine what agreement + // means. + const chip = relationChipFor('red', resolve); + const lane = relationLanes([item('a', 'red')], 'car', resolve)[0]; + expect(chip).toEqual(lane); + }); + + it('describes a live value in the shape both surfaces render', () => { + // The independent half: this is what a chip and a lane are, spelled out + // rather than derived from either producer. + expect(relationChipFor('red', resolve)).toEqual({ + value: 'red', + ref: 'COLOR-1', + title: 'Red', + label: 'Red', + state: 'live', + }); + }); + + it('is null for an empty value — the caller owns what "no filter" says', () => { + expect(relationChipFor('', resolve)).toBeNull(); + expect(relationChipFor(' ', resolve)).toBeNull(); + }); + + it('never echoes an unresolvable value back', () => { + const chip = relationChipFor('not-an-item', resolve); + expect(chip?.state).toBe('unresolved'); + expect(JSON.stringify(chip)).not.toContain('not-an-item'); + }); + + it('marks a deleted target without hiding which one it is', () => { + expect(relationChipFor('gone', resolve)).toMatchObject({ + ref: 'COLOR-3', + title: 'Gone', + state: 'deleted', + }); + }); +}); + +describe('narrowRelationRow with no collection list yet', () => { + it('does not judge the collection when the set is null', () => { + // FieldEditor's own set is `Set | null` — null while the + // collection list has not loaded — and that is the same "do not judge" + // case as a slug the list does not know. Typed rather than coerced at + // the call site, so the shared helper states the rule. + const stray = { ...row('red', 'Red'), collection_slug: 'tasks' } as ItemIndexRow; + expect(narrowRelationRow(stray, 'red', 'colors', null)).toBe(stray); + }); +}); + +describe('codex round 1 — values the write path used to accept', () => { + it('trims a padded value, so the lane and the chip agree', () => { + // `relationChipFor` has always trimmed; `relationLaneValueFor` did not. + // A legacy `" red "` therefore produced a LIVE chip and an UNRESOLVED + // lane — the same value described two ways by the two functions this + // module exists to keep in agreement. The pre-TASK-2878 write path + // accepted any string, padding included. + expect(relationLaneValueFor(item('a', ' red '), 'car', resolve)).toBe('red'); + expect(relationChipFor(' red ', resolve)).toMatchObject({ value: 'red', state: 'live' }); + const lanes = relationLanes([item('a', ' red ')], 'car', resolve); + expect(lanes).toHaveLength(1); + expect(lanes[0]).toMatchObject({ value: 'red', state: 'live' }); + }); + + it('collapses padded and unpadded forms of the same value into ONE lane', () => { + // Otherwise the board shows "Red" twice, which is worse than either + // version of the bug on its own. + const lanes = relationLanes([item('a', 'red'), item('b', ' red')], 'car', resolve); + expect(lanes).toHaveLength(1); + }); +}); + +describe('relationLaneAriaName', () => { + it('gives assistive technology the words the eye gets', () => { + // The group's aria-label used to format the raw column VALUE, so a lane + // reading "COLOR-1 Red" announced as "Id-Red column". A claim that a + // lane is named from its target rather than its id is not a claim if it + // holds only for sighted users. + const [live] = relationLanes([item('a', 'red')], 'car', resolve); + expect(relationLaneAriaName(live, 'fallback')).toBe('COLOR-1 Red'); + }); + + it('says a deleted target is deleted, and never echoes an unresolved value', () => { + const [deleted] = relationLanes([item('a', 'gone')], 'car', resolve); + expect(relationLaneAriaName(deleted, 'fallback')).toBe('COLOR-3 Gone (deleted)'); + const [unresolved] = relationLanes([item('a', 'nope')], 'car', resolve); + expect(relationLaneAriaName(unresolved, 'fallback')).toBe('Unresolved reference'); + }); + + it('falls back for a lane that is not a relation lane at all', () => { + // UNCATEGORIZED and every ordinary select lane come through here too. + expect(relationLaneAriaName(undefined, 'Uncategorized')).toBe('Uncategorized'); + }); +}); + +describe('relationFilterMatches (codex round 2)', () => { + it('matches a padded stored value against an unpadded filter', () => { + // The defect it closes: the board trimmed and the filter did not, so an + // item storing `" red "` sat in the Red LANE and vanished when you + // filtered for Red. One value, two answers, one screen apart. + expect(relationFilterMatches(' red ', 'red')).toBe(true); + expect(relationFilterMatches('red', ' red ')).toBe(true); + expect(relationFilterMatches('red', 'red')).toBe(true); + }); + + it('still refuses a different value', () => { + // The counterfactual: trimming must not become "matches anything". + expect(relationFilterMatches('blue', 'red')).toBe(false); + expect(relationFilterMatches('', 'red')).toBe(false); + }); + + it('refuses a non-string, including the multi_relation ARRAY shape', () => { + // U4 stores an array, and no amount of trimming makes `===` match one. + // Matching an array is that unit's to define; guessing here would fix + // half of it in a way U4 would have to undo. + expect(relationFilterMatches(['red'], 'red')).toBe(false); + expect(relationFilterMatches(null, 'red')).toBe(false); + expect(relationFilterMatches(42, 'red')).toBe(false); + }); +}); diff --git a/web/src/lib/collections/relationGroups.ts b/web/src/lib/collections/relationGroups.ts new file mode 100644 index 000000000..0d0f86059 --- /dev/null +++ b/web/src/lib/collections/relationGroups.ts @@ -0,0 +1,265 @@ +// Grouping a board or list by a RELATION field (TASK-2998 / PLAN-2857 U7). +// +// Every other groupable field type carries its own lane vocabulary: a `select` +// has `options`, and `bucketByColumn` buckets against that fixed list. A +// relation has no options — its lanes are whatever target items the rows +// actually point at — so the lanes have to be DERIVED from the data, and their +// labels resolved against the local index. +// +// WHY A PURE MODULE, like `unparentedFilter` and `boardColumns` beside it: the +// rules below (which lanes exist, how each is labelled, which lane a dangling +// value lands in, which lanes accept a drop) are decisions, and a decision that +// can only be exercised by mounting a board view is a decision nobody tests. +// Nothing here imports a rune. + +import type { Item, ItemIndexRow } from '$lib/types'; +import { formatItemRef, parseFields } from '$lib/types'; +import { UNCATEGORIZED } from './boardColumns'; + +/** + * The lane a value with no resolvable target lands in. + * + * Deliberately NOT a bare UUID and not the stored string. A relation value that + * resolves to nothing is usually free text an older, unvalidated write left + * behind (the pre-TASK-2878 fallback accepted any string), so showing it as + * though it were a lane name would present a typo as a category. `FieldEditor` + * says "Unresolved reference" for exactly this state; the lane says the same. + * + * Reserved with a `$` prefix for the reason `$unparented` is: a real field + * value can never collide with it, because schema validation refuses new field + * keys starting with `$` and an item id is a UUID. + */ +export const UNRESOLVED_LANE = '$unresolved'; + +export type RelationLaneState = 'empty' | 'live' | 'deleted' | 'unresolved'; + +/** + * How a single relation VALUE presents — the lane header and the filter chip + * say the same things about the same value, so they are built from one + * function rather than two that drift. + */ +export interface RelationChip { + value: string; + ref: string | null; + title: string | null; + /** What to say when there is no ref/title — never the stored value. */ + label: string; + state: RelationLaneState; +} + +/** + * Describe one relation value. `null` for an empty value (the caller decides + * what "no filter" or "uncategorised" looks like); an unresolvable value + * becomes the honest unresolved chip rather than the string it holds. + */ +export function relationChipFor(value: string, resolve: ResolveRow): RelationChip | null { + const raw = typeof value === 'string' ? value.trim() : ''; + if (!raw) return null; + const row = resolve(raw); + if (!row) { + return { + value: UNRESOLVED_LANE, + ref: null, + title: null, + label: 'Unresolved reference', + state: 'unresolved', + }; + } + return { + value: raw, + ref: formatItemRef(row), + title: row.title ?? null, + label: row.title ?? '', + state: row.deleted_at ? 'deleted' : 'live', + }; +} + +/** A lane IS a chip plus its position on the board. Same vocabulary. */ +export type RelationLane = RelationChip; + +/** Resolve an item id against whatever row source the caller has. */ +export type ResolveRow = (id: string) => ItemIndexRow | null | undefined; + +/** + * The two narrowings a relation value needs on top of a workspace-wide lookup. + * + * `localIndex.findByIdOrSlug` resolves by id OR SLUG and searches the whole + * workspace, and both of those are wrong for a relation: + * + * 1. ID ONLY. A legacy free-text value like "red" — exactly what the + * pre-TASK-2878 fallback wrote — resolves to whatever item is SLUGGED + * "red" and would render as a working reference. The field stores an item + * id; a slug match makes the label lie about what is stored, and slugs are + * mutable, so the same value could point somewhere else tomorrow. + * 2. TARGET COLLECTION. Without it, a relation declared against `colors` can + * label a lane with an item from `tasks` that happens to share an + * identifier. + * + * The collection check only fires when both the declared target and the row's + * own collection are slugs the current list knows — a renamed collection, or a + * rename applied to the index before the collection list caught up, are not the + * value's fault and must not turn every lane unresolved. + * + * WORKED OUT FIRST IN `FieldEditor.svelte`'s relation branch, which now calls + * this rather than carrying its own copy. It was briefly duplicated — that file + * was being edited under TASK-2996 when the board needed the same rule — and + * the fork was closed as soon as 2996 merged without touching it. The reason to + * keep it single is concrete: a divergence shows up as a board lane and a + * properties chip disagreeing about what the same id is called. + */ +export function narrowRelationRow( + row: ItemIndexRow | null | undefined, + value: string, + declaredCollection: string | undefined, + // `null` accepted as well as omitted: FieldEditor's own set is + // `Set | null` — null while the collection list has not loaded — + // and "we do not know the collections yet" is exactly the case this + // argument's guard already treats as "do not judge". + knownCollectionSlugs?: ReadonlySet | null, +): ItemIndexRow | null { + if (!row) return null; + if (row.id !== value) return null; + if ( + declaredCollection && + knownCollectionSlugs?.has(row.collection_slug ?? '') && + knownCollectionSlugs.has(declaredCollection) && + row.collection_slug !== declaredCollection + ) { + return null; + } + return row; +} + +/** + * TRIMMED (codex round 1, P1). `relationChipFor` has always trimmed, so a + * legacy value stored as `" id-red "` produced a live CHIP and an unresolved + * LANE — the same value described two ways by the two functions this module + * exists to keep in agreement. The pre-TASK-2878 write path accepted any + * string, padding included, so those rows exist. + */ +function laneValue(raw: unknown): string { + return typeof raw === 'string' ? raw.trim() : ''; +} + +/** + * The lanes a relation-grouped view should render, in display order. + * + * Derived from the ITEMS rather than from the schema, because a relation field + * has no option list — the lanes are the targets the rows point at. An empty + * result means nothing in view carries a value, which the caller renders as the + * uncategorised lane alone. + * + * ORDERING: live lanes first, alphabetically by title, then deleted ones, then + * the single unresolved lane. Alphabetical rather than first-appearance because + * items arrive asynchronously through the local index, and a first-appearance + * order would reshuffle the board as they land. + * + * DELETED TARGETS KEEP THEIR OWN LANE. They still have a ref and a title, so + * they can be labelled honestly, and merging them would silently combine rows + * that point at different things. Only values that resolve to NOTHING share a + * lane, because there is nothing to tell them apart with. + */ +export function relationLanes(items: Item[], fieldKey: string, resolve: ResolveRow): RelationLane[] { + const seen = new Set(); + const live: RelationLane[] = []; + const deleted: RelationLane[] = []; + let hasUnresolved = false; + + for (const item of items) { + const value = laneValue(parseFields(item)[fieldKey]); + if (!value || seen.has(value)) continue; + seen.add(value); + const chip = relationChipFor(value, resolve); + if (!chip || chip.state === 'unresolved') { + hasUnresolved = true; + continue; + } + (chip.state === 'deleted' ? deleted : live).push(chip); + } + + const byTitle = (a: RelationLane, b: RelationLane) => + (a.title ?? '').localeCompare(b.title ?? ''); + live.sort(byTitle); + deleted.sort(byTitle); + + const lanes = [...live, ...deleted]; + if (hasUnresolved) { + lanes.push({ + value: UNRESOLVED_LANE, + ref: null, + title: null, + label: 'Unresolved reference', + state: 'unresolved', + }); + } + return lanes; +} + +/** + * The value each item should be bucketed on, with unresolvable values folded + * onto the sentinel. + * + * `bucketByColumn` routes any value not in its column list to UNCATEGORIZED, + * which would put "points at a deleted item" and "points at nothing" in the + * same lane as "has no value at all" — three different facts. Rewriting the + * value first keeps them apart while leaving that helper's own rule intact: + * every item still lands in exactly one lane. + */ +export function relationLaneValueFor(item: Item, fieldKey: string, resolve: ResolveRow): string { + const value = laneValue(parseFields(item)[fieldKey]); + if (!value) return UNCATEGORIZED; + return resolve(value) ? value : UNRESOLVED_LANE; +} + +/** + * The lane label for assistive technology — the same words the eye gets. + * + * The board's group `aria-label` used to format the raw column VALUE, so a lane + * reading "COLOR-1 Red" announced as "Id-Red column" and the unresolved lane as + * "$Unresolved column" (codex round 1). The claim this unit makes is that a + * lane is named from its target rather than from the stored id, and a claim + * that holds only for sighted users is not the claim. + */ +export function relationLaneAriaName(lane: RelationLane | undefined, fallback: string): string { + if (!lane) return fallback; + const parts = [lane.ref, lane.title ?? lane.label].filter(Boolean); + const name = parts.join(' '); + return lane.state === 'deleted' ? `${name} (deleted)` : name; +} + +/** + * Can a board drop write this lane's value onto an item? + * + * A live lane is a legitimate drop target — dragging a card there sets the + * relation to that item, which is the natural board gesture. The other three + * are not: UNCATEGORIZED would clear the field (a delete disguised as a move), + * and the deleted and unresolved lanes would write a reference the validator + * refuses anyway (TASK-2878 — a relation value must name a LIVE item in the + * declared collection), so the drop would fail at the server after moving the + * card on screen. + */ +export function relationLaneAcceptsDrop(lane: Pick): boolean { + return lane.state === 'live'; +} + +/** + * Does this item's value for a RELATION field match a filter value? + * + * Strict `===` was the whole comparison, and the board had already started + * trimming — so an item storing `" id-red "` appeared in the `Red` LANE and + * vanished when you filtered for `Red` (codex round 2). The same value, two + * answers, one screen apart. + * + * Trimming on BOTH sides rather than normalising the stored data: the write + * path has refused padded values since TASK-2878, so there is nothing new to + * clean up and nothing to migrate — only old rows to read correctly. + * + * Scalar only, deliberately. `multi_relation` (U4) stores an ARRAY and no + * amount of trimming makes `===` match one; matching an array is that unit's + * to define, and guessing here would fix half of it in a way U4 would have to + * undo. + */ +export function relationFilterMatches(stored: unknown, filterValue: string): boolean { + if (typeof stored !== 'string') return false; + return stored.trim() === filterValue.trim(); +} diff --git a/web/src/lib/components/collections/BoardView.relationGroup.svelte.test.ts b/web/src/lib/components/collections/BoardView.relationGroup.svelte.test.ts new file mode 100644 index 000000000..007404c51 --- /dev/null +++ b/web/src/lib/components/collections/BoardView.relationGroup.svelte.test.ts @@ -0,0 +1,565 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { render, cleanup } from '@testing-library/svelte'; +import { tick } from 'svelte'; +import type { Collection, Item, ItemIndexRow } from '$lib/types'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; + +/** + * TASK-2998 / PLAN-2857 U7 — the BoardView half of grouping by a relation. + * + * The rules are unit-tested in `relationGroups.test.ts`; what is only + * observable HERE is the wiring (CONVE-19): that the board asks for relation + * lanes at all when the group field is a relation, labels them with the chip + * vocabulary rather than the stored id, and keeps the three non-live cases + * apart on screen. + */ + +const ROWS: Record = { + 'id-red': { + id: 'id-red', + title: 'Red', + item_number: 1, + collection_prefix: 'COLOR', + collection_slug: 'colors', + deleted_at: null, + } as unknown as ItemIndexRow, + 'id-blue': { + id: 'id-blue', + title: 'Blue', + item_number: 2, + collection_prefix: 'COLOR', + collection_slug: 'colors', + deleted_at: null, + } as unknown as ItemIndexRow, + 'id-gone': { + id: 'id-gone', + title: 'Gone', + item_number: 9, + collection_prefix: 'COLOR', + collection_slug: 'colors', + deleted_at: '2026-01-01T00:00:00Z', + } as unknown as ItemIndexRow, +}; + +vi.mock('$lib/stores/localIndex.svelte', () => ({ + localIndex: { + findByIdOrSlug: (_ws: string, id: string) => ROWS[id] ?? null, + getByCollection: () => [], + }, +})); + +vi.mock('$lib/stores/collections.svelte', () => ({ + collectionStore: { collections: [{ slug: 'colors' }, { slug: 'cars' }] }, +})); + +import BoardView from './BoardView.svelte'; + +/** BoardView's source with comments stripped, so a guard cannot be satisfied by prose. */ +const SRC = readFileSync(resolve(__dirname, './BoardView.svelte'), 'utf8').replace( + /^[ \t]*\/\/.*$/gm, + '', +); + +function collection(): Collection { + return { + id: 'c1', + workspace_id: 'ws1', + name: 'Cars', + slug: 'cars', + icon: '', + description: '', + schema: JSON.stringify({ + fields: [{ key: 'car_color', label: 'Colour', type: 'relation', collection: 'colors' }], + }), + settings: JSON.stringify({}), + sort_order: 0, + is_default: true, + is_system: false, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + prefix: 'CAR', + } as unknown as Collection; +} + +function item(id: string, value?: string): Item { + return { + id, + workspace_id: 'ws1', + collection_id: 'c1', + slug: id, + title: id, + content: '', + fields: JSON.stringify(value === undefined ? {} : { car_color: value }), + tags: '[]', + status: 'open', + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + } as unknown as Item; +} + +const DANGLING = 'f47ac10b-58cc-4372-a567-0e02b2c3d479'; + +function renderBoard(items: Item[]) { + return render(BoardView, { + props: { + items, + collection: collection(), + wsSlug: 'ws', + groupField: 'car_color', + onStatusChange: vi.fn(), + } as never, + }); +} + +afterEach(() => { + cleanup(); +}); + +describe('BoardView grouped by a relation field', () => { + /** + * Lane names read from the DOM, NOT by substring over the whole page. + * `formatLabel('id-red')` renders "Id-Red", which contains "Red" — so a + * `toContain('Red')` assertion passes against a board that fell back to + * formatting the raw id, which is the defect. Measured: the label mutant + * survived that version of this test. + */ + const lanes = (screen: { container: HTMLElement }) => + [...screen.container.querySelectorAll('.column-name')].map((el) => ({ + ref: el.querySelector('.lane-ref')?.textContent?.trim() ?? null, + note: el.querySelector('.lane-note')?.textContent?.trim() ?? null, + // Everything the lane says that is not the ref or the note. + name: [...el.childNodes] + .filter( + (n) => + !(n instanceof HTMLElement) || + (!n.classList.contains('lane-ref') && !n.classList.contains('lane-note')), + ) + .map((n) => n.textContent ?? '') + .join('') + .trim(), + })); + + /** + * Cards per lane, read from the DOM. Header assertions alone cannot see the + * bucketing (codex round 3): removing `bucketByColumn`'s `valueFor` + * callback leaves every relation lane RENDERED and EMPTY while the cards + * fall into Uncategorized, and every label test stays green. + */ + const cardsByLane = (screen: { container: HTMLElement }) => { + const out: Record = {}; + for (const col of screen.container.querySelectorAll('.kanban-column')) { + // Whitespace stripped ENTIRELY: the ref, name and note are separate + // spans with CSS spacing, so their textContent runs together in one + // place and not another depending on markup indentation. + const name = (col.querySelector('.column-name')?.textContent ?? '').replace(/\s+/g, ''); + out[name] = col.querySelectorAll('.item-card').length; + } + return out; + }; + + it('puts the cards in the lanes, not merely the lanes on the board', () => { + const screen = renderBoard([ + item('car-1', 'id-red'), + item('car-2', 'id-red'), + item('car-3', 'id-gone'), + item('car-4', DANGLING), + item('car-5'), + ]); + + const byLane = cardsByLane(screen); + // PRECONDITION: every card is somewhere, so a lane reading 0 is a + // misplacement rather than a card that never rendered. + expect(Object.values(byLane).reduce((a, b) => a + b, 0)).toBe(5); + expect(byLane['COLOR-1Red']).toBe(2); + expect(byLane['COLOR-9Gone(deleted)']).toBe(1); + expect(byLane['Unresolvedreference']).toBe(1); + expect(byLane['Uncategorized']).toBe(1); + }); + + it('places a PADDED legacy value in the same lane as its clean form', () => { + // The seam the trim closed, asserted where a user would see it. + const screen = renderBoard([item('car-1', 'id-red'), item('car-2', ' id-red ')]); + expect(cardsByLane(screen)['COLOR-1Red']).toBe(2); + }); + + it('does not turn the card\'s status chip into a RELATION setter', () => { + // codex round 3, P1. Cards were handed `statusOptions={columns}`, which + // is fine while a lane value is a status option — under relation + // grouping the lanes are ITEM IDS, and the parent handler writes what it + // receives into `fields[groupField]`. So a click on the status chip set + // the card's relation, cycling through target ids and able to land on + // the deleted or unresolved lane, which the write path then refuses. + // The collection needs a STATUS field as well as the relation, or the + // card renders no status chip for an unrelated reason and the test + // cannot discriminate. Measured: without it the mutant survived. + const coll = collection(); + coll.schema = JSON.stringify({ + fields: [ + { key: 'car_color', label: 'Colour', type: 'relation', collection: 'colors' }, + { key: 'status', label: 'Status', type: 'select', options: ['open', 'done'] }, + ], + }); + const withStatus = (id: string, color: string) => + ({ + ...item(id, color), + fields: JSON.stringify({ car_color: color, status: 'open' }), + }) as Item; + + const screen = render(BoardView, { + props: { + items: [withStatus('car-1', 'id-red'), withStatus('car-2', 'id-blue')], + collection: coll, + wsSlug: 'ws', + groupField: 'car_color', + onStatusChange: vi.fn(), + } as never, + }); + + // PRECONDITION: the cards rendered, so "no chip" is not "no card". + expect(screen.container.querySelectorAll('.item-card')).toHaveLength(2); + expect(screen.container.querySelector('[title="Click to cycle status"]')).toBeNull(); + }); + + it('STILL offers status cycling on an ordinary board — the counterfactual', () => { + // Withholding it everywhere would remove a working affordance from every + // board on the instance, which is worse than the defect. + const coll = collection(); + coll.schema = JSON.stringify({ + fields: [{ key: 'status', label: 'Status', type: 'select', options: ['open', 'done'] }], + }); + const withStatus = (id: string) => + ({ + id, + workspace_id: 'ws1', + collection_id: 'c1', + slug: id, + title: id, + content: '', + fields: JSON.stringify({ status: 'open' }), + tags: '[]', + status: 'open', + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + }) as unknown as Item; + + const screen = render(BoardView, { + props: { + items: [withStatus('car-1')], + collection: coll, + wsSlug: 'ws', + groupField: 'status', + onStatusChange: vi.fn(), + } as never, + }); + + expect(screen.container.querySelector('[title="Click to cycle status"]')).not.toBeNull(); + }); + + it('labels a live lane with REF and title, not the stored id', () => { + const screen = renderBoard([item('car-1', 'id-red')]); + + // PRECONDITION: the lane exists at all. A relation field has no + // `options`, so before this unit every card landed in UNCATEGORIZED and + // there was no lane to label. + expect(lanes(screen)).toContainEqual({ ref: 'COLOR-1', note: null, name: 'Red' }); + expect(screen.container.innerHTML).not.toContain('id-red'); + }); + + it('keeps a DELETED target as its own lane, and says so', () => { + const screen = renderBoard([item('car-1', 'id-red'), item('car-2', 'id-gone')]); + const html = screen.container.innerHTML; + + expect(lanes(screen)).toContainEqual({ ref: 'COLOR-9', note: '(deleted)', name: 'Gone' }); + // And not merged with the live one, which keeps its own honest label. + expect(lanes(screen)).toContainEqual({ ref: 'COLOR-1', note: null, name: 'Red' }); + expect(html).not.toContain('id-gone'); + }); + + it('renders a dangling value as an honest lane, never the bare id', () => { + const screen = renderBoard([item('car-1', DANGLING)]); + const html = screen.container.innerHTML; + + expect(lanes(screen)).toContainEqual({ ref: null, note: null, name: 'Unresolved reference' }); + expect(html).not.toContain(DANGLING); + expect(html).not.toContain('f47ac10b'); + }); + + it('does NOT group by relation when the field is an ordinary select', () => { + // The counterfactual for the whole branch: a select must still bucket + // against its own options, and a board that asked the local index for + // lanes on every field type would relabel ordinary lanes with whatever + // an id-shaped option resolved to. + const coll = collection(); + coll.schema = JSON.stringify({ + fields: [{ key: 'car_color', label: 'Colour', type: 'select', options: ['id-red'] }], + }); + const screen = render(BoardView, { + props: { + items: [item('car-1', 'id-red')], + collection: coll, + wsSlug: 'ws', + groupField: 'car_color', + onStatusChange: vi.fn(), + } as never, + }); + + const html = screen.container.innerHTML; + expect(html).not.toContain('COLOR-1'); + // formatLabel's own casing of the option value, not a resolved title. + expect(lanes(screen)).toContainEqual({ ref: null, note: null, name: 'Id-Red' }); + }); +}); + +describe('the drop gate on a relation lane', () => { + /** + * A SOURCE guard, and the header says why rather than leaving it implied: + * the gate lives inside `commitColumnMove`, which is reached through + * svelte-dnd-action's drag events, and driving those in jsdom tests the + * drag library rather than this decision. Measured first — removing the + * gate leaves every rendering test in this file green, so without something + * here the wiring is unobserved (CONVE-19). + * + * WHAT A SOURCE GUARD CANNOT DO: it checks spellings, not behaviour. A gate + * that consulted the wrong lane, or one made unreachable by an earlier + * return, would still pass — and the rollback below is asserted as an + * ASSIGNMENT rather than as a board that visibly snaps back. The honest + * instrument for that is a drag driven through svelte-dnd-action, which + * tests the drag library as much as this decision. + */ + it('puts the card BACK when it refuses the drop', () => { + // codex round 1, P1, and the most important mutant this file missed: + // svelte-dnd-action has already moved the card in `columnData` by the + // time the gate runs, so refusing the WRITE without undoing that leaves + // the board showing a move that never happened — the exact failure the + // refusal exists to avoid, one step later. Every rendering test stayed + // green through it. + const start = SRC.indexOf('async function commitColumnMove('); + const body = SRC.slice(start, SRC.indexOf('\n\t}', start)); + const gate = body.indexOf('relationLaneAcceptsDrop'); + // Matched as "reassigns columnData from propColumnData", not as one exact + // spelling: the restore became a COPY in round 6 (the two aliased, so + // assigning the derived back restored nothing), and a guard pinned to + // the old literal failed the fix rather than the defect. + const restore = body.indexOf('columnData ='); + expect(restore, 'the refused drop is not reverted').toBeGreaterThan(-1); + expect(restore).toBeGreaterThan(gate); + expect(restore).toBeLessThan(body.indexOf('onStatusChange(')); + expect(body.slice(restore, body.indexOf('onStatusChange('))).toContain('propColumnData'); + }); + + it('consults relationLaneAcceptsDrop BEFORE calling onStatusChange', () => { + const start = SRC.indexOf('async function commitColumnMove('); + expect(start, 'commitColumnMove was renamed or removed').toBeGreaterThan(-1); + const body = SRC.slice(start, SRC.indexOf('\n\t}', start)); + + const gate = body.indexOf('relationLaneAcceptsDrop'); + const write = body.indexOf('onStatusChange('); + expect(gate, 'the relation drop gate is gone').toBeGreaterThan(-1); + expect(write).toBeGreaterThan(-1); + expect(gate, 'the write happens before the gate').toBeLessThan(write); + }); + + it('gates on isRelationGroup, so an ordinary board is untouched', () => { + // THE EIGHTH WRONG-REASON TEST (codex round 6). It asserted only that + // the source CONTAINS `if (isRelationGroup)` — a mutant leaving that + // block empty and calling `onStatusChange` unconditionally would pass. + // It now requires the refusal to live INSIDE that block, ahead of the + // write. + const start = SRC.indexOf('async function commitColumnMove('); + const body = SRC.slice(start, SRC.indexOf('\n\t}', start)); + const gate = body.indexOf('if (isRelationGroup)'); + const refuse = body.indexOf('relationLaneAcceptsDrop'); + const write = body.indexOf('onStatusChange('); + expect(gate).toBeGreaterThan(-1); + expect(refuse).toBeGreaterThan(gate); + expect(refuse).toBeLessThan(write); + // and the block returns rather than falling through + expect(body.slice(gate, write)).toContain('return;'); + }); +}); + +describe('a relation field with no declared target collection', () => { + it('is NOT grouped as a relation', () => { + // `narrowRelationRow` skips the collection check when there is nothing + // to check against, so a legacy or half-written relation field would + // resolve ids ANYWHERE in the workspace and label lanes with whatever it + // found. The filter UI already required a target; the board did not + // (codex round 2). + const coll = collection(); + coll.schema = JSON.stringify({ + fields: [{ key: 'car_color', label: 'Colour', type: 'relation' }], + }); + const screen = render(BoardView, { + props: { + items: [item('car-1', 'id-red')], + collection: coll, + wsSlug: 'ws', + groupField: 'car_color', + onStatusChange: vi.fn(), + } as never, + }); + + // No lane resolved from the workspace-wide index — the ref is the tell. + expect(screen.container.querySelector('.lane-ref')).toBeNull(); + }); + + it('withholds the lane menu\'s "add" for a relation lane, like the visible +', () => { + // Otherwise the menu is an undocumented second creation path into a + // lane whose "+" was deliberately withheld, and it contradicts this + // component's own stated design rather than merely duplicating it. + const start = SRC.indexOf('onAddItem={onCreateInColumn'); + expect(start, 'the lane menu no longer passes onAddItem').toBeGreaterThan(-1); + expect(SRC.slice(start, start + 160)).toContain('!isRelationGroup'); + }); +}); + +describe('the MENU move into a refused relation lane', () => { + /** + * codex round 6, P2 — and the reason this leg is rendered rather than + * source-guarded like its drag sibling. + * + * Round 6 reported that `moveItem`'s `columnData[col] = ...` writes THROUGH + * to `propColumnData`'s cached object, so restoring by assigning it back + * restores nothing. That premise is FALSE (settled in round 7, see the + * failure-exit block at the end of this file): the alias mutant SURVIVES + * this test, and a direct probe of the shape reports the derived's cache + * unchanged by the write. + * + * What this test does prove — verified by a gate mutant killing it — is + * that it drives the refusal path for real. No source guard could have done + * that; it took driving the menu. + */ + const cardsIn = (screen: { container: HTMLElement }, laneName: string) => { + for (const col of screen.container.querySelectorAll('.kanban-column')) { + const name = (col.querySelector('.column-name')?.textContent ?? '').replace(/\s+/g, ''); + if (name === laneName) return [...col.querySelectorAll('.card-title')].map((e) => e.textContent?.trim()); + } + return null; + }; + + it('leaves the card where it was', async () => { + const onStatusChange = vi.fn(); + const screen = render(BoardView, { + props: { + items: [item('car-1', 'id-red'), item('car-2', 'id-gone')], + collection: collection(), + wsSlug: 'ws', + groupField: 'car_color', + onStatusChange, + onReorder: vi.fn(), + } as never, + }); + + // PRECONDITION: two lanes, each holding its own card, and the deleted + // lane is to the RIGHT of the live one (lanes sort live-then-deleted). + expect(cardsIn(screen, 'COLOR-1Red')).toEqual(['car-1']); + expect(cardsIn(screen, 'COLOR-9Gone(deleted)')).toEqual(['car-2']); + + // Open car-1's action menu and move it right, into the deleted lane. + // The card's own ⋮ (ItemActionsMenu), not the lane's ⋯ — and the menu may + // render into , so the search below is document-wide. + const cardMenuButtons = [...screen.container.querySelectorAll('.item-card button')].filter( + (b) => (b.textContent ?? '').trim() === '⋮', + ) as HTMLButtonElement[]; + expect(cardMenuButtons.length, 'no card action menu trigger — re-point this test').toBeGreaterThan(0); + cardMenuButtons[0].click(); + await tick(); + await tick(); + + const moveRight = [...document.querySelectorAll('button')].find((b) => + (b.textContent ?? '').includes('Move right'), + ); + expect(moveRight, '"Move right" was not offered — re-point this test').toBeTruthy(); + moveRight!.click(); + await tick(); + await tick(); + + // The write was refused AND the board did not keep the card there. + expect(onStatusChange).not.toHaveBeenCalled(); + expect(cardsIn(screen, 'COLOR-1Red')).toEqual(['car-1']); + expect(cardsIn(screen, 'COLOR-9Gone(deleted)')).toEqual(['car-2']); + }); +}); + +describe('the MENU move whose WRITE fails', () => { + /** + * codex round 7, P1 — REFUTED, and this test is what refutes it. + * + * Rounds 6 and 7 both reasoned that the sync `$effect` assigns the + * `$derived.by` VALUE into `columnData`, so `moveItem`'s + * `columnData[col] = …` writes THROUGH to the derived cache — round 6 + * against the refusal exit, round 7 against this one, the failure exit + * where `onStatusChange` rejects and the restore rests on + * `dropCooldown = false` alone. + * + * The premise does not hold. A probe over exactly that shape — a + * `$derived.by` object assigned into a `$state`, a property written on the + * `$state`, then the derived read back — reported the derived's cached + * value UNCHANGED (`after mutation, derived.a=[1]`), and the restore then + * produced the original lanes. A `$state` holding a derived's object is a + * deep proxy; its property writes do not reach what the derivation cached. + * + * So this test passes against the tree as written, and that is the finding + * rather than a fix. It is kept because the failure exit had NO behavioural + * coverage before it and because it discriminates: removing + * `dropCooldown = false` turns it red (measured), so its green is evidence + * that the cooldown release is what restores the board, not an accident of + * a card that never moved — which the `toHaveBeenCalledTimes(1)` + * precondition rules out separately. + * + * The class is every exit from `commitColumnMove` that must restore lanes + * after `moveItem` mutated them. There are two, the refusal and the + * failure; both are now covered, and neither is defective. + */ + const cardsIn = (screen: { container: HTMLElement }, laneName: string) => { + for (const col of screen.container.querySelectorAll('.kanban-column')) { + const name = (col.querySelector('.column-name')?.textContent ?? '').replace(/\s+/g, ''); + if (name === laneName) return [...col.querySelectorAll('.card-title')].map((e) => e.textContent?.trim()); + } + return null; + }; + + it('puts the card back when onStatusChange rejects', async () => { + const onStatusChange = vi.fn().mockRejectedValue(new Error('nope')); + const screen = render(BoardView, { + props: { + items: [item('car-1', 'id-blue'), item('car-2', 'id-red')], + collection: collection(), + wsSlug: 'ws', + groupField: 'car_color', + onStatusChange, + onReorder: vi.fn(), + } as never, + }); + + // PRECONDITION: two LIVE lanes, sorted by title, each holding its card. + expect(cardsIn(screen, 'COLOR-2Blue')).toEqual(['car-1']); + expect(cardsIn(screen, 'COLOR-1Red')).toEqual(['car-2']); + + const cardMenuButtons = [...screen.container.querySelectorAll('.item-card button')].filter( + (b) => (b.textContent ?? '').trim() === '⋮', + ) as HTMLButtonElement[]; + expect(cardMenuButtons.length, 'no card action menu trigger — re-point this test').toBeGreaterThan(0); + cardMenuButtons[0].click(); + await tick(); + await tick(); + + const moveRight = [...document.querySelectorAll('button')].find((b) => + (b.textContent ?? '').includes('Move right'), + ); + expect(moveRight, '"Move right" was not offered — re-point this test').toBeTruthy(); + moveRight!.click(); + await tick(); + await tick(); + await tick(); + + // PRECONDITION for the assertion below: the write was ATTEMPTED and + // failed. Without this, a board that never moved the card would pass. + expect(onStatusChange).toHaveBeenCalledTimes(1); + + // The write failed, so the board must show the state it started in. + expect(cardsIn(screen, 'COLOR-2Blue')).toEqual(['car-1']); + expect(cardsIn(screen, 'COLOR-1Red')).toEqual(['car-2']); + }); +}); diff --git a/web/src/lib/components/collections/BoardView.svelte b/web/src/lib/components/collections/BoardView.svelte index b37185215..75bc89352 100644 --- a/web/src/lib/components/collections/BoardView.svelte +++ b/web/src/lib/components/collections/BoardView.svelte @@ -4,6 +4,16 @@ import { itemComparator, type SortMode } from '$lib/collections/itemSort'; import { reorderGroup, disabledDirections, adjacentColumn, type ReorderDirection } from '$lib/collections/reorder'; import { bucketByColumn, UNCATEGORIZED } from '$lib/collections/boardColumns'; + import { + narrowRelationRow, + relationLaneAcceptsDrop, + relationLaneAriaName, + relationLaneValueFor, + relationLanes, + type RelationLane, + } from '$lib/collections/relationGroups'; + 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 type { DndEvent } from 'svelte-dnd-action'; @@ -191,9 +201,61 @@ let schema = $derived(parseSchema(collection)); let field = $derived(schema.fields.find((f) => f.key === groupField)); - let columns = $derived(field?.options ?? []); + + // GROUPING BY A RELATION (TASK-2998 / PLAN-2857 U7). Every other groupable + // type has an option list to bucket against; a relation's lanes are the + // targets the rows point at, so they are derived from the items and + // resolved through the local index. The rules — ordering, what a deleted or + // dangling target is labelled, which lanes accept a drop — live in + // `$lib/collections/relationGroups`, because they are decisions and a + // decision reachable only by mounting a board is one nobody tests. + // A DECLARED TARGET IS REQUIRED (codex round 2). `narrowRelationRow` skips + // the collection check when there is nothing to check against, so a legacy + // or half-written relation field with no `collection` would resolve ids + // ANYWHERE in the workspace and label lanes with whatever it found. The + // filter UI already requires it; the board did not. + let isRelationGroup = $derived(field?.type === 'relation' && !!field?.collection); + let knownCollectionSlugs = $derived( + new Set(collectionStore.collections.map((c) => c.slug)), + ); + let resolveRelation = $derived((id: string) => + wsSlug + ? narrowRelationRow( + localIndex.findByIdOrSlug(wsSlug, id), + id, + field?.collection, + knownCollectionSlugs, + ) + : null, + ); + let relationLaneList = $derived( + isRelationGroup ? relationLanes(items, groupField, resolveRelation) : [], + ); + let relationLaneByValue = $derived( + new Map(relationLaneList.map((lane) => [lane.value, lane])), + ); + let columns = $derived( + isRelationGroup ? relationLaneList.map((lane) => lane.value) : (field?.options ?? []), + ); // Column order state — tracks the displayed order, syncs from schema when not dragging + /** + * What a CARD's status chip cycles through — the board's lanes for every + * ordinary grouping, and nothing at all when the lanes are relation targets + * (codex round 3, P1). + * + * `statusOptions={columns}` was fine while a lane value was always a status + * option. Under relation grouping the lanes are ITEM IDS, and the parent's + * handler writes whatever it receives into `fields[groupField]` — so a + * click on the status chip set the card's RELATION, cycling through target + * ids and able to land on the deleted or unresolved lane, which the write + * path then refuses. The chip is withheld rather than repointed at the real + * status field: a status chip on a relation-grouped board would be cycling + * a field the board is not showing, which is a different feature and not + * one this unit was asked for. + */ + let cardStatusOptions = $derived(isRelationGroup ? [] : columns); + let columnOrder = $state([]); $effect(() => { @@ -257,7 +319,12 @@ let propColumnData = $derived.by(() => { // Bucket items into their lanes, routing empty/unknown-value items // into the UNCATEGORIZED ('') lane instead of dropping them (IDEA-2275). - const result = bucketByColumn(items, groupField, columns); + const result = bucketByColumn( + items, + groupField, + columns, + isRelationGroup ? (item) => relationLaneValueFor(item, groupField, resolveRelation) : undefined, + ); // `preserveOrder` opts out of the in-column sort so search rank // from the parent isn't overridden — TASK-1367. Otherwise sort // each lane by its effective mode — the per-lane override if set, @@ -363,11 +430,63 @@ // dnd-provided target order (e.detail.items); the menu passes 'top' (the // card has already been inserted at the target lane's head). DR-7. async function commitColumnMove(item: Item, targetColumn: string, placement: Item[] | 'top') { + // A RELATION LANE ONLY ACCEPTS A DROP WHEN ITS TARGET IS LIVE + // (TASK-2998). Writing the value of the uncategorised, deleted or + // unresolved lanes would either clear the field or write a reference + // TASK-2878's validator refuses — so the card would move on screen and + // the write would fail behind it, which is worse than not moving. + if (isRelationGroup) { + const lane = relationLaneByValue.get(targetColumn); + if (!lane || !relationLaneAcceptsDrop(lane)) { + // AND PUT THE CARD BACK (codex round 1, P1). By the time this + // runs, svelte-dnd-action has already moved the card in + // `columnData` — refusing the WRITE without undoing that leaves + // the board showing a move that never happened, which is the + // exact failure the refusal exists to avoid, one step later. + // + // Re-reading `propColumnData` is how the failure path below + // reverts too: it is the props-derived truth, and the sync + // effect is gated on the cooldown rather than owning the + // restore. Assigning here rather than releasing a cooldown + // nobody set is the same repair without the two-second window. + // + // COPIED rather than aliased — defensive, and NOT for the + // reason rounds 6 and 7 gave. Both rounds argued that the sync + // effect assigns the derived VALUE into `columnData`, so a + // later `columnData[col] = ...` writes THROUGH to + // `propColumnData`'s cached object and assigning it back + // restores nothing. That premise is FALSE, measured rather than + // argued: a `$state` assigned a `$derived`'s object is a deep + // proxy whose property writes do not reach the derived's cache. + // A probe over exactly this shape ($derived.by object → $state + // → property write → read the derived) reported + // `after mutation, derived.a=[1]`, the pre-mutation value. The + // rendered tests agree from the other side: the round-6 menu + // test survives the alias mutant, and the failure-exit test + // below passes on the cooldown release alone while going red + // when that release is removed. + // So the copy buys nothing against aliasing; it stays only + // because a restore path should not hand its caller an object + // somebody else owns. Do not re-derive the aliasing story from + // the shape of this code — it has now cost two review rounds. + columnData = Object.fromEntries( + Object.entries(propColumnData).map(([key, list]) => [key, [...list]]), + ); + return; + } + } dropCooldown = true; let moveSucceeded = true; const fields = parseFields(item); - if (fields[groupField] !== targetColumn) { + // TRIMMED for a relation, as the list already was (codex round 6). A + // legacy value of `" id-red "` is ALREADY in the `id-red` lane, and a + // raw `!==` fired a pointless write for it — the same asymmetry between + // these two views, in the other direction this time. + const currentValue = isRelationGroup + ? relationLaneValueFor(item, groupField, resolveRelation) + : fields[groupField]; + if (currentValue !== targetColumn) { try { await onStatusChange(item, targetColumn); } catch { @@ -483,14 +602,24 @@ {#each renderColumns as colValue (colValue)} {@const colItems = columnData[colValue] ?? []} {@const isUncategorized = colValue === UNCATEGORIZED} - {@const colDraggable = canEdit && !isUncategorized} + {@const relLane = relationLaneByValue.get(colValue)} + {@const laneName = relLane ? (relLane.title ?? relLane.label) : formatLabel(colValue)} + {@const laneRef = relLane?.ref ?? null} + + {@const colDraggable = canEdit && !isUncategorized && !isRelationGroup}
handleColumnDragOver(e, colValue)} ondragleave={handleColumnDragLeave} ondrop={(e) => handleColumnDrop(e, colValue)} @@ -507,7 +636,10 @@ {#if colDraggable} ⠿ {/if} - {formatLabel(colValue)} + + {#if laneRef}{laneRef}{/if}{laneName} + {#if relLane?.state === 'deleted'}(deleted){/if} +
{colItems.length} - {#if onCreateInColumn && !isUncategorized} + {#if onCreateInColumn && !isUncategorized && !isRelationGroup} + {#if active} + + {/if} + {#if openPickerFor === rf.key} + + {/if} +
+ {/each} + {#if unparentedAvailable}