From f3d987ee03ec60f9f7cb0d991744c98a0b072874 Mon Sep 17 00:00:00 2001 From: murashit Date: Tue, 21 Jul 2026 10:12:12 +0900 Subject: [PATCH] refactor(settings): serialize dynamic refreshes --- src/settings/dynamic-sections-controller.ts | 23 ++------------ .../dynamic-sections-controller.test.ts | 31 ++++++++----------- 2 files changed, 16 insertions(+), 38 deletions(-) diff --git a/src/settings/dynamic-sections-controller.ts b/src/settings/dynamic-sections-controller.ts index 939f86f7..44f297db 100644 --- a/src/settings/dynamic-sections-controller.ts +++ b/src/settings/dynamic-sections-controller.ts @@ -34,7 +34,6 @@ interface SettingsDynamicSectionsSnapshot { export class SettingsDynamicSectionsController { private readonly lifetime = new OwnerLifetime(); private dynamicSectionsAutoLoadStarted = false; - private modelsOperationToken = 0; private archivedThreadsOperationToken = 0; private hookMutationOperation: object | null = null; @@ -69,7 +68,6 @@ export class SettingsDynamicSectionsController { replaceDynamicData(next: SettingsDynamicDataAccess): void { if (next === this.dynamicData) return; this.unsubscribe(); - this.modelsOperationToken += 1; this.archivedThreadsOperationToken += 1; this.hookMutationOperation = null; this.dynamicData = next; @@ -131,7 +129,6 @@ export class SettingsDynamicSectionsController { if (this.archivedThreadsLifecycle.kind === "loading") { this.archivedThreadsLifecycle = createSettingsDynamicSectionLifecycle(); } - this.modelsOperationToken += 1; this.archivedThreadsOperationToken += 1; this.unsubscribe(); } @@ -166,10 +163,9 @@ export class SettingsDynamicSectionsController { async refreshDynamicSections(options: { forceModels?: boolean } = {}): Promise { const lifetime = this.lifetime.signal(); - if (!this.lifetime.isCurrent(lifetime)) return; + if (!this.lifetime.isCurrent(lifetime) || this.isLoading()) return; const dynamicData = this.dynamicData; this.dynamicSectionsAutoLoadStarted = true; - const modelsOperationToken = this.nextModelsOperationToken(); const archivedThreadsOperationToken = this.nextArchivedThreadsOperationToken(); this.modelsLifecycle = settingsDynamicSectionLoading("Loading models..."); this.archivedThreadsLifecycle = settingsDynamicSectionLoading("Loading archived threads..."); @@ -185,9 +181,7 @@ export class SettingsDynamicSectionsController { ] as const); if (!this.lifetime.isCurrent(lifetime) || !this.dynamicDataIsCurrent(dynamicData)) return; - if (this.isStaleModelsOperation(modelsOperationToken)) { - // A newer models operation owns this section. - } else if (modelsResult.status === "fulfilled") { + if (modelsResult.status === "fulfilled") { this.models = [...modelsResult.value]; this.modelsLifecycle = settingsDynamicSectionLoaded( `Loaded ${String(modelsResult.value.length)} model${modelsResult.value.length === 1 ? "" : "s"}.`, @@ -221,9 +215,7 @@ export class SettingsDynamicSectionsController { if (!this.lifetime.isCurrent(lifetime) || !this.dynamicDataIsCurrent(dynamicData)) return; failedCount = 3; const message = errorMessage(error); - if (!this.isStaleModelsOperation(modelsOperationToken)) { - this.modelsLifecycle = settingsDynamicSectionFailed(`Could not load models: ${message}`); - } + this.modelsLifecycle = settingsDynamicSectionFailed(`Could not load models: ${message}`); this.hooksLifecycle = settingsDynamicSectionFailed(`Could not load hooks: ${message}`); if (!this.isStaleArchivedThreadsOperation(archivedThreadsOperationToken)) { this.archivedThreadsLifecycle = settingsDynamicSectionFailed(`Could not load archived threads: ${message}`); @@ -382,20 +374,11 @@ export class SettingsDynamicSectionsController { } } - private nextModelsOperationToken(): number { - this.modelsOperationToken += 1; - return this.modelsOperationToken; - } - private nextArchivedThreadsOperationToken(): number { this.archivedThreadsOperationToken += 1; return this.archivedThreadsOperationToken; } - private isStaleModelsOperation(operationToken: number): boolean { - return operationToken !== this.modelsOperationToken; - } - 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 5c8fdc1b..7d9b0844 100644 --- a/tests/settings/dynamic-sections-controller.test.ts +++ b/tests/settings/dynamic-sections-controller.test.ts @@ -63,19 +63,12 @@ describe("SettingsDynamicSectionsController", () => { expect(display).toHaveBeenCalledOnce(); }); - it("ignores stale dynamic sections refresh results after a newer refresh completes", async () => { + it("ignores duplicate dynamic section refreshes while one is loading", async () => { const firstModels = deferred(); const firstClient = settingsClient(); - const secondClient = settingsClient({ models: [model("gpt-new")] }); - useShortLivedClients(firstClient, secondClient); - const refreshModels = vi - .fn() - .mockReturnValueOnce(firstModels.promise) - .mockResolvedValueOnce(modelMetadataFromCatalogModels([model("gpt-new")])); - const refreshArchived = vi - .fn() - .mockResolvedValueOnce([panelThread({ id: "thread-old", preview: "Old", archived: true })]) - .mockResolvedValueOnce([panelThread({ id: "thread-new", preview: "New", archived: true })]); + useShortLivedClients(firstClient); + const refreshModels = vi.fn(() => firstModels.promise); + const refreshArchived = vi.fn().mockResolvedValue([panelThread({ id: "thread-old", preview: "Old", archived: true })]); const controller = new SettingsDynamicSectionsController(settingsTabHost({ refreshModels, refreshArchived }), { display: vi.fn(), notify: vi.fn(), @@ -85,14 +78,15 @@ describe("SettingsDynamicSectionsController", () => { await flushPromises(); await controller.refreshDynamicSections(); - expect(controller.snapshot().models.map((item) => item.model)).toEqual(["gpt-new"]); - expect(controller.snapshot().archivedThreads.map((thread) => thread.preview)).toEqual(["New"]); + expect(refreshModels).toHaveBeenCalledOnce(); + expect(refreshArchived).toHaveBeenCalledOnce(); + expect(withShortLivedAppServerClientMock).toHaveBeenCalledOnce(); firstModels.resolve(modelMetadataFromCatalogModels([model("gpt-old")])); await firstRefresh; - expect(controller.snapshot().models.map((item) => item.model)).toEqual(["gpt-new"]); - expect(controller.snapshot().archivedThreads.map((thread) => thread.preview)).toEqual(["New"]); + expect(controller.snapshot().models.map((item) => item.model)).toEqual(["gpt-old"]); + expect(controller.snapshot().archivedThreads.map((thread) => thread.preview)).toEqual(["Old"]); }); it("does not display dynamic refresh results after disposal", async () => { @@ -204,7 +198,7 @@ describe("SettingsDynamicSectionsController", () => { expect(notify).not.toHaveBeenCalled(); }); - it("publishes stale-view archived restore facts without replacing a newer section result", async () => { + it("ignores refresh requests while an archived thread operation is loading", async () => { const staleRestore = deferred<{ thread: ThreadRecord }>(); const applyThreadCatalogEvent = vi.fn(); const initialClient = settingsClient(); @@ -227,7 +221,8 @@ describe("SettingsDynamicSectionsController", () => { await flushPromises(); await controller.refreshDynamicSections(); - expect(controller.snapshot().archivedThreads.map((thread) => thread.preview)).toEqual(["New archived"]); + expect(refreshArchived).toHaveBeenCalledOnce(); + expect(controller.snapshot().archivedThreads.map((thread) => thread.preview)).toEqual(["Old archived"]); staleRestore.resolve({ thread: appServerThread({ id: "thread-old", preview: "Restored old" }) }); await restore; @@ -236,7 +231,7 @@ describe("SettingsDynamicSectionsController", () => { type: "thread-restored", thread: expect.objectContaining({ id: "thread-old", preview: "Restored old", archived: false }), }); - expect(controller.snapshot().archivedThreads.map((thread) => thread.preview)).toEqual(["New archived"]); + expect(controller.snapshot().archivedThreads).toEqual([]); }); it("serializes conflicting archived mutations and publishes their facts in order", async () => {