-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: adding prettierrc file #916
Conversation
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a new Prettier configuration file Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Server/routes/authRoute.js (1)
Line range hint
1-34
: Aight, let's wrap this up like a tight rhyme, yo!Overall, your code's lookin' cleaner than a fresh pair of Timbs. The formatting game is on point, makin' it easier to read than mom's recipe for spaghetti. But we gotta address that elephant in the room:
Rate limiting is missin' like the last M&M in the bag. It's crucial for keepin' our routes safe from attacks that'll have us sweatin' more than Eminem before a rap battle.
Here's what we gotta do to make this code bulletproof:
- Add rate limiting to all sensitive routes (registration, login, user management).
- Consider implementing a global rate limiting middleware for extra protection.
You down to make these changes? Let's turn this code into a platinum record, ya feel me?
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 24-24: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 24-24: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 26-26: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 26-26: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 27-27: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 27-27: Missing rate limiting
This route handler performs a database access, but is not rate-limited.Client/package.json (1)
28-28
: Yo, this Prettier addition is fire, but let's tweak it a bit, aight?Yo, I'm sweatin' over here, but this change is lookin' good! Adding Prettier is gonna make our code look fresh, no doubt. But check it, we might wanna move this bad boy to the "devDependencies" section. It's like, you know, for development, not for when we're servin' up that hot spaghetti code to users.
Here's how we can flip this script:
"dependencies": { "@emotion/react": "^11.11.4", "@emotion/styled": "^11.11.5", "@fontsource/roboto": "^5.0.13", "@mui/icons-material": "^5.15.17", "@mui/lab": "^5.0.0-alpha.170", "@mui/material": "^5.15.16", "@mui/x-charts": "^7.5.1", "@mui/x-data-grid": "7.3.2", "@mui/x-date-pickers": "7.3.2", "@reduxjs/toolkit": "2.2.5", "axios": "^1.7.4", "chart.js": "^4.4.3", "dayjs": "1.11.11", "joi": "17.13.1", "jwt-decode": "^4.0.0", - "prettier": "^3.3.3", "react": "^18.2.0", "react-dom": "^18.2.0", "react-redux": "9.1.2", "react-router": "^6.23.0", "react-router-dom": "^6.23.1", "react-toastify": "^10.0.5", "recharts": "2.13.0-alpha.4", "redux-persist": "6.0.0", "vite-plugin-svgr": "^4.2.0" }, "devDependencies": { "@types/react": "^18.2.66", "@types/react-dom": "^18.2.22", "@vitejs/plugin-react": "^4.2.1", "eslint": "^8.57.0", "eslint-plugin-react": "^7.34.1", "eslint-plugin-react-hooks": "^4.6.0", "eslint-plugin-react-refresh": "^0.4.6", + "prettier": "^3.3.3", "vite": "^5.2.0" }This way, we're keepin' it real with where Prettier belongs, you feel me?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Client/package-lock.json
is excluded by!**/package-lock.json
Server/package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (6)
- .prettierrc (1 hunks)
- Client/package.json (1 hunks)
- Client/src/App.css (1 hunks)
- Client/src/App.jsx (3 hunks)
- Server/package.json (1 hunks)
- Server/routes/authRoute.js (1 hunks)
✅ Files skipped from review due to trivial changes (4)
- .prettierrc
- Client/src/App.css
- Client/src/App.jsx
- Server/package.json
🧰 Additional context used
🪛 GitHub Check: CodeQL
Server/routes/authRoute.js
[failure] 24-24: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 24-24: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 26-26: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 26-26: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 27-27: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 27-27: Missing rate limiting
This route handler performs a database access, but is not rate-limited.
🔇 Additional comments (1)
Server/routes/authRoute.js (1)
10-18
: Yo, these imports are lookin' fresh, dawg!The way you've lined up these imports is tighter than mom's spaghetti. It's makin' the code easier to digest, you feel me? Keep that formatting game strong!
verifyJWT, | ||
editUser | ||
); | ||
router.put("/user/:userId", upload.single("profileImage"), verifyJWT, editUser); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo, these routes are lookin' clean, but we gotta talk security, homie!
The way you've tightened up these route definitions is smoother than a fresh beat. But listen up, we got a situation here that's makin' my palms sweaty:
- These routes are wide open like mom's spaghetti on a plate. We need to add some rate limiting to keep the bad guys from goin' ham on our server.
- Especially watch out for that delete route, it's like givin' away the secret sauce without any protection.
You want me to spit some code to add rate limiting? I got you covered like vomit on a sweater. Just say the word, and I'll drop that fire code faster than you can say "Eminem"!
Also applies to: 26-27
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 24-24: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
[failure] 24-24: Missing rate limiting
This route handler performs authorization, but is not rate-limited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting the time prettier config together, looks pretty good to me! We can adjust settings as we go if people gave particularly preference as to how to set up the config.
I don't think we need prettier in the build though so it should probably be a dev dependency. Let me know if I've missed something though!
Client/package.json
Outdated
@@ -25,6 +25,7 @@ | |||
"dayjs": "1.11.11", | |||
"joi": "17.13.1", | |||
"jwt-decode": "^4.0.0", | |||
"prettier": "^3.3.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier should probably be a dev dependency instead of a build dependency, we don't need it to be included in the build I don't think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, Alex! I corrected it. Thanks for that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, ready to merge in!
Resolves Issue #912
Now all formatting will be done following project settings, instead of local settings.