From 48bd261f64b510246453302d19837f36db867983 Mon Sep 17 00:00:00 2001 From: murashit Date: Sat, 18 Jul 2026 06:58:10 +0900 Subject: [PATCH] refactor(settings): centralize dynamic data ownership --- src/settings/app-server-dynamic-data.ts | 72 +++--- src/settings/dynamic-data.ts | 6 +- src/settings/dynamic-sections-controller.ts | 201 ++++++--------- .../dynamic-sections-controller.test.ts | 232 ++++++++---------- tests/settings/test-support.ts | 22 +- 5 files changed, 235 insertions(+), 298 deletions(-) diff --git a/src/settings/app-server-dynamic-data.ts b/src/settings/app-server-dynamic-data.ts index 8698fb80..6347d19a 100644 --- a/src/settings/app-server-dynamic-data.ts +++ b/src/settings/app-server-dynamic-data.ts @@ -4,9 +4,9 @@ import type { ObservedResultListener } from "../app-server/query/observed-result import { isStaleAppServerResourceContextError } from "../app-server/query/resource-store"; import { listHookCatalog, setHookItemEnabled, trustHookItem } from "../app-server/services/catalog"; import { deleteThread, restoreArchivedThread as restoreArchivedThreadOnAppServer } from "../app-server/services/threads"; -import type { ModelMetadata } from "../domain/catalog/metadata"; +import type { HookItem, ModelMetadata } from "../domain/catalog/metadata"; import type { ThreadCatalogArchivedReader, ThreadCatalogEventSink } from "../features/threads/catalog/thread-catalog"; -import { createKeyedOperationQueue, type KeyedOperationQueue } from "../shared/runtime/keyed-operation-queue"; +import { createKeyedOperationQueue } from "../shared/runtime/keyed-operation-queue"; import { type SettingsDynamicDataAccess, type SettingsHookCatalog, StaleSettingsDynamicDataContextError } from "./dynamic-data"; interface SettingsAppServerQueries { @@ -27,24 +27,28 @@ export interface SettingsAppServerDynamicDataOptions { } export function createSettingsAppServerDynamicData(options: SettingsAppServerDynamicDataOptions): SettingsDynamicDataAccess { - const hookMutations = createKeyedOperationQueue<"hooks">(); const archivedThreadMutations = createKeyedOperationQueue(); const withSettingsConnection = (operation: (client: AppServerClient) => Promise): Promise => options.clientAccess.withClient(operation, { serverRequests: { kind: "reject", message: "Codex Panel settings does not handle server requests." }, }); - const withSettingsMutationConnection = (contextKey: string, operation: (client: AppServerClient) => Promise): Promise => - withSettingsConnection((client) => { - if (options.appServerQueries.contextKey() !== contextKey) throw new StaleSettingsDynamicDataContextError(); - return operation(client); - }); - const runMutation = (queue: KeyedOperationQueue, key: K, contextKey: string, operation: () => Promise): Promise => - queue.run(key, () => + const runArchivedThreadMutation = (threadId: string, operation: () => Promise): Promise => { + const contextKey = options.appServerQueries.contextKey(); + return archivedThreadMutations.run(archivedThreadMutationKey(contextKey, threadId), () => mapStaleContextError(async () => { if (options.appServerQueries.contextKey() !== contextKey) throw new StaleSettingsDynamicDataContextError(); return operation(); }), ); + }; + const loadHooks = (client: AppServerClient): Promise => loadSettingsHookCatalog(client, options.vaultPath); + const mutateHook = (hook: HookItem, mutation: (client: AppServerClient, hook: HookItem) => Promise): Promise => + mapStaleContextError(() => + withSettingsConnection(async (client) => { + await mutation(client, hook); + return loadHooks(client); + }), + ); return { modelsSnapshot: () => options.appServerQueries.modelsSnapshot(), @@ -54,46 +58,36 @@ export function createSettingsAppServerDynamicData(options: SettingsAppServerDyn archivedThreadsSnapshot: () => options.threadCatalog.archivedSnapshot(), observeArchivedThreadsResult: (listener, observeOptions) => options.threadCatalog.observeArchived(listener, observeOptions), refreshArchivedThreads: () => mapStaleContextError(() => options.threadCatalog.refreshArchived()), - loadHooks: () => withSettingsConnection((client) => loadSettingsHookCatalog(client, options.vaultPath)), - trustHook: (hook) => { - const contextKey = options.appServerQueries.contextKey(); - return runMutation(hookMutations, "hooks", contextKey, () => - withSettingsMutationConnection(contextKey, (client) => trustHookItem(client, hook)), - ); - }, - setHookEnabled: (hook, enabled) => { - const contextKey = options.appServerQueries.contextKey(); - return runMutation(hookMutations, "hooks", contextKey, () => - withSettingsMutationConnection(contextKey, (client) => setHookItemEnabled(client, hook, enabled)), - ); - }, - restoreArchivedThread: (threadId) => { - const contextKey = options.appServerQueries.contextKey(); - return runMutation(archivedThreadMutations, threadId, contextKey, async () => { - const thread = await withSettingsMutationConnection(contextKey, (client) => restoreArchivedThreadOnAppServer(client, threadId)); + refreshHooks: () => mapStaleContextError(() => withSettingsConnection(loadHooks)), + trustHook: (hook) => mutateHook(hook, trustHookItem), + setHookEnabled: (hook, enabled) => mutateHook(hook, (client, item) => setHookItemEnabled(client, item, enabled)), + restoreArchivedThread: (threadId) => + runArchivedThreadMutation(threadId, async () => { + const thread = await withSettingsConnection((client) => restoreArchivedThreadOnAppServer(client, threadId)); options.threadCatalog.apply({ type: "thread-restored", thread }); return thread; - }); - }, - deleteArchivedThread: (threadId) => { - const contextKey = options.appServerQueries.contextKey(); - return runMutation(archivedThreadMutations, threadId, contextKey, async () => { - await withSettingsMutationConnection(contextKey, (client) => deleteThread(client, threadId)); + }), + deleteArchivedThread: (threadId) => + runArchivedThreadMutation(threadId, async () => { + await withSettingsConnection((client) => deleteThread(client, threadId)); options.threadCatalog.apply({ type: "thread-deleted", threadId }); - }); - }, + }), }; } -async function loadSettingsHookCatalog(client: AppServerClient, cwd: string): Promise { - const hooks = await listHookCatalog(client, cwd); - const hookCount = hooks.hooks.length; +async function loadSettingsHookCatalog(client: AppServerClient, vaultPath: string): Promise { + const catalog = await listHookCatalog(client, vaultPath); + const hookCount = catalog.hooks.length; return { - ...hooks, + ...catalog, status: `Loaded ${String(hookCount)} hook${hookCount === 1 ? "" : "s"}.`, }; } +function archivedThreadMutationKey(contextKey: string, threadId: string): string { + return `${contextKey}\u0000${threadId}`; +} + async function mapStaleContextError(operation: () => Promise): Promise { try { return await operation(); diff --git a/src/settings/dynamic-data.ts b/src/settings/dynamic-data.ts index cd2ce99a..92feb70f 100644 --- a/src/settings/dynamic-data.ts +++ b/src/settings/dynamic-data.ts @@ -17,9 +17,9 @@ export interface SettingsDynamicDataAccess { archivedThreadsSnapshot(): readonly Thread[] | null; observeArchivedThreadsResult(listener: ObservedResultListener, options?: { emitCurrent?: boolean }): () => void; refreshArchivedThreads(): Promise; - loadHooks(): Promise; - trustHook(hook: HookItem): Promise; - setHookEnabled(hook: HookItem, enabled: boolean): Promise; + refreshHooks(): Promise; + trustHook(hook: HookItem): Promise; + setHookEnabled(hook: HookItem, enabled: boolean): Promise; restoreArchivedThread(threadId: string): Promise; deleteArchivedThread(threadId: string): Promise; } diff --git a/src/settings/dynamic-sections-controller.ts b/src/settings/dynamic-sections-controller.ts index 5bcbae98..80d9ef85 100644 --- a/src/settings/dynamic-sections-controller.ts +++ b/src/settings/dynamic-sections-controller.ts @@ -4,7 +4,7 @@ import { findModelMetadataByIdOrName, sortedModelMetadata, supportedEffortsForMo import type { Thread } from "../domain/threads/model"; import { threadArchiveDisplayTitle } from "../domain/threads/title"; import { OwnerLifetime } from "../shared/runtime/owner-lifetime"; -import { isStaleSettingsDynamicDataContextError } from "./dynamic-data"; +import { isStaleSettingsDynamicDataContextError, type SettingsHookCatalog } from "./dynamic-data"; import type { SettingsDynamicSectionsHost } from "./host"; interface SettingsDynamicSectionsControllerCallbacks { @@ -35,12 +35,8 @@ export class SettingsDynamicSectionsController { private readonly lifetime = new OwnerLifetime(); private dynamicSectionsAutoLoadStarted = false; private modelsOperationToken = 0; - private hooksOperationToken = 0; private archivedThreadsOperationToken = 0; - private pendingHookMutations = 0; - private hookReconcileRevision = 0; - private reconciledHookRevision = 0; - private hookReconcileDrain: Promise | null = null; + private hookMutationOperation: object | null = null; private archivedThreads: Thread[] = []; private archivedThreadsLoaded = false; @@ -93,8 +89,8 @@ export class SettingsDynamicSectionsController { resetDynamicSectionContext(): void { this.dynamicSectionsAutoLoadStarted = false; this.modelsOperationToken += 1; - this.hooksOperationToken += 1; this.archivedThreadsOperationToken += 1; + this.hookMutationOperation = null; this.models = [...(this.host.dynamicData.modelsSnapshot() ?? [])]; this.modelsLifecycle = createSettingsDynamicSectionLifecycle(); this.hooks = []; @@ -109,8 +105,15 @@ export class SettingsDynamicSectionsController { dispose(): void { this.lifetime.dispose(); + this.dynamicSectionsAutoLoadStarted = false; + if (this.modelsLifecycle.kind === "loading") this.modelsLifecycle = createSettingsDynamicSectionLifecycle(); + if (!this.hookMutationOperation && this.hooksLifecycle.kind === "loading") { + this.hooksLifecycle = createSettingsDynamicSectionLifecycle(); + } + if (this.archivedThreadsLifecycle.kind === "loading") { + this.archivedThreadsLifecycle = createSettingsDynamicSectionLifecycle(); + } this.modelsOperationToken += 1; - this.hooksOperationToken += 1; this.archivedThreadsOperationToken += 1; this.unsubscribeModels?.(); this.unsubscribeModels = null; @@ -136,12 +139,21 @@ export class SettingsDynamicSectionsController { this.callbacks.display(); } + private receiveHookCatalog(snapshot: SettingsHookCatalog): void { + this.hooks = [...snapshot.hooks]; + this.hookWarnings = [...snapshot.warnings]; + this.hookErrors = [...snapshot.errors]; + this.hooksLoaded = true; + if (this.hooksLifecycle.kind !== "loading") { + this.hooksLifecycle = settingsDynamicSectionLoaded(snapshot.status); + } + } + async refreshDynamicSections(options: { forceModels?: boolean } = {}): Promise { const lifetime = this.lifetime.signal(); if (!this.lifetime.isCurrent(lifetime)) return; this.dynamicSectionsAutoLoadStarted = true; const modelsOperationToken = this.nextModelsOperationToken(); - const hooksOperationToken = this.nextHooksOperationToken(); const archivedThreadsOperationToken = this.nextArchivedThreadsOperationToken(); this.modelsLifecycle = settingsDynamicSectionLoading("Loading models..."); this.archivedThreadsLifecycle = settingsDynamicSectionLoading("Loading archived threads..."); @@ -152,7 +164,7 @@ export class SettingsDynamicSectionsController { try { const [modelsResult, hooksResult, archivedThreadsResult] = await Promise.allSettled([ options.forceModels === false ? this.host.dynamicData.fetchModels() : this.host.dynamicData.refreshModels(), - this.host.dynamicData.loadHooks(), + this.host.dynamicData.refreshHooks(), this.host.dynamicData.refreshArchivedThreads(), ] as const); if (!this.lifetime.isCurrent(lifetime)) return; @@ -171,14 +183,11 @@ export class SettingsDynamicSectionsController { this.modelsLifecycle = settingsDynamicSectionFailed(`Could not load models: ${errorMessage(modelsResult.reason)}`); } - if (this.isStaleHooksOperation(hooksOperationToken)) { - // A newer hooks operation owns this section. - } else if (hooksResult.status === "fulfilled") { - this.hooks = [...hooksResult.value.hooks]; - this.hookWarnings = [...hooksResult.value.warnings]; - this.hookErrors = [...hooksResult.value.errors]; - this.hooksLoaded = true; + if (hooksResult.status === "fulfilled") { + this.receiveHookCatalog(hooksResult.value); this.hooksLifecycle = settingsDynamicSectionLoaded(hooksResult.value.status); + } else if (isStaleSettingsDynamicDataContextError(hooksResult.reason)) { + return; } else { failedCount += 1; this.hooksLifecycle = settingsDynamicSectionFailed(`Could not load hooks: ${errorMessage(hooksResult.reason)}`); @@ -205,9 +214,7 @@ export class SettingsDynamicSectionsController { if (!this.isStaleModelsOperation(modelsOperationToken)) { this.modelsLifecycle = settingsDynamicSectionFailed(`Could not load models: ${message}`); } - if (!this.isStaleHooksOperation(hooksOperationToken)) { - this.hooksLifecycle = settingsDynamicSectionFailed(`Could not load hooks: ${message}`); - } + this.hooksLifecycle = settingsDynamicSectionFailed(`Could not load hooks: ${message}`); if (!this.isStaleArchivedThreadsOperation(archivedThreadsOperationToken)) { this.archivedThreadsLifecycle = settingsDynamicSectionFailed(`Could not load archived threads: ${message}`); } @@ -243,50 +250,27 @@ export class SettingsDynamicSectionsController { } async trustHook(hook: HookItem): Promise { - this.pendingHookMutations += 1; - try { - await this.runDynamicSectionOperation({ - section: "hooks", - loadingStatus: "Loading hooks...", - failureStatus: (error) => `Could not trust hook: ${errorMessage(error)}`, - failureNotice: "Could not trust Codex hook.", - operation: async (operationToken) => { - await this.host.dynamicData.trustHook(hook); - this.requestHookReconciliation(); - if (this.isStaleHooksOperation(operationToken)) return; - this.hooksLifecycle = settingsDynamicSectionLoaded("Trusted hook definition."); - }, - }); - } finally { - this.pendingHookMutations -= 1; - await this.drainHookReconciliation(); - } + await this.runHookOperation({ + loadingStatus: "Loading hooks...", + failureStatus: (error) => `Could not trust hook: ${errorMessage(error)}`, + failureNotice: "Could not trust Codex hook.", + successStatus: "Trusted hook definition.", + operation: () => this.host.dynamicData.trustHook(hook), + }); } async setHookEnabled(hook: HookItem, enabled: boolean): Promise { - this.pendingHookMutations += 1; - try { - await this.runDynamicSectionOperation({ - section: "hooks", - loadingStatus: "Loading hooks...", - failureStatus: (error) => `Could not update hook: ${errorMessage(error)}`, - failureNotice: "Could not update Codex hook.", - operation: async (operationToken) => { - await this.host.dynamicData.setHookEnabled(hook, enabled); - this.requestHookReconciliation(); - if (this.isStaleHooksOperation(operationToken)) return; - this.hooksLifecycle = settingsDynamicSectionLoaded(enabled ? "Enabled hook." : "Disabled hook."); - }, - }); - } finally { - this.pendingHookMutations -= 1; - await this.drainHookReconciliation(); - } + await this.runHookOperation({ + loadingStatus: "Loading hooks...", + failureStatus: (error) => `Could not update hook: ${errorMessage(error)}`, + failureNotice: "Could not update Codex hook.", + successStatus: enabled ? "Enabled hook." : "Disabled hook.", + operation: () => this.host.dynamicData.setHookEnabled(hook, enabled), + }); } async restoreArchivedThread(threadId: string): Promise { - await this.runDynamicSectionOperation({ - section: "archivedThreads", + await this.runArchivedThreadOperation({ loadingStatus: "Loading archived threads...", failureStatus: (error) => `Could not restore archived thread: ${errorMessage(error)}`, failureNotice: "Could not restore archived Codex thread.", @@ -302,8 +286,7 @@ export class SettingsDynamicSectionsController { async deleteArchivedThread(threadId: string): Promise { const thread = this.archivedThreads.find((item) => item.id === threadId); const title = thread ? threadArchiveDisplayTitle(thread) : threadId; - await this.runDynamicSectionOperation({ - section: "archivedThreads", + await this.runArchivedThreadOperation({ loadingStatus: "Loading archived threads...", failureStatus: (error) => `Could not delete archived thread: ${errorMessage(error)}`, failureNotice: "Could not delete archived Codex thread.", @@ -333,54 +316,36 @@ export class SettingsDynamicSectionsController { return !effort || this.effortOptions(this.host.settings.rewriteSelectionModel).includes(effort); } - private requestHookReconciliation(): void { - this.hookReconcileRevision += 1; - } - - private drainHookReconciliation(): Promise { - if (this.hookMutationIsPending() || this.reconciledHookRevision === this.hookReconcileRevision) { - return Promise.resolve(); - } - if (this.hookReconcileDrain) return this.hookReconcileDrain; - const lifetime = this.lifetime.signal(); - const drain = (async (): Promise => { - while ( - !this.hookMutationIsPending() && - this.reconciledHookRevision !== this.hookReconcileRevision && - this.lifetime.isCurrent(lifetime) - ) { - const reconcileRevision = this.hookReconcileRevision; - const operationToken = this.hooksOperationToken; - try { - const hooks = await this.host.dynamicData.loadHooks(); - if (!this.lifetime.isCurrent(lifetime)) return; - if (this.hookMutationIsPending() || this.isStaleHooksOperation(operationToken)) continue; - this.hooks = [...hooks.hooks]; - this.hookWarnings = [...hooks.warnings]; - this.hookErrors = [...hooks.errors]; - this.hooksLoaded = true; - this.reconciledHookRevision = reconcileRevision; - this.callbacks.display(); - } catch (error) { - if (isStaleSettingsDynamicDataContextError(error) || !this.lifetime.isCurrent(lifetime)) return; - if (this.hookMutationIsPending() || this.isStaleHooksOperation(operationToken)) continue; - this.callbacks.notify(`Could not reconcile Codex hooks: ${errorMessage(error)}`); - return; - } + private async runHookOperation(options: { + loadingStatus: string; + failureStatus: (error: unknown) => string; + failureNotice: string; + successStatus: string; + operation: () => Promise; + }): Promise { + const operation = {}; + this.hookMutationOperation = operation; + const isCurrent = (): boolean => this.hookMutationOperation === operation; + this.hooksLifecycle = settingsDynamicSectionLoading(options.loadingStatus); + this.callbacks.display(); + try { + const catalog = await options.operation(); + if (!isCurrent()) return; + this.receiveHookCatalog(catalog); + this.hooksLifecycle = settingsDynamicSectionLoaded(options.successStatus); + } catch (error) { + if (!isCurrent() || isStaleSettingsDynamicDataContextError(error)) return; + this.hooksLifecycle = settingsDynamicSectionFailed(options.failureStatus(error)); + if (this.lifetime.isActive()) this.callbacks.notify(options.failureNotice); + } finally { + if (isCurrent()) { + this.hookMutationOperation = null; + if (this.lifetime.isActive()) this.callbacks.display(); } - })().finally(() => { - if (this.hookReconcileDrain === drain) this.hookReconcileDrain = null; - }); - this.hookReconcileDrain = drain; - return drain; + } } - private hookMutationIsPending(): boolean { - return this.pendingHookMutations > 0; - } - - private async runDynamicSectionOperation(options: { - section: "hooks" | "archivedThreads"; + private async runArchivedThreadOperation(options: { loadingStatus: string; failureStatus: (error: unknown) => string; failureNotice: string; @@ -388,25 +353,16 @@ export class SettingsDynamicSectionsController { }): Promise { const lifetime = this.lifetime.signal(); if (!this.lifetime.isCurrent(lifetime)) return; - const operationToken = options.section === "hooks" ? this.nextHooksOperationToken() : this.nextArchivedThreadsOperationToken(); - const stale = (): boolean => - !this.lifetime.isCurrent(lifetime) || - (options.section === "hooks" ? this.isStaleHooksOperation(operationToken) : this.isStaleArchivedThreadsOperation(operationToken)); - const setLifecycle = (state: SettingsDynamicSectionLifecycleState): void => { - if (options.section === "hooks") { - this.hooksLifecycle = state; - } else { - this.archivedThreadsLifecycle = state; - } - }; + const operationToken = this.nextArchivedThreadsOperationToken(); + const stale = (): boolean => !this.lifetime.isCurrent(lifetime) || this.isStaleArchivedThreadsOperation(operationToken); - setLifecycle(settingsDynamicSectionLoading(options.loadingStatus)); + this.archivedThreadsLifecycle = settingsDynamicSectionLoading(options.loadingStatus); this.callbacks.display(); try { await options.operation(operationToken); } catch (error) { if (stale() || isStaleSettingsDynamicDataContextError(error)) return; - setLifecycle(settingsDynamicSectionFailed(options.failureStatus(error))); + this.archivedThreadsLifecycle = settingsDynamicSectionFailed(options.failureStatus(error)); this.callbacks.notify(options.failureNotice); } finally { if (!stale()) this.callbacks.display(); @@ -418,11 +374,6 @@ export class SettingsDynamicSectionsController { return this.modelsOperationToken; } - private nextHooksOperationToken(): number { - this.hooksOperationToken += 1; - return this.hooksOperationToken; - } - private nextArchivedThreadsOperationToken(): number { this.archivedThreadsOperationToken += 1; return this.archivedThreadsOperationToken; @@ -432,10 +383,6 @@ export class SettingsDynamicSectionsController { return operationToken !== this.modelsOperationToken; } - private isStaleHooksOperation(operationToken: number): boolean { - return operationToken !== this.hooksOperationToken; - } - private isStaleArchivedThreadsOperation(operationToken: number): boolean { return operationToken !== this.archivedThreadsOperationToken; } diff --git a/tests/settings/dynamic-sections-controller.test.ts b/tests/settings/dynamic-sections-controller.test.ts index cf8093ab..c9871be7 100644 --- a/tests/settings/dynamic-sections-controller.test.ts +++ b/tests/settings/dynamic-sections-controller.test.ts @@ -1,7 +1,5 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; -import type { AppServerClient } from "../../src/app-server/connection/client"; -import type { CatalogHookMetadata } from "../../src/app-server/protocol/catalog"; import { modelMetadataFromCatalogModels } from "../../src/app-server/protocol/catalog"; import type { ThreadRecord } from "../../src/app-server/protocol/thread"; import type { ObservedResult } from "../../src/app-server/query/observed-result"; @@ -116,103 +114,92 @@ describe("SettingsDynamicSectionsController", () => { expect(display).not.toHaveBeenCalled(); }); - it("ignores stale hook reload results after a newer dynamic operation completes", async () => { - const staleHooks = deferred<{ - data: { cwd: string; hooks: CatalogHookMetadata[]; warnings: string[]; errors: unknown[] }[]; - }>(); - const initialClient = settingsClient({ - hooks: [hook({ key: "hook-initial", command: "initial hook", currentHash: "initialhash" })], - }); - const trustClient = settingsRequestClient({ - "config/batchWrite": vi.fn().mockResolvedValue({}), - }); - const staleClient = settingsClient(); - staleClient.requestHandlers["hooks/list"]?.mockReturnValue(staleHooks.promise); - const newerClient = settingsClient({ - hooks: [hook({ key: "hook-new", command: "new hook", currentHash: "newhash" })], - }); - useShortLivedClients(initialClient, trustClient, staleClient, newerClient); + it("reloads hooks after the settings view is hidden and shown again", async () => { + const firstHooks = deferred(); + const firstClient = settingsClient(); + firstClient.requestHandlers["hooks/list"] = vi.fn(() => firstHooks.promise); + const secondClient = settingsClient({ hooks: [hook({ key: "hook-after-reopen" })] }); + useShortLivedClients(firstClient, secondClient); const controller = new SettingsDynamicSectionsController(settingsTabHost(), { display: vi.fn(), notify: vi.fn() }); - await controller.refreshDynamicSections(); - const initialHook = controller.snapshot().hooks[0]; - if (!initialHook) throw new Error("Expected initial hook to load"); - const staleReload = controller.trustHook(initialHook); + controller.activate(); + controller.maybeAutoLoadDynamicSections(); + await flushPromises(); + controller.dispose(); + firstHooks.resolve({ data: [{ cwd: "/vault", hooks: [], warnings: [], errors: [] }] }); await flushPromises(); - await controller.refreshDynamicSections(); - expect(controller.snapshot().hooks.map((hook) => hook.currentHash)).toEqual(["newhash"]); + controller.activate(); + controller.maybeAutoLoadDynamicSections(); + await flushPromises(); - staleHooks.resolve({ - data: [{ cwd: "/vault", hooks: [hook({ key: "hook-old", command: "old hook", currentHash: "oldhash" })], warnings: [], errors: [] }], - }); - await staleReload; - - expect(controller.snapshot().hooks.map((item) => item.currentHash)).toEqual(["newhash"]); + expect(controller.snapshot().hooks).toEqual([expect.objectContaining({ key: "hook-after-reopen" })]); + expect(firstClient.requestHandlers["hooks/list"]).toHaveBeenCalledOnce(); + expect(secondClient.requestHandlers["hooks/list"]).toHaveBeenCalledOnce(); }); - it("keeps full refresh models and archived threads current when hook operations overlap", async () => { - const models = deferred(); - const fullRefreshClient = settingsClient({ - hooks: [hook({ key: "hook-full", command: "full refresh hook", currentHash: "fullhash" })], - }); - const trustClient = settingsRequestClient({ - "config/batchWrite": vi.fn().mockResolvedValue({}), - }); - const hookReloadClient = settingsClient({ - hooks: [hook({ key: "hook-new", command: "new hook", currentHash: "newhash" })], - }); - useShortLivedClients(fullRefreshClient, trustClient, hookReloadClient); - const refreshModels = vi.fn().mockReturnValue(models.promise); - const refreshArchived = vi.fn().mockResolvedValue([panelThread({ id: "thread-full", preview: "Full archived", archived: true })]); - const controller = new SettingsDynamicSectionsController(settingsTabHost({ refreshModels, refreshArchived }), { - display: vi.fn(), - notify: vi.fn(), - }); + it("settles a pending hook mutation after the settings view is hidden and shown again", async () => { + const write = deferred(); + const client = settingsClient({ hooks: [hook({ key: "hook-after-write", trustStatus: "trusted" })] }); + client.requestHandlers["config/batchWrite"] = vi.fn(() => write.promise); + useShortLivedClients(client); + const controller = new SettingsDynamicSectionsController(settingsTabHost(), { display: vi.fn(), notify: vi.fn() }); - const fullRefresh = controller.refreshDynamicSections(); + controller.activate(); + const mutation = controller.trustHook(hook({ key: "hook-after-write", trustStatus: "untrusted" })); await flushPromises(); - await controller.trustHook(hook({ key: "hook-initial", command: "initial hook", currentHash: "initialhash" })); + controller.dispose(); + controller.activate(); + controller.maybeAutoLoadDynamicSections(); - models.resolve(modelMetadataFromCatalogModels([model("gpt-refreshed")])); - await fullRefresh; + expect(withShortLivedAppServerClientMock).toHaveBeenCalledOnce(); - const snapshot = controller.snapshot(); - expect(snapshot.models.map((item) => item.model)).toEqual(["gpt-refreshed"]); - expect(snapshot.modelsLifecycle.kind).toBe("loaded"); - expect(snapshot.archivedThreads.map((thread) => thread.preview)).toEqual(["Full archived"]); - expect(snapshot.archivedThreadsLifecycle.kind).toBe("loaded"); - expect(snapshot.hooks.map((item) => item.currentHash)).toEqual(["newhash"]); + write.resolve({}); + await mutation; + + expect(controller.snapshot().hooks).toEqual([expect.objectContaining({ key: "hook-after-write", trustStatus: "trusted" })]); + expect(controller.snapshot().hooksLifecycle).toEqual({ kind: "loaded", status: "Trusted hook definition." }); }); - it("reconciles an earlier successful hook mutation when the latest queued mutation fails", async () => { - const trustWrite = deferred(); - const trustClient = settingsRequestClient({ - "config/batchWrite": vi.fn(() => trustWrite.promise), + it("reloads authoritative hooks on the same client after a mutation", async () => { + const client = settingsClient({ + hooks: [hook({ key: "hook-trusted", currentHash: "trusted-hash", trustStatus: "trusted" })], }); - const toggleClient = settingsRequestClient({ - "config/batchWrite": vi.fn().mockRejectedValue(new Error("toggle failed")), - }); - const reconciledClient = settingsClient({ - hooks: [hook({ key: "hook-trusted", command: "trusted hook", currentHash: "trustedhash", trustStatus: "trusted" })], - }); - useShortLivedClients(trustClient, toggleClient, reconciledClient); + useShortLivedClients(client); + const controller = new SettingsDynamicSectionsController(settingsTabHost(), { display: vi.fn(), notify: vi.fn() }); + + await controller.trustHook(hook({ key: "hook-trusted", currentHash: "untrusted-hash", trustStatus: "untrusted" })); + + expect(client.request.mock.calls.map(([method]) => method)).toEqual(["config/batchWrite", "hooks/list"]); + expect(withShortLivedAppServerClientMock).toHaveBeenCalledOnce(); + expect(controller.snapshot().hooks).toEqual([expect.objectContaining({ key: "hook-trusted", currentHash: "trusted-hash" })]); + expect(controller.snapshot().hooksLifecycle).toEqual({ kind: "loaded", status: "Trusted hook definition." }); + }); + + it("does not publish an old-context hook mutation over a replacement-context refresh", async () => { + const oldWrite = deferred(); + const oldClient = settingsClient({ hooks: [hook({ key: "hook-old-context" })] }); + oldClient.requestHandlers["config/batchWrite"] = vi.fn(() => oldWrite.promise); + const newClient = settingsClient({ hooks: [hook({ key: "hook-new-context" })] }); + useShortLivedClients(oldClient, newClient); + const host = settingsTabHost(); const notify = vi.fn(); - const controller = new SettingsDynamicSectionsController(settingsTabHost(), { display: vi.fn(), notify }); - const target = hook({ key: "hook-trusted", command: "trusted hook", currentHash: "trustedhash" }); + const controller = new SettingsDynamicSectionsController(host, { display: vi.fn(), notify }); + controller.activate(); - const trust = controller.trustHook(target); - const toggle = controller.setHookEnabled(target, false); + const oldMutation = controller.trustHook(hook({ key: "hook-old-context", trustStatus: "untrusted" })); await flushPromises(); - trustWrite.resolve({}); - await Promise.all([trust, toggle]); + await host.publishSettings({ ...host.settings, codexPath: "/opt/codex-next" }); + controller.resetDynamicSectionContext(); + await controller.refreshDynamicSections(); - expect(controller.snapshot().hooks.map((item) => item.currentHash)).toEqual(["trustedhash"]); - expect(controller.snapshot().hooksLifecycle).toEqual({ - kind: "failed", - status: "Could not update hook: toggle failed", - }); - expect(notify).toHaveBeenCalledWith("Could not update Codex hook."); + expect(controller.snapshot().hooks).toEqual([expect.objectContaining({ key: "hook-new-context" })]); + + oldWrite.resolve({}); + await oldMutation; + + expect(controller.snapshot().hooks).toEqual([expect.objectContaining({ key: "hook-new-context" })]); + expect(notify).not.toHaveBeenCalled(); }); it("publishes stale-view archived restore facts without replacing a newer section result", async () => { @@ -317,55 +304,50 @@ describe("SettingsDynamicSectionsController", () => { expect(display).not.toHaveBeenCalled(); }); - it("serializes hook mutations before starting the next short-lived client operation", async () => { - const firstWrite = deferred(); - const firstRequest = vi.fn(() => firstWrite.promise); - const secondRequest = vi.fn().mockResolvedValue({}); - const firstClient = settingsRequestClient({ - "config/batchWrite": firstRequest, + it("does not publish archived mutation facts after its app-server context is replaced", async () => { + const restoreResult = deferred<{ thread: ThreadRecord }>(); + const restoreClient = settingsRequestClient({ + "thread/unarchive": vi.fn(() => restoreResult.promise), }); - const secondClient = settingsRequestClient({ - "config/batchWrite": secondRequest, - }); - useShortLivedClients(firstClient, secondClient); - const dynamicData = settingsTabHost().dynamicData; + const applyThreadCatalogEvent = vi.fn(); + useShortLivedClients(restoreClient); + const host = settingsTabHost({ applyThreadCatalogEvent }); + const controller = new SettingsDynamicSectionsController(host, { display: vi.fn(), notify: vi.fn() }); - const trust = dynamicData.trustHook(hook({ key: "hook-first" })); - const toggle = dynamicData.setHookEnabled(hook({ key: "hook-second" }), false); - await flushPromises(); - - expect(firstRequest).toHaveBeenCalledOnce(); - expect(secondRequest).not.toHaveBeenCalled(); - expect(withShortLivedAppServerClientMock).toHaveBeenCalledOnce(); - - firstWrite.resolve({}); - await trust; - await toggle; - - expect(secondRequest).toHaveBeenCalledOnce(); - expect(withShortLivedAppServerClientMock).toHaveBeenCalledTimes(2); - }); - - it("does not start a settings mutation after client acquisition crosses a context replacement", async () => { - const acquisition = deferred(); - const client = settingsRequestClient({ - "config/batchWrite": vi.fn().mockResolvedValue({}), - }); - withShortLivedAppServerClientMock.mockImplementationOnce( - async (_codexPath: string, _vaultPath: string, operation: (acquiredClient: AppServerClient) => Promise) => { - await acquisition.promise; - return operation(client); - }, - ); - const host = settingsTabHost(); - - const trust = host.dynamicData.trustHook(hook({ key: "hook-old-context" })); + const restore = controller.restoreArchivedThread("thread-old"); await flushPromises(); await host.publishSettings({ ...host.settings, codexPath: "/opt/codex-next" }); - acquisition.resolve(undefined); + restoreResult.resolve({ thread: appServerThread({ id: "thread-old", preview: "Restored after replacement" }) }); + await restore; - await expect(trust).rejects.toBeInstanceOf(StaleSettingsDynamicDataContextError); - expect(client.request).not.toHaveBeenCalled(); + expect(applyThreadCatalogEvent).not.toHaveBeenCalled(); + }); + + it("starts a replacement-context archived mutation while old work is still pending", async () => { + const oldRestore = deferred<{ thread: ThreadRecord }>(); + const oldClient = settingsRequestClient({ + "thread/unarchive": vi.fn(() => oldRestore.promise), + }); + const newClient = settingsRequestClient({ + "thread/unarchive": vi.fn().mockResolvedValue({ + thread: appServerThread({ id: "thread-shared", preview: "New context" }), + }), + }); + useShortLivedClients(oldClient, newClient); + const host = settingsTabHost(); + + const staleMutation = host.dynamicData.restoreArchivedThread("thread-shared"); + await flushPromises(); + await host.publishSettings({ ...host.settings, codexPath: "/opt/codex-next" }); + const currentMutation = host.dynamicData.restoreArchivedThread("thread-shared"); + + await expect(currentMutation).resolves.toMatchObject({ id: "thread-shared", preview: "New context" }); + expect(newClient.requestHandlers["thread/unarchive"]).toHaveBeenCalledOnce(); + + oldRestore.resolve({ + thread: appServerThread({ id: "thread-shared", preview: "Old context" }), + }); + await expect(staleMutation).rejects.toBeInstanceOf(StaleSettingsDynamicDataContextError); }); it("records restored archived threads in the active catalog", async () => { diff --git a/tests/settings/test-support.ts b/tests/settings/test-support.ts index 22c428b4..68f04018 100644 --- a/tests/settings/test-support.ts +++ b/tests/settings/test-support.ts @@ -4,6 +4,7 @@ import type { AppServerClient } from "../../src/app-server/connection/client"; import type { AppServerClientAccessOptions } from "../../src/app-server/connection/client-access"; import type { CatalogHookMetadata, CatalogModel } from "../../src/app-server/protocol/catalog"; import type { ThreadRecord } from "../../src/app-server/protocol/thread"; +import { StaleAppServerResourceContextError } from "../../src/app-server/query/resource-store"; import type { ModelMetadata, ReasoningEffort } from "../../src/domain/catalog/metadata"; import type { Thread } from "../../src/domain/threads/model"; import type { ThreadCatalogEvent } from "../../src/features/threads/catalog/thread-catalog"; @@ -226,16 +227,29 @@ export function settingsTabHost(options: SettingsTabHostOptions = {}): CodexPane observeArchived: options.observeArchived ?? vi.fn(() => () => undefined), apply: options.applyThreadCatalogEvent ?? vi.fn(), }; + const clientAccess = { + withClient: async (operation: (client: AppServerClient) => Promise, clientOptions?: AppServerClientAccessOptions): Promise => { + const contextKey = appServerQueries.contextKey(); + const result = (await currentShortLivedClientMock()( + settings.codexPath, + "/vault", + async (client) => { + if (appServerQueries.contextKey() !== contextKey) throw new StaleAppServerResourceContextError(); + return operation(client); + }, + clientOptions, + )) as T; + if (appServerQueries.contextKey() !== contextKey) throw new StaleAppServerResourceContextError(); + return result; + }, + }; return { settings, dynamicData: options.dynamicData ?? createSettingsAppServerDynamicData({ vaultPath: "/vault", - clientAccess: { - withClient: (operation: (client: AppServerClient) => Promise, clientOptions?: AppServerClientAccessOptions) => - currentShortLivedClientMock()(settings.codexPath, "/vault", operation, clientOptions) as Promise, - }, + clientAccess, appServerQueries, threadCatalog, }),