fix: use updater for username update to ensure transactional consistency

[gauravsingh001-cyber]

Description

Replaced the direct call to Users.setUsername with updater.updateOne to ensure transactional consistency during username updates.

Changes

• Used updater.updateOne when available instead of direct database method
• Added fallback to Users.updateOne when updater is not provided
• Updated local user object after database update

Why this change?

The previous implementation bypassed the updater, which could lead to inconsistent behavior in transactional flows. Using the updater ensures that the username update is executed within the same transactional context.

Notes

This resolves the TODO comment regarding the use of updater and ensures safer database updates.

Summary by CodeRabbit

• Refactoring
  • Improved internal handling of username updates for enhanced flexibility and code maintainability.

[dionisio-bot]

Looks like this PR is not ready to merge, because of the following issues:

• This PR is missing the 'stat: QA assured' label
• This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

[changeset-bot]

⚠️ No Changeset found

Latest commit: d76d75a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 71fa8d2a-1d67-41d0-9473-bc79e3cf3cad

📥 Commits

Reviewing files that changed from the base of the PR and between 4235cd9 and d76d75a.

📒 Files selected for processing (1)

• apps/meteor/app/lib/server/functions/setUsername.ts

📜 Recent 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). (1)

• GitHub Check: cubic · AI code reviewer

🧰 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/setUsername.ts

🧠 Learnings (8)

📓 Common learnings

Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.

Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.

📚 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/setUsername.ts

📚 Learning: 2026-03-20T13:51:23.302Z

Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.

Applied to files:

• apps/meteor/app/lib/server/functions/setUsername.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/setUsername.ts

📚 Learning: 2026-03-15T14:31:28.969Z

Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.

Applied to files:

• apps/meteor/app/lib/server/functions/setUsername.ts

📚 Learning: 2026-03-11T22:04:20.529Z

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:20.529Z
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/setUsername.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/setUsername.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/setUsername.ts

🔇 Additional comments (1)

apps/meteor/app/lib/server/functions/setUsername.ts (1)

133-145: updater.updateOne() may not exist on Updater<IUser> — verify the API.

Based on the BaseRaw.updateFromUpdater implementation in packages/models/src/models/BaseRaw.ts, the Updater class is designed to accumulate update operations via builder methods (like set(), inc()), and the model extracts the accumulated filter via updater.getUpdateFilter(). The updateOne method is on the model, not on Updater.

Additionally, even if this API exists, the updater path doesn't pass session options, which breaks the transactional consistency this PR aims to achieve.

Consider one of these approaches:

1. Use updater.set('username', username) to add the username update to the accumulated operations, letting the caller apply the full update.
2. Use Users.updateFromUpdater({ _id: user._id }, updater, { session }) if you need to apply the update immediately within the transaction.

[raise_critical_issue, request_verification]

#!/bin/bash
# Description: Verify if Updater class has updateOne method
# Expected: updateOne should NOT be a method on Updater; it's on BaseRaw/model classes

echo "=== Searching for Updater class definition ==="
ast-grep --pattern 'class Updater<$_> {
  $$$
}'

echo ""
echo "=== Searching for updateOne method in Updater ==="
rg -n 'updateOne' --type ts -g '**/Updater*'

echo ""
echo "=== Checking Updater interface/type exports ==="
rg -n -A10 'export (class|interface|type) Updater' --type ts

echo ""
echo "=== How is updater used elsewhere in the codebase ==="
rg -n -B2 -A5 'updater\.(set|updateOne|getUpdateFilter)' --type ts -g '!node_modules'

---

Walkthrough

Updated _setUsername function to conditionally persist the username using either a provided updater function or direct Users.updateOne call. Removed the prior Users.setUsername call while maintaining all subsequent side-effect logic including avatar handling and broadcasting.

Changes

Cohort / File(s)	Summary
Username persistence refactoring apps/meteor/app/lib/server/functions/setUsername.ts	Modified _setUsername to support conditional database persistence: added optional updater parameter that uses updater.updateOne() if provided, otherwise falls back to direct Users.updateOne() call. Removed deprecated Users.setUsername() invocation while preserving in-memory assignment and all side effects.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

type: bug

🚥 Pre-merge checks | ✅ 3

✅ 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 'fix: use updater for username update to ensure transactional consistency' directly and clearly describes the main technical change: replacing a direct database call with an updater pattern to maintain transactional consistency.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cubic-dev-ai]

No issues found across 1 file

[gauravsingh001-cyber]

Hi @KevLehman , could you please take a look at this PR when you have time? I’d really appreciate your review. Thanks!