fix: address review feedback — criticals + mediums

Critical fixes: 1. Null channelId guard — popover renders children-only (no popover content) when channelId is null. Action handlers also early-return. 2. pickBotName fallback — safeBotName() wraps pickBotName with a defensive fallback to persona.displayName or 'Agent'. 3. E2E test updates — tests now click through the quick-add popover's 'More options…' button before asserting on the full dialog. 4. Duplicate AddChannelBotDialog — removed the sidebar's own dialog instance. Dialog state is now lifted to ChannelScreen and passed down via props. Only one dialog instance exists (in ChannelMembersBar). Medium fixes: 5. Discriminated union types — QuickAddAgentItem is now a proper tagged union (RunningAvailableItem | RunningInChannelItem | PersonaItem). No more optional agent?/persona? fields. 6. Extracted sortProviders utility — shared between ChannelMembersBar and any future consumer. Single source of truth for provider ordering. 7. Emerald colors kept — matches existing presence pattern (bg-emerald-500) used throughout the codebase. No design token exists for 'running' status. 8. Keyboard navigation — added arrow-key roving focus via onKeyDown handler on the popover container, targeting [data-quick-add-item] buttons. 9. Eager queries — channel members query already gated by popover open state. Other queries (agents, personas, providers) are globally cached by ChannelMembersBar which is always mounted — no extra network cost.
2026-11-05 00:41:28 +00:00 · 2026-05-19 22:13:13 -07:00
parent 0d56b5805f
commit d1b7aeb538
7 changed files with 156 additions and 102 deletions
--- a/desktop/src/features/agents/lib/sortProviders.ts
+++ b/desktop/src/features/agents/lib/sortProviders.ts
@@ -0,0 +1,18 @@
+import type { AcpProvider } from "@/shared/api/types";
+
+/**
+ * Sort ACP providers with "goose" first, then alphabetically by label.
+ * Used by any surface that presents a provider list to the user.
+ */
+export function sortProviders(
+  providers: readonly AcpProvider[],
+): AcpProvider[] {
+  return [...providers].sort((left, right) => {
+    const leftPriority = left.id === "goose" ? 0 : 1;
+    const rightPriority = right.id === "goose" ? 0 : 1;
+    if (leftPriority !== rightPriority) {
+      return leftPriority - rightPriority;
+    }
+    return left.label.localeCompare(right.label);
+  });
+}
--- a/desktop/src/features/channels/ui/ChannelMembersBar.tsx
+++ b/desktop/src/features/channels/ui/ChannelMembersBar.tsx
@@ -9,6 +9,7 @@ import {
  useManagedAgentsQuery,
  useRelayAgentsQuery,
 } from "@/features/agents/hooks";
+import { sortProviders } from "@/features/agents/lib/sortProviders";
 import { useChannelMembersQuery } from "@/features/channels/hooks";
 import type { Channel } from "@/shared/api/types";
 import { normalizePubkey } from "@/shared/lib/pubkey";
@@ -19,6 +20,8 @@ import { QuickAddAgentPopover } from "./QuickAddAgentPopover";
 type ChannelMembersBarProps = {
  channel: Channel;
  currentPubkey?: string;
+  isAddBotDialogOpen?: boolean;
+  onAddBotDialogOpenChange?: (open: boolean) => void;
  onManageChannel: () => void;
  onToggleMembers: () => void;
 };
@@ -26,10 +29,15 @@ type ChannelMembersBarProps = {
 export function ChannelMembersBar({
  channel,
  currentPubkey,
+  isAddBotDialogOpen,
+  onAddBotDialogOpenChange,
  onManageChannel,
  onToggleMembers,
 }: ChannelMembersBarProps) {
-  const [isAddBotOpen, setIsAddBotOpen] = React.useState(false);
+  // Dialog state: controlled externally if props provided, otherwise local.
+  const [localIsAddBotOpen, setLocalIsAddBotOpen] = React.useState(false);
+  const isAddBotOpen = isAddBotDialogOpen ?? localIsAddBotOpen;
+  const setIsAddBotOpen = onAddBotDialogOpenChange ?? setLocalIsAddBotOpen;
  const [isQuickAddOpen, setIsQuickAddOpen] = React.useState(false);
  const { startHuddle, isStarting: isStartingHuddle } = useHuddle();
  const queryClient = useQueryClient();
@@ -41,16 +49,7 @@ export function ChannelMembersBar({
  const members = membersQuery.data ?? [];
  const memberCount = membersQuery.data?.length ?? channel.memberCount;
  const providers = React.useMemo(
-    () =>
-      [...(providersQuery.data ?? [])].sort((left, right) => {
-        const leftPriority = left.id === "goose" ? 0 : 1;
-        const rightPriority = right.id === "goose" ? 0 : 1;
-        if (leftPriority !== rightPriority) {
-          return leftPriority - rightPriority;
-        }
-
-        return left.label.localeCompare(right.label);
-      }),
+    () => sortProviders(providersQuery.data ?? []),
    [providersQuery.data],
  );
  const normalizedCurrentPubkey = currentPubkey
@@ -76,7 +75,7 @@ export function ChannelMembersBar({
    previousChannelIdRef.current = channel.id;
    setIsAddBotOpen(false);
    setIsQuickAddOpen(false);
-  }, [channel.id]);
+  }, [channel.id, setIsAddBotOpen]);

  const dialogErrorMessage =
    providersQuery.error instanceof Error
--- a/desktop/src/features/channels/ui/ChannelScreen.tsx
+++ b/desktop/src/features/channels/ui/ChannelScreen.tsx
@@ -85,6 +85,7 @@ export function ChannelScreen({
    string | null
  >(null);
  const [isMembersSidebarOpen, setIsMembersSidebarOpen] = React.useState(false);
+  const [isAddBotDialogOpen, setIsAddBotDialogOpen] = React.useState(false);
  const [openThreadHeadId, setOpenThreadHeadId] = React.useState<string | null>(
    null,
  );
@@ -436,7 +437,9 @@ export function ChannelScreen({
          activeChannelTitle={activeChannelTitle}
          activeDmPresenceStatus={activeDmPresenceStatus}
          currentPubkey={currentPubkey}
+          isAddBotDialogOpen={isAddBotDialogOpen}
          isJoining={joinChannelMutation.isPending}
+          onAddBotDialogOpenChange={setIsAddBotDialogOpen}
          onJoinChannel={joinChannelMutation.mutateAsync}
          onManageChannel={openChannelManagement}
          onToggleMembers={() => setIsMembersSidebarOpen((prev) => !prev)}
@@ -530,6 +533,7 @@ export function ChannelScreen({
          currentPubkey={currentPubkey}
          open={isMembersSidebarOpen}
          onOpenChange={setIsMembersSidebarOpen}
+          onOpenAddBotDialog={() => setIsAddBotDialogOpen(true)}
          onViewActivity={handleOpenAgentSession}
        />
      </ProfilePanelProvider>
--- a/desktop/src/features/channels/ui/ChannelScreenHeader.tsx
+++ b/desktop/src/features/channels/ui/ChannelScreenHeader.tsx
@@ -14,7 +14,9 @@ type ChannelScreenHeaderProps = {
  activeChannelTitle: string;
  activeDmPresenceStatus: PresenceStatus | null;
  currentPubkey?: string;
+  isAddBotDialogOpen?: boolean;
  isJoining?: boolean;
+  onAddBotDialogOpenChange?: (open: boolean) => void;
  onJoinChannel?: () => Promise<void>;
  onManageChannel: () => void;
  onToggleMembers: () => void;
@@ -26,7 +28,9 @@ export function ChannelScreenHeader({
  activeChannelTitle,
  activeDmPresenceStatus,
  currentPubkey,
+  isAddBotDialogOpen,
  isJoining = false,
+  onAddBotDialogOpenChange,
  onJoinChannel,
  onManageChannel,
  onToggleMembers,
@@ -56,6 +60,8 @@ export function ChannelScreenHeader({
            <ChannelMembersBar
              channel={activeChannel}
              currentPubkey={currentPubkey}
+              isAddBotDialogOpen={isAddBotDialogOpen}
+              onAddBotDialogOpenChange={onAddBotDialogOpenChange}
              onManageChannel={onManageChannel}
              onToggleMembers={onToggleMembers}
            />
--- a/desktop/src/features/channels/ui/MembersSidebar.tsx
+++ b/desktop/src/features/channels/ui/MembersSidebar.tsx
@@ -5,13 +5,7 @@ import {
  useAddChannelMembersMutation,
  useChannelMembersQuery,
 } from "@/features/channels/hooks";
-import {
-  useAcpProvidersQuery,
-  useBackendProvidersQuery,
-  useManagedAgentsQuery,
-  useRelayAgentsQuery,
-  useUpdateManagedAgentMutation,
-} from "@/features/agents/hooks";
+import { useUpdateManagedAgentMutation } from "@/features/agents/hooks";
 import { CreateAgentRespondToField } from "@/features/agents/ui/RespondToField";
 import { useClassifiedMembers } from "@/features/channels/lib/useClassifiedMembers";
 import {
@@ -48,7 +42,6 @@ import { MembersSidebarAgentControls } from "./MembersSidebarAgentControls";
 import { ChannelMemberInviteCard } from "./ChannelMemberInviteCard";
 import { MembersSidebarMemberCard } from "./MembersSidebarMemberCard";
 import { useMembersSidebarActions } from "./useMembersSidebarActions";
-import { AddChannelBotDialog } from "./AddChannelBotDialog";
 import { QuickAddAgentPopover } from "./QuickAddAgentPopover";

 type MembersSidebarProps = {
@@ -56,6 +49,7 @@ type MembersSidebarProps = {
  currentPubkey?: string;
  open: boolean;
  onOpenChange: (open: boolean) => void;
+  onOpenAddBotDialog?: () => void;
  onViewActivity?: (pubkey: string) => void;
 };

@@ -64,37 +58,13 @@ export function MembersSidebar({
  currentPubkey,
  open,
  onOpenChange,
+  onOpenAddBotDialog,
  onViewActivity,
 }: MembersSidebarProps) {
  const channelId = channel?.id ?? null;
  const queryClient = useQueryClient();
  const membersQuery = useChannelMembersQuery(channelId, open);
  const addMembersMutation = useAddChannelMembersMutation(channelId);
-  const providersQuery = useAcpProvidersQuery();
-  const backendProvidersQuery = useBackendProvidersQuery();
-  const sidebarManagedAgentsQuery = useManagedAgentsQuery();
-  const sidebarRelayAgentsQuery = useRelayAgentsQuery();
-  const sidebarProviders = React.useMemo(
-    () =>
-      [...(providersQuery.data ?? [])].sort((left, right) => {
-        const leftPriority = left.id === "goose" ? 0 : 1;
-        const rightPriority = right.id === "goose" ? 0 : 1;
-        if (leftPriority !== rightPriority) {
-          return leftPriority - rightPriority;
-        }
-        return left.label.localeCompare(right.label);
-      }),
-    [providersQuery.data],
-  );
-  const sidebarDialogErrorMessage =
-    providersQuery.error instanceof Error
-      ? providersQuery.error.message
-      : sidebarManagedAgentsQuery.error instanceof Error
-        ? sidebarManagedAgentsQuery.error.message
-        : sidebarRelayAgentsQuery.error instanceof Error
-          ? sidebarRelayAgentsQuery.error.message
-          : null;
-
  const changeRoleMutation = useMutation({
    mutationFn: async ({ pubkey, role }: { pubkey: string; role: string }) => {
      if (!channelId) throw new Error("No channel selected.");
@@ -139,8 +109,6 @@ export function MembersSidebar({

  const [isSidebarQuickAddOpen, setIsSidebarQuickAddOpen] =
    React.useState(false);
-  const [isSidebarFullDialogOpen, setIsSidebarFullDialogOpen] =
-    React.useState(false);

  const managedAgentByPubkey = React.useMemo(
    () =>
@@ -347,7 +315,10 @@ export function MembersSidebar({
                  channelId={channelId}
                  open={isSidebarQuickAddOpen}
                  onOpenChange={setIsSidebarQuickAddOpen}
-                  onMoreOptions={() => setIsSidebarFullDialogOpen(true)}
+                  onMoreOptions={() => {
+                    setIsSidebarQuickAddOpen(false);
+                    onOpenAddBotDialog?.();
+                  }}
                >
                  <Button
                    aria-label="Add agent to channel"
@@ -383,18 +354,6 @@ export function MembersSidebar({
        }}
        open={editRespondToAgent !== null}
      />
-      {canAddAgents ? (
-        <AddChannelBotDialog
-          backendProviders={backendProvidersQuery.data ?? []}
-          backendProvidersLoading={backendProvidersQuery.isLoading}
-          channelId={channelId}
-          onOpenChange={setIsSidebarFullDialogOpen}
-          open={isSidebarFullDialogOpen}
-          providers={sidebarProviders}
-          providersErrorMessage={sidebarDialogErrorMessage}
-          providersLoading={providersQuery.isLoading}
-        />
-      ) : null}
    </>
  );
 }
--- a/desktop/src/features/channels/ui/QuickAddAgentPopover.tsx
+++ b/desktop/src/features/channels/ui/QuickAddAgentPopover.tsx
@@ -17,23 +17,39 @@ import type { AgentPersona, ManagedAgent } from "@/shared/api/types";
 import { normalizePubkey } from "@/shared/lib/pubkey";
 import { rewriteRelayUrl } from "@/shared/lib/mediaUrl";
 import { cn } from "@/shared/lib/cn";
-import {
-  Popover,
-  PopoverContent,
-  PopoverTrigger,
-} from "@/shared/ui/popover";
+import { Popover, PopoverContent, PopoverTrigger } from "@/shared/ui/popover";
 import { Spinner } from "@/shared/ui/spinner";

 // ── Types ─────────────────────────────────────────────────────────────────────

-type QuickAddAgentItem = {
-  kind: "running-available" | "running-in-channel" | "persona";
-  agent?: ManagedAgent;
-  persona?: AgentPersona;
+type RunningAvailableItem = {
+  kind: "running-available";
+  agent: ManagedAgent;
+  persona: AgentPersona | null;
  label: string;
  avatarUrl: string | null;
 };

+type RunningInChannelItem = {
+  kind: "running-in-channel";
+  agent: ManagedAgent;
+  persona: AgentPersona | null;
+  label: string;
+  avatarUrl: string | null;
+};
+
+type PersonaItem = {
+  kind: "persona";
+  persona: AgentPersona;
+  label: string;
+  avatarUrl: string | null;
+};
+
+type QuickAddAgentItem =
+  | RunningAvailableItem
+  | RunningInChannelItem
+  | PersonaItem;
+
 type QuickAddAgentPopoverProps = {
  channelId: string | null;
  open: boolean;
@@ -42,6 +58,27 @@ type QuickAddAgentPopoverProps = {
  children: React.ReactNode;
 };

+// ── Helpers ───────────────────────────────────────────────────────────────────
+
+function getItemKey(item: QuickAddAgentItem): string {
+  switch (item.kind) {
+    case "persona":
+      return `persona:${item.persona.id}`;
+    case "running-available":
+    case "running-in-channel":
+      return `agent:${item.agent.pubkey}`;
+  }
+}
+
+function safeBotName(persona: AgentPersona, usedNames: Set<string>): string {
+  const pool = persona.namePool ?? [];
+  const name = pickBotName(pool, usedNames);
+  // pickBotName always returns a string from the universal pool fallback,
+  // but guard defensively in case of edge cases.
+  if (name && name.trim().length > 0) return name;
+  return persona.displayName || "Agent";
+}
+
 // ── Component ─────────────────────────────────────────────────────────────────

 export function QuickAddAgentPopover({
@@ -51,10 +88,16 @@ export function QuickAddAgentPopover({
  onMoreOptions,
  children,
 }: QuickAddAgentPopoverProps) {
+  // Gate channel members query to only fire when the popover is open.
+  // Managed agents, personas, and providers are globally cached by
+  // ChannelMembersBar (always mounted), so no extra fetch cost here.
  const managedAgentsQuery = useManagedAgentsQuery();
  const personasQuery = usePersonasQuery();
  const providersQuery = useAcpProvidersQuery();
-  const membersQuery = useChannelMembersQuery(channelId, open);
+  const membersQuery = useChannelMembersQuery(
+    channelId,
+    open && channelId !== null,
+  );
  const attachMutation = useAttachManagedAgentToChannelMutation(channelId);
  const createMutation = useCreateChannelManagedAgentMutation(channelId);
  const { recentIds, pushRecent } = useBotRecents();
@@ -94,8 +137,7 @@ export function QuickAddAgentPopover({
        channelMemberPubkeys.has(normalizePubkey(agent.pubkey)),
    );

-    // Personas that don't have a running agent yet (configured but not running)
-    // Exclude personas whose agent is already in the channel
+    // Personas whose agent is already in the channel (exclude from "available" list)
    const personaIdsInChannel = new Set(
      managedAgents
        .filter((agent) =>
@@ -113,12 +155,8 @@ export function QuickAddAgentPopover({

    // Sort running-available by recency
    const sortedRunningAvailable = [...runningAvailable].sort((a, b) => {
-      const aPersonaIdx = a.personaId
-        ? recentIds.indexOf(a.personaId)
-        : -1;
-      const bPersonaIdx = b.personaId
-        ? recentIds.indexOf(b.personaId)
-        : -1;
+      const aPersonaIdx = a.personaId ? recentIds.indexOf(a.personaId) : -1;
+      const bPersonaIdx = b.personaId ? recentIds.indexOf(b.personaId) : -1;
      const aScore = aPersonaIdx >= 0 ? aPersonaIdx : 999;
      const bScore = bPersonaIdx >= 0 ? bPersonaIdx : 999;
      return aScore - bScore;
@@ -126,8 +164,8 @@ export function QuickAddAgentPopover({

    for (const agent of sortedRunningAvailable) {
      const persona = agent.personaId
-        ? personas.find((p) => p.id === agent.personaId)
-        : undefined;
+        ? (personas.find((p) => p.id === agent.personaId) ?? null)
+        : null;
      result.push({
        kind: "running-available",
        agent,
@@ -139,8 +177,8 @@ export function QuickAddAgentPopover({

    for (const agent of runningInChannel) {
      const persona = agent.personaId
-        ? personas.find((p) => p.id === agent.personaId)
-        : undefined;
+        ? (personas.find((p) => p.id === agent.personaId) ?? null)
+        : null;
      result.push({
        kind: "running-in-channel",
        agent,
@@ -170,12 +208,7 @@ export function QuickAddAgentPopover({
    }

    return result;
-  }, [
-    managedAgents,
-    personas,
-    channelMemberPubkeys,
-    recentIds,
-  ]);
+  }, [managedAgents, personas, channelMemberPubkeys, recentIds]);

  // Reset state when popover closes
  React.useEffect(() => {
@@ -186,6 +219,7 @@ export function QuickAddAgentPopover({
  }, [open]);

  async function handleAddRunningAgent(agent: ManagedAgent) {
+    if (!channelId) return;
    const key = `agent:${agent.pubkey}`;
    setPendingKey(key);
    setErrorMessage(null);
@@ -203,6 +237,7 @@ export function QuickAddAgentPopover({
  }

  async function handleAddPersona(persona: AgentPersona) {
+    if (!channelId) return;
    const key = `persona:${persona.id}`;
    setPendingKey(key);
    setErrorMessage(null);
@@ -219,9 +254,8 @@ export function QuickAddAgentPopover({
      return;
    }

-    // Pick a name from the persona's pool
    const usedNames = new Set(managedAgents.map((a) => a.name));
-    const instanceName = pickBotName(persona.namePool, usedNames);
+    const instanceName = safeBotName(persona, usedNames);

    try {
      await createMutation.mutateAsync({
@@ -245,10 +279,11 @@ export function QuickAddAgentPopover({
  function handleItemClick(item: QuickAddAgentItem) {
    if (item.kind === "running-in-channel") return;
    if (pendingKey) return;
+    if (!channelId) return;

-    if (item.kind === "running-available" && item.agent) {
+    if (item.kind === "running-available") {
      void handleAddRunningAgent(item.agent);
-    } else if (item.kind === "persona" && item.persona) {
+    } else {
      void handleAddPersona(item.persona);
    }
  }
@@ -258,15 +293,43 @@ export function QuickAddAgentPopover({
    personasQuery.isLoading ||
    providersQuery.isLoading;

+  // Don't render the popover content when there's no channel
+  if (!channelId) {
+    return <>{children}</>;
+  }
+
  return (
    <Popover open={open} onOpenChange={onOpenChange}>
      <PopoverTrigger asChild>{children}</PopoverTrigger>
-      <PopoverContent
-        align="end"
-        className="w-72 p-0"
-        sideOffset={6}
-      >
-        <div className="flex max-h-80 flex-col">
+      <PopoverContent align="end" className="w-72 p-0" sideOffset={6}>
+        <div
+          className="flex max-h-80 flex-col"
+          role="listbox"
+          aria-label="Available agents"
+          onKeyDown={(e) => {
+            const container = e.currentTarget;
+            const buttons = Array.from(
+              container.querySelectorAll<HTMLButtonElement>(
+                "[data-quick-add-item]:not([disabled])",
+              ),
+            );
+            if (buttons.length === 0) return;
+            const focused = document.activeElement as HTMLElement | null;
+            const currentIdx = focused
+              ? buttons.indexOf(focused as HTMLButtonElement)
+              : -1;
+
+            if (e.key === "ArrowDown") {
+              e.preventDefault();
+              const next = currentIdx < buttons.length - 1 ? currentIdx + 1 : 0;
+              buttons[next]?.focus();
+            } else if (e.key === "ArrowUp") {
+              e.preventDefault();
+              const prev = currentIdx > 0 ? currentIdx - 1 : buttons.length - 1;
+              buttons[prev]?.focus();
+            }
+          }}
+        >
          <div className="border-b px-3 py-2">
            <h3 className="text-xs font-semibold uppercase tracking-wide text-muted-foreground">
              Add agent
@@ -285,15 +348,13 @@ export function QuickAddAgentPopover({
            ) : (
              <div className="py-1">
                {items.map((item) => {
-                  const itemKey =
-                    item.kind === "persona"
-                      ? `persona:${item.persona?.id}`
-                      : `agent:${item.agent?.pubkey}`;
+                  const itemKey = getItemKey(item);
                  const isInChannel = item.kind === "running-in-channel";
                  const isItemPending = pendingKey === itemKey;

                  return (
                    <button
+                      aria-selected={isInChannel}
                      className={cn(
                        "flex w-full items-center gap-2.5 px-3 py-1.5 text-left text-sm transition-colors",
                        isInChannel
@@ -301,9 +362,12 @@ export function QuickAddAgentPopover({
                          : "cursor-pointer hover:bg-accent focus-visible:bg-accent focus-visible:outline-none",
                        isItemPending && "pointer-events-none opacity-60",
                      )}
+                      data-quick-add-item
                      disabled={isInChannel || Boolean(pendingKey)}
                      key={itemKey}
                      onClick={() => handleItemClick(item)}
+                      role="option"
+                      tabIndex={isInChannel ? -1 : 0}
                      type="button"
                    >
                      <QuickAddAgentAvatar
@@ -340,6 +404,8 @@ export function QuickAddAgentPopover({
          <div className="border-t">
            <button
              className="flex w-full items-center gap-2 px-3 py-2 text-sm text-muted-foreground transition-colors hover:bg-accent hover:text-foreground"
+              data-quick-add-item
+              data-testid="quick-add-more-options"
              onClick={() => {
                onOpenChange(false);
                onMoreOptions();
--- a/desktop/tests/e2e/channels.spec.ts
+++ b/desktop/tests/e2e/channels.spec.ts
@@ -83,6 +83,7 @@ async function addGenericAgent(
  await page.getByTestId(`channel-${channelName}`).click();
  await expect(page.getByTestId("chat-title")).toHaveText(channelName);
  await page.getByTestId("channel-add-bot-trigger").click();
+  await page.getByTestId("quick-add-more-options").click();
  await expect(page.getByRole("heading", { name: "Add agents" })).toBeVisible();
  await page.getByRole("button", { name: "Generic" }).click();
  await page.locator("#channel-generic-name").fill(agentName);
@@ -852,6 +853,7 @@ test("open-channel members can add agents from the header", async ({
  await expect(addAgentTrigger).toBeEnabled();

  await addAgentTrigger.click();
+  await page.getByTestId("quick-add-more-options").click();
  await expect(page.getByRole("heading", { name: "Add agents" })).toBeVisible();
  await expect(page.getByTestId("add-channel-bot-dialog-header")).toBeVisible();
  await expect(