Skip to content

perf: derive Autocomplete re-sort and LabelGroup truncation during render - #8005

Closed
mattcosta7 with Copilot wants to merge 26 commits into
mainfrom
copilot/copilotderive-state-from-effects
Closed

perf: derive Autocomplete re-sort and LabelGroup truncation during render#8005
mattcosta7 with Copilot wants to merge 26 commits into
mainfrom
copilot/copilotderive-state-from-effects

Conversation

Copilot AI commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Standalone performance refactor (originally split out of #8001). It narrows two mixed-responsibility effects so derivable state updates happen during render, while genuine side effects stay in effects. Behavior-preserving: no API or visual changes.

Changelog

New

  • None.

Changed

  • AutocompleteMenu

    • Close-time re-sorting is now derived during render instead of running inside the onOpenChange effect.
    • The comparator runs only while the menu is closed (showMenu === false), so it never runs on the frequent renders that happen while the menu is open. AutocompleteOverlay remounts this component on each open/close, so a render-time previous-value transition can't be observed here — guarding on the closed state is what replaces the old close-time effect.
    • onOpenChange remains in useEffect as the actual side effect.
  • LabelGroup

    • Numeric visibleChildCount truncation now derives visibilityMap from child indices during render instead of reading committed DOM state in an effect.
    • 'auto' truncation remains effect-driven via IntersectionObserver.
    • Added a shallow-equality guard and a memoized derived map to avoid redundant render-phase updates.
  • Tests

    • Tightened Autocomplete coverage to assert onOpenChange fires correctly across open/close transitions.
    • Added LabelGroup rerender coverage for numeric truncation updates.

Close-time sort, derived during render while the menu is closed:

if (showMenu === false) {
  const itemIdSortResult = [...sortedItemIds].sort(/* ... */)
  const sortResultMatchesState =
    itemIdSortResult.length === sortedItemIds.length &&
    itemIdSortResult.every((element, index) => element === sortedItemIds[index])

  if (!sortResultMatchesState) {
    setSortedItemIds(itemIdSortResult)
  }
}

Removed

  • The redundant effect-driven numeric-truncation update path in LabelGroup.
  • The now-unneeded set-state-in-effect / derived-state lint suppressions around Autocomplete close-time sorting.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; behavior-preserving internal refactor only, with no API or visual change.

Testing & Reviewing

Review by change site:

  • Autocomplete — confirm close-time sorting still happens only while the menu is closed, and that onOpenChange still fires with the current open state and remains effect-driven.
  • LabelGroup — confirm numeric visibleChildCount truncation updates correctly across rerenders, and that 'auto' truncation, inline expand/collapse, and overlay overflow behavior are unchanged.

Validated with:

  • npx vitest run Autocomplete/Autocomplete.test LabelGroup/LabelGroup.test (40 passing)
  • npx eslint packages/react/src/Autocomplete/AutocompleteMenu.tsx packages/react/src/LabelGroup/LabelGroup.tsx packages/react/src/Autocomplete/Autocomplete.test.tsx packages/react/src/LabelGroup/LabelGroup.test.tsx

Merge checklist

mattcosta7 and others added 4 commits June 16, 2026 14:01
Rewrite four effects flagged by react-hooks/set-state-in-effect to derive
state during render or via useSyncExternalStore:

- useMedia: use useSyncExternalStore (tear-free, no redundant initial render)
- ToggleSwitch: hide loading label via render-time derivation; keep only the
  delayed reveal in an effect
- SelectPanel: reset intermediateSelected by tracking previous open state
- ValidationAnimationContainer: mount via render-time derivation

Also make the useMedia matchMedia mock reflect real MediaQueryList behavior
(matches updates on change).
Account for every remaining react-hooks/set-state-in-effect suppression in
component/hook source. These are necessary effects (committed-DOM reads or
consumer side effects), so add an inline rationale to each rather than a bare
disable:

- TreeView: getAccessibleName reads committed DOM
- LabelGroup: hideChildrenAfterIndex reads DOM children (numeric branch is a
  possible follow-up derivation)
- AutocompleteMenu: sort-on-close shares the effect with onOpenChange
- Dialog: commit-timing focus hack
- SelectPanel2: reads label textContent from DOM
- SelectPanel: no-items announce (commit timing) and first-open fetch (callback)

Demo/test suppressions (stories, storybook data) are left for separate cleanup.
Resolve conflicts from Add eslint-plugin-react-you-might-not-need-an-effect (#8000):
- Keep useSyncExternalStore implementation in useMedia.ts (our refactor)
- Keep render-time derivation in ValidationAnimationContainer.tsx (our refactor)
- Keep render-time derivation in SelectPanel.tsx intermediateSelected (our refactor)
- Keep render-time derivation in ToggleSwitch.tsx; add no-event-handler suppression
- Merge descriptive comments with new react-you-might-not-need-an-effect suppressions
  in Dialog.tsx, AutocompleteMenu.tsx, SelectPanel.tsx, and TreeView.tsx

Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
@changeset-bot

changeset-bot Bot commented Jun 17, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 4cecd51

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

This PR includes changesets to release 1 package
Name Type
@primer/react 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

Copilot AI and others added 2 commits June 17, 2026 12:43
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor flagged suppressions to derive state during render Follow up #8001 by deriving Autocomplete and LabelGroup state during render Jun 17, 2026
Copilot AI requested a review from mattcosta7 June 17, 2026 12:52
@mattcosta7
mattcosta7 marked this pull request as ready for review June 17, 2026 15:24
Copilot AI review requested due to automatic review settings June 17, 2026 15:24
@mattcosta7
mattcosta7 requested a review from a team as a code owner June 17, 2026 15:24
@mattcosta7
mattcosta7 requested a review from llastflowers June 17, 2026 15:24
@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 Jun 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Action required

👋 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. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR is a follow-up refactor to #8001 that narrows two previously “splittable later” effect sites by deriving state during render for derivable transitions (while keeping real side effects in effects). The focus is Autocomplete close-time reordering and LabelGroup numeric truncation.

Changes:

  • AutocompleteMenu: Track showMenu transitions during render and trigger close-time re-sorting on the open→closed transition; keep onOpenChange in an effect.
  • LabelGroup: For numeric visibleChildCount, derive the truncation visibilityMap from child indices (not committed DOM) and add a shallow equality guard; keep 'auto' truncation effect-driven via IntersectionObserver.
  • Tests: Strengthen coverage for onOpenChange(false → true → false) and add rerender coverage for numeric LabelGroup truncation updates.
Show a summary per file
File Description
packages/react/src/LabelGroup/LabelGroup.tsx Derives numeric truncation visibility from child indices and updates render path to use a cached childArray.
packages/react/src/LabelGroup/LabelGroup.test.tsx Adds rerender assertions to ensure numeric truncation updates correctly when visibleChildCount changes.
packages/react/src/Autocomplete/AutocompleteMenu.tsx Moves close-time sorting to a render-time transition check while keeping onOpenChange effect-driven.
packages/react/src/Autocomplete/Autocomplete.test.tsx Tightens onOpenChange assertions to validate open/close sequencing.

Copilot's findings

  • Files reviewed: 4/4 changed files
  • Comments generated: 2

Comment thread packages/react/src/Autocomplete/AutocompleteMenu.tsx Outdated
Comment thread packages/react/src/LabelGroup/LabelGroup.tsx
mattcosta7 and others added 3 commits June 23, 2026 07:59
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actions
github-actions Bot temporarily deployed to storybook-preview-8005 June 23, 2026 14:51 Inactive
@github-actions
github-actions Bot temporarily deployed to storybook-preview-8005 June 23, 2026 16:05 Inactive
@github-actions
github-actions Bot requested a deployment to storybook-preview-8005 July 7, 2026 19:34 Abandoned
@github-actions
github-actions Bot temporarily deployed to storybook-preview-8005 July 7, 2026 19:45 Inactive
@mattcosta7
mattcosta7 changed the base branch from copilot/derive-state-from-effects to main July 7, 2026 21:38
Retargets base to main and drops the old #8001 leftovers (useMedia, SelectPanel,
ToggleSwitch, ValidationAnimationContainer, Dialog, TreeView, SelectPanel2),
which are handled by their own focused PRs. Keeps only the Autocomplete close-time
re-sort and LabelGroup numeric-truncation render derivations.
@mattcosta7 mattcosta7 changed the title Follow up #8001 by deriving Autocomplete and LabelGroup state during render perf: derive Autocomplete re-sort and LabelGroup truncation during render Jul 7, 2026

Copy link
Copy Markdown
Contributor

Rescoped this to a standalone PR: retargeted the base from copilot/derive-state-from-effects to main and dropped the old #8001 leftovers (useMedia, SelectPanel, ToggleSwitch, ValidationAnimationContainer, Dialog, TreeView, SelectPanel2), which are handled by their own focused PRs. It now contains only the Autocomplete close-time re-sort and LabelGroup numeric-truncation render derivations (+ tests). This also clears the merge conflict that appeared when #8001 was rescoped.

@github-actions
github-actions Bot temporarily deployed to storybook-preview-8005 July 15, 2026 13:34 Inactive
@mattcosta7 mattcosta7 closed this Jul 24, 2026
@primer
primer Bot deleted the copilot/copilotderive-state-from-effects branch July 25, 2026 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Canary Release Apply this label when you want CI to create a canary release of the current PR 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.

4 participants