🩹 fix: Keep Edit Action Fully Hidden While Streaming (#14687)

* 🩹 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.
This commit is contained in:
Danny Avila 2026-08-07 10:20:23 -04:00 committed by GitHub
parent 1596df724a
commit 51ed1fab4b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 75 additions and 3 deletions

View file

@ -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,

View file

@ -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');
});
});

View file

@ -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');
});
});