review-followups-2: nav, shared patterns, logging, test stub, CI check

.github/workflows/ci.yml

@@ -41,6 +41,15 @@ jobs:
       - name: Lint
         run: npm run lint
 
+      - name: Marketplace description length
+        # The marketplace truncates listing descriptions around 145
+        # characters in search results. Anything longer loses signal
+        # before users click through. The current copy hugs the limit
+        # deliberately — this step keeps future edits honest.
+        if: matrix.os == 'ubuntu-latest'
+        run: |
+          node -e 'const d=require("./package.json").description; if(d.length>145){console.error("description is "+d.length+" chars (max 145):",d);process.exit(1)}'
+
       - name: TypeScript compile
         run: npm run compile
 

package.json

@@ -109,6 +109,28 @@
         "title": "Change Grouping",
         "category": "Pipeline-Check",
         "icon": "$(list-tree)"
+      },
+      {
+        "command": "pipelineCheck.goToNextFinding",
+        "title": "Go to Next Finding",
+        "category": "Pipeline-Check"
+      },
+      {
+        "command": "pipelineCheck.goToPreviousFinding",
+        "title": "Go to Previous Finding",
+        "category": "Pipeline-Check"
+      }
+    ],
+    "keybindings": [
+      {
+        "command": "pipelineCheck.goToNextFinding",
+        "key": "alt+f8",
+        "when": "editorTextFocus"
+      },
+      {
+        "command": "pipelineCheck.goToPreviousFinding",
+        "key": "shift+alt+f8",
+        "when": "editorTextFocus"
       }
     ],
     "menus": {

src/__testStubs__/vscode.ts

@@ -0,0 +1,113 @@
+// Shared `vscode` module stub for the vitest suite. Each test file
+// registers it via:
+//
+//     vi.mock("vscode", async () => {
+//       const { vscodeStub } = await import("./__testStubs__/vscode");
+//       return vscodeStub();
+//     });
+//
+// `vi.mock` factories are hoisted above imports and must self-contain,
+// so the async-import pattern is the only safe way to share. Returning
+// a fresh object per call keeps tests isolated — none of the classes
+// or stubs leak state between files.
+//
+// `getDiagnostics` reads from `globalThis.__stubDiagnostics`, which
+// tests populate via the per-file `setStubDiagnostics` helper they
+// keep close to their fixtures.
+
+export function vscodeStub(): Record<string, unknown> {
+  class ThemeIcon {
+    constructor(
+      public readonly id: string,
+      public readonly color?: ThemeColor,
+    ) {}
+  }
+  class ThemeColor {
+    constructor(public readonly id: string) {}
+  }
+  class EventEmitter<T> {
+    private listeners: Array<(e: T) => void> = [];
+    fire(e: T): void {
+      for (const l of this.listeners) l(e);
+    }
+    get event() {
+      return (listener: (e: T) => void) => {
+        this.listeners.push(listener);
+        return { dispose: () => undefined };
+      };
+    }
+    dispose(): void {
+      this.listeners = [];
+    }
+  }
+  class TreeItem {
+    iconPath?: unknown;
+    description?: string;
+    tooltip?: unknown;
+    command?: unknown;
+    contextValue?: string;
+    constructor(
+      public readonly label: string,
+      public readonly collapsibleState: number,
+    ) {}
+  }
+  class MarkdownString {
+    isTrusted = false;
+    supportThemeIcons = false;
+    constructor(public value: string) {}
+    appendMarkdown(s: string): this {
+      this.value += s;
+      return this;
+    }
+  }
+  const Uri = {
+    parse: (s: string) => {
+      const noScheme = s.replace(/^file:\/\//, "");
+      return {
+        toString: () => s,
+        path: noScheme,
+        fsPath: noScheme,
+      };
+    },
+  };
+  const TreeItemCollapsibleState = { None: 0, Collapsed: 1, Expanded: 2 };
+  const StatusBarAlignment = { Left: 1, Right: 2 };
+
+  return {
+    ThemeIcon,
+    ThemeColor,
+    EventEmitter,
+    TreeItem,
+    MarkdownString,
+    TreeItemCollapsibleState,
+    StatusBarAlignment,
+    Uri,
+    workspace: {
+      asRelativePath: (uri: { fsPath?: string; path?: string }) =>
+        uri.fsPath ?? uri.path ?? "",
+    },
+    languages: {
+      // Two call shapes:
+      //   - `getDiagnostics()` returns every [uri, diagnostic[]] pair
+      //   - `getDiagnostics(uri)` returns just that uri's diagnostics
+      getDiagnostics: (uri?: { toString: () => string }) => {
+        const all =
+          (
+            globalThis as {
+              __stubDiagnostics?: Array<[
+                { toString: () => string },
+                unknown[],
+              ]>;
+            }
+          ).__stubDiagnostics ?? [];
+        if (uri === undefined) return all;
+        const key = uri.toString();
+        const match = all.find(([u]) => u.toString() === key);
+        return match ? match[1] : [];
+      },
+      onDidChangeDiagnostics: () => ({ dispose: () => undefined }),
+    },
+    commands: { executeCommand: () => Promise.resolve() },
+    window: {},
+  };
+}

src/extension.ts

@@ -19,6 +19,9 @@ import {
   TransportKind,
 } from "vscode-languageclient/node";
 import { FindingsTreeProvider, GroupMode } from "./findingsView";
+import * as clientLog from "./log";
+import { goToFinding } from "./navigate";
+import { TRIGGER_DOCUMENT_SELECTOR } from "./providers";
 import { filterByThreshold } from "./severityFilter";
 import { registerStatusBar } from "./statusBar";
 
@@ -95,21 +98,11 @@ function buildClient(): LanguageClient {
   // in the first place — smaller cross-section, no dependency on whether
   // the user has the official GitHub Actions extension installed
   // (which would otherwise hijack the `github-actions-workflow`
-  // language ID for `.github/workflows/*.yml`).
+  // language ID for `.github/workflows/*.yml`). The pattern list itself
+  // lives in providers.ts so the documentSelector, activationEvents,
+  // and the workspace-scan command can't drift apart.
   const clientOptions: LanguageClientOptions = {
-    documentSelector: [
-      { scheme: "file", pattern: "**/.github/workflows/*.{yml,yaml}" },
-      { scheme: "file", pattern: "**/.gitlab-ci.yml" },
-      { scheme: "file", pattern: "**/azure-pipelines.yml" },
-      { scheme: "file", pattern: "**/bitbucket-pipelines.yml" },
-      { scheme: "file", pattern: "**/.circleci/config.yml" },
-      { scheme: "file", pattern: "**/cloudbuild.yaml" },
-      { scheme: "file", pattern: "**/.buildkite/pipeline.yml" },
-      { scheme: "file", pattern: "**/.drone.{yml,yaml}" },
-      { scheme: "file", pattern: "**/Jenkinsfile" },
-      { scheme: "file", pattern: "**/Dockerfile" },
-      { scheme: "file", pattern: "**/Containerfile" },
-    ],
+    documentSelector: [...TRIGGER_DOCUMENT_SELECTOR],
     synchronize: {
       configurationSection: "pipelineCheck",
     },
@@ -153,8 +146,14 @@ async function startClient(): Promise<void> {
   // drop the broken client on failure (the "Open server log" action
   // below still needs to focus it to surface the server's traceback).
   const outputChannel: vscode.OutputChannel = client.outputChannel;
+  // Point the client-side logger at the same channel the LSP server
+  // writes to, so [client] and [server] lines interleave with shared
+  // timestamps — much easier to read when triaging a bug report.
+  clientLog.setLogChannel(outputChannel);
   try {
+    clientLog.info("language server: starting");
     await client.start();
+    clientLog.info("language server: started");
   } catch (err) {
     // The most common cause is `python -m pipeline_check.lsp` failing:
     // either Python is not on PATH or the [lsp] extra is not installed.
@@ -163,6 +162,7 @@ async function startClient(): Promise<void> {
     // notification body. The notification chrome already shows the
     // extension name, so the message body doesn't repeat it.
     const message = err instanceof Error ? err.message : String(err);
+    clientLog.error(`language server: failed to start — ${message}`);
     const choice = await vscode.window.showErrorMessage(
       `Language server failed to start (${message}).`,
       "Copy install command",
@@ -242,6 +242,12 @@ export async function activate(
       "pipelineCheck.findings.changeGrouping",
       () => changeGrouping(findingsProvider),
     ),
+    vscode.commands.registerCommand("pipelineCheck.goToNextFinding", () =>
+      goToFinding("next"),
+    ),
+    vscode.commands.registerCommand("pipelineCheck.goToPreviousFinding", () =>
+      goToFinding("previous"),
+    ),
     vscode.commands.registerCommand("pipelineCheck.restart", async () => {
       await stopClient();
       await startClient();

src/findingsView.test.ts

@@ -1,110 +1,12 @@
 import { describe, it, expect, vi, beforeEach } from "vitest";
 
-// findingsView.ts imports `vscode` at the top, which is supplied by the
-// editor at runtime and isn't installable from npm. We stub just the
-// surface findingsView actually touches: classes it instantiates
-// (`ThemeIcon`, `ThemeColor`, `EventEmitter`, `TreeItem`,
-// `MarkdownString`, `Uri`) plus the static method it calls
-// (`workspace.asRelativePath`, `languages.getDiagnostics`,
-// `languages.onDidChangeDiagnostics`, `commands.executeCommand`).
-//
-// `vi.mock` must run before the SUT is imported. The factory must not
-// reference outer-scope variables (vitest hoists it), so the mutable
-// state (`stubDiagnostics`) lives on `globalThis` and the
-// `getDiagnostics` stub reads from there.
-vi.mock("vscode", () => {
-  class ThemeIcon {
-    constructor(
-      public readonly id: string,
-      public readonly color?: ThemeColor,
-    ) {}
-  }
-  class ThemeColor {
-    constructor(public readonly id: string) {}
-  }
-  class EventEmitter<T> {
-    private listeners: Array<(e: T) => void> = [];
-    fire(e: T): void {
-      for (const l of this.listeners) l(e);
-    }
-    get event() {
-      return (listener: (e: T) => void) => {
-        this.listeners.push(listener);
-        return { dispose: () => undefined };
-      };
-    }
-    dispose(): void {
-      this.listeners = [];
-    }
-  }
-  class TreeItem {
-    iconPath?: unknown;
-    description?: string;
-    tooltip?: unknown;
-    command?: unknown;
-    contextValue?: string;
-    constructor(
-      public readonly label: string,
-      public readonly collapsibleState: number,
-    ) {}
-  }
-  class MarkdownString {
-    isTrusted = false;
-    supportThemeIcons = false;
-    constructor(public value: string) {}
-    appendMarkdown(s: string): this {
-      this.value += s;
-      return this;
-    }
-  }
-  const Uri = {
-    parse: (s: string) => {
-      const noScheme = s.replace(/^file:\/\//, "");
-      return {
-        toString: () => s,
-        path: noScheme,
-        fsPath: noScheme,
-      };
-    },
-  };
-  const TreeItemCollapsibleState = { None: 0, Collapsed: 1, Expanded: 2 };
-  return {
-    ThemeIcon,
-    ThemeColor,
-    EventEmitter,
-    TreeItem,
-    MarkdownString,
-    TreeItemCollapsibleState,
-    Uri,
-    workspace: {
-      asRelativePath: (uri: { fsPath?: string; path?: string }) =>
-        uri.fsPath ?? uri.path ?? "",
-    },
-    languages: {
-      // Two call shapes:
-      //   - `getDiagnostics()` returns every [uri, diagnostic[]] pair
-      //   - `getDiagnostics(uri)` returns just that uri's diagnostics
-      // The provider's batch-skip path uses the second form; the
-      // collection path uses the first.
-      getDiagnostics: (uri?: { toString: () => string }) => {
-        const all =
-          (
-            globalThis as {
-              __stubDiagnostics?: Array<[
-                { toString: () => string },
-                unknown[],
-              ]>;
-            }
-          ).__stubDiagnostics ?? [];
-        if (uri === undefined) return all;
-        const key = uri.toString();
-        const match = all.find(([u]) => u.toString() === key);
-        return match ? match[1] : [];
-      },
-      onDidChangeDiagnostics: () => ({ dispose: () => undefined }),
-    },
-    commands: { executeCommand: () => Promise.resolve() },
-  };
+// The shared vscode stub in src/__testStubs__/vscode.ts covers the
+// surface findingsView reaches into. The async factory below is the
+// only safe way to share it: vi.mock hoists above imports and the
+// factory cannot reference outer-scope bindings synchronously.
+vi.mock("vscode", async () => {
+  const { vscodeStub } = await import("./__testStubs__/vscode");
+  return vscodeStub();
 });
 
 // Import after the mock is registered.