refactor: ui inbox today #429
Conversation
WalkthroughThe pull request introduces several modifications across multiple components in the frontend application, primarily focusing on visual presentation and layout adjustments. Changes include reduced padding in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
apps/frontend/src/components/Inbox/InboxPage.tsx (1)
22-27: Consider text casing and accessibility improvements
The header structure is well-organized with proper heading hierarchy, but consider these minor enhancements:
- Use title case for headings (e.g., "All Unorganised Items")
- Add an aria-label to the header section for better screen reader context
- <header className="flex flex-col gap-3 pl-4 text-foreground">
+ <header className="flex flex-col gap-3 pl-4 text-foreground" aria-label="Inbox Section">
<div className="flex items-center gap-3 text-secondary-foreground">
<ListFilter size={16} />
- <h1 className="">all unorganised items</h1>
+ <h1 className="">All Unorganised Items</h1>
</div>
<h2 className="text-sm font-semibold">Inbox</h2>
</header>apps/frontend/src/components/atoms/Date.tsx (1)
8-12: Remove redundant comment
The format is self-explanatory from the template literal string. The comment // Desired format doesn't add any value and can be removed.
const weekday = date.toLocaleDateString("en-US", { weekday: "long" })
const month = date.toLocaleDateString("en-US", { month: "short" })
const day = date.getDate()
- return `${weekday}, ${month} ${day}` // Desired format
+ return `${weekday}, ${month} ${day}`apps/frontend/src/components/Inbox/InboxAddItem.tsx (4)
Line range hint 19-21: Remove unused state variables.
The following states are initialized but never used in the component:
descriptiondateselectedPages
This could indicate either missing functionality or leftover code from previous implementations.
const [addingItem, setAddingItem] = useState(false)
const textareaRefTitle = useRef<HTMLTextAreaElement>(null)
const [title, setTitle] = useState("")
- const [description, setDescription] = useState("")
- const [date, setDate] = React.useState<Date | undefined>(new Date())
- const [selectedPages, setSelectedPages] = React.useState<string[]>([])Line range hint 37-37: Address the TODO comment about textarea.
There's a TODO comment about fixing the textarea. Since this component is being refactored, now would be a good time to address this.
Would you like me to help investigate and fix any textarea-related issues? Please let me know what specific problems need to be addressed.
Line range hint 54-67: Add error handling UI feedback.
The error handling only logs to console, which isn't user-friendly. Consider adding toast notifications or inline error messages to inform users when item creation fails.
try {
const data: Partial<CycleItem> = {
title,
}
await createItem(session, data)
setAddingItem(false)
setTitle("")
- setDescription("")
} catch (error) {
console.error("error adding item to inbox:", error)
+ // Add user-facing error notification
+ toast.error("Failed to add item. Please try again.")
}Line range hint 54-58: Consider adding loading state during item creation.
The createItem operation is asynchronous but there's no loading indicator to provide feedback to users while the operation is in progress.
+ const [isCreating, setIsCreating] = useState(false)
const handleAddItemToInbox = async () => {
if (!session) {
console.error("user is not authenticated")
return
}
try {
+ setIsCreating(true)
const data: Partial<CycleItem> = {
title,
}
await createItem(session, data)
setAddingItem(false)
setTitle("")
} catch (error) {
console.error("error adding item to inbox:", error)
} finally {
+ setIsCreating(false)
}
}Then update the UI to show a loading state:
<button
className="hover-bg rounded-lg p-4"
onClick={() => setAddingItem(true)}
+ disabled={isCreating}
>
<div className="flex items-center gap-2">
- <p className="text-sm">add anything..</p>
+ <p className="text-sm">{isCreating ? "Adding..." : "add anything.."}</p>
</div>
</button>📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/frontend/src/app/(app)/inbox/page.tsx (1 hunks)
- apps/frontend/src/components/Inbox/InboxAddItem.tsx (1 hunks)
- apps/frontend/src/components/Inbox/InboxPage.tsx (2 hunks)
- apps/frontend/src/components/Today/TodayPage.tsx (1 hunks)
- apps/frontend/src/components/atoms/Date.tsx (2 hunks)
🔇 Additional comments (7)
apps/frontend/src/app/(app)/inbox/page.tsx (1)
14-14: Verify overflow behavior with reduced padding.
The padding reduction from p-16 to p-10 looks good, but since this section has overflow-y-hidden, please verify that no content is being unintentionally clipped with the smaller padding, especially when the inbox contains many items.
✅ Verification successful
Padding adjustment is safe for overflow behavior
The reduced padding from p-16 to p-10 is safe because:
- The main content is rendered in
InboxItemswhich has its own scrollable container withoverflow-y-auto - The parent components maintain proper height constraints with
h-full - The layout structure shows proper nesting of scroll and height constraints:
- Page section:
h-full overflow-y-hidden - InboxPage wrapper:
h-full - Content container:
h-full flex-col - InboxItems:
h-full overflow-y-auto
- Page section:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for height-related styles or scroll containers that might be affected
rg -g '*.tsx' -g '*.css' 'overflow-y|h-\[|height:' apps/frontend/src/components/Inbox/
Length of output: 237
Script:
#!/bin/bash
# Let's check the InboxItems component usage and its parent containers
ast-grep --pattern 'InboxItems'
# Also check for any height-related styles in the inbox page and its immediate parent
rg -g '*.tsx' 'className="[^"]*h-' "apps/frontend/src/app/(app)/inbox/"
Length of output: 543
Script:
#!/bin/bash
# Let's check the InboxPage component to understand the full layout structure
cat apps/frontend/src/components/Inbox/InboxPage.tsx
# Also check if there are any scroll-related styles in parent layouts
rg -g '*.tsx' 'overflow|h-full' "apps/frontend/src/app/(app)/layout.tsx"
Length of output: 1362
apps/frontend/src/components/Inbox/InboxPage.tsx (2)
4-4: LGTM: Clean import addition
The ListFilter import is properly placed and utilized in the component.
18-18: LGTM: Improved layout spacing
The reduced gap spacing creates a more compact and refined layout, aligning well with the UI improvements objective.
apps/frontend/src/components/atoms/Date.tsx (1)
3-4: LGTM: Clean import addition
The ListFilter icon import is properly placed and follows React conventions.
apps/frontend/src/components/Today/TodayPage.tsx (2)
22-36: Well-structured layout with semantic HTML
The new layout structure is clean and well-organized, with proper use of semantic HTML elements and consistent typography.
21-21:
Verify overflow behavior with varying content lengths
The overflow-y-hidden class might prevent users from accessing content that extends beyond the viewport height. Consider using overflow-y-auto instead to ensure all content remains accessible.
- <main className="ml-[180px] h-full overflow-y-hidden bg-background p-10">
+ <main className="ml-[180px] h-full overflow-y-auto bg-background p-10">apps/frontend/src/components/Inbox/InboxAddItem.tsx (1)
99-99: LGTM! UI text change looks good.
The updated button text is more concise and friendly, aligning well with modern UI patterns.
| @@ -11,7 +11,7 @@ export const metadata: Metadata = generateMetadata({ | |||
|
|
|||
| const Inbox: React.FC = () => { | |||
| return ( | |||
| <section className="ml-[160px] h-full overflow-y-hidden bg-background p-16 text-secondary-foreground"> | |||
| <section className="ml-[160px] h-full overflow-y-hidden bg-background p-10 text-secondary-foreground"> | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding responsive padding.
The fixed left margin (ml-[160px]) and padding (p-10) might not provide the best experience on smaller screens. Consider using Tailwind's responsive prefixes to adjust these values based on screen size.
- <section className="ml-[160px] h-full overflow-y-hidden bg-background p-10 text-secondary-foreground">
+ <section className="ml-[160px] h-full overflow-y-hidden bg-background p-4 sm:p-6 md:p-8 lg:p-10 text-secondary-foreground">📝 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.
| <section className="ml-[160px] h-full overflow-y-hidden bg-background p-10 text-secondary-foreground"> | |
| <section className="ml-[160px] h-full overflow-y-hidden bg-background p-4 sm:p-6 md:p-8 lg:p-10 text-secondary-foreground"> |
| <div className="space-y-2"> | ||
| <div className="flex items-center justify-between"> | ||
| <div className="flex w-48 items-center gap-2 text-secondary-foreground"> | ||
| <ListFilter size={16} /> | ||
| <p className="text-sm">{formatDate(selectedDate)}</p> | ||
| </div> | ||
| <div className="flex items-center gap-2"> | ||
| <button onClick={goToPreviousDay} className="p-2"> | ||
| <LeftChevron /> | ||
| </button> | ||
| <button onClick={goToNextDay} className="p-2"> | ||
| <RightChevron /> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| {isToday && ( | ||
| <p className="text-sm font-semibold text-foreground">Today</p> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user experience
While the layout structure is clean, consider the following improvements:
- Add accessibility attributes to navigation buttons
- Consider responsive width instead of fixed width
- Add hover/focus states for better interaction feedback
<div className="space-y-2">
<div className="flex items-center justify-between">
- <div className="flex w-48 items-center gap-2 text-secondary-foreground">
+ <div className="flex min-w-[12rem] max-w-full items-center gap-2 text-secondary-foreground">
<ListFilter size={16} />
<p className="text-sm">{formatDate(selectedDate)}</p>
</div>
<div className="flex items-center gap-2">
- <button onClick={goToPreviousDay} className="p-2">
+ <button
+ onClick={goToPreviousDay}
+ className="p-2 hover:bg-secondary focus:outline-none focus:ring-2 focus:ring-primary rounded-sm"
+ aria-label="Previous day">
<LeftChevron />
</button>
- <button onClick={goToNextDay} className="p-2">
+ <button
+ onClick={goToNextDay}
+ className="p-2 hover:bg-secondary focus:outline-none focus:ring-2 focus:ring-primary rounded-sm"
+ aria-label="Next day">
<RightChevron />
</button>
</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.
| <div className="space-y-2"> | |
| <div className="flex items-center justify-between"> | |
| <div className="flex w-48 items-center gap-2 text-secondary-foreground"> | |
| <ListFilter size={16} /> | |
| <p className="text-sm">{formatDate(selectedDate)}</p> | |
| </div> | |
| <div className="flex items-center gap-2"> | |
| <button onClick={goToPreviousDay} className="p-2"> | |
| <LeftChevron /> | |
| </button> | |
| <button onClick={goToNextDay} className="p-2"> | |
| <RightChevron /> | |
| </button> | |
| </div> | |
| </div> | |
| {isToday && ( | |
| <p className="text-sm font-semibold text-foreground">Today</p> | |
| )} | |
| <div className="space-y-2"> | |
| <div className="flex items-center justify-between"> | |
| <div className="flex min-w-[12rem] max-w-full items-center gap-2 text-secondary-foreground"> | |
| <ListFilter size={16} /> | |
| <p className="text-sm">{formatDate(selectedDate)}</p> | |
| </div> | |
| <div className="flex items-center gap-2"> | |
| <button | |
| onClick={goToPreviousDay} | |
| className="p-2 hover:bg-secondary focus:outline-none focus:ring-2 focus:ring-primary rounded-sm" | |
| aria-label="Previous day"> | |
| <LeftChevron /> | |
| </button> | |
| <button | |
| onClick={goToNextDay} | |
| className="p-2 hover:bg-secondary focus:outline-none focus:ring-2 focus:ring-primary rounded-sm" | |
| aria-label="Next day"> | |
| <RightChevron /> | |
| </button> | |
| </div> | |
| </div> | |
| {isToday && ( | |
| <p className="text-sm font-semibold text-foreground">Today</p> | |
| )} |
| <section className="w-[96%] max-w-[300px]"> | ||
| <div className="flex items-center justify-end gap-4"> | ||
| <span className="mt-2 text-[11px] font-medium text-foreground"> | ||
| show agenda | ||
| </span> | ||
| <ShowAgenda toggle={showAgenda} onToggle={handleToggleAgenda} /> | ||
| </div> | ||
| )} | ||
| {showAgenda && ( | ||
| <div className="my-4 text-secondary-foreground"> | ||
| <TodayMeetings selectedDate={selectedDate} /> | ||
| </div> | ||
| )} | ||
| </section> |
There was a problem hiding this comment.
Consider increasing the font size for better accessibility
The "show agenda" label uses an 11px font size, which might be too small for optimal readability. The Web Content Accessibility Guidelines (WCAG) recommend a minimum font size of 12px for body text.
- <span className="mt-2 text-[11px] font-medium text-foreground">
+ <span className="mt-2 text-xs font-medium text-foreground">Also, consider adding an aria-label to improve screen reader experience:
- <ShowAgenda toggle={showAgenda} onToggle={handleToggleAgenda} />
+ <ShowAgenda
+ toggle={showAgenda}
+ onToggle={handleToggleAgenda}
+ aria-label="Toggle agenda visibility"
+ />📝 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.
| <section className="w-[96%] max-w-[300px]"> | |
| <div className="flex items-center justify-end gap-4"> | |
| <span className="mt-2 text-[11px] font-medium text-foreground"> | |
| show agenda | |
| </span> | |
| <ShowAgenda toggle={showAgenda} onToggle={handleToggleAgenda} /> | |
| </div> | |
| )} | |
| {showAgenda && ( | |
| <div className="my-4 text-secondary-foreground"> | |
| <TodayMeetings selectedDate={selectedDate} /> | |
| </div> | |
| )} | |
| </section> | |
| <section className="w-[96%] max-w-[300px]"> | |
| <div className="flex items-center justify-end gap-4"> | |
| <span className="mt-2 text-xs font-medium text-foreground"> | |
| show agenda | |
| </span> | |
| <ShowAgenda | |
| toggle={showAgenda} | |
| onToggle={handleToggleAgenda} | |
| aria-label="Toggle agenda visibility" | |
| /> | |
| </div> | |
| {showAgenda && ( | |
| <div className="my-4 text-secondary-foreground"> | |
| <TodayMeetings selectedDate={selectedDate} /> | |
| </div> | |
| )} | |
| </section> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/frontend/src/components/Inbox/InboxPage.tsx (1)
24-32: Consider enhancing accessibility and semantic structure.
While the new header layout improves visual hierarchy, consider these improvements:
- Add aria-label to the ListFilter icon
- Use more semantic heading structure (h1 for "Inbox" instead of "all unorganised items")
- Add aria-live region for the dynamic item count
<header className="flex flex-col gap-3 pl-4 text-foreground">
<div className="flex items-center gap-3 text-secondary-foreground">
- <ListFilter size={16} />
+ <ListFilter size={16} aria-label="Filter list" />
<h1 className="">all unorganised items</h1>
</div>
<div className="flex items-center gap-2">
- <h2 className="text-sm font-semibold">Inbox</h2>
- <p className="text-sm text-secondary-foreground">{totalItems}</p>
+ <h1 className="text-sm font-semibold">Inbox</h1>
+ <p className="text-sm text-secondary-foreground" aria-live="polite">
+ {totalItems} {totalItems === 1 ? 'item' : 'items'}
+ </p>
</div>
</header>📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/frontend/src/components/Inbox/InboxPage.tsx (2 hunks)
- apps/frontend/src/components/atoms/Date.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/frontend/src/components/atoms/Date.tsx
🔇 Additional comments (3)
apps/frontend/src/components/Inbox/InboxPage.tsx (3)
4-4: LGTM: Import statement is correctly placed and utilized.
20-22: LGTM: Layout adjustments maintain component structure.
The reduced gap improves the component's visual density while maintaining the conditional width logic.
13-15: Verify consistent usage of the new store structure.
The state management has been refactored to access items through the inbox object. This structural change might affect other components using the store.
✅ Verification successful
Store structure is consistently used across components
Based on the search results, all components are correctly using the new store structure:
InboxPage.tsxusesconst { inbox } = useCycleItemStore()Date.tsxusesconst { today, overdue } = useCycleItemStore()TodayItems.tsxuses destructured access viaconst { items: todayItems } = todayThisWeekPage.tsxusesconst { items, currentItem } = useCycleItemStore()CustomKanban.tsxusesconst { items } = thisWeek
The pattern of accessing items through their respective containers (inbox, today, thisWeek, etc.) is consistently maintained across all components using the store.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of the new store structure across the codebase
# Test: Search for usages of useCycleItemStore to ensure consistent destructuring
rg -A 3 "useCycleItemStore\(\)" --type tsx --type ts
# Test: Search for direct item access patterns that might need updating
rg "const.*items.*=.*useCycleItemStore" --type tsx --type ts
Length of output: 173
Script:
#!/bin/bash
# Search for usages of useCycleItemStore to ensure consistent destructuring
rg -A 3 "useCycleItemStore"
# Search for direct item access patterns that might need updating
rg "items.*=.*useCycleItemStore"
# Find all TypeScript/React files that might use the store
fd "\.(tsx|ts)$"
Length of output: 15678
Summary by CodeRabbit
Release Notes
New Features
Inboxcomponent's layout for improved spacing.InboxAddItembutton label for clarity.ListFiltericon in theInboxPageheader for better organization.DatePickerwithDateCyclein theTodayPagefor improved date selection.Datecomponent.Bug Fixes
Style