Repository navigation
Fix Button loading announcements - #8528
siddharthkp wants to merge 2 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 9642198 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
github-actions
Bot
added
staff
|
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation is supported by regression coverage for both Strict Mode replay and real unmounting.
0 open findings
What changed in this PR
Fixes Button loading announcements under React Strict Mode while preserving unmount cancellation.
Changes:
- Defers live-region cleanup to distinguish Strict Mode effect replay from unmounting.
- Enables loading announcements and adds regression tests.
- Adds a manual Storybook reproduction and patch changeset.
| File | Description |
|---|---|
packages/react/src/live-region/Announce.tsx |
Makes announcement cleanup Strict Mode-safe. |
packages/react/src/live-region/__tests__/Announce.test.tsx |
Tests Strict Mode and unmount behavior. |
packages/react/src/Button/ButtonBase.tsx |
Announces loading messages when shown. |
packages/react/src/Button/Button.dev.stories.tsx |
Adds an interactive reproduction. |
packages/react/src/Button/__tests__/Button.test.tsx |
Verifies the loading live-region message. |
.changeset/bright-buttons-announce.md |
Documents the patch-level fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
siddharthkp
changed the title
|
Integration test results from github/github-ui PR:
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. |
github-actions
Bot
added
integration-tests: failing
| const element = ref.current | ||
| return () => { | ||
| if (savedAnnouncement.current !== null) { | ||
| savedAnnouncement.current.cancel() | ||
| savedAnnouncement.current = null | ||
| } | ||
| // StrictMode reconnects the rendered element before the next task. | ||
| // eslint-disable-next-line @eslint-react/web-api/no-leaked-timeout -- this task must outlive effect cleanup | ||
| cleanupTimeout.current = setTimeout(() => { | ||
| if (!element?.isConnected && savedAnnouncement.current !== null) { | ||
| savedAnnouncement.current.cancel() | ||
| savedAnnouncement.current = null | ||
| } | ||
| }, 0) |
There was a problem hiding this comment.
I think the underlying issue is our useEffectOnce usage since it won't fire twice 😞 If we want to fix this, I think we would remove that hook usage (and maybe lint against it?) and do something like:
useEffect(() => {
if (announceOnShow) {
announce()
}
}, []);Which would fire twice in StrictMode but would correctly coordinate with the cleanup effect here.
I think this would also need a change in the cleanup effect too for the previousAnnouncementText:
useEffect(() => {
return () => {
if (savedAnnouncement.current !== null) {
savedAnnouncement.current.cancel()
savedAnnouncement.current = null
previousAnnouncementText.current = null
}
}
}, [])Just so we don't noop on duplicate announcements. What do you think @siddharthkp ?
There was a problem hiding this comment.
Asked copilot to replace useEffectOnce with useEffect and reset previousAnnouncementText during cleanup. Hope this looks better?
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes github/primer#7110
Ensure Button loading messages reach the live region on the first transition to loading, including when React Strict Mode re-runs effects. Preserve cancellation after a real unmount and add a focused Storybook reproduction.
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
npm test -- --run packages/react/src/live-region/__tests__/Announce.test.tsx packages/react/src/Button/__tests__/Button.test.tsx(67 tests).npm run type-check --workspace=@primer/react.npm run buildbecause the local install is missing the Rolldown native binding (@rolldown/binding-darwin-arm64).