feat(tree-explorer): add ability to set preset connections in settings.json VSCODE-665 #909

gagik · 2025-01-12T22:04:19Z

Adds option to specify mdb.presetConfigurations inside settings.json (likely for a workspace) which will load a preset list of connections based on that. This setting defines preset connections. It can be used to share connection configurations in a workspace or global scope.

It is important that users do not store sensitive credentials here as it is just saved as a plaintext file and is easily accessible. The common usecase where this setting is useful is if for example some project has a local mongodb instance associated with it for tests and the repository owner wants to share different preset connection settings for when one loads this.

The setting format is an array of preset connections defined with this kind of schema:

{
 "name": {
    "type": "string",
    "description": "Name of the connection."
  },
  "connectionString": {
    "type": "string",
    "description": "Connection string. Do not store sensitive credentials here."
  }
}

Users can discover this by clicking the ... in the Tree View next to the Plus icon and selecting Edit Preset Connections... which will open the relevant file as it is detected (or create a preset connection in the workspace to modify if one doesn't exist already).

Preset connections can only be edited through the settings.json file they originate from which can also be accessed when right clicking the a preset connection and choosing "Edit Preset Connections..."

Motivation and Context

Addresses #368.

gagik · 2025-01-13T09:28:55Z

package.json

          "group": "3@2"
        },
+        {
+          "command": "mdb.openWorkspaceSettingsFile",


I think it'd be nice to include ability to quickly open workspace settings here when the item is preset (piggybacking on an existing VSCode action).

We can't easily detect where this preset comes from (i.e. it could also come from user settings) so we could also include a shortcut to open user settings, but maybe we don't want to encourage that and assume they know what they're doing by that point?

If someone that defined this in their user settings (as opposed to workspace) and clicks the Open Workspace Settings, it'd open or create a .vscode/settings.json file in the workspace, and setting things there would override user settings, which seems reasonable.

gagik · 2025-01-13T09:29:53Z

src/explorer/connectionTreeItem.ts

+      !connectionController.isConnecting();
+
+    this.contextValue = `${isConnected ? 'connected' : 'disconnected'}${
+      isMutable ? '' : 'Immutable'


naming here I'm not so sure about. Immutable could also be "Preset", I just thought of making this more generic. The idea is that it's not a connection that can be removed/edited/etc.

src/storage/connectionStorage.ts

.vscode/settings.json

alenakhineika · 2025-01-13T12:38:33Z

It looks like we use 2 different setting.json files (see the flow described below). Also, have some thoughts about UX for this feature. Let me know what you think!

I opened the MongoDB settings in vscode. This file has the ~/Library/Application Support/Code - Insiders/User/settings.json path
Clicked "Edit in settings.json" for the mdb.presetSavedConnections setting
It gave me the possibility to add a connection to the mdb.presetSavedConnections array
It would be nice to have a placeholder in case of the empty array, so people know about the expected format
Can we also refresh the sidebar on save, so new connections immediately appear there? Additionally, we could add a refresh icon to the Connections level to force a manual refresh.
I also have noticed that we can not collapse a connected connection item in the sidebar (this issue comes from the main)
Then when I click "Open Workspace settings" it actually opens the /Users/alena.khineika/www/app-template/.vscode/settings.json that doesn't have any saved connections
Should we move the "Open Workspace Settings" CTA to the "Connections" header so we can edit the current connection and also add new ones from the sidebar?
The "(From Configuration)" postfix takes a lot of space. Maybe we could chat with the design team and request some kind of special icon for such connections, or add a lock emoji, so a similar approach to save the space, but still indicate that this is a special kind of connection.

…ation suffix

gagik · 2025-01-16T09:33:40Z

src/mdbExtensionController.ts

+        void this._connectionController.loadSavedConnections();
+      }
+    });
+    this._context.subscriptions.push(subscription);


If I understand correctly, if I add this subscription to the context, it'll auto-dispose right so no further manual handling needed? @alenakhineika

This is how I understand this as well.

Now when one clicks edit preset connections, we'll automatically redirect to the correct settings.json (workspace or global).

gagik · 2025-01-16T11:33:56Z

@alenakhineika the new changes with reactivity of settings and automatically deriving where the settings.json. I also made it so one can edit connections from the header, allowing for better discoverability. I think that addresses nearly all of your comments..

…eader Running from header will automatically determine which scope to open and create a preset to edit if none are defined

…and workspace settings

nirinchev

Looks good - I mostly have style comments and a few minor suggestions.

nirinchev · 2025-01-17T16:42:06Z

.vscode/settings.json

  // Turn off tsc task auto detection since we have the necessary tasks as npm scripts
-  "typescript.tsc.autoDetect": "off"
+  "typescript.tsc.autoDetect": "off",
+  "mdb.presetSavedConnections": [


I think preset and saved are a bit redundant here. Probably best to shorten it to presetConnections.

package.json

nirinchev · 2025-01-17T16:53:19Z

src/connectionController.ts

+        // If no preset settings exist in workspace and global scope,
+        // set a default one inside the workspace and open it.
+        source = 'workspaceSettings';
+        await mdbConfiguration.update('presetSavedConnections', [


Can we extract this string literal to a constant somewhere?

I wanted to but we don't have much precedent of this the way with do with EXTENSION_COMMANDS where we also keep the mdb. suffix which is a bit awkward

I could create a follow-up PR to migrate all our settings into EXTENSION_SETTINGS constants for example. or introduce it here and move them later

I think we can introduce it here for this particular case and create a follow-up PR to move others in there.

Was meant to follow up on this but ended up losing this discussion; didn't seem to have a quick obvious location to introduce it here so will create a follow-up PR with everything instead

src/connectionController.ts

nirinchev · 2025-01-17T17:14:39Z

src/connectionController.ts

+  }
+
  async loadSavedConnections(): Promise<void> {
+    this._connections = Object.create(null);


Is this fixing a bug where deleted saved connections would show up because they're not overridden in the for loop? Couldn't spot a test that verifies the behavior before was incorrect - am I just blind or should we add one?

Yeah, this hasn't been an issue because we'd only run loadSavedConnections at startup / when this._connections would be not yet defined. Now with the reloading on save this becomes more obvious. I'll add a test.

nirinchev · 2025-01-17T17:16:53Z

src/explorer/connectionTreeItem.ts

-  connected = 'connectedConnectionTreeItem',
-}
+export type ConnectionItemContextValue = `${'disconnected' | 'connected'}${
+  | ''


[extreme nit] The leading | makes the formatting really awkard - is it even worse without it?

This gets automatically added by the linter 🤷

nirinchev · 2025-01-17T17:28:07Z

src/storage/connectionStorage.ts

+    ];
+
+    return combinedPresetSavedConnections.map((presetConnection) => ({
+      id: uuidv4(),


Why do we generate different ids here as opposed to using the name? Is it because we want to allow multiple connections with the same name as presets?

That may be a good enough reason, It is also because storage saved connections use UUIDs and we end up combining the two, so was kind of thinking of it as a stricter way to stick to a "type" where IDs are arbitrary and not tied to the connection's contents.

nirinchev · 2025-01-17T17:36:06Z

src/test/suite/storage/connectionStorage.test.ts

+          const connection = loadedConnections[i];
+          const expected = expectedConnectionValues[i];
+          expect(connection.name).equals(expected.name);
+          expect(connection.connectionOptions.connectionString).contains(


q: why are we checking for contains here rather than equals?

ah actually it was because the connectionString ends up losing the trailing /, will still adjust it to be stricter and more defined

src/explorer/connectionTreeItem.ts

…se switch

package.json

.vscode/settings.json

alenakhineika

Looks good! I created a ticket to address a remaining issue with collapsing an active connection VSCODE-671.

gagik · 2025-01-20T15:50:03Z

@alenakhineika I see the issue now, 63c719d fixes it. Going to merge and we can verify again during release.

gagik added 2 commits January 12, 2025 23:00

add preset settings

db87e39

add tests

06b1e90

gagik commented Jan 13, 2025

View reviewed changes

gagik marked this pull request as ready for review January 13, 2025 09:31

gagik requested review from alenakhineika and nirinchev January 13, 2025 09:32

feat(tree-explorer): add reactivity, sample value and remove configur…

d2d58e6

…ation suffix

gagik commented Jan 16, 2025

View reviewed changes

feat(tree-explorer): automatically derive workspace or global settings

bf9cd64

Now when one clicks edit preset connections, we'll automatically redirect to the correct settings.json (workspace or global).

gagik force-pushed the gagik/add-save-settings branch from 915a69d to 181f801 Compare January 16, 2025 11:29

feat(tree-explorer): allow running edit preset connections from the h…

0128835

…eader Running from header will automatically determine which scope to open and create a preset to edit if none are defined

gagik force-pushed the gagik/add-save-settings branch from 181f801 to 0128835 Compare January 16, 2025 11:36

fix(tree-explorer): update tests to use inspect and check for global …

0850375

…and workspace settings

gagik force-pushed the gagik/add-save-settings branch from 0850375 to ef9fa97 Compare January 17, 2025 10:14

nirinchev reviewed Jan 17, 2025

View reviewed changes

refactor: rename to presetConnections, update test strict equality, u…

bf39230

…se switch

gagik force-pushed the gagik/add-save-settings branch from ef9fa97 to bf39230 Compare January 20, 2025 10:53

test: add clear connection tests and use test strict equality

a72bad8

alenakhineika reviewed Jan 20, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

alenakhineika reviewed Jan 20, 2025

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

alenakhineika approved these changes Jan 20, 2025

View reviewed changes

gagik added 2 commits January 20, 2025 16:35

refactor: changes from review

6cf0f1c

fix(tree-explorer): fix issues with collapsing active connection

63c719d

gagik merged commit b6e36e8 into main Jan 20, 2025
6 checks passed

gagik deleted the gagik/add-save-settings branch January 20, 2025 16:09

feat(tree-explorer): add ability to set preset connections in settings.json VSCODE-665 #909

feat(tree-explorer): add ability to set preset connections in settings.json VSCODE-665 #909

Uh oh!

Conversation

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

Uh oh!

Motivation and Context

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagik commented Jan 16, 2025

Uh oh!

nirinchev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

Uh oh!

alenakhineika left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

gagik commented Jan 12, 2025 •

edited

Loading

gagik Jan 13, 2025 •

edited

Loading

alenakhineika commented Jan 13, 2025 •

edited

Loading

gagik Jan 20, 2025 •

edited

Loading

gagik Jan 20, 2025 •

edited

Loading

gagik commented Jan 20, 2025 •

edited

Loading