Fix path completions and eliding in path rendering

Author: krassowski

packages/jupyterlab-lsp/src/features/completion/model.ts

@@ -62,19 +62,24 @@ export class GenericCompleterModel<
   setCompletionItems(newValue: T[]) {
     super.setCompletionItems(newValue);
 
-    if (this.settings.preFilterMatches && this.current && this.cursor) {
+    if (this.current && this.cursor) {
       // set initial query to pre-filter items; in future we should use:
       // https://github.com/jupyterlab/jupyterlab/issues/9763#issuecomment-1001603348
       const { start, end } = this.cursor;
       let query = this.current.text.substring(start, end).trim();
+      let trimmedQuotes = false;
       // special case for "Completes Paths In Strings" test case
       if (query.startsWith('"') || query.startsWith("'")) {
         query = query.substring(1);
+        trimmedQuotes = true;
       }
       if (query.endsWith('"') || query.endsWith("'")) {
         query = query.substring(0, -1);
+        trimmedQuotes = true;
+      }
+      if (this.settings.preFilterMatches || trimmedQuotes) {
+        this.query = query;
       }
-      this.query = query;
     }
   }
 
@@ -99,6 +104,20 @@ export class GenericCompleterModel<
     return index > -1 ? item.label.substring(0, index) : item.label;
   }
 
+  createPatch(patch: string) {
+    if (this.subsetMatch) {
+      // Prevent insertion code path when auto-populating subset on tab, to avoid problems with
+      // prefix which is a subset of token incorrectly replacing a string with file system path.
+      // - Q: Which code path is being blocked?
+      //   A: The code path (b) discussed in https://github.com/jupyterlab/jupyterlab/issues/15130.
+      // - Q: Why are we short- circuiting here?
+      //   A: we want to prevent `onCompletionSelected()` from proceeding with text insertion,
+      //      but direct extension of Completer handler is difficult.
+      return undefined;
+    }
+    return super.createPatch(patch);
+  }
+
   private _sortAndFilter(query: string, items: T[]): T[] {
     let results: ICompletionMatch<T>[] = [];
 

packages/jupyterlab-lsp/src/features/completion/provider.ts

@@ -307,6 +307,13 @@ export class CompletionProvider implements ICompletionProvider<CompletionItem> {
     return true;
   }
 }
+
+function stripQuotes(path: string): string {
+  return path.slice(
+    path.startsWith("'") || path.startsWith('"') ? 1 : 0,
+    path.endsWith("'") || path.endsWith('"') ? -1 : path.length
+  );
+}
 export function transformLSPCompletions<T>(
   token: CodeEditor.IToken,
   positionInToken: number,
@@ -317,6 +324,7 @@ export function transformLSPCompletions<T>(
   let prefix = token.value.slice(0, positionInToken + 1);
   let allNonPrefixed = true;
   let items: T[] = [];
+  let prefixLength = prefix.length;
   lspCompletionItems.forEach(match => {
     let kind = match.kind ? CompletionItemKind[match.kind] : '';
 
@@ -338,22 +346,27 @@ export function transformLSPCompletions<T>(
       }
     }
     // add prefix if needed
-    else if (token.type === 'string' && prefix.includes('/')) {
+    else if (token.type === 'String' && prefix.includes('/')) {
       // special case for path completion in strings, ensuring that:
       //     '/Com<tab> → '/Completion.ipynb
       // when the returned insert text is `Completion.ipynb` (the token here is `'/Com`)
       // developed against pyls and pylsp server, may not work well in other cases
-      const parts = prefix.split('/');
+
+      const parts = stripQuotes(prefix).split('/');
       if (
         text.toLowerCase().startsWith(parts[parts.length - 1].toLowerCase())
       ) {
         let pathPrefix = parts.slice(0, -1).join('/') + '/';
-        match.insertText = pathPrefix + text;
-        // for label removing the prefix quote if present
-        if (pathPrefix.startsWith("'") || pathPrefix.startsWith('"')) {
-          pathPrefix = pathPrefix.substr(1);
-        }
+        const withStartingQuote =
+          (prefix.startsWith("'") || prefix.startsWith('"') ? prefix[0] : '') +
+          pathPrefix +
+          text;
+        match.insertText = withStartingQuote;
+        // for label without quotes
         match.label = pathPrefix + match.label;
+        if (prefix.endsWith("'") || prefix.endsWith('"')) {
+          prefixLength = prefix.length - 1;
+        }
         allNonPrefixed = false;
       }
     }
@@ -369,8 +382,8 @@ export function transformLSPCompletions<T>(
   // completion of dictionaries for Python with jedi-language-server was
   // causing an issue for dic['<tab>'] case; to avoid this let's make
   // sure that prefix.length >= prefix.offset
-  if (allNonPrefixed && prefixOffset > prefix.length) {
-    prefixOffset = prefix.length;
+  if (allNonPrefixed && prefixOffset > prefixLength) {
+    prefixOffset = prefixLength;
   }
 
   let response = {
@@ -384,7 +397,7 @@ export function transformLSPCompletions<T>(
     // text = token.value + text;
     // but it did not work for "from statistics <tab>" and lead to "from statisticsimport" (no space)
     start: token.offset + (allNonPrefixed ? prefixOffset : 0),
-    end: token.offset + prefix.length,
+    end: token.offset + prefixLength,
     items: items,
     source: 'LSP'
   };

packages/jupyterlab-lsp/src/features/completion/renderer.ts

@@ -161,14 +161,14 @@ export class LSPCompletionRenderer
     const originalHTMLLabel = labelElement.childNodes;
     let hasMark = false;
     for (const node of originalHTMLLabel) {
-      if (node.nodeType === Node.ELEMENT_NODE) {
-        const element = node as Element;
+      if (node instanceof HTMLElement) {
+        const element = node as HTMLElement;
         const text = element.textContent;
-        if (element.tagName === 'MARK' && text) {
+        if (element.tagName === 'MARK' && text && text.length > 3) {
           const elidableElement = document.createElement('bdo');
           elidableElement.setAttribute('dir', 'ltr');
           elidableElement.textContent = text;
-          elidableElement.title = text;
+          element.title = text;
           element.replaceChildren(elidableElement);
           element.classList.add('lsp-elide');
           hasMark = true;