PR comments

cli/CHANGELOG.md

@@ -5,7 +5,7 @@ _Released 12/19/2023 (PENDING)_
 
 **Bugfixes:**
 
-- Fixed asset capture for Test Replay (which will potentially affect styles) specifically for requests that are routed through service workers. Fixes [#28516](https://github.com/cypress-io/cypress/issues/28516).
+- Fixed asset capture for Test Replay for requests that are routed through service workers. This addresses an issue where styles were not being applied properly in Test Replay. Fixes [#28516](https://github.com/cypress-io/cypress/issues/28516).
 - Cypress no longer errors during `cypress run` or `cypress open` when using Typescript 5.3.2+ with `extends` in `tsconfig.json`. Fixes [#28385](https://github.com/cypress-io/cypress/issues/28385).
 - Fixed a regression in [`13.6.1`](https://docs.cypress.io/guides/references/changelog/13.6.1) where a malformed URI would crash Cypress. Fixes [#28521](https://github.com/cypress-io/cypress/issues/28521).
 - Fixed a regression in [`12.4.0`](https://docs.cypress.io/guides/references/changelog/12.4.0) where erroneous `<br>` tags were displaying in error messages in the Command Log making them less readable. Fixes [#28452](https://github.com/cypress-io/cypress/issues/28452).

packages/proxy/lib/http/index.ts

@@ -27,6 +27,8 @@ import type { CookieJar, SerializableAutomationCookie } from '@packages/server/l
 import type { ResourceTypeAndCredentialManager } from '@packages/server/lib/util/resourceTypeAndCredentialManager'
 import type { ProtocolManagerShape } from '@packages/types'
 import type Protocol from 'devtools-protocol'
+import { ServiceWorkerManager } from './util/service-worker-manager'
+import { hasServiceWorkerHeader } from './util/headers'
 
 function getRandomColorFn () {
   return chalk.hex(`#${Number(
@@ -274,6 +276,7 @@ export class Http {
   autUrl?: string
   getCookieJar: () => CookieJar
   protocolManager?: ProtocolManagerShape
+  serviceWorkerManager: ServiceWorkerManager = new ServiceWorkerManager()
 
   constructor (opts: ServerCtx & { middleware?: HttpMiddlewareStacks }) {
     this.buffers = new HttpBuffers()
@@ -434,8 +437,10 @@ export class Http {
     this.buffers.reset()
     this.setAUTUrl(undefined)
 
+    // Only reset prerequests in situations where we no longer care about the previous prerequests (e.g. when we are starting a new spec in a new browser tab)
     if (options.resetPrerequests) {
       this.preRequests.reset()
+      this.serviceWorkerManager = new ServiceWorkerManager()
     }
   }
 
@@ -444,6 +449,10 @@ export class Http {
   }
 
   addPendingBrowserPreRequest (browserPreRequest: BrowserPreRequest) {
+    if (this.shouldIgnorePendingRequest(browserPreRequest)) {
+      return
+    }
+
     this.preRequests.addPending(browserPreRequest)
   }
 
@@ -456,15 +465,25 @@ export class Http {
   }
 
   updateServiceWorkerRegistrations (data: Protocol.ServiceWorker.WorkerRegistrationUpdatedEvent) {
-    this.preRequests.updateServiceWorkerRegistrations(data)
+    data.registrations.forEach((registration) => {
+      if (registration.isDeleted) {
+        this.serviceWorkerManager.unregisterServiceWorker({ registrationId: registration.registrationId })
+      } else {
+        this.serviceWorkerManager.registerServiceWorker({ registrationId: registration.registrationId, scopeURL: registration.scopeURL })
+      }
+    })
   }
 
   updateServiceWorkerVersions (data: Protocol.ServiceWorker.WorkerVersionUpdatedEvent) {
-    this.preRequests.updateServiceWorkerVersions(data)
+    data.versions.forEach((version) => {
+      if (version.status === 'activated') {
+        this.serviceWorkerManager.addActivatedServiceWorker({ registrationId: version.registrationId, scriptURL: version.scriptURL })
+      }
+    })
   }
 
   updateServiceWorkerClientSideRegistrations (data: { scriptURL: string, initiatorURL: string }) {
-    this.preRequests.updateServiceWorkerClientSideRegistrations(data)
+    this.serviceWorkerManager.addInitiatorToServiceWorker({ scriptURL: data.scriptURL, initiatorURL: data.initiatorURL })
   }
 
   setProtocolManager (protocolManager: ProtocolManagerShape) {
@@ -475,4 +494,23 @@ export class Http {
   setPreRequestTimeout (timeout: number) {
     this.preRequests.setPreRequestTimeout(timeout)
   }
+
+  private shouldIgnorePendingRequest (browserPreRequest: BrowserPreRequest) {
+    // The initial request that loads the service worker does not always get sent to CDP. If it does, we want it to not clog up either the prerequests
+    // or pending requests. Thus, we need to explicitly ignore it here and in `get`. We determine it's the service worker request via the
+    // `service-worker` header
+    if (hasServiceWorkerHeader(browserPreRequest.headers)) {
+      debugVerbose('Ignoring service worker script since we are not guaranteed to receive it: %o', browserPreRequest)
+
+      return true
+    }
+
+    if (this.serviceWorkerManager.processBrowserPreRequest(browserPreRequest)) {
+      debugVerbose('Not correlating request since it is fully controlled by the service worker and the correlation will happen within the service worker: %o', browserPreRequest)
+
+      return true
+    }
+
+    return false
+  }
 }

packages/proxy/lib/http/util/headers.ts

@@ -0,0 +1,3 @@
+export const hasServiceWorkerHeader = (headers: Record<string, string | string[] | undefined>) => {
+  return headers?.['service-worker'] === 'script' || headers?.['Service-Worker'] === 'script'
+}

packages/proxy/lib/http/util/prerequests.ts

@@ -5,8 +5,7 @@ import type {
 } from '@packages/proxy'
 import type { ProtocolManagerShape } from '@packages/types'
 import Debug from 'debug'
-import { ServiceWorkerManager } from './service-worker-manager'
-import type Protocol from 'devtools-protocol'
+import { hasServiceWorkerHeader } from './headers'
 
 const debug = Debug('cypress:proxy:http:util:prerequests')
 const debugVerbose = Debug('cypress-verbose:proxy:http:util:prerequests')
@@ -120,7 +119,6 @@ export class PreRequests {
   pendingUrlsWithoutPreRequests = new QueueMap<PendingUrlWithoutPreRequest>()
   sweepIntervalTimer: NodeJS.Timeout
   protocolManager?: ProtocolManagerShape
-  serviceWorkerManager: ServiceWorkerManager = new ServiceWorkerManager()
 
   constructor (
     requestTimeout = 2000,
@@ -162,10 +160,6 @@ export class PreRequests {
   addPending (browserPreRequest: BrowserPreRequest) {
     const key = `${browserPreRequest.method}-${tryDecodeURI(browserPreRequest.url)}`
 
-    if (this.shouldIgnorePendingRequest(browserPreRequest)) {
-      return
-    }
-
     metrics.browserPreRequestsReceived++
     const pendingRequest = this.pendingRequests.shift(key)
 
@@ -240,7 +234,7 @@ export class PreRequests {
   get (req: CypressIncomingRequest, ctxDebug, callback: GetPreRequestCb) {
     // The initial request that loads the service worker does not always get sent to CDP. Thus, we need to explicitly ignore it. We determine
     // it's the service worker request via the `service-worker` header
-    if (PreRequests.hasServiceWorkerHeader(req.headers)) {
+    if (hasServiceWorkerHeader(req.headers)) {
       ctxDebug('Ignoring service worker script since we are not guaranteed to receive it', req.proxiedUrl)
 
       callback({
@@ -323,30 +317,7 @@ export class PreRequests {
     delete pendingRequest.callback
   }
 
-  updateServiceWorkerRegistrations (data: Protocol.ServiceWorker.WorkerRegistrationUpdatedEvent) {
-    data.registrations.forEach((registration) => {
-      if (registration.isDeleted) {
-        this.serviceWorkerManager.unregisterServiceWorker({ registrationId: registration.registrationId })
-      } else {
-        this.serviceWorkerManager.registerServiceWorker({ registrationId: registration.registrationId, scopeURL: registration.scopeURL })
-      }
-    })
-  }
-
-  updateServiceWorkerVersions (data: Protocol.ServiceWorker.WorkerVersionUpdatedEvent) {
-    data.versions.forEach((version) => {
-      if (version.status === 'activated') {
-        this.serviceWorkerManager.addActivatedServiceWorker({ registrationId: version.registrationId, scriptURL: version.scriptURL })
-      }
-    })
-  }
-
-  updateServiceWorkerClientSideRegistrations (data: { scriptURL: string, initiatorURL: string }) {
-    this.serviceWorkerManager.addInitiatorToServiceWorker({ scriptURL: data.scriptURL, initiatorURL: data.initiatorURL })
-  }
-
   reset () {
-    this.serviceWorkerManager = new ServiceWorkerManager()
     this.pendingPreRequests = new QueueMap<PendingPreRequest>()
 
     // Clear out the pending requests timeout callbacks first then clear the queue
@@ -361,27 +332,4 @@ export class PreRequests {
     this.pendingRequests = new QueueMap<PendingRequest>()
     this.pendingUrlsWithoutPreRequests = new QueueMap<PendingUrlWithoutPreRequest>()
   }
-
-  private static hasServiceWorkerHeader (headers: Record<string, string | string[] | undefined>) {
-    return headers?.['service-worker'] === 'script' || headers?.['Service-Worker'] === 'script'
-  }
-
-  private shouldIgnorePendingRequest (browserPreRequest: BrowserPreRequest) {
-    // The initial request that loads the service worker does not always get sent to CDP. If it does, we want it to not clog up either the prerequests
-    // or pending requests. Thus, we need to explicitly ignore it here and in `get`. We determine it's the service worker request via the
-    // `service-worker` header
-    if (PreRequests.hasServiceWorkerHeader(browserPreRequest.headers)) {
-      debugVerbose('Ignoring service worker script since we are not guaranteed to receive it: %o', browserPreRequest)
-
-      return true
-    }
-
-    if (this.serviceWorkerManager.processBrowserPreRequest(browserPreRequest)) {
-      debugVerbose('Not correlating request since it is fully controlled by the service worker and the correlation will happen within the service worker: %o', browserPreRequest)
-
-      return true
-    }
-
-    return false
-  }
 }

packages/proxy/lib/http/util/service-worker-manager.ts

@@ -1,5 +1,8 @@
+import Debug from 'debug'
 import type { BrowserPreRequest } from '../../types'
 
+const debug = Debug('cypress:proxy:service-worker-manager')
+
 type ServiceWorkerRegistration = {
   registrationId: string
   scopeURL: string
@@ -42,7 +45,7 @@ type AddInitiatorToServiceWorkerOptions = {
  *
  * At some point while 1 and 2 are happening:
  *
- * 3. We receive a message from the browser that a service worker has been registered with the `addInitiatorToServiceWorker` method.
+ * 3. We receive a message from the browser that a service worker has been initiated with the `addInitiatorToServiceWorker` method.
  *
  * At this point, when the manager tries to process a browser pre-request, it will check if the request is controlled by a service worker.
  * It determines it is controlled by a service worker if:
@@ -93,6 +96,8 @@ export class ServiceWorkerManager {
       }
 
       this.pendingInitiators.delete(scriptURL)
+    } else {
+      debug('Could not find service worker registration for registration ID %s', registrationId)
     }
   }
 
@@ -103,13 +108,14 @@ export class ServiceWorkerManager {
   addInitiatorToServiceWorker ({ scriptURL, initiatorURL }: AddInitiatorToServiceWorkerOptions) {
     let initiatorAdded = false
 
-    this.serviceWorkerRegistrations.forEach((registration) => {
+    for (const registration of this.serviceWorkerRegistrations.values()) {
       if (registration.activatedServiceWorker?.scriptURL === scriptURL) {
         registration.activatedServiceWorker.initiatorURL = initiatorURL
 
         initiatorAdded = true
+        break
       }
-    })
+    }
 
     if (!initiatorAdded) {
       this.pendingInitiators.set(scriptURL, initiatorURL)