chore: replace console.* with structured @rocket.chat/logger across production code#39216
Conversation
…roduction code - Server-side (54 files): Replace console.log/warn/error/info/debug with Logger instances from @rocket.chat/logger for structured JSON logging - Client-side (65 files): Add eslint-disable-next-line no-console comments for packages without @rocket.chat/logger dependency - Enable no-console: 'warn' ESLint rule to prevent regressions - Skip intentional console usage (showBox.ts, settings.mocks.ts) Benefits: - Structured pino-based logging compatible with log aggregation (ELK, Loki) - Runtime log-level filtering without code changes - Error context captured in structured fields vs flat text - Lint rule prevents new console.log from creeping in
|
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 |
|
WalkthroughThis pull request systematically replaces console logging statements (console.log, console.warn, console.error) with the Logger utility across the codebase. Most files introduce a Logger import, instantiate a module-level logger, and replace console calls with logger methods. Some files add ESLint directives to suppress no-console warnings where console usage is retained. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes are highly repetitive—primarily mechanical replacements of console methods with Logger equivalents across many files. However, the large number of affected files (100+) and variety of contexts require systematic verification to ensure: Logger imports and naming conventions are consistent, all console calls are properly replaced, logic remains unchanged, and ESLint directives are correctly placed. The few files with actual logic changes (like addUserRoles validation and ufs-store public interface additions) add minor complexity. Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 120 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ee/apps/omnichannel-transcript/src/service.ts">
<violation number="1" location="ee/apps/omnichannel-transcript/src/service.ts:36">
P2: Logger.error only accepts a single argument; the second `err` argument here is ignored, so the error/stack is dropped from logs. Log the error object as part of the structured payload instead.</violation>
</file>
<file name="apps/meteor/ee/server/hooks/federation/index.ts">
<violation number="1" location="apps/meteor/ee/server/hooks/federation/index.ts:71">
P2: Logger.error only accepts a single argument; passing the Error as a second argument drops the error object entirely, so stack/err fields are lost in logs.</violation>
</file>
<file name="apps/meteor/app/api/server/api.ts">
<violation number="1" location="apps/meteor/app/api/server/api.ts:88">
P2: Logger.warn only accepts a single payload; passing an Error as a second argument is ignored, so the stack/context is dropped. Use the structured payload form with `err` to capture the exception.</violation>
</file>
<file name="ee/apps/queue-worker/src/service.ts">
<violation number="1" location="ee/apps/queue-worker/src/service.ts:34">
P2: Logger.error only accepts a single argument and forwards it to pino, so passing the error as a second argument is ignored and drops stack/context. Use a structured object so the error is captured.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| res.end('ok'); | ||
| } catch (err) { | ||
| console.error('Service not healthy', err); | ||
| logger.error('Service not healthy', err); |
There was a problem hiding this comment.
P2: Logger.error only accepts a single argument; the second err argument here is ignored, so the error/stack is dropped from logs. Log the error object as part of the structured payload instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ee/apps/omnichannel-transcript/src/service.ts, line 36:
<comment>Logger.error only accepts a single argument; the second `err` argument here is ignored, so the error/stack is dropped from logs. Log the error object as part of the structured payload instead.</comment>
<file context>
@@ -31,7 +33,7 @@ void (async () => {
res.end('ok');
} catch (err) {
- console.error('Service not healthy', err);
+ logger.error('Service not healthy', err);
res.writeHead(500);
</file context>
| logger.error('Service not healthy', err); | |
| logger.error({ err, msg: 'Service not healthy' }); |
| } catch (error) { | ||
| // Log the error but don't prevent the message from being sent locally | ||
| console.error('[sendMessage] Failed to send message to Native Federation:', error); | ||
| logger.error('[sendMessage] Failed to send message to Native Federation:', error); |
There was a problem hiding this comment.
P2: Logger.error only accepts a single argument; passing the Error as a second argument drops the error object entirely, so stack/err fields are lost in logs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/ee/server/hooks/federation/index.ts, line 71:
<comment>Logger.error only accepts a single argument; passing the Error as a second argument drops the error object entirely, so stack/err fields are lost in logs.</comment>
<file context>
@@ -66,7 +68,7 @@ callbacks.add(
} catch (error) {
// Log the error but don't prevent the message from being sent locally
- console.error('[sendMessage] Failed to send message to Native Federation:', error);
+ logger.error('[sendMessage] Failed to send message to Native Federation:', error);
}
},
</file context>
| logger.error('[sendMessage] Failed to send message to Native Federation:', error); | |
| logger.error({ err: error, msg: '[sendMessage] Failed to send message to Native Federation:' }); |
| API.v1.setLimitedCustomFields(nonPublicCustomFields); | ||
| } catch (error) { | ||
| console.warn('Invalid Custom Fields', error); | ||
| logger.warn('Invalid Custom Fields', error); |
There was a problem hiding this comment.
P2: Logger.warn only accepts a single payload; passing an Error as a second argument is ignored, so the stack/context is dropped. Use the structured payload form with err to capture the exception.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/api.ts, line 88:
<comment>Logger.warn only accepts a single payload; passing an Error as a second argument is ignored, so the stack/context is dropped. Use the structured payload form with `err` to capture the exception.</comment>
<file context>
@@ -85,7 +85,7 @@ settings.watch<string>('Accounts_CustomFields', (value) => {
API.v1.setLimitedCustomFields(nonPublicCustomFields);
} catch (error) {
- console.warn('Invalid Custom Fields', error);
+ logger.warn('Invalid Custom Fields', error);
}
});
</file context>
| logger.warn('Invalid Custom Fields', error); | |
| logger.warn({ msg: 'Invalid Custom Fields', err: error }); |
| res.end('ok'); | ||
| } catch (err) { | ||
| console.error('Service not healthy', err); | ||
| logger.error('Service not healthy', err); |
There was a problem hiding this comment.
P2: Logger.error only accepts a single argument and forwards it to pino, so passing the error as a second argument is ignored and drops stack/context. Use a structured object so the error is captured.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ee/apps/queue-worker/src/service.ts, line 34:
<comment>Logger.error only accepts a single argument and forwards it to pino, so passing the error as a second argument is ignored and drops stack/context. Use a structured object so the error is captured.</comment>
<file context>
@@ -29,7 +31,7 @@ void (async () => {
res.end('ok');
} catch (err) {
- console.error('Service not healthy', err);
+ logger.error('Service not healthy', err);
res.writeHead(500);
</file context>
| logger.error('Service not healthy', err); | |
| logger.error({ err, msg: 'Service not healthy' }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/meteor/app/smarsh-connector/server/functions/generateEml.ts (1)
1-12:⚠️ Potential issue | 🟡 MinorUnused Logger import and instance.
The
Loggerimport andloggervariable are declared but never used in any active code path. The only reference tologgeris on line 110 inside a commented-out block. This will likely trigger unused import/variable warnings.Either remove the logger setup since there are no active
console.*calls to replace in this file, or if the intent is to enable the debug logging at line 110, uncomment that code block.Option 1: Remove unused logger
-import { Logger } from '@rocket.chat/logger'; import { MessageTypes } from '@rocket.chat/message-types'; import { Messages, SmarshHistory, Users, Rooms } from '@rocket.chat/models'; import { Meteor } from 'meteor/meteor'; import moment from 'moment-timezone'; import { sendEmail } from './sendEmail'; import { i18n } from '../../../../server/lib/i18n'; import { settings } from '../../../settings/server'; -const logger = new Logger('SmarshConnector:GenerateEml'); - const start =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/smarsh-connector/server/functions/generateEml.ts` around lines 1 - 12, The file imports Logger and creates a logger constant (Logger, logger) but never uses it; either remove the unused import and the const logger declaration from generateEml.ts to eliminate the warning, or re-enable the intended debug log by uncommenting the logging call where logger is referenced (the commented block around the previous logger.debug/logger.error usage) so the logger is actually used; ensure you update the import list accordingly and run lint to confirm the unused-variable warning is resolved.packages/api-client/src/index.ts (1)
97-104:⚠️ Potential issue | 🟠 MajorBehavioral change: Downgrading from error to warning may cause silent failures.
The original code threw an error to prevent malformed URLs. Now it only warns and continues, which can produce invalid URLs like
/endpoint?existingParam?newParam=value(two?characters) when both the endpoint contains a query string andparamsare provided.If the intent is purely to migrate logging, consider keeping the throw or at least returning early after the warning. Otherwise, this will silently send malformed requests that are difficult to debug.
Option A: Restore fail-fast behavior with structured logging (preferred)
if (/\?/.test(endpoint)) { - // eslint-disable-next-line no-console - console.warn('Endpoint cannot contain query string', endpoint); + throw new Error(`Endpoint cannot contain query string: ${endpoint}`); }Option B: Warn and return early to avoid malformed URL
If throwing is undesirable, consider returning early or stripping params:
if (/\?/.test(endpoint)) { // eslint-disable-next-line no-console console.warn('Endpoint cannot contain query string', endpoint); + const response = await this.send(endpoint, 'GET', options ?? {}); + return response.json(); } - const queryParams = this.getParams(params); - const response = await this.send(`${endpoint}${queryParams ? `?${queryParams}` : ''}`, 'GET', options ?? {}); + const queryParams = this.getParams(params); + const response = await this.send(`${endpoint}${queryParams ? `?${queryParams}` : ''}`, 'GET', options ?? {});Based on learnings: In this codebase, fail-fast behavior is preferred over defensive defaults that hide problems.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/src/index.ts` around lines 97 - 104, The current change downgraded a thrown error to a console.warn when an endpoint already contains a query string, which can cause malformed URLs when later appending this.getParams(params) and calling this.send; restore fail-fast behavior by throwing an Error (e.g., throw new Error('Endpoint cannot contain query string: ' + endpoint)) where the console.warn is now, so the request building (the template using endpoint and queryParams before calling this.send and response.json()) never proceeds with an invalid URL; ensure the thrown error includes the endpoint value for debugging.ee/apps/ddp-streamer/src/DDPStreamer.ts (1)
241-246:⚠️ Potential issue | 🟠 MajorLogger.error signature mismatch — error context lost.
logger.erroraccepts a single argument, but two are passed. Theerrobject won't be logged.Proposed fix
- logger.error('Service not healthy', err); + logger.error({ msg: 'Service not healthy', err });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/apps/ddp-streamer/src/DDPStreamer.ts` around lines 241 - 246, In the catch block inside DDPStreamer.ts where logger.error is called, the logger.error signature accepts a single argument so the current call logger.error('Service not healthy', err) drops the err; change the call to pass a single string that includes the error details (e.g., interpolate or format the err.stack/message into the message) so the error context is logged—update the catch in the relevant async handler/function to call logger.error with one composed argument and keep the existing res.writeHead(500); res.end('not healthy'); flow.apps/meteor/server/ufs/ufs-store.ts (1)
308-330:⚠️ Potential issue | 🟠 MajorSecond argument to
logger.error()is silently ignored.The
Logger.error()method accepts only one parameter (msg: object | string), but these three methods pass two arguments. The error object containing the stack trace will be discarded. To preserve error context in structured logs, pass an object with both message and error:🔧 Proposed fix
onCopyError(err: Error, fileId: string, _file: IUpload) { - logger.error(`ufs: cannot copy file "${fileId}" (${err.message})`, err); + logger.error({ msg: `ufs: cannot copy file "${fileId}" (${err.message})`, err }); } onReadError(err: Error, fileId: string, _file: IUpload) { - logger.error(`ufs: cannot read file "${fileId}" (${err.message})`, err); + logger.error({ msg: `ufs: cannot read file "${fileId}" (${err.message})`, err }); } onWriteError(err: Error, fileId: string, _file: IUpload) { - logger.error(`ufs: cannot write file "${fileId}" (${err.message})`, err); + logger.error({ msg: `ufs: cannot write file "${fileId}" (${err.message})`, err }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/ufs/ufs-store.ts` around lines 308 - 330, The three handlers onCopyError, onReadError, and onWriteError call logger.error with two arguments (a string and an Error) but Logger.error accepts a single msg parameter, so the Error is dropped; change each call to pass a single object (or formatted string) that includes both the message and the error (e.g., { msg: `ufs: cannot ... "${fileId}"`, err }) so the stack/metadata is preserved when calling logger.error in onCopyError, onReadError, and onWriteError.
🟠 Major comments (24)
apps/meteor/app/apps/server/bridges/details.ts-14-14 (1)
14-14:⚠️ Potential issue | 🟠 Major
appIdcontext is silently dropped.The
Logger.warnmethod signature (from@rocket.chat/logger) iswarn(msg: object | string): void— it accepts only one argument. The second argumentappIdwill be ignored, losing valuable diagnostic context.🛠️ Proposed fix: include appId in the message
- const logFailure = () => logger.warn('failed to notify about the setting change.', appId); + const logFailure = () => logger.warn({ msg: 'failed to notify about the setting change.', appId });Alternatively, use a template literal:
- const logFailure = () => logger.warn('failed to notify about the setting change.', appId); + const logFailure = () => logger.warn(`failed to notify about the setting change. appId: ${appId}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/apps/server/bridges/details.ts` at line 14, The log call in logFailure uses logger.warn('failed to notify about the setting change.', appId) but `@rocket.chat/logger.warn` accepts a single argument so appId is dropped; update the log to include appId in the single message argument (for example by embedding appId into the message string or passing an object with message and appId) so the warning retains the appId context when logging from logFailure and ensure you modify the call site using logger.warn accordingly.apps/meteor/ee/server/local-services/instance/service.ts-194-194 (1)
194-194:⚠️ Potential issue | 🟠 MajorError context is silently dropped due to incorrect Logger.error usage.
The
Logger.errormethod accepts a single argument (msg: object | string), but this call passes two arguments. Theerrorobject will be ignored, losing valuable error context in logs.Follow the pattern used at line 141 with
SystemLogger.error:- logger.error('Instance service did not start correctly', error); + logger.error({ msg: 'Instance service did not start correctly', err: error });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/local-services/instance/service.ts` at line 194, The current call to logger.error('Instance service did not start correctly', error) passes two args so the error object is dropped; update the call to pass a single object like the pattern used with SystemLogger.error (e.g., logger.error({ msg: 'Instance service did not start correctly', error })) so the error details are included in logs; locate the logger.error call in service.ts and replace the two-argument invocation accordingly.apps/meteor/ee/server/startup/presence.ts-49-51 (1)
49-51:⚠️ Potential issue | 🟠 MajorSecond argument to
logger.erroris silently dropped.The
Logger.errormethod signature iserror(msg: object | string): void— it accepts only one argument. Theerrobject passed as the second argument will be ignored, losing the actual error details in logs.Pass the error as part of a structured object or interpolate it into the message string:
Proposed fix
void Presence.updateConnection(login.user._id, login.connection.id).catch((err) => { - logger.error('Error updating connection presence on heartbeat:', err); + logger.error({ msg: 'Error updating connection presence on heartbeat', err }); });Alternatively, use string interpolation:
- logger.error('Error updating connection presence on heartbeat:', err); + logger.error(`Error updating connection presence on heartbeat: ${err}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/startup/presence.ts` around lines 49 - 51, The current logger.error call drops the error because Logger.error only accepts one argument; update the error logging in the Presence.updateConnection catch block (where Presence.updateConnection(login.user._id, login.connection.id) is called) to pass a single structured object or interpolated string that includes the error (e.g., include the error under an "err" or "error" key along with context like userId and connectionId) so the actual error details are not lost.packages/ui-voip/src/context/useMediaStream.ts-18-19 (1)
18-19:⚠️ Potential issue | 🟠 MajorReplace
console.error+ lint bypass with the VoIP logger.Line 18/42 reintroduce raw console logging via
eslint-disable-next-line no-console. That bypasses the new lint guard and keeps these errors unstructured. Please use the package logger (MediaCallLogger) for both paths and drop the inline comments.Proposed fix
import { useSafeRefCallback } from '@rocket.chat/fuselage-hooks'; import type { MediaSignalingSession } from '@rocket.chat/media-signaling'; import { useCallback, useRef } from 'react'; +import MediaCallLogger from './MediaCallLogger'; @@ } catch (error) { - // eslint-disable-next-line no-console - console.error('MediaCall: useMediaStream - Error getting remote media stream', error); + MediaCallLogger.error({ err: error }, 'MediaCall: useMediaStream - Error getting remote media stream'); return null; } }; @@ node.srcObject = remoteStream.stream; node.play().catch((error) => { - // eslint-disable-next-line no-console - console.error('MediaCall: useMediaStream - Error playing media stream', error); + MediaCallLogger.error({ err: error }, 'MediaCall: useMediaStream - Error playing media stream'); });As per coding guidelines,
**/*.{ts,tsx,js}: “Avoid code comments in the implementation”.Also applies to: 42-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-voip/src/context/useMediaStream.ts` around lines 18 - 19, Replace the raw console.error calls (and remove the inline eslint-disable-next-line no-console comments) in the useMediaStream hook with the package logger: call MediaCallLogger.error with the same descriptive message and include the caught error object so the error is structured and searchable; update both error paths referenced in the diff (the error when getting the remote media stream and the other error path around line ~42) to use MediaCallLogger.error instead of console.error.packages/apps-engine/src/client/AppClientManager.ts-25-26 (1)
25-26:⚠️ Potential issue | 🟠 MajorRemove ad-hoc console logging from this production path.
load()now logs enabled apps viaconsole.logand bypasses lint with an inline disable. That regresses the no-console direction of this PR and introduces avoidable runtime noise. Please remove both lines (or switch to structured logger in a dedicated follow-up where dependency wiring is available).Suggested minimal fix
- // eslint-disable-next-line no-console - console.log('Enabled apps:', this.apps);As per coding guidelines:
**/*.{ts,tsx,js}—Avoid code comments in the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/client/AppClientManager.ts` around lines 25 - 26, Remove the ad-hoc console logging in AppClientManager: delete the two lines that call console.log('Enabled apps:', this.apps) and the preceding eslint-disable-next-line no-console so the production load path no longer emits runtime console noise; if structured logging is required later, add it via the project's logger in a follow-up, but for now remove those two lines from the load() flow in class AppClientManager (the place referencing this.apps).packages/apps-engine/src/server/accessors/LivechatRead.ts-19-22 (1)
19-22:⚠️ Potential issue | 🟠 MajorAvoid per-call deprecation warnings and inline eslint suppression in runtime path.
Line 20 warns on every invocation, which can flood production logs; Line 19 adds an implementation comment to bypass
no-console. Emit this deprecation warning once instead.As per coding guidelines: "Avoid code comments in the implementation".Proposed fix
+let hasWarnedIsOnlineDeprecation = false; + export class LivechatRead implements ILivechatRead { constructor( private readonly livechatBridge: LivechatBridge, private readonly appId: string, ) {} @@ public isOnline(departmentId?: string): boolean { - // eslint-disable-next-line no-console - console.warn( - "The `LivechatRead.isOnline` method is deprecated and won't behave as intended. Please use `LivechatRead.isOnlineAsync` instead", - ); + if (!hasWarnedIsOnlineDeprecation) { + hasWarnedIsOnlineDeprecation = true; + console.warn( + "The `LivechatRead.isOnline` method is deprecated and won't behave as intended. Please use `LivechatRead.isOnlineAsync` instead", + ); + } return this.livechatBridge.doIsOnline(departmentId, this.appId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/server/accessors/LivechatRead.ts` around lines 19 - 22, The per-call console.warn in LivechatRead.isOnline (and its inline eslint-disable comment) should be changed to emit the deprecation warning only once: add a module-level boolean (e.g., deprecationWarned) and, inside LivechatRead.isOnline, check and set it so console.warn is called a single time, then remove the inline eslint-disable comment; keep the message text the same and consider using the project logger if available, but the core change is to guard the existing console.warn with a one-time flag so it no longer floods logs on every invocation and delete the inline eslint suppression.packages/models/src/models/BaseRaw.ts-36-42 (1)
36-42:⚠️ Potential issue | 🟠 MajorRemove eslint-disable comments and use structured logging instead of console.warn
These changes reintroduce
console.warnwith inline eslint-disable comments (lines 39–40, 102–103, 130) that violate the coding guideline to avoid code comments in implementation. However,@rocket.chat/loggeris not currently a dependency ofpackages/models, so the proposed structured logger approach requires first adding that dependency to this package'spackage.json.Either:
- Add
@rocket.chat/loggeras a dependency topackages/modelsand refactor these three catch blocks to use structured logging, or- If that dependency is not appropriate for this layer, apply an alternative solution that removes the eslint-disable comments and follows the no-console lint rule.
Also applies to lines 102–103 and 130.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/BaseRaw.ts` around lines 36 - 42, Add `@rocket.chat/logger` to packages/models dependencies and replace direct console.warn usages in BaseRaw.ts: import the logger (e.g., import { logger } from '@rocket.chat/logger') and change the warnFields implementation and the three catch blocks that currently call console.warn (the warnFields constant and the two other catch sites referenced) to call logger.warn with a structured message and metadata (include error/stack and contextual fields). Remove the eslint-disable comments after switching to logger.warn so no console.* calls remain in this file.apps/meteor/app/api/server/lib/MultipartUploadHandler.ts-44-46 (1)
44-46:⚠️ Potential issue | 🟠 MajorLogger method signature mismatch — error context will be lost.
The
Logger.warnmethod only accepts a singlemsg: object | stringparameter. The second argument (error) will be silently ignored, losing the error details that are critical for debugging cleanup failures.To preserve the error context, pass an object with both the message and error:
🐛 Proposed fix
} catch (error: any) { - logger.warn(`[UploadService] Failed to cleanup temp file: ${tempFilePath}`, error); + logger.warn({ msg: `Failed to cleanup temp file: ${tempFilePath}`, error }); }Note: The
[UploadService]prefix can be removed since the logger is already instantiated with'Api:MultipartUploadHandler'context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/lib/MultipartUploadHandler.ts` around lines 44 - 46, The catch block in MultipartUploadHandler uses logger.warn(`[UploadService] Failed to cleanup temp file: ${tempFilePath}`, error) which loses the error because logger.warn accepts a single msg parameter; update the catch to call logger.warn with a single object containing both message and error (e.g., { msg: `Failed to cleanup temp file: ${tempFilePath}`, err: error }) so the error context is preserved; use the existing logger context (remove the `[UploadService]` prefix) and ensure you update the catch surrounding tempFilePath and any similar cleanup logging to follow the same pattern.ee/apps/ddp-streamer/src/Client.ts-84-86 (1)
84-86:⚠️ Potential issue | 🟠 MajorLogger.error signature mismatch — error context lost.
logger.erroraccepts a single argument. Theerrobject passed as the second argument will be ignored.Proposed fix
- logger.error('Error updating connection presence:', err); + logger.error({ msg: 'Error updating connection presence', err });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/apps/ddp-streamer/src/Client.ts` around lines 84 - 86, The call in the Presence.updateConnection(this.userId, this.connection.id).catch handler passes the error as a second argument to logger.error which only accepts one argument, so the error context is lost; update the catch to pass a single value that includes both a descriptive message and the error (e.g., a single object or a formatted string) — reference the Promise returned by Presence.updateConnection and replace logger.error('Error updating connection presence:', err) with a single-argument call such as logger.error({ message: 'Error updating connection presence', userId: this.userId, connectionId: this.connection.id, err }) or a formatted string that includes String(err) so the error details are preserved.ee/apps/ddp-streamer/src/DDPStreamer.ts-265-267 (1)
265-267:⚠️ Potential issue | 🟠 MajorLogger.error signature mismatch — startup error context lost.
Same issue: the
errobject is passed as a second argument and will be ignored.Proposed fix
- logger.error('DDPStreamer did not start correctly', err); + logger.error({ msg: 'DDPStreamer did not start correctly', err });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/apps/ddp-streamer/src/DDPStreamer.ts` around lines 265 - 267, In the DDPStreamer startup catch block, the call logger.error('DDPStreamer did not start correctly', err) passes the error as a second argument which gets ignored; update the call in DDPStreamer to pass the error as structured data or include its message/stack in the first argument (for example use logger.error({ err }, 'DDPStreamer did not start correctly') or include err.message/err.stack in the log string) so the actual error context is preserved.ee/apps/ddp-streamer/src/Client.ts-119-121 (1)
119-121:⚠️ Potential issue | 🟠 MajorLogger.error signature mismatch — error context lost.
Same issue: the
errobject is passed as a second argument and will be ignored.Proposed fix
- logger.error('Unexpected error:', err); + logger.error({ msg: 'Unexpected error', err });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/apps/ddp-streamer/src/Client.ts` around lines 119 - 121, The error handler for this.ws uses logger.error('Unexpected error:', err) which drops the error because the logger expects a single message or an object; update the handler in Client (the this.ws.on('error', ...) block) to pass the error as structured context or include its message/stack in the single logger call (e.g., logger.error({ err }, 'Unexpected error') or logger.error(`Unexpected error: ${err.stack || err}`)), keeping the subsequent this.ws.close(WS_ERRORS.CLOSE_PROTOCOL_ERROR, WS_ERRORS_MESSAGES.CLOSE_PROTOCOL_ERROR) invocation unchanged.ee/apps/ddp-streamer/src/Streamer.ts-91-102 (1)
91-102:⚠️ Potential issue | 🟠 MajorLogger method signature mismatch and potential log level consideration.
Two issues in this catch block:
Line 101:
logger.erroraccepts a single argument, but two are passed. Theerrorobject will be ignored.Based on learnings from a similar module (
apps/meteor/server/modules/streamer/streamer.module.ts), per-subscription delivery failures intentionally usedebuglevel to keep logs less noisy. Consider whethererrorlevel is appropriate here or ifdebugwould be more consistent.Proposed fix for signature issue
} catch (error: any) { if (error.code === 'ERR_STREAM_DESTROYED') { logger.warn('Trying to send data to destroyed stream, closing connection.'); // if we still tried to send data to a destroyed stream, we'll try again to close the connection if (subscription.client.ws.readyState !== WebSocket.OPEN) { subscription.stop(); subscription.client.ws.close(); } } - logger.error('Error trying to send data to stream.', error); + logger.error({ msg: 'Error trying to send data to stream.', error }); }Based on learnings: "In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/apps/ddp-streamer/src/Streamer.ts` around lines 91 - 102, The catch block handling send failures (in the catch for send to subscription in Streamer.ts) currently calls logger.error with two args (logger.error('Error trying to send data to stream.', error)) which loses the error object and uses a noisy log level; change this to a single-argument log that preserves the error details and demote the level to debug to match per-subscription delivery behavior (e.g., use logger.debug and include error.message/stack in the single formatted log string). Keep the existing ERR_STREAM_DESTROYED branch and subscription.stop/close behavior intact.ee/apps/ddp-streamer/src/configureServer.ts-23-23 (1)
23-23:⚠️ Potential issue | 🟠 MajorLogger.error signature mismatch — error object will be ignored.
The
Logger.errormethod accepts a singleobject | stringargument, but two arguments are passed here. Theerrparameter will be silently ignored, losing error context. Pass the error as a property within an object instead.Proposed fix
- .catch((err) => logger.error('DDPStreamer not able to retrieve login services configuration', err)); + .catch((err) => logger.error({ msg: 'DDPStreamer not able to retrieve login services configuration', err }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/apps/ddp-streamer/src/configureServer.ts` at line 23, The catch handler passes two arguments to logger.error so the error object is dropped; update the catch in configureServer.ts (the .catch((err) => logger.error(...)) callback) to call logger.error with a single object that includes the message and the err (e.g., { message: 'DDPStreamer not able to retrieve login services configuration', err }) so the error context is preserved; keep the same message text but wrap it and the captured err into one object for logger.error.ee/apps/presence-service/src/service.ts-34-34 (1)
34-34:⚠️ Potential issue | 🟠 MajorPass error as structured object to preserve error context.
The
Logger.error()method accepts only a single parameter (msg: object | string), so theerrargument will be silently dropped. This loses critical error information when the health check fails.Suggested fix
- logger.error('Service not healthy', err); + logger.error({ msg: 'Service not healthy', err });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/apps/presence-service/src/service.ts` at line 34, The logger.error call currently passes two arguments (logger.error('Service not healthy', err)) which drops the error; update the call to pass a single structured object so the error is preserved (e.g., logger.error({ message: 'Service not healthy', error: err })) and, if available, include err.stack or a serialized form; change the invocation in service.ts (the logger.error call handling the service health check) to pass that single object.ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.ts-14-15 (1)
14-15:⚠️ Potential issue | 🟠 MajorRemove this production
console.logand its lint bypass.Lines [14-15] add unstructured console logging of user payload in a hot server path. This should not be introduced in production flow.
Proposed change
- // eslint-disable-next-line no-console - console.log('createOrUpdateFederatedUser ->', options);As per coding guidelines
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.ts` around lines 14 - 15, Remove the production console.log and the accompanying eslint-disable comment in createOrUpdateFederatedUser (the console.log('createOrUpdateFederatedUser ->', options) lines); either delete them or replace with a call to the project's structured logger (e.g., processLogger or logger) at debug/trace level and avoid logging the full user payload—log only minimal, non-sensitive identifiers (or a redacted subset) if telemetry is required; ensure you remove the no-console bypass and use the existing logging utility used elsewhere in the module/function.packages/apps-engine/src/server/AppManager.ts-297-300 (1)
297-300:⚠️ Potential issue | 🟠 MajorRemove inline
eslint-disable-next-linecomments and replace rawconsole.*calls with appropriate logging or error handling.Lines 297–300, 498–500, 525–527, 951–953, 1149–1151, 1163–1164, and 1243–1245 contain inline ESLint suppression comments, which violates the coding guideline: "Avoid code comments in the implementation" (per
**/*.{ts,tsx,js}).However, the proposed fix referencing
@rocket.chat/loggeris not viable—this package is not a dependency ofpackages/apps-engine. Before proceeding, clarify the intended logging strategy:
- Should
@rocket.chat/loggerbe added as a new dependency to apps-engine?- Should these console calls be removed entirely or replaced with a different logging mechanism?
- If this is a style-only cleanup, consider deferring to a follow-up PR to keep scope tight and avoid introducing new dependencies in this PR.
Address the comment violations directly rather than suppressing the linter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/server/AppManager.ts` around lines 297 - 300, Remove the inline eslint-disable comments and replace raw console calls in AppManager with a proper logging or error-handling approach: locate the console.warn/console.error usages around the AppManager methods that reference item.info.name, item.id and variable e and replace them with calls to an injectable logger (e.g., this.logger.warn/this.logger.error) or with throwing/propagating the error if no logger exists; do not add `@rocket.chat/logger` inside this PR—instead either use an existing logger passed into AppManager (add a constructor param or setter if missing) or remove the console calls and rethrow the error, and if you decide a new dependency is needed open a follow-up PR to add it and wire it into AppManager.ee/packages/federation-matrix/src/events/ping.ts-5-6 (1)
5-6:⚠️ Potential issue | 🟠 MajorUse Logger instead of console.log, consistent with other event handlers in this package.
All other event files in federation-matrix (edu.ts, member.ts, message.ts, reaction.ts) already use
@rocket.chat/logger. The Logger import and dependency are available to federation-matrix; ping.ts should follow the same pattern.Rewrite using Logger (pattern from other event files)
+import { Logger } from '@rocket.chat/logger'; import { federationSDK } from '@rocket.chat/federation-sdk'; +const logger = new Logger('federation-matrix:ping'); + export const ping = async () => { federationSDK.eventEmitterService.on('homeserver.ping', async (data) => { - // eslint-disable-next-line no-console - console.log('Message received from homeserver', data); + logger.debug('Message received from homeserver', data); }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/src/events/ping.ts` around lines 5 - 6, Replace the console.log call in ping.ts with the package logger used by other event handlers: import Logger from '@rocket.chat/logger' (or use the same logger instance name used elsewhere, e.g., "logger") and call logger.debug/info with the same message and data instead of console.log; remove the eslint-disable-next-line no-console and ensure the log call follows the same pattern as in edu.ts/member.ts/message.ts/reaction.ts so the event handler uses the shared Logger instance.packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts-615-615 (1)
615-615:⚠️ Potential issue | 🟠 MajorAvoid logging raw subprocess payloads and stderr content.
At Line 615 and Line 727 (plus related error paths), full payload/chunk logging can expose sensitive data from apps/runtime. Log sanitized metadata instead (appId, method, request id, byte length, error type/message).
🔒 Minimal sanitization direction
- console.log('SUBPROCESS LOG', message); + this.debug( + '[%s] subprocess log method=%s id=%s', + this.getAppId(), + message.payload.method, + 'id' in message.payload ? String(message.payload.id) : 'n/a', + ); - console.error('Subprocess stderr', chunk.toString()); + this.debug('[%s] subprocess stderr parse failure (bytes=%d)', this.getAppId(), chunk.length);Also applies to: 686-687, 693-695, 710-710, 726-727
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts` at line 615, Replace raw console.log of subprocess payloads in AppsEngineDenoRuntime (where it currently logs the variable "message" and other raw chunks) with sanitized logging: extract and log only metadata such as appId, method/name, requestId (if present), byte length of payload, and for errors log error type and message; do this in the message handler and the related error/stream-chunk handlers (the spots currently printing "message" and raw chunks) so no full payload or stderr content is written to logs by the class AppsEngineDenoRuntime or its subprocess message/error handlers.packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts-67-68 (1)
67-68:⚠️ Potential issue | 🟠 MajorStop bypassing
no-consolein runtime code with inline suppressions.These additions reintroduce
console.*across production runtime paths and bypass the lint guard with inlineeslint-disablecomments. Please route these branches through the existing structured/runtime logging path in this package and remove the inline suppressions.As per coding guidelines:
**/*.{ts,tsx,js}→Avoid code comments in the implementation.Also applies to: 201-202, 432-433, 614-615, 622-623, 685-687, 693-695, 699-700, 704-705, 709-710, 714-715, 726-727
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts` around lines 67 - 68, The code uses inline eslint-disable and direct console.* calls in AppsEngineDenoRuntime (e.g., the negative APPS_ENGINE_RUNTIME_TIMEOUT message) which violates runtime logging rules; remove the inline suppressions and replace each console.* call with the package's structured runtime logger used elsewhere in AppsEngineDenoRuntime (e.g., use the class logger instance or the existing runtime logging helper such as this.logger/this.getLogger()/runtimeLogger) at the appropriate level (warn/error/info), keep the original messages, and update all reported locations (67-68, 201-202, 432-433, 614-615, 622-623, 685-687, 693-695, 699-700, 704-705, 709-710, 714-715, 726-727) to use that logger instead of console.ee/packages/network-broker/src/index.ts-148-150 (1)
148-150:⚠️ Potential issue | 🟠 MajorRemove console.log and eslint-disable comment from broker startup hook.
Line 149 adds an inline ESLint bypass and Line 150 uses
console.login production code. This regresses the PR's structured-logging objective and bypasses the re-enabledno-consoleprotection. The inline comment also violates the repository's coding guideline to avoid code comments in implementation.🔧 Proposed fix
import { isMeteorError, MeteorError } from '@rocket.chat/core-services'; +import { Logger } from '@rocket.chat/logger'; import EJSON from 'ejson'; import type Moleculer from 'moleculer'; import { Errors, Serializers, ServiceBroker } from 'moleculer'; import { pino } from 'pino'; import { NetworkBroker } from './NetworkBroker'; + +const logger = new Logger('NetworkBroker'); @@ errorRegenerator: new CustomRegenerator(), started(): void { - // eslint-disable-next-line no-console - console.log('NetworkBroker started successfully.'); + logger.info('NetworkBroker started successfully.'); }, ...options, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/network-broker/src/index.ts` around lines 148 - 150, Remove the inline ESLint disable and console.log in the started() hook; replace them with a structured log call using the module's logger (e.g., call this.logger.info('NetworkBroker started successfully.') or processLogger.info(...) depending on which logger instance is used in this module) so the message uses the project's structured-logging facility instead of console.log; ensure you import or reference the correct logger instance and remove the eslint-disable comment and console.log statement from started().packages/cas-validate/src/validate.ts-289-291 (1)
289-291:⚠️ Potential issue | 🟠 MajorDo not log raw CAS response payloads in this error path.
Line 291 logs the full CAS response body, which can contain user identifiers and attributes. This introduces sensitive-data leakage risk.
Proposed fix
// The response was not in any expected format, error callback(new Error('Bad response format.')); - // eslint-disable-next-line no-console - console.error(response);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cas-validate/src/validate.ts` around lines 289 - 291, The code currently prints the entire CAS response object (console.error(response)) right after calling callback(new Error('Bad response format.')), which risks leaking sensitive user attributes; remove the direct logging of the response variable and instead, if telemetry is required, log only non-sensitive metadata derived from response (e.g., response.status or a truncated/error code) or a sanitized error message; update the code around the callback(...) / console.error(response) usage in validate.ts to stop printing the raw response and replace it with a safe, minimal log or no log at all.apps/meteor/server/startup/migrations/v312.ts-18-19 (1)
18-19:⚠️ Potential issue | 🟠 MajorNarrow
unknownbefore passing tologger.warn.Line 19 passes
error(typeunknown) directly tologger.warn(msg: object | string). Theunknowntype must be narrowed before assignment to a more specific type. This pattern is already used elsewhere in the codebase (e.g.,apps/meteor/server/services/video-conference/service.ts:391).Proposed fix
} catch (error: unknown) { logger.warn('Error dropping redundant indexes, continuing...'); - logger.warn(error); + logger.warn(error instanceof Error ? { err: error, message: error.message } : String(error)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/startup/migrations/v312.ts` around lines 18 - 19, The code passes an unknown `error` directly to `logger.warn`; narrow `error` before logging by checking its type (e.g., if (error instanceof Error) logger.warn(error.message, { stack: error.stack }) else if (typeof error === 'string') logger.warn(error) else logger.warn(String(error)); update the `v312` migration where `logger.warn('Error dropping redundant indexes, continuing...')` and `logger.warn(error)` are called to use this narrowed/serialized form so the call matches logger.warn(msg: object | string) and mirrors the pattern used in `apps/meteor/server/services/video-conference/service.ts`.ee/apps/account-service/src/service.ts-34-34 (1)
34-34:⚠️ Potential issue | 🟠 MajorError object won't be logged—Logger.error accepts only one argument.
The
Logger.errormethod signature iserror(msg: object | string): void. Theerrparameter passed as the second argument will be silently ignored, losing valuable error context for debugging health check failures.Pass the error as part of a structured object:
Proposed fix
- logger.error('Service not healthy', err); + logger.error({ msg: 'Service not healthy', err });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/apps/account-service/src/service.ts` at line 34, The call to logger.error in service.ts currently passes two arguments so the error object is dropped; update the logger.error invocation that currently reads logger.error('Service not healthy', err) to pass a single structured object (e.g., include the message and the err under a property) so the error details are logged, ensuring you reference the same logger.error call and the err variable in the surrounding health-check logic.packages/models/src/models/Roles.ts-176-178 (1)
176-178:⚠️ Potential issue | 🟠 MajorDo not reintroduce raw
consolein production model code.This adds a new
console.logplus lint bypass in the failure path, which conflicts with this PR’s migration objective and weakens the no-console guardrail.🔧 Suggested fix
- // eslint-disable-next-line no-console - console.log('Role not found', _id, name); - throw new Error('Role not found'); + throw new Error(`Role not found: id=${_id}, name=${name}`);As per coding guidelines "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/Roles.ts` around lines 176 - 178, Remove the raw console.log and eslint-disable and replace it with the project's logger (e.g., processLogger.error or the module-scoped logger) in the Roles model failure path: log a clear error including the _id and name variables, then throw the Error (optionally with a concise message); update the code around the Role-not-found branch in Roles.ts (the block that currently logs 'Role not found' for _id and name) to use logger.error instead of console and remove lint bypass.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (120)
apps/meteor/app/api/server/ApiClass.tsapps/meteor/app/api/server/api.tsapps/meteor/app/api/server/helpers/getUserInfo.tsapps/meteor/app/api/server/lib/MultipartUploadHandler.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/e2e.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/app/apps/server/bridges/details.tsapps/meteor/app/apps/server/bridges/livechat.tsapps/meteor/app/autotranslate/client/lib/autotranslate.tsapps/meteor/app/cloud/server/index.tsapps/meteor/app/error-handler/server/lib/RocketChat.ErrorHandler.tsapps/meteor/app/file-upload/ufs/Webdav/server.tsapps/meteor/app/file/server/file.server.tsapps/meteor/app/lib/server/methods/restartServer.tsapps/meteor/app/license/client/index.tsapps/meteor/app/mailer/server/api.tsapps/meteor/app/settings/server/applyMiddlewares.tsapps/meteor/app/settings/server/functions/overrideGenerator.tsapps/meteor/app/smarsh-connector/server/functions/generateEml.tsapps/meteor/app/ui-utils/client/lib/LegacyRoomManager.tsapps/meteor/app/ui/client/lib/recorderjs/AudioRecorder.tsapps/meteor/app/ui/client/lib/recorderjs/videoRecorder.tsapps/meteor/app/utils/server/functions/getMongoInfo.tsapps/meteor/ee/app/license/server/startup.tsapps/meteor/ee/server/api/licenses.tsapps/meteor/ee/server/apps/storage/AppGridFSSourceStorage.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/ee/server/lib/audit/methods.tsapps/meteor/ee/server/local-services/instance/service.tsapps/meteor/ee/server/startup/presence.tsapps/meteor/server/lib/dataExport/processDataDownloads.tsapps/meteor/server/lib/roles/addUserRoles.tsapps/meteor/server/modules/notifications/notifications.module.tsapps/meteor/server/modules/streamer/streamer.module.tsapps/meteor/server/oauth2-server/model.tsapps/meteor/server/oauth2-server/oauth.tsapps/meteor/server/routes/avatar/utils.tsapps/meteor/server/services/authorization/canAccessRoom.tsapps/meteor/server/services/authorization/service.tsapps/meteor/server/services/messages/hooks/BeforeSaveMarkdownParser.tsapps/meteor/server/startup/initialData.tsapps/meteor/server/startup/migrations/v312.tsapps/meteor/server/startup/migrations/v319.tsapps/meteor/server/startup/serverRunning.tsapps/meteor/server/ufs/ufs-local.tsapps/meteor/server/ufs/ufs-methods.tsapps/meteor/server/ufs/ufs-server.tsapps/meteor/server/ufs/ufs-store.tsee/apps/account-service/src/service.tsee/apps/authorization-service/src/service.tsee/apps/ddp-streamer/src/Client.tsee/apps/ddp-streamer/src/DDPStreamer.tsee/apps/ddp-streamer/src/Streamer.tsee/apps/ddp-streamer/src/configureServer.tsee/apps/ddp-streamer/src/service.tsee/apps/omnichannel-transcript/src/service.tsee/apps/presence-service/src/service.tsee/apps/queue-worker/src/service.tsee/packages/federation-matrix/src/events/ping.tsee/packages/federation-matrix/src/helpers/createOrUpdateFederatedUser.tsee/packages/federation-matrix/tests/helper/config.tsee/packages/federation-matrix/tests/helper/synapse-client.tsee/packages/license/src/showLicense.tsee/packages/network-broker/src/index.tsee/packages/presence/src/Presence.tsee/packages/presence/src/lib/PresenceReaper.tspackages/api-client/src/index.tspackages/apps-engine/src/client/AppClientManager.tspackages/apps-engine/src/client/AppsEngineUIClient.tspackages/apps-engine/src/server/AppManager.tspackages/apps-engine/src/server/ProxiedApp.tspackages/apps-engine/src/server/accessors/LivechatRead.tspackages/apps-engine/src/server/compiler/AppPackageParser.tspackages/apps-engine/src/server/logging/AppConsole.tspackages/apps-engine/src/server/managers/AppApi.tspackages/apps-engine/src/server/managers/AppListenerManager.tspackages/apps-engine/src/server/managers/AppPermissionManager.tspackages/apps-engine/src/server/managers/AppSchedulerManager.tspackages/apps-engine/src/server/managers/AppSlashCommand.tspackages/apps-engine/src/server/managers/AppVideoConfProvider.tspackages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.tspackages/apps-engine/tests/runner.tspackages/cas-validate/src/validate.tspackages/core-services/src/lib/mongo.tspackages/ddp-client/src/MinimalDDPClient.tspackages/ddp-client/src/types/ClientStream.tspackages/eslint-config/index.jspackages/favicon/src/index.tspackages/i18n/src/index.tspackages/livechat/src/lib/connection.tspackages/livechat/src/lib/hooks.tspackages/livechat/src/lib/transcript.tspackages/livechat/src/routes/LeaveMessage/index.tsxpackages/livechat/src/routes/SwitchDepartment/index.tsxpackages/livechat/src/store/index.tsxpackages/livechat/src/widget.tspackages/message-parser/benchmarks/parser.bench.tspackages/models/src/models/BaseRaw.tspackages/models/src/models/Roles.tspackages/models/src/models/Subscriptions.tspackages/models/src/models/Users.tspackages/release-action/src/getMetadata.tspackages/tracing/src/index.tspackages/ui-client/src/components/Modal/GenericModal/withDoNotAskAgain.tsxpackages/ui-contexts/src/TranslationContext.tspackages/ui-contexts/src/hooks/useAccountsCustomFields.tspackages/ui-contexts/src/hooks/useAvailableDevices.tspackages/ui-contexts/src/hooks/useDeviceConstraints.tspackages/ui-contexts/src/hooks/useSelectedDevices.tspackages/ui-contexts/src/hooks/useSetInputMediaDevice.tspackages/ui-contexts/src/hooks/useSetOutputMediaDevice.tspackages/ui-voip/src/components/DevicePicker.tsxpackages/ui-voip/src/context/MediaCallLogger.tspackages/ui-voip/src/context/MediaCallProvider.tsxpackages/ui-voip/src/context/MockedMediaCallProvider.tsxpackages/ui-voip/src/context/useMediaSession.tspackages/ui-voip/src/context/useMediaSessionInstance.tspackages/ui-voip/src/context/useMediaStream.tspackages/ui-voip/src/context/useTonePlayer.ts
|
|
||
| if (shouldPreventAnonymousRead || shouldPreventUserRead) { | ||
| console.log('shouldPreventAnonymousRead', shouldPreventAnonymousRead); | ||
| logger.info('shouldPreventAnonymousRead', shouldPreventAnonymousRead); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking Logger.info signature ==="
rg -n -A5 "info\(" packages/logger/src/index.ts | head -50
echo
echo "=== Checking context around line 838 in ApiClass.ts ==="
sed -n '830,845p' apps/meteor/app/api/server/ApiClass.ts
echo
echo "=== Checking if Logger has multiple overloads or variadic args ==="
rg -B3 -A10 "class Logger" packages/logger/src/index.tsRepository: RocketChat/Rocket.Chat
Length of output: 1709
Fix invalid logger.info call signature.
Line 838 calls logger.info with two arguments, but Logger.info accepts a single object | string parameter. This violates the type signature and will fail type-checking.
Proposed fix
- logger.info('shouldPreventAnonymousRead', shouldPreventAnonymousRead);
+ logger.info({ event: 'shouldPreventAnonymousRead', shouldPreventAnonymousRead });📝 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.
| logger.info('shouldPreventAnonymousRead', shouldPreventAnonymousRead); | |
| logger.info({ event: 'shouldPreventAnonymousRead', shouldPreventAnonymousRead }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/ApiClass.ts` at line 838, The logger.info call is
using two positional arguments which breaks the Logger.info(type: object|string)
signature; update the call in ApiClass (the logger.info usage referencing
shouldPreventAnonymousRead) to pass a single value—either a single string
message or an object containing both a descriptive field and the
shouldPreventAnonymousRead value (e.g., { event: 'shouldPreventAnonymousRead',
value: shouldPreventAnonymousRead }) so it matches Logger.info's
single-parameter type.
| logger.info('[OAuth2Server]', 'in grantTypeAllowed (clientId:', token.client.id, ', grantType:', `${scope})`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Logger.info signature in `@rocket.chat/logger` ==="
fd 'index.ts|logger.ts' packages/logger/src --type f | head -5
echo
# Search for info method definition
echo "=== Searching for info method signature ==="
rg -nA 3 'info\s*\(\s*' packages/logger/src --type ts | head -50
echo
echo "=== Multi-arg logger.info calls in oauth2-server/model.ts ==="
rg -nP 'logger\.info\s*\(' apps/meteor/server/oauth2-server/model.ts -A 1 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 1776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full context of lines 132-143 in model.ts ==="
sed -n '132,143p' apps/meteor/server/oauth2-server/model.ts
echo
echo "=== Full context of lines 171-186 in model.ts ==="
sed -n '171,186p' apps/meteor/server/oauth2-server/model.ts
echo
echo "=== Check for overloaded signatures in logger ==="
rg -B5 -A5 'interface.*Logger|class.*Logger' packages/logger/src/index.ts | head -60Repository: RocketChat/Rocket.Chat
Length of output: 984
logger.info called with unsupported argument arity.
The @rocket.chat/logger API signature is info(msg: object | string): void (single argument only). All multi-arg console-style calls throughout this file must be collapsed into a single string or object payload.
Fix pattern
- logger.info('[OAuth2Server]', 'in getClient (clientId:', clientId, ', clientSecret:', clientSecret, ')');
+ logger.info({
+ msg: '[OAuth2Server] in getClient',
+ clientId,
+ hasClientSecret: clientSecret != null,
+ });Applies to lines: 30-31, 43-44, 71-72, 132-143, 171-186
📝 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.
| logger.info('[OAuth2Server]', 'in grantTypeAllowed (clientId:', token.client.id, ', grantType:', `${scope})`); | |
| } | |
| logger.info({ | |
| msg: '[OAuth2Server] in grantTypeAllowed', | |
| clientId: token.client.id, | |
| grantType: scope, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/oauth2-server/model.ts` around lines 30 - 31, The logger
calls use console-style multi-argument signatures but `@rocket.chat/logger` only
accepts a single argument; update every multi-arg logger call in this file
(start with the call inside grantTypeAllowed where logger.info('[OAuth2Server]',
'in grantTypeAllowed (clientId:', token.client.id, ', grantType:', `${scope})`)
and also the other occurrences at the reported ranges) to pass a single string
or a single object payload (e.g., build a template literal or an object with
keys like context, clientId, grantType, token) so each logger.info/logger.warn
invocation has exactly one argument; scan the file for all logger.* calls with
multiple args and replace them with a single formatted string or object while
preserving the same data fields.
| } | ||
| } catch (e) { | ||
| console.log(colors.red('Error processing environment variable INITIAL_USER'), e); | ||
| logger.info(colors.red('Error processing environment variable INITIAL_USER'), e); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Logger.info signature and detect multi-arg usage in this file.
set -euo pipefail
echo "Checking Logger.info signature in packages/logger/src/index.ts"
rg -nP --type=ts '^\s*info\s*\(\s*msg:\s*object\s*\|\s*string\s*\)\s*:\s*void' packages/logger/src/index.ts
echo
echo "Scanning for logger.info calls with 2+ arguments in apps/meteor/server/startup/initialData.ts"
rg -nP --type=ts '\blogger\.info\s*\([^)]*,[^)]*\)' apps/meteor/server/startup/initialData.ts
echo
echo "Expected: signature match above + at least one multi-arg logger.info hit at/near Line 187."Repository: RocketChat/Rocket.Chat
Length of output: 527
🏁 Script executed:
# Let me also check the Logger interface/class directly to see all method signatures
echo "Checking full Logger interface structure:"
cat packages/logger/src/index.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 1869
🏁 Script executed:
# Check the exact code at line 187 and surrounding context in initialData.ts
echo "Checking context around line 187 in initialData.ts:"
sed -n '180,195p' apps/meteor/server/startup/initialData.tsRepository: RocketChat/Rocket.Chat
Length of output: 675
Fix invalid Logger.info arity at Line 187.
Logger.info accepts a single object | string argument, but this call passes two. This causes a type error. Additionally, use logger.error() for error conditions in catch blocks instead of info().
🔧 Proposed fix
- logger.info(colors.red('Error processing environment variable INITIAL_USER'), e);
+ logger.error({
+ message: colors.red('Error processing environment variable INITIAL_USER'),
+ error: e instanceof Error ? { name: e.name, message: e.message, stack: e.stack } : String(e),
+ });📝 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.
| logger.info(colors.red('Error processing environment variable INITIAL_USER'), e); | |
| logger.error({ | |
| message: colors.red('Error processing environment variable INITIAL_USER'), | |
| error: e instanceof Error ? { name: e.name, message: e.message, stack: e.stack } : String(e), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/startup/initialData.ts` at line 187, The current call
logger.info(colors.red('Error processing environment variable INITIAL_USER'), e)
passes two arguments and uses the wrong level; update the catch to call
logger.error with a single argument that includes both the colored message and
the error details (e.g. format/concatenate the message and e.stack or include an
object like { msg: colors.red('Error processing environment variable
INITIAL_USER'), error: e }) so the single-argument signature is respected;
locate the catch handling around INITIAL_USER processing and replace
logger.info(...) with the single-argument logger.error(...) call referencing
logger and INITIAL_USER.
| res.end('ok'); | ||
| } catch (err) { | ||
| console.error('Service not healthy', err); | ||
| logger.error('Service not healthy', err); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Find the Logger implementation ==="
fd -t f "index.ts" packages/logger/src/ 2>/dev/null | head -5
echo
echo "=== 2) Check Logger.error signature ==="
if [ -f "packages/logger/src/index.ts" ]; then
cat -n packages/logger/src/index.ts | head -100
fi
echo
echo "=== 3) Check the file and line in question ==="
if [ -f "ee/apps/omnichannel-transcript/src/service.ts" ]; then
wc -l ee/apps/omnichannel-transcript/src/service.ts
echo "Lines 30-45:"
sed -n '30,45p' ee/apps/omnichannel-transcript/src/service.ts | cat -n
fiRepository: RocketChat/Rocket.Chat
Length of output: 3045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 4) Search for logger.error usage patterns in codebase ==="
echo "Pattern 1: logger.error with object argument"
rg -n 'logger\.error\(\s*\{' ee/apps/omnichannel-transcript/src/ -A 2 | head -20
echo
echo "Pattern 2: logger.error with string + additional args"
rg -n 'logger\.error\([^{]' ee/apps/omnichannel-transcript/src/ -A 1 | head -30Repository: RocketChat/Rocket.Chat
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 5) Check actual Logger class definition in packages/logger ==="
find packages/logger/src -name "*.ts" -type f | xargs wc -l | sort -nRepository: RocketChat/Rocket.Chat
Length of output: 250
Fix invalid Logger.error call signature (extra argument).
At line 36, logger.error('Service not healthy', err) passes two arguments, but Logger.error accepts a single object | string parameter. This is a TypeScript correctness error.
Proposed fix
- logger.error('Service not healthy', err);
+ logger.error({ msg: 'Service not healthy', err });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ee/apps/omnichannel-transcript/src/service.ts` at line 36, The logger.error
call uses an invalid two-argument signature; update the call in service.ts where
logger.error('Service not healthy', err) is used to pass a single parameter
(either a single string with the error interpolated or a single object
containing both message and error), e.g., construct one object { message:
'Service not healthy', error: err } or a single formatted string and pass that
to logger.error so the call matches Logger.error(object | string).
| res.end('ok'); | ||
| } catch (err) { | ||
| console.error('Service not healthy', err); | ||
| logger.error('Service not healthy', err); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding Logger.error signature ==="
find . -name "index.ts" -path "*/packages/logger/*" -type f | head -5Repository: RocketChat/Rocket.Chat
Length of output: 135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking Logger.error definition ==="
rg -n "error" packages/logger/src/index.ts -A 3 -B 1 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking queue-worker logger.error at line 34 ==="
sed -n '30,40p' ee/apps/queue-worker/src/service.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking logger type in queue-worker/service.ts ==="
rg -n "logger\s*[:=]" ee/apps/queue-worker/src/service.ts | head -10Repository: RocketChat/Rocket.Chat
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Logger class implementation ==="
cat -n packages/logger/src/index.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 2500
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Examples of logger.error with object arguments ==="
rg -n "logger\.error\(\{" --type ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 2563
logger.error() receives invalid argument shape — expects single object | string, receives two arguments.
At line 34, logger.error('Service not healthy', err) violates the signature. The Logger.error method accepts a single parameter (object or string), not two separate arguments. Pass an object with both msg and err properties instead.
Fix
- logger.error('Service not healthy', err);
+ logger.error({ msg: 'Service not healthy', err });📝 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.
| logger.error('Service not healthy', err); | |
| logger.error({ msg: 'Service not healthy', err }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ee/apps/queue-worker/src/service.ts` at line 34, The logger.error call
currently passes two arguments; change it to pass a single argument (an object
or string) by replacing the two-argument call to logger.error with a single
object that contains both a message field (e.g., "Service not healthy") and the
caught error (err) so the call becomes logger.error with one object argument;
locate the occurrence of logger.error('Service not healthy', err) in service.ts
and update the call signature and any typing to match Logger.error(object |
string).
| logger.info('---- License enabled ----'); | ||
| logger.info(' url ->', JSON.stringify(serverUrls)); | ||
| logger.info(' periods ->', JSON.stringify(validPeriods)); | ||
| logger.info(' limits ->', JSON.stringify(limits)); | ||
| logger.info(' modules ->', modules.join(', ')); | ||
| logger.info('-------------------------'); |
There was a problem hiding this comment.
Logger.info only accepts a single argument; data will be silently discarded.
The Logger.info method signature is info(msg: object | string): void, accepting only one argument. The current calls pass two arguments (e.g., logger.info('url ->', JSON.stringify(serverUrls))), so only the label string is logged while the actual license data is ignored.
Proposed fix using template literals
- logger.info('---- License enabled ----');
- logger.info(' url ->', JSON.stringify(serverUrls));
- logger.info(' periods ->', JSON.stringify(validPeriods));
- logger.info(' limits ->', JSON.stringify(limits));
- logger.info(' modules ->', modules.join(', '));
- logger.info('-------------------------');
+ logger.info('---- License enabled ----');
+ logger.info(` url -> ${JSON.stringify(serverUrls)}`);
+ logger.info(` periods -> ${JSON.stringify(validPeriods)}`);
+ logger.info(` limits -> ${JSON.stringify(limits)}`);
+ logger.info(` modules -> ${modules.join(', ')}`);
+ logger.info('-------------------------');📝 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.
| logger.info('---- License enabled ----'); | |
| logger.info(' url ->', JSON.stringify(serverUrls)); | |
| logger.info(' periods ->', JSON.stringify(validPeriods)); | |
| logger.info(' limits ->', JSON.stringify(limits)); | |
| logger.info(' modules ->', modules.join(', ')); | |
| logger.info('-------------------------'); | |
| logger.info('---- License enabled ----'); | |
| logger.info(` url -> ${JSON.stringify(serverUrls)}`); | |
| logger.info(` periods -> ${JSON.stringify(validPeriods)}`); | |
| logger.info(` limits -> ${JSON.stringify(limits)}`); | |
| logger.info(` modules -> ${modules.join(', ')}`); | |
| logger.info('-------------------------'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ee/packages/license/src/showLicense.ts` around lines 25 - 30, The logger
calls pass two arguments but Logger.info accepts a single msg, so the data
(serverUrls, validPeriods, limits, modules) is dropped; update each call to
build a single string or object (e.g., use template literals or JSON objects)
and pass that single value to logger.info — change calls referencing
logger.info('url ->', JSON.stringify(serverUrls)), logger.info('periods ->',
JSON.stringify(validPeriods)), logger.info('limits ->', JSON.stringify(limits)),
and logger.info('modules ->', modules.join(', ')) to pass one combined message
that includes the label and the serialized data in the same argument.
1 participant
Proposed changes
Replace raw
console.log/warn/error/info/debugcalls with the project's structured@rocket.chat/logger(pino-based) across 120 production files.What this PR does
apps/meteor/server,apps/meteor/app/*/server,ee/apps/*)new Logger('ModuleName')from@rocket.chat/logger// eslint-disable-next-line no-consolecommentsshowBox.ts,settings.mocks.ts— left as-isESLint rule enabled
Uncommented the existing
no-console: 'warn'rule inpackages/eslint-configto prevent regressions.Why this matters
console.logis notlogger.error({ msg, err })captures stack traces in structured fields vs flat text fromconsole.error(string)no-console: warnlint rule ensures no newconsole.logcreeps in@rocket.chat/logger— this closes the gap for the remaining ~270 occurrencesHotspots addressed
apps/meteor/server/startup/initialData.ts(20 replacements)apps/meteor/app/error-handler/server/lib/RocketChat.ErrorHandler.ts(12 replacements)apps/meteor/app/utils/server/functions/getMongoInfo.ts(12 replacements)ee/apps/ddp-streamer/src/service.ts(11 replacements)apps/meteor/server/oauth2-server/model.ts(9 replacements)Summary by CodeRabbit
New Features
Bug Fixes
Chores