Skip to content

Commit 2ebd835

Browse files
committed
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 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C2pdPmf2yZSd4wFDs1NHY5
1 parent 95b7214 commit 2ebd835

5 files changed

Lines changed: 226 additions & 2 deletions

File tree

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
---
2+
"@object-ui/core": patch
3+
---
4+
5+
feat(core): say which column identity key won, out loud (#3104 PR3)
6+
7+
Closes the battle opened in #3104. PR1 (#3119) put the canonicalizing fold at
8+
ingestion; PR2 (#3122) converged all 22 read sites onto `columnIdentity()`.
9+
This is the audible half.
10+
11+
A column carrying two identity keys that **disagree** — `{ field: 'account',
12+
name: 'account_name' }` — now logs a one-time dev-mode warning naming which key
13+
won and what to change:
14+
15+
```
16+
[ObjectUI] Column carries two identities: `field: 'account'` and
17+
`name: 'account_name'`. `field` wins — it is the only key `ListColumnSchema`
18+
declares — and `name` has been rewritten to match, so the rendered column and
19+
the requested field agree. Fix the producer: drop `name` and author `field`
20+
only. (objectui#3104)
21+
```
22+
23+
The fold making the two halves agree is what stops the bug, but silently
24+
rewriting `name` to match `field` also hides that the producer is emitting a
25+
contradiction. The renderer recovering is not the same as the metadata being
26+
right, so the recovery says so.
27+
28+
Deliberately narrow:
29+
30+
- **Only contradictions.** A legacy-only column (`{ name: 'stage' }`) is legacy,
31+
not conflicting — it is stamped without noise.
32+
- **Warn once per (identity, conflicting spelling).** Columns are re-normalized
33+
on every render; a warning that floods the console is a warning that gets
34+
muted. Keyed by the pair rather than the identity alone, so a column carrying
35+
two different stale spellings reports both — the author needs to fix every
36+
producer, not just the first one seen.
37+
- **Silent under `NODE_ENV=production`**, and the fold still runs there.
38+
39+
`resetColumnIdentityWarnings()` is exported for tests.
40+
41+
**No lint rule, and that is a measured decision.** #3104 asked for
42+
`no-restricted-syntax` on `.field ?? .name` to be evaluated on its
43+
false-positive rate first. With the family at zero, all 12 remaining scanner
44+
hits are legitimate — a syntactic rule cannot tell a two-layer join from a dual
45+
read, because the distinction is what the keys mean in that layer, not how the
46+
expression is spelled. Adopting it would mean 12 inline disables on correct
47+
code, which trains the next author to reach for the disable. The ratchet carries
48+
a `verdict` and a `why` per site instead, so a new hit gets triaged rather than
49+
silenced. The evaluation is written into the ratchet's header.
50+
51+
**Ledger item resolved with no change needed.** #3104 flagged `ListColumn` for
52+
disposition under objectstack#4115 (spec-named symbols must be imports, not
53+
declarations). `ListColumnSchema` is already a by-reference re-export of
54+
`@objectstack/spec/ui`, and `spec-subschema-parity.test.ts` already pins it by
55+
reference identity — the only check that distinguishes a re-export from a
56+
faithful fork. Already compliant; nothing to do.

content/docs/guide/troubleshooting.md

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,17 @@ working, and both resolve the canonical key first:
321321
node, for instance) still resolves the same field the request asked for.
322322

323323
Legacy keys are still accepted, but they are a migration bridge, not a second
324-
contract: fix the producer.
324+
contract: fix the producer. A column that carries two identity keys that
325+
*disagree* logs a one-time dev-mode warning naming which key won and what to
326+
change — the renderer recovering is not the same as the metadata being right:
327+
328+
```
329+
[ObjectUI] Column carries two identities: `field: 'account'` and
330+
`name: 'account_name'`. `field` wins — it is the only key `ListColumnSchema`
331+
declares — and `name` has been rewritten to match, so the rendered column and
332+
the requested field agree. Fix the producer: drop `name` and author `field`
333+
only. (objectui#3104)
334+
```
325335

326336
If you read column identity in your own code, use the one reader rather than
327337
spelling out a fallback chain — it resolves canonical-first, so it agrees with

