Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 2, 2026

The useTypeahead hook used startTransition with a mutable ref, creating a race condition where the ref value could change between capture and execution during rapid typing.

// Before: ref read at execution time, not capture time
searchValue.current += event.key
startTransition(() => {
  focusSearchValue(searchValue.current)  // ⚠️ May have changed
})

Refactored to use useDeferredValue pattern matching Autocomplete component:

  • Converted searchValue from useRef to useState
  • Applied useDeferredValue to defer expensive focus operations
  • Moved focus logic from callback to useEffect dependent on deferred value
  • Preserved immediate state updates in keydown handler for responsive typing
// After: stable value captured per render
const [searchValue, setSearchValue] = useState('')
const deferredSearchValue = useDeferredValue(searchValue)

useEffect(() => {
  // Focus logic runs when deferred value changes
}, [deferredSearchValue, ...])

Changelog

Changed

  • useTypeahead now uses useDeferredValue instead of startTransition
  • searchValue is React state instead of mutable ref

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Internal implementation change with identical external behavior.

Testing & Reviewing

All existing TreeView typeahead tests pass. Behavior is identical to users—focus matching on keypress works the same, just without the race condition.

Merge checklist

Original prompt

Problem with Current Implementation

The current PR uses startTransition with a mutable ref, which has a bug:

searchValue.current += event.key
startTransition(() => {
  focusSearchValue(searchValue.current)  // ⚠️ Reads ref at execution time, not capture time
})

This can cause issues when typing quickly - the ref value may have changed by the time the transition executes.

Solution: Use useDeferredValue Pattern

Refactor useTypeahead.ts to use the same pattern as Autocomplete component, which properly handles deferred input filtering:

Changes to packages/react/src/TreeView/useTypeahead.ts:

  1. Convert from mutable ref to React state for searchValue
  2. Use useDeferredValue to defer the expensive focus search operation
  3. Move focus logic to a useEffect that depends on the deferred value
  4. Keep immediate state updates for responsive typing

Target Implementation:

import React, {useDeferredValue, useState, useEffect, useCallback} from 'react'
import useSafeTimeout from '../hooks/useSafeTimeout'
import {getAccessibleName} from './shared'
import {useTreeItemCache} from './useTreeItemCache'

type TypeaheadOptions = {
  containerRef: React.RefObject<HTMLElement>
  onFocusChange: (element: Element) => void
}

export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) {
  // Use state instead of ref so React can track changes
  const [searchValue, setSearchValue] = useState('')
  // Defer the search value for expensive operations
  const deferredSearchValue = useDeferredValue(searchValue)
  
  const timeoutRef = React.useRef(0)
  const onFocusChangeRef = React.useRef(onFocusChange)
  const {safeSetTimeout, safeClearTimeout} = useSafeTimeout()
  const {getTreeItems} = useTreeItemCache(containerRef)

  // Update the ref when the callback changes
  useEffect(() => {
    onFocusChangeRef.current = onFocusChange
  }, [onFocusChange])

  // Focus logic runs when deferred value changes
  useEffect(() => {
    if (!deferredSearchValue || !containerRef.current) return

    const elements = getTreeItems()
    const activeIndex = elements.findIndex(element => element === document.activeElement)
    let sortedElements = wrapArray(elements, activeIndex)

    // Remove the active descendant from the beginning when starting a new search
    if (deferredSearchValue.length === 1) {
      sortedElements = sortedElements.slice(1)
    }

    const nextElement = sortedElements.find(element => {
      const name = getAccessibleName(element).toLowerCase()
      return name.startsWith(deferredSearchValue.toLowerCase())
    })

    if (nextElement) {
      onFocusChangeRef.current(nextElement)
    }
  }, [deferredSearchValue, containerRef, getTreeItems])

  // Keydown handler updates state immediately for responsive UI
  useEffect(() => {
    if (!containerRef.current) return
    const container = containerRef.current

    function onKeyDown(event: KeyboardEvent) {
      // Ignore key presses that don't produce a character value
      if (!event.key || event.key.length > 1 || event.key === ' ') return

      // Ignore key presses that occur with a modifier
      if (event.ctrlKey || event.altKey || event.metaKey) return

      // Update state immediately - React will defer the expensive focus operation
      setSearchValue(prev => prev + event.key)

      // Reset the timeout
      safeClearTimeout(timeoutRef.current)
      timeoutRef.current = safeSetTimeout(() => setSearchValue(''), 300)

      // Prevent default behavior
      event.preventDefault()
      event.stopPropagation()
    }

    container.addEventListener('keydown', onKeyDown)
    return () => container.removeEventListener('keydown', onKeyDown)
  }, [containerRef, safeClearTimeout, safeSetTimeout])
}

/**
 * Wraps an array around itself at a given start index
 */
function wrapArray<T>(array: T[], startIndex: number) {
  return array.map((_, index) => array[(startIndex + index) % array.length])
}

Why This Is Better

Aspect Current (startTransition + ref) New (useDeferredValue + state)
Value consistency ⚠️ Ref can change before execution ✅ Each render has stable value
Pattern consistency ❌ Different from Autocomplete ✅ Matches Autocomplete pattern
React integration Manual transition scheduling React manages priority automatically
Debugging Harder - mutable state Easier - immutable state flow

Testing

  • All existing typeahead tests should continue to pass
  • The behavior should be identical from user perspective
  • Performance improvement should be maintained (expensive search is still deferred)

Files to Modify

  • packages/react/src/TreeView/useTypeahead.ts - Main refactor

This pull request was created from Copilot chat.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@changeset-bot
Copy link

changeset-bot bot commented Jan 2, 2026

⚠️ No Changeset found

Latest commit: 1386461

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

- Convert searchValue from mutable ref to React state
- Use useDeferredValue to defer expensive focus search operation
- Move focus logic from callback to useEffect that depends on deferred value
- Keep immediate state updates in keydown handler for responsive typing
- Remove unused startTransition import and focusSearchValue callback
- All 51 TreeView tests pass including 6 typeahead tests

Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor useTypeahead to use useDeferredValue pattern Refactor useTypeahead to use useDeferredValue instead of startTransition with mutable ref Jan 2, 2026
Copilot AI requested a review from mattcosta7 January 2, 2026 22:58
@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jan 2, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2026

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the integration-tests: skipped manually label to skip these checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants