From 6a03ac191f6003b5d5c5cebe3b8119b81db266fb Mon Sep 17 00:00:00 2001 From: Daniel Martin <56157528+dmartinochoa@users.noreply.github.com> Date: Tue, 19 May 2026 10:48:19 +0200 Subject: [PATCH] review-followups-2: nav, shared patterns, logging, test stub, CI check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on review-followups (#11). Lands the next batch of items from the post-v0.1.1 review: R12, R14, R16, R18, R20. R12 — next/previous finding navigation: - src/navigate.ts: collectFindingLocations enumerates every pipeline-check diagnostic across the workspace, sorted by fsPath then by line. pickNextIndex picks the neighbouring finding relative to the editor cursor, wrapping at both ends. goToFinding pulls the active editor's location, calls pickNextIndex, and reveals the target via showTextDocument. - src/extension.ts: registers pipelineCheck.goToNextFinding and pipelineCheck.goToPreviousFinding. - package.json: declares both commands and binds Alt+F8 / Shift+Alt+F8 (the F8 family is VS Code's "next problem" muscle memory; Alt+F8 stays out of conflict with the global binding). - src/navigate.test.ts: 11 tests pinning the sort order, the wrap behaviour at both ends, the no-cursor case, and the strict-after / strict-before semantics that keep "next" from pinning when the cursor sits exactly on a finding. R14 — single source of truth for trigger patterns: - src/providers.ts: TRIGGER_PATTERNS + TRIGGER_DOCUMENT_SELECTOR. The activationEvents in package.json cannot import this module (VS Code reads the manifest before any code runs), so the events list duplicates the patterns intentionally. - src/providers.test.ts: locks the documentSelector list to the package.json activationEvents — every workspaceContains: event must correspond to a pattern, and vice versa, with brace-glob expansion. Catches drift before it ships. - src/extension.ts: documentSelector now reads from providers.ts. R16 — client-side structured logging: - src/log.ts: appendLine into the LanguageClient's outputChannel with a [client] HH:MM:SS.mmm prefix. info/warn/error helpers and a withTiming wrapper that brackets a thunk with start/ok/failed lines. Silent no-op until setLogChannel is called, so activation-order edge cases don't throw. - src/extension.ts: pointed setLogChannel at outputChannel after the client is constructed; logs "starting" / "started" / "failed to start — " around the LSP boot. - src/log.test.ts: 7 tests covering timestamp formatting, level preservation, the pre-setLogChannel no-op path, and withTiming's success / failure branches. R18 — shared vscode test stub: - src/__testStubs__/vscode.ts: extracted the 93-line vi.mock factory out of findingsView.test.ts. Each test file now reads: vi.mock("vscode", async () => { const { vscodeStub } = await import("./__testStubs__/vscode"); return vscodeStub(); }); vi.mock's hoisting forbids referencing outer-scope bindings, so the async-import pattern is the only way to share. Returns a fresh object per call so tests don't leak state into each other. - src/findingsView.test.ts: collapsed to the shared stub. R20 — marketplace description length CI gate: - .github/workflows/ci.yml: a "Marketplace description length" step (Linux only) fails the build if package.json#description exceeds 145 characters — the marketplace's search-results truncation point. Today's description is 141 chars; the gate keeps future edits honest. Total: 75 tests pass (was 53 on review-followups). Lint, compile, smoke all green. Three-OS CI matrix on every push. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci.yml | 9 ++ package.json | 22 +++++ src/__testStubs__/vscode.ts | 113 ++++++++++++++++++++++ src/extension.ts | 34 ++++--- src/findingsView.test.ts | 112 ++-------------------- src/log.test.ts | 109 +++++++++++++++++++++ src/log.ts | 76 +++++++++++++++ src/navigate.test.ts | 186 ++++++++++++++++++++++++++++++++++++ src/navigate.ts | 143 +++++++++++++++++++++++++++ src/providers.test.ts | 54 +++++++++++ src/providers.ts | 53 ++++++++++ 11 files changed, 792 insertions(+), 119 deletions(-) create mode 100644 src/__testStubs__/vscode.ts create mode 100644 src/log.test.ts create mode 100644 src/log.ts create mode 100644 src/navigate.test.ts create mode 100644 src/navigate.ts create mode 100644 src/providers.test.ts create mode 100644 src/providers.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6af70e3..3d2f9ba 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/package.json b/package.json index 872a987..f633392 100644 --- a/package.json +++ b/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": { diff --git a/src/__testStubs__/vscode.ts b/src/__testStubs__/vscode.ts new file mode 100644 index 0000000..838b62c --- /dev/null +++ b/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 { + class ThemeIcon { + constructor( + public readonly id: string, + public readonly color?: ThemeColor, + ) {} + } + class ThemeColor { + constructor(public readonly id: string) {} + } + class EventEmitter { + 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: {}, + }; +} diff --git a/src/extension.ts b/src/extension.ts index d14cd0b..2cf26d7 100644 --- a/src/extension.ts +++ b/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 { // 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 { // 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(); diff --git a/src/findingsView.test.ts b/src/findingsView.test.ts index a98d150..6fd555d 100644 --- a/src/findingsView.test.ts +++ b/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 { - 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. diff --git a/src/log.test.ts b/src/log.test.ts new file mode 100644 index 0000000..52a24f2 --- /dev/null +++ b/src/log.test.ts @@ -0,0 +1,109 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("vscode", () => ({})); + +import { + formatTimestamp, + info, + warn, + error, + setLogChannel, + withTiming, +} from "./log"; + +// Capture log lines by passing a fake OutputChannel. +function fakeChannel() { + const lines: string[] = []; + return { + lines, + channel: { + appendLine: (s: string) => { + lines.push(s); + }, + // Methods we don't exercise; provided so the type satisfies + // vscode.OutputChannel's minimal shape. + name: "Pipeline-Check", + append: () => undefined, + clear: () => undefined, + show: () => undefined, + hide: () => undefined, + replace: () => undefined, + dispose: () => undefined, + } as unknown as import("vscode").OutputChannel, + }; +} + +beforeEach(() => { + // Reset the module-scope channel so a test that doesn't call + // setLogChannel can verify the no-op path. + setLogChannel( + undefined as unknown as import("vscode").OutputChannel, + ); +}); + +describe("formatTimestamp", () => { + it("zero-pads the components", () => { + const d = new Date(2026, 0, 1, 3, 4, 5, 6); + expect(formatTimestamp(d)).toBe("03:04:05.006"); + }); + + it("uses millisecond precision", () => { + const d = new Date(2026, 0, 1, 23, 59, 59, 999); + expect(formatTimestamp(d)).toBe("23:59:59.999"); + }); +}); + +describe("log levels", () => { + it("info appends a line with the [client] prefix and level", () => { + const { lines, channel } = fakeChannel(); + setLogChannel(channel); + info("hello"); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain("[client]"); + expect(lines[0]).toContain("info"); + expect(lines[0]).toContain("hello"); + }); + + it("warn level is preserved", () => { + const { lines, channel } = fakeChannel(); + setLogChannel(channel); + warn("careful"); + expect(lines[0]).toContain("warn"); + }); + + it("error level is preserved", () => { + const { lines, channel } = fakeChannel(); + setLogChannel(channel); + error("oops"); + expect(lines[0]).toContain("error"); + }); + + it("is a no-op before setLogChannel has been called", () => { + // setLogChannel was reset to undefined in beforeEach. + expect(() => info("nowhere to go")).not.toThrow(); + }); +}); + +describe("withTiming", () => { + it("logs start, ok, and the elapsed milliseconds on success", async () => { + const { lines, channel } = fakeChannel(); + setLogChannel(channel); + await withTiming("test op", async () => undefined); + expect(lines).toHaveLength(2); + expect(lines[0]).toMatch(/test op: start$/); + expect(lines[1]).toMatch(/test op: ok in \d+ms$/); + }); + + it("logs failure and re-throws the original error", async () => { + const { lines, channel } = fakeChannel(); + setLogChannel(channel); + await expect( + withTiming("doomed", async () => { + throw new Error("kaboom"); + }), + ).rejects.toThrow("kaboom"); + expect(lines).toHaveLength(2); + expect(lines[1]).toContain("doomed: failed"); + expect(lines[1]).toContain("kaboom"); + }); +}); diff --git a/src/log.ts b/src/log.ts new file mode 100644 index 0000000..81972d5 --- /dev/null +++ b/src/log.ts @@ -0,0 +1,76 @@ +// Client-side structured logging that lands in the same +// "Pipeline-Check" output channel as the LSP server's +// `window/logMessage` traffic, distinguished by a `[client]` prefix. +// +// The point is to leave breadcrumbs when a user reports a bug: +// without these lines we have no way to tell whether the command +// fired, how long the work took, or where it failed. The output +// channel is the right home — users can already focus it via +// `Pipeline-Check: Show language server output`, and it's the +// natural place to look. + +import * as vscode from "vscode"; + +let channel: vscode.OutputChannel | undefined; + +/** + * Set the output channel logs are written to. Called once from + * activate() after the LanguageClient has constructed its channel, + * so client logs and server logs share the same surface. + */ +export function setLogChannel(c: vscode.OutputChannel): void { + channel = c; +} + +/** + * Append a single line, prefixed `[client] HH:MM:SS.mmm `, to + * the configured output channel. Silent no-op until `setLogChannel` + * has been called — keeps activation-order edge cases from throwing. + */ +export function log( + level: "info" | "warn" | "error", + message: string, +): void { + if (!channel) return; + channel.appendLine(`[client] ${timestamp()} ${level.padEnd(5)} ${message}`); +} + +export const info = (msg: string) => log("info", msg); +export const warn = (msg: string) => log("warn", msg); +export const error = (msg: string) => log("error", msg); + +/** + * Wraps a thunk so its start, end, and duration land in the log. + * Useful for commands the user fires — `command ran for 1.3s` + * is exactly the kind of breadcrumb that turns a vague bug report + * into an actionable one. + */ +export async function withTiming( + label: string, + fn: () => Promise, +): Promise { + const started = Date.now(); + info(`${label}: start`); + try { + const result = await fn(); + info(`${label}: ok in ${Date.now() - started}ms`); + return result; + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + error(`${label}: failed in ${Date.now() - started}ms — ${msg}`); + throw err; + } +} + +/** + * Render the current time as `HH:MM:SS.mmm` so log lines sort and + * align on the leading column. Exported for unit testing. + */ +export function formatTimestamp(d: Date): string { + const pad = (n: number, w = 2) => String(n).padStart(w, "0"); + return `${pad(d.getHours())}:${pad(d.getMinutes())}:${pad(d.getSeconds())}.${pad(d.getMilliseconds(), 3)}`; +} + +function timestamp(): string { + return formatTimestamp(new Date()); +} diff --git a/src/navigate.test.ts b/src/navigate.test.ts new file mode 100644 index 0000000..4461d0a --- /dev/null +++ b/src/navigate.test.ts @@ -0,0 +1,186 @@ +import { describe, it, expect, vi } from "vitest"; + +vi.mock("vscode", () => ({ + // navigate.ts touches `vscode.languages` and `vscode.window` in the + // command path, but the pure helpers we test below don't need them. + // A minimal stub keeps the module-level import resolvable. + languages: {}, + window: {}, +})); + +import { collectFindingLocations, pickNextIndex, type Direction } from "./navigate"; + +// Helpers ---------------------------------------------------------------- + +const uri = (path: string) => + ({ + fsPath: path, + toString: () => `file://${path}`, + }) as unknown as import("vscode").Uri; + +const range = (line: number, ch = 0) => + ({ + start: { line, character: ch }, + end: { line, character: ch }, + }) as unknown as import("vscode").Range; + +const pos = (line: number, ch = 0) => + ({ line, character: ch }) as import("vscode").Position; + +const diag = (line: number, source = "pipeline-check") => ({ + source, + message: "", + range: range(line), + severity: 0, +}); + +const findings = (...rows: Array<[string, number]>) => + rows.map(([file, line]) => ({ + uri: uri(file), + range: range(line), + })); + +// collectFindingLocations ----------------------------------------------- + +describe("collectFindingLocations", () => { + it("ignores diagnostics whose source is not pipeline-check", () => { + const iter: Array<[import("vscode").Uri, import("vscode").Diagnostic[]]> = [ + [ + uri("/a/ci.yml"), + [ + diag(2, "eslint") as unknown as import("vscode").Diagnostic, + diag(5) as unknown as import("vscode").Diagnostic, + ], + ], + ]; + const out = collectFindingLocations(iter); + expect(out).toHaveLength(1); + expect(out[0].range.start.line).toBe(5); + }); + + it("sorts cross-file by fsPath then by line", () => { + const iter: Array<[import("vscode").Uri, import("vscode").Diagnostic[]]> = [ + [ + uri("/z/last.yml"), + [ + diag(0) as unknown as import("vscode").Diagnostic, + diag(2) as unknown as import("vscode").Diagnostic, + ], + ], + [ + uri("/a/first.yml"), + [ + diag(9) as unknown as import("vscode").Diagnostic, + diag(1) as unknown as import("vscode").Diagnostic, + ], + ], + ]; + const out = collectFindingLocations(iter); + expect(out.map((f) => [f.uri.fsPath, f.range.start.line])).toEqual([ + ["/a/first.yml", 1], + ["/a/first.yml", 9], + ["/z/last.yml", 0], + ["/z/last.yml", 2], + ]); + }); +}); + +// pickNextIndex --------------------------------------------------------- + +describe("pickNextIndex", () => { + const list = findings( + ["/a/x.yml", 5], + ["/a/x.yml", 15], + ["/b/y.yml", 0], + ); + + it("returns -1 when there are no findings", () => { + expect(pickNextIndex([], undefined, "next")).toBe(-1); + expect(pickNextIndex([], pos(0) as never, "next" satisfies Direction)).toBe( + -1, + ); + }); + + it("next: with no cursor returns the first finding", () => { + expect( + pickNextIndex( + list, + undefined as unknown as { uri: import("vscode").Uri; position: import("vscode").Position }, + "next", + ), + ).toBe(0); + }); + + it("previous: with no cursor returns the last finding", () => { + expect( + pickNextIndex( + list, + undefined as unknown as { uri: import("vscode").Uri; position: import("vscode").Position }, + "previous", + ), + ).toBe(2); + }); + + it("next: cursor before all findings → first", () => { + expect( + pickNextIndex(list, { uri: uri("/a/x.yml"), position: pos(0) }, "next"), + ).toBe(0); + }); + + it("next: cursor on a finding → the one after", () => { + expect( + pickNextIndex(list, { uri: uri("/a/x.yml"), position: pos(5) }, "next"), + ).toBe(1); + }); + + it("next: cursor at end → wraps to first", () => { + expect( + pickNextIndex( + list, + { uri: uri("/c/zzz.yml"), position: pos(99) }, + "next", + ), + ).toBe(0); + }); + + it("previous: cursor on a finding → the one before", () => { + expect( + pickNextIndex( + list, + { uri: uri("/a/x.yml"), position: pos(15) }, + "previous", + ), + ).toBe(0); + }); + + it("previous: cursor at start → wraps to last", () => { + expect( + pickNextIndex( + list, + { uri: uri("/0/nothing.yml"), position: pos(0) }, + "previous", + ), + ).toBe(2); + }); + + it("next: cursor in a file that sorts between the finding files lands on the next finding's file", () => { + // Cursor in /a/y_after.yml — sorts after /a/x.yml (same dir, + // 'y' > 'x') and before /b/y.yml. Next finding is index 2. + expect( + pickNextIndex( + list, + { uri: uri("/a/y_after.yml"), position: pos(0) }, + "next", + ), + ).toBe(2); + }); + + it("strict comparison: cursor on the SAME column as a finding still advances", () => { + // The finding sits at line 5 char 0. Cursor at line 5 char 0 should + // still move us past it on `next`, not pin. + const single = findings(["/a/x.yml", 5]); + expect( + pickNextIndex(single, { uri: uri("/a/x.yml"), position: pos(5, 0) }, "next"), + ).toBe(0); // wraps because single element and not strictly-after + }); +}); diff --git a/src/navigate.ts b/src/navigate.ts new file mode 100644 index 0000000..e2d9aad --- /dev/null +++ b/src/navigate.ts @@ -0,0 +1,143 @@ +// "Go to next / previous finding" navigation. Walks the workspace's +// pipeline-check diagnostics in a deterministic order (uri.fsPath +// ascending, then line ascending) and moves the cursor to the +// neighbouring one relative to wherever it sits now. +// +// The order matches the Findings tree's file-mode sort so jumping +// from the editor and clicking through the tree produce the same +// sequence — no surprise re-orderings between surfaces. + +import * as vscode from "vscode"; + +const DIAGNOSTIC_SOURCE = "pipeline-check"; + +interface Location { + readonly uri: vscode.Uri; + readonly range: vscode.Range; +} + +/** + * Enumerates every pipeline-check diagnostic in the workspace as a + * flat list of `(uri, range)` pairs, sorted by file path then by + * starting line. Exported for unit testing. + */ +export function collectFindingLocations( + iter: Iterable, +): Location[] { + const out: Location[] = []; + for (const [uri, diags] of iter) { + for (const d of diags) { + if (d.source !== DIAGNOSTIC_SOURCE) continue; + out.push({ uri, range: d.range }); + } + } + out.sort((a, b) => { + const lhs = a.uri.fsPath; + const rhs = b.uri.fsPath; + if (lhs !== rhs) return lhs.localeCompare(rhs); + return a.range.start.line - b.range.start.line; + }); + return out; +} + +export type Direction = "next" | "previous"; + +/** + * Given the sorted findings, the active editor's location, and a + * direction, return the index of the finding to jump to (or -1 when + * the workspace has no findings). + * + * Semantics: + * - `next` from a cursor sitting before any finding → 0 + * - `next` from after the last finding → wraps to 0 + * - `previous` from before all findings → wraps to last + * - `next` from EXACTLY on a finding → the one after + * - `previous` from EXACTLY on a finding → the one before + * + * Wrapping is the convention every navigation command in VS Code + * (Go to Next Problem, search results, etc.) follows; the user + * expects to keep walking the list, not hit an invisible wall. + */ +export function pickNextIndex( + findings: readonly Location[], + current: { uri: vscode.Uri; position: vscode.Position } | undefined, + direction: Direction, +): number { + if (findings.length === 0) return -1; + if (!current) { + return direction === "next" ? 0 : findings.length - 1; + } + + const cursorFs = current.uri.fsPath; + const cursorLine = current.position.line; + const cursorChar = current.position.character; + + if (direction === "next") { + for (let i = 0; i < findings.length; i++) { + const f = findings[i]; + if (isStrictlyAfter(f, cursorFs, cursorLine, cursorChar)) return i; + } + return 0; // wrap + } else { + for (let i = findings.length - 1; i >= 0; i--) { + const f = findings[i]; + if (isStrictlyBefore(f, cursorFs, cursorLine, cursorChar)) return i; + } + return findings.length - 1; // wrap + } +} + +function isStrictlyAfter( + f: Location, + cursorFs: string, + line: number, + ch: number, +): boolean { + const fFs = f.uri.fsPath; + if (fFs !== cursorFs) return fFs.localeCompare(cursorFs) > 0; + if (f.range.start.line !== line) return f.range.start.line > line; + return f.range.start.character > ch; +} + +function isStrictlyBefore( + f: Location, + cursorFs: string, + line: number, + ch: number, +): boolean { + const fFs = f.uri.fsPath; + if (fFs !== cursorFs) return fFs.localeCompare(cursorFs) < 0; + if (f.range.start.line !== line) return f.range.start.line < line; + return f.range.start.character < ch; +} + +/** + * Move the active editor's cursor to the next or previous finding, + * wrapping at the ends. Surfaces an information toast when the + * workspace has no pipeline-check diagnostics — silent failure on + * a deliberate keybinding press is confusing. + */ +export async function goToFinding(direction: Direction): Promise { + const findings = collectFindingLocations(vscode.languages.getDiagnostics()); + if (findings.length === 0) { + void vscode.window.showInformationMessage( + "Pipeline-Check: no findings to navigate.", + ); + return; + } + + const editor = vscode.window.activeTextEditor; + const current = editor + ? { uri: editor.document.uri, position: editor.selection.active } + : undefined; + + const idx = pickNextIndex(findings, current, direction); + if (idx < 0) return; + const target = findings[idx]; + + await vscode.window.showTextDocument(target.uri, { + selection: target.range, + preserveFocus: false, + preview: false, + }); +} diff --git a/src/providers.test.ts b/src/providers.test.ts new file mode 100644 index 0000000..8bcda5e --- /dev/null +++ b/src/providers.test.ts @@ -0,0 +1,54 @@ +import { describe, it, expect, vi } from "vitest"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; + +vi.mock("vscode", () => ({})); + +import { TRIGGER_PATTERNS, TRIGGER_DOCUMENT_SELECTOR } from "./providers"; + +describe("TRIGGER_PATTERNS", () => { + it("derives a `file`-scoped DocumentFilter for each pattern", () => { + expect(TRIGGER_DOCUMENT_SELECTOR).toHaveLength(TRIGGER_PATTERNS.length); + for (const f of TRIGGER_DOCUMENT_SELECTOR) { + expect(f.scheme).toBe("file"); + expect(typeof f.pattern).toBe("string"); + } + }); + + it("stays in sync with package.json#activationEvents", () => { + // The manifest cannot import this module (VS Code reads it before + // any code runs), so the activationEvents list duplicates these + // patterns. The test below catches the drift before it ships: + // every TRIGGER_PATTERNS entry must be reachable from at least + // one `workspaceContains:` event, and every `workspaceContains:` + // event must correspond to a pattern. + const pkg = JSON.parse( + readFileSync(resolve(__dirname, "..", "package.json"), "utf8"), + ) as { activationEvents: string[] }; + + const wsContains = pkg.activationEvents + .filter((e) => e.startsWith("workspaceContains:")) + .map((e) => e.slice("workspaceContains:".length)); + + // Brace-globs collapse to one DocumentFilter pattern but expand + // into multiple activationEvents (one per branch). Expand the + // TRIGGER_PATTERNS list the same way so the comparison is apples + // to apples. + const expanded = TRIGGER_PATTERNS.flatMap(expandBraces).sort(); + const events = [...wsContains].sort(); + expect(events).toEqual(expanded); + }); +}); + +/** + * Expand a single-brace pattern like `**\/foo.{yml,yaml}` into + * `["**\/foo.yml", "**\/foo.yaml"]`. Doesn't handle nested braces — + * good enough for our patterns and trivial to extend if a future + * pattern needs it. + */ +function expandBraces(pattern: string): string[] { + const match = /^(.*)\{([^{}]+)\}(.*)$/.exec(pattern); + if (!match) return [pattern]; + const [, head, body, tail] = match; + return body.split(",").map((alt) => `${head}${alt}${tail}`); +} diff --git a/src/providers.ts b/src/providers.ts new file mode 100644 index 0000000..8060ffb --- /dev/null +++ b/src/providers.ts @@ -0,0 +1,53 @@ +// Single source of truth for which files Pipeline-Check cares about. +// The same list is referenced from three surfaces that used to keep +// their own copies and drift apart: +// +// 1. `documentSelector` in extension.ts — tells the LSP which +// documents to scan when they open in the editor. +// 2. `activationEvents` in package.json — tells VS Code which +// workspace files should activate the extension. +// 3. (post-merge of scan-workspace) `SCAN_PATTERNS` for the +// workspace-wide scan command. +// +// Keeping them in lockstep is mechanical, so this module exports both +// the underlying pattern strings and the VS Code-shaped `DocumentFilter` +// records derived from them. A new provider goes here once; callers +// stay in sync automatically. The package.json `activationEvents` +// remains the only duplication — manifest contributions cannot be +// generated at runtime, so a comment in this file points at the +// strings that must be updated there too. + +// A structural shape rather than `vscode.DocumentFilter` — +// `vscode-languageclient` redeclares `DocumentFilter` and the two +// types don't unify. Plain objects flow into both APIs unchanged. +export interface TriggerSelector { + readonly scheme: "file"; + readonly pattern: string; +} + +/** + * Glob patterns matching every file the upstream `pipeline_check` + * rule registry knows how to analyse. Order is not load-bearing. + */ +export const TRIGGER_PATTERNS: readonly string[] = [ + "**/.github/workflows/*.{yml,yaml}", + "**/.gitlab-ci.yml", + "**/azure-pipelines.yml", + "**/bitbucket-pipelines.yml", + "**/.circleci/config.yml", + "**/cloudbuild.yaml", + "**/.buildkite/pipeline.yml", + "**/.drone.{yml,yaml}", + "**/Jenkinsfile", + "**/Dockerfile", + "**/Containerfile", +]; + +/** + * Document-selector form of `TRIGGER_PATTERNS`, suitable for the + * `LanguageClientOptions.documentSelector`. Each entry restricts the + * filter to the `file` scheme so untitled or memory-backed buffers + * never reach the server. + */ +export const TRIGGER_DOCUMENT_SELECTOR: readonly TriggerSelector[] = + TRIGGER_PATTERNS.map((pattern) => ({ scheme: "file" as const, pattern }));