Skip to content

Conversation

@nbbeeken
Copy link
Collaborator

Description

The welcome page now responds to a connect click and shows the connect progress to each cluster.

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added
  • If this change updates the UI, screenshots/videos are added and a design review is requested
  • 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)

Comment on lines 1783 to 1792
dispatch(
updateConnectionStep(connectionInfo.id, 'topology', 'completed')
);
dispatch(
updateConnectionStep(
connectionInfo.id,
'authentication',
'in-progress'
)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dispatching actions (especially setter actions) in bulk is an antipattern that usually points to a wrongly selected abstraction for the action: actions should roughly map to actual events happening in the app, and then it's reducer job to define what exactly that means. As you are actually dealing with already existing events here, I think the best thing you can do is to actually model your actions around those and make reducer decide how to modify state based on that

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this a web only feature? It doesn't rely on anything web specific

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mainly just layout, it is totally doable on desktop but I wasn't sure what to do with the "start a free cluster" and "add a connection" cards that are also on this page, I guess they can just be wholesale replaced. My hesitation comes from how the plug image replacing the cloud image is a bit of a jolt but it works because they're in the same position, when I was running this on desktop, the cloud image is to one side, and the jump to center for the plug seems like buggy almost? I may be overly sensitive and we should just keep it simple and make both work.

const connectionName = connectionInfo.title;

// Create sub-items for connecting steps
const renderConnectingSteps = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

A cleaner approach would be to define either this or even a single step outside of the render as a separate component ("Thinking in React" is an easy read to get a feeling for how to break down code like that)


// Create a proper 'connected' state, which excludes connectingSteps
const newConnectionState: ConnectionState = {
info: existingConnection?.info || ({} as ConnectionInfo),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This action should not proceed if you don't have a corresponding connection in the state, assigning an empty info is a dangerous assumption that also doesn't really make sense in the context of the whole app

Comment on lines 145 to 151
updateConnectionStep: (
connectionId: ConnectionId,
step: 'topology' | 'authentication' | 'listingDatabases',
status: 'pending' | 'in-progress' | 'completed' | 'failed'
) => {
return dispatch(updateConnectionStep(connectionId, step, status));
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we would want this to be exposed as part of the connections public interface, the logic for updating connection state should stay in the plugin

Comment on lines 212 to 219
// Get IDs of connections that are currently connecting
const connectingConnectionIds = useConnectionIds(
(connection) => connection.status === 'connecting'
);

// Get IDs of connections that are connected
const connectedConnectionIds = useConnectionIds(
(connection) => connection.status === 'connected'
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably include errored connections also, otherwise they jump in and out of this view in a very weird way

Comment on lines 87 to 89
fontSize: '14px',
color: palette.gray.dark1, // Grayish color for connecting state
Copy link
Collaborator

Choose a reason for hiding this comment

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

We usually dont override LG default colors and fonts (especially font override seems useless here). If you have to do this (usually you really really shouldn't and design also really really shouldn't), you should take dark mode into account (the image is also not very dark mode friendly right now):

image

Comment on lines 145 to 168
{
key: 'listingDatabases',
label: 'Listing Databases...',
status: connectingSteps.listingDatabases,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This status will never meaningfully show up as finished (or would even stay in progress for long) because for compass to consider connection connected and to be functional it doesn't need to list databases

@nbbeeken nbbeeken changed the title feat(compass-welcome): show connection progress on welcomeCOMPASS-9634 feat(compass-welcome): show connection progress on welcome COMPASS-9634 Sep 2, 2025
@github-actions github-actions bot added the feat label Sep 3, 2025
@nbbeeken
Copy link
Collaborator Author

nbbeeken commented Sep 8, 2025

@gribnoysup After wrestling with the "real" statuses for long enough I relent, this ticket was intended to give more a reactive UX for the longer wait times on Web while we worked on real improvements to latency. We actually have met our goal latency for GA, so I think some spinners that just react to the current connected/failed state are good enough. Figured we can do desktop too here :) but easily changed to just be web only.

The problem with accurate spinners is they don't give you the satisfaction you're seeking 😅. I want a nice satisfying check, check, check. In practice the steps are happening too fast to capture in an animation.

Screen.Recording.2025-09-08.at.12.47.34.AM.mov
Screen.Recording.2025-09-08.at.12.53.38.AM.mov

So for posterity, international changes from the designs:

  • In order to work on both envs and not jump the images around I reuse the existing layout and replace the spyglass with the plug
  • I picked a plug img with a back splash in mint, so that it's close to a white background in light theme but the plugs don't become transparent in dark theme on web.
  • There is only doing a spinner for connecting to the cluster, no sub-bullets

I put "no release notes" bc I figure we wouldn't right a note about the welcome page spinners, but maybe that's the wrong assumption 🤔

@nbbeeken nbbeeken marked this pull request as ready for review September 8, 2025 04:57
@Copilot Copilot AI review requested due to automatic review settings September 8, 2025 04:57
@nbbeeken nbbeeken requested a review from a team as a code owner September 8, 2025 04:57
@nbbeeken nbbeeken added the no release notes Fix or feature not for release notes label Sep 8, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds connection progress functionality to the Compass welcome page, allowing users to see real-time status updates when connecting to clusters.

  • Displays connection progress status (connecting, connected, failed) with appropriate icons and animations
  • Switches the welcome page visual from the default tab image to a "plug" image when connections are active
  • Shows a dynamic list of active connections with their current status instead of static welcome content

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
packages/compass-welcome/src/components/welcome-image.tsx Adds new WelcomePlugImage SVG component for active connection state
packages/compass-welcome/src/components/web-welcome-tab.tsx Integrates connection progress display and conditional image switching
packages/compass-welcome/src/components/desktop-welcome-tab.tsx Integrates connection progress display and conditional image switching
packages/compass-welcome/src/components/connection-list.tsx New component handling connection status display with animations and icons
Comments suppressed due to low confidence (1)

packages/compass-welcome/src/components/desktop-welcome-tab.tsx:1

  • The nested ternary operator using && and || operators creates confusing logic flow. Consider refactoring to use explicit if-else conditions or separate the logic into clearer conditional blocks for better readability.
import React from 'react';

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

export function WelcomePlugImage(props: SVGProps<SVGSVGElement>) {
return (
<svg
className="darkreader-ignore-child-inline-style"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Prevents cloud's way of doing dark mode from changing the fills in the svg. Looks exactly the same as compass dark mode now

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Seems like you capilalized some styles by accident, otherwise LGTM

minHeight: `${spacing[200] * 3 + 72}px`,
});

const fadeInFromAbove = keyframes({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just FYI: using css animations like that means that every time this tab is shown you are replaying the animation, looks slightly unpolished, but probably not worth spending more time on

Kapture 2025-09-15 at 10 11 06

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I tried some experimentation but since it unmounts I'd have to track the state outside of the component I think. I think most users will navigate away almost immediately 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can do it by making it a style that applies only as effect after initial render of this view on the newly added items, keeping all the state inside the component, but it is still an overkill for the value we get here for sure 😅

</clipPath>
<style>
{
'.g,.h{fill:none}.i{fill:#e3fcf7}.g,.h,.i,.j,.k,.l,.m,.o,.p{stroke:#000000;stroke-linecap:round;stroke-linejoin:round}.j{fill:#ffc010}.k,.o{fill:#ffffff}.l{fill:#ffc010}.m{fill:#00ed64}.h{stroke-dasharray:0 0 2 2}.o{stroke-width:.8px}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is css, not css in js, so shouldn't be capilatized

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!

@gribnoysup gribnoysup added release notes and removed no release notes Fix or feature not for release notes labels Sep 15, 2025
@gribnoysup
Copy link
Collaborator

gribnoysup commented Sep 15, 2025

RE release notes: I think we're trying to keep the rules as simple as possible, so I'd suggest user facing -> goes into release notes, even if it's a small thing

@nbbeeken nbbeeken merged commit 6b2fc4d into main Sep 15, 2025
56 of 58 checks passed
@nbbeeken nbbeeken deleted the COMPASS-9634 branch September 15, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants