feat: FTS5 search index with background rebuild and 30-day default wi…#120
Open
clean6378-max-it wants to merge 4 commits into
Open
feat: FTS5 search index with background rebuild and 30-day default wi…#120clean6378-max-it wants to merge 4 commits into
clean6378-max-it wants to merge 4 commits into
Conversation
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a local FTS5 search index, switches ChangesSearch Index Feature
Benchmark Gate Update
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant api_search
participant utils_search_index
participant SessionCache
Client->>api_search: GET /api/search?q=...&all_history/since_days
api_search->>utils_search_index: resolve_search_since_ms()
api_search->>utils_search_index: _search_via_index()
alt index available
utils_search_index-->>api_search: indexed hits
else index unavailable
api_search->>SessionCache: get_cached_session()
SessionCache-->>api_search: session data
api_search->>api_search: live scan + snippets
end
api_search-->>Client: SearchHitDict list
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check_benchmark_regression.py (1)
174-175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRegression summary message uses the wrong threshold for relaxed benchmarks.
limitis correctly computed per-benchmark (relaxedSEARCH_BENCHMARK_THRESHOLDforRELAXED_THRESHOLD_TESTS), but the final summary at Line 175 always reports the genericthreshold, even when a failure was actually gated against the relaxed 2.0x limit. This mislabels the reported percentage in CI logs (e.g., a search regression at >200% would be reported as "exceeded 20%").🩹 Proposed fix
if failures: - print(f"\nREGRESSION: {len(failures)} benchmark(s) exceeded {threshold:.0%}") + print(f"\nREGRESSION: {len(failures)} benchmark(s) exceeded their threshold")🤖 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 `@scripts/check_benchmark_regression.py` around lines 174 - 175, The regression summary in check_benchmark_regression.py is reporting the generic threshold even when a benchmark was checked against a relaxed per-test limit. Update the final failures summary printed in the benchmark regression flow to use the actual limit that applied to the failing benchmarks, using the existing benchmark selection logic around RELAXED_THRESHOLD_TESTS and the per-benchmark limit calculation instead of the global threshold. Ensure the message reflects the same threshold used when deciding failures.
🧹 Nitpick comments (1)
utils/search_index.py (1)
87-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused search-index path constants.
SEARCH_INDEX_POINTER_FILEandSEARCH_INDEX_FILEare never read; the paths are resolved throughcache_dir()in the code paths that matter. The current “test override hook” comment is misleading because patching these names has no runtime effect.🤖 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 `@utils/search_index.py` around lines 87 - 88, The unused search-index path constants should be removed from search_index.py because SEARCH_INDEX_POINTER_FILE and SEARCH_INDEX_FILE are never referenced by the runtime code paths. Update the module by deleting those constant definitions and any misleading “test override hook” comment, and keep the actual path resolution centralized in cache_dir()-based logic used by the search index helpers.
🤖 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 50-59: The _parse_since_days helper is dropping oversized values,
which later causes resolve_search_since_ms to fall back to the default 30-day
window instead of honoring a larger request. Update _parse_since_days to clamp
inputs above _MAX_SEARCH_SINCE_DAYS to that maximum rather than returning None,
while still rejecting non-positive and invalid values. Use the existing
_parse_since_days and resolve_search_since_ms flow to keep the behavior
consistent.
- Around line 108-138: The indexed search path in _search_via_index is filtering
too late, so excluded hits can consume the capped query_index_hits() results and
return fewer than max_results. Move the _index_hit_excluded check into the
indexed fetch/backfill loop so query_index_hits() can keep pulling additional
hits until enough non-excluded SearchHitDict entries are collected, matching the
live scan behavior. Keep the change localized around query_index_hits(),
_index_hit_excluded(), and the results assembly in _search_via_index.
In `@utils/search_index.py`:
- Around line 219-232: The `_projects_fingerprint()` helper should ignore
sessions that disappear between `list_sessions()` and `os.stat()`, since the
uncaught `OSError` can break the `/api/search` index check and prevent fallback
search. Update `_projects_fingerprint()` to catch `OSError` around the
`sess["path"]` stat/relpath handling, skip that session when it vanishes, and
continue building the manifest normally.
---
Outside diff comments:
In `@scripts/check_benchmark_regression.py`:
- Around line 174-175: The regression summary in check_benchmark_regression.py
is reporting the generic threshold even when a benchmark was checked against a
relaxed per-test limit. Update the final failures summary printed in the
benchmark regression flow to use the actual limit that applied to the failing
benchmarks, using the existing benchmark selection logic around
RELAXED_THRESHOLD_TESTS and the per-benchmark limit calculation instead of the
global threshold. Ensure the message reflects the same threshold used when
deciding failures.
---
Nitpick comments:
In `@utils/search_index.py`:
- Around line 87-88: The unused search-index path constants should be removed
from search_index.py because SEARCH_INDEX_POINTER_FILE and SEARCH_INDEX_FILE are
never referenced by the runtime code paths. Update the module by deleting those
constant definitions and any misleading “test override hook” comment, and keep
the actual path resolution centralized in cache_dir()-based logic used by the
search index helpers.
🪄 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: d7ffadc8-b00a-4240-b3ed-f3ea45c2d8bb
📒 Files selected for processing (10)
api/search.pyapp.pybenchmarks/README.mdbenchmarks/baselines.jsonscripts/check_benchmark_regression.pyscripts/reduce_baselines.pytests/test_api_integration.pytests/test_search.pytests/test_search_index.pyutils/search_index.py
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.
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
6 tasks
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 #112
Summary
This PR adds the FTS search index for Monday item first PR
utils/search_index.pybuilds a SQLite FTS5 index next to the PR #111 summary cache (~/.claude-code-chat-browser/). Rebuilds are keyed off project manifest mtime/size plus exclusion-rules fingerprint. Index files use the cppa pattern:search_index.<uuid>.sqliteandsearch_index.active./api/searchchecks the index first and falls back to live-scan when the index is missing, stale, or the FTS query fails. Default search window is 30 days;all_history=1andsince_days=Nopt into wider ranges. Background rebuild starts fromcreate_app(skipped whenTESTING=True).Summary by CodeRabbit