Worker - actionsToRun[] - determine actions needing run#4
Worker - actionsToRun[] - determine actions needing run#4sweetmantech wants to merge 1 commit intomainfrom
Conversation
WalkthroughThe scheduling logic in Changes
Poem
✨ Finishing Touches
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 (
|
![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 (3)
package.json (1)
17-18: Consider redundancy betweennode-cronandcron-parser.Unless another module still depends on
node-cron, the new logic only relies oncron-parser. Maintaining two cron libraries increases bundle size and cognitive load.
Audit the codebase and keep only the one actually used.controllers/CronController.ts (2)
7-15: Duplicate “enabled” check is unnecessary noise.
getScheduledActions()already filtersenabled = true.
Removing the extra guard simplifies the helper and reduces branching:-function shouldRunAction(action: ScheduledAction, now: Date): boolean { - if (!action.enabled) return false; +function shouldRunAction(action: ScheduledAction, now: Date): boolean {
48-53: High-volume optimisation: avoid parsing identical schedules repeatedly.If many actions share the same cron string,
cron-parseris invoked N times per tick.
CacheparseExpressionresults keyed by schedule during a single run:const cache = new Map<string, parser.CronExpression>(); function getCron(schedule: string) { if (!cache.has(schedule)) cache.set(schedule, parser.parseExpression(schedule)); return cache.get(schedule)!; }Small change, significant CPU savings at scale.
📜 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 (2)
controllers/CronController.ts(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
controllers/CronController.ts (1)
lib/supabase/scheduled_actions/getScheduledActions.ts (2)
ScheduledAction(4-5)getScheduledActions(10-21)
🔇 Additional comments (1)
package.json (1)
17-18: Add typings or verify bundled types forcron-parser.
cron-parserships with TypeScript typings only on versions ≥ 4.10.0. You added^4.9.0, which still requires the separate@types/cron-parserpackage.
Without them,parserwill be typed asany, losing compile-time checks inCronController.ts.- "cron-parser": "^4.9.0" + "cron-parser": "^4.10.1", // bundled types + "@types/cron-parser": "^4.6.3" // remove when upgrading ≥4.10Choose either upgrade to ≥ 4.10 or add the
@typesdev-dependency.
| // If never run, and next_run is now or earlier, run it | ||
| if (!action.last_run) { | ||
| if (!action.next_run) return true; // If next_run and last_run are null, run it | ||
| return new Date(action.next_run) <= now; | ||
| } | ||
|
|
||
| // If next_run is now or earlier, and last_run is before next_run, run it | ||
| if (action.next_run && new Date(action.next_run) <= now) { | ||
| if ( | ||
| !action.last_run || | ||
| new Date(action.last_run) < new Date(action.next_run) | ||
| ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated Date construction is wasteful & can cause subtle TZ mis-parsing.
new Date(action.next_run) is invoked multiple times and assumes the DB string includes a timezone.
If Supabase returns "2025-06-21 12:00:00" (no TZ), Node treats it as local time, whereas cron-parser defaults to system TZ, leading to silent drift on servers not in UTC.
- if (!action.last_run) {
- if (!action.next_run) return true; // If next_run and last_run are null, run it
- return new Date(action.next_run) <= now;
+ if (!action.last_run) {
+ if (!action.next_run) return true;
+ const nextRun = new Date(`${action.next_run}Z`); // force UTC
+ return nextRun <= now;
}Cache the parsed dates once per action (const nextRun = …; const lastRun = …) to avoid repeated parsing and make the TZ handling explicit.
📝 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 never run, and next_run is now or earlier, run it | |
| if (!action.last_run) { | |
| if (!action.next_run) return true; // If next_run and last_run are null, run it | |
| return new Date(action.next_run) <= now; | |
| } | |
| // If next_run is now or earlier, and last_run is before next_run, run it | |
| if (action.next_run && new Date(action.next_run) <= now) { | |
| if ( | |
| !action.last_run || | |
| new Date(action.last_run) < new Date(action.next_run) | |
| ) { | |
| return true; | |
| } | |
| // If never run, and next_run is now or earlier, run it | |
| if (!action.last_run) { | |
| if (!action.next_run) return true; | |
| const nextRun = new Date(`${action.next_run}Z`); // force UTC | |
| return nextRun <= now; | |
| } | |
| // If next_run is now or earlier, and last_run is before next_run, run it | |
| if (action.next_run && new Date(action.next_run) <= now) { | |
| if ( | |
| !action.last_run || | |
| new Date(action.last_run) < new Date(action.next_run) | |
| ) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In controllers/CronController.ts around lines 11 to 24, multiple calls to new
Date() on action.next_run and action.last_run cause repeated parsing and
potential timezone inconsistencies. Fix this by parsing action.next_run and
action.last_run once each into constants (e.g., nextRun and lastRun) at the
start of the function or block, then use these constants throughout the logic to
avoid repeated Date construction and clarify timezone handling.
| // Fallback: use cron-parser to check if a run is due | ||
| try { | ||
| const interval = parser.parseExpression(action.schedule, { | ||
| currentDate: now, | ||
| }); | ||
| const prev = interval.prev().toDate(); | ||
| // If last_run is before the previous scheduled time, it's due | ||
| if (!action.last_run || new Date(action.last_run) < prev) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
prev() + strict < risks skipping on-time executions.
cron-parser’s prev() returns the previous schedule excluding the currentDate by default.
Scenario: schedule * * * * * (every minute); now is exactly on the minute and last_run is 59 s ago.
prev() yields the previous minute, so last_run < prev is false, and the job is skipped even though it should run.
Either use interval.prev(true) (inclusive) or compare against interval.next():
-const prev = interval.prev().toDate();
-if (!action.last_run || new Date(action.last_run) < prev) {
+const next = interval.next().toDate();
+if (!action.last_run || new Date(action.last_run) < next) {
return true;
}This guarantees an execution whenever now aligns with the cron expression.
📝 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.
| // Fallback: use cron-parser to check if a run is due | |
| try { | |
| const interval = parser.parseExpression(action.schedule, { | |
| currentDate: now, | |
| }); | |
| const prev = interval.prev().toDate(); | |
| // If last_run is before the previous scheduled time, it's due | |
| if (!action.last_run || new Date(action.last_run) < prev) { | |
| return true; | |
| } | |
| // Fallback: use cron-parser to check if a run is due | |
| try { | |
| const interval = parser.parseExpression(action.schedule, { | |
| currentDate: now, | |
| }); | |
| const next = interval.next().toDate(); | |
| // If last_run is before the next scheduled time, it's due | |
| if (!action.last_run || new Date(action.last_run) < next) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In controllers/CronController.ts around lines 27 to 36, the use of
interval.prev() with a strict less-than comparison risks skipping executions
that are exactly on schedule. To fix this, change the call to
interval.prev(true) to make it inclusive, or alternatively compare last_run
against interval.next() instead. This ensures that jobs scheduled exactly at the
current time are not skipped.
1 participant
Summary by CodeRabbit
Refactor
Chores