refactor: the timer should not start when the user ignores the 'Todayplan' popup #3159
Conversation
WalkthroughThe pull request introduces enhancements to several modal components and related hooks, focusing on loading states and improved error handling. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/web/lib/features/daily-plan/add-daily-plan-work-hours-modal.tsx (4 hunks)
- apps/web/lib/features/daily-plan/add-task-estimation-hours-modal.tsx (3 hunks)
🧰 Additional context used
🪛 Biome
apps/web/lib/features/daily-plan/add-task-estimation-hours-modal.tsx
[error] 504-504: 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)
🔇 Additional comments (2)
apps/web/lib/features/daily-plan/add-daily-plan-work-hours-modal.tsx (2)
26-26: Initialization of the loading state is appropriateThe introduction of the
loadingstate usinguseState(false)correctly sets up the component to manage loading status during asynchronous operations.
94-98: Button content correctly displays loading stateThe conditional rendering of the
SpinnerLoaderwhenloadingistrueprovides visual feedback to the user, indicating that an operation is in progress.
| // Update the plan work time only if the user changed it | ||
| plan && | ||
| plan.workTimePlanned !== workTimePlanned && | ||
| (await updateDailyPlan({ workTimePlanned }, plan.id ?? '')); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor conditional update for better readability
Using short-circuit evaluation with && and await can reduce code readability and may lead to unintended behavior. Consider refactoring this logic into an explicit if statement for clarity and maintainability.
Refactored code:
- plan &&
- plan.workTimePlanned !== workTimePlanned &&
- (await updateDailyPlan({ workTimePlanned }, plan.id ?? ''));
+ if (plan && plan.workTimePlanned !== workTimePlanned) {
+ await updateDailyPlan({ workTimePlanned }, plan.id ?? '');
+ }This change makes the asynchronous operation more explicit and the code easier to understand.
📝 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.
| // Update the plan work time only if the user changed it | |
| plan && | |
| plan.workTimePlanned !== workTimePlanned && | |
| (await updateDailyPlan({ workTimePlanned }, plan.id ?? '')); | |
| // Update the plan work time only if the user changed it | |
| if (plan && plan.workTimePlanned !== workTimePlanned) { | |
| await updateDailyPlan({ workTimePlanned }, plan.id ?? ''); | |
| } |
There was a problem hiding this comment.
Can you use this suggestion, i think it better readable
@CREDO23
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -489,7 +501,7 @@ export function AddTasksEstimationHoursModal(props: IAddTasksEstimationHoursModa | |||
| return ( | |||
| <> | |||
| {isRenderedInSoftFlow ? ( | |||
| <Modal isOpen={isOpen} closeModal={closeModalAndStartTimer} showCloseIcon={requirePlan ? false : true}> | |||
| <Modal isOpen={isOpen} closeModal={closeModalAndSubmit} showCloseIcon={requirePlan ? false : true}> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the conditional expression in showCloseIcon prop
The ternary operator requirePlan ? false : true can be simplified by negating the condition, which makes the code more concise and easier to read.
Update the code as follows:
- <Modal isOpen={isOpen} closeModal={closeModalAndSubmit} showCloseIcon={requirePlan ? false : true}>
+ <Modal isOpen={isOpen} closeModal={closeModalAndSubmit} showCloseIcon={!requirePlan}>📝 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.
| <Modal isOpen={isOpen} closeModal={closeModalAndSubmit} showCloseIcon={requirePlan ? false : true}> | |
| <Modal isOpen={isOpen} closeModal={closeModalAndSubmit} showCloseIcon={!requirePlan}> |
🧰 Tools
🪛 Biome
[error] 504-504: 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)
There was a problem hiding this comment.
Check also this Suggestion
@CREDO23
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/web/lib/features/daily-plan/add-daily-plan-work-hours-modal.tsx (3 hunks)
🧰 Additional context used
🪛 Biome
apps/web/lib/features/daily-plan/add-daily-plan-work-hours-modal.tsx
[error] 90-90: 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)
🔇 Additional comments (6)
apps/web/lib/features/daily-plan/add-daily-plan-work-hours-modal.tsx (6)
1-1: LGTM: SpinnerLoader import addedThe addition of the
SpinnerLoaderimport is appropriate for implementing the loading state in the component. This change aligns well with the PR objectives to enhance the user experience during time tracking operations.
26-31: LGTM: Loading state added and handleCloseModal optimizedThe addition of the
loadingstate variable is a good implementation for managing loading states during submission, which aligns with the PR objectives. The use ofuseCallbackforhandleCloseModalis a nice performance optimization. These changes contribute to a better user experience and code performance.
82-82: LGTM: Consistent button sizingThe addition of
min-w-[10rem]to the "Skip" button's className ensures consistent button sizing. This is a good UI practice and contributes to a more polished look for the modal.
94-98: LGTM: Loading indicator added to submit buttonThe addition of a conditional render for the SpinnerLoader during the loading state is an excellent improvement. This provides clear visual feedback to the user when the submission is in progress, enhancing the overall user experience. This change aligns well with the PR objectives to improve the time tracking functionality.
Line range hint
1-106: Overall assessment: Good improvements with minor suggestionsThe changes in this file effectively address the issue of premature time tracking and enhance the user experience with loading states and improved error handling. The implementation aligns well with the PR objectives.
Key improvements:
- Addition of loading state and visual feedback.
- Prevention of multiple submissions during loading.
- Proper sequencing of operations in the submit handler.
Suggestions for further improvement:
- Enhance error handling to provide user feedback.
- Simplify the disabled state logic for the submit button.
These changes contribute significantly to resolving the issue described in #3156. Great work overall!
🧰 Tools
🪛 Biome
[error] 53-53: 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)
33-50:⚠️ Potential issueImprove error handling in handleSubmit
The implementation of try-catch-finally blocks for error handling is a good addition. However, the current error handling only logs the error to the console, which doesn't provide feedback to the user. Consider enhancing this to provide a better user experience.
Suggestion for improvement:
} catch (error) { - console.log(error); + // Display an error message to the user + showErrorNotification('An error occurred while starting the timer. Please try again.'); } finally {Ensure that
showErrorNotificationis a method that displays error messages to the user, using your application's notification system. This will inform users when an error occurs during the submission process.Likely invalid or redundant comment.
| disabled={loading || (requirePlan ? (hasWorkHours ? false : true) : false)} | ||
| className="py-3 px-5 min-w-[10rem] rounded-md font-light text-md dark:text-white" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify disabled state logic
The addition of the loading condition to the disabled state is a good improvement to prevent multiple submissions. The consistent sizing with the "Skip" button also enhances the UI. However, the disabled state logic can be simplified.
Consider simplifying the disabled state logic:
- disabled={loading || (requirePlan ? (hasWorkHours ? false : true) : false)}
+ disabled={loading || (requirePlan && !hasWorkHours)}This simplification maintains the same logic but is more readable and concise.
📝 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.
| disabled={loading || (requirePlan ? (hasWorkHours ? false : true) : false)} | |
| className="py-3 px-5 min-w-[10rem] rounded-md font-light text-md dark:text-white" | |
| disabled={loading || (requirePlan && !hasWorkHours)} | |
| className="py-3 px-5 min-w-[10rem] rounded-md font-light text-md dark:text-white" |
🧰 Tools
🪛 Biome
[error] 90-90: 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)
There was a problem hiding this comment.
Can you use this Condition Suggestion disabled={loading || (requirePlan && !hasWorkHours)}
@CREDO23
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| // Update the plan work time only if the user changed it | ||
| plan && | ||
| plan.workTimePlanned !== workTimePlanned && | ||
| (await updateDailyPlan({ workTimePlanned }, plan.id ?? '')); | ||
|
|
There was a problem hiding this comment.
Can you use this suggestion, i think it better readable
@CREDO23
| disabled={loading || (requirePlan ? (hasWorkHours ? false : true) : false)} | ||
| className="py-3 px-5 min-w-[10rem] rounded-md font-light text-md dark:text-white" |
There was a problem hiding this comment.
Can you use this Condition Suggestion disabled={loading || (requirePlan && !hasWorkHours)}
@CREDO23
| @@ -489,7 +501,7 @@ export function AddTasksEstimationHoursModal(props: IAddTasksEstimationHoursModa | |||
| return ( | |||
| <> | |||
| {isRenderedInSoftFlow ? ( | |||
| <Modal isOpen={isOpen} closeModal={closeModalAndStartTimer} showCloseIcon={requirePlan ? false : true}> | |||
| <Modal isOpen={isOpen} closeModal={closeModalAndSubmit} showCloseIcon={requirePlan ? false : true}> | |||
There was a problem hiding this comment.
Check also this Suggestion
@CREDO23
…s-how-it-should-work 3155 bug settings enforce plans how it should work
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (16)
apps/web/lib/components/modal.tsx (1)
57-82: LGTM: Improved modal content structureThe addition of the
Dialog.Panelcomponent improves the structure and semantics of the modal content. This change enhances the organization of the modal's layout without affecting its existing functionality.For consistency, consider moving the
classNameprop application from theDialog.Overlayto theDialog.Panel:-<Dialog.Overlay - className={clsxm('flex justify-center items-center flex-col space-y-1 relative', className)} -> +<Dialog.Overlay> <Dialog.Panel - className={clsxm('flex justify-center items-center flex-col space-y-1 relative', className)} + className={clsxm('flex justify-center items-center flex-col space-y-1 relative', className)} > {/* ... */} </Dialog.Panel> </Dialog.Overlay>This change would make it clearer that the className is applied to the panel itself rather than the overlay.
apps/web/lib/components/switch.tsx (1)
133-135: LGTM: Added logic to clear localStorage when toggle is turned off.The new code aligns well with the PR objectives. It ensures that the daily plan suggestion state is reset when the "Require Daily Plan to Track" feature is disabled. This change should help prevent unexpected behavior related to the "Todayplan" popup.
A minor suggestion for improved clarity:
Consider adding a comment explaining the purpose of this localStorage removal:
if (!enabled) { + // Clear the daily plan suggestion state when the feature is disabled localStorage.removeItem(DAILY_PLAN_SUGGESTION_MODAL_DATE); }apps/web/app/hooks/features/useStartStopTimerHandler.ts (1)
Line range hint
1-238: Overall impact: Improved timer control logic with minimal changesThe change in this file successfully refines the timer control logic by adding a crucial condition check. This modification aligns well with the PR objectives and improves the accuracy of when the enforce planned task modal is opened.
The localized nature of the change minimizes the risk of unintended side effects while effectively addressing the issue at hand. The existing structure and patterns of the hook are maintained, ensuring consistency and readability.
To further enhance code readability, consider adding a brief comment explaining the purpose of this condition:
// Only open the enforce planned task modal if a plan exists and the active task isn't planned } else if (requirePlan && hasPlan && !isActiveTaskPlaned) {This comment would provide quick context for future developers working on this complex logic.
apps/web/locales/he.json (2)
220-221: LGTM! Consider adding a comment for context.The addition of the "DELETE_THIS_PLAN" key with its Hebrew translation looks good. This suggests a new feature for deleting plans has been implemented in the application.
Consider adding a comment above this new entry to provide context for translators or other developers. For example:
"plan": { "ADD_PLAN": "הוסף תוכנית", + // New feature: Ability to delete a plan "DELETE_THIS_PLAN": "מחק את התוכנית הזו" },
Line range hint
1-1037: Consider adding section comments for improved readability.While the changes made are correct and the file is well-structured, we can further improve its readability and maintainability.
Consider adding section comments throughout the file to clearly delineate different parts of the application. This will help translators and developers quickly locate and understand the context of each group of translations. For example:
{ + // General application title "TITLE": "Ever צוותים", + // Common terms used throughout the application "common": { "ACCEPT": "קבל", // ... other common terms ... }, + // Authentication and user management related translations "auth": { "SEND_CODE": "שלח קוד", // ... other auth related translations ... }, // ... continue for other major sections ... }This improvement will make the file more organized and easier to navigate, especially as it grows with more translations in the future.
apps/web/locales/en.json (1)
220-221: LGTM! Consider adding a confirmation message.The addition of the "DELETE_THIS_PLAN" key is consistent with the existing structure and provides a clear label for a plan deletion feature.
Consider also adding a confirmation message for plan deletion to prevent accidental deletions. For example:
"plan": { "ADD_PLAN": "Add Plan", - "DELETE_THIS_PLAN": "Delete this plan" + "DELETE_THIS_PLAN": "Delete this plan", + "CONFIRM_DELETE_PLAN": "Are you sure you want to delete this plan?" },apps/web/locales/bg.json (1)
220-221: LGTM! Consider adding a comment for clarity.The addition of the "DELETE_THIS_PLAN" key with its Bulgarian translation looks good and is consistent with the rest of the file.
For improved maintainability, consider adding a comment above this key-value pair to describe its purpose, similar to other sections in the file. For example:
"plan": { "SINGULAR": "План", "PLURAL": "Планове", "CHOOSE_DATE": "Изберете дата, за да видите план", "PLAN_NOT_FOUND": "Не е намерен план", "FOR_DATE": "ПЛАН ЗА {date}", "FOR_TODAY": "Днешният план", "FOR_TOMORROW": "Утрешният план", "EDIT_PLAN": "Редактиране на план", "TRACKED_TIME": "Проследено време", "SEE_PLANS": "Виж планове", "ADD_PLAN": "Добави план", + // Delete the current plan "DELETE_THIS_PLAN": "Изтрийте този план" },apps/web/locales/it.json (1)
224-225: LGTM! Consider adding a confirmation message.The new translation for "DELETE_THIS_PLAN" has been correctly added and is consistent with the rest of the file. The Italian translation "Elimina questo piano" is accurate for "Delete this plan".
As a minor suggestion, consider adding a confirmation message for the delete action to prevent accidental deletions. For example:
"plan": { ... "ADD_PLAN": "Aggiungi Piano", "DELETE_THIS_PLAN": "Elimina questo piano" + "CONFIRM_DELETE_PLAN": "Sei sicuro di voler eliminare questo piano?" },This addition would enhance the user experience by adding an extra layer of confirmation before deleting a plan.
apps/web/locales/pt.json (2)
224-225: LGTM! Consider adding a period for consistency.The addition of the "DELETE_THIS_PLAN" key with its Portuguese translation "Excluir este plano" is correct and well-placed within the
planobject.For consistency with other similar entries in the file, consider adding a period at the end of the translation:
- "DELETE_THIS_PLAN": "Excluir este plano" + "DELETE_THIS_PLAN": "Excluir este plano."
Line range hint
216-225: Consider alphabetizing keys in theplanobject for better maintainability.While the overall structure of the file is well-organized, the
planobject could benefit from having its keys arranged in alphabetical order. This would make it easier to locate specific translations and maintain consistency as new entries are added.Here's a suggested reordering of the
planobject keys:"plan": { "ADD_PLAN": "Adicionar Plano", "CHOOSE_DATE": "Selecione uma data para poder ver um plano", "DELETE_THIS_PLAN": "Excluir este plano", "EDIT_PLAN": "Editar Plano", "FOR_DATE": "PLANO PARA {date}", "FOR_TODAY": "Plano de hoje", "FOR_TOMORROW": "Plano de amanhã", "PLAN_NOT_FOUND": "Plano não encontrado", "PLURAL": "Planos", "SEE_PLANS": "Ver planos", "SINGULAR": "Plano", "TRACKED_TIME": "Tempo rastreado" }apps/web/locales/es.json (1)
Line range hint
1-1037: Consider splitting translations into smaller files for better maintainability.While the changes made are correct, I noticed that this localization file is quite large. As the application continues to grow, managing all translations in a single file might become challenging.
Consider splitting the translations into smaller, more manageable files based on features or sections of the application. This approach could improve maintainability and make it easier for translators to work on specific parts of the application. For example:
locales/ es/ common.json auth.json dashboard.json settings.json ...This structure would allow for easier navigation and updating of specific sections without the need to search through a large file.
apps/web/locales/de.json (1)
220-221: LGTM! Consider adding a period for consistency.The new translation for deleting a plan is appropriate and well-placed within the "plan" object. The German translation is correct and natural.
For consistency with other entries in this section (e.g., "ADD_PLAN": "Plan hinzufügen"), consider adding a period at the end of the phrase:
- "DELETE_THIS_PLAN": "Diesen Plan löschen" + "DELETE_THIS_PLAN": "Diesen Plan löschen."This is a minor suggestion to maintain consistency in punctuation throughout the localization file.
apps/web/lib/features/daily-plan/add-task-estimation-hours-modal.tsx (2)
110-127: Improved modal closing logicThe new
closeModalAndSubmitfunction addresses the issue mentioned in the PR objectives. It ensures that the timer doesn't start when the user ignores the 'Todayplan' popup, aligning with the expected behavior described in the linked issue.A few suggestions for improvement:
- Consider adding a more descriptive error message in the catch block.
- You might want to add a success notification when the plan is updated successfully.
Consider updating the error handling:
} catch (error) { - console.log(error); + console.error('Failed to update daily plan:', error); + // Consider adding a user-facing error notification here } finally { setLoading(false); }
Line range hint
1-924: Suggestions for further improvementsWhile the main issue has been addressed effectively, consider the following suggestions to further enhance the code:
Improve error handling throughout the component, not just in the
closeModalAndSubmitfunction. Consider using a consistent error handling approach across all async functions.Add more user feedback for actions. For example, show success notifications when a plan is updated or a task is added/removed.
Consider extracting some of the larger functions (like
handleSubmit) into custom hooks or separate utility functions to improve readability and maintainability.Review the use of
anytypes (if any) and replace them with more specific types to improve type safety.These suggestions are not critical for the current PR but could be considered for future improvements to enhance the overall quality of the component.
apps/web/lib/features/daily-plan/suggest-daily-plan-modal.tsx (1)
24-24: Remove unnecessary use ofuseMemoforrequirePlanSince
requirePlanis directly accessing a property without any expensive computations, wrapping it withuseMemois unnecessary. Assigning it directly improves readability.Consider changing to:
- const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]); + const requirePlan = activeTeam?.requirePlanToTrack;apps/web/lib/features/user-profile-plans.tsx (1)
366-369: Use consistent references tolocalStorageIn line 366,
localStorage.removeItem(...)is used, whereas in other parts of the code (e.g., lines 97-99),window.localStorageis used. For consistency, consider usingwindow.localStoragethroughout.Apply this diff:
if (requirePlan) { - localStorage.removeItem( + window.localStorage.removeItem( DAILY_PLAN_SUGGESTION_MODAL_DATE ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
- apps/web/app/hooks/features/useStartStopTimerHandler.ts (1 hunks)
- apps/web/app/hooks/features/useTimer.ts (1 hunks)
- apps/web/lib/components/modal.tsx (3 hunks)
- apps/web/lib/components/switch.tsx (2 hunks)
- apps/web/lib/features/daily-plan/add-daily-plan-work-hours-modal.tsx (3 hunks)
- apps/web/lib/features/daily-plan/add-task-estimation-hours-modal.tsx (4 hunks)
- apps/web/lib/features/daily-plan/suggest-daily-plan-modal.tsx (2 hunks)
- apps/web/lib/features/user-profile-plans.tsx (5 hunks)
- apps/web/locales/ar.json (1 hunks)
- apps/web/locales/bg.json (1 hunks)
- apps/web/locales/de.json (1 hunks)
- apps/web/locales/en.json (1 hunks)
- apps/web/locales/es.json (1 hunks)
- apps/web/locales/fr.json (1 hunks)
- apps/web/locales/he.json (1 hunks)
- apps/web/locales/it.json (1 hunks)
- apps/web/locales/nl.json (1 hunks)
- apps/web/locales/pl.json (1 hunks)
- apps/web/locales/pt.json (1 hunks)
- apps/web/locales/ru.json (1 hunks)
- apps/web/locales/zh.json (1 hunks)
🧰 Additional context used
🪛 Biome
apps/web/lib/features/daily-plan/add-daily-plan-work-hours-modal.tsx
[error] 90-90: 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)
🔇 Additional comments (26)
apps/web/lib/components/modal.tsx (3)
18-18: LGTM: New prop enhances modal flexibilityThe addition of the
closeOnOutsideClickprop is a good improvement. It provides more control over the modal's behavior, allowing developers to decide whether clicking outside the modal should close it or not.
49-49: LGTM: Conditional close behavior implemented correctlyThe modification to the
onCloseprop of the Dialog component correctly implements the desired behavior based on thecloseOnOutsideClickprop. This change aligns with the PR objectives, ensuring that the modal only closes on outside clicks when explicitly configured to do so.
Line range hint
1-89: Summary: Excellent improvements to the Modal componentOverall, the changes to the Modal component are well-implemented and align with the PR objectives. The new
closeOnOutsideClickfunctionality provides better control over the modal's behavior, and the restructuring of the component enhances its organization and semantics.Key points:
- The new
closeOnOutsideClickprop is correctly implemented and defaults tofalsefor backward compatibility.- The Dialog's
onClosehandler now respects thecloseOnOutsideClicksetting.- The modal content structure is improved with the addition of
Dialog.Panel.These changes successfully address the issue of unintended timer starts when closing the modal, as described in the PR objectives. The code is clean, readable, and maintains existing functionality while adding the requested improvements.
apps/web/lib/features/daily-plan/add-daily-plan-work-hours-modal.tsx (4)
1-1: LGTM: SpinnerLoader import addedThe addition of the
SpinnerLoaderimport is a good improvement. It will be used to provide visual feedback during asynchronous operations, enhancing the user experience.
26-26: LGTM: Loading state addedThe addition of the
loadingstate variable is a good practice. It will be used to manage the loading status during the submission process, which is essential for implementing the loading indicator and preventing multiple submissions.
94-98: LGTM: Improved submit button with loading indicatorThe addition of the conditional rendering for the submit button's content is an excellent improvement. It provides clear visual feedback to the user during the loading state, enhancing the overall user experience and preventing accidental multiple submissions.
Line range hint
1-105: Overall assessment: Good improvements with minor suggestionsThe changes to the
AddDailyPlanWorkHourModalcomponent align well with the PR objectives. The addition of loading states, improved error handling, and visual feedback enhance the user experience and address the issue of premature timer starts. The code is generally well-structured and follows good practices.Key improvements:
- Addition of loading state and visual indicator
- Conditional update of plan work time
- Proper sequencing of timer start and modal closing
Suggestions for further improvement:
- Enhance error handling with user-facing feedback
- Simplify the disabled state logic of the submit button
These changes significantly improve the component's functionality and user experience. Great work overall!
apps/web/lib/components/switch.tsx (2)
8-8: LGTM: New import for localStorage key constant.The addition of
DAILY_PLAN_SUGGESTION_MODAL_DATEfrom@app/constantsis a good practice. Using constants for localStorage keys helps maintain consistency and reduces the risk of typos.
Line range hint
1-170: Verify if additional changes are needed to fully address the issue.The changes made in this file look good and align with the PR objectives. However, to ensure that we've fully addressed the issue of the timer starting when the user ignores the 'Todayplan' popup, we should verify if additional changes are needed in other parts of the codebase.
Let's check for any other occurrences of timer start logic that might need adjustment:
This will help us ensure that we've covered all the necessary changes to fully implement the desired behavior.
apps/web/app/hooks/features/useStartStopTimerHandler.ts (1)
165-165: Approval: Refined condition for opening the enforce planned task modalThis change improves the logic for when to open the enforce planned task modal. By adding the
hasPlancondition, we ensure that the modal is only opened when there's actually a plan in place, in addition to the existing conditions.This modification aligns well with the PR objectives, addressing the issue where the timer shouldn't start when the user ignores the 'Todayplan' popup. It provides a more accurate control flow for handling timer start/stop actions.
apps/web/app/hooks/features/useTimer.ts (3)
268-268: Improved timer start conditionThe modification to the
canRunTimerlogic addresses the issue described in the PR objectives. By removing the check for the user having a plan to track time and only verifying if the timer's last log source is not fromTimerSource.TEAMS, we ensure that the timer doesn't start incorrectly when the user dismisses the "Plan Today" popup.This change aligns well with the PR's goal of fixing the bug where the timer starts unintentionally.
Line range hint
380-452: No changes needed in startTimer functionThe
startTimerfunction remains unchanged, which is appropriate given the fix implemented in thecanRunTimerlogic. This approach effectively prevents the timer from starting incorrectly without needing to modify the start process itself.The decision to keep the
startTimerfunction unchanged while modifying thecanRunTimerlogic is a clean and efficient way to address the issue.
268-268: Alignment with PR objectives and verification suggestionThe changes made to the
canRunTimerlogic align well with the PR objectives. By simplifying the condition to only check if the timer's last log source is not fromTimerSource.TEAMS, we address the issue of the timer starting incorrectly when the user dismisses the "Plan Today" popup.However, to ensure full compliance with the requirements, we should verify that:
- The timer starts when the user meets all requirements in the popup and clicks 'Start working'.
- The timer starts when the user closes the popup after meeting the requirements and then clicks 'Start' on the home page.
To verify these conditions, we can run the following script:
This script will help us confirm that the necessary UI elements and logic are in place to support the required timer start conditions.
apps/web/locales/zh.json (3)
224-225: LGTM! New localization keys added for plan management.The changes look good. Two new localization keys have been added to the
plansection:
"ADD_PLAN": "添加计划"(Add plan)"DELETE_THIS_PLAN": "删除此计划"(Delete this plan)These additions enhance the functionality for plan management in the Chinese localization, allowing users to add and delete plans.
224-225: Summary: Localization enhanced for plan management features.These additions to the Chinese localization file improve the user experience for plan management features. The new keys for adding and deleting plans are consistent with the existing localization structure and naming conventions.
To ensure a smooth implementation:
- Verify that the corresponding features (add plan and delete plan) are fully implemented in the application.
- Consider adding similar localization keys for other supported languages to maintain consistency across all localizations.
- Update any relevant documentation or user guides to reflect these new plan management features.
224-225: Verify usage of new localization keys in the application code.The new localization keys
"ADD_PLAN"and"DELETE_THIS_PLAN"have been added correctly. To ensure they are properly implemented:
- Verify that these keys are used in the appropriate components or pages where plan management functionality is present.
- Check that the UI elements for adding and deleting plans are updated to use these new localization keys.
Run the following script to check the usage of these new keys in the application code:
This script will help identify where the new localization keys are being used in the application code.
apps/web/locales/ar.json (1)
220-221: LGTM! Good additions to the localization file.The changes look good and improve the file:
- Adding a trailing comma to the "ADD_PLAN" entry (line 220) is a good practice in JSON files. It makes future additions easier and results in cleaner version control diffs.
- The new "DELETE_THIS_PLAN" entry (line 221) is a logical addition, providing a translation for the action of deleting a plan.
The Arabic translation "احذف هذه الخطة" correctly means "Delete this plan", maintaining consistency with the English version.
apps/web/locales/nl.json (1)
224-225: LGTM: New translation key added correctly.The new translation key "DELETE_THIS_PLAN" has been added correctly to the "plan" section of the Dutch localization file. The translation "Verwijder dit plan" is appropriate for "Delete this plan" in Dutch.
Let's verify if this new key is being used correctly in the codebase:
apps/web/locales/pl.json (1)
224-225: LGTM! Consider verifying the translation.The addition of the "DELETE_THIS_PLAN" key with its Polish translation "Usuń ten plan" is well-formatted and consistent with the file structure. This enhances the plan management functionality by providing a localized option to delete a plan.
As a non-Polish speaker, I recommend verifying the accuracy of the translation. You can use the following script to check for any potential inconsistencies in the translation:
✅ Verification successful
Translation Consistency Verified
The new key "DELETE_THIS_PLAN" with the translation "Usuń ten plan" aligns consistently with existing translations in the
pl.jsonfile. No inconsistencies were found in the usage of "Usuń" or "plan" across different keys.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential translation inconsistencies # Test: Compare the new translation with existing ones containing similar words echo "Checking for similar translations:" rg --json "Usuń" apps/web/locales/pl.json | jq -r '.data.lines.text' echo "---" rg --json "plan" apps/web/locales/pl.json | jq -r '.data.lines.text'Length of output: 2294
apps/web/locales/ru.json (1)
224-225: LGTM! New functionality for plan deletion added.The changes look good:
- The added comma after "ADD_PLAN" corrects the JSON syntax.
- The new "DELETE_THIS_PLAN" entry provides a Russian translation for the plan deletion feature.
The translation "Удалить этот план" (Delete this plan) is appropriate and consistent with the existing style.
apps/web/locales/es.json (1)
220-221: LGTM: New translation added correctly.The changes in the "plan" section look good:
- A comma has been added after the "FOR_TOMORROW" key-value pair on line 220, which is correct JSON syntax.
- A new translation key "DELETE_THIS_PLAN" has been added on line 221 with the appropriate Spanish translation.
These changes are consistent with the AI-generated summary and maintain the correct JSON structure.
apps/web/locales/fr.json (3)
220-221: LGTM: New translation added for deleting a planThe changes look good. A new translation key "DELETE_THIS_PLAN" has been added to the "plan" section with the French translation "Supprimer ce plan". This addition is consistent with the existing structure and naming convention of the file.
Line range hint
1-1042: Overall approval: French localization file updated correctlyThe changes to the French localization file are minimal and correct. The new translation for deleting a plan has been added in the appropriate section, following the existing structure and format of the file. No other issues or inconsistencies were found in the file.
220-221: Verify consistency across language filesWhile the addition of the "DELETE_THIS_PLAN" translation is correct, it's important to ensure that this new functionality is reflected in all other language files used by the application. This will maintain consistency across different localizations.
To check for consistency, you can run the following script:
This script will help identify any language files that might be missing the new translation key.
✅ Verification successful
All language files contain the "DELETE_THIS_PLAN" key
The "DELETE_THIS_PLAN" translation key has been successfully verified across all language files, ensuring consistency in the application's localization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the "DELETE_THIS_PLAN" key exists in all language files # Find all JSON files in the locales directory locale_files=$(fd -e json . apps/web/locales) # Check each file for the presence of the "DELETE_THIS_PLAN" key for file in $locale_files do if ! grep -q '"DELETE_THIS_PLAN"' "$file"; then echo "Warning: DELETE_THIS_PLAN key not found in $file" fi doneLength of output: 1466
apps/web/lib/features/daily-plan/add-task-estimation-hours-modal.tsx (2)
468-472: Updated modal close button behaviorThe close button now uses the new
closeModalAndSubmitfunction when rendered in the soft flow, which is consistent with the new behavior implemented in this PR.
Line range hint
1-924: Overall implementation aligns with PR objectivesThe changes in this file, particularly in the
AddTasksEstimationHoursModalcomponent, effectively address the issue mentioned in the PR objectives. The newcloseModalAndSubmitfunction ensures that the timer doesn't start when the user ignores the 'Todayplan' popup, which aligns with the expected behavior described in the linked issue.The implementation is consistent and maintains the existing structure of the component while introducing the necessary changes to fix the reported bug. The rest of the file remains largely unchanged, which helps maintain the overall integrity of the component.
| onClick={handleCloseModal} | ||
| > | ||
| {t('common.SKIP_ADD_LATER')} | ||
| </Button> | ||
| <Button | ||
| variant="default" | ||
| type="submit" | ||
| disabled={requirePlan ? (hasWorkHours ? false : true) : false} | ||
| className="py-3 px-5 rounded-md font-light text-md dark:text-white" | ||
| disabled={loading || (requirePlan ? (hasWorkHours ? false : true) : false)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the disabled state logic
The addition of the loading condition to the disabled state is a good improvement. However, the current implementation using nested ternary operators can be simplified for better readability.
Consider simplifying the disabled state logic as suggested in a previous review:
- disabled={loading || (requirePlan ? (hasWorkHours ? false : true) : false)}
+ disabled={loading || (requirePlan && !hasWorkHours)}This simplification maintains the same logic but is more readable and concise.
📝 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.
| disabled={loading || (requirePlan ? (hasWorkHours ? false : true) : false)} | |
| disabled={loading || (requirePlan && !hasWorkHours)} |
🧰 Tools
🪛 Biome
[error] 90-90: 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)
| const handleSubmit = useCallback(async () => { | ||
| try { | ||
| setLoading(true); | ||
|
|
||
| // Update the plan work time only if the user changed it | ||
| plan && | ||
| plan.workTimePlanned !== workTimePlanned && | ||
| (await updateDailyPlan({ workTimePlanned }, plan.id ?? '')); | ||
|
|
||
| startTimer(); | ||
|
|
||
| const handleSubmit = useCallback(() => { | ||
| updateDailyPlan({ workTimePlanned }, plan.id ?? ''); | ||
| handleCloseModal(); | ||
| }, [handleCloseModal, plan.id, updateDailyPlan, workTimePlanned]); | ||
| handleCloseModal(); | ||
| } catch (error) { | ||
| console.log(error); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [handleCloseModal, plan, startTimer, updateDailyPlan, workTimePlanned]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM with suggestion: Improved handleSubmit function
The changes to the handleSubmit function are good improvements:
- Proper loading state management
- Conditional update of the plan work time
- Correct sequencing of
startTimerand modal closing
However, the error handling could be enhanced:
Consider replacing the console log with user-facing error feedback:
} catch (error) {
- console.log(error);
+ // TODO: Implement proper error handling
+ // For example: showErrorNotification('Failed to update plan. Please try again.');
} finally {This will provide a better user experience by informing them of any issues during the submission process.
📝 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 handleSubmit = useCallback(async () => { | |
| try { | |
| setLoading(true); | |
| // Update the plan work time only if the user changed it | |
| plan && | |
| plan.workTimePlanned !== workTimePlanned && | |
| (await updateDailyPlan({ workTimePlanned }, plan.id ?? '')); | |
| startTimer(); | |
| const handleSubmit = useCallback(() => { | |
| updateDailyPlan({ workTimePlanned }, plan.id ?? ''); | |
| handleCloseModal(); | |
| }, [handleCloseModal, plan.id, updateDailyPlan, workTimePlanned]); | |
| handleCloseModal(); | |
| } catch (error) { | |
| console.log(error); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }, [handleCloseModal, plan, startTimer, updateDailyPlan, workTimePlanned]); | |
| const handleSubmit = useCallback(async () => { | |
| try { | |
| setLoading(true); | |
| // Update the plan work time only if the user changed it | |
| plan && | |
| plan.workTimePlanned !== workTimePlanned && | |
| (await updateDailyPlan({ workTimePlanned }, plan.id ?? '')); | |
| startTimer(); | |
| handleCloseModal(); | |
| } catch (error) { | |
| // TODO: Implement proper error handling | |
| // For example: showErrorNotification('Failed to update plan. Please try again.'); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }, [handleCloseModal, plan, startTimer, updateDailyPlan, workTimePlanned]); |
|
|
||
| return ( | ||
| <Modal isOpen={isOpen} closeModal={handleCloseModal} showCloseIcon={false}> | ||
| <Modal | ||
| closeOnOutsideClick={requirePlan} |
There was a problem hiding this comment.
Correct the modal's closing behavior logic
The current implementation allows users to close the modal even when a plan is required but not yet created. To prevent users from dismissing the modal under these conditions, update the closeOnOutsideClick and showCloseIcon properties to reflect both requirePlan and hasPlan.
Apply this diff to fix the logic:
<Modal
- closeOnOutsideClick={requirePlan}
+ closeOnOutsideClick={!requirePlan || hasPlan}
isOpen={isOpen}
closeModal={handleCloseModal}
- showCloseIcon={requirePlan}
+ showCloseIcon={!requirePlan || hasPlan}
>Also applies to: 47-47
| if (!requirePlan || (requirePlan && hasPlan)) { | ||
| localStorage.setItem(HAS_SEEN_DAILY_PLAN_SUGGESTION_MODAL, currentDate); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional expressions in handleCloseModal
The conditions if (!requirePlan || (requirePlan && hasPlan)) can be simplified to if (!requirePlan || hasPlan) for clarity and maintainability, as they are logically equivalent.
Apply this diff:
const handleCloseModal = useCallback(() => {
- if (!requirePlan || (requirePlan && hasPlan)) {
+ if (!requirePlan || hasPlan) {
localStorage.setItem(HAS_SEEN_DAILY_PLAN_SUGGESTION_MODAL, currentDate);
}
if (path.split('/')[1] == 'profile') {
- if (!requirePlan || (requirePlan && hasPlan)) {
+ if (!requirePlan || hasPlan) {
localStorage.setItem(DAILY_PLAN_SUGGESTION_MODAL_DATE, currentDate);
}
}
closeModal();
}, [closeModal, currentDate, hasPlan, path, requirePlan]);Also applies to: 35-37
There was a problem hiding this comment.
@CREDO23 why you not follow those!?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const { hasPlan } = useTimer(); | ||
| const { activeTeam } = useTeamTasks(); | ||
| const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]); |
There was a problem hiding this comment.
Include activeTeam in useMemo dependencies
In the useMemo hook, consider including activeTeam in the dependency array to ensure that requirePlan updates whenever activeTeam changes.
Apply this diff to update the dependency array:
- const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]);
+ const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam]);📝 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 { hasPlan } = useTimer(); | |
| const { activeTeam } = useTeamTasks(); | |
| const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]); | |
| const { hasPlan } = useTimer(); | |
| const { activeTeam } = useTeamTasks(); | |
| const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam]); |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if (!requirePlan || (requirePlan && hasPlan)) { | ||
| window.localStorage.setItem( | ||
| DAILY_PLAN_SUGGESTION_MODAL_DATE, | ||
| new Date().toISOString().split('T')[0] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional logic in useEffect
The condition if (!requirePlan || (requirePlan && hasPlan)) can be simplified to if (!requirePlan || hasPlan) without changing the logic, improving readability.
Apply this diff:
useEffect(() => {
// ...
- if (!requirePlan || (requirePlan && hasPlan)) {
+ if (!requirePlan || hasPlan) {
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.
| if (!requirePlan || (requirePlan && hasPlan)) { | |
| window.localStorage.setItem( | |
| DAILY_PLAN_SUGGESTION_MODAL_DATE, | |
| new Date().toISOString().split('T')[0] | |
| ); | |
| } | |
| if (!requirePlan || hasPlan) { | |
| window.localStorage.setItem( | |
| DAILY_PLAN_SUGGESTION_MODAL_DATE, | |
| new Date().toISOString().split('T')[0] | |
| ); | |
| } |
| const { activeTeam } = useTeamTasks(); | ||
| const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]); |
There was a problem hiding this comment.
Include activeTeam in useMemo dependencies
Similarly, in the AllPlans component, include activeTeam in the dependency array of the useMemo hook to properly update requirePlan when activeTeam changes.
Apply this diff:
- const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]);
+ const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam]);📝 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 { activeTeam } = useTeamTasks(); | |
| const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]); | |
| const { activeTeam } = useTeamTasks(); | |
| const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam]); |
evereq
deleted the
3156-bug-planned--the-issue-is-that-tracker-start-tracking-time-without-clicking-start-working
branch
Description
This PR fixes #3156
Type of Change
Checklist
Previous screenshots
Loom
Current screenshots
Loom
Loom
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor