Fix markdown setting scope links #274935
Open
Fix markdown setting scope links #274935
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes microsoft#272118 Lets markdown strings use internal uris for links that should be handled entirely by the action handler instead of the opener. Uses this to support setting scope links
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors link creation in markdown to use centralized helper functions instead of inline string concatenation. The changes introduce createMarkdownLink for general links and rename markdownCommandLink to createMarkdownCommandLink for consistency, adds support for internal URI scheme links, and updates all call sites across the codebase.
- Refactored markdown link creation to use new helper functions
createMarkdownLinkandcreateMarkdownCommandLink - Added support for
Schemas.internalin the markdown renderer and opener service - Introduced
SettingScopeLinknamespace for creating and parsing internal setting scope links
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/base/common/htmlContent.ts | Added createMarkdownLink function and renamed markdownCommandLink to createMarkdownCommandLink |
| src/vs/base/browser/markdownRenderer.ts | Added Schemas.internal to allowed schemes in sanitizer config |
| src/vs/editor/browser/services/openerService.ts | Added early return for internal scheme URIs to prevent them from being opened |
| src/vs/workbench/contrib/preferences/browser/settingsEditorSettingIndicators.ts | Replaced inline markdown link construction with createMarkdownLink, added SettingScopeLink namespace |
| src/vs/workbench/contrib/mcp/browser/mcpServersView.ts | Updated to use createMarkdownCommandLink |
| src/vs/workbench/contrib/mcp/browser/mcpLanguageFeatures.ts | Updated to use createMarkdownCommandLink |
| src/vs/workbench/contrib/mcp/browser/mcpCommands.ts | Updated to use createMarkdownCommandLink |
| src/vs/workbench/contrib/chat/browser/chatContentParts/toolInvocationParts/chatToolInvocationPart.ts | Updated to use createMarkdownCommandLink |
| src/vs/workbench/contrib/chat/browser/chatContentParts/chatProgressContentPart.ts | Updated to use createMarkdownCommandLink |
| src/vs/workbench/contrib/chat/browser/chatContentParts/chatMcpServersInteractionContentPart.ts | Updated to use createMarkdownCommandLink |
| src/vs/workbench/contrib/chat/browser/actions/chatToolPicker.ts | Updated to use createMarkdownCommandLink |
| let defaultOverrideHoverContent; | ||
| if (!Array.isArray(sourceToDisplay)) { | ||
| defaultOverrideHoverContent = localize('defaultOverriddenDetails', "Default setting value overridden by `{0}`", sourceToDisplay); | ||
| defaultOverrideHoverContent = localize('defaultOverriddenDetails', "Default setting value overridden by `{ 0 } `", sourceToDisplay); |
There was a problem hiding this comment.
The localization string contains incorrect spacing. Should be {0} instead of { 0 } .
Suggested change
| defaultOverrideHoverContent = localize('defaultOverriddenDetails', "Default setting value overridden by `{ 0 } `", sourceToDisplay); | |
| defaultOverrideHoverContent = localize('defaultOverriddenDetails', "Default setting value overridden by `{0}`", sourceToDisplay); |
Comment on lines
+653
to
+655
| export function parse(link: string): string { | ||
| const uri = URI.parse(link); | ||
| return decodeURIComponent(uri.query); |
There was a problem hiding this comment.
The parse function is used with optional chaining on line 483 (SettingScopeLink.parse(url)?.split(':')), but it always returns a string. It should return string | undefined to handle cases where the URI might be invalid or the query might be empty. Consider adding validation or changing the return type to match the usage pattern.
Suggested change
| export function parse(link: string): string { | |
| const uri = URI.parse(link); | |
| return decodeURIComponent(uri.query); | |
| export function parse(link: string): string | undefined { | |
| const uri = URI.parse(link); | |
| if (uri.query) { | |
| return decodeURIComponent(uri.query); | |
| } | |
| return undefined; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #272118
Lets markdown strings use internal uris for links that should be handled entirely by the action handler instead of the opener. Uses this to support setting scope links