ci: TEST_MODE='api' to suppress deprecation throws in API tests#40677
Merged
Conversation
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
WalkthroughBroadens TEST_MODE checks to also accept "api" and sets TEST_MODE='api' during CI container startup for API E2E. Changes centralize deprecation throw logic and propagate the 'api' test mode to caches, schedulers, rate-limiters, email/test-user gates, Twilio validation, and client query retry behavior. ChangesTEST_MODE/API Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
ggazzo
changed the title
coderabbitai
Bot
added
the
type: feature
Member
Author
|
/jira ARCH-2156 |
Contributor
|
The milestone "8.6.0" does not exist on the Jira board; the task was created without Fix version. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (1)
29-33: ⚡ Quick winRemove inline rationale comments from implementation.
Please move this rationale to PR/docs and keep this TS implementation comment-free.
As per coding guidelines "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts` around lines 29 - 33, Remove the inline explanatory comment block inside deprecationWarningLogger.ts (the multi-line rationale about TEST_MODE and TEST_MODE_API) from the implementation; leave only concise code comments if necessary but move the detailed rationale to the PR or external docs as requested, ensuring functions/classes like the deprecation warning logic (e.g., the code that checks TEST_MODE/TEST_MODE_API and logs/throws) remain unchanged and documented outside the source file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts`:
- Around line 29-33: Remove the inline explanatory comment block inside
deprecationWarningLogger.ts (the multi-line rationale about TEST_MODE and
TEST_MODE_API) from the implementation; leave only concise code comments if
necessary but move the detailed rationale to the PR or external docs as
requested, ensuring functions/classes like the deprecation warning logic (e.g.,
the code that checks TEST_MODE/TEST_MODE_API and logs/throws) remain unchanged
and documented outside the source file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9886d7f7-4f4e-4dbc-a2b7-c3c6d6e7a1b1
📒 Files selected for processing (3)
.github/workflows/ci-test-e2e.ymlapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsdocker-compose-ci.yml
📜 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). (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/lib/deprecationWarningLogger.ts
🧠 Learnings (3)
📚 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/lib/deprecationWarningLogger.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/lib/deprecationWarningLogger.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
🔇 Additional comments (3)
apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (1)
34-35: LGTM!Also applies to: 41-42, 69-70, 97-98, 124-125, 134-135, 159-160, 172-173
.github/workflows/ci-test-e2e.yml (1)
178-184: LGTM!Also applies to: 195-195, 222-225, 242-242
docker-compose-ci.yml (1)
17-17: LGTM!Also applies to: 167-167
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40677 +/- ##
===========================================
+ Coverage 69.64% 69.66% +0.01%
===========================================
Files 3338 3338
Lines 123246 123248 +2
Branches 21946 22027 +81
===========================================
+ Hits 85838 85855 +17
+ Misses 34047 34029 -18
- Partials 3361 3364 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/test-mode-api
branch
from
8a33ad9 to
bd62af9
Compare
ggazzo
changed the title
ggazzo
changed the title
ggazzo
force-pushed
the
chore/test-mode-api
branch
from
1a0c444 to
d5908a1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (1)
29-33: 💤 Low valueConsider removing inline comments per coding guidelines.
The multi-line comment provides valuable context about TEST_MODE vs TEST_MODE_API behavior, but the coding guideline specifies avoiding code comments in implementation. Consider converting this to JSDoc or moving the rationale to PR/commit documentation.
As per coding guidelines: "Avoid code comments in the implementation" for
**/*.{ts,tsx,js}files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts` around lines 29 - 33, Remove the inline multi-line comment about TEST_MODE vs TEST_MODE_API from the implementation in deprecationWarningLogger.ts and instead attach that rationale as a JSDoc block on the exported logger function (the deprecation warning/logging function in this module) or move the full explanation to the PR/commit message; ensure the code contains only minimal inline notes (if any) and that the behavior references TEST_MODE and TEST_MODE_API remain unchanged while the detailed rationale is documented outside the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci-test-e2e.yml:
- Around line 222-225: The workflow sets TEST_MODE_API but the rocketchat
container never receives it, so deprecationWarningLogger.ts still throws unless
TEST_MODE_API is present in process.env; either (A) add TEST_MODE_API: 'true' to
the rocketchat service environment in docker-compose-ci.yml so the container
sees TEST_MODE_API alongside TEST_MODE, or (B) remove the TEST_MODE_API entries
from .github/workflows/ci-test-e2e.yml to avoid dead config—update
docker-compose-ci.yml (rocketchat service env) if you choose A, or remove all
TEST_MODE_API references from the workflow if you choose B, and run the CI YAML
through a quick lint to confirm no unused vars remain.
In `@docker-compose-ci.yml`:
- Line 16: The PR description and docker-compose-ci.yml are inconsistent: the
file only exposes TEST_MODE (e.g., TEST_MODE=${TEST_MODE:-true}) but the
description claims TEST_MODE_API is passed; fix by making them consistent—either
add TEST_MODE_API to the environment blocks for the rocketchat and
omnichannel-transcript services in docker-compose-ci.yml alongside TEST_MODE
(e.g., TEST_MODE_API=${TEST_MODE_API:-true}) so the description is accurate, or
update the PR description to state that TEST_MODE='api' is the intended
suppression mechanism; reference the TEST_MODE and TEST_MODE_API environment
variables and the rocketchat and omnichannel-transcript service blocks when
making the change.
---
Nitpick comments:
In `@apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts`:
- Around line 29-33: Remove the inline multi-line comment about TEST_MODE vs
TEST_MODE_API from the implementation in deprecationWarningLogger.ts and instead
attach that rationale as a JSDoc block on the exported logger function (the
deprecation warning/logging function in this module) or move the full
explanation to the PR/commit message; ensure the code contains only minimal
inline notes (if any) and that the behavior references TEST_MODE and
TEST_MODE_API remain unchanged while the detailed rationale is documented
outside the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52e3ef37-49c0-4134-997f-533f4ce5f202
📒 Files selected for processing (21)
.github/workflows/ci-test-e2e.ymlapps/meteor/app/lib/server/lib/RateLimiter.jsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/client/lib/queryClient.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/packages/rocketchat-mongo-config/server/index.jsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/methods/registerUser.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/server/startup/initialData.tsapps/meteor/server/startup/serverRunning.tsdocker-compose-ci.yml
✅ Files skipped from review due to trivial changes (2)
- apps/meteor/ee/server/apps/marketplace/isTesting.ts
- apps/meteor/app/livechat/server/lib/AnalyticsTyped.ts
📜 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). (6)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
🧰 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/client/lib/queryClient.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/packages/rocketchat-mongo-config/server/index.jsapps/meteor/server/startup/initialData.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/app/lib/server/lib/RateLimiter.jsapps/meteor/server/methods/registerUser.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
🧠 Learnings (6)
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/queryClient.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/lib/queryClient.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/client/lib/queryClient.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/server/startup/initialData.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/server/methods/registerUser.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.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/client/lib/queryClient.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/server/startup/initialData.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/server/methods/registerUser.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/lib/queryClient.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/server/startup/initialData.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/server/methods/registerUser.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
📚 Learning: 2026-03-09T18:39:14.020Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: When implementing batch processing in server methods, favor a single data pass to collect both the items and any derived fields needed for validation. Use the same dataset for both validation and processing to avoid races between validation and execution, and document the approach in code comments. Apply this pattern to similar Meteor Rocket.Chat server methods under apps/meteor/server/methods to prevent race conditions and ensure consistent batch behavior.
Applied to files:
apps/meteor/server/methods/registerUser.ts
🔇 Additional comments (20)
apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (2)
41-43: LGTM!Also applies to: 69-71, 97-99, 124-126, 134-136, 159-161, 172-174
29-34: ⚖️ Poor tradeoffPropagate
TEST_MODE_APIinto the containers runningdeprecationWarningLogger.
shouldThrowOnDeprecationsuppresses only whenprocess.env.TEST_MODE === 'true' && process.env.TEST_MODE_API === 'true'. Ensuredocker-compose-ci.ymlsetsTEST_MODE_APIin theenvironmentblocks for therocketchatandomnichannel-transcript-serviceservices; otherwise the “passed into containers” behavior won’t match the logger’s condition..github/workflows/ci-test-e2e.yml (1)
178-184: LGTM!Also applies to: 195-195
apps/meteor/app/livechat/server/api/v1/config.ts (1)
11-14: LGTM!apps/meteor/app/livechat/server/livechat.ts (1)
41-43: LGTM!apps/meteor/app/settings/server/CachedSettings.ts (1)
186-186: LGTM!apps/meteor/server/services/authorization/service.ts (1)
24-24: LGTM!apps/meteor/app/livechat/server/startup.ts (1)
123-123: LGTM!apps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.ts (1)
36-36: LGTM!apps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.ts (1)
38-38: LGTM!apps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.ts (2)
42-42: LGTM!
115-115: LGTM!apps/meteor/server/startup/initialData.ts (1)
208-208: LGTM!apps/meteor/server/startup/serverRunning.ts (1)
27-27: LGTM!apps/meteor/server/configuration/configurePassport.ts (1)
60-60: LGTM!apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts (1)
275-275: LGTM!apps/meteor/packages/rocketchat-mongo-config/server/index.js (1)
43-43: LGTM!apps/meteor/app/lib/server/lib/RateLimiter.js (1)
5-5: LGTM!apps/meteor/server/methods/registerUser.ts (1)
153-153: LGTM!apps/meteor/client/lib/queryClient.ts (1)
7-7: LGTM!
| image: ghcr.io/${LOWERCASE_REPOSITORY}/rocket.chat:${DOCKER_TAG}${DOCKER_TAG_SUFFIX_ROCKETCHAT:-} | ||
| environment: | ||
| - TEST_MODE=true | ||
| - 'TEST_MODE=${TEST_MODE:-true}' |
Contributor
There was a problem hiding this comment.
Inconsistency: PR description claims TEST_MODE_API is passed to containers, but it's not in this file.
The changes correctly make TEST_MODE overridable (enabling TEST_MODE='api' to be passed from the workflow), which does suppress deprecation throws. However, the PR description states:
"docker-compose-ci.yml: rocketchat and omnichannel-transcript services now pass TEST_MODE_API into containers."
But TEST_MODE_API is not added to the environment blocks for these services. The actual implementation relies on TEST_MODE='api' instead of TEST_MODE_API='true'.
This works correctly but creates confusion about which environment variable is the intended mechanism. Consider either:
- Adding TEST_MODE_API to these environment blocks to match the PR description, or
- Updating the PR description to accurately reflect that TEST_MODE='api' is the suppression mechanism
Also applies to: 165-165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose-ci.yml` at line 16, The PR description and
docker-compose-ci.yml are inconsistent: the file only exposes TEST_MODE (e.g.,
TEST_MODE=${TEST_MODE:-true}) but the description claims TEST_MODE_API is
passed; fix by making them consistent—either add TEST_MODE_API to the
environment blocks for the rocketchat and omnichannel-transcript services in
docker-compose-ci.yml alongside TEST_MODE (e.g.,
TEST_MODE_API=${TEST_MODE_API:-true}) so the description is accurate, or update
the PR description to state that TEST_MODE='api' is the intended suppression
mechanism; reference the TEST_MODE and TEST_MODE_API environment variables and
the rocketchat and omnichannel-transcript service blocks when making the change.
TEST_MODE now accepts:
- 'true' (default for unit/UI/storybook): deprecation logger throws
on call, rate limiter bypass and short-cache behaviors are active.
- 'api' (API end-to-end suite): same test-mode behaviors as 'true'
(rate-limiter bypass, short cache TTLs, etc.) but the deprecation
logger only logs, does not throw. The API suite intentionally
exercises deprecated DDP methods through /v1/method.call/:method.
- unset: production behavior.
Changes:
- apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts: a
shouldThrowOnDeprecation() helper replaces the seven inline
if (process.env.TEST_MODE === 'true') throw blocks. Only 'true'
triggers the throw; 'api' falls through to the warn-only log.
- Every other strict TEST_MODE === 'true' check across the codebase
(rate limiter, livechat schedulers, settings cache, marketplace
isTesting, registerUser, initialData, serverRunning, twilio
integration, authorization service cache, mongo-config, client
queryClient, CachedSettings) becomes
(... === 'true' || ... === 'api') so the test-mode shortcuts
keep firing for the API suite.
- docker-compose-ci.yml: rocketchat + omnichannel-transcript now
accept the value from the shell via TEST_MODE=\${TEST_MODE:-true}.
Default stays 'true' for any job that does not override.
- .github/workflows/ci-test-e2e.yml: the Start containers step sets
TEST_MODE: 'api' when inputs.type == 'api' or 'api-livechat',
otherwise leaves the docker-compose default. Setting the env on
the test step did not work because the container was already
running with the variable undefined.
ggazzo
force-pushed
the
chore/test-mode-api
branch
from
d5908a1 to
e2720c6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/marketplace/isTesting.ts (1)
1-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale JSDoc (or update it to include
'api').The comment is now inaccurate after Line 7 and can mislead future edits.
Suggested fix
-/** - * Determine whether the application is running in testing mode. - * - * `@returns` `true` if the `TEST_MODE` environment variable equals `'true'`, `false` otherwise. - */ export function isTesting() { return process.env.TEST_MODE === 'true' || process.env.TEST_MODE === 'api'; }As per coding guidelines: "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/ee/server/apps/marketplace/isTesting.ts` around lines 1 - 7, Update the stale JSDoc for the isTesting function: either remove the comment block entirely or modify the description and `@returns` to reflect that isTesting returns true when process.env.TEST_MODE is 'true' or 'api'; locate the isTesting export and update or delete its JSDoc so the doc matches the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts`:
- Around line 29-34: Remove the inline implementation comment block in
deprecationWarningLogger.ts that explains TEST_MODE behavior; instead delete the
comment surrounding the explanation for shouldThrowOnDeprecation() and leave the
helper/function implementation unchanged (keep the function name
shouldThrowOnDeprecation and any usage of it intact); capture the rationale in
the commit message or PR description per coding guidelines.
In `@apps/meteor/packages/rocketchat-mongo-config/server/index.js`:
- Around line 43-46: The test-mode Email.sendAsync stub logs the full options
object which can leak sensitive data; modify the Email.sendAsync replacement
(function _sendAsync) to create a sanitized copy of the options before logging
by redacting fields like to, from, cc, bcc, subject, text, html, headers, and
attachments (replace values with a placeholder like '[REDACTED]' or log only
presence/length), then log that sanitized object instead of the raw options to
avoid exposing addresses or message content in CI/E2E logs.
---
Outside diff comments:
In `@apps/meteor/ee/server/apps/marketplace/isTesting.ts`:
- Around line 1-7: Update the stale JSDoc for the isTesting function: either
remove the comment block entirely or modify the description and `@returns` to
reflect that isTesting returns true when process.env.TEST_MODE is 'true' or
'api'; locate the isTesting export and update or delete its JSDoc so the doc
matches the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51614773-90b1-4c56-8cab-6737f90f0299
📒 Files selected for processing (21)
.github/workflows/ci-test-e2e.ymlapps/meteor/app/lib/server/lib/RateLimiter.jsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/client/lib/queryClient.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/packages/rocketchat-mongo-config/server/index.jsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/methods/registerUser.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/server/startup/initialData.tsapps/meteor/server/startup/serverRunning.tsdocker-compose-ci.yml
📜 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
🧰 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/config.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/methods/registerUser.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/app/lib/server/lib/RateLimiter.jsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/server/startup/initialData.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/client/lib/queryClient.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/packages/rocketchat-mongo-config/server/index.jsapps/meteor/app/settings/server/CachedSettings.ts
🧠 Learnings (6)
📚 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/livechat/server/api/v1/config.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/methods/registerUser.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/server/startup/initialData.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/client/lib/queryClient.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/app/settings/server/CachedSettings.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/livechat/server/api/v1/config.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/methods/registerUser.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/server/startup/initialData.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/client/lib/queryClient.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/app/settings/server/CachedSettings.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/methods/registerUser.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/server/startup/initialData.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/client/lib/queryClient.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/app/settings/server/CachedSettings.ts
📚 Learning: 2026-03-09T18:39:14.020Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: When implementing batch processing in server methods, favor a single data pass to collect both the items and any derived fields needed for validation. Use the same dataset for both validation and processing to avoid races between validation and execution, and document the approach in code comments. Apply this pattern to similar Meteor Rocket.Chat server methods under apps/meteor/server/methods to prevent race conditions and ensure consistent batch behavior.
Applied to files:
apps/meteor/server/methods/registerUser.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/queryClient.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/lib/queryClient.ts
🔇 Additional comments (22)
apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (2)
35-35: LGTM!
42-42: LGTM!Also applies to: 70-70, 98-98, 125-125, 135-135, 160-160, 173-173
.github/workflows/ci-test-e2e.yml (2)
178-184: LGTM!
195-195: LGTM!docker-compose-ci.yml (1)
16-16: LGTM!Also applies to: 165-165
apps/meteor/server/startup/initialData.ts (1)
208-208: LGTM!apps/meteor/server/startup/serverRunning.ts (1)
27-27: LGTM!apps/meteor/server/configuration/configurePassport.ts (1)
60-60: LGTM!apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts (1)
275-275: LGTM!apps/meteor/app/lib/server/lib/RateLimiter.js (1)
5-5: LGTM!apps/meteor/server/methods/registerUser.ts (1)
153-153: LGTM!apps/meteor/app/livechat/server/api/v1/config.ts (1)
11-14: LGTM!apps/meteor/app/livechat/server/lib/AnalyticsTyped.ts (1)
4-7: LGTM!Also applies to: 14-17
apps/meteor/app/livechat/server/livechat.ts (1)
41-43: LGTM!apps/meteor/app/settings/server/CachedSettings.ts (1)
185-192: LGTM!apps/meteor/server/services/authorization/service.ts (1)
22-25: LGTM!apps/meteor/app/livechat/server/startup.ts (1)
123-128: LGTM!apps/meteor/client/lib/queryClient.ts (1)
3-13: LGTM!apps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.ts (1)
36-36: LGTM!apps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.ts (1)
38-38: LGTM!apps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.ts (2)
42-42: LGTM!
115-115: LGTM!
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/ee/server/apps/marketplace/isTesting.ts`:
- Around line 6-7: Update the JSDoc for the isTesting function to reflect its
current behavior: document that the function returns true when
process.env.TEST_MODE equals 'true' or 'api', describe both accepted values and
their meaning (e.g., full test mode vs API-only test mode), and include the
return type and examples if present; ensure the comment sits immediately above
the exported function isTesting so it accurately matches its implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f8001bf-50c6-4f68-8925-efc8feb455b6
📒 Files selected for processing (21)
.github/workflows/ci-test-e2e.ymlapps/meteor/app/lib/server/lib/RateLimiter.jsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/client/lib/queryClient.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/packages/rocketchat-mongo-config/server/index.jsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/methods/registerUser.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/server/startup/initialData.tsapps/meteor/server/startup/serverRunning.tsdocker-compose-ci.yml
✅ Files skipped from review due to trivial changes (1)
- apps/meteor/server/startup/initialData.ts
📜 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). (7)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
🧰 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/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/client/lib/queryClient.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/app/lib/server/lib/RateLimiter.jsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/server/methods/registerUser.tsapps/meteor/packages/rocketchat-mongo-config/server/index.jsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
🧠 Learnings (6)
📚 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/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/client/lib/queryClient.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/server/methods/registerUser.tsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.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/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/client/lib/queryClient.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/server/methods/registerUser.tsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.tsapps/meteor/client/lib/queryClient.tsapps/meteor/app/settings/server/CachedSettings.tsapps/meteor/app/livechat/server/lib/AnalyticsTyped.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.tsapps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.tsapps/meteor/app/livechat/server/livechat.tsapps/meteor/app/livechat/server/startup.tsapps/meteor/server/methods/registerUser.tsapps/meteor/ee/server/apps/marketplace/isTesting.tsapps/meteor/server/configuration/configurePassport.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/server/services/omnichannel-integrations/providers/twilio.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/queryClient.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/lib/queryClient.ts
📚 Learning: 2026-03-09T18:39:14.020Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:14.020Z
Learning: When implementing batch processing in server methods, favor a single data pass to collect both the items and any derived fields needed for validation. Use the same dataset for both validation and processing to avoid races between validation and execution, and document the approach in code comments. Apply this pattern to similar Meteor Rocket.Chat server methods under apps/meteor/server/methods to prevent race conditions and ensure consistent batch behavior.
Applied to files:
apps/meteor/server/methods/registerUser.ts
🔇 Additional comments (20)
apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (2)
29-34: Remove inline implementation rationale comments in TypeScript code.This was already raised earlier and is still present in the current diff.
As per coding guidelines, Avoid code comments in the implementation for
**/*.{ts,tsx,js}files.
35-36: LGTM!Also applies to: 42-43, 70-71, 98-99, 125-126, 135-136, 160-161, 173-174
.github/workflows/ci-test-e2e.yml (1)
178-184: LGTM!Also applies to: 195-195
docker-compose-ci.yml (1)
16-16: LGTM!Also applies to: 165-165
apps/meteor/client/lib/queryClient.ts (1)
7-7: LGTM!apps/meteor/ee/app/livechat-enterprise/server/lib/AutoCloseOnHoldScheduler.ts (1)
36-36: LGTM!apps/meteor/ee/app/livechat-enterprise/server/lib/AutoTransferChatScheduler.ts (1)
38-38: LGTM!apps/meteor/ee/app/livechat-enterprise/server/lib/QueueInactivityMonitor.ts (1)
42-42: LGTM!Also applies to: 115-115
apps/meteor/app/lib/server/lib/RateLimiter.js (1)
5-5: LGTM!apps/meteor/server/methods/registerUser.ts (1)
153-153: LGTM!apps/meteor/app/livechat/server/api/v1/config.ts (1)
11-14: LGTM!apps/meteor/app/livechat/server/lib/AnalyticsTyped.ts (1)
5-5: LGTM!Also applies to: 15-15
apps/meteor/app/livechat/server/livechat.ts (1)
41-43: LGTM!apps/meteor/app/settings/server/CachedSettings.ts (1)
186-186: LGTM!apps/meteor/server/services/authorization/service.ts (1)
24-24: LGTM!apps/meteor/app/livechat/server/startup.ts (1)
123-127: LGTM!apps/meteor/packages/rocketchat-mongo-config/server/index.js (1)
43-46: Sensitive email payload logging concern is already raised in prior review.apps/meteor/server/startup/serverRunning.ts (1)
24-28: LGTM!apps/meteor/server/configuration/configurePassport.ts (1)
58-62: LGTM!apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts (1)
275-276: LGTM!
Comment on lines
6
to
+7
| export function isTesting() { | ||
| return process.env.TEST_MODE === 'true'; | ||
| return process.env.TEST_MODE === 'true' || process.env.TEST_MODE === 'api'; |
Contributor
There was a problem hiding this comment.
Update the function docs to match current behavior.
The JSDoc still says only TEST_MODE === 'true' is treated as testing, but Line 7 now also accepts 'api'.
Suggested fix
- * `@returns` `true` if the `TEST_MODE` environment variable equals `'true'`, `false` otherwise.
+ * `@returns` `true` if the `TEST_MODE` environment variable equals `'true'` or `'api'`, `false` otherwise.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function isTesting() { | |
| return process.env.TEST_MODE === 'true'; | |
| return process.env.TEST_MODE === 'true' || process.env.TEST_MODE === 'api'; | |
| /** | |
| * Determines if the application is running in testing mode. | |
| * `@returns` `true` if the `TEST_MODE` environment variable equals `'true'` or `'api'`, `false` otherwise. | |
| */ | |
| export function isTesting() { | |
| return process.env.TEST_MODE === 'true' || process.env.TEST_MODE === 'api'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/ee/server/apps/marketplace/isTesting.ts` around lines 6 - 7,
Update the JSDoc for the isTesting function to reflect its current behavior:
document that the function returns true when process.env.TEST_MODE equals 'true'
or 'api', describe both accepted values and their meaning (e.g., full test mode
vs API-only test mode), and include the return type and examples if present;
ensure the comment sits immediately above the exported function isTesting so it
accurately matches its implementation.
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
| } | ||
|
|
||
| const memoizedParseExtraAttributes = mem(parseExtraAttributes, { maxAge: process.env.TEST_MODE === 'true' ? 1 : 60000 }); | ||
| const memoizedParseExtraAttributes = mem(parseExtraAttributes, { |
There was a problem hiding this comment.
🟡 Medium: Insecure CSP frame-ancestors policy bypass via omitted Referer header
The /livechat/ endpoint dynamically sets the Content-Security-Policy frame-ancestors directive based on the Referer header to enforce the Livechat_AllowedDomainsList setting. However, if the Referer header is missing, the server removes the Content-Security-Policy header entirely. An attacker can easily bypass the framing restriction by embedding the Livechat widget in an iframe on a page with a <meta name="referrer" content="no-referrer"> tag. This instructs the browser to omit the Referer header, causing the server to fail open and serve the widget without any frame-ancestors protection, enabling clickjacking attacks.
Steps to Reproduce
- The administrator configures
Livechat_AllowedDomainsListto restrict which external domains can embed the Livechat widget. - The attacker creates a malicious page and uses the
no-referrermeta tag to prevent the browser from sending theRefererheader when requesting the iframe:
<!-- attacker.html -->
<html>
<head>
<!-- Instruct the browser not to send the Referer header -->
<meta name="referrer" content="no-referrer">
</head>
<body>
<h1>Clickjacking Test</h1>
<!-- The target livechat endpoint -->
<iframe src="https://target.rocket.chat/livechat/" width="500" height="500"></iframe>
</body>
</html>- When a victim visits
attacker.html, their browser requests/livechat/without aRefererheader. - The server hits the
elseblock inlivechat.ts, removes the CSP header, and serves the response. - The browser renders the iframe because there is no
frame-ancestorsdirective, successfully bypassing the intended domain restrictions.
Fix with AI
Fix the following security vulnerability found by Hacktron.
File: apps/meteor/app/livechat/server/livechat.ts
Lines: 41
Severity: medium
Vulnerability: Insecure CSP frame-ancestors policy bypass via omitted Referer header
Description:
The `/livechat/` endpoint dynamically sets the `Content-Security-Policy` `frame-ancestors` directive based on the `Referer` header to enforce the `Livechat_AllowedDomainsList` setting. However, if the `Referer` header is missing, the server removes the `Content-Security-Policy` header entirely. An attacker can easily bypass the framing restriction by embedding the Livechat widget in an iframe on a page with a `<meta name="referrer" content="no-referrer">` tag. This instructs the browser to omit the `Referer` header, causing the server to fail open and serve the widget without any `frame-ancestors` protection, enabling clickjacking attacks.
Proof of Concept:
**Steps to Reproduce**
1. The administrator configures `Livechat_AllowedDomainsList` to restrict which external domains can embed the Livechat widget.
2. The attacker creates a malicious page and uses the `no-referrer` meta tag to prevent the browser from sending the `Referer` header when requesting the iframe:
```html
<!-- attacker.html -->
<html>
<head>
<!-- Instruct the browser not to send the Referer header -->
<meta name="referrer" content="no-referrer">
</head>
<body>
<h1>Clickjacking Test</h1>
<!-- The target livechat endpoint -->
<iframe src="https://target.rocket.chat/livechat/" width="500" height="500"></iframe>
</body>
</html>
```
3. When a victim visits `attacker.html`, their browser requests `/livechat/` without a `Referer` header.
4. The server hits the `else` block in `livechat.ts`, removes the CSP header, and serves the response.
5. The browser renders the iframe because there is no `frame-ancestors` directive, successfully bypassing the intended domain restrictions.
Affected Code:
if (req.headers.referer && domainWhiteListSetting.trim()) {
domainWhiteList = domainWhiteListSetting.split(',').map((domain) => domain.trim());
const referer = url.parse(req.headers.referer);
if (referer.host && !domainWhiteList.includes(referer.host)) {
res.setHeader('Content-Security-Policy', "frame-ancestors 'none'");
return next();
}
res.setHeader('Content-Security-Policy', `frame-ancestors ${referer.protocol}//${referer.host}`);
} else {
// TODO need to remove inline scripts from this route to be able to enable CSP here as well
res.removeHeader('Content-Security-Policy');
}
Fix this vulnerability. Only change what's necessary - don't modify unrelated code.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing. Any other reply is saved as a triage note.
sampaiodiego
approved these changes
May 25, 2026
This was referenced May 25, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Introduces a second env var, `TEST_MODE_API`, that suppresses the deprecation-logger throw without disabling the warn log. Used by the API end-to-end suite, which intentionally exercises deprecated DDP methods through the `/v1/method.call/:method` proxy and streamer paths.
Why
`apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts` throws under `TEST_MODE` so accidental usage of a deprecated method or endpoint surfaces immediately. That guard is correct for unit tests and UI tests where the call is unintentional. The API integration tests, however, target the proxy specifically — every `methodCall('foo')` is a deliberate cover for the legacy path until the method is migrated to REST.
Without an opt-out, deprecating any used method blows up the API suite even though the test author intends to call it.
How
UI and Unit test runs still throw on accidental deprecated usage.
Why ship separately
Several follow-up PRs (deprecation logging on orphan methods, eventual removal, caller migrations) depend on this opt-out to keep CI green. Landing it ahead of those PRs unblocks them.
Test plan
Summary by CodeRabbit
Chores
Bug Fixes
Task: ARCH-2157