fix: Variable shadowing preventing new visitor registration in livechat messages API #38040
Conversation
🦋 Changeset detectedLatest commit: 481a23e 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 |
|
Looks like this PR is ready to merge! 🎉 |
WalkthroughChanged a shadowed Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🔇 Additional comments (2)
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 |
![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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/livechat/server/api/v1/message.ts (1)
281-284: Consider simplifying redundant visitor check.The check at lines 281-284 appears redundant since:
- If
visitorexists at line 258, it's used directly- If not, it's registered at line 275 and already validated at lines 276-278
That said, this defensive check is harmless and may provide value if the registration logic changes in the future.
Optional simplification
-const guest = visitor; -if (!guest) { - throw new Error('error-invalid-token'); -} +const guest = visitor; // visitor is guaranteed to exist at this point
📜 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/livechat/server/api/v1/message.ts
🧰 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/livechat/server/api/v1/message.ts
🧠 Learnings (1)
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/livechat/server/api/v1/message.ts
🧬 Code graph analysis (1)
apps/meteor/app/livechat/server/api/v1/message.ts (2)
packages/ddp-client/src/livechat/LivechatClientImpl.ts (1)
visitor(151-159)packages/models/src/index.ts (1)
LivechatVisitors(170-170)
⏰ 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: CodeQL analysis (javascript-typescript)
🔇 Additional comments (2)
apps/meteor/app/livechat/server/api/v1/message.ts (2)
256-256: Variable shadowing fix: Correctly enables visitor reassignment.The change from
consttoletis necessary and correct. This allows the newly registered guest to be assigned to the samevisitorvariable in the else block (line 275), fixing the bug where new visitors couldn't register successfully.
275-275: Correct assignment to outer scope variable.The assignment to the outer
visitorvariable (without redeclaring) properly fixes the shadowing bug. Combined with theletdeclaration at line 256, this ensures newly registered guests flow through the same code path as existing visitors.
|
@Dnouv can you please review this |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38040 +/- ##
===========================================
- Coverage 70.65% 70.63% -0.02%
===========================================
Files 3148 3148
Lines 108850 108850
Branches 19543 19606 +63
===========================================
- Hits 76906 76890 -16
- Misses 29933 29955 +22
+ Partials 2011 2005 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KevLehman
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
2 participants
Proposed changes (including videos or screenshots)
This PR fixes a critical variable shadowing bug in the
POST /api/v1/livechat/messagesendpoint that prevented new visitors (those without an existing token) from sending messages.The Bug:
In
apps/meteor/app/livechat/server/api/v1/message.ts, the logic for handling new visitors was inside anelseblock where a newconst visitorwas declared. This declaration shadowed the outervisitorvariable. As a result, when the code exits theelseblock, the outervisitorvariable remainedundefined, causing the subsequent checkif (!guest)to fail and throw anerror-invalid-token.The Fix:
const visitortolet visitor.constdeclaration inside theelseblock to ensure the newly registered visitor is assigned to the outer variable.Issue(s)
Closes #38039
Steps to test or reproduce
/api/v1/livechat/messageswith a brand new visitor token (one that does not exist in the database yet).Before Fix:
Returns
{"success": false, "error": "error-invalid-token"}After Fix:
Returns
{"success": true, "messages": [...]}Further comments
This is a straightforward scope fix. No logical changes were made to the registration process itself, just ensuring the variable is correctly accessible in the outer scope.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.