-
Notifications
You must be signed in to change notification settings - Fork 54
Sync: Fix renderer crash when network disconnects after upload #2334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sync: Fix renderer crash when network disconnects after upload #2334
Conversation
📊 Performance Test ResultsComparing a1359a0 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
| } | ||
| ); | ||
| } catch ( error ) { | ||
| // Network errors (e.g., offline) should be handled gracefully |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Network errors (e.g., offline) should be handled gracefully
I am not sure we need to include this comment though. It seems clear from the code itself. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point. It makes sense to remove it, as the code itself is quite explanatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in a1359a0
| } catch ( error ) { | ||
| // Network errors (e.g., offline) should be handled gracefully | ||
| // The error will be handled when the user comes back online | ||
| console.error( 'Failed to get push progress:', error ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the process of throwing the console.error in this case? What use would it have for the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU, the import status is pooled at intervals, but it continues to do so even when the network goes offline. On the trunk, the renderer crashes due to the network being offline. This is the expected behavior; however, the API itself might return an error from the server. I thought it would be useful to log this for debugging purposes, especially to handle any unknown cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in a1359a0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for explaining!
I am wondering if we should just skip logging it to the console to not pollute the console with something that we know will cause the error e.g. network interruption. What do you think?
Not a strong preference, just a suggestion on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion Kate. I have removed logging after you suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
katinthehatsite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the GH approved the changes but I was not intending to approve them just yet, sorry about that
- Add error handling in getPushProgressInfo to catch network errors gracefully - Skip Sentry reporting for expected crossDomain network errors - Prevent unhandled promise rejections when network goes offline Fixes STU-1180
Excellent observation. I added the offline indicator in #2212, but it only appears during the upload phase and when the network goes offline. I will create a follow-up issue to address this. |
|
@katinthehatsite I have created STU-1195 linear issue to improve offline statuses during different phases of push. |
Nice sounds good to me 👍 |
3904760 to
a1359a0
Compare
@katinthehatsite Do you mind giving another review? |
katinthehatsite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on my end 👍 Thanks for implementing the adjustments!

Related issues
Proposed Changes
getPushProgressInfoto catch network errors gracefullycrossDomainnetwork errors (offline scenarios)Testing Instructions
Pre-merge Checklist