feat: add timeout in searching text in files #1233
Conversation
| const timeoutInMs = 10_000; | ||
| let results = await Promise.race([ | ||
| this.searchAndCollectResults(options.input.query, isRegExp, patterns, maxResults, token), | ||
| new Promise<never>((_, reject) => setTimeout(() => reject(new Error("Timeout in searching text in files")), timeoutInMs))]); |
There was a problem hiding this comment.
Check whether any helpers in async.ts could be reused here- timeout, raceCancellation, etc. If not that's fine, it's pretty simple.
There was a problem hiding this comment.
There areraceTimeout, timeout utilities, but they just resolve to undefined when time's up, while in this case I think we want to throw an error similar to the cancellation case. Do you think we should make a new utility for that?
There was a problem hiding this comment.
Is timeout(ms).then(() => Promise.reject(new Error("...")) cleaner?
bryanchen-d
force-pushed
the
brchen/timeout-in-searching
branch
from
9dec97e to
e1d6814
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds timeout functionality to the findFiles and findTextInFiles tools to prevent them from hanging chat operations. The changes implement a 10-second timeout for both search operations and prompt rendering to improve user experience.
Key changes:
- Added timeout wrapper using
raceTimeoutandraceCancellationutilities - Applied timeout to both file search operations and prompt rendering phases
- Added consistent error handling when timeouts occur
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/extension/tools/node/findTextInFilesTool.tsx | Added timeout logic to text search operations and prompt rendering with proper error handling |
| src/extension/tools/node/findFilesTool.tsx | Added timeout logic to file search operations and prompt rendering with consistent error messaging |
| const timeoutInMs = 10_000; | ||
| const results = await raceTimeout( | ||
| raceCancellation( | ||
| Promise.resolve(this.searchService.findFiles(pattern, undefined, token)), |
There was a problem hiding this comment.
The Promise.resolve() wrapper is unnecessary here. The findFiles method likely already returns a Promise, so this adds unnecessary complexity.
| Promise.resolve(this.searchService.findFiles(pattern, undefined, token)), | |
| this.searchService.findFiles(pattern, undefined, token), |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Unfortunately the findFiles returns a Thenable<> which doesn't fit the required Promise<> type, and fails compilation.
There was a problem hiding this comment.
It would make sense for the search service to return a Promise instead (this comes from our extension API but our searchService could do the Promise.resolve. You can do that if it makes things cleaner, you don't have to
| token); | ||
| const results: vscode.TextSearchResult2[] = []; | ||
| for await (const item of searchResult.results) { | ||
| await new Promise(r => setTimeout(r, 5000)); // yield to allow cancellation to be processed |
There was a problem hiding this comment.
What's this? We need to wait 5s?
There was a problem hiding this comment.
oops, this shouldn't be checked in, I was using it to validate the timeout case
Add timeout in findFiles and findTextInFiles tool so they won't hang the chat.