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

Refactor ProfileView.onAppear code #1748

Merged
merged 3 commits into from
Jan 31, 2025
Merged

Conversation

mplorentz
Copy link
Member

Issues covered

Another attempt at fixing https://github.com/verse-pbc/issues/issues/171 and https://github.com/verse-pbc/issues/issues/175

Description

Story time:
After merging #1744 I had Rabble test the build to see if it fixed the performance issue they were having. It did not. Then this morning while looking into Itunu's comment on #1744 I noticed that the profile photos weren't loading in the home feed on a fresh install. I confirmed that this was caused by #1744 so I reverted it.

I came up with a new theory for the cause of the ProfileView refresh loop. There are a couple issues here:

  1. The "Profile View Opened" analytics event was firing way too often, including when you tap any other tab.
  2. Sometimes a race condition is hit where "Profile View Opened" gets called in an infinite loop. This is a big problem because it's really ProfileView.onAppear getting called in a loop, and ProfileView.onAppear sends a request to download a bunch of events from the Relay Service. When called in a loop this pegs the CPU and fills up the parse queue, causing the app to be unresponsive. My theory is that this is the cause of Rabble's issues. Unfortunately I still have no steps to reproduce this reliably.

This PR sets out to solve issue 1, and I'm hoping that in doing so we maybe solve 2 as well. Issue 1 is somewhat well known. SwiftUI's TabView calls onAppear for all the tabs all the time. We worked around this in the Home, Discover, and Notifications tab by keeping track of @State var isVisible and only firing analytics and other things when the tab actually goes from not being visible to being visible. We never did this on the ProfileView because unlike the others it isn't only presented by the TabView. It's also pushed onto the navigation stack from wherever you tap on an author's name.

I started by refactoring the isVisible code into a nice view modifier. Then I adopted that modifier on all the tabs. This allowed me to fire a new "Profile Tab Opened" event when the tab is selected. To track the other times the profile view is opened I added an analytics call to file "Profile View Opened" in the Router. I also renamed some of the other analytics events to make it clear that they don't only fire when the user taps the tab, it fires any time that view appears, even when navigating back from a pushed view.

I also refactored the call to downloadAuthorData() in ProfileView so that it is called in a .task {} modifier instead of .onAppear {}. This means it will only be called when SwiftUI observes a change in one of its variables, i.e. the author being displayed. This is the change that I'm hoping fixed the weird infinite loop race condition. My working theory is that the race condition had something to do with onAppear() triggering downloadAuthorData(), then downloadAuthorData() somehow causing the author to be mutated enough that AppView would redraw ProfileTab, which caused onAppear() to fire again.

How to test

I guess the main test would be tapping all the tabs rapidly and seeing if you can trigger the infinite loop where the parse queue fills up and the CPU gets stuck at 100%.

@pelumy
Copy link
Contributor

pelumy commented Jan 31, 2025

👀

Copy link
Contributor

@pelumy pelumy left a comment

Choose a reason for hiding this comment

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

Oh my! Oh my! Oh my! Great detective skills! 🕵️‍♂️

This looks like it has solved the issue. I don't see the analytics of the profile view logged constantly again. It only gets logged when that tab actually appears.

I love the new modifier too ❤️. I see a looooott of improvement! 🎉

I hope this fixes the issue for Rabble.

@mplorentz mplorentz added this pull request to the merge queue Jan 31, 2025
Merged via the queue into main with commit 938b4a7 Jan 31, 2025
4 checks passed
@mplorentz mplorentz deleted the refactor-profile-view-onappear branch January 31, 2025 18:54
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