fix(connection-form): vscode support & cleanup COMPASS-8098 #6225

paula-stacho · 2024-09-11T11:56:59Z

Description

Preparing the new connection form for VSCode.
New preferences to be set in VSCode:

saveAndConnectLabel: 'Save & Connect', 
showHelpCardsInForm: false,
showPersonalisationForm: false,

I also used this opportunity to do a bit of cleanup of the pre-MC code (tbh tiptoing around the legacy parts of connection form has been something that bugged me for a while).

Left out (changes that will be visible in VSCode - let me know if you think these should also be customizable so that we avoid them in VSCode):

the new subtitle 'Manage your connection settings'
horizontal line above the buttons

Checklist

New tests and/or benchmarks are included
Documentation is changed or added
I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

Dependents

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

gribnoysup · 2024-09-13T14:13:54Z

packages/compass/src/app/components/home.tsx

@@ -125,8 +83,7 @@ function Home({
  hideCollectionSubMenu,
  showSettings,
 }: Omit<HomeProps, 'connectionStorage'>): React.ReactElement | null {
-  const appRegistry = useLocalAppRegistry();
-  const { connectionInfo, isConnected, disconnect } =
+  const { isConnected, disconnect } =


If we're making it impossible to use single connection mode by removing the form, why are we keeping this code here?

I have reverted this bit and will be only removing the tests here. as much as I'd love to follow this rabbit hole, I don't have time to do that, so I'll leave it for the scheduled MC cleanup. I hope the partial cleanup I've done in the connection-form is okay. This is why I was asking in the channel if it's safe to say we won't be reverting the MC flag.

gribnoysup · 2024-09-13T14:15:11Z

packages/connection-form/src/hooks/use-connect-form-preferences.tsx

@@ -29,6 +34,7 @@ const defaultPreferences = {
  showKerberosAuth: true,
  showCSFLE: true,
  showProxySettings: true,
+  saveAndConnectLabel: 'Connect',


I think the intention for these preferences was only to map compass preferences to connection form without the need to have a dependency on the preferences model (although @mcasimir might correct me on that), this is just something that you'd usually pass as a component property, doesn't feel like it belongs here

Hmm.. I put it there because 'showFavoriteActions' was already there. There is a note above that says '// Not all of these preference map to Compass preferences.', which suggests that what you say used to be true, but at this point maybe we can keep the form preferences together?

If we're already diverging from React patterns in an unconventional ways here and don't want to touch this particular part, I guess it's okay to keep it for now like that for consistency, but just want to make sure we all understand that we are leaving more clean up to do after a "clean up" PR here.

My main concern here is that we are copying patterns in compass codebase often from one place to another (which is totally normal), so leaving code like that is always a risk that antipatterns will start spreading through other parts of the codebase

@gribnoysup as discussed, changed the provider into ConnectFormSettingsProvider, which combines compass preferences and other settings (such as the VSCode customizations). externally, these are top level properties

packages/connection-form/src/components/connection-form.tsx

gribnoysup

A few small comments, but otherwise looks good

gribnoysup · 2024-09-26T14:24:01Z

...ion-form/src/components/advanced-options-tabs/authentication-tab/authentication-tab.spec.tsx

    enableOidc: true,
  },
  updateConnectionFormField,
 }: {
  connectionStringUrl?: ConnectionStringUrl;
-  connectionFormPreferences?: Partial<ConnectionFormPreferences>;
+  ConnectionFormSettings?: Partial<ConnectionFormSettings>;


Super small nit: seems like the fist capitalization here is not expected

gribnoysup · 2024-09-26T14:24:35Z

packages/connection-form/src/components/connection-form-actions.spec.tsx

-        })
-      );
+      const cancelButton = screen.getByRole('button', { name: 'Cancel' });
+      userEvent.click(cancelButton);


Nice cleanup, thanks!

gribnoysup · 2024-09-26T14:28:10Z

packages/connection-form/src/hooks/use-connect-form-preferences.tsx

-export const ConnectionFormPreferencesContext = createContext<
-  Partial<ConnectionFormPreferences>
+export const ConnectionFormSettingsContext = createContext<
+  Partial<ConnectionFormSettings>
 >({});

 export const useConnectionFormPreference = <


Should we rename the use hook to settings instead of preferences too?

gribnoysup · 2024-09-26T14:35:36Z

packages/connection-form/src/components/connection-form.tsx

+      value={{
+        showFavoriteActions,
+        showHelpCardsInForm,
+        showPersonalisationForm,
+        protectConnectionStrings,
+        forceConnectionOptions,
+        showKerberosPasswordField,
+        showOIDCDeviceAuthFlow,
+        enableOidc,
+        enableDebugUseCsfleSchemaMap,
+        protectConnectionStringsForNewConnections,
+        showOIDCAuth,
+        showKerberosAuth,
+        showCSFLE,
+        showProxySettings,
+        saveAndConnectLabel,
+      }}


Before we could've passed this value straight to provider without any extra code because the outer code made sure that the object is stable and is not re-created every render, now that we're destructuring here, it would be better to make sure that the value is stable (even if it makes the code a bit weird), so let's wrap the value in useMemo before passing to provider, kinda like this:

const value = useMemo(() => { return { ... properties } }, [ ... dependencies]) return <Provider value={value}> ... </Provider>

Otherwise anything using the context will be re-rendering erroneously

github-actions bot added the fix label Sep 11, 2024

paula-stacho changed the title ~~fix(connection-form): cleanup & vscode support~~ fix(connection-form): vscode support & cleanup Sep 12, 2024

paula-stacho changed the title ~~fix(connection-form): vscode support & cleanup~~ fix(connection-form): vscode support & cleanup COMPASS-8098 Sep 12, 2024

paula-stacho added the no release notes Fix or feature not for release notes label Sep 12, 2024

paula-stacho force-pushed the COMPASS-8098 branch 2 times, most recently from 731f6b1 to fa57be6 Compare September 13, 2024 08:39

paula-stacho marked this pull request as ready for review September 13, 2024 09:16

paula-stacho force-pushed the COMPASS-8098 branch from 2e1291f to dc18eec Compare September 13, 2024 10:35

gribnoysup reviewed Sep 13, 2024

View reviewed changes

paula-stacho force-pushed the COMPASS-8098 branch from 9e0a9a7 to 3fa4f60 Compare September 24, 2024 08:10

paula-stacho added 8 commits September 25, 2024 14:22

fix(connection-form): cleanup & vscode support

1aa4bc4

more cleanup

5100e95

..

01a96bd

update form action tests

7240153

more cleanup

bc69345

..

48a2e8f

revert part

433df1b

preferences update

e477705

paula-stacho force-pushed the COMPASS-8098 branch from 3fa4f60 to e477705 Compare September 25, 2024 12:22

paula-stacho added 2 commits September 25, 2024 14:34

.

74f8632

update tests

63a46ac

Anemy reviewed Sep 25, 2024

View reviewed changes

packages/connection-form/src/components/connection-form.tsx Outdated Show resolved Hide resolved

more cleanup

ea389dc

gribnoysup approved these changes Sep 26, 2024

View reviewed changes

paula-stacho added 2 commits September 26, 2024 17:19

renames

af7e1f3

useMemo

4f56e3b

Anemy approved these changes Sep 26, 2024

View reviewed changes

paula-stacho merged commit dc13694 into main Sep 27, 2024
27 checks passed

paula-stacho deleted the COMPASS-8098 branch September 27, 2024 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(connection-form): vscode support & cleanup COMPASS-8098 #6225

fix(connection-form): vscode support & cleanup COMPASS-8098 #6225

paula-stacho commented Sep 11, 2024 •

edited

Loading

gribnoysup Sep 13, 2024

paula-stacho Sep 13, 2024

gribnoysup Sep 13, 2024

paula-stacho Sep 13, 2024

gribnoysup Sep 16, 2024 •

edited

Loading

paula-stacho Sep 25, 2024

gribnoysup left a comment

gribnoysup Sep 26, 2024

gribnoysup Sep 26, 2024

gribnoysup Sep 26, 2024

gribnoysup Sep 26, 2024

fix(connection-form): vscode support & cleanup COMPASS-8098 #6225

fix(connection-form): vscode support & cleanup COMPASS-8098 #6225

Conversation

paula-stacho commented Sep 11, 2024 • edited Loading

Description

Left out (changes that will be visible in VSCode - let me know if you think these should also be customizable so that we avoid them in VSCode):

Checklist

Motivation and Context

Open Questions

Dependents

Types of changes

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 Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup left a 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

paula-stacho commented Sep 11, 2024 •

edited

Loading

gribnoysup Sep 16, 2024 •

edited

Loading