chore: replace ids with objects as parameters #38367
Merged
Conversation
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
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
WalkthroughReplaces many userId string parameters with full user objects across server APIs and methods; rewrites twoFactorRequired as a const higher-order wrapper with AuthenticatedContext and runtime two-factor extraction/validation; updates related call sites, type signatures, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Wrapper as twoFactorRequired wrapper
participant TwoFactorSvc as checkCodeForUser
participant Target as original method
Client->>Wrapper: call method(args..., maybeTwoFactor)
Wrapper->>Wrapper: pop last arg, inspect twoFactorCode/method
alt twoFactor provided and validated
Wrapper->>Wrapper: set twoFactorChecked, re-push args
else not provided or not validated
Wrapper->>TwoFactorSvc: checkCodeForUser(user, options)
TwoFactorSvc-->>Wrapper: validated or throw
Wrapper->>Wrapper: set twoFactorChecked, re-push args
end
Wrapper->>Target: fn.apply(this, args)
Target-->>Client: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38367 +/- ##
===========================================
- Coverage 70.80% 70.76% -0.04%
===========================================
Files 3159 3158 -1
Lines 109364 109394 +30
Branches 19680 19649 -31
===========================================
- Hits 77432 77416 -16
- Misses 29897 29948 +51
+ Partials 2035 2030 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/28 12:46 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 28 Jan 2026 12:46:13 GMT |
…rectly from Users model for improved accuracy
…cept user object instead of userId for improved clarity
ggazzo
force-pushed
the
chore/stop-using-ids-for-functions
branch
from
34c7bda to
c0f1958
Compare
ggazzo
changed the title
ggazzo
force-pushed
the
chore/stop-using-ids-for-functions
branch
from
c0f1958 to
671e348
Compare
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/apps/communication/rest.ts (1)
913-918: GuardconvertToAppin the delete route to avoid a runtime throw.Other routes use optional chaining when resolving the users converter, but this path will throw if the converter is unavailable.
🛠️ Suggested fix
- const user = orchestrator?.getConverters()?.get('users').convertToApp(this.user); + const user = orchestrator?.getConverters()?.get('users')?.convertToApp(this.user);
🤖 Fix all issues with AI agents
In `@apps/meteor/app/lib/server/functions/saveUser/saveUser.ts`:
- Around line 127-129: The call to setStatusText currently unsafely casts
userData to IUser; instead pass the full existing user document oldUserData
(which contains required fields like createdAt, roles, type, active, _updatedAt)
as the user argument and keep userData.statusText as the new status string;
update the call from setStatusText(userData as IUser, userData.statusText, {
updater, session }) to setStatusText(oldUserData, userData.statusText, {
updater, session }) so setStatusText receives a complete IUser for presence
broadcast while using the incoming SaveUserData.statusText as the new value.
In `@apps/meteor/app/message-pin/server/pinMessage.ts`:
- Line 112: In pinMessage, the Apps.self?.triggerEvent call passes an undefined
variable user; replace that argument with the existing me object (the
authenticated user retrieved earlier in pinMessage) so the event uses the
correct user object (update the AppEvents.IPostMessagePinned invocation to pass
me instead of user and scan pinMessage for any other references to user that
should be me).
- Line 192: The unpinMessage function calls
Apps.self?.triggerEvent(AppEvents.IPostMessagePinned, originalMessage, user,
originalMessage.pinned) but the variable is named me in this scope; update the
call in unpinMessage to pass the correct user object (me) instead of undefined
user—i.e., replace user with me (or assign const user = me earlier) so
Apps.self?.triggerEvent and any related code use the defined user variable.
In `@apps/meteor/app/slashcommands-status/server/status.ts`:
- Around line 17-24: The Users.findOneById call uses a too-narrow projection
(only language) but setUserStatusMethod ultimately calls setStatusText which
expects user.statusText, user.username, user.status, user.name, and user.roles;
update the projection used in Users.findOneById to include these fields so the
user object passed to setUserStatusMethod/setStatusText has statusText,
username, status, name, roles (in addition to language/_id).
🧹 Nitpick comments (1)
apps/meteor/server/methods/messageSearch.ts (1)
72-74: Consider usinguserIddirectly instead ofuser?._id.Since
userIdis already available as a parameter and should equaluser._idwhen the user exists, this could be simplified. UsinguserIddirectly would also maintain search functionality if the user document is somehow missing but the userId is valid.♻️ Suggested simplification
} else { query.rid = { - $in: user?._id ? (await Subscriptions.findByUserId(user._id).toArray()).map((subscription: ISubscription) => subscription.rid) : [], + $in: (await Subscriptions.findByUserId(userId).toArray()).map((subscription: ISubscription) => subscription.rid), }; }
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
… object instead of userId for consistency
…d of userId for consistency across API endpoints
…ods to accept user object instead of userId for consistency and improved clarity
… user object instead of userId for consistency and improved clarity
…e valid user before proceeding
…to include token in context for improved security and consistency
…e type safety and ensure consistent handling of user context
…ature checks and using user context directly
…dcoded user ID for improved accuracy
ggazzo
force-pushed
the
chore/stop-using-ids-for-functions
branch
from
671e348 to
d531c0e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors several server methods and REST endpoints to pass full objects (e.g., IUser) instead of IDs, improving type safety and reducing repeated lookups.
Changes:
- Updated multiple method signatures to accept
IUserobjects (threads follow/unfollow, status, erase-room/team flows). - Simplified token invalidation by removing
removeOtherTokenshelper and callingUsers.removeNonLoginTokensExceptdirectly. - Tightened typings in 2FA wrappers and personal access token methods; updated affected tests.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/meteor/server/methods/saveUserProfile.ts | Switches to object-based status updates and updates token invalidation logic. |
| apps/meteor/server/methods/messageSearch.ts | Uses model lookup for user context instead of Meteor.userAsync(). |
| apps/meteor/server/lib/removeOtherTokens.ts | Removes helper in favor of direct model call. |
| apps/meteor/server/lib/eraseRoom.ts | Changes API to accept IUser and removes internal Meteor.userAsync() lookup. |
| apps/meteor/imports/personal-access-tokens/server/api/methods/removeToken.ts | Adds explicit param typing / minor refactor. |
| apps/meteor/imports/personal-access-tokens/server/api/methods/regenerateToken.ts | Adds explicit param typing. |
| apps/meteor/imports/personal-access-tokens/server/api/methods/generateToken.ts | Formats/clarifies function signature and method params typing. |
| apps/meteor/ee/server/apps/communication/rest.ts | Uses this.user for app management actions instead of Meteor.userAsync(). |
| apps/meteor/ee/server/api/roles.ts | Uses request context this.userId instead of Meteor.userId(). |
| apps/meteor/app/user-status/server/methods/setUserStatus.ts | Refactors status method to accept IUser and routes through setStatusText(user, ...). |
| apps/meteor/app/threads/server/methods/unfollowMessage.ts | Refactors to accept IUser and reuses it for app event trigger. |
| apps/meteor/app/threads/server/methods/followMessage.ts | Refactors to accept IUser and reuses it for app event trigger. |
| apps/meteor/app/slashcommands-status/server/status.ts | Updates slash command to call setUserStatusMethod(user, ...). |
| apps/meteor/app/message-pin/server/pinMessage.ts | Uses already-fetched me for app event trigger instead of Meteor.userAsync(). |
| apps/meteor/app/lib/server/methods/saveSetting.ts | Tightens editor parameter typing to SettingEditor. |
| apps/meteor/app/lib/server/functions/setStatusText.ts | Refactors to accept a user object instead of fetching by ID. |
| apps/meteor/app/lib/server/functions/saveUser/saveUser.ts | Updates statusText path to call setStatusText(user, ...). |
| apps/meteor/app/api/server/v1/users.ts | Updates REST endpoints to match new signatures and direct token invalidation. |
| apps/meteor/app/api/server/v1/teams.ts | Updates erase-room calls to pass this.user. |
| apps/meteor/app/api/server/v1/rooms.ts | Updates erase-room calls to pass this.user. |
| apps/meteor/app/api/server/v1/im.ts | Updates erase-room calls to pass this.user. |
| apps/meteor/app/api/server/v1/groups.ts | Updates erase-room calls to pass this.user. |
| apps/meteor/app/api/server/v1/chat.ts | Updates follow/unfollow calls to pass this.user. |
| apps/meteor/app/api/server/v1/channels.ts | Updates erase-room calls to pass this.user. |
| apps/meteor/app/api/server/lib/eraseTeam.ts | Updates erase team helpers to pass user objects through to eraseRoom. |
| apps/meteor/app/api/server/lib/eraseTeam.spec.ts | Updates test expectations for new eraseRoom signature. |
| apps/meteor/app/api/server/helpers/getUserFromParams.ts | Simplifies params type annotation / inferred return type. |
| apps/meteor/app/2fa/server/twoFactorRequired.ts | Refactors wrapper typing for broader this compatibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ?.getConverters() | ||
| ?.get('users') | ||
| .convertToApp(await Meteor.userAsync()); | ||
| const user = orchestrator?.getConverters()?.get('users').convertToApp(this.user); |
There was a problem hiding this comment.
This line drops the optional chaining on get('users'). If the converter is missing, orchestrator?.getConverters()?.get('users') can be undefined, and calling .convertToApp(...) will throw.
Keep the ?. before .convertToApp here for consistency with the other occurrences in this file (e.g., around line 877).
Suggested change
| const user = orchestrator?.getConverters()?.get('users').convertToApp(this.user); | |
| const user = orchestrator?.getConverters()?.get('users')?.convertToApp(this.user); |
…s for improved type safety
… text updates, enhancing clarity and consistency
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/helpers/getUserFromParams.ts`:
- Around line 6-7: The function getUserFromParams is missing an explicit return
type and uses an untyped let user;—add a proper return type annotation (e.g.,
the same Promise<User | null> or the exact union used by getUserListFromParams)
to the function signature and type the internal user variable accordingly so
callers can statically access ._id and .username; mirror the return type/pattern
from getUserListFromParams and update getUserFromParams's signature and user
declaration to match.
🧹 Nitpick comments (1)
apps/meteor/app/2fa/server/twoFactorRequired.ts (1)
12-12: Consider adding explicitthistype annotation.The inner async function has an implicit
this: anywhich relies on the final type assertion (line 40) for correctness. While this works, adding an explicit type annotation would make the intent clearer and catch potential issues earlier.♻️ Optional improvement
- async function (this, ...args) { + async function (this: Meteor.MethodThisType & { twoFactorChecked?: boolean }, ...args) {
iihimanshuu
reviewed
Jan 27, 2026
…ns by using AtLeast type for user parameter
KevLehman
reviewed
Jan 27, 2026
sampaiodiego
requested changes
Jan 27, 2026
…methods by using AuthenticatedContext
ggazzo
force-pushed
the
chore/stop-using-ids-for-functions
branch
from
e1eadc0 to
79b20d2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/app/2fa/server/twoFactorRequired.ts`:
- Line 6: The AuthenticatedContext type declares connection as required but the
wrapper and checkCodeForUser treat it as optional; change the
AuthenticatedContext definition (AuthenticatedContext) to make connection
optional (connection?: Meteor.Connection) so it matches checkCodeForUser's
signature and removes the need for defensive this.connection || undefined usage
in the wrapper functions that call checkCodeForUser.
- Around line 8-15: The exported twoFactorRequired wrapper incorrectly exposes a
non-Promise return type because the wrapper is async; update its generic
signature so the returned function type reflects a Promise by either
constraining TFunction to async functions (e.g. TFunction extends (...args:any)
=> Promise<any>) or change the asserted return type to
Promise<Awaited<ReturnType<TFunction>>>; adjust the cast on the returned async
function accordingly and keep the existing type checks involving
ThisParameterType<TFunction>, ReturnType<TFunction>, and ITwoFactorOptions in
sync with the new Promise-wrapped return type.
sampaiodiego
approved these changes
Jan 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/2fa/server/twoFactorRequired.ts (1)
23-43: Don’t drop falsy last arguments when probing for two‑factor options.
args.pop()+if (twoFactor)loses valid falsy values (e.g.,false,0,'',null) and changes the wrapped function’s behavior. Preserve any non‑two‑factor value.🛠️ Suggested fix
- const twoFactor = args.pop(); - if (twoFactor) { - if (twoFactor.twoFactorCode && twoFactor.twoFactorMethod) { + const twoFactor = args.pop(); + if (twoFactor && twoFactor.twoFactorCode && twoFactor.twoFactorMethod) { await checkCodeForUser({ user: this.userId, connection: this.connection || undefined, code: twoFactor.twoFactorCode, method: twoFactor.twoFactorMethod, options, }); this.twoFactorChecked = true; - } else { - // if it was not two factor options, put it back - args.push(twoFactor); - } + } else if (twoFactor !== undefined) { + args.push(twoFactor); }
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
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.
extracted from #38354
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.