✨ server: add pax for platinum to signature card upgrades #670
Conversation
🦋 Changeset detectedLatest commit: b65a3b2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Summary of ChangesHello @aguxez, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the server's card issuance API by integrating with PAX for users who are upgrading from a Platinum card to a Signature card. The system now identifies these upgrade scenarios, retrieves necessary user information from Persona, and then registers the user with PAX for associated benefits, all while ensuring that the card issuance flow remains resilient to external service failures. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure WalkthroughDetects signature-card issuance following a previously deleted platinum card; when detected, fetches persona account, derives an internalId, and calls PAX Changes
Sequence DiagramsequenceDiagram
rect rgba(220,220,255,0.5)
participant Client as Client
participant CardAPI as Card API
participant DB as Database
participant Persona as Persona Service
participant PAX as PAX Service
participant Sentry as Sentry
end
Client->>CardAPI: Request signature card (POST/GET)
CardAPI->>DB: Query cards for credentialId (include deleted/platinum)
DB-->>CardAPI: Card list (deleted platinum found / not)
alt Upgrade detected
CardAPI->>Persona: getAccount(credentialId)
Persona-->>CardAPI: Account + persona data
CardAPI->>CardAPI: Validate identity, derive internalId
CardAPI->>PAX: addCapita({ internalId, travelInsurance, productId })
alt PAX success
PAX-->>CardAPI: OK
else PAX error
PAX-->>CardAPI: Error
CardAPI->>Sentry: captureException(error, { credentialId, account, productId })
end
end
CardAPI->>DB: Insert/fetch signature card
CardAPI-->>Client: Return card data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces functionality to track platinum to signature card upgrades by integrating with the PAX system. The changes include adding logic to detect such upgrades, calling an external API (addCapita) with user data, and comprehensive test cases covering various scenarios, including successful integration, no-upgrade cases, and error handling for external API calls. The addition of vi.clearAllMocks() in the test suite's afterEach hook is a good practice for ensuring test isolation and reliability.
![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
🤖 Fix all issues with AI agents
In `@server/api/card.ts`:
- Around line 383-385: The call to addCapitaForPlatinumUpgrade(credentialId,
account) is intended as fire-and-forget but currently returns a floating
promise; prefix the call with the void operator to make the intent explicit and
satisfy linters (i.e., change the invocation inside the isUpgradeFromPlatinum
block to void addCapitaForPlatinumUpgrade(credentialId, account)). Ensure you do
not await it and keep any surrounding logic unchanged so card creation remains
non-blocking.
In `@server/test/mocks/pax.ts`:
- Around line 3-4: The imports in server/test/mocks/pax.ts are out of order;
move external-library imports before relative imports so "valibot" (InferInput)
is imported before the local "../../utils/pax" (CapitaRequest), ensuring import
order follows eslint-plugin-import conventions (external libs first, then
relative paths) and updating any surrounding import groups similarly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #670 +/- ##
==========================================
+ Coverage 65.82% 65.85% +0.02%
==========================================
Files 190 190
Lines 5996 6012 +16
Branches 1734 1738 +4
==========================================
+ Hits 3947 3959 +12
- Misses 1870 1871 +1
- Partials 179 182 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aguxez
force-pushed
the
pax-card-upgrade
branch
from
e6c1e66 to
1be9a29
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
aguxez
force-pushed
the
pax-card-upgrade
branch
3 times, most recently
from
8f1e6a5 to
1779bbd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/api/card.ts`:
- Around line 334-355: The lookup for deletedPlatinumCard is using the
module-level database client instead of the request context DB; replace the call
to database.query.cards.findFirst with c.var.db.query.cards.findFirst (keeping
the same where/filter using cards, credentialId and PLATINUM_PRODUCT_ID) so the
route uses the injected DB client; also update the other similar lookup(s)
around the later block (the occurrences mentioned at 369-372) to use
c.var.db.query.* instead of database.query.* (ensure you keep the same
columns/where predicates and variable names like deletedPlatinumCard and
isUpgradeFromPlatinum).
1779bbd to
cc3ea01
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/test/api/card.test.ts`:
- Around line 83-86: The afterEach cleanup currently calls both
vi.restoreAllMocks() and vi.clearAllMocks() in the test file; remove the
redundant vi.clearAllMocks() call and keep only vi.restoreAllMocks() in the
afterEach block so mocks are restored and state cleared by restoreAllMocks()
(update the afterEach function that contains vi.restoreAllMocks() and
vi.clearAllMocks()).
cc3ea01 to
71cd8d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/test/api/card.test.ts`:
- Around line 366-367: The inline comment above the expectation uses an
uppercase word "NOT" which violates the repo comment casing rule; edit the
comment preceding the expect(pax.addCapita).not.toHaveBeenCalled() assertion so
all words are lowercase (e.g., "pax should not be called for new users (no
upgrade)"), and apply the same lowercase fix to the other offending comment at
the mentioned location (lines around the second occurrence referencing
pax.addCapita).
- Around line 472-474: The test currently mocks persona.getAccount with a null
cast which violates its declared return type; update the vi.spyOn(...,
"getAccount").mockResolvedValueOnce(...) call to pass undefined (no casts) so it
matches Promise<AccountOutput<T> | undefined> and preserves type safety—i.e.,
replace the null as unknown as ... line with mockResolvedValueOnce(undefined).
501509c to
5cef43c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/test/api/card.test.ts`:
- Line 472: The test uses vi.spyOn(persona,
"getAccount").mockResolvedValueOnce() without an argument causing TS2554; update
the mock call to pass the expected return value explicitly (pass undefined) by
changing the mockResolvedValueOnce invocation for persona.getAccount to
mockResolvedValueOnce(undefined) so it matches the function signature
Promise<AccountOutput<T> | undefined>.
a8e392a to
353edeb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/test/mocks/pax.ts`:
- Around line 8-11: The mock for removeCapita is a Promise<void> but calls
mockResolvedValue() with no argument, which fails strict TypeScript typing;
update the removeCapita mock (the vi.fn<(internalId: string) => Promise<void>>()
mock) to call .mockResolvedValue(undefined) so the resolved value matches
Awaited<ReturnType<...>>; leave addCapita as-is.
353edeb to
a5b538a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/test/api/card.test.ts`:
- Around line 440-466: Update the "handles missing persona account during
signature card creation" test to explicitly mock persona.getAccount to return
undefined so the test doesn't depend on default mock behavior: inside that it
block add a spy/mock for getAccount (or the persona service method named
getAccount) and have it resolve to undefined before calling
appClient.index.$post, leaving existing expectations (e.g., vi.spyOn(pax,
"addCapita") and vi.spyOn(panda, "createCard")) unchanged.
| it("handles missing persona account during signature card creation", async () => { | ||
| const testCredentialId = "no-account-test"; | ||
| await database.insert(credentials).values({ | ||
| id: testCredentialId, | ||
| publicKey: new Uint8Array(), | ||
| account: padHex("0x666", { size: 20 }), | ||
| factory: inject("ExaAccountFactory"), | ||
| pandaId: "no-account-panda", | ||
| }); | ||
|
|
||
| await database.insert(cards).values({ | ||
| id: "old-platinum-card-no-account", | ||
| credentialId: testCredentialId, | ||
| lastFour: "0000", | ||
| status: "DELETED", | ||
| productId: PLATINUM_PRODUCT_ID, | ||
| }); | ||
|
|
||
| vi.spyOn(pax, "addCapita").mockResolvedValueOnce({}); | ||
| vi.spyOn(panda, "createCard").mockResolvedValueOnce({ ...cardTemplate, id: "no-account-card", last4: "7777" }); | ||
|
|
||
| const response = await appClient.index.$post({ header: { "test-credential-id": testCredentialId } }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
|
|
||
| expect(pax.addCapita).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the missing-persona test explicit about getAccount returning undefined.
This test currently relies on the default persona mock behavior; an explicit mock keeps the intent stable if the default changes.
♻️ Suggested change
await database.insert(cards).values({
id: "old-platinum-card-no-account",
credentialId: testCredentialId,
lastFour: "0000",
status: "DELETED",
productId: PLATINUM_PRODUCT_ID,
});
+ vi.spyOn(persona, "getAccount").mockResolvedValueOnce(undefined);
vi.spyOn(pax, "addCapita").mockResolvedValueOnce({});
vi.spyOn(panda, "createCard").mockResolvedValueOnce({ ...cardTemplate, id: "no-account-card", last4: "7777" });📝 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.
| it("handles missing persona account during signature card creation", async () => { | |
| const testCredentialId = "no-account-test"; | |
| await database.insert(credentials).values({ | |
| id: testCredentialId, | |
| publicKey: new Uint8Array(), | |
| account: padHex("0x666", { size: 20 }), | |
| factory: inject("ExaAccountFactory"), | |
| pandaId: "no-account-panda", | |
| }); | |
| await database.insert(cards).values({ | |
| id: "old-platinum-card-no-account", | |
| credentialId: testCredentialId, | |
| lastFour: "0000", | |
| status: "DELETED", | |
| productId: PLATINUM_PRODUCT_ID, | |
| }); | |
| vi.spyOn(pax, "addCapita").mockResolvedValueOnce({}); | |
| vi.spyOn(panda, "createCard").mockResolvedValueOnce({ ...cardTemplate, id: "no-account-card", last4: "7777" }); | |
| const response = await appClient.index.$post({ header: { "test-credential-id": testCredentialId } }); | |
| expect(response.status).toBe(200); | |
| expect(pax.addCapita).not.toHaveBeenCalled(); | |
| }); | |
| it("handles missing persona account during signature card creation", async () => { | |
| const testCredentialId = "no-account-test"; | |
| await database.insert(credentials).values({ | |
| id: testCredentialId, | |
| publicKey: new Uint8Array(), | |
| account: padHex("0x666", { size: 20 }), | |
| factory: inject("ExaAccountFactory"), | |
| pandaId: "no-account-panda", | |
| }); | |
| await database.insert(cards).values({ | |
| id: "old-platinum-card-no-account", | |
| credentialId: testCredentialId, | |
| lastFour: "0000", | |
| status: "DELETED", | |
| productId: PLATINUM_PRODUCT_ID, | |
| }); | |
| vi.spyOn(persona, "getAccount").mockResolvedValueOnce(undefined); | |
| vi.spyOn(pax, "addCapita").mockResolvedValueOnce({}); | |
| vi.spyOn(panda, "createCard").mockResolvedValueOnce({ ...cardTemplate, id: "no-account-card", last4: "7777" }); | |
| const response = await appClient.index.$post({ header: { "test-credential-id": testCredentialId } }); | |
| expect(response.status).toBe(200); | |
| expect(pax.addCapita).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
In `@server/test/api/card.test.ts` around lines 440 - 466, Update the "handles
missing persona account during signature card creation" test to explicitly mock
persona.getAccount to return undefined so the test doesn't depend on default
mock behavior: inside that it block add a spy/mock for getAccount (or the
persona service method named getAccount) and have it resolve to undefined before
calling appClient.index.$post, leaving existing expectations (e.g.,
vi.spyOn(pax, "addCapita") and vi.spyOn(panda, "createCard")) unchanged.
04226a8 to
7643409
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| import { addCapita, deriveAssociateId } from "../utils/pax"; | ||
| import { getAccount } from "../utils/persona"; |
There was a problem hiding this comment.
🔴 server can crash on startup because card route now imports pax module with required env vars
server/api/card.ts now imports addCapita/deriveAssociateId from ../utils/pax at module load time (server/api/card.ts:35-36). The pax module performs top-level environment variable checks and throws if any are missing (server/utils/pax.ts:19-26).
Actual behavior: in any environment where PAX_API_URL, PAX_API_KEY, or PAX_ASSOCIATE_ID_KEY are not set, the server process will throw during module evaluation and fail to boot, even if the card issuance flow never reaches the pax integration.
Expected behavior: pax provisioning should be an optional side-effect (as described in the PR) and must not prevent the API server from starting or serving other routes.
click to expand
Relevant code paths:
// server/api/card.ts
import { addCapita, deriveAssociateId } from "../utils/pax";// server/utils/pax.ts
if (!process.env.PAX_API_URL) throw new Error("missing pax api url");
if (!process.env.PAX_API_KEY) throw new Error("missing pax api key");
if (!process.env.PAX_ASSOCIATE_ID_KEY) throw new Error("missing pax associate id secret");Impact:
- Deployment/startup failure if pax env isn’t present.
- Contradicts the goal of “provisioning failures no longer block card creation”, since lack of configuration blocks the entire server.
Recommendation: Make pax configuration lazy/optional: e.g. move env checks inside addCapita/request, return a rejected promise with a clear error when misconfigured, or dynamically import pax inside addCapitaForPlatinumUpgrade and no-op (with a logged error) when env vars are missing.
Was this helpful? React with 👍 or 👎 to provide feedback.
aguxez
force-pushed
the
pax-card-upgrade
branch
from
7643409 to
b65a3b2
Compare
Summary by CodeRabbit
New Features
Tests
Chores