Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR reorganizes module exports by introducing barrel exports for middleware and models, enabling centralized import paths. It concurrently adds a complete FAQ CRUD API with GET, POST, PATCH, and DELETE route handlers, complete with data models, request/response DTOs, and admin-protected write operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/middleware/convenorAuth.ts (2)
5-5: Add runtime validation for JWT_SECRET.The non-null assertion assumes
JWT_SECRETis set, but if it's missing,jwt.verify()will fail with an unclear error. Add a runtime check to fail fast with a clear message.🔒 Proposed fix to validate JWT_SECRET at initialization
-const JWT_SECRET = process.env.JWT_SECRET!; +const JWT_SECRET = process.env.JWT_SECRET; + +if (!JWT_SECRET) { + throw new Error('JWT_SECRET environment variable is not configured'); +}
67-67: Add null safety check for currentConvenor.Accessing
society.currentConvenor.userIdwithout verifyingcurrentConvenorexists will throw a TypeError if the field is null or undefined. Add a defensive check to handle this gracefully.🛡️ Proposed fix to add null safety
// 🔐 Check if current convenor - if (!society.currentConvenor.userId.equals(decoded.userId)) { + if (!society.currentConvenor || !society.currentConvenor.userId.equals(decoded.userId)) { return { error: 'Forbidden: Not current convenor', status: 403 }; }
🤖 Fix all issues with AI agents
In @app/api/faq/[id]/route.ts:
- Around line 93-104: Wrap the call to request.json() in a try/catch before
using its result (within the handler where connectDB() and the UpdateFAQRequest
destructuring occur) to catch malformed JSON; if parsing fails return a
NextResponse.json with a 400 status and a clear message like "Invalid JSON in
request body" instead of letting the exception propagate, then proceed with the
existing validation of faq when parsing succeeds.
- Around line 15-17: The GET, DELETE and PATCH route handlers access params
synchronously but Next.js exposes params as a Promise in v15+; update each
handler (GET, DELETE, PATCH) to await params (e.g., const { id } = await params)
before using id, then use that id for the rest of the logic (replace direct uses
of params.id). Ensure you apply the same change in the functions named GET,
DELETE and PATCH so they all await params before accessing id.
In @lib/models/FAQ.ts:
- Line 2: Remove the unused import "extend" from zod/mini at the top of the
file; locate the import statement "import { extend } from 'zod/mini';" in FAQ.ts
and delete it so there are no unused imports remaining.
- Around line 11-16: The custom string "id" field in the FAQ schema (the id: {
type: String, unique: true, auto: true, default: ... } block) should be removed
because auto: true only applies to ObjectId and it shadows Mongoose's virtual
id; either rely on the built-in _id (and use _id.toString() where code reads id)
or rename the field to a non-conflicting name like faqId and update the
TypeScript interface and all usages accordingly; also remove the auto/default
properties, update any serialization logic to use toObject/toJSON virtuals or
map _id to id where needed, and adjust references in the codebase that accessed
FAQ.id to use the new property or _id.toString().
In @lib/models/index.ts:
- Line 18: The FAQ model is re-exported but not imported for side effects, so
its Mongoose schema may not be registered; add a side-effect import for the
model (e.g., import './FAQ';) at the top of the file alongside the other
side-effect model imports so the FAQ schema is registered before the line
exporting FAQ (export { default as FAQ } from './FAQ';).
🧹 Nitpick comments (5)
app/api/faq/route.ts (1)
14-34: Consider adding pagination for scalability.The GET endpoint fetches all FAQs without pagination, which could lead to performance issues as the FAQ collection grows. Consider implementing cursor-based or offset-based pagination.
📄 Example pagination implementation
-export async function GET(request: Request) { +export async function GET(request: Request) { + const { searchParams } = new URL(request.url); + const page = parseInt(searchParams.get('page') || '1', 10); + const limit = parseInt(searchParams.get('limit') || '10', 10); + const skip = (page - 1) * limit; + try { await connectDB(); - const faqs = await FAQ.find({}).sort({ createdAt: -1 }); + const [faqs, total] = await Promise.all([ + FAQ.find({}).sort({ createdAt: -1 }).skip(skip).limit(limit), + FAQ.countDocuments({}) + ]); return NextResponse.json<GetAllFAQsResponse>( { success: true, - data: faqs + data: faqs, + pagination: { + page, + limit, + total, + totalPages: Math.ceil(total / limit) + } }, { status: 200 } );lib/middleware/index.ts (1)
1-6: Side-effect imports are redundant.Lines 1-2 import modules purely for side effects, but since lines 5-6 already import these modules for re-export, the side-effect imports serve no purpose and can be removed.
Proposed fix
-import './adminAuth' -import './convenorAuth' - -// (Optional) re-exports for convenience export { default as adminAuth } from './adminAuth'; export { default as convenorAuth } from './convenorAuth';types/dto/faq.ts (2)
2-8: Consider consolidating duplicate request interfaces.
CreateFAQRequestandUpdateFAQRequestare identical. You could define one and alias the other, or use a singleFAQRequestBodytype.Proposed consolidation
-// Request DTOs -export interface CreateFAQRequest { - faq: string; -} - -export interface UpdateFAQRequest { +export interface FAQRequest { faq: string; } + +export type CreateFAQRequest = FAQRequest; +export type UpdateFAQRequest = FAQRequest;
46-49: Makesuccessrequired inErrorResponsefor consistency.Other response types have
success: trueas a required literal. Havingsuccess?: falseas optional breaks the discriminated union pattern and makes type narrowing less reliable.Proposed fix
export interface ErrorResponse { message: string; - success?: false; + success: false; }app/api/convenors/co-convenors/all/route.ts (1)
4-5: Consider using barrel import forSocietyas well.The
convenorAuthimport was updated to use the barrel, butSocietyis still imported directly from its specific path. For consistency with the PR's import standardization:Proposed fix
import connectDB from '@/lib/db'; -import Society from '@/lib/models/Society'; +import '@/lib/models'; +import { Society } from '@/lib/models'; import { convenorAuth } from '@/lib/middleware';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/api/auth/login/route.tsapp/api/convenors/co-convenors/all/route.tsapp/api/convenors/co-convenors/route.tsapp/api/convenors/route.tsapp/api/faq/[id]/route.tsapp/api/faq/route.tsapp/api/user/add/route.tslib/middleware/adminAuth.tslib/middleware/convenorAuth.tslib/middleware/index.tslib/models/FAQ.tslib/models/index.tstypes/dto/faq.ts
🧰 Additional context used
🧬 Code graph analysis (3)
lib/models/FAQ.ts (1)
lib/models/index.ts (1)
IFAQ(26-26)
app/api/faq/route.ts (3)
app/api/faq/[id]/route.ts (1)
GET(15-45)types/dto/faq.ts (4)
GetAllFAQsResponse(19-22)CreateFAQRequest(2-4)CreateFAQResponse(29-33)ErrorResponse(46-49)lib/middleware/adminAuth.ts (1)
adminAuth(6-39)
app/api/faq/[id]/route.ts (2)
app/api/faq/route.ts (1)
GET(14-34)types/dto/faq.ts (5)
GetSingleFAQResponse(24-27)ErrorResponse(46-49)DeleteFAQResponse(41-44)UpdateFAQRequest(6-8)UpdateFAQResponse(35-39)
🔇 Additional comments (8)
lib/middleware/adminAuth.ts (1)
6-6: LGTM! Export refactor aligns with barrel pattern.The change from named to default export is consistent with the PR's goal of centralizing middleware exports via
lib/middleware/index.ts. The authentication logic remains intact and correctly validates admin access.app/api/user/add/route.ts (1)
7-7: LGTM! Barrel import consolidation.The import path now leverages the centralized middleware barrel export, improving consistency across the codebase.
app/api/auth/login/route.ts (1)
6-6: LGTM! Model import standardization.The shift to named imports from the models barrel export improves consistency with the PR's centralized import pattern.
app/api/faq/route.ts (1)
37-75: LGTM! Solid validation and error handling.The POST handler correctly:
- Enforces admin authentication via
adminAuthmiddleware- Validates that the FAQ content is a non-empty string
- Trims whitespace before saving
- Returns appropriate status codes and typed responses
lib/models/index.ts (1)
26-26: IFAQ type export is correctly configured.The IFAQ type is properly exported from
FAQ.ts(line 4) and re-exported inindex.ts(line 26). The interface structure—id(string) andfaq(string)—matches the schema definition exactly. FAQ routes successfully import and use the FAQ model for all CRUD operations with appropriate TypeScript typing.app/api/convenors/co-convenors/route.ts (1)
5-5: LGTM!The import refactor to use the middleware barrel export is consistent with the PR's standardization effort.
app/api/convenors/route.ts (1)
6-7: LGTM!The import refactor to use named exports from the models barrel is consistent with the PR's consolidation pattern.
lib/middleware/convenorAuth.ts (1)
14-14: Export change to default looks good.The migration to a default export aligns with the barrel export pattern introduced in this PR. All import sites have been correctly updated to use the barrel module import pattern from
@/lib/middleware, and no stale direct imports fromconvenorAuth.tsremain.
| export async function GET( | ||
| request: Request, | ||
| { params }: { params: { id: string } } |
There was a problem hiding this comment.
In Next.js 15+, params is a Promise and must be awaited.
The params object is now asynchronous. Accessing params.id directly without await will fail at runtime.
Proposed fix for GET handler
export async function GET(
request: Request,
- { params }: { params: { id: string } }
+ { params }: { params: Promise<{ id: string }> }
) {
try {
await connectDB();
+ const { id } = await params;
- const faq = await FAQ.findById(params.id);
+ const faq = await FAQ.findById(id);Apply the same pattern to DELETE (lines 48-50) and PATCH (lines 85-87) handlers.
🤖 Prompt for AI Agents
In @app/api/faq/[id]/route.ts around lines 15 - 17, The GET, DELETE and PATCH
route handlers access params synchronously but Next.js exposes params as a
Promise in v15+; update each handler (GET, DELETE, PATCH) to await params (e.g.,
const { id } = await params) before using id, then use that id for the rest of
the logic (replace direct uses of params.id). Ensure you apply the same change
in the functions named GET, DELETE and PATCH so they all await params before
accessing id.
| try { | ||
| await connectDB(); | ||
|
|
||
| const { faq }: UpdateFAQRequest = await request.json(); | ||
|
|
||
| // 🔹 Validation | ||
| if (!faq || typeof faq !== 'string' || faq.trim() === '') { | ||
| return NextResponse.json( | ||
| { message: 'FAQ content is required and must be a non-empty string' }, | ||
| { status: 400 } | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add error handling for malformed JSON in request body.
If request.json() fails (e.g., invalid JSON), it throws an unhandled exception that will result in a 500 error rather than a 400 Bad Request.
Proposed fix
try {
await connectDB();
+
+ let body: UpdateFAQRequest;
+ try {
+ body = await request.json();
+ } catch {
+ return NextResponse.json(
+ { message: 'Invalid JSON body' },
+ { status: 400 }
+ );
+ }
+ const { faq } = body;
- const { faq }: UpdateFAQRequest = await request.json();
-
// 🔹 Validation📝 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.
| try { | |
| await connectDB(); | |
| const { faq }: UpdateFAQRequest = await request.json(); | |
| // 🔹 Validation | |
| if (!faq || typeof faq !== 'string' || faq.trim() === '') { | |
| return NextResponse.json( | |
| { message: 'FAQ content is required and must be a non-empty string' }, | |
| { status: 400 } | |
| ); | |
| } | |
| try { | |
| await connectDB(); | |
| let body: UpdateFAQRequest; | |
| try { | |
| body = await request.json(); | |
| } catch { | |
| return NextResponse.json( | |
| { message: 'Invalid JSON body' }, | |
| { status: 400 } | |
| ); | |
| } | |
| const { faq } = body; | |
| // 🔹 Validation | |
| if (!faq || typeof faq !== 'string' || faq.trim() === '') { | |
| return NextResponse.json( | |
| { message: 'FAQ content is required and must be a non-empty string' }, | |
| { status: 400 } | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @app/api/faq/[id]/route.ts around lines 93 - 104, Wrap the call to
request.json() in a try/catch before using its result (within the handler where
connectDB() and the UpdateFAQRequest destructuring occur) to catch malformed
JSON; if parsing fails return a NextResponse.json with a 400 status and a clear
message like "Invalid JSON in request body" instead of letting the exception
propagate, then proceed with the existing validation of faq when parsing
succeeds.
| @@ -0,0 +1,26 @@ | |||
| import mongoose, { Schema, Document } from 'mongoose'; | |||
| import { extend } from 'zod/mini'; | |||
There was a problem hiding this comment.
Remove unused import.
The extend import from zod/mini is never used in this file.
Proposed fix
-import mongoose, { Schema, Document } from 'mongoose';
-import { extend } from 'zod/mini';
+import mongoose, { Schema, Document } from 'mongoose';🤖 Prompt for AI Agents
In @lib/models/FAQ.ts at line 2, Remove the unused import "extend" from zod/mini
at the top of the file; locate the import statement "import { extend } from
'zod/mini';" in FAQ.ts and delete it so there are no unused imports remaining.
| id: { | ||
| type: String, | ||
| unique: true, | ||
| auto: true, | ||
| default: () => new Date().getTime().toString(), | ||
| }, |
There was a problem hiding this comment.
The auto: true option has no effect on String fields, and custom id shadows Mongoose's virtual.
auto: trueis only meaningful forObjectIdtypes—it won't auto-generate values forString.- Mongoose documents already have a virtual
idgetter (returns_id.toString()). Defining a customidfield shadows this, which may cause confusion or unexpected behavior in serialization.
Consider removing the custom id field and relying on the built-in _id, or rename it (e.g., faqId) to avoid the conflict.
Proposed fix: remove custom id field
const faqSchema: Schema<IFAQ> = new Schema<IFAQ>(
{
- id: {
- type: String,
- unique: true,
- auto: true,
- default: () => new Date().getTime().toString(),
- },
faq: {
type: String,
required: true,
},
},
{ timestamps: true }
);And update the interface accordingly:
export interface IFAQ extends Document {
- id: string;
faq: string;
}📝 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.
| id: { | |
| type: String, | |
| unique: true, | |
| auto: true, | |
| default: () => new Date().getTime().toString(), | |
| }, |
🤖 Prompt for AI Agents
In @lib/models/FAQ.ts around lines 11 - 16, The custom string "id" field in the
FAQ schema (the id: { type: String, unique: true, auto: true, default: ... }
block) should be removed because auto: true only applies to ObjectId and it
shadows Mongoose's virtual id; either rely on the built-in _id (and use
_id.toString() where code reads id) or rename the field to a non-conflicting
name like faqId and update the TypeScript interface and all usages accordingly;
also remove the auto/default properties, update any serialization logic to use
toObject/toJSON virtuals or map _id to id where needed, and adjust references in
the codebase that accessed FAQ.id to use the new property or _id.toString().
| export { default as Developer } from './Developer'; | ||
| export { default as Tag } from './Tag'; | ||
| export { default as User } from './User'; | ||
| export { default as FAQ } from './FAQ'; |
There was a problem hiding this comment.
Critical: Missing side-effect import for FAQ model.
The FAQ model is re-exported here, but there's no corresponding side-effect import at the top of the file (lines 1-8). Mongoose models must be imported for side effects to register their schemas before use. Without this, the FAQ model may not be available when other modules attempt to use it.
🐛 Add the required side-effect import
Add the FAQ import at the top of the file alongside other model imports:
// 🔥 Register all models (SIDE EFFECT IMPORTS)
import './User';
import './Society';
import './Lecture';
import './Event';
import './Sponsor';
import './Developer';
import './Tag';
+import './FAQ';📝 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.
| export { default as FAQ } from './FAQ'; | |
| // 🔥 Register all models (SIDE EFFECT IMPORTS) | |
| import './User'; | |
| import './Society'; | |
| import './Lecture'; | |
| import './Event'; | |
| import './Sponsor'; | |
| import './Developer'; | |
| import './Tag'; | |
| import './FAQ'; |
🤖 Prompt for AI Agents
In @lib/models/index.ts at line 18, The FAQ model is re-exported but not
imported for side effects, so its Mongoose schema may not be registered; add a
side-effect import for the model (e.g., import './FAQ';) at the top of the file
alongside the other side-effect model imports so the FAQ schema is registered
before the line exporting FAQ (export { default as FAQ } from './FAQ';).
1 participant
This pull request introduces a complete FAQ API, including model, DTOs, and CRUD endpoints with admin protection and robust error handling. It also refactors middleware and model imports for better maintainability and consistency across the codebase.
FAQ Feature Implementation
FAQMongoose model inlib/models/FAQ.ts, defining schema and export for use in API routes.app/api/faq/route.tsandapp/api/faq/[id]/route.ts, including admin authentication, validation, and error handling. (app/api/faq/route.tsR1-R74, app/api/faq/[id]/route.tsR1-R133)types/dto/faq.tsfor request and response consistency.Middleware Refactoring
adminAuthandconvenorAuthexports to default exports, and updated their usage throughout API routes for consistency. [1] [2]lib/middleware/index.ts, simplifying imports in API routes.Model Import Improvements
lib/models/index.ts, including the newFAQexport. [1] [2] [3]Middleware Import Clean-up
lib/middlewareindex, removing direct file imports foradminAuthandconvenorAuth. [1] [2] [3]These changes collectively add a robust FAQ system and improve code maintainability and consistency across authentication and model usage.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.