fix: added cron to schedule task and split the notification to be its…#41
Conversation
… own task, using batchTrigger
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request refactors the risk task notifications system. It introduces a new function, Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/app/src/jobs/tasks/notifications/risk-task-notification.ts (1)
22-53: Consider enhancing error handling and logging.While the basic error handling is in place, consider these improvements:
- Log the specific task ID in error messages for better debugging
- Consider retrying the database update on transient errors
- Add error type checking for more specific error handling
Apply this diff to enhance error handling:
run: async (payload) => { const { task } = payload; try { const owner = task.owner; const timeUntilDue = task.dueDate ? formatDistance(task.dueDate, new Date(), { addSuffix: true }) : "soon"; - await db.riskMitigationTask.update({ - where: { id: task.id }, - data: { notifiedAt: new Date() }, - }); + try { + await db.riskMitigationTask.update({ + where: { id: task.id }, + data: { notifiedAt: new Date() }, + }); + } catch (dbError) { + logger.error(`Database error updating task ${task.id}: ${dbError}`); + throw dbError; + } await trigger({ name: TriggerEvents.TaskReminderInApp, user: { subscriberId: `${owner.organizationId}_${owner.id}`, email: owner.email, fullName: owner.email, organizationId: owner.organizationId, }, payload: { description: `${task.title} is due ${timeUntilDue}`, recordId: `/risk/${task.riskId}/tasks/${task.id}`, type: NotificationTypes.Task, }, }); } catch (error) { - logger.error(`Error sending risk task notification: ${error}`); + logger.error(`Error processing task ${task.id}: ${error}`); + if (error instanceof Error) { + logger.error(`Stack trace: ${error.stack}`); + } + throw error; // Re-throw to trigger retry mechanism } },apps/app/src/jobs/tasks/notifications/risk-task-schedule.ts (3)
5-8: Consider adjusting cron schedule for optimal performance.The hourly cron schedule (
0 * * * *) might be too frequent for this task. Consider running it less frequently (e.g., every 4 hours) to reduce database load, unless there's a specific requirement for hourly notifications.- cron: "0 * * * *", + cron: "0 */4 * * *", // Runs every 4 hours
9-14: Use date-fns for consistent date handling.Since date-fns is already a dependency (used in risk-task-notification.ts), consider using it here for consistent date manipulation across the codebase.
+import { addWeeks } from "date-fns"; + export const sendRiskTaskSchedule = schedules.task({ id: "risk-task-schedule", cron: "0 * * * *", run: async () => { const now = new Date(); - const upcomingThreshold = new Date(now.getTime() + 7 * 24 * 60 * 60 * 1000); + const upcomingThreshold = addWeeks(now, 1);
39-54: Enhance type safety in filter predicate.The type predicate in the filter function could be improved for better type safety and readability.
- const triggerPayloads = tasks - .filter((task): task is (typeof task & { - owner: { id: string; email: string; organizationId: string } - }) => Boolean(task.owner?.email && task.owner.organizationId)) + interface ValidTask extends typeof tasks[number] { + owner: { + id: string; + email: string; + organizationId: string; + }; + } + + const isValidTask = (task: typeof tasks[number]): task is ValidTask => { + return Boolean( + task.owner?.id && + task.owner?.email && + task.owner?.organizationId + ); + }; + + const triggerPayloads = tasks + .filter(isValidTask)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/app/src/jobs/tasks/notifications/risk-task-notification.ts(1 hunks)apps/app/src/jobs/tasks/notifications/risk-task-notifications.ts(0 hunks)apps/app/src/jobs/tasks/notifications/risk-task-schedule.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/app/src/jobs/tasks/notifications/risk-task-notifications.ts
🔇 Additional comments (1)
apps/app/src/jobs/tasks/notifications/risk-task-notification.ts (1)
7-21: LGTM! Well-structured schema definition.The schema is well-defined using Zod, with clear validation for all required fields.
| if (triggerPayloads.length > 0) { | ||
| await sendRiskTaskNotification.batchTrigger( | ||
| triggerPayloads, | ||
| ); | ||
|
|
||
| logger.info(`Triggered ${triggerPayloads.length} task notifications`); | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| totalTasks: tasks.length, | ||
| triggeredTasks: triggerPayloads.length, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for batch trigger operation.
The batch trigger operation should include error handling to ensure failures are properly logged and handled.
if (triggerPayloads.length > 0) {
+ try {
await sendRiskTaskNotification.batchTrigger(
triggerPayloads,
);
logger.info(`Triggered ${triggerPayloads.length} task notifications`);
+ } catch (error) {
+ logger.error(`Failed to trigger batch notifications: ${error}`);
+ return {
+ success: false,
+ totalTasks: tasks.length,
+ triggeredTasks: 0,
+ error: error instanceof Error ? error.message : String(error),
+ };
+ }
}
return {
success: true,
totalTasks: tasks.length,
triggeredTasks: triggerPayloads.length,
};📝 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 (triggerPayloads.length > 0) { | |
| await sendRiskTaskNotification.batchTrigger( | |
| triggerPayloads, | |
| ); | |
| logger.info(`Triggered ${triggerPayloads.length} task notifications`); | |
| } | |
| return { | |
| success: true, | |
| totalTasks: tasks.length, | |
| triggeredTasks: triggerPayloads.length, | |
| }; | |
| } | |
| if (triggerPayloads.length > 0) { | |
| try { | |
| await sendRiskTaskNotification.batchTrigger( | |
| triggerPayloads, | |
| ); | |
| logger.info(`Triggered ${triggerPayloads.length} task notifications`); | |
| } catch (error) { | |
| logger.error(`Failed to trigger batch notifications: ${error}`); | |
| return { | |
| success: false, | |
| totalTasks: tasks.length, | |
| triggeredTasks: 0, | |
| error: error instanceof Error ? error.message : String(error), | |
| }; | |
| } | |
| } | |
| return { | |
| success: true, | |
| totalTasks: tasks.length, | |
| triggeredTasks: triggerPayloads.length, | |
| }; | |
| } |
| const tasks = await db.riskMitigationTask.findMany({ | ||
| where: { | ||
| dueDate: { gte: now, lte: upcomingThreshold }, | ||
| status: { in: ["open", "pending"] }, | ||
| notifiedAt: null, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| dueDate: true, | ||
| notifiedAt: true, | ||
| riskId: true, | ||
| title: true, | ||
| owner: { | ||
| select: { | ||
| id: true, | ||
| email: true, | ||
| name: true, | ||
| organizationId: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pagination for large result sets.
The findMany query might return a large number of tasks. Consider implementing pagination to handle the results in batches.
+ const BATCH_SIZE = 100;
+ let processedCount = 0;
+
const tasks = await db.riskMitigationTask.findMany({
where: {
dueDate: { gte: now, lte: upcomingThreshold },
status: { in: ["open", "pending"] },
notifiedAt: null,
},
+ take: BATCH_SIZE,
select: {
id: true,
dueDate: true,
notifiedAt: true,
riskId: true,
title: true,
owner: {
select: {
id: true,
email: true,
name: true,
organizationId: true,
},
},
},
});
+ processedCount = tasks.length;
+
+ logger.info(`Processing ${processedCount} tasks in this batch`);📝 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 tasks = await db.riskMitigationTask.findMany({ | |
| where: { | |
| dueDate: { gte: now, lte: upcomingThreshold }, | |
| status: { in: ["open", "pending"] }, | |
| notifiedAt: null, | |
| }, | |
| select: { | |
| id: true, | |
| dueDate: true, | |
| notifiedAt: true, | |
| riskId: true, | |
| title: true, | |
| owner: { | |
| select: { | |
| id: true, | |
| email: true, | |
| name: true, | |
| organizationId: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| const BATCH_SIZE = 100; | |
| let processedCount = 0; | |
| const tasks = await db.riskMitigationTask.findMany({ | |
| where: { | |
| dueDate: { gte: now, lte: upcomingThreshold }, | |
| status: { in: ["open", "pending"] }, | |
| notifiedAt: null, | |
| }, | |
| take: BATCH_SIZE, | |
| select: { | |
| id: true, | |
| dueDate: true, | |
| notifiedAt: true, | |
| riskId: true, | |
| title: true, | |
| owner: { | |
| select: { | |
| id: true, | |
| email: true, | |
| name: true, | |
| organizationId: true, | |
| }, | |
| }, | |
| }, | |
| }); | |
| processedCount = tasks.length; | |
| logger.info(`Processing ${processedCount} tasks in this batch`); |
1 participant
… own task, using batchTrigger
Summary by CodeRabbit
New Features
Chores