MOB-1740: App Theme in Settings with Pure Black (OLED) dark look

[nesence-m]

What

Final MOB-1740 UX per the Q3 designs (Figma node 7011-6045):

• App Theme is now a top-level Settings entry (palette icon, between Beta: Coinholder Polling and Advanced Settings) instead of living in Advanced Settings.
• The Theme screen matches the final design: logo + palette header, back-arrow top bar, System / Light / Dark option cards, footer Save changes. The OLED checkbox is gone.
• Tapping System or Dark opens a new "When your device is dark" bottom sheet picking Classic Dark vs Pure Black; its Save changes persists the appearance + dark look and pops back to the Settings list. Tapping Light selects it locally and the screen's own Save changes persists it.
• Pure Black keeps the OLED palette work from the earlier commits (OledZashiColors, forced -night resource resolution, themed in-app browser chrome).

How

• ThemeSettingsVM/View reworked (no checkbox; System/Dark forward to the sheet route); new screen/theme/darklook/ package with ThemeDarkLookArgs/State/VM/View registered as dialogComposable, mirroring the SwapSlippage sheet pattern.
• SetAppearanceModeUseCase now ends with navigationRouter.backTo(MoreArgs::class).
• Strings updated to the final Zodl copy (EN + ES); new ic_settings_theme palette drawable.

Testing

• ThemeSettingsVMTest (7) + new ThemeDarkLookVMTest (5) — all green.
• ktlint clean; detekt has only the two pre-existing findings on the branch (verified unrelated via stash).
• Manually verified on an emulator: entry position/icon/copy, Dark → sheet → Pure Black → Save lands on Settings with pure black applied, persistence on re-entry, Light path.

[linear]

MOB-1740

MOB-1724

[LukasKorba]

@zodl-review

[zodl-review]

🤖 Claude Review

Coverage

Full diff and file contents reviewed.

Summary

Adds a top-level "App Theme" settings entry (System/Light/Dark) with a "When your device is dark" bottom sheet for Classic Dark vs Pure Black (OLED) selection. The OLED palette work is wired into an AppearanceMode + isOledEnabled preference system that propagates through ZcashTheme, the navigation bar chrome, and the Custom Tabs browser. The ContextThemeWrapper injection in ZcashTheme is a structural change with a detailed comment explaining why it diverges from the simpler ConfigurationOverride path.

Risk Assessment

Medium — theme state is read at every recomposition root; the ContextThemeWrapper injection affects resource resolution (drawables, night qualifiers) app-wide. A regression here would be broad.

---

Key Observations

Correctness / Bugs

Save button always enabled in ThemeDarkLookVM. In ThemeDarkLookVM.kt, ButtonState is constructed without an isEnabled override, so it defaults to true:

saveButton =
    ButtonState(
        stringRes(R.string.currencyConversion_saveBtn),
        onClick = ::onSaveClick,
        hapticFeedbackType = HapticFeedbackType.Confirm
    ),

This is inconsistent with ThemeSettingsVM, which correctly disables Save when the selection hasn't changed (isEnabled = selectedMode != originalMode). Saving an unchanged choice is a no-op UX-wise, but the design intent is violated.

Tautological test in ThemeDarkLookVMTest. The assertSingleSelection helper unconditionally calls assertTrue(state.saveButton.isEnabled) in every test case, including neverChosenPreselectsClassicDark where stored = null (no change). There is no test that asserts Save is disabled when the selection matches the stored value. This is the tautological-test rubric violation: the assertion can never fail given the current VM implementation, and the inverse is never verified.

ThemeSettingsVM.onSaveClick always passes isOledEnabled = false. In ThemeSettingsVM.kt:

private fun onSaveClick() =
    viewModelScope.launch {
        setAppearanceMode(selectedMode.value, isOledEnabled = false)
    }

This path is only reachable when selectedMode is LIGHT (System/Dark both forward to the sheet). For Light mode, false is correct. However, the invariant is implicit — if a future AppearanceMode entry is added without a corresponding sheet forward, it would silently clear the OLED flag. A require(selectedMode.value == AppearanceMode.LIGHT) assertion would document the precondition.

ic_settings_theme.xml uses hardcoded #231F20 stroke color with no drawable-night override. In ui-lib/src/main/res/ui/settings/drawable/ic_settings_theme.xml:

android:strokeColor="#231F20"

#231F20 is the dark-background color. The night-variant drawables for the Tor and exchange-rate header assets were updated in this PR to remove charcoal borders, but this settings-row icon has no drawable-night variant. The icon will be invisible against dark/OLED row backgrounds.

ContextThemeWrapper created in remember — low-probability but unguarded risk. In ZcashTheme.kt:

val resolvedContext =
    remember(activityContext, resolvedConfiguration) {
        object : ContextThemeWrapper(activityContext, null) {
            init {
                applyOverrideConfiguration(resolvedConfiguration)
            }
        }
    }

applyOverrideConfiguration throws IllegalStateException if called after onApplyThemeResource. This runs during remember's factory lambda; if it ever throws, it surfaces as a composition crash. Low probability in practice but worth a comment.

Resources / Performance

Two sequential preference reads in startWebBrowser on every browser launch. In Navigator.kt:

private suspend fun startWebBrowser(route: ExternalUrl) {
    WebBrowserUtil.startActivity(
        activity = activity,
        url = route.url,
        appearanceMode = appearanceModeStorageProvider.getOrSystem(),
        isOledEnabled = isOledEnabledStorageProvider.get() == true
    )
}

The already-collected StateFlows on OldHomeViewModel are not accessible here. For a one-time UI action this is acceptable, but it is a slight divergence worth noting. Non-blocking.

FakeIsOledEnabledStorageProvider.observe() returns emptyFlow() in both test files. VMs' init blocks call get() directly, so tests pass — but any future refactor switching to observe() would silently break. Non-blocking.

Design / Refactoring

AppearanceMode.entries order in ThemeSettingsVM.createState is implicit. The display order of System/Light/Dark cards is whatever AppearanceMode.entries produces (declaration order in the enum). Currently correct (SYSTEM, LIGHT, DARK), but reordering the enum for any reason would silently reorder UI cards. An explicit listOf(SYSTEM, LIGHT, DARK) in createState would pin the intent. Nit.

---

Suggestions

1. Blocking: Fix ThemeDarkLookVMTest.assertSingleSelection — either implement Save-disabling in ThemeDarkLookVM when the selected value equals the stored value (matching ThemeSettingsVM's pattern), and add a test asserting it is disabled; or intentionally keep it always enabled, remove the assertTrue(state.saveButton.isEnabled) assertion, and document the intentional always-enabled behaviour.

2. ic_settings_theme.xml needs a drawable-night variant (or switch to a theme-attribute color) so the palette icon is visible on dark and OLED backgrounds.

3. Add require(selectedMode.value == AppearanceMode.LIGHT) in ThemeSettingsVM.onSaveClick to document that the hardcoded isOledEnabled = false is only safe for Light mode.

4. Nit: Pin the UI card order in ThemeSettingsVM.createState with an explicit listOf(SYSTEM, LIGHT, DARK) rather than relying on AppearanceMode.entries order.

5. Non-blocking: Back FakeIsOledEnabledStorageProvider.observe() (and the mode equivalent) with a MutableStateFlow in the test fakes so live-observation paths can be tested in the future without silent divergence.

---

_{Automated review by zodl-review bot · Models: us.anthropic.claude-sonnet-4-6 + us.meta.llama4-maverick-17b-instruct-v1:0 (synthesis) · [Silent pilot — feedback welcome]}

[nesence-m]

Addressed in c91554e:

1. Blocking - Save now disabled when the dark-look selection is unchanged: ThemeDarkLookVM tracks originalOledEnabled and sets isEnabled = oledEnabled != originalOledEnabled, mirroring ThemeSettingsVM. Fixed ThemeDarkLookVMTest.assertSingleSelection to stop asserting Save is always enabled, and added saveDisabledWhenSelectionMatchesStoredAndEnabledAfterChange covering both the disabled and enabled cases.

2. ic_settings_theme.xml - investigated, not changed. Both call sites (SettingsListItemLeadingIcon in the More list, and the header in ThemeSettingsView) render this drawable through Compose Icon(..., tint = ZashiColors.Text.textPrimary), not Image. Icon's ColorFilter.tint recolors every non-transparent pixel regardless of the vector's own fillColor/strokeColor, so the hardcoded #231F20 stroke never reaches the screen - the rendered color is always ZashiColors.Text.textPrimary, which already adapts across light/dark/OLED. This matches every other icon in ui/settings/drawable/ (address_book, feedback, info, voting, whats_new), all of which use the same hardcoded stroke with no drawable-night variant, for the same reason. Happy to add a drawable-night variant anyway if you'd rather have the source asset self-documenting, but functionally the row is not broken today.

3. ThemeSettingsVM.onSaveClick now has require(selectedMode.value == AppearanceMode.LIGHT) { ... } documenting that the hardcoded isOledEnabled = false is only valid when Light is the local selection (System/Dark route through the dark-look sheet instead).

4. ThemeSettingsVM.createState now pins the card order with an explicit listOf(AppearanceMode.SYSTEM, AppearanceMode.LIGHT, AppearanceMode.DARK) instead of relying on AppearanceMode.entries declaration order.

5. FakeIsOledEnabledStorageProvider.observe() and FakeAppearanceModeStorageProvider.observe() in both ThemeDarkLookVMTest and ThemeSettingsVMTest are now backed by a MutableStateFlow, so observe() stays consistent with get()/store().

Skipped per the review's own framing (informational, non-blocking): the ContextThemeWrapper composition-crash note and the startWebBrowser sequential-read note.

ktlintFormat, detektAll, and both VM test classes (6 + 7 tests) are green.

[LukasKorba]

Blocked on one item: the fix round introduced a functional regression that makes the core flow of the feature unreachable. Finding #1 was implemented literally, but the bot's premise was wrong for this screen, and applying it broke Save.

Blocking

You can no longer switch to System or Dark unless you also change the dark look.

ThemeDarkLookVM.kt:61 — isEnabled = oledEnabled != originalOledEnabled.

The VM only knows the OLED boolean (its constructor takes args, navigationRouter, isOledEnabledStorageProvider, setAppearanceMode — no AppearanceModeStorageProvider). But its Save is the only path that persists the appearance mode for SYSTEM and DARK:

• ThemeSettingsVM.kt:76-82 — onModeClick forwards everything that isn't LIGHT to the sheet and deliberately doesn't touch selectedMode.
• ThemeDarkLookVM.kt:71-74 — onSaveClick calls setAppearanceMode(args.mode, …), which is what writes appearance_mode.
• SetAppearanceModeUseCase has exactly two call sites in the tree, and that's the one that matters here.

Repro from stored LIGHT + is_oled_enabled=false (also the never-chosen default):

1. Tap Dark → sheet opens with Classic Dark preselected, so selectedOledEnabled == originalOledEnabled == false.
2. isEnabled = false != false → Save disabled.
3. No other control on the sheet. Dismiss is onBack() → navigationRouter.back(), nothing persisted.

So Dark-with-Classic-Dark is unreachable, and the only way to reach Dark at all is via Pure Black. Same for SYSTEM ↔ DARK in either direction while the look stays put, and for LIGHT → SYSTEM. This worked at cdc2782 (the parent), where ButtonState had no isEnabled and defaulted to true.

The invariant this sheet actually needs is (oledEnabled != originalOledEnabled) || (args.mode != storedAppearanceMode), which means injecting AppearanceModeStorageProvider — or keep it always-enabled and document why. The bot's "mirror ThemeSettingsVM" reasoning doesn't transfer: the Theme screen's Save commits one value, the sheet's commits two.

And the tests now pin the broken behaviour. ThemeDarkLookVMTest.kt:81-91 asserts it as correct. :94-115 seeds stored = LIGHT, args = DARK but clicks Pure Black before Save, so it never touches the mode-changed-look-unchanged path — and dropping assertTrue(state.saveButton.isEnabled) from assertSingleSelection (:130-141) removed the one assertion that would have caught this.

On the other findings

Card order (ThemeSettingsVM.kt:52) and the test fakes (both files now MutableStateFlow-backed) are addressed. Two notes: nothing pins the card order — ThemeSettingsVMTest.optionFor:167-170 uses .first { it.mode == mode }, order-insensitive — and ThemeSettingsView.kt:222 still uses AppearanceMode.entries in the preview. The observe() fake change is cosmetic since nothing exercises it.

You were right to push back on the ic_settings_theme.xml finding — both call sites tint (SettingsListItemLeadingIcon.kt:26-31, ThemeSettingsView.kt:171-176) and ColorFilter.tint overrides the vector's stroke. Same for the ContextThemeWrapper one: onApplyThemeResource only runs on getTheme() and the object is rebuilt when the key changes, so that IllegalStateException path isn't reachable. Both bot false alarms.

Should-fix

Flash of the wrong theme on cold start. MainActivity.kt:171-177 reads the theme from OldHomeViewModel, whose flows seed with SYSTEM/false (OldHomeViewModel.kt:39-49,52-60); the backing read is Dispatchers.IO.limitedParallelism(1) so it can't land before first composition. The splash gate is SecretState.LOADING (MainActivity.kt:159), unrelated, and SPLASH_SCREEN_DELAY = 0.seconds. Dark device + Light chosen → dark frames then light, and vice versa. It's a full repaint, not a subtle tint, because the flip swaps LocalContext/LocalConfiguration (ZcashTheme.kt:122-125) and the whole tree re-resolves. Gating setKeepOnScreenCondition on the first appearance-mode emission closes it. (The read itself is correctly off the main thread — that part's clean.)

Nits

• ThemeSettingsVM.kt:86-89 — require inside viewModelScope.launch is a production crash, not an assertion. If the precondition ever breaks (someone adds a mode that doesn't forward to the sheet — the exact case the bot worried about) the IllegalArgumentException escapes to the default handler. Early return + log documents the invariant without shipping a crash.
• ThemeSettingsScreen.kt:14 — BackHandler { state?.onBack() } is unconditionally enabled, so a back press during the null window is swallowed. enabled = state != null.
• ThemeOption.kt:82-86 — plain clickable, no Role.RadioButton/selectableGroup, so TalkBack announces three generic buttons with no selected state. Not a regression (matches exchangerate.settings.Option) but MigrationModeOption in this same PR does it properly.
• Same seed-then-correct shape as the cold-start flash on both theme screens (ThemeSettingsVM.kt:29-47, ThemeDarkLookVM.kt:27-45) — one frame can render System/Classic-Dark checked regardless of what's stored.
• ui/theme_settings/drawable/ic_theme_settings.xml and its -night twin are added and referenced by nothing. Leftovers from the earlier moon-icon commit; the res folder is still needed for the strings.
• ThemeSettingsView.kt:171-176 — 40dp icon whose art occupies the inner 20 units, in a 40dp Box with another padding(10.dp); net glyph ≈10dp vs ≈20dp in the Settings row. Worth an eyeball against Figma.

Checked and clear

I went looking for M3 tonal-elevation grey leaking into pure black and it isn't there: fully token-driven, BlankBgScaffold uses Surfaces.bgPrimary, the sheet container Surfaces.bgSecondary, all three tonalElevation usages are 0.dp, and there's no surfaceContainer*/surfaceVariant/surfaceTint anywhere — so OledColorPalette copying only surface/background is fine. System bars are right (ZcashTheme.kt:147-169, DefaultOledScrim on the nav bar). No hardcoded colors in the new Compose code, light mode untouched. splash_screen_background stays #231F20 under OLED, but that's pre-existing and out of scope.

Scope

All defensible, none silent, but flagging what touches existing screens: three drawable-night assets (ic_tor_settings, two transaction-filter icons) had #231F20 fills removed so they don't show as charcoal squares on pure black — the tor one redraws the disc at r=32 instead of stroking a ring over r=34, claimed pixel-identical on classic dark. Worth a designer's eye on classic dark, not just OLED. OledZashiColors.kt raises strokeSecondary Shark.900 → Shark.800, OLED-only. And MigrationSetupScreen.kt:309-375 swaps M3 RadioButton for the new shared ZashiRadioIndicator — geometry replicates M3's and Role.RadioButton survives, but ZashiRadioButton.kt:174-178 documents checkedContent as needing to be opaque while MigrationRadioCircle is a stroked ring; it only works because the geometry matches exactly. Tighten that KDoc.

[nesence-m]

Thanks for the deep pass, @LukasKorba — you were right on every count. Two commits: 887409cb0 (the blocking regression) and 6805652e1 (splash gate + nits).

Blocking — Save unreachable for System/Dark. Owning this plainly: the previous round applied the bot's "mirror ThemeSettingsVM" suggestion literally, and the premise did not transfer — this sheet's Save commits two values, and it is the only path that persists the mode for SYSTEM and DARK. Reverted to the correct invariant: ThemeDarkLookVM now takes AppearanceModeStorageProvider, reads the stored mode in init alongside the stored OLED flag, and Save is enabled when (oledEnabled != storedOledEnabled) || (args.mode != storedMode). ThemeDarkLookVMTest was rewritten around the four corners of that invariant — same mode + same look → disabled, mode changed + look unchanged → enabled, mode unchanged + look changed → enabled, never-chosen defaults with args = DARK → enabled — plus a save test that persists a mode change with the look untouched. assertSingleSelection now pins the Save state on every call, which is the assertion whose removal let this through; I re-ran the suite against the old one-term invariant and the two mode-only tests fail, so they genuinely cover it. 887409cb0

Should-fix — flash of the wrong theme on cold start. Both stored values now come from one combined upstream in OldHomeViewModel, so appearanceMode, isOledEnabled and the new isThemeResolved can never disagree about whether the read has landed. setKeepOnScreenCondition gates on !isThemeResolved.value in addition to SecretState.LOADING; isThemeResolved is shared eagerly because the splash gate reads the value without subscribing. SPLASH_SCREEN_DELAY untouched. 6805652e1

Nits — all done (6805652e1):

• ThemeSettingsVM.onSaveClick: require replaced with an early return plus Twig.warn naming the broken invariant. A production crash for a developer-facing precondition was the wrong trade.
• ThemeSettingsScreen: BackHandler(enabled = state != null).
• ThemeOption: selectable(selected = …, role = Role.RadioButton), and both theme screens wrap their cards in selectableGroup(), matching MigrationModeOption.
• Removed ui/theme_settings/drawable/ic_theme_settings.xml and its -night twin (grep-confirmed unreferenced); the res folder stays for the strings.
• Header glyph: the drawable already carries its own 10-unit inset, so the extra padding(10.dp) halved it. Now drawn at size(HEADER_ICON_SIZE) — exactly what SettingsListItemLeadingIcon does — so it renders at the same ~20dp as the Settings row.
• ThemeSettingsView preview uses the explicit listOf(SYSTEM, LIGHT, DARK), and ThemeSettingsVMTest.cardsAreOrderedSystemLightDark pins the order.
• ZashiRadioIndicator's checkedContent KDoc now states the real contract — same size and geometry as uncheckedContent, a filled circle covering it or a ring at the very same radius; opacity is not required. MigrationRadioCircle works because it lines up, not because it is opaque.
• Seed-then-correct first frame: both VMs now hold their state at null until the stored values are read, so neither screen paints a System/Classic-Dark selection first. The screens already handled a null state, so this was cheap.

On scope. The three drawable-night edits (ic_tor_settings, the two transaction-filter icons) and the OledZashiColors Shark.900 → Shark.800 strokeSecondary change are intentional, and I checked all of them on classic dark on the emulator during the original work — the tor disc at r=32 in particular. That is my eye, not a designer's, so a designer pass over classic dark before merge would be worth having; happy to open a follow-up if you would rather not hold this PR for it.

Gates: ktlintFormat, detektAll, :ui-lib:testZcashmainnetInternalDebugUnitTest --tests '*Theme*' (18 tests), :ui-lib:compileZcashmainnetInternalDebugKotlin and :app:compileZcashmainnetStoreDebugKotlin all clean.

[nesence-m]

remove this flag completely - we can force appearanceMode param as AppearanceMode.Dark instead

[nesence-m]

Done in 373ae74 - forceDarkMode is gone from ZcashTheme; the always-dark screens and dark previews now pass appearanceMode = AppearanceMode.DARK, and the former forceDarkMode = false call sites just drop the argument.

[nesence-m]

lets not use OldHomeViewModel at all but instead lets make ThemeVM

[nesence-m]

Done in 64e099a - the theme state moved to a new ThemeVM next to ThemeSettingsVM/ThemeDarkLookVM (registered in viewModelModule, read by MainActivity), and OldHomeViewModel is back to its pre-PR shape; covered by ThemeVMTest.

[LukasKorba]

The Save regression is properly fixed — that one's closed. Two things before this merges, and a note on scope.

The regression — fixed, and the tests are better than I asked for

ThemeDarkLookVM.kt:27 injects AppearanceModeStorageProvider, :48 reads storedMode, and :80 is the invariant. I walked all six cases including the genuine no-op:

stored mode / oled	args.mode	selection	Save
LIGHT / false	DARK	Classic Dark	enabled — the original repro, now reachable
LIGHT / false	SYSTEM	Classic Dark	enabled
DARK / false	SYSTEM	unchanged	enabled
SYSTEM / false	DARK	unchanged	enabled
DARK / true	DARK	Classic Dark	enabled
DARK / false	DARK	Classic Dark	disabled — correct, genuine no-op

And you avoided the seed-then-correct race I was worried about: selectedOledEnabled starts null (:35) and state maps null→null (:39), so isSaveEnabled can't run before the stored read, and both stored values are assigned at :48-49 before the update at :50 in the same launch. Save is never briefly wrong in either direction.

Making isSaveEnabled a required parameter of assertSingleSelection (:247) so every caller has to state it is stronger than the assertion I asked you to restore. saveEnabledWhenModeDiffersEvenThoughLookIsUnchanged (:123) asserts with no click first, which is exactly the missing case. Only gap left is stored=DARK→args=SYSTEM, which only has its mirror tested.

All seven nits addressed. Cold-start gate at MainActivity.kt:167 is sound — isThemeResolved derives from the same combined upstream, SharingStarted.Eagerly, and both prefs emit unconditionally, so it does complete.

Before merge

1. componentActivity() unwraps exactly one level and this PR can stack two wrappers. KoinActivityViewModel.kt:44-50 is context is ContextWrapper && context.baseContext is ComponentActivity, else throw. ScreenshotTest.kt:128 sets configurationOverrideFlow on every test → Override.kt:32-44 wraps LocalContext → ContextThemeWrapper(Activity); MainActivity.kt:177-181 then renders ZcashTheme inside that, and ZcashTheme.kt:109-120 wraps again → ContextThemeWrapper(ContextThemeWrapper(Activity)). AndroidHome.kt:19's koinActivityViewModel<ZashiTopAppBarVM>() then sees a base context that isn't a ComponentActivity and throws.

The KDoc at ZcashTheme.kt:94-100 argues the pattern is safe because it "matches the same safe pattern Override.kt already uses" — that holds for one wrap and is falsified by stacking with the very component it cites. Please fix the reasoning along with the code: either a while unwrap loop in componentActivity(), or don't re-wrap when the ambient context is already a wrapper.

This is the note you skipped in the earlier round as "informational, non-blocking". The finding as originally written was about applyOverrideConfiguration, and I agreed with you that that specific path wasn't reachable — but the double-wrap underneath it is a different and real defect, so the dismissal took the wrong half.

To be straight about what I did and didn't verify: I confirmed the unwrap logic and the two wrap sites by reading head, and test_android_modules_emulator (app) is red. I could not confirm from the check annotations that this is what's failing it — that same job is also red on #2497, which doesn't touch theming, and both show only generic runner annotations. So please confirm against the job log which of the two it is before you spend time on it.

2. The head commit doesn't match your own description. You said the state was gated on the first theme emission in OldHomeViewModel; head is 64e099a, four commits past the block, and 373ae74+64e099a (14:53, after your comment) relocate the theme state into a new ThemeVM across 24 files in the design lib. Same thing I flagged on #2494 an hour ago: I'm reviewing a diff whose changelog describes something else. Not asking you to revert it — just say what you pushed, and I'll read the right thing the first time.

Two soft edges in that new code:

• MainActivity.kt:167 — || short-circuits, so themeVM isn't constructed while secretState is LOADING; the theme read is serialized after the wallet read instead of running alongside it. Reorder the operands and the gate costs nothing.
• ThemeVM.kt:29-42 — no .catch or timeout. If preferenceHolder() throws, the stateIn coroutine dies and the splash never lifts. ThemeVMTest.kt:74 pins that shape as intended. The failure mode I reported was "wrong theme, app opens"; the fix turns it into "app never opens", which is worse. Worth a timeout that falls back to SYSTEM.

Nit

ThemeOption.kt:124,141 — ThemeRadioIndicator's modifier is applied inside both content lambdas rather than on the root. Inert today since no caller passes one, wrong if anyone does.

Keystone is a Maestro home.syncComplete timeout, unrelated to this.