Feat/contactus page #404
Feat/contactus page #404
Conversation
|
@PriyanshuValiya is attempting to deploy a commit to the bunty's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces a contact form feature in the application. It includes a new controller for handling submissions, an Express router for routing requests, and a React component for the user interface. The backend processes form data with validation and email sending capabilities using Nodemailer, while the frontend captures user input and provides feedback. Additionally, error handling and rate limiting are implemented to enhance robustness and user experience. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (5)
backend/routes/contactUsRouter.js (1)
33-35: Enhance input validation rulesWhile the current validation is good, consider adding these improvements for better security and user experience:
- Add
isLowercase()for email- Add pattern validation for name to prevent special characters
- Consider checking for common spam patterns in the message
- body("email").isEmail().normalizeEmail(), + body("email").isEmail().isLowercase().normalizeEmail(), - body("name").trim().isLength({ min: 2 }).escape(), + body("name").trim().isLength({ min: 2 }).matches(/^[a-zA-Z\s]+$/).escape(),frontend/src/components/Shared/Navbar.jsx (2)
23-23: Consider using a more user-friendly menu label.The current "CONTACTUS" label could be more readable as "CONTACT US" with a space for better visual separation and readability.
- { name: 'CONTACTUS', path: '/contactus'} + { name: 'CONTACT US', path: '/contactus'}
Line range hint
144-166: Add ARIA attributes for better accessibility.The mobile menu toggle button should include proper ARIA attributes to improve accessibility for screen reader users.
<button onClick={toggleMenu} + aria-label="Toggle navigation menu" + aria-expanded={isMenuOpen} className={`${buttonTextClass} focus:outline-none`} >Also consider adding these accessibility improvements:
- Add
role="navigation"to the nav element- Add
aria-label="Main"to the nav element- Add
role="menuitem"to each navigation linkfrontend/src/components/Pages/ContactUs.jsx (2)
54-57: Consolidate and Reorder Validation ChecksThe validation for the email format is performed twice at lines 54-57 and 76-79. Additionally, checking if fields are empty before validating the email format can prevent unnecessary processing.
Consider reorganizing the validation code to eliminate redundancy:
- if (!emailRegex.test(mail)) { - setError('Please enter a valid email address.'); - return; - } // Check if all fields are filled if (!mail || !subject || !message) { setError('All fields are required.'); return; } // Validate email format once if (!emailRegex.test(mail)) { setError('Please enter a valid email address.'); return; }Also applies to: 71-74, 76-79
181-247: Improve Accessibility of Form ElementsEnsure that all form elements have associated labels and ARIA attributes to enhance accessibility for assistive technologies.
For example, the input field for "Subject" lacks an explicit
<label>:+ <label htmlFor="subject" className="sr-only"> + Subject + </label> <input type="text" - id="text" + id="subject" placeholder="Subject"Also, update the
idattribute to match thehtmlForattribute.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- backend/controller/contact.controller.js (1 hunks)
- backend/routes/contactUsRouter.js (1 hunks)
- backend/routes/index.js (1 hunks)
- frontend/src/components/Pages/ContactUs.jsx (1 hunks)
- frontend/src/components/Shared/Navbar.jsx (1 hunks)
- frontend/src/router/index.jsx (2 hunks)
🔇 Additional comments (7)
backend/routes/contactUsRouter.js (1)
20-27: LGTM! Well-configured rate limitingThe rate limit of 5 requests per 15 minutes is appropriate for a contact form, and the error response structure is consistent with the global error handler.
frontend/src/router/index.jsx (1)
25-25: LGTM! Route configuration follows established patterns.The new route and import for the ContactUs page are properly integrated into the existing routing structure.
Let's verify the ContactUs component exists:
Also applies to: 46-46
✅ Verification successful
Import path verified and component exists at the specified location
The ContactUs component exists at the expected path
frontend/src/components/Pages/ContactUs.jsx, confirming that the import statement in the router configuration is correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the ContactUs component exists at the specified path fd -t f "ContactUs.jsx" "frontend/src/components/Pages"Length of output: 97
backend/routes/index.js (1)
35-47: LGTM! Router imports are well-structured.The router imports follow a consistent pattern with clear error messages. The addition of
contactUsRouteraligns with the PR's contact form feature.frontend/src/components/Shared/Navbar.jsx (2)
Line range hint
15-24: LGTM! Clean integration with existing navigation.The new menu item is well-integrated into the existing navigation structure, maintaining consistency in both desktop and mobile views.
Line range hint
190-204: LGTM! Mobile responsiveness maintained.The mobile menu implementation correctly includes the new Contact Us item with consistent styling and behavior.
backend/controller/contact.controller.js (2)
46-46: Disabling 'rejectUnauthorized' may pose a security riskSetting
rejectUnauthorized: falsedisables SSL certificate validation, potentially exposing the application to man-in-the-middle attacks.[security]
It's recommended to enable strict SSL verification in production environments.
Apply this diff to remove the line:
46 - rejectUnauthorized: false, // Disable strict SSL verification
50-52: Sanitize input function may not fully prevent injection attacksThe current
sanitizeInputfunction only removes<and>characters, which may not be sufficient to prevent injection attacks or header injection in email fields.[security]
Consider using a more comprehensive sanitization approach or leveraging existing libraries for input sanitization, such as the
validatorpackage.Example:
const validator = require('validator'); // ... const sanitizeInput = (str) => { return validator.escape(str.trim()); };
| const express = require("express"); | ||
| const router = express.Router(); | ||
| const { createContactUs } = require("../controller/contact.controller"); | ||
| const rateLimit = require("express-rate-limit"); | ||
| const { body } = require("express-validator"); |
There was a problem hiding this comment.
Add missing import for validationResult
The validationResult function is used on line 38 but hasn't been imported from express-validator.
Add the following import:
-const { body } = require("express-validator");
+const { body, validationResult } = require("express-validator");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const express = require("express"); | |
| const router = express.Router(); | |
| const { createContactUs } = require("../controller/contact.controller"); | |
| const rateLimit = require("express-rate-limit"); | |
| const { body } = require("express-validator"); | |
| const express = require("express"); | |
| const router = express.Router(); | |
| const { createContactUs } = require("../controller/contact.controller"); | |
| const rateLimit = require("express-rate-limit"); | |
| const { body, validationResult } = require("express-validator"); |
| async (req, res, next) => { | ||
| const errors = validationResult(req); | ||
| if (!errors.isEmpty()) { | ||
| return res.status(400).json({ status: "error", errors: errors.array() }); | ||
| } | ||
| await createContactUs(req, res, next); | ||
| } |
There was a problem hiding this comment.
Add proper error handling for async route
The async route handler lacks try-catch block for error handling, which could lead to unhandled promise rejections.
Implement proper error handling:
async (req, res, next) => {
- const errors = validationResult(req);
- if (!errors.isEmpty()) {
- return res.status(400).json({ status: "error", errors: errors.array() });
- }
- await createContactUs(req, res, next);
+ try {
+ const errors = validationResult(req);
+ if (!errors.isEmpty()) {
+ return res.status(400).json({ status: "error", errors: errors.array() });
+ }
+ await createContactUs(req, res, next);
+ } catch (error) {
+ next(error);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async (req, res, next) => { | |
| const errors = validationResult(req); | |
| if (!errors.isEmpty()) { | |
| return res.status(400).json({ status: "error", errors: errors.array() }); | |
| } | |
| await createContactUs(req, res, next); | |
| } | |
| async (req, res, next) => { | |
| try { | |
| const errors = validationResult(req); | |
| if (!errors.isEmpty()) { | |
| return res.status(400).json({ status: "error", errors: errors.array() }); | |
| } | |
| await createContactUs(req, res, next); | |
| } catch (error) { | |
| next(error); | |
| } | |
| } |
| router.use((err, req, res, next) => { | ||
| console.error(err.stack); | ||
| const statusCode = err.statusCode || 500; | ||
| const errorType = err.type || "InternalServerError"; | ||
| res.status(500).json({ | ||
| status: "error", | ||
| message: err.message || "An error occurred processing your request", | ||
| type: errorType, | ||
| ...(process.env.NODE_ENV === "development" && { stack: err.stack }), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Fix inconsistent HTTP status code usage
The middleware captures a dynamic statusCode but always responds with 500. This could lead to incorrect error reporting for client-side errors (4xx).
Apply this fix:
- res.status(500).json({
+ res.status(statusCode).json({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.use((err, req, res, next) => { | |
| console.error(err.stack); | |
| const statusCode = err.statusCode || 500; | |
| const errorType = err.type || "InternalServerError"; | |
| res.status(500).json({ | |
| status: "error", | |
| message: err.message || "An error occurred processing your request", | |
| type: errorType, | |
| ...(process.env.NODE_ENV === "development" && { stack: err.stack }), | |
| }); | |
| }); | |
| router.use((err, req, res, next) => { | |
| console.error(err.stack); | |
| const statusCode = err.statusCode || 500; | |
| const errorType = err.type || "InternalServerError"; | |
| res.status(statusCode).json({ | |
| status: "error", | |
| message: err.message || "An error occurred processing your request", | |
| type: errorType, | |
| ...(process.env.NODE_ENV === "development" && { stack: err.stack }), | |
| }); | |
| }); |
| const express = require("express"); | ||
| const router = express.Router(); | ||
| const { createContactUs } = require("../controller/contact.controller"); | ||
| const rateLimit = require("express-rate-limit"); | ||
| const { body } = require("express-validator"); | ||
|
|
||
| // Error handling middleware | ||
| router.use((err, req, res, next) => { | ||
| console.error(err.stack); | ||
| const statusCode = err.statusCode || 500; | ||
| const errorType = err.type || "InternalServerError"; | ||
| res.status(500).json({ | ||
| status: "error", | ||
| message: err.message || "An error occurred processing your request", | ||
| type: errorType, | ||
| ...(process.env.NODE_ENV === "development" && { stack: err.stack }), | ||
| }); | ||
| }); | ||
|
|
||
| const contactFormLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 5, // limit each IP to 5 requests per window | ||
| message: { | ||
| status: "error", | ||
| message: "Too many requests, please try again later", | ||
| }, | ||
| }); | ||
|
|
||
| router.post( | ||
| "/", | ||
| contactFormLimiter, | ||
| [ | ||
| body("email").isEmail().normalizeEmail(), | ||
| body("name").trim().isLength({ min: 2 }).escape(), | ||
| body("message").trim().isLength({ min: 10, max: 1000 }).escape(), | ||
| ], | ||
| async (req, res, next) => { | ||
| const errors = validationResult(req); | ||
| if (!errors.isEmpty()) { | ||
| return res.status(400).json({ status: "error", errors: errors.array() }); | ||
| } | ||
| await createContactUs(req, res, next); | ||
| } | ||
| ); | ||
|
|
||
| module.exports = router; |
There was a problem hiding this comment.
💡 Codebase verification
Add request size limits and CSRF protection
The codebase has proper CORS configuration with specific allowed origins, but still needs additional security measures:
- Add request size limits to
express.json()middleware to prevent large payload attacks - Implement CSRF protection using a package like
csurffor browser-based submissions - Current rate limiting is good but only applied to contact form endpoint
Example implementation:
app.use(express.json({ limit: '10kb' }));🔗 Analysis chain
Consider adding additional security measures
While the current implementation includes rate limiting and input sanitization, consider these security enhancements:
- Add CORS configuration to restrict access to trusted domains
- Implement request size limits to prevent payload attacks
- Consider adding CSRF protection if the form is used in a browser context
Let's check if CORS is configured in the main app:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CORS configuration
rg -l 'cors' --type js
Length of output: 39
Script:
#!/bin/bash
# Check the CORS configuration in index.js
rg 'cors' -A 5 -B 5 backend/index.js
Length of output: 1178
Script:
#!/bin/bash
# Check for request size limits configuration
rg -i 'limit|bodyparser|express.json' -A 5 -B 5 backend/index.js
# Check for CSRF protection
rg -i 'csrf|csurf' --type js
Length of output: 279
|
|
||
| router.get("/", (req, res) => { | ||
| return res.json({ | ||
| message: "Welcome to the restaurant API!", | ||
| version: "1.0.0", | ||
| endpoints: { | ||
| Reservation: "/reservation", | ||
| Feedback: "/feedback", // Added feedback endpoint documentation | ||
| Feedback: "/feedback", | ||
| }, | ||
| documentation: "https://api-docs-url.com", | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Update API documentation to include new endpoints.
The API documentation is missing the newly added contact endpoint. Please update the endpoints object to include it.
endpoints: {
Reservation: "/reservation",
Feedback: "/feedback",
+ Contact: "/contact",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| router.get("/", (req, res) => { | |
| return res.json({ | |
| message: "Welcome to the restaurant API!", | |
| version: "1.0.0", | |
| endpoints: { | |
| Reservation: "/reservation", | |
| Feedback: "/feedback", // Added feedback endpoint documentation | |
| Feedback: "/feedback", | |
| }, | |
| documentation: "https://api-docs-url.com", | |
| }); | |
| }); | |
| router.get("/", (req, res) => { | |
| return res.json({ | |
| message: "Welcome to the restaurant API!", | |
| version: "1.0.0", | |
| endpoints: { | |
| Reservation: "/reservation", | |
| Feedback: "/feedback", | |
| Contact: "/contact", | |
| }, | |
| documentation: "https://api-docs-url.com", | |
| }); | |
| }); |
| const validation = contactSchema.safeParse(req.body); | ||
|
|
||
| if (!validation.success) { | ||
| console.error("Error at validation"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use logger instead of console.error for consistent logging
Replace console.error with logger.error to maintain consistent logging practices throughout the application.
Apply this diff:
26 - console.error("Error at validation");
26 + logger.error("Validation failed", { requestId: req.id });Committable suggestion was skipped due to low confidence.
| const sanitizeInput = (input) => { | ||
| return input.replace(/[<>]/g, ''); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Input Sanitization
The sanitizeInput function currently removes only < and > characters. This may not be sufficient to prevent all types of harmful input or XSS attacks.
Consider using a more robust sanitization method or a library designed for input sanitization to ensure security. Additionally, server-side validation and sanitization should be implemented as the primary defense.
| }; | ||
|
|
||
| // Add to state | ||
| const [lastSubmissionTime, setLastSubmissionTime] = useState(0); |
There was a problem hiding this comment.
Move useState Hook Declaration Outside of handleSubmit
The useState hook for lastSubmissionTime is declared inside the handleSubmit function. React hooks should not be called inside functions, loops, or conditionals. This can lead to unexpected behavior and violates the Rules of Hooks.
Apply this diff to fix the issue:
+ const [lastSubmissionTime, setLastSubmissionTime] = useState(0);
const handleSubmit = async (e) => {
e.preventDefault();
- // Add to state
- const [lastSubmissionTime, setLastSubmissionTime] = useState(0);
// Basic client-side validation for security
const emailRegex =Committable suggestion was skipped due to low confidence.
| if (now - lastSubmissionTime < 60000) { | ||
| // 1 minute | ||
| setError('Please wait before submitting again.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Update lastSubmissionTime After Successful Submission
The lastSubmissionTime state is used for rate limiting but is never updated after a successful form submission. This means the rate limiting check will not function as intended.
Add the following line after a successful submission to update lastSubmissionTime:
if (!response.ok) {
throw new Error(`Network response was not ok: ${response.status}`);
}
+ setLastSubmissionTime(Date.now());
setSubmitted(true);Committable suggestion was skipped due to low confidence.
| const fetchWithTimeout = async (url, options, timeout = 5000) => { | ||
| const controller = new AbortController(); | ||
| const id = setTimeout(() => controller.abort(), timeout); | ||
| try { | ||
| const response = await fetch(url, { | ||
| ...options, | ||
| signal: controller.signal, | ||
| }); | ||
| clearTimeout(id); | ||
| return response; | ||
| } catch (error) { | ||
| clearTimeout(id); | ||
| throw error; | ||
| } | ||
| }; | ||
| const retryWithBackoff = async (fn, retries = 3, backoff = 300) => { | ||
| try { | ||
| return await fn(); | ||
| } catch (error) { | ||
| if (retries === 0) throw error; | ||
| await new Promise((resolve) => setTimeout(resolve, backoff)); | ||
| return retryWithBackoff(fn, retries - 1, backoff * 2); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move Utility Functions Outside of handleSubmit
The fetchWithTimeout and retryWithBackoff functions are defined inside the handleSubmit function. Defining them inside means they are recreated on every form submission, which can impact performance.
Refactor by moving these functions outside of handleSubmit:
+ // Utility function to fetch with timeout
+ const fetchWithTimeout = async (url, options, timeout = 5000) => {
+ // Function body...
+ };
+ // Utility function to retry with backoff
+ const retryWithBackoff = async (fn, retries = 3, backoff = 300) => {
+ // Function body...
+ };
const handleSubmit = async (e) => {
e.preventDefault();
- const fetchWithTimeout = async (url, options, timeout = 5000) => {
- // Function body...
- };
- const retryWithBackoff = async (fn, retries = 3, backoff = 300) => {
- // Function body...
- };
// Rest of the handleSubmit code...Committable suggestion was skipped due to low confidence.
|
|
|
@PriyanshuValiya resolve the conflicts |
|
@RamakrushnaBiswal If I follow all coderabit suggestions, final email functionality is not working properly.. |
|
@PriyanshuValiya As per your issue . you didn't add a contact us page you just add the feedback section again in different route |
Some Minor Changes as per coderabit suggestions.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation