fix: livechat offline form validation #38621
Conversation
|
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 |
WalkthroughPrevents Livechat offline form submissions when no recipient email exists by exposing the global offline email and department emails to frontend visibility logic and adding a backend guard that rejects offline messages when no destination email can be resolved. Changes
Sequence Diagram(s)sequenceDiagram
participant Visitor
participant Frontend
participant SettingsService
participant Server
participant DB
Visitor->>Frontend: open chat widget
Frontend->>SettingsService: request displayOfflineForm
SettingsService->>DB: fetch Livechat_offline_email
SettingsService->>DB: fetch departments (include email)
DB-->>SettingsService: return settings + departments
SettingsService-->>Frontend: displayOfflineForm (true/false)
alt displayOfflineForm == false
Frontend-->>Visitor: hide offline form / show unavailable
else displayOfflineForm == true
Frontend-->>Visitor: show offline form
Visitor->>Frontend: submit offline message
Frontend->>Server: POST /sendOfflineMessage (payload, dept?)
Server->>DB: resolve department email / global email
alt email found
Server-->>Frontend: 200 OK
Frontend-->>Visitor: confirmation
else no email found
Server-->>Frontend: 4xx error (error-no-email-configured-to-receive-offline-messages)
Frontend-->>Visitor: show error / cannot send
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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. No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/app/livechat/server/api/lib/livechat.ts`:
- Around line 125-127: The computed property displayOfflineForm can leak the raw
initSettings.Livechat_offline_email string into the public /livechat/config
response due to JS short-circuiting; update the expression that computes
displayOfflineForm to coerce the result of initSettings.Livechat_offline_email
(and the departments.some(...) check if needed) to a boolean (e.g., use !! or
Boolean()) so displayOfflineForm becomes true/false rather than a string,
keeping references to displayOfflineForm,
initSettings.Livechat_display_offline_form, initSettings.Livechat_offline_email,
departments, and the departments.some(d => d.showOnOfflineForm && d.email)
predicate to locate and change the code.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/livechat-offline-form-validation.mdapps/meteor/app/livechat/server/api/lib/livechat.tsapps/meteor/app/livechat/server/lib/messages.tsapps/meteor/app/livechat/server/lib/settings.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/lib/settings.tsapps/meteor/app/livechat/server/lib/messages.tsapps/meteor/app/livechat/server/api/lib/livechat.ts
🧠 Learnings (5)
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/app/livechat/server/lib/settings.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/livechat/server/lib/settings.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/app/livechat/server/lib/settings.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/livechat/server/lib/messages.ts
📚 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/lib/messages.ts
⏰ 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 comments (5)
apps/meteor/app/livechat/server/lib/settings.ts (1)
48-48: LGTM!Adding
Livechat_offline_emailto the init settings bundle is necessary for the frontend visibility logic inlivechat.tsto work correctly..changeset/livechat-offline-form-validation.md (1)
1-5: LGTM!Changeset accurately describes the fix scope.
apps/meteor/app/livechat/server/lib/messages.ts (1)
76-78: LGTM!The guard is correctly positioned after both the global setting and department-level email resolution, ensuring no offline message can be sent without a recipient.
apps/meteor/app/livechat/server/api/lib/livechat.ts (2)
36-51: LGTM!Including
174-176: StripThe
/livechat/configendpoint is public (no authentication required) and returns the departments array with email addresses included in the projection. Since the
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![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 4 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/livechat/server/api/lib/livechat.ts">
<violation number="1" location="apps/meteor/app/livechat/server/api/lib/livechat.ts:127">
P1: This expression may leak the configured email address in the public API response. Due to JavaScript's short-circuit evaluation, when `Livechat_offline_email` is a non-empty string, the `||` operator returns that string directly instead of a boolean `true`. Wrap the entire expression with `!!` to coerce to boolean.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
🦋 Changeset detectedLatest commit: a1f7347 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/app/livechat/server/api/lib/livechat.ts`:
- Around line 48-57: The department email is being projected by
LivechatDepartment.findEnabledWithAgentsAndBusinessUnit (ILivechatDepartment
email field) and then returned verbatim in the public config response;
remove/leak by stripping the email property from the departments before
returning the public response (i.e., sanitize the "departments" array produced
by findEnabledWithAgentsAndBusinessUnit and any other place that returns
departments verbatim such as the public config return at/around the code that
returns "departments"), ensuring displayOfflineForm logic still runs but the
returned objects do not include the email field.
🧹 Nitpick comments (2)
apps/meteor/app/livechat/server/lib/messages.ts (1)
27-31: Remove JSDoc comments per coding guidelines.The coding guideline for
.tsfiles states: "Avoid code comments in the implementation." These JSDoc comments should be removed.Proposed fix
-/** - * Sends an offline message via email. - * `@param` data The offline message data including visitor info and message content. - * `@throws` {Error} If offline form is disabled or no recipient email is configured. - */ export async function sendOfflineMessage(data: OfflineMessageData) {As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation."apps/meteor/app/livechat/server/api/lib/livechat.ts (1)
33-38: Remove JSDoc comments per coding guidelines.Both the
findDepartments(lines 33–38) andsettings(lines 114–118) docblocks should be removed.As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation."Also applies to: 114-118
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/livechat/server/api/lib/livechat.tsapps/meteor/app/livechat/server/lib/messages.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/lib/messages.tsapps/meteor/app/livechat/server/api/lib/livechat.ts
🧠 Learnings (2)
📚 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/livechat/server/lib/messages.ts
📚 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/lib/messages.ts
🧬 Code graph analysis (1)
apps/meteor/app/livechat/server/api/lib/livechat.ts (1)
packages/models/src/index.ts (1)
LivechatDepartment(164-164)
⏰ 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 comments (2)
apps/meteor/app/livechat/server/lib/messages.ts (1)
81-83: LGTM — good backend guard for missing recipient email.This correctly prevents offline messages from being sent when no email is configured, complementing the frontend visibility change.
apps/meteor/app/livechat/server/api/lib/livechat.ts (1)
136-139: LGTM —displayOfflineFormis now correctly coerced to boolean.The
!!wrapper prevents leaking the raw email string into the public API response. This addresses the previously raised concern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
Proposed changes (including videos or screenshots)
settings.ts): Included theLivechat_offline_emailsetting in the initialization bundle so the system can check for a valid recipient.livechat.ts): Updated the API to hide the offline form if no destination email is setmessages.ts): Added a safety check to throw a clear error if an offline message is submitted without a recipient.Issue(s)
Fixes #38619
Steps to test or reproduce
settings icon>Omnichannel>Livechat Appearance>Omnichannel offlinesection (MUST be admin)Display offline form.http://localhost:3000/livechat) as a visitor when no agents are online.SendSummary by CodeRabbit
Bug Fixes
Chores