[ECO-5577] Pre-v1 public API audit #358
Conversation
WalkthroughSwitched UI and internal logic to use message.serial instead of message.id; renamed ClientId* → ClientID* and clientIds → clientIDs across reactions and JSON; reaction API parameter labels changed to messageSerial; occupancy current() became a property; removed multiple Equatable conformances and added @mainactor for paginated results; assorted initializer and visibility tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as App UI
participant Reactions as MessageReactions
participant API as ChatAPI
participant Backend as Backend
participant Store as Message Store
User->>UI: tap reaction on message
UI->>Reactions: send(messageSerial: serial, params)
Reactions->>API: send(messageSerial: serial, params)
API->>Backend: POST reaction for messageSerial
Backend-->>API: ack + reaction event
API-->>Store: emit MessageReactionSummaryEvent
Store-->>UI: update message.reactions by serial
UI-->>User: render updated reactions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (11)
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 |
lawrence-forooghian
changed the base branch from
main
to
267-revert-typed-throws-crashes
lawrence-forooghian
force-pushed
the
pre-v1-audit
branch
from
a32a1e2 to
e7fb96b
Compare
lawrence-forooghian
force-pushed
the
pre-v1-audit
branch
from
e7fb96b to
8c56a88
Compare
lawrence-forooghian
force-pushed
the
pre-v1-audit
branch
from
8c56a88 to
57dbb76
Compare
lawrence-forooghian
force-pushed
the
pre-v1-audit
branch
from
57dbb76 to
666ebc8
Compare
lawrence-forooghian
changed the base branch from
267-revert-typed-throws-crashes
to
2025-10-03-enable-upcoming-language-features
lawrence-forooghian
force-pushed
the
2025-10-03-enable-upcoming-language-features
branch
from
f59a5b1 to
a43b818
Compare
lawrence-forooghian
force-pushed
the
pre-v1-audit
branch
from
cfe30a3 to
24aa93b
Compare
lawrence-forooghian
changed the base branch from
2025-10-03-enable-upcoming-language-features
to
remove-existential-from-public-api
lawrence-forooghian
force-pushed
the
pre-v1-audit
branch
from
24aa93b to
8a55244
Compare
lawrence-forooghian
changed the base branch from
remove-existential-from-public-api
to
rename-DefaultChatClient-to-ChatClient
I want to be careful about the conformances that we commit to in our public API now that we're locking it down for v1, especially when it looks like they were just added for our testing purposes. We can revisit in #10.
Not sure why I didn't do this in fc83fc1; probably I thought of it as a data container more than a client, but really it's a client given its methods for fetching further pages. (The compiler didn't require us to make it @mainactor because our internal implementation is currently going directly via the ably-cocoa paginated result type which isn't marked as @mainactor, but we want to be sure that we will be able to write a PaginatedResult implementation in any future client that _does_ enforce main actor isolation, e.g. upcoming ably-swift).
lawrence-forooghian
force-pushed
the
pre-v1-audit
branch
from
b2358ce to
612af75
Compare
In line with Swift's API Design Guidelines.
![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 (2)
Tests/AblyChatTests/ChatAPITests.swift (1)
150-162: Likely compile error: Version initializer missing clientIDEarlier in this file (Line 55) Version requires clientID. These initializations omit it.
- version: .init(serial: "3446456", timestamp: Date(timeIntervalSince1970: 1_730_943_049.269)), // from successGetMessagesWithItems + version: .init(serial: "3446456", timestamp: Date(timeIntervalSince1970: 1_730_943_049.269), clientID: "random"), // from successGetMessagesWithItems timestamp: Date(timeIntervalSince1970: 1_730_943_049.269), ), Message( serial: "3446457", action: .create, clientID: "random", text: "hello response", metadata: [:], headers: [:], - version: .init(serial: "3446457", timestamp: Date(timeIntervalSince1970: 1_730_943_051.269)), + version: .init(serial: "3446457", timestamp: Date(timeIntervalSince1970: 1_730_943_051.269), clientID: "random"), timestamp: Date(timeIntervalSince1970: 1_730_943_051.269), ),Sources/AblyChat/Messages.swift (1)
322-333: Restoreinit(message:)to public
External code in Example/AblyChatExample/Mocks/MockClients.swift (lines 147, 175, 190, 209) and Tests/AblyChatTests/MessageSubscriptionAsyncSequenceTests.swift (lines 35, 44–45) calls this initializer and will fail withinternalvisibility. Change it back topublic init(message:).
🧹 Nitpick comments (8)
Sources/AblyChat/SubscriptionAsyncSequence.swift (1)
97-112: Consider optimizing the termination handler pattern.The current implementation replaces
continuation.onTerminationevery time a handler is added. While correct, this has performance implications:
- Each call to
addTerminationHandlerrebuilds the entire closure that calls all accumulated handlers- Handlers added after stream termination will never execute (AsyncStream limitation)
Consider setting
onTerminationonce during initialization and having it iterate over the current state ofterminationHandlersat termination time. However, this would require careful synchronization sinceterminationHandlersis @mainactor isolated whileonTerminationmay be called from any context.Example alternative approach:
// In init(bufferingPolicy:) continuation.onTermination = { [weak self] _ in Task { @MainActor [weak self] in guard let self else { return } for handler in self.terminationHandlers { handler() } } }Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
88-91: Consider isolatingcurrentto MainActor.
currentis observed on the main actor. To avoid data races and match subscription callbacks, consider makingOccupancy.currentmain-actor isolated, or document its thread-safety.Tests/AblyChatTests/MessageSubscriptionAsyncSequenceTests.swift (1)
6-6: Main-actor–isolated Equatable conformance: verify toolchain support or fall back.
@MainActoron a specific protocol conformance is relatively new syntax. If your toolchain balks, prefer either:
- Annotate the type:
@MainActor private final class MockPaginatedResult..., or- Annotate the requirement:
- static func == (lhs: MockPaginatedResult<Item>, rhs: MockPaginatedResult<Item>) -> Bool { + @MainActor + static func == (lhs: MockPaginatedResult<Item>, rhs: MockPaginatedResult<Item>) -> Bool {Sources/AblyChat/RoomOptions.swift (1)
137-160: EquatableBox drift risk — add guardrailsManual mapping is easy to forget when options evolve. Add a small test ensuring equatableBox replays all properties (or generate with Sourcery as noted). A simple approach: assert that EquatableBox init mirrors default-initialized options and property-by-property mutations affect equality.
Also applies to: 162-174, 176-190, 192-204, 206-214, 216-228
Example/AblyChatExample/MessageViews/MessageReactionsSheet.swift (1)
20-31: Nit: name the loop variable clientID for consistencyAligns with new property clientIDs and broader ClientID naming.
- for (emoji, clientList) in uniqueOrDistinct { - for clientId in clientList.clientIDs { - let key = "\(clientId)-\(emoji)" + for (emoji, clientList) in uniqueOrDistinct { + for clientID in clientList.clientIDs { + let key = "\(clientID)-\(emoji)" reactions[key] = ReactionItem( emoji: emoji, - author: clientId, + author: clientID, count: 1, ) } }Tests/AblyChatTests/IntegrationTests.swift (1)
287-291: Remove duplicate serial assertionsSerial equality is asserted twice in each block. Keep one to reduce noise.
- #expect(rxEditedMessageFromSubscription.version.serial == txEditedMessage.version.serial) - #expect(rxEditedMessageFromSubscription.serial == txEditedMessage.serial) + #expect(rxEditedMessageFromSubscription.version.serial == txEditedMessage.version.serial) @@ - #expect(rxDeletedMessageFromSubscription.version.serial == txDeleteMessage.version.serial) - #expect(rxDeletedMessageFromSubscription.serial == txDeleteMessage.serial) + #expect(rxDeletedMessageFromSubscription.version.serial == txDeleteMessage.version.serial)Also applies to: 311-315
Tests/AblyChatTests/Helpers/Equatable.swift (1)
5-47: Scope Equatable operators to test-only (internal) visibilitypublic here is unnecessary in the test target; default/internal is enough.
-extension RoomStatus: Equatable { - public static func == (lhs: RoomStatus, rhs: RoomStatus) -> Bool { +extension RoomStatus: Equatable { + static func == (lhs: RoomStatus, rhs: RoomStatus) -> Bool { @@ -extension RoomStatusChange: Equatable { - public static func == (lhs: RoomStatusChange, rhs: RoomStatusChange) -> Bool { +extension RoomStatusChange: Equatable { + static func == (lhs: RoomStatusChange, rhs: RoomStatusChange) -> Bool { @@ -extension DiscontinuityEvent: Equatable { - public static func == (lhs: DiscontinuityEvent, rhs: DiscontinuityEvent) -> Bool { +extension DiscontinuityEvent: Equatable { + static func == (lhs: DiscontinuityEvent, rhs: DiscontinuityEvent) -> Bool { @@ -extension RoomOptions: Equatable { - public static func == (lhs: RoomOptions, rhs: RoomOptions) -> Bool { +extension RoomOptions: Equatable { + static func == (lhs: RoomOptions, rhs: RoomOptions) -> Bool {Also applies to: 49-53, 55-59, 61-65
Sources/AblyChat/Messages.swift (1)
313-315: Review Equatable removal and mutability change
ChatMessageEventno longer conforms toEquatableand its properties were changed fromlettovar, weakening the public API contract. Ensure no code or tests compare events with==and confirm the increased mutability is intentional.
📜 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 (34)
Example/AblyChatExample/ContentView.swift(6 hunks)Example/AblyChatExample/MessageViews/MessageReactionSummaryView.swift(2 hunks)Example/AblyChatExample/MessageViews/MessageReactionsSheet.swift(1 hunks)Example/AblyChatExample/Mocks/Misc.swift(0 hunks)Example/AblyChatExample/Mocks/MockClients.swift(7 hunks)Sources/AblyChat/ChatAPI.swift(1 hunks)Sources/AblyChat/DefaultMessageReactions.swift(2 hunks)Sources/AblyChat/DefaultOccupancy.swift(2 hunks)Sources/AblyChat/DefaultTyping.swift(3 hunks)Sources/AblyChat/DiscontinuityEvent.swift(1 hunks)Sources/AblyChat/Errors.swift(2 hunks)Sources/AblyChat/InternalError.swift(0 hunks)Sources/AblyChat/Message.swift(3 hunks)Sources/AblyChat/MessageReaction+JSON.swift(6 hunks)Sources/AblyChat/MessageReaction.swift(6 hunks)Sources/AblyChat/MessageReactions.swift(2 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(2 hunks)Sources/AblyChat/PaginatedResult.swift(4 hunks)Sources/AblyChat/Room.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(1 hunks)Sources/AblyChat/RoomOptions.swift(6 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/RoomStatus.swift(2 hunks)Sources/AblyChat/Rooms.swift(1 hunks)Sources/AblyChat/SubscriptionAsyncSequence.swift(1 hunks)Sources/AblyChat/Typing.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(3 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(6 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/DefaultTypingTests.swift(3 hunks)Tests/AblyChatTests/Helpers/Equatable.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(9 hunks)Tests/AblyChatTests/MessageSubscriptionAsyncSequenceTests.swift(2 hunks)
💤 Files with no reviewable changes (2)
- Example/AblyChatExample/Mocks/Misc.swift
- Sources/AblyChat/InternalError.swift
🧰 Additional context used
🧬 Code graph analysis (7)
Tests/AblyChatTests/IntegrationTests.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
Sources/AblyChat/MessageReactions.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
Example/AblyChatExample/ContentView.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
Sources/AblyChat/MessageReaction+JSON.swift (1)
Sources/AblyChat/JSONCodable.swift (4)
arrayValueForKey(94-104)optionalBoolValueForKey(219-233)objectValueForKey(58-68)optionalNumberValueForKey(181-195)
Example/AblyChatExample/Mocks/MockClients.swift (1)
Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (3)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)Sources/AblyChat/DefaultMessages.swift (2)
send(126-132)delete(142-148)
Sources/AblyChat/DefaultMessageReactions.swift (1)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)
🔇 Additional comments (31)
Sources/AblyChat/SubscriptionAsyncSequence.swift (1)
13-14: LGTM! Removing@uncheckedimproves type safety.The removal of
@uncheckedfrom theSendableconformance is appropriate and safe:
terminationHandlers(line 54) is @mainactor isolated, ensuring safe concurrent accessmode(line 55) is immutable, preventing data races- All associated types (
Mode,AsyncStream,AnyNonThrowingAsyncSequence) are properly marked asSendableThe documentation update warning about single iteration is also helpful and aligns with the expected behavior of AsyncStream-based sequences.
Sources/AblyChat/RoomReactions.swift (1)
111-113: Raw‐type removal on RoomReactionEventType is safe
RoomReactionEventTypeisn’t referenced by any.rawValueorinit(rawValue:)—all existing raw‐value lookups use the internalRoomReactionEventsenum. No action required.Sources/AblyChat/Room.swift (1)
201-201: Equatable removed from RoomStatusChange – confirm migration notes and usages.Pre‑v1 break makes sense; please ensure public docs/samples/tests are updated to avoid relying on Equatable and show comparing current/previous instead.
Sources/AblyChat/RoomStatus.swift (2)
6-6: RoomStatus no longer Equatable – check downstream constraints.Looks consistent with the API audit. Please verify no public generics/collections still require RoomStatus: Equatable (docs/samples too).
126-140: New helpers isReleasing/isReleased: LGTM.Consistent with existing case-check helpers.
Sources/AblyChat/Rooms.swift (1)
176-180: Switch to equatableBox for RoomOptions equality: good alignment with internalized equality.Please confirm equatableBox includes all nested option fields (messages/presence/occupancy/typing) and that tests cover equivalence/non‑equivalence cases.
Sources/AblyChat/DiscontinuityEvent.swift (1)
3-3: Equatable removed from DiscontinuityEvent – verify doc/sample updates.Ensure any public examples/tests no longer rely on Equatable and demonstrate comparing error properties instead.
Sources/AblyChat/ChatAPI.swift (1)
14-16: Make ChatAPI init internal: LGTM.Matches class visibility and pre‑v1 surface reduction. Please confirm no public docs/samples still instantiate ChatAPI directly.
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (2)
39-41: Confirm contract: get() does not mutatecurrent.This test assumes
currentstays nil afterget(). If implementation later caches results intocurrent, this will become brittle. Please confirm or document the contract.
125-126: Comment adjustment LGTM.Not testable due to fatalError is fine; aligns with the chosen failure semantics.
Tests/AblyChatTests/MessageSubscriptionAsyncSequenceTests.swift (1)
50-57: Method isolation LGTM.Annotating the test with
@MainActormatches pagination’s main-actor isolation.Example/AblyChatExample/ContentView.swift (1)
63-63: Approve change; no remainingmessage.idreferences
Migration tomessage.serialis consistent across UI flows; grep confirms no occurrences ofmessage.id. Consider adding a helper likeupdateMessage(serial:_:)to DRY the “find index by serial then replace item” pattern.Tests/AblyChatTests/DefaultTypingTests.swift (1)
98-99: No leftover oldclientIdlabels or properties All.init(clientID:)andchange.clientIDoccurrences are updated; nochange.clientIdorinit(clientId:)patterns remain.Sources/AblyChat/RoomOptions.swift (1)
6-39: Public Equatable removal + Sendable usage looks goodAPI surface is cleaner; internal equality handled elsewhere. No functional concerns.
Also applies to: 44-57, 62-87, 92-108, 113-115, 120-133
Sources/AblyChat/DefaultMessageReactions.swift (1)
22-22: LGTM on signature alignmentInternal methods now match protocol label changes; bodies unchanged and correct.
Also applies to: 43-43
Tests/AblyChatTests/IntegrationTests.swift (1)
121-136: Helper comparator for Messages is appropriateGood approach to ignore non-serial version fields in equality checks.
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)
27-29: LGTM on reaction API and ClientID renamesTests align with messageSerial labels and ClientIDList/clientIDs. Request/params expectations remain correct.
Also applies to: 71-74, 96-97, 149-164, 349-350, 388-389
Sources/AblyChat/MessageReactions.swift (1)
21-21: API label rename complete; nosend(to:)/delete(from:)usages found Approving.Sources/AblyChat/MessageReaction+JSON.swift (2)
69-81: Approve the naming update.Renaming
ClientIdListtoClientIDListand updatingclientIdstoclientIDsfollows Swift naming conventions where acronyms are capitalized consistently. The JSONKey still correctly mapsclientIdsfrom the backend format.
83-99: Approve the naming update.Renaming
ClientIdCountstoClientIDCountsand updatingclientIds/totalClientIdstoclientIDs/totalClientIDsmaintains consistency with the updated naming conventions. The JSONKey correctly maps backend keys (clientIds,totalClientIds) to the new property names.Sources/AblyChat/PaginatedResult.swift (4)
3-15: Approve the concurrency annotation.Adding
@MainActorto thePaginatedResultprotocol ensures all pagination operations run on the main thread, which is appropriate for UI-driven pagination. RemovingEquatablefrom the protocol while keeping it on the concrete implementation (PaginatedResultWrapper) reduces the protocol's requirements and allows non-equatable implementations if needed.
21-45: Concurrency context correctly propagated.The
@MainActorannotation onhandleResponseensures the continuation is resumed on the main actor, aligning with the protocol's requirements.
53-107: Equatable conformance correctly scoped.The
@MainActor Equatableconformance onPaginatedResultWrappercorrectly isolates equatable comparison to the main actor. This is necessary since the type is now main-actor-isolated.
111-114: Factory method correctly annotated.The
@MainActorannotation ontoPaginatedResultensures the factory method executes in the main actor context, maintaining consistency with the protocol requirements.Sources/AblyChat/Message.swift (3)
104-124: Approve the copy() refactor.The new implementation is more efficient—it mutates a local copy of
selfrather than constructing a newMessagewith all fields. This is cleaner and reduces the risk of accidentally omitting fields when the struct grows.
218-227: Approve the with(summaryEvent:) implementation.The method correctly validates that the event's
messageSerialmatches the message'sserialbefore applying the summary. The error code40000with status400is appropriate for a client-side validation error.
22-26: RemovedIdentifiableconformance. NoForEach/ListoverMessagefound inExample/orTests/; verify across all SwiftUI views that anyForEach/ListiteratingMessagecollections now includesid: \.serial.Sources/AblyChat/Occupancy.swift (3)
36-40: Confirm runtime guard forcurrentpropertyThe
currentproperty includes aguard options.enableEventscheck that triggersfatalErrorwhen occupancy events are disabled.
103-104: Confirm mutability of OccupancyEvent properties
No assignments found mutatingtypeoroccupancypost-initialization. Ensure changing them fromlettovaraligns with intended use cases; otherwise revert to immutable.
97-99: No references to OccupancyEventType.rawValue found; removal of String conformance is safe.Sources/AblyChat/Messages.swift (1)
306-310: String conformance removal from ChatMessageEventType is safe. No.rawValueusages found in the codebase.
There's no reason for a user to want to compare these, and I don't think that ARTErrorInfo has a useful -isEqual: implementation.
I believe that this was added for the example app (which no longer needs it), but I don't think it was the right approach; Identifiable is a context-specific concept and I don't think there's a one-size-fits-all conformance for Message.
There's no obvious reason for users to want to compare these and I think we should maintain the flexibility to add non-Equatable values (e.g. references to some other object) here.
There's no reason for a user to wish to compare two of these, and we want the flexibility to be able to add non-Equatable properties (e.g. a callback) in the future.
Not sure what the logic was for the `to:`, but it's not clear at the call site what needs to be passed for this value.
Not sure why we added this in d7e9adf; it's not part of the JS public API.
For consistency with how we handle this scenario for other features.
For consistency with the JS API and because it doesn't communicate very much (the method is mysterious either way).
lawrence-forooghian
force-pushed
the
pre-v1-audit
branch
from
4cf1bab to
52fe739
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/AblyChat/RoomReactions.swift (1)
117-118: Event properties should remain immutable.As noted in previous review comments, changing these properties from
lettovarmakes events mutable, which contradicts the typical semantics of events as immutable snapshots. Consider reverting toletunless there's a specific requirement for post-creation mutation.
📜 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 (21)
Example/AblyChatExample/ContentView.swift(6 hunks)Example/AblyChatExample/Mocks/MockClients.swift(7 hunks)Sources/AblyChat/DefaultMessageReactions.swift(2 hunks)Sources/AblyChat/DefaultOccupancy.swift(2 hunks)Sources/AblyChat/DiscontinuityEvent.swift(1 hunks)Sources/AblyChat/Errors.swift(2 hunks)Sources/AblyChat/InternalError.swift(0 hunks)Sources/AblyChat/Message.swift(3 hunks)Sources/AblyChat/MessageReactions.swift(2 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(2 hunks)Sources/AblyChat/Room.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(1 hunks)Sources/AblyChat/RoomOptions.swift(6 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/RoomStatus.swift(2 hunks)Sources/AblyChat/Rooms.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(6 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/Helpers/Equatable.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(9 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/InternalError.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- Tests/AblyChatTests/IntegrationTests.swift
- Sources/AblyChat/DefaultMessageReactions.swift
- Sources/AblyChat/Rooms.swift
- Tests/AblyChatTests/Helpers/Equatable.swift
- Sources/AblyChat/Messages.swift
🧰 Additional context used
🧬 Code graph analysis (4)
Example/AblyChatExample/ContentView.swift (3)
Sources/AblyChat/Message.swift (1)
with(218-227)Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
Example/AblyChatExample/Mocks/MockClients.swift (1)
Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
Sources/AblyChat/MessageReactions.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (2)
Example/AblyChatExample/Mocks/MockClients.swift (5)
send(161-177)send(249-266)send(322-333)delete(194-211)delete(268-278)Sources/AblyChat/DefaultMessageReactions.swift (2)
send(22-40)delete(43-59)
⏰ 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). (11)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: SPM (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: SPM,
releaseconfiguration (Xcode 26.0)
🔇 Additional comments (9)
Sources/AblyChat/DiscontinuityEvent.swift (1)
3-3: LGTM! Intentional breaking change as part of v1 API audit.Removing
Equatableconformance from the public API is appropriate for the v1 lockdown. The test suite maintains equality checking via separate test helpers, ensuring test coverage remains intact.Sources/AblyChat/RoomReactions.swift (1)
111-111: No rawValue usage found; verify serialization behavior.
No code references torawValue,init(rawValue:), orCodablewere detected—ensure JSON encoding/decoding and any messaging serialization still produce the intended string forRoomReactionEventType.Sources/AblyChat/RoomStatus.swift (2)
126-140: LGTM! Consistent API additions.The new
isReleasingandisReleasedcomputed properties follow the established pattern of the existing helper properties and provide convenient boolean checks for these states.
6-6: Document removal of Equatable conformance from RoomStatus. PublicRoomStatusno longer supports==(tests use a private helper extension to restore it). Ensure this breaking change is called out in release notes and the migration guide.Sources/AblyChat/RoomLifecycleManager.swift (1)
613-616: LGTM! TOCTOU issue correctly addressed.The guard statement now correctly inspects the captured
nextRoomStatusChange.currentstatus (rather than the potentially-advancedroomStatus), ensuring the condition and error cause refer to the same captured state. This resolves the race condition flagged in the previous review.Sources/AblyChat/Room.swift (1)
201-201: Document removal of Equatable conformance from RoomStatusChange.
No==usages found; tests include a test-only extension—this remains a breaking change, so update release notes and migration guide accordingly.Sources/AblyChat/Message.swift (1)
218-227: Serial guard matches spec.Nice touch adding the serial check before applying the summary—this lines up with CHA-M11e and keeps reaction updates from drifting onto the wrong message.
Sources/AblyChat/Occupancy.swift (2)
40-40: LGTM: Property-based API is more idiomatic.The change from a throwing method to a non-throwing property is appropriate for accessing cached state, and using a fatalError for programmer errors (accessing
currentwhen events are not enabled) follows Swift conventions.
97-99: Removing String raw‐value conformance is safe
Search found norawValue,Codableconformance, or JSONEncoder/Decoder usage forOccupancyEventType; its removal has no impact.
| } | ||
|
|
||
| func send(to messageSerial: String, params: SendMessageReactionParams) async throws(ARTErrorInfo) { | ||
| func send(messageSerial: String, params: SendMessageReactionParams) async throws(ARTErrorInfo) { |
There was a problem hiding this comment.
But it's not "sending message serial", it's sending a reaction to the message with message serial. Ditto for delete.
There was a problem hiding this comment.
I don't think that the existing signature makes it clear what the argument means, though. It's just a String without any additional information.
I agree that you're not "sending a message serial", the same way that in:
Messages.send(params: SendMessageParams)you're sending a message, not "sending a params"Presence.enter(data: PresenceData)you're not "entering a data"
etc.
And I'm not 100% sure what's the right signature for these things in Swift. Like, would the following be better, or just more verbose? I don't know.
MessageReactions.send(forMessageWithSerial:)Messages.send(withParams:)Presence.enter(withData:)
etc
There was a problem hiding this comment.
The key part of the API design guidelines though is "Prefer method and function names that make use sites form grammatical English phrases."
There was a problem hiding this comment.
So maybe the latter are better?
There was a problem hiding this comment.
I think so, more descriptive and reflects things correctly. Feel free to resolve this once you decide the names.
There was a problem hiding this comment.
OK — can I do that one in a separate PR just so I don't have to deal with merge conflicts on #378?
There was a problem hiding this comment.
Gonna merge this
There was a problem hiding this comment.
Also consider this: MessageReactions.send(for messageWithSerial:) @lawrence-forooghian but I'm not sure, leaving you to decide.
Have gone through the public API to check it before locking it down for v1. Here are a bunch of miscellaneous changes this resulted in. There are still a few more to come.
Docs changes in ably/docs#2868.
Summary by CodeRabbit
New Features
Refactor
Tests