Skip to content

Send echo=true connection param by default (RTC1a) - #2318

Open
Tyagiquamar wants to merge 1 commit into
ably:mainfrom
Tyagiquamar:fix/2206-echo-messages-default
Open

Tyagiquamar wants to merge 1 commit into
ably:mainfrom
Tyagiquamar:fix/2206-echo-messages-default

Conversation

@Tyagiquamar

@Tyagiquamar Tyagiquamar commented Oct 2, 2026 •

Copy link
Copy Markdown

Per spec RTC1a, the echoMessages option (default true) is sent as the echo query parameter on the WebSocket connection URL. Previously only echo=false was sent when explicitly disabled; the default sent no param, relying on server-side default behavior.

  • src/common/lib/transport/connectionmanager.ts: always set params.echo ('true' unless echoMessages is false; transportParams overrides still apply after)
  • test/uts/realtime/unit/client/realtime_client.test.ts: enable the spec assertion (was skipped as a known deviation)
  • test/uts/deviations.md: remove the now-fixed RTC1a deviation entry

Validation (node:20-bookworm container, matching CI 18.x/20.x matrix): RTC1a tests pass (2/2); pre-fix the default test fails with expected null to equal 'true'. Full realtime_client.test.ts (22 passing) and channel_subscribe.test.ts incl. RTL7f echo=false (22 passing). prettier/eslint clean on changed files.

Fixes #2206

Summary by CodeRabbit

  • Bug Fixes

    • Connection requests now explicitly send the echo setting, including echo=true when message echoing is enabled.
  • Tests

    • The default echo setting is now checked without requiring a deviation override.

Per spec RTC1a, the echoMessages option (default true) is sent as the echo query parameter on the WebSocket connection URL. Previously only echo=false was sent when explicitly disabled; the default sent no param.
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: f5cf9809-41de-4bbc-b148-f5789f1d9d36

📥 Commits

Reviewing files that changed from the base of the PR and between 35269c8 and d2ab94b.

📒 Files selected for processing (3)
  • src/common/lib/transport/connectionmanager.ts
  • test/uts/deviations.md
  • test/uts/realtime/unit/client/realtime_client.test.ts
💤 Files with no reviewable changes (2)
  • test/uts/deviations.md
  • test/uts/realtime/unit/client/realtime_client.test.ts

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


Walkthrough

The connection parameters now include echo=true by default and echo=false when echoMessages is false. The default-setting test runs without a deviation-based skip, and the related deviation entry is removed.

Changes

Echo Parameter

Layer / File(s) Summary
Set and test the echo parameter
src/common/lib/transport/connectionmanager.ts, test/uts/realtime/unit/client/realtime_client.test.ts, test/uts/deviations.md
getConnectParams now sends an explicit echo value. The default-setting test runs without the RUN_DEVIATIONS skip, and the related deviation entry is removed.

Priority: ⬇️ Low

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

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: owenpearson

Merge Risk: ⚪ Minimal · up to d2ab9

Connections now explicitly carry the intended echo setting, and the default and opt-out WebSocket behaviors are asserted. No actionable merge-blocking risk remains in the reviewed change.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to d2ab9

The change makes an existing default explicit without changing the opt-out, authentication flow, or connection destinations. No introduced or worsened security risk was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — The affected client-side boundary is outbound Realtime connection establishment through WebSocket and Comet. Both receive the new default through the same parameter producer; no new connection destination is added by the change.

Trust Boundaries and Controls

  • observed — The echo assignment does not replace credential fields or change clientId, resume, or recover construction. Caller transportParams retains its pre-existing final override authority, including over echo; the PR does not introduce that configuration authority.
🚥 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 title clearly and concisely describes the main change: sending the echo=true connection parameter by default for RTC1a.
Linked Issues check ✅ Passed Issue [#2206] requires the default echoMessages: true setting to send echo=true on the WebSocket connection URL and requires a UTS assertion. TransportParams.getConnectParams now sets `params.ec…
Out of Scope Changes check ✅ Passed The changed files stay within issue [#2206]. The source change implements the required query parameter. The UTS change enables the required assertion. Removing the matching deviation entry updates tes…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Warning

Some tools did not complete. Review the errors below.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.


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 query string,
And finds echo=true in the spring.
When echoes take a quiet bend,
echo=false is what we send.
The test now runs each time anew,
While carrots cheer the change through!

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

@ttypic ttypic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for contribution

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.

echoMessages default does not send echo=true query parameter (RTC1a)

2 participants