From 6c35780d59dce3c0fb5d528c8f97adfae2f1fefa Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 27 Oct 2026 16:48:07 -0400 Subject: [PATCH] fix(gui): wait for the browser's prompt before closing the permission popup The deadline on a registration begun from a click counts no time while the browser's own notification prompt is open, in the permission popup and in the enable-push dialog. The enable-push dialog keeps one registration in flight, settles on whenever it resolves while open, and offers a retry only after a real failure. A permission dialog that ends in a denial after Allow began a registration turns push back off and restores the opt-out it cleared. --- src/gui/src/UI/UIEnablePushDialog.js | 50 ++++++++------- src/gui/src/UI/UIEnablePushDialog.test.js | 63 ++++++++++++++++++- .../src/UI/UIPermissionDialog.allow.test.js | 20 ++++++ src/gui/src/UI/UIPermissionDialog.js | 18 ++++++ src/gui/src/helpers/pushClient.js | 23 ++++++- src/gui/src/helpers/pushClient.test.js | 27 ++++++++ src/gui/src/initgui.gates.test.js | 5 +- src/gui/src/initgui.js | 10 +-- 8 files changed, 183 insertions(+), 33 deletions(-) diff --git a/src/gui/src/UI/UIEnablePushDialog.js b/src/gui/src/UI/UIEnablePushDialog.js index 92fa37d3a..0a68bd89b 100644 --- a/src/gui/src/UI/UIEnablePushDialog.js +++ b/src/gui/src/UI/UIEnablePushDialog.js @@ -20,15 +20,14 @@ import { browserPushState, enableBrowserPush, - PUSH_ENABLE_TIMEOUT_MS, pushClientWithin, + withEnableDeadline, } from '../helpers/pushClient.js'; import { guard_input, identity_html, INPUT_PROTECTION_MS, resolve_requesting_entity, - with_timeout, } from './UIPermissionDialog.js'; /** @typedef {import('../helpers/pushClient.js').BrowserPushState} BrowserPushState */ @@ -60,38 +59,45 @@ async function UIEnablePushDialog (options = {}) { const $not_now = $(el_dialog).find('.perm-dialog-deny'); const $error = $(el_dialog).find('.perm-dialog-error'); - let enabling = false; + // The one enable() in flight; cleared only when it failed, so a retry never overlaps it. + let pending = null; + // True from Turn on until the registration settles or outlasts its deadline. + let waiting = false; const sync_buttons = () => { - const enabled = !! input_guard?.ready && ! enabling; - $turn_on.prop('disabled', ! enabled); - $not_now.prop('disabled', ! enabled); + const ready = !! input_guard?.ready && ! waiting; + $turn_on.prop('disabled', ! ready || !! pending); + $not_now.prop('disabled', ! ready); }; sync_buttons(); $turn_on.on('click', async () => { - if ( enabling ) return; - enabling = true; + if ( pending ) return; + pending = enableBrowserPush(client); + pending.then((ok) => { + if ( ok ) settle('on'); + }); + waiting = true; $turn_on.addClass('perm-dialog-busy'); sync_buttons(); $error.hide(); - const enabled = await with_timeout(enableBrowserPush(client), PUSH_ENABLE_TIMEOUT_MS, false); - if ( enabled ) { - settle('on'); - return; + const outcome = await withEnableDeadline(pending, { fallback: null }); + if ( outcome === true ) return; + if ( outcome === false ) { + pending = null; + const now = await browserPushState(client); + if ( now !== 'off' ) { + settle(now); + return; + } } - const now = await browserPushState(client); - if ( now !== 'off' ) { - settle(now); - return; - } - enabling = false; + waiting = false; if ( ! el_dialog.open ) { settle('off'); return; } $error.text(i18n('push_enable_failed', [], false)).show(); - $turn_on.removeClass('perm-dialog-busy'); + if ( ! pending ) $turn_on.removeClass('perm-dialog-busy'); sync_buttons(); }); @@ -99,11 +105,11 @@ async function UIEnablePushDialog (options = {}) { el_dialog.addEventListener('cancel', (e) => { e.preventDefault(); - if ( enabling ) return; + if ( waiting ) return; settle('off'); }); el_dialog.addEventListener('close', () => { - if ( enabling ) return; + if ( waiting ) return; settle('off'); }); @@ -121,7 +127,7 @@ async function UIEnablePushDialog (options = {}) { ms: INPUT_PROTECTION_MS, on_change: (ready) => { sync_buttons(); - if ( ready && ! enabling ) $not_now.get(0)?.focus(); + if ( ready && ! waiting ) $not_now.get(0)?.focus(); }, }); }); diff --git a/src/gui/src/UI/UIEnablePushDialog.test.js b/src/gui/src/UI/UIEnablePushDialog.test.js index 28dc742a6..2fa09ab6e 100644 --- a/src/gui/src/UI/UIEnablePushDialog.test.js +++ b/src/gui/src/UI/UIEnablePushDialog.test.js @@ -17,7 +17,7 @@ * You should have received a copy of the GNU Affero General Public License * along with this program. If not, see . */ -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import jQuery from '../lib/jquery-3.6.1/jquery-3.6.1.min.js'; globalThis.$ = jQuery; @@ -138,6 +138,67 @@ describe('the enable-push dialog', () => { debug.mockRestore(); }); + describe('with the clock under control', () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(() => { + vi.useRealTimers(); + delete globalThis.Notification; + }); + + const shownFake = async () => { + await vi.waitFor(() => expect(document.querySelector('.enable-push-dialog')).not.toBeNull()); + await vi.advanceTimersByTimeAsync(INPUT_PROTECTION_MS + 50); + }; + + it('waits as long as the browser prompt is open, then settles on when it registers', async () => { + globalThis.Notification = { permission: 'default' }; + const client = fakeClient({ permission: 'default', registered: false }); + let finish; + client.enable.mockImplementation(() => new Promise((resolve) => { + finish = resolve; + })); + registerPushClient(client); + + const result = UIEnablePushDialog({ origin: 'https://chat.example' }); + await shownFake(); + $('.perm-dialog-allow').trigger('click'); + await vi.advanceTimersByTimeAsync(60_000); + expect($('.perm-dialog-error').text()).toBe(''); + expect($('.perm-dialog-not-now, .perm-dialog-deny').prop('disabled')).toBe(true); + expect(document.querySelector('dialog')).not.toBeNull(); + + globalThis.Notification.permission = 'granted'; + finish({ deviceUid: 'dev-new' }); + await vi.advanceTimersByTimeAsync(10); + await expect(result).resolves.toBe('on'); + expect(client.enable).toHaveBeenCalledTimes(1); + }); + + it('keeps the one registration after its deadline and settles on when it resolves', async () => { + globalThis.Notification = { permission: 'granted' }; + const client = fakeClient({ permission: 'granted', registered: false }); + let finish; + client.enable.mockImplementation(() => new Promise((resolve) => { + finish = resolve; + })); + registerPushClient(client); + + const result = UIEnablePushDialog({ origin: 'https://chat.example' }); + await shownFake(); + $('.perm-dialog-allow').trigger('click'); + await vi.advanceTimersByTimeAsync(16_000); + expect($('.perm-dialog-error').text()).toBe('push_enable_failed'); + expect($('.perm-dialog-allow').prop('disabled')).toBe(true); + expect($('.perm-dialog-deny').prop('disabled')).toBe(false); + $('.perm-dialog-allow').trigger('click'); + expect(client.enable).toHaveBeenCalledTimes(1); + + finish({ deviceUid: 'dev-new' }); + await vi.advanceTimersByTimeAsync(10); + await expect(result).resolves.toBe('on'); + }); + }); + it('offers a retry when registering failed and push is still off', async () => { const client = fakeClient({ permission: 'default', registered: false }); client.enable.mockRejectedValueOnce(new Error('no_worker')); diff --git a/src/gui/src/UI/UIPermissionDialog.allow.test.js b/src/gui/src/UI/UIPermissionDialog.allow.test.js index 52881d9db..c58c78d33 100644 --- a/src/gui/src/UI/UIPermissionDialog.allow.test.js +++ b/src/gui/src/UI/UIPermissionDialog.allow.test.js @@ -123,6 +123,26 @@ describe('Allow on a request for notifications:push', () => { errors.mockRestore(); }); + it('turns push back off, and restores the opt-out, when the grant fails and the user then denies', async () => { + const client = fakeClient({ permission: 'default', registered: false }); + registerPushClient(client); + localStorage.setItem('puter.push.browser-off:u-1', '1'); + fetch.mockResolvedValue({ ok: false, status: 500 }); + const errors = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const decision = UIPermissionDialog({ permissions: ['notifications:push'], app_uid: `app-${apps}` }); + await shown(); + allow(); + await vi.waitFor(() => expect($('.perm-dialog-error').text()).toBe('perm_dialog_error')); + expect(localStorage.getItem('puter.push.browser-off:u-1')).toBeNull(); + $('.perm-dialog-deny').trigger('click'); + + await expect(decision).resolves.toBe(false); + await vi.waitFor(() => expect(client.disable).toHaveBeenCalledTimes(1)); + await vi.waitFor(() => expect(localStorage.getItem('puter.push.browser-off:u-1')).toBe('1')); + errors.mockRestore(); + }); + it.each([ ['on', { permission: 'granted', registered: true, deviceUid: 'dev-1' }], ['blocked', { permission: 'denied', registered: false }], diff --git a/src/gui/src/UI/UIPermissionDialog.js b/src/gui/src/UI/UIPermissionDialog.js index 210b07d86..d6943ad29 100644 --- a/src/gui/src/UI/UIPermissionDialog.js +++ b/src/gui/src/UI/UIPermissionDialog.js @@ -20,11 +20,13 @@ import { fsCreateKindFor } from '../helpers/fsCreateKind.js'; import { get_app_by_uid } from '../helpers/getAppByUid.js'; import { + browserPushOptedOut, browserPushState, enableBrowserPush, getPushClient, PUSH_PERMISSION, pushClientWithin, + setBrowserPushOptOut, } from '../helpers/pushClient.js'; // Keeps a second request for the same (entity, permission) from stacking a @@ -198,6 +200,8 @@ async function show_permission_dialog (options) { document.body.appendChild(el_dialog); // The registration Allow began, kept so a second Allow doesn't start another. let push_enable = null; + const user_uid = window.user?.uuid; + const opted_out_before = browserPushOptedOut(user_uid); // Set once a grant request has been sent without the server definitively // refusing it. A client-side timeout or a dropped response says nothing @@ -217,6 +221,9 @@ async function show_permission_dialog (options) { // account is reconciled in the background. undo_uncertain_grant(options); } + if ( ! granted && push_enable ) { + undo_browser_push(push_enable, user_uid, opted_out_before); + } // `close()` fires the 'close' event; the handler below resolves // false, so resolve first. resolve(granted); @@ -449,6 +456,17 @@ export function guard_input ({ win, doc, ms, on_change }) { }; } +/** Turns push back off in this browser when Allow began a registration and the dialog still ended in a denial. @param {Promise} push_enable @param {string|undefined} user_uid @param {boolean} opted_out_before */ +function undo_browser_push (push_enable, user_uid, opted_out_before) { + push_enable.then((ok) => { + if ( ! ok ) return; + getPushClient()?.disable().then( + () => setBrowserPushOptOut(user_uid, opted_out_before), + (e) => console.debug('[push] disable failed', e), + ); + }); +} + /** * Withdraws a grant whose POST may have committed even though the dialog never * saw a success response, so a "Don't Allow" can't leave a live permission the diff --git a/src/gui/src/helpers/pushClient.js b/src/gui/src/helpers/pushClient.js index a0e56d23c..adaa098e0 100644 --- a/src/gui/src/helpers/pushClient.js +++ b/src/gui/src/helpers/pushClient.js @@ -34,9 +34,12 @@ export const PUSH_PERMISSION = 'notifications:push'; /** How long a dialog waits for the client an extension registers after its own checks. */ export const PUSH_CLIENT_WAIT_MS = 3000; -/** How long a registration begun from a click may take before the caller moves on. */ +/** How long a registration begun from a click may take, once the browser's prompt is answered, before the caller moves on. */ export const PUSH_ENABLE_TIMEOUT_MS = 15000; +/** How often the deadline below looks at whether the browser's prompt is still open. */ +export const PROMPT_POLL_MS = 250; + /** @type {PushClient|null} */ let pushClient = null; @@ -129,4 +132,22 @@ export const enableBrowserPush = (client, userUid = globalThis.user?.uuid) => { }); }; +/** `promise`, or `fallback` once `ms` have passed with the browser's notification prompt closed; time with the prompt open does not count. @param {Promise} promise @param {{ ms?: number, fallback?: F, NotificationApi?: { permission: string }, poll?: number }} [opts] @returns {Promise} @template T, F */ +export const withEnableDeadline = (promise, { + ms = PUSH_ENABLE_TIMEOUT_MS, + fallback = /** @type {F} */ (false), + NotificationApi = globalThis.Notification, + poll = PROMPT_POLL_MS, +} = {}) => { + let timer; + const deadline = new Promise((resolve) => { + const tick = () => { + const open = NotificationApi?.permission === 'default'; + timer = setTimeout(open ? tick : () => resolve(fallback), open ? poll : ms); + }; + tick(); + }); + return Promise.race([promise, deadline]).finally(() => clearTimeout(timer)); +}; + globalThis.def?.(registerPushClient, 'push.registerPushClient'); diff --git a/src/gui/src/helpers/pushClient.test.js b/src/gui/src/helpers/pushClient.test.js index f15770e10..b0d9ba054 100644 --- a/src/gui/src/helpers/pushClient.test.js +++ b/src/gui/src/helpers/pushClient.test.js @@ -150,6 +150,33 @@ describe('the opt-out record', () => { }); }); +describe('withEnableDeadline', () => { + const never = new Promise(() => {}); + const granted = { permission: 'granted' }; + + it('answers the promise when it settles in time', async () => { + await expect(mod.withEnableDeadline(Promise.resolve(true), { ms: 50, NotificationApi: granted })).resolves.toBe(true); + }); + + it('gives the fallback once the time is up with the prompt closed', async () => { + await expect(mod.withEnableDeadline(never, { ms: 20, fallback: null, NotificationApi: granted })).resolves.toBeNull(); + await expect(mod.withEnableDeadline(never, { ms: 20, NotificationApi: undefined })).resolves.toBe(false); + }); + + it('counts no time while the browser prompt is open', async () => { + const NotificationApi = { permission: 'default' }; + let done = false; + const waited = mod.withEnableDeadline(never, { ms: 20, poll: 5, NotificationApi }).then(() => { + done = true; + }); + await new Promise((resolve) => setTimeout(resolve, 80)); + expect(done).toBe(false); + NotificationApi.permission = 'denied'; + await waited; + expect(done).toBe(true); + }); +}); + describe('enableBrowserPush', () => { beforeEach(() => localStorage.clear()); diff --git a/src/gui/src/initgui.gates.test.js b/src/gui/src/initgui.gates.test.js index 1b851b42e..ff4687a5b 100644 --- a/src/gui/src/initgui.gates.test.js +++ b/src/gui/src/initgui.gates.test.js @@ -93,12 +93,13 @@ describe('the permission popup and browser push', () => { it('takes the registration Allow began and waits for it after answering', () => { expect(requestPermission).toContain('on_push_enable: (enabling) => {'); const answered = requestPermission.indexOf("msg: 'permissionGranted'"); - const waited = requestPermission.indexOf('push_enable,'); + // The deadline leaves the browser's own prompt its time. + const waited = requestPermission.indexOf('await withEnableDeadline(push_enable)'); const closed = requestPermission.indexOf('window.close()'); expect(answered).toBeGreaterThan(-1); expect(waited).toBeGreaterThan(answered); expect(closed).toBeGreaterThan(waited); - expect(requestPermission).toContain('PUSH_ENABLE_TIMEOUT_MS'); + expect(requestPermission).not.toContain('setTimeout'); }); it('answers the enable-push opener after the dialog, then closes', () => { diff --git a/src/gui/src/initgui.js b/src/gui/src/initgui.js index 8085abd8d..bcc574d43 100644 --- a/src/gui/src/initgui.js +++ b/src/gui/src/initgui.js @@ -59,7 +59,7 @@ import { } from './util/authmeGrant.js'; import init_device_signals from './helpers/deviceSignals.js'; import { holdsPermissions } from './helpers/holdsPermissions.js'; -import { PUSH_ENABLE_TIMEOUT_MS, PUSH_PERMISSION } from './helpers/pushClient.js'; +import { PUSH_PERMISSION, withEnableDeadline } from './helpers/pushClient.js'; import { startPushWorker } from './helpers/pushWorker.js'; import item_icon from './helpers/itemIcon.js'; import { installAppIconFallback } from './helpers/appIcon.js'; @@ -864,12 +864,8 @@ const postAuthActions = async (action) => { // The popup exists only to host this dialog; close it once answered. if ( window.embedded_in_popup ) { - if ( push_enable ) { - await Promise.race([ - push_enable, - new Promise((resolve) => setTimeout(resolve, PUSH_ENABLE_TIMEOUT_MS)), - ]); - } + // Closing would take the browser's own prompt down with it, so its time doesn't count. + if ( push_enable ) await withEnableDeadline(push_enable); window.close(); window.open('', '_self').close(); }