Conversation
✅ Deploy Preview for incandescent-pastelito-427a60 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for beamish-phoenix-78fd32 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Pull request overview
This pull request adds an admin dashboard page with functionality to view and edit bike inventory, add new bikes, and view orders. The implementation includes Supabase integration for database operations and file storage for bike images.
Changes:
- Added admin page with authentication and authorization checks
- Implemented API endpoints for file upload and bike creation with Supabase integration
- Created inventory management interface with inline editing and delete capabilities
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| app/admin/page.tsx | Server component that renders the admin dashboard with authentication/authorization checks |
| app/api/upload/route.ts | API route handler for uploading images to Supabase storage |
| app/api/add-bike/route.ts | API route handler for creating new bike records in the database |
| app/admin/image.ts | Client-side utility function for handling image uploads and generating signed URLs |
| app/admin/Orders.tsx | Static component displaying hardcoded order data (placeholder implementation) |
| app/admin/Inventory.tsx | Client component for viewing and editing bike inventory with real-time Supabase updates |
| app/admin/AddProduct.tsx | Client component with form for adding new bikes to the inventory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { createClient } from "@/utils/supabase/client"; | ||
|
|
||
| const uploadImage = async (e: ChangeEvent<HTMLInputElement>) => { | ||
| const supabase = await createClient() |
There was a problem hiding this comment.
The createClient() function is marked as async and is being awaited, but in the client-side context (@/utils/supabase/client), the createClient function is typically synchronous. This await is unnecessary and may indicate confusion between the server and client Supabase client creation patterns. The server version is async, but the client version is not.
| const supabase = await createClient() | |
| const supabase = createClient() |
| const data = await response.json() | ||
|
|
||
| if (data.success) { | ||
| await new Promise(r => setTimeout(r, 1000)); |
There was a problem hiding this comment.
There's a hardcoded 1-second delay using setTimeout before fetching the signed URL. This appears to be a workaround for a race condition or timing issue, but it's not a reliable solution. The delay might not be sufficient in all cases (slow network, high server load), and it adds unnecessary latency in cases where it's not needed. Consider using a proper event-based approach or polling mechanism to ensure the file is available before requesting the signed URL.
| if (supabase.from('admin').select('id').eq('id', user.id).single() === null) { | ||
| alert('You are not authorized to view this page') |
There was a problem hiding this comment.
The authorization check on line 14 is not awaited and will never work correctly. The method supabase.from('admin').select('id').eq('id', user.id).single() returns a Promise, not the actual result. This means the comparison with null will always be false (comparing a Promise object to null), allowing any authenticated user to access the admin page regardless of whether they're in the admin table. This is a critical security vulnerability that bypasses authorization.
Additionally, the alert() function cannot be called in a server component - it's a browser API and will cause a runtime error on the server.
| if (supabase.from('admin').select('id').eq('id', user.id).single() === null) { | |
| alert('You are not authorized to view this page') | |
| const { data: adminRecord } = await supabase | |
| .from('admin') | |
| .select('id') | |
| .eq('id', user.id) | |
| .single(); | |
| if (!adminRecord) { |
| 'use server' | ||
|
|
There was a problem hiding this comment.
The 'use server' directive is misplaced at the top of a Route Handler file. Route Handlers are already server-side by default in Next.js, and 'use server' is intended for Server Actions (functions that can be called from client components), not for API routes. This directive should be removed as it serves no purpose here and may cause confusion.
| 'use server' |
| const arrayBuffer = await file.arrayBuffer(); | ||
| const buffer = Buffer.from(arrayBuffer); | ||
| const filename = `${Date.now()}-${file.name}`; |
There was a problem hiding this comment.
There's no validation of the uploaded file type, size, or content. This could allow malicious users to upload executable files, very large files that could exhaust storage, or files with malicious content. Consider adding validation for:
- File type (verify it's actually an image by checking magic bytes, not just the extension)
- File size (enforce a maximum size limit)
- Filename sanitization (the current filename uses Date.now() which is good, but also includes the original filename which could contain path traversal characters)
| const handleSave = async (id: number) => { | ||
| await supabase.from("bikes").upsert(inventoryData).eq("bike_id", id) | ||
| console.log(inventoryData) | ||
| setEditingId(null) | ||
| } | ||
|
|
||
| const handleDelete = async (id: number) => { | ||
| await supabase.from("bikes").delete().eq("bike_id", id) | ||
| setInventoryData(inventoryData.filter((item) => item.bike_id !== id)) | ||
| } |
There was a problem hiding this comment.
There's no error handling for the database operations in handleSave and handleDelete. If the database operation fails, the user won't be notified, but the local state will still be updated (in the case of handleDelete) or the editing state will be cleared (in handleSave). This can lead to inconsistency between the UI and database state. Add error handling and show appropriate feedback to the user.
| 'use server' | ||
|
|
There was a problem hiding this comment.
The 'use server' directive is misplaced at the top of a Route Handler file. Route Handlers are already server-side by default in Next.js, and 'use server' is intended for Server Actions (functions that can be called from client components), not for API routes. This directive should be removed as it serves no purpose here and may cause confusion.
| 'use server' |
| const bike = await req.json() as Bike; | ||
|
|
There was a problem hiding this comment.
There's no authentication or authorization check in this API endpoint. Any unauthenticated user can add bikes to the database, which is a security vulnerability. You should verify that the user is authenticated and authorized (e.g., is an admin) before allowing bike creation.
| const bike = await req.json() as Bike; | |
| // Ensure the requester is authenticated | |
| const { | |
| data: { user }, | |
| error: authError, | |
| } = await supabase.auth.getUser(); | |
| if (authError || !user) { | |
| return NextResponse.json( | |
| { success: false, message: "Unauthorized" }, | |
| { status: 401 } | |
| ); | |
| } | |
| // Optional: enforce that only admins can add bikes, based on user metadata | |
| const isAdmin = | |
| (user.app_metadata as any)?.is_admin === true || | |
| (user.user_metadata as any)?.is_admin === true; | |
| if (!isAdmin) { | |
| return NextResponse.json( | |
| { success: false, message: "Forbidden: insufficient privileges" }, | |
| { status: 403 } | |
| ); | |
| } | |
| const bike = (await req.json()) as Bike; |
|
|
||
| return NextResponse.json({ success: true, data }, { status: 200 }); | ||
| } catch (error) { | ||
| return NextResponse.json({ success: false, message: error}, { status: 500 }); |
There was a problem hiding this comment.
The error object is being passed directly to the response JSON. This could expose sensitive error details, stack traces, or internal implementation details to the client. Instead, log the full error server-side and return a generic error message to the client, or sanitize the error object before sending.
| return NextResponse.json({ success: false, message: error}, { status: 500 }); | |
| console.error("Error adding bike:", error); | |
| return NextResponse.json({ success: false, message: "Internal server error" }, { status: 500 }); |
| if (data.success) { | ||
| await new Promise(r => setTimeout(r, 1000)); | ||
| console.log('Data:', data) | ||
| const { data: result, error } = await supabase.storage.from('Images').createSignedUrl(data.data.path, 315569520); |
There was a problem hiding this comment.
The hardcoded signed URL expiration time of 315569520 seconds (approximately 10 years) is extremely long and could pose a security risk. Signed URLs should typically have much shorter expiration times (hours to days) to limit the window of exposure if a URL is compromised. Consider using a more reasonable expiration time like 3600 seconds (1 hour) or 86400 seconds (1 day).
There was a problem hiding this comment.
need to make images public
Uh oh!
There was an error while loading. Please reload this page.