feat: Implement CRON jobs API #676
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary
Testing
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements CRON jobs API functionality for VitNode, introducing a comprehensive system for scheduling and managing background tasks. The implementation includes database schema, API routes, adapters, and configuration updates.
Key changes:
- Adds CRON jobs infrastructure with database table, API module, and scheduling framework
- Refactors plugin configuration to use explicit
pluginIdinstead of spread syntax - Updates package dependencies and removes Prettier configuration references
Reviewed Changes
Copilot reviewed 76 out of 77 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vitnode/src/database/cron.ts | Creates core_cron database table schema |
| packages/vitnode/src/api/lib/cron.ts | Defines CRON adapter interface and buildCron function |
| packages/vitnode/src/api/modules/cron/ | Implements CRON API module with routes and test job |
| packages/vitnode/src/api/lib/module.ts | Adds cronJobs support to module builder |
| packages/vitnode/src/api/lib/plugin.ts | Integrates CRON jobs into plugin system |
| packages/vitnode/src/vitnode.config.ts | Adds cronAdapter configuration option |
| Multiple plugin files | Refactors from spread CONFIG_PLUGIN to explicit pluginId |
| Migration files | Database migration for core_cron table |
| Documentation files | Updates SSO, email, and other docs with formatting fixes |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| // biome-ignore lint/suspicious/noConsole: needed for testing cron functionality | ||
| console.log("Cron job triggered", c.get("core").cron); |
There was a problem hiding this comment.
The console.log statement should use the logger from context instead of direct console access. Replace with c.get('log').info('Cron job triggered', c.get('core').cron).
| // biome-ignore lint/suspicious/noConsole: needed for testing cron functionality | |
| console.log("Cron job triggered", c.get("core").cron); | |
| c.get('log').info('Cron job triggered', c.get('core').cron); |
| handler: () => { | ||
| // biome-ignore lint/suspicious/noConsole: needed for testing cron functionality | ||
| console.log("Test cron job executed"); |
There was a problem hiding this comment.
The console.log statement should use the logger from context instead of direct console access. Consider using the context parameter in the handler function to access the logger.
| handler: () => { | |
| // biome-ignore lint/suspicious/noConsole: needed for testing cron functionality | |
| console.log("Test cron job executed"); | |
| handler: (context) => { | |
| context.logger.log("Test cron job executed"); |
|
|
||
| export const NodeCronAdapter = (): CronAdapter => { | ||
| return { | ||
| schedule() {}, |
There was a problem hiding this comment.
The schedule method is empty and doesn't implement any functionality. This will cause CRON jobs to not be scheduled when using the NodeCronAdapter.
| }, | ||
| }, | ||
| handler: c => { | ||
| // biome-ignore lint/suspicious/noConsole: needed for testing cron functionality | ||
| console.log("Cron job triggered", c.get("core").cron); | ||
|
|
||
| return c.text("Not implemented", 200); |
There was a problem hiding this comment.
The route returns 'Not implemented' with a 200 status code. This should either return a proper implementation or use a 501 status code to indicate the feature is not implemented.
| }, | |
| }, | |
| handler: c => { | |
| // biome-ignore lint/suspicious/noConsole: needed for testing cron functionality | |
| console.log("Cron job triggered", c.get("core").cron); | |
| return c.text("Not implemented", 200); | |
| 501: { | |
| content: { | |
| "text/plain": { | |
| schema: z.string(), | |
| }, | |
| }, | |
| description: "Not implemented", | |
| }, | |
| }, | |
| }, | |
| handler: c => { | |
| // biome-ignore lint/suspicious/noConsole: needed for testing cron functionality | |
| console.log("Cron job triggered", c.get("core").cron); | |
| return c.text("Not implemented", 501); |
|
@codex make code review. Find issue about performerce. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| plugins: plugins.map(plugin => ({ | ||
| id: plugin.pluginId, | ||
| })), | ||
| cron: plugins.flatMap(plugin => | ||
| plugin.cronJobs.map(cronJob => ({ | ||
| pluginId: plugin.pluginId, | ||
| module: cronJob.module, | ||
| name: cronJob.name, | ||
| schedule: cronJob.schedule, | ||
| handler: cronJob.handler, | ||
| description: cronJob.description, | ||
| })), |
There was a problem hiding this comment.
[P2] Avoid rebuilding cron metadata on every request
The global middleware rebuilds the list of cron jobs on every request by calling plugins.flatMap and map even though the set of plugins and cron definitions is static for the lifetime of the server. This allocates a new array of job objects and rewraps each handler for every request, adding avoidable work on all API traffic. Consider computing the cron metadata once when the middleware is created and reusing the same reference instead of recalculating it per request, especially if many cron jobs are registered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Precomputed plugin and cron metadata once during middleware initialization to avoid reallocations on every request, reducing overhead for API trafficpackages/vitnode/src/api/middlewares/global.middleware.tsL100-L113packages/vitnode/src/api/middlewares/global.middleware.tsL182-L183
Testing
⚠️ pnpm lint(fails: formatting issues in unrelated files)- ✅
pnpm lint packages/vitnode/src/api/middlewares/global.middleware.ts - ✅
pnpm test
…rotected for this route
Improving Documentation
pnpm lint:fixto fix formatting issues before opening the PR.Description
What?
Why?
How?