From cb6e89b1ae17491c6343cc256c6d7b0f9237bd3a Mon Sep 17 00:00:00 2001 From: murashit Date: Fri, 17 Jul 2026 06:07:30 +0900 Subject: [PATCH] fix(threads): scope mutations to their operation context --- .../threads/rename-editor-actions.ts | 23 ++-- src/features/threads-view/session.ts | 55 +++++---- .../threads/rename-editor-actions.test.ts | 53 +++++++++ tests/features/threads-view/view.test.ts | 108 ++++++++++++++++++ 4 files changed, 210 insertions(+), 29 deletions(-) diff --git a/src/features/chat/application/threads/rename-editor-actions.ts b/src/features/chat/application/threads/rename-editor-actions.ts index c04eaa29..03c013a0 100644 --- a/src/features/chat/application/threads/rename-editor-actions.ts +++ b/src/features/chat/application/threads/rename-editor-actions.ts @@ -70,16 +70,21 @@ export function createThreadRenameEditorActions(host: ThreadRenameEditorActionsH if (current.kind === "idle" || current.threadId !== threadId || current.kind === "generating") return; const editingState = current; - await host.ensureConnected(); - if (renameState(host) !== editingState) return; + try { + await host.ensureConnected(); + if (renameState(host) !== editingState) return; - const result = await host.renameThread(threadId, value); - if (!result) { - if (renameState(host) === editingState) action.cancel(threadId); - return; - } - if (renameState(host) === editingState) { - dispatch(host, { type: "ui/rename-cleared" }); + const result = await host.renameThread(threadId, value); + if (!result) { + if (renameState(host) === editingState) action.cancel(threadId); + return; + } + if (renameState(host) === editingState) { + dispatch(host, { type: "ui/rename-cleared" }); + } + } catch (error) { + if (renameState(host) !== editingState) return; + host.addSystemMessage(error instanceof Error ? error.message : String(error)); } }, diff --git a/src/features/threads-view/session.ts b/src/features/threads-view/session.ts index 3f4e7aad..22906fab 100644 --- a/src/features/threads-view/session.ts +++ b/src/features/threads-view/session.ts @@ -57,6 +57,11 @@ type ThreadsViewStatus = | { kind: "log"; message: string } | { kind: "error"; message: string }; +interface ThreadsViewOperationLease { + readonly lifetime: AbortSignal; + readonly appServerContextGeneration: number; +} + export class ThreadsViewSession { private readonly lifetime = new OwnerLifetime(); private readonly operations: ThreadOperations; @@ -71,7 +76,7 @@ export class ThreadsViewSession { private unsubscribeThreads: (() => void) | null = null; private archiveConfirmThreadId: string | null = null; private observedAppServerContext: AppServerQueryContext; - private operationGeneration = 0; + private appServerContextGeneration = 0; constructor(private readonly environment: ThreadsViewSessionEnvironment) { this.observedAppServerContext = this.currentAppServerContext(); @@ -172,7 +177,7 @@ export class ThreadsViewSession { } prepareAppServerContextChange(): void { - this.operationGeneration += 1; + this.appServerContextGeneration += 1; this.titleService.invalidate(); this.activeRefresh = null; this.renderTask.clear(); @@ -321,19 +326,16 @@ export class ThreadsViewSession { } private async saveRename(threadId: string, value: string): Promise { - const lifetime = this.lifetime.signal(); - const operationGeneration = this.operationGeneration; + const lease = this.captureOperationLease(); const editingState = this.renameStates.get(threadId); if (!editingState || editingState.kind === "generating") return; try { if (this.renameStates.get(threadId) !== editingState) return; - const operationIsCurrent = () => operationGeneration === this.operationGeneration; const result = await this.operations.renameThread(threadId, value, { - shouldStart: operationIsCurrent, - shouldPublish: operationIsCurrent, + shouldStart: () => this.operationContextIsCurrent(lease), + shouldPublish: () => this.operationContextIsCurrent(lease), }); - if (!operationIsCurrent()) return; - if (!this.lifetime.isCurrent(lifetime) || this.renameStates.get(threadId) !== editingState) return; + if (!this.operationViewIsCurrent(lease) || this.renameStates.get(threadId) !== editingState) return; if (!result) { this.cancelRename(threadId); return; @@ -341,15 +343,14 @@ export class ThreadsViewSession { this.renameStates.delete(threadId); this.render(); } catch (error) { - if (!this.lifetime.isCurrent(lifetime)) return; + if (!this.operationViewIsCurrent(lease) || this.renameStates.get(threadId) !== editingState) return; this.status = { kind: "error", message: error instanceof Error ? error.message : String(error) }; this.render(); } } private async autoNameThread(threadId: string): Promise { - const lifetime = this.lifetime.signal(); - const operationGeneration = this.operationGeneration; + const lease = this.captureOperationLease(); const previousState = this.renameStates.get(threadId); const generationToken = this.nextRenameGenerationToken; const generatingState = this.transitionRenameState(threadId, { @@ -363,15 +364,15 @@ export class ThreadsViewSession { try { if (this.renameStates.get(threadId) !== generatingState) return; const title = await this.titleService.generateTitle(threadId); - if (!this.lifetime.isCurrent(lifetime) || operationGeneration !== this.operationGeneration) return; + if (!this.operationViewIsCurrent(lease)) return; this.transitionRenameState(threadId, { type: "generation-succeeded", generationToken, draft: title }); } catch (error) { - if (!this.lifetime.isCurrent(lifetime) || operationGeneration !== this.operationGeneration) return; + if (!this.operationViewIsCurrent(lease)) return; if (this.renameStates.get(threadId) === generatingState) { this.status = { kind: "error", message: error instanceof Error ? error.message : String(error) }; } } finally { - if (this.lifetime.isCurrent(lifetime) && operationGeneration === this.operationGeneration) { + if (this.operationViewIsCurrent(lease)) { this.finishAutoNameThread(threadId, generationToken); } } @@ -390,24 +391,38 @@ export class ThreadsViewSession { } private async archiveThread(threadId: string, saveMarkdown: boolean): Promise { - const lifetime = this.lifetime.signal(); - const operationGeneration = this.operationGeneration; + const lease = this.captureOperationLease(); try { await this.operations.archiveThread(threadId, { saveMarkdown, - shouldPublish: () => operationGeneration === this.operationGeneration, + shouldPublish: () => this.operationContextIsCurrent(lease), }); - if (!this.lifetime.isCurrent(lifetime) || operationGeneration !== this.operationGeneration) return; + if (!this.operationViewIsCurrent(lease)) return; this.host.closeOpenPanelsForThread(threadId); if (this.archiveConfirmThreadId === threadId) this.archiveConfirmThreadId = null; this.renameStates.delete(threadId); } catch (error) { - if (!this.lifetime.isCurrent(lifetime)) return; + if (!this.operationViewIsCurrent(lease)) return; this.status = { kind: "error", message: error instanceof Error ? error.message : String(error) }; this.render(); } } + private captureOperationLease(): ThreadsViewOperationLease { + return { + lifetime: this.lifetime.signal(), + appServerContextGeneration: this.appServerContextGeneration, + }; + } + + private operationContextIsCurrent(lease: ThreadsViewOperationLease): boolean { + return lease.appServerContextGeneration === this.appServerContextGeneration; + } + + private operationViewIsCurrent(lease: ThreadsViewOperationLease): boolean { + return this.operationContextIsCurrent(lease) && this.lifetime.isCurrent(lease.lifetime); + } + private finishAutoNameThread(threadId: string, generationToken: number): void { const previousState = this.renameStates.get(threadId); const nextState = this.transitionRenameState(threadId, { type: "generation-finished", generationToken }); diff --git a/tests/features/chat/application/threads/rename-editor-actions.test.ts b/tests/features/chat/application/threads/rename-editor-actions.test.ts index 77ac0663..4efc2f37 100644 --- a/tests/features/chat/application/threads/rename-editor-actions.test.ts +++ b/tests/features/chat/application/threads/rename-editor-actions.test.ts @@ -87,6 +87,59 @@ describe("ThreadRenameEditorActions", () => { expect(actions.editState("thread")).toBeNull(); }); + it("keeps the draft and reports a connection failure while saving", async () => { + const addSystemMessage = vi.fn(); + const { actions } = actionsFixture({ + ensureConnected: vi.fn().mockRejectedValue(new Error("Could not connect.")), + addSystemMessage, + }); + + actions.start("thread"); + actions.updateDraft("thread", "Unsaved draft"); + await actions.save("thread", "Unsaved draft"); + + expect(actions.editState("thread")).toEqual({ draft: "Unsaved draft", generating: false }); + expect(addSystemMessage).toHaveBeenCalledWith("Could not connect."); + }); + + it("keeps the draft and reports an app-server rename failure", async () => { + const addSystemMessage = vi.fn(); + const renameThreadRequest = vi.fn().mockRejectedValue(new Error("Rename failed.")); + const { actions } = actionsFixture({ + currentClient: () => fakeClient({ renameThreadRequest }), + addSystemMessage, + }); + + actions.start("thread"); + actions.updateDraft("thread", "Unsaved draft"); + await actions.save("thread", "Unsaved draft"); + + expect(actions.editState("thread")).toEqual({ draft: "Unsaved draft", generating: false }); + expect(addSystemMessage).toHaveBeenCalledWith("Rename failed."); + }); + + it("does not report a delayed save failure after the edit is invalidated", async () => { + const saved = deferred(); + const addSystemMessage = vi.fn(); + const { actions } = actionsFixture({ + currentClient: () => fakeClient({ renameThreadRequest: vi.fn(() => saved.promise) }), + addSystemMessage, + }); + + actions.start("thread"); + const save = actions.save("thread", "Old draft"); + await flushPromises(); + + actions.invalidate(); + actions.start("thread"); + actions.updateDraft("thread", "New draft"); + saved.reject(new Error("Stale rename failed.")); + await save; + + expect(actions.editState("thread")).toEqual({ draft: "New draft", generating: false }); + expect(addSystemMessage).not.toHaveBeenCalled(); + }); + it("does not clear a newer inline rename when an older save finishes", async () => { const saved = deferred(); const renameThreadRequest = vi.fn(() => saved.promise); diff --git a/tests/features/threads-view/view.test.ts b/tests/features/threads-view/view.test.ts index 411f13b9..9688bd7d 100644 --- a/tests/features/threads-view/view.test.ts +++ b/tests/features/threads-view/view.test.ts @@ -422,6 +422,36 @@ describe("CodexThreadsView", () => { }); }); + it("does not report an older save failure over a newer rename edit", async () => { + const saved = deferred(); + const renameThreadRequest = vi.fn(() => saved.promise); + connectionMock.state.client = clientFixture({ + "thread/list": vi.fn().mockResolvedValue({ data: [threadFixture({ id: "thread", preview: "Thread preview" })] }), + "thread/name/set": renameThreadRequest, + }); + const view = await threadsView(threadsHost()); + + await view.refresh(); + view.containerEl.querySelector('[aria-label="Rename thread"]')?.click(); + const firstInput = view.containerEl.querySelector(".codex-panel-threads__rename-input"); + if (!firstInput) throw new Error("Missing rename input"); + changeInputValue(firstInput, "Saved title"); + firstInput.dispatchEvent(new FocusEvent("blur")); + await waitForAsyncWork(() => expect(renameThreadRequest).toHaveBeenCalledOnce()); + + firstInput.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true })); + view.containerEl.querySelector('[aria-label="Rename thread"]')?.click(); + const secondInput = view.containerEl.querySelector(".codex-panel-threads__rename-input"); + if (!secondInput) throw new Error("Missing newer rename input"); + changeInputValue(secondInput, "New draft"); + + saved.reject(new Error("Older save failed.")); + for (let index = 0; index < 10; index += 1) await Promise.resolve(); + + expect(view.containerEl.querySelector(".codex-panel-threads__rename-input")?.value).toBe("New draft"); + expect(view.containerEl.textContent).not.toContain("Older save failed."); + }); + it("clears old rows and suppresses a delayed rename across an app-server context replacement", async () => { let codexPath = "codex-a"; const saved = deferred(); @@ -468,6 +498,70 @@ describe("CodexThreadsView", () => { expect(view.containerEl.textContent).not.toContain("Old thread"); }); + it("does not report a delayed rename rejection after an app-server context replacement", async () => { + let codexPath = "codex-a"; + const saved = deferred(); + const renameThreadRequest = vi.fn(() => saved.promise); + connectionMock.state.client = clientFixture({ + "thread/list": vi.fn().mockResolvedValue({ data: [threadFixture({ id: "thread-a", preview: "Old thread" })] }), + "thread/name/set": renameThreadRequest, + }); + const host = threadsHost({ settings: contextSettings(() => codexPath) }); + const view = await threadsView(host); + await view.onOpen(); + await waitForAsyncWork(() => expect(view.containerEl.textContent).toContain("Old thread")); + view.containerEl.querySelector('[aria-label="Rename thread"]')?.click(); + const input = view.containerEl.querySelector(".codex-panel-threads__rename-input"); + if (!input) throw new Error("Missing rename input"); + changeInputValue(input, "Renamed old thread"); + input.dispatchEvent(new FocusEvent("blur")); + await waitForAsyncWork(() => expect(renameThreadRequest).toHaveBeenCalledOnce()); + + view.prepareAppServerContextChange(); + codexPath = "codex-b"; + connectionMock.state.client = clientFixture({ "thread/list": vi.fn().mockResolvedValue({ data: [] }) }); + view.refreshSettings(); + await waitForAsyncWork(() => expect(view.containerEl.textContent).toContain("No threads")); + saved.reject(new Error("Old rename failed.")); + for (let index = 0; index < 10; index += 1) await Promise.resolve(); + + expect(view.containerEl.textContent).toContain("No threads"); + expect(view.containerEl.textContent).not.toContain("Old rename failed."); + }); + + it("does not report a delayed archive rejection after an app-server context replacement", async () => { + let codexPath = "codex-a"; + const archived = deferred(); + const archiveThreadRequest = vi.fn(() => archived.promise); + connectionMock.state.client = clientFixture({ + "thread/list": vi.fn().mockResolvedValue({ data: [threadFixture({ id: "thread-a", preview: "Old thread" })] }), + "thread/archive": archiveThreadRequest, + }); + const closeOpenPanelsForThread = vi.fn(); + const host = threadsHost({ + settings: contextSettings(() => codexPath), + closeOpenPanelsForThread, + }); + const view = await threadsView(host); + await view.onOpen(); + await waitForAsyncWork(() => expect(view.containerEl.textContent).toContain("Old thread")); + view.containerEl.querySelector('[aria-label="Archive thread"]')?.click(); + view.containerEl.querySelector('[aria-label="Archive thread without saving"]')?.click(); + await waitForAsyncWork(() => expect(archiveThreadRequest).toHaveBeenCalledOnce()); + + view.prepareAppServerContextChange(); + codexPath = "codex-b"; + connectionMock.state.client = clientFixture({ "thread/list": vi.fn().mockResolvedValue({ data: [] }) }); + view.refreshSettings(); + await waitForAsyncWork(() => expect(view.containerEl.textContent).toContain("No threads")); + archived.reject(new Error("Old archive failed.")); + for (let index = 0; index < 10; index += 1) await Promise.resolve(); + + expect(view.containerEl.textContent).toContain("No threads"); + expect(view.containerEl.textContent).not.toContain("Old archive failed."); + expect(closeOpenPanelsForThread).not.toHaveBeenCalled(); + }); + it("auto-names a thread rename draft from completed history", async () => { const threadTurnsList = vi.fn().mockResolvedValue({ data: [ @@ -702,6 +796,20 @@ function threadsHost(overrides: Record = {}) { }; } +function contextSettings(codexPath: () => string) { + return { + archiveExportEnabled: () => DEFAULT_SETTINGS.archiveExportEnabled, + codexPath, + threadNamingModel: () => DEFAULT_SETTINGS.threadNamingModel, + threadNamingEffort: () => DEFAULT_SETTINGS.threadNamingEffort, + archiveExportSettings: () => ({ + archiveExportFolderTemplate: DEFAULT_SETTINGS.archiveExportFolderTemplate, + archiveExportFilenameTemplate: DEFAULT_SETTINGS.archiveExportFilenameTemplate, + archiveExportTags: DEFAULT_SETTINGS.archiveExportTags, + }), + }; +} + async function threadsView(host = threadsHost()) { const { CodexThreadsView } = await import("../../../src/features/threads-view/view.obsidian"); const containerEl = document.createElement("div");