diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index bb689390..6bc78ed5 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -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 | diff --git a/.claude/rules/code-style.md b/.claude/rules/code-style.md new file mode 100644 index 00000000..b629d073 --- /dev/null +++ b/.claude/rules/code-style.md @@ -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. + +**`ScreenHost`** — the only place that touches `hiltViewModel()`, installs the event +handlers, and collects state. + +**`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 diff --git a/.claude/rules/pull-requests.md b/.claude/rules/pull-requests.md index 98c24a9b..7a22e2d3 100644 --- a/.claude/rules/pull-requests.md +++ b/.claude/rules/pull-requests.md @@ -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" diff --git a/.claude/rules/testing.md b/.claude/rules/testing.md index 8733e953..a24eb56b 100644 --- a/.claude/rules/testing.md +++ b/.claude/rules/testing.md @@ -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(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.