[ECO-5525] Improve LiveMapValue API
#66
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR replaces the primitive wrapper model with direct enum cases for LiveMapValue across internal and public APIs. Internal conversion and serialization paths are updated accordingly. Public types gain literal conformances. Tests are adjusted to use direct cases and literals instead of the .primitive wrapper. Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant PublicAPI as LiveMapValue (Public)
participant Internal as InternalLiveMapValue
participant Protocol as ObjectData (Realtime)
Developer->>PublicAPI: Provide value (literal or enum case)
PublicAPI->>Internal: Map to InternalLiveMapValue (direct cases)
Internal->>Protocol: Serialize via toObjectData (per-type mapping)
Protocol-->>Internal: Encoded payload
Internal-->>PublicAPI: Values decoded to internal cases
PublicAPI-->>Developer: Expose as public LiveMapValue cases
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
lawrence-forooghian
changed the base branch from
main
to
ECO-5465-port-integration-tests
github-actions
bot
temporarily deployed
to
staging/pull/66/AblyLiveObjects
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
umair-ably
left a comment
umair-ably
left a comment
There was a problem hiding this comment.
simple enough change, don't feel like you have to wait for coderabbit but up to you
![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 (5)
Sources/AblyLiveObjects/Public/Public Proxy Objects/InternalLiveMapValue+ToPublic.swift (1)
20-22: Clarify docstring about caching scopeThe comment suggests the method “fetches the cached public object… creating a new public object if there isn't already one.” That caching behavior applies only to
liveMap/liveCountercases; primitives are value types returned directly with no caching. Consider tightening the wording to avoid implying caching for primitive cases.Sources/AblyLiveObjects/Public/PublicTypes.swift (2)
84-106: Docblock is helpful; consider noting numeric precision and interpolationNice examples showcasing literal support. Two minor clarifications could prevent surprises:
- Integer literals become
.number(Double(value)), so very large integers may lose precision.- Interpolated strings like
"value \(x)"won’t compile asLiveMapValuewithoutExpressibleByStringInterpolationconformance (see suggestion below).If you want, I can add a short note to this doc section.
209-245: Add ExpressibleByStringInterpolation to support interpolated string literalsInterpolated strings currently won’t be accepted as
LiveMapValueunless you addExpressibleByStringInterpolation. Suggest adding:extension LiveMapValue: ExpressibleByStringLiteral { public init(stringLiteral value: String) { self = .string(value) } } + +extension LiveMapValue: ExpressibleByStringInterpolation { + public init(stringInterpolation: DefaultStringInterpolation) { + self = .string(String(stringInterpolation: stringInterpolation)) + } +}Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (2)
145-150: Nit: remove unnecessarytryaroundDictionary(uniqueKeysWithValues:)
Dictionary(uniqueKeysWithValues:)is non-throwing, so thetryis redundant.- #expect( - try Dictionary(uniqueKeysWithValues: map.entries) == [ + #expect( + Dictionary(uniqueKeysWithValues: map.entries) == [ "boolKey": true, "numberKey": 10, ], )
182-186: Nit: remove unnecessarytryin the final entries assertionSame as earlier, this construction doesn’t throw.
- #expect( - try Dictionary(uniqueKeysWithValues: map.entries) == [ + #expect( + Dictionary(uniqueKeysWithValues: map.entries) == [ "numberKey": 10, "counterKey": .liveCounter(counter), ], )
📜 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 (9)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(1 hunks)Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift(4 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/InternalLiveMapValue+ToPublic.swift(1 hunks)Sources/AblyLiveObjects/Public/PublicTypes.swift(3 hunks)Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift(3 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(3 hunks)Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift(4 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(9 hunks)Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-29T08:07:47.875Z
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to AblyLiveObjects/**/!(*Test|*Tests).swift : When importing the Ably module inside the AblyLiveObjects library code (non-test code), use `import Ably`.
Applied to files:
Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
📚 Learning: 2025-07-29T08:07:47.875Z
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to AblyLiveObjects/**/!(*Test|*Tests).swift : When importing the AblyPlugin module inside the AblyLiveObjects library code (non-test code), use `internal import AblyPlugin`.
Applied to files:
Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift
📚 Learning: 2025-07-29T08:07:58.385Z
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-07-29T08:07:58.385Z
Learning: Applies to **/Tests/**/*.swift : When you need to import the following modules in the tests, do so in the following way: Ably: use `import Ably`; AblyLiveObjects: use `testable import AblyLiveObjects`; AblyPlugin: use `import AblyPlugin`; do not do `internal import`.
Applied to files:
Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swiftTests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift
📚 Learning: 2025-07-29T08:07:47.875Z
Learnt from: CR
PR: ably/ably-cocoa-liveobjects-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-07-29T08:07:47.875Z
Learning: Applies to **/*.swift : When writing JSONValue or WireValue types, favour using the literal syntax enabled by their conformance to the `ExpressibleBy*Literal` protocols where possible.
Applied to files:
Sources/AblyLiveObjects/Public/PublicTypes.swift
🧬 Code Graph Analysis (6)
Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift (1)
Sources/AblyLiveObjects/Public/PublicTypes.swift (1)
lhs(185-206)
Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (2)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
map(78-99)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
entries(124-128)entries(840-856)
Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (2)
Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
entries(124-128)entries(840-856)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (2)
createMap(38-50)createMap(52-63)
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (2)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
map(78-99)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift (1)
set(78-82)
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (4)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
map(78-99)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (2)
createMap(38-50)createMap(52-63)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
entries(124-128)entries(840-856)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveMap.swift (1)
set(78-82)
Sources/AblyLiveObjects/Public/PublicTypes.swift (1)
Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift (1)
lhs(143-164)
🔇 Additional comments (34)
Sources/AblyLiveObjects/Public/Public Proxy Objects/InternalLiveMapValue+ToPublic.swift (1)
23-34: Direct primitive mappings look correctThe switch now cleanly maps each internal primitive to the corresponding public
LiveMapValuecase. This aligns with the removal of the wrapper and keeps bridging straightforward.Sources/AblyLiveObjects/Public/PublicTypes.swift (4)
107-116: Enum case split is a solid improvementExplicit per-type cases greatly simplify usage, pattern matching, and docs. This also makes the public surface consistent with internal representation.
135-181: Convenience getters are clear and symmetricThe accessors cover all primitive cases and the object cases already have their dedicated getters above. This is tidy and discoverable.
185-206: Equatable semantics are sensible
- Value equality for primitives and JSON containers.
- Identity for
liveMap/liveCounter.
This mirrors the internal equality and avoids expensive deep object comparisons.
107-116: No lingering references toPrimitiveObjectValuefoundI searched the repo with ripgrep for
\bPrimitiveObjectValue\band\.primitive\s*\(— no matches were returned.No action required.
Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (2)
140-150: Good adoption of literal conformances in map creation and assertionsUsing
trueand10directly keeps tests concise and validates the new API surface.
164-169: Assertions correctly reflect mixed object/primitive entriesThe expected dictionary combining literals and
.liveCounteris a nice end-to-end check of both literal support and object identity.Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
889-906: Primitive mapping to internal variants looks correct and ordered per spec
.bool(boolean)before other types.data(bytes)for binary.number(number.doubleValue)for numeric.string(string)for stringThe order matches the RTLM5d2b–e guidance and aligns with the new internal value cases.
909-916: JSON mapping matches the new cases; confirm null handlingSwitching
.arrayto.jsonArrayand.objectto.jsonObjectis consistent. One question: ifjsoncould ever benull, we currently drop tonil(no value). If that’s intentional under the current spec gap (TODO link), all good—just noting the behavior so it’s explicit in tests/specs later.Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift (1)
39-49: Tests updated cleanly to per-type casesReplacing
.primitive(...)with.jsonArray,.jsonObject,.string,.number,.bool, and.datadirectly is clear and verifies the new construction paths end-to-end.Tests/AblyLiveObjectsTests/InternalDefaultRealtimeObjectsTests.swift (4)
1051-1051: Good migration to direct InternalLiveMapValue caseUsing .string("testValue") directly aligns with the removal of the primitive wrapper and keeps the test intent clear.
1081-1081: LGTM: direct string value in entriesSwitching to .string("stringValue") is consistent with the new API surface and the expected wire format.
1101-1101: Expectation matches updated internal data modelComparing against InternalObjectsMapEntry(ObjectData(string:)) is the right assertion after the model change.
1159-1159: LGTM: createMap uses new value shapePassing ["testKey": .string("testValue")] validates the new entry encoding path end-to-end.
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (5)
1240-1240: LGTM: invalid state test uses direct string valueThis keeps the test concise and correctly targets the state validation without the wrapper indirection.
1267-1277: Parametrized value coverage updated correctlyCovering jsonArray, jsonObject, string, number, bool, and data via direct cases mirrors the internal/public enum changes and exercises the ObjectData mapping thoroughly.
1319-1321: LGTM: publish-failure path uses direct stringThe change preserves the failure semantics while matching the new value shape.
2558-2570: Switching on direct cases simplifies assertionsPattern-matching on .data, .string, .number, .bool reads cleaner and avoids nesting. This is a good update for readability.
2950-2966: CreateMap primitive verification updated correctlyValidations against .data/.string/.number/.bool are accurate given the new API; also the explicit guard for jsonArray/jsonObject not being in the primitive fixtures is appropriate.
Sources/AblyLiveObjects/Internal/InternalLiveMapValue.swift (6)
5-10: Solid enum shape replacing the primitive wrapperIntroducing explicit cases for string/number/bool/data/jsonArray/jsonObject removes ambiguity and makes both equality and serialization straightforward.
21-32: Bridging from public LiveMapValue is correct and exhaustiveEach public case maps 1:1 to the corresponding internal case. This keeps PublicDefaultLiveMap/PublicDefaultRealtimeObjects bridges simple and reliable.
54-65: Realtime protocol mapping is accurate
- bool -> boolean
- data -> bytes
- number -> NSNumber(value:)
- string -> string
- jsonArray -> json.array
- jsonObject -> json.object
These match the wire schema expectations for ObjectsMapOp data.
93-139: Convenience accessors add clarityPer-type getters make tests and internal call sites much more readable than interrogating a single primitive wrapper.
145-163: Equatable semantics match public type and identity for live objectsValue cases compare by content; liveMap/liveCounter compare by identity. This is the right balance for reference-carrying variants.
5-10: No leftover uses ofPrimitiveObjectValueor.primitivefound — transition looks clean.I searched the repo for the exact symbols/patterns from the original comment and found no matches. The only remaining occurrences of "primitive" are in tests and documentation/comments:
- Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift — fixture names, test descriptions and comments referencing "primitive" (test data/fixtures).
- Sources/AblyLiveObjects/Public/PublicTypes.swift — public doc comments describing "primitive" value types.
- Sources/AblyLiveObjects/Utility/WireCodable.swift — comment: "Extracting primitive values from a dictionary".
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift — comment about handling primitive values per spec.
These are doc/tests-only references and do not reference the removed type/case; no code changes required.
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (9)
141-146: Nice use of literal conformances in fixturesUsing "stringValue" and "" directly as LiveMapValue keeps the fixture terse and validates the new ExpressibleBy*Literal conformances.
425-426: LGTM: createMap entries with literalsEntries ["shouldStay": "foo", "shouldDelete": "bar"] cleanly exercise the literal path and subsequent operations.
452-455: LGTM: map.set with literal valueUsing "baz" directly validates the set-path bridging to ObjectData(string:).
2558-2570: Switch cases updated to direct variantsMatching on .data, .string, .number, .bool is consistent with the new public API; the default guard is a reasonable safety net in test code.
2950-2966: Primitive verification for created maps is correctPost-create assertions for .data/.string/.number/.bool against map contents align with the payload shape emitted by PublicDefaultRealtimeObjects.createMap.
3036-3036: LGTM: createMap entries mixing literal and object referenceThis confirms interop between literal values and .liveCounter references in the same operation.
3061-3061: LGTM: client-side initial value for map via literalThis ensures initial map contents are installed locally when publish is suppressed.
3106-3106: LGTM: forge-and-echo flow uses literal entriesKeeps the test succinct while exercising server-applied initialization semantics.
3130-3131: LGTM: local-create test uses literal entryConfirms local initialization isn’t overridden by a subsequent CREATE op with lower applicability.
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
Show resolved
Hide resolved
lawrence-forooghian
force-pushed
the
ECO-5465-port-integration-tests
branch
from
c39d55f to
792683d
Compare
I copied this type blindly from JS in ce8c022, but in JS it doesn't introduce an annoying extra layer of indirection at the point of usage like it does here. Get rid of it.
Missed this in cb9a11c.
The conversion to use literal syntax was done by Cursor; I've vaguely satisfied myself that it's caught most of the usages but there may be some it's missed. Resolves #65.
lawrence-forooghian
force-pushed
the
65-LiveMapValue-improvements
branch
from
46de61a to
714988d
Compare
Note: This PR is based on top of #57; please review that one first.
Get rid of the annoying intermediate
PrimitiveObjectValue, and addExpressibleBy*Literalconformance.Resolves #65.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests