added core-review todo
This commit is contained in:
martin
parent
a87d6da87f
commit
48166e2521
205
skills/code-review-todo/SKILL.md
Normal file
205
skills/code-review-todo/SKILL.md
Normal file
@ -0,0 +1,205 @@
|
||||
---
|
||||
name: code-review-todo
|
||||
description: Creates detailed code-review todo files with problem analysis, impact assessment, best-practice recommendations, and fix examples. Use when documenting code-review findings, performance issues, security concerns, or architectural problems.
|
||||
argument-hint: "[problem description or file path]"
|
||||
allowed-tools: Read, Glob, Grep, Write, Edit, Bash, WebSearch
|
||||
---
|
||||
|
||||
# Code Review Todo Creator
|
||||
|
||||
Creates structured code-review finding files with comprehensive problem descriptions accessible to non-experts, concrete improvement recommendations, and best-practice references.
|
||||
|
||||
## When to Use
|
||||
|
||||
- Documenting a code-review finding (bug, anti-pattern, security issue, performance problem)
|
||||
- Recording an architectural concern with explanation of why it matters
|
||||
- Creating actionable improvement tickets from review observations
|
||||
- Turning vague "this feels wrong" into a clear, teachable todo
|
||||
|
||||
## Workflow
|
||||
|
||||
### Step 1: Gather Context
|
||||
|
||||
1. Read the project's `CLAUDE.md` to understand tech stack, architecture, and conventions
|
||||
2. Parse the user's finding - identify the affected area, files, and nature of the problem
|
||||
3. Use Glob/Grep to locate the actual code exhibiting the problem
|
||||
4. Read the affected files to confirm the issue and understand surrounding context
|
||||
5. If the issue involves a framework feature (Next.js, React, Django, etc.), verify behavior against the project's framework version in `package.json` or `requirements.txt`
|
||||
|
||||
### Step 2: Classify Priority
|
||||
|
||||
Assign a priority level based on impact and urgency:
|
||||
|
||||
| Priority | Tag | Criteria | Examples |
|
||||
|----------|-----|----------|----------|
|
||||
| **Critical** | `P0` | Security vulnerability, data loss risk, broken auth, exposed secrets | XSS, SQL injection, leaked tokens, missing auth guards |
|
||||
| **High** | `P1` | Silent data corruption, unhandled errors causing bad UX, major performance bottleneck | Missing response.ok checks, N+1 queries on hot paths, O(n^2) in render loops |
|
||||
| **Important** | `P2` | Performance degradation, inconsistent patterns, maintainability risk, DRY violations | Missing code splitting, stale cache config, duplicate code paths, inline magic values |
|
||||
| **Medium** | `P3` | Code quality issues with limited blast radius, dead code, debug leftovers | console.log in production, unused components, debug output in DOM |
|
||||
| **Low** | `P4` | Stylistic issues, optional improvements, documentation gaps, nice-to-haves | Missing type annotations, optional stricter patterns, cosmetic inconsistencies |
|
||||
|
||||
### Step 3: Research Best Practice
|
||||
|
||||
Before writing the todo, verify the recommended fix:
|
||||
|
||||
1. **Check project conventions**: Does CLAUDE.md or existing code show the correct pattern? Reference it.
|
||||
2. **Check framework docs**: Is there an official recommendation? Note the framework version.
|
||||
3. **Find reference implementations**: Is the correct pattern already used elsewhere in the codebase? Point to it as a model.
|
||||
4. **Web search if needed**: For framework-specific behavior, verify against current docs (especially for fast-moving frameworks like Next.js, React).
|
||||
|
||||
### Step 4: Choose Filename
|
||||
|
||||
Format: `P[0-4]-[kebab-case-description].md` in the `todos/code-review/` directory.
|
||||
|
||||
Create `todos/code-review/` if it doesn't exist.
|
||||
|
||||
Rules:
|
||||
- Prefix with priority tag: `P0-`, `P1-`, `P2-`, `P3-`, or `P4-`
|
||||
- Use kebab-case after the priority prefix
|
||||
- Be specific: `P1-missing-response-ok-checks` not `P1-error-handling`
|
||||
- Keep filenames under 60 characters (including prefix)
|
||||
|
||||
### Step 5: Write Todo
|
||||
|
||||
Use this template. Write in the project's language (check CLAUDE.md and existing todos for language preference - German or English).
|
||||
|
||||
```markdown
|
||||
# [Concise title describing the problem]
|
||||
|
||||
**Status:** Open
|
||||
**Priority:** [Critical | High | Important | Medium | Low]
|
||||
**Area:** [Module name] - [Sub-area]
|
||||
**Tags:** code-review, [category1], [category2]
|
||||
|
||||
## Beschreibung
|
||||
|
||||
### Was ist das Problem?
|
||||
|
||||
[Explain the problem in plain language. Assume the reader is a competent developer but NOT an expert in this specific framework or pattern. Explain framework-specific concepts when they're central to understanding the issue.]
|
||||
|
||||
[Include a concrete code snippet showing the problematic code - use actual code from the codebase, not pseudocode:]
|
||||
|
||||
```typescript
|
||||
// Aktueller Code in path/to/file.ts (Zeile XX)
|
||||
const data = await fetch(url).then(res => res.json());
|
||||
// ^ Problem: Wenn der Server einen 404 oder 500 zurueckgibt,
|
||||
// wird trotzdem .json() aufgerufen
|
||||
```
|
||||
|
||||
### Warum ist das problematisch?
|
||||
|
||||
[Explain the real-world consequences. Be concrete:]
|
||||
- [What happens to the user?]
|
||||
- [What happens to the data?]
|
||||
- [When does this trigger? How likely is it?]
|
||||
- [What makes debugging difficult?]
|
||||
|
||||
### Wie sollte es richtig gemacht werden?
|
||||
|
||||
[Show the recommended fix with a complete, copy-pasteable code example:]
|
||||
|
||||
```typescript
|
||||
// Empfohlenes Pattern
|
||||
const res = await fetch(url);
|
||||
if (!res.ok) {
|
||||
throw new Error(`API error: ${res.status} ${res.statusText}`);
|
||||
}
|
||||
const data = await res.json();
|
||||
```
|
||||
|
||||
[Explain WHY this is better - connect back to the problem:]
|
||||
- [Benefit 1]
|
||||
- [Benefit 2]
|
||||
|
||||
### Referenz im Projekt
|
||||
|
||||
[Point to existing code in the project that already does it correctly, if available:]
|
||||
|
||||
> Korrekt umgesetzt in `path/to/good-example.ts` (Zeile XX):
|
||||
> ```typescript
|
||||
> // This file already follows the recommended pattern
|
||||
> ```
|
||||
|
||||
[If no project reference exists, link to official documentation or well-known best practices.]
|
||||
|
||||
## Requirements
|
||||
|
||||
1. [Specific, actionable requirement]
|
||||
2. [Specific, actionable requirement]
|
||||
|
||||
## Affected Files
|
||||
|
||||
- `path/to/file.ts` - Zeile XX: [What specifically needs to change]
|
||||
- `path/to/other-file.ts` - Zeile XX: [What specifically needs to change]
|
||||
|
||||
## Best Practice
|
||||
|
||||
[A brief, reusable lesson. Frame it as a general principle that applies beyond this specific finding:]
|
||||
|
||||
> **Regel:** [One-sentence principle, e.g. "Jede fetch()-Antwort muss vor .json() auf res.ok geprueft werden."]
|
||||
|
||||
[Optional: Link to official docs, blog posts, or RFC that support this practice.]
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] [Verifiable criterion]
|
||||
- [ ] [Verifiable criterion]
|
||||
- [ ] `npm run check` laeuft fehlerfrei
|
||||
```
|
||||
|
||||
## Writing Guidelines
|
||||
|
||||
### Problem Description Quality
|
||||
|
||||
The problem description is the most important section. It must:
|
||||
|
||||
1. **Start with the observable symptom**, not the technical root cause
|
||||
- Good: "Wenn ein Nutzer zwischen Seiten navigiert, wird dieselbe API zweimal aufgerufen"
|
||||
- Bad: "QueryClient wird pro Page neu instanziiert"
|
||||
|
||||
2. **Explain framework concepts inline** when they're needed to understand the issue
|
||||
- Good: "React Query cached Daten in einem QueryClient. Wenn jede Page ihren eigenen QueryClient erstellt, kann kein Cache geteilt werden."
|
||||
- Bad: "Redundante QueryClient-Instanzen brechen die Cache-Kohaerenz"
|
||||
|
||||
3. **Show real code, not abstractions** - copy actual lines from the codebase with file paths and line numbers
|
||||
|
||||
4. **Quantify impact where possible**
|
||||
- "~500KB zusaetzlich im initialen Bundle"
|
||||
- "~180 React-Subscriptions statt einer einzigen"
|
||||
- "Bei 30 Spieltagen: 30 x .find() = O(n^2)"
|
||||
|
||||
### Fix Recommendation Quality
|
||||
|
||||
The recommended fix must be:
|
||||
|
||||
1. **Copy-pasteable** - a developer should be able to apply it directly
|
||||
2. **Minimal** - only change what's necessary, don't refactor surroundings
|
||||
3. **Explained** - connect back to why this solves the described problem
|
||||
4. **Verified** - if referencing a framework API, confirm it exists in the project's version
|
||||
|
||||
### Tag Categories
|
||||
|
||||
Use these tag categories after `code-review`:
|
||||
|
||||
| Tag | Use for |
|
||||
|-----|---------|
|
||||
| `security` | Auth, XSS, CSRF, secrets, cookies |
|
||||
| `performance` | Bundle size, render cycles, queries, caching |
|
||||
| `bug` | Incorrect behavior, data corruption, crashes |
|
||||
| `bug-risk` | Code that works now but is fragile/will break |
|
||||
| `architecture` | Structural concerns, patterns, module boundaries |
|
||||
| `cleanup` | Dead code, debug leftovers, naming |
|
||||
| `caching` | React Query, HTTP cache, stale data |
|
||||
| `auth` | Authentication, authorization, tokens, sessions |
|
||||
| `i18n` | Hardcoded strings, locale handling |
|
||||
| `integration` | API layer, Django communication, endpoints |
|
||||
|
||||
## Rules
|
||||
|
||||
1. **Verify before writing** - always read the actual source file. Never create a finding based on assumptions.
|
||||
2. **One finding per file** - if a single review observation reveals multiple independent issues, create separate todos.
|
||||
3. **Reference project code** - always point to the exact file, line number, and code snippet.
|
||||
4. **Find the positive example** - if the correct pattern exists elsewhere in the project, reference it. This makes the fix concrete and shows it's achievable within the project's conventions.
|
||||
5. **Version-aware** - check `package.json` / framework version before claiming something is an anti-pattern. Framework behavior changes between versions (e.g., Next.js 16 renamed middleware.ts to proxy.ts).
|
||||
6. **No false alarms** - if you're not sure the issue is real, investigate further or note the uncertainty explicitly.
|
||||
7. **Match project language** - write in the same language as CLAUDE.md and existing todos (German/English).
|
||||
Loading…
x
Reference in New Issue
Block a user