diff --git a/src/vs/base/browser/ui/list/listView.ts b/src/vs/base/browser/ui/list/listView.ts index ccec24a0b89f07..dd774050479e0f 100644 --- a/src/vs/base/browser/ui/list/listView.ts +++ b/src/vs/base/browser/ui/list/listView.ts @@ -42,6 +42,14 @@ interface IItem { stale: boolean; } +interface IDynamicHeightMeasurement { + readonly item: IItem; + readonly index: number; + readonly previousSize: number; + readonly row: IRow; + rendered: boolean; +} + const StaticDND = { CurrentDragAndDropData: undefined as IDragAndDropData | undefined }; @@ -700,7 +708,9 @@ export class ListView implements IListView { const updateRange = Range.intersect(renderRange, renderedRestRange); for (let i = updateRange.start; i < updateRange.end; i++) { - this.updateItemInDOM(this.items[i], i); + if (this.items[i].row) { + this.updateItemInDOM(this.items[i], i); + } } const removeRanges = Range.relativeComplement(renderedRestRange, renderRange); @@ -964,7 +974,7 @@ export class ListView implements IListView { // DOM operations - private insertItemInDOM(index: number, row?: IRow): void { + private insertItemInDOM(index: number, row?: IRow, alreadyRendered = false): void { const item = this.items[index]; if (!item.row) { @@ -1008,7 +1018,9 @@ export class ListView implements IListView { throw new Error(`No renderer found for template id ${item.templateId}`); } - renderer?.renderElement(item.element, index, item.row.templateData, { height: item.size }); + if (!alreadyRendered) { + renderer.renderElement(item.element, index, item.row.templateData, { height: item.size }); + } const uri = this.dnd.getDragURI(item.element); item.dragStartDisposable.dispose(); @@ -1559,89 +1571,128 @@ export class ListView implements IListView { * to be probed for dynamic height. Adjusts scroll height and top if necessary. */ protected _rerender(renderTop: number, renderHeight: number, inSmoothScrolling?: boolean): void { - const previousRenderRange = this.getRenderRange(renderTop, renderHeight); + let previousRenderRange = this.getRenderRange(renderTop, renderHeight); + const retainedMeasurements = new Map>(); // Let's remember the second element's position, this helps in scrolling up // and preserving a linear upwards scroll movement let anchorElementIndex: number | undefined; let anchorElementTopDelta: number | undefined; - if (renderTop === this.elementTop(previousRenderRange.start)) { - anchorElementIndex = previousRenderRange.start; - anchorElementTopDelta = 0; - } else if (previousRenderRange.end - previousRenderRange.start > 1) { - anchorElementIndex = previousRenderRange.start + 1; - anchorElementTopDelta = this.elementTop(anchorElementIndex) - renderTop; - } + const updateAnchorElement = () => { + anchorElementIndex = undefined; + anchorElementTopDelta = undefined; - let heightDiff = 0; + if (renderTop === this.elementTop(previousRenderRange.start)) { + anchorElementIndex = previousRenderRange.start; + anchorElementTopDelta = 0; + } else if (previousRenderRange.end - previousRenderRange.start > 1) { + anchorElementIndex = previousRenderRange.start + 1; + anchorElementTopDelta = this.elementTop(anchorElementIndex) - renderTop; + } + }; - while (true) { - const renderRange = this.getRenderRange(renderTop, renderHeight); + updateAnchorElement(); - let didChange = false; + let heightDiff = 0; - for (let i = renderRange.start; i < renderRange.end; i++) { - const diff = this.probeDynamicHeight(i); + try { + while (true) { + const renderRange = this.getRenderRange(renderTop, renderHeight); + + let didChange = false; + + const probedItems = this.items.slice(renderRange.start, renderRange.end); + const dynamicHeightDiffs = this.probeDynamicHeights(renderRange, retainedMeasurements); + const modelDidChange = this.items.length < renderRange.end + || probedItems.some((item, index) => item !== this.items[renderRange.start + index]); + if (modelDidChange) { + for (let index = 0; index < probedItems.length; index++) { + const diff = dynamicHeightDiffs[index]; + const currentIndex = this.items.indexOf(probedItems[index]); + if (diff !== 0 && currentIndex !== -1) { + this.rangeMap.splice(currentIndex, 1, [probedItems[index]]); + heightDiff += diff; + } + } - if (diff !== 0) { - this.rangeMap.splice(i, 1, [this.items[i]]); + this.disposeDynamicHeightMeasurements(retainedMeasurements); + previousRenderRange = this.getRenderRange(renderTop, renderHeight); + updateAnchorElement(); + continue; } - heightDiff += diff; - didChange = didChange || diff !== 0; - } + for (let i = renderRange.start; i < renderRange.end; i++) { + const diff = dynamicHeightDiffs[i - renderRange.start]; - if (!didChange) { - if (heightDiff !== 0) { - this.eventuallyUpdateScrollDimensions(); + if (diff !== 0) { + this.rangeMap.splice(i, 1, [this.items[i]]); + } + + heightDiff += diff; + didChange = didChange || diff !== 0; } - const unrenderRanges = Range.relativeComplement(previousRenderRange, renderRange); + if (!didChange) { + if (heightDiff !== 0) { + this.eventuallyUpdateScrollDimensions(); + } - for (const range of unrenderRanges) { - for (let i = range.start; i < range.end; i++) { - if (this.items[i].row) { - this.removeItemFromDOM(i); + const unrenderRanges = Range.relativeComplement(previousRenderRange, renderRange); + + for (const range of unrenderRanges) { + for (let i = range.start; i < range.end; i++) { + if (this.items[i].row) { + this.removeItemFromDOM(i); + } } } - } - const renderRanges = Range.relativeComplement(renderRange, previousRenderRange).reverse(); - const insertedItems: IItem[] = []; + const insertedItems: IItem[] = []; + + for (let i = renderRange.end - 1; i >= renderRange.start; i--) { + const item = this.items[i]; + if (!item.row) { + const measurement = retainedMeasurements.get(i); + const canPromoteMeasurement = measurement?.item === item && measurement.item.templateId === item.templateId; + if (canPromoteMeasurement) { + retainedMeasurements.delete(i); + } + this.insertItemInDOM(i, canPromoteMeasurement ? measurement.row : undefined, canPromoteMeasurement); + insertedItems.push(item); + } + } - for (const range of renderRanges) { - for (let i = range.end - 1; i >= range.start; i--) { - this.insertItemInDOM(i); - insertedItems.push(this.items[i]); + this.disposeDynamicHeightMeasurements(retainedMeasurements); + + if (this.horizontalScrolling && insertedItems.length > 0) { + this.measureItemWidths(insertedItems); + this.eventuallyUpdateScrollWidth(); } - } - if (this.horizontalScrolling && insertedItems.length > 0) { - this.measureItemWidths(insertedItems); - this.eventuallyUpdateScrollWidth(); - } + for (let i = renderRange.start; i < renderRange.end; i++) { + if (this.items[i].row) { + this.updateItemInDOM(this.items[i], i); + } + } - for (let i = renderRange.start; i < renderRange.end; i++) { - if (this.items[i].row) { - this.updateItemInDOM(this.items[i], i); + if (typeof anchorElementIndex === 'number') { + // To compute a destination scroll top, we need to take into account the current smooth scrolling + // animation, and then reuse it with a new target (to avoid prolonging the scroll) + // See https://github.com/microsoft/vscode/issues/104144 + // See https://github.com/microsoft/vscode/pull/104284 + // See https://github.com/microsoft/vscode/issues/107704 + const deltaScrollTop = this.scrollable.getFutureScrollPosition().scrollTop - renderTop; + const newScrollTop = this.elementTop(anchorElementIndex) - anchorElementTopDelta! + deltaScrollTop; + this.setScrollTop(newScrollTop, inSmoothScrolling); } - } - if (typeof anchorElementIndex === 'number') { - // To compute a destination scroll top, we need to take into account the current smooth scrolling - // animation, and then reuse it with a new target (to avoid prolonging the scroll) - // See https://github.com/microsoft/vscode/issues/104144 - // See https://github.com/microsoft/vscode/pull/104284 - // See https://github.com/microsoft/vscode/issues/107704 - const deltaScrollTop = this.scrollable.getFutureScrollPosition().scrollTop - renderTop; - const newScrollTop = this.elementTop(anchorElementIndex) - anchorElementTopDelta! + deltaScrollTop; - this.setScrollTop(newScrollTop, inSmoothScrolling); + this._onDidChangeContentHeight.fire(this.contentHeight); + return; } - - this._onDidChangeContentHeight.fire(this.contentHeight); - return; } + } finally { + this.disposeDynamicHeightMeasurements(retainedMeasurements); } } @@ -1651,22 +1702,12 @@ export class ListView implements IListView { } private probeDynamicHeightForItem(item: IItem, index: number): number { - if (!!this.virtualDelegate.getDynamicHeight) { - const newSize = this.virtualDelegate.getDynamicHeight(item.element); - if (newSize !== null) { - const size = item.size; - item.size = newSize; - item.lastDynamicHeightWidth = this.renderWidth; - this.publishDynamicHeight(item); - return newSize - size; - } - } - - if (!item.hasDynamicHeight || item.lastDynamicHeightWidth === this.renderWidth) { - return 0; + const delegateHeightDiff = this.probeDynamicHeightFromDelegate(item); + if (delegateHeightDiff !== undefined) { + return delegateHeightDiff; } - if (!!this.virtualDelegate.hasDynamicHeight && !this.virtualDelegate.hasDynamicHeight(item.element)) { + if (!this.shouldProbeDynamicHeight(item)) { return 0; } @@ -1709,6 +1750,102 @@ export class ListView implements IListView { return item.size - size; } + private probeDynamicHeights(range: IRange, retainedMeasurements: Map>): number[] { + const diffs = new Array(range.end - range.start).fill(0); + const measurements: IDynamicHeightMeasurement[] = []; + + for (let index = range.start; index < range.end; index++) { + const item = this.items[index]; + const delegateHeightDiff = this.probeDynamicHeightFromDelegate(item); + if (delegateHeightDiff !== undefined) { + diffs[index - range.start] = delegateHeightDiff; + continue; + } + + if (!this.shouldProbeDynamicHeight(item) || retainedMeasurements.has(index)) { + continue; + } + + if (item.row) { + item.row.domNode.style.height = ''; + measurements.push({ item, index, previousSize: item.size, row: item.row, rendered: true }); + continue; + } + + const { row } = this.cache.alloc(item.templateId); + const measurement: IDynamicHeightMeasurement = { item, index, previousSize: item.size, row, rendered: false }; + retainedMeasurements.set(index, measurement); + measurements.push(measurement); + + row.domNode.style.height = ''; + this.rowsContainer.appendChild(row.domNode); + + const renderer = this.renderers.get(item.templateId); + if (!renderer) { + throw new BugIndicatingError('Missing renderer for templateId: ' + item.templateId); + } + + measurement.rendered = true; + renderer.renderElement(item.element, index, row.templateData, { height: item.size }); + } + + for (const measurement of measurements) { + measurement.item.size = measurement.row.domNode.offsetHeight; + } + + for (const measurement of measurements) { + const { item, index, previousSize, row } = measurement; + if (item.size === 0) { + if (!isAncestor(row.domNode, getWindow(row.domNode).document.body)) { + console.warn('Measuring item node that is not in DOM! Add ListView to the DOM before measuring row height!', new Error().stack); + } else { + console.warn('Measured item node at 0px- ensure that ListView is not display:none before measuring row height!', new Error().stack); + } + } + + item.lastDynamicHeightWidth = this.renderWidth; + this.publishDynamicHeight(item); + diffs[index - range.start] = item.size - previousSize; + } + + return diffs; + } + + private disposeDynamicHeightMeasurements(measurements: Map>): void { + for (const [measurementIndex, { item, index, row, rendered }] of measurements) { + measurements.delete(measurementIndex); + try { + if (rendered) { + this.renderers.get(item.templateId)?.disposeElement?.(item.element, index, row.templateData, { height: item.size }); + } + } finally { + row.domNode.remove(); + this.cache.release(row); + } + } + } + + private probeDynamicHeightFromDelegate(item: IItem): number | undefined { + const newSize = this.virtualDelegate.getDynamicHeight?.(item.element); + if (newSize === undefined || newSize === null) { + return undefined; + } + + const size = item.size; + item.size = newSize; + item.lastDynamicHeightWidth = this.renderWidth; + this.publishDynamicHeight(item); + return newSize - size; + } + + private shouldProbeDynamicHeight(item: IItem): boolean { + if (!item.hasDynamicHeight || item.lastDynamicHeightWidth === this.renderWidth) { + return false; + } + + return !this.virtualDelegate.hasDynamicHeight || this.virtualDelegate.hasDynamicHeight(item.element); + } + private publishDynamicHeight(item: IItem): void { if (item.size > 0) { this.virtualDelegate.setDynamicHeight?.(item.element, item.size); diff --git a/src/vs/base/browser/ui/tree/abstractTree.ts b/src/vs/base/browser/ui/tree/abstractTree.ts index 6e9285cec05fcd..0c7cb43b7fb07f 100644 --- a/src/vs/base/browser/ui/tree/abstractTree.ts +++ b/src/vs/base/browser/ui/tree/abstractTree.ts @@ -450,9 +450,11 @@ export class TreeRenderer implements IListR this.renderer.disposeElement?.(node, index, templateData.templateData, { ...details, indent: templateData.indentSize }); - if (typeof details?.height === 'number') { + if (typeof details?.height === 'number' && this.renderedNodes.get(node) === templateData) { this.renderedNodes.delete(node); - this.renderedElements.delete(node.element); + if (this.renderedElements.get(node.element) === node) { + this.renderedElements.delete(node.element); + } } } diff --git a/src/vs/base/test/browser/ui/list/listView.test.ts b/src/vs/base/test/browser/ui/list/listView.test.ts index 1ccb7b0cfe4e94..bb37d185e0053c 100644 --- a/src/vs/base/test/browser/ui/list/listView.test.ts +++ b/src/vs/base/test/browser/ui/list/listView.test.ts @@ -7,6 +7,7 @@ import assert from 'assert'; import { CachedListVirtualDelegate, IListRenderer, IListVirtualDelegate } from '../../../../browser/ui/list/list.js'; import { ListView } from '../../../../browser/ui/list/listView.js'; import { range } from '../../../../common/arrays.js'; +import { IRange } from '../../../../common/range.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../common/utils.js'; suite('ListView', function () { @@ -123,6 +124,347 @@ suite('ListView', function () { } }); + test('batches dynamic height measurements', function () { + const element = document.createElement('div'); + element.style.height = '100px'; + element.style.width = '200px'; + document.body.appendChild(element); + + type TestElement = { height: number }; + const publishedHeights = new Map(); + const delegate: IListVirtualDelegate = { + getHeight() { return 25; }, + getTemplateId() { return 'template'; }, + hasDynamicHeight() { return true; }, + setDynamicHeight(element, height) { publishedHeights.set(element, height); } + }; + + const rows: HTMLElement[] = []; + const heightReads: { renderedRows: number; unconstrainedRows: number }[] = []; + const renderCounts = new Map(); + let disposedElements = 0; + const renderer: IListRenderer = { + templateId: 'template', + renderTemplate(container) { + rows.push(container); + container.style.height = '999px'; + Object.defineProperty(container, 'offsetHeight', { + configurable: true, + get: () => { + heightReads.push({ + renderedRows: rows.filter(row => row.dataset.testHeight !== undefined).length, + unconstrainedRows: rows.filter(row => row.style.height === '').length + }); + return Number(container.dataset.testHeight); + } + }); + return container; + }, + renderElement(element, _index, templateData) { + templateData.dataset.testHeight = String(element.height); + renderCounts.set(element, (renderCounts.get(element) ?? 0) + 1); + }, + disposeElement() { + disposedElements++; + }, + disposeTemplate() { } + }; + + const elements: TestElement[] = range(10).map(() => ({ height: 5 })); + const listView = new ListView(element, delegate, [renderer], { supportDynamicHeights: true }); + try { + listView.layout(100, 200); + listView.splice(0, 0, elements); + + assert.deepStrictEqual({ + heightReads, + publishedHeights: elements.map(element => publishedHeights.get(element)), + renderCounts: elements.map(element => renderCounts.get(element)), + renderedRows: elements.map((_element, index) => listView.domElement(index) !== null), + disposedElements + }, { + heightReads: [ + { renderedRows: 4, unconstrainedRows: 4 }, + { renderedRows: 4, unconstrainedRows: 4 }, + { renderedRows: 4, unconstrainedRows: 4 }, + { renderedRows: 4, unconstrainedRows: 4 }, + { renderedRows: 8, unconstrainedRows: 8 }, + { renderedRows: 8, unconstrainedRows: 8 }, + { renderedRows: 8, unconstrainedRows: 8 }, + { renderedRows: 8, unconstrainedRows: 8 }, + { renderedRows: 10, unconstrainedRows: 10 }, + { renderedRows: 10, unconstrainedRows: 10 }, + ], + publishedHeights: [5, 5, 5, 5, 5, 5, 5, 5, 5, 5], + renderCounts: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1], + renderedRows: [true, true, true, true, true, true, true, true, true, true], + disposedElements: 0 + }); + } finally { + listView.dispose(); + element.remove(); + } + }); + + test('cleans up retained dynamic height rows after a render error', function () { + const element = document.createElement('div'); + element.style.height = '100px'; + element.style.width = '200px'; + document.body.appendChild(element); + + type TestElement = { height: number; throwOnRender?: boolean }; + const delegate: IListVirtualDelegate = { + getHeight() { return 100; }, + getTemplateId() { return 'template'; }, + hasDynamicHeight() { return true; } + }; + + let disposedElements = 0; + const renderer: IListRenderer = { + templateId: 'template', + renderTemplate(container) { + Object.defineProperty(container, 'offsetHeight', { + configurable: true, + get: () => Number(container.dataset.testHeight) + }); + return container; + }, + renderElement(element, _index, templateData) { + templateData.dataset.testHeight = String(element.height); + if (element.throwOnRender) { + throw new Error('render failed'); + } + }, + disposeElement() { + disposedElements++; + }, + disposeTemplate() { } + }; + + const listView = new ListView(element, delegate, [renderer], { supportDynamicHeights: true }); + try { + listView.layout(100, 200); + assert.throws(() => listView.splice(0, 0, [ + { height: 20 }, + { height: 20, throwOnRender: true }, + ]), /render failed/); + assert.deepStrictEqual({ + rowsInDom: element.querySelectorAll('.monaco-list-row').length, + disposedElements + }, { + rowsInDom: 1, + disposedElements: 1 + }); + } finally { + listView.dispose(); + element.remove(); + } + }); + + test('does not promote a retained row after a reentrant splice', function () { + const element = document.createElement('div'); + element.style.height = '100px'; + element.style.width = '200px'; + document.body.appendChild(element); + + type TestElement = { id: string; height: number }; + const delegate: IListVirtualDelegate = { + getHeight() { return 100; }, + getTemplateId() { return 'template'; }, + hasDynamicHeight() { return true; } + }; + + const listViewRef: { value?: ListView } = {}; + let spliceOnRender: TestElement | undefined; + const renderer: IListRenderer = { + templateId: 'template', + renderTemplate(container) { + Object.defineProperty(container, 'offsetHeight', { + configurable: true, + get: () => Number(container.dataset.testHeight) + }); + return container; + }, + renderElement(element, _index, templateData) { + templateData.textContent = element.id; + templateData.dataset.testHeight = String(element.height); + if (spliceOnRender === element) { + spliceOnRender = undefined; + listViewRef.value!.splice(0, 1); + } + }, + disposeTemplate() { } + }; + + const elements: TestElement[] = range(10).map(index => ({ id: String(index), height: 100 })); + const listView = listViewRef.value = new ListView(element, delegate, [renderer], { supportDynamicHeights: true }); + try { + listView.layout(100, 200); + listView.splice(0, 0, elements); + elements[0].height = 20; + elements[1].height = 20; + listView.domElement(0)!.dataset.testHeight = String(elements[0].height); + spliceOnRender = elements[1]; + + listView.layout(100, 201); + + const renderedRows = range(listView.length) + .filter(index => listView.domElement(index) !== null) + .map(index => ({ + element: listView.element(index).id, + rendered: listView.domElement(index)!.textContent + })); + assert.deepStrictEqual({ + length: listView.length, + renderedRows + }, { + length: elements.length - 1, + renderedRows: [ + { element: '1', rendered: '1' }, + { element: '2', rendered: '2' } + ] + }); + } finally { + listView.dispose(); + element.remove(); + } + }); + + test('preserves delegated height measurements after a reentrant replacement', function () { + const element = document.createElement('div'); + element.style.height = '100px'; + element.style.width = '200px'; + document.body.appendChild(element); + + type TestElement = { id: string; height: number; delegated?: boolean }; + const delegate: IListVirtualDelegate = { + getHeight() { return 100; }, + getTemplateId() { return 'template'; }, + hasDynamicHeight() { return true; }, + getDynamicHeight(element) { return element.delegated ? element.height : null; } + }; + + const listViewRef: { value?: ListView } = {}; + let replaceOnRender: TestElement | undefined; + const replacement: TestElement = { id: 'replacement', height: 100 }; + const renderer: IListRenderer = { + templateId: 'template', + renderTemplate(container) { + Object.defineProperty(container, 'offsetHeight', { + configurable: true, + get: () => Number(container.dataset.testHeight) + }); + return container; + }, + renderElement(element, index, templateData) { + templateData.textContent = element.id; + templateData.dataset.testHeight = String(element.height); + if (replaceOnRender === element) { + replaceOnRender = undefined; + listViewRef.value!.splice(index, 1, [replacement]); + } + }, + disposeTemplate() { } + }; + + const elements: TestElement[] = [ + { id: 'delegated', height: 100, delegated: true }, + { id: 'dom', height: 100 }, + { id: 'last', height: 100 } + ]; + const listView = listViewRef.value = new class extends ListView { + includeSecond = false; + + protected override getRenderRange(renderTop: number, renderHeight: number): IRange { + const renderRange = super.getRenderRange(renderTop, renderHeight); + if (this.includeSecond) { + renderRange.end = Math.min(this.length, Math.max(renderRange.end, 2)); + } + return renderRange; + } + }(element, delegate, [renderer], { supportDynamicHeights: true }); + try { + listView.layout(100, 200); + listView.splice(0, 0, elements); + elements[0].height = 20; + listView.includeSecond = true; + replaceOnRender = elements[1]; + + listView.layout(100, 201); + + assert.deepStrictEqual({ + contentHeight: listView.contentHeight, + elementHeights: range(listView.length).map(index => listView.elementHeight(index)), + elements: range(listView.length).map(index => listView.element(index).id) + }, { + contentHeight: 220, + elementHeights: [20, 100, 100], + elements: ['delegated', 'replacement', 'last'] + }); + } finally { + listView.dispose(); + element.remove(); + } + }); + + test('handles removing all items during dynamic height measurement', function () { + const element = document.createElement('div'); + element.style.height = '100px'; + element.style.width = '200px'; + document.body.appendChild(element); + + type TestElement = { id: string; height: number }; + const delegate: IListVirtualDelegate = { + getHeight() { return 100; }, + getTemplateId() { return 'template'; }, + hasDynamicHeight() { return true; } + }; + + const listViewRef: { value?: ListView } = {}; + let removeAllOnRender: TestElement | undefined; + const renderer: IListRenderer = { + templateId: 'template', + renderTemplate(container) { + Object.defineProperty(container, 'offsetHeight', { + configurable: true, + get: () => Number(container.dataset.testHeight) + }); + return container; + }, + renderElement(element, _index, templateData) { + templateData.dataset.testHeight = String(element.height); + if (removeAllOnRender === element) { + removeAllOnRender = undefined; + listViewRef.value!.splice(0, listViewRef.value!.length); + } + }, + disposeTemplate() { } + }; + + const elements: TestElement[] = range(3).map(index => ({ id: String(index), height: 100 })); + const listView = listViewRef.value = new ListView(element, delegate, [renderer], { supportDynamicHeights: true }); + try { + listView.layout(100, 200); + listView.splice(0, 0, elements); + elements[0].height = 20; + listView.domElement(0)!.dataset.testHeight = String(elements[0].height); + removeAllOnRender = elements[1]; + + listView.layout(100, 201); + + assert.deepStrictEqual({ + length: listView.length, + rowsInDom: element.querySelectorAll('.monaco-list-row').length + }, { + length: 0, + rowsInDom: 0 + }); + } finally { + listView.dispose(); + element.remove(); + } + }); + test('publishes freshly measured dynamic heights', function () { const element = document.createElement('div'); element.style.height = '200px'; diff --git a/src/vs/base/test/browser/ui/tree/objectTree.test.ts b/src/vs/base/test/browser/ui/tree/objectTree.test.ts index c06bc33e7374bb..1dcbf78f712867 100644 --- a/src/vs/base/test/browser/ui/tree/objectTree.test.ts +++ b/src/vs/base/test/browser/ui/tree/objectTree.test.ts @@ -5,9 +5,13 @@ import assert from 'assert'; import { IIdentityProvider, IListVirtualDelegate } from '../../../../browser/ui/list/list.js'; +import { TreeRenderer } from '../../../../browser/ui/tree/abstractTree.js'; import { ICompressedTreeNode } from '../../../../browser/ui/tree/compressedObjectTreeModel.js'; import { CompressibleObjectTree, ICompressibleTreeRenderer, ObjectTree } from '../../../../browser/ui/tree/objectTree.js'; +import { ObjectTreeModel } from '../../../../browser/ui/tree/objectTreeModel.js'; import { ITreeNode, ITreeRenderer } from '../../../../browser/ui/tree/tree.js'; +import { Emitter, Event } from '../../../../common/event.js'; +import { SetMap } from '../../../../common/map.js'; import { runWithFakedTimers } from '../../../common/timeTravelScheduler.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../common/utils.js'; @@ -237,6 +241,52 @@ suite('ObjectTree', function () { } }); + test('disposing an older render preserves the current node mapping', function () { + const onDidChangeTwistieState = new Emitter(); + const renderer: ITreeRenderer = { + templateId: 'default', + onDidChangeTwistieState: onDidChangeTwistieState.event, + renderTemplate() { }, + renderElement() { }, + renderTwistie(_element, twistieElement) { + twistieElement.dataset.renderCount = String(Number(twistieElement.dataset.renderCount ?? 0) + 1); + return true; + }, + disposeTemplate() { } + }; + const model = new ObjectTreeModel('test'); + model.setChildren(null, [{ element: 1 }]); + const treeRenderer = new TreeRenderer( + renderer, + model, + model.onDidChangeCollapseState, + { elements: [], onDidChange: Event.None }, + new SetMap, HTMLDivElement>() + ); + + try { + const node = model.getNode(1); + const firstTemplate = treeRenderer.renderTemplate(document.createElement('div')); + const secondTemplate = treeRenderer.renderTemplate(document.createElement('div')); + treeRenderer.renderElement(node, 0, firstTemplate, { height: 100 }); + treeRenderer.renderElement(node, 0, secondTemplate, { height: 100 }); + + treeRenderer.disposeElement(node, 0, firstTemplate, { height: 100 }); + onDidChangeTwistieState.fire(1); + + assert.deepStrictEqual({ + firstRenderCount: firstTemplate.twistie.dataset.renderCount, + secondRenderCount: secondTemplate.twistie.dataset.renderCount + }, { + firstRenderCount: '1', + secondRenderCount: '2' + }); + } finally { + treeRenderer.dispose(); + onDidChangeTwistieState.dispose(); + } + }); + class IdentityProvider implements IIdentityProvider { getId(element: number): { toString(): string } { return `${element % 100}`; diff --git a/src/vs/platform/agentHost/node/agentSideEffects.ts b/src/vs/platform/agentHost/node/agentSideEffects.ts index d1dbf16761b1f4..1f2fdfecdcbb21 100644 --- a/src/vs/platform/agentHost/node/agentSideEffects.ts +++ b/src/vs/platform/agentHost/node/agentSideEffects.ts @@ -982,7 +982,10 @@ export class AgentSideEffects extends Disposable { } } if (action.type === ActionType.ChatUsage) { - this._turnTracker.updateBilledNanoAiu(sessionKey, action.turnId, readUsageInfoMeta(action.usage).copilotUsage?.totalNanoAiu); + // Subagent charges are already folded into the parent turn's aggregate. + if (!isSubagentChatUri(sessionKey)) { + this._turnTracker.updateBilledNanoAiu(sessionKey, action.turnId, readUsageInfoMeta(action.usage).copilotUsage?.totalNanoAiu); + } if (action.usage.model && agent) { const modelContext = this._getModelTelemetryContext(agent, action.usage.model); this._turnTracker.updateModel(sessionKey, action.turnId, modelContext.model, modelContext.modelTelemetryKind); diff --git a/src/vs/platform/agentHost/test/node/agentHostTurnTelemetry.test.ts b/src/vs/platform/agentHost/test/node/agentHostTurnTelemetry.test.ts index d9be775e7f2a28..992e93ec995154 100644 --- a/src/vs/platform/agentHost/test/node/agentHostTurnTelemetry.test.ts +++ b/src/vs/platform/agentHost/test/node/agentHostTurnTelemetry.test.ts @@ -384,6 +384,31 @@ suite('AgentSideEffects — turn tracker telemetry', () => { assert.strictEqual((completedEvents()[0].data as Record).billedNanoAiu, undefined); }); + test('attributes billed nano-AIU only to the parent turn, which already includes subagent cost', () => { + setupSession(); + const subagentChatUri = buildSubagentChatUri(sessionUri, 'tool-call-1'); + stateManager.addChat(sessionKey, subagentChatUri); + + startTurn('turn-parent'); + startTurn('turn-subagent', 'hello', undefined, subagentChatUri); + + // The parent aggregate already folds in the subagent's charge; the + // subagent chat additionally reports its own component. + fire({ type: ActionType.ChatUsage, turnId: 'turn-parent', usage: { _meta: { copilotUsage: { totalNanoAiu: 3_000_000_000 } } } }); + fire({ type: ActionType.ChatUsage, turnId: 'turn-subagent', usage: { _meta: { copilotUsage: { totalNanoAiu: 1_000_000_000 } } } }, subagentChatUri); + + fire({ type: ActionType.ChatTurnComplete, turnId: 'turn-subagent', duration: 1000 }, subagentChatUri); + fire({ type: ActionType.ChatTurnComplete, turnId: 'turn-parent', duration: 1000 }); + + assert.deepStrictEqual(completedEvents().map(event => { + const data = event.data as Record; + return { turnId: data.turnId, isSubagentSession: data.isSubagentSession, billedNanoAiu: data.billedNanoAiu }; + }), [ + { turnId: 'turn-subagent', isSubagentSession: true, billedNanoAiu: undefined }, + { turnId: 'turn-parent', isSubagentSession: false, billedNanoAiu: 3_000_000_000 }, + ]); + }); + test('emits result=cancelled on ChatTurnCancelled', () => { setupSession(); startTurn('turn-1', 'hello', 'auto'); diff --git a/src/vs/platform/github/common/githubHostCapabilitiesService.ts b/src/vs/platform/github/common/githubHostCapabilitiesService.ts index 2b1b65597acf3e..ed2256336b37cc 100644 --- a/src/vs/platform/github/common/githubHostCapabilitiesService.ts +++ b/src/vs/platform/github/common/githubHostCapabilitiesService.ts @@ -17,10 +17,16 @@ const unavailableCapabilities: GitHubHostCapabilities = { checkContextRequiredness: false, }; +/** + * GitHub rejects any query that selects `__Type.fields` more than twice with an + * `INTROSPECTION_LIMIT_EXCEEDED` error, so the two `fields` selections below are the entire budget. + * `RequirableByPullRequest` therefore only gets an existence check: `isRequired` is the sole field + * of that interface, so the interface being present implies the field is available. + */ const capabilitiesQuery = `query AgentHostGitHubCapabilities { pullRequest: __type(name: "PullRequest") { fields { name } } repository: __type(name: "Repository") { fields { name } } - requirableByPullRequest: __type(name: "RequirableByPullRequest") { fields { name } } + requirableByPullRequest: __type(name: "RequirableByPullRequest") { name } rateLimit { limit remaining used resetAt } }`; @@ -31,7 +37,7 @@ interface ITypeFields { interface ICapabilitiesProbe { readonly pullRequest?: ITypeFields; readonly repository?: ITypeFields; - readonly requirableByPullRequest?: ITypeFields; + readonly requirableByPullRequest?: { readonly name?: string }; } interface ICapabilitiesProbeResult { @@ -135,24 +141,33 @@ export class GitHubHostCapabilitiesService extends Disposable implements IGitHub 'enrichment', ); if (response.errors.length > 0) { + const schemaValidation = response.errors.every(isSchemaValidationError); + const detail = response.errors.map(formatGraphQLError).join('; '); + if (schemaValidation) { + this._logService?.debug(`[GitHubHostCapabilitiesService] Host ${credential.account.host} lacks the expected GraphQL schema, disabling GraphQL capabilities: ${detail}`); + } else { + // Disabling GraphQL forces every pull request fragment onto incomplete REST fallbacks, + // which silently stalls Agent Merge, so an unexpected probe error must be visible. + this._logService?.warn(`[GitHubHostCapabilitiesService] Capability probe for ${credential.account.host} returned errors, disabling GraphQL capabilities: ${detail}`); + } return { capabilities: unavailableCapabilities, - cache: response.errors.every(isSchemaValidationError), + cache: schemaValidation, }; } if (!response.data?.pullRequest) { + this._logService?.warn(`[GitHubHostCapabilitiesService] Capability probe for ${credential.account.host} did not return the PullRequest type, disabling GraphQL capabilities`); return { capabilities: unavailableCapabilities, cache: false }; } const pullRequestFields = fieldNames(response.data.pullRequest); const repositoryFields = fieldNames(response.data.repository); - const requirableFields = fieldNames(response.data.requirableByPullRequest); return { capabilities: { graphql: true, mergeQueue: pullRequestFields.has('mergeQueueEntry') && repositoryFields.has('mergeQueue'), internalMergeStatus: false, reviewThreads: pullRequestFields.has('reviewThreads'), - checkContextRequiredness: requirableFields.has('isRequired'), + checkContextRequiredness: typeof response.data.requirableByPullRequest?.name === 'string', }, cache: true, }; @@ -172,6 +187,11 @@ function capabilityErrorKind(error: unknown): string { return error instanceof Error ? error.name : typeof error; } +function formatGraphQLError(error: GitHubGraphQLError): string { + const kind = error.type ?? error.extensions?.code; + return kind ? `${kind}: ${error.message ?? 'no message'}` : error.message ?? 'unknown error'; +} + function waitForCapabilities(promise: Promise, signal: AbortSignal): Promise { return new Promise((resolve, reject) => { const onAbort = () => reject(signal.reason); diff --git a/src/vs/platform/github/common/githubTransport.ts b/src/vs/platform/github/common/githubTransport.ts index 5172204ed4b6cd..4a54e7dee81daa 100644 --- a/src/vs/platform/github/common/githubTransport.ts +++ b/src/vs/platform/github/common/githubTransport.ts @@ -438,16 +438,30 @@ export class GitHubTransport extends Disposable implements IGitHubTransport { this._rateLimits.updateFromResponse(account, response, body); this._logRateLimit(account, response.headers.get('x-ratelimit-resource') ?? 'core'); if (response.status === 304) { - if (!cached || response.headers.get('etag') !== null && response.headers.get('etag') !== cached.etag) { - throw new GitHubRequestError('GitHub returned 304 without the exact cached representation', 'malformedResponse', 304); + if (!cached) { + throw new GitHubRequestError('GitHub returned 304 without a cached representation', 'malformedResponse', 304); } + // A 304 confirms the cached body is current, but the validator itself may be reissued + // (for example a strong tag echoed as weak). Adopt it so the next revalidation sends + // the validator GitHub last handed out instead of resending a stale one forever. + const revalidatedEtag = response.headers.get('etag') ?? cached.etag; + const revalidatedLink = response.headers.get('link') ?? cached.link; + if (revalidatedEtag !== cached.etag) { + this._logService?.trace(`[GitHubTransport] Adopting reissued validator for ${operation}`); + } + this._restCache.set(cacheKey, { + ...cached, + etag: revalidatedEtag, + link: revalidatedLink, + fetchedAt: this._scheduler.now(), + }); this._logService?.trace(`[GitHubTransport] Reused cached representation for ${operation}`); return { data: this._parseJson(cached.body, 'Cached GitHub response was not valid JSON'), statusCode: 304, - etag: cached.etag, + etag: revalidatedEtag, finalUrl: cached.finalUrl, - link: cached.link, + link: revalidatedLink, observedAt: this._scheduler.now(), }; } diff --git a/src/vs/platform/github/test/node/githubHostCapabilitiesService.test.ts b/src/vs/platform/github/test/node/githubHostCapabilitiesService.test.ts index 82532dc11b76ed..5bdb5b47214fac 100644 --- a/src/vs/platform/github/test/node/githubHostCapabilitiesService.test.ts +++ b/src/vs/platform/github/test/node/githubHostCapabilitiesService.test.ts @@ -6,11 +6,20 @@ import assert from 'assert'; import { DeferredPromise } from '../../../../base/common/async.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; +import { NullLogService } from '../../../log/common/log.js'; import { GitHubHostCapabilitiesService } from '../../common/githubHostCapabilitiesService.js'; import { GitHubTransport } from '../../common/githubTransport.js'; import { nodeFetch } from './nodeFetch.js'; import { gitHubGraphQLResponse, gitHubGraphQLStep, ProgrammableGitHubServer } from './programmableGitHubServer.js'; +class RecordingLogService extends NullLogService { + readonly warnings: string[] = []; + + override warn(message: string): void { + this.warnings.push(message); + } +} + suite('GitHubHostCapabilitiesService', () => { const disposables = ensureNoDisposablesAreLeakedInTestSuite(); @@ -30,7 +39,7 @@ suite('GitHubHostCapabilitiesService', () => { response: gitHubGraphQLResponse({ pullRequest: { fields: [{ name: 'mergeQueueEntry' }, { name: 'reviewThreads' }] }, repository: { fields: [{ name: 'mergeQueue' }] }, - requirableByPullRequest: { fields: [{ name: 'isRequired' }] }, + requirableByPullRequest: { name: 'RequirableByPullRequest' }, }), })); const transport = disposables.add(new GitHubTransport(nodeFetch)); @@ -65,6 +74,34 @@ suite('GitHubHostCapabilitiesService', () => { }); }); + test('stays within the GitHub introspection budget', async () => { + await withServer(async server => { + server.enqueue(gitHubGraphQLStep({ + response: gitHubGraphQLResponse({ + pullRequest: { fields: [{ name: 'reviewThreads' }] }, + repository: { fields: [] }, + requirableByPullRequest: { name: 'RequirableByPullRequest' }, + }), + })); + const transport = disposables.add(new GitHubTransport(nodeFetch)); + const service = disposables.add(new GitHubHostCapabilitiesService(transport, server.createEndpointService())); + const signal = new AbortController().signal; + + await service.getCapabilities({ + account: { host: new URL(server.apiBaseUrl).host, accountId: '101' }, + token: 'token', + generation: 1, + signal, + }, undefined, signal); + + // GitHub rejects a query that selects `__Type.fields` more than twice with + // INTROSPECTION_LIMIT_EXCEEDED, which would silently disable every GraphQL capability. + const query = server.requests[0].graphQl?.query ?? ''; + assert.strictEqual(query.match(/\bfields\b/g)?.length, 2); + server.assertSatisfied(); + }); + }); + test('fails closed when the schema probe returns errors', async () => { await withServer(async server => { server.enqueue(gitHubGraphQLStep({ @@ -92,6 +129,37 @@ suite('GitHubHostCapabilitiesService', () => { }); }); + test('warns when an unexpected probe error disables GraphQL capabilities', async () => { + await withServer(async server => { + server.enqueue(gitHubGraphQLStep({ + response: gitHubGraphQLResponse(undefined, [{ + message: 'Introspection fields may only be used 2 times, but some fields were used more than that: __Type.fields (3)', + type: 'INTROSPECTION_LIMIT_EXCEEDED', + }]), + })); + const transport = disposables.add(new GitHubTransport(nodeFetch)); + const logService = disposables.add(new RecordingLogService()); + const service = disposables.add(new GitHubHostCapabilitiesService(transport, server.createEndpointService(), logService)); + const signal = new AbortController().signal; + + const result = await service.getCapabilities({ + account: { host: new URL(server.apiBaseUrl).host, accountId: '101' }, + token: 'token', + generation: 1, + signal, + }, undefined, signal); + + assert.deepStrictEqual({ + graphql: result.graphql, + warnings: logService.warnings.map(warning => warning.includes('INTROSPECTION_LIMIT_EXCEEDED')), + }, { + graphql: false, + warnings: [true], + }); + server.assertSatisfied(); + }); + }); + test('does not infer requiredness from the status-check union', async () => { await withServer(async server => { server.enqueue(gitHubGraphQLStep({ @@ -133,7 +201,7 @@ suite('GitHubHostCapabilitiesService', () => { response: gitHubGraphQLResponse({ pullRequest: { fields: [{ name: 'reviewThreads' }] }, repository: { fields: [] }, - requirableByPullRequest: { fields: [] }, + requirableByPullRequest: null, }), }), ); @@ -185,7 +253,7 @@ suite('GitHubHostCapabilitiesService', () => { response: gitHubGraphQLResponse({ pullRequest: { fields: [{ name: 'reviewThreads' }] }, repository: { fields: [] }, - requirableByPullRequest: { fields: [] }, + requirableByPullRequest: null, }), })); const transport = disposables.add(new GitHubTransport(nodeFetch)); diff --git a/src/vs/platform/github/test/node/githubTransport.test.ts b/src/vs/platform/github/test/node/githubTransport.test.ts index f1f1834a9e8e0f..48edc2e2d7c51f 100644 --- a/src/vs/platform/github/test/node/githubTransport.test.ts +++ b/src/vs/platform/github/test/node/githubTransport.test.ts @@ -123,6 +123,66 @@ suite('GitHubTransport', () => { }); }); + test('adopts a reissued validator when revalidating with a 304', async () => { + await withServer(async server => { + server.enqueue( + gitHubRestStep({ method: 'GET', path: '/repos/o/r/pulls', response: gitHubJsonResponse([{ number: 1 }], { etag: '"old"', link: '; rel="next"' }) }), + gitHubRestStep({ + method: 'GET', + path: '/repos/o/r/pulls', + assert: request => assert.strictEqual(request.headers['if-none-match'], '"old"'), + // GitHub may answer a conditional request with a reissued validator. + response: gitHubNotModifiedResponse({ etag: 'W/"new"', link: '; rel="next"' }), + }), + gitHubRestStep({ + method: 'GET', + path: '/repos/o/r/pulls', + // The reissued validator must be stored, otherwise the stale one is resent forever. + assert: request => assert.strictEqual(request.headers['if-none-match'], 'W/"new"'), + response: gitHubNotModifiedResponse({ etag: 'W/"new"' }), + }), + ); + const transport = disposables.add(new GitHubTransport(nodeFetch)); + const request = { method: 'GET' as const, url: `${server.apiBaseUrl}/repos/o/r/pulls` }; + + await transport.rest(accountA, 'token-a', request, signal()); + const revalidated = await transport.rest(accountA, 'token-a', request, signal()); + const again = await transport.rest(accountA, 'token-a', request, signal()); + + assert.deepStrictEqual({ + data: revalidated.data, + statusCode: revalidated.statusCode, + etag: revalidated.etag, + link: revalidated.link, + againData: again.data, + }, { + data: [{ number: 1 }], + statusCode: 304, + etag: 'W/"new"', + link: '; rel="next"', + againData: [{ number: 1 }], + }); + server.assertSatisfied(); + }); + }); + + test('rejects a 304 that answers no cached representation', async () => { + await withServer(async server => { + server.enqueue(gitHubRestStep({ + method: 'GET', + path: '/repos/o/r/pulls', + response: gitHubNotModifiedResponse({ etag: '"phantom"' }), + })); + const transport = disposables.add(new GitHubTransport(nodeFetch)); + + await assert.rejects( + () => transport.rest(accountA, 'token-a', { method: 'GET', url: `${server.apiBaseUrl}/repos/o/r/pulls` }, signal()), + /GitHub returned 304 without a cached representation/, + ); + server.assertSatisfied(); + }); + }); + test('removes an old validator when a 200 response has no ETag', async () => { await withServer(async server => { server.enqueue( diff --git a/src/vs/workbench/browser/media/floatingPanels.css b/src/vs/workbench/browser/media/floatingPanels.css index 9f147361912024..13e17008289477 100644 --- a/src/vs/workbench/browser/media/floatingPanels.css +++ b/src/vs/workbench/browser/media/floatingPanels.css @@ -204,8 +204,8 @@ .monaco-workbench.floating-panels .part.statusbar { padding-left: var(--vscode-spacing-size60); padding-right: var(--vscode-spacing-size60); - padding-top: var(--vscode-spacing-size40); - padding-bottom: var(--vscode-spacing-size40); + padding-top: var(--vscode-spacing-size20); + padding-bottom: var(--vscode-spacing-size20); } .monaco-workbench.floating-panels .part.statusbar:not(:focus).status-border-top::after { diff --git a/src/vs/workbench/browser/parts/statusbar/statusbarPart.ts b/src/vs/workbench/browser/parts/statusbar/statusbarPart.ts index f58803d6d6fa7c..d0bff27904b846 100644 --- a/src/vs/workbench/browser/parts/statusbar/statusbarPart.ts +++ b/src/vs/workbench/browser/parts/statusbar/statusbarPart.ts @@ -126,7 +126,7 @@ class StatusbarPart extends Part implements IStatusbarEntryContainer { * experiment so its items remain centered. The part grows by this amount and * the matching padding is applied in `floatingPanels.css`. */ - static readonly FLOATING_BOTTOM_PADDING = 10; + static readonly FLOATING_BOTTOM_PADDING = 6; //#region IView diff --git a/src/vs/workbench/contrib/chat/browser/actions/chatClear.ts b/src/vs/workbench/contrib/chat/browser/actions/chatClear.ts index 5507e905d75306..55ecd20efd79c9 100644 --- a/src/vs/workbench/contrib/chat/browser/actions/chatClear.ts +++ b/src/vs/workbench/contrib/chat/browser/actions/chatClear.ts @@ -3,22 +3,13 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { URI } from '../../../../../base/common/uri.js'; -import { generateUuid } from '../../../../../base/common/uuid.js'; import { ServicesAccessor } from '../../../../../platform/instantiation/common/instantiation.js'; import { IEditorService } from '../../../../services/editor/common/editorService.js'; -import { localChatSessionType } from '../../common/chatSessionsService.js'; import { resolveDefaultNewChatSessionType } from '../../common/constants.js'; -import { getChatSessionType, LocalChatSessionUri } from '../../common/model/chatUri.js'; +import { getChatSessionType, getNewChatSessionResource } from '../../common/model/chatUri.js'; import { IChatEditorOptions } from '../widgetHosts/editor/chatEditor.js'; import { ChatEditorInput } from '../widgetHosts/editor/chatEditorInput.js'; -function getNewChatSessionResource(sessionType: string): URI { - return sessionType === localChatSessionType - ? LocalChatSessionUri.getNewSessionUri() - : URI.from({ scheme: sessionType, path: `/untitled-${generateUuid()}` }); -} - export async function clearChatEditor(accessor: ServicesAccessor, chatEditorInput?: ChatEditorInput, targetSessionType?: string): Promise { const editorService = accessor.get(IEditorService); diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostFolderPickerActionItem.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostFolderPickerActionItem.ts index a6a776a97debdc..b42041b78d7646 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostFolderPickerActionItem.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostFolderPickerActionItem.ts @@ -128,13 +128,9 @@ export class AgentHostFolderPickerActionItem extends ChatInputPickerActionViewIt const folders = this._workspaceContextService.getWorkspace().folders; const sessionResource = this._sessionResource(); const stored = sessionResource ? this._newSessionFolderService.getFolder(sessionResource) : undefined; + // Stale workspace selections are cleared by the folder service; standalone selections remain valid. if (stored) { - if (folders.some(folder => folder.uri.toString() === stored.toString())) { - return stored; - } - // The stored folder is no longer part of the workspace (folders - // changed); drop the stale selection and fall back below. - this._newSessionFolderService.clear(sessionResource!); + return stored; } // A started session's working directory is fixed at creation time and may // differ from the current workspace's first folder (e.g. a single-folder @@ -151,7 +147,6 @@ export class AgentHostFolderPickerActionItem extends ChatInputPickerActionViewIt } protected override renderLabel(element: HTMLElement): IDisposable | null { - this.setAriaLabelAttributes(element); const selected = this._selectedFolder(); const folder = selected && this._workspaceContextService.getWorkspace().folders.find(f => f.uri.toString() === selected.toString()); const label = folder ? folder.name : (selected ? basename(selected) : localize('agentHost.selectFolder', "Folder")); @@ -160,6 +155,10 @@ export class AgentHostFolderPickerActionItem extends ChatInputPickerActionViewIt ...renderLabelWithIcons(`$(folder)`), dom.$('span.chat-input-picker-label', undefined, label), ); + // Set the aria label after the visible text is in place: the base class + // derives it from `element.textContent`, so labeling first would lag one + // selection behind. + this.setAriaLabelAttributes(element); return null; } diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostNewSessionFolderService.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostNewSessionFolderService.ts index 0536e703d08545..f12461af4340d9 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostNewSessionFolderService.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostNewSessionFolderService.ts @@ -10,6 +10,7 @@ import { extUriBiasedIgnorePathCase, isEqual, type IExtUri } from '../../../../. import { URI } from '../../../../../../base/common/uri.js'; import { createDecorator } from '../../../../../../platform/instantiation/common/instantiation.js'; import { InstantiationType, registerSingleton } from '../../../../../../platform/instantiation/common/extensions.js'; +import { IUriIdentityService } from '../../../../../../platform/uriIdentity/common/uriIdentity.js'; import { IWorkspaceContextService } from '../../../../../../platform/workspace/common/workspace.js'; import { RootState, type ISessionFolderPickerDecision } from '../../../../../../platform/agentHost/common/state/sessionState.js'; import { IChatService } from '../../../common/chatService/chatService.js'; @@ -211,6 +212,19 @@ export interface IAgentHostNewSessionFolderService { * folder the user last picked instead of resetting to the first folder. */ getDefaultFolder(): URI | undefined; + + /** + * The folder a *new* (not-yet-started) session should use, resolved with the + * same precedence a freshly created chat would apply: a still-valid explicit + * per-session choice, else the still-valid sticky {@link getDefaultFolder}, + * else the first current workspace folder, else `undefined` (no folders). + * + * Used to reselect a draft's primary when the folder it pointed at is removed + * from the workspace. Every candidate is validated against the *current* + * workspace folders, so the removed folder is never returned regardless of the + * order in which workspace-change listeners run. + */ + resolveNewSessionPrimary(sessionResource: URI): URI | undefined; } export class AgentHostNewSessionFolderService extends Disposable implements IAgentHostNewSessionFolderService { @@ -231,6 +245,7 @@ export class AgentHostNewSessionFolderService extends Disposable implements IAge constructor( @IChatService chatService: IChatService, @IWorkspaceContextService private readonly _workspaceContextService: IWorkspaceContextService, + @IUriIdentityService private readonly _uriIdentityService: IUriIdentityService, ) { super(); @@ -244,6 +259,26 @@ export class AgentHostNewSessionFolderService extends Disposable implements IAge this.clear(sessionResource); } })); + + // Clear selections for folders actually removed from the workspace while retaining the sticky default. + this._register(this._workspaceContextService.onDidChangeWorkspaceFolders(e => { + if (e.removed.length === 0) { + return; + } + const extUri = this._uriIdentityService.extUri; + const currentFolders = this._workspaceContextService.getWorkspace().folders; + const staleSessions: URI[] = []; + for (const [sessionResource, folder] of this._folders) { + const wasRemoved = e.removed.some(removed => extUri.isEqual(removed.uri, folder)); + const stillPresent = currentFolders.some(current => extUri.isEqual(current.uri, folder)); + if (wasRemoved && !stillPresent) { + staleSessions.push(sessionResource); + } + } + for (const sessionResource of staleSessions) { + this.clear(sessionResource); + } + })); } getFolder(sessionResource: URI): URI | undefined { @@ -268,11 +303,25 @@ export class AgentHostNewSessionFolderService extends Disposable implements IAge getDefaultFolder(): URI | undefined { const stored = this._defaultFolder; - if (stored && this._workspaceContextService.getWorkspace().folders.some(folder => extUriBiasedIgnorePathCase.isEqual(folder.uri, stored))) { + if (stored && this._workspaceContextService.getWorkspace().folders.some(folder => this._uriIdentityService.extUri.isEqual(folder.uri, stored))) { return stored; } return undefined; } + + resolveNewSessionPrimary(sessionResource: URI): URI | undefined { + const folders = this._workspaceContextService.getWorkspace().folders; + // An explicit choice is honored only while it is still a workspace folder; + // the chip records only workspace folders, so a removed one is skipped here + // even before the workspace-change listener clears it (order-independent). + // Uses the same provider-aware comparator as removal detection so both + // checks agree on case-sensitive remotes. + const explicit = this._folders.get(sessionResource); + if (explicit && folders.some(folder => this._uriIdentityService.extUri.isEqual(folder.uri, explicit))) { + return explicit; + } + return this.getDefaultFolder() ?? folders[0]?.uri; + } } registerSingleton(IAgentHostNewSessionFolderService, AgentHostNewSessionFolderService, InstantiationType.Delayed); diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionListController.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionListController.ts index e17a4fa808c70a..7bbe829b8f85cb 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionListController.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostSessionListController.ts @@ -115,7 +115,7 @@ export class AgentHostSessionListController extends Disposable implements IChatS // untitled chat-input resource to the freshly-minted real resource so // the provisional `getOrCreate` for the real resource seeds it. this._importConversationStore.rename(request.untitledResource, item.resource); - await this._provisional.tryRebind(request.untitledResource, item.resource, this._provider, workingDirectory); + await this._provisional.tryRebind(request.untitledResource, item.resource, this._provider); } return item; diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostUntitledProvisionalSessionService.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostUntitledProvisionalSessionService.ts index b2be299699384c..457954ce5e2bc3 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostUntitledProvisionalSessionService.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostUntitledProvisionalSessionService.ts @@ -68,10 +68,12 @@ import { IConfigurationService } from '../../../../../../platform/configuration/ import { InstantiationType, registerSingleton } from '../../../../../../platform/instantiation/common/extensions.js'; import { createDecorator } from '../../../../../../platform/instantiation/common/instantiation.js'; import { ILogService } from '../../../../../../platform/log/common/log.js'; -import { IWorkspaceContextService, WorkbenchState } from '../../../../../../platform/workspace/common/workspace.js'; +import { IUriIdentityService } from '../../../../../../platform/uriIdentity/common/uriIdentity.js'; +import { IWorkspaceContextService, IWorkspaceFoldersChangeEvent, WorkbenchState } from '../../../../../../platform/workspace/common/workspace.js'; import { IWorkspaceTrustManagementService } from '../../../../../../platform/workspace/common/workspaceTrust.js'; import { IWorkbenchEnvironmentService } from '../../../../../services/environment/common/environmentService.js'; import { ChatConfiguration, getChatPermissionLevelFromDefaultConfiguration, type IChatDefaultConfiguration } from '../../../common/constants.js'; +import { isUntitledChatSession } from '../../../common/model/chatUri.js'; import { IChatService } from '../../../common/chatService/chatService.js'; import { IAgentHostNewSessionFolderService, computeDesiredWorkingDirectories, computeWorkingDirectories, hasImmutablePrimaryWorkingDirectory, supportsMultipleWorkingDirectories } from './agentHostNewSessionFolderService.js'; import { IAgentCustomizationScope, IAgentHostActiveClientService } from './agentHostActiveClientService.js'; @@ -156,7 +158,6 @@ export interface IAgentHostUntitledProvisionalSessionService { oldSessionResource: URI, newSessionResource: URI, provider: string, - workingDirectory: URI | undefined, ): Promise; /** @@ -281,6 +282,7 @@ export class AgentHostUntitledProvisionalSessionService extends Disposable imple @IWorkspaceTrustManagementService private readonly _workspaceTrustManagementService: IWorkspaceTrustManagementService, @IAgentHostImportConversationStore private readonly _importConversationStore: IAgentHostImportConversationStore, @IAgentHostActiveClientService private readonly _activeClientService: IAgentHostActiveClientService, + @IUriIdentityService private readonly _uriIdentityService: IUriIdentityService, ) { super(); @@ -307,6 +309,14 @@ export class AgentHostUntitledProvisionalSessionService extends Disposable imple // a provisional backend session (built up by config chips), recreate that // provisional at the new cwd so chip schemas resolve against it. The // service owns this reaction so concurrent chip instances don't race. + // + // This path is intentionally not gated to untitled drafts: it stays safe + // for started sessions because on workspace-folder removal the folder + // service *clears* (never reselects) the removed selection, so `getFolder` + // returns `undefined` and this becomes a no-op — a started session keeps + // its immutable primary. If the folder service is ever changed to reselect + // a real resource's selection on removal, add an isUntitledChatSession + // guard here too. this._register(this._newSessionFolderService.onDidChangeFolder(sessionResource => { const folder = this._newSessionFolderService.getFolder(sessionResource); if (folder && this._entries.has(sessionResource)) { @@ -317,11 +327,16 @@ export class AgentHostUntitledProvisionalSessionService extends Disposable imple // differs from what the existing provisional was created with, dispose that // backend session and create a replacement provisional session with the new // set of directories. - this._register(this._workspaceContextService.onDidChangeWorkspaceFolders(() => { + this._register(this._workspaceContextService.onDidChangeWorkspaceFolders(e => { for (const [sessionResource, entry] of this._entries) { if (entry.disposed) { continue; } + // Untitled drafts reselect removed primaries; rebound sessions retain their immutable primary. + if (isUntitledChatSession(sessionResource) && this._primaryWasRemoved(entry, e)) { + void this._changeWorkingDirectory(sessionResource, this._newSessionFolderService.resolveNewSessionPrimary(sessionResource)); + continue; + } if (!entry.usesWorkspaceRootSet && (this._computeWorkingDirectories(entry.workingDirectory, entry.provider)?.length ?? 0) > 1) { entry.usesWorkspaceRootSet = true; } @@ -509,6 +524,25 @@ export class AgentHostUntitledProvisionalSessionService extends Disposable imple return first === undefined || second === undefined ? first === second : isEqual(first, second); } + /** + * Whether a draft's primary working directory was just removed from the + * workspace. Only folders present in `e.removed` and no longer among the + * current workspace folders qualify, so a standalone folder outside the + * workspace (never a member) is preserved, and a folder removed and re-added + * in the same event is not treated as gone. + */ + private _primaryWasRemoved(entry: IEntry, e: IWorkspaceFoldersChangeEvent): boolean { + const primary = entry.workingDirectory; + if (!primary || e.removed.length === 0) { + return false; + } + // Provider-aware comparator so a case-distinct sibling on a case-sensitive + // remote isn't mistaken for the removed primary (matches the folder service). + const extUri = this._uriIdentityService.extUri; + const stillPresent = this._workspaceContextService.getWorkspace().folders.some(folder => extUri.isEqual(folder.uri, primary)); + return !stillPresent && e.removed.some(removed => extUri.isEqual(removed.uri, primary)); + } + /** Provider-agnostic: only an agent advertising `immutablePrimary` pins index 0. */ private _sameWorkingDirectories(provider: string, first: readonly URI[] | undefined, second: readonly URI[] | undefined): boolean { return areSessionWorkingDirectoriesEqual(first, second, hasImmutablePrimaryWorkingDirectory(this._agentHostService.rootState.value, provider)); @@ -627,7 +661,6 @@ export class AgentHostUntitledProvisionalSessionService extends Disposable imple oldSessionResource: URI, newSessionResource: URI, provider: string, - workingDirectory: URI | undefined, ): Promise { // Graduation must run after any queued folder or config reconciliation. return this._queue(oldSessionResource, async () => { @@ -649,7 +682,12 @@ export class AgentHostUntitledProvisionalSessionService extends Disposable imple // The workbench cache is authoritative; backend state can lag synchronous chip edits. const config = { ...oldEntry.config }; const configVersion = oldEntry.configVersion; - const targetWorkingDirectory = oldEntry.workingDirectory ?? workingDirectory; + // The draft's own primary is authoritative: it mirrors what + // `_computeEntryWorkingDirectories(oldEntry)` sends to the backend, so + // the scalar and the array can't diverge. A cleared primary + // (`undefined`, last folder removed) therefore lets the host choose + // rather than resurrecting a folder that no longer exists. + const targetWorkingDirectory = oldEntry.workingDirectory; if (!oldEntry.usesWorkspaceRootSet && (this._computeWorkingDirectories(targetWorkingDirectory, provider)?.length ?? 0) > 1) { oldEntry.usesWorkspaceRootSet = true; } @@ -684,7 +722,7 @@ export class AgentHostUntitledProvisionalSessionService extends Disposable imple return undefined; } if (oldEntry.configVersion !== configVersion - || !this._sameUri(oldEntry.workingDirectory ?? workingDirectory, targetWorkingDirectory) + || !this._sameUri(oldEntry.workingDirectory, targetWorkingDirectory) || !this._sameWorkingDirectories(oldEntry.provider, this._computeEntryWorkingDirectories(oldEntry), targetWorkingDirectories)) { const disposed = await this._disposeBackend(created, 'obsolete rebound candidate'); if (!disposed) { @@ -734,8 +772,13 @@ export class AgentHostUntitledProvisionalSessionService extends Disposable imple * session's cwd is immutable, so the only way to honor a folder change is to * dispose and recreate. The replacement uses a fresh backend URI so existing * subscribers acquire an authoritative snapshot for the new incarnation. + * + * `newWorkingDirectory` may be `undefined` when the workspace has no folders + * left (e.g. the draft's only/primary folder was removed); the provisional is + * then recreated with no working directory, letting the host choose — the same + * as a draft first created in an empty workspace. */ - private _changeWorkingDirectory(sessionResource: URI, newWorkingDirectory: URI): Promise { + private _changeWorkingDirectory(sessionResource: URI, newWorkingDirectory: URI | undefined): Promise { const entry = this._entries.get(sessionResource); if (!entry || entry.disposed || this._sameUri(entry.workingDirectory, newWorkingDirectory)) { return Promise.resolve(); diff --git a/src/vs/workbench/contrib/chat/browser/widgetHosts/editor/chatEditorInput.ts b/src/vs/workbench/contrib/chat/browser/widgetHosts/editor/chatEditorInput.ts index ff2aa04047684a..b285bf15a04bdd 100644 --- a/src/vs/workbench/contrib/chat/browser/widgetHosts/editor/chatEditorInput.ts +++ b/src/vs/workbench/contrib/chat/browser/widgetHosts/editor/chatEditorInput.ts @@ -24,11 +24,11 @@ import { IAgentHostEnablementService } from '../../../../../../platform/agentHos import { EditorInputCapabilities, IEditorIdentifier, IEditorSerializer, IUntypedEditorInput, Verbosity } from '../../../../../common/editor.js'; import { EditorInput, IEditorCloseHandler } from '../../../../../common/editor/editorInput.js'; import { IChatModelReference, IChatService } from '../../../common/chatService/chatService.js'; -import { IChatSessionsService, localChatSessionType } from '../../../common/chatSessionsService.js'; -import { ChatAgentLocation, ChatEditorTitleMaxLength, getDefaultNewChatSessionResource, getDefaultNewChatSessionType } from '../../../common/constants.js'; +import { IChatSessionsService, isAgentHostTarget, localChatSessionType } from '../../../common/chatSessionsService.js'; +import { ChatAgentLocation, ChatEditorTitleMaxLength, getDefaultNewChatSessionResource, getDefaultNewChatSessionType, isNewChatSessionTypeUsable } from '../../../common/constants.js'; import { IChatEditingSession, ModifiedFileEntryState } from '../../../common/editing/chatEditingService.js'; import { IChatModel } from '../../../common/model/chatModel.js'; -import { LocalChatSessionUri, getChatSessionType, isUntitledChatSession } from '../../../common/model/chatUri.js'; +import { LocalChatSessionUri, getChatSessionType, getNewChatSessionResource, isUntitledChatSession } from '../../../common/model/chatUri.js'; import { IClearEditingSessionConfirmationOptions } from '../../actions/chatActions.js'; import type { IChatEditorOptions } from './chatEditor.js'; @@ -123,7 +123,22 @@ export class ChatEditorInput extends EditorInput implements IEditorCloseHandler } override copy(): EditorInput { - return this.instantiationService.createInstance(ChatEditorInput, ChatEditorInput.getNewEditorUri(), {}); + return this.instantiationService.createInstance(ChatEditorInput, this.getNewResourceForCopy(), {}); + } + + /** + * A split or duplicate opens a new, empty chat of the same session type as the + * source, falling back to a generic editor URI when that type is unknown or + * cannot start a new session. + */ + private getNewResourceForCopy(): URI { + const sourceType = this._sessionResource ? getChatSessionType(this._sessionResource) : undefined; + if (sourceType !== undefined + && (sourceType === localChatSessionType || isAgentHostTarget(sourceType)) + && isNewChatSessionTypeUsable(sourceType, this.configurationService, this.chatSessionsService, this.workspaceContextService.getWorkspace(), this.agentHostEnablementService.enabled.get())) { + return getNewChatSessionResource(sourceType); + } + return ChatEditorInput.getNewEditorUri(); } override matches(otherInput: EditorInput | IUntypedEditorInput): boolean { diff --git a/src/vs/workbench/contrib/chat/common/constants.ts b/src/vs/workbench/contrib/chat/common/constants.ts index 323a691a49c6f2..e23300dd17b4ab 100644 --- a/src/vs/workbench/contrib/chat/common/constants.ts +++ b/src/vs/workbench/contrib/chat/common/constants.ts @@ -14,8 +14,7 @@ import { ContextKeyExpr, RawContextKey } from '../../../../platform/contextkey/c import { ChatEntitlementContextKeys } from '../../../services/chat/common/chatEntitlementService.js'; import { IsAuxiliaryWindowContext, IsSessionsWindowContext } from '../../../common/contextkeys.js'; import { URI } from '../../../../base/common/uri.js'; -import { generateUuid } from '../../../../base/common/uuid.js'; -import { LocalChatSessionUri } from './model/chatUri.js'; +import { getNewChatSessionResource } from './model/chatUri.js'; import { clearUserSelectedSessionType, getRememberedSessionType, storeUserSelectedSessionType } from './chatSessionTypePreference.js'; import { IAgentHostEnablementService } from '../../../../platform/agentHost/common/agentHostEnablementService.js'; @@ -335,9 +334,7 @@ export function getComputedDefaultSessionResource( agentHostEnabled: boolean ): URI { const defaultType = getComputedDefaultSessionType(configurationService, chatSessionsService, workspace, agentHostEnabled); - return defaultType === localChatSessionType - ? LocalChatSessionUri.getNewSessionUri() - : URI.from({ scheme: defaultType, path: `/untitled-${generateUuid()}` }); + return getNewChatSessionResource(defaultType); } export function isNewChatSessionTypeUsable( @@ -446,9 +443,7 @@ export function getDefaultNewChatSessionResource( options?: IDefaultNewChatSessionTypeOptions ): URI { const defaultType = getDefaultNewChatSessionType(configurationService, chatSessionsService, storageService, workspace, agentHostEnabled, options); - return defaultType === localChatSessionType - ? LocalChatSessionUri.getNewSessionUri() - : URI.from({ scheme: defaultType, path: `/untitled-${generateUuid()}` }); + return getNewChatSessionResource(defaultType); } export function recordUserSelectedSessionType( diff --git a/src/vs/workbench/contrib/chat/common/model/chatUri.ts b/src/vs/workbench/contrib/chat/common/model/chatUri.ts index 84d204d5b25eed..63157c436c0665 100644 --- a/src/vs/workbench/contrib/chat/common/model/chatUri.ts +++ b/src/vs/workbench/contrib/chat/common/model/chatUri.ts @@ -7,6 +7,7 @@ import { encodeBase64, VSBuffer, decodeBase64 } from '../../../../../base/common import { Schemas } from '../../../../../base/common/network.js'; import { extUri } from '../../../../../base/common/resources.js'; import { URI } from '../../../../../base/common/uri.js'; +import { generateUuid } from '../../../../../base/common/uuid.js'; import { localChatSessionType } from '../chatSessionsService.js'; type ChatSessionIdentifier = { @@ -107,3 +108,14 @@ export function getChatSessionType(resource: URI): string { export function isUntitledChatSession(resource: URI): boolean { return resource.path.startsWith('/untitled-'); } + +/** + * Builds a fresh untitled session resource for the given session type. Local + * sessions get a new local-session URI; other types get an untitled URI under + * their own scheme. + */ +export function getNewChatSessionResource(sessionType: string): URI { + return sessionType === localChatSessionType + ? LocalChatSessionUri.getNewSessionUri() + : URI.from({ scheme: sessionType, path: `/untitled-${generateUuid()}` }); +} diff --git a/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostChatContribution.test.ts b/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostChatContribution.test.ts index a3e6b947bc7a21..ab5c59c9f53883 100644 --- a/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostChatContribution.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostChatContribution.test.ts @@ -10,6 +10,7 @@ import { Codicon } from '../../../../../../base/common/codicons.js'; import { Emitter, Event } from '../../../../../../base/common/event.js'; import { DisposableStore, IDisposable, IReference, toDisposable } from '../../../../../../base/common/lifecycle.js'; import { extUriBiasedIgnorePathCase } from '../../../../../../base/common/resources.js'; +import { IUriIdentityService } from '../../../../../../platform/uriIdentity/common/uriIdentity.js'; import { hasKey } from '../../../../../../base/common/types.js'; import { URI } from '../../../../../../base/common/uri.js'; import { autorun, constObservable, derived, ISettableObservable, observableValue, type IObservable } from '../../../../../../base/common/observable.js'; @@ -900,7 +901,7 @@ function createTestServices(disposables: DisposableStore, workingDirectoryResolv take: () => undefined, rename: () => { }, } as Partial as IAgentHostImportConversationStore); - const newSessionFolderService = disposables.add(new AgentHostNewSessionFolderService(chatService as Partial as IChatService, instantiationService.get(IWorkspaceContextService))); + const newSessionFolderService = disposables.add(new AgentHostNewSessionFolderService(chatService as Partial as IChatService, instantiationService.get(IWorkspaceContextService), { extUri: extUriBiasedIgnorePathCase } as Partial as IUriIdentityService)); instantiationService.stub(IAgentHostNewSessionFolderService, newSessionFolderService); const customizationsByScope = new Map; readonly customAgents: IObservable; readonly tools: IObservable; readonly isResolved: IObservable; readonly whenResolved: Promise }>(); const scopeKey = (sessionType: string, roots: readonly URI[]) => `${sessionType}\n${roots.map(root => root.toString()).sort().join('\n')}`; @@ -3481,7 +3482,7 @@ suite('AgentHostChatContribution', () => { })); test('newChatSessionItem rebinds untitled provisional to real resource so chip-selected config survives first send', () => runWithFakedTimers({ useFakeTimers: true }, async () => { - const { instantiationService, agentHostService } = createTestServices(disposables); + const { instantiationService, agentHostService, newSessionFolderService } = createTestServices(disposables); const workspaceFolder = URI.from({ scheme: 'file', path: '/workspace/root' }); instantiationService.stub(IWorkspaceContextService, { @@ -3491,14 +3492,14 @@ suite('AgentHostChatContribution', () => { onDidChangeWorkspaceFolders: Event.None, }); - const rebindCalls: { oldResource: URI; newResource: URI; provider: string; workingDirectory: URI | undefined }[] = []; + const rebindCalls: { oldResource: URI; newResource: URI; provider: string }[] = []; instantiationService.stub(IAgentHostUntitledProvisionalSessionService, { onDidChange: Event.None, get: () => undefined, waitForPending: async () => undefined, getOrCreate: async () => undefined, - tryRebind: async (oldResource: URI, newResource: URI, provider: string, workingDirectory: URI | undefined) => { - rebindCalls.push({ oldResource, newResource, provider, workingDirectory }); + tryRebind: async (oldResource: URI, newResource: URI, provider: string) => { + rebindCalls.push({ oldResource, newResource, provider }); return newResource; }, disposeSession: async () => { }, @@ -3510,12 +3511,16 @@ suite('AgentHostChatContribution', () => { const item = await listController.newChatSessionItem({ prompt: 'Hello', untitledResource }, CancellationToken.None); assert.ok(item); - assert.deepStrictEqual(rebindCalls, [{ - oldResource: untitledResource, - newResource: item.resource, - provider: 'copilot', - workingDirectory: workspaceFolder, - }]); + // The rebind graduates the untitled provisional to the real resource, and + // the chosen working directory is carried onto the real resource (which the + // handler resolves via getFolder) rather than through a tryRebind argument. + assert.deepStrictEqual({ + rebindCalls, + carriedFolder: newSessionFolderService.getFolder(item.resource)?.toString(), + }, { + rebindCalls: [{ oldResource: untitledResource, newResource: item.resource, provider: 'copilot' }], + carriedFolder: workspaceFolder.toString(), + }); })); test('newChatSessionItem skips rebind when no untitled provisional resource is provided', () => runWithFakedTimers({ useFakeTimers: true }, async () => { @@ -3555,14 +3560,14 @@ suite('AgentHostChatContribution', () => { onDidChangeWorkspaceFolders: Event.None, }); - const rebindCalls: { workingDirectory: URI | undefined }[] = []; + const rebindCalls: { oldResource: URI; newResource: URI }[] = []; instantiationService.stub(IAgentHostUntitledProvisionalSessionService, { onDidChange: Event.None, get: () => undefined, waitForPending: async () => undefined, getOrCreate: async () => undefined, - tryRebind: async (_old: URI, newResource: URI, _provider: string, workingDirectory: URI | undefined) => { - rebindCalls.push({ workingDirectory }); + tryRebind: async (oldResource: URI, newResource: URI) => { + rebindCalls.push({ oldResource, newResource }); return newResource; }, disposeSession: async () => { }, @@ -3576,7 +3581,16 @@ suite('AgentHostChatContribution', () => { const item = await listController.newChatSessionItem({ prompt: 'Hello', untitledResource }, CancellationToken.None); assert.ok(item); - assert.deepStrictEqual(rebindCalls, [{ workingDirectory: folderB }]); + // The store-selected folder is carried onto the real resource (the handler + // resolves the working directory from getFolder), and the rebind graduates + // the untitled provisional to that real resource. + assert.deepStrictEqual({ + rebindCalls, + carriedFolder: newSessionFolderService.getFolder(item.resource)?.toString(), + }, { + rebindCalls: [{ oldResource: untitledResource, newResource: item.resource }], + carriedFolder: folderB.toString(), + }); })); test('folder picker is visible only in multi-root agent-host editor windows when the provider pins an immutable primary directory', () => { diff --git a/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostNewSessionFolderService.test.ts b/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostNewSessionFolderService.test.ts index 36d23cc527a715..9c3fb49641b323 100644 --- a/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostNewSessionFolderService.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostNewSessionFolderService.test.ts @@ -5,11 +5,13 @@ import assert from 'assert'; import { DisposableStore } from '../../../../../../base/common/lifecycle.js'; -import { Emitter } from '../../../../../../base/common/event.js'; +import { Emitter, Event } from '../../../../../../base/common/event.js'; +import { ExtUri, extUriBiasedIgnorePathCase } from '../../../../../../base/common/resources.js'; import { URI } from '../../../../../../base/common/uri.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js'; import { mock } from '../../../../../../base/test/common/mock.js'; -import { IWorkspaceContextService, IWorkspaceFolder, IWorkspace } from '../../../../../../platform/workspace/common/workspace.js'; +import { IUriIdentityService } from '../../../../../../platform/uriIdentity/common/uriIdentity.js'; +import { IWorkspaceContextService, IWorkspaceFolder, IWorkspace, IWorkspaceFoldersChangeEvent } from '../../../../../../platform/workspace/common/workspace.js'; import type { AgentInfo, RootState } from '../../../../../../platform/agentHost/common/state/sessionState.js'; import { IChatService } from '../../../common/chatService/chatService.js'; import { AgentHostNewSessionFolderService, computeDesiredWorkingDirectories, computeWorkingDirectories } from '../../../browser/agentSessions/agentHost/agentHostNewSessionFolderService.js'; @@ -20,20 +22,23 @@ suite('AgentHostNewSessionFolderService', () => { let service: AgentHostNewSessionFolderService; let cleanup: DisposableStore; let onDidDisposeSession: Emitter<{ readonly sessionResources: readonly URI[]; readonly reason: 'cleared' }>; + let onDidChangeWorkspaceFolders: Emitter; let workspaceFolders: URI[]; setup(() => { onDidDisposeSession = ds.add(new Emitter<{ readonly sessionResources: readonly URI[]; readonly reason: 'cleared' }>()); + onDidChangeWorkspaceFolders = ds.add(new Emitter()); const chatService = new class extends mock() { override readonly onDidDisposeSession = onDidDisposeSession.event; }; workspaceFolders = [folderA, folderB]; const workspaceContextService = new class extends mock() { + override readonly onDidChangeWorkspaceFolders = onDidChangeWorkspaceFolders.event; override getWorkspace(): IWorkspace { return { folders: workspaceFolders.map(uri => ({ uri } as IWorkspaceFolder)) } as IWorkspace; } }; - service = ds.add(new AgentHostNewSessionFolderService(chatService, workspaceContextService)); + service = ds.add(new AgentHostNewSessionFolderService(chatService, workspaceContextService, { extUri: extUriBiasedIgnorePathCase } as Partial as IUriIdentityService)); cleanup = ds.add(new DisposableStore()); }); @@ -43,6 +48,8 @@ suite('AgentHostNewSessionFolderService', () => { const folderB = URI.file('/repoB'); const folderC = URI.file('/repoC'); + const wsFolder = (uri: URI): IWorkspaceFolder => ({ uri, index: 0, name: uri.path, toResource: relativePath => URI.joinPath(uri, relativePath) }); + test('set/get/clear round-trips and fires onDidChange on real changes only', () => { const changes: string[] = []; cleanup.add(service.onDidChangeFolder(uri => changes.push(uri.toString()))); @@ -118,6 +125,99 @@ suite('AgentHostNewSessionFolderService', () => { afterFolderAdded: folderC.toString(), }); }); + + test('clears an explicit selection when its folder is removed from the workspace', () => { + const changes: string[] = []; + cleanup.add(service.onDidChangeFolder(uri => changes.push(uri.toString()))); + + service.setFolder(sessionA, folderB); + workspaceFolders = [folderA]; + onDidChangeWorkspaceFolders.fire({ added: [], removed: [wsFolder(folderB)], changed: [] }); + + assert.deepStrictEqual({ + afterRemoval: service.getFolder(sessionA), + changes, + }, { + afterRemoval: undefined, + changes: [sessionA.toString(), sessionA.toString()], + }); + }); + + test('leaves the sticky default intact when the selected folder is removed so it resurfaces on re-add', () => { + service.setFolder(sessionA, folderB); + workspaceFolders = [folderA]; + onDidChangeWorkspaceFolders.fire({ added: [], removed: [wsFolder(folderB)], changed: [] }); + const whileRemoved = service.getDefaultFolder(); + + workspaceFolders = [folderA, folderB]; + const afterReadd = service.getDefaultFolder()?.toString(); + + assert.deepStrictEqual({ whileRemoved, afterReadd }, { + whileRemoved: undefined, + afterReadd: folderB.toString(), + }); + }); + + test('preserves a selection outside the workspace when a different folder is removed', () => { + const external = URI.file('/external/repo'); + service.setFolder(sessionA, external); + workspaceFolders = [folderA]; + onDidChangeWorkspaceFolders.fire({ added: [], removed: [wsFolder(folderB)], changed: [] }); + + assert.strictEqual(service.getFolder(sessionA)?.toString(), external.toString()); + }); + + test('resolveNewSessionPrimary applies new-session precedence', () => { + workspaceFolders = [folderA, folderB, folderC]; + // sessionA has a valid explicit choice; setting it also makes folderB the sticky default. + service.setFolder(sessionA, folderB); + + const validExplicit = service.resolveNewSessionPrimary(sessionA)?.toString(); + // sessionB has no explicit choice, so it falls back to the sticky default. + const stickyDefault = service.resolveNewSessionPrimary(sessionB)?.toString(); + + // Remove folderB: the explicit choice and the sticky default both become + // invalid, so it falls back to the first remaining workspace folder. + workspaceFolders = [folderA, folderC]; + onDidChangeWorkspaceFolders.fire({ added: [], removed: [wsFolder(folderB)], changed: [] }); + const firstFolderFallback = service.resolveNewSessionPrimary(sessionA)?.toString(); + + // With no folders at all there is nothing to select. + workspaceFolders = []; + const noFolders = service.resolveNewSessionPrimary(sessionA); + + assert.deepStrictEqual({ validExplicit, stickyDefault, firstFolderFallback, noFolders }, { + validExplicit: folderB.toString(), + stickyDefault: folderB.toString(), + firstFolderFallback: folderA.toString(), + noFolders: undefined, + }); + }); + + test('clears a removed selection on a case-sensitive remote even when a case-variant folder remains', () => { + const repoUpper = URI.parse('vscode-remote://ssh-remote+host/work/Repo'); + const repoLower = URI.parse('vscode-remote://ssh-remote+host/work/repo'); + let remoteFolders = [repoUpper, repoLower]; + const changeEmitter = ds.add(new Emitter()); + const chatSvc = new class extends mock() { + override readonly onDidDisposeSession = Event.None; + }; + const wsSvc = new class extends mock() { + override readonly onDidChangeWorkspaceFolders = changeEmitter.event; + override getWorkspace(): IWorkspace { + return { folders: remoteFolders.map(uri => ({ uri } as IWorkspaceFolder)) } as IWorkspace; + } + }; + // A provider-aware, case-sensitive comparator (as IUriIdentityService.extUri + // resolves for a case-sensitive remote filesystem). + const caseSensitive = ds.add(new AgentHostNewSessionFolderService(chatSvc, wsSvc, { extUri: new ExtUri(() => false) } as Partial as IUriIdentityService)); + + caseSensitive.setFolder(sessionA, repoUpper); + remoteFolders = [repoLower]; + changeEmitter.fire({ added: [], removed: [wsFolder(repoUpper)], changed: [] }); + + assert.strictEqual(caseSensitive.getFolder(sessionA), undefined, 'the case-distinct sibling must not keep the removed selection alive'); + }); }); suite('computeWorkingDirectories', () => { diff --git a/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostUntitledProvisionalSessionService.test.ts b/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostUntitledProvisionalSessionService.test.ts index 004408c0535aa4..69c9982f428364 100644 --- a/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostUntitledProvisionalSessionService.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/agentSessions/agentHostUntitledProvisionalSessionService.test.ts @@ -25,6 +25,7 @@ import { IWorkspaceContextService, IWorkspace, IWorkspaceFolder, IWorkspaceFolde import { IAgentSubscription } from '../../../../../../platform/agentHost/common/state/agentSubscription.js'; import { MessageKind, TurnState, type AgentInfo, type RootState, type Turn } from '../../../../../../platform/agentHost/common/state/sessionState.js'; import { IWorkspaceTrustManagementService } from '../../../../../../platform/workspace/common/workspaceTrust.js'; +import { IUriIdentityService } from '../../../../../../platform/uriIdentity/common/uriIdentity.js'; import { IChatService } from '../../../common/chatService/chatService.js'; import { AgentHostUntitledProvisionalSessionService, IAgentHostUntitledProvisionalSessionService } from '../../../browser/agentSessions/agentHost/agentHostUntitledProvisionalSessionService.js'; import { AgentHostNewSessionFolderService, IAgentHostNewSessionFolderService } from '../../../browser/agentSessions/agentHost/agentHostNewSessionFolderService.js'; @@ -184,6 +185,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { let isSessionsWindow: boolean; let customizations: ReturnType>; let onDidChangeWorkspaceFolders: Emitter; + let acquiredScopeRoots: string[][]; setup(async () => { agentHost = ds.add(new MockAgentHostService()); @@ -194,6 +196,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { workspaceName = undefined; workbenchState = WorkbenchState.EMPTY; isSessionsWindow = false; + acquiredScopeRoots = []; onDidChangeWorkspaceFolders = ds.add(new Emitter()); const insta = ds.add(new TestInstantiationService()); insta.stub(IAgentHostService, agentHost); @@ -217,6 +220,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { override isWorkspaceTrusted(): boolean { return workspaceTrusted; } override async getUriTrustInfo(uri: URI) { return { uri, trusted: !untrustedFolders.has(uri.toString()) }; } }); + insta.stub(IUriIdentityService, { extUri: new ExtUri(() => false) } as Partial as IUriIdentityService); folderService = ds.add(insta.createInstance(AgentHostNewSessionFolderService)); insta.stub(IAgentHostNewSessionFolderService, folderService); importStore = new AgentHostImportConversationStore(); @@ -224,15 +228,18 @@ suite('AgentHostUntitledProvisionalSessionService', () => { customizations = observableValue('customizations', []); insta.stub(IAgentHostActiveClientService, { areScopeRootsEqual: (first, second) => areCustomizationScopeRootsEqual(first, second, new ExtUri(() => false)), - acquireScope: (_sessionType: string, _roots: readonly URI[]) => ({ - customizations, - customAgents: constObservable([]), - tools: constObservable([]), - isResolved: constObservable(true), - whenResolved: () => Promise.resolve(), - activeClient: clientId => derived(reader => ({ clientId, tools: [], customizations: [...customizations.read(reader)] })), - dispose: () => { }, - }), + acquireScope: (_sessionType: string, roots: readonly URI[]) => { + acquiredScopeRoots.push(roots.map(root => root.toString())); + return { + customizations, + customAgents: constObservable([]), + tools: constObservable([]), + isResolved: constObservable(true), + whenResolved: () => Promise.resolve(), + activeClient: clientId => derived(reader => ({ clientId, tools: [], customizations: [...customizations.read(reader)] })), + dispose: () => { }, + }; + }, } as Partial as IAgentHostActiveClientService); provisional = ds.add(insta.createInstance(AgentHostUntitledProvisionalSessionService)); cleanup = ds.add(new DisposableStore()); @@ -306,7 +313,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { }); }); - test('workspace folder changes recreate a multi-root provisional with the latest secondary set', async () => { + test('reselects the primary and recreates the provisional when the primary folder is removed', async () => { const primary = URI.file('/workspace/one'); const secondary = URI.file('/workspace/two'); const added = URI.file('/workspace/three'); @@ -314,26 +321,50 @@ suite('AgentHostUntitledProvisionalSessionService', () => { workspaceConfiguration = URI.file('/workspace/demo.code-workspace'); workbenchState = WorkbenchState.WORKSPACE; agentHost.rootStateAgents = [agentInfo('copilot', true)]; - const ui = untitledChatUri('multi-root-folder-changes'); + const ui = untitledChatUri('multi-root-primary-removed'); await provisional.getOrCreate(ui, 'copilot', primary); + // Removing the primary of a not-yet-started draft reselects the first + // remaining folder (as a freshly created chat would) and recreates there. workspaceFolders = [secondary, added]; onDidChangeWorkspaceFolders.fire({ added: [workspaceFolder(added, 1)], removed: [workspaceFolder(primary, 0)], changed: [], }); - await provisional.waitForPending(ui); - workspaceFolders = [added, secondary]; + // Removing the freshly-selected primary reselects again. + workspaceFolders = [added]; onDidChangeWorkspaceFolders.fire({ added: [], - removed: [], - changed: [workspaceFolder(added, 0), workspaceFolder(secondary, 1)], + removed: [workspaceFolder(secondary, 0)], + changed: [], }); await provisional.waitForPending(ui); - const afterReorderCount = agentHost.createCalls.length; - workspaceFolders = [added]; + + assert.deepStrictEqual( + agentHost.createCalls.map(call => call.workingDirectories?.map(directory => directory.toString())), + [ + [primary.toString(), secondary.toString()], + [secondary.toString(), added.toString()], + [added.toString()], + ], + ); + }); + + test('removing a secondary folder keeps the primary and recreates with the remaining secondaries', async () => { + const primary = URI.file('/workspace/one'); + const secondary = URI.file('/workspace/two'); + const third = URI.file('/workspace/three'); + workspaceFolders = [primary, secondary, third]; + workspaceConfiguration = URI.file('/workspace/demo.code-workspace'); + workbenchState = WorkbenchState.WORKSPACE; + agentHost.rootStateAgents = [agentInfo('copilot', true)]; + const ui = untitledChatUri('secondary-removed'); + + await provisional.getOrCreate(ui, 'copilot', primary); + const createsBeforeRemoval = agentHost.createCalls.length; + workspaceFolders = [primary, third]; onDidChangeWorkspaceFolders.fire({ added: [], removed: [workspaceFolder(secondary, 1)], @@ -342,18 +373,238 @@ suite('AgentHostUntitledProvisionalSessionService', () => { await provisional.waitForPending(ui); assert.deepStrictEqual({ + createsBeforeRemoval, workingDirectories: agentHost.createCalls.map(call => call.workingDirectories?.map(directory => directory.toString())), - afterReorderCount, }, { + createsBeforeRemoval: 1, workingDirectories: [ - [primary.toString(), secondary.toString()], - [primary.toString(), secondary.toString(), added.toString()], - [primary.toString(), added.toString()], + [primary.toString(), secondary.toString(), third.toString()], + [primary.toString(), third.toString()], ], - afterReorderCount: 2, }); }); + test('reselects the primary for a single-working-directory provider draft when the primary is removed', async () => { + const primary = URI.file('/workspace/one'); + const secondary = URI.file('/workspace/two'); + workspaceFolders = [primary, secondary]; + workspaceConfiguration = URI.file('/workspace/demo.code-workspace'); + workbenchState = WorkbenchState.WORKSPACE; + // Provider does NOT advertise multipleWorkingDirectories, so the draft is + // not a workspace-root-set draft (usesWorkspaceRootSet === false). + agentHost.rootStateAgents = [agentInfo('copilot', false)]; + const ui = untitledChatUri('single-wd-primary-removed'); + + await provisional.getOrCreate(ui, 'copilot', primary); + workspaceFolders = [secondary]; + onDidChangeWorkspaceFolders.fire({ + added: [], + removed: [workspaceFolder(primary, 0)], + changed: [], + }); + await provisional.waitForPending(ui); + + assert.deepStrictEqual( + agentHost.createCalls.map(call => call.workingDirectories?.map(directory => directory.toString())), + [[primary.toString()], [secondary.toString()]], + ); + }); + + test('recreates without a working directory when the last workspace folder is removed', async () => { + const only = URI.file('/workspace/one'); + workspaceFolders = [only]; + workbenchState = WorkbenchState.FOLDER; + agentHost.rootStateAgents = [agentInfo('copilot', true)]; + const ui = untitledChatUri('last-folder-removed'); + + await provisional.getOrCreate(ui, 'copilot', only); + workspaceFolders = []; + onDidChangeWorkspaceFolders.fire({ + added: [], + removed: [workspaceFolder(only, 0)], + changed: [], + }); + await provisional.waitForPending(ui); + + assert.deepStrictEqual( + agentHost.createCalls.map(call => call.workingDirectories?.map(directory => directory.toString()) ?? null), + [[only.toString()], null], + ); + }); + + test('reselects only the draft whose primary was removed', async () => { + const a = URI.file('/workspace/a'); + const b = URI.file('/workspace/b'); + const c = URI.file('/workspace/c'); + workspaceFolders = [a, b, c]; + workspaceConfiguration = URI.file('/workspace/demo.code-workspace'); + workbenchState = WorkbenchState.WORKSPACE; + agentHost.rootStateAgents = [agentInfo('copilot', true)]; + const uiA = untitledChatUri('draft-a'); + const uiC = untitledChatUri('draft-c'); + + await provisional.getOrCreate(uiA, 'copilot', a); + await provisional.getOrCreate(uiC, 'copilot', c); + const createsBeforeRemoval = agentHost.createCalls.length; + + // Remove folder a: draft A must reselect a new primary; draft C keeps c. + workspaceFolders = [b, c]; + onDidChangeWorkspaceFolders.fire({ + added: [], + removed: [workspaceFolder(a, 0)], + changed: [], + }); + await provisional.waitForPending(uiA); + await provisional.waitForPending(uiC); + + const afterRemoval = agentHost.createCalls.slice(createsBeforeRemoval).map(call => call.workingDirectories?.map(directory => directory.toString()) ?? []); + const draftAPrimary = afterRemoval.find(directories => directories[0] === b.toString())?.[0]; + const draftCEntry = afterRemoval.find(directories => directories[0] === c.toString()); + + assert.deepStrictEqual({ + draftAReselectedTo: draftAPrimary, + draftCPrimary: draftCEntry?.[0], + draftCDroppedRemovedFolder: !(draftCEntry?.includes(a.toString()) ?? false), + }, { + draftAReselectedTo: b.toString(), + draftCPrimary: c.toString(), + draftCDroppedRemovedFolder: true, + }); + }); + + test('reordering workspace folders does not recreate the provisional', async () => { + const primary = URI.file('/workspace/one'); + const secondary = URI.file('/workspace/two'); + workspaceFolders = [primary, secondary]; + workspaceConfiguration = URI.file('/workspace/demo.code-workspace'); + workbenchState = WorkbenchState.WORKSPACE; + agentHost.rootStateAgents = [agentInfo('copilot', true)]; + const ui = untitledChatUri('reorder-noop'); + + await provisional.getOrCreate(ui, 'copilot', primary); + const createsBeforeReorder = agentHost.createCalls.length; + workspaceFolders = [secondary, primary]; + onDidChangeWorkspaceFolders.fire({ + added: [], + removed: [], + changed: [workspaceFolder(secondary, 0), workspaceFolder(primary, 1)], + }); + await provisional.waitForPending(ui); + + assert.strictEqual(agentHost.createCalls.length, createsBeforeReorder); + }); + + test('does not reselect or dispose a started session when its primary folder is removed', async () => { + const primary = URI.file('/workspace/one'); + const secondary = URI.file('/workspace/two'); + workspaceFolders = [primary, secondary]; + workspaceConfiguration = URI.file('/workspace/demo.code-workspace'); + workbenchState = WorkbenchState.WORKSPACE; + agentHost.rootStateAgents = [agentInfo('copilot', true)]; + const ui = untitledChatUri('started-primary-removed'); + const real = URI.from({ scheme: 'agent-host-copilot', path: '/real-started-primary-removed' }); + + await provisional.getOrCreate(ui, 'copilot', primary); + await provisional.tryRebind(ui, real, 'copilot'); + const realBackend = provisional.get(real); + assert.ok(realBackend); + const createsAfterRebind = agentHost.createCalls.length; + + // Removing the started session's primary must not touch it: its working + // directory is the agent's fixed process root once the session started. + workspaceFolders = [secondary]; + onDidChangeWorkspaceFolders.fire({ + added: [], + removed: [workspaceFolder(primary, 0)], + changed: [], + }); + await provisional.waitForPending(real); + + assert.deepStrictEqual({ + createsAfterRemoval: agentHost.createCalls.length - createsAfterRebind, + liveBackendDisposed: agentHost.disposed.some(uri => uri.toString() === realBackend.toString()), + currentBackend: provisional.get(real)?.toString(), + }, { + createsAfterRemoval: 0, + liveBackendDisposed: false, + currentBackend: realBackend.toString(), + }); + }); + + test('tryRebind reselects when the primary is removed during final creation', async () => { + const primary = URI.file('/workspace/one'); + const secondary = URI.file('/workspace/two'); + workspaceFolders = [primary, secondary]; + workspaceConfiguration = URI.file('/workspace/demo.code-workspace'); + workbenchState = WorkbenchState.WORKSPACE; + agentHost.rootStateAgents = [agentInfo('copilot', true)]; + const ui = untitledChatUri('rebind-primary-removed'); + const real = URI.from({ scheme: 'agent-host-copilot', path: '/real-rebind-primary-removed' }); + + await provisional.getOrCreate(ui, 'copilot', primary); + const gate = new DeferredPromise(); + cleanup.add({ dispose: () => gate.cancel() }); + agentHost.createGate = gate; + + const rebind = provisional.tryRebind(ui, real, 'copilot'); + await timeout(0); + // The primary is removed while the final session creation is in flight; the + // reselection updates the draft so the rebind retries at the remaining folder. + workspaceFolders = [secondary]; + onDidChangeWorkspaceFolders.fire({ + added: [], + removed: [workspaceFolder(primary, 0)], + changed: [], + }); + gate.complete(); + await rebind; + + const finalCreate = agentHost.createCalls.filter(call => call.session?.path === '/real-rebind-primary-removed').at(-1); + assert.deepStrictEqual(finalCreate?.workingDirectories?.map(directory => directory.toString()), [secondary.toString()]); + }); + + test('tryRebind does not root the started session at the removed folder when the last folder is removed during final creation', async () => { + const only = URI.file('/workspace/one'); + workspaceFolders = [only]; + workbenchState = WorkbenchState.FOLDER; + agentHost.rootStateAgents = [agentInfo('copilot', true)]; + const ui = untitledChatUri('rebind-last-folder-removed'); + const real = URI.from({ scheme: 'agent-host-copilot', path: '/real-rebind-last-folder-removed' }); + + await provisional.getOrCreate(ui, 'copilot', only); + const gate = new DeferredPromise(); + cleanup.add({ dispose: () => gate.cancel() }); + agentHost.createGate = gate; + + const rebind = provisional.tryRebind(ui, real, 'copilot'); + await timeout(0); + // The last workspace folder is removed while final creation is in flight. + // The draft's primary is cleared to `undefined`, and the rebind derives its + // working directory from the draft's own primary — so neither the backend + // nor the active-client scope may reference the removed folder. + workspaceFolders = []; + onDidChangeWorkspaceFolders.fire({ + added: [], + removed: [workspaceFolder(only, 0)], + changed: [], + }); + gate.complete(); + await rebind; + await provisional.waitForPending(real); + + const finalCreate = agentHost.createCalls.filter(call => call.session?.path === '/real-rebind-last-folder-removed').at(-1); + assert.deepStrictEqual({ + backendWorkingDirectories: finalCreate?.workingDirectories?.map(directory => directory.toString()) ?? null, + lastScopeRoots: acquiredScopeRoots.at(-1), + anyScopeKeepsRemovedFolder: acquiredScopeRoots.slice(1).some(roots => roots.includes(only.toString())), + }, { + backendWorkingDirectories: null, + lastScopeRoots: [], + anyScopeKeepsRemovedFolder: false, + }); + }); + + test('a single-folder draft adopts secondary roots when the workspace becomes multi-root', async () => { const primary = URI.file('/workspace/one'); const added = URI.file('/workspace/two'); @@ -394,7 +645,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { await provisional.getOrCreate(ui, 'copilot', primary); workspaceFolders = [secondary, added]; - await provisional.tryRebind(ui, real, 'copilot', primary); + await provisional.tryRebind(ui, real, 'copilot'); assert.deepStrictEqual( agentHost.createCalls.at(-1)?.workingDirectories?.map(directory => directory.toString()), @@ -414,7 +665,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { await provisional.getOrCreate(ui, 'copilot', primary); workspaceFolders = [primary, added]; - await provisional.tryRebind(ui, real, 'copilot', primary); + await provisional.tryRebind(ui, real, 'copilot'); assert.deepStrictEqual( agentHost.createCalls.map(call => call.workingDirectories?.map(directory => directory.toString())), @@ -671,7 +922,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { // Rebind must wait behind the config operation rather than graduating // with a partially reconciled draft. const newUi = URI.from({ scheme: 'agent-host-copilot', path: '/real-g' }); - const rebind = provisional.tryRebind(ui, newUi, 'copilot', undefined); + const rebind = provisional.tryRebind(ui, newUi, 'copilot'); assert.strictEqual(agentHost.createCalls.some(c => c.session?.path === '/real-g'), false); blocked.complete({ schema: makeSchema(false), values: { isolation: 'worktree' } }); await rebind; @@ -701,7 +952,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { cleanup.add({ dispose: () => gate.cancel() }); agentHost.createGate = gate; - const rebind = provisional.tryRebind(ui, realUi, 'copilot', undefined); + const rebind = provisional.tryRebind(ui, realUi, 'copilot'); await timeout(0); const configChange = provisional.applyConfigChange(ui, 'copilot', undefined, { isolation: 'worktree' }); gate.complete(); @@ -735,7 +986,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { cleanup.add({ dispose: () => gate.cancel() }); agentHost.createGate = gate; - const rebind = provisional.tryRebind(ui, realUi, 'copilot', undefined); + const rebind = provisional.tryRebind(ui, realUi, 'copilot'); await timeout(0); const disposal = provisional.disposeSession(ui); gate.complete(); @@ -766,7 +1017,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { importStore.set(realUi, imported); agentHost.failNextCreate = true; - const rebound = await provisional.tryRebind(ui, realUi, 'copilot', undefined); + const rebound = await provisional.tryRebind(ui, realUi, 'copilot'); assert.deepStrictEqual({ rebound, @@ -786,7 +1037,7 @@ suite('AgentHostUntitledProvisionalSessionService', () => { const gate = new DeferredPromise(); cleanup.add({ dispose: () => gate.cancel() }); agentHost.createGate = gate; - const rebind = provisional.tryRebind(ui, realUi, 'copilot', undefined); + const rebind = provisional.tryRebind(ui, realUi, 'copilot'); const pendingRead = provisional.waitForPending(ui); await timeout(0); agentHost.resolveQueue = [{ schema: makeSchema(false), values: { isolation: 'worktree' } }]; diff --git a/src/vs/workbench/contrib/chat/test/browser/widgetHosts/editor/chatEditorInput.test.ts b/src/vs/workbench/contrib/chat/test/browser/widgetHosts/editor/chatEditorInput.test.ts index 308e95ff67c703..4426cbd6985deb 100644 --- a/src/vs/workbench/contrib/chat/test/browser/widgetHosts/editor/chatEditorInput.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/widgetHosts/editor/chatEditorInput.test.ts @@ -6,7 +6,9 @@ import assert from 'assert'; import { Event } from '../../../../../../../base/common/event.js'; import { DisposableStore } from '../../../../../../../base/common/lifecycle.js'; +import { Schemas } from '../../../../../../../base/common/network.js'; import { constObservable } from '../../../../../../../base/common/observable.js'; +import { isEqual } from '../../../../../../../base/common/resources.js'; import { URI } from '../../../../../../../base/common/uri.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../../base/test/common/utils.js'; import { IConfigurationService } from '../../../../../../../platform/configuration/common/configuration.js'; @@ -26,7 +28,7 @@ import { IChatService, IChatSessionStartOptions } from '../../../../common/chatS import { IChatSessionsService, localChatSessionType, SessionType } from '../../../../common/chatSessionsService.js'; import { ChatAgentLocation } from '../../../../common/constants.js'; import { IChatModel } from '../../../../common/model/chatModel.js'; -import { getChatSessionType, LocalChatSessionUri } from '../../../../common/model/chatUri.js'; +import { getChatSessionType, isUntitledChatSession, LocalChatSessionUri } from '../../../../common/model/chatUri.js'; import { MockChatSessionsService } from '../../../common/mockChatSessionsService.js'; import { TestContextService, TestStorageService } from '../../../../../../test/common/workbenchTestServices.js'; @@ -193,4 +195,73 @@ suite('ChatEditorInput', () => { store.dispose(); } }); + + function createInputForCopy(store: DisposableStore, resource: URI, agentHostEnabled: boolean): ChatEditorInput { + const instantiationService = store.add(new TestInstantiationService()); + instantiationService.stub(IChatService, {}); + instantiationService.stub(IDialogService, {}); + instantiationService.set(IConfigurationService, new TestConfigurationService()); + instantiationService.set(IChatSessionsService, new MockChatSessionsService()); + instantiationService.set(IStorageService, store.add(new TestStorageService())); + instantiationService.set(ILogService, new NullLogService()); + instantiationService.set(IWorkspaceContextService, new TestContextService()); + instantiationService.set(IAgentHostEnablementService, { _serviceBrand: undefined, enabled: constObservable(agentHostEnabled) }); + return store.add(instantiationService.createInstance(ChatEditorInput, resource, {})); + } + + test('copy preserves an agent host session type as a new untitled session', () => { + const store = disposables.add(new DisposableStore()); + const source = URI.from({ scheme: SessionType.AgentHostCopilot, path: '/untitled-source' }); + const input = createInputForCopy(store, source, true); + + const copied = store.add(input.copy() as ChatEditorInput); + + assert.deepStrictEqual({ + copiedType: getChatSessionType(copied.resource), + copiedUntitled: isUntitledChatSession(copied.resource), + distinctFromSource: !isEqual(copied.resource, source), + sourceUnchanged: isEqual(input.resource, source), + }, { + copiedType: SessionType.AgentHostCopilot, + copiedUntitled: true, + distinctFromSource: true, + sourceUnchanged: true, + }); + }); + + test('copy preserves a local session type as a new local session', () => { + const store = disposables.add(new DisposableStore()); + const source = LocalChatSessionUri.getNewSessionUri(); + const input = createInputForCopy(store, source, true); + + const copied = store.add(input.copy() as ChatEditorInput); + + assert.deepStrictEqual({ + copiedType: getChatSessionType(copied.resource), + copiedScheme: copied.resource.scheme, + distinctFromSource: !isEqual(copied.resource, source), + }, { + copiedType: localChatSessionType, + copiedScheme: LocalChatSessionUri.scheme, + distinctFromSource: true, + }); + }); + + test('copy falls back to a generic editor URI when the source type cannot start a new session', () => { + const store = disposables.add(new DisposableStore()); + const source = URI.from({ scheme: SessionType.AgentHostCopilot, path: '/untitled-source' }); + const input = createInputForCopy(store, source, false); + + const copied = store.add(input.copy() as ChatEditorInput); + + assert.deepStrictEqual({ + copiedScheme: copied.resource.scheme, + copiedSessionResource: copied.sessionResource, + copiedType: getChatSessionType(copied.resource), + }, { + copiedScheme: Schemas.vscodeChatEditor, + copiedSessionResource: undefined, + copiedType: localChatSessionType, + }); + }); }); diff --git a/src/vs/workbench/contrib/styleOverrides/browser/media/notificationsDialogs.css b/src/vs/workbench/contrib/styleOverrides/browser/media/notificationsDialogs.css index 84cad3ed960746..ca5c9cb3fa9232 100644 --- a/src/vs/workbench/contrib/styleOverrides/browser/media/notificationsDialogs.css +++ b/src/vs/workbench/contrib/styleOverrides/browser/media/notificationsDialogs.css @@ -29,8 +29,8 @@ /* * Reposition the notification center to sit within the Modern UI outer gutter: - * an 8px horizontal inset, and a bottom inset that clears the 22px status bar - * plus the gutter (34px total). `!important` is required to win over the base + * an 8px horizontal inset, and a bottom inset that clears the 28px status bar + * plus the 4px gutter (32px total). `!important` is required to win over the base * positioning rules (which vary with the status bar) and, for `top-right`, over * the inline `top` offset the notification controller applies in JS. * @@ -40,13 +40,13 @@ */ .style-override.monaco-workbench > .notifications-center { right: var(--vscode-spacing-size80); - bottom: var(--vscode-spacing-size360); + bottom: var(--vscode-spacing-size320); } .style-override.monaco-workbench > .notifications-center.bottom-left { right: auto; left: var(--vscode-spacing-size80); - bottom: var(--vscode-spacing-size360); + bottom: var(--vscode-spacing-size320); } .style-override.monaco-workbench > .notifications-center.top-right { @@ -57,17 +57,17 @@ /* * Toasts mirror the center, but each toast carries a 4px margin, so every * container offset is reduced by 4px to keep the visible toast surface aligned - * with the center (8px / 34px / 35px once the margin is added back). + * with the center (8px / 32px / 35px once the margin is added back). */ .style-override.monaco-workbench > .notifications-toasts { right: var(--vscode-spacing-size40); - bottom: var(--vscode-spacing-size320); + bottom: var(--vscode-spacing-size280); } .style-override.monaco-workbench > .notifications-toasts.bottom-left { right: auto; left: var(--vscode-spacing-size40); - bottom: var(--vscode-spacing-size320); + bottom: var(--vscode-spacing-size280); } .style-override.monaco-workbench > .notifications-toasts.top-right { diff --git a/src/vs/workbench/test/browser/parts/statusbar/statusbarPart.test.ts b/src/vs/workbench/test/browser/parts/statusbar/statusbarPart.test.ts index 807617bc9b2af5..06a6e5eb29119e 100644 --- a/src/vs/workbench/test/browser/parts/statusbar/statusbarPart.test.ts +++ b/src/vs/workbench/test/browser/parts/statusbar/statusbarPart.test.ts @@ -29,6 +29,14 @@ suite('StatusbarPart', () => { } } + class TestFloatingPanelsLayoutService extends TestLayoutService { + floatingPanelsEnabled = false; + + override isFloatingPanelsEnabled(): boolean { + return this.floatingPanelsEnabled; + } + } + function fireConfigChange(configurationService: TestConfigurationService, key: string): void { configurationService.onDidChangeConfigurationEmitter.fire({ source: ConfigurationTarget.DEFAULT, @@ -75,4 +83,32 @@ suite('StatusbarPart', () => { afterModernUIChange: 2, }); }); + + test('modern UI reserves compact vertical status bar padding', () => { + const configurationService = new TestConfigurationService(); + const instantiationService = store.add(new TestInstantiationService()); + instantiationService.stub(IConfigurationService, configurationService); + instantiationService.stub(IHoverService, new class extends mock() { }); + const contextKeyService = store.add(new ContextKeyService(configurationService)); + const layoutService = new TestFloatingPanelsLayoutService(); + const part = store.add(new TestMainStatusbarPart( + instantiationService, + new TestThemeService(), + new TestContextService(), + store.add(new TestStorageService()), + layoutService, + new TestContextMenuService(), + contextKeyService, + configurationService, + )); + + const defaultConstraints = { minimumHeight: part.minimumHeight, maximumHeight: part.maximumHeight }; + layoutService.floatingPanelsEnabled = true; + const modernUIConstraints = { minimumHeight: part.minimumHeight, maximumHeight: part.maximumHeight }; + + assert.deepStrictEqual({ defaultConstraints, modernUIConstraints }, { + defaultConstraints: { minimumHeight: 22, maximumHeight: 22 }, + modernUIConstraints: { minimumHeight: 28, maximumHeight: 28 }, + }); + }); });