Page MenuHomeElementl

[dagit] Make repo switcher more flexible
ClosedPublic

Authored by dish on Mar 11 2021, 3:50 PM.

Details

Summary

Moving in the direction of allowing the selection of more than one repo in left nav filtering, make the current repo switcher and left nav display more flexible.

  • Stop switching repository context in the left nav based on navigation in Dagit. When the user sets repo context, leave it alone until they change it.
  • Change all pipeline/solid/schedule/sensor lists to allow showing results for an array of repos, not just one.

This requires doing a bit of refactoring of how we handle "workspace context", in that there is no longer an "active" repo to query with. This introduces some complexity in some of our lazy left nav queries, e.g. solids.

The next step here is to replace the existing repo switcher with one that allows selecting multiple repos in the left nav, which will be in a followup.

Test Plan

View Dagit, verify that the left nav repo context remains the same no matter where I navigate in Dagit. Switch repos, verify that all objects are fetched and rendered correctly in the nav.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Remove some commented-out code, don't show reload button if no locations

A little bit more code to delete

dish requested review of this revision.Mar 11 2021, 4:07 PM

This looks great! Just left a couple nits inline. If we get really tired of the Promise.all(client.query...) it'd be fun to explore a useQuery hook that took an array of VariableType and ran N queries and flattened them, but not sure if the response __typename checking would make it hard to generalize...

js_modules/dagit/src/nav/LeftNavRepositorySection.tsx
21

useLSS => useNavVisibleRepos

32

Might want to add a note that this is just for migration so we remember to rm it in 6 months!

js_modules/dagit/src/pipelines/PipelineRoot.tsx
79

Seems slightly odd for this to take repoPath as a path param but infer repoAddress from somewhere else? Probably OK but just want to flag in case!

js_modules/dagit/src/testing/defaultMocks.ts
3โ€“4

hah what exactly is a "word" ๐Ÿ™ˆ I'm sure this was fun to track down...

This revision is now accepted and ready to land.Mar 15 2021, 3:17 AM
js_modules/dagit/src/pipelines/PipelineRoot.tsx
79

Yeah, it's an interesting point.

WorkspaceRoot is currently responsible for determining the validity of the repo address based on the repoPath path parameter, and show a 404-like page if it's bogus.

By this point in the route tree, one level down from WorkspaceRoot, we expect that repoPath *must* map to a valid address. We therefore pass RepoAddress down as a non-null prop, which means we don't have to null-check it or bother with the path param anymore.

I'm probably going to do some refactoring around our route tree at some point in the next several months, so I may revisit it.

js_modules/dagit/src/testing/defaultMocks.ts
3โ€“4

Yeah these produce some pretty lolzy strings...

Rebase, a couple fixes for localStorage, updated tests.

This revision was automatically updated to reflect the committed changes.