fixbug: fix suggest daily plan modal #3148
Conversation
WalkthroughThe changes in this pull request introduce a new constant, Changes
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
fix-suggest-daily-plan-modal
branch
from
08e51ab
to
b1d695d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
apps/web/lib/features/daily-plan/suggest-daily-plan-modal.tsx (1)
29-32: LGTM with a minor suggestion: Improved localStorage managementThe changes to
handleCloseModalcorrectly implement the desired behavior of tracking when the user has seen the modal and conditionally setting theDAILY_PLAN_SUGGESTION_MODAL_DATEfor the profile route. This aligns well with the PR objectives.Consider using a more robust method to check if the current path is the profile route. For example:
if (path.startsWith('/profile')) { localStorage.setItem(DAILY_PLAN_SUGGESTION_MODAL_DATE, currentDate); }This approach is less prone to errors if the path structure changes in the future.
apps/web/app/constants.ts (1)
289-289: LGTM! Consider adding a comment for consistency.The addition of
HAS_SEEN_DAILY_PLAN_SUGGESTION_MODALis appropriate and aligns with the PR objectives. It follows the naming convention of other local storage keys in this file.For consistency with other constants in this section, consider adding a brief comment explaining its purpose:
+// Tracks whether the user has seen the daily plan suggestion modal export const HAS_SEEN_DAILY_PLAN_SUGGESTION_MODAL = 'has-seen-daily-plan-suggestion-modal';apps/web/lib/features/task/task-filters.tsx (1)
Line range hint
179-189: Approve changes with refactor suggestionThe new logic addresses the issue mentioned in the PR objectives and provides a more comprehensive check before setting the default tab. However, the nested conditions could be refactored for better readability and maintainability.
Consider refactoring the nested conditions using early returns or a more declarative approach. Here's a suggestion:
if (estimatedTotalTime(outstandingPlans).totalTasks) { setTab('dailyplan'); return; } if (getTotalTasks(todayPlan)) { return; } if (profile.tasksGrouped.assignedTasks.length) { setTab('assigned'); } else { setTab('unassigned'); }This refactored version reduces nesting and makes the logic flow more clear.
apps/web/lib/features/daily-plan/add-task-estimation-hours-modal.tsx (6)
Line range hint
38-464: Consider refactoring for improved maintainability and performanceThe
AddTasksEstimationHoursModalcomponent is quite large and complex. Consider the following suggestions:
Break down the component into smaller, more focused components. For example, you could create separate components for the modal header, task list, and action buttons.
Extract some of the complex logic into custom hooks. For instance, the task sorting and warning management could be moved to custom hooks.
Optimize the use of
useEffecthooks. Some effects might be combined or optimized to reduce unnecessary re-renders.Consider using
useMemooruseCallbackfor some of the complex calculations or callback functions to prevent unnecessary recalculations.Example of a custom hook for warning management:
function useWarningManagement(workTimePlanned, tasksEstimationTimes, plan) { const [warning, setWarning] = useState(''); useEffect(() => { if (plan?.tasks?.find((task) => !task.estimate)) { setWarning(t('dailyPlan.planned_tasks_popup.warning.TASKS_ESTIMATION')); } else if (!workTimePlanned || workTimePlanned <= 0) { setWarning(t('dailyPlan.planned_tasks_popup.warning.PLANNED_TIME')); } else if (Math.abs(workTimePlanned - tasksEstimationTimes) > 1) { checkPlannedAndEstimateTimeDiff(); } else { setWarning(''); } }, [workTimePlanned, tasksEstimationTimes, plan?.tasks]); return warning; }This refactoring would improve the overall structure and maintainability of the component.
🧰 Tools
🪛 Biome
[error] 461-463: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Line range hint
466-586: Improve input validation and error handling in SearchTaskInputThe
SearchTaskInputcomponent looks good overall, but there are a couple of areas for improvement:
Task creation validation: Currently, the create task button is only disabled if the task name is less than 5 characters. Consider adding more robust validation, such as trimming whitespace and checking for meaningful content.
Error handling: The
handleCreateTaskfunction doesn't have any error handling. Add proper error handling to provide feedback to the user if task creation fails.Here's an example of how you could improve these areas:
const handleCreateTask = useCallback(async () => { const trimmedTaskName = taskName.trim(); if (trimmedTaskName.length < 5) { // Show an error message to the user return; } try { setCreateTaskLoading(true); await createTask({ taskName: trimmedTaskName, status: taskStatus[0].name, taskStatusId: taskStatus[0].id, issueType: 'Bug' // TODO: Let the user choose the issue type }); // Show success message to the user } catch (error) { console.error('Failed to create task:', error); // Show error message to the user } finally { setCreateTaskLoading(false); } }, [createTask, taskName, taskStatus]);This improvement will enhance the user experience by providing better feedback and preventing the creation of invalid tasks.
🧰 Tools
🪛 Biome
[error] 461-463: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Line range hint
588-724: Refactor TaskCard for improved modularity and fix key propThe
TaskCardcomponent handles multiple responsibilities and could benefit from being split into smaller, more focused components. Additionally, there's a minor issue with thekeyprop in the mapped elements.
Consider breaking down the
TaskCardinto smaller components, such asTaskInfo,TaskActions, andTaskEstimate. This will improve readability and maintainability.In the rendered list items, use a unique identifier for the
keyprop instead of the index. Using index as keys can lead to unexpected behavior when the list order changes.Here's an example of how you could refactor part of the component:
const TaskInfo = ({ task, isTaskRenderedInTodayPlan, isDefaultTask, setDefaultTask }) => ( <div onClick={() => { if (isTaskRenderedInTodayPlan) { setDefaultTask(task); window && window.localStorage.setItem(DEFAULT_PLANNED_TASK_ID, task.id); } }} className="min-w-[48%] flex items-center h-full max-w-[50%]" > <TaskNameInfoDisplay task={task} /> </div> ); // In the TaskCard component return ( <Card // ... other props > <TaskInfo task={task} isTaskRenderedInTodayPlan={isTaskRenderedInTodayPlan} isDefaultTask={isDefaultTask} setDefaultTask={setDefaultTask} /> <VerticalSeparator /> {/* ... other parts of the card */} </Card> ); // When mapping tasks {tasks.map((task) => ( <li key={task.id}> {/* Use task.id instead of index */} <TaskCard // ... other props /> </li> ))}This refactoring will improve the overall structure of the component and fix the key prop issue.
🧰 Tools
🪛 Biome
[error] 461-463: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Line range hint
726-862: Improve TaskCardActions by extracting complex logicThe
TaskCardActionscomponent is well-structured, but the unplan logic is quite complex. Consider the following improvements:
Extract the unplan logic into a separate function or custom hook. This will improve readability and make the component easier to maintain.
Add comments to explain the complex unplan logic, especially the conditions for different unplan scenarios.
Here's an example of how you could refactor this:
const useUnplanLogic = (task, selectedPlan, activeTeamTask, timerStatus, isTodayPlan) => { const { user } = useAuthenticateUser(); const { removeTaskFromPlan } = useDailyPlan(); return useCallback(async (closePopover) => { try { // Comment explaining the logic for unplanning the active task if (task.id === activeTeamTask?.id) { if (timerStatus?.running && isTodayPlan) { return 'openUnplanActiveTaskModal'; } } // Comment explaining the general unplan logic if (selectedPlan?.id) { await removeTaskFromPlan( { taskId: task.id, employeeId: user?.employee.id }, selectedPlan.id ); } closePopover(); } catch (error) { console.error('Failed to unplan task:', error); // Show error message to the user } }, [task.id, activeTeamTask?.id, timerStatus?.running, isTodayPlan, selectedPlan?.id, user?.employee.id]); }; // In the TaskCardActions component const unplanLogic = useUnplanLogic(task, selectedPlan, activeTeamTask, timerStatus, isTodayPlan); // Use unplanLogic in your componentThis refactoring will make the component more readable and easier to maintain by isolating the complex unplan logic.
🧰 Tools
🪛 Biome
[error] 461-463: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Line range hint
864-1007: Enhance UnplanTask component with better error handling and extracted logicThe
UnplanTaskcomponent is well-structured, but there are opportunities for improvement:
Extract the complex unplan logic into a custom hook or separate functions. This will improve readability and maintainability.
Add more robust error handling and user feedback for unplan actions.
Consider adding comments to explain the different unplan scenarios.
Here's an example of how you could refactor this:
const useUnplanAllLogic = (taskId, activeTeamTask, timerStatus, isActiveTaskPlannedToday, planIds, user) => { const { removeManyTaskPlans } = useDailyPlan(); return useCallback(async (closePopover, closeActionPopover) => { try { // Comment explaining the logic for unplanning the active task if (taskId === activeTeamTask?.id && timerStatus?.running && isActiveTaskPlannedToday) { return 'openUnplanActiveTaskModal'; } // Comment explaining the general unplan all logic await removeManyTaskPlans({ plansIds: planIds, employeeId: user?.employee.id }, taskId); closePopover(); closeActionPopover(); // Show success message to the user } catch (error) { console.error('Failed to unplan all:', error); // Show error message to the user } }, [taskId, activeTeamTask?.id, timerStatus?.running, isActiveTaskPlannedToday, planIds, user?.employee.id]); }; // In the UnplanTask component const unplanAllLogic = useUnplanAllLogic(taskId, activeTeamTask, timerStatus, isActiveTaskPlannedToday, planIds, user); // Use unplanAllLogic in your component const handleUnplanAll = () => { const result = unplanAllLogic(close, closeActionPopover); if (result === 'openUnplanActiveTaskModal') { openUnplanActiveTaskModal(); } };This refactoring will improve the readability and maintainability of the component, while also providing better error handling and user feedback.
🧰 Tools
🪛 Biome
[error] 461-463: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
Line range hint
1-1007: Overall assessment: Good functionality with room for improvement in structure and error handlingThis file contains a comprehensive set of components for task planning and estimation. The overall functionality seems solid, and the use of React hooks and context is appropriate. However, there are several areas where the code could be improved:
Component Decomposition: Some components, particularly
AddTasksEstimationHoursModalandTaskCard, are quite large and complex. Breaking these down into smaller, more focused components would improve readability and maintainability.Custom Hooks: There are opportunities to extract complex logic into custom hooks, especially for state management and API interactions. This would make the components cleaner and more focused on rendering.
Error Handling: While there is some error handling, it could be more comprehensive and consistent across all API calls and user interactions.
Performance Optimization: Consider using
useMemoanduseCallbackmore extensively to prevent unnecessary re-renders and recalculations.Code Comments: Adding more explanatory comments for complex logic would improve code maintainability.
Despite these areas for improvement, the code demonstrates a good understanding of React patterns and provides a feature-rich interface for task planning. With some refactoring, this could become a very maintainable and efficient piece of the application.
🧰 Tools
🪛 Biome
[error] 461-463: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
apps/web/app/hooks/features/useStartStopTimerHandler.ts (2)
Line range hint
145-151: Handle potentialNaNvalues when calculating time differencesIn the
startTimerOrAskEstimatefunction, the expressionNumber(hasPlan?.workTimePlanned)could result inNaNifhasPlan?.workTimePlannedisundefined. This could causeMath.abs()to returnNaN, and the comparisonNaN > 1would always befalse. To ensure correct behavior, consider providing a default value or adding a check to handleundefinedvalues.Apply this diff to safeguard against
NaNvalues:const startTimerOrAskEstimate = () => { + const workTimePlanned = Number(hasPlan?.workTimePlanned) || 0; + const timeDifference = Math.abs(workTimePlanned - tasksEstimationTimes); if ( requirePlan && (!areAllTasksEstimated || !hasWorkedHours || - Math.abs(Number(hasPlan?.workTimePlanned) - tasksEstimationTimes) > 1) + timeDifference > 1) ) { openAddTasksEstimationHoursModal(); } else { startTimer(); } };
Line range hint
61-63: Clarify the purpose ofenforceTaskSoftCloseModalfunctionThe
enforceTaskSoftCloseModalfunction closes one modal and opens another (openAddTasksEstimationHoursModal()). The current name might not clearly convey this behavior, which could lead to confusion.Consider renaming the function to better represent its actions, such as
handleEnforceTaskSoftCloseAndOpenEstimateModal.
🛑 Comments failed to post (3)
apps/web/lib/features/task/task-filters.tsx (1)
178-178:
⚠️ Potential issueRemove or replace console.log statement
The console.log statement should be removed or replaced with proper logging before merging to production. If this log is necessary for debugging, consider using a more descriptive message and a proper logging mechanism.
Replace the console.log statement with a more informative logging mechanism or remove it entirely:
-console.log('first'); +// TODO: Add proper logging here if necessary📝 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.// TODO: Add proper logging here if necessaryapps/web/lib/features/user-profile-plans.tsx (1)
79-89:
⚠️ Potential issueApprove new state variable, but condition needs revision.
The addition of
haveSeenDailyPlanSuggestionModalis good for tracking the modal visibility state. However, there's an issue with the condition in the useEffect hook.The condition
haveSeenDailyPlanSuggestionModal == new Date().toISOString().split('T')[0]is comparing a boolean (or string 'true'/'false') with a date string, which will always evaluate to false. Consider revising this condition to correctly check if the user has seen the modal:- if (haveSeenDailyPlanSuggestionModal == new Date().toISOString().split('T')[0]) { + if (haveSeenDailyPlanSuggestionModal !== 'true') { window.localStorage.setItem(DAILY_PLAN_SUGGESTION_MODAL_DATE, new Date().toISOString().split('T')[0]); }📝 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.const haveSeenDailyPlanSuggestionModal = window?.localStorage.getItem(HAS_SEEN_DAILY_PLAN_SUGGESTION_MODAL); // Set the tab plan tab to outstanding if user has no daily plan and there are outstanding tasks (on first load) useEffect(() => { if (dailyPlanSuggestionModalDate != new Date().toISOString().split('T')[0] && path.split('/')[1] == 'profile') { if (estimatedTotalTime(outstandingPlans).totalTasks) { setCurrentTab('Outstanding'); } if (haveSeenDailyPlanSuggestionModal !== 'true') { window.localStorage.setItem(DAILY_PLAN_SUGGESTION_MODAL_DATE, new Date().toISOString().split('T')[0]); }apps/web/app/hooks/features/useStartStopTimerHandler.ts (1)
91-97: 🛠️ Refactor suggestion
Reduce repetitive calls to
startTimerOrAskEstimateSeveral functions (
handleCheckSelectedTaskOnTodayPlan,handleMissingDailyPlanWorkHour,handleMissingTasksEstimationHours) and conditional branches callstartTimerOrAskEstimate(). This repetition could be minimized by refactoring the code to streamline the logic and improve maintainability.Consider restructuring the conditional logic to reduce duplication. For example, you might combine similar branches or create a more unified flow that leads to
startTimerOrAskEstimate().Also applies to: 109-112, 127-137, 173-194
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/web/app/hooks/features/useStartStopTimerHandler.ts (3)
Line range hint
145-155: LGTM: NewstartTimerOrAskEstimatefunction improves code organizationThe introduction of the
startTimerOrAskEstimatefunction is a good refactoring decision. It consolidates the logic for starting the timer or asking for estimates, which was previously scattered across multiple functions. This improves code organization and reduces redundancy.Consider extracting the condition for opening the estimation modal into a separate function for better readability:
const shouldAskForEstimate = () => requirePlan && (!areAllTasksEstimated || !hasWorkedHours || Math.abs(Number(hasPlan?.workTimePlanned) - tasksEstimationTimes) > 1); const startTimerOrAskEstimate = () => { if (shouldAskForEstimate()) { openAddTasksEstimationHoursModal(); } else { startTimer(); } };This change would make the function even more readable and easier to maintain.
Line range hint
173-195: LGTM: Consistent refactoring with room for improvementThe changes in this segment maintain consistency with the overall refactoring approach. Multiple direct calls to
startTimerOrAskEstimatehave been added, simplifying the logic across various date conditions.While the current implementation works, the nested if-else structure could be simplified for better readability. Consider refactoring this section using early returns or a switch statement. For example:
if (dailyPlanSuggestionModalDate == currentDate && tasksEstimateHoursModalDate == currentDate && dailyPlanEstimateHoursModalDate == currentDate) { return startTimerOrAskEstimate(); } if (dailyPlanSuggestionModalDate != currentDate) { return hasPlan ? handleMissingTasksEstimationHours() : openSuggestDailyPlanModal(); } if (tasksEstimateHoursModalDate != currentDate) { return handleMissingTasksEstimationHours(); } if (dailyPlanEstimateHoursModalDate != currentDate) { if (hasPlan && areAllTasksEstimated) { return handleMissingDailyPlanWorkHour(); } } startTimerOrAskEstimate();This refactoring would reduce nesting and make the flow of logic easier to follow.
Line range hint
1-238: Overall: Good refactoring with room for further improvementsThe changes in this file represent a solid refactoring effort. The introduction of the
startTimerOrAskEstimatefunction and its consistent use throughout the file have improved code organization and reduced redundancy. The overall structure of the hook has been maintained, and the changes are focused on the timer start/stop logic.To further improve the code, consider the following suggestions:
Extract common patterns: There are several checks for date equality (e.g.,
dailyPlanSuggestionModalDate == currentDate). Consider creating a helper function for these checks to reduce repetition and improve readability.Use constants for magic numbers: The number 1 in the condition
Math.abs(Number(hasPlan?.workTimePlanned) - tasksEstimationTimes) > 1could be extracted into a named constant for better clarity.Consider using TypeScript's nullish coalescing operator (??) instead of logical OR (||) where appropriate, especially when dealing with potentially undefined values.
Review the necessity of all the dependencies in the useCallback hook. Ensure that all listed dependencies are actually used within the callback function.
These suggestions would further enhance the maintainability and readability of the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/web/app/constants.ts (1 hunks)
- apps/web/app/hooks/features/useStartStopTimerHandler.ts (5 hunks)
- apps/web/lib/features/daily-plan/add-task-estimation-hours-modal.tsx (1 hunks)
- apps/web/lib/features/daily-plan/suggest-daily-plan-modal.tsx (2 hunks)
- apps/web/lib/features/user-profile-plans.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/app/constants.ts
- apps/web/lib/features/daily-plan/add-task-estimation-hours-modal.tsx
- apps/web/lib/features/daily-plan/suggest-daily-plan-modal.tsx
- apps/web/lib/features/user-profile-plans.tsx
🧰 Additional context used
🔇 Additional comments (3)
apps/web/app/hooks/features/useStartStopTimerHandler.ts (3)
91-97: LGTM: Simplified logic inhandleCheckSelectedTaskOnTodayPlanThe changes in this segment improve the code by simplifying the logic. The direct call to
startTimerOrAskEstimateis consistent with the overall refactoring approach and makes the code more straightforward.
109-114: LGTM: Consistent refactoring inhandleMissingDailyPlanWorkHourThe changes in this segment maintain consistency with the overall refactoring approach. The direct call to
startTimerOrAskEstimatesimplifies the logic and improves code readability.
127-138: LGTM: Consistent refactoring inhandleMissingTasksEstimationHoursThe changes in this segment maintain consistency with the overall refactoring approach. Multiple direct calls to
startTimerOrAskEstimatehave been added, simplifying the logic and improving code readability across various conditions.
Description
This PR fixes the current issue in SOFT FLOW.
The issue was, if you visit the profile page without a plan, if you start the timer, you should first see a popup that suggests creating a plan for today. (This was not working due to wrong local storage variable management)
Type of Change
Checklist
Summary by CodeRabbit