Completed appointment booking system task#8
Open
DHIRAVIYASUNDARAM wants to merge 1 commit intobotcode-com:mainfrom
Open
Completed appointment booking system task#8DHIRAVIYASUNDARAM wants to merge 1 commit intobotcode-com:mainfrom
DHIRAVIYASUNDARAM wants to merge 1 commit intobotcode-com:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an admin dashboard and login system, along with API endpoints for booking, cancelling, and listing appointments. Several critical security and architectural issues were identified during the review. Multiple API endpoints lack authentication and authorization, exposing sensitive data and allowing unauthorized actions. Furthermore, passwords are being compared in plain text, and the admin dashboard relies on insecure client-side authentication via localStorage. The booking logic also contains a race condition that could lead to overbooking, and the patient dashboard's use of a full page reload should be replaced with a more efficient state update or router refresh.
WalkthroughThis PR implements admin authentication and appointment management features. It adds an admin login system with localStorage-based session tracking, an admin dashboard to view appointments, and completes appointment booking and cancellation workflows with validation rules such as preventing duplicate bookings and enforcing cancellation timing constraints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as /api/appointments/book
participant Supabase as Database
Client->>API: POST { slotId, patientId, doctorId }
API->>Supabase: Query slot by slotId
alt Slot not found or already booked
API-->>Client: 400 { error }
else Slot available
API->>Supabase: Query active appointments for patient & doctor
alt Active appointment exists
API-->>Client: 400 { error }
else No conflict
API->>Supabase: Insert new appointment (status: active)
API->>Supabase: Update slot (is_booked: true)
API-->>Client: 200 { success }
end
end
sequenceDiagram
participant Client
participant API as /api/appointments/cancel
participant Supabase as Database
Client->>API: POST { appointmentId, action, patientId? }
API->>Supabase: Query appointment with related slot
alt Appointment not found or not active
API-->>Client: 404 or 400 { error }
else Valid appointment
alt action: cancel
alt patientId provided and within 1 hour
API-->>Client: 403 { error: "Cannot cancel within 1 hour" }
else Time constraint met or doctorId action
API->>Supabase: Update appointment (status: cancelled/done)
API->>Supabase: Update slot (is_booked: false)
API-->>Client: 200 { success }
end
else action: done
API->>Supabase: Update appointment (status: done)
API->>Supabase: Update slot (is_booked: false)
API-->>Client: 200 { success }
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/admin/dashboard/page.tsx`:
- Around line 34-44: In loadAppointments(), validate the fetch response and the
parsed JSON before calling setRows: first check res.ok and handle non-2xx
responses (e.g., setRows([]) and surface/log the error or handle 401 specially),
then ensure the parsed body is an array using Array.isArray(data) and only call
setRows(data) when it is; if it’s not an array, log the unexpected payload
(include status and body) and call setRows([]) to avoid later rows.map errors.
Ensure these checks occur in the try block before setRows and preserve the
existing setLoading(false) in finally.
In `@app/admin/login/page.tsx`:
- Around line 14-28: The submit path currently assumes fetch succeeds and the
response is JSON; wrap the network call and parsing in a try/catch around the
fetch(...) and res.json() so rejections or non-JSON bodies don't throw and leave
the user without feedback. Specifically, in the login submit handler that calls
fetch("/api/admin/login"), catch network errors and call setError(...) with a
friendly message, and when res.ok is false attempt to parse JSON in a guarded
way (fall back to res.text() if JSON.parse fails) and then call
setError(data.error || text || "Login failed"); keep the existing success
actions (localStorage.setItem("adminLoggedIn", "true") and window.location.href
= "/admin/dashboard") only when res.ok and parsing succeeds.
In `@app/api/admin/appointments/route.ts`:
- Around line 23-29: The code is turning DB/query failures into a 200 with an
empty array; update both error paths to return a 5xx response and include the
error information so the client can show an error state. Specifically, in the
branch that currently does "if (error) { return NextResponse.json([], { status:
200 }); }" and in the catch block that returns 200, change them to return an
appropriate server error response (e.g., NextResponse.json({ error: 'Internal
Server Error', details: String(error) }, { status: 500 })) so the API surfaces
failures instead of pretending there are no appointments; preserve usage of
NextResponse.json and include the caught/received error variable for
logging/response.
In `@app/api/appointments/book/route.ts`:
- Around line 20-24: The handler currently trusts the incoming doctorId instead
of using the slot owner; fetch the slot (the existing `slot` result from the
`slots` query) and derive the appointment's doctor as `const doctorId =
slot.doctor_id` (after checking `slot` exists), then use that derived `doctorId`
everywhere: in the duplicate-check query that looks for an active appointment
with the same doctor and patient, and in the `insert` payload for the new
appointment; ignore or remove the doctorId coming from the request body so
callers cannot substitute a different doctor. Ensure the same change is applied
to the other checks/inserts in the function (the duplicate-check block and the
appointment insert blocks referenced around lines 33-54 and 58-61).
In `@app/api/appointments/cancel/route.ts`:
- Around line 20-24: The handler uses SUPABASE_SERVICE_ROLE_KEY so RLS is
bypassed; before updating an appointment's status, fetch the appointment (you
already query into appt) and verify ownership: if cancelling, assert that the
provided patientId equals appt.patient_id; if marking done, assert provided
doctorId equals appt.doctor_id; return 403/appropriate error if the IDs don't
match or if appt is missing, then proceed to call
supabase.from("appointments").update(...) only after the ownership check passes.
Use the existing variables appointmentId, patientId, doctorId, appt, and the
"appointments" table/fields patient_id and doctor_id to locate where to add the
checks.
In `@app/patient/dashboard/page.tsx`:
- Around line 106-113: Remove the window.location.reload() call and instead
update the local React state that drives the dashboard: keep the setActionMsg
and setAvailableSlots logic, then either push the new appointment into the
appointments state (e.g., call setAppointments(prev => [newAppointment,
...prev])) or call the existing fetch/refetch helper (e.g., fetchAppointments()
or mutateAppointments()) to refresh only the appointment list; locate this
change near setActionMsg, setAvailableSlots and slotId in page.tsx and ensure no
full-page reload is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a27f12a-c221-440c-a3a4-281e79889173
📒 Files selected for processing (8)
app/admin/dashboard/page.tsxapp/admin/login/page.tsxapp/api/admin/appointments/route.tsapp/api/admin/login/route.tsapp/api/appointments/book/route.tsapp/api/appointments/cancel/route.tsapp/doctor/dashboard/page.tsxapp/patient/dashboard/page.tsx
Comment on lines
+34
to
+44
| async function loadAppointments() { | ||
| try { | ||
| const res = await fetch("/api/admin/appointments"); | ||
| const data = await res.json(); | ||
|
|
||
| setRows(data || []); | ||
| } catch (error) { | ||
| console.log(error); | ||
| } finally { | ||
| setLoading(false); | ||
| } |
There was a problem hiding this comment.
Validate the response before storing it in rows.
loadAppointments() assumes every JSON body is an array. Once the API starts returning proper 401/500 payloads, setRows(data || []) can store an object, and the render path later calls rows.map(...) on a non-array value.
Suggested fix
+ const [error, setError] = useState("");
+
async function loadAppointments() {
try {
const res = await fetch("/api/admin/appointments");
const data = await res.json();
-
- setRows(data || []);
+ if (!res.ok || !Array.isArray(data)) {
+ throw new Error("Failed to load appointments");
+ }
+ setRows(data);
} catch (error) {
- console.log(error);
+ setError("Failed to load appointments");
+ setRows([]);
} finally {
setLoading(false);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/admin/dashboard/page.tsx` around lines 34 - 44, In loadAppointments(),
validate the fetch response and the parsed JSON before calling setRows: first
check res.ok and handle non-2xx responses (e.g., setRows([]) and surface/log the
error or handle 401 specially), then ensure the parsed body is an array using
Array.isArray(data) and only call setRows(data) when it is; if it’s not an
array, log the unexpected payload (include status and body) and call setRows([])
to avoid later rows.map errors. Ensure these checks occur in the try block
before setRows and preserve the existing setLoading(false) in finally.
Comment on lines
+14
to
+28
| const res = await fetch("/api/admin/login", { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ email, password }), | ||
| }); | ||
|
|
||
| const data = await res.json(); | ||
|
|
||
| if (res.ok) { | ||
| localStorage.setItem("adminLoggedIn", "true"); | ||
| window.location.href = "/admin/dashboard"; | ||
| } else { | ||
| setError(data.error || "Login failed"); |
There was a problem hiding this comment.
Handle request and JSON parse failures in the submit path.
If fetch() rejects, or the server returns a non-JSON error body, this function throws before setError(...) runs. The login flow then fails with no feedback to the user.
Suggested fix
async function handleLogin(e: React.FormEvent) {
e.preventDefault();
setError("");
-
- const res = await fetch("/api/admin/login", {
- method: "POST",
- headers: {
- "Content-Type": "application/json",
- },
- body: JSON.stringify({ email, password }),
- });
-
- const data = await res.json();
-
- if (res.ok) {
- localStorage.setItem("adminLoggedIn", "true");
- window.location.href = "/admin/dashboard";
- } else {
- setError(data.error || "Login failed");
+ try {
+ const res = await fetch("/api/admin/login", {
+ method: "POST",
+ headers: {
+ "Content-Type": "application/json",
+ },
+ body: JSON.stringify({ email, password }),
+ });
+
+ const data = await res.json().catch(() => ({}));
+
+ if (res.ok) {
+ localStorage.setItem("adminLoggedIn", "true");
+ window.location.href = "/admin/dashboard";
+ } else {
+ setError(data.error || "Login failed");
+ }
+ } catch {
+ setError("Login failed");
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/admin/login/page.tsx` around lines 14 - 28, The submit path currently
assumes fetch succeeds and the response is JSON; wrap the network call and
parsing in a try/catch around the fetch(...) and res.json() so rejections or
non-JSON bodies don't throw and leave the user without feedback. Specifically,
in the login submit handler that calls fetch("/api/admin/login"), catch network
errors and call setError(...) with a friendly message, and when res.ok is false
attempt to parse JSON in a guarded way (fall back to res.text() if JSON.parse
fails) and then call setError(data.error || text || "Login failed"); keep the
existing success actions (localStorage.setItem("adminLoggedIn", "true") and
window.location.href = "/admin/dashboard") only when res.ok and parsing
succeeds.
Comment on lines
+23
to
+29
| if (error) { | ||
| return NextResponse.json([], { status: 200 }); | ||
| } | ||
|
|
||
| return NextResponse.json(data || []); | ||
| } catch (error) { | ||
| return NextResponse.json([], { status: 200 }); |
There was a problem hiding this comment.
Do not convert database failures into 200 [].
Both failure paths currently masquerade as an empty dashboard, so a broken query or missing env var becomes “No appointments found” instead of an actionable server error. Return a 5xx here and let the client render an error state.
Suggested fix
if (error) {
- return NextResponse.json([], { status: 200 });
+ return NextResponse.json(
+ { error: "Failed to load appointments" },
+ { status: 500 }
+ );
}
return NextResponse.json(data || []);
} catch (error) {
- return NextResponse.json([], { status: 200 });
+ return NextResponse.json(
+ { error: "Failed to load appointments" },
+ { 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.
Suggested change
| if (error) { | |
| return NextResponse.json([], { status: 200 }); | |
| } | |
| return NextResponse.json(data || []); | |
| } catch (error) { | |
| return NextResponse.json([], { status: 200 }); | |
| if (error) { | |
| return NextResponse.json( | |
| { error: "Failed to load appointments" }, | |
| { status: 500 } | |
| ); | |
| } | |
| return NextResponse.json(data || []); | |
| } catch (error) { | |
| return NextResponse.json( | |
| { error: "Failed to load appointments" }, | |
| { status: 500 } | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/admin/appointments/route.ts` around lines 23 - 29, The code is
turning DB/query failures into a 200 with an empty array; update both error
paths to return a 5xx response and include the error information so the client
can show an error state. Specifically, in the branch that currently does "if
(error) { return NextResponse.json([], { status: 200 }); }" and in the catch
block that returns 200, change them to return an appropriate server error
response (e.g., NextResponse.json({ error: 'Internal Server Error', details:
String(error) }, { status: 500 })) so the API surfaces failures instead of
pretending there are no appointments; preserve usage of NextResponse.json and
include the caught/received error variable for logging/response.
Comment on lines
+20
to
+24
| const { data: slot } = await supabase | ||
| .from("slots") | ||
| .select("*") | ||
| .eq("id", slotId) | ||
| .single(); |
There was a problem hiding this comment.
Derive the appointment doctor from the slot, not from the request.
The handler never checks that doctorId matches slot.doctor_id, then uses the client value for both the duplicate check and the inserted appointment. A caller can therefore book doctor A's slot while storing doctor B on the appointment, which breaks dashboard joins and bypasses the "already have an active appointment with this doctor" rule for the real slot owner.
🩹 Suggested fix
- const { slotId, doctorId, patientId } = await req.json();
+ const { slotId, patientId } = await req.json();
@@
- if (!slotId || !doctorId || !patientId) {
+ if (!slotId || !patientId) {
return NextResponse.json(
{ error: "Missing required fields" },
{ status: 400 }
);
}
@@
- const { data: slot } = await supabase
+ const { data: slot } = await supabase
.from("slots")
- .select("*")
+ .select("id, doctor_id, is_booked")
.eq("id", slotId)
.single();
@@
+ const doctorId = slot.doctor_id;
+
const { data: existing } = await supabase
.from("appointments")
.select("id")
.eq("patient_id", patientId)
.eq("doctor_id", doctorId)
.eq("status", "active");Also applies to: 33-54, 58-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/appointments/book/route.ts` around lines 20 - 24, The handler
currently trusts the incoming doctorId instead of using the slot owner; fetch
the slot (the existing `slot` result from the `slots` query) and derive the
appointment's doctor as `const doctorId = slot.doctor_id` (after checking `slot`
exists), then use that derived `doctorId` everywhere: in the duplicate-check
query that looks for an active appointment with the same doctor and patient, and
in the `insert` payload for the new appointment; ignore or remove the doctorId
coming from the request body so callers cannot substitute a different doctor.
Ensure the same change is applied to the other checks/inserts in the function
(the duplicate-check block and the appointment insert blocks referenced around
lines 33-54 and 58-61).
Comment on lines
+20
to
+24
| const { data: appt } = await supabase | ||
| .from("appointments") | ||
| .select("*, slots(start_time)") | ||
| .eq("id", appointmentId) | ||
| .single(); |
There was a problem hiding this comment.
Enforce appointment ownership before changing status.
This handler uses SUPABASE_SERVICE_ROLE_KEY, so the RLS policies in schema.sql are bypassed. Right now, any caller who knows an appointmentId can cancel it by sending any truthy patientId, or mark it done by sending any doctorId, because neither value is checked against the appointment row.
🔒 Suggested fix
- const { data: appt } = await supabase
+ const { data: appt } = await supabase
.from("appointments")
- .select("*, slots(start_time)")
+ .select("id, status, patient_id, doctor_id, slot_id, slots(start_time)")
.eq("id", appointmentId)
.single();
@@
if (action === "cancel") {
+ if (!patientId || appt.patient_id !== patientId) {
+ return NextResponse.json({ error: "Forbidden" }, { status: 403 });
+ }
+
- if (patientId) {
- const start = new Date(appt.slots.start_time).getTime();
- const now = Date.now();
- const diffHours = (start - now) / (1000 * 60 * 60);
-
- if (diffHours < 1) {
- return NextResponse.json(
- { error: "Cannot cancel less than 1 hour before appointment" },
- { status: 400 }
- );
- }
- }
+ const start = new Date(appt.slots.start_time).getTime();
+ const now = Date.now();
+ const diffHours = (start - now) / (1000 * 60 * 60);
+
+ if (diffHours < 1) {
+ return NextResponse.json(
+ { error: "Cannot cancel less than 1 hour before appointment" },
+ { status: 400 }
+ );
+ }
@@
if (action === "done") {
- if (!doctorId) {
+ if (!doctorId || appt.doctor_id !== doctorId) {
return NextResponse.json(
- { error: "Doctor only action" },
+ { error: "Forbidden" },
{ status: 403 }
);
}Also applies to: 40-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/appointments/cancel/route.ts` around lines 20 - 24, The handler uses
SUPABASE_SERVICE_ROLE_KEY so RLS is bypassed; before updating an appointment's
status, fetch the appointment (you already query into appt) and verify
ownership: if cancelling, assert that the provided patientId equals
appt.patient_id; if marking done, assert provided doctorId equals
appt.doctor_id; return 403/appropriate error if the IDs don't match or if appt
is missing, then proceed to call supabase.from("appointments").update(...) only
after the ownership check passes. Use the existing variables appointmentId,
patientId, doctorId, appt, and the "appointments" table/fields patient_id and
doctor_id to locate where to add the checks.
Comment on lines
+106
to
+113
| setActionMsg("Appointment booked successfully!"); | ||
|
|
||
| setAvailableSlots((prev) => | ||
| prev.filter((s) => s.id !== slotId) | ||
| ); | ||
|
|
||
| window.location.reload(); | ||
| } else { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid a full-page reload after booking.
window.location.reload() throws away the success state you just set and reruns the entire dashboard bootstrap on every successful booking. Refresh the affected data/state instead so the appointment list updates without a hard navigation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/patient/dashboard/page.tsx` around lines 106 - 113, Remove the
window.location.reload() call and instead update the local React state that
drives the dashboard: keep the setActionMsg and setAvailableSlots logic, then
either push the new appointment into the appointments state (e.g., call
setAppointments(prev => [newAppointment, ...prev])) or call the existing
fetch/refetch helper (e.g., fetchAppointments() or mutateAppointments()) to
refresh only the appointment list; locate this change near setActionMsg,
setAvailableSlots and slotId in page.tsx and ensure no full-page reload is used.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implemented doctor, patient, and admin modules.
Features:
Summary by CodeRabbit
New Features
Improvements