Skip to content

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

Open
Copilot wants to merge 3 commits into
mainfrom
copilot/fix-useconfirm-leak
Open

Fix useConfirm host element leaking into the DOM and tests#7935
Copilot wants to merge 3 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: 87ab9c3

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
Open
@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>
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 siddharthkp is a code owner automatically assigned from primer/engineer-reviewers

Assignees

@mattcosta7 mattcosta7

Copilot code review Copilot

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

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)

4 participants

@mattcosta7 @jonrohan