[front] - refacto: connectors modals #9765

JulesBelveze · 2025-01-06T09:59:54Z

Description

This PR migrates "connector-related" modals to use the new Sheet component instead of Modal. Other than layout changes and adapting the existing code to fit the Sheet API the main difference is that we need to keep the Sheet mounted to observe the closing animation.

Note: deleted the front/components/data_source/RequestOrAddDataFromDataSourceModal.tsx file as it appeared unused.

Risk

Breaking UX

Deploy Plan

Deploy front-edge
Deploy front

devloai · 2025-01-06T09:59:58Z

Automatic code review is enabled for this repository. I will perform a review of this PR.

…nectorDataUpdatedModal - Transition to using Sheet component to handle the data sync progress interface instead of Modal - Prevent interactions outside Sheet when data sync is in progress to maintain focus on the process - Improve structure and readability of the component by grouping related elements within SheetContainer and SheetContent components

devloai

I've reviewed the connector modals refactoring PR. While the overall structure is good, there are a few important issues that need to be addressed:

There are several bugs in the Sheet component's onOpenChange handlers that could affect modal behavior
A performance optimization opportunity with useMemo for frequently used calculations

Please address these issues before merging. The bugs in particular could affect user experience.

devloai · 2025-01-06T10:01:49Z

front/components/ConnectorPermissionsModal.tsx

-  const canUpdatePermissions = PERMISSIONS_EDITABLE_CONNECTOR_TYPES.has(
-    connector.type
-  );
+  const isDataAvailable = !!connector && !!dataSource;


Consider extracting the isDataAvailable check into a useMemo hook since it's used in multiple places and depends on connector and dataSource props that could change:

Suggested change

const isDataAvailable = !!connector && !!dataSource;

const isDataAvailable = useMemo(() => !!connector && !!dataSource, [connector, dataSource]);

