Skip to content

Commit 4a05b2c

Browse files
authored
Limit supported client schemes to file and git (#2889)
### Motivation With the new AI developments for editors, the LSP receives a lot of unnecessary requests coming from "ghost" schemes. They are usually some sort of resource URI related to the chat or something happening in the background, but it's not actually relevant for the user. In addition to leading to extra work in the server that is completely unnecessary, it also often confuses users because some of these resources don't send a `textDocument/didOpen` notification before starting to request for other features, which leads to the non existing document error and then people think something is broken, when in reality it's not. ### Implementation Let's limit the schemes we handle to `file` and `git`. ### Automated Tests Updated existing tests.
1 parent d719b86 commit 4a05b2c

File tree

2 files changed

+64
-55
lines changed

2 files changed

+64
-55
lines changed

vscode/src/client.ts

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -150,16 +150,19 @@ function collectClientOptions(
150150

151151
const features: EnabledFeatures = configuration.get("enabledFeatures")!;
152152
const enabledFeatures = Object.keys(features).filter((key) => features[key]);
153+
const supportedSchemes = ["file", "git"];
153154

154155
const fsPath = workspaceFolder.uri.fsPath.replace(/\/$/, "");
155156

156157
// For each workspace, the language client is responsible for handling requests for:
157158
// 1. Files inside of the workspace itself
158159
// 2. Bundled gems
159160
// 3. Default gems
160-
let documentSelector: DocumentSelector = SUPPORTED_LANGUAGE_IDS.map(
161+
let documentSelector: DocumentSelector = SUPPORTED_LANGUAGE_IDS.flatMap(
161162
(language) => {
162-
return { language, pattern: `${fsPath}/**/*` };
163+
return supportedSchemes.map((scheme) => {
164+
return { scheme, language, pattern: `${fsPath}/**/*` };
165+
});
163166
},
164167
);
165168

@@ -175,26 +178,30 @@ function collectClientOptions(
175178
}
176179

177180
ruby.gemPath.forEach((gemPath) => {
178-
documentSelector.push({
179-
language: "ruby",
180-
pattern: `${gemPath}/**/*`,
181-
});
182-
183-
// Because of how default gems are installed, the gemPath location is actually not exactly where the files are
184-
// located. With the regex, we are correcting the default gem path from this (where the files are not located)
185-
// /opt/rubies/3.3.1/lib/ruby/gems/3.3.0
186-
//
187-
// to this (where the files are actually stored)
188-
// /opt/rubies/3.3.1/lib/ruby/3.3.0
189-
//
190-
// Notice that we still need to add the regular path to the selector because some version managers will install gems
191-
// under the non-corrected path
192-
if (/lib\/ruby\/gems\/(?=\d)/.test(gemPath)) {
181+
supportedSchemes.forEach((scheme) => {
193182
documentSelector.push({
183+
scheme,
194184
language: "ruby",
195-
pattern: `${gemPath.replace(/lib\/ruby\/gems\/(?=\d)/, "lib/ruby/")}/**/*`,
185+
pattern: `${gemPath}/**/*`,
196186
});
197-
}
187+
188+
// Because of how default gems are installed, the gemPath location is actually not exactly where the files are
189+
// located. With the regex, we are correcting the default gem path from this (where the files are not located)
190+
// /opt/rubies/3.3.1/lib/ruby/gems/3.3.0
191+
//
192+
// to this (where the files are actually stored)
193+
// /opt/rubies/3.3.1/lib/ruby/3.3.0
194+
//
195+
// Notice that we still need to add the regular path to the selector because some version managers will install
196+
// gems under the non-corrected path
197+
if (/lib\/ruby\/gems\/(?=\d)/.test(gemPath)) {
198+
documentSelector.push({
199+
scheme,
200+
language: "ruby",
201+
pattern: `${gemPath.replace(/lib\/ruby\/gems\/(?=\d)/, "lib/ruby/")}/**/*`,
202+
});
203+
}
204+
});
198205
});
199206

200207
// This is a temporary solution as an escape hatch for users who cannot upgrade the `ruby-lsp` gem to a version that

vscode/src/test/suite/client.test.ts

Lines changed: 38 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ import {
2121
TextEdit,
2222
SelectionRange,
2323
CodeAction,
24-
TextDocumentFilter,
2524
LocationLink,
25+
TextDocumentFilter,
2626
} from "vscode-languageclient/node";
2727
import { after, afterEach, before } from "mocha";
2828

@@ -675,48 +675,50 @@ suite("Client", () => {
675675
}).timeout(20000);
676676

677677
test("document selectors match default gems and bundled gems appropriately", () => {
678-
const [
679-
workspaceRubyFilter,
680-
workspaceERBFilter,
681-
bundledGemsFilter,
682-
defaultPathFilter,
683-
defaultGemsFilter,
684-
] = client.clientOptions.documentSelector!;
685-
686-
assert.strictEqual(
687-
(workspaceRubyFilter as TextDocumentFilter).language!,
688-
"ruby",
689-
);
690-
691-
assert.strictEqual(
692-
(workspaceRubyFilter as TextDocumentFilter).pattern!,
693-
`${workspaceUri.fsPath}/**/*`,
694-
);
678+
const selector = client.clientOptions
679+
.documentSelector! as TextDocumentFilter[];
680+
assert.strictEqual(selector.length, 10);
681+
682+
// We don't care about the order of the document filters, just that they are present. This assertion helper is just
683+
// a convenience to search the registered filters
684+
const assertSelector = (
685+
language: string | undefined,
686+
pattern: RegExp | string,
687+
scheme: string | undefined,
688+
) => {
689+
assert.ok(
690+
selector.find(
691+
(filter: TextDocumentFilter) =>
692+
filter.language === language &&
693+
(typeof pattern === "string"
694+
? pattern === filter.pattern
695+
: pattern.test(filter.pattern!)) &&
696+
filter.scheme === scheme,
697+
),
698+
);
699+
};
695700

696-
assert.strictEqual(
697-
(workspaceERBFilter as TextDocumentFilter).language!,
698-
"erb",
699-
);
701+
assertSelector("ruby", `${workspaceUri.fsPath}/**/*`, "file");
702+
assertSelector("ruby", `${workspaceUri.fsPath}/**/*`, "git");
703+
assertSelector("erb", `${workspaceUri.fsPath}/**/*`, "file");
704+
assertSelector("erb", `${workspaceUri.fsPath}/**/*`, "git");
700705

701-
assert.strictEqual(
702-
(workspaceERBFilter as TextDocumentFilter).pattern!,
703-
`${workspaceUri.fsPath}/**/*`,
706+
assertSelector(
707+
"ruby",
708+
new RegExp(`ruby\\/\\d\\.\\d\\.\\d\\/\\*\\*\\/\\*`),
709+
"file",
704710
);
705-
706-
assert.match(
707-
(bundledGemsFilter as TextDocumentFilter).pattern!,
711+
assertSelector(
712+
"ruby",
708713
new RegExp(`ruby\\/\\d\\.\\d\\.\\d\\/\\*\\*\\/\\*`),
714+
"git",
709715
);
710716

711-
assert.match(
712-
(defaultPathFilter as TextDocumentFilter).pattern!,
713-
/lib\/ruby\/gems\/\d\.\d\.\d\/\*\*\/\*/,
714-
);
717+
assertSelector("ruby", /lib\/ruby\/gems\/\d\.\d\.\d\/\*\*\/\*/, "file");
718+
assertSelector("ruby", /lib\/ruby\/gems\/\d\.\d\.\d\/\*\*\/\*/, "git");
715719

716-
assert.match(
717-
(defaultGemsFilter as TextDocumentFilter).pattern!,
718-
/lib\/ruby\/\d\.\d\.\d\/\*\*\/\*/,
719-
);
720+
assertSelector("ruby", /lib\/ruby\/\d\.\d\.\d\/\*\*\/\*/, "file");
721+
assertSelector("ruby", /lib\/ruby\/\d\.\d\.\d\/\*\*\/\*/, "git");
720722
});
721723

722724
test("requests for non existing documents do not crash the server", async () => {

0 commit comments

Comments
 (0)