Page MenuHomeElementl

[dagit] Prompt to refresh config when repo is refreshed
ClosedPublic

Authored by dish on Jan 25 2021, 10:29 PM.

Details

Summary

When the user reloads their current repository in Dagit, any open playground configurations based on presets or partition sets might now be out of date, even if they don't have any errors. Launching runs using those outdated configs can then result in undesired and confusing output.

This change introduces a warning to give the user an opportunity to reload their configuration. When the repository is reloaded, a message above any preset/partition-based configs appears, indicating that the config may be outdated. Clicking a Refresh config button regenerates the configuration. There is also a "Dismiss" button for those who don't want to respond to the warning.

Sessions that should show this warning are given needsRefresh: true in the session object tracked in localStorage.

In order to make this change, I did a little refactoring to move the config session "save" behavior up a level in the component tree. I also added a function that applies needsRefresh: true across all sessions for pipelines in the reloaded repository, as they are all affected by the reload.

Test Plan

Open the playground for a pipeline, create config tabs based on presets and partition sets. Reload the repository, verify that the warning message appears for all affected config sessions.

Click "Refresh config", verify that the config is regenerated. Alternatively, click "Dismiss" and verify that the warning goes away.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dish requested review of this revision.Jan 25 2021, 10:37 PM
dish edited the test plan for this revision. (Show Details)
dish added reviewers: catherinewu, sashank.

will defer to ben or sashank on the code review. the demo for this looks amazing and can't wait till users get to benefit from it 💃

This looks great and I like the banner styling a lot. I guess there's always a React render after the refresh function is run, so invalidateConfigsForRepo doesn't need to trigger a render after it modifies local storage? It might be good to add a small note to that function to note that the caller should trigger a page refresh in some way, because interacting with the config editor after calling invalidateConfigsForRepo would overwrite the local storage state back to match the config editor's state (I think?) A cheap alternative might be to fire a custom event on the document that components could listen to and handle a repo reload, and then set the dirty bit from the ExecutionSessionContainer, but I think this is clear and is less code!

This revision is now accepted and ready to land.Feb 1 2021, 4:23 PM

Good point, I'm kind of just depending on the cache clearing behavior leading to the re-render, but that is risky.

I could wrap this in a hook to ensure that saving does lead to the refresh. Will give that a try.

Use a stateful hook to ensure React re-renders after updating localStorage.