Feat/search errors and ux#121
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:
📝 WalkthroughWalkthrough
ChangesSearch validation, index locking, and UI updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SearchAPI
participant SearchIndex
participant LiveScan
Client->>SearchAPI: GET /api/search?q=...&since_days=...
SearchAPI->>SearchAPI: validate q length, since_days
alt validation fails
SearchAPI-->>Client: 400 error code
end
SearchAPI->>SearchAPI: check projects directory accessibility
alt inaccessible
SearchAPI-->>Client: 503 SEARCH_PROJECTS_UNAVAILABLE
end
SearchAPI->>SearchIndex: _search_via_index(query, since_ms)
alt index locked, partial hits collected
SearchIndex-->>SearchAPI: ranked partial hits
SearchAPI-->>Client: 200 partial results
else index returns None
SearchAPI->>LiveScan: _search_live_scan(query, since_ms)
LiveScan-->>SearchAPI: hits
SearchAPI-->>Client: 200 hits
end
SearchAPI-->>Client: 500 INTERNAL_ERROR on unexpected exception
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
🧹 Nitpick comments (1)
app.py (1)
109-117: 🧹 Nitpick | 🔵 TrivialMultiple worker processes will each start their own background rebuild thread.
_background_startedis a per-process global, so under a multi-process server (e.g. gunicorn with several workers) every worker independently rebuilds/refreshes the same on-disk SQLite index, causing redundant work and increased lock contention on the shared index file.🤖 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 `@app.py` around lines 109 - 117, The background search index startup in app initialization is still happening in every worker process because `_background_started` only guards within a single process. Update `start_search_index_background` and/or its call site in `app.py` so only one process can launch the rebuild thread, using a cross-process lock or master-only startup check before calling `start_search_index_background(projects_dir, app.config["EXCLUSION_RULES"])`. Keep the existing `TESTING` guard and the exception logging, but ensure the background worker is not started redundantly by each gunicorn worker.
🤖 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 90-112: The exclusion path in _index_hit_excluded currently fails
open when get_cached_session(file_path) raises, which can bypass privacy
filtering. Update this function to match _search_live_scan behavior by treating
session-load failures as excluded/skip-on-error rather than returning False, and
keep the warning in place so the failure remains visible while the index search
stays fail-closed.
In `@app.py`:
- Around line 109-117: The search index startup in create_app is gated by
app.config["TESTING"] too late, so tests that set TESTING after app creation
still spawn the background thread. Add a testing parameter to create_app and use
it before the start_search_index_background branch, then update the call sites
such as tests/test_search.py::_seed_indexed_client to pass testing=True when
constructing the app. Make sure the existing app.config["TESTING"] behavior
still works for callers that already set it, but the startup check should rely
on the new early parameter so the background worker is never launched during
tests.
In `@utils/search_index.py`:
- Around line 635-637: In _search_via_index(), treat tokenless queries from
_fts_match_query(query_lower) as index-unavailable instead of a successful empty
result. Update query_index_hits() so that when fts_q is empty for inputs like
“!!!” or “===”, it returns query_ok=False and an empty response, allowing the
caller to fall back to _search_live_scan() rather than stopping at the index
path.
---
Nitpick comments:
In `@app.py`:
- Around line 109-117: The background search index startup in app initialization
is still happening in every worker process because `_background_started` only
guards within a single process. Update `start_search_index_background` and/or
its call site in `app.py` so only one process can launch the rebuild thread,
using a cross-process lock or master-only startup check before calling
`start_search_index_background(projects_dir, app.config["EXCLUSION_RULES"])`.
Keep the existing `TESTING` guard and the exception logging, but ensure the
background worker is not started redundantly by each gunicorn worker.
🪄 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: c2b92b37-5ada-4126-981d-c896d2428d61
📒 Files selected for processing (16)
api/search.pyapp.pybenchmarks/README.mdbenchmarks/baselines.jsondocs/api-reference.mdmodels/error_codes.pyscripts/check_benchmark_regression.pyscripts/reduce_baselines.pystatic/js/search.jsstatic/js/search.test.jstests/test_api_integration.pytests/test_error_codes.pytests/test_error_propagation.pytests/test_search.pytests/test_search_index.pyutils/search_index.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_api_routes.py (1)
42-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
TESTINGassignment.Line 43 (
app.config["TESTING"] = True) is now dead code sincetesting=Trueis already passed intocreate_app(...)on Line 42, which setsapp.config["TESTING"]internally.🧹 Proposed cleanup
app = create_app(base_dir=str(tmp_path), exclusion_rules_path=str(rules_path), testing=True) - app.config["TESTING"] = True excluded_client = app.test_client()🤖 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_api_routes.py` around lines 42 - 44, The TESTING config is being set twice in the test setup, making the explicit app.config["TESTING"] assignment redundant. In the test that calls create_app(..., testing=True) and then creates excluded_client, remove the extra TESTING assignment and keep the create_app testing flag as the single source of truth.
🤖 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.
Nitpick comments:
In `@tests/test_api_routes.py`:
- Around line 42-44: The TESTING config is being set twice in the test setup,
making the explicit app.config["TESTING"] assignment redundant. In the test that
calls create_app(..., testing=True) and then creates excluded_client, remove the
extra TESTING assignment and keep the create_app testing flag as the single
source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e046ba83-3e57-4b49-9d95-201955775449
📒 Files selected for processing (12)
.github/workflows/ci.ymlapi/search.pyapp.pydocs/api-reference.mdstatic/js/search.jsstatic/js/search.test.jstests/benchmarks/conftest.pytests/conftest.pytests/test_api_routes.pytests/test_search.pytests/test_search_index.pyutils/search_index.py
✅ Files skipped from review due to trivial changes (1)
- tests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (7)
- app.py
- tests/test_search_index.py
- static/js/search.test.js
- static/js/search.js
- api/search.py
- docs/api-reference.md
- utils/search_index.py
clean6378-max-it
force-pushed
the
feat/search-errors-and-ux
branch
from
a066ca4 to
781e1f7
Compare
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 `@static/js/search.js`:
- Around line 103-120: The failed-search handling in search.js is trying to read
the response body twice, so the text fallback never works after res.json()
consumes the stream. Update the error path in the search fetch block to preserve
a separate copy of the response body before parsing, and use that copy for the
raw-text fallback when JSON parsing fails. Keep the existing localRequestId,
res.ok, and container.innerHTML logic unchanged, but ensure the fallback can
surface non-JSON error text instead of always falling back to the generic status
message.
🪄 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: 64855c8d-8e80-423f-8315-ca1c02a2576b
📒 Files selected for processing (16)
.github/workflows/ci.ymlapi/search.pyapp.pydocs/api-reference.mdmodels/error_codes.pystatic/js/search.jsstatic/js/search.test.jstests/benchmarks/conftest.pytests/conftest.pytests/test_api_integration.pytests/test_api_routes.pytests/test_error_codes.pytests/test_error_propagation.pytests/test_search.pytests/test_search_index.pyutils/search_index.py
✅ Files skipped from review due to trivial changes (3)
- tests/benchmarks/conftest.py
- tests/test_api_routes.py
- docs/api-reference.md
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/conftest.py
- models/error_codes.py
- tests/test_error_codes.py
- .github/workflows/ci.yml
- tests/test_api_integration.py
- tests/test_search_index.py
- app.py
- api/search.py
- static/js/search.test.js
- utils/search_index.py
wpak-ai
left a comment
wpak-ai
left a comment
There was a problem hiding this comment.
Must fix
-
api/search.py_search_via_index(~L183) — stop breaking onsql_exhaustedwhile the fetched batch has unscanned rows; keep paging (or top-up via live-scan) so the endpoint returnslimitresults (exclusion-dropped hits on an "exhausted" batch abandon the unscanned tail → silent undercount, no fallback) -
utils/search_index.py_indexable_texts_from_entry/_coerce_role(~L344-360) — index the same roles live-scan searches (system/result/progress/unknown), or restrict live-scan to match (index-served queries silently miss matches live-scan finds; results depend on whether the index is warm) -
api/search.pysearch()+query_index_hits(G3, cross-file) — make the fallback detect index UNDER-RETURN, not just unavailability: when the index yields< max_resultsand isn't provably exhausted (or returns an empty non-Nonelist), fall back to / top-up with live-scan (fixes #1 and #8, mitigates #2, at the point where correctness is decided)
Should fix
-
api/search.py:87_message_searchable_text+utils/search_index.py:352_indexable_texts_from_entry(G1) — unify searchable-text extraction + role policy into one shared helper (two copies that must stay in lock-step already drifted — root cause of #2) -
utils/search_index.pyquery_index_hits+api/search.py_search_via_index(G4) — consolidate pagination-consumption accounting so the tail is guaranteed drained (mechanism of #1) -
tests/test_search_index.py— add a test with a usable index, matches >max_results, and ≥1 excluded match, assertingmax_resultsare still returned (nothing exercises #1; the phrase-pagination test passes on buggy and fixed code alike) -
tests/test_search_index.py— add a system/unknown-role index-vs-live parity test (no test covers #2)
Fresh Claude installs were getting 503 on search when ~/.claude/projects did not exist yet. 503 now means the path is there but listdir failed. CI smoke test uses create_app(testing=True). Search page nags on empty query and highlights snippets by code point.
clean6378-max-it
force-pushed
the
feat/search-errors-and-ux
branch
from
d8a411b to
96a4ce5
Compare
3 participants
Closes #113
Summary
Follow-up to #120. That PR added the FTS index and the 30-day search window. This one is the error codes and search page cleanup.
/api/searchused to return200 []on an empty query. It now returns 400 withSEARCH_EMPTY_QUERY. Same idea for a query over 500 chars, badsince_days, a projects dir you cannot read, and a locked FTS index (503 on the last one; no live-scan fallback when SQLite is locked).Search page: short note about the 30-day default and the 50-result cap, checkbox for all history, warning when you hit the limit, separate empty vs error UI, snippet match highlighting after escape.
Stacked on
feat/search-fts-indexuntil #120 lands, then rebase and point atmaster.Test plan
pytest (search, integration, error propagation, search_index), npm test, mypy, ruff all green locally.
Manual: empty
qand badsince_daysin curl,/searchcheckbox and truncation warning, check highlight in a snippet.Summary by CodeRabbit
New Features
Bug Fixes
Documentation