feat: add item on this week (item add component for this week and inbox) #583
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve the introduction of a new component, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@joaorceschini has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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: 2
🧹 Outside diff range and nitpick comments (5)
apps/frontend/src/components/atoms/ItemAdd.tsx (2)
19-25: Consider debouncing the textarea height adjustmentThe current implementation might cause frequent layout shifts and re-renders when typing quickly. Consider debouncing the height adjustment for better performance.
+import { debounce } from "lodash" + useEffect(() => { - const textarea = textareaRefTitle.current - if (textarea) { - textarea.style.height = "auto" - textarea.style.height = `${textarea.scrollHeight}px` - } + const adjustHeight = debounce(() => { + const textarea = textareaRefTitle.current + if (textarea) { + textarea.style.height = "auto" + textarea.style.height = `${textarea.scrollHeight}px` + } + }, 100) + + adjustHeight() + return () => adjustHeight.cancel() }, [title])
65-75: Enhance keyboard interactionsConsider adding more keyboard shortcuts for better user experience, such as Escape to cancel.
const handleKeyDown = (event: React.KeyboardEvent<HTMLTextAreaElement>) => { if (!addingItem) { setAddingItem(true) } - if (event.key === "Enter") { + + if (event.key === "Escape") { + event.preventDefault() + handleCloseAddItem() + } else if (event.key === "Enter") { event.preventDefault() if (title) { handleAddItem() } } }apps/frontend/src/lib/store/cycle.store.ts (3)
Line range hint
89-89: Fix remaining "unknow" typos in error messagesThere are several instances where error messages still use "unknow" instead of "unknown":
- Line 89:
"unknow: failed to fetch inbox"- Line 127:
"unknow: failed to fetch by date"- Line 165:
"unknow: failed to fetch overdue"Apply this fix consistently across all error messages:
-const errorMessage = "unknow: failed to fetch inbox" +const errorMessage = "unknown: failed to fetch inbox" -const errorMessage = "unknow: failed to fetch by date" +const errorMessage = "unknown: failed to fetch by date" -const errorMessage = "unknow: failed to fetch overdue" +const errorMessage = "unknown: failed to fetch overdue"Also applies to: 127-127, 165-165
Line range hint
385-486: Consider extracting common view update logicThe
updateItemfunction contains repeated patterns for updating items across different views. Consider extracting these patterns into helper functions to improve maintainability and reduce code duplication.Example refactor:
type ViewUpdater = (items: CycleItem[], id: string, updates: Partial<CycleItem>) => CycleItem[] const createViewUpdater = (options: { removeOnDueDate?: boolean, removeOnCycle?: boolean, isOverdue?: boolean }): ViewUpdater => { return (items, id, updates) => { if (options.removeOnDueDate && updates.dueDate) { return items.filter(item => item._id !== id) } if (options.removeOnCycle && updates.cycle) { return items.filter(item => item._id !== id) } return items.map(item => item._id === id ? { ...item, ...updates } : item ) } } const updateItemsInViewInbox = createViewUpdater({ removeOnDueDate: true, removeOnCycle: true })
Line range hint
353-367: Add type guard for error handlingThe error handling could be improved by adding TypeScript type guards to better handle different types of errors and provide more specific error messages.
Example implementation:
const isAxiosError = (error: unknown): error is AxiosError => { return error instanceof AxiosError } try { // ... existing code } catch (error) { const errorMessage = isAxiosError(error) ? `unknown: ${error.response?.data?.message || 'failed to create item'}` : 'unknown: failed to create item' console.error(errorMessage, error) // ... existing error handling }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/frontend/src/components/Inbox/InboxPage.tsx(2 hunks)apps/frontend/src/components/ThisWeek/ThisWeekPage.tsx(3 hunks)apps/frontend/src/components/atoms/ItemAdd.tsx(1 hunks)apps/frontend/src/lib/store/cycle.store.ts(1 hunks)
🔇 Additional comments (11)
apps/frontend/src/components/Inbox/InboxPage.tsx (3)
31-34: Clean integration within the layout
The placement and structure look good. The ItemAdd component is properly integrated within the flex container, maintaining consistent spacing and layout hierarchy.
32-32: Verify props migration from InboxAddItem
The component is used without props, but we should verify if any props from the previous InboxAddItem need to be migrated.
#!/bin/bash
# Description: Check previous implementation for required props
# Search for previous InboxAddItem usage patterns
rg -B 5 -A 5 "InboxAddItem" --type tsx
# Look for any prop interfaces or types
ast-grep --pattern 'interface $name {
$$$
}' -l typescript3-3: Verify the ItemAdd component implementation
The import path suggests this is now an atomic component, which aligns with better component organization. Let's verify its implementation.
✅ Verification successful
Component replacement is properly implemented
The ItemAdd component is a direct refactor of InboxAddItem with identical functionality and props interface. The only changes are:
- Renamed component from
InboxAddItemtoItemAdd - Renamed handlers from
handleAddItemToInboxtohandleAddItem - Moved to atoms directory for better component organization
The component maintains all the original features including link detection, error handling, and textarea auto-resizing. The change is safe and aligns with the goal of creating reusable atomic components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ItemAdd component exists and check its interface
# Check if the component file exists and show its content
fd -e tsx -e ts "ItemAdd" "apps/frontend/src/components/atoms" --exec cat {}
# Look for any existing usages to understand the expected props
rg -A 5 "<ItemAdd" --type tsx
Length of output: 282
Script:
#!/bin/bash
# Let's try different approaches to verify the component
# Check if ItemAdd component exists
fd "ItemAdd" apps/frontend/src/components/atoms
# Search for ItemAdd usage patterns with ripgrep (without file type restriction)
rg -A 5 "<ItemAdd"
# Search for the old InboxAddItem component to understand the migration
rg -A 5 "<InboxAddItem"
# Look for ItemAdd interface or type definitions
rg "interface.*ItemAdd|type.*ItemAdd"
Length of output: 1062
Script:
#!/bin/bash
# Let's examine the ItemAdd component implementation since we confirmed it exists
# Show the content of ItemAdd component
cat apps/frontend/src/components/atoms/ItemAdd.tsx
# Look for the old InboxAddItem implementation to compare
fd "InboxAddItem" apps/frontend/src --exec cat {}
Length of output: 5931
apps/frontend/src/components/atoms/ItemAdd.tsx (2)
1-17: LGTM! Component setup and imports are well-structured.
The component is properly configured for client-side rendering with appropriate imports and type definitions.
1-115: Verify integration with ThisWeekPage component
While the component implementation looks good, let's verify its integration with ThisWeekPage to ensure it meets the PR objectives.
✅ Verification successful
ItemAdd component is properly integrated
The ItemAdd component is correctly imported and integrated in both ThisWeekPage and InboxPage components, positioned appropriately in their respective layouts. The implementation aligns with the PR objectives for adding items to both this week and inbox sections.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ItemAdd component usage in ThisWeekPage
rg -l "ItemAdd" "apps/frontend/src/pages"
# Check if the component is properly imported and positioned
ast-grep --pattern 'import { ItemAdd } from "@/src/components/atoms/ItemAdd"'
Length of output: 405
Script:
#!/bin/bash
# Check how ItemAdd is used in ThisWeekPage and InboxPage
ast-grep --pattern 'function ThisWeekPage() {
$$$
return (
$$$
<ItemAdd $$$/>
$$$
)
}'
ast-grep --pattern 'function InboxPage() {
$$$
return (
$$$
<ItemAdd $$$/>
$$$
)
}'
Length of output: 210
Script:
#!/bin/bash
# Let's try a simpler pattern to check component usage
rg -A 5 -B 5 "<ItemAdd" apps/frontend/src/components/ThisWeek/ThisWeekPage.tsx apps/frontend/src/components/Inbox/InboxPage.tsx
Length of output: 1866
apps/frontend/src/components/ThisWeek/ThisWeekPage.tsx (4)
8-8: LGTM! Import follows component architecture.
The ItemAdd component is correctly imported from the atoms directory, following the atomic design pattern.
62-62: Verify gap spacing matches Figma specs.
The gap has been reduced from gap-12 to gap-5. While this aligns with the PR objective of fixing spacing issues, please verify this matches the Figma specifications mentioned in MAR-253.
Consider extracting these spacing values into a design token system for better maintainability and consistency across the application.
Line range hint 68-74: Verify header format matches design requirements.
The header display has been simplified to show "{todoAndNullItems} / {totalItems} completed" with completion percentage. While this makes the UI cleaner, please ensure this matches the exact format specified in the Figma designs.
Consider extracting the percentage calculation into a separate utility function for better reusability and testability:
+const calculateCompletionPercentage = (doneCount: number, totalCount: number): string => {
+ return totalCount > 0 ? ((doneCount / totalCount) * 100).toFixed(0) : "0"
+}
// Usage in JSX
-{items.length > 0
- ? ((doneItems.length / items.length) * 100).toFixed(0)
- : 0}
+{calculateCompletionPercentage(doneItems.length, items.length)}88-88: Verify ItemAdd component behavior.
The ItemAdd component is integrated without props, suggesting it might be using internal state management. Please ensure:
- The component correctly adds items to the current week view
- It handles all error states appropriately
- The UI/UX matches the Figma specifications
Consider whether the component needs configuration props for flexibility, such as:
- Default date range
- Validation rules
- Custom error handling
apps/frontend/src/lib/store/cycle.store.ts (2)
346-346: LGTM: Consistent item ordering across views
The prepending of new items in the thisWeek view matches the inbox behavior, providing a consistent user experience where new items appear at the top of the list.
Line range hint 290-307: Verify cycle date handling in thisWeek view
The code correctly sets cycle dates for items created in thisWeek view, but let's verify that:
- The dates are properly converted to UTC
- The cycle dates are respected when displaying items
✅ Verification successful
Let me analyze the date handling implementation further by checking the actual implementation of toUtcDate and related date utilities.
Based on the comprehensive analysis of the codebase, I can now provide a final response regarding the cycle date handling verification:
Cycle date handling is properly implemented
The implementation correctly handles dates in the thisWeek view through:
- Proper UTC conversion using
toUtcDateutility that accounts for timezone offset - Consistent ISO string formatting for cycle dates in the store
- Correct date range handling in
getWeekDatesutility
Key verified points:
createItemproperly sets cycle dates using ISO strings from thisWeek view datesupdateItemmaintains UTC consistency when handling date updates- Date utilities properly handle timezone conversions and week boundaries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent date handling across the codebase
# Look for date conversion patterns
echo "Checking date conversion patterns..."
rg -n "toISOString|toUtcDate" --type ts
# Look for cycle date usage
echo "Checking cycle date usage..."
rg -n "cycle\.(startsAt|endsAt)" --type ts
# Look for potential timezone issues
echo "Checking for potential timezone handling..."
rg -n "new Date\(\)" --type ts
Length of output: 6076
Script:
#!/bin/bash
# Check the implementation of date utilities
echo "Checking datetime utility implementations..."
cat apps/frontend/src/utils/datetime.ts
# Check the thisWeek view implementation
echo "Checking thisWeek view implementation..."
cat apps/frontend/src/components/ThisWeek/ThisWeekPage.tsx
# Check cycle store date handling
echo "Checking cycle store date handling..."
cat apps/frontend/src/lib/store/cycle.store.ts
Length of output: 23283
| const handleAddItem = async () => { | ||
| const trimmedTitle = title.trim() | ||
|
|
||
| if (!trimmedTitle) { | ||
| return | ||
| } | ||
|
|
||
| const linkDetected = isLink(trimmedTitle) | ||
|
|
||
| // prepare the final URL if its a link | ||
| const finalTitle = | ||
| linkDetected && !/^https:\/\//i.test(trimmedTitle) | ||
| ? `https://${trimmedTitle}` | ||
| : trimmedTitle | ||
|
|
||
| // prepare the item data | ||
| const data: Partial<CycleItem> = { | ||
| title: finalTitle, | ||
| type: linkDetected ? "link" : "issue", | ||
| } | ||
|
|
||
| if (linkDetected) { | ||
| data.metadata = { | ||
| url: finalTitle, | ||
| } | ||
| } | ||
|
|
||
| await createItem(session, data) | ||
|
|
||
| setAddingItem(false) | ||
| setTitle("") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance URL validation and error handling
The current URL validation and handling could be improved to handle more edge cases and provide better error feedback.
+const isValidUrl = (url: string) => {
+ try {
+ new URL(url)
+ return true
+ } catch {
+ return false
+ }
+}
+
const handleAddItem = async () => {
const trimmedTitle = title.trim()
if (!trimmedTitle) {
+ setError("Title cannot be empty")
return
}
const linkDetected = isLink(trimmedTitle)
+ let finalTitle = trimmedTitle
- // prepare the final URL if its a link
- const finalTitle =
- linkDetected && !/^https:\/\//i.test(trimmedTitle)
- ? `https://${trimmedTitle}`
- : trimmedTitle
+ if (linkDetected) {
+ finalTitle = !/^https?:\/\//i.test(trimmedTitle)
+ ? `https://${trimmedTitle}`
+ : trimmedTitle
+
+ if (!isValidUrl(finalTitle)) {
+ setError("Invalid URL format")
+ return
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <div onBlur={handleOnBlur} className="pl-5"> | ||
| <textarea | ||
| ref={textareaRefTitle} | ||
| value={title} | ||
| onChange={(e) => setTitle(e.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| placeholder="add anything..." | ||
| className="w-full resize-none overflow-hidden truncate whitespace-pre-wrap break-words bg-background text-sm text-foreground outline-none placeholder:text-secondary-foreground focus:outline-none" | ||
| // eslint-disable-next-line jsx-a11y/no-autofocus | ||
| autoFocus | ||
| rows={1} | ||
| /> | ||
| {error && ( | ||
| <div className="truncate text-xs text-danger-foreground"> | ||
| <span>{error}</span> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and error handling
The component needs better accessibility support and the ESLint disable comment should be justified.
<div onBlur={handleOnBlur} className="pl-5">
<textarea
ref={textareaRefTitle}
value={title}
onChange={(e) => setTitle(e.target.value)}
onKeyDown={handleKeyDown}
placeholder="add anything..."
+ aria-label="Add new item"
+ aria-invalid={!!error}
+ aria-describedby={error ? "error-message" : undefined}
className="w-full resize-none overflow-hidden truncate whitespace-pre-wrap break-words bg-background text-sm text-foreground outline-none placeholder:text-secondary-foreground focus:outline-none"
- // eslint-disable-next-line jsx-a11y/no-autofocus
+ // Autofocus is intentional here as this is the primary input component
autoFocus
rows={1}
/>
{error && (
- <div className="truncate text-xs text-danger-foreground">
+ <div
+ id="error-message"
+ role="alert"
+ className="truncate text-xs text-danger-foreground"
+ >
<span>{error}</span>
</div>
)}
</div>📝 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.
| return ( | |
| <div onBlur={handleOnBlur} className="pl-5"> | |
| <textarea | |
| ref={textareaRefTitle} | |
| value={title} | |
| onChange={(e) => setTitle(e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder="add anything..." | |
| className="w-full resize-none overflow-hidden truncate whitespace-pre-wrap break-words bg-background text-sm text-foreground outline-none placeholder:text-secondary-foreground focus:outline-none" | |
| // eslint-disable-next-line jsx-a11y/no-autofocus | |
| autoFocus | |
| rows={1} | |
| /> | |
| {error && ( | |
| <div className="truncate text-xs text-danger-foreground"> | |
| <span>{error}</span> | |
| </div> | |
| )} | |
| </div> | |
| ) | |
| return ( | |
| <div onBlur={handleOnBlur} className="pl-5"> | |
| <textarea | |
| ref={textareaRefTitle} | |
| value={title} | |
| onChange={(e) => setTitle(e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder="add anything..." | |
| aria-label="Add new item" | |
| aria-invalid={!!error} | |
| aria-describedby={error ? "error-message" : undefined} | |
| className="w-full resize-none overflow-hidden truncate whitespace-pre-wrap break-words bg-background text-sm text-foreground outline-none placeholder:text-secondary-foreground focus:outline-none" | |
| // Autofocus is intentional here as this is the primary input component | |
| autoFocus | |
| rows={1} | |
| /> | |
| {error && ( | |
| <div | |
| id="error-message" | |
| role="alert" | |
| className="truncate text-xs text-danger-foreground" | |
| > | |
| <span>{error}</span> | |
| </div> | |
| )} | |
| </div> | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/frontend/src/components/ThisWeek/ThisWeekItems.tsx (2)
42-44: Consider adding error handling and loading stateWhile the fetch logic is correct, consider these improvements:
- Add error handling for failed fetches
- Implement loading state management
- Consider memoizing the date parameters to prevent unnecessary fetches
+ const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState<Error | null>(null) useEffect(() => { + setIsLoading(true) + setError(null) fetchThisWeek(session, startDate, endDate) + .catch(err => setError(err)) + .finally(() => setIsLoading(false)) }, [session, fetchThisWeek, startDate, endDate])
Line range hint
1-200: Consider breaking down the component for better maintainabilityThe component currently handles multiple responsibilities and maintains complex state logic. Consider:
- Extracting rescheduling logic into a custom hook (e.g.,
useRescheduleItem)- Moving the calendar modal into a separate component
- Creating a custom hook for item status management
This would improve code organization, reusability, and testing.
Example structure:
// hooks/useRescheduleItem.ts export function useRescheduleItem(session: Session, onUpdate: (id: string, data: any) => void) { const [reschedulingItemId, setReschedulingItemId] = useState<string | null>(null) // ... other state and logic return { reschedulingItemId, handleReschedule: // ... // ... other methods } } // components/RescheduleModal.tsx export function RescheduleModal({ isOpen, onClose, onReschedule }) { // ... calendar modal logic }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/frontend/src/components/ThisWeek/ThisWeekItems.tsx(1 hunks)
🔇 Additional comments (1)
apps/frontend/src/components/ThisWeek/ThisWeekItems.tsx (1)
38-40: LGTM: Clean and focused date management hook
The useEffect hook correctly manages week dates with proper dependency tracking.
| fetchThisWeek(session, startDate, endDate) | ||
| }, [session, fetchThisWeek, startDate, endDate]) | ||
|
|
||
| useEffect(() => {}, [items]) |
There was a problem hiding this comment.
Remove empty useEffect hook
The empty useEffect hook with a dependency on items serves no purpose and may cause unnecessary re-renders. If this is a placeholder for future functionality, it should be documented or removed until needed.
- useEffect(() => {}, [items])📝 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.
| useEffect(() => {}, [items]) |
What did you ship?
feat: add item on this week (item add component for this week and inbox)
fix: this week header
Fixes:
Checklist:
OR: