From 51ed1fab4b248ccc8a14116ec558a445c60a605d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 7 Aug 2026 10:20:23 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20Keep=20Edit=20Action=20Fu?= =?UTF-8?q?lly=20Hidden=20While=20Streaming=20(#14687)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿฉน fix: Keep Edit Action Fully Hidden While Streaming #14677 stopped the row-hover reveal from un-hiding the edit button, but the pencil still shows as a dimmed ghost mid-generation. The shared Button primitive sets `disabled:opacity-50`, which compiles to `.disabled\:opacity-50:disabled` โ€” specificity (0,2,0). The hidden state used a plain `opacity-0` at (0,1,0), so the disabled style won and painted the icon at half opacity. Verified in Chromium against a running instance: only two opacity rules match the button, and the computed value was 0.5. Switching the hidden state to `!opacity-0` (Tailwind emits `opacity: 0 !important`) drops it to 0 while the sibling actions still reveal at 1 on hover. The existing unit test could not catch this: jsdom applies no stylesheet, so asserting class names never exercised the cascade. It now asserts the important modifier specifically, with a comment explaining why a bare `opacity-0` is insufficient. * ๐Ÿงช test: Browser guard for the hidden edit action The Jest spec can only assert class names โ€” jsdom applies no stylesheet, so it could not see `disabled:opacity-50` (0,2,0) outranking `opacity-0` (0,1,0) and repainting the hidden pencil at half opacity. That is exactly how the ghost survived #14677 with a green suite. Asserts computed opacity in a real browser mid-stream, and asserts the sibling Copy action is at opacity 1 in the same breath so a hover that silently failed to register cannot make the check pass for the wrong reason. Verified to fail on the pre-fix build with `Received: "0.5"`, and to pass 3/3 after. --- .../components/Chat/Messages/HoverButtons.tsx | 4 +- .../Messages/__tests__/HoverButtons.spec.tsx | 8 ++- e2e/specs/mock/hover-actions.spec.ts | 66 +++++++++++++++++++ 3 files changed, 75 insertions(+), 3 deletions(-) create mode 100644 e2e/specs/mock/hover-actions.spec.ts diff --git a/client/src/components/Chat/Messages/HoverButtons.tsx b/client/src/components/Chat/Messages/HoverButtons.tsx index 49ae50c4cd..b1a627953c 100644 --- a/client/src/components/Chat/Messages/HoverButtons.tsx +++ b/client/src/components/Chat/Messages/HoverButtons.tsx @@ -98,7 +98,9 @@ const HoverButton = memo( !isLast && isVisible && 'group-hover:opacity-100 group-focus-within:opacity-100 [@media(hover:hover)]:opacity-0', - !isVisible && 'pointer-events-none opacity-0', + /** `!` is load-bearing: the shared Button sets `disabled:opacity-50`, which outranks a + * plain `opacity-0` and would leave a dimmed ghost of the hidden action on screen. */ + !isVisible && 'pointer-events-none !opacity-0', 'focus-visible:ring-2 focus-visible:ring-text-primary focus-visible:outline-none', isActive && isVisible && 'active text-text-primary bg-surface-hover', className, diff --git a/client/src/components/Chat/Messages/__tests__/HoverButtons.spec.tsx b/client/src/components/Chat/Messages/__tests__/HoverButtons.spec.tsx index e19d7eb40e..4a6ab73c4e 100644 --- a/client/src/components/Chat/Messages/__tests__/HoverButtons.spec.tsx +++ b/client/src/components/Chat/Messages/__tests__/HoverButtons.spec.tsx @@ -62,9 +62,13 @@ describe('HoverButtons edit affordance', () => { const editButton = renderHoverButtons(true); expect(editButton).toBeDisabled(); - expect(editButton).toHaveClass('opacity-0', 'pointer-events-none'); + expect(editButton).toHaveClass('pointer-events-none'); expect(editButton.className).not.toMatch(/group-hover:opacity-100/); expect(editButton.className).not.toMatch(/group-focus-within:opacity-100/); + /** Must outrank the shared Button's `disabled:opacity-50`; a bare `opacity-0` loses to it, + * and jsdom applies no stylesheet, so the important modifier is what we can assert here. */ + expect(editButton).toHaveClass('!opacity-0'); + expect(editButton).not.toHaveClass('opacity-0'); }); it('reveals on row hover once the generation settles', () => { @@ -72,6 +76,6 @@ describe('HoverButtons edit affordance', () => { expect(editButton).toBeEnabled(); expect(editButton).toHaveClass('group-hover:opacity-100'); - expect(editButton).not.toHaveClass('pointer-events-none', 'opacity-0'); + expect(editButton).not.toHaveClass('pointer-events-none', 'opacity-0', '!opacity-0'); }); }); diff --git a/e2e/specs/mock/hover-actions.spec.ts b/e2e/specs/mock/hover-actions.spec.ts new file mode 100644 index 0000000000..a55e7915ae --- /dev/null +++ b/e2e/specs/mock/hover-actions.spec.ts @@ -0,0 +1,66 @@ +import { expect, test } from '@playwright/test'; +import type { Page } from '@playwright/test'; +import { + MOCK_ENDPOINTS, + NEW_CHAT_PATH, + messagesView, + selectMockEndpoint, + sendMessage, +} from './helpers'; + +/** + * Regression guard for the edit action leaking through mid-stream. + * + * The unit spec can only assert class names: jsdom applies no stylesheet, so it + * cannot see that the shared Button's `disabled:opacity-50` (specificity 0,2,0) + * outranks a plain `opacity-0` (0,1,0) and repaints the hidden pencil at half + * opacity. Only a real browser resolves that cascade, which is why this lives + * here rather than in Jest. + */ + +const uniqueLabel = (prefix: string) => + `${prefix}-${Date.now()}-${Math.floor(Math.random() * 1e4)}`; + +const userTurn = (page: Page) => + messagesView(page) + .locator('.message-render') + .filter({ has: page.locator('.user-turn') }) + .last(); + +const stopButton = (page: Page) => page.getByRole('button', { name: 'Stop generating' }); + +test.describe('message hover actions', () => { + test('keeps the edit action fully hidden while a generation streams', async ({ page }) => { + test.setTimeout(120000); + const label = uniqueLabel('hover-edit'); + + await page.goto(NEW_CHAT_PATH, { timeout: 10000 }); + await selectMockEndpoint(page, MOCK_ENDPOINTS[0]); + + const run = await sendMessage(page, `E2E_SLOW_REPLY:${label}`); + expect(run.ok()).toBeTruthy(); + await expect(messagesView(page).getByText('chunk-010')).toBeVisible({ timeout: 15000 }); + + const row = userTurn(page); + const editButton = row.locator('button[id^="edit-"]'); + const copyButton = row.getByRole('button', { name: 'Copy to clipboard' }); + + await row.hover(); + + /** Pin the window: if the stream already settled, the edit assertion below + * would be checking the wrong state and pass for the wrong reason. */ + await expect(stopButton(page)).toBeVisible(); + + /** The sibling action proves the row is genuinely hovered โ€” without it a + * broken hover would make the edit assertion pass for the wrong reason. */ + await expect(copyButton).toHaveCSS('opacity', '1'); + await expect(editButton).toHaveCSS('opacity', '0'); + await expect(editButton).toBeDisabled(); + + /** ...and the affordance must come back, or "hidden" would just be "gone". */ + await expect(stopButton(page)).toBeHidden({ timeout: 60000 }); + await row.hover(); + await expect(editButton).toBeEnabled(); + await expect(editButton).toHaveCSS('opacity', '1'); + }); +});