Skip to content

CORE-2710: Compose render-callback style in react-aria-components wrappers - #137

Open
OpenStaxClaude wants to merge 1 commit into
mainfrom
CORE-2710-compose-render-props-style
Open

CORE-2710: Compose render-callback style in react-aria-components wrappers#137
OpenStaxClaude wants to merge 1 commit into
mainfrom
CORE-2710-compose-render-props-style

Conversation

@OpenStaxClaude

@OpenStaxClaude OpenStaxClaude commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Jira: CORE-2710

The style half of the bug fixed for className in CORE-2708 / #136.

Problem

react-aria-components types style on anything extending StyleRenderProps as CSSProperties | ((values) => CSSProperties). Three wrappers merged the caller's style into their own CSS-variable object with a spread:

const popoverStyle: CSSPropertiesWithVariables = {
  '--navbar-popover-border-color': colors.palette.darkGreen,
  ...style   // <-- if style is a function, this contributes nothing
};

Spreading a function into an object literal is legal TypeScript and copies no enumerable own properties, so a caller-supplied style callback was silently discarded. Unlike the className case there was no type error to catch it.

Fix

NavBarMenuItem, NavBarPopover and TreeCheckbox now build their CSS variables inside a composeRenderProps callback, which resolves the callback form before the merge:

const popoverStyle = composeRenderProps(
  style,
  (resolvedStyle): CSSPropertiesWithVariables => ({
    '--navbar-popover-border-color': colors.palette.darkGreen,
    ...resolvedStyle
  })
);

The caller still spreads last, so its ability to override the wrapper's CSS variables is preserved.

TreeCheckbox also drops its as unknown as RACCheckboxProps['style'] double cast in favour of a CSSPropertiesWithVariables return-type annotation, which types the custom properties properly rather than casting them away.

