Sync android-sdk with js-sdk: Implement updates to v0.6.10#230
Conversation
|
Warning Rate limit exceeded@ntcong1997 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (17)
WalkthroughThis set of changes refactors core CRDT logic by removing max-created-at maps and deprecated fields, simplifies version vector and timestamp management, and updates protobuf schema and converters accordingly. The CI workflow adds an instrumentation test job and uses a script for Yorkie Server URL setup. Tests and utilities are adapted for new APIs, and dependency versions are updated. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions CI
participant Script as config-yorkie-local-server.sh
participant Gradle as Gradle Build
participant Emulator as Android Emulator
participant Jacoco as Jacoco Coverage
CI->>Script: Set Yorkie Server URL
CI->>Gradle: Run unit and instrumentation tests
Gradle->>Emulator: Start and reuse AVD
Gradle->>Gradle: Run connectedCheck (instrumentation tests)
Gradle->>Jacoco: Generate coverage report
Jacoco-->>CI: Upload coverage artifact
sequenceDiagram
participant Client as Client
participant Attachment as Attachment
participant Document as Document
loop runSyncLoop
Client->>Attachment: Filter attachments needing real-time sync
alt remoteChangeEventReceived
Attachment->>Attachment: Reset remoteChangeEventReceived
end
Client->>Document: Push-Pull changes for each attachment
Document-->>Client: Apply change pack
alt Document removed
Client->>Attachment: Remove attachment and mutex
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ntcong1997
force-pushed
the
v0.6.10
branch
2 times, most recently
from
ef22121 to
8ed8bec
Compare
![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
🧹 Nitpick comments (5)
yorkie/build.gradle.kts (1)
147-147: Consider improving developer experience with better error messaging.While the error handling works, the error message could be more helpful for developers setting up the project.
- buildConfigField("String", "YORKIE_SERVER_URL", "\"${localProperties.getProperty("YORKIE_SERVER_URL") ?: error("YORKIE_SERVER_URL missing in local.properties")}\"") + buildConfigField("String", "YORKIE_SERVER_URL", "\"${localProperties.getProperty("YORKIE_SERVER_URL") ?: error("YORKIE_SERVER_URL missing in local.properties. Please add YORKIE_SERVER_URL=your_server_url to local.properties file in the root directory.")}\"")Consider also adding a comment above this line or in the README about the required local.properties setup.
yorkie/src/main/kotlin/dev/yorkie/document/time/VersionVector.kt (1)
9-14: Good defensive copying but consider improving encapsulation.The refactoring from
data classto regular class with explicit copying in theinitblock is well-implemented and prevents external mutation of the input map. However, consider makingvectorMapan immutableMap<String, Long>instead ofMutableMapfor better encapsulation, since the class provides specific methods (set,unset) for mutations.- val vectorMap: MutableMap<String, Long> = mutableMapOf() + private val _vectorMap: MutableMap<String, Long> = mutableMapOf() + val vectorMap: Map<String, Long> get() = _vectorMapThen update internal usages to use
_vectorMapfor mutations.yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt (1)
736-737: Consider making the delay duration configurable or using a constant.The hardcoded 1-second delay might make tests unnecessarily slow and could be flaky if timing requirements change.
- delay(1000) + delay(PRESENCE_INITIALIZATION_DELAY)Add a constant at the class level:
companion object { private const val PRESENCE_INITIALIZATION_DELAY = 1000L }Also applies to: 775-776
.github/workflows/ci.yml (1)
103-108: Consider extracting test execution commands to a script.The multiple gradle commands with complex arguments could be moved to a script for better maintainability.
Create a script
scripts/run-instrumentation-tests.sh:#!/bin/bash adb shell pm list packages | grep dev.yorkie.test && adb uninstall dev.yorkie.test || true ./gradlew yorkie:connectedCheck -Pandroid.testInstrumentationRunnerArguments.notAnnotation=dev.yorkie.TreeTest --no-build-cache --no-daemon --stacktrace ./gradlew yorkie:connectedCheck -Pandroid.testInstrumentationRunnerArguments.class=dev.yorkie.document.json.JsonTreeTest --no-build-cache --no-daemon --stacktrace # ... other commandsThen use in the workflow:
script: ./scripts/run-instrumentation-tests.shyorkie/src/main/kotlin/dev/yorkie/core/Client.kt (1)
226-241: Consider extracting the filtering logic to a named function.The complex filtering and mapping logic could be extracted for better readability.
- val attachments = attachments.value.entries.mapNotNull { - if (it.value.needRealTimeSync()) { - if (it.value.remoteChangeEventReceived) { - AttachmentEntry( - key = it.key, - value = it.value.copy( - remoteChangeEventReceived = false, - ), - ) - } else { - it - } - } else { - null - } - } + val attachments = filterAttachmentsForSync()Add a private function:
private fun filterAttachmentsForSync(): List<Entry<Document.Key, Attachment>> { return attachments.value.entries.mapNotNull { if (it.value.needRealTimeSync()) { if (it.value.remoteChangeEventReceived) { AttachmentEntry( key = it.key, value = it.value.copy( remoteChangeEventReceived = false, ), ) } else { it } } else { null } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.github/workflows/ci.yml(3 hunks)gradle/libs.versions.toml(1 hunks)microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt(10 hunks)yorkie.rb(2 hunks)yorkie/build.gradle.kts(3 hunks)yorkie/proto/yorkie/v1/resources.proto(5 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt(1 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/DocumentTest.kt(1 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt(7 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt(4 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt(4 hunks)yorkie/src/main/kotlin/dev/yorkie/api/ChangeConverter.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/api/OperationConverter.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Client.kt(3 hunks)yorkie/src/main/kotlin/dev/yorkie/document/Document.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/document/change/ChangeID.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt(2 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtText.kt(4 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt(8 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt(7 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplitInfo.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/TextInfo.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/document/json/JsonText.kt(2 hunks)yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt(3 hunks)yorkie/src/main/kotlin/dev/yorkie/document/operation/EditOperation.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/document/operation/StyleOperation.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeEditOperation.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt(0 hunks)yorkie/src/main/kotlin/dev/yorkie/document/time/ActorID.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/document/time/VersionVector.kt(2 hunks)yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt(0 hunks)yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt(2 hunks)
💤 Files with no reviewable changes (11)
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplitInfo.kt
- yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeEditOperation.kt
- yorkie/src/main/kotlin/dev/yorkie/api/ChangeConverter.kt
- yorkie/src/main/kotlin/dev/yorkie/document/operation/EditOperation.kt
- yorkie/src/main/kotlin/dev/yorkie/document/change/ChangeID.kt
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/TextInfo.kt
- yorkie/src/main/kotlin/dev/yorkie/document/operation/StyleOperation.kt
- yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt
- yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt
- yorkie/src/main/kotlin/dev/yorkie/api/OperationConverter.kt
🧰 Additional context used
🧬 Code Graph Analysis (2)
yorkie/src/androidTest/kotlin/dev/yorkie/core/DocumentTest.kt (1)
yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt (1)
withTwoClientsAndDocuments(80-118)
yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt (1)
yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt (1)
withTwoClientsAndDocuments(80-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Checks
- GitHub Check: Instrumentation tests (24, google_apis)
🔇 Additional comments (45)
yorkie.rb (2)
4-5: LGTM: Version bump to v0.6.10The URL and SHA256 checksum updates are consistent with the version bump from 0.6.9 to 0.6.10.
15-20: LGTM: Bottle checksums updatedAll platform-specific bottle SHA256 checksums have been updated to match the new v0.6.10 release.
gradle/libs.versions.toml (1)
12-12: Approve: Upgrade AndroidX Benchmark to 1.4.0 – verify Kotlin plugin versionThis bump to 1.4.0 brings several enhancements over 1.3.0:
- TraceProcessor API changes:
timeout→timeoutMsand constructor now internal (usestartServer/runServer).- Macrobenchmark startup insights for detecting CPU/main-thread issues.
- Improved
killProcessvalidation to avoid silent failures.- Emulator compatibility flag to skip baseline profile tests on emulators.
- Now targets Kotlin 2.0; requires Kotlin Gradle Plugin 2.0.0+.
• Please confirm or update your project’s Kotlin Gradle Plugin to version 2.0.0 or later before merging.
yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt (2)
4-4: LGTM: Updated import for ByteString extensionThe import change from
TimeTickettocom.google.protobuf.kotlin.isNotEmptyreflects the removal of deprecated components and use of more idiomatic Kotlin extensions.
34-34: LGTM: Improved hasSnapshot implementationUsing
isNotEmpty()extension is more idiomatic Kotlin and provides the same functionality as the previous manual empty check.microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt (1)
36-36: LGTM: API migration to runWithMeasurementDisabledThe consistent replacement of
runWithTimingDisabledwithrunWithMeasurementDisabledthroughout all benchmark methods aligns with the AndroidX Benchmark library update to v1.4.0. The new function name is more descriptive and accurate.Also applies to: 59-59, 62-62, 85-85, 88-88, 103-103, 119-119, 175-175, 193-193, 204-204, 216-216, 247-247
yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt (1)
151-170: LGTM: Enhanced test synchronizationThe additional synchronization waits significantly improve test reliability by ensuring:
- Changes are fully propagated between clients
- State modifications are completely applied before assertions
- Both clients receive their respective sync events
This addresses potential race conditions in multi-client scenarios and aligns with the PR's goal of fixing Android test stability.
yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt (3)
283-283: LGTM: Consistent removal of maxCreatedAtMapByActor tracking.The change from triple to pair destructuring correctly reflects the updated return type from the underlying
target.stylemethod, aligning with the systematic removal ofmaxCreatedAtMapByActorthroughout the codebase.
309-309: LGTM: Consistent with the maxCreatedAtMapByActor removal pattern.The destructuring change matches the pattern applied to other similar operations in this refactoring effort.
403-403: LGTM: Final piece of the maxCreatedAtMapByActor removal in this file.The edit operation destructuring is now consistent with the style operations, completing the refactoring for this file.
yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt (2)
153-158: LGTM: Updated test assertions reflect new version vector handling.The test correctly expects a populated
VersionVectorwith the test actor ID mapped to version 2, which aligns with the simplified version vector management approach in this refactoring.
285-290: LGTM: Consistent version vector test update for document removal.The test assertion correctly matches the pattern established in the sync request test, ensuring consistent expectations across different operations.
yorkie/src/main/kotlin/dev/yorkie/document/json/JsonText.kt (2)
59-59: LGTM: Consistent maxCreatedAtMapByActor removal in text editing.The destructuring change correctly reflects the updated
target.editmethod signature, maintaining consistency with the broader refactoring effort.
109-109: LGTM: Consistent with text style operation updates.The change aligns with the pattern applied throughout the codebase for removing
maxCreatedAtMapByActortracking from style operations.yorkie/src/main/kotlin/dev/yorkie/document/time/ActorID.kt (1)
7-7: Data class ActorID performance impact is negligibleThe switch from
@JvmInline value classto adata classwill not materially degrade performance because almost every use ofActorID(in maps, sets, converters, CRDT operations, etc.) already incurs boxing under generics. Keeping it as a data class simplifies the model without sacrificing runtime characteristics.yorkie/build.gradle.kts (2)
1-1: LGTM: Required import for Properties handling.The import is necessary for the new local.properties loading functionality.
128-134: LGTM: Clean implementation of local.properties loading.The implementation properly checks for file existence before attempting to load properties, preventing build failures when the file doesn't exist.
yorkie/src/androidTest/kotlin/dev/yorkie/core/DocumentTest.kt (1)
122-124: LGTM: Improved test readability with named parameter.Using the named parameter
detachDocuments = falsemakes the test intent clearer and aligns with the updated test utility function signature.yorkie/src/main/kotlin/dev/yorkie/document/time/VersionVector.kt (1)
101-109: LGTM: Proper equals and hashCode implementation.The explicit
equalsandhashCodeoverrides correctly maintain equality semantics based on the internal map, preserving the behavior that was previously provided by thedata class.yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt (1)
12-25: Excellent refactoring for improved clarity.The explicit handling of each sync mode makes the logic much more readable and maintainable. The conditions are correct:
RealtimeSyncOff: Never needs syncRealtimePushOnly: Only syncs when there are local changes to push- Other modes: Sync when not manual and there are changes or remote events
This is a significant improvement over the previous combined conditional expression.
yorkie/src/main/kotlin/dev/yorkie/document/Document.kt (1)
370-372: LGTM: Clean formatting improvement.The formatting adjustment improves code readability. The broader changes mentioned in the summary (removal of
filterVersionVectormethod) align well with the PR objectives to simplify version vector handling and remove deprecated synchronization components.yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt (2)
3-3: Excellent improvement: Configurable server URL.Using
BuildConfig.YORKIE_SERVER_URLinstead of a hardcoded localhost URL makes the tests more flexible and suitable for different environments including CI/CD pipelines. This aligns well with the new instrumentation test CI job mentioned in the PR.Also applies to: 20-20
121-121: Good enhancement: Configurable sync modes.Adding the
syncModeparameter with a sensible default (Client.SyncMode.Realtime) improves test flexibility while maintaining backward compatibility. The explicit parameter passing in theattachAsynccalls makes the test behavior clearer and more maintainable.Also applies to: 145-156
yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt (2)
708-708: Sync mode change aligns with the default behavior update.The change from
ManualtoRealtimesync mode is consistent with the updatedwithTwoClientsAndDocumentsutility function's new default behavior.
777-783: Good addition of initial state verification.The new assertions verify the initial event count and type before presence updates, which improves test robustness by ensuring the starting state is as expected.
.github/workflows/ci.yml (1)
22-22: Good refactor to use configuration script.Moving the Yorkie server URL configuration to a dedicated script improves maintainability and consistency across jobs.
Also applies to: 40-40, 67-67
yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt (4)
159-159: Correct update to use dynamic version vectors.Using
maxLamportVersionVector(document)instead of a fixed version vector ensures garbage collection works correctly with the document's actual state.Also applies to: 181-181, 204-204
630-646: Updated garbage collection counts reflect the new version vector behavior.The changes in expected garbage lengths (from 2 to 3, then 3 to 4) correctly reflect the impact of removing the
minSyncedTicketfield on garbage collection behavior.
1079-1091: Well-implemented helper function for multi-actor version vectors.The
maxVersionVectorfunction correctly handles multiple actors and provides a clean way to create version vectors for garbage collection in multi-client scenarios.
937-938: Good addition of explicit sync mode parameter.Making the sync mode explicit in the test improves clarity and ensures the test behavior is predictable.
yorkie/src/main/kotlin/dev/yorkie/core/Client.kt (2)
226-242: Clean refactoring of sync filtering logic.Moving the filtering logic to
runSyncLoop()and resettingremoteChangeEventReceivedbefore creating the flow improves code clarity and ensures proper state management.
312-314: Simplified extension function improves maintainability.Removing the filtering logic from
asSyncFlow()makes it a pure transformation function, which is easier to understand and test.yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtText.kt (2)
65-65: LGTM! Consistent removal ofmaxCreatedAtMapByActor.The changes correctly update the destructuring and return statement to align with the removal of
maxCreatedAtMapByActortracking from the codebase.Also applies to: 85-85
108-110: Clean simplification of concurrency control logic.The refactoring effectively simplifies the lamport timestamp retrieval by directly using the version vector instead of maintaining a separate map. The fallback to
MAX_LAMPORTensures backward compatibility when no version vector is provided.Also applies to: 113-113, 138-138
yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt (5)
62-62: Consistent API simplification in edit method.The changes correctly update the
deleteNodescall and return statement to align with the removal ofmaxCreatedAtMapByActortracking.Also applies to: 98-98
193-193: Clean removal of unused return value.The method correctly simplifies the return type from
TripletoPairby removing the unusedmaxCreatedAtMapByActor.Also applies to: 195-195, 218-218
235-237: Consistent concurrency control simplification.The changes correctly implement the same simplified lamport timestamp retrieval pattern used throughout the codebase.
Also applies to: 239-239
529-529: Simplified concurrency check maintains correctness.The method correctly uses the client's lamport timestamp directly for the existence check, maintaining the same concurrency control semantics with less complexity.
Also applies to: 532-532
543-544: Consistent simplification across style operations.The method correctly mirrors the
canDeletechanges, maintaining API consistency.yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt (6)
91-91: Consistent refactoring in tree style operations.The changes correctly implement the simplified concurrency control pattern, maintaining consistency with the text CRDT implementation.
Also applies to: 93-93, 121-121
197-197: Clean implementation of simplified concurrency control.The edit method correctly adopts the simplified pattern for retrieving client lamport timestamps and checking deletion permissions.
Also applies to: 199-201, 298-298
430-430: Consistent pattern in removeStyle implementation.The method correctly follows the established pattern for simplified concurrency control.
Also applies to: 432-434, 457-457
745-749: Excellent simplification of the helper method.The
getClientInfoForChangemethod has been elegantly simplified to return just the client's lamport timestamp, making the code more maintainable and easier to understand. The null-safe implementation with MAX_LAMPORT fallback ensures robust behavior.
947-948: Consistent node-level concurrency control.The
canDeletemethod correctly implements the simplified concurrency check pattern.
952-952: Complete and consistent refactoring.The
canStylemethod correctly completes the pattern of simplified concurrency control across all tree node operations.Also applies to: 956-956
ntcong1997
force-pushed
the
v0.6.10
branch
3 times, most recently
from
e630a67 to
ebc9bad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/ci.yml (4)
21-23: Make the script invocation more robustIf
scripts/config-yorkie-local-server.shever loses the executable bit (e.g. during a Windows commit) this step will hard-fail.
Call it explicitly withbash(or make the file executable at checkout) to avoid brittle permission issues.- - name: Set Yorkie Server Url - run: ./scripts/config-yorkie-local-server.sh + - name: Set Yorkie Server Url + run: bash ./scripts/config-yorkie-local-server.sh
39-41: Avoid copy-pasting identical steps – extract to a reusable snippetThe “Set Yorkie Server Url” block is now duplicated in three jobs.
Consider a YAML anchor/alias or a composite action to keep the workflow DRY and simplify future maintenance.
82-83: Re-enable Gradle build cache for faster instrumentation runs
--no-build-cacheerases one of the biggest CI accelerators and forces a full compile of every module for each API-level matrix entry.
Unless you hit a reproducibility bug, drop the flag:- ./gradlew yorkie:connectedCheck --no-build-cache --no-daemon --stacktrace + ./gradlew yorkie:connectedCheck --no-daemon --stacktrace
85-90: Extraneous conditionals make future matrix expansion harderThe
if: ${{ matrix.api-level == 33 }}guards are redundant because the matrix currently only lists level 33.
When you add another API level you’ll need to touch three lines. Remove the condition now or parametrize it.- - if: ${{ matrix.api-level == 33 }} - run: ./gradlew yorkie:jacocoDebugTestReport + - run: ./gradlew yorkie:jacocoDebugTestReport
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ci.yml(3 hunks)gradle/libs.versions.toml(1 hunks)microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt(10 hunks)yorkie.rb(2 hunks)yorkie/build.gradle.kts(3 hunks)yorkie/proto/yorkie/v1/resources.proto(5 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt(1 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt(7 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt(4 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt(4 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Client.kt(3 hunks)yorkie/src/main/kotlin/dev/yorkie/document/Document.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt(2 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt(8 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt(7 hunks)yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt
- gradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (14)
- yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt
- yorkie.rb
- yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/main/kotlin/dev/yorkie/document/Document.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt
- yorkie/build.gradle.kts
- yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt
- yorkie/proto/yorkie/v1/resources.proto
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Instrumentation tests (33, google_apis)
- GitHub Check: Unit tests
- GitHub Check: Checks
ntcong1997
force-pushed
the
v0.6.10
branch
2 times, most recently
from
e9af6b0 to
29bec7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
70-72:docker system prune --all --forceis still overly aggressive – previous warning remains validThe command indiscriminately removes every Docker image/container/network on the runner, which can:
• wipe layers pulled/built by earlier jobs (e.g., the Android emulator image),
• increase build times by re-pulling large images,
• intermittently break dependent steps.Scope the cleanup to dangling images only, or drop it entirely unless a clear disk-pressure issue is proven.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
73-88: Manual emulator bootstrap is fragile; prefer the publishedandroid-emulator-runneractionCreating and managing the AVD yourself adds ~30 lines of shell and several minutes of setup time, plus points of failure (e.g., waiting only for
adb wait-for-device, notsys.boot_completed, hard-coding paths, missingcmdline-tools;latestinstall).A leaner, battle-tested alternative:
- - name: Install system image and create AVD - run: | - echo "y" | $ANDROID_HOME/cmdline-tools/latest/bin/sdkmanager \ - "platform-tools" "platforms;android-33" \ - "system-images;android-33;google_apis;x86_64" - export PATH="$ANDROID_HOME/cmdline-tools/latest/bin:$PATH" - echo "no" | $ANDROID_HOME/cmdline-tools/latest/bin/avdmanager create avd \ - -n test -k "system-images;android-33;google_apis;x86_64" \ - --device "pixel" --force - - - name: Start emulator - run: | - export PATH="$ANDROID_HOME/emulator:$ANDROID_HOME/platform-tools:$ANDROID_HOME/cmdline-tools/latest/bin:$PATH" - $ANDROID_HOME/emulator/emulator -avd test -no-audio -no-window -no-snapshot -gpu swiftshader_indirect & - adb wait-for-device - adb shell settings put global window_animation_scale 0 - adb shell settings put global transition_animation_scale 0 - adb shell settings put global animator_duration_scale 0 + - uses: reactivecircus/android-emulator-runner@v2 + with: + api-level: ${{ matrix.api-level }} + target: ${{ matrix.target }} + arch: x86_64 + disable-animations: trueBenefits: faster spin-up (pre-cached images), automatic boot/health checks, less YAML noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ci.yml(3 hunks)gradle/libs.versions.toml(1 hunks)microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt(10 hunks)yorkie.rb(2 hunks)yorkie/build.gradle.kts(3 hunks)yorkie/proto/yorkie/v1/resources.proto(5 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt(1 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt(7 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt(4 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt(4 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Client.kt(3 hunks)yorkie/src/main/kotlin/dev/yorkie/document/Document.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt(2 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt(8 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt(7 hunks)yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt
- yorkie.rb
🚧 Files skipped from review as they are similar to previous changes (14)
- yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt
- gradle/libs.versions.toml
- yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt
- yorkie/build.gradle.kts
- yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt
- yorkie/src/main/kotlin/dev/yorkie/document/Document.kt
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt
- yorkie/proto/yorkie/v1/resources.proto
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Unit tests
- GitHub Check: Instrumentation tests (33, google_apis)
- GitHub Check: Checks
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
90-96: Coverage generation is gated to a single API level – future matrix rows will skip it
jacocoDebugTestReportand artifact upload run only whenmatrix.api-level == 33.
If additional API levels are added later, those jobs will silently skip coverage, lowering overall report accuracy.Consider running coverage unconditionally on the first (or any) matrix job:
- if: ${{ matrix.api-level == fromJson('["33"]')[0] }}or move coverage to a separate, non-matrix step after the emulator has shut down.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
70-72:docker system prune --all --forceis still dangerously over-scoped
This is the exact issue raised in the previous review and remains unresolved.
It can wipe layers required by subsequent steps (e.g. the Android-emulator runner) and adds minutes of image re-pulls.- - run: | - docker system prune --all --force - docker compose -f docker/docker-compose-ci.yml up --build -d + - run: | + # Remove only dangling layers instead of *everything* on the runner. + docker image prune -f + docker compose -f docker/docker-compose-ci.yml up --build -d
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
55-57:fail-fast: falseis redundant for a single-value matrixWith only one
api-level/targetcombination the matrix never fans out, sofail-fasthas no effect and just clutters the YAML.
88-94: Limit-scoped coverage upload may break once the matrix growsCoverage is generated and uploaded only when
api-level == 33.
If more API levels are added later, their coverage will be silently ignored and the Codecov job will miss part of the data.- - if: ${{ matrix.api-level == 33 }} - run: ./gradlew yorkie:jacocoDebugTestReport - - if: ${{ matrix.api-level == 33 }} - uses: actions/upload-artifact@v4 + - run: ./gradlew yorkie:jacocoDebugTestReport + - uses: actions/upload-artifact@v4This keeps the condition unnecessary and future-proofs the workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ci.yml(3 hunks)gradle/libs.versions.toml(1 hunks)microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt(10 hunks)yorkie.rb(2 hunks)yorkie/build.gradle.kts(3 hunks)yorkie/proto/yorkie/v1/resources.proto(5 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt(1 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt(7 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt(4 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt(4 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Client.kt(3 hunks)yorkie/src/main/kotlin/dev/yorkie/document/Document.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt(2 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt(8 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt(7 hunks)yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- yorkie.rb
- microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt
🚧 Files skipped from review as they are similar to previous changes (14)
- yorkie/src/main/kotlin/dev/yorkie/document/Document.kt
- yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt
- yorkie/build.gradle.kts
- yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt
- gradle/libs.versions.toml
- yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt
- yorkie/proto/yorkie/v1/resources.proto
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Checks
- GitHub Check: Unit tests
- GitHub Check: Instrumentation tests (33, google_apis)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
21-22: Centralising Yorkie-server URL setup looks goodReplacing the inline
echowith a dedicated script keeps the workflow DRY and avoids duplicated logic.
Nice touch.Also applies to: 39-40
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
70-72:docker system prune --all --forceis still dangerously broad
This exact issue was raised in an earlier review; it nukes every image/layer on the runner and regularly breaks subsequent Android-emulator steps or lengthens the workflow.
Replace with a scoped prune (e.g. dangling images only) or drop it entirely.
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
21-22: Ensure the helper script is executable and fails fast on errorBoth jobs invoke
./scripts/config-yorkie-local-server.sh, but the permissions of the script are not set explicitly andset -eis not enforced inside the script.
Add an explicitchmod +x(or commit-time executable bit) andset -euo pipefailat the top of the script to avoid silent mis-configurations.Also applies to: 40-41
74-78: Installation step misses license acceptance & repeats heavy downloadsDownloading the system image on every run costs 2-3 min and sometimes fails on flaky mirrors.
Prefer GitHub’s cacheablereactivecircus/android-emulator-runneror pre-install via a reusable action:- uses: reactivecircus/android-emulator-runner@v2 with: api-level: 33 target: google_apis arch: x86_64 script: ./gradlew yorkie:connectedDebugAndroidTestThis handles license acceptance, caching and boot-waiting automatically.
80-86: Emulator might start before the system is fully booted
adb wait-for-deviceonly guarantees a transport; the framework may still be booting, causing random test flakiness.
Insert an explicit boot-complete poll:adb wait-for-device adb shell 'while [[ $(getprop sys.boot_completed) != "1" ]]; do sleep 1; done'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ci.yml(3 hunks)gradle/libs.versions.toml(1 hunks)microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt(10 hunks)yorkie.rb(2 hunks)yorkie/build.gradle.kts(3 hunks)yorkie/proto/yorkie/v1/resources.proto(5 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt(1 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt(7 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt(4 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt(4 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Client.kt(3 hunks)yorkie/src/main/kotlin/dev/yorkie/document/Document.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt(2 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt(8 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt(7 hunks)yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt
🚧 Files skipped from review as they are similar to previous changes (14)
- yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/build.gradle.kts
- yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt
- yorkie/src/main/kotlin/dev/yorkie/document/Document.kt
- yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt
- gradle/libs.versions.toml
- yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt
- yorkie/proto/yorkie/v1/resources.proto
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt
- yorkie.rb
- yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Instrumentation tests (33, google_apis)
- GitHub Check: Unit tests
- GitHub Check: Checks
🔇 Additional comments (4)
yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt (4)
73-122: LGTM: Simplified concurrency control in style method.The removal of
maxCreatedAtMapByActorparameter simplifies the concurrency control logic by relying solely on client lamport timestamps from the version vector. This aligns with the broader refactoring to eliminate complex per-actor maximum creation timestamp tracking.
165-299: LGTM: Consistent refactoring in edit method.The changes follow the same pattern as the
stylemethod, removingmaxCreatedAtMapByActorcomplexity while maintaining correct concurrency semantics through client lamport timestamp checks.
417-458: LGTM: Consistent pattern applied to removeStyle method.The refactoring maintains consistency across all tree operation methods by removing
maxCreatedAtMapByActorcomplexity.
745-749: LGTM: Simplified helper methods maintain correct semantics.The
getClientInfoForChangemethod now returns a simpleLongvalue (client lamport timestamp) instead of complex timestamp mappings, and thecanDelete/canStylemethods are updated accordingly. The concurrency logic usingcreatedAt.lamport <= clientLamportAtChangecorrectly determines node existence at the time of change.Also applies to: 947-958
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
70-72:docker system prune --all --forceis still dangerously broad(Previously raised, still unresolved.)
It wipes every image/network/container on the runner and can:
- Remove layers needed by
reactivecircus/android-emulator-runner,- Add several extra minutes to re-pull images,
- Make local debugging harder.
- docker system prune --all --force + # Remove only dangling layers to free space safely + docker image prune -f
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
39-41: Remove duplication: reuse the “Set Yorkie Server Url” stepThe same script is invoked in two jobs (Lines 21-23 and 39-41). Consider extracting the logic into:
- A YAML anchor/alias, or
- A small composite action shared by all jobs.
This keeps the workflow DRY and reduces maintenance overhead.
73-80: AVD creation downloads ~1 GB every run – cache the system imageDownloading the 33 google_apis x86_64 system image on every CI run lengthens the job by several minutes.
You can cache the emulator directory (≈$ANDROID_HOME/system-images/android-33/google_apis/x86_64) withactions/cache, keyed onapi-level+target. This cuts cold-start time for subsequent runs.
88-96: Make coverage upload deterministic when multiple XMLs existBoth unit and instrumentation tests write the report to the same path. When future API levels are added to the matrix you may end up with several XMLs of identical name; Codecov can pick one at random. Consider:
- Giving each matrix run a unique report filename, or
- Merging XMLs before the
codecovjob (e.g., withjacoco-cli merge).Also applies to: 98-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ci.yml(3 hunks)gradle/libs.versions.toml(1 hunks)microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt(10 hunks)yorkie.rb(2 hunks)yorkie/build.gradle.kts(3 hunks)yorkie/proto/yorkie/v1/resources.proto(5 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt(1 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt(7 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt(4 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt(4 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Client.kt(3 hunks)yorkie/src/main/kotlin/dev/yorkie/document/Document.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt(2 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt(8 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt(7 hunks)yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt
🚧 Files skipped from review as they are similar to previous changes (15)
- yorkie.rb
- gradle/libs.versions.toml
- yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt
- yorkie/build.gradle.kts
- yorkie/proto/yorkie/v1/resources.proto
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt
- yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt
- yorkie/src/main/kotlin/dev/yorkie/document/Document.kt
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Checks
- GitHub Check: Unit tests
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
21-23: Ensureconfig-yorkie-local-server.shtruly persists the Yorkie URL across subsequent stepsIf the script only exports an environment variable, the value will be lost after the step completes because each
run:step is executed in its own shell.
Verify that the script instead writes tolocal.propertiesor appends to$GITHUB_ENV, otherwise the Gradle tasks that follow will not seeYORKIE_SERVER_URL.
ntcong1997
force-pushed
the
v0.6.10
branch
2 times, most recently
from
0cceada to
a779cf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
39-41: Same comment as above – the script-based URL configuration is a welcome cleanup.
70-72:docker system prune --all --forceis still riskyPrior feedback on the exact command (see previous review) remains unresolved.
Consider limiting the prune to dangling images only:- docker system prune --all --force + docker image prune -f
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
73-111: Use an emulator GitHub Action instead of hand-rolled setupManually installing SDK components, creating an AVD, and starting the emulator adds ~2-3 min per run and is fragile (tooling paths change frequently).
reactivecircus/android-emulator-runner@v2encapsulates all this, handles boot-timeouts, and caches system images.Example drop-in replacement:
- name: Start emulator and run tests uses: reactivecircus/android-emulator-runner@v2 with: api-level: ${{ matrix.api-level }} target: ${{ matrix.target }} arch: x86_64 disable-animations: true script: ./gradlew yorkie:connectedDebugAndroidTestReducing boilerplate will make future API-level bumps or parallel matrix expansion smoother.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/ci.yml(3 hunks)gradle/libs.versions.toml(1 hunks)microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt(10 hunks)yorkie.rb(2 hunks)yorkie/build.gradle.kts(3 hunks)yorkie/proto/yorkie/v1/resources.proto(5 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt(1 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt(7 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt(4 hunks)yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt(4 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/core/Client.kt(3 hunks)yorkie/src/main/kotlin/dev/yorkie/document/Document.kt(1 hunks)yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt(2 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt(8 hunks)yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt(7 hunks)yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- microbenchmark/src/androidTest/kotlin/dev/yorkie/microbenchmark/DocumentBenchmark.kt
🚧 Files skipped from review as they are similar to previous changes (15)
- yorkie/src/androidTest/kotlin/dev/yorkie/core/ClientTest.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/PresenceTest.kt
- yorkie/src/test/kotlin/dev/yorkie/core/ClientTest.kt
- gradle/libs.versions.toml
- yorkie/src/main/kotlin/dev/yorkie/document/Document.kt
- yorkie/src/main/kotlin/dev/yorkie/document/change/ChangePack.kt
- yorkie/build.gradle.kts
- yorkie/src/main/kotlin/dev/yorkie/core/Attachment.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/TestUtils.kt
- yorkie/src/androidTest/kotlin/dev/yorkie/core/GCTest.kt
- yorkie/src/main/kotlin/dev/yorkie/core/Client.kt
- yorkie.rb
- yorkie/proto/yorkie/v1/resources.proto
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/RgaTreeSplit.kt
- yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Checks
- GitHub Check: Unit tests
- GitHub Check: Instrumentation tests (33, google_apis)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
21-23: Good move to externalize Yorkie server URL setupReplacing the inline export with a dedicated script keeps the workflow definition leaner and allows re-use across jobs.
121-124: Codecov dependency update looks correctAdding
instrumentation-teststo theneedsarray ensures coverage from both unit and instrumented tests is reported—nice catch.
ntcong1997
force-pushed
the
v0.6.10
branch
4 times, most recently
from
3c1e764 to
e4e54f7
Compare
ntcong1997
force-pushed
the
v0.6.10
branch
3 times, most recently
from
cb93a86 to
7d3e39c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #230 +/- ##
============================================
+ Coverage 76.03% 81.60% +5.56%
============================================
Files 72 72
Lines 4961 4848 -113
Branches 777 754 -23
============================================
+ Hits 3772 3956 +184
+ Misses 777 532 -245
+ Partials 412 360 -52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ntcong1997
requested review from
daeyounglnc
and removed request for
daeyounglnc
| } else { | ||
| maxCreatedAt = maxCreatedAtMapByActor?.get(actorID) ?: InitialTimeTicket | ||
| } | ||
| val clientLamportAtChange = versionVector?.let { |
| if (max == null || max < createdAt) { | ||
| createdAtMapByActor[actorID] = createdAt | ||
| } | ||
| val clientLamportAtChange = getClientInfoForChange(actorID, versionVector) |
3 participants
What this PR does / why we need it?
Any background context you want to provide?
What are the relevant tickets?
Fixes https://jira.navercorp.com/browse/RTCOLLABPLATFORM-320, https://jira.navercorp.com/browse/RTCOLLABPLATFORM-348
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests
Style