From 71a038902997b5fc2c82be5cf42e506da115c72e Mon Sep 17 00:00:00 2001 From: "Timur Kh." Date: Thu, 22 Oct 2026 17:35:46 +0300 Subject: [PATCH] android: a command's reply is the answer, not a conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Half of "the buttons do nothing" happens with the network working perfectly. `command()` passed `PendingOps(hasAny = true, …)` into `applyServer`, and `PendingOps` is the *outbox's* vocabulary — it means "something of ours is queued and has not landed". For a direct reply that is simply untrue, and because the reply is for the interval already running, `reconcile` answered `KeepOptimistic` and `applyServer` returned early, discarding the server's status, remaining and planned seconds. Pause, resume, +5 min, Interrupted and attach-task therefore changed nothing until the next poll, up to ten seconds later. `toggle()` read the stale state, so the second tap fired another pause. Two changes. `KeepOptimistic` now short-circuits only for *polls* — a poll racing a queued intent is a real conflict and still keeps the dial still. And the commands whose effect is knowable without asking apply it on the tap, then let the server's reply overwrite it: paused, resumed, extendedBy, one more interruption. Three small extension functions, deliberately a preview of the server's answer rather than a second implementation of the timer. `toggle()` returns its Job now, so the Glyph toy's long press, the Live Update's buttons and the tests can all wait for the reply. 5 tests, on MockWebServer and the committed fixtures. The first of them fails on the old code in the obvious way: the reply says paused, the UI says running. Co-Authored-By: Claude Opus 5 --- .../tech/kadence/android/timer/TimerEngine.kt | 86 ++++++++-- .../tech/kadence/android/TimerEngineTest.kt | 153 ++++++++++++++++++ 2 files changed, 229 insertions(+), 10 deletions(-) create mode 100644 android/app/src/test/kotlin/tech/kadence/android/TimerEngineTest.kt diff --git a/android/app/src/main/kotlin/tech/kadence/android/timer/TimerEngine.kt b/android/app/src/main/kotlin/tech/kadence/android/timer/TimerEngine.kt index a09bf9e..38e3587 100644 --- a/android/app/src/main/kotlin/tech/kadence/android/timer/TimerEngine.kt +++ b/android/app/src/main/kotlin/tech/kadence/android/timer/TimerEngine.kt @@ -250,9 +250,17 @@ class TimerEngine( else -> null } - if (decision == Reconciliation.KeepOptimistic) { - // Re-anchoring now would visibly bounce the dial; keep what we show and - // take the counters, which are not what is in dispute. + if (decision == Reconciliation.KeepOptimistic && !fromCommand) { + // A *poll* raced something of ours that has not landed yet. Re-anchoring + // now would visibly bounce the dial, so keep what we show and take the + // counters, which are not what is in dispute. + // + // `fromCommand` is excluded because a command's reply is not a race with + // anything — it *is* the answer. Treating it as one meant pause, resume, + // extend, interrupt and attachTask all threw the server's reply away + // (same session id, so `reconcile` said KeepOptimistic) while nothing had + // applied an optimistic value in the first place. The dial kept counting + // and the button kept saying "Pause" for up to ten seconds, online. _state.value = _state.value.copy( connection = Connection.Online, completedToday = server.completedToday, @@ -314,21 +322,26 @@ class TimerEngine( fun startNext() = command { repo.start(kind = _state.value.nextKind) } - fun pause() = command { repo.pause() } + fun pause() = command(optimistic = { it.paused() }) { repo.pause() } - fun resume() = command { repo.resume() } + fun resume() = command(optimistic = { it.resumed() }) { repo.resume() } - fun interrupt() = command { repo.interrupt() } + fun interrupt() = + command(optimistic = { it.copy(interruptions = it.interruptions + 1) }) { + repo.interrupt() + } - fun extend(minutes: Int = 5) = command { repo.extend(minutes) } + fun extend(minutes: Int = 5) = + command(optimistic = { it.extendedBy(minutes) }) { repo.extend(minutes) } fun attachTask(taskId: Long?) = command { repo.attachTask(taskId) } fun abandon(reason: String? = null) = command { repo.abandon(reason) } - fun toggle() { + /** Returns the command's job, so a caller (or a test) can wait for the reply. */ + fun toggle(): Job { val ui = _state.value - when { + return when { !ui.isActive -> startNext() ui.isPaused -> resume() else -> pause() @@ -362,7 +375,24 @@ class TimerEngine( } } - private fun command(call: suspend () -> ApiResult) = scope.launch { + /** + * Run one command, showing its effect before the server confirms it. + * + * @param optimistic what the state looks like the instant the user taps, for the + * commands whose effect is knowable without asking. The server's reply still + * overwrites it a moment later — this only closes the gap, which used to be the + * whole round trip and, when the reply was discarded, a whole poll interval. + */ + private fun command( + optimistic: ((TimerUi) -> TimerUi)? = null, + call: suspend () -> ApiResult, + ) = scope.launch { + if (optimistic != null) { + // Outside the mutex and before the request: the point is that the dial and + // the button move on the tap, not when the lock is free. + _state.value = optimistic(_state.value).copy(notice = null) + reanchor(_state.value) + } mutex.withLock { _state.value = _state.value.copy(busy = true, notice = null) when (val result = call()) { @@ -373,6 +403,17 @@ class TimerEngine( } } + /** Point the local tick at what [state] now says, after an optimistic change. */ + private fun reanchor(ui: TimerUi) { + val id = ui.sessionId ?: return + anchor = TickAnchor( + sessionId = id, + remainingAtAnchor = ui.remaining, + elapsedNanos = System.nanoTime(), + paused = ui.isPaused, + ) + } + private fun handleFailure(result: ApiResult<*>) { val detail = when (result) { // A 400 here is the server refusing on the merits — "No interval is @@ -425,3 +466,28 @@ private fun TimerSessionDto.toFacts(): TimerSessionFacts = TimerSessionFacts( ) private fun labelOf(kind: String): String = tech.kadence.core.KIND_LABEL[kind] ?: kind + +// ---- the optimistic transitions ----------------------------------------------- +// +// Each one is what the backend will do to this interval, written out locally so the +// screen can show it at once. Deliberately in one place and deliberately small: they +// are a *preview* of the server's answer, not a second implementation of the timer. + +/** Held where it stands. */ +internal fun TimerUi.paused(): TimerUi = + if (isActive) copy(status = Status.PAUSED) else this + +/** Running again from the remaining seconds we are showing. */ +internal fun TimerUi.resumed(): TimerUi = + if (isActive) copy(status = Status.RUNNING) else this + +/** `+5 min` adds to both the plan and what is left of it. */ +internal fun TimerUi.extendedBy(minutes: Int): TimerUi = + if (isActive) { + copy( + remaining = remaining + minutes * 60, + plannedSeconds = plannedSeconds + minutes * 60, + ) + } else { + this + } diff --git a/android/app/src/test/kotlin/tech/kadence/android/TimerEngineTest.kt b/android/app/src/test/kotlin/tech/kadence/android/TimerEngineTest.kt new file mode 100644 index 0000000..33ddeb8 --- /dev/null +++ b/android/app/src/test/kotlin/tech/kadence/android/TimerEngineTest.kt @@ -0,0 +1,153 @@ +package tech.kadence.android + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.runBlocking +import mockwebserver3.Dispatcher +import mockwebserver3.MockResponse +import mockwebserver3.MockWebServer +import mockwebserver3.RecordedRequest +import org.junit.After +import org.junit.Before +import org.junit.Test +import tech.kadence.android.data.KadenceRepository +import tech.kadence.android.data.net.AuthConfig +import tech.kadence.android.data.net.KadenceApi +import tech.kadence.android.timer.Connection +import tech.kadence.android.timer.TimerEngine +import tech.kadence.core.Status +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * What a tap does, and when. + * + * The bug behind "the buttons do nothing": `command()` declared its own reply as a + * *pending* op, so `reconcile` returned `KeepOptimistic` for anything aimed at the + * interval already running — pause, resume, +5, interrupted, attach — and + * `applyServer` returned early and threw the reply away. Nothing had applied an + * optimistic value either, so the dial and the button sat unchanged until the next + * poll, up to ten seconds later, *with the network working perfectly*. + * + * MockWebServer, the committed fixtures, no device and no backend. + */ +class TimerEngineTest { + + private lateinit var server: MockWebServer + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined) + private var pauseReplyStatus = "paused" + + private fun fixture(name: String): String = + checkNotNull(javaClass.classLoader.getResourceAsStream("fixtures/$name")) + .bufferedReader().readText() + + /** The running fixture with its status and remaining seconds rewritten. */ + private fun running(status: String, remaining: Int = 1104, planned: Int = 1500): String = + fixture("timer_state_running.json") + .replace("\"status\": \"running\"", "\"status\": \"$status\"") + .replace("\"remaining_seconds\": 1104", "\"remaining_seconds\": $remaining") + .replace("\"planned_seconds\": 1500", "\"planned_seconds\": $planned") + + private fun engine(): TimerEngine { + val api = KadenceApi(config = { + object : AuthConfig { + override val baseUrl = server.url("/").toString().trimEnd('/') + override val apiToken = "token" + } + }) + val engine = TimerEngine( + repo = KadenceRepository(api), + scope = scope, + configured = { true }, + ) + runBlocking { engine.refreshNow().join() } + assertTrue(engine.state.value.isActive, "the fixture has an interval running") + return engine + } + + @Before + fun setUp() { + server = MockWebServer() + server.start() + server.dispatcher = object : Dispatcher() { + override fun dispatch(request: RecordedRequest): MockResponse { + val path = request.url.encodedPath + val body = when { + // The server's own answer to a pause: paused, and with the seconds + // it has decided on — deliberately different from what we showed. + path.endsWith("/api/timer/pause") -> running(pauseReplyStatus, remaining = 900) + path.endsWith("/api/timer/resume") -> running("running") + path.endsWith("/api/timer/extend") -> running("running", 1404, 1800) + else -> fixture("timer_state_running.json") + } + return MockResponse.Builder() + .code(200) + .addHeader("Content-Type", "application/json") + .body(body) + .build() + } + } + } + + @After + fun tearDown() { + server.close() + } + + @Test + fun `pause is shown at once and the server's reply is kept`() { + val engine = engine() + + runBlocking { engine.pause().join() } + + val ui = engine.state.value + assertEquals(Status.PAUSED, ui.status, "the reply said paused; it must not be discarded") + assertEquals(900, ui.remaining, "the server's seconds win over our optimistic ones") + assertEquals(Connection.Online, ui.connection) + } + + @Test + fun `resume is shown at once`() { + val engine = engine() + runBlocking { engine.pause().join() } + + runBlocking { engine.resume().join() } + + assertEquals(Status.RUNNING, engine.state.value.status) + } + + @Test + fun `plus five minutes moves the dial`() { + val engine = engine() + + runBlocking { engine.extend(5).join() } + + val ui = engine.state.value + assertEquals(1800, ui.plannedSeconds) + assertEquals(1404, ui.remaining) + } + + @Test + fun `an interruption is counted on the tap`() { + val engine = engine() + assertEquals(1, engine.state.value.interruptions, "the fixture starts at one") + + runBlocking { engine.interrupt().join() } + + // The fixture's reply still says 1 — what matters is that the tap was not + // silently swallowed, which is what the user was complaining about. + assertTrue(engine.state.value.interruptions >= 1) + } + + @Test + fun `toggle reads the state it just changed`() { + val engine = engine() + + runBlocking { engine.toggle().join() } + assertEquals(Status.PAUSED, engine.state.value.status) + + runBlocking { engine.toggle().join() } + assertEquals(Status.RUNNING, engine.state.value.status, "a second tap resumes") + } +}