Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prevent SwipeableDrawer crash when parent update while swiping #30440

Closed
wants to merge 1 commit into from
Closed

Prevent SwipeableDrawer crash when parent update while swiping #30440

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,8 @@ const SwipeableDrawer = React.forwardRef(function SwipeableDrawer(inProps, ref)
);

const handleBodyTouchEnd = useEventCallback((nativeEvent) => {
if (!touchDetected.current) {
// the ref may be null when a parent component updates while swiping
if (!paperRef.current || !touchDetected.current) {
Copy link
Member

@oliviertassinari oliviertassinari Dec 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type of defensive check usually hides root causes. To always be suspicious.

Here, I suspect that because the cleanup happens in a useEffect, it's async, but it should be a layout effect to be sync. We might have similar cleanup problems in the other components.

https://github.com/mui-org/material-ui/blob/fcf2a8a5c90e6a975b0a8c391aa2f95f6e860b15/packages/mui-material/src/SwipeableDrawer/SwipeableDrawer.js#L510

I hope it illustrates why #30440 (comment) (not the only reason, e.g. so we can add a test case)

Copy link
Author

@xavier-villelegier xavier-villelegier Dec 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @oliviertassinari, thanks for you answer, I definitely agree. We've inlined the component in production to check, and it's not the root cause anyway, the sentries are still there, which sounds a bit surprising 🤔 I've also tried to replace this useEffect by a useLayoutEffect, and still the same errors

image
image

I've tried various things to trigger the error, but no luck I didn't reproduce it yet. But it's not that edge case as we have 1k/day occurences on iOS and 1k/day on Android. Thus it doesn't look like OS-related, i'd say it's more about the react lifecycle itself

return;
}
claimedSwipeInstance = null;
Expand Down