-
Notifications
You must be signed in to change notification settings - Fork 0
Migrate Activity
to consuming and firing state update events
#91
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
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
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.
Pull Request Overview
This PR migrates the Activity
class to consume and fire state update events instead of directly updating state, completing the event-driven architecture transition.
- Migrates
Activity
from consuming WebSocket events toStateUpdateEvent
s - Updates
Activity
to fire state update events on successful API calls instead of directly updating state - Removes option-specific event handlers from
ActivityStateImpl
in favor of general poll update events
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
ActivityEventHandler.kt | Changed from consuming WSEvent to StateUpdateEvent with updated event handling logic |
StateUpdateEvent.kt | Added ActivityUpdated event and modified reaction events to include feed ID |
ActivityStateImpl.kt | Removed option-specific handlers, consolidated poll update logic with ID validation |
ActivityImpl.kt | Updated to fire StateUpdateEvent on API success instead of direct state updates |
FeedsClientImpl.kt | Removed socket subscription manager parameter from ActivityImpl constructor |
Test files | Updated tests to use StateUpdateEvent instead of WSEvent with proper feed ID parameters |
Comments suppressed due to low confidence (1)
stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/internal/state/ActivityStateImpl.kt:123
- The
updatePoll
method has duplicate poll ID validation. The check on line 118 is redundant since each calling method (e.g.,onPollClosed
,onPollDeleted
) already performs the same check before calling this method.
private fun updatePoll(pollId: String, update: PollData.() -> PollData?) {
if (_poll.value?.id != pollId) return
var updated: PollData? = null
_poll.update { current -> current?.let(update).also { updated = it } }
_activity.update { current -> current?.copy(poll = updated) }
}
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...-client/src/test/kotlin/io/getstream/feeds/android/client/internal/state/ActivityImplTest.kt
Outdated
Show resolved
Hide resolved
SDK Size Comparison 📏
|
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.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...client/src/main/kotlin/io/getstream/feeds/android/client/internal/state/ActivityStateImpl.kt
Show resolved
Hide resolved
...roid-client/src/main/kotlin/io/getstream/feeds/android/client/internal/state/ActivityImpl.kt
Show resolved
Hide resolved
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!
Just for my better understanding: Is the idea to trigger these StateUpdateEvent
s on WS events + whenever we make a local update in the state via the Feed
/Activity
operations? Do we make these updates on each operation that we know will trigger a WS event?
In my mind, it's not necessarily that we know that they would trigger a WS event. It's more that we know that the operation can result in a state update. But TBH, that should coincide with the backend also triggering the same WS event, so it's probably a distinction without a difference 😅 |
Yeah that was my assumption, that we should align the local state with the BE state, even if we don't get a WS informing us about it. Thanks for the explanation! |
Goal
Similar to previous PRs like e.g. #83, #89, we migrate
Activity
. But this time, in addition to consuming state update events, we also fire them on successful api calls instead of updating the state directly. This now finally means that whoever is listening to those events will be updated even if we're not receiving socket events (as described in #83).Implementation
WSEvent
toStateUpdateEvent
ActivityImpl
instead of updating the state directly_state.onActivityUpdated(activity)
subscriptionManager.onEvent(ActivityUpdated(fid, activity))
Testing
Disable socket events in the sample and perform operations through an
Activity
, e.g. adding a vote to a poll, or adding a comment, and verify that relevant already-migrated objects (e.g.PollVoteList
,Activity
itself) are update correctly. Note:Feed
object will not yet react in this situation, because it's not migrated yet (coming in the next PR).Checklist