feat: update name references on oauth login#37954
Conversation
🦋 Changeset detectedLatest commit: 04e46d6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces a MongoDB transaction-based flow in the OAuth login path that updates and persists user identity changes within a session, defers change notifications until after commit, and adds a workspace-level username synchronization when an existing user logs in with a changed name. Changes
Sequence Diagram(s)sequenceDiagram
participant OAuth as OAuth/Login Handler
participant Session as MongoDB Session
participant Updater as Updater Builder
participant Identity as saveUserIdentity
participant DB as Database (updateFromUpdater)
participant Notifier as Change Notifier (post-commit)
rect rgba(0,128,96,0.06)
OAuth->>Session: startSession() + attach successCallbacks[]
OAuth->>Updater: build updater (set emails, serviceId, preserve username)
OAuth->>Identity: saveUserIdentity(session, updater)
Updater->>DB: updateFromUpdater(session, updater)
DB->>Session: commitTransaction()
end
rect rgba(64,96,192,0.06)
Session-->>Notifier: onCommit -> run successCallbacks (notify listeners of user change)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
pierre-lehnen-rc
added
the
stat: QA assured
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/custom-oauth/server/custom_oauth_server.js">
<violation number="1" location="apps/meteor/app/custom-oauth/server/custom_oauth_server.js:394">
P1: Typo in condition: `serviceData.emails` should be `serviceData.email`. The `serviceData` object has an `email` property (singular), not `emails`. This condition will always be falsy, causing user emails to never be updated during OAuth login.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (1)
419-419: Consider adding error logging for callback failures.The use of
Promise.allSettledis correct—it ensures all notification callbacks run even if some fail. However, failed callbacks will fail silently, which could make debugging difficult if workspace-wide updates don't propagate as expected.🔎 Optional: Add error logging for failed callbacks
-void Promise.allSettled(successCallbacks.map((cb) => cb())); +void Promise.allSettled(successCallbacks.map((cb) => cb())).then((results) => { + results.forEach((result, index) => { + if (result.status === 'rejected') { + logger.error(`Success callback ${index} failed:`, result.reason); + } + }); +});
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/gold-trainers-shake.mdapps/meteor/app/custom-oauth/server/custom_oauth_server.js
🧰 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/custom-oauth/server/custom_oauth_server.js
🧠 Learnings (1)
📚 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:
.changeset/gold-trainers-shake.md
🧬 Code graph analysis (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (2)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnUserChange(377-389)apps/meteor/app/lib/server/functions/saveUserIdentity.ts (1)
saveUserIdentity(24-103)
🪛 Biome (2.1.2)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
[error] 14-15: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 15-16: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
⏰ 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). (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
.changeset/gold-trainers-shake.md (1)
1-5: LGTM!The changeset is well-formed and clearly describes the feature addition. The minor version bump is appropriate for this functionality enhancement.
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (5)
14-14: LGTM!The new imports for
clientandsaveUserIdentityare necessary for the transactional workflow and follow the existing import patterns in the file.Note: The static analysis hints about "import declarations outside of a module" are false positives—this file uses ES6 modules throughout.
Also applies to: 16-16
371-379: LGTM!The success callback pattern correctly fetches the committed state and notifies listeners. This ensures that workspace-wide updates (like the Direct Messages sidebar) receive the accurate, persisted user data.
390-410: LGTM!The transaction execution correctly orchestrates the identity update:
- Builds an updater with conditional email and service ID updates
- Calls
saveUserIdentityto handle name changes within the transaction- Applies updater changes via
updateFromUpdaterThe comment on line 406 clearly explains why the username parameter is necessary—it preserves the existing username field during name-only updates.
411-417: LGTM!The transaction cleanup is properly implemented with:
- Commit on success
- Abort and re-throw on error
- Guaranteed session cleanup in the finally block
This ensures no resource leaks and proper transaction semantics.
381-388: No action needed—the code correctly implements the ExtendedSession interface.The session extension with
onceSuccesfulCommitmatches theExtendedSessiontype defined inapps/meteor/server/database/utils.ts(lines 49–51) and is used consistently throughout the codebase, including in the type guard and utility functions. The spelling is intentional and part of the established interface contract.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37954 +/- ##
===========================================
+ Coverage 70.63% 70.65% +0.01%
===========================================
Files 3143 3143
Lines 108693 108693
Branches 19577 19542 -35
===========================================
+ Hits 76779 76794 +15
+ Misses 29904 29894 -10
+ Partials 2010 2005 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 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/16 01:28", "11/17 23:50", "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 12:31", "12/26 12:44 (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.11]
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.11]
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.12]
line "omnichannel-transcript-service" [0.14, 0.14, 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]
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.11]
line "queue-worker-service" [0.14, 0.14, 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]
line "rocketchat" [0.36, 0.35, 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.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 26 Dec 2025 12:44:55 GMT |
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
|
Looks like this PR is ready to merge! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (2)
381-388: Fix typo:onceSuccesfulCommit→onceSuccessfulCommit.The method name has a spelling error (missing second 's' in "Successful").
🔎 Proposed fix
const session = client.startSession(); try { // Extend the session to match the ExtendedSession type expected by saveUserIdentity Object.assign(session, { - onceSuccesfulCommit: (cb) => { + onceSuccessfulCommit: (cb) => { successCallbacks.push(cb); }, });
419-419: Consider logging errors from success callbacks.
Promise.allSettledsilently swallows rejections. Adding error logging would improve observability ifnotifyOnUserChangeor other callbacks fail.🔎 Proposed enhancement
- void Promise.allSettled(successCallbacks.map((cb) => cb())); + void Promise.allSettled(successCallbacks.map((cb) => cb())).then((results) => { + results.forEach((result, index) => { + if (result.status === 'rejected') { + logger.error('Success callback failed:', result.reason); + } + }); + });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
🧰 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/custom-oauth/server/custom_oauth_server.js
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
🧬 Code graph analysis (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (2)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnUserChange(377-389)apps/meteor/app/lib/server/functions/saveUserIdentity.ts (1)
saveUserIdentity(24-103)
🪛 Biome (2.1.2)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
[error] 14-15: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 15-16: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (4)
14-17: LGTM!The new imports are correctly used in the transactional user update flow.
371-379: LGTM!The success callback correctly defers user change notification until after transaction commit and appropriately constructs the diff from the updated fields.
390-409: LGTM!The transactional update flow is correctly implemented with proper use of the updater pattern and session-scoped operations. The email condition has been fixed from the previous review.
411-417: LGTM!Transaction commit/abort logic correctly implemented with proper session cleanup in the finally block.
dionisio-bot
bot
added
the
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
SUP-952
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.