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.

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

Components: Fix the Snackbar auto-dismissal timers #58604

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

Mamaduka
Copy link
Member

@Mamaduka Mamaduka commented Feb 2, 2024

What?

Resolves #58594.

PR removes onDismiss/onRemove callbacks from side-effect dependencies to avoid resetting timers on re-render.

Why?

See #58594.

Testing Instructions

  1. Open a post or page.
  2. Insert a block.
  3. Trigger a few snackbar notices. Copying a block can do that.

Expected behavior

Let's assume there is a one-second difference between message triggers.

Message 1 - 10s
Message 2 - 10s + 1s
Message 3 - 10s + 2s

Screenshots or screencast

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Feb 2, 2024
@Mamaduka Mamaduka self-assigned this Feb 2, 2024
@Mamaduka Mamaduka requested a review from ajitbohra as a code owner February 2, 2024 09:12
@Mamaduka Mamaduka requested review from chad1008 and a team and removed request for ajitbohra February 2, 2024 09:12
Copy link

github-actions bot commented Feb 2, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core SVN

If you're a Core Committer, use this list when committing to wordpress-develop in SVN:

Props: mamaduka, mciampini.

GitHub Merge commits

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ciampo ciampo requested a review from mirka February 2, 2024 10:36
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Could also add a CHANGELOG entry? Thank you!

// The `onDismiss/onRemove` can have unstable references,
// trigger side-effect cleanup, and reset timers.
const callbackRefs = useRef( { onDismiss, onRemove } );
useLayoutEffect( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for useLayoutEffect instead of useEffect ? In theory useEffects should be called in order, so we should be guaranteed that the onDimiss and onRemove refs are always up to date

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing specific. This is just a common pattern; we even have a useLatestRef hook, but I don't use here because returned value isn't recognized as Ref and linter complains.

Related: https://epicreact.dev/the-latest-ref-pattern-in-react/

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the link, super interesting read!

I took a look back at this article by Kent that I usually refer to when talking useEffect and useLayoutEffect, I noticed the "special case" section too!

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

🚀

Code changes LGTM and tests well as per instructions.
I also realised that we don't have unit tests for this component, which would have been handy here — but not in the scope for this PR.

Thank you for working on this, @Mamaduka 🙏

@Mamaduka
Copy link
Member Author

Mamaduka commented Feb 2, 2024

Thanks for testing and review, @ciampo!

@Mamaduka Mamaduka merged commit 4076bb1 into trunk Feb 2, 2024
62 checks passed
@Mamaduka Mamaduka deleted the fix/snackbar-dismiss-timers branch February 2, 2024 13:10
@github-actions github-actions bot added this to the Gutenberg 17.7 milestone Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component: The Snackbar auto-dismissal timeout is reset on every rerender
2 participants