Skip to content

Commit 5d17c08

Browse files
authored
Fix: Skip NULL bubble rows in workspace tabs loader (closes #50) (#52)
* fix: added none check for row["value"] to continue without crashing * fix: review comments * fix: review comments * fix: _loads_disk_kv_value to _try_loads_kv_value
1 parent bbaf171 commit 5d17c08

2 files changed

Lines changed: 142 additions & 21 deletions

File tree

services/workspace_tabs.py

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,16 @@ def _extract_chat_id_from_code_block_diff_key(key: str) -> str | None:
3737
return m.group(1) if m else None
3838

3939

40+
def _try_loads_kv_value(raw: str | None) -> Any | None:
41+
"""Parse a cursorDiskKV ``value`` column; ``None`` on missing or unparseable input (no raise)."""
42+
if raw is None:
43+
return None
44+
try:
45+
return json.loads(raw)
46+
except (json.JSONDecodeError, TypeError, ValueError):
47+
return None
48+
49+
4050
def assemble_workspace_tabs(
4151
workspace_id: str,
4252
workspace_path: str,
@@ -97,30 +107,30 @@ def _safe_fetchall(query: str, params: tuple = ()) -> list:
97107
parts = row["key"].split(":")
98108
if len(parts) >= 3:
99109
bid = parts[2]
110+
parsed = _try_loads_kv_value(row["value"])
111+
if parsed is None:
112+
continue
100113
try:
101-
bubble_obj = Bubble.from_dict(json.loads(row["value"]), bubble_id=bid)
114+
bubble_obj = Bubble.from_dict(parsed, bubble_id=bid)
102115
bubble_map[bid] = bubble_obj.raw
103116
except SchemaError as e:
104117
# Drift logged so the operator can chase disappearing
105118
# bubbles instead of guessing. Bad row still skipped so the
106119
# tabs endpoint can't 500 on one malformed bubble.
107120
print(f"Schema drift in bubble {bid}: {e}")
108-
except (json.JSONDecodeError, ValueError):
109-
pass
110121

111122
# Load codeBlockDiffs
112123
for row in _safe_fetchall("SELECT key, value FROM cursorDiskKV WHERE key LIKE 'codeBlockDiff:%'"):
113124
chat_id = _extract_chat_id_from_code_block_diff_key(row["key"])
114125
if not chat_id:
115126
continue
116-
try:
117-
d = json.loads(row["value"])
118-
code_block_diff_map.setdefault(chat_id, []).append({
119-
**d,
120-
"diffId": row["key"].split(":")[2] if len(row["key"].split(":")) > 2 else None,
121-
})
122-
except Exception:
123-
pass
127+
d = _try_loads_kv_value(row["value"])
128+
if not isinstance(d, dict):
129+
continue
130+
code_block_diff_map.setdefault(chat_id, []).append({
131+
**d,
132+
"diffId": row["key"].split(":")[2] if len(row["key"].split(":")) > 2 else None,
133+
})
124134

125135
# Load messageRequestContext rows once; build both
126136
# message_request_context_map and project_layouts_map from the same pass.
@@ -130,10 +140,7 @@ def _safe_fetchall(query: str, params: tuple = ()) -> list:
130140
if len(parts) < 2:
131141
continue
132142
chat_id = parts[1]
133-
try:
134-
ctx = json.loads(row["value"])
135-
except Exception:
136-
continue
143+
ctx = _try_loads_kv_value(row["value"])
137144
if not isinstance(ctx, dict):
138145
continue
139146

@@ -151,9 +158,8 @@ def _safe_fetchall(query: str, params: tuple = ()) -> list:
151158
project_layouts_map.setdefault(chat_id, [])
152159
for layout in layouts:
153160
if isinstance(layout, str):
154-
try:
155-
layout = json.loads(layout)
156-
except Exception:
161+
layout = _try_loads_kv_value(layout)
162+
if not isinstance(layout, dict):
157163
continue
158164
if isinstance(layout, dict) and layout.get("rootPath"):
159165
project_layouts_map[chat_id].append(layout["rootPath"])
@@ -178,16 +184,17 @@ def _safe_fetchall(query: str, params: tuple = ()) -> list:
178184

179185
for row in composer_rows:
180186
composer_id = row["key"].split(":")[1]
187+
parsed = _try_loads_kv_value(row["value"])
188+
if parsed is None:
189+
continue
181190
try:
182-
composer = Composer.from_dict(json.loads(row["value"]), composer_id=composer_id)
191+
composer = Composer.from_dict(parsed, composer_id=composer_id)
183192
except SchemaError as e:
184193
# Drift skipped + logged so the two primary conversation
185194
# paths (list_workspaces + get_workspace_tabs) agree on what
186195
# counts as a valid composer.
187196
print(f"Schema drift in composer {composer_id}: {e}")
188197
continue
189-
except (json.JSONDecodeError, TypeError, ValueError):
190-
continue
191198
try:
192199
cd = composer.raw
193200

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
"""Regression test for issue #50: NULL bubble value crashes GET /tabs.
2+
3+
A cursorDiskKV row with a NULL value column previously caused
4+
json.loads(None) -> TypeError, which propagated as a 500 response.
5+
The fix uses ``_try_loads_kv_value`` in ``services/workspace_tabs.py`` so
6+
NULL / unparseable cursorDiskKV values are skipped without raising.
7+
"""
8+
9+
import json
10+
import os
11+
import sqlite3
12+
import tempfile
13+
import unittest
14+
15+
from services.workspace_tabs import assemble_workspace_tabs
16+
17+
# cursorDiskKV keys use typed prefixes; tabs[].id is the bare suffix only
18+
# (assemble_workspace_tabs: composer_id = row["key"].split(":")[1]).
19+
COMPOSER_ID = "composer-abc"
20+
COMPOSER_KV_KEY = f"composerData:{COMPOSER_ID}"
21+
22+
23+
class TestNullBubbleValueDoesNotCrashTabs(unittest.TestCase):
24+
def setUp(self):
25+
self.tmp = tempfile.TemporaryDirectory()
26+
base = self.tmp.name
27+
28+
# Minimal workspaceStorage layout expected by assemble_workspace_tabs.
29+
ws_dir = os.path.join(base, "workspaceStorage")
30+
os.makedirs(ws_dir)
31+
32+
# Global storage with a cursorDiskKV table containing a NULL-value bubble row.
33+
global_dir = os.path.join(base, "globalStorage")
34+
os.makedirs(global_dir)
35+
db_path = os.path.join(global_dir, "state.vscdb")
36+
conn = sqlite3.connect(db_path)
37+
conn.execute("CREATE TABLE cursorDiskKV ([key] TEXT PRIMARY KEY, value TEXT)")
38+
conn.execute(
39+
"INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
40+
(f"bubbleId:{COMPOSER_ID}:bubble-null", None), # NULL value — the crash case
41+
)
42+
# Healthy bubble that should surface in the assembled tab.
43+
conn.execute(
44+
"INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
45+
(
46+
f"bubbleId:{COMPOSER_ID}:bubble-ok",
47+
json.dumps({"type": 1, "text": "hello world", "createdAt": 1739200000000}),
48+
),
49+
)
50+
# Composer referencing the healthy bubble — required for a tab to be built.
51+
conn.execute(
52+
"INSERT INTO cursorDiskKV ([key], value) VALUES (?, ?)",
53+
(
54+
COMPOSER_KV_KEY,
55+
json.dumps({
56+
"name": "Test Chat",
57+
"modelConfig": {"modelName": "gpt-4o"},
58+
"fullConversationHeadersOnly": [{"bubbleId": "bubble-ok", "type": 1}],
59+
"lastUpdatedAt": 1739300000000,
60+
"createdAt": 1739200000000,
61+
}),
62+
),
63+
)
64+
conn.commit()
65+
conn.close()
66+
67+
self.workspace_path = ws_dir
68+
69+
def tearDown(self):
70+
self.tmp.cleanup()
71+
72+
def test_null_bubble_row_is_skipped_without_exception(self):
73+
"""assemble_workspace_tabs must not raise when a bubble row has NULL value."""
74+
try:
75+
_payload, status = assemble_workspace_tabs(
76+
workspace_id="global",
77+
workspace_path=self.workspace_path,
78+
rules=[],
79+
)
80+
except TypeError as exc:
81+
self.fail(f"NULL bubble row raised TypeError: {exc}")
82+
83+
self.assertEqual(status, 200, "NULL bubble row must not turn tabs load into an error response")
84+
85+
def test_healthy_bubbles_still_load_when_null_row_present(self):
86+
"""The healthy bubble surfaces in a tab even when a NULL row is present."""
87+
payload, status = assemble_workspace_tabs(
88+
workspace_id="global",
89+
workspace_path=self.workspace_path,
90+
rules=[],
91+
)
92+
self.assertEqual(status, 200, "tabs endpoint must succeed when only the null bubble row is bad")
93+
self.assertIsInstance(payload, dict, "tabs response must be a JSON object envelope")
94+
tabs = payload.get("tabs", [])
95+
self.assertEqual(len(tabs), 1, f"Expected exactly one tab for {COMPOSER_ID}")
96+
97+
tab = tabs[0]
98+
# GET /tabs and workspace.html ?tab= use bare composer id, not the KV key.
99+
self.assertEqual(tab["id"], COMPOSER_ID, "tab id must be bare composer id (KV key suffix only)")
100+
self.assertNotEqual(tab["id"], COMPOSER_KV_KEY, "tab id must not include composerData: prefix")
101+
self.assertEqual(tab["title"], "Test Chat", "composer name from seeded cursorDiskKV row")
102+
self.assertIn("bubbles", tab, "tab payload must include bubbles for the conversation view")
103+
self.assertIn("codeBlockDiffs", tab, "tab payload must include codeBlockDiffs field (may be empty)")
104+
105+
bubbles = tab["bubbles"]
106+
self.assertEqual(len(bubbles), 1, "Expected exactly one bubble (null row skipped)")
107+
bubble = bubbles[0]
108+
self.assertEqual(bubble["type"], "user", "header type 1 maps to user bubble")
109+
self.assertEqual(bubble["text"], "hello world", "healthy bubble text must surface in the tab")
110+
self.assertIn("timestamp", bubble, "bubble must carry a timestamp for ordering/display")
111+
112+
113+
if __name__ == "__main__":
114+
unittest.main()

0 commit comments

Comments
 (0)