From 4fdba27bbd3a939c1581f0484107b513c8eea8da Mon Sep 17 00:00:00 2001 From: usrnk1 <7547651+usrnk1@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:43:12 +0200 Subject: [PATCH] fix(app): refine MCP status controls --- .../regression/mcp-workspace-toggle.spec.ts | 108 ++++++++++++- .../e2e/regression/session-summary.spec.ts | 14 +- packages/app/e2e/regression/settings.spec.ts | 21 ++- .../app/src/providers/connect/mcp-dialog.tsx | 64 ++++---- packages/app/src/runtime/i18n/en.ts | 4 + .../app/src/settings/providers/extensions.tsx | 46 ++++-- packages/app/src/settings/settings.css | 12 ++ packages/app/src/shell/search-list.css | 57 +++++++ .../gui-extensions/src/details/details.css | 29 ++-- .../gui-extensions/src/details/i18n/en.ts | 6 + .../src/details/server-panel.tsx | 146 +++++++----------- packages/ui/src/components/error-details.css | 4 + packages/ui/src/components/truncated-text.css | 25 +++ packages/ui/src/components/truncated-text.tsx | 29 ++++ 14 files changed, 397 insertions(+), 168 deletions(-) create mode 100644 packages/ui/src/components/truncated-text.css create mode 100644 packages/ui/src/components/truncated-text.tsx diff --git a/packages/app/e2e/regression/mcp-workspace-toggle.spec.ts b/packages/app/e2e/regression/mcp-workspace-toggle.spec.ts index a5a8fcd4378..a5ff7edf41f 100644 --- a/packages/app/e2e/regression/mcp-workspace-toggle.spec.ts +++ b/packages/app/e2e/regression/mcp-workspace-toggle.spec.ts @@ -1,5 +1,5 @@ import { expect, test, type Page } from "@playwright/test" -import { NO_PROVIDER, sessionHref } from "../utils/app" +import { holdRoute, NO_PROVIDER, sessionHref } from "../utils/app" import type { MockServerConfig } from "../utils/mock-server" import { mockWorkspace } from "../utils/workspace" import { expectSessionTitle } from "../utils/waits" @@ -54,7 +54,7 @@ async function open( } await show() - await expect(panel.getByText("figma-desktop", { exact: true })).toBeVisible() + await expect(panel.locator('[data-slot="truncated-text-label"]').filter({ hasText: "figma-desktop" })).toBeVisible() return { requests, panel, show, push: mock.push, toggle: panel.getByRole("switch") } } @@ -85,7 +85,7 @@ for (const shared of [true, false]) { expect(view.requests.every((request) => request.directory === workspace)).toBe(true) view.requests.length = 0 - await view.panel.getByText("figma-desktop", { exact: true }).click() + await view.panel.locator('[data-slot="truncated-text-label"]').filter({ hasText: "figma-desktop" }).click() await expect(view.toggle).not.toBeChecked() await expect(view.toggle).toBeEnabled() expect(actions).toEqual([ @@ -136,16 +136,39 @@ for (const surface of ["popover", "dialog"] as const) { ]) expect(view.requests.every((request) => request.directory === workspace)).toBe(true) - // Both surfaces replace a failed server's switch with a link that opens its error. - const failed = view.panel.getByRole("button", { name: "figma-desktop failed, view error", exact: true }) - const details = page.getByRole("dialog", { name: "MCP server: figma-desktop", exact: true }) + // Status is separate from the shared v2 button that opens the error. + const failed = view.panel.getByRole("button", { name: "Details for figma-desktop", exact: true }) + const details = page.getByRole("dialog", { name: "figma-desktop", exact: true }) await expect(view.toggle).toHaveCount(0) await expect(view.panel.getByText("Failed", { exact: true })).toHaveCount(1) + await expect(failed).toHaveText("Details") + await expect(failed).toHaveAttribute("data-component", "button-v2") await failed.click() + await expect(details).toHaveAccessibleDescription("Connection failed") await expect(details.getByText(error, { exact: true })).toBeVisible() if (surface === "dialog") { - await details.getByRole("button", { name: "Navigate back", exact: true }).click() + const back = details.getByRole("button", { name: "Navigate back", exact: true }) + + await expect + .poll(async () => { + const navigation = await back.boundingBox() + const heading = await details.getByRole("heading", { name: "figma-desktop", exact: true }).boundingBox() + const error = await details.locator('[data-slot="error-details-text"]').boundingBox() + const status = await details.getByText("Connection failed", { exact: true }).boundingBox() + + return ( + !!navigation && + !!heading && + !!error && + !!status && + Math.abs(heading.y - navigation.y) < 1 && + heading.x >= navigation.x + navigation.width && + Math.abs(status.x - error.x) < 1 + ) + }) + .toBe(true) + await back.click() await expect(failed).toBeVisible() await failed.click() } @@ -191,7 +214,19 @@ for (const surface of ["popover", "dialog"] as const) { await expect(view.toggle).toHaveCount(0) await expect(view.panel.getByText("Sign in required", { exact: true })).toHaveCount(0) const popup = page.waitForEvent("popup") - await view.panel.getByRole("button", { name: "Sign in to figma-desktop", exact: true }).click() + const signIn = view.panel.getByRole("button", { name: "Sign in to figma-desktop", exact: true }) + + const authorization = await holdRoute(page, /\/api\/integration\/figma-oauth\/connect\/oauth(?:\?|$)/, { + method: "POST", + }) + + await expect(signIn).toHaveAttribute("data-component", "button-v2") + await expect(signIn.locator("svg")).toHaveCount(0) + await signIn.click() + await expect(signIn).toHaveText("Signing in…") + await expect(signIn).toBeDisabled() + await authorization.arrived + authorization.release() await expect(await popup).toHaveURL("https://auth.example.test/authorize") expect(attempts).toEqual([workspace]) @@ -212,3 +247,60 @@ for (const surface of ["popover", "dialog"] as const) { await expect(view.panel.getByRole("button", { name: "Sign in to figma-desktop", exact: true })).toHaveCount(0) }) } + +for (const surface of ["popover", "dialog"] as const) { + test(`long MCP names keep their status and controls visible in the ${surface}`, async ({ page }) => { + const names = ["connected", "disabled", "failed", "needs_auth"].map( + (status) => `long-${status}-mcp-server-for-the-internal-knowledge-base-search`, + ) + + const actions: string[] = [] + + const view = await open(page, { + surface, + mcp: [ + { name: "figma-desktop", status: { status: "disabled" } }, + ...names.map((name, index) => ({ + name, + integrationID: index === 3 ? "long-oauth" : undefined, + status: { status: ["connected", "disabled", "failed", "needs_auth"][index], error: "Connection refused" }, + })), + ], + onMcpAction: (input) => void actions.push(input.server), + }) + + for (const [index, name] of names.entries()) { + const row = view.panel + .locator(surface === "dialog" ? '[role="option"]' : ".session-mcp-row") + .filter({ hasText: name }) + + const status = row.getByText(["Connected", "Disabled", "Failed", "Sign in"][index], { exact: true }) + + await expect(status).toBeVisible() + await expect + .poll(() => status.evaluate((el) => el.scrollWidth <= el.clientWidth && el.getBoundingClientRect().width > 0)) + .toBe(true) + } + + const name = view.panel.locator('[data-slot="truncated-text-label"]').filter({ hasText: names[0] }) + await name.hover() + // The modal aria-hides body portals; the full name remains the trigger's accessible text. + const tooltip = page.getByRole("tooltip", { includeHidden: true }) + await expect(tooltip).toHaveText(names[0]) + await expect(tooltip).toBeVisible() + await page.mouse.move(0, 0) + await view.panel.locator(".truncated-text").filter({ hasText: names[0] }).focus() + await expect(tooltip).toHaveText(names[0]) + expect(actions).toEqual([]) + await view.panel.locator(".truncated-text").filter({ hasText: names[0] }).blur() + await view.panel.locator('[data-slot="truncated-text-label"]').filter({ hasText: "figma-desktop" }).hover() + await expect(tooltip).toBeHidden() + + if (surface !== "dialog") return + await page.setViewportSize({ width: 390, height: 844 }) + await expect(name).toHaveCSS("white-space", "normal") + await expect.poll(() => name.evaluate((el) => el.scrollWidth <= el.clientWidth)).toBe(true) + await view.panel.getByRole("button", { name: `Details for ${names[2]}`, exact: true }).scrollIntoViewIfNeeded() + await expect(view.panel.getByRole("button", { name: `Details for ${names[2]}`, exact: true })).toBeInViewport() + }) +} diff --git a/packages/app/e2e/regression/session-summary.spec.ts b/packages/app/e2e/regression/session-summary.spec.ts index 8ff3b2e7839..06c66a0d832 100644 --- a/packages/app/e2e/regression/session-summary.spec.ts +++ b/packages/app/e2e/regression/session-summary.spec.ts @@ -419,7 +419,7 @@ test("summary catalogs load, refresh while cached, and tell errors from empty", await expect(menu("LSP").getByText("rust", { exact: true })).toHaveCount(0) await expect(menu("LSP").locator(".session-service-dot")).toHaveCount(0) await open("MCP") - await expect(menu("MCP").getByText("summary-mcp", { exact: true })).toBeVisible() + await expect(menu("MCP").locator('[data-slot="truncated-text-label"]').filter({ hasText: "summary-mcp" })).toBeVisible() for (const service of [ { name: "MCP", path: "/api/mcp", item: "summary-mcp" }, @@ -475,10 +475,11 @@ test("every MCP row hit area toggles exactly once and keeps the submenu open", a const toggle = submenu.getByRole("switch", { name: "figma", exact: true }) const row = submenu - .locator('[data-component="switch"]') + .locator(".session-mcp-row") .filter({ has: page.getByRole("switch", { name: "figma", exact: true }) }) await expect(toggle).toBeChecked() + await expect(row.getByText("Connected", { exact: true })).toBeVisible() // A pointer open focuses the menu itself, so its first row shows no focus ring; a keyboard open focuses that row. await expect(submenu).toBeFocused() await page.keyboard.press("Escape") @@ -492,7 +493,7 @@ test("every MCP row hit area toggles exactly once and keeps the submenu open", a const enabled = index % 2 !== 0 await expect(toggle).toBeEnabled() - if (target === "label") await row.getByText("figma", { exact: true }).click() + if (target === "label") await row.locator('[data-slot="truncated-text-label"]').click() if (target === "dot") await row.locator(".session-service-dot").click() @@ -502,6 +503,7 @@ test("every MCP row hit area toggles exactly once and keeps the submenu open", a if (target === "keyboard") await toggle.press("Space") await expect(toggle).toBeChecked({ checked: enabled }) + await expect(row.getByText(enabled ? "Connected" : "Disabled", { exact: true })).toBeVisible() await expect(toggle).toBeEnabled() await expect(submenu).toBeVisible() @@ -548,7 +550,7 @@ test("MCP authentication starts before a slow resource catalog finishes", async try { const popup = page.waitForEvent("popup") - await submenu.getByText("linear", { exact: true }).click() + await submenu.locator('[data-slot="truncated-text-label"]').filter({ hasText: "linear" }).click() await expect(await popup).toHaveURL("https://auth.example.test/authorize") await resources.arrived // The switch hands over to the sign-in link, which waits for the catalog before it is ready again. @@ -732,10 +734,10 @@ async function expectRefreshKeeps(page: Page, name: string, path: string, conten await summary.getByRole("button", { name, exact: true }).click() await refresh.arrived await expect(menu).toHaveAttribute("aria-busy", "true") - await expect(menu.getByText(content, { exact: true })).toBeVisible() + await expect(menu.getByText(content, { exact: true }).filter({ visible: true })).toBeVisible() await expect(menu.getByRole("status")).toHaveCount(0) await expect(summary).toBeVisible() refresh.release() await expect(menu).toHaveAttribute("aria-busy", "false") - await expect(menu.getByText(content, { exact: true })).toBeVisible() + await expect(menu.getByText(content, { exact: true }).filter({ visible: true })).toBeVisible() } diff --git a/packages/app/e2e/regression/settings.spec.ts b/packages/app/e2e/regression/settings.spec.ts index e90d67bbed8..4dcf6de2335 100644 --- a/packages/app/e2e/regression/settings.spec.ts +++ b/packages/app/e2e/regression/settings.spec.ts @@ -487,7 +487,7 @@ test.describe("pages open before slow data", () => { await expect(settings.getByRole("switch", { name: "demo-mcp" })).toBeChecked() }) - test("extensions show failed and auth-gated MCPs as links", async ({ page, context }) => { + test("extensions separate MCP status from details, sign-in, and switch controls", async ({ page, context }) => { const error = "Connection closed: MCP server process exited with code 1" const signIns: string[] = [] await context.route("https://auth.example.test/**", (route) => route.fulfill({ body: "Sign in" })) @@ -496,6 +496,7 @@ test.describe("pages open before slow data", () => { mcp: [ { name: "blender", status: { status: "failed", error } }, { name: "demo-mcp", status: { status: "connected" } }, + { name: "disabled-mcp", status: { status: "disabled" } }, { name: "stripe", integrationID: "stripe-oauth", status: { status: "needs_auth", error: "Sign in required" } }, ], integrations: [ @@ -511,15 +512,25 @@ test.describe("pages open before slow data", () => { await settings.getByRole("tab", { name: "Extensions", exact: true }).click() await settings.getByRole("tab", { name: "MCPs", exact: true }).click() await expect(settings.getByRole("switch", { name: "demo-mcp" })).toBeChecked() - await expect(settings.getByRole("switch")).toHaveCount(1) + await expect(settings.getByRole("switch")).toHaveCount(2) + await expect(settings.getByText("Connected", { exact: true })).toBeVisible() + await expect(settings.getByText("Disabled", { exact: true })).toBeVisible() + await expect(settings.getByText("Failed", { exact: true })).toBeVisible() const popup = page.waitForEvent("popup") - await settings.getByRole("button", { name: "Sign in to stripe", exact: true }).click() + const signIn = settings.getByRole("button", { name: "Sign in to stripe", exact: true }) + await expect(signIn).toHaveAttribute("data-component", "button-v2") + await expect(signIn.locator("svg")).toHaveCount(0) + await signIn.click() await expect(await popup).toHaveURL("https://auth.example.test/authorize") expect(signIns).toEqual(["stripe-oauth"]) - await settings.getByRole("button", { name: "blender failed, view error", exact: true }).click() - const details = page.getByRole("dialog", { name: "MCP server: blender", exact: true }) + const action = settings.getByRole("button", { name: "Details for blender", exact: true }) + await expect(action).toHaveText("Details") + await expect(action).toHaveAttribute("data-component", "button-v2") + await action.click() + const details = page.getByRole("dialog", { name: "blender", exact: true }) + await expect(details).toHaveAccessibleDescription("Connection failed") await expect(details.getByText(error, { exact: true })).toBeVisible() }) diff --git a/packages/app/src/providers/connect/mcp-dialog.tsx b/packages/app/src/providers/connect/mcp-dialog.tsx index 8008b026387..706ffbd1d82 100644 --- a/packages/app/src/providers/connect/mcp-dialog.tsx +++ b/packages/app/src/providers/connect/mcp-dialog.tsx @@ -8,9 +8,9 @@ import { useFilteredList } from "@opencode/ui/hooks" import { Icon } from "@opencode/ui/icon" import { ScrollView } from "@opencode/ui/scroll-view" import { ErrorDetails } from "@opencode/ui/error-details" -import { StatusLink } from "@opencode/ui/status-link" import { Switch } from "@opencode/ui/switch" import { TextInput } from "@opencode/ui/text-input" +import { TruncatedText } from "@opencode/ui/truncated-text" import { useLanguage } from "@/runtime/i18n/language" import { useMcpToggle } from "@/providers/connect/mcp" import { usePlatform } from "@/runtime/platform/platform" @@ -102,7 +102,7 @@ export const DialogMcp: Component<{ directory: string; server?: string }> = (pro return (platform.writeClipboardText?.(details) ?? navigator.clipboard.writeText(details)) .then(() => setStore("copied", true)) - .catch((error: unknown) => + .catch((error) => showToast({ variant: "error", title: language.t("dialog.mcp.error.copyFailed"), @@ -151,7 +151,7 @@ export const DialogMcp: Component<{ directory: string; server?: string }> = (pro } return ( - + = (pro onClick={() => select(item.name)} >
- {item.name} - - {detail()} - +
- {/* Sized for the failed and sign-in links so every row's text truncates at the same edge. */} -
event.stopPropagation()} - > +
event.stopPropagation()}> = (pro checked={status()?.status === "connected"} disabled={status()?.status === "pending" || busy()} onChange={() => toggleServer(item.name)} - /> + > + {item.name} + + } > - toggleServer(item.name)} > - {language.t("dialog.mcp.signIn")} - + {language.t(busy() ? "dialog.mcp.signingIn" : "dialog.mcp.signIn")} + } > - {language.t("mcp.status.failed")} +
@@ -293,7 +293,7 @@ export const DialogMcp: Component<{ directory: string; server?: string }> = (pro // Escape steps back to the list, like the TUI, instead of closing the dialog.
-
+
- + + {current().name} + + } + description={language.t("dialog.mcp.error.status")} + />
- } > - {language.t("mcp.status.failed")} +
diff --git a/packages/app/src/settings/settings.css b/packages/app/src/settings/settings.css index 9dd03b26b54..79392c84720 100644 --- a/packages/app/src/settings/settings.css +++ b/packages/app/src/settings/settings.css @@ -2147,6 +2147,18 @@ line-height: var(--line-height-compact); } +.settings-mcp-row .settings-extension-trailing { + min-width: 0; + align-items: center; + gap: 8px; + white-space: nowrap; +} + +.settings-mcp-row { + min-height: 56px; + gap: 16px; +} + .settings-extension-lead { display: flex; flex: 1; diff --git a/packages/app/src/shell/search-list.css b/packages/app/src/shell/search-list.css index 7e873275f8e..68e320aa2e2 100644 --- a/packages/app/src/shell/search-list.css +++ b/packages/app/src/shell/search-list.css @@ -113,3 +113,60 @@ line-height: var(--line-height-compact); letter-spacing: -0.04px; } + +.mcp-dialog .search-list-row { + gap: 16px; +} + +.mcp-dialog .search-list-row-text { + flex: 1; +} + +.mcp-dialog .search-list-title { + flex: 1; +} + +.mcp-trailing { + justify-content: flex-end; + gap: 8px; + white-space: nowrap; +} + +.mcp-error-heading { + display: grid; + min-width: 0; + flex: 1; + grid-template-columns: 20px minmax(0, 1fr); + gap: 4px 8px; +} + +[data-component="dialog-v2"] + [data-slot="dialog-container"] + .mcp-dialog[data-slot="dialog-content"] + .mcp-error-heading + > [data-slot="dialog-title-group"] { + display: contents; +} + +.mcp-error-heading [data-slot="dialog-title"] { + grid-column: 2; + grid-row: 1; +} + +.mcp-error-heading [data-slot="dialog-description"] { + grid-column: 1 / -1; + grid-row: 2; +} + +@media (max-width: 639px), (hover: none) { + .mcp-dialog .search-list-row { + height: auto; + min-height: 44px; + padding-block: 8px; + } + + .mcp-dialog .search-list-title { + white-space: normal; + overflow-wrap: anywhere; + } +} diff --git a/packages/gui-extensions/src/details/details.css b/packages/gui-extensions/src/details/details.css index a47c48490a7..2193a3c3861 100644 --- a/packages/gui-extensions/src/details/details.css +++ b/packages/gui-extensions/src/details/details.css @@ -254,34 +254,31 @@ color: var(--icon-warning-base); } -[data-component="switch"].session-mcp-row [data-slot="switch-label"] { - display: flex; - align-items: center; - gap: 8px; - flex: 1; - min-width: 0; - height: auto; - align-self: stretch; - color: inherit; - font: inherit; - letter-spacing: inherit; -} -/* Every row ends in the same 60px slot as the MCPs dialog, so switches and status links line up. */ .session-mcp-trailing { display: flex; - min-width: 60px; flex-shrink: 0; + align-items: center; justify-content: flex-end; + gap: 8px; + margin-inline-start: 8px; + white-space: nowrap; } -[data-component="switch"].session-mcp-row [data-slot="switch-control"] { - margin-inline-start: 36px; +.session-mcp-failed { + color: var(--v2-state-fg-danger); } div.session-mcp-row { cursor: pointer; } +@media (max-width: 639px), (hover: none) { + .session-mcp-row { + min-height: 44px; + padding-block: 8px; + } +} + .session-mcp-error-body { display: flex; min-height: 0; diff --git a/packages/gui-extensions/src/details/i18n/en.ts b/packages/gui-extensions/src/details/i18n/en.ts index 2208dd411bf..a2ef4fa40e6 100644 --- a/packages/gui-extensions/src/details/i18n/en.ts +++ b/packages/gui-extensions/src/details/i18n/en.ts @@ -34,6 +34,12 @@ export default { "workspace.new": "New worktree", "mcp.interactiveAuth": "MCP server {{name}} requires an interactive authentication form", "mcp.signIn": "Sign in", + "mcp.signingIn": "Signing in…", + "mcp.connected": "Connected", + "mcp.disabled": "Disabled", + "mcp.error.action": "Details", + "mcp.error.status": "Connection failed", + "mcp.error.actionLabel": "Details for {{name}}", "mcp.signIn.open": "Sign in to {{name}}", "mcp.error.open": "{{name}} failed, view error", "mcp.error.title": "MCP server: {{name}}", diff --git a/packages/gui-extensions/src/details/server-panel.tsx b/packages/gui-extensions/src/details/server-panel.tsx index 27990eb130c..7260995a8c9 100644 --- a/packages/gui-extensions/src/details/server-panel.tsx +++ b/packages/gui-extensions/src/details/server-panel.tsx @@ -4,10 +4,10 @@ import { Button } from "@opencode/ui/button" import { Dialog, DialogBody, DialogHeader, DialogTitleGroup } from "@opencode/ui/dialog" import { ErrorDetails } from "@opencode/ui/error-details" import { Icon } from "@opencode/ui/icon" -import { StatusLink } from "@opencode/ui/status-link" import { Switch } from "@opencode/ui/switch" import { showToast } from "@opencode/ui/toast" import { Tooltip } from "@opencode/ui/tooltip" +import { TruncatedText } from "@opencode/ui/truncated-text" import { getDirectory } from "@opencode/util/path" import { useMutation } from "@tanstack/solid-query" import { @@ -221,17 +221,11 @@ function McpMenu(props: ServiceMenuProps) { {(server) => { const enabled = () => server().status.status !== "disabled" const pending = () => toggle.isPending || server().status.status === "pending" - // Like the MCPs dialog, a failed or auth-gated server shows its follow-up where the switch would be. + // Status stays beside its control rather than competing with a long server name. const failed = () => server().status.status === "failed" const signIn = () => server().status.status === "needs_auth" && !!server().integrationID const busy = () => toggle.isPending && toggle.variables?.name === server().name - const error = () => { - const status = server().status - - return status.status === "failed" ? status.error : undefined - } - const label = () => { const status = server().status.status @@ -239,7 +233,7 @@ function McpMenu(props: ServiceMenuProps) { if (status === "needs_auth") return ctx.t("needsAuth") - return undefined + return ctx.t(status === "connected" ? "mcp.connected" : "mcp.disabled") } const change = (value: boolean) => { @@ -249,90 +243,51 @@ function McpMenu(props: ServiceMenuProps) { const follow = () => { if (failed()) return showError(server().name) - change(true) + change(server().status.status === "needs_auth" || !enabled()) } return ( - { - if ( - !(event.target instanceof Element) || - event.target.closest('[data-slot="switch-control"], [data-slot="switch-input"]') - ) - return - - // Outside the switch itself, a row that requires sign-in starts sign-in instead of toggling. - if (server().status.status === "needs_auth") { - event.preventDefault() - - return change(true) - } - - if (event.target === event.currentTarget) change(!enabled()) - }} - title={error() ?? server().name} - > - + + + + ) }} @@ -462,6 +417,7 @@ function ServicePopover(props: ServiceMenuProps & ServiceBodyProps & { empty: bo const placement = createMemo(() => props.mobile ? "top-end" : locale.direction() === "rtl" ? "right-start" : "left-start", ) + // How the trigger last opened the menu: a keyboard open moves focus to the first row, a pointer open does not. const opening = { pointer: false } @@ -771,18 +727,26 @@ function McpErrorDialog(props: { session: MountedSession; directory: string; nam {(current) => ( - + + {current().name} + + } + description={ctx.t("mcp.error.status")} + />