Skip to content

feat: add error reason for auth token - #197

Merged
thisisamank merged 2 commits into
stagingfrom
authtoken-error-message-ui
Nov 5, 2026
Merged

thisisamank merged 2 commits into
stagingfrom
authtoken-error-message-ui

Conversation

@thisisamank

Copy link
Copy Markdown
Contributor

Description

This PR utilizes error messages reason for invalid auth tokens, webcore already has implementation of this: https://github.com/cloudflare/realtimekit/pull/2137

switch (code) {
case '0004':
return { message: t('init.auth_error'), code };
switch ((err as any)?.reason) {

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.

reason has to be set inside the ClientError constructor for this to ever match.

In @cloudflare/realtimekit@2.0.5-staging.2 (the version pinned in packages/core/package.json), dist/index.es.js dispatches the window event from the constructor body itself:

class R extends Error {
  constructor(e, r, i = void 0, a = !1) {
    super(e);
    this.code = r, this.name = "ClientError";
    ...
    typeof window != "undefined" && window.dispatchEvent(new CustomEvent("ClientError", {
      detail: this
    }));
  }
}

detail is the error instance and dispatchEvent is synchronous, so the initErrorListener in rtk-meeting / rtk-ui-provider runs before the constructor returns. If cloudflare/realtimekit#2137 assigns the reason after construction (const e = new ClientError(...); e.reason = 'participant_token_expired'), (err as any)?.reason is undefined here and every 0004 silently falls back to init.auth_error — the five new strings would be unreachable.

Also worth confirming before merge: the pinned ^2.0.5-staging.2 has no reason on ClientError at all (ERROR_CODES['0004'] errors are built as new R(message, '0004')), so this mapping can't be exercised against the current SDK. Please bump the dev dependency to a version that includes #2137 and verify the reason strings match the webcore literals exactly — a typo in any of the five would degrade silently to the generic message with no type error, since the switch operates on any.

@ask-bonk

ask-bonk Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review: 1 finding.

Submitted 1 inline comment.

Needs Fix

  • packages/core/src/utils/init-error.ts:24 — (err as any)?.reason may never be populated. The SDK dispatches the ClientError window event synchronously from inside the ClientError constructor with detail: this (verified in @cloudflare/realtimekit@2.0.5-staging.2, dist/index.es.js). If webcore sets reason after construction rather than as a constructor argument, the listener in rtk-meeting / rtk-ui-provider reads it before assignment and every 0004 falls back to init.auth_error, making all five new strings dead. Additionally, the pinned ^2.0.5-staging.2 has no reason on ClientError, so the mapping cannot be exercised against the current SDK — the dev dependency should be bumped to a build containing cloudflare/realtimekit#2137 so the behaviour is verifiable.

Suggestions

  • The five reason literals are untyped magic strings matched against any. A typo would degrade silently to the generic message with no compile error. Once the SDK exports a reason union type, narrow err to ClientError (already imported elsewhere, e.g. rtk-recording-toggle.tsx:1) instead of as any, so the strings are checked.
  • init-error.ts has no spec file while sibling utils do (config.spec.ts, string.spec.ts, etc.). A small table-driven test over getInitErrorInfo would lock the five reason→key mappings and the default fallback.
  • init.token_invalid and init.auth_error are near-duplicates in intent; consider whether the extra key earns its place or whether participant_token_invalid should just use the existing fallback.

github run

Comment thread packages/core/src/utils/init-error.ts Resolved
@ask-bonk

ask-bonk Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

I verified the SDK behavior that was previously in question, by unpacking the newly pinned dependency.

Previous blocking concern is resolved. In @cloudflare/realtimekit@2.1.0-staging.1, reason is now a constructor parameter assigned before the window event dispatch:

class k extends Error {
  constructor(e, r, i = void 0, a = !1, o = void 0) {
    super(e);
    this.code = r, this.name = "ClientError", this.reason = o;
    ...
    window.dispatchEvent(new CustomEvent("ClientError", { detail: this }));

and the 401/403 path passes it through: throw new k(h1(Z), "0004", void 0, !1, Z) where Z = response.data.error.reason. All five literals (participant_token_not_yet_valid, participant_token_expired, participant_token_revoked, participant_token_invalid, meeting_inactive) exist verbatim in the SDK, so the mapping in init-error.ts:24-37 is reachable and spelled correctly. The lockfile bump is consistent with packages/core/package.json:66.

No medium/high issues found. One low-priority note below; nothing posted inline.

Review: 1 finding.

Suggestions

  • packages/core/src/utils/init-error.ts:26-34 — the || t('init.auth_error') fallback cannot fire with the bundled i18n helper. useLanguage returns locale[key] ?? key (packages/core/src/lib/lang/index.ts:26) and always merges partial overrides onto defaultLanguage (line 22), so a missing key yields the key string — which is truthy — not ''. For consumers using useLanguage the new keys always resolve to the English defaults, and for a consumer-supplied t that mirrors the same key-as-fallback convention, the user would see init.token_expired rendered rather than the generic auth message. The guard only helps a custom t that returns a falsy value for unknown keys. If the intent (per the review request) is real protection, compare against the key, e.g. const m = t('init.token_expired'); return { message: !m || m === 'init.token_expired' ? t('init.auth_error') : m, code }; — otherwise the five || branches are dead weight.

github run

@ravindra-cloudflare
ravindra-cloudflare marked this pull request as ready for review November 5, 2026 09:54

@clanky-the-clanker clanky-the-clanker Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 Approved with comments — The reason-based auth-token messages are correct, and each one falls back to the generic message when its translation is empty. The only note is an INFO about missing unit tests for the new mapping.

Findings: 1 info (1 is an inline comment on the diff)

The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.


📋 Code Quality: 💬 1 info

The mapping is correct; the only gap is that it has no tests.

Recommendation: 💬 Optional Nits

  • [INFO] No unit tests for the new 0004 reason-to-message mapping in getInitErrorInfo. (packages/core/src/utils/init-error.ts:24)
View full review

getInitErrorInfo now has a nested switch on err.reason for code 0004. Each specific message falls back to init.auth_error when its translation is empty, and unknown reasons go to the generic message. The new mapping has no tests.


✅ All other checks passed

🚀 Release: ✅ LGTM

This pull request only bumps the @cloudflare/realtimekit dependency, and the lockfile matches the new range. Risk: low.


🔒 Security: ✅ LGTM

No security concerns.


📖 AGENTS.md: ✅ LGTM

No issues.


⚡ Performance: ✅ LGTM

No performance concerns.


📝 Documentation: ✅ LGTM

The doc comment for code 0004 now says that err.reason picks the message.


Reviewed commit: cd81a4a6

switch (code) {
case '0004':
return { message: t('init.auth_error'), code };
switch ((err as any)?.reason) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ℹ️ INFO · Code Quality

No unit tests for the new 0004 reason-to-message mapping in getInitErrorInfo.

No tests cover the new reason mapping. Consider a small spec that checks: each of the five reason values returns its own message with code: '0004'; an unknown or missing reason returns init.auth_error; and a t that returns '' for a specific key falls back to init.auth_error. The reason strings mirror the SDK's, so a test would also catch a rename on the SDK side.

@thisisamank
thisisamank merged commit 1dffe2a into staging Nov 5, 2026
7 checks passed
@thisisamank
thisisamank deleted the authtoken-error-message-ui branch November 5, 2026 10:13
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 2.1.0-staging.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants