Fix loading last selected deployment

[marcosnav]

Intent

This PR fixes an issue where the last selected deployment does not populate properly on the extension UI when initializing.

The cause of this issue was introduced at commit d8eb18a, such commit introduced two main changes:

1. Removal of an event bus
2. Moving logic from the homeView to PublisherState

Being the second change the cause of this problem, ignoring that a couple state method calls were still required to build up cached data that was used to display the last selected deployment.

Fixes #2459

Type of Change

• • Bug Fix
• • New Feature
• • Breaking Change
• • Documentation
• • Refactor
• • Tooling

Approach

Went through the changes that caused this issue, looking for the area where the data was expected but missing.

User Impact

Users should be able to see the last selected deployment when the publisher extension is opened and initializes.

Automated Tests

We should have tests, we don't have any in this area of the product yet but I'll keep promoting efforts to make that happen. Specially since this issue would have been caught earlier with a test in place.

Directions for Reviewers

• Create a deployment
• close (or reload) VS Code
• navigating to the Publisher extension home should show up the latest deployment data within the deployments dropdown

Checklist

• I have updated CHANGELOG.md to cover notable changes.

[jonkeane]

We should have tests, we don't have any in this area of the product yet but I'll keep promoting efforts to make that happen. Specially since this issue would have been caught earlier with a test in place.

Would it be feasible to add (some of) those tests in this PR?

[marcosnav]

Would it be feasible to add (some of) those tests in this PR?

I took a look and tried to narrow down testing requirements to have something within this PR, but it still needs some amount of test tooling to be built in order to create tests for the HomeViewProvider.

Absolutely worth doing and I'll pursue having those tests sooner rather than later, but IMO that shouldn't hold back the fix in this PR to land.

[dotNomad]

This change works, but I'm not sure why it works - could you explain a bit? My assumption would be that refreshActiveContentRecord and refreshActiveConfig would function the same way since they called getSelected... themselves. Why does adding the fetch to refreshAll and passing them to the functions change the behavior?

[dotNomad]

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.

[marcosnav]

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.

[sagerb]

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.

[dotNomad]

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! 🙇

[sagerb]

Ah, thank you!

[dotNomad]

Thanks for the explanation @marcosnav

Just goes to show how easy it was to accidentally remove logic when we had trouble understanding why it needed to be re-added.

[jonkeane]

Just goes to show how easy it was to accidentally remove logic when we had trouble understanding why it needed to be re-added.

💯 I'm sure @marcosnav would say the same thing, but: it sounds like we should add a test that covers this if we've hit it once?

[dotNomad]

it sounds like we should add a test that covers this if we've hit it once?

This would be a great first test using the integration test methodology we discussed (https://code.visualstudio.com/api/working-with-extensions/testing-extension).

I added a draft issue to our triage board to cover this since building out our integration tests is a bit large to be done in this PR.