Add feature to router: Block navigation
We want to use that for optimistic updates. While saving a comment we
don't want the user to leave the page, neither navigate to another page
within the app, nor close the browser tab.
Ultimately this can be used for all ongoing write requests. It is
implemented in a generic way.
Release-Notes: skip
Google-Bug-Id: b/262228572
Change-Id: Iefd3e9ca84730f6ade5b68a9c6364d95150b7b90
diff --git a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts
index 4af24cc..94241ea 100644
--- a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts
+++ b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.ts
@@ -26,4 +26,22 @@
* page.redirect() eventually just calls `window.history.replaceState()`.
*/
replaceUrl(url: string): void;
+
+ /**
+ * You can ask the router to block all navigation to other pages for a while,
+ * e.g. while there are unsaved comments. You must make sure to call
+ * `releaseNavigation()` with the same string shortly after to unblock the
+ * router.
+ *
+ * The provided reason must be non-empty.
+ */
+ blockNavigation(reason: string): void;
+
+ /**
+ * See `blockNavigation()`.
+ *
+ * This API is not counting. If you block navigation with the same reason
+ * multiple times, then one release call will unblock.
+ */
+ releaseNavigation(reason: string): void;
}
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 902616e..304f9ea 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -11,7 +11,7 @@
computeLatestPatchNum,
convertToPatchSetNum,
} from '../../../utils/patch-set-util';
-import {assertIsDefined} from '../../../utils/common-util';
+import {assert, assertIsDefined} from '../../../utils/common-util';
import {
BasePatchSetNum,
GroupId,
@@ -99,6 +99,11 @@
import {isFileUnchanged} from '../../../embed/diff/gr-diff/gr-diff-utils';
import {Route, ViewState} from '../../../models/views/base';
import {Model} from '../../../models/model';
+import {
+ InteractivePromise,
+ interactivePromise,
+ timeoutPromise,
+} from '../../../utils/async-util';
// TODO: Move all patterns to view model files and use the `Route` interface,
// which will enforce using `RegExp` in its `urlPattern` property.
@@ -291,6 +296,16 @@
private view?: GerritView;
+ // While this set is not empty, the router will refuse to navigate to
+ // other pages, but instead show an alert. It will also install a
+ // `beforeUnload` handler that prevents the browser from closing the tab.
+ private navigationBlockers: Set<string> = new Set<string>();
+
+ // While navigationBlockers is not empty, this promise will continuously
+ // check for navigationBlockers to become empty again.
+ // This is undefined, iff navigationBlockers is empty.
+ private navigationBlockerPromise?: InteractivePromise<void>;
+
readonly page = new Page();
constructor(
@@ -336,12 +351,42 @@
];
}
+ blockNavigation(reason: string): void {
+ assert(!!reason, 'empty reason is not allowed');
+ this.navigationBlockers.add(reason);
+ if (this.navigationBlockers.size === 1) {
+ this.navigationBlockerPromise = interactivePromise();
+ window.addEventListener('beforeunload', this.beforeUnloadHandler);
+ }
+ }
+
+ releaseNavigation(reason: string): void {
+ assert(!!reason, 'empty reason is not allowed');
+ this.navigationBlockers.delete(reason);
+ if (this.navigationBlockers.size === 0) {
+ window.removeEventListener('beforeunload', this.beforeUnloadHandler);
+ this.navigationBlockerPromise?.resolve();
+ }
+ }
+
+ private beforeUnloadHandler = (event: BeforeUnloadEvent) => {
+ const reason = [...this.navigationBlockers][0];
+ if (!reason) return;
+
+ event.preventDefault(); // Cancel the event (per the standard).
+ event.returnValue = reason; // Chrome requires returnValue to be set.
+ // Note that we could as well just use '' instead of `reason`. Browsers will
+ // just show a generic message anyway.
+ return reason;
+ };
+
finalize(): void {
for (const subscription of this.subscriptions) {
subscription.unsubscribe();
}
this.subscriptions = [];
this.page.stop();
+ window.removeEventListener('beforeunload', this.beforeUnloadHandler);
}
start() {
@@ -589,6 +634,30 @@
next();
});
+ // Block navigation while navigationBlockers exist. But wait 1 second for
+ // those blockers to resolve. If they do, then still navigate. We don't want
+ // to annoy users by forcing them to navigate twice only because it took
+ // another 200ms for a comment to save or something similar.
+ this.page.registerRoute(/(.*)/, (_, next) => {
+ if (this.navigationBlockers.size === 0) {
+ next();
+ return;
+ }
+
+ const msg = 'Waiting 1 second for navigation blockers to resolve ...';
+ fireAlert(document, msg);
+ Promise.race([this.navigationBlockerPromise, timeoutPromise(1000)]).then(
+ () => {
+ if (this.navigationBlockers.size === 0) {
+ next();
+ } else {
+ const reason = [...this.navigationBlockers][0];
+ fireAlert(document, `Navigation is blocked by: ${reason}`);
+ }
+ }
+ );
+ });
+
// Middleware
this.page.registerRoute(/(.*)/, (ctx, next) => {
document.body.scrollTop = 0;
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
index c4cb465..234bf95 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.ts
@@ -11,6 +11,8 @@
stubRestApi,
addListenerForTest,
waitUntilCalled,
+ mockPromise,
+ MockPromise,
} from '../../../test/test-utils';
import {GrRouter, routerToken} from './gr-router';
import {GerritView} from '../../../services/router/router-model';
@@ -254,6 +256,80 @@
});
});
+ suite('navigation blockers', () => {
+ let clock: sinon.SinonFakeTimers;
+ let redirectStub: sinon.SinonStub;
+ let urlPromise: MockPromise<string>;
+
+ setup(() => {
+ stubRestApi('setInProjectLookup');
+ urlPromise = mockPromise<string>();
+ redirectStub = sinon
+ .stub(router, 'redirect')
+ .callsFake(urlPromise.resolve);
+ router._testOnly_startRouter();
+ clock = sinon.useFakeTimers();
+ });
+
+ test('no blockers: normal redirect', async () => {
+ router.page.show('/settings/agreements');
+ const url = await urlPromise;
+ assert.isTrue(redirectStub.calledOnce);
+ assert.equal(url, '/settings/#Agreements');
+ });
+
+ test('redirect blocked', async () => {
+ const firstAlertPromise = mockPromise<Event>();
+ addListenerForTest(document, 'show-alert', firstAlertPromise.resolve);
+
+ router.blockNavigation('a good reason');
+ router.page.show('/settings/agreements');
+
+ const firstAlert = (await firstAlertPromise) as CustomEvent;
+ assert.equal(
+ firstAlert.detail.message,
+ 'Waiting 1 second for navigation blockers to resolve ...'
+ );
+
+ const secondAlertPromise = mockPromise<Event>();
+ addListenerForTest(document, 'show-alert', secondAlertPromise.resolve);
+
+ clock.tick(2000);
+
+ const secondAlert = (await secondAlertPromise) as CustomEvent;
+ assert.equal(
+ secondAlert.detail.message,
+ 'Navigation is blocked by: a good reason'
+ );
+
+ assert.isFalse(redirectStub.called);
+ });
+
+ test('redirect blocked, but resolved within one second', async () => {
+ const firstAlertPromise = mockPromise<Event>();
+ addListenerForTest(document, 'show-alert', firstAlertPromise.resolve);
+
+ router.blockNavigation('a good reason');
+ router.page.show('/settings/agreements');
+
+ const firstAlert = (await firstAlertPromise) as CustomEvent;
+ assert.equal(
+ firstAlert.detail.message,
+ 'Waiting 1 second for navigation blockers to resolve ...'
+ );
+
+ const secondAlertPromise = mockPromise<Event>();
+ addListenerForTest(document, 'show-alert', secondAlertPromise.resolve);
+
+ clock.tick(500);
+ router.releaseNavigation('a good reason');
+ clock.tick(2000);
+
+ await urlPromise;
+ assert.isTrue(redirectStub.calledOnce);
+ });
+ });
+
suite('route handlers', () => {
let redirectStub: sinon.SinonStub;
let setStateStub: sinon.SinonStub;
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index 19c3a7b..d897423 100644
--- a/polygerrit-ui/app/test/test-utils.ts
+++ b/polygerrit-ui/app/test/test-utils.ts
@@ -17,25 +17,8 @@
import {Route, ViewState} from '../models/views/base';
import {PageContext} from '../elements/core/gr-router/gr-page';
export {query, queryAll, queryAndAssert} from '../utils/common-util';
-
-export interface MockPromise<T> extends Promise<T> {
- resolve: (value?: T) => void;
- reject: (reason?: any) => void;
-}
-
-export function mockPromise<T = unknown>(): MockPromise<T> {
- let res: (value?: T) => void;
- let rej: (reason?: any) => void;
- const promise: MockPromise<T> = new Promise<T | undefined>(
- (resolve, reject) => {
- res = resolve;
- rej = reject;
- }
- ) as MockPromise<T>;
- promise.resolve = res!;
- promise.reject = rej!;
- return promise;
-}
+export {mockPromise} from '../utils/async-util';
+export type {MockPromise} from '../utils/async-util';
export function isHidden(el: Element | undefined | null) {
if (!el) return true;
diff --git a/polygerrit-ui/app/utils/async-util.ts b/polygerrit-ui/app/utils/async-util.ts
index 6ac3211..2dde07a 100644
--- a/polygerrit-ui/app/utils/async-util.ts
+++ b/polygerrit-ui/app/utils/async-util.ts
@@ -338,3 +338,36 @@
};
return wrappedPromise;
}
+
+export interface MockPromise<T> extends Promise<T> {
+ resolve: (value?: T) => void;
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ reject: (reason?: any) => void;
+}
+
+export function mockPromise<T = unknown>(): MockPromise<T> {
+ let res: (value?: T) => void;
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ let rej: (reason?: any) => void;
+ const promise: MockPromise<T> = new Promise<T | undefined>(
+ (resolve, reject) => {
+ res = resolve;
+ rej = reject;
+ }
+ ) as MockPromise<T>;
+ promise.resolve = res!;
+ promise.reject = rej!;
+ return promise;
+}
+
+// MockPromise is the established name in tests, and we don't want to rename
+// that in 50 files. But "Mock" is a bit misleading and definitely not a great
+// fit for non-test code. So let's also export under a different name.
+export type InteractivePromise<T> = MockPromise<T>;
+export const interactivePromise = mockPromise;
+
+export function timeoutPromise(timeoutMs: number): Promise<void> {
+ return new Promise<void>(resolve => {
+ setTimeout(resolve, timeoutMs);
+ });
+}
diff --git a/polygerrit-ui/app/utils/async-util_test.ts b/polygerrit-ui/app/utils/async-util_test.ts
index ee4f73a..afc16d3 100644
--- a/polygerrit-ui/app/utils/async-util_test.ts
+++ b/polygerrit-ui/app/utils/async-util_test.ts
@@ -7,9 +7,45 @@
import {SinonFakeTimers} from 'sinon';
import '../test/common-test-setup';
import {mockPromise, waitEventLoop, waitUntil} from '../test/test-utils';
-import {asyncForeach, debounceP, DelayedTask} from './async-util';
+import {
+ asyncForeach,
+ debounceP,
+ DelayedTask,
+ interactivePromise,
+ timeoutPromise,
+} from './async-util';
suite('async-util tests', () => {
+ suite('interactivePromise', () => {
+ test('simple test', async () => {
+ let resolved = false;
+ const promise = interactivePromise();
+ promise.then(() => (resolved = true));
+ assert.isFalse(resolved);
+ promise.resolve();
+ await promise;
+ assert.isTrue(resolved);
+ });
+ });
+
+ suite('timeoutPromise', () => {
+ let clock: SinonFakeTimers;
+ setup(() => {
+ clock = sinon.useFakeTimers();
+ });
+ test('simple test', async () => {
+ let resolved = false;
+ const promise = timeoutPromise(1000);
+ promise.then(() => (resolved = true));
+ assert.isFalse(resolved);
+ clock.tick(999);
+ assert.isFalse(resolved);
+ clock.tick(1);
+ await promise;
+ assert.isTrue(resolved);
+ });
+ });
+
suite('asyncForeach', () => {
test('loops over each item', async () => {
const fn = sinon.stub().resolves();
