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-shell, app-shell-odd): Prevent excessive notify logs #14582

Merged
merged 6 commits into from
Mar 1, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Mar 1, 2024

Closes RQA-2431

Overview

Because subscription logic is handled by components currently, logs are excessive in some spots, especially on the OT-2. Let's comment out the logs until we refactor subscription logic to be handled directly by the robot-server (next release). Note that there aren't any excessive network requests - it's just logs.

Test Plan

  • Start a dev build and look at the console.
  • Navigate to an OT-2 and start a pipette flow, run a protocol, etc.
  • Verify that there are no [notify] logs from the app-shell.

Changelog

  • Removed spammy notification logs.

Risk assessment

low

Because subscription logic is handled by components currently, logs are excessive in some spots,
especially on the OT-2. Let's comment out the logs until we refactor subscription logic to be
handled directly by the robot-server.
…ice is a Flex

The app-shell/app-shell-odd properly handles when the robot should (or should not) connect, however,
we already use isFlex within the app, so we might as well save OT-2s the trouble of talking to the
app-shell. This won't be any extra effort to refactor once OT-2s support MQTT.
@mjhuff mjhuff requested a review from a team March 1, 2024 20:59
@mjhuff mjhuff requested review from a team as code owners March 1, 2024 20:59
@mjhuff mjhuff requested review from koji and removed request for a team March 1, 2024 20:59
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.57%. Comparing base (812e799) to head (760926f).
Report is 4 commits behind head on chore_release-7.2.0.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.2.0   #14582      +/-   ##
=======================================================
- Coverage                67.60%   67.57%   -0.03%     
=======================================================
  Files                     2521     2521              
  Lines                    72212    72252      +40     
  Branches                  9289     9322      +33     
=======================================================
+ Hits                     48818    48824       +6     
- Misses                   21204    21238      +34     
  Partials                  2190     2190              
Flag Coverage Δ
app 63.92% <ø> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
app-shell-odd/src/notify.ts 0.00% <ø> (ø)
app-shell/src/notify.ts 0.00% <ø> (ø)

... and 2 files with indirect coverage changes

@ncdiehl11
Copy link
Collaborator

LGTM

@mjhuff mjhuff merged commit d44c529 into chore_release-7.2.0 Mar 1, 2024
22 checks passed
@mjhuff mjhuff deleted the app_prevent-excessive-notify-logs branch March 1, 2024 22:43
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