feat(connections-navigation): connect in new window via connect split-button COMPASS-8473 #6577

kraenhansen · 2024-12-19T22:27:52Z

Description

Merging this PR will:

Adjust the "connect button" in the sidebar to display a SplitButton with a dropdown menu, containing options to "Connect Here" and "Connect in a New Window".
Adding the concept of the ItemActionControls being "hideable" (through the setHideable callback) allowing children (the ConnectButton in particular) to prevent the controls from being hidden once the cursor leaves the connection row. I use this to prevent controls (hence SplitButton and it's Menu) to be removed from the tree as the cursor is moved out of the ConnectButton, across and into the menu: If the menu is opened, the controls stay visible.
Propagate events through "connection actions" → "connections Redux store" → "globalAppRegistry" → Main IPC → Window Manager to finally create a new window and ultimately storing a "connection id" in main-process state associated with this window.
Auto-connect the newly created window from the associated state.
For consistency, I added auto-connect when invoked as a CLI and a connection ID is passed.

compass-connect-in-new-window.mov

TODOs

Rebase once chore(compass-components): refactor ItemActionControls #2 #6560 merge.
Ensure the SplitButton is only shown on Compass Desktop (and add a test for this)
Focus / activate the connection when auto-connected
Update the LeafyGreen SplitButton component once we're able to pass renderDarkMenu LG-4721: Fix SplitButton's menu is rendered in dark-mode regardless of value passed through renderDarkMenu mongodb/leafygreen-ui#2601 and to get LG-4714: Fix using SplitButton as a managed component mongodb/leafygreen-ui#2596. I'm currently using a local install of the @leafygreen-ui/split-button.

Checklist

New tests and/or benchmarks are included
Documentation is changed or added

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Open Questions

I did add support for passing a connection id as positional argument when invoked via the CLI, but is this desirable?

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)

packages/compass-components/package.json

lerouxb · 2024-12-20T12:10:20Z

packages/compass-components/src/components/actions/item-action-controls.tsx

@@ -39,7 +33,7 @@ export type ItemActionControlsProps<Action extends string> = {
  'data-testid'?: string;
 };

