diff --git a/.changeset/8993-kanban-lane-id-sweep-key.md b/.changeset/8993-kanban-lane-id-sweep-key.md new file mode 100644 index 0000000000..ec84065984 --- /dev/null +++ b/.changeset/8993-kanban-lane-id-sweep-key.md @@ -0,0 +1,38 @@ +--- +'@object-ui/plugin-kanban': patch +'@object-ui/types': patch +--- + +Fix `bucketCardsIntoColumns` double-bucketing every record on a board whose lane +ids are not strings (objectui#8993). + +The function decided lane membership **twice**, with two key types: + +- **injection** reads `groups[col.id]`, and a property read coerces its key — a + lane `{ id: 1 }` correctly picks up the group stored under `'1'`; +- **the leftover sweep** (objectui#2792) built its known-id `Set` from the RAW + `col.id` and filtered `Object.keys(groups)`, which are always strings. Since + `new Set([1]).has('1')` is `false`, every record the injection had just placed + was swept a second time into the trailing "Uncategorized" lane. + +⇒ A numeric-id board rendered **every card twice**. Silent by nature: the board +draws, only the totals fail to reconcile. + +The repair is on the sweep side — the set now holds the same key spelling the +property read uses, so both decisions agree. ⛔ Not on the injection side: +making that read strict would break the coercion lanes depend on, and the string +control could not catch it. A symbol id is the one key a property read does not +stringify, so it is kept as-is rather than pushed through `String()`, which +throws on symbols; `Object.keys` never yields a symbol, so such a lane keeps the +reading it has today. + +**Visible change.** A board whose metadata already hits this — a picklist whose +option `value` is a number, which reaches the renderer as a lane id through a +path no schema guards — showed each card twice, once in its lane and once in +"Uncategorized". It now shows each card once. The trailing lane still collects +records whose group value matches no lane, which is objectui#2792's job and is +pinned alongside the repair. + +`@object-ui/types` carries prose only: the lane-id `describe()` and TSDoc that +stated the double-render as current behaviour now state it as repaired. The +authored `id` stays `string` — one declared lane-id type beats two. diff --git a/content/docs/api/schema-reference.md b/content/docs/api/schema-reference.md index f285fc4d62..f4549b1ebc 100644 --- a/content/docs/api/schema-reference.md +++ b/content/docs/api/schema-reference.md @@ -945,7 +945,7 @@ A drag-and-drop Kanban board. The `object-kanban` type key validates the shape t > `groupField` is refused by name (objectui#7322): the renderer reads `groupBy`. -> `columns` is declared on this face since objectui#8913, as the pair of array shapes `@objectstack/spec` declares — an array of `{ id, title }` lanes, **or** an array of bare value strings. A **mixed** array is refused: the renderer decides which shape it has from the first element alone, so a mix yields a blank lane and mis-bucketed cards. A lane accepts `id`, `title`, `cards`, `limit`, `className` and `collapsed`, which are the members the board implementations read; `id` is a **string** — a non-string lane id makes the board render every card twice, once in its lane and once in "Uncategorized" (objectui#8993). When a lane carries `cards`, each card is judged — a card with no `title` is refused. An undeclared lane key is accepted and dropped, not refused, which is this tolerant face's posture; the strict authoring face refuses it by name. +> `columns` is declared on this face since objectui#8913, as the pair of array shapes `@objectstack/spec` declares — an array of `{ id, title }` lanes, **or** an array of bare value strings. A **mixed** array is refused: the renderer decides which shape it has from the first element alone, so a mix yields a blank lane and mis-bucketed cards. A lane accepts `id`, `title`, `cards`, `limit`, `className` and `collapsed`, which are the members the board implementations read; `id` is a **string** — the authored face keeps that narrowing, and since objectui#8993 a non-string lane id no longer renders every card twice: the bucketer's leftover sweep keys membership the way the injection already did (a lane `1` takes the group `'1'`). When a lane carries `cards`, each card is judged — a card with no `title` is refused. An undeclared lane key is accepted and dropped, not refused, which is this tolerant face's posture; the strict authoring face refuses it by name. > > ⚠️ The **bare-string array is accepted but inert on this block.** It is declared so this package does not refuse an authoring the protocol allows. The renderer reads a bare-string lane list only when a board has no `groupBy`, and `groupBy` is required here — so on `object-kanban` the strings are always ignored and the lanes come from the group field's picklist options or from the data. Write the `{ id, title }` array to control the lanes. The requiredness of `groupBy` is tracked as objectui#8990. > diff --git a/content/docs/plugins/plugin-kanban.mdx b/content/docs/plugins/plugin-kanban.mdx index 6a0a6e0529..4a05be0d40 100644 --- a/content/docs/plugins/plugin-kanban.mdx +++ b/content/docs/plugins/plugin-kanban.mdx @@ -149,7 +149,7 @@ An undeclared lane key is accepted and dropped, not refused. | Property | Type | Description | |----------|------|-------------| -| `id` | string | Lane identifier, matched against the `groupBy` value. Strings only: a non-string id makes the board render every card twice, once in its lane and once in "Uncategorized" (objectui#8993) | +| `id` | string | Lane identifier, matched against the `groupBy` value. Declare a string; since objectui#8993 a non-string id is matched by its string spelling (a lane `1` takes the group `'1'`) instead of rendering every such card twice | | `title` | string | Lane title, localized against the `groupBy` picklist's option labels | | `cards` | KanbanCard[] | Cards this lane carries — **optional**, and a static board's option; an object-bound board's cards arrive from the record source | | `limit` | number | Max cards allowed (WIP limit). Never reaches the query — the fetch window is the board's `limit` | diff --git a/packages/plugin-kanban/src/__tests__/laneIdCoercion-8993.test.ts b/packages/plugin-kanban/src/__tests__/laneIdCoercion-8993.test.ts new file mode 100644 index 0000000000..68682a86e3 --- /dev/null +++ b/packages/plugin-kanban/src/__tests__/laneIdCoercion-8993.test.ts @@ -0,0 +1,180 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#8993 — `bucketCardsIntoColumns` decides lane membership TWICE, and + * before this pin the two decisions used different key types. + * + * ## The mechanism + * + * - **Injection** reads `groups[col.id]`. A property read coerces its key, so + * a lane `{ id: 1 }` correctly picks up the group stored under `'1'`. + * - **The leftover sweep** (#2792) built its known-id Set from the RAW value + * and filtered `Object.keys(groups)` — which are ALWAYS strings. Since + * `new Set([1]).has('1')` is `false`, every record the injection had just + * placed was swept a second time into the trailing "Uncategorized" lane. + * + * ⇒ A numeric-id board rendered every card twice. Silent: the board draws, the + * totals just do not reconcile, which is why the counting row below exists. + * + * ## What each row is for + * + * 1. NUMERIC — the reported reading. Was `'1:r1, 2:r2, __uncolumned__:r1+r2'`. + * 2. STRING CONTROL — a live control, green on BOTH sides of the repair. It is + * what makes row 1 a statement about the id TYPE rather than about bucketing + * in general; if it ever reddens, the repair went the wrong way. + * 3. THE CLASS — the defect is "the id is not a string", not "the id is a + * number", so every non-string id type the function can be handed is pinned, + * including the falsy ones (`0`, `false`) that also skip the label map. + * 4. THE LABEL PATH — a record can reach `groups` through the label→id map, + * which stores the RAW id as its VALUE. That path double-bucketed too, so + * one row keeps it honest; keying only the direct path would leave it open. + * 5. THE SWEEP STILL SWEEPS — the control that separates a repair from a + * deletion. Rows 1-4 would all pass if the sweep were simply removed, and + * that would resurrect #2792 (records silently dropped). An unmatched record + * must still surface in the trailing lane. + * 6. NO NEW COLLISION — coercing a key can MINT a collision where there was + * none, so the boundary is asserted rather than argued: `String(null)` is + * `'null'` and `String(undefined)` is `'undefined'`, and neither may swallow + * the empty key that a null/absent VALUE produces; a lane declared with the + * `KANBAN_UNCOLUMNED_ID` spelling keeps its existing reading; and a symbol + * id — the one key a property read does NOT stringify — must keep behaving + * as it does today instead of throwing inside `String()`. + * + * ⛔ The repair belongs on the SWEEP side. Making the injection strict would + * pass rows 1-2 while breaking the coercion lanes depend on, and the string + * control could not catch it. + */ + +import { describe, it, expect } from 'vitest'; +import { bucketCardsIntoColumns, KANBAN_UNCOLUMNED_ID } from '../index'; + +/** `id:card+card` per lane, in lane order — the whole board in one string. */ +const summarise = (lanes: Array): string => + lanes + .map((l: any) => `${String(l.id)}:${(l.cards || []).map((c: any) => c.id).join('+')}`) + .join(', '); + +const bucket = (columns: Array, data: Array): string => + summarise(bucketCardsIntoColumns(columns, data, 'status', undefined, 'Uncategorized')); + +/** Every card id the board renders, across every lane, duplicates included. */ +const renderedCardIds = (columns: Array, data: Array): string[] => + bucketCardsIntoColumns(columns, data, 'status', undefined, 'Uncategorized').flatMap( + (l: any) => (l.cards || []).map((c: any) => c.id), + ); + +describe('bucketCardsIntoColumns — lane id key coercion (objectui#8993)', () => { + it('row 1 — NUMERIC lane ids bucket each record exactly once', () => { + expect( + bucket( + [ + { id: 1, title: 'One' }, + { id: 2, title: 'Two' }, + ], + [ + { id: 'r1', status: 1 }, + { id: 'r2', status: '2' }, + ], + ), + ).toBe('1:r1, 2:r2'); + }); + + it('row 2 — STRING control: unchanged, green on both sides of the repair', () => { + expect(bucket([{ id: 'one', title: 'One' }], [{ id: 'r1', status: 'one' }])).toBe('one:r1'); + }); + + describe('row 3 — the class is "id is not a string", not "id is a number"', () => { + const cases: Array<{ name: string; id: unknown; value: unknown; lane: string }> = [ + { name: 'number', id: 1, value: 1, lane: '1' }, + { name: 'zero (falsy, so the label map skips it)', id: 0, value: 0, lane: '0' }, + { name: 'boolean true', id: true, value: true, lane: 'true' }, + { name: 'boolean false (falsy)', id: false, value: false, lane: 'false' }, + { name: 'null', id: null, value: 'null', lane: 'null' }, + { name: 'undefined (a lane with no id at all)', id: undefined, value: 'undefined', lane: 'undefined' }, + { name: 'NaN', id: NaN, value: NaN, lane: 'NaN' }, + { name: 'array', id: [1, 2], value: '1,2', lane: '1,2' }, + { name: 'plain object', id: { a: 1 }, value: '[object Object]', lane: '[object Object]' }, + ]; + + for (const { name, id, value, lane } of cases) { + it(`${name} — one lane, one card, no trailing "Uncategorized"`, () => { + const columns = [{ id, title: 'Lane' }]; + const data = [{ id: 'r1', status: value }]; + expect(bucket(columns, data)).toBe(`${lane}:r1`); + // The counting row: the visible card total must reconcile with the + // record count. This is the user-visible symptom, stated directly. + expect(renderedCardIds(columns, data)).toEqual(['r1']); + }); + } + }); + + it('row 4 — the label→id path is keyed the same way (it stores the RAW id)', () => { + // 'One' matches the lane's TITLE, so the group key comes out of + // `labelToColumnId`, whose stored VALUE is the raw numeric id. + expect(bucket([{ id: 1, title: 'One' }], [{ id: 'r1', status: 'One' }])).toBe('1:r1'); + }); + + it('row 5 — the sweep still sweeps: an unmatched record still surfaces (#2792)', () => { + // A repair that simply deleted the sweep would pass every row above and + // silently drop this record — the defect #2792 closed. + expect( + bucket( + [ + { id: 1, title: 'One' }, + { id: 2, title: 'Two' }, + ], + [ + { id: 'r1', status: 1 }, + { id: 'r9', status: 'retired_option' }, + ], + ), + ).toBe(`1:r1, 2:, ${KANBAN_UNCOLUMNED_ID}:r9`); + }); + + describe('row 6 — the coercion mints no new collision', () => { + it("a null lane id does not swallow the empty key a null VALUE produces", () => { + // `String(null)` is 'null', but a record whose group VALUE is null is + // keyed '' (`String(item[groupBy] ?? '')`). The two must stay apart. + expect(bucket([{ id: null, title: 'N' }], [{ id: 'r1', status: null }])).toBe( + `null:, ${KANBAN_UNCOLUMNED_ID}:r1`, + ); + }); + + it("an undefined lane id does not swallow the empty key an ABSENT value produces", () => { + expect(bucket([{ title: 'U' }], [{ id: 'r1' }])).toBe( + `undefined:, ${KANBAN_UNCOLUMNED_ID}:r1`, + ); + }); + + it('a lane declared with the sentinel spelling keeps its own records', () => { + const lanes = bucketCardsIntoColumns( + [{ id: KANBAN_UNCOLUMNED_ID, title: 'Declared' }], + [{ id: 'r1', status: KANBAN_UNCOLUMNED_ID }], + 'status', + undefined, + 'Uncategorized', + ); + expect(summarise(lanes)).toBe(`${KANBAN_UNCOLUMNED_ID}:r1`); + expect(lanes).toHaveLength(1); // no second lane under the same id + }); + + it("a symbol lane id keeps today's reading instead of throwing", () => { + // A symbol is the one key a property read does NOT stringify, so it is + // never pushed through `String()` (which throws on symbols). `Object.keys` + // never yields a symbol, so such a lane matches nothing — exactly as + // before this repair. + const columns = [{ id: Symbol('lane'), title: 'S' }]; + const data = [{ id: 'r1', status: 'x' }]; + expect(() => bucketCardsIntoColumns(columns, data, 'status', undefined, 'Uncategorized')) + .not.toThrow(); + expect(renderedCardIds(columns, data)).toEqual(['r1']); + expect(bucket(columns, data)).toBe(`Symbol(lane):, ${KANBAN_UNCOLUMNED_ID}:r1`); + }); + }); +}); diff --git a/packages/plugin-kanban/src/index.tsx b/packages/plugin-kanban/src/index.tsx index 7ec8e6479e..2fc0ea484f 100644 --- a/packages/plugin-kanban/src/index.tsx +++ b/packages/plugin-kanban/src/index.tsx @@ -97,7 +97,20 @@ export function bucketCardsIntoColumns( // still counts. Surface them in a trailing "Uncategorized" lane; dragging // one out to a real column repairs its status (the drag handler refuses to // persist a move INTO here). - const knownIds = new Set(columns.map((col: any) => col.id)); + // ⚠️ Key this membership test the way the injection above keys its READ. + // `groups[col.id]` is a property read, so it coerces the id: a lane + // `{ id: 1 }` correctly picks up the group stored under `'1'`, and every key + // `Object.keys(groups)` yields is a string. A Set built from the RAW id + // therefore answers `new Set([1]).has('1') === false` and sweeps the very + // records the injection already took — the board renders each of them twice, + // once in its lane and once in "Uncategorized" (objectui#8993). Membership is + // decided twice here, so both decisions must use the same key spelling. + // A symbol is the one id a property read does NOT stringify, so it is kept + // as-is rather than pushed through `String()` (which throws on symbols): + // `Object.keys` never yields a symbol, so such a lane keeps today's reading. + const knownIds = new Set( + columns.map((col: any) => (typeof col.id === 'symbol' ? col.id : String(col.id))), + ); const uncolumnedCards = Object.keys(groups) .filter((key) => !knownIds.has(key)) .flatMap((key) => groups[key]); diff --git a/packages/types/src/__tests__/object-kanban-columns-declared-8913.test.ts b/packages/types/src/__tests__/object-kanban-columns-declared-8913.test.ts index c663b76c8f..1ef5a162ba 100644 --- a/packages/types/src/__tests__/object-kanban-columns-declared-8913.test.ts +++ b/packages/types/src/__tests__/object-kanban-columns-declared-8913.test.ts @@ -63,12 +63,13 @@ * - **a NUMERIC lane id.** The first cut admitted one, justified by "the * renderer coerces with `String(col.id)`". That is true at the i18n lookup * in `localizeColumn` and FALSE where lane membership is decided: the - * bucketer builds `knownIds` from the RAW `col.id` and compares it with + * bucketer built `knownIds` from the RAW `col.id` and compared it with * `Object.keys(groups)`, which are strings. Lanes `{ id: 1 }, { id: 2 }` - * with records `status: 1` / `status: '2'` come back + * with records `status: 1` / `status: '2'` came back * `1:r1, 2:r2, __uncolumned__:r1+r2` — every card rendered TWICE — against - * a clean `one:r1` string control. Carrier for the renderer defect: - * objectui#8993. `KanbanColumn.id` and its mirror are `string`, and the + * a clean `one:r1` string control. objectui#8993 repaired that sweep; the + * refusal stays because one declared lane-id type beats two, not because + * the renderer is still broken. `KanbanColumn.id` and its mirror are `string`, and the * protocol names no type, so refusing it here is not a narrowing below the * protocol. * - **a MIXED array.** `columns` is a UNION OF TWO ARRAYS, not an array of a @@ -208,7 +209,7 @@ const REFUSED: Array = [ // before this card AND under objectui#8913's first cut, which is why they // belong here with the same control as every other row: the failure they // guard is a declaration blessing a shape the renderer mishandles. - ['⭐ a NUMERIC lane id — the bucketer renders every such card twice (objectui#8993)', [{ id: 1, title: 'Stage one' }]], + ['⭐ a NUMERIC lane id — the bucketer rendered every such card twice until objectui#8993', [{ id: 1, title: 'Stage one' }]], ['⭐ a MIXED array, string first — the renderer ignores the whole list', ['todo', { id: 'done', title: 'Done' }]], ['⭐ a MIXED array, object first — the renderer emits a blank lane and mis-buckets', [{ id: 'done', title: 'Done' }, 'todo']], ]; diff --git a/packages/types/src/objectql.ts b/packages/types/src/objectql.ts index 8ac14f830b..aa8795c56b 100644 --- a/packages/types/src/objectql.ts +++ b/packages/types/src/objectql.ts @@ -3067,17 +3067,19 @@ export interface ObjectKanbanSchema extends BaseSchema { * * STRING only. objectui#8913's first cut admitted a number here on the * reading that "the renderer coerces with `String(col.id)`" — TRUE at - * the i18n lookup in `localizeColumn`, and FALSE at the site that - * decides lane membership: `bucketCardsIntoColumns` builds its - * `knownIds` set from the RAW `col.id` and compares it against + * the i18n lookup in `localizeColumn`, and FALSE, at the time, where + * lane membership is decided: `bucketCardsIntoColumns` built its + * `knownIds` set from the RAW `col.id` and compared it against * `Object.keys(groups)`, which are always strings. Measured by calling * the real function with lanes `{ id: 1 }, { id: 2 }` and records * `status: 1` / `status: '2'`: `1:r1, 2:r2, __uncolumned__:r1+r2` — * every card rendered TWICE, once in its lane and once in - * "Uncategorized"; the string control is a clean `one:r1`. The retired - * arm ({@link KanbanColumn.id}) and its mirror are `string` too, and the - * protocol names no type. Carrier for the bucketer defect itself: - * objectui#8993. + * "Uncategorized"; the string control was a clean `one:r1`. + * objectui#8993 repaired that sweep, so the two decisions now use one + * key spelling — the narrowing stays because the retired arm + * ({@link KanbanColumn.id}) and its mirror are `string` too and one + * declared type per lane id beats two, not because the renderer is + * still broken. */ id: string; /** Lane heading; localized against the `groupBy` picklist's option labels. */ diff --git a/packages/types/src/zod/objectql.zod.ts b/packages/types/src/zod/objectql.zod.ts index 2ae2b6947d..4e0e18d77c 100644 --- a/packages/types/src/zod/objectql.zod.ts +++ b/packages/types/src/zod/objectql.zod.ts @@ -1235,7 +1235,7 @@ export const KanbanConditionalFormattingRuleSchema = z.union([ * objectui#6939's judging — a lane card with no `title` is refused again. */ const ObjectKanbanLaneSchema = z.object({ - id: z.string().describe('Lane id — matched against the groupBy value. STRING only: the bucketer builds knownIds from the raw col.id and compares it with Object.keys(groups), which are strings, so a numeric id buckets every card TWICE (objectui#8993)'), + id: z.string().describe('Lane id — matched against the groupBy value. STRING only, and the narrowing stands on its own: until objectui#8993 the bucketer built knownIds from the raw col.id and compared it with Object.keys(groups), which are strings, so a numeric id bucketed every card TWICE; the sweep now keys membership the way the injection always did'), title: z.string().describe('Lane heading, localized against the groupBy picklist option labels'), cards: z.array(KanbanCardSchema).optional().describe('Cards this lane carries — a STATIC board only; an object-bound board buckets records into the lane by groupBy'), limit: z.number().optional().describe('WIP limit — the card count at which the lane warns; never reaches the query'),