mirror of
https://github.com/rowboatlabs/rowboat.git
synced 2026-07-12 21:02:17 +02:00
fix(x/renderer): StrictMode-safe feed lifecycle; drop obsolete runs:fetch
The chat stuck at 'Thinking…' with a completed turn on disk: the stores subscribed to the sessions:events feed in their constructors while cleanup was effect-managed, so React StrictMode's dev-mode mount -> cleanup -> mount cycle tore down the one subscription and never restored it. The renderer then loaded a mid-turn snapshot and was deaf to every event after (the backend turn completed fine). Feed attachment is now an effect-managed, idempotent connect() on both stores; hooks return its disconnect as the effect cleanup. New tests pin the bug: a StrictMode-style connect -> cleanup -> connect store test, and the hook tests now render under a StrictMode wrapper. Also removed the composer's runs:fetch model-freeze effect (the ENOENT noise): sessions carry model/permission per message, so the picker stays live for existing chats. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
63917df061
commit
ec5a534465
6 changed files with 74 additions and 29 deletions
|
|
@ -342,22 +342,15 @@ function ChatInputInner({
|
|||
}
|
||||
})
|
||||
|
||||
// When a run exists, freeze the dropdown to the run's resolved model+provider.
|
||||
// Sessions runtime: model and permission mode are per-message turn config,
|
||||
// so nothing is frozen for an existing chat — the picker stays live.
|
||||
useEffect(() => {
|
||||
if (!runId) {
|
||||
setLockedModel(null)
|
||||
setPermissionMode('auto')
|
||||
return
|
||||
}
|
||||
let cancelled = false
|
||||
window.ipc.invoke('runs:fetch', { runId }).then((run) => {
|
||||
if (cancelled) return
|
||||
if (run.provider && run.model) {
|
||||
setLockedModel({ provider: run.provider, model: run.model })
|
||||
}
|
||||
setPermissionMode(run.permissionMode ?? 'manual')
|
||||
}).catch(() => { /* legacy run or fetch failure — leave unlocked */ })
|
||||
return () => { cancelled = true }
|
||||
setLockedModel(null)
|
||||
}, [runId])
|
||||
|
||||
useEffect(() => {
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
import { StrictMode } from 'react'
|
||||
import { act, renderHook, waitFor } from '@testing-library/react'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import type { SessionBusEvent } from '@x/shared/src/sessions.js'
|
||||
|
|
@ -69,7 +70,7 @@ function makeDeps() {
|
|||
describe('useSessionChat', () => {
|
||||
it('seeds from the session, follows live events, and routes actions', async () => {
|
||||
const { deps, calls, emit } = makeDeps()
|
||||
const { result } = renderHook(() => useSessionChat(S1, deps))
|
||||
const { result } = renderHook(() => useSessionChat(S1, deps), { wrapper: StrictMode })
|
||||
|
||||
await waitFor(() => {
|
||||
expect(result.current.latestTurnId).toBe('turn-1')
|
||||
|
|
@ -109,10 +110,14 @@ describe('useSessionChat', () => {
|
|||
])
|
||||
})
|
||||
|
||||
it('unsubscribes from the feed on unmount', async () => {
|
||||
it('unsubscribes from the feed on unmount (StrictMode double-mounts included)', async () => {
|
||||
const { deps, getUnsubscribed } = makeDeps()
|
||||
const { unmount } = renderHook(() => useSessionChat(S1, deps))
|
||||
const { unmount } = renderHook(() => useSessionChat(S1, deps), {
|
||||
wrapper: StrictMode,
|
||||
})
|
||||
unmount()
|
||||
expect(getUnsubscribed()).toBe(1)
|
||||
// StrictMode's simulated cleanup plus the real unmount: every subscribe
|
||||
// was matched by an unsubscribe.
|
||||
expect(getUnsubscribed()).toBe(2)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ export function useSessionChat(
|
|||
deps: SessionChatStoreDeps = defaultDeps,
|
||||
) {
|
||||
const [store] = useState(() => new SessionChatStore(deps))
|
||||
useEffect(() => () => store.dispose(), [store])
|
||||
useEffect(() => store.connect(), [store])
|
||||
useEffect(() => {
|
||||
void store.setSession(sessionId)
|
||||
}, [store, sessionId])
|
||||
|
|
|
|||
|
|
@ -13,8 +13,9 @@ const defaultDeps: SessionChatStoreDeps = {
|
|||
export function useSessions(deps: SessionChatStoreDeps = defaultDeps) {
|
||||
const [store] = useState(() => new SessionListStore(deps))
|
||||
useEffect(() => {
|
||||
const disconnect = store.connect()
|
||||
void store.load()
|
||||
return () => store.dispose()
|
||||
return disconnect
|
||||
}, [store])
|
||||
const snapshot = useSyncExternalStore(store.subscribe, store.getSnapshot)
|
||||
const client = deps.client
|
||||
|
|
|
|||
|
|
@ -86,20 +86,26 @@ class FakeClient implements SessionsClient {
|
|||
function makeStore() {
|
||||
const client = new FakeClient()
|
||||
let emit: SessionFeedListener = () => undefined
|
||||
let subscribed = 0
|
||||
let unsubscribed = 0
|
||||
const store = new SessionChatStore({
|
||||
client,
|
||||
subscribeFeed: (listener) => {
|
||||
subscribed += 1
|
||||
emit = listener
|
||||
return () => {
|
||||
unsubscribed += 1
|
||||
emit = () => undefined
|
||||
}
|
||||
},
|
||||
})
|
||||
const disconnect = store.connect()
|
||||
return {
|
||||
client,
|
||||
store,
|
||||
disconnect,
|
||||
emit: (event: SessionBusEvent) => emit(event),
|
||||
getSubscribed: () => subscribed,
|
||||
getUnsubscribed: () => unsubscribed,
|
||||
}
|
||||
}
|
||||
|
|
@ -245,12 +251,36 @@ describe('SessionChatStore', () => {
|
|||
expect(store.getSnapshot().chatState?.conversation).toEqual([])
|
||||
})
|
||||
|
||||
it('surfaces load errors and disposes its feed subscription', async () => {
|
||||
const { store, getUnsubscribed } = makeStore()
|
||||
it('surfaces load errors and disconnects its feed subscription', async () => {
|
||||
const { store, disconnect, getUnsubscribed } = makeStore()
|
||||
await store.setSession('missing-session')
|
||||
expect(store.getSnapshot().error).toMatch(/session not found/)
|
||||
store.dispose()
|
||||
disconnect()
|
||||
expect(getUnsubscribed()).toBe(1)
|
||||
void store
|
||||
})
|
||||
|
||||
it('survives a StrictMode-style connect -> cleanup -> connect cycle', async () => {
|
||||
// React StrictMode runs every effect's mount/cleanup/mount cycle in dev;
|
||||
// the feed must re-attach or the store goes permanently deaf (the bug
|
||||
// this test pins: a constructor-made subscription torn down by the first
|
||||
// cleanup was never restored, leaving the chat stuck at "Thinking…").
|
||||
const { client, store, disconnect, emit, getSubscribed } = makeStore()
|
||||
client.sessions.set(S1, sessionState(S1, []))
|
||||
disconnect()
|
||||
const disconnect2 = store.connect()
|
||||
expect(getSubscribed()).toBe(2)
|
||||
|
||||
await store.setSession(S1)
|
||||
emit(turnEvent(S1, 'turn-1', created('turn-1', S1, user('go'))))
|
||||
emit(turnEvent(S1, 'turn-1', requested('turn-1', 0, [user('go')])))
|
||||
emit(turnEvent(S1, 'turn-1', completed('turn-1', 0, assistantText('done'))))
|
||||
emit(turnEvent(S1, 'turn-1', turnCompleted('turn-1')))
|
||||
expect(store.getSnapshot().chatState?.isProcessing).toBe(false)
|
||||
expect(
|
||||
store.getSnapshot().chatState?.conversation.filter(isChatMessage).map((m) => m.content),
|
||||
).toEqual(['go', 'done'])
|
||||
disconnect2()
|
||||
})
|
||||
})
|
||||
|
||||
|
|
@ -267,6 +297,7 @@ describe('SessionListStore', () => {
|
|||
return () => undefined
|
||||
},
|
||||
})
|
||||
store.connect()
|
||||
await store.load()
|
||||
expect(store.getSnapshot().sessions.map((s) => s.sessionId).sort()).toEqual(['a', 'b'])
|
||||
|
||||
|
|
|
|||
|
|
@ -39,7 +39,8 @@ export interface SessionChatStoreDeps {
|
|||
// hook is a thin useSyncExternalStore subscription over it.
|
||||
export class SessionChatStore {
|
||||
private readonly client: SessionsClient
|
||||
private readonly unsubscribeFeed: () => void
|
||||
private readonly subscribeFeed: (listener: SessionFeedListener) => () => void
|
||||
private feedDisconnect: (() => void) | null = null
|
||||
private readonly listeners = new Set<() => void>()
|
||||
|
||||
private sessionId: string | null = null
|
||||
|
|
@ -63,12 +64,20 @@ export class SessionChatStore {
|
|||
|
||||
constructor(deps: SessionChatStoreDeps) {
|
||||
this.client = deps.client
|
||||
this.unsubscribeFeed = deps.subscribeFeed(this.onFeedEvent)
|
||||
this.subscribeFeed = deps.subscribeFeed
|
||||
}
|
||||
|
||||
dispose(): void {
|
||||
this.unsubscribeFeed()
|
||||
this.listeners.clear()
|
||||
// Feed attachment is effect-managed and idempotent so React StrictMode's
|
||||
// mount -> cleanup -> mount cycle re-attaches cleanly (a constructor-made
|
||||
// subscription would be torn down by the first cleanup and never restored).
|
||||
connect(): () => void {
|
||||
if (!this.feedDisconnect) {
|
||||
this.feedDisconnect = this.subscribeFeed(this.onFeedEvent)
|
||||
}
|
||||
return () => {
|
||||
this.feedDisconnect?.()
|
||||
this.feedDisconnect = null
|
||||
}
|
||||
}
|
||||
|
||||
subscribe = (onChange: () => void): (() => void) => {
|
||||
|
|
@ -241,7 +250,8 @@ export interface SessionListSnapshot {
|
|||
|
||||
export class SessionListStore {
|
||||
private readonly client: SessionsClient
|
||||
private readonly unsubscribeFeed: () => void
|
||||
private readonly subscribeFeed: (listener: SessionFeedListener) => () => void
|
||||
private feedDisconnect: (() => void) | null = null
|
||||
private readonly listeners = new Set<() => void>()
|
||||
private entries = new Map<string, SessionIndexEntry>()
|
||||
private loading = true
|
||||
|
|
@ -249,12 +259,17 @@ export class SessionListStore {
|
|||
|
||||
constructor(deps: SessionChatStoreDeps) {
|
||||
this.client = deps.client
|
||||
this.unsubscribeFeed = deps.subscribeFeed(this.onFeedEvent)
|
||||
this.subscribeFeed = deps.subscribeFeed
|
||||
}
|
||||
|
||||
dispose(): void {
|
||||
this.unsubscribeFeed()
|
||||
this.listeners.clear()
|
||||
connect(): () => void {
|
||||
if (!this.feedDisconnect) {
|
||||
this.feedDisconnect = this.subscribeFeed(this.onFeedEvent)
|
||||
}
|
||||
return () => {
|
||||
this.feedDisconnect?.()
|
||||
this.feedDisconnect = null
|
||||
}
|
||||
}
|
||||
|
||||
subscribe = (onChange: () => void): (() => void) => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue