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/debug time slider #529

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fix/debug time slider #529

wants to merge 2 commits into from

Conversation

ascibisz
Copy link
Contributor

@ascibisz ascibisz commented Jun 10, 2024

Time estimate or Size

small

Problem

There are a variety of issues with the time slider in Simularium right now, so here's my stab at trying to fix it!

Some of the issues are described in this ticket, which links to this spreadsheet from the octopus bug bash.

Solution

So I did two changes to address some of the issues we're seeing on the time slider, but I'm open to suggestion if there are other ways to better solve it.

  1. I removed setBuffering(true); when we skip to a new time. In the current state, when you slide the time slider, it is continuously switching between the buttons showing as spinners and the buttons showing normally, because every time the slider moves to a new frame, we set buffering to true until the frame is received and it gets set as false, which is a weird user experience. While this eliminates that weirdness when you're sliding the slider, it could be an issue if a frame we just jumped to is actually taking a while to load, in which case the loading state where the buttons are spinners wouldn't show up. I couldn't actually get this case to occur through manual testing, but I'm sure it'd be possible if the octopus cluster was really overrun and it was in the process of starting up a new container to handle the increased traffic. If we're worried about this, maybe I could add some logic so we only show the spinners if buffering has held as true for some non-trivial amount of time?
  2. I removed the antd Slider's onAfterChange functionality and simplified the onChange functionality. We were having issues with having the right frame actually get displayed when jumping / sliding around a lot on the slider, and sometimes we would end up in a state where everything was stuck loading. As part of my debugging process, I removed various pieces of functionality to see how they were impacting the performance, and a lot of our problems seemed to be solved when we just directly called onTimeChange from the sliders onChange? I also poked around in some of our other repos that use this component, and none of them were using onAfterChange. This code has been this way for a couple years, maybe the reason we needed this extra functionality originally (either because of antd or some of our own code) is no longer relevant?

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Copy link

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 66.32% 644/971
🟡 Branches 65.73% 94/143
🔴 Functions 35.1% 86/245
🟡 Lines 64.72% 576/890

Test suite run success

104 tests passing in 7 suites.

Report generated by 🧪jest coverage report action from 2a1ba9f

@ascibisz ascibisz linked an issue Jun 10, 2024 that may be closed by this pull request
@ascibisz ascibisz marked this pull request as ready for review June 25, 2024 15:14
@ascibisz ascibisz requested a review from a team as a code owner June 25, 2024 15:14
@ascibisz ascibisz requested review from ShrimpCryptid and frasercl and removed request for a team June 25, 2024 15:14
@@ -148,8 +120,7 @@ const PlayBackControls = ({
</div>
<Slider
value={time}
onChange={handleTimeChange}
onAfterChange={handleSliderMouseUp}
onChange={onTimeChange}
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid Jun 25, 2024

Choose a reason for hiding this comment

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

Hmm... while I agree that this change does fix the buffering issue, as a user, I would expect playback to pause while I'm interacting with the slider. Could you confirm with UX whether this new behavior is something they're okay with ?

Ant now has an onChangeComplete instead of onAfterChange... could that fix some of the issues? https://ant.design/components/slider

@ascibisz ascibisz marked this pull request as draft October 22, 2024 16:46
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.

Fix Buggy Behavior with Time Slider
2 participants