Closed Sambit003 closed 2 weeks ago
โฑ๏ธ Estimated effort to review: 4 ๐ต๐ต๐ต๐ตโช |
๐งช PR contains tests |
๐ No security concerns identified |
โก Key issues to review Merge Conflict There are unresolved merge conflicts in this file that need to be addressed. API Changes Several API endpoints have been updated to use slugs instead of IDs. This change needs to be carefully reviewed for consistency and potential impacts on other parts of the system. Test Coverage New tests have been added for variable operations. These should be reviewed to ensure they cover all necessary scenarios and edge cases. |
Category | Suggestion | Score |
Possible issue |
Resolve merge conflict by selecting the appropriate code___ **Resolve the merge conflict by choosing the appropriate code and removing theconflict markers. This will ensure the code can be properly executed and maintained.** [apps/api/src/secret/secret.e2e.spec.ts [960-964]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-598db59202ce7ecc8d2fb84528577afd14eac8275a30e174d704d6fc2b7f5c3bR960-R964) ```diff -<<<<<<< HEAD - await secretService.updateSecret(user1, secret1.slug, { -======= - expect(response.statusCode).toBe(404) -}) +await secretService.updateSecret(user1, secret1.slug, { -it('should have created a SECRET_UPDATED event', async () => { - ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 10Why: Resolving merge conflicts is crucial for ensuring the code can be properly executed and maintained, making this suggestion highly important. | 10 |
Merge conflict |
Resolve merge conflict by selecting the updated import statements___ **Remove the merge conflict markers and resolve the conflict by choosing theappropriate code.** [apps/api/src/project/project.e2e.spec.ts [28-48]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-f1b22baab0b4173d825570c9030ef014582bc75bb67ef2e544f0dd879ff941a2R28-R48) ```diff -<<<<<<< HEAD import { WorkspaceService } from '@/workspace/service/workspace.service' import { WorkspaceMembershipService } from '@/workspace-membership/service/workspace-membership.service' import { UserService } from '@/user/service/user.service' import { WorkspaceModule } from '@/workspace/workspace.module' import { WorkspaceMembershipModule } from '@/workspace-membership/workspace-membership.module' import { UserModule } from '@/user/user.module' import { WorkspaceRoleModule } from '@/workspace-role/workspace-role.module' import { WorkspaceRoleService } from '@/workspace-role/service/workspace-role.service' import { EnvironmentService } from '@/environment/service/environment.service' import { SecretService } from '@/secret/service/secret.service' import { VariableService } from '@/variable/service/variable.service' import { VariableModule } from '@/variable/variable.module' import { SecretModule } from '@/secret/secret.module' import { EnvironmentModule } from '@/environment/environment.module' import { QueryTransformPipe } from '@/common/pipes/query.transform.pipe' import { fetchEvents } from '@/common/event' -======= -import { WorkspaceService } from '../workspace/service/workspace.service' -import { UserService } from '../user/service/user.service' -import { WorkspaceModule } from '../workspace/workspace.module' ->>>>>>> 6ac6f14 (Revert "Fix: merge conflicts") ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: The suggestion correctly identifies and resolves the merge conflict by selecting the updated import statements, which is crucial for the code to compile and function correctly. | 9 |
Resolve merge conflict in the 'Create Project Tests' section___ **Remove the merge conflict markers and resolve the conflict by choosing theappropriate code for the 'Create Project Tests' section.** [apps/api/src/project/project.e2e.spec.ts [179-195]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-f1b22baab0b4173d825570c9030ef014582bc75bb67ef2e544f0dd879ff941a2R179-R195) ```diff -<<<<<<< HEAD describe('Create Project Tests', () => { it('should allow workspace member to create a project', async () => { const response = await app.inject({ method: 'POST', url: `/project/${workspace1.slug}`, payload: { name: 'Project 3', description: 'Project 3 description', storePrivateKey: true }, headers: { 'x-e2e-user-email': user1.email -======= -it('should allow workspace member to create a project', async () => { - const response = await app.inject({ - method: 'POST', + } + }) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: The suggestion accurately resolves the merge conflict in the 'Create Project Tests' section, ensuring the test case is complete and functional. | 9 | |
Resolve merge conflict in the 'Key Tests' section___ **Remove the merge conflict markers and resolve the conflict by choosing theappropriate code for the 'Key Tests' section.** [apps/api/src/project/project.e2e.spec.ts [805-819]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-f1b22baab0b4173d825570c9030ef014582bc75bb67ef2e544f0dd879ff941a2R805-R819) ```diff -<<<<<<< HEAD describe('Key Tests', () => { it('should not store the private key if storePrivateKey is false', async () => { const response = await app.inject({ method: 'POST', url: `/project/${workspace1.slug}`, payload: { name: 'Project 2', description: 'Project 2 description', storePrivateKey: false }, headers: { 'x-e2e-user-email': user1.email -======= -it('should generate new key-pair if regenerateKeyPair is true and and the project stores the private key or a private key is specified', async () => { - const response = await app.inject({ - method: 'PUT', + } + }) + expect(response.statusCode).toBe(201) + + const projectId = response.json().id + + project2 = await prisma.project.findUnique({ + where: { + id: projectId + } + }) + + expect(project2).toBeDefined() + expect(project2.privateKey).toBeNull() + }) + + it('should generate new key-pair if regenerateKeyPair is true and and the project stores the private key or a private key is specified', async () => { + const response = await app.inject({ + method: 'PUT', + ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: The suggestion effectively resolves the merge conflict in the 'Key Tests' section, maintaining the integrity and functionality of the test cases. | 9 | |
Best practice |
Remove unused variable to improve code cleanliness___ **Remove the unused variableresponse to improve code cleanliness and avoid potential linting issues.** [apps/api/src/secret/secret.e2e.spec.ts [1106-1107]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-598db59202ce7ecc8d2fb84528577afd14eac8275a30e174d704d6fc2b7f5c3bR1106-R1107) ```diff -// eslint-disable-next-line @typescript-eslint/no-unused-vars -const response = await app.inject({ +await app.inject({ ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Removing an unused variable helps avoid potential linting issues and improves code cleanliness, which is a good practice for maintainability. | 8 |
Use a more descriptive variable name for better code readability___ **Use a more descriptive variable name instead ofversions to improve code readability. Consider using secretVersions or secretRevisions .**
[apps/api/src/secret/secret.e2e.spec.ts [1095]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-598db59202ce7ecc8d2fb84528577afd14eac8275a30e174d704d6fc2b7f5c3bR1095-R1095)
```diff
-let versions: SecretVersion[]
+let secretVersions: SecretVersion[]
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 6Why: The suggestion to use a more descriptive variable name like `secretVersions` improves code readability and maintainability, but it is a minor improvement. | 6 | |
Use more descriptive variable names for test users___ **Consider using a more descriptive name for theuser1 , user2 , and user3 variables. For example, you could use names like adminUser , memberUser , and nonMemberUser to better reflect their roles in the tests.** [apps/api/src/workspace/workspace.e2e.spec.ts [78]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-7a691e843fbdae323f1cabe7a12508154fbc917a1a738e5a67a1747fb7c581eaR78-R78) ```diff -let user1: User, user2: User, user3: User +let adminUser: User, memberUser: User, nonMemberUser: User ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: The suggestion improves code readability by using descriptive variable names, which helps in understanding the roles of the users in the tests. However, it is not crucial for functionality. | 6 | |
Use specific HTTP status code constants in assertions___ **Consider using a more specific assertion for the response status code. Instead ofexpect(response.statusCode).toBe(201) , you could use expect(response.statusCode).toBe(HttpStatus.CREATED) to make the expected status code more explicit.** [apps/api/src/workspace/workspace.e2e.spec.ts [200]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-7a691e843fbdae323f1cabe7a12508154fbc917a1a738e5a67a1747fb7c581eaR200-R200) ```diff -expect(response.statusCode).toBe(201) +expect(response.statusCode).toBe(HttpStatus.CREATED) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 5Why: Using HTTP status code constants can improve code readability and maintainability by making the expected status more explicit. However, it is a minor improvement and not essential for the test's correctness. | 5 | |
Enhancement |
Add a test case for invalid workspace creation input___ **Consider adding a test case to verify that a user cannot create a workspace with anempty name or description. This would help ensure that the API properly validates input data.** [apps/api/src/workspace/workspace.e2e.spec.ts [187-199]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-7a691e843fbdae323f1cabe7a12508154fbc917a1a738e5a67a1747fb7c581eaR187-R199) ```diff it('should be able to create a new workspace', async () => { const response = await app.inject({ method: 'POST', headers: { 'x-e2e-user-email': user1.email }, url: '/workspace', payload: { name: 'Workspace 1', description: 'Workspace 1 description' } }) +it('should not be able to create a workspace with empty name or description', async () => { + const response = await app.inject({ + method: 'POST', + headers: { + 'x-e2e-user-email': user1.email + }, + url: '/workspace', + payload: { + name: '', + description: '' + } + }) + + expect(response.statusCode).toBe(HttpStatus.BAD_REQUEST) +}) + ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Adding a test case for invalid input enhances the test coverage and ensures that the API properly validates input data, which is important for robustness and security. | 8 |
Maintainability |
Remove commented-out code to improve code cleanliness___ **Remove the commented-out code to improve code cleanliness and maintainability. Ifthe code is needed for future reference, consider adding a TODO comment instead.** [apps/api/src/secret/secret.e2e.spec.ts [1115-1124]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-598db59202ce7ecc8d2fb84528577afd14eac8275a30e174d704d6fc2b7f5c3bR1115-R1124) ```diff -// expect(response.statusCode).toBe(200) -// expect(response.json().count).toEqual(2) +// TODO: Implement assertion for response status and secret versions count -// versions = await prisma.secretVersion.findMany({ -// where: { -// secretId: secret1.id -// } -// }) - -// expect(versions.length).toBe(1) - ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Removing commented-out code enhances code cleanliness and maintainability, and replacing it with a TODO comment is a good practice for future reference. | 7 |
Error handling |
Add error handling for the fetchEvents function call___ **Consider adding error handling and validation for thefetchEvents function call. It's good practice to handle potential errors that might occur during API calls or data fetching.** [apps/api/src/workspace/workspace.e2e.spec.ts [363-368]](https://github.com/keyshade-xyz/keyshade/pull/422/files#diff-7a691e843fbdae323f1cabe7a12508154fbc917a1a738e5a67a1747fb7c581eaR363-R368) ```diff -const response = await fetchEvents( - eventService, - user1, - workspace1.slug, - EventSource.WORKSPACE -) +let response; +try { + response = await fetchEvents( + eventService, + user1, + workspace1.slug, + EventSource.WORKSPACE + ); +} catch (error) { + console.error('Error fetching events:', error); + throw error; +} +expect(response).toBeDefined(); + ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Incorporating error handling improves the robustness of the test by ensuring that potential errors during API calls are managed, but it is not critical for the test's primary functionality. | 7 |
User description
Description
Give a summary of the change that you have made
Fixes #[ISSUENO]
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
enhancement, tests, documentation
Description
AdminGuard
class for better documentation.Changes walkthrough ๐
8 files
project.e2e.spec.ts
Enhance project e2e tests with new scenarios and fixes.
apps/api/src/project/project.e2e.spec.ts
secret.e2e.spec.ts
Enhance secret e2e tests with new scenarios and fixes.
apps/api/src/secret/secret.e2e.spec.ts
environment.e2e.spec.ts
Enhance environment e2e tests with new scenarios and fixes.
apps/api/src/environment/environment.e2e.spec.ts
workspace.e2e.spec.ts
Enhanced workspace tests with new scenarios and imports.
apps/api/src/workspace/workspace.e2e.spec.ts
variable.e2e.spec.ts
Enhanced variable tests with new scenarios and imports.
apps/api/src/variable/variable.e2e.spec.ts
workspace-role.e2e.spec.ts
Enhanced workspace role tests with new scenarios and imports.
apps/api/src/workspace-role/workspace-role.e2e.spec.ts
deletion.
workspace-membership.e2e.spec.ts
Add end-to-end tests for workspace membership features
apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts
updating members.
integration.e2e.spec.ts
Update integration tests with slug usage and conflict resolution
apps/api/src/integration/integration.e2e.spec.ts
1 files
admin.guard.ts
Add documentation comments to AdminGuard class.
apps/api/src/auth/guard/admin/admin.guard.ts - Added JSDoc comments to the `AdminGuard` class.
5 files
integration.service.ts
Enhanced integration service with slug handling and pagination.
apps/api/src/integration/service/integration.service.ts
workspace-membership.module.ts
Introduced workspace membership module with services.
apps/api/src/workspace-membership/workspace-membership.module.ts
workspace-membership.service.ts
Implement workspace membership management service
apps/api/src/workspace-membership/service/workspace-membership.service.ts
invitations.
workspace.service.ts
Enhance workspace service with slug-based operations
apps/api/src/workspace/service/workspace.service.ts
twilio.ts
Add Twilio secret detection rules with test cases
packages/secret-scan/src/rules/twilio.ts
101 files
secret.service.ts
...
apps/api/src/secret/service/secret.service.ts ...
variable.service.ts
...
apps/api/src/variable/service/variable.service.ts ...
event.e2e.spec.ts
...
apps/api/src/event/event.e2e.spec.ts ...
project.service.ts
...
apps/api/src/project/service/project.service.ts ...
authority-checker.service.ts
...
apps/api/src/common/authority-checker.service.ts ...
private_key.ts
...
packages/secret-scan/src/rules/private_key.ts ...
api-key.e2e.spec.ts
...
apps/api/src/api-key/api-key.e2e.spec.ts ...
workspace-role.service.ts
...
apps/api/src/workspace-role/service/workspace-role.service.ts ...
environment.service.ts
...
apps/api/src/environment/service/environment.service.ts ...
secret.test.ts
...
packages/secret-scan/src/test/secret.test.ts ...
api-key.service.ts
...
apps/api/src/api-key/service/api-key.service.ts ...
workspace.controller.ts
...
apps/api/src/workspace/controller/workspace.controller.ts ...
secret.spec.ts
...
packages/api-client/tests/secret.spec.ts ...
index.tsx
...
apps/web/src/components/pricing/card/index.tsx ...
variable.spec.ts
...
packages/api-client/tests/variable.spec.ts ...
project.spec.ts
...
packages/api-client/tests/project.spec.ts ...
environment.spec.ts
...
packages/api-client/tests/environment.spec.ts ...
run.command.ts
...
apps/cli/src/commands/run.command.ts ...
grafana.ts
...
packages/secret-scan/src/rules/grafana.ts ...
authress.ts
...
packages/secret-scan/src/rules/authress.ts ...
change-notifier.socket.ts
...
apps/api/src/socket/change-notifier.socket.ts ...
scan.command.ts
...
apps/cli/src/commands/scan.command.ts ...
project.controller.ts
...
apps/api/src/project/controller/project.controller.ts ...
jwt.ts
...
packages/secret-scan/src/rules/jwt.ts ...
workspace-membership.controller.ts
...
apps/api/src/workspace-membership/controller/workspace-membership.controller.ts ...
integration.spec.ts
...
packages/api-client/tests/integration.spec.ts ...
command.tsx
...
apps/platform/src/components/ui/command.tsx ...
base.command.ts
...
apps/cli/src/commands/base.command.ts ...
discord.ts
...
packages/secret-scan/src/rules/discord.ts ...
slug-generator.ts
...
apps/api/src/common/slug-generator.ts ...
auth.service.ts
...
apps/api/src/auth/service/auth.service.ts ...
project.ts
...
packages/api-client/src/controllers/project.ts ...
event.spec.ts
...
packages/api-client/tests/event.spec.ts ...
secret.controller.ts
...
apps/api/src/secret/controller/secret.controller.ts ...
variable.controller.ts
...
apps/api/src/variable/controller/variable.controller.ts ...
denylist.ts
...
packages/secret-scan/src/denylist.ts ...
project.types.d.ts
...
packages/api-client/src/types/project.types.d.ts ...
environment.ts
...
packages/api-client/src/controllers/environment.ts ...
index.ts
...
packages/secret-scan/src/rules/index.ts ...
create.profile.ts
...
apps/cli/src/commands/profile/create.profile.ts ...
workspace-role.controller.ts
...
apps/api/src/workspace-role/controller/workspace-role.controller.ts ...
auth.guard.ts
...
apps/api/src/auth/guard/auth/auth.guard.ts ...
util.ts
...
apps/api/src/common/util.ts ...
integration.controller.ts
...
apps/api/src/integration/controller/integration.controller.ts ...
user.controller.ts
...
apps/api/src/user/controller/user.controller.ts ...
page.tsx
...
apps/platform/src/app/(main)/project/[project]/@secret/page.tsx ...
cloudflare.ts
...
packages/secret-scan/src/rules/cloudflare.ts ...
pricing.ts
...
apps/web/src/constants/pricing.ts ...
client.ts
...
packages/api-client/src/client.ts ...
api-key.controller.ts
...
apps/api/src/api-key/controller/api-key.controller.ts ...
user.service.ts
...
apps/api/src/user/service/user.service.ts ...
secret.ts
...
packages/api-client/src/controllers/secret.ts ...
environment.controller.ts
...
apps/api/src/environment/controller/environment.controller.ts ...
variable.ts
...
packages/api-client/src/controllers/variable.ts ...
integration.types.d.ts
...
packages/api-client/src/types/integration.types.d.ts ...
page.tsx
...
apps/platform/src/app/(main)/page.tsx ...
list.profile.ts
...
apps/cli/src/commands/profile/list.profile.ts ...
pypi.ts
...
packages/secret-scan/src/rules/pypi.ts ...
animated-tabs.tsx
...
apps/web/src/components/ui/animated-tabs.tsx ...
bitbucket.ts
...
packages/secret-scan/src/rules/bitbucket.ts ...
paginate.ts
...
apps/api/src/common/paginate.ts ...
secret.types.d.ts
...
packages/api-client/src/types/secret.types.d.ts ...
openAI.ts
...
packages/secret-scan/src/rules/openAI.ts ...
configuration.ts
...
apps/cli/src/util/configuration.ts ...
page.tsx
...
apps/web/src/app/(main)/pricing/page.tsx ...
init.command.ts
...
apps/cli/src/commands/init.command.ts ...
integration.ts
...
packages/api-client/src/controllers/integration.ts ...
variable.types.d.ts
...
packages/api-client/src/types/variable.types.d.ts ...
environment.ts
...
apps/api/src/common/environment.ts ...
event.service.ts
...
apps/api/src/event/service/event.service.ts ...
update.profile.ts
...
apps/cli/src/commands/profile/update.profile.ts ...
enums.ts
...
packages/schema/src/enums.ts ...
workspace.spec.ts
...
packages/schema/tests/workspace.spec.ts ...
artifactory.ts
...
packages/secret-scan/src/rules/artifactory.ts ...
projects.ts
...
apps/platform/src/lib/api-functions/projects.ts ...
collective-authorities.ts
...
apps/api/src/common/collective-authorities.ts ...
npm.ts
...
packages/secret-scan/src/rules/npm.ts ...
create.environment.ts
...
apps/cli/src/commands/environment/create.environment.ts ...
app.module.ts
...
apps/api/src/app/app.module.ts ...
index.ts
...
apps/platform/src/types/index.ts ...
user.ts
...
apps/api/src/common/user.ts ...
layout.tsx
...
apps/platform/src/app/(main)/project/[project]/layout.tsx ...
cryptography.ts
...
apps/api/src/common/cryptography.ts ...
event.ts
...
apps/api/src/common/event.ts ...
facebook.ts
...
packages/secret-scan/src/rules/facebook.ts ...
ip_public.ts
...
packages/secret-scan/src/rules/ip_public.ts ...
confluent.ts
...
packages/secret-scan/src/rules/confluent.ts ...
atlassian.ts
...
packages/secret-scan/src/rules/atlassian.ts ...
accordion.tsx
...
apps/platform/src/components/ui/accordion.tsx ...
project.spec.ts
...
packages/schema/tests/project.spec.ts ...
shopify.ts
...
packages/secret-scan/src/rules/shopify.ts ...
harness.ts
...
packages/secret-scan/src/rules/harness.ts ...
workspace-role.spec.ts
...
packages/schema/tests/workspace-role.spec.ts ...
integration.spec.ts
...
packages/schema/tests/integration.spec.ts ...
variable.spec.ts
...
packages/schema/tests/variable.spec.ts ...
encrypt-text.tsx
...
apps/web/src/components/ui/encrypt-text.tsx ...
planetscale.ts
...
packages/secret-scan/src/rules/planetscale.ts ...
cryptography.spec.ts
...
apps/api/src/common/cryptography.spec.ts ...
update.environment.ts
...
apps/cli/src/commands/environment/update.environment.ts ...
api-key.guard.ts
...
apps/api/src/auth/guard/api-key/api-key.guard.ts ...
Additional 246 files not shown
...
Additional 246 files not shown ...
1 files
environment.types.d.ts
...
packages/api-client/src/types/environment.types.d.ts ...