Skip to content

fix(Calendar,DropdownMenu,NavigationMenu): stop props leaking into the DOM as attributes - #545

Merged
IgorShevchik merged 4 commits into
mainfrom
claude/repo-rules-35um4e
Sep 7, 2026
Merged

fix(Calendar,DropdownMenu,NavigationMenu): stop props leaking into the DOM as attributes#545
IgorShevchik merged 4 commits into
mainfrom
claude/repo-rules-35um4e

Conversation

@IgorShevchik

@IgorShevchik IgorShevchik commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Linked issue

Closes #477

Type of change

  • Documentation (updates to the documentation or readme)
  • Bug fix (a non-breaking change that fixes an issue)
  • Enhancement (improving an existing functionality)
  • New feature (a non-breaking change that adds functionality)
  • Chore (updates to the build process or auxiliary tools and libraries)
  • Revert (undoing a merged change — retitle this PR revert(Scope): ...)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Three components were putting a prop into the DOM as a fall-through attribute. #477 named one of them; review found the second, and a sweep of the corpus found the third.

Each forwards its props to a reka root through useForwardProps minus a hand-maintained exclusion list, and each list was missing an entry.

where attribute whose bug
Calendar nextmonth nextyear prevmonth prevyear, all ="[object Object]" upstream has it too
DropdownMenuContent arrow="true", or ="[object Object]" for the object form ours — a porting slip
NavigationMenu (test only) arrow="true" on <nav> upstream's test, inherited

Calendar#477

Four button-config props were missing from omittedProps, so Vue rendered each as junk on the picker root:

nextmonth="[object Object]"   nextyear="[object Object]"
prevmonth="[object Object]"   prevyear="[object Object]"

viewControl was already in the list — the tell that the class of bug was known and one of five was covered. The other four are declared identically, as Omit<ButtonProps, LinkPropsKeys>.

Nothing else moved: four changed lines per snapshot file, each dropping only the leaked attribute; [object Object] count 4 → 0.

Worth checking that the existing cases were not vacuous, because before this change with nextYear and its three siblings differed from a plain calendar partly because of the leaked attribute. They did not collapse — ui-btn-lg appears once in each of the four and zero times in the plain calendar, so { size: 'lg' } genuinely reaches its button.

Upstream has the identical defect — same four props, same list without them (only ui vs b24ui differs), same four attributes in its own committed snapshots. Fixing it costs no divergence: that line already differs from upstream, so it is a merge point on every sync either way.

DropdownMenuContent — found in review

arrow is ours, not reka's. Nothing in the DropdownMenuContentMenuContentPropsPopperContentProps chain declares it — only arrowPadding and hideShiftedArrow. Upstream keeps its arrow in DropdownMenu.vue; we moved it down into DropdownMenuContent so submenus could carry one, and never updated the reactiveOmit list. So it reached the content root as an attribute. The arrow element reads props.arrow directly, so omitting it forwards nothing away.

Only the boolean form was in the corpus, because no render case passed an object. with arrow object adds one, and it pins the other branch of arrowProps: width="12" height="8" against the 20 / 10 default.

The other six components declaring arrow build their forwarded props with reactivePick — a whitelist, safe by construction. ContextMenuContent uses reactiveOmit but has no arrow.

NavigationMenu — a vacuous case, removed

Inherited from upstream, whose NavigationMenu does have an arrow prop and a NavigationMenuIndicator. We did not port either (the @memo on the props interface lists arrow among the things to remove), but the test case came across anyway, still passing arrow: true. Vue put the undeclared prop on <nav>.

That stray attribute was the only thing the case asserted. Measured rather than assumed: strip it and the #454 collision guard immediately reports the snapshot as byte-identical to both with modelValue and with defaultValue. Removed, with a comment in its place so the next sync does not bring it back.

The guard, and what it cannot see

The props all worked, every component rendered correctly, and the only symptom was junk in the markup — which the snapshots dutifully recorded as expected output, for as long as the files have existed.

New test/utils/stringified-props.spec.ts scans the whole corpus for ="[object Object]", built in the shape of the collision guard beside it and reusing its traversal so the two cannot disagree about what the corpus is. It asserts an empty list, not a baseline. Verified by restoring each leak: it goes red in both projects and names the file and the attribute.

Its blind spot is stated in the file, because it is not theoretical. A leak with a scalar value — arrow="true" — is indistinguishable from an intentional attribute by any text pattern. Both arrow leaks were in the corpus in exactly that form when the guard was written, and it saw neither; they were found by reading the components. A green run here means no object-valued leak, not no leak.

Correction to this PR's earlier description

It claimed, as a measured fact, that Calendar was the only site of this class, and rejected the issue's suggested general fix on that basis. That was wrong — there were three.

The conclusion survives the correction, for a different reason: of the components that forward props to a reka root, only these use a blacklist; the rest use reactivePick, which cannot leak an undeclared prop. Deriving the exclusion list automatically would still be machinery, but the honest argument is "two blacklists left", not "one site".

Honest note on value

#477 rates itself P3 and that is right: the props work, users see nothing, the fix is cosmetic in the DOM. What it buys is a few bytes less nonsense per server-rendered component, a snapshot corpus that no longer certifies a defect as correct output, one genuinely vacuous test case gone, and a guard that turns half of this bug class from invisible into a failing test.

Checks

  • pnpm lint, pnpm typecheck green
  • indistinguishable-snapshots guard green — no new collision, and the removed NavigationMenu case is not silently rejoining an existing one
  • stringified-props guard verified red under a restored leak, in both projects

Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

🤖 Generated with Claude Code

https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc


Generated by Claude Code

…root

`Calendar` forwards its props to the reka picker root through
`useForwardProps`, minus a hand-maintained `omittedProps` list. Four
button-config props were missing from it, so `reactiveOmit` kept them and Vue
rendered each as a fall-through attribute:

    nextmonth="[object Object]"   nextyear="[object Object]"
    prevmonth="[object Object]"   prevyear="[object Object]"

`viewControl` was already in the list, which is the tell: the class of bug was
known and one of five was covered. The other four are declared exactly the
same way — `Omit<ButtonProps, LinkPropsKeys>`, an object of button
configuration.

Adding them removes exactly those four attributes from eight snapshot entries
and moves nothing else: four changed lines per file, each dropping only the
leaked attribute. That was checked rather than asserted, because "nothing else
should move" is the kind of claim this repository has learned not to take on
trust.

**The existing cases were not vacuous, and now they prove it.** Before this,
`with nextYear` and its three siblings differed from a plain calendar partly
*because of the leaked attribute*. With the leak gone they still differ, and
for the right reason: each renders exactly one `ui-btn-lg` where a plain
calendar renders none, so `{ size: 'lg' }` genuinely reaches its button. The
#454 collision guard reports no new collision either.

**Nothing could have caught this**, which is the more interesting half. The
props worked, the calendar rendered correctly, and the only symptom was junk
in the markup — which the snapshot dutifully recorded as expected output. So
this adds `test/utils/stringified-props.spec.ts`: a scan of the whole snapshot
corpus for `="[object Object]"`, in the shape of the collision guard beside it
and reusing its traversal. It asserts an empty list rather than a baseline,
because the corpus held exactly these four.

Verified by restoring the leak: the guard goes red in both projects and names
the file and the attribute.

**Upstream has the identical defect** — same four props, same `omittedProps`
list without them (only `ui` vs `b24ui` differs), and the same four attributes
sitting in its own committed snapshots. Fixing it here is nonetheless free of
divergence cost: that line already differs from upstream, so it is a merge
point on every sync either way.

Not done, deliberately: the issue suggests deriving the list from the props
that are b24ui's own instead of hand-maintaining it. Measured first — 19
components declare `Omit<ButtonProps, LinkPropsKeys>` props, but only
`Calendar` pairs `omittedProps` with `useForwardProps`, so the class has
exactly one site. A derivation mechanism would be machinery for a single case.

Closes #477

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
`arrow` is a b24ui addition, not reka's -- nothing in the
`DropdownMenuContent` -> `MenuContentProps` -> `PopperContentProps` chain
declares it (only `arrowPadding` and `hideShiftedArrow`). Upstream keeps its
arrow in `DropdownMenu.vue`; we moved it into `DropdownMenuContent` so
submenus could carry one, and the `reactiveOmit` list was never updated, so
`arrow` reached the content root as an attribute: `arrow="true"` for the
boolean form, `arrow="[object Object]"` for the object one. The arrow element
reads `props.arrow` directly, so omitting it forwards nothing away.

Only the boolean form was in the corpus, because no render case passed an
object. `with arrow object` adds one, and it pins the other branch of
`arrowProps`: width 12 / height 8 against the 20 / 10 default.

Drop `NavigationMenu`'s `with arrow` case, inherited from upstream, which
still passed `arrow: true` although neither the prop nor its indicator were
ported. Vue put the undeclared prop on `<nav>`, and that stray attribute was
the only thing distinguishing the case from `with modelValue` -- with it
removed the collision guard reports the two as identical.

Correct the claim in `stringified-props.spec.ts` that `Calendar` was the only
site of this bug: there were three. Record what the guard cannot see -- a leak
with a scalar value is indistinguishable from an intentional attribute by any
text pattern, and this one found neither `arrow` leak.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
@IgorShevchik IgorShevchik changed the title fix(Calendar): stop four button-config props leaking onto the picker root fix(Calendar,DropdownMenu,NavigationMenu): stop props leaking into the DOM as attributes Sep 7, 2026
@IgorShevchik
IgorShevchik merged commit 299a7ab into main Sep 7, 2026
2 checks passed
@IgorShevchik
IgorShevchik deleted the claude/repo-rules-35um4e branch September 7, 2026 10:04
IgorShevchik added a commit that referenced this pull request Sep 9, 2026
…#561)

The guard added with #545 read all 218 snapshot files inside its `it()`, so
28 MB of I/O sat under vitest's 5s per-test default. Measured: ~0.5s in plain
node, 1.9s in the `nuxt` project on its own — 38% of the budget with no
contention — and past 5s under a full run sharing forks with 346 other files.

It timed out that way twice. The first time was reported as an unexplained
single failure that three clean re-runs could not reproduce and that I would
not call flaky without a name; this is the name.

The scan now happens in the describe body, which is how the collision guard
beside it has always done the same read. Describe-body work is not subject to
the per-test timeout, so the assertions are free and the corpus is read once
instead of once per assertion.

No change to what the guard catches: verified by appending an `[object Object]`
attribute to a snapshot, which reds it in both projects and names the file and
the attribute. Worth recording that the first attempt at that mutation stayed
green and the guard was briefly suspect — `printf` had escaped the quotes, so
the injected text did not match a pattern that was working correctly. A
mutation that fails to go red is a claim about the mutation before it is a
claim about the test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Calendar: four button-config props fall through to the picker root as attributes

2 participants