From de5a182e5db6699e6d5f152e98fcd17a075a6b9c Mon Sep 17 00:00:00 2001 From: Shaw Date: Thu, 2 Jul 2026 02:05:14 -0400 Subject: [PATCH] =?UTF-8?q?fix(ui,app):=20PR=20#11174=20review=20follow-up?= =?UTF-8?q?s=20=E2=80=94=20no-dock=20launcher=20spec=20+=20Escape=20deferr?= =?UTF-8?q?al=20for=20the=20desktop=20notification=20panel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two required fixes from the #11174 review, landed as a follow-up since the PR was squash-merged before they could be pushed to its branch: 1. packages/app/test/ui-smoke/launcher-interaction.spec.ts still asserted the removed featured-views dock (docked chat/settings, absent from page 0). Updated to the new contract: no launcher-dock element; Chat/Settings are ordinary page tiles leading the curated Apps page (launcher-curation.ts APPS_PAGE_ORDER); chat launches from its page tile. Evidence dir renamed 9144-default-launcher-dock -> 9144-launcher-page-tiles; swipe helper made bidirectional so the test returns to page 0 before launching. 2. ContinuousChatOverlay Escape-deferral selector matched only [data-testid="notification-sheet"], not the new desktop anchored [data-testid="notification-panel"] (role=dialog, no data-state=open), so Escape with the panel open over an open chat sheet closed both. Added the panel testid + a unit test mirroring the transcript-viewer deferral test. Co-Authored-By: Claude Fable 5 --- .../ui-smoke/launcher-interaction.spec.ts | 74 +++++++++++-------- .../shell/ContinuousChatOverlay.test.tsx | 24 ++++++ .../shell/ContinuousChatOverlay.tsx | 6 +- 3 files changed, 71 insertions(+), 33 deletions(-) diff --git a/packages/app/test/ui-smoke/launcher-interaction.spec.ts b/packages/app/test/ui-smoke/launcher-interaction.spec.ts index e711e5f90795b..e8a8d71e8f6a2 100644 --- a/packages/app/test/ui-smoke/launcher-interaction.spec.ts +++ b/packages/app/test/ui-smoke/launcher-interaction.spec.ts @@ -12,7 +12,7 @@ const OUT_DIR = path.join( process.cwd(), ".github", "issue-evidence", - "9144-default-launcher-dock", + "9144-launcher-page-tiles", ); async function screenshot(page: Page, name: string): Promise { @@ -49,16 +49,19 @@ async function tileIds(scope: Locator): Promise { ); } -async function advanceLauncherPage( +async function swipeLauncherPage( page: Page, + direction: "next" | "prev", ): Promise<"pointer-swipe" | "edge-button"> { const pageWindow = page.getByTestId("launcher-page-window"); - const secondPage = page.getByTestId("launcher-page-1"); + const targetPage = page.getByTestId( + direction === "next" ? "launcher-page-1" : "launcher-page-0", + ); const box = await pageWindow.boundingBox(); if (!box) throw new Error("launcher page window is not laid out"); const y = box.y + box.height * 0.52; - const startX = box.x + box.width * 0.82; - const endX = box.x + box.width * 0.16; + const startX = box.x + box.width * (direction === "next" ? 0.82 : 0.16); + const endX = box.x + box.width * (direction === "next" ? 0.16 : 0.82); const pointer = { bubbles: true, cancelable: true, @@ -88,18 +91,18 @@ async function advanceLauncherPage( clientX: endX, clientY: y + 2, }); - const swiped = await secondPage + const swiped = await targetPage .evaluate((node) => node.getAttribute("aria-hidden") === "false") .catch(() => false); if (swiped) return "pointer-swipe"; - const next = page.getByTestId("launcher-pager-edge-next"); - if ((await next.count()) === 0) { + const edgeButton = page.getByTestId(`launcher-pager-edge-${direction}`); + if ((await edgeButton.count()) === 0) { throw new Error( - "launcher pointer swipe did not advance and no next pager button rendered", + `launcher pointer swipe did not move to the ${direction} page and no ${direction} pager button rendered`, ); } - await next.click(); + await edgeButton.click(); return "edge-button"; } @@ -107,16 +110,19 @@ async function advanceLauncherPage( * Interaction-level coverage for the iOS-like view catalog (Launcher, #8796). * * Unlike builtin-views-visual.spec (which only asserts each view boots without - * crashing), this drives the catalog's actual controls — seeded default dock, - * swipe paging, and tap-to-launch — against a live app boot. Run with - * E2E_RECORD=1 to capture a video walkthrough. + * crashing), this drives the catalog's actual controls against a live app + * boot. The launcher has NO dock / featured-views surface (#11174): every view + * is an ordinary tile on the swipeable pages, with the curated "Apps" page + * (page 0) leading with Chat and Settings. Covered here: the no-dock contract, + * curated page-0 ordering, swipe paging, and tap-to-launch of the Chat tile. + * Run with E2E_RECORD=1 to capture a video walkthrough. */ test.describe("launcher catalog interactions", () => { for (const viewport of [ { name: "desktop", size: { width: 1440, height: 1000 } }, { name: "mobile", size: { width: 390, height: 844 } }, ] as const) { - test(`default dock, swipe paging, and dock Chat launch on ${viewport.name}`, async ({ + test(`no dock, page tiles, swipe paging, and Chat tile launch on ${viewport.name}`, async ({ page, }, testInfo) => { const consoleLines: string[] = []; @@ -142,15 +148,14 @@ test.describe("launcher catalog interactions", () => { await expect(page.getByTestId("launcher")).toBeVisible({ timeout: 60_000, }); - const dock = page.getByTestId("launcher-dock"); const firstPage = page.getByTestId("launcher-page-0"); - await expect(dock).toBeVisible(); - await expect(dock.getByTestId("launcher-tile-chat")).toBeVisible(); - await expect(dock.getByTestId("launcher-tile-settings")).toBeVisible(); - await expect(firstPage.getByTestId("launcher-tile-chat")).toHaveCount(0); - await expect(firstPage.getByTestId("launcher-tile-settings")).toHaveCount( - 0, - ); + // The featured-views dock was removed (#11174): no dock element exists, + // and Chat/Settings are ordinary page tiles at the head of page 0. + await expect(page.getByTestId("launcher-dock")).toHaveCount(0); + await expect(firstPage.getByTestId("launcher-tile-chat")).toBeVisible(); + await expect( + firstPage.getByTestId("launcher-tile-settings"), + ).toBeVisible(); await expect( firstPage.locator('[data-testid^="launcher-tile-"]').first(), ).toBeVisible(); @@ -159,31 +164,38 @@ test.describe("launcher catalog interactions", () => { await expect(page.getByRole("button", { name: "Done" })).toHaveCount(0); await page.waitForTimeout(300); - await screenshot(page, `${viewport.name}-launcher-default-dock`); - const dockTileIds = await tileIds(dock); + await screenshot(page, `${viewport.name}-launcher-page-tiles`); const firstPageTileIds = await tileIds(firstPage); const secondPage = page.getByTestId("launcher-page-1"); let pageAdvanceMethod: "pointer-swipe" | "edge-button" | "single-page" = "single-page"; if ((await secondPage.count()) > 0) { - pageAdvanceMethod = await advanceLauncherPage(page); + pageAdvanceMethod = await swipeLauncherPage(page, "next"); await expect(secondPage).toHaveAttribute("aria-hidden", "false"); await page.waitForTimeout(300); await screenshot(page, `${viewport.name}-launcher-after-swipe`); } - await dock.getByTestId("launcher-tile-chat").locator("button").click(); + // Chat launches from its ordinary page tile. Return to page 0 first if + // the paging step above advanced (page-1 tiles are inert while inactive). + if (pageAdvanceMethod !== "single-page") { + await swipeLauncherPage(page, "prev"); + await expect(firstPage).toHaveAttribute("aria-hidden", "false"); + } + await firstPage + .getByTestId("launcher-tile-chat") + .locator("button") + .click(); await expect .poll(() => new URL(page.url()).hash + new URL(page.url()).pathname) .toContain("/chat"); await expect(page.getByTestId("chat-composer-textarea")).toBeVisible(); await page.waitForTimeout(300); - await screenshot(page, `${viewport.name}-dock-chat-launched`); + await screenshot(page, `${viewport.name}-chat-tile-launched`); const evidence = { viewport: viewport.name, - dockTiles: dockTileIds, firstPageTiles: firstPageTileIds, pageAdvanceMethod, finalUrl: page.url(), @@ -191,9 +203,9 @@ test.describe("launcher catalog interactions", () => { httpErrors, consoleLines, }; - expect(evidence.dockTiles.slice(0, 2)).toEqual(["chat", "settings"]); - expect(evidence.firstPageTiles).not.toContain("chat"); - expect(evidence.firstPageTiles).not.toContain("settings"); + // Curated "Apps" page order leads with Chat then Settings + // (launcher-curation.ts APPS_PAGE_ORDER) — as page tiles, not a dock. + expect(evidence.firstPageTiles.slice(0, 2)).toEqual(["chat", "settings"]); expect(pageErrors, "no uncaught page errors").toEqual([]); expect(httpErrors, "no HTTP error responses").toEqual([]); diff --git a/packages/ui/src/components/shell/ContinuousChatOverlay.test.tsx b/packages/ui/src/components/shell/ContinuousChatOverlay.test.tsx index 0c040a6c0bbee..2e769c11bbfdc 100644 --- a/packages/ui/src/components/shell/ContinuousChatOverlay.test.tsx +++ b/packages/ui/src/components/shell/ContinuousChatOverlay.test.tsx @@ -915,6 +915,30 @@ describe("ContinuousChatOverlay", () => { expect(sheet.getAttribute("data-variant")).toBe("closed"); }); + it("lets the desktop notification panel own Escape — the chat only collapses once the panel is gone", () => { + render(); + const sheet = screen.getByTestId("chat-sheet"); + fireEvent.focus(screen.getByLabelText("message")); + expect(sheet.getAttribute("data-variant")).toBe("open"); + + // The desktop anchored notification panel carries role="dialog" but NO + // data-state="open" (it is not a Radix dialog), so it wouldn't match the + // dialog guard — Escape must close IT alone, not also collapse the chat. + const panel = document.createElement("div"); + panel.setAttribute("role", "dialog"); + panel.setAttribute("data-testid", "notification-panel"); + document.body.appendChild(panel); + try { + fireEvent.keyDown(document.body, { key: "Escape" }); + expect(sheet.getAttribute("data-variant")).toBe("open"); + } finally { + panel.remove(); + } + // Panel gone: Escape collapses the chat as before. + fireEvent.keyDown(document.body, { key: "Escape" }); + expect(sheet.getAttribute("data-variant")).toBe("closed"); + }); + it("Escape closes an in-progress message edit without collapsing the whole sheet (#9148)", () => { render( { if (e.key === "Escape") { // An open Radix dialog (data-state="open" — e.g. the command palette) - // or the notification pull-down sheet (mounts only while open) sits + // or a notification surface (the mobile pull-down sheet or the desktop + // anchored panel — both mount only while open; the panel carries + // role="dialog" with NO data-state="open") sits // above the chat: let ITS Escape handling win — collapsing here too // closed both at once (e.g. an invisible palette + the chat). Scoped // to exactly these; broad role="dialog" would match always-mounted @@ -3359,7 +3361,7 @@ export function ContinuousChatOverlay({ // NOT also collapse the whole sheet + discard the in-progress edit. if ( document.querySelector( - '[role="dialog"][data-state="open"], [data-testid="notification-sheet"], [data-testid="transcript-viewer"], [data-testid="thread-line-edit-input"]', + '[role="dialog"][data-state="open"], [data-testid="notification-sheet"], [data-testid="notification-panel"], [data-testid="transcript-viewer"], [data-testid="thread-line-edit-input"]', ) ) { return;