Skip to content

Report WebSocket connection termination error code and message #156

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

Merged
merged 1 commit into from
Mar 25, 2025

Conversation

vzaidman
Copy link

@vzaidman vzaidman commented Mar 24, 2025

Summary

When WebSocket disconnects, we are reporting an event, but we were not reporting the disconnection code / error type that could tell us why disconnections occur on Frontend.

Test

Tested reporting the event e2e: D71742575

@vzaidman vzaidman force-pushed the websocket-connection-lost-error-reporting branch 2 times, most recently from a3808f9 to 8918992 Compare March 24, 2025 17:51
@vzaidman vzaidman requested review from hezi, huntie and hoxyq March 25, 2025 10:31
static show(reason: string): void {
static show(
uiMessage: string,
connectionLostDetails: {reason: string, code: string, errorType: string} = {reason: '', code: '', errorType: ''}
Copy link

Choose a reason for hiding this comment

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

Does it have to be initialized with empty strings? I thought reason is always present, even if it is just empty string. Other fields may be optional, but do we actually need to set them to empty strings? I think you can have optional fields in event payloads in RNPerfMetrics

Copy link
Author

@vzaidman vzaidman Mar 25, 2025

Choose a reason for hiding this comment

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

Thanks!
You are right. I've made all these fields optional here and in the corresponding diff D71821154.

@vzaidman vzaidman force-pushed the websocket-connection-lost-error-reporting branch from 8918992 to 11843fb Compare March 25, 2025 16:46
@vzaidman vzaidman requested a review from hoxyq March 25, 2025 16:51
@vzaidman vzaidman merged commit 26e19a4 into main Mar 25, 2025
2 of 3 checks passed
@huntie huntie mentioned this pull request May 9, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants