fix(react): mapDensity abstains on non-string rowHeight instead of coercing it (#4459) - #4469
Merged
Merged
Conversation
…ercing it (#4459) `mapDensity` opened with a truthiness guard, so any truthy non-string survived it and was then coerced into a lookup key -- both `hasOwnProperty.call` and the table index run `String(...)`. `['compact']`, a boxed `String('compact')` and `{ toString: () => 'compact' }` therefore each selected a real density, while core's `rowHeightToDensityMode` -- which opens with `typeof rowHeight !== 'string'` -- abstained for the same input. Replace the truthiness guard with core's type guard. The type guard subsumes the old one (undefined/null/0/false are all non-strings) and `''` keeps its answer by a different route: a string now, refused one line later by the existing `hasOwnProperty` guard because it is not one of the five spec keys. Extends the agreement pin with the non-string family, closing the gap between what the file's title claims and what it pinned -- both prior off-spec families are strings. Red-first: 7 failures pre-fix, all three coercing non-strings; 38/38 green after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
The #4459 note said core's twin lives in the "same file". It does not -- `rowHeightToDensityMode` is in `@object-ui/core` (`packages/core/src/utils/normalize-list-view.ts:83`). Comment-only; cite the path the way the rest of this file's notes do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
ACCEPT — step-7 复核 by PM session
rowHeight family is now fully closed on BOTH input families: core, bridge, ListView, and standalone ObjectGrid give one answer on off-spec strings (#4447/#4457/#4458), and core + bridge agree on non-strings (this PR). Flipping ready + arming auto-merge. Slot not refilled — per the current maintainer instruction this wave winds down after #4456. Generated by Claude Code Generated by Claude Code |
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
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.
Closes #4459
The defect
mapDensityinpackages/react/src/spec-bridge/bridges/list-view.tsopened with a truthiness guard:That rejects only falsy values. Any truthy non-string walked straight past it into a lookup that coerces its key — both
Object.prototype.hasOwnProperty.calland the table index runString(...). So a value that is not a string at all still selected a density, while the core twin (packages/core/src/utils/normalize-list-view.ts:83), which opens with a type guard, abstained:rowHeight['compact']undefined'compact'undefinednew String('compact')undefined'compact'undefined{ toString: () => 'compact' }undefined'compact'undefinedNote the direction against #4442: that leak returned a function, visibly wrong to everything downstream. This one returned a legitimate-looking
'compact'that nothing downstream can tell apart from an authored density — andbridgeListViewwrites the key underif (density), so it was not merely returned, it was stored on the SchemaNode.The fix
One line — core's opening guard, mirrored:
The type guard subsumes the one it replaces (
undefined,null,0andfalseare all non-strings), and the #4457hasOwnPropertyguard is kept as-is after it.''keeps its answer while changing route: falsy before, so it never reached the table; a string now, so it passes this guard and is refused one line later because it is not one of the five spec keys. Pinned explicitly rather than argued.Red-first
The extended pin was run against the unfixed source first. It went red, verbatim:
All 7 failures came from the three coercing non-strings (the two
it.eachblocks plus the invariant loop). The non-coercing rows —42,true,0,false,null,undefined— passed before the fix too, which is the honest reading: they are completeness rows, not regression rows. They are pinned anyway because this change replaces the truthiness guard rather than adding to it, so the new guard has to keep catching everything the old one caught.After the fix, the same file:
Tests 38 passed (38). The 31 pre-existing cases — all five spec row heights, and the string off-spec plus prototype-member families from #4447/#4457 — stayed green throughout, in both directions.Verification
pnpm exec vitest run packages/react/src/spec-bridge/__tests__/RowHeightDensityAgreement.test.tspnpm exec vitest run packages/react/ --maxWorkers=2pnpm --filter @object-ui/react type-checktsc --noEmitandtsc -p tsconfig.test.json)eslinton both changed filesnode scripts/check-changeset-presence.mjsEmitted types were diffed pre/post build: no
.d.tsmoved (mapDensityis module-local; the only dist change is the guard line and its comment inlist-view.js), so the changeset is patch for@object-ui/reactper the ruling.Surface:
packages/react/src/spec-bridge/**plus one changeset. Nothing else.Generated by Claude Code