Replace silent except Exception: pass with structured logging in workspace/bubble paths (closes #66)#76
Open
bradjin8 wants to merge 6 commits into
Open
Replace silent except Exception: pass with structured logging in workspace/bubble paths (closes #66)#76bradjin8 wants to merge 6 commits into
bradjin8 wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces silent exception handling and prints with module-level structured logging across APIs and services, adding warnings that include workspace/composer/bubble identifiers and exception details; tests assert WARNING logs for several parse-failure scenarios. ChangesParse failure logging refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@services/workspace_resolver.py`:
- Around line 337-345: The code currently calls json.loads(row["value"]) into cd
and then passes cd to _determine_project_for_conversation which assumes a dict;
guard against non-dict JSON by checking the type of cd after json.loads (e.g.,
isinstance(cd, dict)); if cd is not a dict, log a warning referencing cid and
the actual type/value and continue (so assemble_workspace_tabs/alias inference
won't crash), then only call _determine_project_for_conversation(cd, ...) when
cd is a dict.
In `@services/workspace_tabs.py`:
- Around line 127-131: The code currently swallows JSON decode failures inside
_try_loads_kv_value and only logs SchemaError later (e.g., in the Bubble parse
path that logs "Failed to parse Bubble from bubbleId:%s"), so malformed JSON is
silently skipped; fix by surfacing JSON decoding errors at the bubble/composer
call sites: replace the single call to _try_loads_kv_value(...) with an explicit
json.loads(...) wrapped in try/except JSONDecodeError (or change
_try_loads_kv_value to return the decode exception) and when a JSONDecodeError
occurs call _logger.warning including the context keys (bubbleId or
composerData), the raw value, and the exception, then skip processing; apply the
same pattern for the composerData parse site referenced in the comment so decode
errors are logged for both locations.
🪄 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: 4b02d469-09c7-4fb6-ba44-8400fa6a55b0
📒 Files selected for processing (8)
.github/workflows/tests.ymlapi/logs.pyapi/search.pyapi/workspaces.pyservices/workspace_listing.pyservices/workspace_resolver.pyservices/workspace_tabs.pytests/test_parse_failure_logging.py
Linux CI pip-compile now resolves click==8.4.1 within the flask bound; the lock still pinned 8.4.0, which failed the lockfile freshness job.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/workspace_resolver.py (1)
337-344:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace bare
Exceptionwith specific exception types.Line 338 catches bare
Exception, which violates the PR objective to remove allexcept Exception: passpatterns.json.loadscan raiseJSONDecodeError,TypeError, orValueError; catch these specific types instead.🔧 Proposed fix
try: cd = json.loads(row["value"]) - except Exception as e: + except (json.JSONDecodeError, TypeError, ValueError) as e: _logger.warning( "Failed to decode Composer from composerData:%s: %s", cid, e, ) continue🤖 Prompt for 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. In `@services/workspace_resolver.py` around lines 337 - 344, The current except block catches a bare Exception after calling json.loads(row["value"]); replace it with a specific exception tuple to only handle JSONDecodeError, TypeError, and ValueError (e.g., except (json.JSONDecodeError, TypeError, ValueError) as e) so only relevant decode/type errors are caught, and keep the existing _logger.warning(..., cid, e) and continue logic intact; ensure json.JSONDecodeError is referenced correctly (or imported from json.decoder) if your Python version requires it.
🤖 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 `@services/workspace_tabs.py`:
- Around line 117-126: The json.loads(row["value"]) call in
services/workspace_tabs.py can raise TypeError when row["value"] is None (NULL)
and ValueError for other invalid inputs, but the except only handles
json.JSONDecodeError; update the parsing logic around parsed =
json.loads(row["value"]) (the block that logs "Failed to decode Bubble...") to
also catch TypeError and ValueError (or alternatively guard with an explicit
None check before calling json.loads) and include the same warning/logging
behavior and continue, ensuring the failure mode matches the previous
_try_loads_kv_value behavior.
- Around line 195-204: The JSON parse for composer data can raise TypeError when
row["value"] is None; update the parsing in the try/except around
json.loads(row["value"]) (the block referencing composer_id and row["value"]) to
guard against None by either checking row["value"] is not None before calling
json.loads or by expanding the except to catch TypeError alongside
json.JSONDecodeError, and keep the existing _logger.warning and continue
behavior when parsing fails.
In `@tests/test_parse_failure_logging.py`:
- Line 140: The test unpacks a returned tuple from assemble_workspace_tabs into
payload and status but never uses payload, triggering Ruff RUF059; change the
unused variable to a conventional ignored name (e.g., _payload or _) in the test
calls to assemble_workspace_tabs (the invocations around payload, status =
assemble_workspace_tabs(...) at the two occurrences) so only the used status is
kept and the linter warning is resolved.
---
Outside diff comments:
In `@services/workspace_resolver.py`:
- Around line 337-344: The current except block catches a bare Exception after
calling json.loads(row["value"]); replace it with a specific exception tuple to
only handle JSONDecodeError, TypeError, and ValueError (e.g., except
(json.JSONDecodeError, TypeError, ValueError) as e) so only relevant decode/type
errors are caught, and keep the existing _logger.warning(..., cid, e) and
continue logic intact; ensure json.JSONDecodeError is referenced correctly (or
imported from json.decoder) if your Python version requires it.
🪄 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: 10504aeb-e19a-4d6e-8858-080ff950c843
📒 Files selected for processing (4)
services/workspace_resolver.pyservices/workspace_tabs.pytests/test_invalid_workspace_aliases.pytests/test_parse_failure_logging.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@services/workspace_tabs.py`:
- Around line 117-118: Replace the silent skip for rows with a None payload by
emitting a warning that includes identifying row metadata: instead of just "if
row['value'] is None: continue", call the module logger (e.g., logger.warning or
self.logger.warning) and include row.get("key") or other identifying fields and
the full row (or its id) in the message, then continue; apply the same change to
the second occurrence around the other None-check so malformed NULL KV rows are
logged for observability.
🪄 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: 4617c257-c8c7-410f-9589-1c61bffae090
📒 Files selected for processing (3)
services/workspace_tabs.pytests/test_parse_failure_logging.pytests/test_workspace_tabs_null_bubble.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/workspace_tabs.py (1)
123-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch non-JSONDecode failures when decoding Bubble/Composer KV values
Inservices/workspace_tabs.py, bothjson.loads(row["value"])sites (Bubble and Composer) only catchjson.JSONDecodeError;json.loadscan raiseTypeErrorwhen SQLite returns a non-JSON payload type (and that would escape, potentially breaking/tabs). Reuse_try_loads_kv_value(already catchesjson.JSONDecodeError/TypeError/ValueError) or broaden the exception handling.💡 Minimal hardening patch
- except json.JSONDecodeError as e: + except (json.JSONDecodeError, TypeError, ValueError) as e: _logger.warning( "Failed to decode Bubble from %s: %s (value: %r)", row["key"], e, row["value"], ) continue @@ - except json.JSONDecodeError as e: + except (json.JSONDecodeError, TypeError, ValueError) as e: _logger.warning( "Failed to decode Composer from composerData:%s: %s (value: %r)", composer_id, e, row["value"], ) continue🤖 Prompt for 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. In `@services/workspace_tabs.py` around lines 123 - 126, The JSON decoding in services/workspace_tabs.py currently only catches json.JSONDecodeError around json.loads(row["value"]); change this to use the existing _try_loads_kv_value helper (which already handles JSONDecodeError/TypeError/ValueError) or expand the except clause to also catch TypeError and ValueError so non-string SQLite payloads don't raise and break /tabs; update both Bubble and Composer json.loads sites and ensure the warning log still includes context (row key/id) as before.
🤖 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.
Outside diff comments:
In `@services/workspace_tabs.py`:
- Around line 123-126: The JSON decoding in services/workspace_tabs.py currently
only catches json.JSONDecodeError around json.loads(row["value"]); change this
to use the existing _try_loads_kv_value helper (which already handles
JSONDecodeError/TypeError/ValueError) or expand the except clause to also catch
TypeError and ValueError so non-string SQLite payloads don't raise and break
/tabs; update both Bubble and Composer json.loads sites and ensure the warning
log still includes context (row key/id) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f59ff990-e7bc-4b66-9406-ad43f7246a72
📒 Files selected for processing (2)
services/workspace_tabs.pytests/test_workspace_tabs_null_bubble.py
timon0305
requested changes
May 25, 2026
timon0305
approved these changes
May 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #66 . Replaces bare
except Exception: passin workspace, composer, and bubble iteration loops withlogging.warning()(orlogging.error()for global DB load failures) so schema/parse failures are observable instead of silently dropping records.workspace_listing,workspace_tabs,workspace_resolver— module loggers,Composer.from_dicton listing (aligned with tabs), warnings include model/key and exception messagesearch,logs,workspaces— same pattern on workspace.json reads, composer mapping, legacy search, and bubble row decodetests/test_parse_failure_logging.py— two pytestcaplogtests (listing composer drift, tabs bubble drift)Control flow unchanged: bad rows are still skipped; endpoints do not 500 on a single malformed record.
print()for schema drift inapi/search/api/composersis intentionally unchanged (follow-up).Eval / tracker
Addresses baseline Test 11 (silent except-pass in workspace/bubble loops) and §5.3 of cursor-chat-browser-python-eval. Partially improves Test 10 (parse failures now logged in services + search/logs paths; print-based drift messages deferred).
Test plan
python -m pytest tests/test_parse_failure_logging.py -vpython -m unittest tests.test_project_layouts_dict_shape tests.test_workspace_listing_sql_errors tests.test_workspace_tabs_malformed_nested tests.test_invalid_workspace_aliases -vSummary by CodeRabbit
Bug Fixes
Tests
Chores