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;