refactor: redirect check-in routes to main check-in page#1
Merged
Conversation
- Removed RouteComponent from check-in-2, check-in-3, and check-in-4 routes. - Implemented redirect to '/check-in' in the beforeLoad hook for these routes. - Cleaned up unused state and form handling logic in check-in-2 and check-in-3. - Updated appointment fetching logic in appointment functions to improve API calls. - Added new utility functions for backend URL normalization. - Created unit tests for various routes including admin-only, appointment, auth, check-in, and service routes. - Improved error handling and response structure in the API.
📝 WalkthroughWalkthroughCentralizes runtime config (CORS, JWT, cookie options), tightens route auth/roles, adds reservation lifecycle and check‑in linking, introduces health and DB-startup helpers, adds Docker/Lightsail deployment artifacts, updates frontend baseURL and booking/check‑in flows, and expands tests across API and web. ChangesAPI: Config, Auth, Reservations, Check‑in, Startup & Tests
Deployment & Packaging
Frontend: Runtime, Booking/Check‑in, Auth & Navigation
Sequence Diagram(s)sequenceDiagram
actor Guest as Guest
participant FE as Frontend
participant API as API Server
participant DB as Database
Guest->>FE: submit selected appointment
FE->>API: PUT /check-in/:checkInId/appointment { appointmentId }
API->>DB: Load CheckIn by id
API->>DB: Load Appointment (+Customer)
API->>API: compare normalized phones & check same-day window
API->>DB: assign appointmentId to CheckIn and save
API-->>FE: 200 OK { checkIn }
FE->>Guest: show success
sequenceDiagram
actor User as Authenticated Booker
participant FE as Frontend
participant API as API Server
participant Email as Mailer
participant DB as Database
User->>FE: submit booking confirmation with email
FE->>API: PATCH /appointments/:id { serviceIds, email, ... }
API->>DB: load appointment and services
API->>DB: update appointment.status -> "confirmed"
API->>API: build booking confirmation template
API->>Email: sendMail(template)
Email-->>API: success or error (logged)
API-->>FE: 200 OK { appointment }
FE->>User: display confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/pages/bookings/BookingStepThree.tsx (1)
95-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRevert the local staff choice when the reservation update fails.
updateReservedAppointment()can reject when the slot was taken (apps/api/src/services/appointment.service.ts:1-43), but Line 98 updates the store before the PATCH completes. That leaves the booking flow showing a staff member the reservation never accepted. Restore the previous selection incatch, or only commitselectedStaffIdafter a successful reservation update.Suggested fix
const handleStaffClick = async (staffId: string) => { + const previousStaffId = selectedStaffId try { setIsUpdating(true) - setSelectedStaffId(staffId) // If we have an active reservation, update it to the specific staff if (reservationId && staffId !== 'anyone') { await updateReservedAppointment(reservationId, { staffId }) } + + setSelectedStaffId(staffId) } catch (err: unknown) { + setSelectedStaffId(previousStaffId) toast.error(ErrorHandler.getMessage(err)) } finally { setIsUpdating(false) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/bookings/BookingStepThree.tsx` around lines 95 - 105, handleStaffClick currently calls setSelectedStaffId before awaiting updateReservedAppointment, so if the PATCH fails the UI still shows the new staff; move commitment or restore previous selection: capture the priorSelected = selectedStaffId, then either (a) only call setSelectedStaffId(staffId) after await updateReservedAppointment(reservationId, { staffId }) succeeds, or (b) keep the optimistic setSelectedStaffId but in the catch block call setSelectedStaffId(priorSelected) to revert; ensure reservationId and updateReservedAppointment usage is preserved and show the toast error as currently done.apps/web/src/pages/CheckIn.tsx (1)
170-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSurface the backend error here.
The new appointment-selection endpoint returns specific eligibility failures, but this catch block collapses all of them into a generic toast. Reusing
ErrorHandler.getMessage(error)keeps the kiosk flow actionable when a slot is no longer valid.Suggested fix
- } catch { - toast.error('Failed to confirm selection') + } catch (error: unknown) { + toast.error(ErrorHandler.getMessage(error)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/pages/CheckIn.tsx` around lines 170 - 177, The catch block that calls toast.error('Failed to confirm selection') swallows the backend error; change the try/catch to capture the thrown error (e.g., catch (error)) and pass the backend message into the toast using ErrorHandler.getMessage(error) so users see specific eligibility failures returned by the appointment-selection endpoint; update the catch in the code around api.put(`/check-in/${currentCheckInId}/appointment`, { appointmentId: selectedSlotId }) to call toast.error(ErrorHandler.getMessage(error)) and keep the existing setStep('success') in the try path.
🧹 Nitpick comments (1)
apps/web/src/utils/appointment.function.ts (1)
142-151: ⚡ Quick win
fetchCustomerAppointmentsno longer matches its contract.The
_customerargument is ignored now, while the backend only auto-scopes non-privileged users. For staff/admin callers this helper will return the full appointment list, not the requested customer's appointments. Either preserve customer filtering for privileged callers or remove/rename the parameter so the API surface stays honest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/utils/appointment.function.ts` around lines 142 - 151, fetchCustomerAppointments currently ignores its _customer parameter and always calls api.get('/appointments'), causing privileged callers to receive the full list; either preserve filtering by sending the customer info as query params or remove/rename the unused parameter to keep the surface honest. To fix, update fetchCustomerAppointments to, when _customer has id/email/phone, call api.get('/appointments', { params: { id: _customer.id, email: _customer.email, phone: _customer.phone } }) so the backend can scope results for the requested customer (ensure you serialize only non-null fields), or if you prefer API simplification remove the _customer parameter and rename the function (and update callers) to reflect it returns all appointments; reference fetchCustomerAppointments and the api.get('/appointments') call when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/config/app.ts`:
- Around line 55-56: The current logic lets COOKIE_SECURE=false override
sameSite === "none", which produces an invalid SameSite=None + secure:false
cookie; change the precedence so SameSite=None always forces secure=true:
compute defaultSecure = (sameSite === "none" || process.env.NODE_ENV ===
"production") and then set secure so that if sameSite === "none" you always set
secure = true, otherwise fall back to secureOverride ?? (process.env.NODE_ENV
=== "production"); reference the existing symbols secureOverride and secure (and
env COOKIE_SECURE / sameSite) when making the change.
In `@apps/api/src/controllers/appointment.controller.ts`:
- Around line 30-34: Validate incoming UUIDs in the controller before calling
the service: in getReservedAppointmentById (and the other handlers referenced
around lines 40-49 and 55-59), check that req.params.id and any
req.params.staffId are valid UUIDs (use a UUID validator or a strict regex) and
if not return a 400/Bad Request (e.g., res.status(400).json(...)/res.badRequest)
with a clear message instead of forwarding the malformed value to
appointmentService.getReservedAppointmentById or other service methods; only
call the service when validation passes.
- Around line 30-59: The handlers getReservedAppointmentById,
updateReservedAppointment, and unreserveAppointment currently treat the
reservation UUID as an open bearer token; make these endpoints enforce
caller-bound authorization by validating ownership or a signed reservation
secret before performing any read/update/delete. Update each controller to (a)
require an authenticated principal (e.g. req.user.id) or accept a
reservationSecret header/body issued at reservation time, (b) fetch the
reservation via appointmentService.getReservedAppointmentById and verify
reservation.ownerId === req.user.id (or validate the signed reservationSecret)
and only then call appointmentService.updateReservedAppointment or
deleteReservedAppointment; if validation fails return 401/403. Alternatively,
move this logic into an authorization middleware invoked on these routes and
reference the same appointmentService verification to avoid duplicating checks.
In `@apps/api/src/controllers/check-in.controller.ts`:
- Around line 35-39: The controller currently only checks presence of id and
appointmentId but not format, so validate both id and appointmentId as UUIDs
before calling CheckInService.selectAppointmentForCheckIn; use a UUID validator
(e.g., validator.isUUID or your project's util) to check id and appointmentId
and if either is invalid immediately return res.badRequest with a clear message
like "Invalid check-in ID" / "Invalid appointment ID" so malformed values never
reach findByPk/SelectAppointmentForCheckIn.
In `@apps/api/src/middleware/auth.middleware.ts`:
- Around line 35-39: In authenticateJWT(), extract and validate the JWT secret
by calling getJwtSecret() before entering the try/catch so
missing/placeholder-secret errors are thrown rather than being caught and turned
into a 401; then call jwt.verify(token, secret) inside the try/catch as before.
Apply the same change to refreshToken(): obtain the secret with getJwtSecret()
prior to the try/catch (so configuration errors surface) and then use that
secret in jwt.verify within the existing try block.
In `@apps/api/src/routes/_tests_/auth.routes.test.ts`:
- Line 5: The test suite mutates process.env.JWT_SECRET at module scope
(process.env.JWT_SECRET = "unit-test-secret"), which can leak into other suites;
move this mutation into lifecycle hooks (e.g., beforeAll or beforeEach) and
restore the original value in the corresponding afterAll/afterEach to avoid
cross-suite leakage—use the same test file's setup/teardown
(auth.routes.test.ts) and reference process.env.JWT_SECRET when saving the
original value and restoring it.
In `@apps/api/src/routes/appointment.routes.ts`:
- Around line 8-9: The public GET/PATCH routes exposing reservation by raw id
(router.get('/reserve/:id') -> appointmentController.getReservedAppointmentById
and router.patch('/reserve/:id') ->
appointmentController.updateReservedAppointment) must be protected: either add
an authentication/authorization middleware (e.g., requireAuth or verifyJwt) to
both route declarations so only the reservation owner can read/write, or replace
the :id param with a signed, expiring reservation token (e.g., reserveToken) and
update the controller methods to validate and decrypt that token and check
expiry/ownership before returning or mutating the reservation; implement one of
these approaches and ensure the controller checks proof of possession/ownership,
rejects invalid/expired tokens, and logs unauthorized attempts.
In `@apps/api/src/server.ts`:
- Around line 21-34: The CORS check fails when configured origins include minor
formatting differences (e.g., trailing slash) because getCorsOrigins() values
aren’t normalized before being put into allowedOrigins; update getCorsOrigins()
or the creation of allowedOrigins to normalize each origin (trim whitespace,
strip trailing slash, and optionally lowercase) and also normalize the incoming
origin string inside the cors origin callback used by app.use(cors(...));
reference getCorsOrigins(), the allowedOrigins Set and the origin(origin,
callback) function to locate where to normalize values so that comparisons match
browser Origin headers.
In `@apps/api/src/services/_tests_/appointment.service.test.ts`:
- Around line 329-354: The test for confirmAppointment is missing a stub for
Appointment.findAll which is invoked indirectly via getAppointmentById ->
completeElapsedCheckedInAppointments; add a mock for Appointment.findAll (e.g.,
mockResolvedValueOnce([]) or an appropriate array) before calling
appointmentService.confirmAppointment so that checkedInAppointments.filter(...)
has an array to operate on and the test can reach the final assertions;
reference Appointment.findAll, confirmAppointment, getAppointmentById, and
completeElapsedCheckedInAppointments when adding the stub.
In `@apps/api/src/services/appointment.service.ts`:
- Around line 363-374: The current code throws a generic Error when an
appointment isn't found after a scoped lookup, which can surface as a 500;
update the not-found path to throw an HttpError with a 404 status. Specifically,
replace the throw new Error("Appointment not found") with throw new
HttpError(404, "Appointment not found") (ensure HttpError is imported), and only
apply this 404 behavior for the customer-scoped lookup branch that uses
Appointment.findOne when actor && !isPrivilegedActor(actor); keep behavior
consistent for other branches.
In `@apps/api/src/services/check-in.service.ts`:
- Around line 229-235: The eligibility check currently only verifies phone/date
but not appointment state; update the conditional in the block that checks
matchesPhone, start, end (the clause that throws new HttpError(400, { message:
"Appointment is not eligible for this check-in" })) to also reject non-active
statuses by verifying appointment.status equals the active/scheduled enum value
used elsewhere (e.g., AppointmentStatus.ACTIVE or the same status constant used
in the earlier check-in lookup flow). If AppointmentStatus is not imported,
import/use the same enum or constant as the prior lookup, and include
appointment.status in the OR chain so canceled/checked_in/completed appointments
will trigger the same HttpError.
---
Outside diff comments:
In `@apps/web/src/pages/bookings/BookingStepThree.tsx`:
- Around line 95-105: handleStaffClick currently calls setSelectedStaffId before
awaiting updateReservedAppointment, so if the PATCH fails the UI still shows the
new staff; move commitment or restore previous selection: capture the
priorSelected = selectedStaffId, then either (a) only call
setSelectedStaffId(staffId) after await updateReservedAppointment(reservationId,
{ staffId }) succeeds, or (b) keep the optimistic setSelectedStaffId but in the
catch block call setSelectedStaffId(priorSelected) to revert; ensure
reservationId and updateReservedAppointment usage is preserved and show the
toast error as currently done.
In `@apps/web/src/pages/CheckIn.tsx`:
- Around line 170-177: The catch block that calls toast.error('Failed to confirm
selection') swallows the backend error; change the try/catch to capture the
thrown error (e.g., catch (error)) and pass the backend message into the toast
using ErrorHandler.getMessage(error) so users see specific eligibility failures
returned by the appointment-selection endpoint; update the catch in the code
around api.put(`/check-in/${currentCheckInId}/appointment`, { appointmentId:
selectedSlotId }) to call toast.error(ErrorHandler.getMessage(error)) and keep
the existing setStep('success') in the try path.
---
Nitpick comments:
In `@apps/web/src/utils/appointment.function.ts`:
- Around line 142-151: fetchCustomerAppointments currently ignores its _customer
parameter and always calls api.get('/appointments'), causing privileged callers
to receive the full list; either preserve filtering by sending the customer info
as query params or remove/rename the unused parameter to keep the surface
honest. To fix, update fetchCustomerAppointments to, when _customer has
id/email/phone, call api.get('/appointments', { params: { id: _customer.id,
email: _customer.email, phone: _customer.phone } }) so the backend can scope
results for the requested customer (ensure you serialize only non-null fields),
or if you prefer API simplification remove the _customer parameter and rename
the function (and update callers) to reflect it returns all appointments;
reference fetchCustomerAppointments and the api.get('/appointments') call when
making the change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c56eba1-21f6-4868-9304-d1bb57b50d05
📒 Files selected for processing (46)
apps/api/.env.exampleapps/api/src/config/app.tsapps/api/src/controllers/appointment.controller.tsapps/api/src/controllers/auth.controller.tsapps/api/src/controllers/check-in.controller.tsapps/api/src/middleware/auth.middleware.tsapps/api/src/routes/_tests_/admin-only.routes.test.tsapps/api/src/routes/_tests_/appointment.routes.test.tsapps/api/src/routes/_tests_/auth.routes.test.tsapps/api/src/routes/_tests_/check-in.routes.test.tsapps/api/src/routes/_tests_/service.routes.test.tsapps/api/src/routes/appointment.routes.tsapps/api/src/routes/auth.routes.tsapps/api/src/routes/check-in.routes.tsapps/api/src/routes/email.routes.tsapps/api/src/routes/permission.routes.tsapps/api/src/routes/role.routes.tsapps/api/src/routes/service.routes.tsapps/api/src/server.tsapps/api/src/services/_tests_/appointment.service.test.tsapps/api/src/services/_tests_/auth.service.test.tsapps/api/src/services/appointment.service.tsapps/api/src/services/auth.service.tsapps/api/src/services/check-in.service.tsapps/api/src/utils/zod_schemas/appointment.schema.tsapps/web/src/components/admin/header.tsxapps/web/src/components/admin/sidebar.tsxapps/web/src/components/auth/AuthProvider.tsxapps/web/src/layout/Header.tsxapps/web/src/lib/adminApi.tsapps/web/src/lib/api.tsapps/web/src/lib/runtime.test.tsapps/web/src/lib/runtime.tsapps/web/src/pages/CheckIn.tsxapps/web/src/pages/bookings/BookingStepFour.tsxapps/web/src/pages/bookings/BookingStepThree.tsxapps/web/src/routeTree.gen.tsapps/web/src/routes/_adminLayout/admin-profile/index.tsxapps/web/src/routes/_layout/_check-in/check-in-1/index.tsxapps/web/src/routes/_layout/_check-in/check-in-2/index.tsxapps/web/src/routes/_layout/_check-in/check-in-3/index.tsxapps/web/src/routes/_layout/_check-in/check-in-4/index.tsxapps/web/src/stores/auth.store.tsapps/web/src/utils/appointment.function.tsapps/web/src/utils/auth.function.tsapps/web/vite.config.ts
💤 Files with no reviewable changes (1)
- apps/web/src/stores/auth.store.ts
Comment on lines
+30
to
+34
| getReservedAppointmentById: async (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| const { id } = req.params; | ||
| const reservation = await appointmentService.getReservedAppointmentById(id as string); | ||
| return res.ok(reservation, "Get reservation successfully"); |
There was a problem hiding this comment.
Reject malformed reservation IDs and staffId before calling the service.
These handlers pass arbitrary strings into UUID-backed lookups. Bad input will tend to fail in Sequelize/database code and come back as a 500 instead of a request validation error.
Also applies to: 40-49, 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/controllers/appointment.controller.ts` around lines 30 - 34,
Validate incoming UUIDs in the controller before calling the service: in
getReservedAppointmentById (and the other handlers referenced around lines 40-49
and 55-59), check that req.params.id and any req.params.staffId are valid UUIDs
(use a UUID validator or a strict regex) and if not return a 400/Bad Request
(e.g., res.status(400).json(...)/res.badRequest) with a clear message instead of
forwarding the malformed value to appointmentService.getReservedAppointmentById
or other service methods; only call the service when validation passes.
Comment on lines
+30
to
59
| getReservedAppointmentById: async (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| const { id } = req.params; | ||
| const reservation = await appointmentService.getReservedAppointmentById(id as string); | ||
| return res.ok(reservation, "Get reservation successfully"); | ||
| } catch (err) { | ||
| next(err); | ||
| } | ||
| }, | ||
|
|
||
| updateReservedAppointment: async (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| const { id } = req.params; | ||
| const staffId = typeof req.body?.staffId === "string" ? req.body.staffId : undefined; | ||
|
|
||
| const reservation = await appointmentService.updateReservedAppointment(id as string, { | ||
| staffId, | ||
| }); | ||
|
|
||
| return res.ok(reservation, "Reservation updated successfully"); | ||
| } catch (err) { | ||
| next(err); | ||
| } | ||
| }, | ||
|
|
||
| unreserveAppointment: async (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| const { id } = req.params; | ||
| await appointmentService.deleteAppointment(id as string); | ||
| await appointmentService.deleteReservedAppointment(id as string); | ||
| return res.ok(null, "Slot unreserved successfully"); |
There was a problem hiding this comment.
Reservation IDs are acting as bearer tokens here.
Per apps/api/src/routes/appointment.routes.ts:7-10, these handlers are public, and the only authorization input is :id. Anyone who gets a pending reservation UUID can read it, reassign staff, or cancel it. This needs caller-bound protection, e.g. auth ownership checks or a separate signed reservation secret issued at reserve time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/controllers/appointment.controller.ts` around lines 30 - 59, The
handlers getReservedAppointmentById, updateReservedAppointment, and
unreserveAppointment currently treat the reservation UUID as an open bearer
token; make these endpoints enforce caller-bound authorization by validating
ownership or a signed reservation secret before performing any
read/update/delete. Update each controller to (a) require an authenticated
principal (e.g. req.user.id) or accept a reservationSecret header/body issued at
reservation time, (b) fetch the reservation via
appointmentService.getReservedAppointmentById and verify reservation.ownerId ===
req.user.id (or validate the signed reservationSecret) and only then call
appointmentService.updateReservedAppointment or deleteReservedAppointment; if
validation fails return 401/403. Alternatively, move this logic into an
authorization middleware invoked on these routes and reference the same
appointmentService verification to avoid duplicating checks.
Comment on lines
35
to
39
| try { | ||
| req.user = jwt.verify(token, String(JWT_SECRET)) as AuthUserPayload; | ||
| req.user = jwt.verify(token, getJwtSecret()) as AuthUserPayload; | ||
| return next(); | ||
| } catch (err) { | ||
| return res.unauthorized('invalid or expired token'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.ts" | grep -E "(config|middleware)" | head -20Repository: Nail-Addison/nail-star
Length of output: 353
🏁 Script executed:
cat -n apps/api/src/config/app.ts | sed -n '30,45p'Repository: Nail-Addison/nail-star
Length of output: 584
🏁 Script executed:
cat -n apps/api/src/middleware/auth.middleware.tsRepository: Nail-Addison/nail-star
Length of output: 2511
Move JWT secret validation before the try/catch in authenticateJWT().
getJwtSecret() throws for missing or placeholder values (apps/api/src/config/app.ts:36-38). Currently, authenticateJWT() masks this misconfiguration as a 401 invalid or expired token, while refreshToken() lets the same error propagate as a server error. Extract the secret before jwt.verify() to expose configuration errors instead of hiding them.
Suggested fix
export function authenticateJWT(req: Request, res: Response, next: NextFunction) {
+ const secret = getJwtSecret();
let token = '';
const authHeader = req.headers.authorization || '';
@@ -35,7 +37,7 @@ export function authenticateJWT(req: Request, res: Response, next: NextFunction
}
try {
- req.user = jwt.verify(token, getJwtSecret()) as AuthUserPayload;
+ req.user = jwt.verify(token, secret) as AuthUserPayload;
return next();
} catch (err) {
return res.unauthorized('invalid or expired token');
@@ -45,12 +47,13 @@ export function authenticateJWT(req: Request, res: Response, next: NextFunction
export function refreshToken(req: Request, res: Response) {
if (!req.user) {
return res.unauthorized('unauthenticated');
}
+ const secret = getJwtSecret();
const newToken = jwt.sign(
{
sub: req.user.sub,
email: req.user.email,
roleId: req.user.roleId,
},
- getJwtSecret(),
+ secret,
{ expiresIn: '45m' }
);Also applies to: refreshToken() at lines 48–56.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/middleware/auth.middleware.ts` around lines 35 - 39, In
authenticateJWT(), extract and validate the JWT secret by calling getJwtSecret()
before entering the try/catch so missing/placeholder-secret errors are thrown
rather than being caught and turned into a 401; then call jwt.verify(token,
secret) inside the try/catch as before. Apply the same change to refreshToken():
obtain the secret with getJwtSecret() prior to the try/catch (so configuration
errors surface) and then use that secret in jwt.verify within the existing try
block.
Comment on lines
+21
to
+34
| const allowedOrigins = new Set(getCorsOrigins()); | ||
|
|
||
| const app = express(); | ||
| app.set("trust proxy", 1); | ||
| app.use(express.json()); | ||
| app.use(cookieParser()); | ||
| app.use(cors({ | ||
| origin: "http://localhost:5173", | ||
| origin(origin, callback) { | ||
| if (!origin || allowedOrigins.has(origin)) { | ||
| return callback(null, true); | ||
| } | ||
|
|
||
| return callback(new Error("Origin not allowed by CORS")); | ||
| }, |
There was a problem hiding this comment.
Normalize configured origins before the Set lookup.
getCorsOrigins() currently only trims/splits values, so a configured origin like https://app.example.com/ will never match the browser's Origin: https://app.example.com. That turns a minor env-formatting difference into a complete CORS failure.
Suggested fix
+const normalizeOrigin = (value: string) => {
+ try {
+ return new URL(value).origin;
+ } catch {
+ return value.replace(/\/+$/, "");
+ }
+};
+
-const allowedOrigins = new Set(getCorsOrigins());
+const allowedOrigins = new Set(getCorsOrigins().map(normalizeOrigin));
...
app.use(cors({
origin(origin, callback) {
- if (!origin || allowedOrigins.has(origin)) {
+ const normalizedOrigin = origin ? normalizeOrigin(origin) : undefined;
+ if (!normalizedOrigin || allowedOrigins.has(normalizedOrigin)) {
return callback(null, true);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/server.ts` around lines 21 - 34, The CORS check fails when
configured origins include minor formatting differences (e.g., trailing slash)
because getCorsOrigins() values aren’t normalized before being put into
allowedOrigins; update getCorsOrigins() or the creation of allowedOrigins to
normalize each origin (trim whitespace, strip trailing slash, and optionally
lowercase) and also normalize the incoming origin string inside the cors origin
callback used by app.use(cors(...)); reference getCorsOrigins(), the
allowedOrigins Set and the origin(origin, callback) function to locate where to
normalize values so that comparisons match browser Origin headers.
Comment on lines
+329
to
+354
| describe("confirmAppointment", () => { | ||
| it("defaults a confirmed reservation to confirmed status", async () => { | ||
| const mockAppointment = { | ||
| id: "appointment-1", | ||
| status: "pending", | ||
| phoneNumber: "RESERVED", | ||
| customerId: null, | ||
| customerName: null, | ||
| save: jest.fn().mockResolvedValue(undefined), | ||
| setServices: jest.fn().mockResolvedValue(undefined), | ||
| }; | ||
|
|
||
| (Appointment.findByPk as jest.Mock) | ||
| .mockResolvedValueOnce(mockAppointment) | ||
| .mockResolvedValueOnce({ id: "appointment-1", status: "confirmed" }); | ||
|
|
||
| const result = await appointmentService.confirmAppointment("appointment-1", { | ||
| serviceIds: ["service-1"], | ||
| phoneNumber: "0900000000", | ||
| customerName: "Guest", | ||
| }); | ||
|
|
||
| expect(mockAppointment.status).toBe("confirmed"); | ||
| expect(mockAppointment.save).toHaveBeenCalledWith({ transaction: mockTransaction }); | ||
| expect(result).toEqual({ id: "appointment-1", status: "confirmed" }); | ||
| }); |
There was a problem hiding this comment.
This test is missing the internal findAll mock.
confirmAppointment() finishes by calling getAppointmentById(), and that path first runs completeElapsedCheckedInAppointments(). Without stubbing Appointment.findAll, checkedInAppointments.filter(...) will blow up before the final assertions.
Suggested fix
it("defaults a confirmed reservation to confirmed status", async () => {
const mockAppointment = {
id: "appointment-1",
status: "pending",
phoneNumber: "RESERVED",
customerId: null,
customerName: null,
save: jest.fn().mockResolvedValue(undefined),
setServices: jest.fn().mockResolvedValue(undefined),
};
+ (Appointment.findAll as jest.Mock).mockResolvedValueOnce([]);
(Appointment.findByPk as jest.Mock)
.mockResolvedValueOnce(mockAppointment)
.mockResolvedValueOnce({ id: "appointment-1", status: "confirmed" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe("confirmAppointment", () => { | |
| it("defaults a confirmed reservation to confirmed status", async () => { | |
| const mockAppointment = { | |
| id: "appointment-1", | |
| status: "pending", | |
| phoneNumber: "RESERVED", | |
| customerId: null, | |
| customerName: null, | |
| save: jest.fn().mockResolvedValue(undefined), | |
| setServices: jest.fn().mockResolvedValue(undefined), | |
| }; | |
| (Appointment.findByPk as jest.Mock) | |
| .mockResolvedValueOnce(mockAppointment) | |
| .mockResolvedValueOnce({ id: "appointment-1", status: "confirmed" }); | |
| const result = await appointmentService.confirmAppointment("appointment-1", { | |
| serviceIds: ["service-1"], | |
| phoneNumber: "0900000000", | |
| customerName: "Guest", | |
| }); | |
| expect(mockAppointment.status).toBe("confirmed"); | |
| expect(mockAppointment.save).toHaveBeenCalledWith({ transaction: mockTransaction }); | |
| expect(result).toEqual({ id: "appointment-1", status: "confirmed" }); | |
| }); | |
| describe("confirmAppointment", () => { | |
| it("defaults a confirmed reservation to confirmed status", async () => { | |
| const mockAppointment = { | |
| id: "appointment-1", | |
| status: "pending", | |
| phoneNumber: "RESERVED", | |
| customerId: null, | |
| customerName: null, | |
| save: jest.fn().mockResolvedValue(undefined), | |
| setServices: jest.fn().mockResolvedValue(undefined), | |
| }; | |
| (Appointment.findAll as jest.Mock).mockResolvedValueOnce([]); | |
| (Appointment.findByPk as jest.Mock) | |
| .mockResolvedValueOnce(mockAppointment) | |
| .mockResolvedValueOnce({ id: "appointment-1", status: "confirmed" }); | |
| const result = await appointmentService.confirmAppointment("appointment-1", { | |
| serviceIds: ["service-1"], | |
| phoneNumber: "0900000000", | |
| customerName: "Guest", | |
| }); | |
| expect(mockAppointment.status).toBe("confirmed"); | |
| expect(mockAppointment.save).toHaveBeenCalledWith({ transaction: mockTransaction }); | |
| expect(result).toEqual({ id: "appointment-1", status: "confirmed" }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/services/_tests_/appointment.service.test.ts` around lines 329 -
354, The test for confirmAppointment is missing a stub for Appointment.findAll
which is invoked indirectly via getAppointmentById ->
completeElapsedCheckedInAppointments; add a mock for Appointment.findAll (e.g.,
mockResolvedValueOnce([]) or an appropriate array) before calling
appointmentService.confirmAppointment so that checkedInAppointments.filter(...)
has an array to operate on and the test can reach the final assertions;
reference Appointment.findAll, confirmAppointment, getAppointmentById, and
completeElapsedCheckedInAppointments when adding the stub.
Comment on lines
+363
to
374
| const appointment = actor && !isPrivilegedActor(actor) | ||
| ? await Appointment.findOne({ | ||
| where: { | ||
| id, | ||
| customerId: actor.sub, | ||
| }, | ||
| ...baseQuery, | ||
| }) | ||
| : await Appointment.findByPk(id, baseQuery); | ||
|
|
||
| if (!appointment) throw new Error("Appointment not found"); | ||
| return appointment; |
There was a problem hiding this comment.
Use HttpError for the scoped not-found path.
With customer scoping in place, this branch now covers both genuinely missing records and out-of-scope lookups. Throwing a plain Error here is likely to surface as a 500 instead of a 404.
Suggested fix
- if (!appointment) throw new Error("Appointment not found");
+ if (!appointment) {
+ throw new HttpError(404, { message: "Appointment not found" });
+ }
return appointment;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const appointment = actor && !isPrivilegedActor(actor) | |
| ? await Appointment.findOne({ | |
| where: { | |
| id, | |
| customerId: actor.sub, | |
| }, | |
| ...baseQuery, | |
| }) | |
| : await Appointment.findByPk(id, baseQuery); | |
| if (!appointment) throw new Error("Appointment not found"); | |
| return appointment; | |
| const appointment = actor && !isPrivilegedActor(actor) | |
| ? await Appointment.findOne({ | |
| where: { | |
| id, | |
| customerId: actor.sub, | |
| }, | |
| ...baseQuery, | |
| }) | |
| : await Appointment.findByPk(id, baseQuery); | |
| if (!appointment) { | |
| throw new HttpError(404, { message: "Appointment not found" }); | |
| } | |
| return appointment; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/services/appointment.service.ts` around lines 363 - 374, The
current code throws a generic Error when an appointment isn't found after a
scoped lookup, which can surface as a 500; update the not-found path to throw an
HttpError with a 404 status. Specifically, replace the throw new
Error("Appointment not found") with throw new HttpError(404, "Appointment not
found") (ensure HttpError is imported), and only apply this 404 behavior for the
customer-scoped lookup branch that uses Appointment.findOne when actor &&
!isPrivilegedActor(actor); keep behavior consistent for other branches.
Comment on lines
152
to
154
| } catch (err) { | ||
| console.error('Step 4: Failed to load initial data:', err) | ||
| console.error('Failed to load booking confirmation data:', err) | ||
| } finally { |
There was a problem hiding this comment.
Treat initial booking-data failures as blocking errors.
If the initial fetch fails here, the component only logs and then drops loading to false, so users can land on a partial confirmation screen and still try to submit. Set a blocking error state here and stop the confirm flow until the reservation data is reloaded successfully.
Suggested fix
} catch (err) {
console.error('Failed to load booking confirmation data:', err)
+ setError(
+ 'Failed to load your booking details. Please restart the booking flow.',
+ )
} finally {
setLoading(false)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| console.error('Step 4: Failed to load initial data:', err) | |
| console.error('Failed to load booking confirmation data:', err) | |
| } finally { | |
| } catch (err) { | |
| console.error('Failed to load booking confirmation data:', err) | |
| setError( | |
| 'Failed to load your booking details. Please restart the booking flow.', | |
| ) | |
| } finally { |
…ck endpoint with database synchronization configuration
… to non-root user
feat: implement AWS Lightsail deployment stack and add API health che…
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/config/database-startup.ts`:
- Around line 7-16: The current order lets DB_SYNC_ALTER=true return { alter:
true } before the production guard, so change the conditional logic so
production safety wins: check if env.NODE_ENV === "production" && env.DB_SYNC
!== "true" and return null before honoring DB_SYNC_ALTER, or require both
env.DB_SYNC === "true" and env.DB_SYNC_ALTER === "true" to return { alter: true
} (otherwise fall through); update the conditionals that reference
DB_SYNC_ALTER, DB_SYNC and NODE_ENV accordingly.
In `@deploy/lightsail/compose.yml`:
- Around line 25-27: The Caddy service should not wait for the API healthcheck;
remove the health-based dependency so Caddy starts immediately. In the compose
service definition for Caddy (the depends_on block referencing api with
condition: service_healthy), either remove the whole depends_on entry or change
it to a simple depends_on: - api without the condition (or remove depends_on
entirely) so Caddy binds ports 80/443 regardless of API health; update the caddy
service definition accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b6488cb7-a169-4900-b3e1-0f4a2a6567e2
📒 Files selected for processing (14)
.dockerignore.gitignoreapps/api/Dockerfileapps/api/src/config/_tests_/database-startup.test.tsapps/api/src/config/database-startup.tsapps/api/src/routes/_tests_/health.routes.test.tsapps/api/src/routes/health.routes.tsapps/api/src/server.tsdeploy/lightsail/.env.exampledeploy/lightsail/Caddyfiledeploy/lightsail/api.env.exampledeploy/lightsail/compose.ymldocs/deploy-lightsail.mdpackage.json
✅ Files skipped from review due to trivial changes (7)
- deploy/lightsail/api.env.example
- .dockerignore
- deploy/lightsail/Caddyfile
- docs/deploy-lightsail.md
- .gitignore
- apps/api/src/routes/health.routes.ts
- package.json
Comment on lines
+7
to
+16
| if (env.DB_SYNC_ALTER === "true") { | ||
| return { alter: true }; | ||
| } | ||
|
|
||
| if (env.DB_SYNC === "true") { | ||
| return {}; | ||
| } | ||
|
|
||
| if (env.NODE_ENV === "production" && env.DB_SYNC !== "true") { | ||
| return null; |
There was a problem hiding this comment.
Don't let DB_SYNC_ALTER bypass the production safety check.
Line 7 returns { alter: true } before the production guard runs, so DB_SYNC_ALTER=true still mutates the schema on startup in production even when DB_SYNC is unset. That reintroduces risky automatic DDL during deploys.
Suggested fix
export function getDatabaseSyncOptions(env: StartupEnv = process.env): SyncOptions | undefined | null {
+ const isProduction = env.NODE_ENV === "production";
+
+ if (isProduction && env.DB_SYNC !== "true") {
+ return null;
+ }
+
if (env.DB_SYNC_ALTER === "true") {
return { alter: true };
}
if (env.DB_SYNC === "true") {
return {};
}
-
- if (env.NODE_ENV === "production" && env.DB_SYNC !== "true") {
- return null;
- }
return undefined;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (env.DB_SYNC_ALTER === "true") { | |
| return { alter: true }; | |
| } | |
| if (env.DB_SYNC === "true") { | |
| return {}; | |
| } | |
| if (env.NODE_ENV === "production" && env.DB_SYNC !== "true") { | |
| return null; | |
| export function getDatabaseSyncOptions(env: StartupEnv = process.env): SyncOptions | undefined | null { | |
| const isProduction = env.NODE_ENV === "production"; | |
| if (isProduction && env.DB_SYNC !== "true") { | |
| return null; | |
| } | |
| if (env.DB_SYNC_ALTER === "true") { | |
| return { alter: true }; | |
| } | |
| if (env.DB_SYNC === "true") { | |
| return {}; | |
| } | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/config/database-startup.ts` around lines 7 - 16, The current
order lets DB_SYNC_ALTER=true return { alter: true } before the production
guard, so change the conditional logic so production safety wins: check if
env.NODE_ENV === "production" && env.DB_SYNC !== "true" and return null before
honoring DB_SYNC_ALTER, or require both env.DB_SYNC === "true" and
env.DB_SYNC_ALTER === "true" to return { alter: true } (otherwise fall through);
update the conditionals that reference DB_SYNC_ALTER, DB_SYNC and NODE_ENV
accordingly.
Comment on lines
+25
to
+27
| depends_on: | ||
| api: | ||
| condition: service_healthy |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n deploy/lightsail/compose.ymlRepository: Nail-Addison/nail-star
Length of output: 1258
🏁 Script executed:
find . -name "*.ts" -o -name "*.js" | xargs grep -l "healthz" | head -20Repository: Nail-Addison/nail-star
Length of output: 157
🌐 Web query:
Docker Compose depends_on condition service_healthy vs service_started behavior
💡 Result:
In Docker Compose, the depends_on directive controls the startup and shutdown order of services. It supports conditions to define when a dependency is satisfied before starting a dependent service [1][2][3]. service_started (default): Compose waits only until the dependency container is running/started, but does not verify if the service inside is ready or healthy. This is equivalent to the short syntax depends_on: [service]. A database container might start quickly but still be initializing, leading to connection failures in dependent services [4][1][5]. service_healthy: Compose waits for the dependency's healthcheck to report "healthy" status before starting the dependent service. The dependency must define a healthcheck section. This ensures the service is not just running but operational and ready (e.g., PostgreSQL accepting connections via pg_isready) [1][2][3]. Example from official docs [1]: services: web: depends_on: db: condition: service_healthy db: image: postgres:18 healthcheck: test: ["CMD-SHELL", "pg_isready -U
Citations:
- 1: https://docs.docker.com/compose/how-tos/startup-order/
- 2: https://docs.docker.com/compose/compose-file/05-services/
- 3: https://docs.docker.com/reference/compose-file/services/
- 4: https://docker.recipes/blog/docker-compose-healthchecks-depends-on
- 5: https://oneuptime.com/blog/post/2026-01-25-docker-compose-depends-on/view
- 6: Healthchecks dependent on health status of depends_on docker/compose#11582
- 7: https://stackoverflow.com/questions/52322800/docker-compose-how-to-wait-for-other-service-to-be-ready
- 8: https://oneuptime.com/blog/post/2026-01-16-docker-compose-depends-on-healthcheck/view
- 9: https://medium.com/@pavel.loginov.dev/wait-for-services-to-start-in-docker-compose-wait-for-it-vs-healthcheck-e0248f54962b
🏁 Script executed:
head -50 apps/api/src/server.tsRepository: Nail-Addison/nail-star
Length of output: 1740
🏁 Script executed:
cat deploy/lightsail/CaddyfileRepository: Nail-Addison/nail-star
Length of output: 333
Don't gate Caddy startup on API health.
With condition: service_healthy, Caddy will not bind ports 80/443 until the API's healthcheck passes. If the API is blocked on database startup or fails its healthcheck, the entire site stays down instead of still serving the static frontend and only failing API requests.
Suggested fix
depends_on:
api:
- condition: service_healthy
+ condition: service_started🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/lightsail/compose.yml` around lines 25 - 27, The Caddy service should
not wait for the API healthcheck; remove the health-based dependency so Caddy
starts immediately. In the compose service definition for Caddy (the depends_on
block referencing api with condition: service_healthy), either remove the whole
depends_on entry or change it to a simple depends_on: - api without the
condition (or remove depends_on entirely) so Caddy binds ports 80/443 regardless
of API health; update the caddy service definition accordingly.
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
apps/api/src/middleware/auth.middleware.ts (1)
35-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t swallow JWT secret misconfiguration as token invalidation.
Resolve the secret before the
tryinauthenticateJWT()so config errors surface properly instead of returning401 invalid or expired token.Suggested fix
export function authenticateJWT(req: Request, res: Response, next: NextFunction) { + const secret = getJwtSecret(); let token = ''; const authHeader = req.headers.authorization || ''; @@ try { - req.user = jwt.verify(token, getJwtSecret()) as AuthUserPayload; + req.user = jwt.verify(token, secret) as AuthUserPayload; return next(); } catch (err) { return res.unauthorized('invalid or expired token'); } } @@ export function refreshToken(req: Request, res: Response) { @@ + const secret = getJwtSecret(); const newToken = jwt.sign( @@ - getJwtSecret(), + secret, { expiresIn: '45m' } );Also applies to: 54-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/middleware/auth.middleware.ts` around lines 35 - 37, In authenticateJWT, don't call getJwtSecret() inside the try that wraps jwt.verify; resolve the JWT secret before entering the try so configuration errors throw outside the token-verification catch and aren't misreported as "invalid or expired token". Specifically, call getJwtSecret() and store its result in a local (e.g., secret) before the try, then use that secret in jwt.verify; apply the same change to the other jwt.verify usage in this file (the second occurrence around the logic referenced at lines 54-55) so all secret resolution errors surface correctly.apps/api/src/config/app.ts (1)
55-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForce
secure: truewhensameSiteis"none".Current precedence still allows
COOKIE_SECURE=falseto produceSameSite=None; Secure=false, which browsers reject.Suggested fix
export function getAuthCookieOptions(): CookieOptions { const sameSite = getCookieSameSite(); const secureOverride = parseBoolean(process.env.COOKIE_SECURE); - const secure = secureOverride ?? (sameSite === "none" || process.env.NODE_ENV === "production"); + const secure = + sameSite === "none" + ? true + : (secureOverride ?? process.env.NODE_ENV === "production"); return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/config/app.ts` around lines 55 - 57, The current computation of secure (using secureOverride ?? (sameSite === "none" || process.env.NODE_ENV === "production")) still allows COOKIE_SECURE=false to produce SameSite=None with Secure=false; change the logic in the block that defines secure (which uses secureOverride, parseBoolean, and sameSite) to force secure = true whenever sameSite === "none", otherwise fall back to secureOverride or production default (e.g., secure = sameSite === "none" ? true : (secureOverride ?? process.env.NODE_ENV === "production")). Ensure parseBoolean(process.env.COOKIE_SECURE) is still used for explicit overrides but never permits false when sameSite is "none".apps/api/src/controllers/check-in.controller.ts (1)
35-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate identifier format before service call.
id/appointmentIdpresence is checked, but malformed IDs can still reach the DB path and fail as server errors. Validate format here and returnbadRequest.Suggested fix
export const selectAppointmentForCheckIn = async (req: Request, res: Response, next: NextFunction) => { try { const { id } = req.params; const appointmentId = typeof req.body?.appointmentId === "string" ? req.body.appointmentId : ""; + const uuidPattern = + /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; if (!id || !appointmentId) { return res.badRequest("Check-in ID and appointment ID are required"); } + if (!uuidPattern.test(id)) { + return res.badRequest("Invalid check-in ID"); + } + if (!uuidPattern.test(appointmentId)) { + return res.badRequest("Invalid appointment ID"); + } - const result = await CheckInService.selectAppointmentForCheckIn(id as string, appointmentId); + const result = await CheckInService.selectAppointmentForCheckIn(id, appointmentId); return res.ok(result, "Appointment selected successfully"); } catch (error) { next(error); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/controllers/check-in.controller.ts` around lines 35 - 40, Validate the format of id and appointmentId in the controller before calling CheckInService.selectAppointmentForCheckIn: check that both id and appointmentId are present and conform to the expected identifier format (e.g., use the project's existing validator like isValidObjectId or a UUID validator) and if either fails return res.badRequest with a clear message; only call CheckInService.selectAppointmentForCheckIn(id as string, appointmentId) and then res.ok(result, "Appointment selected successfully") when both IDs pass format validation.apps/api/src/services/check-in.service.ts (1)
229-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReinstate appointment status eligibility before linking.
The current guard still allows linking non-active appointments as long as phone/date match. Please block non-eligible statuses in this check.
Suggested fix
+ const eligibleStatuses = new Set(["pending", "reserved", "confirmed"]); + if ( !matchesPhone || + !eligibleStatuses.has(appointment.status) || appointment.scheduledAt < start || appointment.scheduledAt > end ) { throw new HttpError(400, { message: "Appointment is not eligible for this check-in" }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/services/check-in.service.ts` around lines 229 - 235, The current guard in the check-in flow allows linking appointments when phone/date match even if the appointment is not in an eligible status; update the conditional that checks matchesPhone, start, and end to also validate appointment.status is an allowed status (e.g., appointment.status === 'active' or appointment.status in the eligibleStatuses set) and throw the same HttpError(400, { message: "Appointment is not eligible for this check-in" }) when the status is not eligible; modify the conditional around matchesPhone/start/end (the block referencing appointment, matchesPhone, start, end) to include this status check before proceeding to link.
🧹 Nitpick comments (2)
apps/web/src/routes/_layout/_check-in/check-in-2/index.tsx (1)
4-6: ⚡ Quick winUse
replace: truefor legacy route redirects.Prevents stale
/check-in-2entries in browser history and avoids back-button bounce behavior when users navigate away from this deprecated route.♻️ Suggested change
export const Route = createFileRoute('/_layout/_check-in/check-in-2/')({ beforeLoad: () => { - throw redirect({ to: '/check-in' }) + throw redirect({ to: '/check-in', replace: true }) }, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/routes/_layout/_check-in/check-in-2/index.tsx` around lines 4 - 6, The legacy route handler's beforeLoad currently throws redirect({ to: '/check-in' }) which leaves a stale /check-in-2 entry in history; update the redirect call inside beforeLoad (the beforeLoad function in this module) to include replace: true so it throws redirect({ to: '/check-in', replace: true }) to replace the history entry and prevent back-button bounce.apps/api/src/routes/_tests_/appointment.routes.test.ts (1)
133-139: 💤 Low valueConsider adding STAFF role authorization test for check-in.
The test verifies 401 without authentication but doesn't confirm that a STAFF token can successfully check in. This would complete the authorization coverage for this endpoint.
Suggested test addition
it("returns 200 when a STAFF token checks in an appointment", async () => { const response = await request(app) .patch("/appointments/appointment-1/check-in") .set("Authorization", `Bearer ${buildToken("STAFF")}`); expect(response.status).toBe(200); expect(appointmentController.checkInAppointment).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/_tests_/appointment.routes.test.ts` around lines 133 - 139, Add a positive authorization test to ensure STAFF can check in an appointment: in the test suite that contains the existing unauthenticated check-in test, add a new it block that issues a PATCH to "/appointments/appointment-1/check-in" using request(app) with the Authorization header set to `Bearer ${buildToken("STAFF")}`, then assert the response status is 200 and that appointmentController.checkInAppointment was called (e.g., toHaveBeenCalledTimes(1)); reference the existing test pattern and the appointmentController.checkInAppointment and buildToken symbols when adding this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.dockerignore:
- Around line 10-12: The .dockerignore currently ignores .env and .env.* but
does not cover deploy/lightsail/api.env so secret envs can be sent in the build
context; update .dockerignore to explicitly ignore deploy/lightsail/api.env (or
deploy/lightsail/*.env) by adding that pattern as a new line (ensure it is not
negated by any ! patterns) so Lightsail environment files are excluded from the
Docker build context.
In `@apps/api/src/routes/_tests_/admin-only.routes.test.ts`:
- Line 5: The test file currently mutates process.env.JWT_SECRET at module
scope; move that mutation into test lifecycle hooks (e.g., beforeAll or
beforeEach) and restore the original value in afterAll or afterEach to prevent
cross-suite leakage—capture the original value (const orig =
process.env.JWT_SECRET) in the test file, set process.env.JWT_SECRET =
"unit-test-secret" in beforeAll/beforeEach, and restore process.env.JWT_SECRET =
orig in afterAll/afterEach so other suites aren’t affected.
In `@apps/api/src/routes/_tests_/service.routes.test.ts`:
- Line 5: The test mutates process.env.JWT_SECRET at module scope which can leak
across test suites; move the assignment into the test lifecycle (e.g., add a
beforeAll or beforeEach in the same test file) and restore the original value in
an afterAll or afterEach to avoid cross-suite pollution: capture the previous
value of process.env.JWT_SECRET, set process.env.JWT_SECRET = "unit-test-secret"
in beforeAll/beforeEach, and reset process.env.JWT_SECRET back to the captured
value in afterAll/afterEach in
apps/api/src/routes/_tests_/service.routes.test.ts.
In `@apps/web/src/components/admin/header.tsx`:
- Around line 89-93: The button's aria-label currently says "Open admin profile"
while the onClick navigates to either '/admin-profile' or '/admin-services'
based on user?.role; update the aria-label on the same button (the element that
calls navigate) to a neutral or role-aware label (e.g., "Open admin menu" or
compute label conditionally using user?.role to select "Open admin profile" for
ADMIN and "Open admin services" for others) so assistive tech matches the actual
destination.
In `@apps/web/src/utils/appointment.function.ts`:
- Around line 142-156: fetchCustomerAppointments currently accepts a _customer
parameter but never uses it, causing unfiltered fetches; either remove the
unused parameter or implement filtering. Fix by updating
fetchCustomerAppointments to accept and use customer.id/email/phone to call
api.get('/appointments', { params: { customerId, email, phone } }) or apply
client-side filtering to the returned response.data.data (keep the same return
shape CustomerAppointment[]) and update callers of fetchCustomerAppointments (or
remove the parameter) so signatures match; ensure the function name
fetchCustomerAppointments and the api.get('/appointments') call are the points
you change.
In `@apps/web/vite.config.ts`:
- Line 19: Replace the insecure allowedHosts: true with an explicit allowlist in
the Vite dev server config: update the server.allowedHosts property (in the
exported defineConfig/server object where allowedHosts is currently set) to an
array of allowed hostnames or read them from an env variable (e.g.,
['localhost','127.0.0.1','::1', '.your-domain.com']) so only known hosts are
accepted; ensure the same change is applied to the other occurrence of
allowedHosts in the file.
---
Duplicate comments:
In `@apps/api/src/config/app.ts`:
- Around line 55-57: The current computation of secure (using secureOverride ??
(sameSite === "none" || process.env.NODE_ENV === "production")) still allows
COOKIE_SECURE=false to produce SameSite=None with Secure=false; change the logic
in the block that defines secure (which uses secureOverride, parseBoolean, and
sameSite) to force secure = true whenever sameSite === "none", otherwise fall
back to secureOverride or production default (e.g., secure = sameSite === "none"
? true : (secureOverride ?? process.env.NODE_ENV === "production")). Ensure
parseBoolean(process.env.COOKIE_SECURE) is still used for explicit overrides but
never permits false when sameSite is "none".
In `@apps/api/src/controllers/check-in.controller.ts`:
- Around line 35-40: Validate the format of id and appointmentId in the
controller before calling CheckInService.selectAppointmentForCheckIn: check that
both id and appointmentId are present and conform to the expected identifier
format (e.g., use the project's existing validator like isValidObjectId or a
UUID validator) and if either fails return res.badRequest with a clear message;
only call CheckInService.selectAppointmentForCheckIn(id as string,
appointmentId) and then res.ok(result, "Appointment selected successfully") when
both IDs pass format validation.
In `@apps/api/src/middleware/auth.middleware.ts`:
- Around line 35-37: In authenticateJWT, don't call getJwtSecret() inside the
try that wraps jwt.verify; resolve the JWT secret before entering the try so
configuration errors throw outside the token-verification catch and aren't
misreported as "invalid or expired token". Specifically, call getJwtSecret() and
store its result in a local (e.g., secret) before the try, then use that secret
in jwt.verify; apply the same change to the other jwt.verify usage in this file
(the second occurrence around the logic referenced at lines 54-55) so all secret
resolution errors surface correctly.
In `@apps/api/src/services/check-in.service.ts`:
- Around line 229-235: The current guard in the check-in flow allows linking
appointments when phone/date match even if the appointment is not in an eligible
status; update the conditional that checks matchesPhone, start, and end to also
validate appointment.status is an allowed status (e.g., appointment.status ===
'active' or appointment.status in the eligibleStatuses set) and throw the same
HttpError(400, { message: "Appointment is not eligible for this check-in" })
when the status is not eligible; modify the conditional around
matchesPhone/start/end (the block referencing appointment, matchesPhone, start,
end) to include this status check before proceeding to link.
---
Nitpick comments:
In `@apps/api/src/routes/_tests_/appointment.routes.test.ts`:
- Around line 133-139: Add a positive authorization test to ensure STAFF can
check in an appointment: in the test suite that contains the existing
unauthenticated check-in test, add a new it block that issues a PATCH to
"/appointments/appointment-1/check-in" using request(app) with the Authorization
header set to `Bearer ${buildToken("STAFF")}`, then assert the response status
is 200 and that appointmentController.checkInAppointment was called (e.g.,
toHaveBeenCalledTimes(1)); reference the existing test pattern and the
appointmentController.checkInAppointment and buildToken symbols when adding this
case.
In `@apps/web/src/routes/_layout/_check-in/check-in-2/index.tsx`:
- Around line 4-6: The legacy route handler's beforeLoad currently throws
redirect({ to: '/check-in' }) which leaves a stale /check-in-2 entry in history;
update the redirect call inside beforeLoad (the beforeLoad function in this
module) to include replace: true so it throws redirect({ to: '/check-in',
replace: true }) to replace the history entry and prevent back-button bounce.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa6445da-56d7-46e6-981f-329f3929725b
📒 Files selected for processing (59)
.dockerignore.gitignoreapps/api/.env.exampleapps/api/Dockerfileapps/api/src/config/_tests_/database-startup.test.tsapps/api/src/config/app.tsapps/api/src/config/database-startup.tsapps/api/src/controllers/appointment.controller.tsapps/api/src/controllers/auth.controller.tsapps/api/src/controllers/check-in.controller.tsapps/api/src/middleware/auth.middleware.tsapps/api/src/routes/_tests_/admin-only.routes.test.tsapps/api/src/routes/_tests_/appointment.routes.test.tsapps/api/src/routes/_tests_/auth.routes.test.tsapps/api/src/routes/_tests_/check-in.routes.test.tsapps/api/src/routes/_tests_/health.routes.test.tsapps/api/src/routes/_tests_/service.routes.test.tsapps/api/src/routes/appointment.routes.tsapps/api/src/routes/auth.routes.tsapps/api/src/routes/check-in.routes.tsapps/api/src/routes/email.routes.tsapps/api/src/routes/health.routes.tsapps/api/src/routes/permission.routes.tsapps/api/src/routes/role.routes.tsapps/api/src/routes/service.routes.tsapps/api/src/server.tsapps/api/src/services/_tests_/appointment.service.test.tsapps/api/src/services/_tests_/auth.service.test.tsapps/api/src/services/appointment.service.tsapps/api/src/services/auth.service.tsapps/api/src/services/check-in.service.tsapps/api/src/utils/zod_schemas/appointment.schema.tsapps/web/src/components/admin/header.tsxapps/web/src/components/admin/sidebar.tsxapps/web/src/components/auth/AuthProvider.tsxapps/web/src/layout/Header.tsxapps/web/src/lib/adminApi.tsapps/web/src/lib/api.tsapps/web/src/lib/runtime.test.tsapps/web/src/lib/runtime.tsapps/web/src/pages/CheckIn.tsxapps/web/src/pages/bookings/BookingStepFour.tsxapps/web/src/pages/bookings/BookingStepThree.tsxapps/web/src/routeTree.gen.tsapps/web/src/routes/_adminLayout/admin-profile/index.tsxapps/web/src/routes/_layout/_check-in/check-in-1/index.tsxapps/web/src/routes/_layout/_check-in/check-in-2/index.tsxapps/web/src/routes/_layout/_check-in/check-in-3/index.tsxapps/web/src/routes/_layout/_check-in/check-in-4/index.tsxapps/web/src/stores/auth.store.tsapps/web/src/utils/appointment.function.tsapps/web/src/utils/auth.function.tsapps/web/vite.config.tsdeploy/lightsail/.env.exampledeploy/lightsail/Caddyfiledeploy/lightsail/api.env.exampledeploy/lightsail/compose.ymldocs/deploy-lightsail.mdpackage.json
💤 Files with no reviewable changes (1)
- apps/web/src/stores/auth.store.ts
Comment on lines
+142
to
156
| export async function fetchCustomerAppointments(_customer: { | ||
| id?: string | null | ||
| email?: string | null | ||
| phone?: string | null | ||
| }) { | ||
| try { | ||
| const response = await api.get('/appointments') | ||
| const appointments = Array.isArray(response.data.data) | ||
| return Array.isArray(response.data.data) | ||
| ? (response.data.data as CustomerAppointment[]) | ||
| : [] | ||
|
|
||
| return appointments.filter((appointment) => { | ||
| if (customer.id && appointment.customerId === customer.id) return true | ||
| if (customer.id && appointment.customer?.id === customer.id) return true | ||
| if (customer.email && appointment.customer?.email === customer.email) { | ||
| return true | ||
| } | ||
| if (customer.phone && appointment.customer?.phone === customer.phone) { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| }) | ||
| } catch (error) { | ||
| console.error('Error fetching customer appointments:', error) | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
fetchCustomerAppointments ignores the customer parameter.
The _customer parameter is accepted but never used—the function now fetches all appointments from /appointments without any filtering. If this is intentional (e.g., server-side filtering is expected), the unused parameter should be removed or documented. If client-side filtering was previously in place, this is a behavior change.
#!/bin/bash
# Check how fetchCustomerAppointments is called to understand if callers expect filtering
rg -n -A3 'fetchCustomerAppointments' --type=ts --type=tsx -g '!*.test.*' -g '!*.spec.*'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/utils/appointment.function.ts` around lines 142 - 156,
fetchCustomerAppointments currently accepts a _customer parameter but never uses
it, causing unfiltered fetches; either remove the unused parameter or implement
filtering. Fix by updating fetchCustomerAppointments to accept and use
customer.id/email/phone to call api.get('/appointments', { params: { customerId,
email, phone } }) or apply client-side filtering to the returned
response.data.data (keep the same return shape CustomerAppointment[]) and update
callers of fetchCustomerAppointments (or remove the parameter) so signatures
match; ensure the function name fetchCustomerAppointments and the
api.get('/appointments') call are the points you change.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
apps/api/src/middleware/auth.middleware.ts (1)
35-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSurface JWT misconfiguration instead of returning
401.
getJwtSecret()can throw for a missing or placeholder secret, but thistry/catchcurrently converts that server-side misconfiguration intoinvalid or expired token. Pull the secret lookup out of thetryso config errors fail loudly instead of being masked as auth failures.Suggested patch
export function authenticateJWT(req: Request, res: Response, next: NextFunction) { + const secret = getJwtSecret(); let token = ''; const authHeader = req.headers.authorization || ''; @@ try { - req.user = jwt.verify(token, getJwtSecret()) as AuthUserPayload; + req.user = jwt.verify(token, secret) as AuthUserPayload; return next(); } catch (err) { return res.unauthorized('invalid or expired token'); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/middleware/auth.middleware.ts` around lines 35 - 39, The current try/catch masks server-side JWT secret configuration errors because getJwtSecret() is called inside the try; move the call to getJwtSecret() out of the try so any thrown configuration errors surface instead of being converted to a 401, then use the returned secret variable in the jwt.verify(...) call inside the try/catch that still returns res.unauthorized('invalid or expired token') on verification failures; update references to getJwtSecret(), jwt.verify, and req.user accordingly.apps/api/src/config/app.ts (1)
53-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let
COOKIE_SECURE=falsebreakSameSite=Nonecookies.Line 56 still allows
COOKIE_SECURE=falseto win oversameSite === "none". Browsers reject that combination, so auth cookies may never persist.Suggested patch
export function getAuthCookieOptions(): CookieOptions { const sameSite = getCookieSameSite(); const secureOverride = parseBoolean(process.env.COOKIE_SECURE); - const secure = secureOverride ?? (sameSite === "none" || process.env.NODE_ENV === "production"); + const secure = + sameSite === "none" + ? true + : (secureOverride ?? process.env.NODE_ENV === "production"); return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/config/app.ts` around lines 53 - 57, The current getAuthCookieOptions() computes secure so COOKIE_SECURE=false can override sameSite === "none", which breaks cookies; change the logic in getAuthCookieOptions (variables sameSite, secureOverride, secure) so that if sameSite === "none" then secure is always true, otherwise fall back to secureOverride ?? (process.env.NODE_ENV === "production"); update the secure assignment in getAuthCookieOptions accordingly to enforce Secure for SameSite=None.apps/api/src/services/check-in.service.ts (1)
229-235:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-active appointments in this eligibility check.
This path still accepts any appointment whose phone/date match, so a direct call can attach
canceled,checked_in, orcompletedappointments to the check-in. Please keep this guard aligned with the active-status filtering already used earlier in this file.Suggested patch
+ const eligibleStatuses = new Set(["pending", "reserved", "confirmed"]); + if ( !matchesPhone || + !eligibleStatuses.has(appointment.status) || appointment.scheduledAt < start || appointment.scheduledAt > end ) { throw new HttpError(400, { message: "Appointment is not eligible for this check-in" }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/services/check-in.service.ts` around lines 229 - 235, The current eligibility guard only checks matchesPhone and scheduledAt range and thus allows canceled/checked_in/completed appointments; extend the condition to reject any appointment whose status is not active by adding a status check to the same if that throws the HttpError (e.g., include "!activeStatuses.includes(appointment.status)" or "appointment.status !== AppointmentStatus.ACTIVE" consistent with the active-status check used earlier in this file), referencing matchesPhone, appointment.scheduledAt, start, end and leaving the thrown HttpError as-is so non-active appointments are rejected here the same way they are filtered elsewhere.apps/api/src/config/database-startup.ts (1)
7-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProduction guard is still bypassed when
DB_SYNC_ALTER=true.
DB_SYNC_ALTERis evaluated before the production check, so production can still runsync({ alter: true })whenDB_SYNCis not explicitly enabled.Suggested fix
export function getDatabaseSyncOptions(env: StartupEnv = process.env): SyncOptions | undefined | null { + const isProduction = env.NODE_ENV === "production"; + + if (isProduction && env.DB_SYNC !== "true") { + return null; + } + if (env.DB_SYNC_ALTER === "true") { return { alter: true }; } if (env.DB_SYNC === "true") { return {}; } - - if (env.NODE_ENV === "production" && env.DB_SYNC !== "true") { - return null; - } return undefined; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/config/database-startup.ts` around lines 7 - 16, The current ordering checks DB_SYNC_ALTER before the production guard, allowing sync({ alter: true }) in production; change the logic in the function that determines DB sync options so the production guard (env.NODE_ENV === "production" && env.DB_SYNC !== "true") runs before any DB_SYNC_ALTER or DB_SYNC checks — return null for production when DB_SYNC is not explicitly "true", then handle DB_SYNC_ALTER and DB_SYNC afterward (use env.DB_SYNC_ALTER and env.DB_SYNC in that order) so alter mode cannot bypass the production guard.apps/api/src/controllers/check-in.controller.ts (1)
35-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate UUID format for both IDs before calling the service.
Presence checks alone are not enough; malformed IDs can bubble into DB errors and become 500 responses instead of clean 400s.
Suggested fix
+const UUID_RE = + /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; + export const selectAppointmentForCheckIn = async (req: Request, res: Response, next: NextFunction) => { try { const { id } = req.params; const appointmentId = typeof req.body?.appointmentId === "string" ? req.body.appointmentId : ""; if (!id || !appointmentId) { return res.badRequest("Check-in ID and appointment ID are required"); } + + if (!UUID_RE.test(id)) { + return res.badRequest("Invalid check-in ID"); + } + + if (!UUID_RE.test(appointmentId)) { + return res.badRequest("Invalid appointment ID"); + } const result = await CheckInService.selectAppointmentForCheckIn(id as string, appointmentId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/controllers/check-in.controller.ts` around lines 35 - 39, The controller currently only checks presence of id and appointmentId but must also validate they are proper UUIDs before calling CheckInService.selectAppointmentForCheckIn; add a UUID format check (using your project's UUID helper or a validator like isUUID/validate from 'uuid' or validator.isUUID) for both variables (id and appointmentId) and if either fails call res.badRequest with a clear message (e.g., "Invalid check-in ID" / "Invalid appointment ID") instead of proceeding to the service; keep the existing presence checks and ensure the validation occurs prior to invoking CheckInService.selectAppointmentForCheckIn so malformed IDs return 400 not 500.
🧹 Nitpick comments (8)
apps/api/src/config/_tests_/database-startup.test.ts (1)
31-36: ⚡ Quick winAdd a regression test for
NODE_ENV=production+DB_SYNC_ALTER=true.Current tests don’t lock the production safety behavior for alter mode. Add one assertion that this combination does not enable sync/alter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/config/_tests_/database-startup.test.ts` around lines 31 - 36, Add a regression test asserting that getDatabaseSyncOptions does not enable alter when NODE_ENV is "production" even if DB_SYNC_ALTER is "true": create a new it(...) case (in the same test suite) that calls getDatabaseSyncOptions with { NODE_ENV: "production", DB_SYNC_ALTER: "true" } and expects the result to equal {} (or not contain alter: true) so production safety is preserved; reference the getDatabaseSyncOptions function used in database-startup.test.ts to locate where to add this assertion.apps/web/src/pages/bookings/BookingStepThree.tsx (1)
95-109: ⚡ Quick winConsider reverting
selectedStaffIdon API failure.The current flow updates UI state optimistically but doesn't revert on error. If the API call fails, the UI shows the new staff selection while the server still has the old value.
💡 Suggested improvement
const handleStaffClick = async (staffId: string) => { + const previousStaffId = selectedStaffId try { setIsUpdating(true) setSelectedStaffId(staffId) // If we have an active reservation, update it to the specific staff if (reservationId && staffId !== 'anyone') { await updateReservedAppointment(reservationId, { staffId }) } } catch (err: unknown) { + setSelectedStaffId(previousStaffId) toast.error(ErrorHandler.getMessage(err)) } finally { setIsUpdating(false) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/pages/bookings/BookingStepThree.tsx` around lines 95 - 109, In handleStaffClick, preserve the previous selectedStaffId before optimistically calling setSelectedStaffId(staffId), then if updateReservedAppointment(reservationId, { staffId }) throws, restore the old value in the catch block (e.g., setSelectedStaffId(previousId)) and still call toast.error; keep the setIsUpdating(true)/finally setIsUpdating(false) behavior unchanged and only perform the revert when an API update was attempted (reservationId && staffId !== 'anyone') to avoid unnecessarily reverting local-only selections.apps/api/src/services/appointment.service.ts (1)
161-163: 💤 Low valueRemove redundant try/catch that only rethrows.
The catch block only rethrows the error without any additional handling. This pattern adds noise without value.
getAllAppointments: async (actor?: AppointmentActor) => { - try { await completeElapsedCheckedInAppointments(); const where = actor && !isPrivilegedActor(actor) ? { customerId: actor.sub } : undefined; const appointments = await Appointment.findAll({ ...(where ? { where } : {}), include: [ { model: Service, as: "services" }, { model: User, as: "customer" }, { model: User, as: "staff" }, ], order: [["scheduledAt", "ASC"]], }); return appointments; - } catch (err) { - throw err; - } },The same pattern exists in
getAppointmentById(lines 375-377).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/services/appointment.service.ts` around lines 161 - 163, The try/catch blocks in the service that simply "catch (err) { throw err; }" are redundant—remove the surrounding try/catch in the affected functions (the one shown and the getAppointmentById function) so errors bubble naturally; if special handling/logging is required later, add it there, but for now delete the empty catch blocks around the logic in appointment.service.ts to simplify the code and preserve stack traces.deploy/lightsail/Caddyfile (1)
13-13: ⚡ Quick winUse
{$WEB_ROOT}environment variable instead of hardcoded path.The
.env.exampledefinesWEB_ROOT=/srv/nail-star/web, but this line hardcodes the same path. Using the variable improves configurability.- root * /srv/nail-star/web + root * {$WEB_ROOT}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/lightsail/Caddyfile` at line 13, Replace the hardcoded web root path in the Caddyfile directive "root * /srv/nail-star/web" with the environment variable token {$WEB_ROOT}; locate the "root * /srv/nail-star/web" line and change it to use {$WEB_ROOT} so the server reads the path from the WEB_ROOT env var defined in .env/example.docs/deploy-lightsail.md (1)
21-26: 💤 Low valueConsider adding file permission guidance for sensitive env files.
The
api.envfile contains database credentials, JWT secrets, and SMTP credentials. Consider documenting that this file should have restricted permissions (e.g.,chmod 600 deploy/lightsail/api.env) to prevent accidental exposure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/deploy-lightsail.md` around lines 21 - 26, Add guidance to restrict permissions on the sensitive env files: instruct users to set strict file permissions (e.g., chmod 600) and appropriate ownership for deploy/lightsail/api.env (and deploy/lightsail/.env if it contains secrets), and to ensure these files are not committed to source control (e.g., via .gitignore). Reference the deploy/lightsail/api.env and deploy/lightsail/.env file names in the docs so readers know which files to secure.apps/api/src/routes/_tests_/appointment.routes.test.ts (1)
133-139: 💤 Low valueConsider adding positive test case for STAFF role check-in.
The test validates that unauthenticated requests return 401, but doesn't verify that a STAFF token successfully reaches the controller. Adding a test with
buildToken("STAFF")would confirm theauthorizeRoles("staff")middleware works correctly for the happy path.Example test case
it("returns 200 when a STAFF token checks in an appointment", async () => { const response = await request(app) .patch("/appointments/appointment-1/check-in") .set("Authorization", `Bearer ${buildToken("STAFF")}`); expect(response.status).toBe(200); expect(appointmentController.checkInAppointment).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/_tests_/appointment.routes.test.ts` around lines 133 - 139, Add a positive test that verifies the authorizeRoles("staff") middleware allows STAFF users to reach the controller: call request(app).patch("/appointments/appointment-1/check-in") and set the Authorization header to `Bearer ${buildToken("STAFF")}`, then assert response.status is 200 and that appointmentController.checkInAppointment was called once (use toHaveBeenCalledTimes(1)) to confirm the happy-path behavior.apps/api/Dockerfile (1)
23-28: 💤 Low valueRedundant
chownafterCOPY --chown.Line 28 runs
chown -R node:node /appafter the COPY commands already use--chown=node:node. This adds unnecessary build time and creates an extra layer.Suggested fix
COPY --from=build --chown=node:node /app/package.json /app/package-lock.json ./ COPY --from=build --chown=node:node /app/node_modules ./node_modules COPY --from=build --chown=node:node /app/apps/api/package.json ./apps/api/package.json COPY --from=build --chown=node:node /app/apps/api/dist ./apps/api/dist -RUN chown -R node:node /app - USER node🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/Dockerfile` around lines 23 - 28, The RUN chown -R node:node /app line is redundant because the preceding COPY instructions already use --chown=node:node; remove the RUN chown step from the Dockerfile (delete the RUN chown -R node:node /app line) to avoid the extra layer and build time; keep the COPY --from=build --chown=node:node ... lines (the ones that copy package.json, node_modules, apps/api/package.json, and apps/api/dist) as-is so ownership is preserved by those COPY commands.apps/web/src/utils/appointment.function.ts (1)
142-151: ⚡ Quick winThe signature still implies customer-based filtering.
This helper ignores
_customerand just proxiesGET /appointments. Either drop the argument or rename the helper so callers do not assume the passed customer object affects the result.✏️ Suggested refactor
-export async function fetchCustomerAppointments(_customer: { - id?: string | null - email?: string | null - phone?: string | null -}) { +export async function fetchCurrentUserAppointments() { try { const response = await api.get('/appointments') return Array.isArray(response.data.data) ? (response.data.data as CustomerAppointment[]) : []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/utils/appointment.function.ts` around lines 142 - 151, The function fetchCustomerAppointments currently accepts a _customer parameter but ignores it and always calls api.get('/appointments'), misleading callers; either remove the unused _customer parameter from fetchCustomerAppointments (and update all callers) or implement customer-based filtering (e.g., build query params from the provided customer object and call api.get('/appointments', { params })) so the passed id/email/phone affect the request — locate the function fetchCustomerAppointments and the api.get('/appointments') call to apply the chosen change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/controllers/appointment.controller.ts`:
- Around line 85-116: The controller currently awaits sendMail in appointment
confirmation which ties API latency/availability to SMTP; change the call in
appointment.controller.ts so sending is fire-and-forget instead of awaited:
invoke sendMail(...) without awaiting (e.g., call it and attach a .catch to log
errors or use void/sendMail(...).catch(...)) so confirmation returns immediately
while still logging any email errors; keep the same arguments (sendMail,
getBookingConfirmationTemplate, and the computed template values) and remove the
surrounding await/try that blocks the response.
- Around line 93-104: The date/time formatters are hard-coded to "Asia/Bangkok"
and must use the salon's actual timezone when formatting scheduledAt; update the
two Intl.DateTimeFormat calls in
apps/api/src/controllers/appointment.controller.ts to accept a timezone variable
(e.g., salon.timeZone or salon.timezone) instead of the literal "Asia/Bangkok",
defaulting to a sensible fallback like "UTC" if the salon timezone is missing,
and apply that same timezone variable to both the date and time formatters that
format scheduledAt.
In `@apps/api/src/routes/_tests_/admin-only.routes.test.ts`:
- Around line 5-6: The test mutates process.env.JWT_SECRET at module scope; move
that assignment into a beforeAll hook and add an afterAll hook to restore the
original value so other suites aren't affected. Specifically, in
admin-only.routes.test.ts remove the top-level assignment to
process.env.JWT_SECRET, add a beforeAll that saves the original (e.g., const
prevJwtSecret = process.env.JWT_SECRET) and sets process.env.JWT_SECRET =
"unit-test-secret", and add an afterAll that restores process.env.JWT_SECRET =
prevJwtSecret (or deletes it if undefined). Ensure the hooks wrap the whole
suite so tests see the test secret only during execution.
In `@apps/api/src/routes/_tests_/service.routes.test.ts`:
- Around line 5-6: The JWT secret is being set at module load; move the
assignment into a test lifecycle hook: capture the original value (e.g., const
originalJwt = process.env.JWT_SECRET) and set process.env.JWT_SECRET =
"unit-test-secret" in a beforeAll() block, then restore process.env.JWT_SECRET =
originalJwt in an afterAll() block; update the file containing the tests
(service.routes.test.ts) and apply the same pattern to the other route test
files referenced (dashboard, auth, appointment, admin-only) to avoid cross-suite
state leakage.
In `@apps/web/src/components/admin/header.tsx`:
- Around line 89-96: The avatar button's static aria-label is incorrect after
role-based routing; update the aria-label to be conditional alongside the
navigate call (based on user?.role === 'ADMIN') so it matches the destination
used in navigate (e.g., "Open admin profile" for ADMIN and "Open admin services"
or similar for others); locate the button that uses navigate and user?.role in
header.tsx and change the aria-label prop to compute the correct label from the
same condition.
In `@apps/web/src/utils/appointment.function.ts`:
- Around line 132-135: The helper fetchAppointmentById is misnamed because it
calls the reservation-only endpoint "/appointments/reserve/:id"; rename the
function to fetchReservationById (and update its export) so its name matches
behavior, update all call sites/imports that reference fetchAppointmentById to
use fetchReservationById, and ensure any tests or docs referencing the old name
are updated; keep the same implementation (await
api.get(`/appointments/reserve/${id}`) and return response.data.data) but update
the identifier to avoid confusion with confirmed appointment APIs.
In `@apps/web/vite.config.ts`:
- Around line 17-24: The Vite config currently sets server.allowedHosts and
preview.allowedHosts to true (in the server and preview config blocks), which
disables host validation; change both allowedHosts settings to a restrictive
array like ['localhost', '127.0.0.1'] to limit host access and mitigate
DNS-rebinding risk, updating the server and preview objects in vite.config.ts
accordingly so the dev and preview servers only accept those hosts.
---
Duplicate comments:
In `@apps/api/src/config/app.ts`:
- Around line 53-57: The current getAuthCookieOptions() computes secure so
COOKIE_SECURE=false can override sameSite === "none", which breaks cookies;
change the logic in getAuthCookieOptions (variables sameSite, secureOverride,
secure) so that if sameSite === "none" then secure is always true, otherwise
fall back to secureOverride ?? (process.env.NODE_ENV === "production"); update
the secure assignment in getAuthCookieOptions accordingly to enforce Secure for
SameSite=None.
In `@apps/api/src/config/database-startup.ts`:
- Around line 7-16: The current ordering checks DB_SYNC_ALTER before the
production guard, allowing sync({ alter: true }) in production; change the logic
in the function that determines DB sync options so the production guard
(env.NODE_ENV === "production" && env.DB_SYNC !== "true") runs before any
DB_SYNC_ALTER or DB_SYNC checks — return null for production when DB_SYNC is not
explicitly "true", then handle DB_SYNC_ALTER and DB_SYNC afterward (use
env.DB_SYNC_ALTER and env.DB_SYNC in that order) so alter mode cannot bypass the
production guard.
In `@apps/api/src/controllers/check-in.controller.ts`:
- Around line 35-39: The controller currently only checks presence of id and
appointmentId but must also validate they are proper UUIDs before calling
CheckInService.selectAppointmentForCheckIn; add a UUID format check (using your
project's UUID helper or a validator like isUUID/validate from 'uuid' or
validator.isUUID) for both variables (id and appointmentId) and if either fails
call res.badRequest with a clear message (e.g., "Invalid check-in ID" / "Invalid
appointment ID") instead of proceeding to the service; keep the existing
presence checks and ensure the validation occurs prior to invoking
CheckInService.selectAppointmentForCheckIn so malformed IDs return 400 not 500.
In `@apps/api/src/middleware/auth.middleware.ts`:
- Around line 35-39: The current try/catch masks server-side JWT secret
configuration errors because getJwtSecret() is called inside the try; move the
call to getJwtSecret() out of the try so any thrown configuration errors surface
instead of being converted to a 401, then use the returned secret variable in
the jwt.verify(...) call inside the try/catch that still returns
res.unauthorized('invalid or expired token') on verification failures; update
references to getJwtSecret(), jwt.verify, and req.user accordingly.
In `@apps/api/src/services/check-in.service.ts`:
- Around line 229-235: The current eligibility guard only checks matchesPhone
and scheduledAt range and thus allows canceled/checked_in/completed
appointments; extend the condition to reject any appointment whose status is not
active by adding a status check to the same if that throws the HttpError (e.g.,
include "!activeStatuses.includes(appointment.status)" or "appointment.status
!== AppointmentStatus.ACTIVE" consistent with the active-status check used
earlier in this file), referencing matchesPhone, appointment.scheduledAt, start,
end and leaving the thrown HttpError as-is so non-active appointments are
rejected here the same way they are filtered elsewhere.
---
Nitpick comments:
In `@apps/api/Dockerfile`:
- Around line 23-28: The RUN chown -R node:node /app line is redundant because
the preceding COPY instructions already use --chown=node:node; remove the RUN
chown step from the Dockerfile (delete the RUN chown -R node:node /app line) to
avoid the extra layer and build time; keep the COPY --from=build
--chown=node:node ... lines (the ones that copy package.json, node_modules,
apps/api/package.json, and apps/api/dist) as-is so ownership is preserved by
those COPY commands.
In `@apps/api/src/config/_tests_/database-startup.test.ts`:
- Around line 31-36: Add a regression test asserting that getDatabaseSyncOptions
does not enable alter when NODE_ENV is "production" even if DB_SYNC_ALTER is
"true": create a new it(...) case (in the same test suite) that calls
getDatabaseSyncOptions with { NODE_ENV: "production", DB_SYNC_ALTER: "true" }
and expects the result to equal {} (or not contain alter: true) so production
safety is preserved; reference the getDatabaseSyncOptions function used in
database-startup.test.ts to locate where to add this assertion.
In `@apps/api/src/routes/_tests_/appointment.routes.test.ts`:
- Around line 133-139: Add a positive test that verifies the
authorizeRoles("staff") middleware allows STAFF users to reach the controller:
call request(app).patch("/appointments/appointment-1/check-in") and set the
Authorization header to `Bearer ${buildToken("STAFF")}`, then assert
response.status is 200 and that appointmentController.checkInAppointment was
called once (use toHaveBeenCalledTimes(1)) to confirm the happy-path behavior.
In `@apps/api/src/services/appointment.service.ts`:
- Around line 161-163: The try/catch blocks in the service that simply "catch
(err) { throw err; }" are redundant—remove the surrounding try/catch in the
affected functions (the one shown and the getAppointmentById function) so errors
bubble naturally; if special handling/logging is required later, add it there,
but for now delete the empty catch blocks around the logic in
appointment.service.ts to simplify the code and preserve stack traces.
In `@apps/web/src/pages/bookings/BookingStepThree.tsx`:
- Around line 95-109: In handleStaffClick, preserve the previous selectedStaffId
before optimistically calling setSelectedStaffId(staffId), then if
updateReservedAppointment(reservationId, { staffId }) throws, restore the old
value in the catch block (e.g., setSelectedStaffId(previousId)) and still call
toast.error; keep the setIsUpdating(true)/finally setIsUpdating(false) behavior
unchanged and only perform the revert when an API update was attempted
(reservationId && staffId !== 'anyone') to avoid unnecessarily reverting
local-only selections.
In `@apps/web/src/utils/appointment.function.ts`:
- Around line 142-151: The function fetchCustomerAppointments currently accepts
a _customer parameter but ignores it and always calls api.get('/appointments'),
misleading callers; either remove the unused _customer parameter from
fetchCustomerAppointments (and update all callers) or implement customer-based
filtering (e.g., build query params from the provided customer object and call
api.get('/appointments', { params })) so the passed id/email/phone affect the
request — locate the function fetchCustomerAppointments and the
api.get('/appointments') call to apply the chosen change.
In `@deploy/lightsail/Caddyfile`:
- Line 13: Replace the hardcoded web root path in the Caddyfile directive "root
* /srv/nail-star/web" with the environment variable token {$WEB_ROOT}; locate
the "root * /srv/nail-star/web" line and change it to use {$WEB_ROOT} so the
server reads the path from the WEB_ROOT env var defined in .env/example.
In `@docs/deploy-lightsail.md`:
- Around line 21-26: Add guidance to restrict permissions on the sensitive env
files: instruct users to set strict file permissions (e.g., chmod 600) and
appropriate ownership for deploy/lightsail/api.env (and deploy/lightsail/.env if
it contains secrets), and to ensure these files are not committed to source
control (e.g., via .gitignore). Reference the deploy/lightsail/api.env and
deploy/lightsail/.env file names in the docs so readers know which files to
secure.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b1cb674-3ee9-4b38-92b0-15f9683a66cd
📒 Files selected for processing (59)
.dockerignore.gitignoreapps/api/.env.exampleapps/api/Dockerfileapps/api/src/config/_tests_/database-startup.test.tsapps/api/src/config/app.tsapps/api/src/config/database-startup.tsapps/api/src/controllers/appointment.controller.tsapps/api/src/controllers/auth.controller.tsapps/api/src/controllers/check-in.controller.tsapps/api/src/middleware/auth.middleware.tsapps/api/src/routes/_tests_/admin-only.routes.test.tsapps/api/src/routes/_tests_/appointment.routes.test.tsapps/api/src/routes/_tests_/auth.routes.test.tsapps/api/src/routes/_tests_/check-in.routes.test.tsapps/api/src/routes/_tests_/health.routes.test.tsapps/api/src/routes/_tests_/service.routes.test.tsapps/api/src/routes/appointment.routes.tsapps/api/src/routes/auth.routes.tsapps/api/src/routes/check-in.routes.tsapps/api/src/routes/email.routes.tsapps/api/src/routes/health.routes.tsapps/api/src/routes/permission.routes.tsapps/api/src/routes/role.routes.tsapps/api/src/routes/service.routes.tsapps/api/src/server.tsapps/api/src/services/_tests_/appointment.service.test.tsapps/api/src/services/_tests_/auth.service.test.tsapps/api/src/services/appointment.service.tsapps/api/src/services/auth.service.tsapps/api/src/services/check-in.service.tsapps/api/src/utils/zod_schemas/appointment.schema.tsapps/web/src/components/admin/header.tsxapps/web/src/components/admin/sidebar.tsxapps/web/src/components/auth/AuthProvider.tsxapps/web/src/layout/Header.tsxapps/web/src/lib/adminApi.tsapps/web/src/lib/api.tsapps/web/src/lib/runtime.test.tsapps/web/src/lib/runtime.tsapps/web/src/pages/CheckIn.tsxapps/web/src/pages/bookings/BookingStepFour.tsxapps/web/src/pages/bookings/BookingStepThree.tsxapps/web/src/routeTree.gen.tsapps/web/src/routes/_adminLayout/admin-profile/index.tsxapps/web/src/routes/_layout/_check-in/check-in-1/index.tsxapps/web/src/routes/_layout/_check-in/check-in-2/index.tsxapps/web/src/routes/_layout/_check-in/check-in-3/index.tsxapps/web/src/routes/_layout/_check-in/check-in-4/index.tsxapps/web/src/stores/auth.store.tsapps/web/src/utils/appointment.function.tsapps/web/src/utils/auth.function.tsapps/web/vite.config.tsdeploy/lightsail/.env.exampledeploy/lightsail/Caddyfiledeploy/lightsail/api.env.exampledeploy/lightsail/compose.ymldocs/deploy-lightsail.mdpackage.json
💤 Files with no reviewable changes (1)
- apps/web/src/stores/auth.store.ts
Comment on lines
+85
to
+116
| try { | ||
| await sendMail( | ||
| email, | ||
| "Your Nail Session is Confirmed! - Nail Star", | ||
| getBookingConfirmationTemplate({ | ||
| fullName: customerName ?? appointment.customerName ?? "Guest", | ||
| email, | ||
| phone: phoneNumber ?? appointment.phoneNumber ?? "", | ||
| date: new Intl.DateTimeFormat("en-GB", { | ||
| day: "2-digit", | ||
| month: "2-digit", | ||
| year: "numeric", | ||
| timeZone: "Asia/Bangkok", | ||
| }).format(scheduledAt), | ||
| time: new Intl.DateTimeFormat("en-GB", { | ||
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| hour12: false, | ||
| timeZone: "Asia/Bangkok", | ||
| }).format(scheduledAt), | ||
| isGuest: !customerId, | ||
| services: services.map((service: any) => ({ | ||
| name: service.name, | ||
| price: Number(service.promotionPrice ?? service.startingPrice ?? 0), | ||
| categoryName: service.categoryInfo?.name ?? "", | ||
| })), | ||
| totalPrice, | ||
| }) | ||
| ); | ||
| } catch (emailError) { | ||
| console.error("Booking confirmation email failed:", emailError); | ||
| } |
There was a problem hiding this comment.
Don't wait on SMTP before returning confirmation success.
The appointment is already confirmed before the mail send starts. Because apps/api/src/services/email.service.ts:11-29 awaits nodemailer and rethrows, this path now makes confirmation latency and availability depend on the mail provider even when the write already succeeded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/controllers/appointment.controller.ts` around lines 85 - 116,
The controller currently awaits sendMail in appointment confirmation which ties
API latency/availability to SMTP; change the call in appointment.controller.ts
so sending is fire-and-forget instead of awaited: invoke sendMail(...) without
awaiting (e.g., call it and attach a .catch to log errors or use
void/sendMail(...).catch(...)) so confirmation returns immediately while still
logging any email errors; keep the same arguments (sendMail,
getBookingConfirmationTemplate, and the computed template values) and remove the
surrounding await/try that blocks the response.
Comment on lines
+93
to
+104
| date: new Intl.DateTimeFormat("en-GB", { | ||
| day: "2-digit", | ||
| month: "2-digit", | ||
| year: "numeric", | ||
| timeZone: "Asia/Bangkok", | ||
| }).format(scheduledAt), | ||
| time: new Intl.DateTimeFormat("en-GB", { | ||
| hour: "2-digit", | ||
| minute: "2-digit", | ||
| hour12: false, | ||
| timeZone: "Asia/Bangkok", | ||
| }).format(scheduledAt), |
There was a problem hiding this comment.
Use the salon's local timezone in the email.
This email is branded for Addison, Texas in apps/api/src/utils/email-template.util.ts:104-126, but both formatters hard-code Asia/Bangkok. Customers will receive the wrong local date/time.
🕒 Suggested fix
+ const appointmentTimeZone = "America/Chicago";
+
try {
await sendMail(
email,
"Your Nail Session is Confirmed! - Nail Star",
getBookingConfirmationTemplate({
@@
- date: new Intl.DateTimeFormat("en-GB", {
+ date: new Intl.DateTimeFormat("en-US", {
day: "2-digit",
month: "2-digit",
year: "numeric",
- timeZone: "Asia/Bangkok",
+ timeZone: appointmentTimeZone,
}).format(scheduledAt),
- time: new Intl.DateTimeFormat("en-GB", {
+ time: new Intl.DateTimeFormat("en-US", {
hour: "2-digit",
minute: "2-digit",
hour12: false,
- timeZone: "Asia/Bangkok",
+ timeZone: appointmentTimeZone,
}).format(scheduledAt),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/controllers/appointment.controller.ts` around lines 93 - 104,
The date/time formatters are hard-coded to "Asia/Bangkok" and must use the
salon's actual timezone when formatting scheduledAt; update the two
Intl.DateTimeFormat calls in apps/api/src/controllers/appointment.controller.ts
to accept a timezone variable (e.g., salon.timeZone or salon.timezone) instead
of the literal "Asia/Bangkok", defaulting to a sensible fallback like "UTC" if
the salon timezone is missing, and apply that same timezone variable to both the
date and time formatters that format scheduledAt.
Comment on lines
132
to
135
| export async function fetchAppointmentById(id: string) { | ||
| try { | ||
| const response = await api.get(`/appointments/${id}`) | ||
| const response = await api.get(`/appointments/reserve/${id}`) | ||
| return response.data.data |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Rename this to a reservation-specific helper.
fetchAppointmentById now calls the reservation-only route from apps/api/src/routes/appointment.routes.ts:7-10, so the exported API no longer matches its behavior. That makes it easy for callers to pass a confirmed appointment ID and get the wrong endpoint.
✏️ Suggested refactor
-export async function fetchAppointmentById(id: string) {
+export async function fetchReservedAppointmentById(id: string) {
try {
const response = await api.get(`/appointments/reserve/${id}`)
return response.data.data
} catch (error) {
console.error('Error fetching appointment by id:', error)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function fetchAppointmentById(id: string) { | |
| try { | |
| const response = await api.get(`/appointments/${id}`) | |
| const response = await api.get(`/appointments/reserve/${id}`) | |
| return response.data.data | |
| export async function fetchReservedAppointmentById(id: string) { | |
| try { | |
| const response = await api.get(`/appointments/reserve/${id}`) | |
| return response.data.data |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/utils/appointment.function.ts` around lines 132 - 135, The
helper fetchAppointmentById is misnamed because it calls the reservation-only
endpoint "/appointments/reserve/:id"; rename the function to
fetchReservationById (and update its export) so its name matches behavior,
update all call sites/imports that reference fetchAppointmentById to use
fetchReservationById, and ensure any tests or docs referencing the old name are
updated; keep the same implementation (await
api.get(`/appointments/reserve/${id}`) and return response.data.data) but update
the identifier to avoid confusion with confirmed appointment APIs.
…andling, and improve Docker ignore settings - Added UUID validation for check-in and appointment IDs in the check-in controller. - Updated JWT secret handling in the authentication middleware to improve security. - Modified Docker ignore file to exclude environment files in the Lightsail deployment directory. - Refactored cookie options logic in app configuration for better clarity. - Adjusted Vite configuration to dynamically set allowed hosts for development and preview environments.
- Improved appointment fetching logic to optimize API calls. - Enhanced error handling and response structure in the API for better clarity. - Cleaned up unused state and form handling in check-in routes.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
apps/api/src/routes/_tests_/service.routes.test.ts (1)
43-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDelete
JWT_SECRETwhen it was originally unset.Line 44 still restores by assignment only. In Node, assigning
undefinedtoprocess.envleaves a non-empty value rather than removing the variable, so this can still leak config into later suites.Suggested patch
afterAll(() => { - process.env.JWT_SECRET = originalJwtSecret; + if (originalJwtSecret === undefined) { + delete process.env.JWT_SECRET; + } else { + process.env.JWT_SECRET = originalJwtSecret; + } });#!/bin/bash node - <<'NODE' delete process.env.CR_TEST_SECRET; process.env.CR_TEST_SECRET = undefined; console.log(process.env.CR_TEST_SECRET); NODEExpected result: the output is a defined string-like value instead of the variable being removed, which confirms
deleteis required when the original env var was unset.<details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@apps/api/src/routes/_tests_/service.routes.test.tsaround lines 43 - 45, The
afterAll cleanup restores process.env.JWT_SECRET via assignment which leaves the
env var set when the original was undefined; in the afterAll hook that
references originalJwtSecret and process.env.JWT_SECRET, change the restore
logic to delete process.env.JWT_SECRET when originalJwtSecret is undefined,
otherwise reassign process.env.JWT_SECRET = originalJwtSecret, so the test suite
does not leak the value into later suites.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@apps/api/src/config/app.ts:
- Around line 24-30: getCorsOrigins currently uses
parseList(process.env.CORS_ORIGINS ?? process.env.WEB_ORIGIN) so an empty
CORS_ORIGINS ("") short-circuits WEB_ORIGIN; instead, call parseList on
CORS_ORIGINS first and if that returns an empty array, call parseList on
WEB_ORIGIN, and finally default to [DEFAULT_WEB_ORIGIN] if still empty. Update
getCorsOrigins to: let origins = parseList(process.env.CORS_ORIGINS); if
(origins.length === 0) origins = parseList(process.env.WEB_ORIGIN); return
origins.length > 0 ? origins : [DEFAULT_WEB_ORIGIN]; reference parseList,
getCorsOrigins, CORS_ORIGINS, WEB_ORIGIN, and DEFAULT_WEB_ORIGIN.In
@apps/api/src/middleware/_tests_/auth.middleware.test.ts:
- Around line 13-15: The afterEach block currently restores
process.env.JWT_SECRET by assignment, which writes the string "undefined" if the
variable was originally unset; update the afterEach in the auth middleware test
to restore JWT_SECRET safely by checking originalJwtSecret: if originalJwtSecret
is undefined use delete process.env.JWT_SECRET, otherwise assign
process.env.JWT_SECRET = originalJwtSecret, and keep the existing
jest.clearAllMocks() call; reference the afterEach function, originalJwtSecret
variable, and process.env.JWT_SECRET to locate and apply this change.In
@apps/api/src/routes/_tests_/appointment.routes.test.ts:
- Line 5: The test suite is mutating process.env.JWT_SECRET at module load and
not restoring it; wrap the assignment in the appointment.routes.test.ts suite
using setup/teardown so the global env is isolated—e.g., save the original
process.env.JWT_SECRET in a variable in a beforeAll or beforeEach, set
process.env.JWT_SECRET = "unit-test-secret" for the suite, and restore the saved
value in afterAll or afterEach; update any existing top-level assignment of
process.env.JWT_SECRET so it only runs inside the describe/test hooks to avoid
leaking the secret to other suites.In
@apps/api/src/services/_tests_/check-in.service.test.ts:
- Around line 21-30: The test is asserting appointment.save was called but
selectAppointmentForCheckIn() persists a CheckIn, so update the test to assert
against the CheckIn save call instead of appointment.save: locate the test that
calls selectAppointmentForCheckIn() and replace the appointment.save expectation
with an expectation that the created CheckIn instance's save method (or the
mock/stub representing CheckIn.save) was called; ensure you reference the return
value or mock used for the CheckIn (e.g., the variable named mockCheckIn or the
CheckIn factory/mock) and verify its save was invoked while still asserting
appointment.save was not called if desired.In
@apps/api/src/services/check-in.service.ts:
- Around line 222-228: The current exact-match logic for normalizedCheckInPhone
(used to set matchesPhone) is too strict; replace it with the same
phone-matching rule used in apps/api/src/services/appointment.service.ts (lines
~82-92) so variants like leading 0 or +1 are accepted. Concretely, either import
and call the shared helper from appointment.service (e.g.,
phoneNumbersMatch/normalizePhone if present) or implement the same logic here:
strip non-digits for checkIn, appointment.phoneNumber, and
appointmentCustomer.phone, then consider a match when the full normalized
numbers are equal OR when their last 10 digits are equal; update the
matchesPhone assignment to use that logic (keeping the normalizedCheckInPhone
variable name to locate the place to change).
Duplicate comments:
In@apps/api/src/routes/_tests_/service.routes.test.ts:
- Around line 43-45: The afterAll cleanup restores process.env.JWT_SECRET via
assignment which leaves the env var set when the original was undefined; in the
afterAll hook that references originalJwtSecret and process.env.JWT_SECRET,
change the restore logic to delete process.env.JWT_SECRET when originalJwtSecret
is undefined, otherwise reassign process.env.JWT_SECRET = originalJwtSecret, so
the test suite does not leak the value into later suites.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Repository UI **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `d9b45169-7197-45c7-97ca-0ffaed6028be` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 86b40caf1ef3960c0f67245abfce6a09320c767b and e6c5bba26d2bd8e6969eeb8b6c298f9ddd39cac7. </details> <details> <summary>📒 Files selected for processing (15)</summary> * `.dockerignore` * `apps/api/src/config/_tests_/app.test.ts` * `apps/api/src/config/app.ts` * `apps/api/src/controllers/_tests_/check-in.controller.test.ts` * `apps/api/src/controllers/check-in.controller.ts` * `apps/api/src/middleware/_tests_/auth.middleware.test.ts` * `apps/api/src/middleware/auth.middleware.ts` * `apps/api/src/routes/_tests_/admin-only.routes.test.ts` * `apps/api/src/routes/_tests_/appointment.routes.test.ts` * `apps/api/src/routes/_tests_/service.routes.test.ts` * `apps/api/src/services/_tests_/check-in.service.test.ts` * `apps/api/src/services/check-in.service.ts` * `apps/web/src/components/admin/header.tsx` * `apps/web/src/routes/_layout/_check-in/check-in-2/index.tsx` * `apps/web/vite.config.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+24
to
+30
| const configuredOrigins = parseList( | ||
| process.env.CORS_ORIGINS ?? process.env.WEB_ORIGIN | ||
| ); | ||
|
|
||
| return configuredOrigins.length > 0 | ||
| ? configuredOrigins | ||
| : [DEFAULT_WEB_ORIGIN]; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How does JavaScript nullish coalescing (??) behave with empty strings, and why is a ?? bdifferent froma || bwhena = ""?
💡 Result:
When a is the empty string "": - ""?? b evaluates to "" (the left operand), because?? only falls back when the left operand is null or undefined, not when it’s an empty string [1]. - "" || b evaluates to b, because || falls back for any falsy left operand value, and the empty string is falsy [1]. So a?? b differs from a || b specifically because "" is falsy but not nullish:?? preserves "" while || treats it like “missing” and replaces it with the right-hand side [1].
Citations:
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
cat -n apps/api/src/config/app.ts | head -50Repository: Nail-Addison/nail-star
Length of output: 1775
🏁 Script executed:
# Search for the parseList function definition
rg "function parseList|const parseList|export.*parseList" -A 5Repository: Nail-Addison/nail-star
Length of output: 385
🏁 Script executed:
# Check if there are any test files that might show parseList behavior
fd -e test.ts -e spec.ts -e test.js | xargs grep -l "parseList" 2>/dev/null || echo "No test files found with parseList"Repository: Nail-Addison/nail-star
Length of output: 101
getCorsOrigins fallback logic skips WEB_ORIGIN when CORS_ORIGINS is empty.
At line 25, when CORS_ORIGINS="", the nullish coalescing operator ?? treats the empty string as a valid value (not null/undefined), so it never evaluates WEB_ORIGIN. Even though parseList("") returns an empty array (due to the filter), the code then defaults to DEFAULT_WEB_ORIGIN instead of checking WEB_ORIGIN as a fallback. This can silently produce the wrong allowlist in deployments where CORS_ORIGINS is set to empty string but WEB_ORIGIN has a valid value.
Suggested patch
export function getCorsOrigins() {
- const configuredOrigins = parseList(
- process.env.CORS_ORIGINS ?? process.env.WEB_ORIGIN
- );
-
- return configuredOrigins.length > 0
- ? configuredOrigins
- : [DEFAULT_WEB_ORIGIN];
+ const corsOrigins = parseList(process.env.CORS_ORIGINS);
+ if (corsOrigins.length > 0) return corsOrigins;
+
+ const webOrigin = parseList(process.env.WEB_ORIGIN);
+ return webOrigin.length > 0 ? webOrigin : [DEFAULT_WEB_ORIGIN];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const configuredOrigins = parseList( | |
| process.env.CORS_ORIGINS ?? process.env.WEB_ORIGIN | |
| ); | |
| return configuredOrigins.length > 0 | |
| ? configuredOrigins | |
| : [DEFAULT_WEB_ORIGIN]; | |
| export function getCorsOrigins() { | |
| const corsOrigins = parseList(process.env.CORS_ORIGINS); | |
| if (corsOrigins.length > 0) return corsOrigins; | |
| const webOrigin = parseList(process.env.WEB_ORIGIN); | |
| return webOrigin.length > 0 ? webOrigin : [DEFAULT_WEB_ORIGIN]; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/config/app.ts` around lines 24 - 30, getCorsOrigins currently
uses parseList(process.env.CORS_ORIGINS ?? process.env.WEB_ORIGIN) so an empty
CORS_ORIGINS ("") short-circuits WEB_ORIGIN; instead, call parseList on
CORS_ORIGINS first and if that returns an empty array, call parseList on
WEB_ORIGIN, and finally default to [DEFAULT_WEB_ORIGIN] if still empty. Update
getCorsOrigins to: let origins = parseList(process.env.CORS_ORIGINS); if
(origins.length === 0) origins = parseList(process.env.WEB_ORIGIN); return
origins.length > 0 ? origins : [DEFAULT_WEB_ORIGIN]; reference parseList,
getCorsOrigins, CORS_ORIGINS, WEB_ORIGIN, and DEFAULT_WEB_ORIGIN.
Comment on lines
+13
to
+15
| afterEach(() => { | ||
| process.env.JWT_SECRET = originalJwtSecret; | ||
| jest.clearAllMocks(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, what is the behavior of process.env.MY_VAR = undefined? Does it unset the variable or store the string "undefined"?
💡 Result:
In Node.js, assigning process.env.MY_VAR = undefined stores the string "undefined", not unsetting the variable. To unset it, use delete process.env.MY_VAR.[1][2][3][4] The official Node.js documentation explicitly states this behavior with an example: env.test = undefined; console.log(env.test); // => 'undefined' This implicit string conversion for process.env properties is deprecated, and future versions may throw an error for non-string/number/boolean values, but currently (as of v25+ docs), it coerces undefined to the string "undefined".[1] This is confirmed across multiple sources, including GitHub issues and Stack Overflow discussions, noting that process.env forces string coercion for environment variables.[2][3][4]
Citations:
- 1: https://nodejs.org/docs/latest-v18.x/api/process.html
- 2: Setting
undefinedtoprocess.env.*sets a"undefined"string value nodejs/node#48875 - 3: https://stackoverflow.com/questions/40203869/why-cant-i-set-node-env-to-undefined
- 4: https://stackoverflow.com/questions/10265208/node-js-process-env-assigning-process-env-property-to-undefined-results-in-stri
Restore JWT_SECRET safely when it was originally unset.
At Line 14, assigning undefined to process.env.JWT_SECRET stores the string "undefined" rather than unsetting the variable, which can leak across test suites. Use delete when the variable was originally absent.
Suggested patch
afterEach(() => {
- process.env.JWT_SECRET = originalJwtSecret;
+ if (originalJwtSecret === undefined) {
+ delete process.env.JWT_SECRET;
+ } else {
+ process.env.JWT_SECRET = originalJwtSecret;
+ }
jest.clearAllMocks();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| afterEach(() => { | |
| process.env.JWT_SECRET = originalJwtSecret; | |
| jest.clearAllMocks(); | |
| afterEach(() => { | |
| if (originalJwtSecret === undefined) { | |
| delete process.env.JWT_SECRET; | |
| } else { | |
| process.env.JWT_SECRET = originalJwtSecret; | |
| } | |
| jest.clearAllMocks(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/middleware/_tests_/auth.middleware.test.ts` around lines 13 -
15, The afterEach block currently restores process.env.JWT_SECRET by assignment,
which writes the string "undefined" if the variable was originally unset; update
the afterEach in the auth middleware test to restore JWT_SECRET safely by
checking originalJwtSecret: if originalJwtSecret is undefined use delete
process.env.JWT_SECRET, otherwise assign process.env.JWT_SECRET =
originalJwtSecret, and keep the existing jest.clearAllMocks() call; reference
the afterEach function, originalJwtSecret variable, and process.env.JWT_SECRET
to locate and apply this change.
| import jwt from "jsonwebtoken"; | ||
| import request from "supertest"; | ||
|
|
||
| process.env.JWT_SECRET = "unit-test-secret"; |
There was a problem hiding this comment.
Isolate JWT_SECRET inside this suite.
Line 5 mutates process.env.JWT_SECRET at module load and never restores it, so later suites inherit this secret and can stop exercising missing-secret behavior reliably.
Suggested patch
-process.env.JWT_SECRET = "unit-test-secret";
+const originalJwtSecret = process.env.JWT_SECRET;
@@
describe("Appointment Routes Authorization", () => {
const app = express();
beforeAll(() => {
+ process.env.JWT_SECRET = "unit-test-secret";
app.use(express.json());
app.use(restResponse);
app.use("/appointments", appointmentRoutes);
app.use(ErrorHandler);
});
+
+ afterAll(() => {
+ if (originalJwtSecret === undefined) {
+ delete process.env.JWT_SECRET;
+ } else {
+ process.env.JWT_SECRET = originalJwtSecret;
+ }
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/_tests_/appointment.routes.test.ts` at line 5, The test
suite is mutating process.env.JWT_SECRET at module load and not restoring it;
wrap the assignment in the appointment.routes.test.ts suite using setup/teardown
so the global env is isolated—e.g., save the original process.env.JWT_SECRET in
a variable in a beforeAll or beforeEach, set process.env.JWT_SECRET =
"unit-test-secret" for the suite, and restore the saved value in afterAll or
afterEach; update any existing top-level assignment of process.env.JWT_SECRET so
it only runs inside the describe/test hooks to avoid leaking the secret to other
suites.
Comment on lines
+21
to
+30
| const mockCheckIn = { | ||
| phoneNumber: "090-000-0000", | ||
| createdAt: new Date("2026-05-06T09:00:00.000Z"), | ||
| }; | ||
| const mockAppointment = { | ||
| status: "canceled", | ||
| scheduledAt: new Date("2026-05-06T10:00:00.000Z"), | ||
| phoneNumber: "0900000000", | ||
| get: jest.fn().mockReturnValue({ phone: "0900000000" }), | ||
| save: jest.fn(), |
There was a problem hiding this comment.
Assert against checkIn.save, not appointment.save.
selectAppointmentForCheckIn() persists the CheckIn, so Line 45 currently passes even if this path starts saving the check-in before the eligibility guard fails.
Suggested patch
const mockCheckIn = {
phoneNumber: "090-000-0000",
createdAt: new Date("2026-05-06T09:00:00.000Z"),
+ save: jest.fn(),
};
@@
- expect(mockAppointment.save).not.toHaveBeenCalled();
+ expect(mockCheckIn.save).not.toHaveBeenCalled();Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/services/_tests_/check-in.service.test.ts` around lines 21 - 30,
The test is asserting appointment.save was called but
selectAppointmentForCheckIn() persists a CheckIn, so update the test to assert
against the CheckIn save call instead of appointment.save: locate the test that
calls selectAppointmentForCheckIn() and replace the appointment.save expectation
with an expectation that the created CheckIn instance's save method (or the
mock/stub representing CheckIn.save) was called; ensure you reference the return
value or mock used for the CheckIn (e.g., the variable named mockCheckIn or the
CheckIn factory/mock) and verify its save was invoked while still asserting
appointment.save was not called if desired.
Comment on lines
+222
to
+228
| const normalizedCheckInPhone = checkIn.phoneNumber.replace(/\D/g, ""); | ||
| const appointmentPhone = (appointment.phoneNumber ?? "").replace(/\D/g, ""); | ||
| const customerPhone = (appointmentCustomer?.phone ?? "").replace(/\D/g, ""); | ||
| const matchesPhone = normalizedCheckInPhone.length > 0 && ( | ||
| appointmentPhone === normalizedCheckInPhone || | ||
| customerPhone === normalizedCheckInPhone | ||
| ); |
There was a problem hiding this comment.
Reuse the existing phone-matching rule here.
Lines 222-228 only accept an exact normalized match, but apps/api/src/services/appointment.service.ts:82-92 also treats matching last 10 digits as eligible. That means a check-in created from 090.../+1... variants can be rejected here even though it matched during the earlier lookup flow.
Suggested patch
- const normalizedCheckInPhone = checkIn.phoneNumber.replace(/\D/g, "");
- const appointmentPhone = (appointment.phoneNumber ?? "").replace(/\D/g, "");
- const customerPhone = (appointmentCustomer?.phone ?? "").replace(/\D/g, "");
- const matchesPhone = normalizedCheckInPhone.length > 0 && (
- appointmentPhone === normalizedCheckInPhone ||
- customerPhone === normalizedCheckInPhone
- );
+ const normalizedCheckInPhone = checkIn.phoneNumber.replace(/\D/g, "");
+ const matchesPhone =
+ normalizedCheckInPhone.length > 0 &&
+ [appointment.phoneNumber, appointmentCustomer?.phone].some((candidate) => {
+ const normalizedCandidate = (candidate ?? "").replace(/\D/g, "");
+ return (
+ normalizedCandidate === normalizedCheckInPhone ||
+ normalizedCandidate.slice(-10) === normalizedCheckInPhone.slice(-10)
+ );
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const normalizedCheckInPhone = checkIn.phoneNumber.replace(/\D/g, ""); | |
| const appointmentPhone = (appointment.phoneNumber ?? "").replace(/\D/g, ""); | |
| const customerPhone = (appointmentCustomer?.phone ?? "").replace(/\D/g, ""); | |
| const matchesPhone = normalizedCheckInPhone.length > 0 && ( | |
| appointmentPhone === normalizedCheckInPhone || | |
| customerPhone === normalizedCheckInPhone | |
| ); | |
| const normalizedCheckInPhone = checkIn.phoneNumber.replace(/\D/g, ""); | |
| const matchesPhone = | |
| normalizedCheckInPhone.length > 0 && | |
| [appointment.phoneNumber, appointmentCustomer?.phone].some((candidate) => { | |
| const normalizedCandidate = (candidate ?? "").replace(/\D/g, ""); | |
| return ( | |
| normalizedCandidate === normalizedCheckInPhone || | |
| normalizedCandidate.slice(-10) === normalizedCheckInPhone.slice(-10) | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/services/check-in.service.ts` around lines 222 - 228, The
current exact-match logic for normalizedCheckInPhone (used to set matchesPhone)
is too strict; replace it with the same phone-matching rule used in
apps/api/src/services/appointment.service.ts (lines ~82-92) so variants like
leading 0 or +1 are accepted. Concretely, either import and call the shared
helper from appointment.service (e.g., phoneNumbersMatch/normalizePhone if
present) or implement the same logic here: strip non-digits for checkIn,
appointment.phoneNumber, and appointmentCustomer.phone, then consider a match
when the full normalized numbers are equal OR when their last 10 digits are
equal; update the matchesPhone assignment to use that logic (keeping the
normalizedCheckInPhone variable name to locate the place to change).
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.
Summary by CodeRabbit
New Features
Security
Bug Fixes
Tests
Documentation