fix: address PR review comments for Kotlin/Android/KMP docs

This commit is contained in:
ali
2026-03-06 23:17:01 +01:00
committed by Affaan Mustafa
parent f10d638bfa
commit 8961f24821
6 changed files with 40 additions and 21 deletions

View File

@@ -127,15 +127,14 @@ End every review with:
| Severity | Count | Status | | Severity | Count | Status |
|----------|-------|--------| |----------|-------|--------|
| CRITICAL | 0 | pass | | CRITICAL | 0 | pass |
| HIGH | 1 | warn | | HIGH | 1 | block |
| MEDIUM | 2 | info | | MEDIUM | 2 | info |
| LOW | 0 | note | | LOW | 0 | note |
Verdict: WARNING — 1 HIGH issue should be resolved before merge. Verdict: BLOCK — HIGH issues must be fixed before merge.
``` ```
## Approval Criteria ## Approval Criteria
- **Approve**: No CRITICAL or HIGH issues - **Approve**: No CRITICAL or HIGH issues
- **Warning**: HIGH issues only (can merge with caution) - **Block**: Any CRITICAL or HIGH issues — must fix before merge
- **Block**: CRITICAL issues — must fix before merge

View File

@@ -68,8 +68,8 @@ Single responsibility, `operator fun invoke`:
```kotlin ```kotlin
class GetItemsUseCase(private val repository: ItemRepository) { class GetItemsUseCase(private val repository: ItemRepository) {
suspend operator fun invoke(filter: Filter): Result<List<Item>> { suspend operator fun invoke(id: String): Result<Item> {
return repository.getAll(filter) return repository.getById(id)
} }
} }
``` ```

View File

@@ -19,7 +19,8 @@ paths:
```kotlin ```kotlin
@Test @Test
fun `loading state emitted then data`() = runTest { fun `loading state emitted then data`() = runTest {
val repo = FakeItemRepository(items = listOf(testItem)) val repo = FakeItemRepository()
repo.addItem(testItem)
val viewModel = ItemListViewModel(GetItemsUseCase(repo)) val viewModel = ItemListViewModel(GetItemsUseCase(repo))
viewModel.state.test { viewModel.state.test {
@@ -119,7 +120,7 @@ fun `delete item emits updated list without deleted item`() = runTest { }
``` ```
src/ src/
├── commonTest/kotlin/ # Shared tests (ViewModel, UseCase, Repository) ├── 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) ├── androidInstrumentedTest/kotlin/ # Instrumented tests (Room, UI)
└── iosTest/kotlin/ # iOS-specific tests └── iosTest/kotlin/ # iOS-specific tests
``` ```

View File

@@ -81,7 +81,8 @@ data class Item(
val title: String, val title: String,
val description: String, val description: String,
val tags: List<String>, val tags: List<String>,
val status: Status val status: Status,
val category: String
) )
enum class Status { DRAFT, ACTIVE, ARCHIVED } enum class Status { DRAFT, ACTIVE, ARCHIVED }
@@ -119,6 +120,12 @@ class ItemRepositoryImpl(
} }
} }
override suspend fun saveItem(item: Item): Result<Unit> {
return runCatching {
localDataSource.insertItems(listOf(item.toEntity()))
}
}
override fun observeItems(): Flow<List<Item>> { override fun observeItems(): Flow<List<Item>> {
return localDataSource.observeAll().map { entities -> return localDataSource.observeAll().map { entities ->
entities.map { it.toDomain() } entities.map { it.toDomain() }
@@ -138,7 +145,8 @@ fun ItemEntity.toDomain() = Item(
title = title, title = title,
description = description, description = description,
tags = tags.split("|"), tags = tags.split("|"),
status = Status.valueOf(status) status = Status.valueOf(status),
category = category
) )
fun ItemDto.toEntity() = ItemEntity( fun ItemDto.toEntity() = ItemEntity(
@@ -146,7 +154,8 @@ fun ItemDto.toEntity() = ItemEntity(
title = title, title = title,
description = description, description = description,
tags = tags.joinToString("|"), tags = tags.joinToString("|"),
status = status status = status,
category = category
) )
``` ```
@@ -159,7 +168,8 @@ data class ItemEntity(
val title: String, val title: String,
val description: String, val description: String,
val tags: String, val tags: String,
val status: String val status: String,
val category: String
) )
@Dao @Dao
@@ -184,15 +194,16 @@ CREATE TABLE ItemEntity (
title TEXT NOT NULL, title TEXT NOT NULL,
description TEXT NOT NULL, description TEXT NOT NULL,
tags TEXT NOT NULL, tags TEXT NOT NULL,
status TEXT NOT NULL status TEXT NOT NULL,
category TEXT NOT NULL
); );
getByCategory: getByCategory:
SELECT * FROM ItemEntity WHERE category = ?; SELECT * FROM ItemEntity WHERE category = ?;
upsert: upsert:
INSERT OR REPLACE INTO ItemEntity (id, title, description, tags, status) INSERT OR REPLACE INTO ItemEntity (id, title, description, tags, status, category)
VALUES (?, ?, ?, ?, ?); VALUES (?, ?, ?, ?, ?, ?);
observeAll: observeAll:
SELECT * FROM ItemEntity; SELECT * FROM ItemEntity;

View File

@@ -91,7 +91,7 @@ fun onEvent(event: ItemListEvent) {
when (event) { when (event) {
is ItemListEvent.Search -> onSearch(event.query) is ItemListEvent.Search -> onSearch(event.query)
is ItemListEvent.Delete -> deleteItem(event.itemId) 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 // BAD — new lambda and list every recomposition
items.filter { it.isActive }.forEach { ActiveItem(it, onClick = { handle(it) }) } 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 } } 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 ## Theming

View File

@@ -125,8 +125,13 @@ searchQuery
// Retry with exponential backoff // Retry with exponential backoff
fun fetchWithRetry(): Flow<Data> = flow { emit(api.fetch()) } fun fetchWithRetry(): Flow<Data> = flow { emit(api.fetch()) }
.retry(3) { cause -> .retryWhen { cause, attempt ->
cause is IOException && run { delay(1000L * (1 shl (3 - remainingAttempts))) ; true } 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: Long-running loops must check for cancellation:
```kotlin ```kotlin
suspend fun processItems(items: List<Item>) { suspend fun processItems(items: List<Item>) = coroutineScope {
for (item in items) { for (item in items) {
ensureActive() // throws CancellationException if cancelled ensureActive() // throws CancellationException if cancelled
process(item) process(item)