fix(session-overview): done sessions never transition to idle

[dimakis]

Summary

• Idle transition timer: touch() now schedules a broadcast at DONE_TIMEOUT_MS (5 min) so the done→idle transition fires proactively, even when no other events occur
• Persistent session timeout: persistentToActivity() now applies the same elapsed-time check instead of hardcoding state: 'done' — old attention sessions transition to idle and disappear from the active list

Root cause

Sessions were stuck as "done" in the Active Sessions list indefinitely because:

1. State derivation was purely on-demand — no timer existed to trigger re-evaluation after 5 minutes of inactivity
2. Sessions from getAttentionSessions() (assistant spoke last) were always returned as done regardless of age

Test plan

• Existing tests pass (38/38)
• New test: idle transition timer broadcasts after DONE_TIMEOUT_MS
• New test: subsequent touch resets the idle timer
• New test: persistent sessions transition to idle after timeout
• Updated test: recent persistent sessions still show as done

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 4 issue(s) (1 warning).

packages/client/src/sse-connection.ts

Solid fix: persistent sessions correctly transition done→idle, and SSE reconnect moves from fragile URL params to a reliable POST. One minor race (unawaited POST before flush) and a timer-precision tradeoff worth noting, but nothing blocking.

• 🟡 bugs (L224): The reconnect POST (doPost) is fire-and-forget — not awaited before flushPendingSends() on line 234. If the client has queued sends, they may arrive at the server before the reconnect POST completes, meaning the server hasn't yet set up watches or reattached sessions. In practice this is likely benign (sends route independently of watch setup, and the server replays missed events), but under high latency the ordering gap widens. Consider awaiting the POST or moving flushPendingSends into the POST's .then() callback. [fixable]

server/session-overview.ts

Solid fix: persistent sessions correctly transition done→idle, and SSE reconnect moves from fragile URL params to a reliable POST. One minor race (unawaited POST before flush) and a timer-precision tradeoff worth noting, but nothing blocking.

• 🔵 unsafe_assumptions (L110): The idle-transition timer is a single shared timer that resets on every touch() call for any session. If session A finishes at t=0 and session B finishes at t=4min, the timer resets to t=9min. Session A's idle broadcast is delayed 4 minutes beyond its actual DONE_TIMEOUT_MS threshold. This is safe because compute() evaluates all sessions at broadcast time (so A will correctly show as idle when the broadcast fires), but the UX lateness scales with inter-session activity gaps. [fixable]
• 🔵 style (L218): The 6-line block comment on the reconnect POST in doConnect() (lines 218–223) largely restates the 4-line comment already present at the const url declaration (lines 198–201). The duplication adds noise; a single-line reference like // POST reconnect — see comment above would suffice. [fixable]

packages/client/src/__tests__/sse-connection.test.ts

Solid fix: persistent sessions correctly transition done→idle, and SSE reconnect moves from fragile URL params to a reliable POST. One minor race (unawaited POST before flush) and a timer-precision tradeoff worth noting, but nothing blocking.

• 🔵 missing_tests: No test verifies behavior when the reconnect POST fails (e.g., network error or non-ok response). The doPost catch block silently swallows errors, which is intentional — a test documenting that the connection stays healthy after a failed reconnect POST would guard this invariant. [fixable]

[dimakis]

Centaur Review

Found 3 issue(s).

server/session-overview.ts

Clean, well-tested fix for two real bugs (SSE reconnect missing session reattach, done sessions never transitioning to idle). No correctness issues found — just minor suggestions around multi-session timer edge-case documentation and test coverage.

• 🔵 unsafe_assumptions (L110): scheduleIdleTransition resets to a single timer on every touch, keyed to the most recent touch only. If multiple sessions have staggered touch times (e.g., client-1 touched 4 min ago, client-2 touched just now), the timer fires at DONE_TIMEOUT_MS after client-2's touch (5 more minutes). Client-1's done→idle transition at the 5-minute mark is not proactively broadcast — it relies on compute() being triggered by some other event (e.g., the 60s uncommitted refresh). In practice this means up to 60s of stale 'done' state for sessions that hit the threshold between the last touch and the next refresh. Acceptable latency for a UI badge, but worth documenting. [fixable]

packages/client/src/__tests__/sse-connection.test.ts

Clean, well-tested fix for two real bugs (SSE reconnect missing session reattach, done sessions never transitioning to idle). No correctness issues found — just minor suggestions around multi-session timer edge-case documentation and test coverage.

• 🔵 missing_tests (L293): No test covers reconnect POST with multiple tracked sessions. The current test tracks only one session. A test with 2+ sessions would verify the serialization of the sessions array and confirm ordering is stable. [fixable]

packages/client/src/sse-connection.ts

Clean, well-tested fix for two real bugs (SSE reconnect missing session reattach, done sessions never transitioning to idle). No correctness issues found — just minor suggestions around multi-session timer edge-case documentation and test coverage.

• 🔵 style (L218): The 6-line comment block (lines 218-223) inside the welcome handler restates the problem description from the commit message and the comment at lines 198-201. The rationale is already captured once above doConnect(); repeating it in the handler adds noise without new information. [fixable]

[dimakis]

🔵 unsafe_assumptions: scheduleIdleTransition resets to a single timer on every touch, keyed to the most recent touch only. If multiple sessions have staggered touch times (e.g., client-1 touched 4 min ago, client-2 touched just now), the timer fires at DONE_TIMEOUT_MS after client-2's touch (5 more minutes). Client-1's done→idle transition at the 5-minute mark is not proactively broadcast — it relies on compute() being triggered by some other event (e.g., the 60s uncommitted refresh). In practice this means up to 60s of stale 'done' state for sessions that hit the threshold between the last touch and the next refresh. Acceptable latency for a UI badge, but worth documenting. [fixable]

[dimakis]

🔵 missing_tests: No test covers reconnect POST with multiple tracked sessions. The current test tracks only one session. A test with 2+ sessions would verify the serialization of the sessions array and confirm ordering is stable. [fixable]

[dimakis]

🔵 style: The 6-line comment block (lines 218-223) inside the welcome handler restates the problem description from the commit message and the comment at lines 198-201. The rationale is already captured once above doConnect(); repeating it in the handler adds noise without new information. [fixable]

[dimakis]

Requesting second Centaur review. Context:

• First review found 4 items, 3 of which are in SSE code that's already on main (will drop on rebase)
• The SSE reconnect race (finding feat: image attachment support for chat sessions #1) has been fixed separately in fix(client): await reconnect POST before flushing pending sends #390
• The remaining finding (fix: auto-reconnect WebSocket and show connection state #2, single idle timer) was acknowledged as safe-by-design
• Ready for second review of the actual session-overview changes

[dimakis]

Centaur Review

Found 4 issue(s).

server/session-overview.ts

Clean, well-tested fix. The SSE reconnect change correctly moves session data from URL query params to POST (fixing EventSource auto-reconnect), and the session-overview change properly transitions persistent done sessions to idle. Only minor suggestions around timer precision with multiple staggered sessions.

• 🔵 unsafe_assumptions (L110): scheduleIdleTransition() uses a single timer that resets on every touch(). With multiple sessions touched at different times, earlier sessions don't get a precisely-timed idle broadcast — they rely on the 60-second uncommitted-refresh cycle. For example, session A touched at T=0 and session B touched at T=3min means session A won't get an idle broadcast until T=8min (from B's timer) or T=5min+60s (from the background refresh). Consider scheduling based on the earliest pending transition rather than always resetting to DONE_TIMEOUT_MS from now. [fixable]
• 🔵 style (L107): Comment says 'only the most recent touch matters since compute() evaluates all sessions at broadcast time' — this is true for correctness but misleading about timeliness. Earlier sessions' done→idle transitions are delayed until either this timer fires or the 60s background refresh runs. Consider noting the worst-case delay. [fixable]

server/__tests__/session-overview.test.ts

Clean, well-tested fix. The SSE reconnect change correctly moves session data from URL query params to POST (fixing EventSource auto-reconnect), and the session-overview change properly transitions persistent done sessions to idle. Only minor suggestions around timer precision with multiple staggered sessions.

• 🔵 missing_tests: No test covers the multi-session stagger scenario: session A touched at T=0, session B touched at T=3min — verify session A transitions to idle at approximately T=5min (not delayed until B's timer fires at T=8min). This would document the current behavior and catch regressions if the timer strategy changes. [fixable]
• 🔵 missing_tests: No test verifies that persistent sessions (from getAttentionSessions) schedule an idle transition timer. Currently persistent sessions only transition to idle when some other event triggers a broadcast (background refresh, another touch). A test exercising this path would make the timing contract explicit. [fixable]

[dimakis]

🔵 unsafe_assumptions: scheduleIdleTransition() uses a single timer that resets on every touch(). With multiple sessions touched at different times, earlier sessions don't get a precisely-timed idle broadcast — they rely on the 60-second uncommitted-refresh cycle. For example, session A touched at T=0 and session B touched at T=3min means session A won't get an idle broadcast until T=8min (from B's timer) or T=5min+60s (from the background refresh). Consider scheduling based on the earliest pending transition rather than always resetting to DONE_TIMEOUT_MS from now. [fixable]

[dimakis]

🔵 style: Comment says 'only the most recent touch matters since compute() evaluates all sessions at broadcast time' — this is true for correctness but misleading about timeliness. Earlier sessions' done→idle transitions are delayed until either this timer fires or the 60s background refresh runs. Consider noting the worst-case delay. [fixable]