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 all commits
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
32 changes: 25 additions & 7 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,12 @@ 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 `no-preload` to disable initial
* preloading of the connections, otherwise connections are always preloaded
* before rendering when using helper methods
*/
connections?: ConnectionInfo[];
connections?: ConnectionInfo[] | 'no-preload';
/**
* Connection function that returns DataService when connecting to a
* connection with the connections store. Second argument is a constructor
Expand Down Expand Up @@ -245,6 +248,12 @@ const EmptyWrapper = ({ children }: { children: React.ReactElement }) => {
return <>{children}</>;
};

function getConnectionsFromConnectionsOption(
connections: TestConnectionsOptions['connections']
): Exclude<TestConnectionsOptions['connections'], 'no-preload'> {
return connections === 'no-preload' ? undefined : connections ?? [];
}

const TEST_ENV_CURRENT_CONNECTION = {
info: {
id: 'TEST',
Expand All @@ -266,6 +275,7 @@ function createWrapper(
TestingLibraryWrapper: ComponentWithChildren = EmptyWrapper,
container?: HTMLElement
) {
const connections = getConnectionsFromConnectionsOption(options.connections);
const wrapperState = {
globalAppRegistry: new AppRegistry(),
localAppRegistry: new AppRegistry(),
Expand All @@ -274,7 +284,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 +369,7 @@ function createWrapper(
onAutoconnectInfoRequest={
options.onAutoconnectInfoRequest
}
preloadStorageConnectionInfos={options.connections}
preloadStorageConnectionInfos={connections}
>
<StoreGetter>
<TestEnvCurrentConnectionContext.Provider
Expand Down Expand Up @@ -416,7 +426,9 @@ function renderWithConnections(
hydrate,
});
expect(
(connectionsOptions.connections ?? []).every((info) => {
(
getConnectionsFromConnectionsOption(connectionsOptions.connections) ?? []
).every((info) => {
return !!wrapperState.connectionsStore.getState().connections.byId[
info.id
];
Expand Down Expand Up @@ -510,7 +522,10 @@ async function renderWithActiveConnection(
const renderResult = renderWithConnections(ui, {
...options,
wrapper: ConnectionInfoWrapper,
connections: [connectionInfo, ...(connections ?? [])],
connections: [
connectionInfo,
...(getConnectionsFromConnectionsOption(connections) ?? []),
],
});
await waitForConnect(renderResult.connectionsStore, connectionInfo);
return renderResult;
Expand All @@ -532,7 +547,10 @@ async function renderHookWithActiveConnection<HookProps, HookResult>(
const renderHookResult = renderHookWithConnections(cb, {
...options,
wrapper: ConnectionInfoWrapper,
connections: [connectionInfo, ...(connections ?? [])],
connections: [
connectionInfo,
...(getConnectionsFromConnectionsOption(connections) ?? []),
],
});
await waitForConnect(renderHookResult.connectionsStore, connectionInfo);
return renderHookResult;
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: 'no-preload',
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[]
): 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[] | undefined,
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);
}
7 changes: 6 additions & 1 deletion packages/compass-e2e-tests/helpers/commands/connect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,12 @@ export async function waitForConnectionResult(
): Promise<string | undefined> {
const waitOptions = typeof timeout !== 'undefined' ? { timeout } : undefined;

if (await browser.$(Selectors.SidebarFilterInput).isDisplayed()) {
if (
(await browser.$(Selectors.SidebarFilterInput).isDisplayed()) &&
(await browser
.$(Selectors.SidebarFilterInput)
.getAttribute('aria-disabled')) !== 'true'
) {
// Clear the filter to make sure every connection shows
await browser.clickVisible(Selectors.SidebarFilterInput);
await browser.setValueVisible(Selectors.SidebarFilterInput, '');
Expand Down
7 changes: 6 additions & 1 deletion packages/compass-e2e-tests/helpers/commands/disconnect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,12 @@ async function resetForDisconnect(
// and therefore be rendered.
await browser.clickVisible(Selectors.CollapseConnectionsButton);

if (await browser.$(Selectors.SidebarFilterInput).isDisplayed()) {
if (
(await browser.$(Selectors.SidebarFilterInput).isDisplayed()) &&
(await browser
.$(Selectors.SidebarFilterInput)
.getAttribute('aria-disabled')) !== 'true'
) {
// Clear the filter to make sure every connection shows
await browser.clickVisible(Selectors.SidebarFilterInput);
await browser.setValueVisible(Selectors.SidebarFilterInput, '');
Expand Down
7 changes: 6 additions & 1 deletion packages/compass-e2e-tests/helpers/commands/remove-connections.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,12 @@ async function resetForRemove(browser: CompassBrowser) {
// and therefore be rendered.
await browser.clickVisible(Selectors.CollapseConnectionsButton);

if (await browser.$(Selectors.SidebarFilterInput).isDisplayed()) {
if (
(await browser.$(Selectors.SidebarFilterInput).isDisplayed()) &&
(await browser
.$(Selectors.SidebarFilterInput)
.getAttribute('aria-disabled')) !== 'true'
) {
// Clear the filter to make sure every connection shows
await browser.clickVisible(Selectors.SidebarFilterInput);
await browser.setValueVisible(Selectors.SidebarFilterInput, '');
Expand Down
7 changes: 6 additions & 1 deletion packages/compass-e2e-tests/helpers/commands/sidebar-connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,12 @@ export async function removeConnection(
connectionName: string
): Promise<boolean> {
// make sure there's no filter because if the connection is not displayed then we can't remove it
if (await browser.$(Selectors.SidebarFilterInput).isExisting()) {
if (
(await browser.$(Selectors.SidebarFilterInput).isExisting()) &&
(await browser
.$(Selectors.SidebarFilterInput)
.getAttribute('aria-disabled')) !== 'true'
) {
await browser.clickVisible(Selectors.SidebarFilterInput);
await browser.setValueVisible(Selectors.SidebarFilterInput, '');

Expand Down
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
Loading
Loading