feat: passenger-to-contact mapping + booking dedup warning (#168)#169
Merged
Systemsaholic merged 12 commits intomainfrom Apr 8, 2026
Merged
feat: passenger-to-contact mapping + booking dedup warning (#168)#169Systemsaholic merged 12 commits intomainfrom
Systemsaholic merged 12 commits intomainfrom
Conversation
Adds suggestContactMatches() private method to ImportBookingService that performs name-based (case-insensitive) + DOB disambiguation contact lookups per passenger without creating or modifying any records. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntacts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ping Async searchable combobox for selecting contacts during import passenger mapping. Uses passengerName to seed results on open, debounces user input 300ms, and always shows a "Create New Contact" option. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…senger Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add contactOverrides state pre-initialized from contactMatches suggestions - Add amber dedup warning banner when preview.existingImport is set - Wire contactOverrides into PassengerList props and handleConfirm mutateAsync - Disable Import button when isAlreadyImported Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ContactCombobox: use data (not contacts) from PaginatedContactsResponseDto - PassengerList: nullish coalesce to avoid undefined in contactOverrides lookup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…idation - Only send user-edited overrides, not pre-seeded suggestions - Strengthen dedup with traveltekBookingId match - Cache selected contact label to prevent UUID display - Validate override values are UUIDs before DB query Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
FusionAPI returns passenger names in ALL CAPS which may not match the contacts search API. Title-case the name before using as the initial search query. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds per-passenger contact mapping to the cruise booking import: frontend combobox UI per passenger, preview returns contact suggestions and existing-import detection, confirm accepts per-pax Changes
Sequence DiagramssequenceDiagram
participant User
participant Frontend as Booking Preview UI
participant ContactsAPI as useContacts
participant Backend as Import Service
participant DB as Database
User->>Frontend: Open import preview
Frontend->>Backend: POST /preview
Backend->>DB: findExistingImportWithName / suggestContactMatches / checkTravelerConflicts
DB-->>Backend: existingImport + contactMatches + conflicts
Backend-->>Frontend: preview (contactMatches, existingImport, travelerConflicts)
alt existingImport present
Frontend->>User: Show amber "Already Imported" warning
Frontend->>Frontend: Disable Import button
else no existing import
Frontend->>User: Render ContactCombobox per passenger
User->>Frontend: Type / select contact
Frontend->>ContactsAPI: useContacts(search)
ContactsAPI->>DB: Query contacts
DB-->>ContactsAPI: matching contacts
ContactsAPI-->>Frontend: contact results
User->>Frontend: Select contact or "Create New"
Frontend->>Frontend: Update contactOverrides[paxno]
User->>Frontend: Click Import
Frontend->>Backend: POST /confirm with contactOverrides
Backend->>Backend: validate overrides / matchOrCreateContacts
Backend->>DB: create/match contacts & create trip
DB-->>Backend: trip created
Backend-->>Frontend: Import result
end
Estimated Code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/cruise-booking/services/import-booking.service.ts (1)
690-756: Consider extracting shared contact-matching logic.The name-matching and DOB-disambiguation logic in
suggestContactMatchesduplicates what's inmatchOrCreateContacts(around lines 616-636). If this logic needs to evolve (e.g., fuzzy matching), both places would need updates.💡 Optional: Extract shared helper
// Example helper signature private async findBestContactMatch( firstName: string, lastName: string, dob: string | null, agencyId: string, ): Promise<{ id: string; firstName: string; lastName: string; dateOfBirth: string | null } | null>This would consolidate the case-insensitive name query and DOB preference logic into one place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/cruise-booking/services/import-booking.service.ts` around lines 690 - 756, suggestContactMatches duplicates name-match + DOB-disambiguation logic from matchOrCreateContacts; extract that shared logic into a helper (e.g., private async findBestContactMatch(firstName: string, lastName: string, dob: string | null, agencyId: string)) that performs the case-insensitive contacts query against this.db.schema.contacts, limits results, and applies the DOB-preference disambiguation, returning the chosen contact or null; then replace the matching block in both suggestContactMatches and matchOrCreateContacts to call findBestContactMatch and use its returned contact id/name/isNewContact values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/cruise-booking/dto/import-booking.dto.ts`:
- Around line 53-58: The contactOverrides property is being stripped by
ValidationPipe because it lacks an object/transform decorator; update the
ImportBooking DTO by adding class-validator/class-transformer decorators to
preserve it at validation time — e.g., keep `@IsOptional`(), add `@IsObject`() to
contactOverrides and add a class-transformer decorator such as `@Type`(() =>
Object) (or an appropriate `@Transform`) so the property is recognized and not
removed by whitelist:true; refer to the contactOverrides field in
import-booking.dto.ts when making this change.
---
Nitpick comments:
In `@apps/api/src/cruise-booking/services/import-booking.service.ts`:
- Around line 690-756: suggestContactMatches duplicates name-match +
DOB-disambiguation logic from matchOrCreateContacts; extract that shared logic
into a helper (e.g., private async findBestContactMatch(firstName: string,
lastName: string, dob: string | null, agencyId: string)) that performs the
case-insensitive contacts query against this.db.schema.contacts, limits results,
and applies the DOB-preference disambiguation, returning the chosen contact or
null; then replace the matching block in both suggestContactMatches and
matchOrCreateContacts to call findBestContactMatch and use its returned contact
id/name/isNewContact values.
🪄 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: 7d0c36ef-df4d-4403-94b0-a41638c8f1da
📒 Files selected for processing (6)
apps/admin/src/app/trips/import/_components/booking-preview.tsxapps/admin/src/app/trips/import/_components/contact-combobox.tsxapps/admin/src/app/trips/import/_components/passenger-list.tsxapps/admin/src/types/import-booking.types.tsapps/api/src/cruise-booking/dto/import-booking.dto.tsapps/api/src/cruise-booking/services/import-booking.service.ts
Comment on lines
+53
to
+58
|
|
||
| @ApiPropertyOptional({ | ||
| description: 'Map paxno → contactId (UUID to link, null to create new). Omitted paxno falls back to auto-match.', | ||
| }) | ||
| @IsOptional() | ||
| contactOverrides?: Record<number, string | null> |
There was a problem hiding this comment.
contactOverrides will be silently stripped by ValidationPipe due to missing decorators.
The global ValidationPipe in apps/api/src/main.ts (lines 75-85) uses whitelist: true, which strips any properties that don't have class-validator decorators beyond @IsOptional(). Since contactOverrides lacks @IsObject() or proper transformation decorators, this field will be removed before reaching the service, breaking the feature.
🐛 Proposed fix: Add required decorators
import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'
import {
IsOptional,
IsInt,
IsString,
IsNotEmpty,
IsUUID,
ValidateIf,
+ IsObject,
} from 'class-validator'
-import { Type } from 'class-transformer'
+import { Type, Transform } from 'class-transformer'
...
`@ApiPropertyOptional`({
description: 'Map paxno → contactId (UUID to link, null to create new). Omitted paxno falls back to auto-match.',
})
`@IsOptional`()
+ `@IsObject`()
+ `@Transform`(({ value }) => {
+ // Ensure keys are numbers (JSON parse may stringify them)
+ if (typeof value !== 'object' || value === null) return value
+ const result: Record<number, string | null> = {}
+ for (const [k, v] of Object.entries(value)) {
+ result[Number(k)] = v as string | null
+ }
+ return result
+ })
contactOverrides?: Record<number, string | null>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/cruise-booking/dto/import-booking.dto.ts` around lines 53 - 58,
The contactOverrides property is being stripped by ValidationPipe because it
lacks an object/transform decorator; update the ImportBooking DTO by adding
class-validator/class-transformer decorators to preserve it at validation time —
e.g., keep `@IsOptional`(), add `@IsObject`() to contactOverrides and add a
class-transformer decorator such as `@Type`(() => Object) (or an appropriate
`@Transform`) so the property is recognized and not removed by whitelist:true;
refer to the contactOverrides field in import-booking.dto.ts when making this
change.
The dedup was only checking source='traveltek' + fusionBookingRef, so it missed bookings imported via TES, OCR, or manual entry. Now also checks custom_cruise_details.bookingNumber across ANY source as a final fallback. Fixes both the preview warning and the confirm-time idempotency check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/cruise-booking/services/import-booking.service.ts (1)
428-460:⚠️ Potential issue | 🔴 CriticalMake confirm-side dedup authoritative, not query-only.
confirm()treats this helper as the idempotency barrier, butfusionBookingRefis only stamped on Lines 363-374 and this path is still weaker than preview because it never checksresult.bookingid. Two concurrent confirms can both pass, and a direct confirm can still miss a booking that preview already recognizes as existing. Reserve the import incruise_booking_idempotencybefore any writes, then do a post-fetch dedup keyed by the Traveltek booking id before creating contacts/trips.Based on learnings: Cruise booking import endpoints (
/cruise-booking/import/previewand/cruise-booking/import/confirm) must be idempotent and scoped by agency + source + booking reference to prevent duplicate imports. Use thecruise_booking_idempotencytable (24h TTL) for deduplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/cruise-booking/services/import-booking.service.ts` around lines 428 - 460, findExistingImport currently only queries existing trips and relies on fusionBookingRef being stamped later, which allows race conditions during confirm; change confirm() to acquire a reservation in the cruise_booking_idempotency table (keyed by agencyId + source + bookingReference, 24h TTL) before performing any writes, use that reservation as the authoritative idempotency barrier, then call findExistingImport as a post-reservation dedupe (add an explicit check against result.bookingid / trip id returned by the DB) before creating contacts/trips; ensure the reservation is released/kept according to success/failure and that the lookup in findExistingImport continues to check both customCruiseDetails.fusionBookingRef and customCruiseDetails.bookingNumber as currently implemented.
🧹 Nitpick comments (1)
apps/api/src/cruise-booking/services/import-booking.service.ts (1)
463-569: This helper adds repeated preview-time scans on lookup fields that aren’t keyed for it.
preview()now calls this on every request, and this helper can execute up to three nearly identical joins. Inpackages/database/src/schema/custom-cruise-details.schema.ts:43-103,fusionBookingRefis a plainvarcharwith no index, so the fallback path will become a full scan ascustom_cruise_detailsgrows. Add indexes for the lookup columns used here, or move preview dedup to a keyed/idempotency-backed lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/cruise-booking/services/import-booking.service.ts` around lines 463 - 569, The findExistingImportWithName helper (method findExistingImportWithName) issues up to three full-table joins against custom_cruise_details (fields fusionBookingRef, traveltekBookingId, bookingNumber) on every preview() call; add proper DB indexes and schema definitions for customCruiseDetails.fusionBookingRef, customCruiseDetails.bookingNumber and customCruiseDetails.traveltekBookingId (create a migration to add these indexes and update the schema file where those columns are defined) so the three WHERE paths use indexed lookups, or alternatively refactor preview() to use an idempotency/keyed lookup (e.g., a unique import_key) instead of running findExistingImportWithName on every request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/cruise-booking/services/import-booking.service.ts`:
- Around line 581-607: The contact-override branch (check of contactOverrides
and overrideId) must not silently fall back to auto-match on malformed, deleted,
or foreign IDs; instead, when overrideId is neither null nor a valid in-agency
UUID, return/throw a bad-request error so the caller/user must reselect. Update
the logic around contactOverrides[pax.paxno]/overrideId (the block that
currently regex-checks overrideId, queries contacts via this.db.client and logs
warnings) so that: if overrideId fails the UUID regex, or the DB query does not
return a contact belonging to auth.agencyId, the method throws a BadRequest (or
validation) error referencing pax.paxno and the invalid overrideId; only allow
null to proceed as "force create new contact" and only allow a found contact.id
to map via map.set(pax.paxno, contact.id).
---
Outside diff comments:
In `@apps/api/src/cruise-booking/services/import-booking.service.ts`:
- Around line 428-460: findExistingImport currently only queries existing trips
and relies on fusionBookingRef being stamped later, which allows race conditions
during confirm; change confirm() to acquire a reservation in the
cruise_booking_idempotency table (keyed by agencyId + source + bookingReference,
24h TTL) before performing any writes, use that reservation as the authoritative
idempotency barrier, then call findExistingImport as a post-reservation dedupe
(add an explicit check against result.bookingid / trip id returned by the DB)
before creating contacts/trips; ensure the reservation is released/kept
according to success/failure and that the lookup in findExistingImport continues
to check both customCruiseDetails.fusionBookingRef and
customCruiseDetails.bookingNumber as currently implemented.
---
Nitpick comments:
In `@apps/api/src/cruise-booking/services/import-booking.service.ts`:
- Around line 463-569: The findExistingImportWithName helper (method
findExistingImportWithName) issues up to three full-table joins against
custom_cruise_details (fields fusionBookingRef, traveltekBookingId,
bookingNumber) on every preview() call; add proper DB indexes and schema
definitions for customCruiseDetails.fusionBookingRef,
customCruiseDetails.bookingNumber and customCruiseDetails.traveltekBookingId
(create a migration to add these indexes and update the schema file where those
columns are defined) so the three WHERE paths use indexed lookups, or
alternatively refactor preview() to use an idempotency/keyed lookup (e.g., a
unique import_key) instead of running findExistingImportWithName on every
request.
🪄 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: 0c038dfd-c4ac-42d6-84ba-8547c2cf0502
📒 Files selected for processing (1)
apps/api/src/cruise-booking/services/import-booking.service.ts
Comment on lines
+581
to
+607
| if (contactOverrides && pax.paxno in contactOverrides) { | ||
| const overrideId = contactOverrides[pax.paxno] | ||
| if (overrideId) { | ||
| // Validate UUID format before querying the database | ||
| if (!/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(overrideId)) { | ||
| this.logger.warn(`Invalid contact override format for paxno ${pax.paxno}: ${overrideId}`) | ||
| // Fall through to auto-match | ||
| } else { | ||
| // Validate the contact exists and belongs to this agency | ||
| const [contact] = await this.db.client | ||
| .select({ id: this.db.schema.contacts.id }) | ||
| .from(this.db.schema.contacts) | ||
| .where( | ||
| and( | ||
| eq(this.db.schema.contacts.id, overrideId), | ||
| eq(this.db.schema.contacts.agencyId, auth.agencyId), | ||
| ), | ||
| ) | ||
| .limit(1) | ||
| if (contact) { | ||
| map.set(pax.paxno, contact.id) | ||
| continue | ||
| } | ||
| this.logger.warn(`Contact override ${overrideId} not found for paxno ${pax.paxno}, falling back to auto-match`) | ||
| } | ||
| } else { | ||
| // null = force create new contact (skip auto-match) |
There was a problem hiding this comment.
Don’t silently ignore an explicit contact override.
Once the user edits a pax mapping, falling back to auto-match on a malformed, deleted, or foreign contactId can attach that passenger to a different contact than the one they chose. Treat anything other than a valid in-agency UUID or explicit null as a bad request so the user has to reselect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/cruise-booking/services/import-booking.service.ts` around lines
581 - 607, The contact-override branch (check of contactOverrides and
overrideId) must not silently fall back to auto-match on malformed, deleted, or
foreign IDs; instead, when overrideId is neither null nor a valid in-agency
UUID, return/throw a bad-request error so the caller/user must reselect. Update
the logic around contactOverrides[pax.paxno]/overrideId (the block that
currently regex-checks overrideId, queries contacts via this.db.client and logs
warnings) so that: if overrideId fails the UUID regex, or the DB query does not
return a contact belonging to auth.agencyId, the method throws a BadRequest (or
validation) error referencing pax.paxno and the invalid overrideId; only allow
null to proceed as "force create new contact" and only allow a found contact.id
to map via map.set(pax.paxno, contact.id).
Comment on lines
+741
to
+778
| const rawDob = pax.dob || null | ||
| const dob = rawDob && /^\d{4}-\d{2}-\d{2}$/.test(rawDob) && !isNaN(Date.parse(rawDob)) | ||
| ? rawDob | ||
| : null | ||
|
|
||
| const nameMatches = await this.db.client | ||
| .select({ | ||
| id: this.db.schema.contacts.id, | ||
| firstName: this.db.schema.contacts.firstName, | ||
| lastName: this.db.schema.contacts.lastName, | ||
| dateOfBirth: this.db.schema.contacts.dateOfBirth, | ||
| }) | ||
| .from(this.db.schema.contacts) | ||
| .where( | ||
| and( | ||
| eq(this.db.schema.contacts.agencyId, auth.agencyId), | ||
| sql`LOWER(${this.db.schema.contacts.firstName}) = LOWER(${firstName})`, | ||
| sql`LOWER(${this.db.schema.contacts.lastName}) = LOWER(${lastName})`, | ||
| ), | ||
| ) | ||
| .limit(10) | ||
|
|
||
| // Disambiguate: prefer DOB match, then any name match | ||
| let matched = nameMatches[0] || null | ||
| if (dob && nameMatches.length > 1) { | ||
| const dobMatch = nameMatches.find((c) => c.dateOfBirth === dob) | ||
| if (dobMatch) matched = dobMatch | ||
| } | ||
|
|
||
| results.push({ | ||
| paxno: pax.paxno, | ||
| firstName, | ||
| lastName, | ||
| matchedContactId: matched ? matched.id : null, | ||
| matchedContactName: matched | ||
| ? `${matched.firstName} ${matched.lastName}`.trim() | ||
| : null, | ||
| isNewContact: !matched, |
There was a problem hiding this comment.
This doesn’t implement the fuzzy/disambiguation requirement yet.
The query is exact-match only, and when multiple contacts share the same name but no DOB is available it seeds the first row arbitrarily. That means preview can both miss likely existing contacts and preselect the wrong one. Reuse the same fuzzy/contact-search matcher here and return “no default” when the result is still ambiguous.
Preview now checks if matched contacts already have booked cruise activities with overlapping dates: - Same ship: "already has a cabin on this ship in [Trip]" - Different ship: "is booked on [Ship] during these dates in [Trip]" Warnings are displayed as amber banners (not hard blocks) so the user can still proceed if the conflict is intentional. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/api/src/cruise-booking/services/import-booking.service.ts (1)
674-723:⚠️ Potential issue | 🟠 MajorExplicit override failures should reject, not silently fall back.
When a user explicitly maps a passenger to a contact, falling back to auto-match on validation failure (malformed UUID at line 680, or missing/foreign contact at line 698) can silently link that passenger to a different contact than intended. This creates a data integrity risk.
Only
nullshould mean "force create new contact." Any other override value that fails validation should throwBadRequestExceptionso the user must reselect.🐛 Proposed fix
if (contactOverrides && pax.paxno in contactOverrides) { const overrideId = contactOverrides[pax.paxno] if (overrideId) { // Validate UUID format before querying the database if (!/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i.test(overrideId)) { - this.logger.warn(`Invalid contact override format for paxno ${pax.paxno}: ${overrideId}`) - // Fall through to auto-match - } else { + throw new BadRequestException( + `Invalid contact override format for passenger ${pax.paxno}: ${overrideId}`, + ) + } // Validate the contact exists and belongs to this agency const [contact] = await this.db.client .select({ id: this.db.schema.contacts.id }) .from(this.db.schema.contacts) .where( and( eq(this.db.schema.contacts.id, overrideId), eq(this.db.schema.contacts.agencyId, auth.agencyId), ), ) .limit(1) - if (contact) { - map.set(pax.paxno, contact.id) - continue + if (!contact) { + throw new BadRequestException( + `Contact override ${overrideId} not found or not accessible for passenger ${pax.paxno}`, + ) } - this.logger.warn(`Contact override ${overrideId} not found for paxno ${pax.paxno}, falling back to auto-match`) - } + map.set(pax.paxno, contact.id) + continue } else { // null = force create new contact (skip auto-match)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/cruise-booking/services/import-booking.service.ts` around lines 674 - 723, The review points out that when an explicit contact override is provided in contactOverrides[pax.paxno] but is malformed or not found, the current code silently falls back to auto-match; change this so any non-null override that fails UUID format or fails the DB existence/agency check throws a BadRequestException instead of falling through. Specifically, in the block that checks contactOverrides and sets overrideId, replace the warn+fallthrough behavior (UUID regex failure and missing contact branch around the select against this.db.schema.contacts) with throwing new BadRequestException with a clear message referencing pax.paxno and overrideId; keep the existing behavior where overrideId === null continues to create a new contact via this.contactsService.create and map.set(pax.paxno, contact.id).
🧹 Nitpick comments (2)
apps/api/src/cruise-booking/services/import-booking.service.ts (2)
475-582: Consider extracting shared join logic to reduce duplication.The join chain from
customCruiseDetailsthrough totripsis repeated three times in this method (and once more infindExistingImport). Extracting a helper that builds the base query and accepts the select clause would improve maintainability.♻️ Example refactor sketch
private buildCruiseToTripQuery<T extends Record<string, unknown>>(selectClause: T) { return this.db.client .select(selectClause) .from(customCruiseDetails) .innerJoin(this.db.schema.itineraryActivities, eq(customCruiseDetails.activityId, this.db.schema.itineraryActivities.id)) .innerJoin(this.db.schema.itineraryDays, eq(this.db.schema.itineraryActivities.itineraryDayId, this.db.schema.itineraryDays.id)) .innerJoin(this.db.schema.itineraries, eq(this.db.schema.itineraryDays.itineraryId, this.db.schema.itineraries.id)) .innerJoin(this.db.schema.trips, eq(this.db.schema.itineraries.tripId, this.db.schema.trips.id)) }Then use it in both methods with appropriate
whereclauses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/cruise-booking/services/import-booking.service.ts` around lines 475 - 582, The method findExistingImportWithName duplicates the same join chain to link customCruiseDetails -> itineraryActivities -> itineraryDays -> itineraries -> trips three times (and the reviewer noted the same duplication in findExistingImport); extract a helper (e.g., buildCruiseToTripQuery) that accepts a select clause and returns this base query, then replace each repeated .select(...).from(...).innerJoin(...) chain in findExistingImportWithName (and in findExistingImport) with calls to buildCruiseToTripQuery(selectClause) and then apply the differing .where(...) and .limit(1) clauses.
610-661: N+1 query pattern — consider batching for scalability.The loop issues one DB query per matched contact. While typical cruise bookings have few passengers, this could be batched into a single query using an
INclause oncontactIdfor better performance.♻️ Suggested batching approach
+ const contactIds = matchedContacts.map(m => m.matchedContactId!) + + const existingCruises = await this.db.client + .select({ + contactId: this.db.schema.tripTravelers.contactId, + tripId: this.db.schema.trips.id, + tripName: this.db.schema.trips.name, + shipName: customCruiseDetails.shipName, + }) + .from(this.db.schema.tripTravelers) + .innerJoin(/* ... same joins ... */) + .where( + and( + sql`${this.db.schema.tripTravelers.contactId} IN ${contactIds}`, + eq(this.db.schema.trips.agencyId, agencyId), + sql`${customCruiseDetails.departureDate} <= ${endDate}`, + sql`${customCruiseDetails.arrivalDate} >= ${startDate}`, + ), + ) + + // Then group results by contactId and map back to paxno - for (const match of matchedContacts) { - const existingCruises = await this.db.client - // ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/cruise-booking/services/import-booking.service.ts` around lines 610 - 661, The current loop runs the same DB query per matched contact (matchedContacts) causing an N+1 pattern; instead build one batched query against this.db.client.select joining tripTravelers, activityTravelers, itineraryActivities, customCruiseDetails and trips with a where clause using tripTravelers.contactId IN (list of matchedContactIds) plus the existing agency/date filters, fetch all matching cruises (limit as needed), then in JS map/group the returned rows by matchedContactId and produce conflicts for each pax using the grouped results (preserving the same conflict fields and sameShip logic); update code that references matchedContactId, tripId, tripName, shipName, departureDate/arrivalDate to use the batched result set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/api/src/cruise-booking/services/import-booking.service.ts`:
- Around line 674-723: The review points out that when an explicit contact
override is provided in contactOverrides[pax.paxno] but is malformed or not
found, the current code silently falls back to auto-match; change this so any
non-null override that fails UUID format or fails the DB existence/agency check
throws a BadRequestException instead of falling through. Specifically, in the
block that checks contactOverrides and sets overrideId, replace the
warn+fallthrough behavior (UUID regex failure and missing contact branch around
the select against this.db.schema.contacts) with throwing new
BadRequestException with a clear message referencing pax.paxno and overrideId;
keep the existing behavior where overrideId === null continues to create a new
contact via this.contactsService.create and map.set(pax.paxno, contact.id).
---
Nitpick comments:
In `@apps/api/src/cruise-booking/services/import-booking.service.ts`:
- Around line 475-582: The method findExistingImportWithName duplicates the same
join chain to link customCruiseDetails -> itineraryActivities -> itineraryDays
-> itineraries -> trips three times (and the reviewer noted the same duplication
in findExistingImport); extract a helper (e.g., buildCruiseToTripQuery) that
accepts a select clause and returns this base query, then replace each repeated
.select(...).from(...).innerJoin(...) chain in findExistingImportWithName (and
in findExistingImport) with calls to buildCruiseToTripQuery(selectClause) and
then apply the differing .where(...) and .limit(1) clauses.
- Around line 610-661: The current loop runs the same DB query per matched
contact (matchedContacts) causing an N+1 pattern; instead build one batched
query against this.db.client.select joining tripTravelers, activityTravelers,
itineraryActivities, customCruiseDetails and trips with a where clause using
tripTravelers.contactId IN (list of matchedContactIds) plus the existing
agency/date filters, fetch all matching cruises (limit as needed), then in JS
map/group the returned rows by matchedContactId and produce conflicts for each
pax using the grouped results (preserving the same conflict fields and sameShip
logic); update code that references matchedContactId, tripId, tripName,
shipName, departureDate/arrivalDate to use the batched result set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a7d1b68-7359-4c19-998e-2fefd147a0cd
📒 Files selected for processing (3)
apps/admin/src/app/trips/import/_components/booking-preview.tsxapps/admin/src/types/import-booking.types.tsapps/api/src/cruise-booking/services/import-booking.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/admin/src/app/trips/import/_components/booking-preview.tsx
- apps/admin/src/types/import-booking.types.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fusionBookingRef,traveltekBookingId, andbookingNumbercontactOverrides— only user-edited overrides are sent.Changes
Backend (
import-booking.service.ts,import-booking.dto.ts)suggestContactMatches()— pure read-only contact matching for previewfindExistingImportWithName()— 3-level dedup: traveltekBookingId → fusionBookingRef → bookingNumber (any source)findExistingImport()— confirm-time dedup also checks bookingNumber across all sourcescheckTravelerConflicts()— detects overlapping cruise bookings per contact (same/different ship)contactOverridesonImportBookingConfirmDtowith UUID validation + agency checkFrontend
ContactCombobox— async combobox with debounced search, cached labels, ALL CAPS name normalizationPassengerList— interactive version with contact combobox per passengerBookingPreview— dedup warning banner, traveler conflict warnings, contactOverrides state wired into confirmCodex Review
Validated by Codex — 5 issues identified and all fixed.
Test plan
closes #168
🤖 Generated with Claude Code