Page MenuHomePhabricator

clear cross repo sessions from local storage
ClosedPublic

Authored by prha on Thu, Jan 23, 11:10 PM.

Details

Summary

We persist playground sessions across repos... we should instead
actively check the sessions we load from local storage and filter out any
unrecognizable references.

This implementation does mean that we lose some workspaces across starts
of dagit across different repos. When we start pushing repo information to the client, we can start namespacing sessions by repo...

Test Plan

Load prezi repo, select various presets / partitions, restart
dagit in examples repo, see cleared out sessions

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

prha created this revision.Thu, Jan 23, 11:10 PM
prha retitled this revision from RFC: clear cross repo sessions from local storage to clear cross repo sessions from local storage.Thu, Jan 23, 11:47 PM
prha edited the summary of this revision. (Show Details)
schrockn accepted this revision.Fri, Jan 24, 1:41 AM

Huge fan of this change. This was making demos pretty rough. Let's have @bengotow take a pass on the actual implementation

This revision is now accepted and ready to land.Fri, Jan 24, 1:41 AM
bengotow accepted this revision.Wed, Jan 29, 8:12 PM

Looks good! Just a few minor suggestions ๐Ÿ‘

js_modules/dagit/src/execute/PipelineExecutionRoot.tsx
21

I wonder if it'd make sense to rename this something like useStorageForCurrentPipelines that makes it more clear what the transform is?

31

If you want you could use pipelineNames.includes(session.pipeline) here to make it a bit more concise.

41

I'm not 100% this applyCreateSession transform should be here. I think we may want to just say onLocalStorageSave({...localStorageData, sessions: filteredSessions})

prha added inline comments.Wed, Jan 29, 8:58 PM
js_modules/dagit/src/execute/PipelineExecutionRoot.tsx
21

yeah, I like that better, +1

31

good tip

41

We need to create a session if all of the sessions get filtered out. I'll explicitly pull that out into separate cases instead of unilaterally creating one.

prha updated this revision to Diff 9119.Wed, Jan 29, 8:59 PM

update

This revision was landed with ongoing or failed builds.Wed, Jan 29, 9:14 PM
This revision was automatically updated to reflect the committed changes.