diff --git a/src/vs/workbench/contrib/chat/browser/attachments/chatImplicitContext.ts b/src/vs/workbench/contrib/chat/browser/attachments/chatImplicitContext.ts index 069deb6aa09..eac7f983360 100644 --- a/src/vs/workbench/contrib/chat/browser/attachments/chatImplicitContext.ts +++ b/src/vs/workbench/contrib/chat/browser/attachments/chatImplicitContext.ts @@ -29,6 +29,8 @@ import { ILanguageModelIgnoredFilesService } from '../../common/ignoredFiles.js' import { getPromptsTypeForLanguageId } from '../../common/promptSyntax/promptTypes.js'; import { IChatWidget, IChatWidgetService } from '../chat.js'; import { IChatContextService } from '../contextContrib/chatContextService.js'; +import { ITextModel } from '../../../../../editor/common/model.js'; +import { IRange } from '../../../../../editor/common/core/range.js'; export class ChatImplicitContextContribution extends Disposable implements IWorkbenchContribution { static readonly ID = 'chat.implicitContext'; @@ -210,18 +212,22 @@ export class ChatImplicitContextContribution extends Disposable implements IWork const selection = codeEditor?.getSelection(); const visibleRanges = codeEditor?.getVisibleRanges() || []; newValue = activeCell.uri; - if (isEqual(codeEditor?.getModel()?.uri, activeCell.uri)) { + const cellModel = codeEditor?.getModel(); + if (cellModel && isEqual(cellModel.uri, activeCell.uri)) { if (selection && !selection.isEmpty()) { newValue = { uri: activeCell.uri, range: selection } satisfies Location; isSelection = true; } else if (visibleRanges.length > 0) { - // Merge visible ranges. Maybe the reference value could actually be an array of Locations? - // Something like a Location with an array of Ranges? - let range = visibleRanges[0]; - visibleRanges.slice(1).forEach(r => { - range = range.plusRange(r); - }); - newValue = { uri: activeCell.uri, range } satisfies Location; + // If the entire cell is visible, just use the cell URI, no need to specify range. + if (!isEntireCellVisible(cellModel, visibleRanges)) { + // Merge visible ranges. Maybe the reference value could actually be an array of Locations? + // Something like a Location with an array of Ranges? + let range = visibleRanges[0]; + visibleRanges.slice(1).forEach(r => { + range = range.plusRange(r); + }); + newValue = { uri: activeCell.uri, range } satisfies Location; + } } } } else { @@ -267,6 +273,13 @@ export class ChatImplicitContextContribution extends Disposable implements IWork } } +function isEntireCellVisible(cellModel: ITextModel, visibleRanges: IRange[]): boolean { + if (visibleRanges.length === 1 && visibleRanges[0].startLineNumber === 1 && visibleRanges[0].startColumn === 1 && visibleRanges[0].endLineNumber === cellModel.getLineCount() && visibleRanges[0].endColumn === cellModel.getLineMaxColumn(visibleRanges[0].endLineNumber)) { + return true; + } + return false; +} + export class ChatImplicitContext extends Disposable implements IChatRequestImplicitVariableEntry { get id() { if (URI.isUri(this.value)) { diff --git a/src/vs/workbench/contrib/chat/browser/attachments/implicitContextAttachment.ts b/src/vs/workbench/contrib/chat/browser/attachments/implicitContextAttachment.ts index ea58286526e..5a4815ff15d 100644 --- a/src/vs/workbench/contrib/chat/browser/attachments/implicitContextAttachment.ts +++ b/src/vs/workbench/contrib/chat/browser/attachments/implicitContextAttachment.ts @@ -14,7 +14,7 @@ import { Disposable, DisposableStore } from '../../../../../base/common/lifecycl import { Schemas } from '../../../../../base/common/network.js'; import { basename, dirname } from '../../../../../base/common/resources.js'; import { URI } from '../../../../../base/common/uri.js'; -import { Location } from '../../../../../editor/common/languages.js'; +import { isLocation, Location } from '../../../../../editor/common/languages.js'; import { ILanguageService } from '../../../../../editor/common/languages/language.js'; import { IModelService } from '../../../../../editor/common/services/model.js'; import { localize } from '../../../../../nls.js'; @@ -214,7 +214,11 @@ export class ImplicitContextAttachmentWidget extends Disposable { this.attachmentModel.addContext(context); } else { const file = URI.isUri(this.attachment.value) ? this.attachment.value : this.attachment.value.uri; - this.attachmentModel.addFile(file); + if (file.scheme === Schemas.vscodeNotebookCell && isLocation(this.attachment.value)) { + this.attachmentModel.addFile(file, this.attachment.value.range); + } else { + this.attachmentModel.addFile(file); + } } this.widgetRef()?.focusInput(); } diff --git a/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts b/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts index 1a91d126ce6..e828b4061fb 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts @@ -772,10 +772,13 @@ export class CodeCellLayout { * observe transient Monaco content heights that reflect the current clipped layout (rather than * the full input height). To keep the notebook list layout stable (avoiding overlapping cells * while navigating/scrolling), we store the actual content height in `_establishedContentHeight` - * and reuse it for all layout reasons except `onDidContentSizeChange`. This prevents the editor - * from shrinking back to its initial height after content has been added (e.g., pasting text). - * When `onDidContentSizeChange` fires, we update `_establishedContentHeight` to reflect the new - * content size, which subsequent scroll events will then reuse. + * and reuse it for scroll-driven relayouts. This prevents the editor from shrinking back to its + * initial height after content has been added (e.g., pasting text) or when Monaco reports a + * transient smaller content height while the cell is clipped. + * + * We refresh `_establishedContentHeight` when the editor's content size changes + * (`onDidContentSizeChange`) and also when width/layout changes can affect wrapping-driven height + * (`viewCellLayoutChange`/`nbLayoutChange`). * - Pointer-drag gating: while the user is holding the mouse button down in the editor (drag * selection or potential drag selection), we avoid programmatic `editor.setScrollTop(...)` updates * to prevent selection/scroll feedback loops and "stuck selection" behavior. @@ -870,17 +873,51 @@ export class CodeCellLayout { const elementTop = this.notebookEditor.getAbsoluteTopOfElement(this.viewCell); const elementBottom = this.notebookEditor.getAbsoluteBottomOfElement(this.viewCell); const elementHeight = this.notebookEditor.getHeightOfElement(this.viewCell); - const gotContentHeight = editor.getContentHeight(); - // If we've already calculated the editor content height once before and the contents haven't changed, use that. - const fallbackEditorContentHeight = gotContentHeight === -1 ? Math.max(editor.getLayoutInfo().height, this._initialEditorDimension.height) : gotContentHeight; let editorContentHeight: number; - if (this._initialized && reason !== 'onDidContentSizeChange') { - // Reuse the previously established content height to avoid transient Monaco content height changes during scroll - editorContentHeight = this._establishedContentHeight ?? fallbackEditorContentHeight; - } else { - // Update the established content height when content actually changes or during initialization - editorContentHeight = fallbackEditorContentHeight; + const isInit = !this._initialized && reason === 'init'; + if (isInit) { + // CONTENT HEIGHT SELECTION (INIT) + // ------------------------------- + // Editors are pooled and may be re-attached to different cells as the user scrolls. + // At the moment a pooled editor is first attached to a new cell, Monaco can still + // report the previous cell's `getContentHeight()` (for example a tall multi-line + // cell) even though the new cell only contains a single line. If we trusted that + // stale value here, the very first layout of the new cell would render with an + // oversized editor and visually overlap the next cell. + // + // To avoid this, the initial layout ignores `getContentHeight()` entirely and uses + // the notebook's own notion of the editor height for this cell + // (`_initialEditorDimension.height`). This value is derived from the cell model + // (line count + padding) and is stable across editor reuse. Once the model has + // been resolved and Monaco reports a real content height, subsequent layout + // reasons (`onDidContentSizeChange`, `viewCellLayoutChange`, `nbLayoutChange`) + // will refresh `_establishedContentHeight` in the normal way. + editorContentHeight = this._initialEditorDimension.height; this._establishedContentHeight = editorContentHeight; + } else { + // CONTENT HEIGHT SELECTION (NON-INIT) + // ----------------------------------- + // For all non-init reasons, we rely on Monaco's `getContentHeight()` together with + // `_establishedContentHeight` to keep the notebook list layout stable while + // scrolling and resizing: + // - `onDidContentSizeChange` / `viewCellLayoutChange` / `nbLayoutChange` update + // `_establishedContentHeight` to the latest full content height. + // - `nbDidScroll` reuses `_establishedContentHeight` so that transient, smaller + // values reported while the editor itself is clipped do not shrink the row + // height (which would otherwise cause overlapping cells). + const gotContentHeight = editor.getContentHeight(); + // If we've already calculated the editor content height once before and the contents haven't changed, use that. + const fallbackEditorContentHeight = gotContentHeight === -1 ? Math.max(editor.getLayoutInfo().height, this._initialEditorDimension.height) : gotContentHeight; + const shouldRefreshContentHeight = !this._initialized || reason === 'onDidContentSizeChange' || reason === 'viewCellLayoutChange' || reason === 'nbLayoutChange'; + if (shouldRefreshContentHeight) { + // Update the established content height when content changes, during initialization, + // or when width/layout changes can affect wrapping-driven height. + editorContentHeight = fallbackEditorContentHeight; + this._establishedContentHeight = editorContentHeight; + } else { + // Reuse the previously established content height to avoid transient Monaco content height changes during scroll + editorContentHeight = this._establishedContentHeight ?? fallbackEditorContentHeight; + } } const editorBottom = elementTop + this.viewCell.layoutInfo.outputContainerOffset; const scrollBottom = this.notebookEditor.scrollBottom; diff --git a/src/vs/workbench/contrib/notebook/test/browser/view/cellPart.test.ts b/src/vs/workbench/contrib/notebook/test/browser/view/cellPart.test.ts index 05f60f51c74..36cfcb94107 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/view/cellPart.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/view/cellPart.test.ts @@ -496,6 +496,100 @@ suite('CellPart', () => { ); }); + test('CodeCellLayout refreshes content height on viewCellLayoutChange', () => { + const LINE_HEIGHT = 21; + const CELL_TOP_MARGIN = 6; + const CELL_OUTLINE_WIDTH = 1; + const STATUSBAR_HEIGHT = 22; + const VIEWPORT_HEIGHT = 1000; + const ELEMENT_TOP = 100; + const ELEMENT_HEIGHT = 1200; + const INITIAL_CONTENT_HEIGHT = 37; + const OUTPUT_CONTAINER_OFFSET = 300; + const UPDATED_CONTENT_HEIGHT = 200; + + let contentHeight = INITIAL_CONTENT_HEIGHT; + const stubEditor = { + layoutCalls: [] as { width: number; height: number }[], + _lastScrollTopSet: -1, + getLayoutInfo: () => ({ width: 600, height: INITIAL_CONTENT_HEIGHT }), + getContentHeight: () => contentHeight, + layout: (dim: { width: number; height: number }) => { + stubEditor.layoutCalls.push(dim); + }, + setScrollTop: (v: number) => { + stubEditor._lastScrollTopSet = v; + }, + hasModel: () => true, + }; + const editorPart = { style: { top: '' } }; + const template: Partial = { + editor: stubEditor as unknown as ICodeEditor, + editorPart: editorPart as unknown as HTMLElement, + }; + const viewCell: Partial = { + isInputCollapsed: false, + layoutInfo: { + statusBarHeight: STATUSBAR_HEIGHT, + topMargin: CELL_TOP_MARGIN, + outlineWidth: CELL_OUTLINE_WIDTH, + editorHeight: INITIAL_CONTENT_HEIGHT, + outputContainerOffset: OUTPUT_CONTAINER_OFFSET, + editorWidth: 600, + } as unknown as CodeCellLayoutInfo, + }; + const notebookEditor = { + scrollTop: 0, + get scrollBottom() { + return VIEWPORT_HEIGHT; + }, + setScrollTop: (v: number) => { + /* no-op */ + }, + getLayoutInfo: () => ({ + fontInfo: { lineHeight: LINE_HEIGHT }, + height: VIEWPORT_HEIGHT, + stickyHeight: 0, + }), + getAbsoluteTopOfElement: () => ELEMENT_TOP, + getAbsoluteBottomOfElement: () => ELEMENT_TOP + OUTPUT_CONTAINER_OFFSET, + getHeightOfElement: () => ELEMENT_HEIGHT, + notebookOptions: { + getLayoutConfiguration: () => ({ editorTopPadding: 6 }), + }, + }; + + const layout = new CodeCellLayout( + true, + notebookEditor as unknown as IActiveNotebookEditorDelegate, + viewCell as CodeCellViewModel, + template as CodeCellRenderTemplate, + { debug: () => { } }, + { width: 600, height: INITIAL_CONTENT_HEIGHT } + ); + + layout.layoutEditor('init'); + assert.strictEqual(stubEditor.layoutCalls.at(-1)?.height, INITIAL_CONTENT_HEIGHT); + + // Simulate wrapping-driven height increase after width/layout settles. + contentHeight = UPDATED_CONTENT_HEIGHT; + layout.layoutEditor('viewCellLayoutChange'); + assert.strictEqual( + stubEditor.layoutCalls.at(-1)?.height, + UPDATED_CONTENT_HEIGHT, + 'viewCellLayoutChange should refresh the content height' + ); + + // Ensure subsequent scrolls still reuse the established (larger) height. + contentHeight = 50; + layout.layoutEditor('nbDidScroll'); + assert.strictEqual( + stubEditor.layoutCalls.at(-1)?.height, + UPDATED_CONTENT_HEIGHT, + 'nbDidScroll should reuse the refreshed content height' + ); + }); + test('CodeCellLayout maintains content height after paste when scrolling', () => { /** * Regression test for https://github.com/microsoft/vscode/issues/284524 @@ -709,4 +803,143 @@ suite('CellPart', () => { 'Expected editor.setScrollTop to resume once pointer is released' ); }); + + test('CodeCellLayout init ignores stale pooled editor content height', () => { + /** + * Regression guard for fast-scroll overlap when editors are pooled. + * + * A Monaco editor instance can be reused between cells. If we trusted the pooled + * editor's `getContentHeight()` during the first layout of a new cell, a short + * cell might inherit a previous tall cell's content height and render with an + * oversized editor, visually overlapping the next cell. The layout should instead + * seed its initial content height from the cell's own initial editor dimension. + */ + const LINE_HEIGHT = 21; + const CELL_TOP_MARGIN = 6; + const CELL_OUTLINE_WIDTH = 1; + const STATUSBAR_HEIGHT = 22; + const VIEWPORT_HEIGHT = 400; + const ELEMENT_TOP = 100; + const ELEMENT_HEIGHT = 500; + const OUTPUT_CONTAINER_OFFSET = 200; + + let pooledContentHeight = 200; // tall previous cell + const pooledEditor = { + layoutCalls: [] as { width: number; height: number }[], + _lastScrollTopSet: -1, + getLayoutInfo: () => ({ width: 600, height: pooledContentHeight }), + getContentHeight: () => pooledContentHeight, + layout: (dim: { width: number; height: number }) => { + pooledEditor.layoutCalls.push(dim); + }, + setScrollTop: (v: number) => { + pooledEditor._lastScrollTopSet = v; + }, + hasModel: () => true, + }; + const editorPart = { style: { top: '' } }; + const template: Partial = { + editor: pooledEditor as unknown as ICodeEditor, + editorPart: editorPart as unknown as HTMLElement, + }; + + // First, layout a tall cell to establish a large content height on the pooled editor. + const tallViewCell: Partial = { + isInputCollapsed: false, + layoutInfo: { + statusBarHeight: STATUSBAR_HEIGHT, + topMargin: CELL_TOP_MARGIN, + outlineWidth: CELL_OUTLINE_WIDTH, + editorHeight: 200, + outputContainerOffset: OUTPUT_CONTAINER_OFFSET, + editorWidth: 600, + } as unknown as CodeCellLayoutInfo, + }; + const tallNotebookEditor = { + scrollTop: 0, + get scrollBottom() { + return VIEWPORT_HEIGHT; + }, + setScrollTop: (_v: number) => { + /* no-op for this test */ + }, + getLayoutInfo: () => ({ + fontInfo: { lineHeight: LINE_HEIGHT }, + height: VIEWPORT_HEIGHT, + stickyHeight: 0, + }), + getAbsoluteTopOfElement: () => ELEMENT_TOP, + getAbsoluteBottomOfElement: () => ELEMENT_TOP + OUTPUT_CONTAINER_OFFSET, + getHeightOfElement: () => ELEMENT_HEIGHT, + notebookOptions: { + getLayoutConfiguration: () => ({ editorTopPadding: 6 }), + }, + }; + + const tallLayout = new CodeCellLayout( + true, + tallNotebookEditor as unknown as IActiveNotebookEditorDelegate, + tallViewCell as CodeCellViewModel, + template as CodeCellRenderTemplate, + { debug: () => { } }, + { width: 600, height: 200 } + ); + + tallLayout.layoutEditor('init'); + assert.strictEqual( + pooledEditor.layoutCalls.at(-1)?.height, + 200, + 'Expected tall cell to lay out using its own height' + ); + + // Now reuse the same editor for a short cell while leaving the pooled content height large. + pooledContentHeight = 200; // simulate stale value from previous cell + const shortViewCell: Partial = { + isInputCollapsed: false, + layoutInfo: { + statusBarHeight: STATUSBAR_HEIGHT, + topMargin: CELL_TOP_MARGIN, + outlineWidth: CELL_OUTLINE_WIDTH, + editorHeight: 37, + outputContainerOffset: OUTPUT_CONTAINER_OFFSET, + editorWidth: 600, + } as unknown as CodeCellLayoutInfo, + }; + const shortNotebookEditor = { + scrollTop: 0, + get scrollBottom() { + return VIEWPORT_HEIGHT; + }, + setScrollTop: (_v: number) => { + /* no-op for this test */ + }, + getLayoutInfo: () => ({ + fontInfo: { lineHeight: LINE_HEIGHT }, + height: VIEWPORT_HEIGHT, + stickyHeight: 0, + }), + getAbsoluteTopOfElement: () => ELEMENT_TOP, + getAbsoluteBottomOfElement: () => ELEMENT_TOP + OUTPUT_CONTAINER_OFFSET, + getHeightOfElement: () => ELEMENT_HEIGHT, + notebookOptions: { + getLayoutConfiguration: () => ({ editorTopPadding: 6 }), + }, + }; + + const shortLayout = new CodeCellLayout( + true, + shortNotebookEditor as unknown as IActiveNotebookEditorDelegate, + shortViewCell as CodeCellViewModel, + template as CodeCellRenderTemplate, + { debug: () => { } }, + { width: 600, height: 37 } + ); + + shortLayout.layoutEditor('init'); + assert.strictEqual( + pooledEditor.layoutCalls.at(-1)?.height, + 37, + 'Init layout for a short cell should use the cell\'s initial height, not the pooled editor\'s stale content height' + ); + }); });