fix(web): close reasoning picker after selection - #4523

💭 Looking at MenuRadioItem, it already has a closeOnClick prop that handles closing the menu, so adding onRequestClose might create redundancy since the controlled Menu state would trigger onOpenChange(false) anyway. There's also a memoization concern if onRequestClose is passed as an inline arrow function, though there seem to be other issues to consider as well. I'm noticing that MenuRadioItem already passes closeOnClick through to Base UI's RadioItem, and there's even a comment explaining that closeOnClick was added to close the menu on selection since Base UI keeps radio menus open by default. So I'm wondering why the PR is adding onRequestClose if closeOnClick should already be handling this — maybe closeOnClick isn't actually working as expected, so I should check the git history to see when closeOnClick was added and what might have changed. The new approach of calling setIsMenuOpen(false) directly does bypass Base UI's close props, though it should still trigger the exit animation and focus return since Base UI's FloatingFocusManager handles that on unmount regardless of how the open prop changes. The real concern is potential redundancy — closeOnClick would already close the menu, and now onRequestClose would close it again, which is harmless but unnecessary. I should check if TraitsMenuContent is used elsewhere without onRequestClose being provided. The real concern here is whether directly setting isMenuOpen to false actually bypasses Base UI's internal close logic in a way that breaks accessibility or interaction semantics. If the controlled state override prevents Base UI from running its normal close path, that could be a regression—the guideline pushes toward using Base UI's built-in mechanisms like closeOnClick rather than working around them with manual state management. I should also check whether the same pattern appears elsewhere, like in ChatComposer's menu handling, to see if this is a systemic issue or isolated to this one component. I'm trying to track down where providerTraitsMenuContent is being used in the JSX, so I'm searching through the codebase to find its rendering location. I'm wondering if the uncontrolled compact menu's closeOnClick already works, but the issue is specific to the controlled TraitsPicker menu where closeOnClick isn't dismissing the menu. With a controlled Base UI Menu, calling closeOnClick should trigger onOpenChange(false) and close it, unless Base UI's RadioItem doesn't actually support the closeOnClick prop and it's being passed as an unknown attribute to the DOM element. I should check the Base UI version and whether RadioItem has this prop. The closeOnClick prop is already implemented on MenuRadioItem, so the issue isn't missing functionality—it's that the menu isn't actually closing when it should. I'm tracing through the logic: MenuRadioGroup's onValueChange updates state and triggers a parent re-render, which should cause closeOnClick to fire and call onOpenChange(false) to close the menu. But the bug report suggests this isn't happening, so something's breaking the chain. I'm wondering if the real culprit is that state updates from updateModelOptions or composerDraftStore cause the component tree to remount, resetting the menu state, or if onPromptChange triggers a re-render that reopens it. Regardless, the PR adds an imperative close alongside closeOnClick, which creates a duplicate close path—and that's potentially a violation of the accessibility rules about preserving Base UI's render/close semantics during migrations. But checking for actual regressions: Base UI's FloatingFocusManager returns focus to the trigger regardless of how the close is initiated, and controlled open works fine with external state changes, so there's no clear a11y issue. Keyboard navigation through RadioItem doesn't trigger on highlight, only on click or Enter/Space, so that's fine too. The onValueChange and closeOnClick both fire and close the menu redundantly but consistently. There's one real behavioral shift though with the boolean radio group and select handlers now closing unconditionally on user selection, though disabled items can't be clicked anyway. The existing comment about keeping the menu open by default and closing on pick still holds—the new onRequestClose makes closeOnClick technically redundant for the TraitsPicker path, but the compact uncontrolled menu still needs it, so both have to stay. That's actually correct. The bug must be specific to the controlled Menu variant in Base UI. When open is controlled, closeOnClick internally calls setOpen which triggers onOpenChange, and TraitsPicker's onOpenChange updates isMenuOpen state—that should close the menu. Unless selecting an option causes a store update that re-renders and recreates the MenuTrigger render prop, though that shouldn't reopen it. Let me check issue #4522 for more context. The issue was already closed by a bot on August 10, 2026, likely fixed in another PR. Looking at the merge base, I can see that closeOnClick was already implemented on MenuRadioItem with a comment explaining that it closes the menu on selection to match the behavior of the model picker, since Base UI keeps radio menus open by default. This means the current PR is adding a redundant close mechanism that duplicates what's already been handled through the primitive's close prop. Let me check the git history for TraitsPicker to confirm when closeOnClick was originally added. Looking at the bug history, the main branch already resolved this issue back in August using Base UI's closeOnClick prop on MenuRadioItem, which handles both the controlled and uncontrolled menu variants. This PR appears to be adding a redundant imperative close mechanism that duplicates existing functionality. I'm checking whether this violates the accessibility rules around preserving Base UI's render and close props. The PR's approach creates a component-specific workaround by manually controlling state instead of relying on the shared Menu primitive's built-in close behavior. There is a real behavioral difference: calling onRequestClose imperatively during the change handler bypasses Base UI's close pipeline, which normally handles close reasons and focus restoration. However, Base UI's focus management through useFocusGuard and FloatingFocusManager should still restore focus to the trigger regardless of how the close is initiated, so that part is probably fine. The more concrete issue is that synchronously setting open=false before Base UI finishes its own close handling could cause the popup to unmount mid-event, potentially losing focus since the item gets removed before the event completes. While React batches both updates in the same commit, this still represents a duplication of close ownership — the shared primitive already handles closing via closeOnClick, which was added specifically to address this exact issue. The real finding is consistency: TraitsPicker now has a manual onRequestClose prop wired to close the menu, but CompactComposerControlsMenu still relies on the primitive's closeOnClick, creating two divergent close mechanisms for the same menu content. Additionally, the inline arrow function passed as onRequestClose defeats memoization on TraitsMenuContent since it creates a new identity on each render, which is a minor performance issue. The PR changes behavior and prop forwarding without focused tests to validate those changes. This feels like a legitimate failure case: the PR reintroduces a fix that already exists in the main branch through the primitive, creating redundancy and divergence. I've verified the merge base has closeOnClick present, and the onRequestClose approach achieves the same behavioral result as the existing mechanism, making this a no-op that adds complexity rather than solving a real problem. There's a potential risk worth considering though — if Base UI defers the close operation for animation timing, the manual immediate close could unmount the popup before Base UI's handler completes, potentially leaving stale state or skipping focus restoration. But that's speculative; the core issue remains the duplication and unclear ownership of the close logic. I should check the Base UI v1.4.1 implementation directly to understand how useMenuItem handles the close behavior. The real issue here is that the PR introduces a redundant close mechanism. The Menu component from Base UI already handles closing synchronously through its closeOnClick prop, which triggers onOpenChange(false) and updates the menu state. Adding a separate onRequestClose prop with manual setIsMenuOpen(false) creates a duplicate close path that diverges from how the component was originally designed to work. This is a genuine consistency problem — two competing close mechanisms in the same component. I should post an inline comment at the onRequestClose handler and fail the check, since this represents a regression in design consistency even though it's functionally redundant. I need to reference line 548 on the right side of the diff for the review comment.

1. modify_pr — One finding: the traits menu now has two competing close paths. The shared Menu primitive already owns closing via closeOnClick; see the inline comment.