feat(payment-method-management): add allowedModes + allowedPaymentMethodTypes props and bank-account-us return variant#1919
feat(payment-method-management): add allowedModes + allowedPaymentMethodTypes props and bank-account-us return variant#1919AngelPaella wants to merge 6 commits into
Conversation
… props and bank-account-us return variant - mode: new-only | new-and-existing (default new-and-existing) - allowedPaymentMethodTypes: filter array (default ["card"]) - CrossmintPaymentMethod is now a discriminated union on type with a bank-account-us variant (safe display summary; no token id or raw number) - params serialize into the iframe URL via the existing query-param builder - add service URL-builder + type-level tests
🦋 Changeset detectedLatest commit: bc57d87 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/client/base/src/types/payment-method-management/CrossmintPaymentMethodManagementProps.ts:24
**Empty-array sent to iframe instead of applying default**
`appendObjectToQueryParams` skips `undefined` (via `!value`) but NOT an empty array, because `![]` is `false` in JS. If a consumer passes `allowedPaymentMethodTypes: []`, the iframe receives `allowedPaymentMethodTypes=[]` rather than treating the prop as absent — meaning no payment-method types would be offered instead of falling back to the documented default `["card"]`. Adding a guard that maps an empty array to `undefined` before serialization (or inside `appendObjectToQueryParams`) would eliminate this subtle footgun.
Reviews (1): Last reviewed commit: "feat(payment-method-management): add mod..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * (default `["card"]`). LM0 renders the first supported entry; passing more | ||
| * than one type does not yet render a type picker. | ||
| */ | ||
| allowedPaymentMethodTypes?: PaymentMethodManagementAllowedType[]; |
There was a problem hiding this comment.
appendObjectToQueryParams skips undefined (via !value) but NOT an empty array, because ![] is false in JS. If a consumer passes allowedPaymentMethodTypes: [], the iframe receives allowedPaymentMethodTypes=[] rather than treating the prop as absent — meaning no payment-method types would be offered instead of falling back to the documented default ["card"]. Adding a guard that maps an empty array to undefined before serialization (or inside appendObjectToQueryParams) would eliminate this subtle footgun.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/client/base/src/types/payment-method-management/CrossmintPaymentMethodManagementProps.ts
Line: 24
Comment:
**Empty-array sent to iframe instead of applying default**
`appendObjectToQueryParams` skips `undefined` (via `!value`) but NOT an empty array, because `![]` is `false` in JS. If a consumer passes `allowedPaymentMethodTypes: []`, the iframe receives `allowedPaymentMethodTypes=[]` rather than treating the prop as absent — meaning no payment-method types would be offered instead of falling back to the documented default `["card"]`. Adding a guard that maps an empty array to `undefined` before serialization (or inside `appendObjectToQueryParams`) would eliminate this subtle footgun.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Already guarded in paymentMethodManagementService.ts (commit 4fada33): getIFrameUrl drops empty allowedPaymentMethodTypes and allow arrays before serialization (via ?.length), so an empty array is omitted from the iframe URL and the documented defaults apply. The empty array never reaches appendObjectToQueryParams.
|
Reviews (2): Last reviewed commit: "chore(changeset): payment-method-managem..." | Re-trigger Greptile |
…before serialization An empty array serialized as `[]` (appendObjectToQueryParams skips only falsy values), which the iframe read as 'offer no types' instead of the default ["card"]. Drop the key when empty. Addresses greptile review comment on #1919.
|
Reviews (3): Last reviewed commit: "fix(payment-method-management): drop emp..." | Re-trigger Greptile |
Replace mode?: "new-only" | "new-and-existing" with allow?: Array<"new" | "existing"> (default ["new"]). ["new"] hides the saved-methods section (old new-only); include "existing" to show it (old new-and-existing). The array shape mirrors allowedPaymentMethodTypes and also permits ["existing"] (saved list only).
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/client/base/src/services/payment-method-management/paymentMethodManagementService.test.ts:58-67
The test suite covers `allowedPaymentMethodTypes: []` being dropped (line 58–66), but there is no parallel test for `allow: []`. The serialization guard for both props is identical (`?.length` short-circuit), so the same `![]` footgun applies. Adding a mirror case keeps the documented contract explicit and prevents a future refactor from silently regressing it.
```suggestion
it("drops an empty allowedPaymentMethodTypes so the iframe applies the default", () => {
const service = makeService();
const url = service.iframe.getUrl({ jwt: "jwt-token", allowedPaymentMethodTypes: [] });
// An empty array must not serialize as `[]` (which the iframe would read as
// "offer no types"); the key is dropped so the documented default applies.
expect(new URL(url).searchParams.has("allowedPaymentMethodTypes")).toBe(false);
});
it("drops an empty allow so the iframe applies the default", () => {
const service = makeService();
const url = service.iframe.getUrl({ jwt: "jwt-token", allow: [] });
// Same ![] footgun as allowedPaymentMethodTypes; the key must be dropped
// so the iframe default (["new"]) applies instead of "render no sections".
expect(new URL(url).searchParams.has("allow")).toBe(false);
});
});
```
Reviews (4): Last reviewed commit: "refactor(payment-method-management): rep..." | Re-trigger Greptile |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| * A payment method surfaced to the integrator via `onPaymentMethodSelected`. | ||
| * Discriminated on `type`; narrow before reading variant-specific fields. | ||
| */ | ||
| export type CrossmintPaymentMethod = CrossmintCardPaymentMethod | CrossmintBankAccountUSPaymentMethod; |
There was a problem hiding this comment.
🚩 Discriminated union is a type-breaking change shipped as a minor bump
The changeset declares this as a minor version bump for @crossmint/client-sdk-base, but the CrossmintPaymentMethod type changed from a single object type ({ type: "card"; card: ...; ... }) to a discriminated union (CrossmintCardPaymentMethod | CrossmintBankAccountUSPaymentMethod). Consumers that previously accessed pm.card without narrowing on pm.type will get TypeScript compilation errors. The changeset description acknowledges this ("Type-breaking for consumers of onPaymentMethodSelected"). Whether this warrants a major bump depends on the project's versioning policy for TypeScript-only breaking changes. Runtime behavior is additive (the union only adds a new variant), so existing code consuming only card data would continue to work at runtime.
Was this helpful? React with 👍 or 👎 to provide feedback.
| * than one type does not yet render a type picker. | ||
| */ | ||
| allowedPaymentMethodTypes?: PaymentMethodManagementAllowedType[]; | ||
| onPaymentMethodSelected?: (paymentMethod: CrossmintPaymentMethod) => void | Promise<void>; |
There was a problem hiding this comment.
🚩 Event schema for payment-method:selected uses z.any() — no runtime validation of the new union
The incoming event schema at packages/client/base/src/types/payment-method-management/events/incoming.ts:7 uses z.any() for payment-method:selected. This means the iframe could send a malformed bank-account payload and the SDK would pass it through to onPaymentMethodSelected without validation. While this is a pre-existing issue (not introduced by this PR), it becomes more relevant now that there are two variants — a consumer narrowing on type === "bank-account-us" could receive an object missing bankAccount fields if the iframe has a bug. A zod discriminated union schema would catch this at the SDK boundary.
Was this helpful? React with 👍 or 👎 to provide feedback.
…match the iframe The iframe (Paella-Labs crossbit-main) parses the modes param as `allowedModes`, but the SDK was serializing it as `allow` — so the prop silently never reached the iframe. Rename the prop (and its query-param serialization + tests) to `allowedModes` so the cross-repo contract matches. Unreleased prop; no public API churn. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Reviews (5): Last reviewed commit: "refactor(payment-method-management): ren..." | Re-trigger Greptile |
AngelPaella
changed the title
2 participants
M1 of the offramp US bank-account work. Adds two optional props and a return-type variant to
<CrossmintPaymentMethodManagement>.packages/client/base/src/types/payment-method-management/CrossmintPaymentMethodManagementProps.ts.What's in it
allowedModes?: Array<"new" | "existing">(default["new"]) — which sections render:"new"shows the "add new" form; include"existing"to also show the saved-methods section.allowedPaymentMethodTypes?: ("card" | "bank-account-us")[](default["card"]). LM0 renders the first supported entry; a multi-type picker is not yet rendered.CrossmintPaymentMethodis now a discriminated union ontypewith abank-account-usvariant:paymentMethodId+bankAccount: { accountSuffix, bankName, accountType }. No token id, no raw account number.Backward-compatible: both props are optional with the prior defaults; the card variant is identical to the previous single shape, so existing card integrators are unaffected (a consumer that read
pm.cardwithout first narrowing onpm.typenow needs to narrow — the intended cost of the union).Depends on the iframe PR (Paella-Labs/crossbit-main
angel/offramp-pm-bank-us-pr1) that renders the bank UI these props target. The modes param is serialized asallowedModes, matching the param the iframe parses;allowedPaymentMethodTypesand the emittedbank-account-uspayload match the iframe field-for-field.