fix(theme): stop SYSTEM + Material You flipping light on Pixel (#677) - #696
Conversation
Extract a pure resolver that prefers application night mode and refuses to treat NIGHT_UNDEFINED as light.
Resolve SYSTEM appearance from application night, Activity night, and Compose, and keep last-known dark when the night mask is undefined.
Replace Theme.Material.Light first-frame context and size enableEdgeToEdge from saved theme_mode plus application night.
Move the rep counter and next-exercise cards off unclamped primaryContainer so wallpaper Fixed/container roles cannot paint those surfaces light.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bc79f8bad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code Review Roast 🔥Verdict: Critical resolved | Recommendation: Merge-ready on correctness; Ponytail cleanup optional Overview
Issue Status (click to expand)✅ Resolved in 7468667
|
| Severity | Count |
|---|---|
| 🚨 critical | 1 |
| 0 | |
| 💡 suggestion | 0 |
| 🤏 nitpick | 0 |
Issue Details (click to expand)
| File | Line | Roast |
|---|---|---|
androidApp/src/main/kotlin/com/devil/phoenixproject/VitruvianApp.kt |
32 | Still calling applyPersistedApplicationNightMode(base) BEFORE super.attachBaseContext(base). The refactor moved the helper into PlatformNightMode.android.kt and split it into two functions — getSharedPreferences now uses base directly (no NPE there), but applyApplicationNightMode at L35 still does context.applicationContext ?: context and calls getSystemService on the Application whose mBase is still null. The crash just relocated; it didn't leave. |
Correctness / Safety Findings
- critical:
androidApp/src/main/kotlin/com/devil/phoenixproject/VitruvianApp.kt:L31-34— Re-verified against58ec2fb1.applyPersistedApplicationNightMode(base)runs beforesuper.attachBaseContext(base). The refactored helper atshared/src/androidMain/kotlin/com/devil/phoenixproject/ui/theme/PlatformNightMode.android.kt:L23-38reads prefs viacontext.getSharedPreferences(...)on the base ContextImpl (no NPE there), but the innerapplyApplicationNightModeresolvesval appContext = context.applicationContext ?: context(L35) which returns the Application instance, andappContext.getSystemService(UiModeManager::class.java)(L36) delegates tomBase.getSystemService→ NPE becauseApplication.mBaseis null until super runs. Fires on every cold start on API 31+. Required fix: callsuper.attachBaseContext(base)first, thenapplyPersistedApplicationNightMode(this)(orbase— after super,base.applicationContextreturns the Application withmBase = basepopulated, sogetSystemServiceworks). Existing comment 3787207933 at L32 still applies — root cause unchanged, only the NPE location moved fromgetSharedPreferencestogetSystemService.
Ponytail Review
shared/src/androidMain/kotlin/com/devil/phoenixproject/ui/theme/PlatformNightMode.android.kt:L9-16: yagni —ApplicationContextHoldersingleton +rememberApplicationContextduplicate the fallback already at L48 (ActivityHolder.getActivity()?.applicationContext).applyPlatformNightModeis only called fromThemeViewModel.setThemeMode, which runs after an Activity is resumed, soActivityHolderis always populated. Delete the singleton + setter, drop bothrememberApplicationContextcalls, and simplifyapplyPlatformNightModetoActivityHolder.getActivity()?.applicationContext?.let { applyApplicationNightMode(it, themeMode) }.shared/src/androidMain/kotlin/com/devil/phoenixproject/ui/theme/PlatformNightMode.android.kt:L33: shrink —rememberApplicationContext(context)inapplyApplicationNightModeis redundant; already called inapplyPersistedApplicationNightModeat L24 and would be deleted with the yagni finding above.shared/src/androidMain/kotlin/com/devil/phoenixproject/ui/theme/PlatformNightMode.android.kt:L35: shrink —context.applicationContext ?: contextis a defensive fallback that turns into a footgun before super (see critical).contextis always a valid Context here; drop the?: contextand usecontext.applicationContextdirectly.
Ponytail net: -16 lines.
Suggested Minimal Patch
Reorder VitruvianApp.attachBaseContext:
override fun attachBaseContext(base: Context) {
super.attachBaseContext(base)
applyPersistedApplicationNightMode(this)
}…and delete ApplicationContextHolder + rememberApplicationContext from PlatformNightMode.android.kt, remove the redundant rememberApplicationContext call at L33, and drop the ?: context fallback at L35. Simplify applyPlatformNightMode to use ActivityHolder.getActivity()?.applicationContext directly.
Final Merge Guidance
Do not merge until the critical NPE is fixed. The refactor in 58ec2fb1 addressed the chatgpt-codex P1 about reapplying night mode on theme change (good — ThemeViewModel.setThemeMode now calls applyPlatformNightMode), but the context-before-super crash from the previous review was not fixed. The helper moved; the bug tagged along. One-line reorder and we're cooking. Until then this is still a bug dressed up as a fix.
🏆 Best part: The expect/actual split (PlatformNightMode.kt + .android.kt + .ios.kt) is the right shape for multiplatform, and the ThemeViewModel.setThemeMode → applyPlatformNightMode wiring correctly addresses the live SYSTEM→device-following gap that the previous review flagged.
💀 Worst part: Splitting the helper into two functions to dodge the previous review's getSharedPreferences NPE callout, without fixing the underlying context-before-super. The NPE just relocated to getSystemService in the same lifecycle position. This is the code equivalent of moving the smoke detector to the room that's not on fire.
📊 Overall: The intent is right, the multiplatform shape is right, the ThemeViewModel wiring is right — but the one-line reorder that was the entire fix from the previous review still hasn't landed. Until it does, this crashes on every cold start on the only API level it targets.
Files Reviewed (3 changed files)
shared/src/androidMain/kotlin/com/devil/phoenixproject/ui/theme/PlatformNightMode.android.kt- 1 issue (critical carryover) + 3 Ponytailshared/src/commonMain/kotlin/com/devil/phoenixproject/presentation/viewmodel/ThemeViewModel.kt- no new issuesshared/src/androidHostTest/kotlin/com/devil/phoenixproject/presentation/theme/Issue677WindowThemeContractTest.kt- no new issues
Reviewed by minimax-m3 · Input: 51.2K · Output: 11.3K · Cached: 465.3K
Review guidance: REVIEW.md from base branch main
Qualify day/night window colors, sync permission-screen system bars, and share theme pref keys so cold-start no longer flashes the wrong palette.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 172dc31b23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Force application night mode from saved theme_mode in Application.attachBaseContext so values vs values-night match explicit LIGHT/DARK before MainActivity.onCreate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb3778cbb0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ThemeViewModel.setThemeMode now calls applyPlatformNightMode so LIGHT/DARK/SYSTEM updates UiModeManager live. The helper lives in shared so attachBaseContext and the ViewModel share one path.
Call applyPersistedApplicationNightMode(this) after super so SharedPreferences does not run against an Application with a null mBase.
Fixes #677
Why
Two shipped one-boundary patches failed:
uiModefromconfigChanges. Still flipped.Activity.resources.configuration.uiModeonON_RESUME. After the Aug 7 build, lock made Phoenix lighter while Android stayed dark.RCA: SYSTEM treated
NIGHT_UNDEFINEDas light, sampled a Light XML Activity context after lock recreate (keyboardHidden/colorMode), then #691 made that sample sticky. Secondary leak: large workout chrome used unclampedprimaryContainer.What this PR does (three boundaries)
resolveSystemDarkprefers application night, keeps last-known when undefined, and refuses to go light from a lone ActivityNOunless Compose also says light.rememberPlatformSystemDark()andTheme.ktuse it. The#691isDark = refreshedlatch is gone.Theme.Material.Light.NoActionBaris gone. Values + values-night useTheme.Material.NoActionBarwith Slate900windowBackground.enableEdgeToEdgesizes bars from persistedtheme_mode+ application night.ApplyStatusBarAppearancenow updates nav-bar icons too.surfaceContainerHighest/onSurface. FABs and selected-routine 0.4 tint stay onprimaryContainer.#640clamp file is untouched.uiModeis not added back toconfigChanges. Material You is kept. iOS actual is unchanged. DARK/LIGHT modes are unchanged.Tests (parent-verified)
./gradlew -Pskip.supabase.check=true :shared:testAndroidHostTestfocused:SystemDarkResolverTest10/10ThemeModeUiContractGuardTest4/4Issue677WindowThemeContractTest5/5MaterialYouDarkClampContractTest5/5RoutinesChromeLuminanceContractTest8/8:androidApp:compileDebugKotlinBUILD SUCCESSFUL.Plan:
docs/superpowers/plans/2026-08-14-issue-677-system-dark-stability.md(this checkout does not include the plan file).Human close gate (do not close from CI green)
On the reporter Pixel: Theme = System, Material You on, Android dark.
Optional logcat:
PlatformSystemDark.app=YES activity=NOresolving to dark is the #691 failure mode being corrected.