Task2 completed.#1
Open
codebyNJ wants to merge 4 commits intobotcode-com:mainfrom
Open
Conversation
WalkthroughAdds admin authentication pages and session utilities, implements authenticated booking and cancellation APIs with validations and role checks, introduces server-side Supabase admin client, updates client dashboards to send Bearer tokens, and adds Vitest tests, seed adjustments, and schema/index changes. Changes
Sequence DiagramssequenceDiagram
participant UI as Admin Login UI
participant API as /api/admin/login
participant DB as Supabase (system_admins)
participant Browser as Browser
UI->>API: POST { email, password }
API->>DB: Query system_admins for email
alt admin found & password ok
DB-->>API: admin record
API->>Browser: Set `admin_session` cookie (HTTP-only, 8h)
API-->>UI: 200 { success: true }
UI->>UI: Navigate to /admin/dashboard
else invalid
DB-->>API: no match / bad password
API-->>UI: 401 { error }
end
sequenceDiagram
participant PatientUI as Patient UI
participant Auth as Supabase Auth
participant API as /api/appointments/book
participant DB as Supabase (slots, appointments)
PatientUI->>Auth: getSession()
Auth-->>PatientUI: access_token
PatientUI->>API: POST { slotId } + Authorization: Bearer token
API->>DB: Verify requester is patient
API->>DB: Attempt atomic update to claim slot if is_booked=false
alt slot claim fails
DB-->>API: no rows updated
API-->>PatientUI: 409 Conflict (slot unavailable)
else slot claimed
API->>DB: Check existing active appointment with doctor
alt conflict exists
API->>DB: release claimed slot
API-->>PatientUI: 409 Conflict (duplicate)
else no conflict
API->>DB: Insert appointment, mark slot booked
DB-->>API: appointment created
API-->>PatientUI: 200 { appointment }
end
end
sequenceDiagram
participant UserUI as User UI (Patient/Doctor)
participant Auth as Supabase Auth
participant API as /api/appointments/cancel
participant DB as Supabase (appointments, slots)
UserUI->>Auth: getSession()
Auth-->>UserUI: access_token
UserUI->>API: POST { appointmentId, action } + Authorization: Bearer token
API->>DB: Load appointment + slot.start_time
API->>DB: Determine caller role (doctor or patient)
API->>API: Validate appointment active and role/time rules
alt validation fails
API-->>UserUI: 400/403/409 with error
else validation passes
API->>DB: Update appointment.status (only if currently active)
alt action == "cancel"
API->>DB: Update slot.is_booked = false
end
DB-->>API: update success
API-->>UserUI: 200 { status }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 administrative dashboard and login system, along with backend APIs for managing appointments, including booking and cancellation flows. It also adds a comprehensive testing suite using Vitest. Key feedback focuses on critical security vulnerabilities regarding plaintext password storage and insecure session management. Additionally, there are concerns regarding the lack of atomicity and potential race conditions in the booking and cancellation logic, as well as the need for pagination in the admin appointments endpoint to ensure scalability.
| .from("system_admins") | ||
| .select("id, email") | ||
| .eq("email", email) | ||
| .eq("password", password) |
There was a problem hiding this comment.
Comparing passwords in plaintext is a critical security vulnerability. Passwords must be hashed using a strong algorithm like bcrypt or Argon2 before storage. The login logic should verify the provided password against the stored hash using a secure comparison function.
Comment on lines
+26
to
+54
| const { data: slot } = await supabaseAdmin | ||
| .from("slots") | ||
| .select("id, is_booked") | ||
| .eq("id", slotId) | ||
| .single(); | ||
| if (!slot) return NextResponse.json({ error: "Slot not found" }, { status: 404 }); | ||
|
|
||
| const slotError = validateSlotAvailable(slot.is_booked); | ||
| if (slotError) return NextResponse.json({ error: slotError }, { status: 409 }); | ||
|
|
||
| const { data: existing } = await supabaseAdmin | ||
| .from("appointments") | ||
| .select("id") | ||
| .eq("patient_id", user.id) | ||
| .eq("doctor_id", doctorId) | ||
| .eq("status", "active") | ||
| .maybeSingle(); | ||
|
|
||
| const duplicateError = validateNoActiveAppointmentWithDoctor(!!existing); | ||
| if (duplicateError) return NextResponse.json({ error: duplicateError }, { status: 409 }); | ||
|
|
||
| const { data: appointment, error: insertError } = await supabaseAdmin | ||
| .from("appointments") | ||
| .insert({ patient_id: user.id, doctor_id: doctorId, slot_id: slotId }) | ||
| .select() | ||
| .single(); | ||
| if (insertError) return NextResponse.json({ error: insertError.message }, { status: 500 }); | ||
|
|
||
| await supabaseAdmin.from("slots").update({ is_booked: true }).eq("id", slotId); |
There was a problem hiding this comment.
This booking logic is susceptible to race conditions and lacks atomicity. Multiple concurrent requests could pass the availability check (lines 26-34) before any of them update the slot status. Additionally, if the slot update (line 54) fails after the appointment is inserted (line 47), the database will be in an inconsistent state. These operations should be performed within a single database transaction or handled via a stored procedure (RPC) to ensure atomicity and prevent overbooking.
Comment on lines
+47
to
+51
| await supabaseAdmin.from("appointments").update({ status: newStatus }).eq("id", appointmentId); | ||
|
|
||
| if (newStatus === "cancelled") { | ||
| await supabaseAdmin.from("slots").update({ is_booked: false }).eq("id", appointment.slot_id); | ||
| } |
There was a problem hiding this comment.
The updates to the appointment and slot statuses are not atomic and lack error handling. If the second update fails, the system enters an inconsistent state. Additionally, the code does not check the error object returned by the Supabase client for either update, which could lead to returning a success response even if the database operation failed. These operations should be wrapped in a transaction with proper error checking.
| if (!admin) return NextResponse.json({ error: "Invalid credentials" }, { status: 401 }); | ||
|
|
||
| const res = NextResponse.json({ success: true }); | ||
| res.cookies.set("admin_session", admin.id, { |
There was a problem hiding this comment.
Using the database ID directly as a session identifier in a cookie is insecure, especially if the IDs are predictable (e.g., auto-incrementing integers). It is recommended to generate a cryptographically secure random session token or use a signed JWT to manage sessions securely.
Comment on lines
+15
to
+18
| const { data: appointments } = await supabaseAdmin | ||
| .from("appointments") | ||
| .select("id, status, created_at, patients(name), doctors(name, specialty), slots(start_time, end_time)") | ||
| .order("created_at", { ascending: false }); |
There was a problem hiding this comment.
This endpoint fetches all appointments without pagination. As the number of records grows, this will lead to performance degradation and increased memory usage. Consider implementing pagination (e.g., using .range() or .limit()) to handle large datasets efficiently.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
♻️ Duplicate comments (1)
app/patient/dashboard/page.tsx (1)
98-108: 🧹 Nitpick | 🔵 TrivialSame empty-Bearer issue applies to both
handleBookandhandleCancel.When
getSession()returns no session, both handlers sendAuthorization: Bearerand surface a generic error. Short-circuit on missing session and route to/patient/login, consistent with theuseEffectguard above.♻️ Proposed refactor (apply to both handlers)
const { data: { session }, } = await supabase.auth.getSession(); + if (!session?.access_token) { + router.push("/patient/login"); + return; + } const res = await fetch("/api/appointments/book", { method: "POST", headers: { "Content-Type": "application/json", - Authorization: `Bearer ${session?.access_token ?? ""}`, + Authorization: `Bearer ${session.access_token}`, }, body: JSON.stringify({ slotId, doctorId }), });Also applies to: 123-133
🤖 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 98 - 108, Both handleBook and handleCancel call supabase.auth.getSession() then send an Authorization header even when session is null, causing an empty "Bearer " token; update both handlers (handleBook, handleCancel) to short-circuit when session is missing by redirecting to "/patient/login" (or using the same client-side navigation used in the useEffect guard) instead of performing the fetch, and only include Authorization: `Bearer ${session.access_token}` when session exists.
🤖 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 27-40: The fetch in the useEffect's async load function can throw
transport errors and currently never reaches setLoading(false); wrap the await
fetch("/api/admin/appointments") and subsequent JSON parsing in a
try/catch/finally inside load (or return early on non-ok responses) so any
thrown error is caught, call router.push("/admin/login") or set an error state
inside the catch, and ensure setLoading(false) is executed in finally; update
references to load, setAppointments, setLoading and router.push accordingly.
In `@app/admin/login/page.tsx`:
- Around line 14-32: handleSubmit currently calls fetch and res.json without
error handling, so network or JSON errors can leave loading true and no error
shown; wrap the fetch/response parsing and router.push in a try/catch/finally:
call setLoading(true) at start, await fetch and parse inside try, on success
call setLoading(false) then router.push("/admin/dashboard") (or
setLoading(false) in finally before navigation if you prefer), on non-ok
responses setError from parsed body, and in catch setError to a generic message
plus optional error.message; always call setLoading(false) in finally to ensure
the button is re-enabled. Reference: handleSubmit, setLoading, setError, fetch,
res.json, router.push.
In `@app/api/admin/appointments/route.ts`:
- Around line 15-20: The appointments query currently fetches all rows and
ignores Supabase errors; modify the supabaseAdmin
.from("appointments").select(...).order(...) call to include a range/limit
(e.g., .range(...) or .limit(...)) to paginate or enforce a hard cap, and
capture the returned { data, error } (not just data) so you can propagate
failures: if error is non-null return an appropriate error NextResponse (e.g.,
401/500 or redirect to login) instead of returning { appointments: [] },
otherwise return the paginated data; update any handlers that expect the full
list to accept paginated results.
In `@app/api/admin/login/route.ts`:
- Around line 4-7: The POST handler currently calls await req.json() directly
which throws on non-JSON or empty bodies; wrap the parsing in a try/catch inside
the POST function to catch JSON parse errors and return NextResponse.json({
error: "Invalid or missing JSON body" }, { status: 400 }); also validate that
the parsed payload is an object and contains email and password before
proceeding (keep the existing NextResponse.json error for missing fields),
referencing the POST function and the req.json() call so the fix is applied
around that code path.
- Around line 17-24: The current code writes the raw admin.id into the cookie
via NextResponse.json/res.cookies.set("admin_session", admin.id) which is
forgeable and lacks secure transport; change to issue a strong opaque
server-generated session token (≥128 bits) stored in an admin_sessions table
with admin_id and expires_at (or alternatively sign a JWT with admin_id+exp
using a server secret), return that token in res.cookies.set("admin_session",
<token>) and set secure: true (conditionally allow false only when NODE_ENV ===
"development"), then update the validation logic in
app/api/admin/appointments/route.ts to verify the token by looking up
admin_sessions (and checking expires_at) rather than querying system_admins by
UUID.
- Around line 9-15: The current login handler queries system_admins by comparing
raw passwords (the .eq("password", password) call) which implies storing plain
text; change the logic to query by email only (remove .eq("password",
password)), return 401 if no row, then verify the provided password using a
secure KDF (e.g., bcrypt/argon2) against the stored hash field (e.g.,
admin.password_hash) via bcrypt.compare (or equivalent) inside the route handler
in route.ts; alternatively migrate admin accounts to Supabase Auth and enforce
an admin role/claim so the route no longer stores or compares passwords
directly.
In `@app/api/admin/logout/route.ts`:
- Around line 3-7: The logout handler's res.cookies.delete("admin_session") must
pass the same cookie attributes used when setting the session cookie so deletion
is reliable; update the POST function to call
res.cookies.delete("admin_session", { path: "/", domain: <same domain if set on
login>, secure: <same secure flag>, sameSite: <sameSameSite>, httpOnly: true })
(i.e., mirror the options used in the login/set-cookie code) so the delete
matches the original cookie attributes and reliably removes the cookie.
In `@app/api/appointments/book/route.ts`:
- Around line 22-24: Wrap the call to await req.json() in a try/catch and
validate the parsed body before destructuring to avoid throwing on malformed
JSON; specifically, catch JSON parse errors around req.json(), return
NextResponse.json({ error: "Invalid or missing request body" }, { status: 400 })
on parse failure, then ensure the parsed body has slotId and doctorId (the
existing slotId and doctorId checks) and return the existing 400 response if
they are missing or falsy.
- Around line 22-45: The request currently trusts doctorId from the client;
change the flow to get doctor_id from the fetched slot and use that value for
validations and inserts: update the supabaseAdmin query that reads the slot
(currently selecting "id, is_booked") to also select "doctor_id", then either
assert that the incoming doctorId equals slot.doctor_id or replace the request
doctorId with slot.doctor_id before calling
validateNoActiveAppointmentWithDoctor and before creating the appointment;
ensure the symbol names involved are slotId, doctorId (from request),
slot.doctor_id (from DB), supabaseAdmin .from("slots"), validateSlotAvailable
and validateNoActiveAppointmentWithDoctor so the duplicate check and the
eventual insert use the authoritative doctor_id from the slot.
- Around line 33-54: The current flow checks
validateSlotAvailable(slot.is_booked) then inserts an appointment and updates
slots, which allows a race; instead perform an atomic claim of the slot (e.g., a
conditional UPDATE on the slots row that sets is_booked = true only when
is_booked = false and returns the row) before creating the appointment, check
the returned result to ensure the claim succeeded, then insert into
appointments; if the insert fails revert the claim by setting slots.is_booked
back to false (or better: move both operations into a single DB transaction /
Postgres RPC and execute the slot claim + appointments.insert together); update
the code paths around validateSlotAvailable, the conditional
slots.update(eq("id", slotId) ...) and the appointments.insert to implement this
atomic-claim-then-insert (or replace with a transactional RPC) and handle
rollback on insert failure.
In `@app/api/appointments/cancel/route.ts`:
- Around line 19-21: Wrap the call to req.json() in a try/catch inside the route
handler so malformed or non-JSON request bodies return NextResponse.json(..., {
status: 400 }) instead of throwing a 500; specifically, catch the JSON parse
error around the existing const { appointmentId, action } = await req.json() and
return a 400 with a clear message (e.g., "invalid or malformed JSON body") when
parsing fails, then continue to validate appointmentId and action as before.
- Around line 46-53: The current handler discards DB results and performs two
non-atomic writes (supabaseAdmin.from("appointments").update and
supabaseAdmin.from("slots").update) after validateAppointmentActive, which can
return success on failure and leave inconsistent state; change the appointment
update to be conditional by adding .eq("id", appointmentId).eq("status",
"active") so it only updates if still active, check the returned rowCount or
data from that update and only call supabaseAdmin.from("slots").update({
is_booked: false }).eq("id", appointment.slot_id) if the appointment update
actually affected a row, surface and propagate any DB errors to the HTTP
response instead of always returning NextResponse.json({ success: true }), and
consider moving both mutations into a Postgres RPC/transaction if you need true
atomicity and to eliminate the TOCTOU between validateAppointmentActive and the
unconditional update.
- Around line 25-44: The current code unsafely casts appointment.slots to access
start_time which can be undefined and let validatePatientCancelTime receive
undefined; update the route to first verify appointment.slots exists and that
(appointment.slots as { start_time?: string }).start_time is a non-empty string
(or use optional chaining like appointment.slots?.start_time) and if missing
return a 409/Bad Request JSON error (e.g., "Missing slot start_time") before
calling validatePatientCancelTime; reference the appointment variable,
appointment.slots, start_time, and validatePatientCancelTime to locate and fix
the check.
In `@app/doctor/dashboard/page.tsx`:
- Around line 94-104: After calling supabase.auth.getSession() in page.tsx,
check whether session and session.access_token exist and, if not, perform a
redirect to "/doctor/login" and return early instead of making the
fetch("/api/appointments/cancel") call; in other words, gate the fetch behind a
presence check of the session (supabase.auth.getSession) and mirror the existing
useEffect early-exit pattern so you never send an empty Authorization: Bearer
header when session is missing.
In `@lib/appointments.ts`:
- Around line 11-21: The validator currently treats unparseable startTime as
allowed because new Date(startTime).getTime() can be NaN; update
validatePatientCancelTime to detect invalid dates by computing const startMs =
new Date(startTime).getTime() and if Number.isNaN(startMs) return an explicit
error string (e.g., "Invalid appointment start time") instead of proceeding,
then compute diffHours from startMs and now and keep the existing 1‑hour check;
reference validatePatientCancelTime and the startTime/now parameters when making
the change.
In `@tests/integration/booking.test.ts`:
- Around line 40-45: The afterAll teardown currently calls
supabaseAdmin.from(...).update(...).eq("id", slotId) and deletes using
createdAppointmentId without checking if those variables were ever assigned;
change the afterAll block to guard both operations by only performing the delete
when createdAppointmentId is truthy and only updating the "slots" row when
slotId is truthy (e.g., if (createdAppointmentId) await
supabaseAdmin.from("appointments").delete().eq("id", createdAppointmentId); and
if (slotId) await supabaseAdmin.from("slots").update({ is_booked: false
}).eq("id", slotId);) so undefined IDs are never sent to supabase.
- Around line 17-38: Replace hardcoded credentials in the beforeAll setup by
reading TEST_PATIENT_EMAIL and TEST_PATIENT_PASSWORD from env and use them in
createClient(...).auth.signInWithPassword; after calling signInWithPassword (and
after the supabaseAdmin.from("slots").select(...) query) check the returned
error/result instead of using session!/slot! non-null assertions: if error or
missing data, throw or fail the test with a clear message including the error
and relevant response (e.g., indicate signInWithPassword failed for
TEST_PATIENT_EMAIL or slots query returned no data), and then assign token,
patientId, slotId from the validated response values.
In `@tests/integration/cancellation.test.ts`:
- Around line 15-57: The setup can leave appointmentId/testSlotId undefined and
cause unsafe cleanup or cross-test interference; in beforeAll validate the
signInWithPassword result (check session and session.user.id) and the slot query
(ensure slot is returned) and fail fast with a clear error if missing, then
create the appointment; in afterAll only run supabaseAdmin.delete/update if
appointmentId/testSlotId are truthy (guard with if (appointmentId) / if
(testSlotId)); to avoid parallel-run clashes scope the test to a dedicated
slot/doctor (use a seeded doctor_id or create a new slot record for this test)
instead of selecting the first available, and replace hardcoded credentials used
in signInWithPassword (patient3@test.com / patient123) with environment
variables to make the test portable.
In `@tests/setup.ts`:
- Around line 1-5: process.loadEnvFile in tests/setup.ts can fail silently on
older Node versions; add an "engines" constraint to package.json (e.g., "node":
">=20.12.0") to declare the minimum supported Node, update CI workflow(s) to use
that Node version (set node-version to the same minimum), and add a small
runtime check in tests/setup.ts to detect unsupported Node (or absence of
process.loadEnvFile) and throw a clear error so the .env loading failure is not
silently ignored.
In `@tests/unit/book.test.ts`:
- Around line 4-22: The tests currently only assert non-null for
validateSlotAvailable and validateNoActiveAppointmentWithDoctor, which is weak;
update the two failing-case assertions to verify the returned value is a
non-empty string and matches the exact error message (or a stable regex)
expected by app/api/appointments/book/route.ts (e.g., use
expect(result).toEqual("...") or expect(result).toMatch(/.../)) and keep the
success-case assertions asserting null; target the test cases that call
validateSlotAvailable(true) and validateNoActiveAppointmentWithDoctor(true) and
replace not.toBeNull() with explicit string-type/content assertions.
---
Duplicate comments:
In `@app/patient/dashboard/page.tsx`:
- Around line 98-108: Both handleBook and handleCancel call
supabase.auth.getSession() then send an Authorization header even when session
is null, causing an empty "Bearer " token; update both handlers (handleBook,
handleCancel) to short-circuit when session is missing by redirecting to
"/patient/login" (or using the same client-side navigation used in the useEffect
guard) instead of performing the fetch, and only include Authorization: `Bearer
${session.access_token}` when session exists.
🪄 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: d1a5c192-cfbb-4cb4-b4db-c5a884a622fc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
app/admin/dashboard/page.tsxapp/admin/login/page.tsxapp/api/admin/appointments/route.tsapp/api/admin/login/route.tsapp/api/admin/logout/route.tsapp/api/appointments/book/route.tsapp/api/appointments/cancel/route.tsapp/doctor/dashboard/page.tsxapp/page.tsxapp/patient/dashboard/page.tsxlib/appointments.tslib/supabase-server.tspackage.jsontests/integration/booking.test.tstests/integration/cancellation.test.tstests/setup.tstests/unit/book.test.tstests/unit/cancel.test.tsvitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
app/api/appointments/cancel/route.ts (1)
69-71:⚠️ Potential issue | 🟠 MajorSlot unbook still discards errors and the two writes are non-atomic.
If the appointment update succeeded but
slots.update({ is_booked: false })fails (RLS, constraint, network), the appointment iscancelledwhile the slot staysis_booked = true— an orphaned booking — and the handler still returns{ success: true }. At minimum, capture and surface the error; for true atomicity, fold both mutations into a Postgres function (Supabase RPC) so they commit in one transaction.Proposed minimum fix (best-effort error surface)
if (newStatus === "cancelled") { - await supabaseAdmin.from("slots").update({ is_booked: false }).eq("id", appointment.slot_id); + const { error: slotErr } = await supabaseAdmin + .from("slots") + .update({ is_booked: false }) + .eq("id", appointment.slot_id); + if (slotErr) { + return NextResponse.json( + { error: "Appointment cancelled but failed to release slot" }, + { status: 500 } + ); + } }🤖 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 69 - 71, The slot unbook call (supabaseAdmin.from("slots").update({ is_booked: false }).eq("id", appointment.slot_id")) discards errors and runs separately from the appointment update (newStatus flow), risking inconsistent state; catch and surface the update error and return failure if it fails, or better yet move both mutations into a single Postgres transaction via a Supabase RPC (create an RPC that sets appointment status and clears slots.is_booked atomically) and call that RPC from this handler so both writes commit or roll back together.
🤖 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/api/admin/login/route.ts`:
- Around line 7-16: The parsed request body (variable body from await
req.json()) can be null or a non-object (e.g., array/primitive), so the current
cast and destructuring into const { email, password } will throw a TypeError;
update the parsing block in route.ts to validate that body is a non-null plain
object (e.g., typeof body === "object" && body !== null && !Array.isArray(body))
before casting/destructuring, and if that check fails return the 400
NextResponse.json({ error: "Invalid or missing JSON body" }, { status: 400 }) so
email/password extraction is safe.
- Around line 18-25: The code leaks user-existence via timing because
bcrypt.compare is skipped when `admin` is falsy; to fix, perform a dummy
bcrypt.compare against a constant known hash whenever `admin` is missing so
timing matches a real lookup: ensure you introduce a constant DUMMY_HASH (used
only for timing), call `await bcrypt.compare(password as string, DUMMY_HASH)`
when `admin` is undefined, then compute `valid` only from the real compare
result when `admin` exists (or false otherwise), and keep the existing response
behavior (`NextResponse.json({ error: "Invalid credentials" }, { status: 401
})`) when not valid; reference `supabaseAdmin`, `system_admins`, `admin`, and
`bcrypt.compare` when making the change.
In `@app/api/appointments/book/route.ts`:
- Around line 42-56: The current check-then-insert flow around
validateNoActiveAppointmentWithDoctor (in route.ts) is racy; add a DB-level
partial unique index (e.g., UNIQUE on (patient_id, doctor_id) WHERE
status='active') and change the insert path to handle unique-violation errors
from supabaseAdmin when creating the appointment: on a unique-violation, catch
the DB error, release the claimed slot (supabaseAdmin.from("slots").update({
is_booked: false }).eq("id", slotId)), and return a 409 JSON error (same
behavior as the existing duplicate handling). Keep existing optimistic check if
desired but rely on the DB constraint + error handling as the authoritative
prevention of duplicate active appointments.
- Line 6: The authorization header parsing is brittle: replace("Bearer ", "") is
case-sensitive and only handles a single space; update the token extraction in
route.ts to robustly parse req.headers.get("authorization") by validating the
scheme case-insensitively and trimming any whitespace before passing to getUser.
Specifically, ensure you check that the header exists, split or match the header
into scheme and token (case-insensitive for "bearer") and fail gracefully
(null/unauthorized) if the scheme is not bearer or the token is missing, then
pass the cleaned token to getUser.
In `@lib/session.ts`:
- Around line 23-24: Replace the naive string equality check for the HMAC in the
verification logic with a constant-time comparison: decode both the provided
signature (`sig`) and the computed `expected` into Buffer objects (ensuring you
handle the base64url encoding consistently), check that their lengths match and
if so use `crypto.timingSafeEqual(bufSig, bufExpected)` to compare; if lengths
differ or timingSafeEqual returns false, return null. Target the code around the
HMAC creation (`createHmac("sha256",
getSecret()).update(payload).digest("base64url")`) and the subsequent signature
check to implement this change.
In `@package.json`:
- Around line 15-16: Remove the redundant `@types/bcryptjs` dependency from
package.json since bcryptjs@^3 includes its own TypeScript declarations; delete
the "@types/bcryptjs" entry (leaving "bcryptjs": "^3.0.3" intact), then
reinstall/update lockfile (npm install or yarn install) so the lockfile reflects
the change.
In `@tests/integration/booking.test.ts`:
- Around line 49-79: Move the assignment of createdAppointmentId to immediately
after parsing the response (assign createdAppointmentId = data.appointment.id
right after const data = await res.json()), before any assertions, so that any
test failure still leaves the created appointment id available for cleanup;
update references in this test (the local variable createdAppointmentId) and
leave the subsequent database verification/assertions unchanged.
---
Duplicate comments:
In `@app/api/appointments/cancel/route.ts`:
- Around line 69-71: The slot unbook call (supabaseAdmin.from("slots").update({
is_booked: false }).eq("id", appointment.slot_id")) discards errors and runs
separately from the appointment update (newStatus flow), risking inconsistent
state; catch and surface the update error and return failure if it fails, or
better yet move both mutations into a single Postgres transaction via a Supabase
RPC (create an RPC that sets appointment status and clears slots.is_booked
atomically) and call that RPC from this handler so both writes commit or roll
back together.
🪄 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: bff19ab3-4b5b-4963-b0b3-c23873a0b353
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
app/admin/dashboard/page.tsxapp/admin/login/page.tsxapp/api/admin/appointments/route.tsapp/api/admin/login/route.tsapp/api/admin/logout/route.tsapp/api/appointments/book/route.tsapp/api/appointments/cancel/route.tsapp/doctor/dashboard/page.tsxapp/patient/dashboard/page.tsxlib/appointments.tslib/session.tspackage.jsonschema.sqlseed.mjstests/integration/booking.test.tstests/integration/cancellation.test.tstests/setup.tstests/unit/book.test.ts
Comment on lines
+42
to
+56
| // Check for duplicate active appointment with the same doctor (doctor_id comes from slot, not client) | ||
| const { data: existing } = await supabaseAdmin | ||
| .from("appointments") | ||
| .select("id") | ||
| .eq("patient_id", user.id) | ||
| .eq("doctor_id", slot.doctor_id) | ||
| .eq("status", "active") | ||
| .maybeSingle(); | ||
|
|
||
| const duplicateError = validateNoActiveAppointmentWithDoctor(!!existing); | ||
| if (duplicateError) { | ||
| // Release the slot we just claimed | ||
| await supabaseAdmin.from("slots").update({ is_booked: false }).eq("id", slotId); | ||
| return NextResponse.json({ error: duplicateError }, { status: 409 }); | ||
| } |
There was a problem hiding this comment.
Residual race: two concurrent bookings can still create duplicate active appointments with the same doctor.
The slot-claim is now atomic, but the duplicate-doctor check at lines 43-49 + insert at 58-62 is still a non-atomic check-then-write. A patient firing two requests in parallel for two different free slots of the same doctor will pass validateNoActiveAppointmentWithDoctor on both before either insert lands, ending up with two active appointments for the same (patient_id, doctor_id).
A reliable fix is a partial unique index in the DB (e.g., CREATE UNIQUE INDEX one_active_per_doctor ON appointments (patient_id, doctor_id) WHERE status = 'active';) and treating the resulting unique-violation on insert as a 409, then releasing the claimed slot.
🤖 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 42 - 56, The current
check-then-insert flow around validateNoActiveAppointmentWithDoctor (in
route.ts) is racy; add a DB-level partial unique index (e.g., UNIQUE on
(patient_id, doctor_id) WHERE status='active') and change the insert path to
handle unique-violation errors from supabaseAdmin when creating the appointment:
on a unique-violation, catch the DB error, release the claimed slot
(supabaseAdmin.from("slots").update({ is_booked: false }).eq("id", slotId)), and
return a 409 JSON error (same behavior as the existing duplicate handling). Keep
existing optimistic check if desired but rely on the DB constraint + error
handling as the authoritative prevention of duplicate active appointments.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/api/appointments/cancel/route.ts`:
- Around line 54-59: The current code assumes appointment.slots is a single
object; make it defensive for Supabase relation shapes by checking whether
appointment.slots is an array or object before reading start_time: if
Array.isArray(appointment.slots) pick the first element (or handle empty array
as an error), else if it's an object read its start_time; also guard that the
chosen slot has a start_time and return the same NextResponse.json error (500)
if missing; then pass the extracted start_time into
validatePatientCancelTime(startTime, isDoctor) as before.
In `@seed.mjs`:
- Around line 84-91: Reorder and tighten error handling so seeding fails fast
and avoids partial writes: after performing the slot insert call, check
slotsError immediately and call process.exit(1) if it exists before doing the
admin upsert; only when slot insertion succeeded, perform the admin upsert (the
code block that sets adminError) and if adminError is set, log the error and
call process.exit(1) instead of continuing; update the control flow around the
variables slotsError and adminError (and the admin upsert call) so slot
validation runs first, then admin upsert, and both failure paths call
process.exit(1).
In `@tests/integration/booking.test.ts`:
- Around line 7-8: Replace the unsafe non-null assertions on process.env with a
failing-fast guard: check that supabaseUrl and supabaseAnonKey (the variables
defined in tests/integration/booking.test.ts) are present and throw a
descriptive Error if either is missing (or centralize this in a small helper
like getRequiredEnv(varName) used to initialize supabaseUrl/supabaseAnonKey) so
tests fail with a clear message instead of a cryptic TypeError.
🪄 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: ca19e6ec-99b7-487e-b24b-29da798ec6f9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.gitignoreapp/api/admin/login/route.tsapp/api/appointments/book/route.tsapp/api/appointments/cancel/route.tslib/session.tspackage.jsonschema.sqlseed.mjstests/integration/booking.test.ts
Comment on lines
+54
to
+59
| if (!appointment.slots) | ||
| return NextResponse.json({ error: "Slot data missing for this appointment" }, { status: 500 }); | ||
|
|
||
| const startTime = (appointment.slots as { start_time: string }).start_time; | ||
| const timeError = validatePatientCancelTime(startTime, isDoctor); | ||
| if (timeError) return NextResponse.json({ error: timeError }, { status: 409 }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Slots cast is now guarded but could be more defensive.
The null check on line 54 handles the missing slots case. However, Supabase's joined relations can return either an object or array depending on query configuration. Adding explicit handling would be more robust.
♻️ Defensive handling for relation shape
if (!appointment.slots)
return NextResponse.json({ error: "Slot data missing for this appointment" }, { status: 500 });
- const startTime = (appointment.slots as { start_time: string }).start_time;
+ const slotsData = appointment.slots as { start_time: string } | { start_time: string }[];
+ const startTime = Array.isArray(slotsData) ? slotsData[0]?.start_time : slotsData?.start_time;
+ if (!startTime)
+ return NextResponse.json({ error: "Slot start time missing" }, { status: 500 });
const timeError = validatePatientCancelTime(startTime, isDoctor);🤖 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 54 - 59, The current code
assumes appointment.slots is a single object; make it defensive for Supabase
relation shapes by checking whether appointment.slots is an array or object
before reading start_time: if Array.isArray(appointment.slots) pick the first
element (or handle empty array as an error), else if it's an object read its
start_time; also guard that the chosen slot has a start_time and return the same
NextResponse.json error (500) if missing; then pass the extracted start_time
into validatePatientCancelTime(startTime, isDoctor) as before.
Comment on lines
+84
to
91
| const { error: adminError } = await supabase | ||
| .from("system_admins") | ||
| .upsert({ email: "admin@test.com", password: ADMIN_PASSWORD_HASH }, { onConflict: "email" }); | ||
| if (adminError) console.error("upsert admin", adminError.message); | ||
| else console.log("seeded admin@test.com"); | ||
|
|
||
| if (slotsError) { console.error("insert slots", slotsError.message); process.exit(1); } | ||
| console.log("created slots"); |
There was a problem hiding this comment.
Make seed failures fail-fast and avoid partial writes.
slotsError is handled only at Line 90, after the admin upsert has already run. Also, adminError only logs and continues. This can leave a partially seeded DB with a success exit code.
Use fail-fast ordering: validate slot insert result first, then run admin upsert, and process.exit(1) on admin failure too.
Suggested fix
-// Seed admin — password is bcrypt hash of "admin123" (cost 10), pre-computed
-const ADMIN_PASSWORD_HASH = "$2b$10$NmgnMhacdKYuHL6fWmsKYOAPkkUVfyXNWdjfp8bHQWd8aB08VX6qa";
-const { error: adminError } = await supabase
- .from("system_admins")
- .upsert({ email: "admin@test.com", password: ADMIN_PASSWORD_HASH }, { onConflict: "email" });
-if (adminError) console.error("upsert admin", adminError.message);
-else console.log("seeded admin@test.com");
-
if (slotsError) { console.error("insert slots", slotsError.message); process.exit(1); }
console.log("created slots");
+
+// Seed admin — password is bcrypt hash of "admin123" (cost 10), pre-computed
+const ADMIN_PASSWORD_HASH = "$2b$10$NmgnMhacdKYuHL6fWmsKYOAPkkUVfyXNWdjfp8bHQWd8aB08VX6qa";
+const { error: adminError } = await supabase
+ .from("system_admins")
+ .upsert({ email: "admin@test.com", password: ADMIN_PASSWORD_HASH }, { onConflict: "email" });
+if (adminError) { console.error("upsert admin", adminError.message); process.exit(1); }
+console.log("seeded admin@test.com");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@seed.mjs` around lines 84 - 91, Reorder and tighten error handling so seeding
fails fast and avoids partial writes: after performing the slot insert call,
check slotsError immediately and call process.exit(1) if it exists before doing
the admin upsert; only when slot insertion succeeded, perform the admin upsert
(the code block that sets adminError) and if adminError is set, log the error
and call process.exit(1) instead of continuing; update the control flow around
the variables slotsError and adminError (and the admin upsert call) so slot
validation runs first, then admin upsert, and both failure paths call
process.exit(1).
Comment on lines
+7
to
+8
| const supabaseUrl = process.env.NEXT_PUBLIC_SUPABASE_URL!; | ||
| const supabaseAnonKey = process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider failing fast with descriptive errors when required env vars are missing.
The non-null assertions (!) will throw a cryptic TypeError at runtime if these variables are unset. A guard with a clear message aids debugging.
♻️ Proposed improvement
-const supabaseUrl = process.env.NEXT_PUBLIC_SUPABASE_URL!;
-const supabaseAnonKey = process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!;
+const supabaseUrl = process.env.NEXT_PUBLIC_SUPABASE_URL;
+const supabaseAnonKey = process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY;
+if (!supabaseUrl || !supabaseAnonKey) {
+ throw new Error("NEXT_PUBLIC_SUPABASE_URL and NEXT_PUBLIC_SUPABASE_ANON_KEY must be set");
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/booking.test.ts` around lines 7 - 8, Replace the unsafe
non-null assertions on process.env with a failing-fast guard: check that
supabaseUrl and supabaseAnonKey (the variables defined in
tests/integration/booking.test.ts) are present and throw a descriptive Error if
either is missing (or centralize this in a small helper like
getRequiredEnv(varName) used to initialize supabaseUrl/supabaseAnonKey) so tests
fail with a clear message instead of a cryptic TypeError.
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.
All the endpoints with the testing requirements have been implemented.
Summary by CodeRabbit