Skip to content

fix(ios): honor echoMessages=false in client options - #614

Open
bobo928 wants to merge 1 commit into
ably:mainfrom
bobo928:codex/fix-ios-echo-messages
Open

bobo928 wants to merge 1 commit into
ably:mainfrom
bobo928:codex/fix-ios-echo-messages

Conversation

@bobo928

@bobo928 bobo928 commented Nov 3, 2026 •

Copy link
Copy Markdown

On iOS, ClientOptions(echoMessages: false) is decoded as true: READ_VALUE assigns the boxed NSNumber directly to the native BOOL, so the non-null @NO object becomes YES on arm64. This prevents callers from disabling echoes from their own connection.

Use the existing READ_BOOL macro for echoMessages so its value is unboxed with boolValue. Add an AblyFlutterCodecTests XCTest target covering explicit false, explicit true, a missing option, and NSNull. The tests encode the ClientOptions wire format with Flutter's standard writer and pass it through the production AblyFlutterReader; they exercise the native decoding boundary without connecting to Ably.

Validation on an arm64 iPhone 17 simulator running iOS 26.5, with Xcode 26.6 and Flutter 3.44.7:

  • The new testEchoMessagesFalseIsPreserved regression fails against unchanged upstream production code (xcodebuild exits with 65); the two existing iOS tests pass.
  • After the fix, ios/run_unit_tests.sh passes all six iOS tests: four codec tests and the two existing stream-channel tests.
  • flutter test --no-pub: all 65 tests pass.
  • flutter analyze --no-pub: no issues found, after resolving the root, example, and integration-test app dependencies.

This PR targets main (2.x). Would you consider backporting the same fix to the ably_flutter 1.2.x release line?

Summary by CodeRabbit

  • Bug Fixes
    • Client options now correctly preserve an explicitly supplied false value for message echoing. When the option is omitted or set to null, the native SDK default remains in effect.
  • Tests
    • Added coverage for true, false, omitted, and null values to verify client-option handling.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The iOS client-options reader now decodes echoMessages as a Boolean. A new XCTest target checks explicit true and false values and confirms that omitted or null values retain the native SDK default.

Changes

iOS client-options decoding

Layer / File(s) Summary
Boolean decoding and tests
ios/ably_pubsub_device_flutter/Package.swift, ios/ably_pubsub_device_flutter/Sources/ably_pubsub_device_flutter/codec/AblyFlutterReader.m, ios/ably_pubsub_device_flutter/Tests/AblyFlutterCodecTests/AblyFlutterReaderTests.m
The reader decodes echoMessages as a Boolean. The new test target and tests check explicit true and false values, and verify that omitted or null values retain the native default.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: uxduck, ttypic

Merge Risk: Minimal · up to c04e8

The change decodes explicit false as a Boolean and adds tests for the option values. No merge-blocking risk is identified.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title "fix(ios): honor echoMessages=false in client options" matches the main change. The diff switches the iOS decoding of ClientOptions.echoMessages from READ_VALUE to READ_BOOL, so an explicit …
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "fix(ios): honor echoMessages=false in client options" matches the main change. The diff switches the iOS decoding of ClientOptions.echoMessages from READ_VALUE to READ_BOOL, so an explicit false is no longer treated as true. The title names the platform, the affected option, and the intended behavior. The added AblyFlutterCodecTests target and its tests support the same fix. The title is short, specific, and in a conventional commit form.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (2 skipped: 2 unsupported.)

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

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

A rabbit checks the values as they flow,
True stays true, and false stays so.
When fields are missing, defaults remain,
Null leaves the native choice the same.
The codec hops along its way.

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: dfee3607-d72c-40ae-964f-e45157635731
Commits

Reviewing files that changed from the base of the PR and between 34b2214 and c04e8cb.

Files selected for processing: 3
  • ios/ably_pubsub_device_flutter/Package.swift
  • ios/ably_pubsub_device_flutter/Sources/ably_pubsub_device_flutter/codec/AblyFlutterReader.m
  • ios/ably_pubsub_device_flutter/Tests/AblyFlutterCodecTests/AblyFlutterReaderTests.m

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@bobo928

bobo928 commented Nov 4, 2026

Copy link
Copy Markdown
Author

Hi maintainers, could you please approve the CI workflows for this PR? Thanks!

This branch has not been deployed

No deployments
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.

1 participant