☑️ Refacto "Select All/Unselect all" on indexes

[gitstart-twenty]

Description

☑️ Refacto "Select All/Unselect all" on indexes

Refs

#4397

Demo

https://github.com/twentyhq/twenty/assets/140154534/5dd78e33-8e66-4de1-953a-971b3f77cb5d

Fixes #4397

[gitstart-twenty]

Hey @charlesBochet What do you think of this v1?

[charlesBochet]

@gitstart-twenty could you rebase your PR on main, tests should be green :)

[gitstart-twenty]

@gitstart-twenty could you rebase your PR on main, tests should be green :)

Alright

[gitstart-twenty]

Hey @charlesBochet, We have duplicate code in useFindManyRecords and useLazyFindManyRecords, what do you think of this approach to making it DRY?

// packages/twenty-front/src/modules/object-record/hooks/useFindManyRecords.ts
import { useCallback, useMemo } from 'react';
import {
  ApolloQueryResult,
  FetchMoreQueryOptions,
  OperationVariables,
  useQuery,
  WatchQueryFetchPolicy,
} from '@apollo/client';
import { isNonEmptyArray } from '@apollo/client/utilities';
import { isNonEmptyString } from '@sniptt/guards';
import { useRecoilState, useRecoilValue, useSetRecoilState } from 'recoil';

import { currentWorkspaceMemberState } from '@/auth/states/currentWorkspaceMemberState';
import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem';
import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
import { ObjectMetadataItemIdentifier } from '@/object-metadata/types/ObjectMetadataItemIdentifier';
import { getRecordsFromRecordConnection } from '@/object-record/cache/utils/getRecordsFromRecordConnection';
import { RecordGqlConnection } from '@/object-record/graphql/types/RecordGqlConnection';
import { RecordGqlEdge } from '@/object-record/graphql/types/RecordGqlEdge';
import { RecordGqlOperationFindManyResult } from '@/object-record/graphql/types/RecordGqlOperationFindManyResult';
import { RecordGqlOperationGqlRecordFields } from '@/object-record/graphql/types/RecordGqlOperationGqlRecordFields';
import { RecordGqlOperationVariables } from '@/object-record/graphql/types/RecordGqlOperationVariables';
import { useFindManyRecordsQuery } from '@/object-record/hooks/useFindManyRecordsQuery';
import { ObjectRecord } from '@/object-record/types/ObjectRecord';
import { filterUniqueRecordEdgesByCursor } from '@/object-record/utils/filterUniqueRecordEdgesByCursor';
import { useSnackBar } from '@/ui/feedback/snack-bar-manager/hooks/useSnackBar';
import { isDefined } from '~/utils/isDefined';
import { logError } from '~/utils/logError';
import { capitalize } from '~/utils/string/capitalize';

import { cursorFamilyState } from '../states/cursorFamilyState';
import { hasNextPageFamilyState } from '../states/hasNextPageFamilyState';
import { isFetchingMoreRecordsFamilyState } from '../states/isFetchingMoreRecordsFamilyState';

export type UseFindManyRecordsParams<T> = ObjectMetadataItemIdentifier &
  RecordGqlOperationVariables & {
    onCompleted?: (
      records: T[],
      options?: {
        pageInfo?: RecordGqlConnection['pageInfo'];
        totalCount?: number;
      },
    ) => void;
    skip?: boolean;
    recordGqlFields?: RecordGqlOperationGqlRecordFields;
    fetchPolicy?: WatchQueryFetchPolicy;
  };

type UseFindManyRecordsStateParams<
  T,
  TData = RecordGqlOperationFindManyResult,
> = Omit<
  UseFindManyRecordsParams<T>,
  'skip' | 'recordGqlFields' | 'fetchPolicy'
> & {
  data: RecordGqlOperationFindManyResult | undefined;
  fetchMore<
    TFetchData = TData,
    TFetchVars extends OperationVariables = OperationVariables,
  >(
    fetchMoreOptions: FetchMoreQueryOptions<TFetchVars, TFetchData> & {
      updateQuery?: (
        previousQueryResult: TData,
        options: {
          fetchMoreResult: TFetchData;
          variables: TFetchVars;
        },
      ) => TData;
    },
  ): Promise<ApolloQueryResult<TFetchData>>;
  objectMetadataItem: ObjectMetadataItem;
};

export const useFindManyRecordsState = <T extends ObjectRecord = ObjectRecord>({
  objectNameSingular,
  filter,
  orderBy,
  limit,
  onCompleted,
  data,
  fetchMore,
  objectMetadataItem,
}: UseFindManyRecordsStateParams<T>) => {
  const findManyQueryStateIdentifier =
    objectNameSingular +
    JSON.stringify(filter) +
    JSON.stringify(orderBy) +
    limit;

  const [lastCursor, setLastCursor] = useRecoilState(
    cursorFamilyState(findManyQueryStateIdentifier),
  );

  const [hasNextPage, setHasNextPage] = useRecoilState(
    hasNextPageFamilyState(findManyQueryStateIdentifier),
  );

  const setIsFetchingMoreObjects = useSetRecoilState(
    isFetchingMoreRecordsFamilyState(findManyQueryStateIdentifier),
  );

  const { enqueueSnackBar } = useSnackBar();

  const fetchMoreRecords = useCallback(async () => {
    if (hasNextPage) {
      setIsFetchingMoreObjects(true);

      try {
        await fetchMore({
          variables: {
            filter,
            orderBy,
            lastCursor: isNonEmptyString(lastCursor) ? lastCursor : undefined,
          },
          updateQuery: (prev, { fetchMoreResult }) => {
            const previousEdges = prev?.[objectMetadataItem.namePlural]?.edges;
            const nextEdges =
              fetchMoreResult?.[objectMetadataItem.namePlural]?.edges;

            let newEdges: RecordGqlEdge[] = [];

            if (isNonEmptyArray(previousEdges) && isNonEmptyArray(nextEdges)) {
              newEdges = filterUniqueRecordEdgesByCursor([
                ...(prev?.[objectMetadataItem.namePlural]?.edges ?? []),
                ...(fetchMoreResult?.[objectMetadataItem.namePlural]?.edges ??
                  []),
              ]);
            }

            const pageInfo =
              fetchMoreResult?.[objectMetadataItem.namePlural]?.pageInfo;

            if (isDefined(data?.[objectMetadataItem.namePlural])) {
              setLastCursor(pageInfo.endCursor ?? '');
              setHasNextPage(pageInfo.hasNextPage ?? false);
            }

            const records = getRecordsFromRecordConnection({
              recordConnection: {
                edges: newEdges,
                pageInfo,
              },
            }) as T[];

            onCompleted?.(records, {
              pageInfo,
              totalCount:
                fetchMoreResult?.[objectMetadataItem.namePlural]?.totalCount,
            });

            return Object.assign({}, prev, {
              [objectMetadataItem.namePlural]: {
                __typename: `${capitalize(
                  objectMetadataItem.nameSingular,
                )}Connection`,
                edges: newEdges,
                pageInfo:
                  fetchMoreResult?.[objectMetadataItem.namePlural].pageInfo,
                totalCount:
                  fetchMoreResult?.[objectMetadataItem.namePlural].totalCount,
              },
            } as RecordGqlOperationFindManyResult);
          },
        });
      } catch (error) {
        logError(
          `fetchMoreObjects for "${objectMetadataItem.namePlural}" error : ` +
            error,
        );
        enqueueSnackBar(
          `Error during fetchMoreObjects for "${objectMetadataItem.namePlural}", ${error}`,
          {
            variant: 'error',
          },
        );
      } finally {
        setIsFetchingMoreObjects(false);
      }
    }
  }, [
    hasNextPage,
    setIsFetchingMoreObjects,
    fetchMore,
    filter,
    orderBy,
    lastCursor,
    objectMetadataItem.namePlural,
    objectMetadataItem.nameSingular,
    onCompleted,
    data,
    setLastCursor,
    setHasNextPage,
    enqueueSnackBar,
  ]);

  const totalCount = data?.[objectMetadataItem.namePlural].totalCount ?? 0;

  const records = useMemo(
    () =>
      data?.[objectMetadataItem.namePlural]
        ? getRecordsFromRecordConnection<T>({
            recordConnection: data?.[objectMetadataItem.namePlural],
          })
        : ([] as T[]),

    [data, objectMetadataItem.namePlural],
  );

  return {
    findManyQueryStateIdentifier,
    setLastCursor,
    setHasNextPage,
    fetchMoreRecords,
    totalCount,
    records,
  };
};

export const useFindManyRecords = <T extends ObjectRecord = ObjectRecord>({
  objectNameSingular,
  filter,
  orderBy,
  limit,
  onCompleted,
  skip,
  recordGqlFields,
  fetchPolicy,
}: UseFindManyRecordsParams<T>) => {
  const { enqueueSnackBar } = useSnackBar();
  const currentWorkspaceMember = useRecoilValue(currentWorkspaceMemberState);
  const { objectMetadataItem } = useObjectMetadataItem({
    objectNameSingular,
  });
  const { findManyRecordsQuery } = useFindManyRecordsQuery({
    objectNameSingular,
    recordGqlFields,
  });

  const { data, loading, error, fetchMore } =
    useQuery<RecordGqlOperationFindManyResult>(findManyRecordsQuery, {
      skip: skip || !objectMetadataItem || !currentWorkspaceMember,
      variables: {
        filter,
        limit,
        orderBy,
      },
      fetchPolicy: fetchPolicy,
      onCompleted: (data) => {
        if (!isDefined(data)) {
          onCompleted?.([]);
        }

        const pageInfo = data?.[objectMetadataItem.namePlural]?.pageInfo;

        const records = getRecordsFromRecordConnection({
          recordConnection: data?.[objectMetadataItem.namePlural],
        }) as T[];

        onCompleted?.(records, {
          pageInfo,
          totalCount: data?.[objectMetadataItem.namePlural]?.totalCount,
        });

        if (isDefined(data?.[objectMetadataItem.namePlural])) {
          setLastCursor(pageInfo.endCursor ?? '');
          setHasNextPage(pageInfo.hasNextPage ?? false);
        }
      },
      onError: (error) => {
        logError(
          `useFindManyRecords for "${objectMetadataItem.namePlural}" error : ` +
            error,
        );
        enqueueSnackBar(
          `Error during useFindManyRecords for "${objectMetadataItem.namePlural}", ${error.message}`,
          {
            variant: 'error',
          },
        );
      },
    });

  const {
    findManyQueryStateIdentifier,
    setLastCursor,
    setHasNextPage,
    fetchMoreRecords,
    totalCount,
    records,
  } = useFindManyRecordsState<T>({
    objectNameSingular,
    filter,
    orderBy,
    limit,
    onCompleted,
    fetchMore,
    data,
    objectMetadataItem,
  });

  return {
    objectMetadataItem,
    records,
    totalCount,
    loading,
    error,
    fetchMoreRecords,
    queryStateIdentifier: findManyQueryStateIdentifier,
  };
};

// packages/twenty-front/src/modules/object-record/hooks/useLazyFindManyRecords.ts
import { useLazyQuery } from '@apollo/client';
import { useRecoilValue } from 'recoil';

import { currentWorkspaceMemberState } from '@/auth/states/currentWorkspaceMemberState';
import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem';
import { getRecordsFromRecordConnection } from '@/object-record/cache/utils/getRecordsFromRecordConnection';
import { RecordGqlOperationFindManyResult } from '@/object-record/graphql/types/RecordGqlOperationFindManyResult';
import {
  UseFindManyRecordsParams,
  useFindManyRecordsState,
} from '@/object-record/hooks/useFindManyRecords';
import { useFindManyRecordsQuery } from '@/object-record/hooks/useFindManyRecordsQuery';
import { ObjectRecord } from '@/object-record/types/ObjectRecord';
import { useSnackBar } from '@/ui/feedback/snack-bar-manager/hooks/useSnackBar';
import { isDefined } from '~/utils/isDefined';
import { logError } from '~/utils/logError';

type UseLazyFindManyRecordsParams<T> = Omit<
  UseFindManyRecordsParams<T>,
  'skip'
>;

export const useLazyFindManyRecords = <T extends ObjectRecord = ObjectRecord>({
  objectNameSingular,
  filter,
  orderBy,
  limit,
  onCompleted,
  recordGqlFields,
  fetchPolicy,
}: UseLazyFindManyRecordsParams<T>) => {
  const { objectMetadataItem } = useObjectMetadataItem({
    objectNameSingular,
  });

  const { findManyRecordsQuery } = useFindManyRecordsQuery({
    objectNameSingular,
    recordGqlFields,
  });

  const { enqueueSnackBar } = useSnackBar();
  const currentWorkspaceMember = useRecoilValue(currentWorkspaceMemberState);

  const [findManyRecords, { data, loading, error, fetchMore }] =
    useLazyQuery<RecordGqlOperationFindManyResult>(findManyRecordsQuery, {
      variables: {
        filter,
        limit,
        orderBy,
      },
      fetchPolicy: fetchPolicy,
      onCompleted: (data) => {
        if (!isDefined(data)) {
          onCompleted?.([]);
        }

        const pageInfo = data?.[objectMetadataItem.namePlural]?.pageInfo;

        const records = getRecordsFromRecordConnection({
          recordConnection: data?.[objectMetadataItem.namePlural],
        }) as T[];

        onCompleted?.(records, {
          pageInfo,
          totalCount: data?.[objectMetadataItem.namePlural]?.totalCount,
        });

        if (isDefined(data?.[objectMetadataItem.namePlural])) {
          setLastCursor(pageInfo.endCursor ?? '');
          setHasNextPage(pageInfo.hasNextPage ?? false);
        }
      },
      onError: (error) => {
        logError(
          `useFindManyRecords for "${objectMetadataItem.namePlural}" error : ` +
            error,
        );
        enqueueSnackBar(
          `Error during useFindManyRecords for "${objectMetadataItem.namePlural}", ${error.message}`,
          {
            variant: 'error',
          },
        );
      },
    });

  const {
    findManyQueryStateIdentifier,
    setLastCursor,
    setHasNextPage,
    fetchMoreRecords,
    totalCount,
    records,
  } = useFindManyRecordsState<T>({
    objectNameSingular,
    filter,
    orderBy,
    limit,
    onCompleted,
    fetchMore,
    data,
    objectMetadataItem,
  });

  return {
    objectMetadataItem,
    records,
    totalCount,
    loading,
    error,
    fetchMoreRecords,
    queryStateIdentifier: findManyQueryStateIdentifier,
    findManyRecords: currentWorkspaceMember ? findManyRecords : () => {},
  };
};

[FelixMalfait]

Hey, video looks great (nice improvement!) but the total count in the view switcher doesn't get refreshed apparently, we should fix that if that hasn't been fixed already. (I didn't check the code)

[lucasbordeau]

@gitstart-twenty Seems like a good DRY increment for me, this starts to be a big file and difficult to read but it would need a more in depth refactor.

[lucasbordeau]

Though I would name it according to its context which is mainly about pagination. Maybe something like useFindManyRecordsPaginationUtils ?

[gitstart-twenty]

Though I would name it according to its context which is mainly about pagination. Maybe something like useFindManyRecordsPaginationUtils ?

Hey @lucasbordeau Thanks for the comments. Looking into them!

[gitstart-twenty]

Hey, video looks great (nice improvement!) but the total count in the view switcher doesn't get refreshed apparently, we should fix that if that hasn't been fixed already. (I didn't check the code)

Hey @FelixMalfait Thanks for the comment. Looking into this!

[gitstart-twenty]

@FelixMalfait We're attempting to refetch the records after the import(to get the updated total count) but we keep running into the error below

 + refetchQueries: [getOperationName(findManyRecordsQuery) ?? ''],

The error

 [
    InternalServerErrorException: GraphQL errors on person: {"message":"Unknown field \"personCollection\" on type Query"}
        at computePgGraphQLError (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/compute-pg-graphql-error.util.ts:52:10)
        at WorkspaceQueryRunnerService.parseResult (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts:612:42)
        at WorkspaceQueryRunnerService.findMany (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts:116:17)
        at processTicksAndRejections (node:internal/process/task_queues:95:5)
        at field.resolve (/.../node_modules/@envelop/on-resolve/cjs/index.js:36:42)
        at /.../node_modules/graphql-yoga/node_modules/@graphql-tools/executor/cjs/execution/promiseForObject.js:18:35
        at async Promise.all (index 0) {
      path: [ 'people' ],
      locations: [ [Object] ],
      extensions: [Object: null prototype] {}
    }
  ]

[FelixMalfait]

Not sure sorry!

[gitstart-twenty]

Not sure sorry!

Alright, debugging!

[gitstart-twenty]

Not sure sorry!

Alright!

[gitstart-twenty]

@FelixMalfait We're attempting to refetch the records after the import(to get the updated total count) but we keep running into the error below

 + refetchQueries: [getOperationName(findManyRecordsQuery) ?? ''],

The error

 [
    InternalServerErrorException: GraphQL errors on person: {"message":"Unknown field \"personCollection\" on type Query"}
        at computePgGraphQLError (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/utils/compute-pg-graphql-error.util.ts:52:10)
        at WorkspaceQueryRunnerService.parseResult (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts:612:42)
        at WorkspaceQueryRunnerService.findMany (/.../packages/twenty-server/src/engine/api/graphql/workspace-query-runner/workspace-query-runner.service.ts:116:17)
        at processTicksAndRejections (node:internal/process/task_queues:95:5)
        at field.resolve (/.../node_modules/@envelop/on-resolve/cjs/index.js:36:42)
        at /.../node_modules/graphql-yoga/node_modules/@graphql-tools/executor/cjs/execution/promiseForObject.js:18:35
        at async Promise.all (index 0) {
      path: [ 'people' ],
      locations: [ [Object] ],
      extensions: [Object: null prototype] {}
    }
  ]

Seems to work now!

[gitstart-twenty]

Something(new changes) broke the deletion

[lucasbordeau]

@gitstart-twenty Do you need something from our side to move forward ?

[gitstart-twenty]

@gitstart-twenty Do you need something from our side to move forward ?

@lucasbordeau Going to update you on this ASAP!

[FelixMalfait]

@gitstart-twenty let's try to close this one this week since it's been open for long! Or let us know if we should close it now. Thanks

[gitstart-twenty]

@gitstart-twenty let's try to close this one this week since it's been open for long! Or let us know if we should close it now. Thanks

Hey @FelixMalfait This is going to be completed!

[gitstart-twenty]

https://github.com/twentyhq/twenty/assets/140154534/8ed841c0-03f4-427f-8ff1-951d516052a9

[lucasbordeau]

@gitstart-twenty I just tested and I have unpredictable results, sometimes the export gets triggered before 100%

Would it be difficult for you to remove the useEffect and use a loop/map that awaits for each lazy fetchMore ?

[gitstart-twenty]

@gitstart-twenty I just tested and I have unpredictable results, sometimes the export gets triggered before 100%

Would it be difficult for you to remove the useEffect and use a loop/map that awaits for each lazy fetchMore ?

Hey @lucasbordeau Let's look into this

[gitstart-twenty]

Would it be difficult for you to remove the useEffect and use a loop/map that awaits for each lazy fetchMore ?

Hey @lucasbordeau,

Using a loop has proven tricky, this is because fetchMoreRecords completing doesn't necessarily mean the records have also been updated with the new ones. So, much as we can loop and wait for each to complete before running the next one, we will most likely still have incomplete results (tried and tested)!

Consequently, we'd have to watch the records which brings us right back to the useEffect

[gitstart-twenty]

Perhaps we can look into ensuring the useEffect always ensures all records have been updated before running the callback?

[gitstart-twenty]

Hey @lucasbordeau We resorted to fixing the useEffect. The change below seems to make a difference, please test out on your side

The assumption the code before made was that if the record count in a new iteration equals the previous count, then all records have been fetched(the new fetch didn't add any more records). However, testing shows this is not always the case, sometimes record count changes but we're still pages away from the totalCount

[lucasbordeau]

@gitstart-twenty That works for me.

Just a minor issue left : the header checkbox should be checked if all rows are selected, not in indeterminate state like now.

[lucasbordeau]

And after export, the header checkbox stays at selected state, even if all the rows are unselected.

[gitstart-twenty]

Hey @lucasbordeau

And after export, the header checkbox stays at selected state, even if all the rows are unselected.

https://github.com/twentyhq/twenty/assets/140154534/e7ae3a44-8c8f-45f1-92e5-2b4026efcc83

[lucasbordeau]

When we deselect after select all we should deselect from total, not from loaded rows in front.

Please make sure that you get a table with more than 300/400 rows so that you're not having loaded rows = db rows, which hides some edge cases

[gitstart-twenty]

When we deselect after select all we should deselect from total, not from loaded rows in front.

Please make sure that you get a table with more than 300/400 rows so that you're not having loaded rows = db rows, which hides some edge cases

Interesting! Working on it!