fix(#640): clamp dark Material You surface family to static Slate ramp

[9thLevelSoftware]

Summary

Fixes #640 — Routines screen: expanded card flickers to light when Dark + Material You is enabled.

On Android 12+ with Dark mode + Material You, dynamicDarkColorScheme(context) returns a wallpaper-derived dark palette whose surface / surfaceContainerHigh / surfaceContainerHighest roles can resolve to high-luminance values for some wallpapers. The Routines screen reads these directly via MaterialTheme.colorScheme:

Surface	Token sampled
Top app bar	MaterialTheme.colorScheme.surface
Bottom nav	MaterialTheme.colorScheme.surfaceContainerHigh
Expanded RoutineCard (8.dp elevation)	MaterialTheme.colorScheme.surfaceContainerHighest

Meanwhile surfaceContainerLowest stays dark, so the gradient anchors dark and the screen reads half-light/half-dark on the same scheme. The per-card expand/collapse flicker is the surfaceColorAtElevation tint amplifying that divergence.

Fix

New clampDynamicDarkScheme(dynamic, fallback) helper in shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkScheme.kt returns a ColorScheme whose:

• Surface family (conservative clamp): surface / onSurface / surfaceContainerLowest..Highest / surfaceVariant / onSurfaceVariant / surfaceDim / surfaceBright / background / onBackground all come from fallback (the static DarkColorScheme Slate950/900/800/700 ramp). These tokens are clamped as a unit because they are part of the same wallpaper-miscalibrated surface family that the reporter saw flip light — a wallpaper that produces high-luminance surfaceContainerHighest can just as easily produce high-luminance surfaceVariant or background.
• Wallpaper hue (preserved): primary / secondary / tertiary / error / outline / outlineVariant come from dynamic, so the wallpaper hue still flows through toggles, sliders, selection chips, badges, and error states — the user-interactive accents that the user expects to be wallpaper-colored.

Wired into Theme.kt on the dark + dynamic branch only:

val dynamic = platformDynamicColorScheme(useDarkColors)
when {
    dynamic != null && useDarkColors -> clampDynamicDarkScheme(dynamic, DarkColorScheme)
    else -> dynamic
}

Light + Material You and Material You = OFF are unchanged; iOS never invokes the helper (its platformDynamicColorScheme returns null).

Acceptance criteria (RCA for #640, §5)

• Dark + Material You = ON, high-luminance wallpaper: top app bar, bottom nav, expanded RoutineCard, collapsed RoutineCard all render with the static DarkColorScheme surface family.
• Dark + Material You = ON, high-luminance wallpaper: screen background gradient remains dark (Slate950-anchored via surfaceContainerLowest).
• Dark + Material You = ON, any wallpaper: expand/collapse a routine card, the 2.dp vs 8.dp elevation shift is ≤ 6% luminance.
• Dark + Material You = ON, any wallpaper: primary / secondary / tertiary / error / outline / outlineVariant come from the dynamic scheme (wallpaper hue preserved on user-interactive accents).
• Dark + Material You = ON, any wallpaper: surface / onSurface / surfaceVariant / onSurfaceVariant / surfaceContainerLowest..Highest / surfaceDim / surfaceBright / background / onBackground come from the static DarkColorScheme (conservative clamp on the surface family as a unit).
• Dark + Material You = OFF: behavior identical to today (static DarkColorScheme).
• Light + Material You = ON or OFF: behavior identical to today.
• iOS path: unchanged.
• All existing tests (ThemeViewModelTest, ThemeModeUiContractGuardTest, ThemeToggleTest, ColorSchemeTest, DesignSystemRatchetTest) pass unmodified.

Tests added (20 new, all green, full suite 2409/2409 passing)

• ClampedDynamicDarkSchemeTest (10 cases, commonTest): boundary luminance 0/0.49/0.5/0.51/1.0, surface family equals DarkColorScheme, conservative-clamp regression on surfaceVariant / onSurfaceVariant / background / onBackground / surfaceDim / surfaceBright, accent preservation, no-mutation contract, ≤ 6% adjacent-shift invariant, identity-when-equal.
• MaterialYouDarkClampContractTest (4 cases, androidHostTest): Theme.kt invokes the clamp on the dark branch only; helper co-located with Theme.kt; DarkColorScheme unmodified.
• RoutinesChromeLuminanceContractTest (4 cases, androidHostTest): TopAppBar / PhoenixBottomNavigationBar / RoutineCard continue to read from MaterialTheme.colorScheme.*; no hardcoded Color.White containerColor crept into the chrome.
• RoutineCardElevationContractTest (2 cases, androidHostTest): Card.defaultElevation = if (expanded) 8.dp else 2.dp preserved so the elevation tint shift stays bounded.

Compose UI tests / Robolectric would add equivalent coverage at the rendered-tree level but are not configured by this repo today; the helper-level coverage plus the read-site contract tests close the same loop with zero new build dependencies.

Files changed

shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/Theme.kt                       | 9 ++++++++-   (1 file, clamp call site)
shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkScheme.kt   | new          (helper)
shared/src/commonTest/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkSchemeTest.kt | new          (10 cases)
shared/src/androidHostTest/kotlin/com/devil/phoenixproject/ui/theme/MaterialYouDarkClampContractTest.kt | new  (4 cases)
shared/src/androidHostTest/kotlin/com/devil/phoenixproject/presentation/theme/RoutinesChromeLuminanceContractTest.kt | new  (4 cases)
shared/src/androidHostTest/kotlin/com/devil/phoenixproject/presentation/theme/RoutineCardElevationContractTest.kt | new  (2 cases)

Files NOT changed (per RCA non-goals)

• EnhancedMainScreen.kt, RoutinesTab.kt, RoutineGroupHeader.kt — chrome reads from MaterialTheme.colorScheme.*; the fix propagates through the theme.
• ThemeViewModel.kt — no theme persistence / race changes.
• androidApp/.../AndroidTheme.kt — deprecated, unreachable from App.kt.
• iOS-specific code under shared/src/iosMain/.
• The static DarkColorScheme / LightColorScheme definitions — they remain the brand contract for iOS / non-dynamic Android.

Fixes #640

[gemini-code-assist]

Code Review

This pull request addresses issue #640 by introducing a clampDynamicDarkScheme helper to prevent high-luminance wallpaper colors from leaking into dark-mode surfaces, along with several contract and unit tests to verify the behavior. Feedback suggests clamping inverseSurface and inverseOnSurface as well to avoid accessibility and contrast issues with components like Snackbars. Additionally, the reviewer advises against reading physical source files in contract tests due to environment and refactoring fragility, recommending static analysis tools instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[kilo-code-bot]

Code Review Roast 🔥

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this increment is actually clean too. Two for two. Either the author is listening, or they've installed a code-review-avoidance script that strips out anything I might roast. I'll take it.

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	0
🤏 nitpick	0

No new findings on changed code in this increment (commit 77a690a7 — fix(#640): clamp surfaceTint with dark surface family).

Re-verification of previous findings

• ✅ ClampedDynamicDarkScheme.kt:39 (prior: doc-comment token list was incomplete) — The KDoc's surface-family list on lines 22–24 now includes surfaceTint (surfaceBright / surfaceTint / background / onBackground / inverseSurface / inverseOnSurface), and a new explanatory paragraph on lines 33–35 documents why surfaceTint is clamped (Material 3 applies it as an overlay to elevated surface containers, so a bright wallpaper tint would reintroduce chrome drift even after the surface roles are clamped). The prior Gemini-referenced concern is fully closed by this increment — surfaceTint now lives in both the what we clamp list and the why we clamp it prose. Clean.

No other prior findings touch changed lines in this increment.

What the increment does (correctness check)

• ClampedDynamicDarkScheme.kt:78 — surfaceTint = fallback.surfaceTint added to the .copy(...). Correct: surfaceTint is the M3 elevation-tint overlay color, and clamping it keeps elevated bars/cards on the brand-controlled dark ramp instead of letting the wallpaper's primary-derived tint bleed through Card/Surface elevation overlays.
• ClampedDynamicDarkSchemeTest.kt:101 — fixture gets surfaceTint = Color(0xFFFFF59D) (a bright pale yellow) so the precondition / clamp assertions below are actually meaningful (the dynamic surfaceTint must differ from fallback.surfaceTint).
• ClampedDynamicDarkSchemeTest.kt:136–138 — adds assertNotEquals(dynamic.surfaceTint, fallback.surfaceTint, ...) as a precondition guard, then assertEquals(fallback.surfaceTint, clamped.surfaceTint) to pin the clamp. The assertNotEquals precondition is the right shape — it stops a future fixture refactor from silently making the equality assertion tautological.
• ClampedDynamicDarkSchemeTest.kt:307,314 — boundary test 1_0 (pure-white dynamic surfaces) now also covers surfaceTint = Color.White, proving the clamp survives the extreme-white edge case.
• MaterialYouDarkClampContractTest.kt:98,112,113–116 — contract test renamed to clampHelper_clampsInverseSurfaceAndSurfaceTintFromFallback and extended to grep for the surfaceTint = fallback.surfaceTint substring. Keeps the source-level contract test in sync with the helper.

Ponytail Review

• ClampedDynamicDarkSchemeTest.kt:316–330 — test-shrink (carry-forward, pre-existing, not in this diff): the symmetric boundary test 0_0 — black dynamic surface is also clamped to fallback was not updated to mirror the new surfaceTint = Color.Black + assertion that 1_0 got on lines 307/314. The asymmetry was already there for surfaceBright before this increment, and this increment made it marginally worse by extending 1_0 without touching 0_0. Optional cleanup: add surfaceTint = Color.Black to the .copy(...) and an assertEquals(fallback.surfaceTint, clamped.surfaceTint) line so the two boundary tests are mirror images. Not blocking; the test name + the 1_0 assertion already pin the clamp.

Ponytail net: 0 lines (this increment is itself the smallest possible diff to extend the surface-family clamp — 1 production line, 2 doc-comment paragraphs, 2 source-grep test lines, 1 test fixture field, 2 assertions, 1 boundary test extension, 0 new abstractions, 0 new imports).

Suggested Minimal Patch

No patch needed. The prior ClampedDynamicDarkScheme.kt:39 doc-comment finding is correctly addressed by the new surfaceTint mention in both the token list and the rationale prose.

Final Merge Guidance

Can merge as-is. This increment is the minimum viable diff to close the surfaceTint leak: one production-code line plus the doc/test contract to prove it stays closed. The only optional carry-forward is the boundary-test asymmetry, which has been on the optional-cleanup pile since before this PR and is not made materially worse by shipping this increment.

🏆 Best part: The assertNotEquals(dynamic.surfaceTint, fallback.surfaceTint, "precondition: ...") guard at ClampedDynamicDarkSchemeTest.kt:136–137. Most "equality" tests silently degenerate to tautologies when someone refactors the fixture to match the expectation. This one refuses to. That's the kind of defensive test-writing that makes future-you send thank-you notes to past-you.

💀 Worst part: The doc comment on ClampedDynamicDarkScheme.kt:33–35 has a sentence that runs light, while the underlying screen is dark from the clamp → no contrast). \surfaceTint` is also clamped because…` — the parenthetical close happens, then a new top-level sentence starts. It's readable, but it's the kind of run-on that a fresh-eyed reviewer will eventually flag in a future pass for KDoc style. Optional: split the inverseSurface rationale and the surfaceTint rationale into two paragraphs.

📊 Overall: Like a clean compile after a refactor — boring in the best possible way. Same shape as the previous increment: small, targeted, no scope creep, no new helpers, no new dependencies. The fact that the prior review's ClampedDynamicDarkScheme.kt:39 doc-comment finding is now also closed is the cherry on top.

Files Reviewed (3 files in this increment)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkScheme.kt — 1 production line added (surfaceTint = fallback.surfaceTint), 2 doc-comment paragraphs updated to reflect the new clamped token. Correctness fix for Material 3 elevation-tint drift.
• shared/src/commonTest/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkSchemeTest.kt — test fixture gains surfaceTint = Color(0xFFFFF59D), primary test gains precondition + equality assertion for surfaceTint, boundary test 1_0 gains surfaceTint = Color.White + assertion. All assertions meaningful, no tautologies.
• shared/src/androidHostTest/kotlin/com/devil/phoenixproject/ui/theme/MaterialYouDarkClampContractTest.kt — test renamed to clampHelper_clampsInverseSurfaceAndSurfaceTintFromFallback, source-grep assertion extended with surfaceTint = fallback.surfaceTint, error message updated.

_{Reviewed by minimax-m3 · incremental review of 1 commit · Fix these issues in Kilo Cloud}

Previous Review Summaries (6 snapshots, latest commit 39a3a35)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 39a3a35)

Verdict: No Issues Found | Recommendation: Merge

Oh wait, this PR is actually clean now. I had my flamethrower warmed up and everything, and the author went and deleted the exact two things I asked them to delete. That's either growth or surrender. I'll take it.

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	0
🤏 nitpick	0

No new findings on changed code in this increment (commit 39a3a35e — test(#640): dedupe chrome contract assertions).

Re-verification of previous findings

• ✅ nitpick RoutinesChromeLuminanceContractTest.kt (was lines 96-100) — The redundant assertTrue(color != Color(0xFFFFFFFF), …) block is gone. assertNotEquals(Color.White, color) on the line above was doing exactly the same job, and the duplicate is now correctly deleted. The assertTrue import was removed from line 4 as well. Clean.
• ✅ suggestion RoutineCardElevationContractTest.kt (was lines 35-42) — The arithmetic tautology test routineCard_expandedElevationIsAboveCollapsedElevation is gone. The companion assertTrue import was removed from line 4 too. The surviving test (routineCard_elevationIsTwoOrEightDp) pins both values exactly, so the ordering relationship is the laws of arithmetic's problem now, not the test file's. Clean.

Both fixes are exactly the shape the previous review asked for: pure deletion, no new imports, no new test scaffolding, no new abstractions introduced to fix the old ones.

Ponytail Review

• shared/src/commonMain/.../PhoenixChromeColorContracts.kt:18-22 — yagni (carry-forward, optional): phoenixTopAppBarContainerColor and phoenixBottomNavigationContainerColor are still one-line pass-throughs to colorScheme.surface / colorScheme.surfaceContainerHigh. The author chose to keep them as semantic accessors for testability, which is a defensible position; the previous review's Ponytail note acknowledged that the helper layer makes the call sites addressable. Leaving as-is is fine. The two branching helpers (routineCardContainerColor, routineCardDefaultElevation) clearly earn their keep.
• shared/src/androidHostTest/.../RoutinesChromeLuminanceContractTest.kt:77-96 — test-shrink (carry-forward, optional): chromeHelpers_areDrivenByInputSchemeNotHardcodedLightColors is now a single-assertion smoke test (assertNotEquals(Color.White, color) for three helpers). It's not tautological — a future return Color.White in any helper would trip it — but the per-helper assertEquals(scheme.surface, phoenixTopAppBarContainerColor(scheme)) tests above it already pin the same property with stronger messages. The remaining test is borderline; dropping it would lose a thin regression net but reclaim ~20 lines. Optional.

Ponytail net: 0 lines (this increment is the Ponytail — 16 lines of redundant test code deleted, no new code added).

Suggested Minimal Patch

No patch needed. Both prior findings are correctly addressed.

Final Merge Guidance

Can merge as-is. This increment is the smallest possible diff to close the previous review's two findings: delete the redundant test, delete the redundant assertion, delete the now-orphan assertTrue imports, ship. The remaining Ponytail observations are about pre-existing helper/test choices that the author has already weighed in on; this increment doesn't change that calculus.

🏆 Best part: The two cleanups are surgical. No "well actually let me also…" follow-on edits, no scope creep, no new helpers to manage the old helpers. Just git rm of the redundant test method and the redundant assertTrue block, plus the two assertTrue import lines that became orphans. This is what a focused fix-it cycle looks like.

💀 Worst part: chromeHelpers_areDrivenByInputSchemeNotHardcodedLightColors is now a one-assertion test with a 19-line method body (mostly the listOf(...) and forEach { … } ceremony around a single assertNotEquals). The boilerplate-to-assertion ratio is roughly 18:1, which is the kind of stat that makes me want to file a Ponytail ticket. But the previous review already left it on the optional-cleanup pile, and the author has the right to ship.

📊 Overall: Like the third pancake — the previous two were over- or under-done, this one's the right shape. Same batter, no extra ingredients, no missing ingredients. Eat it now.

Files Reviewed (2 files in this increment)

• shared/src/androidHostTest/.../RoutineCardElevationContractTest.kt — 10 lines removed (redundant routineCard_expandedElevationIsAboveCollapsedElevation test + assertTrue import). Both prior findings resolved.
• shared/src/androidHostTest/.../RoutinesChromeLuminanceContractTest.kt — 6 lines removed (redundant assertTrue(color != Color(0xFFFFFFFF), …) block + assertTrue import). Both prior findings resolved.

_{Reviewed by minimax-m3 · incremental review of 1 commit}

Previous review (commit 8dd6985)

Verdict: Approve with optional cleanup | Recommendation: Merge after the two redundancies are removed (or leave them — they're harmless).

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	1
🤏 nitpick	1

Issue Details (click to expand)

File	Line	Roast
RoutinesChromeLuminanceContractTest.kt	96	assertTrue(color != Color(0xFFFFFFFF)) is identical to assertNotEquals(Color.White, color) on the prior line — Color.White == Color(0xFFFFFFFF). Drop one.
RoutineCardElevationContractTest.kt	36	routineCard_expandedElevationIsAboveCollapsedElevation tests 8.dp > 2.dp while the test above pins exact values to 2.dp and 8.dp. Arithmetic tautology. Drop the test.

---

Correctness / Safety Findings

nitpick: shared/src/androidHostTest/.../RoutinesChromeLuminanceContractTest.kt:96 — assertTrue(color != Color(0xFFFFFFFF)) is redundant with assertNotEquals(Color.White, color) on line 90. Color.White is defined as Color(0xFFFFFFFF) in Compose, so the two checks express identical conditions. Drop the assertTrue block at lines 96-100; the assertNotEquals already covers it.

suggestion: shared/src/androidHostTest/.../RoutineCardElevationContractTest.kt:36 — routineCard_expandedElevationIsAboveCollapsedElevation is mathematically implied by routineCard_elevationIsTwoOrEightDp, which pins the exact values 2.dp and 8.dp. The ordering test cannot fail in any universe where the first test passes — it's a regression test against the laws of arithmetic. Delete the entire test method (lines 35-42), or downgrade the first test to a range-check if exact pinning isn't important.

---

Ponytail Review

• shared/src/commonMain/.../PhoenixChromeColorContracts.kt:18-22 — yagni/shrink: phoenixTopAppBarContainerColor and phoenixBottomNavigationContainerColor are one-line pass-throughs to colorScheme.surface and colorScheme.surfaceContainerHigh respectively. With the helper layer established, the testable behavior is now "does this function call the property it claims to" — which is testing implementation, not behavior. Inline the property reads at the call sites and drop these two helpers. The two branching helpers (routineCardContainerColor, routineCardDefaultElevation) earn their keep because they encode if/else logic; the pure property accessors don't.
• shared/src/androidHostTest/.../RoutinesChromeLuminanceContractTest.kt:79-102 — test-shrink: chromeHelpers_areDrivenByInputSchemeNotHardcodedLightColors is tautological with the new thin-helper architecture — it's effectively testing that darkColorScheme().surface != Color.White, which is a property of the Material 3 library, not of these helpers. Drop it.

Ponytail net: ~-22 lines if all Ponytail suggestions are taken (delete phoenixTopAppBarContainerColor + phoenixBottomNavigationContainerColor definitions, the tautological test, and the redundant assertion).

Suggested Minimal Patch

Optional cleanup:

1. Delete the assertTrue(color != Color(0xFFFFFFFF), ...) block at RoutinesChromeLuminanceContractTest.kt:96-100.
2. Delete the entire routineCard_expandedElevationIsAboveCollapsedElevation test at RoutineCardElevationContractTest.kt:35-42.
3. (Optional, lower priority) Inline MaterialTheme.colorScheme.surface / surfaceContainerHigh at the two EnhancedMainScreen.kt call sites and delete the two pure pass-through helpers in PhoenixChromeColorContracts.kt.

Final Merge Guidance

Can merge. The helper-extraction approach to retire the source-scanning tests is a legitimate answer to the prior "fragile .kt-string-matching" concern (the previous reviewer declined tightening the regex; you took the other exit and made the call sites addressable). The two findings are dead-code redundancies, not correctness bugs. Ship if you don't want to touch it; the optional cleanup takes 2 minutes if you do.

🏆 Best part: The contract tests now read like behavior assertions instead of regex prisoners. assertEquals(scheme.surface, phoenixTopAppBarContainerColor(scheme)) is the kind of test that ages well.

💀 Worst part: routineCard_expandedElevationIsAboveCollapsedElevation — a test that exists to prove 8 > 2. The laws of mathematics wrote that test for you, free of charge, no maintenance required.

📊 Overall: Like a fourth pancake — slightly overworked compared to the third, but the ingredients are sound and the stack is consistent. Eat it before it gets cold.

Files Reviewed (5 files in this increment)

• shared/src/commonMain/.../PhoenixChromeColorContracts.kt — new helper file with 4 tiny accessors (2 pass-throughs, 2 with branches). Sound shape.
• shared/src/commonMain/.../EnhancedMainScreen.kt — TopAppBar + bottom nav now route through the helpers. Correct.
• shared/src/commonMain/.../RoutinesTab.kt — RoutineCard uses routineCardContainerColor + routineCardDefaultElevation; unused Color import correctly removed.
• shared/src/androidHostTest/.../RoutinesChromeLuminanceContractTest.kt — rewritten to assert on helpers; one redundant assert, one tautological test.
• shared/src/androidHostTest/.../RoutineCardElevationContractTest.kt — rewritten to assert on the helper; one redundant test.

---

_{Reviewed by minimax-m3 · Input: ~30K · Output: ~4K}

Previous review (commit baea6ab)

Verdict: Approve | Recommendation: Both prior findings resolved cleanly. Ship.

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	0
🤏 nitpick	0

Correctness / Safety Findings

No correctness or safety findings on changed code.

Ponytail Review

Ponytail: Lean already. Ship.

Ponytail net: 0 lines (this increment is the Ponytail — -7 lines net).

Suggested Minimal Patch

No patch needed.

Final Merge Guidance

Can merge. The previous nitpick (partial 4-token enumeration in the second KDoc paragraph) and suggestion (3rd/4th-copy inverseSurface / inverseOnSurface asserts in the conservative-clamp test) are both correctly addressed in commit baea6ab. The "Why not pass..." paragraph is now generic and applies uniformly to all 9 clamped tokens; the redundant asserts are gone with a clear comment explaining the deduplication. The contract test in MaterialYouDarkClampContractTest continues to pin the composition.

🏆 Best part: The author actually listened. Two findings, two clean fixes, net -7 lines, and the new comment at ClampedDynamicDarkSchemeTest.kt:172-175 reads like an apology to the next reviewer. "Don't add it here too — see the broader test above." That's the kind of in-source signposting that turns a fragile test file into a self-maintaining one.

💀 Worst part: Honestly, this is one of those annoying increments where the diff is so small and so on-point that roasting it would be picking a fight with a parking meter. The most roastable thing here is that the precondition assert was reduced from 5 conditions to 3 in conservative clamp routes... — but that's literally the fix you asked for. Quibbling would be unbecoming.

📊 Overall: Like the second pancake — same batter, better shape. The conservative clamp, the Snackbar rationale, and the test composition are all in agreement now, and the file is shorter for it. Ship before someone "improves" it.

Files Reviewed (2 files in this increment)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkScheme.kt — KDoc paragraph 2 rewritten from partial 4-token enumeration to generic rationale. Clean; addresses prior nitpick.
• shared/src/commonTest/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkSchemeTest.kt — dropped redundant inverseSurface / inverseOnSurface asserts (lines 172-175) and reverted precondition to 3-token form. Clean; addresses prior suggestion.

_{Reviewed by minimax-m3 · Input: ~25K · Output: ~3K}

Previous review (commit 3098c57)

Verdict: Approve | Recommendation: Gemini high-priority concern correctly resolved; one optional Ponytail cleanup and one KDoc polish.

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	1
🤏 nitpick	1

Issue Details (click to expand)

File	Line	Roast
shared/src/commonTest/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkSchemeTest.kt	175	New inverseSurface / inverseOnSurface asserts duplicate what clamped surface family equals the static dark fallback surface (lines 117-137) already pins. The previous review flagged this test as redundant; the new commit doubled down on the redundancy.
shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkScheme.kt	35	"Why not pass..." paragraph enumerates only 4 tokens but the conservative clamp now pins 9 — the rationale for inverseSurface / inverseOnSurface (and the surfaceContainer* family) is missing.

---

🏆 Best part: The Gemini high-priority concern (Snackbar dark-on-dark via inverseSurface) is fixed correctly and the KDoc explanation is the most honest paragraph in this PR — it walks through the exact failure mode (light surface → dark inverseSurface → unreadable Snackbar) and the fix (clamp the inverse tokens too). The new clampHelper_clampInverseSurfaceFromFallback contract test pins the composition so a future refactor can't quietly drop these two lines.

💀 Worst part: The conservative clamp routes... test at line 140 is a textbook example of "the previous review said delete this, you said add to it." It's now asserting 8 tokens that are already pinned by the surface-family test, with the new inverseSurface/inverseOnSurface asserts being the third and fourth copy of the same check. Stop. Deleting the test would lose zero coverage and reclaim ~40 lines.

📊 Overall: The increment does what the Gemini review asked for — extends the conservative clamp to inverseSurface / inverseOnSurface, explains the dark-on-dark Snackbar trap in the KDoc, pins the composition with a contract test, and extends the unit tests. The fix can land; the duplicate test and the partial-enumeration KDoc paragraph are optional cleanup.

Files Reviewed (3 files in this increment)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkScheme.kt — KDoc extended, two new .copy(...) lines. Correct; one KDoc polish note.
• shared/src/commonTest/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkSchemeTest.kt — fixture, KDoc, and 2 tests extended. Correct; one test-shrink note (duplicated coverage).
• shared/src/androidHostTest/kotlin/com/devil/phoenixproject/ui/theme/MaterialYouDarkClampContractTest.kt — new contract test pins the inverse-surface composition. Clean.

Fix these issues in Kilo Cloud

Previous review (commit 478cc6f)

Verdict: Can Merge | Recommendation: Previous warning resolved; one Ponytail cleanup note.

Overview

Severity	Count
🚨 critical	0
⚠️ warning	0
💡 suggestion	0
🤏 nitpick	0

No new correctness or safety findings on changed code.

---

🏆 Best part: The KDoc on ClampedDynamicDarkScheme.kt:21-35 is now the most honest paragraph in this PR — it owns the conservative clamp, explains why surfaceVariant / background / surfaceDim / surfaceBright are pulled from fallback instead of dynamic, and explicitly enumerates which 6 accents keep the wallpaper hue. The liar got fired. The new regression test (conservative clamp routes...) pins this behavior so a future refactor can't quietly drift it back. That's a clean fix-it cycle.

💀 Worst part: The new regression test is doing the same job twice. The first test (clamped surface family equals the static dark fallback surface) already asserts surfaceVariant / onSurfaceVariant / surfaceDim / surfaceBright / background / onBackground at lines 115-125 — the new test at line 128 re-asserts the same six tokens with longer error messages. Either fold the new test's precondition into the existing one, or delete the new test entirely. As written, you've earned ~35 lines of duplicate coverage and a maintenance line item the next reviewer will skim past.

📊 Overall: The increment does exactly what the previous review asked for — reconciles docs, code, and a regression test, all on the dark-branch only. The fix can land; one optional collapse will keep the test file from sprawling.

Files Reviewed (2 changed in this increment)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkScheme.kt — KDoc reconciled; code unchanged. Clean.
• shared/src/commonTest/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkSchemeTest.kt — new regression test added at line 128. Redundant with existing assertions (see Ponytail).

---

Correctness / Safety Findings

No correctness or safety findings on changed code.

Ponytail Review

• shared/src/commonTest/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkSchemeTest.kt:128: test-shrink — conservative clamp routes surfaceVariant background surfaceDim surfaceBright from fallback re-asserts the same 6 tokens already pinned at lines 115-118, 124-125 by clamped surface family equals the static dark fallback surface. Fold the precondition check (lines 143-149) into the first test, or delete the new test entirely. The first test's assertion list is already exhaustive.

Ponytail net: ~35 lines removable.

Suggested Minimal Patch

1. ClampedDynamicDarkSchemeTest.kt: Either delete the new conservative clamp routes... test (lines 128-161) — its 6 surface-family assertions are already covered by clamped surface family equals the static dark fallback surface — or fold its precondition check into the existing test and keep the redundant assertions as a defense-in-depth comment. Prefer deletion.

Final Merge Guidance

Can merge. The previous warning is properly addressed (KDoc reconciled with the conservative implementation, regression test added — redundantly). The Ponytail note is optional cleanup; it does not block merge.

Fix these issues in Kilo Cloud

Previous review (commit cc5beb1)

Verdict: 3 Issues Found | Recommendation: Address the warning before merge; suggestions are optional cleanup.

Overview

Severity	Count
🚨 critical	0
⚠️ warning	1
💡 suggestion	2
🤏 nitpick	0

Issue Details (click to expand)

File	Line	Roast
shared/.../ClampedDynamicDarkScheme.kt	45	Acceptance criteria promise 12 tokens come from dynamic; code routes 6 of those to fallback (incl. surfaceVariant, background, surfaceDim/Bright, onBackground, onSurfaceVariant). KDoc on lines 24-26 backs the wrong story.
shared/.../RoutinesChromeLuminanceContractTest.kt	49	source.contains("containerColor = MaterialTheme.colorScheme.surface,") matches one exact phrasing — trailing comma and all. Reformat the line and 113 lines of "contract tests" explode.
shared/.../RoutinesChromeLuminanceContractTest.kt	102	noHardcodedSurfaceColorInChrome only catches the literal containerColor = Color.White. Color(0xFFFFFFFF) slips through and reintroduces the bug.

---

🏆 Best part: The actual fix in Theme.kt is a 9-line, two-branch when with one clamp call. Minimal, readable, doesn't touch the static DarkColorScheme, and the unit tests around the helper (ClampedDynamicDarkSchemeTest) genuinely exercise the merge semantics — boundary luminance at 0 / 0.49 / 0.5 / 0.51 / 1.0, no-mutation, accent preservation, ≤ 6% adjacent-shift invariant. That's a real test suite, not a vibes-based one.

💀 Worst part: The acceptance criteria in the PR description promise wallpaper hue on surfaceVariant / surfaceDim / surfaceBright / background / onBackground / onSurfaceVariant, but the implementation clamps all 6 to fallback. Either the criteria or the code is wrong, and right now the unit test agrees with the code — so the PR description is the liar. A reviewer skimming the description will sign off on behavior the code doesn't actually deliver.

📊 Overall: The fix is correct and the helper is well-scoped, but the PR is selling wallpaper hue it isn't shipping. Reconcile the criteria, the KDoc, and the code — and trim the brittle source-grep contract tests before they become a maintenance tax the next reviewer has to pay.

Files Reviewed (6 files)

• shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkScheme.kt — 1 issue
• shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/Theme.kt — clean
• shared/src/commonTest/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkSchemeTest.kt — clean (solid coverage)
• shared/src/androidHostTest/kotlin/com/devil/phoenixproject/ui/theme/MaterialYouDarkClampContractTest.kt — covered by the brittle-string finding above
• shared/src/androidHostTest/kotlin/com/devil/phoenixproject/presentation/theme/RoutinesChromeLuminanceContractTest.kt — 2 issues
• shared/src/androidHostTest/kotlin/com/devil/phoenixproject/presentation/theme/RoutineCardElevationContractTest.kt — covered by the brittle-string finding above

---

Correctness / Safety Findings

• warning: shared/src/commonMain/kotlin/com/devil/phoenixproject/ui/theme/ClampedDynamicDarkScheme.kt:45 — Acceptance criteria promise 12 dynamic-sourced tokens; code sources only 6. Reconcile criteria + KDoc (lines 24-26) with the imp

[Snapshot truncated.]

Additional previous summary content was truncated to keep this comment within platform limits.

---

_{Reviewed by minimax-m3 · Input: 33.1K · Output: 13.1K · Cached: 198.8K}

[9thLevelSoftware]

Thanks for the thorough review — addressing the warning on ClampedDynamicDarkScheme.kt:45.

You were right: the code correctly routes surfaceVariant / onSurfaceVariant / surfaceDim / surfaceBright / background / onBackground to fallback, but the KDoc and PR body promised wallpaper hue on those tokens. Reconciled in commit 478cc6f:

• ClampedDynamicDarkScheme.kt KDoc rewritten to explicitly justify the conservative clamp: the surface family is clamped as a unit because wallpaper-miscalibration can hit any of those tokens. Only the user-interactive accents (primary / secondary / tertiary / error / outline / outlineVariant) flow through from dynamic.
• ClampedDynamicDarkSchemeTest.kt now has a regression test conservative clamp routes surfaceVariant background surfaceDim surfaceBright from fallback that pins the implementation so future drift is caught.
• PR body acceptance criteria updated to call out which tokens come from dynamic (accents) vs fallback (surface family as a unit).

The code itself was correct and matches the RCA's rca_fix_direction; the liar was the doc. My fault for skimping on the doc.

On the two suggestions (brittle source-grep contract tests + narrow Color.White check): acknowledging but deferring. The source-grep pattern matches the project's existing convention (ThemeModeUiContractGuardTest, DesignSystemRatchetTest) and the brittleness is the price of avoiding a Compose UI-test dep the project hasn't wired up. Happy to follow up in a separate PR if you'd prefer a different convention.

CI will re-run automatically with the new commit (force-push avoided — just a follow-up commit on the same branch).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 39a3a35e15

ℹ️ 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".