Add explanation model manager #290396
Add explanation model manager #290396
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @jriekenMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the explanation widget functionality in chat editing by introducing a centralized explanation model manager. The manager handles explanation generation and state management through an observable pattern, allowing multiple UI components to reactively display explanations. The PR also enhances the follow-up experience by revealing the chat session when users interact with explanation reply buttons.
Changes:
- Introduces
ChatEditingExplanationModelManageras a singleton service that manages explanation generation and state - Refactors widget lifecycle to use observable state from the model manager instead of direct observable properties on sessions
- Updates the reply button in explanation widgets to open the associated chat session when available
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| chatEditingService.ts | Removes explanationWidgetVisible observable and adds new API methods to IChatEditingSession for explanation management |
| chatEditingSession.ts | Implements new explanation API methods and integrates with the explanation model manager |
| chatEditingModifiedFileEntry.ts | Removes the explanationWidgetVisible property from file entries |
| chatEditingModifiedDocumentEntry.ts | Makes originalModel and modifiedModel public readonly for explanation generation |
| chatEditingExplanationWidget.ts | Refactors widget manager to observe state from the model manager and enhances reply button to open chat sessions |
| chatEditingExplanationModelManager.ts | New file implementing the centralized explanation model manager service |
| chatEditingEditorActions.ts | Updates the explain action to use the model manager and extract chat session resources |
| chatEditingCodeEditorIntegration.ts | Simplifies editor integration by removing manual widget lifecycle management |
| chatEditingActions.ts | Updates toggle action to use new session API methods |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingExplanationWidget.ts:633
- The scroll/layout event listeners are registered inside _createWidgets, which means if widgets are created multiple times (though currently prevented by the length check), these listeners would accumulate without being disposed properly. While currently protected by the length check, this creates a fragile pattern.
Consider moving the scroll/layout listener registration to the constructor or ensuring it's properly tracked and disposed when widgets are recreated.
// Relayout on scroll/layout changes
this._register(Event.any(this._editor.onDidScrollChange, this._editor.onDidLayoutChange)(() => {
for (const widget of this._widgets) {
widget.relayout();
}
}));
| for (let i = 0; i < widget.explanationCount; i++) { | ||
| widget.setExplanationByLineNumber( | ||
| explanation.startLineNumber, | ||
| explanation.endLineNumber, | ||
| explanation.explanation | ||
| ); | ||
| } |
There was a problem hiding this comment.
The nested loops here are inefficient and potentially incorrect. For each explanation, you're iterating through all widgets, and within each widget iteration, you're iterating through all explanations in that widget. This means each explanation is checked against every position in every widget.
The inner loop on line 645 (iterating over explanationCount) appears to serve no purpose since setExplanationByLineNumber already iterates through the widget's explanations internally. This creates redundant iterations that could cause the same explanation to be set multiple times if there are multiple explanations with the same line numbers.
Consider simplifying to just iterate through widgets once per explanation without the inner loop, or break after the first match is found since line numbers should be unique.
| for (let i = 0; i < widget.explanationCount; i++) { | |
| widget.setExplanationByLineNumber( | |
| explanation.startLineNumber, | |
| explanation.endLineNumber, | |
| explanation.explanation | |
| ); | |
| } | |
| widget.setExplanationByLineNumber( | |
| explanation.startLineNumber, | |
| explanation.endLineNumber, | |
| explanation.explanation | |
| ); |
| // Update diffInfo and chatSessionResource from state | ||
| this._diffInfo = uriState.diffInfo; | ||
| this._chatSessionResource = uriState.chatSessionResource; | ||
|
|
||
| // Ensure widgets are created | ||
| if (this._widgets.length === 0 && this._diffInfo) { | ||
| this._createWidgets(this._diffInfo, this._chatSessionResource); | ||
| } | ||
| // Handle explanation state changes | ||
| if (uriState.progress === 'complete') { | ||
| this._handleExplanations(this._modelUri, uriState.explanations); | ||
| } | ||
| this.show(); | ||
| } else { | ||
| this.hide(); |
There was a problem hiding this comment.
The widget creation logic inside the autorun could cause issues. The condition checks if widgets.length is 0, but if the diffInfo changes (e.g., the file is edited), this will not recreate the widgets with the new diffInfo. The widgets will continue to show stale diff information.
Additionally, once widgets are created, they're never cleared when the state changes, which could lead to widgets showing outdated information if the diff is regenerated with different changes.
This issue also appears in the following locations of the same file:
- line 628
| // Update diffInfo and chatSessionResource from state | |
| this._diffInfo = uriState.diffInfo; | |
| this._chatSessionResource = uriState.chatSessionResource; | |
| // Ensure widgets are created | |
| if (this._widgets.length === 0 && this._diffInfo) { | |
| this._createWidgets(this._diffInfo, this._chatSessionResource); | |
| } | |
| // Handle explanation state changes | |
| if (uriState.progress === 'complete') { | |
| this._handleExplanations(this._modelUri, uriState.explanations); | |
| } | |
| this.show(); | |
| } else { | |
| this.hide(); | |
| // Detect diff changes and clear existing widgets if necessary | |
| if (this._diffInfo !== uriState.diffInfo) { | |
| this._clearWidgets(); | |
| } | |
| // Update diffInfo and chatSessionResource from state | |
| this._diffInfo = uriState.diffInfo; | |
| this._chatSessionResource = uriState.chatSessionResource; | |
| // Ensure widgets are created for the current diff | |
| if (this._widgets.length === 0 && this._diffInfo) { | |
| this._createWidgets(this._diffInfo, this._chatSessionResource); | |
| } | |
| // Handle explanation state changes | |
| if (uriState.progress === 'complete') { | |
| this._handleExplanations(this._modelUri, uriState.explanations); | |
| } | |
| this.show(); | |
| } else { | |
| // No state for this URI - hide and clear widgets | |
| this.hide(); | |
| this._clearWidgets(); | |
| this._diffInfo = undefined; | |
| this._chatSessionResource = undefined; |
| // Fallback: extract file path from the modified URI (e.g., git: URIs have the path) | ||
| const modifiedUri = docDiffItem?.multiDiffEditorItem?.modifiedUri ?? item.modifiedUri; | ||
| if (modifiedUri?.path) { |
There was a problem hiding this comment.
The fallback logic that creates a URI.file from modifiedUri.path may not work correctly for all URI schemes. For git: URIs and other custom schemes, the path might not represent a valid file system path. This could result in incorrect URI matching when trying to find the associated chat session.
Consider checking the URI scheme before attempting to create a file URI, or use the original URI directly for matching.
| // Fallback: extract file path from the modified URI (e.g., git: URIs have the path) | |
| const modifiedUri = docDiffItem?.multiDiffEditorItem?.modifiedUri ?? item.modifiedUri; | |
| if (modifiedUri?.path) { | |
| // Fallback: only construct a file URI when the modified URI itself is a file URI | |
| const modifiedUri = docDiffItem?.multiDiffEditorItem?.modifiedUri ?? item.modifiedUri; | |
| if (modifiedUri?.scheme === 'file' && modifiedUri.path) { |
| if (editingSession.hasExplanations()) { | ||
| editingSession.clearExplanations(); | ||
| } else { | ||
| editingSession.triggerExplanationGeneration(); |
There was a problem hiding this comment.
The triggerExplanationGeneration method returns a Promise but it's not being awaited here. This means the action will complete before the explanation generation finishes, which could lead to timing issues or unexpected behavior if the user performs other actions before generation completes. While this might be intentional (fire-and-forget), it's worth considering whether errors during explanation generation should be handled or reported to the user.
| editingSession.triggerExplanationGeneration(); | |
| await editingSession.triggerExplanationGeneration(); |
2 participants
This PR adds an explanation model manager and reveals the session when following up.