Refactor provider capabilities#194
Closed
maxktz wants to merge 2 commits into
Closed
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughPayKit's provider architecture transitions from an adapter-based pattern ( ChangesProvider Architecture Refactor to Capability-Driven Model
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/content/docs/providers/polar.mdx (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the docs to say “provider” instead of “adapter.”
This page still tells users that
polar()is an adapter, but this PR removes the adapter layer and exposes providers directly. Keeping the old term here makes the new surface area look older than it is.Also applies to: 14-14
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/content/docs/providers/polar.mdx` at line 6, Replace the outdated term "adapter" with "provider" throughout this doc (e.g., the sentence referencing the `@paykitjs/polar` adapter and the `polar()` adapter), updating wording like "adapter handles all Polar API interactions" to "provider handles all Polar API interactions" and any other occurrences (including the other instance noted at line 14) so the docs reflect the new provider surface area.packages/paykit/src/customer/customer.service.ts (1)
461-471:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't turn
hardDeleteCustomerinto a scheduled-cancellation flow.This path now schedules cancellation for period end and also gates
deleteCustomer()behind subscription-management capabilities. That can leave active remote subscriptions or an orphaned provider customer behind while the local customer is permanently deleted. Keep customer deletion separate from capability-gated subscription cleanup, and use the immediate cancel path here instead ofcancelSubscriptionAtPeriodEnd.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/paykit/src/customer/customer.service.ts` around lines 461 - 471, The current flow schedules cancellations via cancelSubscriptionAtPeriodEnd and gates deleteCustomer behind subscription-capability checks; instead, keep deletion separate and use the immediate cancel path. Change the block that calls assertProviderHasCapability("cancelSubscriptionsAtPeriodEnd"), listActiveSubscriptions, and cancelSubscriptionAtPeriodEnd so that deleteCustomer is always invoked (no capability gating), and if the provider supports subscription cancellation attempt immediate cancellation by calling the provider's cancelSubscription (immediate) method for each providerSubscriptionId returned by ctx.provider.listActiveSubscriptions; remove or replace the assertProviderHasCapability("cancelSubscriptionsAtPeriodEnd") check and do not prevent ctx.provider.deleteCustomer({ providerCustomerId }) from running even if subscription cancellation capability is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/paykit-architecture/SKILL.md:
- Line 3: The front-matter key "description" currently reads "Not use
automatically." which conflicts with the documented purpose; update the SKILL.md
front matter so the description accurately summarizes the file's intent (e.g.,
that this skill is for architecture and provider-integration guidance) so
discovery and invocation work correctly—locate the "description" field in the
SKILL.md front matter and replace the text with a concise, intent-aligned phrase
describing architecture/provider-integration usage.
In `@e2e/core/checkout/resubscribe-after-cancel.test.ts`:
- Around line 106-111: The test currently only waits for the webhook via
waitForWebhook but needs to wait for the subscription record to be fully
materialized as the "pro" plan before running
expectProduct/expectSingleActivePlanInGroup; replace or augment the
waitForWebhook call with the DB-polling approach used in the other checkout
tests: poll t.database (using the same timeout 120_000 and after=beforeCheckout
semantics) until the customer's subscription row shows plan "pro" (or until
expectSingleActivePlanInGroup/expectProduct would pass), then proceed. Locate
the webhook wait call and swap it for a poll that queries the subscription table
for the given customer ID and verifies the active plan is "pro" (or reuse the
helper used in other tests that waits for the active plan) to make the check
deterministic.
In `@e2e/core/checkout/subscribe-paid-checkout.test.ts`:
- Around line 113-123: The polling predicate in the rows.some check is failing
due to strict timestamp comparison (startedAt > after); remove the startedAt >
after clause so the helper no longer depends on exact timestamp precision—keep
the other checks (row.status === "active", row.planId === planId,
row.currentPeriodEndAt !== null, row.providerData !== null) and return as soon
as a matching active pro subscription is seen without comparing startedAt to
after.
In `@packages/paykit/src/cli/commands/listen.ts`:
- Around line 651-655: The CLI is hardcoding "Stripe" in progress/summary
output; replace that with the actual provider name from the loaded
webhook-endpoint-capable provider and thread it through render paths. Locate
where you call assertWebhookEndpointProvider and
provider.getWebhookEndpointAccount (and where loadRelayRuntimeContext is used)
and: 1) read a stable display name from the provider object (e.g., provider.name
or provider.displayName or provider.getDisplayName()) instead of the string
"Stripe"; 2) pass that provider name into params.devLogger.update and any
summary/render functions (including the loadRelayRuntimeContext call chain) so
all CLI messages use the real provider name; and 3) update any tests or call
sites that assumed "Stripe" to use the provider-derived name.
- Around line 233-243: The guard in assertWebhookEndpointProvider currently only
checks for method presence and must also respect the provider capability flag:
check provider.manageWebhookEndpoints and if it's explicitly false (or not
truthy) throw the same error saying the provider does not support listen; keep
the existing method checks (getWebhookEndpointAccount, ensureWebhookEndpoint,
deleteWebhookEndpoint) and only return provider as
WebhookEndpointCapableProvider when manageWebhookEndpoints is truthy and the
methods exist, updating the error message as needed to reference the capability.
In `@packages/paykit/src/product/product-sync.service.ts`:
- Around line 181-202: The per-plan sync loop must detect and fail fast on
duplicate provider product IDs before persisting conflicting mappings: after
obtaining providerResult.providerProduct.productId in the paidPlansToSync loop
(where ctx.provider.upsertSubscriptionProduct and upsertProviderProduct are
used), check against a Set of seen providerProductIds (the same collection used
to build activeProviderProductIds); if the providerProductId is already present,
throw or assert with details about the two conflicting plan IDs (e.g., current
plan.id and the previously-seen plan id) and do not call upsertProviderProduct
for the conflicting plan, otherwise add it to the Set and continue; keep
cleanupSubscriptionProducts call unchanged and pass the deduplicated
activeProviderProductIds.
In `@packages/paykit/src/testing/testing.service.ts`:
- Around line 16-20: Remove the manual capability short-circuit that throws
PayKitError.from("BAD_REQUEST", PAYKIT_ERROR_CODES.TESTING_NOT_ENABLED) when
ctx.provider.capabilities.testClocks is falsy; instead rely on the shared helper
assertProviderHasCapability(ctx.provider, "testClocks") as the single source of
truth for capability checks. Delete the initial if-block that checks
ctx.provider.capabilities.testClocks and also remove the now-redundant
additional assertProviderHasCapability(...) calls elsewhere in this file related
to "testClocks" so the helper alone enforces and emits the capability-specific
error.
---
Outside diff comments:
In `@apps/web/content/docs/providers/polar.mdx`:
- Line 6: Replace the outdated term "adapter" with "provider" throughout this
doc (e.g., the sentence referencing the `@paykitjs/polar` adapter and the
`polar()` adapter), updating wording like "adapter handles all Polar API
interactions" to "provider handles all Polar API interactions" and any other
occurrences (including the other instance noted at line 14) so the docs reflect
the new provider surface area.
In `@packages/paykit/src/customer/customer.service.ts`:
- Around line 461-471: The current flow schedules cancellations via
cancelSubscriptionAtPeriodEnd and gates deleteCustomer behind
subscription-capability checks; instead, keep deletion separate and use the
immediate cancel path. Change the block that calls
assertProviderHasCapability("cancelSubscriptionsAtPeriodEnd"),
listActiveSubscriptions, and cancelSubscriptionAtPeriodEnd so that
deleteCustomer is always invoked (no capability gating), and if the provider
supports subscription cancellation attempt immediate cancellation by calling the
provider's cancelSubscription (immediate) method for each providerSubscriptionId
returned by ctx.provider.listActiveSubscriptions; remove or replace the
assertProviderHasCapability("cancelSubscriptionsAtPeriodEnd") check and do not
prevent ctx.provider.deleteCustomer({ providerCustomerId }) from running even if
subscription cancellation capability is absent.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 08011305-07e6-47e6-bc3d-9a79d72e00d2
📒 Files selected for processing (41)
.agents/skills/paykit-architecture/SKILL.mdapps/web/content/docs/providers/polar.mdxe2e/core/cancel/cancel-then-upgrade.test.tse2e/core/checkout/resubscribe-after-cancel.test.tse2e/core/checkout/subscribe-paid-checkout.test.tse2e/core/entitlements/stacked-metered.test.tse2e/core/lifecycle/subscription.test.tse2e/core/subscribe/downgrade-scheduled.test.tse2e/core/webhook/duplicate-webhook.test.tse2e/paykit.config.tse2e/test-utils/env.tse2e/test-utils/harness/polar.tse2e/test-utils/harness/stripe.tse2e/test-utils/harness/types.tse2e/test-utils/hub.tse2e/test-utils/setup.tspackages/paykit/src/api/__tests__/define-route.test.tspackages/paykit/src/api/__tests__/methods.test.tspackages/paykit/src/cli/commands/listen.tspackages/paykit/src/cli/utils/shared.tspackages/paykit/src/core/__tests__/context.test.tspackages/paykit/src/core/context.tspackages/paykit/src/core/errors.tspackages/paykit/src/customer/__tests__/customer.service.test.tspackages/paykit/src/customer/customer.api.tspackages/paykit/src/customer/customer.service.tspackages/paykit/src/index.tspackages/paykit/src/product/product-sync.service.tspackages/paykit/src/providers/__tests__/capabilities.test.tspackages/paykit/src/providers/capabilities.tspackages/paykit/src/providers/provider.tspackages/paykit/src/subscription/subscription.service.tspackages/paykit/src/testing/testing.service.tspackages/paykit/src/types/events.tspackages/paykit/src/types/options.tspackages/paykit/src/webhook/webhook.service.tspackages/polar/src/__tests__/polar-provider.test.tspackages/polar/src/polar-provider.tspackages/stripe/src/__tests__/stripe-provider.test.tspackages/stripe/src/__tests__/stripe.test.tspackages/stripe/src/stripe-provider.ts
💤 Files with no reviewable changes (1)
- packages/paykit/src/api/tests/define-route.test.ts
| --- | ||
| name: paykit-architecture | ||
| description: Use before architectural, API design, provider integration, billing lifecycle, database model, or product-scope decisions in PayKit. | ||
| description: Not use automatically. |
There was a problem hiding this comment.
Keep the skill description aligned with the file's actual intent.
description: Not use automatically. now conflicts with the guidance below telling readers to use this skill for architecture and provider-integration work. If this front matter drives skill discovery, this change makes the right skill harder to invoke for exactly the scenarios this file documents.
🤖 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 @.agents/skills/paykit-architecture/SKILL.md at line 3, The front-matter key
"description" currently reads "Not use automatically." which conflicts with the
documented purpose; update the SKILL.md front matter so the description
accurately summarizes the file's intent (e.g., that this skill is for
architecture and provider-integration guidance) so discovery and invocation work
correctly—locate the "description" field in the SKILL.md front matter and
replace the text with a concise, intent-aligned phrase describing
architecture/provider-integration usage.
Comment on lines
+106
to
+111
| await waitForWebhook({ | ||
| database: t.database, | ||
| eventType: "checkout.completed", | ||
| after: beforeCheckout, | ||
| timeout: 120_000, | ||
| }); |
There was a problem hiding this comment.
Wait for the pro plan to become active here, not just for checkout.completed.
This webhook can be processed before the follow-up subscription state is fully materialized, so the expectProduct and expectSingleActivePlanInGroup checks below can still race on slower runs. Reusing the DB polling approach from the other checkout tests would make this path deterministic.
Proposed fix
- await waitForWebhook({
- database: t.database,
- eventType: "checkout.completed",
- after: beforeCheckout,
- timeout: 120_000,
- });
+ await waitForSingleActivePlanInGroup({
+ database: t.database,
+ customerId,
+ group: "base",
+ planId: "pro",
+ timeout: 120_000,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await waitForWebhook({ | |
| database: t.database, | |
| eventType: "checkout.completed", | |
| after: beforeCheckout, | |
| timeout: 120_000, | |
| }); | |
| await waitForSingleActivePlanInGroup({ | |
| database: t.database, | |
| customerId, | |
| group: "base", | |
| planId: "pro", | |
| timeout: 120_000, | |
| }); |
🤖 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 `@e2e/core/checkout/resubscribe-after-cancel.test.ts` around lines 106 - 111,
The test currently only waits for the webhook via waitForWebhook but needs to
wait for the subscription record to be fully materialized as the "pro" plan
before running expectProduct/expectSingleActivePlanInGroup; replace or augment
the waitForWebhook call with the DB-polling approach used in the other checkout
tests: poll t.database (using the same timeout 120_000 and after=beforeCheckout
semantics) until the customer's subscription row shows plan "pro" (or until
expectSingleActivePlanInGroup/expectProduct would pass), then proceed. Locate
the webhook wait call and swap it for a poll that queries the subscription table
for the given customer ID and verifies the active plan is "pro" (or reuse the
helper used in other tests that waits for the active plan) to make the check
deterministic.
Comment on lines
+113
to
+123
| if ( | ||
| rows.some( | ||
| (row) => | ||
| row.status === "active" && | ||
| row.planId === planId && | ||
| row.currentPeriodEndAt !== null && | ||
| row.startedAt > after && | ||
| row.providerData !== null, | ||
| ) | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
Drop the strict startedAt > after predicate from this poll.
Line 119 makes this helper depend on timestamp precision between the test process and the persisted subscription row. If startedAt is stored at the same instant as beforeCheckout or rounded slightly earlier, this loop can burn the full 120s even though the pro subscription is already active.
Proposed fix
async function waitForActiveSubscription(
t: TestPayKit,
customerId: string,
timeout: number,
- after: Date,
planId: string,
): Promise<void> {
@@
if (
rows.some(
(row) =>
row.status === "active" &&
row.planId === planId &&
row.currentPeriodEndAt !== null &&
- row.startedAt > after &&
row.providerData !== null,
)
) {
return;
}🤖 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 `@e2e/core/checkout/subscribe-paid-checkout.test.ts` around lines 113 - 123,
The polling predicate in the rows.some check is failing due to strict timestamp
comparison (startedAt > after); remove the startedAt > after clause so the
helper no longer depends on exact timestamp precision—keep the other checks
(row.status === "active", row.planId === planId, row.currentPeriodEndAt !==
null, row.providerData !== null) and return as soon as a matching active pro
subscription is seen without comparing startedAt to after.
Comment on lines
+233
to
243
| function assertWebhookEndpointProvider(provider: PaymentProvider): WebhookEndpointCapableProvider { | ||
| if ( | ||
| typeof provider.getTunnelAccount !== "function" || | ||
| typeof provider.ensureTunnelWebhook !== "function" || | ||
| typeof provider.disableTunnelWebhook !== "function" | ||
| typeof provider.getWebhookEndpointAccount !== "function" || | ||
| typeof provider.ensureWebhookEndpoint !== "function" || | ||
| typeof provider.deleteWebhookEndpoint !== "function" | ||
| ) { | ||
| throw new Error(`Provider "${provider.name}" does not support paykitjs listen yet.`); | ||
| } | ||
|
|
||
| return provider as TunnelCapableProvider; | ||
| return provider as WebhookEndpointCapableProvider; | ||
| } |
There was a problem hiding this comment.
Honor manageWebhookEndpoints in the runtime guard.
This assertion only checks for method presence, so a provider can still pass listen even when it declares manageWebhookEndpoints: false. That makes the CLI drift from the new capability-driven contract.
Suggested fix
function assertWebhookEndpointProvider(provider: PaymentProvider): WebhookEndpointCapableProvider {
if (
+ !provider.capabilities.manageWebhookEndpoints ||
typeof provider.getWebhookEndpointAccount !== "function" ||
typeof provider.ensureWebhookEndpoint !== "function" ||
typeof provider.deleteWebhookEndpoint !== "function"
) {
throw new Error(`Provider "${provider.name}" does not support paykitjs listen yet.`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/paykit/src/cli/commands/listen.ts` around lines 233 - 243, The guard
in assertWebhookEndpointProvider currently only checks for method presence and
must also respect the provider capability flag: check
provider.manageWebhookEndpoints and if it's explicitly false (or not truthy)
throw the same error saying the provider does not support listen; keep the
existing method checks (getWebhookEndpointAccount, ensureWebhookEndpoint,
deleteWebhookEndpoint) and only return provider as
WebhookEndpointCapableProvider when manageWebhookEndpoints is truthy and the
methods exist, updating the error message as needed to reference the capability.
Comment on lines
+651
to
+655
| const provider = assertWebhookEndpointProvider(config.options.provider); | ||
| const deviceToken = getOrCreateDeviceToken(); | ||
|
|
||
| params.devLogger.update("Connecting to Stripe"); | ||
| const account = await provider.getTunnelAccount(); | ||
| const account = await provider.getWebhookEndpointAccount(); |
There was a problem hiding this comment.
Stop hardcoding Stripe in this now-provider-agnostic flow.
loadRelayRuntimeContext() now accepts any webhook-endpoint-capable provider, but the progress/summary output in this command still says "Stripe". Polar users will get incorrect CLI messaging unless the loaded provider name is threaded through those render paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/paykit/src/cli/commands/listen.ts` around lines 651 - 655, The CLI
is hardcoding "Stripe" in progress/summary output; replace that with the actual
provider name from the loaded webhook-endpoint-capable provider and thread it
through render paths. Locate where you call assertWebhookEndpointProvider and
provider.getWebhookEndpointAccount (and where loadRelayRuntimeContext is used)
and: 1) read a stable display name from the provider object (e.g., provider.name
or provider.displayName or provider.getDisplayName()) instead of the string
"Stripe"; 2) pass that provider name into params.devLogger.update and any
summary/render functions (including the loadRelayRuntimeContext call chain) so
all CLI messages use the real provider name; and 3) update any tests or call
sites that assumed "Stripe" to use the provider-derived name.
Comment on lines
181
to
+202
| if (paidPlansToSync.length > 0) { | ||
| const providerResults = await ctx.provider.syncProducts({ | ||
| products: paidPlansToSync.map((p) => ({ | ||
| existingProviderProduct: p.existingProviderProduct, | ||
| id: p.id, | ||
| name: p.name, | ||
| priceAmount: p.priceAmount, | ||
| priceInterval: p.priceInterval, | ||
| })), | ||
| }); | ||
|
|
||
| const requestedIds = new Set(paidPlansToSync.map((p) => p.id)); | ||
| const resultById = new Map<string, (typeof providerResults.results)[number]>(); | ||
| for (const r of providerResults.results) { | ||
| if (resultById.has(r.id)) { | ||
| throw PayKitError.from( | ||
| "INTERNAL_SERVER_ERROR", | ||
| PAYKIT_ERROR_CODES.PLAN_SYNC_FAILED, | ||
| `Provider syncProducts returned duplicate mapping for id: ${r.id}`, | ||
| ); | ||
| assertProviderHasCapability(ctx.provider, "subscriptionProducts"); | ||
| const activeProviderProductIds: string[] = []; | ||
| for (const plan of paidPlansToSync) { | ||
| const providerResult = await ctx.provider.upsertSubscriptionProduct({ | ||
| existingProviderProduct: plan.existingProviderProduct, | ||
| id: plan.id, | ||
| name: plan.name, | ||
| priceAmount: plan.priceAmount, | ||
| priceInterval: plan.priceInterval, | ||
| }); | ||
| const providerProductId = providerResult.providerProduct.productId; | ||
| if (providerProductId) { | ||
| activeProviderProductIds.push(providerProductId); | ||
| } | ||
| resultById.set(r.id, r); | ||
| } | ||
| const missingIds = [...requestedIds].filter((id) => !resultById.has(id)); | ||
| if (missingIds.length > 0 || resultById.size !== requestedIds.size) { | ||
| throw PayKitError.from( | ||
| "INTERNAL_SERVER_ERROR", | ||
| PAYKIT_ERROR_CODES.PLAN_SYNC_FAILED, | ||
| `Provider syncProducts returned invalid mapping: missing=[${missingIds.join(", ")}], expected=${String(requestedIds.size)}, got=${String(resultById.size)}`, | ||
| ); | ||
| } | ||
|
|
||
| for (const [, providerResult] of resultById) { | ||
| const plan = paidPlansToSync.find((p) => p.id === providerResult.id); | ||
| if (!plan) continue; | ||
| await upsertProviderProduct(ctx.database, { | ||
| productInternalId: plan.storedProductInternalId, | ||
| providerId, | ||
| providerProduct: providerResult.providerProduct, | ||
| }); | ||
| } | ||
| await ctx.provider.cleanupSubscriptionProducts?.({ activeProviderProductIds }); |
There was a problem hiding this comment.
Restore duplicate provider-product safeguards in the per-plan sync path.
This refactor writes each mapping immediately, but it no longer fails fast if two plans resolve to the same provider product. In that case we silently persist conflicting mappings, and cleanupSubscriptionProducts will also treat that shared ID as valid for both plans.
Suggested guard
if (paidPlansToSync.length > 0) {
assertProviderHasCapability(ctx.provider, "subscriptionProducts");
const activeProviderProductIds: string[] = [];
+ const seenProviderProductIds = new Set<string>();
for (const plan of paidPlansToSync) {
const providerResult = await ctx.provider.upsertSubscriptionProduct({
existingProviderProduct: plan.existingProviderProduct,
id: plan.id,
name: plan.name,
priceAmount: plan.priceAmount,
priceInterval: plan.priceInterval,
});
const providerProductId = providerResult.providerProduct.productId;
if (providerProductId) {
+ if (seenProviderProductIds.has(providerProductId)) {
+ throw PayKitError.from(
+ "INTERNAL_SERVER_ERROR",
+ PAYKIT_ERROR_CODES.PLAN_SYNC_FAILED,
+ `Provider product "${providerProductId}" was returned for multiple plans during sync`,
+ );
+ }
+ seenProviderProductIds.add(providerProductId);
activeProviderProductIds.push(providerProductId);
}
await upsertProviderProduct(ctx.database, {
productInternalId: plan.storedProductInternalId,
providerId,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/paykit/src/product/product-sync.service.ts` around lines 181 - 202,
The per-plan sync loop must detect and fail fast on duplicate provider product
IDs before persisting conflicting mappings: after obtaining
providerResult.providerProduct.productId in the paidPlansToSync loop (where
ctx.provider.upsertSubscriptionProduct and upsertProviderProduct are used),
check against a Set of seen providerProductIds (the same collection used to
build activeProviderProductIds); if the providerProductId is already present,
throw or assert with details about the two conflicting plan IDs (e.g., current
plan.id and the previously-seen plan id) and do not call upsertProviderProduct
for the conflicting plan, otherwise add it to the Set and continue; keep
cleanupSubscriptionProducts call unchanged and pass the deduplicated
activeProviderProductIds.
Comment on lines
16
to
+20
| if (!ctx.provider.capabilities.testClocks) { | ||
| throw PayKitError.from("BAD_REQUEST", PAYKIT_ERROR_CODES.TESTING_NOT_ENABLED); | ||
| } | ||
|
|
||
| assertProviderHasCapability(ctx.provider, "testClocks"); |
There was a problem hiding this comment.
Don’t mask unsupported testClocks as TESTING_NOT_ENABLED.
Line 16 throws TESTING_NOT_ENABLED before assertProviderHasCapability() can run, so providers without testClocks never emit the capability-specific error this refactor introduced. If the shared helper is the new source of truth, remove the manual capability branch here and let it own the check; that also makes the extra assertions on Lines 25 and 68 unnecessary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/paykit/src/testing/testing.service.ts` around lines 16 - 20, Remove
the manual capability short-circuit that throws PayKitError.from("BAD_REQUEST",
PAYKIT_ERROR_CODES.TESTING_NOT_ENABLED) when
ctx.provider.capabilities.testClocks is falsy; instead rely on the shared helper
assertProviderHasCapability(ctx.provider, "testClocks") as the single source of
truth for capability checks. Delete the initial if-block that checks
ctx.provider.capabilities.testClocks and also remove the now-redundant
additional assertProviderHasCapability(...) calls elsewhere in this file related
to "testClocks" so the helper alone enforces and emits the capability-specific
error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Notes
Summary by CodeRabbit
Release Notes
Documentation
Improvements