From b27a3135638bb32b59026a2aeabc5f6332a92271 Mon Sep 17 00:00:00 2001 From: Ivan Malison Date: Fri, 24 Jul 2026 17:59:21 -0700 Subject: [PATCH] fix(web): keep rapid settings edits from reverting each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server settings only change locally once the server echoes `settingsUpdated` back over the websocket, and `useUpdateSettingsTarget` applied nothing locally in the meantime. Any edit issued inside that round trip was therefore computed from pre-edit settings, and because keys such as `providerInstances` are whole-value replacements rather than deep merges, the second write silently reverted the first: toggle one provider off, toggle a second off a moment later, and the first provider's switch snaps back on and never reaches settings.json. Retain in-flight patches per environment and replay them over the server value with the same `applyServerSettingsPatch` the server runs, so reads and follow-up patches build on the edit the user just made. Patches are dropped once no write is outstanding — including after a failed write, so the server value stays authoritative. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/hooks/pendingServerSettings.test.ts | 118 ++++++++++++++++++ apps/web/src/hooks/pendingServerSettings.ts | 107 ++++++++++++++++ apps/web/src/hooks/useSettings.ts | 40 +++++- 3 files changed, 260 insertions(+), 5 deletions(-) create mode 100644 apps/web/src/hooks/pendingServerSettings.test.ts create mode 100644 apps/web/src/hooks/pendingServerSettings.ts diff --git a/apps/web/src/hooks/pendingServerSettings.test.ts b/apps/web/src/hooks/pendingServerSettings.test.ts new file mode 100644 index 00000000000..0d8f99d3892 --- /dev/null +++ b/apps/web/src/hooks/pendingServerSettings.test.ts @@ -0,0 +1,118 @@ +import { + DEFAULT_SERVER_SETTINGS, + EnvironmentId, + ProviderDriverKind, + ProviderInstanceId, +} from "@t3tools/contracts"; +import { afterEach, describe, expect, it } from "vite-plus/test"; + +import { + __resetPendingServerPatchesForTests, + applyPendingServerPatches, + getPendingServerPatches, + releasePendingServerPatch, + retainPendingServerPatch, + subscribePendingServerPatches, +} from "./pendingServerSettings"; + +const environmentId = EnvironmentId.make("environment-1"); +const codexId = ProviderInstanceId.make("codex"); +const claudeId = ProviderInstanceId.make("claudeAgent"); + +const providerInstance = (driver: string, enabled: boolean) => ({ + driver: ProviderDriverKind.make(driver), + enabled, +}); + +afterEach(() => { + __resetPendingServerPatchesForTests(); +}); + +describe("pendingServerSettings", () => { + it("has no overlay before a write is dispatched", () => { + expect(getPendingServerPatches(environmentId)).toEqual([]); + expect(getPendingServerPatches(null)).toEqual([]); + expect(applyPendingServerPatches(DEFAULT_SERVER_SETTINGS, [])).toBe(DEFAULT_SERVER_SETTINGS); + }); + + it("keeps a second provider toggle from reverting the first one", () => { + // Disabling codex is dispatched from the server's own (empty) map. + const disableCodex = { + providerInstances: { [codexId]: providerInstance("codex", false) }, + }; + retainPendingServerPatch(environmentId, disableCodex); + + // Before the echo lands, the panel must already see codex disabled so the + // next whole-map replacement it builds carries that edit forward. + const optimistic = applyPendingServerPatches( + DEFAULT_SERVER_SETTINGS, + getPendingServerPatches(environmentId), + ); + expect(optimistic.providerInstances[codexId]?.enabled).toBe(false); + + const disableClaude = { + providerInstances: { + ...optimistic.providerInstances, + [claudeId]: providerInstance("claudeAgent", false), + }, + }; + retainPendingServerPatch(environmentId, disableClaude); + + const both = applyPendingServerPatches( + DEFAULT_SERVER_SETTINGS, + getPendingServerPatches(environmentId), + ); + expect(both.providerInstances[codexId]?.enabled).toBe(false); + expect(both.providerInstances[claudeId]?.enabled).toBe(false); + }); + + it("retains the overlay until the last outstanding write settles", () => { + const patch = { providerInstances: { [codexId]: providerInstance("codex", false) } }; + retainPendingServerPatch(environmentId, patch); + retainPendingServerPatch(environmentId, patch); + + releasePendingServerPatch(environmentId); + expect(getPendingServerPatches(environmentId)).toHaveLength(2); + + releasePendingServerPatch(environmentId); + expect(getPendingServerPatches(environmentId)).toEqual([]); + }); + + it("drops the overlay for a failed write so the server value wins again", () => { + retainPendingServerPatch(environmentId, { + providerInstances: { [codexId]: providerInstance("codex", false) }, + }); + releasePendingServerPatch(environmentId); + + const settings = applyPendingServerPatches( + DEFAULT_SERVER_SETTINGS, + getPendingServerPatches(environmentId), + ); + expect(settings).toBe(DEFAULT_SERVER_SETTINGS); + }); + + it("scopes pending writes to their own environment", () => { + const otherEnvironmentId = EnvironmentId.make("environment-2"); + retainPendingServerPatch(environmentId, { + providerInstances: { [codexId]: providerInstance("codex", false) }, + }); + + expect(getPendingServerPatches(otherEnvironmentId)).toEqual([]); + releasePendingServerPatch(otherEnvironmentId); + expect(getPendingServerPatches(environmentId)).toHaveLength(1); + }); + + it("notifies subscribers when the pending set changes", () => { + let notifications = 0; + const unsubscribe = subscribePendingServerPatches(() => { + notifications += 1; + }); + + retainPendingServerPatch(environmentId, { enableAssistantStreaming: false }); + releasePendingServerPatch(environmentId); + unsubscribe(); + retainPendingServerPatch(environmentId, { enableAssistantStreaming: false }); + + expect(notifications).toBe(2); + }); +}); diff --git a/apps/web/src/hooks/pendingServerSettings.ts b/apps/web/src/hooks/pendingServerSettings.ts new file mode 100644 index 00000000000..04197cf620c --- /dev/null +++ b/apps/web/src/hooks/pendingServerSettings.ts @@ -0,0 +1,107 @@ +/** + * Optimistic overlay for server settings writes that are still in flight. + * + * Server settings only change locally once the server echoes a + * `settingsUpdated` broadcast back over the websocket. Until that round trip + * lands, readers still see pre-edit settings, so a second edit issued inside + * the window is computed from stale state — and because keys such as + * `providerInstances` are whole-value replacements rather than deep merges, + * that second write silently reverts the first one. (Toggle one provider off, + * toggle a second off a moment later, and the first provider's switch snaps + * back on once its own write is overwritten.) + * + * Each in-flight patch is retained here and replayed on top of the server value + * through the same `applyServerSettingsPatch` the server runs, so both reads and + * follow-up patches are built from the edit the user just made. Retained patches + * are dropped once no write is outstanding for that environment, at which point + * the server value is authoritative again — including when a write failed. + * + * @module hooks/pendingServerSettings + */ +import type { EnvironmentId, ServerSettings, ServerSettingsPatch } from "@t3tools/contracts"; +import { applyServerSettingsPatch } from "@t3tools/shared/serverSettings"; + +interface PendingServerPatches { + readonly patches: ReadonlyArray; + /** Writes dispatched for this environment that have not settled yet. */ + readonly inFlight: number; +} + +export const NO_PENDING_SERVER_PATCHES: ReadonlyArray = []; + +const pendingByEnvironment = new Map(); +const listeners = new Set<() => void>(); + +function emitChange(): void { + for (const listener of listeners) { + listener(); + } +} + +export function subscribePendingServerPatches(listener: () => void): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +/** + * Snapshot accessor for `useSyncExternalStore`: the returned array is stable + * until the environment's pending set actually changes. + */ +export function getPendingServerPatches( + environmentId: EnvironmentId | null, +): ReadonlyArray { + if (environmentId === null) return NO_PENDING_SERVER_PATCHES; + return pendingByEnvironment.get(environmentId)?.patches ?? NO_PENDING_SERVER_PATCHES; +} + +/** Record a patch that has just been dispatched to the server. */ +export function retainPendingServerPatch( + environmentId: EnvironmentId, + patch: ServerSettingsPatch, +): void { + const pending = pendingByEnvironment.get(environmentId); + pendingByEnvironment.set(environmentId, { + patches: [...(pending?.patches ?? NO_PENDING_SERVER_PATCHES), patch], + inFlight: (pending?.inFlight ?? 0) + 1, + }); + emitChange(); +} + +/** + * Mark one dispatched write as settled. Patches are only forgotten when the + * last outstanding write settles: releasing them one at a time would expose the + * server value before its later echo arrived, flickering the UI back to a state + * the user has already edited away from. + */ +export function releasePendingServerPatch(environmentId: EnvironmentId): void { + const pending = pendingByEnvironment.get(environmentId); + if (pending === undefined) return; + if (pending.inFlight <= 1) { + pendingByEnvironment.delete(environmentId); + } else { + pendingByEnvironment.set(environmentId, { + patches: pending.patches, + inFlight: pending.inFlight - 1, + }); + } + emitChange(); +} + +/** Replay in-flight patches over an authoritative server settings value. */ +export function applyPendingServerPatches( + settings: ServerSettings, + patches: ReadonlyArray, +): ServerSettings { + if (patches.length === 0) return settings; + return patches.reduce( + (current, patch) => applyServerSettingsPatch(current, patch), + settings, + ); +} + +export function __resetPendingServerPatchesForTests(): void { + pendingByEnvironment.clear(); + listeners.clear(); +} diff --git a/apps/web/src/hooks/useSettings.ts b/apps/web/src/hooks/useSettings.ts index 514484d896a..88821857be5 100644 --- a/apps/web/src/hooks/useSettings.ts +++ b/apps/web/src/hooks/useSettings.ts @@ -24,6 +24,14 @@ import { type UnifiedSettings, } from "@t3tools/contracts/settings"; import { safeErrorLogAttributes } from "@t3tools/client-runtime/errors"; +import { + applyPendingServerPatches, + getPendingServerPatches, + NO_PENDING_SERVER_PATCHES, + releasePendingServerPatch, + retainPendingServerPatch, + subscribePendingServerPatches, +} from "./pendingServerSettings"; import { ensureLocalApi } from "~/localApi"; import * as Struct from "effect/Struct"; import { primaryServerSettingsAtom, serverEnvironment } from "~/state/server"; @@ -140,6 +148,17 @@ function persistClientSettings(settings: ClientSettings): void { }); } +function usePendingServerPatches( + environmentId: EnvironmentId | null, +): ReadonlyArray { + const getSnapshot = useCallback(() => getPendingServerPatches(environmentId), [environmentId]); + return useSyncExternalStore( + subscribePendingServerPatches, + getSnapshot, + () => NO_PENDING_SERVER_PATCHES, + ); +} + // ── Key sets for routing patches ───────────────────────────────────── const SERVER_SETTINGS_KEYS = new Set(Struct.keys(ServerSettings.fields)); @@ -198,14 +217,21 @@ export function mergeEnvironmentSettings( } function useMergedSettings( + environmentId: EnvironmentId | null, serverSettings: ServerSettings, selector: ((settings: UnifiedSettings) => T) | undefined, ): T { const clientSettings = useClientSettingsValue(); + const pendingPatches = usePendingServerPatches(environmentId); + + const optimisticServerSettings = useMemo( + () => applyPendingServerPatches(serverSettings, pendingPatches), + [pendingPatches, serverSettings], + ); const merged = useMemo( - () => mergeEnvironmentSettings(serverSettings, clientSettings), - [clientSettings, serverSettings], + () => mergeEnvironmentSettings(optimisticServerSettings, clientSettings), + [clientSettings, optimisticServerSettings], ); return useMemo(() => (selector ? selector(merged) : (merged as T)), [merged, selector]); @@ -224,20 +250,21 @@ export function useEnvironmentSettings( selector?: (settings: UnifiedSettings) => T, ): T { const serverSettings = useAtomValue(serverEnvironment.settingsValueAtom(environmentId)); - return useMergedSettings(serverSettings ?? DEFAULT_SERVER_SETTINGS, selector); + return useMergedSettings(environmentId, serverSettings ?? DEFAULT_SERVER_SETTINGS, selector); } /** Primary-only settings access for the settings UI and other explicitly global surfaces. */ export function usePrimarySettings( selector?: (settings: UnifiedSettings) => T, ): T { - return useMergedSettings(useAtomValue(primaryServerSettingsAtom), selector); + const environmentId = usePrimaryEnvironment()?.environmentId ?? null; + return useMergedSettings(environmentId, useAtomValue(primaryServerSettingsAtom), selector); } /** * Returns an updater that routes each key to the correct backing store. * - * Server keys are optimistically patched in atom-backed server state, then + * Server keys are applied optimistically through `./pendingServerSettings` and * persisted via RPC. Client keys go through client persistence. */ function useUpdateSettingsTarget(environmentId: EnvironmentId | null) { @@ -251,9 +278,12 @@ function useUpdateSettingsTarget(environmentId: EnvironmentId | null) { if (Object.keys(serverPatch).length > 0) { if (environmentId) { + retainPendingServerPatch(environmentId, serverPatch); void persistServerSettings({ environmentId, input: { patch: serverPatch }, + }).finally(() => { + releasePendingServerPatch(environmentId); }); } }