feat(app): Add incompatible module notification modals #15181

sfoster1 · 2024-05-14T18:57:40Z

We meant to add this for the launch of the Flex, but it got cut for scope; in the time since, people connecting modules like first-generation temperature modules to Flexes that don't support them has been a pretty consistent thorn in the side of support. This PR implements the modals designed by design for telling the user when there's incompatible modules.

This PR is pretty big and might be best viewed by commit, since it has both the backend and the frontend, and both the ODD and the desktop - and a couple ancillary support PRs that I'll rebase out of here as they get merged. Specifically,

Overview

90dc748 - Python, adds data to module configs to say what kinds of deck they're compatible with, and adds code to the api and robot server to expose a bool about whether the pipette's compatible or not in the HTTP API
788e022 - JS, adds the above to the api clients
2e49f61 - JS, adds a blocking modal to the ODD that pops up whenever modules are attached per figma
c5021ca - JS, adds a modal that blocks interaction with a robot but not the navbar or breadcrumbs whenever modules are attached per figma

Notes and Review requests

90dc748 - I'm pretty sure these are the module compatibility rules, but we'll need to make sure. This will get exposed via HTTP as soon as the module is plugged in. Review requests:

bool should be correct for plugged-in modules basically as soon as they appear in the app

The above, and 788e022 - This is set up to support polling. Let me know if you think this should be notifications instead, will be another couple commits though.

2e49f61 - ODD modal. Is this according to design, is there something I'm missing? Is the testing approach OK? You can test by editing robot-server/simulator/test-flex.env to specify a ThermocyclerModuleV1 instead of a ThermocyclerModuleV2 and run with make -C robot-server dev-flex, as well as actually plugging stuff in to a real robot.

c5021ca - Desktop modal, basically same questions and same testing approach. Note that this is the first ish time we've added a modal that doesn't inhibit interaction with navbar or breadcrumbs.

TamarZanzouri · 2024-05-15T14:48:32Z

robot-server/robot_server/modules/module_data_mapper.py

