Skip to content

Commit eab807f

Browse files
authored
General: Dedupe DataStore reads by porting SD Maid SE's DataStoreValue DSL (#26)
Amply keeps one shared Preferences DataStore, and Preferences DataStore hands the entire snapshot to every collector on any write. None of the 14 preference flows deduplicated, so an unrelated write woke all of them. That was user-visible: ChargeStatsRecorder stamps lastCaptureWallMillis on every recorded sample (~20s while charging, always a new timestamp), which re-emitted captureEnabled unchanged, restarted the flatMapLatest in statsDashboardStates and replayed its loading marker — collapsing the dashboard's charging card to its loading size for a frame, every 20s. Settings are now declared with a createValue() DSL in common/datastore, which dedupes on the raw stored value before the reader runs. The guard lives in the primitive, so a facade cannot forget it, and it never depends on a domain type's equals. Settings read as a unit — a session and its provenance, the recovery target, the interruption event, the alarm config, theme and quick-access state — are now one @serializable record under one key (new dependency: kotlinx-serialization-json), so a partially-written state cannot exist and read-modify-write collapses to update {}. Independent scalars stay separate so a hot-path write doesn't wake unrelated collectors. Corruption semantics are chosen per record rather than globally. Records whose decode was already all-or-nothing keep a whole-record fallback, but ChargingPreferences parses field by field: its four facts degrade independently, and a wholesale fallback would silently downgrade a user's 90% protective baseline to the 80% default and then persist the downgrade. For the same reason every field except the charge policy itself is optional — a missing pid must not take an owed restore down with it. Also fixes a pre-existing torn read in InterruptionAssessor, which read recovery provenance, work id and session through separate suspend calls and could pair fields from either side of a concurrent adopt or clear. No migration: old keys are abandoned and new ones are suffixed .v2. Test users only. An update installed mid-session therefore loses that session's record — wipe app data before installing. Verified: 706 tests pass on both flavors; lintVital clean on all four beta/release variants; R8 release builds clean with no serialization stripping. Both blip regression tests were confirmed against a negative control (removing the dedupe makes them fail: 6 emissions instead of 2, providers recreated 4x instead of 1x), as were the per-field corruption, missing-metadata and alarm-normalization guards.
1 parent d00b9ef commit eab807f

35 files changed

Lines changed: 1505 additions & 417 deletions

.claude/CLAUDE.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ Under `app/src/main/java/eu/darken/amply/`:
4747
- `main/ui` — activity, onboarding, dashboard, settings, setup guide, `tile`, `widget`
4848
- `diagnostics/core` + `diagnostics/ui` — privileged settings comparison and its guided UI
4949
- `common` — shared DataStore owner (`AppDataStore`) and cross-feature primitives
50+
- `common/datastore` — the `createValue()` settings DSL every preference facade is built on (`DataStoreValue`)
51+
- `common/serialization` — the single `Json` plus `ChargePolicySerializer`, for JSON-backed setting records
5052
- `common/theming` — brand, Material You, mode, contrast preferences
5153
- `common/settings` — reusable hierarchical settings rows/sections
5254
- `common/debug/logging` — opt-in debug sessions and logging backends

.claude/rules/architecture.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,10 @@ eu.darken.amply
2727
└── debug/logging Logging fan-out + backends (Logcat, File)
2828
```
2929

30-
Feature-specific preference facades live with their owning feature but share the one `AppDataStore` instance.
30+
Feature-specific preference facades live with their owning feature but share the one `AppDataStore` instance. They
31+
declare their settings with the `createValue()` DSL (`common/datastore`) rather than touching `store.data` — with a
32+
single shared store, every write reaches every collector, so the deduplication has to live in the primitive. See
33+
`code-style.md`.
3134

3235
## Data Flow
3336

.claude/rules/code-style.md

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,29 @@ Debug logs stay **local**: the file logger records only after explicit consent,
106106

107107
- Reactive with Kotlin Flow / StateFlow.
108108
- **Preferences DataStore** via the single `AppDataStore` (`common/AppDataStore.kt`) — `preferencesDataStore(name = "amply")`.
109-
This is the raw AndroidX Preferences API (read/write `Preferences.Key`s through `store.data` / `store.edit`); there
110-
is no custom `createValue()` typed-setting DSL. Feature preference facades wrap `AppDataStore` but all share the one
111-
process-safe instance — do not create a second `DataStore`.
109+
Feature preference facades wrap `AppDataStore` but all share the one process-safe instance — do not create a second
110+
`DataStore`.
111+
- **Never read `store.data` directly in a facade.** Declare settings with the `createValue()` DSL in
112+
`common/datastore/` (ported from SD Maid SE). Because there is exactly one shared store, Preferences DataStore hands
113+
the *entire* snapshot to *every* collector on *any* write — an unrelated key's write would otherwise re-emit every
114+
setting in the app, restarting downstream `flatMapLatest` chains and rebuilding whole UI states. `DataStoreValue`
115+
dedupes on the **raw stored value, before the reader runs**, so the guard cannot be forgotten per-facade and never
116+
depends on a domain type's `equals`. (This is not hypothetical: an undeduplicated `captureEnabled` made the
117+
dashboard's charging card flash its loading state on every stats-recorder tick.)
118+
119+
```kotlin
120+
val captureEnabled = dataStore.createValue("stats.capture_enabled", false) // scalar
121+
val session = dataStore.createValue<ChargeSessionRecord?>("session.v2", null, json, fallbackToDefault = true)
122+
```
123+
124+
Read with `.value()` / collect `.flow`; write with `.value(x)` or `.update { }` (read-modify-write in one
125+
transaction). Settings consumed **as a unit** — a session and its provenance, the alarm config — are one
126+
`@Serializable` record under one key, so a partially-written state cannot exist. Independent scalars stay separate,
127+
so a hot-path write doesn't wake unrelated collectors.
128+
- **Persisted records are a stored wire format.** Give every persisted property and enum constant an explicit
129+
`@SerialName` and a default, and never reuse a key name for a different type (Preferences keys compare by name).
130+
`fallbackToDefault` is chosen **per record**: fine where the decode is already all-or-nothing (an unreadable session
131+
is no session) or cosmetic, but a record whose fields degrade *independently*`ChargingPreferences` — must
132+
validate field by field, or one bad field silently resets a user's real protective baseline to the default.
133+
`StoredRecordFormatTest` pins the JSON.
112134
- Debug builds attach extra logging; the file logger requires explicit user consent before recording.

app/build.gradle.kts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ plugins {
3030
id("projectConfig")
3131
id("com.android.application")
3232
id("org.jetbrains.kotlin.plugin.compose")
33+
id("org.jetbrains.kotlin.plugin.serialization")
3334
id("com.google.devtools.ksp")
3435
id("com.google.dagger.hilt.android")
3536
id("com.android.compose.screenshot")
@@ -193,6 +194,7 @@ tasks.withType<KotlinCompile>().configureEach {
193194
jvmTarget.set(JvmTarget.JVM_17)
194195
freeCompilerArgs.addAll(
195196
"-opt-in=kotlinx.coroutines.ExperimentalCoroutinesApi",
197+
"-opt-in=kotlinx.serialization.ExperimentalSerializationApi",
196198
"-opt-in=androidx.compose.material3.ExperimentalMaterial3Api",
197199
"-Xannotation-default-target=param-property",
198200
)

app/src/main/java/eu/darken/amply/alarm/core/ChargeAlarmStore.kt

Lines changed: 42 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,23 @@
11
package eu.darken.amply.alarm.core
22

3-
import androidx.datastore.preferences.core.Preferences
4-
import androidx.datastore.preferences.core.booleanPreferencesKey
5-
import androidx.datastore.preferences.core.edit
6-
import androidx.datastore.preferences.core.intPreferencesKey
73
import eu.darken.amply.common.AppDataStore
4+
import eu.darken.amply.common.datastore.createValue
5+
import eu.darken.amply.common.datastore.value
86
import kotlinx.coroutines.flow.Flow
9-
import kotlinx.coroutines.flow.first
7+
import kotlinx.coroutines.flow.distinctUntilChanged
108
import kotlinx.coroutines.flow.map
9+
import kotlinx.serialization.SerialName
10+
import kotlinx.serialization.Serializable
11+
import kotlinx.serialization.json.Json
1112
import javax.inject.Inject
1213
import javax.inject.Singleton
1314
import kotlin.math.roundToInt
1415

15-
/** User-facing charge-alarm configuration, derived from a single DataStore snapshot. */
16+
/** User-facing charge-alarm configuration, persisted as one record. */
17+
@Serializable
1618
data class ChargeAlarmConfig(
17-
val enabled: Boolean = false,
18-
val targetPercent: Int = DEFAULT_TARGET_PERCENT,
19+
@SerialName("enabled") val enabled: Boolean = false,
20+
@SerialName("targetPercent") val targetPercent: Int = DEFAULT_TARGET_PERCENT,
1921
) {
2022
companion object {
2123
const val DEFAULT_TARGET_PERCENT = 80
@@ -27,43 +29,56 @@ data class ChargeAlarmConfig(
2729

2830
/**
2931
* DataStore facade for the charge alarm, sharing the single [AppDataStore]. Exposes the config as
30-
* one flow (mapped from a single snapshot, so `enabled` and `targetPercent` never transiently
31-
* disagree) plus the durable "fired this plug cycle" latch that makes the alarm fire at most once
32+
* one record — `enabled` and `targetPercent` live under a single key, so they can never transiently
33+
* disagree plus the durable "fired this plug cycle" latch that makes the alarm fire at most once
3234
* per charge even across process death.
35+
*
36+
* The latch is a separate value on purpose: it is written by the watcher on a completely different
37+
* cadence than the user's config, and folding it into the record would wake every config collector
38+
* each time the alarm fires or resets.
3339
*/
3440
@Singleton
3541
class ChargeAlarmStore @Inject constructor(
36-
private val dataStore: AppDataStore,
42+
dataStore: AppDataStore,
43+
json: Json,
3744
) {
38-
val config: Flow<ChargeAlarmConfig> = dataStore.store.data.map(::toConfig)
45+
private val configValue = dataStore.createValue(
46+
key = "alarm.config.v2",
47+
defaultValue = ChargeAlarmConfig(),
48+
json = json,
49+
fallbackToDefault = true,
50+
)
51+
52+
private val firedCycleValue = dataStore.createValue("alarm.fired_cycle.v2", false)
53+
54+
/**
55+
* Normalized on the way out — the setter snaps too, but a hand-edited or future-written record
56+
* could still carry an off-step target and the UI's stepper assumes a valid tick.
57+
*
58+
* Deduped *again* after normalizing, because snapping is many-to-one: a stored 83 and a
59+
* subsequent explicit 85 are two distinct raw records that both normalize to 85, so the upstream
60+
* raw-value dedupe cannot catch the duplicate.
61+
*/
62+
val config: Flow<ChargeAlarmConfig> = configValue.flow.map(::normalize).distinctUntilChanged()
3963

40-
suspend fun configNow(): ChargeAlarmConfig = config.first()
64+
suspend fun configNow(): ChargeAlarmConfig = normalize(configValue.value())
4165

4266
suspend fun setEnabled(enabled: Boolean) {
43-
dataStore.store.edit { it[ENABLED] = enabled }
67+
configValue.update { it.copy(enabled = enabled) }
4468
}
4569

4670
suspend fun setTargetPercent(percent: Int) {
47-
dataStore.store.edit { it[TARGET_PERCENT] = normalizeTarget(percent) }
71+
configValue.update { it.copy(targetPercent = normalizeTarget(percent)) }
4872
}
4973

5074
/** Whether the alarm has already fired (or been suppressed) for the current plug cycle. */
51-
suspend fun firedCycle(): Boolean = dataStore.store.data.first()[FIRED_CYCLE] ?: false
75+
suspend fun firedCycle(): Boolean = firedCycleValue.value()
5276

5377
suspend fun setFiredCycle(fired: Boolean) {
54-
dataStore.store.edit { it[FIRED_CYCLE] = fired }
78+
firedCycleValue.value(fired)
5579
}
5680

57-
private fun toConfig(prefs: Preferences) = ChargeAlarmConfig(
58-
enabled = prefs[ENABLED] ?: false,
59-
targetPercent = normalizeTarget(prefs[TARGET_PERCENT] ?: ChargeAlarmConfig.DEFAULT_TARGET_PERCENT),
60-
)
61-
62-
private companion object {
63-
val ENABLED = booleanPreferencesKey("alarm.enabled")
64-
val TARGET_PERCENT = intPreferencesKey("alarm.target_percent")
65-
val FIRED_CYCLE = booleanPreferencesKey("alarm.fired_cycle")
66-
}
81+
private fun normalize(raw: ChargeAlarmConfig) = raw.copy(targetPercent = normalizeTarget(raw.targetPercent))
6782
}
6883

6984
/** Snap to the nearest [ChargeAlarmConfig.TARGET_STEP] and clamp to the allowed range. */
Lines changed: 99 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,69 +1,134 @@
11
package eu.darken.amply.charging.core
22

3-
import androidx.datastore.preferences.core.edit
4-
import androidx.datastore.preferences.core.longPreferencesKey
53
import androidx.datastore.preferences.core.stringPreferencesKey
64
import eu.darken.amply.common.AppDataStore
5+
import eu.darken.amply.common.datastore.createValue
6+
import eu.darken.amply.common.datastore.value
77
import kotlinx.coroutines.flow.Flow
8-
import kotlinx.coroutines.flow.first
8+
import kotlinx.coroutines.flow.distinctUntilChanged
99
import kotlinx.coroutines.flow.map
10+
import kotlinx.serialization.SerialName
11+
import kotlinx.serialization.Serializable
12+
import kotlinx.serialization.json.Json
13+
import kotlinx.serialization.json.JsonNull
14+
import kotlinx.serialization.json.JsonObject
15+
import kotlinx.serialization.json.JsonPrimitive
1016
import javax.inject.Inject
1117
import javax.inject.Singleton
1218

19+
/**
20+
* The stored form of Amply's policy bookkeeping.
21+
*
22+
* Policies are held as **raw [ChargePolicy.stableId] strings**, not as decoded `ChargePolicy`s,
23+
* because these four facts degrade *independently*: an unreadable `lastRequested` must still leave a
24+
* perfectly good `protective` baseline intact. Losing that baseline is not cosmetic — it is the
25+
* limit Amply restores the battery to, so a wholesale fallback would quietly downgrade a user's
26+
* Adaptive or 90 % choice to the 80 % default, and the next write would persist the downgrade.
27+
*
28+
* Which is why decoding goes through [decodePolicyState] field by field instead of
29+
* `decodeFromString`: a typed whole-record decode fails on the *first* bad field and takes the other
30+
* three with it, so `{"lastRequestedAt":"bad", "protective":"fixed:90"}` would lose a valid 90 %.
31+
*/
32+
@Serializable
33+
internal data class PolicyState(
34+
@SerialName("lastRequested") val lastRequested: String? = null,
35+
@SerialName("lastRequestedAt") val lastRequestedAt: Long = 0L,
36+
@SerialName("protective") val protective: String? = null,
37+
@SerialName("lastPersistent") val lastPersistent: String? = null,
38+
)
39+
40+
/**
41+
* Reads each field on its own terms. A field that is absent, the wrong JSON type, or an unreadable
42+
* policy id yields only *that* field's default; only unparseable JSON loses the whole record.
43+
*/
44+
internal fun decodePolicyState(raw: String?, json: Json): PolicyState {
45+
if (raw == null) return PolicyState()
46+
val obj = runCatching { json.parseToJsonElement(raw) as? JsonObject }.getOrNull() ?: return PolicyState()
47+
return PolicyState(
48+
lastRequested = obj.stringOrNull("lastRequested"),
49+
lastRequestedAt = obj.longOrDefault("lastRequestedAt"),
50+
protective = obj.stringOrNull("protective"),
51+
lastPersistent = obj.stringOrNull("lastPersistent"),
52+
)
53+
}
54+
55+
private fun JsonObject.primitiveOrNull(name: String): JsonPrimitive? =
56+
(this[name] as? JsonPrimitive)?.takeUnless { it is JsonNull }
57+
58+
private fun JsonObject.stringOrNull(name: String): String? = primitiveOrNull(name)?.takeIf { it.isString }?.content
59+
60+
private fun JsonObject.longOrDefault(name: String, default: Long = 0L): Long =
61+
primitiveOrNull(name)?.takeUnless { it.isString }?.content?.toLongOrNull() ?: default
62+
1363
@Singleton
1464
class ChargingPreferences @Inject constructor(
15-
private val dataStore: AppDataStore,
65+
dataStore: AppDataStore,
66+
json: Json,
1667
) {
17-
val lastRequested: Flow<ChargePolicy?> = dataStore.store.data.map {
18-
ChargePolicy.fromStableId(it[LAST_REQUESTED])
19-
}
68+
private val policyState = dataStore.createValue(
69+
key = stringPreferencesKey("policy.v2"),
70+
reader = { raw -> decodePolicyState(raw as? String, json) },
71+
writer = { state -> json.encodeToString(PolicyState.serializer(), state) },
72+
)
73+
74+
// Each projection dedupes on its own: they all ride one record now, so without this a
75+
// lastRequestedAt-only write would re-emit every one of them.
76+
val lastRequested: Flow<ChargePolicy?> = policyState.flow
77+
.map { ChargePolicy.fromStableId(it.lastRequested) }
78+
.distinctUntilChanged()
2079

2180
/** Wall-clock time of the last request; paired atomically with [lastRequested]. 0 = never requested. */
22-
val lastRequestedAt: Flow<Long> = dataStore.store.data.map { it[LAST_REQUESTED_AT] ?: 0L }
81+
val lastRequestedAt: Flow<Long> = policyState.flow
82+
.map { it.lastRequestedAt }
83+
.distinctUntilChanged()
2384

24-
val protectivePolicy: Flow<ChargePolicy> = dataStore.store.data.map {
25-
ChargePolicy.fromStableId(it[PROTECTIVE_POLICY])?.takeUnless { policy ->
26-
policy == ChargePolicy.Unrestricted
27-
} ?: ChargePolicy.FixedLimit(80)
28-
}
85+
val protectivePolicy: Flow<ChargePolicy> = policyState.flow
86+
.map { it.protectivePolicy() }
87+
.distinctUntilChanged()
2988

3089
/**
3190
* The last policy Amply successfully applied as a *persistent* configuration — including
3291
* [ChargePolicy.Unrestricted], unlike [protectivePolicy]. Temporary session overrides never
3392
* update this, so it answers "what did the user configure through Amply" without a session's
3493
* transient Unrestricted write polluting the answer. Null until Amply's first persistent write.
3594
*/
36-
val lastPersistentPolicy: Flow<ChargePolicy?> = dataStore.store.data.map {
37-
ChargePolicy.fromStableId(it[LAST_PERSISTENT_POLICY])
38-
}
95+
val lastPersistentPolicy: Flow<ChargePolicy?> = policyState.flow
96+
.map { ChargePolicy.fromStableId(it.lastPersistent) }
97+
.distinctUntilChanged()
3998

4099
suspend fun recordRequested(
41100
policy: ChargePolicy,
42101
persistent: Boolean,
43102
nowMillis: Long = System.currentTimeMillis(),
44103
) {
45-
dataStore.store.edit {
46-
it[LAST_REQUESTED] = policy.stableId
47-
it[LAST_REQUESTED_AT] = nowMillis
48-
if (persistent) {
49-
it[LAST_PERSISTENT_POLICY] = policy.stableId
50-
if (policy != ChargePolicy.Unrestricted) it[PROTECTIVE_POLICY] = policy.stableId
51-
}
104+
policyState.update { current ->
105+
current.copy(
106+
lastRequested = policy.stableId,
107+
lastRequestedAt = nowMillis,
108+
lastPersistent = if (persistent) policy.stableId else current.lastPersistent,
109+
protective = if (persistent && policy != ChargePolicy.Unrestricted) {
110+
policy.stableId
111+
} else {
112+
current.protective
113+
},
114+
)
52115
}
53116
}
54117

55-
suspend fun lastRequestedNow(): ChargePolicy? = lastRequested.first()
118+
suspend fun lastRequestedNow(): ChargePolicy? = ChargePolicy.fromStableId(policyState.value().lastRequested)
56119

57-
suspend fun lastRequestedAtNow(): Long = lastRequestedAt.first()
120+
suspend fun lastRequestedAtNow(): Long = policyState.value().lastRequestedAt
58121

59-
suspend fun protectivePolicyNow(): ChargePolicy = protectivePolicy.first()
122+
suspend fun protectivePolicyNow(): ChargePolicy = policyState.value().protectivePolicy()
60123

61-
suspend fun lastPersistentPolicyNow(): ChargePolicy? = lastPersistentPolicy.first()
62-
63-
private companion object {
64-
val LAST_REQUESTED = stringPreferencesKey("policy.last_requested")
65-
val LAST_REQUESTED_AT = longPreferencesKey("policy.last_requested_at")
66-
val PROTECTIVE_POLICY = stringPreferencesKey("policy.protective")
67-
val LAST_PERSISTENT_POLICY = stringPreferencesKey("policy.last_persistent")
68-
}
124+
suspend fun lastPersistentPolicyNow(): ChargePolicy? =
125+
ChargePolicy.fromStableId(policyState.value().lastPersistent)
69126
}
127+
128+
/**
129+
* Unrestricted is never a protective baseline, and neither is an unreadable value — both fall back to
130+
* the 80 % limit rather than leaving the battery uncapped.
131+
*/
132+
private fun PolicyState.protectivePolicy(): ChargePolicy =
133+
ChargePolicy.fromStableId(protective)?.takeUnless { it == ChargePolicy.Unrestricted }
134+
?: ChargePolicy.FixedLimit(80)

app/src/main/java/eu/darken/amply/charging/core/access/AutoWssGrantCoordinator.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ class AutoWssGrantCoordinator @Inject constructor(
4949
if (!started.compareAndSet(false, true)) return
5050
scope.launch {
5151
observeAutoWssGrant(
52-
inputs = autoWssGrantInputs(repository.state, onboardingSettings.isComplete),
52+
inputs = autoWssGrantInputs(repository.state, onboardingSettings.isComplete.flow),
5353
grantScope = scope,
5454
) {
5555
// The repository single-flights this call, so a concurrent manual tap or an overlapping

0 commit comments

Comments
 (0)