Better use Suspense to get a spinner when data is loading

julienw commented 2 months ago

This is ready for review !

~~Commit 1 is #708.~~ Commit 2 is the thing that you did in other PRs: remove defer Commit 3 adds a key like mentioned in the comment below. Hopefully the comments in the code should be clear but tell me if we need more. Commit 4 moves the results table header out of the Suspend area, so that it's visible while the data loads. Commit 5 is a small fix for the types returned by the loaders. Commit 6 fixes the DownloadButton after commit 4: because it's now outside of the Suspense/Await, it gets a promise in the normal results page, but the non-promise in the subtests page. This commit makes it work in both cases.

netlify[bot] commented 2 months ago

Deploy Preview for mozilla-perfcompare ready!

Name	Link
Latest commit	197d4b3ed661ab0e2eb643288d9312f5f6e8f967
Latest deploy log	https://app.netlify.com/sites/mozilla-perfcompare/deploys/66b5ee3ab13acf0008da0c1c
Deploy Preview	https://deploy-preview-702--mozilla-perfcompare.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.

julienw commented 2 months ago

The tests are not passing but I won't have time to fix them today. Just wanted to push that out to get feedback from y'all.

Carla-Moz commented 2 months ago

    Received
           1
            - [Error: Error when requesting treeherder: Treeherder request error]
            + [Error: Uncaught [TypeError: Cannot read properties of undefined (reading 'id')]],
           2: [TypeError: Cannot read properties of undefined (reading 'id')]
           3
            - [Error: Error when requesting treeherder: Treeherder request error]
            + [Error: Uncaught [TypeError: Cannot read properties of undefined (reading 'id')]],

I got this error too when I removed the 'defer' in my frameworks dropdown PR. But removing 'defer' allows the spinner to appear on framework change in my PR post rebase. Note, however, this error appeared before I rebased with your merged memoization PRs. The network calls are all green so I'm not sure what's missing in the tests as the error doesn't give enough details. :/

julienw commented 2 months ago

I ask because after rebasing and resolving your comments on my PR, the spinner appears on updates and framework updates.

Thanks to the memoizing of the revision info from PR #701, when only the framework changes, then the revision info fetches are near instant and therefore the loader promise is also near instant. Then the results fetch takes a significantly bigger amount of time to resolve, and therefore the spinner appears most of the time indeed, following my guess that it appears if the Awaited promise takes longer than the loader to resolve.

But it doesn't (always) appear when changing the revisions in edit mode, because in that case the revision info fetches take approximately the same time (not always) as the compare results fetches.

Tell me if something isn't clear for you. Working with a simpler project such as https://codepen.io/julienw/pen/MWMooEB helped me understand the behavior better too, take the time to play with it as well!

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.48%. Comparing base (0b5e066) to head (7218567).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## beta #702 +/- ## ======================================= Coverage 85.48% 85.48% ======================================= Files 83 83 Lines 2142 2143 +1 Branches 394 393 -1 ======================================= + Hits 1831 1832 +1 Misses 274 274 Partials 37 37 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

julienw commented 2 months ago

dang, by using the key that change at each render, we get back the problem that I solved in #700 grrr. I'll try to find a better key... or maybe more memoization

julienw commented 2 months ago

dang, by using the key that change at each render, we get back the problem that I solved in #700 grrr. I'll try to find a better key... or maybe more memoization

I solved that by using an incremented value returning from the loaders. This seems to work a lot better now.

mozilla / perfcompare