Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe updates introduce a new API route for disabling the auto-draft feature for users who have not used their last 10 drafts, including logic for querying relevant users, checking draft usage, deleting draft actions, logging, and error handling. The README is updated to clarify that the Google watch cron job is mandatory while others are optional, and it adds a new example cron job for running the disable-unused-auto-draft task daily at 3:00 AM. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant CronJob as Cron Job
participant API as /api/reply-tracker/disable-unused-auto-draft
participant DB as Database
CronJob->>API: Scheduled POST request with cron secret
API->>API: Authorize request (check secret)
API->>DB: Query users with auto-draft enabled
loop For each user
API->>DB: Fetch last 10 drafts older than 1 day
API->>DB: Check if any drafts were sent
alt None sent
API->>DB: Delete DRAFT_EMAIL actions for user
end
end
API->>API: Log progress and errors
API-->>CronJob: Return summary (users checked, disabled, errors)
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/web/app/api/reply-tracker/disable-unused-auto-draft/route.tsOops! Something went wrong! :( ESLint: 9.24.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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: 3
🧹 Nitpick comments (2)
README.md (1)
214-231: JSON snippet is not strictly valid – trailing comma after the last objectSome cron providers (and JSON linters) reject trailing commas. In the block below the last object ends with a comma on line 229:
{ "path": "/api/reply-tracker/disable-unused-auto-draft", "schedule": "0 3 * * *" },Removing the comma keeps the example copy‑paste‑able:
@@ { "path": "/api/reply-tracker/disable-unused-auto-draft", - "schedule": "0 3 * * *" - } + "schedule": "0 3 * * *" + }apps/web/app/api/reply-tracker/disable-unused-auto-draft/route.ts (1)
24-25: Timezone edge case:subDaysvs. “more than 24 h ago”
subDays(new Date(), 1)creates a timestamp exactly 24 h in the past.
If the job runs close to midnight UTC, drafts created “yesterday” but only a few hours old will already be included. That may disable auto‑draft prematurely for users in later time zones.Consider using
subHours(new Date(), 24)or even making the threshold configurable (e.g. 36 h) to avoid accidental disabling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
README.md(2 hunks)apps/web/app/api/reply-tracker/disable-unused-auto-draft/route.ts(1 hunks)apps/web/package.json(1 hunks)
🔇 Additional comments (1)
apps/web/package.json (1)
78-80: Confirm the need for the newai‑fallbackdependency and keep lock‑files in syncThe package is pinned at
0.1.2, but no references appear elsewhere in the PR. Please verify that:
- The new route (or other code) really imports
ai-fallback; otherwise the dependency will bloat the bundle.pnpm-lock.yaml(or equivalent) is committed to avoid CI drift.If the lib is only intended for future work, consider adding it when the code that consumes it lands.
| // Process each user | ||
| for (const user of usersWithAutoDraft) { | ||
| try { | ||
| // Find the last 10 drafts created for the user | ||
| const lastTenDrafts = await prisma.executedAction.findMany({ | ||
| where: { | ||
| executedRule: { | ||
| userId: user.id, | ||
| rule: { | ||
| systemType: SystemType.TO_REPLY, | ||
| }, | ||
| }, | ||
| type: ActionType.DRAFT_EMAIL, | ||
| draftId: { not: null }, | ||
| createdAt: { lt: oneDayAgo }, // Only check drafts older than a day | ||
| }, | ||
| orderBy: { | ||
| createdAt: "desc", | ||
| }, | ||
| take: MAX_DRAFTS_TO_CHECK, | ||
| select: { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
N + 1 queries – fetch drafts in batch instead of per‑user loop
The loop issues one executedAction.findMany per user. If thousands of users have auto‑draft enabled the route will:
- hammer the DB with many small queries,
- hold the Lambda/function warm longer and increase cold‑start cost,
- risk timing out for the 10 s default Vercel edge timeout (if not using background).
You can reduce round‑trips with a single query that groups by userId and aggregates the last 10 drafts per user, e.g.:
const draftsByUser = await prisma.executedAction.groupBy({
by: ["executedRule.userId"],
where: { /* same filters */ },
_count: { id: true },
_max: { createdAt: true },
take: MAX_DRAFTS_TO_CHECK,
});Then process the in‑memory groups.
This will scale linearly with returned rows instead of users.
apps/web/app/api/reply-tracker/disable-unused-auto-draft/route.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/app/api/reply-tracker/disable-unused-auto-draft/route.ts (2)
22-138: Comprehensive implementation with database performance concernsThe implementation is thorough with good error handling, logging, and filtering logic. However, it still contains the N+1 query pattern mentioned in previous reviews, which could cause performance issues at scale.
Consider retrieving draft usage statistics in a single aggregated query before processing individual users. Run this script to estimate the performance impact at your current scale:
#!/bin/bash # Check user scale and potential query performance impact # Count total users and rules to understand scale echo "Checking system scale:" rg -A 5 "user.findMany|usersWithAutoDraft" apps/web/ --include "*.ts" echo "" # Look for similar patterns in codebase that might use batch operations echo "Finding examples of batch operations:" rg -A 5 "groupBy|$transaction" apps/web/ --include "*.ts" | head -n 20
105-125: 🛠️ Refactor suggestionRace condition in check-then-act pattern
The current implementation checks if drafts were used and then deletes actions in separate database operations. This creates a race condition where a user might send a draft between the check and the deletion.
Implement the transaction-based approach suggested in the previous review to ensure atomicity:
- // Check if any of the drafts were sent - const anyDraftsSent = lastTenDrafts.some( - (draft) => draft.wasDraftSent === true || draft.draftSendLog, - ); - - // If none of the drafts were sent, disable auto-draft - if (!anyDraftsSent) { - logger.info("Disabling auto-draft for user - last 10 drafts not used", { - userId: user.id, - }); - - // Delete the DRAFT_EMAIL actions from all TO_REPLY rules - await prisma.action.deleteMany({ - where: { - rule: { - userId: user.id, - systemType: SystemType.TO_REPLY, - }, - type: ActionType.DRAFT_EMAIL, - content: null, - }, - }); - - results.usersDisabled++; - } + // Extract draft IDs for the verification in the transaction + const lastTenDraftIds = lastTenDrafts.map(draft => draft.id); + + // Use a transaction to ensure atomicity + await prisma.$transaction(async (tx) => { + // Re-check that none of the drafts were sent (could have changed since initial check) + const countUnused = await tx.executedAction.count({ + where: { + id: { in: lastTenDraftIds }, + OR: [ + { wasDraftSent: false }, + { draftSendLog: null }, + ], + }, + }); + + // If all drafts are still unused, proceed with deletion + if (countUnused === MAX_DRAFTS_TO_CHECK) { + logger.info("Disabling auto-draft for user - last 10 drafts not used", { + userId: user.id, + }); + + await tx.action.deleteMany({ + where: { + rule: { + userId: user.id, + systemType: SystemType.TO_REPLY, + }, + type: ActionType.DRAFT_EMAIL, + content: null, + }, + }); + + results.usersDisabled++; + } + });
🧹 Nitpick comments (1)
apps/web/app/api/reply-tracker/disable-unused-auto-draft/route.ts (1)
140-151: Commented code should be removedThe commented-out GET endpoint implementation should be removed before production deployment, as it's unused and could lead to confusion.
- // For easier local testing - // export const GET = withError(async (request) => { - // if (!hasCronSecret(request)) { - // captureException( - // new Error("Unauthorized request: api/auto-draft/disable-unused"), - // ); - // return new Response("Unauthorized", { status: 401 }); - // } - - // const results = await disableUnusedAutoDrafts(); - // return NextResponse.json(results); - // });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/reply-tracker/disable-unused-auto-draft/route.ts(1 hunks)
🔇 Additional comments (2)
apps/web/app/api/reply-tracker/disable-unused-auto-draft/route.ts (2)
13-14: Good configuration for background processingSetting
dynamicto "force-dynamic" ensures fresh data on each request, and the 5-minutemaxDurationgives this background task enough time to process many users without hitting timeout limits.
153-163: Appropriate POST endpoint with security measuresGood implementation using POST instead of GET for a state-changing operation. The authentication check with
hasPostCronSecretensures only authorized cron jobs can trigger this action.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores