Skip to content

Fix flaky follows on sign up, mobile C-2718 #6219

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.

Sign up for GitHub

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

Merged
nicoback2 merged 1 commit into from
Oct 4, 2023
Merged

Fix flaky follows on sign up, mobile C-2718 #6219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/common/src/utils/sagaHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@ export function* doEvery(
return chan
}

/**
* Waits for account to finish loading, whether it fails or succeeds.
*/
export function* waitForAccount() {
yield* call(
waitForValue,
Expand Down
1 change: 0 additions & 1 deletion packages/web/src/common/store/pages/signon/sagas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,6 @@ function* watchConfigureMetaMask() {
function* watchFollowArtists() {
while (
yield all([
take(signOnActions.SIGN_UP_SUCCEEDED),

@dylanjeffers dylanjeffers Oct 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

interesting, so simply removing this allows the fetch to continue, that right?

@nicoback2 nicoback2 Oct 4, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah this was redundant because we call fetchAccountAsync after putting SIGN_UP_SUCCEEDED https://github.com/AudiusProject/audius-protocol/blob/main/packages/web/src/common/store/pages/signon/sagas.js#L499-L501.
For some reason web always executed the fetchAccountAsync first before the SIGN_UP_SUCCEEDED watchers (which includes the follow artists fn), while mobile did not always do the same.

@nicoback2 nicoback2 Oct 4, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are def other ways to fix this though... We could remove the watch for accountActions.fetchAccountSucceeded.type here as well and just call signOnActions.FOLLOW_ARTISTS at the end of the sign up saga after fetchAccountAsync https://github.com/AudiusProject/audius-protocol/blob/main/packages/web/src/common/store/pages/signon/sagas.js#L499-L501. Or we could add a watcher for signOnActions.signUpSucceededWithId(userId) (called here) that sets the user id in the account store so that we don't have to wait to fetch the full account before doing the follows.

@dylanjeffers dylanjeffers Oct 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is fine for now cause with the new sign up flow pretty much all of this will go away

@nicoback2 nicoback2 Oct 4, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ooh gotcha I thought all the sagas were staying the same and just the Ui was changing?

@dylanjeffers dylanjeffers Oct 4, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

True but some of these sagas are not needed. For instance this fetch artists saga can just be an Audius-query thing inside the relevant components

@nicoback2 nicoback2 Oct 4, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See: https://github.com/AudiusProject/audius-protocol/blob/main/packages/web/src/common/store/pages/signon/sagas.js#L499-L501

We don't need to watch sign_up_succeeded here. watching fetchAccountSucceeded ensures that the account is loaded before we attempt to follow the artists

take(accountActions.fetchAccountSucceeded.type),
take(signOnActions.FOLLOW_ARTISTS)
])
Expand Down
4 changes: 3 additions & 1 deletion packages/web/src/utils/sagaHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import {
} from 'store/reachability/sagas'

/**
* Required for all writes
* Required for all writes.
* NOTE: This does not ensure that the account exists. If the account is required
* for your operation, call `ensureLoggedIn` or an equivalent after calling `waitForWrite`.
*/
export function* waitForWrite() {
yield* call(waitForBackendSetup)
Expand Down