diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorActions.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorActions.ts index 2fd5134bb04..eb9a50f5b97 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorActions.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorActions.ts @@ -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); diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidgetContribution.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidgetContribution.ts index 987c39acfef..ad4b69371fc 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidgetContribution.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidgetContribution.ts @@ -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(); @@ -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); diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts index 9d99b64cada..d4f30fd99e5 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts @@ -79,6 +79,12 @@ export interface IAgentFeedbackService { */ revealFeedback(sessionResource: URI, feedbackId: string): Promise; + /** + * 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; + /** * 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 { 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 { diff --git a/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorOverlay.css b/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorOverlay.css index 0de61925c9e..766e481b9eb 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorOverlay.css +++ b/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorOverlay.css @@ -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; diff --git a/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorWidget.css b/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorWidget.css index 8eed23bc26c..608af1249b1 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorWidget.css +++ b/src/vs/sessions/contrib/agentFeedback/browser/media/agentFeedbackEditorWidget.css @@ -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; diff --git a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.fixture.ts b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.fixture.ts index 818c87c9a36..b8b75bec923 100644 --- a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.fixture.ts +++ b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.fixture.ts @@ -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); diff --git a/src/vs/sessions/contrib/changes/browser/changesView.ts b/src/vs/sessions/contrib/changes/browser/changesView.ts index efd27ab3a77..9c30d453409 100644 --- a/src/vs/sessions/contrib/changes/browser/changesView.ts +++ b/src/vs/sessions/contrib/changes/browser/changesView.ts @@ -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') { diff --git a/src/vs/sessions/contrib/changes/browser/media/changesView.css b/src/vs/sessions/contrib/changes/browser/media/changesView.css index 948b1a898bc..1b187233832 100644 --- a/src/vs/sessions/contrib/changes/browser/media/changesView.css +++ b/src/vs/sessions/contrib/changes/browser/media/changesView.css @@ -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; +} diff --git a/src/vs/sessions/contrib/codeReview/browser/codeReview.contributions.ts b/src/vs/sessions/contrib/codeReview/browser/codeReview.contributions.ts index 805ad59cca0..bb3818e8a46 100644 --- a/src/vs/sessions/contrib/codeReview/browser/codeReview.contributions.ts +++ b/src/vs/sessions/contrib/codeReview/browser/codeReview.contributions.ts @@ -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.") diff --git a/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts b/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts index acd401cf4e3..8230f01c789 100644 --- a/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts +++ b/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts @@ -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(); 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 { @@ -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 = 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 = {}; + 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(); + } + })); + } } diff --git a/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts b/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts index 76dcc8c1385..000bf528d12 100644 --- a/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts +++ b/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts @@ -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; + readonly onDidChangeSessionArchivedState: Event; + private readonly _onDidChangeSessions: Emitter; + readonly model: IAgentSessionsModel; + private readonly _sessions = new Map(); + + constructor(disposables: DisposableStore) { + this._onDidChangeSessionArchivedState = disposables.add(new Emitter()); + this.onDidChangeSessionArchivedState = this._onDidChangeSessionArchivedState.event; + this._onDidChangeSessions = disposables.add(new Emitter()); + 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 | 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 {