refactor: Convert missing JavaScript modules to TypeScript#38357
refactor: Convert missing JavaScript modules to TypeScript#38357
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughWidespread TypeScript migration and typings added across the Meteor app, test infra migrated to Jest, Storybook mock replaced with a typed TS mock, many converters/bridges refactored and typed, OAuth access-token login handler reintroduced, uploads/file-type detection made async, numerous JS→TS handler rewrites, and tests/mocks updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant LoginHandler as Accounts.registerLoginHandler
participant Registry as AccessTokenServices
participant Config as ServiceConfiguration
participant AccountsSvc as Accounts.updateOrCreateUserFromExternalService
participant DB as Database
Client->>LoginHandler: login({ accessToken, serviceName })
LoginHandler->>Registry: lookup handler for serviceName
alt handler found
LoginHandler->>Config: ensure ServiceConfiguration exists
LoginHandler->>Registry: invoke handler.handleAccessTokenRequest(options)
Registry-->>LoginHandler: { serviceData, options }
LoginHandler->>AccountsSvc: updateOrCreateUserFromExternalService(serviceName, serviceData, options)
AccountsSvc->>DB: create/update user
DB-->>AccountsSvc: user created/updated
AccountsSvc-->>LoginHandler: login result
LoginHandler-->>Client: success
else no handler
LoginHandler-->>Client: Meteor.Error (no registered oauth service)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38357 +/- ##
===========================================
+ Coverage 70.40% 70.62% +0.22%
===========================================
Files 3161 3168 +7
Lines 110639 112445 +1806
Branches 19870 20481 +611
===========================================
+ Hits 77895 79417 +1522
- Misses 30717 30843 +126
- Partials 2027 2185 +158
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "02/04 21:45 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 04 Feb 2026 21:45:41 GMT |
tassoevan
force-pushed
the
refactor/js-to-ts
branch
25 times, most recently
from
c8df5d1 to
7327b4b
Compare
…twitter}.js` to TypeScript
…d-responses.js` to TypeScript
…rver/lib/UAParserCustom.tests.js`
…ageReport.spec.ts` test to Jest - Replace proxyquire with jest.mock() for module mocking - Replace sinon stubs with jest.fn() mock functions - Convert all assertions to Jest matchers - Remove file from Mocha configuration
…server/transform_helpers.tests.js`
…ds/helper.tests.js`
…essages.tests.js`
tassoevan
force-pushed
the
refactor/js-to-ts
branch
from
e5b1ff1 to
ef173c9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (15)
apps/meteor/app/utils/lib/getURL.spec.ts (1)
106-113:⚠️ Potential issue | 🟡 MinorFix typo in test descriptions: "fase" → "false".
Lines 106 and 111 contain the typo "fase" instead of "false" in the test descriptions.
✏️ Proposed fix
if (!options.full && !options.cdn) { - it('should return with cloud host if full: fase and cdn: false', () => { + it('should return with cloud host if full: false and cdn: false', () => { testPaths(options, (path) => getCloudUrl(_site_url, path)); }); } } else if (!options.full && !options.cdn) { - it('should return with cloud host if full: fase and cdn: false', () => { + it('should return with cloud host if full: false and cdn: false', () => { testPaths(options, (path) => getCloudUrl(_site_url, path)); }); }apps/meteor/app/irc/server/irc-bridge/peerHandlers/disconnected.ts (1)
23-28:⚠️ Potential issue | 🟡 MinorInconsistent status value representation.
Line 23 correctly uses
UserStatus.OFFLINE, but line 28 still uses the string literal'offline'in thenotifyOnUserChangecall. For consistency and type safety, both should use the same representation.Proposed fix
- void notifyOnUserChange({ id: user._id, clientAction: 'updated', diff: { status: 'offline' } }); + void notifyOnUserChange({ id: user._id, clientAction: 'updated', diff: { status: UserStatus.OFFLINE } });apps/meteor/definition/externals/meteor/ddp-common.d.ts (1)
6-19:⚠️ Potential issue | 🟠 MajorMissing
MethodInvocationinstance type definition.The constructor signature returns
MethodInvocation(line 16), but no corresponding instance type is defined. Only thelet MethodInvocation: MethodInvocationConstructorvariable is declared (line 19). In TypeScript, the return type annotation refers to a TYPE that must represent the instance shape. Without aninterface MethodInvocation { ... }, TypeScript cannot properly type instances created vianew DDPCommon.MethodInvocation(...).Suggested fix
+ interface MethodInvocation { + userId: string | null; + isSimulation: boolean; + connection: { + id: string; + close: () => void; + clientAddress: string; + httpHeaders: Record<string, any>; + }; + setUserId(userId: string | null): void; + unblock(): void; + } + interface MethodInvocationConstructor { new (options: { connection: { id: string; close: () => void; clientAddress: string; httpHeaders: Record<string, any>; }; isSimulation?: boolean; userId?: string; }): MethodInvocation; } let MethodInvocation: MethodInvocationConstructor;apps/meteor/app/lib/server/functions/parseUrlsInMessage.ts (1)
8-29:⚠️ Potential issue | 🟡 MinorMissing return statement leaves function returning
undefined.The function signature implies it should return the modified message, but there's no explicit
return message;at the end. After the delete operations on lines 27-28, the function falls through without returning anything. This appears unintentional given the early return on line 10.Proposed fix
message.msg = message.html || message.msg; delete message.html; delete message.tokens; + + return message; };apps/meteor/app/markdown/lib/parser/original/code.ts (1)
46-55:⚠️ Potential issue | 🟠 MajorRace condition:
register(lang)is async but not awaited, causinghljs.highlight()to execute before language registration completes.The
registerfunction (hljs.ts line 11) is async and dynamically imports language modules, butvoid register(lang)discards the promise. This meanshljs.highlight(lang, code)on line 49 may execute before the language is registered, causing incorrect highlighting or errors that fall back tohljs.highlightAuto().Resolve by awaiting registration. This requires making the IIFE async:
Proposed fix
- const result = (() => { + const result = (async () => { if (lang) { try { - void register(lang); + await register(lang); return hljs.highlight(lang, code); } catch (error) { console.error(error); } } return hljs.highlightAuto(lang + code); - })(); + })();Note: The parent
codeblocksfunction would need to become async as well to properly handle the promise.apps/meteor/ee/app/api-enterprise/server/lib/canned-responses.ts (2)
105-111:⚠️ Potential issue | 🔴 CriticalPreserve authorization constraints when
departmentIdis provided.Overwriting
extraFilterremoves the permission-based$orand can allow querying departments the user should not access. Keep the auth filter and adddepartmentIdas an additional AND constraint.🔒 Suggested fix
- if (departmentId) { - extraFilter = [ - { - departmentId, - }, - ]; - } + if (departmentId) { + extraFilter = [...extraFilter, { departmentId }]; + }
113-119:⚠️ Potential issue | 🟠 MajorGuard
textFiltercreation to avoid runtime errors.
textis optional, buttext!forces a non-null assertion and can throw when omitted. Create the regex only whentextis present.✅ Suggested fix
- const textFilter = new RegExp(escapeRegExp(text!), 'i'); + const textFilter = text ? new RegExp(escapeRegExp(text), 'i') : undefined; - ...(text ? [{ $or: [{ shortcut: textFilter }, { text: textFilter }] }] : []), + ...(textFilter ? [{ $or: [{ shortcut: textFilter }, { text: textFilter }] }] : []),apps/meteor/app/google-oauth/server/index.ts (1)
31-53:⚠️ Potential issue | 🟡 MinorRename local
escapehelper to avoid shadowing the globalescape.Biome flags this as a variable shadowing issue in the recommended rules. The local function shadows the global
escape()function, which can confuse readers and debuggers. Rename toescapeHtmlacross all 4 usages within the config object (lines 48-51).Proposed fix
- const escape = (s: string | undefined): string | undefined => { + const escapeHtml = (s: string | undefined): string | undefined => { if (!s) { return s; } @@ - credentialToken: escape(options.credentialToken), - credentialSecret: escape(options.credentialSecret), - storagePrefix: escape((OAuth as any)._storageTokenPrefix), - redirectUrl: escape(options.redirectUrl), + credentialToken: escapeHtml(options.credentialToken), + credentialSecret: escapeHtml(options.credentialSecret), + storagePrefix: escapeHtml((OAuth as any)._storageTokenPrefix), + redirectUrl: escapeHtml(options.redirectUrl),apps/meteor/app/irc/server/irc-bridge/peerHandlers/sentMessage.ts (1)
50-59:⚠️ Potential issue | 🔴 CriticalGuard against missing
recipientNickto avoid incorrect user lookup.The function accepts both
roomNameandrecipientNickas optional parameters. WhenroomNameis absent, it queries users byrecipientNickwithout validating it is defined. IfrecipientNickis undefined or empty, the MongoDB query{'profile.irc.nick': undefined}can match unintended users (those without an IRC nick set), risking messages sent to the wrong recipient. Add an explicit guard before querying.Proposed fix
} else { + if (!args.recipientNick) { + throw new Error('recipientNick is required when roomName is not provided'); + } const recipientUser = await Users.findOne({ 'profile.irc.nick': args.recipientNick, });apps/meteor/app/authentication/server/startup/index.ts (1)
112-136:⚠️ Potential issue | 🟡 MinorGuard email template placeholders when
user.nameis missing.
IUser.nameis optional, but lines 113, 128, and 136 assume it always exists via type assertionas IUser & { name: string }. If missing at runtime,safeHtmlDotswill throw a TypeError when callingtext.replace(), andMailer.replacewill render the literal string"undefined"in emails. Use the nullish coalescing operator as shown in theenrollAccount.htmlhandler (line 150):🛠️ Suggested fix
Accounts.emailTemplates.verifyEmail.html = function (userModel, url) { - const name = safeHtmlDots((userModel as IUser & { name: string }).name); + const name = safeHtmlDots((userModel as IUser).name ?? ''); return Mailer.replace(verifyEmailTemplate, { Verification_Url: url, name }); }; Accounts.emailTemplates.resetPassword.subject = function (userModel) { return Mailer.replace(settings.get<string>('Forgot_Password_Email_Subject') || '', { - name: (userModel as IUser & { name: string }).name, + name: (userModel as IUser).name ?? '', }); }; Accounts.emailTemplates.resetPassword.html = function (userModel, url) { return Mailer.replacekey( Mailer.replace(resetPasswordTemplate, { - name: (userModel as IUser & { name: string }).name, + name: (userModel as IUser).name ?? '', }), 'Forgot_Password_Url', url, ); };apps/meteor/app/custom-oauth/server/custom_oauth_server.ts (1)
362-413:⚠️ Potential issue | 🟡 MinorPreserve the original error as a cause.
Lines 371, 384, 398, and 411 pass
error.messageas a second argument to the Error constructor, which is ignored. The Error constructor accepts only one parameter, so the original error context is lost. Use error cause with{ cause: error }instead to preserve the error chain.🛠️ Suggested fix (apply to all four catch blocks)
- } catch (error: any) { - throw new Error('CustomOAuth: Failed to extract username', error.message); + } catch (error: any) { + throw new Error('CustomOAuth: Failed to extract username', { cause: error }); }apps/meteor/ee/server/apps/orchestrator.ts (1)
262-299:⚠️ Potential issue | 🟠 MajorGuard missing
installedAtbefore computing upgrade date.Line 293 uses
stat.installedAt!without checking if it exists. ThefindInstallationDates()method returnsPromise<Pick<IStats, 'version' | 'installedAt'>[]>, andinstalledAtis optional inIStats(defined asinstalledAt?: string). IfinstalledAtis undefined,new Date()creates an Invalid Date andtoISOString()throws. Line 274 already handles this case with|| 0, but line 293 doesn't. Add a guard to skip entries withoutinstalledAt.Suggested fix
- if (hadPreTargetVersion && majorVersion >= targetVersion) { - upgradeToV7Date = new Date(stat.installedAt!); + if (hadPreTargetVersion && majorVersion >= targetVersion) { + if (!stat.installedAt) { + continue; + } + upgradeToV7Date = new Date(stat.installedAt); this._rocketchatLogger.info({ msg: 'Found upgrade to target version date', targetVersion, upgradeToDate: upgradeToV7Date.toISOString(), }); break; }apps/meteor/app/apps/server/converters/messages.ts (2)
111-149:⚠️ Potential issue | 🟠 MajorDon't delete
message.ubefore the fallback conversion.
message.uis deleted at line 141 before the fallback call at line 148, so the fallback always receivesundefinedand cannot recover a sender. Preserve the original value until after the fallback, then delete. Also remove the inline FIXME comments in this block once corrected, as they violate the coding guideline to avoid code comments in the implementation.🔧 Suggested fix
sender: async (message: IMessage) => { if (!message.u?._id) { return undefined; } // When the message contains token, means the message is from the visitor(omnichannel) - const user = await (isMessageFromVisitor(msgObj) - ? cache.get('user.convertToApp')(message.u) - : cache.get('user.convertById')(message.u._id)); - - delete (message as any).u; // FIXME the property is used right after, so we can't delete it before, what??? + const originalUser = message.u; + const user = await (isMessageFromVisitor(msgObj) + ? cache.get('user.convertToApp')(originalUser) + : cache.get('user.convertById')(originalUser._id)); + + delete (message as any).u; /** * Old System Messages from visitor doesn't have the `token` field, to not return * `sender` as undefined, so we need to add this fallback here. */ - return user || cache.get('user.convertToApp')(message.u); + return user || cache.get('user.convertToApp')(originalUser); },
197-204:⚠️ Potential issue | 🟠 MajorAdd null guard for editor lookup to prevent crash.
Users.findOneById()returnsPromise<IUser | null>, so the non-null assertioneditor!will crash if the user no longer exists. The similar sender lookup above uses proper null guarding (if (user)); apply the same pattern here with fallback tomessage.editor.idandmessage.editor.username.🔧 Suggested fix
let editedBy; if (message.editor) { const editor = await Users.findOneById(message.editor.id); - editedBy = { - _id: editor!._id, - username: editor!.username, - }; + if (editor) { + editedBy = { + _id: editor._id, + username: editor.username, + }; + } else { + editedBy = { + _id: message.editor.id, + username: message.editor.username, + }; + } }apps/meteor/app/apps/server/converters/rooms.ts (1)
146-166:⚠️ Potential issue | 🟠 MajorGuard missing
visitorChannelInfobefore destructuring.
visitorChannelInfois optional in theILivechatRoominterface and has no null check before being destructured with a non-null assertion. Use a safe default instead.🔧 Suggested fix
- const { lastMessageTs, phone } = visitorChannelInfo!; + const { lastMessageTs, phone } = visitorChannelInfo ?? {};
🤖 Fix all issues with AI agents
In `@apps/meteor/app/apps/server/converters/messages.ts`:
- Around line 341-345: The timestamp converter in messages.ts uses a non-null
assertion on attachment.ts causing Invalid Date when undefined; update the
timestamp function (the converter for MessageAttachment) to first check if
attachment.ts is defined (e.g., a ternary or if) and only call new
Date(attachment.ts) when present, otherwise return undefined (matching the
threads.ts sibling), and preserve the delete attachment.ts behavior when ts
exists; adjust the return type if necessary to match other converters.
In `@apps/meteor/app/apps/server/converters/rooms.ts`:
- Around line 18-20: convertByName currently uses a non-null assertion on
IRoom['name'] when calling Rooms.findOneByName; add a guard clause at the start
of convertByName to handle undefined roomName (e.g., if (!roomName) return
undefined) and remove the non-null assertion so Rooms.findOneByName is only
called with a real string; update convertByName to explicitly return undefined
for missing names and proceed with the lookup when roomName is present.
In `@apps/meteor/app/apps/server/converters/users.ts`:
- Around line 16-18: The convertByUsername function currently uses a non-null
assertion on IUser['username'] and may pass undefined into
Users.findOneByUsername; update convertByUsername to explicitly guard for
undefined by checking the username parameter at the top (e.g., if (!username)
return undefined or handle error) and remove the non-null assertion so you only
call Users.findOneByUsername(username) when username is a defined string;
reference convertByUsername and Users.findOneByUsername in your change.
In `@apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts`:
- Around line 43-45: The call to decodeURIComponent when computing fileId inside
the WebApp.connectHandlers.use callback can throw a URIError for malformed
percent-encoding; wrap the decodeURIComponent(...) that defines fileId in a
try/catch, catch URIError (and other decoding errors), and return a 400 response
(set statusCode 400 and end with a short error message) instead of allowing the
error to bubble to a 500; update the handler around the fileId computation in
this connectHandlers callback so subsequent logic only runs when decoding
succeeds.
- Line 86: Replace the non-null assertion on file.contentType when calling
res.setHeader: instead of using file.contentType! pass a safe fallback value
(e.g. 'application/octet-stream') so that res.setHeader('Content-Type', ...)
never receives undefined; locate the call to res.setHeader and update it to use
file.contentType ?? 'application/octet-stream' (or equivalent) to match the
FileSystem implementation.
In `@apps/meteor/app/file/server/file.server.ts`:
- Around line 25-27: The interface methods createWriteStream and
createReadStream currently return unknown; change their return types to the
appropriate Node stream base types so callers can use .on()/.pipe(): update the
interface signature for createWriteStream(fileName: string, contentType: string)
to return NodeJS.WritableStream and createReadStream(fileName: string) to return
NodeJS.ReadableStream; ensure this matches existing implementations (GridFS
returning GridFSBucketWriteStream/GridFSBucketReadStream and FileSystem
returning fs.WriteStream/fs.ReadStream) so no behavioral changes are needed.
In `@apps/meteor/app/importer/server/startup/setImportsToInvalid.ts`:
- Line 10: The conditional is using an unnecessary cast and array includes
check: locate the check that references lastOperation and ProgressStep (the line
containing [ProgressStep.USER_SELECTION].includes(lastOperation.status as any))
and replace it with a direct equality comparison against the enum (compare
lastOperation.status === ProgressStep.USER_SELECTION) and remove the as any cast
so the type system validates the enum value directly.
In `@apps/meteor/app/irc/server/irc-bridge/localHandlers/onCreateUser.ts`:
- Line 41: The forEach callback currently uses an implicit return by writing
rooms.forEach((room: any) => this.sendCommand('joinedChannel', { room, user })),
which the linter flags; change the callback to a block-bodied arrow function to
avoid returning a value (e.g., use rooms.forEach((room: any) => {
this.sendCommand('joinedChannel', { room, user }); })), ensuring the call to
this.sendCommand in the onCreateUser handler remains unchanged but is executed
without an implicit return.
In `@apps/meteor/app/lib/server/functions/notifications/email.ts`:
- Around line 237-244: The Reply-To header construction uses
settings.get('Direct_Reply_Separator') directly in split(), which can be
undefined or empty; read the separator into a local const (e.g., const sep =
(settings.get('Direct_Reply_Separator') as string) || '+'), ensure sep is
non-empty (fallback to a safe default like '+'), then use sep for both the
left-side split and when concatenating the message id in the
email.headers['Reply-To'] assignment (replace direct uses of
settings.get('Direct_Reply_Separator') and the inline split with the new sep
variable) so you never call split with undefined or an empty string and the
Reply-To remains well-formed using message.tmid || message._id.
In `@apps/meteor/app/lib/server/lib/interceptDirectReplyEmails.ts`:
- Around line 137-138: The isActive() method incorrectly treats the helper as
active because stop() never sets the running flag to false and callback typing
was weakened; update stop() to set this.running = false to properly deactivate,
and replace usages of undefined as any for the stored callback with an optional
parameter type (e.g., callback?: ...) and adjust the code paths that
assign/clear this.callback (e.g., in start(), stop(), and any setter) so the
callback is set when starting and cleared when stopping; ensure isActive()
continues to read this.running !== false and that related logic around methods
referenced (isActive, stop, start, running, callback) is updated accordingly.
In `@apps/meteor/app/lib/server/oauth/twitter.ts`:
- Around line 25-26: In the catch block that throws the OAuth error (the code
creating the Error with message "Failed to fetch identity from Twwiter.
${err.message}"), fix the typo by changing "Twwiter" to "Twitter" so the
thrown/logged message reads "Failed to fetch identity from Twitter.
${err.message}"; update this string in the catch inside
apps/meteor/app/lib/server/oauth/twitter.ts (the error thrown when failing to
fetch identity).
- Around line 51-55: The current whitelisting loop drops falsy but valid values
because it uses a truthy check; update the loop that builds fields from
whitelistedFields (the block referencing fields, whitelistedFields, and
identity) to test property existence instead (e.g., use "key in identity" or
Object.prototype.hasOwnProperty.call(identity, key)) so empty strings, 0, false,
etc. are preserved when assigning fields[key] = identity[key].
In `@apps/meteor/app/lib/server/startup/rateLimiter.ts`:
- Around line 143-164: The rate-limit slowdown is ineffective because the async
callback (callback) that awaits sleep() is invoked via rule._executeCallback()
from check() without awaiting its Promise; update the call sites where
rule._executeCallback(...) is invoked (the two places that call it from check())
to await the returned Promise and make check() async (propagate async/await) so
the sleep actually blocks, or alternatively convert the callback to a
synchronous path (remove async/sleep) if making check() async is
undesirable—ensure consistency between callback (function callback) and its
invoker rule._executeCallback() so the slowdown delay is awaited.
In `@apps/meteor/app/mentions/lib/MentionsParser.spec.ts`:
- Around line 87-97: The three skipped tests in the getChannelMentions describe
block incorrectly call mentionsParser.getUserMentions; update each skipped spec
to call mentionsParser.getChannelMentions instead (the tests titled 'should
return without the "." from "#general."', 'should return without the "_" from
"#general_"', and 'should return without the "-" from "#general."') so that when
unskipped they exercise the correct function; keep the rest of each expectation
the same.
In `@apps/meteor/definition/externals/meteor/google-oauth.d.ts`:
- Around line 2-12: The module declares Google and references LoginError but
LoginError is missing; fix by adding a local type/import for LoginError in this
declaration file—either declare type LoginError = Meteor.Error (or import Meteor
and alias Meteor.Error as LoginError) or define the LoginError shape expected by
signIn; update the google-oauth.d.ts module so the referenced LoginError type is
defined alongside the existing Google, requestCredential, and signIn symbols.
In `@apps/meteor/tests/unit/app/apps/server/mocks/models/BaseModel.mock.ts`:
- Around line 4-6: findOneById declares return type T | null but currently
returns this.data[id] which yields undefined when not present; update the
function to be consistent by either changing its signature to return T |
undefined or by explicitly returning null for missing entries (e.g., in
findOneById use a null coalescing behavior to return this.data[id] ?? null).
Ensure references to the data property and the findOneById method are updated so
callers and types align.
In `@apps/meteor/tests/unit/app/apps/server/mocks/models/Messages.mock.ts`:
- Around line 5-6: Override of BaseModelMock's property "data" in class
MessagesMock currently lacks visibility and becomes public; change the
declaration in MessagesMock to explicitly use the protected modifier (i.e., make
"protected data = { ... }") so it matches BaseModelMock's protected visibility
and preserves encapsulation for the IMessage data.
🧹 Nitpick comments (38)
apps/meteor/app/lib/server/startup/robots.ts (1)
9-9: Remove the inline comment to comply with implementation comment ban.The
/* , next*/comment violates the “avoid code comments in the implementation” guideline; it can be removed with no behavior change.As per coding guidelines “Avoid code comments in the implementation”.🧹 Proposed change
-return WebApp.connectHandlers.use('/robots.txt', (_req: IncomingMessage, res: ServerResponse /* , next*/): void => { +return WebApp.connectHandlers.use('/robots.txt', (_req: IncomingMessage, res: ServerResponse): void => {apps/meteor/app/importer/server/startup/store.ts (1)
12-16: Redundant double call tosettings.get.The setting is retrieved twice: once for the null check and again to store in
filePath. This can be simplified to a single call.♻️ Suggested simplification
- if (settings.get<string>('ImportFile_FileSystemPath') != null) { - const filePath = settings.get<string>('ImportFile_FileSystemPath'); - if (typeof filePath === 'string' && filePath.trim() !== '') { - path = filePath; - } + const filePath = settings.get<string>('ImportFile_FileSystemPath'); + if (typeof filePath === 'string' && filePath.trim() !== '') { + path = filePath; }apps/meteor/app/irc/server/irc-bridge/peerHandlers/userRegistered.ts (2)
26-26: Inconsistent status value representation.Line 26 uses the string literal
'online', line 54 usesUserStatus.ONLINE, and line 62 uses'online'again. For consistency and type safety, consider using theUserStatusenum throughout.♻️ Suggested fix for consistency
const userToInsert = { name: args.nick, username: `${args.username}-irc`, - status: 'online', + status: UserStatus.ONLINE, utcOffset: 0,- void notifyOnUserChange({ id: user._id, clientAction: 'updated', diff: { status: 'online' } }); + void notifyOnUserChange({ id: user._id, clientAction: 'updated', diff: { status: UserStatus.ONLINE } });Also applies to: 54-54, 62-62
40-41:as anyassertion weakens type safety.The
as anycast bypasses TypeScript checks. This is understandable for a migration sinceuserToInsertis a partial user object, but consider creating a proper input type forUsers.create()or using a more specific assertion likeas Parameters<typeof Users.create>[0]if available.apps/meteor/app/mailer/server/api.ts (1)
34-34: Minor type inconsistency across related functions.The
replaceandreplaceEscapedfunctions now useRecord<string, any>, whilewrap(line 73) andsend(line 199) still use{ [key: string]: unknown }. This works correctly sinceunknownis assignable toany, but consider aligning all these functions to use the same type for consistency.For a migration PR,
Record<string, any>is acceptable, thoughRecord<string, unknown>would provide stronger type safety if you revisit this later.Also applies to: 57-57
apps/meteor/app/markdown/lib/hljs.ts (1)
10-11: Avoid inline eslint-disable; refactor or move suppression to config.The added
// eslint-disable-next-line complexityviolates the “no comments in implementation” guideline. Prefer refactoring to reduce complexity or apply an eslint override in configuration for this file instead of an inline comment. As per coding guidelines: Avoid code comments in the implementation.apps/meteor/app/statistics/server/functions/sendUsageReport.spec.ts (1)
5-7: Consider making the mock logger more robust.The
mockLoggeronly implementserror, butLoggerhas many methods (info,warn,debug,log, etc.). IfsendUsageReportcalls any other method, the test will fail at runtime with an undefined function error.♻️ Suggested improvement
const mockLogger: Logger = { error: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + debug: jest.fn(), + log: jest.fn(), } as any;Alternatively, consider using a utility to create a complete mock:
const mockLogger = { error: jest.fn(), info: jest.fn(), warn: jest.fn(), debug: jest.fn(), log: jest.fn(), startup: jest.fn(), success: jest.fn(), method: jest.fn(), subscription: jest.fn(), fatal: jest.fn(), } as unknown as Logger;apps/meteor/app/lib/server/functions/validateCustomFields.ts (1)
28-33:customFieldsis populated but never used.The
customFieldsobject is built inside the loop but is never returned or used after the function validates the fields. This appears to be dead code carried over from the original JS.♻️ Suggested removal of dead code
- const customFields: Record<string, any> = {}; - Object.keys(customFieldsMeta).forEach((fieldName) => { const field = customFieldsMeta[fieldName]; - - customFields[fieldName] = fields[fieldName]; const fieldValue = trim(fields[fieldName]);apps/meteor/app/irc/server/irc-bridge/peerHandlers/leftChannel.ts (1)
10-10: Consider stronger typing forthiscontext in future.Using
this: anyworks for this migration, but for better type safety, consider defining an interface for the IRC bridge context (with thelogmethod) and using it across all peer handlers. This is optional and can be deferred.apps/meteor/app/lib/server/functions/notifications/mobile.ts (1)
37-45: Well-structured parameter types with proper null safety.The inline type definition is clear and the
AtLeastgenerics ensure required fields are present. The|| ''fallbacks on lines 63 and 74 correctly handle cases whereusernamemay be undefined.One optional consideration: the
receivertype requires fields (settings,active,emails,status,statusConnection) that aren't accessed in the function body—onlylanguageandusernameare used. You could narrow this toAtLeast<IUser, 'language' | 'username'>for minimal requirements, though the current broader type likely matches callers' object shapes.apps/meteor/app/irc/server/irc-bridge/localHandlers/onSaveMessage.ts (1)
15-21: Consider narrowing theas anycasts with a type guard or interface extension.The repeated
(userData as any).profilecasts work but are fragile. A small type guard or local interface for the IRC profile shape would improve maintainability:interface IrcProfile { irc?: { nick?: string } } const profile = (userData as { profile?: IrcProfile }).profile;However, this is a minor readability nit for a migration PR; the current approach is functionally correct.
apps/meteor/app/irc/server/irc-bridge/localHandlers/onLogin.ts (1)
44-50: Consider typingroomasIRoomfor better type safety.The
room: anycast loses type safety. SinceIRoomis available from@rocket.chat/core-typings, consider:- rooms.forEach((room: any) => { + rooms.forEach((room: IRoom) => {This is a minor improvement for a migration PR.
apps/meteor/app/irc/server/irc-bridge/index.ts (2)
192-194: Consider preserving error type information.Casting the error to
anyloses type information. Consider logging with more context:} catch (e) { - this.logQueue(e as any); + this.logQueue({ msg: 'Error processing command', error: String(e) }); }Or update
logQueueto acceptunknownfor error cases. This is a minor improvement.
153-158: Non-null assertion oninitTimeis safe but could be guarded.The
this.initTime!assertion is safe becauseinit()sets it beforerunQueue()is called. However, adding a guard would be more defensive:const lastResetTime = await Settings.findOneById('IRC_Bridge_Reset_Time'); - if (lastResetTime?.value && lastResetTime.value > this.initTime!) { + if (lastResetTime?.value && this.initTime && lastResetTime.value > this.initTime) {This is optional as the current flow guarantees
initTimeis set.apps/meteor/app/markdown/lib/parser/original/markdown.ts (1)
34-38: Consider narrowingany[]for better type safety.The
replaceFunction: (...args: any[]) => stringsignature loses type information. While this is acceptable for regex replace callbacks where argument counts vary, you could consider typing it more precisely based on the actual regex groups used.apps/meteor/app/markdown/lib/markdown.ts (1)
73-81: Type annotation could be more precise for the options parameter.The
{ ...options }spread with no type annotation loses type information. Consider adding explicit typing to ensure callers pass valid options:Proposed improvement
export const createMarkdownMessageRenderer = - ({ ...options }) => + (options: { supportSchemesForLink?: string; headers?: boolean; rootUrl?: string }) => (message: { html: string; tokens?: Token[] }) => {apps/meteor/app/lib/server/functions/notifications/email.ts (1)
230-233: Redundant length check.The expression
sender.emails?.length && sender.emails.length > 0is redundant. Ifsender.emails?.lengthis truthy, it's already greater than 0.♻️ Simplified check
- if (sender.emails?.length && sender.emails.length > 0 && settings.get('Add_Sender_To_ReplyTo')) { + if (sender.emails?.length && settings.get('Add_Sender_To_ReplyTo')) {apps/meteor/app/irc/server/servers/RFC2813/parseMessage.ts (1)
38-47: Non-null assertions could cause runtime errors on malformed input.The non-null assertions at lines 39-40 (
match![1]) will throw ifmatchis null (empty or whitespace-only line after prefix removal). While this may represent existing behavior from the JS version, consider adding a guard or returning early for robustness.♻️ Optional defensive check
// Parse command match = line.match(/^([^ ]+) */); + if (!match) { + throw new Error(`Malformed IRC message: no command found in "${line}"`); + } - message.command = match![1] as keyof typeof peerCommandHandlers; - message.rawCommand = match![1]; + message.command = match[1] as keyof typeof peerCommandHandlers; + message.rawCommand = match[1];apps/meteor/app/irc/server/servers/RFC2813/index.ts (2)
150-152: Non-null assertion on socket may cause runtime error.
write()is a public method that could be called beforeregister()initializes the socket. Consider adding a guard or returningfalseif socket is undefined.♻️ Optional defensive check
- return this.socket!.write(`${buffer}\r\n`); + if (!this.socket) { + this.log({ msg: 'Cannot write: socket not initialized' }); + return false; + } + return this.socket.write(`${buffer}\r\n`);
168-168: Consider removing the control character from the regex.Similar to
localCommandHandlers.ts, this regex contains a control character (\u0007) which Biome flags. Apply the same normalization pattern for consistency.As per coding guidelines: Avoid code comments in the implementation.
♻️ Suggested fix
- const lines = this.receiveBuffer.toString().split(/\r\n|\r|\n|\u0007/); // eslint-disable-line no-control-regex + const bufferStr = this.receiveBuffer.toString().replace(/\u0007/g, '\n'); + const lines = bufferStr.split(/\r\n|\r|\n/);packages/apps-engine/src/definition/settings/ISetting.ts (1)
24-25: Avoid introducing new inline comments.If we’re adhering to the no-comments guideline, please drop the newly added JSDoc line and keep the type-only change.
♻️ Suggested edit
- /** The group this setting belongs to. */ group?: string;As per coding guidelines, "Avoid code comments in the implementation".
apps/meteor/app/apps/server/converters/settings.ts (1)
6-6: Remove commented‑out import.This keeps the implementation comment‑free and avoids dead code noise.
♻️ Suggested edit
-// import { exhaustiveCheck } from '../../../../lib/utils/exhaustiveCheck';As per coding guidelines, "Avoid code comments in the implementation".
apps/meteor/app/mentions/server/Mentions.spec.ts (1)
22-34: Remove inline comments from the fixture setup.Guidelines ask to avoid code comments; these can be expressed directly in code.
Proposed cleanup
- // return RocketChat.models.Rooms.find({ name: {$in: _.unique(channels)}, t: 'c' }, { fields: {_id: 1, name: 1 }}).fetch(); getUser: async (userId) => ({ _id: userId, language: 'en' }) as any, - getTotalChannelMembers: async (/* rid*/) => 2, - onMaxRoomMembersExceeded: async () => { - /* do nothing */ - }, + getTotalChannelMembers: async (_rid) => 2, + onMaxRoomMembersExceeded: async () => undefined,As per coding guidelines: “Avoid code comments in the implementation”.
apps/meteor/app/lib/server/startup/rateLimiter.ts (1)
13-13: Consider adding explicit radix toparseInt.While
parseInttypically handles numeric strings correctly, explicitly specifying the radix is a best practice to avoid edge cases.🔧 Suggested fix
-const slowDownRate = parseInt(process.env.RATE_LIMITER_SLOWDOWN_RATE || '0'); +const slowDownRate = parseInt(process.env.RATE_LIMITER_SLOWDOWN_RATE || '0', 10);apps/meteor/app/lib/server/lib/interceptDirectReplyEmails.ts (1)
1-2: Prefer a minimal ambient type instead of@ts-expect-error.Line 1 suppresses typing and adds an implementation comment. Consider adding a minimal ambient declaration for
@rocket.chat/poplib(or installing official types) and remove the directive. As per coding guidelines: Avoid code comments in the implementation.♻️ Proposed change
-// `@ts-expect-error` - no types available for `@rocket.chat/poplib` import POP3Lib from '@rocket.chat/poplib';Add a declaration file (e.g.,
types/@rocket.chat__poplib.d.ts):declare module '@rocket.chat/poplib';apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts (1)
43-43: Remove the inline commentednextparameter.
It’s dead commentary in implementation code.🧹 Suggested cleanup
- return WebApp.connectHandlers.use('/custom-sounds/', async (req: IncomingMessage, res: ServerResponse /* , next*/) => { + return WebApp.connectHandlers.use('/custom-sounds/', async (req: IncomingMessage, res: ServerResponse) => {As per coding guidelines: Avoid code comments in the implementation.
apps/meteor/app/authentication/server/startup/index.ts (1)
155-205: Remove newly added inline comments/TODO to align with the no-comment guideline.Consider relying on naming/structure instead of inline comments and move the TODO to an issue tracker.
🧹 Proposed cleanup
const getLinkedInName = ({ firstName, lastName }: LinkedInName): string => { - // Check if it's the old format (simple strings) if (typeof firstName === 'string' && typeof lastName === 'string') { return lastName ? `${firstName} ${lastName}` : firstName; } - // LinkedIn new format (objects with localized data) if (typeof firstName === 'object' && typeof lastName === 'object') { const { preferredLocale, localized: firstNameLocalized } = firstName; const { localized: lastNameLocalized } = lastName; if (preferredLocale && firstNameLocalized && preferredLocale.language && preferredLocale.country) { const locale = `${preferredLocale.language}_${preferredLocale.country}`; if (firstNameLocalized[locale] && lastNameLocalized[locale]) { return `${firstNameLocalized[locale]} ${lastNameLocalized[locale]}`; } if (firstNameLocalized[locale]) { return firstNameLocalized[locale]; } } } - // Fallback: return empty string or first available value return typeof firstName === 'string' ? firstName : ''; }; @@ - // TODO I believe this find is unnecessary user = (await Users.findOne({ _id, })) as Meteor.User & { globalRoles?: string[] };As per coding guidelines: Avoid code comments in the implementation.
Also applies to: 362-366
apps/meteor/app/apps/server/converters/threads.ts (2)
12-14: Redundant assignment in constructor.The parameter property
private readonly orchalready handles the assignment. The explicitthis.orch = orchin the constructor body is unnecessary.♻️ Suggested fix
- constructor(private readonly orch: IAppServerOrchestrator) { - this.orch = orch; - } + constructor(private readonly orch: IAppServerOrchestrator) {}
42-46: Consider extracting repeated converter access.
this.orch.getConverters().get('users')is called three times. Extracting to a local variable would improve readability.♻️ Suggested improvement
+ const usersConverter = this.orch.getConverters().get('users'); - const convertToApp = cachedFunction(this.orch.getConverters().get('users').convertToApp.bind(this.orch.getConverters().get('users'))); - - const convertUserById = cachedFunction(this.orch.getConverters().get('users').convertById.bind(this.orch.getConverters().get('users'))); + const convertToApp = cachedFunction(usersConverter.convertToApp.bind(usersConverter)); + const convertUserById = cachedFunction(usersConverter.convertById.bind(usersConverter));apps/meteor/server/configuration/accounts_meld.ts (1)
20-32: Remove the inline parameter comment.
Line 26 adds/* , options*/, which conflicts with the no‑comments guideline.♻️ Suggested cleanup
- ...args: any[] /* , options*/ + ...args: any[]apps/meteor/app/custom-oauth/server/transform_helpers.ts (1)
120-138: Array overload should reflect key transformations.
The array overload returnsT, butrenameInvalidPropertiestransforms object keys inside arrays, so callers keep stale types. UseTransformArrayItems<T>for array inputs.♻️ Suggested typing fix
-export function renameInvalidProperties<T extends any[]>(input: T): T; +export function renameInvalidProperties<T extends any[]>(input: T): TransformArrayItems<T>;apps/meteor/ee/server/apps/orchestrator.ts (1)
211-215: Drop the inline FIXME comment per guideline.
Keep implementation comment‑free; track the note in an issue if needed.♻️ Suggested cleanup
- // FIXME: Logger.debug expects only one argument, but the method signature allows multiple this.getRocketChatLogger().debug(...(args as [any]));apps/meteor/app/apps/server/bridges/bridges.ts (1)
165-167: Avoidanycast + inline FIXME in getListenerBridge.
To keep typings consistent (and follow the no‑comments guideline), consider updating AppListenerBridge to implement the expected interface and return it directly, then drop the cast/comment.apps/meteor/app/apps/server/converters/uploads.ts (1)
105-109: Drop inline// FIXMEcomments in implementation.Prefer tracking this via an issue or a typed TODO elsewhere rather than inline comments in production code.
Suggested cleanup
- ...{ _updatedAt: upload.updatedAt }, // FIXME + ...{ _updatedAt: upload.updatedAt }, @@ - ...{ visitorToken }, // FIXME + ...{ visitorToken },As per coding guidelines "Avoid code comments in the implementation".
apps/meteor/app/apps/server/converters/rooms.spec.ts (1)
68-78: Use more descriptive test names.“should return a proper schema” doesn’t convey expected behavior or scenario. Consider naming the expected mapping or conditions directly.
As per coding guidelines "Use descriptive test names that clearly communicate expected behavior in Playwright tests".
Also applies to: 105-115, 117-146
apps/meteor/app/apps/server/converters/messages.spec.ts (1)
74-91: Make test names more specific.“should return a proper schema” is vague; naming the expected fields/behavior would be clearer.
As per coding guidelines "Use descriptive test names that clearly communicate expected behavior in Playwright tests".
Also applies to: 131-144
apps/meteor/app/apps/server/converters/users.ts (1)
52-52: Remove inline FIXME by typingappIdexplicitly.Instead of an inline FIXME, narrow the cast (or extend the type) and drop the comment. As per coding guidelines, Avoid code comments in the implementation.
🔧 Suggested fix
- appId: (user as any).appId, // FIXME + appId: (user as IUser & { appId?: string }).appId,apps/meteor/app/apps/server/converters/rooms.ts (1)
82-86: Resolve inline FIXME markers in implementation.Either fix the underlying logic or move the TODOs to an issue tracker; avoid inline FIXME comments in the code. As per coding guidelines, Avoid code comments in the implementation.
Also applies to: 462-462
| timestamp: (attachment: MessageAttachment) => { | ||
| const result = new Date(attachment.ts!); | ||
| delete attachment.ts; | ||
| return result; | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the file
fd "messages.ts" apps/meteor/app/apps/server/converters/Repository: RocketChat/Rocket.Chat
Length of output: 116
🏁 Script executed:
# Read the relevant section of the file
cat -n apps/meteor/app/apps/server/converters/messages.ts | sed -n '330,360p'Repository: RocketChat/Rocket.Chat
Length of output: 819
🏁 Script executed:
# Search for MessageAttachment type definition to understand ts property
rg "interface MessageAttachment|type MessageAttachment" --type tsRepository: RocketChat/Rocket.Chat
Length of output: 820
🏁 Script executed:
# Look for the converter context to understand the expected return type
cat -n apps/meteor/app/apps/server/converters/messages.ts | sed -n '335,355p'Repository: RocketChat/Rocket.Chat
Length of output: 596
🏁 Script executed:
# Check the MessageAttachmentDefault to see if ts is optional
cat packages/core-typings/src/IMessage/MessageAttachment/MessageAttachmentDefault.tsRepository: RocketChat/Rocket.Chat
Length of output: 699
🏁 Script executed:
# Check MessageAttachmentBase for ts property
cat packages/core-typings/src/IMessage/MessageAttachment/MessageAttachmentBase.tsRepository: RocketChat/Rocket.Chat
Length of output: 831
🏁 Script executed:
# Look at the converter context - what type does the timestamp field expect?
cat -n apps/meteor/app/apps/server/converters/messages.ts | sed -n '310,350p'Repository: RocketChat/Rocket.Chat
Length of output: 1694
🏁 Script executed:
# Check what the converter output type is - look for the map or conversion structure
cat -n apps/meteor/app/apps/server/converters/messages.ts | sed -n '1,50p'Repository: RocketChat/Rocket.Chat
Length of output: 2209
🏁 Script executed:
# Search for how the converter is used to understand expected return type
rg "transformMappedData" apps/meteor/app/apps/server/converters/messages.ts -B 5 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 1142
🏁 Script executed:
# Check tests to see how attachment.ts is used
fd "messages.test" apps/meteor/app/apps/server/converters/ --type fRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Search for usage of ts in MessageAttachment context
rg "\.ts\s*=" apps/meteor/app/apps/server/converters/messages.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the transformMappedData function to understand expected return type
cat -n apps/meteor/app/apps/server/converters/transformMappedData.tsRepository: RocketChat/Rocket.Chat
Length of output: 6234
🏁 Script executed:
# Check IMessageAttachment from apps-engine to see what the expected output is
rg "timestamp" packages --type ts -A 2 -B 2 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 3607
🏁 Script executed:
# Look at the actual return type or interface for converted attachments
rg "IMessageAttachment" packages/core-typings -lRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Search for where attachments come from to see if ts is always defined
rg "attachment\.ts" apps/meteor/app/apps/server/converters/messages.tsRepository: RocketChat/Rocket.Chat
Length of output: 136
🏁 Script executed:
# Check if there's any validation before attachments are processed
cat -n apps/meteor/app/apps/server/converters/messages.ts | sed -n '280,320p'Repository: RocketChat/Rocket.Chat
Length of output: 1716
🏁 Script executed:
# Verify behavior of new Date(undefined)
node -e "console.log(new Date(undefined)); console.log(isNaN(new Date(undefined))); console.log(new Date(undefined).toString());"Repository: RocketChat/Rocket.Chat
Length of output: 96
🏁 Script executed:
# Check if there are any tests that show expected behavior with undefined ts
rg "\.ts" apps/meteor/app/apps/server/converters --type ts | grep -i testRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look at how the converted attachments are consumed - do they expect Date or Date | undefined?
rg "timestamp" apps/meteor/app/apps/server/converters -A 2 -B 2 | grep -A 5 -B 5 "IMessageAttachment\|timestamp"Repository: RocketChat/Rocket.Chat
Length of output: 3001
🏁 Script executed:
# Check the IMessageAttachment interface definition to see expected types
cat packages/core-typings/src/IMessage/MessageAttachment/MessageAttachment.tsRepository: RocketChat/Rocket.Chat
Length of output: 627
Guard against undefined ts before creating Date.
attachment.ts is optional in MessageAttachmentBase and can be undefined. The current code uses a non-null assertion, but new Date(undefined) produces an Invalid Date. The sibling converter in threads.ts handles this correctly with a ternary guard.
🔧 Suggested fix
timestamp: (attachment: MessageAttachment) => {
- const result = new Date(attachment.ts!);
- delete attachment.ts;
- return result;
+ if (!attachment.ts) {
+ return undefined;
+ }
+ const result = new Date(attachment.ts);
+ delete attachment.ts;
+ return result;
},📝 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.
| timestamp: (attachment: MessageAttachment) => { | |
| const result = new Date(attachment.ts!); | |
| delete attachment.ts; | |
| return result; | |
| }, | |
| timestamp: (attachment: MessageAttachment) => { | |
| if (!attachment.ts) { | |
| return undefined; | |
| } | |
| const result = new Date(attachment.ts); | |
| delete attachment.ts; | |
| return result; | |
| }, |
🤖 Prompt for AI Agents
In `@apps/meteor/app/apps/server/converters/messages.ts` around lines 341 - 345,
The timestamp converter in messages.ts uses a non-null assertion on
attachment.ts causing Invalid Date when undefined; update the timestamp function
(the converter for MessageAttachment) to first check if attachment.ts is defined
(e.g., a ternary or if) and only call new Date(attachment.ts) when present,
otherwise return undefined (matching the threads.ts sibling), and preserve the
delete attachment.ts behavior when ts exists; adjust the return type if
necessary to match other converters.
| async convertByName(roomName: IRoom['name']): Promise<IAppsRoom | undefined> { | ||
| const room = await Rooms.findOneByName(roomName!); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file in question
cat -n apps/meteor/app/apps/server/converters/rooms.ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 1524
🏁 Script executed:
# Find the IRoom type definition
fd -t f IRoom.ts --search-path . | head -5Repository: RocketChat/Rocket.Chat
Length of output: 155
🏁 Script executed:
# Search for IRoom definition in the codebase
rg "interface IRoom|type IRoom" -A 5 --type ts --type tsx | head -50Repository: RocketChat/Rocket.Chat
Length of output: 93
🏁 Script executed:
# Look at the IRoom type definition
cat packages/core-typings/src/IRoom.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 2648
🏁 Script executed:
# Search for Rooms.findOneByName method
rg "findOneByName" -A 3 -B 1 --type ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 2551
🏁 Script executed:
# Search for Rooms.findOneByName specifically
rg "findOneByName\(" packages/models/src/models/Rooms.ts -A 5 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 369
🏁 Script executed:
# Check the Rooms model interface/class definition
cat packages/models/src/models/Rooms.ts | grep -A 3 "findOneByName"Repository: RocketChat/Rocket.Chat
Length of output: 826
Add a guard clause to handle undefined room names.
IRoom['name'] is optional (string | undefined), but Rooms.findOneByName() expects a non-null value. The non-null assertion bypasses this type safety check.
🔧 Suggested fix
async convertByName(roomName: IRoom['name']): Promise<IAppsRoom | undefined> {
- const room = await Rooms.findOneByName(roomName!);
+ if (!roomName) {
+ return undefined;
+ }
+ const room = await Rooms.findOneByName(roomName);
return this.convertRoom(room);
}📝 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.
| async convertByName(roomName: IRoom['name']): Promise<IAppsRoom | undefined> { | |
| const room = await Rooms.findOneByName(roomName!); | |
| async convertByName(roomName: IRoom['name']): Promise<IAppsRoom | undefined> { | |
| if (!roomName) { | |
| return undefined; | |
| } | |
| const room = await Rooms.findOneByName(roomName); | |
| return this.convertRoom(room); | |
| } |
🤖 Prompt for AI Agents
In `@apps/meteor/app/apps/server/converters/rooms.ts` around lines 18 - 20,
convertByName currently uses a non-null assertion on IRoom['name'] when calling
Rooms.findOneByName; add a guard clause at the start of convertByName to handle
undefined roomName (e.g., if (!roomName) return undefined) and remove the
non-null assertion so Rooms.findOneByName is only called with a real string;
update convertByName to explicitly return undefined for missing names and
proceed with the lookup when roomName is present.
| async convertByUsername(username: IUser['username']): Promise<IAppsUser | undefined> { | ||
| const user = await Users.findOneByUsername(username!); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the IUser type definition
find . -type f -name "*.ts" -o -name "*.tsx" | head -20
rg "interface IUser|type IUser" --type ts -A 5 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 4633
🏁 Script executed:
# Look at the file under review
find . -path "*/apps/meteor/app/apps/server/converters/users.ts" -type fRepository: RocketChat/Rocket.Chat
Length of output: 115
🏁 Script executed:
# Search for Users.findOneByUsername implementation
rg "findOneByUsername" --type ts -B 2 -A 5 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 6285
🏁 Script executed:
# Get the full IUser interface definition
rg "export interface IUser" --type ts -A 30 packages/core-typings/src/IUser.tsRepository: RocketChat/Rocket.Chat
Length of output: 1055
🏁 Script executed:
# Read the file under review
cat -n apps/meteor/app/apps/server/converters/users.tsRepository: RocketChat/Rocket.Chat
Length of output: 4889
🏁 Script executed:
# Check what IAppsUser is
rg "interface IAppsUser|type IAppsUser" --type ts -A 10 | head -30Repository: RocketChat/Rocket.Chat
Length of output: 48
Guard against undefined username before querying.
IUser['username'] is optional (string | undefined); the non-null assertion masks this and would pass undefined to Users.findOneByUsername(), creating a query with undefined instead of handling it explicitly.
🔧 Suggested fix
async convertByUsername(username: IUser['username']): Promise<IAppsUser | undefined> {
- const user = await Users.findOneByUsername(username!);
+ if (!username) {
+ return undefined;
+ }
+ const user = await Users.findOneByUsername(username);
return this.convertToApp(user);
}📝 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.
| async convertByUsername(username: IUser['username']): Promise<IAppsUser | undefined> { | |
| const user = await Users.findOneByUsername(username!); | |
| async convertByUsername(username: IUser['username']): Promise<IAppsUser | undefined> { | |
| if (!username) { | |
| return undefined; | |
| } | |
| const user = await Users.findOneByUsername(username); | |
| return this.convertToApp(user); | |
| } |
🤖 Prompt for AI Agents
In `@apps/meteor/app/apps/server/converters/users.ts` around lines 16 - 18, The
convertByUsername function currently uses a non-null assertion on
IUser['username'] and may pass undefined into Users.findOneByUsername; update
convertByUsername to explicitly guard for undefined by checking the username
parameter at the top (e.g., if (!username) return undefined or handle error) and
remove the non-null assertion so you only call Users.findOneByUsername(username)
when username is a defined string; reference convertByUsername and
Users.findOneByUsername in your change.
| return WebApp.connectHandlers.use('/custom-sounds/', async (req: IncomingMessage, res: ServerResponse /* , next*/) => { | ||
| const fileId = decodeURIComponent(req.url?.replace(/^\//, '').replace(/\?.*$/, '') || ''); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
JavaScript decodeURIComponent throw malformed percent-encoding
💡 Result:
decodeURIComponent() throws URIError: malformed URI sequence when the input is not valid percent-encoding / UTF‑8—i.e., it contains a % that isn’t followed by two hex digits (0-9A-Fa-f), or it contains incomplete/invalid UTF‑8 byte sequences (example: "%E0%A4%A"). [1]
Common fixes:
- Encode literal
%as%25(a raw%in user input will break decoding). - If decoding query strings from form encoding, convert
+to space first:const s = decodeURIComponent(str.replace(/\+/g, " "));
- Validate or guard before decoding:
function safeDecodeURIComponent(str) { try { return decodeURIComponent(str); } catch { return str; } // or handle/log/reject }
- If you want to pre-check for obvious bad
%sequences:const hasBadPercents = /%(?![0-9A-Fa-f]{2})/.test(str);
Best practice: only call decodeURIComponent on strings you know were produced by proper URL encoding (e.g., encodeURIComponent), or treat invalid inputs as data-quality errors and handle them explicitly. [1]
Sources
[1] MDN – “URIError: malformed URI sequence” (covers invalid encoding/decoding and the decodeURIComponent("%E0%A4%A") example) (developer.mozilla.org)
Citations:
🏁 Script executed:
cat -n apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 3006
🏁 Script executed:
rg "decodeURIComponent" apps/meteor/app/custom-sounds/ -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 608
🏁 Script executed:
rg "decodeURIComponent" apps/meteor/ -B 3 -A 3 --max-count=5Repository: RocketChat/Rocket.Chat
Length of output: 9769
Guard decodeURIComponent against malformed URLs.
Malformed percent-encoding (e.g., %2 or incomplete UTF-8 sequences) will throw URIError and bubble up as a 500 response. Return 400 instead to signal client error.
🔧 Suggested fix
- const fileId = decodeURIComponent(req.url?.replace(/^\//, '').replace(/\?.*$/, '') || '');
+ let fileId = '';
+ try {
+ fileId = decodeURIComponent(req.url?.replace(/^\//, '').replace(/\?.*$/, '') || '');
+ } catch {
+ res.writeHead(400);
+ res.write('Bad Request');
+ res.end();
+ return;
+ }📝 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.
| return WebApp.connectHandlers.use('/custom-sounds/', async (req: IncomingMessage, res: ServerResponse /* , next*/) => { | |
| const fileId = decodeURIComponent(req.url?.replace(/^\//, '').replace(/\?.*$/, '') || ''); | |
| return WebApp.connectHandlers.use('/custom-sounds/', async (req: IncomingMessage, res: ServerResponse /* , next*/) => { | |
| let fileId = ''; | |
| try { | |
| fileId = decodeURIComponent(req.url?.replace(/^\//, '').replace(/\?.*$/, '') || ''); | |
| } catch { | |
| res.writeHead(400); | |
| res.write('Bad Request'); | |
| res.end(); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts` around lines
43 - 45, The call to decodeURIComponent when computing fileId inside the
WebApp.connectHandlers.use callback can throw a URIError for malformed
percent-encoding; wrap the decodeURIComponent(...) that defines fileId in a
try/catch, catch URIError (and other decoding errors), and return a 400 response
(set statusCode 400 and end with a short error message) instead of allowing the
error to bubble to a 500; update the handler around the fileId computation in
this connectHandlers callback so subsequent logic only runs when decoding
succeeds.
| } | ||
|
|
||
| res.setHeader('Content-Type', file.contentType); | ||
| res.setHeader('Content-Type', file.contentType!); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts | sed -n '75,95p'Repository: RocketChat/Rocket.Chat
Length of output: 582
🏁 Script executed:
rg -A 5 -B 5 "res.setHeader\('Content-Type'" apps/meteor/app/custom-sounds/server/startup/custom-sounds.tsRepository: RocketChat/Rocket.Chat
Length of output: 332
🏁 Script executed:
rg "contentType" apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts -A 2 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 171
🌐 Web query:
Node.js ServerResponse setHeader undefined value behavior throw error
💡 Result:
In Node.js, calling res.setHeader(name, undefined) (or outgoingMessage.setHeader) throws a synchronous TypeError with code: 'ERR_HTTP_INVALID_HEADER_VALUE' (message like Invalid value "undefined" for header "X"). This is part of Node’s built-in header validation (http.validateHeaderValue() documents the same behavior for undefined). [1]
setHeader() only accepts header values that can be sent as HTTP header content (documented as number | string | string[]), so undefined is treated as an invalid value rather than “skip setting”. [2]
If you want “no header when the value is missing”, either:
- don’t call
setHeaderwhen the value isnull/undefined, or - remove it:
res.removeHeader(name)(if it may have been set earlier). [2]
Rationale: undefined usually indicates a bug (e.g., an unset variable), so Node throws to surface it early instead of silently sending a wrong/missing header. [3]
Sources:
[1] Node.js HTTP docs — http.validateHeaderValue() (undefined => ERR_HTTP_INVALID_HEADER_VALUE) https://nodejs.org/api/http.html
[2] Node.js HTTP docs — response.setHeader() / response.removeHeader() https://nodejs.org/api/http.html
[3] Stack Overflow discussion on ERR_HTTP_INVALID_HEADER_VALUE with undefined https://stackoverflow.com/questions/63032787/why-is-undefined-illegal-as-a-request-header-value-of-nodes-lib-http-js
🏁 Script executed:
rg "file:" apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts -B 10 -A 3 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
rg -A 20 "getFileByPath\|findFileByPath" apps/meteor/app/custom-sounds/server/startup/custom-sounds.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
rg "contentType" apps/meteor/app/custom-sounds/ -t ts --max-count=15Repository: RocketChat/Rocket.Chat
Length of output: 689
🏁 Script executed:
head -n 100 apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts | tail -n 50Repository: RocketChat/Rocket.Chat
Length of output: 1426
🏁 Script executed:
cat -n apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts | sed -n '1,90p'Repository: RocketChat/Rocket.Chat
Length of output: 3339
🏁 Script executed:
find apps/meteor -name "*.ts" -type f | xargs grep -l "class RocketChatFile" | head -5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
rg "getFileWithReadStream" apps/meteor -A 10 -t ts | head -50Repository: RocketChat/Rocket.Chat
Length of output: 3437
🏁 Script executed:
find apps/meteor/app/file -name "*.ts" -type f | head -10Repository: RocketChat/Rocket.Chat
Length of output: 145
Use a fallback for contentType to handle cases where it may be undefined.
The contentType property is typed as optional (contentType?: string), but the non-null assertion (!) only suppresses TypeScript warnings and doesn't prevent runtime errors. If the file lacks a contentType (possible with GridFS storage), Node.js setHeader will throw ERR_HTTP_INVALID_HEADER_VALUE since it rejects undefined values. The FileSystem implementation already uses 'application/octet-stream' as a fallback; apply the same pattern here:
🔧 Suggested fix
- res.setHeader('Content-Type', file.contentType!);
+ res.setHeader('Content-Type', file.contentType || 'application/octet-stream');📝 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.
| res.setHeader('Content-Type', file.contentType!); | |
| res.setHeader('Content-Type', file.contentType || 'application/octet-stream'); |
🤖 Prompt for AI Agents
In `@apps/meteor/app/custom-sounds/server/startup/custom-sounds.ts` at line 86,
Replace the non-null assertion on file.contentType when calling res.setHeader:
instead of using file.contentType! pass a safe fallback value (e.g.
'application/octet-stream') so that res.setHeader('Content-Type', ...) never
receives undefined; locate the call to res.setHeader and update it to use
file.contentType ?? 'application/octet-stream' (or equivalent) to match the
FileSystem implementation.
| const callback = | ||
| (msg: string, name: string) => | ||
| async (reply: RateLimiterReply, input: RateLimiterInput): Promise<void> => { | ||
| if (reply.allowed === false) { | ||
| rateLimiterLog({ msg, reply, input }); | ||
| metrics.ddpRateLimitExceeded.inc({ | ||
| limit_name: name, | ||
| user_id: input.userId, | ||
| client_address: input.clientAddress, | ||
| type: input.type, | ||
| name: input.name, | ||
| connection_id: input.connectionId, | ||
| }); | ||
| // sleep before sending the error to slow down next requests | ||
| if (slowDownRate > 0 && reply.numInvocationsExceeded) { | ||
| await sleep(slowDownRate * reply.numInvocationsExceeded); | ||
| } | ||
| // } else { | ||
| // console.log('DDP RATE LIMIT:', message); | ||
| // console.log(JSON.stringify({ ...reply, ...input }, null, 2)); | ||
| } | ||
| // } else { | ||
| // console.log('DDP RATE LIMIT:', message); | ||
| // console.log(JSON.stringify({ ...reply, ...input }, null, 2)); | ||
| } | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's read the file to understand the context around the callback invocation
wc -l apps/meteor/app/lib/server/startup/rateLimiter.tsRepository: RocketChat/Rocket.Chat
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Read the relevant sections of the file
cat -n apps/meteor/app/lib/server/startup/rateLimiter.ts | sed -n '110,170p'Repository: RocketChat/Rocket.Chat
Length of output: 2717
🏁 Script executed:
#!/bin/bash
# Search for _executeCallback to understand how it's implemented and used
rg -n '_executeCallback' --type ts --type js -C 2Repository: RocketChat/Rocket.Chat
Length of output: 1057
🏁 Script executed:
#!/bin/bash
# Search for where rule is created and how the callback is registered
rg -n 'rule.onError|rule._executeCallback|addRule|createRule' apps/meteor/app/lib/server/startup/rateLimiter.ts -B 3 -A 1Repository: RocketChat/Rocket.Chat
Length of output: 1136
🏁 Script executed:
#!/bin/bash
# Look for imports from meteor rate-limit or similar
rg -n 'import|from|require' apps/meteor/app/lib/server/startup/rateLimiter.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 459
🏁 Script executed:
#!/bin/bash
# Search for where callback is actually assigned to rule
rg -n '\.onError\s*=' apps/meteor/app/lib/server/startup/rateLimiter.ts -C 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Look for where the callback is actually registered with the rule
rg -n 'callback' apps/meteor/app/lib/server/startup/rateLimiter.ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 951
🏁 Script executed:
#!/bin/bash
# Check the full context around the addRule call
cat -n apps/meteor/app/lib/server/startup/rateLimiter.ts | sed -n '175,200p'Repository: RocketChat/Rocket.Chat
Length of output: 960
🏁 Script executed:
#!/bin/bash
# Search for how rules are created in the Meteor package files
find . -type f -name "*.ts" -o -name "*.js" | xargs grep -l "DDPRateLimiter\|_executeCallback" 2>/dev/null | head -10Repository: RocketChat/Rocket.Chat
Length of output: 580
🏁 Script executed:
#!/bin/bash
# Check the type definitions for DDPRateLimiter
cat -n ./apps/meteor/definition/externals/meteor/ddp-rate-limiter.d.tsRepository: RocketChat/Rocket.Chat
Length of output: 441
🏁 Script executed:
#!/bin/bash
# Search for _executeCallback in the codebase to understand what it does
rg -n '_executeCallback' . --type ts --type js -B 5 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 2162
🏁 Script executed:
#!/bin/bash
# Let's check if there's any Meteor rate-limit package source or docs in the repo
find . -type f \( -name "*.ts" -o -name "*.js" -o -name "*.d.ts" \) | xargs grep -l "rate-limit\|RateLimiter" 2>/dev/null | head -15Repository: RocketChat/Rocket.Chat
Length of output: 798
🏁 Script executed:
#!/bin/bash
# Check if there's any information about how callbacks are supposed to work
rg -n 'onError|_executeCallback' ./apps/meteor/app/lib/server/lib/RateLimiter.ts -C 3Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Look at the entire context of the override to understand what's being done
cat -n apps/meteor/app/lib/server/startup/rateLimiter.ts | sed -n '60,140p'Repository: RocketChat/Rocket.Chat
Length of output: 3840
🏁 Script executed:
#!/bin/bash
# Let's check the RateLimiter.ts to understand how the framework is being used
cat -n ./apps/meteor/app/lib/server/lib/RateLimiter.tsRepository: RocketChat/Rocket.Chat
Length of output: 841
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about async callback handling in Meteor packages
rg -n 'async|promise|Promise' apps/meteor/app/lib/server/startup/rateLimiter.tsRepository: RocketChat/Rocket.Chat
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Check if there are any tests or usage examples showing how async callbacks should work
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | xargs grep -l "rateLimiter\|RateLimiter" 2>/dev/nullRepository: RocketChat/Rocket.Chat
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for how the callback function is assigned/registered to understand the flow
cat -n apps/meteor/app/lib/server/startup/rateLimiter.ts | sed -n '183,190p'Repository: RocketChat/Rocket.Chat
Length of output: 388
🏁 Script executed:
#!/bin/bash
# Let's trace backwards - see what callback function is passed to addRule
rg -n 'callback\(' apps/meteor/app/lib/server/startup/rateLimiter.ts -A 2 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 298
🏁 Script executed:
#!/bin/bash
# Search for documentation or source comments about how Meteor's rate-limit handles callbacks
rg -n 'onError\|_executeCallback\|callback' . --type ts --type d.ts | grep -i "meteor\|rate" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Let's search for any usage of the rule._executeCallback in context
rg -n 'rule\.' apps/meteor/app/lib/server/startup/rateLimiter.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 923
🏁 Script executed:
#!/bin/bash
# Check if there are any other places where callbacks are being used with async/await patterns
rg -n 'await.*callback|callback.*await' . --type ts --type jsRepository: RocketChat/Rocket.Chat
Length of output: 17143
🏁 Script executed:
#!/bin/bash
# Let's see if there's any information about how the callback is supposed to work by looking at comments
cat -n apps/meteor/app/lib/server/startup/rateLimiter.ts | sed -n '1,70p'Repository: RocketChat/Rocket.Chat
Length of output: 3165
Async callback not awaited; slowdown delay won't block rate-limited responses.
The callback at lines 143-164 is async and awaits sleep() when rate-limited, but rule._executeCallback() at lines 118 and 131 calls it without await. Since check() is synchronous and returns immediately, the Promise from the async callback is fire-and-forget, and the sleep delay won't actually block the response. The slowDownRate feature won't have its intended effect. Either await the callback's Promise or restructure to handle async callbacks in a synchronous context.
🤖 Prompt for AI Agents
In `@apps/meteor/app/lib/server/startup/rateLimiter.ts` around lines 143 - 164,
The rate-limit slowdown is ineffective because the async callback (callback)
that awaits sleep() is invoked via rule._executeCallback() from check() without
awaiting its Promise; update the call sites where rule._executeCallback(...) is
invoked (the two places that call it from check()) to await the returned Promise
and make check() async (propagate async/await) so the sleep actually blocks, or
alternatively convert the callback to a synchronous path (remove async/sleep) if
making check() async is undesirable—ensure consistency between callback
(function callback) and its invoker rule._executeCallback() so the slowdown
delay is awaited.
| it.skip('should return without the "." from "#general."', () => { | ||
| expect(result).to.be.deep.equal(mentionsParser.getUserMentions('#general.')); | ||
| expect(result).toEqual(mentionsParser.getUserMentions('#general.')); | ||
| }); | ||
|
|
||
| it.skip('should return without the "_" from "#general_"', () => { | ||
| expect(result).to.be.deep.equal(mentionsParser.getUserMentions('#general_')); | ||
| expect(result).toEqual(mentionsParser.getUserMentions('#general_')); | ||
| }); | ||
|
|
||
| it.skip('should return without the "-" from "#general."', () => { | ||
| expect(result).to.be.deep.equal(mentionsParser.getUserMentions('#general-')); | ||
| expect(result).toEqual(mentionsParser.getUserMentions('#general-')); | ||
| }); |
There was a problem hiding this comment.
Skipped tests call the wrong method.
These skipped tests are in the getChannelMentions describe block but call getUserMentions instead of getChannelMentions. If these tests are ever unskipped, they will test the wrong functionality.
🐛 Proposed fix
it.skip('should return without the "." from "#general."', () => {
- expect(result).toEqual(mentionsParser.getUserMentions('#general.'));
+ expect(result).toEqual(mentionsParser.getChannelMentions('#general.'));
});
it.skip('should return without the "_" from "#general_"', () => {
- expect(result).toEqual(mentionsParser.getUserMentions('#general_'));
+ expect(result).toEqual(mentionsParser.getChannelMentions('#general_'));
});
it.skip('should return without the "-" from "#general."', () => {
- expect(result).toEqual(mentionsParser.getUserMentions('#general-'));
+ expect(result).toEqual(mentionsParser.getChannelMentions('#general-'));
});📝 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.
| it.skip('should return without the "." from "#general."', () => { | |
| expect(result).to.be.deep.equal(mentionsParser.getUserMentions('#general.')); | |
| expect(result).toEqual(mentionsParser.getUserMentions('#general.')); | |
| }); | |
| it.skip('should return without the "_" from "#general_"', () => { | |
| expect(result).to.be.deep.equal(mentionsParser.getUserMentions('#general_')); | |
| expect(result).toEqual(mentionsParser.getUserMentions('#general_')); | |
| }); | |
| it.skip('should return without the "-" from "#general."', () => { | |
| expect(result).to.be.deep.equal(mentionsParser.getUserMentions('#general-')); | |
| expect(result).toEqual(mentionsParser.getUserMentions('#general-')); | |
| }); | |
| it.skip('should return without the "." from "#general."', () => { | |
| expect(result).toEqual(mentionsParser.getChannelMentions('#general.')); | |
| }); | |
| it.skip('should return without the "_" from "#general_"', () => { | |
| expect(result).toEqual(mentionsParser.getChannelMentions('#general_')); | |
| }); | |
| it.skip('should return without the "-" from "#general."', () => { | |
| expect(result).toEqual(mentionsParser.getChannelMentions('#general-')); | |
| }); |
🤖 Prompt for AI Agents
In `@apps/meteor/app/mentions/lib/MentionsParser.spec.ts` around lines 87 - 97,
The three skipped tests in the getChannelMentions describe block incorrectly
call mentionsParser.getUserMentions; update each skipped spec to call
mentionsParser.getChannelMentions instead (the tests titled 'should return
without the "." from "#general."', 'should return without the "_" from
"#general_"', and 'should return without the "-" from "#general."') so that when
unskipped they exercise the correct function; keep the rest of each expectation
the same.
| export const Google: { | ||
| readonly name: string; | ||
| readonly whitelistedFields: string[]; | ||
| requestCredential( | ||
| options: Meteor.LoginWithExternalServiceOptions | undefined, | ||
| credentialRequestCompleteCallback: (credentialTokenOrError?: string | Error) => void, | ||
| ): void; | ||
| signIn?( | ||
| options: Meteor.LoginWithExternalServiceOptions | undefined, | ||
| callback?: (error: LoginError | undefined, result?: unknown) => void, | ||
| ): void; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for LoginError declaration and usage in the codebase
echo "=== Searching for LoginError declaration/definition ==="
rg "type LoginError|interface LoginError|declare.*LoginError" --type ts --type tsx --type=d.ts -B 2 -A 2
echo ""
echo "=== Checking google-oauth.d.ts context and imports ==="
head -20 apps/meteor/definition/externals/meteor/google-oauth.d.ts
echo ""
echo "=== Checking for LoginError in related Meteor type definitions ==="
fd "\.d\.ts$" apps/meteor/definition/externals/meteor/ --exec grep -l "LoginError" {}Repository: RocketChat/Rocket.Chat
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Search for LoginError everywhere in the codebase
echo "=== All occurrences of LoginError ==="
rg "LoginError" -t ts -t js -t d.ts --context=3
echo ""
echo "=== Check node_modules or meteor types (if available) ==="
find . -path ./node_modules -prune -o -name "*.d.ts" -type f -print | head -20 | xargs grep -l "LoginError" 2>/dev/null || echo "No matches in .d.ts files outside node_modules"
echo ""
echo "=== Check the full google-oauth.d.ts file ==="
cat apps/meteor/definition/externals/meteor/google-oauth.d.tsRepository: RocketChat/Rocket.Chat
Length of output: 780
🏁 Script executed:
#!/bin/bash
# Check other meteor definition files for how they import/use account types
echo "=== List all meteor definition files ==="
fd "\.d\.ts$" apps/meteor/definition/externals/meteor/
echo ""
echo "=== Check accounts-base.d.ts or similar for error types ==="
cat apps/meteor/definition/externals/meteor/accounts-base.d.ts 2>/dev/null | grep -A 5 -B 5 "Error\|error" | head -40
echo ""
echo "=== Check if any other files import from accounts-base ==="
rg "accounts-base" apps/meteor/definition/externals/meteor/ -A 2 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 3430
🏁 Script executed:
#!/bin/bash
# Check similar OAuth files to see if they have the same issue
echo "=== facebook-oauth.d.ts ==="
cat apps/meteor/definition/externals/meteor/facebook-oauth.d.ts
echo ""
echo "=== twitter-oauth.d.ts ==="
cat apps/meteor/definition/externals/meteor/twitter-oauth.d.tsRepository: RocketChat/Rocket.Chat
Length of output: 263
Add missing LoginError type to this module declaration.
Line 11 references LoginError which is not defined in the codebase. Either import or declare this type within the module. If LoginError should match Meteor's standard error types, consider using Meteor.Error instead, or define LoginError as a type alias if it has a specific shape.
🤖 Prompt for AI Agents
In `@apps/meteor/definition/externals/meteor/google-oauth.d.ts` around lines 2 -
12, The module declares Google and references LoginError but LoginError is
missing; fix by adding a local type/import for LoginError in this declaration
file—either declare type LoginError = Meteor.Error (or import Meteor and alias
Meteor.Error as LoginError) or define the LoginError shape expected by signIn;
update the google-oauth.d.ts module so the referenced LoginError type is defined
alongside the existing Google, requestCredential, and signIn symbols.
| findOneById(id: string): T | null { | ||
| return this.data[id]; | ||
| } |
There was a problem hiding this comment.
Return type mismatch: returns undefined but declares T | null.
When id is not found in data, the property access returns undefined, not null. This creates a type inconsistency that could cause issues in consuming code expecting null for missing entries.
🔧 Option 1: Fix return type to match behavior
- findOneById(id: string): T | null {
- return this.data[id];
+ findOneById(id: string): T | undefined {
+ return this.data[id];
}🔧 Option 2: Explicitly return null for missing entries
findOneById(id: string): T | null {
- return this.data[id];
+ return this.data[id] ?? null;
}📝 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.
| findOneById(id: string): T | null { | |
| return this.data[id]; | |
| } | |
| findOneById(id: string): T | undefined { | |
| return this.data[id]; | |
| } |
| findOneById(id: string): T | null { | |
| return this.data[id]; | |
| } | |
| findOneById(id: string): T | null { | |
| return this.data[id] ?? null; | |
| } |
🤖 Prompt for AI Agents
In `@apps/meteor/tests/unit/app/apps/server/mocks/models/BaseModel.mock.ts` around
lines 4 - 6, findOneById declares return type T | null but currently returns
this.data[id] which yields undefined when not present; update the function to be
consistent by either changing its signature to return T | undefined or by
explicitly returning null for missing entries (e.g., in findOneById use a null
coalescing behavior to return this.data[id] ?? null). Ensure references to the
data property and the findOneById method are updated so callers and types align.
| export class MessagesMock extends BaseModelMock<IMessage> { | ||
| override data = { |
There was a problem hiding this comment.
Keep data protected when overriding BaseModelMock.
BaseModelMock declares data as protected, but the override lacks a visibility modifier, unintentionally widening it to public.
🔧 Suggested fix
export class MessagesMock extends BaseModelMock<IMessage> {
- override data = {
+ protected override data = {📝 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.
| export class MessagesMock extends BaseModelMock<IMessage> { | |
| override data = { | |
| export class MessagesMock extends BaseModelMock<IMessage> { | |
| protected override data = { |
🤖 Prompt for AI Agents
In `@apps/meteor/tests/unit/app/apps/server/mocks/models/Messages.mock.ts` around
lines 5 - 6, Override of BaseModelMock's property "data" in class MessagesMock
currently lacks visibility and becomes public; change the declaration in
MessagesMock to explicitly use the protected modifier (i.e., make "protected
data = { ... }") so it matches BaseModelMock's protected visibility and
preserves encapsulation for the IMessage data.
1 participant
Proposed changes (including videos or screenshots)
It does a raw migration to TypeScript on modules current coded as JavaScript.
Issue(s)
Steps to test or reproduce
Further comments
Heavily AI-assisted, needs careful review.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores