Conversation
📊 Performance Test ResultsComparing 4d4054e vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
There was a problem hiding this comment.
The changes in this file aren't related to the core purpose of this PR. I've just found the subscribeSiteEvents structure to be unnecessarily difficult to reason about (and I stumbled on it again while tracing the screenshot generation logic in this PR). My change simply moves the daemon bus event listeners to this file.
| console.error( '[Proxy Error]', err.message ); | ||
| if ( res && res instanceof http.ServerResponse ) { | ||
| res.writeHead( 500, { 'Content-Type': 'text/plain' } ); | ||
| res.writeHead( 502, { 'Content-Type': 'text/plain' } ); |
There was a problem hiding this comment.
Not important to the core purpose of this PR, but 502 Bad Gateway better conveys the specific type of error.
| }; | ||
| }, | ||
| { concurrent: 3, max: 100, deduplicateKey: ( args: string[] ) => args.join( ' ' ) } | ||
| { concurrent: 3, max: 100, deduplicateKey: ( args ) => args.join( ' ' ) } |
There was a problem hiding this comment.
The sequential type definition was giving me grief. This is a leftover from when I was battling that for a while. It makes sense to remove the type since TS infers it from the type definitions of the arguments to the sequential callback.
| throw new Error( 'Site not found.' ); | ||
| } | ||
|
|
||
| void captureSiteThumbnail( id, emitLoadingEvent ); |
There was a problem hiding this comment.
I made it so captureSiteThumbnail is triggered earlier because server.getThemeDetails() calls WP-CLI. When WP-CLI returns a response, the CLI generates a SITE_EVENT.UPDATED event to Studio, which triggers another captureSiteThumbnail call.
In the end, I made it so neither of the captureSiteThumbnail calls triggers a thumbnail-loading event, but previously, the order in which things were called triggered two such events.
| // captures across all sites (one at a time). | ||
| export const captureSiteThumbnail = sequential( | ||
| async ( id: string ): Promise< void > => { | ||
| export const captureSiteThumbnail = sequential< [ string, boolean? ], void >( |
There was a problem hiding this comment.
This type definition is important because the type inference in sequential.ts cannot handle a callback with two arguments (where the second one is optional) and a deduplicateKey function that takes just a single argument.
There was a problem hiding this comment.
Again, sites without custom URLs will encounter network errors here because no server is listening on the target port. For sites with custom URLs, there's always the custom domain proxy listening on port 80/443, which is why we need the HTTP status code check as an additional safety measure.
2 participants
Related issues
How AI was used in this PR
For small and directed tasks.
Proposed Changes
emitLoadingEventargument tocaptureSiteThumbnailthat suppressesthumbnail-loadingevents. This argument is true when the screenshot is updated in response to aSITE_EVENTS.UPDATEDevent. It's also inherited from the corresponding argument inloadThemeDetails.waitForCapturecallback fromcreateScreenshotWindowthrow if the page returns an HTTP status code >= 500. For sites without custom domains, we get similar behavior for free, since the most common error is thatwindow.loadURLfails because no server is listening on the port. For custom domains, that never happens because the custom domain proxy always listens on ports 80/443.Testing Instructions
Proxy errorscreenshot is captured.Pre-merge Checklist