Fix obsolete change views taking over again
We create one change view per change number. When the user navigates
from one change to another (for example in the "relation chain" panel),
then a new change view is created and the old change view becomes
obsolete.
The change view detects becoming obsolete when the router params change.
But it would still reload its data and while doing so potentially
trigger a navigation event, which could send the user back to the
obsolete change. This change adds a check to loadData() and thus
prevents the undesired navigation.
The change view was also checked that it does not emit navigation events
in other locations. So the same or a similar bug might be lurking
somewhere, but at least not in an obvious fashion. :-)
In the future when we are changing more towards working with reactive
observables, we will then be able to add `takeUntil(obsolete$)` to all
state updates and event streams. That will eventually take care of the
issue in a more appropriate and safer way.
Change-Id: I0a6f8fd5250c12f4a572ecadbd62293bb97391c9
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 4304734..2ede186e 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -1254,16 +1254,37 @@
this.$.fileList.collapseAllDiffs();
}
+ /**
+ * ChangeView is never re-used for different changes. It is safer and simpler
+ * to just re-create another change view when the user switches to a new
+ * change page. Thus we need a reliable way to detect that the change view
+ * does not match the current change number anymore.
+ *
+ * If this method returns true, then the change view should not do anything
+ * anymore. The app element makes sure that an obsolete change view is not
+ * shown anymore, so if the change view is still and doing some update to
+ * itself, then that is not dangerous. But for example it should not call
+ * navigateToChange() anymore. That would very likely cause erroneous
+ * behavior.
+ */
+ private isChangeObsolete() {
+ // While this._changeNum is undefined the change view is fresh and has just
+ // not updated it to params.changeNum yet. Not obsolete in that case.
+ if (this._changeNum === undefined) return false;
+ // this.params reflects the current state of the URL. If this._changeNum
+ // does not match it anymore, then this view must be considered obsolete.
+ return this._changeNum !== this.params?.changeNum;
+ }
+
_paramsChanged(value: AppElementChangeViewParams) {
if (value.view !== GerritView.CHANGE) {
this._initialLoadComplete = false;
return;
}
- // Everything in the change view is tied to the change. It seems better to
- // force the re-creation of the change view when the change number changes.
- const changeChanged = this._changeNum !== value.changeNum;
- if (this._changeNum !== undefined && changeChanged) {
+ if (this.isChangeObsolete()) {
+ // Tell the app element that we are not going to handle the new change
+ // number and that they have to create a new change view.
fireEvent(this, EventType.RECREATE_CHANGE_VIEW);
return;
}
@@ -1298,7 +1319,7 @@
// If the change has already been loaded and the parameter change is only
// in the patch range, then don't do a full reload.
- if (!changeChanged && patchChanged && patchKnown) {
+ if (this._changeNum !== undefined && patchChanged && patchKnown) {
if (!patchRange.patchNum) {
patchRange.patchNum = computeLatestPatchNum(this._allPatchSets);
rightPatchNumChanged = true;
@@ -2116,6 +2137,7 @@
* promise resolves.
*/
loadData(isLocationChange?: boolean, clearPatchset?: boolean) {
+ if (this.isChangeObsolete()) return Promise.resolve([]);
if (clearPatchset && this._change) {
GerritNav.navigateToChange(this._change);
return Promise.resolve([]);
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index 5128948..7d47abb 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -1614,7 +1614,7 @@
});
});
- test('don’t reload entire page when patchRange changes', () => {
+ test('don’t reload entire page when patchRange changes', async () => {
const reloadStub = sinon
.stub(element, 'loadData')
.callsFake(() => Promise.resolve([]));
@@ -1629,7 +1629,8 @@
view: GerritView.CHANGE,
patchNum: 1 as RevisionPatchSetNum,
};
- element._paramsChanged(value);
+ element.params = value;
+ await flush();
assert.isTrue(reloadStub.calledOnce);
element._initialLoadComplete = true;
@@ -1643,13 +1644,14 @@
value.basePatchNum = 1 as BasePatchSetNum;
value.patchNum = 2 as RevisionPatchSetNum;
- element._paramsChanged(value);
+ element.params = {...value};
+ await flush();
assert.isFalse(reloadStub.calledTwice);
assert.isTrue(reloadPatchDependentStub.calledOnce);
assert.isTrue(collapseStub.calledTwice);
});
- test('reload ported comments when patchNum changes', () => {
+ test('reload ported comments when patchNum changes', async () => {
sinon.stub(element, 'loadData').callsFake(() => Promise.resolve([]));
sinon.stub(element, '_getCommitInfo');
sinon.stub(element.$.fileList, 'reload');
@@ -1665,7 +1667,8 @@
view: GerritView.CHANGE,
patchNum: 1 as RevisionPatchSetNum,
};
- element._paramsChanged(value);
+ element.params = value;
+ await flush();
element._initialLoadComplete = true;
element._change = {
@@ -1678,24 +1681,42 @@
value.basePatchNum = 1 as BasePatchSetNum;
value.patchNum = 2 as RevisionPatchSetNum;
- element._paramsChanged(value);
+ element.params = {...value};
+ await flush();
assert.isTrue(reloadPortedCommentsStub.calledOnce);
});
- test('reload entire page when patchRange doesnt change', () => {
+ test('reload entire page when patchRange doesnt change', async () => {
const reloadStub = sinon
.stub(element, 'loadData')
.callsFake(() => Promise.resolve([]));
const collapseStub = sinon.stub(element.$.fileList, 'collapseAllDiffs');
const value: AppElementChangeViewParams = createAppElementChangeViewParams();
- element._paramsChanged(value);
+ element.params = value;
+ await flush();
assert.isTrue(reloadStub.calledOnce);
element._initialLoadComplete = true;
- element._paramsChanged(value);
+ element.params = {...value};
+ await flush();
assert.isTrue(reloadStub.calledTwice);
assert.isTrue(collapseStub.calledTwice);
});
+ test('do not handle new change numbers', async () => {
+ const recreateSpy = sinon.spy();
+ element.addEventListener('recreate-change-view', recreateSpy);
+
+ const value: AppElementChangeViewParams = createAppElementChangeViewParams();
+ element.params = value;
+ await flush();
+ assert.isFalse(recreateSpy.calledOnce);
+
+ value.changeNum = 555111333 as NumericChangeId;
+ element.params = {...value};
+ await flush();
+ assert.isTrue(recreateSpy.calledOnce);
+ });
+
test('related changes are not updated after other action', done => {
sinon.stub(element, 'loadData').callsFake(() => Promise.resolve([]));
flush();
@@ -2026,7 +2047,7 @@
element.handleScroll();
});
- test('scrollTop is set correctly', () => {
+ test('scrollTop is set correctly', async () => {
element.viewState = {scrollTop: TEST_SCROLL_TOP_PX};
sinon.stub(element, 'loadData').callsFake(() => {
@@ -2039,7 +2060,8 @@
// simulate reloading component, which is done when route
// changes to match a regex of change view type.
- element._paramsChanged({...createAppElementChangeViewParams()});
+ element.params = {...createAppElementChangeViewParams()};
+ await flush();
});
test('scrollTop is reset when new change is loaded', () => {
@@ -2591,7 +2613,7 @@
});
});
- test('report changeDisplayed on _paramsChanged', done => {
+ test('report changeDisplayed on _paramsChanged', async () => {
const changeDisplayStub = sinon.stub(
appContext.reportingService,
'changeDisplayed'
@@ -2600,16 +2622,14 @@
appContext.reportingService,
'changeFullyLoaded'
);
- element._paramsChanged({
+ element.params = {
...createAppElementChangeViewParams(),
changeNum: TEST_NUMERIC_CHANGE_ID,
project: TEST_PROJECT_NAME,
- });
- flush(() => {
- assert.isTrue(changeDisplayStub.called);
- assert.isTrue(changeFullyLoadedStub.called);
- done();
- });
+ };
+ await flush();
+ assert.isTrue(changeDisplayStub.called);
+ assert.isTrue(changeFullyLoadedStub.called);
});
});
