Repository navigation
Conversation
The arrows read SelectPositionerContext, whose value changes with every position update, and subscribed to openMethod. Give them a small context of what they use and subscribe to the computed visibility.
Birzool
requested review from
atomiks,
colmtuite,
flaviendelangle,
jjenzz and
michaldudak
as code owners
November 6, 2026 14:21
commit: |
Bundle size
PerformanceNo regressions in 36 benchmarks · ran 3m 25s · details Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
A mouse press opens the popup in one frame, and the popup reads its scroller in the next. React 18 commits the open only as act exits, so one act never reached that second frame: the mouse test failed, and the touch tests passed without the arrows' visibility being computed. Give the second frame its own act.
This branch has not been deployed
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.
Select.ScrollUpArrowandSelect.ScrollDownArrowre-render on things they don't draw:SelectPositionerContext, whose value changes with every position update, but they only usesideand two refs that never change.openMethod, which changes on every open and close, but they only need to know whether it istouchwhile the arrow is visible.When the list doesn't overflow, the arrows stay hidden, so these renders can't change anything.
Reproduction
The new tests in
SelectScrollArrow.test.tsxrender a Select once, withkeepMountedarrows and a countingrender, then open and close it. They setalignItemWithTrigger={false}, soside, the one position value the arrow draws, stays the same.On
master, opening alone takes each arrow from 4 renders to the counts below, with Strict Mode rendering twice. With this PR the count stays at 4 through the open and the close. These are React 19 numbers; on React 18 the baseline is 6 and it stays at 6.masterChanges
SelectScrollArrowContextholdssideand the two refs. Its value only changes withside. The refs move there, because the arrows are their only readers.scrollUpArrowShownandscrollDownArrowShownselectors compute the visibility, so the arrow re-renders when the result changes, and not on eachopenMethodchange.There is no API change. The cost is one more provider per
Select.Positioner.Evidence
The counts below come from a production build of an app on Base UI 1.8.0, with an equivalent edit applied to the published files, not from this branch. A script ran the same interactions on each build. The counter takes the re-renders of the arrows that changed no DOM or landed in a hidden node:
SelectScrollArrowre-rendersSome of these builds also carried an unrelated Base UI edit. I only quote the component's own row, because whole-run totals vary too much between builds to show a change this small.
Tests
master. They also fail when the arrow subscribes toopenMethodagain, or when its context value changes with the position.mastertoo, as this behavior doesn't change, and fails when either selector drops its touch check.src/selectpassed in JSDOM before the last commit, which changes only this test file; that file passes in JSDOM on React 18 and 19.src/selectpassed once and failed twice on this commit. Each failure was onlydismisses the popover with one outside press after reopening an aligned select, on anact()warning fromPopoverPositioner. That test isn't changed here and fails the same way without the patch's sources (1 of 2 runs). An earlier Chromium run of the suite on Linux, before the tests were added, passed.prettier --checkandeslint --max-warnings 0pass on the changed files. For types I rantsc -p packages/react/tsconfig.json, notpnpm typescript, which needs more memory than my 2 GB job limit.