Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the agent session progress cleanup logic in the chat sessions service. The main change replaces the extractFileNameFromLink method with a more comprehensive cleanUpProgress method that handles both file link cleanup and removal of markdown formatting (backticks). The refactoring also improves type safety by preserving the IMarkdownString type through the processing pipeline instead of converting to string immediately.
Key changes:
- Replaced
extractFileNameFromLinkwithcleanUpProgressthat handles both file link cleanup and backtick removal - Updated
getSessionDescriptionto preserveIMarkdownString | string | undefinedtypes until final cleanup - Consolidated cleanup logic to run once at the end instead of multiple times during processing
src/vs/workbench/contrib/chat/browser/chatSessions.contribution.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/chatSessions.contribution.ts
Outdated
Show resolved
Hide resolved
mjbvz
left a comment
mjbvz
left a comment
There was a problem hiding this comment.
Let's see if it's possible to avoid having custom parsing logic
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
…ega/agent-session-progress-cleanup
| renderer.link = ({ text, href }: marked.Tokens.Link): string => { | ||
| if (href && href.startsWith('file://')) { | ||
| const fileName = href.split('/').pop() || href; | ||
| return text.trim() || fileName; |
There was a problem hiding this comment.
This should use basename to get the file name
| let renderer = plainTextRenderer.value; | ||
| if (options?.includeCodeBlocksFences) { | ||
| renderer = plainTextWithCodeBlocksRenderer.value; | ||
| } else if (options?.preserveFileLinks) { |
There was a problem hiding this comment.
Using the API it would be surprising you can't set both includeCodeBlocksFences and preserveFileLinks can be set. Can we make it so both can be enabled?
| /** Controls if the ``` of code blocks should be preserved in the output or not */ | ||
| readonly includeCodeBlocksFences?: boolean; | ||
| /** Controls if file:// links should be preserved as filenames in the output */ | ||
| readonly preserveFileLinks?: boolean; |
There was a problem hiding this comment.
Instead of something so specific, could this be a linkFormatter that takes the text and href and returns the text to display? That way the caller can handle all the file uri / chat specific logic
src/vs/workbench/contrib/chat/browser/chatSessions.contribution.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/chatSessions.contribution.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/chat/browser/chatSessions.contribution.ts
Outdated
Show resolved
Hide resolved
osortega
dismissed
mjbvz’s stale review
Dismissing to be able to make it for next insiders
| ? toolInvocation.confirmationMessages.title | ||
| : toolInvocation.confirmationMessages.title.value); | ||
| description = message ?? localize('chat.sessions.description.waitingForConfirmation', "Waiting for confirmation: {0}", description); | ||
| description = message ?? localize('chat.sessions.description.waitingForConfirmation', "Waiting for confirmation: {0}", typeof description === 'string' ? description : description.value); |
There was a problem hiding this comment.
Should this use renderAsPlainText too? Or even better, preserve it as a markdown string if it starts as one?
| if (options?.includeCodeBlocksFences) { | ||
| renderer.code = codeBlockFences; | ||
| } | ||
| if (options?.useLinkFormatter) { |
There was a problem hiding this comment.
useLinkFormatter still feels like an odd name to me. Could we try doing one of these (best to worst):
- Make whoever is generating the string generate a valid link in the first place
- Make the new link logic the default behavior so we don't need an option. That seems reasonable if there's no
text - Give the option a more clear name such as
useHrefForLinks
No description provided.