google-listings-and-ads icon indicating copy to clipboard operation
google-listings-and-ads copied to clipboard

Investigate slow requests to ads/accounts

Open joemcgill opened this issue 1 year ago • 3 comments

During a review of #2653, @mikkamp made the following observation about slow requests to ads/accounts:

I noticed that loading the account information step is rather slow (in particular for me because I have a large amount of linked test accounts). On one of the requests to ads/accounts the request timed out. This timed out request is not visible but it leaves the user in a state where they can not reconnect to a new ads account.

It might not be vital to address since in this scenario I did already have an ads account connected so I could still proceed with the connected account, also a refresh cleared it up and the call didn't timeout the second time. However I just wanted to mention the behaviour.

Acceptance Criteria

  • [ ] These requests are not causing the UI to load slowly when there are many linked accounts (if possible).
  • [ ] Add error handling for when the request times out.

joemcgill avatar Nov 14 '24 03:11 joemcgill

After a bit of investigation, I'm not convinced that the current implementation is considerably worse than the previous in terms of handling slow responses to get existing accounts. Currently, the GoogleAdsAccountCard will show a spinner until the API response resolves. If the response errors, the spinner persists:

Image

Now a spinner is shown on the combo card until the existing accounts can be resolved. If the ads accounts error during resolution then only the MC connection card is displayed since the existing Ads accounts cannot be displayed:

Image

In both cases, if there is an error, a toast error message is shown that describes the problem that has occurred.

Just to be sure, I've investigated how the API requests are triggered to make sure we're not delaying the requests in some way (e.g., requesting existing MC accounts and Ads accounts in parallel, etc.) and couldn't find any clear opportunities for improvement.

joemcgill avatar Nov 18 '24 02:11 joemcgill

That's a great point, that there were circumstances where a failing call would continue to show a spinner.

One of the differences I was also trying to highlight was that if all the accounts are connected, then we used to just show everything as connected, without fetching all account details:

Image

Only if we decided to connect another account would it fetch all accounts so a different one could be connected.

But now even if we are connected it still makes the request to fetch all the accounts, both from MC and Ads accounts. So the connected screen is slower to load and I have to wait for it to complete: Image

mikkamp avatar Nov 18 '24 13:11 mikkamp

But now even if we are connected it still makes the request to fetch all the accounts, both from MC and Ads accounts. So the connected screen is slower to load and I have to wait for it to complete

Interesting! Yes, I guess that use case would a bit different now. With the new design, we need to know if there are existing accounts in order to determine which card actions to show, i.e., whether we should show an edit button to show the individual connected accounts and the UI to switch, or whether we should just show the button to switch Google accounts, etc. I'm unsure how we can avoid this extra request without a big refactor of how the combo card is constructed at this stage.

joemcgill avatar Nov 18 '24 23:11 joemcgill

We were not able to determine that there was anything actionable here, so I'm closing for now. We can consider reopening if this problem resurfaces.

joemcgill avatar May 27 '25 13:05 joemcgill