-
Notifications
You must be signed in to change notification settings - Fork 66
IOS-5365 implement the async membership handling #4111
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
IOS-5365 implement the async membership handling #4111
Conversation
refactor the membership storage and event handling
Bugs/IssuesRace condition in event handling (MembershipStatusStorage.swift:73-91, 93-102)
Missing tiers refresh (MembershipStatusStorage.swift:55-58)
Silent error swallowing (MembershipCoordinatorModel.swift:65-68)
Best PracticesDebug print statements in production (MembershipStatusStorage.swift:76, 87, 89)
Missing error handling (MembershipStatusStorage.swift:55-58)
Summary: 🚨 Major Issues - Race conditions in event handling, incomplete refresh logic, and silent error swallowing need fixing |
| var tiers: [MembershipTier] { | ||
| let currentTierId = userMembership.tier?.type.id ?? 0 | ||
| return allTiers | ||
| .filter { FeatureFlags.membershipTestTiers || !$0.isTest } | ||
| .filter { !$0.iosProductID.isEmpty || $0.type.id == currentTierId } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use this on the UI level, it will really affect performance. It's better to make this a published property and discard @Published private var allTiers: [MembershipTier] = []
because seems like it's not used anymore.
Also naming is really ambiguous. We have allTiers and tiers with no significant distinction and ability to understand what is what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. UX fields should not be computable.
| } | ||
|
|
||
| case .membershipTiersUpdate(let update): | ||
| Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Task created inside main actor will still be called on the main actor. If you want to do it in the backgroind you need to call Task.detached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or remove main action from storage (legacy). Its better.
| public let isTest: Bool | ||
| public let iosProductID: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have this properties available inside Anytype_Model_MembershipTierData I am not sure we need to introduce them here once again
| ) | ||
| _status.tier.flatMap { AnytypeAnalytics.instance().logChangePlan(tier: $0) } | ||
| AnytypeAnalytics.instance().setMembershipTier(tier: _status.tier) | ||
| print("[Membership] Updated membership status - tier: \(_status.tier?.name ?? "none")") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As claude mentioned - no prints in production code. It should be either assert or remove it
| @Published private var _status: MembershipStatus = .empty | ||
|
|
||
|
|
||
| var tiersPublisher: AnyPublisher<[MembershipTier], Never> { $_tiers.eraseToAnyPublisher() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legacy way. Try to use Async stream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reworked to use asynchronous streams
|
|
||
| private var subscription: AnyCancellable? | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please disable "trim whitespace-only lines" in Xcode and rollback all this changes
…o ios-5365-membership-data-fetching-refactoring-integration-ios-refactor # Conflicts: # Libraryfile # Modules/ProtobufMessages/Sources/Protocol/Events/Anytype_Event.Membership.TiersUpdate.swift # Modules/ProtobufMessages/Sources/Protocol/Events/Anytype_Event.Message.swift
| membershipStatusStorage.statusPublisher.receiveOnMain().assign(to: &$userMembership) | ||
|
|
||
| statusTask = Task { [weak self] in | ||
| guard let self else { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retain cycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use view.task.
Look example
| } | ||
|
|
||
| tiersTask = Task { [weak self] in | ||
| guard let self else { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| private func combinedStream() -> AsyncStream<(MembershipStatus, [MembershipTier])> { | ||
| let storage = membershipStatusStorage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use combineLastes in AsyncAlgoritms
| showTiersLoadingError = true | ||
|
|
||
| func retryLoadTiers() { | ||
| Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EmptyStateView support async throws methods ->retryLoadTiers() async throws
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We try not to write Task { wherever possible.
| } | ||
| .onChange(of: reason) { _, reason in | ||
| model.updateState(reason: reason) | ||
| Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make task(id: reason)
| .foregroundColor(.Text.primary) | ||
| case nil: | ||
| Rectangle().hidden().onAppear { | ||
| Rectangle().hidden().task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why task here?
| let storage = Container.shared.membershipStatusStorage.resolve() | ||
| storage.statusPublisher.receiveOnMain().assign(to: &$userMembership) | ||
| statusTask = Task { [weak self] in | ||
| guard let self else { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let storage = Container.shared.membershipStatusStorage.resolve() | ||
| storage.statusPublisher.receiveOnMain().assign(to: &$membership) | ||
| statusTask = Task { [weak self] in | ||
| guard let self else { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private var _status: MembershipStatus = .empty | ||
| private var _tiers: [MembershipTier] = [] | ||
|
|
||
| private var statusContinuations: [UUID: AsyncStream<MembershipStatus>.Continuation] = [:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use AsyncToManyStream
|
|
||
| nonisolated init() { } | ||
|
|
||
| nonisolated func statusStream() -> AsyncStream<MembershipStatus> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are write var for publisher and stream without filters
| _status | ||
| } | ||
|
|
||
| nonisolated func tiersStream() -> AsyncStream<[MembershipTier]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
| } | ||
|
|
||
| func currentTiers() async -> [MembershipTier] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsyncToManyStream allows you to read the last value in a nonisolated var. We try to read the var synchronously. This greatly simplifies the code.
|
|
||
| AnytypeAnalytics.instance().setMembershipTier(tier: _status.tier) | ||
| membershipUpdateTask?.cancel() | ||
| membershipUpdateTask = Task { [weak self, builder] in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why task? You can mark handle as async and call any async methods inside
mgolovko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many tasks in an asynchronous context whose purpose is unclear. There’s code that could be simplified by using solutions from project.
|
decided to reimplement this by ios team, as original PR became bigger than I expected |
Refactored membership data management to work with new middleware caching behavior for faster app startup.
Problem
Middleware now returns cached membership/tiers data immediately (with noCache: false) instead of blocking on network calls. App startup was slow because we were forcing network calls with noCache: true.
Changes
Single Source of Truth Architecture
Fast Startup
Event Handling
Tier Filtering
Files Modified