Stage paid bookings without holding seats - #1832
Conversation
Write a paid order as an attendee with quantity-zero booking rows and a checkout_stages record BEFORE creating the provider session, then let the payment claim those exact rows atomically when it lands. This closes the window where a completed payment could find no record to attach to, and gives every terminal outcome — ticket, refund, or operator conflict — one place to resolve. Deterministic activation problems now end safely instead of crash-looping the webhook with the customer's money held and no record: - Changed booking lines (all rows still quantity 0, nothing live) take the normal keep-and-refund path with a clear "the booking changed between checkout and payment" reason. - Rows already given a real quantity may be a live booking, so that becomes a no-refund operator conflict: a loud alert, a note on the record saying what to check, and a recorded outcome so replays answer the same. - Transient and system errors still retry on the provider's next delivery. Held and refunded money is always on the record: every kept record for a charged session stamps the provider's payment reference, and the ledger shows the charge plus whatever refund actually happened. A failed refund stays retryable (the stage resolves only after the money records land), and a crash between the ledger post and the stage flip heals on the next delivery. Mid-payment records are protected from admin edits and merges (both fail closed with a plain message; deleting still works). Checkouts can start on overbooked or just-deactivated listings because quantity-zero staging skips the capacity gate — real capacity is enforced at activation. Stripe checkouts get a hard deadline (CHECKOUT_SESSION_EXPIRY_MINUTES, default 60); expired sessions are discarded the moment Stripe reports them. Shared mechanisms extracted along the way: a queryAllPrimary/queryOnePrimary client helper, a matchingIdSet batch-membership helper, the claim-refusal classification in its own directly-tested module, and one shared attendee-echo projection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018pdUrVrzMwSgWMxdAn4qmu
A pending checkout stage losing its attendee mid-payment used to be "recovered" by booking fresh from the signed order (createFresh / the dangling-stage null return). That re-ran the whole create at the worst moment — the customer has paid and left — trading a loud, catchable failure for a second attempt with its own failure modes. Make it an invariant instead: a pending stage and its rows only ever die together. Enforce it, then throw if it is ever violated. - Block deleting a mid-payment record (admin delete + delete-incomplete share one guard) and deleting a listing while it has a pending checkout (one listingDeleteError guard on all three delete paths). Abandoned checkouts still auto-clean via the prune / provider-expiry discard. - Add checkout_stages to the orphan-purge cascade, so a listing deleted mid-checkout can't leave a dangling stage behind. - findStageProblem (zero rows) and getCheckoutStageOrNull (dangling pending stage) now throw a contextful error instead of returning a "book fresh" signal; drop stage_gone from ActivationFailure and the createFresh recovery branch (createFresh stays for the genuine no-stage path). - Fix the copy that told operators to "delete the record" as an escape hatch, now that deleting mid-payment is blocked. Reworks the stage-gone tests to expect the loud throw and adds regression tests for both delete guards, the listing pending-checkout check, and the orphan stage cascade. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018pdUrVrzMwSgWMxdAn4qmu
Capture the locked design for the staged-checkout money model: validate fully before the provider session, save the booking at quantity zero with the owed ledger legs, add received-funds legs at payment, exclude pending-staged legs from ledger sums (derived from the stage link), and delete legs when an abandoned stage is pruned. Records the never-hold-a-seat policy and the remaining review items. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018pdUrVrzMwSgWMxdAn4qmu
Reorder createStagedCheckout so the order is fully validated BEFORE the provider session is created: check the real quantities fit and every listing is still on sale, and if not, stop and tell the customer up front with a plain message instead of letting them pay and then refunding. The staged rows themselves still claim nothing (quantity 0), so the authoritative capacity claim continues to run at activation with the real quantities — this new gate only stops a checkout that provably cannot succeed. It is the single chokepoint for every new-booking entry point (single booking, folded order, ticket page, QR), each of which already checked availability separately; folding the check into the one shared function keeps callers from ever handing a sold-out or off-sale order to the provider. Only new bookings flow through createStagedCheckout — a balance payment settles an existing booking and goes straight to the provider — so the dead balance-attendee handling is removed and the function always validates and stages. Adds tests that an already-overbooked listing and an off-sale listing are both refused before the provider is reached (stubbed to throw), returning the unavailable error and writing no stage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018pdUrVrzMwSgWMxdAn4qmu
The listing-delete guard blocks a delete while a pending checkout exists, but it is a preflight read: a checkout that stages its booking in the window between the guard and the delete would then have its listing_attendees rows cascaded away. When that customer's payment lands, the deleted-listing refund path reuses the now-empty staged attendee, so the refund and note attach to a record with no booking rows — the operator loses the paid order. Make the bad state impossible at the source: deleteListing no longer cascades the rows of an attendee whose checkout is still pending, so a raced delete leaves the quantity-0 order whole. The checkout then resolves normally (its payment keeps the order and refunds) or the prune removes the attendee outright. A resolved (booked/failed) stage's rows are cascaded like any other booking. Adds a regression test that a pending checkout's rows survive a listing delete while a booked one's are removed (fails before the fix: the pending rows were deleted).
Keep all ledger legs at activation rather than posting owed legs at staging: the ledger posts an event group as one immutable set, so owed-at-staging plus received-at-activation would need two event groups and careful handling of the sale-scoped per-row projection — for no change in any total, since a pending stage (no legs, quantity 0) is already excluded from every sum and activation is already atomic. plan.md records the decision and why the owed-at-staging slices are dropped. Also document the standing no-seat-holding policy in AGENTS.md: a checkout never reserves capacity (quantity-0 staging; first payment wins; a loser is refunded), because holding seats invites botting.
The activation flip to 'booked' was an unconditional UPDATE by session id: if the stage were no longer this attendee's pending stage — a concurrent resolution, or a session/attendee mismatch — it would silently succeed and book a stage that was already resolved. Make it a compare-and-set: only a row that is still this attendee's PENDING stage may flip to booked, and exactly one must. The row-level pre-check and the fail-closed guard mean the stage IS pending here, so a miss is an impossible state — throw to roll the whole activation back rather than book a stage that isn't ours, and let the webhook redelivery re-resolve it (a resolved stage reads as already-handled at the preflight). Adds a test that activation throws and rolls back (rows stay quantity 0) when the stage was resolved out from under it, and folds the shared throw+rollback assertion into a helper.
A booking whose listing was deleted is dropped from the operator attendee table (attendeeBookingsFromLines resolves each line's listing from getAllListings and skips a missing one). This is inherent to any deleted-listing ghost — the no-stage datelessGhostBookings refund path and, after the delete/stage race fix, a raced-delete staged order. The data is intact; only the table line is hidden. Records the "renderable deleted-listing placeholder line" follow-up alongside the admin-lifecycle work so the look is decided once across every deleted/pending case.
Two coupled fixes to the terminal staged-refund paths, both about a failed
ledger post:
- Stamp the payment reference only AFTER the money is recorded. Both
recordHeldStagedMoney and storeRefundedBooking stamped the provider payment
id onto the staged attendee before posting the held-payment leg. If that post
failed, the throw left a still-pending record carrying a refundable reference,
so the Actions tab would offer an in-app refund for a charge the ledger had
not recorded — refund it there and the retry records held cash out of sync.
Posting first means a failed post throws before any reference is stamped.
- Honour a failed placeholder post on a staged order. storeRefundedBooking
ignored recordPlaceholderRefund's {posted:false}, so a staged keep-and-refund
whose ledger round-trip could not be written still wrote the note, resolved
the stage, and went terminal with the money missing from the books — later
deliveries then replayed the resolved stage and could not repair it. It now
throws on a failed post when a stage can absorb the retry, leaving the stage
pending for the redelivery (the settled refund reads back as refunded, so it
never refunds twice). The no-stage path can't retry without minting a
duplicate placeholder, so it still proceeds — the miss stays logged.
Tests: the kept-and-refund placeholder path throws and stays pending on a
blocked post (then resolves on retry), and neither terminal path leaves a
payment reference stamped after a failed post.
A stage_active conflict records a provider `payment` leg with NO sale, so the attendee holds cash we must return but the line projects price_paid 0. The two no-quantity guards both look for a sale — the per-line price_paid check and the DB-side hasPaidLine — so neither blocked the operator from marking the line no quantity. The in-app refund needs an active booking line, so doing that stranded the held cash with no in-app way back. Adds attendeeHoldsUnreturnedCash — a decryption-free check that the attendee account has a positive ledger balance (cash in, nothing owed against it), which only a held conflict payment produces (a fully-paid booking nets to 0, a deposit is negative, a refund nets back to 0). The save path now refuses a no-quantity edit while that cash is unreturned, with the existing "refund this booking's payment first" message. Test: an attendee holding a payment leg with no sale is blocked from marking its line no quantity, and the line is left intact so the refund path still reaches.
The held-cash state a stage_active conflict leaves — the stage resolved to `failed`, but a provider `payment` leg with no sale still on the account — was only guarded against no-quantity edits. Delete and merge checked only for a still-pending stage, so either could remove or repoint that record before the held cash was refunded: a delete orphans the ledger cash (losing the note, the row, and the in-app refund path), and a merge mixes it onto an account the refund ledger can no longer reconcile. Both now also block a record holding unreturned cash, with a plain "refund this payment first" message. The held-cash check is folded into one batch, primary-pinned lookup (attendeeIdsHoldingUnreturnedCash) — a replica lagging the just-posted held-payment leg must not let a mutation slip past, the same reason the pending-stage guard reads the primary. Both batch guards now share one curried primaryMatchingIdSet helper, and the two merge guard sites share one mergeMoneyBlock reason resolver. Tests: a held-cash record is refused deletion (and left intact so the refund still reaches) and refused as a merge participant.
Staged-refund paths now post the money legs BEFORE stamping the provider payment reference (so a failed post never exposes a refund). That moved the crash window: a crash after the legs land but before stampStagedPaymentId leaves the stage pending with an empty payment_id. The next delivery answers off the ledger and takes the orphan-heal path, which resolved the stage and wrote the note but never stamped the reference — so the kept record's payment panel and refund path stayed hidden for money the note tells the operator to reconcile. The heal now stamps the payment reference too (it already had the session and intent threaded in), rebuilding the same stored details staging wrote. It reads the full stage via getCheckoutStageOrNull (which carries the ticket token the stamp needs), so the single-purpose pendingStageAttendeeIdOrNull is now dead and removed. Test: the wedged-stage heal now asserts the kept record's payment_id is the session's reference, not empty.
The owner's call on the last Codex finding: a paid-but-stuck staged checkout (processing failing past the provider's ~3-day webhook-retry window) can be pruned at 7 days, deleting our record of a payment still captured at the provider. Left as-is because the money is never lost (recoverable at the provider) and every failing delivery alerts the operator (ntfy + admin activity log + Sentry) across the retry window, so they reconcile well before the prune. Records the accepted trade and the two future hard-fix paths (a durable capture mark, or recording held cash on deferral) so the decision is visible and not re-litigated.
Records everything shipped (items 1, 6, 3-decision, the compare-and-set flip, and all eight Codex findings including the accepted paid-stage-prune edge) and lays out the remaining order of work: #8 admin lifecycle next, then #9 provider lifecycle, then #10 cleanup, with #5/#7 deliberately parked in TODO.md.
Item 8 of the staged-checkout review, first half: a record whose checkout is still pending is locked server-side, but the UI still offered every control and read like an ordinary zero-quantity booking. Now: - The attendee page hides the Edit, Logistics, and Actions tabs (and the send-email button) while a checkout is pending — every mutation they lead to is blocked anyway, and a hidden tab's URL is a 404. A banner alert explains: "Checkout pending: the customer may still be paying. This record is locked until the payment finishes or expires." The flag loads once on the shared page entity. - An edit submission that raced the checkout redirects to the always-visible overview with the same refusal as a flash (the hidden Edit tab could no longer re-render the form refusal in place), replacing the in-save guard. - Every attendee row now carries a `pending_checkout` flag, projected per row from checkout_stages in the one shared attendee SELECT — so the attendee tables' status and ticket cells say "Payment in progress" instead of the "No quantity" wording that invites an edit. - CSV exports (attendees + calendar) gain a "Checkout pending" column, emitted only when some exported row is mid-payment, so a pending row never reads as a plain zero-quantity booking. Tests: tabs hidden + 404 while pending and restored once resolved, the banner alert, the raced-edit redirect, the table wording end-to-end through the browsing page, and the conditional CSV column.
Item 8 of the staged-checkout review, second half: - A stored note can carry a markdown link to the ledger (the refund notes do), but the ledger pages are owner-only — every other admin saw a link that only 404s. Note bodies now demote an /admin/ledger link to its plain text for non-owners (new withoutLinksTo markdown helper), threaded through the attendee banner, the notes summaries above the attendee/listing lists, and the delete-note page. - A booking row can outlive its listing (a delete racing a mid-payment checkout keeps the staged rows), but the read-only bookings table silently dropped such a line — the operator couldn't see what the customer paid for. It now shows a plain "Deleted listing" placeholder row instead, with no link (the listing page would 404) and no "(Inactive)" note. The line builder's untrue "deleting a listing deletes its rows" non-null assertion becomes an honest null. - The Logistics mid-payment refusal now redirects to the always-visible overview, like the Edit one — its old target (the Logistics tab) is hidden while pending, so the flash landed on a 404 (raised by Codex). Tests: owner keeps the note's ledger link and a non-owner gets plain text (unit + banner), the placeholder line renders without a dead link (unit + end-to-end through a raced-delete staged record), and the logistics refusal lands on the overview.
Item 9 of the staged-checkout review. Webhook setup deleted only the endpoint id recorded in our settings before recreating — an endpoint on the same url whose id we lost (a database restore, a re-setup that didn't save) was never found, so it lived on at Stripe and failed signature verification against the new secret on every delivery, forever. Setup now lists the account's endpoints and removes every one pointing at OUR webhook url (plus the recorded id, which may point at an old url after a domain change) before creating the fresh endpoint. Endpoints on other urls are never touched. Listing is best-effort: if it fails, setup still deletes the recorded endpoint and creates the new one. The two places that list webhook endpoints (this reconcile and the settings health check) now share one listWebhookEndpoints helper. Also locks the rest of item 9 with a direct test and documentation: - SumUp EXPIRED maps to the terminal "failed" status (new provider test), so a lapsed checkout takes the same discard-and-cancel-page path as a declined card. - The cancel path's local-discard-then-remote-expire order is deliberate (the discard's claim guard is what makes closing the remote session safe) and now documented in plan.md, with every gap landing on the designed no-stage fresh-booking path. Test: setup against a mocked endpoints API removes the recorded endpoint and the same-url stray, keeps the other-url endpoint, and returns the fresh secret (fails before the fix — only the recorded endpoint was removed).
Item 10 of the staged-checkout review. Three places each kept their own list of the tables that hang off an attendee: the single-attendee delete, the orphaned-attendee purge (with a special case for service_costs' different key column), and the pending-checkout discard (importing the raw DEPENDENT_ROW_TARGETS list to build its own statements). Three parallel implementations of one operation is exactly the drift the review flagged. All three now build from one mechanism in delete.ts: attendeePurgeStatements (internal — the raw table list is no longer exported) run through runAttendeePurge, which reports the final statement's affected rows. The discard's one genuine difference is a named option (stagesLast: its id-select reads checkout_stages, so those rows must outlive every other delete); the orphan purge gains service_costs parity through the shared field mapping instead of a hand-written special case. A new dependent table added to the one list is cleaned by every purge path automatically. Behaviour is unchanged — the existing delete, orphan-purge, discard, and prune suites all pass as-is, and plan.md now records items 8-10 as shipped.
CI failed on "generates different slugs on multiple calls": 20 random 5-character slugs (~1.15M combinations) asserted zero collisions, which is a birthday problem — a real collision fires about once per 6,000 runs, and one just did. Unrelated to this branch; fixed in passing per the good-citizen rule. The test now drives generateSlug with two disjoint pinned Math.random sequences and asserts the outputs differ — deterministic, and it still catches a cached or constant slug that ignores the randomness (the format and Fisher-Yates tests beside it cover the rest of the contract).
Two Codex findings on PR #1802, both from the delete/mid-payment race that keeps a booking row alive after its listing is gone: - The Ledger tab now hides (and its URL 404s) while a checkout is pending, like the other write tabs — it embeds manual charge/payment forms, and a manual leg posted mid-payment would combine with activation's own legs into a surprise balance. - The Edit tab no longer throws on a kept row whose listing was deleted. The row renders locked: a 'Deleted listing' placeholder, a hidden no-quantity tick, and no controls, so every save submits it unchanged (the save deletes any row its form leaves out). Validation refuses a hand-crafted submission that un-ticks the lock, and the saved line keeps the row's own stored date range instead of the shared range. Regression tests fail before each fix: the hides-write-tabs test now covers /ledger, and new edit/unit tests cover the locked row's render, retention, range preservation, and the un-lock refusal.
…d money Two more Codex findings on the same theme: - The standalone ledger add route (POST /admin/ledger/attendee/:id/add) now refuses a manual charge or payment while the attendee's checkout is pending — the activation posts its own sale/payment legs when the payment lands, so a manual entry added mid-payment would combine with them into a surprise balance. This is the write route behind both the (already hidden) Ledger tab and the standalone statement page, so the freeze is enforced where the money moves. - Deleting a listing is now refused while any attendee booked on it holds unreturned conflict cash. The delete cascades the booking rows, and the in-app refund needs an active booking line, so the delete would strand the held charge with no refund path. New listingHoldsUnreturnedCash reuses the primary-pinned held-cash batch guard over the listing's attendees. Both regression tests fail before their fix: the mid-payment manual entry is refused with no transfer written, and the held-cash listing delete is refused with the listing intact.
…ead action links Three more Codex findings on the staged-checkout flow: - A staged order whose provider refund FAILED now stays retryable instead of going terminal. Before, storeRefundedBooking posted the held-money payment leg and resolved the stage even when the refund didn't settle; the next delivery then read the ledger as 'already handled' and never re-attempted the refund, stranding the money until an operator refunded by hand. It now leaves the stage pending with no ledger legs and reports the refund as unsettled, so the provider redelivers and the refund is retried until it lands — mirroring the closed-listing path. The no-stage path stays terminal (a retry there would mint a duplicate placeholder). - The held-cash no-quantity guard no longer over-blocks. It refused any edit that touched a no-quantity line while conflict cash was held, so an operator couldn't fix the other quantities the conflict note asks them to check. It now blocks only a save that removes the active home line the in-app refund needs, leaving edits that keep it intact. - The attendee Actions tab now hides when the record's listing was deleted mid-payment. Every attendee-scoped action route loads the home listing and 404s once it is gone, so those delete/resend/refund links only broke on click; the tab now hides like it does while a checkout is pending. Each ships with a regression test that fails before its fix. The staged-checkout test file crossed the 1000-line ceiling, so its shared setup moved to server-payment-staging-helpers.ts and the recovery-focused tests to their own file.
📝 WalkthroughWalkthroughThis PR introduces staged checkout lifecycle management with quantity-zero placeholders, post-payment activation, atomic booking writes, refund recovery, checkout cleanup, admin mutation locks, deleted-listing retention, backup validation, Stripe webhook reconciliation, and broad integration coverage. ChangesStaged checkout lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 44
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/shared/stripe.ts (1)
285-300: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep the recorded endpoint until the retry succeeds. Deleting
existingEndpointIdbefore the second create leaves the DB pointing at a deleted endpoint if that retry fails, which stops webhook delivery until the endpoint is recreated.🤖 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 `@src/shared/stripe.ts` around lines 285 - 300, The endpoint cleanup in the endpoint-limit recovery path must not delete existingEndpointId before the replacement is created successfully. Update the catch block around createStripeWebhookEndpoint to perform the retry first, then call deleteEndpointsBestEffort for existingEndpointId only after that retry succeeds, preserving the recorded endpoint if creation fails.src/shared/db/attendees/delete.ts (1)
54-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPositional reliance on
checkout_stagesbeing last is fragile.The split at
deletes.slice(0, -1)/deletes.slice(-1)assumescheckout_stagesstays the final entry inDEPENDENT_ROW_TARGETS, enforced only by a comment. A future addition after it (or a reorder) would silently break the ordering guarantee this function exists to provide.♻️ Proposed fix: derive the split by table name
+const CHECKOUT_STAGE_INDEX = DEPENDENT_ROW_TARGETS.findIndex( + ({ table }) => table === "checkout_stages", +); + export const attendeeDependentDeleteStatements = ( attendeeIds: SqlStatement, beforeCheckoutStage: SqlStatement[] = [], ): SqlStatement[] => { const deletes = DEPENDENT_ROW_TARGETS.map(({ field, table }) => ({ args: attendeeIds.args, sql: `DELETE FROM ${table} WHERE ${field} IN (${attendeeIds.sql})`, })); return [ - ...deletes.slice(0, -1), + ...deletes.slice(0, CHECKOUT_STAGE_INDEX), ...beforeCheckoutStage, - ...deletes.slice(-1), + ...deletes.slice(CHECKOUT_STAGE_INDEX), ]; };🤖 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 `@src/shared/db/attendees/delete.ts` around lines 54 - 79, Update attendeeDependentDeleteStatements to locate the checkout_stages delete by its table name rather than relying on DEPENDENT_ROW_TARGETS ordering. Insert beforeCheckoutStage immediately before that identified statement, while preserving all other dependent-delete ordering and behavior.src/shared/db/attendees/create.ts (1)
111-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
capacity_exceededfailure literal.The same
{ reason: "capacity_exceeded", success: false }shape is built inline here and again incapacityFailure()(lines 250-253). Reuse the helper instead of a second inline literal so the failure reason has one source of truth.♻️ Proposed fix
-const capacityFailure = (): CreateAttendeeResult => ({ - reason: "capacity_exceeded", - success: false, -}); +const capacityFailure = (): Extract<CreateAttendeeResult, { success: false }> => ({ + reason: "capacity_exceeded", + success: false, +});if ( rawBookings.length === 0 || rawBookings.some((b) => (b.quantity ?? 1) < 0) || hasDuplicateBookingSlot(rawBookings) ) { - return { - failure: { reason: "capacity_exceeded", success: false }, - ok: false, - }; + return { failure: capacityFailure(), ok: false }; }Based on learnings, "Eliminate all code duplication. Extract shared helpers or curry differing inputs; do not evade jscpd or use
jscpd:ignoreexcept for import blocks and rare unavoidable boilerplate."Also applies to: 250-253
🤖 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 `@src/shared/db/attendees/create.ts` around lines 111 - 120, Replace the inline capacity-exceeded failure object in the validation condition with the existing capacityFailure() helper, matching the failure construction already used around capacityFailure(). Preserve the current early-return behavior and validation checks while keeping the failure reason defined in one place.Source: Learnings
test/shared/db/processed-payments.test.ts (1)
24-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
finalizeSessionwrapper across two test files. Both files independently re-implement an identical local wrapper aroundfinalizeTestPaymentSessionto default the payment reference topi_${sessionId};test/shared/merge/attendee-merge/repoint.test.tsshows the simpler pattern of importing the shared helper directly with no wrapper.
test/shared/db/processed-payments.test.ts#L24-L34: remove this local wrapper and import a shared default-reference helper (or callfinalizeTestPaymentSessiondirectly) instead.test/shared/db/processed-payments/staleness.test.ts#L17-L27: remove this identical local wrapper and use the same shared helper.Move the
pi_${sessionId}default-reference convenience into#test-utils/db-helpers/processed-payments.tsitself (e.g., as a documented default/overload offinalizeTestPaymentSession) so both call sites use one implementation.Based on learnings, "Eliminate all code duplication. Extract shared helpers or curry differing inputs; do not evade jscpd or use
jscpd:ignoreexcept for import blocks and rare unavoidable boilerplate."🤖 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 24 - 34, Remove the duplicated local finalizeSession wrappers in test/shared/db/processed-payments.test.ts (lines 24-34) and test/shared/db/processed-payments/staleness.test.ts (lines 17-27), then update both callers to use one shared implementation. Extend finalizeTestPaymentSession in `#test-utils/db-helpers/processed-payments.ts` with the pi_${sessionId} default-reference behavior, using a documented default or overload, and import/use that helper directly in both test files.Source: 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.
Inline comments:
In `@PR_SPLIT_PLAN.md`:
- Line 30: Update the line beginning with “#1827” in PR_SPLIT_PLAN.md to prefix
the reference with “PR”, preserving the surrounding prose and meaning.
In `@scripts/mutation/equivalent-mutants.txt`:
- Line 837: Remove the suppression entry for splitTokenBlob’s separatorAt === -1
mutant from equivalent-mutants.txt. Add a malformed-blob regression test
covering a separator-less line with a valid blind-index marker, asserting the
original branch preserves the marker and distinguishes it from the
truncated-marker mutant.
In `@src/features/admin/attendee-delete.ts`:
- Around line 18-22: Declare explicit return types for all listed exported
functions: annotate handleAdminAttendeeDeleteGet, handleAttendeeDelete, and
handleDeleteIncomplete in src/features/admin/attendee-delete.ts, plus
closeListingMidPayment, blockSessionPaymentLeg, unblockSessionPaymentLeg,
ageReservation, and stubSuccessfulRefund in
test/lib/server-payment-staging-helpers.ts. Use the existing shared
route-handler type for handleAdminAttendeeDeleteGet and types matching each
function’s current return behavior; no other logic changes are needed.
In `@src/features/admin/attendee-form-routes.ts`:
- Around line 390-393: Update the createAttendeeAtomic failure handling around
createResult so it switches on createResult.reason: keep capacity_exceeded
mapped to attendee_form.error_capacity, and map encryption_error to the
appropriate non-capacity encryption error message. Preserve the existing
successful path and avoid treating encryption failures as capacity errors.
In `@src/features/admin/ledger/entries.ts`:
- Around line 213-221: The pending-checkout validation around hasPendingCheckout
and postManualLedgerEntry is non-atomic, allowing a checkout stage to be
inserted between the read and ledger insert. Move this guard into the same
transaction or statement as postManualLedgerEntry, or enforce the invariant with
a database trigger/constraint, while preserving the existing attendee error
redirect behavior.
In `@src/features/api/payment-processing/create.ts`:
- Around line 351-356: Update the failure handling around honourFailure in
create payment processing so it uses the affected listing from result when that
listing is identified, rather than always using validatedItems[0]. When no
affected listing is provided, use the generic post-payment failure message;
apply the same logic to both capacity-failure branches.
In `@src/features/api/payment-processing/index.ts`:
- Line 431: Update the flow around processReservedSession to release the
reservation when recoverOrRefundUnexpectedCreate rethrows a transient staged
failure, allowing immediate redelivery to reserve the session; add an exact
regression test verifying the next delivery reserves successfully after the
throw.
In `@src/features/api/payment-processing/store-refund.ts`:
- Around line 399-402: Move the beginCheckoutStageRefund call in the
refund-processing flow so it executes before validateAllItems performs the
provider refund and before any irreversible provider I/O. Ensure the refund-only
state is persisted before validation/provider handling, while preserving the
existing local outcome processing afterward.
- Around line 140-147: Update the documentation for the shared refund-processing
function around the ledger posting and payment-reference stamping flow so it
accurately states that the money is posted first and the held charge’s payment
reference is stamped afterward. Keep the existing description of the ledger
failure behavior and retry semantics unchanged.
- Around line 95-168: Split the oversized modules into focused files, keeping
each under approximately 400 lines: extract placeholder facts,
stampStagedPaymentId, and recordHeldStagedMoney from
src/features/api/payment-processing/store-refund.ts lines 95-168; extract staged
failure and conflict routing from
src/features/api/payment-processing/store-refund.ts lines 356-499; extract
request-maintenance and Stripe reconciliation scheduling from
src/features/index.ts lines 623-677; separate checkout orchestration from
free-reservation persistence in src/features/public/ticket-payment.ts lines
55-495; and split success, rollback/failure, and refusal-classification tests in
test/shared/db/attendees/activate.test.ts lines 1-483. Update imports and
exports so behavior remains unchanged.
In `@src/features/api/webhooks.ts`:
- Around line 250-255: The optional provider cleanup in the cancellation flow
must not propagate rejection after local sessions are discarded. Wrap
provider.expireCheckoutSession in best-effort error handling, log any failure,
and continue returning the cancellation page; add a regression test covering a
rejecting expiration call.
In `@src/features/index.ts`:
- Around line 672-676: The stale Stripe webhook reconciliation path around
reconcileStripeWebhook() must not use loadEffectiveDomain() when it can fall
back to requestUrl.hostname. Require a validated custom domain or configured
Bunny subdomain as the canonical domain before invoking reconciliation, and fail
closed without persisting a webhook URL when neither is available.
In `@src/features/public/ticket-payment.ts`:
- Line 55: Split the checkout orchestration and free-reservation persistence
responsibilities from the current module into focused files, keeping each under
approximately 400 lines. Move the logic referenced by the checkout flow and the
free-reservation sections into their respective modules, then update imports and
callers so the existing behavior remains unchanged.
In `@src/shared/attendee-table-rows.ts`:
- Around line 47-60: Hoist construction of the live listing ID set out of the
per-attendee grouping loop so it is created once per grouping operation. Update
the code around liveIds and the surrounding listing assembly to reuse that set
for every group while preserving the existing filtering and deleted-listing
behavior.
In `@src/shared/db/attendees/activate.ts`:
- Around line 175-186: Extract the inline activation input intersection and the
activation result union from activateStagedBooking into named, reusable types,
such as StagedActivationInput and StagedActivationResult. Update the function
signature to use those named types while preserving the existing fields and
success/failure behavior.
In `@src/shared/db/attendees/create-batch.ts`:
- Around line 35-43: Define and export a named finalize payload type in
src/shared/db/attendees/create-batch.ts, then use it for both
BookingBatchPlan.finalize and FinalizedBookingBatchPlan.finalize. In
src/shared/checkout-complete.ts, import and use the same type for the finalize
parameter instead of repeating the anonymous object shape.
In `@src/shared/db/attendees/queries.ts`:
- Around line 393-410: Update listingHoldsUnreturnedCash to perform the guard
check in one queryAllPrimary call instead of first loading attendee IDs and then
calling attendeeIdsHoldingUnreturnedCash. Use a single EXISTS-style statement
that joins listing_attendees for the given listing to the same unreturned-cash
balance conditions used by attendeeIdsHoldingUnreturnedCash, and return the
resulting boolean while preserving primary-pinned reads.
In `@src/shared/db/backup-snapshot.ts`:
- Around line 128-134: The checkoutStageRevision function must fail when the
expected revision row or revision value is missing instead of converting it to
0. Remove the optional/default fallback and explicitly validate the query
result, throwing an error so captureBackup cannot certify consistency from an
invalid revision state.
In `@src/shared/db/backup.ts`:
- Around line 143-181: Update readManifest so it continues returning null only
when manifest.json is absent, but throws when the present manifest fails
BackupManifestSchema validation by using the validation error path instead of
silently returning null. Preserve validateArchiveFiles and restoreFromZip
behavior for genuinely missing manifests, while ensuring corrupted manifests
cannot bypass table-file and row-count integrity checks.
In `@src/shared/db/checkout-stage-state.ts`:
- Around line 1-18: Derive OPEN_CHECKOUT_STAGE_SQL from a single shared
open-state collection used by isOpenCheckoutStage, replacing the duplicated
"pending" and "refunding" literals while preserving the existing predicate and
SQL membership behavior. Keep the shared source typed against CheckoutStageState
and format its values into the SQL expression without introducing another
independently maintained list.
In `@src/shared/db/checkout-stages.ts`:
- Around line 192-194: Annotate the exported stagedSessionCreator factory and
attendeeIdsWithPendingStage with explicit return types, including the curried
function signature for stagedSessionCreator. Preserve their existing behavior
and inferred parameter types while making each exported function’s complete
return contract explicit.
- Around line 141-167: Handle the possible failure result from
createAttendeeAtomic in the checkout flow instead of casting it to
CreateAttendeeSuccess. Check for the encryption_error or other failure response
before accessing result.attendees[0], and propagate or return that failure
through the existing checkout error path; only read the attendee ID after
confirming success.
In `@src/shared/db/client.ts`:
- Around line 355-371: Move the multi-line JSDoc describing buildSql, empty
inputs, and primary reads so it directly documents matchingIdSet. Keep the
separate idSetFromRows comment above that helper, and do not alter either
implementation.
- Around line 368-371: Move the JSDoc block currently preceding idSetFromRows so
it directly precedes the matchingIdSet declaration, ensuring the documentation
describes the correct exported helper without changing its implementation.
In `@src/shared/db/migrations.ts`:
- Around line 287-291: Make the schema rebuild atomic by creating one
transaction-scoped database handle and passing it through both the executeBatch
step and the executeMultiple trigger-rebuild step before sealFreshSchema().
Update the relevant migration flow and helper signatures to reuse that same
handle instead of calling executeBatch and getDb().executeMultiple through
separate clients, while preserving the existing SQL and ordering.
In `@src/shared/db/migrations/schema/checkout-stage-triggers.ts`:
- Around line 42-55: Extend the trigger definition for processed_payments beyond
the insert-only guard by adding equivalent validation for updates that change
payment_session_id or checkout_stage_attendee_id. Preserve the existing mismatch
condition and abort message, and add a regression test covering both claim-field
update paths.
In `@src/shared/db/modifier-usage.ts`:
- Around line 133-145: Update the stock validation query in the modifier usage
flow to group json_each(?) requests by modifierId and compare each modifier’s
remaining stock against the summed requested quantity, so duplicate requests are
evaluated together. Preserve the existing sold_out result contract and add a
regression test covering 5 remaining stock with two requests of 3, which must be
rejected.
In `@src/shared/settings/registry.ts`:
- Around line 146-151: Replace the duplicated setting declaration for
lastPrunedCheckoutStages with pruneSetting("lastPrunedCheckoutStages",
CONFIG_KEYS.LAST_PRUNED_CHECKOUT_STAGES), preserving the existing accessor name
and configuration key while reusing the shared helper.
In `@src/shared/stripe-provider.ts`:
- Line 24: Update the export or object definition around expireCheckoutSession
to use property shorthand instead of a redundant alias, preserving the existing
direct forwarding behavior.
In `@src/shared/stripe.ts`:
- Around line 190-203: Update createStripeWebhookEndpoint so that when the
created endpoint has no secret, it first deletes the newly created endpoint
using its identifier, then throws the existing error. Preserve the successful
return path and ensure cleanup occurs only for the missing-secret case.
In `@test/lib/attendee-table.test.ts`:
- Around line 502-507: Strengthen the payment-progress test around the status
and ticket indicator cells so it independently verifies both display “Payment in
progress,” rather than relying on a single page-wide toContain assertion. Target
the relevant cell selectors or assert the expected occurrence count, while
preserving the existing “No quantity” and disallowed-link checks.
In `@test/lib/payment-processing/stage-gone.test.ts`:
- Around line 38-99: Update the stage-gone test setup around createTestListing,
stageCheckout, and deleteAttendee so it preserves checkout_stages while removing
the booking rows, allowing the intended “has no booking rows at activation”
assertion; alternatively, if using deleteAttendee remains necessary, change the
expectation to the earlier “was not this attendee's pending stage at activation”
error.
In `@test/lib/server-attendees/delete.test.ts`:
- Around line 215-239: Replace the inline dynamic import of getAttendeeRaw in
the test file with a top-level static import from the existing shared attendees
queries module, then reuse that imported symbol in both repeated test locations.
Keep the test assertions and behavior unchanged.
In `@test/lib/server-attendees/helpers.ts`:
- Line 1: Replace the repeated inline dynamic imports with static top-level
imports throughout the affected test helpers and tests. In the attendee helpers,
statically import deleteListing, markCheckoutStage, and loadExistingLines from
their existing modules; in the attendee deletion tests, statically import
getAttendeeRaw; in the ledger entry test, import stageMidPaymentAttendee from
the helpers; and in the listings deletion test, import postHeldPayment from the
ledger utilities. Preserve existing behavior and reuse the established import
sources.
- Around line 64-70: Hoist the dynamic imports for deleteListing,
markCheckoutStage, and loadExistingLines into top-level static imports in the
helper module. Reuse the existing static checkout-stages import for
markCheckoutStage alongside stageCheckout, and remove the corresponding
in-function import statements while preserving call order and behavior.
In `@test/lib/server-ledger/add-entry.test.ts`:
- Around line 97-121: Replace the dynamic import of stageMidPaymentAttendee
inside the “refuses a manual attendee entry while a checkout is pending” test
with a static top-level import, matching the existing import style used by other
tests. Remove the in-test import while preserving the helper invocation and test
behavior.
In `@test/lib/server-listings/delete.test.ts`:
- Around line 137-169: Replace the dynamic import of postHeldPayment in the
“refuses to delete a listing whose attendee holds unreturned cash” test with a
static top-level import from `#test-utils/ledger.ts`, matching the existing
imports in the related attendee deletion tests. Keep the test setup and payment
invocation unchanged.
In `@test/lib/server-payment-staging-recovery.test.ts`:
- Around line 137-141: Replace the duplicated filler booking setup in both
capacity-loss blocks with the existing fillListing helper: import fillListing
and call fillListing(listing), preserving the existing success/error behavior
and removing the direct bookAttendee calls.
- Around line 42-519: Split the oversized describeWithEnv recovery suite into
focused test files, keeping related cases together: expiry/healing, refund retry
flows, and ledger or invariant failures. Preserve each test’s setup, teardown,
helpers, assertions, and behavior while ensuring every resulting test file stays
under approximately 400 lines.
In `@test/shared/db/listing-overview-stats.test.ts`:
- Line 27: Remove the local compatibility aliases and import
finalizeTestPaymentSession directly in
test/shared/db/listing-overview-stats.test.ts:27-27 and
test/shared/db/listings/delete.test.ts:42-42; update all usages of
finalizeSession and finalizePaymentSession in those tests to call
finalizeTestPaymentSession instead.
In `@test/shared/db/listings/delete.test.ts`:
- Around line 262-295: Extend the test around pending and booked checkout
deletion to assert that the pending checkout stage and its associated
booking/session metadata remain recoverable after deleteListing, using the
existing checkout lookup or recovery helpers. Keep the current listing_attendees
row-count assertions and confirm the booked checkout retains its existing
cascade behavior.
In `@test/shared/db/processed-payments.test.ts`:
- Around line 356-358: Strengthen the “is a no-op if the session was pruned”
test by asserting observable database state after calling
finalizeSessionIfUnresolved for “sess_gone”. Verify the pruned session remains
absent and unchanged, using the existing test database/query helpers and clear
assertion failure semantics.
In `@test/shared/limits.test.ts`:
- Line 171: Update the relevant limits-value test near the key-set assertions to
explicitly assert that LIMIT_ENTRIES contains CHECKOUT_SESSION_EXPIRY_MINUTES
and PRUNE_CHECKOUT_STAGES_RETENTION_DAYS with their current expected values,
rather than only verifying the keys exist. Use strong value assertions that
would fail if either debug-page limit becomes stale or incorrect.
In `@test/test-utils/db-helpers/processed-payments.ts`:
- Around line 7-23: Add an explicit return type annotation to the exported
stageTestCheckout function, matching the return type of the stageCheckout call
it returns. Keep its existing parameters and checkout construction unchanged.
---
Outside diff comments:
In `@src/shared/db/attendees/create.ts`:
- Around line 111-120: Replace the inline capacity-exceeded failure object in
the validation condition with the existing capacityFailure() helper, matching
the failure construction already used around capacityFailure(). Preserve the
current early-return behavior and validation checks while keeping the failure
reason defined in one place.
In `@src/shared/db/attendees/delete.ts`:
- Around line 54-79: Update attendeeDependentDeleteStatements to locate the
checkout_stages delete by its table name rather than relying on
DEPENDENT_ROW_TARGETS ordering. Insert beforeCheckoutStage immediately before
that identified statement, while preserving all other dependent-delete ordering
and behavior.
In `@src/shared/stripe.ts`:
- Around line 285-300: The endpoint cleanup in the endpoint-limit recovery path
must not delete existingEndpointId before the replacement is created
successfully. Update the catch block around createStripeWebhookEndpoint to
perform the retry first, then call deleteEndpointsBestEffort for
existingEndpointId only after that retry succeeds, preserving the recorded
endpoint if creation fails.
In `@test/shared/db/processed-payments.test.ts`:
- Around line 24-34: Remove the duplicated local finalizeSession wrappers in
test/shared/db/processed-payments.test.ts (lines 24-34) and
test/shared/db/processed-payments/staleness.test.ts (lines 17-27), then update
both callers to use one shared implementation. Extend finalizeTestPaymentSession
in `#test-utils/db-helpers/processed-payments.ts` with the pi_${sessionId}
default-reference behavior, using a documented default or overload, and
import/use that helper directly in both test files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e057fc84-ccd4-408b-aa10-cbc35df0d839
📒 Files selected for processing (229)
AGENTS.mdPR_SPLIT_PLAN.mdTODO.mdplan.mdreview.mdscripts/mutation/equivalent-mutants.txtsrc/docs/database.tssrc/features/admin/api.tssrc/features/admin/attendee-action-state.tssrc/features/admin/attendee-delete.tssrc/features/admin/attendee-form-model.tssrc/features/admin/attendee-form-routes.tssrc/features/admin/attendee-logistics-routes.tssrc/features/admin/attendee-page-data.tssrc/features/admin/attendee-page.tssrc/features/admin/attendees-csv.tssrc/features/admin/attendees-merge.tssrc/features/admin/attendees-route-helpers.tssrc/features/admin/attendees.tssrc/features/admin/backup.tssrc/features/admin/built-sites.tssrc/features/admin/calendar-csv.tssrc/features/admin/ledger/entries.tssrc/features/admin/listings-lifecycle.tssrc/features/admin/settings-stripe.tssrc/features/admin/update.tssrc/features/api/folded-booking.tssrc/features/api/payment-processing/classify.tssrc/features/api/payment-processing/committed-entries.tssrc/features/api/payment-processing/completion.tssrc/features/api/payment-processing/create.tssrc/features/api/payment-processing/index.tssrc/features/api/payment-processing/recovery-decision.tssrc/features/api/payment-processing/recovery.tssrc/features/api/payment-processing/refunds.tssrc/features/api/payment-processing/store-refund.tssrc/features/api/webhooks.tssrc/features/index.tssrc/features/public/qr-book.tssrc/features/public/ticket-payment.tssrc/features/settings-bundles.tssrc/features/tickets/token-utils.tssrc/locales/en/admin.jsonsrc/locales/en/attendees.jsonsrc/locales/en/csv.jsonsrc/locales/en/public.jsonsrc/shared/accounting/rows.tssrc/shared/attendee-table-rows.tssrc/shared/booking-lines.tssrc/shared/booking.tssrc/shared/checkout-complete.tssrc/shared/columns/attendee-columns.tssrc/shared/db/attendee-types.tssrc/shared/db/attendees/activate.tssrc/shared/db/attendees/activation-refusal.tssrc/shared/db/attendees/api.tssrc/shared/db/attendees/atomic-update.tssrc/shared/db/attendees/capacity/checks.tssrc/shared/db/attendees/capacity/range.tssrc/shared/db/attendees/create-batch.tssrc/shared/db/attendees/create.tssrc/shared/db/attendees/delete.tssrc/shared/db/attendees/order-parents.tssrc/shared/db/attendees/pii.tssrc/shared/db/attendees/queries.tssrc/shared/db/attendees/select.tssrc/shared/db/attendees/servicing.tssrc/shared/db/backup-snapshot.tssrc/shared/db/backup-storage.tssrc/shared/db/backup.tssrc/shared/db/capacity.tssrc/shared/db/checkout-stage-cleanup.tssrc/shared/db/checkout-stage-state.tssrc/shared/db/checkout-stages.tssrc/shared/db/client.tssrc/shared/db/contact-preferences.tssrc/shared/db/contact-tokens.tssrc/shared/db/groups.tssrc/shared/db/listing-prices.tssrc/shared/db/listings/delete.tssrc/shared/db/migrations.tssrc/shared/db/migrations/2026-07-12_checkout_stages.tssrc/shared/db/migrations/registry.tssrc/shared/db/migrations/schema-sync.tssrc/shared/db/migrations/schema/checkout-stage-triggers.tssrc/shared/db/migrations/schema/tables-attendees.tssrc/shared/db/migrations/schema/triggers.tssrc/shared/db/migrations/schema/types.tssrc/shared/db/migrations/schema/version.tssrc/shared/db/modifier-usage.tssrc/shared/db/orphan-attendees.tssrc/shared/db/payment-finalize.tssrc/shared/db/processed-payments.tssrc/shared/db/prune.tssrc/shared/db/settings.tssrc/shared/limits.tssrc/shared/listings-actions.tssrc/shared/payments.tssrc/shared/refund-ledger.tssrc/shared/seeds.tssrc/shared/settings/keys.tssrc/shared/settings/registry.tssrc/shared/stripe-provider.tssrc/shared/stripe-webhook-events.tssrc/shared/stripe-webhook-reconcile.tssrc/shared/stripe.tssrc/shared/types.tssrc/ui/templates/admin/attendee-detail.tsxsrc/ui/templates/admin/attendee-form.tsxsrc/ui/templates/admin/attendee-notes.tsxsrc/ui/templates/admin/attendee-page.tsxsrc/ui/templates/admin/attendees.tsxsrc/ui/templates/attendee-table.tsxtest/features/admin/attendee-form-deleted-listing.test.tstest/features/admin/attendee-form-fixtures.tstest/features/admin/attendee-form-model.test.tstest/features/admin/attendees-csv.test.tstest/features/public/ticket-payment.test.tstest/integration/servicing/capacity.test.tstest/lib/attendee-table.test.tstest/lib/code-quality.test.tstest/lib/db/attendees/availability-consistency.test.tstest/lib/db/attendees/create-attendee-atomic.test.tstest/lib/db/attendees/delete-attendee.test.tstest/lib/db/attendees/select.test.tstest/lib/db/migration-schema-guard.test.tstest/lib/payment-processing/stage-gone.test.tstest/lib/processed-payments/locking.test.tstest/lib/server-attendee-form/quantity.test.tstest/lib/server-attendee-logistics-tab.test.tstest/lib/server-attendee-refresh-payment.test.tstest/lib/server-attendees-list.test.tstest/lib/server-attendees/attendee-detail.test.tstest/lib/server-attendees/attendee-edit.test.tstest/lib/server-attendees/delete-incomplete.test.tstest/lib/server-attendees/delete.test.tstest/lib/server-attendees/deleted-listing.test.tstest/lib/server-attendees/helpers.tstest/lib/server-attendees/merge-panel.test.tstest/lib/server-attendees/merge-post.test.tstest/lib/server-backup.test.tstest/lib/server-balance-payment-replay.test.tstest/lib/server-balance-webhook.test.tstest/lib/server-built-sites-update.test.tstest/lib/server-bulk-email/notes-and-history.test.tstest/lib/server-ledger/add-entry.test.tstest/lib/server-listings/delete.test.tstest/lib/server-payment-staging-helpers.tstest/lib/server-payment-staging-recovery.test.tstest/lib/server-payment-staging-refund-rail.test.tstest/lib/server-payment-staging-rollback.test.tstest/lib/server-payment-staging.test.tstest/lib/server-payments-success-replay.test.tstest/lib/server-payments/cancel.test.tstest/lib/server-payments/confirm.test.tstest/lib/server-payments/replay.test.tstest/lib/server-privacy.test.tstest/lib/server-public/ticket-additional-coverage.test.tstest/lib/server-qr-book.test.tstest/lib/server-refunds-balance-payments.test.tstest/lib/server-reservation/helpers.tstest/lib/server-reservation/public-default-modifiers.test.tstest/lib/server-scheduled.test.tstest/lib/server-settings/stripe.test.tstest/lib/server-update.test.tstest/lib/server-webhooks/already-processed-rollback.test.tstest/lib/server-webhooks/concurrent-processing.test.tstest/lib/server-webhooks/custom-questions-multi.test.tstest/lib/server-webhooks/modifier-refunds.test.tstest/lib/server-webhooks/multi-ticket-refunds.test.tstest/lib/server-webhooks/refund-helper-functions.test.tstest/lib/server-webhooks/registration-closed.test.tstest/lib/stripe-mock/ports.test.tstest/lib/stripe/core.test.tstest/lib/stripe/webhook-reconcile.test.tstest/lib/stripe/webhook-setup.test.tstest/lib/test-utils/factories.test.tstest/lib/webhook-price-signature/helpers.tstest/lib/webhook-price-signature/post-commit.test.tstest/lib/webhook-price-signature/recovery-decision.test.tstest/lib/webhook-price-signature/stored-refund-and-ignore.test.tstest/lib/webhook-price-signature/trusted-and-mismatch.test.tstest/routes/unsubscribe.test.tstest/shared/accounting/rows.test.tstest/shared/columns/attendee-columns.test.tstest/shared/db/attendees/activate-large-cart.test.tstest/shared/db/attendees/activate.test.tstest/shared/db/attendees/create-errors.test.tstest/shared/db/attendees/create.test.tstest/shared/db/attendees/pii.test.tstest/shared/db/attendees/servicing/editing.test.tstest/shared/db/backup.test.tstest/shared/db/backup/checkout-stage-snapshot.test.tstest/shared/db/backup/restore.test.tstest/shared/db/capacity.test.tstest/shared/db/checkout-stage-fences.test.tstest/shared/db/checkout-stages.test.tstest/shared/db/client.test.tstest/shared/db/contact-preferences.test.tstest/shared/db/contact-token-activity.test.tstest/shared/db/contact-tokens.test.tstest/shared/db/listing-overview-stats.test.tstest/shared/db/listings/delete.test.tstest/shared/db/migrations/trigger-dependencies.test.tstest/shared/db/modifier-resolve.test.tstest/shared/db/modifier-usage.test.tstest/shared/db/orphan-attendees.test.tstest/shared/db/payment-references.test.tstest/shared/db/processed-payments.test.tstest/shared/db/processed-payments/staleness.test.tstest/shared/db/prune/helpers.tstest/shared/db/prune/scheduler.test.tstest/shared/db/prune/tables.test.tstest/shared/limits.test.tstest/shared/merge/attendee-merge/repoint.test.tstest/shared/refund-ledger.test.tstest/shared/settings/registry.test.tstest/shared/slug.test.tstest/shared/sumup-provider.test.tstest/test-utils/db-helpers/attendees.tstest/test-utils/db-helpers/contacts.tstest/test-utils/db-helpers/processed-payments.tstest/test-utils/factories.tstest/test-utils/ledger.tstest/test-utils/settings.tstest/ui/templates/admin/attendee-detail.test.tstest/ui/templates/admin/attendee-notes.test.tsxtest/ui/templates/admin/attendee-page.test.tstest/ui/templates/admin/dashboard.test.ts
💤 Files with no reviewable changes (5)
- src/shared/db/contact-preferences.ts
- test/ui/templates/admin/attendee-notes.test.tsx
- test/lib/code-quality.test.ts
- test/features/public/ticket-payment.test.ts
- src/ui/templates/admin/attendee-notes.tsx
| - #1824: Stripe refund status correctness. | ||
| - #1826: owner-safe links in attendee notes. | ||
|
|
||
| #1827, Stripe webhook setup hardening, is the final prerequisite. At the latest |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the malformed PR reference.
Line 30 starts with #1827, triggering MD018. Prefix it with PR so it remains prose rather than heading-like Markdown.
Proposed fix
-#1827, Stripe webhook setup hardening, is the final prerequisite.
+PR `#1827`, Stripe webhook setup hardening, is the final prerequisite.📝 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.
| #1827, Stripe webhook setup hardening, is the final prerequisite. At the latest | |
| PR `#1827`, Stripe webhook setup hardening, is the final prerequisite. At the latest |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 30-30: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 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 `@PR_SPLIT_PLAN.md` at line 30, Update the line beginning with “#1827” in
PR_SPLIT_PLAN.md to prefix the reference with “PR”, preserving the surrounding
prose and meaning.
Source: Linters/SAST tools
| src/shared/db/contact-tokens.ts:177:35 ?? → || # loadTokenBlob's `row?.attendee_tokens_blob ?? null`: attendee_tokens_blob is string|undefined; the only falsy-non-null is "" and tokenLinesFrom("") === tokenLinesFrom(null) === [], so ?? and || agree | ||
| src/shared/db/contact-tokens.ts:218:22 return null → return undefined # removeBookingToken's no-match return; removedSource is only ever consumed by `removedSource ?? sync.source`, and null and undefined agree under ?? | ||
| src/shared/db/contact-tokens.ts:289:18 ?? → || # `removedSource ?? sync.source`: removedSource is BookingSource|null, and both BookingSource values ("admin","public") are non-empty truthy strings, so it is never falsy-but-non-null; ?? and || agree | ||
| src/shared/db/contact-tokens.ts:97:30 - → + # splitTokenBlob's `separatorAt === -1`: every app-written line has a tab at the marker-length position (64), and a malformed separator-less line's first-chars marker can never equal a real BlindIndex (an hmac hex), and its ciphertext never decrypts, so taking the else-branch for a tab-less line (marker = raw.slice(0, -1)) is unobservable through ensureBookingToken/removeBookingToken/getRecentBookingTokens |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the non-equivalent splitTokenBlob suppression.
The proof covers only app-written rows. A restored or corrupted separator-less line containing a valid blind-index marker distinguishes the original branch from the mutant’s truncated marker before decryption. Remove this entry and add a malformed-blob 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 `@scripts/mutation/equivalent-mutants.txt` at line 837, Remove the suppression
entry for splitTokenBlob’s separatorAt === -1 mutant from
equivalent-mutants.txt. Add a malformed-blob regression test covering a
separator-less line with a valid blind-index marker, asserting the original
branch preserves the marker and distinguishes it from the truncated-marker
mutant.
| export const handleAdminAttendeeDeleteGet = attendeeRecordActionPage( | ||
| adminDeleteAttendeePage, | ||
| async ({ attendee }) => | ||
| (await loadAttendeeActionState(attendee.id)).canDelete, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Declare explicit types for every exported function value.
These exports currently rely on inferred types from factories or expressions.
src/features/admin/attendee-delete.ts#L18-L22: annotatehandleAdminAttendeeDeleteGetwith a shared route-handler type.src/features/admin/attendee-delete.ts#L61-L76: annotatehandleAttendeeDelete.src/features/admin/attendee-delete.ts#L79-L103: annotatehandleDeleteIncomplete.test/lib/server-payment-staging-helpers.ts#L81-L84: add a return type tocloseListingMidPayment.test/lib/server-payment-staging-helpers.ts#L88-L99: add a return type toblockSessionPaymentLeg.test/lib/server-payment-staging-helpers.ts#L102-L116: add return types tounblockSessionPaymentLegandageReservation.test/lib/server-payment-staging-helpers.ts#L128-L133: add a return type tostubSuccessfulRefund.
As per coding guidelines, every exported or public function requires an explicit return type.
📍 Affects 2 files
src/features/admin/attendee-delete.ts#L18-L22(this comment)src/features/admin/attendee-delete.ts#L61-L76src/features/admin/attendee-delete.ts#L79-L103test/lib/server-payment-staging-helpers.ts#L81-L84test/lib/server-payment-staging-helpers.ts#L88-L99test/lib/server-payment-staging-helpers.ts#L102-L116test/lib/server-payment-staging-helpers.ts#L128-L133
🤖 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 `@src/features/admin/attendee-delete.ts` around lines 18 - 22, Declare explicit
return types for all listed exported functions: annotate
handleAdminAttendeeDeleteGet, handleAttendeeDelete, and handleDeleteIncomplete
in src/features/admin/attendee-delete.ts, plus closeListingMidPayment,
blockSessionPaymentLeg, unblockSessionPaymentLeg, ageReservation, and
stubSuccessfulRefund in test/lib/server-payment-staging-helpers.ts. Use the
existing shared route-handler type for handleAdminAttendeeDeleteGet and types
matching each function’s current return behavior; no other logic changes are
needed.
Source: Coding guidelines
| if (!createResult.success) { | ||
| return { ok: false, saveError: t("attendee_form.error_capacity") }; | ||
| } | ||
| const { attendees } = createResult as Extract< | ||
| CreateAttendeeResult, | ||
| { success: true } | ||
| >; | ||
| const { attendees } = createResult; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What failure reasons can Deno's createAttendeeAtomic-style batch booking creation report in the chobbledotcom/tickets repo — is it capacity-only or does it have other reasons?
💡 Result:
In the chobbledotcom/tickets repository, the createAttendeeAtomic-style operation is used to ensure booking consistency, particularly by combining capacity checks and booking insertion into a single atomic transaction [1]. This operation is not limited solely to capacity failures; it encompasses broader data validation and system state requirements to maintain integrity [2]. Failure reasons for booking creation flows in this repository include: - Capacity and Availability: The primary purpose of the atomic check is to verify that the event has available spots. If the event is full or the requested quantity exceeds remaining capacity, the operation fails, and for paid bookings, any associated payment is automatically refunded [3][1]. - Date Validation: The booking flow explicitly validates requested dates. Invalid booking dates (such as missing or non-string values where a string is expected) trigger rejection via specific error handling, such as bookingError.invalidDate [2]. - System State: The system checks for environmental and configuration states. For example, in read-only mode (which occurs after a paid subscription period ends), the system prevents new bookings, and actions that cannot be performed are hidden or rejected [3][2]. - General Integrity: As a best-effort safeguard, the atomic operation protects against race conditions where multiple users attempt to secure the last remaining ticket simultaneously. Because it utilizes Deno's atomic transaction model, it also inherently protects against failures caused by conflicting concurrent mutations (optimistic concurrency control), where the state changes between the read and the commit [4][5][1]. While "createAttendeeAtomic" specifically refers to the atomic database commit pattern [6][1], the broader booking creation flow integrates these multiple layers of validation to ensure that bookings are only created when all criteria—date, availability, and account status—are met [2].
Citations:
- 1: https://chobble.com/tickets/
- 2: Make the duplicate-code check stricter and remove the duplication it finds #1795
- 3: https://tickets.chobble.com/faq/
- 4: https://docs.deno.com/api/deno/~/Deno.AtomicOperation
- 5: https://docs.deno.com/deploy/kv/transactions/
- 6: https://chobbledotcom.github.io/tickets/doc.ts/index.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and helper definitions
git ls-files 'src/features/admin/attendee-form-routes.ts' 'src/**' | sed -n '1,200p'
rg -n "createAttendeeAtomic|no_lines|error_capacity|applyEdit|applyCreate|reason ===" src/features/admin/attendee-form-routes.ts src -S
# Show the relevant slice of the target file
wc -l src/features/admin/attendee-form-routes.ts
sed -n '340,520p' src/features/admin/attendee-form-routes.tsRepository: chobbledotcom/tickets
Length of output: 20596
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared create result types and failure mapping.
ast-grep outline src/shared/db/attendees/create.ts --view expanded
ast-grep outline src/shared/db/attendees/atomic-update.ts --view expanded
ast-grep outline src/shared/capacity-error.ts --view expanded
printf '\n--- create.ts relevant slice ---\n'
sed -n '1,220p' src/shared/db/attendees/create.ts
printf '\n--- atomic-update.ts relevant slice ---\n'
sed -n '1,360p' src/shared/db/attendees/atomic-update.ts
printf '\n--- capacity-error.ts ---\n'
sed -n '1,200p' src/shared/capacity-error.tsRepository: chobbledotcom/tickets
Length of output: 25141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the public create result type and all failure reason handling sites.
ast-grep outline src/shared/db/attendee-types.ts --view expanded
rg -n "type CreateAttendeeResult|CreateAttendeeResult|reason: \"encryption_error\"|reason: \"capacity_exceeded\"|reason: \"sold-out\"|capacityErrorFormatter|error_capacity" src/shared/db/attendee-types.ts src/shared src/features -S
printf '\n--- attendee-types.ts relevant slice ---\n'
sed -n '1,260p' src/shared/db/attendee-types.ts
printf '\n--- create.ts lower slice ---\n'
sed -n '220,340p' src/shared/db/attendees/create.ts
printf '\n--- callers around error mapping ---\n'
sed -n '140,220p' src/features/admin/attendees.ts
sed -n '1,220p' src/shared/capacity-error.tsRepository: chobbledotcom/tickets
Length of output: 20507
Handle encryption_error separately.
createAttendeeAtomic can fail with encryption_error as well as capacity_exceeded, but this branch turns both into attendee_form.error_capacity. no_lines is already filtered before the call, so switch on result.reason here and surface a non-capacity message for encryption failures.
🤖 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 `@src/features/admin/attendee-form-routes.ts` around lines 390 - 393, Update
the createAttendeeAtomic failure handling around createResult so it switches on
createResult.reason: keep capacity_exceeded mapped to
attendee_form.error_capacity, and map encryption_error to the appropriate
non-capacity encryption error message. Preserve the existing successful path and
avoid treating encryption failures as capacity errors.
| if ( | ||
| loaded.account.type === ATTENDEE && | ||
| (await hasPendingCheckout(Number(loaded.account.id))) | ||
| ) { | ||
| return errorRedirect( | ||
| redirectUrl, | ||
| t("attendee_form.error_pending_checkout"), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and symbols
git ls-files 'src/features/admin/ledger/entries.ts' 'src/**' | sed -n '1,120p'
echo '--- hasPendingCheckout / postManualLedgerEntry references ---'
rg -n "hasPendingCheckout|postManualLedgerEntry" src -g '!**/node_modules/**'
echo '--- entries.ts around target lines ---'
nl -ba src/features/admin/ledger/entries.ts | sed -n '180,260p'Repository: chobbledotcom/tickets
Length of output: 5790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- entries.ts target section ---'
sed -n '190,245p' src/features/admin/ledger/entries.ts
echo '--- checkout-stages.ts hasPendingCheckout ---'
sed -n '240,330p' src/shared/db/checkout-stages.ts
echo '--- manual-entries.ts postManualLedgerEntry ---'
sed -n '180,280p' src/shared/accounting/manual-entries.ts
echo '--- transaction helpers / db wrappers mentions ---'
rg -n "transaction|tx|withTransaction|db\.transaction|BEGIN|COMMIT|rollback|serializable|lock" src/shared src/features/admin/ledger -g '!**/node_modules/**'Repository: chobbledotcom/tickets
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- postTransfers / ledger transaction helpers ---'
rg -n "export const postTransfers|function postTransfers|const postTransfers|ledgerTx|withTransaction|inOwnTx" src/shared/accounting src/shared/db -g '!**/node_modules/**'
echo '--- manual-entries.ts top/middle for imports and helper wiring ---'
sed -n '1,240p' src/shared/accounting/manual-entries.ts
echo '--- search schema/migrations for checkout_stages or manual ledger constraints ---'
rg -n "checkout_stages|manual ledger|manual_ledger|ledger entry|transfers" src/shared/db src/shared/accounting src -g '!**/node_modules/**' | sed -n '1,220p'Repository: chobbledotcom/tickets
Length of output: 37028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- store.ts ---'
sed -n '1,240p' src/shared/accounting/store.ts
echo '--- attendees table schema around checkout_stages ---'
sed -n '150,240p' src/shared/db/migrations/schema/tables-attendees.ts
echo '--- global triggers schema excerpt ---'
sed -n '1,220p' src/shared/db/migrations/schema/triggers.tsRepository: chobbledotcom/tickets
Length of output: 21169
Make the pending-checkout guard atomic. hasPendingCheckout() runs as a separate read before postManualLedgerEntry(), so a stage can appear in between and the manual ledger row still lands. Fold the check into the same transaction/statement as the insert, or enforce it with a trigger/constraint.
🤖 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 `@src/features/admin/ledger/entries.ts` around lines 213 - 221, The
pending-checkout validation around hasPendingCheckout and postManualLedgerEntry
is non-atomic, allowing a checkout stage to be inserted between the read and
ledger insert. Move this guard into the same transaction or statement as
postManualLedgerEntry, or enforce the invariant with a database
trigger/constraint, while preserving the existing attendee error redirect
behavior.
| createPaidTestAttendee, | ||
| } from "#test-utils/db-helpers/attendee-payments.ts"; | ||
| import { createTestListing } from "#test-utils/db-helpers/listings.ts"; | ||
| import { finalizeTestPaymentSession as finalizeSession } from "#test-utils/db-helpers/processed-payments.ts"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the local compatibility aliases for the replacement test helper.
Both tests should import finalizeTestPaymentSession directly and update their call sites instead of retaining legacy/local aliases.
test/shared/db/listing-overview-stats.test.ts#L27-L27: removeas finalizeSessionand migrate its usages.test/shared/db/listings/delete.test.ts#L42-L42: removeas finalizePaymentSessionand migrate its usages.
As per coding guidelines, internal compatibility aliases should be removed when replacing an internal API.
📍 Affects 2 files
test/shared/db/listing-overview-stats.test.ts#L27-L27(this comment)test/shared/db/listings/delete.test.ts#L42-L42
🤖 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/listing-overview-stats.test.ts` at line 27, Remove the local
compatibility aliases and import finalizeTestPaymentSession directly in
test/shared/db/listing-overview-stats.test.ts:27-27 and
test/shared/db/listings/delete.test.ts:42-42; update all usages of
finalizeSession and finalizePaymentSession in those tests to call
finalizeTestPaymentSession instead.
Source: Coding guidelines
| test("keeps a pending checkout's rows, but cascades a resolved one's", async () => { | ||
| const listing = await createTestListing({ maxAttendees: 50 }); | ||
| const intent = checkoutIntent({ | ||
| items: [ | ||
| checkoutItem({ | ||
| listingId: listing.id, | ||
| name: listing.name, | ||
| slug: listing.slug, | ||
| }), | ||
| ], | ||
| }); | ||
| // Two checkouts staged onto the listing about to be deleted: one still | ||
| // mid-payment (pending) and one that already resolved (booked). | ||
| const pending = await stageCheckout("sess_del_pending", "stripe", intent); | ||
| const booked = await stageCheckout("sess_del_booked", "stripe", intent); | ||
| await markCheckoutStage("sess_del_booked", "booked"); | ||
|
|
||
| await deleteListing(listing.id); | ||
|
|
||
| // The delete guard is a preflight, so a stage that lands in its race window | ||
| // must not lose its rows — otherwise the paid order strands on an empty | ||
| // record. The pending checkout's quantity-0 row survives; the resolved | ||
| // one's is cascaded like any ordinary booking. | ||
| const rowCount = async (attendeeId: number): Promise<number> => | ||
| ( | ||
| await queryAll<{ id: number }>( | ||
| "SELECT id FROM listing_attendees WHERE attendee_id = ?", | ||
| [attendeeId], | ||
| ) | ||
| ).length; | ||
| expect(await rowCount(pending.attendeeId)).toBe(1); | ||
| expect(await rowCount(booked.attendeeId)).toBe(0); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Assert that the staged checkout metadata survives deletion.
This test only counts listing_attendees rows. If listing deletion removed the checkout-stage/session metadata while leaving the quantity-zero attendee row, the test would pass even though payment activation or refund recovery could no longer find the booking. Assert that the pending stage and its booking metadata remain recoverable, in addition to the existing row-count checks.
As per coding guidelines, tests should verify observable behavior with strong assertions; the PR objective also requires deleted-listing staged payments to remain recoverable.
🤖 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/listings/delete.test.ts` around lines 262 - 295, Extend the
test around pending and booked checkout deletion to assert that the pending
checkout stage and its associated booking/session metadata remain recoverable
after deleteListing, using the existing checkout lookup or recovery helpers.
Keep the current listing_attendees row-count assertions and confirm the booked
checkout retains its existing cascade behavior.
Source: Coding guidelines
| test("is a no-op if the session was pruned", async () => { | ||
| await finalizeSessionIfUnresolved("sess_gone", 1); | ||
| await finalizeSessionIfUnresolved("sess_gone", 1, ""); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Strengthen the pruned-session no-op test with an actual assertion.
The test only calls finalizeSessionIfUnresolved and relies on the absence of a thrown error; it never checks that the session remained untouched. A regression that silently mutates or re-creates state for a pruned session would pass this test.
As per path instructions for test/**/*.{ts,tsx}: "Tests must call production code, test observable behavior rather than implementation details... and have clear failure semantics."
✅ Proposed strengthening
test("is a no-op if the session was pruned", async () => {
await finalizeSessionIfUnresolved("sess_gone", 1, "");
+ expect(await isSessionProcessed("sess_gone")).toBeUndefined();
});📝 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.
| test("is a no-op if the session was pruned", async () => { | |
| await finalizeSessionIfUnresolved("sess_gone", 1); | |
| await finalizeSessionIfUnresolved("sess_gone", 1, ""); | |
| }); | |
| test("is a no-op if the session was pruned", async () => { | |
| await finalizeSessionIfUnresolved("sess_gone", 1, ""); | |
| expect(await isSessionProcessed("sess_gone")).toBeUndefined(); | |
| }); |
🤖 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 356 - 358, Strengthen
the “is a no-op if the session was pruned” test by asserting observable database
state after calling finalizeSessionIfUnresolved for “sess_gone”. Verify the
pruned session remains absent and unchanged, using the existing test
database/query helpers and clear assertion failure semantics.
Source: Path instructions
| "APIKEY_LOCKOUT_MS", | ||
| "ATTACHMENT_URL_MAX_AGE_S", | ||
| "BOOKING_LOCKOUT_MS", | ||
| "CHECKOUT_SESSION_EXPIRY_MINUTES", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the new entries’ current values.
The key-set test does not verify LIMIT_ENTRIES uses CHECKOUT_SESSION_EXPIRY_MINUTES and PRUNE_CHECKOUT_STAGES_RETENTION_DAYS as their current values. Add explicit assertions in the following test; otherwise a stale or incorrect debug-page value passes.
As per coding guidelines, “Use strong assertions that resist realistic mutations.”
Also applies to: 187-187
🤖 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/limits.test.ts` at line 171, Update the relevant limits-value
test near the key-set assertions to explicitly assert that LIMIT_ENTRIES
contains CHECKOUT_SESSION_EXPIRY_MINUTES and
PRUNE_CHECKOUT_STAGES_RETENTION_DAYS with their current expected values, rather
than only verifying the keys exist. Use strong value assertions that would fail
if either debug-page limit becomes stale or incorrect.
Source: Coding guidelines
| export const stageTestCheckout = ( | ||
| sessionId: string, | ||
| listing: { id: number; name: string; slug: string }, | ||
| ) => | ||
| stageCheckout( | ||
| sessionId, | ||
| "stripe", | ||
| checkoutIntent({ | ||
| items: [ | ||
| checkoutItem({ | ||
| listingId: listing.id, | ||
| name: listing.name, | ||
| slug: listing.slug, | ||
| }), | ||
| ], | ||
| }), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add an explicit return type to stageTestCheckout.
Unlike finalizeTestPaymentSession below, this exported function has no explicit return type annotation. As per coding guidelines, "Annotate every exported or public function with an explicit return type."
♻️ Proposed fix
export const stageTestCheckout = (
sessionId: string,
listing: { id: number; name: string; slug: string },
-) =>
+): ReturnType<typeof stageCheckout> =>
stageCheckout(📝 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.
| export const stageTestCheckout = ( | |
| sessionId: string, | |
| listing: { id: number; name: string; slug: string }, | |
| ) => | |
| stageCheckout( | |
| sessionId, | |
| "stripe", | |
| checkoutIntent({ | |
| items: [ | |
| checkoutItem({ | |
| listingId: listing.id, | |
| name: listing.name, | |
| slug: listing.slug, | |
| }), | |
| ], | |
| }), | |
| ); | |
| export const stageTestCheckout = ( | |
| sessionId: string, | |
| listing: { id: number; name: string; slug: string }, | |
| ): ReturnType<typeof stageCheckout> => | |
| stageCheckout( | |
| sessionId, | |
| "stripe", | |
| checkoutIntent({ | |
| items: [ | |
| checkoutItem({ | |
| listingId: listing.id, | |
| name: listing.name, | |
| slug: listing.slug, | |
| }), | |
| ], | |
| }), | |
| ); |
🤖 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/test-utils/db-helpers/processed-payments.ts` around lines 7 - 23, Add an
explicit return type annotation to the exported stageTestCheckout function,
matching the return type of the stageCheckout call it returns. Keep its existing
parameters and checkout construction unchanged.
Source: Coding guidelines
| test("refuses to delete a mid-payment staged attendee", async () => { | ||
| const { listing } = await setupListingAndLogin({ | ||
| maxAttendees: 100, | ||
| unitPrice: 1000, | ||
| }); | ||
| // A staged quantity-0 attendee whose payment can still land and claim its | ||
| // exact rows: confirming by name gets past the confirmation step, but the | ||
| // pending guard blocks the delete until the checkout finishes or expires. | ||
| const stage = await stageMidPaymentAttendee( | ||
| listing, | ||
| "cs_attendee_delete_guard", | ||
| ); | ||
|
|
||
| const { response } = await adminFormPost( | ||
| `/admin/attendees/${stage.attendeeId}/delete`, | ||
| { confirm_identifier: "Buyer" }, | ||
| ); | ||
| expect(response.status).toBe(302); | ||
| expectFlash(response, expect.stringContaining("mid-payment"), false); | ||
| // The staged rows survive the refused delete. | ||
| const { getAttendeeRaw } = await import( | ||
| "#shared/db/attendees/queries.ts" | ||
| ); | ||
| expect(await getAttendeeRaw(stage.attendeeId)).not.toBeNull(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test logic is correct; prefer a static import over the inline dynamic import.
getAttendeeRaw is fetched via await import(...) (Lines 235-238) instead of a top-level static import — deleted-listing.test.ts already imports the same symbol from the same path statically. This exact 3-line block is also repeated verbatim later in this file (Lines 265-268).
♻️ Proposed fix
+import { getAttendeeRaw } from "`#shared/db/attendees/queries.ts`";
...
- const { getAttendeeRaw } = await import(
- "`#shared/db/attendees/queries.ts`"
- );
expect(await getAttendeeRaw(stage.attendeeId)).not.toBeNull();Based on retrieved learnings ("Eliminate all code duplication... do not evade jscpd or use jscpd:ignore except for import blocks") and the test/**/*.{ts,tsx} guideline reserving dynamic imports for heavy SDKs.
📝 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.
| test("refuses to delete a mid-payment staged attendee", async () => { | |
| const { listing } = await setupListingAndLogin({ | |
| maxAttendees: 100, | |
| unitPrice: 1000, | |
| }); | |
| // A staged quantity-0 attendee whose payment can still land and claim its | |
| // exact rows: confirming by name gets past the confirmation step, but the | |
| // pending guard blocks the delete until the checkout finishes or expires. | |
| const stage = await stageMidPaymentAttendee( | |
| listing, | |
| "cs_attendee_delete_guard", | |
| ); | |
| const { response } = await adminFormPost( | |
| `/admin/attendees/${stage.attendeeId}/delete`, | |
| { confirm_identifier: "Buyer" }, | |
| ); | |
| expect(response.status).toBe(302); | |
| expectFlash(response, expect.stringContaining("mid-payment"), false); | |
| // The staged rows survive the refused delete. | |
| const { getAttendeeRaw } = await import( | |
| "#shared/db/attendees/queries.ts" | |
| ); | |
| expect(await getAttendeeRaw(stage.attendeeId)).not.toBeNull(); | |
| }); | |
| import { getAttendeeRaw } from "`#shared/db/attendees/queries.ts`"; | |
| test("refuses to delete a mid-payment staged attendee", async () => { | |
| const { listing } = await setupListingAndLogin({ | |
| maxAttendees: 100, | |
| unitPrice: 1000, | |
| }); | |
| // A staged quantity-0 attendee whose payment can still land and claim its | |
| // exact rows: confirming by name gets past the confirmation step, but the | |
| // pending guard blocks the delete until the checkout finishes or expires. | |
| const stage = await stageMidPaymentAttendee( | |
| listing, | |
| "cs_attendee_delete_guard", | |
| ); | |
| const { response } = await adminFormPost( | |
| `/admin/attendees/${stage.attendeeId}/delete`, | |
| { confirm_identifier: "Buyer" }, | |
| ); | |
| expect(response.status).toBe(302); | |
| expectFlash(response, expect.stringContaining("mid-payment"), false); | |
| // The staged rows survive the refused delete. | |
| expect(await getAttendeeRaw(stage.attendeeId)).not.toBeNull(); | |
| }); |
🤖 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/lib/server-attendees/delete.test.ts` around lines 215 - 239, Replace the
inline dynamic import of getAttendeeRaw in the test file with a top-level static
import from the existing shared attendees queries module, then reuse that
imported symbol in both repeated test locations. Keep the test assertions and
behavior unchanged.
Sources: Coding guidelines, Learnings
| const { deleteListing } = await import("#shared/db/listings/delete.ts"); | ||
| await deleteListing(listing.id); | ||
| const { markCheckoutStage } = await import("#shared/db/checkout-stages.ts"); | ||
| await markCheckoutStage(sessionId, "failed"); | ||
| const { loadExistingLines } = await import( | ||
| "#shared/db/attendees/atomic-update.ts" | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hoist these dynamic imports to static top-level imports.
markCheckoutStage is dynamically re-imported from #shared/db/checkout-stages.ts even though that same module is already statically imported at Line 3 for stageCheckout. deleteListing and loadExistingLines are ordinary internal DB helpers, not heavy SDKs, so lazy-loading buys nothing here and diverges from how deleted-listing.test.ts imports these same symbols statically.
♻️ Proposed fix
import { stageCheckout } from "`#shared/db/checkout-stages.ts`";
+import { markCheckoutStage } from "`#shared/db/checkout-stages.ts`";
+import { deleteListing } from "`#shared/db/listings/delete.ts`";
+import { loadExistingLines } from "`#shared/db/attendees/atomic-update.ts`";
...
export const resolvedDeletedListingAttendee = async (...) => {
const listing = await createTestListing({ unitPrice: 1000 });
const stage = await stageMidPaymentAttendee(listing, sessionId, otherListings);
- const { deleteListing } = await import("`#shared/db/listings/delete.ts`");
await deleteListing(listing.id);
- const { markCheckoutStage } = await import("`#shared/db/checkout-stages.ts`");
await markCheckoutStage(sessionId, "failed");
- const { loadExistingLines } = await import(
- "`#shared/db/attendees/atomic-update.ts`"
- );
const lines = await loadExistingLines(stage.attendeeId);As per coding guidelines, test/**/*.{ts,tsx} reserves dynamic loading for "heavy SDKs"; these are not.
📝 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 { deleteListing } = await import("#shared/db/listings/delete.ts"); | |
| await deleteListing(listing.id); | |
| const { markCheckoutStage } = await import("#shared/db/checkout-stages.ts"); | |
| await markCheckoutStage(sessionId, "failed"); | |
| const { loadExistingLines } = await import( | |
| "#shared/db/attendees/atomic-update.ts" | |
| ); | |
| import { stageCheckout } from "`#shared/db/checkout-stages.ts`"; | |
| import { markCheckoutStage } from "`#shared/db/checkout-stages.ts`"; | |
| import { deleteListing } from "`#shared/db/listings/delete.ts`"; | |
| import { loadExistingLines } from "`#shared/db/attendees/atomic-update.ts`"; | |
| ... | |
| export const resolvedDeletedListingAttendee = async (...) => { | |
| const listing = await createTestListing({ unitPrice: 1000 }); | |
| const stage = await stageMidPaymentAttendee(listing, sessionId, otherListings); | |
| await deleteListing(listing.id); | |
| await markCheckoutStage(sessionId, "failed"); | |
| const lines = await loadExistingLines(stage.attendeeId); |
🤖 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/lib/server-attendees/helpers.ts` around lines 64 - 70, Hoist the dynamic
imports for deleteListing, markCheckoutStage, and loadExistingLines into
top-level static imports in the helper module. Reuse the existing static
checkout-stages import for markCheckoutStage alongside stageCheckout, and remove
the corresponding in-function import statements while preserving call order and
behavior.
Source: Coding guidelines
| test("refuses a manual attendee entry while a checkout is pending", async () => { | ||
| const listing = await createTestListing({ | ||
| maxAttendees: 10, | ||
| unitPrice: 1000, | ||
| }); | ||
| const { stageMidPaymentAttendee } = await import( | ||
| "../server-attendees/helpers.ts" | ||
| ); | ||
| const stage = await stageMidPaymentAttendee(listing, "cs_ledger_add"); | ||
| // The checkout's activation posts its own sale/payment legs when the | ||
| // payment lands; a manual entry added mid-payment would combine with | ||
| // them into a surprise balance, so the write is refused. | ||
| const { response } = await adminFormPost( | ||
| `/admin/ledger/attendee/${stage.attendeeId}/add`, | ||
| { | ||
| amount: "12.34", | ||
| entry_type: MANUAL_ATTENDEE_PAYMENT, | ||
| occurred_at: "2026-06-22T09:30", | ||
| }, | ||
| ); | ||
| expect(response.status).toBe(302); | ||
| expectFlash(response, expect.stringContaining("mid-payment"), false); | ||
| expect(await allTransfers()).toEqual([]); | ||
| }); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer a static import of stageMidPaymentAttendee.
Lines 102-104 dynamically import stageMidPaymentAttendee from ../server-attendees/helpers.ts inside the test body; every other file that uses this helper (e.g. server-listings/delete.test.ts) imports it statically at the top of the file.
♻️ Proposed fix
+import { stageMidPaymentAttendee } from "../server-attendees/helpers.ts";
...
- const { stageMidPaymentAttendee } = await import(
- "../server-attendees/helpers.ts"
- );
const stage = await stageMidPaymentAttendee(listing, "cs_ledger_add");As per coding guidelines, test/**/*.{ts,tsx} reserves dynamic loading for "heavy SDKs"; a local test helper is not one.
📝 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.
| test("refuses a manual attendee entry while a checkout is pending", async () => { | |
| const listing = await createTestListing({ | |
| maxAttendees: 10, | |
| unitPrice: 1000, | |
| }); | |
| const { stageMidPaymentAttendee } = await import( | |
| "../server-attendees/helpers.ts" | |
| ); | |
| const stage = await stageMidPaymentAttendee(listing, "cs_ledger_add"); | |
| // The checkout's activation posts its own sale/payment legs when the | |
| // payment lands; a manual entry added mid-payment would combine with | |
| // them into a surprise balance, so the write is refused. | |
| const { response } = await adminFormPost( | |
| `/admin/ledger/attendee/${stage.attendeeId}/add`, | |
| { | |
| amount: "12.34", | |
| entry_type: MANUAL_ATTENDEE_PAYMENT, | |
| occurred_at: "2026-06-22T09:30", | |
| }, | |
| ); | |
| expect(response.status).toBe(302); | |
| expectFlash(response, expect.stringContaining("mid-payment"), false); | |
| expect(await allTransfers()).toEqual([]); | |
| }); | |
| import { stageMidPaymentAttendee } from "../server-attendees/helpers.ts"; | |
| test("refuses a manual attendee entry while a checkout is pending", async () => { | |
| const listing = await createTestListing({ | |
| maxAttendees: 10, | |
| unitPrice: 1000, | |
| }); | |
| const stage = await stageMidPaymentAttendee(listing, "cs_ledger_add"); | |
| // The checkout's activation posts its own sale/payment legs when the | |
| // payment lands; a manual entry added mid-payment would combine with | |
| // them into a surprise balance, so the write is refused. | |
| const { response } = await adminFormPost( | |
| `/admin/ledger/attendee/${stage.attendeeId}/add`, | |
| { | |
| amount: "12.34", | |
| entry_type: MANUAL_ATTENDEE_PAYMENT, | |
| occurred_at: "2026-06-22T09:30", | |
| }, | |
| ); | |
| expect(response.status).toBe(302); | |
| expectFlash(response, expect.stringContaining("mid-payment"), false); | |
| expect(await allTransfers()).toEqual([]); | |
| }); |
🤖 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/lib/server-ledger/add-entry.test.ts` around lines 97 - 121, Replace the
dynamic import of stageMidPaymentAttendee inside the “refuses a manual attendee
entry while a checkout is pending” test with a static top-level import, matching
the existing import style used by other tests. Remove the in-test import while
preserving the helper invocation and test behavior.
Source: Coding guidelines
| test("refuses to delete a listing whose attendee holds unreturned cash", async () => { | ||
| // A free listing keeps the booking's sale at 0, so the held payment | ||
| // below projects as a positive balance (cash in, nothing owed). | ||
| const { listing } = await setupListingAndLogin({ | ||
| maxAttendees: 100, | ||
| name: "Held Cash Listing", | ||
| }); | ||
| const attendee = await createTestAttendee( | ||
| listing.id, | ||
| listing.slug, | ||
| "Held Buyer", | ||
| "held-listing-delete@example.com", | ||
| ); | ||
| // A stage_active conflict leaves a held provider payment (no sale) on | ||
| // the attendee. Deleting the listing would cascade the booking line the | ||
| // in-app refund needs, stranding that charge — so the delete is refused | ||
| // until the cash is refunded. | ||
| const { postHeldPayment } = await import("#test-utils/ledger.ts"); | ||
| await postHeldPayment({ | ||
| amount: 1000, | ||
| attendeeId: attendee.id, | ||
| listingId: listing.id, | ||
| }); | ||
|
|
||
| const { response } = await adminFormPost( | ||
| `/admin/listing/${listing.id}/delete`, | ||
| { confirm_identifier: listing.name }, | ||
| ); | ||
| expect(response.status).toBe(302); | ||
| expectFlash(response, expect.stringContaining("not refunded"), false); | ||
| // The listing (and the refund path) survive the refused delete. | ||
| expect(await getListingWithCount(listing.id)).not.toBeNull(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer a static import of postHeldPayment.
Line 154 dynamically imports postHeldPayment from #test-utils/ledger.ts; test/lib/server-attendees/delete.test.ts and merge-post.test.ts already import this same symbol statically.
♻️ Proposed fix
+import { postHeldPayment } from "`#test-utils/ledger.ts`";
...
- const { postHeldPayment } = await import("`#test-utils/ledger.ts`");
await postHeldPayment({As per coding guidelines, test/**/*.{ts,tsx} reserves dynamic loading for "heavy SDKs"; this is not one.
📝 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.
| test("refuses to delete a listing whose attendee holds unreturned cash", async () => { | |
| // A free listing keeps the booking's sale at 0, so the held payment | |
| // below projects as a positive balance (cash in, nothing owed). | |
| const { listing } = await setupListingAndLogin({ | |
| maxAttendees: 100, | |
| name: "Held Cash Listing", | |
| }); | |
| const attendee = await createTestAttendee( | |
| listing.id, | |
| listing.slug, | |
| "Held Buyer", | |
| "held-listing-delete@example.com", | |
| ); | |
| // A stage_active conflict leaves a held provider payment (no sale) on | |
| // the attendee. Deleting the listing would cascade the booking line the | |
| // in-app refund needs, stranding that charge — so the delete is refused | |
| // until the cash is refunded. | |
| const { postHeldPayment } = await import("#test-utils/ledger.ts"); | |
| await postHeldPayment({ | |
| amount: 1000, | |
| attendeeId: attendee.id, | |
| listingId: listing.id, | |
| }); | |
| const { response } = await adminFormPost( | |
| `/admin/listing/${listing.id}/delete`, | |
| { confirm_identifier: listing.name }, | |
| ); | |
| expect(response.status).toBe(302); | |
| expectFlash(response, expect.stringContaining("not refunded"), false); | |
| // The listing (and the refund path) survive the refused delete. | |
| expect(await getListingWithCount(listing.id)).not.toBeNull(); | |
| }); | |
| import { postHeldPayment } from "`#test-utils/ledger.ts`"; | |
| test("refuses to delete a listing whose attendee holds unreturned cash", async () => { | |
| // A free listing keeps the booking's sale at 0, so the held payment | |
| // below projects as a positive balance (cash in, nothing owed). | |
| const { listing } = await setupListingAndLogin({ | |
| maxAttendees: 100, | |
| name: "Held Cash Listing", | |
| }); | |
| const attendee = await createTestAttendee( | |
| listing.id, | |
| listing.slug, | |
| "Held Buyer", | |
| "held-listing-delete@example.com", | |
| ); | |
| // A stage_active conflict leaves a held provider payment (no sale) on | |
| // the attendee. Deleting the listing would cascade the booking line the | |
| // in-app refund needs, stranding that charge — so the delete is refused | |
| // until the cash is refunded. | |
| await postHeldPayment({ | |
| amount: 1000, | |
| attendeeId: attendee.id, | |
| listingId: listing.id, | |
| }); | |
| const { response } = await adminFormPost( | |
| `/admin/listing/${listing.id}/delete`, | |
| { confirm_identifier: listing.name }, | |
| ); | |
| expect(response.status).toBe(302); | |
| expectFlash(response, expect.stringContaining("not refunded"), false); | |
| // The listing (and the refund path) survive the refused delete. | |
| expect(await getListingWithCount(listing.id)).not.toBeNull(); | |
| }); |
🤖 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/lib/server-listings/delete.test.ts` around lines 137 - 169, Replace the
dynamic import of postHeldPayment in the “refuses to delete a listing whose
attendee holds unreturned cash” test with a static top-level import from
`#test-utils/ledger.ts`, matching the existing imports in the related attendee
deletion tests. Keep the test setup and payment invocation unchanged.
Source: Coding guidelines
Summary
Checks
deno task lintdeno task cpdThe full test, typecheck, coverage, and mutation gates were intentionally left to CI/supervising review during the merge-resolution handoff.
Summary by CodeRabbit
New Features
Bug Fixes