refactor: replace current() function with current property in Occupancy and Typing APIs
#177
Conversation
WalkthroughPublic API signatures changed: Occupancy.current() and Typing.current() were converted to read-only properties (exposed as getCurrent on JVM/Android), Typing.getChannel()/channel exposure removed (DefaultTyping.channel made internal), and call sites/tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant TypingAPI as Typing (interface)
participant DefaultTyping as DefaultTyping
participant OccupancyAPI as Occupancy (interface)
participant DefaultOccupancy as DefaultOccupancy
Client->>TypingAPI: access current
Note right of TypingAPI: property access → getCurrent()
TypingAPI->>DefaultTyping: getCurrent()
DefaultTyping-->>Client: Set<String>
Client->>OccupancyAPI: access current
Note right of OccupancyAPI: property access → getCurrent()
OccupancyAPI->>DefaultOccupancy: getCurrent()
alt occupancy events enabled
DefaultOccupancy-->>Client: OccupancyData?
else events disabled
DefaultOccupancy-->>Client: throws (events disabled)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
ttypic
force-pushed
the
ECO-5434/current-to-prop
branch
from
1884c06 to
d0be280
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: 1
🧹 Nitpick comments (3)
chat/src/commonMain/kotlin/com/ably/chat/Typing.kt (2)
211-213: Nit: log message still references a functionUpdate to reflect property access.
- logger.trace("DefaultTyping.current()") + logger.trace("DefaultTyping.current")
349-351: Avoid recomputing current for each listenercurrent creates a new Set; compute once per emit.
- listeners.forEach { - it.invoke(DefaultTypingEvent(current, typingEventChange)) - } + val snapshot = current + listeners.forEach { + it.invoke(DefaultTypingEvent(snapshot, typingEventChange)) + }chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
166-176: Getter logic looks good; minor logging nitEvent-enabled guard and returning the last value is correct. Update the log message to match property access.
- logger.trace("Occupancy.current()") + logger.trace("Occupancy.current")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
chat/api/android/chat.api(2 hunks)chat/api/jvm/chat.api(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Typing.kt(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-28T11:10:20.947Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Typing.kt:178-183
Timestamp: 2024-11-28T11:10:20.947Z
Learning: In `chat-android/src/main/java/com/ably/chat/Typing.kt`, within the `DefaultTyping` class, the `stop()` method must execute within `typingScope` (a `CoroutineScope` with parallelism set to 1) to avoid race conditions when setting and cancelling `typingJob`.
Applied to files:
chat/api/jvm/chat.apichat/src/commonMain/kotlin/com/ably/chat/Typing.ktchat/api/android/chat.api
📚 Learning: 2024-11-28T11:11:20.423Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Occupancy.kt:145-145
Timestamp: 2024-11-28T11:11:20.423Z
Learning: In `chat-android/src/main/java/com/ably/chat/Occupancy.kt`, within the `DefaultOccupancy` class, when methods use `room.chatApi`, which utilizes the REST API, there's no need to call `room.ensureAttached()` before performing the operation.
Applied to files:
chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt
🧬 Code graph analysis (1)
chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/Exceptions.kt (1)
clientError(110-110)
⏰ 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). (5)
- GitHub Check: build
- GitHub Check: test (Android Debug tests, testDebugUnitTest)
- GitHub Check: test (Android Release tests, testReleaseUnitTest)
- GitHub Check: coverage
- GitHub Check: check
🔇 Additional comments (5)
chat/src/commonMain/kotlin/com/ably/chat/Typing.kt (2)
36-39: API change LGTMExposing current as a read-only property is consistent with the PR objective. Call sites become clearer.
165-166: Good: channel is now internalMaking Channel internal matches the removal of Typing.getChannel from public APIs and reduces surface area.
chat/api/jvm/chat.api (1)
346-346: API surface change acknowledgedOccupancy.getCurrent and Typing.getCurrent additions look consistent with the move to properties.
- Confirm no lingering usages of Typing.getChannel remain in the codebase or samples.
- Verify nullability annotations emitted for Occupancy.getCurrent match Kotlin’s property type (current: OccupancyData?) to avoid surprises for Java callers.
Also applies to: 578-578
chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
29-35: Property-based current added — check Java nullability expectationscurrent is nullable, which is sensible before the first event. Ensure the emitted nullability in platform APIs reflects this for Java callers.
- If Java should never see null, consider making current non-null and documenting that it throws until first event (or return a default). Otherwise, keep nullable but ensure @nullable is visible in API dumps/docs.
chat/api/android/chat.api (1)
346-346: Android API surface alignedOccupancy.getCurrent and Typing.getCurrent align with JVM and common changes.
- Double-check that nullability metadata for Occupancy.getCurrent matches Kotlin’s nullable property.
- Validate there are no remaining references to Typing.getChannel in Android-facing code or docs.
Also applies to: 577-579
| /** | ||
| * Spec: CHA-T9 | ||
| */ | ||
| override val current: Set<String> | ||
| get() { | ||
| logger.trace("DefaultTyping.current()") | ||
| return currentlyTypingMembers.toSet() | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Return a stable snapshot to avoid concurrent iteration of a mutable set
current builds a Set by iterating a mutable backing set that’s updated on a different coroutine context. Reads can happen from any thread, risking CME or visibility issues. Prefer a precomputed, thread-safe snapshot.
Consider maintaining a snapshot and returning it:
// Add near currentlyTypingMembers
private val typingSnapshot = kotlinx.coroutines.flow.MutableStateFlow<Set<String>>(emptySet())
// Update snapshot inside processReceivedTypingEvents, right before emit(...)
typingSnapshot.value = currentlyTypingMembers.toSet()
// Getter
override val current: Set<String>
get() {
logger.trace("DefaultTyping.current")
return typingSnapshot.value
}This avoids iterating the mutable set on arbitrary threads and computes the copy only on writes. If you’d rather not introduce StateFlow here, at least document the threading expectations for readers. [Based on learnings]
🏁 Script executed:
#!/bin/bash
rg -n "currentlyTypingMembers" -C3 --glob '*.kt'Length of output: 3626
🏁 Script executed:
#!/bin/bash
# Get the class definition and surrounding context to understand scope/dispatcher
rg -n "class DefaultTyping" -A20 --glob '*.kt' chat/src/commonMain/kotlin/com/ably/chat/Typing.ktLength of output: 1254
Avoid concurrent iteration over a mutable set
currentlyTypingMembers is a plain MutableSet updated on typingScope but read via current from any coroutine/thread, risking CME or visibility issues. Guard access with a thread-safe snapshot (e.g. a MutableStateFlow<Set<String>> updated on writes) or synchronize reads/writes to eliminate concurrency hazards.
🤖 Prompt for AI Agents
In chat/src/commonMain/kotlin/com/ably/chat/Typing.kt around lines 206 to 214,
currentlyReading from the plain MutableSet currentlyTypingMembers without
synchronization risks concurrent-modification or visibility issues; change the
backing storage to a thread-safe snapshot (e.g. replace MutableSet with a
MutableStateFlow<Set<String>> and update it on all writers, returning
state.value for current) or protect both reads and writes with a shared
lock/Mutex (ensure every mutation and the current getter acquire the lock).
Update all places that mutate currentlyTypingMembers to emit a new Set to the
StateFlow (or perform mutations inside the same lock) so reads return a stable
snapshot and eliminate CME/visibility hazards.
…ccupancy` and `Typing` APIs
ttypic
force-pushed
the
ECO-5434/current-to-prop
branch
from
d0be280 to
24268b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
chat/src/commonMain/kotlin/com/ably/chat/Typing.kt (1)
206-214: Fix concurrent iteration/visibility oncurrentlyTypingMembersincurrentgetterThe getter iterates a mutable set that’s mutated on
typingScope. Readers can run on any thread, risking ConcurrentModificationException and stale reads.Use a precomputed, thread-safe snapshot updated on writes:
@@ -import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow @@ - private var currentlyTypingMembers: MutableSet<String> = mutableSetOf() + private var currentlyTypingMembers: MutableSet<String> = mutableSetOf() + private val typingSnapshot = MutableStateFlow<Set<String>>(emptySet()) @@ - override val current: Set<String> - get() { - logger.trace("DefaultTyping.current") - return currentlyTypingMembers.toSet() - } + override val current: Set<String> + get() { + logger.trace("DefaultTyping.current") + return typingSnapshot.value + }Update writers to refresh the snapshot:
@@ TypingEventType.Started -> { // CHA-T13b1 currentlyTypingMembers.add(clientId) + typingSnapshot.value = currentlyTypingMembers.toSet() // CHA-T13b2 - Cancel the current self stopping event and replace with new one typingStartEventPrunerJobs[clientId]?.cancel() @@ // typingEventWaitingTimeout elapsed, so remove given clientId and emit stop event currentlyTypingMembers.remove(clientId) + typingSnapshot.value = currentlyTypingMembers.toSet() typingStartEventPrunerJobs.remove(clientId) emit(TypingEventType.Stopped, clientId) } @@ TypingEventType.Stopped -> { // CHA-T13b4 val clientIdPresent = currentlyTypingMembers.remove(clientId) typingStartEventPrunerJobs[clientId]?.cancel() typingStartEventPrunerJobs.remove(clientId) if (!clientIdPresent) { // CHA-T13b5 return } + typingSnapshot.value = currentlyTypingMembers.toSet() }
🧹 Nitpick comments (1)
chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
166-175: Avoid cross-thread visibility issues forcurrent(use a StateFlow-backed snapshot)
latestOccupancyDatais mutated onDispatchers.Defaultand read from arbitrary threads; without a memory barrier, readers may see stale values. Prefer a thread-safe snapshot.Apply this minimal refactor:
@@ -import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.MutableStateFlow @@ - private var latestOccupancyData: OccupancyData? = null + private val occupancySnapshot = MutableStateFlow<OccupancyData?>(null) @@ - override val current: OccupancyData? + override val current: OccupancyData? get() { logger.trace("Occupancy.current") if (!room.options.occupancy.enableEvents) { // CHA-O7c throw clientError("cannot get current occupancy; occupancy events are not enabled in room options") } // CHA-O7a // CHA-O7b - return latestOccupancyData + return occupancySnapshot.value } @@ - latestOccupancyData = occupancyData + occupancySnapshot.value = occupancyData
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
chat/api/android/chat.api(2 hunks)chat/api/jvm/chat.api(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt(2 hunks)chat/src/commonMain/kotlin/com/ably/chat/Typing.kt(5 hunks)chat/src/commonTest/kotlin/com/ably/chat/TypingTest.kt(9 hunks)chat/src/commonTest/kotlin/com/ably/chat/integration/TypingIntegrationTest.kt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- chat/api/jvm/chat.api
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-28T11:11:20.423Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Occupancy.kt:145-145
Timestamp: 2024-11-28T11:11:20.423Z
Learning: In `chat-android/src/main/java/com/ably/chat/Occupancy.kt`, within the `DefaultOccupancy` class, when methods use `room.chatApi`, which utilizes the REST API, there's no need to call `room.ensureAttached()` before performing the operation.
Applied to files:
chat/src/commonMain/kotlin/com/ably/chat/Occupancy.ktchat/api/android/chat.api
📚 Learning: 2024-11-28T11:10:20.947Z
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Typing.kt:178-183
Timestamp: 2024-11-28T11:10:20.947Z
Learning: In `chat-android/src/main/java/com/ably/chat/Typing.kt`, within the `DefaultTyping` class, the `stop()` method must execute within `typingScope` (a `CoroutineScope` with parallelism set to 1) to avoid race conditions when setting and cancelling `typingJob`.
Applied to files:
chat/src/commonTest/kotlin/com/ably/chat/integration/TypingIntegrationTest.ktchat/api/android/chat.apichat/src/commonMain/kotlin/com/ably/chat/Typing.kt
🧬 Code graph analysis (3)
chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
chat/src/commonMain/kotlin/com/ably/chat/Exceptions.kt (1)
clientError(110-110)
chat/src/commonTest/kotlin/com/ably/chat/integration/TypingIntegrationTest.kt (1)
chat/src/commonTest/kotlin/com/ably/chat/TestUtils.kt (1)
assertWaiter(160-169)
chat/src/commonTest/kotlin/com/ably/chat/TypingTest.kt (1)
chat/src/commonTest/kotlin/com/ably/chat/TestUtils.kt (1)
assertWaiter(160-169)
⏰ 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). (6)
- GitHub Check: test (JVM tests, jvmTest)
- GitHub Check: test (Android Release tests, testReleaseUnitTest)
- GitHub Check: test (Android Debug tests, testDebugUnitTest)
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: check
🔇 Additional comments (8)
chat/src/commonMain/kotlin/com/ably/chat/Occupancy.kt (1)
29-35: API change to a read-onlycurrentproperty looks goodDocs reflect throws behavior; naming and nullability align with prior semantics.
chat/src/commonMain/kotlin/com/ably/chat/Typing.kt (3)
36-39: API switch tocurrentproperty is consistent and clearProperty form reads naturally at call sites and matches the PR goal.
165-165: Makingchannelinternal is appropriateReduces surface area and aligns with encapsulation goals. No API regressions observed in Android stubs.
350-351: Event emission usingcurrentproperty is fineConstructing the event from the
currentsnapshot keeps listeners consistent with the exposed API.chat/api/android/chat.api (2)
346-347: Verify nullability forOccupancy.getCurrent()on Android/JVM surfaceKotlin declares
Occupancy.current: OccupancyData?. Ensure the generated Android API carries nullability (e.g., @nullable) so Java callers aren’t misled.If missing, consider adding explicit nullability annotations or a typealias annotated for the Java surface.
578-582: Android stub forTyping.getCurrent()matches the Kotlin property changeSurface aligns with the refactor and removes prior method form.
chat/src/commonTest/kotlin/com/ably/chat/integration/TypingIntegrationTest.kt (1)
38-40: Tests correctly updated to property access
typing.currentusage aligns with the new API; assertions remain valid.Also applies to: 53-55, 97-99, 114-116, 131-133
chat/src/commonTest/kotlin/com/ably/chat/TypingTest.kt (1)
231-231: Unit tests updated totyping.currentproperty — looks goodAll call sites now use the property; expectations unchanged.
Also applies to: 243-243, 247-248, 251-252, 321-321, 340-340, 361-361, 378-378, 387-388, 410-410, 421-421
Code Coverage
|
1 participant
Replaced
current()function withcurrentproperty inOccupancyandTypingAPIsSummary by CodeRabbit