fix(BpkBottomSheet): clear close-animation timer on unmount - #4709
fix(BpkBottomSheet): clear close-animation timer on unmount#4709tuxiu.luo (LuoTuxiu) wants to merge 2 commits into
Conversation
The 240ms close-animation setTimeout was never cancelled when the component unmounted. If the host component unmounted before the timer fired (e.g. during test teardown), the timer would still call onClose, triggering a setState on an already-unmounted consumer and causing a ReferenceError: window is not defined in JSDOM environments. Store the timer ID in a ref and clear it in a useEffect cleanup so the callback is always cancelled on unmount. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Visit https://backpack.github.io/storybook-prs/4709 to see this build running in a browser. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR addresses a timer leak in BpkBottomSheet by tracking the close-animation timeout and clearing it on unmount to prevent onClose firing after the component has been removed.
Changes:
- Store the close
setTimeouthandle in a ref and clear it in auseEffectunmount cleanup. - Add a regression test intended to verify
onCloseis not called after unmount.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/backpack-web/src/bpk-component-bottom-sheet/src/BpkBottomSheet.tsx | Track/clear close-animation timer on unmount to prevent post-unmount callbacks. |
| packages/backpack-web/src/bpk-component-bottom-sheet/src/BpkBottomSheet-test.tsx | Adds a regression test targeting the timer leak scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -147,7 +157,7 @@ const BpkBottomSheet = ({ | |||
| const timeoutDuration = isAboveMobile ? 0 : animationTimeout; | |||
|
|
|||
| setExiting(true); | |||
| setTimeout(() => { | |||
| closeTimerRef.current = setTimeout(() => { | |||
| onClose(arg0, arg1); | |||
| setExiting(false); | |||
| }, timeoutDuration); | |||
There was a problem hiding this comment.
Fixed — now clears any in-flight timer before scheduling a new one, and resets closeTimerRef.current = null after the timer fires so state accurately reflects whether a timer is pending.
| it('does not call onClose after unmount (timer leak regression)', () => { | ||
| jest.useFakeTimers(); | ||
| const onClose = jest.fn(); | ||
| const { unmount } = render( | ||
| <BpkBottomSheet | ||
| ariaLabelledby="bottom-sheet" | ||
| closeLabel="Close" | ||
| id="my-bottom-sheet" | ||
| isOpen | ||
| onClose={onClose} | ||
| > | ||
| Content | ||
| </BpkBottomSheet>, | ||
| ); | ||
| // Simulate close button click would normally schedule a 240ms timer. | ||
| // Unmounting before the timer fires should cancel it. | ||
| unmount(); | ||
| jest.advanceTimersByTime(300); | ||
| expect(onClose).not.toHaveBeenCalled(); | ||
| jest.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
Fixed — the test now clicks the Close button first (via userEvent) so the 240ms timer is actually scheduled before unmounting. The assertion now meaningfully catches the regression.
| it('does not call onClose after unmount (timer leak regression)', () => { | ||
| jest.useFakeTimers(); | ||
| const onClose = jest.fn(); | ||
| const { unmount } = render( | ||
| <BpkBottomSheet | ||
| ariaLabelledby="bottom-sheet" | ||
| closeLabel="Close" | ||
| id="my-bottom-sheet" | ||
| isOpen | ||
| onClose={onClose} | ||
| > | ||
| Content | ||
| </BpkBottomSheet>, | ||
| ); | ||
| // Simulate close button click would normally schedule a 240ms timer. | ||
| // Unmounting before the timer fires should cancel it. | ||
| unmount(); | ||
| jest.advanceTimersByTime(300); | ||
| expect(onClose).not.toHaveBeenCalled(); | ||
| jest.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
Fixed — moved timer restoration to afterEach(() => jest.useRealTimers()) so it runs even if the test throws, and wrapped advanceTimersByTime in act() to avoid React update warnings.
- Clear any in-flight timer before scheduling a new one (multiple handleClose calls no longer leave orphaned timers) - Reset closeTimerRef.current to null after the timer fires - Fix regression test: click Close first so the timer is actually scheduled before unmounting - Use afterEach to restore real timers (guards against test-throw leaks) - Wrap advanceTimersByTime in act() to avoid React update warnings Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Visit https://backpack.github.io/storybook-prs/4709 to see this build running in a browser. |
Summary
setTimeoutID in arefuseEffectcleanup that callsclearTimeouton unmountonCloseis not calledWhy
BpkBottomSheet.handleCloseschedules a 240mssetTimeoutto wait for the slide-out animation before calling the consumer'sonClose. This timer was never cancelled on unmount.When the host component unmounted before the timer fired (e.g. a test finishing before the animation completes), the timer would still fire and call
onClose, which triggeredsetStateon an already-unmounted component. In JSDOM test environments this surfaces as:The error originates in the consumer's test but is attributed to unrelated test files, making it very hard to diagnose.
Consumers have been working around this by guarding their
onClosecallback with anisMountedref (e.g. hotels-website#11795). The root fix belongs here in Backpack.Test plan
onClosenot calledBpkBottomSheettests pass