Merged
Conversation
WalkthroughThe changes introduce comprehensive asynchronous unit tests for the Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant DefaultMessageReactions
participant MockHTTP
participant MockChannel
TestSuite->>DefaultMessageReactions: sendReactionToMessage()
DefaultMessageReactions->>MockHTTP: POST /send-reaction
MockHTTP-->>DefaultMessageReactions: HTTP Response
DefaultMessageReactions-->>TestSuite: Result/Error
TestSuite->>DefaultMessageReactions: deleteReactionFromMessage()
DefaultMessageReactions->>MockHTTP: DELETE /delete-reaction
MockHTTP-->>DefaultMessageReactions: HTTP Response
DefaultMessageReactions-->>TestSuite: Result/Error
TestSuite->>DefaultMessageReactions: subscribeToReactionSummaries()
DefaultMessageReactions->>MockChannel: Subscribe to summary events
MockChannel-->>DefaultMessageReactions: Event Data
DefaultMessageReactions-->>TestSuite: Parsed Event
TestSuite->>DefaultMessageReactions: subscribeRaw()
DefaultMessageReactions->>MockChannel: Subscribe to raw events
MockChannel-->>DefaultMessageReactions: Raw Event Data
DefaultMessageReactions-->>TestSuite: Parsed Event/Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
maratal
force-pushed
the
304-message-reactions-tests
branch
from
39a6c94 to
f078af2
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
f078af2 to
9b72e35
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
9b72e35 to
82bfaf3
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
82bfaf3 to
3a63c26
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
3a63c26 to
52c3dd2
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
52c3dd2 to
987931e
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
987931e to
15a9d60
Compare
Collaborator
Author
|
Coverage fails with: But those spec points exist in the main branch: What might be wrong here? @AndyTWF @lawrence-forooghian |
maratal
force-pushed
the
304-message-reactions-tests
branch
from
15a9d60 to
57ba666
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
57ba666 to
e737a23
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/ChatAPI.swift(1 hunks)Sources/AblyChat/DefaultMessageReactions.swift(5 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Sources/AblyChat/Message.swift:163-175
Timestamp: 2025-06-14T21:58:57.802Z
Learning: In the AblyChat Swift SDK, the JSON key "reactions" is used for parsing reaction summaries in the Chat SDK layer, while "summary" is used in the ably-cocoa layer. Reactions can be sourced from either the realtime message's summary property or directly from the "reactions" dictionary in the JSON.
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Example/AblyChatExample/MessageViews/MessageReactionsSheet.swift:15-31
Timestamp: 2025-06-14T16:18:47.038Z
Learning: In the AblyChat Swift SDK, there are three message reaction types: unique (one reaction per client per message), distinct (one reaction of each type per client per message), and multiple (any number of reactions including repeats, with counts). The MessageReactionsSheet specifically handles unique/distinct reactions where the count is always 1 per client per emoji. Only the "multiple" reaction type would have accumulating counts.
Learnt from: maratal
PR: ably/ably-chat-swift#165
File: Sources/AblyChat/Reaction.swift:14-14
Timestamp: 2024-12-10T01:59:02.065Z
Learning: For the 'ably-chat-swift' repository, documentation-only PRs should focus exclusively on public methods and properties. Avoid suggesting changes to internal comments or private methods in such PRs.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:29:39.712Z
Learning: The ably-chat-swift project uses Swift Testing framework (`#expect` macros) rather than XCTest for test assertions.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:29:39.712Z
Learning: The ably-chat-swift project uses Swift Testing framework (`#expect` macros) rather than XCTest for test assertions.
Learnt from: maratal
PR: ably/ably-chat-swift#249
File: Tests/AblyChatTests/DefaultMessagesTests.swift:45-48
Timestamp: 2025-05-22T19:17:21.392Z
Learning: The `let doIt = {}` closures in DefaultMessagesTests.swift are temporary workarounds for compiler crashes (related to issue #233) and will be removed once Xcode 16.3 is released, so they should not be flagged for missing async/throws annotations.
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Tests/AblyChatTests/Mocks/MockRealtime.swift:41-47
Timestamp: 2025-06-14T15:18:17.427Z
Learning: In the MockRealtime class in Tests/AblyChatTests/Mocks/MockRealtime.swift, the body parameter in the request method is constrained to dictionary or array types, but currently only dictionary types are used. Arrays are not yet used for this method, so the current implementation assumes dictionary-only for simplicity.
Sources/AblyChat/ChatAPI.swift (8)
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Sources/AblyChat/Message.swift:163-175
Timestamp: 2025-06-14T21:58:57.802Z
Learning: In the AblyChat Swift SDK, the JSON key "reactions" is used for parsing reaction summaries in the Chat SDK layer, while "summary" is used in the ably-cocoa layer. Reactions can be sourced from either the realtime message's summary property or directly from the "reactions" dictionary in the JSON.
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Example/AblyChatExample/MessageViews/MessageReactionsSheet.swift:15-31
Timestamp: 2025-06-14T16:18:47.038Z
Learning: In the AblyChat Swift SDK, there are three message reaction types: unique (one reaction per client per message), distinct (one reaction of each type per client per message), and multiple (any number of reactions including repeats, with counts). The MessageReactionsSheet specifically handles unique/distinct reactions where the count is always 1 per client per emoji. Only the "multiple" reaction type would have accumulating counts.
Learnt from: maratal
PR: ably/ably-chat-swift#165
File: Sources/AblyChat/Reaction.swift:14-14
Timestamp: 2024-12-10T01:59:02.065Z
Learning: For the 'ably-chat-swift' repository, documentation-only PRs should focus exclusively on public methods and properties. Avoid suggesting changes to internal comments or private methods in such PRs.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/SubscriptionAsyncSequence.swift:36-41
Timestamp: 2025-06-29T16:02:43.988Z
Learning: In the AblyChat Swift codebase, the fatalError in SubscriptionAsyncSequence.swift's mock async sequence handling (around line 39) is intentional and should not be flagged. This fatalError catches programmer errors when a throwing AsyncSequence is incorrectly passed to the mock initializer, which is meant for testing/development purposes.
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Tests/AblyChatTests/Mocks/MockRealtime.swift:41-47
Timestamp: 2025-06-14T15:18:17.427Z
Learning: In the MockRealtime class in Tests/AblyChatTests/Mocks/MockRealtime.swift, the body parameter in the request method is constrained to dictionary or array types, but currently only dictionary types are used. Arrays are not yet used for this method, so the current implementation assumes dictionary-only for simplicity.
Learnt from: maratal
PR: ably/ably-chat-swift#249
File: Tests/AblyChatTests/DefaultMessagesTests.swift:45-48
Timestamp: 2025-05-22T19:17:21.392Z
Learning: The `let doIt = {}` closures in DefaultMessagesTests.swift are temporary workarounds for compiler crashes (related to issue #233) and will be removed once Xcode 16.3 is released, so they should not be flagged for missing async/throws annotations.
Learnt from: maratal
PR: ably/ably-chat-swift#165
File: docs-coverage-report:5-8
Timestamp: 2024-12-10T01:59:12.404Z
Learning: In the AblyChat Swift project, documentation coverage should focus on public methods and properties. Private and internal entities do not require documentation.
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Example/AblyChatExample/ContentView.swift:94-99
Timestamp: 2025-06-14T21:47:20.509Z
Learning: In the Ably Chat Swift example app, using fatalError for missing clientID is intentional and appropriate since it represents a programmer error rather than a recoverable runtime condition. The clientID is considered an essential configuration requirement.
Sources/AblyChat/DefaultMessageReactions.swift (17)
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Example/AblyChatExample/MessageViews/MessageReactionsSheet.swift:15-31
Timestamp: 2025-06-14T16:18:47.038Z
Learning: In the AblyChat Swift SDK, there are three message reaction types: unique (one reaction per client per message), distinct (one reaction of each type per client per message), and multiple (any number of reactions including repeats, with counts). The MessageReactionsSheet specifically handles unique/distinct reactions where the count is always 1 per client per emoji. Only the "multiple" reaction type would have accumulating counts.
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Sources/AblyChat/Message.swift:163-175
Timestamp: 2025-06-14T21:58:57.802Z
Learning: In the AblyChat Swift SDK, the JSON key "reactions" is used for parsing reaction summaries in the Chat SDK layer, while "summary" is used in the ably-cocoa layer. Reactions can be sourced from either the realtime message's summary property or directly from the "reactions" dictionary in the JSON.
Learnt from: maratal
PR: ably/ably-chat-swift#249
File: Tests/AblyChatTests/DefaultMessagesTests.swift:45-48
Timestamp: 2025-05-22T19:17:21.392Z
Learning: The `let doIt = {}` closures in DefaultMessagesTests.swift are temporary workarounds for compiler crashes (related to issue #233) and will be removed once Xcode 16.3 is released, so they should not be flagged for missing async/throws annotations.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The `Occupancy` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultOccupancy` automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:11-14
Timestamp: 2025-05-12T21:03:31.914Z
Learning: The `Typing` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultTyping` automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:11-14
Timestamp: 2025-05-12T21:03:31.914Z
Learning: The `Typing` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultTyping` automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: ably/ably-chat-swift#165
File: Sources/AblyChat/Reaction.swift:14-14
Timestamp: 2024-12-10T01:59:02.065Z
Learning: For the 'ably-chat-swift' repository, documentation-only PRs should focus exclusively on public methods and properties. Avoid suggesting changes to internal comments or private methods in such PRs.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:131-138
Timestamp: 2025-05-12T21:04:36.263Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle closures should capture self weakly with `[weak self]` to avoid potential retain cycles, particularly when calling channel.unsubscribe() within the closure.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/SubscriptionAsyncSequence.swift:36-41
Timestamp: 2025-06-29T16:02:43.988Z
Learning: In the AblyChat Swift codebase, the fatalError in SubscriptionAsyncSequence.swift's mock async sequence handling (around line 39) is intentional and should not be flagged. This fatalError catches programmer errors when a throwing AsyncSequence is incorrectly passed to the mock initializer, which is meant for testing/development purposes.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/RoomReactions.swift:42-56
Timestamp: 2025-05-16T21:35:46.634Z
Learning: In the AblyChat framework, all subscription callbacks are annotated with @MainActor, ensuring they always execute on the main actor thread without requiring additional Task wrappers.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:29:39.712Z
Learning: In the ably-chat-swift project, MockRealtimeChannel processes messages synchronously when subscribe is called, immediately delivering the configured messages to the callback.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:29:39.712Z
Learning: In the ably-chat-swift project, MockRealtimeChannel processes messages synchronously when subscribe is called, immediately delivering the configured messages to the callback.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/SubscriptionHandleStorage.swift:37-40
Timestamp: 2025-05-12T21:02:28.274Z
Learning: In the AblyChat Swift codebase, the `SubscriptionHandle` struct has an `unsubscribe()` method for terminating subscriptions, not a `cancel()` method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/RoomLifecycleManager.swift:611-624
Timestamp: 2025-05-22T20:57:48.147Z
Learning: In AblyChat Swift, when using continuations with subscription callbacks, the continuation object is captured directly by the closure and doesn't depend on `self`. Even with `[weak self]` capture, the continuation will still be resumed properly.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription callbacks continue to work even when the handle is not stored, as the underlying services maintain their own storage of callbacks that remain registered until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription continues to work even if the handle is not stored, as the underlying services maintain references to the callbacks until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:20:38.904Z
Learning: In Swift tests using mock objects like MockRealtimeChannel, explicit subscription cleanup (via unsubscribe() or similar methods) is generally unnecessary since the mocks don't maintain real connections and will be garbage collected after the test completes.
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (14)
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Example/AblyChatExample/MessageViews/MessageReactionsSheet.swift:15-31
Timestamp: 2025-06-14T16:18:47.038Z
Learning: In the AblyChat Swift SDK, there are three message reaction types: unique (one reaction per client per message), distinct (one reaction of each type per client per message), and multiple (any number of reactions including repeats, with counts). The MessageReactionsSheet specifically handles unique/distinct reactions where the count is always 1 per client per emoji. Only the "multiple" reaction type would have accumulating counts.
Learnt from: maratal
PR: ably/ably-chat-swift#249
File: Tests/AblyChatTests/DefaultMessagesTests.swift:45-48
Timestamp: 2025-05-22T19:17:21.392Z
Learning: The `let doIt = {}` closures in DefaultMessagesTests.swift are temporary workarounds for compiler crashes (related to issue #233) and will be removed once Xcode 16.3 is released, so they should not be flagged for missing async/throws annotations.
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Sources/AblyChat/Message.swift:163-175
Timestamp: 2025-06-14T21:58:57.802Z
Learning: In the AblyChat Swift SDK, the JSON key "reactions" is used for parsing reaction summaries in the Chat SDK layer, while "summary" is used in the ably-cocoa layer. Reactions can be sourced from either the realtime message's summary property or directly from the "reactions" dictionary in the JSON.
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Example/AblyChatExample/Mocks/MockRealtime.swift:348-384
Timestamp: 2025-06-14T16:19:29.327Z
Learning: The MockRealtime class in Example/AblyChatExample/Mocks/MockRealtime.swift is specifically for example app construction, not for testing. The actual test mocks are located in the Tests directory. If example app mocks are unused, fatalError calls are appropriate to catch accidental usage.
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Tests/AblyChatTests/Mocks/MockRealtime.swift:41-47
Timestamp: 2025-06-14T15:18:17.427Z
Learning: In the MockRealtime class in Tests/AblyChatTests/Mocks/MockRealtime.swift, the body parameter in the request method is constrained to dictionary or array types, but currently only dictionary types are used. Arrays are not yet used for this method, so the current implementation assumes dictionary-only for simplicity.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:29:39.712Z
Learning: The ably-chat-swift project uses Swift Testing framework (`#expect` macros) rather than XCTest for test assertions.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:29:39.712Z
Learning: The ably-chat-swift project uses Swift Testing framework (`#expect` macros) rather than XCTest for test assertions.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:11-14
Timestamp: 2025-05-12T21:03:31.914Z
Learning: The `Typing` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultTyping` automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:11-14
Timestamp: 2025-05-12T21:03:31.914Z
Learning: The `Typing` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultTyping` automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/SubscriptionAsyncSequence.swift:36-41
Timestamp: 2025-06-29T16:02:43.988Z
Learning: In the AblyChat Swift codebase, the fatalError in SubscriptionAsyncSequence.swift's mock async sequence handling (around line 39) is intentional and should not be flagged. This fatalError catches programmer errors when a throwing AsyncSequence is incorrectly passed to the mock initializer, which is meant for testing/development purposes.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:29:39.712Z
Learning: In the ably-chat-swift project, MockRealtimeChannel processes messages synchronously when subscribe is called, immediately delivering the configured messages to the callback.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:29:39.712Z
Learning: In the ably-chat-swift project, MockRealtimeChannel processes messages synchronously when subscribe is called, immediately delivering the configured messages to the callback.
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:20:38.904Z
Learning: In Swift tests using mock objects like MockRealtimeChannel, explicit subscription cleanup (via unsubscribe() or similar methods) is generally unnecessary since the mocks don't maintain real connections and will be garbage collected after the test completes.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/Mocks/MockClients.swift:0-0
Timestamp: 2025-05-16T21:04:26.244Z
Learning: In the ably-chat-swift project, mock implementations (like those in MockClients.swift) are intentionally kept simple, sometimes omitting parameter-based filtering behavior for testing simplicity.
🧬 Code Graph Analysis (1)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (5)
Sources/AblyChat/ChatAPI.swift (1)
sendReactionToMessage(201-216)Sources/AblyChat/DefaultMessageReactions.swift (4)
send(22-40)delete(43-59)subscribe(62-97)subscribeRaw(100-143)Example/AblyChatExample/Mocks/MockClients.swift (13)
send(158-173)send(245-262)send(318-329)delete(192-207)delete(264-274)subscribe(125-152)subscribe(276-300)subscribe(331-348)subscribe(362-378)subscribe(530-532)subscribe(534-536)subscribe(550-561)subscribeRaw(302-304)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
subscribe(140-142)subscribe(145-151)
🔇 Additional comments (8)
Sources/AblyChat/ChatAPI.swift (1)
202-202: Documentation fix looks correct.The spec reference has been properly updated from "(CHA-MR11a1)" to "(CHA-MR4a1)" to match the send reaction method context. This aligns with the specification structure where CHA-MR4 covers sending reactions and CHA-MR11 covers deleting reactions.
Sources/AblyChat/DefaultMessageReactions.swift (3)
10-10: Clean refactoring to use MessagesOptions.The change from storing
defaultReactionto storing the fulloptionsobject is a good improvement. It provides better encapsulation and allows access to other message-related configuration in the future without additional refactoring.Also applies to: 18-18, 30-30, 44-44
103-107: Intentional breaking change: fatalError instead of throwing.The change from throwing an error to using
fatalErroris a breaking API change. However, the comment clearly explains this is intentional because callingsubscribeRawwithout proper configuration is a programmer error that should be caught during development, not a runtime condition.This aligns with the established pattern in the codebase for handling programmer errors.
128-128: Simplified reaction name handling.Good simplification - removing the special case logic for delete operations and always using the fallback to empty string aligns with the CHA-MR7b3 specification requirements for default values.
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (4)
7-60: Excellent test coverage for message reactions.The test comprehensively covers both sending and deleting reactions with proper verification of HTTP request parameters. The spec references are well documented.
114-168: Well-structured subscription test with comprehensive type validation.Great test coverage for reaction summaries including proper type checking and validation of all three reaction types (unique, distinct, multiple). The synchronous callback verification ensures the mock behavior is working as expected.
203-246: Good validation of raw reaction subscription requirements.The test properly sets up the room with
rawMessageReactions: trueoption, which is required for thesubscribeRawmethod to work without triggering the fatalError added in the implementation.
332-408: Comprehensive coverage of default reaction type configuration.Both tests effectively validate the default reaction type behavior - custom configuration with
.multipleand the default.distincttype. This ensures the refactored options-based approach works correctly.
maratal
force-pushed
the
304-message-reactions-tests
branch
from
e737a23 to
8558e46
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
8558e46 to
8765aad
Compare
…Info(chatError:...`.
maratal
force-pushed
the
301-all-errors-via-ChatError
branch
from
2065a8a to
a87908c
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
8765aad to
079a904
Compare
maratal
force-pushed
the
301-all-errors-via-ChatError
branch
from
a87908c to
42874c6
Compare
maratal
force-pushed
the
304-message-reactions-tests
branch
from
079a904 to
3f590f8
Compare
…e beginning of the new line).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
Sources/BuildTool/BuildTool.swift (2)
304-313: Regex still too rigid; allow intra-token whitespace and trim captured ID to fix missed spec points (e.g., CHA-MR3b, CHA-MR7b).Your change allows indentation before “*”, but the pattern still assumes a single literal space between tokens and doesn’t trim the captured ID. If the spec has variable whitespace (e.g., “**@( CHA-MR3b )@” or multiple spaces), the ID will be captured with spaces and won’t match test tags, leading to “conformance to nonexistent spec points.”
Adjust the regex to tolerate whitespace around tokens and trim the ID:
- let specPointLineRegex = /^\s*\*+ @\((.*?)\)@( @\[Testable\]@ )?/ + // Tolerate whitespace between list marker, ID tokens, and Testable marker + let specPointLineRegex = /^\s*\*+\s*@\(\s*(.*?)\s*\)@\s*( @\[\s*Testable\s*\]@\s*)?/ @@ - id = String(match.output.1) + // Trim any whitespace the spec may include inside @( ... )@ + id = String(match.output.1).trimmingCharacters(in: .whitespacesAndNewlines) isTestable = match.output.2 != nilI can add a unit test for SpecPoint parsing that covers these whitespace variations if useful.
363-401: Conformance tag regex: handle // vs ///, no-indent lines, multi-digit OneOf counts, and stricter ID capture.Current pattern requires indentation before “//”, supports only single-digit m/n for @specOneOf, and may be too permissive around ID spacing. This can cause legitimate tags to be missed.
Make the matcher more permissive and precise without changing capture indices used below:
- let conformanceTagSourceLineRegex = /^\s+\/\/ @spec(OneOf|Partial|Untested|NotApplicable)?(?:\((\d)?\/(\d)?\))? (.*?)(?: - (.*))?$/ + // - Allow 0+ indent and // or /// (doc comments) + // - Support multi-digit OneOf counts + // - Require at least one space before the ID, and trim-able capture for ID/comments + let conformanceTagSourceLineRegex = /^\s*\/\/+ @spec(OneOf|Partial|Untested|NotApplicable)?(?:\((\d+)\/(\d+)\))?\s+([A-Za-z0-9-]+)(?:\s*-\s*(.*))?$/This keeps:
- match.output.1 → type (“OneOf”, “Partial”, etc.)
- match.output.2 → index (now multi-digit)
- match.output.3 → total (now multi-digit)
- match.output.4 → specPointID
- match.output.5 → optional comment
If any tags rely on characters beyond [A-Za-z0-9-] in IDs, widen that character class accordingly, but this should match current CHA-* IDs.
If needed, I can scan the test sources to confirm if any tags use /// or 0 indent and adjust accordingly.
🧹 Nitpick comments (3)
Sources/BuildTool/BuildTool.swift (3)
456-462: Add quick debug output when spec-point IDs are invalid.When throwing conformanceToNonexistentSpecPoints, logging the commit SHA and a small sample of matched spec IDs helps diagnosis.
I can submit a small follow-up adding a debug flag to print e.g. first 10 parsed IDs and the offending IDs to stderr before throwing.
580-593: Guard against division by zero in coverage percentage.If there are zero testable points (edge case), double division will be invalid.
- percentageFormatter.string(from: NSNumber(value: Double(specPointCount) / Double(summary.testableSpecPointCount)))!, + summary.testableSpecPointCount > 0 + ? percentageFormatter.string(from: NSNumber(value: Double(specPointCount) / Double(summary.testableSpecPointCount)))! + : "n/a",
653-675: Optional: echo the spec commit SHA in the coverage report output.Helps correlate CI logs with the exact spec version used.
Append a line like “Spec commit: ” to the printed summary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/BuildTool/BuildTool.swift(1 hunks)
🔇 Additional comments (1)
Sources/BuildTool/BuildTool.swift (1)
299-314: Spec point regex covers all CHA-MR lines in the live spec*The verification script confirmed that the proposed
specPointLineRegexmatches every CHA-MR* ID (including CHA-MR3b, 3b2, 3b3, 6a, 6a3, 7b, 7b1, 7b3) in the currentchat-features.textile. No changes are needed.
umair-ably
approved these changes
Aug 12, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #304
Summary by CodeRabbit
Refactor
Tests