-
-
Notifications
You must be signed in to change notification settings - Fork 9.1k
feat: add Kotlin, Android, and KMP rules, agent, skills, and command #345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
f0e0cb8
feat: add Kotlin, Android, and KMP rules, agent, skills, and command
rezaiyan 39e584c
fix: address PR review comments for Kotlin/Android/KMP docs
rezaiyan 6d12aa3
fix: rename GetItemsUseCase to GetItemUseCase for consistency
rezaiyan 5e1a8be
fix: address remaining PR review comments for Kotlin/Android/KMP docs
rezaiyan 02d5450
fix: resolve semantic mismatch between UseCase naming and ViewModel u…
rezaiyan db3cf86
docs: tighten kotlin support examples
affaan-m File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| --- | ||
| name: kotlin-reviewer | ||
| description: Kotlin and Android/KMP code reviewer. Reviews Kotlin code for idiomatic patterns, coroutine safety, Compose best practices, clean architecture violations, and common Android pitfalls. | ||
| tools: ["Read", "Grep", "Glob", "Bash"] | ||
| model: sonnet | ||
| --- | ||
|
|
||
| You are a senior Kotlin and Android/KMP code reviewer ensuring idiomatic, safe, and maintainable code. | ||
|
|
||
| ## Your Role | ||
|
|
||
| - Review Kotlin code for idiomatic patterns and Android/KMP best practices | ||
| - Detect coroutine misuse, Flow anti-patterns, and lifecycle bugs | ||
| - Enforce clean architecture module boundaries | ||
| - Identify Compose performance issues and recomposition traps | ||
| - You DO NOT refactor or rewrite code — you report findings only | ||
|
|
||
| ## Workflow | ||
|
|
||
| ### Step 1: Gather Context | ||
|
|
||
| Run `git diff --staged` and `git diff` to see changes. If no diff, check `git log --oneline -5`. Identify Kotlin/KTS files that changed. | ||
|
|
||
| ### Step 2: Understand Project Structure | ||
|
|
||
| Check for: | ||
| - `build.gradle.kts` or `settings.gradle.kts` to understand module layout | ||
| - `CLAUDE.md` for project-specific conventions | ||
| - Whether this is Android-only, KMP, or Compose Multiplatform | ||
|
|
||
| ### Step 3: Read and Review | ||
|
|
||
| Read changed files fully. Apply the review checklist below, checking surrounding code for context. | ||
|
|
||
| ### Step 4: Report Findings | ||
|
|
||
| Use the output format below. Only report issues with >80% confidence. | ||
|
|
||
| ## Review Checklist | ||
|
|
||
| ### Architecture (CRITICAL) | ||
|
|
||
| - **Domain importing framework** — `domain` module must not import Android, Ktor, Room, or any framework | ||
| - **Data layer leaking to UI** — Entities or DTOs exposed to presentation layer (must map to domain models) | ||
| - **ViewModel business logic** — Complex logic belongs in UseCases, not ViewModels | ||
| - **Circular dependencies** — Module A depends on B and B depends on A | ||
|
|
||
| ### Coroutines & Flows (HIGH) | ||
|
|
||
| - **GlobalScope usage** — Must use structured scopes (`viewModelScope`, `coroutineScope`) | ||
| - **Catching CancellationException** — Must rethrow or not catch; swallowing breaks cancellation | ||
| - **Missing `withContext` for IO** — Database/network calls on `Dispatchers.Main` | ||
| - **StateFlow with mutable state** — Using mutable collections inside StateFlow (must copy) | ||
| - **Flow collection in `init {}`** — Should use `stateIn()` or launch in scope | ||
| - **Missing `WhileSubscribed`** — `stateIn(scope, SharingStarted.Eagerly)` when `WhileSubscribed` is appropriate | ||
|
|
||
| ```kotlin | ||
| // BAD — swallows cancellation | ||
| try { fetchData() } catch (e: Exception) { log(e) } | ||
|
|
||
| // GOOD — preserves cancellation | ||
| try { fetchData() } catch (e: CancellationException) { throw e } catch (e: Exception) { log(e) } | ||
| // or use runCatching and check | ||
| ``` | ||
|
|
||
| ### Compose (HIGH) | ||
|
|
||
| - **Unstable parameters** — Composables receiving mutable types cause unnecessary recomposition | ||
| - **Side effects outside LaunchedEffect** — Network/DB calls must be in `LaunchedEffect` or ViewModel | ||
| - **NavController passed deep** — Pass lambdas instead of `NavController` references | ||
| - **Missing `key()` in LazyColumn** — Items without stable keys cause poor performance | ||
| - **`remember` with missing keys** — Computation not recalculated when dependencies change | ||
| - **Object allocation in parameters** — Creating objects inline causes recomposition | ||
|
|
||
| ```kotlin | ||
| // BAD — new lambda every recomposition | ||
| Button(onClick = { viewModel.doThing(item.id) }) | ||
|
|
||
| // GOOD — stable reference | ||
| val onClick = remember(item.id) { { viewModel.doThing(item.id) } } | ||
| Button(onClick = onClick) | ||
| ``` | ||
|
|
||
| ### Kotlin Idioms (MEDIUM) | ||
|
|
||
| - **`!!` usage** — Non-null assertion; prefer `?.`, `?:`, `requireNotNull`, or `checkNotNull` | ||
| - **`var` where `val` works** — Prefer immutability | ||
| - **Java-style patterns** — Static utility classes (use top-level functions), getters/setters (use properties) | ||
| - **String concatenation** — Use string templates `"Hello $name"` instead of `"Hello " + name` | ||
| - **`when` without exhaustive branches** — Sealed classes/interfaces should use exhaustive `when` | ||
| - **Mutable collections exposed** — Return `List` not `MutableList` from public APIs | ||
|
|
||
| ### Android Specific (MEDIUM) | ||
|
|
||
| - **Context leaks** — Storing `Activity` or `Fragment` references in singletons/ViewModels | ||
| - **Missing ProGuard rules** — Serialized classes without `@Keep` or ProGuard rules | ||
| - **Hardcoded strings** — User-facing strings not in `strings.xml` or Compose resources | ||
| - **Missing lifecycle handling** — Collecting Flows in Activities without `repeatOnLifecycle` | ||
|
|
||
| ### Gradle & Build (LOW) | ||
|
|
||
| - **Version catalog not used** — Hardcoded versions instead of `libs.versions.toml` | ||
| - **Unnecessary dependencies** — Dependencies added but not used | ||
| - **Missing KMP source sets** — Declaring `androidMain` code that could be `commonMain` | ||
|
|
||
| ## Output Format | ||
|
|
||
| ``` | ||
| [CRITICAL] Domain module imports Android framework | ||
| File: domain/src/main/kotlin/com/app/domain/UserUseCase.kt:3 | ||
| Issue: `import android.content.Context` — domain must be pure Kotlin with no framework dependencies. | ||
| Fix: Move Context-dependent logic to data or platforms layer. Pass data via repository interface. | ||
|
|
||
| [HIGH] StateFlow holding mutable list | ||
| File: presentation/src/main/kotlin/com/app/ui/ListViewModel.kt:25 | ||
| Issue: `_state.value.items.add(newItem)` mutates the list inside StateFlow — Compose won't detect the change. | ||
| Fix: Use `_state.update { it.copy(items = it.items + newItem) }` | ||
| ``` | ||
|
|
||
| ## Summary Format | ||
|
|
||
| End every review with: | ||
|
|
||
| ``` | ||
| ## Review Summary | ||
|
|
||
| | Severity | Count | Status | | ||
| |----------|-------|--------| | ||
| | CRITICAL | 0 | pass | | ||
| | HIGH | 1 | warn | | ||
| | MEDIUM | 2 | info | | ||
| | LOW | 0 | note | | ||
|
|
||
| Verdict: WARNING — 1 HIGH issue should be resolved before merge. | ||
| ``` | ||
|
|
||
| ## Approval Criteria | ||
|
|
||
| - **Approve**: No CRITICAL or HIGH issues | ||
| - **Warning**: HIGH issues only (can merge with caution) | ||
| - **Block**: CRITICAL issues — must fix before merge | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| --- | ||
| description: Fix Gradle build errors for Android and KMP projects | ||
| --- | ||
|
|
||
| # Gradle Build Fix | ||
|
|
||
| Incrementally fix Gradle build and compilation errors for Android and Kotlin Multiplatform projects. | ||
|
|
||
| ## Step 1: Detect Build Configuration | ||
|
|
||
| Identify the project type and run the appropriate build: | ||
|
|
||
| | Indicator | Build Command | | ||
| |-----------|---------------| | ||
| | `build.gradle.kts` + `composeApp/` (KMP) | `./gradlew composeApp:compileKotlinMetadata 2>&1` | | ||
| | `build.gradle.kts` + `app/` (Android) | `./gradlew app:compileDebugKotlin 2>&1` | | ||
| | `settings.gradle.kts` with modules | `./gradlew assemble 2>&1` | | ||
| | Detekt configured | `./gradlew detekt 2>&1` | | ||
|
|
||
| Also check `gradle.properties` and `local.properties` for configuration. | ||
|
|
||
| ## Step 2: Parse and Group Errors | ||
|
|
||
| 1. Run the build command and capture output | ||
| 2. Separate Kotlin compilation errors from Gradle configuration errors | ||
| 3. Group by module and file path | ||
| 4. Sort: configuration errors first, then compilation errors by dependency order | ||
|
|
||
| ## Step 3: Fix Loop | ||
|
|
||
| For each error: | ||
|
|
||
| 1. **Read the file** — Full context around the error line | ||
| 2. **Diagnose** — Common categories: | ||
| - Missing import or unresolved reference | ||
| - Type mismatch or incompatible types | ||
| - Missing dependency in `build.gradle.kts` | ||
| - Expect/actual mismatch (KMP) | ||
| - Compose compiler error | ||
| 3. **Fix minimally** — Smallest change that resolves the error | ||
| 4. **Re-run build** — Verify fix and check for new errors | ||
| 5. **Continue** — Move to next error | ||
|
|
||
| ## Step 4: Guardrails | ||
|
|
||
| Stop and ask the user if: | ||
| - Fix introduces more errors than it resolves | ||
| - Same error persists after 3 attempts | ||
| - Error requires adding new dependencies or changing module structure | ||
| - Gradle sync itself fails (configuration-phase error) | ||
| - Error is in generated code (Room, SQLDelight, KSP) | ||
|
|
||
| ## Step 5: Summary | ||
|
|
||
| Report: | ||
| - Errors fixed (module, file, description) | ||
| - Errors remaining | ||
| - New errors introduced (should be zero) | ||
| - Suggested next steps | ||
|
|
||
| ## Common Gradle/KMP Fixes | ||
|
|
||
| | Error | Fix | | ||
| |-------|-----| | ||
| | Unresolved reference in `commonMain` | Check if the dependency is in `commonMain.dependencies {}` | | ||
| | Expect declaration without actual | Add `actual` implementation in each platform source set | | ||
| | Compose compiler version mismatch | Align Kotlin and Compose compiler versions in `libs.versions.toml` | | ||
| | Duplicate class | Check for conflicting dependencies with `./gradlew dependencies` | | ||
| | KSP error | Run `./gradlew kspCommonMainKotlinMetadata` to regenerate | | ||
| | Configuration cache issue | Check for non-serializable task inputs | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| --- | ||
| paths: | ||
| - "**/*.kt" | ||
| - "**/*.kts" | ||
| --- | ||
| # Kotlin Coding Style | ||
|
|
||
| > This file extends [common/coding-style.md](../common/coding-style.md) with Kotlin specific content. | ||
|
|
||
| ## Formatting | ||
|
|
||
| - **ktlint** or **Detekt** for style enforcement | ||
| - Official Kotlin code style (`kotlin.code.style=official` in `gradle.properties`) | ||
|
|
||
| ## Immutability | ||
|
|
||
| - Prefer `val` over `var` — default to `val` and only use `var` when mutation is required | ||
| - Use `data class` for value types; use immutable collections (`List`, `Map`, `Set`) in public APIs | ||
| - Copy-on-write for state updates: `state.copy(field = newValue)` | ||
|
|
||
| ## Naming | ||
|
|
||
| Follow Kotlin conventions: | ||
| - `camelCase` for functions and properties | ||
| - `PascalCase` for classes, interfaces, objects, and type aliases | ||
| - `SCREAMING_SNAKE_CASE` for constants (`const val` or `@JvmStatic`) | ||
| - Prefix interfaces with behavior, not `I`: `Clickable` not `IClickable` | ||
|
|
||
| ## Null Safety | ||
|
|
||
| - Never use `!!` — prefer `?.`, `?:`, `requireNotNull()`, or `checkNotNull()` | ||
| - Use `?.let {}` for scoped null-safe operations | ||
| - Return nullable types from functions that can legitimately have no result | ||
|
|
||
| ```kotlin | ||
| // BAD | ||
| val name = user!!.name | ||
|
|
||
| // GOOD | ||
| val name = user?.name ?: "Unknown" | ||
| val name = requireNotNull(user) { "User must be set before accessing name" }.name | ||
| ``` | ||
|
|
||
| ## Sealed Types | ||
|
|
||
| Use sealed classes/interfaces to model closed state hierarchies: | ||
|
|
||
| ```kotlin | ||
| sealed interface UiState<out T> { | ||
| data object Loading : UiState<Nothing> | ||
| data class Success<T>(val data: T) : UiState<T> | ||
| data class Error(val message: String) : UiState<Nothing> | ||
| } | ||
| ``` | ||
|
|
||
| Always use exhaustive `when` with sealed types — no `else` branch. | ||
|
|
||
| ## Extension Functions | ||
|
|
||
| Use extension functions for utility operations, but keep them discoverable: | ||
| - Place in a file named after the receiver type (`StringExt.kt`, `FlowExt.kt`) | ||
| - Keep scope limited — don't add extensions to `Any` or overly generic types | ||
|
|
||
| ## Scope Functions | ||
|
|
||
| Use the right scope function: | ||
| - `let` — null check + transform: `user?.let { greet(it) }` | ||
| - `run` — compute a result using receiver: `service.run { fetch(config) }` | ||
| - `apply` — configure an object: `builder.apply { timeout = 30 }` | ||
| - `also` — side effects: `result.also { log(it) }` | ||
| - Avoid deep nesting of scope functions (max 2 levels) | ||
|
|
||
| ## Error Handling | ||
|
|
||
| - Use `Result<T>` or custom sealed types | ||
| - Use `runCatching {}` for wrapping throwable code | ||
| - Never catch `CancellationException` — always rethrow it | ||
| - Avoid `try-catch` for control flow | ||
|
|
||
| ```kotlin | ||
| // BAD — using exceptions for control flow | ||
| val user = try { repository.getUser(id) } catch (e: NotFoundException) { null } | ||
|
|
||
| // GOOD — nullable return | ||
| val user: User? = repository.findUser(id) | ||
| ``` |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.