Skip to content

fix(skills): remove the human-only menu treatment and its hover jitter #1236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
backnotprop merged 1 commit into from
Aug 7, 2026
Merged

fix(skills): remove the human-only menu treatment and its hover jitter #1236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 35 additions & 33 deletions packages/ui/components/CommentPopover.skillReferences.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,54 +373,56 @@ describe('CommentPopover skill references — no-preselection keyboard state mac
);

test.skipIf(!hasDom)(
'the human-only explanation is disclosed only on engagement, but stays reachable by AT',
'human-only skills render identically to every other row: no badge, no disclosure, no ARIA note',
async () => {
// The menu no longer surfaces humanOnly at pick time (the instructions
// ride along with the exported feedback automatically). The old
// hover-disclosed footer changed the bottom-anchored menu's height under
// the pointer and oscillated the hovered row every frame.
await mountPopover();
const el = textarea();
await type(el, '$plannotator-rev');
await type(el, '$a'); // animate, annotate-helper, humanizer, plannotator-review
expect(menu()).not.toBeNull();
// Nothing active: the explanation is not VISIBLE, even though the only
// row is human-only...
expect(document.querySelector('[data-skill-menu-disclosure="true"]')).toBeNull();
// ...but it is in the DOM (sr-only) and the row references it, so the
// state is exposed to assistive tech, not just as a visual badge.
const note = document.querySelector('[data-skill-menu-disclosure]');
expect(note).not.toBeNull();
expect(note!.className).toContain('sr-only');
expect(note!.textContent).toContain('cannot be invoked by a model');
expect(note!.textContent).toContain('included with your feedback');
const row = document.querySelector('[data-skill-item="plannotator-review"]')!;
expect(row.getAttribute('data-skill-item-human-only')).toBe('true');
expect(row.getAttribute('aria-describedby')).toBe(note!.id);
// Keyboard activation discloses it visibly.
await press(el, 'ArrowDown');
expect(document.querySelector('[data-skill-menu-disclosure="true"]')).not.toBeNull();
expect(document.querySelector('[data-skill-menu-disclosure]')).toBeNull();
const humanOnlyRow = document.querySelector('[data-skill-item="plannotator-review"]')!;
const plainRow = document.querySelector('[data-skill-item="animate"]')!;
expect(humanOnlyRow.hasAttribute('data-skill-item-human-only')).toBe(false);
expect(humanOnlyRow.hasAttribute('aria-describedby')).toBe(false);
expect(humanOnlyRow.className).toBe(plainRow.className);
expect(menu()!.textContent).not.toContain('human-only');
expect(menu()!.textContent).not.toContain('cannot be invoked');
},
);

test.skipIf(!hasDom)(
'pointer hover over a human-only row discloses the explanation WITHOUT arming Enter',
'REGRESSION (hover jitter): hovering any row, human-only included, leaves the menu markup untouched',
async () => {
await mountPopover();
const el = textarea();
await type(el, 'This costs $');
const row = document.querySelector('[data-skill-item="plannotator-review"]')!;
await act(async () => {
row.dispatchEvent(new Event('pointerover', { bubbles: true }));
});
// The disclosure is a visual affordance only: no activation, and Enter
// still means newline (the no-preselection invariant holds under hover).
expect(document.querySelector('[data-skill-menu-disclosure="true"]')).not.toBeNull();
expect(activeRow()).toBeNull();
const menuEl = menu() as HTMLElement;
const before = menuEl.outerHTML;
for (const name of ['plannotator-review', 'animate']) {
const row = document.querySelector(`[data-skill-item="${name}"]`)!;
await act(async () => {
row.dispatchEvent(new Event('pointermove', { bubbles: true }));
row.dispatchEvent(new Event('pointerover', { bubbles: true }));
row.dispatchEvent(new Event('pointerenter', { bubbles: true }));
});
// Hover must not change ANY rendered output — no class flip, no new
// element, no style change — so the menu cannot grow, re-measure, or
// shift the row out from under the pointer.
expect((menu() as HTMLElement).outerHTML).toBe(before);
expect(activeRow()).toBeNull(); // and it still never arms Enter
await act(async () => {
row.dispatchEvent(new Event('pointerout', { bubbles: true }));
row.dispatchEvent(new Event('pointerleave', { bubbles: true }));
});
expect((menu() as HTMLElement).outerHTML).toBe(before);
}
const enter = await press(el, 'Enter');
expect(enter.defaultPrevented).toBe(false);
expect(el.value).toBe('This costs $');
// Leaving the row folds the explanation back to sr-only.
await act(async () => {
row.dispatchEvent(new Event('pointerout', { bubbles: true }));
});
expect(document.querySelector('[data-skill-menu-disclosure="true"]')).toBeNull();
expect(document.querySelector('[data-skill-menu-disclosure]')).not.toBeNull();
},
);

Expand Down
49 changes: 47 additions & 2 deletions packages/ui/components/SkillReferenceMenu.placement.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,15 @@ const CommentPopover =
popoverMod?.CommentPopover as typeof import('./CommentPopover')['CommentPopover'];

// 12 filterable entries: `$` shows all 12 (natural height 480 under the 40px
// row stub, so the 256px cap engages), `$zeb` narrows to exactly one.
// row stub, so the 256px cap engages), `$zeb` narrows to exactly one. `zebra`
// is human-only: the menu must render (and place) it exactly like the rest.
const catalog: SkillCatalogEntry[] = [
...Array.from({ length: 11 }, (_, i) => ({
name: `alpha-${String(i).padStart(2, '0')}`,
root: 'claude' as const,
humanOnly: false,
})),
{ name: 'zebra', root: 'universal', humanOnly: false },
{ name: 'zebra', root: 'universal', humanOnly: true },
];

/** Stubbed per-row height. Layout does not exist in happy-dom; the component
Expand Down Expand Up @@ -352,6 +353,50 @@ describe('SkillReferenceMenu adaptive placement', () => {
},
);

test.skipIf(!hasDom)(
'REGRESSION (hover jitter): hovering any row, human-only included, changes neither the menu markup nor its committed placement',
async () => {
// The bug this pins down: hovering a human-only row used to disclose a
// warning footer, growing the bottom-anchored menu upward and shrinking
// the re-measured list clamp — the hovered row shifted out from under
// the pointer, hover ended, the footer collapsed, the row shifted back,
// and the cycle repeated every frame. Hover must not change any row's
// rendered output or the placement the component commits.
setInnerHeight(768);
await mountPopover();
const el = textarea();
await type(el, '$'); // all 12 rows, zebra (human-only) among them
stubGeometry({ top: 560, bottom: 750 });
await remeasure();
const menuEl = menu();
const before = menuEl.outerHTML;
const placementBefore = assertMenuInsideViewport();
for (const name of ['zebra', 'alpha-00']) {
const row = document.querySelector(`[data-skill-item="${name}"]`)!;
await act(async () => {
row.dispatchEvent(new Event('pointermove', { bubbles: true }));
row.dispatchEvent(new Event('pointerover', { bubbles: true }));
row.dispatchEvent(new Event('pointerenter', { bubbles: true }));
});
// Byte-identical markup: no class flip, no disclosed footer, no style
// change — nothing for the placement effect to re-measure differently.
expect(menu().outerHTML).toBe(before);
// And a forced re-measure with the pointer "resting" on the row still
// commits the same placement.
await remeasure();
const placementAfter = assertMenuInsideViewport();
expect(placementAfter.direction).toBe(placementBefore.direction);
expect(placementAfter.maxListHeight).toBe(placementBefore.maxListHeight);
expect(menu().outerHTML).toBe(before);
await act(async () => {
row.dispatchEvent(new Event('pointerout', { bubbles: true }));
row.dispatchEvent(new Event('pointerleave', { bubbles: true }));
});
expect(menu().outerHTML).toBe(before);
}
},
);

test.skipIf(!hasDom)(
'dragging the popover to the top edge flips an open menu below',
async () => {
Expand Down
74 changes: 17 additions & 57 deletions packages/ui/components/SkillReferenceMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useCallback, useEffect, useId, useLayoutEffect, useRef, useState } from 'react';
import React, { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react';
import type { SkillCatalogEntry } from '../utils/skillReferences';

/** Which skill root a row came from, shown as the right-aligned source column. */
Expand Down Expand Up @@ -65,14 +65,13 @@ function computeMenuPlacement(
* below depending on available viewport space (see computeMenuPlacement), and
* clamped so it never runs off screen. Each row: icon, bold name, dimmed
* inline description (ellipsis-truncated), right-aligned source root.
* Human-invocation-only skills stay listed and selectable at full strength,
* carrying only a quiet "human-only" pill after the name — the state is a
* property of the skill, not an error. The plain-language explanation (a model
* cannot invoke it, so its instructions ride along with the feedback) is
* disclosed progressively: it appears as a muted footer while such a row is
* active (keyboard) or pointer-hovered, and is otherwise kept screen-reader
* accessible via an sr-only description that human-only rows reference with
* aria-describedby. Hover reveal is purely visual and never activates a row.
* Human-invocation-only skills render identically to every other row: their
* instructions are injected into the exported feedback automatically, so the
* distinction needs no surfacing at pick time. (A hover-disclosed warning
* used to live here; because it changed the menu's height while the pointer
* sat over a row of a bottom-anchored menu, it oscillated the row under the
* cursor every frame. Hover must never change any row's rendered size or the
* menu's measured geometry — color-only hover styling is fine.)
*
* Activation is KEYBOARD-ONLY (see useSkillReferenceAutocomplete): the menu
* floats directly over the composer, exactly where the mouse rests while
Expand All @@ -86,21 +85,11 @@ export const SkillReferenceMenu: React.FC<SkillReferenceMenuProps> = ({
}) => {
const menuRef = useRef<HTMLDivElement>(null);
const listRef = useRef<HTMLDivElement>(null);
const humanOnlyNoteId = useId();
// Visual-only pointer hover, for progressively disclosing the human-only
// footer to mouse users. NEVER feeds activeIndex — hover must not arm Enter.
const [hoverIndex, setHoverIndex] = useState<number | null>(null);
const [placement, setPlacement] = useState<MenuPlacement>({
direction: 'above',
maxListHeight: MAX_LIST_HEIGHT,
});

// The list identity changes on every re-filter; a stale hover index would
// point at whichever row slid under the resting cursor.
useEffect(() => {
setHoverIndex(null);
}, [items]);

const measure = useCallback(() => {
const menuEl = menuRef.current;
const listEl = listRef.current;
Expand All @@ -109,8 +98,9 @@ export const SkillReferenceMenu: React.FC<SkillReferenceMenuProps> = ({
const anchor = menuEl?.parentElement;
if (!menuEl || !listEl || !anchor) return;
const rect = anchor.getBoundingClientRect();
// Non-list height the menu carries (border, human-only footer when
// disclosed; the sr-only variant is absolutely positioned and adds none).
// Non-list height the menu carries (border). This must stay constant
// while the pointer moves — hover-dependent chrome is what caused the
// jitter loop this component once shipped.
const chrome = Math.max(0, menuEl.offsetHeight - listEl.offsetHeight);
// scrollHeight reports full content height even while clamped.
const next = computeMenuPlacement(rect, listEl.scrollHeight, chrome, window.innerHeight);
Expand All @@ -122,9 +112,9 @@ export const SkillReferenceMenu: React.FC<SkillReferenceMenuProps> = ({
}, []);

// Re-measure on EVERY commit: the popover re-renders on drag moves, flips,
// filtering (item-count changes), and human-only footer disclosure, and each
// of those can change the geometry without any window event firing. The
// setState above is equality-guarded, so this converges instead of looping.
// and filtering (item-count changes), and each of those can change the
// geometry without any window event firing. The setState above is
// equality-guarded, so this converges instead of looping.
useLayoutEffect(() => {
measure();
});
Expand All @@ -147,14 +137,6 @@ export const SkillReferenceMenu: React.FC<SkillReferenceMenuProps> = ({
row?.scrollIntoView({ block: 'nearest' });
}, [activeIndex]);

const active = activeIndex !== null ? items[activeIndex] : undefined;
const hovered = hoverIndex !== null ? items[hoverIndex] : undefined;
const hasHumanOnly = items.some((item) => item.humanOnly);
// Progressive disclosure: the explanation surfaces while a human-only row is
// active (keyboard) or hovered (pointer); otherwise it stays sr-only so
// assistive tech can always reach it through aria-describedby.
const humanOnlyDisclosed = active?.humanOnly === true || hovered?.humanOnly === true;

return (
<div
ref={menuRef}
Expand All @@ -177,29 +159,21 @@ export const SkillReferenceMenu: React.FC<SkillReferenceMenuProps> = ({
type="button"
data-skill-item={item.name}
data-skill-item-active={index === activeIndex ? 'true' : undefined}
data-skill-item-human-only={item.humanOnly ? 'true' : undefined}
aria-describedby={item.humanOnly ? humanOnlyNoteId : undefined}
// Insert on pointerdown so the textarea never loses focus. A
// click is explicit selection; hover deliberately does NOT
// activate the row (see the component docblock) — enter/leave
// below only drive the visual footer disclosure.
// activate the row (see the component docblock), and its only
// styling is the color-only hover:bg — hover must never change
// a row's rendered size or the menu's geometry.
onPointerDown={(e) => {
e.preventDefault();
onSelect(index);
}}
onPointerEnter={() => setHoverIndex(index)}
onPointerLeave={() => setHoverIndex((prev) => (prev === index ? null : prev))}
className={`w-full flex items-center gap-2.5 px-2.5 py-2 rounded-lg text-left text-[13px] leading-snug transition-colors ${
index === activeIndex ? 'bg-muted' : 'hover:bg-muted/50'
}`}
>
<SkillIcon />
<span className="shrink-0 font-semibold text-foreground">{item.name}</span>
{item.humanOnly && (
<span className="shrink-0 px-1.5 py-px rounded-full bg-muted text-[9px] font-medium tracking-wide text-muted-foreground/80">
human-only
</span>
)}
<span className="min-w-0 flex-1 truncate text-muted-foreground">
{item.description ?? ''}
</span>
Expand All @@ -209,20 +183,6 @@ export const SkillReferenceMenu: React.FC<SkillReferenceMenuProps> = ({
</button>
))}
</div>
{hasHumanOnly && (
<div
id={humanOnlyNoteId}
data-skill-menu-disclosure={humanOnlyDisclosed ? 'true' : 'hidden'}
className={
humanOnlyDisclosed
? 'px-3 py-2 border-t border-border/40 text-[11px] leading-snug text-muted-foreground'
: 'sr-only'
}
>
This skill cannot be invoked by a model, so its instructions will be
included with your feedback for the agent to follow.
</div>
)}
</div>
);
};
Expand Down