ui: Fix refresh and re-route behaviour #7846

rohityadavcloud · 2023-08-10T12:41:11Z

This introduces change in AutogenView to do the following:

Cancel old list API requests when the route is changed (between different list view or list and resource views)
In the handler of an API call made in the fetchData(), to check if the json received is for the current route/id (API)
Add console log for debugging which may be removed if this passes code review and testing

Fixes #7840

How was this tested?

In my env the listVirtualMachines API takes time to complete, so I go to the instances page and go quick back/forth other views; for example, go to instances page that doesn't load and click on 2nd VM (or go to instance page and search something that returns faster than the whole list API and go the VM). When the original list API call completes, the current VM is replaced by the first VM in the list.
Check console log with the fix and network to see that old API (xhr) requests that haven't completed are cancelled when the route/view is changed between list and resource views of different resources

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

rohityadavcloud · 2023-08-10T12:42:13Z

ui/src/views/AutogenView.vue

        this.items = json[responseName][objectName]
        if (!this.items || this.items.length === 0) {
          this.items = []
        }
+        this.itemCount = apiItemCount
+        if (this.itemCount !== this.items.length) {
+          console.log('WARN: API items length does not match the API return count, something is wrong')


this is one part that needs testing, I observed it a few times

rohityadavcloud · 2023-08-10T12:42:34Z

@blueorangutan ui
cc @borisstoyanov @vladimirpetrov @kiranchavala @NuxRo

blueorangutan · 2023-08-10T12:43:03Z

@rohityadavcloud a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2023-08-10T12:55:48Z

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/7846 (QA-JID-137)

codecov · 2023-08-10T18:06:07Z

Codecov Report

Merging #7846 (ee52f36) into 4.18 (3c38ed7) will not change coverage.
Report is 1 commits behind head on 4.18.
The diff coverage is n/a.

❗ Current head ee52f36 differs from pull request most recent head 3857af2. Consider uploading reports for the commit 3857af2 to get more accurate results

@@            Coverage Diff            @@
##               4.18    #7846   +/-   ##
=========================================
  Coverage     13.06%   13.06%           
  Complexity     9088     9088           
=========================================
  Files          2720     2720           
  Lines        257391   257391           
  Branches      40130    40130           
=========================================
  Hits          33621    33621           
  Misses       219548   219548           
  Partials       4222     4222

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DaanHoogland · 2023-08-11T07:07:40Z

@rohityadavcloud , I'll test but I have my doubt because of the following; If you reroute it will not help to cancel any outstanding APIs. These are still being executed and I am unsure of the value of the concept of "cancel". Can you explain that?
There is only a problem if the user will start an action (fire a new call) fr wich it can not know the changed parameters.

rohityadavcloud · 2023-08-11T07:51:07Z

@DaanHoogland in case of the UI, routing/re-routing, refer to the browser/history URL change; when we move from one list view or resource view to any other, the AutogenView is the same component that render such views; since it's the same component it doesn't unmount/recreated, and any previous APIs (list APIs generally) fired by the fetchData() may return and call the handler (see the api() in fetchData()) which can mess the new list/resource view and cause it to update its this.items or this.resource. This is generally rare, but can happen when the list API is slow (in my case I can reproduce, and share a video of the same if it helps - before and after this PR).

Due to this side effect, something like this can be produced https://www.loom.com/share/97225b22110944b689687409729f2938?sid=435ff224-bb2c-4d14-8ab1-867d91352cfe

ui/src/views/AutogenView.vue

rohityadavcloud · 2023-08-11T11:10:11Z

@blueorangutan ui

blueorangutan · 2023-08-11T11:11:04Z

@rohityadavcloud a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

rohityadavcloud · 2023-08-11T11:50:29Z

@blueorangutan ui

blueorangutan · 2023-08-11T11:51:03Z

@rohityadavcloud a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2023-08-11T12:04:17Z

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/7846 (QA-JID-140)

rohityadavcloud · 2023-08-11T12:40:03Z

cc @shwstppr @Pearl1594 I'll need some help to fix the failing UI unit tests - any tips or assistance you can offer?

rohityadavcloud · 2023-08-11T15:53:31Z

cc @nvazquez - if you can advise/assist on the failing unit tests, thanks.

DaanHoogland · 2023-08-15T13:42:40Z

@blueorangutan ui

Signed-off-by: Rohit Yadav <[email protected]>

rohityadavcloud · 2023-08-25T06:17:41Z

@blueorangutan package
@blueorangutan ui

blueorangutan · 2023-08-25T06:18:03Z

@rohityadavcloud a [SF] Jenkins job has been kicked to build packages. It will be bundled with@blueorangutan ui SystemVM template(s). I'll keep you posted as I make progress.

blueorangutan · 2023-08-25T06:18:04Z

@rohityadavcloud a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan · 2023-08-25T06:30:42Z

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/7846 (QA-JID-168)

DaanHoogland · 2023-08-25T06:45:30Z

@blueorangutan package

blueorangutan · 2023-08-25T06:46:03Z

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-08-25T07:05:50Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6888

blueorangutan · 2023-08-25T07:38:51Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6890

DaanHoogland · 2023-08-25T10:39:07Z

all seems fine but one strange thing I noticed is that an "Add VPC" notification remained "in progress"

while the VPC was allready added

DaanHoogland · 2023-08-25T10:45:45Z

I think this is no issue in this code. can we merge @weizhouapache @rohityadavcloud ?

rohityadavcloud · 2023-08-25T10:46:41Z

@DaanHoogland that could a notification regression or somehow things got opened in a new tab / refreshed, I've noticed that too (but not related to changes in autogenview)

rohityadavcloud · 2023-08-25T10:47:20Z

@DaanHoogland I've asked @andrijapanicsb to confirm testing, perhaps @GutoVeronezi was testing as well - appreciate all the QA, feedback we can get on this. Thanks.

weizhouapache · 2023-08-25T10:50:59Z

I think this is no issue in this code. can we merge @weizhouapache @rohityadavcloud ?

@DaanHoogland
thanks for testing
I am ok with merging .

andrijapanicsb · 2023-08-25T11:12:10Z

Wait for it... give me 1h

andrijapanicsb · 2023-08-25T11:30:29Z

Reproduction with high load on the mgmt server and 400+ VMs in DB
Short video: https://somup.com/c0jTXqAO10

Issues does NOT happen after upgrading to packages from this PR:
Short video: https://screenpal.com/watch/c0jTXuVpzoA

andrijapanicsb

LGTM

DaanHoogland · 2023-08-25T11:45:10Z

ok, merging. @GutoVeronezi if you find anything please open a new issue on that? thanks

rohityadavcloud added component:UI Severity:Critical Critical bug status:needs-testing labels Aug 10, 2023

rohityadavcloud added this to the 4.18.1.0 milestone Aug 10, 2023

rohityadavcloud requested review from DaanHoogland, andrijapanicsb and weizhouapache August 10, 2023 12:41

rohityadavcloud commented Aug 10, 2023

View reviewed changes

weizhouapache linked an issue Aug 10, 2023 that may be closed by this pull request

UI bug: internal "refresh" of the UI can cause a wrong VM to be deleted #7840

Closed

rohityadavcloud requested review from shwstppr and Pearl1594 August 11, 2023 07:51

shwstppr reviewed Aug 11, 2023

View reviewed changes

ui/src/views/AutogenView.vue Outdated Show resolved Hide resolved

rohityadavcloud force-pushed the refresh-bug-fix branch from f9c0b92 to a71a28f Compare August 11, 2023 11:54

rohityadavcloud requested a review from shwstppr August 11, 2023 12:38

rohityadavcloud requested a review from nvazquez August 11, 2023 15:53

DaanHoogland force-pushed the refresh-bug-fix branch from 685ccef to c9e52d6 Compare August 15, 2023 13:42

fix unit test

3b1b399

Signed-off-by: Rohit Yadav <[email protected]>

andrijapanicsb approved these changes Aug 25, 2023

View reviewed changes

DaanHoogland merged commit 6f7725a into apache:4.18 Aug 25, 2023

DaanHoogland deleted the refresh-bug-fix branch August 25, 2023 11:45

weizhouapache mentioned this pull request Aug 25, 2023

UI bug: internal "refresh" of the UI can cause a wrong VM to be deleted #7840

Closed

rohityadavcloud removed the status:needs-testing label Aug 26, 2023

nvazquez mentioned this pull request Sep 6, 2023

ui: don't skip if dataview has multiple items in response #7947

Merged

5 tasks

DaanHoogland unassigned shwstppr, kiranchavala, NuxRo, DaanHoogland, vladimirpetrov, borisstoyanov and weizhouapache Nov 16, 2023

ui: Fix refresh and re-route behaviour #7846

ui: Fix refresh and re-route behaviour #7846

Uh oh!

Conversation

rohityadavcloud commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

rohityadavcloud Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Aug 10, 2023

Uh oh!

blueorangutan commented Aug 10, 2023

Uh oh!

blueorangutan commented Aug 10, 2023

Uh oh!

codecov bot commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Aug 11, 2023

Uh oh!

rohityadavcloud commented Aug 11, 2023

Uh oh!

Uh oh!

rohityadavcloud commented Aug 11, 2023

Uh oh!

blueorangutan commented Aug 11, 2023

Uh oh!

rohityadavcloud commented Aug 11, 2023

Uh oh!

blueorangutan commented Aug 11, 2023

Uh oh!

blueorangutan commented Aug 11, 2023

Uh oh!

rohityadavcloud commented Aug 11, 2023

Uh oh!

rohityadavcloud commented Aug 11, 2023

Uh oh!

DaanHoogland commented Aug 15, 2023

Uh oh!

rohityadavcloud commented Aug 25, 2023

Uh oh!

blueorangutan commented Aug 25, 2023

Uh oh!

blueorangutan commented Aug 25, 2023

Uh oh!

blueorangutan commented Aug 25, 2023

Uh oh!

DaanHoogland commented Aug 25, 2023

Uh oh!

blueorangutan commented Aug 25, 2023

Uh oh!

blueorangutan commented Aug 25, 2023

Uh oh!

blueorangutan commented Aug 25, 2023

Uh oh!

DaanHoogland commented Aug 25, 2023

Uh oh!

DaanHoogland commented Aug 25, 2023

Uh oh!

rohityadavcloud commented Aug 25, 2023

Uh oh!

rohityadavcloud commented Aug 25, 2023

Uh oh!

weizhouapache commented Aug 25, 2023

Uh oh!

andrijapanicsb commented Aug 25, 2023

Uh oh!

andrijapanicsb commented Aug 25, 2023

Uh oh!

andrijapanicsb left a comment

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Aug 10, 2023 •

edited

Loading

codecov bot commented Aug 10, 2023 •

edited

Loading