Repository navigation
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: f97853f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
Bot
added
the
integration-tests: recommended
|
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
Capture test state setters from effects instead of mutating outer bindings during render. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
integration-tests: failing
|
Hello Primer 👋🏻 This PR is a part of our performance improvement epic and we would appreciate your review 🙏🏻 I've tested each Storybook scenario and haven't identified any unexpected behaviour. I also asked 🤖 to profile main against this PR's branch; the results are below. We aren't expecting noticeable performance gains from this PR alone. However, we expect the cumulative reduction in unnecessary rendering across all the related sub-issues to have a positive impact on overall performance. Rendering comparison — before vs. afterProfiled ActionBar and UnderlineNav with three runs per component on each branch. Scenario: mount wide → narrow → wide → narrow → open overflow menu.
Result: Both components showed one fewer render pass during mount, consistently across all three runs. Ordinary resizing showed no further render-count reduction. Overflow contents and ordering matched on both branches. |
There was a problem hiding this comment.
🟡 Changes recommended
An intervening Provider commit can cancel a required rebuild and leave mounted descendants missing from the registry.
1 open issue
What changed in this PR
Addresses #8606 by deferring descendant registry publication until after effects, aiming to reduce cascading renders while preserving item order and metadata.
Changes:
- Uses a shared scheduler and authoritative snapshot to batch registry updates.
- Adds lifecycle stress tests and render-count assertions.
- Includes a patch changeset for ActionBar and UnderlineNav.
| File | Description |
|---|---|
packages/react/src/utils/descendant-registry.tsx |
Defers and coalesces registry rebuilds and publication. |
packages/react/src/utils/__tests__/descendant-registry.test.tsx |
Adds asynchronous assertions and rapid lifecycle coverage. |
.changeset/batch-descendant-registry-updates.md |
Documents the consumer-facing batching improvement. |
Review effort: Balanced
| if (workingRegistryRef.current === 'queued') { | ||
| rebuildRegistry() | ||
| } else if (workingRegistryRef.current === 'idle') { | ||
| setRegistry(new Map(registryRef.current)) |
There was a problem hiding this comment.
This seems to be a valid call-out—wondering if this has been investigated? A test that covers this case sounds like a good idea too!
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 23a42dde-c59d-4f5a-97ca-4fa6f7c41164
|
Integration test results from github/github-ui PR:
All checks passed! |
github-actions
Bot
added
integration-tests: passing
Closes #8606
Moves descendant registry state publication onto a shared deferred scheduler. Registration rebuilds, value changes, and cleanup now update one authoritative snapshot so rapid lifecycle changes coalesce without stale descendants, lost updates, or ordering regressions.
Adds stress coverage for rapid mount, update, unmount, unregister, remount, and reorder sequences, including render-count assertions for batched publication.
Changelog
New
None.
Changed
Removed
None.
Rollout strategy
Testing & Reviewing
npm test -- packages/react/src/utils/__tests__/descendant-registry.test.tsx --run— passed (11 tests)npm run build— passednpm run type-check— passednpm run lint— passednpm run lint:css— passednpm run format:diff— passedCI=true npm test— passed (160 files, 2,902 tests; 3 skipped, 10 todo)