Skip to content

fix(Select): match search against rendered DOM text - #1154

Open
ariser wants to merge 9 commits into
mainfrom
fix/internal-select-search
Open

fix(Select): match search against rendered DOM text#1154
ariser wants to merge 9 commits into
mainfrom
fix/internal-select-search

Conversation

@ariser

@ariser ariser commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

What

Select search now matches the text the browser actually renders (read from the DOM), instead of a static walk of the React tree. Any item whose visible text isn't a plain string child was invisible to that walk, so matching rows got hidden. Now search works for any item.

Bugs fixed

  • Search missed non-string item text. Items whose visible text comes from a child component, the label prop, or a number were unsearchable and dropped from results. (Also now matches an item's description)
  • Disabled options were keyboard-navigable (options API) — arrow keys could land on a disabled row.

How

  • Searchable text is captured from the rendered DOM (textContent) into state, rather than statically extracted from the React tree.
  • Filtered items render hidden (not unmounted), so their text stays readable and the capture always sees the full set.
  • Visibility, navigation, and the active highlight are now derived values — the old effect + imperative refs (updateList / onUpdateSearch / isInitialized / visibleList / navigatable) are gone.

Notes

  • Contract change: filtered items now stay mounted (hidden, and removed from the a11y tree) instead of unmounting. Only matters for very large, non-virtualized lists.
  • Behavior change: the highlight used to reset to the first item on every open; it now keeps the last highlighted item if it's still navigable.
  • AutoComplete shares the same search pattern and the same latent bug; it'll be ported to this approach separately.

Testing

New Select.search.test.tsx covers the custom-component / label / numeric / description / group-heading / async / keyboard cases; the Select-family search tests assert visibility (toBeVisible), so a wrongly-hidden option fails the suite. 82 tests green.

🤖 Generated with Claude Code

@changeset-bot

changeset-bot Bot commented Jul 27, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 64078fb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment thread src/components/Select/common/InternalSelect.tsx Outdated
@ariser
ariser force-pushed the fix/internal-select-search branch from 639286a to 56c9741 Compare July 27, 2026 23:17
@ariser ariser changed the title fix(Select): match search against rendered DOM text (UC-1466) fix(Select): match search against rendered DOM text Jul 27, 2026

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 56c9741. Configure here.

Comment thread src/components/Select/common/InternalSelect.tsx Outdated
@ariser
ariser marked this pull request as draft July 27, 2026 23:43
Search matched a statically-extracted approximation of each item's text
(getTextFromNodes walking the React tree), so any item whose visible text
wasn't a plain string child was invisible to search and got filtered out:
text rendered by a child component, the `label` prop, and numeric children.
Now search matches the text the browser actually rendered, read from the
DOM — like a native <select>.

Also fixes two issues uncovered along the way:
- Disabled options (options API) were keyboard-navigable: the initial
  navigable set was built from the component's `disabled` prop instead of
  the per-item `disabled`, so arrow keys could land on a disabled row.
- The trigger rendered empty on first paint / SSR because the selected
  value and placeholder were gated behind an `isInitialized` flag flipped
  only in a post-mount effect.

Technical shape: split the old `list` (useState + useEffect) into
`normalizedOptions` (memo; value + disabled; feeds the hidden native
<select> and the trigger's valueNode) and a `searchSource` state map read
from the DOM by a callback ref on the list container. The ref runs once the
list node commits (robust to Radix mounting the popover a commit later) and
re-attaches when the option set changes, so async-loaded options stay
searchable. Visibility, navigability, and the effective highlight are
derived from search + normalizedOptions + searchSource, removing
onUpdateSearch, the rebuild effect, onOpenAutoFocus, and the isInitialized/
visibleList/navigatable state. Group heading text is captured too, via
cui-select-group markers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ariser
ariser force-pushed the fix/internal-select-search branch from 56c9741 to 9228141 Compare July 28, 2026 00:12
@ariser
ariser marked this pull request as ready for review July 28, 2026 00:13
@ariser
ariser requested a review from XOP July 28, 2026 00:17
ariser and others added 4 commits July 28, 2026 02:25
Rename the search derivations to say what they hold — matchedOptions
(option objects), visibleValues / navigableValues (value strings) — inline
the two single-use helpers, and drop comments that restated the type or a
path not taken. Add a test pinning that search matches an option's
description text.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Filtered items now render hidden (via the `hidden` attribute) instead of
unmounting, so every option's text stays in the DOM for the search-source
capture. This makes options that arrive while the menu is open — e.g. async
loads, or fetch-as-you-type — searchable without reopening, and lets the
capture drop its carry-over since it always reads the complete set.

Group visibility now counts non-hidden items. Select-family search tests
assert visibility (toBeVisible / not.toBeVisible) rather than DOM presence,
since a filtered row is now present-but-hidden.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ariser
ariser requested a review from vineethasok July 28, 2026 02:23
InternalSelect no longer builds a static item model, so the shared
SelectItemObject type (and its re-export) is dead — AutoComplete keeps
its own local copy.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@workflow-authentication-public

Copy link
Copy Markdown
Contributor

Storybook Preview Deployed

✅ Preview URL: https://click-l709v3pin-clickhouse.vercel.app

Built from commit: 7c9031650a7b27e218362e6c2b5ac1bc99a8c67d

if ('options' in option) {
return (option.options ?? []).map(item => {
valueNode.current.set(item.value, item);
return { value: item.value, disabled: item.disabled };

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Should be registerValueNode as well

setSearchSource(next);
},
[highlighted, list]
// eslint-disable-next-line react-hooks/exhaustive-deps -- depend on normalizedOptions to re-read DOM if options change

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Think again

}
const searchLowerCase = search.toLowerCase();
return normalizedOptions.filter(item =>
(searchSource.get(item.value) ?? '').includes(searchLowerCase)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Might want to split the search by whitespace and parts.some(p => source includes(p)) for a better search ux

return null;
}
// Filtered items stay mounted (just hidden) so their text remains readable
// from the DOM for search; see the searchSource capture in InternalSelect.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Cleanup comments 🫠

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.

1 participant