Implement Hierarchical Task Assignment & Progress Tracking System #237
Open
codewkaushik404 wants to merge 33 commits intoOpenLake:mainfrom
Open
Implement Hierarchical Task Assignment & Progress Tracking System #237codewkaushik404 wants to merge 33 commits intoOpenLake:mainfrom
codewkaushik404 wants to merge 33 commits intoOpenLake:mainfrom
Conversation
…batches for clubs
…batches for clubs
…cal authentication
Switched to session-based authentication and configured persistent session storage with connect-mongo.
…ession-based authentication and configured persistent session storage with connect-mongo.
…ce error handling, and remove unused code
|
@codewkaushik404 is attempting to deploy a commit to the openlake's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughModularized backend models, added Task and Certificate workflows (models, controllers, routes, validation), introduced Passport (Google + local) and JWT auth variants, switched to MongoDB-backed sessions, updated many route imports, and added frontend Task & Certificate UIs and services. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FE as Frontend
participant Auth as Auth (Passport/JWT)
participant API as Backend (TaskController)
participant DB as MongoDB
User->>FE: Open Task UI / Submit Create Task
FE->>Auth: Send session cookie or JWT
Auth->>API: Validate session/token (attach req.user)
FE->>API: POST /api/tasks (createTask)
API->>DB: Resolve creator -> PositionHolder -> unit_id
API->>DB: Validate assignees exist & authority (PRESIDENT/GENSEC/CLUB rules)
DB-->>API: Validation results
API->>DB: Insert Task (status: pending)
API-->>FE: 201 Created (task)
FE->>User: Render created task
sequenceDiagram
actor Coordinator
participant FE as Frontend
participant Auth as Auth (JWT)
participant API as Backend (CertificateController)
participant OU as Org Models
participant C as Certificate Models
participant DB as MongoDB
Coordinator->>FE: Submit Certificate Batch form
FE->>Auth: Send JWT in cookie
Auth->>API: Verify token, attach req.user
FE->>API: POST /api/certificate-batches
API->>OU: Validate unit is Club and fetch parent Council/category
API->>OU: Find GENSEC_<CATEGORY> user and President PositionHolder
API->>API: Build approverIds [GENSEC, President]
API->>DB: Validate user IDs in users array
API->>C: Create CertificateBatch document
API-->>FE: 201 Created (batch)
FE->>Coordinator: Show success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
backend/config/db.js (1)
7-13:⚠️ Potential issue | 🟠 MajorFail fast on DB connection failure.
At Line 12, the error is only logged. This can let the server continue without a DB connection. Add an env guard and rethrow so startup fails deterministically.
Suggested fix
const connectDB = async () => { try { const ConnectDB = process.env.MONGODB_URI; + if (!ConnectDB) { + throw new Error("MONGODB_URI is not set"); + } //Removing the options as they are no longer needed from mongoose6+ await mongoose.connect(ConnectDB); console.log("MongoDB Connected"); } catch (error) { console.error("MongoDB Connection Error:", error); + throw error; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/config/db.js` around lines 7 - 13, Ensure startup fails deterministically by validating the MONGODB_URI and rethrowing on connection errors: check the ConnectDB (process.env.MONGODB_URI) value before calling mongoose.connect (throw an error if missing), and in the catch block for the mongoose.connect call, after logging via console.error include rethrowing the caught error (or throw a new Error) so the process does not continue without a DB connection.backend/routes/orgUnit.js (1)
110-161:⚠️ Potential issue | 🔴 CriticalMove validation before session creation and fix transaction session handling.
Line 133's validation early return occurs after
startTransaction()(line 111) without cleanup—the session and transaction are abandoned. Additionally, line 153 usessave(session)but Mongoose 7.6.8 requiressave({ session })as shown correctly on line 178.Suggested fix
router.post( "/create", isAuthenticated, authorizeRole([...ROLE_GROUPS.GENSECS, "PRESIDENT"]), async (req, res) => { - const session = await mongoose.startSession(); - session.startTransaction(); - - try { - const { + const { name, type, description, parent_unit_id, hierarchy_level, category, is_active, contact_info, budget_info, - } = req.body; + } = req.body; - if ( - !name || - !type || - !category || - !hierarchy_level || - !contact_info.email - ) { - return res.status(400).json({ - message: - "Validation failed: name, type, category, and hierarchy_level are required.", - }); - } + if (!name || !type || !category || !hierarchy_level || !contact_info?.email) { + return res.status(400).json({ + message: + "Validation failed: name, type, category, and hierarchy_level are required.", + }); + } + + const session = await mongoose.startSession(); + session.startTransaction(); + try { const newUnitData = { unit_id: `org-${uuidv4()}`, name, @@ }; const newUnit = new OrganizationalUnit(newUnitData); - await newUnit.save(session); + await newUnit.save({ session }); @@ await session.commitTransaction(); - session.endSession(); res.status(201).json(newUnit); } catch (error) { await session.abortTransaction(); - session.endSession(); console.error("Error creating organizational unit and user: ", error); @@ res .status(500) .json({ message: "Server error while creating organizational unit." }); + } finally { + session.endSession(); } }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/orgUnit.js` around lines 110 - 161, Move the request-body validation block (the checks for name, type, category, hierarchy_level, contact_info.email) to before creating the mongoose session (before mongoose.startSession()) so you never start a transaction for invalid input; change the Mongoose save call from await newUnit.save(session) to await newUnit.save({ session }) to match Mongoose 7.6.8 API; ensure every early return that aborts the flow (e.g., when existingUser is found) properly calls await session.abortTransaction() and session.endSession() before returning, and ensure the success path calls await session.commitTransaction() followed by session.endSession(); look for symbols session, startSession, startTransaction, abortTransaction, commitTransaction, endSession, newUnit.save, OrganizationalUnit, and User.findOne to apply the fixes.backend/models/passportConfig.js (2)
15-22:⚠️ Potential issue | 🟡 MinorPotential null-pointer dereference on
profile.emails.If the Google profile doesn't include emails (e.g., due to scope issues or account settings), accessing
profile.emails[0].valuewill throw. Add a guard before accessing the array.🛡️ Suggested defensive check
async (accessToken, refreshToken, profile, done) => { // Check if the user already exists in your database + if (!profile.emails || profile.emails.length === 0) { + return done(null, false, { message: "No email found in Google profile." }); + } if (!isIITBhilaiEmail(profile.emails[0].value)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models/passportConfig.js` around lines 15 - 22, Add a null/undefined guard before reading profile.emails inside the async (accessToken, refreshToken, profile, done) callback: check that profile and profile.emails exist and that profile.emails[0] has a value before calling isIITBhilaiEmail; if the guard fails, log a helpful message and call done(null, false, { message: "Email not available from provider." }) (or the existing "Only `@iitbhilai.ac.in` emails are allowed." message) so you avoid a null-pointer dereference when profile.emails is missing.
1-68:⚠️ Potential issue | 🟠 MajorDelete
backend/models/passportConfig.js—this file is dead code.No code in the repository imports from this file. The active Passport configuration is at
backend/config/passportConfig.js(imported bybackend/index.jsandbackend/routes/auth.js). This orphaned file should be removed to prevent confusion and accidental future modifications.Additionally, the active configuration at
backend/config/passportConfig.jsincludes improvements over this file: proper null-safety checks onprofile.emails, session serialization that stores only the user ID instead of the entire user object, and support for both Google and Local authentication strategies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models/passportConfig.js` around lines 1 - 68, This file defines and exports a Passport instance (module.exports = passport) with a GoogleStrategy and serializeUser/deserializeUser implementations but is not used anywhere; remove this orphaned Passport configuration file from the codebase to avoid confusion and duplication, ensuring the active configuration that uses the same exported symbol and improved logic (Google + Local strategies, null-safety for profile.emails, and ID-only session serialization) remains in place and is the sole passport config.frontend/src/config/navbarConfig.js (1)
34-46:⚠️ Potential issue | 🟠 MajorExpose the task page for presidents too.
The task workflow in this PR makes presidents top-level assigners/reviewers, but this config still ends the
PRESIDENTmenu at Line 46 without atasksentry. From the main UI, that role has no obvious way to open the new task screen.🔧 Minimal nav fix
{ key: "profile", label: "Profile", icon: User }, { key: "organization", label: "Clubs", icon: Users }, { key: "certificates", label: "Certificates", icon: Dock }, + { key: "tasks", label: "My Tasks", icon: LayoutList },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/config/navbarConfig.js` around lines 34 - 46, The PRESIDENT navbar entry is missing a tasks route so presidents cannot access the new task screen; update frontend/src/config/navbarConfig.js by adding a menu item into the PRESIDENT array with key "tasks" (label "Tasks") and an appropriate icon (e.g., reuse ClipboardList or a tasks/check icon) alongside the other entries so the president role shows the tasks page in the top-level nav.backend/routes/feedbackRoutes.js (1)
222-229:⚠️ Potential issue | 🟠 MajorAuthorize access to
/:userId.Any authenticated user can read another user's submitted feedback by swapping the path param. Reject unless
req.params.userIdmatchesreq.user.idor the caller is allowed to view others' feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/feedbackRoutes.js` around lines 222 - 229, The GET handler for router.get("/:userId") currently returns any user's feedback once authenticated; add an authorization check that allows access only when req.params.userId === req.user.id or when the caller has an elevated permission (e.g., admin/manager flag or a specific permission on req.user). If unauthorized, short-circuit and respond with 403. Keep the remainder of the handler (the Feedback.find(...).populate(...)) unchanged; reference the existing symbols router.get, isAuthenticated, req.params.userId, req.user.id and Feedback.find to locate where to insert the check.
🟠 Major comments (23)
backend/controllers/eventControllers.js-11-13 (1)
11-13:⚠️ Potential issue | 🟠 MajorIneffective null check -
find()returns an empty array, not null.
Event.find({})always returns an array (possibly empty), nevernullorundefined. This condition will never be true. Check array length instead.🐛 Proposed fix
- if(!latestEvents){ - return res.status(404).json({message: "No events are created"}); - } + if(latestEvents.length === 0){ + return res.status(404).json({message: "No events are created"}); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/eventControllers.js` around lines 11 - 13, The null check on latestEvents after Event.find({}) is ineffective because find() returns an array; update the check in the controller that calls Event.find (the latestEvents variable) to test array length (e.g., if (latestEvents.length === 0) or if (!latestEvents || latestEvents.length === 0)) and return the 404 JSON when the array is empty so empty results are handled correctly.backend/utils/authValidate.js-12-12 (1)
12-12:⚠️ Potential issue | 🟠 MajorName validation is too restrictive.
min(5)rejects valid names like "John", "Mary", "Alex", or "Li". This will cause registration failures for users with short but legitimate names.🐛 Proposed fix
- name: zod.string().min(5), + name: zod.string().min(1, "Name is required"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/authValidate.js` at line 12, The name field validation (name: zod.string().min(5)) is too strict and rejects valid short names; update the schema in authValidate.js to allow shorter legitimate names by replacing .min(5) with a more permissive rule such as .min(2).trim() (e.g., name: zod.string().min(2).trim()) so short names like "Li" are accepted while still trimming whitespace.frontend/src/Components/TaskManagement/taskCard.jsx-77-77 (1)
77-77:⚠️ Potential issue | 🟠 MajorTailwind v4 breaking change: Important modifier syntax.
!rounded-xluses Tailwind v3 syntax. In Tailwind v4, the!modifier must come after the utility name:rounded-xl!.🐛 Proposed fix
<button - className={`text-xs font-semibold border px-3 py-1 !rounded-xl transition-colors ${style.viewBtn}`} + className={`text-xs font-semibold border px-3 py-1 rounded-xl! transition-colors ${style.viewBtn}`} >As per coding guidelines: "Important Modifier - NOW AT THE END (!) - OLD: !text-3xl !font-bold - NEW: text-3xl! font-bold!"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/TaskManagement/taskCard.jsx` at line 77, Update the Tailwind important modifier in the JSX className string: replace the old v3-style "!rounded-xl" with the Tailwind v4 syntax "rounded-xl!" inside the template literal used for the className (the string that includes ${style.viewBtn}) so the element's className attribute uses the new modifier position.backend/routes/task.routes.js-1-4 (1)
1-4:⚠️ Potential issue | 🟠 MajorProtect tasks endpoint with explicit authentication middleware.
The
/api/tasksmount point has no authentication middleware. While Passport is initialized globally, this only sets up methods on the request object—it does not enforce authentication. ThegetTaskscontroller directly accessesreq.user._idwithout validation, causing a runtime error for unauthenticated requests instead of a proper authorization rejection. Add theisAuthenticatedmiddleware to the route:Suggested fix
const router = require("express").Router(); const { getTasks } = require("../controllers/taskController"); +const { isAuthenticated } = require("../middlewares/isAuthenticated"); -router.get("/", getTasks); +router.get("/", isAuthenticated, getTasks); module.exports = router;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/task.routes.js` around lines 1 - 4, The tasks route currently calls router.get("/", getTasks) without enforcing authentication; import and apply your authentication middleware (e.g., isAuthenticated) to the route so it becomes router.get("/", isAuthenticated, getTasks) and ensure the getTasks controller safely accesses req.user._id (validate req.user before using) to avoid runtime errors for unauthenticated requests.frontend/src/Components/Dashboard/Dashboard.jsx-22-23 (1)
22-23:⚠️ Potential issue | 🟠 MajorRedirect incomplete onboarding instead of showing loading state.
Dashboard conflates loading with incomplete onboarding. Other protected components (
ProtectedRoute,RoleRedirect) explicitly handleisOnboardingComplete === falseby redirecting to/onboarding, but Dashboard shows a generic loading screen. When onboarding is definitely incomplete, users should be redirected to the onboarding flow, not kept on a loading screen.Evidence from codebase
ProtectedRoute.js(lines 12–13):if (isOnboardingComplete === false) { return <Navigate to="/onboarding" replace />; }
RoleRedirect.jsx(lines 14–15):if (isOnboardingComplete === false) { return <Navigate to="/onboarding" replace />; }Both components treat
isOnboardingComplete === falseas a distinct state requiring navigation, whereas Dashboard groups it with a loading state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Dashboard/Dashboard.jsx` around lines 22 - 23, Dashboard currently treats isOnboardingComplete === false as a loading state; update the conditional in the Dashboard component so it only shows the loading UI when onboarding state is unknown (e.g., isOnboardingComplete == null) or when !isUserLoggedIn, and explicitly redirect when isOnboardingComplete === false (use <Navigate to="/onboarding" replace />) similar to ProtectedRoute and RoleRedirect; locate the check around isUserLoggedIn/isOnboardingComplete in Dashboard.jsx and change the logic to distinguish null/undefined (loading) from false (navigate).backend/controllers/taskController.js-26-30 (1)
26-30:⚠️ Potential issue | 🟠 MajorReturn an empty array instead of 404 for “no tasks”.
GET /api/taskshas a valid empty state. Line 27 turns that into an error response, which makes a first-time user look like a failed request and complicates the frontend empty-state flow.🔧 Suggested response shape
- if (!tasks || tasks.length === 0) { - return res.status(404).json({ message: "No tasks found" }); - } - - res.json(tasks); + return res.status(200).json(tasks);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/taskController.js` around lines 26 - 30, The handler in taskController.js currently returns a 404 when tasks is empty; change it to return a successful empty-array response so GET /api/tasks has a valid empty state: remove the 404 branch that checks "if (!tasks || tasks.length === 0)" and instead always send res.status(200).json(tasks || []) (or res.json(tasks || [])). Update the code paths around the tasks variable and the response send so frontends receive [] rather than a 404.backend/models/taskSchema.js-12-20 (1)
12-20:⚠️ Potential issue | 🟠 MajorAdd explicit non-empty validation to assignees.
In Mongoose 7.6.x,
required: trueon an array field does not prevent empty arrays—it only fails if the field is undefined or null. Without a custom validator, tasks can be saved with zero assignees, violating the business rule. Add a length validator to enforce at least one assignee.🛡️ Suggested validator
assignees: { type: [ { type: mongoose.Schema.Types.ObjectId, ref: "User", }, ], required: true, + validate: { + validator: (value) => Array.isArray(value) && value.length > 0, + message: "At least one assignee is required", + }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models/taskSchema.js` around lines 12 - 20, The assignees array in the Task schema currently uses required: true but allows empty arrays; update the assignees field in backend/models/taskSchema.js (the assignees property on the Task schema using mongoose.Schema.Types.ObjectId ref "User") to include a custom validator that enforces at least one element (e.g., validate: { validator: v => Array.isArray(v) && v.length > 0, message: "Task must have at least one assignee" }) so saving a Task with an empty assignees array fails validation; keep the existing type and ref and only add the length-check validator alongside required.frontend/src/services/tasks.js-128-130 (1)
128-130:⚠️ Potential issue | 🟠 MajorStub implementation:
createTaskdoes not create tasks.This function returns an empty string and doesn't call the API. According to the PR objectives, task creation should be functional. This appears to be incomplete implementation.
Would you like me to help implement this function to call the backend API?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/services/tasks.js` around lines 128 - 130, The createTask function currently returns an empty string; replace the stub with a real API call that posts newTask to the backend and returns the created task object (or throws/returns an error). Implement createTask to call the appropriate endpoint (e.g., POST /tasks) using the same HTTP client used elsewhere in this file (fetch or axios), send newTask as JSON, check the response status, parse and return the JSON payload (the created task), and handle errors by throwing or returning a consistent error object so callers can handle failures; reference the createTask function to locate where to implement this logic.backend/utils/batchValidate.js-3-3 (1)
3-3:⚠️ Potential issue | 🟠 MajorObjectId regex is too permissive.
The regex
/^[0-9a-zA-Z]{24}$/allows characters that aren't valid in MongoDB ObjectIds. ObjectIds are 24 hexadecimal characters (0-9, a-f). This could allow invalid IDs to pass validation.Proposed fix
-const zodObjectId = zod.string().regex(/^[0-9a-zA-Z]{24}$/, "Invalid ObjectId"); +const zodObjectId = zod.string().regex(/^[0-9a-fA-F]{24}$/, "Invalid ObjectId");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/batchValidate.js` at line 3, The zodObjectId regex is too permissive (allows non-hex chars); update the zodObjectId definition to use a hex-only 24-char regex (e.g., /^[0-9a-fA-F]{24}$/) and keep or adjust the error message accordingly so invalid non-hex IDs are rejected by the validator (refer to zodObjectId constant).frontend/src/services/tasks.js-118-126 (1)
118-126:⚠️ Potential issue | 🟠 MajorInconsistent return types between success and error cases.
fetchTasksreturnsresponse.data(object/array) on success but returns a string (err?.message || "An error...") on error. This makes it difficult for callers to handle the result consistently.Consider throwing the error or returning a consistent shape.
Proposed fix - throw error for caller to handle
export async function fetchTasks() { try { const response = await api.get("api/tasks"); return response.data; } catch (err) { console.error(err); - return err?.message || "An error occurred while fetching tasks"; + throw err; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/services/tasks.js` around lines 118 - 126, The fetchTasks function returns different types on success (response.data) and failure (string), making callers inconsistent; update the catch block in fetchTasks to not return a string but instead rethrow the original error (or throw a new Error) after optional logging so callers can handle errors via try/catch or promise rejection; locate the fetchTasks function in frontend/src/services/tasks.js and replace the current catch behavior (console.error + return err?.message) with logging plus throw err (or throw new Error(err?.message)) to ensure a consistent API surface.frontend/src/Components/TaskManagement/tasks.jsx-296-304 (1)
296-304:⚠️ Potential issue | 🟠 MajorSend stable assignee/assigner identifiers from the create flow.
This payload drops
assigneeIdsand hard-codesassigner: "Profile Name", so the backend can't reliably enforce hierarchy rules or attach the task to real users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/TaskManagement/tasks.jsx` around lines 296 - 304, The create payload in the onCreate call drops stable user IDs and hard-codes assigner, preventing backend from enforcing rules; update the onCreate invocation in tasks.jsx to send real identifiers: include assigneeIds (array of user IDs) alongside assignees (assigneeNames) and replace the hard-coded assigner string with a real assignerId (and optionally assignerName) pulled from the current profile/context before calling onCreate so the backend receives stable identifiers for Task creation.frontend/src/Components/Certificates/CertificatesList.jsx-18-23 (1)
18-23:⚠️ Potential issue | 🟠 MajorReplace the mock certificate dataset before shipping this page.
The live API call is commented out, so every user will see the same hard-coded certificates and real approval/rejection changes will never appear here.
Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Certificates/CertificatesList.jsx` around lines 18 - 23, Replace the hard-coded mockCertificates with a real API call: uncomment and use the existing api.get call (api.get(`/api/certificates/${isUserLoggedIn._id}`)) inside the CertificatesList component so you populate state via setCertificates(response.data) instead of the mockCertificates constant; remove the mockCertificates array and any code that forces those values, and ensure the same fix is applied to the other occurrence referenced around line 52 so approval/rejection actions reflect real backend changes (keep isUserLoggedIn and setCertificates usage intact and handle errors from the API call).backend/controllers/certificateController.js-44-47 (1)
44-47:⚠️ Potential issue | 🟠 MajorGuard
positionbefore reading_id.Line 45 dereferences
positionbefore the null check, so an orphanedPositionHolderturns into a 500 instead of the intended authorization error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/certificateController.js` around lines 44 - 47, The code currently dereferences position._id before checking if Position.findById returned a value (in certificateController.js within the certificate controller using positionHolder and Position), causing a possible 500; change the flow to first check if the fetched position is truthy and return the 403 authorization response if not, then access or log position._id afterwards (i.e., move the console.log(position._id) or any use of position._id to after the null-check). Ensure any subsequent logic that relies on position uses the guarded position variable.backend/controllers/certificateController.js-105-114 (1)
105-114:⚠️ Potential issue | 🟠 MajorCheck
presidentHolderandpresidentObjbefore dereferencing them.If the president position exists but is vacant, or its user record was deleted,
presidentHolder.user_id/presidentObj._idthrows and this endpoint returns a 500 instead of a controlled error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/certificateController.js` around lines 105 - 114, The code dereferences presidentHolder.user_id and presidentObj._id without null checks; before using presidentHolder.user_id, ensure PositionHolder.findOne(...) returned a value (check presidentHolder) and if not return a controlled error (e.g., 404 with "President position is vacant"); only then compute presidentId and call User.findById; after User.findById ensure presidentObj exists before accessing presidentObj._id and if missing return a clear error (e.g., 404 "President user not found"). Use the existing symbols presidentHolder, presidentId, presidentObj and the calls PositionHolder.findOne and User.findById to locate where to add these checks and early returns.frontend/src/Components/TaskManagement/tasks.jsx-18-76 (1)
18-76:⚠️ Potential issue | 🟠 MajorReplace the hard-coded assignee list with server-scoped options.
MOCK_USERSbypasses the PR's PRESIDENT/GENSEC/CLUB_COORDINATOR visibility rules, so the picker can show invalid assignees and hide valid ones as soon as real data diverges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/TaskManagement/tasks.jsx` around lines 18 - 76, Replace the hard-coded MOCK_USERS in tasks.jsx with server-scoped assignee options: remove the MOCK_USERS constant and instead fetch or receive the authoritative assignee list that respects PRESIDENT/GENSEC/CLUB_COORDINATOR visibility rules (e.g., call the existing API helper like fetchAssigneesForCurrentScope or accept a prop/context named assigneeOptions), wire that data into the AssigneePicker usage in this file (populate picker from assigneeOptions/state rather than MOCK_USERS), and add simple loading/error handling so the picker shows the server-provided, visibility-filtered list rather than the local mock.backend/routes/feedbackRoutes.js-29-40 (1)
29-40:⚠️ Potential issue | 🟠 MajorValidate
target_idagainstTargetModelbefore saving feedback.This block only validates the target type. A bogus
target_idstill gets persisted, which leaves orphaned feedback records that/view-feedbackcan't resolve cleanly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/feedbackRoutes.js` around lines 29 - 40, The code validates target_type but not target_id, allowing orphaned feedback; update the POST feedback flow that uses targetModels and TargetModel (lookup by target_type) to asynchronously verify the target exists before creating the feedback: use TargetModel.findByPk(target_id) or TargetModel.findOne({ where: { id: target_id } }) (or the appropriate ORM lookup for your models) and if the lookup returns null respond with res.status(400).json({ message: "Invalid target id" }) instead of proceeding to save; perform this check right after determining TargetModel and before calling the feedback creation logic.frontend/src/Components/TaskManagement/tasks.jsx-501-520 (1)
501-520:⚠️ Potential issue | 🟠 MajorPersist status changes through the task API before updating local state.
This handler only changes React state, so the server-side transition checks never run and the UI can show unauthorized/completed states until the next refresh.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/TaskManagement/tasks.jsx` around lines 501 - 520, handleStatusUpdate currently only mutates local state; call the task update API (e.g., an existing updateTask / putTaskStatus endpoint) to persist status change and notes before calling setTasks. Modify handleStatusUpdate to send id, newStatus, submissionNote, and adminNote to the API, await the response, and only on success update state (using setTasks and the existing progressMap); on failure show/throw the API error and do not update state (or revert any optimistic change). Ensure you handle promise rejection and use the same field names (status, submission_note, admin_notes) so server-side transition checks run and the UI reflects the authoritative server state.backend/config/passportConfig.js-24-29 (1)
24-29:⚠️ Potential issue | 🟠 MajorDon't log raw email addresses on rejected OAuth attempts.
This writes PII into server logs for every blocked login. Log the reason or a redacted identifier instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/config/passportConfig.js` around lines 24 - 29, The code logs raw emails on rejected OAuth attempts in the Google strategy verify block; change the console.log in passportConfig.js to avoid PII by either logging a redacted identifier or a one-way hash instead of profile.emails[0].value and use your app logger (e.g., processLogger.warn) with a clear message. Specifically, update the logic around isIITBhilaiEmail(profile.emails[0].value) to stop emitting the full email string, and instead log something like "Google OAuth blocked for user" plus a redacted form (e.g., keep only domain or mask local-part) or a SHA-256 hash of profile.emails[0].value, while retaining the existing done(null, false, { message: ... }) behavior.backend/routes/feedbackRoutes.js-182-210 (1)
182-210:⚠️ Potential issue | 🟠 MajorDerive
resolved_byfromreq.user, not the request body.Right now an admin can spoof who resolved the feedback, and the raw payload is also written to logs. Use the authenticated user as the source of truth and remove the debug logging.
Suggested fix
async (req, res) => { const feedbackId = req.params.id; - const { actions_taken, resolved_by } = req.body; - console.log(req.body); - console.log("User resolving feedback:", resolved_by); + const { actions_taken } = req.body; if (!actions_taken || actions_taken.trim() === "") { return res.status(400).json({ error: "Resolution comment is required." }); @@ feedback.is_resolved = true; feedback.resolved_at = new Date(); feedback.actions_taken = actions_taken; - feedback.resolved_by = resolved_by; + feedback.resolved_by = req.user.id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/feedbackRoutes.js` around lines 182 - 210, The route handler registered with router.put("/mark-resolved/:id", protected by isAuthenticated and authorizeRole) should stop taking resolved_by from req.body and remove the debug console.log calls; instead derive the resolver from the authenticated user (req.user, e.g. req.user.id or req.user.email as your app uses) and set feedback.resolved_by = req.user.<identifier>, leaving other updates (is_resolved, resolved_at, actions_taken) intact; ensure you delete the two console.log lines and validate actions_taken as before, then save the Feedback instance (Feedback.findById -> feedback.save()) and return the response.backend/controllers/certificateController.js-121-123 (1)
121-123:⚠️ Potential issue | 🟠 MajorFix the validation check to use
.successproperty.
zodObjectId.safeParse()returns an object and will never be falsy, so the conditionif (!validation)always passes. Invalid IDs bypass validation and triggerUser.findById(uid), causing a CastError/500 error. The validation should checkif (!validation.success)instead, matching the correct pattern used elsewhere in the file (lines 33-36).Suggested fix
users.map(async (uid) => { const validation = zodObjectId.safeParse(uid); - if (!validation) { + if (!validation.success) { return { uid, ok: false, reason: "Invalid ID" }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/certificateController.js` around lines 121 - 123, The validation currently uses zodObjectId.safeParse(uid) but incorrectly tests the returned object for truthiness; update the check in the certificateController.js block where zodObjectId.safeParse(uid) is called (variable validation) to use if (!validation.success) so invalid IDs short-circuit and return { uid, ok: false, reason: "Invalid ID" } instead of proceeding to User.findById(uid); keep the same return shape and follow the same pattern used elsewhere in this file.backend/routes/auth.js-145-148 (1)
145-148:⚠️ Potential issue | 🟠 MajorRedirect browser failures back to the SPA.
/google/verifyis reached by top-level browser navigation from Google, so a401JSON body leaves the user stranded on an API URL. Redirect to a frontend login/error route and surface the reason there instead.↩️ Suggested fix
- if (!user) - return res - .status(401) - .json({ message: info?.message || "Google Authentication failed" }); + if (!user) { + const reason = encodeURIComponent( + info?.message || "Google Authentication failed" + ); + return res.redirect(`${process.env.FRONTEND_URL}/login?error=${reason}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 145 - 148, The current /google/verify handler returns a 401 JSON body when no user is found; change it to redirect the browser back to the SPA (e.g., the frontend login/error route) with the error reason as a query parameter so the UI can display it. In the handler that contains the if (!user) check in backend/routes/auth.js, replace the res.status(401).json(...) response with a res.redirect(...) to a configured frontend URL (use your FRONTEND_URL or a configured ROUTE like /login or /auth/error) and append the message/info?.message as an encoded query param (encodeURIComponent) so the SPA can read and surface the failure reason. Ensure you still set an appropriate fallback message when info?.message is missing.backend/routes/auth.js-94-127 (1)
94-127:⚠️ Potential issue | 🟠 MajorHandle duplicate usernames at write time too.
User.findOne({ username })andUser.create(...)can race under concurrent signups. Ifusernameis uniquely indexed, this path currently falls through to the catch block and returns a 500 with the raw database error; if it is not, duplicates can be inserted.🛡️ Suggested fix
- const newUser = await User.create({ - strategy: "local", - username, - password, - personal_info: { - name, - email: username, - }, - role, - }); + try { + await User.create({ + strategy: "local", + username, + password, + personal_info: { + name, + email: username, + }, + role, + }); + } catch (err) { + if (err?.code === 11000) { + return res.status(409).json({ + message: "Account with username already exists", + success: false, + }); + } + throw err; + } ... - } catch (err) { - return res.status(500).json({ message: err.message }); + } catch (err) { + console.error(err); + return res + .status(500) + .json({ message: "Internal server error", success: false }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 94 - 127, The current signup can race between User.findOne and User.create causing duplicate usernames or a DB error; update the signup error handling around User.create to detect a unique-constraint/duplicate-key error (from the database/ORM) and return a 409 with the same "Account with username already exists" JSON shape instead of a 500, while preserving other error paths as 500; locate the logic using User.findOne and User.create in the signup route in backend/routes/auth.js and add specific handling for the DB duplicate-key error case.backend/routes/auth.js-62-70 (1)
62-70:⚠️ Potential issue | 🟠 MajorWrap both
req.login()calls withreq.session.regenerate()to prevent session fixation attacks.Both login flows use a pre-authenticated session without regeneration. This allows attackers to hijack user sessions by planting a session ID before authentication.
Wrap the
req.login()calls at lines 62–71 (local strategy) and 157–173 (Google OAuth) withreq.session.regenerate()as the outer callback.🔐 Suggested fix for local strategy (lines 62–71)
- req.login(user, (err) => { - if (err) - return res.status(500).json({ message: "Internal server error" }); - const { personal_info, role, onboardingComplete, ...restData } = user; - return res.json({ - message: "Login Successful", - success: true, - data: { personal_info, role, onboardingComplete }, - }); - }); + req.session.regenerate((sessionErr) => { + if (sessionErr) { + return res.status(500).json({ message: "Internal server error" }); + } + req.login(user, (err) => { + if (err) { + return res.status(500).json({ message: "Internal server error" }); + } + const { personal_info, role, onboardingComplete } = user; + return res.json({ + message: "Login Successful", + success: true, + data: { personal_info, role, onboardingComplete }, + }); + }); + });Apply the same pattern to the Google callback at lines 157–173.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 62 - 70, The login flows call req.login(user, ...) without regenerating the session, enabling session fixation; wrap each req.login call inside req.session.regenerate(callback) and in that callback call req.login(user, (err) => { handle regenerate error and login error consistently and then return the same JSON response }, ensuring you preserve the existing extracted user fields (personal_info, role, onboardingComplete) and response shape; apply this pattern for both the local login handler (the block using req.login(user, ...)) and the Google OAuth callback handler (the other req.login(user, ...) block) so session regeneration occurs before authentication state is established.
🟡 Minor comments (11)
frontend/src/Components/TaskManagement/taskModal.jsx-102-102 (1)
102-102:⚠️ Potential issue | 🟡 MinorIncorrect fallback -
style.progressis a CSS class, not a number.
style.progress || 0doesn't make sense.style.progressis a Tailwind class string like"bg-rose-400", so the fallback0would never apply (non-empty strings are truthy), and if it did apply,0is not a valid class name.🐛 Proposed fix
<div - className={`h-full rounded-full transition-all duration-500 ${style.progress || 0}`} + className={`h-full rounded-full transition-all duration-500 ${style.progress}`} style={{ width: `${task.progress || 0}%` }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/TaskManagement/taskModal.jsx` at line 102, The className construction in taskModal.jsx is using `style.progress || 0` but `style.progress` is a Tailwind class string (e.g. "bg-rose-400") so the `0` fallback is invalid; change the fallback to an empty string or a sensible default CSS class (e.g. `""` or a default color class) and update the JSX where `className={`h-full rounded-full transition-all duration-500 ${style.progress || 0}`}` appears to use `style.progress || ""` (or a proper default class) so the template always receives a valid class string.frontend/src/Components/Dashboard/Dashboard.jsx-13-13 (1)
13-13:⚠️ Potential issue | 🟡 MinorRemove render-time debug logging.
console.loginside render will fire on every re-render and pollute client logs.Proposed fix
- console.log("Selected Route is: ", selectedRoute);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Dashboard/Dashboard.jsx` at line 13, The render contains a console.log("Selected Route is: ", selectedRoute) that will run on every re-render and should be removed; delete that console.log from the Dashboard component (in frontend/src/Components/Dashboard/Dashboard.jsx) and, if you need to inspect selectedRoute during development, move the logging into a useEffect that depends on selectedRoute (or replace with a debug-only logger) so it only runs when selectedRoute changes.frontend/src/Components/common/Sidebar.jsx-64-64 (1)
64-64:⚠️ Potential issue | 🟡 MinorRemoving tooltip when collapsed may impact accessibility.
Setting
title=""when collapsed removes the native browser tooltip that previously showed the navigation item's label. This tooltip is useful for users who may not recognize icons alone, particularly for accessibility. Consider keeping the tooltip when collapsed or adding anaria-labelattribute.🛡️ Suggested fix to preserve accessibility
- title={isCollapsed ? "" : item.label} + title={item.label} + aria-label={item.label}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/common/Sidebar.jsx` at line 64, The title attribute is being cleared when the sidebar is collapsed which removes the native tooltip and hurts accessibility; update the Sidebar.jsx usage (where title={isCollapsed ? "" : item.label}) to always provide the navigation label (e.g., title={item.label}) and also add an aria-label={item.label} on the same element (or on the interactive control that renders the icon) so screen readers still announce the label when collapsed; keep existing visual behavior unchanged while ensuring item.label is used for both title and aria-label in the component that renders each nav item.frontend/src/hooks/useAuth.js-21-27 (1)
21-27:⚠️ Potential issue | 🟡 MinorRemove debug
console.logstatement.Line 23 contains a debug log that should be removed before merging to avoid cluttering browser console in production.
🧹 Remove debug log
const response = await fetchCredentials(); const user = response.message; - console.log("User is:", user); if (response?.success) { handleLogin(user); - //console.log("User role:", user.role); - //console.log("Onboarding complete:", user.onboardingComplete); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useAuth.js` around lines 21 - 27, Remove the debug console.log in the useAuth hook: delete the line console.log("User is:", user); inside the async flow that awaits fetchCredentials() (the block that assigns const response = await fetchCredentials(); const user = response.message;). Ensure you keep the subsequent logic that checks response?.success and calls handleLogin(user) intact and scan the useAuth hook for any other leftover debug console logs to remove.backend/models/taskSchema.js-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorReject negative progress values.
Line 37 caps the upper bound but still allows
-1,-20, etc. That can leak invalid percentages into the progress bar and any analytics built on this field.🔧 Minimal validation fix
- progress: { type: Number, default: 0, max: 100 }, + progress: { type: Number, default: 0, min: 0, max: 100 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models/taskSchema.js` at line 37, The progress field in the Task schema currently only enforces a max but allows negatives; update the progress definition in backend/models/taskSchema.js (the progress field on the Task schema) to reject negative values by adding a min: 0 (or a validator that enforces 0 <= value <= 100) so negative percentages cannot be stored.frontend/src/Components/Auth/UserOnboarding.jsx-25-31 (1)
25-31:⚠️ Potential issue | 🟡 MinorSilent early return may leave form fields empty.
When
fetchCredentialsreturns no user (e.g., session expired), the early return on line 27 silently exits without notifying the user or redirecting. The form will display with empty name/email fields, which could be confusing.Consider redirecting to login or showing an error state when credentials cannot be fetched.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/UserOnboarding.jsx` around lines 25 - 31, The early return after calling fetchCredentials in the UserOnboarding component silently leaves form fields blank; instead handle the missing user by setting an explicit error/unauthenticated state or redirecting to login. Update the logic around fetchCredentials/response.message: when user is falsy, call your navigation method (e.g., navigate('/login')) or set an error flag via setUserData (or a separate setError state) and ensure the component renders a message/loading/redirect rather than returning silently; adjust any JSX that reads user data to show the error/redirect UI when that flag is set.backend/utils/batchValidate.js-6-6 (1)
6-6:⚠️ Potential issue | 🟡 MinorError message doesn't match the validation constraint.
The
min(5)constraint validates minimum length, but the error message says "Title is required" which describes a presence check, not a length check.Proposed fix
- title: zod.string().min(5, "Title is required"), + title: zod.string().min(5, "Title must be at least 5 characters"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/batchValidate.js` at line 6, The validation message for the "title" schema is misleading: update the constraint on title (the zod string where you call min(5)) so the error text reflects a length rule (e.g., "Title must be at least 5 characters") or alternately add a presence check using .nonempty("Title is required") plus min(5, "Title must be at least 5 characters"); locate the title schema in backend/utils/batchValidate.js and change the message accordingly.frontend/src/Components/Auth/Login.jsx-130-139 (1)
130-139:⚠️ Potential issue | 🟡 MinorInconsistent button text: "Sign up with Google" on the Login page.
The button text says "Sign up with Google" but this is a Login page. This could confuse users about whether they're signing up or logging in.
Proposed fix
> - <span>Sign up with Google</span> + <span>Sign in with Google</span> <GoogleIcon /> </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/Login.jsx` around lines 130 - 139, The Login page button in Login.jsx currently reads "Sign up with Google" which is inconsistent; update the button label inside the button element (the JSX that renders the Google OAuth button in Login.jsx) to "Sign in with Google" (or "Sign in with Google" / "Login with Google") so it matches the page intent—leave the onClick handler (window.location.href -> `${process.env.REACT_APP_BACKEND_URL}/auth/google`) and the GoogleIcon component unchanged.backend/utils/batchValidate.js-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorTypo: "Atleast" should be "At least".
Proposed fix
- users: zod.array(zodObjectId).min(1, "Atleast 1 user must be associated."), + users: zod.array(zodObjectId).min(1, "At least 1 user must be associated."),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils/batchValidate.js` at line 10, Update the validation message for the users field in the zod schema: locate the users: zod.array(zodObjectId).min(1, "Atleast 1 user must be associated.") entry and change the string to use the correct wording "At least 1 user must be associated." so the min() error message is spelled correctly.backend/models/userSchema.js-102-106 (1)
102-106:⚠️ Potential issue | 🟡 MinorMissing semicolon and consider more descriptive env variable name.
Line 104 is missing a semicolon. Also, the environment variable
SALTis typically namedSALT_ROUNDSorBCRYPT_ROUNDSfor clarity.Proposed fix
userSchema.pre("save", async function () { if (!this.isModified("password")) return; - const SALT_ROUNDS = Number(process.env.SALT) || 12 + const SALT_ROUNDS = Number(process.env.SALT_ROUNDS) || 12; this.password = await bcrypt.hash(this.password, SALT_ROUNDS); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/models/userSchema.js` around lines 102 - 106, Add the missing semicolon and make the env var name clearer: in the userSchema.pre("save") async hook (the function that checks this.isModified("password") and calls bcrypt.hash), terminate the SALT_ROUNDS assignment with a semicolon and change the source of the value from process.env.SALT to a more descriptive name like process.env.SALT_ROUNDS (or process.env.BCRYPT_ROUNDS), parsing it safely (Number/parseInt) and keeping the fallback to 12; update the identifier SALT_ROUNDS accordingly so bcrypt.hash(this.password, SALT_ROUNDS) still receives the intended numeric rounds.frontend/src/Components/Certificates/CertificatesList.jsx-93-100 (1)
93-100:⚠️ Potential issue | 🟡 MinorParse date-only strings without
new Date(...).
new Date("2024-01-15")is interpreted in UTC, so users west of UTC can see the previous day. Build the date from its parts instead.Suggested fix
const formatDate = (dateString) => { if (!dateString) return "N/A"; - const date = new Date(dateString); + const [year, month, day] = dateString.split("-").map(Number); + const date = new Date(year, month - 1, day); return date.toLocaleDateString("en-US", { year: "numeric", month: "long", day: "numeric", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Certificates/CertificatesList.jsx` around lines 93 - 100, The formatDate function currently uses new Date(dateString) which treats date-only ISO strings as UTC and can shift the displayed day for users west of UTC; update formatDate to detect date-only strings (e.g., /^\d{4}-\d{2}-\d{2}$/), split into year, month, day and construct the Date via new Date(year, monthIndex, day) (monthIndex = month - 1) so it uses local time, falling back to the original new Date(dateString) behavior if the input is not a date-only string or parsing fails; keep the same toLocaleDateString formatting and the "N/A" return for falsy input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ad1b13a-7c19-453e-9738-605c9afff131
⛔ Files ignored due to path filters (2)
backend/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
.husky/pre-commitbackend/config/db.jsbackend/config/passportConfig.jsbackend/controllers/analyticsController.jsbackend/controllers/certificateController.jsbackend/controllers/dashboardController.jsbackend/controllers/eventControllers.jsbackend/controllers/taskController.jsbackend/index.jsbackend/middlewares/isAuthenticated.jsbackend/models/achievementSchema.jsbackend/models/certificateSchema.jsbackend/models/eventSchema.jsbackend/models/feedbackSchema.jsbackend/models/organizationSchema.jsbackend/models/passportConfig.jsbackend/models/positionHolderSchema.jsbackend/models/positionSchema.jsbackend/models/schema.jsbackend/models/taskSchema.jsbackend/models/userSchema.jsbackend/package.jsonbackend/routes/achievements.jsbackend/routes/analytics.jsbackend/routes/announcements.jsbackend/routes/auth.jsbackend/routes/certificateRoutes.jsbackend/routes/dashboard.jsbackend/routes/events.jsbackend/routes/feedbackRoutes.jsbackend/routes/onboarding.jsbackend/routes/orgUnit.jsbackend/routes/positionRoutes.jsbackend/routes/profile.jsbackend/routes/skillsRoutes.jsbackend/routes/task.routes.jsbackend/utils/authValidate.jsbackend/utils/batchValidate.jsfrontend/src/App.jsxfrontend/src/Components/Auth/Login.jsxfrontend/src/Components/Auth/Register.jsxfrontend/src/Components/Auth/RoleRedirect.jsxfrontend/src/Components/Auth/UserOnboarding.jsxfrontend/src/Components/Certificates/CertificatesList.jsxfrontend/src/Components/Dashboard/Dashboard.jsxfrontend/src/Components/Dashboard/Home.jsxfrontend/src/Components/Dashboard/Sidebar.jsxfrontend/src/Components/TaskManagement/taskCard.jsxfrontend/src/Components/TaskManagement/taskModal.jsxfrontend/src/Components/TaskManagement/tasks.jsxfrontend/src/Components/TaskManagement/ui.jsxfrontend/src/Components/common/Sidebar.jsxfrontend/src/config/dashboardComponents.jsfrontend/src/config/navbarConfig.jsfrontend/src/hooks/useAuth.jsfrontend/src/index.jsxfrontend/src/pages/certificatesPage.jsxfrontend/src/pages/tasksPage.jsxfrontend/src/services/auth.jsfrontend/src/services/tasks.js
💤 Files with no reviewable changes (1)
- frontend/src/Components/Dashboard/Sidebar.jsx
…APIs and role-based user resolution
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
frontend/src/Components/TaskManagement/tasks.jsx (1)
231-233:⚠️ Potential issue | 🟡 MinorUse
_idfor the React key.The task list comes from Mongo documents, so
task.idis not the stable field here. This leaves keys undefined/unstable and has already been flagged elsewhere in the task UI.🧩 Suggested fix
- <TaskCard key={task.id} task={task} View={setSelectedTask} /> + <TaskCard key={task._id} task={task} View={setSelectedTask} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/TaskManagement/tasks.jsx` around lines 231 - 233, The list is using task.id which is not the stable MongoDB identifier; update the TaskCard key to use the Mongo _id field instead (replace uses of task.id with task._id in the mapped render of filtered), e.g. in the mapped expression that renders <TaskCard key={...} task={task} View={setSelectedTask} /> use task._id (or task._id || task.id as a safe fallback) so React gets a stable key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/taskController.js`:
- Around line 152-155: The PRESIDENT branch currently returns all users;
restrict the query so PRESIDENT only sees assignable users (roles matching
GENSEC_* prefix, CLUB_COORDINATOR, and position-holder roles) by replacing
User.find().select("personal_info, role") with a filtered query such as
User.find({ $or: [ { role: { $regex: '^GENSEC_' } }, { role: 'CLUB_COORDINATOR'
}, { role: /* add specific PositionHolder role names or a regex for them */ } ]
}).select('personal_info role') so the response JSON returns only those
assignable users; adjust the PositionHolder filter to match your app's exact
role names.
- Around line 97-135: In createTask: replace the current existence-only check
for assignees (the User.find + existingSet logic) with an authorization check
that verifies each assignee is within the creator's permitted hierarchy before
calling Task.create; specifically, fetch PositionHolder entries for assignees
(e.g., PositionHolder.find({ user_id: { $in: assignees } }).populate({ path:
"position_id", select: "unit_id role..." })) and ensure every returned
assignee.position_id.unit_id (or role) is allowed relative to the creator's
unitId (derived earlier from PositionHolder for userId) using your existing
hierarchy rules (same unit, subordinate units, or allowed roles), and return 403
if any assignee is outside that scope; also replace the current existingSet size
check with validation against the fetched PositionHolder results to catch
missing users and unauthorized assignments before calling Task.create.
- Around line 171-190: The GENSEC branch is querying with the wrong values:
change the OrganizationalUnit lookup to use the scalar category value from the
returned document (use category.category or destructure the category field) when
building the query for OrganizationalUnit.find({ category: ... }) for
categoryOrgsIds, and when collecting users from PositionHolder, select and map
the user_id field (PositionHolder.find(...).select("user_id") and .map(u =>
u.user_id)) so that User.find({ _id: { $in: userIds } }) receives actual user
IDs; update references to the variables category, categoryOrgsIds, positionsIds,
and userIds in the GENSEC branch accordingly.
- Line 153: The Mongoose select calls in taskController are using
comma-separated fields which is invalid; update the User.find() selects to use
space-delimited field names (e.g., change User.find().select("personal_info,
role") to User.find().select("personal_info role") in the code that contains the
User.find() call and the other select call around the same area (the select
usage at lines referenced in the review), ensuring both occurrences use
"personal_info role" so personal_info is returned correctly.
- Around line 48-73: The updateTask flow currently allows any assignee to set
task.status (task.status) to "completed", bypassing the required transition and
reviewer check; modify the updateTask handler to enforce allowed transitions and
actor roles: validate the current task.status and only allow status changes
along the approved flow (pending -> in-progress -> under-review -> completed),
require that only an assignee can change pending->in-progress and
in-progress->under-review, and require that only the assigner (task.assigned_by)
can change under-review->completed (or set an explicit review/approval flag)
before accepting "completed"; keep existing membership checks (task.assigned_by,
assignees.some(...)) but add explicit role-based checks around setting
task.status and reject unauthorized transition attempts with 403 and a clear
message.
In `@backend/index.js`:
- Around line 39-55: The session cookie is currently named "token" in the
session middleware (app.use(session(..., name: "token"))), which conflicts with
backend/middlewares/isAuthenticated.js that expects req.cookies.token to hold a
JWT; rename the session cookie to a non-conflicting name (e.g., "sessionId" or
"sid") in the session configuration and update any code that reads the session
cookie name accordingly so that JWT logic in isAuthenticated.js continues to
read req.cookies.token for the JWT while session middleware uses the new cookie
name.
In `@backend/routes/auth.js`:
- Around line 200-203: The password-reset token flow currently only proves the
token was signed by this app; update the handlers that call jwt.verify (the GET
and POST reset handlers in auth.js) to validate the decoded payload's user
identifier against the target route parameter (i.e., require decoded.id or
decoded.email to match req.params.id or the route's :id/value) before
proceeding, and return an unauthorized/error response if they don't match;
ensure this same check is applied wherever jwt.verify is used for password
resets to prevent replay of a token issued for another account (token creation
is done via jwt.sign({ email, id: user._id }, ...)).
In `@backend/seedTasks.js`:
- Around line 257-267: The seeded tasks currently set submission_note and
admin_notes to empty strings in the tasksToInsert object, which produces
unrealistic rows for statuses like "under-review" and "completed"; update the
seeding logic where tasksToInsert is constructed so that when status ===
"under-review" you populate submission_note with a realistic sample (e.g., a
brief reviewer/requester comment referencing template.title or using a small
random review text), and when status === "completed" you populate admin_notes
with a completion note (e.g., who completed it and a short summary); reference
the tasksToInsert creation block and the status variable/field so the
conditional assigns appropriate non-empty strings instead of always "".
In `@backend/utils/taskValidate.js`:
- Around line 1-2: Update the zodObjectId regex to enforce strictly hexadecimal
characters: replace the current pattern used in the zodObjectId constant
(zodObjectId in backend/utils/taskValidate.js) from /^[0-9a-zA-Z]{24}$/ to a
regex that only allows hex digits (0-9a-fA-F) for exactly 24 characters; apply
the identical change to the zodObjectId definition in
backend/utils/batchValidate.js so both validators only accept valid MongoDB
ObjectId strings.
In `@frontend/src/Components/TaskManagement/taskModal.jsx`:
- Around line 187-190: Change the status sent by the reviewer button from
"pending" to "in-progress" so the review flow returns to in-progress with
reviewer notes; specifically update the onClick call that calls
onStatusUpdate(task._id, "pending", "", adminNote) to use "in-progress"
(preserving the adminNote argument) so tasks.jsx does not reset progress to 0
and reviewer notes are retained before calling onClose().
- Around line 224-233: handleSubmit is currently forcing priority: 0 and only
guards title/deadline, causing backend validation failures; update handleSubmit
to submit a valid priority string (use form.priority if it already holds
"low"|"medium"|"high" or map the numeric UI value to one of
"low"/"medium"/"high") instead of hardcoding 0, and add client-side checks
before calling onCreate to ensure form.description is present and assigneeIds is
non-empty; keep calling onCreate({...form, status: "pending", priority:
<valid-string>, assignees: assigneeIds}) and then onClose() only after these
validations pass.
In `@frontend/src/Components/TaskManagement/tasks.jsx`:
- Around line 61-67: The handler handleCreate currently appends the draft
newTask (missing server _id and populated assignees) into state; instead use the
persisted task returned by the API (e.g., response.task or response.data) when
createTask succeeds, or trigger a refetch of tasks; update the setTasks call to
push the API-returned task (not newTask) so TaskCard/TaskDetailModal receive the
stable Mongo _id and populated assignee objects.
In `@frontend/src/services/auth.js`:
- Around line 9-16: The catch in completeOnboarding(userData) currently returns
error.response which resolves the promise and hides failures; instead either
rethrow the error so callers (e.g., the UserOnboarding.jsx handler) hit their
catch block or return a normalized failure object (e.g., { success: false,
error: ... }) that the caller must explicitly check; update completeOnboarding
to throw (or throw error.response || error) in the catch, or change its return
shape to a consistent { success, data, error } contract and update the caller
accordingly.
---
Duplicate comments:
In `@frontend/src/Components/TaskManagement/tasks.jsx`:
- Around line 231-233: The list is using task.id which is not the stable MongoDB
identifier; update the TaskCard key to use the Mongo _id field instead (replace
uses of task.id with task._id in the mapped render of filtered), e.g. in the
mapped expression that renders <TaskCard key={...} task={task}
View={setSelectedTask} /> use task._id (or task._id || task.id as a safe
fallback) so React gets a stable key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6a048b5-9487-4f31-8099-e1606c316bf5
📒 Files selected for processing (15)
backend/controllers/eventControllers.jsbackend/controllers/taskController.jsbackend/index.jsbackend/routes/analytics.jsbackend/routes/auth.jsbackend/routes/profile.jsbackend/routes/task.routes.jsbackend/seedTasks.jsbackend/utils/taskValidate.jsfrontend/src/Components/TaskManagement/taskAssigneeModal.jsxfrontend/src/Components/TaskManagement/taskCard.jsxfrontend/src/Components/TaskManagement/taskModal.jsxfrontend/src/Components/TaskManagement/tasks.jsxfrontend/src/services/auth.jsfrontend/src/services/tasks.js
✅ Files skipped from review due to trivial changes (1)
- backend/routes/profile.js
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/controllers/eventControllers.js
- frontend/src/Components/TaskManagement/taskCard.jsx
- frontend/src/services/tasks.js
- backend/routes/analytics.js
- backend/routes/task.routes.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/services/auth.js (1)
9-15:⚠️ Potential issue | 🟠 MajorDo not resolve onboarding failures as successful results.
On Line 14, returning
error.responseresolves the promise, so callertry/catchblocks won’t run on API failure.Proposed fix
export async function completeOnboarding(userData) { try { const response = await api.put(`/onboarding`, userData); return response.data; } catch (error) { - //console.error("Error obj is:",error.response); - return error.response; + throw error.response || error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/services/auth.js` around lines 9 - 15, The catch block in the async onboarding API call currently returns error.response (from the api.put(`/onboarding`, userData) call), which resolves the promise and prevents caller try/catch from running; change the catch to propagate the failure instead — either rethrow the caught error (throw error) or return Promise.reject(error) so callers receive a rejected promise. Update the catch in the function containing api.put(`/onboarding`, userData) accordingly and ensure any callers handle the rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/services/auth.js`:
- Around line 18-29: The registerUser function returns different shapes on
success (full response) vs failure (error.response or undefined); change it to
return a consistent success value and throw on error: have registerUser return
response.data (not the full response) and in the catch rethrow a normalized
error (throw error.response || error) so callers always get a thrown error to
handle via try/catch and a predictable success payload; update the catch in
registerUser to rethrow instead of returning.
---
Duplicate comments:
In `@frontend/src/services/auth.js`:
- Around line 9-15: The catch block in the async onboarding API call currently
returns error.response (from the api.put(`/onboarding`, userData) call), which
resolves the promise and prevents caller try/catch from running; change the
catch to propagate the failure instead — either rethrow the caught error (throw
error) or return Promise.reject(error) so callers receive a rejected promise.
Update the catch in the function containing api.put(`/onboarding`, userData)
accordingly and ensure any callers handle the rejection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d78f122-9e09-4db7-8d84-7a54a0583194
📒 Files selected for processing (20)
backend/index.jsfrontend/src/App.jsxfrontend/src/Components/AddPositionHolder.jsxfrontend/src/Components/Announcements/Announcements.jsxfrontend/src/Components/Announcements/AnnouncementsCard.jsxfrontend/src/Components/Announcements/CreateAnnouncementModal.jsxfrontend/src/Components/Auth/Forgot-Password/ForgotPassword.jsxfrontend/src/Components/Auth/Forgot-Password/ResetPassword.jsxfrontend/src/Components/Dashboard/Dashboard.jsxfrontend/src/Components/Dashboard/Home.jsxfrontend/src/Components/Endorsement/Endorsement.jsxfrontend/src/Components/Events/EventTile.jsxfrontend/src/Components/Feedback/FeedbackCard.jsxfrontend/src/Components/Feedback/FeedbackStats.jsxfrontend/src/Components/OldComponents/ClubCoorinatorDashboard.jsxfrontend/src/Components/Positions/PositionCard.jsxfrontend/src/Components/Positions/ViewPositionHolder.jsxfrontend/src/config/dashboardComponents.jsfrontend/src/config/navbarConfig.jsfrontend/src/services/auth.js
💤 Files with no reviewable changes (2)
- frontend/src/Components/Feedback/FeedbackStats.jsx
- frontend/src/Components/Feedback/FeedbackCard.jsx
✅ Files skipped from review due to trivial changes (13)
- frontend/src/Components/Auth/Forgot-Password/ResetPassword.jsx
- frontend/src/Components/OldComponents/ClubCoorinatorDashboard.jsx
- frontend/src/App.jsx
- frontend/src/Components/Positions/PositionCard.jsx
- frontend/src/Components/Endorsement/Endorsement.jsx
- frontend/src/Components/Auth/Forgot-Password/ForgotPassword.jsx
- frontend/src/Components/Announcements/CreateAnnouncementModal.jsx
- frontend/src/Components/Positions/ViewPositionHolder.jsx
- frontend/src/Components/Events/EventTile.jsx
- frontend/src/Components/Announcements/AnnouncementsCard.jsx
- frontend/src/Components/AddPositionHolder.jsx
- frontend/src/Components/Announcements/Announcements.jsx
- backend/index.js
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/Components/Dashboard/Dashboard.jsx
- frontend/src/Components/Dashboard/Home.jsx
- frontend/src/config/dashboardComponents.js
- frontend/src/config/navbarConfig.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
backend/routes/auth.js (1)
264-266:⚠️ Potential issue | 🔴 CriticalPOST reset-password handler is still vulnerable to token replay.
The GET handler was fixed to verify
decoded.idmatches the target user, but this POST handler still doesn't perform that check. A token issued for user A can be replayed against/reset-password/B/...to reset user B's password—this is an account-takeover vulnerability.🔐 Proposed fix
const secret = process.env.JWT_SECRET_TOKEN; try { - jwt.verify(token, secret); + const decoded = jwt.verify(token, secret); + if (decoded.id !== user._id.toString()) { + return res.status(400).json({ message: "Invalid or expired token" }); + } user.setPassword(password, async (error) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 264 - 266, The POST /reset-password handler currently calls jwt.verify(token, secret) but doesn't ensure the token belongs to the target user; extract the decoded payload (e.g., const decoded = jwt.verify(token, secret)) and verify decoded.id (or decoded.userId depending on token shape) strictly equals the route's target user identifier (e.g., req.params.id or req.body.userId used in this handler) before allowing the password change; if they don't match, reject the request (401/403) and do not proceed with updating the password.backend/controllers/taskController.js (3)
203-203:⚠️ Potential issue | 🟡 MinorMongoose
.select()requires space-delimited fields, not comma-separated.
"personal_info, role"will not select both fields correctly. Mongoose interprets this as a single malformed field name.🐛 Proposed fix
- const users = await User.find({ _id: { $in: userIds } }).select("personal_info, role"); + const users = await User.find({ _id: { $in: userIds } }).select("personal_info role");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/taskController.js` at line 203, The select call on the User query in taskController.js is using a comma-separated string ("personal_info, role") which Mongoose treats as a single invalid field; update the User.find(...).select usage (the line that assigns to users) to request fields with space-delimited names (e.g., "personal_info role") or an object/array format so both personal_info and role are actually selected.
201-201:⚠️ Potential issue | 🔴 CriticalStill selecting
_idinstead ofuser_id.The
PositionHolder.find()call selectsuser_idbut the.map()extractsu._id. This returns PositionHolder document IDs instead of the actual user IDs, causing the subsequentUser.find()to return no results.🐛 Proposed fix
- const userIds = (await PositionHolder.find({position_id: { $in: positionsIds }}).select("user_id")).map((u) => u._id); + const userIds = (await PositionHolder.find({position_id: { $in: positionsIds }}).select("user_id")).map((u) => u.user_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/taskController.js` at line 201, The bug is that PositionHolder.find(...).select("user_id") is correct but the mapping extracts u._id (document id) instead of the referenced user id; change the mapping so userIds is built from the user_id field (e.g. map each result to u.user_id or u.user_id.toString()) so the subsequent User.find({ _id: { $in: userIds }}) receives actual user IDs; update the mapping expression in the PositionHolder.find result handling (the arrow function that builds userIds) accordingly.
136-142:⚠️ Potential issue | 🔴 CriticalBackend hierarchy authorization is still missing.
The existence check verifies assignee IDs exist but does not enforce that the creator is authorized to assign tasks to those users based on organizational hierarchy rules. This endpoint remains vulnerable to direct API calls that bypass the frontend's assignee filtering.
The
getTaskUsersfunction already implements the hierarchy logic. Extracting it into a shared helper would enable reuse here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/taskController.js` around lines 136 - 142, The assignee existence check in taskController.js (the existingUserIds / existingSet logic) doesn't enforce organizational hierarchy authorization; reuse the hierarchy logic from getTaskUsers by extracting it into a shared helper (e.g., authorizeTaskAssignees or getAuthorizedAssignees) and call that helper from the controller before accepting assignees. Update the controller to call the new helper (passing the requesting user/context and the requested assignees) and reject with 400 if the helper indicates any assignees are outside the creator's permitted hierarchy; keep the existing User.find existence check as a fallback or combine it into the helper so the controller uses the helper's result to validate assignees. Ensure the helper reuses the same logic currently in getTaskUsers and is imported into taskController.js.frontend/src/Components/TaskManagement/taskModal.jsx (1)
187-191:⚠️ Potential issue | 🔴 Critical"Request Revision" sends
"pending"but backend only accepts"in-progress".The backend's assigner transition validation (lines 76-83 in taskController.js) only permits
under-review → completedorunder-review → in-progress. Sending"pending"will result in a 403 "Invalid assigner transition" error.🐛 Proposed fix
<button onClick={() => { - onStatusUpdate(task._id, "pending", "", adminNote); + onStatusUpdate(task._id, "in-progress", "", adminNote); onClose(); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/TaskManagement/taskModal.jsx` around lines 187 - 191, The "Request Revision" button calls onStatusUpdate(task._id, "pending", "", adminNote) but the backend (taskController.js assigner transition) only accepts "in-progress" from under-review; change the status argument passed by the button to "in-progress" so onStatusUpdate(task._id, "in-progress", "", adminNote) is used (update the handler in taskModal.jsx where onStatusUpdate is invoked and keep onClose and adminNote unchanged).
🧹 Nitpick comments (4)
backend/routes/auth.js (2)
23-30: Consider renamingmessagetodatafor clarity.Using
messageto hold user data is unconventional—typicallymessageis a status string anddataholds the payload. This may confuse frontend consumers who expectresponse.datafor the user object.♻️ Suggested fix
res.json({ - message: { personal_info, role, onboardingComplete, _id }, + data: { personal_info, role, onboardingComplete, _id }, success: true, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 23 - 30, The response object returned by the GET route handler router.get("/fetchAuth", isAuthenticated, function (req, res) { ... }) uses the key message to carry the user payload; rename that key to data (i.e., return { data: { personal_info, role, onboardingComplete, _id }, success: true }) so the payload follows conventional response shape; update any local usages of message in this route and ensure callers expect response.data instead of response.message.
47-76: Outer try-catch is ineffective.The
try-catchblock (lines 48, 73-75) won't catch errors from inside thepassport.authenticatecallback since they occur asynchronously. Error handling is already done correctly within the callback. The outer wrapper is misleading.♻️ Suggested fix
-router.post("/login", async (req, res) => { - try { - passport.authenticate("local", (err, user, info) => { +router.post("/login", (req, res) => { + passport.authenticate("local", (err, user, info) => { if (err) { console.error(err); return res.status(500).json({ message: "Internal server error" }); } if (!user) return res .status(401) .json({ message: info?.message || "Login failed" }); req.login(user, (err) => { if (err) return res.status(500).json({ message: "Internal server error" }); const { personal_info, role, onboardingComplete, ...restData } = user; return res.json({ message: "Login Successful", success: true, data: { personal_info, role, onboardingComplete }, }); }); - })(req, res); - } catch (err) { - return res.status(500).json({ message: err.message }); - } + })(req, res); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 47 - 76, Remove the misleading outer try-catch in the router.post("/login") handler because errors inside the asynchronous passport.authenticate callback aren’t caught by it; instead, keep the existing error handling inside the passport.authenticate callback (and inside req.login) or convert the flow to a Promise/async pattern if you want top-level try/catch. Specifically, delete the try { ... } catch(...) wrapper around passport.authenticate in the POST "/login" route and ensure passport.authenticate("local", (err, user, info) => { ... })(req, res) and req.login(user, ...) continue to handle errors and responses.backend/controllers/taskController.js (1)
86-87: Remove duplicate status assignment.Line 86 already assigns
task.status = status, making the conditional assignment on line 87 redundant.♻️ Proposed fix
- task.status = status; - if (status) task.status = status; + task.status = status;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/taskController.js` around lines 86 - 87, Remove the redundant assignment to task.status: there's already an unconditional assignment "task.status = status" followed immediately by "if (status) task.status = status"; delete the conditional line so only the first assignment remains. Update in the taskController code where task.status is set to ensure no duplicate assignment occurs (leave the existing unconditional assignment in place and remove the conditional one).frontend/src/Components/TaskManagement/taskModal.jsx (1)
224-233: Consider validatingdescriptionandassigneesbefore submission.The form only guards
titleanddeadline(line 226), but the backend'staskValidateschema requiresdescriptionandassigneesas well. Submitting without these will succeed locally but fail on the API call.♻️ Proposed validation
const handleSubmit = (e) => { e.preventDefault(); - if (!form.title || !form.deadline) return; + if (!form.title || !form.deadline || !form.description || assigneeIds.length === 0) { + // Optionally show toast/error feedback + return; + } onCreate({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/TaskManagement/taskModal.jsx` around lines 224 - 233, handleSubmit currently only checks form.title and form.deadline; add checks for form.description (non-empty) and assigneeIds (non-empty array) before calling onCreate to match backend taskValidate. Inside handleSubmit, return early and surface validation (e.g., set an error state or prevent submit) if !form.description.trim() or !assigneeIds.length, and only call onCreate({...form, status: "pending", progress: 0, assignees: assigneeIds}) and onClose() when all four required fields (title, deadline, description, assignees) are present; reference the handleSubmit function, form.description, and assigneeIds when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/taskController.js`:
- Around line 191-194: The code incorrectly calls .map() on the Mongoose Query
returned by OrganizationalUnit.findById(...).select("category"); instead, await
the query result and read the category property from the returned document.
Replace the .map usage with awaiting the query (e.g., call
OrganizationalUnit.findById(unitId).select("category").exec() or await the query
directly), assign unit?.category to the existing category variable, and keep the
existing null check that returns res.status(404).json({ message: "Category not
found" }) when category is missing.
In `@frontend/src/Components/TaskManagement/taskModal.jsx`:
- Around line 220-244: The assigneeNames state is never updated so the UI never
shows selected names; update the AssigneePickerModal onConfirm handler in
taskModal.jsx to also setAssigneeNames (call setAssigneeNames(...)) when
selections are confirmed (alongside setAssigneeIds), or change
AssigneePickerModal.onConfirm to return objects with {id,name} and then extract
ids and names in the callback; reference assigneeNames, setAssigneeNames,
assigneeIds, setAssigneeIds and AssigneePickerModal/onConfirm to locate the
changes.
---
Duplicate comments:
In `@backend/controllers/taskController.js`:
- Line 203: The select call on the User query in taskController.js is using a
comma-separated string ("personal_info, role") which Mongoose treats as a single
invalid field; update the User.find(...).select usage (the line that assigns to
users) to request fields with space-delimited names (e.g., "personal_info role")
or an object/array format so both personal_info and role are actually selected.
- Line 201: The bug is that PositionHolder.find(...).select("user_id") is
correct but the mapping extracts u._id (document id) instead of the referenced
user id; change the mapping so userIds is built from the user_id field (e.g. map
each result to u.user_id or u.user_id.toString()) so the subsequent User.find({
_id: { $in: userIds }}) receives actual user IDs; update the mapping expression
in the PositionHolder.find result handling (the arrow function that builds
userIds) accordingly.
- Around line 136-142: The assignee existence check in taskController.js (the
existingUserIds / existingSet logic) doesn't enforce organizational hierarchy
authorization; reuse the hierarchy logic from getTaskUsers by extracting it into
a shared helper (e.g., authorizeTaskAssignees or getAuthorizedAssignees) and
call that helper from the controller before accepting assignees. Update the
controller to call the new helper (passing the requesting user/context and the
requested assignees) and reject with 400 if the helper indicates any assignees
are outside the creator's permitted hierarchy; keep the existing User.find
existence check as a fallback or combine it into the helper so the controller
uses the helper's result to validate assignees. Ensure the helper reuses the
same logic currently in getTaskUsers and is imported into taskController.js.
In `@backend/routes/auth.js`:
- Around line 264-266: The POST /reset-password handler currently calls
jwt.verify(token, secret) but doesn't ensure the token belongs to the target
user; extract the decoded payload (e.g., const decoded = jwt.verify(token,
secret)) and verify decoded.id (or decoded.userId depending on token shape)
strictly equals the route's target user identifier (e.g., req.params.id or
req.body.userId used in this handler) before allowing the password change; if
they don't match, reject the request (401/403) and do not proceed with updating
the password.
In `@frontend/src/Components/TaskManagement/taskModal.jsx`:
- Around line 187-191: The "Request Revision" button calls
onStatusUpdate(task._id, "pending", "", adminNote) but the backend
(taskController.js assigner transition) only accepts "in-progress" from
under-review; change the status argument passed by the button to "in-progress"
so onStatusUpdate(task._id, "in-progress", "", adminNote) is used (update the
handler in taskModal.jsx where onStatusUpdate is invoked and keep onClose and
adminNote unchanged).
---
Nitpick comments:
In `@backend/controllers/taskController.js`:
- Around line 86-87: Remove the redundant assignment to task.status: there's
already an unconditional assignment "task.status = status" followed immediately
by "if (status) task.status = status"; delete the conditional line so only the
first assignment remains. Update in the taskController code where task.status is
set to ensure no duplicate assignment occurs (leave the existing unconditional
assignment in place and remove the conditional one).
In `@backend/routes/auth.js`:
- Around line 23-30: The response object returned by the GET route handler
router.get("/fetchAuth", isAuthenticated, function (req, res) { ... }) uses the
key message to carry the user payload; rename that key to data (i.e., return {
data: { personal_info, role, onboardingComplete, _id }, success: true }) so the
payload follows conventional response shape; update any local usages of message
in this route and ensure callers expect response.data instead of
response.message.
- Around line 47-76: Remove the misleading outer try-catch in the
router.post("/login") handler because errors inside the asynchronous
passport.authenticate callback aren’t caught by it; instead, keep the existing
error handling inside the passport.authenticate callback (and inside req.login)
or convert the flow to a Promise/async pattern if you want top-level try/catch.
Specifically, delete the try { ... } catch(...) wrapper around
passport.authenticate in the POST "/login" route and ensure
passport.authenticate("local", (err, user, info) => { ... })(req, res) and
req.login(user, ...) continue to handle errors and responses.
In `@frontend/src/Components/TaskManagement/taskModal.jsx`:
- Around line 224-233: handleSubmit currently only checks form.title and
form.deadline; add checks for form.description (non-empty) and assigneeIds
(non-empty array) before calling onCreate to match backend taskValidate. Inside
handleSubmit, return early and surface validation (e.g., set an error state or
prevent submit) if !form.description.trim() or !assigneeIds.length, and only
call onCreate({...form, status: "pending", progress: 0, assignees: assigneeIds})
and onClose() when all four required fields (title, deadline, description,
assignees) are present; reference the handleSubmit function, form.description,
and assigneeIds when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1394be7-f3f3-4afb-8136-cf5e44088d5a
📒 Files selected for processing (4)
backend/controllers/taskController.jsbackend/routes/auth.jsbackend/utils/taskValidate.jsfrontend/src/Components/TaskManagement/taskModal.jsx
✅ Files skipped from review due to trivial changes (1)
- backend/utils/taskValidate.js
Member
|
@codewkaushik404 fix imports in seed.js |
Member
|
@codewkaushik404 remove the hardcoded certificates |
Member
|
@codewkaushik404 Please fix the below in tenure pages, the backend is already running on correct port: |
Member
|
@codewkaushik404 also student role should not have the permission to assign task to others |
Author
|
@harshitap1305 Here are the changes made:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
name: Task Management System
about: Full-stack implementation of task creation, assignment, filtering, and status management with role-based access
title: "PR: Implement Hierarchical Task Assignment & Progress Tracking System "
labels: enhancement
assignees: kaushik
Related Issue
Changes Introduced
useCallbackanduseEffectWhy This Change?
Screenshots / Video (if applicable)
Testing
Documentation Updates
README.mdChecklist
Deployment Notes
💬 Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Improvements