From 08bc5e00eff360f629bb0caf4903a7b3171b0720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ricardo=20Iv=C3=A1n=20Vieitez=20Parra?= <3857362+corrideat@users.noreply.github.com> Date: Sun, 29 Sep 2024 15:56:50 +0000 Subject: [PATCH] Various minor fixes (#2360) * Various minor fixes * Add comment * Add comments --- frontend/controller/app/chatroom.js | 22 +++++++++++- frontend/controller/service-worker.js | 52 +++++++++++++++++---------- test/cypress/support/index.js | 17 +++++---- 3 files changed, 65 insertions(+), 26 deletions(-) diff --git a/frontend/controller/app/chatroom.js b/frontend/controller/app/chatroom.js index 2971c86675..f74b733c41 100644 --- a/frontend/controller/app/chatroom.js +++ b/frontend/controller/app/chatroom.js @@ -14,7 +14,27 @@ sbp('okTurtles.events/on', JOINED_CHATROOM, ({ identityContractID, groupContract const rootState = sbp('state/vuex/state') if (rootState.loggedIn?.identityContractID !== identityContractID) return if (!rootState.chatroom.currentChatRoomIDs[groupContractID] || rootState.chatroom.pendingChatRoomIDs[groupContractID] === chatRoomID) { - sbp('state/vuex/commit', 'setCurrentChatRoomId', { groupID: groupContractID, chatRoomID }) + let attemptCount = 0 + // Sometimes, the state may not be ready (it needs to be copied from the SW + // to Vuex). In this case, we try again after a short delay. + // The specific issue is that the browsing-side state is updated in response + // to the EVENT_HANDLED event. Although that event is correctly emitted + // prior to JOINED_CHATROOM, processing might take slightly longer, causing + // rootState[chatRoomID]?.members?.[identityContractID] to be briefly + // undefined. + // TODO: Figure out a better way of doing this that doesn't require a timeout + const setCurrentChatRoomId = () => { + if (!rootState[chatRoomID]?.members?.[identityContractID]) { + if (++attemptCount > 5) { + console.warn('[JOINED_CHATROOM] Given up on setCurrentChatRoomId after 5 attempts', { identityContractID, groupContractID, chatRoomID }) + return + } + setTimeout(setCurrentChatRoomId, 5 + 5 * attemptCount) + } else { + sbp('state/vuex/commit', 'setCurrentChatRoomId', { groupID: groupContractID, chatRoomID }) + } + } + setCurrentChatRoomId() } }) sbp('okTurtles.events/on', LEFT_CHATROOM, switchCurrentChatRoomHandler) diff --git a/frontend/controller/service-worker.js b/frontend/controller/service-worker.js index 2259341d16..744ba52da7 100644 --- a/frontend/controller/service-worker.js +++ b/frontend/controller/service-worker.js @@ -29,31 +29,47 @@ sbp('sbp/selectors/register', { // TODO: move ahead with encryption stuff ignoring this service worker stuff for now // TODO: improve updating the sw: https://stackoverflow.com/a/49748437 // NOTE: user should still be able to use app even if all the SW stuff fails - if (!('serviceWorker' in navigator)) { return } + if (!('serviceWorker' in navigator)) { + throw new Error('Service worker APIs missing') + } try { const swRegistration = await navigator.serviceWorker.register('/assets/js/sw-primary.js', { scope: '/' }) - if (swRegistration) { + // This ensures that the 'store-client-id' message is always sent to the + // service worker, even if it's not immediately available. Calling `removeEventListener` is fine because presumably the page performing an + // update is sending this message. However, the real question is whether + // we _need_ to store a client ID (broadcasting should be preferred) and + // for instances where a single client ID is needed, this should be + // done periodically (e.g., to identify an active window). + // TODO: Clarify the use and need for 'store-client-id'. This will become + // clearer once implementing notifications. + if (swRegistration.active) { swRegistration.active?.postMessage({ type: 'store-client-id' }) - - // if an active service-worker exists, checks for the updates immediately first and then repeats it every 1hr - await swRegistration.update() - setInterval(() => sbp('service-worker/update'), HOURS_MILLIS) - - // Keep the service worker alive while the window is open - // The default idle timeout on Chrome and Firefox is 30 seconds. We send - // a ping message every 5 seconds to ensure that the worker remains - // active. - // The downside of this is that there are messges going back and forth - // between the service worker and each tab, the number of which is - // proportional to the number of tabs open. - // The upside of this is that the service worker remains active while - // there are open tabs, which makes it faster and smoother to interact - // with contracts than if the service worker had to be restarted. - setInterval(() => navigator.serviceWorker.controller?.postMessage({ type: 'ping' }), 5000) + } else { + const handler = () => { + navigator.serviceWorker.removeEventListener('controllerchange', handler, false) + navigator.serviceWorker.controller.postMessage({ type: 'store-client-id' }) + } + navigator.serviceWorker.addEventListener('controllerchange', handler, false) } + // if an active service-worker exists, checks for the updates immediately first and then repeats it every 1hr + await swRegistration.update() + setInterval(() => sbp('service-worker/update'), HOURS_MILLIS) + + // Keep the service worker alive while the window is open + // The default idle timeout on Chrome and Firefox is 30 seconds. We send + // a ping message every 5 seconds to ensure that the worker remains + // active. + // The downside of this is that there are messges going back and forth + // between the service worker and each tab, the number of which is + // proportional to the number of tabs open. + // The upside of this is that the service worker remains active while + // there are open tabs, which makes it faster and smoother to interact + // with contracts than if the service worker had to be restarted. + setInterval(() => navigator.serviceWorker.controller?.postMessage({ type: 'ping' }), 5000) + navigator.serviceWorker.addEventListener('message', event => { const data = event.data diff --git a/test/cypress/support/index.js b/test/cypress/support/index.js index f76cb4867a..de5ec13e45 100644 --- a/test/cypress/support/index.js +++ b/test/cypress/support/index.js @@ -20,9 +20,7 @@ import './output-logs.js' before(function () { console.log('[cypress] `before`: cleaning up') - if (!process.env.CI && - typeof navigator === 'object' && - navigator.serviceWorker) { + if (typeof navigator === 'object' && navigator.serviceWorker) { cy.wrap(navigator.serviceWorker.getRegistrations() .then((registrations) => { console.log('[cypress] Service worker registrations', registrations) @@ -33,8 +31,7 @@ before(function () { registration.waiting?.postMessage({ type: 'shutdown' }) return registration.unregister() })) - } - ) + }) ) } @@ -61,8 +58,14 @@ afterEach(function () { // Prevent errors when English is not the current OS locale language. Cypress.on('window:before:load', window => { - Object.defineProperty(window.navigator, 'language', { value: 'en-US-POSIX' }) - Object.defineProperty(window.navigator, 'languages', { value: ['en-US-POSIX', 'en-US', 'en'] }) + // We use defineProperty because the property may be read-only, and thus + // setting it directly may not work + // Also, `configurable` is set to true so that running this code multiple + // times doesn't raise an error. Otherwise, the property is marked as + // non-configurable and if this code runs more than once, an error with be + // thrown. + Object.defineProperty(window.navigator, 'language', { value: 'en-US-POSIX', configurable: true }) + Object.defineProperty(window.navigator, 'languages', { value: ['en-US-POSIX', 'en-US', 'en'], configurable: true }) }) Cypress.on('uncaught:exception', (err, runnable, promise) => {