@@ -131,6 +140,9 @@ def map_data(
            firmwareVersion=module_identity.firmware_version,
            hardwareRevision=module_identity.hardware_revision,
            hasAvailableUpdate=has_available_update,
+            compatibleWithRobot=(
+                not (self.deck_type.value in module_definition["incompatibleWithDecks"])


not a big deal but why not compatibleWithDecks instead of incompatibleWithDecks? and lose the not? this way it matches compatibleWith

@sfoster1 answered privately, this way we dont need to update for every deck we add.

TamarZanzouri

Backend changes look good! glanced on fe changes but someone else can probably confirm better than me :-)

The robot server's /modules endpoint now has a compatibleWithRobot field that shows whether the connected module is considered compatible with the robot it's running on. That compatibility determination is based on the deck name for now (module model definitions list what deck names they're compatible with) though we might want to change it to the reverse, listing in the deck configuration which modules are compatible with this deck. Closes RSQ-6

This is a FE middleware side of adding module compatibility - we have to know about it.

This is a blocking modal takeover that pops when you have incompatible modules (as determined by the machine) connected to your flex, and goes away when you remove them. This also adds a test id to the modal and app root elements (it's just the id again) because testing-library/react doesn't offer a way to search just by id, and it's nice to be able to test that your component is hanging off the right root. Closes RSQ-6

feat(app): IncompatibleModules on desktop Display a modal on the desktop app if incompatible modules are attached to the robot you're currently viewing. This modal allows interaction with the navbar but otherwise is displayed at any time that the app is viewing a specific robot (i.e. prepare for run, device details) and that robot has incompatible modules attached.

mjhuff

This looks good! Just a few nits.

Both of the modals and their respective testing lgtm.

It would be good to support module incompatibility notifications at some point, but I think that would be more involved than we probably think, and the poll interval is reasonable.

Thank you for tackling all of this!

mjhuff · 2024-05-21T12:55:57Z

app/src/organisms/IncompatibleModule/IncompatibleModuleDesktopModalBody.tsx

+          maxHeight="196px"
+          as="ul"
+        >
+          {...modules.map(module => (


I believe we don't need to spread here.

mjhuff · 2024-05-21T13:08:14Z

app/src/organisms/IncompatibleModule/IncompatibleModuleDesktopModalBody.tsx

+          as="ul"
+        >
+          {...modules.map(module => (
+            <li key={module.id}>


Kind of surprised we don't have a component wrapper for li, but the pattern we use in these modal + list spots is to nix the li and use Flex as the top level component. Using li here (or a component that wraps it) is better practice, though, so I think we should keep it.

discussed in person, keeping these

mjhuff · 2024-05-21T13:40:36Z

app/src/organisms/IncompatibleModule/IncompatibleModuleODDModalBody.tsx

+          gridGap={SPACING.spacing8}
+          maxHeight="196px"
+        >
+          {...modules.map(module => (


mjhuff · 2024-05-21T13:42:10Z

app/src/organisms/IncompatibleModule/IncompatibleModuleTakeover.tsx

+      {incompatibleModules.length !== 0
+        ? isOnDevice
+          ? createPortal(
+              <IncompatibleModuleODDModalBody modules={incompatibleModules} />,
+              getTopPortalEl()
+            )
+          : createPortal(
+              <IncompatibleModuleDesktopModalBody
+                modules={incompatibleModules}
+                robotName={robotName ?? ''}
+              />,
+              getModalPortalEl()
+            )
+        : null}
+    </>
+  )


Sweet, the logic here makes sense. Can we avoid the nested ternaries here? It's a bit hard to read.

We meant to add this for the launch of the Flex, but it got cut for scope; in the time since, people connecting modules like first-generation temperature modules to Flexes that don't support them has been a pretty consistent thorn in the side of support. This PR implements the modals designed by design for telling the user when there's incompatible modules. This PR is pretty big and might be best viewed by commit, since it has both the backend and the frontend, and both the ODD and the desktop - and a couple ancillary support PRs that I'll rebase out of here as they get merged. Specifically, ## Overview 90dc748 - Python, adds data to module configs to say what kinds of deck they're compatible with, and adds code to the api and robot server to expose a bool about whether the pipette's compatible or not in the HTTP API 788e022 - JS, adds the above to the api clients 2e49f61 - JS, adds a blocking modal to the ODD that pops up whenever modules are attached per [figma](https://www.figma.com/file/0kuPeCi1t2Auu2GPMnqRb2/Primary%3A-Flex?type=design&node-id=19961-155919&mode=design&t=pkbLPA9150bwQQNw-4) c5021ca - JS, adds a modal that blocks interaction with a robot but _not_ the navbar or breadcrumbs whenever modules are attached per [figma](https://www.figma.com/design/Nx7ORMnfyJNP4FQYxN4e5x/Primary%3A-Desktop-App?node-id=2032-304206&t=iat6IH7YQD4pMijc-4) ## Notes and Review requests 90dc748 - I'm pretty sure these are the module compatibility rules, but we'll need to make sure. This will get exposed via HTTP as soon as the module is plugged in. Review requests: - [ ] bool should be correct for plugged-in modules basically as soon as they appear in the app The above, and 788e022 - This is set up to support polling. Let me know if you think this should be notifications instead, will be another couple commits though. 2e49f61 - ODD modal. Is this according to design, is there something I'm missing? Is the testing approach OK? You can test by editing `robot-server/simulator/test-flex.env` to specify a `ThermocyclerModuleV1` instead of a `ThermocyclerModuleV2` and run with `make -C robot-server dev-flex`, as well as actually plugging stuff in to a real robot. c5021ca - Desktop modal, basically same questions and same testing approach. Note that this is the first ish time we've added a modal that doesn't inhibit interaction with navbar or breadcrumbs.

sfoster1 requested review from a team as code owners May 14, 2024 18:57

sfoster1 requested review from shlokamin, SyntaxColoring, TamarZanzouri, ryanthecoder, mjhuff and caila-marashaj and removed request for a team May 14, 2024 18:57

TamarZanzouri reviewed May 15, 2024

View reviewed changes

TamarZanzouri approved these changes May 15, 2024

View reviewed changes

sfoster1 added 4 commits May 17, 2024 13:46

chore(api-client): add module compatibility returns

145ea3c

This is a FE middleware side of adding module compatibility - we have to know about it.

sfoster1 force-pushed the rsq-6-incompatible-modules-backend branch from c5021ca to ab2df12 Compare May 17, 2024 17:46

fix: add incompatibleWithDeck to absorbanceReader

5ea8e9b

mjhuff approved these changes May 21, 2024

View reviewed changes

sfoster1 added 3 commits May 21, 2024 13:27

de-ternary

da383e7

de-spread

ca080dd

types update

1729b1f

sfoster1 merged commit 99df21c into edge May 21, 2024
42 checks passed

sfoster1 deleted the rsq-6-incompatible-modules-backend branch May 21, 2024 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app): Add incompatible module notification modals #15181

feat(app): Add incompatible module notification modals #15181

sfoster1 commented May 14, 2024 •

edited

TamarZanzouri May 15, 2024

TamarZanzouri May 15, 2024

TamarZanzouri left a comment

mjhuff left a comment

mjhuff May 21, 2024

mjhuff May 21, 2024 •

edited

sfoster1 May 21, 2024

mjhuff May 21, 2024

mjhuff May 21, 2024

feat(app): Add incompatible module notification modals #15181

feat(app): Add incompatible module notification modals #15181

Conversation

sfoster1 commented May 14, 2024 • edited

Overview

Notes and Review requests

TamarZanzouri May 15, 2024

Choose a reason for hiding this comment

TamarZanzouri May 15, 2024

Choose a reason for hiding this comment

TamarZanzouri left a comment

Choose a reason for hiding this comment

mjhuff left a comment

Choose a reason for hiding this comment

mjhuff May 21, 2024

Choose a reason for hiding this comment

mjhuff May 21, 2024 • edited

Choose a reason for hiding this comment

sfoster1 May 21, 2024

Choose a reason for hiding this comment

mjhuff May 21, 2024

Choose a reason for hiding this comment

mjhuff May 21, 2024

Choose a reason for hiding this comment

sfoster1 commented May 14, 2024 •

edited

mjhuff May 21, 2024 •

edited