diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedback.contribution.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedback.contribution.ts index f0183f68f87e04..2786b6a9aaffc1 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedback.contribution.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedback.contribution.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import './agentFeedbackEditorInputContribution.js'; +import { AGENTS_WINDOW_PR_COMMENTS_SETTING } from './agentFeedbackEditorInputContribution.js'; import './agentFeedbackEditorWidgetContribution.js'; import './agentFeedbackOverviewRulerContribution.js'; import { Event } from '../../../../base/common/event.js'; @@ -29,6 +29,8 @@ import { IChatAttachmentWidgetRegistry } from '../../../../workbench/contrib/cha import { IAgentFeedbackVariableEntry } from '../../../../workbench/contrib/chat/common/attachments/chatVariableEntries.js'; import { Codicon } from '../../../../base/common/codicons.js'; import { ISessionsService } from '../../../services/sessions/browser/sessionsService.js'; +import { ConfigurationScope, Extensions as ConfigurationExtensions, IConfigurationRegistry } from '../../../../platform/configuration/common/configurationRegistry.js'; +import { Registry } from '../../../../platform/registry/common/platform.js'; /** * Sets the `hasActiveSessionAgentFeedback` context key to true when the * currently active session has pending agent feedback items. @@ -90,6 +92,20 @@ registerAgentFeedbackReviewCommands(); registerSingleton(IAgentFeedbackService, AgentFeedbackService, InstantiationType.Delayed); +Registry.as(ConfigurationExtensions.Configuration).registerConfiguration({ + id: 'chat', + properties: { + [AGENTS_WINDOW_PR_COMMENTS_SETTING]: { + type: 'boolean', + default: false, + scope: ConfigurationScope.APPLICATION, + tags: ['experimental'], + experiment: { mode: 'auto' }, + description: localize('chat.experimental.agentsWindowPRComments', "Enables the PR Comment option when adding feedback in the Agents Window."), + }, + }, +}); + // Register the custom attachment widget for agentFeedback attachments class AgentFeedbackAttachmentWidgetContribution { static readonly ID = 'workbench.contrib.agentFeedbackAttachmentWidgetFactory'; diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorActions.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorActions.ts index 47c0c17c397bf0..f9ec4863fcd039 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorActions.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorActions.ts @@ -17,7 +17,7 @@ import { IEditorService } from '../../../../workbench/services/editor/common/edi import { GroupsOrder, IEditorGroupsService } from '../../../../workbench/services/editor/common/editorGroupsService.js'; import { ChatContextKeys } from '../../../../workbench/contrib/chat/common/actions/chatContextKeys.js'; import { CHAT_CATEGORY } from '../../../../workbench/contrib/chat/browser/actions/chatActions.js'; -import { AgentFeedbackState, IAgentFeedbackService } from './agentFeedbackService.js'; +import { AgentFeedbackState, IAgentFeedbackService, shouldIncludeRawPRReviewComments } from './agentFeedbackService.js'; import { getActiveResourceCandidates, getFeedbackSessionCandidates } from './agentFeedbackEditorUtils.js'; import { Menus } from '../../../browser/menus.js'; import { ICodeReviewService } from '../../codeReview/browser/codeReviewService.js'; @@ -62,6 +62,7 @@ abstract class AgentFeedbackEditorAction extends Action2 { agentFeedbackService.getFeedback(sessionResource), codeReviewService.getPRReviewState(sessionResource).get(), agentFeedbackService.getVisibleResolvedFeedbackIds(sessionResource), + shouldIncludeRawPRReviewComments(agentFeedbackService, sessionResource), ); if (comments.length > 0) { return this.runWithSession(accessor, sessionResource, resource); @@ -133,6 +134,7 @@ class NavigateFeedbackAction extends AgentFeedbackEditorAction { agentFeedbackService.getFeedback(sessionResource), codeReviewService.getPRReviewState(sessionResource).get(), agentFeedbackService.getVisibleResolvedFeedbackIds(sessionResource), + shouldIncludeRawPRReviewComments(agentFeedbackService, sessionResource), ); const comment = agentFeedbackService.getNextNavigableItem(sessionResource, comments, this._next); diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts index f7b8ece70c2f8b..67397ade0c5e85 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorInputContribution.ts @@ -4,7 +4,9 @@ *--------------------------------------------------------------------------------------------*/ import './media/agentFeedbackEditorInput.css'; +import { getErrorMessage } from '../../../../base/common/errors.js'; import { Disposable, DisposableStore } from '../../../../base/common/lifecycle.js'; +import { ActionRunner, toAction } from '../../../../base/common/actions.js'; import { MarkdownString } from '../../../../base/common/htmlContent.js'; import { ICodeEditor, IEditorMouseEvent, IOverlayWidget, IOverlayWidgetPosition } from '../../../../editor/browser/editorBrowser.js'; import { IEditorContribution, IEditorDecorationsCollection } from '../../../../editor/common/editorCommon.js'; @@ -15,8 +17,10 @@ import { Position } from '../../../../editor/common/core/position.js'; import { Range } from '../../../../editor/common/core/range.js'; import { Selection, SelectionDirection } from '../../../../editor/common/core/selection.js'; import { addStandardDisposableListener, getWindow, isHTMLElement } from '../../../../base/browser/dom.js'; +import { IAnchor } from '../../../../base/browser/ui/contextview/contextview.js'; import { URI } from '../../../../base/common/uri.js'; import { isEqual } from '../../../../base/common/resources.js'; +import { isIOS } from '../../../../base/common/platform.js'; import { KeyCode } from '../../../../base/common/keyCodes.js'; import { Keybinding, KeyCodeChord, ResolvedKeybinding } from '../../../../base/common/keybindings.js'; import { IAgentFeedbackService } from './agentFeedbackService.js'; @@ -29,13 +33,18 @@ import { ContextKeyExpr, IContextKey, IContextKeyService, RawContextKey } from ' import { IInstantiationService, ServicesAccessor } from '../../../../platform/instantiation/common/instantiation.js'; import { IContextMenuService } from '../../../../platform/contextview/browser/contextView.js'; import { IKeybindingService } from '../../../../platform/keybinding/common/keybinding.js'; +import { INotificationService } from '../../../../platform/notification/common/notification.js'; +import { IConfigurationService } from '../../../../platform/configuration/common/configuration.js'; import { ChatContextKeys } from '../../../../workbench/contrib/chat/common/actions/chatContextKeys.js'; import { CHAT_CATEGORY } from '../../../../workbench/contrib/chat/browser/actions/chatActions.js'; import { FeedbackInputWidget } from './feedbackInputWidget.js'; +import { ICodeReviewService, IPRReviewCommentTarget } from '../../codeReview/browser/codeReviewService.js'; +import { IGitHubPullRequestRef } from '../../../services/sessions/common/session.js'; const addFeedbackAtCurrentLineActionId = 'agentFeedbackEditor.action.addAtCurrentLine'; const agentFeedbackHoverGlyphClassName = 'agent-feedback-glyph'; const hasAgentFeedbackSessionForEditor = new RawContextKey('agentFeedbackEditor.hasSession', false); +export const AGENTS_WINDOW_PR_COMMENTS_SETTING = 'chat.experimental.agentsWindowPRComments'; /** * The inline "Add Feedback" input shown in the editor when the user selects a @@ -139,6 +148,18 @@ export class AgentFeedbackInputWidget extends Disposable implements IOverlayWidg this._core.updateActionEnabled(); } + get isBusy(): boolean { + return this._core.isBusy; + } + + setBusy(busy: boolean, statusLabel?: string): void { + this._core.setBusy(busy, statusLabel); + } + + setActionLabels(primaryLabel: string, secondaryLabel: string): void { + this._core.setActionLabels(primaryLabel, secondaryLabel); + } + private _computeContentWidth(): number { // The widget sticks to the editor's content left edge, so the space it // has available is the content area width (to the right of the line @@ -161,6 +182,8 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements private _anchorPosition: Position | undefined; private _preferBelow = true; private _hoverLineNumber: number | undefined; + private _selectedPRCommentTarget: IPRReviewCommentTarget | undefined; + private _selectingCommentTarget = false; private readonly _hoverDecorations: IEditorDecorationsCollection; private readonly _hasAgentFeedbackSessionContext: IContextKey; private readonly _widgetListeners = this._store.add(new DisposableStore()); @@ -171,6 +194,10 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements @ICodeEditorService private readonly _codeEditorService: ICodeEditorService, @IContextKeyService private readonly _contextKeyService: IContextKeyService, @IInstantiationService private readonly _instantiationService: IInstantiationService, + @ICodeReviewService private readonly _codeReviewService: ICodeReviewService, + @IContextMenuService private readonly _contextMenuService: IContextMenuService, + @INotificationService private readonly _notificationService: INotificationService, + @IConfigurationService private readonly _configurationService: IConfigurationService, ) { super(); @@ -204,7 +231,7 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements e.event.stopPropagation(); const lineNumber = e.target.position?.lineNumber; if (lineNumber !== undefined) { - this._selectLine(lineNumber); + void this._selectLine(lineNumber, { x: e.event.posx, y: e.event.posy }); } return; } @@ -262,8 +289,8 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements private _ensureWidget(): AgentFeedbackInputWidget { if (!this._widget) { this._widget = this._instantiationService.createInstance(AgentFeedbackInputWidget, this._editor); - this._store.add(this._widget.onDidTriggerAdd(() => this._addFeedback())); - this._store.add(this._widget.onDidTriggerAddAndSubmit(() => this._addFeedbackAndSubmit())); + this._store.add(this._widget.onDidTriggerAdd(() => void this._addFeedback())); + this._store.add(this._widget.onDidTriggerAddAndSubmit(() => void this._addFeedbackAndSubmit())); this._editor.addOverlayWidget(this._widget); } return this._widget; @@ -343,6 +370,9 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements } private _onSelectionChanged(): void { + if (this._selectingCommentTarget) { + return; + } if (this._suppressSelectionChangeOnce) { this._suppressSelectionChangeOnce = false; return; @@ -364,6 +394,9 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements this._autoHide(); return; } + if (this._visible && this._pinnedRange?.equalsRange(selection)) { + return; + } const model = this._editor.getModel(); if (!model) { @@ -378,6 +411,7 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements } this._sessionResource = sessionResource; + this._selectedPRCommentTarget = undefined; const preferBelow = selection.getDirection() === SelectionDirection.LTR; const anchorPosition = preferBelow ? selection.getEndPosition() : selection.getStartPosition(); this._show(Range.lift(selection), anchorPosition, preferBelow); @@ -395,7 +429,13 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements this._pinnedRange = range; this._anchorPosition = anchorPosition; this._preferBelow = preferBelow; - widget.setPlaceholder(this._getPlaceholder()); + widget.setPlaceholder(this._selectedPRCommentTarget + ? localize('agentFeedback.addPRComment', "Add PR Comment") + : this._getPlaceholder()); + widget.setActionLabels( + this._selectedPRCommentTarget ? localize('agentFeedback.addPRCommentAction', "Add PR Comment") : localize('agentFeedback.addAction', "Add"), + this._selectedPRCommentTarget ? localize('agentFeedback.addPRCommentAction', "Add PR Comment") : localize('agentFeedback.addAndSubmit', "Add and Submit"), + ); widget.clearInput(); widget.show(); this._updatePosition(); @@ -420,6 +460,7 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements this._visible = false; this._pinnedRange = undefined; this._anchorPosition = undefined; + this._selectedPRCommentTarget = undefined; this._widgetListeners.clear(); if (this._widget) { @@ -438,42 +479,30 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements if (!position) { return; } - this._showAtLine(position.lineNumber, focusInput); + void this._selectLine(position.lineNumber, this._getLineAnchor(position.lineNumber), focusInput); } - private _showAtLine(lineNumber: number, focusInput: boolean): void { - if (this._visible && this._hasInputText()) { - this.focusInput(); - return; + private _getLineAnchor(lineNumber: number): IAnchor { + const editorRect = this._editor.getDomNode()?.getBoundingClientRect(); + const visiblePosition = this._editor.getScrolledVisiblePosition(new Position(lineNumber, 1)); + if (!editorRect || !visiblePosition) { + return { x: 0, y: 0 }; } - - const model = this._editor.getModel(); - if (!model || lineNumber < 1 || lineNumber > model.getLineCount()) { - this._autoHide(); - return; - } - - const sessionResource = this._getSessionForModel(); - if (!sessionResource) { - this._autoHide(); - return; - } - - this._sessionResource = sessionResource; - this._show(new Range(lineNumber, 1, lineNumber, model.getLineMaxColumn(lineNumber)), new Position(lineNumber, 1), true, focusInput); + return { + x: editorRect.left + this._editor.getLayoutInfo().contentLeft, + y: editorRect.top + visiblePosition.top + visiblePosition.height, + }; } - /** - * Select the whole line as a result of clicking the gutter glyph. Selecting - * the line triggers the selection-change handler which opens the feedback - * input automatically, so we don't open it directly here. Empty lines are - * ignored as there is nothing to give feedback on. - */ - private _selectLine(lineNumber: number): void { + /** Choose the comment target before selecting the line and opening its input. */ + private async _selectLine(lineNumber: number, anchor: IAnchor, focusInput = true): Promise { if (this._visible && this._hasInputText()) { this.focusInput(); return; } + if (this._visible) { + this._hide(); + } const model = this._editor.getModel(); if (!model || lineNumber < 1 || lineNumber > model.getLineCount()) { @@ -484,17 +513,93 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements return; } - // Set the selection before focusing: the selection change while the - // editor is unfocused is ignored, then focusing re-evaluates the - // selection and opens the input for the freshly selected line. - this._editor.setSelection(new Selection(lineNumber, 1, lineNumber, model.getLineMaxColumn(lineNumber))); - this._editor.focus(); + const sessionResource = this._getSessionForModel(); + if (!sessionResource) { + return; + } - // Focusing the editor synchronously opens the input via the - // selection-change handler, so move focus into it now that it is - // visible. This lets the user type feedback immediately after clicking - // the gutter glyph without having to click the input first. - this.focusInput(); + this._selectingCommentTarget = true; + try { + const range = new Range(lineNumber, 1, lineNumber, model.getLineMaxColumn(lineNumber)); + const pullRequests = this._agentFeedbackService.isAgentHostSession(sessionResource) + && this._configurationService.getValue(AGENTS_WINDOW_PR_COMMENTS_SETTING) === true + ? this._codeReviewService.getPRReviewCommentPullRequests(sessionResource, model.uri) + : []; + const pullRequest = await this._pickCommentTarget(pullRequests, anchor); + if (pullRequest === undefined || !isEqual(this._editor.getModel()?.uri, model.uri)) { + return; + } + + let target: IPRReviewCommentTarget | undefined; + if (pullRequest) { + try { + [target] = await this._codeReviewService.getPRReviewCommentTargets(sessionResource, model.uri, range, model.getValue(), pullRequest); + } catch (error) { + this._notificationService.error(localize('agentFeedback.loadPRCommentTargetsFailed', "Failed to load pull request comment targets: {0}", getErrorMessage(error))); + return; + } + if (!target) { + this._notificationService.warn(localize('agentFeedback.prCommentUnavailableForLine', "A pull request comment cannot be added to this line.")); + return; + } + } + + this._selectedPRCommentTarget = target; + this._editor.setSelection(new Selection(lineNumber, 1, lineNumber, model.getLineMaxColumn(lineNumber))); + this._editor.focus(); + this._show(range, new Position(lineNumber, 1), true, focusInput); + } finally { + this._selectingCommentTarget = false; + } + } + + private _pickCommentTarget(pullRequests: readonly IGitHubPullRequestRef[], anchor: IAnchor): Promise { + if (pullRequests.length === 0) { + return Promise.resolve(null); + } + + return new Promise(resolve => { + const disposables = new DisposableStore(); + let selectedPullRequest: IGitHubPullRequestRef | null | undefined; + const actions = [ + toAction({ + id: 'agentFeedback.commentTarget.agentFeedback', + label: localize('agentFeedback.commentTarget', "Agent Feedback"), + run: () => { }, + }), + ...pullRequests.map(pullRequest => toAction({ + id: `agentFeedback.commentTarget.pullRequest.${pullRequest.owner}.${pullRequest.repo}.${pullRequest.number}`, + label: pullRequests.length === 1 + ? localize('agentFeedback.prCommentTarget', "Pull Request Comment") + : localize('agentFeedback.prCommentTargetWithPR', "Pull Request ({0}/{1}#{2}) Comment", pullRequest.owner, pullRequest.repo, pullRequest.number), + run: () => { }, + })), + ]; + const pullRequestsByAction = new Map([ + [actions[0].id, null], + ...actions.slice(1).map((action, index) => [action.id, pullRequests[index]] as const), + ]); + const actionRunner = disposables.add(new ActionRunner()); + disposables.add(actionRunner.onWillRun(event => { + selectedPullRequest = pullRequestsByAction.get(event.action.id); + resolve(selectedPullRequest); + })); + this._contextMenuService.showContextMenu({ + domForShadowRoot: this._editor.getOption(EditorOption.useShadowDOM) && !isIOS ? this._editor.getDomNode() ?? undefined : undefined, + useWindowContainerForShadowRoot: this._editor.getOption(EditorOption.useShadowDOM) && !isIOS && this._editor.getOption(EditorOption.fixedOverflowWidgets), + getAnchor: () => anchor, + getActions: () => actions, + getMenuClassName: () => 'agent-feedback-comment-target-menu', + actionRunner, + autoSelectFirstItem: true, + onHide: didCancel => { + if (didCancel) { + resolve(undefined); + } + getWindow(this._editor.getDomNode()!).setTimeout(() => disposables.dispose(), 0); + }, + }); + }); } private _getSessionForModel(): URI | undefined { @@ -600,14 +705,14 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements if (e.keyCode === KeyCode.Enter && e.altKey) { e.preventDefault(); e.stopPropagation(); - this._addFeedbackAndSubmit(); + void this._addFeedbackAndSubmit(); return; } if (e.keyCode === KeyCode.Enter) { e.preventDefault(); e.stopPropagation(); - this._addFeedback(); + void this._addFeedback(); return; } })); @@ -651,12 +756,13 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements this._editor.focus(); } - private _addFeedback(): boolean { - if (!this._widget) { + private async _addFeedback(): Promise { + const widget = this._widget; + if (!widget || widget.isBusy) { return false; } - const text = this._widget.inputElement.value.trim(); + const text = widget.inputElement.value.trim(); if (!text) { return false; } @@ -667,12 +773,33 @@ export class AgentFeedbackEditorInputContribution extends Disposable implements return false; } - this._agentFeedbackService.addFeedback(this._sessionResource, model.uri, range, text, undefined, createAgentFeedbackContext(this._editor, this._codeEditorService, model.uri, range)); + if (this._selectedPRCommentTarget) { + widget.setBusy(true, localize('agentFeedback.addingPRComment', "Adding pull request comment")); + try { + await this._codeReviewService.createPRReviewComment(this._selectedPRCommentTarget, text); + } catch (error) { + this._notificationService.error(localize('agentFeedback.addPRCommentFailed', "Failed to add pull request comment: {0}", getErrorMessage(error))); + return false; + } finally { + if (!this._store.isDisposed) { + widget.setBusy(false); + } + } + } else { + this._agentFeedbackService.addFeedback(this._sessionResource, model.uri, range, text, undefined, createAgentFeedbackContext(this._editor, this._codeEditorService, model.uri, range)); + } + if (this._store.isDisposed) { + return false; + } this._hideAndRefocusEditor(); return true; } - private _addFeedbackAndSubmit(): void { + private async _addFeedbackAndSubmit(): Promise { + if (this._selectedPRCommentTarget) { + await this._addFeedback(); + return; + } if (!this._widget) { return; } diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorOverlay.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorOverlay.ts index 29c64998764345..9fb8900f85dc33 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorOverlay.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorOverlay.ts @@ -13,7 +13,7 @@ import { IWorkbenchContribution } from '../../../../workbench/common/contributio import { EditorGroupView } from '../../../../workbench/browser/parts/editor/editorGroupView.js'; import { IEditorGroup, IEditorGroupsService } from '../../../../workbench/services/editor/common/editorGroupsService.js'; import { AgentEditorCommentsOverlayWidget } from '../../../../workbench/services/agentEditorComments/browser/agentEditorCommentsOverlayWidget.js'; -import { IAgentFeedbackService } from './agentFeedbackService.js'; +import { IAgentFeedbackService, shouldIncludeRawPRReviewComments } from './agentFeedbackService.js'; import { hasUnsubmittedAgentFeedback, hasSessionEditorComments, navigateNextFeedbackActionId, navigatePreviousFeedbackActionId, navigationBearingFakeActionId, submitFeedbackActionId } from './agentFeedbackEditorActions.js'; import { getActiveResourceCandidates, getFeedbackSessionCandidates } from './agentFeedbackEditorUtils.js'; import { Menus } from '../../../browser/menus.js'; @@ -99,6 +99,7 @@ export class AgentFeedbackOverlayController { agentFeedbackService.getFeedback(sessionResource), codeReviewService.getPRReviewState(sessionResource).read(r), agentFeedbackService.getVisibleResolvedFeedbackIds(sessionResource), + shouldIncludeRawPRReviewComments(agentFeedbackService, sessionResource), ); if (comments.length > 0) { navigationBearings = agentFeedbackService.getNavigationBearing(sessionResource, comments); diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidget.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidget.ts index c442a1a2cc0e77..fc320a5453ccaf 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidget.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidget.ts @@ -565,6 +565,9 @@ export class AgentFeedbackEditorWidget extends Disposable implements IOverlayWid return; } + if (comment.kind === AgentFeedbackKind.PRReview && comment.sourcePRReviewCommentId) { + this._codeReviewService.dismissPRReviewComment(this._sessionResource, comment.sourcePRReviewCommentId); + } this._agentFeedbackService.removeFeedback(this._sessionResource, comment.sourceId); } diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidgetContribution.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidgetContribution.ts index ee9f231415638f..efa750388d0d8b 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidgetContribution.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackEditorWidgetContribution.ts @@ -19,7 +19,7 @@ import { ISessionFileChange } from '../../../services/sessions/common/session.js import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; import { ICodeReviewService, IPRReviewState } from '../../codeReview/browser/codeReviewService.js'; import { AgentFeedbackEditorWidget, IComposerDraft, IComposerDraftState } from './agentFeedbackEditorWidget.js'; -import { IAgentFeedbackService } from './agentFeedbackService.js'; +import { IAgentFeedbackService, shouldIncludeRawPRReviewComments } from './agentFeedbackService.js'; import { getSessionEditorComments, groupNearbySessionEditorComments, ISessionEditorComment } from './sessionEditorComments.js'; /** @@ -116,6 +116,7 @@ export class AgentFeedbackEditorWidgetContribution extends Disposable implements this._agentFeedbackService.getFeedback(this._sessionResource), prReviewState, this._agentFeedbackService.getVisibleResolvedFeedbackIds(this._sessionResource), + shouldIncludeRawPRReviewComments(this._agentFeedbackService, this._sessionResource), ); const fileComments = this._getCommentsForModel(model.uri, comments); if (fileComments.length === 0) { @@ -233,6 +234,7 @@ export class AgentFeedbackEditorWidgetContribution extends Disposable implements this._agentFeedbackService.getFeedback(this._sessionResource), this._codeReviewService.getPRReviewState(this._sessionResource).get(), this._agentFeedbackService.getVisibleResolvedFeedbackIds(this._sessionResource), + shouldIncludeRawPRReviewComments(this._agentFeedbackService, this._sessionResource), ); const bearing = this._agentFeedbackService.getNavigationBearing(this._sessionResource, comments); if (bearing.activeIdx < 0) { diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackItemsBackend.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackItemsBackend.ts index 62e78ad8eae5b4..ffd947dda73223 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackItemsBackend.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackItemsBackend.ts @@ -5,6 +5,7 @@ import { Emitter, Event } from '../../../../base/common/event.js'; import { Disposable, DisposableMap, DisposableStore } from '../../../../base/common/lifecycle.js'; +import { getComparisonKey } from '../../../../base/common/resources.js'; import { URI } from '../../../../base/common/uri.js'; import { IRange } from '../../../../editor/common/core/range.js'; import { IAgentConnection } from '../../../../platform/agentHost/common/agentService.js'; @@ -338,7 +339,7 @@ export class AnnotationsAgentFeedbackItemsBackend extends Disposable implements private readonly _channels = this._register(new DisposableMap()); private readonly _channelBySession = new Map(); private readonly _sessionResourceByKey = new Map(); - /** Local cache so reads work before the first snapshot arrives. */ + /** Optimistic local state, reconciled from each annotations snapshot/action. */ private readonly _cacheBySession = new Map(); /** * Signature of the feedback set we last fired {@link onDidChangeItems} for, @@ -354,6 +355,7 @@ export class AnnotationsAgentFeedbackItemsBackend extends Disposable implements * the authoritative set before acting. */ private readonly _loadedBySession = new Set(); + private readonly _suspendedAnnotationsReconciliation = new Set(); constructor( @ISessionsManagementService private readonly _sessionsManagementService: ISessionsManagementService, @@ -370,10 +372,16 @@ export class AnnotationsAgentFeedbackItemsBackend extends Disposable implements getItems(sessionResource: URI): readonly IAgentFeedback[] { const channel = this._ensureChannel(sessionResource); + const cached = this._cacheBySession.get(sessionResource.toString()); + if (cached) { + return orderFeedbackItems(cached); + } if (channel && this._hasSnapshot(channel.subscription)) { - return orderFeedbackItems(this._decode(channel, sessionResource)); + const items = this._decode(channel, sessionResource); + this._cacheBySession.set(sessionResource.toString(), items); + return orderFeedbackItems(items); } - return orderFeedbackItems(this._cacheBySession.get(sessionResource.toString()) ?? []); + return []; } hasLoaded(sessionResource: URI): boolean { @@ -387,48 +395,48 @@ export class AnnotationsAgentFeedbackItemsBackend extends Disposable implements upsert(feedback: IAgentFeedback): void { const channel = this._ensureChannel(feedback.sessionResource); this._cacheUpsert(feedback); + this._emitIfChanged(feedback.sessionResource); if (!channel) { - this._onDidChangeItems.fire(feedback.sessionResource); return; } channel.connection.dispatch(channel.annotationsUri.toString(), { type: ActionType.AnnotationsSet, annotation: feedbackToAnnotation(feedback, channel.connection), }); - if (!this._hasSnapshot(channel.subscription)) { - this._onDidChangeItems.fire(feedback.sessionResource); - } } remove(sessionResource: URI, feedbackId: string): void { const channel = this._ensureChannel(sessionResource); this._cacheRemove(sessionResource, feedbackId); + this._emitIfChanged(sessionResource); if (!channel) { - this._onDidChangeItems.fire(sessionResource); return; } channel.connection.dispatch(channel.annotationsUri.toString(), { type: ActionType.AnnotationsRemoved, annotationId: feedbackId, }); - if (!this._hasSnapshot(channel.subscription)) { - this._onDidChangeItems.fire(sessionResource); - } } clear(sessionResource: URI): void { + const key = sessionResource.toString(); const items = this.getItems(sessionResource); const channel = this._ensureChannel(sessionResource); - this._cacheBySession.delete(sessionResource.toString()); + this._cacheBySession.set(key, []); + this._emitIfChanged(sessionResource); if (channel) { - for (const item of items) { - channel.connection.dispatch(channel.annotationsUri.toString(), { - type: ActionType.AnnotationsRemoved, - annotationId: item.id, - }); + this._suspendedAnnotationsReconciliation.add(key); + try { + for (const item of items) { + channel.connection.dispatch(channel.annotationsUri.toString(), { + type: ActionType.AnnotationsRemoved, + annotationId: item.id, + }); + } + } finally { + this._suspendedAnnotationsReconciliation.delete(key); } } - this._onDidChangeItems.fire(sessionResource); } getSessionsWithItems(): URI[] { @@ -479,21 +487,32 @@ export class AnnotationsAgentFeedbackItemsBackend extends Disposable implements */ private _onAnnotationsChange(sessionResource: URI): void { const key = sessionResource.toString(); + if (this._suspendedAnnotationsReconciliation.has(key)) { + return; + } const channel = this._channelBySession.get(key); if (!channel) { return; } + if (!this._hasSnapshot(channel.subscription)) { + return; + } + this._cacheBySession.set(key, this._decode(channel, sessionResource)); // Fire once when the snapshot first arrives so consumers learn that the // feedback set is now authoritative, even if it is empty (and thus has // the same — empty — signature as before loading). - if (this._hasSnapshot(channel.subscription) && !this._loadedBySession.has(key)) { + if (!this._loadedBySession.has(key)) { this._loadedBySession.add(key); - this._signatureBySession.set(key, this._feedbackSignature(channel.subscription)); - this._onDidChangeItems.fire(sessionResource); + this._emitIfChanged(sessionResource, true); return; } - const signature = this._feedbackSignature(channel.subscription); - if (this._signatureBySession.get(key) === signature) { + this._emitIfChanged(sessionResource); + } + + private _emitIfChanged(sessionResource: URI, force = false): void { + const key = sessionResource.toString(); + const signature = this._feedbackSignature(this._cacheBySession.get(key) ?? []); + if (!force && this._signatureBySession.get(key) === signature) { return; } this._signatureBySession.set(key, signature); @@ -501,29 +520,30 @@ export class AnnotationsAgentFeedbackItemsBackend extends Disposable implements } /** - * A stable signature of the feedback-bearing annotations in the - * subscription's current snapshot (sorted by id). Excludes annotations - * without feedback metadata so unrelated annotation activity on the shared - * channel is ignored. + * A stable signature of the visible feedback state, sorted by id. */ - private _feedbackSignature(subscription: IAgentSubscription): string { - const value = subscription.value; - if (!value || value instanceof Error) { - return ''; - } - const feedback = value.annotations - .map(annotation => ({ annotation, meta: readFeedbackMeta(annotation) })) - .filter(({ annotation, meta }) => meta !== undefined && (annotation.entries?.length ?? 0) > 0) - .map(({ annotation, meta }) => ({ - id: annotation.id, - resource: annotation.resource, - range: annotation.range, - resolved: annotation.resolved, - entries: annotation.entries, - meta, - })) - .sort((a, b) => a.id.localeCompare(b.id)); - return JSON.stringify(feedback); + private _feedbackSignature(items: readonly IAgentFeedback[]): string { + return JSON.stringify(items + .map(item => [ + item.id, + item.text, + getComparisonKey(item.resourceUri), + item.range.startLineNumber, + item.range.startColumn, + item.range.endLineNumber, + item.range.endColumn, + getComparisonKey(item.sessionResource), + item.suggestion ?? null, + item.codeSelection ?? null, + item.diffHunks ?? null, + item.kind, + item.sourcePRReviewCommentId ?? null, + item.sourcePullRequest ?? null, + item.replies ?? null, + item.state, + item.pendingAgentReveal ?? null, + ] as const) + .sort((a, b) => a[0].localeCompare(b[0]))); } private _cacheUpsert(feedback: IAgentFeedback): void { @@ -590,9 +610,11 @@ export class AnnotationsAgentFeedbackItemsBackend extends Disposable implements annotationsUri: resolved.annotationsUri, subscription: ref.object, }; - this._signatureBySession.set(key, this._feedbackSignature(ref.object)); if (this._hasSnapshot(ref.object)) { this._loadedBySession.add(key); + const items = this._decode(channel, sessionResource); + this._cacheBySession.set(key, items); + this._signatureBySession.set(key, this._feedbackSignature(items)); } store.add(ref.object.onDidChange(() => this._onAnnotationsChange(sessionResource))); diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackPRReviewSeeder.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackPRReviewSeeder.ts index 2efca63401c26f..50ea375fbda48e 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackPRReviewSeeder.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackPRReviewSeeder.ts @@ -26,9 +26,8 @@ import { AgentFeedbackKind, AgentFeedbackState, IAgentFeedback, IAgentFeedbackSe * aware of the comments so the user can reveal and accept them. * * The mirror carries {@link IAgentFeedback.sourcePRReviewCommentId} (the GitHub - * review thread id) so it can be deduplicated against the raw PR comment in the - * editor (see `getSessionEditorComments`) and so resolving the agent feedback - * resolves the originating GitHub thread (see + * review thread id) so it replaces the raw PR comment in the editor and so + * resolving the agent feedback resolves the originating GitHub thread (see * {@link import('./agentFeedbackPRThreadResolver.js').AgentFeedbackPRThreadResolverContribution}). * * Seeding is keyed off the session resource (the same key every other feedback @@ -124,8 +123,7 @@ export class AgentFeedbackPRReviewSeederContribution extends Disposable implemen } continue; } - // A mirror the user already accepted/submitted supersedes the raw PR - // comment; only seed a new created mirror when none exists yet. + // Any existing mirror supersedes the raw PR comment in the editor. if (!mirroredSourceIds.has(comment.id)) { this._agentFeedbackService.addFeedback( sessionResource, diff --git a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts index 92ced5622acd8d..dc7f471ab07933 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/agentFeedbackService.ts @@ -199,6 +199,8 @@ export interface IAgentFeedbackService { * Get all feedback items for a session. */ getFeedback(sessionResource: URI): readonly IAgentFeedback[]; + /** Whether feedback for this session is owned by an Agent Host annotations channel. */ + isAgentHostSession(sessionResource: URI): boolean; /** Show resolved feedback items in editor comment surfaces for this window. */ showFeedbackInEditor(sessionResource: URI, feedbackIds: readonly string[]): void; @@ -294,6 +296,10 @@ export interface IAgentFeedbackService { addFeedbackAndSubmit(sessionResource: URI, resourceUri: URI, range: IRange, text: string, suggestion?: ICodeReviewSuggestion, context?: IAgentFeedbackContext, sourcePRReviewCommentId?: string, kind?: AgentFeedbackKind): Promise; } +export function shouldIncludeRawPRReviewComments(agentFeedbackService: IAgentFeedbackService, sessionResource: URI): boolean { + return !agentFeedbackService.isAgentHostSession(sessionResource) || !agentFeedbackService.hasLoadedFeedback(sessionResource); +} + // --- Implementation ----------------------------------------------------------- /** Stable identity of a session's workspace, or `undefined` when it has none (yet). */ @@ -462,7 +468,7 @@ export class AgentFeedbackService extends Disposable implements IAgentFeedbackSe /** Resolves the storage backend that owns feedback for the given session. */ private _backendForSession(sessionResource: URI): IAgentFeedbackItemsBackend { - if (this._isAgentHostSession(sessionResource)) { + if (this.isAgentHostSession(sessionResource)) { return this._getAnnotationsBackend(); } return this._inMemoryBackend; @@ -943,7 +949,7 @@ export class AgentFeedbackService extends Disposable implements IAgentFeedbackSe return; } - if (!this._isAgentHostSession(sessionResource)) { + if (!this.isAgentHostSession(sessionResource)) { // Wait for the attachment contribution to update the chat widget's attachment model const widget = await whenChatWidgetForSession(this._chatWidgetService, sessionResource); if (widget) { @@ -963,7 +969,7 @@ export class AgentFeedbackService extends Disposable implements IAgentFeedbackSe await this.submitFeedback(sessionResource); } - private _isAgentHostSession(sessionResource: URI): boolean { + isAgentHostSession(sessionResource: URI): boolean { const session = this._resolveSession(sessionResource); return session ? isAgentHostProviderId(session.providerId) : false; } @@ -988,7 +994,7 @@ export class AgentFeedbackService extends Disposable implements IAgentFeedbackSe // items — which are about to become submitted — to this single request // so the agent receives the comments, then remove the transient // attachment again once the request has been accepted. - if (this._isAgentHostSession(sessionResource)) { + if (this.isAgentHostSession(sessionResource)) { const feedbackIds = options?.feedbackIds ? new Set(options.feedbackIds) : undefined; const acceptedItems = this.getFeedback(sessionResource).filter(item => item.state === AgentFeedbackState.Accepted && (!feedbackIds || feedbackIds.has(item.id))); @@ -1050,7 +1056,7 @@ export class AgentFeedbackService extends Disposable implements IAgentFeedbackSe // items stay visible in the submitted state until then. Other providers // have no such agent loop, so submitting resolves the comments directly // to hide them from the UI. - const submittedState = this._isAgentHostSession(sessionResource) + const submittedState = this.isAgentHostSession(sessionResource) ? AgentFeedbackState.Submitted : AgentFeedbackState.Resolved; diff --git a/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts b/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts index 30ef37ccf8f0b5..c1865141a97a82 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/feedbackInputWidget.ts @@ -215,6 +215,13 @@ export class FeedbackInputWidget extends Disposable { } } + setActionLabels(primaryLabel: string, secondaryLabel?: string): void { + this._primaryAction.label = primaryLabel; + if (this._secondaryAction && secondaryLabel) { + this._secondaryAction.label = secondaryLabel; + } + } + /** * Toggles an accessible busy state: disables the input/actions, swaps the * action bar for a spinning loading codicon, and (when turning busy on) diff --git a/src/vs/sessions/contrib/agentFeedback/browser/nullAgentFeedbackService.contribution.ts b/src/vs/sessions/contrib/agentFeedback/browser/nullAgentFeedbackService.contribution.ts index 23f2cbb0f24054..b9e40469e8008d 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/nullAgentFeedbackService.contribution.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/nullAgentFeedbackService.contribution.ts @@ -55,6 +55,7 @@ class NullAgentFeedbackService extends Disposable implements IAgentFeedbackServi setFeedbackResolved(_sessionResource: URI, _feedbackId: string, _resolved: boolean): void { } addReply(_sessionResource: URI, _feedbackId: string, _replyText: string): void { } getFeedback(_sessionResource: URI): readonly IAgentFeedback[] { return []; } + isAgentHostSession(_sessionResource: URI): boolean { return false; } showFeedbackInEditor(_sessionResource: URI, _feedbackIds: readonly string[]): void { } hideFeedbackInEditor(_sessionResource: URI, _feedbackId: string): void { } getVisibleResolvedFeedbackIds(_sessionResource: URI): ReadonlySet { return new Set(); } diff --git a/src/vs/sessions/contrib/agentFeedback/browser/sessionEditorComments.ts b/src/vs/sessions/contrib/agentFeedback/browser/sessionEditorComments.ts index 477fb60f10b959..49998da1a1824a 100644 --- a/src/vs/sessions/contrib/agentFeedback/browser/sessionEditorComments.ts +++ b/src/vs/sessions/contrib/agentFeedback/browser/sessionEditorComments.ts @@ -25,6 +25,7 @@ export interface ISessionEditorComment { readonly range: IRange; readonly text: string; readonly suggestion?: ICodeReviewSuggestion; + readonly sourcePRReviewCommentId?: string; readonly sourcePullRequest?: IFeedbackPullRequest; readonly canConvertToAgentFeedback: boolean; /** @@ -48,19 +49,15 @@ export function getSessionEditorComments( agentFeedbackItems: readonly IAgentFeedback[], prReviewState?: IPRReviewState, visibleResolvedFeedbackIds?: ReadonlySet, + includeRawPRReviewComments = true, ): readonly ISessionEditorComment[] { const comments: ISessionEditorComment[] = []; - // PR review comments are mirrored onto the feedback channel as `created` - // `prReview` items so the agent can see them (see - // `agentFeedbackPRReviewSeeder.ts`). Deduplicate the two representations by - // the originating PR thread id: while a mirror is still `created` the raw PR - // comment is shown (preserving its native actions) and the mirror is hidden; - // once the user accepts the mirror it supersedes the raw PR comment. - const supersededPRCommentIds = new Set(); + // Prefer the Agent Host annotation so the agent and editor share one representation. + const mirroredPRCommentIds = new Set(); for (const item of agentFeedbackItems) { - if (item.kind === AgentFeedbackKind.PRReview && item.sourcePRReviewCommentId && item.state !== AgentFeedbackState.Created) { - supersededPRCommentIds.add(item.sourcePRReviewCommentId); + if (item.kind === AgentFeedbackKind.PRReview && item.sourcePRReviewCommentId) { + mirroredPRCommentIds.add(item.sourcePRReviewCommentId); } } @@ -69,11 +66,6 @@ export function getSessionEditorComments( if (item.state === AgentFeedbackState.Resolved && !visibleResolvedFeedbackIds?.has(item.id)) { continue; } - // Hide the still-unaccepted PR review mirror; the raw PR comment is - // shown instead. - if (item.kind === AgentFeedbackKind.PRReview && item.state === AgentFeedbackState.Created && item.sourcePRReviewCommentId) { - continue; - } comments.push({ id: toSessionEditorCommentId(SessionEditorCommentSource.AgentFeedback, item.id), sourceId: item.id, @@ -84,6 +76,7 @@ export function getSessionEditorComments( range: item.range, text: item.text, suggestion: item.suggestion, + sourcePRReviewCommentId: item.sourcePRReviewCommentId, sourcePullRequest: item.sourcePullRequest, canConvertToAgentFeedback: false, replies: item.replies, @@ -91,10 +84,10 @@ export function getSessionEditorComments( }); } - for (const item of getPRReviewComments(prReviewState)) { - // Hide raw PR comments that the user has already accepted into agent - // feedback (shown via the accepted mirror above). - if (supersededPRCommentIds.has(item.id)) { + for (const item of includeRawPRReviewComments ? getPRReviewComments(prReviewState) : []) { + // The Agent Host annotation is the editor representation whenever one + // exists; raw client-side PR data is only a fallback before seeding. + if (mirroredPRCommentIds.has(item.id)) { continue; } comments.push({ @@ -106,6 +99,7 @@ export function getSessionEditorComments( resourceUri: item.uri, range: item.range, text: item.body, + sourcePRReviewCommentId: item.id, sourcePullRequest: { owner: item.pullRequest.owner, repo: item.pullRequest.repo, 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 7a64fa760cc87c..38b2bba68f79f0 100644 --- a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.fixture.ts +++ b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.fixture.ts @@ -114,6 +114,7 @@ function createMockAgentFeedbackService(): IAgentFeedbackService { override readonly onDidConvertFeedback = Event.None; override readonly onDidAddReply = Event.None; override readonly onDidSubmitFeedback = Event.None; + override isAgentHostSession(): boolean { return false; } override getVisibleResolvedFeedbackIds(): ReadonlySet { return new Set(); @@ -284,6 +285,7 @@ function renderViaContribution(context: ComponentFixtureContext, code: string, c override readonly onDidChangeNavigation = Event.None; override readonly onDidChangeFeedbackScope = Event.None; override readonly onDidRevealSessionComment = Event.None; + override isAgentHostSession(): boolean { return false; } override getVisibleResolvedFeedbackIds(): ReadonlySet { return new Set(); diff --git a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.test.ts b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.test.ts index 5b94fe9471f164..01155d99691e73 100644 --- a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.test.ts +++ b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackEditorWidget.test.ts @@ -45,6 +45,8 @@ suite('AgentFeedbackEditorWidget', () => { /** Comment ids passed to `setNavigationAnchor`, in call order. */ readonly navigations: readonly string[]; readonly hiddenFeedbackIds: readonly string[]; + readonly removedFeedbackIds: readonly string[]; + readonly dismissedPRCommentIds: readonly string[]; readonly sourcePullRequests: readonly (IFeedbackPullRequest | undefined)[]; readonly domNode: HTMLElement; /** Tears the widget down and builds a new one, as the contribution does on any feedback change. */ @@ -54,12 +56,15 @@ suite('AgentFeedbackEditorWidget', () => { function withWidget(callback: (harness: ITestHarness) => void, testComment: ISessionEditorComment = comment): void { const navigations: string[] = []; const hiddenFeedbackIds: string[] = []; + const removedFeedbackIds: string[] = []; + const dismissedPRCommentIds: string[] = []; const sourcePullRequests: (IFeedbackPullRequest | undefined)[] = []; const services = new ServiceCollection(); services.set(IAgentFeedbackService, new class extends mock() { override setNavigationAnchor(_sessionResource: URI, commentId: string): void { navigations.push(commentId); } override updateFeedback(): void { } override hideFeedbackInEditor(_sessionResource: URI, feedbackId: string): void { hiddenFeedbackIds.push(feedbackId); } + override removeFeedback(_sessionResource: URI, feedbackId: string): void { removedFeedbackIds.push(feedbackId); } override addFeedback(sessionResource: URI, resourceUri: URI, range: IRange, text: string, suggestion?: ICodeReviewSuggestion, _context?: IAgentFeedbackContext, sourcePRReviewCommentId?: string, kind: AgentFeedbackKind = AgentFeedbackKind.UserReview, state: AgentFeedbackState = AgentFeedbackState.Accepted, sourcePullRequest?: IFeedbackPullRequest): IAgentFeedback { sourcePullRequests.push(sourcePullRequest); return { @@ -79,6 +84,7 @@ suite('AgentFeedbackEditorWidget', () => { }); services.set(ICodeReviewService, new class extends mock() { override markPRReviewCommentConverted(): void { } + override dismissPRReviewComment(_sessionResource: URI, commentId: string): void { dismissedPRCommentIds.push(commentId); } }); services.set(IMarkdownRendererService, new SyncDescriptor(MarkdownRendererService)); @@ -107,7 +113,7 @@ suite('AgentFeedbackEditorWidget', () => { }; try { - callback({ navigations, hiddenFeedbackIds, sourcePullRequests, domNode: createWidget(), rebuild }); + callback({ navigations, hiddenFeedbackIds, removedFeedbackIds, dismissedPRCommentIds, sourcePullRequests, domNode: createWidget(), rebuild }); } finally { widget?.getDomNode().remove(); store.dispose(); @@ -203,6 +209,28 @@ suite('AgentFeedbackEditorWidget', () => { }, createdComment); }); + test('deleting an Agent Host PR comment suppresses its raw source before removing it', () => { + const prComment: ISessionEditorComment = { + ...comment, + kind: AgentFeedbackKind.PRReview, + state: AgentFeedbackState.Created, + sourcePRReviewCommentId: 'thread-1', + }; + withWidget(({ domNode, dismissedPRCommentIds, removedFeedbackIds }) => { + const deleteButton = [...domNode.querySelectorAll('.agent-feedback-widget-actions-bar .monaco-button')] + .find(button => button.textContent === 'Delete'); + deleteButton?.click(); + + assert.deepStrictEqual({ + dismissedPRCommentIds, + removedFeedbackIds, + }, { + dismissedPRCommentIds: ['thread-1'], + removedFeedbackIds: [prComment.sourceId], + }); + }, prComment); + }); + function prReviewComment(sourcePullRequest: IFeedbackPullRequest): ISessionEditorComment { return { ...comment, diff --git a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackInputWidget.fixture.ts b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackInputWidget.fixture.ts index a157a941dc62a7..4e459174c185b3 100644 --- a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackInputWidget.fixture.ts +++ b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackInputWidget.fixture.ts @@ -22,6 +22,8 @@ import { IAgentFeedbackService } from '../../browser/agentFeedbackService.js'; import { ISession, ISessionFileChange } from '../../../../services/sessions/common/session.js'; import { ComponentFixtureContext, createEditorServices, createTextModel, defineComponentFixture, defineThemedFixtureGroup, registerWorkbenchServices } from '../../../../../workbench/test/browser/componentFixtures/fixtureUtils.js'; import '../../../../../base/browser/ui/codicons/codiconStyles.js'; +import { ICodeReviewService } from '../../../codeReview/browser/codeReviewService.js'; +import { createMockCodeReviewService } from '../../../../../workbench/test/browser/componentFixtures/sessions/mockCodeReviewService.js'; import '../../browser/media/agentFeedbackEditorInput.css'; const sessionResource = URI.parse('vscode-agent-session://fixture/session-1'); @@ -147,6 +149,7 @@ function renderInEditor(context: ComponentFixtureContext): Promise { override readonly onDidChangeNavigation = Event.None; override readonly onDidChangeFeedbackScope = Event.None; override readonly onDidRevealSessionComment = Event.None; + override isAgentHostSession(): boolean { return false; } override getVisibleResolvedFeedbackIds(): ReadonlySet { return new Set(); } @@ -173,6 +176,7 @@ function renderInEditor(context: ComponentFixtureContext): Promise { additionalServices: reg => { registerWorkbenchServices(reg); reg.defineInstance(IAgentFeedbackService, agentFeedbackService); + reg.defineInstance(ICodeReviewService, createMockCodeReviewService()); reg.defineInstance(IContextKeyService, contextKeyService); }, }); diff --git a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackItemsBackend.test.ts b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackItemsBackend.test.ts index b9f14a53ab2881..f7bf798aa6a6bc 100644 --- a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackItemsBackend.test.ts +++ b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackItemsBackend.test.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import assert from 'assert'; -import { Event } from '../../../../../base/common/event.js'; +import { Emitter, Event } from '../../../../../base/common/event.js'; import { IReference } from '../../../../../base/common/lifecycle.js'; import { URI } from '../../../../../base/common/uri.js'; import { mock } from '../../../../../base/test/common/mock.js'; @@ -14,6 +14,7 @@ import { createAgentHostResourceUriMapper } from '../../../../../platform/agentH import { FEEDBACK_ANNOTATION_META_KEY } from '../../../../../platform/agentHost/common/meta/agentFeedbackAnnotations.js'; import { IAgentSubscription } from '../../../../../platform/agentHost/common/state/agentSubscription.js'; import { ActionType, ClientAnnotationsAction } from '../../../../../platform/agentHost/common/state/sessionActions.js'; +import { annotationsReducer } from '../../../../../platform/agentHost/common/state/sessionReducers.js'; import { AnnotationsState, ComponentToState, StateComponents } from '../../../../../platform/agentHost/common/state/sessionState.js'; import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; import { IAgentHostSessionsProvider } from '../../../../common/agentHostSessionsProvider.js'; @@ -22,6 +23,7 @@ import { ISession } from '../../../../services/sessions/common/session.js'; import { ISessionsManagementService } from '../../../../services/sessions/common/sessionsManagement.js'; import { ISessionsProvider } from '../../../../services/sessions/common/sessionsProvider.js'; import { AnnotationsAgentFeedbackItemsBackend } from '../../browser/agentFeedbackItemsBackend.js'; +import { AgentFeedbackKind, AgentFeedbackState } from '../../browser/agentFeedbackService.js'; suite('AnnotationsAgentFeedbackItemsBackend', () => { const store = ensureNoDisposablesAreLeakedInTestSuite(); @@ -120,4 +122,102 @@ suite('AnnotationsAgentFeedbackItemsBackend', () => { }, }); }); + + test('shows local mutations immediately after the annotations snapshot has loaded', () => { + const sessionResource = URI.parse('remote-agent-host:///session'); + const annotationsUri = URI.parse('copilot:///session/annotations'); + const resourceUris = createAgentHostResourceUriMapper('remote-test'); + const existingResource = resourceUris.fromAgentHost(URI.file('existing.ts')); + const addedResource = resourceUris.fromAgentHost(URI.file('added.ts')); + let state: AnnotationsState = { + annotations: [{ + id: 'existing', + origin: { session: sessionResource.toString() }, + resource: resourceUris.toAgentHost(existingResource).toString(), + resolved: false, + entries: [{ id: 'existing:0', text: 'Existing feedback' }], + _meta: { + [FEEDBACK_ANNOTATION_META_KEY]: { + kind: 'user', + state: 'accepted', + sessionResource: sessionResource.toString(), + }, + }, + }], + }; + const onDidChange = store.add(new Emitter()); + const subscription: IAgentSubscription = { + get value() { return state; }, + get verifiedValue() { return state; }, + onDidChange: onDidChange.event, + onWillApplyAction: Event.None, + onDidApplyAction: Event.None, + }; + const connection = new class extends mock() { + override readonly resourceUris = resourceUris; + override getSubscription(): IReference> { + return { + object: subscription as IAgentSubscription, + dispose() { }, + }; + } + override dispatch(_channel: string, action: ClientAnnotationsAction): void { + state = annotationsReducer(state, action, () => { }); + onDidChange.fire(state); + } + }(); + const provider = new class extends mock() { + override getFeedbackAnnotationsChannel() { + return { connection, annotationsUri }; + } + }(); + const session = new class extends mock() { + override readonly providerId = 'agenthost-test'; + override readonly sessionId = 'session'; + }(); + const instantiationService = store.add(new TestInstantiationService()); + instantiationService.stub(ISessionsManagementService, new class extends mock() { + override onDidDeleteSession = Event.None; + override getSession() { return session; } + }); + instantiationService.stub(ISessionsProvidersService, new class extends mock() { + override getProvider(): T { + return provider as unknown as T; + } + }); + const backend = store.add(instantiationService.createInstance(AnnotationsAgentFeedbackItemsBackend)); + const events: string[][] = []; + store.add(backend.onDidChangeItems(resource => events.push(backend.getItems(resource).map(item => item.id)))); + + backend.getItems(sessionResource); + backend.upsert({ + id: 'added', + text: 'Added feedback', + resourceUri: addedResource, + range: { startLineNumber: 1, startColumn: 1, endLineNumber: 1, endColumn: 1 }, + sessionResource, + kind: AgentFeedbackKind.UserReview, + state: AgentFeedbackState.Accepted, + }); + const afterAdd = backend.getItems(sessionResource).map(item => item.id); + backend.remove(sessionResource, 'added'); + const afterRemove = backend.getItems(sessionResource).map(item => item.id); + backend.clear(sessionResource); + + assert.deepStrictEqual({ + afterAdd, + afterRemove, + afterClear: backend.getItems(sessionResource).map(item => item.id), + events, + }, { + afterAdd: ['existing', 'added'], + afterRemove: ['existing'], + afterClear: [], + events: [ + ['existing', 'added'], + ['existing'], + [], + ], + }); + }); }); diff --git a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackService.test.ts b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackService.test.ts index 414f5a3af5e0bf..70979a7804f420 100644 --- a/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackService.test.ts +++ b/src/vs/sessions/contrib/agentFeedback/test/browser/agentFeedbackService.test.ts @@ -11,7 +11,7 @@ import { Range } from '../../../../../editor/common/core/range.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; import { mock } from '../../../../../base/test/common/mock.js'; -import { AGENT_FEEDBACK_NEW_SESSION_RESOURCE, AgentFeedbackKind, AgentFeedbackService, AgentFeedbackState, IAgentFeedbackService } from '../../browser/agentFeedbackService.js'; +import { AGENT_FEEDBACK_NEW_SESSION_RESOURCE, AgentFeedbackKind, AgentFeedbackService, AgentFeedbackState, IAgentFeedbackService, shouldIncludeRawPRReviewComments } from '../../browser/agentFeedbackService.js'; import { getSessionEditorComments } from '../../browser/sessionEditorComments.js'; import { IChatEditingService } from '../../../../../workbench/contrib/chat/common/editing/chatEditingService.js'; import { IChatWidget, IChatWidgetService, IChatAcceptInputOptions, IChatWidgetViewModelChangeEvent } from '../../../../../workbench/contrib/chat/browser/chat.js'; @@ -37,6 +37,31 @@ function feedbackSummary(items: readonly { resourceUri: URI; range: { startLineN return items.map(f => `${f.resourceUri.path}:${f.range.startLineNumber}`); } +suite('AgentFeedbackService - PR review authority', () => { + + ensureNoDisposablesAreLeakedInTestSuite(); + + test('includes raw PR comments until Agent Host feedback has loaded', () => { + let loaded = false; + const service = new class extends mock() { + override isAgentHostSession(): boolean { return true; } + override hasLoadedFeedback(): boolean { return loaded; } + }(); + const session = URI.parse('vscode-agent-session://test/session'); + + const beforeLoad = shouldIncludeRawPRReviewComments(service, session); + loaded = true; + + assert.deepStrictEqual({ + beforeLoad, + afterLoad: shouldIncludeRawPRReviewComments(service, session), + }, { + beforeLoad: true, + afterLoad: false, + }); + }); +}); + suite('AgentFeedbackService - Ordering', () => { const store = new DisposableStore(); diff --git a/src/vs/sessions/contrib/agentFeedback/test/browser/sessionEditorComments.test.ts b/src/vs/sessions/contrib/agentFeedback/test/browser/sessionEditorComments.test.ts index 529c9049c253d6..bed983dd3194bf 100644 --- a/src/vs/sessions/contrib/agentFeedback/test/browser/sessionEditorComments.test.ts +++ b/src/vs/sessions/contrib/agentFeedback/test/browser/sessionEditorComments.test.ts @@ -129,6 +129,18 @@ suite('SessionEditorComments', () => { assert.strictEqual(comments.length, 0); }); + test('omits raw PR review comments when Agent Host annotations are authoritative', () => { + const prState: IPRReviewState = { + kind: PRReviewStateKind.Loaded, + incompletePullRequests: [], + comments: [ + { id: 'pr-thread-1', pullRequest, uri: fileA, range: new Range(5, 1, 5, 1), body: 'Please fix this', author: 'reviewer' }, + ], + }; + + assert.deepStrictEqual(getSessionEditorComments(session, [], prState, undefined, false), []); + }); + test('excludes resolved feedback from the editor comments', () => { const feedback = [ { id: 'feedback-accepted', text: 'accepted', resourceUri: fileA, range: new Range(2, 1, 2, 1), sessionResource: session, kind: AgentFeedbackKind.UserReview, state: AgentFeedbackState.Accepted }, @@ -146,7 +158,7 @@ suite('SessionEditorComments', () => { }); }); - test('hides a created PR-review mirror and shows the raw PR comment instead', () => { + test('shows the Agent Host PR-review annotation and hides the raw PR comment', () => { const prState: IPRReviewState = { kind: PRReviewStateKind.Loaded, incompletePullRequests: [], @@ -158,7 +170,7 @@ suite('SessionEditorComments', () => { { id: 'mirror-1', text: 'Please fix this', resourceUri: fileA, range: new Range(5, 1, 5, 1), sessionResource: session, kind: AgentFeedbackKind.PRReview, sourcePRReviewCommentId: 'pr-thread-1', state: AgentFeedbackState.Created }, ], prState); - assert.deepStrictEqual(comments.map(c => `${c.source}:${c.sourceId}`), ['prReview:pr-thread-1']); + assert.deepStrictEqual(comments.map(c => `${c.source}:${c.sourceId}:${c.state}`), ['agentFeedback:mirror-1:created']); }); test('shows an accepted PR-review mirror and hides the superseded raw PR comment', () => { diff --git a/src/vs/sessions/contrib/changes/test/browser/agentsDiffEditor.fixture.ts b/src/vs/sessions/contrib/changes/test/browser/agentsDiffEditor.fixture.ts index 647788c946cdd3..6962ca6eb51cc3 100644 --- a/src/vs/sessions/contrib/changes/test/browser/agentsDiffEditor.fixture.ts +++ b/src/vs/sessions/contrib/changes/test/browser/agentsDiffEditor.fixture.ts @@ -42,6 +42,8 @@ import { clearAllFeedbackActionId, navigateNextFeedbackActionId, navigatePreviou import { AgentFeedbackKind, AgentFeedbackState, IAgentFeedback, IAgentFeedbackService } from '../../../agentFeedback/browser/agentFeedbackService.js'; import { Menus } from '../../../../browser/menus.js'; import { ISession } from '../../../../services/sessions/common/session.js'; +import { ICodeReviewService } from '../../../codeReview/browser/codeReviewService.js'; +import { createMockCodeReviewService } from '../../../../../workbench/test/browser/componentFixtures/sessions/mockCodeReviewService.js'; const SESSION_RESOURCE = URI.parse('fixture-session://agents-diff'); const MODIFIED_FIRST_RESOURCE = URI.file('/workspace/src/first.ts'); @@ -143,6 +145,7 @@ function createAgentFeedbackService(feedback: readonly IAgentFeedback[] = [], fe override readonly onDidChangeNavigation = Event.None; override readonly onDidChangeFeedbackScope = Event.None; override readonly onDidRevealSessionComment = Event.None; + override isAgentHostSession(): boolean { return false; } override getVisibleResolvedFeedbackIds(): ReadonlySet { return new Set(); } @@ -244,6 +247,7 @@ async function renderAgentsDiffEditor({ container, disposableStore, disposableSt additionalServices: reg => { registerWorkbenchServices(reg); reg.defineInstance(IAgentFeedbackService, agentFeedbackService); + reg.defineInstance(ICodeReviewService, createMockCodeReviewService()); reg.defineInstance(IContextKeyService, createContextKeyService()); reg.define(IMenuService, FixtureAgentFeedbackMenuService); reg.defineInstance(IDecorationsService, new class extends mock() { override onDidChangeDecorations = Event.None; }()); diff --git a/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts b/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts index 958bbe630ce10e..a8f98e2363cf8d 100644 --- a/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts +++ b/src/vs/sessions/contrib/codeReview/browser/codeReviewService.ts @@ -6,15 +6,19 @@ import { Disposable } from '../../../../base/common/lifecycle.js'; import { arrayEquals } from '../../../../base/common/equals.js'; import { autorun, derivedOpts, IObservable, ISettableObservable, observableValue } from '../../../../base/common/observable.js'; -import { isEqual } from '../../../../base/common/resources.js'; +import { isEqual, isEqualOrParent, relativePath } from '../../../../base/common/resources.js'; import { URI } from '../../../../base/common/uri.js'; import { IRange, Range } from '../../../../editor/common/core/range.js'; +import { linesDiffComputers } from '../../../../editor/common/diff/linesDiffComputers.js'; +import { LineRangeMapping } from '../../../../editor/common/diff/rangeMapping.js'; import { createDecorator } from '../../../../platform/instantiation/common/instantiation.js'; import { ILogService } from '../../../../platform/log/common/log.js'; import { IGitHubService } from '../../github/browser/githubService.js'; -import { getGitHubPullRequestRefs, IGitHubPullRequestRef } from '../../../services/sessions/common/session.js'; +import { IGitHubPullRequestReview } from '../../github/common/types.js'; +import { getGitHubPullRequestRefs, IGitHubPullRequestRef, ISessionFileChange } from '../../../services/sessions/common/session.js'; import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; import { ISessionsService } from '../../../services/sessions/browser/sessionsService.js'; +import { isIChatSessionFileChange2 } from '../../../../workbench/contrib/chat/common/chatSessionsService.js'; // --- Types ------------------------------------------------------------------- export interface ICodeReviewSuggestion { @@ -51,6 +55,14 @@ export interface IPRReviewComment { readonly author: string; } +export interface IPRReviewCommentTarget { + readonly pullRequest: IGitHubPullRequestRef; + readonly commitId: string; + readonly path: string; + readonly line: number; + readonly pendingReview: Pick | undefined; +} + // --- Service Interface ------------------------------------------------------- export const ICodeReviewService = createDecorator('codeReviewService'); @@ -63,6 +75,9 @@ export interface ICodeReviewService { * Returns unresolved review comments from every PR associated with the session. */ getPRReviewState(sessionResource: URI): IObservable; + getPRReviewCommentPullRequests(sessionResource: URI, resource: URI): readonly IGitHubPullRequestRef[]; + getPRReviewCommentTargets(sessionResource: URI, resource: URI, range: IRange, currentContent: string, pullRequest?: Pick): Promise; + createPRReviewComment(target: IPRReviewCommentTarget, body: string): Promise; /** * Resolve a PR review thread on GitHub and remove it from local state. @@ -93,12 +108,62 @@ interface IPRSessionReviewData { readonly state: ISettableObservable; } +interface IPRReviewCommentContext { + readonly path: string; + readonly pullRequests: readonly IGitHubPullRequestRef[]; +} + interface IActivePRReviewContext { readonly sessionResource: URI; readonly workingDirectory: URI | undefined; readonly pullRequests: readonly IGitHubPullRequestRef[]; } +export function commentableRightLines(patch: string): ReadonlySet { + const lines = new Set(); + let rightLine: number | undefined; + for (const patchLine of patch.split('\n')) { + const hunk = /^@@ -\d+(?:,\d+)? \+(?\d+)(?:,\d+)? @@/.exec(patchLine); + if (hunk?.groups) { + rightLine = Number(hunk.groups.start); + continue; + } + if (rightLine === undefined || patchLine.startsWith('\\')) { + continue; + } + if (patchLine.startsWith('-')) { + continue; + } + if (patchLine.startsWith('+') || patchLine.startsWith(' ')) { + lines.add(rightLine++); + } + } + return lines; +} + +export function mapCurrentLineToPullRequestLine(pullRequestContent: string, currentContent: string, currentLine: number): number | undefined { + const pullRequestLines = splitLines(pullRequestContent); + const currentLines = splitLines(currentContent); + const diff = linesDiffComputers.getDefault().computeDiff(pullRequestLines, currentLines, { + ignoreTrimWhitespace: false, + maxComputationTimeMs: 1000, + computeMoves: false, + }); + if (diff.hitTimeout) { + return undefined; + } + const unchanged = LineRangeMapping.inverse(diff.changes, pullRequestLines.length, currentLines.length); + const mapping = unchanged.find(mapping => mapping.modified.contains(currentLine)); + if (!mapping) { + return undefined; + } + return mapping.original.startLineNumber + currentLine - mapping.modified.startLineNumber; +} + +function splitLines(content: string): string[] { + return content.split(/\r\n|\r|\n/); +} + export class CodeReviewService extends Disposable implements ICodeReviewService { declare readonly _serviceBrand: undefined; @@ -219,6 +284,108 @@ export class CodeReviewService extends Disposable implements ICodeReviewService return this._getOrCreatePRReviewData(sessionResource).state; } + getPRReviewCommentPullRequests(sessionResource: URI, resource: URI): readonly IGitHubPullRequestRef[] { + return this._getPRReviewCommentContext(sessionResource, resource)?.pullRequests ?? []; + } + + private _getPRReviewCommentContext(sessionResource: URI, resource: URI): IPRReviewCommentContext | undefined { + const session = this._sessionsManagementService.getSession(sessionResource); + const workspace = session?.workspace.get(); + const workspaceResource = this._resolveWorkspaceResource(resource, session?.changes.get()); + const folder = workspace?.folders.find(folder => isEqualOrParent(workspaceResource, folder.workingDirectory)); + const path = folder ? relativePath(folder.workingDirectory, workspaceResource) : undefined; + if (!folder || !path) { + return undefined; + } + + return { + path, + pullRequests: getGitHubPullRequestRefs(folder.gitRepository?.gitHubInfo.get()) + .filter(pullRequest => { + const state = pullRequest.liveState ?? pullRequest.state; + return state === undefined || state === 'open'; + }), + }; + } + + async getPRReviewCommentTargets( + sessionResource: URI, + resource: URI, + range: IRange, + currentContent: string, + pullRequest?: Pick, + ): Promise { + const context = this._getPRReviewCommentContext(sessionResource, resource); + if (!context) { + return []; + } + + const pullRequests = pullRequest + ? context.pullRequests.filter(candidate => candidate.owner === pullRequest.owner && candidate.repo === pullRequest.repo && candidate.number === pullRequest.number) + : context.pullRequests; + const targets = await Promise.all(pullRequests.map(async pullRequest => { + const pullRequestRef = this._gitHubService.createPullRequestModelReference(pullRequest.owner, pullRequest.repo, pullRequest.number); + try { + await pullRequestRef.object.refresh(); + const details = pullRequestRef.object.pullRequest.get(); + if (!details) { + return undefined; + } + const changedFiles = await this._gitHubService.getPullRequestChangedFiles(pullRequest.owner, pullRequest.repo, pullRequest.number); + const changedFile = changedFiles.find(file => file.filename === context.path); + if (!changedFile?.patch) { + return undefined; + } + const headContent = await this._gitHubService.getFileContent(pullRequest.owner, pullRequest.repo, context.path, details.headSha); + const line = mapCurrentLineToPullRequestLine(headContent, currentContent, range.endLineNumber); + if (line === undefined || !commentableRightLines(changedFile.patch).has(line)) { + return undefined; + } + const pendingReview = pullRequestRef.object.reviews.get()?.find(review => review.state === 'PENDING'); + return { + pullRequest, + commitId: details.headSha, + path: context.path, + line, + pendingReview: pendingReview ? { id: pendingReview.id, nodeId: pendingReview.nodeId } : undefined, + }; + } finally { + pullRequestRef.dispose(); + } + })); + return targets.filter(target => target !== undefined); + } + + private _resolveWorkspaceResource(resource: URI, changes: readonly ISessionFileChange[] | undefined): URI { + for (const change of changes ?? []) { + const current = isIChatSessionFileChange2(change) ? change.modifiedUri ?? change.uri : change.modifiedUri; + const candidates = isIChatSessionFileChange2(change) + ? [change.uri, change.modifiedUri, change.originalUri] + : [change.modifiedUri, change.originalUri]; + if (candidates.some(candidate => candidate && (isEqual(candidate, resource) || candidate.fsPath === resource.fsPath))) { + return current; + } + } + return resource; + } + + async createPRReviewComment(target: IPRReviewCommentTarget, body: string): Promise { + const { owner, repo, number } = target.pullRequest; + const pullRequestRef = this._gitHubService.createPullRequestModelReference(owner, repo, number); + try { + await pullRequestRef.object.postReviewComment(body, target.commitId, target.path, target.line, target.pendingReview); + } finally { + pullRequestRef.dispose(); + } + + const reviewThreadsRef = this._gitHubService.createPullRequestReviewThreadsModelReference(owner, repo, number); + try { + await reviewThreadsRef.object.refresh(true); + } finally { + reviewThreadsRef.dispose(); + } + } + async resolvePRReviewThread(sessionResource: URI, threadId: string, pullRequest?: Pick): Promise { const session = this._sessionsManagementService.getSession(sessionResource); const gitHubInfo = session?.workspace.get()?.folders[0]?.gitRepository?.gitHubInfo.get(); 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 f5f6016555973a..3be39112bffdf6 100644 --- a/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts +++ b/src/vs/sessions/contrib/codeReview/test/browser/codeReviewService.test.ts @@ -7,6 +7,7 @@ import assert from 'assert'; import { DeferredPromise } from '../../../../../base/common/async.js'; import { Codicon } from '../../../../../base/common/codicons.js'; import { URI } from '../../../../../base/common/uri.js'; +import { Range } from '../../../../../editor/common/core/range.js'; import { IObservable, constObservable, derived, observableValue } from '../../../../../base/common/observable.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { isIMenuItem, MenuId, MenuRegistry } from '../../../../../platform/actions/common/actions.js'; @@ -25,10 +26,11 @@ import { SessionHasChangesContext, SessionIsCreatedContext, SinglePaneLayoutEnab import { IGitHubService } from '../../../github/browser/githubService.js'; import { GitHubPRFetcher } from '../../../github/browser/fetchers/githubPRFetcher.js'; import { GitHubPullRequestReviewThreadsModel } from '../../../github/browser/models/githubPullRequestReviewThreadsModel.js'; -import { IGitHubPRComment, IGitHubPullRequestReviewThread } from '../../../github/common/types.js'; +import { GitHubPullRequestModel } from '../../../github/browser/models/githubPullRequestModel.js'; +import { GitHubPullRequestState, IGitHubPRComment, IGitHubPullRequestReview, IGitHubPullRequestReviewThread } from '../../../github/common/types.js'; import { SessionChangesEditorInput } from '../../../changes/browser/sessionChangesEditorInput.js'; import { IGitHubInfo, ISession, ISessionWorkspace } from '../../../../services/sessions/common/session.js'; -import { ICodeReviewService, CodeReviewService, PRReviewStateKind } from '../../browser/codeReviewService.js'; +import { commentableRightLines, mapCurrentLineToPullRequestLine, ICodeReviewService, CodeReviewService, PRReviewStateKind } from '../../browser/codeReviewService.js'; import { ISessionsService } from '../../../../services/sessions/browser/sessionsService.js'; import { IActiveSession, ISendRequestOptions, ISessionsChangeEvent, ISessionsManagementService } from '../../../../services/sessions/common/sessionsManagement.js'; import { IChatWidgetService } from '../../../../../workbench/contrib/chat/browser/chat.js'; @@ -176,6 +178,8 @@ suite('CodeReviewService', () => { getPullRequestCalls = 0; getPullRequestReviewThreadsCalls = 0; + readonly failingPullRequestNumbers = new Set(); + readonly postedReviewComments: { owner: string; repo: string; number: number; body: string; commitId: string; path: string; line: number; pendingReview: Pick | undefined }[] = []; override readonly activeSessionPullRequestReviewThreadsObs: IObservable; @@ -218,6 +222,66 @@ suite('CodeReviewService', () => { return new ImmortalReference(this.getReviewThreadsModel(owner, repo, prNumber)); } + override createPullRequestModelReference(owner: string, repo: string, prNumber: number): IReference { + this.getPullRequestCalls++; + const postedReviewComments = this.postedReviewComments; + const shouldFail = this.failingPullRequestNumbers.has(prNumber); + return new ImmortalReference(new class extends mock() { + override readonly pullRequest = constObservable({ + number: prNumber, + title: 'Test PR', + body: '', + state: GitHubPullRequestState.Open, + author: { login: 'author', avatarUrl: '' }, + headRef: 'feature', + headSha: 'abc123', + baseRef: 'main', + isDraft: false, + createdAt: '', + updatedAt: '', + mergedAt: undefined, + mergeable: true, + mergeableState: 'clean', + }); + override readonly reviews = constObservable([{ + id: 42, + nodeId: 'PRR_pending', + author: { login: 'reviewer', avatarUrl: '' }, + state: 'PENDING', + submittedAt: undefined, + }]); + override refresh(): Promise { + return shouldFail ? Promise.reject(new Error('not found')) : Promise.resolve(); + } + override async postReviewComment(body: string, commitId: string, path: string, line: number, pendingReview?: Pick): Promise { + postedReviewComments.push({ owner, repo, number: prNumber, body, commitId, path, line, pendingReview }); + } + }()); + } + + override getPullRequestChangedFiles() { + return Promise.resolve([{ + filename: 'src/a.ts', + previous_filename: undefined, + status: 'modified' as const, + additions: 2, + deletions: 1, + patch: '@@ -3,2 +4,4 @@\n context\n+added\n+also added\n context', + }]); + } + + override getFileContent(): Promise { + return Promise.resolve([ + 'one', + 'two', + 'three', + 'context', + 'added', + 'also added', + 'context', + ].join('\n')); + } + private _key(owner: string, repo: string, prNumber: number): string { return `${owner}/${repo}#${prNumber}`; } @@ -268,9 +332,136 @@ suite('CodeReviewService', () => { legacyThreadRefreshes: 0, reviewThreadRefreshes: 1, }); + } }); + test('creates line-comment targets and posts a PR review comment', async () => { + const workspaceResource = URI.file('/workspace/src/a.ts'); + const virtualResource = URI.parse('git:/workspace/src/a.ts?ref=head'); + sessionsManagement.addSession(session, [{ + uri: workspaceResource, + originalUri: virtualResource, + modifiedUri: workspaceResource, + insertions: 1, + deletions: 0, + }]); + sessionsManagement.setGitHubInfo(session, makeGitHubInfo()); + const currentContent = [ + 'one', + 'two', + 'three', + 'context', + 'added', + 'also added', + 'context', + ].join('\n'); + const pullRequests = service.getPRReviewCommentPullRequests(session, virtualResource); + const pullRequestModelCallsAfterChoices = gitHubService.getPullRequestCalls; + const target = (await service.getPRReviewCommentTargets(session, virtualResource, new Range(4, 1, 7, 1), currentContent))[0]; + assert.ok(target); + + await service.createPRReviewComment(target, 'Please update this.'); + + assert.deepStrictEqual({ + pullRequests: pullRequests.map(pullRequest => ({ + owner: pullRequest.owner, + repo: pullRequest.repo, + number: pullRequest.number, + })), + pullRequestModelCallsAfterChoices, + target: { + pr: target.pullRequest.number, + commitId: target.commitId, + path: target.path, + line: target.line, + pendingReview: target.pendingReview, + }, + outsideTargets: await service.getPRReviewCommentTargets(session, URI.file('/outside/a.ts'), new Range(1, 1, 1, 1), currentContent), + nonDiffTargets: await service.getPRReviewCommentTargets(session, URI.file('/workspace/src/a.ts'), new Range(20, 1, 20, 1), currentContent), + postedReviewComments: gitHubService.postedReviewComments, + threadRefreshes: gitHubService.reviewThreadsFetcher.getReviewThreadsCalls, + }, { + pullRequests: [{ owner: 'owner', repo: 'repo', number: 1 }], + pullRequestModelCallsAfterChoices: 0, + target: { + pr: 1, + commitId: 'abc123', + path: 'src/a.ts', + line: 7, + pendingReview: { id: 42, nodeId: 'PRR_pending' }, + }, + outsideTargets: [], + nonDiffTargets: [], + postedReviewComments: [{ + owner: 'owner', + repo: 'repo', + number: 1, + body: 'Please update this.', + commitId: 'abc123', + path: 'src/a.ts', + line: 7, + pendingReview: { id: 42, nodeId: 'PRR_pending' }, + }], + threadRefreshes: 1, + }); + }); + + test('parses right-side commentable lines from a unified patch', () => { + assert.deepStrictEqual([...commentableRightLines([ + '@@ -2,3 +4,4 @@', + ' context', + '-removed', + '+added', + ' context', + '+last', + '@@ -20 +22 @@', + '-old', + '+new', + ].join('\n'))], [4, 5, 6, 7, 22]); + }); + + test('resolves only the selected pull request comment target', async () => { + const workspaceResource = URI.file('/workspace/src/a.ts'); + sessionsManagement.addSession(session); + sessionsManagement.setGitHubInfo(session, { + ...makeGitHubInfo(), + pullRequests: [1, 2].map(number => ({ + owner: 'owner', + repo: 'repo', + number, + uri: URI.parse(`https://github.com/owner/repo/pull/${number}`), + })), + }); + gitHubService.failingPullRequestNumbers.add(2); + + const targets = await service.getPRReviewCommentTargets( + session, + workspaceResource, + new Range(7, 1, 7, 1), + ['one', 'two', 'three', 'context', 'added', 'also added', 'context'].join('\n'), + { owner: 'owner', repo: 'repo', number: 1 }, + ); + + assert.deepStrictEqual({ + targets: targets.map(target => target.pullRequest.number), + pullRequestModelCalls: gitHubService.getPullRequestCalls, + }, { + targets: [1], + pullRequestModelCalls: 1, + }); + }); + + test('maps unchanged current lines back to the pull request head', () => { + assert.deepStrictEqual({ + shiftedLine: mapCurrentLineToPullRequestLine('one\ntwo\nthree', 'inserted\none\ntwo\nthree', 3), + localOnlyLine: mapCurrentLineToPullRequestLine('one\ntwo\nthree', 'inserted\none\ntwo\nthree', 1), + }, { + shiftedLine: 2, + localOnlyLine: undefined, + }); + }); + test('PR review state combines comments from every associated pull request', async () => { sessionsManagement.addSession(session); sessionsManagement.setGitHubInfo(session, { diff --git a/src/vs/sessions/contrib/github/browser/fetchers/githubChangesFetcher.ts b/src/vs/sessions/contrib/github/browser/fetchers/githubChangesFetcher.ts index 778e66796a0cf3..b85d393c3b96b5 100644 --- a/src/vs/sessions/contrib/github/browser/fetchers/githubChangesFetcher.ts +++ b/src/vs/sessions/contrib/github/browser/fetchers/githubChangesFetcher.ts @@ -13,6 +13,7 @@ interface IGitHubCompareResponse { readonly status: IGitHubChangedFile['status']; readonly additions: number; readonly deletions: number; + readonly patch?: string; }[]; } @@ -38,6 +39,7 @@ export class GitHubChangesFetcher { status: file.status, additions: file.additions, deletions: file.deletions, + patch: file.patch, })) ?? []; } } diff --git a/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts b/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts index adb6a5d674b4b8..e68eae4b417635 100644 --- a/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts +++ b/src/vs/sessions/contrib/github/browser/fetchers/githubPRFetcher.ts @@ -13,6 +13,7 @@ import { IMergeBlocker, MergeBlockerKind, IGitHubPullRequestReviewThread, + IGitHubChangedFile, } from '../../common/types.js'; import { GitHubApiClient, IGitHubApiResponse } from '../githubApiClient.js'; @@ -37,9 +38,19 @@ interface IGitHubPRResponse { interface IGitHubReviewResponse { readonly id: number; + readonly node_id: string; readonly user: { readonly login: string; readonly avatar_url: string }; readonly state: string; - readonly submitted_at: string; + readonly submitted_at: string | null; +} + +interface IGitHubChangedFileResponse { + readonly filename: string; + readonly previous_filename?: string; + readonly status: IGitHubChangedFile['status']; + readonly additions: number; + readonly deletions: number; + readonly patch?: string; } interface IGitHubReviewCommentResponse { @@ -102,6 +113,14 @@ interface IGitHubGraphQLResolveReviewThreadResponse { } | null; } +interface IGitHubGraphQLAddReviewThreadResponse { + readonly addPullRequestReviewThread: { + readonly thread: { + readonly id: string; + } | null; + } | null; +} + //#endregion const GET_REVIEW_THREADS_QUERY = [ @@ -149,6 +168,16 @@ const RESOLVE_REVIEW_THREAD_MUTATION = [ '}', ].join('\n'); +const ADD_REVIEW_THREAD_MUTATION = [ + 'mutation AddReviewThread($reviewId: ID!, $body: String!, $path: String!, $line: Int!) {', + ' addPullRequestReviewThread(input: { pullRequestReviewId: $reviewId, body: $body, path: $path, line: $line, side: RIGHT }) {', + ' thread {', + ' id', + ' }', + ' }', + '}', +].join('\n'); + /** * Stateless fetcher for GitHub pull request data. * Handles all PR-related REST API calls including reviews, comments, and mergeability. @@ -176,21 +205,73 @@ export class GitHubPRFetcher { } async getReviews(owner: string, repo: string, prNumber: number, etag?: string): Promise> { - const response = await this._apiClient.request( - 'GET', - `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}/reviews`, + const response = await this._getPaginatedPullRequestData( + owner, + repo, + prNumber, + 'reviews', 'githubApi.getReviews', - { etag } + etag, ); return { ...response, - data: response.data - ? response.data.map(mapReview) - : undefined + data: response.data?.map(mapReview) }; } + async getChangedFiles(owner: string, repo: string, prNumber: number): Promise { + const response = await this._getPaginatedPullRequestData( + owner, + repo, + prNumber, + 'files', + 'githubApi.getPullRequestChangedFiles', + ); + return response.data?.map(file => ({ + filename: file.filename, + previous_filename: file.previous_filename, + status: file.status, + additions: file.additions, + deletions: file.deletions, + patch: file.patch, + })) ?? []; + } + + private async _getPaginatedPullRequestData( + owner: string, + repo: string, + prNumber: number, + resource: 'files' | 'reviews', + callSite: string, + etag?: string, + ): Promise> { + const perPage = 100; + const firstPage = await this._apiClient.request( + 'GET', + `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}/${resource}?per_page=${perPage}&page=1`, + callSite, + { etag } + ); + if (!firstPage.data || firstPage.statusCode !== 200) { + return firstPage; + } + + const data = [...firstPage.data]; + let pageData = firstPage.data; + for (let page = 2; pageData.length === perPage; page++) { + const response = await this._apiClient.request( + 'GET', + `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}/${resource}?per_page=${perPage}&page=${page}`, + callSite, + ); + pageData = response.data ?? []; + data.push(...pageData); + } + + return { ...firstPage, data }; + } + async getReviewThreads(owner: string, repo: string, prNumber: number): Promise { const data = await this._apiClient.graphql( GET_REVIEW_THREADS_QUERY, @@ -225,6 +306,44 @@ export class GitHubPRFetcher { return mapReviewComment(response.data); } + async postPullRequestReviewComment( + owner: string, + repo: string, + prNumber: number, + body: string, + commitId: string, + path: string, + line: number, + pendingReview?: Pick, + ): Promise { + if (pendingReview) { + const data = await this._apiClient.graphql( + ADD_REVIEW_THREAD_MUTATION, + 'githubApi.addPullRequestReviewThread', + { reviewId: pendingReview.nodeId, body, path, line }, + ); + if (!data.addPullRequestReviewThread?.thread) { + throw new Error(`Failed to add review comment to pending review on ${owner}/${repo}#${prNumber}`); + } + return; + } + + const response = await this._apiClient.request( + 'POST', + `/repos/${e(owner)}/${e(repo)}/pulls/${prNumber}/reviews`, + 'githubApi.postPullRequestReviewComment', + { + data: { + commit_id: commitId, + comments: [{ body, path, line, side: 'RIGHT' }], + } + } + ); + if (!response.data) { + throw new Error(`Failed to post review comment to ${owner}/${repo}#${prNumber}`); + } + } + async postIssueComment( owner: string, repo: string, @@ -356,9 +475,10 @@ function mapPullRequest(data: IGitHubPRResponse): IGitHubPullRequest { function mapReview(data: IGitHubReviewResponse): IGitHubPullRequestReview { return { id: data.id, + nodeId: data.node_id, author: mapUser(data.user), state: data.state, - submittedAt: data.submitted_at, + submittedAt: data.submitted_at ?? undefined, }; } diff --git a/src/vs/sessions/contrib/github/browser/githubService.ts b/src/vs/sessions/contrib/github/browser/githubService.ts index 2e9435efb02805..fa71b96a101b50 100644 --- a/src/vs/sessions/contrib/github/browser/githubService.ts +++ b/src/vs/sessions/contrib/github/browser/githubService.ts @@ -15,12 +15,14 @@ import { GitHubPullRequestReviewThreadsModel, GitHubPullRequestReviewThreadsMode import { GitHubPullRequestCIModel, GitHubPullRequestCIModelReferenceCollection } from './models/githubPullRequestCIModel.js'; import { GitHubIssueModel, GitHubIssueModelReferenceCollection } from './models/githubIssueModel.js'; import { GitHubChangesFetcher } from './fetchers/githubChangesFetcher.js'; +import { GitHubPRFetcher } from './fetchers/githubPRFetcher.js'; import { GitHubRecentUserWorkFetcher, IGitHubRecentIssue, IGitHubRecentPullRequest, IGitHubRecentPullRequestReviewThread } from './fetchers/githubRecentUserWorkFetcher.js'; import { GitHubPullRequestsFetcher } from './fetchers/githubPullRequestsFetcher.js'; import { GitHubPullRequestContextFetcher } from './fetchers/githubPullRequestContextFetcher.js'; import { getPullRequestKey } from '../common/utils.js'; import { derived, derivedOpts, IObservable } from '../../../../base/common/observable.js'; import { structuralEquals } from '../../../../base/common/equals.js'; +import { decodeBase64 } from '../../../../base/common/buffer.js'; import { ISessionsService } from '../../../services/sessions/browser/sessionsService.js'; /** @@ -67,6 +69,8 @@ export interface IGitHubService { * List files changed between two refs using the GitHub compare API. */ getChangedFiles(owner: string, repo: string, base: string, head: string): Promise; + getPullRequestChangedFiles(owner: string, repo: string, pullRequestNumber: number): Promise; + getFileContent(owner: string, repo: string, path: string, ref: string): Promise; /** List one page of open pull requests, ordered by most recently updated. */ getPullRequests(owner: string, repo: string, cursor?: string): Promise; @@ -104,6 +108,7 @@ export class GitHubService extends Disposable implements IGitHubService { private readonly _changesFetcher: GitHubChangesFetcher; private readonly _recentUserWorkFetcher: GitHubRecentUserWorkFetcher; private readonly _pullRequestsFetcher: GitHubPullRequestsFetcher; + private readonly _pullRequestFetcher: GitHubPRFetcher; private readonly _pullRequestContextFetcher: GitHubPullRequestContextFetcher; private readonly _repositoryReferences: GitHubRepositoryModelReferenceCollection; private readonly _pullRequestReferences: GitHubPullRequestModelReferenceCollection; @@ -138,6 +143,7 @@ export class GitHubService extends Disposable implements IGitHubService { this._changesFetcher = new GitHubChangesFetcher(apiClient); this._recentUserWorkFetcher = new GitHubRecentUserWorkFetcher(apiClient); this._pullRequestsFetcher = new GitHubPullRequestsFetcher(apiClient); + this._pullRequestFetcher = new GitHubPRFetcher(apiClient); this._pullRequestContextFetcher = new GitHubPullRequestContextFetcher(apiClient); this._repositoryReferences = instantiationService.createInstance(GitHubRepositoryModelReferenceCollection, apiClient); @@ -255,6 +261,22 @@ export class GitHubService extends Disposable implements IGitHubService { return this._changesFetcher.getChangedFiles(owner, repo, base, head); } + getPullRequestChangedFiles(owner: string, repo: string, pullRequestNumber: number): Promise { + return this._pullRequestFetcher.getChangedFiles(owner, repo, pullRequestNumber); + } + + async getFileContent(owner: string, repo: string, path: string, ref: string): Promise { + const response = await this._apiClient.request<{ content: string; encoding: string }>( + 'GET', + `/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/contents/${path.split('/').map(encodeURIComponent).join('/')}?ref=${encodeURIComponent(ref)}`, + 'githubApi.getFileContent', + ); + if (!response.data || response.data.encoding !== 'base64') { + throw new Error(`GitHub file content not found: ${owner}/${repo}/${path}@${ref}`); + } + return decodeBase64(response.data.content.replace(/\s/g, '')).toString(); + } + getPullRequests(owner: string, repo: string, cursor?: string): Promise { return this._pullRequestsFetcher.getPullRequests(owner, repo, cursor); } diff --git a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts index a3184b75873e1c..71842d60cf43cf 100644 --- a/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts +++ b/src/vs/sessions/contrib/github/browser/models/githubPullRequestModel.ts @@ -92,6 +92,13 @@ export class GitHubPullRequestModel extends Disposable { return this._fetcher.postIssueComment(this.owner, this.repo, this.prNumber, body); } + /** + * Post a review comment on a line in the pull request head. + */ + async postReviewComment(body: string, commitId: string, path: string, line: number, pendingReview?: Pick): Promise { + await this._fetcher.postPullRequestReviewComment(this.owner, this.repo, this.prNumber, body, commitId, path, line, pendingReview); + } + /** * Start periodic polling. Each cycle refreshes all PR data. */ diff --git a/src/vs/sessions/contrib/github/common/types.ts b/src/vs/sessions/contrib/github/common/types.ts index f88873c7996662..11165a5ba7fdee 100644 --- a/src/vs/sessions/contrib/github/common/types.ts +++ b/src/vs/sessions/contrib/github/common/types.ts @@ -43,6 +43,7 @@ export interface IGitHubChangedFile { readonly status: 'added' | 'removed' | 'modified' | 'renamed' | 'copied' | 'changed' | 'unchanged'; readonly additions: number; readonly deletions: number; + readonly patch?: string; } //#endregion @@ -146,9 +147,10 @@ export interface IGitHubPullRequestMergeability { export interface IGitHubPullRequestReview { readonly id: number; + readonly nodeId: string; readonly author: IGitHubUser; readonly state: string; - readonly submittedAt: string; + readonly submittedAt: string | undefined; } /** Coarse pull request state, recoverable from the icon carried on session GitHub info. */ diff --git a/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts b/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts index dabdf8f3d7dd45..737d6de1d2a53e 100644 --- a/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/githubFetchers.test.ts @@ -389,19 +389,157 @@ suite('GitHubPRFetcher', () => { assert.deepStrictEqual(mockApi.graphqlCalls[0].variables, { threadId: 'thread-a' }); }); + test('postPullRequestReviewComment sends a line comment against the head commit', async () => { + mockApi.setNextResponse({ + id: 1, + node_id: 'PRR_review', + user: { login: 'reviewer', avatar_url: '' }, + state: 'COMMENTED', + submitted_at: '2024-01-01T00:00:00Z', + }); + + await fetcher.postPullRequestReviewComment('owner', 'repo', 1, 'Please update this.', 'abc123', 'src/a.ts', 12); + + assert.deepStrictEqual(mockApi.requestCalls, [{ + method: 'POST', + path: '/repos/owner/repo/pulls/1/reviews', + body: { + commit_id: 'abc123', + comments: [{ + body: 'Please update this.', + path: 'src/a.ts', + line: 12, + side: 'RIGHT', + }], + }, + }]); + }); + + test('postPullRequestReviewComment adds to an existing pending review without submitting it', async () => { + mockApi.setNextResponse({ + addPullRequestReviewThread: { thread: { id: 'thread-a' } }, + id: 1, + node_id: 'PRR_review', + user: { login: 'reviewer', avatar_url: '' }, + state: 'COMMENTED', + submitted_at: '2024-01-01T00:00:00Z', + }); + + await fetcher.postPullRequestReviewComment( + 'owner', + 'repo', + 1, + 'Please update this.', + 'abc123', + 'src/a.ts', + 12, + { id: 42, nodeId: 'PRR_pending' }, + ); + + assert.deepStrictEqual({ + graphql: mockApi.graphqlCalls.map(call => call.variables), + requests: mockApi.requestCalls, + }, { + graphql: [{ + reviewId: 'PRR_pending', + body: 'Please update this.', + path: 'src/a.ts', + line: 12, + }], + requests: [], + }); + }); + test('getReviews maps API response', async () => { mockApi.setNextResponse([ - { id: 1, user: { login: 'reviewer', avatar_url: '' }, state: 'APPROVED', submitted_at: '2024-01-01T00:00:00Z' }, - { id: 2, user: { login: 'other', avatar_url: '' }, state: 'CHANGES_REQUESTED', submitted_at: '2024-01-02T00:00:00Z' }, + { id: 1, node_id: 'PRR_1', user: { login: 'reviewer', avatar_url: '' }, state: 'APPROVED', submitted_at: '2024-01-01T00:00:00Z' }, + { id: 2, node_id: 'PRR_2', user: { login: 'other', avatar_url: '' }, state: 'CHANGES_REQUESTED', submitted_at: '2024-01-02T00:00:00Z' }, ]); const reviews = await fetcher.getReviews('owner', 'repo', 1); assert.deepStrictEqual(reviews.data, [ - { id: 1, author: { login: 'reviewer', avatarUrl: '' }, state: 'APPROVED', submittedAt: '2024-01-01T00:00:00Z' }, - { id: 2, author: { login: 'other', avatarUrl: '' }, state: 'CHANGES_REQUESTED', submittedAt: '2024-01-02T00:00:00Z' }, + { id: 1, nodeId: 'PRR_1', author: { login: 'reviewer', avatarUrl: '' }, state: 'APPROVED', submittedAt: '2024-01-01T00:00:00Z' }, + { id: 2, nodeId: 'PRR_2', author: { login: 'other', avatarUrl: '' }, state: 'CHANGES_REQUESTED', submittedAt: '2024-01-02T00:00:00Z' }, ]); assert.strictEqual(mockApi.requestCalls.length, 1); - assert.strictEqual(mockApi.requestCalls[0].path, '/repos/owner/repo/pulls/1/reviews'); + assert.strictEqual(mockApi.requestCalls[0].path, '/repos/owner/repo/pulls/1/reviews?per_page=100&page=1'); + }); + + test('getReviews loads pending reviews after the first page', async () => { + const firstPage = Array.from({ length: 100 }, (_, index) => ({ + id: index + 1, + node_id: `PRR_${index + 1}`, + user: { login: 'reviewer', avatar_url: '' }, + state: 'COMMENTED', + submitted_at: '2024-01-01T00:00:00Z', + })); + mockApi.setResponses(firstPage, [{ + id: 101, + node_id: 'PRR_pending', + user: { login: 'reviewer', avatar_url: '' }, + state: 'PENDING', + submitted_at: null, + }]); + + const reviews = await fetcher.getReviews('owner', 'repo', 1); + + assert.deepStrictEqual({ + count: reviews.data?.length, + pending: reviews.data?.find(review => review.state === 'PENDING'), + paths: mockApi.requestCalls.map(call => call.path), + }, { + count: 101, + pending: { + id: 101, + nodeId: 'PRR_pending', + author: { login: 'reviewer', avatarUrl: '' }, + state: 'PENDING', + submittedAt: undefined, + }, + paths: [ + '/repos/owner/repo/pulls/1/reviews?per_page=100&page=1', + '/repos/owner/repo/pulls/1/reviews?per_page=100&page=2', + ], + }); + }); + + test('getChangedFiles loads files after the first page', async () => { + const firstPage = Array.from({ length: 100 }, (_, index) => ({ + filename: `src/file-${index}.ts`, + status: 'modified', + additions: 1, + deletions: 1, + })); + mockApi.setResponses(firstPage, [{ + filename: 'src/target.ts', + previous_filename: 'src/old-target.ts', + status: 'renamed', + additions: 2, + deletions: 1, + patch: '@@ -1 +1 @@\n-old\n+new', + }]); + + const files = await fetcher.getChangedFiles('owner', 'repo', 1); + + assert.deepStrictEqual({ + count: files.length, + target: files.at(-1), + paths: mockApi.requestCalls.map(call => call.path), + }, { + count: 101, + target: { + filename: 'src/target.ts', + previous_filename: 'src/old-target.ts', + status: 'renamed', + additions: 2, + deletions: 1, + patch: '@@ -1 +1 @@\n-old\n+new', + }, + paths: [ + '/repos/owner/repo/pulls/1/files?per_page=100&page=1', + '/repos/owner/repo/pulls/1/files?per_page=100&page=2', + ], + }); }); test('computeMergeability detects draft blocker', () => { @@ -421,7 +559,7 @@ suite('GitHubPRFetcher', () => { test('computeMergeability detects changes requested blocker', () => { const pr = makePR({ state: GitHubPullRequestState.Open, isDraft: false, mergeable: true, mergeableState: 'clean' }); const reviews: IGitHubPullRequestReview[] = [ - { id: 1, author: { login: 'reviewer', avatarUrl: '' }, state: 'CHANGES_REQUESTED', submittedAt: '2024-01-01T00:00:00Z' }, + { id: 1, nodeId: 'PRR_1', author: { login: 'reviewer', avatarUrl: '' }, state: 'CHANGES_REQUESTED', submittedAt: '2024-01-01T00:00:00Z' }, ]; const result = computeMergeability(pr, reviews); assert.strictEqual(result.canMerge, false); diff --git a/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts b/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts index 95c03606f39ade..d644e8a844415d 100644 --- a/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/githubModels.test.ts @@ -50,6 +50,7 @@ class MockPRFetcher { getPullRequestGate: DeferredPromise | undefined; getReviewThreadsGate: DeferredPromise | undefined; postReviewCommentCalls: { body: string; inReplyTo: number }[] = []; + postPullRequestReviewCommentCalls: { body: string; commitId: string; path: string; line: number; pendingReview: Pick | undefined }[] = []; postIssueCommentCalls: { body: string }[] = []; resolveThreadCalls: { threadId: string }[] = []; @@ -79,6 +80,10 @@ class MockPRFetcher { return makeComment(999, body); } + async postPullRequestReviewComment(_owner: string, _repo: string, _prNumber: number, body: string, commitId: string, path: string, line: number, pendingReview?: Pick): Promise { + this.postPullRequestReviewCommentCalls.push({ body, commitId, path, line, pendingReview }); + } + async postIssueComment(_owner: string, _repo: string, _prNumber: number, body: string): Promise { this.postIssueCommentCalls.push({ body }); return makeComment(998, body); @@ -290,6 +295,18 @@ suite('GitHubPullRequestModel', () => { assert.strictEqual(mockFetcher.postIssueCommentCalls.length, 1); }); + test('postReviewComment uses the pull request head and selected line', async () => { + const model = store.add(new GitHubPullRequestModel('owner', 'repo', 1, mockFetcher as unknown as GitHubPRFetcher, logService)); + mockFetcher.nextPR = makePR(); + mockFetcher.nextReviews = []; + + await model.postReviewComment('Please update this.', 'abc123', 'src/a.ts', 12); + + assert.deepStrictEqual(mockFetcher.postPullRequestReviewCommentCalls, [ + { body: 'Please update this.', commitId: 'abc123', path: 'src/a.ts', line: 12, pendingReview: undefined } + ]); + }); + test('polling can be started and stopped', () => { const model = store.add(new GitHubPullRequestModel('owner', 'repo', 1, mockFetcher as unknown as GitHubPRFetcher, logService)); // Just ensure no errors; actual polling behavior is timer-based diff --git a/src/vs/workbench/contrib/comments/browser/commentsController.ts b/src/vs/workbench/contrib/comments/browser/commentsController.ts index ab9cb872b2cbdb..bfb504ac3c6725 100644 --- a/src/vs/workbench/contrib/comments/browser/commentsController.ts +++ b/src/vs/workbench/contrib/comments/browser/commentsController.ts @@ -48,6 +48,7 @@ import { URI } from '../../../../base/common/uri.js'; import { IUriIdentityService } from '../../../../platform/uriIdentity/common/uriIdentity.js'; import { threadHasMeaningfulComments } from './commentsModel.js'; import { INotificationService } from '../../../../platform/notification/common/notification.js'; +import { IsSessionsWindowContext } from '../../../common/contextkeys.js'; export const ID = 'editor.contrib.review'; @@ -501,7 +502,7 @@ export class CommentController extends Disposable implements IEditorContribution this._activeEditorHasCommentingRange = CommentContextKeys.activeEditorHasCommentingRange.bindTo(contextKeyService); this._commentWidgetVisible = CommentContextKeys.commentWidgetVisible.bindTo(contextKeyService); - if (editor instanceof EmbeddedCodeEditorWidget) { + if (editor instanceof EmbeddedCodeEditorWidget || IsSessionsWindowContext.getValue(contextKeyService)) { return; } diff --git a/src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts b/src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts index 725d18e466940b..1209db58fb1392 100644 --- a/src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts +++ b/src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts @@ -741,6 +741,7 @@ export function createEditorServices(disposables: DisposableStore, options?: Cre acceptFeedback: () => { }, addReply: () => { }, getFeedback: () => [], + isAgentHostSession: () => false, showFeedbackInEditor: () => { }, hideFeedbackInEditor: () => { }, getVisibleResolvedFeedbackIds: () => new Set(),