Skip to content

Conversation

@saeedvaziry
Copy link
Member

No description provided.

@saeedvaziry saeedvaziry requested a review from Copilot November 2, 2025 19:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the server selection component to use infinite scroll pagination with lazy loading, similar to the project select component. The key change is migrating from a static server list (project_servers in shared data) to a dynamic, paginated API-based approach.

  • Converts ServerSelect from useQuery to useInfiniteQuery for pagination support
  • Removes project_servers from shared data and PHP middleware
  • Adds GetServers action class to centralize server fetching logic with pagination
  • Updates ServerSwitch component to use the refactored ServerSelect

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
resources/js/types/index.d.ts Removes project_servers from SharedData interface
resources/js/pages/servers/components/server-select.tsx Refactors to use infinite scroll with pagination, debouncing, and IntersectionObserver
resources/js/components/server-switch.tsx Updates to use new ServerSelect API with controlled state and custom trigger/footer
resources/js/components/project-switch.tsx Removes unused refetch logic that's no longer needed
resources/js/components/project-select.tsx Cleans up unused refetch callback props and improves getNextPageParam safety
app/Http/Middleware/HandleInertiaRequests.php Removes eager loading of all project servers from shared data
app/Http/Controllers/ServerController.php Delegates server fetching to new GetServers action class
app/Actions/Server/GetServers.php New action class implementing paginated server search logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

observer.disconnect();
}
};
}, [open, hasNextPage, isFetchingNextPage, fetchNextPage, debouncedQuery, servers.length]);
Copy link

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

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

The useEffect dependency array includes debouncedQuery and servers.length which will cause the IntersectionObserver to be recreated on every query change or when servers load. This differs from the project-select.tsx implementation (line 96) which uses query and projects.length. For consistency and to avoid unnecessary observer recreation, consider using query instead of debouncedQuery in the dependency array.

Suggested change
}, [open, hasNextPage, isFetchingNextPage, fetchNextPage, debouncedQuery, servers.length]);
}, [open, hasNextPage, isFetchingNextPage, fetchNextPage, query, servers.length]);

Copilot uses AI. Check for mistakes.
<PopoverTrigger asChild>{trigger || defaultTrigger}</PopoverTrigger>
<PopoverContent className="flex max-h-[400px] w-56 flex-col p-0" align="start">
<Command shouldFilter={false} className="flex flex-col overflow-hidden">
<CommandInput placeholder="Search server..." value={query} onValueChange={setQuery} />
Copy link

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

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

The onWheel handler with stopPropagation() is added but lacks a comment explaining why it's needed. This pattern doesn't exist in the similar project-select.tsx component. Consider adding a comment to explain the purpose of preventing wheel event propagation, or remove it if it's not necessary.

Suggested change
<CommandInput placeholder="Search server..." value={query} onValueChange={setQuery} />
<CommandInput placeholder="Search server..." value={query} onValueChange={setQuery} />
{/* Prevent wheel events from propagating to parent elements (e.g., Popover), which could cause unwanted scrolling of the underlying page while scrolling the server list. */}

Copilot uses AI. Check for mistakes.
className="gap-0"
>
<div className="flex items-center">
<PlusIcon size={5} />
Copy link

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

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

The size prop value 5 appears to be incorrect. The PlusIcon from lucide-react expects pixel values or uses CSS classes for sizing. In the original code (removed DropdownMenuItem), this was also size={5}. This should likely be size={16} or use the className prop with Tailwind classes like className=\"size-4\" for consistency with other icons in the codebase (see line 151 in server-select.tsx which uses className=\"ml-2 size-4 shrink-0 opacity-50\").

Suggested change
<PlusIcon size={5} />
<PlusIcon className="size-4" />

Copilot uses AI. Check for mistakes.
<span className="hidden lg:flex">Select a server</span>
</>
)}
<ChevronsUpDownIcon size={5} />
Copy link

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

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

The size prop value 5 appears to be incorrect. The ChevronsUpDownIcon from lucide-react expects pixel values or uses CSS classes for sizing. This should likely be size={16} or use the className prop with Tailwind classes like className=\"size-4\" for consistency with other icons in the codebase (see line 151 in server-select.tsx which uses className=\"ml-2 size-4 shrink-0 opacity-50\").

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@saeedvaziry saeedvaziry merged commit e04518b into 3.x Nov 2, 2025
3 checks passed
@saeedvaziry saeedvaziry deleted the server-select branch November 2, 2025 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants