Skip to content

fix: scroll the selected item into view by value, not stale aria-selected (#389)#409

Open
thatssoheil wants to merge 1 commit into
dip:mainfrom
thatssoheil:fix/scroll-selected-by-value
Open

fix: scroll the selected item into view by value, not stale aria-selected (#389)#409
thatssoheil wants to merge 1 commit into
dip:mainfrom
thatssoheil:fix/scroll-selected-by-value

Conversation

@thatssoheil

Copy link
Copy Markdown

Problem

Changing the search in <Command> can scroll the list to the previously-selected item instead of the new first result (#389). It's most visible when a search broadens and the top result changes.

Root cause

scrollSelectedIntoView finds its target via [aria-selected="true"]. cmdk's scheduler (useScheduleLayoutEffect) runs all scheduled callbacks in a single layout-effect pass, so on a search change the scroll fires in the same pass as selectFirstItembefore React commits the re-render that moves aria-selected onto the newly-selected item. The scroll therefore reads the stale attribute and scrolls to the old selection.

The element for the new selection is already in the DOM (it matched the search); only its aria-selected lags by one commit. Its data-value is already correct.

Fix

Resolve the scroll target from state.current.value (the source of truth) by matching data-value, rather than the lagging aria-selected. Scoped to the scroll path, so getSelectedItem() and its other callers are untouched. Comparing data-value in JS (instead of building a selector) also sidesteps escaping arbitrary values into querySelector (cf. #387).

Test

Adds a Playwright regression (test/scroll.test.ts + test/pages/scroll.tsx): narrow the search to an item far down the list, broaden it, and assert the list scrolls back to the new top selection. Fails on main (scrollTop parks at the stale item), passes with this change.

Notes

Fixes #389


Implemented with AI assistance (Claude), reviewed and verified by me before submitting.

…cted (dip#389)

scrollSelectedIntoView resolved its target via `[aria-selected="true"]`, which
lags one React commit behind a value change. cmdk's scheduler runs all scheduled
callbacks in a single layout-effect pass, so on a search change the scroll fires
in the same pass as selectFirstItem, before the re-render that moves
aria-selected onto the new item commits. The list therefore scrolled to the
previously-selected item.

Resolve the scroll target from state.current.value (the source of truth) by
matching data-value instead. Scoped to the scroll path, so getSelectedItem() and
its other callers are unchanged. Comparing data-value in JS also avoids escaping
arbitrary values into a selector (dip#387).

Adds a Playwright regression test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 23, 2026 23:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

[Bug] The List scrolls to a stale top item when search changes

2 participants