-
Notifications
You must be signed in to change notification settings - Fork 639
Fix Details
flickering
#7020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Details
flickering
#7020
Conversation
🦋 Changeset detectedLatest commit: 488ab49 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
There was a problem hiding this comment.
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 flickering and re-rendering issues in the Details
component by replacing dynamic child checking with a static dev-time warning. The original implementation used a MutationObserver
and state updates to conditionally inject a default Summary
, which caused performance issues and UI flickering.
Key Changes:
- Removed dynamic summary detection and conditional rendering logic
- Replaced with dev-only warning using the
warning
utility - Removed default summary injection when no summary is provided
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/react/src/Details/Details.tsx | Replaced MutationObserver-based summary detection with dev-only warning; removed state management and default summary injection |
packages/react/src/Details/tests/Details.test.tsx | Removed tests validating automatic default summary injection behavior |
packages/react/src/Details/Details.stories.tsx | Added story demonstrating usage with native <summary> element |
.changeset/dark-birds-dont.md | Added changeset documenting the patch fix for Details flickering |
warning( | ||
summary === null, | ||
'The <Details> component must have a <summary> child component. You can either use <Details.Summary> or a native <summary> element.', | ||
) |
Copilot
AI
Oct 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning condition is inverted. It currently warns when a summary is present (summary === null
evaluates to true
only when summary is null). It should warn when summary is not present. Change to summary !== null
or !summary
.
Copilot uses AI. Check for mistakes.
Closes https://github.com/github/primer/issues/5211
The original solution was created as a fix for this issue.
It really is not practical to add a component by default, have to check the children and then update the component. It really breaks the React philosophy and also patching bad code encourages bad patterns for users of the component.
Before (Flickering bug)
Screen.Recording.2025-10-17.at.16.44.15.mov
After (No flickering, new story)
Screen.Recording.2025-10-17.at.16.45.56.mov
Changelog
Added
Story with a custom summary
Changed
I replaced the previous behavior of adding a default
summary
with the standard solution we've used in other cases where we want to check similar things at runtime. This is just to show a warning on dev environments.Please note that this is an extra step we're taking and should not even be necessary; the documentation is already clear and we're following the
details
/summary
pattern in HTML. It should not be a source of issues at all.Having to check the children in the existing implementation was updating the state of the component and rerendering everything, with not only performance issues but also UI flickering. This fixes both issues.
Removed
Old tests that checked the addition of a default
Summary
when it was not present.Rollout strategy
Testing & Reviewing
Merge checklist