feat(compass-workspaces): introduce workspaces plugin; implement single top level tabs COMPASS-7354 #5169

gribnoysup · 2023-11-28T14:39:38Z

This patch is a first step in refactoring all tab handling logic into a single plugin to make sure that we have a single source of truth for the current active tab and don't need to rely on global events to sync those. For more details refer to the tech design.

As this touches a lot of code in Compass, this current PR introduces a minimal amount of changes to preserve feature parity with current tabs implementation, but doesn't include a full refactor for the tab opening events. It's just too much code changes already, I wanted to stop at something that is already mergeable. Some additional refactoring will be introduced in the follow-up PRs.

Changes in this PR:

Introduce a single top level row of tabs for all workspace types through the compass-workspaces plugin. Instead of having instance workspaces, database workspaces, and collection tab workspaces have a different top level tab logic, all these are now merged into a single row of tabs that work similar to the collection tabs
Remove compass-instance, compass-database, and collection-tabs plugins. They are redundant, compass-workspaces is now handling this logic.
Convert collection tab content plugin to new interface. Required so that compass-workspaces plugin can render it without relying on the old configureStore interface

Still TODO in this PR:

Update tab visuals according to the new designs
Add "Performance" item to the sidebar navigation
Update e2e tests to handle the new tabs

Follow-up PRs to finish the work on the compass-workspaces will include:

Move compass-sidebar inside the compass-workspaces plugin, pass active tab state to the sidebar component directly
Make compass-collection responsible for getting collection metadata from the model. Only pass namespace and initialAgg / initialQuery to the plugin component as a prop
Convert database collections-list plugin to the new interface. Pass database namespace as a prop to the plugin component
Remove namespace state from the compass-home plugin, use onWorkspaceChange prop instead of all the side-effects
Provide a convenient openWorkspace method exported from the compass-workspaces plugin to be used instead of events

All listed follow ups are better isolated from each other, so hopefully this is the only big PR needed for this particular work

…le top level tabs

gribnoysup · 2023-11-28T14:40:18Z

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

-      })
-    );
-  };
-  onAppRegistryEvent('active-collection-dropped', onActiveCollectionDropped);


Not needed anymore, we just listen to the add / remove change events on the instance model

gribnoysup · 2023-11-28T14:45:11Z

packages/compass-workspaces/src/stores/workspaces.ts

GitHub collapsed the main store for the plugin

gribnoysup · 2023-11-28T14:52:07Z

packages/compass-workspaces/src/stores/workspaces.ts

+      action.workspace.type === 'My Queries' ||
+      action.workspace.type === 'Databases' ||
+      action.workspace.type === 'Performance'


I'll be moving tests from tabs.ts store to this one, in the meantime, the gist for the tab behavior is that we're trying to preserve the current tab / workspace relation:

If you open a new tab for the instance stuff, we'll always try to find an open one and switch to it, this matches the current compass behavior where we don't really have any way to open any of those in the new tab.

If you open anything that requires namespace: database collections list or collection tab, we'll replace the current active tab content with the new namespace tab, but only if the current tab type matches the new one that we're trying to open.

For example: if you'd try to open a collection while looking at "my queries" tab, this will open a new tab with the selected collection namespace. But if it's a collection tab for a different namespace that is active, we'll replace the content, same way it works right now.

For this project we're just keeping the behavior as close to the current one as possible with the change. Some changes might be made in the follow up project that is more focused on the UI part of compass web.

packages/compass-workspaces/src/stores/workspaces.ts

gribnoysup · 2023-11-28T14:54:45Z

packages/compass-workspaces/src/stores/workspaces.ts

+  };
+};
+
+// TODO: events are re-emitted when tab is changed for compatibility reasons,


This is basically the biggest hack this PR has while sidebar and collections list plugins still relies on multiple events to get the current active workspaces state synced with its internal state. Removing the need for this will be the first thing I'll be addressing in the follow-up PRs

addaleax

Looks pretty neat so far!

packages/compass-home/package.json

package-lock.json

addaleax · 2023-11-28T15:22:17Z

packages/compass-workspaces/.prettierrc.json

interesting that git recognizes these a file move :) makes sense, still surprising

packages/compass-components/src/components/workspace-tabs/tab.tsx

packages/compass-collection/src/stores/collection-tab.ts

packages/compass-collection/src/stores/collection-tab.spec.ts

packages/compass-collection/src/index.ts

packages/compass-workspaces/src/components/workspaces.tsx

packages/compass-workspaces/src/stores/workspaces.ts

…workspaces

lerouxb · 2023-12-01T08:51:39Z

packages/compass-collection/src/stores/collection-tab.ts

  pipelineText?: string;
  editViewName?: string;
-} & CollectionMetadata;
+} & CollectionMetadata; // TODO: make collection-tab resovle metadata on its own


I'm gonna assume these TODO comments are for the immediate follow-up PRs and not insist on ticket numbers :)

Actually let me still add them, it would be easier to track those

packages/compass-collection/src/stores/collection-tab.ts

lerouxb · 2023-12-01T09:00:02Z

packages/compass-components/src/components/workspace-tabs/workspace-tabs.tsx

    navigationProps
  );

+  useHotkeys(


All this is 😘

lerouxb · 2023-12-01T09:03:03Z

packages/compass-e2e-tests/helpers/commands/collection-workspaces.ts

+  // Close all the workspace tabs to get rid of all the state we
+  // might have accumulated. This is the only way to get back to the zero
+  // state of Schema, and Validation tabs without re-connecting.
+  await browser.closeWorkspaceTabs();


Nice work cleaning up all these commands. Must have been a pain.

lerouxb · 2023-12-01T09:08:42Z

packages/compass-sidebar/src/components/navigation-items.tsx

-  isDataLake,
-  isWritable,
-  changeFilterRegex,
+  showCreateDatabaseAction = false,


Nice! Much better naming here.

lerouxb · 2023-12-01T09:12:06Z

packages/compass-sidebar/src/components/navigation-items.tsx

+  );
+  const isDataLake = instanceFetched
+    ? state.instance?.dataLake.isDataLake
+    : true;


This is a total nit, but since you're making the assumption that something is datalake when the instance isn't fetched so that the perf item won't show by default it is probably better/safer to just name this temp var showPerformanceItem immediately. Just in case someone comes along in future and uses isDataLake to display something specific to datalake and in that case you probably want false as a safer default?

Although that would complicate showCreateDatabaseAction below where you use isDataLake again, so maybe more trouble than its worth 🤔

Hmmm, yeah, good point, I feel like I can just move this logic to a function where default value is provided and then it's not that complicated to use and the intent is explicit

Refactored this a bit in 60f022d. A bit more verbose, but probably not a bad thing?

lerouxb

Amazing work! The new tabs are so much nicer.

… first try

Anemy · 2023-12-01T16:53:11Z

Trying this out a bit, I noticed that if you have two tabs of the same namespace open and click the tab on the right it only will open the tab on the left.

…nt active one when selecting

gribnoysup · 2023-12-01T17:23:54Z

@Anemy nice catch! Fixed and added a test in b47a66e

Anemy

When I'm on the database tab and I click on a collection it opens the collection in a new tab. Is that expected? I'm thinking folks might end up with a lot of database tabs. Might have missed this if it was brought up elsewhere.

Looks like I get a runtime error when the active collection tab's collection is deleted and it's the only open tab:

Might be something we want to catch?

I dig the leaf on no open tab states, and the navigation to database when the active tab collection is deleted. Tabs look great.

gribnoysup · 2023-12-02T07:10:22Z

When I'm on the database tab and I click on a collection it opens the collection in a new tab. Is that expected?

It is in this implementation, yes. We can't exactly re-create the old behavior without bringing some old weirdness with it because we never had just one level of tabs before, so the behavior is as close to it as possible, but not exactly the same, I described it in the comment above and you can take a closer look at the reducer action handling. I personally feel like that's okay, we're adding tabs so that users can open a bunch of them and close if needed, and would rather adjust this later based on user feedback 🙂

Do you have concerns about more tabs opening? If yes, I can introduce a special workaround specifically for "Collections" workspace for now to make it even closer to the current behavior, it will stand out from how other tabs will work, but I'd rather try to avoid blocking this work on a bigger UX discussion, so happy to change this.

Looks like I get a runtime error when the active collection tab's collection is deleted and it's the only open tab:

Thanks for spotting! Fixed in 5da6966

Anemy

lgtm!!
I do feel folks could end up with too many tabs if we're opening a new one everytime they click on a collection/database from one of the higher up namespace pages. I think it's fine for this version though. We can adjust later if we get feedback from folks.

gribnoysup · 2023-12-04T09:51:45Z

Alrighty, merging!

feat(compass-workspaces): introduce workspaces plugin; implement sing…

be25e18

…le top level tabs

github-actions bot added the feat label Nov 28, 2023

gribnoysup commented Nov 28, 2023

View reviewed changes

gribnoysup added the release notes label Nov 28, 2023

addaleax reviewed Nov 28, 2023

View reviewed changes

chore(compass-components): redesign workspace tabs

e075009

gribnoysup force-pushed the compass-7354-compass-workspaces branch from 3ff379c to e075009 Compare November 28, 2023 17:52

chore(sidebar): add Performance item to the sidebar

4e7dc6f

gribnoysup force-pushed the compass-7354-compass-workspaces branch from 9ef8d84 to 4e7dc6f Compare November 29, 2023 11:08

gribnoysup added 11 commits November 29, 2023 14:58

chore(workspaces): workspace type based on tab plugins

1ec06fb

chore: depcheck

afe2d8c

Merge remote-tracking branch 'origin/main' into compass-7354-compass-…

0164414

…workspaces

chore: more depcheck

68ea770

chore: lint and broken unit test fixes

8331011

Merge remote-tracking branch 'origin/main' into compass-7354-compass-…

cf721b0

…workspaces

chore(workspaces): add missing event listeners

c3dd82f

chore(app-registry): add activate helpers

300e337

chore(e2e): update e2e tests for the new workspace tabs

8252a6a

chore(workspaces): empty state and plus button behavior

19d049f

Merge branch 'main' into compass-7354-compass-workspaces

9afc34c

gribnoysup marked this pull request as ready for review November 30, 2023 13:53

chore(workspaces): add tabs tests

b1fa44f

gribnoysup force-pushed the compass-7354-compass-workspaces branch from b98f460 to b1fa44f Compare November 30, 2023 15:18

chore(components): fix tab focus state and visible focus color

5fb0eb0

lerouxb reviewed Dec 1, 2023

View reviewed changes

packages/compass-collection/src/stores/collection-tab.ts Outdated Show resolved Hide resolved

chore(components): update tab test

c3134e8

lerouxb reviewed Dec 1, 2023

View reviewed changes

packages/compass-components/src/components/workspace-tabs/workspace-tabs.tsx

navigationProps

);

useHotkeys(

Copy link

Contributor

lerouxb Dec 1, 2023

Choose a reason for hiding this comment

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

All this is 😘

lerouxb reviewed Dec 1, 2023

View reviewed changes

lerouxb approved these changes Dec 1, 2023

View reviewed changes

gribnoysup added 2 commits December 1, 2023 17:43

chore(app-stores): show error toast if failed to get instance info on…

d32ff56

… first try

chore(collection): remove duplicated prop from expression

1a85724

gribnoysup added 2 commits December 1, 2023 18:09

chore(sidebar): be more explicit about default value for instance props

60f022d

fix(workspaces): when opening an existing workspace, prioritize curre…

b47a66e

…nt active one when selecting

Anemy reviewed Dec 1, 2023

View reviewed changes

fix(workspaces): handle empty tabs state on namespace remove

5da6966

Anemy approved these changes Dec 2, 2023

View reviewed changes

gribnoysup merged commit 87f6468 into main Dec 4, 2023

gribnoysup deleted the compass-7354-compass-workspaces branch December 4, 2023 09:51

feat(compass-workspaces): introduce workspaces plugin; implement single top level tabs COMPASS-7354 #5169

feat(compass-workspaces): introduce workspaces plugin; implement single top level tabs COMPASS-7354 #5169

Uh oh!

Conversation

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

gribnoysup Dec 1, 2023 • 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!

lerouxb left a comment

Choose a reason for hiding this comment

Uh oh!

Anemy commented Dec 1, 2023

Uh oh!

gribnoysup commented Dec 1, 2023

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!

gribnoysup commented Dec 2, 2023

Uh oh!

Anemy left a comment

Choose a reason for hiding this comment

Uh oh!

gribnoysup commented Dec 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gribnoysup commented Nov 28, 2023 •

edited

Loading

gribnoysup Dec 1, 2023 •

edited

Loading

Anemy left a comment •

edited

Loading