Skip to content

Improvements to site thumbnail generation - #1081

Merged
fredrikekelund merged 4 commits into
trunkfrom
f26d/site-thumbnail-improvements
Mar 18, 2025
Merged

Improvements to site thumbnail generation#1081
fredrikekelund merged 4 commits into
trunkfrom
f26d/site-thumbnail-improvements

Conversation

@fredrikekelund

@fredrikekelund fredrikekelund commented Mar 17, 2025

Copy link
Copy Markdown
Contributor

Related issues

Proposed Changes

  • Despite my best efforts, I couldn't come up with a better solution for the Windows thumbnail bug than simply increasing the hard-coded timeout in waitForCapture.
  • I also changed the logic so a new thumbnail is not generated (and the old one is removed) when the home page responds with an HTTP error status code. This can happen if port 80 is already busy, for example.

QUESTION: I'm open to suggestions for which behavior to implement when the site home page returns an error. I think generating a thumbnail makes for weird behavior, but maybe it's better to leave the old thumbnail in place, I'm not sure…

Testing Instructions

  1. Ping me on Slack to get a site backup for reproducing this error
  2. On Windows, create a Studio site from that backup
  3. Stop and start the site a few times
  4. Ensure that the site thumbnail appears correctly with each restart
  5. Quit Studio
  6. Have Local installed and start a site with a custom domain
  7. Start Studio
  8. Start the Studio site
  9. Ensure that the site thumbnail displays No preview available

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@fredrikekelund
fredrikekelund requested a review from a team March 17, 2025 15:58
@fredrikekelund fredrikekelund self-assigned this Mar 17, 2025
Comment thread src/screenshot-window.ts
Comment on lines -15 to -17
const finishedLoading = new Promise< void >( ( resolve ) => {
window.webContents.on( 'did-finish-load', () => resolve() );
} );

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

window.loadURL returns a promise that resolves when the did-finish-load event has fired, so I took the opportunity to simplify this logic.

Comment thread src/screenshot-window.ts
window.webContents.on( 'did-finish-load', () => resolve() );
} );
const responseStatusCodePromise = new Promise< void >( ( resolve, reject ) => {
newSession.webRequest.onCompleted( ( details ) => {

@fredrikekelund fredrikekelund Mar 17, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a URL filter (docs) did not work well in my testing. Likely because of potential redirects.

Comment thread src/site-server.ts
.catch( Sentry.captureException )
.catch( async ( error ) => {
Sentry.captureException( error );
await fs.promises.unlink( outPath );

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting the thumbnail path makes the UI look like this:

Screenshot 2025-03-17 at 17 00 02

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is expected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's intentional for the thumbnail to look this way when there's no image. My point was that deleting the thumbnail when the homepage returns an errant HTTP status is new behavior, so I wanted to clarify what the UI will look like in those cases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying that, makes sense.

@wojtekn wojtekn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well on Windows now. MacOS still works fine.

Comment thread src/site-server.ts
.catch( Sentry.captureException )
.catch( async ( error ) => {
Sentry.captureException( error );
await fs.promises.unlink( outPath );

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is expected.

@fredrikekelund
fredrikekelund merged commit 5c75b7b into trunk Mar 18, 2025
@fredrikekelund
fredrikekelund deleted the f26d/site-thumbnail-improvements branch March 18, 2025 13:33
bgrgicak pushed a commit that referenced this pull request Mar 20, 2025
* Throw error when thumbnail page responds with error status

* Tweaks

* Only increase timeout on Windows

* Promise
@sentry

sentry Bot commented Mar 28, 2025

Copy link
Copy Markdown

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: Page returned status code: 500 Function.<anonymous>(main/studio/./src/screensh... View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants