Refacto views #10272

charlesBochet · 2025-02-17T19:59:56Z

In this huge (sorry!) PR:

introducing objectMetadataItem in contextStore instead of objectMetadataId which is more convenient
splitting some big hooks into smaller parts to avoid re-renders
removing Effects to avoid re-renders (especially onViewChange)
making the view prefetch separate from favorites to avoid re-renders
making the view prefetch load a state and add selectors on top of it to avoir re-renders

As a result, the performance is WAY better (I suspect the favorite implementation to trigger a lot of re-renders unfortunately).
However, we are still facing a random app freeze on view creation. I could not investigate the root cause. As this seems to be already there in the precedent release, we can move forward but this seems a urgent follow up to me ==> EDIT: I've found the root cause after a few ours of deep dive... an infinite loop in RecordTableNoRecordGroupBodyEffect...

prastoin edit: close #10253

greptile-apps

PR Summary

This PR implements a significant refactoring of the views and context store management system. Here's a concise summary of the key changes:

Replaces contextStoreCurrentObjectMetadataIdComponentState with contextStoreCurrentObjectMetadataItemComponentState across the application, moving from storing just IDs to complete metadata objects.
Introduces new RecordIndexContainerGater component to centralize record index view management and context providers.
Removes ViewBarEffect, QueryParamsViewIdEffect, and ViewEventContext in favor of a simpler, more centralized state management approach.
Adds new Recoil selectors for prefetching views and managing view state, including prefetchViewsState and related selector families.
Changes error handling in mapViewFiltersToFilters to handle race conditions during view changes by returning undefined instead of throwing errors.

Key points to review:

The removal of error throwing in mapViewFiltersToFilters could mask legitimate metadata issues
The TODO comment in RecordIndexContainerGater indicates potential technical debt around state reset logic
The new prefetch selectors assume single INDEX views per object metadata item
The change from null to undefined for certain states while keeping null for others suggests inconsistent null handling
The removal of direct URL parameter handling in RecordIndexPage might affect deep linking functionality

_{66 file(s) reviewed, 17 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-02-17T20:02:30Z

packages/twenty-front/src/modules/action-menu/components/RecordShowActionMenu.tsx

@@ -42,7 +41,7 @@ export const RecordShowActionMenu = ({

  return (
    <>
-      {contextStoreCurrentObjectMetadataId && (
+      {contextStoreCurrentObjectMetadataItem && (


style: The conditional check here may be redundant since objectMetadataItem is already passed as a required prop to this component

False positive

greptile-apps · 2025-02-17T20:03:57Z

packages/twenty-front/src/modules/context-store/components/MainContextStoreProviderEffect.tsx

    mainContextStoreComponentInstanceId,
  );
+  console.log('Provider effect');


style: Remove debug console.log before merging

Suggested change

console.log('Provider effect');

greptile-apps · 2025-02-17T20:04:28Z

...ges/twenty-front/src/modules/context-store/hooks/useContextStoreObjectMetadataItemOrThrow.ts

+  );
+
+  if (!objectMetadataItem) {
+    throw new Error('Object metadata item is not se in context store');


syntax: 'not se' should be 'not set'

Suggested change

throw new Error('Object metadata item is not se in context store');

throw new Error('Object metadata item is not set in context store');

...ages/twenty-front/src/modules/object-metadata/hooks/useColumnDefinitionsFromFieldMetadata.ts

greptile-apps · 2025-02-17T20:05:59Z

...ages/twenty-front/src/modules/object-metadata/hooks/useColumnDefinitionsFromFieldMetadata.ts

+  const columnDefinitions: ColumnDefinition<FieldMetadata>[] =
+    activeFieldMetadataItems
+      .map((field, index) =>
+        formatFieldMetadataItemAsColumnDefinition({
+          position: index,
+          field,
+          objectMetadataItem,
+        }),
+      )
+      .filter(filterAvailableTableColumns)
+      .map((column) => {
+        const existsInFilterDefinitions = filterableFieldMetadataItems.some(
+          (fieldMetadataItem) =>
+            fieldMetadataItem.id === column.fieldMetadataId,
+        );
+        const existsInSortDefinitions = sortDefinitions.some(
+          (sort) => sort.fieldMetadataId === column.fieldMetadataId,
+        );
+        return {
+          ...column,
+          isFilterable: existsInFilterDefinitions,
+          isSortable: existsInSortDefinitions,
+        };
+      });


style: Complex computation chain (multiple maps and filters) should be memoized to prevent unnecessary recalculations on each render

greptile-apps · 2025-02-17T20:12:55Z

packages/twenty-front/src/modules/views/events/contexts/ViewEventContext.ts

logic: Removing this context will break any components that consume ViewEventContext.onCurrentViewChange. Ensure all consumers have been updated to use the new pattern.

greptile-apps · 2025-02-17T20:17:35Z

packages/twenty-front/src/modules/views/states/currentViewIdComponentState.ts

logic: This state was used by critical view management hooks like useGetCurrentView and useUpdateCurrentView. Ensure all dependencies have been updated to use the new state management approach.

greptile-apps · 2025-02-17T20:17:52Z

packages/twenty-front/src/modules/views/utils/mapViewFiltersToFilters.ts

-        throw new Error(
-          `Field metadata item not found for view filter ${viewFilter.id} and field metadata id ${viewFilter.fieldMetadataId}`,
-        );
+        // Todo: we we don't throw an error yet as we have race condition on view change


syntax: Typo in comment: 'we we' should be 'we'

Suggested change

// Todo: we we don't throw an error yet as we have race condition on view change

// Todo: we don't throw an error yet as we have race condition on view change

greptile-apps · 2025-02-17T20:18:28Z

packages/twenty-front/src/pages/object-record/RecordIndexPage.tsx

+    prefetchIsLoadedFamilyState(PrefetchKey.AllViews),
+  );
+
+  console.log('prefetchIsLoaded', prefetchIsLoaded);


style: console.log statement should be removed before production

Suggested change

console.log('prefetchIsLoaded', prefetchIsLoaded);

greptile-apps · 2025-02-17T20:19:03Z

packages/twenty-front/src/testing/jest/JestContextStoreSetter.tsx

    setContextStoreNumberOfSelectedRecords(contextStoreNumberOfSelectedRecords);
    setcontextStoreFiltersComponentState(contextStoreFilters);

    setIsLoaded(true);
  }, [
    setContextStoreTargetedRecordsRule,
-    setContextStoreCurrentObjectMetadataId,
+    setContextStoreCurrentObjectMetadataItem,
    contextStoreTargetedRecordsRule,
    contextStoreCurrentObjectMetadataId,


logic: contextStoreCurrentObjectMetadataId in dependencies array is not needed since it's not used in the effect

Suggested change

contextStoreCurrentObjectMetadataId,

Good bot
Not wrong but out of scope a little bit

charlesBochet · 2025-02-17T23:14:21Z

...twenty-front/src/modules/object-record/record-index/components/RecordIndexContainerGater.tsx

+  width: 100%;
+`;
+
+export const RecordIndexContainerGater = () => {


Adding this "Gater" to introduce an intermediate level to avoid re-renders

I don't think the naming is appropriate because there's no conditionnal rendering here. Also maybe we should let backend naming for backend related stuff.

charlesBochet · 2025-02-17T23:15:24Z

...rc/modules/object-record/record-index/components/RecordIndexLoadBaseOnContextStoreEffect.tsx

+import { useRecoilValue } from 'recoil';
+import { isDefined } from 'twenty-shared';
+
+export const RecordIndexLoadBaseOnContextStoreEffect = () => {


I have introduced this effect for now but I have voluntarily put everything into a loadRecordIndexStates hook. I think we would like to load this from the PageChangeEffect at some point. open to discussion

Seems like a good idea if it's based on the URL.

Also should we use BasedOn in the naming ?

The naming loaded, load seems to be duplicating some concept that could already be described as context store current view no ?

charlesBochet · 2025-02-17T23:15:57Z

packages/twenty-front/src/modules/object-record/record-index/hooks/useLoadRecordIndexStates.ts

+import { isDefined } from 'twenty-shared';
+import { isDeeplyEqual } from '~/utils/isDeeplyEqual';
+
+export const useLoadRecordIndexStates = () => {


the monster, which is actually code moved from different places. This code should disappear as we remove all intermediate states

charlesBochet · 2025-02-17T23:16:22Z

packages/twenty-front/src/modules/object-record/record-table/hooks/useSetTableColumns.ts

+import { useRecoilCallback } from 'recoil';
+import { isDeeplyEqual } from '~/utils/isDeeplyEqual';
+
+export const useSetTableColumns = () => {


moved from useRecordTable

charlesBochet · 2025-02-17T23:16:58Z

packages/twenty-front/src/modules/prefetch/constants/PrefetchConfig.ts

@@ -12,10 +11,6 @@ export const PREFETCH_CONFIG: Record<
    operationSignatureFactory: RecordGqlOperationSignatureFactory;
  }
 > = {
-  ALL_VIEWS: {


removing ALL Views from here. Prefetch was a bad idea overall

charlesBochet · 2025-02-17T23:20:05Z

.../twenty-front/src/modules/favorites/states/selectors/favoriteViewsWithMinimalDataSelector.ts

+import { View } from '@/views/types/View';
+import { selector } from 'recoil';
+
+export const favoriteViewsWithMinimalDataSelector = selector<


selector designed for favorites that need to have minimal info on views (to avoid re-renders too)

Could we use viewsForMenuItem or something that tells us what the fields curation is for ?

charlesBochet · 2025-02-18T01:34:43Z

==> Fixed

prastoin · 2025-02-18T08:50:39Z

packages/twenty-front/src/modules/action-menu/components/RecordShowActionMenu.tsx

-  const contextStoreCurrentObjectMetadataId = useRecoilComponentValueV2(
-    contextStoreCurrentObjectMetadataIdComponentState,
+  const contextStoreCurrentObjectMetadataItem = useRecoilComponentValueV2(
+    contextStoreCurrentObjectMetadataItemComponentState,


Question: Went from an atomic ID listener to the whole objectMetadataItem ?

yes, because the DevXP was painful (always have to find the objectMetadataItem from the Id). Another possibility is to create a hook or a selector on top of that. Which would actually be better I believe

...nt/src/modules/prefetch/states/selector/prefetchViewsFromObjectMetadataItemFamilySelector.ts

prastoin · 2025-02-18T09:49:51Z

.../src/modules/prefetch/states/selector/prefetchViewIdsFromObjectMetadataItemFamilySelector.ts

+      ({ get }) => {
+        const views = get(prefetchViewsState);
+        return views
+          .filter((view) => view.objectMetadataId === objectMetadataItemId)


Recoil newbie question: Could we and is it recommended to use nested recoil selectors ? ( in this case overkill but could have used prefetchViewsFromObjectMetadataItemFamilySelector)

Yes we can call other selectors in selectors, as long as we have the necessary parameters available, what we cannot do is call hooks in selectors.

Would it make sense here to use the prefetchViewsFromObjectMetadataItemFamilySelector selector in order to retrieve objectMetadaItem views ? I feel like it's redundant states access even if very cheap to perform

lucasbordeau · 2025-02-18T09:50:32Z

.../src/modules/action-menu/actions/record-actions/components/RecordActionMenuEntriesSetter.tsx

-
-  const objectMetadataItem = objectMetadataItems.find(
-    (item) => item.id === contextStoreCurrentObjectMetadataId,
+  const contextStoreCurrentObjectMetadataItem = useRecoilComponentValueV2(


Is there any risk that this state get desynchronized from the objectMetadataItems state ? Could this be a component selector instead ? Based on the id ?

prastoin · 2025-02-18T09:55:08Z

packages/twenty-front/src/modules/prefetch/components/PrefetchRunViewQueryEffect.tsx

+    ),
+  });
+
+  const { records } = useFindManyRecords({


Suggestion: Unless I'm mistaken passing View predicate to useFindManyRecords could avoid having to force casting in below useEffect

const { records: views } = useFindManyRecords<View>({ //... useEffect(() => { if (isDefined(views) && !isPersistingViewFields) { setPrefetchViewsState(views); } }, [isPersistingViewFields, records, setPrefetchViewsState]);

prastoin · 2025-02-18T10:48:55Z

.../twenty-front/src/modules/views/view-picker/components/ViewPickerFavoriteFoldersDropdown.tsx

-  const view = views.find((view) => view.id === viewPickerReferenceViewId);
+  const view = useRecoilValue(
+    prefetchViewFromViewIdFamilySelector({
+      viewId: viewPickerReferenceViewId ?? '',


Remark: Not the first occurence of passing an empty string to this selector
Maybe we should create a specific one or mutate exsiting for it to accept optional viewId ?

I think I would prefer introduce a hook useContextStoreViewIdOrThrow to force the caller to take a decision

lucasbordeau · 2025-02-18T10:49:59Z

packages/twenty-front/src/modules/ui/field/display/components/BooleanDisplay.tsx

@@ -15,7 +13,7 @@ type BooleanDisplayProps = {
 };

 export const BooleanDisplay = ({ value }: BooleanDisplayProps) => {
-  if (!isDefined(value)) {
+  if (value === null || value === undefined) {


Why ? Is it different than !isDefined ?

because of storybook build failing:
BooleanDisplay is in vyw + using twenty-shared, this seems to be an issue currently. We should investigate but I'm protecting the devXP

What kind of error had you when building over this ? Emotion.styled is not a function one ?

prastoin · 2025-02-18T10:50:52Z

packages/twenty-front/src/modules/views/view-picker/hooks/useCloseAndResetViewPicker.ts

-    setViewPickerIsPersisting,
-    setViewPickerMode,
-  ]);
+    closeDropdown(VIEW_PICKER_KANBAN_FIELD_DROPDOWN_ID);


Note: We should strictly typed as much as possible any static occurences of specificComponentId and gather them all in a Literal type or tsc enum, as I guess there're also dynamic dropdown ids

In order to keep consistency between each declarated const like VIEW_PICKER_KANBAN_FIELD_DROPDOWN_ID and bind them to a single TypeScript source of thruth

Something like:

const closeDropdown: (specificComponentId: DeterministicDropdownId | string) => void

we don't want to type from the inside as the dropdown is a lower layer component that should not take dependency on the app.
In this case we could introduce an enum
ViewDropdownsInstanceId to help the devXp

we don't want to type from the inside as the dropdown is a lower layer component that should not take dependency on the app.

We could add an abstraction layer over it scoped to the app or add a TypeScript generic to the closeDropdown to make it as generic as possible

lucasbordeau · 2025-02-18T10:53:50Z

packages/twenty-front/src/modules/views/hooks/useGetViewFromPrefetchState.ts

+import { prefetchViewFromViewIdFamilySelector } from '@/prefetch/states/selector/prefetchViewFromViewIdFamilySelector';
+import { useRecoilCallback } from 'recoil';
+
+export const useGetViewFromPrefetchState = () => {


useGetPrefetchedView ?

lucasbordeau

For naming I would suggest :

Prefetched when we talk about values
Removing state suffix

prastoin · 2025-02-18T11:06:35Z

packages/twenty-front/src/modules/views/hooks/useUpsertCombinedViewSorts.ts

@@ -53,7 +52,7 @@ export const useUpsertCombinedViewSorts = (viewBarComponentId?: string) => {
          return;
        }

-        const currentView = await getViewFromCache(currentViewId);
+        const currentView = await getViewFromPrefetchState(currentViewId);


Nitpick: Useless await

prastoin · 2025-02-18T11:08:02Z

packages/twenty-front/src/modules/views/hooks/useUpsertCombinedViewFilters.ts

@@ -53,7 +53,7 @@ export const useUpsertCombinedViewFilters = (viewBarComponentId?: string) => {
          return;
        }

-        const currentView = await getViewFromCache(currentViewId);
+        const currentView = await getViewFromPrefetchState(currentViewId);


Nitpick: useless await

prastoin · 2025-02-18T11:08:29Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewGroups.ts

@@ -31,7 +31,7 @@ export const useSaveCurrentViewGroups = (viewBarComponentId?: string) => {
          return;
        }

-        const view = await getViewFromCache(currentViewId);
+        const view = await getViewFromPrefetchState(currentViewId);


Nitpick: useless await

prastoin · 2025-02-18T11:08:49Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewGroups.ts

@@ -31,7 +31,7 @@ export const useSaveCurrentViewGroups = (viewBarComponentId?: string) => {
          return;
        }

-        const view = await getViewFromCache(currentViewId);
+        const view = await getViewFromPrefetchState(currentViewId);

        if (isUndefinedOrNull(view)) {


Nitpick: Could now become isDefined

prastoin · 2025-02-18T11:09:22Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewFiltersAndSorts.ts

@@ -77,7 +77,7 @@ export const useSaveCurrentViewFiltersAndSorts = (
          unsavedToUpsertViewSortsCallbackState({ viewId }),
        );

-        const view = await getViewFromCache(viewId);
+        const view = await getViewFromPrefetchState(viewId);


Nitpick: useless await

prastoin · 2025-02-18T11:09:33Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewFiltersAndSorts.ts

@@ -123,7 +123,7 @@ export const useSaveCurrentViewFiltersAndSorts = (
          unsavedToUpsertViewFilterGroupsCallbackState({ viewId }),
        );

-        const view = await getViewFromCache(viewId);
+        const view = await getViewFromPrefetchState(viewId);


Nitpick: useless await

prastoin · 2025-02-18T11:09:46Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewFields.ts


-        const view = await getViewFromCache(currentViewId);
+        const view = await getViewFromPrefetchState(currentViewId);


Nitpick: useless await

prastoin · 2025-02-18T11:11:45Z

packages/twenty-front/src/modules/views/hooks/useGetCurrentView.ts

-    views,
+  const viewsOnCurrentObject = useRecoilValue(
+    prefetchViewsFromObjectMetadataItemFamilySelector({
+      objectMetadataItemId: objectMetadataItem.id,


Nitpick: Unfortunately id here is typed as any ( not for too long ), would stick to ?? '' to keep consistency and avoid future tsc errors

prastoin · 2025-02-18T11:11:56Z

packages/twenty-front/src/modules/views/hooks/useGetCurrentView.ts

-    instanceId,
+  const indexViewId = useRecoilValue(
+    prefetchIndexViewIdFromObjectMetadataItemFamilySelector({
+      objectMetadataItemId: objectMetadataItem.id,


Nitpick: Unfortunately id here is typed as any ( not for too long ), would stick to ?? '' to keep consistency and avoid future tsc errors

prastoin · 2025-02-18T11:13:32Z

packages/twenty-front/src/modules/views/hooks/useDeleteCombinedViewFilters.ts

@@ -50,7 +50,7 @@ export const useDeleteCombinedViewFilters = (viewBarComponentId?: string) => {
          return;
        }

-        const currentView = await getViewFromCache(currentViewId);
+        const currentView = await getViewFromPrefetchState(currentViewId);


Nitpick: Useless await

prastoin · 2025-02-18T11:15:51Z

packages/twenty-front/src/modules/views/hooks/useCreateViewFromCurrentView.ts

-      getViewFromCache,
-      isPersistingViewFieldsCallbackState,
+      findManyRecords,
+      objectMetadataItem?.fields,


React newbie concerns: optional and specific property ? Could this lead to re-render issue ?

prastoin · 2025-02-18T11:22:58Z

...wenty-front/src/modules/navigation/hooks/useSetLastVisitedViewForObjectMetadataNamePlural.ts

@@ -54,7 +49,7 @@ export const useSetLastVisitedViewForObjectMetadataNamePlural = () => {
          });
        }
      },
-    [findManyRecords, views],
+    [],


praise: yummy

prastoin · 2025-02-18T11:23:24Z

...y-front/src/modules/object-metadata/components/NavigationDrawerItemForObjectMetadataItem.tsx

-    views,
+  const views = useRecoilValue(
+    prefetchViewsFromObjectMetadataItemFamilySelector({
+      objectMetadataItemId: objectMetadataItem.id,


Nitpick: typed as any would ?? ''

prastoin · 2025-02-18T11:23:54Z

...ty-front/src/modules/object-record/advanced-filter/hooks/useDeleteCombinedViewFilterGroup.ts

@@ -56,7 +55,7 @@ export const useDeleteCombinedViewFilterGroup = (
          return;
        }

-        const currentView = await getViewFromCache(currentViewId);
+        const currentView = await getViewFromPrefetchState(currentViewId);


Nitpick: useless await

prastoin · 2025-02-18T11:24:51Z

packages/twenty-front/src/modules/object-record/record-index/hooks/useHandleRecordGroupField.ts

@@ -120,7 +113,7 @@ export const useHandleRecordGroupField = ({
          return;
        }

-        const view = await getViewFromCache(currentViewId);
+        const view = await getViewFromPrefetchState(currentViewId);


Nitpick: useless await

prastoin · 2025-02-18T11:25:02Z

packages/twenty-front/src/modules/object-record/record-index/hooks/useHandleRecordGroupField.ts

@@ -36,7 +29,7 @@ export const useHandleRecordGroupField = ({
          return;
        }

-        const view = await getViewFromCache(currentViewId);
+        const view = await getViewFromPrefetchState(currentViewId);


Nitpick: useless await

prastoin · 2025-02-18T11:26:07Z

...s/object-record/record-show/record-detail-section/components/RecordDetailRelationSection.tsx

-      view.objectMetadataId === relationObjectMetadataItem.id,
+  const indexViewId = useRecoilValue(
+    prefetchIndexViewIdFromObjectMetadataItemFamilySelector({
+      objectMetadataItemId: relationObjectMetadataItem.id,


praise: nice

prastoin

Gg a lot of work, re-rendering master !
I think we should merge this after fixing critical points raised by reviews, and handle nitpicks afterwards to avoid any conflicts and friction due to the PR size

prastoin

Stamping this to get to the release ! We will handle nitpicks and non-critical feedbacks afterwards

charlesBochet added 3 commits February 17, 2025 18:06

Refacto views

9ad87d4

Fix

2c95605

Fix

ec415ef

greptile-apps bot reviewed Feb 17, 2025

View reviewed changes

charlesBochet added 2 commits February 17, 2025 22:15

Continue refactoring

3f29d16

Fix

355035a

charlesBochet commented Feb 17, 2025

View reviewed changes

charlesBochet added 3 commits February 18, 2025 00:20

Fix lint

b32c205

Refactor useGetViewFromPrefetchState

e081da6

Fix lint

16309f1

charlesBochet and others added 4 commits February 18, 2025 09:05

Fix

6b4928e

Fix tests

13b5a5e

Fix lint

42daab9

Merge branch 'main' into refacto-views

f247fc0