-
Notifications
You must be signed in to change notification settings - Fork 0
Add Stripe links ACH option #378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughA new boolean field Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/common/types/stripe.ts(2 hunks)src/ui/pages/stripe/CreateLink.tsx(4 hunks)
🧰 Additional context used
🪛 ESLint
src/common/types/stripe.ts
[error] 13-13: Insert ,
(prettier/prettier)
[error] 37-37: Insert ⏎
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (4)
src/ui/pages/stripe/CreateLink.tsx (4)
9-9: LGTM: Switch import added correctly.The Switch component import is properly added to support the new ACH payment toggle.
46-46: LGTM: Form state initialized correctly.The
achPaymentsEnabledfield is correctly initialized tofalse, which aligns with the disabled state of the UI Switch.
57-58: Good defensive validation for upcoming feature.The validation logic prevents form submission if
achPaymentsEnabledis somehow set totrue, which is good defensive coding even though the Switch is currently disabled. This ensures the feature can't be accidentally enabled through browser dev tools or other means.
117-124: LGTM: UI clearly indicates upcoming feature.The disabled Switch with the "(upcoming)" label and descriptive text clearly communicates that this is a future feature. The component is properly bound to the form state using
getInputProps.
|
|
||
| export const invoiceLinkPostRequestSchema = z.object({ | ||
| invoiceId: z.string().min(1), | ||
| invoiceAmountUsd: z.number().min(50), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Schema mismatch with frontend validation.
The backend schema enforces a minimum of 50 (dollars) for invoiceAmountUsd, but the frontend validation in CreateLink.tsx (line 52) allows amounts as low as $0.50. This mismatch will cause the backend to reject valid frontend submissions, resulting in runtime errors and poor UX.
Apply this diff to align the backend schema with the frontend validation:
- invoiceAmountUsd: z.number().min(50),
+ invoiceAmountUsd: z.number().min(0.5),Alternatively, if $50 is the correct minimum, update the frontend validation:
In src/ui/pages/stripe/CreateLink.tsx:
invoiceAmountUsd: (value) =>
- value < 0.5 ? "Amount must be at least $0.50" : null,
+ value < 50 ? "Amount must be at least $50.00" : null,And update the placeholder:
- placeholder="100"
- min={0.5}
+ placeholder="100"
+ min={50}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| invoiceAmountUsd: z.number().min(50), | |
| invoiceAmountUsd: z.number().min(0.5), |
🤖 Prompt for AI Agents
In src/common/types/stripe.ts around line 10, the zod schema sets
invoiceAmountUsd to a minimum of 50 which conflicts with the frontend allowing
$0.50; change the schema to z.number().min(0.5) to match the frontend validation
(or alternatively, if $50 is intended, update src/ui/pages/stripe/CreateLink.tsx
validation and its placeholder to enforce and display $50 minimum) — ensure
types, tests, and any API docs are updated accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashleyyli please reconcile now that we're here. We want it to be 50 cents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| contactEmail: z.string().email(), | ||
| achPaymentsEnabled: z.boolean() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix formatting issues flagged by ESLint.
The static analysis tool indicates missing trailing commas and formatting inconsistencies.
Apply this diff to fix the formatting:
- contactEmail: z.string().email(),
- achPaymentsEnabled: z.boolean()
+ contactEmail: z.string().email(),
+ achPaymentsEnabled: z.boolean(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contactEmail: z.string().email(), | |
| achPaymentsEnabled: z.boolean() | |
| contactEmail: z.string().email(), | |
| achPaymentsEnabled: z.boolean(), |
🧰 Tools
🪛 ESLint
[error] 13-13: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In src/common/types/stripe.ts around lines 12 to 13, the object schema lines are
missing trailing commas and have formatting inconsistencies flagged by ESLint;
add a trailing comma after each property (e.g., after the email validator and
after achPaymentsEnabled) and reformat the block to match the project's style
rules (run the project's formatter or ESLint --fix) so spacing and commas comply
with the linter.
|
|
||
| export type GetInvoiceLinksResponse = z.infer< | ||
| typeof invoiceLinkGetResponseSchema>; | ||
| typeof invoiceLinkGetResponseSchema>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix formatting issue flagged by ESLint.
Apply this diff:
- typeof invoiceLinkGetResponseSchema>;
+ typeof invoiceLinkGetResponseSchema
+>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| typeof invoiceLinkGetResponseSchema>; | |
| typeof invoiceLinkGetResponseSchema | |
| >; |
🧰 Tools
🪛 ESLint
[error] 37-37: Insert ⏎
(prettier/prettier)
🤖 Prompt for AI Agents
In src/common/types/stripe.ts around line 37, fix the ESLint formatting problem
on the type line by correcting the trailing punctuation and spacing so the
generic/type expression is formatted consistently with project style (remove the
stray/misplaced semicolon or extra whitespace around the closing angle bracket),
then run or apply eslint --fix / prettier to ensure the file matches project
formatting.
devksingh4
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashleyyli is there a real eslint issue?
|
|
||
| export const invoiceLinkPostRequestSchema = z.object({ | ||
| invoiceId: z.string().min(1), | ||
| invoiceAmountUsd: z.number().min(50), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashleyyli please reconcile now that we're here. We want it to be 50 cents
Switch to enable ACH for Stripe link (currently disabled)
Addresses #377
Summary by CodeRabbit