🪟 🎉 Connection job list pagination #15938

lmossman · 2022-08-25T00:42:26Z

What

Resolves #10323

Adds pagination of jobs to the status view of a connection. On initial load, the most recent 25 jobs are shown, and when the user clicks the new Load more button below the job list, the page size is increased by 25 per click until there are no more jobs to fetch, at which point the Load more button is disabled.

How

This utilizes the already-existing pagination functionality available in the listJobs API endpoint, by increasing the page size sent in the API request each time Load more is clicked.

As explained in the issue linked above, this page uses auto-polling to refetch jobs every few seconds, so I needed to always keep the offset set to the default of 0 and only increase the page size that was being requested, since the request is continuously re-sent.

An analytics event is also fired whenever the Load more button is clicked, as specified in the ticket.

Here is a screen recording showing this functionality in action:
https://www.loom.com/share/44fde86ef61d48cc80552ff9d9a91700

🚨 User Impact 🚨

Users can now page through jobs in a connection, which will improve performance and usability especially for connections with many jobs

…ew data

lmossman · 2022-08-25T00:45:12Z

airbyte-webapp/src/services/job/JobService.tsx

    refetchInterval: 2500, // every 2,5 seconds,
+    keepPreviousData: true,


This was necessary to prevent the jobs from disappearing and the loading spinner showing every time Load more is clicked. With this keepPreviousData option set to true, it now seamlessly appends the new query results to the end once they are available. For more info see the official docs on this: https://react-query-v3.tanstack.com/guides/paginated-queries

This is a really cool solution for this.

timroes · 2022-08-25T10:20:07Z

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/StatusView.tsx

