Skip to content
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

fix(app): fix browser layer notification error handling #16207

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Sep 6, 2024

Closes RQA-3108 (for the third time)

Overview

Fixes an issue in which browser layer error messages were dropped. The app shell sends MQTT error event messages on the ALL_TOPICS topic, but browser layer callbacks do not ever subscribe to the ALL_TOPICS topic, just their individual topic. This means when the app receives a disconnect event that is applicable to all topics, those topics do not default back to polling.

We know this fixes the ticket (all of the prior fixes are valid for this specific issue, too!), because you can see the error message at 2024-09-05T18:57:34.783Z in the Windows App Logs (attached in the ticket). The app, however, never defaults to polling, as evidence by the lack of network requests in the server logs after this time (only the ODD network requests show up).

Test Plan and Hands on Testing

CURRENT BEHAVIOR REPRO

  • On the built version of the desktop app (or any other branch -- much easier to do this on the desktop than the ODD), run a protocol with a manual move, such as this one.
  • Once at the manual move step, SSH into the robot and do a systemctl stop mosquitto.
  • Verify in the app-shell that the Error - Error: read ECONNRESET message appears.
  • Click continue on the manual move modal.
  • You'll see the modal never dissapears.

PR FIX VALDIATION

  • After switching to this branch, FULLY close out of the app and re-open(need to do this based on how notification connectivity works).
  • Perform the above sequence of steps.
  • After clicking continue, you should see the modal disappear!

Changelog

  • Fixed the desktop/app not falling back to polling in some spots.

Risk assessment

  • lowish. It's networking, which is always scary, but the fix is quite localized.

@mjhuff mjhuff requested review from sfoster1 and shlokamin September 6, 2024 16:24
@mjhuff mjhuff requested a review from a team as a code owner September 6, 2024 16:24
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Makes sense to me!

@mjhuff mjhuff merged commit 6948d93 into chore_release-8.0.0 Sep 6, 2024
20 checks passed
@mjhuff mjhuff deleted the app_fix-notify-error-handling-browser branch September 6, 2024 17:00
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.

2 participants