Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chat: fix diff not showing for streaming chat pills #241799

Merged
merged 1 commit into from
Feb 25, 2025
+40 −19
Merged

chat: fix diff not showing for streaming chat pills #241799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/vs/workbench/contrib/chat/browser/chatContentParts/chatContentParts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { ChatTreeItem, IChatCodeBlockInfo } from '../chat.js';
import { IChatRendererContent } from '../../common/chatViewModel.js';

export interface IChatContentPart extends IDisposable {
domNode: HTMLElement;
domNode: HTMLElement | undefined;

/**
* Used to indicate a part's ownership of a code block.
Expand Down
57 changes: 39 additions & 18 deletions src/vs/workbench/contrib/chat/browser/chatListRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ import { ChatAgentLocation, IChatAgentMetadata } from '../common/chatAgents.js';
import { ChatContextKeys } from '../common/chatContextKeys.js';
import { IChatRequestVariableEntry, IChatTextEditGroup } from '../common/chatModel.js';
import { chatSubcommandLeader } from '../common/chatParserTypes.js';
import { ChatAgentVoteDirection, ChatAgentVoteDownReason, IChatConfirmation, IChatContentReference, IChatFollowup, IChatMarkdownContent, IChatTask, IChatToolInvocation, IChatToolInvocationSerialized, IChatTreeData } from '../common/chatService.js';
import { ChatAgentVoteDirection, ChatAgentVoteDownReason, IChatConfirmation, IChatContentReference, IChatFollowup, IChatMarkdownContent, IChatTask, IChatToolInvocation, IChatToolInvocationSerialized, IChatTreeData, IChatUndoStop } from '../common/chatService.js';
import { IChatCodeCitations, IChatReferences, IChatRendererContent, IChatRequestViewModel, IChatResponseViewModel, IChatWorkingProgress, isRequestVM, isResponseVM } from '../common/chatViewModel.js';
import { getNWords } from '../common/chatWordCounter.js';
import { CodeBlockModelCollection } from '../common/codeBlockModelCollection.js';
Expand Down Expand Up @@ -565,14 +565,18 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
&& isRequestVM(element) && element.agentOrSlashCommandDetected && element.slashCommand
&& data.kind === 'markdownContent' // TODO this is fishy but I didn't find a better way to render on the same inline as the MD request part
) {
newPart.domNode.style.display = 'inline-flex';
if (newPart.domNode) {
newPart.domNode.style.display = 'inline-flex';
}
const cmdPart = this.instantiationService.createInstance(ChatAgentCommandContentPart, element.slashCommand, () => this._onDidClickRerunWithAgentOrCommandDetection.fire({ sessionId: element.sessionId, requestId: element.id }));
templateData.value.appendChild(cmdPart.domNode);
parts.push(cmdPart);
inlineSlashCommandRendered = true;
}

templateData.value.appendChild(newPart.domNode);
if (newPart.domNode) {
templateData.value.appendChild(newPart.domNode);
}
parts.push(newPart);
}
});
Expand Down Expand Up @@ -715,21 +719,24 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
};
const newPart = this.renderChatContentPart(partToRender, templateData, context);
if (newPart) {
renderedParts[index] = newPart;
// Maybe the part can't be rendered in this context, but this shouldn't really happen
if (alreadyRenderedPart) {
try {
// This method can throw HierarchyRequestError
alreadyRenderedPart.domNode.replaceWith(newPart.domNode);
} catch (err) {
this.logService.error('ChatListItemRenderer#renderChatContentDiff: error replacing part', err);
try {
if (alreadyRenderedPart?.domNode) {
if (newPart.domNode) {
// This method can throw HierarchyRequestError
alreadyRenderedPart.domNode.replaceWith(newPart.domNode);
} else {
alreadyRenderedPart.domNode.remove();
}
} else if (newPart.domNode) {
templateData.value.appendChild(newPart.domNode);
}
} else {
templateData.value.appendChild(newPart.domNode);
} catch (err) {
this.logService.error('ChatListItemRenderer#renderChatContentDiff: error replacing part', err);
}

renderedParts[index] = newPart;
} else if (alreadyRenderedPart) {
alreadyRenderedPart.domNode.remove();
} else {
alreadyRenderedPart?.domNode?.remove();
}
});
}
Expand All @@ -744,7 +751,6 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch

this.traceLayout('getNextProgressiveRenderContent', `Want to render ${data.numWordsToRender} at ${data.rate} words/s, counting...`);
let numNeededWords = data.numWordsToRender;
const typesToSkip = new Set<string>(['undoStop']);
const partsToRender: IChatRendererContent[] = [];
if (element.contentReferences.length) {
partsToRender.push({ kind: 'references', references: element.contentReferences });
Expand All @@ -763,7 +769,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch

// Consumed full markdown chunk- need to ensure that all following non-markdown parts are rendered
for (const nextPart of renderableResponse.slice(i + 1)) {
if (nextPart.kind !== 'markdownContent' && !typesToSkip.has(nextPart.kind)) {
if (nextPart.kind !== 'markdownContent') {
i++;
partsToRender.push(nextPart);
} else {
Expand All @@ -783,7 +789,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
}
break;
}
} else if (!typesToSkip.has(part.kind)) {
} else {
partsToRender.push(part);
}
}
Expand Down Expand Up @@ -877,11 +883,26 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
return this.renderToolInvocation(content, context, templateData);
} else if (content.kind === 'working') {
return this.renderWorkingProgress(content, context);
} else if (content.kind === 'undoStop') {
return this.renderUndoStop(content);
}

// todo@connor4312/roblourens: should this throw or renderNoContent to avoid progressive render thrashing?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roblourens what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry for the break! If this method returns undefined, it tries to render that part forever? Your renderNoContent makes sense. Actually there's a couple cases already that render an empty dom element, like when a progress message hides.

return undefined;
}

private renderUndoStop(content: IChatUndoStop) {
return this.renderNoContent(other => other.kind === 'undoStop' && other.id === content.id);
}

private renderNoContent(equals: (otherContent: IChatRendererContent) => boolean): IChatContentPart {
return {
dispose: () => { },
domNode: undefined,
hasSameContent: equals,
};
}

private renderTreeData(content: IChatTreeData, templateData: IChatListItemTemplate, context: IChatContentPartRenderContext): IChatContentPart {
const data = content.treeData;
const treeDataIndex = context.preceedingContentParts.filter(part => part instanceof ChatTreeContentPart).length;
Expand Down
Loading