feat: cruise library search redesign + bookings tab fixes#9
Merged
Systemsaholic merged 2 commits intomainfrom Feb 16, 2026
Merged
feat: cruise library search redesign + bookings tab fixes#9Systemsaholic merged 2 commits intomainfrom
Systemsaholic merged 2 commits intomainfrom
Conversation
Restructure filters into 3 tiers for faster cruise discovery: - Tier 1 (Quick Find): live debounced search, Ship combobox with type-ahead, date range pickers — always visible - Tier 2 (Common): cruise line, region, duration, departure port combobox, cabin type tabs — always visible - Tier 3 (More Filters): return port, ports of call, price range — collapsed by default Also adds: - Active filter badges with per-filter clear and Clear All - Expanded filter cascading (6 params instead of 2) - Auto-clear orphaned filters on cruise line change - URL sync for disembarkPortId, portOfCallIds, price range Validated by Codex (2 passes, all findings addressed). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… events, traveler guard - Compute package payment status from actual transactions (expected_payment_items) instead of stale manual enum; populate totalPaidCents/totalUnpaidCents - Enrich unlinked activities with isBooked, confirmationNumber, paymentStatus, paidCents, currency — replace hardcoded badges in packages-table UI - Invalidate trip overview queries (activities, itinerary-days, tripTotals) on booking/payment mutations so changes reflect immediately - Emit audit events for link/unlink package operations with actorId and metadata - Require at least one traveler before recording payments (skip for refunds/adjustments) - Fix overview totals to include all top-level items (standalone + packages) - Fix overview fields: show bookedTotalCents, correct outstanding calculation - Fix cruise supplier fallback and custom-cruise-form hydration recompute Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces significant updates across the cruise search interface, package display metrics, activity enrichment, and API audit/validation capabilities. Changes include a tiered-filter UI restructure with debounced search, new query parameters for cruise library filtering, enriched activity data with supplier and payment details, audit logging for link/unlink operations, expanded cache invalidations, traveler validation for payments, and replacement of "Authorized" with "Booked" metrics in package displays. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/trips/activities.service.ts (3)
1852-1955:⚠️ Potential issue | 🟡 MinorAvoid defaulting currency to CAD when pricing is missing
When pricing isn’t present, returning a default currency can mislead downstream consumers. Prefer null or the activity’s own currency.
🔧 Proposed fix
- currency: pricing?.currency ?? 'CAD', + currency: pricing?.currency ?? baseResponse.currency ?? null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/trips/activities.service.ts` around lines 1852 - 1955, The code defaults currency to 'CAD' when pricing is missing (in the activities.map return block), which can mislead consumers; update the returned currency field to prefer the activity's own currency then null instead of 'CAD' (use pricing?.currency ?? r.activity.currency ?? null) and ensure any related consumers handle nulls; check the returned object assembly in the activities.map callback where pricing is read from pricingMap and the field named currency is set.
1760-1797:⚠️ Potential issue | 🟠 MajorAudit metadata may overstate unlinked activities
The audit entry uses the requested
activityIdseven if some weren’t actually linked to the package. That can make audit logs inaccurate. Consider deriving the list/count from rows that are actually unlinked.🔧 Proposed fix
- const activities = await this.db.client - .select({ id: this.db.schema.itineraryActivities.id, name: this.db.schema.itineraryActivities.name }) - .from(this.db.schema.itineraryActivities) - .where(inArray(this.db.schema.itineraryActivities.id, activityIds)) + const activitiesToUnlink = await this.db.client + .select({ id: this.db.schema.itineraryActivities.id, name: this.db.schema.itineraryActivities.name }) + .from(this.db.schema.itineraryActivities) + .where( + and( + inArray(this.db.schema.itineraryActivities.id, activityIds), + eq(this.db.schema.itineraryActivities.parentActivityId, packageId) + ) + ) ... - if (pkg) { + if (pkg && activitiesToUnlink.length > 0) { const tripId = pkg.tripId ?? (pkg.itineraryDayId ? await this.getTripIdFromDayId(pkg.itineraryDayId) : null) if (tripId) { - const unlinkedNames = activities.map(a => a.name).join(', ') + const unlinkedNames = activitiesToUnlink.map(a => a.name).join(', ') + const count = activitiesToUnlink.length this.eventEmitter.emit( 'audit.updated', new AuditEvent( 'activity', packageId, 'updated', tripId, actorId ?? null, - `Unlinked ${activityIds.length} activit${activityIds.length === 1 ? 'y' : 'ies'} from ${pkg.name}`, - { childrenUnlinked: activityIds, unlinkedNames, count: activityIds.length, subType: 'package' } + `Unlinked ${count} activit${count === 1 ? 'y' : 'ies'} from ${pkg.name}`, + { childrenUnlinked: activitiesToUnlink.map(a => a.id), unlinkedNames, count, subType: 'package' } ) ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/trips/activities.service.ts` around lines 1760 - 1797, The audit currently reports using the input activityIds even if some weren’t actually linked; in unlinkChildrenFromPackage, query the DB for the subset of activities that both match activityIds and have parentActivityId === packageId (e.g., select id and name where inArray(id, activityIds) AND eq(parentActivityId, packageId)) before performing the update, then use that resulting list (ids and names) and its length for the audit payload (unlinkedNames, count, childrenUnlinked) instead of the original activityIds so the audit reflects only actually-unlinked rows.
2348-2402:⚠️ Potential issue | 🟠 MajorAdd activity_type filter to package totals query
The method
getTripPackageTotalsclaims to return "aggregated financial data for all packages in a trip" but currently selects all top-level activities without filtering byactivity_type = 'package'. This includes tours, lodging, and other activity types. Parallel code inactivity-totals.service.tsshows the correct filtering pattern. Add the activity_type filter to ensure package-only scope matches the documented intent.Suggested fix
WHERE ia.parent_activity_id IS NULL + AND ia.activity_type = 'package' AND (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/trips/activities.service.ts` around lines 2348 - 2402, The query in getTripPackageTotals selects top-level itinerary_activities without restricting to packages, causing non-package activities to be included; update the trip_packages CTE WHERE clause to also require ia.activity_type = 'package' (same filter pattern used in activity-totals.service.ts) so only package activities are included, and verify downstream CTEs (package_totals, payments) continue to reference tp.id as before.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/library/cruises/_components/cruise-filters.tsx`:
- Around line 61-77: The local state searchInput initialized with
useState(filters.q ?? '') doesn't update when filters.q changes externally; add
a short sync effect that watches filters.q and calls setSearchInput(filters.q ??
'') so the input reflects external updates (e.g., URL/back-nav). Locate the
state variables searchInput and setSearchInput and add a useEffect that compares
filters.q to searchInput (or previous value) and updates via setSearchInput when
different to avoid clobbering user typing while keeping external sync.
- Around line 604-608: The duration badge currently builds the label as `Nights:
${min}-${max}` which yields awkward output like `Nights: 15-any`; update the
badge creation in the block that pushes the duration badge (the badges.push call
for key 'duration') to instead: if both nightsMin and nightsMax are present keep
the "Nights: min-max" format, but when one end is open-ended (e.g., nightsMax
=== undefined or nightsMin === undefined) look up the human-friendly label from
DURATION_OPTIONS (match by value) and use that label for the badge; keep the
same onClear behavior (onChange({ nightsMin: undefined, nightsMax: undefined
})). Ensure you reference filters.nightsMin, filters.nightsMax, DURATION_OPTIONS
and the badges.push for key 'duration' when making the change.
- Around line 278-331: Add explicit Label elements for the Tier 2 controls
(Cruise Line, Region, Duration) to match Tier 1 accessibility and visual
alignment: insert a <Label> for each control (e.g., "Cruise Line", "Region",
"Duration") and associate it with the corresponding Select by giving the
SelectTrigger a stable id (or the Select a generated id) and using that id as
the Label's htmlFor; alternatively set an aria-label on the SelectTrigger if you
prefer not to add an id. Update the JSX around the
Select/SelectTrigger/SelectValue for cruiseLineId, regionId and duration to
include these Labels and ensure their text matches existing placeholders for
screen readers. Ensure keys remain unchanged and no logic in onValueChange or
value props is modified.
In `@apps/admin/src/app/library/cruises/page.tsx`:
- Around line 200-202: The Biome rule flags the implicit return in the forEach
arrow callback; update the callback used on filters.portOfCallIds.forEach to use
a statement body instead of an expression so it does not implicitly return a
value — i.e. change the callback for filters.portOfCallIds.forEach(id =>
params.append('portOfCallIds', id)) to use braces (id => {
params.append('portOfCallIds', id); }) so params.append remains a statement and
the lint rule is satisfied.
- Around line 53-56: The URL integer query parsing for nightsMin, nightsMax,
priceMinCents, and priceMaxCents currently uses parseInt directly and can yield
NaN for malformed values; modify the parsing to return undefined on missing or
invalid input by using a small helper (e.g., parseIntOrUndefined) or inline
logic that checks for null and Number.isNaN(result) and only assigns the numeric
value when valid, otherwise sets the field to undefined so the filter state and
downstream API calls never receive NaN.
In `@apps/admin/src/components/packages/packages-table.tsx`:
- Around line 1158-1176: Header label "Commission" is mismatched with
activity-row content that renders a Booked/Not Booked badge using row.isBooked
while package rows show commissionStatus; update the table to be consistent by
either renaming the header text from "Commission" to a neutral label like
"Status" or "Booking Status" where the header string is defined, or change the
activity-row cell to render a commission-related value (e.g., use
row.commissionStatus) instead of the isBooked badge; locate the header string
and the cell rendering code referencing row.isBooked and commissionStatus to
apply the chosen fix so header semantics match row content.
- Around line 364-369: The UnlinkedActivity property paidCents is declared but
never used; decide whether it should flow into UnifiedBookingRow or be removed:
if it’s required downstream, add paidCents to the UnifiedBookingRow shape and
include it in the mapping code that converts UnlinkedActivity ->
UnifiedBookingRow (the mapping logic around the code that constructs
UnifiedBookingRow from UnlinkedActivity), and then add a column/cell in the
packages-table rendering to display paidCents (formatted via currency using
currency/cents fields); otherwise remove paidCents from the UnlinkedActivity
type and any API mapping that populates it, and add a short comment documenting
the removal if needed.
- Line 1070: Remove the unsafe "as any" casts on the Badge's variant prop and
make the types align: either change getPaymentStatusVariant to explicitly return
the Badge component's Variant union (or export/alias Badge's Variant type and
use it as the return type of getPaymentStatusVariant) or map the function's
returned union to the Badge's expected union before passing it; update the four
Badge usages that currently use getPaymentStatusVariant(... ) as any (the
instances around the Badge components) so they pass a value whose type matches
Badge's variant prop without casting.
In `@apps/admin/src/hooks/use-payment-schedules.ts`:
- Around line 384-391: The delete hook useDeletePaymentTransaction is missing
the same cache invalidations as useCreatePaymentTransaction so deleting a
transaction leaves bookings/activities stale; update useDeletePaymentTransaction
to accept/traverse the tripId (like useCreatePaymentTransaction) and call
queryClient.invalidateQueries for ['bookings','list'], ['bookings','tripTotals',
tripId], ['bookings','unlinkedActivities', tripId], ['activities'], and
['itinerary-days'] after a successful delete, ensuring you locate the
invalidation logic near the current invalidateQueries calls in
useCreatePaymentTransaction and mirror it in the delete success handler.
- Around line 385-391: The invalidation calls in use-payment-schedules.ts are
using hardcoded query key arrays (e.g., ['bookings','list'],
['bookings','tripTotals', tripId], ['activities'], ['itinerary-days']) which
risks drift and unnecessary refetches; refactor to use a centralized query-key
factory or constants (exported from your queries/keys module) and replace the
inline arrays in the queryClient.invalidateQueries calls with those named keys
(e.g., BookingQueryKeys.list(), BookingQueryKeys.tripTotals(tripId),
ActivityQueryKeys.forTrip(tripId), ItineraryDayQueryKeys.forTrip(tripId)) so
keys stay consistent across hooks and scope the broad ['activities'] and
['itinerary-days'] invalidations by tripId to avoid unneeded global refetches.
In `@apps/api/src/trips/activities.service.ts`:
- Around line 1852-1955: The code defaults currency to 'CAD' when pricing is
missing (in the activities.map return block), which can mislead consumers;
update the returned currency field to prefer the activity's own currency then
null instead of 'CAD' (use pricing?.currency ?? r.activity.currency ?? null) and
ensure any related consumers handle nulls; check the returned object assembly in
the activities.map callback where pricing is read from pricingMap and the field
named currency is set.
- Around line 1760-1797: The audit currently reports using the input activityIds
even if some weren’t actually linked; in unlinkChildrenFromPackage, query the DB
for the subset of activities that both match activityIds and have
parentActivityId === packageId (e.g., select id and name where inArray(id,
activityIds) AND eq(parentActivityId, packageId)) before performing the update,
then use that resulting list (ids and names) and its length for the audit
payload (unlinkedNames, count, childrenUnlinked) instead of the original
activityIds so the audit reflects only actually-unlinked rows.
- Around line 2348-2402: The query in getTripPackageTotals selects top-level
itinerary_activities without restricting to packages, causing non-package
activities to be included; update the trip_packages CTE WHERE clause to also
require ia.activity_type = 'package' (same filter pattern used in
activity-totals.service.ts) so only package activities are included, and verify
downstream CTEs (package_totals, payments) continue to reference tp.id as
before.
In `@apps/api/src/trips/payment-schedules.service.ts`:
- Around line 821-867: The validateTripHasTravelersForPayment function currently
returns early when the expectedPaymentItem → activity → trip chain can’t be
resolved (checks for result[0], activity, and tripId), which allows payments to
bypass traveler validation; update those early-return branches to throw a
BadRequestException (or another appropriate HTTP error) with a clear message
referencing the missing link (include expectedPaymentItemId and which step
failed) so the caller is blocked instead of silently skipping validation; ensure
you update the checks at the top of validateTripHasTravelersForPayment (the
result[0] check), the activity resolution block (the activity missing check),
and the tripId resolution block (the !tripId check) to throw, not return.
In `@apps/api/src/trips/trips.controller.ts`:
- Around line 482-498: The activities.map callback in trips.controller.ts uses a
generic "a: any" which loses type-safety and uses a generic identifier; change
the parameter to a domain-specific name (e.g., "activity" or "unlinkedActivity")
and give it a proper type instead of any (for example UnlinkedActivity or the
appropriate DTO/interface used by findUnlinkedByTrip) so the mapped fields
remain type-checked; update the mapping expression (activities.map(...)) to use
the new typed identifier and adjust any downstream property access accordingly.
In `@packages/shared-types/src/api/activities.types.ts`:
- Around line 333-349: Change the paymentStatus field on UnlinkedActivityDto
from a plain string to a typed union (e.g., PaymentStatus | null) to lock
allowed values; update the UnlinkedActivityDto definition to use the
PaymentStatus type (or import it if defined elsewhere) and ensure PaymentStatus
is exported/defined as the union of valid values ('paid' | 'deposit_paid' |
'unpaid') so consumers and type-checking align with valid statuses; then update
any imports/usages of UnlinkedActivityDto to compile against the new
PaymentStatus type.
🧹 Nitpick comments (8)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app/library/cruises/_components/cruise-filters.tsx`: - Around line 604-608: The duration badge currently builds the label as `Nights: ${min}-${max}` which yields awkward output like `Nights: 15-any`; update the badge creation in the block that pushes the duration badge (the badges.push call for key 'duration') to instead: if both nightsMin and nightsMax are present keep the "Nights: min-max" format, but when one end is open-ended (e.g., nightsMax === undefined or nightsMin === undefined) look up the human-friendly label from DURATION_OPTIONS (match by value) and use that label for the badge; keep the same onClear behavior (onChange({ nightsMin: undefined, nightsMax: undefined })). Ensure you reference filters.nightsMin, filters.nightsMax, DURATION_OPTIONS and the badges.push for key 'duration' when making the change. - Around line 278-331: Add explicit Label elements for the Tier 2 controls (Cruise Line, Region, Duration) to match Tier 1 accessibility and visual alignment: insert a <Label> for each control (e.g., "Cruise Line", "Region", "Duration") and associate it with the corresponding Select by giving the SelectTrigger a stable id (or the Select a generated id) and using that id as the Label's htmlFor; alternatively set an aria-label on the SelectTrigger if you prefer not to add an id. Update the JSX around the Select/SelectTrigger/SelectValue for cruiseLineId, regionId and duration to include these Labels and ensure their text matches existing placeholders for screen readers. Ensure keys remain unchanged and no logic in onValueChange or value props is modified. In `@apps/admin/src/app/library/cruises/page.tsx`: - Around line 53-56: The URL integer query parsing for nightsMin, nightsMax, priceMinCents, and priceMaxCents currently uses parseInt directly and can yield NaN for malformed values; modify the parsing to return undefined on missing or invalid input by using a small helper (e.g., parseIntOrUndefined) or inline logic that checks for null and Number.isNaN(result) and only assigns the numeric value when valid, otherwise sets the field to undefined so the filter state and downstream API calls never receive NaN. In `@apps/admin/src/components/packages/packages-table.tsx`: - Around line 364-369: The UnlinkedActivity property paidCents is declared but never used; decide whether it should flow into UnifiedBookingRow or be removed: if it’s required downstream, add paidCents to the UnifiedBookingRow shape and include it in the mapping code that converts UnlinkedActivity -> UnifiedBookingRow (the mapping logic around the code that constructs UnifiedBookingRow from UnlinkedActivity), and then add a column/cell in the packages-table rendering to display paidCents (formatted via currency using currency/cents fields); otherwise remove paidCents from the UnlinkedActivity type and any API mapping that populates it, and add a short comment documenting the removal if needed. - Line 1070: Remove the unsafe "as any" casts on the Badge's variant prop and make the types align: either change getPaymentStatusVariant to explicitly return the Badge component's Variant union (or export/alias Badge's Variant type and use it as the return type of getPaymentStatusVariant) or map the function's returned union to the Badge's expected union before passing it; update the four Badge usages that currently use getPaymentStatusVariant(... ) as any (the instances around the Badge components) so they pass a value whose type matches Badge's variant prop without casting. In `@apps/admin/src/hooks/use-payment-schedules.ts`: - Around line 385-391: The invalidation calls in use-payment-schedules.ts are using hardcoded query key arrays (e.g., ['bookings','list'], ['bookings','tripTotals', tripId], ['activities'], ['itinerary-days']) which risks drift and unnecessary refetches; refactor to use a centralized query-key factory or constants (exported from your queries/keys module) and replace the inline arrays in the queryClient.invalidateQueries calls with those named keys (e.g., BookingQueryKeys.list(), BookingQueryKeys.tripTotals(tripId), ActivityQueryKeys.forTrip(tripId), ItineraryDayQueryKeys.forTrip(tripId)) so keys stay consistent across hooks and scope the broad ['activities'] and ['itinerary-days'] invalidations by tripId to avoid unneeded global refetches. In `@apps/api/src/trips/trips.controller.ts`: - Around line 482-498: The activities.map callback in trips.controller.ts uses a generic "a: any" which loses type-safety and uses a generic identifier; change the parameter to a domain-specific name (e.g., "activity" or "unlinkedActivity") and give it a proper type instead of any (for example UnlinkedActivity or the appropriate DTO/interface used by findUnlinkedByTrip) so the mapped fields remain type-checked; update the mapping expression (activities.map(...)) to use the new typed identifier and adjust any downstream property access accordingly. In `@packages/shared-types/src/api/activities.types.ts`: - Around line 333-349: Change the paymentStatus field on UnlinkedActivityDto from a plain string to a typed union (e.g., PaymentStatus | null) to lock allowed values; update the UnlinkedActivityDto definition to use the PaymentStatus type (or import it if defined elsewhere) and ensure PaymentStatus is exported/defined as the union of valid values ('paid' | 'deposit_paid' | 'unpaid') so consumers and type-checking align with valid statuses; then update any imports/usages of UnlinkedActivityDto to compile against the new PaymentStatus type.apps/admin/src/hooks/use-payment-schedules.ts (1)
385-391: Consider centralizing query key definitions to prevent silent breakage.The invalidation keys (
['bookings', 'list'],['bookings', 'tripTotals', tripId],['activities'],['itinerary-days']) are hardcoded as inline arrays. If these keys are also defined in other hooks or service utilities, hardcoding them here creates drift risk—changes to the actual query key structure elsewhere won't automatically reflect in invalidations.The broad prefix invalidations (
['activities']and['itinerary-days']) correctly refetch all trips but will become a performance concern if the app scales. Consider scoping bytripIdif unnecessary refetches become bottlenecks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/hooks/use-payment-schedules.ts` around lines 385 - 391, The invalidation calls in use-payment-schedules.ts are using hardcoded query key arrays (e.g., ['bookings','list'], ['bookings','tripTotals', tripId], ['activities'], ['itinerary-days']) which risks drift and unnecessary refetches; refactor to use a centralized query-key factory or constants (exported from your queries/keys module) and replace the inline arrays in the queryClient.invalidateQueries calls with those named keys (e.g., BookingQueryKeys.list(), BookingQueryKeys.tripTotals(tripId), ActivityQueryKeys.forTrip(tripId), ItineraryDayQueryKeys.forTrip(tripId)) so keys stay consistent across hooks and scope the broad ['activities'] and ['itinerary-days'] invalidations by tripId to avoid unneeded global refetches.apps/admin/src/app/library/cruises/page.tsx (1)
53-56:parseIntcan produceNaNfrom malformed URL params.If someone manually edits the URL to
?priceMinCents=abc,parseIntreturnsNaN, which propagates into the filter state and API calls. This is a pre-existing pattern (same fornightsMin/nightsMax), but worth hardening now that more integer params are added.🛡️ Suggested guard (apply to all four integer params)
- priceMinCents: searchParams.get('priceMinCents') ? parseInt(searchParams.get('priceMinCents')!, 10) : undefined, - priceMaxCents: searchParams.get('priceMaxCents') ? parseInt(searchParams.get('priceMaxCents')!, 10) : undefined, + priceMinCents: searchParams.get('priceMinCents') ? (Number.isNaN(parseInt(searchParams.get('priceMinCents')!, 10)) ? undefined : parseInt(searchParams.get('priceMinCents')!, 10)) : undefined, + priceMaxCents: searchParams.get('priceMaxCents') ? (Number.isNaN(parseInt(searchParams.get('priceMaxCents')!, 10)) ? undefined : parseInt(searchParams.get('priceMaxCents')!, 10)) : undefined,Or extract a small helper for readability:
const parseIntOrUndefined = (v: string | null): number | undefined => { if (!v) return undefined const n = parseInt(v, 10) return Number.isNaN(n) ? undefined : n }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app/library/cruises/page.tsx` around lines 53 - 56, The URL integer query parsing for nightsMin, nightsMax, priceMinCents, and priceMaxCents currently uses parseInt directly and can yield NaN for malformed values; modify the parsing to return undefined on missing or invalid input by using a small helper (e.g., parseIntOrUndefined) or inline logic that checks for null and Number.isNaN(result) and only assigns the numeric value when valid, otherwise sets the field to undefined so the filter state and downstream API calls never receive NaN.apps/admin/src/app/library/cruises/_components/cruise-filters.tsx (2)
604-608: Duration badge label can read awkwardly for open-ended ranges.When
nightsMaxisundefined(e.g., the "15+ Nights" option), the badge renders asNights: 15-any. Consider using the matchingDURATION_OPTIONSlabel instead for a more user-friendly display:Suggested improvement
if (filters.nightsMin !== undefined || filters.nightsMax !== undefined) { - const min = filters.nightsMin ?? 'any' - const max = filters.nightsMax ?? 'any' - badges.push({ key: 'duration', label: `Nights: ${min}-${max}`, onClear: () => onChange({ nightsMin: undefined, nightsMax: undefined }) }) + const match = DURATION_OPTIONS.find(o => o.min === filters.nightsMin && o.max === filters.nightsMax) + const durationLabel = match?.label ?? `${filters.nightsMin ?? '?'}–${filters.nightsMax ?? '?'} Nights` + badges.push({ key: 'duration', label: durationLabel, onClear: () => onChange({ nightsMin: undefined, nightsMax: undefined }) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app/library/cruises/_components/cruise-filters.tsx` around lines 604 - 608, The duration badge currently builds the label as `Nights: ${min}-${max}` which yields awkward output like `Nights: 15-any`; update the badge creation in the block that pushes the duration badge (the badges.push call for key 'duration') to instead: if both nightsMin and nightsMax are present keep the "Nights: min-max" format, but when one end is open-ended (e.g., nightsMax === undefined or nightsMin === undefined) look up the human-friendly label from DURATION_OPTIONS (match by value) and use that label for the badge; keep the same onClear behavior (onChange({ nightsMin: undefined, nightsMax: undefined })). Ensure you reference filters.nightsMin, filters.nightsMax, DURATION_OPTIONS and the badges.push for key 'duration' when making the change.
278-331: Tier 2 selects lack<Label>elements, unlike Tier 1 controls.Tier 1 filters have explicit
<Label>elements for each control (Search, Ship, Departs From/To), but Cruise Line, Region, and Duration in Tier 2 do not. For consistent accessibility (screen reader association) and visual alignment, consider adding labels here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/app/library/cruises/_components/cruise-filters.tsx` around lines 278 - 331, Add explicit Label elements for the Tier 2 controls (Cruise Line, Region, Duration) to match Tier 1 accessibility and visual alignment: insert a <Label> for each control (e.g., "Cruise Line", "Region", "Duration") and associate it with the corresponding Select by giving the SelectTrigger a stable id (or the Select a generated id) and using that id as the Label's htmlFor; alternatively set an aria-label on the SelectTrigger if you prefer not to add an id. Update the JSX around the Select/SelectTrigger/SelectValue for cruiseLineId, regionId and duration to include these Labels and ensure their text matches existing placeholders for screen readers. Ensure keys remain unchanged and no logic in onValueChange or value props is modified.apps/api/src/trips/trips.controller.ts (1)
482-498: Avoidany+ generic identifier in unlinked-activities mappingLeaning on inference keeps the new fields type-checked and the mapping clearer.
♻️ Suggested change
- activities: activities.map((a: any) => ({ - id: a.id, - name: a.name, - activityType: a.activityType, + activities: activities.map((activity) => ({ + id: activity.id, + name: activity.name, + activityType: activity.activityType, itineraryId: '', // Not returned by findUnlinkedByTrip - frontend doesn't use this - itineraryDayId: a.itineraryDayId || '', + itineraryDayId: activity.itineraryDayId || '', dayNumber: null, // Would require additional query - frontend doesn't use this date: null, // Would require additional query - frontend doesn't use this - sequenceOrder: a.sequenceOrder, - totalPriceCents: a.pricing?.totalPriceCents ?? null, - parentActivityId: a.parentActivityId, - supplierName: a.supplierName ?? null, - isBooked: a.isBooked ?? false, - confirmationNumber: a.confirmationNumber ?? null, - paymentStatus: a.paymentStatus ?? null, - paidCents: a.paidCents ?? null, - currency: a.currency ?? null, + sequenceOrder: activity.sequenceOrder, + totalPriceCents: activity.pricing?.totalPriceCents ?? null, + parentActivityId: activity.parentActivityId, + supplierName: activity.supplierName ?? null, + isBooked: activity.isBooked ?? false, + confirmationNumber: activity.confirmationNumber ?? null, + paymentStatus: activity.paymentStatus ?? null, + paidCents: activity.paidCents ?? null, + currency: activity.currency ?? null, })),As per coding guidelines: Prefer explicit, domain-focused names (
trip,booking,itinerary) over generic terms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/trips/trips.controller.ts` around lines 482 - 498, The activities.map callback in trips.controller.ts uses a generic "a: any" which loses type-safety and uses a generic identifier; change the parameter to a domain-specific name (e.g., "activity" or "unlinkedActivity") and give it a proper type instead of any (for example UnlinkedActivity or the appropriate DTO/interface used by findUnlinkedByTrip) so the mapped fields remain type-checked; update the mapping expression (activities.map(...)) to use the new typed identifier and adjust any downstream property access accordingly.packages/shared-types/src/api/activities.types.ts (1)
333-349: TypepaymentStatusinstead of usingstringUsing a shared union keeps consumers aligned with valid values and prevents typos.
♻️ Suggested change
- paymentStatus: string | null // 'paid' | 'deposit_paid' | 'unpaid' | null (no schedule) + paymentStatus: PackagePaymentStatus | null // 'paid' | 'deposit_paid' | 'unpaid' | ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-types/src/api/activities.types.ts` around lines 333 - 349, Change the paymentStatus field on UnlinkedActivityDto from a plain string to a typed union (e.g., PaymentStatus | null) to lock allowed values; update the UnlinkedActivityDto definition to use the PaymentStatus type (or import it if defined elsewhere) and ensure PaymentStatus is exported/defined as the union of valid values ('paid' | 'deposit_paid' | 'unpaid') so consumers and type-checking align with valid statuses; then update any imports/usages of UnlinkedActivityDto to compile against the new PaymentStatus type.apps/admin/src/components/packages/packages-table.tsx (2)
364-369:paidCentsis declared but never read.
paidCentsis included in theUnlinkedActivitytype and presumably populated from the API, but it isn't mapped intoUnifiedBookingRow(lines 788-792) or rendered anywhere in this component. If it's carried solely for downstream consumers, consider documenting that intent; otherwise it's dead data in this context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/components/packages/packages-table.tsx` around lines 364 - 369, The UnlinkedActivity property paidCents is declared but never used; decide whether it should flow into UnifiedBookingRow or be removed: if it’s required downstream, add paidCents to the UnifiedBookingRow shape and include it in the mapping code that converts UnlinkedActivity -> UnifiedBookingRow (the mapping logic around the code that constructs UnifiedBookingRow from UnlinkedActivity), and then add a column/cell in the packages-table rendering to display paidCents (formatted via currency using currency/cents fields); otherwise remove paidCents from the UnlinkedActivity type and any API mapping that populates it, and add a short comment documenting the removal if needed.
1070-1070:as anycasts on Badgevariantprop bypass type safety.Four instances cast the variant to
any. The helpergetPaymentStatusVariantreturns'default' | 'secondary' | 'outline' | 'destructive', which likely already matches the Badge component'svariantprop. If it does, the cast is unnecessary; if it doesn't, a proper union type or a mapped type would be safer.Also applies to: 1165-1165, 1229-1229, 1237-1237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/components/packages/packages-table.tsx` at line 1070, Remove the unsafe "as any" casts on the Badge's variant prop and make the types align: either change getPaymentStatusVariant to explicitly return the Badge component's Variant union (or export/alias Badge's Variant type and use it as the return type of getPaymentStatusVariant) or map the function's returned union to the Badge's expected union before passing it; update the four Badge usages that currently use getPaymentStatusVariant(... ) as any (the instances around the Badge components) so they pass a value whose type matches Badge's variant prop without casting.
Comment on lines
61
to
+77
| const [searchInput, setSearchInput] = useState(filters.q ?? '') | ||
| const [portsPopoverOpen, setPortsPopoverOpen] = useState(false) | ||
|
|
||
| // Count active filters (excluding search and sort) | ||
| const activeFilterCount = [ | ||
| filters.cruiseLineId, | ||
| filters.shipId, | ||
| filters.regionId, | ||
| filters.embarkPortId, | ||
| // Live debounced search | ||
| const debouncedSearch = useDebounce(searchInput, 300) | ||
| const prevDebouncedRef = useRef(debouncedSearch) | ||
|
|
||
| useEffect(() => { | ||
| // Only fire when debounced value actually changes | ||
| if (debouncedSearch === prevDebouncedRef.current) return | ||
| prevDebouncedRef.current = debouncedSearch | ||
|
|
||
| const newQ = debouncedSearch || undefined | ||
| if (newQ !== filters.q) { | ||
| onChange({ q: newQ }) | ||
| } | ||
| }, [debouncedSearch, filters.q, onChange]) |
There was a problem hiding this comment.
searchInput won't sync if filters.q is updated externally (e.g., URL/back-navigation).
useState(filters.q ?? '') captures the initial value only. If the parent ever updates filters.q from outside (URL sync, browser back), the text input will show stale text. Consider adding a sync effect:
Proposed sync effect
+ // Keep local input in sync when filters.q changes externally
+ useEffect(() => {
+ const externalQ = filters.q ?? ''
+ if (externalQ !== searchInput && externalQ !== debouncedSearch) {
+ setSearchInput(externalQ)
+ prevDebouncedRef.current = externalQ
+ }
+ }, [filters.q])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/library/cruises/_components/cruise-filters.tsx` around
lines 61 - 77, The local state searchInput initialized with useState(filters.q
?? '') doesn't update when filters.q changes externally; add a short sync effect
that watches filters.q and calls setSearchInput(filters.q ?? '') so the input
reflects external updates (e.g., URL/back-nav). Locate the state variables
searchInput and setSearchInput and add a useEffect that compares filters.q to
searchInput (or previous value) and updates via setSearchInput when different to
avoid clobbering user typing while keeping external sync.
Comment on lines
+200
to
+202
| if (filters.portOfCallIds && filters.portOfCallIds.length > 0) { | ||
| filters.portOfCallIds.forEach(id => params.append('portOfCallIds', id)) | ||
| } |
There was a problem hiding this comment.
Biome flags implicit return from forEach callback.
The arrow function id => params.append(...) implicitly returns the result of append (void). While harmless at runtime, Biome's useIterableCallbackReturn rule flags this. Add braces to make the callback a statement body.
🔧 Proposed fix
if (filters.portOfCallIds && filters.portOfCallIds.length > 0) {
- filters.portOfCallIds.forEach(id => params.append('portOfCallIds', id))
+ filters.portOfCallIds.forEach(id => { params.append('portOfCallIds', id) })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (filters.portOfCallIds && filters.portOfCallIds.length > 0) { | |
| filters.portOfCallIds.forEach(id => params.append('portOfCallIds', id)) | |
| } | |
| if (filters.portOfCallIds && filters.portOfCallIds.length > 0) { | |
| filters.portOfCallIds.forEach(id => { params.append('portOfCallIds', id) }) | |
| } |
🧰 Tools
🪛 Biome (2.3.14)
[error] 201-201: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/app/library/cruises/page.tsx` around lines 200 - 202, The
Biome rule flags the implicit return in the forEach arrow callback; update the
callback used on filters.portOfCallIds.forEach to use a statement body instead
of an expression so it does not implicitly return a value — i.e. change the
callback for filters.portOfCallIds.forEach(id => params.append('portOfCallIds',
id)) to use braces (id => { params.append('portOfCallIds', id); }) so
params.append remains a statement and the lint rule is satisfied.
Comment on lines
1158
to
1176
| <td className="px-4 py-3 text-sm text-gray-900"> | ||
| {formatCurrency(row.totalPriceCents ?? 0, currency)} | ||
| </td> | ||
| <td className="px-4 py-3 text-sm text-gray-500">–</td> | ||
| <td className="px-4 py-3 text-sm text-gray-500">–</td> | ||
| <td className="px-4 py-3 text-sm text-gray-500">{row.supplierName || '–'}</td> | ||
| <td className="px-4 py-3 text-sm text-gray-500">{row.confirmationNumber || '–'}</td> | ||
| <td className="px-4 py-3"> | ||
| <Badge variant="outline">Unpaid</Badge> | ||
| {row.paymentStatus ? ( | ||
| <Badge variant={getPaymentStatusVariant(row.paymentStatus) as any}> | ||
| {getPaymentStatusLabel(row.paymentStatus)} | ||
| </Badge> | ||
| ) : ( | ||
| <span className="text-sm text-gray-400">–</span> | ||
| )} | ||
| </td> | ||
| <td className="px-4 py-3"> | ||
| <Badge variant="outline">Not Booked</Badge> | ||
| <Badge variant={row.isBooked ? 'default' : 'outline'}> | ||
| {row.isBooked ? 'Booked' : 'Not Booked'} | ||
| </Badge> | ||
| </td> |
There was a problem hiding this comment.
Column semantics: "Commission" header now shows booking status for activity rows.
The table header at line 994 reads "Commission", but the activity-row cell (lines 1173-1175) now renders a Booked / Not Booked badge driven by row.isBooked. Package rows still render commissionStatus (which can be "Booked" / "Upcoming" / "Not Booked"). This mismatch between the header label and the actual cell content for activity rows could confuse users — consider renaming the header to something more inclusive like "Status" or "Booking Status", or keeping a commission-relevant value in the activity rows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/components/packages/packages-table.tsx` around lines 1158 -
1176, Header label "Commission" is mismatched with activity-row content that
renders a Booked/Not Booked badge using row.isBooked while package rows show
commissionStatus; update the table to be consistent by either renaming the
header text from "Commission" to a neutral label like "Status" or "Booking
Status" where the header string is defined, or change the activity-row cell to
render a commission-related value (e.g., use row.commissionStatus) instead of
the isBooked badge; locate the header string and the cell rendering code
referencing row.isBooked and commissionStatus to apply the chosen fix so header
semantics match row content.
Comment on lines
+384
to
+391
| // Refresh Bookings tab (payment status computed from transactions) | ||
| queryClient.invalidateQueries({ queryKey: ['bookings', 'list'] }) | ||
| queryClient.invalidateQueries({ queryKey: ['bookings', 'tripTotals', tripId] }) | ||
| queryClient.invalidateQueries({ queryKey: ['bookings', 'unlinkedActivities', tripId] }) | ||
| } | ||
| // Refresh Trip Overview (activities may show updated payment status) | ||
| queryClient.invalidateQueries({ queryKey: ['activities'] }) | ||
| queryClient.invalidateQueries({ queryKey: ['itinerary-days'] }) |
There was a problem hiding this comment.
useDeletePaymentTransaction is missing symmetric cache invalidations.
Creating a transaction now correctly invalidates bookings and activity caches, but useDeletePaymentTransaction (Line 411) does not perform the same invalidations. Deleting a transaction also changes payment status, so the bookings tab totals, unlinked activities, and activity/itinerary-day views will show stale data until the user manually triggers a refetch.
Proposed fix
Add matching invalidations in useDeletePaymentTransaction. This requires threading tripId into the hook (similar to useCreatePaymentTransaction):
-export function useDeletePaymentTransaction(activityPricingId: string, expectedPaymentItemId: string) {
+export function useDeletePaymentTransaction(activityPricingId: string, expectedPaymentItemId: string, tripId?: string) {
const queryClient = useQueryClient()
const { toast } = useToast()
return useMutation({
mutationFn: async (transactionId: string) => {
return api.delete(`/payment-schedules/transactions/${transactionId}`)
},
onSuccess: () => {
queryClient.invalidateQueries({
queryKey: paymentScheduleKeys.byActivityPricing(activityPricingId),
})
queryClient.invalidateQueries({
queryKey: paymentScheduleKeys.transactions(expectedPaymentItemId),
})
+ if (tripId) {
+ queryClient.invalidateQueries({
+ queryKey: paymentScheduleKeys.tripExpectedPayments(tripId),
+ })
+ queryClient.invalidateQueries({
+ queryKey: paymentScheduleKeys.tripTransactions(tripId),
+ })
+ queryClient.invalidateQueries({ queryKey: ['bookings', 'list'] })
+ queryClient.invalidateQueries({ queryKey: ['bookings', 'tripTotals', tripId] })
+ queryClient.invalidateQueries({ queryKey: ['bookings', 'unlinkedActivities', tripId] })
+ }
+ queryClient.invalidateQueries({ queryKey: ['activities'] })
+ queryClient.invalidateQueries({ queryKey: ['itinerary-days'] })
toast({
title: 'Transaction deleted',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Refresh Bookings tab (payment status computed from transactions) | |
| queryClient.invalidateQueries({ queryKey: ['bookings', 'list'] }) | |
| queryClient.invalidateQueries({ queryKey: ['bookings', 'tripTotals', tripId] }) | |
| queryClient.invalidateQueries({ queryKey: ['bookings', 'unlinkedActivities', tripId] }) | |
| } | |
| // Refresh Trip Overview (activities may show updated payment status) | |
| queryClient.invalidateQueries({ queryKey: ['activities'] }) | |
| queryClient.invalidateQueries({ queryKey: ['itinerary-days'] }) | |
| export function useDeletePaymentTransaction(activityPricingId: string, expectedPaymentItemId: string, tripId?: string) { | |
| const queryClient = useQueryClient() | |
| const { toast } = useToast() | |
| return useMutation({ | |
| mutationFn: async (transactionId: string) => { | |
| return api.delete(`/payment-schedules/transactions/${transactionId}`) | |
| }, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ | |
| queryKey: paymentScheduleKeys.byActivityPricing(activityPricingId), | |
| }) | |
| queryClient.invalidateQueries({ | |
| queryKey: paymentScheduleKeys.transactions(expectedPaymentItemId), | |
| }) | |
| if (tripId) { | |
| queryClient.invalidateQueries({ | |
| queryKey: paymentScheduleKeys.tripExpectedPayments(tripId), | |
| }) | |
| queryClient.invalidateQueries({ | |
| queryKey: paymentScheduleKeys.tripTransactions(tripId), | |
| }) | |
| queryClient.invalidateQueries({ queryKey: ['bookings', 'list'] }) | |
| queryClient.invalidateQueries({ queryKey: ['bookings', 'tripTotals', tripId] }) | |
| queryClient.invalidateQueries({ queryKey: ['bookings', 'unlinkedActivities', tripId] }) | |
| } | |
| queryClient.invalidateQueries({ queryKey: ['activities'] }) | |
| queryClient.invalidateQueries({ queryKey: ['itinerary-days'] }) | |
| toast({ | |
| title: 'Transaction deleted', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/admin/src/hooks/use-payment-schedules.ts` around lines 384 - 391, The
delete hook useDeletePaymentTransaction is missing the same cache invalidations
as useCreatePaymentTransaction so deleting a transaction leaves
bookings/activities stale; update useDeletePaymentTransaction to accept/traverse
the tripId (like useCreatePaymentTransaction) and call
queryClient.invalidateQueries for ['bookings','list'], ['bookings','tripTotals',
tripId], ['bookings','unlinkedActivities', tripId], ['activities'], and
['itinerary-days'] after a successful delete, ensuring you locate the
invalidation logic near the current invalidateQueries calls in
useCreatePaymentTransaction and mirror it in the delete success handler.
Comment on lines
+821
to
+867
| private async validateTripHasTravelersForPayment(expectedPaymentItemId: string): Promise<void> { | ||
| const result = await this.db.client | ||
| .select({ | ||
| activityId: this.db.schema.activityPricing.activityId, | ||
| }) | ||
| .from(this.db.schema.expectedPaymentItems) | ||
| .innerJoin( | ||
| this.db.schema.paymentScheduleConfig, | ||
| eq(this.db.schema.expectedPaymentItems.paymentScheduleConfigId, this.db.schema.paymentScheduleConfig.id) | ||
| ) | ||
| .innerJoin( | ||
| this.db.schema.activityPricing, | ||
| eq(this.db.schema.paymentScheduleConfig.activityPricingId, this.db.schema.activityPricing.id) | ||
| ) | ||
| .where(eq(this.db.schema.expectedPaymentItems.id, expectedPaymentItemId)) | ||
| .limit(1) | ||
|
|
||
| if (!result[0]) return // Can't resolve chain — skip validation | ||
|
|
||
| // Resolve tripId from the activity (direct trip_id or via itinerary chain) | ||
| const [activity] = await this.db.client | ||
| .select({ | ||
| directTripId: this.db.schema.itineraryActivities.tripId, | ||
| itineraryDayId: this.db.schema.itineraryActivities.itineraryDayId, | ||
| }) | ||
| .from(this.db.schema.itineraryActivities) | ||
| .where(eq(this.db.schema.itineraryActivities.id, result[0].activityId)) | ||
| .limit(1) | ||
|
|
||
| if (!activity) return | ||
|
|
||
| let tripId = activity.directTripId | ||
| if (!tripId && activity.itineraryDayId) { | ||
| const [dayResult] = await this.db.client | ||
| .select({ tripId: this.db.schema.itineraries.tripId }) | ||
| .from(this.db.schema.itineraryDays) | ||
| .innerJoin( | ||
| this.db.schema.itineraries, | ||
| eq(this.db.schema.itineraryDays.itineraryId, this.db.schema.itineraries.id) | ||
| ) | ||
| .where(eq(this.db.schema.itineraryDays.id, activity.itineraryDayId)) | ||
| .limit(1) | ||
| tripId = dayResult?.tripId ?? null | ||
| } | ||
|
|
||
| if (!tripId) return // Can't resolve trip — skip validation | ||
|
|
There was a problem hiding this comment.
Fail closed when trip resolution fails in traveler validation
The early return paths skip validation entirely if any link in the chain is missing. That allows payments to be recorded without travelers, which undermines the guard. Consider throwing a BadRequestException (or at least logging and blocking) when the chain can’t be resolved.
🔧 Proposed fix
- if (!result[0]) return // Can't resolve chain — skip validation
+ if (!result[0]) {
+ this.logger.warn(`Payment item ${expectedPaymentItemId} could not resolve activity for traveler validation`)
+ throw new BadRequestException('Cannot record payment: unable to resolve trip for this payment item.')
+ }
...
- if (!activity) return
+ if (!activity) {
+ throw new BadRequestException('Cannot record payment: activity not found for this payment item.')
+ }
...
- if (!tripId) return // Can't resolve trip — skip validation
+ if (!tripId) {
+ throw new BadRequestException('Cannot record payment: trip not found for this payment item.')
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/trips/payment-schedules.service.ts` around lines 821 - 867, The
validateTripHasTravelersForPayment function currently returns early when the
expectedPaymentItem → activity → trip chain can’t be resolved (checks for
result[0], activity, and tripId), which allows payments to bypass traveler
validation; update those early-return branches to throw a BadRequestException
(or another appropriate HTTP error) with a clear message referencing the missing
link (include expectedPaymentItemId and which step failed) so the caller is
blocked instead of silently skipping validation; ensure you update the checks at
the top of validateTripHasTravelersForPayment (the result[0] check), the
activity resolution block (the activity missing check), and the tripId
resolution block (the !tripId check) to throw, not return.
7 tasks
3 tasks
4 tasks
4 tasks
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
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements