Closed PriyobrotoKar closed 1 month ago
β±οΈ Estimated effort to review: 2 π΅π΅βͺβͺβͺ |
π§ͺ No relevant tests |
π No security concerns identified |
β‘ Key issues to review API Change The PR changes the API response by replacing 'id' with 'slug'. This might break existing client implementations that rely on the 'id' field. |
Category | Suggestion | Score |
Maintainability |
β Extract common search logic into a separate method to reduce code duplication___ **Consider extracting the common search logic into a separate method to reduce codeduplication across similar queries.** [apps/api/src/workspace/service/workspace.service.ts [439-445]](https://github.com/keyshade-xyz/keyshade/pull/455/files#diff-a92613f23cf2834d52eaedb87c4e8c460d48b6fca8df4ccc968b2a8af19a9c3fR439-R445) ```diff -{ - where: { - id: { in: projectIds }, - OR: [ - { name: { contains: searchTerm, mode: 'insensitive' } }, - { description: { contains: searchTerm, mode: 'insensitive' } } - ] - }, - select: { slug: true, name: true, description: true } -} +this.createSearchQuery(projectIds, searchTerm, ['name', 'description']) ``` `[Suggestion has been applied]` Suggestion importance[1-10]: 8Why: Extracting common logic into a separate method significantly improves maintainability by reducing code duplication and enhancing readability. | 8 |
Type safety |
Add a type annotation to the query's select clause for improved type safety___ **Consider adding a type annotation for the return value of the query to ensure typesafety and improve code readability.** [apps/api/src/workspace/service/workspace.service.ts [445]](https://github.com/keyshade-xyz/keyshade/pull/455/files#diff-a92613f23cf2834d52eaedb87c4e8c460d48b6fca8df4ccc968b2a8af19a9c3fR445-R445) ```diff -select: { slug: true, name: true, description: true } +select: { slug: true, name: true, description: true } as const ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 5Why: Adding a type annotation can improve type safety and readability, but the suggestion does not address a critical issue and the improvement is minor. | 5 |
π‘ Need additional feedback ? start a PR chat
Oh ok. Like in that global search test, I have to create some dummy resources (projects, environments, secrets and variables) and then make sure that the response array's length for each matches. Right?
All of that work is already done! You just need to update the response types both in the test and types file.
@PriyobrotoKar can you please apply this suggestion?
Yes sure! Doing it rn.
:tada: This PR is included in version 2.6.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:
User description
Description
Updated the response of the global search API with slug instead of id.
Fixes #426
Dependencies
Mention any dependencies/packages used
Future Improvements
Mention any improvements to be done in future related to any file/feature
Mentions
Mention and tag the people
Screenshots of relevant screens
Add screenshots of relevant screens
Developer's checklist
If changes are made in the code:
Documentation Update
PR Type
Bug fix
Description
slug
instead ofid
in the response.WorkspaceService
to selectslug
instead ofid
.Changes walkthrough π
workspace.service.ts
Replace `id` with `slug` in global search queries
apps/api/src/workspace/service/workspace.service.ts
id
withslug
in the selection fields of multiple querymethods.
slug
instead ofid
.