Skip to content

Fix useConfirm host element leaking into the DOM and tests - #7935

Merged
mattcosta7 merged 10 commits into
mainfrom
copilot/fix-useconfirm-leak
Jun 23, 2026
Merged

Fix useConfirm host element leaking into the DOM and tests#7935
mattcosta7 merged 10 commits into
mainfrom
copilot/fix-useconfirm-leak

Conversation

CopilotAI commented Jun 8, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

useConfirm() / confirm() rendered ConfirmationDialog into a detached createRoot whose host <div>, appended to document.body, was never removed on close. The empty host accumulated across calls and leaked into consumers—most visibly poisoning subsequent @testing-library/react tests whose cleanup() cannot reach a sibling root.

This is the surgical, non-breaking subset of the issue's proposals (#3 + dropping the stale module-level host). The full in-tree provider/portal rewrite (#1) and exported teardown handle (#2) remain follow-ups for a major release.

Changelog

New

  • Regression test under a useConfirm describe block asserting the host element is detached from <body> on close.

Changed

  • confirm() now creates a fresh host element per call and removes it from document.body after root.unmount():
consthostElement=document.createElement('div')document.body.append(hostElement)constroot=createRoot(hostElement)constonClose: ConfirmationDialogProps['onClose']=gesture=>{root.unmount()hostElement.remove()resolve(gesture==='confirm')}

Removed

  • Module-level let hostElement that was reused across calls and lingered on <body>.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Verify the new test fails on main (leaked host persists) and passes here. Behavior is unchanged on the happy path; only the orphaned container is now cleaned up. Worth confirming no consumer relied on scraping/reusing the persistent [role="alertdialog"] host.

Merge checklist

@changeset-bot

changeset-botBot commented Jun 8, 2026•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ddbe5a2

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

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

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

CopilotAI linked an issue Jun 8, 2026 that may be closed by this pull request
useConfirm renders into a detached createRoot that leaks to consumers (esp. tests) #7934
Closed
@mattcosta7
Detach confirm() host element from body on close to fix leak
2765933
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
CopilotAI changed the title [WIP] Fix useConfirm rendering issues with detached createRootFix useConfirm host element leaking into the DOM and testsJun 8, 2026
CopilotAI requested a review from mattcosta7June 8, 2026 15:50
@github-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jun 8, 2026
@github-actions

github-actionsBot commented Jun 8, 2026

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.

@mattcosta7

mattcosta7 commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

this is a fix for now

ideally we wouldn't be creating a new react root for this, but we don't have a good path to avoid that currently

@mattcosta7
mattcosta7 marked this pull request as ready for review June 8, 2026 15:59
@mattcosta7
mattcosta7 requested a review from a team as a code ownerJune 8, 2026 15:59
@github-actions
github-actionsBottemporarily deployed to storybook-preview-7935 June 8, 2026 16:02 Inactive
CopilotAI reviewed Jun 8, 2026
View reviewed changes

CopilotAI 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 fixes a DOM leak in useConfirm() / confirm() where the detached createRoot() host <div> appended to document.body was never removed, accumulating empty containers and interfering with downstream consumers (notably RTL test cleanup).

Changes:

  • Remove the module-level reusable hostElement and create a fresh host element per confirm() call.
  • On dialog close, unmount the React root and remove the host element from document.body.
  • Add a regression test and a patch changeset documenting the behavior change.
Show a summary per file
FileDescription
packages/react/src/ConfirmationDialog/ConfirmationDialog.tsxSwitch confirm() to per-call host creation and explicit host removal on close.
packages/react/src/ConfirmationDialog/ConfirmationDialog.test.tsxAdd regression test intended to ensure the confirm() host element is removed on close.
.changeset/confirm-dialog-host-cleanup.mdPatch changeset describing the host cleanup behavior change.

Copilot's findings

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

Comment threadpackages/react/src/ConfirmationDialog/ConfirmationDialog.test.tsx Outdated
@mattcosta7
mattcosta7 requested a review from jonrohanJune 9, 2026 16:04
@mattcosta7
Potential fix for pull request finding
87ab9c3
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actions
github-actionsBottemporarily deployed to storybook-preview-7935 June 9, 2026 23:53 Inactive
@primer
primerBot enabled auto-merge June 12, 2026 15:35
@mattcosta7

mattcosta7 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

@mattcosta7
Merge origin/main into copilot/fix-useconfirm-leak and apply fix to u…
8cf6ec8
…seConfirm.ts
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
auto-merge was automatically disabled June 17, 2026 12:55

Head branch was pushed to by a user without write access

CopilotAI commented Jun 17, 2026

Copy link
Copy Markdown
ContributorAuthor

@copilot resolve the merge conflicts in this pull request

Done. Merged origin/main and resolved the conflict — main had moved useConfirm/confirm into useConfirm.ts, so I applied the fix there (dropping the module-level hostElement and calling hostElement.remove() on close) and removed the now-duplicate code from ConfirmationDialog.tsx. All 19 tests pass.

@github-actions
github-actionsBottemporarily deployed to storybook-preview-7935 June 17, 2026 13:17 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-7935 June 17, 2026 20:20 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-7935 June 18, 2026 00:03 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-7935 June 19, 2026 13:54 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-7935 June 23, 2026 12:11 Inactive
@primer-integration

primer-integrationBot commented Jun 23, 2026

Copy link
Copy Markdown

⚠️ Integration PR Outdated

This integration PR does not have the latest commit from the primer/react PR.

Integration PR references:8d07b19528af43150072e292535d20226e71158d
Latest commit on primer/react PR:ddbe5a2c7384f67f42c45518fa070f71ebb8be92

Please update the integration PR to reference the latest commit from the primer/react PR before reviewing workflow results.

@mattcosta7
mattcosta7 enabled auto-merge June 23, 2026 12:18
@mattcosta7mattcosta7 added the integration-tests: skipped manually Changes in this PR do not require an integration test label Jun 23, 2026
@mattcosta7
mattcosta7 added this pull request to the merge queueJun 23, 2026
@mattcosta7mattcosta7 removed the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jun 23, 2026
Merged via the queue into main with commit 5b3c806Jun 23, 2026
61 checks passed
@mattcosta7
mattcosta7 deleted the copilot/fix-useconfirm-leak branch June 23, 2026 12:30
@primerprimerBot mentioned this pull request Jun 23, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code reviewCopilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@jonrohanjonrohanjonrohan approved these changes
@mattcosta7mattcosta7Awaiting requested review from mattcosta7
@siddharthkpsiddharthkpAwaiting requested review from siddharthkp

Assignees

@mattcosta7mattcosta7

Copilot code reviewCopilot

Labels

Canary ReleaseApply this label when you want CI to create a canary release of the current PRintegration-tests: skipped manuallyChanges in this PR do not require an integration test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

useConfirm renders into a detached createRoot that leaks to consumers (esp. tests)

5 participants

@mattcosta7@jonrohan@siddharthkp