( i won't do that - useMemo will be more expensive than redoing the test on every render )

devloai · 2025-01-06T10:01:49Z

front/components/ConnectorPermissionsModal.tsx

-            )}
-          </div>
-          {OptionsComponent && plan && (
+      <Sheet open={modalToShow === "selection"} onOpenChange={onClose}>


The Sheet's onOpenChange prop is receiving onClose directly, but onClose expects a boolean parameter while onOpenChange provides a boolean. You should wrap it to match the expected signature:

Suggested change

<Sheet open={modalToShow === "selection"} onOpenChange={onClose}>

<Sheet open={modalToShow === "selection"} onOpenChange={(open) => onClose(!open)}>

boolean parameters have not the same semantic, but should probably be : (open) => if (!open) { onClose(false) } (does it happen that we have onOpenChange called with true) ?

onOpenChange is actually only called when closing the dialog

it would still be bad to call onClose(true) (which means close and save) when opening the modal if onOpenChange is ever called with true. so better to be clear on it

devloai · 2025-01-06T10:01:49Z

front/components/data_source/CreateConnectionConfirmationModal.tsx

-                  , including the Limited Use requirements.
-                </div>
-              </div>
+    <Sheet open={isOpen} onOpenChange={onClose}>


Similar to the previous comment, the Sheet's onOpenChange prop should be wrapped to match onClose's expected signature:

Suggested change

<Sheet open={isOpen} onOpenChange={onClose}>

<Sheet open={isOpen} onOpenChange={(open) => onClose()}>

maybe (open) => if (!open) { onClose() } ?

devloai · 2025-01-06T10:01:49Z

front/components/spaces/ConnectorDataUpdatedModal.tsx

-              <div>Data sync in progress...</div>
-            </div>
-          </div>
+    <Sheet open={isOpen}>


The Sheet component should have an onOpenChange handler to match the parent's onClose behavior:

Suggested change

<Sheet open={isOpen}>

<Sheet open={isOpen} onOpenChange={(open) => !open && onClose()}>

- Increment the version of @dust-tt/sparkle to incorporate the latest features and fixes - Ensure compatibility with the updated version of the sparkle package in the project dependencies

…irmation - Switch out the Modal component for the new Sheet component layout in CreateConnectionConfirmationModal - Adjust the components and structure inside the modal to fit the Sheet design such as adding SheetContent and SheetHeader components

…ns modal - Replace the use of Modal with Sheet for improved user experience and consistency - Introduce SheetHeader and SheetContent to structure the content within the Sheet - Ensure actions like saving and deletion are still facilitated through updated UI components within SheetContent

- Bump @dust-tt/sparkle version from 0.2.352 to 0.2.355 to include latest features and fixes - Update @dust-tt/client version from 1.0.16 to 1.0.18 to align with current dependencies and improvements

…nvocation - Simplify how ConnectorPermissionsModal is rendered by removing conditional checks - Adjust connector and dataSource to accept null values, allowing for more generic usage [front/components] - fix: add missing padding in ConnectorPermissionsModal - Add horizontal padding to the main container in ConnectorPermissionsModal for better layout consistency [front/components/spaces] - refactor: remove redundant connector check before modal - Remove unnecessary check for connector existence before rendering ConnectorPermissionsModal in SpaceResourcesList [front/lib/swr] - refactor: enhance useConnectorPermissions hook flexibility - Modify useConnectorPermissions hook to handle a null dataSource gracefully - Construct API URL conditionally based on existence of dataSource in useConnectorPermissions hook - Clean up the URL construction process for fetching data source permissions

…component - Deleted RequestOrAddDataFromDataSourceModal component and associated code due to redundancy or migration to new system.

…bust - Handle null values for connector and dataSource props to prevent crashes - Add checks for data availability before rendering components and executing save procedure - Import SheetTitle from "@dust-tt/sparkle" and use it in the SheetHeader to ensure consistency across modals

…t using SheetContainer - Wrap options and content tree in new SheetContainer component for improved structure - Simplify button and header layout within modal for enhanced visual coherence - Remove redundant div elements to streamline modal markup

- Use type-only import for NotificationType to improve code clarity and reduce potential for runtime import issues

…ogic - Removed the state and dialog for showing data connection synchronization from ConnectorPermissionsModal - Adjusted logic handling after data update to close modal without showing the data connection dialog [front/components] - style: update ContentMessage to amber variant in ConnectorDataUpdatedModal - Changed ContentMessage color from 'slate' to 'amber' to presumably enhance visibility or indication status

…omponent - Remove owner prop and Page wrapper to simplify the modal structure - Rework content layout and presentation for clarity - Adjust modal title and instructions for syncing data sources and using space access

tdraier · 2025-01-06T12:53:58Z

front/components/ConnectorPermissionsModal.tsx

-  const canUpdatePermissions = PERMISSIONS_EDITABLE_CONNECTOR_TYPES.has(
-    connector.type
-  );
+  const isDataAvailable = !!connector && !!dataSource;


( i won't do that - useMemo will be more expensive than redoing the test on every render )

tdraier · 2025-01-06T12:57:03Z

front/components/data_source/CreateConnectionConfirmationModal.tsx

-                  , including the Limited Use requirements.
-                </div>
-              </div>
+    <Sheet open={isOpen} onOpenChange={onClose}>


maybe (open) => if (!open) { onClose() } ?

tdraier · 2025-01-06T12:59:53Z

front/components/ConnectorPermissionsModal.tsx

-            )}
-          </div>
-          {OptionsComponent && plan && (
+      <Sheet open={modalToShow === "selection"} onOpenChange={onClose}>


boolean parameters have not the same semantic, but should probably be : (open) => if (!open) { onClose(false) } (does it happen that we have onOpenChange called with true) ?

tdraier · 2025-01-06T13:01:47Z

front/lib/swr/connectors.ts

-  if (includeParents) {
-    url += "&includeParents=true";
+  let url = "";
+  if (dataSource) {


Shouldn't we set disabled if dataSource is null ? what would happen by calling useSWRWithDefaults with empty url ?

Yeah you're right, I was setting the disable parameter on the function call, but let's move the logic within the hook 👍🏼

tdraier · 2025-01-06T13:04:53Z

front/components/spaces/SpaceResourcesList.tsx

-            />
-          </>
-        )}
+      <ConnectorPermissionsModal


is it really needed to render this if there's no connector/datasource ? it complexifies the code a lot and it seems ConnectorPermissionsModal can't do a lot if we don't have data to display ?

You right, let's not render it here. Good catch thx 🙏🏼

- Ensure ConnectorPermissionsModal is not disabled based on data availability, only on update permission [front/components] - refactor: conditionally render ConnectorPermissionsModal only if dataSource and connector exist - Modify SpaceResourcesList to render ConnectorPermissionsModal only with valid dataSource and connector [front/lib] - fix: correct disable condition in useConnectorPermissions hook - Address an issue in useConnectorPermissions hook by disabling it if dataSource is not present along with existing disabled conditions

…rops and logic - Remove nullability from `connector` and `dataSource` props, assuming they must always be provided - Simplify `canUpdatePermissions` logic by removing redundant data availability checks - Eliminate redundant conditional checks for rendering modals and OptionsComponent within ConnectorPermissionsModal [front/components/spaces] - fix: ensure connector exists before using it in SpaceDataSourceViewContentList - Add a safety check to confirm the presence of `connector` before attempting to use it in the SpaceDataSourceViewContentList component

- Modify `onOpenChange` prop in `Sheet` components to only call `onClose` when closing - Ensure modals 'ConnectorPermissionsModal' and 'CreateConnectionConfirmationModal' only execute onClose when the `Sheet` is being closed

- Remove conditional dataSource check as dataSource is now always provided - Streamline the url assignment process by eliminating unnecessary conditionals

JulesBelveze marked this pull request as draft January 6, 2025 09:59

devloai bot reviewed Jan 6, 2025

View reviewed changes

JulesBelveze added 8 commits January 6, 2025 11:02

[front] - refactor: update @dust-tt/sparkle to version 0.2.352

7ac7344

- Increment the version of @dust-tt/sparkle to incorporate the latest features and fixes - Ensure compatibility with the updated version of the sparkle package in the project dependencies

[front] - feature: update @dust-tt/sparkle and @dust-tt/client packages

c6a9551

- Bump @dust-tt/sparkle version from 0.2.352 to 0.2.355 to include latest features and fixes - Update @dust-tt/client version from 1.0.16 to 1.0.18 to align with current dependencies and improvements

[data_source] - refactor: remove RequestOrAddDataFromDataSourceModal …

0130a0b

…component - Deleted RequestOrAddDataFromDataSourceModal component and associated code due to redundancy or migration to new system.

JulesBelveze force-pushed the front/refacto/connectors-modal branch from 2d67c44 to 31a78f4 Compare January 6, 2025 10:03

JulesBelveze added 2 commits January 6, 2025 11:10

[front] - refactor: change import method for NotificationType

769a1e4

- Use type-only import for NotificationType to improve code clarity and reduce potential for runtime import issues

JulesBelveze requested a review from tdraier January 6, 2025 12:11

JulesBelveze marked this pull request as ready for review January 6, 2025 12:11

tdraier reviewed Jan 6, 2025

View reviewed changes

JulesBelveze requested a review from tdraier January 6, 2025 15:54

JulesBelveze added 3 commits January 6, 2025 17:03

JulesBelveze force-pushed the front/refacto/connectors-modal branch from defa747 to 2251c19 Compare January 6, 2025 16:19

[front] - refactor: simplify url construction in useConnectorPermissions

c9e40a9

- Remove conditional dataSource check as dataSource is now always provided - Streamline the url assignment process by eliminating unnecessary conditionals

tdraier approved these changes Jan 6, 2025

View reviewed changes

JulesBelveze merged commit 08d442e into main Jan 7, 2025
3 checks passed

JulesBelveze deleted the front/refacto/connectors-modal branch January 7, 2025 07:28

	const isDataAvailable = !!connector && !!dataSource;
	const isDataAvailable = useMemo(() => !!connector && !!dataSource, [connector, dataSource]);

	<Sheet open={modalToShow === "selection"} onOpenChange={onClose}>
	<Sheet open={modalToShow === "selection"} onOpenChange={(open) => onClose(!open)}>

	<Sheet open={isOpen} onOpenChange={onClose}>
	<Sheet open={isOpen} onOpenChange={(open) => onClose()}>

	<Sheet open={isOpen}>
	<Sheet open={isOpen} onOpenChange={(open) => !open && onClose()}>

[front] - refacto: connectors modals #9765

[front] - refacto: connectors modals #9765

Uh oh!

Conversation

JulesBelveze commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Risk

Deploy Plan

Uh oh!

devloai bot commented Jan 6, 2025

Uh oh!

devloai bot left a comment

Choose a reason for hiding this comment

Uh oh!

devloai bot Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devloai bot Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devloai bot Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devloai bot Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JulesBelveze commented Jan 6, 2025 •

edited

Loading