fix: restore toasts in a way which prevents infinite rerenders

jaredcwhite commented 2 weeks ago

This PR addresses #4181

[x] Addresses the issue in full
[ ] Addresses only certain aspects of the issue

Description

The Next.js router.push method can return a promise which is resolved when the new route has loaded. Instead of swallowing that (as void), we can provide a callback and use that for adding the toast to the toast stack. That resolves the infinite rendering issue. (I'm not quite sure why we haven't needed this solution everywhere, but at least now we know about it).

Turns out there was a very specific bug which that callback fix worked around but it didn't really address it. I've since updated this PR. Here's what was going on:

Apparently, functions pulled out of React Contexts aren't stable (aka addToast), meaning even if you put them in the dependency array of a useEffect hook, it won't prevent that hook to re-execute. The fix was to write a custom hook that wraps the context in a stable ref. That way, you can put the ref itself in the dependency array, then pull addToast out of the ref's current value within useEffect for use, thus preventing unnecessary re-execution (aka infinite rerenders).

So that's good, but now there's two different methods within the codebase of accessing addToast—the original way which is normally fine, and the new way just for useEffect. I'm not thrilled about that, but I also don't want to mandate having to use the ref everywhere now which is a bit more cumbersome. Happy to entertain feedback about that in either direction.

How Can This Be Tested/Reviewed?

Try applying to a listing which the backend has just switched off applications, or the due date is past, etc. You should see a single toast notification that the listing is now closed.

Author Checklist:

[ ] Added QA notes to the issue with applicable URLs
[x] Reviewed in a desktop view
[ ] Reviewed in a mobile view
[ ] Reviewed considering accessibility
[ ] Added tests covering the changes
[ ] Made corresponding changes to the documentation
[ ] Ran yarn generate:client and/or created a migration when required

Review Process:

Read and understand the issue
Ensure the author has added QA notes
Review the code itself from a style point of view
Pull the changes down locally and test that the acceptance criteria is met
Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

netlify[bot] commented 2 weeks ago

Deploy Preview for partners-bloom-dev ready!

Name	Link
Latest commit	f542dc5303e5daa62ef8429d6ca0d591ff7737a0
Latest deploy log	https://app.netlify.com/sites/partners-bloom-dev/deploys/66ea099f1e6e6e0009cfaecd
Deploy Preview	https://deploy-preview-4302--partners-bloom-dev.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

emilyjablonski commented 2 weeks ago

We'll need this fix in more places, potentially everywhere we are wanting to show a toast on a different page than where we start. For example, the RequireLogin component, which kicks in when you try and visit account/dashboard on the public site when you're logged out.

netlify[bot] commented 2 weeks ago

Deploy Preview for bloom-exygy-dev ready!

Name	Link
Latest commit	f542dc5303e5daa62ef8429d6ca0d591ff7737a0
Latest deploy log	https://app.netlify.com/sites/bloom-exygy-dev/deploys/66ea099f74ab4d00080881a3
Deploy Preview	https://deploy-preview-4302--bloom-exygy-dev.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jaredcwhite commented 2 weeks ago

@emilyjablonski Oof…this might require a whole lot of refactoring in various places. I'm starting to wonder if there's an alternative solution, ~~like maybe I can add a de-dup check inside of addToast somehow.~~

OK, I think I actually figured out the underlying problem now and how to solve it. Will work on a fix and then document what it is.

emilyjablonski commented 4 days ago

@jaredcwhite Is this ready for re-review?

jaredcwhite commented 4 days ago

@emilyjablonski It is now…I had to make a change based on how I actually determined was the root cause of the rerenders. See my updated PR description for the explanation: https://github.com/bloom-housing/bloom/pull/4302#issue-2506373944

jaredcwhite commented 20 hours ago

@ColinBuyck Hey, I tried to replicate this and I couldn't…I'm just seeing the single toast. Any other tips you could provide to trigger this?

bloom-housing / bloom