Contextual move #558
Contextual move #558
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new React component, Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
|
Hey there! 👋 We require pull request titles to follow the Conventional Commits specification. Valid types:
Format: Error details: |
![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 (3)
apps/frontend/src/components/Inbox/MoveInboxItem.tsx (2)
17-25: Consider adding loading states and error boundaries.While the basic setup is good, the component could benefit from better error handling and loading states to improve user experience.
Consider adding:
- A loading state while spaces are being fetched
- An error boundary to gracefully handle rendering failures
const MoveInboxItem = ({ inboxItemId }: Props) => { const { session } = useAuth() const { hideModal } = useModal() const inputRef = useRef<HTMLInputElement>(null) const [searchTerm, setSearchTerm] = useState("") + const [isLoading, setIsLoading] = useState(false) const { spaces, fetchSpaces } = useSpaceStore() const { updateItem, fetchInbox } = useCycleItemStore() const { toast } = useToast()
52-55: Optimize search functionality.The current filtering logic could be enhanced for better performance and user experience:
+import { useMemo } from 'react' +import { matchSorter } from 'match-sorter' -const filteredSpaces = spaces.filter((space) => - space.name.toLowerCase().includes(searchTerm.toLowerCase()) -) +const filteredSpaces = useMemo(() => + matchSorter(spaces, searchTerm, { keys: ['name'] }), + [spaces, searchTerm] +)apps/frontend/src/components/Inbox/InboxItems.tsx (1)
233-236: Consider adding keyboard accessibility for the move action.The move functionality is currently only accessible via mouse click. Consider adding keyboard support for better accessibility.
<MoveIcon size={14} className="hover-text" + role="button" + tabIndex={0} onClick={(e) => { e.stopPropagation() showModal(<MoveInboxItem inboxItemId={item._id} />) }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + e.stopPropagation() + showModal(<MoveInboxItem inboxItemId={item._id} />) + } + }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/frontend/src/components/Inbox/InboxItems.tsx(3 hunks)apps/frontend/src/components/Inbox/MoveInboxItem.tsx(1 hunks)
🔇 Additional comments (5)
apps/frontend/src/components/Inbox/MoveInboxItem.tsx (1)
1-15: LGTM! Clean imports and type definitions.
The component setup follows React best practices with proper type definitions and necessary imports.
apps/frontend/src/components/Inbox/InboxItems.tsx (4)
8-8: LGTM: Import statements are properly structured.
The new imports for MoveInboxItem and useModal are correctly placed and follow project conventions.
Also applies to: 13-13
29-29: LGTM: Modal hook integration is clean.
The useModal hook is properly integrated alongside other hooks, following React conventions.
233-236: LGTM: Move icon handler implementation is correct.
The click handler correctly prevents event bubbling and shows the move modal with the appropriate item ID.
8-8: Verify MoveInboxItem implementation matches PR objectives.
Let's verify that the MoveInboxItem component implements all required features:
- Space selection modal
- Search functionality
- Confirmation toast
Also applies to: 233-236
✅ Verification successful
Let me check the actual implementation details to verify the features more thoroughly.
MoveInboxItem implementation successfully includes all required features
The component implementation includes all necessary features:
- Space selection with a searchable list of spaces
- Search functionality with real-time filtering
- Success/error toast notifications
- Modal integration with proper header and dialog components
- Proper error handling and state management
The code shows a complete implementation that matches the PR objectives, with:
- Search input that filters spaces in real-time
- Space selection through clickable buttons
- Toast notifications for both success and error cases
- Proper modal structure with DialogHeader and DialogTitle
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MoveInboxItem implementation includes all required features
# Test 1: Check for search input implementation
echo "Checking for search functionality..."
rg -l "search|filter|query" "apps/frontend/src/components/Inbox/MoveInboxItem.tsx"
# Test 2: Check for toast implementation
echo "Checking for toast notifications..."
rg -l "toast|notification" "apps/frontend/src/components/Inbox/MoveInboxItem.tsx"
# Test 3: Check for space selection implementation
echo "Checking for space selection..."
rg -l "spaces?|destination" "apps/frontend/src/components/Inbox/MoveInboxItem.tsx"
Length of output: 643
Script:
#!/bin/bash
# Get detailed implementation of search, toast, and space selection
echo "Checking search implementation details..."
rg "search|filter|query" -B 2 -A 2 "apps/frontend/src/components/Inbox/MoveInboxItem.tsx"
echo -e "\nChecking toast implementation details..."
rg "toast|notification" -B 2 -A 2 "apps/frontend/src/components/Inbox/MoveInboxItem.tsx"
echo -e "\nChecking space selection implementation details..."
rg "spaces?|destination" -B 2 -A 2 "apps/frontend/src/components/Inbox/MoveInboxItem.tsx"
# Check for modal component usage
echo -e "\nChecking modal implementation..."
rg "Modal|Dialog" -B 2 -A 2 "apps/frontend/src/components/Inbox/MoveInboxItem.tsx"
Length of output: 5393
| const handleSpaceClick = async (spaceId: string) => { | ||
| try { | ||
| await updateItem(session, { spaces: [spaceId] }, inboxItemId) | ||
| await fetchInbox(session) | ||
| toast({ title: "🚀 Moved successfully!" }) | ||
| hideModal() | ||
| } catch (error) { | ||
| toast({title: "Oops something seems wrong!", variant: "destructive"}) | ||
| console.error("Failed to move item:", error) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and user feedback.
The space movement handler needs improvements in error handling and user feedback:
const handleSpaceClick = async (spaceId: string) => {
+ setIsLoading(true)
try {
await updateItem(session, { spaces: [spaceId] }, inboxItemId)
await fetchInbox(session)
- toast({ title: "🚀 Moved successfully!" })
+ const space = spaces.find(s => s._id === spaceId)
+ toast({ title: `🚀 Moved to ${space?.name || 'space'} successfully!` })
hideModal()
} catch (error) {
- toast({title: "Oops something seems wrong!", variant: "destructive"})
- console.error("Failed to move item:", error)
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'
+ toast({
+ title: "Failed to move item",
+ description: errorMessage,
+ variant: "destructive"
+ })
+ console.error("Failed to move item:", errorMessage)
} finally {
+ setIsLoading(false)
}
}Committable suggestion skipped: line range outside the PR's diff.
| // Fetch spaces if they don't exist | ||
| useEffect(() => { | ||
| if (!spaces) { | ||
| void fetchSpaces(session) | ||
| } | ||
| }, [fetchSpaces, session, spaces]) | ||
|
|
||
| // Focus the input field when the modal opens | ||
| useEffect(() => { | ||
| if (inputRef.current) { | ||
| inputRef.current.focus() | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Add cleanup and handle potential race conditions.
The effects need some improvements for better reliability:
- Add cleanup to prevent memory leaks and race conditions:
useEffect(() => {
+ let mounted = true;
if (!spaces) {
- void fetchSpaces(session)
+ void fetchSpaces(session).then(() => {
+ if (!mounted) return;
+ })
}
+ return () => {
+ mounted = false;
+ }
}, [fetchSpaces, session, spaces])
useEffect(() => {
+ const currentRef = inputRef.current;
if (inputRef.current) {
inputRef.current.focus()
}
+ return () => {
+ if (currentRef) {
+ currentRef.blur();
+ }
+ }
}, [])Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <> | ||
| <DialogHeader className="hidden h-0 p-0"> | ||
| <DialogTitle className="hidden p-0"></DialogTitle> | ||
| </DialogHeader> | ||
| <div className="flex justify-between gap-2 px-4 pt-1 text-xs text-secondary-foreground"> | ||
| <span className="flex-1 truncate text-primary-foreground"> | ||
| <Input | ||
| ref={inputRef} | ||
| className="w-full border-none px-0 text-primary-foreground outline-none placeholder:text-secondary-foreground" | ||
| placeholder="Specify the target item: todo, note, event etc" | ||
| value={searchTerm} | ||
| onChange={(e) => setSearchTerm(e.target.value)} | ||
| /> | ||
| </span> | ||
| </div> | ||
| <div className="flex items-center gap-5 bg-transparent text-secondary-foreground"> | ||
| <div className="flex h-fit min-w-[350px] flex-col gap-5 overflow-hidden rounded-lg bg-background p-5 text-sm"> | ||
| <div className="flex max-h-96 flex-col gap-1.5 overflow-y-auto"> | ||
| {filteredSpaces.length > 0 ? ( | ||
| filteredSpaces.map((space) => ( | ||
| <button | ||
| key={space._id} | ||
| className="cursor-pointer text-left hover:text-primary-foreground" | ||
| onClick={() => handleSpaceClick(space._id)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter") { | ||
| handleSpaceClick(space._id) | ||
| } | ||
| }} | ||
| type="button" | ||
| > | ||
| {space.name} | ||
| </button> | ||
| )) | ||
| ) : ( | ||
| <div>No matching results found!</div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and loading states.
The UI implementation needs improvements in accessibility and loading feedback:
- <DialogHeader className="hidden h-0 p-0">
- <DialogTitle className="hidden p-0"></DialogTitle>
+ <DialogHeader className="sr-only">
+ <DialogTitle>Move Item to Space</DialogTitle>
</DialogHeader>
<div className="flex justify-between gap-2 px-4 pt-1 text-xs text-secondary-foreground">
<span className="flex-1 truncate text-primary-foreground">
<Input
ref={inputRef}
+ aria-label="Search spaces"
className="w-full border-none px-0 text-primary-foreground outline-none placeholder:text-secondary-foreground"
placeholder="Specify the target item: todo, note, event etc"
value={searchTerm}
onChange={(e) => setSearchTerm(e.target.value)}
+ disabled={isLoading}
/>
</span>
</div>
<div className="flex items-center gap-5 bg-transparent text-secondary-foreground">
<div className="flex h-fit min-w-[350px] flex-col gap-5 overflow-hidden rounded-lg bg-background p-5 text-sm">
<div className="flex max-h-96 flex-col gap-1.5 overflow-y-auto">
+ {isLoading && <div>Loading spaces...</div>}
{filteredSpaces.length > 0 ? (
filteredSpaces.map((space) => (
<button
key={space._id}
className="cursor-pointer text-left hover:text-primary-foreground"
onClick={() => handleSpaceClick(space._id)}
onKeyDown={(e) => {
if (e.key === "Enter") {
handleSpaceClick(space._id)
}
}}
type="button"
+ disabled={isLoading}
+ aria-label={`Move to ${space.name}`}
>
{space.name}
</button>Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/frontend/src/components/Inbox/MoveInboxItem.tsx(1 hunks)apps/frontend/src/components/atoms/ItemList.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/frontend/src/components/Inbox/MoveInboxItem.tsx
🔇 Additional comments (2)
apps/frontend/src/components/atoms/ItemList.tsx (2)
3-3: LGTM! Import statements are well-organized.
The new imports are correctly placed and follow the project's conventions.
Also applies to: 6-6, 10-10
60-60: LGTM! Hook usage follows React best practices.
The useModal hook is correctly implemented at the component's top level.
| <MoveIcon | ||
| size={14} | ||
| className="hover-text" | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| showModal(<MoveInboxItem inboxItemId={item._id} />) | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes and tooltip to MoveIcon.
While the implementation is functionally correct, consider these accessibility and UX improvements:
<MoveIcon
size={14}
- className="hover-text"
+ className="hover-text relative"
+ aria-label="Move item to different space"
+ role="button"
+ data-tooltip-id="item-actions"
+ data-tooltip-content="Move to space"
onClick={(e) => {
e.stopPropagation()
showModal(<MoveInboxItem inboxItemId={item._id} />)
}}
/>📝 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.
| <MoveIcon | |
| size={14} | |
| className="hover-text" | |
| onClick={(e) => { | |
| e.stopPropagation() | |
| showModal(<MoveInboxItem inboxItemId={item._id} />) | |
| }} | |
| /> | |
| <MoveIcon | |
| size={14} | |
| className="hover-text relative" | |
| aria-label="Move item to different space" | |
| role="button" | |
| data-tooltip-id="item-actions" | |
| data-tooltip-content="Move to space" | |
| onClick={(e) => { | |
| e.stopPropagation() | |
| showModal(<MoveInboxItem inboxItemId={item._id} />) | |
| }} | |
| /> |
| className="hover-text" | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| showModal(<MoveInboxItem inboxItemId={item._id} />) |
There was a problem hiding this comment.
💡 Codebase verification
MoveInboxItem component does not implement available spaces.
The component is missing the functionality to display available spaces as required by the PR objectives.
availableSpacesor related implementations are absent inapps/frontend/src/components/Inbox/MoveInboxItem.tsx.
🔗 Analysis chain
Verify MoveInboxItem component implementation.
Let's verify that the MoveInboxItem component exists and implements all required features from the PR objectives.
Locate MoveInboxItem component implementation across the repository.
Verify MoveInboxItem component implements available spaces and search functionality.
[/run_scripts]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MoveInboxItem component implementation
# Check if the component file exists
echo "Checking for MoveInboxItem component..."
fd "MoveInboxItem.tsx" src/components/Inbox
# Verify component implements required features
echo "Verifying component features..."
rg -A 10 "const MoveInboxItem" src/components/Inbox
# Check for search functionality
echo "Checking search implementation..."
rg -l "search|filter" src/components/Inbox/MoveInboxItem
Length of output: 639
Script:
#!/bin/bash
# Description: Locate MoveInboxItem component implementation
# Search for MoveInboxItem.tsx in the entire repository
fd "MoveInboxItem.tsx"
Length of output: 75
Script:
#!/bin/bash
# Description: Verify MoveInboxItem component implements required features
# Search for available spaces implementation
echo "Checking for available spaces implementation..."
rg "availableSpaces|spacesList|renderSpaces" apps/frontend/src/components/Inbox/MoveInboxItem.tsx
# Search for search functionality implementation
echo "Checking for search functionality implementation..."
rg "search|filter|useSearch" apps/frontend/src/components/Inbox/MoveInboxItem.tsx
Length of output: 730
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
apps/frontend/src/components/Inbox/MoveInboxItem.tsx (1)
97-100: Improve clarity of the error messageUpdate the error message in
console.errorfor better clarity and readability.Apply this diff to adjust the error message:
console.error( - "Error file moving item to another item description::", + "Error while moving item to another item's description:", error )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/frontend/src/components/Inbox/MoveInboxItem.tsx(1 hunks)apps/frontend/src/components/atoms/ItemList.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/frontend/src/components/atoms/ItemList.tsx
| description: ` | ||
| <p>${destinationItemDescription}</p> | ||
| <ul data-type="taskList"> | ||
| <li data-checked="false" data-type="taskItem"> | ||
| <label><input type="checkbox"><span></span></label> | ||
| <div data-indent="true"> | ||
| <p>${sourceItem[0].title}</p> | ||
| </div> | ||
| </li> | ||
| </ul> | ||
| <span data-indent="true">${sourceItem[0].description}</span> | ||
| `, |
There was a problem hiding this comment.
Sanitize user input in HTML strings to prevent XSS vulnerabilities
When constructing HTML content with user-provided data, ensure that inputs like destinationItemDescription, sourceItem[0].title, and sourceItem[0].description are properly sanitized to prevent XSS attacks.
Consider using a library or utility function to sanitize these inputs before including them in the HTML string.
| destiantionItemID: string, | ||
| destinationItemDescription: string |
There was a problem hiding this comment.
Fix typo in variable name 'destiantionItemID'
The variable destiantionItemID is misspelled. It should be destinationItemID to maintain consistency and avoid confusion.
Apply this diff to correct the typo:
- destiantionItemID: string,
+ destinationItemID: string,
destinationItemDescription: string📝 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.
| destiantionItemID: string, | |
| destinationItemDescription: string | |
| destinationItemID: string, | |
| destinationItemDescription: string |
| items, | ||
| isLoading: isInboxItemsLoading, | ||
| } = useCycleItemStore() | ||
| const [modalItems, setModalItems] = useState<CycleItem[]>(items) |
There was a problem hiding this comment.
Initialize 'modalItems' to an empty array if 'items' is undefined
If items is undefined initially, modalItems will also be undefined, leading to runtime errors when calling modalItems.filter.
Apply this diff to ensure modalItems is initialized properly:
- const [modalItems, setModalItems] = useState<CycleItem[]>(items)
+ const [modalItems, setModalItems] = useState<CycleItem[]>(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.
| const [modalItems, setModalItems] = useState<CycleItem[]>(items) | |
| const [modalItems, setModalItems] = useState<CycleItem[]>(items || []) |
| const filteredSpaces = spaces.filter((space) => | ||
| space.name.toLowerCase().includes(searchTerm.toLowerCase()) | ||
| ) |
There was a problem hiding this comment.
Handle undefined 'spaces' to prevent runtime errors
If spaces is undefined, calling spaces.filter will cause a runtime error. Initialize spaces to an empty array or guard against undefined.
Apply this diff to guard against undefined spaces:
- const filteredSpaces = spaces.filter((space) =>
+ const filteredSpaces = (spaces || []).filter((space) =>
space.name.toLowerCase().includes(searchTerm.toLowerCase())
)📝 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 filteredSpaces = spaces.filter((space) => | |
| space.name.toLowerCase().includes(searchTerm.toLowerCase()) | |
| ) | |
| const filteredSpaces = (spaces || []).filter((space) => | |
| space.name.toLowerCase().includes(searchTerm.toLowerCase()) | |
| ) |
| const sourceItem: CycleItem[] = items.filter( | ||
| (item) => item._id === inboxItemId | ||
| ) |
There was a problem hiding this comment.
Ensure 'sourceItem' is not empty before accessing 'sourceItem[0]'
To prevent potential runtime errors, verify that sourceItem contains at least one item before accessing sourceItem[0].
Apply this diff to add a check for sourceItem length:
const sourceItem: CycleItem[] = items.filter(
(item) => item._id === inboxItemId
)
+ if (sourceItem.length === 0) {
+ throw new Error('Source item not found');
+ }📝 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 sourceItem: CycleItem[] = items.filter( | |
| (item) => item._id === inboxItemId | |
| ) | |
| const sourceItem: CycleItem[] = items.filter( | |
| (item) => item._id === inboxItemId | |
| ) | |
| if (sourceItem.length === 0) { | |
| throw new Error('Source item not found'); | |
| } |
| <div className="flex max-h-48 flex-col gap-1.5 overflow-y-auto"> | ||
| {isInboxItemsLoading && <div>Loading Inbox Items...</div>} | ||
| {filteredItems.length > 0 ? ( | ||
| filteredItems?.map((item) => ( | ||
| <button | ||
| key={item._id} | ||
| className="cursor-pointer text-left hover:text-primary-foreground" | ||
| onClick={() => handleItemClick(item._id, item.description)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter") { | ||
| handleItemClick(item._id, item.description) | ||
| } | ||
| }} | ||
| type="button" | ||
| disabled={isInboxItemsLoading} | ||
| aria-label={`Move to ${item.title}`} | ||
| > | ||
| {item.title} | ||
| </button> | ||
| )) | ||
| ) : ( | ||
| <div>No matching results found!</div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent 'No matching results found!' message during loading
Ensure that the 'No matching results found!' message is displayed only after loading is complete and no results are found, to avoid confusing the user.
Apply this diff to adjust the conditional rendering:
{isInboxItemsLoading && <div>Loading Inbox Items...</div>}
-{filteredItems.length > 0 ? (
+{!isInboxItemsLoading && filteredItems.length > 0 ? (
filteredItems.map((item) => (
// ...
))
) : (
- <div>No matching results found!</div>
+ !isInboxItemsLoading && <div>No matching results found!</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="flex max-h-48 flex-col gap-1.5 overflow-y-auto"> | |
| {isInboxItemsLoading && <div>Loading Inbox Items...</div>} | |
| {filteredItems.length > 0 ? ( | |
| filteredItems?.map((item) => ( | |
| <button | |
| key={item._id} | |
| className="cursor-pointer text-left hover:text-primary-foreground" | |
| onClick={() => handleItemClick(item._id, item.description)} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter") { | |
| handleItemClick(item._id, item.description) | |
| } | |
| }} | |
| type="button" | |
| disabled={isInboxItemsLoading} | |
| aria-label={`Move to ${item.title}`} | |
| > | |
| {item.title} | |
| </button> | |
| )) | |
| ) : ( | |
| <div>No matching results found!</div> | |
| )} | |
| <div className="flex max-h-48 flex-col gap-1.5 overflow-y-auto"> | |
| {isInboxItemsLoading && <div>Loading Inbox Items...</div>} | |
| {!isInboxItemsLoading && filteredItems.length > 0 ? ( | |
| filteredItems?.map((item) => ( | |
| <button | |
| key={item._id} | |
| className="cursor-pointer text-left hover:text-primary-foreground" | |
| onClick={() => handleItemClick(item._id, item.description)} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter") { | |
| handleItemClick(item._id, item.description) | |
| } | |
| }} | |
| type="button" | |
| disabled={isInboxItemsLoading} | |
| aria-label={`Move to ${item.title}`} | |
| > | |
| {item.title} | |
| </button> | |
| )) | |
| ) : ( | |
| !isInboxItemsLoading && <div>No matching results found!</div> | |
| )} |
| {isSpaceLoading && <div>Loading spaces...</div>} | ||
| {filteredSpaces.length > 0 ? ( | ||
| filteredSpaces.map((space) => ( | ||
| <button | ||
| key={space._id} | ||
| className="cursor-pointer text-left hover:text-primary-foreground" | ||
| onClick={() => handleSpaceClick(space._id)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter") { | ||
| handleSpaceClick(space._id) | ||
| } | ||
| }} | ||
| disabled={isSpaceLoading} | ||
| aria-label={`Move to ${space.name}`} | ||
| type="button" | ||
| > | ||
| {space.name} | ||
| </button> | ||
| )) | ||
| ) : ( | ||
| <div>No matching results found!</div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent 'No matching results found!' message during loading
Similarly, ensure that the 'No matching results found!' message for spaces is displayed only after loading is complete.
Apply this diff to adjust the conditional rendering:
{isSpaceLoading && <div>Loading spaces...</div>}
-{filteredSpaces.length > 0 ? (
+{!isSpaceLoading && filteredSpaces.length > 0 ? (
filteredSpaces.map((space) => (
// ...
))
) : (
- <div>No matching results found!</div>
+ !isSpaceLoading && <div>No matching results found!</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.
| {isSpaceLoading && <div>Loading spaces...</div>} | |
| {filteredSpaces.length > 0 ? ( | |
| filteredSpaces.map((space) => ( | |
| <button | |
| key={space._id} | |
| className="cursor-pointer text-left hover:text-primary-foreground" | |
| onClick={() => handleSpaceClick(space._id)} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter") { | |
| handleSpaceClick(space._id) | |
| } | |
| }} | |
| disabled={isSpaceLoading} | |
| aria-label={`Move to ${space.name}`} | |
| type="button" | |
| > | |
| {space.name} | |
| </button> | |
| )) | |
| ) : ( | |
| <div>No matching results found!</div> | |
| )} | |
| {isSpaceLoading && <div>Loading spaces...</div>} | |
| {!isSpaceLoading && filteredSpaces.length > 0 ? ( | |
| filteredSpaces.map((space) => ( | |
| <button | |
| key={space._id} | |
| className="cursor-pointer text-left hover:text-primary-foreground" | |
| onClick={() => handleSpaceClick(space._id)} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter") { | |
| handleSpaceClick(space._id) | |
| } | |
| }} | |
| disabled={isSpaceLoading} | |
| aria-label={`Move to ${space.name}`} | |
| type="button" | |
| > | |
| {space.name} | |
| </button> | |
| )) | |
| ) : ( | |
| !isSpaceLoading && <div>No matching results found!</div> | |
| )} |
What did you ship?
Fixes:
Checklist:
Screencast.From.2024-11-06.21-27-21.mp4