Conversation
🦋 Changeset detectedLatest commit: 22a957e 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 persona email-based credential fallback for bridge webhook handling, idempotent Ramp customer creation with retry-on-timeout/5xx, a Persona accounts search helper, a unique constraint on Changes
Sequence DiagramsequenceDiagram
participant Client as Client/API
participant Hook as Bridge Hook
participant DB as Database
participant Persona as Persona API
participant Sentry as Sentry/Events
Client->>Hook: webhook customer.updated.status_transitioned
Hook->>DB: lookup credential by account/bridgeId
alt credential found
Hook-->>Client: proceed with credential
else credential missing
Hook->>Hook: getCustomer(bridgeId)
alt customer found
Hook->>Persona: searchAccounts(customer.email)
Persona-->>Hook: accounts list
alt single match (reference-id)
Hook->>DB: update credentials where id==reference-id set bridgeId
DB-->>Hook: update success / constraint error
alt update success
Hook->>Sentry: captureEvent("bridge credential paired")
Hook-->>Client: return updated credential
else unique constraint (already paired)
Hook->>Sentry: captureEvent("bridge credential already paired")
Hook-->>Client: credential not found
end
else multiple matches
Hook->>Sentry: captureException(multiple matches)
Hook-->>Client: credential not found
else no matches
Hook->>Sentry: captureException(credential not found)
Hook-->>Client: credential not found
end
else customer missing
Hook->>Sentry: captureException(credential not found)
Hook-->>Client: credential not found
end
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)
📝 Coding Plan
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 |
Summary of ChangesHello, 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 significantly improves the robustness of the server's Bridge integration. It introduces a fallback mechanism for identifying user credentials when Bridge webhooks are received, leveraging Persona accounts to match users by email if a direct ID lookup fails. Additionally, it adds retry capabilities to Bridge API calls, specifically for customer creation, to handle transient network issues or server errors more gracefully. These changes aim to reduce credential lookup failures and improve the reliability of user onboarding with Bridge. 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. Footnotes
|
![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
This pull request introduces a robust fallback mechanism for bridge credentials and enhances the createCustomer process with retry logic and idempotency. The changes include importing necessary utilities, modifying the bridge.ts webhook to attempt to find credentials via Persona email if the initial lookup fails, and updating the bridge.ts utility to use withRetry for customer creation. Comprehensive test cases have been added to cover the new fallback scenarios, ensuring the reliability of the system. The updates improve the resilience and error handling of the bridge integration.
![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.
|
✅ All tests passed. |
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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| .then(([updated]) => { | ||
| if (!updated) throw new Error("credential already paired"); |
There was a problem hiding this comment.
🟡 Nonexistent credential misreported as "already paired" with fatal severity
When the persona searchAccounts fallback returns a reference-id that doesn't correspond to any credential in the database, the UPDATE at server/hooks/bridge.ts:120-122 returns 0 rows because eq(credentials.id, referenceId) matches nothing. The destructured updated is undefined, so line 125 throws "credential already paired". The .catch at line 136 matches this message and logs a captureEvent at level "fatal" with message "bridge credential already paired" — even though no credential was found at all.
This conflates two distinct failure modes: (a) the credential exists but already has a bridgeId (true "already paired"), and (b) no credential with that referenceId exists. In case (b), a fatal-level Sentry event is created with an incorrect message, which will generate false alerts and hamper debugging in production. The test at line 360-384 exercises this exact path without asserting on the misleading captureEvent.
Prompt for agents
In server/hooks/bridge.ts, lines 124-125, the code assumes that 0 rows returned from the UPDATE means the credential is "already paired", but it could also mean the credential with that referenceId doesn't exist at all. To fix this, after getting 0 rows from the UPDATE, perform an additional check (e.g., a SELECT to see if a credential with that referenceId exists). If it doesn't exist, either return undefined silently or log a different, more accurate message (e.g., "credential not found for reference-id"). Only log "bridge credential already paired" at fatal level when you can confirm the credential exists but already has a bridgeId.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!credential && payload.event_type === "customer.updated.status_transitioned") { | ||
| credential = await getCustomer(bridgeId) | ||
| .then((customer) => (customer ? searchAccounts(customer.email) : undefined)) | ||
| .then((accounts) => { | ||
| if (accounts && accounts.length > 1) | ||
| captureException(new Error("multiple persona accounts found"), { | ||
| level: "fatal", | ||
| contexts: { details: { bridgeId, matches: accounts.length } }, | ||
| }); | ||
| return accounts?.length === 1 ? accounts[0]?.attributes["reference-id"] : undefined; | ||
| }) | ||
| .then((referenceId) => | ||
| referenceId | ||
| ? database | ||
| .update(credentials) | ||
| .set({ bridgeId }) | ||
| .where(and(eq(credentials.id, referenceId), isNull(credentials.bridgeId))) | ||
| .returning({ account: credentials.account, source: credentials.source }) | ||
| .then(([updated]) => { | ||
| if (!updated) throw new Error("credential already paired"); | ||
| captureEvent({ | ||
| message: "bridge credential paired", | ||
| level: "warning", | ||
| contexts: { details: { bridgeId, referenceId } }, | ||
| }); | ||
| return updated; | ||
| }) | ||
| .catch((error: unknown): undefined => { | ||
| if ( | ||
| !( | ||
| (error instanceof Error && error.message === "credential already paired") || | ||
| (error instanceof DrizzleQueryError && | ||
| error.cause && | ||
| "code" in error.cause && | ||
| error.cause.code === "23505") | ||
| ) | ||
| ) | ||
| throw error; | ||
| captureEvent({ | ||
| message: "bridge credential already paired", | ||
| level: "fatal", | ||
| contexts: { details: { bridgeId } }, | ||
| }); | ||
| }) | ||
| : undefined, | ||
| ); |
There was a problem hiding this comment.
🚩 Fallback chain errors propagate to Hono's error handler
The fallback logic in server/hooks/bridge.ts:107-151 calls external services (getCustomer, searchAccounts) that can throw on network errors or API failures. These exceptions are not caught locally and would propagate to Hono's default error handler, returning a 500 to Bridge. This is likely acceptable since Bridge would retry the webhook on non-200 responses, giving a natural retry mechanism for transient failures. However, a persistent Persona API outage would cause repeated 500s and potentially exhaust Bridge's retry budget for the webhook.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
not relevant, as it is the intended behavior
|
|
||
| const CustomerResponse = object({ | ||
| id: string(), | ||
| email: string(), |
There was a problem hiding this comment.
🚩 email field added as required to CustomerResponse schema
The CustomerResponse valibot schema at server/utils/ramps/bridge.ts:701 now includes email: string() as a required field. This means all Bridge API responses parsed through this schema must include an email field, or the parse/safeParse call will throw. This affects getCustomer, getProvider (which calls getCustomer), and indirectly the hook fallback. If Bridge ever returns a customer response without an email field, this would be a runtime failure. Worth verifying against Bridge's API documentation that email is always present.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
can you check the api doc?
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22a957ed07
ℹ️ 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 (!credential && payload.event_type === "customer.updated.status_transitioned") { | ||
| credential = await getCustomer(bridgeId) |
There was a problem hiding this comment.
When a customer.updated.status_transitioned webhook arrives for an unpaired Bridge customer, this branch will persist bridgeId before we inspect payload.event_object.status. That is dangerous for terminal statuses like rejected, paused, or offboarded: once the credential is paired, bridge.onboarding() immediately returns ALREADY_ONBOARDED for any non-null customerId (server/utils/ramps/bridge.ts:299-300), and getProvider() maps those same statuses to NOT_AVAILABLE (server/utils/ramps/bridge.ts:184-193). In other words, a rejected/offboarded webhook that used to be ignored now permanently blocks the user from starting a fresh Bridge onboarding flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this is a fallback for the user creation endpoint, doesn't matter the customer status
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11359394-81ab-417e-9df2-87898b74896d
📒 Files selected for processing (8)
.changeset/twco-dcxi-kedl.mdserver/database/schema.tsserver/hooks/bridge.tsserver/test/api/ramp.test.tsserver/test/hooks/bridge.test.tsserver/test/utils/bridge.test.tsserver/utils/persona.tsserver/utils/ramps/bridge.ts
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Data Integrity
Tests