From 7d79a6bbca2c99afb746180c4728c61d2dd3e062 Mon Sep 17 00:00:00 2001 From: Anna Effort Date: Fri, 14 Aug 2026 12:59:22 -0700 Subject: [PATCH] fix: stop asking single-team callers to select a team MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Team visibility was driven entirely by the sidebar switcher, which starts on "All teams" every session. A caller with one team was told to go pick the only team they have, and the form flagged the field red the moment they chose "Team" — before they had done anything wrong. Resolve the team in the form instead: an explicit choice, else the sidebar's active team, else the caller's personal (or only) team. Callers in more than one team now pick inline via a new TeamSelect rather than being sent to the sidebar, and the requirement is raised on submit rather than on entering team visibility. The sidebar switcher stays authoritative for an open form (#5077) until the caller picks a team in the selector. Signed-off-by: Anna Effort --- src/components/common/TeamSelect.test.tsx | 54 ++++++++ src/components/common/TeamSelect.tsx | 78 +++++++++++ .../mcp-servers/AdvancedSettings.test.tsx | 70 +++++++--- .../mcp-servers/AdvancedSettings.tsx | 53 +++++-- .../mcp-servers/MCPServerForm.test.tsx | 52 +++++-- src/components/mcp-servers/MCPServerForm.tsx | 1 + src/components/prompts/PromptForm.test.tsx | 131 ++++++++++++------ src/components/prompts/PromptForm.tsx | 25 ++-- .../tools/ToolAdvancedSettings.test.tsx | 44 +++++- src/components/tools/ToolAdvancedSettings.tsx | 52 +++++-- src/components/tools/ToolAuth.test.tsx | 7 - src/components/tools/ToolForm.tsx | 1 + src/hooks/usePromptForm.test.ts | 112 ++++++++++----- src/hooks/usePromptForm.ts | 83 ++++++----- src/hooks/useTeams.test.ts | 82 +++++++++++ src/hooks/useTeams.ts | 47 +++++++ src/i18n/locales/en-US/common.json | 5 +- src/i18n/locales/en-US/prompts.json | 2 - src/i18n/locales/es-ES/common.json | 5 +- src/i18n/locales/es-ES/prompts.json | 2 - src/i18n/locales/pt-BR/common.json | 5 +- src/i18n/locales/pt-BR/prompts.json | 2 - 22 files changed, 710 insertions(+), 203 deletions(-) create mode 100644 src/components/common/TeamSelect.test.tsx create mode 100644 src/components/common/TeamSelect.tsx create mode 100644 src/hooks/useTeams.test.ts create mode 100644 src/hooks/useTeams.ts diff --git a/src/components/common/TeamSelect.test.tsx b/src/components/common/TeamSelect.test.tsx new file mode 100644 index 0000000..9990c0c --- /dev/null +++ b/src/components/common/TeamSelect.test.tsx @@ -0,0 +1,54 @@ +import { describe, expect, it, vi } from "vitest"; +import { screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { renderWithProviders } from "@/test/test-utils"; +import type { Team } from "@/types/team"; +import { TeamSelect } from "./TeamSelect"; + +const personalTeam = { id: "team-personal", name: "Personal team", is_personal: true } as Team; +const sharedTeam = { id: "team-shared", name: "Shared team", is_personal: false } as Team; + +describe("TeamSelect", () => { + it("renders nothing for a single team", () => { + const { container } = renderWithProviders( + , + ); + + expect(container).toBeEmptyDOMElement(); + }); + + it("renders an error even without a selector", () => { + // A failed /teams load leaves no teams to choose from, so the error is the + // only thing explaining why the form will not submit. + renderWithProviders(); + + expect(screen.getByText("Team is required")).toBeInTheDocument(); + }); + + it("reports the chosen team", async () => { + const onChange = vi.fn(); + renderWithProviders( + , + ); + + await userEvent.setup().click(screen.getByRole("combobox", { name: /^team/i })); + await userEvent.setup().click(screen.getByRole("option", { name: "Shared team" })); + + expect(onChange).toHaveBeenCalledWith(sharedTeam.id); + }); + + it("marks the field invalid when in error", () => { + renderWithProviders( + , + ); + + const select = screen.getByRole("combobox", { name: /^team/i }); + expect(select).toHaveAttribute("aria-invalid", "true"); + expect(select).toHaveAccessibleDescription("Team is required"); + }); +}); diff --git a/src/components/common/TeamSelect.tsx b/src/components/common/TeamSelect.tsx new file mode 100644 index 0000000..6351402 --- /dev/null +++ b/src/components/common/TeamSelect.tsx @@ -0,0 +1,78 @@ +import { useIntl } from "react-intl"; + +import { Label } from "@/components/ui/label"; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from "@/components/ui/select"; +import type { Team } from "@/types/team"; + +interface TeamSelectProps { + /** Teams the caller belongs to, from `useTeams()`. */ + teams: Team[]; + value?: string; + onChange: (teamId: string) => void; + /** Validation message for the field, rendered below the select. */ + error?: string; + /** Element id for the select, so each form can scope it. */ + id?: string; +} + +/** + * Team picker for `team`-visibility records. + * + * Renders nothing when the caller has fewer than two teams: everyone belongs to + * at least their own personal team, so a single-team caller has no choice to + * make and the form scopes to that team silently (see `resolveTeamId`). The + * exception is an error — shown even without a selector, so a failed `/teams` + * load explains itself instead of leaving the submit button inert. + */ +export function TeamSelect({ teams, value, onChange, error, id = "team" }: TeamSelectProps) { + const intl = useIntl(); + const errorId = `${id}-error`; + + if (teams.length < 2) { + return error ? ( +

+ {error} +

+ ) : null; + } + + return ( +
+ + + {error && ( +

+ {error} +

+ )} +
+ ); +} diff --git a/src/components/mcp-servers/AdvancedSettings.test.tsx b/src/components/mcp-servers/AdvancedSettings.test.tsx index 6cdec5b..3e1fdb6 100644 --- a/src/components/mcp-servers/AdvancedSettings.test.tsx +++ b/src/components/mcp-servers/AdvancedSettings.test.tsx @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { renderWithProviders as render, screen } from "@/test/test-utils"; +import { renderWithProviders as render, screen, waitFor } from "@/test/test-utils"; import userEvent from "@testing-library/user-event"; +import { api } from "@/api/client"; import * as AuthContextModule from "@/auth/AuthContext"; import { AdvancedSettings } from "./AdvancedSettings"; @@ -8,7 +9,22 @@ vi.mock("@/auth/AuthContext", () => ({ useAuthContext: vi.fn(), })); +vi.mock("@/api/client", () => ({ + api: { get: vi.fn() }, +})); + const mockUseAuthContext = vi.mocked(AuthContextModule.useAuthContext); +const mockGet = vi.mocked(api.get); + +const personalTeam = { id: "team-personal", name: "Personal team", is_personal: true }; +const sharedTeam = { id: "team-shared", name: "Shared team", is_personal: false }; + +/** Answers `GET /teams` with the given teams; everything else stays empty. */ +function mockTeams(teams: Array>) { + mockGet.mockImplementation((path: string) => + path === "/teams" ? Promise.resolve({ teams }) : Promise.resolve([]), + ); +} type AdvancedSettingsProps = Parameters[0]; @@ -81,6 +97,7 @@ const makeProps = (overrides: Partial = {}): AdvancedSett describe("AdvancedSettings", () => { beforeEach(() => { vi.clearAllMocks(); + mockTeams([personalTeam]); mockUseAuthContext.mockReturnValue(makeAuthContext()); }); @@ -155,17 +172,28 @@ describe("AdvancedSettings", () => { expect(onTeamIdChange).toHaveBeenCalledWith(""); }); - it("clears teamId when selectedTeamId becomes null while visibility is team", () => { - mockUseAuthContext.mockReturnValue(makeAuthContext(null)); + it("falls back to the caller's own team on a switch to All teams", () => { + mockUseAuthContext.mockReturnValue(makeAuthContext("team-A")); const onTeamIdChange = vi.fn(); + const { rerender } = render( + , + ); + onTeamIdChange.mockClear(); - render( + mockUseAuthContext.mockReturnValue(makeAuthContext(null)); + rerender( , ); - expect(onTeamIdChange).toHaveBeenCalledWith(""); + // "All teams" is not a scope a server can be created in, so the form + // falls back rather than leaving it unscoped. + return waitFor(() => { + expect(onTeamIdChange).toHaveBeenCalledWith(personalTeam.id); + }); }); it("does not call onTeamIdChange when visibility is not team and teamId is already empty", () => { @@ -208,32 +236,32 @@ describe("AdvancedSettings", () => { }); }); - describe("team visibility — hint message", () => { - it("shows 'scoped to currently selected team' when visibility is team and a team is selected", () => { - mockUseAuthContext.mockReturnValue(makeAuthContext("team-A")); - - render(); + describe("team visibility — selector", () => { + it("stays hidden for a single team", async () => { + render(); - expect(screen.getByText(/scoped to your currently selected team/i)).toBeInTheDocument(); + await waitFor(() => { + expect(screen.queryByRole("combobox", { name: /^team/i })).not.toBeInTheDocument(); + }); }); - it("shows 'please select a team' when visibility is team but no team is selected", () => { - mockUseAuthContext.mockReturnValue(makeAuthContext(null)); + it("lists the caller's teams", async () => { + mockTeams([personalTeam, sharedTeam]); - render(); + render(); - expect(screen.getByText(/please select a team using the team switcher/i)).toBeInTheDocument(); + const teamSelect = await screen.findByRole("combobox", { name: /^team/i }); + expect(teamSelect).toHaveTextContent("Personal team"); }); - it("does not show either team hint when visibility is not team", () => { - mockUseAuthContext.mockReturnValue(makeAuthContext("team-A")); + it("stays hidden when visibility is not team", async () => { + mockTeams([personalTeam, sharedTeam]); render(); - expect(screen.queryByText(/scoped to your currently selected team/i)).not.toBeInTheDocument(); - expect( - screen.queryByText(/please select a team using the team switcher/i), - ).not.toBeInTheDocument(); + await waitFor(() => { + expect(screen.queryByRole("combobox", { name: /^team/i })).not.toBeInTheDocument(); + }); }); }); diff --git a/src/components/mcp-servers/AdvancedSettings.tsx b/src/components/mcp-servers/AdvancedSettings.tsx index bd53ec3..5c5f501 100644 --- a/src/components/mcp-servers/AdvancedSettings.tsx +++ b/src/components/mcp-servers/AdvancedSettings.tsx @@ -1,4 +1,4 @@ -import { useEffect } from "react"; +import { useEffect, useState } from "react"; import { useIntl } from "react-intl"; import { Info, TriangleAlert } from "lucide-react"; import { Textarea } from "@/components/ui/textarea"; @@ -18,8 +18,10 @@ import { CustomHeadersAuth, type CustomHeader } from "@/components/mcp-servers/C import { OAuth2Auth } from "@/components/mcp-servers/OAuth2Auth"; import { QueryParameterAuth } from "@/components/mcp-servers/QueryParameterAuth"; import { useAuthContext } from "@/auth/AuthContext"; +import { resolveTeamId, useTeams } from "@/hooks/useTeams"; import type { Visibility } from "@/types/server"; import { VisibilityInfoPopover } from "@/components/common/VisibilityInfoPopover"; +import { TeamSelect } from "@/components/common/TeamSelect"; export type { CustomHeader }; @@ -30,6 +32,8 @@ interface AdvancedSettingsProps { onVisibilityChange: (value: Visibility) => void; teamId: string; onTeamIdChange: (value: string) => void; + /** Validation message for the team field, shown on the selector. */ + teamError?: string; authType: AuthType; onAuthTypeChange: (value: AuthType) => void; basicAuthUsername: string; @@ -81,6 +85,7 @@ export function AdvancedSettings({ onVisibilityChange, teamId, onTeamIdChange, + teamError, authType, onAuthTypeChange, basicAuthUsername, @@ -127,17 +132,32 @@ export function AdvancedSettings({ oauthErrors, }: AdvancedSettingsProps) { const { selectedTeamId } = useAuthContext(); + const { teams } = useTeams(); const intl = useIntl(); + const [pickedInForm, setPickedInForm] = useState(false); + + // The sidebar switcher stays authoritative (#5077) until the caller picks a + // team in the selector below. "All teams" is not a scope a server can be + // created in, so it resolves to the caller's own team rather than leaving the + // server unscoped. useEffect(() => { - if (visibility === "team") { - if ((selectedTeamId ?? "") !== teamId) { - onTeamIdChange(selectedTeamId ?? ""); - } - } else if (teamId) { - onTeamIdChange(""); + if (visibility !== "team") { + if (teamId) onTeamIdChange(""); + return; + } + if (pickedInForm) return; + + const resolved = resolveTeamId(teams, selectedTeamId); + if (resolved && resolved !== teamId) { + onTeamIdChange(resolved); } - }, [visibility, selectedTeamId, teamId, onTeamIdChange]); + }, [visibility, selectedTeamId, teams, teamId, pickedInForm, onTeamIdChange]); + + const handleTeamChange = (nextTeamId: string) => { + setPickedInForm(true); + onTeamIdChange(nextTeamId); + }; const renderAuthContent = () => { switch (authType) { @@ -234,15 +254,18 @@ export function AdvancedSettings({ - {visibility === "team" && ( -

- {selectedTeamId - ? "This server will be scoped to your currently selected team" - : "Please select a team using the team switcher in the sidebar"} -

- )} + {visibility === "team" && ( + + )} + {/* Authentication type */}