Skip to content

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

Closed
dmartinochoa wants to merge 1 commit into
review-followupsfrom
review-followups-batch-2
Closed

review-followups-2: nav, shared patterns, logging, test stub, CI check#12
dmartinochoa wants to merge 1 commit into
review-followupsfrom
review-followups-batch-2

Conversation

@dmartinochoa
Copy link
Copy Markdown
Member

@dmartinochoa dmartinochoa commented May 19, 2026

Summary

Second batch of items from the post-v0.1.1 review. Stacked on #11, so this PR's base is `review-followups` and the diff shows only this batch's work. Lands R12, R14, R16, R18, R20 from ROADMAP.md.

What changed

R12 — Go to next / previous finding

  • New src/navigate.ts. `collectFindingLocations` and `pickNextIndex` are pure; the test file pins the sort order, wrap behaviour, and strict-after / strict-before semantics.
  • Commands `pipelineCheck.goToNextFinding` / `pipelineCheck.goToPreviousFinding` registered.
  • Keybindings: Alt+F8 / Shift+Alt+F8 — sibling of VS Code's global `F8` (Next Problem) so the muscle memory carries.

R14 — Single source of truth for trigger patterns

  • New src/providers.ts exports `TRIGGER_PATTERNS` and a derived `TRIGGER_DOCUMENT_SELECTOR`.
  • extension.ts's `documentSelector` reads from there.
  • src/providers.test.ts reads package.json and asserts the activationEvents list stays in lockstep with the patterns (with brace-glob expansion). Catches the most common drift class.
  • `activationEvents` in package.json is the one remaining duplication (manifest contributions can't import code); the test covers it.

R16 — Client-side structured logging

  • New src/log.ts writes `[client] HH:MM:SS.mmm ` into the LanguageClient's outputChannel — same surface as the LSP's `window/logMessage` traffic.
  • `withTiming(label, fn)` brackets a thunk with start / ok / failed lines.
  • extension.ts logs activation: `starting`, `started`, `failed to start — `.

R18 — Shared vscode test stub

  • 93-line `vi.mock("vscode")` factory extracted from findingsView.test.ts into src/testStubs/vscode.ts.
  • Each test file uses the async-import pattern (`vi.mock` factories hoist and can't reference outer scope, so an async import is the only safe way to share).
  • Future test files (`extension.test.ts`, etc.) reuse the same stub.

R20 — Marketplace description length CI gate

  • ci.yml fails the build if `package.json#description` exceeds 145 chars (the marketplace search-results truncation point). Linux-only step.
  • Today's description is 141 chars — the gate keeps future edits honest.

Test plan

  • `npm run lint` clean
  • `npm run compile` clean
  • `npm test` — 75 tests pass (was 53 on review-followups: code fixes, perf, docs links, status bar, CI matrix #11; +11 navigate, +5 providers, +6 log)
  • `npm run smoke` clean
  • Manual under F5: press Alt+F8 in the editor with the sample-workflow fixture open, confirm the cursor jumps between findings; press Shift+Alt+F8 to go back; confirm wrap at the ends.
  • Manual under F5: open the Pipeline-Check output channel, restart the server, confirm `[client]` lines interleave with `[server]` lines around the activation.

Notes

  • The Alt+F8 binding avoids conflict with VS Code's global F8 (Next Problem) so users can still walk all problems with F8 and just pipeline-check findings with Alt+F8.
  • `TriggerSelector` in providers.ts is a structural interface, not the imported `DocumentFilter` from vscode — vscode-languageclient redeclares its own `DocumentFilter` and the two don't unify. Plain objects flow into both APIs unchanged.

🤖 Generated with Claude Code

@dmartinochoa @claude
review-followups-2: nav, shared patterns, logging, test stub, CI check
6a03ac1 
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 <level> 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 — <reason>" 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) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9cac00cf-cc14-49bd-be18-9e72816a5d44

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch review-followups-batch-2

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

This was referenced May 19, 2026
Closed
Merged
@dmartinochoa
Copy link
Copy Markdown
Member Author

dmartinochoa commented May 19, 2026

Rolled into #16 (v0.2.0 mega-PR). Every commit from this branch is in #16's history; closing to consolidate the merge.

@dmartinochoa dmartinochoa deleted the review-followups-batch-2 branch May 19, 2026 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@dmartinochoa