reset main context store viewId and current view type on settings page #11206

ehconitin · 2025-03-26T18:01:17Z

…store-on-settings-page

greptile-apps

PR Summary

This PR implements a reset mechanism for the main context store when navigating to settings pages, ensuring proper state management and view type handling across the application.

Added new MainContextStoreSettingsSideEffect component to reset viewId and set view type to 'Settings'
Modified useContextStoreObjectMetadataItemOrThrow to prevent errors on settings pages when metadata is undefined
Added 'Settings' to ContextStoreViewType enum and updated getActionViewType to return null for settings views
Updated MainContextStoreProvider to conditionally render settings-specific components
Consider adding validation or warning for undefined objectMetadataItem in settings pages to prevent potential issues

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

greptile-apps · 2025-03-26T18:01:46Z

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

+  // not sure what should happen here, should we return null when page name and object metadata item are not defined as it was before?
+  // or should we return the provider with the default/reset values? -- ie like one of settings side effect?
+
+  // if (
+  //   !isDefined(pageName) ||
+  //   !isDefined(objectMetadataItem) ||
+  //   isSettingsPage
+  // ) {
+  //   return <MainContextStoreSettingsSideEffect />;
+  // }


logic: This commented code block contains an unresolved question about behavior. Need to make a clear decision on whether to return null or reset values before merging.

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

...ges/twenty-front/src/modules/context-store/components/MainContextStoreSettingsSideEffect.tsx

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

packages/twenty-front/src/modules/context-store/types/ContextStoreViewType.ts

charlesBochet · 2025-03-26T20:42:57Z

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

  const { objectMetadataItem } = useObjectMetadataItemById({
    objectId: objectMetadataItemId ?? '',
  });

-  if (!objectMetadataItem) {
+  if (!objectMetadataItem && !isSettingsPage) {


no, this useContextStoreObjectMetadataItemOrThrow.ts should not be loaded if we are on a settings page. The higher the edge cases are treated in the hieararchy, the happier we are :)

packages/twenty-front/src/modules/action-menu/actions/utils/getActionViewType.ts

...ges/twenty-front/src/modules/context-store/components/MainContextStoreSettingsSideEffect.tsx

charlesBochet

Thanks @ehconitin, see comments :)

…store-on-settings-page

ehconitin · 2025-03-27T09:16:56Z

@charlesBochet
Regarding handling edge cases higher in the hierarchy: #11206 (comment)
I've run into an issue where the CommandMenu component throws an error on settings pages because it unconditionally calls useContextStoreObjectMetadataItemOrThrow.
Should we add a CommandMenuSettingsPage component and a Settings option to the CommandMenuPages enum? I've tried this approach, and while the context is set correctly when navigating from another page to settings, we still get an error when opening the command menu directly on a settings page.
Any guidance on how specialized command menu pages are typically set based on the current route? I don't see any effects or logic that would automatically set the appropriate command menu page based on the current route for other specialized pages.
Or would an even simpler solution be to add a check in the CommandMenu component itself? Though this doesn't seem to align with the existing code patterns and your feedback about handling edge cases higher in the hierarchy.

2025-03-27.14-42-37.mov

…store-on-settings-page

charlesBochet · 2025-03-28T07:46:37Z

@ehconitin, I feel we should unmount the CommandMenu in the Settings for now :)

Edit: let's keep it :p

charlesBochet · 2025-03-28T07:48:30Z

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

@@ -67,7 +69,10 @@ export const MainContextStoreProvider = () => {

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

-  if (!isDefined(pageName) || !isDefined(objectMetadataItem)) {
+  if (
+    !isSettingsPage &&


I would make pageName = 'settings' in case we are on a settingsPage, and remove this condition here

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

charlesBochet · 2025-03-28T08:15:40Z

@ehconitin I think Thomas actually wants it with the page shortcuts only.

Previously, we assumed that the command menu was always loaded in a context with objectMetadataItem set.
Changing this hypothesis, we need to authorize both. Here it's quite easy to fix by stopping throwing and do the work manually. If it would get too complex, I would push to split the components at CommandMenuRouter level and introduce SettingsCommandMenu alongside with the existing CommandMenu

charlesBochet · 2025-03-28T08:16:02Z

packages/twenty-front/src/modules/command-menu/components/CommandMenu.tsx

@@ -42,8 +43,12 @@ export const CommandMenu = () => {
      'command-menu-previous',
    );

-  const { objectMetadataItem: currentObjectMetadataItem } =
-    useContextStoreObjectMetadataItemOrThrow();
+  const objectMetadataItemId = useRecoilComponentValueV2(


@ehconitin here

charlesBochet · 2025-03-28T08:16:45Z

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

+    viewId,
+  ]);
+
+  useEffect(() => {


splitting the effects to keep the logic simple and atomic

github-actions · 2025-03-28T08:28:03Z

Thanks @ehconitin for your contribution!
This marks your 122nd PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

ehconitin added 2 commits March 26, 2025 23:29

fix

cb8eb7b

Merge remote-tracking branch 'upstream/main' into reset-main-context-…

3c79894

…store-on-settings-page

ehconitin requested a review from bosiraphael March 26, 2025 18:01

ehconitin self-assigned this Mar 26, 2025

greptile-apps bot reviewed Mar 26, 2025

View reviewed changes

charlesBochet reviewed Mar 26, 2025

View reviewed changes

packages/twenty-front/src/modules/context-store/components/MainContextStoreProvider.tsx Outdated Show resolved Hide resolved

charlesBochet reviewed Mar 26, 2025

View reviewed changes

packages/twenty-front/src/modules/context-store/types/ContextStoreViewType.ts Outdated Show resolved Hide resolved

charlesBochet reviewed Mar 26, 2025

View reviewed changes

packages/twenty-front/src/modules/action-menu/actions/utils/getActionViewType.ts Outdated Show resolved Hide resolved

charlesBochet reviewed Mar 26, 2025

View reviewed changes

...ges/twenty-front/src/modules/context-store/components/MainContextStoreSettingsSideEffect.tsx Outdated Show resolved Hide resolved

charlesBochet requested changes Mar 26, 2025

View reviewed changes

charlesBochet added the -PR: awaiting author label Mar 26, 2025

ehconitin added 2 commits March 27, 2025 13:08

Merge remote-tracking branch 'upstream/main' into reset-main-context-…

8bbd6e5

…store-on-settings-page

charles review

1cd6a4f

Merge remote-tracking branch 'upstream/main' into reset-main-context-…

9dbfea1

…store-on-settings-page

ehconitin added -PR: wip -PR: awaiting review and removed -PR: awaiting author -PR: wip labels Mar 27, 2025

ehconitin requested a review from charlesBochet March 27, 2025 09:21

charlesBochet reviewed Mar 28, 2025

View reviewed changes

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

charlesBochet added 3 commits March 28, 2025 08:53

Merge branch 'main' into reset-main-context-store-on-settings-page

b219da7

Refactor

5f2a34a

Fix

59ea8ed

charlesBochet reviewed Mar 28, 2025

View reviewed changes

charlesBochet approved these changes Mar 28, 2025

View reviewed changes

charlesBochet merged commit c294d96 into twentyhq:main Mar 28, 2025
45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reset main context store viewId and current view type on settings page #11206

reset main context store viewId and current view type on settings page #11206

ehconitin commented Mar 26, 2025 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Mar 26, 2025

charlesBochet Mar 26, 2025

charlesBochet left a comment

ehconitin commented Mar 27, 2025 •

edited

Loading

charlesBochet commented Mar 28, 2025 •

edited

Loading

charlesBochet Mar 28, 2025 •

edited

Loading

charlesBochet commented Mar 28, 2025

charlesBochet Mar 28, 2025

charlesBochet Mar 28, 2025

github-actions bot commented Mar 28, 2025

reset main context store viewId and current view type on settings page #11206

reset main context store viewId and current view type on settings page #11206

Conversation

ehconitin commented Mar 26, 2025 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Mar 26, 2025

Choose a reason for hiding this comment

charlesBochet Mar 26, 2025

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

ehconitin commented Mar 27, 2025 • edited Loading

charlesBochet commented Mar 28, 2025 • edited Loading

charlesBochet Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

charlesBochet commented Mar 28, 2025

charlesBochet Mar 28, 2025

Choose a reason for hiding this comment

charlesBochet Mar 28, 2025

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2025

ehconitin commented Mar 26, 2025 •

edited

Loading

ehconitin commented Mar 27, 2025 •

edited

Loading

charlesBochet commented Mar 28, 2025 •

edited

Loading

charlesBochet Mar 28, 2025 •

edited

Loading