feat(compass-sidebar): Add loading states for collection in sidebar COMPASS-5211 #2600

gribnoysup · 2021-11-19T09:28:29Z

Description

This PR extracts sidebar navigation into its own component that is fully detached from the application state and rewritten with keyboard support and using leafygreen components and styles where possible. Here's how it looks:

Mouse Navigation	Keyboard Navigation	Async Collections

This PR doesn't flip the switch on the global overlay removal yet as we need the databases-collections change to also be ready before this happens, so if you want to try this new sidenav locally with async you need to set the COMPASS_NO_GLOBAL_OVERLAY env var to true

Direct link to the navigation tree component because github collapsed it (tell me if you think I should split it into more parts, so far I extracted only things that we might use outside of the component)

Checklist

Motivation and Context

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)

It's always there when listing collections

…d a few helpful deps; Fix testing-lib version collision

…e in components

…ehavior following WAI-ARIA spec

…ebouncing when dealing wih asyncronously loaded content

… for the sidebar

…nd clean-up unused code

…debar Conflicts: package-lock.json packages/compass-components/package.json

packages/compass-components/src/components/databases-navigation-tree.tsx

gribnoysup · 2021-11-19T09:38:37Z

packages/compass-components/src/components/databases-navigation-tree.tsx

+});
+
+const databaseItemContainer = css({
+  contentVisibility: 'auto',


As we are now on a pretty modern version of Chromium I though that we might try using content-visibility instead of the react-virtualized. I didn't notice any significant performance degradation, even the opposite as we are doing much less re-layouts now and don't need to watch for the container size changes, but please try it out locally and tell me if you see any weird behaviors

…tion list

…debar Conflicts: package-lock.json packages/compass-components/package.json

mcasimir · 2021-11-19T11:56:18Z

packages/compass-components/src/components/databases-navigation-tree.spec.tsx

@@ -0,0 +1,369 @@
+/* eslint-disable @typescript-eslint/no-empty-function */


Having this component here implies a broader architectural decision, would be great if compass-components could only contain domain-free components and raw building blocks, that's how we are approaching this so far at least.

Breaking this pattern has a good potential to create confusion and we won't have anymore a clear cut as for when to put something in components and when not. For example by the same token the connection form could have ended up here.

Is there a reason (other than ts) why this should not be in the sidebar?

I was assuming the only goal with this package is not to have any direct connection to compass internals (which some hadron components have for example), thanks for clarifying that! The main reason was indeed that converting the whole sidebar plugin codebase to the new ts, code formatting, linting, and testing setup would be too much work upfront and as this side navigation is just a react component, putting it here made sense to me. I'm happy to move all this to the separate new monorepo package or work on converting the plugin to our new tooling, whatever makes more sense in your opinion.

Breaking this pattern has a good potential to create confusion and we won't have anymore a clear cut as for when to put something in components and when not. For example by the same token the connection form could have ended up here.

FWIW just the fact that we would need to make a decision when creating a new component already makes it more confusing than just "put it in compass-components"

I'm totally okay with following the "only contain domain-free components and raw building blocks", but not really sure though why we are doing it. From my POV if keeping plugins (and tbh most of the other packages in the monorepo) in separate packages was more or less a decision based on how they were published before, consumed by Compass, and the fact that they have slightly different build/test setups, for the new components I don't really see a reason to create multiple packages like that, so I wonder what might be the motivation that I'm missing.

Yeah I totally get your point, if is not a big deal for you I'd stick to the pattern where compass-components just domain-free for now. Practically speaking honestly is fine to do this in a separate PR.

I don't mean to be too annoying here, and we can change this rule, but this is my rationale: despite of how good this convention is, and how fit is the current "plugin-based" design with the monorepo, for me we should stick with it until we consciously decide to change, and if and when we do, we should change things everywhere and consistently.

To me is really important that we don't take this decision lightly, and we don't make it happen by accident or as a byproduct of something else (ie ts) cause essentially it would create a situation where 2 different "frontends" would exist, one in the plugins and one in compass-components, and since it won't be fun to undo that, if that's the direction we want to take we should also make a commitment and a plan to move to only one, at least not leaving it to the case.

I'm totally okay with following the "only contain domain-free components and raw building blocks", but not really sure though why we are doing it. From my POV if keeping plugins (and tbh most of the other packages in the monorepo) in separate packages was more or less a decision based on how they were published before, consumed by Compass, and the fact that they have slightly different build/test setups, for the new components I don't really see a reason to create multiple packages like that, so I wonder what might be the motivation that I'm missing.

I think here there are 2 design decisions involved though:

The first decision is for compass-components: this one is basically a central place for reusable UI components, is a way to change in one place how the application looks, kinda a re-export from LG + the components we miss from LG, but that's the level and purpose, just a way to factor together duplicate styles / interactions.

The second one is about the plugins: i don't think the separation they create is so terrible, but i get that we don't need to have them in 100 packages. Also we have many that do not make so much sense. The general fact that we have things with significantly different state / doing different actions and with an independent set of functionalities separated makes sense to me, but yeah those could be just different folders in a package rather then different packages and i'd be ok with that.

Personally i think those should be 2 different decisions, for example if we remove the packages then we could make everything just a folder inside the same package, but compass-components could still be a subfolder, we would import <Button> and colors.gray1 from there and sidebar could be another one.

if is not a big deal for you I'd stick to the pattern where compass-components just domain-free for now

Yeah, absolutely, will move it out, thanks for sharing your thoughts on this.

Done in 22c421e, but tbh I'm not sure if I should've moved more or less stuff, in theory useTree hooks are pretty generic, but then also would we really have another navigation tree view in the app? Idk, anyway, tell me what you think and if this matches what you were expecting

- Provide new app registry events that instance-store can handle to resolve collections metadata and show collection screens - Move collection submenu calls to one place that closest to showing collections on the screen so that we don't need to do it all over the place

…debar

… navigation to its own package

…debar

Anemy

This is real cool. Sick cleanup and adding of accessibility. Feels nice to use/open/close databases and collections. I like the nav tree component and hooks, solid tests too.
Added a couple comments and noticed a few spots we can remove some old code.

I ran into two things playing around with this branch on my computer:
One was scrolling with a lot of collections in the sidebar was making the scroll jump unexpectedly. This can be seen with the test db in the compass-data-sets deployment.

many.collections.scroll.issue.mp4

The other issue I noticed was when I searched for dbs/collections with the filter they weren't loading. Is that work planned elsewhere?

Do you have a database with a lot of collections to test the content-visibility? I think the main reason why we used react-virtualized in the collection/db list was for cases where the user has hundreds or thousands of dbs/collections. Probably would be good to test that manually before merging.

packages/collection-model/lib/model.js

packages/compass-components/src/components/content-with-fallback.tsx

packages/compass-components/src/hooks/use-navigation-tree.ts

Anemy · 2021-11-22T23:19:33Z

packages/compass-components/src/hooks/use-navigation-tree.ts

+
+        const letter = evt.key.toLocaleLowerCase();
+        const maybeNode = nodes
+          .slice(currentFocusElementIndex + 1)


Something from the aria tree view docs:
Search wraps to first node if a matching name is not found among the nodes that follow the focused node.
Maybe on this check we can add the preceding elements after the slice?

I like how we're using the Letter regex here - I couldn't really find good docs for it though, does it match more than just A-Za-z? I'm wondering if we're supporting more languages by using it. Nice.

Good catch! Missed this part of the spec, will fix.

Yeah, it's basically catching all the unicode symbols that have LETTER property. This \p is called unicode property escape and it allows you to match by unicode properties. There is a ton of cases where it can simplify targeting your regex, for example you can catch all emojis with {Emoji_Presentation} group

packages/compass-databases-navigation/src/databases-navigation-tree.tsx

Anemy · 2021-11-22T23:43:57Z

packages/compass-collection/src/modules/tabs.js

            sourcePipeline
          })
        );
+        showCollectionSubmenu({ isReadOnly: isReadonly });


I think we also want to add this to createNewTab or somewhere so that this show collection submenu gets called when the user clicks the Open in New Tab menu item when they're on a database page.

This selectOrCreateTab method uses both createNewTab and replaceTabContent, it also the only method that is always called when tabs are switching, this is why I put it here, I can either move the call out to those methods separately, or keep it here, what do you prefer?

I'm thinking it probably makes sense to move it to them into both createNewTab and replaceTabContent since we can open up a collection view from

compass/packages/compass-collection/src/stores/store.js

Line 36 in 51d3865

createNewTab({

which uses createNewTab without calling selectOrCreateTab.

packages/compass-databases-navigation/src/databases-navigation-tree.tsx

Anemy · 2021-11-22T23:58:22Z

packages/compass-sidebar/src/components/sidebar/sidebar.jsx

 import { toggleIsModalVisible } from '../../modules/is-modal-visible';
 import { saveFavorite } from '../../modules/connection-model';

-import { TOOLTIP_IDS } from '../../constants/sidebar-constants';


Can we remove this variable? I think it's unused now.

Anemy · 2021-11-22T23:58:28Z

packages/compass-sidebar/src/components/sidebar/sidebar.jsx

-
  renderCreateDatabaseButton() {
    if (!this.isReadonlyDistro() && !this.props.isDataLake) {
-      const tooltipText = this.props.description;


Can we remove this description prop now? Looks like it's unused.

Anemy · 2021-11-23T00:10:09Z

packages/compass-databases-navigation/src/databases-navigation-tree.tsx

+});
+
+const databaseItem = css({
+  height: DATABASE_ROW_HEIGHT,


Currently in Compass the database and collections tree items have a cursor: pointer on hover, do we want to keep that functionality? I have no preference.

gribnoysup · 2021-11-23T09:48:31Z

Do you have a database with a lot of collections to test the content-visibility? I think the main reason why we used react-virtualized in the collection/db list was for cases where the user has hundreds or thousands of dbs/collections. Probably would be good to test that manually before merging.

So content-visibility is a property that does same thing that react-virtualized does just natively by the browser. Yes, as I mentioned I tested it with both a database that has 10k collections and a server that has ~1k databases (my local setup couldn't handle creating more than that), the performance looked similar to me (the rendering of the sidebar is mostly fine, but generally it's bad both in this branch and in current Compass, for a different reason though, see COMPASS-5316), but I am not getting the scroll jitter so I'll change it back to react-virtualized (or whatever it was deprecated in favor of, seems like it's react-window now?)

gribnoysup · 2021-11-23T09:59:41Z

The other issue I noticed was when I searched for dbs/collections with the filter they weren't loading. Is that work planned elsewhere?

Is this with or without global overlay? If without then it's kinda expected for now, but I might just fix it in this branch so it's not confusing

…debar

Anemy · 2021-11-23T19:28:08Z

The other issue I noticed was when I searched for dbs/collections with the filter they weren't loading. Is that work planned elsewhere?

Is this with or without global overlay? If without then it's kinda expected for now, but I might just fix it in this branch so it's not confusing

Without. Cool, no worries, just making sure we're aware of it.

…rtualized Keyboard support is not implemented yet, it also needs full rewrite to work with virtual lists (and some tests are definitely broken now)

… for virtual list * and first letter are still not implemented and the tests need a re-write now

…ify types Still need to fix tests and remove stuff from compass-components as it is tightly coupled to the virtual list implementation now

gribnoysup · 2021-11-24T09:52:14Z

@Anemy this turned out to be an almost full rewrite to make it accessible with react-virtualized so even though I'm still cleaning up stuff and writing tests, maybe you already want to take a look if you have a moment?

…debar

…mpass into compass-5211-async-sidebar

Anemy

Good stuff. Tested it out a bit and works so smooth. Feels solid to the touch. Nice work on the accessibility improvements. Good tests and nice comments on some of the trickier things with relevant links 👍
Couple questions/ideas/suggestions, but no blockers. Looks good to go once tests are passing!

Anemy · 2021-11-29T18:27:58Z

configs/mocha-config-compass/register/jsdom-global-register.js

-  window.cancelAnimationFrame = function (id) {
-    clearTimeout(id);
-  };
+  require('global-jsdom')(undefined, { pretendToBeVisual: true });


Anemy · 2021-11-29T18:33:59Z

packages/compass-app-stores/src/stores/instance-store.js

-  if (db.collectionsStatus === 'initial') {
+  if (
+    db.collectionsStatus === 'initial' ||
+    db.collectionsStatus === 'fetching'


Small thing, but how do you feel about having these states as constants/an enum we can refer to?

Does changing that we perform the queries here and in fetchCollectionDetails when the status is fetching have any performance implications? Could we potentially be redoing the same query many times? Just asking for my own understanding, not sure I understand why we're adding this extra status check.

Small thing, but how do you feel about having these states as constants/an enum we can refer to?

I would like to, but I legit don't know where to put them and I don't want to move the models in one package at least in the scope of this work if that's okay (I might do this as a separate PR or something, there is enough reasons for that already, sounds alright?)

Could we potentially be redoing the same query many times?

All inflight requests for the model data are currently debounced (for all the instance related models, but I'll link only to one code example), so if multiple parts of the application are calling instance.fetch, they will be awaiting on the same request promise:

compass/packages/instance-model/lib/model.js

Lines 6 to 35 in a499788

const Inflight = new Map();

function debounceInflight(fn) {

return function (...args) {

const callId = this.isCollection

? `${this.parent.cid}$$coll$$${fn.name}`

: `${this.cid}$$${fn.name}`;

if (Inflight.has(callId)) {

return Inflight.get(callId);

}

const promise = fn.call(this, ...args).finally(() => {

Inflight.delete(callId);

});

Inflight.set(callId, promise);

return promise;

};

}

function debounceActions(actions) {

return {

initialize() {

actions.forEach((key) => {

if (key in this && typeof this[key] === 'function') {

const origFn = this[key];

this[key] = debounceInflight(origFn);

}

});

},

};

}

(side note: this type of debouncing probably belongs more on a transport layer, which is data service in our case, but this is a bigger architectural change, we discussed this in one of the previous PRs and decided that it's okay to keep it at the data model level for now)

This particular check here doesn't mean that we start another fetch while the current one is in progress, it just guarantees we wait for the in-progress one to finish before starting to fetch collection info.

Giving it another look I am thinking that I'll probably move this to the models, similar how the whole instance.refresh flow is now part of the model so that the stores are not doing much except for just calling model methods directly and at least that way the statuses are leaking less outside

Done in 5548eec

Anemy · 2021-11-29T18:50:15Z

packages/compass-collection/src/modules/tabs.js

            sourcePipeline
          })
        );
+        showCollectionSubmenu({ isReadOnly: isReadonly });


I'm thinking it probably makes sense to move it to them into both createNewTab and replaceTabContent since we can open up a collection view from

compass/packages/compass-collection/src/stores/store.js

Line 36 in 51d3865

createNewTab({

which uses createNewTab without calling selectOrCreateTab.

packages/compass-components/src/hooks/use-focus-hover.ts

Anemy · 2021-11-29T19:12:12Z

packages/compass-databases-navigation/src/databases-navigation-tree.tsx

+      >
+        <AutoSizer
+          disableWidth={Boolean(process.env.TEST_AUTOSIZER_DEFAULT_WIDTH)}
+          disableHeight={Boolean(process.env.TEST_AUTOSIZER_DEFAULT_HEIGHT)}


In the interest of making Compass more web compatible, would it make sense to have these as optional props or have them polluting the global namespace a bit for tests instead of process env variables? Fine how it is for Compass at the moment, just a thought.

Yeah, totally, I can move them to props

Done in a2405d7

gribnoysup · 2021-11-30T08:15:49Z

I'm thinking it probably makes sense to move it to them into both createNewTab and replaceTabContent since we can open up a collection view from

Oh, I totally missed that spot, will fix!

Done in 3eeaddc. Although it's not all possible ways to change active tab and thus doesn't work correctly always, it matches the current behavior, so I'm not fixing some issues in this PR, instead I opened a few tickets (COMPASS-5330 and COMPASS-5331) to sort out these existing issues with the menu

…debar

…l fetching logic and status checks to the models

…ethods

gribnoysup added 8 commits November 18, 2021 20:50

fix(collection-model): Do not exctract type as part of the collInfo

9783bba

It's always there when listing collections

misc(compass-components): Move ui colors out of components folder: Ad…

bf3e88a

…d a few helpful deps; Fix testing-lib version collision

feat(compass-components): Add hooks for handling focus and hover stat…

fb1185d

…e in components

feat(compass-components): Add hooks that manage treeview navigation b…

fbffada

…ehavior following WAI-ARIA spec

feat(compass-components): Add helper component to handle loading UI d…

5ba0478

…ebouncing when dealing wih asyncronously loaded content

feat(compass-components): Add new databases navigation tree component…

feb0ae7

… for the sidebar

feat(compass-sidebar): Replace old navigation tree with the new one a…

1c99d63

…nd clean-up unused code

Merge remote-tracking branch 'origin/main' into compass-5211-async-si…

437f504

…debar Conflicts: package-lock.json packages/compass-components/package.json

gribnoysup commented Nov 19, 2021

View reviewed changes

gribnoysup added 2 commits November 19, 2021 11:07

chore(compass-components): Also provide content-visibility for collec…

15c9e88

…tion list

Merge remote-tracking branch 'origin/main' into compass-5211-async-si…

9bfcd9b

…debar Conflicts: package-lock.json packages/compass-components/package.json

mcasimir reviewed Nov 19, 2021

View reviewed changes

gribnoysup added 7 commits November 19, 2021 20:28

Merge remote-tracking branch 'origin/main' into compass-5211-async-si…

758d5ea

…debar

chore(compass-components, compass-databases-navigation): Move sidebar…

22c421e

… navigation to its own package

chore(e2e): Update sidebar selector

20d20e1

chore: Ignore testing-library/dom dependency at the root

52239a3

Merge remote-tracking branch 'origin/main' into compass-5211-async-si…

daf9472

…debar

test(mongodb-compass): Update test for the new menu behavior

9f4d84a

Anemy reviewed Nov 23, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into compass-5211-async-si…

d68819b

…debar

gribnoysup added 4 commits November 23, 2021 21:17

chore(databases-navigation): Refactor the whole thing to use react-vi…

d5f1cc3

…rtualized Keyboard support is not implemented yet, it also needs full rewrite to work with virtual lists (and some tests are definitely broken now)

chore(databases-navigation): Reimplement most of the keyboard support…

4075a31

… for virtual list * and first letter are still not implemented and the tests need a re-write now

chore(databases-navigation): Add rest of the keyboard handlers; Simpl…

89a6bec

…ify types Still need to fix tests and remove stuff from compass-components as it is tightly coupled to the virtual list implementation now

chore(databases-navigation): Add back animations for the loading list

423f1bc

chore(compass-sidebar): Fetch all collections when user starts to filter

87eea6a

gribnoysup added 4 commits November 29, 2021 15:18

Merge remote-tracking branch 'origin/main' into compass-5211-async-si…

d8fab7e

…debar

chore(databases-navigation): Fix tests

7dcb94b

Merge branch 'compass-5211-async-sidebar' of github.com:mongodb-js/co…

24f268e

…mpass into compass-5211-async-sidebar

chore(databases-navigation): Fix more tests

51d3865

Anemy approved these changes Nov 29, 2021

View reviewed changes

gribnoysup added 7 commits November 30, 2021 10:42

Merge remote-tracking branch 'origin/main' into compass-5211-async-si…

7f13e1b

…debar

chore(instance-model, database-model, collection-model): Move optiona…

5548eec

…l fetching logic and status checks to the models

fix(databases-collections): Fix missing initial state

be820e0

chore(compass-components): Remove log statement

ba0aafa

fix(sidebar): Fix initial state test

e045661

chore(databases-navigation): Do not use process.env for test values

a2405d7

chore(collection): Move showCollectionsSubmenu to create/replaceTab m…

3eeaddc

…ethods

gribnoysup merged commit b284d99 into main Nov 30, 2021

gribnoysup deleted the compass-5211-async-sidebar branch November 30, 2021 13:35

		@@ -0,0 +1,369 @@
		/* eslint-disable @typescript-eslint/no-empty-function */

	const Inflight = new Map();

	function debounceInflight(fn) {
	return function (...args) {
	const callId = this.isCollection
	? `${this.parent.cid}$$coll$$${fn.name}`
	: `${this.cid}$$${fn.name}`;
	if (Inflight.has(callId)) {
	return Inflight.get(callId);
	}
	const promise = fn.call(this, ...args).finally(() => {
	Inflight.delete(callId);
	});
	Inflight.set(callId, promise);
	return promise;
	};
	}

	function debounceActions(actions) {
	return {
	initialize() {
	actions.forEach((key) => {
	if (key in this && typeof this[key] === 'function') {
	const origFn = this[key];
	this[key] = debounceInflight(origFn);
	}
	});
	},
	};
	}

feat(compass-sidebar): Add loading states for collection in sidebar COMPASS-5211 #2600

feat(compass-sidebar): Add loading states for collection in sidebar COMPASS-5211 #2600

Conversation

gribnoysup commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Motivation and Context

Types of changes

Uh oh!

Uh oh!

gribnoysup Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcasimir Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcasimir Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anemy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Anemy Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gribnoysup commented Nov 23, 2021

Uh oh!

Anemy commented Nov 23, 2021

Uh oh!

gribnoysup commented Nov 24, 2021

Uh oh!

Anemy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

gribnoysup commented Nov 19, 2021 •

edited

Loading

gribnoysup Nov 19, 2021 •

edited

Loading

mcasimir Nov 19, 2021 •

edited

Loading

gribnoysup Nov 19, 2021 •

edited

Loading

mcasimir Nov 19, 2021 •

edited

Loading

gribnoysup Nov 22, 2021 •

edited

Loading

Anemy left a comment •

edited

Loading

Anemy Nov 22, 2021 •

edited

Loading

gribnoysup commented Nov 23, 2021 •

edited

Loading

gribnoysup Nov 30, 2021 •

edited

Loading

gribnoysup commented Nov 30, 2021 •

edited

Loading