Closed rajdip-b closed 2 weeks ago
Here are some key observations to aid the review process:
β±οΈ Estimated effort to review: 2 π΅π΅βͺβͺβͺ |
π§ͺ No relevant tests |
π No security concerns identified |
β‘ Recommended focus areas for review Unused Parameters The functions uploadFile, getFileUrl, and deleteFile have parameters marked as unused with eslint-disable comments. Consider removing unused parameters or documenting why they are needed. Code Duplication Similar toast notification div structure is duplicated. Consider extracting the common toast notification UI into a reusable component. |
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 87.92%. Comparing base (
ce50743
) to head (90c11c7
). Report is 214 commits behind head on develop.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Explore these optional code suggestions:
Category | Suggestion | Score |
Possible bug |
Add error handling for JSON parsing to prevent potential runtime errors___ **Add error type checking and handling for the JSON.parse() call to prevent runtimeerrors if localStorage contains invalid JSON data.** [apps/web/src/components/hero/index.tsx [31-33]](https://github.com/keyshade-xyz/keyshade/pull/525/files#diff-7613ce5c9e898c20a2427ba5bf13810f61bb0a532139896e00a54cd0285ad40cR31-R33) ```diff const emailsInWaitlist: string[] = dataInStorage - ? (JSON.parse(dataInStorage) as string[]) + ? (try { JSON.parse(dataInStorage) as string[] } catch { [] }) : [] ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: The suggestion addresses a potential runtime error by adding try-catch error handling for JSON.parse(), which could prevent application crashes if localStorage contains invalid data. | 7 |
Typo |
β Fix typo in collection name to improve clarity and maintain consistency___Suggestion Impact:The typo in the collection name was corrected from "Api Kei Controller" to "Api Key Controller" as suggested. code diff: ```diff - "name": "Api Kei Controller", + "name": "Api Key Controller", ```Controller" to maintain consistency with the folder name and improve clarity.** [api-collection/Api Key Controller/bruno.json [3]](https://github.com/keyshade-xyz/keyshade/pull/525/files#diff-f40a16a121f3db792eca7d2b0c21643a45abab368c21819589bf8d0afe78f90aR3-R3) ```diff { - "version": "1", - "name": "Api Kei Controller", + "version": "1", + "name": "Api Key Controller", "type": "collection", "ignore": [ "node_modules", ".git" ] } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Fixing the typo from "Api Kei Controller" to "Api Key Controller" improves clarity and maintains consistency with the folder name and overall API documentation structure. | 7 |
Enhancement |
Include example values for required path parameters to improve API documentation___ **Add a default or example value for the empty project_slug parameter to improve APIdocumentation and testing.** [api-collection/Environment Controller/Get all environments of project.bru [14]](https://github.com/keyshade-xyz/keyshade/pull/525/files#diff-6b3e00b7ba5c8df0b901ac49df19479058451c07fb74cb3205ab248af7e25173R14-R14) ```diff params:path { - project_slug: + project_slug: example-project-123 } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 6Why: Adding example values for path parameters improves API documentation clarity and makes testing easier by providing a reference format. | 6 |
Maintainability |
Extract duplicate CSS classes into a reusable class to improve maintainability and reduce code duplication___ **Move the duplicate toast notification div styling into a reusable CSS class to avoidcode duplication and maintain consistent styling.** [apps/web/src/components/hero/index.tsx [23]](https://github.com/keyshade-xyz/keyshade/pull/525/files#diff-7613ce5c9e898c20a2427ba5bf13810f61bb0a532139896e00a54cd0285ad40cR23-R23) ```diff -
+
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 5Why: The suggestion correctly identifies duplicate CSS classes used in multiple toast notifications and proposes a valid solution to improve code maintainability through CSS class extraction. | 5 |
Best practice |
Standardize documentation format to maintain consistency across API endpoints___ **Add proper markdown formatting for the description section to maintain consistencywith other API documentation.** [api-collection/Variable Controller/Delete variable.bru [21-23]](https://github.com/keyshade-xyz/keyshade/pull/525/files#diff-0257586b01df1235e0d70f67bcc40d36b8149eb2b081ba580b5d641c2f44b2eeR21-R23) ```diff docs { + ## Description + Delete a variable by its ID } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 5Why: Adding proper markdown formatting to the documentation section helps maintain consistency with other API endpoints and improves readability. | 5 |
π‘ Need additional feedback ? start a PR chat
User description
Driving force behind the change
Since we are an open-source project, we believe that we should be using the help of open-source tools wherever we can. Postman (tool that we were using till date) is of course a very good tool, but Bruno matches our requirements better.
Changes made
PR Type
documentation, enhancement
Description
zod
dependency.Changes walkthrough π
3 files
app.module.ts
Remove unused imports in app module
apps/api/src/app/app.module.ts
ThrottlerGuard
andThrottlerModule
.minio.provider.ts
Add ESLint directive for unused variables
apps/api/src/provider/minio.provider.ts
index.tsx
Reformat JSX for better readability
apps/web/src/components/hero/index.tsx
1 files
workspace.e2e.spec.ts
Remove unused test variables
apps/api/src/workspace/workspace.e2e.spec.ts
user3
.3 files
Can access live updates.bru
Add endpoint for live update access check
api-collection/Api Key Controller/Can access live updates.bru
Create API key.bru
Add endpoint for API key creation
api-collection/Api Key Controller/Create API key.bru
api-testing.md
Update API testing documentation to use Bruno
docs/contributing-to-keyshade/running-things-locally/api-testing.md
1 files
package.json
Remove Postman configuration from package.json
package.json
1 files
package-lock.json
Add zod dependency to package-lock
packages/schema/package-lock.json
zod
dependency to package-lock.