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
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 1a18e9c..0bf49db 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
@@ -98,10 +98,7 @@ } 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'; @@ -1462,10 +1459,8 @@ 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 2414107..bba7e47 100644 --- a/polygerrit-ui/app/models/change/change-model.ts +++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -19,7 +19,13 @@ } 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 @@ 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 @@ }; 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 @@ 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 @@ 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 @@ 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 @@ /** * 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 f074ac3..ebe6066 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 @@ // 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 767e604..a8eda0f 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 @@ } 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 @@ }); 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 @@ }); 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 @@ }); 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 @@ }); 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 43e06a4..f48588b 100644 --- a/polygerrit-ui/app/types/types.ts +++ b/polygerrit-ui/app/types/types.ts
@@ -41,7 +41,6 @@ export enum LoadingStatus { NOT_LOADED = 'NOT_LOADED', LOADING = 'LOADING', - RELOADING = 'RELOADING', LOADED = 'LOADED', }