fix: review comments

Author: bradjin8

services/workspace_tabs.py

@@ -37,6 +37,16 @@ def _extract_chat_id_from_code_block_diff_key(key: str) -> str | None:
     return m.group(1) if m else None
 
 
+def _loads_disk_kv_value(raw: Any) -> Any | None:
+    """Parse a cursorDiskKV ``value`` column; ``None`` if missing or unparseable."""
+    if raw is None:
+        return None
+    try:
+        return json.loads(raw)
+    except (json.JSONDecodeError, TypeError, ValueError):
+        return None
+
+
 def assemble_workspace_tabs(
     workspace_id: str,
     workspace_path: str,
@@ -97,32 +107,30 @@ def _safe_fetchall(query: str, params: tuple = ()) -> list:
             parts = row["key"].split(":")
             if len(parts) >= 3:
                 bid = parts[2]
-                if row["value"] is None:
+                parsed = _loads_disk_kv_value(row["value"])
+                if parsed is None:
                     continue
                 try:
-                    bubble_obj = Bubble.from_dict(json.loads(row["value"]), bubble_id=bid)
+                    bubble_obj = Bubble.from_dict(parsed, bubble_id=bid)
                     bubble_map[bid] = bubble_obj.raw
                 except SchemaError as e:
                     # Drift logged so the operator can chase disappearing
                     # bubbles instead of guessing. Bad row still skipped so the
                     # tabs endpoint can't 500 on one malformed bubble.
                     print(f"Schema drift in bubble {bid}: {e}")
-                except (json.JSONDecodeError, ValueError):
-                    pass
 
         # Load codeBlockDiffs
         for row in _safe_fetchall("SELECT key, value FROM cursorDiskKV WHERE key LIKE 'codeBlockDiff:%'"):
             chat_id = _extract_chat_id_from_code_block_diff_key(row["key"])
             if not chat_id:
                 continue
-            try:
-                d = json.loads(row["value"])
-                code_block_diff_map.setdefault(chat_id, []).append({
-                    **d,
-                    "diffId": row["key"].split(":")[2] if len(row["key"].split(":")) > 2 else None,
-                })
-            except Exception:
-                pass
+            d = _loads_disk_kv_value(row["value"])
+            if not isinstance(d, dict):
+                continue
+            code_block_diff_map.setdefault(chat_id, []).append({
+                **d,
+                "diffId": row["key"].split(":")[2] if len(row["key"].split(":")) > 2 else None,
+            })
 
         # Load messageRequestContext rows once; build both
         # message_request_context_map and project_layouts_map from the same pass.
@@ -132,10 +140,7 @@ def _safe_fetchall(query: str, params: tuple = ()) -> list:
             if len(parts) < 2:
                 continue
             chat_id = parts[1]
-            try:
-                ctx = json.loads(row["value"])
-            except Exception:
-                continue
+            ctx = _loads_disk_kv_value(row["value"])
             if not isinstance(ctx, dict):
                 continue
 
@@ -153,9 +158,8 @@ def _safe_fetchall(query: str, params: tuple = ()) -> list:
                 project_layouts_map.setdefault(chat_id, [])
                 for layout in layouts:
                     if isinstance(layout, str):
-                        try:
-                            layout = json.loads(layout)
-                        except Exception:
+                        layout = _loads_disk_kv_value(layout)
+                        if not isinstance(layout, dict):
                             continue
                     if isinstance(layout, dict) and layout.get("rootPath"):
                         project_layouts_map[chat_id].append(layout["rootPath"])
@@ -180,16 +184,17 @@ def _safe_fetchall(query: str, params: tuple = ()) -> list:
 
         for row in composer_rows:
             composer_id = row["key"].split(":")[1]
+            parsed = _loads_disk_kv_value(row["value"])
+            if parsed is None:
+                continue
             try:
-                composer = Composer.from_dict(json.loads(row["value"]), composer_id=composer_id)
+                composer = Composer.from_dict(parsed, composer_id=composer_id)
             except SchemaError as e:
                 # Drift skipped + logged so the two primary conversation
                 # paths (list_workspaces + get_workspace_tabs) agree on what
                 # counts as a valid composer.
                 print(f"Schema drift in composer {composer_id}: {e}")
                 continue
-            except (json.JSONDecodeError, TypeError, ValueError):
-                continue
             try:
                 cd = composer.raw
 

tests/test_workspace_tabs_null_bubble.py

@@ -2,8 +2,8 @@
 
 A cursorDiskKV row with a NULL value column previously caused
 json.loads(None) -> TypeError, which propagated as a 500 response.
-The fix adds an explicit None-guard before json.loads in the bubble
-loading loop of services/workspace_tabs.py.
+The fix uses ``_loads_disk_kv_value`` in ``services/workspace_tabs.py`` so
+NULL / unparseable cursorDiskKV values are skipped without raising.
 """
 
 import json
@@ -12,6 +12,13 @@
 import tempfile
 import unittest
 
+from services.workspace_tabs import assemble_workspace_tabs
+
+# cursorDiskKV keys use typed prefixes; tabs[].id is the bare suffix only
+# (assemble_workspace_tabs: composer_id = row["key"].split(":")[1]).
+COMPOSER_ID = "composer-abc"
+COMPOSER_KV_KEY = f"composerData:{COMPOSER_ID}"
+
 
 class TestNullBubbleValueDoesNotCrashTabs(unittest.TestCase):
     def setUp(self):
@@ -30,21 +37,21 @@ def setUp(self):
         conn.execute("CREATE TABLE cursorDiskKV ([key] TEXT PRIMARY KEY, value TEXT)")
         conn.execute(
             "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
-            ("bubbleId:composer-abc:bubble-null", None),  # NULL value — the crash case
+            (f"bubbleId:{COMPOSER_ID}:bubble-null", None),  # NULL value — the crash case
         )
         # Healthy bubble that should surface in the assembled tab.
         conn.execute(
             "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
             (
-                "bubbleId:composer-abc:bubble-ok",
+                f"bubbleId:{COMPOSER_ID}:bubble-ok",
                 json.dumps({"type": 1, "text": "hello world", "createdAt": 1739200000000}),
             ),
         )
         # Composer referencing the healthy bubble — required for a tab to be built.
         conn.execute(
             "INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
             (
-                "composerData:composer-abc",
+                COMPOSER_KV_KEY,
                 json.dumps({
                     "name": "Test Chat",
                     "modelConfig": {"modelName": "gpt-4o"},
@@ -64,8 +71,6 @@ def tearDown(self):
 
     def test_null_bubble_row_is_skipped_without_exception(self):
         """assemble_workspace_tabs must not raise when a bubble row has NULL value."""
-        from services.workspace_tabs import assemble_workspace_tabs
-
         try:
             _payload, status = assemble_workspace_tabs(
                 workspace_id="global",
@@ -75,34 +80,34 @@ def test_null_bubble_row_is_skipped_without_exception(self):
         except TypeError as exc:
             self.fail(f"NULL bubble row raised TypeError: {exc}")
 
-        self.assertEqual(status, 200)
+        self.assertEqual(status, 200, "NULL bubble row must not turn tabs load into an error response")
 
     def test_healthy_bubbles_still_load_when_null_row_present(self):
         """The healthy bubble surfaces in a tab even when a NULL row is present."""
-        from services.workspace_tabs import assemble_workspace_tabs
-
         payload, status = assemble_workspace_tabs(
             workspace_id="global",
             workspace_path=self.workspace_path,
             rules=[],
         )
-        self.assertEqual(status, 200)
-        self.assertIsInstance(payload, dict)
+        self.assertEqual(status, 200, "tabs endpoint must succeed when only the null bubble row is bad")
+        self.assertIsInstance(payload, dict, "tabs response must be a JSON object envelope")
         tabs = payload.get("tabs", [])
-        self.assertEqual(len(tabs), 1, "Expected exactly one tab for composer-abc")
+        self.assertEqual(len(tabs), 1, f"Expected exactly one tab for {COMPOSER_ID}")
 
         tab = tabs[0]
-        self.assertEqual(tab["id"], "composer-abc")
-        self.assertEqual(tab["title"], "Test Chat")
-        self.assertIn("bubbles", tab)
-        self.assertIn("codeBlockDiffs", tab)
+        # GET /tabs and workspace.html ?tab= use bare composer id, not the KV key.
+        self.assertEqual(tab["id"], COMPOSER_ID, "tab id must be bare composer id (KV key suffix only)")
+        self.assertNotEqual(tab["id"], COMPOSER_KV_KEY, "tab id must not include composerData: prefix")
+        self.assertEqual(tab["title"], "Test Chat", "composer name from seeded cursorDiskKV row")
+        self.assertIn("bubbles", tab, "tab payload must include bubbles for the conversation view")
+        self.assertIn("codeBlockDiffs", tab, "tab payload must include codeBlockDiffs field (may be empty)")
 
         bubbles = tab["bubbles"]
         self.assertEqual(len(bubbles), 1, "Expected exactly one bubble (null row skipped)")
         bubble = bubbles[0]
-        self.assertEqual(bubble["type"], "user")
-        self.assertEqual(bubble["text"], "hello world")
-        self.assertIn("timestamp", bubble)
+        self.assertEqual(bubble["type"], "user", "header type 1 maps to user bubble")
+        self.assertEqual(bubble["text"], "hello world", "healthy bubble text must surface in the tab")
+        self.assertIn("timestamp", bubble, "bubble must carry a timestamp for ordering/display")
 
 
 if __name__ == "__main__":