fix(compass-connections, sidebar): move all connection related logic and state to compass-connections COMPASS-8067 #6059

gribnoysup · 2024-07-25T13:37:33Z

The goal of this patch is to fix various inconsistencies in connection logic caused by the logic being spread across two places in the application. The issue was highlighted by the autoconnection flow just not working as expected at all, but the fix was not possible to do in a clean way without refactoring the connection store as the store shape was just not accounting for multiple connection mode at all (it's honestly mostly luck that e2e tests were still passing, mostly because even in multiple connections mode we're still testing more or less a single connection flow at a time).

The main changes to the store that this patch introduces:

Remove activeConnectionId, connectingConnectionId states: they don't make any sense in the multiple connection world
Remove state that can be derived or already provided by other hooks, like connections list or connection status / status text
Generally align the reducer following Redux style guide (which is only somewhat possible because React reducers work differently and doesn't allow to follow all the best practices): meaningful UI event based actions, no derived state in store, etc.
Remove all the "legacy" prefixed methods and logic: there is nothing in the connection flow or connection state methods that would require us to export legacy methods and not share the same ones between two modes, all the switch logic can be done in place by just checking the feature flag if needed
Move "Non genuine" modal and connection flow toasts to compass-connections: showing those is part of the connection flow logic and the UI is related to the connections

I want to note that it's not the final shape of this store, it's still not aligned with Compass architecture and using React reduxers here puts some restrictions on how separated the logic can be and the async nature of state updates spread around here.

For the reviewer: connections-store.tsx file is probably the main one to focus on, it changed a lot because very little of the old store was relevant and had to be adjusted for multiple connections to work, so it would probably be easier to just read through it as if it's a new file and not a diff.

I'll try to leave comments in places that I think are most notable, but if something doesn't make sense, please ask for clarifications!

…and state to compass-connections

addaleax · 2024-07-25T13:49:18Z

packages/compass-connections/src/stores/connections-store.tsx

+      };
+
+      if (duplicate.favorite?.name) {
+        const copyFormat = duplicate.favorite?.name.match(/(.*)\s\(([0-9])+\)/); // title (2) -> [title (2), title, 2]


Is this missing anchors? Otherwise we match something like foo (2) bar as well and lose everything after the closing parenthesis

Suggested change

const copyFormat = duplicate.favorite?.name.match(/(.*)\s$([0-9])+$/); // title (2) -> [title (2), title, 2]

const copyFormat = duplicate.favorite?.name.match(/^(.*)\s$([0-9])+$$/); // title (2) -> [title (2), title, 2]

Good catch! I'll fix this implementation and add a test (we can also probably reduce the complexity of the algorithm there by reducing to the highest number right away instead of looping over for every number 🤔)

Updated in ade3ada, also fixed handling multi digit numbers

addaleax · 2024-07-25T13:50:41Z

packages/compass-connections/src/stores/connections-store.tsx

+          error: error as Error,
+          // Autoconnect flow might fail before we can even load connection info
+          // so connectionInfo might be undefiend at this point. In
+          // single-conneciton mode this requires some special handling so that


Suggested change

// single-conneciton mode this requires some special handling so that

// single-connection mode this requires some special handling so that

gribnoysup · 2024-07-25T13:58:25Z

packages/compass-connections/src/components/connection-status-toasts.tsx

+  const isOIDC = isOIDCAuth(connectionInfo.connectionOptions.connectionString);
+  return {
+    title: `Connecting to ${connectionTitle}`,
+    description: isOIDC ? 'Go to the browser to complete authentication' : '',


This text is now applied both to multiple and single connections flow now that we're sharing more UI code between the two

gribnoysup · 2024-07-25T14:01:00Z

packages/compass-connections/src/components/legacy-connections.tsx

+// connection info object to be passed around. For that purposes this hook will
+// either return an editing connection info or will create a new one as a
+// fallback if nothing is actively being edited
+function useActiveConnectionInfo(


It might be something that we want to just make part of the connection-form logic, it's convenient to have a default there and then we don't need to manually create new connection info in two different places all the time

gribnoysup · 2024-07-25T14:01:39Z

packages/compass-connections/src/components/legacy-connections.tsx

  );

  const onConnectClick = (connectionInfo: ConnectionInfo) => {
-    void connect({ ...cloneDeep(connectionInfo) }, true).catch(() => {


Honestly not sure why this was shallow and deep cloning, if someone knows what's the reason for this, please tell

gribnoysup · 2024-07-25T14:04:15Z

packages/compass-connections/src/components/legacy-connections.tsx

-      enableDebugUseCsfleSchemaMap,
-      protectConnectionStringsForNewConnections,
-    ]
+  const activeConnectionInfo = useActiveConnectionInfo(editingConnectionInfo);


"Active" is like this concept only applicable to single connection form: what is being edited is also what you are connecting to (not sure if it's worth a comment in the code taking into account that we're going to remove it)

gribnoysup · 2024-07-25T14:05:23Z

packages/compass-connections/src/components/non-genuine-connection-modal.tsx

+export function showNonGenuineMongoDBWarningModal(
+  connectionInfo?: ConnectionInfo
+) {
+  return showConfirmation({


Button names are slightly different because of this change, but I'm not sure if it matters much tbh, I didn't try to make the names exactly the same

gribnoysup · 2024-07-25T14:08:54Z

packages/compass-connections/src/hooks/use-connection-repository.ts

+  const favoriteConnectionsRef = useRef(favoriteConnections);
+  favoriteConnectionsRef.current = favoriteConnections;
+
+  const nonFavoriteConnectionsRef = useRef(nonFavoriteConnections);
+  nonFavoriteConnectionsRef.current = nonFavoriteConnections;
+
+  const autoConnectInfoRef = useRef(autoConnectInfo);
+  autoConnectInfoRef.current = autoConnectInfo;


Drive-by: in a lot of places we need to just access the current state of those, which without changing some of the methods here required subscribing to changes to those (invalidating dependencies of a lot of other hooks) even when we don't care about it and only want to read the current value

gribnoysup · 2024-07-25T14:09:32Z

packages/compass-connections/src/stores/connections-store.tsx

-  activeConnectionId?: string;
-  activeConnectionInfo: ConnectionInfo;
-  connectingConnectionId: string | null;
-  connectingStatusText: string;
-  connectionErrorMessage: string | null;
-  oidcDeviceAuthVerificationUrl: string | null;
-  oidcDeviceAuthUserCode: string | null;


Literally none of this state makes sense in multiple connections mode 🙃

gribnoysup · 2024-07-25T14:11:44Z

packages/compass-connections/src/stores/connections-store.tsx

+          // In multiple connections mode we don't allow to start another edit
+          // when one is already in progress


I'm not sure if this was an intentional decision, before this was not possible mostly because of the bugs in the state. Currently it should be technically possible to "open" multiple times while editing if you get a toast with an error for a different connection, but maybe it's still a good idea not to allow resetting the form while one is already opened

gribnoysup · 2024-07-25T14:17:38Z

packages/compass-connections/src/stores/connections-store.tsx

-        duplicate.favorite.name = `${name} (${copyNumber})`;
+  const connectWithInflightCheck = useCallback(
+    async (connectionInfo: ConnectionInfo) => {
+      const inflightConnect = InFlightConnections.get(connectionInfo.id);


@himanshusinghs is this something that you had in mind?

yea something similar. I was probably thinking of having that in ConnectionsManager itself but having it here must have its own reasons. Sorry, I am yet to go through the PR (for my own understanding of the change).

My thinking is that this connect method is the main and only interface that other parts of UI should use to start a connection, so debounce should happen as high as possible. We are also planning to dissolve manager and repository in the connections redux store with the follow-up refactor, so this logic will end up in the store eventually anyway

gribnoysup · 2024-07-25T14:18:32Z

packages/compass-sidebar/src/components/multiple-connections/sidebar.tsx

        />
+        {editingConnectionInfo && (
+          <ConnectionFormModal


Technically could've moved the form to compass-connections too already, but all the tests are also here and I'm trying to limit this already massive PR 😅

…ot grab number from the middle of the string; reduce algo complexity by doing just one pass over array

…onnection mode after connection

gribnoysup added feature flagged PRs labeled with this label will not be included in the release notes of the next release fix labels Jul 25, 2024

fix(compass-connections, sidebar): move all connection related logic …

41e9d44

…and state to compass-connections

gribnoysup force-pushed the compass-8067-consolidate-connection-store-logic branch from 8408ac4 to 41e9d44 Compare July 25, 2024 13:44

addaleax approved these changes Jul 25, 2024

View reviewed changes

gribnoysup commented Jul 25, 2024

View reviewed changes

gribnoysup added 7 commits July 25, 2024 17:33

fix(connections): add missing dependency

0601031

chore(connections): fix active connections count

2649ce1

chore(sidebar): fix test failures

8e02c11

chore(connections): more tests

27c41a8

fix(connections): adjust regex to handle multi digit numbers and to n…

ade3ada

…ot grab number from the middle of the string; reduce algo complexity by doing just one pass over array

fix(connections): fix electron tests

9293cdb

chore(connections): add more tests

b2b4ac6

gribnoysup marked this pull request as ready for review July 29, 2024 12:03

gribnoysup added 3 commits July 29, 2024 14:07

chore(connections): different fix for a test

f013de7

chore(connections): fix typos

b04b478

fix(connections): correctly use existing connection info for single c…

f625304

…onnection mode after connection

gribnoysup merged commit d5fe7d7 into main Jul 29, 2024
34 of 35 checks passed

gribnoysup deleted the compass-8067-consolidate-connection-store-logic branch July 29, 2024 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compass-connections, sidebar): move all connection related logic and state to compass-connections COMPASS-8067 #6059

fix(compass-connections, sidebar): move all connection related logic and state to compass-connections COMPASS-8067 #6059

gribnoysup commented Jul 25, 2024 •

edited

Loading

addaleax Jul 25, 2024

gribnoysup Jul 25, 2024

gribnoysup Jul 29, 2024

addaleax Jul 25, 2024

gribnoysup Jul 25, 2024

gribnoysup Jul 25, 2024 •

edited

Loading

gribnoysup Jul 25, 2024

gribnoysup Jul 25, 2024

gribnoysup Jul 25, 2024

gribnoysup Jul 25, 2024 •

edited

Loading

gribnoysup Jul 25, 2024

gribnoysup Jul 25, 2024

gribnoysup Jul 25, 2024

himanshusinghs Jul 29, 2024

gribnoysup Jul 29, 2024

gribnoysup Jul 25, 2024

	const copyFormat = duplicate.favorite?.name.match(/(.*)\s\(([0-9])+\)/); // title (2) -> [title (2), title, 2]
	const copyFormat = duplicate.favorite?.name.match(/^(.*)\s\(([0-9])+\)$/); // title (2) -> [title (2), title, 2]

	// single-conneciton mode this requires some special handling so that
	// single-connection mode this requires some special handling so that

		// In multiple connections mode we don't allow to start another edit
		// when one is already in progress

fix(compass-connections, sidebar): move all connection related logic and state to compass-connections COMPASS-8067 #6059

fix(compass-connections, sidebar): move all connection related logic and state to compass-connections COMPASS-8067 #6059

Conversation

gribnoysup commented Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup commented Jul 25, 2024 •

edited

Loading

gribnoysup Jul 25, 2024 •

edited

Loading

gribnoysup Jul 25, 2024 •

edited

Loading