-
Notifications
You must be signed in to change notification settings - Fork 14
fix: enable cluster shard/node switching and improve dropdown usability #253
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,9 +19,11 @@ type AppHeaderProps = { | |
| function AppHeader({ title, icon, className }: AppHeaderProps) { | ||
| const [isOpen, setIsOpen] = useState(false) | ||
| const dropdownRef = useRef<HTMLDivElement>(null) | ||
| const badgeRef = useRef<HTMLDivElement>(null) | ||
| const navigate = useNavigate() | ||
| const { id, clusterId } = useParams<{ id: string; clusterId: string }>() | ||
| const { host, port, username, alias } = useSelector(selectConnectionDetails(id!)) | ||
| const connectionDetails = useSelector(selectConnectionDetails(id!)) ?? {} as Partial<ReturnType<ReturnType<typeof selectConnectionDetails>>> | ||
| const { host, port, username, alias } = connectionDetails as { host?: string; port?: string; username?: string; alias?: string } | ||
| const clusterData = useSelector(selectCluster(clusterId!)) | ||
| const ToggleIcon = isOpen ? CircleChevronUp : CircleChevronDown | ||
|
|
||
|
|
@@ -34,6 +36,14 @@ function AppHeader({ title, icon, className }: AppHeaderProps) { | |
| state.valkeyConnection?.connections, | ||
| ) | ||
|
|
||
| // For cluster mode, consider connected if any node in the cluster is connected | ||
| const isClusterConnected = clusterId | ||
| ? Object.values(allConnections ?? {}).some( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's move the nullish coalescing operator higher where (line 35) where allConnections is initialized using useSelector |
||
| (conn) => conn.connectionDetails.clusterId === clusterId && conn.status === CONNECTED, | ||
| ) | ||
| : isConnected | ||
| const effectiveConnected = isConnected || isClusterConnected | ||
|
|
||
| const handleNavigate = (primaryKey: string) => { | ||
| navigate(`/${clusterId}/${primaryKey}/dashboard`) | ||
| setIsOpen(false) | ||
|
|
@@ -42,7 +52,10 @@ function AppHeader({ title, icon, className }: AppHeaderProps) { | |
| // for closing the dropdown when we click anywhere in screen | ||
| useEffect(() => { | ||
| const handleClickOutside = (event: MouseEvent) => { | ||
| if (dropdownRef.current && !dropdownRef.current.contains(event.target as Node)) { | ||
| if ( | ||
| dropdownRef.current && !dropdownRef.current.contains(event.target as Node) && | ||
| badgeRef.current && !badgeRef.current.contains(event.target as Node) | ||
| ) { | ||
| setIsOpen(false) | ||
| } | ||
| } | ||
|
|
@@ -75,41 +88,43 @@ function AppHeader({ title, icon, className }: AppHeaderProps) { | |
| </Typography> | ||
| <div> | ||
| <Badge | ||
| className="h-5 w-auto text-nowrap px-2 py-4 flex items-center gap-2 justify-between cursor-pointer" | ||
| className={cn( | ||
| "h-5 w-auto text-nowrap px-2 py-4 flex items-center gap-2 justify-between", | ||
| effectiveConnected ? "cursor-pointer" : "cursor-not-allowed", | ||
| )} | ||
| onClick={() => effectiveConnected && setIsOpen(!isOpen)} | ||
| ref={badgeRef} | ||
| variant="default" | ||
| > | ||
| <div className="flex flex-col gap-1"> | ||
| <Typography | ||
| className="flex items-center" | ||
| variant="bodySm" | ||
| > | ||
| <Dot className={isConnected ? "text-green-500" : "text-gray-400"} size={45} /> | ||
| <Dot className={effectiveConnected ? "text-green-500" : "text-gray-400"} size={45} /> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this change the semantic of the Dot? I.e. it used to mean that "you, user, are connected to this node in the cluster" but now it means "you, user, are connected to SOME node in this cluster" — if so, this is not a desired outcome — the users connecting from Electron app will hate it because they cannot even physically connect to all nodes, they actually need to see what instances (of 12 max) they are connected to. Instead of "connected", we can think of it as "have active glide client + ws connections". |
||
| {id} | ||
| </Typography> | ||
| </div> | ||
| <button aria-label="Toggle dropdown" disabled={!isConnected} onClick={() => isConnected && setIsOpen(!isOpen)}> | ||
| <ToggleIcon | ||
| className={isConnected | ||
| ? "text-primary cursor-pointer hover:text-primary/80" | ||
| : "text-gray-400 cursor-not-allowed" | ||
| } | ||
| size={18} | ||
| /> | ||
| </button> | ||
| <ToggleIcon | ||
| className={effectiveConnected | ||
| ? "text-primary hover:text-primary/80" | ||
| : "text-gray-400" | ||
| } | ||
| size={18} | ||
| /> | ||
| </Badge> | ||
| {isOpen && ( | ||
| <div className="p-4 w-auto text-nowrap py-3 border bg-gray-50 dark:bg-gray-800 text-sm dark:border-tw-dark-border | ||
| rounded z-100 absolute top-10 right-0" ref={dropdownRef}> | ||
| <ul className="space-y-2"> | ||
| {Object.entries(clusterData.clusterNodes).map(([primaryKey, primary]) => { | ||
| const nodeIsConnected = allConnections?.[primaryKey]?.status === CONNECTED | ||
| const isCurrentNode = primaryKey === id | ||
|
|
||
| return ( | ||
| <li className="flex flex-col gap-1" key={primaryKey}> | ||
| <button className="flex items-center cursor-pointer hover:bg-primary/20" | ||
| disabled={!nodeIsConnected} | ||
| <button className={`flex items-center cursor-pointer hover:bg-primary/20 ${isCurrentNode ? "bg-primary/10" : ""}`} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's use |
||
| onClick={() => handleNavigate(primaryKey)}> | ||
| <Dot className={nodeIsConnected ? "text-green-500" : "text-gray-400"} size={45} /> | ||
| <Dot className={isCurrentNode ? "text-green-500" : "text-primary"} size={45} /> | ||
| <Typography variant="bodySm"> | ||
| {`${primary.host}:${primary.port}`} | ||
| </Typography> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,21 @@ | ||
| import { useSelector } from "react-redux" | ||
| import { useParams } from "react-router" | ||
| import { CONNECTED } from "@common/src/constants.ts" | ||
| import { selectStatus } from "@/state/valkey-features/connection/connectionSelectors.ts" | ||
| import { selectStatus, selectConnections } from "@/state/valkey-features/connection/connectionSelectors.ts" | ||
|
|
||
| const useIsConnected = (): boolean => { | ||
| const { id } = useParams<{ id: string }>() | ||
| const { id, clusterId } = useParams<{ id: string; clusterId: string }>() | ||
| const status = useSelector(selectStatus(id!)) | ||
| return status === CONNECTED | ||
| const connections = useSelector(selectConnections) | ||
|
|
||
| // For cluster routes, consider connected if ANY node in the same cluster is connected | ||
| if (clusterId && status !== CONNECTED) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This hook is a convenience util to get connection status by URL params effectively. This change breaks the abstraction boundary — part of the job is done in the selector, another part — in the hook. What effectively this change is doing — is making clusterId an argument for the selector that has only one argument. So the selector now needs to accept two parameters (this is a minimal change surface) or accept an object Another option is (also within the selector) is to flatten.filter connections — this way we don't even need clusterId. |
||
| return Object.values(connections).some( | ||
| (conn) => conn.connectionDetails.clusterId === clusterId && conn.status === CONNECTED, | ||
| ) | ||
| } | ||
|
|
||
| return status === CONNECTED | ||
| } | ||
|
|
||
| export default useIsConnected | ||
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.
I think, this change is unnecessary, let's keep the original (line 24)