we should not plan the same task twice #3244
Conversation
WalkthroughThis pull request introduces a new utility function, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
CREDO23
force-pushed
the
3117-bug-planned--task-was-planned-two-times
branch
from
e711e5a to
641f3a0
Compare
![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 and nitpick comments (5)
apps/web/app/utils/remove-duplicate-item.ts (2)
1-8: Improve TypeScript documentation.The JSDoc comments should be enhanced to better reflect the TypeScript types and business context.
/** - * A dynamic function that removes duplicates from an array of items. + * Removes duplicate items from an array by comparing their IDs. * - * @param {any[]} items - The items to check against. + * @template TItem - Type of items with an 'id' property + * @param {TItem[]} items - Array of items to deduplicate * - * @returns {any[]} - The array of items without duplicates. + * @returns {TItem[]} Array of items with duplicates removed */
9-22: Implementation looks good, but consider additional validations.The function efficiently removes duplicates using Set, but consider adding input validation for empty or invalid IDs.
export const removeDuplicateItems = <TItem extends { id: string }>(items?: TItem[]): TItem[] => { + if (!items?.length) return []; + const seenIds = new Set<string>(); - return items - ? items.filter((item) => { + return items.filter((item) => { + if (!item.id.trim()) { + throw new Error('Invalid item ID'); + } if (seenIds.has(item.id)) { return false; } else { seenIds.add(item.id); return true; } - }) - : []; + }); };apps/web/lib/features/daily-plan/create-daily-plan-form-modal.tsx (2)
253-253: Consider enhancing the UX by distinguishing between different types of disabled dates.While the implementation correctly prevents selecting dates where the task is already planned, it might be helpful to visually distinguish between dates disabled due to being in the past versus dates disabled due to existing task plans.
Consider adding a new modifier for task-specific disabled dates:
-disabled={[{ from: new Date(1970, 1, 1), to: yesterdayDate }, ...existingTaskPlanDates]} +disabled={[{ from: new Date(1970, 1, 1), to: yesterdayDate }]} +modifiers={{ + booked: existingPlanDates, + taskPlanned: existingTaskPlanDates +}} +modifiersClassNames={{ + booked: 'bg-primary text-white', + taskPlanned: 'bg-red-200 text-gray-500 cursor-not-allowed', + today: clsxm('border-2 !border-yellow-700 rounded') +}}
48-54: Architectural approach effectively solves the duplicate task planning issue.The solution elegantly prevents duplicate task planning through a combination of:
- Efficient filtering of existing plans
- Proper state management with useMemo
- Clean integration with the calendar component
The implementation is maintainable and follows React best practices.
Consider documenting this behavior in the component's documentation or README to make it clear that tasks cannot be planned multiple times for the same date.
Also applies to: 238-244, 253-253
apps/web/app/hooks/features/useDailyPlan.ts (1)
124-124: Add error handling for duplicate removal.While the state updates are consistent, consider adding try-catch blocks around the
removeDuplicateItemscalls to handle potential errors gracefully.Example implementation:
-return { ...res.data, tasks: removeDuplicateItems(res.data.tasks) }; +try { + return { ...res.data, tasks: removeDuplicateItems(res.data.tasks) }; +} catch (error) { + console.error('Error removing duplicate tasks:', error); + return res.data; // Fallback to original data +}Also applies to: 137-140, 144-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/app/hooks/features/useDailyPlan.ts(3 hunks)apps/web/app/utils/remove-duplicate-item.ts(1 hunks)apps/web/lib/features/daily-plan/create-daily-plan-form-modal.tsx(4 hunks)
🔇 Additional comments (7)
apps/web/app/utils/remove-duplicate-item.ts (1)
9-22: Consider architectural improvements for task date validation.
While this utility effectively removes duplicate tasks by ID, it doesn't address the core issue of preventing the same task from being planned on the same date. Consider extending the uniqueness check to include the date.
Let's verify how this utility is being used in the daily plan context:
apps/web/lib/features/daily-plan/create-daily-plan-form-modal.tsx (2)
238-244: LGTM! Props update is clean and well-typed.
The addition of existingTaskPlanDates to the CustomCalendar props maintains consistency with the existing implementation.
48-54: LGTM! Implementation effectively prevents duplicate task planning.
The existingTaskPlanDates implementation correctly identifies dates where a task is already planned, using efficient filtering and proper memoization.
Let's verify the task filtering logic:
✅ Verification successful
Let me gather more context about the task planning implementation to ensure consistency.
Let me try again with corrected commands:
Implementation is correct and consistent with the codebase
The verification confirms that:
- The task filtering logic in
existingTaskPlanDatesis consistent with other task-related filtering implementations across the codebase - The
CustomCalendarcomponent correctly receives and usesexistingTaskPlanDatesto prevent duplicate task planning - The implementation aligns with other task planning checks in the codebase, particularly in
add-task-estimation-hours-modal.tsxandplan-day-badge.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the task filtering logic is consistent across the codebase
# Look for other implementations that might conflict with this approach
# Search for similar task filtering patterns
rg -A 3 "plan.tasks?.some\(\(task\)"
Length of output: 517
Script:
#!/bin/bash
# Check for any other implementations related to task planning dates
rg -A 5 "task.*plan.*date" --type typescript
# Look for the CustomCalendar component usage
ast-grep --pattern 'CustomCalendar $$$'
# Check for other date-related filtering in daily plans
rg -A 5 "profileDailyPlans.*filter" --type typescript
Length of output: 214
Script:
#!/bin/bash
# Check for any other implementations related to task planning dates
rg -A 5 "task.*plan.*date"
# Look for the CustomCalendar component usage
rg -A 5 "<CustomCalendar"
# Check for other date-related filtering in daily plans
rg -A 5 "profileDailyPlans.*filter"
# Look for task filtering implementations
rg -A 5 "tasks\?\.some"
Length of output: 6789
apps/web/app/hooks/features/useDailyPlan.ts (4)
30-30: LGTM: Appropriate utility import for duplicate removal.
The import of removeDuplicateItems aligns with the PR's objective of preventing duplicate task planning.
124-124: LGTM: Comprehensive duplicate task removal implementation.
The code consistently applies removeDuplicateItems across all state updates:
- When updating existing plans
- When creating new plans
- When updating employee plans
This ensures no duplicate tasks can exist in any plan state.
Also applies to: 137-140, 144-147
124-124: Verify duplicate handling across time periods.
Please verify that duplicate task prevention works correctly in these scenarios:
- Moving tasks between plans (today → future, past → today)
- Adding tasks to outstanding plans
- Bulk operations (removeManyTaskPlans)
Let's check for potential task movement operations:
Also applies to: 137-140, 144-147
✅ Verification successful
Let me gather more information about the duplicate handling implementation.
Let me check the implementation of task plan updates and bulk operations to complete the verification.
Based on the gathered information, I can now provide the final response:
Duplicate handling is properly implemented across all operations
The codebase consistently handles duplicates through:
- A generic
removeDuplicateItemsutility that filters by task ID - This utility is applied to all task plan operations:
- After fetching plans
- When adding tasks to plans
- During bulk operations
- The implementation uses a Set for efficient duplicate detection
The duplicate prevention is correctly implemented at the data transformation layer, ensuring consistency across all time periods and operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for task movement operations
rg -g "*.ts" -g "*.js" "moveTask|transferTask|addTaskTo.*Plan"
Length of output: 2971
Script:
#!/bin/bash
# Search for duplicate handling functions and their implementations
rg -g "*.ts" -g "*.js" "removeDuplicate" -A 5
# Also check the interface definitions for task plans
ast-grep --pattern 'interface $_ {
$$$
tasks: $_
$$$
}'
Length of output: 3245
Script:
#!/bin/bash
# Check the implementation of task plan updates and bulk operations
rg -g "*.ts" "removeManyTaskFromPlans|updateDailyPlan" -B 2 -A 5
# Also check the task plan interfaces
ast-grep --pattern 'interface IDailyPlan {
$$$
}'
Length of output: 5180
124-124: Consider adding server-side validation.
While client-side duplicate removal is implemented, consider adding server-side validation in the API to ensure data integrity even if the client-side validation is bypassed.
Let's check if the API already has duplicate validation:
Also applies to: 137-140, 144-147
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/web/components/ui/dropdown-menu.tsx (1)
Line range hint
61-74: Avoid using type assertion toanySimilar to the DropdownMenuSubContent, using
{...(props as any)}here bypasses TypeScript's type checking.- {...(props as any)} + {...props}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/components/ui/dropdown-menu.tsx(2 hunks)
🔇 Additional comments (1)
apps/web/components/ui/dropdown-menu.tsx (1)
Line range hint 42-74: Verify relationship to PR objectives
The changes to these dropdown components appear unrelated to the PR's objective of preventing task duplication. While these components might be used in the task planning interface, the type changes don't directly contribute to preventing duplicate task planning.
Description
Fix #3117
Type of Change
Checklist
Previous screenshots
Loom
Current screenshots
loom
Summary by CodeRabbit
New Features
Create Daily Planmodal by preventing selection of already booked dates for specific tasks.Bug Fixes