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 bfaddee..21b4bbf 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]; @@ -82,6 +98,7 @@ const makeProps = (overrides: Partial = {}): AdvancedSett describe("AdvancedSettings", () => { beforeEach(() => { vi.clearAllMocks(); + mockTeams([personalTeam]); mockUseAuthContext.mockReturnValue(makeAuthContext()); }); @@ -91,7 +108,9 @@ describe("AdvancedSettings", () => { expect(screen.getByRole("button", { name: "About visibility levels" })).toBeInTheDocument(); }); - describe("team visibility — teamId sync (issue #5077)", () => { + // The sidebar switcher is authoritative while *creating* only. Edit mode is + // covered separately below: an existing server keeps its own team. + describe("team visibility — teamId sync while creating (issue #5077)", () => { it("syncs teamId with selectedTeamId on mount when visibility is team and teamId is unset", () => { mockUseAuthContext.mockReturnValue(makeAuthContext("team-A")); const onTeamIdChange = vi.fn(); @@ -103,7 +122,7 @@ describe("AdvancedSettings", () => { expect(onTeamIdChange).toHaveBeenCalledWith("team-A"); }); - it("propagates selectedTeamId change after teamId is already set (regression: was ignored by !teamId guard)", () => { + it("propagates a sidebar switch made after teamId is already resolved", () => { mockUseAuthContext.mockReturnValue(makeAuthContext("team-A")); const onTeamIdChange = vi.fn(); const { rerender } = render( @@ -156,17 +175,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", () => { @@ -209,32 +239,139 @@ 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")); + describe("team visibility — teamId sync while editing", () => { + it("keeps the server's own team when the sidebar is on All teams", async () => { + mockTeams([personalTeam, sharedTeam]); + mockUseAuthContext.mockReturnValue(makeAuthContext(null)); + const onTeamIdChange = vi.fn(); - render(); + render( + , + ); - expect(screen.getByText(/scoped to your currently selected team/i)).toBeInTheDocument(); + // Resolving to the personal team here would silently retarget the server. + await screen.findByRole("combobox", { name: /^team/i }); + expect(onTeamIdChange).not.toHaveBeenCalled(); }); - it("shows 'please select a team' when visibility is team but no team is selected", () => { + it("restores the server's own team once it loads after the fallback resolved", async () => { + mockTeams([personalTeam, sharedTeam]); mockUseAuthContext.mockReturnValue(makeAuthContext(null)); + const onTeamIdChange = vi.fn(); + + // The server request has not landed yet, so the form falls back. + const { rerender } = render( + , + ); + await waitFor(() => { + expect(onTeamIdChange).toHaveBeenCalledWith(personalTeam.id); + }); + onTeamIdChange.mockClear(); - render(); + rerender( + , + ); - expect(screen.getByText(/please select a team using the team switcher/i)).toBeInTheDocument(); + await waitFor(() => { + expect(onTeamIdChange).toHaveBeenCalledWith(sharedTeam.id); + }); }); - it("does not show either team hint when visibility is not team", () => { - mockUseAuthContext.mockReturnValue(makeAuthContext("team-A")); + it("ignores a sidebar switch, unlike create mode", () => { + mockTeams([personalTeam, sharedTeam]); + mockUseAuthContext.mockReturnValue(makeAuthContext(sharedTeam.id)); + const onTeamIdChange = vi.fn(); + const { rerender } = render( + , + ); + onTeamIdChange.mockClear(); + + mockUseAuthContext.mockReturnValue(makeAuthContext("team-B")); + rerender( + , + ); + + expect(onTeamIdChange).not.toHaveBeenCalled(); + }); + + it("still lets the caller retarget the server from the selector", async () => { + mockTeams([personalTeam, sharedTeam]); + mockUseAuthContext.mockReturnValue(makeAuthContext(null)); + const onTeamIdChange = vi.fn(); + const user = userEvent.setup(); + + render( + , + ); + + await user.click(await screen.findByRole("combobox", { name: /^team/i })); + await user.click(screen.getByRole("option", { name: personalTeam.name })); + + expect(onTeamIdChange).toHaveBeenCalledWith(personalTeam.id); + }); + }); + + describe("team visibility — selector", () => { + it("stays hidden for a single team", async () => { + render(); + + await waitFor(() => { + expect(screen.queryByRole("combobox", { name: /^team/i })).not.toBeInTheDocument(); + }); + }); + + it("lists the caller's teams", async () => { + mockTeams([personalTeam, sharedTeam]); + + render(); + + const teamSelect = await screen.findByRole("combobox", { name: /^team/i }); + expect(teamSelect).toHaveTextContent("Personal team"); + }); + + 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 dc544f5..de81b6e 100644 --- a/src/components/mcp-servers/AdvancedSettings.tsx +++ b/src/components/mcp-servers/AdvancedSettings.tsx @@ -1,4 +1,3 @@ -import { useEffect } from "react"; import { useIntl } from "react-intl"; import { Info, TriangleAlert } from "lucide-react"; import { Textarea } from "@/components/ui/textarea"; @@ -17,9 +16,10 @@ import { BearerTokenAuth } from "@/components/mcp-servers/BearerTokenAuth"; import { CustomHeadersAuth, type CustomHeader } from "@/components/mcp-servers/CustomHeadersAuth"; import { OAuth2Auth } from "@/components/mcp-servers/OAuth2Auth"; import { QueryParameterAuth } from "@/components/mcp-servers/QueryParameterAuth"; -import { useAuthContext } from "@/auth/AuthContext"; +import { useTeamScope } 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 +30,10 @@ interface AdvancedSettingsProps { onVisibilityChange: (value: Visibility) => void; teamId: string; onTeamIdChange: (value: string) => void; + /** Validation message for the team field, shown on the selector. */ + teamError?: string; + /** The server's own team, in edit mode. Pins the form to it. */ + initialTeamId?: string; authType: AuthType; onAuthTypeChange: (value: AuthType) => void; basicAuthUsername: string; @@ -81,6 +85,8 @@ export function AdvancedSettings({ onVisibilityChange, teamId, onTeamIdChange, + teamError, + initialTeamId, authType, onAuthTypeChange, basicAuthUsername, @@ -126,18 +132,13 @@ export function AdvancedSettings({ onCACertificateFilesSelected, oauthErrors, }: AdvancedSettingsProps) { - const { selectedTeamId } = useAuthContext(); const intl = useIntl(); - - useEffect(() => { - if (visibility === "team") { - if ((selectedTeamId ?? "") !== teamId) { - onTeamIdChange(selectedTeamId ?? ""); - } - } else if (teamId) { - onTeamIdChange(""); - } - }, [visibility, selectedTeamId, teamId, onTeamIdChange]); + const { teams, onTeamChange } = useTeamScope({ + visibility, + teamId, + onTeamIdChange, + recordTeamId: initialTeamId, + }); const renderAuthContent = () => { switch (authType) { @@ -220,9 +221,7 @@ export function AdvancedSettings({ id="visibility" className="h-10 w-full border-neutral-300 dark:border-neutral-700" > - + @@ -236,29 +235,41 @@ export function AdvancedSettings({ - {visibility === "team" && ( -

- {intl.formatMessage({ - id: selectedTeamId - ? "mcpServer.advanced.teamScoped" - : "mcpServer.advanced.teamNotSelected", - })} -

- )} + {visibility === "team" && ( + + )} + {/* Authentication type */}
{(["none", "basic", "bearer", "custom", "oauth", "query"] as AuthType[]).map((type) => { - const label = intl.formatMessage({ id: `mcpServer.advanced.authType.${type}` }); + const label = + type === "none" + ? "None" + : type === "basic" + ? "Basic" + : type === "bearer" + ? "Bearer token" + : type === "custom" + ? "Custom headers" + : type === "oauth" + ? "OAuth 2.0" + : "Query parameter"; const isLongerLabel = type === "custom" || type === "query"; return (
@@ -293,7 +304,7 @@ export function AdvancedSettings({ htmlFor="one-time-auth" className="text-sm font-medium text-neutral-950 dark:text-white" > - {intl.formatMessage({ id: "mcpServer.advanced.oneTimeAuthLabel" })} + One-time authentication
@@ -302,14 +313,14 @@ export function AdvancedSettings({

- {intl.formatMessage({ id: "mcpServer.advanced.oneTimeAuthDescription" })} + {"Use credentials once, don't store them. Health checks will be disabled."}

{oneTimeAuth && (

- {intl.formatMessage({ id: "mcpServer.advanced.oneTimeAuthWarning" })} + Add passthrough headers when one-time authentication is enabled.

)} @@ -321,16 +332,17 @@ export function AdvancedSettings({ htmlFor="passthrough-headers" className="text-sm font-medium text-neutral-950 dark:text-white" > - {intl.formatMessage({ id: "mcpServer.advanced.passthroughLabel" })} + Passthrough headers

- {intl.formatMessage({ id: "mcpServer.advanced.passthroughDescription" })} + Add comma-separate headers to forward from client requests. Leave empty to use global + defaults.