Skip to content

NES: refactor InlineEditRequestLogContext state management#308749

Merged
ulugbekna merged 4 commits intomainfrom
ulugbekna/nes-log-context-refactor
Apr 9, 2026
Merged

NES: refactor InlineEditRequestLogContext state management#308749
ulugbekna merged 4 commits intomainfrom
ulugbekna/nes-log-context-refactor

Conversation

@ulugbekna
Copy link
Copy Markdown
Contributor

Summary

Refactors InlineEditRequestLogContext state management to eliminate implicit icon state, unify logging paths, fix stale icon caching, and enforce lifecycle.

Fixes #308719, fixes #308720, fixes #308721, fixes #308722

Changes

1. Replace implicit _icon with explicit outcome state machine (#308719)

Replace the implicit _icon: Icon.t | undefined field with an explicit _outcome: LogContextOutcome discriminated union (8 states: pending, succeeded, noSuggestions, cached, cachedFromGhostText, skipped, cancelled, errored).

  • _resolveIcon() is now a switch on _outcome — every state maps to exactly one icon
  • _isCompleted stays orthogonal (lifecycle flag controlling spinner text + Icon.check fallback)
  • _setOutcome() warns on invalid re-transitions
  • setIsCachedResult() refactored to direct field copy — avoids triggering cascading outcome transitions
  • No caller changes required — public method signatures unchanged

2. Unify live and static log entry paths (#308720, #308722)

Remove the static add() path from InlineEditLogger — all entries now use the live pattern with callbacks.

  • Ghost text provider now uses addLive() + markCompleted() in finally block, ensuring lifecycle is always closed
  • Remove redundant add() call in NES inlineCompletionProvider (was already a no-op)
  • Eliminates the static snapshot path that was the source of frozen spinner bugs

3. Fix stale icon caching in ChatPromptItem (#308721)

Store the main entry reference in ChatPromptItem and always resolve the icon from it.

  • setMainEntry() always sets iconPath (even to undefined), clearing any stale value
  • withFilteredChildren() re-resolves from the entry ref via setMainEntry() instead of copying a potentially stale iconPath snapshot
  • Makes stale icons structurally impossible

Testing

  • All 7731 copilot unit tests pass (vitest)
  • All hygiene checks pass
  • TypeScript compilation clean (only pre-existing unrelated error in copilotcliSessionService.ts)

…#308719)

Replace the implicit \`_icon\` field with an explicit \`_outcome: LogContextOutcome\`
discriminated union in InlineEditRequestLogContext.

- Define LogContextOutcome type with 8 states: pending, succeeded,
  noSuggestions, cached, cachedFromGhostText, skipped, cancelled, errored
- Derive icon from outcome via switch in _resolveIcon()
- Keep _isCompleted orthogonal (lifecycle flag, not an outcome)
- Add _setOutcome() with debug warning on invalid re-transitions
- Refactor setIsCachedResult to use direct field copy (avoids
  triggering outcome transitions during bulk data inheritance)
- Add double-completion warning in markCompleted()
- No caller changes required — public method signatures unchanged
Remove the static \`add()\` path from InlineEditLogger — all log entries
now use the live pattern with callbacks for icon/content resolution.

- Remove InlineEditLogger.add() and _liveRequestIds tracking
- Ghost text provider now uses addLive() + markCompleted() in finally
  block, ensuring lifecycle is always closed
- Remove redundant add() call in NES inlineCompletionProvider (the live
  entry was already registered via addLive())

This eliminates the static snapshot path that was the source of frozen
spinner bugs, and ensures ghost text entries have proper lifecycle
management matching the NES pattern.
Store the main entry reference in ChatPromptItem and always resolve
the icon unconditionally from it, preventing stale icon snapshots.

- Add _mainEntryRef field to ChatPromptItem
- setMainEntry() always sets iconPath (even to undefined when icon
  resolves to undefined), clearing any previous stale value
- withFilteredChildren() re-resolves from the entry ref via
  setMainEntry() instead of copying a potentially stale iconPath

This makes stale icons structurally impossible — the icon is always
derived from the live entry state on each tree refresh.
Copilot AI review requested due to automatic review settings April 9, 2026 10:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors NES/ghost text request logging to make log-tree icons and lifecycle updates deterministic and avoid stale UI state, primarily by moving to explicit outcome state and live log entries.

Changes:

  • Replaced implicit icon mutation in InlineEditRequestLogContext with an explicit outcome (LogContextOutcome) that drives icon resolution.
  • Removed the static/snapshot InlineEditLogger.add() path so requests are logged via live callbacks (addLive) and updated via onDidChange.
  • Fixed stale icon propagation in the request log tree by storing the main entry reference in ChatPromptItem and re-resolving icons.
Show a summary per file
File Description
extensions/copilot/src/platform/inlineEdits/common/inlineEditLogContext.ts Introduces explicit outcome state for icon resolution; refactors cached-result state copying.
extensions/copilot/src/extension/log/vscode-node/requestLogTree.ts Ensures parent tree nodes re-resolve icons from the main entry to prevent stale icon caching.
extensions/copilot/src/extension/inlineEdits/vscode-node/parts/inlineEditLogger.ts Removes static add/snapshot logging path; keeps only live entry logging.
extensions/copilot/src/extension/inlineEdits/vscode-node/inlineCompletionProvider.ts Removes now-invalid static logger add call; relies on existing live logging.
extensions/copilot/src/extension/completions-core/vscode-node/extension/src/vscodeInlineCompletionItemProvider.ts Switches ghost text logging to addLive() and completes the lifecycle with markCompleted().

Copilot's findings

  • Files reviewed: 5/5 changed files
  • Comments generated: 1

Comment thread extensions/copilot/src/platform/inlineEdits/common/inlineEditLogContext.ts Outdated
Cancelled requests should show a terminal icon, not a spinner.
Map 'cancelled' to Icon.skipped alongside 'skipped' to avoid
frozen-spinner appearance in the log tree.
@ulugbekna ulugbekna merged commit 286d98a into main Apr 9, 2026
23 checks passed
@ulugbekna ulugbekna deleted the ulugbekna/nes-log-context-refactor branch April 9, 2026 12:02
@vs-code-engineering vs-code-engineering bot added this to the 1.116.0 milestone Apr 9, 2026
joshspicer pushed a commit that referenced this pull request Apr 9, 2026
* nes: replace implicit _icon state with explicit outcome state machine (#308719)

Replace the implicit \`_icon\` field with an explicit \`_outcome: LogContextOutcome\`
discriminated union in InlineEditRequestLogContext.

- Define LogContextOutcome type with 8 states: pending, succeeded,
  noSuggestions, cached, cachedFromGhostText, skipped, cancelled, errored
- Derive icon from outcome via switch in _resolveIcon()
- Keep _isCompleted orthogonal (lifecycle flag, not an outcome)
- Add _setOutcome() with debug warning on invalid re-transitions
- Refactor setIsCachedResult to use direct field copy (avoids
  triggering outcome transitions during bulk data inheritance)
- Add double-completion warning in markCompleted()
- No caller changes required — public method signatures unchanged

* nes: unify live and static log entry paths (#308720, #308722)

Remove the static \`add()\` path from InlineEditLogger — all log entries
now use the live pattern with callbacks for icon/content resolution.

- Remove InlineEditLogger.add() and _liveRequestIds tracking
- Ghost text provider now uses addLive() + markCompleted() in finally
  block, ensuring lifecycle is always closed
- Remove redundant add() call in NES inlineCompletionProvider (the live
  entry was already registered via addLive())

This eliminates the static snapshot path that was the source of frozen
spinner bugs, and ensures ghost text entries have proper lifecycle
management matching the NES pattern.

* nes: fix stale icon caching in ChatPromptItem tree nodes (#308721)

Store the main entry reference in ChatPromptItem and always resolve
the icon unconditionally from it, preventing stale icon snapshots.

- Add _mainEntryRef field to ChatPromptItem
- setMainEntry() always sets iconPath (even to undefined when icon
  resolves to undefined), clearing any previous stale value
- withFilteredChildren() re-resolves from the entry ref via
  setMainEntry() instead of copying a potentially stale iconPath

This makes stale icons structurally impossible — the icon is always
derived from the live entry state on each tree refresh.

* nes: map cancelled outcome to skipped icon instead of loading

Cancelled requests should show a terminal icon, not a spinner.
Map 'cancelled' to Icon.skipped alongside 'skipped' to avoid
frozen-spinner appearance in the log tree.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment