feat: add ability to re-assign tasks #143
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,117 @@ | ||||||
| import { TasksApi } from '@/api/tasks/tasks.api' | ||||||
| import { TUser } from '@/api/users/users.types' | ||||||
| import { useMutation, useQueryClient } from '@tanstack/react-query' | ||||||
| import { UserSearchIcon } from 'lucide-react' | ||||||
| import { ReactNode, useState } from 'react' | ||||||
| import { toast } from 'sonner' | ||||||
| import { TeamUserSearchDropdown } from './team-user-search-dropdown' | ||||||
| import { | ||||||
| AlertDialog, | ||||||
| AlertDialogAction, | ||||||
| AlertDialogCancel, | ||||||
| AlertDialogContent, | ||||||
| AlertDialogFooter, | ||||||
| AlertDialogHeader, | ||||||
| AlertDialogTitle, | ||||||
| AlertDialogTrigger, | ||||||
| } from './ui/alert-dialog' | ||||||
| import { Button } from './ui/button' | ||||||
| import { Tooltip, TooltipContent, TooltipTrigger } from './ui/tooltip' | ||||||
|
|
||||||
| type ReassignUserModalProps = { | ||||||
| taskId: string | ||||||
| teamId: string | ||||||
| open: boolean | ||||||
| children: ReactNode | ||||||
| onOpenChange: (open: boolean) => void | ||||||
| } | ||||||
|
|
||||||
| const ReassignUserModal = ({ | ||||||
| open, | ||||||
| taskId, | ||||||
| teamId, | ||||||
| children, | ||||||
| onOpenChange, | ||||||
| }: ReassignUserModalProps) => { | ||||||
| const queryClient = useQueryClient() | ||||||
| const [selectedUser, setSelectedUser] = useState<TUser | null>(null) | ||||||
|
|
||||||
| const reassignTaskMutation = useMutation({ | ||||||
| mutationFn: TasksApi.reassignTask.fn, | ||||||
| onSuccess: () => { | ||||||
| toast.success(`Task assigned to ${selectedUser?.name}`) | ||||||
| void queryClient.invalidateQueries({ queryKey: TasksApi.getTasks.key(teamId) }) | ||||||
|
There was a problem hiding this comment. Overly Broad Cache Invalidation
Tell me moreWhat is the issue?Invalidating the entire task query cache when only a single task is modified is inefficient. Why this mattersThis causes unnecessary refetching of all tasks when only one task's assignment has changed, potentially causing performance issues with large task lists. Suggested change ∙ Feature PreviewUse more specific cache updates by either invalidating just the specific task or using cache mutation: queryClient.setQueryData([TasksApi.getTasks.key(teamId)], (old: any) => {
// Update only the specific task in the cache
return old.map((task: any) =>
task.id === taskId ? { ...task, executor_id: selectedUser.id } : task
);
});Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||
| onOpenChange(false) | ||||||
| setSelectedUser(null) | ||||||
| }, | ||||||
| onError: () => { | ||||||
| toast.error('Failed to reassign task, please try again') | ||||||
| }, | ||||||
|
Comment on lines
+47
to
+49
There was a problem hiding this comment. Loss of Error Context in Mutation Handler
Tell me moreWhat is the issue?The error handler for the reassignTask mutation loses the actual error information by only showing a generic message. Why this mattersWithout the actual error details, debugging production issues becomes more difficult as developers won't know the root cause of the failure. Suggested change ∙ Feature PreviewInclude the error information in the error handler: onError: (error) => {
console.error('Task reassignment failed:', error);
toast.error(`Failed to reassign task: ${error.message || 'Please try again'}`);
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||
| }) | ||||||
|
|
||||||
| const handleReassign = () => { | ||||||
| if (!selectedUser) return | ||||||
|
|
||||||
| debugger | ||||||
|
There was a problem hiding this comment. Remove debugger statement before merging. Debugger statements should not be committed to production code. Apply this diff to remove the debugger statement: - debugger
reassignTaskMutation.mutate({📝 Committable suggestion
Suggested change
🧰 Tools🪛 Biome (1.9.4)[error] 55-55: This is an unexpected use of the debugger statement. Unsafe fix: Remove debugger statement (lint/suspicious/noDebugger) 🤖 Prompt for AI AgentsThere was a problem hiding this comment. Debugger Statement in Production Code
Tell me moreWhat is the issue?There is a debugger statement left in the production code in the handleReassign function. Why this mattersThis will cause the application to pause execution when the developer tools are open, disrupting the normal flow of the application in production environments. Suggested change ∙ Feature PreviewRemove the debugger statement: const handleReassign = () => {
if (!selectedUser) return
reassignTaskMutation.mutate({
task_id: taskId,
executor_id: selectedUser.id,
})
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||
| reassignTaskMutation.mutate({ | ||||||
| task_id: taskId, | ||||||
| executor_id: selectedUser.id, | ||||||
| }) | ||||||
| } | ||||||
|
Comment on lines
+52
to
+60
There was a problem hiding this comment. Development Artifact in Production Code
Tell me moreWhat is the issue?The handleReassign function contains a debugger statement which should not be in production code. Why this mattersLeaving debugger statements in production code can cause unexpected pauses in execution when developer tools are open and makes the code less professional. Suggested change ∙ Feature PreviewRemove the debugger statement: const handleReassign = () => {
if (!selectedUser) return
reassignTaskMutation.mutate({
task_id: taskId,
executor_id: selectedUser.id,
})
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||
|
|
||||||
| return ( | ||||||
| <AlertDialog open={open} onOpenChange={onOpenChange}> | ||||||
| <AlertDialogTrigger asChild>{children}</AlertDialogTrigger> | ||||||
|
|
||||||
| <AlertDialogContent> | ||||||
| <AlertDialogHeader> | ||||||
| <AlertDialogTitle>Reassign user</AlertDialogTitle> | ||||||
| </AlertDialogHeader> | ||||||
|
|
||||||
| <TeamUserSearchDropdown | ||||||
| teamId={teamId} | ||||||
| value={selectedUser?.id} | ||||||
| placeholder="Search user" | ||||||
| onUserSelect={setSelectedUser} | ||||||
| /> | ||||||
|
|
||||||
| <AlertDialogFooter> | ||||||
| <AlertDialogCancel disabled={reassignTaskMutation.isPending}>Cancel</AlertDialogCancel> | ||||||
| <AlertDialogAction | ||||||
| onClick={handleReassign} | ||||||
| disabled={!selectedUser || reassignTaskMutation.isPending} | ||||||
| > | ||||||
| {reassignTaskMutation.isPending ? 'Reassigning...' : 'Reassign'} | ||||||
| </AlertDialogAction> | ||||||
| </AlertDialogFooter> | ||||||
| </AlertDialogContent> | ||||||
| </AlertDialog> | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| type ReassignUserProps = { | ||||||
| taskId: string | ||||||
| teamId: string | ||||||
| } | ||||||
|
|
||||||
| export const ReassignUser = ({ taskId, teamId }: ReassignUserProps) => { | ||||||
| const [showModal, setShowModal] = useState(false) | ||||||
|
|
||||||
| return ( | ||||||
| <ReassignUserModal open={showModal} onOpenChange={setShowModal} taskId={taskId} teamId={teamId}> | ||||||
| <Tooltip> | ||||||
| <TooltipTrigger asChild> | ||||||
| <Button | ||||||
| size="icon" | ||||||
| variant="ghost" | ||||||
| onClick={() => setShowModal(true)} | ||||||
|
There was a problem hiding this comment. Inline Handler Recreation
Tell me moreWhat is the issue?The click handler is recreated on every render due to the inline arrow function. Why this mattersCreating new function references on each render causes unnecessary memory allocations and potential re-renders of child components that rely on prop referential equality. Suggested change ∙ Feature PreviewExtract the click handler to a memoized function using useCallback: const handleClick = useCallback(() => setShowModal(true), []);
// Then use it as:
<Button onClick={handleClick} ... >Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||
| className="cursor-pointer hover:bg-gray-200 hover:text-gray-800 active:bg-gray-300 active:text-gray-900" | ||||||
|
There was a problem hiding this comment. Long inline Tailwind classes
Tell me moreWhat is the issue?Long inline Tailwind CSS classes reduce code readability. Why this mattersMultiple inline Tailwind classes make the component markup harder to scan and understand. These styles could be abstracted into a reusable class or component style. Suggested change ∙ Feature PreviewExtract the styles into a dedicated CSS class or component style: const buttonStyles = "cursor-pointer hover:bg-gray-200 hover:text-gray-800 active:bg-gray-300 active:text-gray-900"
<Button className={buttonStyles} ... />Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||
| > | ||||||
| <UserSearchIcon className="h-5 w-5" /> | ||||||
| </Button> | ||||||
| </TooltipTrigger> | ||||||
| <TooltipContent>Reassign user</TooltipContent> | ||||||
| </Tooltip> | ||||||
| </ReassignUserModal> | ||||||
| ) | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import { TeamsApi } from '@/api/teams/teams.api' | ||
| import { TUser } from '@/api/users/users.types' | ||
| import { cn } from '@/lib/utils' | ||
| import { useQuery } from '@tanstack/react-query' | ||
| import { ChevronDown, User } from 'lucide-react' | ||
| import { useState } from 'react' | ||
| import { Button } from './ui/button' | ||
| import { | ||
| Command, | ||
| CommandEmpty, | ||
| CommandGroup, | ||
| CommandInput, | ||
| CommandItem, | ||
| CommandList, | ||
| } from './ui/command' | ||
| import { Popover, PopoverContent, PopoverTrigger } from './ui/popover' | ||
|
|
||
| type TeamUserSearchDropdownProps = { | ||
| teamId: string | ||
| value?: string | ||
| placeholder?: string | ||
| onUserSelect: (user: TUser) => void | ||
| } | ||
|
|
||
| export const TeamUserSearchDropdown = ({ | ||
| teamId, | ||
| value, | ||
| placeholder = 'Search user', | ||
| onUserSelect, | ||
| }: TeamUserSearchDropdownProps) => { | ||
| const [open, setOpen] = useState(false) | ||
| const [search, setSearch] = useState('') | ||
|
|
||
| const { data: usersList, isLoading } = useQuery({ | ||
| queryKey: TeamsApi.getTeamById.key({ teamId, member: true }), | ||
| queryFn: () => TeamsApi.getTeamById.fn({ teamId, member: true }), | ||
| select: (res) => res.users, | ||
| }) | ||
|
|
||
| const selectedUser = usersList?.find((user) => user.id === value) | ||
|
|
||
| const handleSelect = (user: TUser) => { | ||
| onUserSelect(user) | ||
| setOpen(false) | ||
| setSearch('') | ||
| } | ||
|
|
||
| return ( | ||
| <Popover open={open} onOpenChange={setOpen}> | ||
| <PopoverTrigger asChild> | ||
| <Button | ||
| variant="outline" | ||
| role="combobox" | ||
| aria-expanded={open} | ||
| className={cn( | ||
| 'w-full justify-between font-normal', | ||
| !selectedUser && 'text-muted-foreground', | ||
| )} | ||
| > | ||
| {selectedUser?.name ?? placeholder} | ||
| <ChevronDown className="opacity-50" /> | ||
| </Button> | ||
| </PopoverTrigger> | ||
|
|
||
| <PopoverContent | ||
| align="start" | ||
| className="p-0" | ||
| style={{ width: 'var(--radix-popover-trigger-width)' }} | ||
| > | ||
| <Command> | ||
| <CommandInput placeholder={placeholder} value={search} onValueChange={setSearch} /> | ||
|
|
||
| <CommandList> | ||
| <CommandEmpty>{isLoading ? 'Searching users...' : 'No users found.'}</CommandEmpty> | ||
|
|
||
| <CommandGroup> | ||
| {usersList?.map((user) => ( | ||
| <CommandItem | ||
| key={user.id} | ||
| value={user.name} | ||
| onSelect={() => handleSelect(user)} | ||
| className="flex items-center gap-2" | ||
| > | ||
| <User className="text-muted-foreground h-4 w-4" /> | ||
| <div className="flex-1"> | ||
| <div className="font-medium">{user.name}</div> | ||
| </div> | ||
| </CommandItem> | ||
| ))} | ||
| </CommandGroup> | ||
| </CommandList> | ||
| </Command> | ||
| </PopoverContent> | ||
| </Popover> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,7 +94,7 @@ export const TeamsLayoutHeader = ({ teamId }: TeamsLayoutHeaderProps) => { | |
|
|
||
| <Button size="sm" onClick={() => setIsAddMembersModalOpen(true)}> | ||
| <UserRoundPlusIcon /> | ||
| Add members | ||
| </Button> | ||
|
|
||
| <AlertDialog open={isAddMembersModalOpen} onOpenChange={handleCloseModal}> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent naming convention usage
Tell me more
What is the issue?
The codebase uses both camelCase (taskId) and snake_case (task_id) naming conventions for properties.
Why this matters
Mixed naming conventions across the codebase reduce readability and make it harder to predict property names. TypeScript conventionally uses camelCase.
Suggested change ∙ Feature Preview
Standardize on camelCase throughout:
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.