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

Music: Fix level_data loading when using Next button #51990

Merged
merged 1 commit into from
May 19, 2023

Conversation

breville
Copy link
Member

@breville breville commented May 19, 2023

This fixes an issue in which pressing the "Next" button inside Music Lab was loading both the current level and the next level's "level_data" more or less simultaneously, and sometimes the new level would show the previous level's instructions.

@molly-moen did an excellent analysis which is reproduced here with slight tweaks:

When you click "Next" in a script-level, the following things happen:

  1. onNextPanel calls ProgressManager’s next, which increments the step in the ProgressManager and then calls back to MusicView’s onProgressChange.
  2. onProgressChange sets the current progress state in musicRedux, which is different from setting the current level in progressRedux.
  3. onNextPanel then calls handlePanelChange, which fires off an async method to load the next level. It calls the update with currentLevelIndex, which has not been changed yet.
  4. Back in onNextPanel, if we are a script-level, then we dispatch a navigateToLevelId in progressRedux. When we are a script-level, this will result in the new level index arriving in the currentLevelIndex prop.
  5. In componentDidUpdate, we see that currentLevelIndex has changed, so we call handlePanelChange again with the correct new level index, which fires off a second async method to load the new level.
  6. In some repro situations (sometimes on the first load for a given user on the progression), the first request finishes after the second request, so we see the old level data.

Really, we are prematurely calling handlePanelChange in onNextPanel.

Given this analysis, the fix indeed seems to be to simply remove the unnecessary call to handlePanelChange in onNextPanel.

@breville breville requested review from moneppo and a team May 19, 2023 00:56
Copy link
Contributor

@sanchitmalhotra126 sanchitmalhotra126 left a comment

Choose a reason for hiding this comment

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

🎉 thanks Brendan for the quick fix and thanks Molly for the diagnosis! I'm going to go ahead and merge this now to get this in for today's deploy.

@sanchitmalhotra126 sanchitmalhotra126 merged commit 454841a into staging May 19, 2023
2 checks passed
@sanchitmalhotra126 sanchitmalhotra126 deleted the music-fix-level-data-loading branch May 19, 2023 17: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.

None yet

2 participants