Browse Source

fix(tui): harden session tab state hygiene (#39941)

Kit Langton 2 weeks ago
parent
commit
47c6840752

+ 9 - 3
packages/tui/src/context/session-tabs-model.ts

@@ -29,9 +29,11 @@ export function openSessionTab(tabs: SessionTab[], tab: SessionTab): SessionTab[
   return tabs.map((item, position) => (position === index ? { ...item, title: tab.title } : item))
 }
 
-export function closeSessionTab(tabs: readonly SessionTab[], sessionID: string) {
+export function closeSessionTab(tabs: SessionTab[], sessionID: string) {
   const index = tabs.findIndex((tab) => tab.sessionID === sessionID)
-  if (index === -1) return { tabs: [...tabs], next: undefined }
+  // Like openSessionTab and moveSessionTab, a no-op returns the same reference so callers can
+  // detect it by identity.
+  if (index === -1) return { tabs, next: undefined }
   return {
     tabs: tabs.filter((tab) => tab.sessionID !== sessionID),
     next: tabs[index + 1]?.sessionID ?? tabs[index - 1]?.sessionID,
@@ -87,9 +89,13 @@ export function cycleSessionTab(tabs: readonly SessionTab[], active: string | un
   return tabs[(start + direction + tabs.length) % tabs.length]
 }
 
+// In-memory navigation history is bounded so a long-lived TUI does not accumulate one entry per
+// session switch forever; the oldest entries fall off first.
+const SESSION_TAB_HISTORY_LIMIT = 100
+
 export function recordSessionTabHistory(history: SessionTabHistory, sessionID: string): SessionTabHistory {
   if (history.entries[history.index] === sessionID) return history
-  const entries = [...history.entries.slice(0, history.index + 1), sessionID]
+  const entries = [...history.entries.slice(0, history.index + 1), sessionID].slice(-SESSION_TAB_HISTORY_LIMIT)
   return { entries, index: entries.length - 1 }
 }
 

+ 3 - 2
packages/tui/src/context/session-tabs.tsx

@@ -73,7 +73,8 @@ export const { use: useSessionTabs, provider: SessionTabsProvider } = createSimp
     function update(mutation: (draft: TabsState) => void) {
       const scope = config.tabs?.scope ?? "global"
       void updateStore((draft) => mutation(scope === "cwd" ? (draft.cwd[paths.cwd] ??= empty()) : draft.global)).catch(
-        () => {},
+        // Failed writes lose only tab layout, but silence would hide tabs resetting every launch.
+        (error) => console.error("Failed to persist session tabs", error),
       )
     }
 
@@ -222,7 +223,7 @@ export const { use: useSessionTabs, provider: SessionTabsProvider } = createSimp
     function remove(sessionID: string, navigate: boolean) {
       const target = root(sessionID)
       const closed = closeSessionTab(state().tabs, target)
-      if (closed.tabs.length === state().tabs.length) return
+      if (closed.tabs === state().tabs) return
       const selected = navigate && current() === target
       const previous = selected
         ? moveSessionTabHistory(recordSessionTabHistory(history, target), closed.tabs, target, -1)

+ 14 - 0
packages/tui/test/context/session-tabs-model.test.ts

@@ -80,6 +80,11 @@ describe("session tabs", () => {
     expect(closeSessionTab([{ sessionID: "a" }], "a").next).toBeUndefined()
   })
 
+  test("closing an unknown session returns the same tabs reference", () => {
+    const tabs = [{ sessionID: "a" }, { sessionID: "b" }]
+    expect(closeSessionTab(tabs, "missing").tabs).toBe(tabs)
+  })
+
   test("cycles through a filtered tab set in either direction", () => {
     const tabs = ["a", "c", "e"].map((sessionID) => ({ sessionID }))
     expect(cycleSessionTab(tabs, "c", 1)?.sessionID).toBe("e")
@@ -122,6 +127,15 @@ describe("session tabs", () => {
     expect(recordSessionTabHistory(history, "b")).toBe(history)
   })
 
+  test("drops the oldest history entries beyond the limit", () => {
+    const sessions = Array.from({ length: 150 }, (_, index) => `session-${index}`)
+    const history = sessions.reduce(recordSessionTabHistory, { entries: [], index: -1 })
+
+    expect(history.entries.length).toBe(100)
+    expect(history.entries[0]).toBe("session-50")
+    expect(history.entries[history.index]).toBe("session-149")
+  })
+
   test("returns to the latest history entry when no tab is active", () => {
     const tabs = ["a", "b"].map((sessionID) => ({ sessionID }))
     const history = ["a", "b"].reduce(recordSessionTabHistory, { entries: [], index: -1 })

+ 28 - 6
packages/tui/test/context/session-tabs.test.tsx

@@ -1,8 +1,8 @@
 /** @jsxImportSource @opentui/solid */
-import { expect, test } from "bun:test"
+import { afterAll, expect, test } from "bun:test"
 import type { OpenCodeEvent } from "@opencode-ai/client"
 import { testRender } from "@opentui/solid"
-import { mkdtempSync, rmSync, watch } from "fs"
+import { mkdtempSync, readdirSync, rmSync, watch } from "fs"
 import { tmpdir } from "os"
 import path from "path"
 import { ConfigProvider } from "../../src/config"
@@ -25,8 +25,32 @@ async function wait(fn: () => boolean | Promise<boolean>, timeout = 2_000) {
   }
 }
 
+// State directories are removed after the whole suite instead of per test: persistence writes are
+// fire-and-forget behind a file lock, so a teardown-time removal races any still-queued write.
+const stateDirs: string[] = []
+
+afterAll(async () => {
+  for (const dir of stateDirs) {
+    // Drain any lock still held by a late write before deleting the tree beneath it.
+    await wait(() => {
+      try {
+        return readdirSync(path.join(dir, "test", "locks")).length === 0
+      } catch {
+        return true
+      }
+    }).catch(() => undefined)
+    rmSync(dir, { recursive: true, force: true })
+  }
+})
+
+function stateDir(prefix: string) {
+  const dir = mkdtempSync(path.join(tmpdir(), prefix))
+  stateDirs.push(dir)
+  return dir
+}
+
 async function renderSessionTabs(initialSessionID: string, options?: { state?: string; title?: string }) {
-  const state = options?.state ?? mkdtempSync(path.join(tmpdir(), "opencode-session-tabs-"))
+  const state = options?.state ?? stateDir("opencode-session-tabs-")
   const events = createEventStream()
   const calls = createFetch((url) => {
     if (url.pathname !== `/api/session/${initialSessionID}`) return
@@ -84,7 +108,6 @@ async function renderSessionTabs(initialSessionID: string, options?: { state?: s
     emit: (event: OpenCodeEvent) => events.emit({ ...event, location: { directory } }),
     destroy() {
       app.renderer.destroy()
-      if (!options?.state) rmSync(state, { recursive: true, force: true })
     },
   }
 }
@@ -105,7 +128,7 @@ test("stores session tabs globally by default", async () => {
 })
 
 test("concurrent TUIs do not alternate shared tab titles from divergent session caches", async () => {
-  const state = mkdtempSync(path.join(tmpdir(), "opencode-session-tabs-shared-"))
+  const state = stateDir("opencode-session-tabs-shared-")
   let titled: Awaited<ReturnType<typeof renderSessionTabs>> | undefined
   let untitled: Awaited<ReturnType<typeof renderSessionTabs>> | undefined
 
@@ -144,7 +167,6 @@ test("concurrent TUIs do not alternate shared tab titles from divergent session
   } finally {
     titled?.destroy()
     untitled?.destroy()
-    rmSync(state, { recursive: true, force: true })
   }
 })