From 824851acbde74a9bea7707bdf994b963b9f7d1e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Wed, 26 Aug 2026 14:13:35 +0000 Subject: [PATCH] fix(ui): contain growing dialog content (#3060) --- .../fixture-package/ui/bundle.js | 35 +++- .../office/projects/create-project-dialog.tsx | 44 +++-- .../plugins/plugin-modal-host.test.tsx | 42 +++++ .../components/plugins/plugin-modal-host.tsx | 20 ++- .../agent-profile-delete-dialog.test.tsx | 20 +++ .../settings/agent-profile-delete-dialog.tsx | 80 +++++---- .../marketplace-sources-dialog.test.tsx | 79 +++++++++ .../plugins/marketplace-sources-dialog.tsx | 24 ++- .../system-health/health-indicator.test.tsx | 63 +++++++ .../system-health/health-indicator.tsx | 19 ++- .../tests/mobile-system-health-dialog.spec.ts | 130 ++++++++++++++ .../mobile-project-create-dialog.spec.ts | 119 +++++++++++++ .../office/project-repository-picker.spec.ts | 111 ++++++++++++ .../tests/plugins/mobile-plugin-modal.spec.ts | 89 ++++++++++ apps/web/e2e/tests/plugins/plugins.spec.ts | 80 +++++++++ .../settings/agent-profile-delete.spec.ts | 121 +++++++++++++ .../mobile-agent-profile-delete.spec.ts | 133 +++++++++++++++ .../mobile-plugin-marketplace-sources.spec.ts | 116 +++++++++++++ .../plugin-marketplace-sources.spec.ts | 114 +++++++++++++ .../e2e/tests/system-health-dialog.spec.ts | 116 +++++++++++++ apps/web/src/locales/en/plugins.json | 1 + apps/web/src/locales/pseudo/plugins.json | 1 + apps/web/src/locales/pt-pt/plugins.json | 1 + apps/web/src/locales/zh-cn/plugins.json | 1 + apps/web/src/locales/zh-hk/plugins.json | 1 + apps/web/src/locales/zh-tw/plugins.json | 1 + docs/plans/dialog-content-containment/plan.md | 144 ++++++++++++++++ .../task-01-agent-profile-conflict.md | 102 +++++++++++ .../task-02-marketplace-sources.md | 103 +++++++++++ .../task-03-health-issues.md | 101 +++++++++++ .../task-04-plugin-dialog.md | 109 ++++++++++++ .../task-05-create-project.md | 103 +++++++++++ docs/specs/ui/README.md | 2 + .../dialog-content-containment.md | 86 ++++++++++ .../dialog-content-containment.md | 161 ++++++++++++++++++ 35 files changed, 2408 insertions(+), 64 deletions(-) create mode 100644 apps/web/components/settings/plugins/marketplace-sources-dialog.test.tsx create mode 100644 apps/web/components/system-health/health-indicator.test.tsx create mode 100644 apps/web/e2e/tests/mobile-system-health-dialog.spec.ts create mode 100644 apps/web/e2e/tests/office/mobile-project-create-dialog.spec.ts create mode 100644 apps/web/e2e/tests/plugins/mobile-plugin-modal.spec.ts create mode 100644 apps/web/e2e/tests/settings/mobile-plugin-marketplace-sources.spec.ts create mode 100644 apps/web/e2e/tests/settings/plugin-marketplace-sources.spec.ts create mode 100644 apps/web/e2e/tests/system-health-dialog.spec.ts create mode 100644 docs/plans/dialog-content-containment/plan.md create mode 100644 docs/plans/dialog-content-containment/task-01-agent-profile-conflict.md create mode 100644 docs/plans/dialog-content-containment/task-02-marketplace-sources.md create mode 100644 docs/plans/dialog-content-containment/task-03-health-issues.md create mode 100644 docs/plans/dialog-content-containment/task-04-plugin-dialog.md create mode 100644 docs/plans/dialog-content-containment/task-05-create-project.md create mode 100644 docs/specs/ui/requirements/dialog-content-containment.md create mode 100644 docs/specs/ui/system-design/dialog-content-containment.md diff --git a/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js b/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js index 3bc29182fe..6ebd7cab89 100644 --- a/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js +++ b/apps/backend/cmd/plugin-fixture/fixture-package/ui/bundle.js @@ -760,6 +760,22 @@ registry.registerKeybinding("open-demo", function () { function DemoModalContent() { + var completedState = React.useState(false); + var completed = completedState[0]; + var setCompleted = completedState[1]; + var longRows = []; + for (var index = 0; index < 32; index += 1) { + longRows.push( + jsx( + "p", + { key: "long-modal-row-" + index }, + "Long plugin modal content row " + + String(index + 1) + + " keeps the opaque plugin surface growing beyond the viewport.", + ), + ); + } + // The Tooltip is the point of this modal in e2e: PluginModalHost // mounts outside AppShell, so without its own TooltipProvider a // Tooltip here throws on render and the error boundary swallows the @@ -767,7 +783,11 @@ // hover, so real pointer hover is only assertable here. return jsx( "div", - { id: "hello-demo-modal", "data-testid": "hello-demo-modal" }, + { + id: "hello-demo-modal", + "data-testid": "hello-demo-modal", + style: { display: "grid", gap: "8px" }, + }, "Hello from the plugin modal", jsx( ui.Tooltip, @@ -775,6 +795,19 @@ jsx(ui.TooltipTrigger, { "data-testid": "hello-modal-tooltip-trigger" }, "hover me"), jsx(ui.TooltipContent, null, "Tooltip inside a plugin modal"), ), + jsx("div", { "data-testid": "hello-long-modal-content" }, longRows), + jsx( + "button", + { + type: "button", + "data-testid": "hello-long-modal-final-action", + style: { minHeight: "44px", padding: "8px 12px" }, + onClick: function () { + setCompleted(true); + }, + }, + completed ? "Plugin modal action complete" : "Complete plugin modal action", + ), ); } host.openModal({ diff --git a/apps/web/app/office/projects/create-project-dialog.tsx b/apps/web/app/office/projects/create-project-dialog.tsx index 7254ca8dd8..badef924ab 100644 --- a/apps/web/app/office/projects/create-project-dialog.tsx +++ b/apps/web/app/office/projects/create-project-dialog.tsx @@ -311,30 +311,48 @@ export function CreateProjectDialog({ open, onOpenChange, workspaceId }: CreateP const { form, update, submitting, handleAddRepo, handleRemoveRepo, handleCreate } = useProjectForm(workspaceId, () => onOpenChange(false)); + // ProjectRepositoryPicker renders its popover inline. Paint clipping keeps + // that wide popover from creating a horizontal scroll range on the dialog. return ( - + {t("office:newProject")} - -
- diff --git a/apps/web/components/plugins/plugin-modal-host.test.tsx b/apps/web/components/plugins/plugin-modal-host.test.tsx index 2615d82308..1c86df939c 100644 --- a/apps/web/components/plugins/plugin-modal-host.test.tsx +++ b/apps/web/components/plugins/plugin-modal-host.test.tsx @@ -43,6 +43,48 @@ describe("PluginModalHost", () => { expect(screen.getByText("Use a Bitbucket pull request URL for this task.")).not.toBeNull(); }); + it("keeps dialog content in a local scroll body while retaining the close control", () => { + pluginModalManager.openModal("plugin-a", { + title: "Growing modal", + content: () =>
Long plugin content
, + }); + + render(); + + const dialog = screen.getByTestId(/^plugin-modal-dialog-/); + const body = screen.getByTestId(/^plugin-modal-body-/); + expect(dialog.getAttribute("data-layout")).toBe("contained"); + expect(dialog.contains(body)).toBe(true); + expect(screen.getByRole("button", { name: "Close" })).not.toBeNull(); + }); + + it("keeps titleless task-link content in the bounded scroll row", () => { + pluginModalManager.openTaskLinkDialog("plugin-a", { + content: () =>
Long task-link content
, + }); + + render(); + + const dialog = screen.getByTestId(/^plugin-modal-dialog-/); + const body = screen.getByTestId(/^plugin-modal-body-/); + expect(dialog.getAttribute("data-layout")).toBe("contained"); + expect(body.className).toContain("row-start-2"); + expect(screen.getByRole("dialog", { name: "Plugin dialog" })).toBeTruthy(); + }); + + it("does not add a close control to a nondismissible dialog", () => { + pluginModalManager.openModal("plugin-a", { + title: "Locked modal", + content: () =>
Content
, + dismissible: false, + }); + + render(); + + expect(screen.getByTestId(/^plugin-modal-dialog-/)).not.toBeNull(); + expect(screen.queryByRole("button", { name: "Close" })).toBeNull(); + }); + it("renders a host-owned drawer when the plugin requests mobile presentation", () => { pluginModalManager.openModal("plugin-a", { title: "Link pull request", diff --git a/apps/web/components/plugins/plugin-modal-host.tsx b/apps/web/components/plugins/plugin-modal-host.tsx index 24d8573626..8ae019a1cf 100644 --- a/apps/web/components/plugins/plugin-modal-host.tsx +++ b/apps/web/components/plugins/plugin-modal-host.tsx @@ -32,6 +32,9 @@ const SIZE_CLASSES: Record, string> = { xl: "sm:max-w-5xl", }; +const DIALOG_CONTAINMENT_CLASSES = + "max-h-[calc(100dvh-2rem)] max-w-[calc(100vw-2rem)] grid-rows-[auto_minmax(0,1fr)] overflow-hidden"; + function preventWhenNotDismissible(dismissible: boolean) { return (event: Event) => { if (!dismissible) event.preventDefault(); @@ -90,10 +93,12 @@ function PluginDialog({ modal, dismissible, onOpenChange }: ModalSurfaceProps) { {pluginDialogLabel()} )} - - - +
+ + + +
); diff --git a/apps/web/components/settings/agent-profile-delete-dialog.test.tsx b/apps/web/components/settings/agent-profile-delete-dialog.test.tsx index 5d1608ae06..76b3319dce 100644 --- a/apps/web/components/settings/agent-profile-delete-dialog.test.tsx +++ b/apps/web/components/settings/agent-profile-delete-dialog.test.tsx @@ -241,3 +241,23 @@ describe("AgentProfileDeleteConflictDialog", () => { expect(screen.queryByText(/Delete agent profile/i)).toBeNull(); }); }); + +describe("AgentProfileDeleteConflictDialog layout", () => { + it("keeps conflict details scrollable and decisions outside the scroll region", () => { + renderConflictDialog({ + activeSessions: [{ task_id: "t1", task_title: "Live task", is_ephemeral: false }], + watchers: [], + routingTiers: [], + automations: [], + }); + + const dialog = screen.getByTestId("agent-profile-delete-conflict-dialog"); + const body = screen.getByTestId("agent-profile-delete-conflict-body"); + const footer = screen.getByTestId("agent-profile-delete-conflict-footer"); + + expect(dialog.getAttribute("data-layout")).toBe("contained"); + expect(dialog.contains(body)).toBe(true); + expect(body.contains(footer)).toBe(false); + expect(within(footer).getByRole("button", { name: "Cancel" })).toBeTruthy(); + }); +}); diff --git a/apps/web/components/settings/agent-profile-delete-dialog.tsx b/apps/web/components/settings/agent-profile-delete-dialog.tsx index 3ef70622a4..c730550a26 100644 --- a/apps/web/components/settings/agent-profile-delete-dialog.tsx +++ b/apps/web/components/settings/agent-profile-delete-dialog.tsx @@ -171,49 +171,59 @@ export function AgentProfileDeleteConflictDialog({ return ( - + {hasHardBlockers ? t("agents:cannotDeleteAgentProfile") : t(DELETE_PROFILE_TITLE_KEY)} - -
-

{t("agents:profileInUseIntro")}

- - - - [w.id, w.name]))} - providerLabels={new Map(providers.map((p) => [p.id, p.name]))} - /> - [w.id, w.name]))} - /> - - {hasHardBlockers ? ( -

{t("agents:changeTierMappingsFirst")}

- ) : ( -

{t("agents:deleteAnywayConsequences")}

- )} -
- - - {t(CANCEL_LABEL_KEY)} + +
+

{t("agents:profileInUseIntro")}

+ + + + [w.id, w.name]))} + providerLabels={new Map(providers.map((p) => [p.id, p.name]))} + /> + [w.id, w.name]))} + /> + + {hasHardBlockers ? ( +

{t("agents:changeTierMappingsFirst")}

+ ) : ( +

{t("agents:deleteAnywayConsequences")}

+ )} +
+ + + + {t(CANCEL_LABEL_KEY)} + {hasHardBlockers ? null : ( {t("agents:deleteAnyway")} diff --git a/apps/web/components/settings/plugins/marketplace-sources-dialog.test.tsx b/apps/web/components/settings/plugins/marketplace-sources-dialog.test.tsx new file mode 100644 index 0000000000..9bf8603a47 --- /dev/null +++ b/apps/web/components/settings/plugins/marketplace-sources-dialog.test.tsx @@ -0,0 +1,79 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; +import { MarketplaceSourcesDialog } from "./marketplace-sources-dialog"; +import type { MarketplaceSource } from "@/lib/types/plugins"; + +const marketplaceApi = vi.hoisted(() => ({ + addMarketplaceSource: vi.fn(), + deleteMarketplaceSource: vi.fn(), + updateMarketplaceSource: vi.fn(), +})); + +vi.mock("@/lib/api/domains/marketplace-api", () => marketplaceApi); + +afterEach(cleanup); + +beforeEach(() => { + vi.clearAllMocks(); + marketplaceApi.deleteMarketplaceSource.mockResolvedValue({ deleted: true }); + marketplaceApi.updateMarketplaceSource.mockResolvedValue({}); +}); + +function source(overrides: Partial = {}): MarketplaceSource { + return { + id: "team", + name: "Team Registry", + url: "https://registry.example/team/index.json", + enabled: true, + builtin: false, + healthy: true, + ...overrides, + }; +} + +function renderDialog(sources: MarketplaceSource[] = [source()]) { + return render( + , + ); +} + +describe("MarketplaceSourcesDialog", () => { + it("keeps source actions wired to their source while preserving the add form", async () => { + renderDialog(); + + expect(screen.getByTestId("marketplace-source-team")).toBeTruthy(); + expect(screen.getByTestId("marketplace-add-source-name")).toBeTruthy(); + expect(screen.getByTestId("marketplace-add-source-url")).toBeTruthy(); + + fireEvent.click(screen.getByRole("switch")); + await waitFor(() => + expect(marketplaceApi.updateMarketplaceSource).toHaveBeenCalledWith("team", { + enabled: false, + }), + ); + + fireEvent.click(screen.getByRole("button", { name: /Remove/i })); + await waitFor(() => + expect(marketplaceApi.deleteMarketplaceSource).toHaveBeenCalledWith("team"), + ); + }); + + it("exposes one scroll body with the add form and touch-sized row actions outside it", () => { + renderDialog([ + source({ id: "official", name: "Official", builtin: true }), + source({ id: "team-2", name: "Team Two" }), + ]); + + const dialog = screen.getByTestId("marketplace-sources-dialog"); + const sourceList = screen.getByTestId("marketplace-sources-list"); + const addForm = screen.getByTestId("marketplace-add-source-form"); + const remove = within(screen.getByTestId("marketplace-source-team-2")).getByRole("button", { + name: /Remove/i, + }); + + expect(dialog.getAttribute("data-layout")).toBe("contained"); + expect(dialog.contains(sourceList)).toBe(true); + expect(sourceList.contains(addForm)).toBe(false); + expect(remove).toBeTruthy(); + }); +}); diff --git a/apps/web/components/settings/plugins/marketplace-sources-dialog.tsx b/apps/web/components/settings/plugins/marketplace-sources-dialog.tsx index 7a308c731f..c16a770a43 100644 --- a/apps/web/components/settings/plugins/marketplace-sources-dialog.tsx +++ b/apps/web/components/settings/plugins/marketplace-sources-dialog.tsx @@ -43,7 +43,11 @@ export function MarketplaceSourcesDialog({ const { t } = useTranslation(); return ( - + {t("plugins:marketplaceSources")} @@ -52,7 +56,10 @@ export function MarketplaceSourcesDialog({ -
+
{sources.map((source) => ( ))} @@ -121,7 +128,7 @@ function SourceItem({ source, onChanged }: { source: MarketplaceSource; onChange size="icon" disabled={busy} onClick={remove} - className="cursor-pointer" + className="min-h-11 min-w-11 cursor-pointer sm:min-h-0 sm:min-w-0" aria-label={t("plugins:removeSource", { name: source.name })} > @@ -154,24 +161,29 @@ function AddSourceForm({ onChanged }: { onChanged: () => void }) { }; return ( -
+
setName(e.target.value)} + className="min-h-11 sm:min-h-7" data-testid="marketplace-add-source-name" />
setUrl(e.target.value)} + className="min-h-11 sm:min-h-7" data-testid="marketplace-add-source-url" />