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

💭 I'll examine the file to see what changes were made. I need to check menu.tsx to see if MenuItem and MenuRadioItem already have cursor-pointer styling, and whether MenuTrigger uses a render prop that might cause className conflicts. I should also look at ComposerControl to understand how it's structured. I'm noticing that MenuRadioItem already has cursor-pointer built into its base classes, so adding it again at the call sites would be redundant. The real issue is how tailwind-merge handles the interaction between cursor-pointer and the data-disabled:cursor-not-allowed variant—since they're in different modifier groups, the call-site class might not properly override the disabled state behavior. Actually, looking at CSS specificity, the attribute selector in data-disabled:cursor-not-allowed has higher specificity than the unprefixed cursor-pointer, so the disabled cursor would win anyway. But this redundancy is still worth flagging as a violation of the primitive's ownership of the cursor property—the call sites shouldn't be duplicating what the component already declares. Now I'm checking the Button primitive to see if it has similar issues. Button likely has cursor-pointer built in. I'm noticing the onOptionSelected callback combined with closeOnClick on MenuRadioItem could trigger a double-close, though that's not harmful. The disabled case where ultrathinkInBodyText is true prevents the close anyway since the item is disabled. I should check if TraitsMenuContent is used elsewhere and whether this menu-close behavior causes issues at those other call sites. I'm noticing a discrepancy between the line numbers from my view and what git grep is showing — the view had TraitsMenuContent at line 222, but grep reports it at 226 with TraitsMenuContentImplProps at 221. Both should be referencing the same commit (c4fdd43), so it's strange that the results don't match up. Let me check if the PR head has been updated since I last viewed it, or if git grep is pulling from a different revision than I expected. I'll fetch the file directly at c4fdd43 using git show to verify what's actually there. I see there's a recent commit on main that might already handle the trait menu closing behavior. Let me check the diff to understand what changes are actually in this branch versus what's already been merged from main — the merge base should clarify what's new here versus what's already implemented. Looking at the line numbers in the diff, it seems the default revision is pointing to main rather than the specific commit, but that's not the key issue. What matters is whether main already has the menu closing logic via closeOnClick — and it does, from commit #5879. So this PR appears to be adding a second closing mechanism with onOptionSelected and a controlled open state, which might be redundant. I need to check whether the controlled state itself is new in this PR or if it already existed at the merge base. The diff only shows onOptionSelected={() => setIsMenuOpen(false)} being added, suggesting isMenuOpen was already there. Since main includes closeOnClick, the PR's imperative close through state is potentially redundant — two separate paths closing the menu. That could be a duplication concern, though it's mild since both mechanisms should work together. The more subtle issue is that onOptionSelected is now a new optional behavioral prop on TraitsMenuContent, which changes its contract. But here's the real problem: TraitsPicker uses TraitsMenuContentProps for its own props type, so consumers can pass onOptionSelected to TraitsPicker even though it doesn't explicitly handle it. Since TraitsPicker doesn't destructure onOptionSelected, it ends up in the persistence spread that gets passed to TraitsMenuContent — potentially overriding the internal setIsMenuOpen close handler depending on prop order. I should check if any consumers are actually passing onOptionSelected to TraitsPicker and whether the persistence object contains unexpected keys. The fix would be to either reorder the spreads or explicitly destructure onOptionSelected in TraitsPicker to prevent it from leaking through. Now looking at the cursor-pointer findings... MenuRadioItem already applies cursor-pointer at the base level, so it's redundant at the call sites. For MenuTrigger, Base UI's render mechanics merge the trigger's className with the rendered element's className by concatenating them, so ComposerControl ends up receiving both the cursor-pointer class and whatever triggerClassName props are passed through. But here's the thing: Button's base styles already include cursor-pointer, and ComposerControl doesn't override it with cursor-default or anything else. So all three places adding cursor-pointer are just duplicating what the primitives already declare — this entire part of the PR is essentially a no-op. I've identified two issues to report: first, there's redundant cursor-pointer classes scattered across MenuRadioItem and Button/ComposerControl/MenuTrigger that duplicate what the primitive already owns, creating maintenance drift. Second, there's a prop-order bug where the {...persistence} spread comes after onOptionSelected, potentially clobbering the close handler since onOptionSelected is part of TraitsMenuContentProps but isn't being destructured properly—I need to check composerProviderState to see if any consumers are actually passing this prop. Main already addressed the close behavior through closeOnClick, but this PR introduces a redundant onOptionSelected callback that duplicates that functionality by imperatively closing the menu from TraitsPicker — which means the menu closes even when it shouldn't in the consumer's own container. The redundancy isn't harmful in practice since closeOnClick would close it anyway, but it creates an unnecessary prop-ordering hazard. I need to check whether the controlled open state for TraitsPicker was also part of this PR or if it existed at the merge base. The cursor-pointer additions are redundant since the base primitives already include them, and the close behavior duplicates what's already handled by closeOnClick. This PR essentially duplicates both the primitive's cursor styling and existing close functionality, which constitutes a concrete ownership violation—call sites shouldn't be re-applying cursor-pointer when the primitive already owns that styling. There's also a real prop-clobbering hazard: spreading persistence after onOptionSelected means the rest type includes onOptionSelected as an optional property, which would override the explicitly set close handler. No consumer currently passes it, so it's a latent bug rather than an active one, but the patch makes this pre-existing pattern relevant and reportable. On keyboard interaction, imperatively closing on selection means arrow+Enter also closes the menu just like closeOnClick does, which is fine. And with MenuTrigger's className merging through Base UI's render prop system, the cursor-pointer class gets concatenated with ComposerControl's className without issues, though the merge happens inside Trigger's useRender before Button receives the combined className. The cursor-pointer duplication across four call sites is actually a consistency violation — the primitive already owns that styling, so repeating it at each call site represents drift from the primitive contract. I'm going to report this as a concrete finding and fail the check. But I should verify whether the PR's intent was to fix a missing cursor. If Button and MenuRadioItem already have cursor-pointer built in, then adding it again would be redundant. Let me check if something's overriding the cursor elsewhere — maybe a global rule in the stylesheet or a Base UI popup applying cursor-default to menu items. I'm verifying the context at line 868. The cursor-pointer additions turned out to be unnecessary. Now I'm moving on to add inline comments to the TraitsPicker component file, targeting specific lines in the diff where I can post feedback on the right side. I'm deciding how to structure the comments — I could combine them or split them across multiple lines. The challenge is that inline suggestions need to stay within the diff boundaries, so I'm considering whether to use a unified diff snippet in the comment rather than trying to create a multi-line suggestion block that might reference non-diff context lines. I'll post a separate comment on line 528 with a suggestion to clean up the conditional styling, and then add another comment on line 522 addressing the MenuTrigger className issue. Then I need to look at line 559 where there's a potential redundancy with the onOptionSelected prop. On line 559, spreading persistence after onOptionSelected creates a prop-clobbering hazard—if a consumer passes onOptionSelected in the persistence object, it silently overrides the close handler. Plus, since closeOnClick already handles menu closure upstream, the extra callback is redundant; I should either destructure onOptionSelected in TraitsPicker or move the spread before it. I'll combine both the redundancy and prop-order issue into one concise comment. I need to verify that Base UI's closeOnClick actually handles all the cases properly—the upstream commit claims it's fixed and it's on main, so the callback would be redundant. But I should check the prompt-injected select where handleSelectChange modifies the prompt state; that still uses closeOnClick, which is consistent. For comment 4, I'll frame it as the callback duplicating closeOnClick functionality, and if they want to keep it, the spread order becomes a potential issue—I'll ask for the simpler fix. Let me double-check the line numbers against the current file: 330 for the className inside MenuRadioItem, 385, 522 for MenuTrigger className, 528-529, and 559 for onOptionSelected all check out. Ready to post the review.

1. modify_pr — ## UI consistency review The pointer-cursor additions in this PR duplicate cursor state that the shared primitives already own, so they are no-ops at the call site: - Button's base buttonVariants already includes cursor-pointer (apps/web/src/components/ui/button.tsx), which ComposerControl renders through — so the cursor-pointer on MenuTrigger and on both ComposerControl class branches adds nothing. - MenuRadioItem's base classes already include cursor-pointer plus data-disabled:cursor-not-allowed (apps/web/src/components/ui/menu.tsx), so the per-item className="cursor-pointer" only re-states the primitive contract. Separately, the menu already closes on pick via closeOnClick on the radio items (added upstream in #5879, present at the merge base), so the new onOptionSelected close path is a second mechanism for the same behavior — and it is passed before {...persistence}, which makes it clobberable. Details inline.

💭 I need to revise that comment on lines 528-529 since the suggestion I left was sloppy. Let me pull up the review comments to find the ID so I can edit it. I need to fix a malformed suggestion in comment 3791096119 using a PATCH request.

2. modify_pr — PATCH repos/{owner}/{repo}/pulls/comments/3791096119