Skip to content

Sync: Fix renderer crash when network disconnects after upload - #2334

Merged
gavande1 merged 2 commits into
trunkfrom
stu-1180-fix-renderer-crash-when-disconnect-occurs-after-upload
Jan 8, 2026
Merged

Sync: Fix renderer crash when network disconnects after upload#2334
gavande1 merged 2 commits into
trunkfrom
stu-1180-fix-renderer-crash-when-disconnect-occurs-after-upload

Conversation

@gavande1

@gavande1 gavande1 commented Dec 30, 2025

Copy link
Copy Markdown
Contributor

Related issues

Proposed Changes

  • Added error handling in getPushProgressInfo to catch network errors gracefully
  • Skip Sentry reporting for expected crossDomain network errors (offline scenarios)
  • Prevent unhandled promise rejections when network goes offline

Testing Instructions

  1. Start a new sync operation
  2. Allow the upload step to complete successfully
  3. Disconnect the network (e.g., turn off Wi‑Fi) during subsequent sync steps
  4. Verify the renderer no longer crashes and handles the offline state gracefully
  5. Verify that when the network reconnects, the sync operation can continue

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@gavande1 gavande1 self-assigned this Dec 30, 2025
@gavande1 gavande1 changed the title Fix renderer crash when network disconnects after upload (STU-1180) Dec 30, 2025
@gavande1
gavande1 requested a review from a team December 30, 2025 07:52
@github-actions

github-actions Bot commented Dec 30, 2025

Copy link
Copy Markdown
Contributor

📊 Performance Test Results

Comparing a1359a0 vs trunk

site-editor

Metric trunk a1359a0 Diff Change
load 8244.00 ms 7315.00 ms -929.00 ms 🟢 -11.3%

site-startup

Metric trunk a1359a0 Diff Change
siteCreation 9065.00 ms 9067.00 ms +2.00 ms 🔴 0.0%
siteStartup 3959.00 ms 3954.00 ms -5.00 ms 🟢 -0.1%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change

Comment thread src/hooks/sync-sites/use-sync-push.ts Outdated
}
);
} catch ( error ) {
// Network errors (e.g., offline) should be handled gracefully

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.

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?

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.

That is a good point. It makes sense to remove it, as the code itself is quite explanatory.

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.

Removed in a1359a0

Comment thread src/hooks/sync-sites/use-sync-push.ts Outdated
} 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 );

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.

What is the process of throwing the console.error in this case? What use would it have for the user?

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.

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.

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.

Removed in a1359a0

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.

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.

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.

Thanks for the suggestion Kate. I have removed logging after you suggestion.

@katinthehatsite katinthehatsite 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.

It seems it works as described but when I was testing this, the sync progress did not indicate to me in any way that I was offline. Perhaps we could consider improving this area to be more indicative that the user is disconnected:

Image

What do you think?

@katinthehatsite katinthehatsite 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.

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
@gavande1

gavande1 commented Jan 6, 2026

Copy link
Copy Markdown
Contributor Author

It seems it works as described but when I was testing this, the sync progress did not indicate to me in any way that I was offline. Perhaps we could consider improving this area to be more indicative that the user is disconnected:

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.

@gavande1

gavande1 commented Jan 6, 2026

Copy link
Copy Markdown
Contributor Author

@katinthehatsite I have created STU-1195 linear issue to improve offline statuses during different phases of push.

@katinthehatsite

Copy link
Copy Markdown
Contributor

@katinthehatsite I have created STU-1195 linear issue to improve offline statuses during different phases of push.

Nice sounds good to me 👍

@gavande1
gavande1 force-pushed the stu-1180-fix-renderer-crash-when-disconnect-occurs-after-upload branch from 3904760 to a1359a0 Compare January 6, 2026 09:09
@gavande1

gavande1 commented Jan 7, 2026

Copy link
Copy Markdown
Contributor Author

It seems that the GH approved the changes but I was not intending to approve them just yet, sorry about that

@katinthehatsite Do you mind giving another review?

@gavande1
gavande1 requested a review from a team January 8, 2026 10:29

@katinthehatsite katinthehatsite 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.

Looks good on my end 👍 Thanks for implementing the adjustments!

@gavande1
gavande1 merged commit c960fe3 into trunk Jan 8, 2026
13 checks passed
@gavande1
gavande1 deleted the stu-1180-fix-renderer-crash-when-disconnect-occurs-after-upload branch January 8, 2026 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants