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, app-shell, app-shell-odd): Fix host context for notifications #14548

Merged
merged 4 commits into from
Feb 26, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Feb 26, 2024

Closes RAUT-1018

Overview

This PR contains a few fixes for a few spots in the app that did not properly pass a host object to the notification utility, therefore causing the app to use polling instead of MQTT.

Also, this updates MQTT analytics reporting - it's necessary to only report Flexes, because currently all OT-2s will report that MQTT is blocked (there's no MQTT on OT-2s). Filtering out false positives for the 7.2 release would be nice!

Test Plan

  • Verified that maintenance runs flows are still accessible on the ODD under the InstrumentDetails page (and utilizing MQTT).
  • Verified slideouts use MQTT as well when possible.
  • Verified via Mixpanel that OT-2s are no longer reporting.
  • Verified that an annoying shell message no longer displays.

Changelog

Risk assessment

low

Adds the host context provider to a couple missing react nice modal components. Because react nice
modal exists on a layer outside of the scope of the host context intentionally, we need to provide
it manually when it is needed.
There are a few spots (slideout cards) that utilize the notification service but don't have a host
object associated with them. In these spots, we pass a host object manually via a host override
parameter. Host override was not passed to the notification service, which means that slideout cards
did not properly show available robots.
Because OT-2s won't support MQTT for the 7.2 release, mixpanel will report a lot of false positive
port block events unless we filter out OT-2s. Let's only send report data for the Flex for now.
@mjhuff mjhuff requested a review from a team February 26, 2024 14:36
@mjhuff mjhuff requested review from a team as code owners February 26, 2024 14:36
@mjhuff mjhuff requested review from brenthagen and removed request for a team February 26, 2024 14:36
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 67.74%. Comparing base (926536a) to head (c81a6ee).

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.2.0   #14548      +/-   ##
=======================================================
- Coverage                67.74%   67.74%   -0.01%     
=======================================================
  Files                     2517     2517              
  Lines                    72085    72090       +5     
  Branches                  9282     9286       +4     
=======================================================
+ Hits                     48834    48835       +1     
- Misses                   21033    21035       +2     
- Partials                  2218     2220       +2     
Flag Coverage Δ
app 64.63% <55.55%> (-0.02%) ⬇️

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

Files Coverage Δ
.../InstrumentDetail/InstrumentDetailOverflowMenu.tsx 84.61% <100.00%> (+0.61%) ⬆️
app/src/pages/RunSummary/index.tsx 9.41% <ø> (ø)
app/src/resources/runs/useNotifyAllRunsQuery.ts 81.81% <ø> (ø)
app/src/resources/useNotifyService.ts 85.71% <100.00%> (+1.09%) ⬆️
.../src/organisms/DropTipWizard/TipsAttachedModal.tsx 73.91% <50.00%> (-3.36%) ⬇️
app/src/pages/InstrumentDetail/index.tsx 81.81% <50.00%> (+1.81%) ⬆️
app-shell-odd/src/notify.ts 0.00% <0.00%> (ø)
app-shell/src/notify.ts 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

@mjhuff mjhuff merged commit 5172fc1 into chore_release-7.2.0 Feb 26, 2024
28 of 30 checks passed
@mjhuff mjhuff deleted the app_fix-host-context-for-notifications branch February 26, 2024 15:33
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