refactor: Reduce as any usage in core functions#39573
Open
aniruddhaadak80 wants to merge 1 commit into
Open
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
Contributor
WalkthroughTwo server-side utility functions are refactored to improve TypeScript type safety. Error handling is enhanced with proper type checking instead of unsafe casts, and email template fields receive safe default values to prevent undefined references. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan for PR comments
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. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens TypeScript typing and runtime safety in server-side Meteor functions that send activation emails and add users to rooms.
Changes:
- Removes
as anycasts when building the activation/deactivation email payload and adds string fallbacks for template params. - Improves error typing in
addUserToRoomby treating caught errors asunknownand extracting a message safely.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/meteor/app/lib/server/functions/setUserActiveStatus.ts | Refactors email template invocation to be more type-safe and avoids passing undefined values into the email HTML template. |
| apps/meteor/app/lib/server/functions/addUserToRoom.ts | Adjusts error handling in the beforeAddUserToRoom callback block to use unknown and produce a string message reliably. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+155
to
156
| subject: subject({ active }), | ||
| html: html({ |
There was a problem hiding this comment.
Accounts.emailTemplates.userActivated.subject expects { active, username } (see authentication/server/startup/index.js). Here it’s called with only { active }, which will make the subject pick the "Approved/Deactivated" variant even when the HTML template (below) now correctly receives username and may pick "Activated". This can lead to mismatched subject/body and incorrect localization key selection. Pass username into subject(...) and adjust the local UserActivated.subject type accordingly (likely username?: string).
| name: user.name, | ||
| username: user.username, | ||
| } as any), | ||
| name: user.name || '', |
There was a problem hiding this comment.
name is what the email template actually renders (it doesn’t interpolate username), so defaulting to an empty string can produce emails with a blank name even when user.username is present. Consider falling back to user.username (or the same display value used in destinations) before falling back to ''.
Suggested change
| name: user.name || '', | |
| name: user.name || user.username || '', |
| } catch (error) { | ||
| throw new Meteor.Error((error as any)?.message); | ||
| } catch (error: unknown) { | ||
| throw new Meteor.Error(error instanceof Error ? error.message : String(error)); |
There was a problem hiding this comment.
This catch re-wraps any thrown error into a new Meteor.Error using only error.message. That discards important structured info when the callback throws a Meteor.Error or core-services MeteorError (e.g. federation hooks throw MeteorError('error-not-authorized-federation', ...)), so callers can no longer rely on error.error / reason / details for i18n and branching. Consider rethrowing Meteor.Error as-is, and if it’s a core-services MeteorError (has errorType === 'Meteor.Error'), map its error/reason/details into a Meteor.Error instead of stringifying the message.
Suggested change
| throw new Meteor.Error(error instanceof Error ? error.message : String(error)); | |
| // Preserve structured Meteor errors when thrown by callbacks/hooks | |
| if (error instanceof Meteor.Error) { | |
| throw error; | |
| } | |
| if (error && typeof error === 'object' && (error as any).errorType === 'Meteor.Error') { | |
| const { error: code, reason, details } = error as any; | |
| throw new Meteor.Error(code ?? reason ?? 'error', reason, details); | |
| } | |
| const message = error instanceof Error ? error.message : String(error); | |
| // Fallback: preserve previous behavior for unstructured errors | |
| throw new Meteor.Error(message); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/lib/server/functions/addUserToRoom.ts`:
- Around line 69-70: The catch in addUserToRoom.ts currently wraps every hook
failure into a plain Meteor.Error losing the original error code and details;
update the catch to detect if error is already a Meteor.Error (thrown by
beforeAddUserToRoom) and re-throw it unchanged, otherwise create a Meteor.Error
preserving error.error (code), message and details when available so callers can
branch on specific error codes (e.g., preserve 'error-not-authorized-federation'
from beforeAddUserToRoom).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6ad992e-aebb-4174-8350-0aff1fca9645
📒 Files selected for processing (2)
apps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.ts
📜 Review details
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.ts
📚 Learning: 2026-03-11T22:04:16.136Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:16.136Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.ts
🔇 Additional comments (1)
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts (1)
155-160: Good removal of the template-sideas anyusage.Passing a typed
{ active }object here and defaultingname/usernameto empty strings makes the template call safer without changing the surrounding flow.
Comment on lines
+69
to
+70
| } catch (error: unknown) { | ||
| throw new Meteor.Error(error instanceof Error ? error.message : String(error)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether the hooks called here already throw Meteor.Error codes/details.
fd 'beforeAddUserToRoom.*\.(ts|js)$' | while read -r file; do
echo "=== $file ==="
rg -n -C3 'throw new Meteor\.Error|new Meteor\.Error|throw ' "$file"
done
# Inspect the touched catch block in context.
rg -n -C4 'beforeAddUserToRoomPatch|beforeAddUserToRoom\.run|catch \(error: unknown\)|new Meteor\.Error' apps/meteor/app/lib/server/functions/addUserToRoom.tsRepository: RocketChat/Rocket.Chat
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Directly read the beforeAddUserToRoom hook file
echo "=== beforeAddUserToRoom.ts content ==="
cat -n apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts
echo ""
echo "=== addUserToRoom.ts catch block context ==="
sed -n '50,85p' apps/meteor/app/lib/server/functions/addUserToRoom.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 2019
🏁 Script executed:
#!/bin/bash
# Find implementations/patches of beforeAddUserToRoom in federation and EE modules
echo "=== Searching for beforeAddUserToRoom implementations/patches ==="
rg -l 'beforeAddUserToRoom' --type ts --type js | head -20
echo ""
echo "=== Check federation module hooks ==="
fd 'federation.*hook' -t f | head -10
echo ""
echo "=== Look for patching of beforeAddUserToRoom ==="
rg 'beforeAddUserToRoom.*throw|throw.*Meteor\.Error.*beforeAddUserToRoom' -B5 -A5Repository: RocketChat/Rocket.Chat
Length of output: 735
🏁 Script executed:
#!/bin/bash
# Check the main implementations/hooks for beforeAddUserToRoom
echo "=== apps/meteor/server/lib/callbacks/beforeAddUserToRoom.ts ==="
cat -n apps/meteor/server/lib/callbacks/beforeAddUserToRoom.ts
echo ""
echo "=== apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts ==="
cat -n apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts
echo ""
echo "=== apps/meteor/ee/server/hooks/federation/index.ts (lines around beforeAddUserToRoom) ==="
rg -n -B3 -A10 'beforeAddUserToRoom' apps/meteor/ee/server/hooks/federation/index.ts | head -50Repository: RocketChat/Rocket.Chat
Length of output: 3665
🏁 Script executed:
#!/bin/bash
# Continue reading the federation beforeAddUserToRoom hook
echo "=== apps/meteor/ee/server/hooks/federation/index.ts (lines 105-150) ==="
sed -n '105,150p' apps/meteor/ee/server/hooks/federation/index.ts | cat -n
echo ""
echo "=== Check if any beforeAddUserToRoom implementations throw MeteorError ==="
rg -n 'beforeAddUserToRoom' apps/meteor/ee/server/hooks/ -A20 | grep -E '(throw|Meteor\.Error|Error\()'Repository: RocketChat/Rocket.Chat
Length of output: 2208
🏁 Script executed:
#!/bin/bash
# Check if MeteorError is being used (verify the actual error type name)
echo "=== Check imports of Meteor.Error in federation/index.ts ==="
head -20 apps/meteor/ee/server/hooks/federation/index.ts
echo ""
echo "=== Verify the actual error thrown (check if it's MeteorError vs Meteor.Error) ==="
rg 'MeteorError|Meteor\.Error' apps/meteor/ee/server/hooks/federation/index.ts | head -5
echo ""
echo "=== Check type definition of Meteor.Error ==="
rg -A5 'class Meteor\.Error|type MeteorError|export.*Error' packages/core-typings --type ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 2057
🏁 Script executed:
#!/bin/bash
# Check what MeteorError is
echo "=== Find MeteorError definition in core-services ==="
rg -n 'export.*MeteorError|class MeteorError|type MeteorError' --type ts | head -20
echo ""
echo "=== Check if MeteorError is Meteor.Error or a different type ==="
rg -B2 -A8 'class MeteorError|export.*MeteorError' packages/core-services --type ts | head -40Repository: RocketChat/Rocket.Chat
Length of output: 2522
🏁 Script executed:
#!/bin/bash
# Check what Meteor.Error is in addUserToRoom.ts
echo "=== Check imports in addUserToRoom.ts ==="
head -30 apps/meteor/app/lib/server/functions/addUserToRoom.ts
echo ""
echo "=== Check what Meteor is (global or imported) ==="
rg -n '^import.*Meteor|^import.*from.*meteor' apps/meteor/app/lib/server/functions/addUserToRoom.ts
echo ""
echo "=== Full MeteorError definition ==="
cat -n packages/core-services/src/MeteorError.tsRepository: RocketChat/Rocket.Chat
Length of output: 2351
🏁 Script executed:
#!/bin/bash
# Check if Meteor.Error and MeteorError are related/compatible
echo "=== Check Meteor.Error constructor ==="
rg -n 'class.*Error.*{|constructor.*error.*reason' packages/meteor --type ts | head -10
echo ""
echo "=== Check how federation hooks are used in server methods ==="
rg -B5 -A10 'beforeAddUserToRoom\.run|beforeAddUserToRoomPatch' apps/meteor/app/lib/server/functions/addUserToRoom.ts
echo ""
echo "=== Check if MeteorError needs special handling in methods ==="
rg -n 'isMeteorError|instanceof MeteorError|MeteorError' apps/meteor/app/lib/server/functions/ --type ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 4977
🏁 Script executed:
#!/bin/bash
# Check what type Meteor.Error is and how it's used
echo "=== Search for Meteor.Error usage in the codebase ==="
rg 'new Meteor\.Error|throw.*Meteor\.Error' apps/meteor/app/lib/server/functions/ --type ts -A1 | head -30
echo ""
echo "=== Check if there's a wrapper or compatibility between MeteorError and Meteor.Error ==="
rg -n 'MeteorError|Meteor\.Error' apps/meteor/server/lib/callbacks/beforeAddUserToRoom.ts
echo ""
echo "=== Look at how errors are handled in similar catch blocks ==="
rg -B2 -A4 'catch.*error.*unknown' apps/meteor/app/lib/server/functions/ --type ts | head -40Repository: RocketChat/Rocket.Chat
Length of output: 3041
🏁 Script executed:
#!/bin/bash
# Check how MeteorError is used in other parts of the codebase when propagating errors
echo "=== Check if MeteorError is caught/handled anywhere ==="
rg -n 'catch.*MeteorError|isMeteorError' apps/meteor/app/lib/server/functions/ --type ts
echo ""
echo "=== Check if MeteorError and Meteor.Error are compatible/convertible ==="
rg -B3 -A3 'instanceof.*MeteorError|instanceof.*Meteor\.Error' apps/meteor --type ts | head -50
echo ""
echo "=== Verify the exact inheritance/compatibility ==="
node <<'EOF'
// Check if Meteor.Error is being used consistently
const fs = require('fs');
const path = require('path');
// Show MeteorError structure again
const meteorErrorPath = 'packages/core-services/src/MeteorError.ts';
const content = fs.readFileSync(meteorErrorPath, 'utf8');
console.log('MeteorError class:');
console.log(content.split('\n').slice(0, 13).join('\n'));
EOFRepository: RocketChat/Rocket.Chat
Length of output: 4202
🏁 Script executed:
#!/bin/bash
# Verify what type of error is actually thrown by the hooks
echo "=== Inspect hook signatures and what they throw ==="
cat -n apps/meteor/app/lib/server/lib/beforeAddUserToRoom.ts
echo ""
echo "=== Check ABAC hook error type ==="
sed -n '16,22p' apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts | cat -n
echo ""
echo "=== Check federation hook error type ==="
sed -n '105,120p' apps/meteor/ee/server/hooks/federation/index.ts | cat -n
echo ""
echo "=== Check what happens when errors propagate through hooks ==="
rg -n 'class.*Callbacks|\.run\(' apps/meteor/server/lib/callbacks/callbacksBase.ts -A8 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 2175
🏁 Script executed:
#!/bin/bash
# Verify error types more carefully
echo "=== Check what type of errors are thrown in federation vs ABAC ==="
echo "Federation throws:"
sed -n '115,117p' apps/meteor/ee/server/hooks/federation/index.ts
echo ""
echo "ABAC throws:"
sed -n '16,22p' apps/meteor/ee/server/hooks/abac/beforeAddUserToRoom.ts
echo ""
echo "=== Verify if these are the same or different error types ==="
echo "MeteorError is from core-services and has: error, reason, details"
echo "Meteor.Error is from meteor/meteor package"
echo ""
echo "Check if they're compatible:"
rg -n 'Meteor.Error' packages/core-services --type ts
echo ""
echo "=== Check how other similar catch blocks handle this ==="
rg -B3 -A6 'beforeAddUserToRoom\.run' apps/meteor --type ts -A10 | grep -A10 'catch'Repository: RocketChat/Rocket.Chat
Length of output: 2839
🏁 Script executed:
#!/bin/bash
# Check how MeteorError and Meteor.Error relate
echo "=== Check if MeteorError can be thrown and caught as Meteor.Error ==="
rg -n 'throw new MeteorError|catch.*Meteor\.Error' apps/meteor/ee/server/hooks/federation/ --type ts
echo ""
echo "=== Check if Meteor.Error can be constructed with error code ==="
rg -B2 -A2 'new Meteor\.Error.*error-' apps/meteor/app/lib/server/functions/ --type ts | head -40
echo ""
echo "=== Final check: does the codebase convert MeteorError to Meteor.Error anywhere? ==="
rg -n 'MeteorError.*Meteor\.Error|new Meteor\.Error.*MeteorError' apps/meteor --type tsRepository: RocketChat/Rocket.Chat
Length of output: 3878
🏁 Script executed:
#!/bin/bash
# Verify the actual error types being used
echo "=== Verify MeteorError vs Meteor.Error relationship ==="
echo "MeteorError (from core-services):"
rg -A3 'export class MeteorError' packages/core-services/src/MeteorError.ts
echo ""
echo "Is MeteorError a Meteor.Error?"
echo "Looking for Meteor.Error class definition..."
rg -n 'class Meteor\.Error' apps/meteor --type ts
echo ""
echo "=== Check if addUserToRoom catches and should preserve MeteorError ==="
echo "The hooks throw:"
echo "- Federation: MeteorError('error-not-authorized-federation', ...)"
echo "- ABAC: Error('error-room-is-abac-managed')"
echo ""
echo "Current catch: new Meteor.Error(error.message) - LOSES error codes"
echo ""
echo "Verify: does MeteorError have a message property?"
sed -n '10,12p' packages/core-services/src/MeteorError.tsRepository: RocketChat/Rocket.Chat
Length of output: 734
Preserve error code metadata when re-throwing hook failures.
Line 70 turns every hook failure into a plain Meteor.Error(message) without preserving the error code. The beforeAddUserToRoom hook in federation (apps/meteor/ee/server/hooks/federation/index.ts:116) throws MeteorError('error-not-authorized-federation', ...) with a structured error code that is lost in the re-wrap, breaking callers that branch on specific error codes.
Proposed fix
} catch (error: unknown) {
- throw new Meteor.Error(error instanceof Error ? error.message : String(error));
+ if (error instanceof Meteor.Error) {
+ throw error;
+ }
+
+ throw new Meteor.Error('error-add-user-to-room', error instanceof Error ? error.message : String(error));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (error: unknown) { | |
| throw new Meteor.Error(error instanceof Error ? error.message : String(error)); | |
| } catch (error: unknown) { | |
| if (error instanceof Meteor.Error) { | |
| throw error; | |
| } | |
| throw new Meteor.Error('error-add-user-to-room', error instanceof Error ? error.message : String(error)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/lib/server/functions/addUserToRoom.ts` around lines 69 - 70,
The catch in addUserToRoom.ts currently wraps every hook failure into a plain
Meteor.Error losing the original error code and details; update the catch to
detect if error is already a Meteor.Error (thrown by beforeAddUserToRoom) and
re-throw it unchanged, otherwise create a Meteor.Error preserving error.error
(code), message and details when available so callers can branch on specific
error codes (e.g., preserve 'error-not-authorized-federation' from
beforeAddUserToRoom).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #39568
This PR reduces the usage of 'as any' type casting in the core logic within 'apps/meteor/app/lib'.
Specifically:
These changes help leverage TypeScript's safety checks and improve overall code reliability in the message and user handling pipelines.
Summary by CodeRabbit