NES: use git-merge icon for reused in-flight requests#311099
Conversation
Previously both cache hits and reused in-flight requests (async pending or speculative) showed the database icon in the NES debug log. This made it hard to tell whether a result was served from cache or joined an existing stream. Add a new \`reusedInFlight\` outcome with a git-merge icon ($(git-merge)) to visually distinguish 'joined an in-flight request' from 'loaded from cache'. The database icon is preserved for true cache hits via \`setIsCachedResult\`; reused in-flight requests now go through the new \`setIsReusedInFlightResult\` method.
There was a problem hiding this comment.
Pull request overview
This PR improves the NES (Next Edit Suggestions) debug log by visually distinguishing “joined an existing in-flight request” from a true cache hit, using a new reusedInFlight outcome rendered with the $(git-merge) icon.
Changes:
- Add a
gitMergeicon to the inline-edits icon set. - Introduce
reusedInFlightas a new log outcome, including legend text and icon mapping. - Update the in-flight join path to mark joined requests as
reusedInFlightinstead ofcached.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/platform/inlineEdits/common/utils/utils.ts | Adds Icon.gitMerge (ThemeIcon + SVG) for use in debug log rendering. |
| extensions/copilot/src/platform/inlineEdits/common/inlineEditLogContext.ts | Adds reusedInFlight outcome, legend entry, icon resolution, and a new setter to mark reused-in-flight results. |
| extensions/copilot/src/extension/inlineEdits/node/nextEditProvider.ts | Switches the “join existing request” path to use setIsReusedInFlightResult(...). |
Copilot's findings
Comments suppressed due to low confidence (2)
extensions/copilot/src/platform/inlineEdits/common/inlineEditLogContext.ts:323
setIsReusedInFlightResultassignsthis._logContextOfCachedEdit = .... That makes reused-in-flight requests show up as “cached” intoLogDocument()/toMinimalLog()(e.g.(cached #...),**From cache:** YES) and also setstoJSON().isCachedResultto true. This undermines the goal of distinguishing cache hits vs in-flight reuse; consider tracking reuse separately (e.g. a dedicated field / enum) and updating the derived strings/JSON flags accordingly.
setIsReusedInFlightResult(logContextOfReusedRequest: InlineEditRequestLogContext): void {
this._logContextOfCachedEdit = logContextOfReusedRequest;
extensions/copilot/src/platform/inlineEdits/common/inlineEditLogContext.ts:329
setIsReusedInFlightResultlargely duplicates the direct-field-copy logic fromsetIsCachedResult. To prevent the two paths from drifting (and to keep future changes consistent), it would be better to extract the shared “inherit fields from other context” logic into a private helper that both methods call, with only the outcome/source-kind differing.
this.recordingBookmark = logContextOfReusedRequest.recordingBookmark;
this._nextEditRequest = logContextOfReusedRequest._nextEditRequest ?? this._nextEditRequest;
this._resultEdit = logContextOfReusedRequest._resultEdit ?? this._resultEdit;
this._diagnosticsResultEdit = logContextOfReusedRequest._diagnosticsResultEdit ?? this._diagnosticsResultEdit;
this._endpointInfo = logContextOfReusedRequest._endpointInfo ?? this._endpointInfo;
this._headerRequestId = logContextOfReusedRequest._headerRequestId ?? this._headerRequestId;
- Files reviewed: 3/3 changed files
- Comments generated: 2
| * - `cancelled`: request was cancelled via CancellationToken (shown as skipped) | ||
| * - `errored`: an error occurred | ||
| * - `previouslyRejected`: result matches a suggestion that was previously rejected | ||
| */ | ||
| type LogContextOutcome = 'pending' | 'succeeded' | 'noSuggestions' | 'cached' | 'cachedFromGhostText' | 'skipped' | 'cancelled' | 'errored' | 'previouslyRejected'; | ||
| type LogContextOutcome = 'pending' | 'succeeded' | 'noSuggestions' | 'cached' | 'cachedFromGhostText' | 'reusedInFlight' | 'skipped' | 'cancelled' | 'errored' | 'previouslyRejected'; |
There was a problem hiding this comment.
The LogContextOutcome JSDoc list above doesn’t mention the new reusedInFlight outcome, so the documentation is now incomplete/misleading. Please add a bullet describing reusedInFlight (and how it differs from cached).
This issue also appears in the following locations of the same file:
- line 321
- line 324
|
|
||
| telemetryBuilder.setRequest(nextEditRequest); | ||
| logContext.setRequestInput(nextEditRequest); | ||
| logContext.setIsCachedResult(nextEditRequest.logContext); | ||
| logContext.setIsReusedInFlightResult(nextEditRequest.logContext); | ||
|
|
There was a problem hiding this comment.
This change introduces a new user-visible debug outcome for “reused in-flight” requests, but there doesn’t appear to be test coverage ensuring the joined-request path sets reusedInFlight (and not cached). Consider adding a test (likely in the existing speculative/async reuse provider specs) that exercises _joinNextEditRequest and asserts the resulting log context outcome/icon/source flags.
Previously both cache hits and reused in-flight requests (async pending
or speculative) showed the database icon in the NES debug log. This
made it hard to tell whether a result was served from cache or joined
an existing stream.
Add a new `reusedInFlight` outcome with a git-merge icon ($(git-merge))
to visually distinguish 'joined an in-flight request' from 'loaded from
cache'. The database icon is preserved for true cache hits via
`setIsCachedResult`; reused in-flight requests now go through the new
`setIsReusedInFlightResult` method.