Skip to content

Conversation

@AndyTWF
Copy link
Contributor

@AndyTWF AndyTWF commented Oct 8, 2025

We currently have the RTC17 clientId declared in ably.d.ts but it's not implemented anywhere, so it always returns undefined. This change adds a getter to BaseRealtime.

Summary by CodeRabbit

  • New Features

    • Realtime client now exposes the authenticated client ID via a clientId property, letting apps read the identifier directly without changing behavior or error handling.
  • Tests

    • Added tests to verify the realtime client mirrors the authentication client ID during connection, ensuring the exposed clientId matches the auth-provided identifier.

@coderabbitai
Copy link

coderabbitai bot commented Oct 8, 2025

Walkthrough

Adds a public getter clientId to BaseRealtime that returns this.auth.clientId. Adds a test (RTC17) that verifies a realtime client’s clientId mirrors the auth client's clientId when connected via an authCallback.

Changes

Cohort / File(s) Change Summary
Realtime client API
src/common/lib/client/baserealtime.ts
Added public getter get clientId() on BaseRealtime that proxies to this.auth.clientId.
Realtime auth tests
test/realtime/auth.test.js
Added test case RTC17 asserting realtime.clientId equals the provided auth clientId on successful connection; includes error handling and cleanup.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A twitch of whiskers, code aligned,
A getter peeks where auth's defined.
Tests hop in to prove the tie,
clientId matches, no surprise.
I nibble carrots, CI green and kind. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title uses conventional commit style and succinctly describes the main change of implementing the RTC17 clientId getter, matching the PR’s objective to fix the undefined clientId issue without including unnecessary details.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ 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 rtc-17-implementation

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/2100/features October 8, 2025 15:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/2100/bundle-report October 8, 2025 15:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/2100/typedoc October 8, 2025 15:32 Inactive
@AndyTWF AndyTWF force-pushed the rtc-17-implementation branch from ebee036 to 10f1553 Compare October 8, 2025 16:08
We currently have the RTC17 clientId declared in `ably.d.ts` but it's
not implemented anywhere, so it always returns undefined. This change
adds a getter to `BaseRealtime`.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
test/realtime/auth.test.js (1)

1317-1349: LGTM! Test correctly verifies RTC17 spec.

The test properly verifies that the new realtime.clientId getter returns the expected value from auth.clientId. The implementation follows established patterns and includes appropriate error handling.

Optional: Consider reducing duplication with existing test.

This test is nearly identical to auth_jwt_with_clientid (lines 1216-1245), with the only difference being the use of realtime.clientId instead of realtime.auth.clientId. Consider consolidating these tests or extracting common logic to reduce maintenance burden:

/**
 * @spec RSA7b3
 * @spec RTC17
 * @specpartial RSA8g - authCallback returned JWT token string
 */
it('auth_jwt_with_clientid', function (done) {
  const helper = this.test.helper;
  var currentKey = helper.getTestApp().keys[0];
  var keys = { keyName: currentKey.keyName, keySecret: currentKey.keySecret };
  var clientId = 'testJWTClientId';
  helper.recordPrivateApi('call.Utils.mixin');
  var params = helper.Utils.mixin(keys, { clientId: clientId });
  var authCallback = function (tokenParams, callback) {
    getJWT(params, helper, callback);
  };

  var realtime = helper.AblyRealtime({ authCallback: authCallback });

  realtime.connection.on('connected', function () {
    try {
      expect(realtime.auth.clientId).to.equal(clientId);
      // RTC17: Verify realtime.clientId getter mirrors auth.clientId
      expect(realtime.clientId).to.equal(clientId);
      realtime.connection.close();
      done();
    } catch (err) {
      done(err);
    }
    return;
  });

  realtime.connection.on('failed', function (err) {
    realtime.close();
    done(err);
    return;
  });
});

This approach would reduce code duplication while maintaining comprehensive test coverage for both the auth clientId and the RTC17 getter.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between ebee036 and 6f33494.

📒 Files selected for processing (2)
  • src/common/lib/client/baserealtime.ts (1 hunks)
  • test/realtime/auth.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/common/lib/client/baserealtime.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/realtime/auth.test.js (1)
src/common/lib/client/baserealtime.ts (1)
  • clientId (86-89)
⏰ 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). (7)
  • GitHub Check: test-browser (webkit)
  • GitHub Check: test-node (18.x)
  • GitHub Check: test-node (16.x)
  • GitHub Check: test-node (20.x)
  • GitHub Check: test-npm-package
  • GitHub Check: test-browser (firefox)
  • GitHub Check: test-browser (chromium)

@AndyTWF AndyTWF merged commit 4967374 into main Oct 9, 2025
18 of 20 checks passed
@AndyTWF AndyTWF deleted the rtc-17-implementation branch October 9, 2025 08:21
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.

3 participants