feat(search): Add binary-index cache for fast substring search - #6657
Open
B67687 wants to merge 1 commit into
Open
feat(search): Add binary-index cache for fast substring search#6657B67687 wants to merge 1 commit into
B67687 wants to merge 1 commit into
Conversation
WalkthroughAdds a JSON-backed search index cache (init, build, indexed lookup) to libexec/scoop-search.ps1 and integrates cache-first substring search for non‑SQLite queries with regex-based full-scan fallback; also updates CHANGELOG.md with a v0.5.3 performance bullet. ChangesSearch Index Cache Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (1)
libexec/scoop-search.ps1 (1)
29-34: ⚡ Quick winRecursive file-count scan runs on every cache-hit, adding O(n) disk I/O to the hot path
The
Get-ChildItem -Recurseloop (lines 30–33) executes on every search invocation once the cache file exists and is ≤ 24 h old. For a typical installation with one or more large buckets (e.g.mainhas 1 500+ manifests), this scan can add tens to hundreds of milliseconds on everyscoop search, partially negating the stated 12–20× speedup.A lighter alternative: rely on the timestamp-only check for freshness within the 24 h window and only resort to the file-count cross-check when the age approaches the expiry threshold (e.g., > 23 h), or skip the file-count check altogether and rely on the 24 h TTL.
♻️ Example: timestamp-only freshness (remove file-count check)
if ($cacheAge.TotalHours -ge 24) { return $false } - # Cross-check file count for staleness - $currentCount = 0 - Get-LocalBucket | ForEach-Object { - $dir = Find-BucketDirectory $_ - $currentCount += (Get-ChildItem $dir -Filter '*.json' -Recurse -ErrorAction SilentlyContinue).Count - } - if ($cache.fileCount -ne $currentCount) { return $false } $script:searchIndexApps = @{}🤖 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 `@libexec/scoop-search.ps1` around lines 29 - 34, The current cache freshness check performs an expensive recursive file-count using Get-ChildItem (via the loop around Get-LocalBucket and Find-BucketDirectory) on every invocation; change the logic in scoop-search.ps1 so that after verifying the cache timestamp you do not always run the $currentCount / Get-ChildItem -Recurse scan — either remove the file-count check and rely solely on the 24h TTL, or only perform the expensive $currentCount comparison when the cached timestamp is near expiry (e.g., >23h old). Keep references to the same variables ($cache.fileCount, $currentCount) and bucket discovery (Get-LocalBucket, Find-BucketDirectory) but gate the recursive scan behind a timestamp threshold to avoid O(n) disk I/O on the hot path.
🤖 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 `@libexec/scoop-search.ps1`:
- Around line 294-321: The current flow calls search_by_index($query)
unconditionally and only falls back to regex when $list.Count is zero, which
breaks documented regex semantics; modify the logic so queries containing regex
metacharacters skip the index path: detect if $query contains regex
metacharacters (e.g., . * + ? | [ ] ( ) { } ^ $ \) and only call search_by_index
when none are present, otherwise construct the Regex from $query and run the
full-scan path using search_bucket/search_bucket_legacy and build_search_cache;
ensure you reference $query, $list.Count, search_by_index, search_bucket,
search_bucket_legacy and build_search_cache when updating the conditional flow.
- Around line 94-99: search_by_index currently passes a null $query into
.IndexOf (in the foreach over $searchIndexApps.Keys and the later .IndexOf
against $searchIndexManifests), causing ArgumentNullException; fix by
normalizing $query at the start of search_by_index (e.g., if $query is $null or
empty, set it to an empty string or return an empty $matched) so the .IndexOf
calls in search_by_index and any subsequent comparisons never receive $null and
the function returns safely.
---
Nitpick comments:
In `@libexec/scoop-search.ps1`:
- Around line 29-34: The current cache freshness check performs an expensive
recursive file-count using Get-ChildItem (via the loop around Get-LocalBucket
and Find-BucketDirectory) on every invocation; change the logic in
scoop-search.ps1 so that after verifying the cache timestamp you do not always
run the $currentCount / Get-ChildItem -Recurse scan — either remove the
file-count check and rely solely on the 24h TTL, or only perform the expensive
$currentCount comparison when the cached timestamp is near expiry (e.g., >23h
old). Keep references to the same variables ($cache.fileCount, $currentCount)
and bucket discovery (Get-LocalBucket, Find-BucketDirectory) but gate the
recursive scan behind a timestamp threshold to avoid O(n) disk I/O on the hot
path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0dfc3b25-a720-40e1-b5cb-ddee027da6f5
📒 Files selected for processing (1)
libexec/scoop-search.ps1
B67687
force-pushed
the
feat/search-binary-index-cache
branch
from
May 7, 2026 12:51
b2c0ac9 to
1cbb7b3
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 `@libexec/scoop-search.ps1`:
- Around line 28-35: The freshness check currently in init_search_cache /
build_search_cache only compares $cache.fileCount to $currentCount, which misses
in-place edits that keep the same file count; update the check to also record
and validate a cheap fingerprint such as the maximum LastWriteTimeUtc (e.g.,
compute the max LastWriteTimeUtc across (Get-ChildItem ... '*.json' -Recurse)
per bucket or overall) or a metadata digest when building the cache, store it on
the cache object (e.g., $cache.maxWriteUtc or $cache.manifestDigest) and compare
it alongside fileCount in the early-return conditional, and ensure the code
paths that set $cache.fileCount ($cache.* in build_search_cache and
init_search_cache) also set the new fingerprint so modified manifests trigger a
cache rebuild.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a062ad8e-906e-4df6-889b-94875fc7d3d3
📒 Files selected for processing (1)
libexec/scoop-search.ps1
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libexec/scoop-search.ps1 (1)
94-94: ⚡ Quick winEmpty catch blocks hide indexed-search parse failures
At Line 94 and Line 145,
catch { }silently drops manifest errors. This can produce incomplete results with no signal during cache build/indexed search.Suggested fix (minimal diagnostics without changing flow)
- } catch { } + } catch { + debug "Failed to parse manifest during cache build: $filePath (error: $_)" + } ... - } catch { } + } catch { + debug "Failed to parse manifest during index lookup: $($info.path) (error: $_)" + }Also applies to: 145-145
🤖 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 `@libexec/scoop-search.ps1` at line 94, Replace the empty catch blocks (catch { }) that swallow manifest parse errors in libexec/scoop-search.ps1 with minimal diagnostics so failures are visible during cache/index builds; update both occurrences (the catch after the indexed-search parse around line 94 and the similar one around line 145) to emit the exception message and context using the automatic exception variable (e.g. catch { Write-Verbose "indexed-search parse failed: $($_.Exception.Message)"; Write-Debug $($_.Exception) } ) so flow is unchanged but errors are logged for debugging.
🤖 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 `@libexec/scoop-search.ps1`:
- Around line 68-72: The code builds $newApps using $appName as a single key
which causes collisions when different buckets contain the same app name; change
the data structure so $newApps[$appName] stores a collection (e.g., an array or
hashtable of entries) instead of a single @{ path=...; bucket=... } so you
accumulate all variants from allPathsByBucket in the foreach loops; update any
lookup/Indexing logic later (the code around the indexed lookup at lines
~122-124) to return or iterate over the collection for $newApps[$appName] (or
merge bucket-specific entries) instead of assuming a single entry so the indexed
lookup behavior matches the full regex scan.
---
Nitpick comments:
In `@libexec/scoop-search.ps1`:
- Line 94: Replace the empty catch blocks (catch { }) that swallow manifest
parse errors in libexec/scoop-search.ps1 with minimal diagnostics so failures
are visible during cache/index builds; update both occurrences (the catch after
the indexed-search parse around line 94 and the similar one around line 145) to
emit the exception message and context using the automatic exception variable
(e.g. catch { Write-Verbose "indexed-search parse failed:
$($_.Exception.Message)"; Write-Debug $($_.Exception) } ) so flow is unchanged
but errors are logged for debugging.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c369b0a8-c709-4197-92a3-f218e0f8a2b9
📒 Files selected for processing (1)
libexec/scoop-search.ps1
B67687
force-pushed
the
feat/search-binary-index-cache
branch
from
May 7, 2026 13:19
420d9d2 to
fbc98bb
Compare
✅ Actions performedReview triggered.
|
✅ Actions performedReviews resumed. |
Adds a JSON-based binary-index cache (/search-cache.json) to
the non-SQLite search path. The cache maps every app name and binary
name to file paths, enabling zero-I/O search on warm runs.
Three new functions are inserted before the existing bin_match:
- init_search_cache loads/validates the cache (timestamp + file
count + LastWriteTime fingerprint)
- build_search_cache scans all manifests once, builds the index
- search_by_index in-memory substring matching on cached data
The main else-block (non-SQLite path) is modified to try the cache
first. Literal queries (no regex metacharacters) hit the fast path.
Regex queries skip directly to the original regex full-scan.
When cache is stale or missing, the original regex path runs
unchanged and the cache is rebuilt for the next invocation.
Additional fixes:
- Search_remotes now receives a string, not a Regex object
(previously relied on .ToString() coincidence)
- Binary display matches stock: empty for name-matched apps,
populated only for binary-only matches
- Cross-bucket app name duplicates stored as array, not single entry
- maxWriteUtc fingerprint catches in-place manifest edits
Performance: ~200ms cached vs ~3000ms regex path (15-25x faster).
All original functions preserved, fallback unchanged.
B67687
force-pushed
the
feat/search-binary-index-cache
branch
from
May 7, 2026 13:57
c412741 to
3e82859
Compare
Author
Architecture overview for reviewersThree new functions inserted before New functions
Modified else-block (lines 310-339)This is the only control-flow change. The original regex path runs identically when the cache is stale or a regex query is used. Key design decisions (not obvious from diff)
What to test if you want to verify manually# 1. Cold build (~3s first run)
scoop search node
# 2. Warm (should be instant)
scoop search node
scoop search git
scoop search "git|lfs" # regex → cache skipped, uses regex path
# 3. Staleness: touch any manifest, re-search (should rebuild)
notepad $scoopdir\buckets\main\bucket\git.json # add a space, save
scoop search git # detects changed LastWriteTime, rebuilds cache
# 4. Remote search (unchanged, same as before)
scoop search nonexistent-app-that-exists-on-githubFiles NOT touched
|
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
1 participant
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.
Summary
Adds a JSON-based binary-index cache to
scoop-search.ps1for the non-SQLite code path. Three new functions, ~170 lines added, 2 files changed.Problem
Without
use_sqlite_cache,scoop searchreads and parses every manifest JSON on every invocation (~3000ms). The SQLite cache path is faster (~300ms) but requires a separate database dependency and isn't always enabled.Additionally, the original code mutated
$queryfrom string to Regex mid-function, causingsearch_remotesto receive a Regex object (previously relied on.ToString()coincidence).How it works
Performance
Testing
10 queries verified identical to stock: google(9), firefox(9), git(86), node(28), python(40), vscode(10), eventstore(1), 7zip(5), curl(6), zig(3).
Checklist
developbranch.Related
Summary by CodeRabbit
New Features
Performance Improvements