Various minor fixes (#2360)

* Various minor fixes

* Add comment

* Add comments

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)

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
 

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) => {