Adopt StorageScope.WORKSPACE and StorageTarget.USER

[joyceerhl]

We now support transferring additional workspace-specific state beyond uncommitted changes in a Continue Working On transition, e.g. from web to a local clone but for the same repository like microsoft/vscode (ref #179898). This allows us to provide continuity when a user needs to work on the same repo while transitioning across different development environments.

Ask: We generally want to transfer any state that is user-generated and ephemeral, such as user queries and drafted changes. Not all state needs to be transferred, and I've compiled an initial list that could make sense to adopt this for below. If I missed any or if you have any questions, please reach out.

Area owners:

• Notebook controller affinity @rebornix
• Find history within an editor @rebornix
• Source control commit message history @joaomoreno
• Pending comments @alexr00
• Text search history @andreamah
• Breakpoints @roblourens @connor4312
• Open editors @bpasero

Details: Your component can opt into this by using the IStorageService under the StorageScope.WORKSPACE and StorageTarget.USER combination to store and retrieve state based on onWillSaveState and onDidChangeValue, e.g.

// in environment A e.g. github.dev
this.storageService.onWillSaveState(() => {
    // no need to serialize objects before storing
    const myObj = { resourceUri };
    this.storageService.store('myKey', myObj, StorageScope.WORKSPACE, StorageTarget.USER);
})

// in environment B e.g. desktop clone of repo
this.storageService.onDidChangeValue((v) => {
    if (v.key === 'myKey') {
        const restoredObj = this.storageService.getObject('myKey', StorageScope.WORKSPACE);
        // apply restored state for your component
    }
});

Any URIs in your stored object values that originated in another instance of VS Code but for the same workspace, e.g. vscode-vfs to file, will be automatically converted for you (if a conversion is possible) before onDidChangeValue is emitted.

Note: The key that you use to retrieve your state should not include non-portable data like URI paths or serialized URIs, since keys will not be automatically converted.

If you have any state already in the workspace and the restored state might step on it, as much as possible your component should try to merge that state (e.g. append new history to existing history) without showing the user a confirmation UI, since the goal is to provide a seamless transition. Again please reach out if this is infeasible.

[roblourens]

So in this scenario, environment B might be already open, and that's why we should listen to onDidChangeValue, right? Currently I only read stored data on startup

[bpasero]

You need to listen for onDidChangeValue because the edit session is applied after the window has opened, possibly after the restored phase. Similar to settings sync, these changes are applied while you are already inside the workbench and thus require the event from being used.

We had to do this kind of adoption for global state already back when settings sync got introduced.

[alexr00]

Can this be a June item? I will not get to it today.

[connor4312]

I suggest that we add a reason field to the IStorageValueChangeEvent, otherwise we may spend a good deal of complexity reconciling things. At least in debug land, we've never listened to this event before and would exclusively be interested in changes that happen via sync.

[bpasero]

@connor4312 can you clarify what you mean, I am not sure I follow that reasoning. Why would you ever not want to react to storage changes?

This issue is about workspace scoped storage. So far, it never changed unless through the component itself in the current window. With settings sync roaming workspace scope, it may now change when the synced data is applied.

[connor4312]

With the addition of a listener, I get events both for sync and as a result of the component itself making changes to the stored values.

In the latter case, I now need to deal with deduplicating these events to avoid doing extra work. In some cases, doing this is simple, but in others it's a little more complex. For example, for breakpoint changes, we need to diff the array of breakpoints (or replace it) and trigger rendering as needed. The requisite diffing for every component and prone to error since it's not easy to locally test "continue working on" (as far as I know)

If there was a reason on the event, I could check for that instead of having to do more complex logic to figure out that I actually don't need to react to this event, and the logic could usually reuse the 'initialization' logic which would reduce the opportunity for error.

[bpasero]

I see the problem but I would argue that its good to deduplicate even when state comes in from a roamed session because it may not be any different from what is already there.

[connor4312]

Perhaps, but I would tend to replace for simplicity if I had that reason. In the breakpoint case, it's an event that will happen at most once per session versus something that happens every time a user modifies a breakpoint.

[andreamah]

@rebornix I think that the search/replace text history and the findInput history both use HistoryInputBox. That contains HistoryNavigator, which is where we store history in memory. Should we sync about how we should listen for changes in history and store properly?

[rebornix]

@andreamah yes let's meet, I wonder if it can be handled by HistoryNavigator by giving it an unique id for each component to make it persistent.

Update

With that said, I wonder if it makes sense to transfer the find history though. It will increase the roaming data size but I'm not sure if we are adding enough value.

[andreamah]

That's true, perhaps if find isn't transferred, it makes sense for search to also not be transferred (and vice versa)?
As for the implementation, that seems like a good way; we could have an optional storageServiceID when initializing the HistoryNavigator

[andreamah]

Currently, how much history do we store?

[joyceerhl]

@connor4312 is the addition of a reason property to the onDidChangeValue event a blocker for you to adopt StorageScope.WORKSPACE with StorageTarget.USER?

[connor4312]

It would definitely make adopting it way easier

[joaomoreno]

@joyceerhl, looked into tackling this for SCM history and need some guidance.

Today, we store SCM history in StorageScope.APPLICATION and StorageTarget.MACHINE.

• We chose StorageScope.APPLICATION since SCM history belongs to a repository, which can appear in multiple workspaces; a repository's history should be preserved even if that repository appears in a different repo workspace.
• We chose StorageTarget.MACHINE since we store URIs that do not translate well between machines. Until now, that is.

I believe we have to drop the repo history preserves across workspaces feature under this new model. Is this a fair assumption? I don't mind if so, I suspect most users won't either.

Also, looking at this from the desired state perspective, and considering that we need to use StorageScope.WORKSPACE and StorageTarget.USER, I see no other viable storage key than something all-encompassing like scm.history, holding one single object with all histories of all repositories within a workspace. If so, would such a schema be reasonable for the stored object?

type SCMHistory = { repositoryRoot: URI, history: string[] }[];

If that's the case, will those repositoryRoot URIs be properly migrated, when roaming? Note that we get those URIs from source control providers.

Thanks for your help!

[joyceerhl]

Yes this is all correct @joaomoreno and the repositoryRoot URIs should be properly migrated as part of the roaming transition.

[joaomoreno]

SCM commit history done: #193142

Most work was in figuring out what the storage format would end up being and then writing the migration code.