Extract reusable "descendant registry" pattern from ActionBar, with new support for reordering elements#7585
Extract reusable "descendant registry" pattern from ActionBar, with new support for reordering elements#7585iansan5653 wants to merge 11 commits intomainfrom
ActionBar, with new support for reordering elements#7585Conversation
🦋 Changeset detectedLatest commit: f818594 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 |
ActionBar, with new support for reordering elementsActionBar, with new support for reordering elements
|
👋 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 |
|
@copilot Write unit tests for
|
|
@iansan5653 I've opened a new pull request, #7586, to work on those changes. Once the pull request is ready, I'll request review from you. |
… new support for reordering elements (#7586) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: iansan5653 <[email protected]>
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
Extract reusable 'descendant registry' pattern from ActionBar, adding support for reordering elements. Fixes a bug with menu item order and improves initial render performance.
There was a problem hiding this comment.
Pull request overview
This PR extracts a reusable "descendant registry" pattern from ActionBar into a new utility (descendant-registry.tsx) and fixes an ordering bug where items added to the middle or beginning of the component tree after initial render would appear in the wrong order. The new implementation rebuilds the entire registry when new items are added (to ensure correct ordering) but uses a working ref to batch all descendant registrations into a single state update, significantly improving performance over the previous approach.
Changes:
- Introduces
createDescendantRegistry<T>()utility for tracking deeply nested children via context and effects - Fixes ordering issues by triggering a full registry rebuild when new descendants are added
- Refactors
ActionBarto use the new registry pattern, replacing the oldregisterChild/unregisterChildapproach - Adds comprehensive tests for the new registry pattern covering ordering, updates, and unmounting
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
packages/react/src/utils/descendant-registry.tsx |
New utility implementing the descendant registry pattern with proper ordering support |
packages/react/src/utils/__tests__/descendant-registry.test.tsx |
Test suite covering registry behavior including ordering edge cases |
packages/react/src/ActionBar/ActionBar.tsx |
Refactored to use new registry utility, removing old registration logic and adding useWidth helper |
Comments suppressed due to low confidence (3)
packages/react/src/ActionBar/ActionBar.tsx:566
- Including
widthRefin the dependency array is unnecessary because refs are stable objects that never change identity. Refs should not be included in effect or callback dependency arrays. RemovewidthReffrom the dependency array.
[ariaLabel, overflowIcon, icon, items, returnFocusRef, widthRef],
packages/react/src/ActionBar/ActionBar.tsx:592
- Including
widthRefin the dependency array is unnecessary because refs are stable objects that never change identity. Refs should not be included in effect or callback dependency arrays. RemovewidthReffrom the dependency array.
const registryProps = useCallback((): ChildProps => ({type: 'divider', width: widthRef.current}), [widthRef])
packages/react/src/ActionBar/ActionBar.tsx:494
- Including
widthRefin the dependency array is unnecessary because refs are stable objects that never change identity. Refs should not be included in effect or callback dependency arrays. RemovewidthReffrom the dependency array.
[props, disabled, onClick, groupId, widthRef],
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14585 |
|
Sounds good! Can you go a little into what this means for UnderlineNav? If you'd like, we can save that conversation for a different PR
|
|
Hmm, looks like integration CI is failing because of an unrelated change to
Sure! You can see my work in progress PR here #7506 |
|
Integration test results from github/github-ui:
CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures. VRT check ensures that when visual differences are detected, the PR cannot proceed until someone acknowledges the changes by adding the "visual difference acknowledged" label. Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance. |
Strange because the CI for main is passing in github-ui: https://github.com/github/github-ui/pull/14190. Have you debugged this in a codespace yet? |
I previously introduced a 'registry' pattern
ActionBar, which uses context to allow items to register themselves even if deeply nested in the React component tree. This is somewhat similar to theuseSlotspattern except that it doesn't rely onChildrenoperations, so it works with items wrapped in other components and fragments. However, it has the disadvantage of not working during SSR (because it relies on effects). This is not an issue for components likeActionBarwhere the data is being collected for a not-initially-visible overflow menu.There is a flaw in the current implementation, however: it always preserves the order from the initial render. So if an item is added to the middle or beginning after the initial render, the order of overflow items in the menu will be incorrect.
In addition, I'd like to reuse this pattern in the work I'm doing on
UnderlineNav.So this PR extracts the pattern out to be reusable and fixes the ordering bug. The new approach is essentially the same when updating a value or removing an item, but the key difference is that we rebuild the registry from scratch when adding a new item. We do this by updating a state value that triggers all the registration effects to re-run.
To avoid many consecutive state updates when triggering a rebuild, we use a working ref to build the map, then only commit it to state at the end of the process. This is a significant performance improvement over the previous approach, which would trigger a state update and re-render once for each descendant that registers.
This is somewhat comparable to the deprecated
useSlotspattern, however it has several improvements to try to solve for the challenges we've faces with that solution:children-focused but rather can be used for any kind of dataMapNote that this does not mean that we should just use this new pattern everywhere that we use slots today. There's still a case for the
useSlotsChildren-based approach when we need SSR compatibility. This pattern is primarily useful for cases where rendered UI depending on the data is not immediately visible, like in overflow menus or non-visible DOM attributes.Usage
The pattern works like this: First, we will instantiate a new registry via
createDescendantRegistry(comparable tocreateContext). The type of the entries can be anything you want:Then, the top-level component instantiates the registry state and provides the context to its
children. The registry is aMapso that the component can access the unique item IDs to use askeys if necessary. The registry is initiallyundefineduntil built for the first time:Finally, item components register themselves:
Now, the container component has a complete, ordered registry of all
ExampleItemcomponents contained inside of it.Changelog
Fixes
ActionBaroverflow menu when new items are added after initial renderActionBarRollout strategy
Testing & Reviewing
Merge checklist