fix(android): make the sync chip's state decision resolvable without an Application - #859
Conversation
…an Application
`SyncChipStateTest.lastSyncedAt_takesPriorityOverHistorySyncExperimental` fails on
main — the only failure in the suite (3060 tests, 1 failure):
java.lang.IllegalStateException: NoopApplication is not attached
at com.noop.NoopApplication$Companion.localizedString(NoopApplication.kt:134)
at com.noop.ui.UiStringsKt.uiString(UiStrings.kt:16)
at com.noop.ui.TodayScoringKt.shortSyncAgo(TodayScoring.kt:412)
at com.noop.ui.SyncChipState$Companion.resolve(TodayScoring.kt:399)
`SyncChipState.resolve` is documented "Pure + unit-tested" but reached for two
ambient dependencies: the process-wide Application's resources (for the translated
"now" word) and the system clock. These are plain JVM tests with no Robolectric, so
no Application is ever attached and the resource lookup throws.
Only the `< 60s` branch of `shortSyncAgo` needs a word — every other bucket is
digits plus a unit letter — which is why five of the six cases passed and the sixth,
the one with a 5-second-old sync, did not. Same reason `lastSyncedAt_isSyncedWith-
AgeText` survives at 65 seconds.
Hoist both dependencies to parameters. `SyncStatusChip` — a composable that already
depends on the clock and the string catalog — resolves them and hands them down, so
the decision itself becomes a genuinely pure function. Same injected-clock style as
`recordingStateFor` directly above it, which already takes its own `nowSec`.
No behaviour change: the clock is still read at composition time, exactly as
`shortSyncAgo` did on every recomposition, and the chip renders identical text.
Tests are now deterministic (fixed `nowSec` rather than `System.currentTimeMillis()`)
and gain a case for the sub-minute branch, which was previously impossible to reach
from a unit test — it is the regression guard against the lookup moving back inside.
The Swift twin is deliberately unchanged. Its `shortAgo` resolves its own clock and
`String(localized:)` and is fine doing so: XCTest runs against a real bundle, and
`SyncChipStateTests` passes. The two signatures now differ on purpose; the priority
order they encode does not. Noted in the KDoc so it does not get "fixed" back.
Verified locally: 3061 tests, 0 failures (was 3060 / 1), 381 test classes both
before and after. i18n-coverage and source-hygiene gates both green. android.yml is
disabled by default, which is why CI never caught this.
ryanbr
left a comment
There was a problem hiding this comment.
Verified the diagnosis and the fix independently. Approving.
-
Mechanism confirmed.
NoopApplication.localizedStringischeckNotNull(instance) { "NoopApplication is not attached" }, andinstanceis only set inonCreate, which never runs under a plain JVM test. -
The "only 1 failure" claim holds. Traced all six cases:
backfilling_takesPriorityOverLastSyncedAtalso usesnow - 5but short-circuits toSyncingbeforeshortSyncAgois called, andisSyncedWithAgeTextat 65s lands in theNmbranch. Exactly one case reaches the< 60sbranch without short-circuiting, which is the one that fails. -
No stragglers. All eight
resolvecall sites pass all six named args; the onlyshortSyncAgoreference is the single internal call. Since nothing compiles Android in CI, this was the thing most worth checking. -
Dropping
import com.noop.Ris safe — zero remainingR.oruiStringreferences in TodayScoring.kt. Nice side effect: that file is now framework-free, only analytics/data/java.time. The call site hasRimported anduiStringis same-package. -
Ran it. Can't use Gradle here (aarch64, and
aapt2is x86-64 only), so I extracted the implementation verbatim, compiled it with the Kotlin compiler from the Gradle cache, and executed all seven cases — green. -
Parity is intact, not just asserted. Swift computes
mins = secs/60,hrs = mins/60,days = hrs/24; Kotlin uses 3600/86400 directly. Floor division composes, so they agree — checked across 255 offsets including every bucket boundary, zero mismatches. Priority order and themax(0, …)/coerceAtLeast(0)clamp match too. Only the signature differs, which is what CLAUDE.md's "or explicitly call out why not" is for, and the KDoc says so. -
Arithmetic is unchanged — the composable passes
System.currentTimeMillis() / 1000L, which is whatshortSyncAgoread itself. -
i18n-coverageandsource-hygienepass locally;l10n_today_screen_sync_chip_now_c9bc849ais still referenced and still translated in all five locales.
One correction to "no behaviour change": nowLabel is now resolved on every recomposition, including when the chip is Hidden or Syncing and the word is never used. Before, the lookup only happened in the < 60s branch. It is a getString call so this is not a perf concern, and there is no @Preview anywhere in com.noop.ui that an unconditional resolve could break — but the statement is not quite exact. Not worth changing; a () -> String would be over-engineering.
Nit, non-blocking: inside a composable stringResource is the more idiomatic lookup than uiString, since it tracks configuration changes rather than going through the process Application. The rest of SyncStatusChip already uses uiString, so staying consistent with the file is defensible.
Not verified by me: the full 3061-test suite, and that TodayScreen.kt compiles as a whole — both blocked by the same arm64 aapt2 wall.
On the last point in your description: agreed, and worth doing. testFullDebugUnitTest is pure JVM, and this bug shipped one day ago in #806 and would have been caught on the PR.
…ker instrumentation (ryanbr#52) On some iOS 26 builds the system folder picker's Open button never fires, so users can't select any folder (external) — three reports, works for one. Two prior blind picker fixes (ryanbr#859, #1000a) shipped and didn't resolve it. Fix, two parts: - Fallback: add "Use NOOP's own folder" — backs up inside NOOP's own Documents/Backups, already exposed in Files (UIFileSharingEnabled + LSSupportsOpeningDocumentsInPlace) under On My iPhone -> NOOP, with zero dependence on the picker. resolveFolder() short-circuits to it (no security-scoped bookmark; the existing scoped brackets no-op), so backup / restore / prune / auto-catchup all work unchanged. An explicit external pick turns it back off. The no-folder alert now points users at it. - Instrumentation: record the last picker delegate outcome (picked / cancelled) and, on a returned URL, whether scoped-access opened and the bookmark minted, surfaced in the debug export. One on-device export now tells us whether the picker's Open never fired (iOS-side) vs a returned folder we failed to persist (our bug) — instead of guessing.
…ker instrumentation (ryanbr#52) On some iOS 26 builds the system folder picker's Open button never fires, so users can't select any folder (external) — three reports, works for one. Two prior blind picker fixes (ryanbr#859, #1000a) shipped and didn't resolve it. Fix, two parts: - Fallback: add "Use NOOP's own folder" — backs up inside NOOP's own Documents/Backups, already exposed in Files (UIFileSharingEnabled + LSSupportsOpeningDocumentsInPlace) under On My iPhone -> NOOP, with zero dependence on the picker. resolveFolder() short-circuits to it (no security-scoped bookmark; the existing scoped brackets no-op), so backup / restore / prune / auto-catchup all work unchanged. An explicit external pick turns it back off. The no-folder alert now points users at it. - Instrumentation: record the last picker delegate outcome (picked / cancelled) and, on a returned URL, whether scoped-access opened and the bookmark minted, surfaced in the debug export. One on-device export now tells us whether the picker's Open never fired (iOS-side) vs a returned folder we failed to persist (our bug) — instead of guessing.
…an Application (ryanbr#859) SyncChipStateTest.lastSyncedAt_takesPriorityOverHistorySyncExperimental fails on main — the only failure in the suite, 3060 tests, 1 failure — with: java.lang.IllegalStateException: NoopApplication is not attached at com.noop.NoopApplication$Companion.localizedString(NoopApplication.kt:134) at com.noop.ui.UiStringsKt.uiString(UiStrings.kt:16) at com.noop.ui.TodayScoringKt.shortSyncAgo(TodayScoring.kt:412) at com.noop.ui.SyncChipState$Companion.resolve(TodayScoring.kt:399) ROOT CAUSE. SyncChipState.resolve is documented "Pure + unit-tested" but reached for two ambient dependencies: the process-wide Application resources (shortSyncAgo -> uiString -> NoopApplication.localizedString, which is checkNotNull(instance) and throws because onCreate never runs) and the system clock. These are plain JVM tests with no Robolectric, so no Application is ever attached. What made it look arbitrary: only the < 60s branch of shortSyncAgo needs a translated word, every other bucket being digits plus a unit letter. Tracing all six cases confirms exactly one reaches that branch without short-circuiting — backfilling_takesPriorityOverLastSyncedAt also uses now - 5 but returns Syncing first, and isSyncedWithAgeText at 65s lands in the Nm branch. FIX. Hoist both dependencies to parameters. SyncStatusChip — a composable that already depends on the clock and the string catalog — resolves them and passes them down, so the decision becomes genuinely pure. Same injected-clock shape as recordingStateFor directly above it. Dropping the now unused com.noop.R import leaves TodayScoring.kt framework-free: analytics, data and java.time only. The tests become deterministic (a fixed nowSec rather than System.currentTimeMillis()) and gain a case for the sub-minute branch, previously unreachable from a unit test and now the regression guard against the lookup migrating back inside. PARITY. The Swift twin is deliberately unchanged, called out per CLAUDE.md. Swift computes mins = secs/60, hrs = mins/60, days = hrs/24 where Kotlin uses 3600/86400 directly; floor division composes, so the two agree across every bucket boundary — verified over 255 offsets with zero mismatches, along with the max(0,) / coerceAtLeast(0) clamp and the priority order. Only the SIGNATURES differ, and a KDoc note says why so it does not get "restored" later. No behaviour change in what renders. One honest exception: nowLabel is now resolved on every recomposition rather than only in the < 60s branch. It is a getString call, and there is no @Preview anywhere in com.noop.ui that an unconditional resolve could break. VERIFICATION. Gradle cannot run on the reviewing host (aarch64; aapt2 ships x86-64 only), so the implementation was extracted verbatim, compiled with the Kotlin compiler from the Gradle cache and executed: all seven cases green. All eight resolve call sites carry all six named args and the sole shortSyncAgo call is updated — worth checking directly, since android.yml is disabled and nothing compiles Kotlin on a PR. Current main has exactly one production call site, so no semantic conflict. i18n-coverage and source-hygiene pass; l10n_today_screen_sync_chip_now_c9bc849a is still referenced, from TodayScreen.kt now, and still translated in all five locales. Regression dates to ryanbr#806, merged one day earlier. testFullDebugUnitTest is pure JVM with no emulator and would have caught it on the PR; enabling that job alone, while the heavier compile job stays off, is worth a separate change.
Follow-up to the note I left in review on #738 — kept separate so an unrelated UI test fix doesn't ride along with a sleep-staging change.
The failure
SyncChipStateTest.lastSyncedAt_takesPriorityOverHistorySyncExperimentalfails onmain(verified at cca23d6 on a clean checkout). It is the only failure in the suite — 3060 tests, 1 failure.Repro:
Root cause
SyncChipState.resolveis documentedPure + unit-tested, but it reached for two ambient dependencies: the process-wide Application's resources (viashortSyncAgo→uiString→NoopApplication.localizedString) and the system clock. These are plain JVM tests with no Robolectric, so no Application is ever attached and the resource lookup throws.What made it look arbitrary: only the
< 60sbranch ofshortSyncAgoneeds a translated word — every other bucket is digits plus a unit letter. So five of the six cases passed, and the one with a 5-second-old sync did not. It is also whylastSyncedAt_isSyncedWithAgeTextsurvives: it uses 65 seconds and lands in theNmbranch.The fix
Hoist both dependencies to parameters.
SyncStatusChip— a composable that already depends on the clock and the string catalog — resolves them and passes them down, so the decision itself becomes genuinely pure. This is the same injected-clock shape asrecordingStateFordirectly above it, which already takes its ownnowSec.No behaviour change. The clock is still read at composition time, exactly as
shortSyncAgodid on every recomposition, and the chip renders identical text in every bucket.The tests become deterministic (a fixed
nowSecinstead ofSystem.currentTimeMillis()) and gain one case for the sub-minute branch — previously impossible to reach from a unit test, and now the regression guard against the lookup migrating back inside.On the parity contract
The Swift twin is deliberately unchanged, calling that out per CLAUDE.md.
SyncChipState.shortAgoresolves its ownDate()andString(localized:)and is fine doing so — XCTest runs against a real bundle, andSyncChipStateTestspasses. The two signatures now differ on purpose; the four cases and the priority order they encode do not. There is a KDoc note saying so, so it doesn't get "restored" later.Verification
Measured on this machine by running the full suite against a pristine
upstream/maintree, then against the patch. The+1is the new sub-minute case; no test was dropped.i18n-coverageandsource-hygiene(the two workflows that actually run) are both green locally:The
l10n_today_screen_sync_chip_now_c9bc849acatalog entry is still referenced, just fromTodayScreen.ktnow.Why CI didn't catch it
android.ymlis disabled by default, so nothing runs the Android unit tests on a PR. Worth a separate conversation about whether thetestFullDebugUnitTestjob is cheap enough to enable on its own (it is pure JVM — ~2 min here, no emulator, no SDK-heavy steps) even while the full compile job stays off.