Fetch item/total counts separate

[rijkvanzanten]

Description

Every time the item parameters changed in use-items, the total_count and filter_count were fetched separately. This causes some pretty nasty performance regressions in browsing the app, as you'd have to wait for the counts even if you're doing actions that don't change the counts. This PR resolves that by fetching the total / filtered counts asynchronously, and only updating the counts when needed.

Fixes #14796, fixes ENG-214, fixes ENG-75

Type of Change

• Bugfix
• Improvement
• New Feature
• Refactor / codestyle updates
• Other, please describe:

Requirements Checklist

• New / updated tests are included
• All tests are passing locally
• Performed a self-review of the submitted code

If adding a new feature:

• Documentation was added/updated. PR:

[br41nslug]

This does prevent the total count from being updated in the app, in the case where it's updated on the backend, until you've opened another page but that's such a small edge case i doubt most will notice.
On a huge table this makes the App a lot more responsive especially noticeable when searching 🙌

[azrikahar]

Review

FeelsGTM! But I do notice when we're doing batch deletion, the count isn't being updated now.

Update: same issue applies for batch archiving.

• Before this PR (does still update after batch delete)

  chrome_SU4yjxEEOS.mp4

• After this PR (doesn't update anymore after batch delete)

  chrome_PqjYIeF0oA.mp4

This is because whenever a layout gets refreshed (such as after batch deletion), refresh() does call getItems() again, but "deleting item(s)" doesn't prompt it to re-fetch the counts:

directus/app/src/layouts/tabular/index.ts

Lines 123 to 125 in cb05cc2

	function refresh() {
	getItems();
	}

As a quick test, I can confirm exposing and adding getTotalCount + getFilterCount into this refresh() function does seem to resolve this, but there might be a better approach.

Additional Thoughts

itemCount ref naming

This isn't really related to the changes in this PR, but wondering can/should we rename itemCount ref to filterCount? 🤔

Additional aggregate count API calls in Export sidebar

I've also noticed that the getItemCount here in export-sidebar-detail is also firing whenever we navigate to any collection without opening the Export Items drawer, and whenever we add/remove headers in table layout:

directus/app/src/views/private/components/export-sidebar-detail.vue

Lines 343 to 372 in cb05cc2

	const getItemCount = debounce(async () => {
	itemCountLoading.value = true;
	try {
	const count = await api
	.get(getEndpoint(collection.value), {
	params: {
	...exportSettings,
	aggregate: {
	count: ['*'],
	},
	},
	})
	.then((response) => {
	if (response.data.data?.[0]?.count) {
	return Number(response.data.data[0].count);
	}
	});
	itemCount.value = count;
	} finally {
	itemCountLoading.value = false;
	}
	}, 250);
	getItemCount();
	watch(exportSettings, () => {
	getItemCount();
	});

Wondering about the following points:

• Can it be optimized to only be fetched when the drawer is opened? (but then "Exporting all X items" notice may require a loading state for the number of items)

• Would ...exportSettings in the params be slightly overkill? I believe fields and sort for example is already "cleaned" in the API side when aggregating, so perhaps we can avoid passing it in the request in the first place to make it slightly cleaner.

  directus/api/src/utils/get-ast-from-query.ts

  Lines 53 to 59 in 8f5a072

  	/**
  	* When using aggregate functions, you can't have any other regular fields
  	* selected. This makes sure you never end up in a non-aggregate fields selection error
  	*/
  	if (Object.keys(query.aggregate || {}).length > 0) {
  	fields = [];
  	}

  directus/api/src/utils/get-ast-from-query.ts

  Lines 94 to 98 in 8f5a072

  	// When no group by is supplied, but an aggregate function is used, only a single row will be
  	// returned. In those cases, we'll ignore the sort field altogether
  	if (query.aggregate && Object.keys(query.aggregate).length && !query.group?.[0]) {
  	delete query.sort;
  	}

• seems like it is using .then here to get the count compared to approach in this PR, while both does achieve the same result, maybe we should make them consistent, depending on which approach is preferred

but we can also move this export-sidebar-detail tangent to a separate discussion/PR if needed.

[azrikahar]

Additional note: Took a stab at adding test for this, but had to resort to installing @vue/test-utils in shared package to use flushPromises so that the computed values are updated, not sure if there's a better way.

I suspect the reason that they're not updating without flushPromises, has something to do with the usage of lodash throttle() when fetching items, but using vi.useFakeTimers() and advancing the time past the throttle duration doesn't seem to update the computed values either from what I've tried.

Mocking throttle() directly as vi.fn().mockImplementation(fn => fn) also isn't really limiting the number of requests either so the test case will be inaccurate, but maybe it's also somewhat related to fetchOnInit which seems to be fetching it multiple times and I'm not sure if that's intended, as discussed in #16290 (reply in thread).

[br41nslug]

But I do notice when we're doing batch deletion, the count isn't being updated now.

Azri brings up a good point when batch deleting (and probably archiving) having the counter off on smaller tables feels like it has a bigger impact than on huge tables. We should make sure that the total counter gets updated when these actions are used.

Refetching is the quick solution here but for a more performance oriented fix we could fix this on the frontend because we know the total count and amount to be removed. (This is however easier said than done in the current structure)

[rijkvanzanten]

@azrikahar I agree with all your points, but I feel like most of them are optimizations that are out of scope for this fix in the current setup 😬 I'd rather create a new issue to collect these refactor points, and then do spend a cycle on cleaning it up (and adding the proper tests!) 🙂