Skip to content

fix(sessions): live-refresh session list via SSE #346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dimakis merged 1 commit into from
May 29, 2026
+30 −0
Merged

fix(sessions): live-refresh session list via SSE #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions frontend/src/hooks/useSessionList.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,19 @@ export function useSessionList(): UseSessionListReturn {
};
document.addEventListener('visibilitychange', onVisible);

// Refetch session list when sessions are created, renamed, or deleted
const unsubChanged = eventBus.on('sessions_changed', () => {
Copy link
Copy Markdown
Owner Author

@dimakis dimakis May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

Copy link
Copy Markdown
Owner Author

@dimakis dimakis May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

Copy link
Copy Markdown
Owner Author

@dimakis dimakis May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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]

Copy link
Copy Markdown
Owner Author

@dimakis dimakis May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

apiFetch('/api/sessions')
.then((r) => r.json())
.then((data) => {
const { sessions: page, hasMore: more } = parseSessionsResponse(data);
setSessions(page);
setHasMore(more);
nextOffset.current = page.length;
Copy link
Copy Markdown
Owner Author

@dimakis dimakis May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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]

})
.catch(() => {});
});

// Live session dots via SSE — update isActive/isAttached without full refetch
const unsubActivity = eventBus.on('session_activity', (data) => {
const activities = data as SessionActivity[];
Expand All @@ -105,6 +118,7 @@ export function useSessionList(): UseSessionListReturn {

return () => {
document.removeEventListener('visibilitychange', onVisible);
unsubChanged();
unsubActivity();
};
}, []);
Expand Down
5 changes: 5 additions & 0 deletions server/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,8 @@ async function handleSessionCreate(
log.info('headless session started', { sessionId: wtId, prompt: initialPrompt });
}

sseRegistry.broadcast('sessions_changed', {});
Copy link
Copy Markdown
Owner Author

@dimakis dimakis May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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]


res.status(initialPrompt ? 201 : 200).json({
sessionId: wtId,
worktrees,
Expand Down Expand Up @@ -1103,6 +1105,7 @@ app.get('/api/sessions/:id/messages', async (req, res) => {

app.delete('/api/sessions/:id', (req, res) => {
hideSession(req.params.id as string);
sseRegistry.broadcast('sessions_changed', {});
res.json({ ok: true });
});

Expand Down Expand Up @@ -1139,6 +1142,7 @@ app.get('/api/sessions/:id/events', (req, res) => {

app.delete('/api/sessions', (_req, res) => {
hideAllSessions();
sseRegistry.broadcast('sessions_changed', {});
res.json({ ok: true });
});

Expand All @@ -1150,6 +1154,7 @@ app.put('/api/sessions/:id/rename', async (req, res) => {
}
try {
await renameSessionById(req.params.id, title.slice(0, 200));
sseRegistry.broadcast('sessions_changed', {});
res.json({ ok: true });
} catch {
res.status(404).json({ error: 'Session not found' });
Expand Down
6 changes: 6 additions & 0 deletions server/chat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ let _onSessionChange: SessionChangeCallback | null = null;
export function setSessionChangeCallback(cb: SessionChangeCallback): void {
_onSessionChange = cb;
}

let _onSessionsChanged: (() => void) | null = null;
Copy link
Copy Markdown
Owner Author

@dimakis dimakis May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

Copy link
Copy Markdown
Owner Author

@dimakis dimakis May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

export function setSessionsChangedCallback(cb: () => void): void {
_onSessionsChanged = cb;
}
import { EventStore } from './event-store.js';
import { capturePromptComparison } from './prompt-compare.js';
import { shouldAutoRename, extractRecentPrompts, generateSessionName } from './auto-rename.js';
Expand Down Expand Up @@ -1015,6 +1020,7 @@ async function tryAutoRename(sessionId: string, clientId: string): Promise<void>

// Persist to EventStore first — survives SDK rename failures.
eventStore.upsertSession({ sessionId, summary: newName });
_onSessionsChanged?.();
Copy link
Copy Markdown
Owner Author

@dimakis dimakis May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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]


// Update the SDK session name (best-effort, fire-and-forget)
renameSessionById(sessionId, newName, false).catch((err: unknown) => {
Expand Down
5 changes: 5 additions & 0 deletions server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
getRepoConfig,
setConnectionRegistry,
setSessionChangeCallback,
setSessionsChangedCallback,
reconcileSessionsBackground,
} from './chat.js';
import { cleanupStaleWorktrees, countWorktrees } from './worktree.js';
Expand Down Expand Up @@ -271,6 +272,10 @@ setSessionChangeCallback((clientId, event, sessionId) => {
overviewEmitter.scheduleBroadcast();
});

setSessionsChangedCallback(() => {
sseRegistry.broadcast('sessions_changed', {});
});

server.on('upgrade', async (req, socket, head) => {
const url = new URL(req.url || '', `http://${req.headers.host}`);

Expand Down
Loading