New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rfc] Prevent 'Error starting aiconfig server' notification when closing w/ unsaved changes #1201

Merged

rholinshead merged 1 commit into main from pr1201

Feb 9, 2024

Contributor

rholinshead commented Feb 8, 2024 •

edited

Loading

[rfc] Prevent 'Error starting aiconfig server' notification when closing w/ unsaved changes

[rfc] Prevent 'Error starting aiconfig server' notification when closing w/ unsaved changes

Currently, whenever an aiconfig editor webview is closed with unsaved changes, a notifcation shows at the bottom right:

This is caused by the following sequence of events:

changeDocumentSubscription happens with the "unsaved" changes being undone; this results in updating webview & server
server /load request is sent as a result
Another changeDocumentSubscription happens with no changed content (can probably just skip -- in all cases?)
Webview disposed, kills editor process
/load Request (STILL IN FLIGHT) fails, causing the notification

I spent some time but wasn't able to figure out why changeDocumentSubscription was happening (note -- NOT happening from the notifying dirty). Seems like this could potentially be a KP - microsoft/vscode#81902 but not concrete.

For now, RFC on this, we could just handle the server request error specially in this case when the webview is already disposed (and ignore it).

Screen.Recording.2024-02-08.at.6.26.28.PM.mov

Make sure the error still shows for legit cases (e.g. opening config without correct parsers installed):

Screen.Recording.2024-02-08.at.6.41.30.PM.mov

rholinshead force-pushed the pr1201 branch from 5840254 to 1cc05bb Compare

February 8, 2024 23:43

rholinshead marked this pull request as ready for review

February 8, 2024 23:43

rholinshead requested review from saqadri, suyoglastmileai, Ankush-lastmile, jonathanlastmileai and rossdanlm as code owners

February 8, 2024 23:43

rossdanlm reviewed

View reviewed changes

vscode-extension/src/aiConfigEditor.ts

+ vscode.window
+ .showErrorMessage(
+ "Failed to start aiconfig server. You can view the aiconfig but cannot modify it.",
+ ...["Details", "Retry"]

Contributor

rossdanlm Feb 9, 2024

What does the "Details" here add? Looks the same in test plans

Contributor Author

rholinshead Feb 9, 2024

It adds the 'Details' button in the notification, clicking it opens the Output tab with the exception message

rossdanlm reviewed

View reviewed changes

vscode-extension/src/aiConfigEditor.ts

  if (editorServer) {
  console.log("Killing editor server process");
  editorServer.proc.kill();
+ editorServer = null;

Contributor

rossdanlm Feb 9, 2024

Why do we need to explicitly set this to null? Is this related to the logic in the rest of this PR or just finishing the TODO from Sarmad on L206?

Contributor Author

rholinshead Feb 9, 2024

Honestly, I'm not sure if it needs to be (seems same behaviour with or without) but the fact that the existing code has webViewDisposed check and editorServer check suggests we should be setting it to null when its process is killed. More for completeness than anything

rossdanlm reviewed

View reviewed changes

vscode-extension/src/aiConfigEditor.ts

  // Make sure we get rid of the listener when our editor is closed.
  webviewPanel.onDidDispose(() => {
+ isWebviewDisposed = true;

Contributor

rossdanlm Feb 9, 2024

Ok so just to clarify, this needs to be moved up top here becasue it's a flag that will get read on L171? And L171 flow occurs when we call changeDocumentSubscription on L240 below?

Contributor Author

rholinshead Feb 9, 2024

When testing it didn't actually seem to matter if this was at the top or bottom but it makes more sense to set it immediately, considering there are async events that are checking it

rossdanlm reviewed

View reviewed changes

vscode-extension/src/aiConfigEditor.ts Outdated

+ console.log("changeDocumentSubscription -- updating server");
+ await updateServerState(editorServer.url, e.document);
+ } catch (e) {
+ if (isWebviewDisposed || !editorServer) {

Contributor

rossdanlm Feb 9, 2024 •

edited

Loading

What are the use cases where this is true because of the 2nd condition instead of the 1st (ie: isWebviewDisposed is false, but editorServer is null)? Is it when we first try to open the file in AIConfig editor before it's had time to initialize the server connection? If that's the case, should we update the console.info line on 176 to include that use case as well (we can also have two separate branches for two different messages)

Contributor Author

rholinshead Feb 9, 2024

Hmm, that's a good point. In theory I guess that scenario may be able to happen. I had just added this check for completeness but maybe we do actually want to surface the error if this scenario does happen. I'll remove the !editorServer check

rossdanlm reviewed

View reviewed changes

vscode-extension/src/aiConfigEditor.ts

+ // TODO: Should we skip events with no content changes?
+ // if (e.contentChanges.length === 0) {
+ // console.log(
+ // `changeDocumentSubscription ${e.document.uri} - skipping event because there are no content changes`

Contributor

rossdanlm Feb 9, 2024

In general, would this case even be possible? Why would changeDocumentSubscription change if the content hasn't been changed: https://lastmileai.dev/workbooks/clsf1z185004bqpof6z9lbpvn

Contributor Author

rholinshead Feb 9, 2024

Yep, it's possible and 100% happens every time you close the editor with unsaved changes. It also seems to happen pretty regularly when just making normal changes in the editor

Contributor

rossdanlm Feb 10, 2024

Ok then yea, think it would be good just to ignore if there are no contentChanges detected, as long as the "Are you sure you want to remove unsaved changes" dialog still pops up

rossdanlm reviewed

View reviewed changes

vscode-extension/src/aiConfigEditor.ts

Comment on lines +173 to +174

		// request is in flight (e.g. closing config w/ unsaved changes will
		// emit onDidChangeTextDocument with reverted unsaved changes)

Contributor

rossdanlm Feb 9, 2024

Nit: add something like "please refer to #1201 for full details" becuase the explanation you have in summary is really helpful

rossdanlm approved these changes

View reviewed changes

Contributor

rossdanlm left a comment

I think this is super well investigated, thanks for the thorough explanation in summary


[rfc] Prevent 'Error starting aiconfig server' notification when clos…

d180e17

…ing w/ unsaved changes

# [rfc] Prevent 'Error starting aiconfig server' notification when closing w/ unsaved changes

Currently, whenever an aiconfig editor webview is closed with unsaved changes, a notifcation shows at the bottom right:
![Screenshot 2024-02-08 at 6 29 06 PM](https://github.com/lastmile-ai/aiconfig/assets/5060851/9e45fced-a60e-42e9-8a2f-e8f65cfcd27f)

This is caused by the following sequence of events:
1. changeDocumentSubscription happens with the "unsaved" changes being undone; this results in updating webview & server
2. server /load request is sent as a result
3. Another changeDocumentSubscription happens with no changed content (can probably just skip -- in all cases?)
4. Webview disposed, kills editor process
5. /load Request (STILL IN FLIGHT) fails, causing the notification

I spent some time but wasn't able to figure out why changeDocumentSubscription was happening (note -- NOT happening from the notifying dirty). Seems like this could potentially be a KP - microsoft/vscode#81902 but not concrete.

For now, RFC on this, we could just handle the server request error specially in this case when the webview is already disposed (and ignore it).

https://github.com/lastmile-ai/aiconfig/assets/5060851/4e986654-31d9-4192-bba7-d07fe0756448

Make sure the error still shows for legit cases (e.g. opening config without correct parsers installed):

https://github.com/lastmile-ai/aiconfig/assets/5060851/912b8123-137f-4e8e-ab64-b05438adb44f

rholinshead force-pushed the pr1201 branch from 1cc05bb to d180e17 Compare

February 9, 2024 20:22

Contributor Author

rholinshead commented Feb 9, 2024

Changes from review:

add comment to refer to this PR for full details
remove check for null server when webview not disposed so that notification shows in that case

rholinshead merged commit a741af3 into main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet