From 2b01b625068155cde0119997444456d4bed56331 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 20:04:08 +0000 Subject: [PATCH] fix(plugin-kanban): key the leftover sweep the way the injection reads (objectui#8993) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bucketCardsIntoColumns` decided lane membership twice, with two key types. Injection reads `groups[col.id]`, and a property read coerces its key, so a lane `{ id: 1 }` correctly picks up the group stored under `'1'`. The leftover sweep (objectui#2792) then built its known-id `Set` from the RAW `col.id` and filtered `Object.keys(groups)` — always strings. `new Set([1]).has('1')` is `false`, so every record the injection had just placed was swept a second time into the trailing "Uncategorized" lane and the board rendered each card twice. The set now holds the same key spelling the property read uses. The injection side is untouched: making that read strict would break the coercion lanes depend on, and the string control could not catch it. 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. Pinned in laneIdCoercion-8993.test.ts: the reported numeric leg, the string control (green on both sides of the repair), the whole non-string class, the label→id path, a control proving the sweep still sweeps — a repair that simply deleted it would pass every other row and resurrect objectui#2792 — and the boundary rows showing the coercion mints no new collision. `@object-ui/types` carries prose only: the lane-id `describe()` and TSDoc stated the double-render as current behaviour. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01MPaVWWMuWeT5LgB1qoXjVB --- .changeset/8993-kanban-lane-id-sweep-key.md | 38 ++++ content/docs/api/schema-reference.md | 2 +- content/docs/plugins/plugin-kanban.mdx | 2 +- .../src/__tests__/laneIdCoercion-8993.test.ts | 180 ++++++++++++++++++ packages/plugin-kanban/src/index.tsx | 15 +- ...bject-kanban-columns-declared-8913.test.ts | 11 +- packages/types/src/objectql.ts | 16 +- packages/types/src/zod/objectql.zod.ts | 2 +- 8 files changed, 250 insertions(+), 16 deletions(-) create mode 100644 .changeset/8993-kanban-lane-id-sweep-key.md create mode 100644 packages/plugin-kanban/src/__tests__/laneIdCoercion-8993.test.ts 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'),