feat(search): surface Zoekt non-exhaustive results in UI and logs (#504)#1098
Open
h30s wants to merge 2 commits intosourcebot-dev:mainfrom
Open
feat(search): surface Zoekt non-exhaustive results in UI and logs (#504)#1098h30s wants to merge 2 commits intosourcebot-dev:mainfrom
h30s wants to merge 2 commits intosourcebot-dev:mainfrom
Conversation
Contributor
WalkthroughAdds user-facing explanations when searches are incomplete by propagating search stats through the streaming hook, computing limit explanations, logging non-exhaustive Zoekt searches, and conditionally rendering an alert in the search results UI. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as SearchResultsPage (UI)
participant Hook as useStreamedSearch (hook/cache)
participant Zoekt as zoektSearcher (backend)
UI->>Hook: initiate search request
Hook->>Zoekt: stream search request
Zoekt-->>Hook: incremental stream responses (hits / partial stats)
Hook->>Hook: update cached entry with partial results
Zoekt-->>Hook: final response with accumulated stats
Hook->>Hook: persist stats into cache/state
Hook-->>UI: deliver final results + stats
UI->>UI: compute getSearchLimitExplanation(stats, maxMatchDisplayCount)
alt explanation present and not streaming
UI->>UI: render amber Alert with summary/detail above results
end
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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/app/(app)/search/useStreamedSearch.ts (1)
110-119:⚠️ Potential issue | 🟡 MinorMissing
statsreset when starting a new search.When initiating a new search (not from cache), the state is reset but
statsis not explicitly set toundefined. This could potentially leave stale stats from a previous search visible until the new search completes.🛠️ Proposed fix
setState({ isStreaming: true, isExhaustive: false, error: null, files: [], repoInfo: {}, timeToSearchCompletionMs: 0, timeToFirstSearchResultMs: 0, numMatches: 0, + stats: undefined, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/`(app)/search/useStreamedSearch.ts around lines 110 - 119, When starting a new search in useStreamedSearch you reset many state fields but forgot to clear previous stats, which can leave stale stats visible; update the setState call inside the new-search branch to also set stats: undefined (or null) so the previous SearchStats value is cleared, referencing the setState call in useStreamedSearch and the stats property on the search state object.
🧹 Nitpick comments (2)
packages/web/src/features/search/searchLimitExplanation.ts (1)
59-62: Fallback message may be unreachable or indicate a logic gap.This fallback returns a generic "more matches may exist" message when none of the preceding conditions match. However, if the search is non-exhaustive (which is the precondition for calling this function based on
searchResultsPage.tsx), at least one of the earlier conditions should logically be true:
totalMatchCount > actualMatchCountimpliestotalMatchCount > maxMatchDisplayCount(given how limits are configured)- Or
shardsSkipped > 0/filesSkipped > 0/ a non-defaultflushReasonConsider whether this fallback is reachable. If it is, it may indicate an unexpected edge case worth investigating. If it's purely defensive, adding a comment would help future maintainers understand its purpose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/search/searchLimitExplanation.ts` around lines 59 - 62, The fallback message in searchLimitExplanation appears defensive/unreachable given the preconditions; update the function searchLimitExplanation to either (a) add a concise comment above the fallback return explaining that this branch is purely defensive (describe the expected conditions such as totalMatchCount/actualMatchCount, shardsSkipped/filesSkipped, or non-default flushReason) so future maintainers understand why it exists, or (b) if you prefer stronger guarantees, replace the fallback with an explicit assertion or error/log when reached (e.g., throw or process a diagnostic) so unexpected edge cases are surfaced; ensure the change references the same return object shape (summary/detail) to keep callers unchanged.packages/web/src/features/search/searchLimitExplanation.test.ts (1)
73-83: Consider adding test coverage forFLUSH_REASON_MAX_SIZE.The test suite covers
FLUSH_REASON_TIMER_EXPIREDbut notFLUSH_REASON_MAX_SIZE. Adding a test would ensure both flush reason branches behave correctly.💡 Suggested test case
+test('flushReason max size when no higher-priority signal', () => { + const out = getSearchLimitExplanation( + stats({ + flushReason: 'FLUSH_REASON_MAX_SIZE', + totalMatchCount: 10, + actualMatchCount: 10, + }), + 100, + ); + expect(out.summary).toContain('size limit'); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/search/searchLimitExplanation.test.ts` around lines 73 - 83, Add a new unit test for the FLUSH_REASON_MAX_SIZE branch by duplicating the existing timer test pattern: call getSearchLimitExplanation with stats({ flushReason: 'FLUSH_REASON_MAX_SIZE', totalMatchCount: 10, actualMatchCount: 10 }) and the same limit (100), then assert the returned out.summary reflects the max-size flush (e.g., expect(out.summary).toContain('max size') or another project-specific phrase used for that branch). Ensure the test name references "flushReason max size" so getSearchLimitExplanation and the FLUSH_REASON_MAX_SIZE branch are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/web/src/app/`(app)/search/useStreamedSearch.ts:
- Around line 110-119: When starting a new search in useStreamedSearch you reset
many state fields but forgot to clear previous stats, which can leave stale
stats visible; update the setState call inside the new-search branch to also set
stats: undefined (or null) so the previous SearchStats value is cleared,
referencing the setState call in useStreamedSearch and the stats property on the
search state object.
---
Nitpick comments:
In `@packages/web/src/features/search/searchLimitExplanation.test.ts`:
- Around line 73-83: Add a new unit test for the FLUSH_REASON_MAX_SIZE branch by
duplicating the existing timer test pattern: call getSearchLimitExplanation with
stats({ flushReason: 'FLUSH_REASON_MAX_SIZE', totalMatchCount: 10,
actualMatchCount: 10 }) and the same limit (100), then assert the returned
out.summary reflects the max-size flush (e.g.,
expect(out.summary).toContain('max size') or another project-specific phrase
used for that branch). Ensure the test name references "flushReason max size" so
getSearchLimitExplanation and the FLUSH_REASON_MAX_SIZE branch are covered.
In `@packages/web/src/features/search/searchLimitExplanation.ts`:
- Around line 59-62: The fallback message in searchLimitExplanation appears
defensive/unreachable given the preconditions; update the function
searchLimitExplanation to either (a) add a concise comment above the fallback
return explaining that this branch is purely defensive (describe the expected
conditions such as totalMatchCount/actualMatchCount, shardsSkipped/filesSkipped,
or non-default flushReason) so future maintainers understand why it exists, or
(b) if you prefer stronger guarantees, replace the fallback with an explicit
assertion or error/log when reached (e.g., throw or process a diagnostic) so
unexpected edge cases are surfaced; ensure the change references the same return
object shape (summary/detail) to keep callers unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2831eea1-3e4f-497b-b06e-ce7a55043400
📒 Files selected for processing (6)
packages/web/src/app/(app)/search/components/searchResultsPage.tsxpackages/web/src/app/(app)/search/useStreamedSearch.tspackages/web/src/features/search/index.tspackages/web/src/features/search/searchLimitExplanation.test.tspackages/web/src/features/search/searchLimitExplanation.tspackages/web/src/features/search/zoektSearcher.ts
Contributor
Author
|
@brendan-kellam PTA, all green 💚 |
Contributor
|
could you include screenshots in your description? |
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 #504
Summary by CodeRabbit
New Features
Tests