-
Notifications
You must be signed in to change notification settings - Fork 1.6k
ghost: use CompletionsFetchService #2694
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
da853ea to
fd25c2a
Compare
ab0f1c5 to
0abbb75
Compare
0abbb75 to
c861abc
Compare
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.
Pull request overview
This pull request refactors the ghost text completion system to use the CompletionsFetchService abstraction layer. This change aims to consolidate the network fetching logic and improve code maintainability by removing duplicate streaming/filtering logic.
Changes:
- Introduces a new
fetchAndStreamCompletions2method in the fetcher service that usesCompletionsFetchServicefor network requests - Moves
CopilotAnnotationtypes to a shared platform location for reuse across different parts of the codebase - Removes the
QuotaExceedederror type in favor of handling it throughUnsuccessfulResponsewith proper status codes - Simplifies function signatures by removing unused parameters (e.g.,
triggerCategory,triggerKind)
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/completions-core/common/openai/copilotAnnotations.ts | New shared location for CopilotAnnotation types, moved from extension-specific code |
| src/platform/nesFetch/node/completionsFetchServiceImpl.ts | Updated to return enhanced FetchResponse with requestId and response, removed obsolete headersObjectToKv helper |
| src/platform/nesFetch/common/responseStream.ts | Added destroy() method and constructor parameters for request tracking |
| src/platform/nesFetch/common/completionsFetchService.ts | Removed QuotaExceeded error class, updated UnsuccessfulResponse to include headers and text getter |
| src/platform/networking/common/fetch.ts | Refactored getRequestId to accept IHeaders instead of Response for better reusability |
| src/extension/completions-core/vscode-node/lib/src/openai/fetch.ts | Added fetchAndStreamCompletions2 implementation with CompletionsFetchService integration and retry logic |
| src/extension/completions-core/vscode-node/lib/src/ghostText/ghostText.ts | Refactored to use class-based CompletionsFromNetwork and added experiment flag for new fetch service |
| src/extension/completions-core/vscode-node/lib/src/ghostText/telemetry.ts | Simplified telemetryShown by removing redundant insertionCategory parameter |
| src/extension/completions-core/vscode-node/lib/src/ghostText/last.ts | Removed unused triggerCategory and triggerKind parameters from acceptance handlers |
| src/platform/configuration/common/configurationService.ts | Added GhostTextUseCompletionsFetchService experiment configuration flag |
| test/base/*.ts | Updated test mocks to match new FetchResponse interface with additional fields |
Comments suppressed due to low confidence (1)
src/platform/nesFetch/common/responseStream.ts:77
- Missing await. The value 'body' is always a promise.
if (body && 'destroy' in body && typeof body.destroy === 'function') {
src/extension/completions-core/vscode-node/lib/src/ghostText/ghostText.ts
Show resolved
Hide resolved
Pull request was converted to draft
also comes with lots of fixes in CompletionsFetchService
c861abc to
3a117ee
Compare
No description provided.