-
Notifications
You must be signed in to change notification settings - Fork 8
Collect all invalid usernames in batch payment validation #372
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
- Modified processRecords() to continue validation even when encountering invalid usernames - Collect all validation errors and return them together - Added comprehensive unit tests for the new validation behavior Co-authored-by: k9ert <117085+k9ert@users.noreply.github.com>
- Added whiteSpace: pre-line CSS to preserve line breaks in error messages - Improved error message formatting with extra spacing Co-authored-by: k9ert <117085+k9ert@users.noreply.github.com>
|
Asked Claude Opus 4.5 to review: PR Review: #372 - Collect all invalid usernames in batch payment validationSummaryPR 372 effectively addresses Issue #69. The issue requested a validation feature that identifies and returns ALL errors for invalid usernames when uploading a CSV file, rather than failing on the first invalid entry. Changes Made1. The
if (!getDefaultWalletID.responsePayload) {
Added CSS to properly display multi-line error messages:
Added comprehensive unit tests (216 lines) covering:
✅ Does it Fix Issue #69?YES - The PR properly addresses the core problem:
🔶 Suggestions for Improvement
Verdict: APPROVE ✅The PR correctly implements the feature requested in Issue #69. Felix (the issue author) wanted to disburse sats to 130+ Blink addresses without having to discover invalid usernames one at a time. This PR enables exactly that - all invalid usernames are collected and displayed together, allowing users to fix all issues in a single pass before re-uploading. The code is clean, follows existing patterns, includes good test coverage, and properly handles the UI display of multi-line error messages. |
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.
Pull request overview
This PR enhances batch payment CSV validation by collecting all invalid usernames in a single pass, eliminating the need for multiple upload iterations. Previously, validation stopped at the first error, forcing users processing 130+ recipients to re-upload repeatedly to discover each invalid username.
Key changes:
- Modified
processRecords()to accumulate validation errors instead of failing fast - Enhanced error display to support multi-line formatted error messages
- Added comprehensive test coverage for batch validation scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
apps/dashboard/app/batch-payments/utils.ts |
Refactored validation logic to collect all errors before returning, enabling single-pass validation |
apps/dashboard/components/batch-payments/index.tsx |
Added CSS styling to support multi-line error message display |
apps/dashboard/test/unit/batch-payments-utils.test.ts |
Added test suite validating error aggregation and successful processing of mixed valid/invalid records |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (validationErrors.length > 0) { | ||
| const errorMessage = `Invalid username(s) found:\n\n${validationErrors.join("\n")}` | ||
| return new Error(errorMessage) | ||
| } |
Copilot
AI
Dec 22, 2025
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.
The hardcoded error message prefix 'Invalid username(s) found:' could be extracted as a constant for easier maintenance and potential internationalization. Consider defining it at the module level alongside other constants.
| jest.clearAllMocks() | ||
| }) | ||
|
|
||
| it("should collect all invalid usernames and return them in a single error", async () => { |
Copilot
AI
Dec 22, 2025
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.
Test description uses 'single error' which could be ambiguous. Consider rephrasing to 'should collect all invalid usernames and return aggregated error message' to better describe the batched error behavior.
| it("should collect all invalid usernames and return them in a single error", async () => { | |
| it("should collect all invalid usernames and return aggregated error message", async () => { |
Address Copilot code review suggestions: - Extract hardcoded error messages to ERROR_MESSAGES constant for maintainability and i18n - Rename test description to better describe aggregated error behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Prefix error message with count for easier debugging (e.g. "3 Invalid username(s) found:") - Add test for empty records input - Add test for all-invalid usernames scenario 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
PR review also from Opus 4.5: PR Review: #372 - Collect all invalid usernames in batch payment validation Overall: ✅ LGTM with minor suggestions This PR correctly addresses the feature request from issue #69 where batch payment validation stopped at the first invalid username, forcing users with 130+ recipients to upload multiple times to find all errors. Code Changes Analysis apps/dashboard/app/batch-payments/utils.ts ✅ Good:
apps/dashboard/components/batch-payments/index.tsx ✅ Good:
apps/dashboard/test/unit/batch-payments-utils.test.ts ✅ Good:
Suggestions
Verdict Approve - The PR correctly solves the stated problem with clean, well-tested code. The implementation matches the approach outlined in the issue comments. Edge case tests would be nice but not blocking. |
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Batch payment CSV validation previously stopped at the first invalid username, requiring multiple upload iterations to identify all errors. For users processing 130+ recipients, finding 1-2 invalid entries meant repeated uploads.
Changes
Validation logic (
apps/dashboard/app/batch-payments/utils.ts)processRecords()now collects all validation errors before returningError display (
apps/dashboard/components/batch-payments/index.tsx)whiteSpace: "pre-line"to modal for multi-line error renderingTests (
apps/dashboard/test/unit/batch-payments-utils.test.ts)Example output
Users can now fix all invalid usernames in one iteration instead of discovering them sequentially.
Original prompt
This section details on the original issue you should resolve
<issue_title>Feature Suggestion – Batch Payment Validation for Blink</issue_title>
<issue_description>Migrated from GaloyMoney/blink#4804
Originally created by @Divoar on 4/17/2025, 7:16:45 AM
Hello Blink Team,
It would be a huge improvement to have a validation feature within the batch payment option, where the system can identify and return errors for invalid usernames upon uploading the .csv file. This would make it easier to detect and fix incorrect addresses before proceeding with the payment.
I’m currently trying to disburse sats to over 130 Blink addresses for The Core Cohort 6 graduates, and it’s quite difficult to identify the one or two incorrect entries that block the entire batch. Additionally, some students delay in confirming their usernames, which slows down the whole process.
Having this feature would make disbursements much smoother and faster, especially for educators or project leads managing bulk payments.
Thanks so much for all the great work you're doing at Blink — it’s really powering grassroots Bitcoin adoption.
Best
Felix
</issue_description>
<agent_instructions>Check #69 (comment) for specific implementation hints.</agent_instructions>
Comments on the Issue (you are @copilot in this section)
@k9ert Just had a discussion with claude about it. Here is the result:Deep Dive: Batch Payment Validation
Relevant Files
Current Flow (utils.ts)
`validateCSV()` (L24-140): Parses CSV, validates headers/fields, aggregates totals
`processRecords()` (L142-177): Validates usernames one-by-one via GraphQL
Root Cause Hypothesis
The feature request maps to this code path:
```
processRecords() → getUserDetailsAction() → getWalletDetailsByUsername()
```
When a username is invalid, the function returns immediately (L156-157):
```typescript
if (getDefaultWalletID.error) {
return new Error(getDefaultWalletID.message)
}
```
This stops processing at the first bad username instead of collecting all errors.
Issues From @pretyflaco's Comment
Related Issues
Implementation Approach (for future dev)
To fix, `processRecords()` should:
<comment_new>@grimen
@pretyflaco You can also use "Assign to Copilot" and work that way through GitHub integrated tools as a workflow. </comment_new>
<comment_new>@grimen
Regarding these issues they seem to not be directly tied to this feature request, so it would make sense to break this out to BUG ticket(s) to not mash it all und...
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.