Add background auto-scheduling for tasks #3291
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis set of changes removes user authentication and permission checks from the calendar auto-schedule API route, switches the Supabase client to an admin client, and updates the default Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as scheduleTasksTrigger
participant Helper as schedulableTasksHelper
participant API as /api/[wsId]/calendar/auto-schedule
participant DB as Database
Scheduler->>Helper: schedulableTasksHelper(ws_id)
Helper->>API: POST /api/[wsId]/calendar/auto-schedule
API->>DB: Fetch events, schedule, upsert
API-->>Helper: JSON response
Helper-->>Scheduler: Success/Failure
Estimated code review effort3 (30–60 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @phatgg221, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request implements a new background service designed to automate the scheduling of tasks within workspaces. It achieves this by setting up a recurring job that invokes the existing calendar auto-scheduling API endpoint. To enable this background operation, the API endpoint has been adapted to utilize an admin Supabase client and bypass user-specific authentication and permission checks, ensuring it can execute with the necessary privileges.
Highlights
- Background Task Scheduling: Introduces a new scheduled task (
schedule-tasks) using@trigger.dev/sdkto periodically trigger automatic calendar scheduling for all active workspaces. This task is configured to run every minute. - API Endpoint Adaptation for Background Use: The existing
/api/[wsId]/calendar/auto-scheduleendpoint has been modified to support calls from a background service. This includes switching tocreateAdminClient()for Supabase interactions and removing user-level authentication and permission checks, allowing it to run with elevated privileges without a user context. - New Scheduling Helper Modules: Adds two new files,
schedule-tasks-helper.tsandschedule-tasks.ts, to encapsulate the logic for initiating and managing the background scheduling process, including making the necessary API calls. - Google Calendar Event Locking Default: Google Calendar events synced into the system will now default to
locked: truewhen formatted for the database, indicating they are not intended for further automatic modification by the system.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a background task to automatically schedule calendar events. The overall approach is good, but there are several critical issues that need to be addressed before merging.
Most importantly, there are significant security vulnerabilities: a publicly exposed API endpoint without authentication and hardcoded credentials in the source code. Additionally, there's a critical logic bug in the new scheduled task that will prevent it from working correctly. I've provided detailed comments and suggestions to fix these issues.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
🔧 Biome Check ReportIssues Found: 0📊 Analysis Summary
🔍 Issue Breakdown
🎉 All Issues Resolved!Your code is now 100% clean! Great job! 🏆
🤖 Auto-generated by Biome Check workflow • Last updated: 7/24/2025, 3:27:53 PM
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3291 +/- ##
==========================================
- Coverage 1.50% 1.49% -0.01%
==========================================
Files 1918 1920 +2
Lines 248066 248098 +32
Branches 2605 2604 -1
==========================================
- Hits 3721 3720 -1
- Misses 242779 242811 +32
- Partials 1566 1567 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/trigger/schedule-tasks.ts (1)
22-30: Fix critical variable scoping and logic errors.There are critical issues in this code block:
Variable scoping error: The
resultvariable is declared withconstinside theforloop (line 25), making it inaccessible outside the loop's scope. This will cause a compile error at lines 28-30.Logic error: Even if the scoping were fixed, the current logic only checks the result of the last workspace, ignoring failures in previous workspaces.
Apply this fix to handle results properly:
for (const workspace of workspaces) { const ws_id = workspace.ws_id; console.log(`Processing workspace: ${ws_id}`); const result = await schedulableTasksHelper(ws_id); + if (!result.success) { + // Fail fast on the first error to ensure the task run is marked as failed. + throw new Error(result.error || `Schedule tasks failed for workspace ${ws_id}`); + } } - if (!result.success) { - throw new Error(result.error || 'Schedule tasks failed'); - } console.log('=== Schedule tasks completed ==='); - return result; + return { success: true, message: 'All workspaces processed successfully.' };apps/web/src/app/api/[wsId]/calendar/auto-schedule/route.ts (1)
7-7: Critical security vulnerability: API endpoint is now publicly accessible.By switching from
createClienttocreateAdminClientand removing authentication/permission checks (as mentioned in the AI summary), this API endpoint has become publicly accessible and unprotected. Anyone who knows a validwsIdcan now trigger the auto-scheduling process.This creates a significant security risk. You should implement a secure authentication mechanism for the scheduled task, such as:
- Token-based authentication: Check for a secret token in the
Authorizationheader- IP allowlisting: Restrict access to specific IP ranges
- Service-to-service authentication: Use a dedicated service key
Example secure implementation:
export async function POST( request: NextRequest, { params }: { params: Promise<{ wsId: string }> } ) { + // Check for service token + const authHeader = request.headers.get('Authorization'); + const expectedToken = process.env.SCHEDULE_SERVICE_TOKEN; + + if (!authHeader || authHeader !== `Bearer ${expectedToken}` || !expectedToken) { + return NextResponse.json( + { error: 'Unauthorized' }, + { status: 401 } + ); + } const supabase = createAdminClient(); // ... rest of the logicAnd update the helper to include the token:
// In schedule-tasks-helper.ts const response = await fetch(url, { method: 'POST', headers: { 'Content-Type': 'application/json', 'Authorization': `Bearer ${process.env.SCHEDULE_SERVICE_TOKEN}`, }, });Also applies to: 53-53
packages/trigger/schedule-tasks-helper.ts (1)
1-5: Improve type definition for better clarity and type safety.The type definition has several issues:
- The name
Paramsis misleading - this represents a response/result type, not parameters- Using
anyfor thedataproperty reduces type safetyConsider renaming and improving the type definition:
-type Params = { +type SchedulableTasksResult = { success: boolean; - data?: any; + data?: { message: string; scheduledCount?: number }; // Or define based on actual API response error?: string; };
🧹 Nitpick comments (2)
packages/trigger/schedule-tasks-helper.ts (2)
10-16: URL construction logic is well-structured.The environment variable fallback chain and URL construction are implemented correctly. The hard-coded
stream=falseparameter appears intentional for background scheduling tasks.Consider making the localhost port configurable:
- 'http://localhost:7803'; + process.env.DEV_PORT ? `http://localhost:${process.env.DEV_PORT}` : 'http://localhost:7803';
17-29: HTTP request implementation is solid.The POST request setup and error handling are well-implemented. The status check and descriptive error messages will help with debugging.
Consider adding a timeout to prevent hanging requests in a background task:
const response = await fetch(fullUrl, { method: 'POST', headers: { 'Content-Type': 'application/json', }, + signal: AbortSignal.timeout(30000), // 30 second timeout });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/src/app/api/[wsId]/calendar/auto-schedule/route.ts(2 hunks)packages/trigger/__tests__/google-calendar-batched-sync.test.ts(3 hunks)packages/trigger/google-calendar-incremental-sync.ts(2 hunks)packages/trigger/google-calendar-sync.ts(2 hunks)packages/trigger/schedule-tasks-helper.ts(1 hunks)packages/trigger/schedule-tasks.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
packages/trigger/google-calendar-incremental-sync.ts (1)
Learnt from: DennieDan
PR: #2891
File: packages/ui/src/hooks/use-calendar.tsx:1511-1513
Timestamp: 2025-05-21T09:22:15.348Z
Learning: In the GoogleCalendarSettings component's handleSyncNow function, the boolean return value from syncGoogleCalendarNow is used to determine if changes were made during a successful sync, while errors are handled through a separate try/catch block with detailed error messages.
packages/trigger/__tests__/google-calendar-batched-sync.test.ts (1)
Learnt from: DennieDan
PR: #2891
File: packages/ui/src/hooks/use-calendar.tsx:1511-1513
Timestamp: 2025-05-21T09:22:15.348Z
Learning: In the GoogleCalendarSettings component's handleSyncNow function, the boolean return value from syncGoogleCalendarNow is used to determine if changes were made during a successful sync, while errors are handled through a separate try/catch block with detailed error messages.
packages/trigger/google-calendar-sync.ts (1)
Learnt from: DennieDan
PR: #2891
File: packages/ui/src/hooks/use-calendar.tsx:1511-1513
Timestamp: 2025-05-21T09:22:15.348Z
Learning: In the GoogleCalendarSettings component's handleSyncNow function, the boolean return value from syncGoogleCalendarNow is used to determine if changes were made during a successful sync, while errors are handled through a separate try/catch block with detailed error messages.
🧬 Code Graph Analysis (1)
apps/web/src/app/api/[wsId]/calendar/auto-schedule/route.ts (1)
packages/supabase/src/next/server.ts (1)
createAdminClient(42-53)
🔇 Additional comments (10)
packages/trigger/google-calendar-sync.ts (2)
5-5: LGTM on type-only import!Converting to a type-only import is a good practice that ensures the
calendar_v3type is used solely for type annotations without including runtime code.
74-74: Confirm defaultlocked: truefor Google Calendar sync is intentionalLocking imported Google Calendar events by default in packages/trigger/google-calendar-sync.ts aligns with the rest of the system:
- The auto-scheduler separates out
lockedevents to prevent rescheduling (it pushes them intolockedEventsand only reschedules flexible events).- The UI disables drag/resize/edit for locked events.
- The AI scheduling algorithm treats locked events as fixed placeholders.
No further changes needed.
packages/trigger/google-calendar-incremental-sync.ts (2)
1-1: LGTM on import reorganization!Moving the import to the top of the import block improves code organization.
27-28: LGTM on variable initialization cleanup!Removing the explicit
undefinedinitialization is good cleanup since TypeScript variables areundefinedby default when declared without an initializer.packages/trigger/__tests__/google-calendar-batched-sync.test.ts (1)
147-147: LGTM on test updates to reflect locked state change!The test assertions have been correctly updated to expect
locked: true, which is consistent with the default locked state change in theformatEventForDbfunction. This ensures tests accurately reflect the current behavior.Also applies to: 157-157, 169-169
packages/trigger/schedule-tasks.ts (1)
8-8: Appropriate cron frequency chosen.The cron pattern
15 * * * *(hourly at the 15th minute) is much more appropriate than the previously suggested every-minute execution. This addresses the performance and cost concerns while still providing regular scheduling updates.packages/trigger/schedule-tasks-helper.ts (4)
7-9: Function declaration looks good.The async function declaration and initial logging are well-structured. The parameter naming is clear and the console logging is appropriate for a background task.
30-37: Response handling in success case is correct.The JSON parsing, logging, and return structure are properly implemented for the success path.
38-44: Error handling is comprehensive and well-structured.The try-catch implementation properly handles both Error instances and unknown error types, with appropriate logging for background task debugging.
47-47: Export statement is correct.The named export syntax properly exposes the helper function for use in other modules.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| for (const workspace of workspaces) { | ||
| const ws_id = workspace.ws_id; | ||
| console.log(`Processing workspace: ${ws_id}`); | ||
| const result = await schedulableTasksHelper(ws_id); | ||
| } | ||
|
|
||
| if (!result.success) { | ||
| throw new Error(result.error || 'Schedule tasks failed'); | ||
| } |
There was a problem hiding this comment.
There's a scope issue with the result variable. It's declared inside the for loop at line 25 but then referenced outside the loop at line 28, which will cause a ReferenceError at runtime.
To fix this, either:
- Declare
resultbefore the loop and update it inside the loop, or - Move the success check inside the loop to validate each workspace individually
Example fix for option 1:
let result = { success: true };
for (const workspace of workspaces) {
const ws_id = workspace.ws_id;
console.log(`Processing workspace: ${ws_id}`);
result = await schedulableTasksHelper(ws_id);
if (!result.success) {
break;
}
}| for (const workspace of workspaces) { | |
| const ws_id = workspace.ws_id; | |
| console.log(`Processing workspace: ${ws_id}`); | |
| const result = await schedulableTasksHelper(ws_id); | |
| } | |
| if (!result.success) { | |
| throw new Error(result.error || 'Schedule tasks failed'); | |
| } | |
| let result = { success: true }; | |
| for (const workspace of workspaces) { | |
| const ws_id = workspace.ws_id; | |
| console.log(`Processing workspace: ${ws_id}`); | |
| result = await schedulableTasksHelper(ws_id); | |
| if (!result.success) { | |
| break; | |
| } | |
| } | |
| if (!result.success) { | |
| throw new Error(result.error || 'Schedule tasks failed'); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| for (const workspace of workspaces) { | ||
| const ws_id = workspace.ws_id; | ||
| console.log(`Processing workspace: ${ws_id}`); | ||
| const result = await schedulableTasksHelper(ws_id); | ||
| } | ||
|
|
||
| if (!result.success) { | ||
| throw new Error(result.error || 'Schedule tasks failed'); | ||
| } |
There was a problem hiding this comment.
There's a logical issue in the error handling for workspace processing. The code checks result.success after the loop completes, but result will only contain the outcome from the last workspace processed. This means failures in earlier workspaces would be ignored if the final workspace succeeds.
Consider either:
- Checking each result inside the loop and handling failures immediately
- Collecting all results in an array and evaluating overall success after the loop
- Using a pattern like
Promise.all()with appropriate error handling
This ensures all workspace processing outcomes are properly accounted for in determining the overall success of the operation.
| for (const workspace of workspaces) { | |
| const ws_id = workspace.ws_id; | |
| console.log(`Processing workspace: ${ws_id}`); | |
| const result = await schedulableTasksHelper(ws_id); | |
| } | |
| if (!result.success) { | |
| throw new Error(result.error || 'Schedule tasks failed'); | |
| } | |
| const results = []; | |
| for (const workspace of workspaces) { | |
| const ws_id = workspace.ws_id; | |
| console.log(`Processing workspace: ${ws_id}`); | |
| const result = await schedulableTasksHelper(ws_id); | |
| results.push({ ws_id, result }); | |
| } | |
| const failures = results.filter(item => !item.result.success); | |
| if (failures.length > 0) { | |
| const errorMessages = failures.map(f => | |
| `Workspace ${f.ws_id}: ${f.result.error || 'unknown error'}` | |
| ); | |
| throw new Error( | |
| `Schedule tasks failed for ${failures.length} workspace(s): ${errorMessages.join('; ')}` | |
| ); | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| @@ -50,7 +50,7 @@ export async function POST( | |||
| request: NextRequest, | |||
| { params }: { params: Promise<{ wsId: string }> } | |||
| ) { | |||
| const supabase = createClient(); | |||
| const supabase = createAdminClient(); | |||
There was a problem hiding this comment.
- if you use admin client, your
supabasevariable should be namedsbAdminto avoid confusion. - you must add auth and permission checks if you are using
sbAdmin.
| for (const workspace of workspaces) { | ||
| const ws_id = workspace.ws_id; | ||
| console.log(`Processing workspace: ${ws_id}`); | ||
| const result = await schedulableTasksHelper(ws_id); |
There was a problem hiding this comment.
please convert this into a trigger task so it can be ran in parallel @phatgg221 @DennieDan
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/web/.env.example (3)
39-41: Swap Polar keys to satisfy dotenv-linter ordering
dotenv-linterwarns becausePOLAR_ACCESS_TOKENshould precedePOLAR_WEBHOOK_SECRET. Re-ordering avoids CI noise and keeps alphabetical grouping.# Payment Polar.sh -POLAR_WEBHOOK_SECRET=YOUR_POLAR_WEBHOOK_SECRET -POLAR_ACCESS_TOKEN=YOUR_POLAR_ACCESS_TOKEN +POLAR_ACCESS_TOKEN=YOUR_POLAR_ACCESS_TOKEN +POLAR_WEBHOOK_SECRET=YOUR_POLAR_WEBHOOK_SECRET
43-45: Provide explicit placeholders for new Trigger.dev keysLeaving values empty makes it unclear whether a value is required or optional and causes
.envdiff-noise when developers fill them in. Mirror the style used above by addingYOUR_…placeholders:-# Trigger.dev -TRIGGER_API_PROD= -VERCEL_TRIGGER_API_KEY= +# Trigger.dev +TRIGGER_API_PROD=YOUR_TRIGGER_API_PROD_KEY +VERCEL_TRIGGER_API_KEY=YOUR_VERCEL_TRIGGER_API_KEY
45-45: Add a trailing newlineMost linters (including
dotenv-linter) expect a newline at EOF.VER-CEL_TRIGGER_API_KEY=YOUR_VERCEL_TRIGGER_API_KEY +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/.env.example(1 hunks)apps/web/src/app/api/[wsId]/calendar/auto-schedule/route.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/api/[wsId]/calendar/auto-schedule/route.ts
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
apps/web/.env.example
[warning] 41-41: [UnorderedKey] The POLAR_ACCESS_TOKEN key should go before the POLAR_WEBHOOK_SECRET key
[warning] 45-45: [EndingBlankLine] No blank line at the end of the file
⏰ 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). (7)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy-Preview
- GitHub Check: Verify generated types
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
vhpx
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Chores
Bug Fixes