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

Onboarding fixes #3966

Merged
merged 7 commits into from
May 11, 2024
Merged

Onboarding fixes #3966

merged 7 commits into from
May 11, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented May 11, 2024

See individual commits.

  • d8ee3b6: Fixes a flash after new onboarding that caused Following to get selected instead of Discover. The problem is we were writing Discover into savedFeeds via overwriteSavedFeeds, but RQ has already cached initial savedFeeds (just the following), and we had a flash of that during the initial Home render. The fix is to invalidate RQ cache before going to Home.
  • d8265e5: There is no reason for this call to be serial from what I can see. It can be parallelized against bulk follows and setting prefs. Only setting prefs has to be serial (because lol, we designed it to be this way).
  • 03ecf4c: Unnecessary await if we're not uploading anything. Let's remove that.
  • cf29021: Unnecessary waterfall. Let the image upload begin in parallel with getting the profile.
  • fd69294: Drive-by fix for the same issue in regular profile editor. We can upload in parallel without waiting for account info.
  • 528877b: It's weird to see an empty profile pic after finishing onboarding. Let's invalidate that query.
  • a72bb81: The code in main adds the profile image step both to old and new onboarding, but only new onboarding used to persist this image. Oops. This makes it persist in both cases. If we don't want that step, we can gate this code again.

Test Plan

Hardcode either true or false like this:

--- a/src/lib/statsig/statsig.tsx
+++ b/src/lib/statsig/statsig.tsx
@@ -115,6 +115,9 @@ export function useGate(): (gateName: Gate) => boolean {
   }
   const gate = React.useCallback(
     (gateName: Gate): boolean => {
+      if (gateName === 'reduced_onboarding_and_home_algo') {
+        return false
+      }

Then go through both onboardings. Observe that:

  • There is no flash of Following tab after onboard
  • You reliably land on Discover
  • Your profile pic is set

Also verify the normal "edit profile" workflow isn't borked (incl. avatar and cover upload).

New onboarding:

flow.mov

Old onboarding:

old_flow.mov

@gaearon gaearon requested a review from haileyok May 11, 2024 17:47
Copy link

render bot commented May 11, 2024

Copy link

Old size New size Diff
7.14 MB 7.14 MB -43 B (-0.00%)

@haileyok
Copy link
Contributor

haileyok commented May 11, 2024

Okay going through each commit right now:

  • d8ee3b6: Fixes a flash after new onboarding that caused Following to get selected instead of Discover. The problem is we were writing Discover into savedFeeds via overwriteSavedFeeds, but RQ has already cached initial savedFeeds (just the following), and we had a flash of that during the initial Home render. The fix is to invalidate RQ cache before going to Home.
  • d8265e5: There is no reason for this call to be serial from what I can see. It can be parallelized against bulk follows and setting prefs. Only setting prefs has to be serial (because lol, we designed it to be this way).
  • 03ecf4c: Unnecessary await if we're not uploading anything. Let's remove that.
  • cf29021: Unnecessary waterfall. Let the image upload begin in parallel with getting the profile.
  • fd69294: Drive-by fix for the same issue in regular profile editor. We can upload in parallel without waiting for account info.
  • 528877b: It's weird to see an empty profile pic after finishing onboarding. Let's invalidate that query.
  • a72bb81: The code in main adds the profile image step both to old and new onboarding, but only new onboarding used to persist this image. Oops. This makes it persist in both cases. If we don't want that step, we can gate this code again.

@haileyok
Copy link
Contributor

haileyok commented May 11, 2024

There is no reason for this call to be serial from what I can see. It can be parallelized against bulk follows and setting prefs. Only setting prefs has to be serial (because lol, we designed it to be this way).

Okay I agree with this one, but I think we need to invalidate the profile query afterwards. If I upload an avatar during onboarding, then the profile icon in the bottom right-hand corner does not update until after I open up my profile, which could be a little confusing if I expect to have uploaded an avatar.

Marking it as working for right now but am going to come back to this one.

RocketSim_Recording_iPhone_15_Pro_6.1_2024-05-11_11.31.53.mp4

@gaearon
Copy link
Collaborator Author

gaearon commented May 11, 2024

Okay I agree with this one, but I think we need to invalidate the profile query afterwards. If I upload an avatar during onboarding, then the profile icon in the bottom right-hand corner does not update until after I open up my profile, which could be a little confusing if I expect to have uploaded an avatar.

Yeah, 528877b should do it.

I'm not sure if it's not guaranteed to come through (e.g. useProfileUpdateMutation has whenAppViewReady call to be more rigorous). However I think we should also be mindful of the delay we're adding here — don't want to bounce. So I'd probably lean towards not adding more waiting here. Note the image you're using is very high res.

What we really should do (IMO) is upload the avatar in the earlier step. Of course that would have to be able to retry on failure here, so it would have to be more like an optimistic upload than a "let's just move this code there" kind of thing.

@haileyok
Copy link
Contributor

Yea, agree w/ those points. FWIW it also happens even if I upload the generic created avatar (I have not gotten to your mentioned commit yet though so will check against that here in a minute).

@haileyok
Copy link
Contributor

Okay yea, that does fix the issue (even w/ the high-res image). Seems like your idea would be a good one. And you're right about not wanting to make this take longer (especially on slower connections). Maybe:

  • Attempt to upload the image as soon as next is pressed from that screen
  • If for some reason it fails, attempt to do it again the same way we are right now
  • If the second attempt fails...?

I think we can do that in a separate PR though.

@gaearon gaearon merged commit 51b4b22 into main May 11, 2024
6 checks passed
@gaearon gaearon deleted the onboarding-fixes branch May 11, 2024 18:55
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.

2 participants