Batch asset invalidations #11937

somebody1234 · 2024-12-24T04:29:58Z

Pull Request Description

Close https://github.com/enso-org/cloud-v2/issues/1627
- Batch asset invalidations for:
  - Bulk deletes
  - Bulk undo deletes (restores)
  - Bulk copy/move
  - Bulk download
  - This avoids flickering when the directory list is invalidated multiple times (once for the mutation corresponding to each asset)

Codebase changes:

Remove all AssetEvents and AssetListEvents. Remaining events have been moved to TanStack Query mutations in this PR, as it is neccessary for batch invalidation functionality.
Remove key and directoryKey from AssetTreeNode, and keys in general in favor of ids. Not strictly necessary, but it was causing logic issues and is (IMO) a nice simplification to be able to do.

Important Notes

None

Testing instructions

Ideally test all asset list interactions, especially ones that fire mutations:
- Copy, delete, undelete
- Create assets (optional)
- Add labels (via drag)
- Download

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

~~The documentation has been updated, if necessary.~~
~~Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.~
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
~~Unit tests have been written where possible.~~
If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
or the Snowflake database integration, a run of the Extra Tests has been scheduled.
- ~~If applicable, it is suggested to paste a link to a successful run of the Extra Tests.~~

…tion to the backend is in flight

…ation`

…PasteHooks`

somebody1234 · 2024-12-30T08:55:34Z

each asset get rerendered on each refetch

would you happen to know how we managed to avoid that previously?

somebody1234 · 2024-12-30T08:56:35Z

anyway @MrFlashAccount ready for re-review i guess. lmk if there are specific things i should split out

app/gui/src/dashboard/components/dashboard/column/LabelsColumn.tsx

MrFlashAccount · 2024-12-30T15:40:33Z

app/gui/src/dashboard/hooks/backendHooks.tsx

-      return result
-    }
-  }, [listUserGroupsQuery.data, listUsersQuery.data])
+  const listUserGroupsQuery = useBackendQuery(backend, 'listUserGroups', EMPTY_ARRAY)


FWIW, using queries like this should be discouraged as it throws away the state of the query - loading, fetching, error, etc. instead, we should fetch on components level and if we want to hide some logic behind an abstraction - introduce a hook/function that accepts data as parameters

MrFlashAccount

Still not final yet, tired of reading changes like [] -> EMPTY_ARRAY

app/gui/src/dashboard/hooks/backendHooks.tsx

MrFlashAccount · 2024-12-30T15:46:12Z

app/gui/src/dashboard/hooks/backendHooks.tsx

+}
+
+/** Clear the trash folder. */
+export function useClearTrashMutation(backend: Backend) {


Maybe we can unify this hook with removing a group of items, a single item, by passing the ids into mutation params?

how's that different to the current code?

i think this has the same "wrap mutation into a mutation" issue btw - but i'm not sure how to fix it because we potentially need to do a fetch for this one

how's that different to the current code?

The difference is in queryClient.ensureQueryData. This hook implicitly get the ids to remove, but I'm proposing an explicit way.

MrFlashAccount · 2024-12-30T15:48:03Z

app/gui/src/dashboard/hooks/backendHooks.tsx

+}
+
+/** Duplicate a specific version of a project. */
+export function useDuplicateProjectMutation(backend: Backend) {


Please ask Pawel if we need to specify a name for duplicating assest (now we don't need to pass a name when we create them), but this makes sense for the LocalBackend.

MrFlashAccount · 2024-12-30T15:56:41Z

app/gui/src/dashboard/hooks/backendHooks.tsx

+}
+
+/** Call "add label" mutations for a list of assets. */
+export function addAssetsLabelsMutationOptions(backend: Backend) {


This makes me wondering, why somewhere we use useMutation, in other places we use mutationOptions?

ideally everything should be mutationOptions imo, but some of them need to use hooks so that is not possible as is

Why we can't pass all the needed as args?

fair enough, what about useRemoveSelfPermissionMutation? which needs to wrap another mutation (both input shape and the list of invalidates)

Let's keep it as is, eventually we'll remove it: https://github.com/enso-org/cloud-v2/issues/1645

there are also the useNew* which depend on a lot of hooks. i think it's fine for now as well (because they already exist) but should also be discussed in a new issue what we're planning on doing with those

app/gui/src/dashboard/hooks/backendHooks.tsx

app/gui/src/dashboard/layouts/AssetVersions/AssetVersion.tsx

app/gui/src/dashboard/layouts/Settings/ActivityLogSettingsSection.tsx

app/gui/src/dashboard/layouts/Settings/MembersTable.tsx

app/gui/src/dashboard/components/dashboard/column/LabelsColumn.tsx

…ion`

…utation`

…tions`

somebody1234 added 30 commits December 16, 2024 21:37

Formatting

139613e

Batch deleteAsset calls

3bef491

Cancel refetching directories naively by skipping refetch if any muta…

f48ccde

…tion to the backend is in flight

Add invalidations to batched delete mutation

a1e2eb5

Fail batched delete mutation if any requests fail

3c3bfc5

Get rid of AssetEventType.delete

79af793

Batched restore mutation and remove AssetEventType.restore

defc5c7

Replace AssetEventType.deleteForever with calls to `deleteAssetsMut…

8a9f8b5

…ation`

Replace AssetEventType.removeSelf with hook

6992664

Remove obsolete AssetListEventType.emptyTrash

01867ff

Replace AssetListEventType.copy with hook

b14a1b3

Replace AssetListEventType.move with hook

4a38163

Fix transferBetweenCategories

0f8514f

Remove AssetListEventType.delete

b33fd6f

Replace AssetListEventType.clearTrash with hook

16f36af

Replace AssetListEventType.duplicateProject with hook

1ce159c

Remove AssetListEvents completely; move useCutAndPaste to `cutAnd…

587f976

…PasteHooks`

Formatting

8aaa6b0

Remove some section headers

a6487f8

Remove move and delete events and callbacks

1210d7c

Formatting

383289d

Replace AssetEventType.download with hook

298686c

Simplify setCanDownload subscriber code

5207e8a

Formatting

1dea1d6

WIP: Include more selection info to bring back downloadSelected

354e025

Fix type errors

7588040

Replace AssetEventType.downladSelected with hook

dc1daf2

Add hooks to add and remove labels

1288aa2

Add invalidations to label mutation hooks

0a2791e

Remove unused addLabels and removeLabels events

156479a

Fix bad merge

f95a5fc

MrFlashAccount reviewed Dec 30, 2024

View reviewed changes

app/gui/src/dashboard/components/dashboard/column/LabelsColumn.tsx Outdated Show resolved Hide resolved

MrFlashAccount reviewed Dec 30, 2024

View reviewed changes

MrFlashAccount requested changes Dec 30, 2024

View reviewed changes

MrFlashAccount reviewed Dec 30, 2024

View reviewed changes

app/gui/src/dashboard/components/dashboard/column/LabelsColumn.tsx Outdated Show resolved Hide resolved

MrFlashAccount reviewed Dec 30, 2024

View reviewed changes

app/gui/src/dashboard/components/dashboard/column/LabelsColumn.tsx Show resolved Hide resolved

somebody1234 added 3 commits January 2, 2025 18:42

Merge branch 'develop' into wip/sb/batch-invalidations

0aa1f9e

Fix errors

25ce57d

Remove overzealous use of EMPTY_ARRAY

a1d8517

somebody1234 force-pushed the wip/sb/batch-invalidations branch from b0a3b11 to a1d8517 Compare January 3, 2025 09:15

somebody1234 added 17 commits January 3, 2025 19:23

Address some CRs

78886e3

Un-lift directories query subscriber

53c9ade

Address more CRs

59ca5bc

Split backendHooks

c830ea6

Rename backendHooks

bd824ca

Fix errors

cd3417b

Fix errors

0a6fdf5

Merge branch 'develop' into wip/sb/batch-invalidations

8b199f6

Move download logic to RemoteBackend and LocalBackend

2208ae4

Avoid wrapping mutation in mutation for `useRemoveSelfPermissionMutat…

938337d

…ion`

Refactor useDownloadAssetsMutation to downloadAssetsMutationOptions

03297f9

Refactor useUploadFileMutation to return a proper mutation result

c828375

Return UseQueryResult from useListUserGroupsWithUsers

ae968e5

Fix errors

e9de7af

Fix logic error

6f28ad4

Remove clearTrashMutation instead of ensureQuery + `deleteAssetsM…

4b9f8fd

…utation`

Refactor useDuplicateProjectMutation to `duplicateProjectMutationOp…

46c57e5

…tions`

somebody1234 requested a review from MrFlashAccount January 6, 2025 11:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch asset invalidations #11937

Batch asset invalidations #11937

somebody1234 commented Dec 24, 2024 •

edited

Loading

somebody1234 commented Dec 30, 2024

somebody1234 commented Dec 30, 2024 •

edited

Loading

MrFlashAccount Dec 30, 2024

MrFlashAccount left a comment

MrFlashAccount Dec 30, 2024

somebody1234 Jan 3, 2025

somebody1234 Jan 3, 2025

MrFlashAccount Jan 6, 2025

MrFlashAccount Dec 30, 2024 •

edited

Loading

MrFlashAccount Dec 30, 2024

somebody1234 Jan 3, 2025

MrFlashAccount Jan 6, 2025

somebody1234 Jan 6, 2025

MrFlashAccount Jan 6, 2025

somebody1234 Jan 6, 2025

Batch asset invalidations #11937

Are you sure you want to change the base?

Batch asset invalidations #11937

Conversation

somebody1234 commented Dec 24, 2024 • edited Loading

Pull Request Description

Important Notes

Testing instructions

Checklist

somebody1234 commented Dec 30, 2024

somebody1234 commented Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

MrFlashAccount 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

MrFlashAccount Dec 30, 2024 • edited Loading

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

somebody1234 commented Dec 24, 2024 •

edited

Loading

somebody1234 commented Dec 30, 2024 •

edited

Loading

MrFlashAccount Dec 30, 2024 •

edited

Loading