-
Notifications
You must be signed in to change notification settings - Fork 0
Introduce pending state for unresolved follows #648
Conversation
a8f4897
to
610ff22
Compare
xcode/Subconscious/Shared/Components/Common/Story/StoryUserView.swift
Outdated
Show resolved
Hide resolved
e3d28fc
to
bf00658
Compare
606dd84
to
f3bb630
Compare
Catches an edgecase where if you're spamming refresh you'll get the resolved data before the background listener.
28869b0
to
2f183eb
Compare
switch action { | ||
case .refresh: | ||
app.send(.syncAll) |
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.
Ensure everything is refreshed if a user is trying to manually refresh their following list.
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.
Another way we do this is through the slightly more compact/fancy approach of:
.onReceive(
store.actions.compactMap(AppAction.from),
perform: app.send
)
where AppAction.from
is an extension that creates an app action from a local action, or else nil. The advantage being that the AppAction.from
extension can be tested.
Non-blocking.
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.
Clean! I like that this moves the mapping nearer to the model and away from the view.
@gordonbrander not related to this work but I think I'll disable the swiftlint github check, it just adds too much noise to the PR review. I'm using it in Xcode locally so I might update #573 to remove the github action as well. |
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.
LGTM with a couple questions
switch action { | ||
case .refresh: | ||
app.send(.syncAll) |
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.
Another way we do this is through the slightly more compact/fancy approach of:
.onReceive(
store.actions.compactMap(AppAction.from),
perform: app.send
)
where AppAction.from
is an extension that creates an app action from a local action, or else nil. The advantage being that the AppAction.from
extension can be tested.
Non-blocking.
@@ -116,6 +129,10 @@ struct StoryUserView: View { | |||
} | |||
.contentShape(.interaction, RectangleCroppedTopRightCorner()) | |||
.onTapGesture { | |||
guard story.resolutionStatus == .resolved 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.
Preference for this no-op to belong to the update function, rather than the view. When actions handle their own no-op based on model state, it's more easily testable. It also makes sending the action foolproof.
Non-blocking.
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.
I introduced a new action requestNavigateToProfile
which can result in pushDetail
if resolved or failPushDetail
if not. This logs a debug message and does nothing else atm.
@@ -58,6 +62,8 @@ struct Func { | |||
) async throws -> T? { | |||
do { | |||
return try await perform(attempts) | |||
} catch RetryError.cancelled { | |||
return nil |
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.
Open question, but would it be better to have this retry with backoff logic be part of an update function instead? (If fx fails, send another after a growing delay).
The advantage would be that retry would be forced to fold with app state at each step. The actions would be sent through the logs, and current backoff value would be written to the model where we can inspect it.
Non-blocking.
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.
I can see the value of this but it could be a little over-complicated right now.
The main issue I see is that multiple of these processes can be running at once (rapidly following multiple users before the last one resolves), so tracking the state will require attaching an ID to each background job etc.
I'd like to think a bit more about the right abstraction to make it trivial for different components to sign up for this functionality and share the code, put some initial thoughts down here: #658
@@ -7,6 +7,12 @@ | |||
|
|||
import Foundation | |||
|
|||
enum PetnameResolutionStatus: Equatable, Hashable, Codable { |
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.
Qs:
- Should this be a general enum, similar to
LoadingState
?- Is this the same semantics as
LoadingState
? It's ok if subtly different. - If different, still worth generalizing this I bet. We have a lot of places that have the same network resolution lifecycle.
- Is this the same semantics as
- Is there a resolved success/failure? (
resolved(Result)
)
Non-blocking.
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.
I dropped the Petname
prefix and extracted the type and added a Cid
payload to resolved
since I currently view the definition of resolution as "take this address and give me the CID on the other end".
Builds on #647 because I already threaded
app
into the profile view there..syncAll
onapp
Demo
Screen.Recording.2023-05-16.at.5.46.10.pm.mov
Fixes #555
Fixes #492
Related to #604