perf fix: calculateNextScheduledTimestamp speed improvement #2099
Conversation
…millions of iterations on dev schedues
|
WalkthroughThe changes introduce a new attribute, "last_scheduled_timestamp," to the tracing span in 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: 1
🧹 Nitpick comments (3)
apps/webapp/test/calculateNextSchedule.test.ts (3)
44-45: Consider relaxing strict performance thresholds for CI compatibility.The 10ms performance expectation might be too strict for CI environments or slower machines, potentially causing flaky tests.
Consider increasing the threshold or making it configurable:
- expect(duration).toBeLessThan(10); + expect(duration).toBeLessThan(process.env.CI ? 50 : 10);
102-102: Performance threshold is very strict.The 5ms expectation is extremely strict and may cause test instability in different environments.
Consider a more lenient threshold:
- expect(duration).toBeLessThan(5); + expect(duration).toBeLessThan(20);
269-270: Performance threshold in fuzzy tests may be too lenient.The 100ms threshold for fuzzy tests seems quite generous compared to the strict thresholds in other tests. Consider consistency.
Consider aligning with other performance expectations:
- expect(duration).toBeLessThan(100); // Should complete within 100ms + expect(duration).toBeLessThan(50); // Should complete within 50ms
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/test/calculateNextSchedule.test.ts(1 hunks)
⏰ 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 / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- 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: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/webapp/test/calculateNextSchedule.test.ts (6)
1-13: Excellent test setup with deterministic time mocking.The setup properly uses Vitest's fake timers to ensure test determinism, which is crucial for time-based functionality testing.
184-216: Well-designed helper function for generating test cases.The random cron expression generator covers a good variety of scheduling patterns, from frequent to complex schedules.
247-283: Excellent invariant testing approach.The fuzzy testing validates key invariants that must hold regardless of inputs. The error logging for debugging failed cases is particularly valuable.
392-399: Good error handling for unsupported expressions.The try-catch properly handles cases where complex cron expressions might not be supported, allowing tests to continue gracefully.
422-447: Excellent boundary testing around optimization threshold.This test specifically validates the 10-step optimization threshold mentioned in the performance fix, ensuring the skip-ahead optimization kicks in at the right time.
304-331: Comprehensive DST edge case coverage.The DST testing covers multiple timezone transitions and validates graceful handling of time zone complexities.
No description provided.