-export function ItemActionControls<Action extends string>({
+export function ItemActionControls<Action extends string = string>({


TIL that you can have a default

Yeah – it usually just reduces type safety, though, because then consumers are likely to omit the type (and the default is typically the most generic type available).

This also seems to be one of those cases where type safety goes away – I don't seen an obvious reason for that in the rest of the code, though, so maybe in this particular case we could reconsider?

🙂 TBH, it's likely not strictly needed here, since we'll always be using this in a way that can infer the type of this type parameter from the props.

That being said - those changes are actually a part of #6560 (which needs to be reviewed and merged first).

gribnoysup

Overall looks good, would be nice to try to run this locally after leafygreen patch is released

gribnoysup · 2025-01-06T15:08:25Z

packages/compass-e2e-tests/helpers/selectors.ts

@@ -282,6 +282,8 @@ export const Single = {
 export const Multiple = {
  ConnectionsTitle: '[data-testid="connections-header"]',
  ConnectButton: '[data-action="connection-connect"]',
+  ConnectInNewWindowButton: '[data-action="connection-connect-in-new-window"]',
+  ConnectDropdownButton: '[aria-label="see more connection options"]',


Doesn't look like a great selector, anything better we can target here?

What in particular don't you like about this? That it will break if the button label gets updated?

This is targeting a "trigger" button within the SplitButton, but most props are forwarded to the "left most" button: https://github.com/mongodb/leafygreen-ui/blob/main/packages/split-button/src/SplitButton/SplitButton.tsx#L88

This is the trigger I'm targeting 🤔

<button data-lgid="lg-button" type="button" class="lg-ui-button-0000 leafygreen-ui-lt73jo" aria-disabled="false" aria-label="see more connection options" aria-controls="lg-split-button-menu-56" aria-expanded="true" aria-haspopup="true"> ... </button>

Alternatively I can select this relative to its sibling:

[data-testid="sidebar-navigation-item-actions-connection-connect-action"] + button

I don't like a brittle selector that targets specifically an aria-label attribute, if leafygreen team changes how they apply a label (for example switching to visually hidden element or using labelledby instead of label directly) this will break. If you want to select by inferred button label, you should use selectors designed for that, either a button=<text> or aria/<text> selectors. You can learn more about wdio selectors in the documentation

We also usually use testid selectors in e2e tests, which are also mentioned as recommented by the documentation, so if we can pass a testid here, that's also an option and consistent with most of the other selectors. If we can't, it might be worth asking leafygreen team to add the option to pass it

I've made a PR for LG adding a constant data-testid attribute to the trigger element: mongodb/leafygreen-ui#2621

Related, I noticed that LG was also selecting the trigger in their test, relative to the other button: https://github.com/mongodb/leafygreen-ui/blob/f5d842764faf5b498f799d1710288ca964a93141/packages/split-button/src/SplitButton/SplitButton.spec.tsx#L52

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

packages/compass/src/main/auto-connect.ts

betsybutton · 2025-02-12T23:31:39Z

packages/compass-connections-navigation/src/connect-button-with-menu.tsx

+          glyph="OpenNewTab"
+          onClick={onClick}
+        >
+          Connect in a New Window


Can we make this simply "In New Window"

betsybutton · 2025-02-12T23:31:57Z

packages/compass-connections-navigation/src/connect-button-with-menu.tsx

+          glyph="Connect"
+          onClick={onClick}
+        >
+          Connect Here


Can we simplify this to just "Here"

gribnoysup · 2025-02-13T08:50:10Z

packages/compass-components/package.json

@@ -46,7 +46,7 @@
    "@leafygreen-ui/guide-cue": "^7.0.2",
    "@leafygreen-ui/hooks": "^8.3.4",
    "@leafygreen-ui/icon": "^13.1.2",
-    "@leafygreen-ui/icon-button": "^16.0.2",
+    "@leafygreen-ui/icon-button": "16.0.2",


Any reason this is pinned now?

Yep. I meant to capture an issue on upgrading this as well, the v16.0.3 release is updating the types of the component, heres a comment on the original LG PR introducing the breaking change: mongodb/leafygreen-ui#2703 (comment)

Do we have a ticket to follow up with a fix for that?

Do we have a ticket to follow up with a fix for that?

Not yet. I'll create that as part of merging this PR - this is what I meant by:

I meant to capture an issue on upgrading this as well

I've created COMPASS-8973 now.

gribnoysup · 2025-02-13T08:50:13Z

packages/compass/package.json

@@ -180,6 +180,7 @@
  },
  "dependencies": {
    "@mongosh/node-runtime-worker-thread": "^3.0.2",
+    "bson": "^6.10.1",


This is a dev dependency

How is that? It's being used at runtime here:

compass/packages/compass/src/main/auto-connect.ts

Line 8 in 9959e8d

import { UUID } from 'bson';

Is this because you expect this to be included in the Webpack bundle?

This code is bundled with webpack before being packaged, bson ends up bundled within the distribution, this dependency that will be installed with node_modules in "production" will never be required, you're basically just adding some dead code to the packaged application. Only dependencies that are externalized out of the bundle are part of the dependencies list here. This is why most of the dependencies are dev here

Makes sense. I've moved it to devDependencies

gribnoysup · 2025-02-13T09:15:16Z

packages/compass/src/app/index.tsx

+    // Propagate events from global app registry to the main process
+    globalAppRegistry.on('connect-in-new-window', (connectionId: string) => {
+      void ipcRenderer?.call('app:connect-in-new-window', connectionId);
+    });


FWIW adding support for web is literally just adding same three lines that call window.open instead of ipcRenderer in compass-web entrypoint and if we want to be extra minimal in the amount of required changes it's just propagating the callback as an interface on compass-web component and dealing with it on mms side

three lines that call window.open

Ideally, I don't want the compass codebase to make assumptions on what URL it is being rendered at, which is why I wanted to delegate the opening of a separate browser tab to MMS.

Awesome, as I said, even simpler, you can expose the callback as part of the CompassWeb interface and let mms deal with that, we already do this with onActiveWorkspaceTabChange

Makes sense. I've linked to this thread from the COMPASS-8970 ticket.

For next time, please let's discuss excluding features like that before jumping on implementation, not only this is technically and product-wise against the goals of compass-web, we also just added more extra work than the actual implementation would require to ourselves

let's discuss excluding features like that before jumping on implementation

I definitely don't share your sentiment that I've been "jumping on implementation". I have had extensive discussions with @bsradcliffe in Figma on the design and I've implemented this against the design detailed in COMPASS-8381, which has clearly stated the dropdown on "compass-web" as a non-goal:

A few clarifications on the design, since it doesn't have all the details flushed out:

Clicking the primary trigger surface connects you immediately in the same window.

Button appears on hover of a connection node.

Definitely disable dropdown for Compass Web.

this is technically and product-wise against the goals of compass-web

I suppose I've been trusting the goals of our product was incorporated into the design,
but I'll make sure to keep that in mind going forward 👍

we also just added more extra work than the actual implementation would require to ourselves

Please clarify that statement. What extra work? To whom? And compared to what?

We had a zoom call to talk through this ☝️

kraenhansen · 2025-02-13T13:25:15Z

@betsybutton this is what it looks like now 👍

Co-authored-by: Sergey Petushkov <[email protected]>

gribnoysup

Hmmm, when running this locally, I'm getting an error in the main console every time I open a connection in the new window:

Error sending from webFrameMain:  Error: Failed to serialize arguments
    at s.send (node:electron/js2c/browser_init:2:91812)
    at _.send (node:electron/js2c/browser_init:2:75243)
    at resolve (webpack://mongodb-compass/../hadron-ipc/src/main.ts?:95:20)
    at IpcMainImpl.eval (webpack://mongodb-compass/../hadron-ipc/src/main.ts?:111:7)
    at IpcMainImpl.emit (node:events:518:28)
    at WebContents.<anonymous> (node:electron/js2c/browser_init:2:85784)
    at WebContents.emit (node:events:518:28)
Error sending from webFrameMain:  Error: Failed to serialize arguments
    at s.send (node:electron/js2c/browser_init:2:91812)
    at _.send (node:electron/js2c/browser_init:2:75243)
    at resolve (webpack://mongodb-compass/../hadron-ipc/src/main.ts?:95:20)
    at IpcMainImpl.eval (webpack://mongodb-compass/../hadron-ipc/src/main.ts?:111:7)
    at IpcMainImpl.emit (node:events:518:28)
    at WebContents.<anonymous> (node:electron/js2c/browser_init:2:85784)
    at WebContents.emit (node:events:518:28)

Have you seen that?

gribnoysup · 2025-02-14T08:31:57Z

packages/compass-e2e-tests/tests/connection.test.ts

+  });
+
+  it('can connect in new window', async function (this) {
+    skipForWeb(this, 'connecting in new window is not supported on web');


Leave a TODO here with a ticket numer so that it's easier to find?

Added a todo 👍

gribnoysup · 2025-02-14T08:32:10Z

packages/compass-preferences-model/src/preferences-schema.ts

+  /**
+   * Enables a dropdown in the connections sidebar to connect in a new window.
+   */
+  enableConnectInNewWindow: {


Ditto about TODO

Added a todo here as well 👍

gribnoysup · 2025-02-14T08:44:07Z

Added some logging, a whole browser window is getting passed to the resolve there at some point:

BrowserWindow {
  setBounds: [Function (anonymous)],
  _events: [Object: null prototype] {
    blur: [Function (anonymous)],
    focus: [ [Function (anonymous)], [Function: onFocus] ],
    show: [Function: visibilityChanged],
    hide: [Function: visibilityChanged],
    minimize: [Function: visibilityChanged],
    maximize: [Function: visibilityChanged],
    restore: [Function: visibilityChanged],
    closed: [ [Function (anonymous)], [Function], [Function] ],
    'ready-to-show': [Function: bound onceWrapper] { listener: [Function: resolver] },
    error: [Function: bound onceWrapper] {
      listener: [Function: errorListener]
    }
  },
  _eventsCount: 10,
  _browserViews: [],
  devToolsWebContents: [Getter]
}
Error sending from webFrameMain:  Error: Failed to serialize arguments

packages/compass/src/main/window-manager.ts

Co-authored-by: Sergey Petushkov <[email protected]>

gribnoysup

Awesome work! Happy to see this landing in main!

kraenhansen self-assigned this Dec 19, 2024

github-actions bot added the feat label Dec 19, 2024

kraenhansen commented Dec 19, 2024

View reviewed changes

packages/compass-components/package.json Outdated Show resolved Hide resolved

kraenhansen force-pushed the kh/connect-in-new-window branch from 9bbdf1c to 1dcbbfb Compare December 19, 2024 23:04

lerouxb reviewed Dec 20, 2024

View reviewed changes

kraenhansen force-pushed the kh/connect-in-new-window branch 3 times, most recently from cffbfbb to ddc2f45 Compare December 20, 2024 20:26

gribnoysup reviewed Jan 6, 2025

View reviewed changes

kraenhansen force-pushed the kh/connect-in-new-window branch from ce961a6 to 77c4fa7 Compare February 11, 2025 22:19

Add dependency on split-button

df3c848

kraenhansen force-pushed the kh/connect-in-new-window branch 3 times, most recently from 35118a6 to 9959e8d Compare February 12, 2025 23:18

betsybutton reviewed Feb 12, 2025

View reviewed changes

gribnoysup reviewed Feb 13, 2025

View reviewed changes

kraenhansen added 4 commits February 13, 2025 13:42

Make ItemActionControls hidable and use a SplitButton

a78cc06

Fix menu item width

81364a7

Fix event emitter types on CompassApplication

dcffcef

Propagate from connections-navigation action through main process IPC

070641f

kraenhansen force-pushed the kh/connect-in-new-window branch from 9959e8d to 806e5e6 Compare February 13, 2025 12:42

kraenhansen added the release notes label Feb 13, 2025

kraenhansen and others added 6 commits February 13, 2025 15:12

Add connection-id support to the auto-connect feature

faa13e5

Wire up the ConnectButton

48879d8

Propagate from main IPC into window manager

70b558b

Add a test

2edd457

Remove an outdated comment

c77f2a8

Apply suggestions from code review

0a8e142

Co-authored-by: Sergey Petushkov <[email protected]>

kraenhansen added 6 commits February 13, 2025 15:12

Add enableConnectInNewWindow preference controlling connect button

3804739

Add test ensuring the right connect button is displayed

0c638cb

Pin [email protected]

dce2e37

Revert use of PolymorphicProps

4be7eb5

Fix ConnectMenuItem

a2744e4

Simplify text in the connect menu

37815b1

kraenhansen force-pushed the kh/connect-in-new-window branch from 806e5e6 to 37815b1 Compare February 13, 2025 14:12

Reset windows after each test

d63ab8f

kraenhansen marked this pull request as ready for review February 14, 2025 05:09

kraenhansen requested a review from gribnoysup February 14, 2025 05:09

gribnoysup reviewed Feb 14, 2025

View reviewed changes

packages/compass/src/main/window-manager.ts Outdated Show resolved Hide resolved

kraenhansen and others added 2 commits February 14, 2025 10:31

Update packages/compass/src/main/window-manager.ts

b9d0dc1

Co-authored-by: Sergey Petushkov <[email protected]>

Add comments mentioning COMPASS-8970

a4f53be

kraenhansen requested a review from gribnoysup February 14, 2025 11:43

gribnoysup approved these changes Feb 14, 2025

View reviewed changes

kraenhansen merged commit a203c74 into main Feb 14, 2025
35 checks passed

kraenhansen deleted the kh/connect-in-new-window branch February 14, 2025 14:03

feat(connections-navigation): connect in new window via connect split-button COMPASS-8473 #6577

feat(connections-navigation): connect in new window via connect split-button COMPASS-8473 #6577

Conversation

kraenhansen commented Dec 19, 2024 • edited Loading

Description

TODOs

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Jan 7, 2025 • 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

kraenhansen Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribnoysup Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraenhansen Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

kraenhansen Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

kraenhansen commented Feb 13, 2025 • edited Loading

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

gribnoysup commented Feb 14, 2025

gribnoysup left a comment

Choose a reason for hiding this comment

kraenhansen commented Dec 19, 2024 •

edited

Loading

kraenhansen Jan 6, 2025 •

edited

Loading

kraenhansen Jan 7, 2025 •

edited

Loading

kraenhansen Feb 14, 2025 •

edited

Loading

kraenhansen Feb 13, 2025 •

edited

Loading

gribnoysup Feb 13, 2025 •

edited

Loading

kraenhansen Feb 13, 2025 •

edited

Loading

kraenhansen Feb 14, 2025 •

edited

Loading

kraenhansen Feb 14, 2025 •

edited

Loading

kraenhansen commented Feb 13, 2025 •

edited

Loading