Conversation
Implements account deletion and data export functionalities. Adds routes and pages for privacy policy and terms of service. Includes agreement links on the signup page and updates dependencies.
Adds reconnection flow for Plaid institutions when credentials expire. Improves Plaid webhook verification by implementing JWT verification. Enables user data export and account deletion, respecting user privacy. Adds institution status tracking for better error handling and user guidance.
WalkthroughAdds Plaid update‑mode reauth and consent flows, ES256 JWT Plaid webhook verification, user export and account deletion, Institution status/errorCode with migration, Privacy/Terms pages and footer/signup links, dialog scroll constraints, UI/animation and amount-sign convention changes, CI/deploy configs, and Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
src/user/operations.ts (1)
15-78: Clarify behavior when external cleanup fails (subscriptions and bank links)Right now, account deletion proceeds even if external cleanup fails (subscriptions or linked financial items), which can leave:
- Active paid subscriptions with no corresponding user record.
- Linked financial items not fully removed on the provider’s side.
That may be acceptable by design (e.g., “never block deletion”), but it’s a meaningful trade-off. Consider either:
- Treating hard failures in external cleanup as a 500 that stops deletion, or
- At minimum, surfacing a clear message to the user and/or recording an internal “manual cleanup required” signal so you can reconcile orphaned subscriptions/items later.
src/ProfilePage.tsx (1)
78-99: Separate deletion vs. logout/navigation failures and resetisDeletingmore defensivelyIn
handleDeleteAccount, any error in the whole block (deletion, toast, logout, navigate) lands in the same catch and is surfaced as “Deletion failed,” andisDeletingis only reset there. That means:
- If deletion succeeds but logout or navigation fails, the user sees a misleading “Deletion failed” message.
- In rare edge cases where navigation fails but the component stays mounted,
isDeletingwould remaintrueon the success path.Consider:
- Using a
finallyblock to always resetisDeleting, and- Narrowing the
tryto the actual delete call (and perhaps the success toast), handling logout/navigation errors separately so the messaging reflects which step failed.Also applies to: 164-199
.env.server.example (1)
1-70: Helpful env documentation; key ordering warnings are cosmeticThe added structure, examples, and Plaid production checklist are very useful and keep secrets out of version control. The dotenv-linter warnings about key ordering (
PLAID_ENVbeforePLAID_SECRET,STRIPE_PRICE_IDbeforeSTRIPE_PUBLISHABLE_KEY) are purely cosmetic; you can reorder those lines if you want a clean lint run, but behavior is unaffected.Based on static analysis hints
src/plaid/webhooks.ts (1)
27-30: Consider adding explicit webhook verification control.The development mode bypass is pragmatic for local testing, but consider adding an explicit environment variable (e.g.,
SKIP_WEBHOOK_VERIFICATION=true) to make this behavior more explicit and controllable across different environments.src/dashboard/components/plaid-link-button.tsx (1)
55-58: Tighten hook dependencies and callback structure aroundopenPlaidLink.
handleConsentAcceptandhandleOpencallopenPlaidLinkbut the callbacks are declared beforeopenPlaidLinkand have empty dependency arrays. This works today (the closure will see the variable at call time), but it will trip exhaustive-deps lint and can become stale if any of the props used insideopenPlaidLinkever change (e.g. newonError/onEventbehavior). Consider either:
- Wrapping
openPlaidLinkitself inuseCallbackand listing it in the consent handlers’ dependency arrays, or- Inlining the minimal logic they need, so their deps remain accurate.
This keeps React hook usage idiomatic and future‑proof.
Also applies to: 72-77, 117-121, 123-135, 137-203
src/plaid/components/plaid-consent-dialog.tsx (1)
25-36: Avoid double-callingonDeclinefrom bothonOpenChangeand the Cancel button.Right now
onDeclineis invoked viaonOpenChangewhenopenbecomes false and also directly on the Cancel button. That’s fine for the current parent (idempotent close), but it’s an API footgun ifonDeclineever does more than toggle state. Consider routing the Cancel button through the dialog’s close mechanism (e.g.DialogClose) and keeping only one code path that triggersonDecline.Also applies to: 84-118
src/dashboard/components/plaid-integration.tsx (1)
72-73: Reauth flow wiring looks good; align types to avoidas anyand clarify loading semantics.The reconnection flow (
reconnectingInstitutionId,handleReconnectSuccess/Error, and theneeds_reauthalert) is well wired tocreateUpdateModeLinkToken/exchangeUpdateModeToken, and the per‑institution loading state viareconnectingInstitutionIdis clear. Two follow‑ups worth considering:
- Instead of
(institution as any)?.status, extend the shared institution type (GetInstitutionsResultandInstitutionsSummaryProps) to includestatusanderrorCode. That keeps reauth UI type‑safe and avoidsanyleaks.- The
setIsLoadingprop passed intoPlaidLinkButtoncurrently assumes it will always be called with a boolean (not a functional updater). If you ever changePlaidLinkButtonto use functional updates (setIsLoading(prev => !prev)), this wrapper will misinterpret the argument; documenting or tightening that contract now can prevent surprises later.Also applies to: 181-207, 291-340
src/plaid/operations.ts (5)
103-159: Update-mode link token flow is correct; add a small guard aroundaccessToken.
createUpdateModeLinkTokencorrectly verifies user ownership, decrypts the storedaccessToken, and reuses_internalCreateLinkTokenwith the decrypted token to enter Plaid’s update mode, with consistentHttpErrorhandling. One improvement: if there’s any chance of legacy rows with a missing or malformedaccessToken, add an explicit check beforedecrypt(e.g. 404/409 or a clearer 500) so failures are easier to distinguish from general Plaid errors in logs.
256-334: Exchange-update flow is sound; consider whether to trigger an immediate sync after reauth.
exchangeUpdateModeTokendoes the right checks (auth, institution ownership), reuses_internalExchangePublicToken, and updates the institution with the new encryptedaccessToken,itemId, and resetsstatus/errorCode. Currently it stops there and relies on a later sync to pull fresh transactions. If you want “Reconnect” to immediately refresh data, you might optionally callsyncTransactionsfor that institution in this action (or from the caller) once the credentials are updated.
531-595: Reauth vs error status handling in_syncSingleInstitutionis a good pattern; verify Plaid error codes.Marking institutions as
needs_reauthvserrorbased onerror.response?.data?.error_code || error.error_codeand persistingstatus/errorCodeis a solid way to drive the re‑connect UI. I’d just recommend double‑checking the list ofrequiresReauthcodes against Plaid’s latest docs—some codes likeINSTITUTION_DOWNorINSTITUTION_NOT_RESPONDINGmay be transient outages rather than genuine reauth requirements, and you might prefer to treat them aserrorinstead ofneeds_reauth.
720-723: Deleting the Plaid item before DB deletion is good; tighten error logging and null handling.The new
deleteInstitutionlogic that decryptsaccessTokenand callsplaidClient.itemRemovebefore deleting from the database is the right direction and correctly doesn’t block deletion on Plaid failures. Two small hardening tweaks to consider:
- If
institution.accessTokencan ever be null/empty (e.g. legacy data), short‑circuit the Plaid call instead of attemptingdecryptand relying on the catch.- When logging Plaid errors, ensure no sensitive token fragments are ever included (the current code logs
response?.data/message, which is usually safe but worth keeping an eye on).Also applies to: 734-753, 754-773
1146-1154: UpdateGetInstitutionsResultto includestatus/errorCodeinstead of relying onas anyin the UI.
getInstitutionsnow selectsstatusanderrorCodein the Prisma query, butGetInstitutionsResultstill only exposes id/name/sync/logo/accounts. On the frontend (PlaidIntegration), this forces a cast(institution as any).statusto drive the reauth alert. It would be cleaner to extendGetInstitutionsResult(and downstream props likeInstitutionsSummaryProps) to includestatusanderrorCodeso the reauth UI is fully type‑safe and you get autocomplete/TS checks for these new fields.Also applies to: 1168-1178
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.env.server.example(1 hunks)main.wasp(4 hunks)migrations/20251120044613_add_institution_status_fields/migration.sql(1 hunks)package.json(1 hunks)schema.prisma(1 hunks)src/ProfilePage.tsx(3 hunks)src/auth/auth.tsx(1 hunks)src/dashboard/components/plaid-integration.tsx(5 hunks)src/dashboard/components/plaid-link-button.tsx(6 hunks)src/legal/PrivacyPolicyPage.tsx(1 hunks)src/legal/TermsOfServicePage.tsx(1 hunks)src/plaid/components/plaid-consent-dialog.tsx(1 hunks)src/plaid/operations.ts(14 hunks)src/plaid/service.ts(2 hunks)src/plaid/webhooks.ts(3 hunks)src/root/components/footer.tsx(4 hunks)src/transactions/components/transaction-detail-dialog.tsx(1 hunks)src/transactions/components/transactions-list.tsx(1 hunks)src/user/operations.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/legal/PrivacyPolicyPage.tsx (1)
src/motion/transitionPresets.tsx (1)
fadeIn(74-84)
src/legal/TermsOfServicePage.tsx (1)
src/motion/transitionPresets.tsx (1)
fadeIn(74-84)
src/dashboard/components/plaid-link-button.tsx (1)
src/plaid/components/plaid-consent-dialog.tsx (1)
PlaidConsentDialog(18-130)
src/user/operations.ts (2)
src/plaid/utils/encryption.ts (1)
decrypt(32-52)src/plaid/client.ts (1)
plaidClient(34-34)
src/plaid/webhooks.ts (1)
src/plaid/client.ts (1)
plaidClient(34-34)
src/ProfilePage.tsx (1)
src/user/operations.ts (2)
exportUserData(97-180)deleteUserAccount(7-95)
src/plaid/components/plaid-consent-dialog.tsx (2)
src/client/components/ui/dialog.tsx (6)
Dialog(112-112)DialogContent(117-117)DialogHeader(118-118)DialogTitle(120-120)DialogDescription(121-121)DialogFooter(119-119)src/client/components/ui/button.tsx (1)
Button(63-63)
src/dashboard/components/plaid-integration.tsx (2)
src/dashboard/components/plaid-link-button.tsx (1)
PlaidLinkButton(61-239)src/plaid/operations.ts (2)
createUpdateModeLinkToken(114-159)exchangeUpdateModeToken(268-334)
src/plaid/operations.ts (3)
src/plaid/utils/encryption.ts (1)
decrypt(32-52)src/plaid/service.ts (2)
_internalCreateLinkToken(22-64)_internalExchangePublicToken(72-140)src/plaid/client.ts (1)
plaidClient(34-34)
🪛 dotenv-linter (4.0.0)
.env.server.example
[warning] 12-12: [UnorderedKey] The PLAID_ENV key should go before the PLAID_SECRET key
(UnorderedKey)
[warning] 27-27: [UnorderedKey] The STRIPE_PRICE_ID key should go before the STRIPE_PUBLISHABLE_KEY key
(UnorderedKey)
🔇 Additional comments (16)
src/transactions/components/transaction-detail-dialog.tsx (2)
33-36: LGTM! Amount styling logic correctly implements Plaid convention.The implementation correctly treats positive amounts as expenses (red with minus sign) and negative amounts as income (green with plus sign), aligning with Plaid's transaction amount convention. The logic is clear and the derived values (color and sign) are used consistently.
40-40: Good UX enhancement for scrollable content.Adding vertical scrolling with a 90vh max height ensures transaction details remain accessible on smaller screens or when there are many details to display.
src/auth/auth.tsx (1)
56-67: Signup TOS/Privacy notice looks good; keep routes in syncThe inline consent copy and links to Terms of Service and Privacy Policy are a solid addition. Just ensure
/termsand/privacyremain aligned with the routes defined in your routing config/main.wasp if paths change later.src/user/operations.ts (1)
127-172: Export payload looks well-scoped and avoids obvious secretsThe export shape sensibly:
- Includes user metadata and related institutions/accounts/transactions.
- Omits sensitive fields like access tokens, encryption keys, and any Stripe identifiers.
As the schema evolves, just keep this list in sync so you don’t accidentally add new sensitive fields to the export.
src/ProfilePage.tsx (1)
45-76: Data export handler is straightforward and user-friendlyThe export handler is well-structured: guards with
isExporting, awaits the operation, generates a JSON blob with pretty-print, triggers a download, and cleans up the object URL, with clear success/error toasts. This looks good as-is.migrations/20251120044613_add_institution_status_fields/migration.sql (1)
1-3: LGTM!The migration correctly adds the
statusanderrorCodecolumns to support Plaid re-authentication flows. The syntax is valid, the default value aligns with the schema, and the nullability matches the intended design.schema.prisma (1)
35-36: LGTM!The new
statusanderrorCodefields are well-designed to support Plaid re-authentication flows. The inline comments provide clear documentation of expected values, and the default "active" status is appropriate for new connections.src/root/components/footer.tsx (1)
40-43: LGTM!The legal navigation links are well-integrated into both mobile and desktop layouts. The implementation is consistent, and the support email provides a clear contact method for users.
Also applies to: 112-130, 163-181
src/plaid/service.ts (1)
22-52: LGTM!The Update Mode implementation correctly handles both new connections and re-authentication flows. The conditional logic appropriately configures the Link Token request for each scenario, and the comments clearly explain the two modes.
src/legal/TermsOfServicePage.tsx (1)
5-23: Well-structured Terms of Service page.The page uses appropriate animations and provides comprehensive coverage of terms. The dynamic "Last Updated" date is a good practice for maintaining transparency with users.
src/legal/PrivacyPolicyPage.tsx (1)
4-360: LGTM!The Privacy Policy is comprehensive and well-structured. It appropriately covers data collection, usage, security measures, and user rights. The inclusion of GDPR and CCPA-specific sections demonstrates compliance awareness.
src/plaid/webhooks.ts (1)
105-117: LGTM!The JWT token extraction and validation correctly checks for the presence of the
Plaid-Verificationheader and returns appropriate error responses. The async verification call is properly awaited.src/transactions/components/transactions-list.tsx (1)
292-293: The code change is correct and aligns with Plaid's API documentation.Plaid's Transactions API specifies that amount is positive when money moves out of the account (debits) and negative when money moves into the account (credits). The updated logic correctly interprets this:
transaction.amount > 0identifies expenses (outflows), and negative amounts represent income (inflows). The comment accurately documents this convention.src/dashboard/components/plaid-integration.tsx (1)
454-483: Manage Accounts dialog implementation looks solid.The account management dialog correctly scopes accounts to the selected institution, guards with
?? [], and delegates toggling tohandleToggleAccountwhile keeping UI minimal. No issues from a correctness or UX standpoint.main.wasp (1)
51-52: New routes and actions are wired consistently with existing Wasp config.Privacy/Terms routes and pages, Plaid update‑mode actions, and user delete/export operations all follow the existing configuration patterns and entity wiring, so from a routing/API surface perspective this looks good.
Also applies to: 102-108, 119-122, 129-132, 208-223
src/plaid/operations.ts (1)
831-832: Spending queries now treatamount > 0as “expense only”; confirm this matches all account types.
getSpendingSummary,getMonthlySpending, andgetCategorySpendingnow filter onamount.gt(0)and aggregate the rawamountassuming “positive = money out”. That matches Plaid’s documented convention for most cases, but credit products and certain edge cases can sometimes be counter‑intuitive. It’s worth sanity‑checking with a few real transactions (debits, refunds, card payments) to confirm that what users perceive as “spending” always shows up as positive here.Also applies to: 855-857, 961-983, 1039-1065
Updates Node.js version and enhances webhook verification for better security by using timing-safe comparison to prevent timing attacks. Also, updates Terms of Service to use Washington, USA as the governing law. Improves UI layout of the subscription page and updates transaction list animation for better UX. Changes error messages to be more generic when deleting user accounts and exporting user data.
Improves the user experience by adding scrollability to dialogs and alerts, preventing content overflow. Adds immediate transaction syncing after re-authentication to keep data up-to-date. Handles edge cases in Plaid operations, such as missing access tokens, to prevent errors and improve reliability. Refactors error handling to better manage Plaid API issues during institution deletion.
Specifies the required Node.js version. Sets up Wasp by installing it and adding its binary directory to the PATH. This ensures that Wasp commands can be run from the command line.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dashboard/components/plaid-integration.tsx (1)
79-115:handleConnectionSuccessmustawait refetch()to properly handle errors and timingThe issue is confirmed. Line 94 calls
refetch()withoutawaitin anasyncfunction withtry/catch/finally. This means:
- Errors from
refetch()won't be caught by thecatchblock- The
finallyblock (which clears the spinner and shows the success toast) runs immediately instead of waiting for the refetch to complete- The success toast displays before data actually refreshes
The fix is correct—add
await:try { - refetch() + await refetch()This aligns with the correct pattern already used in
handleReconnectSuccessat line 185. Note: the code comment at line 78 claims the handler was "modified to await refetch," but it hasn't been applied yet.
🧹 Nitpick comments (7)
src/legal/TermsOfServicePage.tsx (1)
161-175: Consider linking directly to Plaid’s Terms and Privacy Policy.The text says users agree to Plaid’s Terms of Service and Privacy Policy but doesn’t link to them, which is less user‑friendly and may not fully satisfy partner expectations.
You could inline links, e.g.:
- <li>You agree to Plaid's Terms of Service and Privacy Policy</li> + <li> + You agree to Plaid's{' '} + <a + href='https://plaid.com/legal/#end-user-terms-of-use' + target='_blank' + rel='noreferrer' + > + Terms of Service + </a>{' '} + and{' '} + <a + href='https://plaid.com/legal/#end-user-privacy-policy' + target='_blank' + rel='noreferrer' + > + Privacy Policy + </a> + </li>(Adjust URLs/attributes to match your legal/partner guidance.)
src/transactions/components/transactions-list.tsx (2)
238-255: Group expand/collapse animation structure looks good; consider minor a11y polishThe animated expand/collapse container for each transaction group is composed correctly: conditional rendering, presence-based exit animation, and overflow clipping all line up, and the timing/easing are reasonable for production.
One optional improvement: consider respecting the user’s
prefers-reduced-motionsetting (e.g., skip or greatly shorten these height/opacity transitions when reduced motion is requested) to avoid unnecessary motion for sensitive users.Also applies to: 342-345
269-340: Plaid-style amount sign/color logic is clear; double-check upstream normalization & zero handlingThe per-row container change is fine, and the inline amount logic is easy to read. Mapping
transaction.amount > 0to expenses (red,-sign) and<= 0to income (green,+sign) matches the intended Plaid convention and keeps formatting straightforward.Two small follow-ups to verify/consider:
- Ensure you’re not already flipping the sign when building
TransactionWithDetails; otherwise this would double-invert and mislabel income vs. expense.- Decide how you want to treat
amount === 0transactions (currently they render as+$0.00in green), in case you ever surface zero-amount holds/reversals and want them styled as neutral instead.Both are minor, but clarifying now will prevent subtle UX inconsistencies later.
src/dashboard/components/plaid-integration.tsx (1)
72-73: Update‑mode reconnection flow and per‑institution loading state look solid (double‑check Wasp action usage)The new
reconnectingInstitutionIdstate,handleReconnectSuccess/handleReconnectErrorhandlers, and theneeds_reauthalert wiring intoPlaidLinkButtonwithisUpdateMode={true}form a coherent reconnection flow:
- Ownership and institution ID are passed down cleanly to
createUpdateModeLinkTokenandexchangeUpdateModeToken.isLoading/setIsLoadingare correctly scoped per institution viareconnectingInstitutionId, so only the reconciling institution shows a spinner.- Reauth success triggers a refetch and user‑facing toast; failures clear loading and show an error toast.
One thing to verify: here you call
createUpdateModeLinkToken/exchangeUpdateModeTokenfromwasp/client/operationsdirectly inside the lambdas. In other places (e.g.,deleteInstitutionAction,toggleAccountTrackingAction, andSubscriptionPage.tsx) actions are invoked viauseAction. Ensure that in your Wasp version these operations are indeed directly callable on the client; if not, mirror theuseActionpattern and pass the resulting functions intoPlaidLinkButton.Also applies to: 181-207, 301-340
src/plaid/webhooks.ts (1)
6-10: JWT webhook verification flow is correct overall; consider a few hardening tweaksThe new flow (ES256 enforcement, JWK fetch via Plaid,
jwtVerify, 5‑minute window, and constant‑time SHA‑256 body hash comparison) looks aligned with Plaid’s JWT docs and fixes the earlier timing‑attack concern.A few small improvements to consider:
- Header decoding: Instead of manually splitting the JWT and doing
Buffer.from(headerB64, 'base64'), usejose’sdecodeProtectedHeader(jwtToken)so you get robust base64url handling and fewer parsing edge cases if the header format changes.iatvalidation: BeforecurrentTime - issuedAt, checktypeof payload.iat === 'number'; if it’s missing or not numeric, log andreturn falseso failures are explicit rather than falling through due toNaN.- Body hash canonicalization: You currently hash
JSON.stringify(req.body). Plaid’srequest_body_sha256is computed over a specific canonical form of the body; please double‑check that this matches their documented normalization. If not, capture the raw request body inplaidWebhookMiddlewareConfigFn(e.g., via theverifyoption on the JSON/body parser storingreq.rawBody) and pass that raw string intoverifyPlaidWebhookto avoid subtle hash mismatches.These changes would make the verifier more robust without altering the high‑level behavior.
Also applies to: 22-103, 117-127
src/user/operations.ts (2)
7-95: Account deletion flow is robust; add a small null guard for Plaid access tokensThe deletion operation does a good job of:
- Canceling Stripe subscriptions and deleting the customer best‑effort, without blocking account deletion on Stripe errors.
- Iterating the user’s institutions and calling Plaid
itemRemovefor each, logging but not failing the overall deletion on Plaid issues.- Relying on Prisma cascades to remove institutions, accounts, and transactions in one
User.deletecall.One minor improvement: in the Plaid cleanup loop you unconditionally call
decrypt(institution.accessToken). If an older row has a missing/emptyaccessToken,decryptwill throw, get caught, and log “Failed to decrypt data.” for every such institution. Functionally you still continue, but this adds noisy logs.Consider mirroring
deleteInstitutionand short‑circuiting:for (const institution of institutions) { try { if (!institution.accessToken) { console.warn( `Institution ${institution.id} has no accessToken, skipping Plaid item removal`, ) continue } const decryptedAccessToken = decrypt(institution.accessToken) await plaidClient.itemRemove({ access_token: decryptedAccessToken }) // ... } catch (plaidError: any) { // existing logging } }This keeps logs cleaner while preserving the current best‑effort behavior.
97-180: Data export shape is comprehensive and avoids sensitive fields (optional error behavior tweak)The export operation:
- Fetches user → institutions → accounts → transactions in one query.
- Returns a structured JSON export including identifiers and timestamps but omits secrets like Plaid access tokens and Stripe customer IDs.
- Provides a clear
exportedAtandexportVersionfor future evolution.Currently, a
HttpError(404, 'User not found')thrown inside thetryis caught and rethrown as a generic 500. Since this action is only callable by an authenticated user, that case should be extremely rare; but if you ever want the client to distinguish “user not found” from generic errors, you could mirror your Plaid operations pattern and rethrowHttpErrorinstances:} catch (error: any) { console.error('Error exporting user data:', error) if (error instanceof HttpError) throw error throw new HttpError(500, 'Failed to export data') }Not required for correctness, but it keeps semantically meaningful status codes if this edge case ever occurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/format.yml(1 hunks)src/SubscriptionPage.tsx(1 hunks)src/client/components/ui/alert-dialog.tsx(1 hunks)src/client/components/ui/dialog.tsx(1 hunks)src/dashboard/components/plaid-integration.tsx(5 hunks)src/legal/TermsOfServicePage.tsx(1 hunks)src/plaid/operations.ts(15 hunks)src/plaid/webhooks.ts(3 hunks)src/transactions/components/transactions-list.tsx(5 hunks)src/user/operations.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/legal/TermsOfServicePage.tsx (1)
src/motion/transitionPresets.tsx (1)
fadeIn(74-84)
src/dashboard/components/plaid-integration.tsx (2)
src/dashboard/components/plaid-link-button.tsx (1)
PlaidLinkButton(61-239)src/plaid/operations.ts (2)
createUpdateModeLinkToken(114-164)exchangeUpdateModeToken(273-348)
src/plaid/webhooks.ts (1)
src/plaid/client.ts (1)
plaidClient(34-34)
src/user/operations.ts (2)
src/plaid/utils/encryption.ts (1)
decrypt(32-52)src/plaid/client.ts (1)
plaidClient(34-34)
src/plaid/operations.ts (3)
src/plaid/utils/encryption.ts (1)
decrypt(32-52)src/plaid/service.ts (2)
_internalCreateLinkToken(22-64)_internalExchangePublicToken(72-140)src/plaid/client.ts (1)
plaidClient(34-34)
🔇 Additional comments (11)
src/client/components/ui/dialog.tsx (1)
41-41: Good overflow handling for long content.The addition of
max-h-[90vh]andoverflow-y-autoappropriately constrains dialog height and enables scrolling when content overflows.Please verify the close button remains visible and accessible when scrolling through long content, especially on mobile/smaller viewports.
src/client/components/ui/alert-dialog.tsx (1)
37-37: Consistent overflow handling applied.The same height constraint and scrolling behavior applied here maintains consistency with the dialog component changes.
.github/workflows/format.yml (1)
20-20: Node 22.x upgrade is compatible and production-safe.Node.js 22 entered Active LTS (production‑stable) on October 29, 2024. Verification confirms:
- jose 6.1.2 has no Node version constraints and works with Node 22.x
- All other dependencies are compatible with Node 22.x
- Only one workflow file exists (no consistency issues)
- No engines field is needed in package.json for this change to be functional
The upgrade from Node 20.x to 22.x is appropriate for this codebase.
src/transactions/components/transactions-list.tsx (1)
3-3: Animation import aligns with usage; just ensure dependency wiring is correctThe added import for the animation helpers matches how they’re used below in the component. As long as the corresponding animation package is installed and your bundler is configured for its new entrypoint, this should be fine.
src/SubscriptionPage.tsx (1)
123-176: Subscription layout container update looks goodWrapping the card in a full-width, flex-1 centered container with an inner
max-w-mdkeeps behavior unchanged while aligning layout with the rest of the app.src/dashboard/components/plaid-integration.tsx (1)
454-483: Manage Accounts dialog mapping is safe and resilientUsing
institutions?.find(... )?.accounts ?? []avoids crashes when the institution is missing and keeps theSwitchstrictly controlled viaaccount.isTrackedwith a single toggle handler, which is a clean, maintainable pattern.src/plaid/operations.ts (5)
103-164: Update Mode server actions correctly enforce auth and institution ownershipBoth
createUpdateModeLinkTokenandexchangeUpdateModeTokenlook solid:
- They require
context.userand validate theinstitutionIdis present and belongs to the current user before doing anything with Plaid.createUpdateModeLinkTokenensures an access token exists, decrypts it once, and passes it into_internalCreateLinkTokento generate an Update Mode link token.exchangeUpdateModeTokenreuses_internalExchangePublicTokento get a fresh encrypted access token and item ID, then updates the institution and resetsstatusto'active'witherrorCode: null.- Immediate post‑reauth sync is attempted but failures are logged and don’t cause the action itself to fail, which is the right trade‑off.
This is a clean, least‑privilege implementation for the new reconnection flows.
Also applies to: 261-348
545-555: New institutionstatus/errorCodehandling improves resilience and UXThe
_syncSingleInstitutionhelper now:
- Marks an institution as
'active'and clearserrorCodeafter a successful sync, so previously errored connections recover cleanly.- Examines Plaid
error_codevalues and, for known reauth‑required codes, setsstatus: 'needs_reauth', stores the code, and throws a 400HttpErrorinstructing the client to reconnect.- For other Plaid errors with an
error_code, setsstatus: 'error'and persists the code for debugging, while still throwing a 500 back to the caller.This gives the UI a clear, queryable source of truth about whether an institution is healthy, needs reauth, or has a non‑reauth error, and avoids tight retry loops by updating
lastSyncon error. Just keep therequiresReauthlist in sync with Plaid’s latest error codes over time.Also applies to: 563-610
737-772: Per‑institution Plaid/item/removeindeleteInstitutionis well‑behaved and non‑blockingEnhancing
deleteInstitutionto:
- Select
accessTokenwhen loading the institution,- Skip Plaid removal when there is no token, and
- Decrypt and call
plaidClient.itemRemovewith best‑effort error handling,ensures Plaid items are cleaned up for individual deletions while still allowing the database delete to proceed even if Plaid returns an error. This matches the behavior in
deleteUserAccountand aligns with Plaid’s data‑retention guidance.
851-852: Consistently filtering to positive Plaid amounts clarifies “spending” semanticsUpdating the spending queries to use:
amount: { gt: 0 }ingetSpendingSummary,getMonthlySpending, andgetCategorySpending, and- Using
tx.amountdirectly in aggregationmakes it explicit that “spending” is based only on outgoing transactions (Plaid convention: positive = money out, negative = money in). This avoids income inflows skewing charts and keeps aggregation logic simple and consistent across all three endpoints.
Also applies to: 875-877, 981-982, 1001-1003, 1059-1060, 1082-1085
1165-1169: ExtendingGetInstitutionsResultwithstatusanderrorCodesupports the new reconnection UXIncluding
statusanderrorCodeinGetInstitutionsResultand selecting them ingetInstitutionsgives the client exactly what it needs to:
- Show the “Reconnection required” banner for
status === 'needs_reauth'.- Potentially surface or log Plaid error codes for diagnostics.
This also keeps the type alias aligned with the Prisma select, so consuming components get a strongly‑typed view of these new fields.
Also applies to: 1196-1198
Updates dashboard to default to a 1-month view for a more immediate user experience. Modifies data fetching in the Plaid integration component to ensure data consistency after user actions. Updates the Privacy Policy and Terms of Service pages to use a static last updated date for better maintainability, related to work on account deletion and data export features.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/dashboard/DashboardPage.tsx (1)
51-67: InitialtimeRangeand Tabs default aligned to1mThe new
'1m'default is consistent with theTimeRangeunion and with theTabssetup, so behavior stays coherent with the chart props. SinceTabsis fully controlled viavalue={timeRange},defaultValueis effectively unused; you could optionally dropdefaultValue(or derive both from a shared constant) to reduce confusion.src/dashboard/components/plaid-integration.tsx (4)
139-179: Refetch after delete/toggle is correct; consider guarding repeated callsAwaiting
refetch()afterdeleteInstitutionActionandtoggleAccountTrackingActionkeeps the institutions/accounts list in sync with server state, which is production-friendly. As a future improvement, you might temporarily disable the delete button and switches while these calls are in flight (or batch multiple toggles into a single refetch) to avoid rapid-fire refetches on large account sets.
72-74: Update-mode reconnection flow is well-wired; tighten busy-state handlingThe new
reconnectingInstitutionIdstate plus thePlaidLinkButtonwrapper forcreateUpdateModeLinkToken/exchangeUpdateModeTokencorrectly scopes re-auth to a specific institution and surfaces clear success/error toasts, with a refetch on success. UsingisLoading={reconnectingInstitutionId === institution.id}and the customsetIsLoadingkeeps the update-mode button behavior aligned with the existing connect flow.Two optional robustness tweaks to consider:
- While an institution is reconnecting, it’s probably safer UX-wise to treat it as “busy” the same way you do when syncing. For example, derive a flag like
const isBusy = syncingInstitutionId === institution.id || reconnectingInstitutionId === institution.idand use that both to hide the action area and to drive thedisabledprops on the manage/delete buttons, so users can’t delete/manage an institution mid–re-auth.- Because
reconnectingInstitutionIdis a single scalar, starting a reconnect on institution B while A’s Plaid Link is still open will move the “Connecting…” state to B. If you ever care about multiple concurrent reconnects, switching this to a per-institution map/set would avoid that visual race.Overall the flow looks correct; these would just harden the UX around edge cases.
Also applies to: 181-207, 296-403
200-207: Unusederrorparameter inhandleReconnectError
handleReconnectErroraccepts anerrorargument but doesn’t use it. To keep linters happy and aid debugging, either log/inspect it (e.g., surface a more specific message when a status code is available) or rename to_errorto indicate intentional non-use.
454-483: Manage Accounts mapping is solid; refine Switch handler to usecheckedThe dialog’s account list safely handles missing institutions via
find(...)?.accounts ?? [], and the per-account layout looks good. The Switch currently callshandleToggleAccount(account.id, account.isTracked)and then flips that boolean inside the handler; relying on the capturedaccount.isTrackedis slightly more brittle than using thecheckedvalue from the control.You could make this more idiomatic and future-proof by letting the Switch drive the next state directly, e.g.:
-const handleToggleAccount = async ( - accountId: string, - currentlyTracked: boolean, -) => { +const handleToggleAccount = async ( + accountId: string, + nextTracked: boolean, +) => { await toggleAccountTrackingAction({ accountId, - isTracked: !currentlyTracked, + isTracked: nextTracked, }) await refetch() } -<Switch - checked={account.isTracked} - onCheckedChange={() => - handleToggleAccount(account.id, account.isTracked) - } /> +<Switch + checked={account.isTracked} + onCheckedChange={(checked) => + handleToggleAccount(account.id, checked) + } />Functionally it’s the same today, but it avoids stale-closure issues if the component tree or data flow gets more complex.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/format.yml(1 hunks)src/dashboard/DashboardPage.tsx(2 hunks)src/dashboard/components/plaid-integration.tsx(8 hunks)src/legal/PrivacyPolicyPage.tsx(1 hunks)src/legal/TermsOfServicePage.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/format.yml
🧰 Additional context used
🧬 Code graph analysis (3)
src/legal/TermsOfServicePage.tsx (1)
src/motion/transitionPresets.tsx (1)
fadeIn(74-84)
src/dashboard/components/plaid-integration.tsx (2)
src/dashboard/components/plaid-link-button.tsx (1)
PlaidLinkButton(61-239)src/plaid/operations.ts (2)
createUpdateModeLinkToken(114-164)exchangeUpdateModeToken(273-348)
src/legal/PrivacyPolicyPage.tsx (1)
src/motion/transitionPresets.tsx (1)
fadeIn(74-84)
🔇 Additional comments (5)
src/dashboard/components/plaid-integration.tsx (2)
13-18: New update-mode operations correctly wired into Plaid integrationImporting
createUpdateModeLinkTokenandexchangeUpdateModeTokenalongside the existing Plaid actions fits the new re-auth flow and keeps all institution-related operations centralized here. As long as their client signatures match the server-side definitions (returning a link token and{ institutionId }respectively), this is consistent with howPlaidLinkButtonis used elsewhere.
79-115: Awaitingrefetchon initial connect improves sync UXMaking
handleConnectionSuccessasync and awaitingrefetch()before clearingsyncingInstitutionIdand showing the “Initial transaction sync complete” toast gives a more accurate signal that the UI has actually refreshed from the backend. No behavior issues spotted here.src/legal/TermsOfServicePage.tsx (1)
1-21: Static LAST_UPDATED and motion wrapper look goodUsing a fixed
LAST_UPDATEDstring and the sharedfadeInvariant withmotion.divis clean and avoids the dynamic-date and hydration issues from the earlier version. No runtime or typing concerns here.src/legal/PrivacyPolicyPage.tsx (2)
1-21: Privacy page scaffold and static LAST_UPDATED are solidThe component structure mirrors the Terms page nicely: shared
fadeInpreset, motion wrapper, and a staticLAST_UPDATEDstring. This is a straightforward, production‑friendly setup with no obvious runtime issues.
199-281: Address policy text contradictions; verify infrastructure retention settings outside codebaseThe review's core concern is confirmed: the policy contains internal contradictions that should be harmonized.
Confirmed issues in text:
- Lines 212–215 state deletion is "permanent and cannot be undone"
- Lines 269–273 describe a 30-day grace period where accounts can be restored
- Lines 236–247 & 277–281 mention backup/log retention up to 90 days
These statements are inconsistent as written. The suggested diff rewording (clarifying that deletion starts a process with a grace window, after which it becomes permanent) correctly resolves this contradiction.
Infrastructure verification:
Codebase search shows:
- ✓ Cascade delete logic implemented in
schema.prismaandsrc/user/operations.ts- ✗ No explicit infrastructure configs found for backup retention (30 days), log retention (90 days), or Plausible retention (30 days)
These retention periods are typically configured in external systems (cloud provider dashboards, Plausible account settings) rather than in code. Manual verification is required to confirm that your actual backup retention, log retention, and Plausible configuration match the stated 30/90-day windows in the policy. If they don't match, either the policy numbers or the external configurations need adjustment.
Refactors Plaid integration for better user experience and adds minor UI tweaks: - Improves Plaid component by adding busy states for sync and reconnect actions and account toggling. - Fixes some layout issues on the legal pages - Removes a default value from the Tabs component in the Dashboard page. - Minor CSS class ordering in `AlertDialogContent` and `DialogContent`.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
src/transactions/components/transactions-list.tsx (2)
238-253: Animated expand/collapse container looks solid; watch for over-optimizationThe
AnimatePresence+motion.divsetup (height/opacity, overflow-hidden, short duration) is a good fit for this grouped list and should feel snappy. The explicitwillChange: 'height, opacity'andtransform: 'translateZ(0)'are fine, but they’re micro‑optimizations—if you notice any odd rendering on low‑end devices, those are easy to remove without changing behavior.
300-325: Plaid sign/color semantics look correct; consider zero-amount behavior and reuseThe new logic (positive → expense → red
-, negative → income → green+) matches Plaid’s convention foramountand is clear in the UI. One small edge case:amount === 0is currently treated as income (+and green). If zero-amount transactions can appear, you might want a neutral style instead, and if you need this convention in multiple places, extracting a smallformatPlaidAmount(amount)helper could help keep things consistent.src/dashboard/DashboardPage.tsx (1)
51-51: Controlled time range Tabs and loading wiring look correctThe
TimeRangetyping plus controlledTabsdriven bytimeRangeis consistent and avoids default/initial mismatches; theas TimeRangecast is acceptable given the fixed trigger values. ExtendingCategorySummary.isLoadingto includesyncingInstitutionIdandisConnectingPlaidalso makes sense to avoid showing stale data during sync/connect.If you ever add dynamic tab values, consider tightening
onValueChangeto validate againstTimeRangeinstead of casting.Also applies to: 65-67, 101-101
src/client/components/ui/dialog.tsx (1)
41-41: Scrollable DialogContent default is sensible and non-breakingAdding
max-h-[90vh]andoverflow-y-autoat the baseDialogContentlevel improves usability for long content and still allows callers to override viaclassName(since it’s merged last). This looks good; just be aware that components likePlaidConsentDialognow duplicate these classes, which is harmless but could be cleaned up later.src/root/components/footer.tsx (1)
133-159: Desktop footer structure, social icons, and copyrightThe desktop split into main and bottom rows reads well: main nav in the top row, legal/support plus social/copyright in the bottom row. Social links retain proper
target/reland aria labels, and reducing icon size to 18 keeps visual balance. The dynamicnew Date().getFullYear()usage is fine in this SPA context.Also applies to: 183-205
src/ProfilePage.tsx (1)
45-76: Data export flow is correct; consider minor robustness tweaksThe export handler correctly calls
exportUserData, builds a JSON Blob, triggers a download, revokes the object URL, and usesisExportingto gate the button and show “Exporting…”. Toasts cover success and failure states cleanly.As minor polish, you could:
- Set the Blob type to include charset (
'application/json;charset=utf-8') for maximum compatibility.- Optionally log the error in the catch for easier debugging.
These are non-blocking.
Also applies to: 132-150
src/plaid/components/plaid-consent-dialog.tsx (1)
11-27: Plaid consent dialog behavior and copy look solid; a couple of small nitsThe consent dialog is well-structured: clear explanation of what’s collected, how it’s used, Plaid’s role, and a direct Privacy Policy link with proper
target/rel. TheisUpdateModevariations in title, description, and button label are straightforward and keep the flow understandable for reconnect vs first connect.A couple of minor points you might consider:
- You’re already handling declines via the Cancel button (
onClick={onDecline}) and viaonOpenChange={open => !open && onDecline()}for overlay/ESC close. That’s fine, but be mindful not to also close the dialog via parent state changes without checking, oronDeclinemay fire more than once; if that’s a concern, you could centralize closing logic in the parent and letonOpenChangesimply forwardopenup.DialogContenthere repeatsmax-h-[90vh] overflow-y-auto, which are now also defaults in the sharedDialogContent. Not an issue, but you can simplify to justsm:max-w-[500px]if you prefer to avoid duplication.Overall, this is ready for production as-is.
Also applies to: 38-125
src/dashboard/components/plaid-integration.tsx (2)
387-422: Consider aligning button visibility with disabled logic.The action buttons are hidden when the current institution is busy (
!isBusy), but disabled when any institution is syncing or reconnecting. This creates an inconsistency where buttons may be visible but disabled without clear user feedback.Consider one of these approaches:
Use per-institution disabling (allow operations on other institutions):
disabled={isBusy}Hide buttons globally (prevent all operations during sync):
{!syncingInstitutionId && !reconnectingInstitutionId && ( <div className='flex gap-1'>...</div> )}Option 1 is generally preferred as it allows users to manage multiple institutions concurrently.
497-503: Switch disabled state is too broad.The
Switchis disabled when any account is being toggled (!!togglingAccountId), preventing users from toggling other accounts simultaneously.Apply this diff to enable per-account disabling:
<Switch checked={account.isTracked} onCheckedChange={checked => handleToggleAccount(account.id, checked) } - disabled={!!togglingAccountId} + disabled={togglingAccountId === account.id} />This allows toggling multiple accounts concurrently without conflicts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/ProfilePage.tsx(4 hunks)src/client/components/ui/alert-dialog.tsx(1 hunks)src/client/components/ui/dialog.tsx(1 hunks)src/dashboard/DashboardPage.tsx(1 hunks)src/dashboard/components/plaid-integration.tsx(7 hunks)src/legal/PrivacyPolicyPage.tsx(1 hunks)src/legal/TermsOfServicePage.tsx(1 hunks)src/plaid/components/plaid-consent-dialog.tsx(1 hunks)src/plaid/operations.ts(15 hunks)src/root/components/footer.tsx(5 hunks)src/transactions/TransactionsPage.tsx(1 hunks)src/transactions/components/transactions-list.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/transactions/TransactionsPage.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/client/components/ui/alert-dialog.tsx
- src/legal/PrivacyPolicyPage.tsx
- src/legal/TermsOfServicePage.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/dashboard/components/plaid-integration.tsx (2)
src/dashboard/components/plaid-link-button.tsx (1)
PlaidLinkButton(61-239)src/plaid/operations.ts (2)
createUpdateModeLinkToken(114-180)exchangeUpdateModeToken(289-371)
src/transactions/components/transactions-list.tsx (3)
src/utils/categories.ts (3)
getCategoryIcon(75-78)getPrettyCategoryName(40-42)getCategoryCssVariable(45-51)src/utils/dateUtils.ts (1)
formatTransactionDisplayDate(56-75)src/lib/utils.ts (1)
cn(9-11)
src/ProfilePage.tsx (1)
src/user/operations.ts (2)
exportUserData(97-180)deleteUserAccount(7-95)
src/plaid/operations.ts (3)
src/plaid/utils/encryption.ts (1)
decrypt(32-52)src/plaid/service.ts (2)
_internalCreateLinkToken(22-64)_internalExchangePublicToken(72-140)src/plaid/client.ts (1)
plaidClient(34-34)
src/plaid/components/plaid-consent-dialog.tsx (2)
src/client/components/ui/dialog.tsx (6)
Dialog(112-112)DialogContent(117-117)DialogHeader(118-118)DialogTitle(120-120)DialogDescription(121-121)DialogFooter(119-119)src/client/components/ui/button.tsx (1)
Button(63-63)
🔇 Additional comments (14)
src/transactions/components/transactions-list.tsx (1)
3-3: Confirmmotion/reactimport matches the animation library in useThe import looks fine assuming the project is actually using the
motionpackage that exports bothmotionandAnimatePresencefrom'motion/react'. If the app is still onframer-motion, this path would instead need to be'framer-motion'. Please double‑check it aligns with your installed dependency.src/root/components/footer.tsx (1)
39-42: Legal navigation and support links are wired cleanlyThe new
navigation.legalstructure and its usage in both mobile and desktop layouts are straightforward and consistent with the existingnavigation.mainpattern. UsingScrollToTopLinkfor internal legal routes and amailto:link for Support is appropriate. No issues from a routing or UX standpoint.Also applies to: 111-129, 161-180
src/dashboard/components/plaid-integration.tsx (6)
13-13: LGTM!The imports for update-mode operations and the new state management for reconnection and account toggling are correctly implemented and consistent with existing patterns.
Also applies to: 15-15, 72-77
98-98: LGTM!Awaiting
refetch()ensures the UI state is synchronized with the server response before clearing loading indicators. This prevents displaying stale data.Also applies to: 151-151, 172-172
162-186: LGTM!The refactored
handleToggleAccountproperly tracks per-account loading state and provides clear user feedback. The use oftogglingAccountIdprevents UI conflicts when toggling multiple accounts.
188-216: LGTM!The reconnection handlers properly manage state cleanup and provide appropriate user feedback. Error handling for the refetch operation is well-implemented.
316-354: LGTM!The update-mode reconnection flow is correctly implemented with proper error handling and state management. The inline async functions correctly extract and pass the required values to
PlaidLinkButton.
328-333: Return type mismatch in createLinkTokenAction.The
createLinkTokenActionprop expects a function that returns astring(the link token), but you're returning the entire result object{ linkToken: string }fromcreateUpdateModeLinkToken.Apply this diff to extract the linkToken:
createLinkTokenAction={async () => { const result = await createUpdateModeLinkToken({ institutionId: institution.id, }) return result.linkToken }}Wait, looking at the code again, line 332 already returns
result.linkToken. Let me reconsider... Actually, the code is correct. The inline function already extracts and returnsresult.linkToken. Let me mark this for skip.Likely an incorrect or invalid review comment.
src/plaid/operations.ts (6)
4-4: LGTM!The imports and type definitions for update-mode operations are correctly structured and consistent with the Wasp framework patterns.
Also applies to: 6-6, 23-24, 103-108
114-180: LGTM!The
createUpdateModeLinkTokenoperation properly validates authorization, checks for null access tokens before decryption, and correctly enables Plaid Update Mode by passing the decrypted access token.
289-371: LGTM!The
exchangeUpdateModeTokenoperation correctly updates the institution credentials and triggers an immediate sync. The decision to log sync errors without failing the re-authentication is appropriate.
568-577: LGTM!Automatically resetting the institution status to 'active' after successful sync enables graceful recovery from error states without requiring manual intervention.
760-760: LGTM!The enhanced deletion flow properly deactivates the Plaid item while ensuring user data can still be deleted even if the Plaid API call fails. This prevents users from being blocked by external API issues.
Also applies to: 771-793
872-872: LGTM!The clarifying comments about Plaid's amount convention prevent future confusion, and the addition of
statusanderrorCodeto the query result correctly exposes the re-authentication state to the UI.Also applies to: 896-897, 1002-1002, 1022-1023, 1080-1080, 1103-1105, 1186-1201, 1223-1224
Improves Plaid error handling by adding more granular error codes for re-authentication. Refactors account deletion flow to ensure navigation after logout, even if the logout process fails. Adds fly.toml configuration files for both client and server, and ignores `env.prod.server` in `.gitignore`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
fly-server.toml (1)
19-22: Consider resource monitoring and scaling strategy for production workloads.The VM specification allocates 1GB memory and 1 shared CPU core. While this is reasonable for initial deployment, monitor application performance under production load to determine if this is adequate for the Plaid integrations and user data export/deletion features mentioned in the PR objectives. Be prepared to scale resources if needed.
src/dashboard/components/plaid-integration.tsx (2)
399-402: Consider scoping button disabled state to per-institution busy status.The action buttons are disabled when ANY institution is syncing or reconnecting (
!!syncingInstitutionId || !!reconnectingInstitutionId), which prevents users from managing accounts of idle institutions while one is busy. Since the buttons are already hidden when the current institution is busy (line 389 checks!isBusy), the global disable check may be unnecessary.Consider removing the global disabled checks if there's no risk of race conditions:
<Button variant='ghost' size='icon' className='flex-shrink-0 text-muted-foreground hover:bg-muted hover:text-foreground' onClick={() => setManageAccountsInstitutionId(institution.id) } - disabled={ - !!syncingInstitutionId || - !!reconnectingInstitutionId - } > <Gear /> </Button>Apply similar changes to the delete button at lines 415-418.
Also applies to: 415-418
499-505: Consider per-account disabled state instead of global.The
Switchis disabled for ALL accounts when ANY account is toggling (disabled={!!togglingAccountId}). This prevents users from toggling account A while account B is in progress, which may be overly restrictive.Consider disabling only the specific account being toggled:
<Switch checked={account.isTracked} onCheckedChange={checked => handleToggleAccount(account.id, checked) } - disabled={!!togglingAccountId} + disabled={togglingAccountId === account.id} />This would allow toggling multiple accounts if there's no risk of backend race conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)fly-client.toml(1 hunks)fly-server.toml(1 hunks)src/ProfilePage.tsx(4 hunks)src/dashboard/components/plaid-integration.tsx(7 hunks)src/plaid/operations.ts(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ProfilePage.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/dashboard/components/plaid-integration.tsx (2)
src/dashboard/components/plaid-link-button.tsx (1)
PlaidLinkButton(61-239)src/plaid/operations.ts (2)
createUpdateModeLinkToken(114-180)exchangeUpdateModeToken(289-371)
src/plaid/operations.ts (3)
src/plaid/utils/encryption.ts (1)
decrypt(32-52)src/plaid/service.ts (2)
_internalCreateLinkToken(22-64)_internalExchangePublicToken(72-140)src/plaid/client.ts (1)
plaidClient(34-34)
🔇 Additional comments (19)
fly-client.toml (2)
9-10: Verify that build configuration is defined elsewhere.The
[build]section is empty, which means the build process must be configured through other means (e.g., a Dockerfile orfly deployconventions). Confirm that the build configuration for spender-client is properly defined to ensure successful deployments.
6-17: Deployment configuration aligns with production readiness objectives.The configuration appropriately sets the client service to auto-scale to zero machines (
min_machines_running = 0) for cost optimization, enables HTTPS enforcement, and defines reasonable resource specifications for a client application. The internal port 8043 and auto-start/stop behavior are suitable for a development/staging client.fly-server.toml (2)
9-10: Verify that build configuration is defined elsewhere.The
[build]section is empty, which means the build process must be configured through other means (e.g., a Dockerfile orfly deployconventions). Confirm that the build configuration for spender-server is properly defined to ensure successful deployments.
6-17: Production-appropriate server configuration with high availability.The configuration appropriately ensures the server is always running with
min_machines_running = 1, enabling HTTPS enforcement, and maintaining reasonable resource specifications. The asymmetry with the client (which can scale to zero) is correct for a production backend architecture..gitignore (1)
17-17: Confirm the intended filename:env.prod.serveror.env.prod.server?The entry
env.prod.serverbreaks the naming convention established by other dotenv entries (.env,.env.*,.env.example,.env.*.example). Additionally, the catch-all pattern.env.*on line 6 should already exclude.env.prod.serverif that were the intended filename.The codebase contains no references to either
env.prod.serveror.env.prod.server, so clarify:
- If the file is named
env.prod.serverwithout a leading dot (and why it differs from convention), or- If it should be renamed to
.env.prod.serverfor consistency.src/dashboard/components/plaid-integration.tsx (6)
13-13: LGTM! Update-mode operations imported correctly.The new imports for
createUpdateModeLinkTokenandexchangeUpdateModeTokensupport the reconnection flow and are used appropriately throughout the component.Also applies to: 15-15
72-77: LGTM! Per-operation loading states added correctly.The
reconnectingInstitutionIdandtogglingAccountIdstate variables properly track loading states for reconnection and account toggling operations.
98-98: LGTM! Data synchronization improved with awaited refetch.Awaiting
refetch()after mutations ensures the UI displays up-to-date data before showing success messages. This prevents race conditions and stale data issues.Also applies to: 151-151, 172-172
162-186: LGTM! Account toggling logic improved.The refactored
handleToggleAccountcorrectly accepts an explicitnextTrackedparameter, manages loading state withtogglingAccountId, and provides clear user feedback.
188-218: LGTM! Reconnection handlers implemented correctly.The
handleReconnectSuccessandhandleReconnectErrorhandlers properly manage state, refresh data, and provide user feedback. The unused parameters are acceptable as they maintain consistent callback signatures.
318-356: LGTM! Update-mode reconnection flow implemented correctly.The reconnection alert and update-mode
PlaidLinkButtonare properly configured:
- Async token creation and exchange callbacks correctly call update-mode operations
- Loading state properly scoped to the reconnecting institution
isUpdateMode={true}enables appropriate consent dialog handlingsrc/plaid/operations.ts (8)
23-24: LGTM! Required utilities imported correctly.The
decryptandplaidClientimports support update-mode operations and item removal functionality.
103-180: LGTM! Update-mode link token creation implemented securely.The
createUpdateModeLinkTokenoperation correctly:
- Validates user authentication and input
- Verifies institution ownership before allowing re-authentication
- Checks for accessToken existence before decryption
- Creates update-mode link token with decrypted access token
- Handles errors appropriately
277-371: LGTM! Update-mode token exchange handles re-authentication robustly.The
exchangeUpdateModeTokenoperation correctly:
- Verifies institution ownership
- Exchanges public token for new encrypted access token
- Resets status to 'active' and clears error codes
- Triggers immediate sync without failing re-authentication if sync errors occur
This defensive approach ensures successful re-authentication even when sync encounters transient issues.
568-577: LGTM! Institution status automatically recovered on successful sync.Marking the institution as 'active' and clearing error codes after successful sync is good defensive coding that automatically recovers from transient errors.
772-772: LGTM! Plaid item removal integrated with defensive error handling.The institution deletion now properly calls Plaid's
itemRemoveAPI before database deletion. The defensive error handling ensures users can delete institutions even if the Plaid API call fails, while still logging errors for debugging. This follows Plaid's best practices.Also applies to: 783-805
884-884: LGTM! Spending calculations correctly aligned with Plaid convention.Removing
Math.absis correct since the queries filter foramount > 0(expenses only per Plaid's convention: positive = money OUT, negative = money IN). The comments clearly document this convention.Also applies to: 908-909, 1014-1014, 1034-1035, 1092-1092, 1115-1117
1199-1213: LGTM! Institution status and error code exposed for UI handling.The
GetInstitutionsResulttype and query correctly exposestatusanderrorCodefields, enabling the frontend to display appropriate reconnection UI when institutions require re-authentication.Also applies to: 1235-1236
585-643: Remove the suggested error code additions—PENDING_DISCONNECT and PENDING_EXPIRATION are webhook codes, not error codes.PENDING_DISCONNECT and PENDING_EXPIRATION are webhook codes that Plaid sends proactively to notify you of pending Item issues, not error codes returned in API error responses. These webhooks indicate that the Item should be re-initialized via update mode, but they would never appear in the
error.response?.data?.error_codefield that this code is checking.The current error handling is correct as implemented. All six error codes mentioned in the original review (INVALID_CREDENTIALS, INSUFFICIENT_CREDENTIALS, ITEM_NOT_SUPPORTED, MFA_NOT_SUPPORTED, NO_ACCOUNTS, INSTITUTION_NO_LONGER_SUPPORTED) are still valid for 2025 and appropriately included in the requiresReauth array.
Likely an incorrect or invalid review comment.
This commit fixes critical issues with Plaid webhook handling and transaction time display: **Webhook Signature Verification Fix:** - Capture raw request body before Express parsing for signature verification - The body hash must match the exact bytes Plaid sent, not a re-stringified version - Add verify callback to express.json() middleware to store raw body - Fixes 401 "Webhook body hash mismatch" errors in production **Transaction Time Display Fix:** - Treat common default times (midnight, 4 AM, 5 AM UTC) as date-only values - When institutions don't provide specific times, Plaid sends default times that shouldn't be displayed - Show just the date for default times, preserve actual times for real transaction timestamps - Display times in UTC to preserve original transaction time **Webhook Handler Improvements:** - Add handlers for INITIAL_UPDATE and HISTORICAL_UPDATE webhook codes - These webhooks now trigger automatic transaction syncing - Improves automatic transaction updates when connecting new institutions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/plaid/webhooks.ts (3)
42-53: PreferdecodeProtectedHeaderfromjoseover manual base64 parsing of the JWT header.Manually splitting on
.and decoding withBuffer.from(..., 'base64')is more brittle (base64url vs base64, malformed tokens, etc.) than using jose’s built-in helper.You can simplify and harden this part by letting jose parse the header:
-import { jwtVerify, importJWK } from 'jose' +import { jwtVerify, importJWK, decodeProtectedHeader } from 'jose' @@ - // Step 1: Decode JWT header without verification to get kid (key ID) - const [headerB64] = jwtToken.split('.') - const header = JSON.parse(Buffer.from(headerB64, 'base64').toString()) + // Step 1: Decode JWT header without verification to get kid (key ID) + const header = decodeProtectedHeader(jwtToken) @@ - if (header.alg !== 'ES256') { + if (header.alg !== 'ES256') { @@ - const keyResponse = await plaidClient.webhookVerificationKeyGet({ - key_id: header.kid, - }) + const keyResponse = await plaidClient.webhookVerificationKeyGet({ + key_id: header.kid, + })This keeps the logic the same while deferring header parsing to jose’s trusted implementation.
125-135: HandlePlaid-Verificationheader asstring | string[]for robustness.
req.headers['plaid-verification']can bestring | string[] | undefined; the current cast tostringworks in the common case but is a bit optimistic. Consider normalizing it like this:- const jwtToken = req.headers['plaid-verification'] as string - const rawBody = (req as any).rawBody || JSON.stringify(req.body) + const plaidVerificationHeader = req.headers['plaid-verification'] + const jwtToken = Array.isArray(plaidVerificationHeader) + ? plaidVerificationHeader[0] + : plaidVerificationHeader + const rawBody = (req as any).rawBody || JSON.stringify(req.body)The subsequent
if (!jwtToken)check then naturally handles theundefinedcase, while avoiding surprises if multiple headers are ever present.
146-187: Dynamic import ofsyncTransactionsshould handleimport()rejection explicitly.The async
import('./operations.js').then(...)correctly keeps the webhook response fast by not awaitingsyncTransactions, and you already catch errors fromsyncTransactionsitself. However, if theimport()fails (e.g., bundling/runtime issue), that rejection is currently unhandled.You can add an outer
.catchto ensure all failures are logged cleanly:- // Trigger async sync - don't await to avoid webhook timeout - // Import syncTransactions dynamically to avoid circular dependencies - import('./operations.js').then(({ syncTransactions }) => { - syncTransactions({ institutionId: institution.id }, { - user: { id: institution.userId }, - entities: prisma, - } as any).catch(error => { - console.error( - `Error syncing transactions for institution ${institution.id}:`, - error, - ) - }) - }) + // Trigger async sync - don't await to avoid webhook timeout. + // Import syncTransactions dynamically to avoid circular dependencies. + import('./operations.js') + .then(({ syncTransactions }) => { + return syncTransactions( + { institutionId: institution.id }, + { + user: { id: institution.userId }, + entities: prisma, + } as any, + ).catch(error => { + console.error( + `Error syncing transactions for institution ${institution.id}:`, + error, + ) + }) + }) + .catch(error => { + console.error( + `Error loading Plaid operations module for institution ${institution.id}:`, + error, + ) + })This keeps the non‑blocking behavior while avoiding silent failures if the module load itself breaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plaid/webhooks.ts(4 hunks)src/utils/dateUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plaid/webhooks.ts (1)
src/plaid/client.ts (1)
plaidClient(34-34)
🔇 Additional comments (3)
src/utils/dateUtils.ts (1)
50-51: LGTM: Comments accurately reflect UTC handling.The updated comments clearly describe the UTC-based behavior and default time detection.
src/plaid/webhooks.ts (2)
11-22: Raw body capture middleware looks correct for signature verification.Using
express.json’sverifyhook to stashreq.rawBodybefore parsing is a solid way to preserve the exact payload for Plaid’s body hash check; this integration with the WaspmiddlewareConfig.set('express.json', …)API looks good.
85-105: Constant‑time body hash comparison is correctly implemented.The move to compute
bodyHashand compare it withrequest_body_sha256usingcrypto.timingSafeEqual, with a prior length check, addresses the timing‑attack concern from the previous review and matches Plaid’s body‑hash verification guidance.The note about needing the exact raw body (canonical JSON, no extra whitespace) is also helpful for future maintainers.
Strengthens Plaid webhook JWT validation by explicitly checking for a valid `iat` claim in addition to verifying token age. Improves transaction date display by: - Defining Plaid's default time values for better identification. - Using a list to check for default times. - Adding 'UTC' suffix to displayed times for clarity.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plaid/webhooks.ts(4 hunks)src/utils/dateUtils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/dateUtils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plaid/webhooks.ts (1)
src/plaid/client.ts (1)
plaidClient(34-34)
🔇 Additional comments (5)
src/plaid/webhooks.ts (5)
1-10: LGTM - Proper imports for JWT verification.The imports correctly include
josefor ES256 JWT verification and the Plaid client for dynamic key retrieval, aligning with Plaid's documented webhook verification flow.
11-24: LGTM - Raw body capture for hash verification.The middleware correctly captures the raw request body before JSON parsing, which is essential for computing the SHA-256 hash that matches Plaid's
request_body_sha256claim.
34-38: LGTM - Development bypass with clear warning.Skipping verification in development mode is reasonable for local testing, and the warning ensures developers are aware that production will enforce full JWT verification.
71-113: Excellent - Critical security issues resolved.Both critical security issues from the previous review have been properly addressed:
Timing attack mitigation (lines 99-113): Now uses
crypto.timingSafeEqual()with proper buffer conversion and length validation, preventing timing-based attacks on the body hash comparison.Robust iat validation (lines 73, 76-91): The implementation uses
maxTokenAge: '5m'injwtVerify()for built-in enforcement, plus defense-in-depth manual validation that ensuresiatis present, numeric, and finite. TheMath.abs()check also defends against future-dated tokens.The implementation now correctly follows Plaid's JWT verification requirements with proper security safeguards.
154-199: LGTM - Proper async webhook handling.The webhook handling correctly:
- Responds immediately to avoid timeouts (line 194)
- Triggers transaction sync asynchronously (lines 182-192)
- Uses dynamic import to avoid circular dependencies
- Includes proper error handling in the promise chain
The expanded webhook code coverage for
SYNC_UPDATES_AVAILABLE,INITIAL_UPDATE, andHISTORICAL_UPDATEaligns with Plaid's transaction sync patterns.
Ensures the `request_body_sha256` claim in Plaid webhook JWTs is a valid SHA-256 hex digest. It also handles cases where the claim is missing or is of an invalid data type, preventing potential verification failures. Also ensures the raw request body is correctly extracted for JWT validation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/plaid/webhooks.ts (2)
174-211: Add a top‑level.catchon the dynamic import to prevent unhandled rejections.The current code only catches errors from
syncTransactions(), but ifimport('./operations.js')itself fails, the rejection goes unhandled. Chaining.catch()directly to theimport()call ensures both import and execution errors are caught.- // Trigger async sync - don't await to avoid webhook timeout - // Import syncTransactions dynamically to avoid circular dependencies - import('./operations.js').then(({ syncTransactions }) => { - syncTransactions({ institutionId: institution.id }, { - user: { id: institution.userId }, - entities: prisma, - } as any).catch(error => { - console.error( - `Error syncing transactions for institution ${institution.id}:`, - error, - ) - }) - }) + // Trigger async sync - don't await to avoid webhook timeout + // Import syncTransactions dynamically to avoid circular dependencies + import('./operations.js') + .then(({ syncTransactions }) => { + return syncTransactions( + { institutionId: institution.id }, + { + user: { id: institution.userId }, + entities: prisma, + } as any, + ) + }) + .catch(error => { + console.error( + `Error loading or running syncTransactions for institution ${institution.id}:`, + error, + ) + })
41-69: UsedecodeProtectedHeaderfromjosefor more robust JWT header decoding and addkidvalidation.Manual JWT header decoding with
Buffer.from(..., 'base64')bypasses base64url validation that the JWS spec requires. Replace withdecodeProtectedHeader(jwtToken)fromjose, and validatekidbefore calling Plaid to avoid unnecessary API calls with missing or invalid key IDs.-import { jwtVerify, importJWK } from 'jose' +import { jwtVerify, importJWK, decodeProtectedHeader } from 'jose' - // Step 1: Decode JWT header without verification to get kid (key ID) - const [headerB64] = jwtToken.split('.') - const header = JSON.parse(Buffer.from(headerB64, 'base64').toString()) + // Step 1: Decode JWT header without verification to get kid (key ID) + const header = decodeProtectedHeader(jwtToken) // Step 2: Verify algorithm is ES256 if (header.alg !== 'ES256') { console.error(`Invalid JWT algorithm: ${header.alg}, expected ES256`) return false } + // Step 2a: Validate kid before API call + if (!header.kid || typeof header.kid !== 'string') { + console.error('Webhook JWT header missing or invalid kid') + return false + } + // Step 3: Get verification key from Plaid using the kid const keyResponse = await plaidClient.webhookVerificationKeyGet({ key_id: header.kid, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/plaid/webhooks.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plaid/webhooks.ts (1)
src/plaid/client.ts (1)
plaidClient(34-34)
🔇 Additional comments (2)
src/plaid/webhooks.ts (2)
71-132: JWT freshness + body-hash verification implementation confirmed and correct.The code properly implements defense-in-depth webhook verification:
- jose.jwtVerify with
maxTokenAge: '5m'enforces a 5-minute window based on the JWT'siatclaim- Plaid's webhook JWT payload includes
request_body_sha256which should be compared against the SHA-256 hash of the webhook body- The explicit
iatvalidation, SHA-256 hashing, andcrypto.timingSafeEqualconstant-time comparison align with Plaid's documented webhook verification requirements and prevent replay/timing attacks.
11-22: Now let me search the codebase to verify the middleware registration and rawBody usage patterns:Based on the web search results, I've confirmed that Express middleware and routes are evaluated in the order they are registered, with middleware declared before routes running for all routes. The pattern shown in your code—using
bodyParser.json({ verify: (req, res, buf, encoding) => { req.rawBody = buf.toString(...) } })—is the recommended approach for webhook signature verification.Plaid webhook verification requires computing the SHA-256 of the webhook body using constant-time comparison, and is sensitive to whitespace with 2-space tab-spacing. This confirms the review comment's approval of the raw-body capture approach is correct.
No actionable issues remain; the implementation pattern is sound. The code correctly uses Express's middleware
verifycallback to capture the raw body before JSON parsing—this runs before route handlers if registered before routes. The main verification concern (thatplaidWebhookMiddlewareConfigFnis guaranteed to run) depends on deployment configuration being correct, which is outside the scope of code review.
Ensures the code correctly handles cases where NODE_ENV is explicitly undefined, preventing potential issues with default environment configurations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/plaid/webhooks.ts (4)
14-22: Tightenreqtyping in the rawBody capture middleware.The raw body capture is correct and essential for Plaid’s body-hash check. To avoid
anyand get better type safety, you can typereqasRequest & { rawBody?: string }:- verify: (req: any, _res, buf) => { + verify: (req: Request & { rawBody?: string }, _res, buf) => { // Store raw body buffer for signature verification req.rawBody = buf.toString('utf8') },This keeps the behavior but improves TS correctness.
27-136: JWT webhook verification flow is solid; consider usingdecodeProtectedHeaderand optional key caching.The overall flow (alg guard, Plaid key fetch, ES256 verify, strict
iatchecks, SHA‑256 body hash withtimingSafeEqual) aligns well with Plaid’s guidance and is security‑sound. Two optional refinements:
- Use jose’s header decoder instead of manual base64/JSON parsing.
This avoids manual
split('.')+Buffer.from(..., 'base64')and relies on jose’s base64url handling:-import { jwtVerify, importJWK } from 'jose' +import { jwtVerify, importJWK, decodeProtectedHeader } from 'jose' ... - // Step 1: Decode JWT header without verification to get kid (key ID) - const [headerB64] = jwtToken.split('.') - const header = JSON.parse(Buffer.from(headerB64, 'base64').toString()) + // Step 1: Decode JWT header without verification to get kid (key ID) + const header = decodeProtectedHeader(jwtToken)
- Optionally cache Plaid verification keys by
kid.If webhook volume grows, you might add a small in‑memory cache (e.g.,
Map<string, JWK>with a TTL) aroundplaidClient.webhookVerificationKeyGetto reduce latency and external dependency load. Not required for correctness, just a performance/resiliency improvement.The extra manual
iatage check on top ofmaxTokenAge: '5m'is reasonable defense‑in‑depth.
150-160: NormalizePlaid-Verificationheader to a string before verification.
req.headers['plaid-verification']can bestring | string[] | undefined. Passing astring[]intoverifyPlaidWebhookwould cause.split('.')to throw. You can normalize once up front:- const jwtToken = req.headers['plaid-verification'] as string + const headerValue = req.headers['plaid-verification'] + const jwtToken = Array.isArray(headerValue) ? headerValue[0] : headerValue const rawBody = (req as any).rawBody ?? JSON.stringify(req.body) if (!jwtToken) {This keeps the behavior but guards against multiple header values.
171-215: Handle errors from the dynamic import ofsyncTransactionsto avoid unhandled rejections.The async, fire‑and‑forget
syncTransactionstrigger is appropriate to avoid webhook timeouts, but ifimport('./operations.js')itself rejects (e.g., transient load error), that rejection isn’t currently handled. You can add a.catchon the import promise:- import('./operations.js').then(({ syncTransactions }) => { - syncTransactions({ institutionId: institution.id }, { - user: { id: institution.userId }, - entities: prisma, - } as any).catch(error => { - console.error( - `Error syncing transactions for institution ${institution.id}:`, - error, - ) - }) - }) + import('./operations.js') + .then(({ syncTransactions }) => { + return syncTransactions( + { institutionId: institution.id }, + { + user: { id: institution.userId }, + entities: prisma, + } as any, + ).catch(error => { + console.error( + `Error syncing transactions for institution ${institution.id}:`, + error, + ) + }) + }) + .catch(error => { + console.error( + `Error loading syncTransactions module for institution ${institution.id}:`, + error, + ) + })This keeps the non‑blocking behavior while making failures more observable and avoiding unhandled promise rejections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/plaid/webhooks.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plaid/webhooks.ts (1)
src/plaid/client.ts (1)
plaidClient(34-34)
🔇 Additional comments (1)
src/plaid/webhooks.ts (1)
6-9: JWT verification imports and NODE_ENV handling look good.Using
josefor JWT verification and dropping the default'development'fallback forNODE_ENVensures verification won’t be silently skipped in misconfigured environments. No changes needed here.
Introduces WASP_WEB_CLIENT_URL environment variable, which lets the client app run on a different port. It allows overriding the default client URL for Stripe integration, making it easier to develop and test in different environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/stripe/service.ts (1)
9-12: Approve fallback chain; consider adding URL validation and production safeguards.The environment variable precedence and fallback to localhost for development is correct. However, consider these improvements for production readiness:
- Validate URL format: Stripe requires absolute HTTPS URLs in production. Invalid URLs will cause runtime failures in checkout flows.
- Warn on localhost in production: The fallback to
localhost:3000could mask misconfiguration in production environments.Apply this diff to add validation and production warnings:
const CLIENT_URL = process.env.WASP_WEB_CLIENT_URL || process.env.CLIENT_URL || 'http://localhost:3000' + +// Validate CLIENT_URL format +try { + const url = new URL(CLIENT_URL) + if (process.env.NODE_ENV === 'production' && !url.protocol.startsWith('https')) { + console.warn('Warning: CLIENT_URL should use HTTPS in production') + } + if (process.env.NODE_ENV === 'production' && url.hostname === 'localhost') { + console.error('Error: CLIENT_URL is set to localhost in production environment') + } +} catch (error) { + throw new Error(`Invalid CLIENT_URL: ${CLIENT_URL}. Must be a valid absolute URL.`) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
prisma.config.ts(1 hunks)src/stripe/service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- prisma.config.ts
1 participant
Summary by CodeRabbit
New Features
Improvements
Security
Data
Chores
✏️ Tip: You can customize this high-level summary in your review settings.