Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contextual move #558

Merged
merged 24 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
2f33993
Added basen modal for moveinbox items
madhav-relish Nov 6, 2024
3be6c36
Move to space fucntionality added via a modal
madhav-relish Nov 6, 2024
e5d9f77
Added search feature for spaces and added a toast message for user
madhav-relish Nov 6, 2024
ccd2615
Resolved the warning
madhav-relish Nov 6, 2024
9d2419d
Fixed the input focus
madhav-relish Nov 6, 2024
143f92c
Fixed lints
madhav-relish Nov 6, 2024
2cb2b3d
Added user notification if error happens
madhav-relish Nov 6, 2024
b11b6ec
Resolved conflicts
madhav-relish Nov 13, 2024
7949e98
Updated the logic of move item
madhav-relish Nov 13, 2024
ab19615
Fixed the linting errors
madhav-relish Nov 13, 2024
9503ac6
Added move item to another item description
madhav-relish Nov 13, 2024
943295b
Added some styles for tiptap
madhav-relish Nov 13, 2024
13c73e0
Remove item from list when moved, removed logs and comments
madhav-relish Nov 13, 2024
38d476d
Improved modal styling
madhav-relish Nov 13, 2024
53a845a
Added saerch for items also
madhav-relish Nov 13, 2024
ae9c9b0
Fixed the css issue
madhav-relish Nov 13, 2024
13527eb
Added updation logic
madhav-relish Nov 13, 2024
44a1895
Fixed lnting errors
madhav-relish Nov 13, 2024
5d8b358
Added coderabbit suggestions
madhav-relish Nov 13, 2024
9b3e294
Fixed linting and build errors
madhav-relish Nov 13, 2024
2a9be52
Added better UI texts
madhav-relish Nov 13, 2024
cb44eb2
better messages
madhav-relish Nov 13, 2024
173b8a1
Merge branch 'preview' of https://github.com/marchhq/march into conte…
madhav-relish Nov 14, 2024
0fe8d1d
Added more error handling
madhav-relish Nov 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion apps/frontend/src/components/Inbox/InboxItems.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ import React, { useEffect, useCallback, useState } from "react"
import { CalendarIcon, MoveIcon, GithubIcon, MailsIcon } from "lucide-react"
import Image from "next/image"

import MoveInboxItem from "./MoveInboxItem"
import { RescheduleCalendar } from "./RescheduleCalendar/RescheduleCalendar"
import BoxIcon from "@/public/icons/box.svg"
import LinearIcon from "@/public/icons/linear.svg"
import { useAuth } from "@/src/contexts/AuthContext"
import { useModal } from "@/src/contexts/ModalProvider"
import { CycleItem } from "@/src/lib/@types/Items/Cycle"
import { useCycleItemStore } from "@/src/lib/store/cycle.store"
import classNames from "@/src/utils/classNames"
Expand All @@ -24,6 +26,7 @@ export const InboxItems: React.FC = () => {
)
const [date, setDate] = React.useState<Date | null>(new Date())
const [cycleDate, setCycleDate] = React.useState<Date | null>(new Date())
const { showModal } = useModal()
const { inbox, currentItem, setCurrentItem, fetchInbox, updateItem, error } =
useCycleItemStore()

Expand Down Expand Up @@ -227,7 +230,10 @@ export const InboxItems: React.FC = () => {
<MoveIcon
size={14}
className="hover-text"
onClick={(e) => e.stopPropagation()}
onClick={(e) => {
e.stopPropagation()
showModal(<MoveInboxItem inboxItemId={item._id} />)
}}
/>
</div>
</button>
Expand Down
102 changes: 102 additions & 0 deletions apps/frontend/src/components/Inbox/MoveInboxItem.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
"use client"

import React, { useEffect, useRef, useState } from "react"

import { DialogHeader, DialogTitle } from "../ui/dialog"
import { Input } from "../ui/input"
import { useAuth } from "@/src/contexts/AuthContext"
import { useModal } from "@/src/contexts/ModalProvider"
import { useToast } from "@/src/hooks/use-toast"
import { useCycleItemStore } from "@/src/lib/store/cycle.store"
import useSpaceStore from "@/src/lib/store/space.store"

type Props = {
inboxItemId: string
}

const MoveInboxItem = ({ inboxItemId }: Props) => {
const { session } = useAuth()
const { hideModal } = useModal()
const inputRef = useRef<HTMLInputElement>(null)
const [searchTerm, setSearchTerm] = useState("")
const { spaces, fetchSpaces } = useSpaceStore()
const { updateItem, fetchInbox } = useCycleItemStore()
const { toast } = useToast()

// 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()
}
}, [])
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add cleanup and handle potential race conditions.

The effects need some improvements for better reliability:

  1. 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.


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)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


// Filter spaces based on the search term
const filteredSpaces = spaces.filter((space) =>
space.name.toLowerCase().includes(searchTerm.toLowerCase())
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
const filteredSpaces = spaces.filter((space) =>
space.name.toLowerCase().includes(searchTerm.toLowerCase())
)
const filteredSpaces = (spaces || []).filter((space) =>
space.name.toLowerCase().includes(searchTerm.toLowerCase())
)


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>
</>
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

}

export default MoveInboxItem
Loading