Illia Krauchenia - #10
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. Pull Request Developer GuidanceAction Required: Please review and fix any Critical or High severity findings identified above.
Need help or have questions? Reach out to the Security team on Slack: #rnd-wiz |
There was a problem hiding this comment.
Pull request overview
Implements an end-to-end reservations workflow spanning guest booking, staff-only reservation visibility, and a same-day check-in flow, adding both backend API endpoints/validation and corresponding UI pages.
Changes:
- Implemented real reservation creation in the UI and API (validation, guest auto-create, overlap prevention).
- Added staff login/session handling plus a staff UI to view upcoming/today reservations and perform check-in.
- Added supporting infrastructure (toast helper, routing, new API models/exceptions, room number validation).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/utils/toasts.tsx | Refactors info toast into a reusable non-hook helper. |
| ui/src/staff/api.ts | Adds staff ky client + react-query hooks for staff session, reservations list, and check-in. |
| ui/src/staff/StaffPage.tsx | New staff page for shared-code login, reservation listing/filtering, and check-in. |
| ui/src/router.tsx | Registers /staff route. |
| ui/src/reservations/api.ts | Switches booking from stubbed Promise to real POST call to the API. |
| ui/src/reservations/ReservationPage.tsx | Adds booking error handling + controls dialog open state. |
| ui/src/reservations/BookingDetailsModal.tsx | Adds client-side validation and async submission/disabled UI state. |
| ui/src/LandingPage.tsx | Wires “Login” card to staff route. |
| api/Repositories/ReservationRepository.cs | Implements reservation create/upcoming list/check-in, validation, overlap checks, and guest creation. |
| api/Models/Room.cs | Adds room number validation and enforces it in conversion. |
| api/Models/Errors/ReservationConflictException.cs | New exception type for overlap conflicts. |
| api/Models/Errors/InvalidReservationException.cs | New exception type for validation failures. |
| api/Models/Errors/InvalidCheckInException.cs | New exception type for invalid check-in attempts. |
| api/Models/CheckInReservationRequest.cs | Adds request model for check-in endpoint. |
| api/Controllers/StaffController.cs | Moves shared staff auth logic into a base controller; adjusts cookie options. |
| api/Controllers/StaffAccessController.cs | New base controller for staff cookie authorization checks. |
| api/Controllers/ReservationController.cs | Restricts reservation reads/check-in to staff; adds check-in endpoint; improves error mapping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public async Task<IEnumerable<Reservation>> GetUpcomingReservations() | ||
| { | ||
| var today = DateTime.Today; | ||
| var reservations = await _db.QueryAsync<ReservationDb>( | ||
| @" | ||
| SELECT * | ||
| FROM Reservations | ||
| WHERE End > @today | ||
| ORDER BY Start ASC, RoomNumber ASC; | ||
| ", | ||
| new { today } |
There was a problem hiding this comment.
GetUpcomingReservations uses DateTime.Today for filtering, but reservation Start/End values originate from UTC ISO strings (toISOString()), so “today” can differ between server local time and stored UTC timestamps. This can incorrectly hide/show reservations around day boundaries. Use a consistent basis (prefer date-only values, or use DateTime.UtcNow.Date with UTC-normalized stored values) for both filtering and validation.
| await EnsureNoReservationConflict(newReservation); | ||
| await EnsureGuestExists(newReservation.GuestEmail); | ||
|
|
||
| var createdReservation = await _db.QuerySingleAsync<ReservationDb>( | ||
| @" | ||
| INSERT INTO Reservations( | ||
| Id, | ||
| GuestEmail, | ||
| RoomNumber, | ||
| Start, | ||
| End, | ||
| CheckedIn, | ||
| CheckedOut | ||
| ) | ||
| VALUES( | ||
| @Id, | ||
| @GuestEmail, | ||
| @RoomNumber, | ||
| @Start, | ||
| @End, | ||
| @CheckedIn, | ||
| @CheckedOut | ||
| ) | ||
| RETURNING *; | ||
| ", | ||
| new ReservationDb(newReservation) | ||
| ); | ||
|
|
||
| return createdReservation.ToDomain(); |
There was a problem hiding this comment.
The overlap check (EnsureNoReservationConflict) runs before the INSERT without a transaction/lock, so two concurrent requests can both pass the check and then both insert, resulting in double-booking despite the validation. To make this guarantee robust, wrap the conflict check + insert in a single transaction with an appropriate lock level (e.g., SQLite BEGIN IMMEDIATE) or enforce it at the DB level (constraint/index + retry).
| await EnsureNoReservationConflict(newReservation); | |
| await EnsureGuestExists(newReservation.GuestEmail); | |
| var createdReservation = await _db.QuerySingleAsync<ReservationDb>( | |
| @" | |
| INSERT INTO Reservations( | |
| Id, | |
| GuestEmail, | |
| RoomNumber, | |
| Start, | |
| End, | |
| CheckedIn, | |
| CheckedOut | |
| ) | |
| VALUES( | |
| @Id, | |
| @GuestEmail, | |
| @RoomNumber, | |
| @Start, | |
| @End, | |
| @CheckedIn, | |
| @CheckedOut | |
| ) | |
| RETURNING *; | |
| ", | |
| new ReservationDb(newReservation) | |
| ); | |
| return createdReservation.ToDomain(); | |
| await EnsureGuestExists(newReservation.GuestEmail); | |
| // Ensure that the overlap check and insert are performed atomically to avoid | |
| // double-booking under concurrent requests. | |
| if (_db is SqliteConnection sqliteConnection) | |
| { | |
| // For SQLite, use BEGIN IMMEDIATE to acquire a write lock before checking | |
| // for conflicts and inserting. | |
| await sqliteConnection.ExecuteAsync("BEGIN IMMEDIATE;"); | |
| try | |
| { | |
| await EnsureNoReservationConflict(newReservation); | |
| var createdReservation = await sqliteConnection.QuerySingleAsync<ReservationDb>( | |
| @" | |
| INSERT INTO Reservations( | |
| Id, | |
| GuestEmail, | |
| RoomNumber, | |
| Start, | |
| End, | |
| CheckedIn, | |
| CheckedOut | |
| ) | |
| VALUES( | |
| @Id, | |
| @GuestEmail, | |
| @RoomNumber, | |
| @Start, | |
| @End, | |
| @CheckedIn, | |
| @CheckedOut | |
| ) | |
| RETURNING *; | |
| ", | |
| new ReservationDb(newReservation) | |
| ); | |
| await sqliteConnection.ExecuteAsync("COMMIT;"); | |
| return createdReservation.ToDomain(); | |
| } | |
| catch | |
| { | |
| // Roll back the transaction if either the conflict check or insert fails. | |
| try | |
| { | |
| await sqliteConnection.ExecuteAsync("ROLLBACK;"); | |
| } | |
| catch | |
| { | |
| // Ignore rollback failures; original exception will be rethrown. | |
| } | |
| throw; | |
| } | |
| } | |
| else | |
| { | |
| // Fallback for non-SQLite connections: preserve existing behavior. | |
| await EnsureNoReservationConflict(newReservation); | |
| var createdReservation = await _db.QuerySingleAsync<ReservationDb>( | |
| @" | |
| INSERT INTO Reservations( | |
| Id, | |
| GuestEmail, | |
| RoomNumber, | |
| Start, | |
| End, | |
| CheckedIn, | |
| CheckedOut | |
| ) | |
| VALUES( | |
| @Id, | |
| @GuestEmail, | |
| @RoomNumber, | |
| @Start, | |
| @End, | |
| @CheckedIn, | |
| @CheckedOut | |
| ) | |
| RETURNING *; | |
| ", | |
| new ReservationDb(newReservation) | |
| ); | |
| return createdReservation.ToDomain(); | |
| } |
| [HttpGet, Produces("application/json"), Route("")] | ||
| public async Task<ActionResult<Reservation>> GetReservations() | ||
| { | ||
| var reservations = await _repo.GetReservations(); | ||
| if (IsNotStaff(Request, out ActionResult? result)) | ||
| { | ||
| return result!; | ||
| } | ||
|
|
||
| var reservations = await _repo.GetUpcomingReservations(); | ||
|
|
||
| return Json(reservations); |
There was a problem hiding this comment.
GetReservations is declared as Task<ActionResult<Reservation>> but it returns a collection (Json(reservations)). This can confuse Swagger / consumers and weakens type-safety. Update the signature to Task<ActionResult<IEnumerable<Reservation>>> (or similar) to accurately reflect the response shape.
| } | ||
|
|
||
| setAccessCode(""); | ||
| showStaffWelcomeToast(); |
There was a problem hiding this comment.
handleLogin doesn’t catch errors from loginStaff (e.g., network failure / unexpected 5xx). In that case the promise rejection will bubble up and the user gets no feedback. Add a catch branch to surface a toast (and optionally keep isAuthorized false) so the login flow fails gracefully.
| showStaffWelcomeToast(); | |
| showStaffWelcomeToast(); | |
| } catch { | |
| setIsAuthorized(false); | |
| showInfoToast("We could not log you in right now."); |
| return ky | ||
| .post("api/reservation", { json: newReservation }) | ||
| .json() | ||
| .then(ReservationSchema.parseAsync); |
There was a problem hiding this comment.
The API call now parses the response via ReservationSchema.parseAsync, but the schema uses PascalCase keys while ASP.NET Core JSON responses are camelCase by default (and the rest of the UI already assumes camelCase). This will cause bookings to fail at runtime due to Zod parse errors. Update ReservationSchema to match the API’s actual casing (or configure the API to emit PascalCase consistently).
| const durationMs = dateRange[1].getTime() - dateRange[0].getTime(); | ||
| if (durationMs < ONE_DAY_MS || durationMs > MAX_DURATION_MS) { | ||
| showInvalidDateToast(); | ||
| evt.preventDefault(); | ||
| return; |
There was a problem hiding this comment.
The stay-length validation uses a raw millisecond diff (end.getTime() - start.getTime()) and compares against 24 * 60 * 60 * 1000. This will be wrong across DST transitions (a 1-night stay can be 23h/25h locally) and can incorrectly reject valid bookings. Prefer validating by calendar days (e.g., compare DateOnly/YYYY-MM-DD values or use a date library’s differenceInCalendarDays) rather than milliseconds.
| newReservation.GuestEmail = newReservation.GuestEmail.Trim(); | ||
| newReservation.RoomNumber = newReservation.RoomNumber.Trim(); | ||
|
|
||
| ValidateReservation(newReservation); |
There was a problem hiding this comment.
CreateReservation calls .Trim() on newReservation.GuestEmail / RoomNumber without guarding for null. Since these controllers don’t use [ApiController]/ModelState validation, a malformed request missing those fields can result in a 500 NullReferenceException instead of a 400 with a clear message. Add explicit null/empty validation before trimming (or enable automatic model validation).
| if (reservation.Start.Date != DateTime.Today) | ||
| { | ||
| throw new InvalidCheckInException( | ||
| "Only reservations starting today can be checked in." | ||
| ); |
There was a problem hiding this comment.
ValidateCheckIn compares reservation.Start.Date to DateTime.Today. The UI sends ISO timestamps via toISOString() (UTC), so the stored/parsed Start can shift by timezone and cause same-day arrivals to be rejected (especially outside UTC). Consider treating reservation dates as date-only (e.g., DateOnly end-to-end) or normalizing everything to UTC and comparing against DateTime.UtcNow.Date after converting reservation.Start to UTC consistently.
Overview
Illia Krauchenia
Implemented the core reservation workflow end to end across the API and UI, covering guest booking, booking protection, staff access, and front-desk check-in.
RE-001 completed: added guest room booking with validation for dates, stay length, email format, and valid room numbers.
RE-002 completed: added overlap checks to prevent double booking the same room.
RE-003 completed: added staff access with a shared code and a staff view for current and upcoming reservations, including guest email details.
RE-004 completed: added same-day check-in flow with reservation filtering for today, email confirmation, and room occupancy updates.
Task Report
The work was delivered in task order from RE-001 through RE-004. Most of the time have gone into RE-001, RE-003, and RE-004, since those touched both backend and UI, while RE-002 was a smaller, focused validation change.
The whole work took roughly 4 hours.