Skip to content

Conversation

@microbit-grace
Copy link

@microbit-grace microbit-grace commented Nov 5, 2024

  • Handle tab visibility in getNextConnectionState as Rob suggested and add logic to show radio bridge error dialog if tab is visible, otherwise show connection status as reconnecting automatically
  • Consider failing twice state as a connection start over and reset onFirstConnectAttempt and hasAttemptedReconnect

Private link to related issues:

@github-actions
Copy link

github-actions bot commented Nov 5, 2024

Preview build will be at
https://review-createai.microbit.org/fix-conn-status/

@microbit-grace microbit-grace changed the title [WIP - DO NOT MERGE] Minimise losing out on connection status updates when tab is not visible Minimise losing out on connection status updates when tab is not visible Nov 6, 2024
@microbit-grace microbit-grace marked this pull request as ready for review November 6, 2024 09:22
@microbit-grace microbit-grace requested a review from a team November 6, 2024 09:22
@microbit-matt-hillsdon
Copy link

I find this change hard to understand. Can you write a bit more explanation (on the PR in the first instance) ?

Do you think there's something different we could do in the connection library to simplify the code here / remove special cases?

@microbit-grace
Copy link
Author

I find this change hard to understand. Can you write a bit more explanation (on the PR in the first instance) ?

Have added more explanation in the PR description. Hope that helps!

Do you think there's something different we could do in the connection library to simplify the code here / remove special cases?

Possibly? One way I can think of is if we introduced new connection status(es) that can distinguish between reason behind different disconnected states maybe (e.g. disconnected explicitly by user, connection loss, connect error, tab switching)? But on the other hand, I'm not sure if the connection library should be aware of these reasons if it was a general-use library

@microbit-grace
Copy link
Author

Possibly can handle another tab visibility issue as well (private link to issue: https://microbit-global.monday.com/boards/1550536443/pulses/1696956864)

@microbit-grace microbit-grace removed the request for review from a team November 8, 2024 09:56
@microbit-grace microbit-grace marked this pull request as draft November 8, 2024 09:56
@microbit-grace microbit-grace changed the title Minimise losing out on connection status updates when tab is not visible [WIP - DO NOT MERGE] Minimise losing out on connection status updates when tab is not visible Nov 8, 2024
// If bridge micro:bit causes radio bridge reconnect to fail twice
hasAttempedReconnect
) {
setHasAttemptedReconnect(false);
Copy link
Author

@microbit-grace microbit-grace Nov 11, 2024

Choose a reason for hiding this comment

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

Letting hasStartedOver logic handle the resetting of this instead for when reconnect fail twice. Also, important to setOnFirstConnectAttempt(true) for attempts after reconnect fail twice to avoid reconnect fail twice error dialog from being triggered again in subsequent start over connection attempt errors

@microbit-grace microbit-grace changed the base branch from main to monday November 11, 2024 16:14
@microbit-grace microbit-grace changed the title [WIP - DO NOT MERGE] Minimise losing out on connection status updates when tab is not visible Minimise losing out on connection status updates when tab is not visible Nov 11, 2024
@microbit-grace microbit-grace changed the title Minimise losing out on connection status updates when tab is not visible Handling tab visibility in getNextConnectionState Nov 11, 2024
@microbit-grace microbit-grace marked this pull request as ready for review November 11, 2024 16:15
Base automatically changed from monday to main November 11, 2024 19:36
@microbit-matt-hillsdon microbit-matt-hillsdon merged commit 075a420 into main Nov 12, 2024
1 check passed
@microbit-matt-hillsdon microbit-matt-hillsdon deleted the fix-conn-status branch November 12, 2024 10:31
@microbit-matt-hillsdon
Copy link

Thanks for working through this one.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants