microsoft / microsoft/vscode

NES: simplify NextEditProvider — remaining refactors after streamed-edit dedup

Open
#324,752 0 comments 0 reactions 1 assignee Claimed by @ulugbekna View on GitHub
Dominant language
TypeScript
Stars
193k
Forks
42.4k
PR merge metrics
PR metrics pending

Description

## Context

Follow-up tracking issue for the effort to simplify [`extensions/copilot/src/extension/inlineEdits/node/nextEditProvider.ts`](https://github.com/microsoft/vscode/blob/main/extensions/copilot/src/extension/inlineEdits/node/nextEditProvider.ts) — a ~1640-line god class that mixes ~7 responsibilities.

An initial engineering/architecture review identified 6 issues. The **near-duplicate streaming loops** issue was already addressed in #324451 (extracted `_rebaseAndCacheStreamedEdit`, shared by the regular and speculative paths). This issue tracks the **remaining** items so they aren't lost.

Line references are against `main` at the time of writing and will drift.

## Remaining items

### 1. Class has too many responsibilities (god object)
One class owns: public entry + outcome bookkeeping, cache lookup/validity gating, in-flight request dedup/reuse, stream consumption, speculative request orchestration, delay computation, snippy post-insertion, cross-file caching, rejection collection, and telemetry plumbing. State is spread across ~14 mutable fields (`_pendingStatelessNextEditRequest`, `_lastShownTime`, `_lastShownSuggestionId`, `_lastRejectionTime`, `_lastTriggerTime`, `_lastOutcome`, `_lastNextEditResult`, `_shouldExpandEditWindow`, …).

Suggested split into collaborators:
- **OutcomeTracker** — the `_last*` fields + `handleShown`/`handleAcceptance`/`handleRejection`/`handleIgnored` + the `lastRejectionTime`/`lastTriggerTime`/`lastOutcome` getters.
- **RequestReuseCoordinator** — `_pendingStatelessNextEditRequest`, the `fetchNextEdit` reuse/rebase logic, `_hookupCancellation`.
- **SpeculativeRequestController** — `_triggerSpeculativeRequest`, `_createSpeculativeRequest`, `_runSpeculativeProviderCall`, `shiftSelectionAfterEditWindow` (pairs with the existing `SpeculativeRequestManager`).

### 3. `_getNextEditCanThrow` is a ~205-line method with two tangled paths
`_getNextEditCanThrow` (~lines 339–543) carries 9 mutable locals (`edit`, `currentDocument`, `error`, `req`, `targetDocumentId`, `isRebasedCachedEdit`, `isSubsequentCachedEdit`, `isFromSpeculativeRequest`, `cacheEntry`) reconciled across a cache-hit vs fetch branch, cross-file gating, rejection checks, and delay. Split into `tryServeFromCache()` / `fetchFresh()` returning one result object.

### 4. Config reads scattered across ~13 call sites
`getExperimentBasedConfig(...)` is inlined at 13 sites throughout the file. `INesConfigs`/`determineNesConfigs` exist but hold a single bool. Centralize into one config snapshot per request to remove `_expService` threading and make behavior testable.

### 5. Cross-cutting params threaded through ~10 methods
`telemetryBuilder`, `logContext`, `logger`, `cancellationToken` are passed through nearly every method — bundle into a per-request context object.

### 6. Minor cleanups
- `INextEditProvider` (line ~184) is generic but has a single implementation — consider dropping the generics.
- `assertDefined` (line ~1622) duplicates the existing `assert` helper.
- Several very long invariant comments (e.g. `getSourcePatchIndex`, cross-file cache gating) signal fragility that the structural splits above would reduce.

## Notes
- There are ~2,600 lines of tests (`nextEditProviderCaching.spec.ts`, `nextEditProviderSpeculative.spec.ts`, `nextEditProviderTelemetry.spec.ts`) to validate these refactors against — all should stay green.
- These are independent and can be tackled in separate PRs. Recommended order: #4 (config snapshot) → #5 (request context) → #3 (split `_getNextEditCanThrow`) → #1 (extract collaborators) → #6 (cleanups).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.