Skip to content
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

Fix loading last selected deployment #2460

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Fix loading last selected deployment #2460

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions extensions/vscode/src/views/homeView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,8 +335,10 @@ export class HomeViewProvider implements WebviewViewProvider, Disposable {
return this.updateWebViewViewCredentials();
}

private async refreshActiveConfig() {
const cfg = await this.state.getSelectedConfiguration();
private async refreshActiveConfig(cfg?: Configuration | ConfigurationError) {
if (!cfg) {
cfg = await this.state.getSelectedConfiguration();
}

this.sendRefreshedFilesLists();
this.updateServerEnvironment();
Expand Down Expand Up @@ -381,8 +383,12 @@ export class HomeViewProvider implements WebviewViewProvider, Disposable {
);
}

private async refreshActiveContentRecord() {
const contentRecord = await this.state.getSelectedContentRecord();
private async refreshActiveContentRecord(
contentRecord?: ContentRecord | PreContentRecord,
) {
if (!contentRecord) {
contentRecord = await this.state.getSelectedContentRecord();
}
this.contentRecordWatchers?.dispose();

this.contentRecordWatchers = new ContentRecordWatcherManager(contentRecord);
Expand Down Expand Up @@ -1399,15 +1405,17 @@ export class HomeViewProvider implements WebviewViewProvider, Disposable {
return;
}

const selectedContentRecord = await this.state.getSelectedContentRecord();
const selectedConfig = await this.state.getSelectedConfiguration();
const selectionState = includeSavedState
? this.state.getSelection()
: undefined;
this.updateWebViewViewCredentials();
this.updateWebViewViewConfigurations();
this.updateWebViewViewContentRecords(selectionState || null);
if (includeSavedState && selectionState) {
this.refreshActiveContentRecord();
this.refreshActiveConfig();
this.refreshActiveContentRecord(selectedContentRecord);
this.refreshActiveConfig(selectedConfig);
Comment on lines +1417 to +1418
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I think I understand. These were not awaited here, and awaiting on the selection above caused this to work since refreshAll is used in initialization. So this looks like a timing issue. If I'm on the wrong track please let me know.

It may be beneficial to instead surround these in a await Promise.all and then won't need to pass the resources.

      await Promise.all([
        this.refreshActiveContentRecord(),
        this.refreshActiveConfig(),
      ]);

Alternatively we could require passing the resources and make those methods synchronous, but that is a larger change.

Copy link
Collaborator Author

@marcosnav marcosnav Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close but there's more to it, the main reason is having the content record in state when this.updateWebViewView... methods are called, it is more an "order of operations" problem.

So,

  • state.getSelectedContentRecord pulls record and stores in memory at state.contentRecords
  • then updateWebViewViewContentRecords makes use of the record in state
  • ...same goes for the configurations methods

It is true that refreshActiveContentRecord and refreshActiveConfig now also call getSelected... and another option was to move those calls before this.updateWebViewView..., but since those methods do some other operations I found it safer to have an optional param and receive the pre-fetched record.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure that I understand either. It really does feel like a bit of a timing issue in which we are simply being more explicit to make sure it doesn't happen, but... why did the timing issue not occur in the original code when we had an event bus?

Was this because the event bus has an implicit order of execution because messages were inserted into a FIFO queue, so it was not an issue?

I'm sure you're on the right track here, but I'd feel easier about this if I understood this impact, as I'm a little worried we might also have some other impacts from the change which may not have surfaced yet in our testing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation @marcosnav I totally understand the change now.

@sagerb let me see if I can explain in a different way. updateWebViewViewContentRecords just sends the message to the webview with the current state of state.contentRecords, but state.getSelectedContentRecord() actually updates state.contentRecords with the newest data from the API. It is similar for the configuration code.

The onInitializingMsg function calls refreshAll with forceAll = false to optimize initial load times; avoiding retrieving all content records and configurations when we only need the last ones used. So if we don't call state.getSelected... we actually don't call the grab the previous selection from the workspace state and call the API.

The removal of the bus actually removed these calls: d8eb18a#diff-f129ed54e45c65dc728784d713300ce0800add2ee009330128624da760d3ff4eL1480

So this wasn't a timing issue due to the removed bus, but an accidental removal of logic.

I'll mark as approved @marcosnav! 🙇

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you!

}
};

Expand Down
Loading