Skip to content

perf(SelectPanel): reset intermediate selection during render, not in an effect#8025

Open
mattcosta7 wants to merge 8 commits into
mainfrom
fix/selectpanel-intermediate-selected-derive
Open

perf(SelectPanel): reset intermediate selection during render, not in an effect#8025
mattcosta7 wants to merge 8 commits into
mainfrom
fix/selectpanel-intermediate-selected-derive

Conversation

@mattcosta7

Copy link
Copy Markdown
Contributor

Overview

SelectPanel reset the single-select modal's intermediate selection from an effect:

useEffect(() => {
  setIntermediateSelected(isSingleSelectModal ? selected : undefined)
}, [isSingleSelectModal, open, selected])

That's the set-state-in-effect / no-derived-state pattern and forces an extra post-commit render whenever the panel opens/closes or the selection changes. This replaces it with the adjust-state-during-render pattern (tracking the previous inputs), which React restarts before committing — so the reset is behavior-identical with no extra commit:

const [prev, setPrev] = useState({open, selected, isSingleSelectModal})
if (prev.open !== open || prev.selected !== selected || prev.isSingleSelectModal !== isSingleSelectModal) {
  setPrev({open, selected, isSingleSelectModal})
  setIntermediateSelected(isSingleSelectModal ? selected : undefined)
}

Removing the eslint-disable also lets the lint rule guard against regressions.

Changelog

Changed

  • SelectPanel: the single-select modal's intermediate selection is reset during render instead of in an effect, avoiding an extra re-render on open/close/selection change. No behavior or API change.

Rollout strategy

  • Patch release
  • Minor release
  • Major release
  • None

Testing & Reviewing

  • Behavior is identical to the previous effect (same {open, selected, isSingleSelectModal} reset triggers).
  • The full SelectPanel suite (152 tests), which covers the single-select-modal intermediate-selection / cancel / confirm flows, passes unchanged.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Storybook)
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

… an effect

Replace the useEffect that reset the single-select modal's intermediate selection with the adjust-state-during-render pattern (tracking previous {open, selected, isSingleSelectModal}). Behavior is identical but avoids the extra post-commit render, and removing the eslint-disable lets the lint rule guard against regressions.
@changeset-bot

changeset-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 50046a3

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

@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 22, 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. If this doesn't work, you can also use the original workflow here. 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.

@mattcosta7 mattcosta7 marked this pull request as ready for review June 23, 2026 12:24
@mattcosta7 mattcosta7 requested a review from a team as a code owner June 23, 2026 12:24
@mattcosta7 mattcosta7 requested review from Copilot and liuliu-dev June 23, 2026 12:24
@mattcosta7 mattcosta7 self-assigned this Jun 23, 2026
@github-actions github-actions Bot temporarily deployed to storybook-preview-8025 June 23, 2026 12:24 Inactive

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 updates SelectPanel’s single-select modal behavior to reset the intermediate selection during render (tracking prior inputs) instead of syncing it from a useEffect, aiming to avoid an extra post-commit render and removing the related ESLint disable.

Changes:

  • Replace the useEffect-based intermediate-selection reset with an adjust-state-during-render approach keyed on {open, selected, isSingleSelectModal} changes.
  • Add a patch changeset describing the performance-oriented internal behavior adjustment.
Show a summary per file
File Description
packages/react/src/SelectPanel/SelectPanel.tsx Moves intermediate selection reset logic from an effect to render-time adjustment based on previous inputs.
.changeset/selectpanel-intermediate-selected-derive.md Adds a patch changeset documenting the internal behavior/perf change.

Copilot's findings

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

Comment thread packages/react/src/SelectPanel/SelectPanel.tsx
@github-actions github-actions Bot temporarily deployed to storybook-preview-8025 July 7, 2026 15:27 Inactive
@mattcosta7 mattcosta7 added the Canary Release Apply this label when you want CI to create a canary release of the current PR label Jul 7, 2026

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.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread packages/react/src/SelectPanel/SelectPanel.tsx
…nputs change

Decouples the reset-key advance from the value-difference check so it can't go
stale and clobber a user-driven selection on the next render (single-select
modal). Adds a regression test.
mattcosta7 added a commit that referenced this pull request Jul 7, 2026
Drop the overlapping ToggleSwitch/SelectPanel/ValidationAnimationContainer
changes (handled by #8024/#8025/#8023) and the comment-only edits to
Autocomplete/Dialog/LabelGroup/TreeView/SelectPanel2. Retarget the changeset
to the useMedia change only.
@primer-integration

primer-integration Bot commented Jul 7, 2026

Copy link
Copy Markdown

Integration test results from github/github-ui PR:

Passed  CI   Passed
Running  VRT   Running
Waiting  Projects   Waiting

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.

Review details

  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Low

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.

3 participants