Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions .claude/skills/address-pr-review/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
---
name: address-pr-review
description: Use when you have PR review comments to address and want to evaluate each comment's validity before deciding to fix, reply, or skip
---

# Address PR Review Comments

## Overview

Interactive workflow: analyze PR review comment validity, recommend action, let user decide (fix/reply/skip).

## When to Use

- PR has review comments needing evaluation before action
- Reviewer feedback might be incorrect or needs discussion
- Comments require varied responses (fix/reply/skip)
- Need to balance code quality with respectful reviewer engagement

## When NOT to Use

- All comments are clearly valid and straightforward to fix
- No comments yet or doing pre-review self-review
- Comments only on non-code files without technical analysis needed

## Workflow Overview

```dot
digraph pr_review_flow {
"Fetch PR comments" [shape=box];
"More comments?" [shape=diamond];
"Show comment + file context" [shape=box];
"Analyze validity" [shape=box];
"Recommend action" [shape=box];
"Ask user: Fix/Reply/Skip/Quit?" [shape=diamond];
"Make code changes" [shape=box];
"Draft reply" [shape=box];
"Track as skipped" [shape=box];
"Show summary" [shape=box];

"Fetch PR comments" -> "More comments?";
"More comments?" -> "Show comment + file context" [label="yes"];
"More comments?" -> "Show summary" [label="no"];
"Show comment + file context" -> "Analyze validity";
"Analyze validity" -> "Recommend action";
"Recommend action" -> "Ask user: Fix/Reply/Skip/Quit?";
"Ask user: Fix/Reply/Skip/Quit?" -> "Make code changes" [label="Fix"];
"Ask user: Fix/Reply/Skip/Quit?" -> "Draft reply" [label="Reply"];
"Ask user: Fix/Reply/Skip/Quit?" -> "Track as skipped" [label="Skip"];
"Ask user: Fix/Reply/Skip/Quit?" -> "Show summary" [label="Quit"];
"Make code changes" -> "More comments?";
"Draft reply" -> "More comments?";
"Track as skipped" -> "More comments?";
}
```

## Quick Reference

**Critical principle:** Reviewer may be wrong - analyze validity before recommending action.

| Phase | Actions |
|-------|---------|
| **Fetch** | `gh api repos/{owner}/{repo}/pulls/$PR/comments`<br>Extract: path, line, body, user.login, id<br>Exit if no comments |
| **Per Comment** | Show: file:line, author, comment, ±10 lines context<br>Analyze: Valid/Nitpick/Disagree/Question<br>Recommend: Fix/Reply/Skip with reasoning |
| **Fix** | Minimal changes per llm/rules-*.md<br>Offer reply draft: `Fixed: [what]. [why]`<br>Show: `gh api --method POST repos/{owner}/{repo}/pulls/comments/$ID/replies -f body="..."` |
| **Reply** | Draft based on type: Question/Suggestion/Disagreement<br>Let user edit<br>Show gh command (never auto-post) |
| **Summary** | Processed X/N: Fixed Y, Replied Z, Skipped W<br>List: files modified, reply drafts, next steps |

## Critical Principles

| Principle | Violation Pattern |
|-----------|-------------------|
| **Analyze first** | Accepting all feedback as valid without critical analysis |
| **Never auto-post** | Posting replies automatically instead of showing gh command |
| **One at a time** | Batch processing all comments without individual analysis |
| **Show context** | Making changes without displaying ±10 lines around code |
| **Minimal changes** | Large refactors in response to small comments |
| **Follow standards** | Ignoring llm/rules-*.md when fixing |
| **Respectful honesty** | Being defensive/dismissive when reviewer is wrong |
| **User control** | Posting drafts without letting user edit first |

## Reply Formats

- Fix: `Fixed: [what]. [why]`
- Update: `Updated: [what]`
- Answer: `[explanation]`
- Acknowledge: `Good catch, [action/reason]`
- Disagree: `[respectful reasoning]`

## Setup & Usage

Requires: `gh` CLI authenticated, GitHub remote configured

```bash
# Start session
"use address-pr-review for PR <number>"

# Or list PRs first
"use address-pr-review"
```
170 changes: 170 additions & 0 deletions .claude/skills/code-review/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
---
name: code-review
description: Use after completing implementation to review code quality, user impact, test coverage, and documentation before creating a PR
---

