feat(ui): session attention indicators — streaming spinner, unread dot, timestamps (#856)

Closes #856. Co-authored-by: Frank Song <138988108+franksong2702@users.noreply.github.com>
Reviewed-by: nesquena (709bd37 — test isolation fix also included)

tests/test_issue798.py

@@ -133,18 +133,24 @@ def test_concurrent_new_sessions_get_correct_profiles():
     results = {}
     errors = []
 
+    # Patch Session.save ONCE around both threads — not once per thread.
+    # Per-thread `with patch.object(...)` nested across threads has a known
+    # concurrency bug in unittest.mock where one thread's __exit__ can capture
+    # the other thread's mock as the "original" and leave the class attribute
+    # permanently pointing at a MagicMock, breaking every later test that
+    # calls Session.save (any test writing a real session file).
     def make_session(profile_name, key):
         try:
-            with patch.object(m.Session, 'save', return_value=None):
-                s = m.new_session(profile=profile_name)
+            s = m.new_session(profile=profile_name)
             results[key] = s.profile
         except Exception as exc:
             errors.append(exc)
 
-    t1 = threading.Thread(target=make_session, args=('alice', 'alice'))
-    t2 = threading.Thread(target=make_session, args=('bob', 'bob'))
-    t1.start(); t2.start()
-    t1.join(timeout=5); t2.join(timeout=5)
+    with patch.object(m.Session, 'save', return_value=None):
+        t1 = threading.Thread(target=make_session, args=('alice', 'alice'))
+        t2 = threading.Thread(target=make_session, args=('bob', 'bob'))
+        t1.start(); t2.start()
+        t1.join(timeout=5); t2.join(timeout=5)
 
     assert not errors, f"Threads raised: {errors}"
     assert results.get('alice') == 'alice', f"alice session had profile {results.get('alice')!r}"

tests/test_issue856_active_session_read_state.py

@@ -0,0 +1,49 @@
+"""Regression checks for #856 active-session unread state handling."""
+
+from pathlib import Path
+
+
+MESSAGES_JS = (Path(__file__).resolve().parent.parent / "static" / "messages.js").read_text()
+
+
+def test_messages_js_defines_active_session_viewed_helper():
+    assert "function _markSessionViewed(" in MESSAGES_JS, (
+        "messages.js should define a helper that marks the active session as viewed"
+    )
+    assert "_setSessionViewedCount" in MESSAGES_JS, (
+        "active-session viewed helper must delegate to the sidebar viewed-count store"
+    )
+
+
+def test_done_path_marks_active_session_as_viewed():
+    done_idx = MESSAGES_JS.find("source.addEventListener('done'")
+    assert done_idx != -1, "done handler not found in messages.js"
+    done_block = MESSAGES_JS[done_idx:MESSAGES_JS.find("source.addEventListener('stream_end'", done_idx)]
+    assert "_markSessionViewed(activeSid" in done_block, (
+        "done handler must mark the active session as viewed so unread dot does not linger"
+    )
+
+
+def test_cancel_path_marks_active_session_as_viewed():
+    cancel_idx = MESSAGES_JS.find("source.addEventListener('cancel'")
+    assert cancel_idx != -1, "cancel handler not found in messages.js"
+    cancel_block = MESSAGES_JS[cancel_idx:MESSAGES_JS.find("async function _restoreSettledSession()", cancel_idx)]
+    assert "_markSessionViewed(activeSid" in cancel_block, (
+        "cancel handler must mark the active session as viewed after settling messages"
+    )
+
+
+def test_restore_and_error_paths_mark_active_session_as_viewed():
+    restore_idx = MESSAGES_JS.find("async function _restoreSettledSession()")
+    assert restore_idx != -1, "_restoreSettledSession() not found in messages.js"
+    restore_block = MESSAGES_JS[restore_idx:MESSAGES_JS.find("function _handleStreamError()", restore_idx)]
+    assert "_markSessionViewed(activeSid" in restore_block, (
+        "_restoreSettledSession() must mark the active session as viewed"
+    )
+
+    error_idx = MESSAGES_JS.find("function _handleStreamError()")
+    assert error_idx != -1, "_handleStreamError() not found in messages.js"
+    error_block = MESSAGES_JS[error_idx:]
+    assert "_markSessionViewed(activeSid" in error_block, (
+        "_handleStreamError() must mark the active session as viewed"
+    )

tests/test_issue856_pinned_indicator_layout.py

@@ -0,0 +1,68 @@
+"""Regression checks for #856 pinned-star layout in the session list."""
+
+from pathlib import Path
+
+
+SESSIONS_JS = (Path(__file__).resolve().parent.parent / "static" / "sessions.js").read_text()
+STYLE_CSS = (Path(__file__).resolve().parent.parent / "static" / "style.css").read_text()
+
+
+def test_pinned_indicator_renders_inside_title_row():
+    title_row_idx = SESSIONS_JS.find("titleRow.className='session-title-row';")
+    assert title_row_idx != -1, "session title row construction not found"
+
+    pin_idx = SESSIONS_JS.find("pinInd.className='session-pin-indicator';", title_row_idx)
+    assert pin_idx != -1, "pinned indicator creation not found after title row"
+
+    append_to_title_row_idx = SESSIONS_JS.find("titleRow.appendChild(pinInd);", pin_idx)
+    assert append_to_title_row_idx != -1, "pinned indicator should be appended to titleRow"
+
+    append_to_el_idx = SESSIONS_JS.find("el.appendChild(pinInd);", pin_idx)
+    assert append_to_el_idx == -1, (
+        "pinned indicator should not be appended to the outer session row; "
+        "it must align inside the title row with the spinner/unread indicator"
+    )
+
+
+def test_pinned_indicator_uses_fixed_indicator_box():
+    assert ".session-pin-indicator{" in STYLE_CSS, "session pin indicator CSS block missing"
+    css_block = STYLE_CSS[STYLE_CSS.find(".session-pin-indicator{"):STYLE_CSS.find(".session-pin-indicator svg{")]
+    assert "width:10px;" in css_block, "pin indicator should reserve a fixed 10px width"
+    assert "height:10px;" in css_block, "pin indicator should reserve a fixed 10px height"
+    assert "justify-content:center;" in css_block, "pin indicator should center the star inside its box"
+
+
+def test_state_indicator_always_appended_to_prevent_layout_shift():
+    """State span is always added to the DOM (visibility:hidden when inactive) to prevent
+    titles shifting left/right when the spinner or unread dot appears/disappears."""
+    title_row_idx = SESSIONS_JS.find("titleRow.className='session-title-row';")
+    assert title_row_idx != -1, "title row construction not found"
+
+    # state span must be appended unconditionally (no surrounding if-check)
+    append_idx = SESSIONS_JS.find("titleRow.appendChild(state);", title_row_idx)
+    assert append_idx != -1, "state span must always be appended to titleRow"
+
+    # Verify CSS uses visibility:hidden to reserve the slot
+    assert "session-state-indicator{" in STYLE_CSS, "session-state-indicator CSS rule missing"
+    base_block_start = STYLE_CSS.find("session-state-indicator{")
+    base_block_end = STYLE_CSS.find("}", base_block_start)
+    base_block = STYLE_CSS[base_block_start:base_block_end]
+    assert "visibility:hidden;" in base_block, (
+        "session-state-indicator should default to visibility:hidden so it reserves slot "
+        "without being visible — prevents title layout shift on state changes"
+    )
+
+
+def test_apperror_path_calls_render_session_list():
+    """apperror handler must call renderSessionList() to clear the streaming indicator
+    immediately rather than waiting for the 5s streaming poll interval."""
+    messages_js = (Path(__file__).resolve().parent.parent / "static" / "messages.js").read_text()
+    apperror_idx = messages_js.find("source.addEventListener('apperror'")
+    assert apperror_idx != -1, "apperror handler not found in messages.js"
+    warning_idx = messages_js.find("source.addEventListener('warning'", apperror_idx)
+    assert warning_idx != -1, "warning handler not found after apperror handler"
+    apperror_block = messages_js[apperror_idx:warning_idx]
+    assert "renderSessionList()" in apperror_block, (
+        "apperror handler must call renderSessionList() so the streaming indicator "
+        "clears immediately on server errors, not after a 5s poll delay"
+    )

tests/test_issue856_session_streaming_state.py

@@ -0,0 +1,93 @@
+"""
+Regression tests for session streaming indicator payloads used by the session list.
+
+This ensures backend payloads report per-session streaming status from active stream
+tracking, not only for the foreground conversation.
+"""
+
+import threading
+
+import pytest
+
+import api.models as models
+from api.models import Session, all_sessions
+
+
+@pytest.fixture(autouse=True)
+def _isolate_session_stream_state(tmp_path, monkeypatch):
+    """Keep session/index/stream state isolated from the host environment."""
+    session_dir = tmp_path / "sessions"
+    session_dir.mkdir()
+    index_file = session_dir / "_index.json"
+
+    monkeypatch.setattr(models, "SESSION_DIR", session_dir)
+    monkeypatch.setattr(models, "SESSION_INDEX_FILE", index_file)
+    models.SESSIONS.clear()
+
+    stream_map = {}
+    stream_lock = threading.Lock()
+    monkeypatch.setattr(models, "STREAMS", stream_map)
+    monkeypatch.setattr(models, "STREAMS_LOCK", stream_lock)
+
+    yield
+
+    models.SESSIONS.clear()
+
+
+def _make_session(session_id, stream_id=None, message_count=1):
+    s = Session(
+        session_id=session_id,
+        title=session_id,
+        messages=[{"role": "user", "content": f"seed-{session_id}"}] * message_count,
+    )
+    s.active_stream_id = stream_id
+    return s
+
+
+def test_all_sessions_marks_indexed_and_in_memory_streaming_sessions():
+    """Session records from both index and in-memory cache should expose is_streaming."""
+    s_disk = _make_session("disk_session", stream_id="stream-1")
+    s_disk.save()
+
+    s_memory = _make_session("memory_session", stream_id="stream-2")
+    with models.LOCK:
+        models.SESSIONS[s_memory.session_id] = s_memory
+
+    models.STREAMS["stream-1"] = object()
+    models.STREAMS["stream-2"] = object()
+
+    listed = all_sessions()
+    by_sid = {s["session_id"]: s for s in listed}
+
+    assert by_sid["disk_session"]["is_streaming"] is True
+    assert by_sid["memory_session"]["is_streaming"] is True
+    assert by_sid["memory_session"]["active_stream_id"] == "stream-2"
+
+
+def test_all_sessions_marks_streaming_false_when_stream_is_not_active():
+    """Stale active_stream_id should not imply streaming without active STREAMS entry."""
+    s = _make_session("stalesession", stream_id="stale-stream")
+    s.save()
+
+    assert all_sessions()[0]["is_streaming"] is False
+
+    models.STREAMS["stale-stream"] = object()
+    assert all_sessions()[0]["is_streaming"] is True
+
+    models.STREAMS.pop("stale-stream", None)
+    assert all_sessions()[0]["is_streaming"] is False
+
+
+def test_all_sessions_does_not_report_streaming_after_restart_without_active_registry():
+    """Server restarts should not resurrect sidebar streaming state from disk alone."""
+    s = _make_session("restart_session", stream_id="old-stream")
+    s.save()
+
+    models.SESSIONS.clear()
+    reloaded = Session.load("restart_session")
+    assert reloaded is not None
+    assert reloaded.active_stream_id == "old-stream"
+
+    listed = all_sessions()
+    assert listed[0]["active_stream_id"] == "old-stream"
+    assert listed[0]["is_streaming"] is False