[Feat]:Implement EditTaskModal component with enhanced task editing UI #3238
Conversation
…and state management
WalkthroughThis pull request introduces several enhancements to the timesheet application, including a new Changes
Possibly related PRs
Suggested reviewers
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: 10
🧹 Outside diff range and nitpick comments (10)
apps/web/lib/features/multiple-select/index.tsx (2)
89-102: Fix incomplete documentation and enhance type definitions.The JSDoc comment on line 92 has an incomplete sentence: "a selectable , such as". Additionally, consider enhancing the type definitions for better accessibility and customization.
type CustomSelectProps = { /** * Array of string options to be displayed in the dropdown. - * Each string represents a selectable , such as "daily" or "weekly". + * Each string represents a selectable option, such as "daily" or "weekly". */ options: string[]; /** * Optional render function that customizes the display of each option. * Receives the option string as an argument and returns a ReactNode to render. * If not provided, options will display with the first letter capitalized. */ renderOption?: (option: string) => React.ReactNode; + + /** + * Optional aria-label for accessibility. + */ + ariaLabel?: string; + + /** + * Optional className for custom styling of the select container. + */ + className?: string; };
104-125: Improve component documentation completeness.The component documentation has a placeholder " " in the description and could benefit from a more comprehensive example that aligns with the PR's task editing context.
/** * CustomSelect Component * * This component renders a customizable dropdown select menu for choosing a . + * This component renders a customizable dropdown select menu for choosing task-related options. * It allows passing an array of options and optionally customizes the appearance of each option. * * @component * @param {CustomSelectProps} props - Props for configuring the select component. * @param {string[]} props.options - Array of selectable options to be displayed in the dropdown. * @param {(option: string) => React.ReactNode} [props.renderOption] - Optional function for custom rendering of each option. * * @example * <CustomSelect - * options={['daily', 'weekly', 'monthly']} + * options={['In Progress', 'Completed', 'On Hold']} * renderOption={(option) => ( * <div className="flex items-center"> - * <span className="text-blue-500">{option.charAt(0).toUpperCase()}</span> - * <span className="ml-1">{option.slice(1)}</span> + * <StatusIcon status={option.toLowerCase()} /> + * <span className="ml-2">{option}</span> * </div> * )} * /> */apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx (3)
Line range hint
39-45: Inconsistent onChange callback handling in date range updates.The date handling logic has inconsistencies in how changes are propagated:
handleToChangeupdates internal state but doesn't trigger theonChangecallbackhandlePresetClickupdates state directly without triggeringonChangeThis could lead to the parent component being out of sync with the actual selected date range.
Consider applying this fix:
const handleToChange = (toDate: Date | null) => { if (dateRange.from && toDate && toDate < dateRange.from) { return; } setDateRange((prev) => ({ ...prev, to: toDate })); + onChange?.({ ...dateRange, to: toDate }); }; const handlePresetClick = (preset: string) => { const today = new Date(); + let newRange: { from: Date | null; to: Date | null }; switch (preset) { case 'Today': - setDateRange({ from: today, to: today }); + newRange = { from: today, to: today }; break; // ... other cases similarly updated ... } + setDateRange(newRange); + onChange?.(newRange); };Also applies to: 47-52
Line range hint
92-111: Enhance accessibility with additional ARIA attributes.While the component implements basic accessibility, it could be improved for better screen reader support:
- Add
aria-labelto the date range buttons- Include
aria-liveregion for dynamic updates- Add keyboard shortcuts for common actions
Consider these enhancements:
<Button variant={"outline"} role="combobox" aria-label="Select date range" aria-expanded="false" + aria-haspopup="true" + aria-controls="date-range-popup" className={cn( "w-44 justify-start dark:bg-dark--theme-light dark:text-gray-300 h-[2.2rem] items-center gap-x-2 text-left font-normal overflow-hidden text-clip", !dateRange.from && "text-muted-foreground" )}>
180-180: Consider extracting complex className into utility constants.The className string is becoming quite long and combines multiple concerns (layout, dark mode, colors, etc.). This could be harder to maintain as styles evolve.
Consider breaking it down:
+const datePickerInputStyles = { + base: "w-[150px] justify-start text-left font-normal bg-transparent hover:bg-transparent h-8", + borders: "border border-transparent dark:border-transparent", + colors: "text-black dark:text-gray-100" +}; + <Button variant="outline" - className={cn( - "w-[150px] justify-start text-left font-normal bg-transparent hover:bg-transparent text-black dark:text-gray-100 h-8 border border-transparent dark:border-transparent", - !date && "text-muted-foreground" - )} + className={cn( + datePickerInputStyles.base, + datePickerInputStyles.borders, + datePickerInputStyles.colors, + !date && "text-muted-foreground" + )} >apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (1)
18-31: Remove debug code and consolidate form state.
- Remove
console.logstatements before production deployment- Consider using a single form state object instead of multiple useState hooks
Example refactor:
interface ITaskFormState { dateRange: { from: Date | null }; startTime: string; endTime: string; isBillable: boolean; reason: string; } const [formState, setFormState] = useState<ITaskFormState>({ dateRange: { from: new Date() }, startTime: '', endTime: '', isBillable: false, reason: '' });apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (4)
234-236: Implement the onReject handler.The onReject callback is currently empty with a "Pending implementation" comment.
Would you like me to help implement the rejection logic or create a GitHub issue to track this task?
432-436: Use consistent naming for modal state variables.The current naming is inconsistent and potentially confusing. The 'is' prefix in
isCloseModalEditTaskandisOpenModalEditTaskdoesn't follow common React naming conventions.const { isOpen: isEditTask, - openModal: isOpenModalEditTask, - closeModal: isCloseModalEditTask + openModal: openEditTaskModal, + closeModal: closeEditTaskModal } = useModal();
438-444: Simplify the EditTaskModal rendering.The current implementation uses unnecessary fragment and conditional rendering.
- <> - { - <EditTaskModal - closeModal={isCloseModalEditTask} - isOpen={isEditTask} - /> - } + <> + <EditTaskModal + closeModal={closeEditTaskModal} + isOpen={isEditTask} + />
453-455: Add aria-label to Edit menu item.For better accessibility, add an aria-label to the Edit menu item.
- <DropdownMenuItem className="cursor-pointer" onClick={isOpenModalEditTask}> + <DropdownMenuItem + className="cursor-pointer" + onClick={openEditTaskModal} + aria-label="Edit task" + > Edit </DropdownMenuItem>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetFilterDate.tsx(1 hunks)apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(3 hunks)apps/web/lib/features/multiple-select/index.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
[error] 137-137: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 152-152: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
[error] 232-241: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (4)
apps/web/app/[locale]/timesheet/[memberId]/components/index.tsx (1)
Line range hint 1-12: LGTM! Well-organized barrel file exports.
The barrel file is well-structured with logically grouped exports. The addition of EditTaskModal aligns with the PR objectives of implementing enhanced task editing functionality.
Let's verify the EditTaskModal component exists and has the expected interface:
✅ Verification successful
Export of EditTaskModal component is correctly implemented
The verification confirms that:
- The
EditTaskModalcomponent exists at the correct location - It has the expected interface with
isOpenandcloseModalprops - The component is properly exported and used in the codebase
- The barrel file export aligns with the component's implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify EditTaskModal component exists and exports expected props
# Expected: Component file exists with isOpen and closeModal props
# Check if EditTaskModal.tsx exists in the same directory
fd -t f "EditTaskModal.tsx" "apps/web/app/\[locale\]/timesheet/\[memberId\]/components"
# Search for interface/type definitions containing isOpen and closeModal
ast-grep --pattern 'interface $_ {
$$$
isOpen: $_
closeModal: $_
$$$
}'
Length of output: 314
Script:
#!/bin/bash
# Let's try a different approach to verify the EditTaskModal component
# First, find the correct path pattern
fd -t f "EditTaskModal.tsx"
# Then search for the component definition and its props
ast-grep --pattern 'interface EditTaskModalProps {
$$$
}'
# Also search for the component itself
ast-grep --pattern 'const EditTaskModal = ($$$) => {
$$$
}'
# Backup search using ripgrep
rg "EditTaskModal" -A 5
Length of output: 2747
apps/web/lib/features/multiple-select/index.tsx (1)
126-147: Verify integration with EditTaskModal component.
Since this component is part of the EditTaskModal feature, we should verify its integration and usage patterns.
✅ Verification successful
CustomSelect component is properly integrated with EditTaskModal
The component is correctly used within EditTaskModal for status selection:
- Properly imports and uses the CustomSelect component
- Correctly passes status options from a well-defined statusTable (Pending, Approved, Rejected)
- Implements custom rendering with appropriate styling for status indicators
- Follows the component's API contract with required props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CustomSelect usage in EditTaskModal and related components
echo "Searching for CustomSelect usage patterns..."
rg -l "CustomSelect" --type tsx
# Look for specific task-related options being passed to CustomSelect
echo "Checking task-related options passed to CustomSelect..."
rg "options=\{.*\}" -A 3 -B 3 --type tsx
Length of output: 350
Script:
#!/bin/bash
# Search for CustomSelect usage without file type restriction
echo "Searching for CustomSelect usage patterns..."
rg -l "CustomSelect"
# Look for specific task-related options being passed to CustomSelect
echo "Checking task-related options passed to CustomSelect..."
rg "options=\{.*\}" -A 3 -B 3
# Search for EditTaskModal component
echo "Searching for EditTaskModal component..."
rg -l "EditTaskModal"
Length of output: 3821
Script:
#!/bin/bash
# Let's examine the EditTaskModal implementation to understand how CustomSelect is used
echo "Checking EditTaskModal implementation..."
rg -A 10 -B 10 "CustomSelect" "apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx"
# Check for statusTable definition
echo "Checking statusTable definition..."
rg "statusTable" -A 5 -B 5 "apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx"
Length of output: 3753
Script:
#!/bin/bash
# Let's check the TimesheetAction file to understand the statusTable structure
echo "Checking TimesheetAction implementation..."
rg -A 5 -B 5 "statusTable" "apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx"
# Let's also verify if there are any other instances of CustomSelect with similar status selection patterns
echo "Checking other status selection patterns..."
rg "statusTable.*flatMap" -A 5 -B 5
Length of output: 2301
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (1)
31-55: 🛠️ Refactor suggestion
Improve event handlers implementation.
- Add proper error handling in event handlers
- Remove unnecessary conditional checks in setIsBillable calls
Fix the optional chaining warnings:
-onClick={() => setIsBillable && setIsBillable(!isBillable)}
+onClick={() => setIsBillable?.(!isBillable)}Likely invalid or redundant comment.
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx (1)
59-59: LGTM! Import changes align with new feature requirements.
The new imports are properly organized and include all necessary components for the EditTaskModal implementation.
apps/web/app/[locale]/timesheet/[memberId]/components/TimesheetAction.tsx
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
apps/web/lib/features/multiple-select/index.tsx (3)
85-88: Consider reducing consecutive empty lines.Multiple consecutive empty lines don't add to readability. Consider keeping just one or two lines between components.
89-106: Complete the type documentation.The JSDoc comment on line 96 is incomplete: "Each string represents a selectable , such as...". Please specify what type of item is being selected.
130-133: Consider adding controlled mode support.The component uses internal state for selection but doesn't support controlled mode. Consider adding value/onChange props to allow parent components to control the selection state.
-export function CustomSelect({ options, renderOption, ariaLabel, className }: CustomSelectProps) { +export function CustomSelect({ + options, + renderOption, + ariaLabel, + className, + value, + onChange +}: CustomSelectProps & { + value?: string; + onChange?: (value: string) => void; +}) { - const [selectedValue, setSelectedValue] = React.useState<string>(); + const [internalValue, setInternalValue] = React.useState<string>(); + const selectedValue = value ?? internalValue; + + const handleValueChange = (newValue: string) => { + setInternalValue(newValue); + onChange?.(newValue); + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(3 hunks)apps/web/lib/features/multiple-select/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
🧰 Additional context used
🪛 Biome
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
[error] 141-141: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 156-156: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
apps/web/lib/features/multiple-select/index.tsx (2)
108-129: LGTM! Well-documented component.
The JSDoc documentation is comprehensive, including a clear description, proper param documentation, and a practical usage example.
146-146: Reduce z-index value.
Using such a high z-index (10000) is excessive and could cause stacking context issues. Consider using a lower value like 50 or 100.
- <SelectContent className='z-[10000] dark:bg-dark--theme-light'>
+ <SelectContent className='z-50 dark:bg-dark--theme-light'>apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (3)
13-17:
Enhance interface with task-related props.
The current interface is insufficient for proper task editing functionality.
Add the following props:
export interface IEditTaskModalProps {
isOpen: boolean;
closeModal: () => void;
+ task: {
+ id: string;
+ title: string;
+ startTime: string;
+ endTime: string;
+ date: Date;
+ projectId: string;
+ isBillable: boolean;
+ notes: string;
+ status: string;
+ };
+ onSave: (updatedTask: any) => Promise<void>;
}66-71:
Remove hardcoded task details.
- <span> #321 Spike for creating calendar views on mobile</span>
+ <span> #{task.id} {task.title}</span>
<div className="flex items-center gap-x-1 ">
<span className="text-gray-400">for</span>
- <span className="text-primary dark:text-primary-light">Savannah Nguyen </span>
+ <span className="text-primary dark:text-primary-light">{task.assignee}</span>
<IoMdArrowDropdown className="cursor-pointer" />
</div>213-220:
Add form submission handling and loading state.
<Button
type="submit"
+ disabled={isLoading || !isValid}
className={clsxm(
'bg-[#3826A6] h-[2.3rem] font-normal flex items-center text-white',
+ isLoading && 'opacity-50'
)}
+ onClick={handleSubmit}
>
- Save
+ {isLoading ? 'Saving...' : 'Save'}
</Button>Add form submission handler:
const [isLoading, setIsLoading] = useState(false);
const [isValid, setIsValid] = useState(false);
const handleSubmit = async () => {
try {
setIsLoading(true);
await onSave({
id: task.id,
startTime,
endTime,
date: dateRange.from,
isBillable,
notes,
// ... other fields
});
closeModal();
} catch (error) {
// Handle error
} finally {
setIsLoading(false);
}
};
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (1)
216-231: Enhance ToggleButton accessibility.Add ARIA attributes to improve accessibility:
const ToggleButton = ({ isActive, onClick, label }: ToggleButtonProps) => ( <div className="flex items-center gap-x-2"> <div className="w-6 h-6 flex items-center bg-[#6c57f4b7] rounded-full p-1 cursor-pointer" onClick={onClick} + role="switch" + aria-checked={isActive} + aria-label={label} + tabIndex={0} + onKeyPress={(e) => e.key === 'Enter' && onClick()} style={{ background: isActive ? 'linear-gradient(to right, #9d91efb7, #8a7bedb7)' : '#6c57f4b7' }} > <div className="bg-[#3826A6] w-4 h-4 rounded-full shadow-md transform transition-transform translate-x-0" /> </div> <span>{label}</span> </div> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx(1 hunks)apps/web/lib/features/integrations/calendar/table-time-sheet.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/lib/features/integrations/calendar/table-time-sheet.tsx
🔇 Additional comments (3)
apps/web/app/[locale]/timesheet/[memberId]/components/EditTaskModal.tsx (3)
13-17:
Enhance interface with essential task-related props.
The current interface is insufficient for task editing functionality. Consider adding:
task: Task- The task being editedonSave: (updatedTask: Task) => Promise<void>- Callback for saving changesisLoading?: boolean- Loading state during save operation
66-66:
Remove hardcoded task details.
Task ID and title should be passed through props.
192-199:
Enhance save button with loading and validation states.
The save button needs:
- Loading state indicator
- Disable during submission
- Form validation before submission
Innocent-Akim
removed
the
Don't Merge
Description
Please include a summary of the changes and the related issue.
Type of Change
Checklist
Previous screenshots
Please add here videos or images of previous status
Current screenshots
Please add here videos or images of previous status
Summary by CodeRabbit
Release Notes
New Features
EditTaskModalfor editing task details, enhancing task management capabilities.CustomSelectcomponent for customizable dropdown menus.Improvements
TimesheetFilterDatefor better date handling and improved styling consistency.DataTableTimeSheetwith integrated task editing options.statusTable.Bug Fixes
Chores