Display consistent results after user searches for an address

[eileen-nava]

🎫 Ticket

LG-8475: Clear previous search results

🛠 Summary of changes

1. If a user searches for an address that returns post office results, and then searches for invalid input that returns nothing, the first search's results won't persist.
2. If a user searches for an address that returns post office results, and then searches for another address that returns post office results, the new address and new number of post office results will update at the same time.

📜 Testing Plan

Scenario 1

• Search for an address that returns post office results, like 1600 Pennsylvania Ave NW. Observe the results.
• Search for invalid input, like asdkf.
• Observe that no results display.

Scenario 2

• Search for an address that returns post office results, like 1600 Pennsylvania Ave NW. Observe the results.
• Search for an address that returns a different number of post office results than the previous search, like 125 N Dallas, Mentone, TX 79754.
• Note that the new address and new number of post office results update simultaneously.

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before (Scenario 1):

8575Scenario1BeforeNoAudio.mov

After (Scenario 1):

8475Scenario1AfterNoAudio.mov

Before (Scenario 2):

8475Scenario2BeforeNoAudio.mov

After (Scenario 2):

8475Scenario2AfterNoAudio.mov

[allthesignals]

I found it a little simpler to change it this way:

  useEffect(() => {
    onFoundLocations(locationResults);

    foundAddress && onFoundAddress(foundAddress);
  }, [locationResults]);

[allthesignals]

This allows consumer to deal with a null state instead of blocking communication of null state.

[eileen-nava]

Looking into this! 👍🏻

[eileen-nava]

Update: I responded to outstanding feedback.

[aduth]

Should foundAddress stay here? Do we want onFoundAddress to be called when foundAddress changes?

[allthesignals]

IMO, yes.

[NavaTim]

@eileen-nava @allthesignals There's an exhaustive-deps rule published by Facebook that if enabled would flag this as a violation b/c foundAddress is missing from the hook dependencies.

Generally when the dependencies don't match the values used in a hook like this, it's considered a code smell in React. This is a very likely location for a bug to occur because the value is missing.

[allthesignals]

That’s a good rule of thumb to follow, but in this case, we don’t want it to update when either value is updated. We only want it to update when usps locations are found for a given valid address.

[NavaTim]

How about we split this into two effects, one for locationResults/onFoundLocations and a separate one for foundAddress/onFoundAddress?

[allthesignals]

I will want to check that it doesn't produce the edge case I was describing earlier. I'm also not totally sure it's worth the trade off in code complexity (is exhaustive-deps even enforced by Login.gov lint rules?)

@eileen-nava obvi it's your call on updating it or not! I know this PR has had a lot of feedback since you opened it.

[eileen-nava]

To second what Matt is saying, if I add foundAddress to the dependency array without adding back in some sort of conditional logic, it causes the address to update before the locations update. You can see this bug in the “before video” for scenario 2.

I'm not against adding the conditional logic back in and re-adding the foundAddress dependency to the dependency array. I’m mostly concerned about getting this PR approved and merged by sprint’s end.

[allthesignals]

Ticket created — @eileen-nava and I are going to look at it. Thanks Tim!

[eileen-nava]

I discussed this with Matt in Slack and I am also going to share here for posterity.

When I set the dependencies array to [locationResults, foundAddress, isLoading], I found a bug where the first search’s results would display before the second search’s results.

I poked through debugger and found that isLoading incorrectly equaled true before both the second search’s address candidates and usps locations had loaded. (I think this has to do with swr caching? More investigation is needed.) This caused the useDidUpdateEffect hook to incorrectly set the foundAddress and locationResults, even when there was a conditional like

    if (isLoading) {
      return;
    }

The plan is to
A) merge working code that doesn’t have exhaustive dependencies
B) make a ticket to satisfy the exhaustive dependencies requirement and investigate what’s going on with isLoading

[eileen-nava]

How about we split this into two effects, one for locationResults/onFoundLocations and a separate one for foundAddress/onFoundAddress?

When I tried that, it made the updated address display before the updated locations results. That contradicts the ticket's AC.

[allthesignals]

I think it's good to go but I think we should address @NavaTim's concerns in a separate ticket