From 70bd78fcb45979a57715a2f5dea85d4e7112ed22 Mon Sep 17 00:00:00 2001 From: Adam Dalloul <47503782+Adam-Dalloul@users.noreply.github.com> Date: Thu, 8 Oct 2026 11:23:25 -0700 Subject: [PATCH 1/2] fix(tabs): keep a reordered tab's transcript on screen Dragging a tab along the strip permutes the tab list, and each group rendered its conversation views in that same order. React carries out a keyed permutation by moving DOM nodes, and a node that is taken out of the document and put back loses every scroll offset inside it: Chromium resets scrollTop to 0 and fires no scroll event. The virtualized transcript keeps rendering the rows for the offset it last saw (usually the bottom of a long conversation), while the viewport now sits at the top over an empty spacer, so the moved tab, or a hidden tab React happened to move, shows blank until it is scrolled. The views are now emitted in an order a reorder cannot change (by tab id), and the strip order reaches the screen only through the CSS order property, which only a tiled row uses. Opening or closing a tab still inserts or removes just its own node. --- .../conversation-detail-panel.tsx | 10 +- src/lib/tab-view-order.test.tsx | 137 ++++++++++++++++++ src/lib/tab-view-order.ts | 33 +++++ 3 files changed, 178 insertions(+), 2 deletions(-) create mode 100644 src/lib/tab-view-order.test.tsx create mode 100644 src/lib/tab-view-order.ts 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..1df20664e4 --- /dev/null +++ b/src/lib/tab-view-order.test.tsx @@ -0,0 +1,137 @@ +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 resets scrollTop to 0 on reinsertion and fires 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" + ) + expect(panel).toMatch( + /stableTabViewOrder\(groupTabs\)\.map\(\(\{ tab, visualIndex \}\) =>/ + ) + 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)) +} From 3dfa3b79cf29267fc71cc3ebda25beb93e1ad2ec Mon Sep 17 00:00:00 2001 From: xintaofei Date: Fri, 9 Oct 2026 14:29:31 +0800 Subject: [PATCH 2/2] test(tabs): pin the strip position each tab view is laid out by The panel wiring check only matched the destructuring of the id-sorted map, so handing renderTabWrapper any other index (the sorted position, a constant) still passed while a tiled row fell back to tab-id order and the wrong tile dropped its left border. Require the strip position itself to be the index the wrapper receives. Also note that WebKit, not only Chromium, resets a reinserted node's scroll offsets without a scroll event. --- src/lib/tab-view-order.test.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/lib/tab-view-order.test.tsx b/src/lib/tab-view-order.test.tsx index 1df20664e4..bb240d0d7e 100644 --- a/src/lib/tab-view-order.test.tsx +++ b/src/lib/tab-view-order.test.tsx @@ -31,10 +31,11 @@ describe("stableTabViewOrder", () => { /** * 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 resets scrollTop to 0 on reinsertion and fires 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. + * 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 ( @@ -129,8 +130,11 @@ describe("conversation views across a strip reorder", () => { ), "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 \}\) =>/ + /stableTabViewOrder\(groupTabs\)\.map\(\(\{ tab, visualIndex \}\) =>\s*renderTabWrapper\(tab, visualIndex, groupId, canTileG\)/ ) expect(panel).toMatch(/style=\{canTileG \? \{ order: indexInGroup \}/) })