[FEATURE] Add Bridge BYOK provider support#2
[FEATURE] Add Bridge BYOK provider support#2falcononrails wants to merge 2 commits intocdinnison:mainfrom
Conversation
|
@falcononrails is attempting to deploy a commit to the Otherness Team on Vercel. A member of the Team first needs to authorize it. |
d116da7 to
e3000a0
Compare
cdinnison
left a comment
There was a problem hiding this comment.
Thanks for this contribution — really well-structured PR overall. The provider abstraction is clean, the Bridge client is well-factored (injectable fetch, pagination, proper error class), and the incremental approach of wrapping Plaid rather than moving it is the right call for keeping this reviewable.
Test coverage is solid — the deleted transaction, incremental sync cursor, and reconnect state tests are thorough. The import-existing-items flow is a nice UX touch.
A few things to address before merging — see inline comments. The main ones are:
- Stale provider_state for reconnect messages — the daily-sync reconnect block reads pre-sync state and is redundant with
result.message getItemfallback catches all errors — should scope to 404formatMoneytests were gutted — need to still verify actual output for default locale- Inline
import()types — should use top-level imports bridgeClientIdmasked as password — client IDs aren't secret
Also a couple of non-blocking notes on currency formatting scope and the polling timeout.
| if (result.reconnectRequired && providerKey === "bridge") { | ||
| const state = parseProviderState<BridgeProviderState>(inst.provider_state); | ||
| const reconnectMessage = describeBridgeProviderState(state); | ||
| if (reconnectMessage) console.log(` ${reconnectMessage}`); |
There was a problem hiding this comment.
This block is redundant — syncBridgeInstitution already sets result.message with the reconnect description when reconnectRequired is true (see index.ts:405). And it has a bug: it reads the pre-sync inst.provider_state rather than the state that was just updated in the DB by syncInstitution.
Suggestion: drop this entire if (result.reconnectRequired && providerKey === "bridge") block. The result.message line above it already handles this.
There was a problem hiding this comment.
Fixed in 3f06cc9. I removed that redundant reconnect block from daily sync so it now relies on provider sync result messaging only.
| } | ||
|
|
||
| const accessToken = await client.ensureAccessToken(externalUserId); | ||
| const item = await client.getItem(accessToken, institution.item_id).catch(async () => { |
There was a problem hiding this comment.
This .catch() swallows all errors — network failures, auth errors, rate-limits — and falls back to listing all items. If getItem fails for a non-404 reason, listItems will likely fail too, just with a more confusing error.
Suggestion: scope the catch to 404 only:
const item = await client.getItem(accessToken, institution.item_id).catch(async (err) => {
if (!(err instanceof BridgeApiError && err.status === 404)) throw err;
const items = await client.listItems(accessToken);
// ...
});There was a problem hiding this comment.
Fixed in 3f06cc9. The getItem fallback now only catches BridgeApiError 404; all other errors are re-thrown.
| } | ||
|
|
||
| function upsertBridgeInstitution( | ||
| db: import("../types.js").Database, |
There was a problem hiding this comment.
Database and InstitutionRecord are used throughout this file via inline import("../types.js") annotations (~10 occurrences). Since InstitutionProvider is already imported from types.js at the top, just add them to that import:
import type { InstitutionProvider, Database, InstitutionRecord } from "../types.js";There was a problem hiding this comment.
Fixed in 3f06cc9. I imported Database and InstitutionRecord at the top-level type import and removed inline import() type annotations.
| }, | ||
| ): Promise<BridgeItem[]> { | ||
| const previousById = new Map(options.previousItems.map(item => [String(item.id), item])); | ||
| const deadline = Date.now() + 10 * 60 * 1000; |
There was a problem hiding this comment.
Non-blocking: 10-minute polling at 3-second intervals with no escape hatch — if the user abandons the Bridge Connect flow, they're stuck waiting. Consider printing a hint like "Press Ctrl+C to cancel" alongside the spinner, or shortening the timeout.
There was a problem hiding this comment.
Addressed in 3f06cc9. Added an explicit "Press Ctrl+C to cancel waiting" hint before polling Bridge Connect completion.
| }, | ||
| { | ||
| theme, | ||
| type: "password", |
There was a problem hiding this comment.
Client IDs aren't secret — this should use type: "input" instead of type: "password" so the user can see what they're typing. Keep bridgeClientSecret as password.
There was a problem hiding this comment.
Fixed in 3f06cc9. bridgeClientId prompt now uses type: input (bridgeClientSecret remains password).
| it("formats negative as absolute", () => expect(formatMoney(-99)).toBe("$99.00")); | ||
| it("formats zero", () => expect(formatMoney(0)).toBe("$0.00")); | ||
| it("formats positive", () => { | ||
| expect(formatMoney(1234.5)).toMatch(/\d/); |
There was a problem hiding this comment.
These assertions are too weak now — toMatch(/\d/) and toContain("1") pass for almost any string containing a number. The currency formatting change is fine, but the tests should still verify the default output.
Suggestion: set config.displayLocale = "en-US" and config.displayCurrency = "USD" in the test (with afterEach cleanup) and assert toBe("$1,234.50"). That way you're testing the formatting logic, not just that a digit exists.
There was a problem hiding this comment.
Fixed in 3f06cc9. I strengthened formatMoney tests by forcing en-US/USD in test setup and asserting exact outputs ($1,234.50, $99.00, $0.00), with cleanup after each test.
| @@ -0,0 +1,109 @@ | |||
| import { config } from "./config.js"; | |||
There was a problem hiding this comment.
Non-blocking: this module is a nice addition, but it changes output for all users (not just Bridge), and the displayLocale/displayCurrency config options aren't exposed in ray setup. Users can only set them via env vars or editing the config file directly. Consider either adding them as optional fields in setup, or at least mentioning the env vars (RAY_DISPLAY_LOCALE, RAY_DISPLAY_CURRENCY) in ray doctor or help output so they're discoverable.
There was a problem hiding this comment.
Addressed in 3f06cc9. I added display format discoverability in ray doctor, including explicit mention of RAY_DISPLAY_LOCALE and RAY_DISPLAY_CURRENCY.
|
@cdinnison I pushed follow-up commit 3f06cc9 addressing all inline review items.
Verified with npm run build and npm test. |
3f06cc9 to
fcc60af
Compare
|
Thank you! I've been out sick so will review ASAP. appreciate the follow-ups |
Summary
What changed
ray setupconfig to store optional Bridge BYOK credentials and a reusable default Bridgeexternal_user_idray linkprovider-aware, with Bridge support for:external_user_idprovider,provider_user_id, andprovider_stateaccountsandtransactionstablesray accountsandray doctorNotes
src/plaid/*implementation is intentionally left in place for this PR; the provider boundary is added here, and moving Plaid fully undersrc/providers/should be done in a follow-up refactor to keep this diff reviewable and lower-riskDiscussion
Validation
npm run buildnpm test