Simplify last visited #10259

charlesBochet · 2025-02-17T14:03:12Z

In this PR, I'm simplifying the lastVisitedView / Object logic:

removing fallback logic as it's not useful
splitting hooks into smaller hooks (to avoir re-renders)
removing componentState on those states that are global

greptile-apps

PR Summary

This PR significantly simplifies the navigation state management system by removing and consolidating last visited functionality.

Removes useLastVisitedObjectMetadataItem hook and related selectors, replacing with simpler useSetLastVisitedObjectMetadataId hook
Simplifies useLastVisitedView to only provide getter functionality, removing workspace-specific state management
Changes state management from createComponentState to createState in lastVisitedObjectMetadataItemIdState.ts, modifying type from Record<string, string> to string | null
Introduces potential issue in useSetLastVisitedViewForObjectMetadataNamePlural using incorrect state for view tracking
Removes accidental debug console.log in RecordIndexContainer.tsx

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

greptile-apps · 2025-02-17T14:04:54Z

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

+  console.log(
+    'mainContextStoreComponentInstanceIdToSet',
+    mainContextStoreComponentInstanceIdToSet,
+  );


style: console.log statement should be removed before merging to production

Suggested change

console.log(

'mainContextStoreComponentInstanceIdToSet',

mainContextStoreComponentInstanceIdToSet,

);

greptile-apps · 2025-02-17T14:06:36Z

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

+        const lastVisitedViewPerObjectMetadataItem = snapshot
+          .getLoadable(lastVisitedObjectMetadataItemIdState)
+          .getValue();


logic: incorrect state usage - lastVisitedObjectMetadataItemIdState should contain IDs, not views. Should use lastVisitedViewPerObjectMetadataItemState instead to get the last visited view

Suggested change

const lastVisitedViewPerObjectMetadataItem = snapshot

.getLoadable(lastVisitedObjectMetadataItemIdState)

.getValue();

const lastVisitedViewPerObjectMetadataItem = snapshot

.getLoadable(lastVisitedViewPerObjectMetadataItemState)

.getValue();

greptile-apps · 2025-02-17T14:06:37Z

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

+          set(lastVisitedViewPerObjectMetadataItemState, {
+            [objectMetadataItem.id]: viewId,
+          });


logic: this overwrites the entire state object instead of merging with existing entries. Should spread existing state: set(lastVisitedViewPerObjectMetadataItemState, (prev) => ({ ...prev, [objectMetadataItem.id]: viewId }))

Suggested change

set(lastVisitedViewPerObjectMetadataItemState, {

[objectMetadataItem.id]: viewId,

});

set(lastVisitedViewPerObjectMetadataItemState, (prev) => ({

...prev,

[objectMetadataItem.id]: viewId,

}));

greptile-apps · 2025-02-17T14:07:25Z

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

@@ -187,6 +187,8 @@ export const RecordIndexContainer = () => {
    FeatureFlagKey.IsCommandMenuV2Enabled,
  );

+  console.log(columnDefinitions);


style: Remove debug console.log statement before merging to production

Suggested change

console.log(columnDefinitions);

greptile-apps · 2025-02-17T14:09:30Z

...enty-front/src/modules/settings/data-model/object-details/components/tabs/ObjectSettings.tsx

@@ -143,19 +131,6 @@ export const ObjectSettings = ({ objectMetadataItem }: ObjectSettingsProps) => {

      formConfig.reset(undefined, { keepValues: true });


logic: reset is called before navigation, could lead to state inconsistency if navigation fails

charlesBochet · 2025-02-17T14:37:07Z

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

+import { isDefined } from 'twenty-shared';
+
+export const useSetLastVisitedViewForObjectMetadataNamePlural = () => {
+  const { records: views, findManyRecords } = useLazyPrefetchedData<View>(


introducing this hook but I think we should use a state and stop reading from apollo cache here

prastoin · 2025-02-17T14:41:10Z

@greptileai trigger

greptile-apps

PR Summary

(updates since last review)

This PR continues to refine the navigation state management system with additional changes focused on view handling and prefetching. Here are the key new changes:

Added new useLazyPrefetchedData hook in prefetch/hooks/useLazyPrefetchData.ts for optimized data loading with operation signature factory support
Modified MainContextStoreProvider to include robust view fallback logic using indexView when no view ID is available
Removed redundant state checks and optional chaining in useSetLastVisitedViewForObjectMetadataNamePlural hook
Simplified SettingsObjectSummaryCard by removing useLastVisitedView hook and its fallback logic
Fixed state update in useSetLastVisitedViewForObjectMetadataNamePlural where it overwrites entire state instead of merging entries

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

greptile-apps · 2025-02-17T14:42:48Z

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

  const navigationPath = getAppPath(
    AppPath.RecordIndexPage,
    { objectNamePlural: objectMetadataItem.namePlural },
-    viewId ? { viewId } : undefined,
+    lastVisitedViewId ? { viewId: lastVisitedViewId } : undefined,
  );


logic: navigationPath now uses lastVisitedViewId while active states use contextStoreCurrentViewId, which could cause inconsistent UI state

greptile-apps · 2025-02-17T14:43:07Z

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

+    setLastVisitedViewForObjectMetadataNamePlural({
+      objectNamePlural: objectMetadataItem.namePlural,
+      viewId: viewId,
+    });


style: setLastVisitedViewForObjectMetadataNamePlural is called unconditionally on every effect run. Consider adding a condition to only update when values change.

greptile-apps · 2025-02-17T14:43:08Z

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

+    setLastVisitedObjectMetadataId({
+      objectMetadataItemId: objectMetadataItem.id,
+    });


style: setLastVisitedObjectMetadataId is called unconditionally on every effect run. Consider adding a condition to only update when the ID changes.

greptile-apps · 2025-02-17T14:45:14Z

packages/twenty-front/src/modules/prefetch/hooks/useLazyPrefetchData.ts

+  const recordGqlFields =
+    operationSignatureFactory({ objectMetadataItem }).fields ?? filter;


logic: The nullish coalescing operator here could be problematic if operationSignatureFactory().fields is an empty object - it would not fall back to the filter. Consider checking for empty object as well.

prastoin

Nice refactor ! Only nitpicks

prastoin · 2025-02-17T14:44:09Z

packages/twenty-front/src/modules/navigation/hooks/useOpenSettings.ts

    isNavigationDrawerExpandedState,
  );
-  const [, setCurrentMobileNavigationDrawer] = useRecoilState(
+  const setCurrentMobileNavigationDrawer = useSetRecoilState(


Praise: Nice

prastoin · 2025-02-17T14:46:10Z

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

+      }) => {
+        await findManyRecords();
+
+        const view = views.find((view: View) => view.id === viewId);


Nitpick: Redundant arg typing

prastoin · 2025-02-17T14:48:47Z

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

+          lastVisitedViewPerObjectMetadataItem?.[objectMetadataItem?.id];
+
+        if (isDefined(objectMetadataItem) && lastVisitedViewId !== viewId) {
+          set(lastVisitedViewPerObjectMetadataItemState, {


Nitpick: Could avoid making findManyRecords each run

prastoin · 2025-02-17T14:51:30Z

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

+  const mainContextStoreComponentInstanceId = useRecoilValue(
+    mainContextStoreComponentInstanceIdState,
+  );

-  const lastVisitedViewId = getLastVisitedViewIdFromObjectMetadataItemId(
-    objectMetadataItem.id,
+  const contextStoreCurrentViewId = useRecoilComponentValueV2(
+    contextStoreCurrentViewIdComponentState,
+    mainContextStoreComponentInstanceId,
  );


Nitpick: ~~Could be unused ?~~

prastoin · 2025-02-17T16:20:22Z

packages/twenty-front/src/modules/views/components/ViewBarRecordFilterEffect.tsx

-  const { filterableFieldMetadataItems } =
-    useFilterableFieldMetadataItemsInRecordIndexContext();
+  const { filterableFieldMetadataItems } = useFilterableFieldMetadataItems(
+    objectMetadataItem?.id,


Remark: We should not use any on the following fields

export type Scalars = { ID: { input: string; output: string; } String: { input: string; output: string; } Boolean: { input: boolean; output: boolean; } Int: { input: number; output: number; } Float: { input: number; output: number; } /** Cursor for paging through collections */ ConnectionCursor: { input: any; output: any; } /** A date-time string at UTC, such as 2019-12-03T09:54:33Z, compliant with the date-time format. */ DateTime: { input: any; output: any; } /** The `JSON` scalar type represents JSON values as specified by [ECMA-404](http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf). */ JSON: { input: any; output: any; } /** The `JSONObject` scalar type represents JSON objects as specified by [ECMA-404](http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf). */ JSONObject: { input: any; output: any; } /** A UUID scalar type */ UUID: { input: unknown; output: unknown; } /** The `Upload` scalar type represents a file upload. */ Upload: { input: any; output: any; } };

Using unknown on UUID shows possible runtime error here 431 tsc-errors
About to create an issue to address this

charlesBochet added 2 commits February 17, 2025 15:00

Simplify last visited object

10bc82b

Fix

386eeff

greptile-apps bot reviewed Feb 17, 2025

View reviewed changes

charlesBochet added 3 commits February 17, 2025 15:20

Fix

d6310fb

Fix

2450f94

Fix

fdbc063

charlesBochet commented Feb 17, 2025

View reviewed changes

Fixe

bf0b770

greptile-apps bot reviewed Feb 17, 2025

View reviewed changes

Fix

7c7abf0

prastoin self-requested a review February 17, 2025 14:55

prastoin reviewed Feb 17, 2025

View reviewed changes

Fix

bb9dd4b

prastoin approved these changes Feb 17, 2025

View reviewed changes

charlesBochet merged commit 5b4cb4b into main Feb 17, 2025
47 checks passed

charlesBochet deleted the simplify-last-visited branch February 17, 2025 16:27

etiennejouan mentioned this pull request Feb 17, 2025

[Bug] Fix export action twentyhq/core-team-issues#407

Closed

prastoin mentioned this pull request Feb 18, 2025

[REFACTOR] ObjectMetadataItem Scalars twentyhq/core-team-issues#445

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify last visited #10259

Simplify last visited #10259

charlesBochet commented Feb 17, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Feb 17, 2025

greptile-apps bot Feb 17, 2025

greptile-apps bot Feb 17, 2025

greptile-apps bot Feb 17, 2025

greptile-apps bot Feb 17, 2025

charlesBochet Feb 17, 2025

prastoin commented Feb 17, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Feb 17, 2025

greptile-apps bot Feb 17, 2025

greptile-apps bot Feb 17, 2025

greptile-apps bot Feb 17, 2025

prastoin left a comment

prastoin Feb 17, 2025

prastoin Feb 17, 2025

prastoin Feb 17, 2025

prastoin Feb 17, 2025

prastoin Feb 17, 2025

		@@ -143,19 +131,6 @@ export const ObjectSettings = ({ objectMetadataItem }: ObjectSettingsProps) => {

		formConfig.reset(undefined, { keepValues: true });

		const recordGqlFields =
		operationSignatureFactory({ objectMetadataItem }).fields ?? filter;

Simplify last visited #10259

Simplify last visited #10259

Conversation

charlesBochet commented Feb 17, 2025 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Feb 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 17, 2025

Choose a reason for hiding this comment

charlesBochet Feb 17, 2025

Choose a reason for hiding this comment

prastoin commented Feb 17, 2025 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Feb 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 17, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 17, 2025

Choose a reason for hiding this comment

prastoin left a comment

Choose a reason for hiding this comment

prastoin Feb 17, 2025

Choose a reason for hiding this comment

prastoin Feb 17, 2025

Choose a reason for hiding this comment

prastoin Feb 17, 2025

Choose a reason for hiding this comment

prastoin Feb 17, 2025

Choose a reason for hiding this comment

prastoin Feb 17, 2025

Choose a reason for hiding this comment

charlesBochet commented Feb 17, 2025 •

edited

Loading

prastoin commented Feb 17, 2025 •

edited

Loading