Fix: Skip NULL bubble rows in workspace tabs loader (closes #50)#52
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a safe JSON loader for ChangesNull Bubble Value Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_workspace_tabs_null_bubble.py`:
- Line 56: The test unpacks the return of assemble_workspace_tabs into a
variable named payload that is never used; rename that variable to _payload (or
_) in the test call to assemble_workspace_tabs to satisfy the linter and clarify
intent, keeping the existing assignment to status unchanged.
- Around line 66-78: Update the
test_healthy_bubbles_still_load_when_null_row_present test to assert not only
that payload contains "tabs" but that assemble_workspace_tabs actually surfaces
the expected tab and bubble data: call assemble_workspace_tabs (as already
done), then locate the specific tab object within payload["tabs"] (by title or
id) and assert it contains the expected bubble entry with minimal composerData
(e.g., check bubble keys like "id", "type"/"title" and that "composerData" is
present and matches the minimal expected structure). Use the test function name
test_healthy_bubbles_still_load_when_null_row_present and the
assemble_workspace_tabs return value to guide your assertions so the test fails
if the valid bubble is omitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 076c242e-bf7c-4316-afcf-d8a8642a3d37
📒 Files selected for processing (2)
services/workspace_tabs.pytests/test_workspace_tabs_null_bubble.py
Closes #50
Problem
GET /api/workspaces/<id>/tabsreturned a 500 for any workspace that had acursorDiskKVrow with aNULLvalue column. The bubble-loading loop inservices/workspace_tabs.pycalledjson.loads(row["value"])without aNoneguard, andjson.loads(None)raisesTypeError. The existingexcept (json.JSONDecodeError, ValueError)clause did not catchTypeError, so the exception propagated all the way up and the project detail view was completely inaccessible.Root Cause
Fix
services/workspace_tabs.py— one guard added beforejson.loads:NULL rows are silently skipped; all other rows (valid, malformed JSON, schema drift) continue through their existing handlers unchanged.
Tests
New file
tests/test_workspace_tabs_null_bubble.pyusing a real SQLite temp DB:test_null_bubble_row_is_skipped_without_exception— workspace containing a NULL-value bubble row returns 200 with noTypeErrortest_healthy_bubbles_still_load_when_null_row_present— valid bubble rows alongside the NULL row are not droppedVerification
Summary by CodeRabbit
Bug Fixes
Tests