From 6305d86a01a2344cb2cd7b3496a41d7e6dab27c2 Mon Sep 17 00:00:00 2001 From: wtclaude Date: Sat, 8 Aug 2026 11:02:46 -0500 Subject: [PATCH] docs(android): M12 phase 8 as landed, and the two defects AC-5 found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 8 was scoped as docs, coverage and the cutover. The AC-5 walk turned it into a phase with code in it, which is the outcome the walk existed to produce - so the record is longer than a phase-8 note would normally be. THEMING_AND_NAV.md gains "Phase 8 as landed": what the walk confirmed, the two defects it found and why they are phase 2's trap repeating, the role sweep that says which Material roles are still unmapped and which of them have a reader, the coverage-glob narrowing, and the adb-reverse rig note for the next walk. The phase 2 note is amended where it claimed the card shadow was the milestone's only visible change to an untouched instance - it is now one of three, and the third (an untouched instance drawing a hero, because brand.hero carries the env default) is a correction to the record with no code behind it. PLAN.md's M12 entry stopped at phase 2; it now runs through 6, records phase 8 including the defects, and drops the same "one deliberate change" claim. COVERAGE_PLAN.md gains an amendment section explaining why the ui/theme/** directory glob became wrong the moment M12 put three pure resolvers behind it, and the general rule it suggests: prefer file globs to directory globs once a directory is mixed. Its own §5 already forbade excluding testable code. Pairs with Android-app phase 8. Co-Authored-By: Claude --- android/COVERAGE_PLAN.md | 25 +++++++++ android/PLAN.md | 39 ++++++++++++- android/THEMING_AND_NAV.md | 111 ++++++++++++++++++++++++++++++++++++- 3 files changed, 170 insertions(+), 5 deletions(-) diff --git a/android/COVERAGE_PLAN.md b/android/COVERAGE_PLAN.md index 0b4ab51..ce4afd2 100644 --- a/android/COVERAGE_PLAN.md +++ b/android/COVERAGE_PLAN.md @@ -202,3 +202,28 @@ class LoginViewModelTest { - `MainDispatcherRule` + a documented ViewModel test pattern exist and are reused. - Coverage exclusions list only genuinely non-unit-testable files (UI composables, Android-framework glue) — no ViewModel, repository, DTO, or pure core-logic file is excluded. + +## 6. Amendment (2026-08-08, M12 phase 8): `ui/theme/**` narrowed to one file + +**A directory glob in the exclusion list went stale as soon as another milestone put testable code +in that directory.** §2 phase 0 excluded `app/src/main/java/**/ui/theme/**` because at the time the +directory held `Color.kt`, `Type.kt` and the composables — constants and composable bodies, nothing +a JVM test could execute. M12 then added three *pure* resolvers to it: `ShardPalette`, +`ShardStructure` and `ShardTypeface`, which exist precisely so the theming milestone's no-op +invariants could be plain JVM assertions. JaCoCo measures them at **98%, 100% and 100%**, and the +glob was discarding every line. + +The exclusion is now the single file it was really about, `ui/theme/Theme.kt` (the composable, 52%). +Everything else in `ui/theme/` is measured and all of it covers at 93% or better. + +Two things worth carrying forward: + +- **This did not rescue the gate and was not meant to.** M12's already-measured code + (`data/appearance/` at 100%, `ui/navigation/` at 93–100%) clears `new_coverage ≥ 50` on its own. + The point is that a future change deleting those resolvers' tests would now move the number, where + before it would not have — the exclusion was hiding well-tested code, which is the opposite of what + §1 built the list for and what §5's third bullet asks for. +- **Prefer file globs to directory globs when a directory is mixed.** `ui/components/**` stays a + directory glob and correctly so: `BrandAssets.kt` sits at 11% because only `brandAssetUrl` is pure, + and the rest is composable bodies. The distinction is whether the directory is *uniformly* + untestable, not whether it is under `ui/`. diff --git a/android/PLAN.md b/android/PLAN.md index 4590373..e59689e 100644 --- a/android/PLAN.md +++ b/android/PLAN.md @@ -1048,9 +1048,42 @@ push, and Play (M6–M8) follow the designed app. the shipped palette reproduces the pre-M12 `ColorScheme` role for role. **Phase 2 landed 2026-08-08** (Android-app#35 / docs#114): the four radii scale the app's own dp by their ratio to the `runic-gateway` baseline, and `--shadow-card` maps onto card elevation. The shadow is - the milestone's **one deliberate change to an untouched instance** — the app has been flat - since M5 while the preset it was drawn from selects a shadow — and is why every `Card(` became - a `ShardCard(`: Material takes elevation as a default argument, not from the theme. + a **deliberate change to an untouched instance** — the app has been flat since M5 while the + preset it was drawn from selects a shadow — and is why every `Card(` became a `ShardCard(`: + Material takes elevation as a default argument, not from the theme. **Phase 3 landed + 2026-08-08** (Android-app#36 / docs#115): seven bundled families beside Cinzel, resolved from + the stacks the theme publishes. It **nearly tripled the APK** — 4.80 → 12.94 MiB against a + drafted 1.5–2.5 MB estimate, with Merriweather alone 6.08 MiB of the growth because upstream + ships it as a barely-compressible three-axis variable font; verbatim bundling was chosen with + the cheaper options costed and declined. **Phase 4 landed 2026-08-08** (Android-app#37 / + docs#116): logo in the drawer header and top bar, hero on Home, no new dependency and no APK + cost. **Phase 5 landed 2026-08-08** (Android-app#38 / docs#117): the public nav's label, order + and hiding, keyed by the path→route table — whose sort keys must be indices into the + *website's* sixteen-row nav, not the app's nine. **Phase 6 landed 2026-08-08** + (Android-app#39 / docs#118): the drawer gains admin-authored sections and added links, a link + the app can open natively doing so and one it cannot handing off to a Custom Tab. + + **Phase 8 landed 2026-08-08** (Android-app#40 / docs#119) and did more than its name. Scoped + as docs, coverage and the cutover, its **AC-5 on-device walk** — two AVD passes against a local + website, the themed one at every role rung — confirmed everything phases 1–6 had deferred to it + (including that a Custom Tab really opens, checked in `dumpsys`, and that an anonymous caller + sees the reordered, sectioned nav with no player or staff row leaking) **and found two theming + defects that 476 passing tests could not see**: + + - every `ShardCard` — 26 sites in 20 files — drew in Material's grey rather than the shard's + panel colour, because `CardDefaults.cardColors()` takes its container from + `surfaceContainerHighest` and the mapping had `surfaceContainer`, `…High` and `…Low` but not + `…Highest`; and + - the drawer's selected row ignored `--radius-pill`, because `NavigationDrawerItem` takes + `shape` as a default argument. + + Both are **phase 2's trap repeating**: Material takes these as default arguments, not from the + theme. Phase 2 found it for depth and swept the call sites; nobody asked whether colour and + shape had the same problem. The card fix also **changes the untouched app** — those grey cards + predate M12 and go back to M5 — so the milestone ships two deliberate changes to a shard that + has set nothing, the shadow and the card container, with AC-1 rewritten to record the second + rather than absorb it. Phase 8 also narrowed a stale `ui/theme/**` Sonar coverage glob that was + discarding three 98–100%-covered resolvers. **477 tests green.** ### Deferred (not a milestone) diff --git a/android/THEMING_AND_NAV.md b/android/THEMING_AND_NAV.md index e7ead72..756f786 100644 --- a/android/THEMING_AND_NAV.md +++ b/android/THEMING_AND_NAV.md @@ -665,7 +665,7 @@ PR against `main`. | **5** ✅ | **Public nav: label / order / hidden** | The path→route table (§6.2), `Routes.news(category)`, the merge, drawer wiring. AC-3 | | **6** ✅ | **Public nav: sections & added links** | Drawer groups, `pruneNav` port, link path validation, native-route resolution + Custom Tab fallback (§6.3) | | **7** | **Authenticated navs** | ❌ **cancelled** — was: `GET /api/v1/settings/nav` behind a session-keyed repository; label/hidden for the four mapped rows (§6.4). See "Phase 7, cancelled" below | -| **8** | **Docs, coverage & cutover** | This doc's "as landed" notes and any amendments the build forces, the `PLAN.md` §9 M12 entry refreshed, Sonar coverage for the new modules, AC-5 on-device walk, then `edge` → `main` | +| **8** ✅ | **Docs, coverage & cutover** | This doc's "as landed" notes and any amendments the build forces, the `PLAN.md` §9 M12 entry refreshed, Sonar coverage for the new modules, AC-5 on-device walk, then `edge` → `main`. **The walk found two theming defects and phase 8 fixed them** — see "Phase 8 as landed" | Phase 0 is the one with a hard rule attached: **it must change nothing on screen.** Everything after it is additive on top of a store that is already @@ -762,7 +762,12 @@ proven not to have moved anything. ### Phase 2 as landed -- **The app is flat, so §5.4 is the milestone's one visible default change.** +- **The app is flat, so §5.4 is a visible default change.** (It was written here + as the milestone's *only* one. Phase 8 found a second — the card *container* + colour — and a third that is not a change at all but a correction to this + record: an instance with a `BRAND_HERO_IMAGE` and no `brand_assets` row now + draws a hero on Home, because `brand.hero` carries the env default. See + "Phase 8 as landed".) Material3 1.3.0's filled `Card` is `ElevationTokens.Level0` — 0dp, checked in the artifact's bytecode, not assumed — and `FeatureCard` was a `Box` with a clip, a gradient and a border, drawing none of the "soft shadow" its own KDoc @@ -1022,6 +1027,108 @@ Consequences, all of them wanted: If it is ever picked up, §6.4 is the spec and the merge would reuse phase 5's. +### Phase 8 as landed + +Phase 8 was scoped as docs, coverage and the cutover. The AC-5 walk turned it +into a phase with code in it, which is the outcome the walk existed to produce. + +**AC-5 confirmed what the JVM could not reach.** Two passes on the AVD against a +local website, the themed one walked at every role rung. Everything phases 1–6 +had deferred to it held: the Fantasy palette on screen; radii visibly square +against the untouched build's rounded; **EB Garamond arriving in the *sans* role**, +which is phase 3's preset-bypass finding proved on a device rather than in a test; +the logo replacing the top bar's text with the 6× width cap holding; the hero +centre-cropped into its 180dp band with the top and bottom of a 4:3 image cut; +reorder, relabel and hide; the section header with its indented children; the +added link with a native route carrying **no** badge and the handing-off one +carrying it; and the Custom Tab genuinely opening — confirmed as +`CustomTabActivity` in `dumpsys activity`, not by eye. AC-3's role dimension held +at every rung: an anonymous caller saw the reordered, sectioned public nav with +**no player or staff row leaking**, and the player rung stopped at `My houses`. +AC-4 fell out for free when the second AVD started against an unreachable server: +shipped theme, coded menu, one retry, no error surface. + +**A correction the walk forced, with no code behind it.** An untouched instance +**gains a hero on Home**. `brand.hero` carries the `BRAND_HERO_IMAGE` env default +— `/assets/img/runic-emblem.png` on this one — so "admin has set nothing" does not +mean "the slot is empty", and phase 4's `BrandHero` draws it. Nothing is wrong +here: the website's own Home shows the same emblem, and §5.6's rule is that a +blank slot draws nothing, which still holds. What was wrong is the *record* — +phase 2 claimed the card shadow was the milestone's only visible change to an +untouched app, and this was the second before the card container made it three. +Worth stating plainly because a shard operator upgrading the app sees it without +having touched Appearance at all. + +**Two defects, one root cause, and it is phase 2's.** Material takes some themed +values as **default arguments** rather than from the theme. Phase 2 found this for +card elevation and answered it by sweeping every `Card(` into `ShardCard(`. Nobody +then asked whether *colour* and *shape* had the same problem. They did: + +- **Every `ShardCard` drew in Material's grey, not the shard's panel colour.** + `CardDefaults.cardColors()` takes its container from `surfaceContainerHighest` + — `FilledCardTokens.ContainerColor`, checked in the material3 1.3.0 artifact's + bytecode the same way phases 1 and 2 checked theirs — and `shardColorScheme` + mapped `surfaceContainer`, `…High` and `…Low` but not `…Highest`. All 26 call + sites in 20 files. `FeatureCard` was unaffected because it reads + `LocalShardPalette` directly, which is exactly why Home looked right and every + other screen did not — the single most misleading thing about the bug. +- **The drawer's selected row ignored `--radius-pill`.** `NavigationDrawerItem` + takes `shape` as a default argument (`CircleShape`); the three call sites set + `colors` and never `shape`. On Fantasy every other radius went square while the + selected row stayed a full pill. + +**The card fix changes the untouched app, and that is the honest framing.** The +grey cards are not an M12 regression — an untouched instance draws them too, and +has since M5. M12 only made it visible by theming everything around them. So +fixing it moves the shipped app's cards from Material's grey to `--panel-flat`: +**the milestone's second deliberate change to a shard that has set nothing**, +alongside phase 2's card shadow. §9's last bullet had already called this shot — +"if a screen looks wrong under a warm preset, that is a token the screen should +have been reading and did not" — and this is that, at 26 call sites. + +There is no version of the fix that spares untouched instances: the shipped +palette *is* the `runic-gateway` preset, so the value the cards should take is the +same value either way. AC-1 therefore **records** the change instead of absorbing +it — every other role is still asserted byte-for-byte against the verbatim pre-M12 +scheme, and the two that moved are named, given their new values, and checked to +have genuinely differed before, so the test cannot pass by coincidence. + +**The role sweep, so the next person does not redo it.** Of the roles +`shardColorScheme` leaves at `darkColorScheme()`'s defaults, exactly one had a +live reader in this app: `surfaceContainerHighest`. Checked against the token +class of every Material component the app actually draws — +`NavigationDrawerTokens`, `SheetBottomTokens`, `AssistChipTokens`, +`FilterChipTokens`, `TopAppBarSmallTokens`, `BadgeTokens` — all of which read only +roles already mapped. `scrim` is read by the modal drawer and **stays Material's +black on purpose**; a tinted scrim is a design change, not a gap. +`surfaceContainerLowest` is mapped for consistency with `…Low` and has no reader. +`primaryContainer`, `tertiaryContainer`, `inverse*`, `surfaceBright`, `surfaceDim` +and `surfaceTint` are latent — the app draws no Snackbar, tonal button, elevated +or outlined card. Adding one of those components means checking this list again. + +**Coverage: the exclusion list had gone stale under the milestone.** +`sonar.coverage.exclusions` carried a `ui/theme/**` directory glob from the M11 +coverage push, when that directory held only constants and composables. M12 put +three pure resolvers in it — `ShardPalette`, `ShardStructure`, `ShardTypeface`, +the reason phases 1–3 could prove their no-op invariants as JVM assertions — and +the glob was dropping all three out of the denominator at 98%, 100% and 100%. It +is now the one file it was really about, `ui/theme/Theme.kt`. This did not rescue +the gate: M12's already-measured code (`data/appearance/` and `ui/navigation/`, +93–100%) clears `new_coverage ≥ 50` on its own. It makes the number honest about +which code the tests hold, so deleting those tests would move it. + +**Tests: 477 green** (476 + the card-container assertion), `lintDebug` and +`assembleDebug` clean. The JVM limit from phase 4 is unchanged and is why this +phase needed a device at all: no Robolectric, no `androidTest` source set, so a +composable body cannot run in a unit test. Both defects were invisible to 476 +passing tests and obvious within two minutes of looking at a themed screen. + +**Rig notes, for the next walk.** The emulator reaches a local website through +`adb reverse tcp:3000 tcp:3000`, which lets the app use `http://127.0.0.1:3000` +and the debug `network_security_config`'s loopback exception **unchanged** — do +not add `10.0.2.2` to it for a test rig. The Custom Tab resolves against the same +base URL, so hand-off links work under the same tunnel. + ## 9. Out of scope - **Light mode / a light preset.** The website cancelled its Parchment phase; the -- 2.49.1