Skip to content
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.

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

[ui] Code location: Graphs page #23738

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

hellendag
Copy link
Member

@hellendag hellendag commented Aug 19, 2024

Summary & Motivation

Add a Graphs section to the new Code Locations definitions page.

Screenshot 2024-08-19 at 13 26 48

How I Tested These Changes

Storybook example.

Copy link
Member Author

hellendag commented Aug 19, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @hellendag and the rest of your teammates on Graphite Graphite

@hellendag hellendag requested review from bengotow and salazarm August 19, 2024 18:31
@hellendag hellendag marked this pull request as ready for review August 19, 2024 18:31
Copy link

github-actions bot commented Aug 19, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-ijochgrji-elementl.vercel.app
https://dish-fe-514-code-location-page-graphs.core-storybook.dagster-docs.io

Built with commit c227f53.
This pull request is being automatically deployed with vercel-action

}),
},
delay: 2000,
maxUsageCount: 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, me too. It turns out that otherwise, when you click back and forth between the sections in the storybook example, it repeats the query but fails because it hasn't been mocked more than once! 😆

Comment on lines +28 to +31
const queryResult = useQuery<WorkspaceGraphsQuery, WorkspaceGraphsQueryVariables>(
WORSKPACE_GRAPHS_QUERY,
{variables: {selector}},
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the WorkspaceContext have this same data? I believe it does, in which case we should just pull this data from there using useRepository()

Copy link
Contributor

Choose a reason for hiding this comment

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

(that is only rely on the WorkspaceContext for this data, which should update itself automatically if the code location changes)

Copy link
Contributor

Choose a reason for hiding this comment

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

If not, then I think we should add the extra fields to the WorkspaceContext (though we should make sure they're not super expensive / rely on non-definition level data). cc @alangenfeld

Copy link
Member Author

@hellendag hellendag Aug 20, 2024

Choose a reason for hiding this comment

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

It doesn't look like we query for usedSolids for the workspace queries. I'm also not sure we should add that to the workspace query, since this is will be a fairly low-traffic page and I don't think we need the usedSolids data elsewhere in the app. This page will be (fairly buried) under Deployment > Code location > Graphs, and serves mostly as a browsing tool.

@hellendag hellendag force-pushed the dish/fe-514-code-location-page-graphs branch from 6aba186 to a1d22c7 Compare August 20, 2024 14:23
Copy link
Contributor

@salazarm salazarm left a comment

Choose a reason for hiding this comment

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

Still think this should be fetched as part of the WorkspaceContext (maybe lazily, and cached there)

@hellendag
Copy link
Member Author

Still think this should be fetched as part of the WorkspaceContext (maybe lazily, and cached there)

I don't really have a strong opinion on it. No objection though, as long as it's cheap. (I don't know if it's cheap.)

@salazarm
Copy link
Contributor

I don't really have a strong opinion on it. No objection though, as long as it's cheap. (I don't know if it's cheap.)

The heavy work is pulling the workspace snapshot which this query has to do just to retrieve a tiny sliver of information. The general strategy we've been trying to use is to fetch all of the workspace snapshot definition level data up front and cache it for the whole app. In this case though I do think its true this is rarely used... so I think fetching it lazily would be a better call (use the cache if its available and up to date (if its available then its up to date, otherwise a new snapshot would have overwritten it), otherwise fetch it and put it in the cache).

@hellendag hellendag force-pushed the dish/fe-514-code-location-page-graphs branch from a1d22c7 to f5f965a Compare August 20, 2024 15:55
@hellendag hellendag force-pushed the dish/fe-514-code-location-page-graphs branch from f5f965a to 2d05a6f Compare August 21, 2024 15:42
Copy link
Member Author

hellendag commented Aug 21, 2024

Merge activity

  • Aug 21, 11:10 AM CDT: @hellendag started a stack merge that includes this pull request via Graphite.
  • Aug 21, 11:11 AM CDT: Graphite rebased this pull request as part of a merge.
  • Aug 21, 11:12 AM CDT: @hellendag merged this pull request with Graphite.

@hellendag hellendag force-pushed the dish/fe-514-code-location-page-graphs branch from 2d05a6f to c227f53 Compare August 21, 2024 16:11
@hellendag hellendag merged commit 00e2305 into master Aug 21, 2024
1 of 2 checks passed
@hellendag hellendag deleted the dish/fe-514-code-location-page-graphs branch August 21, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants