Divyashree v/develop#7
Open
divya280 wants to merge 3 commits intobotcode-com:mainfrom
Open
Conversation
…ehensive testing FEATURES: - Implement POST /api/appointments/book endpoint - Implement POST /api/appointments/cancel endpoint - Add system admin login and dashboard - Add comprehensive test coverage (5 unit + 6 integration tests) IMPROVEMENTS: - Fix NOT NULL constraint violation on system_admins.password column - Simplify schema by removing plaintext password storage - Add validators for business rules (duplicate booking, cancellation window, etc.) - Implement role-based access control (patient, doctor, admin) - Add database-level security with RLS policies and RPC functions SECURITY: - Bearer token authentication on API routes - httpOnly cookies for admin sessions - Role-based authorization checks - Transactional operations via stored procedures TESTING: - Unit tests: 5 validators tests - Integration tests: 6 tests covering booking, cancellation, and auth flows - All edge cases covered (duplicate bookings, 1-hour cancellation window, role permissions) CLEANUP: - Update .gitignore to exclude build artifacts and temporary files
WalkthroughThis pull request introduces a complete admin authentication system, implements appointment booking and cancellation endpoints with validation, adds Supabase client auth configuration, creates a Jest testing framework with integration and unit tests, updates the database schema with hashed password authentication and new RPC functions, and enhances documentation with security and API details. Changes
Sequence DiagramssequenceDiagram
actor User as Admin User
participant Client as Login Page
participant API as POST /api/admin/login
participant Supabase as Supabase RPC
participant Auth as Auth Provider
User->>Client: Enter email & password
Client->>API: POST { email, password }
API->>Supabase: RPC authenticate_system_admin()
Supabase->>Auth: Verify hashed password
Auth-->>Supabase: Password hash match
Supabase-->>API: Return admin UUID
API->>API: Set httpOnly admin_session cookie
API-->>Client: { success: true }
Client->>Client: Redirect to /admin/dashboard
User->>Client: View dashboard
sequenceDiagram
actor Patient
participant Client as Patient Dashboard
participant API as POST /api/appointments/book
participant Validator as Validators
participant Supabase as RPC book_appointment
participant DB as Database
Patient->>Client: Select slot & doctor
Client->>Client: Retrieve access token
Client->>API: POST { slotId, doctorId } + Bearer token
API->>API: Authenticate token
API->>Validator: validateSlotAvailable()
API->>Validator: validateSlotInFuture()
API->>Validator: validateNoDuplicateBooking()
Validator-->>API: All validations pass
API->>Supabase: RPC book_appointment()
Supabase->>DB: Update slot is_booked & insert appointment
DB-->>Supabase: Success
Supabase-->>API: Return created appointment
API-->>Client: 201 { appointmentId, status: "active" }
Client->>Client: Reload dashboard
sequenceDiagram
actor User as Patient/Doctor
participant Client as Dashboard
participant API as POST /api/appointments/cancel
participant Validator as Validators
participant Supabase as Update Appointment
participant DB as Database
User->>Client: Click cancel/done button
Client->>Client: Retrieve access token & determine role
Client->>API: POST { appointmentId, action } + Bearer token
API->>API: Authenticate token & verify ownership
API->>Validator: validateCancellable()
API->>Validator: validateCancellationWindow(role)
Validator-->>API: Validations pass
API->>Supabase: Update appointment status
alt action = cancel
Supabase->>DB: Update slot is_booked = false
end
DB-->>Supabase: Success
Supabase-->>API: Return updated appointment
API-->>Client: 200 { status: "cancelled"/"done" }
Client->>Client: Reload dashboard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 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 implements a full-stack appointment booking system with admin and portal dashboards, secure API endpoints, and robust database-level integrity via unique indexes and RPC functions. It also introduces password hashing for admins and a comprehensive test suite. Feedback focuses on replacing standard console error logging in API routes with a structured logging approach to prevent potential information leakage and improve system observability.
| const { data: { user }, error: authError } = await supabaseAdmin.auth.getUser(token); | ||
|
|
||
| if (authError || !user) { | ||
| console.error("book_auth_failed", authError?.message ?? "missing user"); |
There was a problem hiding this comment.
Logging sensitive information or internal error messages directly to the console can lead to information leakage. Consider logging a generic error message and using a structured logging service for debugging.
|
|
||
| const { data: { user }, error: authError } = await supabaseAdmin.auth.getUser(token); | ||
| if (authError || !user) { | ||
| console.error("cancel_auth_failed", authError?.message ?? "missing user"); |
There was a problem hiding this comment.
Logging internal error messages directly to the console can lead to information leakage. Consider logging a generic error message and using a structured logging service for debugging.
| .single(); | ||
|
|
||
| if (slotError || !slot) { | ||
| console.error("book_slot_lookup_failed", slotError?.message ?? "slot not found"); |
There was a problem hiding this comment.
Logging internal error messages directly to the console can lead to information leakage. Consider logging a generic error message and using a structured logging service for debugging.
| .eq("status", "active"); | ||
|
|
||
| if (existingError) { | ||
| console.error("book_existing_lookup_failed", existingError.message); |
There was a problem hiding this comment.
Logging internal error messages directly to the console can lead to information leakage. Consider logging a generic error message and using a structured logging service for debugging.
| }); | ||
|
|
||
| if (rpcError) { | ||
| console.error("book_rpc_failed", rpcError.message); |
There was a problem hiding this comment.
Logging internal error messages directly to the console can lead to information leakage. Consider logging a generic error message and using a structured logging service for debugging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/patient/dashboard/page.tsx (1)
153-170:⚠️ Potential issue | 🟠 MajorClear the in-flight button state in a
finallyblock.If
fetch,res.json(), or the follow-uploadDashboard()throws, the reset at Lines 170 and 202 never runs, and the button stays disabled until a full refresh.Also applies to: 185-202
🤖 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 153 - 170, Wrap the async booking flow that calls fetch("/api/appointments/book"), awaits res.json(), and invokes loadDashboard() inside a try/catch/finally so the "in-flight" UI state is always cleared; specifically move the setBookingSlotId(null) (and any other button/disabled state resets) into a finally block while keeping success/failure handling (setActionMsg) in try/catch, touching the code paths around the fetch call, the res.json() usage, setActionMsg, and loadDashboard so the button state is reset even if fetch, res.json(), or loadDashboard throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 19-24: The .gitignore currently uses broad patterns
(migration-*.sql and migration-*.txt) that will hide versioned migrations;
update the ignore entries so they only match temporary/generated migration files
(for example by moving them under a tmp/ or dist/ directory or by using exact
generated filenames) instead of the general patterns; keep any necessary script
ignores like migrate-*.mjs, apply-migration.mjs, run-migration.mjs but replace
migration-*.sql and migration-*.txt with scoped patterns that target only
transient artifacts (e.g., tmp/migration-*.sql or generated/migration-12345.txt)
or remove them entirely so real schema migration files remain tracked.
In `@app/admin/login/page.tsx`:
- Around line 39-56: The Email and Password labels are not associated with their
inputs; update the label/input pairs so clicking the label focuses the field by
adding matching id and htmlFor attributes. For the Email field, add an id (e.g.,
id="admin-email") to the input with value={email} and set the corresponding
label's htmlFor="admin-email"; do the same for the Password field (e.g.,
id="admin-password") for the input with value={password} and its label's
htmlFor="admin-password". Ensure the onChange handlers setEmail and setPassword
are unchanged.
- Around line 13-31: Wrap the network call and response parsing in a
try/catch/finally inside handleSubmit so failures won't leave loading true: move
the fetch, res.ok check, and await res.json() into a try block, setError(...) in
the catch with a sensible fallback message, and ensure setLoading(false) runs in
finally; specifically update handleSubmit to catch fetch/network errors and JSON
parse errors (when calling fetch and res.json()), use
router.push("/admin/dashboard") only on successful res.ok, and on non-JSON
responses fallback to a generic error string before calling setError.
In `@app/api/admin/login/route.ts`:
- Around line 21-27: The admin session cookie set via
response.cookies.set("admin_session", `admin_${adminId}`, ...) must include the
Secure flag in production; update the call in app/api/admin/login/route.ts (the
response.cookies.set invocation) to add secure: true when NODE_ENV ===
"production" (or using your existing environment check) while keeping httpOnly,
sameSite, maxAge and path unchanged so the cookie is only sent over HTTPS in
prod.
In `@app/api/appointments/book/route.ts`:
- Around line 17-22: The endpoint authenticates but does not enforce that the
caller is a patient; update the authorization after
supabaseAdmin.auth.getUser(token) to check the user's role (e.g., inspect
user.user_metadata.role or user.app_metadata role field used elsewhere) and
return NextResponse.json({ error: "Forbidden" }, { status: 403 }) if the role is
not "patient"; apply the same role check to the other identical block around
lines 80-84 so only callers with role "patient" can invoke the booking RPC path.
- Around line 24-31: Wrap the await req.json() call in a try/catch inside the
route's request handler so malformed JSON is caught and results in a 400
response; on catch, return NextResponse.json({ error: "Invalid JSON" }, {
status: 400 }), and then validate slotId and doctorId as before (referencing the
req.json() call and the existing slotId/doctorId checks and NextResponse usage).
- Around line 86-89: The handler currently returns raw RPC error text
(rpcError.message) to clients; update the error path in the booking route where
rpcError is handled (look for the rpcError branch and the
console.error("book_rpc_failed", ...) / NextResponse.json call) to map known
conflict/error codes to safe, user-facing messages (e.g., "time slot
unavailable" for conflict) and for all other cases return a generic message like
"Unable to complete booking" while still logging the full rpcError server-side;
ensure NextResponse.json sends the mapped safe message and appropriate status
(409 for known conflicts, 500 otherwise) rather than rpcError.message.
In `@app/api/appointments/cancel/route.ts`:
- Around line 57-63: Move the doctor-only authorization check ahead of the
cancellation-window validation: before calling validateCancellationWindow(slot,
role), short-circuit and return NextResponse.json({ error: "Forbidden" }, {
status: 403 }) when action === "done" && role !== "doctor"; keep the existing
try/catch around validateCancellationWindow unchanged for other cases so
non-authorized "done" requests always return 403 instead of 409.
- Around line 65-70: The current two-step cancellation frees the slot separately
and ignores both mutation errors; replace this with a single atomic RPC/stored
procedure (e.g., cancel_appointment) that performs the appointments update and
slot is_booked flip inside a DB transaction, then call it via
supabaseAdmin.rpc('cancel_appointment', { appointment_id: appointmentId,
new_status: action === "cancel" ? "cancelled" : "done" }); check the RPC
response and its error and return non-200 if it fails, removing the separate
supabaseAdmin.from("appointments") / supabaseAdmin.from("slots") calls and any
unchecked mutations (refer to variables appointmentId, action and appt.slot_id
to locate the current logic).
In `@app/doctor/dashboard/page.tsx`:
- Around line 90-94: formattedAppts maps relation arrays without checking
emptiness, causing patients[0]/slots[0] to be undefined and later render invalid
dates; update the mapping in formattedAppts to explicitly check length before
indexing (e.g. use Array.isArray(a.patients) && a.patients.length > 0 ?
a.patients[0] : null and similarly for slots) so that DisplayAppointment
receives null (or a safe default) instead of undefined, and propagate that
normalized value where rendering expects a date to avoid invalid-date output.
- Around line 117-149: The handler handleAction can throw before clearing the
pendingActionId, so wrap the async flow (calls to getAccessToken, fetch,
res.json, and loadDashboard) in a try/catch/finally: in try perform token check,
fetch POST, parse response and call loadDashboard on success; in catch set an
appropriate action message from the caught error; and in finally always call
setPendingActionId(null) so the row is never left disabled. Ensure you still
navigate on missing token (router.push) within the try or before returning, and
update setActionMsg inside catch when needed.
In `@app/patient/dashboard/page.tsx`:
- Around line 93-97: The current slots query (supabase.from("slots") ...
.eq("is_booked", false) .order("start_time")) can return past/expired slots
causing 409s; update the query to filter out past times by adding a condition
comparing slot time to now (e.g., use .gte("start_time", new
Date().toISOString()) or .gt("end_time", new Date().toISOString()) on the same
query) so only future/unexpired slots are returned before rendering slotsData.
In `@package.json`:
- Line 10: The package.json lacks an engines constraint while the "test" script
relies on the --env-file flag (Node >=20.10.0); add an "engines" field to
package.json declaring node >=20.10.0 (or alternatively add an
.nvmrc/.node-version with 20.10.0+ and ensure CI enforces it) so the minimum
Node.js version is explicitly required.
In `@README.md`:
- Around line 13-14: The table-of-contents entries use anchors that don't match
actual headings; update either the TOC links or the headings so they match
exactly (e.g., change the TOC entries "- [Database Schema](`#database-schema`)"
and "- [Testing](`#testing`)" to the correct anchors that correspond to the actual
headings in the README, or rename the headings to "Database Schema" and
"Testing" so the anchors resolve correctly), ensuring the link text and heading
text are identical so the anchors jump to the proper sections.
- Around line 64-90: The unlabeled triple-backtick code fences in README.md
(e.g., the ASCII diagram block shown and the similar block around lines 307-333)
are causing markdownlint warnings; update each fence to include a language tag
such as text (replace ``` with ```text) so the fences are labeled and rendering
is unchanged, ensuring every occurrence of ``` in those diagram sections is
updated to ```text.
In `@schema.sql`:
- Around line 42-52: The migration currently drops the old credential column and
mass-assigns a shared default ('admin123') to NULL hashes; instead preserve the
old column (remove the ALTER TABLE ... DROP COLUMN IF EXISTS password and the
DROP FUNCTION call), backfill password_hash from the existing plaintext/old
column (use the old column value with crypt/gen_salt to populate password_hash
for non-NULL old passwords), only then set ALTER COLUMN password_hash SET NOT
NULL and finally drop the old column (and any seed function) after verifying
backfill; if plaintext isn't available, remove the UPDATE that assigns a shared
default and implement an explicit admin-reset flow (flagged migration or
audit-required reset) rather than seeding a common password.
- Around line 136-139: The schema currently seeds system_admins with a known
credential (email 'admin@test.com' / password_hash derived from
crypt('admin123', gen_salt('bf'))) which makes every deployment insecure; remove
this unconditional INSERT from schema.sql and instead implement a safe bootstrap
path: either require an out-of-band creation step (manual migration or
admin-creation script) or gate the seed behind a deployment-only secret/ENV flag
(e.g., only run when a BOOTSTRAP_ADMIN_TOKEN is present), leaving the
system_admins table empty by default and ensuring any created admin password is
provided securely and not hard-coded.
- Around line 159-219: Lock down the SECURITY DEFINER RPCs by (1) adding a SET
search_path = public; clause at the top of both seed_system_admin and
book_appointment definitions to prevent search_path injection, (2) removing
implicit EXECUTE access by revoking PUBLIC/anon/authenticated execute privileges
and then explicitly GRANT-ing only the intended role(s) permission to call
seed_system_admin and book_appointment, and (3) enforcing caller identity inside
book_appointment (and where appropriate in seed_system_admin) by checking
auth.uid() (or your auth function) against p_patient_id or the admin identity
before performing any UPDATE/INSERT to prevent callers from mutating or creating
records they don’t own. Ensure these checks are done before any FOR
UPDATE/UPDATE/INSERT and keep SECURITY DEFINER only if absolutely necessary
after these controls are in place.
In `@seed.mjs`:
- Around line 48-56: The seeding loop currently discards failures (using
continue) when createUser fails and listUsers() doesn't find a match, leaving
doctorIds/patientIds incomplete and causing later crashes (e.g., bookedSlot.id).
Change the logic in the createUser handling (the block around createUser,
doctorIds, patientIds, and the call to supabase.auth.admin.listUsers) to abort
immediately when you cannot recover an ID: log the full error and either throw
an Error or call process.exit(1) instead of continue; apply the same fix to the
corresponding patient loop (the other block mentioned). Ensure the error message
includes the email and context so the script fails fast with a clear diagnostic.
In `@tests/integration/booking.test.ts`:
- Around line 39-73: The test "successful booking flow" currently performs
cleanup only on the happy path, so if an assertion or network error occurs the
inserted slot/appointment remain; wrap the core test actions (the supabaseAdmin
insert, fetch to POST /api/appointments/book, and assertions) in a try/finally
and move the deletions into the finally block, guarding deletes with optional
checks like slot?.id and appointment?.id so cleanup always runs; apply the same
try/finally pattern to the other affected test block referenced (lines ~75-105)
to ensure seeded data is removed even on failure.
- Around line 5-6: The integration tests hit BASE_URL http://localhost:3000 but
no automation starts the Next.js server, causing ECONNREFUSED; add a test runner
that launches the app before tests by adding a "test:integration" script to
package.json that uses a server lifecycle tool (e.g., start-server-and-test) to
run the Next.js dev/start command, wait for http://localhost:3000, then run your
integration Jest command (or alternatively implement a Jest globalSetup that
programmatically spawns the Next process and waits for the port), and ensure the
new script is installed in CI and any required dependency
(start-server-and-test) is added to devDependencies so tests run reliably.
In `@tests/integration/cancellation.test.ts`:
- Around line 32-55: The test dereferences DB insert/select results (e.g., the
doctor lookup used inside the insert for slots, the resulting slot variable, and
the appt variable from appointments) without guarding for null/undefined; update
the test around the supabaseAdmin calls that produce slot and appt (and the
doctor lookup used to get doctor id) to explicitly check the returned objects
(the .data on the doctor select, the slot from the slots insert, and appt from
the appointments insert) and fail the test with an assertion or descriptive
error if any are null/undefined before using properties like slot.id,
slot.doctor_id, or appt.id, and keep waitForPersistence() calls as-is after
successful assertions so subsequent code won't throw runtime errors.
- Around line 29-81: The test "successful cancellation flow" currently performs
cleanup after assertions which can be skipped if an error is thrown; wrap the
test body (the setup, POST call and assertions) in a try block and move the
teardown deletes (the supabaseAdmin.from(...).delete() calls that remove
appointments and slots) into a finally block so they always run; locate this in
the test function using the test name "successful cancellation flow" and the
variables slot, appt, supabaseAdmin, waitForPersistence and apply the same
try/finally pattern to the other test referenced (lines 83-126) to ensure
consistent teardown.
In `@tests/integration/guards.test.ts`:
- Around line 7-9: The helper waitForPersistence currently sleeps for a fixed
250ms which is flaky; replace it with a deterministic polling helper that
repeatedly queries the DB for the expected row (e.g., call the existing
repository/check function used in these tests or accept a check callback) until
the row appears or a sensible timeout (e.g., 5s) elapses. Implement
waitForPersistence to poll at short intervals (e.g., 50–200ms), resolve when the
DB check returns true, and reject after the timeout so tests fail fast if the
row never appears; remove the setTimeout sleep and use that polling logic inside
waitForPersistence.
- Around line 77-120: The test "patient cannot mark an appointment done" leaves
seeded rows when an assertion or fetch throws; wrap the main test body (creation
of slot/appointment, the fetch call and assertions) in a try/finally and move
the cleanup calls that delete the appointment and slot (the
supabaseAdmin.from(...).delete().eq("id", ... ) lines referencing slot.id and
appointment.id) into the finally block so they always run; apply the same
try/finally pattern to the other affected test block around lines 138-168 in
guards.test.ts.
---
Outside diff comments:
In `@app/patient/dashboard/page.tsx`:
- Around line 153-170: Wrap the async booking flow that calls
fetch("/api/appointments/book"), awaits res.json(), and invokes loadDashboard()
inside a try/catch/finally so the "in-flight" UI state is always cleared;
specifically move the setBookingSlotId(null) (and any other button/disabled
state resets) into a finally block while keeping success/failure handling
(setActionMsg) in try/catch, touching the code paths around the fetch call, the
res.json() usage, setActionMsg, and loadDashboard so the button state is reset
even if fetch, res.json(), or loadDashboard throws.
🪄 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: 0f6968b6-df00-4ed2-ba43-32453f3299f0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.gitignoreREADME.mdapp/admin/dashboard/page.tsxapp/admin/login/page.tsxapp/api/admin/login/route.tsapp/api/appointments/book/route.tsapp/api/appointments/cancel/route.tsapp/doctor/dashboard/page.tsxapp/patient/dashboard/page.tsxinstrumentation.tsjest.config.tslib/supabase.tslib/supabaseAdmin.tslib/validators.tspackage.jsonschema.sqlseed.mjstests/integration/booking.test.tstests/integration/cancellation.test.tstests/integration/guards.test.tstests/unit/validators.test.ts
Comment on lines
+19
to
+24
| # Temporary files | ||
| migrate-*.mjs | ||
| apply-migration.mjs | ||
| run-migration.mjs | ||
| migration-*.sql | ||
| migration-*.txt |
There was a problem hiding this comment.
Don't ignore versioned SQL migrations by filename pattern.
migration-*.sql and migration-*.txt are broad enough to hide real schema migrations, which makes database changes easy to ship without source control. Scope these ignores to a temp directory or to exact generated filenames instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 19 - 24, The .gitignore currently uses broad
patterns (migration-*.sql and migration-*.txt) that will hide versioned
migrations; update the ignore entries so they only match temporary/generated
migration files (for example by moving them under a tmp/ or dist/ directory or
by using exact generated filenames) instead of the general patterns; keep any
necessary script ignores like migrate-*.mjs, apply-migration.mjs,
run-migration.mjs but replace migration-*.sql and migration-*.txt with scoped
patterns that target only transient artifacts (e.g., tmp/migration-*.sql or
generated/migration-12345.txt) or remove them entirely so real schema migration
files remain tracked.
Comment on lines
+13
to
+31
| async function handleSubmit(e: React.FormEvent) { | ||
| e.preventDefault(); | ||
| setError(""); | ||
| setLoading(true); | ||
|
|
||
| const res = await fetch("/api/admin/login", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ email, password }), | ||
| }); | ||
|
|
||
| if (res.ok) { | ||
| router.push("/admin/dashboard"); | ||
| } else { | ||
| const data = await res.json(); | ||
| setError(data.error || "Login failed"); | ||
| } | ||
| setLoading(false); | ||
| } |
There was a problem hiding this comment.
Handle network/parsing failures with try/catch/finally.
A failed request or non-JSON error response will currently throw, leave loading stuck at true, and strand the form until refresh.
Suggested change
async function handleSubmit(e: React.FormEvent) {
e.preventDefault();
setError("");
setLoading(true);
-
- const res = await fetch("/api/admin/login", {
- method: "POST",
- headers: { "Content-Type": "application/json" },
- body: JSON.stringify({ email, password }),
- });
-
- if (res.ok) {
- router.push("/admin/dashboard");
- } else {
- const data = await res.json();
- setError(data.error || "Login failed");
- }
- setLoading(false);
+ try {
+ const res = await fetch("/api/admin/login", {
+ method: "POST",
+ headers: { "Content-Type": "application/json" },
+ body: JSON.stringify({ email, password }),
+ });
+
+ if (res.ok) {
+ router.push("/admin/dashboard");
+ return;
+ }
+
+ const data = await res.json().catch(() => null);
+ setError(data?.error || "Login failed");
+ } catch {
+ setError("Unable to reach the server");
+ } finally {
+ setLoading(false);
+ }
}🤖 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 13 - 31, Wrap the network call and
response parsing in a try/catch/finally inside handleSubmit so failures won't
leave loading true: move the fetch, res.ok check, and await res.json() into a
try block, setError(...) in the catch with a sensible fallback message, and
ensure setLoading(false) runs in finally; specifically update handleSubmit to
catch fetch/network errors and JSON parse errors (when calling fetch and
res.json()), use router.push("/admin/dashboard") only on successful res.ok, and
on non-JSON responses fallback to a generic error string before calling
setError.
Comment on lines
+39
to
+56
| <label className="mb-1 block text-sm font-medium text-gray-700">Email</label> | ||
| <input | ||
| type="email" | ||
| value={email} | ||
| onChange={(e) => setEmail(e.target.value)} | ||
| required | ||
| className="w-full rounded-lg border px-3 py-2 focus:outline-none focus:ring-2 focus:ring-blue-500" | ||
| /> | ||
| </div> | ||
| <div> | ||
| <label className="mb-1 block text-sm font-medium text-gray-700">Password</label> | ||
| <input | ||
| type="password" | ||
| value={password} | ||
| onChange={(e) => setPassword(e.target.value)} | ||
| required | ||
| className="w-full rounded-lg border px-3 py-2 focus:outline-none focus:ring-2 focus:ring-blue-500" | ||
| /> |
There was a problem hiding this comment.
Associate each label with its input.
These labels are only visual text right now, so clicking them will not focus the field and some assistive tech loses the relationship. Add id/htmlFor pairs for both inputs.
🤖 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 39 - 56, The Email and Password labels
are not associated with their inputs; update the label/input pairs so clicking
the label focuses the field by adding matching id and htmlFor attributes. For
the Email field, add an id (e.g., id="admin-email") to the input with
value={email} and set the corresponding label's htmlFor="admin-email"; do the
same for the Password field (e.g., id="admin-password") for the input with
value={password} and its label's htmlFor="admin-password". Ensure the onChange
handlers setEmail and setPassword are unchanged.
Comment on lines
+21
to
+27
| const response = NextResponse.json({ success: true }, { status: 200 }); | ||
| response.cookies.set("admin_session", `admin_${adminId}`, { | ||
| httpOnly: true, | ||
| sameSite: "lax", | ||
| maxAge: 86400, | ||
| path: "/", | ||
| }); |
There was a problem hiding this comment.
Mark the admin session cookie as Secure in production.
This is an admin auth cookie. Without the Secure attribute, browsers will also send it over plain HTTP if the site is reached that way.
Proposed fix
response.cookies.set("admin_session", `admin_${adminId}`, {
httpOnly: true,
+ secure: process.env.NODE_ENV === "production",
sameSite: "lax",
maxAge: 86400,
path: "/",
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/admin/login/route.ts` around lines 21 - 27, The admin session cookie
set via response.cookies.set("admin_session", `admin_${adminId}`, ...) must
include the Secure flag in production; update the call in
app/api/admin/login/route.ts (the response.cookies.set invocation) to add
secure: true when NODE_ENV === "production" (or using your existing environment
check) while keeping httpOnly, sameSite, maxAge and path unchanged so the cookie
is only sent over HTTPS in prod.
Comment on lines
+17
to
+22
| const { data: { user }, error: authError } = await supabaseAdmin.auth.getUser(token); | ||
|
|
||
| if (authError || !user) { | ||
| console.error("book_auth_failed", authError?.message ?? "missing user"); | ||
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } |
There was a problem hiding this comment.
Enforce patient-role authorization before booking mutation.
This endpoint authenticates the caller but does not authorize role. Any valid authenticated token can reach booking RPC path, which weakens RBAC guarantees.
Also applies to: 80-84
🤖 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 17 - 22, The endpoint
authenticates but does not enforce that the caller is a patient; update the
authorization after supabaseAdmin.auth.getUser(token) to check the user's role
(e.g., inspect user.user_metadata.role or user.app_metadata role field used
elsewhere) and return NextResponse.json({ error: "Forbidden" }, { status: 403 })
if the role is not "patient"; apply the same role check to the other identical
block around lines 80-84 so only callers with role "patient" can invoke the
booking RPC path.
Comment on lines
+39
to
+73
| test("successful booking flow", async () => { | ||
| const startTime = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString(); | ||
| const endTime = new Date(Date.now() + 25 * 60 * 60 * 1000).toISOString(); | ||
|
|
||
| const { data: slot } = await supabaseAdmin | ||
| .from("slots") | ||
| .insert({ | ||
| doctor_id: doctorId, | ||
| start_time: startTime, | ||
| end_time: endTime, | ||
| is_booked: false, | ||
| }) | ||
| .select() | ||
| .single(); | ||
|
|
||
| if (!slot) throw new Error("Failed to create slot for booking test"); | ||
| await waitForPersistence(); | ||
|
|
||
| const res = await fetch(`${BASE_URL}/api/appointments/book`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| body: JSON.stringify({ slotId: slot.id, doctorId }), | ||
| }); | ||
|
|
||
| expect(res.status).toBe(201); | ||
| const appointment = await res.json(); | ||
| expect(appointment.status).toBe("active"); | ||
|
|
||
| // Cleanup | ||
| await supabaseAdmin.from("appointments").delete().eq("id", appointment.id); | ||
| await supabaseAdmin.from("slots").delete().eq("id", slot.id); | ||
| }); |
There was a problem hiding this comment.
Put seeded test data cleanup in finally.
Both tests delete rows only on the happy path. A failed assertion or network error leaves appointments/slots behind, which can poison later runs with duplicate-booking or slot-state conflicts.
Also applies to: 75-105
🤖 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 39 - 73, The test "successful
booking flow" currently performs cleanup only on the happy path, so if an
assertion or network error occurs the inserted slot/appointment remain; wrap the
core test actions (the supabaseAdmin insert, fetch to POST
/api/appointments/book, and assertions) in a try/finally and move the deletions
into the finally block, guarding deletes with optional checks like slot?.id and
appointment?.id so cleanup always runs; apply the same try/finally pattern to
the other affected test block referenced (lines ~75-105) to ensure seeded data
is removed even on failure.
Comment on lines
+29
to
+81
| test("successful cancellation flow", async () => { | ||
| // 1. Create a slot far in the future | ||
| const futureDate = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString(); | ||
| const { data: slot } = await supabaseAdmin | ||
| .from("slots") | ||
| .insert({ | ||
| doctor_id: (await supabaseAdmin.from("doctors").select("id").limit(1).single()).data?.id, | ||
| start_time: futureDate, | ||
| end_time: futureDate, // simplification | ||
| is_booked: true, | ||
| }) | ||
| .select() | ||
| .single(); | ||
| await waitForPersistence(); | ||
|
|
||
| // 2. Create appointment | ||
| const { data: appt } = await supabaseAdmin | ||
| .from("appointments") | ||
| .insert({ | ||
| patient_id: patientId, | ||
| doctor_id: slot.doctor_id, | ||
| slot_id: slot.id, | ||
| status: "active", | ||
| }) | ||
| .select() | ||
| .single(); | ||
| await waitForPersistence(); | ||
|
|
||
| // 3. POST to cancel | ||
| const res = await fetch(`${BASE_URL}/api/appointments/cancel`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| body: JSON.stringify({ appointmentId: appt.id, action: "cancel" }), | ||
| }); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| const updatedAppt = await res.json(); | ||
| expect(updatedAppt.status).toBe("cancelled"); | ||
|
|
||
| const { data: updatedSlot } = await supabaseAdmin | ||
| .from("slots") | ||
| .select("is_booked") | ||
| .eq("id", slot.id) | ||
| .single(); | ||
| expect(updatedSlot?.is_booked).toBe(false); | ||
|
|
||
| // Cleanup | ||
| await supabaseAdmin.from("appointments").delete().eq("id", appt.id); | ||
| await supabaseAdmin.from("slots").delete().eq("id", slot.id); | ||
| }); |
There was a problem hiding this comment.
Use try/finally so teardown always runs.
Current cleanup is skipped if request/assertion/setup throws, which can leave test residue and cause cascading failures.
Also applies to: 83-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/cancellation.test.ts` around lines 29 - 81, The test
"successful cancellation flow" currently performs cleanup after assertions which
can be skipped if an error is thrown; wrap the test body (the setup, POST call
and assertions) in a try block and move the teardown deletes (the
supabaseAdmin.from(...).delete() calls that remove appointments and slots) into
a finally block so they always run; locate this in the test function using the
test name "successful cancellation flow" and the variables slot, appt,
supabaseAdmin, waitForPersistence and apply the same try/finally pattern to the
other test referenced (lines 83-126) to ensure consistent teardown.
Comment on lines
+32
to
+55
| const { data: slot } = await supabaseAdmin | ||
| .from("slots") | ||
| .insert({ | ||
| doctor_id: (await supabaseAdmin.from("doctors").select("id").limit(1).single()).data?.id, | ||
| start_time: futureDate, | ||
| end_time: futureDate, // simplification | ||
| is_booked: true, | ||
| }) | ||
| .select() | ||
| .single(); | ||
| await waitForPersistence(); | ||
|
|
||
| // 2. Create appointment | ||
| const { data: appt } = await supabaseAdmin | ||
| .from("appointments") | ||
| .insert({ | ||
| patient_id: patientId, | ||
| doctor_id: slot.doctor_id, | ||
| slot_id: slot.id, | ||
| status: "active", | ||
| }) | ||
| .select() | ||
| .single(); | ||
| await waitForPersistence(); |
There was a problem hiding this comment.
Guard doctor/slot/appointment creation before dereferencing.
Lines 49-50 and 103-104 assume inserts always succeed. If doctor lookup at Line 35/89 returns null, these tests fail with runtime errors instead of meaningful assertions.
Suggested fix
-const { data: slot } = await supabaseAdmin
+const { data: doctorRow, error: doctorError } = await supabaseAdmin
+ .from("doctors")
+ .select("id")
+ .limit(1)
+ .single();
+if (doctorError || !doctorRow?.id) throw new Error("No doctor found for test setup");
+
+const { data: slot, error: slotError } = await supabaseAdmin
.from("slots")
.insert({
- doctor_id: (await supabaseAdmin.from("doctors").select("id").limit(1).single()).data?.id,
+ doctor_id: doctorRow.id,
// ...
})
.select()
.single();
+if (slotError || !slot) throw new Error("Failed to create slot");
-const { data: appt } = await supabaseAdmin
+const { data: appt, error: apptError } = await supabaseAdmin
.from("appointments")
.insert({
// ...
})
.select()
.single();
+if (apptError || !appt) throw new Error("Failed to create appointment");Also applies to: 86-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/cancellation.test.ts` around lines 32 - 55, The test
dereferences DB insert/select results (e.g., the doctor lookup used inside the
insert for slots, the resulting slot variable, and the appt variable from
appointments) without guarding for null/undefined; update the test around the
supabaseAdmin calls that produce slot and appt (and the doctor lookup used to
get doctor id) to explicitly check the returned objects (the .data on the doctor
select, the slot from the slots insert, and appt from the appointments insert)
and fail the test with an assertion or descriptive error if any are
null/undefined before using properties like slot.id, slot.doctor_id, or appt.id,
and keep waitForPersistence() calls as-is after successful assertions so
subsequent code won't throw runtime errors.
Comment on lines
+7
to
+9
| async function waitForPersistence(): Promise<void> { | ||
| await new Promise((resolve) => setTimeout(resolve, 250)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace fixed sleep with deterministic DB polling.
Line 8 uses a hardcoded 250ms delay, which makes these tests timing-sensitive and flaky in CI. Prefer polling for the inserted row (or awaiting a known DB condition) before proceeding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/guards.test.ts` around lines 7 - 9, The helper
waitForPersistence currently sleeps for a fixed 250ms which is flaky; replace it
with a deterministic polling helper that repeatedly queries the DB for the
expected row (e.g., call the existing repository/check function used in these
tests or accept a check callback) until the row appears or a sensible timeout
(e.g., 5s) elapses. Implement waitForPersistence to poll at short intervals
(e.g., 50–200ms), resolve when the DB check returns true, and reject after the
timeout so tests fail fast if the row never appears; remove the setTimeout sleep
and use that polling logic inside waitForPersistence.
Comment on lines
+77
to
+120
| test("patient cannot mark an appointment done", async () => { | ||
| const futureDate = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString(); | ||
| const { data: slot } = await supabaseAdmin | ||
| .from("slots") | ||
| .insert({ | ||
| doctor_id: doctorId, | ||
| start_time: futureDate, | ||
| end_time: new Date(Date.now() + 25 * 60 * 60 * 1000).toISOString(), | ||
| is_booked: true, | ||
| }) | ||
| .select() | ||
| .single(); | ||
|
|
||
| if (!slot) throw new Error("Failed to create slot for forbidden-action test"); | ||
| await waitForPersistence(); | ||
|
|
||
| const { data: appointment } = await supabaseAdmin | ||
| .from("appointments") | ||
| .insert({ | ||
| patient_id: patientId, | ||
| doctor_id: doctorId, | ||
| slot_id: slot.id, | ||
| status: "active", | ||
| }) | ||
| .select() | ||
| .single(); | ||
| await waitForPersistence(); | ||
|
|
||
| if (!appointment) throw new Error("Failed to create appointment for forbidden-action test"); | ||
|
|
||
| const res = await fetch(`${BASE_URL}/api/appointments/cancel`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${patientToken}`, | ||
| }, | ||
| body: JSON.stringify({ appointmentId: appointment.id, action: "done" }), | ||
| }); | ||
|
|
||
| expect(res.status).toBe(403); | ||
|
|
||
| await supabaseAdmin.from("appointments").delete().eq("id", appointment.id); | ||
| await supabaseAdmin.from("slots").delete().eq("id", slot.id); | ||
| }); |
There was a problem hiding this comment.
Protect seeded-data cleanup with try/finally.
Cleanup at Line 118-119 and Line 167 runs only on the happy path. If fetch/assertions throw, rows remain and can corrupt later tests.
Suggested fix
- test("patient cannot mark an appointment done", async () => {
+ test("patient cannot mark an appointment done", async () => {
+ let slotId: string | null = null;
+ let appointmentId: string | null = null;
+ try {
// create slot + appointment ...
- const res = await fetch(`${BASE_URL}/api/appointments/cancel`, {
+ const res = await fetch(`${BASE_URL}/api/appointments/cancel`, {
// ...
});
expect(res.status).toBe(403);
-
- await supabaseAdmin.from("appointments").delete().eq("id", appointment.id);
- await supabaseAdmin.from("slots").delete().eq("id", slot.id);
+ } finally {
+ if (appointmentId) await supabaseAdmin.from("appointments").delete().eq("id", appointmentId);
+ if (slotId) await supabaseAdmin.from("slots").delete().eq("id", slotId);
+ }
});Also applies to: 138-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/guards.test.ts` around lines 77 - 120, The test "patient
cannot mark an appointment done" leaves seeded rows when an assertion or fetch
throws; wrap the main test body (creation of slot/appointment, the fetch call
and assertions) in a try/finally and move the cleanup calls that delete the
appointment and slot (the supabaseAdmin.from(...).delete().eq("id", ... ) lines
referencing slot.id and appointment.id) into the finally block so they always
run; apply the same try/finally pattern to the other affected test block around
lines 138-168 in guards.test.ts.
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.
Project Overview: Appointment Booking System
This project is a production-ready, full-stack appointment booking platform designed for clinics and healthcare providers. It provides a seamless interface for patients to book slots, doctors to manage their schedules, and admins to oversee the entire system.
Tech Stack
Frontend/API: Next.js 14 (App Router) & TypeScript.
Database & Auth: Supabase (PostgreSQL, Auth, Row-Level Security).
Business Logic: Centralized validators and PostgreSQL RPC functions for atomic operations.
Testing: Jest for Unit and Integration testing.
Core Features
Patient Portal: Interactive slot selection, real-time availability, and a 1-hour cancellation window policy.
Doctor Portal: Dashboard to track upcoming appointments and mark them as completed/done.
Admin Module: Secure administrative login with session-based tracking to view all system activity.
Robust Booking Engine: Prevents double-bookings and race conditions using database-level locking and constraints.
Security & Architecture
RBAC (Role-Based Access Control): Strictly enforced rules for Patients, Doctors, and Admins.
Data Integrity: Uses Supabase RLS (Row-Level Security) to ensure users can only access their own data.
API Documentation: Detailed endpoint references and sequence diagrams for all major workflows (Booking, Cancellation, Admin Login).
Quality Assurance
Integration Tests: Full-flow testing for booking and cancellation logic.
Security Guards: Tests to ensure unauthorized users cannot access sensitive API routes.
Validation: Strict business rule enforcement (e.g., no booking in the past, no duplicate active appointments)
Summary by CodeRabbit
New Features
Documentation
Tests