fix: removed loader from inbox #420
Conversation
WalkthroughThe changes in this pull request involve modifications to 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: 1
🧹 Outside diff range and nitpick comments (3)
apps/frontend/src/components/Inbox/InboxItems.tsx (2)
Line range hint
128-134: Remove or properly disable the delete functionality.Having a non-functional delete button in the menu can confuse users. Either:
- Remove the delete option entirely from the menu
- Disable it visually and add a tooltip explaining why
- Re-implement the delete functionality if it's still needed
If you're temporarily disabling delete, consider this implementation:
{ name: "Delete", icon: "weui:delete-outlined", color: "#C45205", - // onClick: () => handleDelete(item._id!), + disabled: true, + tooltip: "Delete functionality temporarily unavailable" },
Line range hint
52-71: Improve status handling and error management.The current implementation has several potential issues:
- Using "null" as a status is not semantically clear
- No error handling for failed optimistic updates
- Hardcoded animation duration
Consider this improved implementation:
const handleDone = useCallback( (event: React.MouseEvent, id: string, currentStatus: string | undefined) => { event.stopPropagation() if (id) { - const newStatus = currentStatus === "done" ? "null" : "done" + const newStatus = currentStatus === "done" ? "pending" : "done" setAnimatingItems((prev) => new Set(prev).add(id)) setOptimisticDoneItems((prev) => { const newSet = new Set(prev) if (newStatus === "done") { newSet.add(id) } else { newSet.delete(id) } return newSet }) + const ANIMATION_DURATION = 400 // ms setTimeout(async () => { - updateItem(session, { status: newStatus }, id) + try { + await updateItem(session, { status: newStatus }, id) + } catch (error) { + // Revert optimistic update on failure + setOptimisticDoneItems((prev) => { + const newSet = new Set(prev) + if (currentStatus === "done") { + newSet.add(id) + } else { + newSet.delete(id) + } + return newSet + }) + console.error("Failed to update item status:", error) + } setAnimatingItems((prev) => { const newSet = new Set(prev) newSet.delete(id) return newSet }) - }, 400) + }, ANIMATION_DURATION) } }, [updateItem, session] )apps/frontend/src/lib/store/cycle.store.ts (1)
Line range hint
292-379: Consider improving the update mechanism.The current implementation has several areas that could be enhanced:
- The
updateItemsInViewfunction is duplicated - once for optimistic updates and once for server response updates.- There's a potential race condition where a slow server response could override more recent optimistic updates.
- Failed updates don't rollback the optimistic changes.
Consider refactoring like this:
interface UpdateState { previousState: ExtendedCycleItemStore; version: number; } // Store the previous state and version for potential rollback const updateState = (updates: Partial<CycleItem>, id: string, version: number) => { set((state) => { // Store current state for potential rollback previousStates.set(version, state); const updateItemsInView = (items: CycleItem[], isOverdue = false) => { if (isOverdue && updates.status === "done") { return items.filter((item) => item._id !== id); } return items.map((item) => item._id === id ? { ...item, ...updates } : item ); }; return { inbox: { ...state.inbox, items: updateItemsInView(state.inbox.items) }, today: { ...state.today, items: updateItemsInView(state.today.items) }, overdue: { ...state.overdue, items: updateItemsInView(state.overdue.items, true), }, thisWeek: { ...state.thisWeek, items: updateItemsInView(state.thisWeek.items), }, items: updateItemsInView(state.items), currentItem: state.currentItem?._id === id ? { ...state.currentItem, ...updates } : state.currentItem, }; }); }; // Rollback to previous state if update fails const rollback = (version: number) => { const previousState = previousStates.get(version); if (previousState) { set(previousState); previousStates.delete(version); } };This approach:
- Eliminates code duplication
- Provides rollback capability
- Tracks versions to handle race conditions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/frontend/src/components/Inbox/InboxItems.tsx (1 hunks)
- apps/frontend/src/lib/store/cycle.store.ts (1 hunks)
🔇 Additional comments (3)
apps/frontend/src/components/Inbox/InboxItems.tsx (1)
Line range hint
136-136: Verify the filtering logic with optimistic updates.The current implementation filters out items with status "done", but the optimistic updates might cause inconsistencies in the UI. We should ensure that items are properly removed from view when marked as done.
Let's verify the component's behavior:
apps/frontend/src/lib/store/cycle.store.ts (2)
285-285: LGTM: Improved error handling.The addition of
throw errorafter setting the error state is a good practice. It allows both local state updates and error propagation to calling components for additional error handling if needed.
Line range hint
1-379: Verify alignment with PR objective.The PR's objective is to "remove loader from inbox", but the changes include significant refactoring of error handling and item management logic. While these changes appear to be improvements, please clarify if they are intentional and related to the loader removal.
Let's check for any loader-related changes in the codebase:
| /* if (isLoading) { | ||
| return ( | ||
| <div className="flex flex-col gap-4"> | ||
| <p>loading...</p> | ||
| </div> | ||
| ) | ||
| } | ||
| } */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing a better loading state pattern.
While removing the basic loading text improves the UI, we should still provide users with feedback during data fetching. Consider implementing a skeleton loader or a subtle loading indicator to maintain good UX.
Here's a suggested implementation using a skeleton loader:
+ import { Skeleton } from "@/components/ui/skeleton"
+
+ if (isLoading) {
+ return (
+ <div className="flex flex-col gap-4">
+ {[...Array(3)].map((_, i) => (
+ <div key={i} className="flex items-center space-x-4">
+ <Skeleton className="h-4 w-4 rounded-full" />
+ <div className="space-y-2">
+ <Skeleton className="h-4 w-[250px]" />
+ <Skeleton className="h-4 w-[200px]" />
+ </div>
+ </div>
+ ))}
+ </div>
+ )
+ }Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor