Page MenuHomeElementl

Merge explorer into the Overview tab, remove Definition tab for all users
ClosedPublic

Authored by bengotow on Jun 30 2021, 7:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jun 6, 5:54 AM
Unknown Object (File)
Tue, May 30, 3:37 AM
Unknown Object (File)
Sun, May 28, 11:55 PM
Unknown Object (File)
Fri, May 26, 6:14 AM
Unknown Object (File)
Fri, May 26, 2:08 AM
Unknown Object (File)
Fri, May 26, 1:32 AM
Unknown Object (File)
Sun, May 14, 10:59 AM
Unknown Object (File)
Sat, May 13, 11:32 PM
Subscribers
None

Details

Summary

This diff consolidates the "Definition" and "Overview" pipeline tabs for all users (feature flagged or otherwise). The pipeline's DAG is shown on the left, and the right sidebar shows an expanded "Info" tab with sections for everything on the old overview tab. The info tab is more or less the same with and without the feature flag. The only differences are that with pipeline:mode tuples, the schedules, sensors, and modes displayed are only the ones bound to the mode, and references to "pipeline" are swapped out for "job".

I used a context to inject a separate version of the Info tab into the sidebar when the pipeline explorer is used for the Overview tab as opposed to the Graph explorer pages. I set up a context rather than using props.children or passing the data down into the tree because I think it'd be nice to sprinkle more job- or run- related stuff into the DAG as well, and we might pass down in a single run's logs to colorize the DAG, etc. and need data from the top container all the way down in the tree. If it doesn't pan out I think we could change it.

This diff also fixes the scrolling issue caused by styles on the graph pages and cleans up the pipeline explorer so it doesn't internally modify the explorerPath and can be mounted more flexibly.

Misc:

  • The /overview URL path is gone. For our existing set of "explorerPath" helpers to work, the URL has to contain /pipeline:mode~query/solid/solid and I didn't want to mess with it in this diff. 🙈
  • The explorer sidebar was choosing tabs in a super strange way (my fault i'm sure) and I changed it to use a simple ?tab=types or ?tab=info query string for tab state + updated places where this was set.

image.png (1×1 px, 395 KB)

Test Plan

Give it a spin, run snapshots

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bengotow edited the summary of this revision. (Show Details)
bengotow edited the summary of this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2021, 7:21 AM
Harbormaster failed remote builds in B32885: Diff 40523!
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2021, 7:50 AM
Harbormaster failed remote builds in B32886: Diff 40524!
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2021, 10:09 PM
Harbormaster failed remote builds in B32951: Diff 40609!

Fold the new Graph and Job tabs back down into a single Info tab. This info tab appears more or less the same with and without the feature flag. The only differences are that in the new pipeline:mode tuples setup, the schedules, sensors, and modes displayed are only the ones bound to the mode, and references to pipeline are swapped out for job.

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2021, 11:56 PM
Harbormaster failed remote builds in B32967: Diff 40631!
  • Revert snapshot change - only broken for me?

A handful of comments inline.

js_modules/dagit/packages/core/src/app/App.tsx
52

Does this also need to be removed?

js_modules/dagit/packages/core/src/pipelines/CompositeSupport.ts
141

This looks like a lot of mutating of fragment objects that probably shouldn't be mutated. Seems like it's copied from elsewhere so nbd for now, but we should see if that's avoidable.

js_modules/dagit/packages/core/src/pipelines/PipelineExplorerRoot.tsx
37

This can use useFeatureFlags now.

44

Referencing the mode by keying into history makes me a little itchy. I'm not sure I trust TS to typecheck this correctly. Thoughts on this instead?

const builtPath = buildPath(...);
mode === 'push' ? history.push(builtPath) : history.replace(builtPath);
js_modules/dagit/packages/core/src/pipelines/PipelineRoot.test.tsx
107

Remove?

This revision is now accepted and ready to land.Jul 1 2021, 8:45 PM
  • Fix tests
  • Revert snapshot change - only broken for me?
  • diff feedback