Improve sheduling algorithm #3151
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. |
WalkthroughThis update introduces support for "locked events" and task splitting in the scheduling system. It modifies scheduling algorithms, UI components, and type definitions to handle locked events, allow toggling task splitting, and improve scheduling flexibility. The changes include new UI controls, state management, and enhanced scheduling test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SchedulerPage
participant LockedEventForm
participant ScheduleAlgorithm
User->>SchedulerPage: Click "Add Locked Event"
SchedulerPage->>LockedEventForm: Open modal
User->>LockedEventForm: Submit locked event details
LockedEventForm->>SchedulerPage: onAdd(newLockedEvent)
SchedulerPage->>SchedulerPage: Add to lockedEvents state
User->>SchedulerPage: Click "Schedule"
SchedulerPage->>ScheduleAlgorithm: scheduleTasks(tasks, activeHours, lockedEvents)
ScheduleAlgorithm->>SchedulerPage: Returns scheduled events (including locked)
SchedulerPage->>User: Display updated schedule
sequenceDiagram
participant User
participant TaskList
participant TaskModal
participant SchedulerPage
participant ScheduleAlgorithm
User->>TaskList: Toggle "Allow Split" switch
TaskList->>SchedulerPage: onUpdateTask (updates allowSplit)
User->>TaskModal: Toggle "Allow Split" in task creation
TaskModal->>SchedulerPage: onAddTaskAction (with allowSplit)
User->>SchedulerPage: Click "Schedule"
SchedulerPage->>ScheduleAlgorithm: scheduleTasks(tasks with allowSplit, ...)
ScheduleAlgorithm->>SchedulerPage: Returns scheduled events (split as needed)
SchedulerPage->>User: Display scheduled tasks (split or not)
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsxOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@typescript-eslint/parser' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ 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 @datnguyen-en, 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 introduces significant new features to the task scheduling application, allowing users greater control over how tasks are scheduled. It enables the definition of fixed time blocks that the scheduler must respect and provides granular control over whether individual tasks can be split into multiple sessions. The underlying scheduling algorithm has been updated to incorporate these new constraints, and the user interface has been enhanced to expose and manage these capabilities.
Highlights
- Enhanced Scheduling Algorithm: The core task scheduling algorithm has been significantly refactored to support new constraints, including respecting pre-defined 'locked events' and handling tasks that are explicitly marked as non-splittable.
- Task Splitting Control: Introduced a new
allowSplitproperty for tasks, enabling users to specify whether a task can be broken down into multiple sessions. This is exposed via new UI toggles in the task list and the task creation modal. - Locked Events Feature: Added the ability to define 'locked events' – fixed time blocks that the scheduling algorithm must avoid. Users can add and manage these events through a new dedicated section and modal in the scheduler interface.
- Improved Task Modal UI: The task creation/editing modal has been redesigned with a more organized two-column layout, enhancing user experience for defining task properties.
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. ↩
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3151 +/- ##
==========================================
+ Coverage 0.80% 0.87% +0.07%
==========================================
Files 2572 2572
Lines 328251 328578 +327
Branches 3074 3126 +52
==========================================
+ Hits 2626 2877 +251
- Misses 323430 323507 +77
+ Partials 2195 2194 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces significant new features related to task scheduling, specifically the ability to mark events as 'locked' and control whether tasks can be 'split' into multiple sessions. The core scheduling algorithm has been refactored to accommodate these features, and the UI has been updated accordingly. While the new functionalities are valuable additions, the pull request title 'Fix/fix ci/cd for algorithm branch' is misleading, as the changes are primarily feature development and UI refactoring rather than CI/CD fixes. This discrepancy in scope should be addressed. Overall, the implementation of the new features appears to be correct, but there are areas for improvement in terms of UI consistency, maintainability (magic numbers/strings), and code clarity in the algorithm.
apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx
Show resolved
Hide resolved
apps/calendar/src/app/[locale]/(root)/scheduler/components/ScheduleDisplay.tsx
Show resolved
Hide resolved
apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx
Show resolved
Hide resolved
![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
🔭 Outside diff range comments (1)
packages/ai/src/scheduling/templates.ts (1)
17-74: Consider adding allowSplit property to all template tasks for consistency.While the semantic choices for
allowSplitvalues are appropriate (false for "Basic Work Day", true for "Task Splitting Challenge"), several other template scenarios in this file are missing theallowSplitproperty entirely. This inconsistency could lead to undefined behavior.Consider adding explicit
allowSplitvalues to all tasks in remaining scenarios (Mixed Categories, Deadline Pressure, Overloaded Schedule, etc.) to ensure predictable behavior across all templates.For example, add to each task object:
+ allowSplit: true, // or false based on the scenario's intent
🧹 Nitpick comments (5)
apps/calendar/src/app/[locale]/(root)/scheduler/components/ScheduleDisplay.tsx (1)
229-233: Consider improving conditional styling priority for locked events.The current logic prioritizes locked events over past deadline styling, which might hide important deadline information. A locked event that's also past deadline should probably show both visual indicators.
Consider combining the styles or showing both states:
- event.locked - ? 'border-dynamic-blue/30 bg-dynamic-blue/10' - : event.isPastDeadline - ? 'border-destructive/30 bg-destructive/5' - : 'hover:bg-accent/5' + event.locked && event.isPastDeadline + ? 'border-destructive/50 bg-dynamic-blue/10 ring-1 ring-destructive/30' + : event.locked + ? 'border-dynamic-blue/30 bg-dynamic-blue/10' + : event.isPastDeadline + ? 'border-destructive/30 bg-destructive/5' + : 'hover:bg-accent/5'apps/calendar/src/app/[locale]/(root)/scheduler/page.tsx (1)
428-541: Consider adding validation for end time being after start time.The form validation checks for valid dates but doesn't explicitly validate that end time is after start time when they're on the same date.
Add this validation in the handleSubmit function:
if (!start.isValid() || !end.isValid() || end.isBefore(start)) { - setError('Invalid date or time'); + setError(end.isBefore(start) ? 'End time must be after start time' : 'Invalid date or time'); return; }apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx (1)
1-359: Consider adding unit tests for TaskModal component.Static analysis indicates this component lacks test coverage. Given the complex form validation and state management logic, adding tests would help prevent regressions.
Would you like me to help generate comprehensive unit tests for this component covering form validation, state updates, and user interactions?
packages/ai/src/scheduling/algorithm.ts (2)
106-163: Non-splittable task scheduling logic is comprehensive.The implementation correctly ensures non-splittable tasks are scheduled as single continuous blocks with appropriate retry logic and error handling.
Consider making the retry limit configurable:
+ const MAX_BLOCK_ATTEMPTS = 50; let blockAttempts = 0; - while (!scheduled && blockAttempts < 50) { + while (!scheduled && blockAttempts < MAX_BLOCK_ATTEMPTS) {
165-279: Splittable task logic handles edge cases well.The implementation comprehensively handles task splitting with respect to duration constraints, deadlines, and slot availability. The part numbering provides clear visibility of split tasks.
Consider extracting the complex part duration calculation logic (lines 203-240) into a separate helper function to improve readability and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/calendar/src/app/[locale]/(root)/scheduler/components/ScheduleDisplay.tsx(2 hunks)apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskList.tsx(3 hunks)apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx(8 hunks)apps/calendar/src/app/[locale]/(root)/scheduler/page.tsx(7 hunks)packages/ai/src/scheduling/algorithm.test.ts(4 hunks)packages/ai/src/scheduling/algorithm.ts(2 hunks)packages/ai/src/scheduling/default.ts(1 hunks)packages/ai/src/scheduling/templates.ts(6 hunks)packages/ai/src/scheduling/types.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
apps/calendar/src/app/[locale]/(root)/scheduler/components/ScheduleDisplay.tsx
[warning] 16-16: apps/calendar/src/app/[locale]/(root)/scheduler/components/ScheduleDisplay.tsx#L16
Added line #L16 was not covered by tests
[warning] 229-233: apps/calendar/src/app/[locale]/(root)/scheduler/components/ScheduleDisplay.tsx#L229-L233
Added lines #L229 - L233 were not covered by tests
[warning] 240-251: apps/calendar/src/app/[locale]/(root)/scheduler/components/ScheduleDisplay.tsx#L240-L251
Added lines #L240 - L251 were not covered by tests
apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskList.tsx
[warning] 21-21: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskList.tsx#L21
Added line #L21 was not covered by tests
[warning] 32-32: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskList.tsx#L32
Added line #L32 was not covered by tests
[warning] 250-272: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskList.tsx#L250-L272
Added lines #L250 - L272 were not covered by tests
apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx
[warning] 14-14: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L14
Added line #L14 was not covered by tests
[warning] 25-25: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L25
Added line #L25 was not covered by tests
[warning] 32-33: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 57-61: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L57-L61
Added lines #L57 - L61 were not covered by tests
[warning] 71-71: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L71
Added line #L71 was not covered by tests
[warning] 125-125: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L125
Added line #L125 was not covered by tests
[warning] 128-128: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L128
Added line #L128 was not covered by tests
[warning] 142-142: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L142
Added line #L142 was not covered by tests
[warning] 145-145: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L145
Added line #L145 was not covered by tests
[warning] 148-148: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L148
Added line #L148 was not covered by tests
[warning] 157-157: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L157
Added line #L157 was not covered by tests
[warning] 168-172: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L168-L172
Added lines #L168 - L172 were not covered by tests
[warning] 174-175: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L174-L175
Added lines #L174 - L175 were not covered by tests
[warning] 178-182: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L178-L182
Added lines #L178 - L182 were not covered by tests
[warning] 184-185: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L184-L185
Added lines #L184 - L185 were not covered by tests
[warning] 189-193: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L189-L193
Added lines #L189 - L193 were not covered by tests
[warning] 195-195: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L195
Added line #L195 was not covered by tests
[warning] 197-197: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L197
Added line #L197 was not covered by tests
[warning] 201-208: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L201-L208
Added lines #L201 - L208 were not covered by tests
[warning] 210-244: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L210-L244
Added lines #L210 - L244 were not covered by tests
[warning] 246-247: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L246-L247
Added lines #L246 - L247 were not covered by tests
[warning] 252-253: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L252-L253
Added lines #L252 - L253 were not covered by tests
[warning] 255-256: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L255-L256
Added lines #L255 - L256 were not covered by tests
[warning] 258-337: apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx#L258-L337
Added lines #L258 - L337 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
- GitHub Check: Verify generated types
🔇 Additional comments (19)
packages/ai/src/scheduling/default.ts (1)
34-34: LGTM! Consistent addition of allowSplit property.The addition of
allowSplit: trueto the default task aligns with the new Task interface and enables task splitting functionality by default, which is a reasonable choice.packages/ai/src/scheduling/types.ts (2)
16-17: LGTM! Well-designed type extensions for new features.The addition of optional
lockedandcategoryproperties to the Event interface provides the necessary type support for locked events while maintaining backward compatibility.
29-29: LGTM! Consistent type addition for task splitting.The optional
allowSplitproperty on the Task interface properly supports the task splitting feature while maintaining backward compatibility.apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskList.tsx (2)
21-21: LGTM! Proper imports added for new functionality.The imports for
SplitIconandSwitchcomponents are correctly added to support the new task splitting toggle feature.Also applies to: 32-32
250-272: ```shell
#!/bin/bashLocate TaskList component file
COMP_PATH=$(fd TaskList.tsx | head -n1)
echo "Component Path: $COMP_PATH"
DIR=$(dirname "$COMP_PATH")
echo "Component Directory: $DIR"List files in component directory
echo "Files in component directory:"
ls "$DIR"Search for any test files in the same directory
echo "Looking for tests in $DIR:"
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts "$DIR"Search globally for test files referencing TaskList
echo "Global search for TaskList in test files:"
rg -l "TaskList" -g ".test.tsx" -g ".test.ts" -g ".spec.tsx" -g ".spec.ts"</details> <details> <summary>apps/calendar/src/app/[locale]/(root)/scheduler/components/ScheduleDisplay.tsx (2)</summary> `16-16`: **LGTM! Proper import added for locked event visualization.** The `LockIcon` import is correctly added to support the new locked events display functionality. --- `240-251`: **Well-implemented locked event indicator with clear UX.** The lock icon and tooltip provide clear visual feedback for locked events. The positioning before the deadline warning maintains good information hierarchy. Static analysis indicates this new functionality lacks test coverage. Consider adding tests for locked event display behavior. </details> <details> <summary>packages/ai/src/scheduling/algorithm.test.ts (3)</summary> `1-1`: **LGTM! Import statement correctly added.** The import of `scheduleTasks` function is appropriate for the new test cases. --- `180-197`: **Well-structured test for non-splittable task scheduling.** The test correctly validates that a non-splittable task is scheduled as a single continuous block when possible, with appropriate assertions for event count, name, and absence of part numbering. --- `217-233`: **Good test coverage for task splitting functionality.** The test appropriately validates that splittable tasks are divided into multiple parts when needed, with correct part numbering in event names. </details> <details> <summary>apps/calendar/src/app/[locale]/(root)/scheduler/page.tsx (4)</summary> `21-60`: **Imports and state management correctly implemented.** The new imports and state variables for locked events functionality are properly structured and follow React best practices. --- `104-118`: **Locked event management functions are well-implemented.** The functions correctly handle adding and removing locked events with proper type safety and immutable state updates. --- `120-132`: **Scheduling function correctly updated to include locked events.** The `handleSchedule` function properly passes locked events to the scheduling algorithm. --- `212-277`: **Locked events UI section is well-designed and functional.** The UI provides clear visual distinction for locked events with appropriate icons, displays essential information, and includes intuitive controls for adding and removing events. </details> <details> <summary>apps/calendar/src/app/[locale]/(root)/scheduler/components/TaskModal.tsx (2)</summary> `30-34`: **Props renamed consistently for better clarity.** The renaming from `onClose`/`onAddTask` to `onCloseAction`/`onAddTaskAction` better indicates these are action handlers and is applied consistently throughout the component. Also applies to: 57-61 --- `71-71`: **Task splitting toggle correctly implemented.** The `allowSplit` field is properly integrated with appropriate default value, form state management, and intuitive UI controls. Also applies to: 125-125, 142-142, 327-342 </details> <details> <summary>packages/ai/src/scheduling/algorithm.ts (3)</summary> `47-68`: **Function signature and initialization properly updated.** The addition of `lockedEvents` parameter and the task pool initialization with normalized durations and tracking fields are well-implemented. --- `87-92`: **Iterative scheduling approach with proper safeguards.** The implementation includes appropriate safeguards against infinite loops with a maximum attempt limit and early exit when no progress is made. Also applies to: 282-284 --- `286-300`: **Comprehensive logging for scheduling outcomes.** The final logging section provides valuable insights into task splitting and any unscheduled portions, helping users understand the scheduling results. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
vhpx
changed the title
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Documentation