Cancel all debouncers when the element is detached
The long living (60 seconds) debouncer from the error manager was most
likely causing major headaches by making the CI test suite extremely
flaky. Canceling on detach will hopefully make frontend developers much
happier again. :-)
All the other debouncers are not likely not cause major problems,
beacuse they are typically short lived, but it should be common best
practice to always cancel all debouncers on detach. So this change
fixes the entire code base.
There is a call to `flushDebouncers()` is the common-test-setup.ts,
which was thought to be a safeguard against the problem, but in fact
it is not. Added a commment explaining that.
Change-Id: I18f7a8e0304d3f75cf4317ea567b9f76b585398b
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 fe68cdc..197cef6 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
@@ -239,6 +239,10 @@
export type ChangeViewPatchRange = Partial<PatchRange>;
+const DEBOUNCER_REPLY_OVERLAY_REFIT = 'reply-overlay-refit';
+
+const DEBOUNCER_SCROLL = 'scroll';
+
@customElement('gr-change-view')
export class GrChangeView extends KeyboardShortcutMixin(
GestureEventListeners(LegacyElementMixin(PolymerElement))
@@ -710,6 +714,8 @@
super.detached();
this.unlisten(window, 'scroll', '_handleScroll');
this.unlisten(document, 'visibilitychange', '_handleVisibilityChange');
+ this.cancelDebouncer(DEBOUNCER_REPLY_OVERLAY_REFIT);
+ this.cancelDebouncer(DEBOUNCER_SCROLL);
if (this._updateCheckTimerHandle) {
this._cancelUpdateCheckTimer();
@@ -1230,7 +1236,7 @@
_handleReplyAutogrow() {
// If the textarea resizes, we need to re-fit the overlay.
this.debounce(
- 'reply-overlay-refit',
+ DEBOUNCER_REPLY_OVERLAY_REFIT,
() => {
this.$.replyOverlay.refit();
},
@@ -1248,7 +1254,7 @@
_handleScroll() {
this.debounce(
- 'scroll',
+ DEBOUNCER_SCROLL,
() => {
this.viewState.scrollTop = document.body.scrollTop;
},
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index d41a662..9f3f5f0 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -153,6 +153,8 @@
export type FileNameToReviewedFileInfoMap = {[name: string]: ReviewedFileInfo};
+const DEBOUNCER_LOADING_CHANGE = 'loading-change';
+
/**
* Type for FileInfo
*
@@ -407,6 +409,7 @@
detached() {
super.detached();
this._cancelDiffs();
+ this.cancelDebouncer(DEBOUNCER_LOADING_CHANGE);
}
/**
@@ -1598,7 +1601,7 @@
*/
_loadingChanged(loading?: boolean) {
this.debounce(
- 'loading-change',
+ DEBOUNCER_LOADING_CHANGE,
() => {
// Only show set the loading if there have been files loaded to show. In
// this way, the gray loading style is not shown on initial loads.
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 ca3161f..e5396c3 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
@@ -168,6 +168,8 @@
};
}
+const DEBOUNCER_STORE = 'store';
+
@customElement('gr-reply-dialog')
export class GrReplyDialog extends KeyboardShortcutMixin(
GestureEventListeners(LegacyElementMixin(PolymerElement))
@@ -425,6 +427,11 @@
this.jsAPI.addElement(TargetElement.REPLY_DIALOG, this);
}
+ /** @override */
+ detached() {
+ this.cancelDebouncer(DEBOUNCER_STORE);
+ }
+
open(focusTarget?: FocusTarget) {
if (!this.change) throw new Error('missing required change property');
this.knownLatestState = LatestPatchState.CHECKING;
@@ -1332,7 +1339,7 @@
_draftChanged(newDraft: string, oldDraft?: string) {
this.debounce(
- 'store',
+ DEBOUNCER_STORE,
() => {
if (!newDraft.length && oldDraft) {
// If the draft has been modified to be empty, then erase the storage
diff --git a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
index 07dec91..4a86005 100644
--- a/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
+++ b/polygerrit-ui/app/elements/core/gr-error-manager/gr-error-manager.ts
@@ -73,6 +73,9 @@
errorOverlay: GrOverlay;
};
}
+
+const DEBOUNCER_CHECK_LOGGED_IN = 'checkLoggedIn';
+
@customElement('gr-error-manager')
export class GrErrorManager extends GestureEventListeners(
LegacyElementMixin(PolymerElement)
@@ -148,6 +151,7 @@
this.unlisten(document, 'show-error', '_handleShowErrorDialog');
this.unlisten(document, 'visibilitychange', '_handleVisibilityChange');
this.unlisten(document, 'show-auth-required', '_handleAuthRequired');
+ this.cancelDebouncer(DEBOUNCER_CHECK_LOGGED_IN);
if (this._authErrorHandlerDeregistrationHook) {
this._authErrorHandlerDeregistrationHook();
@@ -404,7 +408,7 @@
_requestCheckLoggedIn() {
this.debounce(
- 'checkLoggedIn',
+ DEBOUNCER_CHECK_LOGGED_IN,
this._checkSignedIn,
CHECK_SIGN_IN_INTERVAL_MS
);
@@ -480,7 +484,7 @@
}
_handleWindowFocus() {
- this.flushDebouncer('checkLoggedIn');
+ this.flushDebouncer(DEBOUNCER_CHECK_LOGGED_IN);
}
_handleShowErrorDialog(e: CustomEvent) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
index d5c7d8e..094f2d7 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.ts
@@ -54,6 +54,8 @@
rootId: string;
}
+const DEBOUNCER_SELECTION_CHANGE = 'selectionChange';
+
@customElement('gr-diff-highlight')
export class GrDiffHighlight extends GestureEventListeners(
LegacyElementMixin(PolymerElement)
@@ -88,6 +90,11 @@
);
}
+ /** @override */
+ detached() {
+ this.cancelDebouncer(DEBOUNCER_SELECTION_CHANGE);
+ }
+
get diffBuilder() {
if (!this._cachedDiffBuilder) {
this._cachedDiffBuilder = this.querySelector(
@@ -123,7 +130,7 @@
// ms, then you will have about 50 _handleSelection calls when doing a
// simple drag for select.
this.debounce(
- 'selectionChange',
+ DEBOUNCER_SELECTION_CHANGE,
() => this._handleSelection(selection, isMouseUp),
10
);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
index 034081d..9fafcfa 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.ts
@@ -64,6 +64,8 @@
*/
const MAX_GROUP_SIZE = 120;
+const DEBOUNCER_RESET_IS_SCROLLING = 'resetIsScrolling';
+
/**
* Converts the API's `DiffContent`s to `GrDiffGroup`s for rendering.
*
@@ -123,6 +125,7 @@
/** @override */
detached() {
super.detached();
+ this.cancelDebouncer(DEBOUNCER_RESET_IS_SCROLLING);
this.cancel();
this.unlisten(window, 'scroll', '_handleWindowScroll');
}
@@ -130,7 +133,7 @@
_handleWindowScroll() {
this._isScrolling = true;
this.debounce(
- 'resetIsScrolling',
+ DEBOUNCER_RESET_IS_SCROLLING,
() => {
this._isScrolling = false;
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index f6f80b3..7946061 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -291,6 +291,7 @@
/** @override */
detached() {
+ this.cancelDebouncer(RENDER_DIFF_TABLE_DEBOUNCE_NAME);
super.detached();
this._unobserveIncrementalNodes();
this._unobserveNodes();
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
index 84ca10a..e9637af 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
@@ -55,6 +55,8 @@
const STORAGE_DEBOUNCE_INTERVAL_MS = 100;
+const DEBOUNCER_STORE = 'store';
+
@customElement('gr-editor-view')
export class GrEditorView extends KeyboardShortcutMixin(
GestureEventListeners(LegacyElementMixin(PolymerElement))
@@ -145,6 +147,11 @@
});
}
+ /** @override */
+ detached() {
+ this.cancelDebouncer(DEBOUNCER_STORE);
+ }
+
get storageKey() {
return `c${this._changeNum}_ps${this._patchNum}_${this._path}`;
}
@@ -348,7 +355,7 @@
_handleContentChange(e: CustomEvent<{value: string}>) {
this.debounce(
- 'store',
+ DEBOUNCER_STORE,
() => {
const content = e.detail.value;
if (content) {
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.ts
index 6151a1d9..f4eb053 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete/gr-autocomplete.ts
@@ -68,6 +68,8 @@
AutocompleteCommitEventDetail
>;
+const DEBOUNCER_UPDATE_SUGGESTIONS = 'update-suggestions';
+
@customElement('gr-autocomplete')
export class GrAutocomplete extends KeyboardShortcutMixin(
GestureEventListeners(LegacyElementMixin(PolymerElement))
@@ -217,7 +219,7 @@
detached() {
super.detached();
this.unlisten(document.body, 'click', '_handleBodyClick');
- this.cancelDebouncer('update-suggestions');
+ this.cancelDebouncer(DEBOUNCER_UPDATE_SUGGESTIONS);
}
get focusStart() {
@@ -330,7 +332,7 @@
if (noDebounce) {
update();
} else {
- this.debounce('update-suggestions', update, DEBOUNCE_WAIT_MS);
+ this.debounce(DEBOUNCER_UPDATE_SUGGESTIONS, update, DEBOUNCE_WAIT_MS);
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 5d5f4f3..898aff3 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -100,6 +100,12 @@
};
}
+const DEBOUNCER_FIRE_UPDATE = 'fire-update';
+
+const DEBOUNCER_STORE = 'store';
+
+const DEBOUNCER_DRAFT_TOAST = 'draft-toast';
+
@customElement('gr-comment')
export class GrComment extends KeyboardShortcutMixin(
GestureEventListeners(LegacyElementMixin(PolymerElement))
@@ -293,7 +299,9 @@
/** @override */
detached() {
super.detached();
- this.cancelDebouncer('fire-update');
+ this.cancelDebouncer(DEBOUNCER_FIRE_UPDATE);
+ this.cancelDebouncer(DEBOUNCER_STORE);
+ this.cancelDebouncer(DEBOUNCER_DRAFT_TOAST);
if (this.textarea) {
this.textarea.closeDropdown();
}
@@ -486,7 +494,7 @@
_eraseDraftComment() {
// Prevents a race condition in which removing the draft comment occurs
// prior to it being saved.
- this.cancelDebouncer('store');
+ this.cancelDebouncer(DEBOUNCER_STORE);
if (!this.comment?.path) throw new Error('Cannot erase Draft Comment');
if (this.changeNum === undefined) {
@@ -538,7 +546,7 @@
}
_fireUpdate() {
- this.debounce('fire-update', () => {
+ this.debounce(DEBOUNCER_FIRE_UPDATE, () => {
this.dispatchEvent(
new CustomEvent('comment-update', {
detail: this._getEventPayload(),
@@ -647,7 +655,7 @@
const {path, line, range} = this.comment;
if (path) {
this.debounce(
- 'store',
+ DEBOUNCER_STORE,
() => {
const message = this._messageText;
if (this.changeNum === undefined) {
@@ -729,7 +737,7 @@
}
_fireDiscard() {
- this.cancelDebouncer('fire-update');
+ this.cancelDebouncer(DEBOUNCER_FIRE_UPDATE);
this.dispatchEvent(
new CustomEvent('comment-discard', {
detail: this._getEventPayload(),
@@ -852,7 +860,7 @@
// Cancel the debouncer so that error toasts from the error-manager will
// not be overridden.
- this.cancelDebouncer('draft-toast');
+ this.cancelDebouncer(DEBOUNCER_DRAFT_TOAST);
this._updateRequestToast(
this._numPendingDraftRequests.number,
/* requestFailed=*/ true
@@ -862,7 +870,7 @@
_updateRequestToast(numPending: number, requestFailed?: boolean) {
const message = this._getSavingMessage(numPending, requestFailed);
this.debounce(
- 'draft-toast',
+ DEBOUNCER_DRAFT_TOAST,
() => {
// Note: the event is fired on the body rather than this element because
// this element may not be attached by the time this executes, in which
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts
index e825d21..c744eab 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts
+++ b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts
@@ -37,6 +37,8 @@
}
}
+const DEBOUNCER_STORE = 'store';
+
@customElement('gr-editable-content')
export class GrEditableContent extends GestureEventListeners(
LegacyElementMixin(PolymerElement)
@@ -126,6 +128,11 @@
);
}
+ /** @override */
+ detached() {
+ this.cancelDebouncer(DEBOUNCER_STORE);
+ }
+
_contentChanged() {
/* A changed content means that either a different change has been loaded
* or new content was saved. Either way, let's reset the component.
@@ -143,7 +150,7 @@
const storageKey = this.storageKey;
this.debounce(
- 'store',
+ DEBOUNCER_STORE,
() => {
if (newContent.length) {
this.storage.setEditableContentItem(storageKey, newContent);
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
index 94f99d3..e67e1f6 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard/gr-hovercard-behavior.ts
@@ -143,6 +143,8 @@
detached() {
super.detached();
+ this.cancelShowDebouncer();
+ this.cancelHideDebouncer();
this.unlock();
}
diff --git a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts
index 97b45ac..bf532ef 100644
--- a/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts
+++ b/polygerrit-ui/app/elements/shared/gr-list-view/gr-list-view.ts
@@ -35,6 +35,8 @@
}
}
+const DEBOUNCER_RELOAD = 'reload';
+
@customElement('gr-list-view')
class GrListView extends GestureEventListeners(
LegacyElementMixin(PolymerElement)
@@ -67,7 +69,7 @@
/** @override */
detached() {
super.detached();
- this.cancelDebouncer('reload');
+ this.cancelDebouncer(DEBOUNCER_RELOAD);
}
_filterChanged(newFilter?: string, oldFilter?: string) {
@@ -81,7 +83,7 @@
_debounceReload(filter?: string) {
this.debounce(
- 'reload',
+ DEBOUNCER_RELOAD,
() => {
if (this.path) {
if (filter) {
diff --git a/polygerrit-ui/app/test/common-test-setup.ts b/polygerrit-ui/app/test/common-test-setup.ts
index beba88e..5f9051a 100644
--- a/polygerrit-ui/app/test/common-test-setup.ts
+++ b/polygerrit-ui/app/test/common-test-setup.ts
@@ -199,5 +199,10 @@
checkGlobalSpace();
removeIronOverlayBackdropStyleEl();
// Clean Polymer debouncer queue, so next tests will not be affected.
+ // WARNING! This will most likely not do what you expect. `flushDebouncers()`
+ // will only flush debouncers that were added using `enqueueDebouncer()`. So
+ // this will not affect "normal" debouncers that were added using
+ // `this.debounce()`. For those please be careful and cancel them using
+ // `this.cancelDebouncer()` in the `detached()` lifecycle hook.
flushDebouncers();
});
