From 210e6b564340a0964ef8b2367b838818d424b58f Mon Sep 17 00:00:00 2001 From: ethernet Date: Fri, 17 Jul 2026 16:35:43 -0400 Subject: [PATCH] fix(desktop): replace atom-mirrored refs with synchronous writes + direct reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The atom-mirrored-ref antipattern (useEffect syncing a ref from a store value) lags the atom by one render. Callbacks that read the ref after an await or through a stable-ref bag get stale values. This fixes all 11 sites identified in the audit: Confirmed bugs (PR #66485 class): - use-session-state-cache.ts: activeSessionIdRef, busyRef, selectedStoredSessionIdRef — now synced synchronously during render instead of via useEffect. Fixes the one-render lag that caused cancelRun to interrupt the wrong session. Latent bugs: - use-gateway-request.ts: gatewayStateRef → $gatewayState.get() in ensureGatewayOpen (deps=[]), removed mirroring effect entirely - use-voice-conversation.ts: enabledRef, mutedRef, busyRef, statusRef → synced synchronously during render (same pattern as session-state-cache) - model-settings.tsx: moaRef → setMoa(updater) with functional update, removed mirroring effect - i18n/context.tsx: localeRef → locale captured directly in setLocale callback, added to dep array - user-edit-composer.tsx: draftRef mirror removed, appendExternalText reads draft from closure (added to deps) The remaining eslint-disable comments on useEffect ref writes are legitimate non-mirror patterns: prev-value tracking, staging buffers, state flags, and direct ref writes paired with atom setters. --- .../composer/hooks/use-voice-conversation.ts | 35 +++++++---------- .../app/gateway/hooks/use-gateway-request.ts | 10 +---- .../hooks/use-session-actions/index.ts | 2 +- .../session/hooks/use-session-state-cache.ts | 23 +++++------ .../src/app/settings/model-settings.tsx | 39 +++++++------------ .../thread/user-edit-composer.tsx | 8 +--- apps/desktop/src/i18n/context.tsx | 9 ++--- 7 files changed, 44 insertions(+), 82 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/hooks/use-voice-conversation.ts b/apps/desktop/src/app/chat/composer/hooks/use-voice-conversation.ts index 4e2ad369fca..7d3e80bc35c 100644 --- a/apps/desktop/src/app/chat/composer/hooks/use-voice-conversation.ts +++ b/apps/desktop/src/app/chat/composer/hooks/use-voice-conversation.ts @@ -45,31 +45,22 @@ export function useVoiceConversation({ const responseIdRef = useRef(null) const spokenSourceLengthRef = useRef(0) const speechBufferRef = useRef('') + + // Props/state mirrored into refs synchronously during render (NOT via useEffect) + // so callbacks that fire after an await read values that are current as of the + // last render, not lagged by one effect tick. The atom-mirrored-ref eslint rule + // bans useEffect-based mirroring; these synchronous writes are the correct + // pattern for props that must be read inside stable callbacks. const enabledRef = useRef(enabled) const mutedRef = useRef(muted) const busyRef = useRef(busy) - const statusRef = useRef('idle') + const statusRef = useRef(status) const wasEnabledRef = useRef(enabled) - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) - useEffect(() => { - enabledRef.current = enabled - }, [enabled]) - - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) - useEffect(() => { - mutedRef.current = muted - }, [muted]) - - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) - useEffect(() => { - busyRef.current = busy - }, [busy]) - - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) - useEffect(() => { - statusRef.current = status - }, [status]) + enabledRef.current = enabled + mutedRef.current = muted + busyRef.current = busy + statusRef.current = status const clearTurnTimeout = () => { if (turnTimeoutRef.current) { @@ -331,7 +322,7 @@ export function useVoiceConversation({ // Drive the loop: after a voice-submitted turn, speak stable chunks as the // assistant stream grows. Otherwise start listening when idle between turns. - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) + // eslint-disable-next-line no-restricted-syntax -- staging buffers + state flags, not atom mirrors useEffect(() => { if (!enabled || muted) { return @@ -389,7 +380,7 @@ export function useVoiceConversation({ } }, [busy, consumePendingResponse, enabled, muted, pendingResponse, speak, startListening, status]) - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) + // eslint-disable-next-line no-restricted-syntax -- prev-value tracking for edge detection, not atom mirror useEffect(() => { if (enabled && !wasEnabledRef.current) { void start() diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-request.ts b/apps/desktop/src/app/gateway/hooks/use-gateway-request.ts index 5e71275fa5b..3fca7b7b3ee 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-request.ts +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-request.ts @@ -1,5 +1,4 @@ import { isGatewayReauthRequired, resolveGatewayWsUrl } from '@hermes/shared' -import { useStore } from '@nanostores/react' import { useCallback, useEffect, useRef } from 'react' import type { HermesGateway } from '@/hermes' @@ -8,25 +7,18 @@ import { $activeGatewayProfile } from '@/store/profile' import { $gatewayState, setConnection } from '@/store/session' export function useGatewayRequest() { - const gatewayState = useStore($gatewayState) const gatewayRef = useRef(null) const connectionRef = useRef['getConnection']>> | null>( null ) - const gatewayStateRef = useRef(gatewayState) const reconnectingRef = useRef | null>(null) // Holds the reauth error from the most recent failed reconnect so // requestGateway can surface the gateway's "session expired, sign in again" // message instead of the opaque "connection closed" that triggered the retry. const reauthErrorRef = useRef(null) - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) - useEffect(() => { - gatewayStateRef.current = gatewayState - }, [gatewayState]) - // Track the active gateway (primary or a background profile's socket) so // outbound requests and overlay props always target the focused profile. useEffect( @@ -44,7 +36,7 @@ export function useGatewayRequest() { return null } - if (gatewayStateRef.current === 'open') { + if ($gatewayState.get() === 'open') { return existing } diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts index 733e3fde7cf..53ea37ec7c9 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/index.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/index.ts @@ -186,7 +186,7 @@ export function useSessionActions({ // history entry. const rotatedStoredId = useStore($activeSessionStoredId) - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) + // eslint-disable-next-line no-restricted-syntax -- direct ref write paired with atom setter, not a mirroring effect useEffect(() => { if (!rotatedStoredId || rotatedStoredId === selectedStoredSessionIdRef.current) { return diff --git a/apps/desktop/src/app/session/hooks/use-session-state-cache.ts b/apps/desktop/src/app/session/hooks/use-session-state-cache.ts index 5d5307cc58e..46516970934 100644 --- a/apps/desktop/src/app/session/hooks/use-session-state-cache.ts +++ b/apps/desktop/src/app/session/hooks/use-session-state-cache.ts @@ -81,20 +81,15 @@ export function useSessionStateCache({ // flush below tell a same-session refresh from a thread switch. const viewSessionIdRef = useRef(null) - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) - useEffect(() => { - activeSessionIdRef.current = activeSessionId - }, [activeSessionId]) - - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) - useEffect(() => { - setMutableRef(busyRef, busy) - }, [busy, busyRef]) - - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) - useEffect(() => { - selectedStoredSessionIdRef.current = selectedStoredSessionId - }, [selectedStoredSessionId]) + // Sync refs from atoms synchronously during render (NOT via useEffect) so + // callbacks that fire after an await or inside a stable-ref bag read values + // that are current as of the last render, not lagged by one effect tick. + // The atom-mirrored-ref eslint rule bans useEffect-based mirroring; these + // synchronous writes are the correct pattern for values that must be read + // inside stable callbacks passed through the actionsRef bag. + activeSessionIdRef.current = activeSessionId + selectedStoredSessionIdRef.current = selectedStoredSessionId + setMutableRef(busyRef, busy) const ensureSessionState = useCallback((sessionId: string, storedSessionId?: string | null) => { const existing = sessionStateByRuntimeIdRef.current.get(sessionId) diff --git a/apps/desktop/src/app/settings/model-settings.tsx b/apps/desktop/src/app/settings/model-settings.tsx index 545609dbb94..16bf89ebf62 100644 --- a/apps/desktop/src/app/settings/model-settings.tsx +++ b/apps/desktop/src/app/settings/model-settings.tsx @@ -316,15 +316,6 @@ export function ModelSettings({ onMainModelChanged }: ModelSettingsProps) { return moa.presets[selectedMoaPreset] || moa.presets[moa.default_preset] || Object.values(moa.presets)[0] || null }, [moa, selectedMoaPreset]) - // Mirror of `moa` so inline edits compute the next state purely (outside the - // setState updater) and hand it straight to the debounced autosave. - const moaRef = useRef(null) - - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) - useEffect(() => { - moaRef.current = moa - }, [moa]) - const moaSaveTimer = useRef(null) useEffect( @@ -379,23 +370,23 @@ export function ModelSettings({ onMainModelChanged }: ModelSettingsProps) { const updateMoaPreset = useCallback( (updater: (preset: NonNullable) => NonNullable) => { - const prev = moaRef.current - - if (!prev || !selectedMoaPreset || !prev.presets[selectedMoaPreset]) { - return - } - - const next: MoaConfigResponse = { - ...prev, - presets: { - ...prev.presets, - [selectedMoaPreset]: updater(prev.presets[selectedMoaPreset]) + setMoa(prev => { + if (!prev || !selectedMoaPreset || !prev.presets[selectedMoaPreset]) { + return prev } - } - moaRef.current = next - setMoa(next) - scheduleMoaSave(next) + const next: MoaConfigResponse = { + ...prev, + presets: { + ...prev.presets, + [selectedMoaPreset]: updater(prev.presets[selectedMoaPreset]) + } + } + + scheduleMoaSave(next) + + return next + }) }, [scheduleMoaSave, selectedMoaPreset] ) diff --git a/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx b/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx index e52e5312b0d..c7f12b2c827 100644 --- a/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx +++ b/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx @@ -128,11 +128,10 @@ export const UserEditComposer: FC = ({ cwd, gateway, sess return } - const base = mode === 'inline' ? draftRef.current.trimEnd() : draftRef.current + const base = mode === 'inline' ? draft.trimEnd() : draft const sep = mode === 'inline' ? (base ? ' ' : '') : base && !base.endsWith('\n') ? '\n\n' : '' const next = `${base}${sep}${value}` - draftRef.current = next aui.composer().setText(next) const editor = editorRef.current @@ -144,13 +143,10 @@ export const UserEditComposer: FC = ({ cwd, gateway, sess setFocusRequestId(id => id + 1) }, - [aui] + [aui, draft] ) - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) useEffect(() => { - draftRef.current = draft - const editor = editorRef.current if ( diff --git a/apps/desktop/src/i18n/context.tsx b/apps/desktop/src/i18n/context.tsx index d69f07b5602..ef1789d3386 100644 --- a/apps/desktop/src/i18n/context.tsx +++ b/apps/desktop/src/i18n/context.tsx @@ -1,4 +1,4 @@ -import { createContext, type ReactNode, useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react' +import { createContext, type ReactNode, useCallback, useContext, useEffect, useMemo, useState } from 'react' import { getHermesConfigRecord, type HermesConfigRecord, saveHermesConfig } from '@/hermes' @@ -87,11 +87,8 @@ export function I18nProvider({ children, configClient = defaultConfigClient, ini const [isSavingLocale, setIsSavingLocale] = useState(false) const [configLoadError, setConfigLoadError] = useState(null) const [saveError, setSaveError] = useState(null) - const localeRef = useRef(locale) - // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) useEffect(() => { - localeRef.current = locale setRuntimeI18nLocale(locale) }, [locale]) @@ -131,7 +128,7 @@ export function I18nProvider({ children, configClient = defaultConfigClient, ini const setLocale = useCallback( async (next: Locale) => { - const previousLocale = localeRef.current + const previousLocale = locale setSaveError(null) setLocaleState(next) @@ -160,7 +157,7 @@ export function I18nProvider({ children, configClient = defaultConfigClient, ini setIsSavingLocale(false) } }, - [configClient] + [configClient, locale] ) const value = useMemo(