Replace Stripe SDK with a smaller edge client - #1864
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces embedded Stripe SDK usage with a typed REST client, validated schemas, centralized runtime caching, dedicated webhook modules, expanded Stripe tests, and enhanced Stripe payment sandbox journeys. It also centralizes non-empty text validation and moves attendee quantity styling into CSS. ChangesStripe REST integration
Payment E2E flow
Validation and test migration
UI and supporting changes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/stripe.ts (1)
1-51: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile exceeds the ~400-line guideline.
This file spans well past 400 lines (checkout/refund/webhook/connection-test logic is all shown up to line 528+). As per coding guidelines,
**/*.{ts,tsx}files should be kept under roughly 400 lines with overloaded concepts split into focused modules (e.g., checkout, webhook management, and connection-testing could become separate modules importing shared client/schema types).🤖 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 1 - 51, Split the oversized Stripe integration module into focused modules for checkout/refund operations, webhook management, and connection testing, while keeping shared client, schema, and helper types reusable. Update the existing exported entry points and imports so behavior remains unchanged and each TypeScript file stays near the 400-line guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/stripe.ts`:
- Around line 153-180: Remove the unnecessary await from the createStripeClient
call in setupWebhookEndpointImpl, keeping the client assignment synchronous now
that createStripeClient returns a StripeClient directly.
In `@src/shared/stripe/form.ts`:
- Around line 10-18: Split encodePart into separate key and value encoding
paths: keep bracket un-escaping only for structural keys, while values must
retain encodeURIComponent’s %5B/%5D escaping. Update the form construction logic
using encodePart around the key/value handling, and add the specified regression
test in the Stripe form tests for a value containing brackets.
---
Outside diff comments:
In `@src/shared/stripe.ts`:
- Around line 1-51: Split the oversized Stripe integration module into focused
modules for checkout/refund operations, webhook management, and connection
testing, while keeping shared client, schema, and helper types reusable. Update
the existing exported entry points and imports so behavior remains unchanged and
each TypeScript file stays near the 400-line guideline.
🪄 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: 271dfb59-c04b-4ea6-b99b-a7b524465225
📒 Files selected for processing (12)
src/shared/stripe.tssrc/shared/stripe/client.tssrc/shared/stripe/form.tssrc/shared/stripe/schemas.tstest/integration/stripe-client-parity.test.tstest/lib/stripe/config.test.tstest/lib/stripe/webhook-mocks.tstest/lib/stripe/webhook-setup.test.tstest/shared/stripe/client.test.tstest/shared/stripe/form.test.tstest/shared/stripe/schemas.test.tstest/shared/stripe/webhook-cleanup.test.ts
|
Addressed the review’s oversized-module finding in 42c727c. The former 703-line Stripe module is now split by responsibility: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/stripe/runtime.ts`:
- Around line 17-33: Refactor sanitizeStripeError to replace the four repeated
statusCode, code, type, and requestId checks with a shared field definition and
per-entry extractor/handler, while preserving the existing type validation and
output formats. Keep the unknown-error fallback and error.name fallback
unchanged.
In `@src/shared/stripe/webhook.ts`:
- Around line 22-37: Reject malformed timestamp values in the webhook header
parser by retaining the final t value, validating that it is a complete safe
decimal integer, and only then converting it; preserve the existing
missing-timestamp and signature outcomes. Add a regression test using a valid
signedHeader with a suffix appended to its timestamp and assert the structured
invalid-header result.
🪄 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: d23150d1-4fb7-4589-84ce-2c21dab70efc
📒 Files selected for processing (21)
src/shared/stripe-provider.tssrc/shared/stripe.tssrc/shared/stripe/endpoints.tssrc/shared/stripe/runtime.tssrc/shared/stripe/webhook.tstest/integration/code-quality.test.tstest/integration/server-settings/stripe.test.tstest/integration/stripe-client-parity.test.tstest/integration/stripe/connection.test.tstest/integration/stripe/core.test.tstest/integration/stripe/webhook-setup.test.tstest/lib/code-quality/detectors.tstest/lib/stripe/config.test.tstest/lib/stripe/fixtures.tstest/lib/stripe/webhook-mocks.tstest/shared/stripe-provider.test.tstest/shared/stripe.test.tstest/shared/stripe/endpoints.test.tstest/shared/stripe/form.test.tstest/shared/stripe/runtime.test.tstest/shared/stripe/webhook.test.ts
💤 Files with no reviewable changes (1)
- test/lib/stripe/config.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/stripe/webhook.ts`:
- Around line 22-27: Update the header parsing loop in the webhook verification
function to locate the separator explicitly and reject any part containing
additional "=" delimiters before assigning timestampText or collecting
signatures. Preserve existing timestamp and signature validation, and add a
regression test covering a valid timestamp followed by "=junk" that must be
rejected.
In `@test/shared/stripe/webhook.test.ts`:
- Around line 36-46: Update the test around verifyWebhookSignature to assert
that errorSpy recorded exactly one call before inspecting its arguments. After
validating the call count, access the first call directly rather than using
optional chaining, while preserving the existing detail assertion.
🪄 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: bdcc5fae-553b-44aa-a181-254f3055005a
📒 Files selected for processing (3)
src/shared/stripe/runtime.tssrc/shared/stripe/webhook.tstest/shared/stripe/webhook.test.ts
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/integration/server/payments/purchase.test.ts (1)
27-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove stale comments describing the deleted Stripe reset teardown.
test/integration/server/payments/purchase.test.ts#L27-L30: delete or update the comment claiming the client is reset after each test.test/integration/server/payments/success.test.ts#L27-L30: delete or update the comment claiming the client is reset after each test.As per coding guidelines, comments must explain the current code and must not describe replaced implementations.
🤖 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/integration/server/payments/purchase.test.ts` around lines 27 - 30, Remove or update the stale Stripe client reset teardown comments in the test setup for test/integration/server/payments/purchase.test.ts lines 27-30 and test/integration/server/payments/success.test.ts lines 27-30; ensure any remaining comments accurately describe the current code and do not reference the deleted reset behavior.Source: Coding guidelines
src/shared/stripe-provider.ts (1)
114-115: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon't fall back to
""for a missingpayment_intent— reuseasStringlike the sibling path does.This falls back to the empty-string sentinel
""whenpayment_intentisn't a string, which the coding guidelines explicitly flag: "must throw when an expected result is missing instead of returning a sentinel such as null, '', 0, -1, or []." It's also inconsistent withresolveWebhookSessiona few lines above in this same file, which handles the identical field viaasString(obj.payment_intent)(yieldingundefined, not"").🐛 Proposed fix
return validatedPaymentSession({ amountTotal: amount_total, createdAt: isoFromUnixSeconds(session.created), id, metadata, - paymentReference: - typeof payment_intent === "string" ? payment_intent : "", + paymentReference: asString(payment_intent), paymentStatus: toPaymentStatus(payment_status), });As per coding guidelines, "A lookup, resolver, computation, or finder must throw when an expected result is missing instead of returning a sentinel such as null, '', 0, -1, or []."
🤖 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-provider.ts` around lines 114 - 115, Update the paymentReference assignment in the current resolver to reuse asString(payment_intent), matching resolveWebhookSession, instead of returning an empty-string fallback for non-string or missing values.Source: Coding guidelines
🤖 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 `@e2e-payments/src/main.ts`:
- Around line 148-176: Isolate every teardown and failure-report action so one
failure cannot prevent later steps or replace the original journey error. Update
reportFailure to independently guard screenshot, dumpServerLog, and
notifyFailure while preserving and rethrowing its original error; update stopRun
to independently attempt session.stop, tunnel.stop, provider cleanup, and
server.stop, ensuring all cleanup steps run even when an earlier step fails.
In `@src/shared/payment-helpers.ts`:
- Around line 173-176: Update the exported createWithClient function to declare
an explicit return type, using the existing named or inferred helper type
associated with guardedWithValue rather than leaving the signature inferred.
Preserve its current generic Client parameter, getClient input, and
errorHandling behavior.
In `@src/shared/stripe/mock.ts`:
- Around line 6-13: Rename the inner variable in the port function to avoid
shadowing the enclosing function name, and update the validation and return
references to use the new variable consistently.
In `@src/shared/stripe/request.ts`:
- Around line 208-218: Update the exported createStripeRequest function to
declare an explicit return type, preferably by introducing or reusing a named
type for the returned generic request function rather than using an inline
anonymous function type.
In `@src/shared/stripe/runtime.ts`:
- Around line 70-77: Rename the nullable resolver runtimeConfig to
runtimeConfigOrNull to reflect its intentional null result, and update the
getConfig reference in cachedClientFactory to use the renamed function. Keep the
existing secret-key check and returned configuration behavior unchanged.
In `@test/integration/server/webhooks/modifier-refunds.test.ts`:
- Line 1: Update the imports of createServiceChargeScenario in
modifier-refunds.test.ts and modifiers.test.ts to use the established
`#test/lib/server-webhooks/service-charge-scenario.ts` alias instead of the
../../../lib/server-webhooks relative path.
In `@test/shared/stripe-provider/operations.test.ts`:
- Around line 68-91: Replace the manual setupWebhookEndpoint reassignment and
try/finally restoration in the setupWebhookEndpoint test with the file’s
existing stub() helper, and apply the same refactor to createCheckoutSession.
Configure each stub with the current mocked result and let the standard stub
lifecycle handle restoration, removing the origSetup variables and manual
cleanup.
In `@test/shared/stripe/request.test.ts`:
- Around line 243-270: Strengthen the test “returns structured error fields
without exposing the response body” by asserting that the caught error’s message
and relevant observable string representation do not contain the mocked private
value “Private value.” Keep the existing structured-field and error-name
assertions unchanged, using mutation-resistant negative assertions that would
fail if the response body were leaked.
In `@test/ui/templates/admin/attendee-form/quantity.test.ts`:
- Around line 123-137: Update the test “the quantity input uses CSP-safe
styling” to target the quantity input element specifically, then assert that
this element has the line-qty class and does not contain the inline width style.
Replace the broad HTML string checks while preserving the existing page fetch
and mutation-resistant coverage.
---
Outside diff comments:
In `@src/shared/stripe-provider.ts`:
- Around line 114-115: Update the paymentReference assignment in the current
resolver to reuse asString(payment_intent), matching resolveWebhookSession,
instead of returning an empty-string fallback for non-string or missing values.
In `@test/integration/server/payments/purchase.test.ts`:
- Around line 27-30: Remove or update the stale Stripe client reset teardown
comments in the test setup for test/integration/server/payments/purchase.test.ts
lines 27-30 and test/integration/server/payments/success.test.ts lines 27-30;
ensure any remaining comments accurately describe the current code and do not
reference the deleted reset behavior.
🪄 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: bfee2243-8309-4d0e-bedc-b0bfd4a53b11
📒 Files selected for processing (119)
.github/workflows/payment-sandbox-e2e.ymle2e-payments/README.mde2e-payments/src/browser.tse2e-payments/src/config.tse2e-payments/src/flow.tse2e-payments/src/main.tse2e-payments/src/providers/stripe.tse2e-payments/src/providers/types.tsscripts/stripe-mock.tssrc/features/admin/settings-stripe.tssrc/features/api/request-schemas.tssrc/shared/bulk-email-targets.tssrc/shared/payment-helpers.tssrc/shared/stripe-provider.tssrc/shared/stripe.tssrc/shared/stripe/client.tssrc/shared/stripe/endpoints.tssrc/shared/stripe/mock.tssrc/shared/stripe/request.tssrc/shared/stripe/runtime.tssrc/shared/stripe/schemas.tssrc/shared/stripe/webhook.tssrc/shared/validation/string.tssrc/ui/static/style.scsssrc/ui/templates/admin/attendee-form.tsxtest/e2e/accounting/drivers.tstest/features/admin/settings-stripe.test.tstest/features/api/request-schemas.test.tstest/integration/bulk-email.test.tstest/integration/code-quality.test.tstest/integration/routes/renewal.test.tstest/integration/routes/site-assignment-checkout.test.tstest/integration/server/balance-get.test.tstest/integration/server/balance-payment-replay.test.tstest/integration/server/balance-post.test.tstest/integration/server/balance-webhook.test.tstest/integration/server/bulk-email/previous-bookings.test.tstest/integration/server/package-children.test.tstest/integration/server/payments-success-basic.test.tstest/integration/server/payments-success-refunds.test.tstest/integration/server/payments-success-replay.test.tstest/integration/server/payments/cancel.test.tstest/integration/server/payments/confirm.test.tstest/integration/server/payments/purchase.test.tstest/integration/server/payments/replay.test.tstest/integration/server/payments/success.test.tstest/integration/server/public/can-pay-more-multi.test.tstest/integration/server/public/can-pay-more-single.test.tstest/integration/server/public/daily-listings-multi.test.tstest/integration/server/public/daily-listings-single.test.tstest/integration/server/public/ticket-additional-coverage.test.tstest/integration/server/public/ticket-csrf-and-capacity.test.tstest/integration/server/public/ticket-paid-flow-edge-cases.test.tstest/integration/server/public/ticket-reserved-and-edge-cases.test.tstest/integration/server/reservation/deposit-basics.test.tstest/integration/server/reservation/edge-cases.test.tstest/integration/server/reservation/no-provider.test.tstest/integration/server/reservation/promo-addons.test.tstest/integration/server/reservation/public-default-modifiers.test.tstest/integration/server/webhook-dual-path.test.tstest/integration/server/webhooks/acknowledge-edge-cases.test.tstest/integration/server/webhooks/already-processed-rollback.test.tstest/integration/server/webhooks/can-pay-more-multi-ticket.test.tstest/integration/server/webhooks/can-pay-more-single-ticket.test.tstest/integration/server/webhooks/concurrent-processing.test.tstest/integration/server/webhooks/custom-questions-multi.test.tstest/integration/server/webhooks/custom-questions-single.test.tstest/integration/server/webhooks/customisable-days-pricing.test.tstest/integration/server/webhooks/extract-intent-redirect.test.tstest/integration/server/webhooks/item-validation.test.tstest/integration/server/webhooks/modifier-refunds.test.tstest/integration/server/webhooks/modifiers.test.tstest/integration/server/webhooks/multi-ticket-booking.test.tstest/integration/server/webhooks/multi-ticket-refunds.test.tstest/integration/server/webhooks/price-paid-calculation.test.tstest/integration/server/webhooks/price-signature-hidden-and-standalone.test.tstest/integration/server/webhooks/price-signature-package-overrides.test.tstest/integration/server/webhooks/price-signature-post-commit-recovery.test.tstest/integration/server/webhooks/price-signature-stored-refund-and-ignore.test.tstest/integration/server/webhooks/price-signature-trusted-and-mismatch.test.tstest/integration/server/webhooks/promo-codes.test.tstest/integration/server/webhooks/refund-helper-functions.test.tstest/integration/server/webhooks/refund-logging.test.tstest/integration/server/webhooks/refund-skip-conditions.test.tstest/integration/server/webhooks/registration-closed.test.tstest/integration/server/webhooks/session-resolution.test.tstest/integration/server/webhooks/signature-validation.test.tstest/integration/server/webhooks/single-ticket-booking.test.tstest/integration/server/webhooks/single-ticket-refunds.test.tstest/integration/server/webhooks/sumup.test.tstest/integration/server/webhooks/unrecognized-sessions.test.tstest/integration/stripe-client-parity.test.tstest/integration/stripe-mock-ports.test.tstest/integration/stripe/checkout.test.tstest/integration/stripe/connection.test.tstest/integration/stripe/core.test.tstest/integration/stripe/webhook-config.test.tstest/integration/stripe/webhook-setup.test.tstest/integration/stripe/webhook.test.tstest/integration/test-utils/stubs-and-mocks.test.tstest/lib/stripe/fixtures.tstest/lib/stripe/harness.tstest/lib/stripe/responses.tstest/lib/stripe/webhook-mocks.tstest/shared/bulk-email-targets.test.tstest/shared/payment-helpers/dispatch.test.tstest/shared/stripe-provider.test.tstest/shared/stripe-provider/operations.test.tstest/shared/stripe.test.tstest/shared/stripe/client.test.tstest/shared/stripe/endpoints.test.tstest/shared/stripe/mock.test.tstest/shared/stripe/request.test.tstest/shared/stripe/runtime.test.tstest/shared/stripe/schemas.test.tstest/shared/stripe/webhook.test.tstest/shared/validation/string.test.tstest/test-utils/order-journey.tstest/ui/templates/admin/attendee-form/quantity.test.ts
💤 Files with no reviewable changes (8)
- test/test-utils/order-journey.ts
- src/shared/stripe/webhook.ts
- src/ui/templates/admin/attendee-form.tsx
- test/integration/server/payments/confirm.test.ts
- test/integration/server/public/ticket-additional-coverage.test.ts
- test/integration/bulk-email.test.ts
- test/integration/server/payments/cancel.test.ts
- test/integration/code-quality.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aec331c6c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value, | ||
| webhookUrl, | ||
| previousEndpointId, | ||
| keyChanged ? undefined : previousEndpointId, |
There was a problem hiding this comment.
When the new secret is another key for the same Stripe account, passing undefined here means setupWebhookEndpoint no longer knows which endpoint is currently live. In the endpoint-limit retry path it lists same-URL endpoints and deletes every ID except the one passed as existingEndpointId; with this value omitted, it can delete the current saved endpoint before the replacement is created and stored, leaving paid bookings without a working webhook if the retry or DB save then fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in ec23855. saveSecret now always passes previousEndpointId to setupWebhookEndpoint, including on a key rotation. Inside setupWebhookEndpoint the endpoint-limit retry path filters the listed same-URL endpoints by id !== existingEndpointId (src/shared/stripe/endpoints.ts:28), so the recorded endpoint stays live until the new one is created — the existing keeps the recorded endpoint live when the cap retry fails regression in test/integration/stripe/webhook-setup.test.ts:110 covers that safety. The post-activate cleanupOldWebhookEndpoints calls then delete the previous endpoint once the replacement is saved. If the new key is on a different Stripe account, passing the old id is a no-op — the list will not return it.
The pre-existing rotationCases test in test/features/admin/settings-stripe.test.ts now asserts existingEndpointId="we_old" for both rotation cases (the key-rotation case previously asserted undefined and would have allowed this bug to slip through), so a regression that drops the recorded id here would fail the test directly.
| }); | ||
|
|
||
| /** Build the versioned Stripe request transport with bounded retries. */ | ||
| export const createStripeRequest = ( |
There was a problem hiding this comment.
The repo's AGENTS.md requires exported functions to declare their return type, but this new exported factory infers the returned generic request function from its body. That leaves callers without a stable named contract and makes the checker re-derive this transport shape; add an explicit return type here, ideally via a named request-function type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No change — leaving as inferred. I tried both the suggested refactor (: StripeRequest + inferred inline parameters) and a typed-const form (assigning transport: StripeRequest with arrow parameters implied by context). Both fail with TS7006: TypeScript cannot infer arrow-function parameter types from a generic contextual type. Annotating with : StripeRequest would force restating the parameter list inline, duplicating the type alias's parameter list verbatim and tripping jscpd's non-negotiable 0% threshold. The sibling ClientRunner<Client> annotation in src/shared/payment-helpers.ts avoids this only because guardedWithValue's impl uses a different type-parameter name (Value) than the alias's Client; no such divergence is possible here since both surfaces reference the same Method / StripeParams / ResponseSchema<T>. The inferred return shape is still a stable contract for callers (client.ts:109 and the request tests rely on it). If a future TypeScript version lifts the contextual-typing limitation for generic arrow functions, the named annotation can land cleanly without parameter-list duplication.
- Preserve the recorded webhook endpoint across a key-rotation limit-retry
by always passing previousEndpointId to setupWebhookEndpoint, so a limit
retry cannot delete the live webhook before its replacement is saved
(regression test asserts the recorded endpoint id is forwarded in both
rotation cases).
- Isolate each teardown/failure-report step in the e2e payments runner via
.catch(() => {}), so a failed session.stop or tunnel.stop cannot leave the
app-server child alive and hang CI instead of failing cleanly.
- Annotate createWithClient with an exported ClientRunner<Client> type so
the steel-thread runs (stripeClientRuntime.run) hold a named contract
instead of leaning on inference.
- Rename runtimeConfig to runtimeConfigOrNull to match the *OrNull naming
convention for intentional nullable resolvers.
- Rename the inner port variable in stripe-mock port parser to avoid
shadowing the enclosing function.
- Use #test/lib/server-webhooks/... alias instead of ../... relative path
in the modifier webhook tests.
- Replace manual reassignment + try/finally with stub() in the
stripe-provider operations test for consistency with the rest of the file.
- Strengthen the 'returns structured error fields' test by asserting the
structured fields never carry Stripe's raw response message.
- Scope the CSP-safe-styling test's assertions to the quantity <input>
element itself rather than the full page HTML.
- Remove the stale 'reset the client after each test' comment from
success.test.ts (the reset was deleted with the SDK removal).
|
{"body":"Addressed the two "outside diff range" notes from the latest CodeRabbit review summary (these don't get their own inline threads, so following up here):\n\n** |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec23855fb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const event = stripeCheckoutEvents(await response.json()).find( | ||
| ({ checkoutId }) => checkoutId === sessionId, | ||
| ); | ||
| return event?.pendingWebhooks === 0 ? event : null; |
There was a problem hiding this comment.
When the Stripe sandbox account has any stale enabled webhook endpoint from a canceled/failed run, this waits for pending_webhooks to become 0 even though Stripe defines that field as pending deliveries to all configured URLs, not just this run's endpoint (https://docs.stripe.com/api/events/object). Because the stale trycloudflare endpoints are only cleaned in finally after this check, an orphaned endpoint can keep the nightly Stripe leg failing even after the app's current webhook has processed the checkout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d6dd985. waitForStripeWebhook no longer gates on pending_webhooks === 0; it now waits for the checkout.session.completed event to exist for the session id. By the time this wait runs, the caller (assertPaidBookingConfirmed in main.ts:145) has already verified the booking is confirmed in the app, which means our own webhook endpoint has already received and processed the event. The previous pending_webhooks === 0 check was account-wide and could stall the nightly Stripe leg on a stale enabled endpoint left by an earlier cancelled/failed run — cleanup only deletes those stale endpoints in finally AFTER this wait, so the wait would never satisfy.
The removed pendingWebhooks field was also dead code on the type (only populated, never read after the check was dropped), so it was deleted from StripeCheckoutEvent and the parsing function per the house rule on dead code.
Note: the e2e-payments package is intentionally typecheck-only (no in-process test layer) per the codebase convention, so the only end-to-end surface for this code is the nightly sandbox run linked in the PR description's verification section.
| const requested = response.headers.get("stripe-should-retry"); | ||
| if (requested === "false") return false; | ||
| if (requested === "true") return true; | ||
| return response.status === 409 || response.status >= 500; |
There was a problem hiding this comment.
When Stripe returns HTTP 429 with code=lock_timeout during object contention, Stripe documents these as intermittent errors that should be retried and notes that its SDK retry mechanisms handle lock-timeout 429s (https://docs.stripe.com/rate-limits#object-lock-timeouts). This fallback only retries 409/5xx unless Stripe-Should-Retry is present, so replacing stripe-node can make a retryable refund or PaymentIntent lookup fail immediately instead of using the configured retry budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in d6dd985. The transport now retries 429 responses whose body has error.code === "lock_timeout" or error.type === "lock_timeout", per Stripe's rate-limits docs (https://docs.stripe.com/rate-limits#object-lock-timeouts). The check clones the response before reading the body, so the original body stays readable for the error path if the check returns false (no retry). Vanilla 429 rate-limits still only retry when Stripe asks via its stripe-should-retry: true header — compounding a real rate-limit with retries would make the limit worse.
Three regression tests cover the new path in test/shared/stripe/request.test.ts: (1) retries a 429 lock_timeout (object contention) mocks a 429 with error.code === "lock_timeout" then a 200, asserts the request retried once and succeeded; (2) does not retry a 429 rate-limit without lock_timeout mocks a 429 with error.code === "rate_limit", asserts a StripeApiError without retrying; (3) does not retry a 429 whose body cannot be parsed as JSON covers the catch branch in isLockTimeoutResponse, asserts a StripeProtocolError ("Invalid JSON received from the Stripe API"). deno task precommit (including 100% line/branch coverage) passes.
CodeRabbit follow-up:
- Guard notifyFailure(target) with .catch(() => {}) in the e2e payments
runner so a future change to the helper can never replace the original
journey error before rethrow.
Codex (new comments):
- Retry Stripe 429 lock_timeout responses (object contention) per Stripe's
rate-limits docs. The transport now clones the response, parses the body
on a 429, and retries if error.code or error.type is 'lock_timeout'.
Vanilla 429 rate-limits still only retry when Stripe asks via its
stripe-should-retry header. Regression tests cover the retryable
lock_timeout, the non-retryable rate-limit, and a 429 with unparseable
JSON body.
- Stop gating the e2e-payments Stripe webhook wait on
pending_webhooks === 0. That counter is account-wide, so a stale enabled
webhook endpoint left by an earlier canceled/failed run (only cleaned in
finally after this wait) would keep the nightly Stripe leg failing even
though the app's own webhook had already been delivered and the booking
confirmed. The wait now gates on the checkout.session.completed event
existing for the session id, which is the actual signal we need. Removed
the now-dead pendingWebhooks field from StripeCheckoutEvent.
CodeRabbit follow-up (request.test.ts): narrow the structured-error-fields
comment to explicitly exclude error.message, which intentionally mirrors
Stripe's raw error.message for stripe-node parity; the privacy boundary
lives one layer up in sanitizeStripeError, which never logs it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6dd985eb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const event = stripeCheckoutEvents(await response.json()).find( | ||
| ({ checkoutId }) => checkoutId === sessionId, | ||
| ); | ||
| return event ?? null; |
There was a problem hiding this comment.
When the Stripe leg returns via /payment/success, handlePaymentSuccess processes the session directly when a session_id is present (src/features/api/webhooks.ts lines 212-216), so the preceding assertPaidBookingConfirmed can pass without any webhook POST being delivered. This poll only looks for the session's event in Stripe's Events API; every completed checkout creates that event even if our registered endpoint 404s or rejects the signature. As a result the nightly Stripe leg can report that webhook delivery/signature works while the actual webhook path is broken; check a delivery attempt to this run's endpoint, or an app-side webhook marker, rather than just event existence.
Useful? React with 👍 / 👎.
| # Stripe request null checks: both values are typed as string|null, never undefined, | ||
| # so strict and loose null equality agree for every possible input. | ||
| src/shared/stripe/request.ts:89:17 === → == # retryAfter is string|null; both comparisons are true only for null | ||
| src/shared/stripe/request.ts:123:15 === → == # Headers.get returns string|null; both comparisons are true only for null |
There was a problem hiding this comment.
When precommit:mutation mutates src/shared/stripe/request.ts, the ignore-list checker validates entries for that file against generated mutants and fails stale ones. This entry points at line 123, but the Headers.get null check it describes is now at line 151, so a mutation run for the newly added Stripe transport reports this as stale (and the actual survivor is not suppressed). Update the recorded location or add a test that kills the mutant.
Useful? React with 👍 / 👎.
| existingEndpointId, | ||
| ) => { | ||
| try { | ||
| const client = stripeClientRuntime.create(secretKey, 0); |
There was a problem hiding this comment.
When saving Stripe settings hits a transient Stripe/network failure while creating the webhook endpoint, this 0 disables the retry handling implemented in the new transport for 409/5xx and connection errors. The replaced Stripe SDK client used the configured retry budget for this setup call, and the custom transport already reuses one idempotency key across POST retries, so a single retryable blip can now make credential setup fail instead of recovering. Let this use the default retry count unless a specific non-retryable setup path is required.
Useful? React with 👍 / 👎.
- Preserve the recorded webhook endpoint across a key-rotation limit-retry
by always passing previousEndpointId to setupWebhookEndpoint, so a limit
retry cannot delete the live webhook before its replacement is saved
(regression test asserts the recorded endpoint id is forwarded in both
rotation cases).
- Isolate each teardown/failure-report step in the e2e payments runner via
.catch(() => {}), so a failed session.stop or tunnel.stop cannot leave the
app-server child alive and hang CI instead of failing cleanly.
- Annotate createWithClient with an exported ClientRunner<Client> type so
the steel-thread runs (stripeClientRuntime.run) hold a named contract
instead of leaning on inference.
- Rename runtimeConfig to runtimeConfigOrNull to match the *OrNull naming
convention for intentional nullable resolvers.
- Rename the inner port variable in stripe-mock port parser to avoid
shadowing the enclosing function.
- Use #test/lib/server-webhooks/... alias instead of ../... relative path
in the modifier webhook tests.
- Replace manual reassignment + try/finally with stub() in the
stripe-provider operations test for consistency with the rest of the file.
- Strengthen the 'returns structured error fields' test by asserting the
structured fields never carry Stripe's raw response message.
- Scope the CSP-safe-styling test's assertions to the quantity <input>
element itself rather than the full page HTML.
- Remove the stale 'reset the client after each test' comment from
success.test.ts (the reset was deleted with the SDK removal).
CodeRabbit follow-up:
- Guard notifyFailure(target) with .catch(() => {}) in the e2e payments
runner so a future change to the helper can never replace the original
journey error before rethrow.
Codex (new comments):
- Retry Stripe 429 lock_timeout responses (object contention) per Stripe's
rate-limits docs. The transport now clones the response, parses the body
on a 429, and retries if error.code or error.type is 'lock_timeout'.
Vanilla 429 rate-limits still only retry when Stripe asks via its
stripe-should-retry header. Regression tests cover the retryable
lock_timeout, the non-retryable rate-limit, and a 429 with unparseable
JSON body.
- Stop gating the e2e-payments Stripe webhook wait on
pending_webhooks === 0. That counter is account-wide, so a stale enabled
webhook endpoint left by an earlier canceled/failed run (only cleaned in
finally after this wait) would keep the nightly Stripe leg failing even
though the app's own webhook had already been delivered and the booking
confirmed. The wait now gates on the checkout.session.completed event
existing for the session id, which is the actual signal we need. Removed
the now-dead pendingWebhooks field from StripeCheckoutEvent.
CodeRabbit follow-up (request.test.ts): narrow the structured-error-fields
comment to explicitly exclude error.message, which intentionally mirrors
Stripe's raw error.message for stripe-node parity; the privacy boundary
lives one layer up in sanitizeStripeError, which never logs it.
165c229 to
3cf2b6e
Compare
- Preserve the recorded webhook endpoint across a key-rotation limit-retry
by always passing previousEndpointId to setupWebhookEndpoint, so a limit
retry cannot delete the live webhook before its replacement is saved
(regression test asserts the recorded endpoint id is forwarded in both
rotation cases).
- Isolate each teardown/failure-report step in the e2e payments runner via
.catch(() => {}), so a failed session.stop or tunnel.stop cannot leave the
app-server child alive and hang CI instead of failing cleanly.
- Annotate createWithClient with an exported ClientRunner<Client> type so
the steel-thread runs (stripeClientRuntime.run) hold a named contract
instead of leaning on inference.
- Rename runtimeConfig to runtimeConfigOrNull to match the *OrNull naming
convention for intentional nullable resolvers.
- Rename the inner port variable in stripe-mock port parser to avoid
shadowing the enclosing function.
- Use #test/lib/server-webhooks/... alias instead of ../... relative path
in the modifier webhook tests.
- Replace manual reassignment + try/finally with stub() in the
stripe-provider operations test for consistency with the rest of the file.
- Strengthen the 'returns structured error fields' test by asserting the
structured fields never carry Stripe's raw response message.
- Scope the CSP-safe-styling test's assertions to the quantity <input>
element itself rather than the full page HTML.
- Remove the stale 'reset the client after each test' comment from
success.test.ts (the reset was deleted with the SDK removal).
CodeRabbit follow-up:
- Guard notifyFailure(target) with .catch(() => {}) in the e2e payments
runner so a future change to the helper can never replace the original
journey error before rethrow.
Codex (new comments):
- Retry Stripe 429 lock_timeout responses (object contention) per Stripe's
rate-limits docs. The transport now clones the response, parses the body
on a 429, and retries if error.code or error.type is 'lock_timeout'.
Vanilla 429 rate-limits still only retry when Stripe asks via its
stripe-should-retry header. Regression tests cover the retryable
lock_timeout, the non-retryable rate-limit, and a 429 with unparseable
JSON body.
- Stop gating the e2e-payments Stripe webhook wait on
pending_webhooks === 0. That counter is account-wide, so a stale enabled
webhook endpoint left by an earlier canceled/failed run (only cleaned in
finally after this wait) would keep the nightly Stripe leg failing even
though the app's own webhook had already been delivered and the booking
confirmed. The wait now gates on the checkout.session.completed event
existing for the session id, which is the actual signal we need. Removed
the now-dead pendingWebhooks field from StripeCheckoutEvent.
CodeRabbit follow-up (request.test.ts): narrow the structured-error-fields
comment to explicitly exclude error.message, which intentionally mirrors
Stripe's raw error.message for stripe-node parity; the privacy boundary
lives one layer up in sanitizeStripeError, which never logs it.
3cf2b6e to
9a25e94
Compare
1 participant
Summary
Why
Stripe was the largest dependency in the edge bundle even though the app uses only a small part of it. This removes 200,991 bytes from each bundle variant and about 46,000 parsed JavaScript nodes, which reduces edge cold-start work.
Review decisions
Verification
Summary by CodeRabbit