Skip to content

fix(compass-web): Rename connections list to clusters list in compass-web COMPASS-8698 #6611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 15, 2025
Merged

fix(compass-web): Rename connections list to clusters list in compass-web COMPASS-8698 #6611

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3bdc7eb
rename connections to context using a provider for is atlas connections
djechlin-mongodb Jan 14, 2025
a5d5cce
move the context to sidebar
djechlin-mongodb Jan 15, 2025
13d5e78
add unit test
djechlin-mongodb Jan 15, 2025
a5d489f
only export the provider not the whole context
djechlin-mongodb Jan 15, 2025
6f163dc
remove import
djechlin-mongodb Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions packages/compass-sidebar/src/components/multiple-connections/connections-navigation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import toNS from 'mongodb-ns';
import React, { useCallback, useEffect, useMemo } from 'react';
import React, { useCallback, useContext, useEffect, useMemo } from 'react';
import { connect } from 'react-redux';
import {
connectionStorageLocator,
IsAtlasConnectionStorageContext,
} from '@mongodb-js/connection-storage/provider';
import {
ChevronCollapse,
type ItemAction,
Expand Down Expand Up @@ -477,14 +481,16 @@ const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({
}
}, [activeWorkspace, onDatabaseToggle, onConnectionToggle]);

const isAtlasConnectionStorage = useContext(IsAtlasConnectionStorageContext);

return (
<div className={connectionsContainerStyles}>
<div
className={connectionListHeaderStyles}
data-testid="connections-header"
>
<Subtitle className={connectionListHeaderTitleStyles}>
Connections
{isAtlasConnectionStorage ? 'Clusters' : 'Connections'}
{connections.length !== 0 && (
<span className={connectionCountStyles}>
({connections.length})
Expand All @@ -503,7 +509,11 @@ const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({
{connections.length > 0 && (
<>
<NavigationItemsFilter
placeholder="Search connections"
placeholder={
isAtlasConnectionStorage
? 'Search clusters'
: 'Search connections'
}
filter={filter}
onFilterChange={onFilterChange}
/>
Expand Down
2 changes: 1 addition & 1 deletion packages/compass-sidebar/src/components/multiple-connections/sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useCallback, useState } from 'react';
import React, { useCallback, useContext, useState } from 'react';
import { type MapStateToProps, connect } from 'react-redux';
import {
ConnectionStatus,
Expand Down
5 changes: 4 additions & 1 deletion packages/compass-web/src/connection-storage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, { useContext, useRef } from 'react';
import { IsAtlasConnectionStorageContext } from '@mongodb-js/connection-storage/provider';
import type {
ConnectionStorage,
ConnectionInfo,
Expand Down Expand Up @@ -374,7 +375,9 @@ export const AtlasCloudConnectionStorageProvider = createServiceProvider(
<ConnectionStorageProvider
value={sandboxConnectionStorage ?? storage.current}
>
{children}
<IsAtlasConnectionStorageContext.Provider value={true}>
{children}
</IsAtlasConnectionStorageContext.Provider>
</ConnectionStorageProvider>
);
}
Expand Down
2 changes: 2 additions & 0 deletions packages/connection-storage/src/provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ export const ConnectionStorageContext = createContext<ConnectionStorage | null>(
null
);

export const IsAtlasConnectionStorageContext = createContext<boolean>(false);
Copy link
Collaborator

@gribnoysup gribnoysup Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're lacking a bit of documentation around this, I'm sorry for that, but so far we were going a bit back and forth on this and decided not to introduce catch-all contexts that would allow to identify that you're in compass-web like that: it makes it too easy to introduce behavior and UI that's different per runtime and this goes against the goal of unification, so we usually either have very focused preferences to toggle some feature or provide interfaces for something like this without tying them to the runtime identification specifically, this forces us to develop this without necessarily tying the behavior to the runtime unless absolutely necessary.

In this case I would suggest to go with the latter option and make these labels configurable with a property on the sidebar component:

type SidebarPluginProps = {
  // ... existing props
  navigationItemsTitle?: string;
  navigationItemsFilterLabel?: string;
}

Then in compass-web you can pass those directly to the place where we render the sidebar:

renderSidebar={() => {
  return (
    <CompassSidebarPlugin
        navigationItemsTitle="Clusters"
        showSidebarHeader={false}
        
// ... rest of the code

This requires a bit more wiring, but is in line with the existing showSidebarHeader interface, in line with just normal React component composition patterns, and allows us to avoid introducing this catch-all "is mms" flag that we're trying to avoid.

What do you think, does this makes sense? Happy to discuss more!

Copy link
Contributor Author

@djechlin-mongodb djechlin-mongodb Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd push back here. A general unification goal is minimize web/desktop diffs. One necessary diff is loading atlas clusters v connections. This JIRA bug is not asking for a new diff, it is a bug following from how we implemented the atlas v connections diff. So implementation should not introduce any new "are we in compass" logic, but make the text decision using whatever component decided to load clusters instead of connections.

Context v prop drilling is mostly just a code organization choice, with context being more decoupled/less boilerplate for where the context/property gets set. The important thing is the context/property gets set by an object with direct knowledge of what it should be. I decided AtlasCloudConnectionsProvider is a sensible object to own that knowledge. (The store classes are too; they just got messy and don't really have fixed methods like that.) I would try not to use compass-web, as compass-web doesn't have direct knowledge that we are fetching atlas cloud connections, it actually first delegates that decision to AtlasCloudConnectionsProvider. Prop-drilling from compass-web would be less preferred because it's introducing a second, incorrectly decoupled way of saying if we're in atlas or not. It would be like if compass web said something like

<Atlas>welcome to atlas!</Atlas><Desktop>Here's your local connections!</Desktop> -- you'd rather write compass-web in a way that it's impossible to have code like that.

Also note this complexity is irreducible. Sidebar is now a component flexible enough to load Connections or Clusters. It will therefore need context/prop/some other way to tell and display either Connections or Clusters. (not that you're suggesting it) but I've avoid making it super duper general like taking in 'Connections' or 'Clusters' text since it sees to dilute its responsibility as a few to display a bunch of ConnectionInfo[] (that may be clusters) sensibly; instead it starts delegating how to display them to a bunch of other components, instead of asking which to display and doing that.

LMK what you think or discuss. I'm not committed to one of a few different implementation choices, but I do think I minimized maintenance cost by not introducing any "are we in MMS" decision points, provided we are all aligned on the initial bug.

Copy link
Collaborator

@gribnoysup gribnoysup Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One necessary diff is loading atlas clusters v connections.

That's only temporarily true while Atlas login is not fully available in Compass, so I'd avoid making design decisions assuming this. This is specifically the reason I'm hesitant to tie this check to anything related to the storage like you're doing in the current implementation.

Context v prop drilling is mostly just a code organization choice

That's totally fair, I don't have a strong preference here whether to use one or another. I do have a strong preference for this context to be more tied to the sidebar code though and not explicitly connect a label override to the environment code is running in / specific store implementation being used.

Also note this complexity is irreducible. Sidebar is now a component flexible enough to load Connections or Clusters. It will therefore need context/prop/some other way to tell and display either Connections or Clusters.

Sidebar is not responsible for loading connections, and most importantly all connections are abstracted into a single ConnectionInfo type by design, you shouldn't be able to do this sort of catch all distinction for all connections sidebar is rendering based on the service used as we are planning for eventually being able to render a mix of those.

If you want to delegate the job of detecting what label to render to the sidebar component, then the most correct way right now would be to check that every ConnectionInfo item that is available has the atlasMetadata property available, but I'm pretty sure we'll have to change this UI at least once more when adding support for Atlas connections in Compass desktop, so I'd really consider opting for something that is just specifically made to allow you to override the labels in the sidebar.

Copy link
Contributor Author

@djechlin-mongodb djechlin-mongodb Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the situation is

  • compass-web knows it should say Clusters
  • you can't derive that it should say Clusters from connection loading strategy because Compass will at some point be able to load atlas clusters by this mechanic
  • you could drive it from if all(connectioninfos have atlas metadata) say clusters, I actually thought of that but didn't like the idea that a mix of atlas/nonatlas in compass, as when a nonatlas connection appears/disappears for whatever reason, will wind up toggling the "Clusters" heading in Compass desktop. And you're saying it's not that future-proofed anyway.

I think what I'll do is rename the Context object, and move it outside the "AtlasCloudConnectionsProvider", but have it so that compass-web picks the connection load strategy and the header name next to each other, so the reader hopefully understands that you need to pick a header name that makes sense for your connection load strategy and maintains them together.

Commenting now... code forthcoming.

Copy link
Collaborator

@gribnoysup gribnoysup Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan 👍 I mentioned this already, I do think that sidebar package itself is probably a good place for this contex


export const ConnectionStorageProvider = ConnectionStorageContext.Provider;

// TODO(COMPASS-7397): storage context should not be leaking out of the service
Expand Down
Loading