From 205893cc1fe31371071214a9bce623ac3ea90fa4 Mon Sep 17 00:00:00 2001 From: murashit Date: Mon, 20 Jul 2026 19:03:05 +0900 Subject: [PATCH] fix(thread-picker): keep one active picker per runtime --- src/execution-runtime.ts | 31 ++++---- src/features/thread-picker/modal.obsidian.ts | 23 ++++-- tests/execution-runtime.test.ts | 79 ++++++++++++++++++++ tests/features/thread-picker/modal.test.ts | 76 ++++++++++++++++++- 4 files changed, 187 insertions(+), 22 deletions(-) create mode 100644 tests/execution-runtime.test.ts diff --git a/src/execution-runtime.ts b/src/execution-runtime.ts index 21b15418..e5c1c89f 100644 --- a/src/execution-runtime.ts +++ b/src/execution-runtime.ts @@ -54,7 +54,7 @@ export class CodexExecutionRuntime implements AppServerClientAccess { private readonly shortLivedClients = new Set(); private readonly structuredTurnClients = new Set(); private readonly structuredTurnOperations = new Set(); - private readonly threadPickers = new Set(); + private activeThreadPicker: ThreadPickerController | null = null; private readonly chatViews = new Set(); private readonly threadsViews = new Set(); private disposed = false; @@ -206,13 +206,19 @@ export class CodexExecutionRuntime implements AppServerClientAccess { } openThreadPicker(): void { - const picker = openThreadPicker({ - app: this.options.app, - threadCatalog: this.threadCatalog, - openThreadInCurrentView: (threadId) => this.options.openThreadInCurrentView(threadId), - openThreadInAvailableView: (threadId) => this.options.openThreadInAvailableView(threadId), - }); - this.threadPickers.add(picker); + this.activeThreadPicker?.close(); + const picker = openThreadPicker( + { + app: this.options.app, + threadCatalog: this.threadCatalog, + openThreadInCurrentView: (threadId) => this.options.openThreadInCurrentView(threadId), + openThreadInAvailableView: (threadId) => this.options.openThreadInAvailableView(threadId), + }, + () => { + if (this.activeThreadPicker === picker) this.activeThreadPicker = null; + }, + ); + this.activeThreadPicker = picker; } dispose(): ExecutionRuntimeViews { @@ -237,11 +243,10 @@ export class CodexExecutionRuntime implements AppServerClientAccess { } this.chatViews.clear(); this.threadsViews.clear(); - for (const picker of this.threadPickers) - this.tryCleanup(() => { - picker.close(); - }); - this.threadPickers.clear(); + this.tryCleanup(() => { + this.activeThreadPicker?.close(); + }); + this.activeThreadPicker = null; for (const operation of this.structuredTurnOperations) this.tryCleanup(() => { operation.abort(); diff --git a/src/features/thread-picker/modal.obsidian.ts b/src/features/thread-picker/modal.obsidian.ts index 562c3e76..d2ebde8a 100644 --- a/src/features/thread-picker/modal.obsidian.ts +++ b/src/features/thread-picker/modal.obsidian.ts @@ -24,9 +24,15 @@ type ThreadOpenMode = "current" | "available"; const THREAD_PICKER_MODIFIER_ENTER_LISTENER_OPTIONS = { capture: true } as const; -export function openThreadPicker(host: ThreadPickerHost): ThreadPickerController { +export function openThreadPicker(host: ThreadPickerHost, onClosed: () => void): ThreadPickerController { const state: { closed: boolean; modal: ThreadPickerModal | null } = { closed: false, modal: null }; - void (async () => { + const finish = (): void => { + if (state.closed) return; + state.closed = true; + state.modal = null; + onClosed(); + }; + const loadAndOpen = async (): Promise => { try { const recentSnapshot = host.threadCatalog.recentActiveSnapshot(); const loadedThreads = recentSnapshot ?? (await host.threadCatalog.loadActive()); @@ -34,19 +40,22 @@ export function openThreadPicker(host: ThreadPickerHost): ThreadPickerController const recentThreads = host.threadCatalog.recentActiveSnapshot() ?? loadedThreads; if (recentThreads.length === 0 && !host.threadCatalog.hasMoreActive()) { new Notice("No Codex threads found."); + finish(); return; } - state.modal = new ThreadPickerModal(host, recentThreads); + state.modal = new ThreadPickerModal(host, recentThreads, finish); state.modal.open(); } catch (error) { if (!state.closed) new Notice(error instanceof Error ? error.message : String(error)); + finish(); } - })(); + }; + queueMicrotask(() => void loadAndOpen()); return { close: () => { - state.closed = true; + if (state.closed) return; state.modal?.close(); - state.modal = null; + finish(); }, }; } @@ -71,6 +80,7 @@ class ThreadPickerModal extends SuggestModal { constructor( private readonly host: ThreadPickerHost, recentThreads: readonly Thread[], + private readonly onClosed: () => void, ) { super(host.app); this.recentThreads = Object.freeze([...recentThreads]); @@ -90,6 +100,7 @@ class ThreadPickerModal extends SuggestModal { override onClose(): void { this.inputEl.removeEventListener("keydown", this.handleInputKeydown, THREAD_PICKER_MODIFIER_ENTER_LISTENER_OPTIONS); super.onClose(); + this.onClosed(); } override async getSuggestions(query: string): Promise { diff --git a/tests/execution-runtime.test.ts b/tests/execution-runtime.test.ts new file mode 100644 index 00000000..17a3ffd3 --- /dev/null +++ b/tests/execution-runtime.test.ts @@ -0,0 +1,79 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { CodexExecutionRuntime } from "../src/execution-runtime"; +import type { ThreadPickerController } from "../src/features/thread-picker/modal.obsidian"; +import { DEFAULT_SETTINGS } from "../src/settings/model"; + +const { openThreadPickerMock } = vi.hoisted(() => ({ + openThreadPickerMock: vi.fn(), +})); + +vi.mock("../src/features/thread-picker/modal.obsidian", () => ({ + openThreadPicker: openThreadPickerMock, +})); + +describe("CodexExecutionRuntime thread picker ownership", () => { + beforeEach(() => { + openThreadPickerMock.mockReset(); + }); + + it("replaces the active picker and disposes only the current picker", () => { + const pickers = pickerFactory(); + const runtime = executionRuntime(); + + runtime.openThreadPicker(); + runtime.openThreadPicker(); + + expect(pickers.controllers[0]?.close).toHaveBeenCalledOnce(); + expect(pickers.controllers[1]?.close).not.toHaveBeenCalled(); + + runtime.dispose(); + + expect(pickers.controllers[0]?.close).toHaveBeenCalledOnce(); + expect(pickers.controllers[1]?.close).toHaveBeenCalledOnce(); + }); + + it("releases a naturally closed picker before runtime disposal", () => { + const pickers = pickerFactory(); + const runtime = executionRuntime(); + + runtime.openThreadPicker(); + pickers.finish[0]?.(); + runtime.dispose(); + + expect(pickers.controllers[0]?.close).not.toHaveBeenCalled(); + }); +}); + +function pickerFactory(): { + controllers: Array }>; + finish: Array<() => void>; +} { + const controllers: Array }> = []; + const finish: Array<() => void> = []; + openThreadPickerMock.mockImplementation((_host: unknown, onClosed: () => void) => { + const controller = { + close: vi.fn(() => { + onClosed(); + }), + }; + controllers.push(controller); + finish.push(onClosed); + return controller; + }); + return { controllers, finish }; +} + +function executionRuntime(): CodexExecutionRuntime { + return new CodexExecutionRuntime({ + app: {} as never, + context: { codexPath: "codex", vaultPath: "/vault" }, + settings: () => ({ ...DEFAULT_SETTINGS }), + workspace: {} as never, + onThreadCatalogEvent: vi.fn(), + openNewPanel: vi.fn(), + openThreadInCurrentView: vi.fn(), + openThreadInAvailableView: vi.fn(), + openPanelActivities: () => [], + }); +} diff --git a/tests/features/thread-picker/modal.test.ts b/tests/features/thread-picker/modal.test.ts index ac18089e..b226f8da 100644 --- a/tests/features/thread-picker/modal.test.ts +++ b/tests/features/thread-picker/modal.test.ts @@ -1,7 +1,7 @@ // @vitest-environment jsdom import { SuggestModal } from "obsidian"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import type { Thread } from "../../../src/domain/threads/model"; import { openThreadPicker, type ThreadPickerHost } from "../../../src/features/thread-picker/modal.obsidian"; @@ -120,6 +120,61 @@ describe("threadOpenModeFromEvent", () => { }); }); +describe("thread picker lifecycle", () => { + it("reports natural close only once", async () => { + const onClosed = vi.fn(); + const { controller, modal } = await openedThreadPickerSession([thread({ id: "thread" })], onClosed); + + modal.onClose(); + controller.close(); + + expect(onClosed).toHaveBeenCalledOnce(); + }); + + it("finishes when the initial inventory is empty", async () => { + const onClosed = vi.fn(); + + openThreadPicker(threadPickerHost([]), onClosed); + await flushMicrotasks(); + + expect(onClosed).toHaveBeenCalledOnce(); + }); + + it("finishes when the initial inventory fails to load", async () => { + const host = threadPickerHost([], [], [], false); + host.threadCatalog.loadActive = async () => { + throw new Error("inventory unavailable"); + }; + const onClosed = vi.fn(); + + openThreadPicker(host, onClosed); + await flushMicrotasks(); + + expect(onClosed).toHaveBeenCalledOnce(); + }); + + it("does not open after being replaced during the initial inventory load", async () => { + const pending = deferred(); + const host = threadPickerHost([], [], [], false); + host.threadCatalog.loadActive = () => pending.promise; + const open = vi.spyOn(SuggestModal.prototype, "open"); + const onClosed = vi.fn(); + + try { + const controller = openThreadPicker(host, onClosed); + await Promise.resolve(); + controller.close(); + pending.resolve([thread({ id: "stale" })]); + await flushMicrotasks(); + + expect(open).not.toHaveBeenCalled(); + expect(onClosed).toHaveBeenCalledOnce(); + } finally { + open.mockRestore(); + } + }); +}); + interface ThreadSuggestion { thread: Thread; title: string; @@ -129,23 +184,33 @@ interface CapturedThreadPickerModal { emptyStateText: string; resultContainerEl: HTMLElement; getSuggestions(query: string): Promise; + onClose(): void; onChooseSuggestion(item: ThreadSuggestion, evt: MouseEvent | KeyboardEvent): void; } async function openedThreadPicker(input: readonly Thread[] | TestThreadPickerHost): Promise { + return (await openedThreadPickerSession(input)).modal; +} + +async function openedThreadPickerSession( + input: readonly Thread[] | TestThreadPickerHost, + onClosed: () => void = () => undefined, +): Promise<{ controller: ReturnType; modal: CapturedThreadPickerModal }> { const captured: CapturedThreadPickerModal[] = []; const originalOpen = Object.getOwnPropertyDescriptor(SuggestModal.prototype, "open"); SuggestModal.prototype.open = function captureOpen(this: SuggestModal): void { captured.push(this as CapturedThreadPickerModal); }; + let controller: ReturnType; try { - await openThreadPicker(isThreadPickerHost(input) ? input : threadPickerHost(input)); + controller = openThreadPicker(isThreadPickerHost(input) ? input : threadPickerHost(input), onClosed); + await flushMicrotasks(); } finally { if (originalOpen) Object.defineProperty(SuggestModal.prototype, "open", originalOpen); } const modal = captured[0]; if (!modal) throw new Error("Expected thread picker modal to open"); - return modal; + return { controller, modal }; } async function firstSuggestion(modal: CapturedThreadPickerModal): Promise { @@ -225,3 +290,8 @@ function deferred() { }); return { promise, resolve }; } + +async function flushMicrotasks(): Promise { + await Promise.resolve(); + await Promise.resolve(); +}