Notebook diffs not showing when triggered from GHPR extension

[dynamicwebpaige]

When viewing changes I make to a .ipynb file while using github.dev, the diffs render as expected (nicely, with red and green highlighting to indicate changes in code or state):

When reviewing changes that other users have made to .ipynb files, the pull requests still render as JSON blobs:

Browser: Microsoft Edge Version 94.0.992.31 (Official build) (x86_64) OS: MacOS Big Sur v11.6 Repro link: https://github.dev/ageron/handson-ml2/pull/408

[roblourens]

This is about opening notebooks through the GHPR extension vs SCM view.

@lramos15 how is that route different, how do we get it to open in the notebook editor?

[lramos15]

I assume they're both using the vscode.diff command?

[roblourens]

Yeah. Main difference at openEditor is that the GHPR case has an editor with scheme pr:, we should still be picking the notebook editor and opening it there in the same way.

[lramos15]

Yeah file scheme shouldn't matter. I'll investigate this as I assume it's a resolver issue. We match on the extension using a glob. Will reassign to notebook folks if it turns out to be a notebook problem.

[bpasero]

This is the culprit:

https://github.com/microsoft/vscode/blob/bfc47075381995d15a31bb93e69a4f50c9557756/src/vs/workbench/contrib/notebook/browser/notebookServiceImpl.ts#L164

If you check for canHandleResource you might wrongly assume it cannot be handled because the file system provider extension has not been activated yet. I suggest that the editor resolver calls fileService.activateProvider when in a Promise context or we remove the check altogether, not sure why it was added in the first place.

[lramos15]

Now I feel like we're moving in circles 😅 .

canHandleResource was added because we wanted to make sure editors were only called for schemes which they could support. The original bug was filed because notebooks couldn't support the PR scheme?

See https://github.com/microsoft/vscode/issues/122627

[bpasero]

Its OK to use that method but you need to call the other one too. In next debt week I will deprecate the "sync" version and offer a "async" version that does that internally.

[lramos15]

@bpasero Can I activate all providers similar to how we wait for extensions to register before proceeding with the lookup

[bpasero]

@lramos15 I do not think there is a way to activate all extensions file system providers. File system provider registration is dynamic (from code) and not declarative.

Awaiting the activation for a particular scheme and then proceeding should be good enough in this case.

//cc @jrieken

[lramos15]

@bpasero I added the activation of the provider. Was this just being done deeper in the stack before?

@jrieken Do you know if we still need canSupportResource. It due to the fact that notebook diffs checked against the filesystem prior to create an input that we had this issue. I'm not sure if that's still something we need and what cases we would run into trying to open a file not supported by the file service. Wouldn't that then be an issue which plagues everyone?

[bpasero]

@lramos15 we only activate providers when you call activate (and it returns a Promise so you need to await) or when the file service is used. So yeah, it was happening as part of loading the file contents.

[IanMatthewHuff]

@lramos15 Hmm, I'm still seeing this: I'm in insiders version of github.dev.

[TylerLeonhardt]

reopening and adding label based on what @IanMatthewHuff said

[rebornix]

@lramos15 can we please have a look this iteration?

[lramos15]

The problem is still the file system providers.

@bpasero I do await the pr scheme activation but it still doesn't appear as a provider in the file service

[bpasero]

@lramos15 yup, I pushed a change to rename it. please use the method IFileService.canHandleResource method. It will make sure to activate the provider before checking.

[lramos15]

@bpasero But I'm doing both of those. I await the provider activation in the async code and then call hasProvider in the sync canHandleResource code. So I am doing both those steps

[bpasero]

Are we certain that is the right scheme? Can you debug how it comes in later eventually?

I am not an expert in how extension provided file system providers end up in the workbench, maybe @jrieken knows.

[lramos15]

@rebornix and I synced up to figure out what was going on here.

Notebooks expect a file system provider, but the GHPR extension only registers a text document content provider. This means a text file can resolve the contents but a notebook cannot. This was the original issue of https://github.com/microsoft/vscode/issues/122627 and why the canSupportResource option was added to prevent notebooks from being opened in PRs.

The two possible fixes are

• Notebooks support text content providers
• GHPR adds a fullblown PR filesystem provider.

If notebooks support text content providers the code will then use ITextModelService to check if it can handle the resource.

[rebornix]

The TextDocumentContentProvider provides readonly content for text models, does it make sense to fake an internal readonly-file-system-provider for it to support notebook @jrieken ?

[jrieken]

Well, a _TextDocument_ContentProvider is returning strings whereas the notebook serializer works with bytes. So, making a magic text document to notebook converter sounds like pushing it too far. Think of binary notebooks. How does GHPR handle other non-text files, like images or custom editors?

[lramos15]

@jrieken They just don't work (This is me trying to open the hex editor)

[jrieken]

A TextDocumentContentProvider is not sufficient when opening/comparing notebooks is desired (https://github.com/microsoft/vscode/issues/133892#issuecomment-951238545). This leaves the question if notebook-lands handle this properly. I assume it does because there is no failed attempt in opening a notebook editor but text-diff is shown. Also canSupportResource looks like it is doing the right thing.

Still, the observed experience isn't good (just like it isn't good for image editors and other non-text editors) and we should drag @alexr00 into this discussion so see if GHPR can replace its text document content provider with a fs implementation.

[rebornix]

When we started the GHPRI extension we don't seem to have stable fs api yet so we went with text document provider, now the fs API fits well. @alexr00 let me know if you this makes sense to you.

[alexr00]

@rebornix makes sense, thank you.

[rebornix]

@alexr00 just sent a PR https://github.com/microsoft/vscode-pull-request-github/pull/3091;)