packages/core/src/utils/__tests__/column-identity.ratchet.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,24 @@
3030
* accesses. That is precise enough to ratchet and loose enough to catch
3131
* spellings nobody has written yet; it is NOT a judgement that every hit is a
3232
* defect, which is what `verdict` below records.
33+
*
34+
* ## Why this is the gate, and no `no-restricted-syntax` lint rule
35+
*
36+
* objectui#3104 PR3 asked for the eslint option to be evaluated on its
37+
* false-positive rate before adopting it. It was, and the answer is decisive:
38+
* with the family at zero, **all 12 remaining scanner hits are legitimate** —
39+
* two-layer joins where both precedences are correct, the form cluster #3090
40+
* settled the other way, and display fallbacks that merely share the key names.
41+
* A syntactic rule matching `.field ?? .name` cannot tell any of those from a
42+
* real dual read, because the distinction is what the keys MEAN in that layer,
43+
* not how the expression is spelled. Adopting it would mean 12 inline disables
44+
* on correct code — which trains the next author to reach for the disable, the
45+
* precise reflex that lets a real one through.
46+
*
47+
* The ratchet does what the lint rule cannot: it carries a `verdict` and a
48+
* `why` per site, so a new hit has to be triaged rather than silenced. The
49+
* assertion that the family is 0 (below) is exactly the statement that every
50+
* hit a lint rule would fire on today is a false positive.
3351
*/
3452

3553
import { describe, it, expect } from 'vitest';

packages/core/src/utils/__tests__/column-identity.test.ts

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* LICENSE file in the root directory of this source tree.
77
*/
88

9-
import { describe, it, expect } from 'vitest';
9+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
1010
import {
1111
CANONICAL_COLUMN_IDENTITY_KEY,
1212
LEGACY_COLUMN_IDENTITY_KEYS,
@@ -15,6 +15,7 @@ import {
1515
hasConflictingColumnIdentity,
1616
normalizeColumnIdentity,
1717
normalizeColumnIdentities,
18+
resetColumnIdentityWarnings,
1819
} from '../column-identity';
1920
import { normalizeListViewSchema } from '../normalize-list-view';
2021
import { buildExpandFields } from '../expand-fields';
@@ -214,3 +215,84 @@ describe('normalizeListViewSchema — column identity fold (#3104)', () => {
214215
expect(normalizeListViewSchema(schema)).toBe(schema);
215216
});
216217
});
218+
219+
describe('conflicting-identity warning (#3104 PR3)', () => {
220+
let warn: ReturnType<typeof vi.spyOn>;
221+
222+
beforeEach(() => {
223+
resetColumnIdentityWarnings();
224+
warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
225+
});
226+
afterEach(() => warn.mockRestore());
227+
228+
it('names the winner, the loser, and both values', () => {
229+
normalizeColumnIdentity({ field: 'account', name: 'account_name' });
230+
expect(warn).toHaveBeenCalledTimes(1);
231+
const msg = String(warn.mock.calls[0]?.[0]);
232+
// The whole point is that the author can act on it without reading source.
233+
expect(msg).toContain("field: 'account'");
234+
expect(msg).toContain("name: 'account_name'");
235+
expect(msg).toContain('`field` wins');
236+
expect(msg).toContain('objectui#3104');
237+
});
238+
239+
it('warns once per conflict, not once per fold', () => {
240+
const entry = { field: 'account', name: 'account_name' };
241+
normalizeColumnIdentity(entry);
242+
normalizeColumnIdentity(entry);
243+
normalizeColumnIdentity({ ...entry });
244+
expect(warn).toHaveBeenCalledTimes(1);
245+
});
246+
247+
it('reports each distinct conflicting spelling, not just the first', () => {
248+
normalizeColumnIdentity({ field: 'account', name: 'a_name', fieldName: 'a_fname' });
249+
expect(warn).toHaveBeenCalledTimes(2);
250+
});
251+
252+
it('stays silent for a legacy-only column — that is not a contradiction', () => {
253+
normalizeColumnIdentity({ name: 'stage' });
254+
expect(warn).not.toHaveBeenCalled();
255+
});
256+
257+
it('stays silent when the keys already agree', () => {
258+
normalizeColumnIdentity({ field: 'stage', name: 'stage' });
259+
expect(warn).not.toHaveBeenCalled();
260+
});
261+
262+
it('stays silent for a clean spec column', () => {
263+
normalizeColumnIdentity({ field: 'stage' });
264+
expect(warn).not.toHaveBeenCalled();
265+
});
266+
267+
it('fires through the ingestion fold, which is where hosts actually hit it', () => {
268+
normalizeListViewSchema({
269+
viewType: 'grid',
270+
columns: [{ field: 'account', name: 'account_name' }],
271+
});
272+
expect(warn).toHaveBeenCalledTimes(1);
273+
});
274+
275+
it('is silent in production — a shipped app gets no console noise', () => {
276+
const prev = process.env.NODE_ENV;
277+
process.env.NODE_ENV = 'production';
278+
try {
279+
normalizeColumnIdentity({ field: 'account', name: 'account_name' });
280+
expect(warn).not.toHaveBeenCalled();
281+
} finally {
282+
process.env.NODE_ENV = prev;
283+
}
284+
});
285+
286+
it('still folds the column when the warning is suppressed', () => {
287+
const prev = process.env.NODE_ENV;
288+
process.env.NODE_ENV = 'production';
289+
try {
290+
expect(normalizeColumnIdentity({ field: 'account', name: 'account_name' })).toEqual({
291+
field: 'account',
292+
name: 'account',
293+
});
294+
} finally {
295+
process.env.NODE_ENV = prev;
296+
}
297+
});
298+
});

packages/core/src/utils/column-identity.ts

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,58 @@ export function hasConflictingColumnIdentity(entry: unknown): boolean {
9595
return false;
9696
}
9797

98+
// Warn once per (identity, conflicting spelling), not once per fold: columns are
99+
// re-normalized on every ListView render, and a warning that floods the console
100+
// is a warning that gets muted. Keyed by the pair rather than by the identity
101+
// alone so a column carrying two different stale spellings reports both — the
102+
// author needs to fix every producer, not just the first one seen.
103+
const warnedConflicts = new Set<string>();
104+
105+
/** Reset the warn-once memo. Exported for tests. */
106+
export function resetColumnIdentityWarnings(): void {
107+
warnedConflicts.clear();
108+
}
109+
110+
const isDev = (): boolean =>
111+
(globalThis as { process?: { env?: Record<string, string | undefined> } }).process?.env?.NODE_ENV !==
112+
'production';
113+
114+
/**
115+
* Dev-mode only: say which key won, out loud, when a column carries two identity
116+
* spellings that disagree.
117+
*
118+
* This is the audible half of objectui#3104. The fold below makes the two halves
119+
* of such a column agree, which is what stops the bug — but silently rewriting
120+
* `name` to match `field` also hides that the metadata producer is emitting a
121+
* contradiction. The renderer recovering is not the same as the metadata being
122+
* right, so the recovery says so.
123+
*
124+
* Non-breaking by construction: changes no types, rejects nothing, and is a
125+
* no-op under `NODE_ENV=production`.
126+
*/
127+
function warnOnConflictingIdentity(
128+
entry: Record<string, unknown>,
129+
identity: string,
130+
losing: readonly string[],
131+
): void {
132+
if (!isDev() || losing.length === 0) return;
133+
for (const key of losing) {
134+
const memo = `${identity}:${key}:${String(entry[key])}`;
135+
if (warnedConflicts.has(memo)) continue;
136+
warnedConflicts.add(memo);
137+
// eslint-disable-next-line no-console
138+
console.warn(
139+
`[ObjectUI] Column carries two identities: \`${CANONICAL_COLUMN_IDENTITY_KEY}: ` +
140+
`'${identity}'\` and \`${key}: '${String(entry[key])}'\`. ` +
141+
`\`${CANONICAL_COLUMN_IDENTITY_KEY}\` wins — it is the only key ` +
142+
`\`ListColumnSchema\` declares — and \`${key}\` has been rewritten to match, ` +
143+
`so the rendered column and the requested field agree. Fix the producer: ` +
144+
`drop \`${key}\` and author \`${CANONICAL_COLUMN_IDENTITY_KEY}\` only. ` +
145+
`(objectui#3104)`,
146+
);
147+
}
148+
}
149+
98150
/**
99151
* Stamp the canonical identity onto one column entry.
100152
*
@@ -135,6 +187,12 @@ export function normalizeColumnIdentity<T>(entry: T): T {
135187
);
136188
if (!needsCanonical && staleLegacy.length === 0) return entry;
137189

190+
// Say who won before rewriting, while the losing spelling is still readable.
191+
// Only genuine contradictions are reported: a column that merely lacks the
192+
// canonical key (`{ name: 'stage' }`) is legacy, not conflicting, and gets
193+
// stamped without noise.
194+
warnOnConflictingIdentity(entry, identity, staleLegacy);
195+
138196
const next: Record<string, unknown> = { ...entry };
139197
next[CANONICAL_COLUMN_IDENTITY_KEY] = identity;
140198
for (const key of staleLegacy) next[key] = identity;

0 commit comments

Comments
 (0)