fix(realtime): make Push associated to MainActor #721
Conversation
grdsdev
force-pushed
the
fix/push-mainactor
branch
from
8c4dc93 to
469d805
Compare
| @@ -81,6 +83,7 @@ public final class RealtimeChannelV2: Sendable { | |||
| } | |||
|
|
|||
| /// Subscribes to the channel | |||
| @MainActor | |||
There was a problem hiding this comment.
This isn't a breaking change since the method is async, the call site already have to call this method with an await independent of the running actor.
Tests/RealtimeTests/_PushTests.swift
Outdated
| // let status = await task.value | ||
| // XCTAssertEqual(status, .ok) | ||
| // } | ||
| func testPushWithAck() async { |
There was a problem hiding this comment.
Since Push is MainActor now, this flaky test passes consistently
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Push implementation by converting it from an Actor to a MainActor-attached class for improved debuggability and reasoning. Key changes include:
- Removing the custom invokeTest logic in tests and annotating the test class with @mainactor.
- Updating the RealtimeChannelV2 to use direct MainActor access for mutable state.
- Refactoring PushV2 from an actor to a final class annotated with @mainactor.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/RealtimeTests/_PushTests.swift | Removed custom invokeTest logic and updated async test flow |
| Sources/Realtime/RealtimeChannelV2.swift | Refactored mutable state accesses and updated async patterns |
| Sources/Realtime/PushV2.swift | Changed from an actor to a MainActor-attached final class |
Tests/RealtimeTests/_PushTests.swift
Outdated
|
|
||
| let task = Task { | ||
| await push.send() | ||
| } |
There was a problem hiding this comment.
[nitpick] Consider adding a comment explaining the use of Task.megaYield() to clarify why it is used instead of the standard Task.yield(), ensuring its necessity for test scheduling.
| } | |
| } | |
| // `Task.megaYield()` is used here instead of `Task.yield()` to ensure that all pending tasks | |
| // are fully processed before proceeding. This is necessary for proper test scheduling and | |
| // to simulate realistic asynchronous behavior in the test environment. |
Copilot uses AI. Check for mistakes.
| Task { @MainActor in | ||
| mutableState.clientChanges.append(config) | ||
| } |
There was a problem hiding this comment.
Since the enclosing context is already on the MainActor, consider appending directly to mutableState.clientChanges instead of dispatching via a Task, to simplify execution and ensure immediate consistency.
| Task { @MainActor in | |
| mutableState.clientChanges.append(config) | |
| } | |
| mutableState.clientChanges.append(config) |
Copilot uses AI. Check for mistakes.
Pull Request Test Coverage Report for Build 15296140878Details
💛 - Coveralls |
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Pushis currently implemented as anActor.What is the new behavior?
Refactor
Pushto be a class attached to theMainActor, which makes it easier to debug and reason about behavior.Additional context
Add any other context or screenshots.