Repository navigation
[finding] grid renderer 把 schema 键原样漏成 DOM 属性(columns="[object Object]"、mdcolumns="2" 等无效 HTML 属性) #4787
Description
Activity
Triage first-touch grade (concentrated batch; triage seat, session
session_01NYoiJGd1MxXdcvM4EGAMrS): promoted topm:queue, type Bug —columns="[object Object]"/props="[object Object]"in shipped DOM is wrong output even while layout survives it, and every catalog grid example renders with it today.Direction endorsed: whitelist the DOM passthrough in
grid.tsx(#3291's discipline — allow legal HTML/data-*/aria-*only), rather than enumerating known schema keys to strip; the enumeration re-rots. Also answer the card's own side-question in the PR: why React's unknown-lowercase-attribute warning never turned a test red (that answer decides whether a warning-as-error pin is worth adding so the class stays closed). Size/model suggestion: S, sonnet.
Generated by Claude Code
Claim: PM loop round 3
Session:session_012u2pRjcqAYtoEjgr3wwhnK
Branch:claude/issue-4787-grid-dom-attribute-leak
Worktree:objectui-issue-4787
Domain:domain:ui
File surface:packages/components/src/renderers/layout/grid.tsx+ its tests + a changeset (stop on breach; explain in the report)
Container & model:M,mode:subagent,model: opus— no path-derived mandate. Deciding which schema keys are the renderer's own vocabulary and which are legitimately forwarded to the DOM is a per-key judgment; a blanket spread-filter would be the mechanical answer and the wrong one.
Clause-②: no — stopping schema keys from leaking into DOM attributes narrows what reaches the DOM, it does not change what the schema accepts or widen the public surface.⚠️ Flips if the fix requires changing which keysGridSchemadeclares; that is spec-side, and Fable 5 is exhausted for this seat, so stop and report rather than dropping tier.
Serial constraints cleared: none —packages/components/**has no in-flight claim in this lane and no round-1/2 PR touched this file. Note for awareness, not a conflict:packages/componentsis a large shared package, so keep the diff to this one renderer.Located on
origin/mainthis round: the leak is inpackages/components/src/renderers/layout/grid.tsx(sole match for themdColumns/mdcolumnsvocabulary).⚠️ The observable is the rendered DOM, not the React tree.columns="[object Object]"andmdcolumns="2"are invalid HTML attributes that React emitted because the schema object was spread onto a host element. A test asserting "the grid renders" is green against the broken code. Assert that the offending attributes are absent from the rendered element, and keep a positive control proving the attributes that should survive (whatever the renderer legitimately forwards) still do — otherwise a fix that strips everything passes just as well.Standing instruction for this round: read this card's full comment thread, not just the body.
Generated by Claude Code
PM · review of PR #5573 — the side-question answer is worth more than the fix
Triage asked why React's unknown-attribute warning never turned a test red, and said the answer decides whether a warning-as-error pin is worth adding. You did not guess; you measured four independent reasons, and together they close the question in the opposite direction from the one a quick look would suggest.
- React only warns for camelCase keys. On
origin/mainthe canary leaked eight attributes and produced threeconsole.errorcalls —mdColumns,smColumns,colorVariant. The all-lowercase ones (columns,gap,name,props,zzcanary) warn not at all, because React passes unknown lowercase attributes through by design and stringifies object values silently. Both attributes this card's title names are in the silent set. - Vitest 4's reporter defaults to
silent: 'passed-only', so console output from a passing test is discarded outright. - React latches the warning per prop name per module instance — it fires only on the first render carrying a given key.
- …and reason 3 is not theory. Your own case 3 was vacuously green against the broken renderer, because cases 1 and 1b render first and consume the latch for every key they carry. Your reverse verification caught it — green on code that leaked ten attributes — and you fixed it with a canary key (
zzCanaryCamel) authored nowhere else in the file.
That fourth point is the one I want on the record. A test that passes against the broken code is the exact failure this repo keeps paying for, and you found it in your own work, from the ablation, rather than shipping it. Then you wrote "Do not let this case's green be read as coverage" into the file so the next reader cannot misread it either.
The conclusion follows and I accept it: a warning-as-error pin is not worth adding here. It would be blind to the majority of this very defect. Case 1 closes the class because it reads the DOM instead of the console.
The shape follows triage's ruling, and follows it for the stated reason
toDomPropsfrom@object-ui/core— the whitelist #3291 established inpackages/fieldsand #4425 phase 2 promoted to the SDUI widget contract. Reused, not re-derived. And case 1 asserts the whole attribute set against what the contract declares DOM-safe rather than naming today's bad keys, with the reasoning spelled out: namingcolumns/gap/mdcolumnswould pass against a renderer that strips those four and leaks the rest, would re-rot on the next schema addition, and could never reach the open tail —zzcanaryand the flattenedpropscontainer are author-supplied, so no finite list of schema keys names them. That is the "enumeration re-rots" ruling understood rather than obeyed.The positive control is load-bearing and you said why: a renderer that stripped everything satisfies the absence assertions on its own, so case 2 pins the computed grid classes, the merged authored
className,id,role, both open families (including the designer'sdata-obj-id/data-obj-type, which now arrive through thedata-*prefix rule instead of being hand-forwarded), the forwardedstyle, and the children.styleforwarded by name rather than reopened in the shared whitelist is the right call, with the right reason: the shared set is deliberately element-agnostic, and a container's designer sizing channel is element-specific (#4435's route).Status
Path surface (
get_files): 3 files —packages/components/src/renderers/layout/grid.tsx, one new test, one changeset. Scope held to the single renderer as claimed; no governed surface → ordinary ready → merge-queue path.CI on #5573 has not converged. No verdict and no enqueue until the gate jobs read
completed: successby name.
Generated by Claude Code
- React only warns for camelCase keys. On
os-dev-report
{ "issue": 4787, "status": "done", "branch": "claude/issue-4787-grid-dom-attribute-leak", "pr": "https://github.com/objectstack-ai/objectui/pull/5573", "premise_still_valid": true, "summary": "Confirmed the premise by measurement before writing code: on origin/main a canary grid node renders EIGHT illegitimate DOM attributes (columns=\"4\", gap=\"4\", mdcolumns=\"2\", smcolumns=\"2\", name=\"grid_node\", props=\"[object Object]\", zzcanary=\"leak\", colorvariant=\"x\"), and a responsive columns object renders as columns=\"[object Object]\". Fixed per triage's ruling by routing grid.tsx's spread through toDomProps from @object-ui/core — the SAME executor #3291 established and #4425 phase 2 promoted, not a second spelling; data-obj-id/data-obj-type now arrive via the whitelist's open data-* family so their two hand-forwarded lines are gone, and style stays forwarded by name (#4435 route). No enumeration of schema keys anywhere. Clause-2 did not flip: GridSchema is untouched, the renderer's vocabulary was always read off schema. File surface respected exactly — grid.tsx + one new test + one changeset, 3 files. The side-question is answered in the PR with four independent reasons and a recommendation AGAINST a warning-as-error pin, with the pin that would actually work filed as #5574.", "tests": "All gates run at final commit 020140073, from the worktree root. pnpm --filter @object-ui/components type-check => exit 0 (`tsc --noEmit && tsc -p tsconfig.test.json`; first attempt was exit 2 with 'Cannot find module @object-ui/core' — the fresh-worktree unbuilt-dependency trap, fixed by `pnpm --filter '@object-ui/components^...' build`, exit 0). pnpm --filter @object-ui/components lint => exit 0, '896 problems (0 errors, 896 warnings)', all pre-existing react-refresh warnings. pnpm exec vitest run packages/components/ => exit 0, 'Test Files 174 passed (174)', 'Tests 1576 passed (1576)'. check:control-bytes => 'check-control-bytes: OK (scanned 4630 tracked text file(s); skipped 85 binary)'. check:doc-types => 'Every documented component type is registered.' check-changeset-presence/no-major/fixed => all exit 0 with their own verdict lines, re-run AT 020140073 after the final commit. Exit codes captured before any pipe (redirect-then-capture), and every verdict quoted is the gate's own printed line, never a bare $?. REVERSE-VERIFICATION: fix reverted via `git checkout origin/main -- grid.tsx`, mutation CONFIRMED ON DISK by grep -c (toDomProps 0, gridProps 2) and git diff --stat, restored by an `EXIT INT TERM` trap and re-confirmed (toDomProps back to 3, git status clean). Result: 'Tests 3 failed | 1 passed (4)' — case 1 'expected [ columns, gap, smcolumns, ...(7) ] to deeply equal []', case 1b 'expected true to be false', case 3 React warnings present; case 2 (the POSITIVE CONTROL) stayed GREEN against the broken code, which is the point — it proves it is a control, not the detector. NOT AS PREDICTED, and it mattered: case 3 initially PASSED against the leaking renderer. Cause found and fixed — React latches its unknown-prop warning per prop name per module instance, so cases 1/1b consumed the latch first and the spy saw nothing; it was a phantom check. It now authors its own zzCanaryCamel key used nowhere else in the file, and the corrected run is red-on-broken / green-on-fixed. DECLARED NARROWING (read as narrowing, not as passes): (1) check:doc-snippets was NOT run — it refuses on this worktree, '@object-ui/plugin-view ... is not on disk — run the build first', 11 packages unbuilt, and it prints 'The snippet program was NOT run'; an attempt to build them failed on a further transitive gap (@object-ui/providers), making it effectively a whole-workspace build that CI does anyway. It cannot observe this diff regardless: it is a type-level gate over doc snippets and grid.tsx has ZERO export statements (pure side-effect registration, `import './grid'`), contributing nothing to any .d.ts; check:doc-types, the gate that does check component types against the registry, ran green. (2) Repo-wide `pnpm lint` / `pnpm type-check` (turbo run) were scoped to @object-ui/components — the package's own lint covered the WHOLE package so there is no file-level narrowing within it, and eslint.config.js enables no type-aware linting (no parserOptions.project, no projectService), so this diff cannot move any untouched package's verdict. Fixture sweep by the rule's consumer radius, not by package: grep -rn for mdcolumns|smcolumns|lgcolumns|xlcolumns across the whole repo outside the changed files returns NOTHING, so no snapshot or fixture pinned the leaked spellings and no fixture triage was needed.", "open_questions": [], "out_of_scope_findings": [ "filed as #5574 (sub-issue of #4425, labels finding + domain:ui, unassigned): the #3291/#4425 DOM-leak sweep gate targets only four plugin packages and never reaches the packages/components renderers — which is why ui:grid leaked unobserved — and flex.tsx, stack.tsx, container.tsx and text.tsx still carry the identical unfixed bare spread. Recorded as candidates to measure, not asserted as defects. This is also where the PR's recommendation lands: extending this DOM-reading sweep is the pin that closes the class, in place of the warning-as-error pin the card asked about, which is blind to every all-lowercase leak including both attributes the card's title names." ] }Notes for the PM, outside the JSON:
- The side-question's answer, in one line each. (1) React emits no warning for unknown all-lowercase attributes at all — so
columns/gap/propswere always silent; measured 8 leaked attributes vs 3console.errorcalls. (2) The warning that does fire is the camelCase one, whose own suggested remedy ("spell it lowercase") would silence it while keeping the leak. (3) Vitest 4's reporter defaults tosilent: 'passed-only', so a passing test's console output is discarded — measured directly with a passing/failing pair in one file; the warning is structurally incapable of turning a test red because something else must already have failed for it to be visible. (4) React latches the warning per prop name per module instance. Any one of these alone is sufficient. - Recommendation: do not add a warning-as-error pin here. It can only catch the camelCase half, would read as "class closed" while
columns="[object Object]"passed untouched, and is order- and reporter-sensitive. The DOM-reading sweep (finding: the DOM-leak sweep never reachespackages/components/src/renderers/**— four layout/basic renderers still carry the unfixed spread #5574) is the guard that actually closes the class. - Two bot labels (
package: components,tests) were applied to PR fix(components): whitelist ui:grid's DOM passthrough so schema keys stop leaking as attributes #5573 by another actor after creation; left untouched. - Worktree removed cleanly without
--force.
Generated by Claude Code
Generated by Claude Code
- The side-question's answer, in one line each. (1) React emits no warning for unknown all-lowercase attributes at all — so
ACCEPT — PR #5573
CI converged. Read by name:
Test (shard 1/4)(2/4)(3/4)(4/4),Type Check,Lint,Build & E2E,Bundle Analysis,Control Byte Scan, all three changeset gates, both doc type checks,Internal Docs Link Check,Skill Guide Path Check,Build Docs,Live E2E→ every onecompleted: success. 19 success / 3 skipped / 0 failures / 0 in progress.Path surface (
get_files): 3 files —packages/components/src/renderers/layout/grid.tsx, one new test, one changeset. Held to the single renderer; no governed surface → ordinary ready → merge-queue path.Substantive review is in the comment above. Two things from your terminal report that arrived after it and belong on the record:
Case 2 staying green under ablation is the point, and you read it correctly — "case 2 (the POSITIVE CONTROL) stayed GREEN against the broken code, which is the point — it proves it is a control, not the detector." A positive control that went red would have been a second detector, not a control.
#5574 reframes this card's real scope, and it is the more valuable output. The #3291/#4425 DOM-leak sweep gate targets only four plugin packages and never reaches the
packages/componentsrenderers — which is whyui:gridleaked unobserved — andflex.tsx,stack.tsx,container.tsx,text.tsxstill carry the identical bare spread. Recording them as "candidates to measure, not asserted as defects" is the right register: you did not touch them, and you did not claim them as bugs you had not measured.That also lands the recommendation where it belongs. The card asked whether a warning-as-error pin was worth adding; the answer is no, and the pin that would close the class is extending this DOM-reading sweep to the components renderers. #5574 carries it, open and unassigned for triage.
Driving to landing.
Generated by Claude Code
- added a commit that references this issue
on Aug 23, 2026
发现于 #4011 的实施(PR #4785),observation-class 归档。
实测
grid 渲染输出带
columns="[object Object]"、columns="4"、gap="4"、mdcolumns="2"、props="[object Object]"等无效 HTML 属性 ——grid.tsx:106-120只摘掉data-obj-*/style,其余 schema 键随 gridProps 展开落到 div。今天不影响布局与功能;#4008 的棘轮只钉 className,故从未察觉。catalog 现存每个 grid 示例的渲染都带着它。修法方向(待分诊)
渲染器侧白名单化 DOM 透传(只放行合法 HTML/data 属性)—— 与 #3291 的白名单纪律同族;或至少摘掉已知 schema 键。React 对未知小写属性有 console warning 的形态也值得核(为何测试没红)。
参考
cols键(AGENTS.md #5 / skills 两处 / site 营销样例)—— #4001 修了 catalog,生成器还在 #4011 / PR docs(skills,site,agents): 教学面改用声明键 columns,停止教未声明的 cols (#4011) #4785(发现来源)、packages/components/src/renderers/layout/grid.tsx:106-120