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

Copilot AI 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():
const hostElement = document.createElement('div')
document.body.append(hostElement)
const root = createRoot(hostElement)
const onClose: 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-bot Bot 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
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

Copilot AI 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>
Copilot AI changed the title [WIP] Fix useConfirm rendering issues with detached createRoot Fix useConfirm host element leaking into the DOM and tests Jun 8, 2026
Copilot AI requested a review from mattcosta7 June 8, 2026 15:50
@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 8, 2026
@github-actions

github-actions Bot 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 owner June 8, 2026 15:59
@mattcosta7 mattcosta7 requested review from Copilot and siddharthkp June 8, 2026 15:59
@github-actions github-actions Bot temporarily deployed to storybook-preview-7935 June 8, 2026 16:02 Inactive
Copilot AI reviewed Jun 8, 2026
View reviewed changes

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 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
File Description
packages/react/src/ConfirmationDialog/ConfirmationDialog.tsx Switch confirm() to per-call host creation and explicit host removal on close.
packages/react/src/ConfirmationDialog/ConfirmationDialog.test.tsx Add regression test intended to ensure the confirm() host element is removed on close.
.changeset/confirm-dialog-host-cleanup.md Patch changeset describing the host cleanup behavior change.

Copilot's findings

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

Comment thread packages/react/src/ConfirmationDialog/ConfirmationDialog.test.tsx Outdated
@mattcosta7 mattcosta7 requested a review from jonrohan June 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-actions Bot temporarily deployed to storybook-preview-7935 June 9, 2026 23:53 Inactive
@primer primer Bot 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

Copilot AI commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

@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-actions Bot temporarily deployed to storybook-preview-7935 June 17, 2026 13:17 Inactive
@github-actions github-actions Bot temporarily deployed to storybook-preview-7935 June 17, 2026 20:20 Inactive
@github-actions github-actions Bot temporarily deployed to storybook-preview-7935 June 18, 2026 00:03 Inactive
@github-actions github-actions Bot temporarily deployed to storybook-preview-7935 June 19, 2026 13:54 Inactive
@primer-integration

primer-integration Bot 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
@mattcosta7 mattcosta7 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 queue Jun 23, 2026
@mattcosta7 mattcosta7 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 5b3c806 Jun 23, 2026
61 checks passed
@mattcosta7 mattcosta7 deleted the copilot/fix-useconfirm-leak branch June 23, 2026 12:30
@primer primer Bot mentioned this pull request Jun 23, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@jonrohan jonrohan jonrohan approved these changes
@mattcosta7 mattcosta7 Awaiting requested review from mattcosta7
@siddharthkp siddharthkp Awaiting requested review from siddharthkp

Assignees

@mattcosta7 mattcosta7

Copilot code review Copilot

Labels

Canary Release Apply this label when you want CI to create a canary release of the current PR integration-tests: skipped manually Changes 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