Improve change-model load change consistency

1. Only update the change if change is undefined, or if the change
number is the same:
There must be a consistency between ChangeViewModel and
ChangeModel. Only change to a ChangeViewModel is allowed to change the
changeNum, while all other calls to updateStateChange can only
update the info of the current change to a more recent one. The update
through rxjs can load a different change since updateLoadingState sets
the change to undefined.

2. Simplify LoadingState and remove RELOADING:
RELOADING state previously could only happen as the result of a
following race condition ChangeViewModel forceLoad sets changNum to
undefined and then async schedules setting it back to the original
state. The RELOADING is only set if the processing of restApi results
(no actual calls are performed since changeNum is undefined) is
scheduled later in the Event Queue than the restoration of the
original state. Moreover the actual difference between LOADING and
RELOADING never matters in code.

We simplify the logic by removing RELOADING state. Passing a undefined
changeNum, will immediately set the change to undefined and LoadingState
to NOT_LOADED. Previously it would be "change is undefined", loading
state "LOADED", until it changes to "NOT_LOADED" in the next Event Queue
cycle (or "RELOADING" if the race is won by the forceLoad)

3. Move updateRevisionsWithCommitShas into updateStateChange:
This makes the state more consistent. Now the change in ChangeModel
always has the values set, regardless whether the value came from a
manual call or subscription.

4. Add catchError, to reset LoadingState if the RestApi calls throw an
error

Google-Bug-Id: b/328632912
Release-Notes: skip
Change-Id: I0ed368df7152150ed974c0b337894758ee531344
(cherry picked from commit 22f4e92288ac07a679765c62000068d2fa8a7aed)

5 files changed, 64 insertions, 45 deletions

diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 6e48e575a1..a0ad2f0873 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -99,10 +99,7 @@ import {
 } from '../../../utils/attention-set-util';
 import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
 import {resolve} from '../../../models/dependency';
-import {
-  changeModelToken,
-  updateRevisionsWithCommitShas,
-} from '../../../models/change/change-model';
+import {changeModelToken} from '../../../models/change/change-model';
 import {LabelNameToValuesMap, PatchSetNumber} from '../../../api/rest-api';
 import {css, html, PropertyValues, LitElement, nothing} from 'lit';
 import {sharedStyles} from '../../../styles/shared-styles';
