mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
fix(desktop): replace atom-mirrored refs with synchronous writes + direct reads
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.
This commit is contained in:
parent
505a4e8267
commit
210e6b5643
7 changed files with 44 additions and 82 deletions
|
|
@ -45,31 +45,22 @@ export function useVoiceConversation({
|
|||
const responseIdRef = useRef<string | null>(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<ConversationStatus>('idle')
|
||||
const statusRef = useRef<ConversationStatus>(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()
|
||||
|
|
|
|||
|
|
@ -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<HermesGateway | null>(null)
|
||||
|
||||
const connectionRef = useRef<Awaited<ReturnType<NonNullable<typeof window.hermesDesktop>['getConnection']>> | null>(
|
||||
null
|
||||
)
|
||||
|
||||
const gatewayStateRef = useRef(gatewayState)
|
||||
const reconnectingRef = useRef<Promise<HermesGateway | null> | 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<unknown>(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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -81,20 +81,15 @@ export function useSessionStateCache({
|
|||
// flush below tell a same-session refresh from a thread switch.
|
||||
const viewSessionIdRef = useRef<string | null>(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)
|
||||
|
|
|
|||
|
|
@ -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<MoaConfigResponse | null>(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<number | null>(null)
|
||||
|
||||
useEffect(
|
||||
|
|
@ -379,23 +370,23 @@ export function ModelSettings({ onMainModelChanged }: ModelSettingsProps) {
|
|||
|
||||
const updateMoaPreset = useCallback(
|
||||
(updater: (preset: NonNullable<typeof currentMoaPreset>) => NonNullable<typeof currentMoaPreset>) => {
|
||||
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]
|
||||
)
|
||||
|
|
|
|||
|
|
@ -128,11 +128,10 @@ export const UserEditComposer: FC<UserEditComposerProps> = ({ 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<UserEditComposerProps> = ({ 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 (
|
||||
|
|
|
|||
|
|
@ -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<Error | null>(null)
|
||||
const [saveError, setSaveError] = useState<Error | null>(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<I18nContextValue>(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue