feat(ui): Settings theme picker dialog (#87)#111
Merged
Merged
Conversation
Wires the Settings Theme row to an M3 single-choice AlertDialog
("System default" / "Light" / "Dark"). Hoists the row's read + write
of ThemeMode out of the route composable into a new SettingsViewModel
that owns appPreferences.themeMode and an onSelectTheme(...) entry
point. The persisted write fans out to MainActivity's root-level
collector so PyrycodeMobileTheme re-renders without a process restart.
Closes #87
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ilmoniemi
commented
May 14, 2026
Contributor
Author
ilmoniemi
left a comment
There was a problem hiding this comment.
Code Review: #87
Decision: PASS
Findings
No MUST FIX / SHOULD FIX findings.
- [NIT]
ThemePickerDialog.kt:43,48— usingandroid.R.string.ok/android.R.string.cancelfor the dialog buttons works (localized for free) but diverges slightly from the rest of the codebase, which keeps button strings inapp/src/main/res/values/strings.xml(e.g.promote_dialog_confirm,promote_dialog_cancel). Not a blocker — flagging only because future localisation passes may want consistency. The spec didn't prescribe which path to take.
Verified
SettingsViewModelfollowsChannelListViewModel's canonical shape exactly —stateIn(viewModelScope, WhileSubscribed(5_000L), initial)for the read flow,viewModelScope.launchfor the write. No sealedSettingsState/SettingsEvent(correctly deferred per spec).ThemePickerDialoguses the correct accessibility pattern:Modifier.selectableGroup()on the column +Modifier.selectable(..., role = Role.RadioButton, onClick = …)on each row withRadioButton(onClick = null)— avoids the dual-click-handler trap.MaterialTheme.typography.bodyLargeused; no hardcoded colors /TextStyle()literals anywhere in the diff.remember(selected)keying onpendingis correct — if the persisted value changes while the dialog is closed and the parent recomposes with a newselected, the next open picks it up. Confirm path callsonSelectTheme(pending)→ VM writes; Cancel/dismiss discardspendingvia composition exit. Matches AC #2.ThemeMode.label()promoted fromprivatetointernal(single source of truth, shared between row subtitle and dialog labels). Not moved intodata/preferences/— correct, these are UI strings anddata/is the CMP walk-back boundary.MainActivity.kt:54-62(root-level theme collection) untouched. The new VM adds a second subscriber to the same DataStore flow; one write fans out to both surfaces, which is what AC #3 depends on. VerifiedkoinInjectimport retained (Scanner route + root theme still use it).SettingsViewModelTestreuses theAppPreferencesTestrig (realPreferenceDataStoreFactory+TemporaryFolder+ realAppPreferences) — exercises the round-trip end-to-end. All six scenarios from the spec covered, including the flow re-emit gate.SettingsScreenTest(instrumented) updated to pass the new required params; both@Previewcomposables updated. No stale defaults../gradlew :app:testDebugUnitTestand./gradlew :app:lintDebugclean locally.
Summary
Slice is small, focused, and faithfully tracks the spec. No code-level findings, no acceptance-criteria gaps. Approved.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Wires the Settings → Appearance → Theme row to an M3 single-choice
AlertDialog("System default" / "Light" / "Dark") and hoists the read + write ofThemeModeout of the Settings route composable into a newSettingsViewModel.SettingsViewModel— exposesthemeMode: StateFlow<ThemeMode>viastateIn(viewModelScope, WhileSubscribed(5_000L), SYSTEM)(mirrorsChannelListViewModelshape) andonSelectTheme(mode)that writes throughAppPreferences.setThemeMode(...).ThemePickerDialog— new internal composable inui/settings/. M3AlertDialogwith aselectableGroup()column of three radio rows tracking localpendingstate (remember(selected)), OK/Cancel buttons; confirm →onConfirm(pending), cancel/dismiss →onDismiss().SettingsScreen— new signature(themeMode, onSelectTheme, onBack, onOpenLicense, …); oldthemeMode = SYSTEMdefault deleted. Theme rowonClickflips a localshowThemeDialogopen-state.ThemeMode.label()extension promoted fromprivatetointernalso the dialog composable reuses it — single source of truth for the three label strings.MainActivity— Settings route now useskoinViewModel<SettingsViewModel>()+vm.themeMode.collectAsStateWithLifecycle(), replacing directAppPreferencesinjection. Root-levelappPreferences.themeModecollector that drivesPyrycodeMobileTheme(darkTheme = …)is untouched, so the persisted write fans out to both surfaces and the app theme re-renders without a process restart.AppModule— one-lineviewModel { SettingsViewModel(get()) }Koin registration.strings.xml— addssettings_theme_dialog_title("Theme"). Radio labels reuseThemeMode.label().Issue
Closes #87
Testing
./gradlew test— pass (newSettingsViewModelTestcovers initial state, mirror-of-persisted-value, persistence of each of LIGHT/DARK/SYSTEM, and re-emission afteronSelectTheme)../gradlew lint— pass../gradlew assembleDebug— pass../gradlew connectedAndroidTest— not run (no device/emulator wired in this worktree).SettingsScreenTestwas updated to pass the new params (compile-only verified).Architecture compliance
Follows the spec at
docs/specs/architecture/87-settings-theme-picker-dialog.mdverbatim:SettingsViewModelshape mirrorsChannelListViewModel(stateIn+WhileSubscribed(5_000L)); dialog uses OK/Cancel (decided in spec § Design);ThemeMode.label()promoted tointernal(decided in spec § Label helper); dialog open-state is local to the screen composable;pendingkeyed byselectedso a reopen picks up an updated persisted value (decided in spec § State + concurrency model);MainActivityroot-level collector left intact (decided in spec § Files to read first).🤖 Generated with Claude Code