Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify last visited #10259

Merged
merged 8 commits into from
Feb 17, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import { MainContextStoreProviderEffect } from '@/context-store/components/MainContextStoreProviderEffect';
import { useLastVisitedView } from '@/navigation/hooks/useLastVisitedView';
import { objectMetadataItemsState } from '@/object-metadata/states/objectMetadataItemsState';
import { usePrefetchedData } from '@/prefetch/hooks/usePrefetchedData';
import { PrefetchKey } from '@/prefetch/types/PrefetchKey';
import { AppPath } from '@/types/AppPath';
import { View } from '@/views/types/View';
import { isNonEmptyString } from '@sniptt/guards';
import { useParams, useSearchParams } from 'react-router-dom';
import { useRecoilValue } from 'recoil';
Expand All @@ -14,7 +11,6 @@ import { useIsMatchingLocation } from '~/hooks/useIsMatchingLocation';

const getViewId = (
viewIdFromQueryParams: string | null,
indexView?: View,
lastVisitedViewId?: string,
) => {
if (isDefined(viewIdFromQueryParams)) {
Expand All @@ -25,10 +21,6 @@ const getViewId = (
return lastVisitedViewId;
}

if (isDefined(indexView)) {
return indexView.id;
}

return undefined;
};

Expand All @@ -48,8 +40,6 @@ export const MainContextStoreProvider = () => {
const [searchParams] = useSearchParams();
const viewIdQueryParam = searchParams.get('viewId');

const { records: views } = usePrefetchedData<View>(PrefetchKey.AllViews);

const objectMetadataItems = useRecoilValue(objectMetadataItemsState);

const objectMetadataItem = objectMetadataItems.find(
Expand All @@ -62,12 +52,7 @@ export const MainContextStoreProvider = () => {
objectMetadataItem?.namePlural ?? '',
);

const viewsOnCurrentObject = views.filter(
(view) => view.objectMetadataId === objectMetadataItem?.id,
);
const indexView = viewsOnCurrentObject.find((view) => view.key === 'INDEX');

const viewId = getViewId(viewIdQueryParam, indexView, lastVisitedViewId);
const viewId = getViewId(viewIdQueryParam, lastVisitedViewId);

const mainContextStoreComponentInstanceId = `${pageName}-${objectMetadataItem?.namePlural}-${viewId}`;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { contextStoreCurrentObjectMetadataIdComponentState } from '@/context-store/states/contextStoreCurrentObjectMetadataIdComponentState';
import { contextStoreCurrentViewIdComponentState } from '@/context-store/states/contextStoreCurrentViewIdComponentState';
import { mainContextStoreComponentInstanceIdState } from '@/context-store/states/mainContextStoreComponentInstanceId';
import { useLastVisitedObjectMetadataItem } from '@/navigation/hooks/useLastVisitedObjectMetadataItem';
import { useLastVisitedView } from '@/navigation/hooks/useLastVisitedView';
import { useSetLastVisitedObjectMetadataId } from '@/navigation/hooks/useSetLastVisitedObjectMetadataId';
import { useSetLastVisitedViewForObjectMetadataNamePlural } from '@/navigation/hooks/useSetLastVisitedViewForObjectMetadataNamePlural';
import { ObjectMetadataItem } from '@/object-metadata/types/ObjectMetadataItem';
import { useRecoilComponentStateV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentStateV2';
import { useEffect } from 'react';
Expand All @@ -17,20 +17,20 @@ export const MainContextStoreProviderEffect = ({
viewId: string;
objectMetadataItem: ObjectMetadataItem;
}) => {
console.log(
'mainContextStoreComponentInstanceIdToSet',
mainContextStoreComponentInstanceIdToSet,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
console.log(
'mainContextStoreComponentInstanceIdToSet',
mainContextStoreComponentInstanceIdToSet,
);

const [
mainContextStoreComponentInstanceId,
setMainContextStoreComponentInstanceId,
] = useRecoilState(mainContextStoreComponentInstanceIdState);

const { getLastVisitedViewIdFromObjectNamePlural, setLastVisitedView } =
useLastVisitedView();
const { setLastVisitedViewForObjectMetadataNamePlural } =
useSetLastVisitedViewForObjectMetadataNamePlural();

const { lastVisitedObjectMetadataItemId, setLastVisitedObjectMetadataItem } =
useLastVisitedObjectMetadataItem();

const lastVisitedViewId = getLastVisitedViewIdFromObjectNamePlural(
objectMetadataItem.namePlural,
);
const { setLastVisitedObjectMetadataId } =
useSetLastVisitedObjectMetadataId();

const [contextStoreCurrentViewId, setContextStoreCurrentViewId] =
useRecoilComponentStateV2(
Expand Down Expand Up @@ -60,33 +60,29 @@ export const MainContextStoreProviderEffect = ({
);
}

if (viewId !== lastVisitedViewId) {
setLastVisitedView({
objectNamePlural: objectMetadataItem.namePlural,
viewId: viewId,
});
}
setLastVisitedViewForObjectMetadataNamePlural({
objectNamePlural: objectMetadataItem.namePlural,
viewId: viewId,
});
Comment on lines +59 to +62
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (objectMetadataItem.id !== lastVisitedObjectMetadataItemId) {
setLastVisitedObjectMetadataItem(objectMetadataItem.namePlural);
}
setLastVisitedObjectMetadataId({
objectMetadataItemId: objectMetadataItem.id,
});
Comment on lines +64 to +66
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (contextStoreCurrentViewId !== viewId) {
setContextStoreCurrentViewId(viewId);
}
}, [
contextStoreCurrentObjectMetadataId,
contextStoreCurrentViewId,
lastVisitedObjectMetadataItemId,
lastVisitedViewId,
mainContextStoreComponentInstanceId,
mainContextStoreComponentInstanceIdToSet,
objectMetadataItem,
objectMetadataItem.namePlural,
setContextStoreCurrentObjectMetadataId,
setContextStoreCurrentViewId,
setLastVisitedObjectMetadataItem,
setLastVisitedView,
setLastVisitedObjectMetadataId,
setLastVisitedViewForObjectMetadataNamePlural,
setMainContextStoreComponentInstanceId,
viewId,
]);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { currentUserState } from '@/auth/states/currentUserState';
import { useLastVisitedObjectMetadataItem } from '@/navigation/hooks/useLastVisitedObjectMetadataItem';
import { lastVisitedObjectMetadataItemIdState } from '@/navigation/states/lastVisitedObjectMetadataItemIdState';
import { ObjectPathInfo } from '@/navigation/types/ObjectPathInfo';
import { useFilteredObjectMetadataItems } from '@/object-metadata/hooks/useFilteredObjectMetadataItems';
import { usePrefetchedData } from '@/prefetch/hooks/usePrefetchedData';
Expand All @@ -16,8 +16,9 @@ export const useDefaultHomePagePath = () => {
const { activeObjectMetadataItems, alphaSortedActiveObjectMetadataItems } =
useFilteredObjectMetadataItems();
const { records: views } = usePrefetchedData<View>(PrefetchKey.AllViews);
const { lastVisitedObjectMetadataItemId } =
useLastVisitedObjectMetadataItem();
const lastVisitedObjectMetadataItemId = useRecoilValue(
lastVisitedObjectMetadataItemIdState,
);

const getActiveObjectMetadataItemMatchingId = useCallback(
(objectMetadataId: string) => {
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,54 +1,15 @@
import { currentWorkspaceState } from '@/auth/states/currentWorkspaceState';
import { lastVisitedViewPerObjectMetadataItemStateSelector } from '@/navigation/states/selectors/lastVisitedViewPerObjectMetadataItemStateSelector';
import { lastVisitedViewPerObjectMetadataItemState } from '@/navigation/states/lastVisitedViewPerObjectMetadataItemState';
import { useFilteredObjectMetadataItems } from '@/object-metadata/hooks/useFilteredObjectMetadataItems';
import { extractComponentState } from '@/ui/utilities/state/component-state/utils/extractComponentState';
import { useRecoilState, useRecoilValue } from 'recoil';
import { isDefined } from 'twenty-shared';
import { useRecoilValue } from 'recoil';

export const useLastVisitedView = () => {
const currentWorkspace = useRecoilValue(currentWorkspaceState);
const scopeId = currentWorkspace?.id ?? '';

const lastVisitedViewPerObjectMetadataItemState = extractComponentState(
lastVisitedViewPerObjectMetadataItemStateSelector,
scopeId,
const lastVisitedViewPerObjectMetadataItem = useRecoilValue(
lastVisitedViewPerObjectMetadataItemState,
);

const [
lastVisitedViewPerObjectMetadataItem,
setLastVisitedViewPerObjectMetadataItem,
] = useRecoilState(lastVisitedViewPerObjectMetadataItemState);

const { findActiveObjectMetadataItemByNamePlural } =
useFilteredObjectMetadataItems();

const setFallbackForLastVisitedView = (objectMetadataItemId: string) => {
/* ...{} allows us to pass value as undefined to remove that particular key
even though param type is of type Record<string,string> */

setLastVisitedViewPerObjectMetadataItem({
...{},
[objectMetadataItemId]: undefined,
});
};

const setLastVisitedView = ({
objectNamePlural,
viewId,
}: {
objectNamePlural: string;
viewId: string;
}) => {
const objectMetadataItem =
findActiveObjectMetadataItemByNamePlural(objectNamePlural);

if (isDefined(objectMetadataItem)) {
setLastVisitedViewPerObjectMetadataItem({
[objectMetadataItem.id]: viewId,
});
}
};

const getLastVisitedViewIdFromObjectNamePlural = (
objectNamePlural: string,
) => {
Expand All @@ -65,9 +26,7 @@ export const useLastVisitedView = () => {
return lastVisitedViewPerObjectMetadataItem?.[objectMetadataItemId];
};
return {
setLastVisitedView,
getLastVisitedViewIdFromObjectNamePlural,
getLastVisitedViewIdFromObjectMetadataItemId,
setFallbackForLastVisitedView,
};
};
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { currentMobileNavigationDrawerState } from '@/navigation/states/currentMobileNavigationDrawerState';
import { isNavigationDrawerExpandedState } from '@/ui/navigation/states/isNavigationDrawerExpanded';
import { useRecoilState } from 'recoil';
import { useSetRecoilState } from 'recoil';

export const useOpenSettingsMenu = () => {
const [, setIsNavigationDrawerExpanded] = useRecoilState(
const setIsNavigationDrawerExpanded = useSetRecoilState(
isNavigationDrawerExpandedState,
);
const [, setCurrentMobileNavigationDrawer] = useRecoilState(
const setCurrentMobileNavigationDrawer = useSetRecoilState(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Praise: Nice

currentMobileNavigationDrawerState,
);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { lastVisitedObjectMetadataItemIdState } from '@/navigation/states/lastVisitedObjectMetadataItemIdState';
import { objectMetadataItemsState } from '@/object-metadata/states/objectMetadataItemsState';
import { useRecoilCallback } from 'recoil';
import { isDefined } from 'twenty-shared';

export const useSetLastVisitedObjectMetadataId = () => {
const setLastVisitedObjectMetadataId = useRecoilCallback(
({ set, snapshot }) =>
({ objectMetadataItemId }: { objectMetadataItemId: string }) => {
const objectMetadataItems = snapshot
.getLoadable(objectMetadataItemsState)
.getValue();

const objectMetadataItem = objectMetadataItems.find(
(item) => item.id === objectMetadataItemId,
);

const lastVisitedObjectMetadataItemId = snapshot
.getLoadable(lastVisitedObjectMetadataItemIdState)
.getValue();

if (
isDefined(objectMetadataItem) &&
lastVisitedObjectMetadataItemId !== objectMetadataItemId
) {
set(lastVisitedObjectMetadataItemIdState, objectMetadataItemId);
}
},
[],
);

return {
setLastVisitedObjectMetadataId,
};
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { lastVisitedObjectMetadataItemIdState } from '@/navigation/states/lastVisitedObjectMetadataItemIdState';
import { lastVisitedViewPerObjectMetadataItemState } from '@/navigation/states/lastVisitedViewPerObjectMetadataItemState';
import { objectMetadataItemsState } from '@/object-metadata/states/objectMetadataItemsState';
import { useRecoilCallback } from 'recoil';
import { isDefined } from 'twenty-shared';

export const useSetLastVisitedViewForObjectMetadataNamePlural = () => {
const setLastVisitedViewForObjectMetadataNamePlural = useRecoilCallback(
({ set, snapshot }) =>
({
objectNamePlural,
viewId,
}: {
objectNamePlural: string;
viewId: string;
}) => {
const objectMetadataItems = snapshot
.getLoadable(objectMetadataItemsState)
.getValue();

const objectMetadataItem = objectMetadataItems.find(
(item) => item.namePlural === objectNamePlural,
);

const lastVisitedViewPerObjectMetadataItem = snapshot
.getLoadable(lastVisitedObjectMetadataItemIdState)
.getValue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();


const lastVisitedViewId =
lastVisitedViewPerObjectMetadataItem?.[objectMetadataItem?.id];

if (isDefined(objectMetadataItem) && lastVisitedViewId !== viewId) {
set(lastVisitedViewPerObjectMetadataItemState, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Could avoid making findManyRecords each run

[objectMetadataItem.id]: viewId,
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
}));

}
},
[],
);

return {
setLastVisitedViewForObjectMetadataNamePlural,
};
};
Loading
Loading