Skip to content

Conversation

@maratal
Copy link
Collaborator

@maratal maratal commented Dec 28, 2025

Closes #2167

Summary by CodeRabbit

  • New Features
    • Added support for optional query parameters when retrieving message versions, enabling enhanced filtering and customization of version retrieval requests.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 28, 2025

Walkthrough

This adds support for optional query parameters to the getMessageVersionsWithSerial method across multiple channel implementations (ARTChannel, ARTRealtimeChannel, ARTRestChannel, and ARTWrapperSDKProxyRealtimeChannel) by introducing new overloaded method signatures that accept a params dictionary while preserving backward compatibility.

Changes

Cohort / File(s) Summary
Channel Base & Protocol
Source/PrivateHeaders/Ably/ARTChannel.h, Source/include/Ably/ARTChannelProtocol.h, Source/ARTChannel.m
Added new getMessageVersionsWithSerial:params:wrapperSDKAgents:callback: method declaration and implementation to support optional query parameters for message version retrieval across the base channel interfaces.
Realtime Channel
Source/ARTRealtimeChannel.m, Source/ARTWrapperSDKProxyRealtimeChannel.m
Introduced new public method variant getMessageVersionsWithSerial:params:callback: on both ARTRealtimeChannel and ARTWrapperSDKProxyRealtimeChannel. Existing no-params method updated to delegate to the new variant with params:nil. Internal realtime channel replaced old signature with multi-parameter version forwarding to restChannel.
REST Channel
Source/ARTRestChannel.m
Added public method getMessageVersionsWithSerial:params:callback: and extended internal API signature to accept params. Implements conditional URL query parameter construction from the params dictionary, attaching parameters to the GET request only when present.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A param hopped through the channel flow,
From surface down to REST below,
Query strings now find their way,
Versions versioned, hip hooray! 🎉
No breaking changes here to stay.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly summarizes the main change: adding a missing params parameter to getMessageVersionsWithSerial as required by specification RSL14a.
Linked Issues check ✅ Passed The PR successfully restores the missing params parameter across all affected classes (ARTChannel, ARTRealtimeChannel, ARTRestChannel, ARTWrapperSDKProxyRealtimeChannel, and protocol definitions) while maintaining backward compatibility through method overloads.
Out of Scope Changes check ✅ Passed All changes are directly related to restoring the missing params parameter to getMessageVersionsWithSerial API as specified in the linked issues; no out-of-scope modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/2167-missing-parameter

📜 Recent review details

Configuration used: Organization 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.

📥 Commits

Reviewing files that changed from the base of the PR and between f0b0045 and 4f97af6.

📒 Files selected for processing (6)
  • Source/ARTChannel.m
  • Source/ARTRealtimeChannel.m
  • Source/ARTRestChannel.m
  • Source/ARTWrapperSDKProxyRealtimeChannel.m
  • Source/PrivateHeaders/Ably/ARTChannel.h
  • Source/include/Ably/ARTChannelProtocol.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.

Applied to files:

  • Source/ARTRestChannel.m
  • Source/ARTRealtimeChannel.m
  • Source/ARTWrapperSDKProxyRealtimeChannel.m
  • Source/ARTChannel.m
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: build
  • GitHub Check: check (macOS, test_macOS)
  • GitHub Check: check
  • GitHub Check: check (iOS, test_iOS18_4)
  • GitHub Check: check
  • GitHub Check: check (tvOS, test_tvOS18_4)
🔇 Additional comments (11)
Source/ARTChannel.m (1)

178-183: LGTM! Abstract method correctly added.

The new params-enabled variant follows the established pattern for abstract methods in this base class, consistent with other message operation methods like updateMessage and deleteMessage.

Source/include/Ably/ARTChannelProtocol.h (1)

107-116: LGTM! Protocol extension well-documented.

The new method variant properly extends the public API with clear documentation explaining the params parameter's purpose.

Source/PrivateHeaders/Ably/ARTChannel.h (1)

64-67: LGTM! Header declaration consistent with protocol.

The method signature correctly matches the protocol definition and maintains the established parameter naming conventions.

Source/ARTWrapperSDKProxyRealtimeChannel.m (2)

132-137: LGTM! Wrapper delegation correctly implemented.

The new method properly forwards all parameters including the wrapper SDK agents to the underlying channel's internal implementation.


139-144: LGTM! Backward compatibility maintained.

The existing method correctly delegates to the new variant with params:nil, preserving backward compatibility while leveraging the new implementation.

Source/ARTRealtimeChannel.m (3)

159-161: LGTM! Public API method correctly implemented.

The method properly delegates to the internal implementation with wrapperSDKAgents:nil, following the established pattern for public-facing channel methods.


163-165: LGTM! Backward compatibility preserved.

The existing method now delegates to the internal implementation with both params:nil and wrapperSDKAgents:nil, maintaining backward compatibility.


494-499: LGTM! Internal delegation chain complete.

The internal method correctly forwards all parameters to the REST channel implementation. ARTRestChannelInternal (lines 630–652 in ARTRestChannel.m) properly constructs NSURLQueryItems from the params parameter, completing the implementation chain.

Source/ARTRestChannel.m (3)

116-118: LGTM! Proper API addition with backward compatibility.

The new method variant correctly accepts the params parameter and delegates to the internal implementation. This maintains consistency with other similar methods in the class (like updateMessage and deleteMessage), and the approach of adding a new overload preserves backward compatibility.


120-122: LGTM! Clean backward-compatible routing.

The existing method correctly routes through the new internal implementation by passing params:nil, ensuring existing callers continue to work without modification.


631-631: LGTM! Implementation follows established patterns.

The params handling implementation is consistent with the existing _updateMessage method (lines 502-507) and properly:

  • Guards against nil/empty params dictionaries
  • Constructs query items using the ARTStringifiable protocol
  • Relies on NSURLComponents for safe URL encoding

The implementation matches established patterns in this file and safely handles the query parameter construction.

Also applies to: 647-653


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot temporarily deployed to staging/pull/2168/features December 28, 2025 18:16 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/2168/jazzydoc December 28, 2025 18:17 Inactive
@maratal maratal changed the base branch from main to 2156-rest-requests-building December 28, 2025 18:22
@maratal maratal changed the title Missing parameter [ECO-5669] Missing parameter Dec 28, 2025
@maratal maratal force-pushed the 2156-rest-requests-building branch from 986cadb to 58952a8 Compare January 3, 2026 20:37
@maratal maratal force-pushed the fix/2167-missing-parameter branch from 71925bb to c64f7f9 Compare January 3, 2026 20:58
@github-actions github-actions bot temporarily deployed to staging/pull/2168/features January 3, 2026 20:58 Inactive
@maratal maratal changed the base branch from 2156-rest-requests-building to fix/2166-docstrings-for-message-update-delete January 3, 2026 20:58
@maratal maratal marked this pull request as ready for review January 3, 2026 20:58
@github-actions github-actions bot temporarily deployed to staging/pull/2168/jazzydoc January 3, 2026 21:01 Inactive
@maratal maratal changed the title [ECO-5669] Missing parameter [ECO-5669] Missing parameter in getMessageVersionsWithSerial Jan 3, 2026
@maratal maratal force-pushed the fix/2166-docstrings-for-message-update-delete branch from bd6c333 to ae7e2e6 Compare January 5, 2026 17:34
Base automatically changed from fix/2166-docstrings-for-message-update-delete to main January 6, 2026 10:02
@maratal maratal changed the base branch from main to fix/2135-refactor-execute-request January 6, 2026 13:49
@maratal maratal changed the base branch from fix/2135-refactor-execute-request to 2156-rest-requests-building January 6, 2026 14:00
@maratal maratal force-pushed the fix/2167-missing-parameter branch from c64f7f9 to f0b0045 Compare January 8, 2026 22:51
@maratal maratal changed the base branch from 2156-rest-requests-building to main January 8, 2026 23:10
…method (by adding a new method to avoid breaking change).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Missing parameter in getMessageVersionsWithSerial

2 participants