@@ -145,6 +170,11 @@ const StatusView: React.FC<StatusViewProps> = ({ connection }) => {
      >
        {jobs.length ? <JobsList jobs={jobs} /> : <EmptyResource text={<FormattedMessage id="sources.noSync" />} />}
      </ContentCard>
+      <footer className={styles.footer}>
+        <Button onClick={onLoadMoreJobs} disabled={jobs.length < jobPageSize}>


Two things:

I think it would be cool if we could show the button in a loading state while we're still loading the query. To do that we must use useQuery directly instead of our custom useSuspenseQuery wrapper in the JobService so we can get access to the underlying react-query properties. There is isPreviousValue which will be true while we're still loading the next data but show the previous, which we can use as an indicator that we're still loading.

In general I'd consider it good UX to not hide buttons but disable them if they are not available. For this specific case of paginating an "infinite list" I feel it might be more appropriate (and more commonly used as a pattern) to actually hide the button and not disable it, if there are no further items to paginate over. Since a disabled button might leave the question: "Why can't I load more jobs anymore?" and looks more like an issue than "the end of the list".

@timroes thanks for the suggestions, both make sense to me!

I have pushed up the changes to accomplish this, and I think it looks cleaner now.

I recorded a new video to demonstrate how it looks now, and to also show one small quirk of this implementation (I lowered the page size increment from 25 to 2 for this demonstration to make it a bit easier to show the behavior, but I've kept it as 25 in the PR):
https://www.loom.com/share/edc11614687b4f4f8b60f348abb982d4

Notice that at the end of the video, I click the Load more button, which causes it to load, but then it disappears. This is because the total number of jobs for this connection is a multiple of the page size increment (2 in that demonstration). So when all jobs are loaded, the logic of moreJobPagesAvailable = jobs && jobs.length === jobPageSize evaluates to true, because our code can only make an assumption about there being more jobs available based on the fact that we displaying the full page size number of jobs.

It isn't until we click the Load more button once more that our code can understand that the page size is higher than the number of jobs being returned by the API, so we can assume there are no more jobs to load.

In production, I doubt anyone will run into this case very often, as it would require that the number of jobs for their connection is an exact multiple of 25. And regardless, I don't think the UX for this is all that confusing, since the user clicks Load more, it loads, and the button disappears, indicating that there are no more jobs to show.

Sorry if what I said above was super verbose or confusing - I just wanted to confirm that that edge case is acceptable behavior!

(also, the style of the loading state of the button in that loom is a bit different than in the current state of the PR, since I just now realized we have a LoadingButton component. I have switched to using that instead of inserting a spinner into the button conditionally)

In production, I doubt anyone will run into this case very often, as it would require that the number of jobs for their connection is an exact multiple of 25. And regardless, I don't think the UX for this is all that confusing, since the user clicks Load more, it loads, and the button disappears, indicating that there are no more jobs to show.

I thought about that as well when I suggested that and feel it's an "okay" behavior int his case. Given we'll still do the request we're shortly in a loading state and then hide the button, which feels like users get the idea that there is nothing more to add. If we'd want to solve that really well, we'd need to adjust the API to also return us the totalCount as part of the response (which shouldn't be too expensive to get via SQL directly), so we can basically use this as a boundary.

Yep exactly. I think this solution is good enough for now, but if we see complaints about this we can do the totalCount solution at that time

timroes · 2022-08-25T10:33:52Z

@lmossman is the backend issue for this already fixed? Since we can only merge this once the BE issue is solved (since otherwise the check to disable/hide the button won't work properly).

lmossman · 2022-08-25T14:34:32Z

@timroes yes it is, here was the PR: #15908

I wanted to solve that issue in a separate PR so that it was easier for backend folks to review in an isolated way

…he button

timroes

Some more nitpicks and comments. Other than that functionality looks great :)

timroes · 2022-08-26T14:04:13Z

...e-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/StatusView.module.scss

+
+  .footer {
+    width: 100%;
+    height: 50px;


🎨 Minor nitpick: Usually we should try to avoid setting explicit heights. This comes with the problem that if any styling of elements changes inside we could end up overflowing this height at a later point. Usually in all cases you'd want to set a height to a container, you'll likely just want to apply padding or margin instead, to give spacing to the surrounding elements (or its contents in case of padding).

That makes sense! Removed

timroes · 2022-08-26T14:08:14Z

airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/components/StatusView.tsx

      </ContentCard>
+      <footer className={styles.footer}>
+        {(moreJobPagesAvailable || isJobPageLoading) && (


🎨 Given that the footer itself has some dimension (atm because of the height, if that's changed likely still because of the margin/padding), I'd suggest to also hide that if the button isn't shown, so we don't leave unnecessary "white-space" on the page.

timroes · 2022-08-26T14:10:46Z

airbyte-webapp/src/services/job/JobService.tsx

    refetchInterval: 2500, // every 2,5 seconds,
-  }).jobs;
+    keepPreviousData: true,


We'd now want to pass the suspense: true here, that the useSuspenseQuery usually set. This will cause React to not render the component (beyond the point of this method call) as long as the initial data is still loaded, and instead show the fallback element of the React.Suspense next up in the React tree.

timroes

Code LGTM. tested locally on Chrome Linux. works as expected.

* add load more jobs button * add pagination to query key * add keepPreviousData: true to prevent loading spinner when fetching new data * show loading spinner when loading more jobs * set page size increment back to 25 * extend input html attributes instead of adding className explicitly * use LoadingButton instead of inserting a spinner conditionally into the button * use suspense: true * get rid of unnecessary undefined * remove footer height and hide if no more jobs to load

lmossman added 3 commits August 24, 2022 16:22

add load more jobs button

8b54ef1

add pagination to query key

5d96907

add keepPreviousData: true to prevent loading spinner when fetching n…

93384e0

…ew data

github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Aug 25, 2022

lmossman commented Aug 25, 2022

View reviewed changes

Merge branch 'master' into lmossman/paginate-job-list

ae02669

lmossman marked this pull request as ready for review August 25, 2022 00:45

lmossman requested a review from a team as a code owner August 25, 2022 00:45

timroes reviewed Aug 25, 2022

View reviewed changes

lmossman added 3 commits August 25, 2022 12:40

show loading spinner when loading more jobs

ddb4171

set page size increment back to 25

4a9a323

Merge branch 'master' into lmossman/paginate-job-list

370d056

lmossman requested a review from timroes August 25, 2022 19:51

lmossman added 2 commits August 25, 2022 14:44

extend input html attributes instead of adding className explicitly

42e70ad

use LoadingButton instead of inserting a spinner conditionally into t…

62671d6

…he button

timroes reviewed Aug 26, 2022

View reviewed changes

lmossman added 4 commits August 26, 2022 10:25

use suspense: true

6e1c4f2

Merge branch 'master' into lmossman/paginate-job-list

9eb7084

get rid of unnecessary undefined

34fc82c

remove footer height and hide if no more jobs to load

1cbe502

lmossman requested a review from timroes August 26, 2022 21:38

lmossman added 2 commits August 29, 2022 11:55

Merge branch 'master' into lmossman/paginate-job-list

98f91ac

Merge branch 'master' into lmossman/paginate-job-list

527ed6f

timroes approved these changes Aug 30, 2022

View reviewed changes

lmossman merged commit 20a5b98 into master Aug 30, 2022

lmossman deleted the lmossman/paginate-job-list branch August 30, 2022 21:54

octavia-squidington-iii mentioned this pull request Sep 2, 2022

Bump Airbyte version from 0.40.3 to 0.40.4 #16275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🎉 Connection job list pagination #15938

🪟 🎉 Connection job list pagination #15938

lmossman commented Aug 25, 2022 •

edited

Loading

lmossman Aug 25, 2022

timroes Aug 25, 2022

timroes Aug 25, 2022

lmossman Aug 25, 2022

lmossman Aug 25, 2022

timroes Aug 26, 2022

lmossman Aug 26, 2022

timroes commented Aug 25, 2022

lmossman commented Aug 25, 2022

timroes left a comment

timroes Aug 26, 2022

lmossman Aug 26, 2022

timroes Aug 26, 2022

lmossman Aug 26, 2022

timroes Aug 26, 2022

lmossman Aug 26, 2022

timroes left a comment

		refetchInterval: 2500, // every 2,5 seconds,
		keepPreviousData: true,

🪟 🎉 Connection job list pagination #15938

🪟 🎉 Connection job list pagination #15938

Conversation

lmossman commented Aug 25, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes commented Aug 25, 2022

lmossman commented Aug 25, 2022

timroes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

lmossman commented Aug 25, 2022 •

edited

Loading