Acceptance criteria

  • The three components merge a render-callback style instead of discarding it.
  • The object form is unchanged, including caller-last precedence over the wrapper's variables.
  • Each component has a callback-style test, plus an override test and an object-form test. Confirmed failing before the fix: checking out only the two source files from main leaves 6 failed, 18 passed across the two specs — the 6 callback/override cases fail, the rest pass.
  • No snapshot churn. All 116 snapshots pass untouched; the emitted inline styles for the object form are byte-identical (RAC's useRenderProps resolves both forms to the same object, and none of MenuItem/Popover/Checkbox supply a defaultStyle).

Rebase onto #136

Rebased onto main at bf6ef32 (#136, CORE-2708) per review. Resolution notes:

Post-rebase verification: tsc --noEmit clean, eslint clean, full suite 256 passed / 116 snapshots green.

Notes

  • ProfileMenu › opens menu on click failed its document.body snapshot once during post-rebase verification, with a stray data-pressed="true" in the received output, then passed on the immediately following full-suite run and on 3 consecutive isolated runs. Same pre-existing press-state timing flake flagged before the rebase; it involves no style and nothing this PR touches. There is a CORE-2715-profile-menu-flaky-snapshot branch for it. Flagging in case CI hits it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes render-callback style handling in React Aria component wrappers while preserving caller override precedence.

Changes:

  • Composes callback and object styles for three wrappers.
  • Adds regression and override-precedence tests.
  • Corrects Jira secret references and documents the fix.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/components/Tree/TreeCheckbox.tsx Composes checkbox styles safely.
src/components/Tree/TreeCheckbox.spec.tsx Tests callback and object styles.
src/components/NavBarMenuButtons.tsx Composes menu item and popover styles.
src/components/NavBarMenuButtons.spec.tsx Adds style regression tests.
CHANGELOG.md Documents the corrected behavior.
.github/workflows/checks.yml Corrects Jira secret names.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@RoyEJohnson
RoyEJohnson force-pushed the CORE-2710-compose-render-props-style branch from a35fb0f to 61e5db8 Compare August 27, 2026 19:46
@RoyEJohnson
RoyEJohnson marked this pull request as ready for review August 27, 2026 19:51
@RoyEJohnson
RoyEJohnson requested a review from bethshook August 27, 2026 19:52
OpenStaxClaude added a commit that referenced this pull request Aug 31, 2026
Addresses review: the four wrappers that bind CSS custom properties merged
the caller's style with an object spread. react-aria-components types style
as `CSSProperties | ((renderProps) => CSSProperties)`, and spreading a
function copies nothing, so a render-callback style was silently dropped and
replaced by the wrapper's static object.

ProfileMenuButton, ProfileMenuItem, HelpMenuButton and HelpMenuItem now build
their variables inside a composeRenderProps callback, the same fix applied to
NavBarMenuItem/NavBarPopover/TreeCheckbox in CORE-2710 (#137). The caller
still spreads last, so its ability to override the wrapper's variables is
preserved, and the object form is unchanged (all 116 snapshots pass untouched).

ProfileMenuItem and HelpMenuItem pass style down to NavBarMenuItem, so they
need the CORE-2710 fix underneath to reach the DOM; this branch is stacked on
that PR rather than duplicating it.

Each wrapper gets a callback test, an override test and an object-form test.
Confirmed failing before the fix: stashing only the two component files leaves
8 failed, 33 passed across the two specs -- the 8 callback/override cases fail,
the 4 object-form cases pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Aug 31, 2026
Addresses Copilot's two review comments. ProfileMenuButton and HelpMenuButton
merged the caller's style into their CSS-variable object with a spread.
react-aria-components types style as `CSSProperties | ((renderProps) =>
CSSProperties)`, and spreading a function copies nothing, so a render-callback
style was silently dropped and replaced by the wrapper's static object. Both now
build their variables inside composeRenderProps, the same shape CORE-2710 (#137)
uses for NavBarMenuItem/NavBarPopover/TreeCheckbox. The caller still spreads
last, and the object form is unchanged -- all 116 snapshots pass untouched.

Their style reaches react-aria's Button directly (HelpMenuButton's by way of
NavBarButton, which passes it straight through), so this works against main as
it stands.

ProfileMenuItem and HelpMenuItem have the same defect but are left spreading for
now, with a comment saying why: they hand style to NavBarMenuItem, which spreads
it as well, so composing only here would send a function into that spread and
lose these variables along with the caller's style. Both levels have to compose
together, which needs #137 on main first.

Eight tests: three per button wrapper (render-callback merged, render-callback
overriding the wrapper variables, object form merging caller-last) plus
object-form coverage for each menu item. Confirmed failing before the fix:
stashing only the two component files leaves 4 failed, 33 passed across the two
specs -- the four callback and override cases fail, the object-form cases pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@RoyEJohnson RoyEJohnson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase to main and resolve conflicts.

@OpenStaxClaude

Copy link
Copy Markdown
Contributor Author

@RoyEJohnson Rebased onto main at bf6ef32 (#136) and force-pushed — 27ab430. All checks green.

Three files conflicted; the two components auto-merged.

Verification after the rebase:

  • tsc --noEmit clean, eslint clean.
  • Full suite 256 passed / 116 snapshots, no snapshot updates.
  • Regression tests still bite: checking out only the two source files from main leaves 6 failed, 18 passed across the two specs — the six callback/override cases fail, the rest pass.

Two heads-ups:

  • ProfileMenu › opens menu on click failed its document.body snapshot once during verification with a stray data-pressed="true", then passed on the next full run and on three isolated runs. Same pre-existing press-state flake I flagged earlier — nothing to do with style. I see there's a CORE-2715-profile-menu-flaky-snapshot branch for it.
  • CORE-2006: Migrate ProfileMenu and HelpMenu components to plain CSS #141 (CORE-2006) has its base set to this branch, so the force-push moved its base out from under it. GitHub still reports it MERGEABLE — its head branched from 5fdc635 and never contained the CORE-2710 commit — but its diff will now read against the rebased tip. Worth a glance from whoever picks that one up.

Re-requesting review.

RoyEJohnson pushed a commit that referenced this pull request Aug 31, 2026
Addresses Copilot's two review comments. ProfileMenuButton and HelpMenuButton
merged the caller's style into their CSS-variable object with a spread.
react-aria-components types style as `CSSProperties | ((renderProps) =>
CSSProperties)`, and spreading a function copies nothing, so a render-callback
style was silently dropped and replaced by the wrapper's static object. Both now
build their variables inside composeRenderProps, the same shape CORE-2710 (#137)
uses for NavBarMenuItem/NavBarPopover/TreeCheckbox. The caller still spreads
last, and the object form is unchanged -- all 116 snapshots pass untouched.

Their style reaches react-aria's Button directly (HelpMenuButton's by way of
NavBarButton, which passes it straight through), so this works against main as
it stands.

ProfileMenuItem and HelpMenuItem have the same defect but are left spreading for
now, with a comment saying why: they hand style to NavBarMenuItem, which spreads
it as well, so composing only here would send a function into that spread and
lose these variables along with the caller's style. Both levels have to compose
together, which needs #137 on main first.

Eight tests: three per button wrapper (render-callback merged, render-callback
overriding the wrapper variables, object form merging caller-last) plus
object-form coverage for each menu item. Confirmed failing before the fix:
stashing only the two component files leaves 4 failed, 33 passed across the two
specs -- the four callback and override cases fail, the object-form cases pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RoyEJohnson pushed a commit that referenced this pull request Sep 1, 2026
Addresses Copilot's two review comments. ProfileMenuButton and HelpMenuButton
merged the caller's style into their CSS-variable object with a spread.
react-aria-components types style as `CSSProperties | ((renderProps) =>
CSSProperties)`, and spreading a function copies nothing, so a render-callback
style was silently dropped and replaced by the wrapper's static object. Both now
build their variables inside composeRenderProps, the same shape CORE-2710 (#137)
uses for NavBarMenuItem/NavBarPopover/TreeCheckbox. The caller still spreads
last, and the object form is unchanged -- all 116 snapshots pass untouched.

Their style reaches react-aria's Button directly (HelpMenuButton's by way of
NavBarButton, which passes it straight through), so this works against main as
it stands.

ProfileMenuItem and HelpMenuItem have the same defect but are left spreading for
now, with a comment saying why: they hand style to NavBarMenuItem, which spreads
it as well, so composing only here would send a function into that spread and
lose these variables along with the caller's style. Both levels have to compose
together, which needs #137 on main first.

Eight tests: three per button wrapper (render-callback merged, render-callback
overriding the wrapper variables, object form merging caller-last) plus
object-form coverage for each menu item. Confirmed failing before the fix:
stashing only the two component files leaves 4 failed, 33 passed across the two
specs -- the four callback and override cases fail, the object-form cases pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@RoyEJohnson
RoyEJohnson force-pushed the CORE-2710-compose-render-props-style branch from 27ab430 to e18e5ec Compare September 1, 2026 18:19
OpenStaxClaude added a commit that referenced this pull request Sep 1, 2026
Consequence of stacking this onto #137. Three of that PR's new tests asserted the
component still sets its own --navbar-menu-item-* / --navbar-popover-border-color
inline (`expect(...).toBeTruthy()`), which is exactly what this PR moves into
NavBarMenuButtons.css.

The assertions on the vanished inline defaults are gone; everything else in those
tests stays. What CORE-2710 was actually protecting — that a caller's style reaches
the element in both the object and render-callback forms — is still asserted, and
tokens.spec.ts now guards the defaults centrally.

Comments in place pointing at where the defaults live now, so the next reader does
not think the coverage was simply dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@RoyEJohnson
RoyEJohnson force-pushed the CORE-2710-compose-render-props-style branch from e18e5ec to 590bda0 Compare September 1, 2026 19:09
OpenStaxClaude added a commit that referenced this pull request Sep 1, 2026
Addresses Copilot's two review comments. ProfileMenuButton and HelpMenuButton
merged the caller's style into their CSS-variable object with a spread.
react-aria-components types style as `CSSProperties | ((renderProps) =>
CSSProperties)`, and spreading a function copies nothing, so a render-callback
style was silently dropped and replaced by the wrapper's static object. Both now
build their variables inside composeRenderProps, the same shape CORE-2710 (#137)
uses for NavBarMenuItem/NavBarPopover/TreeCheckbox. The caller still spreads
last, and the object form is unchanged -- all 116 snapshots pass untouched.

Their style reaches react-aria's Button directly (HelpMenuButton's by way of
NavBarButton, which passes it straight through), so this works against main as
it stands.

ProfileMenuItem and HelpMenuItem have the same defect but are left spreading for
now, with a comment saying why: they hand style to NavBarMenuItem, which spreads
it as well, so composing only here would send a function into that spread and
lose these variables along with the caller's style. Both levels have to compose
together, which needs #137 on main first.

Eight tests: three per button wrapper (render-callback merged, render-callback
overriding the wrapper variables, object form merging caller-last) plus
object-form coverage for each menu item. Confirmed failing before the fix:
stashing only the two component files leaves 4 failed, 33 passed across the two
specs -- the four callback and override cases fail, the object-form cases pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 1, 2026
CORE-2708 (#136) is on main now, so NavBarButton, NavBarMenuItem and
NavBarPopover compose className through composeRenderProps. The wrappers added
here can follow: ProfileMenuButton, ProfileMenuPopover, ProfileMenuItem,
HelpMenuButton and HelpMenuItem passed the caller's className straight into
classNames(), which ignores functions, so a render-callback className was
silently dropped -- and unlike the style case TypeScript does not catch it.

Composing only at this level was not possible before #136: the composed function
would have reached NavBarMenuItem's classNames() and been discarded there,
taking the profile-menu-item / help-menu-item class with it.

Two tests per component, matching the shape #136 uses: a render-callback
className reaches the DOM alongside the wrapper's own class, and a string
className still composes. Confirmed failing before the fix: stashing only the
two component files leaves the two render-callback cases red.

The style half of this is still split. The two button wrappers compose it; the
two menu items cannot until CORE-2710 (#137) lands, because NavBarMenuItem
still spreads style. Comments in both files say so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 1, 2026
Consequence of stacking this onto #137. Three of that PR's new tests asserted the
component still sets its own --navbar-menu-item-* / --navbar-popover-border-color
inline (`expect(...).toBeTruthy()`), which is exactly what this PR moves into
NavBarMenuButtons.css.

The assertions on the vanished inline defaults are gone; everything else in those
tests stays. What CORE-2710 was actually protecting — that a caller's style reaches
the element in both the object and render-callback forms — is still asserted, and
tokens.spec.ts now guards the defaults centrally.

Comments in place pointing at where the defaults live now, so the next reader does
not think the coverage was simply dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ppers

NavBarMenuItem, NavBarPopover and TreeCheckbox merged the caller's `style`
into their own CSS-variable object with a spread. RAC types `style` as
`CSSProperties | ((renderProps) => CSSProperties)`, and spreading a function
into an object literal copies no enumerable own properties, so a
render-callback `style` was silently discarded — with no type error to catch
it.

Each wrapper now builds its CSS variables inside a `composeRenderProps`
callback, so the caller's declarations land on the element in both forms.
The caller still spreads last and keeps its ability to override the
wrapper's variables, and the emitted inline styles for the object form are
unchanged (snapshots pass untouched).

TreeCheckbox's `as unknown as RACCheckboxProps['style']` double cast is
replaced by a `CSSPropertiesWithVariables` return-type annotation, which
types the custom properties properly instead of casting them away.
@RoyEJohnson
RoyEJohnson force-pushed the CORE-2710-compose-render-props-style branch from 590bda0 to 9a9bae3 Compare September 1, 2026 19:52
OpenStaxClaude added a commit that referenced this pull request Sep 1, 2026
Addresses Copilot's two review comments. ProfileMenuButton and HelpMenuButton
merged the caller's style into their CSS-variable object with a spread.
react-aria-components types style as `CSSProperties | ((renderProps) =>
CSSProperties)`, and spreading a function copies nothing, so a render-callback
style was silently dropped and replaced by the wrapper's static object. Both now
build their variables inside composeRenderProps, the same shape CORE-2710 (#137)
uses for NavBarMenuItem/NavBarPopover/TreeCheckbox. The caller still spreads
last, and the object form is unchanged -- all 116 snapshots pass untouched.

Their style reaches react-aria's Button directly (HelpMenuButton's by way of
NavBarButton, which passes it straight through), so this works against main as
it stands.

ProfileMenuItem and HelpMenuItem have the same defect but are left spreading for
now, with a comment saying why: they hand style to NavBarMenuItem, which spreads
it as well, so composing only here would send a function into that spread and
lose these variables along with the caller's style. Both levels have to compose
together, which needs #137 on main first.

Eight tests: three per button wrapper (render-callback merged, render-callback
overriding the wrapper variables, object form merging caller-last) plus
object-form coverage for each menu item. Confirmed failing before the fix:
stashing only the two component files leaves 4 failed, 33 passed across the two
specs -- the four callback and override cases fail, the object-form cases pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 1, 2026
CORE-2708 (#136) is on main now, so NavBarButton, NavBarMenuItem and
NavBarPopover compose className through composeRenderProps. The wrappers added
here can follow: ProfileMenuButton, ProfileMenuPopover, ProfileMenuItem,
HelpMenuButton and HelpMenuItem passed the caller's className straight into
classNames(), which ignores functions, so a render-callback className was
silently dropped -- and unlike the style case TypeScript does not catch it.

Composing only at this level was not possible before #136: the composed function
would have reached NavBarMenuItem's classNames() and been discarded there,
taking the profile-menu-item / help-menu-item class with it.

Two tests per component, matching the shape #136 uses: a render-callback
className reaches the DOM alongside the wrapper's own class, and a string
className still composes. Confirmed failing before the fix: stashing only the
two component files leaves the two render-callback cases red.

The style half of this is still split. The two button wrappers compose it; the
two menu items cannot until CORE-2710 (#137) lands, because NavBarMenuItem
still spreads style. Comments in both files say so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 1, 2026
Addresses Copilot's two review comments. ProfileMenuButton and HelpMenuButton
merged the caller's style into their CSS-variable object with a spread.
react-aria-components types style as `CSSProperties | ((renderProps) =>
CSSProperties)`, and spreading a function copies nothing, so a render-callback
style was silently dropped and replaced by the wrapper's static object. Both now
build their variables inside composeRenderProps, the same shape CORE-2710 (#137)
uses for NavBarMenuItem/NavBarPopover/TreeCheckbox. The caller still spreads
last, and the object form is unchanged -- all 116 snapshots pass untouched.

Their style reaches react-aria's Button directly (HelpMenuButton's by way of
NavBarButton, which passes it straight through), so this works against main as
it stands.

ProfileMenuItem and HelpMenuItem have the same defect but are left spreading for
now, with a comment saying why: they hand style to NavBarMenuItem, which spreads
it as well, so composing only here would send a function into that spread and
lose these variables along with the caller's style. Both levels have to compose
together, which needs #137 on main first.

Eight tests: three per button wrapper (render-callback merged, render-callback
overriding the wrapper variables, object form merging caller-last) plus
object-form coverage for each menu item. Confirmed failing before the fix:
stashing only the two component files leaves 4 failed, 33 passed across the two
specs -- the four callback and override cases fail, the object-form cases pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 1, 2026
CORE-2708 (#136) is on main now, so NavBarButton, NavBarMenuItem and
NavBarPopover compose className through composeRenderProps. The wrappers added
here can follow: ProfileMenuButton, ProfileMenuPopover, ProfileMenuItem,
HelpMenuButton and HelpMenuItem passed the caller's className straight into
classNames(), which ignores functions, so a render-callback className was
silently dropped -- and unlike the style case TypeScript does not catch it.

Composing only at this level was not possible before #136: the composed function
would have reached NavBarMenuItem's classNames() and been discarded there,
taking the profile-menu-item / help-menu-item class with it.

Two tests per component, matching the shape #136 uses: a render-callback
className reaches the DOM alongside the wrapper's own class, and a string
className still composes. Confirmed failing before the fix: stashing only the
two component files leaves the two render-callback cases red.

The style half of this is still split. The two button wrappers compose it; the
two menu items cannot until CORE-2710 (#137) lands, because NavBarMenuItem
still spreads style. Comments in both files say so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 1, 2026
Follows the CORE-2720 (#143) sweep, which this branch is now stacked on. The two
new stylesheets repeated eleven palette hexes as var() fallbacks; they now read
the --ox-* tokens instead, e.g.

  color: var(--help-menu-button-color, var(--ox-color-gray));

The override hooks are unchanged -- only their defaults moved from JavaScript to
the CSS side, so the components no longer bind static custom properties inline.
That means style is no longer destructured in ProfileMenuButton,
ProfileMenuItem, HelpMenuButton or HelpMenuItem: it passes through in ...props
and react-aria handles both the object and render-callback forms itself.

Two consequences worth naming:

- The CORE-2710 (#137) dependency is gone rather than deferred. The bug it
  guards against was a wrapper overwriting the caller's style, which these
  wrappers no longer do, so the menu items need nothing from #137. Same
  reasoning as the note #143 leaves on NavBarMenuItem.
- iframeWrapperStyle and putAwayStyle are gone; the iframe wrapper and the
  put-away bar take their colours from HelpMenu.css.

className composition stays -- that one is a real bug fix, not a default.

The specs that asserted the inline defaults now assert what matters instead:
the caller's style reaches the element in both forms, and the override hook
still wins. Defaults are covered centrally by src/theme/tokens.spec.ts, which
also fails on any colour literal that duplicates a theme value -- both new
stylesheets pass it.

Also carries CORE-2715 (#138) as a cherry-pick: it is on main but not yet in
this base, and without it the old flaky ProfileMenu snapshot fails against the
migrated component. It drops out as a duplicate when #143 rebases onto main.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 2, 2026
Consequence of stacking this onto #137. Three of that PR's new tests asserted the
component still sets its own --navbar-menu-item-* / --navbar-popover-border-color
inline (`expect(...).toBeTruthy()`), which is exactly what this PR moves into
NavBarMenuButtons.css.

The assertions on the vanished inline defaults are gone; everything else in those
tests stays. What CORE-2710 was actually protecting — that a caller's style reaches
the element in both the object and render-callback forms — is still asserted, and
tokens.spec.ts now guards the defaults centrally.

Comments in place pointing at where the defaults live now, so the next reader does
not think the coverage was simply dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 2, 2026
Addresses Copilot's two review comments. ProfileMenuButton and HelpMenuButton
merged the caller's style into their CSS-variable object with a spread.
react-aria-components types style as `CSSProperties | ((renderProps) =>
CSSProperties)`, and spreading a function copies nothing, so a render-callback
style was silently dropped and replaced by the wrapper's static object. Both now
build their variables inside composeRenderProps, the same shape CORE-2710 (#137)
uses for NavBarMenuItem/NavBarPopover/TreeCheckbox. The caller still spreads
last, and the object form is unchanged -- all 116 snapshots pass untouched.

Their style reaches react-aria's Button directly (HelpMenuButton's by way of
NavBarButton, which passes it straight through), so this works against main as
it stands.

ProfileMenuItem and HelpMenuItem have the same defect but are left spreading for
now, with a comment saying why: they hand style to NavBarMenuItem, which spreads
it as well, so composing only here would send a function into that spread and
lose these variables along with the caller's style. Both levels have to compose
together, which needs #137 on main first.

Eight tests: three per button wrapper (render-callback merged, render-callback
overriding the wrapper variables, object form merging caller-last) plus
object-form coverage for each menu item. Confirmed failing before the fix:
stashing only the two component files leaves 4 failed, 33 passed across the two
specs -- the four callback and override cases fail, the object-form cases pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 2, 2026
CORE-2708 (#136) is on main now, so NavBarButton, NavBarMenuItem and
NavBarPopover compose className through composeRenderProps. The wrappers added
here can follow: ProfileMenuButton, ProfileMenuPopover, ProfileMenuItem,
HelpMenuButton and HelpMenuItem passed the caller's className straight into
classNames(), which ignores functions, so a render-callback className was
silently dropped -- and unlike the style case TypeScript does not catch it.

Composing only at this level was not possible before #136: the composed function
would have reached NavBarMenuItem's classNames() and been discarded there,
taking the profile-menu-item / help-menu-item class with it.

Two tests per component, matching the shape #136 uses: a render-callback
className reaches the DOM alongside the wrapper's own class, and a string
className still composes. Confirmed failing before the fix: stashing only the
two component files leaves the two render-callback cases red.

The style half of this is still split. The two button wrappers compose it; the
two menu items cannot until CORE-2710 (#137) lands, because NavBarMenuItem
still spreads style. Comments in both files say so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenStaxClaude added a commit that referenced this pull request Sep 2, 2026
Follows the CORE-2720 (#143) sweep, which this branch is now stacked on. The two
new stylesheets repeated eleven palette hexes as var() fallbacks; they now read
the --ox-* tokens instead, e.g.

  color: var(--help-menu-button-color, var(--ox-color-gray));

The override hooks are unchanged -- only their defaults moved from JavaScript to
the CSS side, so the components no longer bind static custom properties inline.
That means style is no longer destructured in ProfileMenuButton,
ProfileMenuItem, HelpMenuButton or HelpMenuItem: it passes through in ...props
and react-aria handles both the object and render-callback forms itself.

Two consequences worth naming:

- The CORE-2710 (#137) dependency is gone rather than deferred. The bug it
  guards against was a wrapper overwriting the caller's style, which these
  wrappers no longer do, so the menu items need nothing from #137. Same
  reasoning as the note #143 leaves on NavBarMenuItem.
- iframeWrapperStyle and putAwayStyle are gone; the iframe wrapper and the
  put-away bar take their colours from HelpMenu.css.

className composition stays -- that one is a real bug fix, not a default.

The specs that asserted the inline defaults now assert what matters instead:
the caller's style reaches the element in both forms, and the override hook
still wins. Defaults are covered centrally by src/theme/tokens.spec.ts, which
also fails on any colour literal that duplicates a theme value -- both new
stylesheets pass it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@RoyEJohnson
RoyEJohnson requested review from Dantemss and a balanced review from Copilot September 2, 2026 20:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

All reviewed changes are covered by regression tests with no unresolved issues.

Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

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.

3 participants