Revert "Replace `useRefObjectAsForwardedRef` with `useMergedRefs` internally" by liuliu-dev · Pull Request #7737 · primer/react

liuliu-dev · 2026-04-08T16:38:39Z

Reverts #7638

…ernally …" This reverts commit f04e85d.

changeset-bot · 2026-04-08T16:38:42Z

⚠️ No Changeset found

Latest commit: c5f47ce

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

github-actions · 2026-04-08T16:39:11Z

⚠️ 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

Pull request overview

Reverts #7638 by switching internal “combined refs” usage back from useMergedRefs to useRefObjectAsForwardedRef across multiple components in @primer/react.

Changes:

Replace useMergedRefs usage with useRefObjectAsForwardedRef and pass local ref objects directly to underlying elements.
Update useMergedRefs docs/tests variable naming (mergedRef → combinedRef).
Remove the changeset that described the (now-reverted) internal ref-handling update.

Show a summary per file

File	Description
packages/react/src/TextInputWithTokens/TextInputWithTokens.tsx	Reverts forwarded-ref wiring back to `useRefObjectAsForwardedRef` + local `ref`.
packages/react/src/PageLayout/PageLayout.tsx	Reverts Pane/Sidebar forwarded-ref wiring back to `useRefObjectAsForwardedRef`.
packages/react/src/Overlay/Overlay.tsx	Reverts overlay ref wiring back to `useRefObjectAsForwardedRef`.
packages/react/src/Link/Link.tsx	Reverts ref wiring and introduces a ref cast to satisfy polymorphic typing.
packages/react/src/hooks/useMergedRefs.ts	Renames variables in JSDoc examples (`mergedRef` → `combinedRef`).
packages/react/src/hooks/tests/useMergedRefs.test.tsx	Renames test variable usage accordingly.
packages/react/src/Heading/Heading.tsx	Reverts forwarded-ref wiring back to `useRefObjectAsForwardedRef`.
packages/react/src/Dialog/Dialog.tsx	Reverts forwarded-ref wiring back to `useRefObjectAsForwardedRef`.
packages/react/src/deprecated/DialogV1/Dialog.tsx	Reverts forwarded-ref wiring back to `useRefObjectAsForwardedRef`.
packages/react/src/Button/ButtonBase.tsx	Reverts forwarded-ref wiring and changes ref assignment on polymorphic component.
packages/react/src/Autocomplete/AutocompleteOverlay.tsx	Reverts combined ref handling for overlay scroll container.
packages/react/src/Autocomplete/AutocompleteInput.tsx	Reverts forwarded-ref wiring back to `useRefObjectAsForwardedRef`.
packages/react/src/Autocomplete/Autocomplete.test.tsx	Removes an explicit focus call in a menu-open test.
packages/react/src/ActionList/Heading.tsx	Reverts forwarded-ref wiring back to `useRefObjectAsForwardedRef`.
packages/react/src/ActionBar/ActionBar.tsx	Reverts forwarded-ref wiring back to `useRefObjectAsForwardedRef` for icon button.
.changeset/combined-refs-hook.md	Deletes the changeset introduced by the reverted PR.

Copilot's findings

Files reviewed: 16/16 changed files
Comments generated: 4

Copilot · 2026-04-08T16:48:29Z

 data-inline={inline}
 data-hover-color={hoverColor}
 {...restProps}
- ref={mergedRef}
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ ref={innerRef as any}


ref={innerRef as any} (and disabling the lint rule) bypasses type-safety for the polymorphic as prop and can mask cases where the rendered component doesn’t support refs. Prefer keeping the ref fully typed (e.g., by using useMergedRefs again, or by expressing the polymorphic ref type explicitly) so TS can enforce that ref is valid for As without resorting to any.

Copilot · 2026-04-08T16:48:30Z

+ // @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
+ ref={innerRef}


Avoid introducing @ts-ignore on the ref assignment here; it hides a real typing mismatch between the polymorphic as element and innerRef’s declared element type. Consider typing innerRef to match the union of supported elements (e.g., button/anchor) or switching back to useMergedRefs so forwardedRef can be attached without suppressing TypeScript errors.
Suggested change
// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
ref={innerRef}
ref={
Component==='a'
? (innerRefasReact.Ref<HTMLAnchorElement>)
: (innerRefasReact.Ref<HTMLButtonElement>)
}

Copilot · 2026-04-08T16:48:30Z

 const inputNode = getByLabelText(AUTOCOMPLETE_LABEL)

 expect(inputNode.getAttribute('aria-expanded')).not.toBe('true')
- inputNode.focus()
 fireEvent.click(inputNode)
 fireEvent.keyDown(inputNode, {key: 'ArrowDown'})



Removing inputNode.focus() means this test now relies on fireEvent.click(inputNode) to focus/open the menu, which can make it pass even if the intended “ArrowDown when focused opens menu” behavior regresses (click may open the menu by itself). To keep the assertion targeted, focus the input (or assert the menu remains closed after click) before the ArrowDown keyDown.

Copilot · 2026-04-08T16:48:30Z

 const featureFlagMaxHeightClampToViewport = useFeatureFlag('primer_react_overlay_max_height_clamp_to_viewport')
 const overlayRef = useRef<HTMLDivElement>(null)
-const mergedRef = useMergedRefs(forwardedRef, overlayRef)
+useRefObjectAsForwardedRef(forwardedRef, overlayRef)
 const slideAnimationDistance = 8 // var(--base-size-8), hardcoded to do some math


Switching back to useRefObjectAsForwardedRef reintroduces its known behavior of invoking callback refs on every re-render (see useRefObjectAsForwardedRef tests’ TODO), which can cause unnecessary work/side effects and is especially risky with React 19 callback refs that return cleanup functions. Prefer useMergedRefs here so refs are only updated on mount/unmount/target changes and React 19 cleanup refs are handled correctly.

primer-integration · 2026-04-09T09:55:44Z

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/18179

primer-integration · 2026-04-09T10:10:43Z

Integration test results from github/github-ui:

CI Failed
VRT Passed
Projects Passed

CI check runs linting, type checking, and unit tests. Check the workflow logs for specific failures.

Need help? If you believe this failure is unrelated to your changes, please reach out to the Primer team for assistance.

siddharthkp · 2026-04-09T10:11:40Z

github/ci is failing, but the failures are not related to this PR, they are failing on main because of feature flags that were removed. Adding integration-tests: skipped manually

Revert "Replace useRefObjectAsForwardedRef with useMergedRefs int…
c5f47ce
…ernally …" This reverts commit f04e85d.

github-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Apr 8, 2026

liuliu-dev marked this pull request as ready for review April 8, 2026 16:43

liuliu-dev requested a review from a team as a code owner April 8, 2026 16:43

liuliu-dev requested review from Copilot and siddharthkp April 8, 2026 16:43

Copilotstarted reviewing on behalf of liuliu-dev April 8, 2026 16:43 View session

CopilotAI reviewed Apr 8, 2026
View reviewed changes

liuliu-dev deployed to github-pages April 8, 2026 16:49 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-7737 April 8, 2026 16:50 Inactive

siddharthkp added Canary Release Apply this label when you want CI to create a canary release of the current PR and removed Canary Release Apply this label when you want CI to create a canary release of the current PR labels Apr 9, 2026

siddharthkp enabled auto-merge April 9, 2026 10:11

siddharthkp added integration-tests: skipped manually Changes in this PR do not require an integration test and removed integration-tests: skipped manually Changes in this PR do not require an integration test labels Apr 9, 2026

siddharthkp approved these changes Apr 9, 2026
View reviewed changes

siddharthkp disabled auto-merge April 9, 2026 10:13

siddharthkp enabled auto-merge April 9, 2026 10:13

siddharthkp disabled auto-merge April 9, 2026 10:13

siddharthkp merged commit 903e739 into mainApr 9, 2026
89 checks passed

siddharthkp deleted the revert-7638-create-use-combined-refs branch April 9, 2026 10:13

llastflowers mentioned this pull request Jun 25, 2026
Replace useRefObjectAsForwardedRef with useMergedRefs internally #8065
Closed

Navigation Menu

Uh oh!

Revert "Replace `useRefObjectAsForwardedRef` with `useMergedRefs` internally" - #7737

Revert "Replace `useRefObjectAsForwardedRef` with `useMergedRefs` internally"#7737
siddharthkp merged 1 commit into
mainfrom
revert-7638-create-use-combined-refs

liuliu-dev commented Apr 8, 2026

Uh oh!

changeset-botBot commented Apr 8, 2026

Uh oh!

github-actionsBot commented Apr 8, 2026

Uh oh!

CopilotAI left a comment

Uh oh!

CopilotAIApr 8, 2026

Uh oh!

CopilotAIApr 8, 2026

Uh oh!

CopilotAIApr 8, 2026

Uh oh!

CopilotAIApr 8, 2026

Uh oh!

primer-integrationBot commented Apr 9, 2026

Uh oh!

primer-integrationBot commented Apr 9, 2026

Uh oh!

siddharthkp commented Apr 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
		ref={innerRef}

-// @ts-ignore temporary disable as we migrate to css modules, until we remove PolymorphicForwardRefComponent
-ref={innerRef}
+ref={
+Component==='a'
+ ? (innerRefasReact.Ref<HTMLAnchorElement>)
+ : (innerRefasReact.Ref<HTMLButtonElement>)
+}

Uh oh!

Conversation

liuliu-dev commented Apr 8, 2026

Uh oh!

changeset-botBot commented Apr 8, 2026

⚠️ No Changeset found

Uh oh!

github-actionsBot commented Apr 8, 2026

⚠️ Action required

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

CopilotAIApr 8, 2026

Choose a reason for hiding this comment

Uh oh!

CopilotAIApr 8, 2026

Choose a reason for hiding this comment

Uh oh!

CopilotAIApr 8, 2026

Choose a reason for hiding this comment

Uh oh!

CopilotAIApr 8, 2026

Choose a reason for hiding this comment

Uh oh!

primer-integrationBot commented Apr 9, 2026

Uh oh!

primer-integrationBot commented Apr 9, 2026

Uh oh!

siddharthkp commented Apr 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants