From 8961f2482117109415f513a287bc5b19f3b88139 Mon Sep 17 00:00:00 2001 From: ali Date: Fri, 6 Mar 2026 23:17:01 +0100 Subject: [PATCH] fix: address PR review comments for Kotlin/Android/KMP docs --- agents/kotlin-reviewer.md | 7 +++--- rules/kotlin/patterns.md | 4 +-- rules/kotlin/testing.md | 5 ++-- skills/android-clean-architecture/SKILL.md | 25 +++++++++++++------ .../compose-multiplatform-patterns/SKILL.md | 9 ++++--- skills/kotlin-coroutines-flows/SKILL.md | 11 +++++--- 6 files changed, 40 insertions(+), 21 deletions(-) diff --git a/agents/kotlin-reviewer.md b/agents/kotlin-reviewer.md index 3a8e23f9..1a728b5d 100644 --- a/agents/kotlin-reviewer.md +++ b/agents/kotlin-reviewer.md @@ -127,15 +127,14 @@ End every review with: | Severity | Count | Status | |----------|-------|--------| | CRITICAL | 0 | pass | -| HIGH | 1 | warn | +| HIGH | 1 | block | | MEDIUM | 2 | info | | LOW | 0 | note | -Verdict: WARNING — 1 HIGH issue should be resolved before merge. +Verdict: BLOCK — HIGH issues must be fixed 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 +- **Block**: Any CRITICAL or HIGH issues — must fix before merge diff --git a/rules/kotlin/patterns.md b/rules/kotlin/patterns.md index 96da1663..2b6f0763 100644 --- a/rules/kotlin/patterns.md +++ b/rules/kotlin/patterns.md @@ -68,8 +68,8 @@ Single responsibility, `operator fun invoke`: ```kotlin class GetItemsUseCase(private val repository: ItemRepository) { - suspend operator fun invoke(filter: Filter): Result> { - return repository.getAll(filter) + suspend operator fun invoke(id: String): Result { + return repository.getById(id) } } ``` diff --git a/rules/kotlin/testing.md b/rules/kotlin/testing.md index bfa5d776..98a2e38d 100644 --- a/rules/kotlin/testing.md +++ b/rules/kotlin/testing.md @@ -19,7 +19,8 @@ paths: ```kotlin @Test fun `loading state emitted then data`() = runTest { - val repo = FakeItemRepository(items = listOf(testItem)) + val repo = FakeItemRepository() + repo.addItem(testItem) val viewModel = ItemListViewModel(GetItemsUseCase(repo)) viewModel.state.test { @@ -119,7 +120,7 @@ fun `delete item emits updated list without deleted item`() = runTest { } ``` src/ ├── commonTest/kotlin/ # Shared tests (ViewModel, UseCase, Repository) -├── androidTest/kotlin/ # Android unit tests (JUnit) +├── androidUnitTest/kotlin/ # Android unit tests (JUnit) ├── androidInstrumentedTest/kotlin/ # Instrumented tests (Room, UI) └── iosTest/kotlin/ # iOS-specific tests ``` diff --git a/skills/android-clean-architecture/SKILL.md b/skills/android-clean-architecture/SKILL.md index cb48f032..1b4963f5 100644 --- a/skills/android-clean-architecture/SKILL.md +++ b/skills/android-clean-architecture/SKILL.md @@ -81,7 +81,8 @@ data class Item( val title: String, val description: String, val tags: List, - val status: Status + val status: Status, + val category: String ) enum class Status { DRAFT, ACTIVE, ARCHIVED } @@ -119,6 +120,12 @@ class ItemRepositoryImpl( } } + override suspend fun saveItem(item: Item): Result { + return runCatching { + localDataSource.insertItems(listOf(item.toEntity())) + } + } + override fun observeItems(): Flow> { return localDataSource.observeAll().map { entities -> entities.map { it.toDomain() } @@ -138,7 +145,8 @@ fun ItemEntity.toDomain() = Item( title = title, description = description, tags = tags.split("|"), - status = Status.valueOf(status) + status = Status.valueOf(status), + category = category ) fun ItemDto.toEntity() = ItemEntity( @@ -146,7 +154,8 @@ fun ItemDto.toEntity() = ItemEntity( title = title, description = description, tags = tags.joinToString("|"), - status = status + status = status, + category = category ) ``` @@ -159,7 +168,8 @@ data class ItemEntity( val title: String, val description: String, val tags: String, - val status: String + val status: String, + val category: String ) @Dao @@ -184,15 +194,16 @@ CREATE TABLE ItemEntity ( title TEXT NOT NULL, description TEXT NOT NULL, tags TEXT NOT NULL, - status TEXT NOT NULL + status TEXT NOT NULL, + category TEXT NOT NULL ); getByCategory: SELECT * FROM ItemEntity WHERE category = ?; upsert: -INSERT OR REPLACE INTO ItemEntity (id, title, description, tags, status) -VALUES (?, ?, ?, ?, ?); +INSERT OR REPLACE INTO ItemEntity (id, title, description, tags, status, category) +VALUES (?, ?, ?, ?, ?, ?); observeAll: SELECT * FROM ItemEntity; diff --git a/skills/compose-multiplatform-patterns/SKILL.md b/skills/compose-multiplatform-patterns/SKILL.md index 6889a8df..70e4ac65 100644 --- a/skills/compose-multiplatform-patterns/SKILL.md +++ b/skills/compose-multiplatform-patterns/SKILL.md @@ -91,7 +91,7 @@ fun onEvent(event: ItemListEvent) { when (event) { is ItemListEvent.Search -> onSearch(event.query) is ItemListEvent.Delete -> deleteItem(event.itemId) - is ItemListEvent.Refresh -> loadItems() + is ItemListEvent.Refresh -> loadItems(_state.value.searchQuery) } } @@ -252,9 +252,12 @@ val showScrollToTop by remember { // BAD — new lambda and list every recomposition items.filter { it.isActive }.forEach { ActiveItem(it, onClick = { handle(it) }) } -// GOOD — remember filtered list, use method reference or remembered lambda +// GOOD — remember filtered list, stable lambda with key val activeItems = remember(items) { items.filter { it.isActive } } -activeItems.forEach { ActiveItem(it, onClick = remember { { handle(it) } }) } +activeItems.forEach { item -> + val onClick = remember(item.id) { { handle(item) } } + ActiveItem(item, onClick = onClick) +} ``` ## Theming diff --git a/skills/kotlin-coroutines-flows/SKILL.md b/skills/kotlin-coroutines-flows/SKILL.md index 972b43af..6650b0b7 100644 --- a/skills/kotlin-coroutines-flows/SKILL.md +++ b/skills/kotlin-coroutines-flows/SKILL.md @@ -125,8 +125,13 @@ searchQuery // Retry with exponential backoff fun fetchWithRetry(): Flow = flow { emit(api.fetch()) } - .retry(3) { cause -> - cause is IOException && run { delay(1000L * (1 shl (3 - remainingAttempts))) ; true } + .retryWhen { cause, attempt -> + if (cause is IOException && attempt < 3) { + delay(1000L * (1 shl attempt.toInt())) + true + } else { + false + } } ``` @@ -183,7 +188,7 @@ In KMP, use `Dispatchers.Default` and `Dispatchers.Main` (available on all platf Long-running loops must check for cancellation: ```kotlin -suspend fun processItems(items: List) { +suspend fun processItems(items: List) = coroutineScope { for (item in items) { ensureActive() // throws CancellationException if cancelled process(item)