From 076a67b0e23bbdb0920c806f3844754d04a5b42c Mon Sep 17 00:00:00 2001 From: Charles Bochet Date: Wed, 6 Dec 2023 16:55:09 +0100 Subject: [PATCH] Fix optimistic rendering issues on views (#2851) * Fix optimistic rendering issues on views * Remove virtualizer --- .../object-record/hooks/useFindManyRecords.ts | 26 ++-- .../hooks/useObjectRecordTable.ts | 22 ++-- .../object-record/hooks/useUpdateOneRecord.ts | 13 +- .../components/RecordTableBody.tsx | 71 +---------- .../components/RecordTableBodyEffect.tsx | 22 +++- .../RecordTableBodyFetchMoreLoader.tsx | 53 +++++++++ .../components/RecordTableRow.tsx | 60 ++++++---- .../utilities/virtualizer/RenderIfVisible.tsx | 112 ------------------ .../components/UpdateViewButtonGroup.tsx | 75 ++++++------ .../src/modules/views/components/ViewBar.tsx | 2 +- .../views/components/ViewBarEffect.tsx | 90 +++++++------- .../views/constants/UpdateViewDropdownId.ts | 1 + 12 files changed, 229 insertions(+), 318 deletions(-) create mode 100644 front/src/modules/ui/object/record-table/components/RecordTableBodyFetchMoreLoader.tsx delete mode 100644 front/src/modules/ui/utilities/virtualizer/RenderIfVisible.tsx create mode 100644 front/src/modules/views/constants/UpdateViewDropdownId.ts diff --git a/front/src/modules/object-record/hooks/useFindManyRecords.ts b/front/src/modules/object-record/hooks/useFindManyRecords.ts index 00a2fdad4..f16034719 100644 --- a/front/src/modules/object-record/hooks/useFindManyRecords.ts +++ b/front/src/modules/object-record/hooks/useFindManyRecords.ts @@ -2,7 +2,7 @@ import { useCallback, useMemo } from 'react'; import { useQuery } from '@apollo/client'; import { isNonEmptyArray } from '@apollo/client/utilities'; import { isNonEmptyString } from '@sniptt/guards'; -import { useRecoilState, useRecoilValue } from 'recoil'; +import { useRecoilState, useRecoilValue, useSetRecoilState } from 'recoil'; import { useOptimisticEffect } from '@/apollo/optimistic-effect/hooks/useOptimisticEffect'; import { currentWorkspaceState } from '@/auth/states/currentWorkspaceState'; @@ -55,7 +55,7 @@ export const useFindManyRecords = < hasNextPageFamilyState(findManyQueryStateIdentifier), ); - const [, setIsFetchingMoreObjects] = useRecoilState( + const setIsFetchingMoreObjects = useSetRecoilState( isFetchingMoreRecordsFamilyState(findManyQueryStateIdentifier), ); @@ -142,6 +142,18 @@ export const useFindManyRecords = < ...fetchMoreResult?.[objectMetadataItem.namePlural]?.edges, ]); } + + if (data?.[objectMetadataItem.namePlural]) { + setLastCursor( + fetchMoreResult?.[objectMetadataItem.namePlural]?.pageInfo + .endCursor, + ); + setHasNextPage( + fetchMoreResult?.[objectMetadataItem.namePlural]?.pageInfo + .hasNextPage, + ); + } + onCompleted?.({ __typename: `${capitalize( objectMetadataItem.nameSingular, @@ -151,15 +163,6 @@ export const useFindManyRecords = < fetchMoreResult?.[objectMetadataItem.namePlural].pageInfo, }); - if (data?.[objectMetadataItem.namePlural]) { - setLastCursor( - data?.[objectMetadataItem.namePlural]?.pageInfo.endCursor, - ); - setHasNextPage( - data?.[objectMetadataItem.namePlural]?.pageInfo.hasNextPage, - ); - } - return Object.assign({}, prev, { [objectMetadataItem.namePlural]: { __typename: `${capitalize( @@ -218,5 +221,6 @@ export const useFindManyRecords = < loading, error, fetchMoreRecords, + queryStateIdentifier: findManyQueryStateIdentifier, }; }; diff --git a/front/src/modules/object-record/hooks/useObjectRecordTable.ts b/front/src/modules/object-record/hooks/useObjectRecordTable.ts index b2cfd8060..c322df5ec 100644 --- a/front/src/modules/object-record/hooks/useObjectRecordTable.ts +++ b/front/src/modules/object-record/hooks/useObjectRecordTable.ts @@ -1,4 +1,4 @@ -import { useRecoilValue } from 'recoil'; +import { useRecoilValue, useSetRecoilState } from 'recoil'; import { useObjectMetadataItem } from '@/object-metadata/hooks/useObjectMetadataItem'; import { useObjectNameSingularFromPlural } from '@/object-metadata/hooks/useObjectNameSingularFromPlural'; @@ -21,11 +21,12 @@ export const useObjectRecordTable = () => { objectNameSingular, }, ); - - const { tableFiltersState, tableSortsState } = useRecordTableScopedStates(); + const { tableFiltersState, tableSortsState, tableLastRowVisibleState } = + useRecordTableScopedStates(); const tableFilters = useRecoilValue(tableFiltersState); const tableSorts = useRecoilValue(tableSortsState); + const setLastRowVisible = useSetRecoilState(tableLastRowVisibleState); const filter = turnFiltersIntoWhereClause( tableFilters, @@ -36,16 +37,21 @@ export const useObjectRecordTable = () => { foundObjectMetadataItem?.fields ?? [], ); - const { records, loading, fetchMoreRecords } = useFindManyRecords({ - objectNameSingular, - filter, - orderBy, - }); + const { records, loading, fetchMoreRecords, queryStateIdentifier } = + useFindManyRecords({ + objectNameSingular, + filter, + orderBy, + onCompleted: () => { + setLastRowVisible(false); + }, + }); return { records, loading, fetchMoreRecords, + queryStateIdentifier, setRecordTableData, }; }; diff --git a/front/src/modules/object-record/hooks/useUpdateOneRecord.ts b/front/src/modules/object-record/hooks/useUpdateOneRecord.ts index 2966b1b2a..dcf81f809 100644 --- a/front/src/modules/object-record/hooks/useUpdateOneRecord.ts +++ b/front/src/modules/object-record/hooks/useUpdateOneRecord.ts @@ -7,21 +7,16 @@ import { capitalize } from '~/utils/string/capitalize'; export const useUpdateOneRecord = ({ objectNameSingular, }: ObjectMetadataItemIdentifier) => { - const { - objectMetadataItem, - updateOneRecordMutation, - getRecordFromCache, - findManyRecordsQuery, - } = useObjectMetadataItem({ - objectNameSingular, - }); + const { objectMetadataItem, updateOneRecordMutation, getRecordFromCache } = + useObjectMetadataItem({ + objectNameSingular, + }); const apolloClient = useApolloClient(); const updateOneRecord = async ({ idToUpdate, input, - forceRefetch, }: { idToUpdate: string; input: Record; diff --git a/front/src/modules/ui/object/record-table/components/RecordTableBody.tsx b/front/src/modules/ui/object/record-table/components/RecordTableBody.tsx index d3dd474f2..24df0d282 100644 --- a/front/src/modules/ui/object/record-table/components/RecordTableBody.tsx +++ b/front/src/modules/ui/object/record-table/components/RecordTableBody.tsx @@ -1,92 +1,33 @@ -import { useContext } from 'react'; -import { useInView } from 'react-intersection-observer'; -import { useRecoilCallback, useRecoilState, useRecoilValue } from 'recoil'; +import { useRecoilValue } from 'recoil'; -import { isFetchingMoreRecordsFamilyState } from '@/object-record/states/isFetchingMoreRecordsFamilyState'; -import { - RecordTableRow, - StyledRow, -} from '@/ui/object/record-table/components/RecordTableRow'; +import { RecordTableBodyFetchMoreLoader } from '@/ui/object/record-table/components/RecordTableBodyFetchMoreLoader'; +import { RecordTableRow } from '@/ui/object/record-table/components/RecordTableRow'; import { RowIdContext } from '@/ui/object/record-table/contexts/RowIdContext'; import { RowIndexContext } from '@/ui/object/record-table/contexts/RowIndexContext'; -import { useRecordTable } from '@/ui/object/record-table/hooks/useRecordTable'; import { isFetchingRecordTableDataState } from '@/ui/object/record-table/states/isFetchingRecordTableDataState'; import { tableRowIdsState } from '@/ui/object/record-table/states/tableRowIdsState'; -import { getRecordTableScopedStates } from '@/ui/object/record-table/utils/getRecordTableScopedStates'; -import { ScrollWrapperContext } from '@/ui/utilities/scroll/components/ScrollWrapper'; -import RenderIfVisible from '@/ui/utilities/virtualizer/RenderIfVisible'; export const RecordTableBody = () => { - const { scopeId } = useRecordTable(); - - const onLastRowVisible = useRecoilCallback( - ({ set }) => - async (inView: boolean) => { - const { tableLastRowVisibleState } = getRecordTableScopedStates({ - recordTableScopeId: scopeId, - }); - - set(tableLastRowVisibleState, inView); - }, - [scopeId], - ); - - const { ref: lastTableRowRef } = useInView({ - onChange: onLastRowVisible, - }); - const tableRowIds = useRecoilValue(tableRowIdsState); - const [isFetchingMoreObjects] = useRecoilState( - isFetchingMoreRecordsFamilyState(scopeId), - ); - const isFetchingRecordTableData = useRecoilValue( isFetchingRecordTableDataState, ); - const lastRowId = tableRowIds[tableRowIds.length - 1]; - - const scrollWrapperRef = useContext(ScrollWrapperContext); - if (isFetchingRecordTableData) { return <>; } return ( <> - {tableRowIds.map((rowId, rowIndex) => ( + {tableRowIds.slice().map((rowId, rowIndex) => ( - - 30 - ? lastTableRowRef - : undefined - } - rowId={rowId} - /> - + ))} - - {isFetchingMoreObjects && ( - - - Loading more... - - - )} - + ); }; diff --git a/front/src/modules/ui/object/record-table/components/RecordTableBodyEffect.tsx b/front/src/modules/ui/object/record-table/components/RecordTableBodyEffect.tsx index 00701a6d2..e92e1d109 100644 --- a/front/src/modules/ui/object/record-table/components/RecordTableBodyEffect.tsx +++ b/front/src/modules/ui/object/record-table/components/RecordTableBodyEffect.tsx @@ -1,28 +1,40 @@ import { useEffect } from 'react'; -import { useRecoilValue } from 'recoil'; +import { useRecoilState, useRecoilValue } from 'recoil'; import { useObjectRecordTable } from '@/object-record/hooks/useObjectRecordTable'; +import { isFetchingMoreRecordsFamilyState } from '@/object-record/states/isFetchingMoreRecordsFamilyState'; import { useRecordTableScopedStates } from '@/ui/object/record-table/hooks/internal/useRecordTableScopedStates'; -import { isDefined } from '~/utils/isDefined'; export const RecordTableBodyEffect = () => { const { fetchMoreRecords: fetchMoreObjects, records, setRecordTableData, + queryStateIdentifier, } = useObjectRecordTable(); const { tableLastRowVisibleState } = useRecordTableScopedStates(); - const tableLastRowVisible = useRecoilValue(tableLastRowVisibleState); + const [tableLastRowVisible, setTableLastRowVisible] = useRecoilState( + tableLastRowVisibleState, + ); + + const isFetchingMoreObjects = useRecoilValue( + isFetchingMoreRecordsFamilyState(queryStateIdentifier), + ); useEffect(() => { setRecordTableData(records); }, [records, setRecordTableData]); useEffect(() => { - if (tableLastRowVisible && isDefined(fetchMoreObjects)) { + if (tableLastRowVisible && !isFetchingMoreObjects) { fetchMoreObjects(); } - }, [fetchMoreObjects, tableLastRowVisible]); + }, [ + fetchMoreObjects, + isFetchingMoreObjects, + setTableLastRowVisible, + tableLastRowVisible, + ]); return <>; }; diff --git a/front/src/modules/ui/object/record-table/components/RecordTableBodyFetchMoreLoader.tsx b/front/src/modules/ui/object/record-table/components/RecordTableBodyFetchMoreLoader.tsx new file mode 100644 index 000000000..ed821ac93 --- /dev/null +++ b/front/src/modules/ui/object/record-table/components/RecordTableBodyFetchMoreLoader.tsx @@ -0,0 +1,53 @@ +import { useInView } from 'react-intersection-observer'; +import { useRecoilCallback, useRecoilValue } from 'recoil'; + +import { useObjectRecordTable } from '@/object-record/hooks/useObjectRecordTable'; +import { isFetchingMoreRecordsFamilyState } from '@/object-record/states/isFetchingMoreRecordsFamilyState'; +import { StyledRow } from '@/ui/object/record-table/components/RecordTableRow'; +import { useRecordTable } from '@/ui/object/record-table/hooks/useRecordTable'; +import { isFetchingRecordTableDataState } from '@/ui/object/record-table/states/isFetchingRecordTableDataState'; +import { getRecordTableScopedStates } from '@/ui/object/record-table/utils/getRecordTableScopedStates'; + +export const RecordTableBodyFetchMoreLoader = () => { + const { queryStateIdentifier } = useObjectRecordTable(); + const { scopeId } = useRecordTable(); + + const isFetchingMoreObjects = useRecoilValue( + isFetchingMoreRecordsFamilyState(queryStateIdentifier), + ); + + const isFetchingRecordTableData = useRecoilValue( + isFetchingRecordTableDataState, + ); + + const onLastRowVisible = useRecoilCallback( + ({ set }) => + async (inView: boolean) => { + const { tableLastRowVisibleState } = getRecordTableScopedStates({ + recordTableScopeId: scopeId, + }); + set(tableLastRowVisibleState, inView); + }, + [scopeId], + ); + + const { ref: tbodyRef } = useInView({ + onChange: onLastRowVisible, + }); + + if (isFetchingRecordTableData) { + return <>; + } + + return ( + + {isFetchingMoreObjects && ( + + + Loading more... + + + )} + + ); +}; diff --git a/front/src/modules/ui/object/record-table/components/RecordTableRow.tsx b/front/src/modules/ui/object/record-table/components/RecordTableRow.tsx index a8a66137e..38788a170 100644 --- a/front/src/modules/ui/object/record-table/components/RecordTableRow.tsx +++ b/front/src/modules/ui/object/record-table/components/RecordTableRow.tsx @@ -1,7 +1,10 @@ -import { forwardRef } from 'react'; +import { useContext } from 'react'; +import { useInView } from 'react-intersection-observer'; import styled from '@emotion/styled'; import { useRecoilValue } from 'recoil'; +import { ScrollWrapperContext } from '@/ui/utilities/scroll/components/ScrollWrapper'; + import { ColumnContext } from '../contexts/ColumnContext'; import { useRecordTableScopedStates } from '../hooks/internal/useRecordTableScopedStates'; import { useCurrentRowSelected } from '../record-table-row/hooks/useCurrentRowSelected'; @@ -18,36 +21,53 @@ type RecordTableRowProps = { rowId: string; }; -export const RecordTableRow = forwardRef< - HTMLTableRowElement, - RecordTableRowProps ->(({ rowId }, ref) => { +const StyledPlaceholder = styled.td` + height: 30px; +`; + +export const RecordTableRow = ({ rowId }: RecordTableRowProps) => { const { visibleTableColumnsSelector } = useRecordTableScopedStates(); const visibleTableColumns = useRecoilValue(visibleTableColumnsSelector); const { currentRowSelected } = useCurrentRowSelected(); + const scrollWrapperRef = useContext(ScrollWrapperContext); + + const { ref: elementRef, inView } = useInView({ + root: scrollWrapperRef.current, + rootMargin: '1000px', + }); + return ( - - - - {[...visibleTableColumns] - .sort((columnA, columnB) => columnA.position - columnB.position) - .map((column, columnIndex) => { - return ( - - - - ); - })} - + {inView ? ( + <> + + + + {[...visibleTableColumns] + .sort((columnA, columnB) => columnA.position - columnB.position) + .map((column, columnIndex) => { + return ( + + + + ); + })} + + + ) : ( + + )} ); -}); +}; diff --git a/front/src/modules/ui/utilities/virtualizer/RenderIfVisible.tsx b/front/src/modules/ui/utilities/virtualizer/RenderIfVisible.tsx deleted file mode 100644 index e2c446252..000000000 --- a/front/src/modules/ui/utilities/virtualizer/RenderIfVisible.tsx +++ /dev/null @@ -1,112 +0,0 @@ -import React, { useEffect, useMemo, useRef, useState } from 'react'; - -type RenderIfVisibleProps = { - /** - * Whether the element should be visible initially or not. - * Useful e.g. for always setting the first N items to visible. - * Default: false - */ - initialVisible?: boolean; - /** An estimate of the element's height */ - defaultHeight?: number; - /** How far outside the viewport in pixels should elements be considered visible? */ - visibleOffset?: number; - /** Should the element stay rendered after it becomes visible? */ - stayRendered?: boolean; - root?: HTMLElement | null; - /** E.g. 'span', 'tbody'. Default = 'div' */ - rootElement?: string; - rootElementClass?: string; - /** E.g. 'span', 'tr'. Default = 'div' */ - placeholderElement?: string; - placeholderElementClass?: string; - children: React.ReactNode; -}; - -const RenderIfVisible = ({ - initialVisible = false, - defaultHeight = 300, - visibleOffset = 1000, - stayRendered = false, - root = null, - rootElement = 'div', - rootElementClass = '', - placeholderElement = 'div', - placeholderElementClass = '', - children, -}: RenderIfVisibleProps) => { - const [isVisible, setIsVisible] = useState(initialVisible); - - // eslint-disable-next-line twenty/no-state-useref - const wasVisible = useRef(initialVisible); - // eslint-disable-next-line twenty/no-state-useref - const placeholderHeight = useRef(defaultHeight); - const intersectionRef = useRef(null); - - // Set visibility with intersection observer - useEffect(() => { - if (intersectionRef.current) { - const localRef = intersectionRef.current; - const observer = new IntersectionObserver( - (entries) => { - // Before switching off `isVisible`, set the height of the placeholder - if (!entries[0].isIntersecting) { - placeholderHeight.current = localRef!.offsetHeight; - } - if (typeof window !== undefined && window.requestIdleCallback) { - window.requestIdleCallback( - () => setIsVisible(entries[0].isIntersecting), - { - timeout: 600, - }, - ); - } else { - setIsVisible(entries[0].isIntersecting); - } - }, - { root, rootMargin: `${visibleOffset}px 0px ${visibleOffset}px 0px` }, - ); - - observer.observe(localRef); - return () => { - if (localRef) { - observer.unobserve(localRef); - } - }; - } - return () => {}; - }, [root, visibleOffset]); - - useEffect(() => { - if (isVisible) { - wasVisible.current = true; - } - }, [isVisible]); - - const placeholderStyle = { height: placeholderHeight.current }; - const rootClasses = useMemo( - () => `renderIfVisible ${rootElementClass}`, - [rootElementClass], - ); - const placeholderClasses = useMemo( - () => `renderIfVisible-placeholder ${placeholderElementClass}`, - [placeholderElementClass], - ); - - // eslint-disable-next-line react/no-children-prop - return React.createElement(rootElement, { - children: - isVisible || (stayRendered && wasVisible.current) ? ( - <>{children} - ) : ( - React.createElement(placeholderElement, { - className: placeholderClasses, - style: placeholderStyle, - }) - ), - ref: intersectionRef, - className: rootClasses, - }); -}; - -export default RenderIfVisible; diff --git a/front/src/modules/views/components/UpdateViewButtonGroup.tsx b/front/src/modules/views/components/UpdateViewButtonGroup.tsx index acadeb0c9..6d914d3cf 100644 --- a/front/src/modules/views/components/UpdateViewButtonGroup.tsx +++ b/front/src/modules/views/components/UpdateViewButtonGroup.tsx @@ -1,14 +1,16 @@ -import { useCallback, useState } from 'react'; +import { useCallback } from 'react'; import styled from '@emotion/styled'; import { useRecoilValue } from 'recoil'; -import { Key } from 'ts-key-enum'; import { IconChevronDown, IconPlus } from '@/ui/display/icon'; import { Button } from '@/ui/input/button/components/Button'; import { ButtonGroup } from '@/ui/input/button/components/ButtonGroup'; +import { Dropdown } from '@/ui/layout/dropdown/components/Dropdown'; import { DropdownMenuItemsContainer } from '@/ui/layout/dropdown/components/DropdownMenuItemsContainer'; +import { DropdownScope } from '@/ui/layout/dropdown/scopes/DropdownScope'; import { MenuItem } from '@/ui/navigation/menu-item/components/MenuItem'; -import { useScopedHotkeys } from '@/ui/utilities/hotkey/hooks/useScopedHotkeys'; +import { HotkeyScope } from '@/ui/utilities/hotkey/types/HotkeyScope'; +import { UpdateViewDropdownId } from '@/views/constants/UpdateViewDropdownId'; import { useViewBar } from '@/views/hooks/useViewBar'; import { useViewScopedStates } from '../hooks/internal/useViewScopedStates'; @@ -20,7 +22,7 @@ const StyledContainer = styled.div` `; export type UpdateViewButtonGroupProps = { - hotkeyScope: string; + hotkeyScope: HotkeyScope; onViewEditModeChange?: () => void; }; @@ -28,7 +30,6 @@ export const UpdateViewButtonGroup = ({ hotkeyScope, onViewEditModeChange, }: UpdateViewButtonGroupProps) => { - const [isDropdownOpen, setIsDropdownOpen] = useState(false); const { updateCurrentView, setViewEditMode } = useViewBar(); const { canPersistFiltersSelector, canPersistSortsSelector } = useViewScopedStates(); @@ -38,53 +39,43 @@ export const UpdateViewButtonGroup = ({ const canPersistView = canPersistFilters || canPersistSorts; - const handleArrowDownButtonClick = useCallback(() => { - setIsDropdownOpen((previousIsOpen) => !previousIsOpen); - }, []); - const handleCreateViewButtonClick = useCallback(() => { setViewEditMode('create'); onViewEditModeChange?.(); - setIsDropdownOpen(false); }, [setViewEditMode, onViewEditModeChange]); - const handleDropdownClose = useCallback(() => { - setIsDropdownOpen(false); - }, []); - const handleViewSubmit = async () => { await updateCurrentView?.(); }; - useScopedHotkeys( - [Key.Enter, Key.Escape], - handleDropdownClose, - hotkeyScope, - [], - ); - - if (!canPersistView) return null; + if (!canPersistView) { + return <>; + } return ( - - -