From 35651d9b457e1701bfc3292d3363d56c6e55e27f Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Thu, 3 Sep 2026 12:22:54 +0200 Subject: [PATCH 1/2] Remove the GHPR uri opening workaround in favor of external URI opener See https://github.com/microsoft/vscode-pull-request-github/pull/8922 --- .../editor/browser/services/openerService.ts | 46 ++++-- .../browser/services/openerService.test.ts | 131 ++++++++++++++++++ src/vs/platform/opener/common/opener.ts | 6 +- .../github/browser/pullRequestActions.ts | 24 +--- .../test/browser/pullRequestActions.test.ts | 125 ++--------------- 5 files changed, 185 insertions(+), 147 deletions(-) diff --git a/src/vs/editor/browser/services/openerService.ts b/src/vs/editor/browser/services/openerService.ts index 1453d666dfa9f1..24b778d40523d3 100644 --- a/src/vs/editor/browser/services/openerService.ts +++ b/src/vs/editor/browser/services/openerService.ts @@ -98,6 +98,10 @@ class EditorOpener implements IOpener { } } +function shouldOpenExternal(target: URI | string, options: OpenOptions | undefined): boolean { + return !!options?.openExternal || matchesSomeScheme(target, Schemas.mailto, Schemas.http, Schemas.https, Schemas.vsls); +} + export class OpenerService implements IOpenerService { declare readonly _serviceBrand: undefined; @@ -109,6 +113,7 @@ export class OpenerService implements IOpenerService { private _defaultExternalOpener: IExternalOpener; private readonly _externalOpeners = new LinkedList(); + private readonly _externalResourceOpener: IOpener; constructor( @ICodeEditorService editorService: ICodeEditorService, @@ -131,16 +136,17 @@ export class OpenerService implements IOpenerService { }; // Default opener: any external, maito, http(s), command, and catch-all-editors - this._openers.push({ + this._externalResourceOpener = { open: async (target: URI | string, options?: OpenOptions) => { - if (options?.openExternal || matchesSomeScheme(target, Schemas.mailto, Schemas.http, Schemas.https, Schemas.vsls)) { + if (shouldOpenExternal(target, options)) { // open externally await this._doOpenExternal(target, options); return true; } return false; } - }); + }; + this._openers.push(this._externalResourceOpener); this._openers.push(new CommandOpener(commandService)); this._openers.push(new EditorOpener(editorService)); } @@ -177,18 +183,19 @@ export class OpenerService implements IOpenerService { return false; } - // check with contributed validators - if (!options?.skipValidation) { + const deferValidation = !!options?.allowContributedOpeners && shouldOpenExternal(target, options); + if (!options?.skipValidation && !deferValidation) { const validationTarget = this._resolvedUriTargets.get(targetURI) ?? target; // validate against the original URI that this URI resolves to, if one exists - for (const validator of this._validators) { - if (!(await validator.shouldOpen(validationTarget, options))) { - return false; - } + if (!(await this._validate(validationTarget, options))) { + return false; } } // check with contributed openers for (const opener of this._openers) { + if (deferValidation && opener === this._externalResourceOpener) { + return this._doOpenExternal(target, options, true); + } const handled = await opener.open(target, options); if (handled) { return true; @@ -216,7 +223,7 @@ export class OpenerService implements IOpenerService { throw new Error('Could not resolve external URI: ' + resource.toString()); } - private async _doOpenExternal(resource: URI | string, options: OpenOptions | undefined): Promise { + private async _doOpenExternal(resource: URI | string, options: OpenOptions | undefined, validateResolved = false): Promise { //todo@jrieken IExternalUriResolver should support `uri: URI | string` const uri = typeof resource === 'string' ? URI.parse(resource) : resource; @@ -228,8 +235,9 @@ export class OpenerService implements IOpenerService { externalUri = uri; } + const preserveOriginalString = typeof resource === 'string' && uri.toString() === externalUri.toString(); let href: string; - if (typeof resource === 'string' && uri.toString() === externalUri.toString()) { + if (preserveOriginalString) { // open the url-string AS IS href = resource; } else { @@ -250,9 +258,25 @@ export class OpenerService implements IOpenerService { } } + if (validateResolved && !options?.skipValidation) { + const validationTarget = preserveOriginalString ? resource : externalUri; + if (!(await this._validate(validationTarget, options))) { + return false; + } + } + return this._defaultExternalOpener.openExternal(href, { sourceUri: uri }, CancellationToken.None); } + private async _validate(resource: URI | string, options: OpenOptions | undefined): Promise { + for (const validator of this._validators) { + if (!(await validator.shouldOpen(resource, options))) { + return false; + } + } + return true; + } + dispose() { this._validators.clear(); } diff --git a/src/vs/editor/test/browser/services/openerService.test.ts b/src/vs/editor/test/browser/services/openerService.test.ts index 2046fc54d4e746..c35de4840a26d4 100644 --- a/src/vs/editor/test/browser/services/openerService.test.ts +++ b/src/vs/editor/test/browser/services/openerService.test.ts @@ -151,6 +151,137 @@ suite('OpenerService', function () { assert.strictEqual(openCount, 2); }); + test('contributed external URI openers run before validators', async function () { + const openerService = new OpenerService(editorService, commandService); + const sourceUri = URI.parse('https://source.example.com'); + const resolvedUri = URI.parse('https://resolved.example.com'); + const calls: string[] = []; + + store.add(openerService.registerExternalUriResolver({ + async resolveExternalUri() { + calls.push('resolve'); + return { resolved: resolvedUri, dispose() { } }; + } + })); + store.add(openerService.registerValidator({ + shouldOpen() { + calls.push('validate'); + return Promise.resolve(false); + } + })); + store.add(openerService.registerOpener({ + async open() { + calls.push('opener'); + return false; + } + })); + store.add(openerService.registerExternalOpener({ + async openExternal(href, context) { + calls.push(`contributed:${href}:${context.sourceUri.toString()}`); + return true; + } + })); + + const didOpen = await openerService.open(sourceUri, { openExternal: true, allowContributedOpeners: true }); + + assert.deepStrictEqual({ + didOpen, + calls, + }, { + didOpen: true, + calls: [ + 'opener', + 'resolve', + `contributed:${resolvedUri.toString()}:${sourceUri.toString()}`, + ], + }); + }); + + test('external URI fallback validates the resolved URI', async function () { + const openerService = new OpenerService(editorService, commandService); + const sourceUri = URI.parse('https://source.example.com'); + const resolvedUri = URI.parse('https://resolved.example.com'); + const calls: string[] = []; + + store.add(openerService.registerExternalUriResolver({ + async resolveExternalUri() { + calls.push('resolve'); + return { resolved: resolvedUri, dispose() { } }; + } + })); + store.add(openerService.registerExternalOpener({ + async openExternal(href, context) { + calls.push(`contributed:${href}:${context.sourceUri.toString()}`); + return false; + } + })); + store.add(openerService.registerValidator({ + shouldOpen(resource) { + calls.push(`validate:${resource.toString()}`); + return Promise.resolve(false); + } + })); + openerService.setDefaultExternalOpener({ + async openExternal(href) { + calls.push(`default:${href}`); + return true; + } + }); + + const didOpen = await openerService.open(sourceUri, { openExternal: true, allowContributedOpeners: true }); + + assert.deepStrictEqual({ + didOpen, + calls, + }, { + didOpen: false, + calls: [ + 'resolve', + `contributed:${resolvedUri.toString()}:${sourceUri.toString()}`, + `validate:${resolvedUri.toString()}`, + ], + }); + }); + + test('external URI fallback preserves strings for validation and opening', async function () { + const openerService = new OpenerService(editorService, commandService); + const source = 'https://source.example.com/path?value=%2B'; + const calls: string[] = []; + + store.add(openerService.registerExternalOpener({ + async openExternal() { + calls.push('contributed'); + return false; + } + })); + store.add(openerService.registerValidator({ + shouldOpen(resource) { + calls.push(`validate:${resource.toString()}`); + return Promise.resolve(true); + } + })); + openerService.setDefaultExternalOpener({ + async openExternal(href) { + calls.push(`default:${href}`); + return true; + } + }); + + const didOpen = await openerService.open(source, { openExternal: true, allowContributedOpeners: true }); + + assert.deepStrictEqual({ + didOpen, + calls, + }, { + didOpen: true, + calls: [ + 'contributed', + `validate:${source}`, + `default:${source}`, + ], + }); + }); + test('links aren\'t manipulated before being passed to validator: PR #118226', async function () { const openerService = new OpenerService(editorService, commandService); diff --git a/src/vs/platform/opener/common/opener.ts b/src/vs/platform/opener/common/opener.ts index 81322829405739..4d858b75b92d7d 100644 --- a/src/vs/platform/opener/common/opener.ts +++ b/src/vs/platform/opener/common/opener.ts @@ -41,6 +41,10 @@ export type OpenInternalOptions = { export type OpenExternalOptions = { readonly openExternal?: boolean; readonly allowTunneling?: boolean; + /** + * Allows contributed external URI openers. These openers are tried before validators, + * which validate the resolved URI only when falling back to the default external opener. + */ readonly allowContributedOpeners?: boolean | string; readonly fromWorkspace?: boolean; readonly skipValidation?: boolean; @@ -82,7 +86,7 @@ export interface IOpenerService { /** * Register a participant that can validate if the URI resource be opened. - * Validators are run before openers. + * Validators run before openers unless contributed external URI openers are enabled. */ registerValidator(validator: IValidator): IDisposable; diff --git a/src/vs/sessions/contrib/github/browser/pullRequestActions.ts b/src/vs/sessions/contrib/github/browser/pullRequestActions.ts index f6e92582361788..e6948f1506f58e 100644 --- a/src/vs/sessions/contrib/github/browser/pullRequestActions.ts +++ b/src/vs/sessions/contrib/github/browser/pullRequestActions.ts @@ -25,9 +25,7 @@ import { IHoverService } from '../../../../platform/hover/browser/hover.js'; import { ServicesAccessor } from '../../../../platform/instantiation/common/instantiation.js'; import { IOpenerService } from '../../../../platform/opener/common/opener.js'; import { asCssVariable } from '../../../../platform/theme/common/colorUtils.js'; -import { IURLService } from '../../../../platform/url/common/url.js'; import { IWorkbenchContribution, registerWorkbenchContribution2, WorkbenchPhase } from '../../../../workbench/common/contributions.js'; -import { IExtensionService } from '../../../../workbench/services/extensions/common/extensions.js'; import { Menus } from '../../../browser/menus.js'; import { ChatPillActionViewItem } from '../../../../workbench/browser/chatPills.js'; import { IActionViewItemOptions } from '../../../../base/browser/ui/actionbar/actionViewItems.js'; @@ -66,9 +64,6 @@ interface IPullRequestListEntry extends IGitHubReferenceListEntry { // --- Open Pull Request action -const githubPullRequestsExtensionId = 'github.vscode-pull-request-github'; -const openPullRequestWebviewPath = '/open-pull-request-webview'; - class PullRequestActionContext { constructor(readonly pullRequest: IGitHubPullRequestRef) { } } @@ -119,25 +114,8 @@ class OpenPullRequestAction extends Action2 { return; } - const extensionService = accessor.get(IExtensionService); - const urlService = accessor.get(IURLService); const openerService = accessor.get(IOpenerService); - if (await extensionService.getExtension(githubPullRequestsExtensionId)) { - const uri = urlService.create({ - authority: githubPullRequestsExtensionId, - path: openPullRequestWebviewPath, - query: JSON.stringify({ - owner: pullRequest.owner, - repo: pullRequest.repo, - pullRequestNumber: pullRequest.number, - }), - }); - if (await urlService.open(uri, { trusted: true })) { - return; - } - } - - await openerService.open(pullRequest.uri, { openExternal: true }); + await openerService.open(pullRequest.uri, { openExternal: true, allowContributedOpeners: true }); } } registerAction2(OpenPullRequestAction); diff --git a/src/vs/sessions/contrib/github/test/browser/pullRequestActions.test.ts b/src/vs/sessions/contrib/github/test/browser/pullRequestActions.test.ts index eecfad0ba4ffe1..983af869ce5caa 100644 --- a/src/vs/sessions/contrib/github/test/browser/pullRequestActions.test.ts +++ b/src/vs/sessions/contrib/github/test/browser/pullRequestActions.test.ts @@ -6,17 +6,14 @@ import assert from 'assert'; import { Codicon } from '../../../../../base/common/codicons.js'; import { constObservable } from '../../../../../base/common/observable.js'; -import { URI, UriComponents } from '../../../../../base/common/uri.js'; +import { URI } from '../../../../../base/common/uri.js'; import { mock } from '../../../../../base/test/common/mock.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { CommandsRegistry } from '../../../../../platform/commands/common/commands.js'; import { isIMenuItem, MenuRegistry } from '../../../../../platform/actions/common/actions.js'; import { IClipboardService } from '../../../../../platform/clipboard/common/clipboardService.js'; -import { IExtensionDescription } from '../../../../../platform/extensions/common/extensions.js'; import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; import { IOpenerService } from '../../../../../platform/opener/common/opener.js'; -import { IOpenURLOptions, IURLService } from '../../../../../platform/url/common/url.js'; -import { IExtensionService } from '../../../../../workbench/services/extensions/common/extensions.js'; import { Menus } from '../../../../browser/menus.js'; import { SessionHasPullRequestContext } from '../../../../common/contextkeys.js'; import { IGitHubPullRequestRef, ISession, ISessionWorkspace } from '../../../../services/sessions/common/session.js'; @@ -54,24 +51,15 @@ function createSessionWithPullRequest(pullRequestUri: URI | undefined, pullReque }; } -class TestURLService extends mock() { - readonly opened: { readonly uri: URI; readonly options: IOpenURLOptions | undefined }[] = []; - - override create(options?: Partial): URI { - return URI.from({ scheme: 'code-oss', ...options }); - } - - override async open(uri: URI, options?: IOpenURLOptions): Promise { - this.opened.push({ uri, options }); - return true; - } -} - class TestOpenerService extends mock() { - readonly opened: { readonly resource: URI; readonly openExternal: boolean | undefined }[] = []; + readonly opened: { readonly resource: URI; readonly openExternal: boolean | undefined; readonly allowContributedOpeners: boolean | string | undefined }[] = []; - override async open(resource: URI, options?: { readonly openExternal?: boolean }): Promise { - this.opened.push({ resource, openExternal: options?.openExternal }); + override async open(resource: URI, options?: { readonly openExternal?: boolean; readonly allowContributedOpeners?: boolean | string }): Promise { + this.opened.push({ + resource, + openExternal: options?.openExternal, + allowContributedOpeners: options?.allowContributedOpeners, + }); return true; } } @@ -137,63 +125,24 @@ suite('Pull Request Actions', () => { assert.deepStrictEqual(clipboardService.writes, []); }); - test('Open Pull Request opens the pull request URL externally when the GitHub Pull Requests extension is unavailable', async () => { + test('Open Pull Request allows contributed external URI openers', async () => { const pullRequestUri = URI.parse('https://github.com/owner/repo/pull/1'); const session = createSessionWithPullRequest(pullRequestUri); const instantiationService = new TestInstantiationService(); - const urlService = new TestURLService(); const openerService = new TestOpenerService(); - instantiationService.stub(IExtensionService, new class extends mock() { - override async getExtension(): Promise { - return undefined; - } - }); instantiationService.stub(IOpenerService, openerService); - instantiationService.stub(IURLService, urlService); instantiationService.stub(ISessionsService, new class extends mock() { override readonly activeSession = constObservable(undefined); }); await instantiationService.invokeFunction(accessor => CommandsRegistry.getCommand('workbench.agentSessions.action.openPullRequest')!.handler(accessor, session)); - assert.deepStrictEqual({ - handledUris: urlService.opened, - opened: openerService.opened, - }, { - handledUris: [], - opened: [{ resource: pullRequestUri, openExternal: true }], - }); - }); - - test('Open Pull Request prefers the explicit pull request repository identity', async () => { - const pullRequestUri = URI.parse('https://github.com/upstream/project/pull/7'); - const session = createSessionWithPullRequest(pullRequestUri, [{ - owner: 'upstream', - repo: 'project', - number: 7, - uri: pullRequestUri, + assert.deepStrictEqual(openerService.opened, [{ + resource: pullRequestUri, + openExternal: true, + allowContributedOpeners: true, }]); - const instantiationService = new TestInstantiationService(); - const urlService = new TestURLService(); - instantiationService.stub(IExtensionService, new class extends mock() { - override async getExtension(): Promise { - return new class extends mock() { }; - } - }); - instantiationService.stub(IOpenerService, new TestOpenerService()); - instantiationService.stub(IURLService, urlService); - instantiationService.stub(ISessionsService, new class extends mock() { - override readonly activeSession = constObservable(undefined); - }); - - await instantiationService.invokeFunction(accessor => CommandsRegistry.getCommand('workbench.agentSessions.action.openPullRequest')!.handler(accessor, session)); - - assert.deepStrictEqual(JSON.parse(urlService.opened[0].uri.query), { - owner: 'upstream', - repo: 'project', - pullRequestNumber: 7, - }); }); test('Copy Pull Request URL uses an explicit contextual pull request', async () => { @@ -217,52 +166,4 @@ suite('Pull Request Actions', () => { assert.deepStrictEqual(clipboardService.writes, [secondPullRequestUri.toString(true)]); }); - test('Open Pull Request uses the GitHub Pull Requests extension when available', async () => { - const pullRequestUri = URI.parse('https://github.com/owner/repo/pull/1'); - const session = createSessionWithPullRequest(pullRequestUri); - - const instantiationService = new TestInstantiationService(); - const requestedExtensionIds: string[] = []; - const urlService = new TestURLService(); - const openerService = new TestOpenerService(); - instantiationService.stub(IExtensionService, new class extends mock() { - override async getExtension(id: string): Promise { - requestedExtensionIds.push(id); - return new class extends mock() { }; - } - }); - instantiationService.stub(IOpenerService, openerService); - instantiationService.stub(IURLService, urlService); - instantiationService.stub(ISessionsService, new class extends mock() { - override readonly activeSession = constObservable(undefined); - }); - - await instantiationService.invokeFunction(accessor => CommandsRegistry.getCommand('workbench.agentSessions.action.openPullRequest')!.handler(accessor, session)); - - assert.deepStrictEqual({ - requestedExtensionIds, - handledUris: urlService.opened.map(({ uri, options }) => ({ - scheme: uri.scheme, - authority: uri.authority, - path: uri.path, - query: JSON.parse(uri.query), - trusted: options?.trusted, - })), - opened: openerService.opened, - }, { - requestedExtensionIds: ['github.vscode-pull-request-github'], - handledUris: [{ - scheme: 'code-oss', - authority: 'github.vscode-pull-request-github', - path: '/open-pull-request-webview', - query: { - owner: 'owner', - repo: 'repo', - pullRequestNumber: 1, - }, - trusted: true, - }], - opened: [], - }); - }); }); From 11501f7c95fb3928d66b77d95f4e9d1c561cd0ec Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:59:43 +0200 Subject: [PATCH 2/2] CCR feedback --- .../editor/browser/services/openerService.ts | 76 ++++++++----- .../browser/services/openerService.test.ts | 100 ++++++++++++++++-- src/vs/platform/opener/common/opener.ts | 2 + .../externalUriOpener/common/configuration.ts | 3 +- 4 files changed, 148 insertions(+), 33 deletions(-) diff --git a/src/vs/editor/browser/services/openerService.ts b/src/vs/editor/browser/services/openerService.ts index 24b778d40523d3..6672d9647b851c 100644 --- a/src/vs/editor/browser/services/openerService.ts +++ b/src/vs/editor/browser/services/openerService.ts @@ -16,7 +16,13 @@ import { URI } from '../../../base/common/uri.js'; import { ICodeEditorService } from './codeEditorService.js'; import { ICommandService } from '../../../platform/commands/common/commands.js'; import { EditorOpenSource } from '../../../platform/editor/common/editor.js'; -import { extractSelection, IExternalOpener, IExternalUriResolver, IOpener, IOpenerService, IResolvedExternalUri, IValidator, OpenOptions, ResolveExternalUriOptions } from '../../../platform/opener/common/opener.js'; +import { defaultExternalUriOpenerId, extractSelection, IExternalOpener, IExternalUriResolver, IOpener, IOpenerService, IResolvedExternalUri, IValidator, OpenOptions, ResolveExternalUriOptions } from '../../../platform/opener/common/opener.js'; + +interface IExternalUriOpenTarget { + readonly sourceUri: URI; + readonly href: string; + readonly validationTarget: URI | string; +} class CommandOpener implements IOpener { @@ -102,6 +108,12 @@ function shouldOpenExternal(target: URI | string, options: OpenOptions | undefin return !!options?.openExternal || matchesSomeScheme(target, Schemas.mailto, Schemas.http, Schemas.https, Schemas.vsls); } +function shouldUseContributedExternalOpeners(target: URI | string, options: OpenOptions | undefined): boolean { + return !!options?.allowContributedOpeners + && options.allowContributedOpeners !== defaultExternalUriOpenerId + && shouldOpenExternal(target, options); +} + export class OpenerService implements IOpenerService { declare readonly _serviceBrand: undefined; @@ -183,9 +195,16 @@ export class OpenerService implements IOpenerService { return false; } - const deferValidation = !!options?.allowContributedOpeners && shouldOpenExternal(target, options); - if (!options?.skipValidation && !deferValidation) { - const validationTarget = this._resolvedUriTargets.get(targetURI) ?? target; // validate against the original URI that this URI resolves to, if one exists + let externalUriOpenTarget: IExternalUriOpenTarget | undefined; + if (shouldUseContributedExternalOpeners(target, options)) { + externalUriOpenTarget = await this._resolveExternalUriOpenTarget(target, options); + if (await this._openWithContributedExternalOpeners(externalUriOpenTarget, options)) { + return true; + } + } + + if (!options?.skipValidation) { + const validationTarget = externalUriOpenTarget?.validationTarget ?? this._resolvedUriTargets.get(targetURI) ?? target; if (!(await this._validate(validationTarget, options))) { return false; } @@ -193,8 +212,8 @@ export class OpenerService implements IOpenerService { // check with contributed openers for (const opener of this._openers) { - if (deferValidation && opener === this._externalResourceOpener) { - return this._doOpenExternal(target, options, true); + if (externalUriOpenTarget && opener === this._externalResourceOpener) { + return this._openDefaultExternal(externalUriOpenTarget); } const handled = await opener.open(target, options); if (handled) { @@ -223,8 +242,7 @@ export class OpenerService implements IOpenerService { throw new Error('Could not resolve external URI: ' + resource.toString()); } - private async _doOpenExternal(resource: URI | string, options: OpenOptions | undefined, validateResolved = false): Promise { - + private async _resolveExternalUriOpenTarget(resource: URI | string, options: OpenOptions | undefined): Promise { //todo@jrieken IExternalUriResolver should support `uri: URI | string` const uri = typeof resource === 'string' ? URI.parse(resource) : resource; let externalUri: URI; @@ -245,27 +263,35 @@ export class OpenerService implements IOpenerService { href = encodeURI(externalUri.toString(true)); } - if (options?.allowContributedOpeners) { - const preferredOpenerId = typeof options?.allowContributedOpeners === 'string' ? options?.allowContributedOpeners : undefined; - for (const opener of this._externalOpeners) { - const didOpen = await opener.openExternal(href, { - sourceUri: uri, - preferredOpenerId, - }, CancellationToken.None); - if (didOpen) { - return true; - } - } - } + return { + sourceUri: uri, + href, + validationTarget: preserveOriginalString ? resource : externalUri, + }; + } - if (validateResolved && !options?.skipValidation) { - const validationTarget = preserveOriginalString ? resource : externalUri; - if (!(await this._validate(validationTarget, options))) { - return false; + private async _openWithContributedExternalOpeners(target: IExternalUriOpenTarget, options: OpenOptions | undefined): Promise { + const preferredOpenerId = typeof options?.allowContributedOpeners === 'string' ? options.allowContributedOpeners : undefined; + for (const opener of this._externalOpeners) { + const didOpen = await opener.openExternal(target.href, { + sourceUri: target.sourceUri, + preferredOpenerId, + }, CancellationToken.None); + if (didOpen) { + return true; } } - return this._defaultExternalOpener.openExternal(href, { sourceUri: uri }, CancellationToken.None); + return false; + } + + private _openDefaultExternal(target: IExternalUriOpenTarget): Promise { + return this._defaultExternalOpener.openExternal(target.href, { sourceUri: target.sourceUri }, CancellationToken.None); + } + + private async _doOpenExternal(resource: URI | string, options: OpenOptions | undefined): Promise { + const target = await this._resolveExternalUriOpenTarget(resource, options); + return this._openDefaultExternal(target); } private async _validate(resource: URI | string, options: OpenOptions | undefined): Promise { diff --git a/src/vs/editor/test/browser/services/openerService.test.ts b/src/vs/editor/test/browser/services/openerService.test.ts index c35de4840a26d4..f41628496d5dec 100644 --- a/src/vs/editor/test/browser/services/openerService.test.ts +++ b/src/vs/editor/test/browser/services/openerService.test.ts @@ -13,6 +13,7 @@ import { NullCommandService } from '../../../../platform/commands/test/common/nu import { ITextEditorOptions } from '../../../../platform/editor/common/editor.js'; import { matchesScheme, matchesSomeScheme } from '../../../../base/common/network.js'; import { TestThemeService } from '../../../../platform/theme/test/common/testThemeService.js'; +import { defaultExternalUriOpenerId } from '../../../../platform/opener/common/opener.js'; suite('OpenerService', function () { const themeService = new TestThemeService(); @@ -163,18 +164,18 @@ suite('OpenerService', function () { return { resolved: resolvedUri, dispose() { } }; } })); - store.add(openerService.registerValidator({ - shouldOpen() { - calls.push('validate'); - return Promise.resolve(false); - } - })); store.add(openerService.registerOpener({ async open() { calls.push('opener'); return false; } })); + store.add(openerService.registerValidator({ + shouldOpen() { + calls.push('validate'); + return Promise.resolve(false); + } + })); store.add(openerService.registerExternalOpener({ async openExternal(href, context) { calls.push(`contributed:${href}:${context.sourceUri.toString()}`); @@ -190,7 +191,6 @@ suite('OpenerService', function () { }, { didOpen: true, calls: [ - 'opener', 'resolve', `contributed:${resolvedUri.toString()}:${sourceUri.toString()}`, ], @@ -221,6 +221,12 @@ suite('OpenerService', function () { return Promise.resolve(false); } })); + store.add(openerService.registerOpener({ + async open() { + calls.push('opener'); + return true; + } + })); openerService.setDefaultExternalOpener({ async openExternal(href) { calls.push(`default:${href}`); @@ -243,6 +249,86 @@ suite('OpenerService', function () { }); }); + test('default external URI opener validates before regular openers', async function () { + const openerService = new OpenerService(editorService, commandService); + const sourceUri = URI.parse('https://source.example.com'); + const calls: string[] = []; + + store.add(openerService.registerExternalOpener({ + async openExternal() { + calls.push('contributed'); + return true; + } + })); + store.add(openerService.registerValidator({ + shouldOpen(resource) { + calls.push(`validate:${resource.toString()}`); + return Promise.resolve(false); + } + })); + store.add(openerService.registerOpener({ + async open() { + calls.push('opener'); + return true; + } + })); + + const didOpen = await openerService.open(sourceUri, { openExternal: true, allowContributedOpeners: defaultExternalUriOpenerId }); + + assert.deepStrictEqual({ + didOpen, + calls, + }, { + didOpen: false, + calls: [`validate:${sourceUri.toString()}`], + }); + }); + + test('default external URI opener skips contributed openers', async function () { + const openerService = new OpenerService(editorService, commandService); + const sourceUri = URI.parse('https://source.example.com'); + const calls: string[] = []; + + store.add(openerService.registerExternalOpener({ + async openExternal() { + calls.push('contributed'); + return true; + } + })); + store.add(openerService.registerValidator({ + shouldOpen(resource) { + calls.push(`validate:${resource.toString()}`); + return Promise.resolve(true); + } + })); + store.add(openerService.registerOpener({ + async open() { + calls.push('opener'); + return false; + } + })); + openerService.setDefaultExternalOpener({ + async openExternal(href) { + calls.push(`default:${href}`); + return true; + } + }); + + const didOpen = await openerService.open(sourceUri, { openExternal: true, allowContributedOpeners: defaultExternalUriOpenerId }); + + assert.deepStrictEqual({ + didOpen, + calls, + }, { + didOpen: true, + calls: [ + `validate:${sourceUri.toString()}`, + 'opener', + `default:${sourceUri.toString()}`, + ], + }); + }); + test('external URI fallback preserves strings for validation and opening', async function () { const openerService = new OpenerService(editorService, commandService); const source = 'https://source.example.com/path?value=%2B'; diff --git a/src/vs/platform/opener/common/opener.ts b/src/vs/platform/opener/common/opener.ts index 4d858b75b92d7d..27908c18df45cc 100644 --- a/src/vs/platform/opener/common/opener.ts +++ b/src/vs/platform/opener/common/opener.ts @@ -11,6 +11,8 @@ import { createDecorator } from '../../instantiation/common/instantiation.js'; export const IOpenerService = createDecorator('openerService'); +export const defaultExternalUriOpenerId = 'default'; + export type OpenInternalOptions = { /** diff --git a/src/vs/workbench/contrib/externalUriOpener/common/configuration.ts b/src/vs/workbench/contrib/externalUriOpener/common/configuration.ts index f54ddfe2109a73..3672c4bb33c659 100644 --- a/src/vs/workbench/contrib/externalUriOpener/common/configuration.ts +++ b/src/vs/workbench/contrib/externalUriOpener/common/configuration.ts @@ -7,9 +7,10 @@ import { IConfigurationNode, IConfigurationRegistry, Extensions } from '../../.. import { workbenchConfigurationNodeBase } from '../../../common/configuration.js'; import * as nls from '../../../../nls.js'; import { IJSONSchema } from '../../../../base/common/jsonSchema.js'; +import { defaultExternalUriOpenerId } from '../../../../platform/opener/common/opener.js'; import { Registry } from '../../../../platform/registry/common/platform.js'; -export const defaultExternalUriOpenerId = 'default'; +export { defaultExternalUriOpenerId }; export const externalUriOpenersSettingId = 'workbench.externalUriOpeners';