From 2ebd835b01e01b98e613acb9d5e6f6edebf1901e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 12:15:36 +0000 Subject: [PATCH] feat(core): say which column identity key won, out loud (#3104 PR3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the battle opened in #3104. PR1 (#3119) put the canonicalizing fold at ingestion; PR2 (#3122) converged all 22 read sites onto columnIdentity(). This is the audible half. A column carrying two identity keys that DISAGREE now logs a one-time dev-mode warning naming which key won and what to change. The fold making the two halves agree is what stops the bug, but silently rewriting `name` to match `field` also hides that the producer is emitting a contradiction. The renderer recovering is not the same as the metadata being right, so the recovery says so. Deliberately narrow: - Only contradictions. `{name:'stage'}` is legacy, not conflicting — stamped without noise. - Warn once per (identity, conflicting spelling). Columns are re-normalized on every render, and a warning that floods the console is one that gets muted. Keyed by the pair rather than the identity alone, so a column carrying two different stale spellings reports both. - Silent under NODE_ENV=production, and the fold still runs there. No lint rule, and that is a measured decision. #3104 asked for no-restricted-syntax on `.field ?? .name` to be evaluated on its false-positive rate first. With the family at zero, all 12 remaining scanner hits are legitimate — a syntactic rule cannot tell a two-layer join from a dual read, because the distinction is what the keys MEAN in that layer, not how the expression is spelled. Adopting it would mean 12 inline disables on correct code, which trains the next author to reach for the disable. The ratchet carries a verdict and a why per site instead. The evaluation is written into its header. Ledger item resolved with no change needed: #3104 flagged ListColumn for disposition under objectstack#4115 (spec-named symbols must be imports, not declarations). ListColumnSchema is already a by-reference re-export of @objectstack/spec/ui, and spec-subschema-parity.test.ts already pins it by reference identity — the only check that distinguishes a re-export from a faithful fork. Already compliant. Verified: M5 (drop the warn call) turns 4 of the new tests red. vitest core+list+grid+detail+tree+view+types -> 188 files / 2663 tests green vitest app-shell -> 250 files / 2089 tests green turbo type-check, eslint -> green Refs: objectstack#4115, #3090 (playbook), #3119 (PR1), #3122 (PR2) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01C2pdPmf2yZSd4wFDs1NHY5 --- .changeset/column-identity-say-who-wins.md | 56 +++++++++++++ content/docs/guide/troubleshooting.md | 12 ++- .../__tests__/column-identity.ratchet.test.ts | 18 ++++ .../utils/__tests__/column-identity.test.ts | 84 ++++++++++++++++++- packages/core/src/utils/column-identity.ts | 58 +++++++++++++ 5 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 .changeset/column-identity-say-who-wins.md diff --git a/.changeset/column-identity-say-who-wins.md b/.changeset/column-identity-say-who-wins.md new file mode 100644 index 000000000..660ea3e18 --- /dev/null +++ b/.changeset/column-identity-say-who-wins.md @@ -0,0 +1,56 @@ +--- +"@object-ui/core": patch +--- + +feat(core): say which column identity key won, out loud (#3104 PR3) + +Closes the battle opened in #3104. PR1 (#3119) put the canonicalizing fold at +ingestion; PR2 (#3122) converged all 22 read sites onto `columnIdentity()`. +This is the audible half. + +A column carrying two identity keys that **disagree** — `{ field: 'account', +name: 'account_name' }` — now logs a one-time dev-mode warning naming which key +won and what to change: + +``` +[ObjectUI] Column carries two identities: `field: 'account'` and +`name: 'account_name'`. `field` wins — it is the only key `ListColumnSchema` +declares — and `name` has been rewritten to match, so the rendered column and +the requested field agree. Fix the producer: drop `name` and author `field` +only. (objectui#3104) +``` + +The fold making the two halves agree is what stops the bug, but silently +rewriting `name` to match `field` also hides that the producer is emitting a +contradiction. The renderer recovering is not the same as the metadata being +right, so the recovery says so. + +Deliberately narrow: + +- **Only contradictions.** A legacy-only column (`{ name: 'stage' }`) is legacy, + not conflicting — it is stamped without noise. +- **Warn once per (identity, conflicting spelling).** Columns are re-normalized + on every render; a warning that floods the console is a warning that gets + muted. Keyed by the pair rather than the identity alone, so a column carrying + two different stale spellings reports both — the author needs to fix every + producer, not just the first one seen. +- **Silent under `NODE_ENV=production`**, and the fold still runs there. + +`resetColumnIdentityWarnings()` is exported for tests. + +**No lint rule, and that is a measured decision.** #3104 asked for +`no-restricted-syntax` on `.field ?? .name` to be evaluated on its +false-positive rate first. With the family at zero, all 12 remaining scanner +hits are legitimate — a syntactic rule cannot tell a two-layer join from a dual +read, because the distinction is what the keys mean in that layer, not how the +expression is spelled. Adopting it would mean 12 inline disables on correct +code, which trains the next author to reach for the disable. The ratchet carries +a `verdict` and a `why` per site instead, so a new hit gets triaged rather than +silenced. The evaluation is written into the ratchet's header. + +**Ledger item resolved with no change needed.** #3104 flagged `ListColumn` for +disposition under objectstack#4115 (spec-named symbols must be imports, not +declarations). `ListColumnSchema` is already a by-reference re-export of +`@objectstack/spec/ui`, and `spec-subschema-parity.test.ts` already pins it by +reference identity — the only check that distinguishes a re-export from a +faithful fork. Already compliant; nothing to do. diff --git a/content/docs/guide/troubleshooting.md b/content/docs/guide/troubleshooting.md index cf1be0151..69868e6b1 100644 --- a/content/docs/guide/troubleshooting.md +++ b/content/docs/guide/troubleshooting.md @@ -321,7 +321,17 @@ working, and both resolve the canonical key first: node, for instance) still resolves the same field the request asked for. Legacy keys are still accepted, but they are a migration bridge, not a second -contract: fix the producer. +contract: fix the producer. A column that carries two identity keys that +*disagree* logs a one-time dev-mode warning naming which key won and what to +change — the renderer recovering is not the same as the metadata being right: + +``` +[ObjectUI] Column carries two identities: `field: 'account'` and +`name: 'account_name'`. `field` wins — it is the only key `ListColumnSchema` +declares — and `name` has been rewritten to match, so the rendered column and +the requested field agree. Fix the producer: drop `name` and author `field` +only. (objectui#3104) +``` If you read column identity in your own code, use the one reader rather than spelling out a fallback chain — it resolves canonical-first, so it agrees with diff --git a/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts b/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts index ac110cb2c..0f74266f5 100644 --- a/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts +++ b/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts @@ -30,6 +30,24 @@ * accesses. That is precise enough to ratchet and loose enough to catch * spellings nobody has written yet; it is NOT a judgement that every hit is a * defect, which is what `verdict` below records. + * + * ## Why this is the gate, and no `no-restricted-syntax` lint rule + * + * objectui#3104 PR3 asked for the eslint option to be evaluated on its + * false-positive rate before adopting it. It was, and the answer is decisive: + * with the family at zero, **all 12 remaining scanner hits are legitimate** — + * two-layer joins where both precedences are correct, the form cluster #3090 + * settled the other way, and display fallbacks that merely share the key names. + * A syntactic rule matching `.field ?? .name` cannot tell any of those from a + * real dual read, because the distinction is what the keys MEAN in that layer, + * not how the expression is spelled. Adopting it would mean 12 inline disables + * on correct code — which trains the next author to reach for the disable, the + * precise reflex that lets a real one through. + * + * The ratchet does what the lint rule cannot: it carries a `verdict` and a + * `why` per site, so a new hit has to be triaged rather than silenced. The + * assertion that the family is 0 (below) is exactly the statement that every + * hit a lint rule would fire on today is a false positive. */ import { describe, it, expect } from 'vitest'; diff --git a/packages/core/src/utils/__tests__/column-identity.test.ts b/packages/core/src/utils/__tests__/column-identity.test.ts index 0302c481d..cf077125d 100644 --- a/packages/core/src/utils/__tests__/column-identity.test.ts +++ b/packages/core/src/utils/__tests__/column-identity.test.ts @@ -6,7 +6,7 @@ * LICENSE file in the root directory of this source tree. */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { CANONICAL_COLUMN_IDENTITY_KEY, LEGACY_COLUMN_IDENTITY_KEYS, @@ -15,6 +15,7 @@ import { hasConflictingColumnIdentity, normalizeColumnIdentity, normalizeColumnIdentities, + resetColumnIdentityWarnings, } from '../column-identity'; import { normalizeListViewSchema } from '../normalize-list-view'; import { buildExpandFields } from '../expand-fields'; @@ -214,3 +215,84 @@ describe('normalizeListViewSchema — column identity fold (#3104)', () => { expect(normalizeListViewSchema(schema)).toBe(schema); }); }); + +describe('conflicting-identity warning (#3104 PR3)', () => { + let warn: ReturnType; + + beforeEach(() => { + resetColumnIdentityWarnings(); + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => warn.mockRestore()); + + it('names the winner, the loser, and both values', () => { + normalizeColumnIdentity({ field: 'account', name: 'account_name' }); + expect(warn).toHaveBeenCalledTimes(1); + const msg = String(warn.mock.calls[0]?.[0]); + // The whole point is that the author can act on it without reading source. + expect(msg).toContain("field: 'account'"); + expect(msg).toContain("name: 'account_name'"); + expect(msg).toContain('`field` wins'); + expect(msg).toContain('objectui#3104'); + }); + + it('warns once per conflict, not once per fold', () => { + const entry = { field: 'account', name: 'account_name' }; + normalizeColumnIdentity(entry); + normalizeColumnIdentity(entry); + normalizeColumnIdentity({ ...entry }); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('reports each distinct conflicting spelling, not just the first', () => { + normalizeColumnIdentity({ field: 'account', name: 'a_name', fieldName: 'a_fname' }); + expect(warn).toHaveBeenCalledTimes(2); + }); + + it('stays silent for a legacy-only column — that is not a contradiction', () => { + normalizeColumnIdentity({ name: 'stage' }); + expect(warn).not.toHaveBeenCalled(); + }); + + it('stays silent when the keys already agree', () => { + normalizeColumnIdentity({ field: 'stage', name: 'stage' }); + expect(warn).not.toHaveBeenCalled(); + }); + + it('stays silent for a clean spec column', () => { + normalizeColumnIdentity({ field: 'stage' }); + expect(warn).not.toHaveBeenCalled(); + }); + + it('fires through the ingestion fold, which is where hosts actually hit it', () => { + normalizeListViewSchema({ + viewType: 'grid', + columns: [{ field: 'account', name: 'account_name' }], + }); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('is silent in production — a shipped app gets no console noise', () => { + const prev = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + try { + normalizeColumnIdentity({ field: 'account', name: 'account_name' }); + expect(warn).not.toHaveBeenCalled(); + } finally { + process.env.NODE_ENV = prev; + } + }); + + it('still folds the column when the warning is suppressed', () => { + const prev = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + try { + expect(normalizeColumnIdentity({ field: 'account', name: 'account_name' })).toEqual({ + field: 'account', + name: 'account', + }); + } finally { + process.env.NODE_ENV = prev; + } + }); +}); diff --git a/packages/core/src/utils/column-identity.ts b/packages/core/src/utils/column-identity.ts index f537528f9..6357331a5 100644 --- a/packages/core/src/utils/column-identity.ts +++ b/packages/core/src/utils/column-identity.ts @@ -95,6 +95,58 @@ export function hasConflictingColumnIdentity(entry: unknown): boolean { return false; } +// Warn once per (identity, conflicting spelling), not once per fold: columns are +// re-normalized on every ListView render, and a warning that floods the console +// is a warning that gets muted. Keyed by the pair rather than by the identity +// alone so a column carrying two different stale spellings reports both — the +// author needs to fix every producer, not just the first one seen. +const warnedConflicts = new Set(); + +/** Reset the warn-once memo. Exported for tests. */ +export function resetColumnIdentityWarnings(): void { + warnedConflicts.clear(); +} + +const isDev = (): boolean => + (globalThis as { process?: { env?: Record } }).process?.env?.NODE_ENV !== + 'production'; + +/** + * Dev-mode only: say which key won, out loud, when a column carries two identity + * spellings that disagree. + * + * This is the audible half of objectui#3104. The fold below makes the two halves + * of such a column agree, which is what stops the bug — but silently rewriting + * `name` to match `field` also hides that the metadata producer is emitting a + * contradiction. The renderer recovering is not the same as the metadata being + * right, so the recovery says so. + * + * Non-breaking by construction: changes no types, rejects nothing, and is a + * no-op under `NODE_ENV=production`. + */ +function warnOnConflictingIdentity( + entry: Record, + identity: string, + losing: readonly string[], +): void { + if (!isDev() || losing.length === 0) return; + for (const key of losing) { + const memo = `${identity}:${key}:${String(entry[key])}`; + if (warnedConflicts.has(memo)) continue; + warnedConflicts.add(memo); + // eslint-disable-next-line no-console + console.warn( + `[ObjectUI] Column carries two identities: \`${CANONICAL_COLUMN_IDENTITY_KEY}: ` + + `'${identity}'\` and \`${key}: '${String(entry[key])}'\`. ` + + `\`${CANONICAL_COLUMN_IDENTITY_KEY}\` wins — it is the only key ` + + `\`ListColumnSchema\` declares — and \`${key}\` has been rewritten to match, ` + + `so the rendered column and the requested field agree. Fix the producer: ` + + `drop \`${key}\` and author \`${CANONICAL_COLUMN_IDENTITY_KEY}\` only. ` + + `(objectui#3104)`, + ); + } +} + /** * Stamp the canonical identity onto one column entry. * @@ -135,6 +187,12 @@ export function normalizeColumnIdentity(entry: T): T { ); if (!needsCanonical && staleLegacy.length === 0) return entry; + // Say who won before rewriting, while the losing spelling is still readable. + // Only genuine contradictions are reported: a column that merely lacks the + // canonical key (`{ name: 'stage' }`) is legacy, not conflicting, and gets + // stamped without noise. + warnOnConflictingIdentity(entry, identity, staleLegacy); + const next: Record = { ...entry }; next[CANONICAL_COLUMN_IDENTITY_KEY] = identity; for (const key of staleLegacy) next[key] = identity;