Feat/search errors and ux#121
Open
clean6378-max-it wants to merge 7 commits into
Open
Conversation
Clamp oversized since_days instead of dropping them back to the 30-day default. Keep paging FTS hits when exclusion rules filter out the first batch so limit still fills. Skip session files that vanish during the fingerprint walk, drop dead index path constants, and print the real regression limit in the benchmark gate. Refresh test_search_full_corpus baseline for the indexed search path.
|
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 (6)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a local FTS5 search index with live-scan fallback, new ChangesSearch index, API, and UI overhaul
Benchmark gating update
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 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
timon0305
approved these changes
Jul 7, 2026
…h_index.py Added ORDER BY m.timestamp_ms DESC so index hits are newest-first Replaced the 20× over-fetch with a fixed _FTS_BATCH_SIZE = 200; _search_via_index keeps paging until results are filled or FTS is exhausted Fixed offset: sql_rows_fetched is now rows actually scanned (not full batch size), so early break no longer skips unprocessed rows api/search.py Added _rank_search_hits() for timestamp-desc ordering Index and live-scan paths both rank before returning (live scan collects all matches, then sorts and slices — correct global newest-first on fallback) Tests test_results_ordered_newest_first — verifies DESC ordering test_phrase_filter_paginates_past_decoy_token_matches — 250 token-only decoys + real phrase at end; would fail with the old offset bug 30 tests passed (ruff + mypy clean). Suggested commit message: fix: FTS search ordering and pagination for phrase filtering Add ORDER BY timestamp_ms DESC, advance SQL offset by rows scanned, and page in fixed batches until enough hits or FTS exhausted. Rank live-scan results the same way for parity. Want me to commit and push to PR #120? /humanizer commit message fix: FTS search ordering and pagination
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
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
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.
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
/api/searchinput validation with dedicated error codes; improved behavior when projects/index are unavailable (including 503 handling) and when the index is locked (safe fallback)./api/searchAPI reference with new parameters and error-code catalog.