Implementación Completa del Task Management System para MicroboxLabs#7
Implementación Completa del Task Management System para MicroboxLabs#7breyeszx wants to merge 8 commits into
Conversation
WalkthroughThe changes in this pull request encompass the introduction of several new files and components for a task management system built with Next.js. Key additions include user authentication, task management APIs, and user interfaces for both admin and regular users. New types for tasks and users have been defined, and a middleware for authentication has been implemented. The README.md file was deleted, and various components were added to facilitate task creation, editing, and management, along with user registration and login functionalities. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 57
🧹 Outside diff range and nitpick comments (37)
nextjs-task-system/app/page.tsx (1)
7-7: Consider enhancing dark mode implementation.The current dark mode implementation could be improved by respecting system preferences and providing a smooth transition experience.
Consider adding these Tailwind classes to support system preferences and transitions:
- <main className="flex min-h-screen flex-col items-center justify-center bg-gray-100 dark:bg-gray-800"> + <main className="flex min-h-screen flex-col items-center justify-center bg-gray-100 dark:bg-gray-800 transition-colors duration-200">Also, consider adding this to your
tailwind.config.js:module.exports = { darkMode: 'media', // respects system preferences // ... rest of your config }nextjs-task-system/app/api/auth/login/route.ts (3)
55-60: Enhance Session Cookie Security SettingsTo mitigate potential CSRF attacks and enhance security, consider setting the
SameSiteattribute to'strict'and ensuring thesecureattribute is correctly applied in production environments.Apply this diff to adjust the cookie settings:
response.cookies.set("sessionId", sessionId, { httpOnly: true, - secure: process.env.NODE_ENV === "production", + secure: process.env.NODE_ENV !== "development", - sameSite: "lax", + sameSite: "strict", path: "/", });
63-66: Avoid Logging Errors Directly to the Console in ProductionLogging detailed error information to the console may expose sensitive data in production environments. Consider using a logging library that can differentiate between development and production modes and handle logs appropriately.
Also applies to: 102-107
6-13: Reuse Existing User Interface DefinitionsThe
Userinterface is manually defined in this file. If there's a sharedUserinterface elsewhere in the codebase, consider importing it to maintain consistency and reduce duplication.nextjs-task-system/app/api/tasks/[id]/route.ts (1)
31-79: UsePATCHMethod for Partial UpdatesSince only the
statusfield of a task is being updated, it's more appropriate to use thePATCHmethod instead ofPUTto align with RESTful API conventions for partial updates.Apply this diff to change the method signature:
-export async function PUT( +export async function PATCH( request: Request, { params }: { params: { id: string } }, ) { // Function body... }nextjs-task-system/app/api/tasks/route.ts (1)
139-145: Return Updated Task Data After ModificationAfter updating or deleting a task, consider returning the updated task data or a confirmation message with details. This provides immediate feedback to the client and can be useful for front-end updates.
Also applies to: 172-178
nextjs-task-system/app/user/layout.tsx (2)
36-49: Add Error Handling for Fetch RequestsIn the
getCurrentUserfunction, consider adding error handling for the fetch request to manage unexpected errors and provide better user experience.Apply this diff to include error handling:
const getCurrentUser = async () => { try { const response = await fetch("/api/auth/me"); if (response.ok) { const userData = await response.json(); setCurrentUser(userData); } else { + const errorData = await response.json(); + console.error("Error fetching current user:", errorData); router.push("/login"); } } catch (error) { console.error("Error fetching current user:", error); router.push("/login"); } };
145-152: Improve Accessibility for Mobile Toggle ButtonThe mobile toggle button lacks an accessible label for screen readers. Adding an
aria-labelor hidden text improves accessibility for users relying on assistive technologies.Apply this diff to enhance accessibility:
<Button onClick={toggleSidebar}> + <span className="sr-only">Toggle Sidebar</span> <HiMenu className="h-6 w-6" /> </Button>nextjs-task-system/app/admin/layout.tsx (1)
81-83: Addaria-labelto icon-only buttons for accessibilityIcon-only buttons should have
aria-labelattributes to improve accessibility for users relying on screen readers.Apply this diff to add
aria-labelattributes:// Close button for mobile (lines 81-83) <Button color="gray" className="!p-2" onClick={toggleSidebar} + aria-label="Close sidebar" > <HiX className="h-6 w-6" /> </Button> // Toggle button for mobile (lines 160-162) <Button onClick={toggleSidebar} + aria-label="Open sidebar" > <HiMenu className="h-6 w-6" /> </Button>Also applies to: 160-162
nextjs-task-system/app/admin/tasks/[id]/page.tsx (1)
80-82: Replace the browserconfirmwith a modal dialogUsing the native
confirmdialog provides a less consistent user experience and might not align with the application's design. Consider using aModalcomponent for the confirmation.Implement a confirmation modal using
flowbite-react:
- Remove the
confirmcall inhandleSubmit.- Add state to manage the visibility of the confirmation modal.
- Display the modal when the form is submitted.
- Proceed with form submission if the user confirms.
nextjs-task-system/app/user/page.tsx (1)
96-107: Handle fetch errors more gracefully when getting the current userWhen fetching the current user, if an error occurs (e.g., network issue), the user is redirected to the login page without an explanation. Consider providing user feedback or retry logic.
Modify the
getCurrentUserfunction to handle errors:try { const response = await fetch("/api/auth/me"); if (!response.ok) { throw new Error("No authenticated user"); } const userData = await response.json(); setCurrentUserId(userData.id); } catch (error) { console.error("Error fetching current user:", error); + // Optionally display an error message or a notification router.push("/login"); }nextjs-task-system/app/api/auth/me/route.ts (2)
8-10: Enhance authentication error handlingThe error message "Not authenticated" is too generic. Consider providing more specific error codes and messages to help clients handle different authentication scenarios (e.g., expired token, invalid token, etc.).
if (!user) { - return NextResponse.json({ error: "Not authenticated" }, { status: 401 }); + return NextResponse.json({ + error: "authentication_required", + message: "Valid authentication credentials are required to access this resource" + }, { status: 401 }); }
4-20: Implement rate limiting for securityThe authentication endpoint should implement rate limiting to prevent brute force attacks.
Consider implementing rate limiting middleware using a library like
express-rate-limitor implementing a custom solution with Redis. Would you like me to provide an example implementation?nextjs-task-system/Utils/auth.ts (1)
1-1: Remove unused importThe
dbimport is not used in this file.-import db from "./db";nextjs-task-system/hooks/useAuth.ts (1)
16-26: Add refresh token functionalityThe current implementation doesn't handle token refresh or session expiry. Consider adding a refresh mechanism.
Would you like me to provide an implementation for token refresh functionality?
nextjs-task-system/components/Skeletons/DashboardSkeleton.tsx (2)
9-14: Consider using more meaningful keys for mapped elementsUsing array indices as keys (
key={i}) might lead to rendering issues if items are reordered or filtered. While this is less critical for static skeleton loaders, it's a good practice to use more meaningful keys.-{[1, 2, 3].map((i) => ( +{['status-1', 'status-2', 'status-3'].map((key) => ( <div - key={i} + key={key} className="h-24 animate-pulse rounded-lg bg-gray-200 dark:bg-gray-700" ></div> ))}Also applies to: 19-24
3-26: Enhance accessibility with ARIA attributesThe skeleton loader should indicate its loading state to screen readers.
-<div className="space-y-6"> +<div className="space-y-6" role="status" aria-label="Loading dashboard content"> {/* Total Tasks Card */} - <div className="h-32 w-full animate-pulse rounded-lg bg-gray-200 dark:bg-gray-700"></div> + <div className="h-32 w-full animate-pulse rounded-lg bg-gray-200 dark:bg-gray-700" aria-hidden="true"></div> ... + <span className="sr-only">Loading dashboard...</span> </div>nextjs-task-system/Utils/server/auth.ts (1)
15-15: Remove debug logging statementProduction code should not contain debug console logs.
- console.log("Debug - sessionId:", sessionId); // Temporary debug lognextjs-task-system/middleware.ts (1)
11-14: Centralize protected paths configurationMove the protected paths array to a configuration file to maintain consistency across the application.
Create a new file
config/auth.ts:export const AUTH_CONFIG = { protectedPaths: { user: ["/user"], admin: ["/admin"] }, authPages: ["/login", "/register"] } as const;nextjs-task-system/components/Skeletons/TaskCardSkeleton.tsx (1)
1-27: Consider enhancing reusability with customizable dimensionsThe skeleton loader effectively matches the TaskCard structure, but could be more reusable across different card sizes.
Consider accepting props for customizing dimensions:
-const TaskCardSkeleton = () => { +interface TaskCardSkeletonProps { + titleWidth?: string; // e.g., 'w-3/4' + descriptionLines?: number; +} + +const TaskCardSkeleton: React.FC<TaskCardSkeletonProps> = ({ + titleWidth = 'w-3/4', + descriptionLines = 2, +}) => {nextjs-task-system/app/api/tasks/user/[id]/route.ts (1)
1-8: Add type definition for task creation payloadConsider adding an interface to define the expected shape of the POST request body for better type safety and documentation.
interface CreateTaskPayload { title: string; description: string; due_date: string; priority: 'high' | 'medium' | 'low'; status: 'pending' | 'in progress' | 'completed'; comments?: string; }nextjs-task-system/componentes/UserComponents/TaskCard.tsx (2)
5-17: Improve type safety and documentationThe type definition could be improved with better documentation and stricter types.
+ import { Task } from '@/types/tasks'; // Add this import type TaskCardProps = { - task: { - id: number; - title: string; - description: string; - assigned_to: number; - due_date: string; - priority: string; - status: string; - comments?: string; - }; + task: Task; /** Determines the display mode of the card */ variant?: "user" | "admin"; };
20-44: Extract color mapping to constantsThe color mapping functions could be simplified using constants.
+ const PRIORITY_COLORS = { + high: "failure", + medium: "warning", + low: "success", + } as const; + const STATUS_COLORS = { + completed: "success", + "in progress": "warning", + pending: "gray", + } as const; const getPriorityColor = (priority: string) => { - switch (priority.toLowerCase()) { - case "high": - return "failure"; - case "medium": - return "warning"; - case "low": - return "success"; - default: - return "info"; - } + return PRIORITY_COLORS[priority.toLowerCase() as keyof typeof PRIORITY_COLORS] ?? "info"; }; const getStatusColor = (status: string) => { - switch (status.toLowerCase()) { - case "completed": - return "success"; - case "in progress": - return "warning"; - case "pending": - return "gray"; - default: - return "info"; - } + return STATUS_COLORS[status.toLowerCase() as keyof typeof STATUS_COLORS] ?? "info"; };nextjs-task-system/Utils/script.sql (1)
44-68: Optimize view and index configurationThe view and index configuration could be improved for better performance.
CREATE VIEW IF NOT EXISTS tasks_with_users AS SELECT t.id, t.title, t.description, t.due_date, t.priority, t.status, t.comments, t.created_at, t.updated_at, t.user_id, - u1.first_name || ' ' || u1.last_name as creator_name, + COALESCE(u1.first_name || ' ' || u1.last_name, 'Unknown') as creator_name, t.assigned_to, - u2.first_name || ' ' || u2.last_name as assignee_name + COALESCE(u2.first_name || ' ' || u2.last_name, 'Unknown') as assignee_name FROM tasks t - JOIN users u1 ON t.user_id = u1.id - JOIN users u2 ON t.assigned_to = u2.id; + LEFT JOIN users u1 ON t.user_id = u1.id + LEFT JOIN users u2 ON t.assigned_to = u2.id; -- Create indexes to optimize queries CREATE INDEX IF NOT EXISTS idx_tasks_user_id ON tasks(user_id); CREATE INDEX IF NOT EXISTS idx_tasks_assigned_to ON tasks(assigned_to); CREATE INDEX IF NOT EXISTS idx_tasks_status ON tasks(status); CREATE INDEX IF NOT EXISTS idx_users_email ON users(email); + CREATE INDEX IF NOT EXISTS idx_tasks_due_date ON tasks(due_date); + CREATE INDEX IF NOT EXISTS idx_sessions_expires_at ON sessions(expires_at);nextjs-task-system/componentes/Sidebar.tsx (2)
12-15: Consider using a custom hook for window-based initializationThe current window-based initialization could cause hydration mismatches. Consider extracting this logic into a custom hook that handles window resize events properly.
+import { useEffect, useState } from "react"; + +function useWindowSize() { + const [isWide, setIsWide] = useState(false); + + useEffect(() => { + function handleResize() { + setIsWide(window.innerWidth >= 768); + } + + handleResize(); + window.addEventListener('resize', handleResize); + return () => window.removeEventListener('resize', handleResize); + }, []); + + return isWide; +} export function SidebarComponent() { - const isClient = typeof window !== "undefined"; - const [isOpen, setIsOpen] = useState( - isClient ? window.innerWidth >= 768 : false, - ); + const isWide = useWindowSize(); + const [isOpen, setIsOpen] = useState(isWide);
53-55: Consider making logo and brand name configurableThe hardcoded "Flowbite" text and logo path should be made configurable through props.
+interface SidebarComponentProps { + logoSrc?: string; + brandName?: string; +} + -export function SidebarComponent() { +export function SidebarComponent({ + logoSrc = "/favicon.ico", + brandName = "Flowbite" +}: SidebarComponentProps) { // ... - <Sidebar.Logo href="/" img="/favicon.ico" imgAlt="Flowbite logo"> - Flowbite + <Sidebar.Logo href="/" img={logoSrc} imgAlt={`${brandName} logo`}> + {brandName} </Sidebar.Logo>nextjs-task-system/components/TaskComponents/TaskCard.tsx (2)
6-8: Consider moving interfaces to a shared types fileThe
TaskWithUserNameinterface extendsTaskand should be moved to the types directory for better reusability.Move this interface to
@/tipos/tasks.ts:// @/tipos/tasks.ts export interface TaskWithUserName extends Task { user_name?: string; }
46-57: Improve date formatting and status visualizationThe task details could benefit from better formatting and visual representation.
+import { format } from 'date-fns'; + <p> <strong>Assigned to:</strong> {task.user_name || "Unassigned"} </p> <p> - <strong>Due Date:</strong> {task.due_date} + <strong>Due Date:</strong> {format(new Date(task.due_date), 'PPP')} </p> <p> <strong>Priority:</strong> {task.priority} </p> <p> - <strong>Status:</strong> {task.status} + <strong>Status:</strong> + <span className={`inline-flex items-center px-2.5 py-0.5 rounded-full text-xs font-medium + ${task.status === 'completed' ? 'bg-green-100 text-green-800' : + task.status === 'in_progress' ? 'bg-yellow-100 text-yellow-800' : + 'bg-gray-100 text-gray-800'}`}> + {task.status} + </span> </p>nextjs-task-system/app/login/page.tsx (1)
82-87: Add loading state feedbackThe submit button should show a loading state to prevent multiple submissions.
<button type="submit" + disabled={loading} className="focus:ring-primary-300 dark:bg-primary-600 dark:hover:bg-primary-500 dark:focus:ring-primary-800 w-full rounded-lg bg-blue-500 px-5 py-2.5 text-center text-sm font-medium text-white hover:bg-blue-900 focus:outline-none focus:ring-4" > - Sign in + {loading ? ( + <span className="flex items-center justify-center"> + <svg className="animate-spin h-5 w-5 mr-3" viewBox="0 0 24 24"> + <circle className="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" strokeWidth="4" fill="none" /> + <path className="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z" /> + </svg> + Signing in... + </span> + ) : ( + 'Sign in' + )} </button>nextjs-task-system/app/admin/page.tsx (2)
1-36: LGTM! Consider exporting the TaskSummary type.The type definition and state management setup are well-structured. Consider exporting the
TaskSummarytype if it might be useful in other components.-type TaskSummary = { +export type TaskSummary = {
93-104: Enhance accessibility for status cards.Consider adding ARIA labels and roles to improve screen reader support for the status cards.
<Card className="bg-yellow-50"> - <div className="text-center"> + <div className="text-center" role="status" aria-label="Pending tasks count"> <h3 className="text-lg font-semibold text-yellow-700"> Pending </h3>Also applies to: 156-173
nextjs-task-system/app/admin/tasks/page.tsx (2)
24-54: Optimize filter performance with debouncing.The current implementation triggers filtering on every state change. Consider debouncing the filter operation to improve performance with large datasets.
+import { useCallback, useEffect } from 'react'; +import debounce from 'lodash/debounce'; export default function TasksPage() { + const debouncedFilter = useCallback( + debounce(() => { + let filtered = tasks; + if (priorityFilter) { + filtered = filtered.filter((task) => task.priority === priorityFilter); + } + if (statusFilter) { + filtered = filtered.filter((task) => task.status === statusFilter); + } + setFilteredTasks(filtered); + }, 300), + [tasks, priorityFilter, statusFilter] + ); useEffect(() => { - filterTasks(); + debouncedFilter(); + return () => debouncedFilter.cancel(); }, [priorityFilter, statusFilter, tasks]);
65-84: Implement optimistic updates for better UX.Consider implementing optimistic updates to improve the perceived performance of the delete operation.
const handleDeleteConfirm = async () => { if (!taskToDelete) return; + const previousTasks = tasks; + // Optimistically update UI + setTasks(tasks.filter((task) => task.id !== taskToDelete.id)); + setShowDeleteModal(false); + setTaskToDelete(null); try { const response = await fetch(`/api/tasks/${taskToDelete.id}`, { method: "DELETE", }); if (!response.ok) { throw new Error("Failed to delete task"); } - // Actualizar la lista de tareas - setTasks(tasks.filter((task) => task.id !== taskToDelete.id)); - setShowDeleteModal(false); - setTaskToDelete(null); } catch (error) { console.error("Error deleting task:", error); + // Revert on error + setTasks(previousTasks); + alert("Failed to delete task. Please try again."); } };nextjs-task-system/app/user/tasks/page.tsx (4)
7-13: Consider adding audit fields to the User interfaceFor better tracking and compliance, consider adding
createdAtandupdatedAtfields to the User interface.interface User { id: number; email: string; firstName: string; lastName: string; role: string; + createdAt: string; + updatedAt: string; }
113-135: Refactor color mapping functions for better maintainabilityConsider moving color mappings to constants and using object literals for better maintainability.
+ const STATUS_COLORS = { + completed: "bg-green-100 text-green-800", + "in progress": "bg-blue-100 text-blue-800", + pending: "bg-yellow-100 text-yellow-800", + } as const; + const PRIORITY_COLORS = { + high: "bg-red-100 text-red-800", + medium: "bg-orange-100 text-orange-800", + low: "bg-green-100 text-green-800", + } as const; const getStatusColor = (status: string) => { - switch (status.toLowerCase()) { - case "completed": - return "bg-green-100 text-green-800"; - case "in progress": - return "bg-blue-100 text-blue-800"; - default: - return "bg-yellow-100 text-yellow-800"; - } + return STATUS_COLORS[status.toLowerCase() as keyof typeof STATUS_COLORS] ?? STATUS_COLORS.pending; }; const getPriorityColor = (priority: string) => { - switch (priority.toLowerCase()) { - case "high": - return "bg-red-100 text-red-800"; - case "medium": - return "bg-orange-100 text-orange-800"; - case "low": - return "bg-green-100 text-green-800"; - default: - return "bg-gray-100 text-gray-800"; - } + return PRIORITY_COLORS[priority.toLowerCase() as keyof typeof PRIORITY_COLORS] ?? "bg-gray-100 text-gray-800"; };
137-139: Enhance loading and empty statesConsider adding more informative loading and empty states with proper accessibility attributes.
- if (!currentUser) return <div>Loading...</div>; - if (tasks.length === 0) return <div>No tasks found.</div>; + if (!currentUser) return ( + <div role="status" aria-live="polite" className="text-center p-4"> + <div className="animate-spin h-8 w-8 border-4 border-blue-500 border-t-transparent rounded-full mx-auto mb-2"></div> + <p>Loading user data...</p> + </div> + ); + if (tasks.length === 0) return ( + <div role="status" aria-live="polite" className="text-center p-4"> + <p>No tasks found. Tasks assigned to you will appear here.</p> + </div> + );
144-170: Extract TaskCard component for better maintainabilityConsider extracting the task card into a separate component to improve code organization.
+ interface TaskCardProps { + task: Task; + onClick: (task: Task) => void; + } + + const TaskCard: React.FC<TaskCardProps> = ({ task, onClick }) => ( + <div + className="cursor-pointer rounded-lg border bg-white p-4 shadow-sm transition-shadow hover:shadow-md" + onClick={() => onClick(task)} + > + <div className="mb-2 flex items-start justify-between"> + <h2 className="text-xl font-semibold">{task.title}</h2> + <span + className={`rounded-full px-2 py-1 text-xs font-medium ${getPriorityColor(task.priority)}`} + > + {task.priority} + </span> + </div> + <p className="mb-3 text-gray-600">{task.description}</p> + <div className="flex flex-col space-y-2"> + <span + className={`w-fit rounded-full px-2 py-1 text-xs font-medium ${getStatusColor(task.status)}`} + > + {task.status} + </span> + <p className="text-sm text-gray-500"> + Due: {format(new Date(task.dueDate), "MMM dd, yyyy")} + </p> + </div> + </div> + ); {tasks.map((task) => ( - <div key={task.id} className="cursor-pointer rounded-lg border bg-white p-4 shadow-sm transition-shadow hover:shadow-md" onClick={() => openModal(task)}> - {/* ... task card content ... */} - </div> + <TaskCard key={task.id} task={task} onClick={openModal} /> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
nextjs-task-system/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
README.md(0 hunks)nextjs-task-system/Utils/auth.ts(1 hunks)nextjs-task-system/Utils/db.ts(1 hunks)nextjs-task-system/Utils/script.sql(1 hunks)nextjs-task-system/Utils/server/auth.ts(1 hunks)nextjs-task-system/app/admin/create/page.tsx(1 hunks)nextjs-task-system/app/admin/layout.tsx(1 hunks)nextjs-task-system/app/admin/page.tsx(1 hunks)nextjs-task-system/app/admin/tasks/[id]/page.tsx(1 hunks)nextjs-task-system/app/admin/tasks/page.tsx(1 hunks)nextjs-task-system/app/api/auth/currentUser.ts(1 hunks)nextjs-task-system/app/api/auth/login/route.ts(1 hunks)nextjs-task-system/app/api/auth/logout/route.ts(1 hunks)nextjs-task-system/app/api/auth/me/route.ts(1 hunks)nextjs-task-system/app/api/auth/register/route.ts(1 hunks)nextjs-task-system/app/api/tasks/[id]/route.ts(1 hunks)nextjs-task-system/app/api/tasks/route.ts(1 hunks)nextjs-task-system/app/api/tasks/user/[id]/route.ts(1 hunks)nextjs-task-system/app/api/users/route.ts(1 hunks)nextjs-task-system/app/layout.tsx(1 hunks)nextjs-task-system/app/login/page.tsx(1 hunks)nextjs-task-system/app/page.tsx(1 hunks)nextjs-task-system/app/register/page.tsx(1 hunks)nextjs-task-system/app/user/layout.tsx(1 hunks)nextjs-task-system/app/user/page.tsx(1 hunks)nextjs-task-system/app/user/tasks/page.tsx(1 hunks)nextjs-task-system/componentes/AdminComponents/TaskCard.tsx(1 hunks)nextjs-task-system/componentes/Sidebar.tsx(1 hunks)nextjs-task-system/componentes/UserComponents/TaskCard.tsx(1 hunks)nextjs-task-system/components/Skeletons/DashboardSkeleton.tsx(1 hunks)nextjs-task-system/components/Skeletons/TaskCardSkeleton.tsx(1 hunks)nextjs-task-system/components/TaskComponents/TaskCard.tsx(1 hunks)nextjs-task-system/hooks/useAuth.ts(1 hunks)nextjs-task-system/middleware.ts(1 hunks)nextjs-task-system/package.json(1 hunks)nextjs-task-system/tipos/tasks.ts(1 hunks)nextjs-task-system/tipos/usuarios.ts(1 hunks)nextjs-task-system/tsconfig.json(2 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- README.md
✅ Files skipped from review due to trivial changes (3)
- package.json
- nextjs-task-system/Utils/db.ts
- nextjs-task-system/tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
nextjs-task-system/app/admin/page.tsx
[error] 63-63: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 68-68: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
nextjs-task-system/app/user/page.tsx
[error] 146-146: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 150-150: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (10)
nextjs-task-system/app/page.tsx (2)
1-4: LGTM! Clean imports and proper use of client directive.
The imports are minimal and the "use client" directive is correctly used for client-side interactivity.
5-5: LGTM! Clear and descriptive component naming.
The rename from Home to HomePage improves clarity and follows React component naming conventions.
nextjs-task-system/app/api/auth/login/route.ts (1)
15-67: LGTM!
The authentication API endpoints are correctly implemented with proper error handling and security measures.
Also applies to: 69-108
nextjs-task-system/app/api/tasks/[id]/route.ts (2)
1-27: LGTM!
The GET and DELETE methods are implemented correctly with appropriate error handling and validations.
Also applies to: 128-145
81-121:
Remove the Non-Standard UPDATE HTTP Method
The UPDATE function (lines 81-121) uses a non-standard HTTP method UPDATE, which is not recognized in the HTTP protocol. Consider removing this function and consolidating its logic into the standard PATCH method for partial updates.
Apply this diff to remove the UPDATE method:
-// PUT: Update task status
-export async function UPDATE(
- request: Request,
- { params }: { params: { id: string } },
-) {
- // Function body...
-}Likely invalid or redundant comment.
nextjs-task-system/app/api/tasks/route.ts (1)
8-35: LGTM!
The GET method successfully retrieves tasks with proper authentication and authorization checks.
nextjs-task-system/app/user/layout.tsx (1)
1-157: LGTM!
The UserLayout component is well-structured and provides a responsive and user-friendly interface.
nextjs-task-system/package.json (1)
20-20: Verify Next.js major version upgrade compatibility
The upgrade from Next.js 14 to 15 is a major version change that might introduce breaking changes.
✅ Verification successful
Let me gather more information about the app directory structure and any potential breaking changes.
Let me check the specific usage of navigation-related features since they had significant changes in Next.js 15.
Let me check the middleware implementation and the specific usage of navigation methods since these are critical areas in Next.js 15.
Next.js 15 upgrade appears safe to proceed
Based on the comprehensive analysis of the codebase:
- The app uses the App Router pattern which is fully supported in Next.js 15
- All route handlers use the stable
NextResponseandNextRequestAPIs - Navigation methods (
router.push,router.refresh) are used in a way that's compatible with Next.js 15 - The middleware implementation follows the current patterns
- No usage of deprecated data fetching methods (
getInitialProps,getServerSideProps,getStaticProps)
The codebase appears to be using modern Next.js patterns that are maintained in version 15, with no deprecated APIs or breaking changes that would affect the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Next.js 15 breaking changes in the codebase
echo "Checking for potential breaking changes..."
# Check for usage of removed or changed APIs
rg -l "getInitialProps|getServerSideProps|getStaticProps|getStaticPaths"
# Check for app directory structure which might need updates
fd -t d "app" -x echo "Found app directory: {}"
Length of output: 263
Script:
#!/bin/bash
# Check the contents of the app directory and its structure
fd . ./nextjs-task-system/app -t f
# Check for usage of specific Next.js features that might be affected
rg -l "useRouter|Link|Image|next/router|next/navigation"
# Check package.json to see the previous Next.js version
cat nextjs-task-system/package.json
Length of output: 2632
Script:
#!/bin/bash
# Check for specific usage of navigation features that might need updates
rg -A 2 "useRouter|next/navigation"
# Check for usage of middleware
fd middleware.ts
# Check for any custom server configuration
fd next.config.js -x cat {}
Length of output: 4148
Script:
#!/bin/bash
# Check middleware implementation
cat nextjs-task-system/middleware.ts
# Check for specific navigation method calls that might need updates
rg -A 2 "router\.(push|replace|back|forward|refresh|prefetch)"
# Check for any usage of deprecated or changed APIs in route handlers
rg -A 2 "NextResponse|NextRequest" ./nextjs-task-system/app/api
Length of output: 21154
nextjs-task-system/app/api/users/route.ts (1)
28-33:
Use parameterized queries to prevent SQL injection
The current implementation is vulnerable to SQL injection attacks.
While SQLite's prepare statement provides some protection, ensure consistent usage:
- const statement = db.prepare(
- "INSERT INTO users (first_name, last_name, email, password, role) VALUES (?, ?, ?, ?, ?)",
- );
+ const statement = db.prepare(`
+ INSERT INTO users (
+ first_name,
+ last_name,
+ email,
+ password,
+ role
+ ) VALUES (
+ @firstName,
+ @lastName,
+ @email,
+ @password,
+ @role
+ )
+ `);Likely invalid or redundant comment.
nextjs-task-system/app/user/tasks/page.tsx (1)
29-35: LGTM! Clean state management setup
The state management is well-organized with clear separation of concerns for tasks, selected task, modal state, and user data.
| interface Task { | ||
| id: number; | ||
| title: string; | ||
| description: string; | ||
| status: string; | ||
| priority: string; | ||
| due_date: string; | ||
| comments: string; | ||
| creator_name: string; | ||
| assignee_name: string; | ||
| created_at: string; | ||
| updated_at: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent naming conventions in the Task interface
The interface mixes snake_case and camelCase naming conventions. Consider using consistent camelCase for better TypeScript conventions.
interface Task {
id: number;
title: string;
description: string;
status: string;
priority: string;
- due_date: string;
+ dueDate: string;
comments: string;
- creator_name: string;
- assignee_name: string;
- created_at: string;
- updated_at: string;
+ creatorName: string;
+ assigneeName: string;
+ createdAt: string;
+ updatedAt: string;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface Task { | |
| id: number; | |
| title: string; | |
| description: string; | |
| status: string; | |
| priority: string; | |
| due_date: string; | |
| comments: string; | |
| creator_name: string; | |
| assignee_name: string; | |
| created_at: string; | |
| updated_at: string; | |
| } | |
| interface Task { | |
| id: number; | |
| title: string; | |
| description: string; | |
| status: string; | |
| priority: string; | |
| dueDate: string; | |
| comments: string; | |
| creatorName: string; | |
| assigneeName: string; | |
| createdAt: string; | |
| updatedAt: string; | |
| } |
| const getCurrentUser = async () => { | ||
| try { | ||
| const response = await fetch("/api/auth/me"); | ||
| if (response.ok) { | ||
| const userData = await response.json(); | ||
| setCurrentUser(userData); | ||
| } else { | ||
| router.push("/login"); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error fetching current user:", error); | ||
| router.push("/login"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in getCurrentUser
The error handling could be enhanced to provide better user feedback and distinguish between different types of errors.
const getCurrentUser = async () => {
try {
const response = await fetch("/api/auth/me");
if (response.ok) {
const userData = await response.json();
setCurrentUser(userData);
} else {
+ const errorData = await response.json();
+ if (response.status === 401) {
router.push("/login");
+ } else {
+ throw new Error(errorData.message || 'Failed to fetch user data');
+ }
}
} catch (error) {
- console.error("Error fetching current user:", error);
+ const errorMessage = error instanceof Error ? error.message : 'An unexpected error occurred';
+ // Consider using a toast notification system
+ console.error(errorMessage);
router.push("/login");
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getCurrentUser = async () => { | |
| try { | |
| const response = await fetch("/api/auth/me"); | |
| if (response.ok) { | |
| const userData = await response.json(); | |
| setCurrentUser(userData); | |
| } else { | |
| router.push("/login"); | |
| } | |
| } catch (error) { | |
| console.error("Error fetching current user:", error); | |
| router.push("/login"); | |
| } | |
| }; | |
| const getCurrentUser = async () => { | |
| try { | |
| const response = await fetch("/api/auth/me"); | |
| if (response.ok) { | |
| const userData = await response.json(); | |
| setCurrentUser(userData); | |
| } else { | |
| const errorData = await response.json(); | |
| if (response.status === 401) { | |
| router.push("/login"); | |
| } else { | |
| throw new Error(errorData.message || 'Failed to fetch user data'); | |
| } | |
| } | |
| } catch (error) { | |
| const errorMessage = error instanceof Error ? error.message : 'An unexpected error occurred'; | |
| // Consider using a toast notification system | |
| console.error(errorMessage); | |
| router.push("/login"); | |
| } | |
| }; |
| const fetchTasks = async () => { | ||
| if (!currentUser) return; | ||
|
|
||
| try { | ||
| const response = await fetch(`/api/tasks/user/${currentUser.id}`); | ||
| const data = await response.json(); | ||
| setTasks(data); | ||
| } catch (error) { | ||
| console.error("Error fetching tasks:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance fetchTasks with proper loading states and error handling
Consider adding loading states and proper error handling for better user experience.
const fetchTasks = async () => {
if (!currentUser) return;
+ setIsLoading(true);
try {
const response = await fetch(`/api/tasks/user/${currentUser.id}`);
+ if (!response.ok) {
+ const errorData = await response.json();
+ throw new Error(errorData.message || 'Failed to fetch tasks');
+ }
const data = await response.json();
setTasks(data);
} catch (error) {
- console.error("Error fetching tasks:", error);
+ const errorMessage = error instanceof Error ? error.message : 'An unexpected error occurred';
+ // Consider using a toast notification system
+ console.error(errorMessage);
} finally {
+ setIsLoading(false);
}
};Committable suggestion skipped: line range outside the PR's diff.
| const updateStatus = async () => { | ||
| if (!selectedTask) return; | ||
|
|
||
| try { | ||
| const response = await fetch(`/api/tasks/${selectedTask.id}`, { | ||
| method: "PUT", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ status: newStatus }), | ||
| }); | ||
|
|
||
| if (response.ok) { | ||
| const updatedTask = await response.json(); | ||
| setTasks((prevTasks) => | ||
| prevTasks.map((task) => | ||
| task.id === selectedTask.id | ||
| ? { ...task, status: updatedTask.status } | ||
| : task, | ||
| ), | ||
| ); | ||
| setShowModal(false); | ||
| } else { | ||
| const errorData = await response.json(); | ||
| alert(`Error: ${errorData.error}`); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error updating task status:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance updateStatus with optimistic updates and validation
Consider implementing optimistic updates and status validation for better UX and data integrity.
+ const VALID_STATUSES = ['pending', 'in progress', 'completed'] as const;
+ type TaskStatus = typeof VALID_STATUSES[number];
const updateStatus = async () => {
if (!selectedTask) return;
+ if (!VALID_STATUSES.includes(newStatus as TaskStatus)) {
+ alert('Invalid status value');
+ return;
+ }
+ // Optimistic update
+ const previousTasks = [...tasks];
+ setTasks((prevTasks) =>
+ prevTasks.map((task) =>
+ task.id === selectedTask.id
+ ? { ...task, status: newStatus }
+ : task
+ )
+ );
try {
const response = await fetch(`/api/tasks/${selectedTask.id}`, {
method: "PUT",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ status: newStatus }),
});
if (response.ok) {
const updatedTask = await response.json();
setTasks((prevTasks) =>
prevTasks.map((task) =>
task.id === selectedTask.id
? { ...task, status: updatedTask.status }
: task
)
);
setShowModal(false);
} else {
const errorData = await response.json();
+ // Revert optimistic update on error
+ setTasks(previousTasks);
alert(`Error: ${errorData.error}`);
}
} catch (error) {
console.error("Error updating task status:", error);
+ // Revert optimistic update on error
+ setTasks(previousTasks);
+ alert('Failed to update task status. Please try again.');
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateStatus = async () => { | |
| if (!selectedTask) return; | |
| try { | |
| const response = await fetch(`/api/tasks/${selectedTask.id}`, { | |
| method: "PUT", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ status: newStatus }), | |
| }); | |
| if (response.ok) { | |
| const updatedTask = await response.json(); | |
| setTasks((prevTasks) => | |
| prevTasks.map((task) => | |
| task.id === selectedTask.id | |
| ? { ...task, status: updatedTask.status } | |
| : task, | |
| ), | |
| ); | |
| setShowModal(false); | |
| } else { | |
| const errorData = await response.json(); | |
| alert(`Error: ${errorData.error}`); | |
| } | |
| } catch (error) { | |
| console.error("Error updating task status:", error); | |
| } | |
| }; | |
| const VALID_STATUSES = ['pending', 'in progress', 'completed'] as const; | |
| type TaskStatus = typeof VALID_STATUSES[number]; | |
| const updateStatus = async () => { | |
| if (!selectedTask) return; | |
| if (!VALID_STATUSES.includes(newStatus as TaskStatus)) { | |
| alert('Invalid status value'); | |
| return; | |
| } | |
| // Optimistic update | |
| const previousTasks = [...tasks]; | |
| setTasks((prevTasks) => | |
| prevTasks.map((task) => | |
| task.id === selectedTask.id | |
| ? { ...task, status: newStatus } | |
| : task | |
| ) | |
| ); | |
| try { | |
| const response = await fetch(`/api/tasks/${selectedTask.id}`, { | |
| method: "PUT", | |
| headers: { "Content-Type": "application/json" }, | |
| body: JSON.stringify({ status: newStatus }), | |
| }); | |
| if (response.ok) { | |
| const updatedTask = await response.json(); | |
| setTasks((prevTasks) => | |
| prevTasks.map((task) => | |
| task.id === selectedTask.id | |
| ? { ...task, status: updatedTask.status } | |
| : task | |
| ), | |
| ); | |
| setShowModal(false); | |
| } else { | |
| const errorData = await response.json(); | |
| // Revert optimistic update on error | |
| setTasks(previousTasks); | |
| alert(`Error: ${errorData.error}`); | |
| } | |
| } catch (error) { | |
| console.error("Error updating task status:", error); | |
| // Revert optimistic update on error | |
| setTasks(previousTasks); | |
| alert('Failed to update task status. Please try again.'); | |
| } | |
| }; |
| {selectedTask && ( | ||
| <Modal show={showModal} onClose={() => setShowModal(false)} size="xl"> | ||
| <Modal.Header>Task Details</Modal.Header> | ||
| <Modal.Body> | ||
| <div className="space-y-4"> | ||
| <div> | ||
| <h3 className="mb-2 text-xl font-semibold"> | ||
| {selectedTask.title} | ||
| </h3> | ||
| <p className="text-gray-600">{selectedTask.description}</p> | ||
| </div> | ||
|
|
||
| <div className="grid grid-cols-2 gap-4"> | ||
| <div> | ||
| <p className="text-sm text-gray-500">Status</p> | ||
| <Select | ||
| value={newStatus} | ||
| onChange={handleStatusChange} | ||
| className="mt-1" | ||
| > | ||
| <option value="pending">Pending</option> | ||
| <option value="in progress">In Progress</option> | ||
| <option value="completed">Completed</option> | ||
| </Select> | ||
| </div> | ||
|
|
||
| <div> | ||
| <p className="text-sm text-gray-500">Priority</p> | ||
| <p | ||
| className={`mt-1 w-fit rounded-full px-2 py-1 text-sm font-medium ${getPriorityColor(selectedTask.priority)}`} | ||
| > | ||
| {selectedTask.priority} | ||
| </p> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="grid grid-cols-2 gap-4"> | ||
| <div> | ||
| <p className="text-sm text-gray-500">Assigned To</p> | ||
| <p className="mt-1">{selectedTask.assignee_name}</p> | ||
| </div> | ||
| <div> | ||
| <p className="text-sm text-gray-500">Created By</p> | ||
| <p className="mt-1">{selectedTask.creator_name}</p> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="grid grid-cols-2 gap-4"> | ||
| <div> | ||
| <p className="text-sm text-gray-500">Due Date</p> | ||
| <p className="mt-1"> | ||
| {format(new Date(selectedTask.due_date), "MMMM dd, yyyy")} | ||
| </p> | ||
| </div> | ||
| <div> | ||
| <p className="text-sm text-gray-500">Created At</p> | ||
| <p className="mt-1"> | ||
| {format( | ||
| new Date(selectedTask.created_at), | ||
| "MMM dd, yyyy HH:mm", | ||
| )} | ||
| </p> | ||
| </div> | ||
| </div> | ||
|
|
||
| {selectedTask.comments && ( | ||
| <div> | ||
| <p className="text-sm text-gray-500">Comments</p> | ||
| <p className="mt-1 text-gray-600">{selectedTask.comments}</p> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </Modal.Body> | ||
| <Modal.Footer> | ||
| <Button onClick={updateStatus}>Update Status</Button> | ||
| <Button color="gray" onClick={() => setShowModal(false)}> | ||
| Close | ||
| </Button> | ||
| </Modal.Footer> | ||
| </Modal> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to modal
The modal component should have proper accessibility attributes for better user experience.
{selectedTask && (
<Modal
show={showModal}
onClose={() => setShowModal(false)}
size="xl"
+ aria-labelledby="task-modal-title"
+ aria-describedby="task-modal-description"
>
<Modal.Header>
- Task Details
+ <h2 id="task-modal-title">Task Details</h2>
</Modal.Header>
<Modal.Body>
- <div className="space-y-4">
+ <div id="task-modal-description" className="space-y-4">
{/* ... modal content ... */}
</div>
</Modal.Body>
<Modal.Footer>
- <Button onClick={updateStatus}>Update Status</Button>
+ <Button onClick={updateStatus} aria-label="Update task status">
+ Update Status
+ </Button>
- <Button color="gray" onClick={() => setShowModal(false)}>
+ <Button
+ color="gray"
+ onClick={() => setShowModal(false)}
+ aria-label="Close modal"
+ >
Close
</Button>
</Modal.Footer>
</Modal>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {selectedTask && ( | |
| <Modal show={showModal} onClose={() => setShowModal(false)} size="xl"> | |
| <Modal.Header>Task Details</Modal.Header> | |
| <Modal.Body> | |
| <div className="space-y-4"> | |
| <div> | |
| <h3 className="mb-2 text-xl font-semibold"> | |
| {selectedTask.title} | |
| </h3> | |
| <p className="text-gray-600">{selectedTask.description}</p> | |
| </div> | |
| <div className="grid grid-cols-2 gap-4"> | |
| <div> | |
| <p className="text-sm text-gray-500">Status</p> | |
| <Select | |
| value={newStatus} | |
| onChange={handleStatusChange} | |
| className="mt-1" | |
| > | |
| <option value="pending">Pending</option> | |
| <option value="in progress">In Progress</option> | |
| <option value="completed">Completed</option> | |
| </Select> | |
| </div> | |
| <div> | |
| <p className="text-sm text-gray-500">Priority</p> | |
| <p | |
| className={`mt-1 w-fit rounded-full px-2 py-1 text-sm font-medium ${getPriorityColor(selectedTask.priority)}`} | |
| > | |
| {selectedTask.priority} | |
| </p> | |
| </div> | |
| </div> | |
| <div className="grid grid-cols-2 gap-4"> | |
| <div> | |
| <p className="text-sm text-gray-500">Assigned To</p> | |
| <p className="mt-1">{selectedTask.assignee_name}</p> | |
| </div> | |
| <div> | |
| <p className="text-sm text-gray-500">Created By</p> | |
| <p className="mt-1">{selectedTask.creator_name}</p> | |
| </div> | |
| </div> | |
| <div className="grid grid-cols-2 gap-4"> | |
| <div> | |
| <p className="text-sm text-gray-500">Due Date</p> | |
| <p className="mt-1"> | |
| {format(new Date(selectedTask.due_date), "MMMM dd, yyyy")} | |
| </p> | |
| </div> | |
| <div> | |
| <p className="text-sm text-gray-500">Created At</p> | |
| <p className="mt-1"> | |
| {format( | |
| new Date(selectedTask.created_at), | |
| "MMM dd, yyyy HH:mm", | |
| )} | |
| </p> | |
| </div> | |
| </div> | |
| {selectedTask.comments && ( | |
| <div> | |
| <p className="text-sm text-gray-500">Comments</p> | |
| <p className="mt-1 text-gray-600">{selectedTask.comments}</p> | |
| </div> | |
| )} | |
| </div> | |
| </Modal.Body> | |
| <Modal.Footer> | |
| <Button onClick={updateStatus}>Update Status</Button> | |
| <Button color="gray" onClick={() => setShowModal(false)}> | |
| Close | |
| </Button> | |
| </Modal.Footer> | |
| </Modal> | |
| )} | |
| {selectedTask && ( | |
| <Modal | |
| show={showModal} | |
| onClose={() => setShowModal(false)} | |
| size="xl" | |
| aria-labelledby="task-modal-title" | |
| aria-describedby="task-modal-description" | |
| > | |
| <Modal.Header> | |
| <h2 id="task-modal-title">Task Details</h2> | |
| </Modal.Header> | |
| <Modal.Body> | |
| <div id="task-modal-description" className="space-y-4"> | |
| <div> | |
| <h3 className="mb-2 text-xl font-semibold"> | |
| {selectedTask.title} | |
| </h3> | |
| <p className="text-gray-600">{selectedTask.description}</p> | |
| </div> | |
| <div className="grid grid-cols-2 gap-4"> | |
| <div> | |
| <p className="text-sm text-gray-500">Status</p> | |
| <Select | |
| value={newStatus} | |
| onChange={handleStatusChange} | |
| className="mt-1" | |
| > | |
| <option value="pending">Pending</option> | |
| <option value="in progress">In Progress</option> | |
| <option value="completed">Completed</option> | |
| </Select> | |
| </div> | |
| <div> | |
| <p className="text-sm text-gray-500">Priority</p> | |
| <p | |
| className={`mt-1 w-fit rounded-full px-2 py-1 text-sm font-medium ${getPriorityColor(selectedTask.priority)}`} | |
| > | |
| {selectedTask.priority} | |
| </p> | |
| </div> | |
| </div> | |
| <div className="grid grid-cols-2 gap-4"> | |
| <div> | |
| <p className="text-sm text-gray-500">Assigned To</p> | |
| <p className="mt-1">{selectedTask.assignee_name}</p> | |
| </div> | |
| <div> | |
| <p className="text-sm text-gray-500">Created By</p> | |
| <p className="mt-1">{selectedTask.creator_name}</p> | |
| </div> | |
| </div> | |
| <div className="grid grid-cols-2 gap-4"> | |
| <div> | |
| <p className="text-sm text-gray-500">Due Date</p> | |
| <p className="mt-1"> | |
| {format(new Date(selectedTask.due_date), "MMMM dd, yyyy")} | |
| </p> | |
| </div> | |
| <div> | |
| <p className="text-sm text-gray-500">Created At</p> | |
| <p className="mt-1"> | |
| {format( | |
| new Date(selectedTask.created_at), | |
| "MMM dd, yyyy HH:mm", | |
| )} | |
| </p> | |
| </div> | |
| </div> | |
| {selectedTask.comments && ( | |
| <div> | |
| <p className="text-sm text-gray-500">Comments</p> | |
| <p className="mt-1 text-gray-600">{selectedTask.comments}</p> | |
| </div> | |
| )} | |
| </div> | |
| </Modal.Body> | |
| <Modal.Footer> | |
| <Button onClick={updateStatus} aria-label="Update task status"> | |
| Update Status | |
| </Button> | |
| <Button | |
| color="gray" | |
| onClick={() => setShowModal(false)} | |
| aria-label="Close modal" | |
| > | |
| Close | |
| </Button> | |
| </Modal.Footer> | |
| </Modal> | |
| )} |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
nextjs-task-system/app/admin/layout.tsx (1)
122-123: Enhance Sidebar active state handling for nested routesCurrently, the
activeprop in theSidebar.Itemcomponents uses strict equality to determine the active state. This may not highlight the menu item correctly when on nested routes like/admin/tasks/123.Apply this diff to improve active state handling:
- active={pathname === "/admin"} + active={pathname.startsWith("/admin")}Similarly, update the other menu items:
- active={pathname === "/admin/tasks"} + active={pathname.startsWith("/admin/tasks")}- active={pathname === "/admin/create"} + active={pathname.startsWith("/admin/create")}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
nextjs-task-system/app/admin/layout.tsx(1 hunks)nextjs-task-system/app/api/tasks/route.ts(1 hunks)nextjs-task-system/app/layout.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nextjs-task-system/app/layout.tsx
🔇 Additional comments (1)
nextjs-task-system/app/admin/layout.tsx (1)
59-60: Add user feedback on logout failure
If the logout request fails, the error is logged to the console, but the user is not notified. Consider displaying an error message to inform the user of the issue.
Apply this diff to update the handleLogout function:
if (response.ok) {
router.push("/login");
} else {
console.error("Logout request failed with status:", response.status);
+ // Optionally display an error message to the user
+ alert("Logout failed. Please try again.");
}| console.error(error); | ||
| return NextResponse.json( | ||
| { error: "Task not found for deletion" }, | ||
| { status: 500 }, | ||
| ); |
There was a problem hiding this comment.
Correct the error message and status code in the DELETE method's catch block
In the DELETE method's catch block, the error message "Task not found for deletion" with a 500 status code may confuse clients. Update the error message to better match the status code.
Apply this diff to correct the error handling:
} catch (error) {
console.error(error);
- return NextResponse.json(
- { error: "Task not found for deletion" },
- { status: 500 },
- );
+ return NextResponse.json(
+ { error: "Failed to delete task" },
+ { status: 500 },
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error(error); | |
| return NextResponse.json( | |
| { error: "Task not found for deletion" }, | |
| { status: 500 }, | |
| ); | |
| console.error(error); | |
| return NextResponse.json( | |
| { error: "Failed to delete task" }, | |
| { status: 500 }, | |
| ); |
| export async function DELETE(req: Request) { | ||
| try { | ||
| const { searchParams } = new URL(req.url); | ||
| const id = searchParams.get("id"); | ||
|
|
||
| if (!id) { | ||
| return NextResponse.json( | ||
| { error: "Task ID is required" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| const statement = db.prepare("DELETE FROM tasks WHERE id = ?"); | ||
| const result = statement.run(id); | ||
|
|
||
| if (result.changes === 0) { | ||
| return NextResponse.json( | ||
| { error: "Task not found for deletion" }, | ||
| { status: 404 }, | ||
| ); | ||
| } | ||
|
|
||
| return NextResponse.json({ success: true }); | ||
| } catch (error) { | ||
| console.error(error); | ||
| return NextResponse.json( | ||
| { error: "Task not found for deletion" }, | ||
| { status: 500 }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add authentication and authorization checks to the DELETE method
The DELETE method should verify that the user is authenticated and authorized to delete tasks.
Insert the following code at the beginning of the DELETE method:
+ const currentUser = await getCurrentUserServer();
+
+ if (!currentUser) {
+ return NextResponse.json({ error: "Not authenticated" }, { status: 401 });
+ }
+
+ if (currentUser.role !== "admin") {
+ return NextResponse.json({ error: "Unauthorized" }, { status: 403 });
+ }Committable suggestion skipped: line range outside the PR's diff.
| console.error(error); | ||
| return NextResponse.json({ error: "Task ID is required" }, { status: 500 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Correct the error message and status code in the PUT method's catch block
In the PUT method's catch block, the error message "Task ID is required" is misleading, and the status code 500 suggests a server error. Update the error message to accurately reflect the issue.
Apply this diff to correct the error handling:
} catch (error) {
console.error(error);
- return NextResponse.json({ error: "Task ID is required" }, { status: 500 });
+ return NextResponse.json({ error: "Failed to update task" }, { status: 500 });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error(error); | |
| return NextResponse.json({ error: "Task ID is required" }, { status: 500 }); | |
| } | |
| } | |
| console.error(error); | |
| return NextResponse.json({ error: "Failed to update task" }, { status: 500 }); | |
| } | |
| } |
| export async function POST(request: Request) { | ||
| try { | ||
| const body = await request.json(); | ||
| const { | ||
| title, | ||
| description, | ||
| user_id, | ||
| assigned_to, | ||
| due_date, | ||
| priority, | ||
| status, | ||
| comments, | ||
| } = body; | ||
|
|
||
| if (!user_id) { | ||
| return NextResponse.json( | ||
| { error: "User ID is required" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| const result = db | ||
| .prepare( | ||
| ` | ||
| INSERT INTO tasks ( | ||
| title, description, user_id, assigned_to, | ||
| due_date, priority, status, comments | ||
| ) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?) | ||
| `, | ||
| ) | ||
| .run([ | ||
| title, | ||
| description, | ||
| user_id, | ||
| assigned_to, | ||
| due_date, | ||
| priority, | ||
| status, | ||
| comments, | ||
| ]); | ||
|
|
||
| // Fetch the newly created task with user details | ||
| const newTask = db | ||
| .prepare("SELECT * FROM tasks_with_users WHERE id = ?") | ||
| .get(result.lastInsertRowid); | ||
|
|
||
| return NextResponse.json(newTask); | ||
| } catch (error) { | ||
| console.error("Error creating task:", error); | ||
| return NextResponse.json( | ||
| { error: "Failed to create task" }, | ||
| { status: 500 }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add authentication and authorization checks to the POST method
The POST method lacks authentication checks. This could allow unauthorized users to create tasks. Ensure that only authenticated admin users can perform this action.
Add the following authentication and authorization checks at the beginning of the POST method:
+ const currentUser = await getCurrentUserServer();
+
+ if (!currentUser) {
+ return NextResponse.json({ error: "Not authenticated" }, { status: 401 });
+ }
+
+ if (currentUser.role !== "admin") {
+ return NextResponse.json({ error: "Unauthorized" }, { status: 403 });
+ }Also, modify the code to use the current user's ID:
- const { title, description, user_id, assigned_to, due_date, priority, status, comments } = body;
+ const { title, description, assigned_to, due_date, priority, status, comments } = body;
- if (!user_id) {
- return NextResponse.json(
- { error: "User ID is required" },
- { status: 400 },
- );
- }
// ...
const result = db
.prepare(
`
INSERT INTO tasks (
- title, description, user_id, assigned_to,
+ title, description, user_id, assigned_to,
due_date, priority, status, comments
)
VALUES (?, ?, ?, ?, ?, ?, ?, ?)
`,
)
.run([
title,
description,
- user_id,
+ currentUser.id,
assigned_to,
due_date,
priority,
status,
comments,
]);Committable suggestion skipped: line range outside the PR's diff.
| export async function PUT(req: Request) { | ||
| try { | ||
| const { | ||
| id, | ||
| title, | ||
| description, | ||
| assigned_to, | ||
| due_date, | ||
| priority, | ||
| status, | ||
| comments, | ||
| } = await req.json(); | ||
|
|
||
| if (!id) { | ||
| return NextResponse.json( | ||
| { error: "Task ID is required" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| const statement = db.prepare(` | ||
| UPDATE tasks | ||
| SET title = ?, description = ?, assigned_to = ?, due_date = ?, priority = ?, status = ?, comments = ?, updated_at = CURRENT_TIMESTAMP | ||
| WHERE id = ? | ||
| `); | ||
| const result = statement.run( | ||
| title, | ||
| description, | ||
| assigned_to, | ||
| due_date, | ||
| priority, | ||
| status, | ||
| comments, | ||
| id, | ||
| ); | ||
|
|
||
| if (result.changes === 0) { | ||
| return NextResponse.json( | ||
| { error: "Task not found for update" }, | ||
| { status: 404 }, | ||
| ); | ||
| } | ||
|
|
||
| return NextResponse.json({ success: true }); | ||
| } catch (error) { | ||
| console.error(error); | ||
| return NextResponse.json({ error: "Task ID is required" }, { status: 500 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add authentication and authorization checks to the PUT method
The PUT method lacks authentication checks, potentially allowing unauthorized updates. Ensure that only authenticated admin users can update tasks.
Add the following checks at the beginning of the PUT method:
+ const currentUser = await getCurrentUserServer();
+
+ if (!currentUser) {
+ return NextResponse.json({ error: "Not authenticated" }, { status: 401 });
+ }
+
+ if (currentUser.role !== "admin") {
+ return NextResponse.json({ error: "Unauthorized" }, { status: 403 });
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
nextjs-task-system/components/TaskComponents/TaskCard.tsx (2)
18-42: Consider moving color utilities to a shared file.Since these color utility functions are exported and likely used across different components (as mentioned in the AI summary), consider moving them to a shared utilities file (e.g.,
utils/colors.ts) to promote reusability and maintain consistency.
65-154: Consider improving component structure and accessibility.While the implementation works, consider these improvements:
- Break down the large JSX structure into smaller components
- Add ARIA labels for better accessibility
- Add keyboard navigation support for the modal
<Card key={task.id} className="relative"> - <div className="mb-2 flex items-start justify-between"> + <div className="mb-2 flex items-start justify-between" role="heading" aria-level={2}> <h5 className="text-xl font-bold tracking-tight text-gray-900 dark:text-white"> {task.title} </h5> <span className={`rounded-full px-2 py-1 text-xs font-medium ${getPriorityColor( task.priority, )}`} + role="status" + aria-label={`Priority: ${task.priority}`} > {task.priority} </span> </div>nextjs-task-system/app/admin/layout.tsx (1)
16-22: Consider importing theUserinterface to avoid duplicationThe
Userinterface is defined in this file but also exists innextjs-task-system/Utils/auth.ts. To promote reusability and maintain consistency across the codebase, consider importing theUserinterface from a shared location instead of redefining it.nextjs-task-system/app/admin/tasks/[id]/page.tsx (1)
309-333: Generalize the Error Modal to handle different error scenariosThe error modal currently has a hardcoded title "Error updating task." To make it reusable for different error cases (e.g., updating, deleting), consider making the title dynamic based on the context.
Apply this diff to make the modal title dynamic:
<Modal show={showErrorModal} size="md" onClose={() => setShowErrorModal(false)} popup > <Modal.Header /> <Modal.Body> <div className="text-center"> <HiX className="mx-auto mb-4 h-14 w-14 text-red-500" /> <h3 className="mb-5 text-lg font-normal text-gray-500 dark:text-gray-400"> - Error updating task + {errorMessageTitle} </h3> <p className="text-sm text-gray-500 dark:text-gray-400"> {errorMessage} </p> <div className="mt-4 flex justify-center"> <Button color="gray" onClick={() => setShowErrorModal(false)}> Close </Button> </div> </div> </Modal.Body> </Modal>Ensure that the
errorMessageTitlestate is managed appropriately in your component:const [errorMessage, setErrorMessage] = useState(""); +const [errorMessageTitle, setErrorMessageTitle] = useState("");Set the
errorMessageTitlein your error handling code. For example, in theconfirmUpdatefunction:} catch (error) { setShowUpdateModal(false); setErrorMessage( error instanceof Error ? error.message : "An error occurred" ); + setErrorMessageTitle("Error updating task"); setShowErrorModal(true); }And in the
handleDeletefunction:} catch (error) { console.error("Error deleting task:", error); setErrorMessage( error instanceof Error ? error.message : "An error occurred" ); + setErrorMessageTitle("Error deleting task"); setShowErrorModal(true); }nextjs-task-system/app/user/tasks/page.tsx (2)
127-136: RefactorgetStatusColorto use an object mappingUsing an object mapping improves readability and maintainability.
Apply this diff:
- const getStatusColor = (status: string) => { - switch (status.toLowerCase()) { - case "completed": - return "bg-green-100 text-green-800"; - case "in progress": - return "bg-blue-100 text-blue-800"; - default: - return "bg-yellow-100 text-yellow-800"; - } - }; + const statusColors: { [key: string]: string } = { + 'completed': 'bg-green-100 text-green-800', + 'in progress': 'bg-blue-100 text-blue-800', + 'pending': 'bg-yellow-100 text-yellow-800', + }; + const getStatusColor = (status: string) => + statusColors[status.toLowerCase()] || 'bg-gray-100 text-gray-800';
138-149: RefactorgetPriorityColorto use an object mappingUsing an object mapping improves readability and maintainability.
Apply this diff:
- const getPriorityColor = (priority: string) => { - switch (priority.toLowerCase()) { - case "high": - return "bg-red-100 text-red-800"; - case "medium": - return "bg-orange-100 text-orange-800"; - case "low": - return "bg-green-100 text-green-800"; - default: - return "bg-gray-100 text-gray-800"; - } - }; + const priorityColors: { [key: string]: string } = { + 'high': 'bg-red-100 text-red-800', + 'medium': 'bg-orange-100 text-orange-800', + 'low': 'bg-green-100 text-green-800', + }; + const getPriorityColor = (priority: string) => + priorityColors[priority.toLowerCase()] || 'bg-gray-100 text-gray-800';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
nextjs-task-system/app/admin/layout.tsx(1 hunks)nextjs-task-system/app/admin/tasks/[id]/page.tsx(1 hunks)nextjs-task-system/app/api/tasks/[id]/route.ts(1 hunks)nextjs-task-system/app/user/tasks/page.tsx(1 hunks)nextjs-task-system/components/TaskComponents/TaskCard.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nextjs-task-system/app/api/tasks/[id]/route.ts
🔇 Additional comments (10)
nextjs-task-system/components/TaskComponents/TaskCard.tsx (4)
1-16: LGTM! Well-structured interfaces and imports.
The interfaces are well-defined with proper TypeScript types, and imports are appropriately organized.
44-46: LGTM! Clean component setup.
The component is well-typed and uses appropriate state management.
47-63: 🛠️ Refactor suggestion
Enhance error handling and add loading state.
The current implementation could be improved by:
- Adding proper response parsing
- Including a loading state during deletion
- Providing more specific error messages
const TaskCard: React.FC<TaskCardProps> = ({ task, onEdit, onDelete }) => {
const [showDeleteModal, setShowDeleteModal] = useState(false);
+ const [isDeleting, setIsDeleting] = useState(false);
+ const [error, setError] = useState<string | null>(null);
const handleDelete = async () => {
+ setIsDeleting(true);
+ setError(null);
try {
const response = await fetch(`/api/tasks/${task.id}`, {
method: "DELETE",
});
+ const data = await response.json();
if (!response.ok) {
- throw new Error("Failed to delete task");
+ throw new Error(data.message || "Failed to delete task");
}
setShowDeleteModal(false);
onDelete(task);
} catch (error) {
console.error("Error deleting task:", error);
- alert("Error deleting task");
+ setError(error instanceof Error ? error.message : "Failed to delete task");
+ } finally {
+ setIsDeleting(false);
}
};49-49: Verify API endpoint structure.
Let's ensure the API endpoint structure is consistent across the application.
✅ Verification successful
API endpoint structure is consistent with Next.js API routes
The API endpoint structure /api/tasks/${task.id} is correctly implemented and matches the Next.js API route handlers found in the codebase. The following evidence confirms this:
- The endpoint
/api/tasks/${task.id}corresponds to the route handler atnextjs-task-system/app/api/tasks/[id]/route.ts - The route handler implements all necessary HTTP methods (GET, PUT, DELETE)
- Other task-related endpoints follow the same pattern:
/api/tasks/user/[id]for user-specific tasks/api/tasksfor general task operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent API endpoint structure
# Search for API endpoint patterns
echo "Checking API endpoint patterns..."
rg -g '*.{ts,tsx,js,jsx}' '/api/tasks/[^"'\'']*' --no-filename
# Search for route handlers
echo "Checking route handlers..."
fd -e ts -e js 'route.ts' app/api/tasks
Length of output: 874
Script:
#!/bin/bash
# Let's check for API route handlers in the Next.js app directory structure
echo "Checking for API route handlers..."
fd -e ts -e js 'route' app
# Let's also check the entire codebase for any API-related files
echo "Checking for API-related files..."
fd -e ts -e js -e tsx 'api'
# Let's examine the content of any route handlers we find
echo "Examining route handler implementations..."
rg -g '*.{ts,tsx,js,jsx}' -A 5 'export async function' app
Length of output: 501
Script:
#!/bin/bash
# Let's first check the directory structure to understand the project layout
echo "Checking directory structure..."
ls -R
# Then let's look for any Next.js API routes or handlers across the project
echo "Searching for API route handlers..."
rg -g '*.{ts,tsx,js,jsx}' -A 5 'export.*function.*handler|export.*async function'
# Let's also check for any route configuration files
echo "Checking for route configuration..."
fd -e ts -e js -e tsx route
Length of output: 11422
nextjs-task-system/app/admin/tasks/[id]/page.tsx (1)
2-2:
Correct the usage of params and remove unnecessary import of use
- The
usefunction is not a standard export from React and is unnecessary here. - The
paramsinPagePropsshould not be aPromise. In Next.js,paramsare passed as a plain object to the page component.
Apply this diff to fix the issue:
-import { useState, useEffect, use } from "react";
+import { useState, useEffect } from "react";
interface PageProps {
- params: Promise<{
+ params: {
id: string;
- }>;
+ };
}
export default function EditTask({ params }: PageProps) {
- const resolvedParams = use(params);
- const taskId = resolvedParams.id;
+ const taskId = params.id;Also applies to: 21-25, 27-29
nextjs-task-system/app/user/tasks/page.tsx (5)
15-27: Maintain consistent naming conventions in the Task interface
The interface mixes snake_case and camelCase naming conventions. Consider using consistent camelCase for better TypeScript conventions.
Apply this diff to standardize the property names:
interface Task {
id: number;
title: string;
description: string;
status: string;
comments: string | null;
priority: string;
- due_date: string;
+ dueDate: string;
- creator_name: string;
+ creatorName: string;
- assignee_name: string;
+ assigneeName: string;
- created_at: string;
+ createdAt: string;
- updated_at: string;
+ updatedAt: string;
}48-61: Improve error handling in getCurrentUser
The error handling could be enhanced to provide better user feedback and distinguish between different types of errors.
Consider applying this diff:
const getCurrentUser = async () => {
try {
const response = await fetch("/api/auth/me");
if (response.ok) {
const userData = await response.json();
setCurrentUser(userData);
} else {
+ const errorData = await response.json();
+ if (response.status === 401) {
router.push("/login");
+ } else {
+ throw new Error(errorData.message || 'Failed to fetch user data');
+ }
}
} catch (error) {
- console.error("Error fetching current user:", error);
+ const errorMessage = error instanceof Error ? error.message : 'An unexpected error occurred';
+ // Consider using a toast notification system to notify the user
+ console.error(errorMessage);
router.push("/login");
}
};63-73: Enhance fetchTasks with loading states and error handling
Consider adding loading states and proper error handling for a better user experience.
Apply this diff:
+ const [isLoading, setIsLoading] = useState<boolean>(false);
const fetchTasks = async () => {
if (!currentUser) return;
+ setIsLoading(true);
try {
const response = await fetch(`/api/tasks/user/${currentUser.id}`);
+ if (!response.ok) {
+ const errorData = await response.json();
+ throw new Error(errorData.message || 'Failed to fetch tasks');
+ }
const data = await response.json();
setTasks(data);
} catch (error) {
- console.error("Error fetching tasks:", error);
+ const errorMessage = error instanceof Error ? error.message : 'An unexpected error occurred';
+ // Consider using a toast notification system to notify the user
+ console.error(errorMessage);
} finally {
+ setIsLoading(false);
}
};90-125: Enhance updateStatus with optimistic updates and validation
Consider implementing optimistic updates and status validation for better UX and data integrity.
Apply this diff:
+ const VALID_STATUSES = ['pending', 'in progress', 'completed'] as const;
+ type TaskStatus = typeof VALID_STATUSES[number];
const updateStatus = async () => {
if (!selectedTask) return;
+ if (!VALID_STATUSES.includes(newStatus as TaskStatus)) {
+ alert('Invalid status value');
+ return;
+ }
+ // Optimistic update
+ const previousTasks = [...tasks];
+ setTasks((prevTasks) =>
+ prevTasks.map((task) =>
+ task.id === selectedTask.id
+ ? { ...task, status: newStatus, comments: newComment }
+ : task
+ )
+ );
try {
const response = await fetch(`/api/tasks/${selectedTask.id}`, {
method: "PUT",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({
status: newStatus,
comments: newComment,
}),
});
if (response.ok) {
const updatedTask = await response.json();
setTasks((prevTasks) =>
prevTasks.map((task) =>
task.id === selectedTask.id
? { ...task, status: updatedTask.status, comments: updatedTask.comments }
: task
)
);
setShowModal(false);
setNewComment("");
} else {
const errorData = await response.json();
+ // Revert optimistic update on error
+ setTasks(previousTasks);
alert(`Error: ${errorData.error}`);
}
} catch (error) {
console.error("Error updating task:", error);
+ // Revert optimistic update on error
+ setTasks(previousTasks);
+ alert('Failed to update task. Please try again.');
}
};205-261: Add accessibility attributes to the modal
The modal component should have proper accessibility attributes for better user experience.
Consider applying this diff:
{selectedTask && (
<Modal
show={showModal}
onClose={() => setShowModal(false)}
size="xl"
+ aria-labelledby="task-modal-title"
+ aria-describedby="task-modal-description"
>
<Modal.Header>
- Edit Task
+ <h2 id="task-modal-title">Edit Task</h2>
</Modal.Header>
<Modal.Body>
- <div className="space-y-4">
+ <div id="task-modal-description" className="space-y-4">
{/* ... modal content ... */}
</div>
</Modal.Body>
<Modal.Footer>
- <Button onClick={updateStatus}>Update Task</Button>
+ <Button onClick={updateStatus} aria-label="Update task">
+ Update Task
+ </Button>
- <Button color="gray" onClick={() => setShowModal(false)}>
+ <Button
+ color="gray"
+ onClick={() => setShowModal(false)}
+ aria-label="Cancel"
+ >
Cancel
</Button>
</Modal.Footer>
</Modal>
)}| console.error("Error deleting task:", error); | ||
| alert("Error deleting task"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using alert for error messages
Using alert to display error messages can disrupt the user experience and is generally discouraged in modern web applications. Consider using the existing error modal to display the error message instead.
Apply this diff to replace the alert with the error modal:
try {
// ...
} catch (error) {
console.error("Error deleting task:", error);
- alert("Error deleting task");
+ setErrorMessage(
+ error instanceof Error ? error.message : "An error occurred"
+ );
+ setErrorMessageTitle("Error deleting task");
+ setShowErrorModal(true);
}Ensure that you have the errorMessageTitle state defined:
const [errorMessage, setErrorMessage] = useState("");
+const [errorMessageTitle, setErrorMessageTitle] = useState("");Committable suggestion skipped: line range outside the PR's diff.
| {tasks.length === 0 && ( | ||
| <div className="mt-8 text-center text-gray-500">No tasks found.</div> | ||
| )} |
There was a problem hiding this comment.
Remove redundant 'No tasks found' message
The check for tasks.length === 0 at line 152 already handles the case where no tasks are found. The additional check here is redundant and will never be reached.
Apply this diff to remove the redundant code:
- {tasks.length === 0 && (
- <div className="mt-8 text-center text-gray-500">No tasks found.</div>
- )}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
nextjs-task-system/app/admin/tasks/page.tsx (2)
42-54: Optimize filter performance with useMemo.The filterTasks function recreates the filtered array on every render. Using useMemo would optimize performance.
Replace the filterTasks function with a memoized version:
- const filterTasks = () => { - let filtered = tasks; - if (priorityFilter) { - filtered = filtered.filter((task) => task.priority === priorityFilter); - } - if (statusFilter) { - filtered = filtered.filter((task) => task.status === statusFilter); - } - setFilteredTasks(filtered); - }; + const filteredTasks = useMemo(() => { + let filtered = tasks; + if (priorityFilter) { + filtered = filtered.filter((task) => task.priority === priorityFilter); + } + if (statusFilter) { + filtered = filtered.filter((task) => task.status === statusFilter); + } + return filtered; + }, [tasks, priorityFilter, statusFilter]);
131-132: Translate comment to English and add aria-label.The code contains a Spanish comment and lacks accessibility attributes.
- // Mostrar skeletons mientras carga + // Show skeletons while loading Array.from({ length: 6 }).map((_, index) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
nextjs-task-system/app/admin/tasks/page.tsx(1 hunks)
🔇 Additional comments (1)
nextjs-task-system/app/admin/tasks/page.tsx (1)
1-9:
Verify the component import path and add authentication check.
The import path for TaskCard (@/components/TaskComponents/TaskCard) differs from the path mentioned in the AI summary (nextjs-task-system/componentes/AdminComponents/TaskCard.tsx).
Additionally, this admin page requires authentication but lacks the necessary auth check.
Add the following authentication check:
import { useRouter } from "next/navigation";
+import { getCurrentUser } from "@/Utils/auth";
+import { useEffect } from "react";
export default function TasksPage() {
+ const router = useRouter();
+
+ useEffect(() => {
+ const checkAuth = async () => {
+ const user = await getCurrentUser();
+ if (!user || user.role !== 'admin') {
+ router.push('/login');
+ }
+ };
+ checkAuth();
+ }, [router]);| export default function TasksPage() { | ||
| const [tasks, setTasks] = useState<Task[]>([]); | ||
| const [filteredTasks, setFilteredTasks] = useState<Task[]>([]); | ||
| const [priorityFilter, setPriorityFilter] = useState<string>(""); | ||
| const [statusFilter, setStatusFilter] = useState<string>(""); | ||
| const [showDeleteModal, setShowDeleteModal] = useState(false); | ||
| const [taskToDelete, setTaskToDelete] = useState<Task | null>(null); | ||
| const [isLoading, setIsLoading] = useState(true); | ||
| const router = useRouter(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error state management for better user experience.
While loading state is handled, there's no error state management for API failures. Users won't see any feedback when errors occur.
Add error state management:
export default function TasksPage() {
const [tasks, setTasks] = useState<Task[]>([]);
const [filteredTasks, setFilteredTasks] = useState<Task[]>([]);
const [priorityFilter, setPriorityFilter] = useState<string>("");
const [statusFilter, setStatusFilter] = useState<string>("");
const [showDeleteModal, setShowDeleteModal] = useState(false);
const [taskToDelete, setTaskToDelete] = useState<Task | null>(null);
const [isLoading, setIsLoading] = useState(true);
+ const [error, setError] = useState<string | null>(null);
const router = useRouter();Committable suggestion skipped: line range outside the PR's diff.
| const fetchTasks = async () => { | ||
| try { | ||
| setIsLoading(true); | ||
| const response = await fetch("/api/tasks"); | ||
| const data = await response.json(); | ||
| setTasks(data); | ||
| setFilteredTasks(data); | ||
| } catch (error) { | ||
| console.error("Error:", error); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in fetchTasks function.
The current error handling only logs to console. Users should be notified of API failures.
Update the error handling:
const fetchTasks = async () => {
try {
setIsLoading(true);
+ setError(null);
const response = await fetch("/api/tasks");
+ if (!response.ok) {
+ throw new Error(`Failed to fetch tasks: ${response.statusText}`);
+ }
const data = await response.json();
setTasks(data);
setFilteredTasks(data);
} catch (error) {
console.error("Error:", error);
+ setError(error instanceof Error ? error.message : 'Failed to fetch tasks');
} finally {
setIsLoading(false);
}
};Committable suggestion skipped: line range outside the PR's diff.
| const handleDeleteConfirm = async () => { | ||
| if (!taskToDelete) return; | ||
|
|
||
| try { | ||
| const response = await fetch(`/api/tasks/${taskToDelete.id}`, { | ||
| method: "DELETE", | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error("Failed to delete task"); | ||
| } | ||
|
|
||
| // Actualizar la lista de tareas | ||
| setTasks(tasks.filter((task) => task.id !== taskToDelete.id)); | ||
| setShowDeleteModal(false); | ||
| setTaskToDelete(null); | ||
| } catch (error) { | ||
| console.error("Error deleting task:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance delete operation with better error handling and optimistic updates.
The current implementation could be improved with proper error handling and optimistic updates for better UX.
Implement optimistic updates and better error handling:
const handleDeleteConfirm = async () => {
if (!taskToDelete) return;
+ // Optimistic update
+ const previousTasks = tasks;
+ setTasks(tasks.filter((task) => task.id !== taskToDelete.id));
+ setShowDeleteModal(false);
+ setTaskToDelete(null);
try {
const response = await fetch(`/api/tasks/${taskToDelete.id}`, {
method: "DELETE",
});
if (!response.ok) {
+ const errorData = await response.json();
+ throw new Error(errorData.message || 'Failed to delete task');
}
-
- // Actualizar la lista de tareas
- setTasks(tasks.filter((task) => task.id !== taskToDelete.id));
- setShowDeleteModal(false);
- setTaskToDelete(null);
} catch (error) {
console.error("Error deleting task:", error);
+ // Revert optimistic update
+ setTasks(previousTasks);
+ setError(error instanceof Error ? error.message : 'Failed to delete task');
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDeleteConfirm = async () => { | |
| if (!taskToDelete) return; | |
| try { | |
| const response = await fetch(`/api/tasks/${taskToDelete.id}`, { | |
| method: "DELETE", | |
| }); | |
| if (!response.ok) { | |
| throw new Error("Failed to delete task"); | |
| } | |
| // Actualizar la lista de tareas | |
| setTasks(tasks.filter((task) => task.id !== taskToDelete.id)); | |
| setShowDeleteModal(false); | |
| setTaskToDelete(null); | |
| } catch (error) { | |
| console.error("Error deleting task:", error); | |
| } | |
| }; | |
| const handleDeleteConfirm = async () => { | |
| if (!taskToDelete) return; | |
| // Optimistic update | |
| const previousTasks = tasks; | |
| setTasks(tasks.filter((task) => task.id !== taskToDelete.id)); | |
| setShowDeleteModal(false); | |
| setTaskToDelete(null); | |
| try { | |
| const response = await fetch(`/api/tasks/${taskToDelete.id}`, { | |
| method: "DELETE", | |
| }); | |
| if (!response.ok) { | |
| const errorData = await response.json(); | |
| throw new Error(errorData.message || 'Failed to delete task'); | |
| } | |
| } catch (error) { | |
| console.error("Error deleting task:", error); | |
| // Revert optimistic update | |
| setTasks(previousTasks); | |
| setError(error instanceof Error ? error.message : 'Failed to delete task'); | |
| } | |
| }; |
| return ( | ||
| <div className="container mx-auto p-4"> | ||
| <h1 className="mb-6 mt-12 text-3xl font-bold md:mt-0">Tasks List</h1> | ||
|
|
||
| {/* Filters and Create Button */} | ||
| <div className="mb-6 flex flex-col space-y-4 sm:flex-row sm:items-center sm:justify-between sm:space-x-4 sm:space-y-0"> | ||
| <div className="flex flex-1 space-x-4"> | ||
| <Select | ||
| id="priorityFilter" | ||
| name="priorityFilter" | ||
| value={priorityFilter} | ||
| onChange={(e) => setPriorityFilter(e.target.value)} | ||
| className="flex-1" | ||
| > | ||
| <option value="">All Priorities</option> | ||
| <option value="low">Low</option> | ||
| <option value="medium">Medium</option> | ||
| <option value="high">High</option> | ||
| </Select> | ||
|
|
||
| <Select | ||
| id="statusFilter" | ||
| name="statusFilter" | ||
| value={statusFilter} | ||
| onChange={(e) => setStatusFilter(e.target.value)} | ||
| className="flex-1" | ||
| > | ||
| <option value="">All Statuses</option> | ||
| <option value="pending">Pending</option> | ||
| <option value="in progress">In Progress</option> | ||
| <option value="completed">Completed</option> | ||
| </Select> | ||
| </div> | ||
|
|
||
| <Button | ||
| onClick={() => router.push("/admin/create")} | ||
| className="w-full sm:w-auto" | ||
| color="blue" | ||
| > | ||
| Create New Task | ||
| </Button> | ||
| </div> | ||
|
|
||
| <div className="grid grid-cols-1 gap-4 md:grid-cols-2 lg:grid-cols-3"> | ||
| {isLoading | ||
| ? // Mostrar skeletons mientras carga | ||
| Array.from({ length: 6 }).map((_, index) => ( | ||
| <TaskCardSkeleton key={index} /> | ||
| )) | ||
| : filteredTasks.map((task) => ( | ||
| <div key={task.id}> | ||
| <TaskCard | ||
| task={task} | ||
| onEdit={() => handleTaskClick(task, "edit")} | ||
| onDelete={() => handleTaskClick(task, "delete")} | ||
| /> | ||
| </div> | ||
| ))} | ||
| </div> | ||
|
|
||
| {/* Delete Confirmation Modal */} | ||
| <Modal | ||
| show={showDeleteModal} | ||
| onClose={() => setShowDeleteModal(false)} | ||
| popup | ||
| size="md" | ||
| > | ||
| <Modal.Header /> | ||
| <Modal.Body> | ||
| <div className="text-center"> | ||
| <HiOutlineExclamationCircle className="mx-auto mb-4 h-14 w-14 text-gray-400 dark:text-gray-200" /> | ||
| <h3 className="mb-5 text-lg font-normal text-gray-500 dark:text-gray-400"> | ||
| Are you sure you want to delete this task? | ||
| </h3> | ||
| <div className="flex justify-center gap-4"> | ||
| <Button color="failure" onClick={handleDeleteConfirm}> | ||
| Yes, delete | ||
| </Button> | ||
| <Button color="gray" onClick={() => setShowDeleteModal(false)}> | ||
| No, cancel | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| </Modal.Body> | ||
| </Modal> | ||
|
|
||
| {filteredTasks.length === 0 && ( | ||
| <div className="mt-8 text-center text-gray-500"> | ||
| No tasks found matching the selected filters. | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error state display and improve accessibility.
The UI needs to handle error states and improve accessibility for screen readers.
Add error state display and accessibility improvements:
return (
<div className="container mx-auto p-4">
<h1 className="mb-6 mt-12 text-3xl font-bold md:mt-0">Tasks List</h1>
+ {error && (
+ <div role="alert" className="mb-4 rounded-lg bg-red-100 p-4 text-red-700">
+ {error}
+ </div>
+ )}
{/* Filters and Create Button */}
<div className="mb-6 flex flex-col space-y-4 sm:flex-row sm:items-center sm:justify-between sm:space-x-4 sm:space-y-0">
<div className="flex flex-1 space-x-4">
<Select
id="priorityFilter"
name="priorityFilter"
value={priorityFilter}
onChange={(e) => setPriorityFilter(e.target.value)}
className="flex-1"
+ aria-label="Filter tasks by priority"
>
<Select
id="statusFilter"
name="statusFilter"
value={statusFilter}
onChange={(e) => setStatusFilter(e.target.value)}
className="flex-1"
+ aria-label="Filter tasks by status"
>
<Button
onClick={() => router.push("/admin/create")}
className="w-full sm:w-auto"
color="blue"
+ aria-label="Create new task"
>Committable suggestion skipped: line range outside the PR's diff.
korutx
left a comment
korutx
left a comment
There was a problem hiding this comment.
Final Review - Breyeszx
Strengths 🌟
Technical Implementation
-
Excellent Backend Architecture
- Very good use of NextJS Backend Route API
- Well-structured API endpoints
- Clean separation of concerns
-
Strong Security Implementation
- Robust session management with UUID
- Proper cookie security settings (httpOnly, secure, sameSite)
- Good password handling with bcrypt
- Database-backed session validation with expiration
-
Frontend Development
- Very good implementation of Tailwind CSS
- Clean and responsive UI using Flowbite components
- Mobile-friendly layouts
- Intuitive user interface
Completed Features
- ✅ Comprehensive user role system (Admin/Regular)
- ✅ Task management functionality
- ✅ Comments and status tracking
- ✅ Basic filtering capabilities
- ✅ Responsive design
- ✅ Database integration with SQLite
Areas for Improvement 🎯
Missing Features
- Group tasks functionality not implemented
- Sorting capabilities pending (due date, priority, creation date)
Technical Considerations
- No Server-Side Rendering (SSR) implementation
- Could improve initial page load performance
- Better SEO potential with SSR
- Consider implementing for future iterations
Overall Assessment 🏆
This is a solid implementation of a task management system that demonstrates strong understanding of:
- Modern web development practices
- Security considerations
- UI/UX design principles
- Database integration
The core functionality is well-implemented, and while some features are missing, the existing codebase provides a strong foundation for future enhancements.
Recommendations for Next Steps
- Implement missing group tasks functionality
- Add sorting capabilities
- Consider implementing SSR for performance optimization
- Add session cleanup routine for expired sessions
- Consider adding rate limiting for login attempts
Conclusion
Congratulations on delivering a well-structured and secure task management system! The attention to security details and clean implementation of the core features is particularly commendable. While there's room for some additional features, the current implementation provides a solid foundation for future development.
Keep up the excellent work! 🎉
| ); | ||
| } | ||
|
|
||
| const sessionId = crypto.randomUUID(); |
There was a problem hiding this comment.
👍 Good: Using UUID for session ID provides strong randomness and uniqueness
| const sessionId = crypto.randomUUID(); | ||
| db.prepare( | ||
| "INSERT INTO sessions (id, user_id, created_at) VALUES (?, ?, datetime('now'))", | ||
| ).run(sessionId, user.id); |
There was a problem hiding this comment.
👍 Good: Storing session with timestamp in database
| response.cookies.set("sessionId", sessionId, { | ||
| httpOnly: true, | ||
| secure: process.env.NODE_ENV === "production", | ||
| sameSite: "lax", | ||
| path: "/", | ||
| }); |
There was a problem hiding this comment.
👍 Excellent cookie security settings:
response.cookies.set("sessionId", sessionId, {
httpOnly: true, // Prevents JavaScript access
secure: process.env.NODE_ENV === "production", // Forces HTTPS in production
sameSite: "lax", // Protects against CSRF
path: "/", // Proper scope setting
});
There was a problem hiding this comment.
Potential improvement: Consider adding maxAge or expires option
to match your 24-hour session expiration logic in the GET route
| return NextResponse.json({ error: "No session found" }, { status: 401 }); | ||
| } | ||
|
|
||
| // Obtener usuario de la sesión |
There was a problem hiding this comment.
👍 Good: Session validation includes timestamp check
There was a problem hiding this comment.
Key positive points to highlight:
- Strong security practices with cookie settings
- Use of UUID for session IDs
- Database-backed sessions with timestamps
- Proper session validation including expiration checks
Suggestions for improvement:
- Consider adding cookie expiration to match database session expiration
- Could add session cleanup routine for expired sessions
- Might want to add session invalidation on logout (if not already implemented elsewhere)
- Consider adding rate limiting for login attempts
Overall, this is a well-implemented authentication system following security best practices! 🎉
2 participants
Este pull request presenta el desarrollo completo del Task Management System, cumpliendo con todos los requisitos core y casos de uso solicitados. La implementación utiliza Next.js para el frontend y backend, y SQLite como base de datos.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Chores