Skip to content

feat(sidebar): show loading state when initially loading connections COMPASS-8876 #6710

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 3 commits into from
Feb 12, 2025
Merged

feat(sidebar): show loading state when initially loading connections COMPASS-8876 #6710

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e13c3d4
feat(sidebar): show loading state when initially loading connections
gribnoysup Feb 11, 2025
d1001b6
chore(testing-library, sidebar): make preload skip interface more exp…
gribnoysup Feb 12, 2025
57a595a
chore(e2e): update helper commands to account for search bar being on…
gribnoysup Feb 12, 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
13 changes: 9 additions & 4 deletions configs/testing-library-compass/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,11 @@ type TestConnectionsOptions = {
*/
preferences?: Partial<AllPreferences>;
/**
* Initial list of connections to be "loaded" to the application
* Initial list of connections to be "loaded" to the application. Empty list
* by default. You can explicitly pass `null` to disable initial preloading of
* the connections
*/
connections?: ConnectionInfo[];
connections?: ConnectionInfo[] | null;
Copy link
Contributor

@djechlin-mongodb djechlin-mongodb Feb 11, 2025

Choose a reason for hiding this comment

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

[nonblocking] think about whether you want "null" to have different semantics from "missing" or "empty." It's a common source of bugs that the meaning of null gets overloaded and it turns up to mean different things in a couple different layers. In this case you can make the type ConnectionInfo[] | no-preload to force English meaning to appear in the code when you make the "null" choice.

Copy link
Collaborator Author

@gribnoysup gribnoysup Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that's fair, I'll switch it for tests helper to be something like that. Main reason I'm not putting much effort into this, is that it's literally two test cases that want this to be disabled, but it's not a great reason to not to put a bit more effort into it, so thanks for holding me accountable 😄

/**
* Connection function that returns DataService when connecting to a
* connection with the connections store. Second argument is a constructor
Expand Down Expand Up @@ -266,6 +268,9 @@ function createWrapper(
TestingLibraryWrapper: ComponentWithChildren = EmptyWrapper,
container?: HTMLElement
) {
const connections =
options.connections === null ? undefined : options.connections ?? [];

const wrapperState = {
globalAppRegistry: new AppRegistry(),
localAppRegistry: new AppRegistry(),
Expand All @@ -274,7 +279,7 @@ function createWrapper(
logger: createNoopLogger(),
connectionStorage:
options.connectionStorage ??
(new InMemoryConnectionStorage(options.connections) as ConnectionStorage),
(new InMemoryConnectionStorage(connections) as ConnectionStorage),
connectionsStore: {
getState: undefined as unknown as () => State,
actions: {} as ReturnType<typeof useConnectionActions>,
Expand Down Expand Up @@ -359,7 +364,7 @@ function createWrapper(
onAutoconnectInfoRequest={
options.onAutoconnectInfoRequest
}
preloadStorageConnectionInfos={options.connections}
preloadStorageConnectionInfos={connections}
>
<StoreGetter>
<TestEnvCurrentConnectionContext.Provider
Expand Down
25 changes: 14 additions & 11 deletions packages/compass-connections/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,20 @@ const CompassConnectionsPlugin = registerHadronPlugin(
{ logger, preferences, connectionStorage, track, globalAppRegistry },
{ addCleanup, cleanup }
) {
const store = configureStore(initialProps.preloadStorageConnectionInfos, {
logger,
preferences,
connectionStorage,
track,
getExtraConnectionData: initialProps.onExtraConnectionDataRequest,
appName: initialProps.appName,
connectFn: initialProps.connectFn,
globalAppRegistry,
onFailToLoadConnections: initialProps.onFailToLoadConnections,
});
const store = configureStore(
initialProps.preloadStorageConnectionInfos ?? null,
Copy link
Contributor

@djechlin-mongodb djechlin-mongodb Feb 11, 2025

Choose a reason for hiding this comment

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

[nonblocking] similar to the above point, is there a way we can better help the reader keep track of what null/undef mean as they read through different parts of the code?

Copy link
Collaborator Author

@gribnoysup gribnoysup Feb 12, 2025

Choose a reason for hiding this comment

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

Hmmm, honestly I think I shouldn't have changed this file at all. Undefined (optional) props are a common pattern in React and from component props perspective this should just still expect preloads to either be passed, then do a thing, or not, then do nothing in all the places, no need to make it expect null in some places, it's not really different from "no value" case here

{
logger,
preferences,
connectionStorage,
track,
getExtraConnectionData: initialProps.onExtraConnectionDataRequest,
appName: initialProps.appName,
connectFn: initialProps.connectFn,
globalAppRegistry,
onFailToLoadConnections: initialProps.onFailToLoadConnections,
}
);

setTimeout(() => {
void store.dispatch(loadConnections());
Expand Down
1 change: 1 addition & 0 deletions packages/compass-connections/src/provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export {
useConnectionsList,
useConnectionsListRef,
connectionsLocator,
useConnectionsListLoadingStatus,
} from './stores/store-context';

export type { ConnectionsService } from './stores/store-context';
Expand Down
1 change: 1 addition & 0 deletions packages/compass-connections/src/stores/connections-store-redux.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ describe('CompassConnections store', function () {
.rejects(new Error('loadAll failed'));

renderCompassConnections({
connections: null,
connectionStorage,
onFailToLoadConnections: onFailToLoadConnectionsSpy,
});
Expand Down
16 changes: 12 additions & 4 deletions packages/compass-connections/src/stores/connections-store-redux.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,8 +469,17 @@ const INITIAL_STATE: State = {
};

export function getInitialConnectionsStateForConnectionInfos(
connectionInfos: ConnectionInfo[] = []
connectionInfos: ConnectionInfo[] | null
Copy link
Collaborator Author

@gribnoysup gribnoysup Feb 11, 2025

Choose a reason for hiding this comment

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

Probably should've been like that from the beginning instead of falling back on empty array and building logic around it, but it's only used in tests so have no real effect on anything until now

): State['connections'] {
if (!connectionInfos) {
// Keep initial state if we're not preloading any connections
return {
byId: {},
ids: [],
status: 'initial',
error: null,
};
}
const byId = Object.fromEntries<ConnectionState>(
connectionInfos.map((info) => {
return [info.id, createDefaultConnectionState(info)];
Expand All @@ -479,8 +488,7 @@ export function getInitialConnectionsStateForConnectionInfos(
return {
byId,
ids: getSortedIdsForConnections(Object.values(byId)),
// Keep initial state if we're not preloading any connections
status: connectionInfos.length > 0 ? 'ready' : 'initial',
status: 'ready',
error: null,
};
}
Expand Down Expand Up @@ -2126,7 +2134,7 @@ export const openSettingsModal = (
};

export function configureStore(
preloadConnectionInfos: ConnectionInfo[] = [],
preloadConnectionInfos: ConnectionInfo[] | null,
thunkArg: ThunkExtraArg
) {
return createStore(
Expand Down
11 changes: 11 additions & 0 deletions packages/compass-connections/src/stores/store-context.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,3 +364,14 @@ export function useConnectionsColorList(): {
});
}, isEqual);
}

export function useConnectionsListLoadingStatus() {
return useSelector((state) => {
const status = state.connections.status;
return {
status,
error: state.connections.error?.message ?? null,
isInitialLoad: status === 'initial' || status === 'loading',
};
}, isEqual);
}
3 changes: 3 additions & 0 deletions packages/compass-sidebar/src/components/connections-filter-popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,15 @@ type ConnectionsFilterPopoverProps = PropsWithChildren<{
onFilterChange(
updater: (filter: ConnectionsFilter) => ConnectionsFilter
): void;
disabled?: boolean;
}>;

export default function ConnectionsFilterPopover({
open,
setOpen,
filter,
onFilterChange,
disabled = false,
}: ConnectionsFilterPopoverProps) {
const onExcludeInactiveChange = useCallback(
(excludeInactive: boolean) => {
Expand Down Expand Up @@ -103,6 +105,7 @@ export default function ConnectionsFilterPopover({
active={open}
aria-label="Filter connections"
ref={ref}
disabled={disabled}
>
<Icon glyph="Filter" />
{isActivated && (
Expand Down
94 changes: 66 additions & 28 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
Expand Up @@ -18,6 +18,8 @@ import {
Button,
Icon,
ButtonVariant,
cx,
Placeholder,
} from '@mongodb-js/compass-components';
import { ConnectionsNavigationTree } from '@mongodb-js/compass-connections-navigation';
import type { MapDispatchToProps, MapStateToProps } from 'react-redux';
Expand All @@ -38,6 +40,7 @@ import type { RootState, SidebarThunkAction } from '../../modules';
import {
type useConnectionsWithStatus,
ConnectionStatus,
useConnectionsListLoadingStatus,
} from '@mongodb-js/compass-connections/provider';
import {
useOpenWorkspace,
Expand Down Expand Up @@ -89,6 +92,10 @@ const connectionCountStyles = css({
marginLeft: spacing[100],
});

const connectionCountDisabledStyles = css({
opacity: 0.6,
});

const noDeploymentStyles = css({
paddingLeft: spacing[400],
paddingRight: spacing[400],
Expand Down Expand Up @@ -305,7 +312,7 @@ const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({
}

return actions;
}, [supportsConnectionImportExport]);
}, [supportsConnectionImportExport, enableCreatingNewConnections]);

const onConnectionItemAction = useCallback(
(
Expand Down Expand Up @@ -491,6 +498,17 @@ const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({

const isAtlasConnectionStorage = useContext(AtlasClusterConnectionsOnly);

const { isInitialLoad: isInitialConnectionsLoad } =
useConnectionsListLoadingStatus();

const connectionsCount = isInitialConnectionsLoad ? (
<span className={cx(connectionCountStyles, connectionCountDisabledStyles)}>
(…)
</span>
) : connections.length !== 0 ? (
<span className={connectionCountStyles}>({connections.length})</span>
) : undefined;

return (
<div className={connectionsContainerStyles}>
<div
Expand All @@ -499,11 +517,7 @@ const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({
>
<Subtitle className={connectionListHeaderTitleStyles}>
{isAtlasConnectionStorage ? 'Clusters' : 'Connections'}
{connections.length !== 0 && (
<span className={connectionCountStyles}>
({connections.length})
</span>
)}
{connectionsCount}
</Subtitle>
<ItemActionControls<ConnectionListTitleActions>
iconSize="xsmall"
Expand All @@ -514,27 +528,25 @@ const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({
collapseAfter={2}
></ItemActionControls>
</div>
{connections.length > 0 && (
<>
<NavigationItemsFilter
Copy link
Collaborator Author

@gribnoysup gribnoysup Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'll double-check with design as I couldn't find this explicitly mentioned, but in designs we show search bar while loading, so it would be weird to yank it out when we finished, it's a noticeable and annoying visual jump, so I'm continuing to show it, just similarly disabled as when loading

Copy link
Contributor

@djechlin-mongodb djechlin-mongodb Feb 11, 2025

Choose a reason for hiding this comment

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

[no action] sgtm. I was going to suggest testing by deliberately hanging the load indefinitely and just see how many things you can click in the UI that you shouldn't be able to click. sounds like you experimented with that which is good!

Copy link
Collaborator Author

@gribnoysup gribnoysup Feb 12, 2025

Choose a reason for hiding this comment

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

Can definitely extend the test I added to do that to some extent, good suggestion, thanks!

placeholder={
isAtlasConnectionStorage
? 'Search clusters'
: 'Search connections'
}
filter={filter}
onFilterChange={onFilterChange}
/>
<ConnectionsNavigationTree
connections={filtered || connections}
activeWorkspace={activeWorkspace}
onItemAction={onItemAction}
onItemExpand={onItemExpand}
expanded={expanded}
/>
</>
)}
{connections.length === 0 && (
<NavigationItemsFilter
placeholder={
isAtlasConnectionStorage ? 'Search clusters' : 'Search connections'
}
filter={filter}
onFilterChange={onFilterChange}
disabled={isInitialConnectionsLoad || connections.length === 0}
/>
{isInitialConnectionsLoad ? (
<ConnectionsPlaceholder></ConnectionsPlaceholder>
) : connections.length > 0 ? (
<ConnectionsNavigationTree
connections={filtered || connections}
activeWorkspace={activeWorkspace}
onItemAction={onItemAction}
onItemExpand={onItemExpand}
expanded={expanded}
/>
) : connections.length === 0 ? (
<div className={noDeploymentStyles}>
<Body data-testid="no-deployments-text">
You have not connected to any deployments.
Expand All @@ -550,11 +562,37 @@ const ConnectionsNavigation: React.FC<ConnectionsNavigationProps> = ({
</Button>
)}
</div>
)}
) : null}
</div>
);
};

const placeholderListStyles = css({
Copy link
Contributor

@djechlin-mongodb djechlin-mongodb Feb 11, 2025

Choose a reason for hiding this comment

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

there was also the option of skeleton loader in leafygreen, I think Simon suggested it in the jira, did you want to try that out?

Copy link
Collaborator Author

@gribnoysup gribnoysup Feb 12, 2025

Choose a reason for hiding this comment

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

I'll open a follow-up ticket for that. Our component that we already use in other parts of the navigation tree (and this is why I used it here) is basically proto version of the leafygreen Skeleton, we implemented it before skeleton component existed. We should switch to leafygreen version, but I don't want to make this refactoring part of this PR, I also don't want to leave two different placeholders hanging around in the codebase

display: 'grid',
gridTemplateColumns: '1fr',
// placeholder height that visually matches font size (16px) + vertical
// spacing (12px) to align it visually with real items
gridAutoRows: spacing[400] + spacing[300],
alignItems: 'center',
// navigation list padding + "empty" caret icon space (4px) to align it
// visually with real items
paddingLeft: spacing[400] + spacing[100],
paddingRight: spacing[400],
});

function ConnectionsPlaceholder() {
return (
<div
data-testid="connections-placeholder"
className={placeholderListStyles}
>
{Array.from({ length: 3 }, (_, index) => (
<Placeholder key={index} height={spacing[400]}></Placeholder>
))}
</div>
);
}

const onRefreshDatabases = (connectionId: string): SidebarThunkAction<void> => {
return (_dispatch, getState, { globalAppRegistry }) => {
globalAppRegistry.emit('refresh-databases', { connectionId });
Expand Down
7 changes: 6 additions & 1 deletion packages/compass-sidebar/src/components/multiple-connections/sidebar.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ describe('Multiple Connections Sidebar Component', function () {

function doRender(
activeWorkspace: WorkspaceTab | null = null,
connections: ConnectionInfo[] = [savedFavoriteConnection],
connections: ConnectionInfo[] | null = [savedFavoriteConnection],
atlasClusterConnectionsOnly: boolean | undefined = undefined
) {
workspace = sinon.spy({
Expand Down Expand Up @@ -235,6 +235,11 @@ describe('Multiple Connections Sidebar Component', function () {
});

describe('connections list', function () {
it('should display a loading state while connections are not loaded yet', function () {
doRender(null, null);
expect(screen.getByTestId('connections-placeholder')).to.be.visible;
});

context('when there are no connections', function () {
it('should display an empty state with a CTA to add new connection', function () {
doRender(undefined, []);
Expand Down
4 changes: 4 additions & 0 deletions packages/compass-sidebar/src/components/navigation-items-filter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export default function NavigationItemsFilter({
title = 'Search',
filter,
onFilterChange,
disabled = false,
}: {
placeholder?: string;
ariaLabel?: string;
Expand All @@ -38,6 +39,7 @@ export default function NavigationItemsFilter({
onFilterChange(
updater: (filter: ConnectionsFilter) => ConnectionsFilter
): void;
disabled?: boolean;
}): React.ReactElement {
const onChange = useCallback<React.ChangeEventHandler<HTMLInputElement>>(
(event) => {
Expand Down Expand Up @@ -66,12 +68,14 @@ export default function NavigationItemsFilter({
title={title}
onChange={onChange}
className={textInputStyles}
disabled={disabled}
/>
<ConnectionsFilterPopover
open={isPopoverOpen}
setOpen={setPopoverOpen}
filter={filter}
onFilterChange={onFilterChange}
disabled={disabled}
/>
</form>
);
Expand Down
Loading