This repository was archived by the owner on May 19, 2026. It is now read-only.
feat(api): checkout for emulated payment#74
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdded a transactional checkout flow: new Changes
Sequence DiagramsequenceDiagram
participant Customer as Customer (Client)
participant API as API Handler
participant Service as Order Service
participant DB as Database (Transaction)
Customer->>API: POST /api/orders/[id]/checkout
API->>API: Authenticate & ensure role === 'customer'
API->>API: Parse orderId param
API->>Service: checkout(orderId, customerId)
Service->>DB: BEGIN TRANSACTION
Service->>DB: SELECT orders WHERE id = orderId
DB-->>Service: order record
alt Order not found
Service-->>API: throw NOT_FOUND
else
Service->>Service: validate ownership, status === 'pending', expiresAt > now
alt Validation fails
Service-->>API: throw AppError (ORDER_NOT_OWNED / ORDER_NOT_PENDING / LOCK_EXPIRED)
else
Service->>DB: SELECT orderItems WHERE orderId = ...
DB-->>Service: orderItems
Service->>DB: UPDATE seats SET status='sold', clear lockedBy/lockedAt WHERE id IN (seatIds) AND lockedBy = customerId
DB-->>Service: updatedCount
alt updatedCount !== seatIds.length
Service-->>API: throw SEAT_NOT_AVAILABLE
else
Service->>DB: UPDATE orders SET status='paid', paidAt=now() WHERE id=orderId
Service->>DB: SELECT enriched orderItems JOIN seats, seat_sections
DB-->>Service: enriched items
Service->>DB: COMMIT
Service-->>API: return paid order response (items with ticket_code)
end
end
end
API->>Customer: 200 { data } or error response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/routes/api/orders/[id]/checkout/+server.ts (1)
8-18: Remove the redundant auth/error handling in this route.
requireCustomer()already throws on unauthenticated/admin access, so Lines 12-18 are unreachable. The innertry/catchalso bypassesapiHandler’s shared error envelope and makes this endpoint serialize rawAppErrorobjects differently from the rest of the API.Suggested simplification
export const POST = apiHandler(async ({ params, locals }) => { - try { - const customer = requireCustomer(locals); - - if (!customer) { - return json({ error: Errors.UNAUTHORIZED }, { status: 401 }); - } - - if (customer.role !== 'customer') { - return json({ error: Errors.FORBIDDEN }, { status: 403 }); - } - - const orderId = Number(params.id); - if (!orderId || isNaN(orderId)) { - return json({ error: Errors.INVALID_ID }, { status: 400 }); - } - - const data = await orderService.checkout(orderId, customer.id); - - return json({ data }, { status: 200 }); - } catch (err: unknown) { - if (err instanceof AppError) { - return json({ error: err }, { status: err.statusCode }); - } - - console.error(err); - return json({ error: Errors.INTERNAL_ERROR }, { status: 500 }); - } + const customer = requireCustomer(locals); + const orderId = Number(params.id); + + if (!Number.isInteger(orderId) || orderId <= 0) { + throw Errors.INVALID_ID; + } + + const data = await orderService.checkout(orderId, customer.id); + return json({ data }, { status: 200 }); });Also applies to: 28-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/api/orders/`[id]/checkout/+server.ts around lines 8 - 18, The POST route contains redundant auth checks and an inner try/catch that bypass apiHandler's error envelope: remove the explicit checks for customer presence and role (the if blocks around requireCustomer(locals)) and delete the inner try/catch block so that requireCustomer() can throw and apiHandler will handle AppError serialization consistently; update the exported POST handler to rely solely on requireCustomer, apiHandler, and the existing downstream logic (also remove the identical redundant checks around lines handling the checkout logic referenced at 28-35).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/server/db/seed.ts`:
- Around line 953-979: The seed currently inserts orders with status set by i %
2 and then always marks pickedSeats as sold; change the loop so only orders with
status 'paid' (order from the insert in this block) mark seats status 'sold' via
db.update(seats).set({ status: 'sold' }).where(eq(seats.id, s.id)), while
pending orders set seats to locked: db.update(seats).set({ status: 'locked',
lockedBy: user.id, lockedAt: new Date() }).where(eq(seats.id, s.id)); also
adjust the expiresAt value for pending orders in the insert (the expiresAt field
passed to the orders insert) to a longer TTL or null as appropriate so seeded
pending checkouts remain locked rather than immediately expiring. Ensure you
reference the inserted order variable (order) and the pickedSeats loop when
applying these conditional updates.
In `@src/lib/server/errors.ts`:
- Around line 54-62: The LOCK_EXPIRED AppError currently uses HTTP status 400;
update the LOCK_EXPIRED entry in the errors map to use status 410 so it returns
"Gone" for expired locks as required by the checkout contract (locate the
LOCK_EXPIRED definition in src/lib/server/errors.ts and change the numeric
status from 400 to 410 while keeping the same code and message).
In `@src/lib/server/services/order.service.ts`:
- Around line 12-29: The validateOrder function currently reads orders via
tx.select().from(orders)... and later updates it, allowing a race; modify
validateOrder (or the checkout flow using it) to perform a locked read or a
guarded update: either issue the select ... forUpdate on the orders row to
acquire a row lock (using the transaction tx and the orders table) before
validations, or replace the read+separate update with a single conditional
update that sets status = 'paid' where id = orderId AND userId = customerId AND
status = 'pending' AND expiresAt > now() and then assert exactly one row was
affected (throw Errors.ORDER_NOT_OWNED / ORDER_NOT_PENDING / LOCK_EXPIRED if
affectedRows !== 1). Ensure you reference the same tx, orders, eq helpers and
preserve current error types (Errors.NOT_FOUND, Errors.ORDER_NOT_OWNED,
Errors.ORDER_NOT_PENDING, Errors.LOCK_EXPIRED).
- Around line 97-102: The seat-update is currently best-effort; modify the
transaction block in order.service.ts (the code using tx.update(seats).set({
status: 'sold' }).where(and(inArray(seats.id, seatIds), eq(seats.status,
'locked')))) to (1) set lockedBy and lockedAt to null when promoting seats to
'sold' (e.g., set({ status: 'sold', lockedBy: null, lockedAt: null })), and (2)
capture the number of rows affected and assert it equals items.length (or throw
an error to abort the transaction if it doesn’t) so the checkout fails when not
all seats move to 'sold'.
In `@src/routes/api/orders/`[id]/checkout/+server.ts:
- Around line 20-23: The current validation for orderId (const orderId =
Number(params.id)) only checks falsy/NaN and allows negative, non-integer, and
Infinity values; update the check so that after parsing params.id into orderId
you verify Number.isFinite(orderId) && Number.isInteger(orderId) && orderId > 0,
and if that fails return json({ error: Errors.INVALID_ID }, { status: 400 });
this ensures inputs like -1, 1.5, and Infinity are rejected before hitting the
DB.
---
Nitpick comments:
In `@src/routes/api/orders/`[id]/checkout/+server.ts:
- Around line 8-18: The POST route contains redundant auth checks and an inner
try/catch that bypass apiHandler's error envelope: remove the explicit checks
for customer presence and role (the if blocks around requireCustomer(locals))
and delete the inner try/catch block so that requireCustomer() can throw and
apiHandler will handle AppError serialization consistently; update the exported
POST handler to rely solely on requireCustomer, apiHandler, and the existing
downstream logic (also remove the identical redundant checks around lines
handling the checkout logic referenced at 28-35).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74761415-c740-41ee-bbdb-5baf258e6d80
📒 Files selected for processing (4)
src/lib/server/db/seed.tssrc/lib/server/errors.tssrc/lib/server/services/order.service.tssrc/routes/api/orders/[id]/checkout/+server.ts
Comment on lines
+12
to
+29
| async function validateOrder(tx: Tx, orderId: number, customerId: number) { | ||
| const [order] = await tx.select().from(orders).where(eq(orders.id, orderId)).limit(1); | ||
|
|
||
| if (!order) throwError(Errors.NOT_FOUND); | ||
|
|
||
| if (order.userId !== customerId) { | ||
| throwError(Errors.ORDER_NOT_OWNED); | ||
| } | ||
|
|
||
| if (order.status !== 'pending') { | ||
| throwError(Errors.ORDER_NOT_PENDING); | ||
| } | ||
|
|
||
| if (new Date() > order.expiresAt) { | ||
| throwError(Errors.LOCK_EXPIRED); | ||
| } | ||
|
|
||
| return order; |
Contributor
There was a problem hiding this comment.
Make the pending → paid transition atomic.
validateOrder() reads the row first, and Lines 81-87 update it later without any lock or status guard. Two concurrent checkout requests can both observe pending and both return success, which violates the checkout contract. Lock the order row (FOR UPDATE) or collapse validation into a guarded update and assert exactly one row changed.
Also applies to: 75-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/server/services/order.service.ts` around lines 12 - 29, The
validateOrder function currently reads orders via tx.select().from(orders)...
and later updates it, allowing a race; modify validateOrder (or the checkout
flow using it) to perform a locked read or a guarded update: either issue the
select ... forUpdate on the orders row to acquire a row lock (using the
transaction tx and the orders table) before validations, or replace the
read+separate update with a single conditional update that sets status = 'paid'
where id = orderId AND userId = customerId AND status = 'pending' AND expiresAt
> now() and then assert exactly one row was affected (throw
Errors.ORDER_NOT_OWNED / ORDER_NOT_PENDING / LOCK_EXPIRED if affectedRows !==
1). Ensure you reference the same tx, orders, eq helpers and preserve current
error types (Errors.NOT_FOUND, Errors.ORDER_NOT_OWNED, Errors.ORDER_NOT_PENDING,
Errors.LOCK_EXPIRED).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/server/services/order.service.ts (1)
115-124: Prefer returning persisted payment fields for response consistency.The response currently uses app-side
nowand a pre-updateordersnapshot. Consider usingUPDATE ... RETURNINGto build the response from persisted values (paidAt,totalAmount) and avoid precision/timezone drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/order.service.ts` around lines 115 - 124, The response is built from the app-side now and a pre-update Order snapshot which can drift; change the update call on tx (the tx.update(orders).set(...).where(eq(orders.id, orderId)) invocation) to use UPDATE ... RETURNING to fetch the persisted row (at least paidAt and totalAmount) and use that returned record to construct the response instead of passing now or the old order object to buildOrderResponse; i.e., have the update return the updated order and then call buildOrderResponse (or inline response builder) with the persisted fields from that returned row to ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/server/services/order.service.ts`:
- Around line 76-77: Change the expiry check from a strict greater-than to
inclusive by replacing the condition `now > order.expiresAt` with `now >=
order.expiresAt` so an order that expires exactly at `now` is treated as
expired; update the validation in the same block that throws
`Errors.LOCK_EXPIRED` (in the method in order.service.ts where this check
occurs).
---
Nitpick comments:
In `@src/lib/server/services/order.service.ts`:
- Around line 115-124: The response is built from the app-side now and a
pre-update Order snapshot which can drift; change the update call on tx (the
tx.update(orders).set(...).where(eq(orders.id, orderId)) invocation) to use
UPDATE ... RETURNING to fetch the persisted row (at least paidAt and
totalAmount) and use that returned record to construct the response instead of
passing now or the old order object to buildOrderResponse; i.e., have the update
return the updated order and then call buildOrderResponse (or inline response
builder) with the persisted fields from that returned row to ensure consistency.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bac036b4-243d-471e-83e6-48471b86d7aa
📒 Files selected for processing (4)
src/lib/server/db/seed.tssrc/lib/server/errors.tssrc/lib/server/services/order.service.tssrc/routes/api/orders/[id]/checkout/+server.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/server/db/seed.ts
- src/routes/api/orders/[id]/checkout/+server.ts
- src/lib/server/errors.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/server/services/order.service.ts (1)
57-59:⚠️ Potential issue | 🔴 CriticalMake the
pending→paidtransition atomic in theUPDATEitself.Line 57 reads state, but Line 115 updates by
idonly. Under concurrency, this is still a TOCTOU window and can overwrite a state changed by another transaction.Suggested guarded update
-import { and, eq, inArray } from 'drizzle-orm'; +import { and, eq, gt, inArray } from 'drizzle-orm'; @@ - await tx + const updatedOrder = await tx .update(orders) .set({ status: 'paid', paidAt: now, }) - .where(eq(orders.id, orderId)); + .where( + and( + eq(orders.id, orderId), + eq(orders.userId, customerId), + eq(orders.status, 'pending'), + gt(orders.expiresAt, now), + ), + ) + .returning({ id: orders.id }); + + if (updatedOrder.length !== 1) { + throwError(Errors.ORDER_NOT_PENDING); + }Also applies to: 115-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/order.service.ts` around lines 57 - 59, The current code reads the order state with tx.query.orders.findFirst (order variable) and later updates by id, creating a TOCTOU race; change the update to be guarded/atomic by performing the UPDATE with a WHERE that includes both eq(orders.id, orderId) and eq(orders.state, 'pending') (e.g., use tx.query.orders.updateMany or the equivalent conditional update API) so the transition pending→paid is done in one DB statement, then check the affected count/result and error or abort if zero rows were updated; use the same tx, orderId, and orders identifiers to locate and replace the existing id-only update logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/server/services/order.service.ts`:
- Around line 57-59: The current code reads the order state with
tx.query.orders.findFirst (order variable) and later updates by id, creating a
TOCTOU race; change the update to be guarded/atomic by performing the UPDATE
with a WHERE that includes both eq(orders.id, orderId) and eq(orders.state,
'pending') (e.g., use tx.query.orders.updateMany or the equivalent conditional
update API) so the transition pending→paid is done in one DB statement, then
check the affected count/result and error or abort if zero rows were updated;
use the same tx, orderId, and orders identifiers to locate and replace the
existing id-only update logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7fae83b3-b02d-42d3-8fde-cf25e2e3ccbb
📒 Files selected for processing (1)
src/lib/server/services/order.service.ts
DungxND
changed the title
This was referenced Apr 16, 2026
7 tasks
This was referenced May 7, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Mô tả
Closes #23
Loại thay đổi
Screenshots / Demo
Checklist
bun run dev)bun run check)bun run lint)bun run format)feat:,fix:,chore:,...)Ghi chú cho reviewer
Summary by CodeRabbit
New Features
Data
Bug Fixes / Errors