NES: centralize NextEditProvider config reads into a per-request snapshot (#324752 item 4)#324784
Draft
ulugbekna wants to merge 1 commit into
Draft
NES: centralize NextEditProvider config reads into a per-request snapshot (#324752 item 4)#324784ulugbekna wants to merge 1 commit into
ulugbekna wants to merge 1 commit into
Conversation
…ots (#324752 item 4) Consolidate the experiment-based getExperimentBasedConfig(...) reads that were scattered across the NES request path into two per-flow config snapshots computed once per request: - INesRequestConfigs (main request path): isAsyncCompletions, debounceUseCoreRequestTime, autoExpandEditWindowLines, cacheDelay, rebasedCacheDelay, subsequentCacheDelay, speculativeRequestDelay. Read once in _getNextEditCanThrow via _readRequestConfigs() and threaded through fetchNextEdit, _executeNewNextEditRequest and computeMinimumResponseDelay. - INesSpeculativeConfigs (speculative request path): cursorPlacement, autoExpandEditWindowLinesSetting, autoExpandEditWindowLines. Read once at the top of _triggerSpeculativeRequest via _readSpeculativeConfigs() and threaded into _createSpeculativeRequest. The two request-producing flows are kept as separate snapshots so neither flow reads (or exposes) the other flow's experiment flags. The two per-keystroke / per-shown enablement gates (InlineEditsAsyncCompletions in _cancelPendingRequestDueToDocChange, InlineEditsSpeculativeRequests in handleShown) are intentionally left as direct reads (commented), since they are not part of a request lifecycle. The main-flow telemetry + log side effects are preserved byte-for-byte (setNESConfigs stays the { isAsyncCompletions } subset). _expService is retained (still required by NextEditCache and the two gate reads). Layer 1 of a stacked refactor (#324752); layers #5/#3/#1/#6 build on this branch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is layer 1 of a stacked refactor of nextEditProvider.ts (item 4 of tracking issue #324752). It replaces the 13 inlined getExperimentBasedConfig(...) reads with two per-request config snapshots computed once per request and threaded through the methods that consume them. This is a preparatory, behavior-preserving refactor that later layers (per-request context object, method splits, collaborator extraction) build upon.
Changes:
- Adds two snapshot interfaces to
nesConfigs.ts:INesRequestConfigs(extendsINesConfigs, main request path) andINesSpeculativeConfigs(speculative path). - Introduces
_readRequestConfigs()and_readSpeculativeConfigs()readers, replacingdetermineNesConfigs, and threads the snapshots through the relevant methods. - Leaves 2 enablement-gate reads as direct reads (documented), and preserves the
{ isAsyncCompletions }telemetry/log subset exactly.
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/inlineEdits/node/nesConfigs.ts | Adds INesRequestConfigs and INesSpeculativeConfigs snapshot interfaces with JSDoc explaining the two-flow separation. |
| extensions/copilot/src/extension/inlineEdits/node/nextEditProvider.ts | Replaces inline config reads with two per-request readers, threads snapshots through request/speculative flows, and preserves telemetry/log side effects via an explicit INesConfigs subset. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
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
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.
Part of #324752 (item 4 — "Config reads scattered across ~13 call sites"). Layer 1 of a stacked refactor; layers #5 (per-request context object), #3 (split
_getNextEditCanThrow), #1 (extract collaborators), and #6 (cleanups) will build on this branch.What
getExperimentBasedConfig(...)was inlined at 13 call sites throughoutnextEditProvider.ts. This centralizes those reads into per-request config snapshots computed once per request and threaded through the methods that need them, instead of reading config inline.Two snapshot types are added to
nesConfigs.ts:INesRequestConfigs(main request path) —isAsyncCompletions,debounceUseCoreRequestTime,autoExpandEditWindowLines,cacheDelay,rebasedCacheDelay,subsequentCacheDelay,speculativeRequestDelay. Read once in_getNextEditCanThrowvia_readRequestConfigs()and threaded throughfetchNextEdit→_executeNewNextEditRequestandcomputeMinimumResponseDelay(+ the debounce read).INesSpeculativeConfigs(speculative request path) —cursorPlacement,autoExpandEditWindowLinesSetting,autoExpandEditWindowLines. Read once at the top of_triggerSpeculativeRequestvia_readSpeculativeConfigs()and threaded into_createSpeculativeRequest.INesRequestConfigs extends INesConfigs, so the telemetry-facing{ isAsyncCompletions }subset is unchanged.Why two snapshots (not one)
The issue framed this as "one snapshot", but the reads actually span two independent request-producing flows — the main request and the speculative request — which are separate invocations. Using two focused snapshots (rather than one shared type) keeps each flow reading only its own experiment flags, so it does not shift experiment-exposure timing for the other flow's
TeamInternalflags. This keeps the change strictly behavior-preserving, which matters because later layers stack on it.Consolidation
InlineEditsAsyncCompletionsin_cancelPendingRequestDueToDocChange(per-keystroke autorun gate) andInlineEditsSpeculativeRequestsinhandleShown(per-shown enablement gate). These are enablement gates checked outside a request lifecycle; folding them into a snapshot would run ~10 config reads per keystroke / shown-event for no functional gain.Notes
setNESConfigs(...)stays the{ isAsyncCompletions }subset; the logged codeblock is unchanged)._expServiceis retained — still required by theNextEditCacheconstructor and the two gate reads. It could not be dropped.Validation
cd extensions/copilot && npx vitest --run --pool=forks nextEditProvider→ 3 files, 66 passed / 2 skipped (unchanged from baseline):nextEditProviderCaching.spec.ts,nextEditProviderSpeculative.spec.ts,nextEditProviderTelemetry.spec.ts.npx tsgo --noEmit --project tsconfig.json→ clean.