From 5919465f6c5e867f5f67718a624ce85c658cdba9 Mon Sep 17 00:00:00 2001 From: dovvnloading Date: Tue, 28 Jul 2026 21:49:05 -0400 Subject: [PATCH] R8a: fix the app bar overflowing off-screen below ~1120px window width Below roughly 1120px, AppBar's 13 controls (12 buttons plus a dead provider-mode select) had no shrink/wrap/overflow handling at all. The flex toolbar could never shrink below its own min-content width, so it forced the whole document wider than the window - Settings, About, Help and the connection status indicator ran off the right edge, and a horizontal scrollbar appeared across the ENTIRE app, dragging the canvas and composer sideways with it. Also removes the provider-mode from finding #8 +// is genuinely gone; every real button (both the inline copy and its +// overflow-menu duplicate) is present and calls the correct handler; the +// overflow menu opens/closes correctly; and every duplicated pair carries +// the same data-tier value, since a mismatch there would silently break the +// CSS wiring in a way no visual glance at one window size would catch. + +function makeStore(): SceneStore { + const transport = { subscribe: vi.fn(), intent: vi.fn() } as unknown as WsTransport; + return new SceneStore(transport); +} + +function renderAppBar(store = makeStore()) { + render( + + + + + , + ); + return { store }; +} + +describe("AppBar", () => { + it("no longer renders the provider-mode select (finding #8) - Settings is the only provider surface", () => { + renderAppBar(); + expect(screen.queryByRole("combobox")).toBeNull(); + expect(screen.queryByText("Ollama (Local)")).toBeNull(); + expect(screen.queryByTitle("Switching provider modes isn't available yet")).toBeNull(); + }); + + it("Library, Save and Settings are present as plain toolbar buttons (the tier that never collapses)", () => { + renderAppBar(); + expect(screen.getByRole("button", { name: "Library" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Save" })).toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Settings" })).toBeInTheDocument(); + // None of the three ever has a data-tier - confirmed by absence, not a + // positive class check, since "never collapses" IS "has no tier". + for (const label of ["Library", "Save", "Settings"]) { + expect(screen.getByRole("button", { name: label })).not.toHaveAttribute("data-tier"); + } + }); + + it("Save calls store.saveChat", async () => { + const user = userEvent.setup(); + const { store } = renderAppBar(); + const spy = vi.spyOn(store, "saveChat"); + await user.click(screen.getByRole("button", { name: "Save" })); + expect(spy).toHaveBeenCalledOnce(); + }); + + it("Settings toggles real overlay-open state, not latched click state (audit B6)", async () => { + const user = userEvent.setup(); + renderAppBar(); + const settings = screen.getByRole("button", { name: "Settings" }); + expect(settings.className).not.toContain("checked"); + await user.click(settings); + expect(settings.className).toContain("checked"); + await user.click(settings); + expect(settings.className).not.toContain("checked"); + }); + + describe("overflow menu (finding #5)", () => { + it("does not render until the trigger is clicked", () => { + renderAppBar(); + expect(screen.queryByRole("dialog")).toBeNull(); + }); + + it("opens on trigger click and closes on a second click", async () => { + const user = userEvent.setup(); + renderAppBar(); + const trigger = screen.getByRole("button", { name: "More toolbar actions" }); + expect(trigger).toHaveAttribute("aria-expanded", "false"); + // Popover (overlays.tsx) renders role="dialog", not role="menu" - the + // trigger's aria-haspopup must say so too, or a screen reader + // announces a control that opens something other than what it does. + expect(trigger).toHaveAttribute("aria-haspopup", "dialog"); + + await user.click(trigger); + expect(screen.getByRole("dialog")).toBeInTheDocument(); + expect(trigger).toHaveAttribute("aria-expanded", "true"); + + await user.click(trigger); + expect(screen.queryByRole("dialog")).toBeNull(); + expect(trigger).toHaveAttribute("aria-expanded", "false"); + }); + + it("closes on Escape", async () => { + const user = userEvent.setup(); + renderAppBar(); + await user.click(screen.getByRole("button", { name: "More toolbar actions" })); + expect(screen.getByRole("dialog")).toBeInTheDocument(); + await user.keyboard("{Escape}"); + expect(screen.queryByRole("dialog")).toBeNull(); + }); + + it("contains a duplicate of every collapsible button", async () => { + const user = userEvent.setup(); + renderAppBar(); + + await user.click(screen.getByRole("button", { name: "More toolbar actions" })); + const menuItems = within(screen.getByRole("dialog")); + + for (const label of [ + "Export PNG", + "Pins", + "Organize", + "View", + "Plugins", + "Zoom In", + "Zoom Out", + "Reset", + "Fit All", + "About", + "Help", + ]) { + expect(menuItems.getByRole("button", { name: label })).toBeInTheDocument(); + } + }); + + // R8a: every "plain" action (doesn't open an overlay of its own) is + // tested individually, not just one "representative" action - Export + // PNG's overflow copy shipped without its closeOverflow() call at + // first, and a test that only checked Organize passed anyway, since + // Organize happened to be correct. One shared assertion per action + // closes that exact gap for good, rather than re-opening it the next + // time a plain action is added. + it.each(["Export PNG", "Organize", "Zoom In", "Zoom Out", "Reset", "Fit All"])( + "clicking the overflow copy of a plain action (%s) closes the menu", + async (label) => { + const user = userEvent.setup(); + renderAppBar(); + await user.click(screen.getByRole("button", { name: "More toolbar actions" })); + await user.click(within(screen.getByRole("dialog")).getByRole("button", { name: label })); + expect(screen.queryByRole("dialog")).toBeNull(); + }, + ); + + it("Organize's overflow copy calls store.organizeNodes", async () => { + const user = userEvent.setup(); + const { store } = renderAppBar(); + const organizeSpy = vi.spyOn(store, "organizeNodes"); + await user.click(screen.getByRole("button", { name: "More toolbar actions" })); + await user.click(within(screen.getByRole("dialog")).getByRole("button", { name: "Organize" })); + expect(organizeSpy).toHaveBeenCalledOnce(); + }); + + it("clicking an overlay-opening item (About) opens that overlay and closes the menu", async () => { + const user = userEvent.setup(); + renderAppBar(); + await user.click(screen.getByRole("button", { name: "More toolbar actions" })); + await user.click(within(screen.getByRole("dialog")).getByRole("button", { name: "About" })); + + expect(screen.queryByRole("dialog")).toBeNull(); + // The inline About chip (still in the DOM, jsdom does not evaluate + // the CSS that would visually hide it) now reads real open state. + expect(screen.getByRole("button", { name: "About" }).className).toContain("checked"); + }); + + it("the five overlay-opening overflow items (Pins/View/Plugins/About/Help) carry aria-pressed, matching their inline copies' real-state contract", async () => { + // Cannot be driven to aria-pressed="true" through normal interaction + // in this test: OverlayProvider is single-open, so the instant any of + // these becomes the open surface, "toolbar-overflow" stops being it + // and this whole menu unmounts. This only pins the attribute exists + // (defaulting false) rather than being silently absent, the way it + // was before - see AppBar.tsx's own comment on why it's kept anyway. + const user = userEvent.setup(); + renderAppBar(); + await user.click(screen.getByRole("button", { name: "More toolbar actions" })); + const menu = within(screen.getByRole("dialog")); + for (const label of ["Pins", "View", "Plugins", "About", "Help"]) { + expect(menu.getByRole("button", { name: label })).toHaveAttribute("aria-pressed", "false"); + } + }); + + it("every overflow-menu item shares its data-tier with the matching inline button, and only the always-visible three (Library/Save/Settings) have no overflow duplicate at all", async () => { + const user = userEvent.setup(); + renderAppBar(); + await user.click(screen.getByRole("button", { name: "More toolbar actions" })); + const menu = screen.getByRole("dialog"); + + const pairs: [string, string][] = [ + ["Export PNG", "1"], + ["Pins", "2"], + ["Organize", "2"], + ["View", "2"], + ["Plugins", "2"], + ["Zoom In", "3"], + ["Zoom Out", "3"], + ["Reset", "3"], + ["Fit All", "3"], + ["About", "1"], + ["Help", "1"], + ]; + for (const [label, tier] of pairs) { + // Every duplicated pair shares its exact label except Plugins (the + // inline button's accessible name includes its chevron span, + // "Plugins ▼", while its overflow duplicate is plain "Plugins" - a + // real, intentional difference, see AppBar.tsx) - so match by + // substring, then disambiguate the two hits by DOM position rather + // than by string, which stays correct for every label uniformly + // rather than special-casing the one that needs it. + const matches = screen.getAllByRole("button", { name: new RegExp(label) }); + const inline = matches.find((el) => !menu.contains(el)); + const overflowItem = matches.find((el) => menu.contains(el)); + expect(inline).toHaveAttribute("data-tier", tier); + expect(overflowItem).toHaveAttribute("data-tier", tier); + } + + for (const label of ["Library", "Save", "Settings"]) { + expect(within(menu).queryByRole("button", { name: label })).toBeNull(); + } + }); + }); +}); diff --git a/web_ui/src/app/chrome/AppBar.tsx b/web_ui/src/app/chrome/AppBar.tsx index 3cbbd62..ff10827 100644 --- a/web_ui/src/app/chrome/AppBar.tsx +++ b/web_ui/src/app/chrome/AppBar.tsx @@ -1,7 +1,7 @@ import { useReactFlow } from "@xyflow/react"; import { exportCanvasAsPng } from "../canvas/exportCanvasPng"; import type { SceneStore } from "../canvas/sceneStore"; -import { useOverlays } from "../overlays/overlays"; +import { Popover, useOverlays } from "../overlays/overlays"; /** * The app bar (Qt-removal plan R2) - the toolbar island's SPA successor. @@ -16,13 +16,63 @@ import { useOverlays } from "../overlays/overlays"; * - library/settings/about/help/plugins -> overlay dialogs; chips read REAL * open state from the overlay context (audit B6), never latched clicks * - saveChat -> real as of R6.5 (store.saveChat(), targets the - * app-chat-library topic - see SceneStore's own comment on why); provider - * mode select -> still deferred to R4's own remaining work, rendered - * disabled with the phase called out. + * app-chat-library topic - see SceneStore's own comment on why) * - Export PNG -> real as of R6.8, a net-new capability (no legacy * canvas-wide export exists) - pure client-side DOM rasterization via * exportCanvasAsPng, same "zero backend round-trip" shape as the * Zoom/Fit All buttons right next to it, not an intent dispatch. + * + * R8a (UI/UX issue list finding #8): the provider-mode {}} + {/* Tier-gated the same way as every collapsible button above: CSS + only shows this once at least one tier is hidden, so it never + appears as a "..." button opening an empty menu at full width. */} + + + + + + + + + + + + + +