Repository navigation
VAY-1244: Redesign onboarding payment setup - #826
FlamurMaliqi merged 5 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds Stripe card payments, PayPal and bank-transfer support, finance plan selection, manual PMS payment commands, payment-readiness projections, booking recovery, lifecycle workers, Resend email delivery, and billing-plan setup flows. ChangesBooking and payment platform
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/routes/bookingWebPublic.ts (1)
1862-1893: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBank-transfer bookings never receive their payment instructions.
createTargetGuestBookingpersistspaymentInstructions.bankTransferDetailsinto booking metadata forbank_transferbookings (lines 2775-2783, 2793).getPaymentInstructionsreadspaymentInstructionsfrom metadata for PayPal, but returns a hardcodedbankTransfer: { enabled: false, details: null }.The removed reserved-pending-payment email helper no longer delivers those details either. The parity test at
apps/api/src/routes/bookingWebPublicParity.test.tslines 2204-2206 asserts that no pending-payment email job is emitted.A guest who selects bank transfer therefore has no path to the IBAN, and the booking expires after the 24-hour
pendingExpiresAtwindow.Derive the bank-transfer fields from booking metadata, in the same way as the PayPal branch.
🐛 Proposed fix
const paypalEmail = stringValue(paymentInstructions["paypalEmail"]); + const pendingUnpaid = + booking.lifecycleStatus === "pending_payment" && booking.paymentStatus === "unpaid"; + const bankTransferDetails = paymentInstructions["bankTransferDetails"] ?? null; + const bankTransferEnabled = + pendingUnpaid && + stringValue(metadata["paymentMethod"]) === "bank_transfer" && + bankTransferDetails !== null; const paypalEnabled = - booking.lifecycleStatus === "pending_payment" && - booking.paymentStatus === "unpaid" && + pendingUnpaid && stringValue(metadata["paymentMethod"]) === "paypal" && isValidPaymentEmail(paypalEmail);bankTransfer: { - enabled: false, - details: null, + enabled: bankTransferEnabled, + details: bankTransferEnabled ? bankTransferDetails : null, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/bookingWebPublic.ts` around lines 1862 - 1893, Update getPaymentInstructions to derive bank-transfer instructions from booking.bookingMetadata instead of returning the hardcoded disabled/null values. Read paymentInstructions["bankTransferDetails"], set bankTransfer.enabled based on the booking’s bank-transfer payment method and available details, and return the persisted details while preserving the existing PayPal behavior.apps/api/src/routes/finance.ts (1)
7392-7409: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the unreachable duplicate
case "paypal".Line 7400 already returns
"paypal". The secondcase "paypal"at Line 7404 can never run. Most lint configurations reject duplicate switch cases (no-duplicate-case), so this can failnpm run lint.🧹 Proposed cleanup
case "bank_transfer": case "paypal": case "manual_card": case "wallet": return method; - case "paypal": - return "wallet"; default: return "other";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/finance.ts` around lines 7392 - 7409, Remove the unreachable duplicate case "paypal" branch from paymentMethods, retaining the earlier branch that returns "paypal" and leaving all other method mappings unchanged.
🟠 Major comments (31)
apps/api/src/routes/bookingWebPublic.ts-2085-2092 (1)
2085-2092: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
pay_at_propertyreadiness is defined twice with different rules.serializeTargetCheckoutConfigandassertTargetCheckoutConfigMatchesQuoteeach decide whetherpay_at_propertyis usable, and they disagree. The checkout-config response advertisespay_at_propertywhenever the method is accepted, but booking creation additionally requirescashormanual_card. A property that acceptspay_at_propertyalone shows the option to the guest and then fails the booking with HTTP 409 "Selected payment method is no longer available. Please refresh." The shared root cause is the duplicated readiness predicate; extract one helper and call it from both sites.
apps/api/src/routes/bookingWebPublic.ts#L2085-L2092: replace thereturn truefallthrough sopay_at_propertyrequirescashormanual_cardinrow?.acceptedMethods, matching the booking-time rule.apps/api/src/routes/bookingWebPublic.ts#L2525-L2537: call the same extracted helper instead of restating theaccepted.some((candidate) => candidate === "cash" || candidate === "manual_card")condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/bookingWebPublic.ts` around lines 2085 - 2092, Extract a shared readiness helper for pay_at_property in apps/api/src/routes/bookingWebPublic.ts, requiring acceptedMethods to include cash or manual_card. At lines 2085-2092, replace the unconditional fallback with this helper, and at lines 2525-2537 replace the duplicated accepted-methods predicate with the same helper; preserve the existing readiness checks for other payment methods.apps/api/src/routes/bookingWebPublic.ts-3217-3239 (1)
3217-3239: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
FOR UPDATElocks the joined provider-account row.The query joins
finance.payment_provider_accountsand ends with a bareFOR UPDATE. PostgreSQL locks the selected rows of every table in theFROMlist, so this also takes a row lock on the property's provider-account row.Every concurrent card confirmation and replay for the same property contends on that single row. This serializes settlements per property and increases deadlock risk against
reconcileStripeProviderAccountinapps/api/src/platform/providerWebhooks.ts, which updates provider accounts inside its own transaction.Restrict the lock to the payment row.
🔒️ Proposed fix
ORDER BY payment.created_at DESC LIMIT 1 - FOR UPDATE`, + FOR UPDATE OF payment`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/bookingWebPublic.ts` around lines 3217 - 3239, Update the SQL in loadTargetCardPayment so the trailing row lock targets only the payment table, using the payment alias rather than a bare FOR UPDATE. Leave the provider-account join and query behavior unchanged.apps/api/src/routes/bookingWebPublic.ts-1622-1664 (1)
1622-1664: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd guest verification to
confirmAuthorization.Every other guest-facing booking operation in this adapter resolves the booking through
loadTargetBooking, which requiresrequireGuestEmailand joinsbooking.booking_guestson a matching email.confirmAuthorizationcallsloadTargetHotelBooking, which matches onproperty_idandhandleonly.Before this change the method threw an unavailable error, so this is new exposure. Any caller who knows the booking reference can now trigger settlement and read the booking totals, stay dates, occupancy, and payment status.
The public reference is high entropy, so enumeration is impractical. The authorization model is still inconsistent with
getStatus,lookup,cancel, andwithdraw.Require the guest email on the route and resolve the booking with
loadTargetBooking, or state why this endpoint intentionally accepts reference-only proof.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/bookingWebPublic.ts` around lines 1622 - 1664, Update confirmAuthorization to require guest email verification, matching getStatus, lookup, cancel, and withdraw. Accept the email in the route context and replace loadTargetHotelBooking with loadTargetBooking so resolution validates requireGuestEmail against booking.booking_guests; preserve the existing settlement and confirmation flow after authenticated lookup.apps/api/src/routes/finance.ts-575-576 (1)
575-576: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA publication failure discards a committed settings update.
options.repository.updatePaymentSettingsalready committed when this line runs. Ifpublishrejects, Fastify returns 500 and the client believes the save failed, but the settings are persisted. Log the publication failure and still return the success response, or make the publication retryable through the outbox.🛡️ Proposed fix
- await options.publicBookabilityPublisher?.publish({ propertyId }); + try { + await options.publicBookabilityPublisher?.publish({ propertyId }); + } catch (error) { + request.log.warn( + { err: error, propertyId }, + "Public bookability publication after payment-settings update failed", + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/finance.ts` around lines 575 - 576, Update the flow around repository.updatePaymentSettings and publicBookabilityPublisher.publish so a publication rejection does not propagate as a failed settings update: catch the publish error, log the failure, and continue to the existing success response. Keep the committed settings update and response behavior unchanged; do not let publish determine the request status.apps/api/src/routes/providerWebhooks.ts-137-148 (1)
137-148: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep receipt hashes stable across Stripe redaction
recordReceiptreturnsconflictwhen the existingpayload_hashdiffers. Stripe retries for previously stored payloads that contain redacted fields can therefore return HTTP 409 instead ofduplicate, including retries that must promote an observed receipt. Hash the original verified payload while persisting the redacted payload, or define a compatible migration and retry plan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/providerWebhooks.ts` around lines 137 - 148, Update the Stripe webhook flow in handleAuthenticatedProviderWebhook so receiptKey hashing and recordReceipt comparisons use the original verified payload, while rawPayload continues persisting the redacted payload. Preserve stable hashes for retries, including observed-receipt promotion, and avoid treating redaction changes as payload conflicts.apps/api/src/routes/providerWebhooks.ts-654-680 (1)
654-680: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard provider-account writes against stale events.
reconcileStripeProviderAccountwrites during webhook promotion and does not compareeventCreated. Store the latest event timestamp and conditionally updatecharges_enabled,payouts_enabled,status, andonboarding_statusonly when the incoming event is not older than the stored event.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/providerWebhooks.ts` around lines 654 - 680, Update the Stripe provider-account webhook promotion flow and reconcileStripeProviderAccount to persist the latest eventCreated timestamp, then conditionally update charges_enabled, payouts_enabled, status, and onboarding_status only when the incoming eventCreated is greater than or equal to the stored timestamp. Preserve the existing updates for non-stale events while preventing older webhook events from overwriting newer account state.apps/api/src/routes/finance.ts-3858-3863 (1)
3858-3863: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn a typed error for missing commission terms.
The migration assigns existing commission bookings an empty
commission_terms_snapshot.manualPaymentCommissionthen throws a genericError, which the transaction rolls back and rethrows. The route does not map this exception, so the request returns an unhandled HTTP 500. Returninvalid_commandbefore reserving idempotency or writing the payment, and add a regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/finance.ts` around lines 3858 - 3863, The manual payment flow must handle missing commission terms as a typed invalid command instead of allowing manualPaymentCommission to throw an unhandled error. In the function containing the feeAmount/netAmount calculation, validate the invoice’s commission terms before reserving idempotency or writing the payment, return invalid_command when absent, and add a regression test covering the empty commission_terms_snapshot case.apps/api/src/domains/financeSubscriptionService.ts-47-93 (1)
47-93: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
afterPlanChangefailures behave consistently.
afterPlanChangeruns on six paths with two different error behaviors:
- Line 57, Line 90, Line 106, Line 123: the call sits outside any
try. A rejection propagates as a thrown exception.- Line 149 and Line 226: the call sits inside the
try. A rejection is caught and returned asproviderFailure(error).Two problems follow. First, callers cannot handle a publication failure uniformly, because the same failure is sometimes an exception and sometimes a typed result. Second, at Line 149 the store commit already succeeded, so returning
providerFailurereports a Stripe provider failure for what is a projection failure. The caller can conclude that no checkout was created while a Stripe session exists and the entitlement row is written.Choose one behavior for all paths. Running
afterPlanChangeoutside the providertryon every path keeps the provider error class accurate.♻️ Proposed change for the Fixed checkout path
const recorded = await config.store.recordCheckout(command, value); - await config.afterPlanChange?.(command.propertyId); - return { ok: true, status: recorded.status, value: recorded.result }; + committed = { ok: true as const, status: recorded.status, value: recorded.result }; } catch (error) { return providerFailure(error); } + await config.afterPlanChange?.(command.propertyId); + return committed;Also applies to: 95-155, 226-226
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/financeSubscriptionService.ts` around lines 47 - 93, Make all six afterPlanChange call sites use consistent post-commit failure handling by moving the Fixed checkout path’s call outside the provider try/catch, including the paths in selectCommissionPlan and the other affected operations. Keep Stripe operations inside their existing try blocks so only provider errors become providerFailure results; let afterPlanChange rejections propagate as exceptions after the store commit.apps/api/src/domains/financeBillingConfigReadModel.ts-66-69 (1)
66-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove the hard-coded rate and rule-id predicates from the commission lookup.
The lateral join is an inner join. Two predicates make it fail closed for valid data:
percentage_rate = 5excludes any property whose commission rate differs from 5. The query already selectspercentage_rateinto"percentageRate", so the rate does not need to be constant for the read to be correct.source_rule_id = 'onboarding-booking:' || property.id::textexcludes any activebookingcommission rule that was not created by the onboarding writer, for example a migrated or manually corrected rule.If either predicate rejects the row,
getBillingConfigreturnsnull. The public checkout inapps/api/src/routes/bookingWebPublic.tsthen throws 409 "Finance billing configuration is not available." A commission-rate change therefore becomes a guest-facing checkout outage.Keep the
source_system,product,status, and validity-window predicates. Drop the rate equality. Keep the rule-id filter only if the contract guarantees that onboarding owns every booking commission rule.🛡️ Proposed fix
AND source_system = 'finance' - AND source_rule_id = 'onboarding-booking:' || property.id::text AND commission_type = 'percentage' - AND percentage_rate = 5 AND status = 'active'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/financeBillingConfigReadModel.ts` around lines 66 - 69, Update the commission lookup’s lateral-join predicates near source_system, product, and status: remove the hard-coded percentage_rate = 5 condition and remove the source_rule_id onboarding-booking filter unless the data contract explicitly guarantees onboarding ownership of every booking commission rule. Preserve the source_system, product, status, and validity-window predicates, and continue selecting percentage_rate for the percentageRate field.apps/api/src/domains/financeSubscriptionService.ts-68-78 (1)
68-78: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExpire every stored Checkout session before recording Commission.
recordCheckoutpersistscommand.audit.requestedAt, although Stripe creates the session first. The 24-hour internal window can therefore end before Stripe’s 24-hour session lifetime. Commission selection then clearscheckoutSessionRef, and a later completion event is ignored as stale.Expire the session whenever
checkoutSessionRefexists. Treat an already-expired session as safe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/financeSubscriptionService.ts` around lines 68 - 78, The checkout-expiration logic around recordCheckout must run whenever entitlement.checkoutSessionRef exists, not only while isCheckoutPending(entitlement, now()) is true. Preserve the existing Stripe configuration and providerFailure handling, and treat an already-expired session as a successful no-op so recording Commission can continue.apps/booking-admin/services/api/financePaymentSettingsClient.ts-272-288 (1)
272-288: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
depositPolicyis used as an untyped blob for structured payout data, and the round trip is lossy. Both sites treatdepositPolicyas free-form key-value storage for bank, payout, and PayPal fields. There is no declared schema, no field that records the payout account format, and no preservation of keys the client does not know about. The result is that a save can both erase unknown keys and change the meaning of a stored value on the next load.
apps/booking-admin/services/api/financePaymentSettingsClient.ts#L272-L288: stop replacing the whole object. Spread the loaded policy, then overwrite only the keys this client owns, and add an explicitaccountFormatkey.apps/booking-admin/app/(app)/settings/page.tsx#L393-L397: readaccountFormatfromdepositPolicyinstead of hardcodingpayout_account_typeto"account_number", and restore the stored value intopayout_ibanorpayout_account_numberaccording to that format.Define the payout and PayPal policy shape once in a shared type, and reuse it in both the writer and the reader.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/booking-admin/services/api/financePaymentSettingsClient.ts` around lines 272 - 288, Define one shared typed payout/PayPal policy shape and reuse it in the writer and reader. In financePaymentSettingsClient.ts lines 272-288, preserve the loaded depositPolicy by spreading it, overwrite only client-owned fields, and store the selected account format in accountFormat. In apps/booking-admin/app/(app)/settings/page.tsx lines 393-397, read accountFormat instead of hardcoding payout_account_type, then restore the stored account value into payout_iban or payout_account_number accordingly.apps/marketplace-web/components/setup/operations/PaymentSetupForm.tsx-502-533 (1)
502-533: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA persisted
xenditorvayadaprovider locks the merchant out of Stripe.Line 616 to line 619 keep
xenditorvayadaasdraft.onlineProviderwhen the saved settings use that provider. Line 520 and line 527 disable those radio inputs, and the Stripe radio is a separate input in the same group. The merchant cannot select Stripe, because selecting Stripe requires clicking a radio while the currently checked radio is disabled — the Stripe radio is enabled, so the switch is possible, but the disabled selected option stays checked until the user notices it.The larger problem is submit behavior.
needsStripeSetupreturns false for a non-Stripe provider, sohandleSubmitsavesonline_cardwith an unavailable provider and marks the task complete. The PR states that Xendit and vayada Payments are unavailable.apps/booking-admin/app/(app)/settings/page.tsxline 486 to line 489 rejects the save for both providers. This form accepts it.Block the save when
draft.methodsincludesonline_cardanddraft.onlineProvideris notstripe, and surface the same "coming soon" message.🛡️ Proposed guard in
handleSubmitif (draft.methods.includes("pay_at_property") && draft.payAtHotelMethods.length === 0) { setError("Choose cash, card, or both for Pay at Hotel."); return; } + if (draft.methods.includes("online_card") && draft.onlineProvider !== "stripe") { + setError( + `${draft.onlineProvider === "xendit" ? "Xendit" : "vayada Payments"} is coming soon. Select Stripe Connect to accept online card payments.`, + ); + return; + }Also applies to: 616-619
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/marketplace-web/components/setup/operations/PaymentSetupForm.tsx` around lines 502 - 533, Update handleSubmit to reject submissions when draft.methods includes online_card and draft.onlineProvider is not "stripe", displaying the existing “coming soon” message instead of saving or completing the task. Also ensure the persisted-provider initialization around ProviderOptions does not allow unavailable xendit or vayada selections to bypass this guard; preserve normal Stripe submission behavior.apps/pms-web/app/(app)/bookings/[id]/page.tsx-2010-2034 (1)
2010-2034: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGive the disabled "mark received" control a visible reason, and scope the deadline to bank transfer.
Two problems exist in this block:
- The deadline banner at Line 2015 renders only when
hasAcceptedBankDeadlineis true, which requirespaymentMethod === "bank_transfer". The button at Line 2025 is disabled bypaymentDeadlineExpiredfor bothbank_transferandpay_at_property. A confirmedpay_at_propertybooking that still carries ahostResponseDeadlinefrom its pending phase therefore shows a permanently disabled button with no countdown and no message. The host cannot record the payment and cannot see why.- Even for
bank_transfer, the disabled button has notitleor adjacent text after expiry.Gate the disable on the same condition that renders the countdown, and add an explanation.
🐛 Proposed fix
<button onClick={handleMarkPaid} - disabled={updating || paymentDeadlineExpired} + disabled={updating || (Boolean(hasAcceptedBankDeadline) && paymentDeadlineExpired)} + title={ + hasAcceptedBankDeadline && paymentDeadlineExpired + ? "The payment deadline expired. This reservation is being released." + : undefined + } className="inline-flex items-center gap-1.5 rounded-lg bg-emerald-600 px-4 py-2 text-sm font-medium text-white hover:bg-emerald-700 disabled:opacity-50" >Apply the same condition inside
handleMarkPaidat Line 1279.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/pms-web/app/`(app)/bookings/[id]/page.tsx around lines 2010 - 2034, Update the mark-received control and handleMarkPaid so paymentDeadlineExpired is applied only for bank_transfer bookings with the accepted bank deadline condition, leaving pay_at_property enabled. Add a visible explanation for the disabled expired bank-transfer control, such as a title or adjacent text, while preserving the existing countdown and payment-status behavior.Source: Path instructions
packages/backend-migration/migrations/0066_finance_paypal_payment_method.sql-1-35 (1)
1-35: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd the widened constraints as
NOT VALID, then validate in a follow-up migration.Each
ADD CONSTRAINT ... CHECKtriggers a full table scan under anACCESS EXCLUSIVElock. Reads and writes tofinance.paymentsblock for the duration of the scan, and that table grows with every booking. The new vocabularies are strict supersets of the old ones, so no existing row can violate them. The validation scan therefore provides no value at this point.Also add
IF EXISTSto eachDROP CONSTRAINTso a partial rerun does not abort.This repository already separates constraint validation into a successor migration. Migration
0030defers its validation to migration0031.🔒️ Proposed fix
ALTER TABLE finance.payment_settings - DROP CONSTRAINT chk_finance_payment_settings_accepted_methods; + DROP CONSTRAINT IF EXISTS chk_finance_payment_settings_accepted_methods; ALTER TABLE finance.payment_settings ADD CONSTRAINT chk_finance_payment_settings_accepted_methods CHECK ( accepted_methods <@ ARRAY[ 'card', 'pay_at_property', 'xendit', 'cash', 'bank_transfer', 'paypal', 'manual_card', 'wallet', 'other' ]::TEXT[] - ); + ) NOT VALID; ALTER TABLE finance.payments - DROP CONSTRAINT payments_payment_method_check; + DROP CONSTRAINT IF EXISTS payments_payment_method_check; ALTER TABLE finance.payments ADD CONSTRAINT payments_payment_method_check CHECK ( payment_method IN ( 'card', 'pay_at_property', 'xendit', 'bank_transfer', 'paypal', 'wallet', 'cash', 'manual_card', 'other', 'unknown' ) - ); + ) NOT VALID; ALTER TABLE distribution.public_room_offer_snapshots - DROP CONSTRAINT chk_distribution_room_offer_snapshots_payment_options; + DROP CONSTRAINT IF EXISTS chk_distribution_room_offer_snapshots_payment_options; ALTER TABLE distribution.public_room_offer_snapshots ADD CONSTRAINT chk_distribution_room_offer_snapshots_payment_options CHECK ( payment_options <@ ARRAY[ 'card', 'pay_at_property', 'xendit', 'cash', 'bank_transfer', 'paypal', 'manual_card', 'wallet', 'other' ]::TEXT[] - ); + ) NOT VALID;Then add
0067_finance_paypal_payment_method_validate.sql:ALTER TABLE finance.payment_settings VALIDATE CONSTRAINT chk_finance_payment_settings_accepted_methods; ALTER TABLE finance.payments VALIDATE CONSTRAINT payments_payment_method_check; ALTER TABLE distribution.public_room_offer_snapshots VALIDATE CONSTRAINT chk_distribution_room_offer_snapshots_payment_options;Note that
packages/backend-migration/src/financePaypalPaymentMethod.test.tsasserts three occurrences of'paypal'in this file, so that assertion stays valid. Based on learnings, constraint validation for migration0030is intentionally run in the subsequent migration0031. As per static analysis, Squawk reportsconstraint-missing-not-validfor all three added constraints.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend-migration/migrations/0066_finance_paypal_payment_method.sql` around lines 1 - 35, Update the three ADD CONSTRAINT statements in the migration to create the widened CHECK constraints with NOT VALID, and make each DROP CONSTRAINT use IF EXISTS for safe partial reruns. Add successor migration 0067 to VALIDATE the three named constraints on finance.payment_settings, finance.payments, and distribution.public_room_offer_snapshots, preserving the existing constraint names and all three paypal entries in the original migration.Sources: Learnings, Linters/SAST tools
apps/marketplace-web/services/api/hotelOperationsSetupClient.ts-648-652 (1)
648-652: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject unavailable online providers before saving.
paymentDraftFromSettingscan restorexenditorvayadaeven though the form disables both options. A later save sendspaymentsEnabled: true, and the API accepts both providers. Thevayadaplaceholder also setschargesEnabled: true, so["card"]can become publicly chargeable without a runtime provider. Normalize restored drafts tostripeor reject these providers server-side. Add regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/marketplace-web/services/api/hotelOperationsSetupClient.ts` around lines 648 - 652, The paymentProvider selection in the save flow must not persist restored xendit or vayada values when those options are unavailable. Normalize any online provider to the supported stripe provider, or reject unsupported providers before constructing the API payload, ensuring vayada cannot enable publicly chargeable card payments without a runtime provider; add regression coverage for restored xendit and vayada drafts.apps/api/src/domains/pmsOperationsCommandRepository.ts-4166-4173 (1)
4166-4173: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn a typed failure when Finance rejects the payment. Do not throw.
recordManualPaymentInClientreturns a discriminated result with acode. This block discards that code and throws a genericError. The wrapper converts it to an unhandled failure, so the operator receives a 500 and no reason. Business rejections such as a duplicate settlement or a currency mismatch are expected outcomes, not internal faults.Every other mutation in this file returns a typed error result and lets the executor roll back. Follow that pattern.
🔧 Proposed fix
const financeResult = await recordManualPaymentInClient(client, financeCommand, { enqueueSettlementEmail: false, }); if (!financeResult.ok) { - throw new Error( - `Finance rejected PMS booking payment ${command.guestBookingId}: ${financeResult.code}`, - ); + return { + ok: false, + statusCode: 409, + code: financeResult.code, + message: `Finance rejected the booking payment: ${financeResult.code}`, + }; }Map
financeResult.statusCodethrough when it is available.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/pmsOperationsCommandRepository.ts` around lines 4166 - 4173, Update the financeResult failure branch in the surrounding PMS payment mutation to return the repository’s typed failure result instead of throwing. Preserve and propagate financeResult.code, and map financeResult.statusCode when available, following the existing typed-error mutation pattern so the executor can roll back and callers receive the business rejection.</code>apps/api/src/domains/pmsOperationsCommandRepository.ts-4231-4242 (1)
4231-4242: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe final confirmation email reports a stale outstanding balance.
Line 4180 sets
balance_amount = 0in the database. Line 4241 builds the email payload frombookingEmailSnapshot(booking), andbookingis the row loaded before the update at line 4123.bookingEmailSnapshotcopiesbooking.balanceAmount(line 4319), which is still the pre-payment amount.The guest therefore receives a "payment received, booking confirmed" email that shows the full amount as still outstanding.
🐛 Proposed fix
await enqueueBookingLifecycleEmailJob(client, { kind: "final_confirmation", occurredAt: acceptedAt, correlationId: command.audit.correlationId ?? command.audit.requestId, causationId: command.commandId, actor: command.audit.actor.kind === "user" ? { type: "user", userId: command.audit.actor.userId } : { type: "system" }, source: "apps/api-pms-booking-payment", - booking: bookingEmailSnapshot(booking), + booking: { ...bookingEmailSnapshot(booking), balanceAmount: "0" }, });Add an assertion in
apps/api/src/domains/pmsOperationsCommandRepository.test.tsthat theemail.booking-final-confirmationjob payload carries a zero balance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/pmsOperationsCommandRepository.ts` around lines 4231 - 4242, Update the final-confirmation payload around enqueueBookingLifecycleEmailJob so bookingEmailSnapshot receives the post-payment balance of zero instead of stale booking.balanceAmount; preserve the existing booking fields and avoid relying on the pre-update booking row. Add an assertion in the final-confirmation job test verifying email.booking-final-confirmation carries a zero balance.apps/booking-web/app/[locale]/booking/[reference]/BookingConfirmationPageClient.tsx-129-142 (1)
129-142: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a cancellation guard and surface the failure to the guest.
Two problems in this effect:
- There is no
cancelledflag. The two sibling effects in this file use one (lines 102-126 and lines 148-159). Without it, a late response from a previousbooking.idcan callsetPaypalInfoafter the effect re-runs, and the panel shows PayPal details for the wrong booking..catch(() => {})discards every failure. If the request fails, or if the response omitspaypal,paypalInfostaysnulland the PayPal panel at line 504 never renders. A guest with a pending PayPal booking then has no payment address, no reference, and no error message. The booking expires with no way for the guest to act.🔧 Proposed fix
useEffect(() => { if (booking?.paymentMethod !== "paypal") return; + let cancelled = false; bookingService .getPaymentInstructions(slug, booking.id || booking.bookingReference) .then((instructions) => { - if (instructions.paypal.enabled && instructions.paypal.email) { + if (cancelled) return; + if (instructions?.paypal?.enabled && instructions.paypal.email) { setPaypalInfo({ email: instructions.paypal.email, windowHours: instructions.paypal.paymentWindowHours || 24, }); + } else { + setPaypalInfoError(true); } }) - .catch(() => {}); + .catch(() => { + if (!cancelled) setPaypalInfoError(true); + }); + return () => { + cancelled = true; + }; }, [booking?.id, booking?.bookingReference, booking?.paymentMethod, slug]);Add the
paypalInfoErrorstate and render a short recovery message next to the pending-payment block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/booking-web/app/`[locale]/booking/[reference]/BookingConfirmationPageClient.tsx around lines 129 - 142, Update the PayPal instructions effect around setPaypalInfo to use a cancelled guard, preventing stale responses from updating state after dependencies change or cleanup. Add paypalInfoError state, set it when the request fails or the response lacks usable paypal details, and clear it before a new request; render a concise recovery message alongside the pending-payment block when this error is present.apps/booking-web/lib/storage/bookingDraft.ts-119-128 (1)
119-128: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe checkout idempotency-key store does not durably guarantee replay protection. Both sites weaken the same guarantee: a retry of the same checkout operation can present a different key, and the server then treats the retry as a new booking. The key store needs entry validation, a reported persistence result, and a scoped reset.
apps/booking-web/lib/storage/bookingDraft.ts#L119-L128: validate that the stored entry has a non-empty stringkeybefore returning it, and propagate thesafeSetresult so the caller can react when the key was not persisted.apps/booking-web/lib/storage/bookingDraft.ts#L75-L78: reset only the bindings invalidated by the edited guest details instead of clearing the entirekeysmap, so an in-flightconfirmkey survives a details edit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/booking-web/lib/storage/bookingDraft.ts` around lines 119 - 128, The checkout idempotency-key store must validate entries, report persistence failure, and preserve unrelated keys. In apps/booking-web/lib/storage/bookingDraft.ts lines 119-128, update getCheckoutIdempotencyKey to reuse only entries whose key is a non-empty string and propagate the safeSet result so callers can detect failed persistence. In lines 75-78, reset only bindings invalidated by edited guest details, preserving unrelated in-flight confirm keys.apps/booking-web/services/api/client.ts-36-42 (1)
36-42: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winObject spread drops non-plain
headersvalues.
init?.headersis typedHeadersInit. That type allows aHeadersinstance and an array of tuples. Object spread copies no entries from either form, so those headers are silently discarded. A caller that passesnew Headers({ "Idempotency-Key": key })would then send the request without the key, and the server would lose replay protection with no error. Normalize throughHeadersinstead.🐛 Proposed fix
async post<T>(path: string, body?: unknown, init?: ApiRequestInit): Promise<T> { + const headers = new Headers({ "Content-Type": "application/json" }); + new Headers(init?.headers).forEach((value, key) => headers.set(key, value)); const res = await fetch(`${this.baseUrl}${path}`, { ...init, method: "POST", - headers: { "Content-Type": "application/json", ...init?.headers }, + headers, ...(body !== undefined && { body: JSON.stringify(body) }), }); return parse<T>(res); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/booking-web/services/api/client.ts` around lines 36 - 42, Update the headers construction in ApiClient.post to normalize init?.headers through the Headers API before merging them with the JSON Content-Type header. Preserve caller-provided HeadersInit values, including Headers instances and tuple arrays, while retaining the existing Content-Type behavior and request initialization.apps/booking-web/messages/fr.json-240-241 (1)
240-241: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPost-booking copy still describes a released card authorization. The checkout step now states that the card is charged immediately, but the confirmation section of each locale file still tells the guest that a hold was lifted. A charged guest therefore receives no refund information after a cancellation, a decline, or an expiry.
apps/booking-web/messages/fr.json#L240-L241: updatecancelledCardSubtitle,declinedCardSubtitle, andexpiredSubtitleat Lines 303, 306, and 308 to describe a refund of the charge instead of a released authorization.apps/booking-web/messages/id.json#L240-L241: update the same three keys at Lines 303, 306, and 308 to describe a refund of the charge.Check
en.jsonandes.jsonfor the equivalent keys as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/booking-web/messages/fr.json` around lines 240 - 241, Update the post-booking copy for cancelledCardSubtitle, declinedCardSubtitle, and expiredSubtitle in apps/booking-web/messages/fr.json and apps/booking-web/messages/id.json to describe refunding the immediate card charge rather than releasing an authorization; make the equivalent updates for these keys in en.json and es.json as well.apps/api/src/domains/stripeBookingPayments.ts-61-66 (1)
61-66: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle non-JSON Stripe responses.
await response.json()runs before theresponse.okcheck. If Stripe or an intermediate proxy returns HTML or an empty body on a 5xx or 429,json()throws a parse error. The HTTP status is then lost, and the caller cannot distinguish a retryable gateway failure from a Stripe validation error.🐛 Proposed fix
- const payload = object(await response.json()); + const payload = object(await response.json().catch(() => null)); if (!response.ok) { throw new Error( text(object(payload["error"])["message"]) ?? `Stripe failed (${response.status}).`, ); } + if (!Object.keys(payload).length) { + throw new Error(`Stripe returned an unreadable response (${response.status}).`); + } return payload;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/stripeBookingPayments.ts` around lines 61 - 66, Update the response handling around the Stripe request so non-OK responses are processed before requiring JSON parsing, preserving the HTTP status for HTML or empty 5xx/429 responses. In the non-OK branch, attempt to extract the Stripe error message when JSON is available, but fall back to the existing status-based error message when parsing fails; retain the current payload handling for successful responses.apps/api/src/domains/stripeBookingPayments.ts-49-60 (1)
49-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a timeout and pin the Stripe API version.
Two hardening gaps exist in
request:
- The call uses
fetchImplwithout anAbortSignal. If Stripe stalls, the checkout request thread waits without bound. Add a timeout.- No
Stripe-Versionheader is sent. Stripe then uses the account default version. A dashboard change can alter response shapes without a code change.🛡️ Proposed hardening
export function createStripeBookingPaymentProvider(config: { secretKey: string; endpoint?: string; fetch?: StripeFetch; + apiVersion?: string; + timeoutMs?: number; }): StripeBookingPaymentProvider { const endpoint = config.endpoint ?? "https://api.stripe.com/v1"; const fetchImpl = config.fetch ?? globalThis.fetch; + const apiVersion = config.apiVersion ?? "2024-06-20"; + const timeoutMs = config.timeoutMs ?? 15_000; @@ const response = await fetchImpl( `${endpoint}${path}${method === "GET" && fields.length ? `?${form}` : ""}`, { method, + signal: AbortSignal.timeout(timeoutMs), headers: { Authorization: `Basic ${Buffer.from(`${config.secretKey}:`).toString("base64")}`, + "Stripe-Version": apiVersion, ...(method === "POST" ? { "Content-Type": "application/x-www-form-urlencoded" } : {}),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/stripeBookingPayments.ts` around lines 49 - 60, Update the request function’s fetchImpl call to enforce a finite timeout using an AbortSignal, and include an explicit Stripe-Version header with the application’s pinned API version. Ensure the timeout signal is passed in the request options and the existing authorization, method, body, and idempotency behavior remains unchanged.apps/api/src/jobs/bookingEmailDelivery.ts-33-53 (1)
33-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a timeout to the Resend request.
requesthas no timeout. If the provider stalls, the promise never settles. The worker loop then blocks, and the claimed job stays inrunninguntil the 5-minute stale reclaim inclaimBookingEmailJobreleases it. Each stalled attempt also consumes oneattempts_countwithout recording a terminal state.🛡️ Proposed fix
const response = await request("https://api.resend.com/emails", { method: "POST", + signal: AbortSignal.timeout(10_000), headers: {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/jobs/bookingEmailDelivery.ts` around lines 33 - 53, Add an AbortController-based timeout to the request in the send method, aborting stalled Resend calls after the configured timeout and passing its signal to fetch. Ensure the timeout is cleared when the request settles while preserving existing response error handling.apps/booking-web/app/[locale]/payment/PaymentPageClient.tsx-326-334 (1)
326-334: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
handleSubmitreplays a stale recovery instead of the current selection.
submitBookingdefaultsrecoverytopendingCreateRecovery.handleSubmitcallssubmitBooking()with no argument, so the default applies.Consider this sequence. A recovery replay fails with an ambiguous error. Line 470 keeps
pendingCreateRecoveryset. The guest then changes the payment method and presses the submit button.handleSubmitruns, the default supplies the stale recovery, and the code sends the originalrequestBodyand the originalcreateIdempotencyKey. The new payment method is discarded.The same default also bypasses the terms check at line 328 for that click.
Make the explicit user action always start a new attempt.
🐛 Proposed fix
- const submitBooking = async (recovery = pendingCreateRecovery) => { + const submitBooking = async ( + recovery: typeof pendingCreateRecovery = null, + ) => {Then pass the recovery explicitly at the two recovery call sites (line 517 and line 536). Both already do so.
Also applies to: 510-512
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/booking-web/app/`[locale]/payment/PaymentPageClient.tsx around lines 326 - 334, Make explicit user submission start a fresh attempt by changing handleSubmit to call submitBooking with recovery explicitly unset, preventing the default pendingCreateRecovery from being used and ensuring current payment details and terms validation apply. Preserve explicit recovery arguments at the existing recovery call sites around the recovery replay handlers.apps/booking-web/app/[locale]/payment/PaymentPageClient.tsx-425-440 (1)
425-440: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe card flow triggers a duplicate create call.
Line 436 calls
setPendingCreateRecovery(cardAttempt)on the normal (non-recovery) card path. At that momentresumedCreateAttempt.currentis stillnull, because the normal path never sets it.The effect at lines 514-522 then runs with a new
pendingCreateRecoveryvalue, passes the!resumedCreateAttempt.currentguard, and callssubmitBooking(cardAttempt)again. That issues a secondbookingService.createrequest while the Stripe form is mounting. It also re-runssetSubmitting(true),setPendingBooking, andsetClientSecret, which remountsStripeProviderwith the sameclientSecret.The shared idempotency key limits the damage on the server, but the extra request and the Elements remount are unnecessary and can interrupt the guest.
Set the ref when you store the card attempt.
🐛 Proposed fix
savePendingBookingCreate(cardAttempt); + resumedCreateAttempt.current = createIdempotencyKey; setPendingCreateRecovery(cardAttempt);Also applies to: 514-522
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/booking-web/app/`[locale]/payment/PaymentPageClient.tsx around lines 425 - 440, Update the normal card-flow handling around savePendingBookingCreate and setPendingCreateRecovery so resumedCreateAttempt.current is assigned to cardAttempt before storing the recovery state. Preserve the existing recovery effect and submitBooking behavior, ensuring the effect’s guard prevents a second create request and StripeProvider remount.apps/api/src/platform/providerWebhooks.ts-253-268 (1)
253-268: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThe payload fallback marks an account active without card-capability evidence.
When
stripeConnectProvideris not configured,canonicalis built from the webhook payload. If the payload omitscardPaymentsStatus, line 263 sets it tonull. Line 268 then treatsnullas ready.$7becomestrue, so line 272 setsstatus = 'active'wheneverchargesEnabled,payoutsEnabled, anddetailsSubmittedare true.
apps/api/src/platform/publicBookabilityPublication.ts(lines 489-497 and 502-512) then derivesonline_payment_readyand thecardpublic payment method frompayment_provider_status = 'active'pluscharges_enabled. A property can therefore publish card checkout although the Stripecard_paymentscapability was never confirmed.The fallback also has no event-ordering guard. Line 290 writes
lastStripeEventIdunconditionally, so an out-of-order payload overwrites newer state.Make the unknown capability fail closed.
🛡️ Proposed fix
- const cardPaymentsReady = - canonical.cardPaymentsStatus === null || canonical.cardPaymentsStatus === "active"; + // Only trust a null capability when it came from the canonical Connect account. + const cardPaymentsReady = stripeConnectProvider + ? canonical.cardPaymentsStatus === null || canonical.cardPaymentsStatus === "active" + : canonical.cardPaymentsStatus === "active";Also applies to: 283-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/platform/providerWebhooks.ts` around lines 253 - 268, Update the webhook handling flow around the canonical payload fallback and the unconditionally persisted event in the relevant webhook handler: treat a missing or unknown cardPaymentsStatus as not ready rather than allowing null to satisfy cardPaymentsReady, so active status requires explicit card-capability evidence. Add event-ordering protection before updating lastStripeEventId, preventing an out-of-order payload from overwriting newer provider state.apps/booking-web/app/[locale]/payment/PaymentPageClient.tsx-151-164 (1)
151-164: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExpire stale pending booking-create recovery records.
readPendingBookingCreatevalidates the stored shape but does not enforce a maximum age. The replay effect therefore reuses an expired quote and idempotency key after reload. Add a bounded recovery age and clear records that exceed it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/booking-web/app/`[locale]/payment/PaymentPageClient.tsx around lines 151 - 164, Update the recovery effect in PaymentPageClient around readPendingBookingCreate to enforce a bounded record age before restoring payment state. When the stored recovery exceeds the allowed age, clear it using the existing pending-booking recovery cleanup mechanism and return; preserve the current validation and state restoration for valid, non-expired records.apps/api/src/domains/stripeBookingSettlement.ts-125-180 (1)
125-180: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate the PMS create job on a confirmed booking transition.
When the guarded booking update affects zero rows, the function still inserts
pms.reservation.createwith"paymentStatus": "paid". Insert the job only whenupdated.rows.length > 0, or make the consumer reject bookings that are notconfirmed. Add canceled and expired booking tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/stripeBookingSettlement.ts` around lines 125 - 180, Gate the PMS reservation-create insertion around the booking update result so the INSERT executes only when updated.rows.length > 0, preventing jobs for canceled or expired bookings. Update the surrounding settlement flow using the existing updated result, and add tests covering canceled and expired bookings.apps/api/src/platform/providerWebhooks.ts-163-164 (1)
163-164: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHandle non-retryable settlement mismatches without poisoning provider receipts.
settleStripeBookingPaymentthrows on an amount or currency mismatch.promoteReceiptrolls back and rethrows. No provider-webhook path updatesdelivery_statustofailedordead_lettered, or tracks retry attempts. The domain event, job, idempotency rows, and receipt promotion therefore roll back on every retry. Add bounded retry handling. Record non-retryable mismatches for manual reconciliation and commit a terminal receipt outcome instead of rethrowing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/platform/providerWebhooks.ts` around lines 163 - 164, Update the provider webhook settlement flow around settleCapturedStripeBooking and reconcileStripeProviderAccount to distinguish non-retryable amount/currency mismatches from transient failures. Track bounded retry attempts, persist failed or dead_lettered delivery status and the mismatch details for manual reconciliation, and commit a terminal receipt outcome without rethrowing so domain events, jobs, idempotency rows, and receipt promotion are not rolled back; continue propagating retryable failures.apps/api/src/domains/stripeMoney.ts-1-19 (1)
1-19: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHandle UGX and ISK Stripe charge amounts.
stripeAmountMinorreturns5for5.00 UGX, but Stripe requires500. AddUGXandISKspecial-case handling and regression tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/stripeMoney.ts` around lines 1 - 19, Update stripeAmountMinor in the stripe money conversion logic to handle Stripe’s UGX and ISK charge-amount conventions, ensuring 5.00 UGX produces 500 while preserving existing currency behavior. Add regression tests covering both UGX and ISK conversions, including the expected minor-unit values.Source: Linters/SAST tools
12fab1f to
ee3cb13
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/api/src/routes/bookingWebPublicParity.test.ts (2)
941-985: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name states replay, but the assertions show three adapter calls.
The stub derives
quoteIdfromcontext.idempotencyKey, so identical responses forquote-attempt-afollow from the stub, not from any replay logic. Line 983 asserts threequoteBookinginvocations, which proves the route forwards the key and does not deduplicate. Rename the test to describe key propagation, or assert real replay behavior against an adapter that records completed responses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/bookingWebPublicParity.test.ts` around lines 941 - 985, Update the test around the quoteBooking stub and concurrent app.inject calls so it verifies the intended behavior rather than claiming replay: either rename the test and adjust assertions to describe idempotency-key propagation with three adapter calls, or implement a real replay scenario that records and reuses the completed response for duplicate quote-attempt-a requests. Ensure the assertions match the selected behavior and no longer imply deduplication while expecting three invocations.
1289-1292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
paypalConfigured = falsedoes not affect the asserted PayPal instructions.
getPaymentInstructionsresolves PayPal state frombooking.bookingMetadata.paymentInstructionsand the booking lifecycle, not fromhotel_catalog.propertiesaccepted methods. The mutation at Line 1385 therefore changes no input for the assertion at Lines 1386-1394. Either drop the mutation or assert the intended contract explicitly, for example that stored instructions stay valid after the property removes PayPal.Also update the test title. It now covers terms text, accepted methods, bank transfer, PayPal, and the payment-window value, not only
phoneRequired.Also applies to: 1375-1401
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/bookingWebPublicParity.test.ts` around lines 1289 - 1292, Update the test around the “exposes target checkout phone required settings” case so the PayPal assertion uses an input that getPaymentInstructions actually reads, or remove the ineffective paypalConfigured mutation; if the intended contract is persistence, explicitly assert stored PayPal instructions remain valid after accepted methods change. Rename the test to describe all covered checkout payment settings, including terms, accepted methods, bank transfer, PayPal, phoneRequired, and payment-window behavior.packages/product-onboarding/src/SharedFirstRunPropertySetupWizard.tsx (2)
3012-3061: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink the helper text to the group for assistive technology.
MultiChoiceFieldrenders the helper text as a plain paragraph. Thelegendmarks the "Optional" hint witharia-hidden="true", so assistive technology announces neither the hint nor the helper text. Add anidto the helper paragraph and reference it witharia-describedbyon thefieldset.♿ Proposed change
-}) { +}) { + const helperId = useId(); return ( - <fieldset> + <fieldset aria-describedby={helperId}> <legend className="text-sm font-medium text-gray-700"> {label} <span aria-hidden="true" className="ml-2 text-xs text-gray-400"> Optional </span> </legend> - <p className="mt-1 text-xs text-gray-500">{helper}</p> + <p id={helperId} className="mt-1 text-xs text-gray-500"> + {helper} + </p>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/product-onboarding/src/SharedFirstRunPropertySetupWizard.tsx` around lines 3012 - 3061, Update MultiChoiceField so the helper paragraph has a unique id and the surrounding fieldset references that id via aria-describedby, ensuring assistive technology announces the helper text for the checkbox group.
1595-1607: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
contactStepinstead of the literal3.The skip button navigates with
changeStep(3). The step index of the contact step is already derived ascontactStepon Line 1138. The literal duplicates that derivation and will drift if a step is added.♻️ Proposed change
onClick={() => { onSkipLaunchSettings(); - changeStep(3); + changeStep(contactStep); }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/product-onboarding/src/SharedFirstRunPropertySetupWizard.tsx` around lines 1595 - 1607, Replace the literal step index in the skip button’s onClick handler with the existing contactStep value derived near the wizard’s step configuration. Update the changeStep call while preserving the onSkipLaunchSettings invocation and all other button behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/marketplace-web/services/api/hotelOperationsSetupClient.ts`:
- Around line 427-446: Update the gallery assignments in the flow using
createHotelCatalogStep1MediaAssignments so their sortOrder values are renumbered
after the new cover is prepended at sortOrder 0. Preserve each gallery
assignment’s ordering while offsetting its sortOrder by one before passing
assignments to replacePropertyPresentationMedia.
In `@tests/e2e/marketplace-web/setup-activation.spec.ts`:
- Around line 663-703: In tests/e2e/marketplace-web/setup-activation.spec.ts
lines 663-703, wait for the POST
/api/finance/properties/{propertyId}/select-commission request to complete
before asserting commissionSelection, using the request result as the
synchronization point. In the same file lines 712-781, likewise wait for the
PATCH /api/finance/properties/{propertyId}/payment-settings request before
asserting paymentWrite; update both captured values so TypeScript recognizes
them as populated after the awaited request.
---
Nitpick comments:
In `@apps/api/src/routes/bookingWebPublicParity.test.ts`:
- Around line 941-985: Update the test around the quoteBooking stub and
concurrent app.inject calls so it verifies the intended behavior rather than
claiming replay: either rename the test and adjust assertions to describe
idempotency-key propagation with three adapter calls, or implement a real replay
scenario that records and reuses the completed response for duplicate
quote-attempt-a requests. Ensure the assertions match the selected behavior and
no longer imply deduplication while expecting three invocations.
- Around line 1289-1292: Update the test around the “exposes target checkout
phone required settings” case so the PayPal assertion uses an input that
getPaymentInstructions actually reads, or remove the ineffective
paypalConfigured mutation; if the intended contract is persistence, explicitly
assert stored PayPal instructions remain valid after accepted methods change.
Rename the test to describe all covered checkout payment settings, including
terms, accepted methods, bank transfer, PayPal, phoneRequired, and
payment-window behavior.
In `@packages/product-onboarding/src/SharedFirstRunPropertySetupWizard.tsx`:
- Around line 3012-3061: Update MultiChoiceField so the helper paragraph has a
unique id and the surrounding fieldset references that id via aria-describedby,
ensuring assistive technology announces the helper text for the checkbox group.
- Around line 1595-1607: Replace the literal step index in the skip button’s
onClick handler with the existing contactStep value derived near the wizard’s
step configuration. Update the changeStep call while preserving the
onSkipLaunchSettings invocation and all other button behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 737286a9-4932-4b55-807c-27aa67149889
📒 Files selected for processing (18)
apps/api/src/app.test.tsapps/api/src/app.tsapps/api/src/domains/pmsOperationsReadModel.test.tsapps/api/src/domains/pmsOperationsReadModel.tsapps/api/src/routes/bookingWebPublicParity.test.tsapps/api/src/routes/pmsOperations.tsapps/api/src/server.tsapps/booking-admin/app/(app)/settings/page.tsxapps/marketplace-web/services/api/hotelOperationsSetupClient.test.tsapps/marketplace-web/services/api/hotelOperationsSetupClient.tsapps/pms-web/app/(app)/bookings/[id]/page.tsxapps/pms-web/services/bookings/index.test.tsapps/pms-web/services/bookings/index.tspackages/domain-finance/src/index.test.tspackages/domain-finance/src/index.tspackages/product-onboarding/src/SharedFirstRunPropertySetupWizard.tsxpackages/product-onboarding/src/index.tstests/e2e/marketplace-web/setup-activation.spec.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/api/src/app.ts
- apps/api/src/domains/pmsOperationsReadModel.test.ts
- apps/api/src/routes/pmsOperations.ts
- apps/pms-web/services/bookings/index.test.ts
- apps/api/src/app.test.ts
- apps/api/src/server.ts
- apps/pms-web/app/(app)/bookings/[id]/page.tsx
- apps/pms-web/services/bookings/index.ts
- packages/domain-finance/src/index.ts
- apps/booking-admin/app/(app)/settings/page.tsx
- apps/api/src/domains/pmsOperationsReadModel.ts
- packages/domain-finance/src/index.test.ts
cd9ecd2 to
8263c08
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/routes/finance.ts (2)
7440-7458: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the unreachable duplicate
paypalcase.Line 7448 adds
case "paypal":to the pass-through group, so the switch returns"paypal". The secondcase "paypal": return "wallet";at lines 7452-7453 can never run. Leaving both makes the mapping ambiguous to a reader.♻️ Proposed cleanup
case "manual_card": case "wallet": return method; - case "paypal": - return "wallet"; default: return "other";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/finance.ts` around lines 7440 - 7458, Remove the second, unreachable case "paypal" branch from paymentMethods; retain the existing pass-through case so "paypal" continues returning "paypal".
2719-2858: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftSerialize manual payments per booking.
validateManualPaymentInvoicereadsbooking.balance_amountbeforeinsertManualPaymentadds a payment. Concurrent requests with different idempotency keys can both pass validation and insert payments that exceed the balance. The payment table has no overpayment constraint, and the manual-payment insert does not updatebalance_amount.Add a property-and-invoice scoped lock before
loadInvoiceRows. Production usespg.Pool.connect()and starts a transaction, sopg_advisory_xact_lockcan protect this path. Handle direct non-transaction callers separately or require a transaction-capable client.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/finance.ts` around lines 2719 - 2858, Update recordManualPaymentInClient to acquire a property-and-invoice scoped PostgreSQL transaction advisory lock before loadInvoiceRows, using the transaction-capable client and pg_advisory_xact_lock so concurrent manual payments for the same invoice serialize through validation and insertion. Ensure the transaction is started and committed or rolled back by the production pg.Pool.connect() path, and explicitly handle or reject direct non-transaction callers instead of allowing the unlocked flow.
🧹 Nitpick comments (2)
apps/api/src/finance.test.ts (1)
1963-1976: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
property_currency_conflictbranch.
updatePaymentSettingsInClientreturns a 409property_currency_conflictwhendefaultCurrencyorsupportedCurrenciesdisagree with the canonical property currency (apps/api/src/routes/finance.tslines 1643-1655). This new branch has no coverage. It is user-facing, because it forces a reload before saving.Add one case that posts a stale currency against a property whose canonical currency differs, and assert the 409 code plus the absence of an
INSERT INTO finance.payment_settingscall.💚 Suggested test to add after the currency fallback test
it("rejects a stale payment currency against the canonical property currency", async () => { const target = targetPaymentSettingsPool({ canonicalCurrency: "IDR" }); const repository = createTargetFinancePropertySettingsRepository({ connectionString: "postgresql://finance-target", pool: target.pool, }); await expect( repository.updatePaymentSettings!( paymentSettingsTargetCommand({ payload: { paymentsEnabled: true, acceptedMethods: ["card"], defaultCurrency: "EUR", }, }), ), ).resolves.toMatchObject({ ok: false, statusCode: 409, code: "property_currency_conflict", }); expect( target.calls.some((call) => call.text.includes("INSERT INTO finance.payment_settings")), ).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/finance.test.ts` around lines 1963 - 1976, Add a test covering the property_currency_conflict branch in updatePaymentSettings, using targetPaymentSettingsPool with a canonical currency that differs from the submitted defaultCurrency. Assert the result is unsuccessful with statusCode 409 and code property_currency_conflict, and verify target.calls contains no INSERT INTO finance.payment_settings statement.apps/api/src/routes/finance.ts (1)
584-585: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate the bookability publish from the request result.
options.publicBookabilityPublisher?.publishruns afterupdatePaymentSettingsalready committed. A rejection frompublishpropagates out of the handler, so the operator receives a 500 for a save that succeeded. The call also has no timeout.Catch the failure and log it, or enqueue the publication as a job so the write result stays authoritative.
♻️ Proposed guard
- await options.publicBookabilityPublisher?.publish({ propertyId }); + try { + await options.publicBookabilityPublisher?.publish({ propertyId }); + } catch (error) { + request.log.error( + { err: error, propertyId }, + "Public bookability publication failed after payment-settings update.", + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/finance.ts` around lines 584 - 585, Update the post-commit publication call in the payment-settings handler around updatePaymentSettings so publicBookabilityPublisher.publish cannot reject the request after the save succeeds. Catch and log publication failures, and add a bounded timeout for the publish operation, or enqueue it as a background job while preserving the successful write response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/domains/financeSubscriptionStore.ts`:
- Around line 82-127: Update the cleanup in withPlanMutationLock so
client.release(true) is used when the pg_advisory_unlock query fails, while
retaining normal client.release() after a successful unlock. Preserve the
existing nested finally structure and ensure every client is released or
destroyed.
In `@apps/api/src/routes/finance.ts`:
- Around line 1790-1822: Update paymentSettingsCompletenessError so completeness
validation runs only when the merged settings have paymentsEnabled set to true;
allow an explicitly disabled payment configuration to return null without
requiring acceptedMethods or other fields. Preserve the existing validation
rules for enabled payments, and ensure unrelated partial PATCH updates do not
fail solely because the stored enabled configuration remains incomplete.
- Around line 6844-6854: Update manualPaymentCommission to detect an empty or
missing bookingEngineFeePercent in commissionTermsSnapshot and return the
route’s typed invalid_command outcome before insertManualPayment can fail;
preserve rejection of malformed terms and do not apply a default commission
rate. Ensure booking creation also rejects commission billing when Finance-owned
commission terms are unavailable rather than persisting an empty snapshot.
In `@tests/e2e/marketplace-web/setup-activation.spec.ts`:
- Around line 717-755: Update the CORS headers used by the payment-settings
route around paymentSettingsRequest to include PATCH in
access-control-allow-methods, while preserving the existing GET, POST, PUT, and
OPTIONS methods.
---
Outside diff comments:
In `@apps/api/src/routes/finance.ts`:
- Around line 7440-7458: Remove the second, unreachable case "paypal" branch
from paymentMethods; retain the existing pass-through case so "paypal" continues
returning "paypal".
- Around line 2719-2858: Update recordManualPaymentInClient to acquire a
property-and-invoice scoped PostgreSQL transaction advisory lock before
loadInvoiceRows, using the transaction-capable client and pg_advisory_xact_lock
so concurrent manual payments for the same invoice serialize through validation
and insertion. Ensure the transaction is started and committed or rolled back by
the production pg.Pool.connect() path, and explicitly handle or reject direct
non-transaction callers instead of allowing the unlocked flow.
---
Nitpick comments:
In `@apps/api/src/finance.test.ts`:
- Around line 1963-1976: Add a test covering the property_currency_conflict
branch in updatePaymentSettings, using targetPaymentSettingsPool with a
canonical currency that differs from the submitted defaultCurrency. Assert the
result is unsuccessful with statusCode 409 and code property_currency_conflict,
and verify target.calls contains no INSERT INTO finance.payment_settings
statement.
In `@apps/api/src/routes/finance.ts`:
- Around line 584-585: Update the post-commit publication call in the
payment-settings handler around updatePaymentSettings so
publicBookabilityPublisher.publish cannot reject the request after the save
succeeds. Catch and log publication failures, and add a bounded timeout for the
publish operation, or enqueue it as a background job while preserving the
successful write response.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27d764e0-59d1-4003-bb8f-90c3714a9f8e
📒 Files selected for processing (19)
apps/api/src/app.test.tsapps/api/src/app.tsapps/api/src/domains/financeSubscriptionService.test.tsapps/api/src/domains/financeSubscriptionService.tsapps/api/src/domains/financeSubscriptionStore.test.tsapps/api/src/domains/financeSubscriptionStore.tsapps/api/src/domains/pmsOperationsCommandRepository.test.tsapps/api/src/domains/pmsOperationsReadModel.test.tsapps/api/src/domains/pmsOperationsReadModel.tsapps/api/src/finance.test.tsapps/api/src/jobs/bookingLifecycle.test.tsapps/api/src/jobs/bookingLifecycle.tsapps/api/src/routes/finance.tsapps/api/src/server.tsapps/marketplace-web/services/api/hotelOperationsSetupClient.test.tsapps/marketplace-web/services/api/hotelOperationsSetupClient.tsapps/pms-web/services/bookings/index.test.tsapps/pms-web/services/bookings/index.tstests/e2e/marketplace-web/setup-activation.spec.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/api/src/app.ts
- apps/pms-web/services/bookings/index.test.ts
- apps/api/src/domains/pmsOperationsReadModel.test.ts
- apps/api/src/app.test.ts
- apps/api/src/domains/financeSubscriptionService.test.ts
- apps/api/src/domains/pmsOperationsCommandRepository.test.ts
- apps/api/src/jobs/bookingLifecycle.test.ts
- apps/pms-web/services/bookings/index.ts
- apps/api/src/domains/pmsOperationsReadModel.ts
- apps/api/src/server.ts
- apps/api/src/domains/financeSubscriptionService.ts
- apps/api/src/jobs/bookingLifecycle.ts
- apps/marketplace-web/services/api/hotelOperationsSetupClient.ts
- apps/marketplace-web/services/api/hotelOperationsSetupClient.test.ts
8263c08 to
0d51655
Compare
|
Review follow-up completed on the rebased head 0d51655: serialized Finance settlement, fail-closed billing snapshots, safe advisory-lock cleanup, CORS PATCH support, and preservation of the newly deleted obsolete Financials routes. I intentionally kept publication failures visible: the mutation is idempotent and same-key retries re-run publication, while swallowing the error would silently leave Distribution stale. Verification: API typecheck plus 226 focused tests passed locally. |
0d51655 to
163b22a
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
apps/api/src/domains/financeManualPaymentSettlement.ts (1)
214-238: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRestrict
paymentMethodto manual settlement methods.
validateSettlementaccepts anyFinanceRoutePaymentMethod. The insert then writespayment_kind = 'manual'andstatus = 'paid'unconditionally. A provider-settled method such ascardwould be recorded as a paid manual payment without provider evidence. The current caller passespaypal,bank_transfer, orpay_at_property, so this is a defense-in-depth check at the Finance boundary rather than an active defect.Add an allow-list check in
validateSettlement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/financeManualPaymentSettlement.ts` around lines 214 - 238, Update validateSettlement to reject payment methods outside the manual settlement allow-list of paypal, bank_transfer, and pay_at_property before proceeding with amount validation. Preserve the existing invalidCommand pattern and allow the current caller-supported methods while preventing provider-settled methods such as card from reaching the unconditional manual payment insert.apps/api/src/domains/financeManualPaymentSettlement.test.ts (1)
13-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the replay status and for fee rounding.
The suite covers the created path and one fail-closed path. Two behaviors of
recordBookingManualPaymentInClientstay untested:
status: "idempotent_replay"when the insert conflicts and theUNION ALLbranch returnsreplay: true.- Half-cent rounding in
commissionFeeCents, which rounds half up. A case such as amount"250.05"withbookingEngineFeePercent: 5(12.5025 →"12.50") or an amount that lands exactly on.005would lock this behavior.Both cases reuse the existing client double, so the cost is small.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/financeManualPaymentSettlement.test.ts` around lines 13 - 66, Extend the tests for recordBookingManualPaymentInClient to cover an INSERT conflict returning replay: true and assert status "idempotent_replay". Add a commission-fee rounding case using an amount such as "250.05" with a 5% fee, and assert the half-cent result follows the expected half-up rounding in feeAmount and netAmount.apps/api/src/routes/bookingWebPublic.ts (1)
3224-3238: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNarrow the row lock to the payment row.
FOR UPDATEwithout a target list locks the joinedfinance.payment_provider_accountsrow as well. That row is shared by every booking of the property, so concurrent card confirmations for the same property serialize on it. UseFOR UPDATE OF payment.🛠️ Proposed lock scope change
ORDER BY payment.created_at DESC LIMIT 1 - FOR UPDATE`, + FOR UPDATE OF payment`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/bookingWebPublic.ts` around lines 3224 - 3238, Update the payment query in the TargetCardPaymentRow flow to use a targeted row lock with FOR UPDATE OF payment, ensuring only the finance.payments row is locked while preserving the existing join, ordering, and limit behavior.apps/api/src/domains/pmsOperationsCommandRepository.ts (1)
4167-4172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMap known Finance rejection codes to an operational transition error.
recordBookingManualPaymentInClientreturns typed failures such asinvoice_not_foundandinvalid_command. This code converts every failure into a thrownError, so the PMS operator receives a 500 for a state that is expected and actionable. The throw does preserve rollback, but the response class is wrong.Return
invalidStatusTransition(or a dedicated 409 result) for the known codes, and keep the throw for unexpected codes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/domains/pmsOperationsCommandRepository.ts` around lines 4167 - 4172, Update the error handling around recordBookingManualPaymentInClient in the PMS booking payment flow: map known rejection codes such as invoice_not_found and invalid_command to invalidStatusTransition (or the established dedicated 409 result), while preserving rollback behavior. Continue throwing an Error only for unexpected Finance failure codes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/domains/financeManualPaymentSettlement.ts`:
- Around line 224-226: Update the currency comparison in the manual payment
settlement validation to normalize both command.payload.currency and
booking.currency to the same case before comparing them, while preserving the
existing invalidCommand response for genuinely mismatched currencies.
- Around line 188-208: The finance settlement query already subtracts recorded
payments via payment_totals, but the PMS command must use the effective
remaining balance rather than raw booking.balanceAmount. Update the command’s
payment amount source to the balanceDue value returned by this query, preserving
the existing payment-ledger subtraction and full-settlement behavior.
In `@apps/api/src/domains/pmsOperationsCommandRepository.ts`:
- Around line 4292-4299: Update deadlinePassed to accept a required flag and
treat missing or unparsable deadlines as expired when required, while preserving
the current non-required behavior. Update the
applyBookingMarkPaidCommandMutation mark-paid call to pass isPayPalPending ||
isAcceptedBankTransfer as required so both deadline-bearing paths enforce the
payment window.
In `@apps/api/src/routes/bookingWebPublic.ts`:
- Line 3074: Rewrite the 503 message in the booking checkout flow around
createHttpError so it speaks in the property’s first-person voice, using “we,”
“our,” or “us” instead of referring to “This property.” Preserve the existing
503 status and Stripe Connect setup context.
- Around line 2088-2110: Update the payment-method filter around
targetCheckoutSupportedPaymentMethods so "pay_at_property" is included only when
row.acceptedMethods contains "cash" or "manual_card", matching
assertTargetCheckoutConfigMatchesQuote. Keep the existing checks for card,
bank_transfer, and paypal unchanged so payAtPropertyEnabled and
acceptedPaymentMethods reflect methods booking creation accepts.
- Around line 3091-3118: Update createStripeBookingPaymentProvider and the
createBooking/withTargetCheckoutTransaction flow so Stripe PaymentIntent
creation is bounded by a timeout and supports cancellation, without holding the
quote FOR UPDATE lock during the provider request. Preserve
paymentIdempotencyKey reuse and ensure failures or transaction rollbacks
reconcile the created PaymentIntent before allowing checkout state transitions
to continue.
- Around line 1868-1876: Update the booking-scoped payment response near the
paypalEnabled calculation to also derive persisted bank-transfer details from
the booking metadata and return them instead of hardcoded enabled: false and
details: null. Reuse the existing metadata parsing and bank-transfer field
names, while preserving the current PayPal behavior and pending-payment
eligibility checks.
- Around line 1980-1981: The migration or publication-upsert flow must backfill
existing rows in distribution.public_hotel_bookability_profiles by populating
capabilities.paymentMethods and default_currency before enforcing the new
fail-closed predicates. Update the relevant migration/backfill logic, preserving
the publication upsert behavior for future writes and ensuring existing profiles
become eligible for checkout and quote loading.
---
Nitpick comments:
In `@apps/api/src/domains/financeManualPaymentSettlement.test.ts`:
- Around line 13-66: Extend the tests for recordBookingManualPaymentInClient to
cover an INSERT conflict returning replay: true and assert status
"idempotent_replay". Add a commission-fee rounding case using an amount such as
"250.05" with a 5% fee, and assert the half-cent result follows the expected
half-up rounding in feeAmount and netAmount.
In `@apps/api/src/domains/financeManualPaymentSettlement.ts`:
- Around line 214-238: Update validateSettlement to reject payment methods
outside the manual settlement allow-list of paypal, bank_transfer, and
pay_at_property before proceeding with amount validation. Preserve the existing
invalidCommand pattern and allow the current caller-supported methods while
preventing provider-settled methods such as card from reaching the unconditional
manual payment insert.
In `@apps/api/src/domains/pmsOperationsCommandRepository.ts`:
- Around line 4167-4172: Update the error handling around
recordBookingManualPaymentInClient in the PMS booking payment flow: map known
rejection codes such as invoice_not_found and invalid_command to
invalidStatusTransition (or the established dedicated 409 result), while
preserving rollback behavior. Continue throwing an Error only for unexpected
Finance failure codes.
In `@apps/api/src/routes/bookingWebPublic.ts`:
- Around line 3224-3238: Update the payment query in the TargetCardPaymentRow
flow to use a targeted row lock with FOR UPDATE OF payment, ensuring only the
finance.payments row is locked while preserving the existing join, ordering, and
limit behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e19b282d-b59b-4cb0-91d2-1611813281df
📒 Files selected for processing (15)
apps/api/src/domains/financeManualPaymentSettlement.test.tsapps/api/src/domains/financeManualPaymentSettlement.tsapps/api/src/domains/financeSubscriptionStore.test.tsapps/api/src/domains/financeSubscriptionStore.tsapps/api/src/domains/pmsOperationsCommandRepository.test.tsapps/api/src/domains/pmsOperationsCommandRepository.tsapps/api/src/domains/pmsOperationsReadModel.test.tsapps/api/src/domains/pmsOperationsReadModel.tsapps/api/src/finance.test.tsapps/api/src/routes/bookingWebPublic.tsapps/api/src/routes/bookingWebPublicParity.test.tsapps/api/src/routes/finance.tspackages/domain-finance/src/index.test.tspackages/domain-finance/src/index.tstests/e2e/marketplace-web/utils/cors.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/api/src/domains/pmsOperationsReadModel.test.ts
- packages/domain-finance/src/index.test.ts
- apps/api/src/domains/pmsOperationsReadModel.ts
- apps/api/src/domains/financeSubscriptionStore.test.ts
- apps/api/src/domains/pmsOperationsCommandRepository.test.ts
- packages/domain-finance/src/index.ts
- apps/api/src/finance.test.ts
- apps/api/src/routes/finance.ts
| GREATEST( | ||
| booking.balance_amount - COALESCE(payment_totals.amount_paid, 0), | ||
| 0 | ||
| )::text AS "balanceDue", | ||
| booking.lifecycle_status AS "lifecycleStatus", | ||
| booking.payment_status AS "paymentStatus", | ||
| booking.billing_plan_snapshot AS "billingPlanSnapshot", | ||
| booking.commission_terms_snapshot AS "commissionTermsSnapshot" | ||
| FROM booking.guest_bookings booking | ||
| LEFT JOIN LATERAL ( | ||
| SELECT COALESCE(sum(payment.amount - payment.refunded_amount), 0) AS amount_paid | ||
| FROM finance.payments payment | ||
| WHERE payment.property_id = booking.property_id | ||
| AND payment.guest_booking_id = booking.id | ||
| AND payment.status IN ('authorized', 'pending', 'paid', 'partially_refunded') | ||
| AND payment.visibility_class IN ('pms_finance', 'migration') | ||
| ) payment_totals ON TRUE | ||
| WHERE booking.property_id = $1::uuid | ||
| AND COALESCE(booking.booking_metadata ->> 'invoiceId', booking.id::text) = $2 | ||
| LIMIT 1 | ||
| FOR UPDATE OF booking`, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace balance_amount writes to determine whether it is already net of payments.
rg -n -C 4 'balance_amount\s*=' --glob 'apps/api/src/**/*.ts' --glob 'packages/backend-migration/migrations/*.sql'Repository: vayada-marketplace/vayada
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- financeManualPaymentSettlement.ts ---'
rg -n -C 8 'balance_amount|balanceDue|loadBookingSettlement|validateSettlement' apps/api/src/domains/financeManualPaymentSettlement.ts
printf '%s\n' '--- balance_amount references ---'
rg -n -C 3 'balance_amount|balanceAmount' apps/api/src apps/api/test packages/backend-migration/migrations 2>/dev/null | head -n 1200Repository: vayada-marketplace/vayada
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- non-test balance_amount occurrences ---'
rg -n 'balance_amount|INSERT INTO finance\.payments|UPDATE booking\.guest_bookings' apps/api/src --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts' | rg 'balance_amount|finance\.payments|UPDATE booking\.guest_bookings'
printf '%s\n' '--- booking schema and payment settlement paths ---'
sed -n '70,115p' packages/backend-migration/migrations/0005_booking_checkout.sql
sed -n '1,190p' apps/api/src/domains/stripeBookingSettlement.ts
sed -n '4120,4225p' apps/api/src/domains/pmsOperationsCommandRepository.ts
printf '%s\n' '--- manual settlement tests and payment schema ---'
rg -n -C 8 'financeManualPaymentSettlement|balanceDue|amount_paid|refunded_amount|INSERT INTO finance\.payments|CREATE TABLE.*payments' apps/api/src packages/backend-migration/migrations --glob '*.ts' --glob '*.sql' | head -n 1000Repository: vayada-marketplace/vayada
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- booking creation and card authorization ---'
sed -n '2835,2930p' apps/api/src/routes/bookingWebPublic.ts
sed -n '3095,3185p' apps/api/src/routes/bookingWebPublic.ts
printf '%s\n' '--- booking change balance update ---'
sed -n '3945,4020p' apps/api/src/routes/bookingWebPublic.ts
printf '%s\n' '--- other booking balance mutations ---'
sed -n '4025,4085p' apps/api/src/domains/pmsOperationsCommandRepository.ts
sed -n '600,650p' apps/api/src/jobs/bookingLifecycle.ts
printf '%s\n' '--- all balance assignment contexts ---'
rg -n -C 12 'balance_amount\s*=' apps/api/src --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts'Repository: vayada-marketplace/vayada
Length of output: 20505
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- manual settlement write and callers ---'
sed -n '50,180p' apps/api/src/domains/financeManualPaymentSettlement.ts
rg -n -C 8 'recordBookingManualPaymentInClient|FinanceBookingManualPaymentSettlementCommand' apps/api/src --glob '*.ts' --glob '!*.test.ts' --glob '!*.spec.ts'
printf '%s\n' '--- booking balance inputs ---'
rg -n -C 10 'balanceAmount|balance_amount' apps/api/src/routes/bookingWebPublic.ts | rg -B 10 -A 10 'INSERT INTO booking\.guest_bookings|balanceAmount|balance_amount' | head -n 500Repository: vayada-marketplace/vayada
Length of output: 33380
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- every repository caller ---'
rg -n -C 6 'recordBookingManualPaymentInClient|financeManualPaymentSettlement' . --glob '!node_modules' --glob '!dist' --glob '!build'
printf '%s\n' '--- deterministic partial-payment model ---'
python3 - <<'PY'
from decimal import Decimal
booking_balance = Decimal("600.00")
recorded_payment = Decimal("250.00")
effective_balance = max(booking_balance - recorded_payment, Decimal("0"))
pms_amount = booking_balance
print({
"booking_balance_amount": str(booking_balance),
"recorded_payment_amount": str(recorded_payment),
"computed_balanceDue": str(effective_balance),
"pms_command_amount": str(pms_amount),
"exceeds_balanceDue": pms_amount > effective_balance,
})
assert effective_balance == Decimal("350.00")
assert pms_amount > effective_balance
PYRepository: vayada-marketplace/vayada
Length of output: 7890
Keep the payment-ledger subtraction and fix the PMS amount source.
booking.balance_amount is not decremented when a manual payment is recorded. Only full settlement sets it to 0. Therefore, balanceDue must subtract recorded payments. After a partial payment, the PMS command still sends the raw booking.balanceAmount, so Finance rejects it with "Manual payment amount exceeds the booking balance." Pass the effective remaining balance instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/domains/financeManualPaymentSettlement.ts` around lines 188 -
208, The finance settlement query already subtracts recorded payments via
payment_totals, but the PMS command must use the effective remaining balance
rather than raw booking.balanceAmount. Update the command’s payment amount
source to the balanceDue value returned by this query, preserving the existing
payment-ledger subtraction and full-settlement behavior.
| if (command.payload.currency !== booking.currency.toUpperCase()) { | ||
| return invalidCommand("Manual payment currency must match the booking currency."); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize the case of both currency values.
The comparison uppercases only booking.currency. If a caller supplies a lowercase or mixed-case currency, the command is rejected with "Manual payment currency must match the booking currency." even though the currencies match.
🛠️ Proposed fix
- if (command.payload.currency !== booking.currency.toUpperCase()) {
+ if (command.payload.currency.trim().toUpperCase() !== booking.currency.trim().toUpperCase()) {
return invalidCommand("Manual payment currency must match the booking currency.");
}📝 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.
| if (command.payload.currency !== booking.currency.toUpperCase()) { | |
| return invalidCommand("Manual payment currency must match the booking currency."); | |
| } | |
| if (command.payload.currency.trim().toUpperCase() !== booking.currency.trim().toUpperCase()) { | |
| return invalidCommand("Manual payment currency must match the booking currency."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/domains/financeManualPaymentSettlement.ts` around lines 224 -
226, Update the currency comparison in the manual payment settlement validation
to normalize both command.payload.currency and booking.currency to the same case
before comparing them, while preserving the existing invalidCommand response for
genuinely mismatched currencies.
| function deadlinePassed(deadline: string | null, commandTime: string): boolean { | ||
| if (!deadline) return false; | ||
| const deadlineTime = Date.parse(deadline); | ||
| const acceptedTime = Date.parse(commandTime); | ||
| return ( | ||
| Number.isFinite(deadlineTime) && Number.isFinite(acceptedTime) && acceptedTime >= deadlineTime | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
deadlinePassed fails open for missing or unparsable deadlines.
The function returns false when deadline is null and when Date.parse yields NaN. applyBookingMarkPaidCommandMutation then accepts a payment for a PayPal or accepted bank-transfer booking whose pendingExpiresAt or acceptedPaymentDeadlineAt metadata is absent or malformed. Bookings created before this change carry no such metadata, so the payment window is not enforced for them.
Require a parsable deadline for the two deadline-bearing paths, and treat an unparsable value as expired.
🛠️ Proposed fix for deadline evaluation
-function deadlinePassed(deadline: string | null, commandTime: string): boolean {
- if (!deadline) return false;
- const deadlineTime = Date.parse(deadline);
- const acceptedTime = Date.parse(commandTime);
- return (
- Number.isFinite(deadlineTime) && Number.isFinite(acceptedTime) && acceptedTime >= deadlineTime
- );
-}
+function deadlinePassed(
+ deadline: string | null,
+ commandTime: string,
+ required = false,
+): boolean {
+ if (!deadline) return required;
+ const deadlineTime = Date.parse(deadline);
+ const acceptedTime = Date.parse(commandTime);
+ if (!Number.isFinite(deadlineTime) || !Number.isFinite(acceptedTime)) return true;
+ return acceptedTime >= deadlineTime;
+}Then pass required at the mark-paid call site:
deadlinePassed(paymentDeadline, acceptedAt, isPayPalPending || isAcceptedBankTransfer)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/domains/pmsOperationsCommandRepository.ts` around lines 4292 -
4299, Update deadlinePassed to accept a required flag and treat missing or
unparsable deadlines as expired when required, while preserving the current
non-required behavior. Update the applyBookingMarkPaidCommandMutation mark-paid
call to pass isPayPalPending || isAcceptedBankTransfer as required so both
deadline-bearing paths enforce the payment window.
| const booking = await loadTargetHotelBooking(pool, property.propertyId, handle); | ||
| const metadata = objectValue(booking.bookingMetadata); | ||
| const paymentInstructions = objectValue(metadata["paymentInstructions"]); | ||
| const paypalEmail = stringValue(paymentInstructions["paypalEmail"]); | ||
| const paypalEnabled = | ||
| booking.lifecycleStatus === "pending_payment" && | ||
| booking.paymentStatus === "unpaid" && | ||
| stringValue(metadata["paymentMethod"]) === "paypal" && | ||
| isValidPaymentEmail(paypalEmail); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every consumer and producer of bank-transfer payment instructions.
set -euo pipefail
rg -nP -C 6 'bankTransferDetails|bankTransfer\b|bankTransferInstructions' --type=ts --type=tsx appsRepository: vayada-marketplace/vayada
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant route sections ---'
sed -n '1825,1905p' apps/api/src/routes/bookingWebPublic.ts
sed -n '2740,2810p' apps/api/src/routes/bookingWebPublic.ts
printf '%s\n' '--- bank-transfer references in the API and booking packages ---'
rg -n -P -C 5 'bankTransferDetails|bankTransferInstructions|\bbankTransfer\b' apps packages -g '*.ts' -g '*.tsx' -g '*.js' -g '*.jsx'
printf '%s\n' '--- route and endpoint references ---'
rg -n -P -C 4 'bookingWebPublic|paymentInstructions|paymentMethod' apps/api/src -g '*.ts' -g '*.tsx'Repository: vayada-marketplace/vayada
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- payment-instruction API contract and callers ---'
rg -n -P -C 8 'getPaymentInstructions|payment_instructions|PaymentInstructions|bankTransferDetails' apps/booking-web apps/api/src -g '*.ts' -g '*.tsx'
printf '%s\n' '--- route registration and response types ---'
rg -n -P -C 10 'getPaymentInstructions|paymentInstructions' apps/api/src/routes apps/api/src -g '*.ts' | head -n 300
printf '%s\n' '--- email production and rendering ---'
sed -n '1,290p' apps/api/src/jobs/bookingEmails.ts
sed -n '4010,4125p' apps/api/src/domains/pmsOperationsCommandRepository.ts
printf '%s\n' '--- booking confirmation data loading ---'
rg -n -P -C 8 'BookingConfirmationPageClient|paymentMethod|payment_instructions|paymentInstructions' apps/booking-web/app apps/booking-web/services -g '*.ts' -g '*.tsx' | head -n 300Repository: vayada-marketplace/vayada
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
route = Path("apps/api/src/routes/bookingWebPublic.ts").read_text()
client = Path("apps/booking-web/services/api/booking.ts").read_text()
confirmation = Path(
"apps/booking-web/app/[locale]/booking/[reference]/BookingConfirmationPageClient.tsx"
).read_text()
pms = Path("apps/api/src/domains/pmsOperationsCommandRepository.ts").read_text()
emails = Path("apps/api/src/jobs/bookingEmails.ts").read_text()
checks = {
"endpoint_disables_bank_transfer": (
'bankTransfer: {\n enabled: false,\n details: null,' in route
),
"booking_creation_persists_bank_transfer_details": (
'quote.paymentMethod === "bank_transfer"' in route
and "bankTransferDetailsFromPolicy(depositPolicy)" in route
),
"client_contract_has_no_bank_transfer_shape": (
"getPaymentInstructions(" in client
and "paypal: { enabled: boolean; email: string | null; paymentWindowHours: number | null }"
in client
and "bankTransfer:" not in client[client.index("getPaymentInstructions("):client.index("getPaymentInstructions(") + 500]
),
"confirmation_calls_instructions_only_for_paypal": (
'if (booking?.paymentMethod !== "paypal") return;' in confirmation
),
"pms_acceptance_reads_stored_details": (
'jsonObject(booking.paymentInstructions)["bankTransferDetails"]' in pms
and "bankTransferDetails," in pms
),
"pending_payment_email_includes_details": (
'input.kind === "reserved_pending_payment" ? (input.bankTransferDetails ?? null)' in emails
and "bankTransferDetails" in emails
),
}
for name, result in checks.items():
print(f"{name}: {'PASS' if result else 'FAIL'}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: vayada-marketplace/vayada
Length of output: 445
Return persisted bank-transfer details from the booking-scoped endpoint. The pending-payment email includes these details, but the endpoint always returns enabled: false and details: null, so the confirmation page cannot retrieve them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/bookingWebPublic.ts` around lines 1868 - 1876, Update the
booking-scoped payment response near the paypalEnabled calculation to also
derive persisted bank-transfer details from the booking metadata and return them
instead of hardcoded enabled: false and details: null. Reuse the existing
metadata parsing and bank-transfer field names, while preserving the current
PayPal behavior and pending-payment eligibility checks.
| AND jsonb_typeof(profile.capabilities -> 'paymentMethods') = 'array' | ||
| AND jsonb_array_length(profile.capabilities -> 'paymentMethods') > 0` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace the paymentMethods capability and default_currency through publication and migrations.
set -euo pipefail
rg -nP -C 6 "paymentMethods" --type=ts apps/api/src/platform packages/domain-distribution/src
rg -nP -C 4 'default_currency|capabilities' --type=sql packages/backend-migration/migrations | rg -n -C 4 'public_hotel_bookability_profiles'Repository: vayada-marketplace/vayada
Length of output: 4675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- publication implementation ---'
sed -n '560,735p' apps/api/src/platform/publicBookabilityPublication.ts
printf '%s\n' '--- publication references and input shape ---'
rg -n -C 5 "public_payment_methods|publicBookabilityPublication|default_currency|public_hotel_bookability_profiles" \
apps/api/src/platform apps/api/src/routes packages/domain-distribution packages/backend-migration/migrations \
-g '*.ts' -g '*.sql'
printf '%s\n' '--- migration file names and relevant definitions ---'
git ls-files packages/backend-migration/migrations | tail -n 30
rg -n -C 8 "public_hotel_bookability_profiles|public_payment_methods|default_currency" \
packages/backend-migration/migrations -g '*.sql'Repository: vayada-marketplace/vayada
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- projection input, payment method derivation, and normalization ---'
sed -n '334,560p' apps/api/src/platform/publicBookabilityPublication.ts
printf '%s\n' '--- all migration references to the projection table ---'
rg -l "public_hotel_bookability_profiles|public_payment_methods|paymentMethods" \
packages/backend-migration/migrations apps/api/src -g '*.sql' -g '*.ts' \
| sort
printf '%s\n' '--- migration excerpts for projection creation, alterations, and backfills ---'
for file in $(rg -l "public_hotel_bookability_profiles" packages/backend-migration/migrations -g '*.sql' | sort); do
echo "### $file"
rg -n -C 12 "public_hotel_bookability_profiles|paymentMethods|default_currency|capabilities" "$file"
doneRepository: vayada-marketplace/vayada
Length of output: 27507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- public read predicates ---'
sed -n '1940,2010p' apps/api/src/routes/bookingWebPublic.ts
sed -n '2560,2610p' apps/api/src/routes/bookingWebPublic.ts
printf '%s\n' '--- projection entry point and callers ---'
rg -n -C 8 "projectPublicBookability|createTargetPublicBookabilityPublication|PROJECT_PUBLIC_BOOKABILITY_PROFILE" \
apps/api/src -g '*.ts'
printf '%s\n' '--- migration updates or backfills for existing projection rows ---'
rg -n -C 6 "(UPDATE|INSERT INTO).*public_hotel_bookability_profiles|public_hotel_bookability_profiles.*(UPDATE|INSERT INTO)|backfill|back-fill" \
packages/backend-migration/migrations apps/api/src -g '*.sql' -g '*.ts'
printf '%s\n' '--- deterministic source-shape check ---'
python3 - <<'PY'
from pathlib import Path
publication = Path("apps/api/src/platform/publicBookabilityPublication.ts").read_text()
migrations = "\n".join(
p.read_text()
for p in Path("packages/backend-migration/migrations").glob("*.sql")
)
checks = {
"capability key is written": "'paymentMethods', input.public_payment_methods" in publication,
"projection currency column is written": "input.currency,\n CASE" in publication,
"payment methods are normalized to text array": "AS public_payment_methods" in publication,
"readiness rejects empty public methods": "cardinality(input.public_payment_methods) = 0" in publication,
"migration contains projection-row backfill": "UPDATE distribution.public_hotel_bookability_profiles" in migrations,
}
for name, result in checks.items():
print(f"{name}: {result}")
PYRepository: vayada-marketplace/vayada
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- concise backfill check ---'
python3 - <<'PY'
from pathlib import Path
migration_files = sorted(Path("packages/backend-migration/migrations").glob("*.sql"))
for path in migration_files:
text = path.read_text()
if "public_hotel_bookability_profiles" in text:
print(path)
for i, line in enumerate(text.splitlines(), 1):
if "public_hotel_bookability_profiles" in line:
print(f"{i}: {line.strip()}")
PY
printf '%s\n' '--- projection-row mutation statements ---'
rg -n "UPDATE distribution\.public_hotel_bookability_profiles|INSERT INTO distribution\.public_hotel_bookability_profiles|ON CONFLICT \(property_id\).*DO UPDATE" \
packages/backend-migration/migrations -g '*.sql' || true
printf '%s\n' '--- non-test publication call sites ---'
rg -n -C 5 "publicBookabilityPublisher|\.publish\(\{.*propertyId|publishPublicBookability|publishBookability" \
apps/api/src -g '*.ts' -g '!*.test.ts'
printf '%s\n' '--- source-shape verifier ---'
python3 - <<'PY'
from pathlib import Path
publication = Path("apps/api/src/platform/publicBookabilityPublication.ts").read_text()
migration_text = "\n".join(p.read_text() for p in Path("packages/backend-migration/migrations").glob("*.sql"))
checks = [
("capability key is written", "'paymentMethods', input.public_payment_methods" in publication),
("projection currency column is written", "input.currency,\n CASE" in publication),
("payment methods are derived", "AS public_payment_methods" in publication),
("empty methods are a readiness blocker", "cardinality(input.public_payment_methods) = 0" in publication),
("projection-row backfill exists", "UPDATE distribution.public_hotel_bookability_profiles" in migration_text),
]
for label, value in checks:
print(f"{label}: {value}")Repository: vayada-marketplace/vayada
Length of output: 18872
Add a backfill for existing bookability profiles. The publication upsert writes capabilities.paymentMethods and default_currency, but no migration updates existing distribution.public_hotel_bookability_profiles rows. Republish existing profiles before relying on these fail-closed predicates, or older rows can remain without paymentMethods and unavailable to checkout and quote loading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/bookingWebPublic.ts` around lines 1980 - 1981, The
migration or publication-upsert flow must backfill existing rows in
distribution.public_hotel_bookability_profiles by populating
capabilities.paymentMethods and default_currency before enforcing the new
fail-closed predicates. Update the relevant migration/backfill logic, preserving
the publication upsert behavior for future writes and ensuring existing profiles
become eligible for checkout and quote loading.
| const methods = targetCheckoutSupportedPaymentMethods(row?.acceptedMethods).filter((method) => { | ||
| if (method === "card") return row?.providerReady === true; | ||
| if (method === "bank_transfer") { | ||
| return bankTransferDetailsFromPolicy(depositPolicy) !== null; | ||
| } | ||
| if (method === "paypal") return isValidPaymentEmail(depositPolicy["paypalEmail"]); | ||
| return true; | ||
| }); | ||
| const refundPolicy = objectValue(row?.refundPolicy); | ||
| return { | ||
| hotelName: property.displayName, | ||
| defaultCurrency: row?.defaultCurrency ?? "EUR", | ||
| payAtPropertyEnabled: methods.includes("pay_at_property"), | ||
| bankTransfer: false, | ||
| paypalEnabled: false, | ||
| onlineCardPayment: methods.includes("card"), | ||
| bankTransfer: methods.includes("bank_transfer"), | ||
| paypalEnabled: methods.includes("paypal"), | ||
| paypalPaymentWindowHours: boundedPaymentWindowHours(depositPolicy["paypalPaymentWindowHours"]), | ||
| paymentsEnabled: (row?.paymentsEnabled ?? false) && methods.length > 0, | ||
| acceptedPaymentMethods: methods, | ||
| payAtHotelMethods: [ | ||
| ...(row?.acceptedMethods?.includes("cash") ? ["cash"] : []), | ||
| ...(row?.acceptedMethods?.includes("manual_card") ? ["card"] : []), | ||
| ], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
payAtPropertyEnabled and acceptedPaymentMethods can advertise a method that booking creation rejects.
assertTargetCheckoutConfigMatchesQuote accepts pay_at_property only when acceptedMethods also contains cash or manual_card (Lines 2534-2535). This serializer applies no such condition. A property that stores pay_at_property without cash or manual_card therefore shows the method in checkout, and the guest receives a 409 at booking creation.
Apply the same condition in the filter so both surfaces agree.
🛠️ Proposed fix to align the method filter
const methods = targetCheckoutSupportedPaymentMethods(row?.acceptedMethods).filter((method) => {
if (method === "card") return row?.providerReady === true;
if (method === "bank_transfer") {
return bankTransferDetailsFromPolicy(depositPolicy) !== null;
}
if (method === "paypal") return isValidPaymentEmail(depositPolicy["paypalEmail"]);
+ if (method === "pay_at_property") {
+ return (row?.acceptedMethods ?? []).some(
+ (candidate) => candidate === "cash" || candidate === "manual_card",
+ );
+ }
return true;
});📝 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.
| const methods = targetCheckoutSupportedPaymentMethods(row?.acceptedMethods).filter((method) => { | |
| if (method === "card") return row?.providerReady === true; | |
| if (method === "bank_transfer") { | |
| return bankTransferDetailsFromPolicy(depositPolicy) !== null; | |
| } | |
| if (method === "paypal") return isValidPaymentEmail(depositPolicy["paypalEmail"]); | |
| return true; | |
| }); | |
| const refundPolicy = objectValue(row?.refundPolicy); | |
| return { | |
| hotelName: property.displayName, | |
| defaultCurrency: row?.defaultCurrency ?? "EUR", | |
| payAtPropertyEnabled: methods.includes("pay_at_property"), | |
| bankTransfer: false, | |
| paypalEnabled: false, | |
| onlineCardPayment: methods.includes("card"), | |
| bankTransfer: methods.includes("bank_transfer"), | |
| paypalEnabled: methods.includes("paypal"), | |
| paypalPaymentWindowHours: boundedPaymentWindowHours(depositPolicy["paypalPaymentWindowHours"]), | |
| paymentsEnabled: (row?.paymentsEnabled ?? false) && methods.length > 0, | |
| acceptedPaymentMethods: methods, | |
| payAtHotelMethods: [ | |
| ...(row?.acceptedMethods?.includes("cash") ? ["cash"] : []), | |
| ...(row?.acceptedMethods?.includes("manual_card") ? ["card"] : []), | |
| ], | |
| const methods = targetCheckoutSupportedPaymentMethods(row?.acceptedMethods).filter((method) => { | |
| if (method === "card") return row?.providerReady === true; | |
| if (method === "bank_transfer") { | |
| return bankTransferDetailsFromPolicy(depositPolicy) !== null; | |
| } | |
| if (method === "paypal") return isValidPaymentEmail(depositPolicy["paypalEmail"]); | |
| if (method === "pay_at_property") { | |
| return (row?.acceptedMethods ?? []).some( | |
| (candidate) => candidate === "cash" || candidate === "manual_card", | |
| ); | |
| } | |
| return true; | |
| }); | |
| const refundPolicy = objectValue(row?.refundPolicy); | |
| return { | |
| hotelName: property.displayName, | |
| defaultCurrency: row?.defaultCurrency ?? "EUR", | |
| payAtPropertyEnabled: methods.includes("pay_at_property"), | |
| onlineCardPayment: methods.includes("card"), | |
| bankTransfer: methods.includes("bank_transfer"), | |
| paypalEnabled: methods.includes("paypal"), | |
| paypalPaymentWindowHours: boundedPaymentWindowHours(depositPolicy["paypalPaymentWindowHours"]), | |
| paymentsEnabled: (row?.paymentsEnabled ?? false) && methods.length > 0, | |
| acceptedPaymentMethods: methods, | |
| payAtHotelMethods: [ | |
| ...(row?.acceptedMethods?.includes("cash") ? ["cash"] : []), | |
| ...(row?.acceptedMethods?.includes("manual_card") ? ["card"] : []), | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/bookingWebPublic.ts` around lines 2088 - 2110, Update the
payment-method filter around targetCheckoutSupportedPaymentMethods so
"pay_at_property" is included only when row.acceptedMethods contains "cash" or
"manual_card", matching assertTargetCheckoutConfigMatchesQuote. Keep the
existing checks for card, bank_transfer, and paypal unchanged so
payAtPropertyEnabled and acceptedPaymentMethods reflect methods booking creation
accepts.
| !checkoutConfig.providerAccountId || | ||
| !checkoutConfig.providerAccountRef | ||
| ) { | ||
| throw createHttpError(503, "This property has not finished Stripe Connect setup."); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rewrite the guest-facing message in first person.
This 503 message reaches the guest during card checkout and calls the property "This property". The coding guidelines require guest-facing direct-booking messages to speak as the property in first person and to not call it "the property" when handling guest payments.
- throw createHttpError(503, "This property has not finished Stripe Connect setup.");
+ throw createHttpError(503, "We have not finished our card payment setup yet.");As per coding guidelines: "Booking Engine guest-facing direct-booking pages and emails must speak as the property in first person (we, our, us) and must not call it the host, the hotel, or the property when handling guest bookings, payments, or changes."
📝 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.
| throw createHttpError(503, "This property has not finished Stripe Connect setup."); | |
| throw createHttpError(503, "We have not finished our card payment setup yet."); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/bookingWebPublic.ts` at line 3074, Rewrite the 503
message in the booking checkout flow around createHttpError so it speaks in the
property’s first-person voice, using “we,” “our,” or “us” instead of referring
to “This property.” Preserve the existing 503 status and Stripe Connect setup
context.
Source: Coding guidelines
| const paymentIdempotencyKey = targetCardPaymentIdempotencyKey( | ||
| booking.propertyId, | ||
| context.idempotencyKey, | ||
| ); | ||
| let intent: StripeBookingPaymentIntent; | ||
| try { | ||
| intent = await config.stripePaymentProvider.createPaymentIntent({ | ||
| propertyId: booking.propertyId, | ||
| bookingReference: booking.publicReference, | ||
| providerAccountRef: checkoutConfig.providerAccountRef, | ||
| amountMinor, | ||
| applicationFeeAmountMinor, | ||
| currency: booking.currency, | ||
| idempotencyKey: paymentIdempotencyKey, | ||
| }); | ||
| } catch (error) { | ||
| throw createHttpError( | ||
| 502, | ||
| error instanceof Error ? error.message : "Stripe could not start card payment.", | ||
| ); | ||
| } | ||
| if ( | ||
| !intent.clientSecret || | ||
| intent.amountMinor !== amountMinor || | ||
| intent.currency !== booking.currency | ||
| ) { | ||
| throw createHttpError(502, "Stripe returned an invalid card payment authorization."); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the Stripe booking payment provider for request timeouts and abort handling.
set -euo pipefail
fd -t f 'stripeBookingPayments.ts' apps/api/src --exec sed -n '1,120p'
rg -nP 'AbortSignal|timeout|signal:' --type=ts apps/api/src/domains/stripeBookingPayments.tsRepository: vayada-marketplace/vayada
Length of output: 4101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- provider implementation ---'
sed -n '1,220p' apps/api/src/domains/stripeBookingPayments.ts
printf '%s\n' '--- transaction wrapper and checkout call sites ---'
rg -n -C 8 'withTargetCheckoutTransaction|createBooking|createPaymentIntent|targetCardPaymentIdempotencyKey' apps/api/src/routes/bookingWebPublic.ts apps/api/src -g '*.ts' | head -n 320
printf '%s\n' '--- relevant route sections ---'
sed -n '2980,3180p' apps/api/src/routes/bookingWebPublic.tsRepository: vayada-marketplace/vayada
Length of output: 39288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transaction helper ---'
sed -n '1234,1298p' apps/api/src/routes/bookingWebPublic.ts
printf '%s\n' '--- createBooking flow ---'
sed -n '1504,1625p' apps/api/src/routes/bookingWebPublic.ts
printf '%s\n' '--- lock-bearing queries in the flow ---'
rg -n -C 5 'FOR UPDATE|FOR NO KEY UPDATE|quote_session|guest_bookings' apps/api/src/routes/bookingWebPublic.ts | head -n 360Repository: vayada-marketplace/vayada
Length of output: 16626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
route = Path("apps/api/src/routes/bookingWebPublic.ts").read_text()
provider = Path("apps/api/src/domains/stripeBookingPayments.ts").read_text()
create_start = route.index("async function createTargetCardPayment(")
create_end = route.index("\nexport function targetCardPaymentIdempotencyKey", create_start)
create_body = route[create_start:create_end]
booking_start = route.index("async function createTargetGuestBooking(")
booking_end = route.index("\nasync function createTargetCardPayment(", booking_start)
booking_body = route[booking_start:booking_end]
transaction_start = route.index("async function withTargetCheckoutTransaction<")
transaction_end = route.index("\nexport function createTargetBookingWebCheckoutAdapter", transaction_start)
transaction_body = route[transaction_start:transaction_end]
print("provider_fetch_has_signal_or_timeout=",
bool(re.search(r"\b(?:signal|AbortSignal|timeout)\b", provider)))
print("create_booking_uses_transaction=",
"withTargetCheckoutTransaction(pool, async (client)" in route[route.index("async createBooking"):route.index("async quoteBooking")])
print("booking_creation_locks_quote=",
bool(re.search(r"FOR UPDATE OF quote", booking_body)))
print("payment_intent_call_inside_transaction_callback=",
route.index("createTargetCardPayment(") > route.index("withTargetCheckoutTransaction(pool, async (client)", route.index("async createBooking"))))
print("payment_insert_after_provider_call=",
create_body.index("createPaymentIntent") < create_body.index("INSERT INTO finance.payments"))
print("booking_metadata_update_after_provider_call=",
create_body.index("createPaymentIntent") < create_body.index("UPDATE booking.guest_bookings"))
print("transaction_commits_only_after_callback=",
"const result = await action(client);" in transaction_body and
transaction_body.index("const result = await action(client);") <
transaction_body.index('await client.query("COMMIT");'))
PYRepository: vayada-marketplace/vayada
Length of output: 516
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
route = Path("apps/api/src/routes/bookingWebPublic.ts").read_text()
provider = Path("apps/api/src/domains/stripeBookingPayments.ts").read_text()
create_start = route.index("async function createTargetCardPayment(")
create_end = route.index("\nexport function targetCardPaymentIdempotencyKey", create_start)
create_body = route[create_start:create_end]
booking_start = route.index("async function createTargetGuestBooking(")
booking_end = route.index("\nasync function createTargetCardPayment(", booking_start)
booking_body = route[booking_start:booking_end]
transaction_start = route.index("async function withTargetCheckoutTransaction<")
transaction_end = route.index("\nexport function createTargetBookingWebCheckoutAdapter", transaction_start)
transaction_body = route[transaction_start:transaction_end]
create_booking_start = route.index("async createBooking")
create_booking_end = route.index("async quoteBooking", create_booking_start)
create_booking_flow = route[create_booking_start:create_booking_end]
print("provider_fetch_has_signal_or_timeout=",
bool(re.search(r"\b(?:signal|AbortSignal|timeout)\b", provider)))
print("create_booking_uses_transaction=",
"withTargetCheckoutTransaction(pool, async (client)" in create_booking_flow)
print("booking_creation_locks_quote=",
bool(re.search(r"FOR UPDATE OF quote", booking_body)))
print("payment_intent_call_inside_transaction_callback=",
"createTargetCardPayment(" in create_booking_flow and
"withTargetCheckoutTransaction(pool, async (client)" in create_booking_flow)
print("payment_insert_after_provider_call=",
create_body.index("createPaymentIntent") < create_body.index("INSERT INTO finance.payments"))
print("booking_metadata_update_after_provider_call=",
create_body.index("createPaymentIntent") < create_body.index("UPDATE booking.guest_bookings"))
print("transaction_commits_only_after_callback=",
"const result = await action(client);" in transaction_body and
transaction_body.index("const result = await action(client);") <
transaction_body.index('await client.query("COMMIT");'))
PYRepository: vayada-marketplace/vayada
Length of output: 471
Bound the Stripe request and avoid holding checkout locks during it.
createBooking calls createTargetCardPayment inside withTargetCheckoutTransaction after acquiring the quote FOR UPDATE lock. createStripeBookingPaymentProvider passes no timeout or abort signal to fetch, so provider latency can hold the transaction indefinitely. If a later database operation fails, Stripe keeps the PaymentIntent while the transaction rolls back. Add timeout and cancellation/reconciliation handling, or move PaymentIntent creation outside the lock-holding transaction while preserving idempotency and state transitions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/routes/bookingWebPublic.ts` around lines 3091 - 3118, Update
createStripeBookingPaymentProvider and the
createBooking/withTargetCheckoutTransaction flow so Stripe PaymentIntent
creation is bounded by a timeout and supports cancellation, without holding the
quote FOR UPDATE lock during the provider request. Preserve
paymentIdempotencyKey reuse and ensure failures or transaction rollbacks
reconcile the created PaymentIntent before allowing checkout state transitions
to continue.
Summary
Verification
git diff --checkpassedPortless browser verification was blocked because the shared Docker stack is owned by
/Users/flamurmaliqi/git/vayada-marketplace/vayada-vay-1203; it was not removed.Deployment prerequisites
Target rollout requires
API_RUNTIME=next, target Booking/Finance/checkout sources,STRIPE_SECRET_KEY, mutatingSTRIPE_WEBHOOK_SECRET/STRIPE_WEBHOOK_INTAKE_MODE=mutating, and production email delivery viaRESEND_API_KEY+BOOKING_EMAIL_FROM.STRIPE_FIXED_PLAN_PRICE_IDremains optional because the canonical Price can be provisioned and verified.Stacked on #825.
Summary by CodeRabbit