refactor(app): add test for shouldUseNotifications false to true

[mjhuff]

Overview

EDIT: See discussion below. Woops, we were already covered. Will just merge in the testing.

#17853 fixes an issue when the shouldUseNotifications flag is initially false and then flips to true. We must also account for scenarios in which the caller repeatedly sets the shouldUseNotifications flag between false and true: we do not want to add a new callback to the store every time shouldUseNotifications is true, only if it is not in the store already. Note that the callback added to the store is memoized, so includes is a sufficient filter check here. Also, we don't need to remove the callback every time shouldUseNotifications becomes false: we remove the callback on dismount.

This fix currently doesn't impact our codebase, fortunately. The one spot shouldUseNotifications flips is a one-time false to true.

This PR also adds testing for the earlier fix in the linked PR.

Test Plan and Hands on Testing

• Can't test in office, but easy to reason through.

Risk assessment

low

[sfoster1]

the memoization happens on the caller-side rather than here, right? we should probably have a comment on this function saying that the callback should be memoized so its identity is stable between calls from the same location.

[SyntaxColoring]

Seems OK, but can you elaborate on this?

Also, we don't need to remove the callback every time shouldUseNotifications becomes false: we remove the callback on dismount.

if the bug was that flipping the feature flag on and off kept accumulating callbacks, isn't the problem that the callback wasn't being removed when the feature flag went off?

[mjhuff]

if the bug was that flipping the feature flag on and off kept accumulating callbacks, isn't the problem that the callback wasn't being removed when the feature flag went off?

No, the problem is that every time shouldUseNotifications flips to true (not strictly the feature flag), we call appShellListener, and we re-add the callback to the store regardless of whether or not it's already present.

If it flips to false, we don't have to remove it immediately, because when the component dismounts, the callback gets cleaned up. See L88.

shouldUseNotifications potentially flipping to true repeatedly is the issue.

[SyntaxColoring]

I've looked more closely at the code and now I'm a little more confused.

I'm looking at this calling code. If I understand correctly:

1. Suppose we start with shouldUseNotifications set to true and the subscription fully set up.
2. Then shouldUseNotifications flips to false for any reason, including turning the feature flag off.
3. useEffect() is triggered.
   1. The cleanup function runs, which should call appShellListener with isDismounting: true, which should remove the callback.
   2. The useEffect() body runs again with the new value of shouldUseNotifications and no-ops because it is false.
4. shouldUseNotifications flips back to true for any reason, including turning the feature flag back on.
5. useEffect() is triggered again.

Because step 3i happens before step 7, how can there still be a callback in the store when we reach step 7?

[mjhuff]

I've looked more closely at the code and now I'm a little more confused.

Ah, no, the useEffect does not trigger the cleanup when shouldUseNotifications is false. That's the catch: the return statement of a useEffect is only invoked on component dismount. Any mounted component will invoke the dismount logic exactly once and only once.

[SyntaxColoring]

I've looked more closely at the code and now I'm a little more confused.

Ah, no, the useEffect does not trigger the cleanup when shouldUseNotifications is false. That's the catch: the return statement of a useEffect is only invoked on component dismount. Any mounted component will invoke the dismount logic exactly once and only once.

I don't think that's true?

My cleanup logic runs even though my component didn’t unmount

The cleanup function runs not only during unmount, but before every re-render with changed dependencies...

[mjhuff]

I don't think that's true?

Ah, yep thanks. You're correct.

I'll just merge the testing changes from this PR then 👍

[SyntaxColoring]

Cool, thanks for looking into this. LGTM if CI passes.

[mjhuff]

Cool, thanks for looking into this. LGTM if CI passes.

Thanks for being patient with me. It's been a week 😅