chore(claude): Port code-style, test gotchas, and PR labels from sdmaid-se

This commit is contained in:
darken
2026-07-28 20:18:54 +02:00
committed by Matthias Urhahn
parent f39ea20690
commit 7c75ec02bd
4 changed files with 151 additions and 0 deletions
+1
View File
@@ -53,6 +53,7 @@ Loaded on demand, when a matching file is read (`paths:` frontmatter):
| Rule | Loads for |
|------|-----------|
| `.claude/rules/code-style.md` | Kotlin/Compose sources in `main/`, `foss/`, `gplay/`, `debug/` |
| `.claude/rules/testing.md` | `app/src/test/`, `testFoss/`, `testGplay/` |
| `.claude/rules/localization.md` | `**/res/values/strings.xml` (base locale) |
| `.claude/rules/screenshots.md` | Screenshot composables, `screenshotTest/`, fastlane scripts |
+110
View File
@@ -0,0 +1,110 @@
---
description: Kotlin and Compose conventions — logging, ViewModel base classes, the ScreenHost/Screen split, DataStore settings
paths:
- "app/src/main/**/*.kt"
- "app/src/foss/**/*.kt"
- "app/src/gplay/**/*.kt"
- "app/src/debug/**/*.kt"
---
# Code Style
## Logging
`logTag()` builds the tag; `log()` takes a lambda so the message is only built if it's emitted.
```kotlin
import eu.darken.capod.common.debug.logging.log
import eu.darken.capod.common.debug.logging.logTag
import eu.darken.capod.common.debug.logging.Logging.Priority.*
companion object {
private val TAG = logTag("Profiles", "Repo") // multi-part tags are the norm
}
log(TAG) { "Processing $item" } // DEBUG is the default
log(TAG, VERBOSE) { "Devices changed" }
log(TAG, ERROR) { "Failed: ${e.asLog()}" } // asLog() for stacktraces
```
Never suppress protocol logging — downgrading a level is fine, removing the call is not.
## ViewModel base classes
Four exist. Use **`ViewModel4`** for new work — it's the current one (12 subclasses) and wires
`NavigationEventSource` + `ErrorEventSource2`.
- `ViewModel4` — current, use this
- `ViewModel2` — plain base, no nav/error event sources (4 subclasses)
- `ViewModel1` — legacy (1 subclass)
- `ViewModel3`**dead, zero subclasses.** It's the `ViewModel4` shape against the older
`NavEventSource`/`ErrorEventSource` interfaces. Don't extend it.
## Compose: the Host/Screen split
Every screen is two composables.
**`<Feature>ScreenHost`** — the only place that touches `hiltViewModel()`, installs the event
handlers, and collects state.
**`<Feature>Screen`** — presentation only. Takes a plain state object plus `on*` callbacks, so it
previews without Hilt.
```kotlin
@Composable
fun SettingsScreenHost(vm: SettingsViewModel = hiltViewModel()) {
ErrorEventHandler(vm)
NavigationEventHandler(vm)
val state by vm.state.collectAsStateWithLifecycle(initialValue = null)
state?.let {
SettingsScreen(
state = it,
onNavigateUp = { vm.navUp() },
onWiki = { vm.openUrl("https://github.com/d4rken-org/capod/wiki") },
)
}
}
@Composable
fun SettingsScreen(
state: SettingsViewModel.State,
onNavigateUp: () -> Unit,
onWiki: () -> Unit,
modifier: Modifier = Modifier, // last, after the required params
) { ... }
```
- `modifier: Modifier = Modifier` goes **last**, after required params — not first
- The Host null-guards state; `collectAsStateWithLifecycle(initialValue = null)` is the usual shape
- Wrap previews in `PreviewWrapper` (`common/compose/PreviewWrapper.kt`), which applies `CapodTheme`
plus a background `Surface`
- Trailing commas on multi-line parameter lists and argument lists
## DataStore settings
`createValue()` is overloaded. Primitives need no serializer:
```kotlin
val monitorMode = dataStore.createValue("core.monitor.mode", MonitorMode.AUTOMATIC)
```
`@Serializable` types take a `Json`, and optionally fall back instead of throwing on corrupt or
legacy stored JSON:
```kotlin
val config = dataStore.createValue("some.config", SomeConfig(), json, onErrorFallbackToDefault = true)
```
Read and write via `.value()` / `.value(x)` (suspend) or `.flow` (reactive). Both `value` functions
are **extension functions**, not members — see `.claude/rules/testing.md` for what that means when
mocking.
## General
- Package by feature, not by layer
- Prefer adding to an existing file over creating a new one
- Prefer flow-based, cancellable solutions
- No comments for self-evident code
- Place `@Suppress` as close to the affected code as possible — on the function or constructor,
not the whole class
+12
View File
@@ -47,6 +47,18 @@ Cover only what the diff can't show:
**Review checklist**`- [ ]` items, only when there are several non-trivial things to verify.
A single tricky point stays a Technical Context bullet.
## Labels
Apply labels that match the change. Run `gh label list` to confirm what exists — do not invent new
ones. Skip labels that don't fit; no labels beats wrong labels.
- **Type**: `bug` for fixes, `enhancement` for new features or improvements
- **Transport**: `coms/AAP` when the change touches the L2CAP session path, `coms/BLE` when it
touches advertisement parsing. Both if it spans the merge in `DeviceMonitor`
- **Scope**: `device support` for new or fixed pod models, `Translations` for string/locale work,
`Build/Deploy` for CI, Gradle, and release tooling
- `Needs Info/Repro` is a triage label for issues — not for your own PRs
## Conventions
- Link issues with "Closes #123" / "Fixes #123" / "Resolves #123"
+28
View File
@@ -48,3 +48,31 @@ Each task compiles and runs only its own flavor — running the wrong one silent
CI runs both. Flavor-specific tests are for code that only exists in that flavor — billing in
`gplay`, the sponsor-based upgrade flow in `foss`.
## Helpers that already exist
- `runTest2(autoCancel, context, expectedError, testBody)` in `testhelpers/coroutine/TestExtensions.kt`
use `expectedError = SomeException::class` instead of hand-rolling a throws-assertion around `runTest`
- `FakeDataStoreValue<T>(initial)` in `testhelpers/datastore/` — a working fake with a real backing
`MutableStateFlow`; read/write it through `.value` and pass `.mock` to the code under test
## Mocking `DataStoreValue`
`DataStoreValue.value()` and `.value(T)` are **extension functions** (`DataStoreValue.kt:54,56`), not
members, so MockK cannot stub them. They delegate to `flow.first()` and `update { }` — stub those:
```kotlin
every { someSetting.flow } returns flowOf(value) // covers .value() reads
coVerify { someSetting.update(any()) } // verifies .value(x) writes
```
`UpgradeRepoGplayTest` uses this shape. Prefer `FakeDataStoreValue` when you need reads and writes to
actually round-trip.
## Reading ViewModel state
`ViewModel2.asLiveState()` is `stateIn(..., initialValue = null).filterNotNull()` with
`SharingStarted.WhileSubscribed(5_000)` — so `vm.state` is a `Flow`, not a `StateFlow`, and has no
`.value` to read. Collect it: `vm.state.first()` is the established pattern across the existing
ViewModel tests. Because the upstream only runs while subscribed, a test that never collects sees
nothing happen at all.