feat/cli: archive removed products#135
Open
Blackmamoth wants to merge 2 commits intogetpaykit:mainfrom
Open
Conversation
|
@Blackmamoth is attempting to deploy a commit to the maxktz Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR introduces product archiving functionality to PayKit. When products are removed from the configuration during Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as paykitjs push
participant SyncService as product-sync.service
participant ProductService as product.service
participant Database as Database
participant Provider as Stripe Provider
User->>CLI: Run push with removed product
CLI->>SyncService: syncProducts(ctx)
SyncService->>ProductService: listLatestActiveProducts()
ProductService->>Database: Query active products
Database-->>ProductService: [active products]
ProductService-->>SyncService: [all active products]
SyncService->>SyncService: Identify products in config<br/>vs. active products
Note over SyncService: Products not in config<br/>need archiving
SyncService->>ProductService: archiveProductsByIds(ids)
ProductService->>Database: UPDATE archivedAt WHERE id IN (...)
Database-->>ProductService: [archived products]
ProductService-->>SyncService: [archived products]
SyncService->>Provider: archiveProduct({ providerProductId })
Provider->>Provider: Stripe: products.update<br/>({ active: false })
Provider-->>SyncService: void
SyncService-->>CLI: [SyncResult { action: "archived", ... }]
CLI-->>User: Display archived products
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (4)
packages/stripe/src/__tests__/stripe-provider.test.ts (1)
17-30: Consider a behavior assertion forarchiveProduct, not just existence.Type checks can still pass if implementation becomes a no-op. A mocked SDK expectation would make this regression-proof.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stripe/src/__tests__/stripe-provider.test.ts` around lines 17 - 30, Test currently only checks that archiveProduct exists on the adapter which won't catch regressions where it becomes a no-op; update the test that constructs the adapter via stripe({ secretKey..., webhookSecret... }).createAdapter() to assert archiveProduct performs the expected behavior by calling adapter.archiveProduct with a sample product id and using a mocked Stripe SDK to verify the correct SDK method was invoked (e.g., expect(stripeMock.products.del).toHaveBeenCalledWith(productId) or whichever SDK call your implementation uses), instead of only checking typeof archiveProduct.packages/paykit/src/product/product-sync.service.ts (2)
171-178: Consider handlingrestoreProductreturningnullexplicitly.The
restoreProductfunction can returnnullif no rows are updated. While the error check at lines 181-187 catches this, the generic error message may obscure the actual cause.💡 Suggested improvement for clearer error handling
} else if (existing.product.name !== plan.name || existing.product.archivedAt) { if (existing.product.name !== plan.name) { await updateProductName(ctx.database, existing.product.internalId, plan.name); } - storedProduct = existing.product.archivedAt - ? await restoreProduct(ctx.database, existing.product.internalId) - : { ...existing.product, name: plan.name }; + if (existing.product.archivedAt) { + const restored = await restoreProduct(ctx.database, existing.product.internalId); + if (!restored) { + throw PayKitError.from( + "INTERNAL_SERVER_ERROR", + PAYKIT_ERROR_CODES.PLAN_SYNC_FAILED, + `Failed to restore archived plan "${plan.id}"`, + ); + } + storedProduct = restored; + } else { + storedProduct = { ...existing.product, name: plan.name }; + } action = "updated"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/paykit/src/product/product-sync.service.ts` around lines 171 - 178, The restoreProduct call may return null when no rows are updated, but the code currently proceeds and only later throws a generic error; update the branch in product-sync.service.ts where restoreProduct(ctx.database, existing.product.internalId) is called so you explicitly check its return value, and if null throw a descriptive error (include existing.product.internalId and plan.name) instead of letting a later generic check run; ensure storedProduct is only set when restoreProduct returns a non-null product, and keep the existing updateProductName(existing.product.internalId, plan.name) call as-is.
241-243: Provider archiving errors will abort the sync without returning partial results.If
archiveProductthrows for any product, the function exits without returning results for successfully synced plans. The database will already have archived the products, leaving a partial state.This may be acceptable if the caller can retry, but consider whether you want to catch and log provider errors while continuing with other products.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/paykit/src/product/product-sync.service.ts` around lines 241 - 243, The loop over archivedProviderProductIds calls ctx.provider.archiveProduct which, if it throws, will abort the sync and prevent returning partial results; update the loop in product-sync.service.ts to wrap each await ctx.provider.archiveProduct({ providerProductId }) call in a try/catch, log the error with the providerProductId and any error details (using the existing logger/context), continue processing remaining providerProductIds, and optionally collect failedProviderProductIds into an array to include in the function's return value so callers can see which archives failed while others succeeded.packages/paykit/src/product/product.service.ts (1)
96-112: Implementation is correct; performance note for scale.The logic correctly retrieves the latest version per product ID. For large product catalogs with many versions, fetching all rows could be inefficient. Consider using
DISTINCT ON (id)(PostgreSQL) or a subquery for better database-level deduplication if this becomes a hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/paykit/src/product/product.service.ts` around lines 96 - 112, The current listLatestActiveProducts function fetches all active product rows into rows and de-duplicates in memory via productsById, which will be inefficient at scale; change the query to let the database return only the latest version per product id (e.g., use a DISTINCT ON (id) clause or a subquery that selects max(version) per id) instead of post-filtering rows, so modify the call in listLatestActiveProducts (the database.query.product.findMany usage) to perform DB-level deduplication and then remove the in-memory loop over rows/productsById.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/paykit/src/cli/utils/shared.ts`:
- Around line 74-76: The CLI is still passing diff.id to formatPlanLine so the
new product name field is never used; update the calls at/around lines where
formatPlanLine is invoked (e.g., the call that currently passes diff.id) to pass
diff.name instead, and if necessary adjust the formatPlanLine signature/usage to
accept and display a name (function name: formatPlanLine) so diff.name is
rendered in the diff output rather than the ID.
---
Nitpick comments:
In `@packages/paykit/src/product/product-sync.service.ts`:
- Around line 171-178: The restoreProduct call may return null when no rows are
updated, but the code currently proceeds and only later throws a generic error;
update the branch in product-sync.service.ts where restoreProduct(ctx.database,
existing.product.internalId) is called so you explicitly check its return value,
and if null throw a descriptive error (include existing.product.internalId and
plan.name) instead of letting a later generic check run; ensure storedProduct is
only set when restoreProduct returns a non-null product, and keep the existing
updateProductName(existing.product.internalId, plan.name) call as-is.
- Around line 241-243: The loop over archivedProviderProductIds calls
ctx.provider.archiveProduct which, if it throws, will abort the sync and prevent
returning partial results; update the loop in product-sync.service.ts to wrap
each await ctx.provider.archiveProduct({ providerProductId }) call in a
try/catch, log the error with the providerProductId and any error details (using
the existing logger/context), continue processing remaining providerProductIds,
and optionally collect failedProviderProductIds into an array to include in the
function's return value so callers can see which archives failed while others
succeeded.
In `@packages/paykit/src/product/product.service.ts`:
- Around line 96-112: The current listLatestActiveProducts function fetches all
active product rows into rows and de-duplicates in memory via productsById,
which will be inefficient at scale; change the query to let the database return
only the latest version per product id (e.g., use a DISTINCT ON (id) clause or a
subquery that selects max(version) per id) instead of post-filtering rows, so
modify the call in listLatestActiveProducts (the database.query.product.findMany
usage) to perform DB-level deduplication and then remove the in-memory loop over
rows/productsById.
In `@packages/stripe/src/__tests__/stripe-provider.test.ts`:
- Around line 17-30: Test currently only checks that archiveProduct exists on
the adapter which won't catch regressions where it becomes a no-op; update the
test that constructs the adapter via stripe({ secretKey..., webhookSecret...
}).createAdapter() to assert archiveProduct performs the expected behavior by
calling adapter.archiveProduct with a sample product id and using a mocked
Stripe SDK to verify the correct SDK method was invoked (e.g.,
expect(stripeMock.products.del).toHaveBeenCalledWith(productId) or whichever SDK
call your implementation uses), instead of only checking typeof archiveProduct.
🪄 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: dfb37da1-f418-428d-86bf-2efebf6d2986
📒 Files selected for processing (20)
.changeset/archive-removed-products.mde2e/cli/push.test.tse2e/cli/setup.tslanding/content/docs/concepts/cli.mdxlanding/content/docs/concepts/database.mdxlanding/content/docs/get-started/installation.mdxpackages/paykit/src/cli/utils/format.tspackages/paykit/src/cli/utils/shared.tspackages/paykit/src/customer/__tests__/customer.service.test.tspackages/paykit/src/database/migrations/0001_add_product_archived_at.sqlpackages/paykit/src/database/migrations/meta/0001_snapshot.jsonpackages/paykit/src/database/migrations/meta/_journal.jsonpackages/paykit/src/database/schema.tspackages/paykit/src/product/__tests__/product-sync.service.test.tspackages/paykit/src/product/product-sync.service.tspackages/paykit/src/product/product.service.tspackages/paykit/src/providers/provider.tspackages/stripe/src/__tests__/stripe-provider.test.tspackages/stripe/src/__tests__/stripe.test.tspackages/stripe/src/stripe-provider.ts
Comment on lines
+74
to
+76
| name: string; | ||
| priceAmount: number | null; | ||
| priceInterval: string | null; |
There was a problem hiding this comment.
Use product name (not ID) when formatting diff lines.
Line 90 still passes diff.id to formatPlanLine, so the newly added name field (Line 74) is ignored in CLI output.
Proposed fix
export function formatProductDiffs(
diffs: ProductDiff[],
plans: readonly NormalizedPlan[],
deps: Pick<CliDeps, "formatPlanLine" | "formatPrice">,
): string[] {
const plansById = new Map(plans.map((pl) => [pl.id, pl]));
return diffs.map((diff) => {
const plan = plansById.get(diff.id);
const amount = plan?.priceAmount ?? diff.priceAmount ?? 0;
const interval = plan?.priceInterval ?? diff.priceInterval;
const price = deps.formatPrice(amount, interval);
- return deps.formatPlanLine(diff.action, diff.id, price);
+ const displayName = plan?.name ?? diff.name;
+ return deps.formatPlanLine(diff.action, displayName, price);
});
}Also applies to: 87-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/paykit/src/cli/utils/shared.ts` around lines 74 - 76, The CLI is
still passing diff.id to formatPlanLine so the new product name field is never
used; update the calls at/around lines where formatPlanLine is invoked (e.g.,
the call that currently passes diff.id) to pass diff.name instead, and if
necessary adjust the formatPlanLine signature/usage to accept and display a name
(function name: formatPlanLine) so diff.name is rendered in the diff output
rather than the ID.
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.
Implements #123
Summary by CodeRabbit
New Features
paykitjs push.Documentation