# Code Review

## Overview

Review code for quality, user impact, tests, and documentation. Balance technical excellence with practical simplicity.

**Core principle:** Clean code should serve users, not just developers.

## When to Use

- After completing a feature or bug fix
- Before creating a pull request
- When reviewing changes before merge

## When NOT to Use

- Trivial changes (typo fixes)
- Documentation-only changes
- Initial exploration/prototyping

## Review Process

| Phase | Focus | Key Question |
|-------|-------|--------------|
| 1. Identify | What changed? | `git diff --name-only develop` |
| 2. User Impact | How does this affect users? | Is UX better or worse? |
| 3. Code Quality | Does it follow standards? | KISS + no anti-patterns? |
| 4. Tests | Is it covered? | New code = new tests? |
| 5. Docs | What needs updating? | llm/state-*.md current? |

---

## Phase 1: Identify Changes

Categorize changed files:
- **Backend:** `lib/`, `app.py`
- **Frontend:** `frontend/src/`
- **Tests:** `tests/`
- **Docs:** `llm/`, `*.md`

Note change type: new feature | bug fix | refactoring | enhancement

---

## Phase 2: User Impact

**Ask for each change:**
1. Does this affect what users see or do?
2. Are error messages user-friendly (not technical jargon)?
3. Are loading states shown?
4. Can users recover from errors?
5. Is this the simplest UX possible?

**Red flags:**
- Silent failures (user doesn't know something failed)
- Lost work on errors
- Unclear feedback ("Error: 500" vs "Could not save")
- Unnecessary complexity exposed to users

---

## Phase 3: Code Quality

### KISS Check

Can each function be explained in one sentence? If not, it's too complex.

### Backend Anti-patterns (blocking)

- [ ] Silent failures (empty except blocks)
- [ ] God functions (>30 lines, >3 params)
- [ ] SQL injection (f-strings in queries)
- [ ] Missing error context
- [ ] Walrus operators / complex one-liners

### Frontend Anti-patterns (blocking)

- [ ] Empty catch blocks
- [ ] Inline fetch (not in service layer)
- [ ] Missing useEffect cleanup
- [ ] `any` types or `as` assertions
- [ ] Hardcoded colors (use theme: fg.*, canvas.*)
- [ ] Prop drilling (>5 props)

### Security

- [ ] Inputs validated at API boundary
- [ ] SQL parameterized (`?` placeholders)
- [ ] No secrets in code/logs

---

## Phase 4: Test Coverage

| Change Type | Required Test |
|-------------|---------------|
| New API endpoint | Unit test |
| New block | `tests/blocks/test_*.py` |
| Bug fix | Regression test |
| User workflow change | E2E test |
| Refactoring | Existing tests pass |

**Test quality:**
- Naming: `test_<method>_<scenario>_<expected>`
- One behavior per test
- Error cases tested, not just happy path

---

## Phase 5: Documentation

**Update llm/state-*.md when:**
- New API endpoint → `state-backend.md`
- New block → `state-backend.md`
- New component/page → `state-frontend.md`
- Architecture change → `state-project.md`

**Code comments:** explain WHY, not what. Lowercase, concise.

---

## Output Format

```markdown
### User Impact
[UX improvements or issues found]

### Anti-patterns
[location + violation + fix, or "none"]

### Code Quality Issues
[severity + location + fix, or "none"]

### Test Coverage
[required: present/missing | gaps if any]

### Documentation Updates
[files needing update, or "none"]

### Verdict
[BLOCK | REQUEST CHANGES | APPROVE]
Reason: [brief explanation]
```

---

## Verdict Rules

| Condition | Verdict |
|-----------|---------|
| Anti-patterns found | BLOCK |
| Security issues | BLOCK |
| Missing required tests | REQUEST CHANGES |
| Needs doc updates | REQUEST CHANGES |
| All checks pass | APPROVE |

---

## Golden Rules

1. Anti-patterns are blocking - always reject
2. User experience matters - clean code that hurts UX is bad code
3. KISS wins - one sentence explanation or it's too complex
4. Tests are not optional - new code needs tests
5. Fail loudly - silent failures are never acceptable
Loading
Loading