diff --git a/src/components/conversations/conversation-detail-panel.tsx b/src/components/conversations/conversation-detail-panel.tsx index 2c18e3ed28..c9101c36ec 100644 --- a/src/components/conversations/conversation-detail-panel.tsx +++ b/src/components/conversations/conversation-detail-panel.tsx @@ -950,6 +950,20 @@ const ConversationTabView = memo(function ConversationTabView({ return () => { mountedRef.current = false syncCancelRef.current?.() + if (isReparentUnmount(useTabStore.getState(), tabId, groupId)) { + // Dragging the tab into another group reparents this view: React + // remounts it under that group's shell while the tab stays open. The + // connection is already held across that unmount (see + // `isTransientUnmount` above) and the runtime session has to be held + // with it — it holds the transcript. Dropping it emptied the message + // list and nothing brought it back: the remounted view re-registers + // its live-message sink on the connection it just kept, which recreates + // the session with a `liveMessage` and no `detail`, and `fetchDetail` + // skips a session that already has live data. Header, title and + // composer all read the tab row, so they looked untouched while the + // transcript stayed blank until the tab was closed and reopened. + return + } if (connStatusRef.current === "prompting" && !isViewerRef.current) { // Owner, agent still responding — keep the session for deferred cleanup // (the background turn_complete handler removes it once done). @@ -963,7 +977,13 @@ const ConversationTabView = memo(function ConversationTabView({ removeConversation(effectiveConversationId) } } - }, [effectiveConversationId, removeConversation, setPendingCleanup]) + }, [ + effectiveConversationId, + groupId, + removeConversation, + setPendingCleanup, + tabId, + ]) const handleSend = useCallback( ( diff --git a/src/components/conversations/group-shell-reconciliation.test.tsx b/src/components/conversations/group-shell-reconciliation.test.tsx index a964fa09f4..42c1a52517 100644 --- a/src/components/conversations/group-shell-reconciliation.test.tsx +++ b/src/components/conversations/group-shell-reconciliation.test.tsx @@ -1,7 +1,19 @@ import { readFileSync } from "node:fs" import { resolve } from "node:path" -import { describe, it, expect } from "vitest" -import { render } from "@testing-library/react" +import { describe, it, expect, vi } from "vitest" +import { act, render } from "@testing-library/react" + +import { + getTimelineTurns, + resetConversationRuntimeStore, + useConversationRuntimeStore, +} from "@/stores/conversation-runtime-store" + +vi.mock("@/lib/api", () => ({ + getFolderConversation: vi.fn(), +})) + +const { getFolderConversation } = await import("@/lib/api") const source = readFileSync( resolve( @@ -155,3 +167,59 @@ describe("split group shell source shape", () => { expect(shellBody.slice(stripIdx, contentIdx)).not.toContain("<>") }) }) + +/** + * The reparents the shells above cannot absorb. + * + * A tab dragged into another group DOES change React parents, so its view is + * remounted by design. The connection is deliberately carried across that + * unmount (`isTransientUnmount`), and the runtime session — which holds the + * transcript — has to be carried with it. Dropping the session there left the + * message list empty for as long as the tab stayed open: the remounted view + * re-registers its live-message sink on the connection it just kept, that + * recreates the session with live data and no detail, and `fetchDetail` skips + * a session that already has live data. Nothing refetches after that. + */ +describe("a reparented conversation view keeps its runtime session", () => { + it("consults the reparent classifier before either destructive branch", () => { + const cleanupStart = source.indexOf( + "// Cleanup runtime data on unmount (tab close)" + ) + expect(cleanupStart).toBeGreaterThan(-1) + const cleanup = source.slice(cleanupStart, cleanupStart + 2000) + const guardIdx = cleanup.indexOf("isReparentUnmount(useTabStore.getState()") + const deferIdx = cleanup.indexOf("setPendingCleanup(") + const removeIdx = cleanup.indexOf("removeConversation(") + expect(guardIdx).toBeGreaterThan(-1) + // Both ways of ending a session sit behind the classifier. + expect(deferIdx).toBeGreaterThan(guardIdx) + expect(removeIdx).toBeGreaterThan(guardIdx) + // Same inputs the connection's own guard uses, so the two agree on what a + // reparent is. + expect(cleanup.slice(guardIdx, deferIdx)).toContain("tabId, groupId") + }) + + it("cannot reload the transcript once a live sink has recreated the session", async () => { + resetConversationRuntimeStore() + const { actions } = useConversationRuntimeStore.getState() + + // What the remounted view does first: re-register its live-message sink on + // the connection it kept. The session comes back empty, but live. + act(() => { + actions.setLiveMessage( + 7, + { id: "lm-1", role: "assistant", content: [], startedAt: 0 }, + true + ) + }) + expect(getTimelineTurns(7)).toHaveLength(0) + + act(() => { + actions.fetchDetail(7) + }) + await act(async () => {}) + + expect(getFolderConversation).not.toHaveBeenCalled() + expect(getTimelineTurns(7)).toHaveLength(0) + }) +}) diff --git a/src/contexts/tab-context.test.tsx b/src/contexts/tab-context.test.tsx index d0587b4512..1e2ff72891 100644 --- a/src/contexts/tab-context.test.tsx +++ b/src/contexts/tab-context.test.tsx @@ -2215,6 +2215,87 @@ describe("TabProvider tab groups", () => { expect(store().rawTabs).toBe(before) }) + /** + * The unsplit strip's reorder, which has to take the same care its + * split-group sibling above already takes. + * + * `Reorder.Group` emits the order of the items it has measured since its own + * last render, filtered by REFERENCE against its current `values`. The filter + * can only remove, and the tab objects it holds are the ones the strip + * rendered with — so the list it hands back is a request to MOVE tabs, not a + * new tab set. Adopting it wholesale closed whichever tab was missing from it + * and rolled the rest back to whatever the strip last rendered. + */ + describe("reorderTabs", () => { + it("keeps a tab the reorder callback left out", async () => { + await renderWithTabs([tabItem(1, 1, true), tabItem(1, 2), tabItem(1, 3)]) + const rendered = store().tabs + + act(() => { + store().reorderTabs([rendered[2], rendered[1]]) + }) + + // The omitted tab is still open — and still the one being looked at. + expect(store().rawTabs.map((t) => t.id)).toContain("conv-1-codex-1") + expect(store().activeTabId).toBe("conv-1-codex-1") + expect(store().rawTabs).toHaveLength(3) + }) + + it("does not roll a tab back to what the strip rendered with", async () => { + await renderWithTabs([tabItem(1, 1, true)]) + act(() => { + store().openNewConversationTab(1, "/repo") + }) + const draftId = store().rawTabs.find((t) => t.conversationId == null)!.id + // What the strip was holding when the drag began. + const rendered = store().tabs + + // The draft's first message lands mid-drag: it binds to a real + // conversation, and its transcript now lives under a virtual runtime id. + act(() => { + store().bindConversationTab(draftId, 7, "codex", "sent", -42) + }) + act(() => { + store().reorderTabs([rendered[1], rendered[0]]) + }) + + expect(store().rawTabs.map((t) => t.id)).toEqual([ + draftId, + "conv-1-codex-1", + ]) + const moved = store().rawTabs.find((t) => t.id === draftId)! + expect(moved.conversationId).toBe(7) + expect(moved.runtimeConversationId).toBe(-42) + }) + + it("still moves the tab on a complete permutation", async () => { + await renderWithTabs([tabItem(1, 1, true), tabItem(1, 2), tabItem(1, 3)]) + const rendered = store().tabs + + act(() => { + store().reorderTabs([rendered[1], rendered[2], rendered[0]]) + }) + + expect(store().rawTabs.map((t) => t.id)).toEqual([ + "conv-1-codex-2", + "conv-1-codex-3", + "conv-1-codex-1", + ]) + }) + + it("refuses a list that names one tab twice", async () => { + await renderWithTabs([tabItem(1, 1, true), tabItem(1, 2)]) + const rendered = store().tabs + const before = store().rawTabs + + act(() => { + store().reorderTabs([rendered[1], rendered[1]]) + }) + + expect(store().rawTabs).toBe(before) + }) + }) + it("per-group draft singleton: each group reuses its own draft", async () => { await renderWithTabs([tabItem(1, 1, true)]) const home = leaves()[0] diff --git a/src/stores/tab-store.ts b/src/stores/tab-store.ts index 0d76967df9..9f119f22a0 100644 --- a/src/stores/tab-store.ts +++ b/src/stores/tab-store.ts @@ -502,6 +502,44 @@ function moveTabToSlot( return insertTab(without, tabs[from], index) } +/** + * `raw` with the tabs sitting at `slots` rearranged into the order a reorder + * callback asked for, or `null` when that list is not a permutation of exactly + * those slots (and so must be ignored). + * + * A reorder callback is a request to MOVE tabs, never a new tab set. The + * distinction matters because `Reorder.Group` emits the order it has measured + * since its own last render and then drops, by reference, whatever is missing + * from its current `values` — so mid-drag it can legitimately hand back a list + * that is short, repeats an id, or carries a tab object from an earlier derive. + * Resolving every entry back to the live `rawTabs` item by id keeps a drag + * unable to close a tab, resurrect a closed one, or write a stale copy of a + * tab's fields over the current one (a draft that bound to a conversation + * mid-drag would otherwise go back to being an unbound draft). + */ +function permuteSlots( + raw: TabItemInternal[], + slots: number[], + orderedTabs: TabItem[] +): TabItemInternal[] | null { + if (orderedTabs.length !== slots.length) return null + const slotIds = new Set(slots.map((i) => raw[i].id)) + const seen = new Set() + const ordered: TabItemInternal[] = [] + for (const tab of orderedTabs) { + if (!slotIds.has(tab.id) || seen.has(tab.id)) return null + seen.add(tab.id) + const item = raw.find((t) => t.id === tab.id) + if (!item) return null + ordered.push(item) + } + const next = [...raw] + slots.forEach((slot, k) => { + next[slot] = ordered[k] + }) + return next.every((tab, i) => tab === raw[i]) ? null : next +} + /** Field-wise equality for derived tab items. Backs the cross-derive reuse in * the `tabs` derivation: an item whose every field matches the previous derive * keeps its old reference, so downstream `Object.is` gates (consumers' memos) @@ -1628,24 +1666,10 @@ export const useTabStore = create()((set, get) => ({ slots.push(i) } }) - if (orderedTabs.length !== slots.length) return - const slotIds = new Set(slots.map((i) => raw[i].id)) - const seen = new Set() - const ordered: TabItemInternal[] = [] - for (const tab of orderedTabs) { - if (!slotIds.has(tab.id) || seen.has(tab.id)) return - seen.add(tab.id) - const item = raw.find((t) => t.id === tab.id) - if (!item) return - ordered.push(item) - } // Partition permutation: only this group's slots move, so the other // groups' persisted positions stay byte-stable. - const next = [...raw] - slots.forEach((slot, k) => { - next[slot] = ordered[k] - }) - if (next.every((tab, i) => tab === raw[i])) return + const next = permuteSlots(raw, slots, orderedTabs) + if (!next) return set({ rawTabs: next }) recomputeTabs() }, @@ -1664,7 +1688,16 @@ export const useTabStore = create()((set, get) => ({ }, reorderTabs: (reorderedTabs) => { - set({ rawTabs: reorderedTabs }) + // The unsplit strip shows every tab, so its slots are the whole array — + // otherwise identical to a group reorder, guards included. + const raw = get().rawTabs + const next = permuteSlots( + raw, + raw.map((_, i) => i), + reorderedTabs + ) + if (!next) return + set({ rawTabs: next }) recomputeTabs() },