fix(opal): anchor SidebarTab tooltips to the control so the row never remounts (#14245) to release v4.6 (#14249)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Jamison Lahman <jamison@lahman.dev>

Author: 3 people

web/lib/opal/src/components/buttons/sidebar-tab/README.md

@@ -10,7 +10,7 @@ A sidebar navigation tab built on `Interactive.Stateful` > `Interactive.Containe
 div.opal-sidebar-tab      <- folded styling hook (see styles.css)
   └─ Interactive.Stateful        <- variant (sidebar-heavy | sidebar-light), state, disabled
        └─ Interactive.Container  <- rounding, height, width
-            ├─ Link | button?    (absolute overlay — the click target)
+            ├─ Link | button?    (absolute overlay — the click target; also the tooltip trigger)
             ├─ rightChildren?    (absolute, above the overlay for inline actions)
             └─ ContentAction     (icon + title + truncation spacer)
 ```
@@ -31,6 +31,8 @@ Pass `folded` only to override the sidebar — outside a sidebar, in Storybook,
 
 The folded-name tooltip is the one part that stays in JS: CSS cannot arm a tooltip. It lives in a small wrapper that subscribes to the fold state on the tab's behalf, so a fold re-renders the wrapper and nothing below it. The wrapper keeps the tooltip mounted and passes `suppressed` while the tab is unfolded, so hover stays Radix's to track and an unfolded tab holds no hover state of its own.
 
+Both tooltips (the folded name and an explicit `tooltip`) wrap the overlay control, not the tab. The tab's own tree shape therefore never depends on whether there is a tooltip, so `children` can switch from a label to an element — an inline rename input — without remounting the row and dropping focus. A disabled tab has no control, so it renders an inert overlay as the trigger when it has a tooltip.
+
 ## Props
 
 | Prop | Type | Default | Description |

web/lib/opal/src/components/buttons/sidebar-tab/SidebarTab.test.tsx

@@ -113,3 +113,60 @@ it("names the tab for assistive technology in both states", () => {
   // The label is hidden by CSS while folded, so the name comes from the link.
   expect(screen.getByLabelText("Settings")).toBeInTheDocument();
 });
+
+it("keeps the same DOM node when children switch from a label to an element", () => {
+  // An inline rename swaps the string label for an input and drops `href`.
+  // A remount here would unmount that input before it could take focus.
+  const { rerender } = render(
+    <SidebarTab href="/app?chatId=1" onClick={() => {}}>
+      My chat
+    </SidebarTab>
+  );
+  const before = screen.getByText("My chat").closest(".opal-sidebar-tab");
+  expect(before).not.toBeNull();
+
+  rerender(
+    <SidebarTab>
+      <input aria-label="Rename chat" defaultValue="My chat" />
+    </SidebarTab>
+  );
+  const after = screen
+    .getByRole("textbox", { name: "Rename chat" })
+    .closest(".opal-sidebar-tab");
+  expect(after).toBe(before);
+});
+
+it("shows the folded label tooltip from the tab's control", async () => {
+  const user = userEvent.setup();
+  render(
+    <TooltipPrimitive.Provider delayDuration={0}>
+      <FoldedSidebar foldable />
+    </TooltipPrimitive.Provider>
+  );
+  await user.hover(screen.getByLabelText("Settings"));
+  await waitFor(() => {
+    expect(screen.getByRole("tooltip")).toHaveTextContent("Settings");
+  });
+});
+
+it("shows an explicit tooltip on a disabled tab", async () => {
+  // A disabled tab has no control, so an inert overlay is the trigger.
+  const user = userEvent.setup();
+  render(
+    <TooltipPrimitive.Provider delayDuration={0}>
+      <SidebarTab disabled tooltip="Enterprise only">
+        Groups
+      </SidebarTab>
+    </TooltipPrimitive.Provider>
+  );
+  const tab = screen.getByText("Groups").closest(".opal-sidebar-tab")!;
+  const overlay = tab.querySelector('[aria-hidden="true"].inset-0');
+  expect(overlay).not.toBeNull();
+  // Disabled is `aria-disabled` on a div, never a native disabled control:
+  // browsers drop pointer events inside one, which would mute the trigger.
+  expect(tab.querySelector("button[disabled]")).toBeNull();
+  await user.hover(overlay!);
+  await waitFor(() => {
+    expect(screen.getByRole("tooltip")).toHaveTextContent("Enterprise only");
+  });
+});

web/lib/opal/src/components/buttons/sidebar-tab/components.tsx

@@ -8,6 +8,7 @@ import { Interactive, type InteractiveStatefulVariant } from "@opal/core";
 import { ContentAction } from "@opal/layouts";
 import { useSidebarFolded } from "@opal/layouts/sidebar/context";
 import { Tooltip } from "@opal/components";
+import { cn } from "@opal/utils";
 import Link from "next/link";
 
 // ---------------------------------------------------------------------------
@@ -81,11 +82,11 @@ interface FoldedTooltipProps {
  * This is the one part of the folded look that CSS cannot express, so it is
  * split out: this component subscribes to the fold state, and the tab does
  * not. On a fold toggle React re-renders this wrapper alone — `children` is
- * the same element it received before, so the tab below it never re-renders.
+ * the same element it received before, so nothing below it re-renders.
  *
  * The tooltip stays mounted and is suppressed while unfolded instead of being
  * added and removed. Dropping it would change the tree shape on a fold, which
- * remounts the tab and cuts the label's fade short.
+ * remounts the trigger.
  */
 function FoldedTooltip({ label, folded, children }: FoldedTooltipProps) {
   const foldedFromSidebar = useSidebarFolded();
@@ -146,14 +147,6 @@ function SidebarTab({
     <div className="w-0 group-hover/SidebarTab:w-6" />
   );
 
-  /* The click target is an overlay that covers the whole row: a `Link` when
-  `href` is set, a `button` otherwise. It stays a sibling of the content so that
-  `rightChildren` and interactive icons remain valid nested controls. The focus
-  outline is inset because the container clips its overflow. `cursor-pointer` is
-  explicit because the UA stylesheet gives `button` a default cursor, which wins
-  over the value inherited from `.interactive`. */
-  const overlayClassName =
-    "absolute z-99 inset-0 rounded-08 cursor-pointer outline-border-04 outline-offset-[-2px] focus-visible:outline-2";
   /* The overlay holds no text of its own, and a folded tab hides its label, so
   name the overlay explicitly. String children name it directly. Other content
   (truncated or animated titles) names it through the element that renders the
@@ -164,24 +157,58 @@ function SidebarTab({
     label !== undefined
       ? { "aria-label": label }
       : { "aria-labelledby": labelId };
-  const overlay = disabled ? null : href ? (
-    <Link
-      href={href as Route}
-      scroll={false}
-      onClick={onClick}
-      {...labelProps}
-      className={overlayClassName}
-    />
-  ) : onClick ? (
-    <button
-      type={type ?? "button"}
-      onClick={onClick}
-      {...labelProps}
-      className={overlayClassName}
-    />
-  ) : null;
-
-  const content = (
+
+  /* The click target is an overlay that covers the whole row: a `Link` when
+  `href` is set, a `button` otherwise. It stays a sibling of the content so that
+  `rightChildren` and interactive icons remain valid nested controls. The focus
+  outline is inset because the container clips its overflow. `cursor-pointer` is
+  explicit because the UA stylesheet gives `button` a default cursor, which wins
+  over the value inherited from `.interactive`. */
+  const overlayClassName = "absolute z-99 inset-0 rounded-08";
+  const controlClassName = cn(
+    overlayClassName,
+    "cursor-pointer outline-border-04 outline-offset-[-2px] focus-visible:outline-2"
+  );
+  /* The tooltip hangs off the overlay, not the tab, so the tab's tree shape
+  never depends on whether there is one. Swapping `children` between a label and
+  an element (an inline rename input) then re-renders the row instead of
+  remounting it. A tab without a control gets an inert overlay as the trigger
+  when it has a tooltip to show. */
+  const overlay =
+    !disabled && href ? (
+      <Link
+        href={href as Route}
+        scroll={false}
+        onClick={onClick}
+        {...labelProps}
+        className={controlClassName}
+      />
+    ) : !disabled && onClick ? (
+      <button
+        type={type ?? "button"}
+        onClick={onClick}
+        {...labelProps}
+        className={controlClassName}
+      />
+    ) : tooltip || label !== undefined ? (
+      <div aria-hidden="true" className={overlayClassName} />
+    ) : null;
+  const trigger =
+    overlay &&
+    (tooltip ? (
+      <Tooltip tooltip={tooltip} side="right">
+        {overlay}
+      </Tooltip>
+    ) : label !== undefined ? (
+      // Only a string label can stand in as its own folded tooltip.
+      <FoldedTooltip label={label} folded={folded}>
+        {overlay}
+      </FoldedTooltip>
+    ) : (
+      overlay
+    ));
+
+  return (
     <div
       className="opal-sidebar-tab"
       data-folded={folded === undefined ? undefined : String(folded)}
@@ -194,16 +221,16 @@ function SidebarTab({
         group="group/SidebarTab"
       >
         <Interactive.Container rounding="sm" size="lg" width="full">
-          {overlay}
+          {trigger}
 
           {rightChildren && (
             <div className="opal-sidebar-tab__actions">{rightChildren}</div>
           )}
 
-          {typeof children === "string" ? (
+          {label !== undefined ? (
             <ContentAction
               icon={Icon ?? undefined}
-              title={children}
+              title={label}
               sizePreset="main-ui"
               variant="body"
               color="interactive"
@@ -231,23 +258,6 @@ function SidebarTab({
       </Interactive.Stateful>
     </div>
   );
-
-  if (tooltip) {
-    return (
-      <Tooltip tooltip={tooltip} side="right">
-        {content}
-      </Tooltip>
-    );
-  }
-
-  // Only a string label can stand in as its own tooltip.
-  if (label === undefined) return content;
-
-  return (
-    <FoldedTooltip label={label} folded={folded}>
-      {content}
-    </FoldedTooltip>
-  );
 }
 
 export { SidebarTab, type SidebarTabProps };

web/src/sections/sidebar/ChatButton.tsx

@@ -141,16 +141,18 @@ const ChatButton = memo(
 
     // Drag and drop setup for chat sessions
     const dragId = `${DRAG_TYPES.CHAT}-${chatSession.id}`;
-    const { attributes, listeners, setNodeRef, transform, isDragging } =
-      useDraggable({
-        id: dragId,
-        data: {
-          type: DRAG_TYPES.CHAT,
-          chatSession,
-          projectId: project?.id,
-        },
-        disabled: !draggable || renaming,
-      });
+    // `attributes` is intentionally dropped: it turns the wrapper into a
+    // focusable role="button", which adds a second tab stop per row and lets
+    // Enter/Space start a keyboard drag that looks like the chat is disabled.
+    const { listeners, setNodeRef, transform, isDragging } = useDraggable({
+      id: dragId,
+      data: {
+        type: DRAG_TYPES.CHAT,
+        chatSession,
+        projectId: project?.id,
+      },
+      disabled: !draggable || renaming,
+    });
 
     // Sync local name state when chatSession.name changes (e.g., after auto-naming)
     useEffect(() => {
@@ -402,7 +404,7 @@ const ChatButton = memo(
     const rightMenu = (
       <>
         <Popover.Trigger asChild onClick={noProp()}>
-          <div>
+          <div data-testid="ChatButton/options">
             {/* TODO(@raunakab): migrate to opal Button once className/iconClassName is resolved */}
             <IconButton
               icon={SvgMoreHorizontal}
@@ -415,7 +417,12 @@ const ChatButton = memo(
             />
           </div>
         </Popover.Trigger>
-        <Popover.Content side="right" align="start" width="md">
+        <Popover.Content
+          data-testid="ChatButton/popover"
+          side="right"
+          align="start"
+          width="md"
+        >
           <PopoverMenu>{popoverItems}</PopoverMenu>
         </Popover.Content>
       </>
@@ -431,7 +438,7 @@ const ChatButton = memo(
           }
         }}
       >
-        <Popover.Anchor>
+        <Popover.Anchor data-testid="ChatButton">
           <SidebarTab
             /* While renaming, drop the click target so the input stays usable. */
             href={
@@ -528,7 +535,6 @@ const ChatButton = memo(
                 : undefined,
               opacity: isDragging ? 0.5 : 1,
             }}
-            {...(mounted ? attributes : {})}
             {...(mounted ? listeners : {})}
           >
             {popover}

web/tests/e2e/chat/sidebar_chat_rename.spec.ts

@@ -0,0 +1,53 @@
+// Regression test for renaming a chat from the sidebar popout.
+//
+// SidebarTab must keep its tree shape stable when its label is swapped for
+// the inline rename editor. When the shape changed, React remounted the row
+// and the open popover; the remounted popover took focus back from the
+// editor, whose blur handler closed it before it could be used.
+import { test, expect } from "@playwright/test";
+import { loginAsWorkerUser } from "@tests/e2e/utils/auth";
+import { OnyxApiClient } from "@tests/e2e/utils/onyxApiClient";
+import { AppSidebarPage } from "@tests/e2e/pages/AppSidebarPage";
+
+const CHAT_NAME = "E2E Rename Target";
+const NEW_NAME = "E2E Renamed Chat";
+
+test.describe("Sidebar chat rename", () => {
+  let sidebar: AppSidebarPage;
+  let chatId: string;
+
+  test.beforeEach(async ({ page }, testInfo) => {
+    await page.context().clearCookies();
+    await loginAsWorkerUser(page, testInfo.workerIndex);
+
+    const apiClient = new OnyxApiClient(page.request);
+    chatId = await apiClient.createChatSession(CHAT_NAME);
+
+    sidebar = new AppSidebarPage(page);
+    await sidebar.goto();
+  });
+
+  test.afterEach(async ({ page }) => {
+    const apiClient = new OnyxApiClient(page.request);
+    await apiClient.deleteChatSession(chatId);
+  });
+
+  test("renames a chat session from the row's options popover", async () => {
+    const row = sidebar.chatRow(CHAT_NAME);
+    await row.startRename();
+
+    // The editor must appear, hold focus, and start from the current name.
+    await expect(row.renameInput).toBeVisible();
+    await expect(row.renameInput).toBeFocused();
+    await expect(row.renameInput).toHaveValue(CHAT_NAME);
+
+    await row.submitRename(NEW_NAME);
+
+    await expect(sidebar.chatRow(NEW_NAME).root).toBeVisible();
+    await expect(row.renameInput).toBeHidden();
+
+    // The new name survives a reload.
+    await sidebar.goto();
+    await expect(sidebar.chatRow(NEW_NAME).root).toBeVisible();
+  });
+});