Search UX tooltip and distinct /api/search error codes (#117)#126
Search UX tooltip and distinct /api/search error codes (#117)#126clean6378-max-it wants to merge 2 commits into
Conversation
Add a search-window info tooltip, return structured 400/404/503/500 responses with machine-readable codes, validate query length and workspace hash, and show API error messages in the search UI.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRestructures Search API hardening and UI tooltip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
🤖 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 57-66: The _workspace_exists helper currently joins
client-controlled workspace_id values directly into os.path.isdir, which can be
abused with absolute paths or .. segments. Update _workspace_exists to
explicitly allow only known-safe workspace IDs before joining paths: keep the
existing "global" and "cli:" handling, and for regular IDs validate they match
the expected workspace hash/ID format before calling
os.path.join(workspace_path, workspace_id). Reject or return false for anything
outside that allowed pattern so workspace existence checks cannot be used as a
local path oracle.
- Around line 107-112: Move workspace discovery into the existing
structured-error guard in the search flow so failures are converted to the JSON
error contract. Specifically, in the request handling around workspace_filter,
resolve_workspace_path should be called inside the try block (the same guard
used by the search endpoint), and any discovery/storage exceptions should be
caught and routed through _search_error instead of escaping before the handler
can format a 500/503 response.
In `@templates/search.html`:
- Around line 17-24: The search help tooltip is not exposed to assistive
technologies because the trigger in search.html uses a focusable span with only
aria-label, while the guidance text is just visual content. Update the tooltip
trigger to a semantic control such as a button, and connect it to the help text
using aria-describedby with the tooltip content marked appropriately (for
example, role="tooltip") so screen readers can access the 30-day/full-history
guidance. Keep the existing search-info-tooltip, search-info-tooltip-text, and
search-info-icon structure as the anchor points while changing the accessibility
semantics.
🪄 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: fdfda060-7323-48ea-8cff-b4a8c4d5fc20
📒 Files selected for processing (4)
api/search.pystatic/css/style.csstemplates/search.htmltests/test_api_search.py
bradjin8
left a comment
bradjin8
left a comment
There was a problem hiding this comment.
Please check the following issues.
[Optional] templates/search.html line 123-126:
result-count banner could mention undated chats when windowed ("last 30 days; undated chats included"). Tooltip covers discoverability; banner would reinforce after search.
| <button | ||
| type="button" | ||
| class="search-info-tooltip" | ||
| id="search-window-help" | ||
| aria-describedby="search-window-help-text" | ||
| > |
There was a problem hiding this comment.
Second commit dropped aria-label="Search window help" when switching span → button. aria-describedby alone may not give the control an accessible name in all AT. Restore aria-label on the button.
| query = request.args.get("q", "").strip() | ||
| if not query: | ||
| return _search_error("No search query provided", "empty_query", 400) | ||
| if len(query) > _MAX_SEARCH_QUERY_LEN: | ||
| return _search_error("Search query is too long", "query_too_long", 400) | ||
|
|
||
| search_type = request.args.get("type", "all") | ||
| if search_type not in _VALID_SEARCH_TYPES: |
There was a problem hiding this comment.
test_workspace_path_resolution_failure_returns_structured_500 documents OSError → 500. Issue #117 suggested 404/503 for storage unavailability — consider 503 + storage_unavailable if you want clients to retry config/discovery failures differently from bugs. Current choice is defensible; note in PR description.
| except sqlite3.OperationalError: | ||
| _logger.exception("Search index unavailable") | ||
| return _search_error( | ||
| "Search index is temporarily unavailable", | ||
| "search_index_unavailable", | ||
| 503, | ||
| ) | ||
| except Exception: | ||
| _logger.exception("Search failed") | ||
| return json_response({"error": "Search failed", "results": []}, 500) No newline at end of file | ||
| return _search_error("Search failed", "internal_error", 500) |
There was a problem hiding this comment.
Only sqlite3.OperationalError maps to 503. Other DB errors (e.g. sqlite3.DatabaseError) fall through to 500. Fine for now; widen only if search layer raises them in practice.
| @@ -101,18 +180,30 @@ def search() -> tuple[Response, int] | Response: | |||
| ) | |||
| ) | |||
|
|
|||
| ranked = rank_results(results) | |||
| if workspace_filter: | |||
| ranked = _filter_results_by_workspace(ranked, workspace_filter) | |||
There was a problem hiding this comment.
workspace query param + post-filter is useful but outside #117 scope (API-only; UI does not expose it). Call out in PR as bonus scope or split to a follow-up if you want strict issue alignment.
| if (!res.ok) { | ||
| showIncompleteResultsBanner('parse-warnings-host', []); | ||
| loading.style.display = 'none'; | ||
| container.innerHTML = '<p class="text-danger">Search failed.</p>'; | ||
| const message = typeof data.error === 'string' ? data.error : 'Search failed.'; | ||
| container.innerHTML = `<p class="text-danger">${escapeHtml(message)}</p>`; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Frontend shows data.error on failure — good. Consider surfacing data.code in a data-* attribute or dev-only suffix for operators debugging 503 vs 500.
| """ | ||
| query = request.args.get("q", "").strip() | ||
| if not query: | ||
| return _search_error("No search query provided", "empty_query", 400) |
There was a problem hiding this comment.
Old 400/500 bodies included no code / 500 included "results": []. New contract is cleaner; note breaking change for any external client that expected results on error.
2 participants
Summary
templates/search.htmlexplaining the default 30-day indexed window, undated-chat inclusion, and the all-history checkbox.except Exceptionhandler inapi/search.pywith narrowed handlers returning structured{"error", "code"}bodies for 400, 404, 503, and 500.errorfield from failed API responses.workspacehash validation (404 when unknown).Problem
Part A: Users did not understand why some search results were missing (30-day window, undated chats always included).
Part B:
/api/searchreturned generic 500 for all failures, preventing clients from distinguishing malformed input, missing workspace, index lock, and internal errors.Test plan
pytest tests/test_api_search.py -q(19 passed)pytest -q(533 passed, 21 skipped)mypy api/search.pysqlite3.OperationalErrorinternal_errorcodeTestSearchWindow/ happy-path behaviorOut of scope
Closes #117
Summary by CodeRabbit
New Features
Bug Fixes