diff --git a/src/components/conversations/conversation-detail-panel.tsx b/src/components/conversations/conversation-detail-panel.tsx index 1b5075fbca..e3f91c26b6 100644 --- a/src/components/conversations/conversation-detail-panel.tsx +++ b/src/components/conversations/conversation-detail-panel.tsx @@ -66,6 +66,7 @@ import { WelcomeHero, WelcomeTip } from "@/components/chat/welcome-hero" import { QuickActions } from "@/components/chat/quick-actions" import type { ComposerInjectContent } from "@/components/chat/message-input" import { TileScrollContainer } from "@/components/conversations/tile-scroll-container" +import { stableTabViewOrder } from "@/lib/tab-view-order" import { GroupSplitHandle } from "@/components/conversations/group-split-handle" import { OverlayHostHiddenProvider } from "@/components/ui/overlay-host-hidden" import { ScrollArea } from "@/components/ui/scroll-area" @@ -2914,6 +2915,7 @@ export function ConversationDetailPanel() { tileTabRefs.current.delete(tab.id) } }} + style={canTileG ? { order: indexInGroup } : undefined} className={cn( canTileG ? cn( @@ -3027,8 +3029,12 @@ export function ConversationDetailPanel() { canTileG && "flex min-w-full flex-row" )} > - {groupTabs.map((tab, indexInGroup) => - renderTabWrapper(tab, indexInGroup, groupId, canTileG) + {/* Strip order reaches the screen only through CSS `order`: + a reorder that moved these nodes would reset each moved + transcript's scroll offset and blank it (see + stableTabViewOrder). */} + {stableTabViewOrder(groupTabs).map(({ tab, visualIndex }) => + renderTabWrapper(tab, visualIndex, groupId, canTileG) )} diff --git a/src/lib/tab-view-order.test.tsx b/src/lib/tab-view-order.test.tsx new file mode 100644 index 0000000000..bb240d0d7e --- /dev/null +++ b/src/lib/tab-view-order.test.tsx @@ -0,0 +1,141 @@ +import { readFileSync } from "node:fs" +import { resolve } from "node:path" +import { describe, expect, it } from "vitest" +import { render } from "@testing-library/react" + +import { stableTabViewOrder } from "./tab-view-order" + +type Tab = { id: string } +const tabs = (...ids: string[]): Tab[] => ids.map((id) => ({ id })) + +describe("stableTabViewOrder", () => { + it("emits the same order for every permutation of the same tabs", () => { + const a = stableTabViewOrder(tabs("conv-2", "draft-9", "conv-1")) + const b = stableTabViewOrder(tabs("conv-1", "conv-2", "draft-9")) + expect(a.map((v) => v.tab.id)).toEqual(["conv-1", "conv-2", "draft-9"]) + expect(b.map((v) => v.tab.id)).toEqual(a.map((v) => v.tab.id)) + }) + + it("carries each tab's strip position as its visual index", () => { + const views = stableTabViewOrder(tabs("conv-2", "draft-9", "conv-1")) + expect( + Object.fromEntries(views.map((v) => [v.tab.id, v.visualIndex])) + ).toEqual({ "conv-2": 0, "draft-9": 1, "conv-1": 2 }) + }) + + it("returns an empty list for an empty group", () => { + expect(stableTabViewOrder([])).toEqual([]) + }) +}) + +/** + * The behaviour the order exists for. A keyed list rendered in strip order is + * reconciled by MOVING DOM nodes on a reorder, and a moved node loses its + * scroll offset (Chromium and WebKit both reset scrollTop to 0 on reinsertion + * and fire no scroll event, which strands the virtualized transcript on rows + * the viewport no longer shows). jsdom keeps scrollTop, so the test watches for + * the move itself: a reorder must not remove any existing view node from the + * tree. + */ +function Views({ order, tiled }: { order: string[]; tiled: boolean }) { + return ( +
+ {stableTabViewOrder(tabs(...order)).map(({ tab, visualIndex }) => ( +
+ ))} +
+ ) +} + +function removedNodesDuring(row: HTMLElement, change: () => void): Node[] { + const observer = new MutationObserver(() => {}) + observer.observe(row, { childList: true }) + change() + const removed = observer + .takeRecords() + .flatMap((record) => Array.from(record.removedNodes)) + observer.disconnect() + return removed +} + +function StripOrderViews({ order }: { order: string[] }) { + return ( +
+ {order.map((id) => ( +
+ ))} +
+ ) +} + +describe("conversation views across a strip reorder", () => { + it("control: views rendered in strip order are moved by a reorder", () => { + const { getByTestId, rerender } = render( + + ) + const row = getByTestId("row") + const removed = removedNodesDuring(row, () => + rerender() + ) + expect(removed.length).toBeGreaterThan(0) + }) + + it("moves no view node when the strip order changes", () => { + const { getByTestId, rerender } = render( + + ) + const row = getByTestId("row") + const before = Array.from(row.children) + const removed = removedNodesDuring(row, () => + rerender() + ) + expect(removed).toEqual([]) + expect(Array.from(row.children)).toEqual(before) + }) + + it("lays a tiled row out in strip order through CSS order", () => { + const { getByTestId, rerender } = render( + + ) + const row = getByTestId("row") + const removed = removedNodesDuring(row, () => + rerender() + ) + expect(removed).toEqual([]) + const orderOf = (id: string) => + (row.querySelector(`[data-view="${id}"]`) as HTMLElement).style.order + expect([orderOf("c"), orderOf("a"), orderOf("b")]).toEqual(["0", "1", "2"]) + }) + + it("only inserts the new node when a tab opens", () => { + const { getByTestId, rerender } = render( + + ) + const row = getByTestId("row") + const removed = removedNodesDuring(row, () => + rerender() + ) + expect(removed).toEqual([]) + }) + + it("is what the conversation panel renders its group views through", () => { + const panel = readFileSync( + resolve( + process.cwd(), + "src/components/conversations/conversation-detail-panel.tsx" + ), + "utf8" + ) + // The wrapper's index is the tab's STRIP position: it becomes the tile's + // CSS `order` and decides which tile goes without a left border, so the + // view's position in the id-sorted list must never reach it. + expect(panel).toMatch( + /stableTabViewOrder\(groupTabs\)\.map\(\(\{ tab, visualIndex \}\) =>\s*renderTabWrapper\(tab, visualIndex, groupId, canTileG\)/ + ) + expect(panel).toMatch(/style=\{canTileG \? \{ order: indexInGroup \}/) + }) +}) diff --git a/src/lib/tab-view-order.ts b/src/lib/tab-view-order.ts new file mode 100644 index 0000000000..0a4b7ba21b --- /dev/null +++ b/src/lib/tab-view-order.ts @@ -0,0 +1,33 @@ +/** + * DOM order for a group's conversation views, decoupled from the strip order. + * + * Dragging a tab in the strip permutes the tab list, and the views used to be + * rendered in that same order. React realises a keyed permutation by moving + * the existing DOM nodes (`insertBefore`), and moving a node out and back into + * the document resets every scroll offset inside it to 0 without firing a + * scroll event. The transcript is virtualized (virtua): it keeps rendering the + * rows for the offset it last saw, which is usually the bottom of a long + * conversation, while the viewport now sits at the top over an empty spacer. + * The moved tab, or any hidden tab React happened to move, shows up blank + * until the user scrolls. + * + * So the views are emitted in an order that a strip reorder cannot change + * (sorted by tab id, which is fixed for the life of a mounted view), and the + * strip order only reaches the screen through the CSS `order` property, which + * matters only in tiled mode (a flex row) and never touches the DOM tree. + * Opening or closing a tab inserts or removes one node and leaves every other + * node where it is. + */ +export interface OrderedTabView { + tab: T + /** Position of the tab in the strip (its visual slot in a tiled row). */ + visualIndex: number +} + +export function stableTabViewOrder( + tabs: readonly T[] +): OrderedTabView[] { + return tabs + .map((tab, visualIndex) => ({ tab, visualIndex })) + .sort((a, b) => (a.tab.id < b.tab.id ? -1 : a.tab.id > b.tab.id ? 1 : 0)) +}