From b1cc4f1b56f3b1f65f85eed0d0306d51d77fd994 Mon Sep 17 00:00:00 2001 From: Sergei Garin Date: Wed, 15 Jan 2025 23:14:57 +0300 Subject: [PATCH] Remove listGroups while building categories (#11988) This PR adds new field called `groups` on the User object to avoid fetching the usergroups list to get the names of the groups. Also this PR removes the `` --- .github/workflows/gui-changed-files.yml | 1 - app/common/src/services/Backend.ts | 24 +- app/gui/playwright.config.ts | 7 +- .../dashboard/authentication/cognito.mock.ts | 6 +- app/gui/src/dashboard/components/Activity.tsx | 125 ++++++ .../Form/components/FormError.tsx | 9 +- app/gui/src/dashboard/components/Await.tsx | 27 +- .../components/__tests__/Activity.test.tsx | 95 ++++ .../dashboard/column/SharedWithColumn.tsx | 48 +- app/gui/src/dashboard/hooks/backendHooks.ts | 12 +- .../hooks/backendUploadFilesHooks.tsx | 3 +- .../dashboard/layouts/AssetContextMenu.tsx | 33 +- app/gui/src/dashboard/layouts/AssetsTable.tsx | 6 +- .../dashboard/layouts/CategorySwitcher.tsx | 6 +- .../layouts/Drive/Categories/Category.ts | 4 +- .../Drive/Categories/categoriesHooks.tsx | 64 +-- app/gui/src/dashboard/layouts/UserBar.tsx | 18 +- app/gui/src/dashboard/layouts/UserMenu.tsx | 10 +- .../modals/ManagePermissionsModal.tsx | 425 ------------------ .../SetupOrganizationAfterSubscribe.tsx | 101 +++-- .../authentication/AuthenticationPage.tsx | 2 +- .../dashboard/pages/authentication/Login.tsx | 16 +- .../dashboard/pages/dashboard/Dashboard.tsx | 44 +- .../dashboard/providers/SessionProvider.tsx | 53 ++- .../src/dashboard/utilities/permissions.ts | 8 +- 25 files changed, 429 insertions(+), 718 deletions(-) create mode 100644 app/gui/src/dashboard/components/Activity.tsx create mode 100644 app/gui/src/dashboard/components/__tests__/Activity.test.tsx delete mode 100644 app/gui/src/dashboard/modals/ManagePermissionsModal.tsx diff --git a/.github/workflows/gui-changed-files.yml b/.github/workflows/gui-changed-files.yml index cc6d950115cb..917ff4cbb2f8 100644 --- a/.github/workflows/gui-changed-files.yml +++ b/.github/workflows/gui-changed-files.yml @@ -39,7 +39,6 @@ jobs: .github/workflows/gui* .github/workflows/storybook.yml files_ignore: | - app/ide-desktop/** app/gui/scripts/** app/gui/.gitignore .git-* diff --git a/app/common/src/services/Backend.ts b/app/common/src/services/Backend.ts index a8fb6ba6ae09..aa7c6693a9c1 100644 --- a/app/common/src/services/Backend.ts +++ b/app/common/src/services/Backend.ts @@ -196,9 +196,25 @@ export interface User extends UserInfo { readonly isOrganizationAdmin: boolean readonly rootDirectoryId: DirectoryId readonly profilePicture?: HttpsUrl + /** + * Contains the IDs of the user groups that the user is a member of. + * @deprecated Use `groups` instead. + */ readonly userGroups: readonly UserGroupId[] | null readonly removeAt?: dateTime.Rfc3339DateTime | null readonly plan?: Plan | undefined + /** + * Contains the user groups that the user is a member of. + * Has enriched metadata, like the name of the group and the home directory ID. + */ + readonly groups?: readonly UserGroup[] +} + +/** A user related to the current user. */ +export interface UserGroup { + readonly id: UserGroupId + readonly name: string + readonly homeDirectoryId: DirectoryId } /** A `Directory` returned by `createDirectory`. */ @@ -488,7 +504,7 @@ export interface UserPermission { /** User permission for a specific user group. */ export interface UserGroupPermission { - readonly userGroup: UserGroupInfo + readonly userGroup: UserGroup readonly permission: permissions.PermissionAction } @@ -544,7 +560,7 @@ export function isUserGroupPermissionAnd(predicate: (permission: UserGroupPermis /** Get the property representing the name on an arbitrary variant of {@link UserPermission}. */ export function getAssetPermissionName(permission: AssetPermission) { - return isUserPermission(permission) ? permission.user.name : permission.userGroup.groupName + return isUserPermission(permission) ? permission.user.name : permission.userGroup.name } /** Get the property representing the id on an arbitrary variant of {@link UserPermission}. */ @@ -1170,8 +1186,8 @@ export function compareAssetPermissions(a: AssetPermission, b: AssetPermission) } else { // NOTE [NP]: Although `userId` is unique, and therefore sufficient to sort permissions, sort // name first, so that it's easier to find a permission in a long list (i.e., for readability). - const aName = 'user' in a ? a.user.name : a.userGroup.groupName - const bName = 'user' in b ? b.user.name : b.userGroup.groupName + const aName = 'user' in a ? a.user.name : a.userGroup.name + const bName = 'user' in b ? b.user.name : b.userGroup.name const aUserId = 'user' in a ? a.user.userId : a.userGroup.id const bUserId = 'user' in b ? b.user.userId : b.userGroup.id return ( diff --git a/app/gui/playwright.config.ts b/app/gui/playwright.config.ts index ed3f7593831e..34172d02c566 100644 --- a/app/gui/playwright.config.ts +++ b/app/gui/playwright.config.ts @@ -13,7 +13,7 @@ import url from 'node:url' const DEBUG = process.env.DEBUG_TEST === 'true' const isCI = process.env.CI === 'true' - +const isProd = process.env.PROD === 'true' const TIMEOUT_MS = DEBUG ? 100_000_000 : 25_000 // We tend to use less CPU on CI to reduce the number of failures due to timeouts. @@ -162,7 +162,10 @@ export default defineConfig({ reuseExistingServer: false, }, { - command: `corepack pnpm exec vite -c vite.test.config.ts build && vite -c vite.test.config.ts preview --port ${ports.dashboard} --strictPort`, + command: + isCI || isProd ? + `corepack pnpm exec vite -c vite.test.config.ts build && vite -c vite.test.config.ts preview --port ${ports.dashboard} --strictPort` + : `NODE_ENV=test corepack pnpm exec vite -c vite.test.config.ts --port ${ports.dashboard}`, timeout: 240 * 1000, port: ports.dashboard, reuseExistingServer: false, diff --git a/app/gui/src/dashboard/authentication/cognito.mock.ts b/app/gui/src/dashboard/authentication/cognito.mock.ts index 0e47b0549487..540932985f4d 100644 --- a/app/gui/src/dashboard/authentication/cognito.mock.ts +++ b/app/gui/src/dashboard/authentication/cognito.mock.ts @@ -229,7 +229,7 @@ export class Cognito { localStorage.setItem(MOCK_EMAIL_KEY, username) const result = await results.Result.wrapAsync(async () => { listen.authEventListener?.(listen.AuthEvent.signIn) - await Promise.resolve() + return Promise.resolve(await this.userSession()) }) return result .mapErr(original.intoAmplifyErrorOrThrow) @@ -238,8 +238,10 @@ export class Cognito { /** Sign out the current user. */ async signOut() { - listen.authEventListener?.(listen.AuthEvent.signOut) this.isSignedIn = false + listen.authEventListener?.(listen.AuthEvent.signOut) + localStorage.removeItem(MOCK_EMAIL_KEY) + localStorage.removeItem(MOCK_ORGANIZATION_ID_KEY) return Promise.resolve(null) } diff --git a/app/gui/src/dashboard/components/Activity.tsx b/app/gui/src/dashboard/components/Activity.tsx new file mode 100644 index 000000000000..95537916b694 --- /dev/null +++ b/app/gui/src/dashboard/components/Activity.tsx @@ -0,0 +1,125 @@ +/** + * @file Activity component + * + * This component is used to suspend the rendering of a subtree until a promise is resolved. + */ +import { unsafeWriteValue } from '#/utilities/write' +import { Suspense, useEffect, useLayoutEffect, useRef, useState } from 'react' +import { useAwait } from './Await' + +/** + * Props for {@link Activity} + */ +export interface ActivityProps { + /** + * The mode of the activity. + * - `active`: The subtree is active (default). + * - `inactive`: The activity of the subtree is paused. + * - `inactive-hidden`: The activity of the subtree is paused, and the subtree is hidden. + * @default 'active' + */ + readonly mode: 'active' | 'inactive-hidden' | 'inactive' + readonly children: React.ReactNode +} + +/** + * A component that pauses all activity inside it's subtree. + * + * --- + * ## The component is EXPERIMENTAL, please use with caution. + * --- + */ +export function Activity(props: ActivityProps) { + const { mode, children } = props + + const contentRef = useRef(null) + + const [promise, setPromise] = useState | null>(null) + + const isActive = mode === 'active' + const fallback = + mode === 'inactive-hidden' ? null : + + useEffect(() => { + if (isActive) { + return + } + + let resolve = () => {} + + setPromise( + new Promise((res) => { + resolve = res + }), + ) + + return () => { + resolve() + setPromise(null) + } + }, [isActive]) + + return ( +
+ + {children} + +
+ ) +} + +/** + * Props for {@link ActivityInner} + */ +interface ActivityInnerProps { + readonly children: React.ReactNode + readonly promise?: Promise | null | undefined +} + +/** + * A component that suspends the tree using promises. + * @param props - The props of the component. + * @returns The children of the component. + */ +function ActivityInner(props: ActivityInnerProps) { + const { promise, children } = props + + // Suspend the subtree + useAwait(promise) + + return children +} + +/** + * Props for {@link UnhideSuspendedTree} + */ +interface UnhideSuspendedTreeProps { + readonly contentRef: React.RefObject +} + +/** + * Hack, that unhides the suspended tree. + */ +function UnhideSuspendedTree(props: UnhideSuspendedTreeProps) { + const { contentRef } = props + + useLayoutEffect(() => { + const element: HTMLDivElement | null = contentRef.current + + if (element == null) { + return + } + + const chidlren = element.childNodes + + for (let i = 0; i < chidlren.length; i++) { + const child = chidlren[i] + + if (child instanceof HTMLElement) { + unsafeWriteValue(child.style, 'display', '') + } + } + }, [contentRef]) + + return null +} diff --git a/app/gui/src/dashboard/components/AriaComponents/Form/components/FormError.tsx b/app/gui/src/dashboard/components/AriaComponents/Form/components/FormError.tsx index 9d3bbd006c26..de6677e7ee49 100644 --- a/app/gui/src/dashboard/components/AriaComponents/Form/components/FormError.tsx +++ b/app/gui/src/dashboard/components/AriaComponents/Form/components/FormError.tsx @@ -28,7 +28,14 @@ export function FormError(props: FormErrorProps) { const icon = error.type === 'offline' ? Offline : null return ( - + {error.message} diff --git a/app/gui/src/dashboard/components/Await.tsx b/app/gui/src/dashboard/components/Await.tsx index 288d428e30dd..4aaf4d961341 100644 --- a/app/gui/src/dashboard/components/Await.tsx +++ b/app/gui/src/dashboard/components/Await.tsx @@ -95,11 +95,34 @@ const PRIVATE_AWAIT_PROMISE_STATE = Symbol('PRIVATE_AWAIT_PROMISE_STATE_REF') * This component throws the promise and trigger the Suspense boundary * inside the {@link Await} component. * @throws {Promise} - The promise that is being awaited by Suspense. - * @throws {unknown} - The error that is being thrown by the promise. Triggers error boundary inside the {@link Await} component. */ function AwaitInternal(props: AwaitProps) { const { promise, children } = props + const data = useAwait(promise) + + return typeof children === 'function' ? children(data) : children +} + +export function useAwait(promise?: null): void +export function useAwait(promise: Promise): PromiseType +export function useAwait( + promise?: Promise | null, +): PromiseType | undefined + +/** + * A hook that accepts a promise and triggers the Suspense boundary until the promise is resolved. + * @param promise - The promise to await. + * @throws {Promise} - The promise that is being awaited by Suspense + * @returns The data of the promise. + */ +export function useAwait( + promise?: Promise | null, +): PromiseType | undefined { + if (promise == null) { + return + } + /** * Define the promise state on the promise. */ @@ -154,5 +177,5 @@ function AwaitInternal(props: AwaitProps) { throw promiseState.error } - return typeof children === 'function' ? children(promiseState.data) : children + return promiseState.data } diff --git a/app/gui/src/dashboard/components/__tests__/Activity.test.tsx b/app/gui/src/dashboard/components/__tests__/Activity.test.tsx new file mode 100644 index 000000000000..8e40e59d4044 --- /dev/null +++ b/app/gui/src/dashboard/components/__tests__/Activity.test.tsx @@ -0,0 +1,95 @@ +import { render, screen } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { useEffect, useState } from 'react' +import { describe } from 'vitest' +import { Activity } from '../Activity' + +describe('Activity', (it) => { + it('should render the children', ({ expect }) => { + render( + +
Hello
+ , + ) + + expect(screen.getByText('Hello')).toBeInTheDocument() + }) + + it('should render children when inactive', ({ expect }) => { + render( + +
Hello
+ , + ) + + expect(screen.getByText('Hello')).toBeInTheDocument() + }) + + it('should render children when inactive-hidden', ({ expect }) => { + render( + +
Hello
+ , + ) + + expect(screen.getByText('Hello')).toBeInTheDocument() + }) + + it('should display children when inactive', ({ expect }) => { + render( + +
Hello
+ , + ) + + expect(screen.getByText('Hello')).toBeVisible() + }) + + it('should not unmount children when inactive', async ({ expect }) => { + let count = 0 + + const Component = () => { + useEffect( + () => () => { + count++ + }, + [], + ) + + return
{count}
+ } + + function Container() { + const [mode, setMode] = useState<'active' | 'inactive'>('active') + + return ( +
+ + + + + +
+ ) + } + + render() + + await userEvent.click(screen.getByText('Inactive')) + await userEvent.click(screen.getByText('Active')) + + expect(count).toBe(0) + }) +}) diff --git a/app/gui/src/dashboard/components/dashboard/column/SharedWithColumn.tsx b/app/gui/src/dashboard/components/dashboard/column/SharedWithColumn.tsx index 4ab254f61e47..40940f76408a 100644 --- a/app/gui/src/dashboard/components/dashboard/column/SharedWithColumn.tsx +++ b/app/gui/src/dashboard/components/dashboard/column/SharedWithColumn.tsx @@ -1,16 +1,8 @@ /** @file A column listing the users with which this asset is shared. */ -import * as React from 'react' - -import Plus2Icon from '#/assets/plus2.svg' -import { Button } from '#/components/AriaComponents' import type { AssetColumnProps } from '#/components/dashboard/column' import PermissionDisplay from '#/components/dashboard/PermissionDisplay' -import { useRemoveSelfPermissionMutation } from '#/hooks/backendHooks' -import ManagePermissionsModal from '#/modals/ManagePermissionsModal' -import { useFullUserSession } from '#/providers/AuthProvider' -import { useSetModal } from '#/providers/ModalProvider' import { getAssetPermissionId, getAssetPermissionName } from '#/services/Backend' -import { PermissionAction, tryFindSelfPermission } from '#/utilities/permissions' +import { PermissionAction } from '#/utilities/permissions' // ======================== // === SharedWithColumn === @@ -30,21 +22,10 @@ interface SharedWithColumnPropsInternal extends Pick { /** A column listing the users with which this asset is shared. */ export default function SharedWithColumn(props: SharedWithColumnPropsInternal) { - const { item, state, isReadonly = false } = props - const { backend, category, setQuery } = state - - const { user } = useFullUserSession() - - const removeSelfPermissionMutation = useRemoveSelfPermissionMutation(backend) + const { item, state } = props + const { category, setQuery } = state const assetPermissions = item.permissions ?? [] - const { setModal } = useSetModal() - const self = tryFindSelfPermission(user, item.permissions) - const plusButtonRef = React.useRef(null) - const managesThisAsset = - !isReadonly && - category.type !== 'trash' && - (self?.permission === PermissionAction.own || self?.permission === PermissionAction.admin) return (
@@ -73,29 +54,6 @@ export default function SharedWithColumn(props: SharedWithColumnPropsInternal) { {getAssetPermissionName(other)} ))} - {managesThisAsset && ( -
) } diff --git a/app/gui/src/dashboard/hooks/backendHooks.ts b/app/gui/src/dashboard/hooks/backendHooks.ts index f6223944b2b4..0de2211283bb 100644 --- a/app/gui/src/dashboard/hooks/backendHooks.ts +++ b/app/gui/src/dashboard/hooks/backendHooks.ts @@ -638,7 +638,6 @@ export function useNewFolder(backend: Backend, category: Category) { const setSelectedAssets = useSetSelectedAssets() const { user } = useFullUserSession() const { data: users } = useBackendQuery(backend, 'listUsers', []) - const { data: userGroups } = useBackendQuery(backend, 'listUserGroups', []) const createDirectoryMutation = useMutation(backendMutationOptions(backend, 'createDirectory')) return useEventCallback(async (parentId: DirectoryId, parentPath: string | null | undefined) => { @@ -658,7 +657,7 @@ export function useNewFolder(backend: Backend, category: Category) { category, user, users ?? [], - userGroups ?? [], + user.groups ?? [], ), ) @@ -681,7 +680,6 @@ export function useNewProject(backend: Backend, category: Category) { const { user } = useFullUserSession() const { data: users } = useBackendQuery(backend, 'listUsers', []) - const { data: userGroups } = useBackendQuery(backend, 'listUserGroups', []) const createProjectMutation = useMutation(backendMutationOptions(backend, 'createProject')) return useEventCallback( @@ -721,7 +719,7 @@ export function useNewProject(backend: Backend, category: Category) { category, user, users ?? [], - userGroups ?? [], + user.groups ?? [], ), user, path, @@ -759,7 +757,6 @@ export function useNewSecret(backend: Backend, category: Category) { const toggleDirectoryExpansion = useToggleDirectoryExpansion() const { user } = useFullUserSession() const { data: users } = useBackendQuery(backend, 'listUsers', []) - const { data: userGroups } = useBackendQuery(backend, 'listUserGroups', []) const createSecretMutation = useMutation(backendMutationOptions(backend, 'createSecret')) return useEventCallback( @@ -778,7 +775,7 @@ export function useNewSecret(backend: Backend, category: Category) { category, user, users ?? [], - userGroups ?? [], + user.groups ?? [], ), ) @@ -798,7 +795,6 @@ export function useNewDatalink(backend: Backend, category: Category) { const toggleDirectoryExpansion = useToggleDirectoryExpansion() const { user } = useFullUserSession() const { data: users } = useBackendQuery(backend, 'listUsers', []) - const { data: userGroups } = useBackendQuery(backend, 'listUserGroups', []) const createDatalinkMutation = useMutation(backendMutationOptions(backend, 'createDatalink')) return useEventCallback( @@ -817,7 +813,7 @@ export function useNewDatalink(backend: Backend, category: Category) { category, user, users ?? [], - userGroups ?? [], + user.groups ?? [], ), ) diff --git a/app/gui/src/dashboard/hooks/backendUploadFilesHooks.tsx b/app/gui/src/dashboard/hooks/backendUploadFilesHooks.tsx index 3492ffdb5713..2cad2aaf36c6 100644 --- a/app/gui/src/dashboard/hooks/backendUploadFilesHooks.tsx +++ b/app/gui/src/dashboard/hooks/backendUploadFilesHooks.tsx @@ -61,7 +61,6 @@ export function useUploadFiles(backend: Backend, category: Category) { const { setModal } = useSetModal() const { user } = useFullUserSession() const { data: users } = useBackendQuery(backend, 'listUsers', []) - const { data: userGroups } = useBackendQuery(backend, 'listUserGroups', []) const uploadFileMutation = useUploadFileWithToastMutation(backend) const setSelectedAssets = useSetSelectedAssets() @@ -83,7 +82,7 @@ export function useUploadFiles(backend: Backend, category: Category) { category, user, users ?? [], - userGroups ?? [], + user.groups ?? [], ) const files = reversedFiles.filter(fileIsNotProject).map((file) => { const asset = createPlaceholderFileAsset( diff --git a/app/gui/src/dashboard/layouts/AssetContextMenu.tsx b/app/gui/src/dashboard/layouts/AssetContextMenu.tsx index e33c4012422c..9a7c35cb2684 100644 --- a/app/gui/src/dashboard/layouts/AssetContextMenu.tsx +++ b/app/gui/src/dashboard/layouts/AssetContextMenu.tsx @@ -4,7 +4,6 @@ import * as React from 'react' import * as reactQuery from '@tanstack/react-query' import * as toast from 'react-toastify' -import * as billingHooks from '#/hooks/billing' import * as copyHooks from '#/hooks/copyHooks' import * as projectHooks from '#/hooks/projectHooks' import * as toastAndLogHooks from '#/hooks/toastAndLogHooks' @@ -24,7 +23,6 @@ import Separator from '#/components/styled/Separator' import ConfirmDeleteModal from '#/modals/ConfirmDeleteModal' import ManageLabelsModal from '#/modals/ManageLabelsModal' -import ManagePermissionsModal from '#/modals/ManagePermissionsModal' import * as backendModule from '#/services/Backend' import * as localBackendModule from '#/services/LocalBackend' @@ -36,7 +34,7 @@ import { downloadAssetsMutationOptions, restoreAssetsMutationOptions, } from '#/hooks/backendBatchedHooks' -import { useNewProject, useRemoveSelfPermissionMutation } from '#/hooks/backendHooks' +import { useNewProject } from '#/hooks/backendHooks' import { useUploadFileWithToastMutation } from '#/hooks/backendUploadFilesHooks' import { usePasteData } from '#/providers/DriveProvider' import { TEAMS_DIRECTORY_ID, USERS_DIRECTORY_ID } from '#/services/remoteBackendPaths' @@ -64,7 +62,7 @@ export interface AssetContextMenuProps { /** The context menu for an arbitrary {@link backendModule.Asset}. */ export default function AssetContextMenu(props: AssetContextMenuProps) { - const { innerProps, rootDirectoryId, event, eventTarget, hidden = false, triggerRef } = props + const { innerProps, rootDirectoryId, event, hidden = false, triggerRef } = props const { doCopy, doCut, doPaste } = props const { asset, path: pathRaw, state, setRowState } = innerProps const { backend, category, nodeMap } = state @@ -84,7 +82,6 @@ export default function AssetContextMenu(props: AssetContextMenuProps) { const restoreAssetsMutation = reactQuery.useMutation(restoreAssetsMutationOptions(backend)) const copyAssetsMutation = reactQuery.useMutation(copyAssetsMutationOptions(backend)) const downloadAssetsMutation = reactQuery.useMutation(downloadAssetsMutationOptions(backend)) - const removeSelfPermissionMutation = useRemoveSelfPermissionMutation(backend) const openProjectMutation = projectHooks.useOpenProjectMutation() const self = permissions.tryFindSelfPermission(user, asset.permissions) const isCloud = categoryModule.isCloudCategory(category) @@ -102,9 +99,6 @@ export default function AssetContextMenu(props: AssetContextMenuProps) { const uploadFileToCloudMutation = useUploadFileWithToastMutation(remoteBackend) const disabledTooltip = !canOpenProjects ? getText('downloadToOpenWorkflow') : undefined - const { isFeatureUnderPaywall } = billingHooks.usePaywall({ plan: user.plan }) - const isUnderPaywall = isFeatureUnderPaywall('share') - const newProject = useNewProject(backend, category) const systemApi = window.systemApi @@ -430,29 +424,6 @@ export default function AssetContextMenu(props: AssetContextMenuProps) { )} {isCloud &&