Skip to content

fix(metrics): Prevent metric selector hover from stealing search input focus#111292

Merged
nsdeschenes merged 13 commits intomasterfrom
nd/LOGS-627/fix-tracemetrics-keep-input-focus-in-metric-selector
Apr 13, 2026
Merged

fix(metrics): Prevent metric selector hover from stealing search input focus#111292
nsdeschenes merged 13 commits intomasterfrom
nd/LOGS-627/fix-tracemetrics-keep-input-focus-in-metric-selector

Conversation

@nsdeschenes
Copy link
Copy Markdown
Contributor

@nsdeschenes nsdeschenes commented Mar 23, 2026

This PR refactors the metrics selector from a list state setup over to a combobox state setup. This resolves the issue with the input losing focus, as well as bringing the selector more in line with what we're trying to achieve with it.

@linear-code
Copy link
Copy Markdown

linear-code bot commented Mar 23, 2026

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 23, 2026
@nsdeschenes
Copy link
Copy Markdown
Contributor Author

@sentry review

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

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 and found 2 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Two items can show focus styling simultaneously
    • Replaced isFocused || isHovered with listState.selectionManager.isFocused && isFocused to properly coordinate focus styling using the list's focus state as a guard.
  • ✅ Fixed: Stale focus persists after mouse leaves hovered item
    • Removed redundant isHovered state and onMouseLeave handler; the proper focus guard now ensures focus styling is cleared when the list loses focus.

Create PR

Or push these changes by commenting:

@cursor push f8f85efef0
Preview (f8f85efef0)
diff --git a/static/app/views/explore/metrics/metricToolbar/metricSelector.tsx b/static/app/views/explore/metrics/metricToolbar/metricSelector.tsx
--- a/static/app/views/explore/metrics/metricToolbar/metricSelector.tsx
+++ b/static/app/views/explore/metrics/metricToolbar/metricSelector.tsx
@@ -597,7 +597,6 @@
 }: MetricListBoxOptionProps) {
   const ref = useRef<HTMLLIElement>(null);
   const option = item.value!;
-  const [isHovered, setIsHovered] = useState(false);
   const {optionProps, isFocused, isSelected, isDisabled, isPressed} = useOption(
     {key: item.key, 'aria-label': option.label},
     listState,
@@ -609,9 +608,7 @@
       // setFocused(true) triggers a useEffect in useSelectableItem that calls
       // focusSafely(ref.current), which would steal DOM focus from the search input.
       listState.selectionManager.setFocusedKey(item.key);
-      setIsHovered(true);
     },
-    onMouseLeave: () => setIsHovered(false),
   });
 
   return (
@@ -622,7 +619,7 @@
       ref={mergeRefs(ref, measureRef)}
       size={size}
       label={option.label}
-      isFocused={isFocused || isHovered}
+      isFocused={listState.selectionManager.isFocused && isFocused}
       isSelected={isSelected}
       isPressed={isPressed}
       disabled={isDisabled}

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Comment thread static/app/views/explore/metrics/metricToolbar/metricSelector.tsx Outdated
Comment thread static/app/views/explore/metrics/metricToolbar/metricSelector.tsx Outdated
@nsdeschenes nsdeschenes force-pushed the nd/LOGS-627/fix-tracemetrics-keep-input-focus-in-metric-selector branch from 9b9dc89 to 31809c1 Compare March 23, 2026 19:08
Comment thread static/app/views/explore/metrics/metricToolbar/metricSelector.tsx Outdated
Comment thread static/app/views/explore/metrics/metricToolbar/metricSelector.tsx
@nsdeschenes nsdeschenes marked this pull request as ready for review March 24, 2026 12:49
@nsdeschenes nsdeschenes requested a review from a team as a code owner March 24, 2026 12:49
Comment thread static/app/views/explore/metrics/metricToolbar/metricSelector.tsx
@nsdeschenes nsdeschenes marked this pull request as draft March 24, 2026 16:25
@nsdeschenes nsdeschenes force-pushed the nd/LOGS-627/fix-tracemetrics-keep-input-focus-in-metric-selector branch from 9ba9f2e to 7bc63ea Compare March 24, 2026 19:21
@nsdeschenes nsdeschenes marked this pull request as ready for review March 24, 2026 19:27
Copy link
Copy Markdown
Member

@k-fish k-fish left a comment

Choose a reason for hiding this comment

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

Do we need to hand roll input logic? Doesn't react aria combobox do this?

Comment thread static/app/views/explore/metrics/metricToolbar/metricSelector.tsx Outdated
nsdeschenes and others added 10 commits March 31, 2026 07:33
…t focus

When hovering over options in the metric selector listbox,
setFocused(true) was triggering a useEffect in useSelectableItem that
called focusSafely(ref.current), stealing DOM focus from the search
input. Replace setFocused(true) with only setFocusedKey for hover, and
track hover state separately so options still get visual focus styling
without moving actual DOM focus.

Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Made-with: Cursor
…lector

Switch to shouldUseVirtualFocus so DOM focus stays on the search input
while browsing the metric list. Replace the old useKeyboard-based search
handler with a plain onKeyDown callback that drives the selectionManager
directly for ArrowDown, ArrowUp, Home, End, and Enter keys. This keeps
the search input editable while allowing full keyboard navigation and
item selection.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Made-with: Cursor
Clear the focused key on close so re-opening the dropdown doesn't
show a stale highlight. The selected item remains visually indicated
via its selection state instead.

Co-Authored-By: Claude Opus <noreply@anthropic.com>
Made-with: Cursor
The metric selector now uses virtual focus for keyboard navigation,
keeping DOM focus on the search input while ArrowDown moves virtual
focus to list options. Update the test assertion accordingly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…h input

Home and End keys were intercepted and prevented from propagating to the
search input, breaking cursor navigation to the start/end of typed text.
Since the input always retains DOM focus via virtual focus, these keys
should be passed through to the input element unchanged.

Co-Authored-By: Claude Sonnet 4 <noreply@example.com>
Replace useListState + useListBox + manual keyboard handling with the
standard useComboBox + useComboBoxState from react-aria. This removes ~40
lines of hand-rolled ArrowDown/ArrowUp/Enter bridging logic and aligns
with the combobox pattern used elsewhere in the codebase.

Key changes:
- useComboBoxState manages collection, selection, and open/close state
- useComboBox provides input props with aria-activedescendant, keyboard
  navigation, and accessibility announcements
- useOverlay and useComboBoxState sync open/close via a shared
  handleOpenChange with a re-entrancy guard
- Search input focuses via a ref callback on mount instead of a useEffect
- Virtual focus (shouldUseVirtualFocus) passed directly to useOption since
  useListBox no longer sets it up via listData

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove wrapperRef by using triggerRef and popoverRef from useOverlay for
focus management on close. Replace searchRefCallback useMemo with
autoFocus on the input. Merge isSyncingOpenStateRef and lastSelectionRef
into a single stateGuardRef object.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…with combobox state

Remove the stateGuard ref and re-entrant open/close synchronization
between useOverlay and useComboBoxState. Instead, pass comboBoxState's
isOpen directly to useOverlay and let each hook manage its own state
with a simple bridging onOpenChange. Filter duplicate onSelectionChange
fires by checking comboBoxState.isOpen.

Fixes LOGS-627
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…vert

Replace the onClick-based close with toggle() in onSelectionChange to avoid
the combobox's commitValue flow re-firing onSelectionChange with the
previous key. Remove the now-unnecessary onSelect prop from
MetricListBoxOption.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Scroll the listbox back to the top when the overlay opens so users
always see the beginning of the metric list.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@nsdeschenes nsdeschenes force-pushed the nd/LOGS-627/fix-tracemetrics-keep-input-focus-in-metric-selector branch from 87520e8 to 77e8355 Compare March 31, 2026 10:37
Comment thread static/app/views/explore/metrics/metricToolbar/metricSelector.tsx
… filter

- Replace scrollTo({top: 0}) with scrollTop = 0 for JSDOM compatibility
- Add defaultFilter: () => true to prevent double-filtering since search
  is already handled server-side via useMetricOptions
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

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 and found 1 potential issue.

Fix All in Cursor

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

Comment thread static/app/views/explore/metrics/metricToolbar/metricSelector.tsx Outdated
…option

When the metric selector overlay is dismissed by clicking outside, the
combobox close() method commits the currently highlighted key, spuriously
firing onSelectionChange. Use toggle() instead so the dismissal does not
alter the selection.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cemetrics-keep-input-focus-in-metric-selector

# Conflicts:
#	static/app/views/explore/metrics/metricToolbar/metricSelector.spec.tsx
@nsdeschenes nsdeschenes merged commit db8e93c into master Apr 13, 2026
65 checks passed
@nsdeschenes nsdeschenes deleted the nd/LOGS-627/fix-tracemetrics-keep-input-focus-in-metric-selector branch April 13, 2026 10:17
wedamija pushed a commit that referenced this pull request Apr 13, 2026
…t focus (#111292)

This PR refactors the metrics selector from a list state setup over to a
combobox state setup. This resolves the issue with the input losing
focus, as well as bringing the selector more in line with what we're
trying to achieve with it.

---------

Co-authored-by: Claude Opus 4 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4 <noreply@example.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants