# Review

> Code review against GitLens standards with optional impact completeness audit

- Skill: `gitkraken/review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add gitkraken/review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/gitkraken/review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Security
- Author: gitkraken (https://skillmd.com/u/gitkraken)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/gitkraken/review

---


# /review - Code Review & Impact Audit

Review code changes against GitLens coding standards and verify change completeness.

## Usage

```
/review [target]
```

- No argument: review staged changes (`git diff --cached`)
- `all`: all uncommitted changes
- `file:path`: specific file
- `pr`: current PR changes (`gh pr diff`)
- `impact`: impact completeness audit only (skip code review checklist)
- `full`: both code review + impact audit

## Review Priorities

Review all changes for: correctness, matching user expectations, high performance (including proper caching and deferring of work), well-factored/structured/named code, minimal complexity, proper error handling and logging, comprehensive telemetry, and adherence to best practices. For webview changes, verify they are responsive, accessible, and work with VS Code theming.

### Known Non-Issues (Do Not Flag)

- `debugger;` statements — Webpack's production build automatically strips all `debugger` statements via the minimizer. They are intentional development aids and NOT a shipping concern. Do NOT flag them in reviews.

## Part 1: Code Review Checklist

### TypeScript & Imports

- [ ] No `any` usage (exceptions only for external APIs)
- [ ] Explicit return types for public methods
- [ ] `.js` extension in all imports (ESM requirement)
- [ ] Import order: node built-ins → external → internal → relative
- [ ] `import type` for type-only imports
- [ ] No default exports
- [ ] Path aliases (`@env/`) for environment-specific code

### Naming

- [ ] Classes: PascalCase (no `I` prefix)
- [ ] Methods/Variables: camelCase
- [ ] Constants: camelCase (not SCREAMING_SNAKE_CASE)
- [ ] Files: camelCase.ts
- [ ] Private members: leading underscore allowed

### Error Handling

- [ ] Git errors use `ErrorClass.is(ex, 'reason')` pattern — not `instanceof` + `.message.includes()`
- [ ] Errors logged with context via `Logger.error()`
- [ ] No suppressed/ignored errors
- [ ] Graceful degradation for network/API failures

### Localization

- [ ] Every new user-facing string is a literal `l10n.t()` message with placeholders for data — no concatenation, computed keys, or English plural suffixes
- [ ] Error text shown to a user comes from `getPresentableErrorMessage(ex)`, not `ex.message`
- [ ] Git syntax, refs, paths, command ids and product names are placeholders, not translated text
- [ ] `l10n/bundle.l10n.json` regenerated when messages changed (`pnpm run generate:l10n`)

### Performance

- [ ] Appropriate caching (`@memoize()`, GitCache, PromiseCache)
- [ ] Debounce expensive operations
- [ ] Lazy loading for heavy services

### Webview (if applicable)

- [ ] Lit Elements for reactive UI
- [ ] VS Code theming via CSS custom properties (`--vscode-*`)
- [ ] Keyboard navigation and ARIA attributes
- [ ] `disconnectedCallback()` cleanup for listeners
- [ ] Z-index stays sane — a raw z-index climbing past ~100 is the signal to use a `--gl-z-*` tier token, `isolation: isolate`, or the top layer (`<dialog>.showModal()` / `[popover]`), not a bigger number (shadow-isolated internals keep small raw ordinals)

### Scope & Simplicity

- [ ] No unnecessary new types/abstractions for single-use scenarios
- [ ] Changes scoped to the request — no unrelated drive-by changes
- [ ] Fix addresses root cause (feature not disabled instead of fixed)

### Telemetry & Security

- [ ] Appropriate telemetry events for user actions
- [ ] No sensitive data in logs/telemetry
- [ ] No command injection, XSS, or hardcoded secrets

### Documentation & Skills

- [ ] Check if changes affect patterns documented in `AGENTS.md`, `docs/architecture.md`, or `docs/coding-standards.md` — update if so
- [ ] Check if any skills in `.claude/skills/` reference changed APIs, patterns, or file paths — update if so
- [ ] CHANGELOG entry needed for user-facing changes

## Part 2: Impact Completeness Audit

Run automatically for changes spanning 3+ files, or when `impact` / `full` is specified.

### 1. Identify Changed Symbols

From the diff, extract:

- Modified function signatures and type definitions
- Renamed or removed exports
- Changed error handling or decorator usage

### 2. Find All Consumers

For each modified symbol:

- Search all import statements referencing the modified file
- Search all call sites of modified functions
- Search all implementations/overrides in subclasses
- Check per-operation git providers:
  - `packages/git/src/providers/` (shared domain providers)
  - `packages/git-cli/src/providers/` (CLI implementations)
  - `src/plus/integrations/host/providers/` (host integrations, e.g. `githubGitProvider.ts`)

### 3. Platform Coverage

- [ ] Node.js code path (`src/env/node/`)
- [ ] Browser code path (`src/env/browser/`)
- [ ] Shared code (`src/git/`, `src/system/`, `packages/`)

### 4. Error & UI Impact

- [ ] New error cases handled at all catch sites
- [ ] Error types use `.is()` pattern correctly
- [ ] Webview IPC protocol changes reflected in both host and app
- [ ] Command changes reflected in `contributions.json`

## Instructions

### Code Review Flow

1. Get diff based on target
2. Read full files for context around each change
3. Analyze against Part 1 checklist
4. Categorize: **Critical** (must fix), **Warnings** (should fix), **Suggestions** (nice to have)
5. Include positive feedback for good patterns

### Impact Audit Flow

1. Extract changed symbols from diff
2. Find all consumers of each symbol
3. Check platform and UI coverage
4. Report results:

```markdown
### Consumers Checked

| Modified Symbol | Call Sites | All Updated?                    |
| --------------- | ---------- | ------------------------------- |
| functionA()     | 12         | Yes                             |
| TypeB           | 8          | 7/8 — MISSING: src/views/xyz.ts |

### Verdict

[COMPLETE / INCOMPLETE with remaining work]
```

### Verification

```bash
pnpm exec tsc --noEmit    # Type-check
pnpm run lint              # Lint check
```