Change Client, Document actor to class #187
Conversation
WalkthroughThe recent updates primarily involve transitioning core structures from actors to classes, refining initialization processes, and enhancing method handling. These changes aim to improve performance, readability, and synchronization logic in document management. Additionally, various methods were modified to remove asynchronous behavior, aligning with the new design approach. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #187 +/- ##
==========================================
- Coverage 45.13% 45.11% -0.03%
==========================================
Files 105 105
Lines 19333 19320 -13
==========================================
- Hits 8726 8716 -10
+ Misses 10607 10604 -3 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- Sources/Core/Client.swift (16 hunks)
- Sources/Document/CRDT/CRDTTree.swift (2 hunks)
- Sources/Document/Document.swift (5 hunks)
- Sources/Util/IndexTree.swift (3 hunks)
- Tests/Unit/Document/DocumentTests.swift (1 hunks)
Additional comments not posted (23)
Sources/Document/Document.swift (9)
73-74: Change from actor to class is approved.The transition from
actorto@MainActor classis correctly implemented, ensuring that instances ofDocumentare still confined to the main actor's context, which is crucial for UI-related operations in iOS.
75-75: Change inSubscribeCallbacktype.The change from using
isolated DocumenttoDocumentin theSubscribeCallbacktype is consistent with the actor-to-class transition. This modification ensures that the callback can still be executed on the main actor without requiring isolation, which is necessary due to the class not supporting isolation like actors.
78-89: Initialization of properties inline.Initializing properties inline (e.g.,
status,changeID,checkpoint,localChanges,root,subscribeCallbacks,presenceSubscribeCallback) is a good practice as it ensures that all instances start with a known state, reducing the potential for bugs related to uninitialized properties.
96-96: Initialization ofonlineClients.The change to initialize
onlineClientsinline ensures that the property is never nil, which simplifies the usage of this property throughout the class by removing the need to check for nil before use.
101-101: Refactor of initializers.Changing
public init(key: String)topublic convenience nonisolated init(key: String)and adjusting the call to another initializer within the same class is a good practice. It simplifies the initialization process by centralizing common initialization steps in one initializer, reducing code duplication.
107-109: Use ofnonisolatedin initializers.Marking the initializer as
nonisolatedallows it to be called from any context, not just the main actor, which increases the flexibility of how instances ofDocumentcan be created.
Line range hint
115-136: Refactor ofupdatemethod to synchronous.Removing
asyncfrom theupdatemethod and its internal logic is consistent with the PR's goal to makeDocumentoperations synchronous. This change should ensure that updates to the document are immediate and predictable, which is often required when dealing with UI elements that should update synchronously with user interactions.
245-245: Refactor ofapplyChangePackmethod to synchronous.The change from
async throwstothrowsin theapplyChangePackmethod aligns with the overall goal of making document operations synchronous. This adjustment should help in scenarios where immediate feedback or state update is necessary after applying changes.
367-367: Optimization ingarbageCollectmethod.The addition of a condition to check
disableGCbefore performing garbage collection is a good optimization. It avoids unnecessary processing when garbage collection is disabled, which can improve performance.Tests/Unit/Document/DocumentTests.swift (1)
741-743: Simplify test structure by removing nestedTaskblocks.The changes made remove unnecessary complexity, aligning with the PR's objectives to simplify and enhance synchronization logic.
Sources/Document/CRDT/CRDTTree.swift (2)
624-624: Refactor of loop usingsuffix(from:)and for-in loop approved.The refactoring of the loop to use
suffix(from:)and a for-in loop improves readability and potentially enhances performance by leveraging Swift's built-in functions.Verify side effects of loop refactoring.
Ensure that the refactoring does not introduce any side effects, especially in edge cases where the loop might behave differently.Verification successful
Refactor of loop using
suffix(from:)and for-in loop approved.The refactoring of the loop to use
suffix(from:)and a for-in loop improves readability and potentially enhances performance by leveraging Swift's built-in functions.
- The existing test functions in
JSONTextTest.swiftindirectly verify the behavior of thefindNodesAndSplitTextmethod through various operations onJSONTextobjects.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the loop behavior by checking the unit tests related to this method. # Test: Search for the unit tests related to `findNodesAndSplitText`. rg --type swift -A 10 $'func findNodesAndSplitText'Length of output: 1062
Script:
#!/bin/bash # Description: Search for unit tests related to `findNodesAndSplitText` in the entire codebase. # Test: Search for the term `findNodesAndSplitText` in test files. rg --type swift -g '*Test.swift' 'findNodesAndSplitText'Length of output: 56
Script:
#!/bin/bash # Description: Search for test functions in the test files and analyze their content to identify any indirect tests for `findNodesAndSplitText`. # Test: Search for test functions in Swift test files. rg --type swift -g '*Test.swift' 'func test'Length of output: 1072
Script:
#!/bin/bash # Description: Extract the content of identified test functions and check for any references to `findNodesAndSplitText`. # Extract the content of the test functions. rg --type swift -g 'JSONTextTest.swift' -A 20 'func test_should_handle'Length of output: 15737
1143-1143: Refactor of loop usingenumerated()approved.The use of
enumerated()in the loop simplifies the access to both the index and the value, enhancing readability and maintainability.Verify side effects of loop refactoring using
enumerated().
Ensure that the use ofenumerated()does not introduce any unintended side effects, especially in edge cases.Sources/Core/Client.swift (8)
130-130: Change from actor to class noted.The change from
actortoclassforClientaligns with the PR's description to handle synchronous operations on the main thread.
131-131: Initialization of properties within the class.Inline initialization of properties like
attachmentMapand various timing configurations ensures that all instances ofClientstart with a defined state, which is crucial for classes to avoid uninitialized state issues.
145-145: Initialization ofstatusproperty.Initializing the
statusproperty inline is a good practice as it ensures that every instance ofClienthas a predictable initial state.
170-170: Use ofconvenienceinitializer.Changing to a
convenienceinitializer that calls another initializer is a standard practice in Swift for providing multiple pathways to initialize an object. This change is well-implemented.
241-241: Handling document attachment logic.The changes in document attachment logic, specifically the checks and status updates, are crucial for ensuring that documents are managed correctly in the new synchronous context.
Also applies to: 269-269, 273-273
315-315: Handling document detachment logic.The logic for detaching documents, including status checks and exception handling, appears correctly implemented to ensure that documents are detached properly.
Also applies to: 322-322, 333-333, 336-336
536-536: Watch loop adjustments.The modifications to the watch loop management, specifically the handling of document watching and event responses, are crucial for maintaining the integrity of document synchronization.
Also applies to: 586-586
696-696: Push-pull synchronization adjustments.The changes to how documents are synchronized with the server, including error handling and status updates, are well-implemented. It's important to ensure that these changes work seamlessly with the rest of the system to maintain data consistency.
Also applies to: 720-720, 726-726, 733-733
Sources/Util/IndexTree.swift (3)
96-96: Refactoring of loop inaddSizeOfLeftSiblings.Using
prefix(upTo:)to iterate over left siblings is a more efficient approach than the previous method, likely improving performance by avoiding unnecessary iterations.
751-751: Refactoring of loop infindTextPos.Iterating directly over
node.childrensimplifies the logic and potentially improves the readability and performance of the function.
858-858: Refactoring of path iteration inpathToTreePos.Using
dropLast()to iterate over path elements is a cleaner and potentially more efficient approach than manually managing indices. This change likely improves both performance and readability.
|
|
||
| /** | ||
| * @param rpcAddr - the address of the RPC server. | ||
| * @param opts - the options of the client. | ||
| */ | ||
| public init(_ urlString: String, _ options: ClientOptions = ClientOptions()) { | ||
| public nonisolated init(_ urlString: String, _ options: ClientOptions = ClientOptions()) { |
There was a problem hiding this comment.
Marking initializer as nonisolated.
Since Client is no longer an actor, marking the initializer as nonisolated allows it to be called from any thread context, which is necessary for synchronous operations but requires careful management to avoid race conditions.
Consider ensuring that all accesses to mutable state within Client are thread-safe, possibly using synchronization mechanisms like locks or serial queues.
| @@ -400,7 +398,7 @@ public actor Client { | |||
| * `changeSyncMode` changes the synchronization mode of the given document. | |||
| */ | |||
| @discardableResult | |||
| public func changeSyncMode(_ doc: Document, _ syncMode: SyncMode) async throws -> Document { | |||
| public func changeSyncMode(_ doc: Document, _ syncMode: SyncMode) throws -> Document { | |||
There was a problem hiding this comment.
Modification of synchronization mode handling.
The removal of asynchronous handling in changeSyncMode and adjustments to the synchronization logic are significant. It's essential to ensure that these changes do not introduce any unintended behaviors or race conditions, especially since synchronization settings are critical for the correct operation of the client.
Verify that the new synchronization logic correctly handles all edge cases and that there are no race conditions introduced by these changes.
Also applies to: 431-431
What this PR does / why we need it:
The client and document generated by the factor should be called asynchronously.
In this case, it cannot be used for a structure that must be handled synchronously by the main thread.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Refactor
Clientstruct to a class and improved initialization and sync handling.Documentfrom an actor to a class with enhanced property and method structures.CRDTTreefor better readability and performance.IndexTreeto use more expressive constructs for iterations.Tests
DocumentTestsby removing unnecessary nestedTaskblocks.