feat: implement booking/cancel APIs, admin module, tests, and race condition handling#9
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR implements a complete appointment management system with admin oversight. It adds admin login/dashboard pages, implements appointment booking and cancellation APIs with authentication and role-based authorization, updates patient and doctor dashboards to use the new APIs, introduces test infrastructure with unit and integration tests, and adds database RLS policies for admin access. Changes
Sequence DiagramssequenceDiagram
actor Admin
participant Browser
participant LoginPage as /admin/login
participant Supabase
participant DB as system_admins<br/>table
Admin->>Browser: Enter email & password
Browser->>LoginPage: Submit form
LoginPage->>Supabase: Create client + getUser()<br/>from headers
LoginPage->>DB: Query system_admins<br/>for email & password
DB-->>Supabase: Return admin record
Supabase-->>LoginPage: Admin found
LoginPage->>Browser: Write localStorage<br/>(admin_logged_in, email)
LoginPage->>Browser: Navigate to /admin/dashboard
Browser->>Admin: Dashboard loaded with<br/>appointments & users
sequenceDiagram
actor Patient
participant Browser
participant PatientDash as Patient Dashboard
participant BookAPI as /api/appointments/book
participant Supabase as Supabase Auth
participant DB as Database
Patient->>Browser: Select slot & doctor
Browser->>PatientDash: Trigger booking
PatientDash->>Supabase: Get session token
PatientDash->>BookAPI: POST with Bearer token<br/>{ slot_id, doctor_id }
BookAPI->>Supabase: Validate user from token
BookAPI->>DB: Check for active appt<br/>same patient/doctor
alt Active appointment exists
DB-->>BookAPI: Conflict (409)
BookAPI-->>PatientDash: Error response
else No active appointment
BookAPI->>DB: UPDATE slot<br/>is_booked=true<br/>WHERE is_booked=false
alt Slot already booked
DB-->>BookAPI: Conflict (409)
BookAPI-->>PatientDash: Slot taken by another
else Slot reserved
BookAPI->>DB: INSERT appointment<br/>status="active"
DB-->>BookAPI: Appointment created (201)
BookAPI-->>PatientDash: Appointment confirmed
PatientDash->>Browser: Refresh appointment list
end
end
sequenceDiagram
actor User as Patient/Doctor
participant Browser
participant Dashboard
participant CancelAPI as /api/appointments/cancel
participant Supabase as Supabase Auth<br/>(anon & service)
participant DB as Database
User->>Browser: Click cancel appointment
Browser->>Dashboard: Request cancellation
Dashboard->>Supabase: Get anon user session token
Dashboard->>CancelAPI: POST with Bearer token<br/>{ appointment_id, action }
CancelAPI->>Supabase: Validate user from token
CancelAPI->>DB: Fetch appointment<br/>with slot details
alt Appointment not found
DB-->>CancelAPI: Not found (404)
else Appointment found
CancelAPI->>CancelAPI: Check permissions:<br/>patient_id/doctor_id match<br/>& role-based rules
alt Patient within 1 hour<br/>of slot OR status done/cancelled
CancelAPI-->>Dashboard: Forbidden (403)
else Permission granted
CancelAPI->>Supabase: Create service-role<br/>client
CancelAPI->>DB: UPDATE appointment<br/>status=action
alt action=="cancelled"
CancelAPI->>DB: UPDATE slot<br/>is_booked=false
end
DB-->>CancelAPI: Updated (200)
CancelAPI-->>Dashboard: Success
Dashboard->>Browser: Refresh appointments
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes The review requires careful examination of authentication flows across multiple endpoints, business logic enforcement (role-based permissions, race-condition handling), coordination between frontend and backend API contracts, database-level atomic operations, and test coverage for new functionality. Heterogeneous changes span admin pages, API endpoints, dashboard updates, schema modifications, and comprehensive test suites with distinct reasoning needed for each area. 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)
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 17 minutes and 49 seconds.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 admin module with a dashboard and login system, implements core appointment booking and cancellation API routes with race condition handling, and adds a comprehensive test suite including unit and integration tests. While the functional additions are significant, several critical security vulnerabilities were identified, specifically regarding overly permissive RLS policies that expose administrative and patient data to the public, and the use of plain-text password comparisons for admin authentication. Additionally, feedback was provided to correct logic errors in the cancellation time-window check and to resolve redundant exports in the Jest configuration.
| CREATE POLICY "allow_admin_login" ON system_admins | ||
| FOR SELECT USING (true); |
There was a problem hiding this comment.
This policy allows any user (including unauthenticated ones using the anonymous key) to read the entire system_admins table. Since this table contains administrator emails and passwords, this is a critical security vulnerability. Access to this table should be strictly restricted or handled via a secure server-side function.
| CREATE POLICY "admin_read_all_appointments" ON appointments | ||
| FOR SELECT USING (true); | ||
|
|
||
| CREATE POLICY "admin_read_all_patients" ON patients | ||
| FOR SELECT USING (true); | ||
|
|
||
| CREATE POLICY "admin_read_all_doctors" ON doctors | ||
| FOR SELECT USING (true); |
There was a problem hiding this comment.
These policies grant public read access (USING (true)) to appointments, patients, and doctors. This exposes sensitive personal and medical scheduling information to anyone with the project's anonymous key. These policies should be restricted to authorized administrative users only.
| const { data: admin, error: adminError } = await supabase | ||
| .from("system_admins") | ||
| .select("*") | ||
| .eq("email", email) | ||
| .eq("password", password) | ||
| .single(); |
There was a problem hiding this comment.
The login logic performs a direct equality check on plain text passwords in the database. This is a highly insecure practice. Passwords should be stored as salted hashes (e.g., using Argon2 or bcrypt) and verified using a secure hash comparison function. Additionally, the client-side query relies on the insecure RLS policies identified in schema.sql.
| module.exports = { | ||
| preset: 'ts-jest', | ||
| testEnvironment: 'node', | ||
| testMatch: ['**/tests/**/*.test.ts'], | ||
| }; | ||
|
|
||
| module.exports = { | ||
| preset: 'ts-jest', | ||
| testEnvironment: 'node', | ||
| testMatch: ['**/tests/**/*.test.ts'], | ||
| setupFiles: ['<rootDir>/tests/setup.ts'], | ||
| }; No newline at end of file |
There was a problem hiding this comment.
The configuration file contains two separate module.exports assignments. The second assignment completely overwrites the first one, making the first block redundant. These should be merged into a single export object.
| module.exports = { | |
| preset: 'ts-jest', | |
| testEnvironment: 'node', | |
| testMatch: ['**/tests/**/*.test.ts'], | |
| }; | |
| module.exports = { | |
| preset: 'ts-jest', | |
| testEnvironment: 'node', | |
| testMatch: ['**/tests/**/*.test.ts'], | |
| setupFiles: ['<rootDir>/tests/setup.ts'], | |
| }; | |
| module.exports = { | |
| preset: 'ts-jest', | |
| testEnvironment: 'node', | |
| testMatch: ['**/tests/**/*.test.ts'], | |
| setupFiles: ['<rootDir>/tests/setup.ts'], | |
| }; |
|
|
||
| } catch (err) { | ||
| console.error("BOOK ROUTE ERROR:", err); | ||
| return NextResponse.json({ error: String(err) }, { status: 500 }); |
There was a problem hiding this comment.
Returning the raw error string String(err) to the client can leak sensitive information about the database schema or internal logic. It is safer to log the detailed error on the server and return a generic error message to the user.
| return NextResponse.json({ error: String(err) }, { status: 500 }); | |
| return NextResponse.json({ error: "Internal server error" }, { status: 500 }); |
| const now = new Date(); | ||
| const diffHours = (slotStart.getTime() - now.getTime()) / (1000 * 60 * 60); | ||
|
|
||
| if (diffHours > 0 &&diffHours < 1) { |
There was a problem hiding this comment.
There is a missing space in the condition &&diffHours. Additionally, the logic diffHours > 0 && diffHours < 1 only prevents cancellation for appointments starting within the next hour. It does not block patients from cancelling appointments that have already started or passed (where diffHours <= 0), provided the status is still 'active'. Consider using diffHours < 1 to cover both cases.
| if (diffHours > 0 &&diffHours < 1) { | |
| if (diffHours < 1) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🤖 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 93-96: Replace the unsafe double-casts (e.g., the use of "as
unknown as Appointment[]") by using Supabase generic typing and/or runtime
validation before calling setAppointments/setDoctors/setPatients/setSlots: call
the Supabase query with the proper generic type (e.g.,
supabase.from<Appointment>('appointments').select(...)) or validate
apptRes.data/doctorRes.data/patientRes.data/slotRes.data with a schema validator
(Zod or similar) and only set state when validation passes; also check
response.error and handle null/undefined data instead of forcing a cast so you
won't silently accept mismatched shapes.
- Around line 85-91: The slots query invoked via
supabase.from("slots").select(...) in the admin dashboard will fail because the
slots table lacks an unrestricted SELECT policy; add a new RLS policy named
admin_read_all_slots on the slots table (FOR SELECT USING (true)) or an
equivalent policy that allows anonymous/admin access to SELECT, or update the
existing policies to permit the anon key to read slots, then deploy the SQL
policy (via a migration or Supabase SQL editor) so the dashboard query succeeds
without an authenticated session.
In `@app/admin/login/page.tsx`:
- Around line 20-23: Replace the inline createClient(...) call by importing and
using the shared Supabase client exported from the centralized module (the
exported symbol is supabase) instead of creating a new client; remove the local
const supabase = createClient(...) declaration and add an import for the shared
supabase client at the top of the file, then reference that imported supabase in
all places where the local variable was used to ensure consistent configuration.
- Around line 25-36: The current admin login queries Supabase with
.eq("password", password) (the supabase.from("system_admins")... .eq("password",
password) call) which compares plaintext passwords—mark this block as DEMO ONLY
with a clear // DEMO ONLY comment; remove/avoid plaintext password comparison
for production and instead use Supabase Auth (auth.signInWithPassword) as done
in app/doctor/login/page.tsx or perform hashed-password verification server-side
(verify hashed password in an API route or auth service) and stop storing raw
passwords; update the logic around the admin fetch/error handling (the
admin/adminError handling and setError/setLoading usage) to use the chosen
secure auth flow.
- Around line 38-42: Replace the insecure localStorage session handling by
marking it as demo-only and wiring a proper server-validated session: add a
comment above the localStorage writes like "// DEMO ONLY - Production should use
signed JWTs or Supabase Auth sessions", keep the existing lines
(localStorage.setItem("admin_logged_in", "true");
localStorage.setItem("admin_email", email); router.push("/admin/dashboard");)
only for demo, and implement a production flow that issues and validates a
signed JWT (e.g., using jose) or uses Supabase Auth with role-based claims from
the server, validating tokens on /admin/dashboard and all admin APIs (replace
checks against admin_logged_in/admin_email with real token/session validation).
In `@app/api/appointments/book/route.ts`:
- Around line 34-48: Add a DB-level unique partial index to enforce one active
appointment per patient-doctor pair (CREATE UNIQUE INDEX ... WHERE
status='active'), then update the route handler around the
supabase.from("appointments").insert(...) call in
app/api/appointments/book/route.ts to catch the unique-constraint violation
(Postgres error code 23505) returned by Supabase and return a 409 JSON response
like the existing duplicate message; ensure the insert error handling
distinguishes the 23505 constraint failure from other errors and surfaces the
same "You already have an active appointment with this doctor" payload.
In `@app/api/appointments/cancel/route.ts`:
- Around line 107-113: The cancel flow updates the appointment status and then
separately frees the slot (the block where action === "cancelled" calls
supabase.from("slots").update), risking inconsistency if one operation fails;
fix by performing both updates atomically—either call a single Supabase
RPC/stored procedure that updates the appointments row and sets slots.is_booked
= false in one transaction, or implement explicit rollback: update the slot
first (supabase.from("slots").update(...).eq("id", appointment.slot_id")) and
then update the appointment, and if the appointment update fails immediately
revert the slot update (set is_booked back to true) while surfacing the original
error; ensure error handling around both operations and surface failures to the
caller.
- Around line 74-84: The cancellation check using slotStart, now, and diffHours
is wrong because it only blocks appointments 0–1 hours in the future; change the
condition to block any appointment that is already started or less than 1 hour
away by using diffHours < 1 (instead of diffHours > 0 && diffHours < 1) before
returning the NextResponse.json error for appointment.slots.start_time; ensure
the error branch covers negative diffHours (started) and keep the same response
payload/status.
- Around line 32-35: The request body currently destructures appointment_id and
action and uses action directly in the DB update; add an allowlist check for
action (accept only "cancel" or "done"), return a 400 NextResponse if action is
missing or not one of those values, and map the allowed action to the correct DB
status string (e.g., "cancel" -> "cancelled", "done" -> "done") before executing
the update that references appointment_id and action (the code around the
appointment_id/action destructuring and the update statement).
In `@app/patient/dashboard/page.tsx`:
- Line 102: Remove the debug console.log that prints the access token in
page.tsx: locate the console.log("TOKEN:", token) call (referencing the token
variable) and delete it (or replace with a non-sensitive debug message that does
not include the token) so no authentication secrets are written to the browser
console.
In `@jest.config.js`:
- Around line 1-12: Remove the duplicate dead module.exports assignment at the
top and keep the second full configuration that includes setupFiles;
specifically, delete the first module.exports block so only the configuration
containing preset, testEnvironment, testMatch and setupFiles remains (ensure the
retained block with setupFiles: ['<rootDir>/tests/setup.ts'] is the exported
object).
In `@README.md`:
- Around line 9-19: Add blank lines before and after each fenced code block and
make the ordered list numbering style consistent (use "5." and "6." as shown) so
the fenced blocks for the commands "npm install", "npm run seed", and "npm run
dev" are each separated by an empty line from surrounding text; also ensure each
```bash fenced block is correctly opened/closed and the README.md ends with a
single trailing newline character.
In `@schema.sql`:
- Around line 74-81: The three RLS policies admin_read_all_appointments,
admin_read_all_patients, and admin_read_all_doctors are overly permissive (USING
(true)); change each policy to require an admin role claim (e.g. USING
(current_setting('jwt.claims.role', true) = 'admin' ) or the equivalent
auth.role() check your stack provides) or restrict them to the service role
only, and add a comment above each policy indicating it is demo-only and must be
replaced with role-based or service-role checks in production.
- Around line 74-81: Add a new row-level security policy named
"admin_read_all_slots" on the slots table that allows SELECT unconditionally
(USING (true)) so the admin dashboard can query slots without requiring an
authenticated session; locate the slots table policies near the existing
"admin_read_all_appointments", "admin_read_all_patients", and
"admin_read_all_doctors" policies and add the new "admin_read_all_slots" policy
analogous to those, ensuring the policy name is exactly admin_read_all_slots and
it uses FOR SELECT USING (true).
- Around line 72-74: Remove the extra blank lines before the CREATE POLICY
statement so there are no consecutive empty lines between policy definitions;
specifically, ensure the CREATE POLICY "admin_read_all_appointments" ON
appointments appears immediately after the prior policy block (or a single blank
line) to satisfy static analysis and keep the schema tidy.
In `@tests/integration.test.ts`:
- Around line 138-168: Test 2 is coupled to Test 1 via shared variables
testAppointmentId and testSlotId; make it independent by creating its own slot
and appointment within this test (or in a dedicated beforeEach for this
describe) and use local variables instead of relying on
testAppointmentId/testSlotId set elsewhere; specifically, insert calls to create
a new slot (and capture its id) and create a new appointment tied to that slot
(capture its appointment id) before performing the
cancellation/update/assertions so the update/query operations in this test
target those locally-created ids.
- Around line 90-133: The test in describe("Integration Test 1: Booking Flow")
incorrectly performs direct Supabase inserts/updates (using
supabase.from("appointments") and supabase.from("slots")) instead of exercising
the HTTP endpoints; change the test to call the real endpoints
(/api/appointments/book and /api/appointments/cancel) using your request helper
or fetch/supertest with proper auth headers (use
testPatientId/testDoctorId/testSlotId to obtain or mint a valid bearer
cookie/token), assert HTTP status and response body (including error shapes),
verify DB state only as a follow-up assertion, and add a race-condition subtest
that fires two concurrent POSTs to /api/appointments/book and asserts one
succeeds and the other fails (or checks slot conditional update behavior); keep
storing testAppointmentId from the API response for cancellation in subsequent
tests and preserve the existing timeout.
- Around line 39-53: Replace the unsafe non-null assertions for testPatientId,
testDoctorId, and testSlotId with explicit checks and clear failure messages:
after awaiting signInWithPassword, assert authData?.user?.id is defined (e.g.,
using expect(...).toBeDefined() or throw new Error("...")) and then assign
testPatientId from the defined value; do the same for the doctor query result
(doctor?.id) and the slot query result (slot?.id) so tests fail fast with a
clear message if the DB is not seeded. Ensure you reference the existing symbols
testPatientId, testDoctorId, testSlotId, authData, doctor, and slot when adding
the checks and assignments.
In `@tests/setup.ts`:
- Around line 7-11: The env parsing currently uses line.split('=') with
destructuring which drops any '=' characters in the value; change the parsing in
tests/setup.ts to split only on the first '=' (e.g., find the first indexOf('=')
and slice key/value) so values like connection strings/URLs are preserved,
continue to trim key and value, and skip empty lines or comments; alternatively
replace the ad-hoc loader with the dotenv package to get robust handling for
multiline/encoded values.
In `@tests/unit.test.ts`:
- Around line 3-5: The test uses a trivial identity function
isSlotBooked(isBooked) that just returns the input and provides no real
validation; either remove the test or replace it to assert actual booking logic
by invoking the real availability check (e.g., call the function/method that
queries the booking API or DB and assert it applies the .eq("is_booked", false)
filter or the equivalent filter/constraint), and also update the duplicate tests
around the other occurrence (the block referenced at lines 37-45) to use the
same real-slot-availability behavior rather than the identity isSlotBooked
function.
- Line 1: The import statement currently brings in beforeAll and afterAll which
are unused; update the imports in tests/unit.test.ts by removing beforeAll and
afterAll from the named import list (leave describe, it, expect) so only used
symbols are imported and ESLint/Jest warnings are resolved.
🪄 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: ab809ca0-2ea3-465e-90a6-3ffcbd36672b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
README.mdapp/admin/dashboard/page.tsxapp/admin/login/page.tsxapp/admin/txtapp/api/appointments/book/route.tsapp/api/appointments/cancel/route.tsapp/doctor/dashboard/page.tsxapp/page.tsxapp/patient/dashboard/page.tsxjest.config.jspackage.jsonschema.sqltests/integration.test.tstests/setup.tstests/unit.test.ts
| useEffect(() => { | ||
| const isLoggedIn = localStorage.getItem("admin_logged_in"); | ||
| if (!isLoggedIn) { | ||
| router.push("/admin/login"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Client-side localStorage check is trivially bypassable.
Any user can open the browser console, run localStorage.setItem("admin_logged_in", "true"), and access the dashboard. Combined with the anon-key Supabase client, this provides read access to appointments, patients, and doctors data (via the overly-permissive USING (true) RLS policies).
This is acceptable for demo purposes if labeled, but in production the dashboard should validate a signed session server-side.
| setAppointments((apptRes.data as unknown as Appointment[]) ?? []); | ||
| setDoctors((doctorRes.data as Doctor[]) ?? []); | ||
| setPatients((patientRes.data as Patient[]) ?? []); | ||
| setSlots((slotRes.data as unknown as Slot[]) ?? []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unsafe type assertions with as unknown as hide potential type mismatches.
Double casting through unknown bypasses TypeScript's type checking entirely. If the Supabase response shape changes (e.g., nested relations like patients, doctors, slots not matching the Appointment type), this will cause silent runtime failures.
Consider using Supabase's generated types or adding runtime validation.
🤖 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 93 - 96, Replace the unsafe
double-casts (e.g., the use of "as unknown as Appointment[]") by using Supabase
generic typing and/or runtime validation before calling
setAppointments/setDoctors/setPatients/setSlots: call the Supabase query with
the proper generic type (e.g.,
supabase.from<Appointment>('appointments').select(...)) or validate
apptRes.data/doctorRes.data/patientRes.data/slotRes.data with a schema validator
(Zod or similar) and only set state when validation passes; also check
response.error and handle null/undefined data instead of forcing a cast so you
won't silently accept mismatched shapes.
| const supabase = createClient( | ||
| process.env.NEXT_PUBLIC_SUPABASE_URL!, | ||
| process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY! | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reuse the centralized Supabase client from lib/supabase.ts.
A shared client already exists at lib/supabase.ts that initializes with the same environment variables. Using it reduces duplication and ensures consistent configuration.
♻️ Proposed refactor
-import { createClient } from "@supabase/supabase-js";
+import { supabase } from "@/lib/supabase";Then remove the inline createClient call and use supabase directly:
- const supabase = createClient(
- process.env.NEXT_PUBLIC_SUPABASE_URL!,
- process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!
- );
-
const { data: admin, error: adminError } = await supabase🤖 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 20 - 23, Replace the inline
createClient(...) call by importing and using the shared Supabase client
exported from the centralized module (the exported symbol is supabase) instead
of creating a new client; remove the local const supabase = createClient(...)
declaration and add an import for the shared supabase client at the top of the
file, then reference that imported supabase in all places where the local
variable was used to ensure consistent configuration.
| const { data: admin, error: adminError } = await supabase | ||
| .from("system_admins") | ||
| .select("*") | ||
| .eq("email", email) | ||
| .eq("password", password) | ||
| .single(); | ||
|
|
||
| if (adminError || !admin) { | ||
| setError("Invalid email or password."); | ||
| setLoading(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Critical security issue: Plaintext password comparison exposes credentials.
The query compares the password directly in plaintext via .eq("password", password), which:
- Sends the raw password over the wire to Supabase
- Relies on plaintext password storage (confirmed by
schema.sqlseeding'admin123') - Allows anyone with DB read access to see all admin passwords
For a demo/task, this should be explicitly labeled with a // DEMO ONLY comment. For production, use Supabase Auth (auth.signInWithPassword) like the doctor login does (see app/doctor/login/page.tsx lines 22-24), or use hashed password verification server-side.
Based on learnings: "In the Next.js admin area, do not implement authentication/authorization by... comparing passwords in plaintext (no hashing). For admin auth, use a secure, verifiable mechanism instead."
🤖 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 25 - 36, The current admin login
queries Supabase with .eq("password", password) (the
supabase.from("system_admins")... .eq("password", password) call) which compares
plaintext passwords—mark this block as DEMO ONLY with a clear // DEMO ONLY
comment; remove/avoid plaintext password comparison for production and instead
use Supabase Auth (auth.signInWithPassword) as done in app/doctor/login/page.tsx
or perform hashed-password verification server-side (verify hashed password in
an API route or auth service) and stop storing raw passwords; update the logic
around the admin fetch/error handling (the admin/adminError handling and
setError/setLoading usage) to use the chosen secure auth flow.
| describe("Integration Test 1: Booking Flow", () => { | ||
| it("should create an appointment in the database when a valid patient books a slot", async () => { | ||
| // Make sure we have required test data | ||
| expect(testPatientId).toBeDefined(); | ||
| expect(testDoctorId).toBeDefined(); | ||
| expect(testSlotId).toBeDefined(); | ||
|
|
||
| // Insert appointment directly via service role | ||
| const { data: appointment, error } = await supabase | ||
| .from("appointments") | ||
| .insert({ | ||
| patient_id: testPatientId, | ||
| doctor_id: testDoctorId, | ||
| slot_id: testSlotId, | ||
| status: "active", | ||
| }) | ||
| .select() | ||
| .single(); | ||
|
|
||
| expect(error).toBeNull(); | ||
| expect(appointment).not.toBeNull(); | ||
| expect(appointment.status).toBe("active"); | ||
| expect(appointment.patient_id).toBe(testPatientId); | ||
| expect(appointment.doctor_id).toBe(testDoctorId); | ||
|
|
||
| // Mark slot as booked | ||
| await supabase | ||
| .from("slots") | ||
| .update({ is_booked: true }) | ||
| .eq("id", testSlotId); | ||
|
|
||
| // Verify slot is now booked in DB | ||
| const { data: slot } = await supabase | ||
| .from("slots") | ||
| .select("is_booked") | ||
| .eq("id", testSlotId) | ||
| .single(); | ||
|
|
||
| expect(slot?.is_booked).toBe(true); | ||
|
|
||
| // Save appointment ID for next test | ||
| testAppointmentId = appointment.id; | ||
| }, 15000); | ||
| }); |
There was a problem hiding this comment.
Integration tests perform direct DB operations instead of calling API endpoints.
These tests are labeled as "integration tests" for the booking/cancellation flow, but they bypass the actual API routes (/api/appointments/book and /api/appointments/cancel) and instead perform direct Supabase inserts/updates. This means:
- Authentication/authorization logic is not tested
- Input validation is not tested
- Race condition handling (
is_booked: falseconditional update) is not tested - The actual HTTP flow and error responses are not verified
True integration tests should make HTTP requests to the endpoints and verify responses.
Would you like me to generate proper integration tests that call the actual API endpoints with authentication headers?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration.test.ts` around lines 90 - 133, The test in
describe("Integration Test 1: Booking Flow") incorrectly performs direct
Supabase inserts/updates (using supabase.from("appointments") and
supabase.from("slots")) instead of exercising the HTTP endpoints; change the
test to call the real endpoints (/api/appointments/book and
/api/appointments/cancel) using your request helper or fetch/supertest with
proper auth headers (use testPatientId/testDoctorId/testSlotId to obtain or mint
a valid bearer cookie/token), assert HTTP status and response body (including
error shapes), verify DB state only as a follow-up assertion, and add a
race-condition subtest that fires two concurrent POSTs to /api/appointments/book
and asserts one succeeds and the other fails (or checks slot conditional update
behavior); keep storing testAppointmentId from the API response for cancellation
in subsequent tests and preserve the existing timeout.
| describe("Integration Test 2: Cancellation Flow", () => { | ||
| it("should update appointment status to cancelled in the database", async () => { | ||
| expect(testAppointmentId).toBeDefined(); | ||
|
|
||
| // Cancel the appointment | ||
| const { data: updated, error } = await supabase | ||
| .from("appointments") | ||
| .update({ status: "cancelled" }) | ||
| .eq("id", testAppointmentId) | ||
| .select() | ||
| .single(); | ||
|
|
||
| expect(error).toBeNull(); | ||
| expect(updated).not.toBeNull(); | ||
| expect(updated.status).toBe("cancelled"); | ||
|
|
||
| // Free up the slot | ||
| await supabase | ||
| .from("slots") | ||
| .update({ is_booked: false }) | ||
| .eq("id", testSlotId); | ||
|
|
||
| // Verify slot is available again in DB | ||
| const { data: slot } = await supabase | ||
| .from("slots") | ||
| .select("is_booked") | ||
| .eq("id", testSlotId) | ||
| .single(); | ||
|
|
||
| expect(slot?.is_booked).toBe(false); | ||
| }, 15000); |
There was a problem hiding this comment.
Test 2 depends on state from Test 1, coupling tests together.
Test 2 relies on testAppointmentId being set by Test 1. If Test 1 fails or tests run in isolation/parallel, Test 2 will fail with an unclear error. Each test should set up its own required state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration.test.ts` around lines 138 - 168, Test 2 is coupled to Test
1 via shared variables testAppointmentId and testSlotId; make it independent by
creating its own slot and appointment within this test (or in a dedicated
beforeEach for this describe) and use local variables instead of relying on
testAppointmentId/testSlotId set elsewhere; specifically, insert calls to create
a new slot (and capture its id) and create a new appointment tied to that slot
(capture its appointment id) before performing the
cancellation/update/assertions so the update/query operations in this test
target those locally-created ids.
| envContent.split('\n').forEach((line) => { | ||
| const [key, value] = line.split('='); | ||
| if (key && value) { | ||
| process.env[key.trim()] = value.trim(); | ||
| } |
There was a problem hiding this comment.
Env parsing truncates values containing = characters.
The line.split('=') with array destructuring [key, value] only captures the portion before the first =. Connection strings and URLs commonly contain multiple = characters (e.g., DATABASE_URL=postgres://host/db?opt=val), which would be silently truncated.
🔧 Proposed fix to handle values with `=` characters
envContent.split('\n').forEach((line) => {
- const [key, value] = line.split('=');
- if (key && value) {
- process.env[key.trim()] = value.trim();
+ const trimmedLine = line.trim();
+ // Skip empty lines and comments
+ if (!trimmedLine || trimmedLine.startsWith('#')) return;
+ const eqIndex = trimmedLine.indexOf('=');
+ if (eqIndex === -1) return;
+ const key = trimmedLine.slice(0, eqIndex).trim();
+ const value = trimmedLine.slice(eqIndex + 1).trim();
+ if (key) {
+ process.env[key] = value;
}
});Alternatively, consider using the dotenv package which handles all edge cases reliably.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/setup.ts` around lines 7 - 11, The env parsing currently uses
line.split('=') with destructuring which drops any '=' characters in the value;
change the parsing in tests/setup.ts to split only on the first '=' (e.g., find
the first indexOf('=') and slice key/value) so values like connection
strings/URLs are preserved, continue to trim key and value, and skip empty lines
or comments; alternatively replace the ad-hoc loader with the dotenv package to
get robust handling for multiline/encoded values.
| function isSlotBooked(isBooked: boolean): boolean { | ||
| return isBooked; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test 1 validates an identity function with no business logic.
isSlotBooked(isBooked) simply returns isBooked. The tests assert isSlotBooked(true) === true and isSlotBooked(false) === false, which don't validate any real booking constraint logic. Consider either:
- Removing this test as it provides no value, or
- Refactoring to test the actual slot availability check from the booking API (e.g., verifying the
.eq("is_booked", false)filter logic)
Also applies to: 37-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit.test.ts` around lines 3 - 5, The test uses a trivial identity
function isSlotBooked(isBooked) that just returns the input and provides no real
validation; either remove the test or replace it to assert actual booking logic
by invoking the real availability check (e.g., call the function/method that
queries the booking API or DB and assert it applies the .eq("is_booked", false)
filter or the equivalent filter/constraint), and also update the duplicate tests
around the other occurrence (the block referenced at lines 37-45) to use the
same real-slot-availability behavior rather than the identity isSlotBooked
function.
1 participant
This PR completes the backend functionality for the appointment booking system as part of the HealthPilot.ai assessment. It includes the two core API routes, a system admin module, a full test suite, and race condition fix.
Changes
API Routes
POST /api/appointments/bookPOST /api/appointments/cancelAdmin Module
/admin/loginwith credentials authenticated against the seeded admin user/admin/dashboardshowing:Tests (
/tests/)Unit Tests (4)
Integration Tests (2)
Race Condition Handling
Concurrent booking requests on the same slot are handled at the database level using locking. Only one request succeeds; the other receives a clear conflict error. This prevents double-booking under simultaneous load and was added beyond the original spec.
Checklist
POST /api/appointments/bookimplementedPOST /api/appointments/cancelimplementedSummary by CodeRabbit
New Features
Documentation
Tests