diff --git a/apps/desktop/src/app/chat/sidebar/session-actions-menu.test.tsx b/apps/desktop/src/app/chat/sidebar/session-actions-menu.test.tsx new file mode 100644 index 000000000000..ef989a4d643a --- /dev/null +++ b/apps/desktop/src/app/chat/sidebar/session-actions-menu.test.tsx @@ -0,0 +1,120 @@ +import { atom } from 'nanostores' +import { cleanup, fireEvent, render, screen } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { SessionActionsMenu } from './session-actions-menu' + +afterEach(cleanup) + +// This file exists specifically to catch the regression flagged in #67500: +// SessionActionsMenu used to be composed as +// {children} +// with the caller wrapping ITS children in . Radix's `asChild` clones +// its single child and injects onClick/aria-haspopup/ref onto it — but Tip +// doesn't forward those extra props to whatever it wraps, so they were +// silently dropped and the menu could stop opening. Tip has since moved +// inside this component (wrapping DropdownMenuTrigger itself, not the other +// way around) — these tests exercise the REAL component end-to-end (no mock +// of DropdownMenu/Tip) so a future regression of this composition fails here. + +vi.mock('@/components/pane-shell/tree/store', () => ({ + closeAllTreeTabs: vi.fn(), + closeOtherTreeTabs: vi.fn(), + closeTreeTabsToRight: vi.fn(), + treeTabCloseTargets: vi.fn(() => null) +})) +vi.mock('@/hermes', () => ({ renameSession: vi.fn() })) +vi.mock('@/i18n', () => ({ + useI18n: () => ({ + t: { + common: { cancel: 'Cancel', close: 'Close', delete: 'Delete', save: 'Save' }, + sidebar: { + projects: { menuAppearance: 'Appearance', noColor: 'No color' }, + row: { + actionsFor: (title: string) => `Actions for ${title}`, + archive: 'Archive', + branchFrom: 'Branch from here', + copyId: 'Copy ID', + copyIdFailed: 'Failed to copy ID', + export: 'Export', + hideTabBar: 'Hide tab bar', + pin: 'Pin', + rename: 'Rename', + renameDesc: 'Rename this session', + renameFailed: 'Rename failed', + renameTitle: 'Rename session', + renamed: 'Renamed', + unpin: 'Unpin', + untitledPlaceholder: 'Untitled' + } + }, + zones: { closeAll: 'Close all', closeOthers: 'Close others', closeToRight: 'Close to the right' } + } + }) +})) +vi.mock('@/lib/haptics', () => ({ triggerHaptic: vi.fn() })) +vi.mock('@/lib/profile-color', () => ({ PROFILE_SWATCHES: [] })) +vi.mock('@/lib/session-export', () => ({ exportSession: vi.fn() })) +vi.mock('@/store/gateway', () => ({ activeGateway: vi.fn(() => null) })) +vi.mock('@/store/notifications', () => ({ notify: vi.fn(), notifyError: vi.fn() })) +vi.mock('@/store/session', () => ({ + $activeSessionId: atom(null), + $selectedStoredSessionId: atom(null), + $sessions: atom([]), + sessionMatchesStoredId: vi.fn(() => false), + sessionPinId: vi.fn((s: { id: string }) => s.id), + setSessions: vi.fn() +})) +vi.mock('@/store/session-color', () => ({ + $sessionColorOverrides: atom>({}), + setSessionColorOverride: vi.fn() +})) +vi.mock('@/store/session-states', () => ({ + $sessionTiles: atom([]), + openSessionTile: vi.fn() +})) +vi.mock('@/store/windows', () => ({ + canOpenSessionWindow: () => false, + openSessionInNewWindow: vi.fn() +})) + +function renderMenu() { + return render( + + + + ) +} + +describe('SessionActionsMenu', () => { + it('shows the tooltip label wired to the real trigger button', () => { + renderMenu() + + const trigger = screen.getByRole('button', { name: 'Actions for My session' }) + + expect(trigger.closest('[data-slot="tooltip-trigger"]')).toBeTruthy() + }) + + it('still opens the dropdown on click with the trigger wrapped in a Tip (#67500)', async () => { + renderMenu() + + const trigger = screen.getByRole('button', { name: 'Actions for My session' }) + + // Radix's dropdown trigger opens on pointerdown (not on the synthetic + // 'click' fireEvent alone would dispatch), so fire the full mouse + // sequence a real click produces. + fireEvent.pointerDown(trigger, { button: 0, pointerType: 'mouse' }) + fireEvent.pointerUp(trigger, { button: 0, pointerType: 'mouse' }) + fireEvent.click(trigger) + + // If Tip (now composed around DropdownMenuTrigger, not the other way + // round) ever stopped forwarding the asChild-injected props again, this + // menu would never open and these queries would throw instead of + // resolving. + expect(await screen.findByRole('menu')).toBeTruthy() + expect(screen.getByRole('menuitem', { name: /rename/i })).toBeTruthy() + expect(screen.getByRole('menuitem', { name: /archive/i })).toBeTruthy() + }) +}) diff --git a/apps/desktop/src/app/chat/sidebar/session-actions-menu.tsx b/apps/desktop/src/app/chat/sidebar/session-actions-menu.tsx index d60f10016fc4..43bde9b516fc 100644 --- a/apps/desktop/src/app/chat/sidebar/session-actions-menu.tsx +++ b/apps/desktop/src/app/chat/sidebar/session-actions-menu.tsx @@ -10,11 +10,15 @@ import { } from '@/components/pane-shell/tree/store' import { Button } from '@/components/ui/button' import { Codicon } from '@/components/ui/codicon' +import { ColorSwatches } from '@/components/ui/color-swatches' import { ContextMenu, ContextMenuContent, ContextMenuItem, ContextMenuSeparator, + ContextMenuSub, + ContextMenuSubContent, + ContextMenuSubTrigger, ContextMenuTrigger } from '@/components/ui/context-menu' import { CopyButton } from '@/components/ui/copy-button' @@ -31,16 +35,29 @@ import { DropdownMenuContent, DropdownMenuItem, DropdownMenuSeparator, + DropdownMenuSub, + DropdownMenuSubContent, + DropdownMenuSubTrigger, DropdownMenuTrigger } from '@/components/ui/dropdown-menu' import { Input } from '@/components/ui/input' +import { Tip } from '@/components/ui/tooltip' import { renameSession } from '@/hermes' import { useI18n } from '@/i18n' import { triggerHaptic } from '@/lib/haptics' +import { PROFILE_SWATCHES } from '@/lib/profile-color' import { exportSession } from '@/lib/session-export' import { activeGateway } from '@/store/gateway' import { notify, notifyError } from '@/store/notifications' -import { $activeSessionId, $selectedStoredSessionId, setSessions } from '@/store/session' +import { + $activeSessionId, + $selectedStoredSessionId, + $sessions, + sessionMatchesStoredId, + sessionPinId, + setSessions +} from '@/store/session' +import { $sessionColorOverrides, setSessionColorOverride } from '@/store/session-color' import { $sessionTiles, openSessionTile } from '@/store/session-states' import { canOpenSessionWindow, openSessionInNewWindow } from '@/store/windows' @@ -116,14 +133,30 @@ interface SessionActions { type MenuItem = typeof DropdownMenuItem | typeof ContextMenuItem -/** A menu flavour (dropdown / context) — item + separator components. */ +/** A menu flavour (dropdown / context) — item + separator + submenu components. */ interface MenuKit { Item: MenuItem Separator: typeof DropdownMenuSeparator | typeof ContextMenuSeparator + Sub: typeof DropdownMenuSub | typeof ContextMenuSub + SubTrigger: typeof DropdownMenuSubTrigger | typeof ContextMenuSubTrigger + SubContent: typeof DropdownMenuSubContent | typeof ContextMenuSubContent } -const DROPDOWN_KIT: MenuKit = { Item: DropdownMenuItem, Separator: DropdownMenuSeparator } -const CONTEXT_KIT: MenuKit = { Item: ContextMenuItem, Separator: ContextMenuSeparator } +const DROPDOWN_KIT: MenuKit = { + Item: DropdownMenuItem, + Separator: DropdownMenuSeparator, + Sub: DropdownMenuSub, + SubContent: DropdownMenuSubContent, + SubTrigger: DropdownMenuSubTrigger +} + +const CONTEXT_KIT: MenuKit = { + Item: ContextMenuItem, + Separator: ContextMenuSeparator, + Sub: ContextMenuSub, + SubContent: ContextMenuSubContent, + SubTrigger: ContextMenuSubTrigger +} interface ItemSpec { className?: string @@ -134,6 +167,27 @@ interface ItemSpec { variant?: 'destructive' } +// The color picker inside the session menu's Appearance submenu. Its own +// component so only an OPEN submenu subscribes to the stores (not every row's +// menu). Reads/writes the override keyed by the DURABLE id so a color survives +// compression; clearing falls back to the inherited project color. +function SessionColorSwatches({ sessionId }: { sessionId: string }) { + const { t } = useI18n() + const overrides = useStore($sessionColorOverrides) + const session = useStore($sessions).find(s => sessionMatchesStoredId(s, sessionId)) + const durableId = session ? sessionPinId(session) : sessionId + + return ( + setSessionColorOverride(durableId, color)} + swatches={PROFILE_SWATCHES} + value={overrides[durableId] ?? null} + /> + ) +} + function useSessionActions({ sessionId, title, @@ -326,6 +380,15 @@ function useSessionActions({ {openItems.map(item => renderMenuItem(kit.Item, item))} {openItems.length > 0 && } {identityItems.map(item => renderMenuItem(kit.Item, item))} + + + + {t.sidebar.projects.menuAppearance} + + + + + , 'align' | 'sideOffset'> { children: React.ReactNode + /** Tooltip label for the trigger. Composed INSIDE the dropdown trigger + * (Tip wraps DropdownMenuTrigger, not the other way around) — Tip doesn't + * forward the extra props/ref an `asChild` clone injects, so putting it as + * the trigger's direct child silently drops onClick/aria-haspopup/ref and + * the menu stops opening (#67500). */ + tooltip?: React.ReactNode } -export function SessionActionsMenu({ children, align = 'end', sideOffset = 6, ...actions }: SessionActionsMenuProps) { +export function SessionActionsMenu({ + children, + tooltip, + align = 'end', + sideOffset = 6, + ...actions +}: SessionActionsMenuProps) { const { t } = useI18n() const { renameDialog, renderItems } = useSessionActions(actions) const [open, setOpen] = useState(false) @@ -389,7 +464,9 @@ export function SessionActionsMenu({ children, align = 'end', sideOffset = 6, .. return ( <> - {children} + + {children} + ({ vi.mock('@/app/chat/profile-tag', () => ({ ProfileTag: () => null })) vi.mock('@/app/chat/session-drag', () => ({ startSessionDrag: vi.fn() })) -vi.mock('@/app/messaging/platform-icon', () => ({ - PlatformAvatar: ({ platformName, ...rest }: { platformName: string } & Record) => ( - {platformName} - ) -})) +// PlatformAvatar is intentionally NOT mocked here (unlike before): it forwards +// ref/props for real now (#67500), so this file exercises the actual +// production component to verify its Tip wiring, instead of a stand-in that +// spreads props the real component didn't. vi.mock('@/lib/chat-runtime', () => ({ sessionTitle: (s: SessionInfo) => (s as unknown as { title: string }).title })) vi.mock('@/lib/haptics', () => ({ triggerHaptic: vi.fn() })) vi.mock('@/lib/session-source', () => ({ @@ -62,10 +63,22 @@ vi.mock('@/store/windows', () => ({ })) // SessionActionsMenu/SessionContextMenu carry their own menu-item deps -// (archive/pin/delete wiring) that are irrelevant here — this file only -// exercises the Tip fix, so pass their children straight through. +// (archive/pin/delete wiring, plus a dozen unrelated stores) that are +// irrelevant here — this file only exercises the Tip fix, so pass their +// children straight through. The Tip-wraps-DropdownMenuTrigger composition +// itself (and that the menu still opens) is covered directly against the +// real component in session-actions-menu.test.tsx (#67500) — that test would +// have caught the original regression; this passthrough mock intentionally +// does NOT re-verify it, to avoid masking a future regression the way the +// old inline mock here once did. +const sessionActionsMenuCalls: Array<{ tooltip?: React.ReactNode }> = [] + vi.mock('./session-actions-menu', () => ({ - SessionActionsMenu: ({ children }: { children: React.ReactNode }) => <>{children}, + SessionActionsMenu: (props: { children: React.ReactNode; tooltip?: React.ReactNode }) => { + sessionActionsMenuCalls.push({ tooltip: props.tooltip }) + + return <>{props.children} + }, SessionContextMenu: ({ children }: { children: React.ReactNode }) => <>{children} })) @@ -90,7 +103,9 @@ const tipTrigger = (el: HTMLElement) => el.closest('[data-slot="tooltip-trigger" const noop = vi.fn() describe('SidebarSessionRow', () => { - it('wraps the actions kebab in a Tip with the session title', () => { + it('passes the actions-kebab tooltip label through to SessionActionsMenu', () => { + sessionActionsMenuCalls.length = 0 + render( { /> ) - const button = screen.getByRole('button', { name: 'Actions for Hermes doctor health check results' }) - expect(tipTrigger(button)).toBeTruthy() + // The Tip itself now lives INSIDE SessionActionsMenu (composed around + // DropdownMenuTrigger, not around the children it receives) — see + // session-actions-menu.test.tsx for proof the menu still opens with it + // there. This just confirms session-row hands over the right label. + expect(sessionActionsMenuCalls[0]?.tooltip).toBe('Actions for Hermes doctor health check results') }) it('does not render a handoff avatar for a locally-started session', () => { - render( + const { container } = render( { /> ) - expect(screen.queryByText('telegram')).toBeNull() + // PlatformAvatar's span is the only aria-hidden SPAN this row ever + // renders (idle dot / arc-border / branch-stem are all inactive here) — + // Codicon icons (e.g. the kebab trigger) are also aria-hidden but render + // as , not , so this selector doesn't accidentally match them. + expect(container.querySelector('span[aria-hidden="true"]')).toBeNull() }) it('wraps the handoff platform avatar in a Tip for a session started on another platform', () => { - render( + const { container } = render( { /> ) - // PlatformAvatar is stubbed to render its platformName as text, and - // sessionSourceLabel is mocked as an identity function, so the visible - // text is the raw platform id. - const avatar = screen.getByText('telegram') - expect(tipTrigger(avatar)).toBeTruthy() + // PlatformAvatar is now the REAL component (see the note above the + // vi.mock block), which renders the Telegram brand SVG rather than the + // platform name as text — so query the avatar span itself (it's the row's + // only aria-hidden element in this state) rather than text content, and + // confirm its tooltip trigger actually attaches to it, not to a mock that + // faked the wiring (#67500). + const avatar = container.querySelector('span[aria-hidden="true"]') + expect(avatar).toBeTruthy() + expect(tipTrigger(avatar as HTMLElement)).toBeTruthy() }) }) diff --git a/apps/desktop/src/app/chat/sidebar/session-row.tsx b/apps/desktop/src/app/chat/sidebar/session-row.tsx index 61ae3319148c..4b81c1ef4aa9 100644 --- a/apps/desktop/src/app/chat/sidebar/session-row.tsx +++ b/apps/desktop/src/app/chat/sidebar/session-row.tsx @@ -137,17 +137,16 @@ export function SidebarSessionRow({ profile={session.profile} sessionId={session.id} title={title} + tooltip={r.actionsFor(title)} > - - - + } diff --git a/apps/desktop/src/app/messaging/platform-icon.tsx b/apps/desktop/src/app/messaging/platform-icon.tsx index 4a6be4354dbb..68bb6697fc5a 100644 --- a/apps/desktop/src/app/messaging/platform-icon.tsx +++ b/apps/desktop/src/app/messaging/platform-icon.tsx @@ -12,7 +12,8 @@ import { SiWechat, SiWhatsapp } from '@icons-pack/react-simple-icons' -import type { ComponentType, SVGProps } from 'react' +import type { ComponentPropsWithoutRef, ComponentType, SVGProps } from 'react' +import { forwardRef } from 'react' import { Globe, Link as LinkIcon, MessageSquareText } from '@/lib/icons' import { cn } from '@/lib/utils' @@ -54,13 +55,20 @@ const PLATFORM_ICONS: Record = { yuanbao: { Icon: SiBilibili, color: '#FB7299', kind: 'brand' } } -interface PlatformAvatarProps { +interface PlatformAvatarProps extends Omit, 'children'> { platformId: string platformName: string - className?: string } -export function PlatformAvatar({ className, platformId, platformName }: PlatformAvatarProps) { +// forwardRef + spreading ...rest is required so a wrapping (Radix +// Tooltip's `asChild`) can actually attach its trigger: asChild clones this +// component and injects a ref plus pointer/focus/aria handlers onto it. A +// plain function component with no ref/rest forwarding drops all of that +// silently — the tooltip renders but never opens (#67500). +export const PlatformAvatar = forwardRef(function PlatformAvatar( + { className, platformId, platformName, style, ...rest }, + ref +) { const spec = PLATFORM_ICONS[platformId] const baseClass = cn( @@ -70,7 +78,13 @@ export function PlatformAvatar({ className, platformId, platformName }: Platform if (!spec) { return ( -