-
Notifications
You must be signed in to change notification settings - Fork 175
fix(web): Search performance improvements #615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughSearch optimization increases default result limits from 5,000 to 100,000 entries. Schema-based validation in the client API is replaced with type assertions. Performance instrumentation and timing measurements are added to the search flow, with debug timings returned in responses. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SearchAPI as Search API
participant Zoekt
participant Response
Client->>SearchAPI: search({ query, matches, ... })
rect rgb(240, 248, 255)
Note over SearchAPI: measure: fetch
SearchAPI->>Zoekt: fetch search results
Zoekt-->>SearchAPI: raw response
end
rect rgb(240, 248, 255)
Note over SearchAPI: measure: parse_json
SearchAPI->>SearchAPI: JSON.parse(response)
end
rect rgb(240, 248, 255)
Note over SearchAPI: measure: transform
SearchAPI->>SearchAPI: transformZoektSearchResponse()
end
SearchAPI->>Response: SearchResponse + __debug_timings
Response-->>Client: { results, __debug_timings }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
This comment has been minimized.
This comment has been minimized.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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/api/(client)/client.ts (1)
43-52: Restore ServiceError handling when fetching repos.
Line 51 currently casts the raw JSON toGetReposResponse, so a backendServiceErrorpayload comes back as if it were a repo list and downstream code will explode when it dereferences repo fields. Please propagate the error by updating the signature and checkingisServiceErrorbefore casting.Apply this diff:
-export const getRepos = async (): Promise<GetReposResponse> => { +export const getRepos = async (): Promise<GetReposResponse | ServiceError> => { const result = await fetch("/api/repos", { method: "GET", headers: { "Content-Type": "application/json", }, }).then(response => response.json()); - return result as GetReposResponse | ServiceError; + if (isServiceError(result)) { + return result; + } + + return result as GetReposResponse; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)packages/web/src/app/[domain]/search/components/searchResultsPage.tsx(1 hunks)packages/web/src/app/api/(client)/client.ts(4 hunks)packages/web/src/features/search/schemas.ts(1 hunks)packages/web/src/features/search/searchApi.ts(4 hunks)packages/web/src/features/search/zoektSchema.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (.cursor/rules/style.mdc)
Filenames should always be camelCase. Exception: if there are filenames in the same directory with a format other than camelCase, use that format to keep things consistent.
Files:
packages/web/src/features/search/schemas.tsCHANGELOG.mdpackages/web/src/app/api/(client)/client.tspackages/web/src/app/[domain]/search/components/searchResultsPage.tsxpackages/web/src/features/search/zoektSchema.tspackages/web/src/features/search/searchApi.ts
🪛 LanguageTool
CHANGELOG.md
[grammar] ~18-~18: Ensure spelling is correct
Context: ... in search api, resulting in a order of magnitutde performance improvement. [#615](https:/...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
After benchmarking our search api, I noticed that the zod
parseAsynccall we were doing insearchApi.tswas contributing to a significant portion of the total search time (average 64.6% in my benchmarks), especially for larger queries that return payloads above > 10MB. This issue colinhacks/zod#205 confirms others are hitting this with zod as well.We don't really need to parse zoekt's response bodies (since we always expect them to be valid), so this PR removes the
parseAsynccall and instead does a simple cast. The results are pretty dramatic: my initial benchmarks are shoing a increase in search performance by a order of magnitude (89.8% reduction in average search time). Here's the before & after on the benchmark:With this change, I figured we can bump the default number of results requested to 100k (up from 5k!). In my testing performance was good and I was able to get 100k results in <5 seconds.
Summary by CodeRabbit
New Features
Changed
Bug Fixes