fix(sessions): live-refresh session list via SSE#346
Conversation
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 4 issue(s) (1 warning).
frontend/src/hooks/useSessionList.ts
Clean, minimal implementation that correctly wires SSE broadcasts for session mutations. Main gaps are missing test coverage for the new broadcast calls and a lack of debouncing on the frontend consumer.
- 🔵 unsafe_assumptions (L92): No debounce/coalescing on the
sessions_changedhandler. If multiple events fire in rapid succession (e.g. auto-rename right after create), each triggers a full/api/sessionsfetch. The existingsession_activitypath coalesces viascheduleBroadcast()(200ms), butsessions_changedfires and is consumed immediately. Consider debouncing the handler or coalescing on the server side.[fixable] - 🔵 unsafe_assumptions (L92): Self-initiated operations (
dismissSession,clearAll,handleRename) optimistically update state and then call the server API, which broadcastssessions_changedback to the same client. This triggers a redundant full refetch that overwrites the already-correct optimistic state. Not harmful, but wasteful — could skip the SSE-triggered refetch for actions the client initiated itself.[fixable]
server/app.ts
Clean, minimal implementation that correctly wires SSE broadcasts for session mutations. Main gaps are missing test coverage for the new broadcast calls and a lack of debouncing on the frontend consumer.
- 🟡 missing_tests (L492): No tests verify that
sseRegistry.broadcast('sessions_changed', {})is called after session create, delete, delete-all, or rename. The existing route tests inserver/__tests__/routes.test.ts(lines 402-431) test these endpoints but don't mock or assert onsseRegistry. Similarly, the newsetSessionsChangedCallbackwiring inchat.ts(line 1003) andindex.ts(line 275) has no test coverage.[fixable]
server/chat.ts
Clean, minimal implementation that correctly wires SSE broadcasts for session mutations. Main gaps are missing test coverage for the new broadcast calls and a lack of debouncing on the frontend consumer.
- 🔵 style (L64): The new
_onSessionsChangeddeclaration (lines 64–67) sits between module-level code and an import statement on line 68 (import { EventStore }). This matches the existing pattern for_onSessionChangeabove it, but imports-after-code is unusual and would be flagged byimport/firstlinting rules. Consider moving the callback declarations below all imports if the file is ever restructured.[fixable]
| document.addEventListener('visibilitychange', onVisible); | ||
|
|
||
| // Refetch session list when sessions are created, renamed, or deleted | ||
| const unsubChanged = eventBus.on('sessions_changed', () => { |
There was a problem hiding this comment.
🔵 unsafe_assumptions: No debounce/coalescing on the sessions_changed handler. If multiple events fire in rapid succession (e.g. auto-rename right after create), each triggers a full /api/sessions fetch. The existing session_activity path coalesces via scheduleBroadcast() (200ms), but sessions_changed fires and is consumed immediately. Consider debouncing the handler or coalescing on the server side. [fixable]
| document.addEventListener('visibilitychange', onVisible); | ||
|
|
||
| // Refetch session list when sessions are created, renamed, or deleted | ||
| const unsubChanged = eventBus.on('sessions_changed', () => { |
There was a problem hiding this comment.
🔵 unsafe_assumptions: Self-initiated operations (dismissSession, clearAll, handleRename) optimistically update state and then call the server API, which broadcasts sessions_changed back to the same client. This triggers a redundant full refetch that overwrites the already-correct optimistic state. Not harmful, but wasteful — could skip the SSE-triggered refetch for actions the client initiated itself. [fixable]
| log.info('headless session started', { sessionId: wtId, prompt: initialPrompt }); | ||
| } | ||
|
|
||
| sseRegistry.broadcast('sessions_changed', {}); |
There was a problem hiding this comment.
🟡 missing_tests: No tests verify that sseRegistry.broadcast('sessions_changed', {}) is called after session create, delete, delete-all, or rename. The existing route tests in server/__tests__/routes.test.ts (lines 402-431) test these endpoints but don't mock or assert on sseRegistry. Similarly, the new setSessionsChangedCallback wiring in chat.ts (line 1003) and index.ts (line 275) has no test coverage. [fixable]
| _onSessionChange = cb; | ||
| } | ||
|
|
||
| let _onSessionsChanged: (() => void) | null = null; |
There was a problem hiding this comment.
🔵 style: The new _onSessionsChanged declaration (lines 64–67) sits between module-level code and an import statement on line 68 (import { EventStore }). This matches the existing pattern for _onSessionChange above it, but imports-after-code is unusual and would be flagged by import/first linting rules. Consider moving the callback declarations below all imports if the file is ever restructured. [fixable]
The "all sessions" list only refreshed on page visibility change, so new sessions, renames, and deletions were invisible until navigating away and back. Broadcast a `sessions_changed` SSE event from every session mutation (create, delete, delete-all, rename, auto-rename) and subscribe to it in useSessionList to trigger a refetch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9f925cd to
0355ac3
Compare
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (3 warning).
frontend/src/hooks/useSessionList.ts
Functional and follows existing patterns, but the full-refetch approach on every sessions_changed event resets pagination and creates redundant fetches after optimistic updates — a debounce and/or smarter merge would harden this.
- 🟡 bugs (L92): No debounce on
sessions_changed. Rapid successive events (e.g., bulk operations, auto-rename + manual rename close together) will fire multiple concurrentGET /api/sessionsfetches that race against each other, potentially causing state flicker as stale responses overwrite fresh ones. Consider coalescing with a short debounce or ignoring stale responses.[fixable] - 🟡 regressions (L99): The
sessions_changedhandler resets pagination (nextOffset.current = page.length) and replaces the full list with just the first page. If a user has scrolled to load more sessions, all additional pages vanish when any session is created, renamed, or deleted. ThedismissSessionandclearAllcallbacks already do optimistic local updates, then the SSE broadcast triggers a redundant full refetch that overwrites them.[fixable] - 🔵 missing_tests (L92): The existing
useSessionList.test.tsmockseventBus.onbut doesn't test the newsessions_changedsubscription — no assertion that the refetch fires or that state updates correctly when the event arrives.[fixable]
server/chat.ts
Functional and follows existing patterns, but the full-refetch approach on every sessions_changed event resets pagination and creates redundant fetches after optimistic updates — a debounce and/or smarter merge would harden this.
- 🔵 style (L65): The new
_onSessionsChangedcallback + setter is placed between existing executable code (line 50) and import statements (line 69), continuing a pre-existing interleaving pattern. While ESM hoists imports so this works at runtime, grouping it with the existing callbacks above the imports is fine — just noting this is a style debt that predates this PR. - 🟡 missing_tests (L1023): The
_onSessionsChanged?.()call insidetryAutoRenamehas no test coverage. The existingauto-rename.test.tstests pure functions only (shouldAutoRename,extractRecentPrompts,generateSessionName) but not the integration intryAutoRename. There's also no test verifying the foursseRegistry.broadcast('sessions_changed', {})calls inapp.tsroute handlers.[fixable]
| document.addEventListener('visibilitychange', onVisible); | ||
|
|
||
| // Refetch session list when sessions are created, renamed, or deleted | ||
| const unsubChanged = eventBus.on('sessions_changed', () => { |
There was a problem hiding this comment.
🟡 bugs: No debounce on sessions_changed. Rapid successive events (e.g., bulk operations, auto-rename + manual rename close together) will fire multiple concurrent GET /api/sessions fetches that race against each other, potentially causing state flicker as stale responses overwrite fresh ones. Consider coalescing with a short debounce or ignoring stale responses. [fixable]
| const { sessions: page, hasMore: more } = parseSessionsResponse(data); | ||
| setSessions(page); | ||
| setHasMore(more); | ||
| nextOffset.current = page.length; |
There was a problem hiding this comment.
🟡 regressions: The sessions_changed handler resets pagination (nextOffset.current = page.length) and replaces the full list with just the first page. If a user has scrolled to load more sessions, all additional pages vanish when any session is created, renamed, or deleted. The dismissSession and clearAll callbacks already do optimistic local updates, then the SSE broadcast triggers a redundant full refetch that overwrites them. [fixable]
| document.addEventListener('visibilitychange', onVisible); | ||
|
|
||
| // Refetch session list when sessions are created, renamed, or deleted | ||
| const unsubChanged = eventBus.on('sessions_changed', () => { |
There was a problem hiding this comment.
🔵 missing_tests: The existing useSessionList.test.ts mocks eventBus.on but doesn't test the new sessions_changed subscription — no assertion that the refetch fires or that state updates correctly when the event arrives. [fixable]
| _onSessionChange = cb; | ||
| } | ||
|
|
||
| let _onSessionsChanged: (() => void) | null = null; |
There was a problem hiding this comment.
🔵 style: The new _onSessionsChanged callback + setter is placed between existing executable code (line 50) and import statements (line 69), continuing a pre-existing interleaving pattern. While ESM hoists imports so this works at runtime, grouping it with the existing callbacks above the imports is fine — just noting this is a style debt that predates this PR.
|
|
||
| // Persist to EventStore first — survives SDK rename failures. | ||
| eventStore.upsertSession({ sessionId, summary: newName }); | ||
| _onSessionsChanged?.(); |
There was a problem hiding this comment.
🟡 missing_tests: The _onSessionsChanged?.() call inside tryAutoRename has no test coverage. The existing auto-rename.test.ts tests pure functions only (shouldAutoRename, extractRecentPrompts, generateSessionName) but not the integration in tryAutoRename. There's also no test verifying the four sseRegistry.broadcast('sessions_changed', {}) calls in app.ts route handlers. [fixable]
Summary
sessions_changedSSE event broadcast from every session mutation point (create, delete, delete-all, rename, auto-rename)useSessionListhook subscribes to the event and refetches the listTest plan
🤖 Generated with Claude Code