Conversation
… options handling
…es in company info handling
…m field use cases
…related use cases
…ling in CronJobsService
📝 WalkthroughWalkthroughThis PR applies a comprehensive refactoring across multiple backend domains: it removes a deprecated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a TypeScript backend refactor focused on cleaning up repository APIs, tightening HTTP semantics (not-found → 404), and extracting/centralizing update logic (notably for connection table categories), alongside several small code quality improvements.
Changes:
- Updated custom-field flows to return 404 for missing resources and aligned E2E tests accordingly.
- Refactored repository APIs/usages (e.g.,
findCompanyInfoByUserId, group repository param naming) and improved various call-sites/DTO builders. - Extracted table-categories sync logic into a dedicated utility and refined connection-properties update behavior.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/saas-tests/custom-field-e2e.test.ts | Aligns expected status codes with new 404 behavior. |
| backend/test/ava-tests/non-saas-tests/non-saas-custom-field-e2e.test.ts | Aligns expected status codes with new 404 behavior. |
| backend/src/entities/user/use-cases/verify-reset-user-password.use.case.ts | Renames company-info repository method usage. |
| backend/src/entities/user/use-cases/usual-login-use.case.ts | Renames company-info repository method usage. |
| backend/src/entities/user/use-cases/request-email-verification.use.case.ts | Renames company-info repository method usage. |
| backend/src/entities/user/use-cases/otp-login-use.case.ts | Renames company-info repository method usage. |
| backend/src/entities/user/use-cases/disable-otp.use.case.ts | Renames company-info repository method usage. |
| backend/src/entities/user/use-cases/change-usual-password-use.case.ts | Renames company-info repository method usage. |
| backend/src/entities/group/use-cases/saas-add-user-in-group-v2.use.case.ts | Adds request-scoped injectable metadata. |
| backend/src/entities/group/use-cases/find-all-users-in-group.use.case.ts | Fixes import path to be relative. |
| backend/src/entities/group/repository/group.repository.interface.ts | Renames param to userId for clarity. |
| backend/src/entities/group/repository/group-custom-repository-extension.ts | Uses shorthand params + aligns query param name to userId. |
| backend/src/entities/group/group.module.ts | Tightens configure return type to void. |
| backend/src/entities/group/group.controller.ts | Ensures finally awaits the use case execution before logging. |
| backend/src/entities/custom-field/utils/validate-create-custom-field-dto.ts | Removes unused userId parameter from validation util. |
| backend/src/entities/custom-field/utils/build-found-custom-fields-ds.ts | Simplifies DTO construction. |
| backend/src/entities/custom-field/use-cases/update-custom-field.use.case.ts | Removes unused userId, returns 404 on not-found, avoids mutating DTO id. |
| backend/src/entities/custom-field/use-cases/delete-custom-field.use.case.ts | Returns 404 on not-found errors. |
| backend/src/entities/custom-field/use-cases/create-custom-fields.use.case.ts | Updates validation call signature. |
| backend/src/entities/custom-field/repository/custom-field-repository-extension.ts | Fixes export name typo (customFields...). |
| backend/src/entities/custom-field/custom-field.controller.ts | Simplifies DTO building with shorthand properties. |
| backend/src/entities/cron-jobs/job-list.entity.ts | Simplifies @PrimaryColumn options. |
| backend/src/entities/cron-jobs/cron-jobs.service.ts | Improves filtering/typing and refactors results collection + Slack notification placement. |
| backend/src/entities/connection/use-cases/unfreeze-connection.use.case.ts | Updates commented reference to renamed repository method. |
| backend/src/entities/connection-properties/utils/sync-table-categories.ts | New helper extracting table-category sync logic. |
| backend/src/entities/connection-properties/utils/build-update-connection-properties-object.ts | Builds partial update object (only defined fields) + makes interface fields optional. |
| backend/src/entities/connection-properties/use-cases/update-connection-properties.use.case.ts | Uses new syncTableCategories helper and updates categories assignment flow. |
| backend/src/entities/connection-properties/connection-properties.interface.ts | Removes unused interface. |
| backend/src/entities/connection-properties/connection-properties.entity.ts | Corrects nullability typing for nullable columns. |
| backend/src/entities/connection-properties/connection-properties.controller.ts | Removes unused RO type, unifies DS building via helper, returns Found DS types. |
| backend/src/entities/connection-properties/application/data-structures/create-connection-properties.ds.ts | Makes properties optional to support partial updates. |
| backend/src/entities/company-info/use-cases/toggle-test-connections-company-display-mode.use.case.ts | Renames company-info repository method usage. |
| backend/src/entities/company-info/use-cases/company-info-use-cases.interface.ts | Adds alias type for unsuspend use case. |
| backend/src/entities/company-info/repository/company-info-repository.interface.ts | Removes misspelled method from interface. |
| backend/src/entities/company-info/repository/company-info-custom-repository.extension.ts | Removes misspelled method impl; flattens connection lists safely. |
| backend/src/entities/company-info/invitation-in-company/repository/invitation-in-company-custom-repository-extension.ts | Fixes query by using andWhere (avoids overwriting prior where). |
| backend/src/entities/company-info/company-info.controller.ts | Uses new alias type for unsuspend use case. |
| backend/src/entities/company-info/company-info-helper.service.ts | Parallelizes independent counts with Promise.all. |
| backend/src/entities/amplitude/amplitude.service.ts | Initializes Amplitude client once; improves typing and await behavior. |
| backend/src/entities/agent/repository/custom-agent-repository-extension.ts | Uses setToken, types extension as IAgentRepository, refactors token creation. |
| backend/src/entities/agent/repository/agent.repository.interface.ts | Adds getTestAgentToken to interface. |
| backend/src/entities/agent/agent.module.ts | Removes middleware wiring; leaves module as simple module. |
| backend/src/entities/agent/agent.entity.ts | Avoids re-hashing tokens on every update via _tokenChanged guard. |
| backend/src/common/application/global-database-context.ts | Updates import/extension name for custom-fields repository extension. |
Comments suppressed due to low confidence (2)
backend/src/entities/cron-jobs/cron-jobs.service.ts:170
foundEmailsis built fromresult.accepted, which can includeMail.Addressobjects. This makesfoundEmails.has(emailString)always return false for those entries, and it also causesaccepted.join(', ')to produce[object Object]in Slack output. Normalize each accepted entry to a string email (e.g.,typeof v === 'string' ? v : v.address) before adding/joining, and keepfoundEmailsasSet<string>.
const foundEmails = new Set<string | Mail.Address>();
filteredResults.forEach((result) => {
if (result?.accepted) {
result.accepted.forEach((email) => foundEmails.add(email));
}
});
const emailsNonFoundInResults = allFoundEmails.filter((email) => !foundEmails.has(email));
backend/src/entities/agent/repository/custom-agent-repository-extension.ts:20
savedAgent.token = tokenoverwrites the post-save (hashed) token value with the plaintext token. With the new_tokenChangedguard inAgentEntity, any latersave(savedAgent)could persist the plaintext token because the setter wasn't used. Avoid mutating the entity’stokenfield after save; instead return the plaintext token separately (or via a non-persisted property) and keeptokenas the stored hashed value.
const savedAgent = await this.save(agent);
savedAgent.token = token;
return savedAgent;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { Constants } from '../../helpers/constants/constants.js'; | ||
| import { slackPostMessage } from '../../helpers/index.js'; | ||
| import { ValidationHelper } from '../../helpers/validators/validation-helper.js'; | ||
| import Mail from 'nodemailer/lib/mailer/index.js'; |
There was a problem hiding this comment.
Mail is imported as a runtime value but only used for typing. This can introduce an unnecessary runtime dependency (and may break builds if the import is elided/preserved differently). Switch this to a type-only import (import type ...) or remove it entirely by normalizing accepted addresses to strings.
Suggested change
| import Mail from 'nodemailer/lib/mailer/index.js'; | |
| import type Mail from 'nodemailer/lib/mailer/index.js'; |
Comment on lines
+28
to
+47
| private _tokenChanged = false; | ||
|
|
||
| setToken(token: string): void { | ||
| this.token = token; | ||
| this._tokenChanged = true; | ||
| } | ||
|
|
||
| @BeforeInsert() | ||
| encryptToken(): void { | ||
| this.token = Encryptor.hashDataHMAC(this.token); | ||
| this._tokenChanged = false; | ||
| } | ||
|
|
||
| @BeforeUpdate() | ||
| updateTimestampAndEncryptToken(): void { | ||
| this.updatedAt = new Date(); | ||
| this.token = Encryptor.hashDataHMAC(this.token); | ||
| if (this._tokenChanged) { | ||
| this.token = Encryptor.hashDataHMAC(this.token); | ||
| this._tokenChanged = false; | ||
| } |
There was a problem hiding this comment.
updateTimestampAndEncryptToken() now hashes only when _tokenChanged is set via setToken(). Since token remains a public field, any direct assignment like agent.token = newToken will bypass hashing and can result in plaintext tokens being stored on update. Consider making token effectively write-protected (e.g., private token + getter) and/or tracking changes via @AfterLoad/previous-value comparison so hashing cannot be skipped.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/src/entities/connection-properties/use-cases/update-connection-properties.use.case.ts (1)
39-46:⚠️ Potential issue | 🟡 MinorConsider using
HttpStatus.NOT_FOUNDfor missing resource.
CONNECTION_PROPERTIES_NOT_FOUNDsuggests the resource doesn't exist, which typically warrants a 404 status rather than 400. The AI summary mentions this PR updates error responses from 400 to 404 for resource-not-found scenarios.♻️ Suggested fix
if (!connectionPropertiesToUpdate) { throw new HttpException( { message: Messages.CONNECTION_PROPERTIES_NOT_FOUND, }, - HttpStatus.BAD_REQUEST, + HttpStatus.NOT_FOUND, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/connection-properties/use-cases/update-connection-properties.use.case.ts` around lines 39 - 46, The current check in update-connection-properties.use.case (when connectionPropertiesToUpdate is falsy) throws an HttpException with HttpStatus.BAD_REQUEST; change the status to HttpStatus.NOT_FOUND so the exception for Messages.CONNECTION_PROPERTIES_NOT_FOUND returns a 404. Locate the throw in updateConnectionPropertiesUseCase (or the function containing connectionPropertiesToUpdate) and replace HttpStatus.BAD_REQUEST with HttpStatus.NOT_FOUND.backend/src/entities/amplitude/amplitude.service.ts (1)
62-62:⚠️ Potential issue | 🟡 MinorUse template literal instead of string concatenation.
Proposed fix
- } catch (e) { - console.error('Failed to send log: ' + e); - } + } catch (e) { + console.error(`Failed to send log: ${e}`); + }As per coding guidelines: "Use template literals instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/amplitude/amplitude.service.ts` at line 62, Replace the string concatenation in the error log with a template literal: change the console.error call that currently uses 'Failed to send log: ' + e to use backticks and ${e} (i.e., console.error(`Failed to send log: ${e}`)); update the call in amplitude.service.ts where the error variable e is logged (the console.error within the Amplitude service's send/logging method).backend/src/entities/cron-jobs/cron-jobs.service.ts (1)
163-170:⚠️ Potential issue | 🟡 MinorConstrain
foundEmailsSet type tostringfor type safety.The type annotation
Set<string | Mail.Address>is misleading—nodemailer'sacceptedfield contains only strings. While the current code works, the type allowsMail.Addressobjects that will never occur, creating type confusion. Simplify toSet<string>and remove the type union:Suggested change
-const foundEmails = new Set<string | Mail.Address>(); +const foundEmails = new Set<string>(); filteredResults.forEach((result) => { if (result?.accepted) { result.accepted.forEach((email) => foundEmails.add(email)); } });Additionally, consider normalizing email addresses to lowercase before membership checks to ensure case-insensitive comparison:
const foundEmails = new Set<string>(); filteredResults.forEach((result) => { if (result?.accepted) { - result.accepted.forEach((email) => foundEmails.add(email)); + result.accepted.forEach((email: string) => foundEmails.add(email.toLowerCase())); } }); -const emailsNonFoundInResults = allFoundEmails.filter((email) => !foundEmails.has(email)); +const emailsNonFoundInResults = allFoundEmails.filter((email: string) => !foundEmails.has(email.toLowerCase()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/cron-jobs/cron-jobs.service.ts` around lines 163 - 170, The Set typed as Set<string | Mail.Address> is incorrect — nodemailer's result.accepted contains only strings — so change the foundEmails declaration to a Set<string> and when populating it from filteredResults (use result.accepted) add email.toLowerCase() to the set; similarly when computing emailsNonFoundInResults (using allFoundEmails) compare using email.toLowerCase() or pre-lowercase allFoundEmails so membership checks against foundEmails are case-insensitive; update references to foundEmails, filteredResults/result.accepted, and emailsNonFoundInResults accordingly.
🧹 Nitpick comments (6)
backend/src/entities/company-info/company-info-helper.service.ts (1)
24-27: Good use ofPromise.allfor concurrent query execution.Running the two independent count queries concurrently is a solid performance improvement. The destructuring cleanly captures both results.
One minor naming suggestion: consider renaming the variables to reflect that they hold count values (e.g.,
usersCount,invitationsCount) rather than sounding like function names.,
💡 Optional naming refinement
- const [countUsersInCompany, countInvitationsInCompany] = await Promise.all([ + const [usersCount, invitationsCount] = await Promise.all([ this._dbContext.userRepository.countUsersInCompany(companyId), this._dbContext.invitationInCompanyRepository.countNonExpiredInvitationsInCompany(companyId), ]);Then update line 30:
- return countUsersInCompany + countInvitationsInCompany < Constants.FREE_PLAN_USERS_COUNT; + return usersCount + invitationsCount < Constants.FREE_PLAN_USERS_COUNT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/company-info/company-info-helper.service.ts` around lines 24 - 27, Rename the destructured result variables from countUsersInCompany and countInvitationsInCompany to clearer count names such as usersCount and invitationsCount (these appear where you destructure the Promise.all result in company-info-helper.service.ts and are later referenced when building the response), and update all subsequent usages of those variables throughout the function to the new names to reflect that they hold numeric counts rather than functions.backend/src/entities/connection-properties/utils/sync-table-categories.ts (2)
20-62: Potential stale reference after removal.The
categoriesToRemoveentities are removed from the database on line 24, butexistingCategoriesarray still holds references to them. Although the current filter logic on line 45-46 correctly excludes removed entities (since they won't match anyinputCategories), theexistingCategories.find()on line 50 operates on the original array that may still contain detached entities.Consider clearing or filtering
existingCategoriesafter removal for clarity and safety:♻️ Suggested improvement
if (categoriesToRemove.length > 0) { await tableCategoriesRepository.remove(categoriesToRemove); } + + const remainingCategories = existingCategories.filter( + (existing) => !categoriesToRemove.includes(existing) + ); const categoriesToCreate = inputCategories.filter((input) => { - return !existingCategories.some((existing) => existing.category_id === input.category_id); + return !remainingCategories.some((existing) => existing.category_id === input.category_id); });Then use
remainingCategoriesin place ofexistingCategoriesfor the update logic as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/connection-properties/utils/sync-table-categories.ts` around lines 20 - 62, After removing categoriesToRemove via tableCategoriesRepository.remove, filter out those removed from the existingCategories array (e.g., create a remainingCategories = existingCategories.filter(e => !categoriesToRemove.some(r => r.category_id === e.category_id))) and then use remainingCategories instead of existingCategories when computing categoriesToUpdate and when calling existingCategories.find (or its replacement) to build updatedEntities; this ensures you don't operate on detached/stale entities and keeps the update path consistent with the removal.
5-10: Consider exporting theICategoryInputinterface.This interface mirrors the shape used in
CreateConnectionPropertiesDs.table_categories. Exporting it would allow reuse in the DS definition and ensure type consistency across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/connection-properties/utils/sync-table-categories.ts` around lines 5 - 10, Export the ICategoryInput interface so it can be reused and ensure type consistency with CreateConnectionPropertiesDs.table_categories; update the declaration of ICategoryInput to be exported (e.g., export interface ICategoryInput { ... }) and then import and use this exported type in the DS/type where CreateConnectionPropertiesDs.table_categories is defined so both places share the same ICategoryInput shape.backend/src/entities/connection-properties/application/data-structures/create-connection-properties.ds.ts (1)
15-20: Consider extracting the category shape to a shared interface.This inline type duplicates the
ICategoryInputinterface defined insync-table-categories.ts. Extracting it to a shared location would ensure consistency and reduce duplication.♻️ Suggested approach
Create a shared interface, e.g., in a common types file:
// e.g., in application/data-structures/table-category.interface.ts export interface ITableCategoryInput { category_name: string; tables: Array<string>; category_color: string; category_id: string; }Then use it in both places:
// In CreateConnectionPropertiesDs table_categories?: Array<ITableCategoryInput>; // In syncTableCategories inputCategories: Array<ITableCategoryInput>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/connection-properties/application/data-structures/create-connection-properties.ds.ts` around lines 15 - 20, The inline table_categories shape in CreateConnectionPropertiesDs duplicates ICategoryInput from sync-table-categories.ts; extract a shared interface (e.g., ITableCategoryInput) into a common types file (for example application/data-structures/table-category.interface.ts) with the fields category_name, tables, category_color, category_id, then update create-connection-properties.ds.ts to use Array<ITableCategoryInput> for table_categories and update sync-table-categories.ts (and any other consumers) to reference ITableCategoryInput instead of ICategoryInput or inline types so both modules share the same definition.backend/src/entities/amplitude/amplitude.service.ts (1)
46-46: Redundant null coalescing operator.The
?? 'unknown'fallback is unnecessary here since this code is inside theif (user_email)block on line 43, which guaranteesuser_emailis truthy.Proposed fix
- email: user_email ?? 'unknown', + email: user_email,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/amplitude/amplitude.service.ts` at line 46, Inside the block that checks if user_email is present (the surrounding if (user_email) in amplitude.service.ts), remove the redundant null-coalescing fallback on the payload field; replace the email: user_email ?? 'unknown' usage with a direct email: user_email so the code relies on the existing if check (search for the email property assignment in the Amplitude event construction where user_email is used).backend/src/entities/cron-jobs/cron-jobs.service.ts (1)
159-159: Add explicit callback parameter type in the type guard.The type predicate return is explicit, but the callback parameter is inferred. Add the parameter annotation to align with repo TS typing rules.
Suggested diff
-const filteredResults = results.filter((result): result is ICronMessagingResults => !!result); +const filteredResults = results.filter( + (result: ICronMessagingResults | null): result is ICronMessagingResults => result !== null, +);As per coding guidelines, "Always add type annotations to function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/cron-jobs/cron-jobs.service.ts` at line 159, The filter call uses an inline type guard but the callback parameter is untyped; update the results.filter callback to annotate the parameter (e.g., give the parameter a type like unknown or the appropriate union) so the type predicate "result is ICronMessagingResults" is paired with an explicitly typed parameter; modify the callback used in results.filter (the one producing filteredResults) to include the parameter type annotation to satisfy the repo TS typing rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/entities/agent/agent.entity.ts`:
- Around line 28-47: Direct assignment to the token property bypasses the
_tokenChanged flag and the `@BeforeInsert/`@BeforeUpdate hooks (encryptToken,
updateTimestampAndEncryptToken), causing plaintext tokens to be saved; replace
any direct token writes (e.g., savedAgent.token = token in
custom-agent-repository-extension.ts) with savedAgent.setToken(token) so the
setter sets _tokenChanged and ensures hashing on the next lifecycle hook; audit
the codebase for other direct token assignments and use setToken() instead,
leaving reads unchanged.
In `@backend/src/entities/amplitude/amplitude.service.ts`:
- Line 18: The client property is typed as ReturnType<typeof Amplitude.init> but
it may remain uninitialized when AMPLITUDE_API_KEY is not set; update the
AmplitudeService.client type to include undefined (e.g., ReturnType<typeof
Amplitude.init> | undefined) so the declaration matches runtime behavior,
leaving the existing early-return check in sendLog intact; locate the client
field declaration and adjust its type to allow undefined.
In `@backend/src/entities/cron-jobs/cron-jobs.service.ts`:
- Line 10: Replace the internal nodemailer import used for the Mail type with
the stable public type import: change the import of Mail (currently from
'nodemailer/lib/mailer/index.js') to a type-only import from
'nodemailer/lib/mailer' so the Mail identifier used in cron-jobs.service.ts is
imported as "import type Mail from 'nodemailer/lib/mailer'"; this keeps the Mail
type stable across package updates and avoids relying on internal module paths.
In `@backend/src/entities/user/use-cases/disable-otp.use.case.ts`:
- Around line 34-37: The code assumes foundUserCompany (result of
this._dbContext.companyInfoRepository.findCompanyInfoByUserId(userId)) is
non-null before reading foundUserCompany.is2faEnabled; add a null check so you
only evaluate is2faEnabled when foundUserCompany exists (e.g., if
(!foundUserCompany) return or skip the admin-forbid branch), otherwise proceed
to throw the ForbiddenException(Messages.DISABLING_2FA_FORBIDDEN_BY_ADMIN) only
when foundUserCompany?.is2faEnabled is true; update the logic in
DisableOtpUseCase/disable-otp.use.case.ts accordingly to mirror the pattern used
in ToggleCompanyTestConnectionsDisplayModeUseCase.
In `@backend/src/entities/user/use-cases/request-email-verification.use.case.ts`:
- Around line 43-44: foundUserCompany returned by findCompanyInfoByUserId may be
null, so add a null check before accessing foundUserCompany.id in
request-email-verification.use.case.ts; update the logic around the call to
saasCompanyGatewayService.getCompanyCustomDomainById(foundUserCompany.id) to
first verify foundUserCompany (e.g., if null, either throw a descriptive error
or skip/return appropriate response), and ensure any subsequent code that
assumes company data is only executed when foundUserCompany is non-null.
---
Outside diff comments:
In `@backend/src/entities/amplitude/amplitude.service.ts`:
- Line 62: Replace the string concatenation in the error log with a template
literal: change the console.error call that currently uses 'Failed to send log:
' + e to use backticks and ${e} (i.e., console.error(`Failed to send log:
${e}`)); update the call in amplitude.service.ts where the error variable e is
logged (the console.error within the Amplitude service's send/logging method).
In
`@backend/src/entities/connection-properties/use-cases/update-connection-properties.use.case.ts`:
- Around line 39-46: The current check in update-connection-properties.use.case
(when connectionPropertiesToUpdate is falsy) throws an HttpException with
HttpStatus.BAD_REQUEST; change the status to HttpStatus.NOT_FOUND so the
exception for Messages.CONNECTION_PROPERTIES_NOT_FOUND returns a 404. Locate the
throw in updateConnectionPropertiesUseCase (or the function containing
connectionPropertiesToUpdate) and replace HttpStatus.BAD_REQUEST with
HttpStatus.NOT_FOUND.
In `@backend/src/entities/cron-jobs/cron-jobs.service.ts`:
- Around line 163-170: The Set typed as Set<string | Mail.Address> is incorrect
— nodemailer's result.accepted contains only strings — so change the foundEmails
declaration to a Set<string> and when populating it from filteredResults (use
result.accepted) add email.toLowerCase() to the set; similarly when computing
emailsNonFoundInResults (using allFoundEmails) compare using email.toLowerCase()
or pre-lowercase allFoundEmails so membership checks against foundEmails are
case-insensitive; update references to foundEmails,
filteredResults/result.accepted, and emailsNonFoundInResults accordingly.
---
Nitpick comments:
In `@backend/src/entities/amplitude/amplitude.service.ts`:
- Line 46: Inside the block that checks if user_email is present (the
surrounding if (user_email) in amplitude.service.ts), remove the redundant
null-coalescing fallback on the payload field; replace the email: user_email ??
'unknown' usage with a direct email: user_email so the code relies on the
existing if check (search for the email property assignment in the Amplitude
event construction where user_email is used).
In `@backend/src/entities/company-info/company-info-helper.service.ts`:
- Around line 24-27: Rename the destructured result variables from
countUsersInCompany and countInvitationsInCompany to clearer count names such as
usersCount and invitationsCount (these appear where you destructure the
Promise.all result in company-info-helper.service.ts and are later referenced
when building the response), and update all subsequent usages of those variables
throughout the function to the new names to reflect that they hold numeric
counts rather than functions.
In
`@backend/src/entities/connection-properties/application/data-structures/create-connection-properties.ds.ts`:
- Around line 15-20: The inline table_categories shape in
CreateConnectionPropertiesDs duplicates ICategoryInput from
sync-table-categories.ts; extract a shared interface (e.g., ITableCategoryInput)
into a common types file (for example
application/data-structures/table-category.interface.ts) with the fields
category_name, tables, category_color, category_id, then update
create-connection-properties.ds.ts to use Array<ITableCategoryInput> for
table_categories and update sync-table-categories.ts (and any other consumers)
to reference ITableCategoryInput instead of ICategoryInput or inline types so
both modules share the same definition.
In `@backend/src/entities/connection-properties/utils/sync-table-categories.ts`:
- Around line 20-62: After removing categoriesToRemove via
tableCategoriesRepository.remove, filter out those removed from the
existingCategories array (e.g., create a remainingCategories =
existingCategories.filter(e => !categoriesToRemove.some(r => r.category_id ===
e.category_id))) and then use remainingCategories instead of existingCategories
when computing categoriesToUpdate and when calling existingCategories.find (or
its replacement) to build updatedEntities; this ensures you don't operate on
detached/stale entities and keeps the update path consistent with the removal.
- Around line 5-10: Export the ICategoryInput interface so it can be reused and
ensure type consistency with CreateConnectionPropertiesDs.table_categories;
update the declaration of ICategoryInput to be exported (e.g., export interface
ICategoryInput { ... }) and then import and use this exported type in the
DS/type where CreateConnectionPropertiesDs.table_categories is defined so both
places share the same ICategoryInput shape.
In `@backend/src/entities/cron-jobs/cron-jobs.service.ts`:
- Line 159: The filter call uses an inline type guard but the callback parameter
is untyped; update the results.filter callback to annotate the parameter (e.g.,
give the parameter a type like unknown or the appropriate union) so the type
predicate "result is ICronMessagingResults" is paired with an explicitly typed
parameter; modify the callback used in results.filter (the one producing
filteredResults) to include the parameter type annotation to satisfy the repo TS
typing rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a375b75a-d016-4817-b242-e659b82f7463
📒 Files selected for processing (44)
backend/src/common/application/global-database-context.tsbackend/src/entities/agent/agent.entity.tsbackend/src/entities/agent/agent.module.tsbackend/src/entities/agent/repository/agent.repository.interface.tsbackend/src/entities/agent/repository/custom-agent-repository-extension.tsbackend/src/entities/amplitude/amplitude.service.tsbackend/src/entities/company-info/company-info-helper.service.tsbackend/src/entities/company-info/company-info.controller.tsbackend/src/entities/company-info/invitation-in-company/repository/invitation-in-company-custom-repository-extension.tsbackend/src/entities/company-info/repository/company-info-custom-repository.extension.tsbackend/src/entities/company-info/repository/company-info-repository.interface.tsbackend/src/entities/company-info/use-cases/company-info-use-cases.interface.tsbackend/src/entities/company-info/use-cases/toggle-test-connections-company-display-mode.use.case.tsbackend/src/entities/connection-properties/application/data-structures/create-connection-properties.ds.tsbackend/src/entities/connection-properties/connection-properties.controller.tsbackend/src/entities/connection-properties/connection-properties.entity.tsbackend/src/entities/connection-properties/connection-properties.interface.tsbackend/src/entities/connection-properties/use-cases/update-connection-properties.use.case.tsbackend/src/entities/connection-properties/utils/build-update-connection-properties-object.tsbackend/src/entities/connection-properties/utils/sync-table-categories.tsbackend/src/entities/connection/use-cases/unfreeze-connection.use.case.tsbackend/src/entities/cron-jobs/cron-jobs.service.tsbackend/src/entities/cron-jobs/job-list.entity.tsbackend/src/entities/custom-field/custom-field.controller.tsbackend/src/entities/custom-field/repository/custom-field-repository-extension.tsbackend/src/entities/custom-field/use-cases/create-custom-fields.use.case.tsbackend/src/entities/custom-field/use-cases/delete-custom-field.use.case.tsbackend/src/entities/custom-field/use-cases/update-custom-field.use.case.tsbackend/src/entities/custom-field/utils/build-found-custom-fields-ds.tsbackend/src/entities/custom-field/utils/validate-create-custom-field-dto.tsbackend/src/entities/group/group.controller.tsbackend/src/entities/group/group.module.tsbackend/src/entities/group/repository/group-custom-repository-extension.tsbackend/src/entities/group/repository/group.repository.interface.tsbackend/src/entities/group/use-cases/find-all-users-in-group.use.case.tsbackend/src/entities/group/use-cases/saas-add-user-in-group-v2.use.case.tsbackend/src/entities/user/use-cases/change-usual-password-use.case.tsbackend/src/entities/user/use-cases/disable-otp.use.case.tsbackend/src/entities/user/use-cases/otp-login-use.case.tsbackend/src/entities/user/use-cases/request-email-verification.use.case.tsbackend/src/entities/user/use-cases/usual-login-use.case.tsbackend/src/entities/user/use-cases/verify-reset-user-password.use.case.tsbackend/test/ava-tests/non-saas-tests/non-saas-custom-field-e2e.test.tsbackend/test/ava-tests/saas-tests/custom-field-e2e.test.ts
💤 Files with no reviewable changes (3)
- backend/src/entities/connection-properties/connection-properties.interface.ts
- backend/src/entities/custom-field/utils/validate-create-custom-field-dto.ts
- backend/src/entities/company-info/repository/company-info-repository.interface.ts
Comment on lines
+28
to
+47
| private _tokenChanged = false; | ||
|
|
||
| setToken(token: string): void { | ||
| this.token = token; | ||
| this._tokenChanged = true; | ||
| } | ||
|
|
||
| @BeforeInsert() | ||
| encryptToken(): void { | ||
| this.token = Encryptor.hashDataHMAC(this.token); | ||
| this._tokenChanged = false; | ||
| } | ||
|
|
||
| @BeforeUpdate() | ||
| updateTimestampAndEncryptToken(): void { | ||
| this.updatedAt = new Date(); | ||
| this.token = Encryptor.hashDataHMAC(this.token); | ||
| if (this._tokenChanged) { | ||
| this.token = Encryptor.hashDataHMAC(this.token); | ||
| this._tokenChanged = false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify all token assignments use setToken() method
# Expected: Only setToken() should be used for token assignment, not direct property access
rg -n '\.token\s*=' --type=ts -g '**/agent/**' -g '!*.entity.ts' -C2Repository: rocket-admin/rocketadmin
Length of output: 588
Replace direct token assignment with setToken() method to ensure proper hashing on subsequent updates.
The _tokenChanged flag pattern is correct for preventing double-hashing. However, direct assignment to the token property (savedAgent.token = token) bypasses this mechanism. This happens in backend/src/entities/agent/repository/custom-agent-repository-extension.ts (line 19), where after the agent is saved and the token is hashed during @BeforeInsert, the raw token is directly assigned. On the next update, since _tokenChanged remains false, the token will not be re-hashed and the plaintext value will be stored.
Use setToken() instead:
- savedAgent.token = token;
+ savedAgent.setToken(token);Ensure all token assignments throughout the codebase use setToken() rather than direct property access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/agent/agent.entity.ts` around lines 28 - 47, Direct
assignment to the token property bypasses the _tokenChanged flag and the
`@BeforeInsert/`@BeforeUpdate hooks (encryptToken,
updateTimestampAndEncryptToken), causing plaintext tokens to be saved; replace
any direct token writes (e.g., savedAgent.token = token in
custom-agent-repository-extension.ts) with savedAgent.setToken(token) so the
setter sets _tokenChanged and ensures hashing on the next lifecycle hook; audit
the codebase for other direct token assignments and use setToken() instead,
leaving reads unchanged.
| @Injectable() | ||
| export class AmplitudeService { | ||
| export class AmplitudeService implements OnModuleInit { | ||
| private client: ReturnType<typeof Amplitude.init>; |
There was a problem hiding this comment.
Type annotation should reflect that client can be undefined.
The client property is only initialized when AMPLITUDE_API_KEY is set, but the type annotation doesn't reflect this. While sendLog handles this with an early return check, the type should explicitly include undefined for type safety.
Proposed fix
- private client: ReturnType<typeof Amplitude.init>;
+ private client: ReturnType<typeof Amplitude.init> | undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private client: ReturnType<typeof Amplitude.init>; | |
| private client: ReturnType<typeof Amplitude.init> | undefined; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/amplitude/amplitude.service.ts` at line 18, The client
property is typed as ReturnType<typeof Amplitude.init> but it may remain
uninitialized when AMPLITUDE_API_KEY is not set; update the
AmplitudeService.client type to include undefined (e.g., ReturnType<typeof
Amplitude.init> | undefined) so the declaration matches runtime behavior,
leaving the existing early-return check in sendLog intact; locate the client
field declaration and adjust its type to allow undefined.
| import { Constants } from '../../helpers/constants/constants.js'; | ||
| import { slackPostMessage } from '../../helpers/index.js'; | ||
| import { ValidationHelper } from '../../helpers/validators/validation-helper.js'; | ||
| import Mail from 'nodemailer/lib/mailer/index.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the currently supported Nodemailer version, is nodemailer/lib/mailer/index.jsa public/stable TypeScript import path forMail.Address? What is the recommended import path?
💡 Result:
No. Importing a file path like nodemailer/lib/mailer/index.js is not a public/stable API surface (it’s the kind of internal deep import Nodemailer has historically mentioned only as a workaround for grabbing internal submodules, and it can change between releases). [2]
Recommended type import path (stable within the published typings): import from the module entry nodemailer/lib/mailer and reference Mail.Address (the Address interface is declared under the Mail namespace there). [3][4]
Examples:
// TS (works well with CommonJS-style typings)
import Mail = require('nodemailer/lib/mailer');
type Address = Mail.Address;// If you prefer ESM syntax (with esModuleInterop / allowSyntheticDefaultImports)
import type Mail from 'nodemailer/lib/mailer';
type Address = Mail.Address;Use the public import path for Nodemailer types.
nodemailer/lib/mailer/index.js is an internal path that can break on package updates. Replace with the stable public import:
import type Mail from 'nodemailer/lib/mailer';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/cron-jobs/cron-jobs.service.ts` at line 10, Replace the
internal nodemailer import used for the Mail type with the stable public type
import: change the import of Mail (currently from
'nodemailer/lib/mailer/index.js') to a type-only import from
'nodemailer/lib/mailer' so the Mail identifier used in cron-jobs.service.ts is
imported as "import type Mail from 'nodemailer/lib/mailer'"; this keeps the Mail
type stable across package updates and avoids relying on internal module paths.
Comment on lines
+34
to
37
| const foundUserCompany = await this._dbContext.companyInfoRepository.findCompanyInfoByUserId(userId); | ||
| if (foundUserCompany.is2faEnabled) { | ||
| throw new ForbiddenException(Messages.DISABLING_2FA_FORBIDDEN_BY_ADMIN); | ||
| } |
There was a problem hiding this comment.
Add null check for foundUserCompany to prevent potential null pointer dereference.
If findCompanyInfoByUserId returns null (e.g., user has no associated company), accessing foundUserCompany.is2faEnabled at line 35 will throw a runtime TypeError. For consistency with other use cases (e.g., ToggleCompanyTestConnectionsDisplayModeUseCase), add a null check.
🛡️ Proposed fix
const foundUserCompany = await this._dbContext.companyInfoRepository.findCompanyInfoByUserId(userId);
+ if (!foundUserCompany) {
+ throw new NotFoundException(Messages.COMPANY_NOT_FOUND);
+ }
if (foundUserCompany.is2faEnabled) {
throw new ForbiddenException(Messages.DISABLING_2FA_FORBIDDEN_BY_ADMIN);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const foundUserCompany = await this._dbContext.companyInfoRepository.findCompanyInfoByUserId(userId); | |
| if (foundUserCompany.is2faEnabled) { | |
| throw new ForbiddenException(Messages.DISABLING_2FA_FORBIDDEN_BY_ADMIN); | |
| } | |
| const foundUserCompany = await this._dbContext.companyInfoRepository.findCompanyInfoByUserId(userId); | |
| if (!foundUserCompany) { | |
| throw new NotFoundException(Messages.COMPANY_NOT_FOUND); | |
| } | |
| if (foundUserCompany.is2faEnabled) { | |
| throw new ForbiddenException(Messages.DISABLING_2FA_FORBIDDEN_BY_ADMIN); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/user/use-cases/disable-otp.use.case.ts` around lines 34
- 37, The code assumes foundUserCompany (result of
this._dbContext.companyInfoRepository.findCompanyInfoByUserId(userId)) is
non-null before reading foundUserCompany.is2faEnabled; add a null check so you
only evaluate is2faEnabled when foundUserCompany exists (e.g., if
(!foundUserCompany) return or skip the admin-forbid branch), otherwise proceed
to throw the ForbiddenException(Messages.DISABLING_2FA_FORBIDDEN_BY_ADMIN) only
when foundUserCompany?.is2faEnabled is true; update the logic in
DisableOtpUseCase/disable-otp.use.case.ts accordingly to mirror the pattern used
in ToggleCompanyTestConnectionsDisplayModeUseCase.
Comment on lines
+43
to
44
| const foundUserCompany = await this._dbContext.companyInfoRepository.findCompanyInfoByUserId(foundUser.id); | ||
| const companyCustomDomain = await this.saasCompanyGatewayService.getCompanyCustomDomainById(foundUserCompany.id); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other similar use cases have null checks for foundUserCompany
rg -n -A3 'findCompanyInfoByUserId' --glob '*.ts' | head -50Repository: rocket-admin/rocketadmin
Length of output: 5534
🏁 Script executed:
# Find the companyInfoRepository definition
fd -t f '*.ts' | xargs grep -l 'companyInfoRepository' | head -5Repository: rocket-admin/rocketadmin
Length of output: 351
🏁 Script executed:
# Search for the findCompanyInfoByUserId method definition
rg -n 'findCompanyInfoByUserId' --type ts -B2 -A5 | grep -E '(interface|class|async|findCompanyInfoByUserId)' | head -30Repository: rocket-admin/rocketadmin
Length of output: 4476
🏁 Script executed:
# Look for the repository implementation
fd -t f -name '*repository*.ts' | xargs grep -l 'findCompanyInfoByUserId'Repository: rocket-admin/rocketadmin
Length of output: 300
🏁 Script executed:
# Check the full context of request-email-verification.use.case.ts
cat -n backend/src/entities/user/use-cases/request-email-verification.use.case.ts | head -60Repository: rocket-admin/rocketadmin
Length of output: 2563
🏁 Script executed:
cat -n backend/src/entities/company-info/repository/company-info-custom-repository.extension.ts | sed -n '25,50p'Repository: rocket-admin/rocketadmin
Length of output: 1318
🏁 Script executed:
# Also check the interface to confirm return type
cat -n backend/src/entities/company-info/repository/company-info-repository.interface.tsRepository: rocket-admin/rocketadmin
Length of output: 1602
Add null check for foundUserCompany before accessing its properties.
The findCompanyInfoByUserId() method uses TypeORM's .getOne() which returns null if no company is associated with the user. Line 44 accesses foundUserCompany.id without a guard, causing a runtime TypeError if no company exists.
Suggested fix
const foundUserCompany = await this._dbContext.companyInfoRepository.findCompanyInfoByUserId(foundUser.id);
+ if (!foundUserCompany) {
+ throw new HttpException(
+ {
+ message: Messages.COMPANY_NOT_FOUND,
+ },
+ HttpStatus.NOT_FOUND,
+ );
+ }
const companyCustomDomain = await this.saasCompanyGatewayService.getCompanyCustomDomainById(foundUserCompany.id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const foundUserCompany = await this._dbContext.companyInfoRepository.findCompanyInfoByUserId(foundUser.id); | |
| const companyCustomDomain = await this.saasCompanyGatewayService.getCompanyCustomDomainById(foundUserCompany.id); | |
| const foundUserCompany = await this._dbContext.companyInfoRepository.findCompanyInfoByUserId(foundUser.id); | |
| if (!foundUserCompany) { | |
| throw new HttpException( | |
| { | |
| message: Messages.COMPANY_NOT_FOUND, | |
| }, | |
| HttpStatus.NOT_FOUND, | |
| ); | |
| } | |
| const companyCustomDomain = await this.saasCompanyGatewayService.getCompanyCustomDomainById(foundUserCompany.id); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/user/use-cases/request-email-verification.use.case.ts`
around lines 43 - 44, foundUserCompany returned by findCompanyInfoByUserId may
be null, so add a null check before accessing foundUserCompany.id in
request-email-verification.use.case.ts; update the logic around the call to
saasCompanyGatewayService.getCompanyCustomDomainById(foundUserCompany.id) to
first verify foundUserCompany (e.g., if null, either throw a descriptive error
or skip/return appropriate response), and ensure any subsequent code that
assumes company data is only executed when foundUserCompany is non-null.
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Bug Fixes
Improvements