Skip to content

Add incomplete-result signaling on parse failure (#67)#78

Open
bradjin8 wants to merge 7 commits into
masterfrom
feat/add-incomplete-result-signaling
Open

Add incomplete-result signaling on parse failure (#67)#78
bradjin8 wants to merge 7 commits into
masterfrom
feat/add-incomplete-result-signaling

Conversation

@bradjin8
Copy link
Copy Markdown
Collaborator

@bradjin8 bradjin8 commented May 26, 2026

Summary

When composer or bubble rows fail JSON/schema parsing, the app previously returned a shorter list with no indication that data was dropped. This change adds structured warnings metadata on the primary browse/search APIs and shows a banner in the web UI when results may be incomplete.

  • Introduce ParseWarningCollector in models/parse_warnings.py to aggregate skipped composers and bubbles into API warnings (type: parse_error, count, detail).
  • Return (projects, warnings) from list_workspace_projects; attach warnings to tabs payloads in assemble_workspace_tabs when any parse failures occurred.
  • Expose warnings from GET /api/workspaces, GET /api/workspaces/<id>/tabs, and GET /api/search (global cursorDiskKV path).
  • Keep backward compatibility: GET /api/workspaces still returns a JSON array when there are no warnings; when warnings exist, the response is { projects: [...], warnings: [...] }.
  • Web UI: normalizeWorkspacesResponse(), showIncompleteResultsBanner(), and .alert-warning styling on Projects, Workspace, and Search pages.

Closes #67.

Test plan

  • pytest tests/test_parse_warnings.py — clean results (no warnings), parse failures (warnings present), service + API shapes
  • Full suite: pytest tests/ (315 passed locally)
  • Manual: corrupt a composerData: row in global storage → Projects page shows warning banner; workspace tabs/search show banner when applicable
  • Manual: healthy data → no banner; GET /api/workspaces returns a plain array

Notes

  • Warnings are opt-in for API clients; clients that ignore warnings behave as before.
  • Parse failures continue to be logged at WARNING (issue Replace except-pass with structured logging at parse sites #66 pattern); this PR adds user-visible signaling on top of that logging.
  • Follow-up (optional): wire load_bubble_map() skips in workspace listing into the collector so bubble-only failures on the project list also emit warnings.

Dependencies / lockfile

Base branch: feat/replace-print-with-logging (stacked after #66 / #68).

This PR does not change requirements-lock.txt. The click==8.4.08.4.1 pin (Flask transitive) lives in PR #76 as commit d55881f: Linux CI pip-compile now resolves click==8.4.1 within the Flask bound, and the lock freshness check failed while the file still pinned 8.4.0. That is a routine lock refresh for CI, not required for parse-warning behavior. Dependabot also tracks click separately (dependabot/pip/click-8.4.1).

Summary by CodeRabbit

  • New Features

    • Collect and return structured parse/process warnings with workspace and search results.
  • Bug Fixes

    • Consistent logging for export and PDF failures; API responses now return generic failure messages on server errors.
    • Workspace/tab listing continues past malformed entries and reports skips/failures.
  • UI/UX

    • Warning banners show on Projects, Search, and Workspace pages for incomplete results.
  • Tests

    • End-to-end tests added/updated to cover parse-warning reporting.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0e831a1b-6cc4-4eb4-9eed-ae40d23db68b

📥 Commits

Reviewing files that changed from the base of the PR and between bc79ce6 and 62b007c.

📒 Files selected for processing (6)
  • api/search.py
  • models/parse_warnings.py
  • services/workspace_listing.py
  • services/workspace_tabs.py
  • templates/index.html
  • templates/search.html

📝 Walkthrough

Walkthrough

Adds ParseWarningCollector and uses it in workspace listing/tabs/search to record skipped/failed parse events; services return (data, warnings) and APIs attach warnings to JSON. Replaces print/traceback error output with logging in export/pdf. Frontend normalizes responses and shows warning banners; tests cover behavior.

Changes

Structured Logging & Parse-Warning Signaling

Layer / File(s) Summary
Parse Warning Model & Package Export
models/parse_warnings.py, models/__init__.py
New ParseWarningCollector dataclass counts composer/bubble parse failures and processing failures; to_api_list()/attach_to() produce structured warning entries. Symbols re-exported from models.
Workspace Listing Service: Parse Warnings & Tuple Return
services/workspace_listing.py
list_workspace_projects initializes ParseWarningCollector, records composer-skip/processing-failure counts on JSON/schema/exception cases, and returns (projects, warnings_list) instead of only projects.
Workspace Tabs Service: Centralized KV decode & warnings
services/workspace_tabs.py
Centralized _loads_kv_value_logged and _kv_payload_log_meta to decode KV JSON with structured logging; records bubble/composer skips and composer-processing failures and returns responses wrapped with warnings.
API Response Integration: Workspaces & Search Warnings
api/workspaces.py, api/search.py
GET /api/workspaces unpacks (projects, warnings) and conditionally returns both; GET /api/workspaces/<id> reports workspace.json parse errors; GET /api/search initializes collector, records skips/processing failures while scanning KV rows, and returns parse_warnings.attach_to({"results": results}).
Error Reporting Refactor: Export & PDF to Logging
api/export_api.py, api/pdf.py
Replace ad-hoc print/traceback output with module _logger.error(..., exc_info=True) and return generic 500 JSON messages ("Export failed", "Failed to generate PDF").
Frontend: JavaScript Helpers & CSS Styling
static/js/app.js, static/css/style.css
Add normalizeWorkspacesResponse(), formatParseWarnings(), and showIncompleteResultsBanner() to normalize API responses and render warning banners; add --warning-* theme variables and .alert-warning CSS rule.
Frontend: Template DOM Hosts & Warning Display Calls
templates/index.html, templates/search.html, templates/workspace.html
Add div#parse-warnings-host mount points and call showIncompleteResultsBanner() during page init to surface parse/incomplete-result warnings.
Comprehensive Test Coverage: Parse Warnings & Logging
tests/test_parse_warnings.py, tests/test_parse_failure_logging.py, other tests updated
New tests seed clean and failure workspaces, validate collector outputs and service/API warning shapes; existing tests updated to unpack (projects, _warnings); parse-failure logging tests adjusted for status capture.

Sequence Diagram

sequenceDiagram
  participant Client
  participant API as Workspaces API
  participant Service as workspace_listing / workspace_tabs
  participant KV as cursorDiskKV
  participant Collector as ParseWarningCollector

  Client->>API: GET /api/workspaces
  API->>Service: list_workspace_projects(workspace_path)
  Service->>KV: read composerData / bubble rows
  KV-->>Service: rows (some invalid)
  Service->>Collector: record_bubble_skipped()/record_composer_skipped()/record_composer_processing_failure()
  Service-->>API: (projects, warnings)
  API->>Collector: attach_to({"projects": projects})
  API-->>Client: JSON { projects, warnings? }
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

  • #67: Add incomplete-result signaling on parse failure — this PR implements structured warnings and UI banners per that issue's objectives.

Possibly related PRs

Suggested reviewers

  • clean6378-max-it
  • timon0305
  • wpak-ai

Poem

🐰 I hopped through parse streams late at night,

Counting skips by moon and loglamp light.
Warnings bloom where stray JSON fell,
Banners whisper when results are not well.
Hooray — no more silent missing threads in sight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.85% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding incomplete-result signaling when JSON/schema parsing fails, matching the core objective.
Linked Issues check ✅ Passed The PR implements all coding requirements from #67: structured parse-failure warnings via ParseWarningCollector [#67], service-layer functions return warnings [#67], API responses include warnings field [#67], web UI displays banner on parse failure [#67], tests for clean and parse-failure cases [#67].
Out of Scope Changes check ✅ Passed All changes are scoped to #67 objectives: parse-warning infrastructure, service/API integration, UI banners, and logging improvements in exception handlers are all directly related to incomplete-result signaling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/add-incomplete-result-signaling

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
api/search.py (1)

160-177: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle non-string/NULL bubble payloads without aborting global search.

At Line 161, json.loads(row["value"]) can raise TypeError (e.g., NULL KV value). That exception is not handled in this block and can terminate the global search pass via the outer catch, dropping valid results. Please treat this as a skipped bubble and keep processing.

Proposed fix
-                        except (json.JSONDecodeError, ValueError):
-                            parse_warnings.record_bubble_skipped()
+                        except (json.JSONDecodeError, TypeError, ValueError) as e:
+                            _logger.warning(
+                                "Failed to decode bubble %s from cursorDiskKV: %s",
+                                bid,
+                                e,
+                            )
+                            parse_warnings.record_bubble_skipped()
🤖 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 `@api/search.py` around lines 160 - 177, The code currently calls
Bubble.from_dict(json.loads(row["value"]), ...) which can raise TypeError for
non-string/NULL payloads and that exception is not handled; update the exception
handling around Bubble.from_dict / json.loads (the try/except that populates
bubble_map) to also catch TypeError and treat it like the other decode errors:
call parse_warnings.record_bubble_skipped() and (optionally) emit a warning via
_logger.warning similar to the SchemaError branch so the global search pass
continues and valid bubbles are not dropped.
templates/search.html (1)

73-85: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clear previous parse-warning UI on new/failed searches.

Line 85 only executes on successful responses. If a later search fails, a prior warning banner can remain and misrepresent current results.

Proposed fix
 async function performSearch(query, type) {
   const loading = document.getElementById('loading');
   const countEl = document.getElementById('result-count');
   const container = document.getElementById('results-container');
   loading.style.display = 'flex';
   countEl.style.display = 'none';
   container.innerHTML = '';
+  showIncompleteResultsBanner('parse-warnings-host', []);
 
   try {
     const res = await fetch(`/api/search?q=${encodeURIComponent(query)}&type=${type}`);
     const data = await res.json();
     const results = data.results || [];
     showIncompleteResultsBanner('parse-warnings-host', data.warnings);
@@
   } catch (e) {
     loading.style.display = 'none';
+    showIncompleteResultsBanner('parse-warnings-host', []);
     container.innerHTML = '<p class="text-danger">Search failed.</p>';
   }
 }

Also applies to: 112-115

🤖 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 `@templates/search.html` around lines 73 - 85, The prior parse-warning banner
can persist across searches; update performSearch to clear the parse-warning UI
at the start of a new search and in the error path: after container.innerHTML =
'' call showIncompleteResultsBanner('parse-warnings-host', null) (or an empty
array) to remove any existing banner, and in the catch block ensure you also
call showIncompleteResultsBanner('parse-warnings-host', null) before
returning/throwing; apply the same change to the other occurrence that currently
only calls showIncompleteResultsBanner('parse-warnings-host', data.warnings')
(lines around the second usage) so failed requests never leave stale warnings
visible.
🧹 Nitpick comments (1)
tests/test_parse_warnings.py (1)

236-241: ⚡ Quick win

Assert both parse-warning categories explicitly.

This assertion block can pass when only one parse-failure path is reported. Strengthen it to verify both conversation- and message-level warnings are present.

Proposed assertion tightening
         self.assertIn("warnings", payload)
         types = {w["type"] for w in payload["warnings"]}
         self.assertEqual(types, {"parse_error"})
-        counts = {w["count"] for w in payload["warnings"]}
-        self.assertIn(1, counts)
-        self.assertTrue(any(w["count"] >= 1 for w in payload["warnings"]))
+        details = {w["detail"] for w in payload["warnings"]}
+        self.assertTrue(any("conversation" in d for d in details))
+        self.assertTrue(any("message" in d for d in details))
+        self.assertGreaterEqual(sum(w["count"] for w in payload["warnings"]), 2)
🤖 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 `@tests/test_parse_warnings.py` around lines 236 - 241, The current assertions
only check that parse_error warnings exist but can pass if only one warning
scope is reported; update the test around payload and payload["warnings"]
(variables types, counts, w) to explicitly assert both conversation- and
message-level parse warnings are present by adding two assertions that there is
at least one warning with the parse type and a scope indicator for conversation
and at least one with the parse type and a scope indicator for message (e.g.,
assert any(w["type"] == "parse_error" and w.get("scope") == "conversation" for w
in payload["warnings"]) and similarly for "message"), so the test fails unless
both categories are reported.
🤖 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 `@api/export_api.py`:
- Around line 218-225: The except block currently returns the raw exception
string to clients; change the response to a generic error message instead (e.g.,
{"error": "Export failed due to an internal error"}) while keeping the detailed
exception logged with _logger.error(..., exc_info=True); update the return
statement that uses jsonify to remove str(e) and return the generic message with
status 500 and ensure variable names (_logger, e, jsonify) and the same except
block around the export handler are used.

In `@api/pdf.py`:
- Around line 172-179: The exception handler in api/pdf.py currently returns the
raw exception text via jsonify(str(e)), exposing internals; retain the detailed
server-side logging using _logger.error(..., exc_info=True) but change the API
response to a generic error payload (e.g., jsonify({"error": "Failed to generate
PDF"}) , 500) so no raw exception message is returned; update the return in the
except block (around the _logger.error and jsonify calls) accordingly and ensure
exc_info remains true for logging.

In `@requirements-lock.txt`:
- Line 9: Update the PR to either document why the transitive dependency bump
click==8.4.1 (previously 8.4.0) is included with the parse-warning feature or
move the lockfile change into its own commit/PR; specifically, in the PR
description or changelog reference the requirements-lock.txt entry for click and
explain whether this bump was needed for a bugfix, compatibility, or tooling
change, and if it's unrelated to parse-warning, create a separate PR (or
separate commit) that only updates click in requirements-lock.txt to keep the
feature PR focused.

In `@static/js/app.js`:
- Around line 98-103: The returned normalized object may contain non-array
values for projects and warnings causing downstream failures; update the return
in static/js/app.js to coerce both fields to arrays (e.g., use
Array.isArray(body.projects) ? body.projects : [] and
Array.isArray(body.warnings) ? body.warnings : []) so projects.filter(...) and
similar operations are safe even if callers supply non-array values.
- Around line 118-124: In showIncompleteResultsBanner, the early return when
!warnings or !warnings.length skips removing a previously rendered banner;
always query for and remove any existing '.incomplete-results-banner' before
returning on clean responses. Locate the function showIncompleteResultsBanner
and the variables container, warnings, and existing, and move or add the removal
logic so existing.remove() is executed even when there are no warnings, then
return if there are no warnings.

In `@tests/test_parse_failure_logging.py`:
- Line 129: The unpacked return values from assemble_workspace_tabs are unused
and trigger lint warnings; rename them to prefixed-unused names (e.g., change
"payload, status = assemble_workspace_tabs(...)" to "_payload, _status =
assemble_workspace_tabs(...)" ) and apply the same change for the other
occurrence mentioned (line 159) so both unpacked variables are prefixed with an
underscore.
- Around line 64-66: The test writes a workspace.json with a hardcoded
"/tmp/proj" path; replace that literal with a path rooted in the temporary
workspace (use ws_dir) so the fixture is portable and passes static checks:
update the json.dump call to use a path built from ws_dir (e.g.,
os.path.join(ws_dir, "proj") or Path(ws_dir) / "proj") instead of "/tmp/proj"
and keep the sqlite3.connect to state.vscdb as-is; look for the workspace.json
write site and the surrounding ws_dir and state.vscdb references to modify.

---

Outside diff comments:
In `@api/search.py`:
- Around line 160-177: The code currently calls
Bubble.from_dict(json.loads(row["value"]), ...) which can raise TypeError for
non-string/NULL payloads and that exception is not handled; update the exception
handling around Bubble.from_dict / json.loads (the try/except that populates
bubble_map) to also catch TypeError and treat it like the other decode errors:
call parse_warnings.record_bubble_skipped() and (optionally) emit a warning via
_logger.warning similar to the SchemaError branch so the global search pass
continues and valid bubbles are not dropped.

In `@templates/search.html`:
- Around line 73-85: The prior parse-warning banner can persist across searches;
update performSearch to clear the parse-warning UI at the start of a new search
and in the error path: after container.innerHTML = '' call
showIncompleteResultsBanner('parse-warnings-host', null) (or an empty array) to
remove any existing banner, and in the catch block ensure you also call
showIncompleteResultsBanner('parse-warnings-host', null) before
returning/throwing; apply the same change to the other occurrence that currently
only calls showIncompleteResultsBanner('parse-warnings-host', data.warnings')
(lines around the second usage) so failed requests never leave stale warnings
visible.

---

Nitpick comments:
In `@tests/test_parse_warnings.py`:
- Around line 236-241: The current assertions only check that parse_error
warnings exist but can pass if only one warning scope is reported; update the
test around payload and payload["warnings"] (variables types, counts, w) to
explicitly assert both conversation- and message-level parse warnings are
present by adding two assertions that there is at least one warning with the
parse type and a scope indicator for conversation and at least one with the
parse type and a scope indicator for message (e.g., assert any(w["type"] ==
"parse_error" and w.get("scope") == "conversation" for w in payload["warnings"])
and similarly for "message"), so the test fails unless both categories are
reported.
🪄 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: d39eab03-6b12-4d4e-8cc2-871cd5c79433

📥 Commits

Reviewing files that changed from the base of the PR and between 731c1e1 and 33db2e0.

📒 Files selected for processing (31)
  • api/composers.py
  • api/config_api.py
  • api/export_api.py
  • api/logs.py
  • api/pdf.py
  • api/search.py
  • api/workspaces.py
  • app.py
  • models/__init__.py
  • models/parse_warnings.py
  • requirements-lock.txt
  • scripts/export.py
  • services/cli_tabs.py
  • services/workspace_listing.py
  • services/workspace_resolver.py
  • services/workspace_tabs.py
  • static/css/style.css
  • static/js/app.js
  • templates/index.html
  • templates/search.html
  • templates/workspace.html
  • tests/test_invalid_workspace_aliases.py
  • tests/test_models_wired_at_read_sites.py
  • tests/test_parse_failure_logging.py
  • tests/test_parse_warnings.py
  • tests/test_project_layouts_dict_shape.py
  • tests/test_workspace_listing_cli.py
  • tests/test_workspace_listing_sql_errors.py
  • tests/test_workspace_tabs_null_bubble.py
  • utils/cli_chat_reader.py
  • utils/path_helpers.py

Comment thread api/export_api.py Outdated
Comment thread api/pdf.py Outdated
Comment thread requirements-lock.txt
Comment thread static/js/app.js
Comment thread static/js/app.js
Comment thread tests/test_parse_failure_logging.py
Comment thread tests/test_parse_failure_logging.py Outdated
@bradjin8 bradjin8 changed the base branch from master to feat/replace-print-with-logging May 26, 2026 13:11
@bradjin8 bradjin8 changed the base branch from feat/replace-print-with-logging to master May 26, 2026 13:14
@bradjin8 bradjin8 self-assigned this May 26, 2026
@bradjin8 bradjin8 requested a review from clean6378-max-it May 26, 2026 14:42
@bradjin8 bradjin8 force-pushed the feat/add-incomplete-result-signaling branch from 1561ea0 to f7688d3 Compare May 26, 2026 16:08
@bradjin8 bradjin8 force-pushed the feat/add-incomplete-result-signaling branch from f7688d3 to bc79ce6 Compare May 26, 2026 16:11
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
templates/search.html (1)

73-85: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clear stale parse-warning banners on failed searches.

When a new search fails, the previous warning banner can remain visible because banner rendering/cleanup only runs on success. Clear it at request start (or in catch) to avoid stale state.

Proposed fix
 async function performSearch(query, type) {
   const loading = document.getElementById('loading');
   const countEl = document.getElementById('result-count');
   const container = document.getElementById('results-container');
   loading.style.display = 'flex';
   countEl.style.display = 'none';
   container.innerHTML = '';
+  showIncompleteResultsBanner('parse-warnings-host', []);
 
   try {
     const res = await fetch(`/api/search?q=${encodeURIComponent(query)}&type=${type}`);
     const data = await res.json();
     const results = data.results || [];
     showIncompleteResultsBanner('parse-warnings-host', data.warnings);
@@
   } catch (e) {
     loading.style.display = 'none';
+    showIncompleteResultsBanner('parse-warnings-host', []);
     container.innerHTML = '<p class="text-danger">Search failed.</p>';
   }
 }

Also applies to: 112-115

🤖 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 `@templates/search.html` around lines 73 - 85, In performSearch, stale
parse-warning banners can persist when a search fails; call the existing
cleanup/render function to clear warnings at the start of the request (before
fetch) and also in the catch block so any previous banner with id
'parse-warnings-host' is removed on error; update performSearch to invoke
showIncompleteResultsBanner('parse-warnings-host', []) or the banner-clearing
variant both immediately after setting up the UI and inside the catch handler
(same for the related code around the other search branch at the 112-115 area)
so warnings never persist after a failed request.
api/search.py (1)

160-177: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Catch TypeError when decoding bubble JSON

json.loads(row["value"]) in the bubble loop can raise TypeError for NULL/non-string DB values. The current handler only catches json.JSONDecodeError and ValueError, so the exception can propagate to the outer global-search except Exception and abort the whole global search pass. Add TypeError to the except tuple.

Proposed fix
-                        except (json.JSONDecodeError, ValueError):
+                        except (json.JSONDecodeError, TypeError, ValueError):
                             parse_warnings.record_bubble_skipped()
🤖 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 `@api/search.py` around lines 160 - 177, The JSON decode of DB row values in
the bubble loop can raise TypeError for NULL/non-string values, which isn't
currently caught and can abort the search; update the exception handling around
json.loads(row["value"]) in the block that constructs Bubble.from_dict to
include TypeError in the except tuple (i.e., change the except
(json.JSONDecodeError, ValueError): branch to except (json.JSONDecodeError,
ValueError, TypeError):) so that parse_warnings.record_bubble_skipped() is
called for these cases and schema drift logging via _logger.warning remains
unchanged for SchemaError from Bubble.from_dict.
🤖 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 `@api/search.py`:
- Around line 160-177: The JSON decode of DB row values in the bubble loop can
raise TypeError for NULL/non-string values, which isn't currently caught and can
abort the search; update the exception handling around json.loads(row["value"])
in the block that constructs Bubble.from_dict to include TypeError in the except
tuple (i.e., change the except (json.JSONDecodeError, ValueError): branch to
except (json.JSONDecodeError, ValueError, TypeError):) so that
parse_warnings.record_bubble_skipped() is called for these cases and schema
drift logging via _logger.warning remains unchanged for SchemaError from
Bubble.from_dict.

In `@templates/search.html`:
- Around line 73-85: In performSearch, stale parse-warning banners can persist
when a search fails; call the existing cleanup/render function to clear warnings
at the start of the request (before fetch) and also in the catch block so any
previous banner with id 'parse-warnings-host' is removed on error; update
performSearch to invoke showIncompleteResultsBanner('parse-warnings-host', [])
or the banner-clearing variant both immediately after setting up the UI and
inside the catch handler (same for the related code around the other search
branch at the 112-115 area) so warnings never persist after a failed request.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 11941e38-ea71-4437-b428-ff590b8d384e

📥 Commits

Reviewing files that changed from the base of the PR and between 1561ea0 and f7688d3.

📒 Files selected for processing (25)
  • api/composers.py
  • api/config_api.py
  • api/export_api.py
  • api/pdf.py
  • api/search.py
  • api/workspaces.py
  • app.py
  • models/__init__.py
  • models/parse_warnings.py
  • scripts/export.py
  • services/cli_tabs.py
  • services/workspace_listing.py
  • services/workspace_tabs.py
  • static/css/style.css
  • static/js/app.js
  • templates/index.html
  • templates/search.html
  • templates/workspace.html
  • tests/test_models_wired_at_read_sites.py
  • tests/test_parse_failure_logging.py
  • tests/test_parse_warnings.py
  • tests/test_project_layouts_dict_shape.py
  • tests/test_workspace_listing_cli.py
  • tests/test_workspace_listing_sql_errors.py
  • utils/cli_chat_reader.py
✅ Files skipped from review due to trivial changes (1)
  • api/config_api.py

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_tabs.py (1)

583-590: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't count generic composer-processing failures as parse errors.

Line 583 catches arbitrary downstream exceptions after the composer has already parsed successfully, but Line 589 still increments the parse_error warning counter. That can turn resolver/rendering bugs into “incomplete results” warnings and silently downgrade real service defects into partial 200 responses.

Suggested fix
             except Exception as e:
                 _logger.warning(
                     "Failed to process Composer from composerData:%s: %s",
                     composer_id,
                     e,
                 )
-                parse_warnings.record_composer_skipped()
+                # Only JSON/schema failures should contribute to parse_error warnings.
+                continue

If you want to keep surfacing these as degraded results, this should use a separate warning type instead of parse_error.

🤖 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 583 - 590, The catch block in the
composer processing code currently treats downstream exceptions as parse errors
by calling parse_warnings.record_composer_skipped(); change this so
downstream/runtime failures after successful parsing are NOT counted as
parse_error: either remove the parse_warnings.record_composer_skipped() call and
log the failure only, or replace it with a distinct warning API (e.g.,
parse_warnings.record_composer_processing_failure() or
parse_warnings.record_degraded_result()) so that Composer parsing
success/parse_error metrics remain accurate; update the catch in the composer
processing function that logs "Failed to process Composer from composerData" to
use the new/alternate warning instead.
🤖 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 `@api/search.py`:
- Around line 175-177: The bubble-decode except block that currently catches
(json.JSONDecodeError, ValueError) around json.loads(row["value"]) must also
catch TypeError so NULL DB values don't escape and abort the global-storage
cursor; update that except to include TypeError and call
parse_warnings.record_bubble_skipped() as before. Also add a consistent
WARNING-level log message when a bubble decode is skipped (use the same logger
used elsewhere in api/search.py) that includes the row id/context and the
exception message. Apply the same pattern to the composer decode path: catch
(json.JSONDecodeError, ValueError, TypeError), call
parse_warnings.record_composer_skipped(), and emit a WARNING log with
identifying context and the exception details so both decode paths count and log
skips consistently.

In `@templates/index.html`:
- Around line 40-43: Before calling projRes.json() and passing the result into
normalizeWorkspacesResponse, check the HTTP status on projRes (e.g., projRes.ok
or projRes.status) and handle non-2xx responses by aborting the normal flow
instead of normalizing/rendering; on non-ok, read the error payload (text() or
json()) if needed and propagate or call the existing failure UI handler so
normalizeWorkspacesResponse, showIncompleteResultsBanner('parse-warnings-host',
...) and renderProjects(...) are not executed for error responses.

In `@templates/search.html`:
- Line 85: The incomplete-results banner is only updated on successful search
responses (showIncompleteResultsBanner('parse-warnings-host', data.warnings)),
so stale banners persist after errors; update the search error/failure handler
to clear the banner by calling
showIncompleteResultsBanner('parse-warnings-host', []) (or an explicit hide
function if available) whenever the request fails, ensuring the
parse-warnings-host banner is cleared in the catch/failure branch of the search
response flow.

---

Outside diff comments:
In `@services/workspace_tabs.py`:
- Around line 583-590: The catch block in the composer processing code currently
treats downstream exceptions as parse errors by calling
parse_warnings.record_composer_skipped(); change this so downstream/runtime
failures after successful parsing are NOT counted as parse_error: either remove
the parse_warnings.record_composer_skipped() call and log the failure only, or
replace it with a distinct warning API (e.g.,
parse_warnings.record_composer_processing_failure() or
parse_warnings.record_degraded_result()) so that Composer parsing
success/parse_error metrics remain accurate; update the catch in the composer
processing function that logs "Failed to process Composer from composerData" to
use the new/alternate warning instead.
🪄 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: 41e0c77f-62de-4acf-8afb-680ff47b7c7b

📥 Commits

Reviewing files that changed from the base of the PR and between f7688d3 and bc79ce6.

📒 Files selected for processing (18)
  • api/export_api.py
  • api/pdf.py
  • api/search.py
  • api/workspaces.py
  • models/__init__.py
  • models/parse_warnings.py
  • services/workspace_listing.py
  • services/workspace_tabs.py
  • static/css/style.css
  • static/js/app.js
  • templates/index.html
  • templates/search.html
  • templates/workspace.html
  • tests/test_parse_failure_logging.py
  • tests/test_parse_warnings.py
  • tests/test_project_layouts_dict_shape.py
  • tests/test_workspace_listing_cli.py
  • tests/test_workspace_listing_sql_errors.py
✅ Files skipped from review due to trivial changes (1)
  • tests/test_workspace_listing_cli.py

Comment thread api/search.py Outdated
Comment thread templates/index.html
Comment thread templates/search.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add incomplete-result signaling on parse failure

1 participant