fix: calculating next schedule should always just be from now #2102
Conversation
WalkthroughThe changes update the scheduling logic across several modules by replacing the use of the Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ 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: 0
🔭 Outside diff range comments (6)
apps/webapp/test/calculateNextSchedule.test.ts (6)
15-23:⚠️ Potential issueTest logic inconsistency: unused variable and misleading description.
This test defines
lastRunbut doesn't use it in the function call sincecalculateNextScheduledTimestampFromNowalways calculates from the current time. The test description mentions "recent timestamp" but the function behavior no longer depends on any previous timestamp.Apply this diff to remove the unused variable and update the test description:
- test("should calculate next run time for a recent timestamp", () => { + test("should calculate next run time from current time", () => { const schedule = "0 * * * *"; // Every hour - const lastRun = new Date("2024-01-01T11:00:00.000Z"); // 1.5 hours ago const nextRun = calculateNextScheduledTimestampFromNow(schedule, null);
25-34: 🛠️ Refactor suggestionClean up unused variable.
The
lastRunvariable is defined but never used since the new function doesn't accept a timestamp parameter.test("should handle timezone correctly", () => { const schedule = "0 * * * *"; // Every hour - const lastRun = new Date("2024-01-01T11:00:00.000Z"); const nextRun = calculateNextScheduledTimestampFromNow(schedule, "America/New_York");
36-52:⚠️ Potential issueTest no longer validates the claimed performance scenario.
This test claims to handle "very old timestamps" for performance, but since
calculateNextScheduledTimestampFromNowalways calculates from the current time, it's not actually testing performance with old timestamps. The test description and comments are misleading.The test should either be:
- Removed if the performance scenario is no longer relevant
- Updated to test a different performance scenario
- Renamed to accurately reflect what it's testing
- test("should efficiently handle very old timestamps (performance fix)", () => { + test("should efficiently calculate next schedule from current time", () => { const schedule = "*/1 * * * *"; // Every minute - const veryOldTimestamp = new Date("2020-01-01T00:00:00.000Z"); // 4 years ago const startTime = performance.now(); const nextRun = calculateNextScheduledTimestampFromNow(schedule, null); const duration = performance.now() - startTime; - // Should complete quickly (under 10ms) instead of iterating millions of times + // Should complete quickly (under 10ms) expect(duration).toBeLessThan(10);
54-62: 🛠️ Refactor suggestionUpdate test description and remove unused variable.
Similar issue: the test references behavior "when timestamp is within threshold" but no timestamp is being passed to the function.
- test("should still work correctly when timestamp is within threshold", () => { + test("should work correctly with 2-hour intervals from current time", () => { const schedule = "0 */2 * * *"; // Every 2 hours - const recentTimestamp = new Date("2024-01-01T10:00:00.000Z"); // 2.5 hours ago const nextRun = calculateNextScheduledTimestampFromNow(schedule, null); - // Should properly iterate: 10:00 -> 12:00 -> 14:00 (since current time is 12:30) + // Should return next 2-hour interval: 14:00 (since current time is 12:30) expect(nextRun).toEqual(new Date("2024-01-01T14:00:00.000Z")); });
64-77: 🛠️ Refactor suggestionMultiple tests have similar issues with unused variables and misleading descriptions.
These tests all define timestamp variables that are no longer used and have descriptions that reference behavior based on those timestamps. Since
calculateNextScheduledTimestampFromNowalways calculates from the current time, these tests need significant updates.Each of these tests should be reviewed to either:
- Remove unused timestamp variables
- Update test descriptions to accurately reflect what's being tested
- Update comments that reference the old behavior
- Consider if some tests are now redundant since they all calculate from the same "now" time
For example, lines 64-77 claim to test "frequent schedules with old timestamps efficiently" but don't actually pass any old timestamp to the function.
Also applies to: 79-90, 108-122, 124-138, 140-156, 158-170
173-448:⚠️ Potential issueFuzzy testing section has systematic issues with unused variables.
The fuzzy testing generates random timestamps via
generateRandomTimestamp()and assigns them to variables likelastTimestamp, but these are never used in the function calls. This means the fuzzy testing isn't actually testing the claimed scenarios.Key issues:
- Line 252:
lastTimestampis generated but never used- Line 265: Logic checks
lastTimestampbut the function doesn't use it- Line 292:
veryOldTimestampis generated but never used- Line 322:
lastTimestampis generated but never used- Line 377:
lastTimestampis generated but never used- Line 407:
lastTimestampis generated but never usedThe fuzzy testing needs to be completely rethought since the function behavior has fundamentally changed. Consider:
- Removing timestamp generation logic that's no longer relevant
- Updating invariant checks that reference old behavior
- Focusing fuzzy testing on aspects that are still relevant (schedule parsing, timezone handling, etc.)
🧹 Nitpick comments (1)
apps/webapp/app/v3/services/registerNextTaskScheduleInstance.server.ts (1)
32-34: Consider updating the span attribute for consistency.The span still sets a "last_scheduled_timestamp" attribute, but the actual calculation no longer uses this value since we're now calculating from the current time. Consider updating this attribute name to reflect the current behavior or removing it if it's no longer relevant for debugging/monitoring.
span.setAttribute( - "last_scheduled_timestamp", + "calculation_base_timestamp", instance.lastScheduledTimestamp?.toISOString() ?? new Date().toISOString() );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webapp/app/presenters/v3/ScheduleListPresenter.server.ts(2 hunks)apps/webapp/app/v3/services/registerNextTaskScheduleInstance.server.ts(2 hunks)apps/webapp/app/v3/services/upsertTaskSchedule.server.ts(2 hunks)apps/webapp/app/v3/utils/calculateNextSchedule.server.ts(1 hunks)apps/webapp/test/calculateNextSchedule.test.ts(21 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/webapp/app/v3/services/upsertTaskSchedule.server.ts (1)
apps/webapp/app/v3/utils/calculateNextSchedule.server.ts (1)
calculateNextScheduledTimestampFromNow(3-5)
apps/webapp/app/v3/services/registerNextTaskScheduleInstance.server.ts (1)
apps/webapp/app/v3/utils/calculateNextSchedule.server.ts (1)
calculateNextScheduledTimestampFromNow(3-5)
apps/webapp/test/calculateNextSchedule.test.ts (1)
apps/webapp/app/v3/utils/calculateNextSchedule.server.ts (1)
calculateNextScheduledTimestampFromNow(3-5)
apps/webapp/app/presenters/v3/ScheduleListPresenter.server.ts (1)
apps/webapp/app/v3/utils/calculateNextSchedule.server.ts (1)
calculateNextScheduledTimestampFromNow(3-5)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/webapp/app/v3/utils/calculateNextSchedule.server.ts (2)
3-5: LGTM! Well-designed abstraction.The new function provides a clean, purpose-built interface for calculating next scheduled timestamps from the current time, which aligns perfectly with the PR objective of making scheduling deterministic based on "now".
7-13: LGTM! Simplified and clearer logic.The function simplification removes the complex skipping logic and the parameter rename from
lastScheduledTimestamptocurrentDateimproves clarity. The default value ensures backward compatibility.apps/webapp/app/v3/services/upsertTaskSchedule.server.ts (2)
7-7: LGTM! Import updated correctly.The import change from
calculateNextScheduledTimestamptocalculateNextScheduledTimestampFromNowis consistent with the new API.
265-268: LGTM! Function usage updated appropriately.The function call now correctly uses
calculateNextScheduledTimestampFromNowwhich ensures thatnextRunis always calculated from the current time, aligning with the PR objective of deterministic scheduling based on "now".apps/webapp/app/presenters/v3/ScheduleListPresenter.server.ts (2)
8-8: LGTM! Import updated correctly.The import change is consistent with the new scheduling API that calculates timestamps from the current time.
260-263: LGTM! Function usage updated appropriately.The updated function call ensures that the
nextRuntimestamp in the schedule list is always calculated from the current time, providing users with accurate information about when each schedule will next execute.apps/webapp/app/v3/services/registerNextTaskScheduleInstance.server.ts (2)
2-2: LGTM! Import updated correctly.The import change is consistent with the new scheduling API that calculates timestamps from the current time.
36-39: LGTM! Function usage updated appropriately.The function call now correctly uses
calculateNextScheduledTimestampFromNow, ensuring that the next scheduled timestamp is always calculated from the current time rather than from a previous timestamp.apps/webapp/test/calculateNextSchedule.test.ts (2)
2-2: Import update looks good.The import statement has been correctly updated to use the new function.
4-4: Test suite name updated appropriately.The test suite description has been correctly updated to reflect the new function being tested.
As they always follow a deterministic step (instead of an interval from an non-deterministic starting point) and previous timestamps don't matter