Repository navigation
feat: add error reason for auth token - #197
Conversation
| switch (code) { | ||
| case '0004': | ||
| return { message: t('init.auth_error'), code }; | ||
| switch ((err as any)?.reason) { |
There was a problem hiding this comment.
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.
|
Review: 1 finding. Submitted 1 inline comment. Needs Fix
Suggestions
|
|
I verified the SDK behavior that was previously in question, by unpacking the newly pinned dependency. Previous blocking concern is resolved. In 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: No medium/high issues found. One low-priority note below; nothing posted inline. Review: 1 finding. Suggestions
|
There was a problem hiding this comment.
💬 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
0004reason-to-message mapping ingetInitErrorInfo. (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) { |
There was a problem hiding this comment.
ℹ️ 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.
|
🎉 This PR is included in version 2.1.0-staging.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
This PR utilizes error messages reason for invalid auth tokens, webcore already has implementation of this: https://github.com/cloudflare/realtimekit/pull/2137