Finish paid bookings in a fixed number of database calls - #2048
Merged
Conversation
Contributor
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe payment flow now loads a single paid-order snapshot for validation, pricing, ledger replay, attendee creation, refunds, and notifications. Shared package facts prevent repeated reads. Processed payments and stored attendee answers use atomic database paths with expanded regression coverage. ChangesPaid-order processing and persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dfe3a6924
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…payment-processing-core # Conflicts: # scripts/mutation/equivalent-mutants/features.txt # scripts/mutation/equivalent-mutants/shared-a-l.txt # scripts/mutation/equivalent-mutants/shared-db.txt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 010db3f0de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/shared/db/processed-payments.test.ts (1)
81-97: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRename this test:
reserveSessionno longer retries.The implementation now claims a stale unresolved row with one conditional upsert. No retry remains. The test asserts exactly one database call, which contradicts the name "retries when stale reservation detected".
✏️ Proposed rename
- test("retries when stale reservation detected", async () => { + test("claims a stale reservation in one database call", async () => {🤖 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 `@test/shared/db/processed-payments.test.ts` around lines 81 - 97, Rename the test describing the stale reservation scenario so it reflects that reserveSession claims the stale row with a single database call rather than retrying. Update only the test name in the test around reserveSession and preserve its assertions and setup.
🤖 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 `@src/features/api/payment-processing/create.ts`:
- Around line 231-247: Update promoCodeActivities to retrieve each modifier
application with requiredMapValue(byId, spec.id) instead of the unchecked
non-null assertion before reading delta. Add a regression test covering the
required spec-to-application contract.
In `@src/shared/db/processed-payments.ts`:
- Around line 139-142: Replace the manual staleBefore calculation in the
processed-payments claim flow with isoBefore(STALE_RESERVATION_MS), while
keeping claimedAt derived from nowIso() unchanged.
In `@src/shared/db/questions/attendee-answers/save.ts`:
- Around line 220-224: Update storedIdsOnly in
src/shared/db/questions/attendee-answers/save.ts around lines 220-224 to
evaluate the normalized map after line 238, using set.textAnswers.length === 0
without excluding arrays, so normalizeAnswerSet reaches storedIdAnswerStatements
for array inputs. In
test/shared/db/questions/attendee-answers/save/stored-ids-behavior.test.ts lines
207-214, rename the test to describe the batch path and change the expected
database call count from three to one.
In `@test/features/api/payment-processing/create.test.ts`:
- Around line 283-289: Replace the as never casts in the promoCodeActivities
call with properly typed, complete ModifierSpec, ModifierApplication, and
listing fixtures. Ensure the fixtures satisfy the production activity contract
directly, including all required fields, so contract changes cause this test to
fail at compile time.
In `@test/features/api/payment-processing/create/answers.test.ts`:
- Around line 16-27: Remove the unnecessary `as CreatedEntry` assertion from the
return statement in `bookedEntry`; return the `{ attendee, listing: loaded }`
object directly while preserving the existing `CreatedEntry` return type.
In `@test/features/api/payment-processing/index/helpers.ts`:
- Around line 106-108: Require the result of getProcessedPayment before
asserting failure_data, replacing optional chaining with a strong assertion that
the processed-payment row exists and then checking its failure_data value. Apply
this change at test/features/api/payment-processing/index/helpers.ts:106-108,
test/features/api/payment-processing/index/balance.test.ts:93-93, and
test/features/api/payment-processing/index/booking.test.ts:141-141; each site
requires the same direct update.
In `@test/features/api/payment-processing/store-refund.test.ts`:
- Line 19: Add an assertion in the refund persistence test to verify the stored
attendee status_id equals await requirePublicStatusId(). Extend the existing
row-count and quantity assertions without changing their behavior, using the
imported requirePublicStatusId symbol.
In `@test/integration/processed-payments/locking.test.ts`:
- Around line 143-172: Update the locking test around the gated db.batch stub
and winner assertion so it does not assume stub invocation order matches the
attempts array order. Release the first invocation gate, await all attempts, and
assert the aggregate result that exactly one attempt reserved the session,
relying on the existing remaining assertions for the full outcome.
In `@test/integration/server/api-packages.test.ts`:
- Around line 479-495: Update the test around apiBookPackage to track whether
the execute branch matching groupRecord.hide_package_listings was invoked, set
the flag when injecting the database failure, and assert it after the call. Keep
the existing recovery and booking-row assertions, ensuring the test fails if the
expected lookup no longer uses that failure path.
In `@test/shared/registration-package-facts.test.ts`:
- Around line 25-60: Update the test named “loads each package once with its
display and complete member pricing” to record package query statements and
assert that both loaders receive group.id exactly once, despite duplicate
row(group.id) inputs. Preserve the existing display and pricing assertions, and
ensure the added assertions cover deduplication while maintaining full coverage
and mutation detection.
In `@test/shared/webhook/budget.test.ts`:
- Around line 147-170: Update the test around sendRegistrationWebhooks to
capture the emitted webhook payload while retaining the zero-database-call
assertion, then assert the payload contains the supplied package name “Supplied
package” and package price 500. Use the existing webhook test capture mechanism
and verify the values originate from the facts object.
In `@test/test-utils/db-poison.ts`:
- Around line 19-24: Extract the shared statement-matching, one-shot poisoning,
rejection, and delegation flow into a helper in db-poison.ts. Update both the
db.batch and tx.batch wrappers to call this helper, passing their respective
real batch functions as delegates, while preserving the existing poisoned state
and error-message behavior.
- Around line 8-10: Declare the explicit return type of the exported
withPoisonedWrite helper as a function accepting body and returning
Promise<void>, while preserving the existing curried behavior and
implementation.
In `@test/test-utils/processed-payments.ts`:
- Around line 19-24: Update expectSessionFailed so failure_data is asserted to
be a populated non-empty string, rather than merely not equal to an empty
string; preserve the existing attendee_id assertion and session error handling.
---
Outside diff comments:
In `@test/shared/db/processed-payments.test.ts`:
- Around line 81-97: Rename the test describing the stale reservation scenario
so it reflects that reserveSession claims the stale row with a single database
call rather than retrying. Update only the test name in the test around
reserveSession and preserve its assertions and setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 106ebbaf-6b47-434f-954b-967da7169416
📒 Files selected for processing (87)
scripts/mutation/equivalent-mutants/features.txtscripts/mutation/equivalent-mutants/shared-a-l.txtscripts/mutation/equivalent-mutants/shared-db.txtscripts/mutation/equivalent-mutants/shared-m-z.txtsrc/features/api/payment-processing/completion.tssrc/features/api/payment-processing/create.tssrc/features/api/payment-processing/index.tssrc/features/api/payment-processing/items.tssrc/features/api/payment-processing/package-pricing.tssrc/features/api/payment-processing/recovery.tssrc/features/api/payment-processing/snapshot/build.tssrc/features/api/payment-processing/snapshot/io.tssrc/features/api/payment-processing/snapshot/types.tssrc/features/api/payment-processing/store-refund.tssrc/shared/db/activity-log.tssrc/shared/db/attendee-types.tssrc/shared/db/attendees/create.tssrc/shared/db/attendees/order-parents.tssrc/shared/db/attendees/queries.tssrc/shared/db/modifier-resolve.tssrc/shared/db/processed-payments.tssrc/shared/db/questions/attendee-answers/save.tssrc/shared/email-renderer.tssrc/shared/email.tssrc/shared/registration-package-facts.tssrc/shared/session-ledger.tssrc/shared/webhook.tstest/features/admin/questions/listing-questions.test.tstest/features/api/payment-processing/completion.test.tstest/features/api/payment-processing/create.test.tstest/features/api/payment-processing/create/answers.test.tstest/features/api/payment-processing/index/balance.test.tstest/features/api/payment-processing/index/booking.test.tstest/features/api/payment-processing/index/helpers.tstest/features/api/payment-processing/index/refunds.test.tstest/features/api/payment-processing/items.test.tstest/features/api/payment-processing/items/boundaries.test.tstest/features/api/payment-processing/items/budget.test.tstest/features/api/payment-processing/items/helpers.tstest/features/api/payment-processing/package-pricing.test.tstest/features/api/payment-processing/package-pricing/database.test.tstest/features/api/payment-processing/recovery.test.tstest/features/api/payment-processing/snapshot/build.test.tstest/features/api/payment-processing/snapshot/io.test.tstest/features/api/payment-processing/store-refund.test.tstest/integration/email/config.test.tstest/integration/email/registration.test.tstest/integration/processed-payments/locking.test.tstest/integration/questions-attendee-answers.test.tstest/integration/server/api-packages.test.tstest/integration/server/balance-payment-replay.test.tstest/integration/server/payments/confirm.test.tstest/integration/server/payments/replay.test.tstest/integration/server/payments/success.test.tstest/integration/server/reservation-edge-cases.test.tstest/integration/server/webhooks/can-pay-more-multi-ticket.test.tstest/integration/server/webhooks/concurrent-processing.test.tstest/integration/server/webhooks/multi-ticket-refunds.test.tstest/integration/server/webhooks/price-signature-package-overrides.test.tstest/integration/server/webhooks/price-signature-post-commit-recovery.test.tstest/integration/server/webhooks/price-signature-stored-refund-and-ignore.test.tstest/integration/server/webhooks/refund-helper-functions.test.tstest/integration/servicing/atomicity.test.tstest/integration/webhook-price-signature-trusted-and-mismatch.test.tstest/shared/checkout-pricing/consistency.test.tstest/shared/db/attendees/api/create-rollback.test.tstest/shared/db/attendees/balance.test.tstest/shared/db/attendees/create.test.tstest/shared/db/attendees/delete.test.tstest/shared/db/listings/delete.test.tstest/shared/db/modifier-resolve.test.tstest/shared/db/processed-payments.test.tstest/shared/db/processed-payments/finalize-guard.test.tstest/shared/db/processed-payments/staleness.test.tstest/shared/db/questions/attendee-answers/save/group-listings.test.tstest/shared/db/questions/attendee-answers/save/stored-ids-behavior.test.tstest/shared/db/questions/attendee-answers/save/stored-ids.test.tstest/shared/registration-package-facts.test.tstest/shared/session-ledger.test.tstest/shared/webhook/budget.test.tstest/shared/webhook/payload-fields.test.tstest/shared/webhook/payload.test.tstest/specs/steps/payment-capacity.tstest/test-utils/db-poison.tstest/test-utils/email.tstest/test-utils/processed-payments.tstest/test-utils/webhooks.ts
💤 Files with no reviewable changes (4)
- src/shared/db/modifier-resolve.ts
- src/shared/db/attendees/queries.ts
- scripts/mutation/equivalent-mutants/shared-a-l.txt
- test/shared/db/modifier-resolve.test.ts
Member
Author
|
CodeRabbit did not return findings because its review was rate limited. All six Codex review threads were verified, fixed in |
Member
Author
|
CodeRabbit outside-diff note resolved in ae81264: the stale reservation test is now named |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/shared/webhook/budget.test.ts (1)
148-177: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate required test values before use.
storedEntry!andpayload.tickets[0]!hide an absent fixture or webhook ticket. Check both values explicitly so the test fails with a local, clear error.Proposed fix
const [storedEntry] = await packagedEntries("Supplied", 1); + if (!storedEntry) throw new Error("Expected one supplied package entry"); const entry = { - ...storedEntry!, - listing: { ...storedEntry!.listing, name: "Supplied package" }, + ...storedEntry, + listing: { ...storedEntry.listing, name: "Supplied package" }, }; @@ ).toBe(0); const payload = fetchSpy.firstBody(); - expect(payload.tickets[0]!.listing_name).toBe("Supplied package"); - expect(payload.tickets[0]!.unit_price).toBe(500); + const [ticket] = payload.tickets; + if (!ticket) throw new Error("Expected one webhook ticket"); + expect(ticket.listing_name).toBe("Supplied package"); + expect(ticket.unit_price).toBe(500);As per coding guidelines, do not use unchecked non-null assertions for required data. Based on learnings, test fixtures should validate missing values locally.
🤖 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 `@test/shared/webhook/budget.test.ts` around lines 148 - 177, Update the test around packagedEntries and fetchSpy.firstBody to validate storedEntry and payload.tickets[0] explicitly before accessing their properties, replacing the non-null assertions with local, clear failures while preserving the existing assertions and webhook behavior.Sources: Coding guidelines, Learnings
🤖 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.
Outside diff comments:
In `@test/shared/webhook/budget.test.ts`:
- Around line 148-177: Update the test around packagedEntries and
fetchSpy.firstBody to validate storedEntry and payload.tickets[0] explicitly
before accessing their properties, replacing the non-null assertions with local,
clear failures while preserving the existing assertions and webhook behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0030f261-8e86-4fcf-a894-e660b5392682
📒 Files selected for processing (17)
src/features/api/payment-processing/create.tssrc/shared/db/processed-payments.tssrc/shared/db/questions/attendee-answers/save.tstest/features/api/payment-processing/create.test.tstest/features/api/payment-processing/create/answers.test.tstest/features/api/payment-processing/index/balance.test.tstest/features/api/payment-processing/index/booking.test.tstest/features/api/payment-processing/index/helpers.tstest/features/api/payment-processing/store-refund.test.tstest/integration/processed-payments/locking.test.tstest/integration/server/api-packages.test.tstest/shared/db/processed-payments.test.tstest/shared/db/questions/attendee-answers/save/stored-ids-behavior.test.tstest/shared/registration-package-facts.test.tstest/shared/webhook/budget.test.tstest/test-utils/db-poison.tstest/test-utils/processed-payments.ts
…payment-processing-core
stefan-burke
commented
Aug 7, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@AGENTS.md`:
- Around line 171-186: Resolve the contradiction in AGENTS.md by splitting its
guidance into linked topic files, or by explicitly exempting root instruction
files from the 400/1,000-line limits and documenting the rationale. Update the
file-size guidance so it consistently applies to the intended files, including
the current AGENTS.md structure, without leaving conflicting requirements.
- Around line 1059-1069: Remove the full-access DB_TOKEN output from the
migration flow in scripts/turso-migration-steps.ts, including any success or
failure recovery messages that print it. Update the AGENTS.md migration
instructions to direct operators to retrieve the token through a secure existing
path instead of stdout, while retaining the DB_URL and DB_ENCRYPTION_KEY
guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5966b37-1db4-421f-b0e5-4fad1abbed09
📒 Files selected for processing (2)
AGENTS.mdsrc/features/api/payment-processing/snapshot/io.ts
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Why
Large paid orders should cost the same small number of database requests as simple orders. This leaves room under the edge request limit and closes races when several callbacks try to reclaim the same stale payment. Provider reads must not silently change prices or lose answers because old links or a lagging replica were observed. Named batch results also make the snapshot code easier to read and safer to reorder.
Checks
nix develop -c deno task precommitSummary by CodeRabbit