-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: bulk send templates via csv #1578
base: main
Are you sure you want to change the base?
Conversation
Adds the ability to bulk send a template using a CSV. Uses a background job for processing the CSV and its rows with an email being sent at the end containing all successful and errored entries.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
5 Skipped Deployments
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive bulk send feature for document templates. The implementation spans multiple components and services, enabling users to upload a CSV file to send multiple documents simultaneously. The feature includes a new dialog for bulk sending, job definitions for processing, and a TRPC router method to handle the upload and job triggering. The system supports file validation, recipient processing, and provides detailed email notifications about the bulk send operation. Changes
Sequence DiagramsequenceDiagram
participant User
participant BulkSendDialog
participant TemplateRouter
participant JobSystem
participant DocumentProcessor
participant EmailNotifier
User->>BulkSendDialog: Upload CSV
BulkSendDialog->>TemplateRouter: Submit bulk send request
TemplateRouter->>JobSystem: Trigger bulk send job
JobSystem->>DocumentProcessor: Process CSV rows
DocumentProcessor-->>JobSystem: Track processing results
JobSystem->>EmailNotifier: Send completion notification
EmailNotifier-->>User: Bulk send summary email
Possibly related PRs
Suggested Labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Copilot reviewed 6 out of 12 changed files in this pull request and generated no comments.
Files not reviewed (6)
- packages/lib/package.json: Language not supported
- packages/lib/jobs/client.ts: Evaluated as low risk
- apps/web/src/app/(dashboard)/templates/[id]/template-page-view.tsx: Evaluated as low risk
- apps/web/src/app/(dashboard)/templates/data-table-action-dropdown.tsx: Evaluated as low risk
- packages/trpc/server/template-router/schema.ts: Evaluated as low risk
- packages/lib/jobs/definitions/internal/bulk-send-template.handler.ts: Evaluated as low risk
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: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.json
is excluded by!**/package-lock.json
,!**/*.json
packages/lib/package.json
is excluded by!**/*.json
📒 Files selected for processing (10)
apps/web/src/app/(dashboard)/templates/[id]/template-page-view.tsx
(2 hunks)apps/web/src/app/(dashboard)/templates/data-table-action-dropdown.tsx
(3 hunks)apps/web/src/components/templates/template-bulk-send-dialog.tsx
(1 hunks)packages/email/templates/bulk-send-complete.tsx
(1 hunks)packages/lib/jobs/client.ts
(2 hunks)packages/lib/jobs/definitions/emails/send-bulk-complete-email.ts
(1 hunks)packages/lib/jobs/definitions/internal/bulk-send-template.handler.ts
(1 hunks)packages/lib/jobs/definitions/internal/bulk-send-template.ts
(1 hunks)packages/trpc/server/template-router/router.ts
(3 hunks)packages/trpc/server/template-router/schema.ts
(2 hunks)
🔇 Additional comments (6)
packages/lib/jobs/definitions/internal/bulk-send-template.ts (1)
1-37
: Job definition is properly implementedThe job definition and schema are correctly defined, and the handler is appropriately imported and invoked.
packages/lib/jobs/definitions/emails/send-bulk-complete-email.ts (1)
1-39
: Email job definition is correctly structuredThe schema and job definition for sending bulk completion emails are well-implemented, ensuring type safety and adherence to the expected structure.
packages/lib/jobs/client.ts (1)
9-9
: LGTM! Clean integration of bulk send job.The bulk send template job definition is properly integrated into the jobs client.
Also applies to: 25-25
packages/email/templates/bulk-send-complete.tsx (1)
17-91
: LGTM! Well-structured email template.The component is well-organized with proper i18n support and conditional rendering of error messages.
apps/web/src/app/(dashboard)/templates/[id]/template-page-view.tsx (1)
115-115
: LGTM! Clean integration of bulk send feature.The TemplateBulkSendDialog is properly integrated with the required props.
apps/web/src/components/templates/template-bulk-send-dialog.tsx (1)
61-81
: 🧹 Nitpick (assertive)Consider using Blob URL cleanup in useEffect.
The URL.revokeObjectURL call should be in a cleanup function to ensure proper resource management.
+ import { useEffect } from 'react'; const onDownloadTemplate = () => { // ... existing code ... const url = window.URL.createObjectURL(blob); + useEffect(() => { + return () => { + window.URL.revokeObjectURL(url); + }; + }, [url]); - window.URL.revokeObjectURL(url); };Likely invalid or redundant comment.
}).catch((err) => { | ||
console.error(err); | ||
|
||
throw new AppError('DOCUMENT_SEND_FAILED'); | ||
}); | ||
}); |
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.
🛠️ Refactor suggestion
Include original error when sending document fails
Capture and include the original error message to aid in debugging.
Apply this diff:
await sendDocument({
documentId: document.id,
userId,
teamId,
requestMetadata,
}).catch((err) => {
console.error(err);
- throw new AppError('DOCUMENT_SEND_FAILED');
+ throw new AppError('DOCUMENT_SEND_FAILED', { cause: err });
});
📝 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.
}).catch((err) => { | |
console.error(err); | |
throw new AppError('DOCUMENT_SEND_FAILED'); | |
}); | |
}); | |
}).catch((err) => { | |
console.error(err); | |
throw new AppError('DOCUMENT_SEND_FAILED', { cause: err }); | |
}); | |
}); |
const results = { | ||
success: 0, | ||
failed: 0, | ||
errors: Array<string>(), |
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.
🧹 Nitpick (assertive)
Initialize errors array directly
You can initialize the errors
array using shorthand syntax.
Apply this diff:
const results = {
success: 0,
failed: 0,
- errors: Array<string>(),
+ errors: [],
};
📝 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.
const results = { | |
success: 0, | |
failed: 0, | |
errors: Array<string>(), | |
const results = { | |
success: 0, | |
failed: 0, | |
errors: [], |
const ZRecipientRowSchema = z.object({ | ||
name: z.string().optional(), | ||
email: z.union([ | ||
z.string().email({ message: 'Value must be a valid email or empty string' }), | ||
z.string().max(0, { message: 'Value must be a valid email or empty string' }), | ||
]), | ||
}); |
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.
🧹 Nitpick (assertive)
Simplify email validation schema
The email
field in ZRecipientRowSchema
can be simplified to allow for empty strings more concisely.
Apply this diff:
const ZRecipientRowSchema = z.object({
name: z.string().optional(),
- email: z.union([
- z.string().email({ message: 'Value must be a valid email or empty string' }),
- z.string().max(0, { message: 'Value must be a valid email or empty string' }),
- ]),
+ email: z.string().email({ message: 'Value must be a valid email' }).or(z.literal('')),
});
📝 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.
const ZRecipientRowSchema = z.object({ | |
name: z.string().optional(), | |
email: z.union([ | |
z.string().email({ message: 'Value must be a valid email or empty string' }), | |
z.string().max(0, { message: 'Value must be a valid email or empty string' }), | |
]), | |
}); | |
const ZRecipientRowSchema = z.object({ | |
name: z.string().optional(), | |
email: z.string().email({ message: 'Value must be a valid email' }).or(z.literal('')), | |
}); |
if (!template) { | ||
throw new Error('Template not found'); | ||
} |
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.
🛠️ Refactor suggestion
Use consistent error handling with AppError
Instead of throwing a generic Error
, use AppError
for consistency and to provide meaningful error codes.
Apply this diff:
if (!template) {
- throw new Error('Template not found');
+ throw new AppError('TEMPLATE_NOT_FOUND');
}
Committable suggestion skipped: line range outside the PR's diff.
export interface BulkSendCompleteEmailProps { | ||
userName: string; | ||
templateName: string; | ||
totalProcessed: number; | ||
successCount: number; | ||
failedCount: number; | ||
errors: string[]; | ||
assetBaseUrl?: string; | ||
} |
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.
🧹 Nitpick (assertive)
Consider using a more specific type for errors.
Instead of using string[], consider defining an error interface to make the error structure more maintainable.
- errors: string[];
+ errors: Array<{
+ message: string;
+ row?: number;
+ column?: string;
+ }>;
📝 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.
export interface BulkSendCompleteEmailProps { | |
userName: string; | |
templateName: string; | |
totalProcessed: number; | |
successCount: number; | |
failedCount: number; | |
errors: string[]; | |
assetBaseUrl?: string; | |
} | |
export interface BulkSendCompleteEmailProps { | |
userName: string; | |
templateName: string; | |
totalProcessed: number; | |
successCount: number; | |
failedCount: number; | |
errors: Array<{ | |
message: string; | |
row?: number; | |
column?: string; | |
}>; | |
assetBaseUrl?: string; | |
} |
export const ZBulkSendTemplateMutationSchema = z.object({ | ||
templateId: z.number(), | ||
teamId: z.number().optional(), | ||
csv: z.string().min(1), | ||
sendImmediately: 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.
🛠️ Refactor suggestion
Add CSV validations for better error handling.
Consider adding these validations to prevent issues during processing:
export const ZBulkSendTemplateMutationSchema = z.object({
templateId: z.number(),
teamId: z.number().optional(),
- csv: z.string().min(1),
+ csv: z.string()
+ .min(1)
+ .max(5 * 1024 * 1024) // 5MB limit
+ .refine(
+ (csv) => csv.includes('email,name'),
+ 'CSV must include email and name headers'
+ ),
sendImmediately: 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.
export const ZBulkSendTemplateMutationSchema = z.object({ | |
templateId: z.number(), | |
teamId: z.number().optional(), | |
csv: z.string().min(1), | |
sendImmediately: z.boolean(), | |
}); | |
export const ZBulkSendTemplateMutationSchema = z.object({ | |
templateId: z.number(), | |
teamId: z.number().optional(), | |
csv: z.string() | |
.min(1) | |
.max(5 * 1024 * 1024) // 5MB limit | |
.refine( | |
(csv) => csv.includes('email,name'), | |
'CSV must include email and name headers' | |
), | |
sendImmediately: z.boolean(), | |
}); |
const onSubmit = async (values: TBulkSendFormSchema) => { | ||
try { | ||
const csv = await values.file.text(); | ||
|
||
await uploadBulkSend({ | ||
templateId, | ||
teamId: team?.id, | ||
csv: csv, | ||
sendImmediately: values.sendImmediately, | ||
}); | ||
|
||
toast({ | ||
title: _(msg`Success`), | ||
description: _( | ||
msg`Your bulk send has been initiated. You will receive an email notification upon completion.`, | ||
), | ||
}); | ||
|
||
form.reset(); | ||
onSuccess?.(); | ||
} catch (err) { | ||
console.error(err); | ||
|
||
toast({ | ||
title: 'Error', | ||
description: 'Failed to upload CSV. Please check the file format and try again.', | ||
variant: 'destructive', | ||
}); | ||
} | ||
}; |
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.
🛠️ Refactor suggestion
Add client-side CSV validation.
The CSV content is only validated server-side. Adding client-side validation would improve user experience by catching format issues early.
Consider adding CSV parsing and validation before submission:
const validateCSV = async (file: File) => {
const content = await file.text();
const rows = content.split('\n').map(row => row.split(','));
if (rows.length > 100) {
throw new Error('CSV exceeds 100 rows limit');
}
// Validate header row matches template
const headers = rows[0];
const expectedHeaders = recipients.flatMap((_, index) => [
`recipient_${index + 1}_email`,
`recipient_${index + 1}_name`,
]);
if (!headers.every((h, i) => h === expectedHeaders[i])) {
throw new Error('Invalid CSV format');
}
};
const ZBulkSendFormSchema = z.object({ | ||
file: z.instanceof(File), | ||
sendImmediately: z.boolean().default(false), | ||
}); |
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.
Add file size and row count validation to form schema.
The UI mentions limits (4MB, 100 rows) but the form schema only validates file type.
Add these validations to prevent unnecessary server requests:
const ZBulkSendFormSchema = z.object({
- file: z.instanceof(File),
+ file: z.instanceof(File).refine(
+ (file) => file.size <= 4 * 1024 * 1024,
+ 'File size exceeds 4MB limit'
+ ),
sendImmediately: z.boolean().default(false),
});
📝 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.
const ZBulkSendFormSchema = z.object({ | |
file: z.instanceof(File), | |
sendImmediately: z.boolean().default(false), | |
}); | |
const ZBulkSendFormSchema = z.object({ | |
file: z.instanceof(File).refine( | |
(file) => file.size <= 4 * 1024 * 1024, | |
'File size exceeds 4MB limit' | |
), | |
sendImmediately: z.boolean().default(false), | |
}); |
await jobs.triggerJob({ | ||
name: 'internal.bulk-send-template', | ||
payload: { | ||
userId: user.id, | ||
teamId, | ||
templateId, | ||
csvContent: csv, | ||
sendImmediately, | ||
requestMetadata: extractNextApiRequestMetadata(ctx.req), | ||
}, | ||
}); |
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.
🧹 Nitpick (assertive)
Consider rate limiting bulk operations.
The bulk send operation could be resource-intensive. Consider adding rate limiting.
Add a rate limit check before triggering the job:
const BULK_SEND_RATE_LIMIT = 5; // operations per minute
const rateLimitKey = `bulk-send:${user.id}`;
// Add rate limiting logic before triggering job
const currentCount = await redis.incr(rateLimitKey);
if (currentCount === 1) {
await redis.expire(rateLimitKey, 60);
}
if (currentCount > BULK_SEND_RATE_LIMIT) {
throw new TRPCError({
code: 'TOO_MANY_REQUESTS',
message: 'Rate limit exceeded for bulk operations',
});
}
if (csv.length > 4 * 1024 * 1024) { | ||
throw new TRPCError({ | ||
code: 'BAD_REQUEST', | ||
message: 'File size exceeds 4MB limit', | ||
}); | ||
} |
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.
🛠️ Refactor suggestion
Add content type validation for CSV.
The file size check is good, but additional content validation would improve security.
Add basic CSV content validation:
+ const isValidCSV = (content: string) => {
+ const firstLine = content.split('\n')[0];
+ return firstLine.includes(',') && /^[^<>]*$/.test(content);
+ };
if (csv.length > 4 * 1024 * 1024) {
throw new TRPCError({
code: 'BAD_REQUEST',
message: 'File size exceeds 4MB limit',
});
}
+
+ if (!isValidCSV(csv)) {
+ throw new TRPCError({
+ code: 'BAD_REQUEST',
+ message: 'Invalid CSV format',
+ });
+ }
📝 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.
if (csv.length > 4 * 1024 * 1024) { | |
throw new TRPCError({ | |
code: 'BAD_REQUEST', | |
message: 'File size exceeds 4MB limit', | |
}); | |
} | |
const isValidCSV = (content: string) => { | |
const firstLine = content.split('\n')[0]; | |
return firstLine.includes(',') && /^[^<>]*$/.test(content); | |
}; | |
if (csv.length > 4 * 1024 * 1024) { | |
throw new TRPCError({ | |
code: 'BAD_REQUEST', | |
message: 'File size exceeds 4MB limit', | |
}); | |
} | |
if (!isValidCSV(csv)) { | |
throw new TRPCError({ | |
code: 'BAD_REQUEST', | |
message: 'Invalid CSV format', | |
}); | |
} |
Description
Implements a bulk send feature allowing users to upload a CSV file to create multiple documents from a template. Includes CSV template generation, background processing, and email notifications.
Changes Made
TemplateBulkSendDialog
with CSV upload/download functionalityTesting Performed
Resolves #1550