Stage 404: PR #2830 — fix(sessions): keep pin state authoritative by @franksong2702 (closes #2821)

Agent reviewer 'LGTM. Ship it.'
- Bug A fix: _session_field helper handles dict-vs-object snapshot in pin-limit check
- Bug B fix: removed stale client-side pinLimitReached short-circuit
- Bug C recovery: renderSessionList() on pin/unpin failure refreshes from server

Co-authored-by: franksong2702 <146128127+franksong2702@users.noreply.github.com>

api/routes.py

@@ -77,6 +77,12 @@ _CSP_REPORT_RATE_LIMIT_MAX = 100
 _CSP_REPORT_MAX_BODY_BYTES = 64 * 1024
 
 
+def _session_field(session, field, default=None):
+    if isinstance(session, dict):
+        return session.get(field, default)
+    return getattr(session, field, default)
+
+
 # ── Profile-scoped session/project filtering (#1611, #1614) ────────────────
 #
 # Sessions and projects are stored in the WebUI sidecar without per-row
@@ -5837,8 +5843,8 @@ def handle_post(handler, parsed) -> bool:
             # Pre-snapshot from persisted index (acquires LOCK internally,
             # so must run outside our own LOCK acquire below).
             persisted_pinned_ids = {
-                getattr(existing, "session_id", None) for existing in all_sessions()
-                if getattr(existing, "pinned", False) and not getattr(existing, "archived", False)
+                _session_field(existing, "session_id", None) for existing in all_sessions()
+                if _session_field(existing, "pinned", False) and not _session_field(existing, "archived", False)
             }
             with LOCK:
                 # Final authoritative count: merge persisted-pinned with the

static/sessions.js

@@ -1828,26 +1828,24 @@ function _openSessionActionMenu(session, anchorEl){
       }
     ));
   }
