Conversation
…methods and improve error handling
…update related data structures
…r improved type safety
…DTO properties for optional fields
…RepositoryExtension type definition
📝 WalkthroughWalkthroughThe PR refactors the connection entity layer by extracting inline connection parameters into a dedicated data structure, fixing a classname typo, removing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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 refactors connection-related TypeScript types across the backend to reduce unsafe casts, improve nullability correctness, and align DTOs/entities/use-cases with shared ConnectionTypesEnum.
Changes:
- Introduces a shared
ConnectionParametersDstype and wires it into create/update connection data structures. - Tightens/clarifies types across repositories, entities, DTOs, and use-cases (e.g.,
unknowncatches,ConnectionTypesEnumusage, nullable returns). - Renames a misspelled DS type (
FoundSipleConnectionInfoDS→FoundSimpleConnectionInfoDS) and propagates the rename.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/entities/connection/utils/validate-create-connection-data.ts | Makes validateConnectionType return a boolean consistently. |
| backend/src/entities/connection/utils/update-connection-entity-for-restoration.ts | Switches agent-check helper to a type-based predicate. |
| backend/src/entities/connection/utils/build-created-connection.ds.ts | Allows nullable/optional token and masterPwd inputs. |
| backend/src/entities/connection/use-cases/use-cases.interfaces.ts | Updates IFindConnections.execute to accept FindUserDs instead of CreateUserDs. |
| backend/src/entities/connection/use-cases/test-connection.use.case.ts | Improves error typing/formatting; adjusts AWS-processing call sites (currently via unsafe cast). |
| backend/src/entities/connection/use-cases/find-one-connection.use.case.ts | Types caught exceptions as unknown. |
| backend/src/entities/connection/use-cases/find-all-users-in-connection.use.case.ts | Adds a relation-omitting user type alias; still relies on a cast for DTO building. |
| backend/src/entities/connection/use-cases/find-all-connections.use.case.ts | Narrows generic input to FindUserDs only. |
| backend/src/entities/connection/use-cases/create-connection.use.case.ts | Makes connectionCopy nullable and guards its usage in finally. |
| backend/src/entities/connection/repository/custom-connection-repository-extension.ts | Improves this typing for repository extension; makes several fetches nullable. |
| backend/src/entities/connection/repository/connection.repository.interface.ts | Updates signatures (string param types + nullable return types). |
| backend/src/entities/connection/connection.module.ts | Refines configure return type to void. |
| backend/src/entities/connection/connection.entity.ts | Types type as `ConnectionTypesEnum |
| backend/src/entities/connection/connection.controller.ts | Removes unused permission interface import; tightens local typings and return types. |
| backend/src/entities/connection/application/dto/delete-connection.dto.ts | Makes reason/message optional and updates Swagger metadata accordingly. |
| backend/src/entities/connection/application/dto/created-connection.dto.ts | Restricts type to ConnectionTypesEnum only. |
| backend/src/entities/connection/application/data-structures/update-connection.ds.ts | Replaces inline connection_parameters shape with ConnectionParametersDs. |
| backend/src/entities/connection/application/data-structures/found-connections.ds.ts | Tightens types (esp. type/author) and fixes DS naming typo. |
| backend/src/entities/connection/application/data-structures/create-connection.ds.ts | Replaces inline connection_parameters shape with ConnectionParametersDs. |
| backend/src/entities/connection/application/data-structures/connection-parameters.ds.ts | Adds the shared connection-parameters interface. |
| backend/src/entities/company-info/utils/build-found-company-info-ds.ts | Updates imports/usages for renamed FoundSimpleConnectionInfoDS. |
| backend/src/entities/company-info/application/data-structures/found-company-info.ds.ts | Updates imports/usages for renamed FoundSimpleConnectionInfoDS. |
Comments suppressed due to low confidence (1)
backend/src/entities/connection/application/data-structures/create-connection.ds.ts:8
CreateConnectionDsnow depends onConnectionParametersDs, where most fields are optional. This significantly weakens compile-time guarantees compared to the previous inline type for create flows (e.g.,host/port/usernamecan be omitted without a type error). Consider splitting into separate parameter types for create vs update (required vs partial) or usingRequired<...>for the fields that must be present during creation.
export class CreateConnectionDs {
connection_parameters: ConnectionParametersDs;
creation_info: {
authorId: string;
masterPwd: string;
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| removeConnection(connection: ConnectionEntity): Promise<ConnectionEntity>; | ||
|
|
||
| findConnectionWithGroups(connectionId: string): Promise<ConnectionEntity>; | ||
| findConnectionWithGroups(connectionId: string): Promise<ConnectionEntity | null>; |
There was a problem hiding this comment.
The repository contract was changed to allow null returns (e.g., findConnectionWithGroups). At least some call sites assume a non-null connection and immediately dereference (e.g., CreateGroupInConnectionUseCase uses .groups without a null check). Either keep these methods non-null and throw on missing records, or update all callers to handle null explicitly to avoid runtime crashes.
Suggested change
| findConnectionWithGroups(connectionId: string): Promise<ConnectionEntity | null>; | |
| findConnectionWithGroups(connectionId: string): Promise<ConnectionEntity>; |
Comment on lines
112
to
115
| const dataForProcessing: CreateConnectionDs = { | ||
| connection_parameters: updated, | ||
| creation_info: null, | ||
| creation_info: null as unknown as CreateConnectionDs['creation_info'], | ||
| }; |
There was a problem hiding this comment.
processAWSConnection only uses connection_parameters, but this use case is now passing creation_info via null as unknown as ..., which masks a real type mismatch and can hide future bugs. Consider changing processAWSConnection to accept a narrower type (e.g., { connection_parameters: ... }) or making CreateConnectionDs['creation_info'] optional/nullable so callers don't need an unsafe cast.
| const dataForProcessing: CreateConnectionDs = { | ||
| connection_parameters: connectionData, | ||
| creation_info: null, | ||
| creation_info: null as unknown as CreateConnectionDs['creation_info'], |
There was a problem hiding this comment.
Same issue as above: creation_info is forced to null via a double cast. It would be safer to adjust the processAWSConnection/CreateConnectionDs types so this call site can pass a correctly typed object without as unknown as ....
Suggested change
| creation_info: null as unknown as CreateConnectionDs['creation_info'], | |
| creation_info: null as CreateConnectionDs['creation_info'], |
Comment on lines
+25
to
28
| const userInConnection: UserWithoutRelations[] = | ||
| await this._dbContext.userRepository.findAllUsersInConnection(connectionId); | ||
| return userInConnection.map((user) => buildFoundUserDto(user as UserEntity)); | ||
| } |
There was a problem hiding this comment.
UserWithoutRelations is introduced to improve typing, but the user as UserEntity cast reintroduces an unchecked assumption and defeats the benefit. Since buildFoundUserDto only needs scalar fields, consider widening its parameter type (e.g., Pick<UserEntity, ...>) so this mapping can be done without a cast.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/entities/company-info/utils/build-found-company-info-ds.ts (1)
29-47:⚠️ Potential issue | 🟡 MinorMissing
typefield in connection mapping.The
FoundSimpleConnectionInfoDStype includes atype?: ConnectionTypesEnumproperty (perfound-connections.ds.tslines 144-145), but the mapping here doesn't include it. If this field should be populated fromconnection.type, it's missing.Proposed fix to include type field
const connectionsRO: Array<FoundSimpleConnectionInfoDS> = companyInfoFromCore.connections.map((connection) => { return { id: connection.id, createdAt: connection.createdAt, updatedAt: connection.updatedAt, title: connection.title, + type: connection.type, isTestConnection: connection.isTestConnection, author: buildSimpleUserInfoDs(connection.author),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/entities/company-info/utils/build-found-company-info-ds.ts` around lines 29 - 47, The mapping that builds connectionsRO for companyInfoFromCore.connections is missing the optional `type` property defined on FoundSimpleConnectionInfoDS; update the object returned in the map (inside the connectionsRO creation) to include `type: connection.type` so each connection entry includes ConnectionTypesEnum when present, ensuring other fields (id, createdAt, updatedAt, title, isTestConnection, author via buildSimpleUserInfoDs, and groups) remain unchanged.
🤖 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/connection/use-cases/test-connection.use.case.ts`:
- Around line 112-115: The code constructs dataForProcessing: CreateConnectionDs
with creation_info set via an unsafe cast (null as unknown as ...); instead
build and pass a real creation_info object using the available
inputData.update_info fields (e.g. inputData.update_info.authorId and
inputData.update_info.masterPwd) and any other required properties for
CreateConnectionDs.creation_info, replacing the null cast in dataForProcessing
(and the similar instance at lines ~144-147) so the type system is satisfied and
no unsafe cast is used.
---
Outside diff comments:
In `@backend/src/entities/company-info/utils/build-found-company-info-ds.ts`:
- Around line 29-47: The mapping that builds connectionsRO for
companyInfoFromCore.connections is missing the optional `type` property defined
on FoundSimpleConnectionInfoDS; update the object returned in the map (inside
the connectionsRO creation) to include `type: connection.type` so each
connection entry includes ConnectionTypesEnum when present, ensuring other
fields (id, createdAt, updatedAt, title, isTestConnection, author via
buildSimpleUserInfoDs, and groups) remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a831b1fc-f2d9-4ddd-98f8-f7d0e864e4ef
📒 Files selected for processing (22)
backend/src/entities/company-info/application/data-structures/found-company-info.ds.tsbackend/src/entities/company-info/utils/build-found-company-info-ds.tsbackend/src/entities/connection/application/data-structures/connection-parameters.ds.tsbackend/src/entities/connection/application/data-structures/create-connection.ds.tsbackend/src/entities/connection/application/data-structures/found-connections.ds.tsbackend/src/entities/connection/application/data-structures/update-connection.ds.tsbackend/src/entities/connection/application/dto/created-connection.dto.tsbackend/src/entities/connection/application/dto/delete-connection.dto.tsbackend/src/entities/connection/connection.controller.tsbackend/src/entities/connection/connection.entity.tsbackend/src/entities/connection/connection.module.tsbackend/src/entities/connection/repository/connection.repository.interface.tsbackend/src/entities/connection/repository/custom-connection-repository-extension.tsbackend/src/entities/connection/use-cases/create-connection.use.case.tsbackend/src/entities/connection/use-cases/find-all-connections.use.case.tsbackend/src/entities/connection/use-cases/find-all-users-in-connection.use.case.tsbackend/src/entities/connection/use-cases/find-one-connection.use.case.tsbackend/src/entities/connection/use-cases/test-connection.use.case.tsbackend/src/entities/connection/use-cases/use-cases.interfaces.tsbackend/src/entities/connection/utils/build-created-connection.ds.tsbackend/src/entities/connection/utils/update-connection-entity-for-restoration.tsbackend/src/entities/connection/utils/validate-create-connection-data.ts
Comment on lines
112
to
115
| const dataForProcessing: CreateConnectionDs = { | ||
| connection_parameters: updated, | ||
| creation_info: null, | ||
| creation_info: null as unknown as CreateConnectionDs['creation_info'], | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether processAWSConnection currently reads creation_info fields.
fd process-aws-connection.util.ts --exec sed -n '1,220p' {}
rg -n -C3 '\bcreation_info\b|\bauthorId\b|\bmasterPwd\b' backend/src/entities/connection/utils/process-aws-connection.util.tsRepository: rocket-admin/rocketadmin
Length of output: 1693
🏁 Script executed:
cat -n backend/src/entities/connection/use-cases/test-connection.use.case.ts | sed -n '100,160p'Repository: rocket-admin/rocketadmin
Length of output: 2296
🏁 Script executed:
rg -n "CreateConnectionDs" backend/src/entities/connection/application/data-structures/ -A 15Repository: rocket-admin/rocketadmin
Length of output: 816
🏁 Script executed:
rg -n "creation_info" backend/src/entities/connection/ --type ts -B 2 -A 2 | head -100Repository: rocket-admin/rocketadmin
Length of output: 5040
🏁 Script executed:
cat -n backend/src/entities/connection/use-cases/test-connection.use.case.ts | sed -n '1,50p'Repository: rocket-admin/rocketadmin
Length of output: 2639
🏁 Script executed:
rg -n "TestConnectionInputData\|TestConnectionDs\|interface.*Test.*Connection" backend/src/entities/connection/ -B 2 -A 10 --type tsRepository: rocket-admin/rocketadmin
Length of output: 50
🏁 Script executed:
cat -n backend/src/entities/connection/use-cases/test-connection.use.case.ts | sed -n '160,180p'Repository: rocket-admin/rocketadmin
Length of output: 1017
🏁 Script executed:
rg -n "class UpdateConnectionDs\|export.*UpdateConnectionDs" backend/src/entities/connection/ -A 10 --type tsRepository: rocket-admin/rocketadmin
Length of output: 50
🏁 Script executed:
cat -n backend/src/entities/connection/use-cases/test-connection.use.case.ts | sed -n '35,50p'Repository: rocket-admin/rocketadmin
Length of output: 663
🏁 Script executed:
fd "update-connection.ds.ts" backend/src/entities/connection/ --exec cat -n {}Repository: rocket-admin/rocketadmin
Length of output: 379
🏁 Script executed:
fd "*.ts" backend/src/entities/connection/application/data-structures/ --exec basename {} \; | sortRepository: rocket-admin/rocketadmin
Length of output: 351
Avoid null as unknown as for required creation_info.
Lines 114 and 146 bypass the type system with unsafe casts. Since inputData.update_info.authorId and inputData.update_info.masterPwd are available, pass a real creation_info object instead:
Suggested patch
const dataForProcessing: CreateConnectionDs = {
connection_parameters: updated,
- creation_info: null as unknown as CreateConnectionDs['creation_info'],
+ creation_info: {
+ authorId: inputData.update_info.authorId,
+ masterPwd: inputData.update_info.masterPwd,
+ },
};
@@
const dataForProcessing: CreateConnectionDs = {
connection_parameters: connectionData,
- creation_info: null as unknown as CreateConnectionDs['creation_info'],
+ creation_info: {
+ authorId: inputData.update_info.authorId,
+ masterPwd: inputData.update_info.masterPwd,
+ },
};Also applies to: 144-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/entities/connection/use-cases/test-connection.use.case.ts` around
lines 112 - 115, The code constructs dataForProcessing: CreateConnectionDs with
creation_info set via an unsafe cast (null as unknown as ...); instead build and
pass a real creation_info object using the available inputData.update_info
fields (e.g. inputData.update_info.authorId and inputData.update_info.masterPwd)
and any other required properties for CreateConnectionDs.creation_info,
replacing the null cast in dataForProcessing (and the similar instance at lines
~144-147) so the type system is satisfied and no unsafe cast is used.
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
Bug Fixes
Refactor
reasonandmessagefields optional when deleting connections, allowing operations to proceed without providing additional details.