|
| 1 | +--- |
| 2 | +name: pr-review |
| 3 | +description: This skill should be used when the user asks to review a pull request, check code quality, run a PR review, or asks "is this PR ready to merge?" for the godtools-android project. It performs a structured review checking project-specific patterns learned from historical PR feedback. |
| 4 | +version: 1.0.0 |
| 5 | +--- |
| 6 | + |
| 7 | +# GodTools Android PR Review |
| 8 | + |
| 9 | +Perform a structured pull request review for the godtools-android project. This skill applies patterns learned from historical code reviews and the project's established conventions. |
| 10 | + |
| 11 | +## Review Workflow |
| 12 | + |
| 13 | +### Step 1: Get the Diff |
| 14 | + |
| 15 | +```bash |
| 16 | +# If a PR exists |
| 17 | +gh pr diff |
| 18 | + |
| 19 | +# Otherwise compare to base branch |
| 20 | +git diff develop...HEAD --name-only |
| 21 | +git diff develop...HEAD |
| 22 | +``` |
| 23 | + |
| 24 | +Identify all changed files and categorize them by type (Kotlin, build scripts, resources, manifests, tests). |
| 25 | + |
| 26 | +### Step 2: Pre-Flight Check — ktlint |
| 27 | + |
| 28 | +Always verify ktlint passes. This is a hard blocker: |
| 29 | + |
| 30 | +```bash |
| 31 | +./gradlew :build-logic:ktlintCheck ktlintCheck |
| 32 | +``` |
| 33 | + |
| 34 | +If it fails, report as **Critical** — PR cannot merge until resolved. |
| 35 | + |
| 36 | +### Step 3: Run the Checklist |
| 37 | + |
| 38 | +For each changed file, apply the relevant checks from `references/patterns.md`. Use the priority levels below to triage findings: |
| 39 | + |
| 40 | +- **Critical** — Hard blocker, must fix before merge |
| 41 | +- **Important** — Should fix in this PR |
| 42 | +- **Suggestion** — Nice to have, can be deferred |
| 43 | + |
| 44 | +### Step 4: PR Hygiene Check |
| 45 | + |
| 46 | +```bash |
| 47 | +git diff develop...HEAD --stat |
| 48 | +``` |
| 49 | + |
| 50 | +Look for signs of unrelated auto-formatter changes: |
| 51 | +- Large line-count diffs on files tangentially touched by the PR |
| 52 | +- Whitespace-only changes in unrelated sections |
| 53 | +- Reformatted import blocks or trailing commas not part of the feature |
| 54 | + |
| 55 | +If found, flag as **Important**: instruct to use git patch-mode staging or scope-selective reformatting in Android Studio. |
| 56 | + |
| 57 | +### Step 5: Report Findings |
| 58 | + |
| 59 | +Structure the output as: |
| 60 | + |
| 61 | +``` |
| 62 | +## PR Review: <branch or PR title> |
| 63 | +
|
| 64 | +### Critical Issues (must fix before merge) |
| 65 | +- [FILE:LINE] Issue description |
| 66 | +
|
| 67 | +### Important Issues (should fix) |
| 68 | +- [FILE:LINE] Issue description |
| 69 | +
|
| 70 | +### Suggestions |
| 71 | +- [FILE:LINE] Suggestion |
| 72 | +
|
| 73 | +### Looks Good |
| 74 | +- What's well done in this PR |
| 75 | +``` |
| 76 | + |
| 77 | +## Quick Reference: Top Issues from Historical Reviews |
| 78 | + |
| 79 | +These are the patterns most frequently flagged in past reviews. Check these first: |
| 80 | + |
| 81 | +1. **ktlint failures** — Run check before reporting anything else. |
| 82 | +2. **AS wizard scaffolding left in** — Delete generated `colors.xml`, `strings.xml` with `app_name`, default launcher icons, and per-module theme definitions from library modules. |
| 83 | +3. **Unrelated auto-formatter changes** — Flag if diff contains formatting-only changes in untouched sections. |
| 84 | +4. **Convention plugin boilerplate in `build.gradle.kts`** — `godtools.library-conventions` already provides minSdk, namespace (via `android {}` block), Kotlin toolchain, proguard, and Compose setup. Don't re-declare them. |
| 85 | +5. **`libs.versions.toml` clutter** — No duplicate version entries, no unused plugin declarations. |
| 86 | +6. **Composables missing `modifier: Modifier = Modifier`** — Required on all public/internal composables. |
| 87 | +7. **Wrong theme** — Use `GodToolsTheme` from `:ui:base`, never define a per-module theme. |
| 88 | +8. **Cross-module Activity intents** — Intent/PendingIntent creation for Activities must use extension functions in `ui/base/src/main/kotlin/org/cru/godtools/base/ui/Activities.kt`. |
| 89 | +9. **Circuit actions as direct calls** — User-triggered actions must flow through named `UiEvent` entries in `UiState.eventSink`, not direct function calls from the UI composable. |
| 90 | +10. **Local `includeBuild` in settings.gradle.kts** — Never commit local KMP project substitutions. Publish the KMP artifact and update the version in `libs.versions.toml` instead. |
| 91 | +11. **`println` for logging** — Use `Timber` instead. |
| 92 | +12. **Overly broad exception catching** — Catch specific exception types, not `Exception`. |
| 93 | +13. **Visibility too wide** — Prefer `internal` for module-scoped composables/functions, `private` where possible. |
| 94 | + |
| 95 | +For full pattern details, consult `references/patterns.md`. |
0 commit comments