VSCODE-222: Edit documents from playground results #232

alenakhineika · 2021-01-05T15:45:49Z

Description

https://jira.mongodb.org/browse/VSCODE-222

When one or more documents returned by a playground are displayed in the read-only editor with the results, documents will have a code lens to access the editing functionality when:
- The playground result is a document returned by findOne().
- Or list of documents returned by find().
- Document's _is is presented.
By clicking on the code lens, the user is taken to a new editable version of the document, displayed in EJSON (the editable document is shown with all its fields)
When the document is saved (Cmd/Ctrl + S or with the menu), the document is saved to MongoDB.
When the document is saved, a notification informs the user that the change has been saved in MongoDB.
When the update results in an error, the user is notified.
It's possible to make edits to a document and save it more than once.
Update playground results behavior:
- To provide a better user experience and to be able to drag and drop playground results to any position in the window (including splitting window down), replace results window reopening with content updating in the current virtual document. This solution removes .txt and .js file extensions from uri.path. Now users have to specify the extension manually when saving document to disc.

https://jira.mongodb.org/browse/VSCODE-223

Show and write mongodb documents using a FileSystemProvider that helps us to avoid storing files on the user's file system, improve the breadcrumbs we show on an open document (before we showed the /tmp/.../file).

https://jira.mongodb.org/browse/VSCODE-185

Improve how open documents are handled when vscode is re-opened. If the user had an open document editor, we want to ensure that such a document is no longer opened when the user closes and reopens VS Code.

Checklist

New tests and/or benchmarks are included
Documentation is changed or added

Motivation and Context

Bugfix
New feature
Dependency update
Misc

Types of changes

Backport Needed
Patch (non-breaking change which fixes an issue)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to change)

Anemy

nice work - this'll be a nice functionality add for playgrounds.
Added a couple comments. I think one main one on the updating results content functionality.
I like separating out of the mongodb document operations from the editor controller. Something I noticed is the documentController is doing is the opening of the documents in the vscode editor - should that be something the editor controller does, and the documentController instead does just the mongodb document content providing side of things?

src/editors/documentController.ts

Anemy · 2021-01-06T05:07:53Z

src/editors/documentController.ts

+        vscode.window
+          .showTextDocument(doc, { preview: false, preserveFocus: true })
+          .then(() => resolve(true), reject);
+      }, reject);


How do you feel about making this use async/await?

try { const doc = await vscode.workspace.openTextDocument(uri); await vscode.window.showTextDocument(doc, { preview: false, preserveFocus: true }); } catch (err: Error) { throw err; } return true;

It was written like this initially but sometimes caused an uncaught promise rejection error that was coming from somewhere. I also would like to see it async/await, but let's come back to it later in a separate PR for better code readability and additional testing.

Anemy · 2021-01-06T05:10:45Z

src/editors/playgroundController.ts

+    namespace: string
+  ): Promise<boolean> {
+    return new Promise(async (resolve) => {
+      return this._documentController.openEditableDocument(


Doesn't look like the promise resolves, maybe we can just return the this._documentController.openEditableDocument result?

Refactored the part about opening and saving mdb docs. Liked a lot your suggestion about using the in-memory file system 👍

src/editors/playgroundResultProvider.ts

src/editors/documentController.ts

…CODE-223)

alenakhineika · 2021-01-06T20:17:51Z

@Anemy thank you very much for the review! ✨ Refactored branch according to your comment. PR is huge, sorry, but it would be much more work to implement and test intermediate solutions. As you can see not only lots of code was added but also removed! Please let me know if you want to review and discuss it together! Would be nice some time to do pair refactoring, I think it would be even cooler do discuss things and share knowledge before PR is ready!

alenakhineika · 2021-01-06T20:30:51Z

src/editors/playgroundResultProvider.ts


-      return resolve(this._playgroundController.playgroundResult?.content);


This part potentially could cause https://jira.mongodb.org/browse/VSCODE-204 error, because provideTextDocumentContent expects Promise result, and this._playgroundController.playgroundResult?.content could be undefined at some point. When manually tested different inputs and outputs of playground execution received once the same error that in the ticket. Refactored this function to check different result types and provided tests for them.

Anemy

Nice - here are the comments from our discussion, I think I didn't hit the post review button, just left the comments yesterday 😅

Anemy · 2021-01-07T13:19:03Z

src/editors/documentController.ts

+          limit: 1
+        }
+      );
+


Maybe we can move this._statusView.hideMessage() up here.

There were actually a couple of places where this._statusView.hideMessage() was missing. Updated to make sure we always hide this message.

Anemy · 2021-01-07T13:27:38Z

src/editors/documentController.ts

+          documentId
+        )}`;
+
+        throw new Error(errorMessage);


I think when we are using this function _fetchDocument, we'd expect it to return null or undefined when a document doesn't exist, and then the caller can decide what to do when the document doesn't exist. Should it be erroring here?

Anemy · 2021-01-07T13:28:47Z

src/editors/documentController.ts

+      this._statusView.hideMessage();
+      vscode.window.showErrorMessage(errorMessage);
+
+      return null;


In this case I think we might want to allow the caller to handle the error that occurs from fetching the document. In the case where the caller is just seeing if this document exists, I think we probably wouldn't always want to show a vscode error message popup.

Glad we had a call and discussed these returning types. Now it is much more clear for me when should be returned what. But pls take a look again at the refactored code to verify that it is what you meant.

Anemy · 2021-01-07T13:32:56Z

src/editors/documentController.ts

+    this._statusView.showMessage('Fetching document...');
+
+    try {
+      const documents = await find(


Maybe there's a findOne we can use in the dataservice

The findOne method is not provided by data service: https://github.com/mongodb-js/data-service/blob/master/lib/data-service.js

src/editors/documentController.ts

src/editors/playgroundController.ts

Anemy · 2021-01-07T13:53:20Z

src/editors/playgroundResultProvider.ts

+    }
+  }
+
+  async reopenResultAsVirtualDocument(


How does this function differ from refreshPlaygroundResult?

One was updating only a class propery, another was refreshing the window. Refactored to clean it up and reflect other changes in the playground controller.

Anemy · 2021-01-07T13:54:41Z

src/editors/playgroundResultProvider.ts

+      type: null,
+      content: undefined
+    };
+    this._uri = vscode.Uri.parse('');


If a new PlaygroundResultProvider is created for each result, can we pass the _uri in the constructor? I think it might be dangerous to create a blank uri if we assume the uri to always exist.

Now uri lives in playgroundResultProvider.ts itself.

src/editors/playgroundResultProvider.ts

src/editors/documentController.ts

Anemy

Looking good, one comment on handling when a document can't be fetched

Anemy

lgtm - I think we might want to associate playground results with the connection id which generated those results, so a user can't try to open up a document from playground results from a different connection. I think we could probably address that later. Left one comment regarding that, lgtm though nice work 🔥

Anemy · 2021-01-11T18:14:11Z

src/editors/mongoDBDocumentService.ts

+
+    if (dataservice === null) {
+      return this._fetchDocumentFailed(
+        `no longer connected to ${connectionName}`


I think connection name we show here will always be an empty string on a disconnected connection - is it possible to show the connection name of the connection which the playground result was run against?

I will address this in the next PR, thx for pointing at it!

alenakhineika added 3 commits January 4, 2021 21:46

feat: edit documents from playground results (VSCODE-222)

0af2e2e

test: add tests for document controller and edit code lenses provider

0cf56fe

refactor: check content type instead of the whole result object

334f57f

alenakhineika requested a review from Anemy January 5, 2021 16:42

refactor: use the same playground result file instead of reopen it

4dac2b0

Anemy reviewed Jan 6, 2021

View reviewed changes

alenakhineika added 2 commits January 6, 2021 18:04

feat: show and write mongodb documents using a FileSystemProvider (VS…

bc50eac

…CODE-223)

test: fix some long-running tests

f01ba23

alenakhineika commented Jan 6, 2021

View reviewed changes

alenakhineika added 6 commits January 7, 2021 13:42

refactor: do not refresh playground results after saving document

36d22f1

refactor: errors clean up

2df9656

refactor: reset memory file system provider

dd75540

refactor: remove repeated code

3fa5086

test: update names to be more specific

813c9e7

refactor: set language not extension for playground results

607f4b0

Anemy reviewed Jan 8, 2021

View reviewed changes

refactor: update code lenses position in code lenses provider

1b88399

alenakhineika requested a review from Anemy January 8, 2021 16:23

refactor: update comments in code

f9914fc

Anemy reviewed Jan 11, 2021

View reviewed changes

src/editors/documentController.ts Outdated Show resolved Hide resolved

Anemy reviewed Jan 11, 2021

View reviewed changes

alenakhineika added 3 commits January 11, 2021 14:49

refactor: use document controller only accesing database

fea2985

refactor: rename document controller to service

8c79d90

refactor: return lost function parameter

529db16

Anemy approved these changes Jan 11, 2021

View reviewed changes

alenakhineika merged commit 7eede49 into master Jan 11, 2021

alenakhineika deleted the VSCODE-222-edit-documents-from-playground-results branch January 11, 2021 20:31

Anemy mentioned this pull request Jan 25, 2021

0.4.0 #244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VSCODE-222: Edit documents from playground results #232

VSCODE-222: Edit documents from playground results #232

alenakhineika commented Jan 5, 2021 •

edited

Loading

Anemy left a comment

Anemy Jan 6, 2021

alenakhineika Jan 6, 2021

Anemy Jan 6, 2021

alenakhineika Jan 6, 2021

alenakhineika commented Jan 6, 2021

alenakhineika Jan 6, 2021

Anemy left a comment

Anemy Jan 7, 2021

alenakhineika Jan 8, 2021

Anemy Jan 7, 2021

Anemy Jan 7, 2021

alenakhineika Jan 8, 2021

Anemy Jan 7, 2021

alenakhineika Jan 8, 2021

Anemy Jan 7, 2021

alenakhineika Jan 8, 2021

Anemy Jan 7, 2021

alenakhineika Jan 8, 2021

Anemy left a comment

Anemy left a comment

Anemy Jan 11, 2021

alenakhineika Jan 11, 2021


		return resolve(this._playgroundController.playgroundResult?.content);

VSCODE-222: Edit documents from playground results #232

VSCODE-222: Edit documents from playground results #232

Conversation

alenakhineika commented Jan 5, 2021 • edited Loading

Description

Checklist

Motivation and Context

Types of changes

Anemy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenakhineika commented Jan 6, 2021

Choose a reason for hiding this comment

Anemy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anemy left a comment

Choose a reason for hiding this comment

Anemy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenakhineika commented Jan 5, 2021 •

edited

Loading