Conversation
🦋 Changeset detectedLatest commit: 3319935 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 |
|
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:
WalkthroughAdds conditional KYC link generation for Bridge onboarding: schema extension, new getKycLink/maybeKycLink helpers with gating based on endorsements/requirements (including nested rules and suppression), Sentry capture on fetch failure, getProvider integration to include kycLink, expanded tests, and a changeset. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GetProvider as getProvider()
participant MaybeKycLink as maybeKycLink()
participant BridgeAPI as Bridge API
participant Sentry
Client->>GetProvider: Request provider info (customerId, redirectURL)
GetProvider->>GetProvider: determine provider/status
alt status == ONBOARDING and bridgeUser present
GetProvider->>MaybeKycLink: maybeKycLink(bridgeUser, redirectURL)
MaybeKycLink->>MaybeKycLink: evaluate offboard/blocklist/region/missing/issues (incl. nested all_of/any_of)
alt allowed
MaybeKycLink->>BridgeAPI: GET /customers/{id}/kyc_link?endorsement=&redirect_uri=&provider=bridge
alt returns URL
BridgeAPI-->>MaybeKycLink: { url }
MaybeKycLink-->>GetProvider: kycLink
else error
BridgeAPI-->>MaybeKycLink: error
MaybeKycLink->>Sentry: captureException(error)
MaybeKycLink-->>GetProvider: undefined
end
else blocked or no matching requirements
MaybeKycLink-->>GetProvider: undefined
end
GetProvider-->>Client: provider info (includes kycLink maybe)
else
GetProvider-->>Client: provider info (no kycLink)
end
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #950 +/- ##
==========================================
+ Coverage 71.73% 71.83% +0.09%
==========================================
Files 229 229
Lines 8315 8347 +32
Branches 2679 2689 +10
==========================================
+ Hits 5965 5996 +31
+ Misses 2120 2118 -2
- Partials 230 233 +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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/utils/ramps/bridge.ts (1)
194-199:⚠️ Potential issue | 🟠 MajorMalformed
redirectURLthrows for users who don't need it.The
new URL(params.redirectURL)on line 196 throwsTypeError: Invalid URLfor malformed input. Since this parsing occurs before theparams.customerIdcheck, even ACTIVE users—who never use the redirect URL—get downgraded toNOT_AVAILABLEwhenserver/api/ramp.ts's.catch()handles the exception.Consider making the parsing best-effort so it doesn't block ACTIVE users:
🛠️ Suggested fix
- let redirectURL: string | undefined; - if (params.redirectURL) { - const url = new URL(params.redirectURL); - url.searchParams.set("provider", "bridge"); - redirectURL = String(url); - } + let redirectURL: string | undefined; + if (params.redirectURL) { + try { + const url = new URL(params.redirectURL); + url.searchParams.set("provider", "bridge"); + redirectURL = String(url); + } catch { + // malformed url -- proceed without redirect + } + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e109ce47-3106-444f-b5fc-9033fff28864
📒 Files selected for processing (4)
.changeset/bright-owls-glow.mdserver/api/ramp.tsserver/test/utils/bridge.test.tsserver/utils/ramps/bridge.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64fdac6f56
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (params.redirectURL) { | ||
| const url = new URL(params.redirectURL); | ||
| url.searchParams.set("provider", "bridge"); |
There was a problem hiding this comment.
getProvider now parses params.redirectURL before the params.customerId branch, so malformed query values (for example "foo" or a relative path) throw in new URL(...) even for already-onboarded customers. Because server/api/ramp.ts accepts redirectURL as a plain string and maps bridge errors to NOT_AVAILABLE, this turns otherwise valid existing-customer responses into provider unavailability when redirect handling is not needed. Parse the redirect URL only in the code paths that actually use it (or enforce URL validation at the request boundary).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/utils/ramps/bridge.ts (1)
194-199:⚠️ Potential issue | 🟠 MajorHandle malformed
redirectURLlazily.Line 196 parses
params.redirectURLbefore the status branches, butserver/api/ramp.tsstill accepts any string here. A malformed or relative value will throw at this point, and the caller’s.catch()downgrades the whole Bridge provider toNOT_AVAILABLEeven for ACTIVE users that never need a redirect. Make this best-effort or defer parsing to the ONBOARDING / NOT_STARTED paths.🛠️ Proposed fix
- let redirectURL: string | undefined; - if (params.redirectURL) { - const url = new URL(params.redirectURL); - url.searchParams.set("provider", "bridge"); - redirectURL = String(url); - } + const redirectURL = params.redirectURL + ? (() => { + try { + const url = new URL(params.redirectURL); + url.searchParams.set("provider", "bridge"); + return String(url); + } catch { + return; + } + })() + : undefined;Run this to verify that the route accepts arbitrary strings while
new URL()rejects malformed or relative ones:#!/bin/bash set -euo pipefail echo "--- server/api/ramp.ts validator ---" sed -n '40,44p' server/api/ramp.ts echo echo "--- server/utils/ramps/bridge.ts redirect normalization ---" sed -n '194,199p' server/utils/ramps/bridge.ts echo echo "--- WHATWG URL constructor behavior ---" node <<'EOF' for (const value of ["not-a-url", "/callback", "https://app.example.com/callback"]) { try { const url = new URL(value); url.searchParams.set("provider", "bridge"); console.log("ok:", value, "=>", String(url)); } catch (error) { console.log("throws:", value, "=>", error.name); } } EOF
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f784cb0-1acf-4383-b6d2-d4bf630018f5
📒 Files selected for processing (4)
.changeset/bright-owls-glow.mdserver/api/ramp.tsserver/test/utils/bridge.test.tsserver/utils/ramps/bridge.ts
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ffd8e7939
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| pending: array(string()), | ||
| missing: nullish(unknown()), | ||
| issues: array(union([string(), unknown()])), | ||
| missing: nullish(MissingRequirement), |
There was a problem hiding this comment.
Parsing requirements.missing as nullish(MissingRequirement) makes getCustomer fail whenever Bridge returns a new requirement token or shape outside this hardcoded list. In that case getProvider throws, and server/api/ramp.ts maps the error to NOT_AVAILABLE, turning otherwise valid customers into provider outages after an upstream enum expansion. Keep parsing forward-compatible (e.g., preserve unknown nodes and only match allowlisted values when deriving kycLink).
Useful? React with 👍 / 👎.
| issues: pipe( | ||
| array(unknown()), | ||
| transform((items) => | ||
| items | ||
| .filter((item): item is string => typeof item === "string") | ||
| .map((item) => parse(picklist(Issues), item)), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🚩 Strict Issues picklist validation makes getCustomer brittle to new Bridge API issue types
The issues field in CustomerResponse (server/utils/ramps/bridge.ts:888-895) now uses pipe(array(unknown()), transform(...)) where the transform filters non-strings and then calls parse(picklist(Issues), item) on each remaining string. If Bridge's API introduces a new issue string not in the Issues picklist, this parse call throws a ValiError, which propagates through getCustomer and ultimately causes getProvider to throw. The caller in server/api/ramp.ts:69-72 catches this and returns NOT_AVAILABLE, silently degrading the user's experience.
This is intentional and well-tested (the test at line 86-101 explicitly verifies this throws). However, the previous schema (array(union([string(), unknown()]))) was lenient and accepted anything. The new behavior means any time Bridge adds a new issue type to their API, affected customers will see NOT_AVAILABLE until the Issues array is updated in this codebase. The same applies to the missing field with Requirements. This is a conscious trade-off between resilience and awareness of API changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f462c4bdb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| items | ||
| .filter((item): item is string => typeof item === "string") | ||
| .map((item) => parse(picklist(Issues), item)), |
There was a problem hiding this comment.
Mapping every requirements.issues string through parse(picklist(Issues), item) makes getCustomer fail whenever Bridge introduces a new issue code that is not in this hardcoded list. In the provider flow, that parse failure bubbles up and is converted to NOT_AVAILABLE, so a harmless upstream enum expansion can take Bridge onboarding offline for affected customers. Keep the parser forward-compatible (e.g., preserve unknown strings and only match allowlisted values when deciding whether to request kyc_link).
Useful? React with 👍 / 👎.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| issues: pipe( | ||
| array(unknown()), | ||
| transform((items) => | ||
| items | ||
| .filter((item): item is string => typeof item === "string") | ||
| .map((item) => parse(picklist(Issues), item)), | ||
| ), | ||
| ), | ||
| }), | ||
| }), | ||
| ), |
There was a problem hiding this comment.
Bug: The /quote endpoint calls bridge.getCustomer() without a try/catch block. An unknown issue or requirement from the external Bridge API will cause an unhandled exception.
Severity: HIGH
Suggested Fix
Wrap the call to await bridge.getCustomer(credential.bridgeId) within the /quote endpoint in a try/catch block. This will handle potential schema validation errors gracefully, similar to how errors are handled in the / endpoint, preventing 500 errors for users.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: server/utils/ramps/bridge.ts#L884-L898
Potential issue: The schema for `issues` and `missing` fields from the Bridge API was
made strict, causing `valibot` to throw an error for any unknown values. While the `/`
endpoint handles potential errors from `bridge.getProvider()`, the `/quote` endpoint
calls `bridge.getCustomer()` directly without any error handling. If the external Bridge
API introduces a new issue or requirement type, this will cause an unhandled exception,
resulting in a 500 error for users attempting to get a quote.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01ae626996
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| items | ||
| .filter((item): item is string => typeof item === "string") | ||
| .map((item) => parse(picklist(Issues), item)), |
There was a problem hiding this comment.
Parsing each requirements.issues entry with parse(picklist(Issues), item) now throws whenever Bridge introduces a new issue token, which makes getCustomer fail hard instead of returning customer data. Fresh evidence in this commit: getCustomer is also used in the webhook pairing flow (server/hooks/bridge.ts around getCustomer(bridgeId)), where this rejection is not caught, so a single new upstream issue code can turn customer.updated.status_transitioned events into 500s and prevent automatic credential pairing.
Useful? React with 👍 / 👎.
| union([ | ||
| picklist(Requirements), | ||
| object({ all_of: array(MissingRequirement) }), | ||
| object({ any_of: array(MissingRequirement) }), |
There was a problem hiding this comment.
Bug: The agreementLink function can generate a malformed URL by unconditionally appending &redirect_uri instead of checking if a ? is needed first.
Severity: MEDIUM
Suggested Fix
Refactor the agreementLink function to correctly handle URL query parameter construction. Use URLSearchParams to build the query string and conditionally append it with a ? only if parameters exist, similar to the implementation of the getKYCLink function.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: server/utils/ramps/bridge.ts#L862
Potential issue: The `agreementLink` function unconditionally appends
`&redirect_uri=...` to the URL returned from the `/customers/tos_links` API endpoint.
This assumes the base URL already contains a query string. If the API returns a bare URL
without a `?`, such as `https://tos.link`, the resulting URL becomes malformed (e.g.,
`https://tos.link&redirect_uri=...`). This will cause broken Terms of Service links in
the UI for users in the "NOT_STARTED" state when a `redirectURL` is provided.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3319935c46
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| pending: array(string()), | ||
| missing: nullish(unknown()), | ||
| issues: array(union([string(), unknown()])), | ||
| missing: nullish(MissingRequirement), |
There was a problem hiding this comment.
Parsing requirements.missing with nullish(MissingRequirement) now throws whenever Bridge returns a requirement token or nested shape outside this hardcoded list, which turns a schema drift into request failures. Fresh evidence in this commit: the bridge quote/deposit path calls bridge.getCustomer(credential.bridgeId) (server/api/ramp.ts, GET /quote) without wrapping parse failures, so a single new missing value can produce 500s for existing customers instead of a degraded but usable response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@chatgpt-codex-connector could you elaborate more about it? what is the hard coded list? give me one example also
There was a problem hiding this comment.
To use Codex here, create a Codex account and connect to github.
2 participants
Summary by CodeRabbit