Skip to content

fix(app,components): Fix unreliable MoveLabwareOnDeck animation #19179

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

Open
wants to merge 2 commits into
base: chore_release-8.6.0
Choose a base branch
from

Conversation

SyntaxColoring
Copy link
Contributor

Overview

Fixes EXEC-1642 / RQA-4511? Maybe?

Test Plan and Hands on Testing

Manually play with the move-labware animations. One place where these animations happen is in manual move-labware commands, and the other place seems to be somewhere in error recovery—not sure exactly where.

Here's the protocol I used, though there's probably value in testing with more variety, if you have a different one handy.

from opentrons import protocol_api

requirements = {
    "apiLevel": "2.23",
    "robotType": "Flex",
}

def run(protocol: protocol_api.ProtocolContext):
    slot = "C2"
    labware = protocol.load_labware("agilent_1_reservoir_290ml", slot)

    module = protocol.load_module("thermocyclerModuleV2")
    module.open_lid()

    protocol.move_labware(labware, module)
    protocol.move_labware(labware, protocol_api.OFF_DECK)
  • Animation loops properly
  • To/from locations are correct between the two instances of the animation; back-to-back animations from separate commands don't "bleed into" each other
Screen.Recording.2025-08-08.at.6.10.15.PM.mov

Changelog

Instead of passing in a straight object to useSpring(), pass in a callback and a dependency array. The idea is that maybe we were spamming react-spring with too many prop changes.

And a completely unrelated thing. The animation has a few steps for animating the "splash" around the labware. For labware being moved to/from off-deck, this created a weird (in my opinion) pause where it looked like nothing was happening, because it was happening off-deck where you couldn't see it. So, now, conditionally skip those animation steps when we know they're not going to be visible.

I can delete the splash change if we don't think it's worth the complexity or if it looks weird.

Review requests

react-spring's rerendering and prop-updating semantics seem completely undocumented, so this is all guesswork on my part. Should we have to do this? Is this the real problem? The answers are unknowable and the questions will lead me to madness. I am at peace with these dreadful mysteries, and I hope that you feel the same way.

Risk assessment

🤷

@SyntaxColoring SyntaxColoring requested review from mjhuff and a team August 8, 2025 22:14
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner August 8, 2025 22:14
Copy link

codecov bot commented Aug 8, 2025

Codecov Report

❌ Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.19%. Comparing base (95b9d6e) to head (014e9dc).
⚠️ Report is 2 commits behind head on chore_release-8.6.0.

Files with missing lines Patch % Lines
...onents/src/hardware-sim/Deck/MoveLabwareOnDeck.tsx 0.00% 32 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-8.6.0   #19179      +/-   ##
=======================================================
+ Coverage                24.00%   24.19%   +0.18%     
=======================================================
  Files                     3377     3377              
  Lines                   296968   297050      +82     
  Branches                 31523    31543      +20     
=======================================================
+ Hits                     71301    71870     +569     
+ Misses                  225649   225157     -492     
- Partials                    18       23       +5     
Flag Coverage Δ
app 2.32% <0.00%> (+0.23%) ⬆️
protocol-designer 18.91% <0.00%> (-0.02%) ⬇️

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

Files with missing lines Coverage Δ
...onents/src/hardware-sim/Deck/MoveLabwareOnDeck.tsx 3.70% <0.00%> (-0.99%) ⬇️

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@SyntaxColoring SyntaxColoring force-pushed the move_labware_animation_fixes branch from 001458b to 014e9dc Compare August 8, 2025 23:43
@SyntaxColoring
Copy link
Contributor Author

This can go into either edge or chore_release-8.6.0 depending on how risky we feel it is.

@SyntaxColoring SyntaxColoring changed the base branch from edge to chore_release-8.6.0 August 8, 2025 23:44
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.

1 participant