refactor: Convert missing JavaScript modules to TypeScript#38357
Open
refactor: Convert missing JavaScript modules to TypeScript#38357
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request migrates JavaScript test infrastructure to Jest, converts JavaScript modules to TypeScript with comprehensive type safety improvements, refactors converter classes to implement interfaces with explicit method signatures, and enhances OAuth and authentication flows with stricter typing and validation logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/App
participant OAuth as Custom OAuth
participant Config as OAuth Config
participant External as External Provider
participant User as User/Accounts
Client->>OAuth: initiateLogin(serviceName, options)
OAuth->>Config: loadServiceConfiguration(serviceName)
Config-->>OAuth: config details
OAuth->>External: requestAccessToken(code)
External-->>OAuth: accessToken
OAuth->>External: getIdentity(accessToken)
External-->>OAuth: identity data
OAuth->>OAuth: normalizeIdentity(identity)
OAuth->>OAuth: registerService() / addHookToProcessUser()
OAuth->>User: Accounts.updateOrCreateUserFromExternalService(serviceName, serviceData)
User-->>OAuth: user result
OAuth-->>Client: login complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38357 +/- ##
===========================================
+ Coverage 70.85% 71.09% +0.24%
===========================================
Files 3208 3215 +7
Lines 113426 115286 +1860
Branches 20489 21107 +618
===========================================
+ Hits 80363 81958 +1595
- Misses 31012 31126 +114
- Partials 2051 2202 +151
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
📦 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/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/12 22:57", "02/13 22:38", "02/15 00:54 (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.11]
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.11]
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.12]
line "omnichannel-transcript-service" [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.13, 0.13, 0.13]
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.11]
line "queue-worker-service" [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.13, 0.13, 0.13]
line "rocketchat" [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.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Sun, 15 Feb 2026 00:54:56 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
90bade6 to
e246502
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 144 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="apps/meteor/app/lib/server/oauth/twitter.ts">
<violation number="1" location="apps/meteor/app/lib/server/oauth/twitter.ts:53">
P2: The new truthiness check skips valid falsy profile fields; this changes behavior from the previous `_.pick`-based logic and can silently drop whitelisted data.</violation>
</file>
<file name="apps/meteor/app/authentication/server/startup/index.ts">
<violation number="1" location="apps/meteor/app/authentication/server/startup/index.ts:113">
P1: Unsafe type cast `as IUser & { name: string }` hides that `name` is optional on `IUser`. If `userModel.name` is `undefined`, `safeHtmlDots` will throw at runtime because it calls `.replace()` on the value. Use a fallback instead.
(Based on your team's feedback about avoiding unsafe type casts and validating values first.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/app/apps/server/converters/visitors.ts">
<violation number="1" location="apps/meteor/app/apps/server/converters/visitors.ts:69">
P2: Avoid casting `visitor.status` to `UserStatus` without validation. Since app visitor status is a plain string, this cast can let unsupported values through; validate against allowed `UserStatus` values before assigning a default.</violation>
</file>
<file name="apps/meteor/app/lib/server/lib/validateEmailDomain.ts">
<violation number="1" location="apps/meteor/app/lib/server/lib/validateEmailDomain.ts:21">
P2: Avoid unsafe cast on settings value before calling `.split()`. Narrow to `string` (or provide a safe fallback) so non-string values cannot cause runtime failures.
(Based on your team's feedback about avoiding unsafe type casts for possibly invalid values.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/app/custom-oauth/server/custom_oauth_server.ts">
<violation number="1" location="apps/meteor/app/custom-oauth/server/custom_oauth_server.ts:79">
P3: The `addAutopublishFields` property is now unused in `CustomOAuthOptions` since the code that consumed it (`Accounts.addAutopublishFields`) was removed from `configure()`. Remove the property from the type to avoid misleading future maintainers.
(Based on your team's feedback about removing unused types and configuration entries.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/meteor/app/custom-oauth/server/custom_oauth_server.ts:298">
P2: Behavioral change: `_OAuthCustom: true` and `serverURL` were not present in the old JS `serviceData` for access token registration — they were added solely to satisfy the `ServiceData` type. This changes what data is stored/returned during access-token-based OAuth login. Consider using a narrower inline type (e.g., `Partial<ServiceData> & { accessToken: string; expiresAt: number }`) or a dedicated type, rather than forcing these extra fields at runtime to match a type that was designed for the full OAuth flow.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| Accounts.emailTemplates.verifyEmail.html = function (userModel, url) { | ||
| const name = safeHtmlDots(userModel.name); | ||
| const name = safeHtmlDots((userModel as IUser & { name: string }).name); |
Contributor
There was a problem hiding this comment.
P1: Unsafe type cast as IUser & { name: string } hides that name is optional on IUser. If userModel.name is undefined, safeHtmlDots will throw at runtime because it calls .replace() on the value. Use a fallback instead.
(Based on your team's feedback about avoiding unsafe type casts and validating values first.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/authentication/server/startup/index.ts, line 113:
<comment>Unsafe type cast `as IUser & { name: string }` hides that `name` is optional on `IUser`. If `userModel.name` is `undefined`, `safeHtmlDots` will throw at runtime because it calls `.replace()` on the value. Use a fallback instead.
(Based on your team's feedback about avoiding unsafe type casts and validating values first.) </comment>
<file context>
@@ -108,13 +110,13 @@ Meteor.startup(() => {
Accounts.emailTemplates.verifyEmail.html = function (userModel, url) {
- const name = safeHtmlDots(userModel.name);
+ const name = safeHtmlDots((userModel as IUser & { name: string }).name);
return Mailer.replace(verifyEmailTemplate, { Verification_Url: url, name });
</file context>
Suggested change
| const name = safeHtmlDots((userModel as IUser & { name: string }).name); | |
| const name = safeHtmlDots((userModel as IUser).name ?? ''); |
Comment on lines
+53
to
+55
| if (identity[key]) { | ||
| fields[key] = identity[key]; | ||
| } |
Contributor
There was a problem hiding this comment.
P2: The new truthiness check skips valid falsy profile fields; this changes behavior from the previous _.pick-based logic and can silently drop whitelisted data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/lib/server/oauth/twitter.ts, line 53:
<comment>The new truthiness check skips valid falsy profile fields; this changes behavior from the previous `_.pick`-based logic and can silently drop whitelisted data.</comment>
<file context>
@@ -38,13 +43,18 @@ registerAccessTokenService('twitter', async (options) => {
- _.extend(serviceData, fields);
+ const fields: Record<string, any> = {};
+ for (const key of whitelistedFields) {
+ if (identity[key]) {
+ fields[key] = identity[key];
+ }
</file context>
Suggested change
| if (identity[key]) { | |
| fields[key] = identity[key]; | |
| } | |
| if (Object.prototype.hasOwnProperty.call(identity, key)) { | |
| fields[key] = identity[key]; | |
| } |
| token: visitor.token, | ||
| phone: visitor.phone, | ||
| livechatData: visitor.livechatData, | ||
| status: (visitor.status as UserStatus | undefined) || UserStatus.ONLINE, |
Contributor
There was a problem hiding this comment.
P2: Avoid casting visitor.status to UserStatus without validation. Since app visitor status is a plain string, this cast can let unsupported values through; validate against allowed UserStatus values before assigning a default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/apps/server/converters/visitors.ts, line 69:
<comment>Avoid casting `visitor.status` to `UserStatus` without validation. Since app visitor status is a plain string, this cast can let unsupported values through; validate against allowed `UserStatus` values before assigning a default.</comment>
<file context>
@@ -0,0 +1,78 @@
+ token: visitor.token,
+ phone: visitor.phone,
+ livechatData: visitor.livechatData,
+ status: (visitor.status as UserStatus | undefined) || UserStatus.ONLINE,
+ ...(visitor.visitorEmails && { visitorEmails: visitor.visitorEmails }),
+ ...(visitor.department && { department: visitor.department }),
</file context>
| } | ||
|
|
||
| emailDomainBlackList = value | ||
| emailDomainBlackList = (value as string) |
Contributor
There was a problem hiding this comment.
P2: Avoid unsafe cast on settings value before calling .split(). Narrow to string (or provide a safe fallback) so non-string values cannot cause runtime failures.
(Based on your team's feedback about avoiding unsafe type casts for possibly invalid values.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/lib/server/lib/validateEmailDomain.ts, line 21:
<comment>Avoid unsafe cast on settings value before calling `.split()`. Narrow to `string` (or provide a safe fallback) so non-string values cannot cause runtime failures.
(Based on your team's feedback about avoiding unsafe type casts for possibly invalid values.) </comment>
<file context>
@@ -9,16 +9,16 @@ import { settings } from '../../../settings/server';
}
- emailDomainBlackList = value
+ emailDomainBlackList = (value as string)
.split(',')
.filter(Boolean)
</file context>
Suggested change
| emailDomainBlackList = (value as string) | |
| emailDomainBlackList = (typeof value === 'string' ? value : '') |
| const identity = await this.getIdentity(response.access_token); | ||
|
|
||
| const serviceData = { | ||
| const serviceData: ServiceData = { |
Contributor
There was a problem hiding this comment.
P2: Behavioral change: _OAuthCustom: true and serverURL were not present in the old JS serviceData for access token registration — they were added solely to satisfy the ServiceData type. This changes what data is stored/returned during access-token-based OAuth login. Consider using a narrower inline type (e.g., Partial<ServiceData> & { accessToken: string; expiresAt: number }) or a dedicated type, rather than forcing these extra fields at runtime to match a type that was designed for the full OAuth flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/custom-oauth/server/custom_oauth_server.ts, line 298:
<comment>Behavioral change: `_OAuthCustom: true` and `serverURL` were not present in the old JS `serviceData` for access token registration — they were added solely to satisfy the `ServiceData` type. This changes what data is stored/returned during access-token-based OAuth login. Consider using a narrower inline type (e.g., `Partial<ServiceData> & { accessToken: string; expiresAt: number }`) or a dedicated type, rather than forcing these extra fields at runtime to match a type that was designed for the full OAuth flow.</comment>
<file context>
@@ -185,21 +284,20 @@ export class CustomOAuth {
+ const identity = await this.getIdentity(response.access_token);
- const serviceData = {
+ const serviceData: ServiceData = {
_OAuthCustom: true,
- serverURL: self.serverURL,
</file context>
| mergeRoles?: boolean; | ||
| rolesToSync?: string; | ||
| showButton?: boolean; | ||
| addAutopublishFields?: Record<string, any>; |
Contributor
There was a problem hiding this comment.
P3: The addAutopublishFields property is now unused in CustomOAuthOptions since the code that consumed it (Accounts.addAutopublishFields) was removed from configure(). Remove the property from the type to avoid misleading future maintainers.
(Based on your team's feedback about removing unused types and configuration entries.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/custom-oauth/server/custom_oauth_server.ts, line 79:
<comment>The `addAutopublishFields` property is now unused in `CustomOAuthOptions` since the code that consumed it (`Accounts.addAutopublishFields`) was removed from `configure()`. Remove the property from the type to avoid misleading future maintainers.
(Based on your team's feedback about removing unused types and configuration entries.) </comment>
<file context>
@@ -20,11 +21,108 @@ import { settings } from '../../settings/server';
+ mergeRoles?: boolean;
+ rolesToSync?: string;
+ showButton?: boolean;
+ addAutopublishFields?: Record<string, any>;
+};
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Bug Fixes
Refactor