fix(desktop): compose Tip around the real trigger instead of inside it

Tip was being placed as SessionActionsMenu's/PlatformAvatar's DIRECT child, which asChild then cloned instead of the actual button/span. Neither Tip nor PlatformAvatar forwarded the injected onClick/ref, so both silently dropped the wiring: - session-actions-menu.tsx: Tip now wraps DropdownMenuTrigger internally (new ooltip prop) instead of the caller wrapping its children in Tip. - platform-icon.tsx: PlatformAvatar now forwards ref and spreads rest props onto its span so a wrapping Tip's trigger actually attaches. - session-row.tsx: updated call site to use the new tooltip prop. - Added session-actions-menu.test.tsx exercising the real DropdownMenu open behavior end-to-end (no Tip/Dropdown mocks). - session-row.test.tsx no longer mocks PlatformAvatar's behavior; it now exercises the real (fixed) component for the handoff-avatar tooltip.
2026-07-23 16:36:23 +00:00 · 2026-07-20 11:34:57 +03:00 · 2026-07-20 11:34:57 +03:00 · cbbbeb2fdb
commit cbbbeb2fdb
parent ab0c131285
5 changed files with 282 additions and 43 deletions
--- a/apps/desktop/src/app/chat/sidebar/session-actions-menu.test.tsx
+++ 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
+//   <DropdownMenuTrigger asChild>{children}</DropdownMenuTrigger>
+// with the caller wrapping ITS children in <Tip>. 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 | string>(null),
+  $selectedStoredSessionId: atom<null | string>(null),
+  $sessions: atom<unknown[]>([]),
+  sessionMatchesStoredId: vi.fn(() => false),
+  sessionPinId: vi.fn((s: { id: string }) => s.id),
+  setSessions: vi.fn()
+}))
+vi.mock('@/store/session-color', () => ({
+  $sessionColorOverrides: atom<Record<string, string>>({}),
+  setSessionColorOverride: vi.fn()
+}))
+vi.mock('@/store/session-states', () => ({
+  $sessionTiles: atom<unknown[]>([]),
+  openSessionTile: vi.fn()
+}))
+vi.mock('@/store/windows', () => ({
+  canOpenSessionWindow: () => false,
+  openSessionInNewWindow: vi.fn()
+}))
+
+function renderMenu() {
+  return render(
+    <SessionActionsMenu sessionId="s1" title="My session" tooltip="Actions for My session">
+      <button aria-label="Actions for My session" type="button">
+        ⋮
+      </button>
+    </SessionActionsMenu>
+  )
+}
+
+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()
+  })
+})
--- 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 (
+    <ColorSwatches
+      clearIcon="circle-slash"
+      clearLabel={t.sidebar.projects.noColor}
+      onChange={color => 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 && <kit.Separator />}
      {identityItems.map(item => renderMenuItem(kit.Item, item))}
+      <kit.Sub>
+        <kit.SubTrigger disabled={!sessionId}>
+          <Codicon name="symbol-color" size="0.875rem" />
+          <span>{t.sidebar.projects.menuAppearance}</span>
+        </kit.SubTrigger>
+        <kit.SubContent className="p-2">
+          <SessionColorSwatches sessionId={sessionId} />
+        </kit.SubContent>
+      </kit.Sub>
      <CopyButton
        appearance={kit.Item === DropdownMenuItem ? 'menu-item' : 'context-menu-item'}
        disabled={!sessionId}
@ -379,9 +442,21 @@ function useSessionActions({
 interface SessionActionsMenuProps
  extends SessionActions, Pick<React.ComponentProps<typeof DropdownMenuContent>, '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 (
    <>
      <DropdownMenu onOpenChange={setOpen} open={open}>
-        <DropdownMenuTrigger asChild>{children}</DropdownMenuTrigger>
+        <Tip label={tooltip}>
+          <DropdownMenuTrigger asChild>{children}</DropdownMenuTrigger>
+        </Tip>
        <DropdownMenuContent
          align={align}
          aria-label={t.sidebar.row.actionsFor(actions.title)}
--- a/apps/desktop/src/app/chat/sidebar/session-row.test.tsx
+++ b/apps/desktop/src/app/chat/sidebar/session-row.test.tsx
@ -1,5 +1,7 @@
+import type * as React from 'react'
+
 import { atom } from 'nanostores'
-import { cleanup, render, screen } from '@testing-library/react'
+import { cleanup, render } from '@testing-library/react'
 import { afterEach, describe, expect, it, vi } from 'vitest'

 import type { SessionInfo } from '@/hermes'
@ -30,11 +32,10 @@ vi.mock('@/i18n', () => ({

 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<string, unknown>) => (
-    <span {...rest}>{platformName}</span>
-  )
-}))
+// 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 <Tip> 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(
      <SidebarSessionRow
        isPinned={false}
@ -104,12 +119,15 @@ describe('SidebarSessionRow', () => {
      />
    )

-    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(
      <SidebarSessionRow
        isPinned={false}
        isSelected={false}
@ -122,11 +140,15 @@ describe('SidebarSessionRow', () => {
      />
    )

-    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 <i>, not <span>, 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(
      <SidebarSessionRow
        isPinned={false}
        isSelected={false}
@ -143,10 +165,14 @@ describe('SidebarSessionRow', () => {
      />
    )

-    // 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()
  })
 })
--- 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)}
            >
-              <Tip label={r.actionsFor(title)}>
-                <Button
-                  aria-label={r.actionsFor(title)}
-                  className="size-5 rounded-[4px] bg-transparent text-transparent transition-colors duration-100 hover:bg-(--ui-control-active-background) hover:text-foreground focus-visible:bg-(--ui-control-active-background) focus-visible:text-foreground focus-visible:ring-0 data-[state=open]:bg-(--ui-control-active-background) data-[state=open]:text-foreground group-hover:text-(--ui-text-tertiary) [&_svg]:size-3.5!"
-                  size="icon"
-                  variant="ghost"
-                >
-                  <Codicon name="kebab-vertical" size="0.875rem" />
-                </Button>
-              </Tip>
+              <Button
+                aria-label={r.actionsFor(title)}
+                className="size-5 rounded-[4px] bg-transparent text-transparent transition-colors duration-100 hover:bg-(--ui-control-active-background) hover:text-foreground focus-visible:bg-(--ui-control-active-background) focus-visible:text-foreground focus-visible:ring-0 data-[state=open]:bg-(--ui-control-active-background) data-[state=open]:text-foreground group-hover:text-(--ui-text-tertiary) [&_svg]:size-3.5!"
+                size="icon"
+                variant="ghost"
+              >
+                <Codicon name="kebab-vertical" size="0.875rem" />
+              </Button>
            </SessionActionsMenu>
          </div>
        }
--- 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<string, PlatformIconSpec> = {
  yuanbao: { Icon: SiBilibili, color: '#FB7299', kind: 'brand' }
 }

-interface PlatformAvatarProps {
+interface PlatformAvatarProps extends Omit<ComponentPropsWithoutRef<'span'>, 'children'> {
  platformId: string
  platformName: string
-  className?: string
 }

-export function PlatformAvatar({ className, platformId, platformName }: PlatformAvatarProps) {
+// forwardRef + spreading ...rest is required so a wrapping <Tip> (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<HTMLSpanElement, PlatformAvatarProps>(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 (
-      <span aria-hidden="true" className={cn(baseClass, 'bg-(--ui-bg-tertiary) text-(--ui-text-tertiary)')}>
+      <span
+        aria-hidden="true"
+        className={cn(baseClass, 'bg-(--ui-bg-tertiary) text-(--ui-text-tertiary)')}
+        ref={ref}
+        style={style}
+        {...rest}
+      >
        {platformName.charAt(0).toUpperCase()}
      </span>
    )
@ -82,14 +96,17 @@ export function PlatformAvatar({ className, platformId, platformName }: Platform
    <span
      aria-hidden="true"
      className={baseClass}
+      ref={ref}
      style={{
        // 16% tint of the brand color so the glyph reads against any surface
        // without the avatar dominating the row.
        backgroundColor: `color-mix(in srgb, ${color} 16%, transparent)`,
-        color
+        color,
+        ...style
      }}
+      {...rest}
    >
      {Icon ? <Icon className="size-3.5" /> : spec.monogram || platformName.charAt(0).toUpperCase()}
    </span>
  )
-}
+})