[ECO-5479][LiveObjects] Implement object lifecycle events #1151
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (10)
WalkthroughIntroduces object lifecycle event support: adds a public lifecycle API (ObjectLifecycleChange, ObjectLifecycleEvent), wires internal coordination to emit DELETED on tombstone, extends LiveMap/LiveCounter to expose lifecycle subscriptions, updates tests to assert deletion events, and performs minor docs wording, logging, null-safety, and annotation tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant LiveObject as LiveMap/LiveCounter (public)
participant Coordinator as ObjectLifecycleCoordinator (internal)
participant BaseObj as BaseRealtimeObject
participant Emitter as ObjectLifecycleEmitter
participant Listener as Client Listener
rect rgba(230,245,255,0.6)
note right of Client: Subscribe to lifecycle
Client->>LiveObject: on(DELETED, listener)
LiveObject->>Coordinator: register(listener for DELETED)
Coordinator-->>Client: ObjectsSubscription
end
rect rgba(240,255,240,0.6)
note right of BaseObj: Tombstone triggered
BaseObj->>BaseObj: tombstone(serialTimestamp)
BaseObj->>Coordinator: objectLifecycleChanged(Deleted)
Coordinator->>Emitter: emit(ObjectLifecycleEvent.DELETED)
Emitter-->>Listener: onLifecycleEvent(DELETED)
end
alt Unsubscribe
Client->>LiveObject: off(listener) / offAll()
LiveObject->>Coordinator: unregister(...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Poem
Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Add the ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/1151/features
github-actions
bot
temporarily deployed
to
staging/pull/1151/features
sacOO7
force-pushed
the
feature/liveobject-lifecycle-events
branch
from
530b9e3 to
9af38df
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1151/features
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.kt (3)
61-65: Track last known state to prevent ensureSynced race and enable re-checksWe need a locally cached state to safely handle races in ensureSynced. Add a volatile field to store the last observed state.
internal abstract class ObjectsStateCoordinator : ObjectsStateChange, HandlesObjectsStateChange { private val tag = "ObjectsStateCoordinator" + @Volatile + private var lastState: ObjectsState = ObjectsState.Initialized private val internalObjectStateEmitter = ObjectsStateEmitter() // related to RTC10, should have a separate EventEmitter for users of the library private val externalObjectStateEmitter = ObjectsStateEmitter()
77-83: Update lastState before emitting eventsSet the cached state for both internal correctness and to enable race-free ensureSynced.
override fun objectsStateChanged(newState: ObjectsState) { - objectsStateToEventMap[newState]?.let { objectsStateEvent -> + lastState = newState + objectsStateToEventMap[newState]?.let { objectsStateEvent -> internalObjectStateEmitter.emit(objectsStateEvent) externalObjectStateEmitter.emit(objectsStateEvent) } }
95-96: Dispose both internal and external state listeners to avoid leaksdisposeObjectsStateListeners currently only clears external listeners. Any pending internal once-listeners (e.g., from ensureSynced) could leak if disposal happens before SYNCED.
- override fun disposeObjectsStateListeners() = offAll() + override fun disposeObjectsStateListeners() { + // Clear both internal and external listeners + internalObjectStateEmitter.off() + externalObjectStateEmitter.off() + }
🧹 Nitpick comments (10)
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (2)
7-7: Avoid applying @NotNull to a Unit-returning function (no effect, can confuse tooling)Kotlin already emits nullability metadata for Java interop. Annotating the function itself (which returns Unit/void) is ineffective and can trip static analyzers. If you intended to document null-safety, prefer relying on Kotlin types (already non-null here) or, if you must, annotate parameters. For this internal API, the cleanest is to drop the method-level @NotNull.
Apply this diff to remove the unused import and the redundant annotation:
-import org.jetbrains.annotations.NotNull @@ - @NotNull internal fun <T> launchWithCallback(callback: ObjectsCallback<T>, block: suspend () -> T) {Also applies to: 69-70
96-104: Harden error path: catch exceptions from callback.onError tooYou already guard exceptions thrown by onSuccess, but not by onError. A user callback throwing in onError would currently bubble out of the coroutine. Mirror the success-path guard for consistency.
} catch (throwable: Throwable) { - when (throwable) { - is AblyException -> { callback.onError(throwable) } - else -> { - val ex = ablyException("Error executing operation", ErrorCode.BadRequest, cause = throwable) - callback.onError(ex) - } - } + try { + when (throwable) { + is AblyException -> { callback.onError(throwable) } + else -> { + val ex = ablyException("Error executing operation", ErrorCode.BadRequest, cause = throwable) + callback.onError(ex) + } + } + } catch (t: Throwable) { + Log.e(tag, "Error occurred while executing callback's onError handler", t) + } // catch and don't rethrow error from callback }lib/src/main/java/io/ably/lib/objects/ObjectsPlugin.java (1)
41-41: Minor Javadoc grammar: “flag indicating …”Prefer gerund for parameter descriptions.
Apply this diff:
- * @param hasObjects flag indicates whether the channel has any associated objects. + * @param hasObjects flag indicating whether the channel has any associated objects.live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (1)
42-42: Nit: Capitalize “Ably” in spec note.Proper noun should be capitalized.
Apply this diff:
- * @spec RTO3a - Pool storing all ably objects by object ID + * @spec RTO3a - Pool storing all Ably objects by object IDlive-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultRealtimeObjectsTest.kt (1)
174-176: Good lifecycle coverage; add a waiter for lifecycleEvents to avoid flakes.Subscriptions and assertions look correct. Since emissions are asynchronous, consider waiting for the expected lifecycle event count before asserting to reduce flakiness.
Apply this diff at the assertion site:
- // Assert lifecycle events - assertEquals(3, lifecycleEvents.size) // Should have received 3 DELETED lifecycle events + // Assert lifecycle events + assertWaiter { lifecycleEvents.size == 3 } // Wait for all DELETED lifecycle events to arrive + assertEquals(3, lifecycleEvents.size) // Should have received 3 DELETED lifecycle events lifecycleEvents.forEach { event -> assertEquals(ObjectLifecycleEvent.DELETED, event) // All events should be DELETED }Also applies to: 186-188, 207-209, 232-234, 250-254
live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt (3)
116-120: Correct the error message to show the actual object’s idCurrently prints the incoming id twice; should include this.objectId for clarity.
internal fun validateObjectId(objectId: String?) { if (this.objectId != objectId) { - throw objectError("Invalid object: incoming objectId=${objectId}; $objectType objectId=$objectId") + throw objectError("Invalid object: incoming objectId=$objectId; $objectType objectId=${this.objectId}") } }
93-97: Nit: Kotlin style – remove trailing semicolon on returnSemicolons are optional; prefer idiomatic return without semicolon.
if (isTombstoned) { // this object is tombstoned so the operation cannot be applied - return; + return }
140-143: Minor readability: simplify boolean expressionAvoid == true on a nullable boolean; prefer explicit defaulting.
internal fun isEligibleForGc(): Boolean { - val currentTime = System.currentTimeMillis() - return isTombstoned && tombstonedAt?.let { currentTime - it >= ObjectsPoolDefaults.GC_GRACE_PERIOD_MS } == true + val at = tombstonedAt ?: return false + return isTombstoned && (System.currentTimeMillis() - at) >= ObjectsPoolDefaults.GC_GRACE_PERIOD_MS }lib/src/main/java/io/ably/lib/objects/type/ObjectLifecycleChange.java (2)
60-67: Make Listener lambda-friendly and add null-safety annotationMark Listener as a functional interface and annotate the event param as @NotNull for consistency with the rest of the API.
- interface Listener { + @FunctionalInterface + interface Listener { /** * Called when a lifecycle event occurs. * * @param lifecycleEvent The lifecycle event that occurred */ - void onLifecycleEvent(ObjectLifecycleEvent lifecycleEvent); + void onLifecycleEvent(@NotNull ObjectLifecycleEvent lifecycleEvent); }
7-18: Doc nit: clarify current scope of lifecycle eventsRight now only DELETED is emitted. Optional: either explicitly state that or add a note that more events will follow as the API evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
lib/src/main/java/io/ably/lib/objects/ObjectsPlugin.java(1 hunks)lib/src/main/java/io/ably/lib/objects/RealtimeObjects.java(1 hunks)lib/src/main/java/io/ably/lib/objects/state/ObjectsStateChange.java(2 hunks)lib/src/main/java/io/ably/lib/objects/state/ObjectsStateEvent.java(1 hunks)lib/src/main/java/io/ably/lib/objects/type/ObjectLifecycleChange.java(1 hunks)lib/src/main/java/io/ably/lib/objects/type/ObjectLifecycleEvent.java(1 hunks)lib/src/main/java/io/ably/lib/objects/type/ObjectUpdate.java(1 hunks)lib/src/main/java/io/ably/lib/objects/type/counter/LiveCounter.java(2 hunks)lib/src/main/java/io/ably/lib/objects/type/map/LiveMap.java(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt(3 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.kt(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt(5 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/ObjectLifecycle.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterChangeCoordinator.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapChangeCoordinator.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultRealtimeObjectsTest.kt(6 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/MapFixtures.kt(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: sacOO7
PR: ably/ably-java#1148
File: lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java:0-0
Timestamp: 2025-08-14T10:43:48.159Z
Learning: In the ably-java codebase, some "LiveObjectsPlugin" references in log messages, Javadoc, build files, and workflows are intentionally retained due to ongoing internal technical debate about isolating API naming from product naming, as discussed in LODR-033. These are not considered incomplete refactoring work but deliberate exceptions to the broader effort to eliminate "liveobject" keyword usage.
📚 Learning: 2025-08-14T10:43:57.974Z
Learnt from: sacOO7
PR: ably/ably-java#1148
File: lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java:24-25
Timestamp: 2025-08-14T10:43:57.974Z
Learning: In the ably-java LiveObjects renaming effort, log messages may intentionally retain "LiveObjects" terminology even when other parts of the codebase are being renamed to "Objects", due to internal technical debates about where the renaming should be applied. The team makes deliberate decisions about which parts of the codebase to rename versus which to keep as-is.
Applied to files:
lib/src/main/java/io/ably/lib/objects/type/ObjectUpdate.javalive-objects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.ktlib/src/main/java/io/ably/lib/objects/RealtimeObjects.javalib/src/main/java/io/ably/lib/objects/ObjectsPlugin.javalib/src/main/java/io/ably/lib/objects/state/ObjectsStateChange.javalive-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.ktlib/src/main/java/io/ably/lib/objects/type/counter/LiveCounter.javalib/src/main/java/io/ably/lib/objects/type/map/LiveMap.javalive-objects/src/main/kotlin/io/ably/lib/objects/type/ObjectLifecycle.ktlive-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.kt
📚 Learning: 2025-08-07T07:19:59.979Z
Learnt from: sacOO7
PR: ably/ably-java#1139
File: live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt:86-92
Timestamp: 2025-08-07T07:19:59.979Z
Learning: In DefaultLiveCounter.notifyUpdated method, sacOO7 prefers to keep the unchecked cast `update as LiveCounterUpdate` without type safety checks, as they are confident the type system guarantees the correct type will always be passed.
Applied to files:
lib/src/main/java/io/ably/lib/objects/type/ObjectUpdate.javalib/src/main/java/io/ably/lib/objects/type/counter/LiveCounter.java
📚 Learning: 2025-08-01T09:53:16.730Z
Learnt from: sacOO7
PR: ably/ably-java#1120
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt:0-0
Timestamp: 2025-08-01T09:53:16.730Z
Learning: In the ably-java LiveObjects test code, an extension property `State` (capital S) is defined on the `LiveObjects` interface in live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/Utils.kt to provide access to the internal `state` field by casting to `DefaultLiveObjects`. This allows tests to access internal state for verification purposes.
Applied to files:
lib/src/main/java/io/ably/lib/objects/state/ObjectsStateEvent.javalive-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.kt
📚 Learning: 2025-08-14T10:43:48.159Z
Learnt from: sacOO7
PR: ably/ably-java#1148
File: lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java:0-0
Timestamp: 2025-08-14T10:43:48.159Z
Learning: In the ably-java codebase, some "LiveObjectsPlugin" references in log messages, Javadoc, build files, and workflows are intentionally retained due to ongoing internal technical debate about isolating API naming from product naming, as discussed in LODR-033. These are not considered incomplete refactoring work but deliberate exceptions to the broader effort to eliminate "liveobject" keyword usage.
Applied to files:
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.ktlib/src/main/java/io/ably/lib/objects/RealtimeObjects.javalib/src/main/java/io/ably/lib/objects/ObjectsPlugin.javalive-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.ktlib/src/main/java/io/ably/lib/objects/type/counter/LiveCounter.javalib/src/main/java/io/ably/lib/objects/type/map/LiveMap.javalive-objects/src/main/kotlin/io/ably/lib/objects/type/ObjectLifecycle.kt
📚 Learning: 2025-08-15T10:25:24.011Z
Learnt from: sacOO7
PR: ably/ably-java#1148
File: lib/src/main/java/io/ably/lib/objects/ObjectsHelper.java:24-25
Timestamp: 2025-08-15T10:25:24.011Z
Learning: In the ably-java codebase, the ObjectsHelper.java log messages that reference "LiveObjects plugin not found" and "LiveObjects functionality" are also part of the deliberate exceptions to the renaming effort, similar to other "LiveObjectsPlugin" references, due to ongoing internal technical debate about isolating API naming from product naming as discussed in LODR-033.
Applied to files:
lib/src/main/java/io/ably/lib/objects/RealtimeObjects.javalib/src/main/java/io/ably/lib/objects/ObjectsPlugin.java
📚 Learning: 2025-08-06T09:22:40.964Z
Learnt from: sacOO7
PR: ably/ably-java#1135
File: live-objects/src/main/kotlin/io/ably/lib/objects/DefaultLiveObjects.kt:100-134
Timestamp: 2025-08-06T09:22:40.964Z
Learning: In DefaultLiveObjects.kt, the object creation pattern using objectsPool.get() followed by withContext(sequentialScope.coroutineContext) { objectsPool.createZeroValueObjectIfNotExists() } is thread-safe because sequentialScope uses limitedParallelism(1) ensuring sequential execution, and createZeroValueObjectIfNotExists() performs an internal get() check before creating, preventing duplicate object creation even when multiple coroutines initially see null from the first get() call.
Applied to files:
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt
📚 Learning: 2025-08-07T07:17:33.340Z
Learnt from: sacOO7
PR: ably/ably-java#1137
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt:6-6
Timestamp: 2025-08-07T07:17:33.340Z
Learning: In the ably-java LiveObjects test code, there are extension properties defined in TestHelpers.kt that provide access to private fields of classes for testing purposes. For example, `internal var DefaultLiveMap.LiveMapManager: LiveMapManager` allows tests to access the private `liveMapManager` field. These extension imports (like `import io.ably.lib.objects.unit.LiveMapManager`) should not be removed as they are necessary for test functionality and are not conflicting imports.
Applied to files:
live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/MapFixtures.ktlib/src/main/java/io/ably/lib/objects/type/map/LiveMap.javalive-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultRealtimeObjectsTest.kt
📚 Learning: 2025-08-01T09:53:16.730Z
Learnt from: sacOO7
PR: ably/ably-java#1120
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveObjectsTest.kt:0-0
Timestamp: 2025-08-01T09:53:16.730Z
Learning: In the ably-java LiveObjects test code, extension properties with capital letter names (like `State`, `ObjectId`) are defined in live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/Utils.kt to provide access to internal fields of concrete implementations through their public interfaces. For example, `LiveObjects.State` casts to `DefaultLiveObjects` to access the internal `state` field for testing purposes.
Applied to files:
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.ktlive-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultRealtimeObjectsTest.kt
📚 Learning: 2025-08-01T10:30:27.049Z
Learnt from: sacOO7
PR: ably/ably-java#1130
File: live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultLiveMapTest.kt:241-241
Timestamp: 2025-08-01T10:30:27.049Z
Learning: In DefaultLiveMapTest.kt integration tests, operations are performed sequentially one after another, and subscription callback list updates happen one at a time, so thread-safe collections are not needed for collecting updates in test scenarios.
Applied to files:
live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultRealtimeObjectsTest.kt
📚 Learning: 2025-08-01T05:50:33.039Z
Learnt from: sacOO7
PR: ably/ably-java#1113
File: live-objects/src/test/kotlin/io/ably/lib/objects/unit/type/livemap/LiveMapManagerTest.kt:16-16
Timestamp: 2025-08-01T05:50:33.039Z
Learning: In LiveMapManagerTest.kt, the private field `livemapManager` is used extensively in the `shouldCalculateMapDifferenceCorrectly` test method to test the `calculateUpdateFromDataDiff` functionality across multiple test scenarios, so it should not be removed as unused.
Applied to files:
live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultRealtimeObjectsTest.kt
🧬 Code Graph Analysis (3)
lib/src/main/java/io/ably/lib/objects/type/ObjectLifecycleChange.java (2)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.kt (3)
on(66-71)off(73-73)offAll(75-75)live-objects/src/main/kotlin/io/ably/lib/objects/type/ObjectLifecycle.kt (3)
on(54-59)off(61-61)offAll(63-63)
live-objects/src/main/kotlin/io/ably/lib/objects/type/ObjectLifecycle.kt (1)
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.kt (1)
offAll(75-75)
live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt (1)
live-objects/src/main/kotlin/io/ably/lib/objects/type/ObjectLifecycle.kt (3)
objectLifecycleChanged(33-47)objectLifecycleChanged(40-40)objectLifecycleChanged(65-69)
⏰ 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: check-realtime-okhttp
- GitHub Check: check-realtime
🔇 Additional comments (24)
lib/src/main/java/io/ably/lib/objects/type/ObjectUpdate.java (1)
8-8: Javadoc wording alignment LGTMUpdated wording from “live object” to “object” reads better and aligns with the broader terminology shifts. No API or behavior changes.
live-objects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapChangeCoordinator.kt (1)
46-46: Clarified warning message LGTMThe more specific log (“LiveMapChange listener callback”) improves diagnostics without altering behavior.
live-objects/src/main/kotlin/io/ably/lib/objects/type/livecounter/LiveCounterChangeCoordinator.kt (1)
46-46: Clarified warning message LGTMConsistent with the LiveMap variant; improves clarity while keeping behavior unchanged.
lib/src/main/java/io/ably/lib/objects/RealtimeObjects.java (1)
16-16: Javadoc tweak LGTMRemoving “live” here aligns with the general terminology update while keeping API and contracts intact.
live-objects/src/main/kotlin/io/ably/lib/objects/DefaultRealtimeObjects.kt (2)
23-23: Doc wording update aligns with terminology shift."managing objects on a channel" reads well and matches the broader rename. No behavioral changes.
28-28: Spec doc tweak looks good."Objects pool storing all objects by object ID" is consistent with the rest of the docs.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsPool.kt (2)
31-34: Docs align with Objects terminology; no functional change.Wording updates to class and spec comment look good.
60-61: Javadoc phrasing LGTM.“Gets an object from the pool by object ID.” reads clearly.
live-objects/src/test/kotlin/io/ably/lib/objects/integration/helpers/fixtures/MapFixtures.kt (2)
11-11: Doc wording change LGTM.Updated wording is accurate and consistent with current terminology.
74-74: Doc wording change LGTM.“hierarchical structure of objects” is clear and consistent.
lib/src/main/java/io/ably/lib/objects/state/ObjectsStateEvent.java (1)
4-4: Doc rename is consistent and clear.The wording update from “Live Objects” to “Objects” looks good and aligns with the broader rename. No behavioral change.
lib/src/main/java/io/ably/lib/objects/state/ObjectsStateChange.java (2)
9-18: Javadoc rename LGTM.Clearer phrasing around “Objects synchronization state event.” No API or behavior change.
43-47: Listener Javadoc rename LGTM.Consistent with the rename; no functional impact.
lib/src/main/java/io/ably/lib/objects/type/map/LiveMap.java (1)
18-18: Public API change: LiveMap now extends ObjectLifecycleChange without default methods
- ObjectLifecycleChange’s
on,off, andoffAllare abstract, so any existing LiveMap implementer must now provide these methods.- A search found no internal classes implementing LiveMap, but external or third-party implementers could still exist—please verify.
- If LiveMap is intended to remain publicly implementable, add default no-op implementations for these methods in ObjectLifecycleChange. Otherwise, annotate LiveMap as non-extendable (e.g.,
@DoNotImplement) and bump the version per your compatibility policy.- Add a release note highlighting the new lifecycle subscription API and its potential impact on implementers.
live-objects/src/test/kotlin/io/ably/lib/objects/integration/DefaultRealtimeObjectsTest.kt (1)
9-9: Lifecycle import is appropriate.Importing ObjectLifecycleEvent matches the new lifecycle subscription API usage below.
live-objects/src/main/kotlin/io/ably/lib/objects/ObjectsState.kt (3)
23-27: LGTM: explicit state-to-event mapping avoids accidental INITIALIZED emissionsMapping INITIALIZED to null is correct and prevents spurious public events. Clean and readable.
100-107: LGTM: null-safe listener invocation with clear loggingSafe-call invocation with a helpful warning on null event is good defensive practice. Consistent with ObjectLifecycle emitter.
84-93: Ignore inapplicable suggestion:lastStatenot definedThe proposed fix references a
lastStatefield that doesn’t exist inObjectsStateCoordinator, so the diff cannot be applied as-is. Please disregard this review comment.Likely an incorrect or invalid review comment.
live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt (2)
30-31: Good integration: BaseRealtimeObject now coordinates lifecycle emissionsExtending ObjectLifecycleCoordinator plugs the base type into the lifecycle event system as intended by this PR.
125-135: Guard DELETED event emission and add coverage testsThe spec requires emitting a single
Deletedlifecycle event when an object is tombstoned, even iftombstone()is called multiple times. Please apply the following changes and add a test case to verify exactly oneDeletedevent per object:• In
live-objects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt:internal fun tombstone(serialTimestamp: Long?): ObjectUpdate { if (serialTimestamp == null) { Log.w(tag, "Tombstoning object $objectId without serial timestamp, using local timestamp instead") } - isTombstoned = true - tombstonedAt = serialTimestamp?: System.currentTimeMillis() - // Emit object lifecycle event for deletion - objectLifecycleChanged(ObjectLifecycle.Deleted) + val firstTimeTombstone = !isTombstoned + isTombstoned = true + tombstonedAt = serialTimestamp ?: System.currentTimeMillis() + // Emit object lifecycle event for deletion (only once) + if (firstTimeTombstone) { + objectLifecycleChanged(ObjectLifecycle.Deleted) + } val update = clearData() return update }• Add a unit test (e.g. in
BaseRealtimeObjectTest.kt) that:
- Calls
tombstone(null)twice on the same object.- Asserts that
objectLifecycleChanged(ObjectLifecycle.Deleted)is emitted exactly once.No existing tests cover this scenario—please add one to ensure correct, idempotent behavior.
lib/src/main/java/io/ably/lib/objects/type/ObjectLifecycleChange.java (1)
31-33: Ensure JetBrains Annotations Dependency Includes @nonblockingThe codebase already imports and uses
@NonBlockingin multiple classes (e.g. RealtimeObjects.java, ObjectsStateChange.java, ObjectLifecycleChange.java, LiveMap.java, LiveCounter.java). If theorg.jetbrains:annotationsartifact isn’t declared in your build, these annotations will fail to resolve.Use this snippet to verify your Gradle/Maven configuration:
#!/bin/bash find . -maxdepth 1 -type f \( -name 'build.gradle*' -o -name 'pom.xml' \) -print \ | xargs -I {} sh -c ' echo "==== {} ====" rg -n "org\.jetbrains:annotations" {} \ || echo "⚠️ Missing JetBrains annotations dependency in {}" 'If it reports “Missing JetBrains annotations,” add the dependency:
• Gradle (build.gradle / build.gradle.kts)
implementation("org.jetbrains:annotations:")• Maven (pom.xml)
org.jetbrains
annotations
‹latest-version›
live-objects/src/main/kotlin/io/ably/lib/objects/type/ObjectLifecycle.kt (3)
21-25: LGTM: mapping internal lifecycle to public events isolates API surfaceOnly Deleted maps to a public event; this matches the PR objective (emit DELETED on tombstone) without over-exposing internal transitions.
65-69: LGTM: lifecycle emission logic is minimal and correctCoordinator maps and emits the event through the emitter; simple and robust.
74-84: LGTM: emitter apply() is null-safe with good diagnosticsConsistent with ObjectsState emitter. Exception handling and logging are appropriate.
sacOO7
force-pushed
the
feature/liveobject-lifecycle-events
branch
from
9af38df to
e6678b0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1151/features
- Updated LiveMap and LiveCounter to extend ObjectLifecycleChange interface - Implemented ObjectLifecycleCoordinator by extending ObjectLifecycleChange - Updated testObjectDelete with relevant assertions for ObjectLifecycleEvent.DELETED
sacOO7
force-pushed
the
feature/liveobject-lifecycle-events
branch
from
e6678b0 to
915f305
Compare
Summary by CodeRabbit