@@ -1454,10 +1451,8 @@ export class GrReplyDialog extends LitElement {
     return this.saveReview(reviewInput, errFn)
       .then(result => {
         this.getChangeModel().updateStateChange(
-          updateRevisionsWithCommitShas(
-            GrReviewerUpdatesParser.parse(
-              result?.change_info as ChangeViewChangeInfo
-            )
+          GrReviewerUpdatesParser.parse(
+            result?.change_info as ChangeViewChangeInfo
           )
         );
 
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 24141079a1..bba7e4729a 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -19,7 +19,13 @@ import {
 } from '../../types/common';
 import {ChangeStatus, DefaultBase} from '../../constants/constants';
 import {combineLatest, from, Observable, forkJoin, of} from 'rxjs';
-import {map, filter, withLatestFrom, switchMap} from 'rxjs/operators';
+import {
+  map,
+  filter,
+  withLatestFrom,
+  switchMap,
+  catchError,
+} from 'rxjs/operators';
 import {
   computeAllPatchSets,
   computeLatestPatchNum,
@@ -55,7 +61,7 @@ const ERR_REVIEW_STATUS = 'Couldn’t change file review status.';
 export interface ChangeState {
   /**
    * If `change` is undefined, this must be either NOT_LOADED or LOADING.
-   * If `change` is defined, this must be either LOADED or RELOADING.
+   * If `change` is defined, this must be either LOADED.
    */
   loadingStatus: LoadingStatus;
   change?: ParsedChangeInfo;
@@ -177,7 +183,13 @@ const initialState: ChangeState = {
 };
 
 export const changeModelToken = define<ChangeModel>('change-model');
-
+/**
+ * Change model maintains information about the current change.
+ *
+ * The "current" change is defined by ChangeViewModel. This model tracks part of
+ * the current view. As such it's a singleton global state. It's NOT meant to
+ * keep the state of an arbitrary change.
+ */
 export class ChangeModel extends Model<ChangeState> {
   private change?: ParsedChangeInfo;
 
@@ -199,8 +211,7 @@ export class ChangeModel extends Model<ChangeState> {
 
   public readonly loading$ = select(
     this.changeLoadingStatus$,
-    status =>
-      status === LoadingStatus.LOADING || status === LoadingStatus.RELOADING
+    status => status === LoadingStatus.LOADING
   );
 
   public readonly reviewedFiles$ = select(
@@ -388,10 +399,7 @@ export class ChangeModel extends Model<ChangeState> {
 
   private reportChangeReload() {
     return this.changeLoadingStatus$.subscribe(loadingStatus => {
-      if (
-        loadingStatus === LoadingStatus.LOADING ||
-        loadingStatus === LoadingStatus.RELOADING
-      ) {
+      if (loadingStatus === LoadingStatus.LOADING) {
         this.reporting.time(Timing.CHANGE_RELOAD);
       }
       if (
@@ -557,16 +565,21 @@ export class ChangeModel extends Model<ChangeState> {
     return this.viewModel.changeNum$
       .pipe(
         switchMap(changeNum => {
-          if (changeNum !== undefined) this.updateStateLoading(changeNum);
-          const change = from(this.restApiService.getChangeDetail(changeNum));
-          const edit = from(this.restApiService.getChangeEdit(changeNum));
+          this.updateStateLoading(changeNum);
+          // if changeNum is undefined restApi calls return undefined.
+          const change = this.restApiService.getChangeDetail(changeNum);
+          const edit = this.restApiService.getChangeEdit(changeNum);
           return forkJoin([change, edit]);
         }),
         withLatestFrom(this.viewModel.patchNum$),
         map(([[change, edit], patchNum]) =>
           updateChangeWithEdit(change, edit, patchNum)
         ),
-        map(updateRevisionsWithCommitShas)
+        catchError(err => {
+          // Reset loading state and re-throw.
+          this.updateState({loadingStatus: LoadingStatus.NOT_LOADED});
+          throw err;
+        })
       )
       .subscribe(change => {
         // The change service is currently a singleton, so we have to be
@@ -752,25 +765,33 @@ export class ChangeModel extends Model<ChangeState> {
   /**
    * Called when change detail loading is initiated.
    *
-   * If the change number matches the current change in the state, then
-   * this is a reload. If not, then we not just want to set the state to
-   * LOADING instead of RELOADING, but we also want to set the change to
+   * We want to set the state to LOADING, but we also want to set the change to
    * undefined right away. Otherwise components could see inconsistent state:
    * a new change number, but an old change.
    */
-  private updateStateLoading(changeNum: NumericChangeId) {
-    const current = this.getState();
-    const reloading = current.change?._number === changeNum;
+  private updateStateLoading(changeNum?: NumericChangeId) {
     this.updateState({
-      change: reloading ? current.change : undefined,
-      loadingStatus: reloading
-        ? LoadingStatus.RELOADING
-        : LoadingStatus.LOADING,
+      change: undefined,
+      loadingStatus: changeNum
+        ? LoadingStatus.LOADING
+        : LoadingStatus.NOT_LOADED,
     });
   }
 
   // Private but used in tests.
+  /**
+   * Update the change information in the state.
+   *
+   * Since the ChangeModel must maintain consistency with ChangeViewModel
+   * The update is only allowed, if the new change has the same number as the
+   * current change or if the current change is not set (it was reset to
+   * undefined when ChangeViewModel.changeNum updated).
+   */
   updateStateChange(change?: ParsedChangeInfo) {
+    if (this.change && change?._number !== this.change?._number) {
+      return;
+    }
+    change = updateRevisionsWithCommitShas(change);
     this.updateState({
       change,
       loadingStatus:
diff --git a/polygerrit-ui/app/models/change/change-model_test.ts b/polygerrit-ui/app/models/change/change-model_test.ts
index f074ac34bc..ebe60664a9 100644
--- a/polygerrit-ui/app/models/change/change-model_test.ts
+++ b/polygerrit-ui/app/models/change/change-model_test.ts
@@ -306,11 +306,11 @@ suite('change model tests', () => {
 
     // Reloading same change
     document.dispatchEvent(new CustomEvent('reload'));
-    state = await waitForLoadingStatus(LoadingStatus.RELOADING);
+    state = await waitForLoadingStatus(LoadingStatus.LOADING);
     assert.equal(stub.callCount, 3);
     assert.equal(stub.getCall(1).firstArg, undefined);
     assert.equal(stub.getCall(2).firstArg, TEST_NUMERIC_CHANGE_ID);
-    assert.deepEqual(state?.change, updateRevisionsWithCommitShas(knownChange));
+    assert.deepEqual(state?.change, undefined);
 
     promise.resolve(knownChange);
     state = await waitForLoadingStatus(LoadingStatus.LOADED);
diff --git a/polygerrit-ui/app/models/checks/checks-model_test.ts b/polygerrit-ui/app/models/checks/checks-model_test.ts
index 767e604717..a8eda0ff30 100644
--- a/polygerrit-ui/app/models/checks/checks-model_test.ts
+++ b/polygerrit-ui/app/models/checks/checks-model_test.ts
@@ -30,12 +30,16 @@ import {
 } from '../../test/test-data-generators';
 import {waitUntil, waitUntilCalled} from '../../test/test-utils';
 import {ParsedChangeInfo} from '../../types/types';
-import {changeModelToken} from '../change/change-model';
+import {
+  changeModelToken,
+  updateRevisionsWithCommitShas,
+} from '../change/change-model';
 import {assert} from '@open-wc/testing';
 import {testResolver} from '../../test/common-test-setup';
 import {changeViewModelToken} from '../views/change';
 import {NumericChangeId, PatchSetNumber} from '../../api/rest-api';
 import {pluginLoaderToken} from '../../elements/shared/gr-js-api-interface/gr-plugin-loader';
+import {deepEqual} from '../../utils/deep-util';
 
 const PLUGIN_NAME = 'test-plugin';
 
@@ -106,17 +110,17 @@ suite('checks-model tests', () => {
     });
     await waitUntil(() => change === undefined);
 
-    const testChange = createParsedChange();
+    const testChange = updateRevisionsWithCommitShas(createParsedChange());
     testResolver(changeModelToken).updateStateChange(testChange);
-    await waitUntil(() => change === testChange);
+    await waitUntil(() => deepEqual(change, testChange));
     await waitUntilCalled(fetchSpy, 'fetch');
 
     assert.equal(
       model.latestPatchNum,
-      testChange.revisions[testChange.current_revision]
+      testChange!.revisions[testChange!.current_revision]
         ._number as PatchSetNumber
     );
-    assert.equal(model.changeNum, testChange._number);
+    assert.equal(model.changeNum, testChange!._number);
   });
 
   test('fetch throttle', async () => {
@@ -133,9 +137,9 @@ suite('checks-model tests', () => {
     });
     await waitUntil(() => change === undefined);
 
-    const testChange = createParsedChange();
+    const testChange = updateRevisionsWithCommitShas(createParsedChange());
     testResolver(changeModelToken).updateStateChange(testChange);
-    await waitUntil(() => change === testChange);
+    await waitUntil(() => deepEqual(change, testChange));
 
     model.reload('test-plugin');
     model.reload('test-plugin');
@@ -339,9 +343,9 @@ suite('checks-model tests', () => {
     });
     await waitUntil(() => change === undefined);
     clock.tick(1);
-    const testChange = createParsedChange();
+    const testChange = updateRevisionsWithCommitShas(createParsedChange());
     testResolver(changeModelToken).updateStateChange(testChange);
-    await waitUntil(() => change === testChange);
+    await waitUntil(() => deepEqual(change, testChange));
     clock.tick(600); // need to wait for 500ms throttle
     await waitUntilCalled(fetchSpy, 'fetch');
     const pollCount = fetchSpy.callCount;
@@ -366,9 +370,9 @@ suite('checks-model tests', () => {
     });
     await waitUntil(() => change === undefined);
     clock.tick(1);
-    const testChange = createParsedChange();
+    const testChange = updateRevisionsWithCommitShas(createParsedChange());
     testResolver(changeModelToken).updateStateChange(testChange);
-    await waitUntil(() => change === testChange);
+    await waitUntil(() => deepEqual(change, testChange));
     clock.tick(600); // need to wait for 500ms throttle
     await waitUntilCalled(fetchSpy, 'fetch');
     clock.tick(1);
diff --git a/polygerrit-ui/app/types/types.ts b/polygerrit-ui/app/types/types.ts
index 43e06a4081..f48588b35c 100644
--- a/polygerrit-ui/app/types/types.ts
+++ b/polygerrit-ui/app/types/types.ts
@@ -41,7 +41,6 @@ export enum ErrorType {
 export enum LoadingStatus {
   NOT_LOADED = 'NOT_LOADED',
   LOADING = 'LOADING',
-  RELOADING = 'RELOADING',
   LOADED = 'LOADED',
 }