-  const pinLimitReached=!session.pinned&&_pinnedSessionCount()>=_getPinnedSessionsLimit();
   menu.appendChild(_buildSessionAction(
     session.pinned?t('session_unpin'):t('session_pin'),
-    pinLimitReached?_pinnedSessionsLimitMessage():(session.pinned?t('session_unpin_desc'):t('session_pin_desc')),
+    session.pinned?t('session_unpin_desc'):t('session_pin_desc'),
     session.pinned?ICONS.pin:ICONS.unpin,
     async()=>{
       closeSessionActionMenu();
-      if(pinLimitReached){
-        if(typeof showToast==='function') showToast(_pinnedSessionsLimitMessage(),3000,'error');
-        return;
-      }
       const newPinned=!session.pinned;
       try{
         await api('/api/session/pin',{method:'POST',body:JSON.stringify({session_id:session.session_id,pinned:newPinned})});
         session.pinned=newPinned;
         if(S.session&&S.session.session_id===session.session_id) S.session.pinned=newPinned;
         renderSessionList();
-      }catch(err){showToast(t('session_pin_failed')+err.message);}
+      }catch(err){
+        showToast(t('session_pin_failed')+err.message);
+        await renderSessionList();
+      }
     },
-    (session.pinned?'is-active':'')+(pinLimitReached?' is-disabled':'')
+    session.pinned?'is-active':''
   ));
   menu.appendChild(_buildSessionAction(
     t('session_move_project'),

tests/test_configurable_pinned_sessions_limit.py

@@ -54,7 +54,8 @@ def test_pin_limit_setting_is_exposed_and_wired_through_ui():
     assert "window._pinnedSessionsLimit=parseInt(s.pinned_sessions_limit||3,10)||3" in BOOT_JS
     assert "function _getPinnedSessionsLimit()" in SESSIONS_JS
     assert "function _pinnedSessionsLimit()" not in SESSIONS_JS
-    assert "_pinnedSessionCount()>=_getPinnedSessionsLimit()" in SESSIONS_JS
+    assert "_pinnedSessionCount()>=_getPinnedSessionsLimit()" not in SESSIONS_JS
+    assert "await api('/api/session/pin'" in SESSIONS_JS
 
 
 def test_settings_api_persists_integer_pin_limit_and_rejects_invalid_values():

tests/test_issue2508_session_pin_cap.py

@@ -77,7 +77,9 @@ def test_session_pin_cap_has_backend_and_frontend_guards():
     assert 'function _pinnedSessionCount()' in SESSIONS_JS
     assert 'function _getPinnedSessionsLimit()' in SESSIONS_JS
     assert 'function _pinnedSessionsLimit()' not in SESSIONS_JS
-    assert 'const pinLimitReached=!session.pinned&&_pinnedSessionCount()>=_getPinnedSessionsLimit();' in SESSIONS_JS
+    assert 'const pinLimitReached=!session.pinned&&_pinnedSessionCount()>=_getPinnedSessionsLimit();' not in SESSIONS_JS
+    assert 'if(pinLimitReached)' not in SESSIONS_JS
+    assert "await api('/api/session/pin'" in SESSIONS_JS
     assert 'Only ${limit} conversations can be pinned' in SESSIONS_JS
     assert ".session-action-opt.is-disabled{opacity:.55;cursor:not-allowed;}" in STYLE_CSS
 

tests/test_issue2821_session_pin_state_sync.py

@@ -0,0 +1,70 @@
+"""Regression checks for #2821 session pin/unpin state sync."""
+
+from pathlib import Path
+
+
+ROOT = Path(__file__).resolve().parent.parent
+ROUTES_PY = (ROOT / "api" / "routes.py").read_text(encoding="utf-8")
+SESSIONS_JS = (ROOT / "static" / "sessions.js").read_text(encoding="utf-8")
+
+
+def _function_block(src: str, name: str) -> str:
+    marker = f"function {name}"
+    start = src.find(marker)
+    assert start != -1, f"{name} not found"
+    brace = src.find("{", start)
+    assert brace != -1, f"{name} body not found"
+    depth = 1
+    i = brace + 1
+    while i < len(src) and depth:
+        if src[i] == "{":
+            depth += 1
+        elif src[i] == "}":
+            depth -= 1
+        i += 1
+    assert depth == 0, f"{name} body did not close"
+    return src[start:i]
+
+
+def test_session_field_helper_reads_dicts_and_objects():
+    from api.routes import _session_field
+
+    class SessionLike:
+        session_id = "obj-1"
+        pinned = True
+        archived = False
+
+    assert _session_field({"session_id": "dict-1", "pinned": True}, "pinned", False) is True
+    assert _session_field({"session_id": "dict-1"}, "archived", False) is False
+    assert _session_field(SessionLike(), "session_id", None) == "obj-1"
+    assert _session_field(SessionLike(), "missing", "fallback") == "fallback"
+
+
+def test_pin_limit_snapshot_counts_index_dict_entries():
+    assert "_session_field(existing, \"session_id\", None)" in ROUTES_PY
+    assert "_session_field(existing, \"pinned\", False)" in ROUTES_PY
+    assert "_session_field(existing, \"archived\", False)" in ROUTES_PY
+    start = ROUTES_PY.find("persisted_pinned_ids = {")
+    assert start != -1, "persisted pin snapshot not found"
+    end = ROUTES_PY.find("with LOCK:", start)
+    assert end != -1, "persisted pin snapshot should be computed before LOCK"
+    persisted_snapshot = ROUTES_PY[start:end]
+    assert 'getattr(existing, "pinned", False)' not in persisted_snapshot
+    assert 'getattr(existing, "archived", False)' not in persisted_snapshot
+
+
+def test_pin_action_does_not_short_circuit_on_stale_client_count():
+    body = _function_block(SESSIONS_JS, "_openSessionActionMenu")
+    assert "const pinLimitReached=" not in body
+    assert "if(pinLimitReached)" not in body
+    assert "_pinnedSessionCount()>=_getPinnedSessionsLimit()" not in body
+    assert "await api('/api/session/pin'" in body
+
+
+def test_pin_action_refreshes_session_list_after_pin_failure():
+    body = _function_block(SESSIONS_JS, "_openSessionActionMenu")
+    catch_idx = body.find("}catch(err){")
+    assert catch_idx != -1, "Pin/unpin action must have an error path"
+    catch_block = body[catch_idx:body.find("}", catch_idx + len("}catch(err){")) + 1]
+    assert "showToast(t('session_pin_failed')+err.message)" in catch_block
+    assert "await renderSessionList()" in catch_block