mirror of
https://github.com/microsoft/vscode.git
synced 2026-07-07 15:26:49 +01:00
Agent feedback improvements (#300202)
* agent feedback improvements * fix test
This commit is contained in:
committed by
GitHub
parent
c3788b7bbe
commit
01669c2233
@@ -13,6 +13,7 @@ import { URI } from '../../../../base/common/uri.js';
|
||||
import { isEqual } from '../../../../base/common/resources.js';
|
||||
import { EditorsOrder, IEditorIdentifier } from '../../../../workbench/common/editor.js';
|
||||
import { IEditorService } from '../../../../workbench/services/editor/common/editorService.js';
|
||||
import { GroupsOrder, IEditorGroupsService } from '../../../../workbench/services/editor/common/editorGroupsService.js';
|
||||
import { IChatWidgetService } from '../../../../workbench/contrib/chat/browser/chat.js';
|
||||
import { ChatContextKeys } from '../../../../workbench/contrib/chat/common/actions/chatContextKeys.js';
|
||||
import { CHAT_CATEGORY } from '../../../../workbench/contrib/chat/browser/actions/chatActions.js';
|
||||
@@ -48,7 +49,12 @@ abstract class AgentFeedbackEditorAction extends Action2 {
|
||||
const agentSessionsService = accessor.get(IAgentSessionsService);
|
||||
const codeReviewService = accessor.get(ICodeReviewService);
|
||||
|
||||
const candidates = getActiveResourceCandidates(editorService.activeEditorPane?.input);
|
||||
const editorGroupsService = accessor.get(IEditorGroupsService);
|
||||
|
||||
const activePane = editorService.activeEditorPane
|
||||
?? editorGroupsService.getGroups(GroupsOrder.MOST_RECENTLY_ACTIVE).find(g => g.activeEditorPane)?.activeEditorPane
|
||||
?? editorService.visibleEditorPanes[0];
|
||||
const candidates = getActiveResourceCandidates(activePane?.input);
|
||||
for (const candidate of candidates) {
|
||||
const sessionResource = getSessionForResource(candidate, chatEditingService, agentSessionsService)
|
||||
?? agentFeedbackService.getMostRecentSessionForResource(candidate);
|
||||
|
||||
+11
-22
@@ -31,6 +31,9 @@ import { ICodeReviewService } from '../../codeReview/browser/codeReviewService.j
|
||||
import { getResourceEditorComments, getSessionEditorComments, groupNearbySessionEditorComments, ISessionEditorComment, SessionEditorCommentSource, toSessionEditorCommentId } from './sessionEditorComments.js';
|
||||
import { ActionBar } from '../../../../base/browser/ui/actionbar/actionbar.js';
|
||||
import { isEqual } from '../../../../base/common/resources.js';
|
||||
import { IMarkdownRendererService } from '../../../../platform/markdown/browser/markdownRenderer.js';
|
||||
import { MarkdownString } from '../../../../base/common/htmlContent.js';
|
||||
import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js';
|
||||
|
||||
/**
|
||||
* Widget that displays agent feedback comments for a group of nearby feedback items.
|
||||
@@ -44,7 +47,6 @@ export class AgentFeedbackEditorWidget extends Disposable implements IOverlayWid
|
||||
private readonly _domNode: HTMLElement;
|
||||
private readonly _headerNode: HTMLElement;
|
||||
private readonly _titleNode: HTMLElement;
|
||||
private readonly _dismissButton: HTMLElement;
|
||||
private readonly _toggleButton: HTMLElement;
|
||||
private readonly _bodyNode: HTMLElement;
|
||||
private readonly _itemElements = new Map<string, HTMLElement>();
|
||||
@@ -63,6 +65,7 @@ export class AgentFeedbackEditorWidget extends Disposable implements IOverlayWid
|
||||
private readonly _sessionResource: URI,
|
||||
@IAgentFeedbackService private readonly _agentFeedbackService: IAgentFeedbackService,
|
||||
@ICodeReviewService private readonly _codeReviewService: ICodeReviewService,
|
||||
@IMarkdownRendererService private readonly _markdownRendererService: IMarkdownRendererService,
|
||||
) {
|
||||
super();
|
||||
|
||||
@@ -88,12 +91,6 @@ export class AgentFeedbackEditorWidget extends Disposable implements IOverlayWid
|
||||
this._updateToggleButton();
|
||||
this._headerNode.appendChild(this._toggleButton);
|
||||
|
||||
// Dismiss button
|
||||
this._dismissButton = $('div.agent-feedback-widget-dismiss');
|
||||
this._dismissButton.appendChild(renderIcon(Codicon.close));
|
||||
this._dismissButton.title = nls.localize('dismiss', "Dismiss");
|
||||
this._headerNode.appendChild(this._dismissButton);
|
||||
|
||||
this._domNode.appendChild(this._headerNode);
|
||||
|
||||
// Body (collapsible) — starts collapsed
|
||||
@@ -128,11 +125,6 @@ export class AgentFeedbackEditorWidget extends Disposable implements IOverlayWid
|
||||
this._toggleExpanded();
|
||||
}));
|
||||
|
||||
// Dismiss button click
|
||||
this._eventStore.add(addDisposableListener(this._dismissButton, 'click', (e) => {
|
||||
e.stopPropagation();
|
||||
this._dismiss();
|
||||
}));
|
||||
}
|
||||
|
||||
private _toggleExpanded(): void {
|
||||
@@ -143,12 +135,6 @@ export class AgentFeedbackEditorWidget extends Disposable implements IOverlayWid
|
||||
}
|
||||
}
|
||||
|
||||
private _dismiss(): void {
|
||||
for (const comment of this._commentItems) {
|
||||
this._removeComment(comment);
|
||||
}
|
||||
}
|
||||
|
||||
private _updateTitle(): void {
|
||||
const count = this._commentItems.length;
|
||||
if (count === 1) {
|
||||
@@ -206,7 +192,7 @@ export class AgentFeedbackEditorWidget extends Disposable implements IOverlayWid
|
||||
actionBar.push(new Action(
|
||||
'agentFeedback.widget.convert',
|
||||
nls.localize('convertComment', "Convert to Agent Feedback"),
|
||||
ThemeIcon.asClassName(Codicon.comment),
|
||||
ThemeIcon.asClassName(Codicon.check),
|
||||
true,
|
||||
() => this._convertToAgentFeedback(comment),
|
||||
), { icon: true, label: false });
|
||||
@@ -221,8 +207,10 @@ export class AgentFeedbackEditorWidget extends Disposable implements IOverlayWid
|
||||
itemHeader.appendChild(actionBarContainer);
|
||||
item.appendChild(itemHeader);
|
||||
|
||||
const text = $('span.agent-feedback-widget-text');
|
||||
text.textContent = comment.text;
|
||||
const text = $('div.agent-feedback-widget-text');
|
||||
const rendered = this._markdownRendererService.render(new MarkdownString(comment.text));
|
||||
this._eventStore.add(rendered);
|
||||
text.appendChild(rendered.element);
|
||||
item.appendChild(text);
|
||||
|
||||
if (comment.suggestion?.edits.length) {
|
||||
@@ -505,6 +493,7 @@ class AgentFeedbackEditorWidgetContribution extends Disposable implements IEdito
|
||||
@IChatEditingService private readonly _chatEditingService: IChatEditingService,
|
||||
@IAgentSessionsService private readonly _agentSessionsService: IAgentSessionsService,
|
||||
@ICodeReviewService private readonly _codeReviewService: ICodeReviewService,
|
||||
@IInstantiationService private readonly _instantiationService: IInstantiationService,
|
||||
) {
|
||||
super();
|
||||
|
||||
@@ -572,7 +561,7 @@ class AgentFeedbackEditorWidgetContribution extends Disposable implements IEdito
|
||||
const groups = groupNearbySessionEditorComments(fileComments, 5);
|
||||
|
||||
for (const group of groups) {
|
||||
const widget = new AgentFeedbackEditorWidget(this._editor, group, this._sessionResource, this._agentFeedbackService, this._codeReviewService);
|
||||
const widget = this._instantiationService.createInstance(AgentFeedbackEditorWidget, this._editor, group, this._sessionResource);
|
||||
this._widgets.push(widget);
|
||||
|
||||
widget.layout(group[0].range.startLineNumber);
|
||||
|
||||
@@ -79,6 +79,12 @@ export interface IAgentFeedbackService {
|
||||
*/
|
||||
revealFeedback(sessionResource: URI, feedbackId: string): Promise<void>;
|
||||
|
||||
/**
|
||||
* Open an editor for the given session comment (feedback or code-review) at its range
|
||||
* and set it as the navigation anchor.
|
||||
*/
|
||||
revealSessionComment(sessionResource: URI, commentId: string, resourceUri: URI, range: IRange): Promise<void>;
|
||||
|
||||
/**
|
||||
* Navigate to next/previous feedback item in a session.
|
||||
*/
|
||||
@@ -271,16 +277,19 @@ export class AgentFeedbackService extends Disposable implements IAgentFeedbackSe
|
||||
if (!feedback) {
|
||||
return;
|
||||
}
|
||||
await this.revealSessionComment(sessionResource, feedbackId, feedback.resourceUri, feedback.range);
|
||||
}
|
||||
|
||||
async revealSessionComment(sessionResource: URI, commentId: string, resourceUri: URI, range: IRange): Promise<void> {
|
||||
await this._editorService.openEditor({
|
||||
resource: feedback.resourceUri,
|
||||
resource: resourceUri,
|
||||
options: {
|
||||
preserveFocus: false,
|
||||
revealIfVisible: true,
|
||||
selection: { startLineNumber: range.startLineNumber, startColumn: range.startColumn },
|
||||
}
|
||||
});
|
||||
setTimeout(() => {
|
||||
this.setNavigationAnchor(sessionResource, feedbackId);
|
||||
}, 50); // delay to ensure editor has revealed the correct position before firing navigation event
|
||||
this.setNavigationAnchor(sessionResource, commentId);
|
||||
}
|
||||
|
||||
getNextFeedback(sessionResource: URI, next: boolean): IAgentFeedback | undefined {
|
||||
|
||||
@@ -8,7 +8,7 @@
|
||||
color: var(--vscode-foreground);
|
||||
background-color: var(--vscode-editorWidget-background);
|
||||
border-radius: 6px;
|
||||
border: 1px solid var(--vscode-contrastBorder);
|
||||
border: 1px solid var(--vscode-editorHoverWidget-border);
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
|
||||
@@ -113,24 +113,6 @@
|
||||
}
|
||||
|
||||
/* Dismiss button */
|
||||
.agent-feedback-widget-dismiss {
|
||||
display: flex;
|
||||
align-items: center;
|
||||
justify-content: center;
|
||||
width: 18px;
|
||||
height: 18px;
|
||||
border-radius: 4px;
|
||||
cursor: pointer;
|
||||
color: var(--vscode-foreground);
|
||||
opacity: 0.7;
|
||||
transition: opacity 0.1s;
|
||||
}
|
||||
|
||||
.agent-feedback-widget-dismiss:hover {
|
||||
opacity: 1;
|
||||
background-color: var(--vscode-toolbar-hoverBackground);
|
||||
}
|
||||
|
||||
/* Body - collapsible */
|
||||
.agent-feedback-widget-body {
|
||||
transition: max-height 0.2s ease-in-out, padding 0.2s ease-in-out;
|
||||
@@ -233,6 +215,18 @@
|
||||
word-wrap: break-word;
|
||||
}
|
||||
|
||||
.agent-feedback-widget-text .rendered-markdown p {
|
||||
margin: 0;
|
||||
}
|
||||
|
||||
.agent-feedback-widget-text .rendered-markdown code {
|
||||
font-family: var(--monaco-monospace-font);
|
||||
font-size: 11px;
|
||||
padding: 1px 4px;
|
||||
border-radius: 3px;
|
||||
background: color-mix(in srgb, var(--vscode-editorWidget-border, var(--vscode-widget-border)) 25%, transparent);
|
||||
}
|
||||
|
||||
.agent-feedback-widget-suggestion {
|
||||
display: flex;
|
||||
flex-direction: column;
|
||||
|
||||
+13
-4
@@ -6,6 +6,7 @@
|
||||
import { Event } from '../../../../../base/common/event.js';
|
||||
import { Color } from '../../../../../base/common/color.js';
|
||||
import { DisposableStore } from '../../../../../base/common/lifecycle.js';
|
||||
import { IMarkdownRendererService, MarkdownRendererService } from '../../../../../platform/markdown/browser/markdownRenderer.js';
|
||||
import { URI } from '../../../../../base/common/uri.js';
|
||||
import { mock } from '../../../../../base/test/common/mock.js';
|
||||
import { observableValue } from '../../../../../base/common/observable.js';
|
||||
@@ -182,7 +183,16 @@ function renderWidget(context: ComponentFixtureContext, options: IFixtureOptions
|
||||
|
||||
ensureTokenColorMap();
|
||||
|
||||
const instantiationService = createEditorServices(scopedDisposables, { colorTheme: context.theme });
|
||||
const agentFeedbackService = createMockAgentFeedbackService();
|
||||
const codeReviewService = createMockCodeReviewService();
|
||||
const instantiationService = createEditorServices(scopedDisposables, {
|
||||
colorTheme: context.theme,
|
||||
additionalServices: reg => {
|
||||
reg.defineInstance(IAgentFeedbackService, agentFeedbackService);
|
||||
reg.defineInstance(ICodeReviewService, codeReviewService);
|
||||
reg.define(IMarkdownRendererService, MarkdownRendererService);
|
||||
},
|
||||
});
|
||||
const model = scopedDisposables.add(createTextModel(instantiationService, sampleCode, fileResource, 'typescript'));
|
||||
|
||||
const editorOptions: ICodeEditorWidgetOptions = {
|
||||
@@ -205,12 +215,11 @@ function renderWidget(context: ComponentFixtureContext, options: IFixtureOptions
|
||||
|
||||
editor.setModel(model);
|
||||
|
||||
const widget = scopedDisposables.add(new AgentFeedbackEditorWidget(
|
||||
const widget = scopedDisposables.add(instantiationService.createInstance(
|
||||
AgentFeedbackEditorWidget,
|
||||
editor,
|
||||
options.commentItems,
|
||||
sessionResource,
|
||||
createMockAgentFeedbackService(),
|
||||
createMockCodeReviewService(),
|
||||
));
|
||||
|
||||
widget.layout(options.commentItems[0].range.startLineNumber);
|
||||
|
||||
@@ -749,6 +749,23 @@ export class ChangesViewPane extends ViewPane {
|
||||
sessionsChangedSignal.read(reader); // Re-evaluate when session metadata changes (e.g. pullRequestUrl)
|
||||
const menuId = isSessionMenu ? MenuId.ChatEditingSessionChangesToolbar : MenuId.ChatEditingWidgetToolbar;
|
||||
|
||||
// Read code review state to update the button label dynamically
|
||||
let codeReviewCommentCount: number | undefined;
|
||||
let codeReviewLoading = false;
|
||||
if (sessionResource) {
|
||||
const sessionChanges = this.agentSessionsService.getSession(sessionResource)?.changes;
|
||||
if (sessionChanges instanceof Array && sessionChanges.length > 0) {
|
||||
const reviewFiles = getCodeReviewFilesFromSessionChanges(sessionChanges as readonly IChatSessionFileChange[] | readonly IChatSessionFileChange2[]);
|
||||
const reviewVersion = getCodeReviewVersion(reviewFiles);
|
||||
const reviewState = this.codeReviewService.getReviewState(sessionResource).read(reader);
|
||||
if (reviewState.kind === CodeReviewStateKind.Loading && reviewState.version === reviewVersion) {
|
||||
codeReviewLoading = true;
|
||||
} else if (reviewState.kind === CodeReviewStateKind.Result && reviewState.version === reviewVersion && reviewState.comments.length > 0) {
|
||||
codeReviewCommentCount = reviewState.comments.length;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
reader.store.add(scopedInstantiationService.createInstance(
|
||||
MenuWorkbenchButtonBar,
|
||||
this.actionsContainer!,
|
||||
@@ -768,6 +785,12 @@ export class ChangesViewPane extends ViewPane {
|
||||
return { showIcon: true, showLabel: true, isSecondary: true, customClass: 'working-set-diff-stats', customLabel: diffStatsLabel };
|
||||
}
|
||||
if (action.id === RUN_SESSION_CODE_REVIEW_ACTION_ID) {
|
||||
if (codeReviewLoading) {
|
||||
return { showIcon: true, showLabel: true, isSecondary: true, customLabel: '$(loading~spin)', customClass: 'code-review-loading' };
|
||||
}
|
||||
if (codeReviewCommentCount !== undefined) {
|
||||
return { showIcon: true, showLabel: true, isSecondary: true, customLabel: String(codeReviewCommentCount), customClass: 'code-review-comments' };
|
||||
}
|
||||
return { showIcon: true, showLabel: false, isSecondary: true };
|
||||
}
|
||||
if (action.id === 'chatEditing.synchronizeChanges') {
|
||||
|
||||
@@ -274,3 +274,9 @@
|
||||
.changes-view-body .chat-editing-session-actions .monaco-button .working-set-lines-removed {
|
||||
color: var(--vscode-chat-linesRemovedForeground);
|
||||
}
|
||||
|
||||
.changes-view-body .chat-editing-session-actions .monaco-button.code-review-comments,
|
||||
.changes-view-body .chat-editing-session-actions .monaco-button.code-review-loading {
|
||||
padding-left: 4px;
|
||||
padding-right: 4px;
|
||||
}
|
||||
|
||||
@@ -20,6 +20,8 @@ import { CHAT_CATEGORY } from '../../../../workbench/contrib/chat/browser/action
|
||||
import { ISessionsManagementService } from '../../sessions/browser/sessionsManagementService.js';
|
||||
import { CodeReviewService, CodeReviewStateKind, getCodeReviewFilesFromSessionChanges, getCodeReviewVersion, ICodeReviewService } from './codeReviewService.js';
|
||||
import { ThemeIcon } from '../../../../base/common/themables.js';
|
||||
import { IAgentFeedbackService } from '../../agentFeedback/browser/agentFeedbackService.js';
|
||||
import { SessionEditorCommentSource, toSessionEditorCommentId } from '../../agentFeedback/browser/sessionEditorComments.js';
|
||||
|
||||
registerSingleton(ICodeReviewService, CodeReviewService, InstantiationType.Delayed);
|
||||
|
||||
@@ -55,6 +57,7 @@ function registerSessionCodeReviewAction(tooltip: string, icon: ThemeIcon): Disp
|
||||
const sessionManagementService = accessor.get(ISessionsManagementService);
|
||||
const agentSessionsService = accessor.get(IAgentSessionsService);
|
||||
const codeReviewService = accessor.get(ICodeReviewService);
|
||||
const agentFeedbackService = accessor.get(IAgentFeedbackService);
|
||||
|
||||
const resource = URI.isUri(sessionResource)
|
||||
? sessionResource
|
||||
@@ -72,6 +75,15 @@ function registerSessionCodeReviewAction(tooltip: string, icon: ThemeIcon): Disp
|
||||
const files = getCodeReviewFilesFromSessionChanges(session.changes);
|
||||
const version = getCodeReviewVersion(files);
|
||||
|
||||
// If a review already exists with comments, navigate to the first comment
|
||||
const reviewState = codeReviewService.getReviewState(resource).get();
|
||||
if (reviewState.kind === CodeReviewStateKind.Result && reviewState.version === version && reviewState.comments.length > 0) {
|
||||
const firstComment = reviewState.comments[0];
|
||||
const commentId = toSessionEditorCommentId(SessionEditorCommentSource.CodeReview, firstComment.id);
|
||||
await agentFeedbackService.revealSessionComment(resource, commentId, firstComment.uri, firstComment.range);
|
||||
return;
|
||||
}
|
||||
|
||||
codeReviewService.requestReview(resource, version, files);
|
||||
}
|
||||
}
|
||||
@@ -126,13 +138,14 @@ class CodeReviewToolbarContribution extends Disposable implements IWorkbenchCont
|
||||
if (reviewState.kind === CodeReviewStateKind.Loading && reviewState.version === version) {
|
||||
canRunCodeReview = false;
|
||||
tooltip = localize('sessions.runCodeReview.tooltip.loading', "Creating code review...");
|
||||
icon = Codicon.commentDraft;
|
||||
icon = Codicon.codeReview;
|
||||
} else if (reviewState.kind === CodeReviewStateKind.Result && reviewState.version === version) {
|
||||
canRunCodeReview = false;
|
||||
if (reviewState.comments.length === 0) {
|
||||
canRunCodeReview = false;
|
||||
tooltip = localize('sessions.runCodeReview.tooltip.allResolved', "All review comments have been addressed.");
|
||||
icon = Codicon.comment;
|
||||
} else {
|
||||
canRunCodeReview = true;
|
||||
icon = Codicon.commentUnresolved;
|
||||
tooltip = reviewState.comments.length === 1
|
||||
? localize('sessions.runCodeReview.tooltip.oneUnresolved', "1 review comment unresolved.")
|
||||
|
||||
@@ -9,6 +9,8 @@ import { URI, UriComponents } from '../../../../base/common/uri.js';
|
||||
import { IRange, Range } from '../../../../editor/common/core/range.js';
|
||||
import { createDecorator } from '../../../../platform/instantiation/common/instantiation.js';
|
||||
import { ICommandService } from '../../../../platform/commands/common/commands.js';
|
||||
import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js';
|
||||
import { IAgentSessionsService } from '../../../../workbench/contrib/chat/browser/agentSessions/agentSessionsService.js';
|
||||
import { generateUuid } from '../../../../base/common/uuid.js';
|
||||
import { hash } from '../../../../base/common/hash.js';
|
||||
import { hasKey } from '../../../../base/common/types.js';
|
||||
@@ -156,6 +158,23 @@ export interface ICodeReviewService {
|
||||
dismissReview(sessionResource: URI): void;
|
||||
}
|
||||
|
||||
// --- Storage Types -----------------------------------------------------------
|
||||
|
||||
interface IStoredCodeReview {
|
||||
readonly version: string;
|
||||
readonly comments: readonly IStoredCodeReviewComment[];
|
||||
}
|
||||
|
||||
interface IStoredCodeReviewComment {
|
||||
readonly id: string;
|
||||
readonly uri: UriComponents;
|
||||
readonly range: IRange;
|
||||
readonly body: string;
|
||||
readonly kind: string;
|
||||
readonly severity: string;
|
||||
readonly suggestion?: ICodeReviewSuggestion;
|
||||
}
|
||||
|
||||
// --- Implementation ----------------------------------------------------------
|
||||
|
||||
interface ISessionReviewData {
|
||||
@@ -225,12 +244,18 @@ export class CodeReviewService extends Disposable implements ICodeReviewService
|
||||
|
||||
declare readonly _serviceBrand: undefined;
|
||||
|
||||
private static readonly _STORAGE_KEY = 'codeReview.reviews';
|
||||
|
||||
private readonly _reviewsBySession = new Map<string, ISessionReviewData>();
|
||||
|
||||
constructor(
|
||||
@ICommandService private readonly _commandService: ICommandService,
|
||||
@IStorageService private readonly _storageService: IStorageService,
|
||||
@IAgentSessionsService private readonly _agentSessionsService: IAgentSessionsService,
|
||||
) {
|
||||
super();
|
||||
this._loadFromStorage();
|
||||
this._registerSessionListeners();
|
||||
}
|
||||
|
||||
getReviewState(sessionResource: URI): IObservable<ICodeReviewState> {
|
||||
@@ -276,12 +301,14 @@ export class CodeReviewService extends Disposable implements ICodeReviewService
|
||||
|
||||
const filtered = state.comments.filter(c => c.id !== commentId);
|
||||
data.state.set({ kind: CodeReviewStateKind.Result, version: state.version, comments: filtered }, undefined);
|
||||
this._saveToStorage();
|
||||
}
|
||||
|
||||
dismissReview(sessionResource: URI): void {
|
||||
const data = this._reviewsBySession.get(sessionResource.toString());
|
||||
if (data) {
|
||||
data.state.set({ kind: CodeReviewStateKind.Idle }, undefined);
|
||||
this._saveToStorage();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -342,6 +369,7 @@ export class CodeReviewService extends Disposable implements ICodeReviewService
|
||||
transaction(tx => {
|
||||
data.state.set({ kind: CodeReviewStateKind.Result, version, comments }, tx);
|
||||
});
|
||||
this._saveToStorage();
|
||||
}
|
||||
} catch (err) {
|
||||
const currentState = data.state.get();
|
||||
@@ -350,4 +378,109 @@ export class CodeReviewService extends Disposable implements ICodeReviewService
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private _loadFromStorage(): void {
|
||||
const raw = this._storageService.get(CodeReviewService._STORAGE_KEY, StorageScope.WORKSPACE);
|
||||
if (!raw) {
|
||||
return;
|
||||
}
|
||||
|
||||
try {
|
||||
const stored: Record<string, IStoredCodeReview> = JSON.parse(raw);
|
||||
for (const [key, review] of Object.entries(stored)) {
|
||||
const comments: ICodeReviewComment[] = review.comments.map(c => ({
|
||||
id: c.id,
|
||||
uri: URI.revive(c.uri),
|
||||
range: c.range,
|
||||
body: c.body,
|
||||
kind: c.kind,
|
||||
severity: c.severity,
|
||||
suggestion: c.suggestion,
|
||||
}));
|
||||
const data = this._getOrCreateData(URI.parse(key));
|
||||
data.state.set({ kind: CodeReviewStateKind.Result, version: review.version, comments }, undefined);
|
||||
}
|
||||
} catch {
|
||||
// Corrupted storage data — ignore
|
||||
}
|
||||
}
|
||||
|
||||
private _saveToStorage(): void {
|
||||
const stored: Record<string, IStoredCodeReview> = {};
|
||||
for (const [key, data] of this._reviewsBySession) {
|
||||
const state = data.state.get();
|
||||
if (state.kind === CodeReviewStateKind.Result) {
|
||||
stored[key] = {
|
||||
version: state.version,
|
||||
comments: state.comments.map(c => ({
|
||||
id: c.id,
|
||||
uri: c.uri.toJSON(),
|
||||
range: c.range,
|
||||
body: c.body,
|
||||
kind: c.kind,
|
||||
severity: c.severity,
|
||||
suggestion: c.suggestion,
|
||||
})),
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
if (Object.keys(stored).length === 0) {
|
||||
this._storageService.remove(CodeReviewService._STORAGE_KEY, StorageScope.WORKSPACE);
|
||||
} else {
|
||||
this._storageService.store(CodeReviewService._STORAGE_KEY, JSON.stringify(stored), StorageScope.WORKSPACE, StorageTarget.MACHINE);
|
||||
}
|
||||
}
|
||||
|
||||
private _registerSessionListeners(): void {
|
||||
// Clean up when a session is archived
|
||||
this._register(this._agentSessionsService.onDidChangeSessionArchivedState(session => {
|
||||
if (session.isArchived()) {
|
||||
const key = session.resource.toString();
|
||||
const data = this._reviewsBySession.get(key);
|
||||
if (data) {
|
||||
data.state.set({ kind: CodeReviewStateKind.Idle }, undefined);
|
||||
this._saveToStorage();
|
||||
}
|
||||
}
|
||||
}));
|
||||
|
||||
// Clean up when session changes make a review version outdated
|
||||
this._register(this._agentSessionsService.model.onDidChangeSessions(() => {
|
||||
let changed = false;
|
||||
for (const [key, data] of this._reviewsBySession) {
|
||||
const state = data.state.get();
|
||||
if (state.kind !== CodeReviewStateKind.Result) {
|
||||
continue;
|
||||
}
|
||||
|
||||
const session = this._agentSessionsService.getSession(URI.parse(key));
|
||||
if (!session) {
|
||||
// Session no longer exists — clean up
|
||||
data.state.set({ kind: CodeReviewStateKind.Idle }, undefined);
|
||||
changed = true;
|
||||
continue;
|
||||
}
|
||||
|
||||
if (!(session.changes instanceof Array) || session.changes.length === 0) {
|
||||
// Session has no file-level changes — clean up
|
||||
data.state.set({ kind: CodeReviewStateKind.Idle }, undefined);
|
||||
changed = true;
|
||||
continue;
|
||||
}
|
||||
|
||||
const files = getCodeReviewFilesFromSessionChanges(session.changes);
|
||||
const currentVersion = getCodeReviewVersion(files);
|
||||
if (state.version !== currentVersion) {
|
||||
// Version mismatch — review is stale
|
||||
data.state.set({ kind: CodeReviewStateKind.Idle }, undefined);
|
||||
changed = true;
|
||||
}
|
||||
}
|
||||
|
||||
if (changed) {
|
||||
this._saveToStorage();
|
||||
}
|
||||
}));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -10,14 +10,21 @@ import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/tes
|
||||
import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js';
|
||||
import { DisposableStore } from '../../../../../base/common/lifecycle.js';
|
||||
import { ICommandService } from '../../../../../platform/commands/common/commands.js';
|
||||
import { Event } from '../../../../../base/common/event.js';
|
||||
import { CodeReviewService, CodeReviewStateKind, ICodeReviewService } from '../../browser/codeReviewService.js';
|
||||
import { Emitter, Event } from '../../../../../base/common/event.js';
|
||||
import { InMemoryStorageService, IStorageService, StorageScope, StorageTarget } from '../../../../../platform/storage/common/storage.js';
|
||||
import { IAgentSessionsService } from '../../../../../workbench/contrib/chat/browser/agentSessions/agentSessionsService.js';
|
||||
import { IAgentSession, IAgentSessionsModel } from '../../../../../workbench/contrib/chat/browser/agentSessions/agentSessionsModel.js';
|
||||
import { IChatSessionFileChange2 } from '../../../../../workbench/contrib/chat/common/chatSessionsService.js';
|
||||
import { CodeReviewService, CodeReviewStateKind, getCodeReviewFilesFromSessionChanges, getCodeReviewVersion, ICodeReviewService } from '../../browser/codeReviewService.js';
|
||||
|
||||
suite('CodeReviewService', () => {
|
||||
|
||||
const store = new DisposableStore();
|
||||
let instantiationService: TestInstantiationService;
|
||||
let service: ICodeReviewService;
|
||||
let commandService: MockCommandService;
|
||||
let storageService: InMemoryStorageService;
|
||||
let agentSessionsService: MockAgentSessionsService;
|
||||
|
||||
let session: URI;
|
||||
let fileA: URI;
|
||||
@@ -87,12 +94,81 @@ suite('CodeReviewService', () => {
|
||||
}
|
||||
}
|
||||
|
||||
class MockAgentSessionsService {
|
||||
declare readonly _serviceBrand: undefined;
|
||||
|
||||
private readonly _onDidChangeSessionArchivedState: Emitter<IAgentSession>;
|
||||
readonly onDidChangeSessionArchivedState: Event<IAgentSession>;
|
||||
private readonly _onDidChangeSessions: Emitter<void>;
|
||||
readonly model: IAgentSessionsModel;
|
||||
private readonly _sessions = new Map<string, IAgentSession>();
|
||||
|
||||
constructor(disposables: DisposableStore) {
|
||||
this._onDidChangeSessionArchivedState = disposables.add(new Emitter<IAgentSession>());
|
||||
this.onDidChangeSessionArchivedState = this._onDidChangeSessionArchivedState.event;
|
||||
this._onDidChangeSessions = disposables.add(new Emitter<void>());
|
||||
this.model = {
|
||||
onWillResolve: Event.None,
|
||||
onDidResolve: Event.None,
|
||||
onDidChangeSessions: this._onDidChangeSessions.event,
|
||||
onDidChangeSessionArchivedState: this._onDidChangeSessionArchivedState.event,
|
||||
resolved: true,
|
||||
sessions: [],
|
||||
getSession: (resource: URI) => this._sessions.get(resource.toString()),
|
||||
resolve: async () => { },
|
||||
};
|
||||
}
|
||||
|
||||
getSession(resource: URI): IAgentSession | undefined {
|
||||
return this._sessions.get(resource.toString());
|
||||
}
|
||||
|
||||
setSession(resource: URI, changes?: readonly IChatSessionFileChange2[], archived = false): IAgentSession {
|
||||
let _archived = archived;
|
||||
const session = {
|
||||
resource,
|
||||
changes,
|
||||
isArchived: () => _archived,
|
||||
setArchived: (v: boolean) => { _archived = v; },
|
||||
isRead: () => true,
|
||||
setRead: () => { },
|
||||
} as unknown as IAgentSession;
|
||||
this._sessions.set(resource.toString(), session);
|
||||
return session;
|
||||
}
|
||||
|
||||
updateSessionChanges(resource: URI, changes: readonly IChatSessionFileChange2[] | undefined): void {
|
||||
const session = this._sessions.get(resource.toString()) as Record<string, unknown> | undefined;
|
||||
if (session) {
|
||||
session.changes = changes;
|
||||
}
|
||||
}
|
||||
|
||||
removeSession(resource: URI): void {
|
||||
this._sessions.delete(resource.toString());
|
||||
}
|
||||
|
||||
fireSessionArchivedState(session: IAgentSession): void {
|
||||
this._onDidChangeSessionArchivedState.fire(session);
|
||||
}
|
||||
|
||||
fireSessionsChanged(): void {
|
||||
this._onDidChangeSessions.fire();
|
||||
}
|
||||
}
|
||||
|
||||
setup(() => {
|
||||
const instantiationService = store.add(new TestInstantiationService());
|
||||
instantiationService = store.add(new TestInstantiationService());
|
||||
|
||||
commandService = new MockCommandService();
|
||||
instantiationService.stub(ICommandService, commandService);
|
||||
|
||||
storageService = store.add(new InMemoryStorageService());
|
||||
instantiationService.stub(IStorageService, storageService);
|
||||
|
||||
agentSessionsService = new MockAgentSessionsService(store);
|
||||
instantiationService.stub(IAgentSessionsService, agentSessionsService);
|
||||
|
||||
service = store.add(instantiationService.createInstance(CodeReviewService));
|
||||
session = URI.parse('test://session/1');
|
||||
fileA = URI.parse('file:///a.ts');
|
||||
@@ -654,6 +730,220 @@ suite('CodeReviewService', () => {
|
||||
CodeReviewStateKind.Idle,
|
||||
]);
|
||||
});
|
||||
|
||||
// --- Storage persistence ---
|
||||
|
||||
test('review results are persisted to storage', async () => {
|
||||
commandService.result = {
|
||||
type: 'success',
|
||||
comments: [{ uri: fileA, range: new Range(1, 1, 5, 1), body: 'Persisted comment', kind: 'bug', severity: 'high' }],
|
||||
};
|
||||
service.requestReview(session, 'v1', [{ currentUri: fileA }]);
|
||||
await tick();
|
||||
|
||||
const raw = storageService.get('codeReview.reviews', StorageScope.WORKSPACE);
|
||||
assert.ok(raw, 'Storage should contain review data');
|
||||
const stored = JSON.parse(raw!);
|
||||
const reviewData = stored[session.toString()];
|
||||
assert.ok(reviewData);
|
||||
assert.strictEqual(reviewData.version, 'v1');
|
||||
assert.strictEqual(reviewData.comments.length, 1);
|
||||
assert.strictEqual(reviewData.comments[0].body, 'Persisted comment');
|
||||
});
|
||||
|
||||
test('reviews are restored from storage on service creation', async () => {
|
||||
commandService.result = {
|
||||
type: 'success',
|
||||
comments: [{ uri: fileA, range: new Range(1, 1, 5, 1), body: 'Restored comment', kind: 'bug', severity: 'high' }],
|
||||
};
|
||||
service.requestReview(session, 'v1', [{ currentUri: fileA }]);
|
||||
await tick();
|
||||
|
||||
// Create a second service with the same storage
|
||||
const service2 = store.add(instantiationService.createInstance(CodeReviewService));
|
||||
const state = service2.getReviewState(session).get();
|
||||
assert.strictEqual(state.kind, CodeReviewStateKind.Result);
|
||||
if (state.kind === CodeReviewStateKind.Result) {
|
||||
assert.strictEqual(state.version, 'v1');
|
||||
assert.strictEqual(state.comments.length, 1);
|
||||
assert.strictEqual(state.comments[0].body, 'Restored comment');
|
||||
assert.strictEqual(state.comments[0].uri.toString(), fileA.toString());
|
||||
assert.deepStrictEqual(state.comments[0].range, { startLineNumber: 1, startColumn: 1, endLineNumber: 5, endColumn: 1 });
|
||||
}
|
||||
});
|
||||
|
||||
test('suggestions are persisted and restored correctly', async () => {
|
||||
commandService.result = {
|
||||
type: 'success',
|
||||
comments: [{
|
||||
uri: fileA,
|
||||
range: new Range(1, 1, 5, 1),
|
||||
body: 'suggestion comment',
|
||||
suggestion: {
|
||||
edits: [{
|
||||
range: new Range(2, 1, 3, 10),
|
||||
oldText: 'let x = 1;',
|
||||
newText: 'const x = 1;',
|
||||
}],
|
||||
},
|
||||
}],
|
||||
};
|
||||
service.requestReview(session, 'v1', [{ currentUri: fileA }]);
|
||||
await tick();
|
||||
|
||||
const service2 = store.add(instantiationService.createInstance(CodeReviewService));
|
||||
const state = service2.getReviewState(session).get();
|
||||
assert.strictEqual(state.kind, CodeReviewStateKind.Result);
|
||||
if (state.kind === CodeReviewStateKind.Result) {
|
||||
assert.strictEqual(state.comments[0].suggestion?.edits.length, 1);
|
||||
assert.strictEqual(state.comments[0].suggestion?.edits[0].oldText, 'let x = 1;');
|
||||
assert.strictEqual(state.comments[0].suggestion?.edits[0].newText, 'const x = 1;');
|
||||
}
|
||||
});
|
||||
|
||||
test('removeComment updates storage', async () => {
|
||||
commandService.result = {
|
||||
type: 'success',
|
||||
comments: [
|
||||
{ uri: fileA, range: new Range(1, 1, 1, 1), body: 'comment1' },
|
||||
{ uri: fileA, range: new Range(5, 1, 5, 1), body: 'comment2' },
|
||||
],
|
||||
};
|
||||
service.requestReview(session, 'v1', [{ currentUri: fileA }]);
|
||||
await tick();
|
||||
|
||||
const state = service.getReviewState(session).get();
|
||||
if (state.kind !== CodeReviewStateKind.Result) { return; }
|
||||
|
||||
service.removeComment(session, state.comments[0].id);
|
||||
|
||||
const raw = storageService.get('codeReview.reviews', StorageScope.WORKSPACE);
|
||||
const stored = JSON.parse(raw!);
|
||||
assert.strictEqual(stored[session.toString()].comments.length, 1);
|
||||
assert.strictEqual(stored[session.toString()].comments[0].body, 'comment2');
|
||||
});
|
||||
|
||||
test('dismissReview removes session from storage', async () => {
|
||||
commandService.result = { type: 'success', comments: [{ uri: fileA, range: new Range(1, 1, 1, 1), body: 'c' }] };
|
||||
service.requestReview(session, 'v1', [{ currentUri: fileA }]);
|
||||
await tick();
|
||||
|
||||
assert.ok(storageService.get('codeReview.reviews', StorageScope.WORKSPACE));
|
||||
|
||||
service.dismissReview(session);
|
||||
|
||||
assert.strictEqual(storageService.get('codeReview.reviews', StorageScope.WORKSPACE), undefined);
|
||||
});
|
||||
|
||||
test('corrupted storage is handled gracefully', () => {
|
||||
storageService.store('codeReview.reviews', 'not-valid-json{{{', StorageScope.WORKSPACE, StorageTarget.MACHINE);
|
||||
|
||||
const service2 = store.add(instantiationService.createInstance(CodeReviewService));
|
||||
const state = service2.getReviewState(session).get();
|
||||
assert.strictEqual(state.kind, CodeReviewStateKind.Idle);
|
||||
});
|
||||
|
||||
// --- Session lifecycle cleanup ---
|
||||
|
||||
test('archived session reviews are cleaned up', async () => {
|
||||
commandService.result = { type: 'success', comments: [{ uri: fileA, range: new Range(1, 1, 1, 1), body: 'comment' }] };
|
||||
service.requestReview(session, 'v1', [{ currentUri: fileA }]);
|
||||
await tick();
|
||||
|
||||
assert.strictEqual(service.getReviewState(session).get().kind, CodeReviewStateKind.Result);
|
||||
|
||||
const mockSession = agentSessionsService.setSession(session, undefined, true);
|
||||
agentSessionsService.fireSessionArchivedState(mockSession);
|
||||
|
||||
assert.strictEqual(service.getReviewState(session).get().kind, CodeReviewStateKind.Idle);
|
||||
assert.strictEqual(storageService.get('codeReview.reviews', StorageScope.WORKSPACE), undefined);
|
||||
});
|
||||
|
||||
test('non-archived session change does not clean up review', async () => {
|
||||
commandService.result = { type: 'success', comments: [{ uri: fileA, range: new Range(1, 1, 1, 1), body: 'comment' }] };
|
||||
service.requestReview(session, 'v1', [{ currentUri: fileA }]);
|
||||
await tick();
|
||||
|
||||
const mockSession = agentSessionsService.setSession(session, undefined, false);
|
||||
agentSessionsService.fireSessionArchivedState(mockSession);
|
||||
|
||||
assert.strictEqual(service.getReviewState(session).get().kind, CodeReviewStateKind.Result);
|
||||
});
|
||||
|
||||
test('session with changed version has review cleaned up', async () => {
|
||||
const changes: IChatSessionFileChange2[] = [
|
||||
{ uri: fileA, modifiedUri: fileA, insertions: 1, deletions: 0 },
|
||||
];
|
||||
agentSessionsService.setSession(session, changes);
|
||||
|
||||
const files = getCodeReviewFilesFromSessionChanges(changes);
|
||||
const version = getCodeReviewVersion(files);
|
||||
|
||||
commandService.result = { type: 'success', comments: [{ uri: fileA, range: new Range(1, 1, 1, 1), body: 'stale comment' }] };
|
||||
service.requestReview(session, version, files);
|
||||
await tick();
|
||||
|
||||
assert.strictEqual(service.getReviewState(session).get().kind, CodeReviewStateKind.Result);
|
||||
|
||||
const newChanges: IChatSessionFileChange2[] = [
|
||||
{ uri: fileA, modifiedUri: fileA, insertions: 1, deletions: 0 },
|
||||
{ uri: fileB, modifiedUri: fileB, insertions: 2, deletions: 0 },
|
||||
];
|
||||
agentSessionsService.updateSessionChanges(session, newChanges);
|
||||
agentSessionsService.fireSessionsChanged();
|
||||
|
||||
assert.strictEqual(service.getReviewState(session).get().kind, CodeReviewStateKind.Idle);
|
||||
assert.strictEqual(storageService.get('codeReview.reviews', StorageScope.WORKSPACE), undefined);
|
||||
});
|
||||
|
||||
test('session that no longer exists has review cleaned up', async () => {
|
||||
commandService.result = { type: 'success', comments: [{ uri: fileA, range: new Range(1, 1, 1, 1), body: 'orphaned comment' }] };
|
||||
service.requestReview(session, 'v1', [{ currentUri: fileA }]);
|
||||
await tick();
|
||||
|
||||
assert.strictEqual(service.getReviewState(session).get().kind, CodeReviewStateKind.Result);
|
||||
|
||||
agentSessionsService.fireSessionsChanged();
|
||||
|
||||
assert.strictEqual(service.getReviewState(session).get().kind, CodeReviewStateKind.Idle);
|
||||
});
|
||||
|
||||
test('session with no changes has review cleaned up', async () => {
|
||||
agentSessionsService.setSession(session, [
|
||||
{ uri: fileA, modifiedUri: fileA, insertions: 1, deletions: 0 },
|
||||
]);
|
||||
|
||||
commandService.result = { type: 'success', comments: [{ uri: fileA, range: new Range(1, 1, 1, 1), body: 'comment' }] };
|
||||
service.requestReview(session, 'v1', [{ currentUri: fileA }]);
|
||||
await tick();
|
||||
|
||||
agentSessionsService.updateSessionChanges(session, undefined);
|
||||
agentSessionsService.fireSessionsChanged();
|
||||
|
||||
assert.strictEqual(service.getReviewState(session).get().kind, CodeReviewStateKind.Idle);
|
||||
});
|
||||
|
||||
test('session with matching version keeps review intact', async () => {
|
||||
const changes: IChatSessionFileChange2[] = [
|
||||
{ uri: fileA, modifiedUri: fileA, insertions: 1, deletions: 0 },
|
||||
];
|
||||
agentSessionsService.setSession(session, changes);
|
||||
|
||||
const files = getCodeReviewFilesFromSessionChanges(changes);
|
||||
const version = getCodeReviewVersion(files);
|
||||
|
||||
commandService.result = { type: 'success', comments: [{ uri: fileA, range: new Range(1, 1, 1, 1), body: 'valid comment' }] };
|
||||
service.requestReview(session, version, files);
|
||||
await tick();
|
||||
|
||||
agentSessionsService.fireSessionsChanged();
|
||||
|
||||
const state = service.getReviewState(session).get();
|
||||
assert.strictEqual(state.kind, CodeReviewStateKind.Result);
|
||||
if (state.kind === CodeReviewStateKind.Result) {
|
||||
assert.strictEqual(state.comments[0].body, 'valid comment');
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
function tick(): Promise<void> {
|
||||
|
||||
Reference in New Issue
Block a user