This repository was archived by the owner on May 19, 2026. It is now read-only.
feat(api): create order with hold seat logic#82
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds idempotency storage and a purchase service implementing row-level locking and GA allocation; expands the AppError catalog (including CartConflictError); introduces Zod checkout schemas and a POST checkout endpoint; adds ticket-code utilities and seed-data/seed refactor; extracts seatmap/show logic into dedicated services; removes Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint as Checkout Endpoint
participant Validator as Zod Validator
participant Purchase as Purchase Service
participant DB as Database
Client->>Endpoint: POST /api/events/:id/checkout (body, Idempotency-Key?)
Endpoint->>Validator: validate params & body
Validator-->>Endpoint: valid
Endpoint->>Purchase: purchaseTickets(userId, eventId, body, idempotencyKey)
Purchase->>DB: BEGIN TRANSACTION
DB-->>Purchase: validate user, event, shows
Purchase->>DB: lock existing pending order FOR UPDATE (if any)
Purchase->>DB: revoke/expire old pending orders (release seats)
Purchase->>DB: SELECT assigned seats FOR UPDATE (all requested)
alt assigned seats missing/unavailable
Purchase->>DB: ROLLBACK
Purchase-->>Endpoint: throw CartConflictError(details)
else
Purchase->>DB: SELECT GA seats FOR UPDATE SKIP LOCKED (per section, limited)
alt GA insufficient
Purchase->>DB: ROLLBACK
Purchase-->>Endpoint: throw CartConflictError(details)
else
Purchase->>DB: INSERT orders & order_items
Purchase->>DB: UPDATE seats -> locked (locked_by/locked_at)
DB-->>Purchase: COMMIT
Purchase->>DB: update idempotency_keys to completed (if provided)
Purchase-->>Endpoint: return PurchaseResponse
end
end
Endpoint-->>Client: 201 {data} or 409 {error, details}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/server/errors.ts (1)
12-24:⚠️ Potential issue | 🟠 Major
detailstypeRecord<string, string>is too narrow for real payloads.
AppError.details(Line 12) and theCART_CONFLICTfactory signature declare details asRecord<string, string>, but the cart-conflict payload is structurallyCartConflictDetail[]({ show_id, unavailable_assigned_seats, unavailable_ga_sections }). The service is forced to cast throughas unknown as Record<string, string>(seeevent.service.tsLine 1471), which defeats type checking and silently allows any shape to leak into responses.Widen the type once, here, so the factory and
throwErrorcan carry structured conflict payloads safely:export class AppError extends Error { public code: string; public statusCode: number; - public details?: Record<string, string>; + public details?: unknown; constructor( code: string, statusCode: number, message?: string, - details?: Record<string, string>, + details?: unknown, ) { ... } } ... - CART_CONFLICT: (details?: Record<string, string>) => + CART_CONFLICT: (details?: unknown) => new AppError('CART_CONFLICT', 409, 'Một số vé trong giỏ hàng đã bị người khác mua hoặc không đủ số lượng', details),And widen the
details?: Record<string, string>parameter onthrowError(Line 121) to match.Also applies to: 97-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/errors.ts` around lines 12 - 24, AppError currently types details as Record<string,string>, which is too narrow; change the AppError.details property and its constructor parameter to Record<string, unknown> (or unknown) and update the throwError(details?) parameter type to match so structured payloads (e.g., CartConflictDetail[]) can be passed without unsafe casts; also update the CART_CONFLICT factory signature to accept the widened details type (and adjust any other factory overloads around the constructor usage noted near the other constructors) and then remove the cast at the call site (event.service.ts) so the real structured payload flows through typed.
🧹 Nitpick comments (4)
src/lib/server/services/event.service.ts (3)
1470-1472:throwError(Errors.CART_CONFLICT(), …)double-wraps the factory and relies on a lying cast.
Errors.CART_CONFLICT()already returns a freshAppError;throwErrorthen clones it into yet anotherAppError. The existing comment inerrors.tsexplicitly says factories should be thrown directly (throw Errors.X(details)). The castconflicts as unknown as Record<string, string>is only necessary becausedetailsis typed too narrowly — see the separate comment onerrors.tsLine 12/97 recommending widening it tounknown.- if (conflicts.length > 0) { - throwError(Errors.CART_CONFLICT(), 'Một số vé không khả dụng.', conflicts as unknown as Record<string, string>); - } + if (conflicts.length > 0) { + throw Errors.CART_CONFLICT(conflicts); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/event.service.ts` around lines 1470 - 1472, The code double-wraps the AppError by calling throwError(Errors.CART_CONFLICT(), ...) and uses a lying cast for conflicts; instead, throw the factory result directly: throw Errors.CART_CONFLICT(conflicts) (or similar direct throw of the AppError returned by Errors.CART_CONFLICT) and remove the cast/usage of throwError; also update the Errors factory signature (errors.ts) to accept details: unknown so you don't need to cast conflicts to Record<string,string>, referencing the throwError helper, Errors.CART_CONFLICT factory, the conflicts variable and AppError type to locate the changes.
1080-1082: Convoluted condition.- if (!['published'].includes(event.status)) { + if (event.status !== 'published') { throwError(Errors.EVENT_NOT_AVAILABLE, 'Sự kiện chưa mở bán hoặc đã hủy.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/event.service.ts` around lines 1080 - 1082, The condition checking event status is convoluted; replace the array-includes pattern with a direct comparison on the event object (e.g., use if (event.status !== 'published') to check status) and keep the existing error throw (throwError(Errors.EVENT_NOT_AVAILABLE, 'Sự kiện chưa mở bán hoặc đã hủy.')) so the intent in the event.status check is clearer and simpler.
1093-1133: Sequential per-item DB round-trips — batch where possible.For every cart item you run one
SELECT … FOR UPDATEagainstevent_shows, then one pergeneral_admissionentry againstseat_sections. With N shows × M GA sections the checkout path is O(N + N·M) sequential awaits while holding a transaction open. For typical carts this is fine, but under load (and especially for users with multi-show carts) these round-trips amplify lock-hold time.Consider one batched
SELECT … WHERE id IN (…) FOR UPDATEfor shows and one for sections, then validate in memory. Not a correctness issue, but a worthwhile optimization given this is the hottest path in the system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/event.service.ts` around lines 1093 - 1133, The loop issues multiple sequential DB round-trips (await tx.select().from(eventShows)...for('update') and per-general-admission seatSections queries) causing long transaction lock time; instead collect show IDs from body.cart_items and all section IDs from item.general_admission, run two batched queries against eventShows and seatSections using WHERE id IN (…) with .for('update') via tx, then in memory validate existence, eventId/ showId ownership, section.type/isSeatPickable/sales windows, detect duplicate showIds using showIdsInCart, and reconstruct gaRequests and allAssignedSeatIds from the validated in-memory rows, calling throwError(Errors.*, ...) when validations fail (use the same error symbols: eventShows, seatSections, tx, gaRequests, allAssignedSeatIds, throwError).src/lib/server/errors.ts (1)
131-138:CartConflictErroris unused dead code.The class is defined but has no imports or usages anywhere in the codebase. Either wire it up as the canonical way to surface conflicts (with its natural
details: unknown[]signature) or remove it to avoid dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/errors.ts` around lines 131 - 138, The CartConflictError class is unused dead code; remove its declaration and export (the class named CartConflictError with its details: unknown[] property and constructor) to eliminate dead code, or if you prefer to keep it, wire it into the cart conflict flow by replacing ad-hoc conflict throws with throw new CartConflictError(details) and ensure any top-level error-handling middleware checks instanceof CartConflictError and reads the details field; choose one approach and apply consistently.
🤖 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/schema.ts`:
- Around line 359-364: The idempotency_keys table uses timestamp for createdAt
(created_at) which is inconsistent with other timestamptz columns and lacks a GC
plan; change createdAt to timestamptz by using timestamp(..., { withTimezone:
true }) for the created_at column in the idempotencyKeys table, add an expiresAt
(expires_at) timestamptz column (suggest defaulting to now() + interval '24
hours' or set by application) and create an index on expires_at (or created_at
if you prefer) to allow efficient TTL/cleanup queries; update any code that
inserts into idempotencyKeys to populate expiresAt when inserting if you don't
use a DB-default.
In `@src/lib/server/errors.ts`:
- Line 76: The exported error mapping has a mismatched error code string: the
key INVALID_SECTION_TYPE is creating new AppError with code
'SEAT_NOT_BELONG_SEAT_SECTION'; update the AppError invocation (the code
argument) to match the key (use 'INVALID_SECTION_TYPE') so the emitted
error.code equals the constant name; locate the new AppError(...) instance for
INVALID_SECTION_TYPE to make this change and leave the existing status and
message intact.
- Line 66: The ACTIVE_ORDER_EXISTS entry currently calls new
AppError('ACTIVE_ORDER_EXISTS', 409) with no human-readable message; add a
Vietnamese message argument to match the other catalog entries and the AppError
constructor usage (e.g., new AppError('ACTIVE_ORDER_EXISTS', 409, '...') using a
Vietnamese string like "Đã có đơn đang hoạt động" or similar) so super(message
|| code) emits the proper localized message for ACTIVE_ORDER_EXISTS.
In `@src/lib/server/services/event.service.ts`:
- Around line 1536-1552: The current idempotency handling is ineffective because
you only run a final UPDATE and never read/lock the idempotencyKeys row; move
idempotency logic to the very start of the transaction (before any work) using
tx to SELECT FROM idempotencyKeys WHERE key = idempotencyKey FOR UPDATE, then:
if existing.status === 'completed' return existing.response (matching
responseData shape), if existing.status === 'processing' throw
Errors.IDEMPOTENCY_CONFLICT (via throwError), otherwise INSERT a row with key =
idempotencyKey and status = 'processing'; leave the existing final
tx.update(idempotencyKeys).set({ status: 'completed', response: responseData
}).where(eq(idempotencyKeys.key, idempotencyKey)) to mark completion so
subsequent requests read the completed row.
- Around line 1474-1485: The deductionMap is built from lockedSeats which
includes both assigned and GA seats, causing tx.update on seatSections to
decrement capacity for assigned-seat sections and potentially drive
seat_sections.capacity negative; when constructing the deductionMap (used in the
loop that calls tx.update(seatSections).set({ capacity:
sql`${seatSections.capacity} - ${qty}` }).where(eq(seatSections.id,
sectionId))), filter lockedSeats by GA/section type (e.g., only count seats
whose section.type === 'GA' or equivalent) so only GA sections get decremented,
and add a DB-level safeguard by adding a non-negative capacity check to the
seatSections schema (check('chk_capacity_non_negative', sql`${table.capacity} >=
0`)) to prevent negative capacities even if application logic slips.
- Around line 1398-1416: The code uses a non-null assertion seat.lockedAt! and
calls getTime(), which can throw if lockedAt is null; instead, null-check
seat.lockedAt before computing lockExpiry in the branch that handles seat.status
=== 'locked' (in the block that manipulates lockedSeats, existingSeatIds and
conflict.unavailable_assigned_seats). If seat.lockedAt is missing, treat the
seat as a conflict (push seat.id onto conflict.unavailable_assigned_seats)
rather than calling getTime; only compute lockExpiry and compare to now when
seat.lockedAt is a valid Date, and preserve the existing logic for
locked-by-this-user vs others.
In `@src/lib/shared/schemas/event.schema.ts`:
- Around line 665-673: The superRefine running on the cart item is adding an
issue at path ['cart_items'] which is incorrect; change the ctx.addIssue call in
the superRefine (the block using assigned_seats and general_admission) to attach
the error to the current item (e.g., use path: [] to mark the item root or path:
['assigned_seats'] / ['general_admission'] to attach to a specific field) or
alternatively move this validation into checkoutBodySchema so the path
['cart_items'] is valid; update the ctx.addIssue usage accordingly (in the same
superRefine block) to fix the error path.
In `@src/routes/api/events/`[id]/checkout/+server.ts:
- Around line 28-40: The handler currently wraps eventService.purchaseTickets in
a try/catch which returns a flat error shape and throws a generic
INTERNAL_ERROR; remove the entire try/catch so apiHandler can handle AppError
serialization and logging, stop calling throwError(Errors.INTERNAL_ERROR), and
change the successful response to return json({ data: result }, { status: 201 })
instead of 200; locate the code around eventService.purchaseTickets, the
surrounding try/catch and throwError usage to remove them and rely on apiHandler
for error handling.
---
Outside diff comments:
In `@src/lib/server/errors.ts`:
- Around line 12-24: AppError currently types details as Record<string,string>,
which is too narrow; change the AppError.details property and its constructor
parameter to Record<string, unknown> (or unknown) and update the
throwError(details?) parameter type to match so structured payloads (e.g.,
CartConflictDetail[]) can be passed without unsafe casts; also update the
CART_CONFLICT factory signature to accept the widened details type (and adjust
any other factory overloads around the constructor usage noted near the other
constructors) and then remove the cast at the call site (event.service.ts) so
the real structured payload flows through typed.
---
Nitpick comments:
In `@src/lib/server/errors.ts`:
- Around line 131-138: The CartConflictError class is unused dead code; remove
its declaration and export (the class named CartConflictError with its details:
unknown[] property and constructor) to eliminate dead code, or if you prefer to
keep it, wire it into the cart conflict flow by replacing ad-hoc conflict throws
with throw new CartConflictError(details) and ensure any top-level
error-handling middleware checks instanceof CartConflictError and reads the
details field; choose one approach and apply consistently.
In `@src/lib/server/services/event.service.ts`:
- Around line 1470-1472: The code double-wraps the AppError by calling
throwError(Errors.CART_CONFLICT(), ...) and uses a lying cast for conflicts;
instead, throw the factory result directly: throw
Errors.CART_CONFLICT(conflicts) (or similar direct throw of the AppError
returned by Errors.CART_CONFLICT) and remove the cast/usage of throwError; also
update the Errors factory signature (errors.ts) to accept details: unknown so
you don't need to cast conflicts to Record<string,string>, referencing the
throwError helper, Errors.CART_CONFLICT factory, the conflicts variable and
AppError type to locate the changes.
- Around line 1080-1082: The condition checking event status is convoluted;
replace the array-includes pattern with a direct comparison on the event object
(e.g., use if (event.status !== 'published') to check status) and keep the
existing error throw (throwError(Errors.EVENT_NOT_AVAILABLE, 'Sự kiện chưa mở
bán hoặc đã hủy.')) so the intent in the event.status check is clearer and
simpler.
- Around line 1093-1133: The loop issues multiple sequential DB round-trips
(await tx.select().from(eventShows)...for('update') and per-general-admission
seatSections queries) causing long transaction lock time; instead collect show
IDs from body.cart_items and all section IDs from item.general_admission, run
two batched queries against eventShows and seatSections using WHERE id IN (…)
with .for('update') via tx, then in memory validate existence, eventId/ showId
ownership, section.type/isSeatPickable/sales windows, detect duplicate showIds
using showIdsInCart, and reconstruct gaRequests and allAssignedSeatIds from the
validated in-memory rows, calling throwError(Errors.*, ...) when validations
fail (use the same error symbols: eventShows, seatSections, tx, gaRequests,
allAssignedSeatIds, throwError).
🪄 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: c1d4f8c0-be62-4414-9952-c3fcbc93ec84
📒 Files selected for processing (6)
src/lib/server/db/schema.tssrc/lib/server/errors.tssrc/lib/server/services/event.service.tssrc/lib/shared/schemas/event.schema.tssrc/lib/utils/ticket-code.tssrc/routes/api/events/[id]/checkout/+server.ts
Member
|
Screenshot cho PR ở đâu? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/lib/server/services/event.service.ts (2)
1093-1130: N+1 queries inside the transaction for show/section validation.Each
cart_itemsentry issues oneSELECT … FOR UPDATEagainsteventShowsplus one per GA section. Under contention this multiplies lock round-trips and extends the transaction window (and the locks held onusers/eventsfrom steps 1–2). Consider batching withinArrayand a singleSELECT … FOR UPDATEper table, then validating fields in JS:
- Fetch all requested
eventShowsbyinArray(id, showIds)constrained toeventId; any missing row →SHOW_NOT_AVAILABLE.- Fetch all referenced GA
seatSectionsin one query; then loop in memory to checktype,salesStartAt,salesEndAt.This also makes the lock acquisition order deterministic (sort by id), reducing deadlock risk under concurrent checkouts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/event.service.ts` around lines 1093 - 1130, The code currently does per-item SELECT ... FOR UPDATE queries (see loop using showIdsInCart and the tx.select().from(eventShows) and tx.select().from(seatSections) calls), causing N+1 locks; instead collect all show_ids and all referenced GA section_ids from body.cart_items, sort them to ensure deterministic lock order, run two batched queries against eventShows (inArray(eventShows.id, showIds) with eq(eventShows.eventId, eventId)).for('update') and seatSections (inArray(seatSections.id, sectionIds)).for('update'), then in JS validate that every requested show exists (throw Errors.SHOW_NOT_AVAILABLE) and that every section exists and passes type/salesStartAt/salesEndAt checks (throw Errors.SECTION_NOT_AVAILABLE / INVALID_SECTION_TYPE / SALES_NOT_STARTED / SALES_ENDED) and build allAssignedSeatIds and gaRequests from the in-memory data instead of performing per-item DB queries.
1080-1082: Simplify single-valueincludescheck.- if (!['published'].includes(event.status)) { + if (event.status !== 'published') { throwError(Errors.EVENT_NOT_AVAILABLE, 'Sự kiện chưa mở bán hoặc đã hủy.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/event.service.ts` around lines 1080 - 1082, Replace the unnecessary array includes check with a direct comparison to simplify the condition: in the event.status check (around the throwError usage with Errors.EVENT_NOT_AVAILABLE), change the guard from if (!['published'].includes(event.status)) to a direct equality check like if (event.status !== 'published') and keep the existing throwError call and message intact.
🤖 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`:
- Line 219: The seed's local generateTicketCode (in src/lib/server/db/seed.ts)
produces an 8-char code with no prefix, which is inconsistent with the shared
generateTicketCode in $lib/utils/ticket-code.ts that returns "TIX-${nano()}"
with a 10-character suffix; fix by either importing and using the shared
generateTicketCode from $lib/utils/ticket-code.ts in seed.ts or modifying the
local generateTicketCode to use the same alphabet/length and add the "TIX-"
prefix so seeded tickets match production format (refer to the functions named
generateTicketCode in both seed.ts and $lib/utils/ticket-code.ts).
In `@src/lib/server/services/event.service.ts`:
- Around line 1475-1491: The code builds deductionMap from lockedSeats but never
applies it, so re-introduce the UPDATE that decrements seatSections.capacity
after the SELECT ... FOR UPDATE: iterate deductionMap (built from
gaRequests/lockedSeats) and for each sectionId run an UPDATE on seatSections
within the same transaction (tx) to set capacity = capacity - qty (use the map
value as qty) scoped to that section id; ensure the UPDATE occurs after the
tx.select().from(seatSections).where(...).for('update') call so rows are locked,
and keep using the same tx to ensure atomicity (consider adding/checking a
non-negative capacity DB constraint on seatSections to prevent oversell).
---
Nitpick comments:
In `@src/lib/server/services/event.service.ts`:
- Around line 1093-1130: The code currently does per-item SELECT ... FOR UPDATE
queries (see loop using showIdsInCart and the tx.select().from(eventShows) and
tx.select().from(seatSections) calls), causing N+1 locks; instead collect all
show_ids and all referenced GA section_ids from body.cart_items, sort them to
ensure deterministic lock order, run two batched queries against eventShows
(inArray(eventShows.id, showIds) with eq(eventShows.eventId,
eventId)).for('update') and seatSections (inArray(seatSections.id,
sectionIds)).for('update'), then in JS validate that every requested show exists
(throw Errors.SHOW_NOT_AVAILABLE) and that every section exists and passes
type/salesStartAt/salesEndAt checks (throw Errors.SECTION_NOT_AVAILABLE /
INVALID_SECTION_TYPE / SALES_NOT_STARTED / SALES_ENDED) and build
allAssignedSeatIds and gaRequests from the in-memory data instead of performing
per-item DB queries.
- Around line 1080-1082: Replace the unnecessary array includes check with a
direct comparison to simplify the condition: in the event.status check (around
the throwError usage with Errors.EVENT_NOT_AVAILABLE), change the guard from if
(!['published'].includes(event.status)) to a direct equality check like if
(event.status !== 'published') and keep the existing throwError call and message
intact.
🪄 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: dfd23d1d-3bec-4bb1-b19c-f52be6753208
📒 Files selected for processing (4)
src/lib/server/db/schema.tssrc/lib/server/db/seed.tssrc/lib/server/errors.tssrc/lib/server/services/event.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/server/db/schema.ts
- src/lib/server/errors.ts
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/lib/server/services/event.service.ts (2)
1504-1527: DeadSELECT … FOR UPDATEonseatSectionsbefore the capacity deduction.The block on Lines 1508-1514 selects from
seatSectionsand does nothing with the result. Those rows were already row-locked at Line 1147 during GA validation, and the subsequentUPDATEon Lines 1523-1526 takes its own write lock. This just adds a round-trip (one per request, proportional to#GAsections) without changing correctness.- //9. Update GA capacity (deduct ONLY newly-locked GA seats) - const gaSectionIds = new Set<number>( - gaRequests.map((g) => g.sectionId), - ); - if (gaSectionIds.size > 0) { - await tx - .select() - .from(seatSections) - .where(inArray(seatSections.id, [...gaSectionIds])) - .for('update'); - } - - const deductionMap = new Map<number, number>(); + // 9. Update GA capacity (deduct ONLY newly-locked GA seats) + const gaSectionIds = new Set<number>(gaRequests.map((g) => g.sectionId)); + const deductionMap = new Map<number, number>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/event.service.ts` around lines 1504 - 1527, The tx.select().from(seatSections).where(inArray(seatSections.id, [...gaSectionIds])).for('update') call is redundant because those rows were already locked during GA validation and the later tx.update(...) will acquire the necessary write lock; remove that dead SELECT ... FOR UPDATE block (the code that builds gaSectionIds and invokes tx.select().from(...).for('update')) and keep the deductionMap construction and the subsequent tx.update(...) loop against seatSections to perform the capacity deductions.
1125-1162: N+1 queries inside the hot transaction path.This loop issues one
SELECT ... FOR UPDATEoneventShowsper cart item, and another per GA entry onseatSections. The reconciliation in Step 5 (Lines 1283-1293) and GA-capacity restore (Lines 1312-1315) repeat the pattern. Each query is a round-trip held inside an open transaction that already holds row locks — contention grows with cart size and network latency. Consider batching:
- Pre-fetch all requested
show_ids withinArray(eventShows.id, showIds) AND eq(eventShows.eventId, eventId)and validate in memory.- Pre-fetch all requested GA sections with a single
inArray(seatSections.id, ...)and validate/assertshowIdmembership in memory.- Batch the GA-reduction
SELECTs into one query grouped bysectionIdwithROW_NUMBER()/LIMIT per partition, or at minimum do the capacityUPDATEs in a single statement per direction.Not a correctness bug, but worth addressing before this endpoint sees real load — the transaction stays open longer than it needs to.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/event.service.ts` around lines 1125 - 1162, The loop over body.cart_items is issuing per-item FOR UPDATE queries (eventShows and seatSections) inside the tx causing N+1 contention; fix by collecting all show_ids and GA section_ids up front, then replace per-item selects with two batched prefetches: one query against eventShows using inArray(eventShows.id, showIds) and eq(eventShows.eventId, eventId) to validate existence and duplicates in memory, and one query against seatSections using inArray(seatSections.id, sectionIds) to validate section membership and types in memory (ensuring seatSections.showId matches each item.show_id); update usages of tx.select(...).for('update') in the loop to rely on the prefetched maps (retain throwing the same Errors when invalid), and similarly refactor later reconciliation and GA-capacity update logic to perform grouped/batched UPDATEs or single-statement adjustments per sectionId instead of per-GA-row queries (affecting gaRequests, allAssignedSeatIds, and the reconciliation/restore code paths).
🤖 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/event.service.ts`:
- Around line 1071-1094: The idempotency lookup currently uses
idempotencyKeys.key alone which allows cross-user leaks; change the logic in the
block handling idempotencyKey (the tx.select/.insert around idempotencyKeys,
existing and PurchaseResponse) to scope by user identity: include userId in the
WHERE when selecting (e.g., where(eq(idempotencyKeys.key, idempotencyKey),
eq(idempotencyKeys.userId, userId))) and include userId in the inserted values
so keys are per-user, or alternatively prefix the stored key with the userId
before lookup/insert; also ensure the idempotencyKeys table has a userId
column/unique composite constraint (or implement TTL/expiry on completed keys)
so completed responses don’t live forever.
- Around line 1353-1359: The check treating event.maxTicketsPerUser === 0 as
"unlimited" is unsafe; update the logic and schema handling so that
maxTicketsPerUser uses an explicit sentinel (e.g., null for unlimited, 0 for "no
purchases") and never silently means unlimited: change creation/validation in
createBasicInfo (and the step-based flow) to reject or convert a missing/0 value
to the chosen sentinel, and update the enforcement here (where
event.maxTicketsPerUser is read and compared before calling
throwError(Errors.MAX_TICKETS_EXCEEDED, ...)) to only apply the > check when the
sentinel is not present (e.g., when maxTicketsPerUser !== null and
maxTicketsPerUser >= 0); ensure any creation path validates and persists the
sentinel correctly so admins can't accidentally disable the limit.
- Around line 1529-1559: The code currently uses floating-point Number
arithmetic (lockedSeats -> s.price -> newItemsTotal, oldTotal, finalTotal) which
causes precision errors; change to integer-cent arithmetic or a decimal library:
when building lockedSeats or before summing, convert prices to integer cents
(e.g., cents = Math.round(Number(seat.price) * 100)) and sum cents into
newItemsTotalCents; when reading existingTotal from the query
(existingTotal[0]?.total) parse it as a decimal/string, convert to cents
(Math.round(parseFloat(existingTotalVal) * 100) or use the decimal helper),
compute finalTotalCents = oldTotalCents + newItemsTotalCents, then when
inserting/updating orders (tx.update(orders).set(...) and
tx.insert(orders).values(...)) format totalAmount as a fixed two-decimal string
from cents ( (finalTotalCents/100).toFixed(2) ); update references to finalTotal
and finalOrderId accordingly.
- Around line 1164-1183: The pending-orders lookup (oldPendingOrders via
tx.select from orders filtered only by userId/status) must be scoped to the
current eventId to avoid mutating other events' reservations; update the query
that builds oldPendingOrders (and any subsequent logic that uses
activeOrder/expiredOrderIds) to join orders → orderItems → seats → eventShows
and add a filter eventShows.eventId = eventId so only pending orders for this
event are selected, or alternatively detect when a pending order belongs to a
different event and throw Errors.ACTIVE_ORDER_EXISTS instead of reconciling it;
ensure references include the same symbols (oldPendingOrders, orders,
orderItems, seats, eventShows, activeOrder, expiredOrderIds, userId, eventId,
tx.select) so the change is applied in the correct query and downstream
handling.
- Around line 1495-1502: The CART_CONFLICT path is using an unsafe cast and
wrong factory usage: stop casting conflicts to Record<string,string> and instead
pass the actual payload to the error factory; change the call site to invoke
Errors.CART_CONFLICT(conflicts) (where conflicts is CartConflictDetail[] or a
single CartConflictDetail) and remove the double-cast, and update the
CART_CONFLICT factory signature in errors.ts to accept CartConflictDetail |
CartConflictDetail[] (or the appropriate array type) so throwError receives a
properly typed error detail rather than an unsound Record<string,string>.
---
Nitpick comments:
In `@src/lib/server/services/event.service.ts`:
- Around line 1504-1527: The
tx.select().from(seatSections).where(inArray(seatSections.id,
[...gaSectionIds])).for('update') call is redundant because those rows were
already locked during GA validation and the later tx.update(...) will acquire
the necessary write lock; remove that dead SELECT ... FOR UPDATE block (the code
that builds gaSectionIds and invokes tx.select().from(...).for('update')) and
keep the deductionMap construction and the subsequent tx.update(...) loop
against seatSections to perform the capacity deductions.
- Around line 1125-1162: The loop over body.cart_items is issuing per-item FOR
UPDATE queries (eventShows and seatSections) inside the tx causing N+1
contention; fix by collecting all show_ids and GA section_ids up front, then
replace per-item selects with two batched prefetches: one query against
eventShows using inArray(eventShows.id, showIds) and eq(eventShows.eventId,
eventId) to validate existence and duplicates in memory, and one query against
seatSections using inArray(seatSections.id, sectionIds) to validate section
membership and types in memory (ensuring seatSections.showId matches each
item.show_id); update usages of tx.select(...).for('update') in the loop to rely
on the prefetched maps (retain throwing the same Errors when invalid), and
similarly refactor later reconciliation and GA-capacity update logic to perform
grouped/batched UPDATEs or single-statement adjustments per sectionId instead of
per-GA-row queries (affecting gaRequests, allAssignedSeatIds, and the
reconciliation/restore code paths).
🪄 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: 52cc372f-e214-4833-8158-df982280e0dd
📥 Commits
Reviewing files that changed from the base of the PR and between d1e7176 and bed0bd60e3c9609ce650c74f645d4ecf0eff7e89.
📒 Files selected for processing (4)
src/lib/server/db/seed.tssrc/lib/server/services/event.service.tssrc/lib/shared/schemas/event.schema.tssrc/routes/api/events/[id]/checkout/+server.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/server/db/seed.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/api/events/[id]/checkout/+server.ts
- src/lib/shared/schemas/event.schema.ts
NguyenVanThanhTung
force-pushed
the
feat/22-api-purchase-ticket
branch
from
bed0bd6 to
24df878
Compare
DungxND
changed the title
- Removed deprecated `is_seat_pickable` property from seat sections in seat.service.ts. - Added new seatmap.service.ts to handle seatmap-related operations, including bulk insertion of sections and seats for shows. - Created show.service.ts to manage show-related functionalities, including adding, updating, and deleting shows. - Updated event service references in API routes to use the new seatmap and show services. - Improved ticket code generation with cryptographically secure randomness in ticket-code.ts. - Cleaned up error handling in checkout routes for orders and events. - Adjusted schemas to remove deprecated fields and ensure consistency across the application.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/server/db/seed.ts (1)
282-332:⚠️ Potential issue | 🟡 MinorSeed orders can mix seats from different shows/events.
availableSeatsis pulled globally withlimit(15)and noorderBy/show filter, then sliced into chunks of 3 per order. Sinceseatsrows span every show seeded bycreateShowWithSections, a single seeded order may end up withorderItemswhose seats belong to differentshowIds (and even different events). This produces semantically inconsistent demo orders and also non-deterministic seeds across runs because Postgres gives no ordering guarantee withoutORDER BY.Consider grouping seats by
showIdbefore allocating them to an order, and adding an explicit ordering for reproducibility, e.g.:♻️ Proposed fix sketch
- const availableSeats = await db - .select() - .from(seats) - .where(eq(seats.status, 'available')) - .limit(15); + const availableSeats = await db + .select() + .from(seats) + .where(eq(seats.status, 'available')) + .orderBy(seats.showId, seats.id) + .limit(15); @@ for (let i = 0; i < 5; i++) { - const user = randPick(allUsers); - const isPaid = i % 2 === 0; - const pickedSeats = availableSeats.slice(seatIndex, seatIndex + SEATS_PER_ORDER); - seatIndex += SEATS_PER_ORDER; - - if (pickedSeats.length === 0) break; + const user = randPick(allUsers); + const isPaid = i % 2 === 0; + const pickedSeats = availableSeats.slice(seatIndex, seatIndex + SEATS_PER_ORDER); + seatIndex += SEATS_PER_ORDER; + + if (pickedSeats.length < SEATS_PER_ORDER) break; + // Ensure all seats in an order belong to the same show + const showId = pickedSeats[0].showId; + if (!pickedSeats.every((s) => s.showId === showId)) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/db/seed.ts` around lines 282 - 332, availableSeats is fetched without ORDER BY or show scoping, causing orders to mix seats across showId; update the seeding to fetch seats ordered and grouped by showId and then allocate SEATS_PER_ORDER from a single show for each order. Specifically: change the query that builds availableSeats to include an explicit ORDER BY (e.g., by showId, sectionId, id) and/or fetch seats per showId, then iterate the grouped lists (group by seat.showId) when picking pickedSeats (replace the global slice logic that uses seatIndex) so each created order (in the loop that inserts into orders and orderItems) consumes only seats from one show; keep the existing status update logic using seats.update but ensure you reference seats, orders, and orderItems as in the current code paths.
🧹 Nitpick comments (3)
src/routes/api/events/[id]/shows/[showId]/+server.ts (1)
8-30: InconsistentshowIdhandling across verbs; PUT swallows bad input as 404.
PUTdoesNumber(params.showId)(Line 11) and passes the result directly, whilePATCH/DELETEhand the raw string to the service and rely onshowIdSchemafor validation (Lines 20, 28). If a client sends/shows/abcto thePUTendpoint,Number('abc')→NaN, the service then queries witheq(eventShows.id, NaN)(no rows) and the client sees a genericNOT_FOUNDinstead of a validation error.Recommend aligning all three with the Zod-validated path used by
PATCH/DELETE— ideally haveseatmapService.updateShowSectionsacceptstring | numberand validate viashowIdSchemalike the show service does, so callers don't need to parse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/api/events/`[id]/shows/[showId]/+server.ts around lines 8 - 30, PUT handler currently coerces params.showId with Number(...) which produces NaN for malformed ids and leads to a 404; align behavior with PATCH/DELETE by removing the direct Number(...) conversion and instead have seatmapService.updateShowSections accept string | number and validate using the same showIdSchema used by showService.updateShow/showService.deleteShow (or perform the showIdSchema parse in the PUT handler before calling seatmapService). Update seatmapService.updateShowSections signature and implementation to run showIdSchema.parse(id) (or validate inside the handler) so invalid path ids produce a validation error rather than silently querying with NaN.src/lib/server/db/seed-tickets.ts (1)
82-154: Consider wrapping each order in a single transaction.Each order performs N + M separate queries (insert
orderItemsthen updateseats.statusper seat, repeated for three orders). If the script aborts partway through — e.g. the process gets killed, DB connection drops — the DB can end up with an order row whose items reference seats still markedavailable, or vice versa. For a dev seed this is usually tolerable, but since the next run ofseedTicketsis not idempotent (no existence check on orders) it will silently re-seed on top of the broken state.Wrapping each
paidOrder1/paidOrder2/pendingOrderblock indb.transaction(async (tx) => { ... })would make the script safe to re-run after a crash.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/db/seed-tickets.ts` around lines 82 - 154, The seeding creates orders (paidOrder1, paidOrder2, pendingOrder) and then performs multiple inserts into orderItems and updates to seats without transactions, risking partial state; wrap each order block in a single db.transaction(async (tx) => { ... }) so all inserts into orderItems and corresponding updates to seats (and the initial insert into orders) use the transactional connection (replace db.insert/update/select with tx.insert/tx.update/tx.select inside the block) and throw on any failure to ensure atomicity and safe re-runs.src/lib/server/db/seed.ts (1)
246-247: Dead code:allCustomersis never used.ESLint flags
allCustomersas unused. The same query is re-issued at line 281 intoallUsers, making this fetch redundant. Either remove this block or consolidate by usingallCustomersin place ofallUsersbelow.♻️ Proposed fix
- // Lấy danh sách customer để tạo orders sau này - const allCustomers = await db.select().from(users).where(eq(users.role, 'customer')); - @@ - const allUsers = await db.select().from(users).where(eq(users.role, 'customer')); + const allCustomers = await db.select().from(users).where(eq(users.role, 'customer'));And update the
randPick(allUsers)call on line 293 accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/db/seed.ts` around lines 246 - 247, The variable allCustomers is fetched but never used; remove the redundant initial query or reuse it instead of re-querying as allUsers: either delete the db.select().from(users).where(eq(users.role, 'customer')) that assigns allCustomers, or replace the later allUsers assignment with a reuse of allCustomers and update every reference (notably randPick(allUsers) -> randPick(allCustomers)); ensure the single source of customer data is used and no unused variable remains.
🤖 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-tickets.ts`:
- Around line 124-134: The expiresAt calculation for the pending order is using
1500 minutes instead of 15, so update the expiresAt value in the orders insert
(the block that creates pendingOrder) to use 15 * 60 * 1000 (15 minutes in ms)
rather than 1500 * 60 * 1000 so the "Countdown test" actually expires in 15
minutes; verify this change is made on the expiresAt property in the
db.insert(orders).values(...) for pendingOrder.
- Around line 156-160: The final seed banner prints wrong credentials; update
the console.log statements at the end of seed-tickets.ts to reflect the actual
seeded user email and password used when creating/looking up the user (the
script references the user email "hnd1@gmail.com" when creating/looking up the
account). Replace the hard-coded "customer@tixtac.io.vn" entry with the real
email variable or literal "hnd1@gmail.com" (and ensure the password printed
matches the password used when calling the user creation logic) so the output
from the seeding script and the created user (e.g., the code that calls or
references "hnd1@gmail.com") are consistent.
In `@src/lib/server/services/purchase.service.ts`:
- Around line 607-629: The idempotency lookup in the tx block (select from
idempotencyKeys where eq(idempotencyKeys.key, idempotencyKey) ... for('update'))
only uses key and can leak responses across users/payloads and is racy; fix by
extending the idempotencyKeys schema to include userId and payloadHash and add a
composite unique constraint on (userId, key, payloadHash), compute a stable
payloadHash from the request body before the db ops, change the lookup/where to
include userId and payloadHash (so the existing status/response check returns
only for the same user+payload), and replace the select-then-insert with an
atomic upsert (INSERT ... ON CONFLICT (...) DO UPDATE/RETURNING) via the
insert(idempotencyKeys).values(...) call so concurrent requests are handled
deterministically (return completed response or surface an IDEMPOTENCY_CONFLICT)
instead of causing PK errors or cross-user leaks.
- Around line 515-519: The update to seatSections using deductionMap
unconditionally subtracts qty from seatSections.capacity and can drive capacity
negative; change the update in the transaction to include a guard predicate
(e.g., add .where(eq(seatSections.id, sectionId), sql`${seatSections.capacity}
>= ${qty}`) or similar) and check the result of the tx.update call for affected
rows—if the update affects 0 rows, throw/fail the transaction so the caller can
roll back; reference the deductionMap loop, the
tx.update(seatSections).set({...}) call, and
seatSections.capacity/seatSections.id when implementing this guard and error
path.
- Around line 122-143: The validation/locking loop that builds gaRequests from
item.general_admission must first aggregate duplicate entries by section_id
(summing quantity) the same way handleCartReplacement uses incomingGaMap;
replace direct iteration over item.general_admission with an aggregated map
(keyed by section_id) and then perform the section lookup/for update, type/sales
checks and push into gaRequests using the aggregated quantity so newGaTotalQty
and lockSeats operate on summed GA quantities rather than raw duplicates.
In `@src/lib/server/services/seatmap.service.ts`:
- Around line 19-28: The file-level doc comment describing seat insertion is
incorrect: it states GA sections "skip seat generation entirely" while the
implementation (see the inline comment around the virtual seat generation at the
GA handling block) creates one virtual/placeholder seat per capacity unit;
update the top-level comment to explicitly state that GA (general admission)
sections generate virtual placeholder seats equal to their capacity (not zero
seats), so availability and row-count logic in checkout/hold flows remain
consistent with the implementation; reference the GA handling block/virtual-seat
generation in your edit to ensure the doc matches the code.
- Around line 103-125: The seatKey concatenation in the seat-generation loop
(where seatKey is built from prefixStr, rowLabel and colNumber) can collide for
numeric rowFormat; change the seatKey scheme to include an explicit delimiter
between rowLabel and colNumber (e.g., `${prefixStr}${rowLabel}-${colNumber}`) so
lookups against disabledSet use an unambiguous key; update any code that builds
disabledSet/disabledSeats input or validate/normalize incoming disabled_seats to
the same delimiter convention so disabledSet.has(seatKey) and the
status/disabled_count logic remain correct.
In `@src/lib/server/services/show.service.ts`:
- Around line 82-192: The updateShows function currently pairs input shows to
existingShows by index which corrupts seat/section associations when shows are
removed or reordered; change updateShows to accept an optional id on each
incoming show and perform matching by id: inside the db.transaction callback
(move the event existence/permission/status checks into the transaction to avoid
TOCTOU), build a map of existingShows by id, iterate incoming shows and if
show.id exists update that specific eventShows row (preserving its
sections/seats), if no id insert a new show, collect payload ids as keptShowIds,
then delete any existingShows whose id is not in keptShowIds (and cascade-delete
seats/seatSections for those ids); ensure functions/identifiers to update are
the existing update block that uses tx.update(eventShows) /
tx.insert(eventShows) and the deletion blocks that delete from
seats/seatSections/eventShows so they are adapted to use matched ids instead of
positional logic.
---
Outside diff comments:
In `@src/lib/server/db/seed.ts`:
- Around line 282-332: availableSeats is fetched without ORDER BY or show
scoping, causing orders to mix seats across showId; update the seeding to fetch
seats ordered and grouped by showId and then allocate SEATS_PER_ORDER from a
single show for each order. Specifically: change the query that builds
availableSeats to include an explicit ORDER BY (e.g., by showId, sectionId, id)
and/or fetch seats per showId, then iterate the grouped lists (group by
seat.showId) when picking pickedSeats (replace the global slice logic that uses
seatIndex) so each created order (in the loop that inserts into orders and
orderItems) consumes only seats from one show; keep the existing status update
logic using seats.update but ensure you reference seats, orders, and orderItems
as in the current code paths.
---
Nitpick comments:
In `@src/lib/server/db/seed-tickets.ts`:
- Around line 82-154: The seeding creates orders (paidOrder1, paidOrder2,
pendingOrder) and then performs multiple inserts into orderItems and updates to
seats without transactions, risking partial state; wrap each order block in a
single db.transaction(async (tx) => { ... }) so all inserts into orderItems and
corresponding updates to seats (and the initial insert into orders) use the
transactional connection (replace db.insert/update/select with
tx.insert/tx.update/tx.select inside the block) and throw on any failure to
ensure atomicity and safe re-runs.
In `@src/lib/server/db/seed.ts`:
- Around line 246-247: The variable allCustomers is fetched but never used;
remove the redundant initial query or reuse it instead of re-querying as
allUsers: either delete the db.select().from(users).where(eq(users.role,
'customer')) that assigns allCustomers, or replace the later allUsers assignment
with a reuse of allCustomers and update every reference (notably
randPick(allUsers) -> randPick(allCustomers)); ensure the single source of
customer data is used and no unused variable remains.
In `@src/routes/api/events/`[id]/shows/[showId]/+server.ts:
- Around line 8-30: PUT handler currently coerces params.showId with Number(...)
which produces NaN for malformed ids and leads to a 404; align behavior with
PATCH/DELETE by removing the direct Number(...) conversion and instead have
seatmapService.updateShowSections accept string | number and validate using the
same showIdSchema used by showService.updateShow/showService.deleteShow (or
perform the showIdSchema parse in the PUT handler before calling
seatmapService). Update seatmapService.updateShowSections signature and
implementation to run showIdSchema.parse(id) (or validate inside the handler) so
invalid path ids produce a validation error rather than silently querying with
NaN.
🪄 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: 54cb00b1-1948-4404-9387-7ef4ffcbdbd6
📥 Commits
Reviewing files that changed from the base of the PR and between bed0bd60e3c9609ce650c74f645d4ecf0eff7e89 and 65402ae.
📒 Files selected for processing (20)
src/lib/server/db/schema.tssrc/lib/server/db/seed-data.tssrc/lib/server/db/seed-tickets.tssrc/lib/server/db/seed.tssrc/lib/server/errors.tssrc/lib/server/services/event.service.tssrc/lib/server/services/order.service.tssrc/lib/server/services/purchase.service.tssrc/lib/server/services/seat.service.tssrc/lib/server/services/seatmap.service.tssrc/lib/server/services/show.service.tssrc/lib/shared/schemas/event.schema.tssrc/lib/types/seat-map.tssrc/lib/utils/ticket-code.tssrc/routes/(admin)/admin/events/[id]/+page.server.tssrc/routes/api/events/[id]/checkout/+server.tssrc/routes/api/events/[id]/shows/[showId]/+server.tssrc/routes/api/events/create/seatmap/+server.tssrc/routes/api/events/create/shows/+server.tssrc/routes/api/orders/[id]/checkout/+server.ts
💤 Files with no reviewable changes (3)
- src/lib/types/seat-map.ts
- src/lib/server/services/seat.service.ts
- src/routes/(admin)/admin/events/[id]/+page.server.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/server/errors.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/server/db/schema.ts
- src/lib/utils/ticket-code.ts
- src/lib/shared/schemas/event.schema.ts
Comment on lines
+19
to
+28
| /** | ||
| * Insert all sections + their seats in bulk for a given show. | ||
| * | ||
| * Optimised path: | ||
| * 1. Single INSERT for all sections → 1 round-trip | ||
| * 2. Collect ALL seat rows across every section in memory | ||
| * 3. Batch-INSERT all seats in chunks of BATCH_SIZE → minimal round-trips | ||
| * | ||
| * GA (general admission) sections skip seat generation entirely — they use capacity only. | ||
| */ |
Contributor
There was a problem hiding this comment.
Doc comment contradicts the implementation.
Line 27 says "GA (general admission) sections skip seat generation entirely — they use capacity only", but Lines 66–80 generate one virtual placeholder seat per capacity unit (and Line 66's inline comment correctly says so). Please fix the top-level comment to avoid misleading future readers about how GA availability is tracked — it affects how the checkout/hold flow reasons about row counts.
📝 Suggested doc fix
- * GA (general admission) sections skip seat generation entirely — they use capacity only.
+ * GA (general admission) sections still get one virtual placeholder seat per capacity
+ * unit (rowLabel '1', incrementing colNumber) so the hold → checkout flow can treat
+ * GA and assigned seats uniformly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/server/services/seatmap.service.ts` around lines 19 - 28, The
file-level doc comment describing seat insertion is incorrect: it states GA
sections "skip seat generation entirely" while the implementation (see the
inline comment around the virtual seat generation at the GA handling block)
creates one virtual/placeholder seat per capacity unit; update the top-level
comment to explicitly state that GA (general admission) sections generate
virtual placeholder seats equal to their capacity (not zero seats), so
availability and row-count logic in checkout/hold flows remain consistent with
the implementation; reference the GA handling block/virtual-seat generation in
your edit to ensure the doc matches the code.
| console.log('👥 10 random customers seeded'); | ||
|
|
||
| // Lấy danh sách customer để tạo orders sau này | ||
| const allCustomers = await db.select().from(users).where(eq(users.role, 'customer')); |
Contributor
Author
|
…rror handling - Simplified user and event validation by removing unnecessary transaction context. - Refactored cart item validation to process assigned seats and GA requests more efficiently. - Improved error handling for seat availability and event status checks. - Consolidated order management logic to handle existing orders and cleanup of expired orders. - Enhanced idempotency key handling to ensure proper response management. - Updated seat locking mechanism to better handle conflicts and ensure atomic operations. feat(event.schema): extend seat label regex for numeric-row formats - Updated seat label regex to support both alphabetic-row (e.g., VIP-A1) and numeric-row (e.g., VIP-1-11) formats. test(seat-label): add tests for numeric-row seat labels - Introduced tests for parsing and building numeric-row seat labels to ensure correct functionality. fix(seat-label): improve parsing and building of seat labels - Enhanced parsing logic to differentiate between alphabetic and numeric row labels. - Updated building logic to correctly format numeric-row seat labels with appropriate separators.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Member
sequenceDiagram
participant C as Client
participant API as API Handler
participant PS as purchase.service
participant DB as PostgreSQL
C->>API: POST /checkout {cart_items, idempotencyKey}
API->>PS: purchaseTickets()
Note over PS, DB: 🟦 PHASE 1: READ & VALIDATE (NO TRANSACTION)
opt Has Idempotency Key
PS->>DB: INSERT INTO idempotency_keys (ON CONFLICT DO NOTHING)
alt Key exists
PS->>DB: SELECT existing key
PS-->>C: Return cached response / Throw Processing/Conflict
end
end
PS->>DB: SELECT user (Validate active & role)
PS->>DB: SELECT event (Validate published)
PS->>DB: SELECT eventShows (Validate shows exist)
Note over PS: Parse & Group cart_items (Assigned vs GA)<br/>Check duplicate seats in request
Note over PS, DB: 🟥 PHASE 2: CRITICAL PATH (SHORT TRANSACTION)
PS->>DB: BEGIN TRANSACTION
%% Cart Replacement
PS->>DB: SELECT existing pending order FOR UPDATE (Limit 1)
opt User has old pending order
PS->>DB: UPDATE seats SET status='available' (Release old seats)
PS->>DB: DELETE FROM order_items
end
%% Limit Check
PS->>DB: SELECT COUNT(paid tickets) for user
Note over PS: Throw if (paid + requested) > maxTicketsPerUser
%% Lock Assigned Seats
opt Has Assigned Seats
PS->>DB: SELECT seats + sections WHERE status='available' FOR UPDATE
Note over PS: Throw if locked seats < requested seats
Note over PS: Calculate Assigned Total Price
end
%% Grab GA Seats
opt Has GA Seats (Loop per section)
PS->>DB: SELECT seats + sections LIMIT N FOR UPDATE SKIP LOCKED
Note over PS: Throw if grabbed seats < requested N
Note over PS: Calculate GA Total Price
end
%% State Update
PS->>DB: UPDATE seats SET status='locked', lockedBy=userId
alt Had old pending order
PS->>DB: UPDATE orders SET totalAmount, expiresAt
else No pending order
PS->>DB: INSERT INTO orders (status='pending')
end
%% Insert Items & Codes
Note over PS: Generate Ticket Codes (Hash)
PS->>DB: INSERT INTO order_items (seatId, ticketCode, priceSnapshot)
opt Has Idempotency Key
PS->>DB: UPDATE idempotency_keys SET status='completed', response
end
PS->>DB: COMMIT TRANSACTION
PS-->>API: { order_id, total_amount, expires_at, locked_items, is_appended }
API-->>C: 200 OK
%% Error Handling
Note over PS, DB: 🟨 ERROR HANDLING
opt If Any Error Occurs
PS->>DB: ROLLBACK TRANSACTION (Auto by Drizzle)
opt Has Idempotency Key
PS->>DB: DELETE idempotency_key (Allow retry)
end
PS-->>API: Throw Custom Error
API-->>C: 4xx / 500 Error Response
end
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/lib/server/db/seed.ts (1)
247-249:⚠️ Potential issue | 🟡 MinorRemove unused
allCustomersquery.
allCustomersis declared but never read (ESLint@typescript-eslint/no-unused-vars); the same set of rows is re-fetched intoallUserson Line 282. Drop this block to remove the dead DB round-trip.🔧 Proposed fix
- // Lấy danh sách customer để tạo orders sau này - const allCustomers = await db.select().from(users).where(eq(users.role, 'customer')); - // ── 4. Categories ──🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/db/seed.ts` around lines 247 - 249, Remove the unused DB query that assigns allCustomers: delete the line "const allCustomers = await db.select().from(users).where(eq(users.role, 'customer'))" (and any surrounding comment) since that variable is never read and the same rows are fetched later into allUsers; ensure no other code relies on allCustomers after removal and keep only the later allUsers query to avoid the extra DB round-trip.
🧹 Nitpick comments (5)
src/lib/server/services/show.service.ts (1)
212-218: Minor TOCTOU: event/show not locked inupdateShow.The reads on Lines 212 and 215 run outside any transaction and without
.for('update'), so a concurrent publish (status flipdraft → published) can slip between the check and the subsequenttx.update(eventShows)on Line 239. Wrapping the whole sequence indb.transaction(async (tx) => { ... })with.for('update')on the show row — matching the pattern used indeleteShow(Line 268) — closes that window.Low-severity because draft→published transitions are admin-initiated and rare, but worth aligning for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/show.service.ts` around lines 212 - 218, Wrap the read-update sequence in updateShow inside db.transaction and perform the initial select of the show with for('update') to lock the row (use db.transaction(async (tx) => { ... }) and tx.select().from(eventShows).where(eq(eventShows.id, showId)).for('update').limit(1)); perform the event lookup and permission/status checks within the same transaction and use tx.update(eventShows) for the update so the draft→published TOCTOU window is closed (mirror the locking pattern used in deleteShow).src/lib/shared/schemas/event.schema.ts (1)
657-684: Consider enforcingshow_iduniqueness incart_items.
checkoutBodySchemaaccepts duplicateshow_identries incart_items. Downstream purchase logic that groups/counts per show can be tripped by a payload that splits the same show across two entries (e.g., double-counting againstmax_tickets_per_user, or the second entry silently shadowing the first during iteration). A.superRefineat the body level to reject duplicateshow_ids would fail fast with a clear validation error instead of relying on service-layer invariants.🔧 Suggested refinement
export const checkoutBodySchema = z.object({ cart_items: z.array(cartItemSchema).min(1, 'Giỏ hàng không được trống'), -}); +}).superRefine((data, ctx) => { + const seen = new Set<number>(); + for (let i = 0; i < data.cart_items.length; i++) { + const showId = data.cart_items[i].show_id; + if (seen.has(showId)) { + ctx.addIssue({ + code: 'custom', + path: ['cart_items', i, 'show_id'], + message: `show_id ${showId} bị trùng trong giỏ hàng`, + }); + } + seen.add(showId); + } +});Similarly,
assigned_seatswithin a single cart item has no dedupe at the schema level — if the service doesn't already reject duplicates, az.array(...).refine(arr => new Set(arr).size === arr.length, ...)would surface this as a 400 rather than a 409 post-lock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shared/schemas/event.schema.ts` around lines 657 - 684, Add validation to reject duplicate show_id across cart_items and to ensure assigned_seats are unique within each cart item: in checkoutBodySchema (validate cart_items) add a .superRefine that collects show_id values and calls ctx.addIssue when a duplicate is found (referencing checkoutBodySchema and cart_items/show_id); in cartItemSchema add a .refine on assigned_seats (or include it inside the existing .superRefine) that checks new Set(assigned_seats).size === assigned_seats.length and calls ctx.addIssue with a clear message when duplicates exist (referencing cartItemSchema and assigned_seats), so both errors become 400-level validation failures instead of service-layer errors.src/lib/server/db/seed.ts (1)
112-195: Consider reusinginsertSectionsWithSeatsfrom the seatmap service.The section/seat generation here duplicates the logic in
src/lib/server/services/seatmap.service.ts(GA virtual-seat generation,rowFormat+colDirectionhandling, numeric-row separator, disabled-seat matching). Refactoring seed to callinsertSectionsWithSeats(tx, showId, sections)inside adb.transactionwould eliminate drift risk (e.g. the numeric-rowseatKeyseparator fix has to be kept in sync in both places).Non-blocking — only flagging because both code paths are already diverging in subtle ways (e.g. seed defaults GA prefix to
'GA', seatmap service does the same; both must stay aligned with how admins authordisabled_seats).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/db/seed.ts` around lines 112 - 195, The seed file duplicates seat/section generation logic from the seatmap service, risking drift; update createShowWithSections to open a db.transaction and call the existing insertSectionsWithSeats transactional helper (insertSectionsWithSeats(tx, newShow.id, show.sections)) instead of reimplementing GA and assigned-seat generation, ensuring you pass the same section objects and preserve defaults (e.g., GA prefix) so the seed uses the single source of truth in src/lib/server/services/seatmap.service.ts.src/lib/server/services/purchase.service.ts (2)
300-308: Use the batchgenerateTicketCodesto guarantee intra-insert uniqueness.
generateTicketCode()is called independently per order item (line 306). Collision probability is low for a single order but non-zero, and iforderItems.ticket_codecarries a unique index, a single-batch duplicate would fail the whole transaction. The util file already exposesgenerateTicketCodes(count)which returns a de-duplicated batch via aSet— use it here.♻️ Proposed refactor
-import { generateTicketCode } from '$lib/utils/ticket-code'; +import { generateTicketCodes } from '$lib/utils/ticket-code'; @@ - // PHASE 3: Generate Ticket Codes - await tx.insert(orderItems).values( - lockedSeatsToProcess.map((s) => ({ - orderId: newOrder.id, - seatId: s.id, - priceSnapshot: s.price.toString(), - ticketCode: generateTicketCode(), - })), - ); + // PHASE 3: Generate Ticket Codes + const ticketCodes = generateTicketCodes(lockedSeatsToProcess.length); + await tx.insert(orderItems).values( + lockedSeatsToProcess.map((s, i) => ({ + orderId: newOrder.id, + seatId: s.id, + priceSnapshot: s.price.toString(), + ticketCode: ticketCodes[i], + })), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/purchase.service.ts` around lines 300 - 308, Replace per-item calls to generateTicketCode() with a single call to generateTicketCodes(count) to ensure intra-insert uniqueness: compute count = lockedSeatsToProcess.length, call generateTicketCodes(count) to get a deduplicated array, then map that array into the tx.insert(orderItems).values payload so each inserted row uses a unique ticketCode; update the mapping that builds objects with orderId (newOrder.id), seatId (s.id), and priceSnapshot to consume ticket codes from the generated array in the same order as lockedSeatsToProcess.
198-225: N+1 lookup when validating shows against the event.The
for (const item of body.cart_items)loop issues oneselect … from eventShows where id=? and eventId=?per item (lines 204–207). For a cart of 5 shows this is 5 round-trips before the transaction even opens. A singleinArray(eventShows.id, showIds)query plus an in-memory check (missing id →SHOW_NOT_AVAILABLEwith the offending id) is equivalent and cheaper. Not a blocker, but worth doing since it also simplifies the duplicate-show loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/services/purchase.service.ts` around lines 198 - 225, Replace the per-item DB lookup inside the for (const item of body.cart_items) loop with a single batch query: collect all show_ids from body.cart_items (and detect duplicates to throw Errors.DUPLICATE_SHOW for repeated ids), run one db.select().from(eventShows).where(and(inArray(eventShows.id, collectedShowIds), eq(eventShows.eventId, eventId))) to fetch all shows, build a Set of found show ids, then iterate cart_items to verify each item.show_id exists in that Set (throw Errors.SHOW_NOT_AVAILABLE with the offending id if missing) and continue populating assignedSeatRequests and gaRequests as before; update references to showIdsInCart, the db.select on eventShows, and validation logic in purchase.service.ts accordingly.
🤖 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 142-145: The paid-idempotency short-circuit uses a non-null
assertion on order.paidAt which can throw if paidAt is null; update the
paid-path in the function containing the check of order.status === 'paid' to
defensively handle a null paidAt before calling buildOrderResponse (e.g.,
compute a safePaidAt variable from order.paidAt or a schema-appropriate fallback
like order.updatedAt or null) and pass that safe value into buildOrderResponse,
or alternatively adjust buildOrderResponse to accept a nullable paidAt and
handle null safely (ensure references to paidAt.toISOString() are guarded).
In `@src/lib/server/services/purchase.service.ts`:
- Around line 153-186: The idempotency logic currently only keys on
idempotencyKey and userId, which lets a reused Idempotency-Key with a different
request body return a stale response; to fix, extend the idempotency_keys schema
with a payload_hash column and enforce a uniqueness constraint (e.g., on (key,
payload_hash) or (user_id, key, payload_hash)), compute a stable hash of the
atomic request identity (e.g., { eventId, body }) before the DB insert, include
payload_hash in the insert into idempotencyKeys (and onConflict target), and
when reading existing entries in this block (the variables idempotencyKey,
userId, existing, and the idempotencyKeys table) match on key + userId +
payload_hash; if a key exists for the same key/user but with a different
payload_hash, return Errors.IDEMPOTENCY_CONFLICT instead of the stored response.
- Around line 236-326: Before locking new seats, inside the transaction and
before calling processAssignedSeats/processGaSeats, query for the user's
existing pending order for this event (lookup orders where userId, status in
['pending','paid'] or specifically 'pending', joined to
orderItems/seats/eventShows), and if found release its locks by updating seats
(set status='available', lockedBy=null, lockedAt=null) and mark that order
cancelled (update orders.status='cancelled') or merge its items as the design
requires; then subtract that order's seat count from totalOwned so the
MAX_TICKETS_EXCEEDED check uses the corrected total, and set
finalResponse.is_appended = true when the new locked seats are intended to
append to an existing order (otherwise false). Ensure all of this runs inside
the same transaction that calls processAssignedSeats, processGaSeats, the
seats.update and the orders insert so no race occurs; reference orders,
orderItems, seats, eventShows, processAssignedSeats, processGaSeats and
finalResponse for where to apply the changes.
- Around line 39-87: The query in processAssignedSeats is not filtering out GA
(general) seats; modify the where clause used in the tx.select(...) in
processAssignedSeats to include eq(seatSections.type, 'assigned') so only
assigned-type sections are selected; after fetching availableAssignedSeats,
detect any requested seatIds that were not returned because they're GA (e.g.,
compare seatIds vs availableAssignedSeats.map(s=>s.id)) and throw a
CART_CONFLICT with a clear message indicating the client included GA (general)
seat IDs in an assigned-seat request.
In `@src/lib/server/services/show.service.ts`:
- Around line 122-171: In the loop inside the show update/insert logic (where
you iterate over shows and reference existingShowMap, tx.update(eventShows), and
tx.insert(eventShows)), change the behavior so that if a show has an id but that
id is not found in existingShowMap you do not silently insert a new show;
instead throw a client-facing error (e.g., BadRequest / ValidationError)
indicating an unknown/stale id so callers can detect mis-routed payloads.
Concretely, modify the else-branch into two branches: (1) if show.id is present
and !existingShowMap.has(show.id) -> raise an explicit error with a clear
message including the offending id, and (2) otherwise (no id provided) perform
the insert as before; keep the existing update logic for the
existingShowMap.has(show.id) case.
---
Duplicate comments:
In `@src/lib/server/db/seed.ts`:
- Around line 247-249: Remove the unused DB query that assigns allCustomers:
delete the line "const allCustomers = await
db.select().from(users).where(eq(users.role, 'customer'))" (and any surrounding
comment) since that variable is never read and the same rows are fetched later
into allUsers; ensure no other code relies on allCustomers after removal and
keep only the later allUsers query to avoid the extra DB round-trip.
---
Nitpick comments:
In `@src/lib/server/db/seed.ts`:
- Around line 112-195: The seed file duplicates seat/section generation logic
from the seatmap service, risking drift; update createShowWithSections to open a
db.transaction and call the existing insertSectionsWithSeats transactional
helper (insertSectionsWithSeats(tx, newShow.id, show.sections)) instead of
reimplementing GA and assigned-seat generation, ensuring you pass the same
section objects and preserve defaults (e.g., GA prefix) so the seed uses the
single source of truth in src/lib/server/services/seatmap.service.ts.
In `@src/lib/server/services/purchase.service.ts`:
- Around line 300-308: Replace per-item calls to generateTicketCode() with a
single call to generateTicketCodes(count) to ensure intra-insert uniqueness:
compute count = lockedSeatsToProcess.length, call generateTicketCodes(count) to
get a deduplicated array, then map that array into the
tx.insert(orderItems).values payload so each inserted row uses a unique
ticketCode; update the mapping that builds objects with orderId (newOrder.id),
seatId (s.id), and priceSnapshot to consume ticket codes from the generated
array in the same order as lockedSeatsToProcess.
- Around line 198-225: Replace the per-item DB lookup inside the for (const item
of body.cart_items) loop with a single batch query: collect all show_ids from
body.cart_items (and detect duplicates to throw Errors.DUPLICATE_SHOW for
repeated ids), run one
db.select().from(eventShows).where(and(inArray(eventShows.id, collectedShowIds),
eq(eventShows.eventId, eventId))) to fetch all shows, build a Set of found show
ids, then iterate cart_items to verify each item.show_id exists in that Set
(throw Errors.SHOW_NOT_AVAILABLE with the offending id if missing) and continue
populating assignedSeatRequests and gaRequests as before; update references to
showIdsInCart, the db.select on eventShows, and validation logic in
purchase.service.ts accordingly.
In `@src/lib/server/services/show.service.ts`:
- Around line 212-218: Wrap the read-update sequence in updateShow inside
db.transaction and perform the initial select of the show with for('update') to
lock the row (use db.transaction(async (tx) => { ... }) and
tx.select().from(eventShows).where(eq(eventShows.id,
showId)).for('update').limit(1)); perform the event lookup and permission/status
checks within the same transaction and use tx.update(eventShows) for the update
so the draft→published TOCTOU window is closed (mirror the locking pattern used
in deleteShow).
In `@src/lib/shared/schemas/event.schema.ts`:
- Around line 657-684: Add validation to reject duplicate show_id across
cart_items and to ensure assigned_seats are unique within each cart item: in
checkoutBodySchema (validate cart_items) add a .superRefine that collects
show_id values and calls ctx.addIssue when a duplicate is found (referencing
checkoutBodySchema and cart_items/show_id); in cartItemSchema add a .refine on
assigned_seats (or include it inside the existing .superRefine) that checks new
Set(assigned_seats).size === assigned_seats.length and calls ctx.addIssue with a
clear message when duplicates exist (referencing cartItemSchema and
assigned_seats), so both errors become 400-level validation failures instead of
service-layer errors.
🪄 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: fd88354f-d36b-473f-841f-7e8b523cae81
📒 Files selected for processing (12)
src/lib/server/db/schema.tssrc/lib/server/db/seed-tickets.tssrc/lib/server/db/seed.tssrc/lib/server/services/order.service.tssrc/lib/server/services/purchase.service.tssrc/lib/server/services/seatmap.service.tssrc/lib/server/services/show.service.tssrc/lib/shared/schemas/event.schema.tssrc/lib/types/db.tssrc/lib/types/purchase.tssrc/lib/utils/seat-label.test.tssrc/lib/utils/seat-label.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/types/purchase.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/server/db/schema.ts
Comment on lines
+122
to
+171
| for (const show of shows) { | ||
| if (show.id && existingShowMap.has(show.id)) { | ||
| // Update existing show in-place — preserves its sections/seats | ||
| keptShowIds.add(show.id); | ||
|
|
||
| const [updated] = await tx | ||
| .update(eventShows) | ||
| .set({ | ||
| title: show.title || null, | ||
| showDate: show.show_date, | ||
| startTime: new Date(show.start_time), | ||
| endTime: show.end_time ? new Date(show.end_time) : null, | ||
| itinerary: show.itinerary ?? [], | ||
| }) | ||
| .where(eq(eventShows.id, show.id)) | ||
| .returning(); | ||
|
|
||
| resultShows.push({ | ||
| id: updated.id, | ||
| title: updated.title, | ||
| show_date: updated.showDate, | ||
| start_time: updated.startTime, | ||
| end_time: updated.endTime, | ||
| status: updated.status, | ||
| }); | ||
| } else { | ||
| // New show — insert (no id provided, or id not found in existing shows) | ||
| const [inserted] = await tx | ||
| .insert(eventShows) | ||
| .values({ | ||
| eventId: event_id, | ||
| title: show.title || null, | ||
| showDate: show.show_date, | ||
| startTime: new Date(show.start_time), | ||
| endTime: show.end_time ? new Date(show.end_time) : null, | ||
| itinerary: show.itinerary ?? [], | ||
| status: 'draft' as const, | ||
| }) | ||
| .returning(); | ||
|
|
||
| resultShows.push({ | ||
| id: inserted.id, | ||
| title: inserted.title, | ||
| show_date: inserted.showDate, | ||
| start_time: inserted.startTime, | ||
| end_time: inserted.endTime, | ||
| status: inserted.status, | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silent insert on unknown id is surprising and can mask client bugs.
When show.id is provided but not present in existingShowMap (e.g. client sends a stale id, an id from another event, or an id that was already deleted in a prior request), the else-branch on Line 147 silently inserts a new show. Combined with the delete-missing behavior below (Line 174), a client that submits an id that doesn't belong to this event will effectively:
- Silently "lose" the intended update target (replaced with a brand-new show), and
- Trigger deletion of the existing show whose id wasn't sent back — wiping its sections/seats.
Prefer rejecting unknown ids explicitly so callers can detect mis-routed payloads:
🔧 Suggested fix
for (const show of shows) {
- if (show.id && existingShowMap.has(show.id)) {
+ if (show.id !== undefined) {
+ if (!existingShowMap.has(show.id)) {
+ throwError(Errors.NOT_FOUND, `Show id ${show.id} không thuộc sự kiện này`);
+ }
// Update existing show in-place — preserves its sections/seats
keptShowIds.add(show.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/server/services/show.service.ts` around lines 122 - 171, In the loop
inside the show update/insert logic (where you iterate over shows and reference
existingShowMap, tx.update(eventShows), and tx.insert(eventShows)), change the
behavior so that if a show has an id but that id is not found in existingShowMap
you do not silently insert a new show; instead throw a client-facing error
(e.g., BadRequest / ValidationError) indicating an unknown/stale id so callers
can detect mis-routed payloads. Concretely, modify the else-branch into two
branches: (1) if show.id is present and !existingShowMap.has(show.id) -> raise
an explicit error with a clear message including the offending id, and (2)
otherwise (no id provided) perform the insert as before; keep the existing
update logic for the existingShowMap.has(show.id) case.
Contributor
Author
|
… for purchase requests
Member
max ticket theo sự kiện |
DungxND
approved these changes
Apr 22, 2026
This was referenced May 8, 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.
Closes #22
Loại thay đổi
Screenshots / Demo
// Auth
// Lỗi dữ liệu
// Lỗi show
// Thành công
// Lỗi max/ticket
// Lỗi khu vực không thuộc suất diễn, show không thuộc khu vực, ghế không thuộc show
// vé bị lock/sold
// dup show-id
Checklist
bun run dev)bun run check)bun run lint)bun run format)fix: xử lý đúng ownership và giới hạn vé trong checkout)Ghi chú cho reviewer
activeOrder(bao gồmsectionIdvàsectionType) để phục vụ tính toán vé GA mới và trừ capacity GA chính xác.seatSections.capacity), chỉ trừ đi phần chênh lệch giữa số lượng yêu cầu và số lượng đã có trong đơn hàng hiện tại.Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores