Repository navigation
fix(components): delete the shadowed SimpleTableRenderer and guard against same-namespace duplicate registration (#5125) - #5338
Merged
os-support-ai merged 3 commits intoAug 19, 2026
Conversation
…ace duplicate-registration gate Baseline commit: the behaviour pins are green on main, the gate is RED and names both offenders (ui:table, ui:kbd). The fix follows in the next commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
…owed kbd tag entry Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
os-support-ai
marked this pull request as ready for review
August 19, 2026 16:42
Collaborator
Author
|
ACCEPT — PM review, round 17.
Both stop conditions were genuinely tested, not waved through(B) the published-surface check — the one I added because the ruling assumed it rather than stating it — was answered the only way that counts: the built (A) the census found 12 The guard found a second instance immediately — which is the point of writing it for the classThe premise was re-verified by instrumenting the barrel: 160 That file was outside the surface I granted, and taking it was right: a guard that must fail on any duplicate would otherwise have shipped red on day one, or been born with an allow-list for the exact class it exists to catch. The file's own comment already claimed the list "deliberately excludes anything already registered" — the change makes that claim true and the guard now enforces it. Surface extension flagged in the report rather than slipped in. The ablation was run on a third key ( The degenerate leg, reported rather than dressed upReverse-verification predicted the guard red naming No build artifact on any leg, and proven rather than asserted: the root vitest config aliases Side effect recorded as the ruling asked: this confirms for #5126 that no reachable table renderer reads Merging via the queue. Generated by Claude Code |
os-support-ai
deleted the
claude/issue-5125-shadowed-simple-table-renderer
branch
August 19, 2026 16:43
os-support-ai
pushed a commit
that referenced
this pull request
Aug 20, 2026
…hree sites (#5126, #5337) Three published skill guides taught `{ "type": "data-table", "bind": "customers" }` and stated that "the table component calls `useDataScope("customers")` and gets the array". `DataTableRenderer` takes its rows from `data: rawData = EMPTY_ROWS` off the node and contains neither `bind` nor `useDataScope`, so copying the example produced no error and no warning — a header over the "No results found" empty state. Direction inherited from #5125 (merged PR #5338): the only bind-reading table renderer was DELETED under enforce-or-remove rather than promoted, so `data-table` does not gain `bind`; the teaching is what changes. - schema-expressions.md, protocol.md, data-integration.md: the `bind` example is now `list`, which does call `useDataScope`, bound to a string array so it renders visible entries rather than the empty `li` a record array produces. - Each site names the real readers (`list` / `tree-view` plus the `object-*` plugin widgets) and states that `data-table` is not one of them. - The `useDataScope` mechanism sentence, which described a different component, is gone. - schema-expressions.md and data-integration.md keep a `data-table` example, now in its real inline-`data` form. Their `columns` entries are byte-identical to main: the `{ name, label }` spelling is the separate open question on #5120, parked with the maintainer, and this change decides nothing about it. A pin test lifts the blocks out of the real guide files and renders them through the real SchemaRenderer: the taught forms put rows/entries on screen, and the retired `bind` form is measured producing the empty state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE
akarma-synetal
pushed a commit
to akarma-synetal/objectui
that referenced
this pull request
Aug 23, 2026
…hree sites (objectstack-ai#5371) * docs(skills): teach the binding `data-table` actually reads, in all three sites (objectstack-ai#5126, objectstack-ai#5337) Three published skill guides taught `{ "type": "data-table", "bind": "customers" }` and stated that "the table component calls `useDataScope("customers")` and gets the array". `DataTableRenderer` takes its rows from `data: rawData = EMPTY_ROWS` off the node and contains neither `bind` nor `useDataScope`, so copying the example produced no error and no warning — a header over the "No results found" empty state. Direction inherited from objectstack-ai#5125 (merged PR objectstack-ai#5338): the only bind-reading table renderer was DELETED under enforce-or-remove rather than promoted, so `data-table` does not gain `bind`; the teaching is what changes. - schema-expressions.md, protocol.md, data-integration.md: the `bind` example is now `list`, which does call `useDataScope`, bound to a string array so it renders visible entries rather than the empty `li` a record array produces. - Each site names the real readers (`list` / `tree-view` plus the `object-*` plugin widgets) and states that `data-table` is not one of them. - The `useDataScope` mechanism sentence, which described a different component, is gone. - schema-expressions.md and data-integration.md keep a `data-table` example, now in its real inline-`data` form. Their `columns` entries are byte-identical to main: the `{ name, label }` spelling is the separate open question on objectstack-ai#5120, parked with the maintainer, and this change decides nothing about it. A pin test lifts the blocks out of the real guide files and renders them through the real SchemaRenderer: the taught forms put rows/entries on screen, and the retired `bind` form is measured producing the empty state. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE * test(components): the guide pin imports `../renderers`, not the package by name `import '@object-ui/components'` from inside that package is a self-import: `tsc -p tsconfig.test.json` refused it (TS2882) and `scripts/check-package-self-import.mjs` guards the same rule. The relative entry registers the same renderers. Also drops the two `any`s the package's lint warns on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE * fix(skills): the pivot widget's registry key is `object-pivot`, and the reader list is pinned The reader list added in the previous commit named `object-pivot-table` — that is the component file (`ObjectPivotTable.tsx`), not the key it registers. An author copying it would have written a type nothing is registered under. Adds the guard that would have caught it: every `object-*` name the guides list must be registered by exactly one plugin package, and that package must read `schema.bind` through `useDataScope`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE * docs(skills): qualify the one remaining unqualified `bind` sentence in data-integration.md `data-integration.md:143` said "Components with `bind: "customers"` will then access `staticData.customers`" with no qualifier — the same over-broad claim this card corrects 40 lines below, in miniature. Names the qualification and points at the section that carries it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5125
Implements the auto-adjudicated ruling: options 1 + 3 together — remove the shadowed
SimpleTableRenderer, and add a same-namespace duplicate-registration guard. Option 2 (teachingcomplex/table.tsxto readbind) is not implemented; it widens the authorable key surface and is a Feature for the maintainer.Verified at
a0c2a72e2.Both stop conditions were checked before deleting anything
(A) Fork clause — census of published teaching of
bind-on-table. Result: zero, and the surface teaches the opposite.12
bindoccurrences, every one classified by thetypeof the node it sits on:object-gridx2,kanbanx1, gantt x1,listx2, prose x3,data-tablex3.table: zero.Counter-probes, because a zero is not a reading:
bindonlist— a genuinely reachable bind reader — is taught, atskills/objectui/guides/schema-expressions.md:407and:441. The grep does reach the surface.tableitself is taught in 14 places across the census surfaces (grep -rInE -A12 "type.*:.*'table'"), and not one of those nodes carriesbind. So the surface covers this component and consistently omits the key.skills/objectui/guides/schema-expressions.md:462states it outright: "tabledoes not readbind." The published teaching already matches today's reachable truth, which is what the ruling says stays unchanged.The three
data-tablecases are the known #5126 class (data-tableis a different registry key and does not readbindeither). Two of them sit in files #5126 does not name — recorded separately below, not touched here.(B) Published-surface check —
SimpleTableRendereris NOT a named export of@object-ui/components. Measured on the built artifact, not a grep ofsrc/:dist/index.d.ts(thetypesentry) is 23 lines; no re-export path reaches./renderers/data-display/table. Itsexport *list is./ui,./custom,./notifications,./debug,./share.grep -rn SimpleTableRenderer dist/returns exactly one hit —dist/renderers/data-display/table.d.ts:2, the symbol's own per-file declaration. Zero hits indist/index.jsanddist/index.umd.cjs.exportsmap is closed (.and./style.cssonly), so a deep import cannot reach it either:require.resolve('@object-ui/components/dist/renderers/data-display/table.js')from a real consumer fails withERR_PACKAGE_PATH_NOT_EXPORTED.src/index.ts:36imports./renderersfor side effects only — there is noexport *from the renderers tree.Deleting it therefore removes no published capability, and the ruling's mechanical boundary holds.
The premise re-verified — and a second instance found
Re-verified on today's
main:data-display/table.tsx:76andcomplex/table.tsx:25both registertableunder namespaceui, andrenderers/index.tsimports./data-display(L13) before./complex(L17), so the complex one registers last and wins.Rather than eyeballing it, the barrel was instrumented and every
register()call counted: 160 calls, 158 distinct keys, 2 same-namespace duplicates.ui:tabledata-display/table.tsx—SimpleTableRenderer, the only table renderer that readbindcomplex/table.tsxui:kbdbasic/html-elements.tsx—kbdin theTAGSloopdata-display/kbd.tsxui:kbdwas not in the card. It is fixed here rather than deferred, for a reason that is load-bearing: the ruled guard must fail on any same-namespace duplicate, so leavingui:kbdin place would have meant shipping the guard red on day one, or allow-listing the exact class the guard exists to catch. The correct shape was not a judgement call —basic/html-elements.tsx's own comment states theTAGSlist "deliberately excludes anything already registered", andkbdviolated that invariant. Removing it keeps the renderer that already served the key, so it is the same no-op-by-construction as the table deletion.In both cases the winner is untouched, so the registry's contents are identical before and after this PR.
The guard catches the class, not the instance
packages/components/src/renderers/__tests__/registration-uniqueness.test.tsxcounts every registration the production barrel makes and fails on any key registered twice under one namespace.A test rather than filling in the runtime warning that
Registry.registeralready carries as a commented-out hook: re-registering a key is a supported pattern here — 72 test files callComponentRegistry.register, many to swap a production renderer for a stub, andRegistry.unregisterexists precisely so they can restore it. A runtime warning would fire mostly on legitimate overrides, alongside the twoconsole.warnchannelsregister()already emits (the meta-less deprecation and the bare-name fallback collision). A third channel that cries wolf trains readers to ignore all three. Scoping the check to whatrenderers/index.tsregisters puts it where deliberate overrides do not happen, and makes it fail instead of scroll past.Note the existing bare-name collision guard cannot cover this: its condition is
existing.type !== fullType, which excludes same-namespace duplicates by construction.Ablation — proving it catches the class. With both real duplicates fixed, a duplicate was introduced on a third key the guard was never written against (
badgeadded to theTAGSloop, colliding withdata-display/badge.tsx):Removed again; suite back to green. No build artifact sits between that edit and the test — the root vitest config aliases
@object-ui/coreand@object-ui/componentsto theirsrc/, and the ablation flipping green-to-red from a pure source edit with no rebuild is itself the proof (haddistmediated, the edit would have been invisible and the ablation would have passed vacuously).The guard also carries a floor assertion (
callsByKey.size > 100) so it cannot pass vacuously over an empty map if the barrel import ever stops registering.Reverse-verification — one leg is degenerate, and that is the finding
Restored the deleted file and its barrel registration, predicted before running:
registration-uniqueness.test.tsxui:table (2x)onlyui:table (registered 2x)onlyshadowed-renderer-behaviour.test.tsxPredicted matched observed. The behaviour leg is degenerate by construction, and stating that plainly is more honest than filling in a template: restoring a shadowed renderer restores a loser.
complex/table.tsxstill registers later and still wins, so nothing observable moves. That degeneracy is precisely the evidence for the ruling's "changes no reachable behaviour" claim — the defect was never visible in rendered output, only in the registration ledger, which is why an accidental probe was needed to find it and why the guard is the half that can actually go red.Tests
packages/components/src/renderers/__tests__/shadowed-renderer-behaviour.test.tsxpins today's reachable behaviour — these passed identically before and after the deletion:tablerenders inlinedataagainstcolumns(2 rows).tableignoresbind: header renders, zero rows, and no "No results." row — the discriminator, since the shadowed renderer emitted one for empty data. This reproduces the probe that found the card.tablestill servescaptionandfooter, which onlycomplex/table.tsxsupports — a positive identity check, so the file cannot be satisfied by no table renderer being registered.listandtree-view, the other twouseDataScopeconsumers, still register and still readbind.kbdrenders one element per entry inkeys, proving the surviving renderer is the one that served the key before.Side effect the ruling asked to record
This confirms for #5126 that no reachable table renderer reads
bind—complex/table.tsxdoes not,complex/data-table.tsxdoes not (re-verified: zero hits forbind/useDataScope), and the only one that did was unreachable and is now gone. That doc card's direction is stable under this ruling. Nothing in #5126 is addressed by this PR.Gates run locally (all at
a0c2a72e2)vitest run packages/components/(repo root, CI's config)pnpm --filter @object-ui/components type-checktsc --noEmit && tsc -p tsconfig.test.json)pnpm --filter @object-ui/components lintsrc/ui/**no-touch zone)pnpm check:doc-typespnpm check:control-bytespnpm check:self-importVitest is run from the repo root deliberately: a package-scoped run is not refused, but it resolves a different config than CI uses, so its green would not be CI's green.
Changeset:
@object-ui/componentspatch.Out of scope, filed not fixed
data-table+bind的失真教学不止 #5126 那一处 —— protocol.md 与 data-integration.md 各有一份同形示例 #5337 — thedata-table+bindteaching distortion also appears inskills/objectui/rules/protocol.md:131andskills/objectui/guides/data-integration.md:186, two files skills/objectui/guides/schema-expressions.md 的「Data binding withbind」示例用data-table+bind,而data-table全篇不读bind—— 照抄即空表 #5126 does not name. Filed as a sub-issue of skills/objectui/guides/schema-expressions.md 的「Data binding withbind」示例用data-table+bind,而data-table全篇不读bind—— 照抄即空表 #5126 since all three examples need one ruling, not three edits.Generated by Claude Code