review-followups: code fixes, perf, docs links, status bar, CI matrix

.github/workflows/ci.yml

@@ -17,7 +17,17 @@ permissions:
 
 jobs:
   check:
-    runs-on: ubuntu-latest
+    # The extension is cross-platform; the LSP child-process spawn
+    # path, the bundle loader, and several file-path helpers are all
+    # Windows-sensitive. Running the gate on three OSes catches the
+    # LF/CRLF and path-separator bugs that single-OS CI silently
+    # ignores. The matrix shares the same step list — only the vsix
+    # upload and the network-bound npm audit are pinned to Linux.
+    strategy:
+      fail-fast: false
+      matrix:
+        os: [ubuntu-latest, windows-latest, macos-latest]
+    runs-on: ${{ matrix.os }}
     steps:
       - uses: actions/checkout@v6
 
@@ -45,16 +55,19 @@ jobs:
         run: npm run smoke
 
       - name: npm audit (prod deps, high+)
+        # Network-bound and platform-independent; one run is enough.
+        if: matrix.os == 'ubuntu-latest'
         run: npm audit --omit=dev --audit-level=high
 
       - name: Verify vsix packs cleanly
         # vsce is a pinned devDependency (see package.json) — Dependabot
         # bumps it via the npm config. `npm ci` has already installed it.
-        run: |
-          npx vsce package --out pipeline-check.vsix
-          ls -lh pipeline-check.vsix
+        run: npx vsce package --out pipeline-check.vsix
 
       - uses: actions/upload-artifact@v7
+        # Single artefact upload from the Linux job; identical-name
+        # uploads from the matrix would collide.
+        if: matrix.os == 'ubuntu-latest'
         with:
           name: vsix
           path: pipeline-check.vsix

ROADMAP.md

@@ -391,3 +391,105 @@ on their own.
       The 50px of vertical space we'd save up front is small
       relative to the structural cost of having to add the header
       back the first time we want a second view.
+
+---
+
+## Review pass (2026-05-19) — improvements from in-depth code review
+
+The findings below came out of a holistic review of the codebase after
+v0.1.1 shipped. Categories cluster related work into reviewable PRs.
+
+### Code-level fixes (cheap wins)
+
+- [ ] **R1** Reorder the `filterByThreshold` import in
+      [src/extension.ts:72](src/extension.ts#L72) up to the rest of the
+      import block. Currently sits after a function declaration.
+- [ ] **R2** "Restart language server" toast at
+      [src/extension.ts:223](src/extension.ts#L223) fires even when
+      `startClient()` failed. Guard with `if (client)`.
+- [ ] **R3** Add a timeout to `stopClient()`
+      ([src/extension.ts:185-192](src/extension.ts#L185-L192)) —
+      a deadlocked LSP child holds the deactivate path indefinitely.
+- [ ] **R4** `groupByFile` does `key = f.uri.toString()` then
+      `vscode.Uri.parse(key)` to get the Uri back
+      ([src/findingsView.ts:357-385](src/findingsView.ts#L357-L385)).
+      Use a `Map<vscode.Uri, Finding[]>` keyed on the Uri directly.
+- [ ] **R5** `compareByLocation`
+      ([src/findingsView.ts:413](src/findingsView.ts#L413)) sorts on
+      `uri.toString()` which includes the `file://` scheme prefix.
+      Sort on `fsPath` instead.
+
+### Performance
+
+- [ ] **R6** Cache `collectFindings()` per refresh. Currently called
+      twice per refresh (`buildRoot` + `updateBadge`); each walks every
+      diagnostic in the workspace.
+- [ ] **R7** Filter `onDidChangeDiagnostics` by the event's `uris`
+      payload — skip the refresh when none of the changed URIs carry a
+      pipeline-check diagnostic.
+
+### UX gaps
+
+- [ ] **R8** Wire `Diagnostic.code.target` into the leaf tooltip. The
+      server publishes the rule's docs URL; we read `code.value` and
+      throw the URL away. Add a docs link at the bottom of the leaf
+      tooltip (set `isTrusted = true` on the `MarkdownString`).
+- [ ] **R9** Status bar item showing critical/high counts. Click
+      reveals the Findings panel.
+- [ ] **R10** Rename `pipelineCheck.findings.refresh` — re-renders
+      from current diagnostics, doesn't trigger a fresh scan. Once
+      scan-workspace lands, have refresh call `scanWorkspace()`.
+- [ ] **R11** `CodeAction` provider for suppression comments.
+      Right-click a finding → "Suppress GHA-001 for this line" that
+      inserts the CLI's suppression syntax.
+- [ ] **R12** "Go to next/previous finding" commands with keybindings.
+- [ ] **R13** Set `Diagnostic.tags` for `Deprecated` /
+      `Unnecessary` where the rule indicates it. Server-side change.
+
+### Architecture
+
+- [ ] **R14** Extract the candidate-file pattern list from
+      `activationEvents`, `documentSelector`, and (post-merge)
+      `SCAN_PATTERNS` into a single shared TS module. Today the list
+      lives in three places.
+- [ ] **R15** Add `onCommand:pipelineCheck.scanWorkspace` to
+      `activationEvents` so opening an isolated file from outside the
+      workspace can still trigger activation.
+- [ ] **R16** Client-side structured logging into the output channel
+      with a `[client]` prefix. Breadcrumbs for bug reports.
+
+### Testing
+
+- [ ] **R17** `@vscode/test-electron` integration tests covering real
+      LSP publish and tree render.
+- [ ] **R18** Extract the `vi.mock("vscode", ...)` factory into a
+      shared `src/__testStubs__/vscode.ts`.
+
+### Marketplace
+
+- [ ] **R19** **Ship the screenshots** the HTML comment at
+      [README.md:13-25](README.md#L13-L25) has been waiting for since
+      v0.1.0. Highest-leverage marketplace conversion improvement.
+- [ ] **R20** CI check that `package.json#description` stays ≤ 145
+      characters (the marketplace-search truncation limit).
+
+### CI / release
+
+- [ ] **R21** Test matrix:
+      `[ubuntu-latest, windows-latest, macos-latest]`. The
+      `LF → CRLF` warnings on every git operation hint at the bugs.
+- [ ] **R22** Finish the eslint flat-config migration so the
+      drift between eslint v8 and TS 6 / esbuild 0.28 stops widening.
+- [ ] **R23** Resolve the CodeQL default-setup conflict — disable
+      default setup or delete codeql.yml. Don't ship with a
+      permanently-red required check.
+- [ ] **R24** `vsce publish --pre-release` channel for the next
+      minor bump.
+
+### Strategic
+
+- [ ] **R25** Per-provider toggles in settings.
+- [ ] **R26** Inline `CodeLens` summarising findings per file.
+- [ ] **R27** Workspace-level config file shared with the CLI.
+- [ ] **R28** Opt-in telemetry (`vscode.env.isTelemetryEnabled`).
+- [ ] **R29** Scan-on-save mode.

src/extension.ts

@@ -19,6 +19,8 @@ import {
   TransportKind,
 } from "vscode-languageclient/node";
 import { FindingsTreeProvider, GroupMode } from "./findingsView";
+import { filterByThreshold } from "./severityFilter";
+import { registerStatusBar } from "./statusBar";
 
 // Group-mode options offered by the Findings panel's "Change
 // Grouping" button. Labels are user-facing; descriptions are the
@@ -69,8 +71,6 @@ async function changeGrouping(
     provider.setGroupMode(choice.mode);
   }
 }
-import { filterByThreshold } from "./severityFilter";
-
 const LANGUAGE_ID = "pipelineCheck";
 const LANGUAGE_NAME = "Pipeline-Check";
 const OUTPUT_CHANNEL = "Pipeline-Check";
@@ -182,13 +182,34 @@ async function startClient(): Promise<void> {
   }
 }
 
+// Hard ceiling on how long deactivate / restart waits for the LSP
+// child to shut down cleanly. A deadlocked server would otherwise
+// hold the deactivate path indefinitely and VS Code reports "Window
+// not responding".
+const STOP_TIMEOUT_MS = 2000;
+
 async function stopClient(): Promise<void> {
   if (!client) {
     return;
   }
   const local = client;
   client = undefined;
-  await local.stop();
+  let timer: NodeJS.Timeout | undefined;
+  try {
+    await Promise.race([
+      local.stop(),
+      new Promise<void>((resolve) => {
+        timer = setTimeout(resolve, STOP_TIMEOUT_MS);
+      }),
+    ]);
+  } finally {
+    if (timer) {
+      clearTimeout(timer);
+    }
+    // If stop() didn't win the race the client is stranded; dispose
+    // explicitly so its subscriptions don't outlive us.
+    local.dispose?.();
+  }
 }
 
 export async function activate(
@@ -208,6 +229,10 @@ export async function activate(
   // badge. Handing the view back closes the loop and triggers an
   // initial badge update.
   findingsProvider.setTreeView(findingsView);
+  // Status bar item lives at the bottom-left and shows the per-
+  // severity tally. Click reveals the Findings panel. registerStatusBar
+  // pushes the item onto context.subscriptions internally.
+  registerStatusBar(context);
   context.subscriptions.push(
     findingsView,
     vscode.commands.registerCommand("pipelineCheck.findings.refresh", () =>
@@ -220,7 +245,12 @@ export async function activate(
     vscode.commands.registerCommand("pipelineCheck.restart", async () => {
       await stopClient();
       await startClient();
-      vscode.window.showInformationMessage("Language server restarted.");
+      // Only confirm success when startClient left a live client behind.
+      // If start failed it surfaced its own error toast; we'd otherwise
+      // show "failed to start" and "restarted" at the same time.
+      if (client) {
+        vscode.window.showInformationMessage("Language server restarted.");
+      }
     }),
     vscode.commands.registerCommand("pipelineCheck.showLog", () => {
       if (client?.outputChannel) {

src/findingsView.test.ts

@@ -49,7 +49,13 @@ vi.mock("vscode", () => {
     ) {}
   }
   class MarkdownString {
-    constructor(public readonly value: string) {}
+    isTrusted = false;
+    supportThemeIcons = false;
+    constructor(public value: string) {}
+    appendMarkdown(s: string): this {
+      this.value += s;
+      return this;
+    }
   }
   const Uri = {
     parse: (s: string) => {
@@ -75,9 +81,26 @@ vi.mock("vscode", () => {
         uri.fsPath ?? uri.path ?? "",
     },
     languages: {
-      getDiagnostics: () =>
-        (globalThis as { __stubDiagnostics?: unknown[] }).__stubDiagnostics ??
-        [],
+      // 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() },
@@ -99,6 +122,7 @@ type FakeFinding = {
   rule: string;
   severity?: string;
   line?: number;
+  docsUrl?: string;
 };
 
 function setStubDiagnostics(findings: FakeFinding[]): void {
@@ -109,7 +133,10 @@ function setStubDiagnostics(findings: FakeFinding[]): void {
     arr.push({
       source: "pipeline-check",
       message: `${f.rule} title\n\nThe long description.\n\nFix: do X.`,
-      code: { value: f.rule, target: { toString: () => "" } },
+      code: {
+        value: f.rule,
+        target: { toString: () => f.docsUrl ?? "" },
+      },
       range: {
         start: { line: f.line ?? 0, character: 0 },
         end: { line: f.line ?? 0, character: 0 },
@@ -481,3 +508,81 @@ describe("FindingsTreeProvider — group mode behaviour", () => {
     expect(p.getGroupMode()).toBe("file");
   });
 });
+
+describe("FindingsTreeProvider — rule docs link in tooltip", () => {
+  // When the server publishes ``Diagnostic.code.target`` (the rule's
+  // documentation URL), the leaf tooltip should carry a clickable
+  // "Read more" link below the message body. When the URL is absent
+  // or empty, the tooltip is just the message. The link target is
+  // exactly what the server published — we don't synthesise URLs.
+
+  it("appends a docs link when the server publishes one", () => {
+    setStubDiagnostics([
+      {
+        file: "a.yml",
+        rule: "GHA-001",
+        severity: "HIGH",
+        docsUrl: "https://example.com/rules/gha-001",
+      },
+    ]);
+    const p = new FindingsTreeProvider(ctx);
+    p.setGroupMode("severity");
+    const roots = p.getChildren();
+    const leaves = p.getChildren(roots[0]);
+    const item = p.getTreeItem(leaves[0]);
+    const tip = item.tooltip as { value: string; isTrusted: boolean };
+    expect(tip.value).toContain("GHA-001 title");
+    expect(tip.value).toContain(
+      "[$(book) GHA-001 documentation](https://example.com/rules/gha-001)",
+    );
+    expect(tip.isTrusted).toBe(true);
+  });
+
+  it("leaves the tooltip clean when the server publishes no URL", () => {
+    setStubDiagnostics([
+      {
+        file: "a.yml",
+        rule: "GHA-001",
+        severity: "HIGH",
+        // no docsUrl
+      },
+    ]);
+    const p = new FindingsTreeProvider(ctx);
+    p.setGroupMode("severity");
+    const roots = p.getChildren();
+    const leaves = p.getChildren(roots[0]);
+    const item = p.getTreeItem(leaves[0]);
+    const tip = item.tooltip as { value: string };
+    expect(tip.value).toContain("GHA-001 title");
+    expect(tip.value).not.toContain("documentation");
+  });
+});
+
+describe("FindingsTreeProvider — findings cache invalidation", () => {
+  // refresh() drops the cached findings list so the next render sees
+  // any new diagnostic publishes. Without invalidation, a freshly
+  // published finding wouldn't appear in the tree until the user
+  // toggled the group mode or restarted VS Code. The test exercises
+  // the path end-to-end: render once, swap the stub data, refresh,
+  // render again.
+
+  it("refresh() picks up newly-published diagnostics", () => {
+    setStubDiagnostics([
+      { file: "a.yml", rule: "GHA-001", severity: "HIGH" },
+    ]);
+    const p = new FindingsTreeProvider(ctx);
+    p.setGroupMode("severity");
+    expect(p.getChildren()).toHaveLength(1);
+
+    setStubDiagnostics([
+      { file: "a.yml", rule: "GHA-001", severity: "HIGH" },
+      { file: "b.yml", rule: "GHA-002", severity: "CRITICAL" },
+    ]);
+    // Without refresh() the second publish would be invisible.
+    p.refresh();
+    const roots = p.getChildren();
+    expect(roots).toHaveLength(2);
+    // CRITICAL is leftmost in the sort.
+    expect(roots[0].kind === "group" && roots[0].label).toBe("CRITICAL");
+  });
+});